Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

On Tue, 2016-03-01 at 17:37 +0000, Moore, Robert wrote:
> 
> > -----Original Message-----
> > From: Toshi Kani [mailto:toshi.kani@xxxxxxx]
> > Sent: Tuesday, March 01, 2016 9:37 AM
> > To: Moore, Robert; rjw@xxxxxxxxxxxxx; Williams, Dan J
> > Cc: Zheng, Lv; elliott@xxxxxxx; linux-nvdimm@xxxxxxxxxxxx; linux-
> > acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx
> > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure
> > to
> > 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
> > bytes.
> 
> 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 5.2.25.9 of ACPI 6.1 further clarifies that Vendor ID needs to be
represented as ("%02x%02x", byte0, byte1), which is "1234" in this case.

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
below.

|ÂÂ- 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 byte-
> > 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...

Thanks,
-Toshi