| Message ID | 20220225155552.30636-1-graf@amazon.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: bus: Match first 9 bytes of device IDs | expand |
Hi Alex, Thanks for this. I tested this out with the vmgenid driver, and I found a bug in this v1: On Fri, Feb 25, 2022 at 04:55:52PM +0100, Alexander Graf wrote: > - if (id->id[0] && !strcmp((char *)id->id, hwid->id)) > + if (id->id[0] && !strncmp((char *)id->id, hwid->id, ACPI_ID_LEN)) This only worked once I made that `ACPI_ID_LEN - 1`, because that length includes the null terminator. The below patch works fine. If you adjust that and send a quick v2 follow-up, I'm happy to ack it. Regards, Jason --------8<-------------------------- diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 07f604832fd6..f179ebf16f21 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -829,7 +829,7 @@ static bool __acpi_match_device(struct acpi_device *device, /* First, check the ACPI/PNP IDs provided by the caller. */ if (acpi_ids) { for (id = acpi_ids; id->id[0] || id->cls; id++) { - if (id->id[0] && !strcmp((char *)id->id, hwid->id)) + if (id->id[0] && !strncmp((char *)id->id, hwid->id, ACPI_ID_LEN - 1)) goto out_acpi_match; if (id->cls && __acpi_match_device_cls(id, hwid)) goto out_acpi_match; diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c index b503c210c2d7..04751fc1d365 100644 --- a/drivers/virt/vmgenid.c +++ b/drivers/virt/vmgenid.c @@ -78,8 +78,7 @@ static void vmgenid_acpi_notify(struct acpi_device *device, u32 event) } static const struct acpi_device_id vmgenid_ids[] = { - { "QEMUVGID", 0 }, /* QEMU */ - { "VMGENID", 0 }, /* Firecracker */ + { "VM_GEN_C", 0 }, /* Truncated "VM_Gen_Counter" */ { }, };
Hey Jason, On 25.02.22 17:57, Jason A. Donenfeld wrote: > Hi Alex, > > Thanks for this. > > I tested this out with the vmgenid driver, and I found a bug in this v1: > > On Fri, Feb 25, 2022 at 04:55:52PM +0100, Alexander Graf wrote: >> - if (id->id[0] && !strcmp((char *)id->id, hwid->id)) >> + if (id->id[0] && !strncmp((char *)id->id, hwid->id, ACPI_ID_LEN)) > This only worked once I made that `ACPI_ID_LEN - 1`, because that length > includes the null terminator. The below patch works fine. If you adjust > that and send a quick v2 follow-up, I'm happy to ack it. > > Regards, > Jason > > --------8<-------------------------- > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 07f604832fd6..f179ebf16f21 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -829,7 +829,7 @@ static bool __acpi_match_device(struct acpi_device *device, > /* First, check the ACPI/PNP IDs provided by the caller. */ > if (acpi_ids) { > for (id = acpi_ids; id->id[0] || id->cls; id++) { > - if (id->id[0] && !strcmp((char *)id->id, hwid->id)) > + if (id->id[0] && !strncmp((char *)id->id, hwid->id, ACPI_ID_LEN - 1)) > goto out_acpi_match; > if (id->cls && __acpi_match_device_cls(id, hwid)) > goto out_acpi_match; > diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c > index b503c210c2d7..04751fc1d365 100644 > --- a/drivers/virt/vmgenid.c > +++ b/drivers/virt/vmgenid.c > @@ -78,8 +78,7 @@ static void vmgenid_acpi_notify(struct acpi_device *device, u32 event) > } > > static const struct acpi_device_id vmgenid_ids[] = { > - { "QEMUVGID", 0 }, /* QEMU */ > - { "VMGENID", 0 }, /* Firecracker */ > + { "VM_GEN_C", 0 }, /* Truncated "VM_Gen_Counter" */ You have to make this "VM_GEN_CO". I now match the full 9 bytes - unlike the previous patch I sent :) Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Fri, 25 Feb 2022 at 16:56, Alexander Graf <graf@amazon.com> wrote: > > 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 > based on the ACPI spec: > > """ > A _HID object evaluates to either a numeric 32-bit compressed EISA > type ID or a string. If a string, the format must be an alphanumeric > PNP or ACPI ID with no asterisk or other leading characters. > A valid PNP ID must be of the form "AAA####" where A is an uppercase > letter and # is a hex digit. > A valid ACPI ID must be of the form "NNNN####" where N is an uppercase > letter or a digit ('0'-'9') and # is a hex digit. This specification > reserves the string "ACPI" for use only with devices defined herein. > It further reserves all strings representing 4 HEX digits for > exclusive use with PCI-assigned Vendor IDs. > """ > > While most people adhere to the ACPI specs, Microsoft decided that its > VM Generation Counter device [1] should only be identifiable by _CID with > an value of "VM_Gen_Counter" - longer than 9 characters. > > To still allow device drivers to match identifiers that exceed the 9 byte > limit, without wasting memory for the unlikely case that you have long > identifiers, let's match only the first 9 characters of the identifier. > > This patch is a prerequisite to add support for VMGenID in Linux [2]. > > [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx > [2] https://lore.kernel.org/lkml/20220225124848.909093-1-Jason@zx2c4.com/ > > Signed-off-by: Alexander Graf <graf@amazon.com> > > --- > > Alternatives to the approach above would be: > > 1) Always set id[8] = '\0' in acpi_add_id() > 2) Allocate the id in struct acpi_device_id dynamically > > I'm happy to explore option 1 instead if people believe it's cleaner. > Option 2 on the other hand seems overkill for the issue at hand. We don't > have a lot of devices that exceed the 8 character threshold, so chance of > collision is quite small. On the other hand, the extra overhead of > maintaining the string allocation dynamically will quickly become a > headache. > I don't like this hack. If we are going to accept the fact that CIDs could be arbitrary length strings, we should handle them properly. The device subsystem side of things already deals with this properly: the modalias of the QEMU vmgenid device comes up as 'acpi:QEMUVGID:VM_GEN_COUNTER', which means it already captures the entire string, and exposes it in the correct way (modulo the all caps) So what we need is a way for a module to describe its compatibility with such a _CID, which shouldn't be that complicated. I'll try to cook something up. > --- > drivers/acpi/bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 07f604832fd6..aba93171739f 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -829,7 +829,7 @@ static bool __acpi_match_device(struct acpi_device *device, > /* First, check the ACPI/PNP IDs provided by the caller. */ > if (acpi_ids) { > for (id = acpi_ids; id->id[0] || id->cls; id++) { > - if (id->id[0] && !strcmp((char *)id->id, hwid->id)) > + if (id->id[0] && !strncmp((char *)id->id, hwid->id, ACPI_ID_LEN)) > goto out_acpi_match; > if (id->cls && __acpi_match_device_cls(id, hwid)) > goto out_acpi_match; > -- > 2.16.4 > > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B > Sitz: Berlin > Ust-ID: DE 289 237 879 > > >
Hi Alex, On Fri, Feb 25, 2022 at 06:09:59PM +0100, Alexander Graf wrote: > You have to make this "VM_GEN_CO". I now match the full 9 bytes - unlike > the previous patch I sent :) Ahh, so no NULL byte, but presumably no need because strncmp is used. Great, seems to work. There's the other aspect of udev matching, by the way. Have you tested what happens in userspace? Need be, we could make the vmgenid driver a bool instead of a tristate, but maybe there's a way to do that right? Jason
On Fri, Feb 25, 2022 at 6:13 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > The device subsystem side of things already deals with this properly: > the modalias of the QEMU vmgenid device comes up as > 'acpi:QEMUVGID:VM_GEN_COUNTER', which means it already captures the > entire string, and exposes it in the correct way (modulo the all caps) Ahh, so the userspace side of this won't work right. Shucks. That's what I was concerned about. > I don't like this hack. If we are going to accept the fact that CIDs > could be arbitrary length strings, we should handle them properly. > > So what we need is a way for a module to describe its compatibility > with such a _CID, which shouldn't be that complicated. Can't we do something more boring and just... 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 @@ -210,9 +210,9 @@ struct css_device_id { __u8 type; /* subchannel type */ kernel_ulong_t driver_data; }; -#define ACPI_ID_LEN 9 +#define ACPI_ID_LEN 16 struct acpi_device_id { __u8 id[ACPI_ID_LEN]; kernel_ulong_t driver_data; As you can see from the context, those additional 7 bytes were being wasted on padding anyway inside the acpi_device_id struct, so it's basically free, it would seem. This seems like the least convoluted way of solving this issue? If we ever encounter _more_ ACPI devices with weird names, we could revisit a fancy dynamic solution, but for now, why don't we keep it simple? Jason
On Fri, 25 Feb 2022 at 18:21, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Fri, Feb 25, 2022 at 6:13 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > The device subsystem side of things already deals with this properly: > > the modalias of the QEMU vmgenid device comes up as > > 'acpi:QEMUVGID:VM_GEN_COUNTER', which means it already captures the > > entire string, and exposes it in the correct way (modulo the all caps) > > Ahh, so the userspace side of this won't work right. Shucks. That's what > I was concerned about. > > > I don't like this hack. If we are going to accept the fact that CIDs > > could be arbitrary length strings, we should handle them properly. > > > > So what we need is a way for a module to describe its compatibility > > with such a _CID, which shouldn't be that complicated. > > Can't we do something more boring and just... > > 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 > @@ -210,9 +210,9 @@ struct css_device_id { > __u8 type; /* subchannel type */ > kernel_ulong_t driver_data; > }; > > -#define ACPI_ID_LEN 9 > +#define ACPI_ID_LEN 16 > > struct acpi_device_id { > __u8 id[ACPI_ID_LEN]; > kernel_ulong_t driver_data; > > > As you can see from the context, those additional 7 bytes were being > wasted on padding anyway inside the acpi_device_id struct, so it's > basically free, it would seem. This seems like the least convoluted way > of solving this issue? If we ever encounter _more_ ACPI devices with > weird names, we could revisit a fancy dynamic solution, but for now, why > don't we keep it simple? > Yeah, good point. I think this is fine, although there are a few other uses of ACPI_ID_LEN in the tree. So perhaps this should be something like #define ACPI_ID_LEN 9 #define ACPI_CID_LEN 16 /* explanation goes here */ struct acpi_device_id { __u8 id[ACPI_CID_LEN]; instead? At a quick glance, none of those ACPI_ID_LEN users seem related to the CID or the match metadata.
On Fri, Feb 25, 2022 at 6:30 PM Ard Biesheuvel <ardb@kernel.org> wrote: > Yeah, good point. I think this is fine, although there are a few other > uses of ACPI_ID_LEN in the tree. So perhaps this should be something > like > > #define ACPI_ID_LEN 9 > #define ACPI_CID_LEN 16 > > /* explanation goes here */ > > struct acpi_device_id { > __u8 id[ACPI_CID_LEN]; > > instead? At a quick glance, none of those ACPI_ID_LEN users seem > related to the CID or the match metadata. Either way is fine by me. Looking at all the current users of ACPI_ID_LEN, none of them really mind if it's >9. I can't see where it'd change any behavior or performance or really anything at all, anywhere. So I'm inclined to go with my original simpler solution. But again, either way is fine. Alex, do you want to pick one of these and submit a v2 based on it? Or do you see a shortcoming in that approach? Jason
On Fri, Feb 25, 2022 at 06:33:42PM +0100, Jason A. Donenfeld wrote: > Either way is fine by me. Looking at all the current users of > ACPI_ID_LEN, none of them really mind if it's >9. I can't see where > it'd change any behavior or performance or really anything at all, > anywhere. So I'm inclined to go with my original simpler solution. But > again, either way is fine. > > Alex, do you want to pick one of these and submit a v2 based on it? Or > do you see a shortcoming in that approach? I can confirm from testing that the below trivial diff fixes the issue. I believe Ard is looking into the userspace/udev ramifications. diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c index b503c210c2d7..4542ebb68ae0 100644 --- a/drivers/virt/vmgenid.c +++ b/drivers/virt/vmgenid.c @@ -78,8 +78,7 @@ static void vmgenid_acpi_notify(struct acpi_device *device, u32 event) } static const struct acpi_device_id vmgenid_ids[] = { - { "QEMUVGID", 0 }, /* QEMU */ - { "VMGENID", 0 }, /* Firecracker */ + { "VM_GEN_COUNTER", 0 }, { }, }; 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];
Okay, the final piece, userspace:
/sys/bus/acpi/devices/QEMUVGID:00/modalias gives:
acpi:QEMUVGID:VM_GEN_COUNTER:
modinfo -F alias vmgenid.ko gives:
acpi*:VM_GEN_COUNTER:*
udev src uses fnmatch.
Bash confirms a match:
$ [[ "acpi:QEMUVGID:VM_GEN_COUNTER:" == acpi*:VM_GEN_COUNTER:* ]] &&
echo matches
matches
So I think with ACPI_ID_LEN --> 16 we are good to go.
Jason
On 25.02.22 19:39, Jason A. Donenfeld wrote: > Okay, the final piece, userspace: > > /sys/bus/acpi/devices/QEMUVGID:00/modalias gives: > acpi:QEMUVGID:VM_GEN_COUNTER: > > modinfo -F alias vmgenid.ko gives: > acpi*:VM_GEN_COUNTER:* > > udev src uses fnmatch. > > Bash confirms a match: > > $ [[ "acpi:QEMUVGID:VM_GEN_COUNTER:" == acpi*:VM_GEN_COUNTER:* ]] && > echo matches > matches > > So I think with ACPI_ID_LEN --> 16 we are good to go. Is the size increase (mostly rodata I suppose? Anywhere else?) measurable? Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Hi Alex, On Fri, Feb 25, 2022 at 08:06:39PM +0100, Alexander Graf wrote: > On 25.02.22 19:39, Jason A. Donenfeld wrote: > > Okay, the final piece, userspace: > > > > /sys/bus/acpi/devices/QEMUVGID:00/modalias gives: > > acpi:QEMUVGID:VM_GEN_COUNTER: > > > > modinfo -F alias vmgenid.ko gives: > > acpi*:VM_GEN_COUNTER:* > > > > udev src uses fnmatch. > > > > Bash confirms a match: > > > > $ [[ "acpi:QEMUVGID:VM_GEN_COUNTER:" == acpi*:VM_GEN_COUNTER:* ]] && > > echo matches > > matches > > > > So I think with ACPI_ID_LEN --> 16 we are good to go. > > > Is the size increase (mostly rodata I suppose? Anywhere else?) measurable? On my test kernel, the size of vmlinux increases from 26918200 bytes to 26918200 bytes. Wait, did I do that test right? I'll try again. Yep, 26918200 -> 26918200, so it doesn't grow at all. I believe the reason is mostly because of: #define ACPI_ID_LEN 16 struct acpi_device_id { __u8 id[ACPI_ID_LEN]; kernel_ulong_t driver_data; __u32 cls; __u32 cls_msk; }; Because of the padding, 9->16 doesn't actually change the way this structure is allocated. Then, additional uses of ACPI_ID_LEN throughout the tree are rather sparse or enclosed within other structures or similar things. In other words, code size seems like very much a non-issue. Also, it's not as though MIPS has ACPI. We're talking about x86, Itanium, and (sometimes, I guess) arm64. Regards, Jason
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 07f604832fd6..aba93171739f 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -829,7 +829,7 @@ static bool __acpi_match_device(struct acpi_device *device, /* First, check the ACPI/PNP IDs provided by the caller. */ if (acpi_ids) { for (id = acpi_ids; id->id[0] || id->cls; id++) { - if (id->id[0] && !strcmp((char *)id->id, hwid->id)) + if (id->id[0] && !strncmp((char *)id->id, hwid->id, ACPI_ID_LEN)) goto out_acpi_match; if (id->cls && __acpi_match_device_cls(id, hwid)) goto out_acpi_match;
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 based on the ACPI spec: """ A _HID object evaluates to either a numeric 32-bit compressed EISA type ID or a string. If a string, the format must be an alphanumeric PNP or ACPI ID with no asterisk or other leading characters. A valid PNP ID must be of the form "AAA####" where A is an uppercase letter and # is a hex digit. A valid ACPI ID must be of the form "NNNN####" where N is an uppercase letter or a digit ('0'-'9') and # is a hex digit. This specification reserves the string "ACPI" for use only with devices defined herein. It further reserves all strings representing 4 HEX digits for exclusive use with PCI-assigned Vendor IDs. """ While most people adhere to the ACPI specs, Microsoft decided that its VM Generation Counter device [1] should only be identifiable by _CID with an value of "VM_Gen_Counter" - longer than 9 characters. To still allow device drivers to match identifiers that exceed the 9 byte limit, without wasting memory for the unlikely case that you have long identifiers, let's match only the first 9 characters of the identifier. This patch is a prerequisite to add support for VMGenID in Linux [2]. [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx [2] https://lore.kernel.org/lkml/20220225124848.909093-1-Jason@zx2c4.com/ Signed-off-by: Alexander Graf <graf@amazon.com> --- Alternatives to the approach above would be: 1) Always set id[8] = '\0' in acpi_add_id() 2) Allocate the id in struct acpi_device_id dynamically I'm happy to explore option 1 instead if people believe it's cleaner. Option 2 on the other hand seems overkill for the issue at hand. We don't have a lot of devices that exceed the 8 character threshold, so chance of collision is quite small. On the other hand, the extra overhead of maintaining the string allocation dynamically will quickly become a headache. --- drivers/acpi/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)