Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial patch for RAS configuration settings #99

Open
wants to merge 3 commits into
base: integ_sp7
Choose a base branch
from

Conversation

abinayaddhandapani
Copy link
Collaborator

@abinayaddhandapani abinayaddhandapani commented Nov 29, 2024

- Added README.md with project overview.
- Added OWNERS file to define project ownership and contributors.
- Added LICENSE file to specify project licensing terms.
- Added clang-18 formatter and prettier

Interface:
com.amd.RAS.Configuration interface - - -
.GetAttribute method s v -
.SetAttribute method sv - -
.RasConfigTable property a{s(ssvx)} 9 "AifsArmed" "com.amd.RAS.Configurat... emits-change writable
org.freedesktop.DBus.Introspectable interface - - -
.Introspect method - s -
org.freedesktop.DBus.Peer interface - - -
.GetMachineId method - s -
.Ping method - - -
org.freedesktop.DBus.Properties interface - - -
.Get method ss v -
.GetAll method s a{sv} -
.Set method ssv - -
.PropertiesChanged signal sa{sv}as - -

Tested:

  1. Service is working well.
  2. All the dbus methods and properties are shown correctly.
  3. Unit test done.

root@congo-d7a4:~# systemctl status com.amd.RAS

  • com.amd.RAS.service - Crash dump manager
    Loaded: loaded (/usr/lib/systemd/system/com.amd.RAS.service; enabled; preset: enabled)
    Active: active (running) since Fri 2024-11-29 08:41:10 UTC; 14min ago
    Main PID: 1993 (amd-bmc-ras)
    CPU: 29ms
    CGroup: /system.slice/com.amd.RAS.service
    `-1993 /usr/bin/amd-bmc-ras

Nov 29 08:41:10 congo-d7a4 systemd[1]: Started Crash dump manager.
Nov 29 08:41:45 congo-d7a4 amd-bmc-ras[1993]: Attribute updated successfully

root@congo-d7a4:# busctl call com.amd.RAS /com/amd/RAS com.amd.RAS.Configuration SetAttribute sv AifsArmed b true
root@congo-d7a4:
# busctl get-property com.amd.RAS /com/amd/RAS com.amd.RAS.Configuration RasConfigTable
a{s(ssvx)} 9 "AifsArmed" "com.amd.RAS.Configuration.AttributeType.Boolean" "If this field is true, AIFS flow is triggered" b true 0 "AifsSignatureId" "com.amd.RAS.Configuration.AttributeType.Boolean" "List of signature Id to check if Aifs is triggered" a{ss} 1 "EX-WDT" "0xaea0000000000108000500b020009a00000000004d000000" 0 "ApmlRetries" "com.amd.RAS.Configuration.AttributeType.Boolean" "Number of APML retry count" x 10 0 "DisableAifsResetOnSyncfloodCounter" "com.amd.RAS.Configuration.AttributeType.Boolean" "Disable AIFS Reset on syncfloow counter " b true 0 "HarvestMicrocode" "com.amd.RAS.Configuration.AttributeType.Boolean" "Harvest microcode version" b true 0 "HarvestPPIN" "com.amd.RAS.Configuration.AttributeType.Boolean" "Harvest PPIN" b true 0 "ResetSignal" "com.amd.RAS.Configuration.AttributeType.Boolean" "Reset Signal Type" s "SYS_RST" 0 "SigIdOffset" "com.amd.RAS.Configuration.AttributeType.Boolean" "List of Signature ID offsets" as 8 "0x30" "0x34" "0x28" "0x2c" "0x08" "0x0c" "null" "null" 0 "SystemRecovery" "com.amd.RAS.Configuration.AttributeType.Boolean" "System recovery mode" s "WARM_RESET" 0

@abinayaddhandapani abinayaddhandapani changed the title Add README, OWNERS, LICENSE , clang-format , prettier files Initial patch for RAS configuration settings Nov 29, 2024
@abinayaddhandapani abinayaddhandapani force-pushed the review branch 3 times, most recently from b48f6a2 to 980f9a6 Compare November 29, 2024 10:28
@ojayanth
Copy link

ojayanth commented Dec 1, 2024

@abinayaddhandapani Can you please add your response PR #98 that will help to avoid duplicate comments here.


#include <fstream>

namespace ras_config
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guidlines:
https://github.com/CodeConstruct/openbmc-docs/blob/master/cpp-style-and-conventions.md#namespaces

Using these conventions helps maintain a clear and organized codebase, making it easier for developers to navigate and understand the structure. some thing like

Example:

namespace amd
{
namespace ras
{
configuration
{
class manager : public Base
}}}

amd::ras::configuration:;manager
amd::ras::interface::manager etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

* parameters in the D-Bus interface.
*
*/
class RasConfigManager : public Base
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be just Manager , namespace will guide the role of this manager.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

meson.build Outdated
]

executable(
'amd-bmc-ras',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amd-ras-manager ?
Service description says crash-dump-manager.
Be consistent of naming definitions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Will rename to amd-ras-manager everywhere.
Updated in service file as well.

}
},
{
"SystemRecovery": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better to include valid mode's here
"SystemRecovery": {
"Description": "System recovery mode",
"Value": "NO_RESET"
},
"ValidModes": [
{
"Mode": "Mode 1",
"Description": "xx"
},
{
"Mode": "mode 2",
"Description": "xxxx"
},

]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add "ValidOptions" field to verify if the given value is correct.
{
"SystemRecovery": {
"Description": "System recovery mode",
"Value": "NO_RESET",
"ValidOptions": ["COLD_RESET","WARM_RESET"]
}
},

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cahnge name to meaningful like SysRecoveryMode ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

"Value": "NO_RESET"
}
},
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to group Harvesting tasks ? osme thing like
{
"HarvestingTasks": {
"HarvestMicrocode": {
"Description": "Harvest microcode version",
"Value": true
},
"HarvestPPIN": {
"Description": "Harvest PPIN",
"Value": true
}
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not combining the options so the code will be generic for all the attributes requested.

}
catch (const std::exception& e)
{
lg2::error("Error : {ERROR}", "ERROR", e.what());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dbus error ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already added dbus errors for all the errrors in the try block. Throwing exception from the catch blocks will crash the service. So adding dbus error is not needed here.

if (!fs::exists(configFile)) // Check if the config file exists
{
std::string copyCommand =
std::string("cp ") + "/usr/share/ras-config/ras_config.json" + " " +
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you keep json file defition at Name space scope or Meson? that wilkl help users to modify path if required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

std::string("cp ") + "/usr/share/ras-config/ras_config.json" + " " +
configFile;

int result = system(copyCommand.c_str());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not advised in modern c++,

if (!fs::exists(configFile)) {
try {
fs::copy(defaultConfigPath, configFile);
} catch (const fs::filesystem_error& e) {
<
return;
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

}

std::ifstream jsonRead(configFile);
nlohmann::json data = nlohmann::json::parse(jsonRead);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add error check if (!jsonRead.is_open()) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack


ConfigTable configMap;

for (const auto& item : data["Configuration"])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using this algorithm( not tested) , which simplify

for (const auto& item : data["Configuration"]) {
if (item.is_object() && item.size() == 1) {
const auto& key = item.begin().key();
const auto& obj = item[key];

        std::string description = obj["Description"];
        AttributeValue value;
        int64_t maxBoundValue = 0;

        if (obj["Value"].is_boolean()) {
            value = obj["Value"].get<bool>();
        } else if (obj["Value"].is_string()) {
            value = obj["Value"].get<std::string>();
        } else if (obj["Value"].is_number_integer()) {
            value = obj["Value"].get<int64_t>();
        } else if (obj["Value"].is_array()) {
            value = obj["Value"].get<std::vector<std::string>>();
        } else if (obj["Value"].is_object()) {
            value = obj["Value"].get<std::map<std::string, std::string>>();
        }

        int attributeType = value.index();
        configMap[key] = std::make_tuple(attributeType, description, value, maxBoundValue);
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to construct rasConfigTable which contains all the datas of the json file.
Since name , description , value all are different types we need to parse it individually and create the tuple.
Else the table is not getting generated as expected

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share the sdbusplus::common::com::amd::ras::
Configuration::AttributeType::xx defeitions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enumerations:
- name: AttributeType
description: >
Attribute Type.
values:
- name: Boolean
description: >
Boolean value Type
- name: String
description: >
string value Type.
- name: Integer
description: >
unsigned integer value Type.
- name: ArrayOfStrings
description: >
Array of strings value Type.
- name: KeyValueMap
description: >
key-value pairs value Type.

README.md Outdated
# AMD BMC RAS
# AMD BMC RAS

The amd - bmc - ras service is intended to discover, configure and exercise OOB
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Out Of Band (OOB)
Reliability Availability and Serviceability (RAS)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add apml documentation public link)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing .

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already added the comments for this review to the chat
#99 (comment)

README.md Outdated
# AMD BMC RAS

The amd - bmc - ras service is intended to discover, configure and exercise OOB
RAS capabilities supported by the processors .The application creates error
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AMD processors. (no blank between the last word and the full stop). The amd-bmc-ras service (instead of the word application)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

README.md Outdated

The amd - bmc - ras service is intended to discover, configure and exercise OOB
RAS capabilities supported by the processors .The application creates error
records from RAS telemetry extracted from the processor over APML.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APML - first time usage, please elaborate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

README.md Outdated
## Features

The application waits on the APML_L gpio pin to check if any events are
detected. When a fatal error is detected in the system , SMU responds to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no space between the word and comma.
SMU first time use

Copy link

@ojayanth ojayanth Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing

Ack

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

register via APML to confirm an MCA error has caused the ALERT_L assertion. The
application collects the MCA / MSR dump via APML and creates CPER record. System
recovery is handled as per the user's preference from the config file.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See if the following paragraph explains the flow:
The amd-bmc-ras service reads the SBRMI registers over the APML upon the ALERT_L assertion by the SMU. It creates CPER records based on the the MCA/MSR dump. <what happens to the just created CPER record? who are the end users of it?>

Explain the System recovery feature in the next paragraph and how it is related to the just created CPER record? Then describe how the user can configure the recovery action, give a pointer where is is explained. for e.g., configuration section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

README.md Outdated

## Configuration

amd-ras is configured per the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean amd-bmc-ras service?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how configuration of the service is linked with meson_options.txt? May be you want to mention what all can be configured briefly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meson_options.txt is not added for the initial commit. But the options.txt file will be used to get the compiler flag is the underlying protocol is APML or PLDM to harvest the errors

## Configuration

amd-ras is configured per the
[meson build files](https://mesonbuild.com/Build-options.html). Available
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link is inactive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link will be active when it is clicked from the repo https://github.com/AMDESE/amd-bmc-ras/blob/review/README.md and not from the PR link

Copy link

@ojayanth ojayanth Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like name an d link is not matching , please look.

Reply : The upstream uses the same name and the link to point out the build-options file.
Attaching link for reference:
https://github.com/openbmc/bmcweb/blob/deae6a789444debc4724fb6902fc5def299afbee/README.md?plain=1#L52

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the upstream repo such as bmcweb uses the same naming convention.

Please find the link for reference.

https://github.com/openbmc/bmcweb/blob/deae6a789444debc4724fb6902fc5def299afbee/README.md?plain=1#L52

@abinayaddhandapani abinayaddhandapani force-pushed the review branch 2 times, most recently from 8b07659 to d46cde6 Compare December 5, 2024 05:35
{
namespace ras
{
namespace configuration
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configuration-> config

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

static constexpr auto service = "com.amd.RAS";
static constexpr auto objectPath = "/com/amd/RAS";

using Base = sdbusplus::com::amd::RAS::server::Configuration;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General: Use Meningfull names , instead of Random
"Base" -- Not able to connect namespace definition , ConfigType?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

README.md Outdated
@@ -1,20 +1,25 @@
# AMD BMC RAS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message rule 50/72.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit3 - Commit message more details based on the covergae added , some thing like ( elaborate this with details)

Enable RAS Configuration Manager with D-Bus Interface

  • Implemented RAS Configuration Manager based on a configuration JSON file.
  • Added functionality to override configuration parameters via D-Bus interface.
  • Established D-Bus infrastructure to support RAS configuration management.

Tested:

  1. Verified service functionality.
  2. Confirmed all D-Bus methods and properties are displayed correctly.
  3. Conducted unit tests to ensure reliability.

"SystemRecovery": {
"Description": "System recovery mode",
"Value": "NO_RESET",
"ValidOptions": ["COLD_RESET", "WARM_RESET"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO_RESET is also valid option right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. I will add NO_RESET to the validoptions.

},
{
"HarvestPPIN": {
"Description": "Harvest PPIN",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harvest Protected Processor Identification Number (PPIN)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

jsonFileOut.close();
}

/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this header .

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

jsonRead.close();
}

/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

fs::path destDir = fs::path(CONFIG_FILE).parent_path();
if (!fs::exists(destDir))
{
fs::create_directories(destDir);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to handle exceptions throw here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack


if (!jsonRead.is_open())
{
lg2::error("Error: Could not read config file");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should inform user. to act on usecase based error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throw exception if the config file is inaccessible


ConfigTable configMap;

for (const auto& item : data["Configuration"])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share the sdbusplus::common::com::amd::ras::
Configuration::AttributeType::xx defeitions?

# It is expected that these 4 sections will be listed in the order above and
# data within them will be kept sorted.

owners:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match with all required details , incase you want to fill it now . Otherwise leave you can update details one we started upstreaming activity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

README.md Outdated
# AMD BMC RAS
# AMD BMC RAS

The amd - bmc - ras service is intended to discover, configure and exercise OOB
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add apml documentation public link)


amd-ras is configured per the
[meson build files](https://mesonbuild.com/Build-options.html). Available
options are documented in `meson_options.txt`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add : Building section.
Eg:
This project uses Meson (>=1.1.1). To build for native architecture, simply run:

meson setup build
ninja -C build

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing

README.md Outdated
@@ -1,20 +1,25 @@
# AMD BMC RAS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit3 - Commit message more details based on the covergae added , some thing like ( elaborate this with details)

Enable RAS Configuration Manager with D-Bus Interface

  • Implemented RAS Configuration Manager based on a configuration JSON file.
  • Added functionality to override configuration parameters via D-Bus interface.
  • Established D-Bus infrastructure to support RAS configuration management.

Tested:

  1. Verified service functionality.
  2. Confirmed all D-Bus methods and properties are displayed correctly.
  3. Conducted unit tests to ensure reliability.

* @brief Manager class which adds the RAS configuration
* parameter values to the D-Bus interface.
*
* The class pulls the default values of ras_config.json file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add @details tag , before detiled description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

meson.build Outdated
]

deps = [
dependency('boost'),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing from previous reveiw

'-DSRC_CONFIG_FILE="' + src_config_file + '"'
]

deps = [
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually boost get unnecessary overheads on binary size and other issues . please look bios-settings-mg or other repos, to set required boost build flags setting , and add relevant here , based on your use cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack


executable(
'amd-ras-manager',
'src/config_manager.cpp',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list will grow . please start grouping src files , refer any of the upstream meson file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

*/
void Manager::setAttribute(AttributeName attribute, AttributeValue value)
{
nlohmann::json j;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comments from previous review.

auto configMap = rasConfigTable();

std::ifstream jsonFile(CONFIG_FILE);
if (!jsonFile.is_open())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two exceptions consecutively, which is not valid. You should throw only one exception.
Looks like you are missing to address error handling , as we diecussed.
I will stoip reveiw here , will continue reveiw after you revisted error strategy in better way , as diecusssed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throwing D-bus error should be fine. I will remove the runtime_error() exception

Copy link

@ojayanth ojayanth Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like d-bus errors are Not documented in dbus review. Please update.

Reply : I have sent for the internal gerrit review https://gerrit-git.amd.com/c/ESE/ec/BMC/openbmc/+/1170402.
once it is approved , I will send for the upstreamreview

@abinayaddhandapani
Copy link
Collaborator Author

@ojayanth There is no public APML documentation link. APML is defined as part of AMD processor programming reference (PPR)

OWNERS Outdated
email: abinaya.dhandapani@amd.com
discord: abinayadhandapani
- name: Jayanth Othayoth
email: jayanth.othayoth@amd.com

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

README.md Outdated
# AMD BMC RAS
# AMD BMC RAS

The amd - bmc - ras service is intended to discover, configure and exercise OOB

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing .

README.md Outdated
## Features

The application waits on the APML_L gpio pin to check if any events are
detected. When a fatal error is detected in the system , SMU responds to
Copy link

@ojayanth ojayanth Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing

Ack


amd-ras is configured per the
[meson build files](https://mesonbuild.com/Build-options.html). Available
options are documented in `meson_options.txt`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing

## Configuration

amd-ras is configured per the
[meson build files](https://mesonbuild.com/Build-options.html). Available
Copy link

@ojayanth ojayanth Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like name an d link is not matching , please look.

Reply : The upstream uses the same name and the link to point out the build-options file.
Attaching link for reference:
https://github.com/openbmc/bmcweb/blob/deae6a789444debc4724fb6902fc5def299afbee/README.md?plain=1#L52

auto configMap = rasConfigTable();

std::ifstream jsonFile(CONFIG_FILE);
if (!jsonFile.is_open())
Copy link

@ojayanth ojayanth Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like d-bus errors are Not documented in dbus review. Please update.

Reply : I have sent for the internal gerrit review https://gerrit-git.amd.com/c/ESE/ec/BMC/openbmc/+/1170402.
once it is approved , I will send for the upstreamreview

src/config_manager.cpp Show resolved Hide resolved
{
if (!fs::exists(CONFIG_FILE))
{ // Check if the config file exists
fs::path destDir = fs::path(CONFIG_FILE).parent_path();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this also in try catch box .

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have already discussed that we are not going to continue the application if the config_file is not found.

Hence I am not adding catch block here which doesnt seem to be of much use.

if I catch the exception, again I need to re-throw to terminate the application

std::variant<bool, std::string, int64_t, std::vector<std::string>,
std::map<std::string, std::string>>
value;
int64_t maxBoundValue = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxBoundValue should get from json , not seeing any code related to this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Abinaya Dhandapani added 2 commits December 19, 2024 10:51
- Added README.md with project overview.
- Added OWNERS file to define project ownership and contributors.
- Added LICENSE file to specify project licensing terms.

Signed-off-by: Abinaya Dhandapani <abinaya.dhandapani@amd.com>
Added clang-18 formatter.

Signed-off-by: Abinaya Dhandapani <abinaya.dhandapani@amd.com>
AttributeType attributeType;
std::string key;
std::string description;
std::variant<bool, std::string, int64_t, std::vector<std::string>,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value variable is declared but not initialized before checking its index.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

key = item.begin().key();

const auto& obj = item[key];
description = obj["Description"];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling fro description missing need to handle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

{
attributeType = sdbusplus::common::com::amd::ras::
Configuration::AttributeType::KeyValueMap;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

un suported value case need to handle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

src/config_manager.cpp Show resolved Hide resolved

Manager::AttributeValue Manager::getAttribute(AttributeName attribute)
{
auto configMap = rasConfigTable();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

AttributeValue value;
bool found = false;

for (auto& [key, tuple] : configMap)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack


if (isValidValue)
{
lg2::error(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add attribute name and value - usefull for debug

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

}
else
{
lg2::error("Valid option not provided");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add attribute name /value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

}
else
{
lg2::error("Attribute not found");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add attribute name .

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

1. Added RAS configuration class which creates the
RAS configuration interface to set and get the config values.

2. The setAttribute and getAttribute methods are overridden
in the class to update the ras_config.json with the latest values.

Tested:
1. Service is working well.
2. All the dbus methods and properties are shown correctly.
   Able to get and update the ras_config.json file using D-Bus methods.
3. Unit test done.

root@xxxx:~# busctl tree com.amd.RAS
`- /com
  `- /com/amd
    `- /com/amd/RAS

Signed-off-by: Abinaya Dhandapani <abinaya.dhandapani@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants