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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1456859438.15454.78.camel@hpe.com>
Date:	Tue, 01 Mar 2016 12:10:38 -0700
From:	Toshi Kani <toshi.kani@....com>
To:	"Moore, Robert" <robert.moore@...el.com>,
	"rjw@...ysocki.net" <rjw@...ysocki.net>,
	"Williams, Dan J" <dan.j.williams@...el.com>
Cc:	"Zheng, Lv" <lv.zheng@...el.com>,
	"elliott@....com" <elliott@....com>,
	"linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...ica.org" <devel@...ica.org>
Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to
 comply ACPI 6.1

On Tue, 2016-03-01 at 17:37 +0000, Moore, Robert wrote:
> 
> > -----Original Message-----
> > From: Toshi Kani [mailto:toshi.kani@....com]
> > Sent: Tuesday, March 01, 2016 9:37 AM
> > To: Moore, Robert; rjw@...ysocki.net; Williams, Dan J
> > Cc: Zheng, Lv; elliott@....com; linux-nvdimm@...ts.01.org; linux-
> > acpi@...r.kernel.org; linux-kernel@...r.kernel.org; devel@...ica.org
> > 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