lkml.org 
[lkml]   [2022]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v5 2/3] ACPI: allow longer device IDs
    On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
    >
    > From: Alexander Graf <graf@amazon.com>
    >

    Please don't invent patch authors like that. Alex's patch that started
    this discussion was completely different.

    > We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
    > entries of the respective devices. However, we squeeze them into struct
    > acpi_device_id, which only has 9 bytes space to store the identifier. It
    > originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
    > ("mod/file2alias: make modalias generation safe for cross compiling"),
    > presumably on the theory that it would match the ACPI spec so it didn't
    > matter.
    >

    Please clarify that this applies to the module metadata side of
    things. The ACPI subsystem already captures and exposes _HIDs and
    _CIDs that are longer than 8 characters, which is why simply
    increasing the size of this field is sufficient to create modules that
    can match devices that expose a CID that is longer than 8 bytes.

    > Unfortunately, while most people adhere to the ACPI specs, Microsoft
    > decided that its VM Generation Counter device [1] should only be
    > identifiable by _CID with a value of "VM_Gen_Counter", which is longer
    > than 9 characters.
    >
    > To allow device drivers to match identifiers that exceed the 9 byte
    > limit, this simply ups the length to 16, just like it was before the
    > aforementioned commit. Empirical testing indicates that this
    > doesn't actually increase vmlinux size, because the ulong in the same
    > struct caused there to be 7 bytes of padding anyway.
    >

    The padding situation only applies to struct acpi_device_id, whereas
    ACPI_ID_LEN is used in other places as well. Also, the size of vmlinux
    only covers statically allocated instances in the core kernel, and
    most of the ACPI_ID_LEN uses are probably in drivers. So whether
    vmlinux changes size or not is not that relevant.


    > This patch is a prerequisite to add support for VMGenID in Linux, the
    > subsequent patch in this series. It has been confirmed to also work on
    > the udev/modalias side in userspace.
    >
    > [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
    >
    > Cc: Ard Biesheuvel <ardb@kernel.org>
    > Cc: Len Brown <lenb@kernel.org>
    > Cc: Rafael J. Wysocki <rafael@kernel.org>
    > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    > Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
    > Signed-off-by: Alexander Graf <graf@amazon.com>
    > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

    Patch 6543becf26ff was wrong to change ACPI_ID_LEN, because it failed
    to take into account any other uses of ACPI_ID_LEN, and did not bother
    to explain why the change was necessary in the context of what it was
    trying to achieve.

    So, given that we need more than 8 characters to match drivers to
    devices exposed by Hyper-V (or other VMMs adhering to the VMGENID
    spec), I think this change is necessary and correct.

    So, with the authorship/signoff corrected, and the commit log clarified,

    Reviewed-by: Ard Biesheuvel <ardb@kernel.org>


    > ---
    > Hi Rafael & Len,
    >
    > This patchset is directed toward you two specifically. Patches 1/3 and
    > 3/3 have been through the ringer of review a bit already and do not
    > specifically require your attention, but in v4 we wound up getting hung
    > up on an ACPI API limitation. This v5 fixes that limitation with this
    > 2/3 patch that you see here, with a trivial one line fix, which does
    > require your attention.
    >
    > Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
    > actually depends on this one here, 2/3, in order to compile without
    > warnings (and be functional at all). Therefore, it would be nice if you
    > would provide an "Acked-by" on it and permit me to /also/ take it
    > through my random.git tree (if it looks like a correct patch to you, of
    > course). This would make the merge logistics a lot easier. Plus it's a
    > small +1/-1 line change.
    >
    > Please have a look and let me know what you think.
    >
    > Thanks,
    > Jason
    >
    > include/linux/mod_devicetable.h | 2 +-
    > 1 file changed, 1 insertion(+), 1 deletion(-)
    >
    > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
    > index 4bb71979a8fd..5da5d990ff58 100644
    > --- a/include/linux/mod_devicetable.h
    > +++ b/include/linux/mod_devicetable.h
    > @@ -211,7 +211,7 @@ struct css_device_id {
    > kernel_ulong_t driver_data;
    > };
    >
    > -#define ACPI_ID_LEN 9
    > +#define ACPI_ID_LEN 16
    >
    > struct acpi_device_id {
    > __u8 id[ACPI_ID_LEN];
    > --
    > 2.35.1
    >

    \
     
     \ /
      Last update: 2022-02-27 08:32    [W:2.903 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site