Re: [Freeipmi-devel] [RFC][PATCH 0/1] Fix apparent bug w/ ACPI SPMI table parsing
Hey Dann, As I thought about it, I'm not sure how much the ACPI code has even been tested (atleast by me). Almost every motherboard I can recall had info in DMI/SMBIOS. Digging into the code history, outside of cleanups & refactoring (which of course could have introduced issues, but I doubt those issues would have been at the level of what you found), this code was developed circa 2004-2005, with the last "real" change in 2006. So I'm willing to accept your change as is b/c A) it makes sense, B) you seem to have a system you can half-test against and C) I assume this code worked long ago on whatever system it was originally developed against, but who knows if that system even did it correctly. That said, you seem to be suggesting there are going to be some further patches down the line for acpi via sysfs. If that code is coming down the pipeline, and this code is for it, I can wait for the entire patch series to come in and it can be added all at once. Al On Mon, 2018-07-30 at 15:30 -0600, dann frazier wrote: > hey. > I'm working on a patch to add support to ipmi-locate code to parse > ACPI > SPMI tables via sysfs. This is to make ipmi-locate work on an ARM > system > we have that describes its BMC only in ACPI (not DMI). Being ARM, > trolling > /dev/mem isn't safe, (freeipmi ifdef's that out), so using the > kernel-exposed > tables, when available, is a better option. > > While doing this I ran across what seems like a bug, which the > following patch > should fix. I say "seems" and "should" because I don't have a system > where the > existing /dev/mem snooping code works - with, or without this bug fix > - > therefore, I cannot emperically demonstrate the bug. On the systems > I've > tested, which do include an SPMI table, ipmi-locate is unable to find > a valid > RSDT signature using the /dev/mem method. I'm not sure what I'm doing > wrong > there - I tried diabling CONFIG_STRICT_DEVMEM andsetting the > vm.mmap_min_addr > sysctl to 0 w/o luck. > > dann frazier (1): > Don't try to separate the header from the ACPI table data > > libfreeipmi/locate/ipmi-locate-acpi-spmi.c | 33 +++- > -- > 1 file changed, 4 insertions(+), 29 deletions(-) > -- Albert Chu ch...@llnl.gov Computer Scientist High Performance Systems Division Lawrence Livermore National Laboratory ___ Freeipmi-devel mailing list Freeipmi-devel@gnu.org https://lists.gnu.org/mailman/listinfo/freeipmi-devel
Re: [Freeipmi-devel] [RFC][PATCH 0/1] Fix apparent bug w/ ACPI SPMI table parsing
On Mon, Jul 30, 2018 at 03:30:44PM -0600, dann frazier wrote:
> hey.
> I'm working on a patch to add support to ipmi-locate code to parse ACPI
> SPMI tables via sysfs. This is to make ipmi-locate work on an ARM system
> we have that describes its BMC only in ACPI (not DMI). Being ARM, trolling
> /dev/mem isn't safe, (freeipmi ifdef's that out), so using the kernel-exposed
> tables, when available, is a better option.
>
> While doing this I ran across what seems like a bug, which the following patch
> should fix. I say "seems" and "should" because I don't have a system where the
> existing /dev/mem snooping code works - with, or without this bug fix -
> therefore, I cannot emperically demonstrate the bug.
I am able to *simulate* the issue with the following patch:
diff --git a/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
b/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
index 13c0e30cc..3b8ee98fb 100644
--- a/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
+++ b/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
@@ -1189,12 +1189,45 @@ _ipmi_acpi_get_firmware_table (ipmi_locate_ctx_t ctx,
{ 0, "", 0}
};
+ /* const uint8_t test_spmi_data[] = { 0x50, 0x53, 0x49, 0x4d, 0x00, 0x41,
+0x00, 0x00, 0xd7, 0x05, 0x49, 0x48,
+0x49, 0x53, 0x20, 0x20, 0x49, 0x48,
+0x30, 0x50, 0x20, 0x38, 0x20, 0x20,
+0x00, 0x00, 0x00, 0x00, 0x4e, 0x49,
+0x4c, 0x54, 0x11, 0x24, 0x20, 0x15,
+0x01, 0x03, 0x02, 0x00, 0x00, 0x00,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+0x08, 0x00, 0x01, 0x00, 0x00, 0xe4,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+0x00, 0x00, 0x00, 0x00 };
+ */
+ const uint8_t test_spmi_data[] = { 0x53, 0x50, 0x4d, 0x49, 0x41, 0x00,
+0x00, 0x00, 0x05, 0xd7, 0x48, 0x49,
+0x53, 0x49, 0x20, 0x20, 0x48, 0x49,
+0x50, 0x30, 0x38, 0x20, 0x20, 0x20,
+0x00, 0x00, 0x00, 0x00, 0x49, 0x4e,
+0x54, 0x4c, 0x24, 0x11, 0x15, 0x20,
+0x03, 0x01, 0x00, 0x02, 0x00, 0x00,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+0x00, 0x08, 0x00, 0x01, 0xe4, 0x00,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+0x00, 0x00, 0x00, 0x00 };
+ const uint32_t test_spmi_data_length = 64;
+
assert (ctx);
assert (ctx->magic == IPMI_LOCATE_CTX_MAGIC);
assert (signature);
assert (sign_table_data);
assert (sign_table_data_length);
+ if ((strcmp ("SPMI", signature)) == 0)
+{
+ acpi_table = malloc(test_spmi_data_length);
+ memcpy(acpi_table, test_spmi_data, test_spmi_data_length);
+ acpi_table_length = test_spmi_data_length;
+ goto fakespmi;
+}
+
*sign_table_data = NULL;
if ((acpi_table_hdr_length = fiid_template_len_bytes (tmpl_acpi_table_hdr))
< 0)
@@ -1332,6 +1365,7 @@ _ipmi_acpi_get_firmware_table (ipmi_locate_ctx_t ctx,
acpi_table_length = 0;
}
+ fakespmi:
if (!acpi_table)
{
LOCATE_SET_ERRNUM (ctx, IPMI_LOCATE_ERR_SYSTEM_ERROR);
--
2.18.0
___
Freeipmi-devel mailing list
Freeipmi-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/freeipmi-devel
[Freeipmi-devel] [RFC][PATCH 1/1] Don't try to separate the header from the ACPI table data
_ipmi_acpi_get_spmi_table() calls _ipmi_acpi_get_firmware_table() to
populate obj_acpi_table_hdr and table_data with the SPMI table header
and SPMI table data, respectively. However, there appears to be an
internal discrepancy as to whether or not the table_data should also
contain the header as well.
_ipmi_acpi_get_firmware_table() only returns the non-header data in
table_data._ipmi_acpi_get_spmi_table() then loads that data into an
object using the SPMI table template (tmpl_acpi_spmi_table_descriptor).
However, you'll notice that the SPMI table template also includes the
ACPI table header fields. So fiid_obj_set_all() is copying the headerless
data into an object expecting header+data, corrupting it.
One way to solve this problem would be removing the header fields from
the SPMI table template, and adjusting the code appropriately. Another is
to stop treating header and non-header data differently here, storing
them both in table_data. That is the approach I've taken here. It also
cleans up a layering issue where ipmi_locate_acpi_spmi_get_device_info
was creating and passing the obj_acpi_table_hdr variable down to lower
levels to use, while not having any use for the variable itself.
---
libfreeipmi/locate/ipmi-locate-acpi-spmi.c | 33 +++---
1 file changed, 4 insertions(+), 29 deletions(-)
diff --git a/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
b/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
index 8e2cfe5e7..13c0e30cc 100644
--- a/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
+++ b/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
@@ -596,12 +596,10 @@ static int _ipmi_acpi_get_table (ipmi_locate_ctx_t ctx,
static int _ipmi_acpi_get_firmware_table (ipmi_locate_ctx_t ctx,
char *signature,
unsigned int table_instance,
- fiid_obj_t obj_acpi_table_hdr,
uint8_t **sign_table_data,
uint32_t *sign_table_data_length);
static int _ipmi_acpi_get_spmi_table (ipmi_locate_ctx_t ctx,
uint8_t interface_type,
- fiid_obj_t obj_acpi_table_hdr,
fiid_obj_t
obj_acpi_spmi_table_descriptor);
#define IPMI_INTERFACE_COUNT 5
@@ -1138,14 +1136,12 @@ _ipmi_acpi_get_table (ipmi_locate_ctx_t ctx,
* PARAMETERS:
* signature - ACPI signature for firmware table header
* table_instance - Which instance of the firmware table
- * obj_acpi_table_hdr - Initialized ACPI table header
* sign_table_data - Initialized with malloc'ed ACPI firmware table
data
* sign_table_data_length - ACPI table DATA length
*
* RETURN:
* return (0) for success. ACPI table header and firmware table DATA are
- * returned through obj_acpi_table_hdr and signed_table_data
- * parameters.
+ * returned through the signed_table_data parameter.
*
* DESCRIPTION:
* Top level call for any ACPI firmware table by table signature string.
@@ -1156,7 +1152,6 @@ static int
_ipmi_acpi_get_firmware_table (ipmi_locate_ctx_t ctx,
char *signature,
unsigned int table_instance,
- fiid_obj_t obj_acpi_table_hdr,
uint8_t **sign_table_data,
uint32_t *sign_table_data_length)
{
@@ -1197,10 +1192,8 @@ _ipmi_acpi_get_firmware_table (ipmi_locate_ctx_t ctx,
assert (ctx);
assert (ctx->magic == IPMI_LOCATE_CTX_MAGIC);
assert (signature);
- assert (fiid_obj_valid (obj_acpi_table_hdr));
assert (sign_table_data);
assert (sign_table_data_length);
- assert (fiid_obj_template_compare (obj_acpi_table_hdr, tmpl_acpi_table_hdr)
== 1);
*sign_table_data = NULL;
@@ -1345,16 +1338,13 @@ _ipmi_acpi_get_firmware_table (ipmi_locate_ctx_t ctx,
goto cleanup;
}
- memcpy (obj_acpi_table_hdr, acpi_table, acpi_table_hdr_length);
- *sign_table_data_length = acpi_table_length - acpi_table_hdr_length;
+ *sign_table_data_length = acpi_table_length;
if (!(*sign_table_data = malloc (*sign_table_data_length)))
{
LOCATE_SET_ERRNUM (ctx, IPMI_LOCATE_ERR_OUT_OF_MEMORY);
goto cleanup;
}
- memcpy (*sign_table_data,
- (acpi_table + acpi_table_hdr_length),
- *sign_table_data_length);
+ memcpy (*sign_table_data, acpi_table, *sign_table_data_length);
rv = 0;
cleanup:
@@ -1372,13 +1362,11 @@ _ipmi_acpi_get_firmware_table (ipmi_locate_ctx_t ctx,
*
* PARAMETERS:
* interface_type - Type of interface to look for (KCS, SSIF,
SMIC, BT)
- * obj_acpi_table_hdr - Initialized ACPI table header
* acpi_table_firmware - Initialized ACPI firmware table
*
* RETURN:
* return (0) for success. ACPI table header and SPMI table is
- * returned thro
[Freeipmi-devel] [RFC][PATCH 0/1] Fix apparent bug w/ ACPI SPMI table parsing
hey. I'm working on a patch to add support to ipmi-locate code to parse ACPI SPMI tables via sysfs. This is to make ipmi-locate work on an ARM system we have that describes its BMC only in ACPI (not DMI). Being ARM, trolling /dev/mem isn't safe, (freeipmi ifdef's that out), so using the kernel-exposed tables, when available, is a better option. While doing this I ran across what seems like a bug, which the following patch should fix. I say "seems" and "should" because I don't have a system where the existing /dev/mem snooping code works - with, or without this bug fix - therefore, I cannot emperically demonstrate the bug. On the systems I've tested, which do include an SPMI table, ipmi-locate is unable to find a valid RSDT signature using the /dev/mem method. I'm not sure what I'm doing wrong there - I tried diabling CONFIG_STRICT_DEVMEM andsetting the vm.mmap_min_addr sysctl to 0 w/o luck. dann frazier (1): Don't try to separate the header from the ACPI table data libfreeipmi/locate/ipmi-locate-acpi-spmi.c | 33 +++--- 1 file changed, 4 insertions(+), 29 deletions(-) -- 2.18.0 ___ Freeipmi-devel mailing list Freeipmi-devel@gnu.org https://lists.gnu.org/mailman/listinfo/freeipmi-devel
