On Tue, 2016-03-01 at 17:37 +0000, Moore, Robert wrote:
> -----Original Message-----
> From: Toshi Kani [mailto:firstname.lastname@example.org]
> Sent: Tuesday, March 01, 2016 9:37 AM
> To: Moore, Robert; rjw(a)rjwysocki.net; Williams, Dan J
> Cc: Zheng, Lv; elliott(a)hpe.com; linux-nvdimm(a)lists.01.org; linux-
> acpi(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; devel(a)acpica.org
> Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure
> comply ACPI 6.1
> On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
> > We have a bunch of macros in include/acmacros.h -- like this:
> > ACPI_MOVE_16_TO_16(d, s)
> There is a problem in using the ACPICA byte-swap macros. ACPI is
> little-endian arch, so the macros are set to perform byte-swappings
> when the CPU arch is big-endian. This case, however, is the other way
> around. The fields in question are defined & stored as arrays of
That's not what I see in the ACPI spec. The fields are defined like any
other ACPI table.
Vendor ID 2 6
Identifier indicating the vendor of the NVDIMM.
This field shall be set to the value of the NVDIMM SPD Module
Manufacturer ID Code field a with byte 0 set to DDR4 SPD byte
320 and byte 1 set to DDR4 SPD byte 321.
They are different from other fields because the spec defines "byte
locations" of the fields. The above case, Vendor ID, is defined that:
- byte 0 set to DDR4 SPD byte 320
- byte 1 set to DDR4 SPD byte 321
For instance, if SPD byte 320 is 0x12 and 321 is 0x34, then this field is
stored as (0x12, 0x34). If you read this field as a 16-bit int value, you
will get 0x3412 on little-endian CPUs, such as x86.
Section 126.96.36.199 of ACPI 6.1 further clarifies that Vendor ID needs to be
represented as ("%02x%02x", byte0, byte1), which is "1234" in this
So, we will need to byte-swap when it is handled as a 16-bit int value on
little-endian CPUs. This is different from other fields with multi-byte
numeric values, which are stored in little-endian format in ACPI.
Hence, my patch avoids this byte-swapping as I described in the change log
| - Change 'struct acpi_nfit_control_region' to reflect the update.
| SPD IDs are defined as arrays of bytes, so that they can be
| treated in the same way regardless of CPU endianness and are
| not miss-treated as little-endian numeric values.
I hope this makes it clear now.
> Another issue is that it is not clear who needs to perform the
> swapping among ACPICA and drivers. If ACPICA, drivers must agree that
ACPICA does not ever do anything with the "data tables" like NFIT, other
than handing off the table when requested by a driver.
So, this means that the alternative is to change the NFIT driver to do the
byte-swapping for the fields when the CPU arch is little-endian. If we
cannot change the structure, we will have to take this course...