| Message ID | 20230913163823.7880-16-james.morse@arm.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | ACPI/arm64: add support for virtual cpuhotplug | expand |
On Wed, 13 Sep 2023 16:38:03 +0000 James Morse <james.morse@arm.com> wrote: > ACPI has two ways of describing processors in the DSDT. Either as a device > object with HID ACPI0007, or as a type 'C' package inside a Processor > Container. The ACPI processor driver probes CPUs described as devices, but > not those described as packages. > Specification reference needed... Terminology wise, I'd just refer to Processor() objects as I think they are named objects rather than data terms like a package (Which include a PkgLength etc) > Duplicate descriptions are not allowed, the ACPI processor driver already > parses the UID from both devices and containers. acpi_processor_get_info() > returns an error if the UID exists twice in the DSDT. > > The missing probe for CPUs described as packages creates a problem for > moving the cpu_register() calls into the acpi_processor driver, as CPUs > described like this don't get registered, leading to errors from other > subsystems when they try to add new sysfs entries to the CPU node. > (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp) > > To fix this, parse the processor container and call acpi_processor_add() > for each processor that is discovered like this. The processor container > handler is added with acpi_scan_add_handler(), so no detach call will > arrive. > > Qemu TCG describes CPUs using packages in a processor container. processor terms in a processor container. > > Signed-off-by: James Morse <james.morse@arm.com> Otherwise looks fine to me. Jonathan > --- > drivers/acpi/acpi_processor.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index c0839bcf78c1..b4bde78121bb 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -625,9 +625,31 @@ static struct acpi_scan_handler processor_handler = { > }, > }; > > +static acpi_status acpi_processor_container_walk(acpi_handle handle, > + u32 lvl, > + void *context, > + void **rv) > +{ > + struct acpi_device *adev; > + acpi_status status; > + > + adev = acpi_get_acpi_dev(handle); > + if (!adev) > + return AE_ERROR; > + > + status = acpi_processor_add(adev, &processor_device_ids[0]); > + acpi_put_acpi_dev(adev); > + > + return status; > +} > + > static int acpi_processor_container_attach(struct acpi_device *dev, > const struct acpi_device_id *id) > { > + acpi_walk_namespace(ACPI_TYPE_PROCESSOR, dev->handle, > + ACPI_UINT32_MAX, acpi_processor_container_walk, > + NULL, NULL, NULL); > + > return 1; > } >
On 9/14/23 02:38, James Morse wrote: > ACPI has two ways of describing processors in the DSDT. Either as a device > object with HID ACPI0007, or as a type 'C' package inside a Processor > Container. The ACPI processor driver probes CPUs described as devices, but > not those described as packages. > > Duplicate descriptions are not allowed, the ACPI processor driver already > parses the UID from both devices and containers. acpi_processor_get_info() > returns an error if the UID exists twice in the DSDT. > > The missing probe for CPUs described as packages creates a problem for > moving the cpu_register() calls into the acpi_processor driver, as CPUs > described like this don't get registered, leading to errors from other > subsystems when they try to add new sysfs entries to the CPU node. > (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp) > > To fix this, parse the processor container and call acpi_processor_add() > for each processor that is discovered like this. The processor container > handler is added with acpi_scan_add_handler(), so no detach call will > arrive. > > Qemu TCG describes CPUs using packages in a processor container. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > drivers/acpi/acpi_processor.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > I don't understand the last sentence of the commit log. QEMU always have "ACPI0007" for the processor devices. #define ACPI_PROCESSOR_DEVICE_HID "ACPI0007" #define ACPI_PROCESSOR_OBJECT_HID "LNXCPU" [gshan@gshan q]$ git grep ACPI0007 hw/acpi/cpu.c: aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007"))); hw/arm/virt-acpi-build.c: aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007"))); hw/riscv/virt-acpi-build.c: aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007"))); [gshan@gshan q]$ git grep LNXCPU > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c > index c0839bcf78c1..b4bde78121bb 100644 > --- a/drivers/acpi/acpi_processor.c > +++ b/drivers/acpi/acpi_processor.c > @@ -625,9 +625,31 @@ static struct acpi_scan_handler processor_handler = { > }, > }; > > +static acpi_status acpi_processor_container_walk(acpi_handle handle, > + u32 lvl, > + void *context, > + void **rv) > +{ > + struct acpi_device *adev; > + acpi_status status; > + > + adev = acpi_get_acpi_dev(handle); > + if (!adev) > + return AE_ERROR; > + > + status = acpi_processor_add(adev, &processor_device_ids[0]); > + acpi_put_acpi_dev(adev); > + > + return status; > +} > + > static int acpi_processor_container_attach(struct acpi_device *dev, > const struct acpi_device_id *id) > { > + acpi_walk_namespace(ACPI_TYPE_PROCESSOR, dev->handle, > + ACPI_UINT32_MAX, acpi_processor_container_walk, > + NULL, NULL, NULL); > + > return 1; > } > Thanks, Gavin
On Thu, Sep 14, 2023 at 02:53:53PM +0100, Jonathan Cameron wrote: > On Wed, 13 Sep 2023 16:38:03 +0000 > James Morse <james.morse@arm.com> wrote: > > > ACPI has two ways of describing processors in the DSDT. Either as a device > > object with HID ACPI0007, or as a type 'C' package inside a Processor > > Container. The ACPI processor driver probes CPUs described as devices, but > > not those described as packages. > > > > Specification reference needed... > > Terminology wise, I'd just refer to Processor() objects as I think they > are named objects rather than data terms like a package (Which include > a PkgLength etc) I'm not sure what kind of reference you want for the above. Looking in ACPI 6.5, I've found in 5.2.12: "Starting with ACPI Specification 6.3, the use of the Processor() object was deprecated. Only legacy systems should continue with this usage. On the Itanium architecture only, a _UID is provided for the Processor() that is a string object. This usage of _UID is also deprecated since it can preclude an OSPM from being able to match a processor to a non-enumerable device, such as those defined in the MADT. From ACPI Specification 6.3 onward, all processor objects for all architectures except Itanium must now use Device() objects with an _HID of ACPI0007, and use only integer _UID values." Also, there is: https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors Unfortunately, using the search facility on that site to try and find Processor() doesn't work - it appears to strip the "()" characters from the search (which is completely dumb, why do search facilities do that?) > > The missing probe for CPUs described as packages creates a problem for > > moving the cpu_register() calls into the acpi_processor driver, as CPUs > > described like this don't get registered, leading to errors from other > > subsystems when they try to add new sysfs entries to the CPU node. > > (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp) > > > > To fix this, parse the processor container and call acpi_processor_add() > > for each processor that is discovered like this. The processor container > > handler is added with acpi_scan_add_handler(), so no detach call will > > arrive. > > > > Qemu TCG describes CPUs using packages in a processor container. > > processor terms in a processor container. Are you wanting this to be: "Qemu TCG describes CPUs using processor terms in a processor container." ? Searching the ACPI spec for "processor terms" (with or without quotes) only brings up results for "terms" - yet another reason to hate site- provided search facilities, I don't know why sites bother. :(
On Mon, Sep 18, 2023 at 03:02:53PM +1000, Gavin Shan wrote: > > On 9/14/23 02:38, James Morse wrote: > > ACPI has two ways of describing processors in the DSDT. Either as a device > > object with HID ACPI0007, or as a type 'C' package inside a Processor > > Container. The ACPI processor driver probes CPUs described as devices, but > > not those described as packages. > > > > Duplicate descriptions are not allowed, the ACPI processor driver already > > parses the UID from both devices and containers. acpi_processor_get_info() > > returns an error if the UID exists twice in the DSDT. > > > > The missing probe for CPUs described as packages creates a problem for > > moving the cpu_register() calls into the acpi_processor driver, as CPUs > > described like this don't get registered, leading to errors from other > > subsystems when they try to add new sysfs entries to the CPU node. > > (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp) > > > > To fix this, parse the processor container and call acpi_processor_add() > > for each processor that is discovered like this. The processor container > > handler is added with acpi_scan_add_handler(), so no detach call will > > arrive. > > > > Qemu TCG describes CPUs using packages in a processor container. > > > > Signed-off-by: James Morse <james.morse@arm.com> > > --- > > drivers/acpi/acpi_processor.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > I don't understand the last sentence of the commit log. QEMU > always have "ACPI0007" for the processor devices. I think what James is referring to is the use of Processor Containers (see https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#processor-container-device) which are defined using HID ACPI0010. This seems to be what build_cpus_aml() is doing. It creates: \_SB.CPUS - processor container with ACPI0010 and then builds the processor devices underneath that object using ACPI0007. I think the use of "packages" there is wrong, it should be "processor devices" - taking the terminology from the above specification link. As far as I can see, QEMU does not (yet) use the option of embedding child processor containers beneath a parent.
On Fri, Nov 03, 2023 at 10:43:15AM +0000, Russell King (Oracle) wrote: > On Thu, Sep 14, 2023 at 02:53:53PM +0100, Jonathan Cameron wrote: > > On Wed, 13 Sep 2023 16:38:03 +0000 > > James Morse <james.morse@arm.com> wrote: > > > > > ACPI has two ways of describing processors in the DSDT. Either as a device > > > object with HID ACPI0007, or as a type 'C' package inside a Processor > > > Container. The ACPI processor driver probes CPUs described as devices, but > > > not those described as packages. > > > > > > > Specification reference needed... > > > > Terminology wise, I'd just refer to Processor() objects as I think they > > are named objects rather than data terms like a package (Which include > > a PkgLength etc) > > I'm not sure what kind of reference you want for the above. Looking in > ACPI 6.5, I've found in 5.2.12: > > "Starting with ACPI Specification 6.3, the use of the Processor() object > was deprecated. Only legacy systems should continue with this usage. On > the Itanium architecture only, a _UID is provided for the Processor() > that is a string object. This usage of _UID is also deprecated since it > can preclude an OSPM from being able to match a processor to a > non-enumerable device, such as those defined in the MADT. From ACPI > Specification 6.3 onward, all processor objects for all architectures > except Itanium must now use Device() objects with an _HID of ACPI0007, > and use only integer _UID values." > > Also, there is: > > https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors > > Unfortunately, using the search facility on that site to try and find > Processor() doesn't work - it appears to strip the "()" characters from > the search (which is completely dumb, why do search facilities do that?) > > > > The missing probe for CPUs described as packages creates a problem for > > > moving the cpu_register() calls into the acpi_processor driver, as CPUs > > > described like this don't get registered, leading to errors from other > > > subsystems when they try to add new sysfs entries to the CPU node. > > > (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp) > > > > > > To fix this, parse the processor container and call acpi_processor_add() > > > for each processor that is discovered like this. The processor container > > > handler is added with acpi_scan_add_handler(), so no detach call will > > > arrive. > > > > > > Qemu TCG describes CPUs using packages in a processor container. > > > > processor terms in a processor container. > > Are you wanting this to be: > > "Qemu TCG describes CPUs using processor terms in a processor > container." > > ? Searching the ACPI spec for "processor terms" (with or without quotes) > only brings up results for "terms" - yet another reason to hate site- > provided search facilities, I don't know why sites bother. :( Given what https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#processor-container-device says, and what QEMU does (as I detailed in my reply to Gavin), I think this should be: "Qemu TCG describes CPUs using processor devices in a processor container." which uses the same terminology as the ACPI specification. Maybe also including a reference to the above URL would be a good idea too?
On Wed, Sep 13, 2023 at 04:38:03PM +0000, James Morse wrote: > ACPI has two ways of describing processors in the DSDT. Either as a device > object with HID ACPI0007, or as a type 'C' package inside a Processor > Container. I'm struggling with the reference to a "type 'C' package inside a Processor Container". ACPI 6.0, which introduced the Processor Container, says: "This optional device is a container object that acts much like a bus node in a namespace. It may contain child objects that are either processor devices or other processor containers" For "Processor device": "For more information on the declaration of the processor device object, see Section 19.6.30, "Device (Declare Device Package)."" which leads one to the Device() object, not the Processor() object. It also states: "A Device definition for a processor is declared using the ACPI0007 hardware identifier (HID)." My understanding from this is that Processor() is not allowed to be used within a Processor Container, only Device()s with _HID of ACPI0007 are permitted. In light of this, please could you clarify your first sentence, as it seems to be contrary to what is stated in ACPI 6.x specs. Thanks.
On Fri, 3 Nov 2023 10:43:14 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Thu, Sep 14, 2023 at 02:53:53PM +0100, Jonathan Cameron wrote: > > On Wed, 13 Sep 2023 16:38:03 +0000 > > James Morse <james.morse@arm.com> wrote: > > > > > ACPI has two ways of describing processors in the DSDT. Either as a device > > > object with HID ACPI0007, or as a type 'C' package inside a Processor > > > Container. The ACPI processor driver probes CPUs described as devices, but > > > not those described as packages. > > > > > > > Specification reference needed... > > > > Terminology wise, I'd just refer to Processor() objects as I think they > > are named objects rather than data terms like a package (Which include > > a PkgLength etc) > > I'm not sure what kind of reference you want for the above. Looking in > ACPI 6.5, I've found in 5.2.12: > > "Starting with ACPI Specification 6.3, the use of the Processor() object > was deprecated. Only legacy systems should continue with this usage. On > the Itanium architecture only, a _UID is provided for the Processor() > that is a string object. This usage of _UID is also deprecated since it > can preclude an OSPM from being able to match a processor to a > non-enumerable device, such as those defined in the MADT. From ACPI > Specification 6.3 onward, all processor objects for all architectures > except Itanium must now use Device() objects with an _HID of ACPI0007, > and use only integer _UID values." > > Also, there is: > > https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#declaring-processors That pair of refs, just as 'where to look if you care' cross references, seem to cover it as well as possible. > > Unfortunately, using the search facility on that site to try and find > Processor() doesn't work - it appears to strip the "()" characters from > the search (which is completely dumb, why do search facilities do that?) Yeah. Not great. > > > > The missing probe for CPUs described as packages creates a problem for > > > moving the cpu_register() calls into the acpi_processor driver, as CPUs > > > described like this don't get registered, leading to errors from other > > > subsystems when they try to add new sysfs entries to the CPU node. > > > (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp) > > > > > > To fix this, parse the processor container and call acpi_processor_add() > > > for each processor that is discovered like this. The processor container > > > handler is added with acpi_scan_add_handler(), so no detach call will > > > arrive. > > > > > > Qemu TCG describes CPUs using packages in a processor container. > > > > processor terms in a processor container. > > Are you wanting this to be: > > "Qemu TCG describes CPUs using processor terms in a processor > container." > > ? Searching the ACPI spec for "processor terms" (with or without quotes) > only brings up results for "terms" - yet another reason to hate site- > provided search facilities, I don't know why sites bother. :( Yup. I just use the PDFs partly for that reason. Not possible to find in 6.5 because as it's deprecated they removed the information.. Look at ACPI 6.3 and there is 19.6.108 Processor (Declare Processor) deep in the ASL operator reference Wording wise I'm not sure exactly what they should be other than they aren't packages (if my rough ASL understanding is right). Different byte encoding. Jonathan >
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index c0839bcf78c1..b4bde78121bb 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -625,9 +625,31 @@ static struct acpi_scan_handler processor_handler = { }, }; +static acpi_status acpi_processor_container_walk(acpi_handle handle, + u32 lvl, + void *context, + void **rv) +{ + struct acpi_device *adev; + acpi_status status; + + adev = acpi_get_acpi_dev(handle); + if (!adev) + return AE_ERROR; + + status = acpi_processor_add(adev, &processor_device_ids[0]); + acpi_put_acpi_dev(adev); + + return status; +} + static int acpi_processor_container_attach(struct acpi_device *dev, const struct acpi_device_id *id) { + acpi_walk_namespace(ACPI_TYPE_PROCESSOR, dev->handle, + ACPI_UINT32_MAX, acpi_processor_container_walk, + NULL, NULL, NULL); + return 1; }
ACPI has two ways of describing processors in the DSDT. Either as a device object with HID ACPI0007, or as a type 'C' package inside a Processor Container. The ACPI processor driver probes CPUs described as devices, but not those described as packages. Duplicate descriptions are not allowed, the ACPI processor driver already parses the UID from both devices and containers. acpi_processor_get_info() returns an error if the UID exists twice in the DSDT. The missing probe for CPUs described as packages creates a problem for moving the cpu_register() calls into the acpi_processor driver, as CPUs described like this don't get registered, leading to errors from other subsystems when they try to add new sysfs entries to the CPU node. (e.g. topology_sysfs_init()'s use of topology_add_dev() via cpuhp) To fix this, parse the processor container and call acpi_processor_add() for each processor that is discovered like this. The processor container handler is added with acpi_scan_add_handler(), so no detach call will arrive. Qemu TCG describes CPUs using packages in a processor container. Signed-off-by: James Morse <james.morse@arm.com> --- drivers/acpi/acpi_processor.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)