Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
在 2020/1/16 20:24, Peter Maydell 写道: On Wed, 15 Jan 2020 at 10:55, Michael S. Tsirkin wrote: Here's a hopefully better patch. Peter does this address the issue? Signed-off-by: Michael S. Tsirkin diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index f1ac2d7e96..3ab4872bd7 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -16,7 +16,10 @@ * 1. add empty files for new tables, if any, under tests/data/acpi * 2. list any changed files in tests/bios-tables-test-allowed-diff.h * 3. commit the above *before* making changes that affect the tables - * Maintainer: + * + * Contributor or ACPI Maintainer (steps 4-7 need to be redone to resolve conflicts + * in binary commit created in step 6): + * * After 1-3 above tests will pass but ignore differences with the expected files. * You will also notice that tests/bios-tables-test-allowed-diff.h lists * a bunch of files. This is your hint that you need to do the below: @@ -28,13 +31,23 @@ * output. If not - disassemble them yourself in any way you like. * Look at the differences - make sure they make sense and match what the * changes you are merging are supposed to do. + * Save the changes, preferably in form of ASL diff for the commit log in + * step 6. * * 5. From build directory, run: * $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh - * 6. Now commit any changes. - * 7. Before doing a pull request, make sure tests/bios-tables-test-allowed-diff.h - *is empty - this will ensure following changes to ACPI tables will - *be noticed. + * 6. Now commit any changes to the expected binary, include diff from step 4 + *in commit log. + * 7. Before sending patches to the list (Contributor) + *or before doing a pull request (Maintainer), make sure + *tests/bios-tables-test-allowed-diff.h is empty - this will ensure + *following changes to ACPI tables will be noticed. + * + * The resulting patchset/pull request then looks like this: + * - patch 1: list changed files in tests/bios-tables-test-allowed-diff.h. + * - patches 2 - n: real changes, may contain multiple patches. + * - patch n + 1: update golden master binaries and empty Though I drafted the inital text, "2 - n" seems like a expression "2 minus n" for myself after a second glance, especially when we have a "n +1" just below. Maybe we can use a different notation :) + * tests/bios-tables-test-allowed-diff.h */ I think that seems reasonable, but you're the ACPI expert. As long as the patches on list: * can be reviewed by somebody for whether their ACPI changes are correct, including whether the golden-master changes are right * can be applied by a maintainer without having to do anything special * don't break bisection then I'm happy. It sounds like those steps will work for that. diff --git a/roms/seabios b/roms/seabios index f21b5a4aeb..c9ba5276e3 16 --- a/roms/seabios +++ b/roms/seabios @@ -1 +1 @@ -Subproject commit f21b5a4aeb020f2a5e2c6503f906a9349dd2f069 +Subproject commit c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d You have a stray submodule update in your patch, though :-) A file config.mak will be generated in roms/seabios after building qemu, and we may probably add it in our commit by "git commit -a" :) Thanks, Heyi thanks -- PMM .
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
在 2020/1/16 21:25, Igor Mammedov 写道: On Thu, 16 Jan 2020 19:56:19 +0800 Guoheyi wrote: 在 2020/1/5 20:53, Michael S. Tsirkin 写道: On Sun, Jan 05, 2020 at 07:34:01AM -0500, Michael S. Tsirkin wrote: On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote: According to ACPI spec, _ADR should be used for device which is on a bus that has a standard enumeration algorithm. It does not make sense to have a _ADR object for devices which already have _HID and will be enumerated by OSPM. Signed-off-by: Heyi Guo Are you sure? I would think this depends on the ID and the device really. E.g. PCI devices all are expected to have _ADR and some of them have a _HID. To clarify I am not commenting on patches. The spec says this: 6.1.5 _HID (Hardware ID) This object is used to supply OSPM with the device’s PNP ID or ACPI ID. 1 When describing a platform, use of any _HID objects is optional. However, a _HID object must be used to describe any device that will be enumerated by OSPM. OSPM only enumerates a device when no bus enumerator can detect the device ID. For example, devices on an ISA bus are enumerated by OSPM. Use the _ADR object to describe devices enumerated by bus enumerators other than OSPM. Note: "detect the device ID" not "enumerate the device" which I think means there's a driver matching this vendor/device ID. So it seems fine to have _ADR so device is enumerated, and still have _HID e.g. so ACPI driver can be loaded as fallback if there's no bus driver. Note I am not saying the patch itself is not correct. Maybe these devices are not on any standard bus and that is why they should not have _ADR? I have not looked. I am just saying that spec does not seem to imply _HID and _ADR can't coexist. More reading on the spec, I found a statement as below (https://uefi.org/sites/default/files/resources/ACPI_6_3_May16.pdf, section 6.1, on top of page 343): I'd replace 'It does not make sense ...' sentence with pointer to spec and quote below in commit message. Sure; actually I hadn't found this statement in the spec when making the patch :) Thanks, Heyi A device object must contain either an _HID object or an _ADR object, but should not contain both [...] .
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Thu, 16 Jan 2020 19:56:19 +0800 Guoheyi wrote: > 在 2020/1/5 20:53, Michael S. Tsirkin 写道: > > On Sun, Jan 05, 2020 at 07:34:01AM -0500, Michael S. Tsirkin wrote: > >> On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote: > >>> According to ACPI spec, _ADR should be used for device which is on a > >>> bus that has a standard enumeration algorithm. It does not make sense > >>> to have a _ADR object for devices which already have _HID and will be > >>> enumerated by OSPM. > >>> > >>> Signed-off-by: Heyi Guo > >> Are you sure? I would think this depends on the ID and the device > >> really. E.g. PCI devices all are expected to have _ADR and some of them > >> have a _HID. > > > > To clarify I am not commenting on patches. > > The spec says this: > > 6.1.5 _HID (Hardware ID) > > > > This object is used to supply OSPM with the device’s PNP ID or ACPI ID. > > 1 > > > > When describing a platform, use of any _HID objects is optional. > > However, a _HID object must be > > > > used to describe any device that will be enumerated by OSPM. OSPM only > > enumerates a device > > > > when no bus enumerator can detect the device ID. For example, devices > > on an ISA bus are > > > > enumerated by OSPM. Use the _ADR object to describe devices enumerated > > by bus enumerators > > > > other than OSPM. > > > > > > Note: "detect the device ID" not "enumerate the device" which I think > > means there's a driver matching this vendor/device ID. > > > > So it seems fine to have _ADR so device is enumerated, and still have > > _HID e.g. so ACPI driver can be loaded as fallback if there's > > no bus driver. > > > > > > Note I am not saying the patch itself is not correct. > > Maybe these devices are not on any standard bus and that > > is why they should not have _ADR? I have not looked. > > > > I am just saying that spec does not seem to imply _HID and _ADR > > can't coexist. > > More reading on the spec, I found a statement as below > (https://uefi.org/sites/default/files/resources/ACPI_6_3_May16.pdf, > section 6.1, on top of page 343): I'd replace 'It does not make sense ...' sentence with pointer to spec and quote below in commit message. > A device object must contain either an _HID object or an _ADR object, > but should not contain both [...]
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Wed, 15 Jan 2020 at 10:55, Michael S. Tsirkin wrote: > > Here's a hopefully better patch. Peter does this address the issue? > > Signed-off-by: Michael S. Tsirkin > > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index f1ac2d7e96..3ab4872bd7 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -16,7 +16,10 @@ > * 1. add empty files for new tables, if any, under tests/data/acpi > * 2. list any changed files in tests/bios-tables-test-allowed-diff.h > * 3. commit the above *before* making changes that affect the tables > - * Maintainer: > + * > + * Contributor or ACPI Maintainer (steps 4-7 need to be redone to resolve > conflicts > + * in binary commit created in step 6): > + * > * After 1-3 above tests will pass but ignore differences with the expected > files. > * You will also notice that tests/bios-tables-test-allowed-diff.h lists > * a bunch of files. This is your hint that you need to do the below: > @@ -28,13 +31,23 @@ > * output. If not - disassemble them yourself in any way you like. > * Look at the differences - make sure they make sense and match what the > * changes you are merging are supposed to do. > + * Save the changes, preferably in form of ASL diff for the commit log in > + * step 6. > * > * 5. From build directory, run: > * $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh > - * 6. Now commit any changes. > - * 7. Before doing a pull request, make sure > tests/bios-tables-test-allowed-diff.h > - *is empty - this will ensure following changes to ACPI tables will > - *be noticed. > + * 6. Now commit any changes to the expected binary, include diff from step 4 > + *in commit log. > + * 7. Before sending patches to the list (Contributor) > + *or before doing a pull request (Maintainer), make sure > + *tests/bios-tables-test-allowed-diff.h is empty - this will ensure > + *following changes to ACPI tables will be noticed. > + * > + * The resulting patchset/pull request then looks like this: > + * - patch 1: list changed files in tests/bios-tables-test-allowed-diff.h. > + * - patches 2 - n: real changes, may contain multiple patches. > + * - patch n + 1: update golden master binaries and empty > + * tests/bios-tables-test-allowed-diff.h > */ I think that seems reasonable, but you're the ACPI expert. As long as the patches on list: * can be reviewed by somebody for whether their ACPI changes are correct, including whether the golden-master changes are right * can be applied by a maintainer without having to do anything special * don't break bisection then I'm happy. It sounds like those steps will work for that. > diff --git a/roms/seabios b/roms/seabios > index f21b5a4aeb..c9ba5276e3 16 > --- a/roms/seabios > +++ b/roms/seabios > @@ -1 +1 @@ > -Subproject commit f21b5a4aeb020f2a5e2c6503f906a9349dd2f069 > +Subproject commit c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d You have a stray submodule update in your patch, though :-) thanks -- PMM
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
在 2020/1/5 20:53, Michael S. Tsirkin 写道:
On Sun, Jan 05, 2020 at 07:34:01AM -0500, Michael S. Tsirkin wrote:
On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
According to ACPI spec, _ADR should be used for device which is on a
bus that has a standard enumeration algorithm. It does not make sense
to have a _ADR object for devices which already have _HID and will be
enumerated by OSPM.
Signed-off-by: Heyi Guo
Are you sure? I would think this depends on the ID and the device
really. E.g. PCI devices all are expected to have _ADR and some of them
have a _HID.
To clarify I am not commenting on patches.
The spec says this:
6.1.5 _HID (Hardware ID)
This object is used to supply OSPM with the device’s PNP ID or ACPI ID.
1
When describing a platform, use of any _HID objects is optional.
However, a _HID object must be
used to describe any device that will be enumerated by OSPM. OSPM only
enumerates a device
when no bus enumerator can detect the device ID. For example, devices
on an ISA bus are
enumerated by OSPM. Use the _ADR object to describe devices enumerated
by bus enumerators
other than OSPM.
Note: "detect the device ID" not "enumerate the device" which I think
means there's a driver matching this vendor/device ID.
So it seems fine to have _ADR so device is enumerated, and still have
_HID e.g. so ACPI driver can be loaded as fallback if there's
no bus driver.
Note I am not saying the patch itself is not correct.
Maybe these devices are not on any standard bus and that
is why they should not have _ADR? I have not looked.
I am just saying that spec does not seem to imply _HID and _ADR
can't coexist.
More reading on the spec, I found a statement as below
(https://uefi.org/sites/default/files/resources/ACPI_6_3_May16.pdf,
section 6.1, on top of page 343):
A device object must contain either an _HID object or an _ADR object,
but should not contain both
So I think it is at least not recomended to use both _HID and _ADR in a
single device object.
Thanks,
Heyi
CC Corey who added a device with both HID and ADR to x86 recenly.
Apropos Corey, why was HID APP0005 chosen?
---
Cc: Shannon Zhao
Cc: Peter Maydell
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: qemu-...@nongnu.org
Cc: qemu-devel@nongnu.org
---
hw/arm/virt-acpi-build.c | 8
tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
4 files changed, 8 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9f4c7d1889..be752c0ad8 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry
*uart_memmap,
AML_EXCLUSIVE, _irq, 1));
aml_append(dev, aml_name_decl("_CRS", crs));
-/* The _ADR entry is used to link this device to the UART described
- * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
- */
-aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
-
aml_append(scope, dev);
}
@@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
@@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
MemMapEntry *gpio_memmap,
{
Aml *dev = aml_device("GPO0");
aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
Aml *crs = aml_resource_template();
@@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
{
Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
aml_append(scope, dev);
}
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
Please do not include binary changes in acpi patches.
See comment at the top of tests/bios-tables-test.c for documentation
on how to update these.
--
MST
.
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo wrote:
> According to ACPI spec, _ADR should be used for device which is on a
> bus that has a standard enumeration algorithm. It does not make sense
> to have a _ADR object for devices which already have _HID and will be
> enumerated by OSPM.
>
> Signed-off-by: Heyi Guo
Acked-by: Igor Mammedov
>
> ---
> Cc: Shannon Zhao
> Cc: Peter Maydell
> Cc: "Michael S. Tsirkin"
> Cc: Igor Mammedov
> Cc: qemu-...@nongnu.org
> Cc: qemu-devel@nongnu.org
> ---
> hw/arm/virt-acpi-build.c | 8
> tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> 4 files changed, 8 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9f4c7d1889..be752c0ad8 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> MemMapEntry *uart_memmap,
> AML_EXCLUSIVE, _irq, 1));
> aml_append(dev, aml_name_decl("_CRS", crs));
>
> -/* The _ADR entry is used to link this device to the UART described
> - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> - */
> -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> -
> aml_append(scope, dev);
> }
>
> @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> MemMapEntry *memmap,
> aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
> aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> MemMapEntry *gpio_memmap,
> {
> Aml *dev = aml_device("GPO0");
> aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>
> Aml *crs = aml_resource_template();
> @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> {
> Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> aml_append(scope, dev);
> }
> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> index
> b5895cb22446860a0b9be3d32ec856feb388be4c..a759ff739a071d5fbf50519a6aea296e5e0f1e0c
> 100644
> GIT binary patch
> delta 72
> zcmbO@f$>*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>
> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
> index
> 69ad844f65d047973a3e55198beecd45a35b8fce..6e5cc61977e4cd24f765fec0693f75a528c144c1
> 100644
> GIT binary patch
> delta 72
> zcmX>#i*fTTMlP3Nmk?uL1_q|eiCof5eHSLGt1wzk^tIeLL4lL8ZSqD=gU!!5U7RH)
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>03)CjmjD0&
>
> delta 94
> zcmdlyi}BPfMlP3Nmk=*s1_q}3iCof5t(PXMt1!8;O!Tqj;)r*23F2X3VBp-?s=&$E
> tGkF=O0W(l&^JPwVXL
> diff --git a/tests/data/acpi/virt/DSDT.numamem
> b/tests/data/acpi/virt/DSDT.numamem
> index
> b5895cb22446860a0b9be3d32ec856feb388be4c..a759ff739a071d5fbf50519a6aea296e5e0f1e0c
> 100644
> GIT binary patch
> delta 72
> zcmbO@f$>*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Wed, Jan 15, 2020 at 05:25:50PM +0800, Guoheyi wrote: > > 在 2020/1/15 14:30, Michael S. Tsirkin 写道: > > Problem is IASL disassembler still doesn't work on all hosts > > we want to support. And its output isn't really stable enough > > to act as a golden master. > > > > Until we have a better tool, I propose the contributor just follows all > > steps 1-6. The reason they have been listed as maintainer action items > > is really just so that multiple patches affecting same ACPI table > > can be applied, with maintainer resolving conflicts himself. > > But this job can be pushed to contributors if as in the case of ARM > > maintainer isn't really interested in reading ACPI code anyway. > > > > So I propose the following patch - comments? > > > > Signed-off-by: Michael S. Tsirkin > > > > > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > > index f1ac2d7e96..3a6a3e7257 100644 > > --- a/tests/qtest/bios-tables-test.c > > +++ b/tests/qtest/bios-tables-test.c > > @@ -16,7 +16,10 @@ > >* 1. add empty files for new tables, if any, under tests/data/acpi > >* 2. list any changed files in tests/bios-tables-test-allowed-diff.h > >* 3. commit the above *before* making changes that affect the tables > > - * Maintainer: > > + * > > + * Contributor or ACPI Maintainer (steps 4-7 need to be redone to resolve > > conflicts > > + * in binary commit created in step 6): > > + * > >* After 1-3 above tests will pass but ignore differences with the > > expected files. > >* You will also notice that tests/bios-tables-test-allowed-diff.h lists > >* a bunch of files. This is your hint that you need to do the below: > > @@ -28,13 +31,17 @@ > >* output. If not - disassemble them yourself in any way you like. > >* Look at the differences - make sure they make sense and match what the > >* changes you are merging are supposed to do. > > + * Save the changes, preferably in form of ASL diff for the the commit log > > in > NIT: 2 "the" before commit log > > + * step 6. > >* > >* 5. From build directory, run: > >* $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh > > - * 6. Now commit any changes. > > - * 7. Before doing a pull request, make sure > > tests/bios-tables-test-allowed-diff.h > > - *is empty - this will ensure following changes to ACPI tables will > > - *be noticed. > > + * 6. Now commit any changes to the expected binary, include diff from > > step 4 > > + *in commit log. > > + * 7. Before sending patches to the list (Contributor) > > + *or before doing a pull request (Maintainer), make sure > > + *tests/bios-tables-test-allowed-diff.h is empty - this will ensure > > + *following changes to ACPI tables will be noticed. > >*/ > > For contributors doing the full work, does that mean the patchset sent to > the list contains the following parts? > > 1. patch 1: list changed files in tests/bios-tables-test-allowed-diff.h. > > 2. patches 2-n: real changes, may contain multiple patches. > > 3. patch n+1: update golden master binaries and empty > tests/bios-tables-test-allowed-diff.h > > Thanks, > > Heyi Here's a hopefully better patch. Peter does this address the issue? Signed-off-by: Michael S. Tsirkin diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index f1ac2d7e96..3ab4872bd7 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -16,7 +16,10 @@ * 1. add empty files for new tables, if any, under tests/data/acpi * 2. list any changed files in tests/bios-tables-test-allowed-diff.h * 3. commit the above *before* making changes that affect the tables - * Maintainer: + * + * Contributor or ACPI Maintainer (steps 4-7 need to be redone to resolve conflicts + * in binary commit created in step 6): + * * After 1-3 above tests will pass but ignore differences with the expected files. * You will also notice that tests/bios-tables-test-allowed-diff.h lists * a bunch of files. This is your hint that you need to do the below: @@ -28,13 +31,23 @@ * output. If not - disassemble them yourself in any way you like. * Look at the differences - make sure they make sense and match what the * changes you are merging are supposed to do. + * Save the changes, preferably in form of ASL diff for the commit log in + * step 6. * * 5. From build directory, run: * $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh - * 6. Now commit any changes. - * 7. Before doing a pull request, make sure tests/bios-tables-test-allowed-diff.h - *is empty - this will ensure following changes to ACPI tables will - *be noticed. + * 6. Now commit any changes to the expected binary, include diff from step 4 + *in commit log. + * 7. Before sending patches to the list (Contributor) + *or before doing a pull request (Maintainer), make sure + *tests/bios-tables-test-allowed-diff.h is empty - this will ensure + *following changes to
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Wed, Jan 15, 2020 at 05:25:50PM +0800, Guoheyi wrote: > > 在 2020/1/15 14:30, Michael S. Tsirkin 写道: > > Problem is IASL disassembler still doesn't work on all hosts > > we want to support. And its output isn't really stable enough > > to act as a golden master. > > > > Until we have a better tool, I propose the contributor just follows all > > steps 1-6. The reason they have been listed as maintainer action items > > is really just so that multiple patches affecting same ACPI table > > can be applied, with maintainer resolving conflicts himself. > > But this job can be pushed to contributors if as in the case of ARM > > maintainer isn't really interested in reading ACPI code anyway. > > > > So I propose the following patch - comments? > > > > Signed-off-by: Michael S. Tsirkin > > > > > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > > index f1ac2d7e96..3a6a3e7257 100644 > > --- a/tests/qtest/bios-tables-test.c > > +++ b/tests/qtest/bios-tables-test.c > > @@ -16,7 +16,10 @@ > >* 1. add empty files for new tables, if any, under tests/data/acpi > >* 2. list any changed files in tests/bios-tables-test-allowed-diff.h > >* 3. commit the above *before* making changes that affect the tables > > - * Maintainer: > > + * > > + * Contributor or ACPI Maintainer (steps 4-7 need to be redone to resolve > > conflicts > > + * in binary commit created in step 6): > > + * > >* After 1-3 above tests will pass but ignore differences with the > > expected files. > >* You will also notice that tests/bios-tables-test-allowed-diff.h lists > >* a bunch of files. This is your hint that you need to do the below: > > @@ -28,13 +31,17 @@ > >* output. If not - disassemble them yourself in any way you like. > >* Look at the differences - make sure they make sense and match what the > >* changes you are merging are supposed to do. > > + * Save the changes, preferably in form of ASL diff for the the commit log > > in > NIT: 2 "the" before commit log > > + * step 6. > >* > >* 5. From build directory, run: > >* $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh > > - * 6. Now commit any changes. > > - * 7. Before doing a pull request, make sure > > tests/bios-tables-test-allowed-diff.h > > - *is empty - this will ensure following changes to ACPI tables will > > - *be noticed. > > + * 6. Now commit any changes to the expected binary, include diff from > > step 4 > > + *in commit log. > > + * 7. Before sending patches to the list (Contributor) > > + *or before doing a pull request (Maintainer), make sure > > + *tests/bios-tables-test-allowed-diff.h is empty - this will ensure > > + *following changes to ACPI tables will be noticed. > >*/ > > For contributors doing the full work, does that mean the patchset sent to > the list contains the following parts? > > 1. patch 1: list changed files in tests/bios-tables-test-allowed-diff.h. > > 2. patches 2-n: real changes, may contain multiple patches. > > 3. patch n+1: update golden master binaries and empty > tests/bios-tables-test-allowed-diff.h > > Thanks, > > Heyi Yes. > > #include "qemu/osdep.h" > > > > > > .
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
在 2020/1/15 14:30, Michael S. Tsirkin 写道: Problem is IASL disassembler still doesn't work on all hosts we want to support. And its output isn't really stable enough to act as a golden master. Until we have a better tool, I propose the contributor just follows all steps 1-6. The reason they have been listed as maintainer action items is really just so that multiple patches affecting same ACPI table can be applied, with maintainer resolving conflicts himself. But this job can be pushed to contributors if as in the case of ARM maintainer isn't really interested in reading ACPI code anyway. So I propose the following patch - comments? Signed-off-by: Michael S. Tsirkin diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index f1ac2d7e96..3a6a3e7257 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -16,7 +16,10 @@ * 1. add empty files for new tables, if any, under tests/data/acpi * 2. list any changed files in tests/bios-tables-test-allowed-diff.h * 3. commit the above *before* making changes that affect the tables - * Maintainer: + * + * Contributor or ACPI Maintainer (steps 4-7 need to be redone to resolve conflicts + * in binary commit created in step 6): + * * After 1-3 above tests will pass but ignore differences with the expected files. * You will also notice that tests/bios-tables-test-allowed-diff.h lists * a bunch of files. This is your hint that you need to do the below: @@ -28,13 +31,17 @@ * output. If not - disassemble them yourself in any way you like. * Look at the differences - make sure they make sense and match what the * changes you are merging are supposed to do. + * Save the changes, preferably in form of ASL diff for the the commit log in NIT: 2 "the" before commit log + * step 6. * * 5. From build directory, run: * $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh - * 6. Now commit any changes. - * 7. Before doing a pull request, make sure tests/bios-tables-test-allowed-diff.h - *is empty - this will ensure following changes to ACPI tables will - *be noticed. + * 6. Now commit any changes to the expected binary, include diff from step 4 + *in commit log. + * 7. Before sending patches to the list (Contributor) + *or before doing a pull request (Maintainer), make sure + *tests/bios-tables-test-allowed-diff.h is empty - this will ensure + *following changes to ACPI tables will be noticed. */ For contributors doing the full work, does that mean the patchset sent to the list contains the following parts? 1. patch 1: list changed files in tests/bios-tables-test-allowed-diff.h. 2. patches 2-n: real changes, may contain multiple patches. 3. patch n+1: update golden master binaries and empty tests/bios-tables-test-allowed-diff.h Thanks, Heyi #include "qemu/osdep.h" .
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
Problem is IASL disassembler still doesn't work on all hosts we want to support. And its output isn't really stable enough to act as a golden master. Until we have a better tool, I propose the contributor just follows all steps 1-6. The reason they have been listed as maintainer action items is really just so that multiple patches affecting same ACPI table can be applied, with maintainer resolving conflicts himself. But this job can be pushed to contributors if as in the case of ARM maintainer isn't really interested in reading ACPI code anyway. So I propose the following patch - comments? Signed-off-by: Michael S. Tsirkin diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index f1ac2d7e96..3a6a3e7257 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -16,7 +16,10 @@ * 1. add empty files for new tables, if any, under tests/data/acpi * 2. list any changed files in tests/bios-tables-test-allowed-diff.h * 3. commit the above *before* making changes that affect the tables - * Maintainer: + * + * Contributor or ACPI Maintainer (steps 4-7 need to be redone to resolve conflicts + * in binary commit created in step 6): + * * After 1-3 above tests will pass but ignore differences with the expected files. * You will also notice that tests/bios-tables-test-allowed-diff.h lists * a bunch of files. This is your hint that you need to do the below: @@ -28,13 +31,17 @@ * output. If not - disassemble them yourself in any way you like. * Look at the differences - make sure they make sense and match what the * changes you are merging are supposed to do. + * Save the changes, preferably in form of ASL diff for the the commit log in + * step 6. * * 5. From build directory, run: * $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh - * 6. Now commit any changes. - * 7. Before doing a pull request, make sure tests/bios-tables-test-allowed-diff.h - *is empty - this will ensure following changes to ACPI tables will - *be noticed. + * 6. Now commit any changes to the expected binary, include diff from step 4 + *in commit log. + * 7. Before sending patches to the list (Contributor) + *or before doing a pull request (Maintainer), make sure + *tests/bios-tables-test-allowed-diff.h is empty - this will ensure + *following changes to ACPI tables will be noticed. */ #include "qemu/osdep.h"
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
Hi Peter, Michael, Have we come to conclusion on how to submit patches for ARM ACPI tables? Some rough thoughts: is it possible to use the disassembled ASL file as the 'golden master' data? One problem I can imagine is that this may introduce dependency on the version of iASL tool. If so, how about adding acpica source code as a submodule, just like what we did for the device tree compile "dtc". There may be much more which I missed; looking forward to your comments. Thanks, Heyi 在 2020/1/6 23:51, Peter Maydell 写道: On Sun, 5 Jan 2020 at 12:34, Michael S. Tsirkin wrote: On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote: hw/arm/virt-acpi-build.c | 8 tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes 4 files changed, 8 deletions(-) Please do not include binary changes in acpi patches. See comment at the top of tests/bios-tables-test.c for documentation on how to update these. If you want the patches not to include binary changes then you will need to take these yourself through your own tree. As the Arm subtree maintainer I am not going to follow a specific process for acpi related patches that requires me to do anything other than "apply patches from email, test them, send pull request". I also have no way to identify whether any differences that I might see if I disassembled the ACPI tables make sense, as that comment suggests I should be doing. The differences in the tables need to be checked by the people reviewing the patches, which will not be me for anything ACPI related -- I just don't know enough about the ACPI specs. Patches should be self contained, including updating test cases as required. The underlying problem here is that the 'golden master' data for the acpi tests is a pile of binary blobs rather than something that's human readable and reviewable as part of a patch. thanks -- PMM .
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
在 2020/1/13 23:26, Andrew Jones 写道:
On Mon, Jan 13, 2020 at 09:57:55PM +0800, Guoheyi wrote:
在 2020/1/13 20:08, Igor Mammedov 写道:
On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo wrote:
According to ACPI spec, _ADR should be used for device which is on a
bus that has a standard enumeration algorithm. It does not make sense
to have a _ADR object for devices which already have _HID and will be
enumerated by OSPM.
Signed-off-by: Heyi Guo
Are you sure it's does not make sense?
Have you checked commit f264d51d8, that added _ADR?
I searched in SPCR spec and ACPI spec, but didn't find such requirement for
serial port device description.
Hi Andrew,
Could you help to explain the reason?
tl;dr: It was a mistake and I agree with removing _ADR from SPCR.
The long version is that ACPI support for ARM took a long time to get
merged upstream. In the meantime Linaro and Red Hat had ACPI support
in their downstream trees. The initial, never upstreamed implementation
of Linux SPCR support used _ADR to find the console UART (probably
because some vendor hacked their ACPI tables that way). I made the
mistake of using the out-of-tree Linux kernel code as my "specification".
Upstream kernels never had this problem and I don't think we need to
worry about any of those downstream kernels which did. They'd be five
years old by now anyway.
Thanks,
drew
Thanks for your confirmation, Andrew.
Hi Igor,
On arm64 physical machine, we didn't have _ADR for SPCR serial port
device attached to system bus either, and we never saw any problem.
Thanks,
Heyi
Thanks,
Heyi
---
Cc: Shannon Zhao
Cc: Peter Maydell
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: qemu-...@nongnu.org
Cc: qemu-devel@nongnu.org
---
hw/arm/virt-acpi-build.c | 8
tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
4 files changed, 8 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9f4c7d1889..be752c0ad8 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry
*uart_memmap,
AML_EXCLUSIVE, _irq, 1));
aml_append(dev, aml_name_decl("_CRS", crs));
-/* The _ADR entry is used to link this device to the UART described
- * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
- */
-aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
-
aml_append(scope, dev);
}
@@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry
*memmap,
aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
@@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
MemMapEntry *gpio_memmap,
{
Aml *dev = aml_device("GPO0");
aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
Aml *crs = aml_resource_template();
@@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
{
Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
aml_append(scope, dev);
}
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index
b5895cb22446860a0b9be3d32ec856feb388be4c..a759ff739a071d5fbf50519a6aea296e5e0f1e0c
100644
GIT binary patch
delta 72
zcmbO@f$>*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
delta 94
zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
index
69ad844f65d047973a3e55198beecd45a35b8fce..6e5cc61977e4cd24f765fec0693f75a528c144c1
100644
GIT binary patch
delta 72
zcmX>#i*fTTMlP3Nmk?uL1_q|eiCof5eHSLGt1wzk^tIeLL4lL8ZSqD=gU!!5U7RH)
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>03)CjmjD0&
delta 94
zcmdlyi}BPfMlP3Nmk=*s1_q}3iCof5t(PXMt1!8;O!Tqj;)r*23F2X3VBp-?s=&$E
tGkF=O0W(l&^JPwVXL*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
delta 94
zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
.
.
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Mon, Jan 13, 2020 at 09:57:55PM +0800, Guoheyi wrote:
>
> 在 2020/1/13 20:08, Igor Mammedov 写道:
> > On Thu, 19 Dec 2019 14:47:59 +0800
> > Heyi Guo wrote:
> >
> > > According to ACPI spec, _ADR should be used for device which is on a
> > > bus that has a standard enumeration algorithm. It does not make sense
> > > to have a _ADR object for devices which already have _HID and will be
> > > enumerated by OSPM.
> > >
> > > Signed-off-by: Heyi Guo
> > Are you sure it's does not make sense?
> > Have you checked commit f264d51d8, that added _ADR?
>
> I searched in SPCR spec and ACPI spec, but didn't find such requirement for
> serial port device description.
>
> Hi Andrew,
>
> Could you help to explain the reason?
tl;dr: It was a mistake and I agree with removing _ADR from SPCR.
The long version is that ACPI support for ARM took a long time to get
merged upstream. In the meantime Linaro and Red Hat had ACPI support
in their downstream trees. The initial, never upstreamed implementation
of Linux SPCR support used _ADR to find the console UART (probably
because some vendor hacked their ACPI tables that way). I made the
mistake of using the out-of-tree Linux kernel code as my "specification".
Upstream kernels never had this problem and I don't think we need to
worry about any of those downstream kernels which did. They'd be five
years old by now anyway.
Thanks,
drew
>
> Thanks,
>
> Heyi
>
>
>
> >
> > > ---
> > > Cc: Shannon Zhao
> > > Cc: Peter Maydell
> > > Cc: "Michael S. Tsirkin"
> > > Cc: Igor Mammedov
> > > Cc: qemu-...@nongnu.org
> > > Cc: qemu-devel@nongnu.org
> > > ---
> > > hw/arm/virt-acpi-build.c | 8
> > > tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> > > tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> > > tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> > > 4 files changed, 8 deletions(-)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 9f4c7d1889..be752c0ad8 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> > > MemMapEntry *uart_memmap,
> > >AML_EXCLUSIVE, _irq, 1));
> > > aml_append(dev, aml_name_decl("_CRS", crs));
> > > -/* The _ADR entry is used to link this device to the UART described
> > > - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> > > - */
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > > -
> > > aml_append(scope, dev);
> > > }
> > > @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> > > MemMapEntry *memmap,
> > > aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> > > aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> > > aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0
> > > Device")));
> > > aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > > @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> > > MemMapEntry *gpio_memmap,
> > > {
> > > Aml *dev = aml_device("GPO0");
> > > aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > > Aml *crs = aml_resource_template();
> > > @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > {
> > > Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> > > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > > aml_append(scope, dev);
> > > }
> > > diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> > > index
> > > b5895cb22446860a0b9be3d32ec856feb388be4c..a759ff739a071d5fbf50519a6aea296e5e0f1e0c
> > > 100644
> > > GIT binary patch
> > > delta 72
> > > zcmbO@f$>*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
> > > c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
> > >
> > > delta 94
> > > zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> > > tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
> > >
> > > diff --git a/tests/data/acpi/virt/DSDT.memhp
> > > b/tests/data/acpi/virt/DSDT.memhp
> > > index
> > > 69ad844f65d047973a3e55198beecd45a35b8fce..6e5cc61977e4cd24f765fec0693f75a528c144c1
> > > 100644
> > > GIT binary patch
> > > delta 72
> > > zcmX>#i*fTTMlP3Nmk?uL1_q|eiCof5eHSLGt1wzk^tIeLL4lL8ZSqD=gU!!5U7RH)
> > > c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>03)CjmjD0&
> > >
> > > delta 94
> > >
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
在 2020/1/13 20:08, Igor Mammedov 写道:
On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo wrote:
According to ACPI spec, _ADR should be used for device which is on a
bus that has a standard enumeration algorithm. It does not make sense
to have a _ADR object for devices which already have _HID and will be
enumerated by OSPM.
Signed-off-by: Heyi Guo
Are you sure it's does not make sense?
Have you checked commit f264d51d8, that added _ADR?
I searched in SPCR spec and ACPI spec, but didn't find such requirement
for serial port device description.
Hi Andrew,
Could you help to explain the reason?
Thanks,
Heyi
---
Cc: Shannon Zhao
Cc: Peter Maydell
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: qemu-...@nongnu.org
Cc: qemu-devel@nongnu.org
---
hw/arm/virt-acpi-build.c | 8
tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
4 files changed, 8 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9f4c7d1889..be752c0ad8 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry
*uart_memmap,
AML_EXCLUSIVE, _irq, 1));
aml_append(dev, aml_name_decl("_CRS", crs));
-/* The _ADR entry is used to link this device to the UART described
- * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
- */
-aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
-
aml_append(scope, dev);
}
@@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
@@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
MemMapEntry *gpio_memmap,
{
Aml *dev = aml_device("GPO0");
aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
Aml *crs = aml_resource_template();
@@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
{
Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
aml_append(scope, dev);
}
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index
b5895cb22446860a0b9be3d32ec856feb388be4c..a759ff739a071d5fbf50519a6aea296e5e0f1e0c
100644
GIT binary patch
delta 72
zcmbO@f$>*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
delta 94
zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
index
69ad844f65d047973a3e55198beecd45a35b8fce..6e5cc61977e4cd24f765fec0693f75a528c144c1
100644
GIT binary patch
delta 72
zcmX>#i*fTTMlP3Nmk?uL1_q|eiCof5eHSLGt1wzk^tIeLL4lL8ZSqD=gU!!5U7RH)
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>03)CjmjD0&
delta 94
zcmdlyi}BPfMlP3Nmk=*s1_q}3iCof5t(PXMt1!8;O!Tqj;)r*23F2X3VBp-?s=&$E
tGkF=O0W(l&^JPwVXL*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
delta 94
zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
.
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo wrote:
> According to ACPI spec, _ADR should be used for device which is on a
> bus that has a standard enumeration algorithm. It does not make sense
> to have a _ADR object for devices which already have _HID and will be
> enumerated by OSPM.
>
> Signed-off-by: Heyi Guo
Are you sure it's does not make sense?
Have you checked commit f264d51d8, that added _ADR?
> ---
> Cc: Shannon Zhao
> Cc: Peter Maydell
> Cc: "Michael S. Tsirkin"
> Cc: Igor Mammedov
> Cc: qemu-...@nongnu.org
> Cc: qemu-devel@nongnu.org
> ---
> hw/arm/virt-acpi-build.c | 8
> tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> 4 files changed, 8 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9f4c7d1889..be752c0ad8 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> MemMapEntry *uart_memmap,
> AML_EXCLUSIVE, _irq, 1));
> aml_append(dev, aml_name_decl("_CRS", crs));
>
> -/* The _ADR entry is used to link this device to the UART described
> - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> - */
> -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> -
> aml_append(scope, dev);
> }
>
> @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> MemMapEntry *memmap,
> aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
> aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> MemMapEntry *gpio_memmap,
> {
> Aml *dev = aml_device("GPO0");
> aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>
> Aml *crs = aml_resource_template();
> @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> {
> Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> aml_append(scope, dev);
> }
> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> index
> b5895cb22446860a0b9be3d32ec856feb388be4c..a759ff739a071d5fbf50519a6aea296e5e0f1e0c
> 100644
> GIT binary patch
> delta 72
> zcmbO@f$>*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>
> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
> index
> 69ad844f65d047973a3e55198beecd45a35b8fce..6e5cc61977e4cd24f765fec0693f75a528c144c1
> 100644
> GIT binary patch
> delta 72
> zcmX>#i*fTTMlP3Nmk?uL1_q|eiCof5eHSLGt1wzk^tIeLL4lL8ZSqD=gU!!5U7RH)
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>03)CjmjD0&
>
> delta 94
> zcmdlyi}BPfMlP3Nmk=*s1_q}3iCof5t(PXMt1!8;O!Tqj;)r*23F2X3VBp-?s=&$E
> tGkF=O0W(l&^JPwVXL
> diff --git a/tests/data/acpi/virt/DSDT.numamem
> b/tests/data/acpi/virt/DSDT.numamem
> index
> b5895cb22446860a0b9be3d32ec856feb388be4c..a759ff739a071d5fbf50519a6aea296e5e0f1e0c
> 100644
> GIT binary patch
> delta 72
> zcmbO@f$>*ABbQ6COUN_q{66S<_BT5Bh^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
Hi Michael,
Any more comments? Or shall I refine the commit message?
Thanks,
Heyi
在 2020/1/6 10:10, Guoheyi 写道:
在 2020/1/5 20:53, Michael S. Tsirkin 写道:
On Sun, Jan 05, 2020 at 07:34:01AM -0500, Michael S. Tsirkin wrote:
On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
According to ACPI spec, _ADR should be used for device which is on a
bus that has a standard enumeration algorithm. It does not make sense
to have a _ADR object for devices which already have _HID and will be
enumerated by OSPM.
Signed-off-by: Heyi Guo
Are you sure? I would think this depends on the ID and the device
really. E.g. PCI devices all are expected to have _ADR and some of them
have a _HID.
To clarify I am not commenting on patches.
The spec says this:
6.1.5 _HID (Hardware ID)
This object is used to supply OSPM with the device’s PNP ID or
ACPI ID. 1
When describing a platform, use of any _HID objects is optional.
However, a _HID object must be
used to describe any device that will be enumerated by OSPM. OSPM
only enumerates a device
when no bus enumerator can detect the device ID. For example,
devices on an ISA bus are
enumerated by OSPM. Use the _ADR object to describe devices
enumerated by bus enumerators
other than OSPM.
Note: "detect the device ID" not "enumerate the device" which I think
means there's a driver matching this vendor/device ID.
So it seems fine to have _ADR so device is enumerated, and still have
_HID e.g. so ACPI driver can be loaded as fallback if there's
no bus driver.
Note I am not saying the patch itself is not correct.
Maybe these devices are not on any standard bus and that
is why they should not have _ADR? I have not looked.
I am just saying that spec does not seem to imply _HID and _ADR
can't coexist.
That's true; I did't find such statement either. Maybe what we can say
is that the _ADR is senseless here.
Thanks,
Heyi
CC Corey who added a device with both HID and ADR to x86 recenly.
Apropos Corey, why was HID APP0005 chosen?
---
Cc: Shannon Zhao
Cc: Peter Maydell
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: qemu-...@nongnu.org
Cc: qemu-devel@nongnu.org
---
hw/arm/virt-acpi-build.c | 8
tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
4 files changed, 8 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9f4c7d1889..be752c0ad8 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
MemMapEntry *uart_memmap,
AML_EXCLUSIVE, _irq, 1));
aml_append(dev, aml_name_decl("_CRS", crs));
- /* The _ADR entry is used to link this device to the UART
described
- * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
- */
- aml_append(dev, aml_name_decl("_ADR",
aml_int(uart_memmap->base)));
-
aml_append(scope, dev);
}
@@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope,
const MemMapEntry *memmap,
aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
- aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0
Device")));
aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
@@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope,
const MemMapEntry *gpio_memmap,
{
Aml *dev = aml_device("GPO0");
aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
- aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
Aml *crs = aml_resource_template();
@@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
{
Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
- aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
aml_append(scope, dev);
}
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
Please do not include binary changes in acpi patches.
See comment at the top of tests/bios-tables-test.c for documentation
on how to update these.
--
MST
.
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Sun, 5 Jan 2020 at 12:34, Michael S. Tsirkin wrote: > > On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote: > > hw/arm/virt-acpi-build.c | 8 > > tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes > > tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes > > tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes > > 4 files changed, 8 deletions(-) > Please do not include binary changes in acpi patches. > > See comment at the top of tests/bios-tables-test.c for documentation > on how to update these. If you want the patches not to include binary changes then you will need to take these yourself through your own tree. As the Arm subtree maintainer I am not going to follow a specific process for acpi related patches that requires me to do anything other than "apply patches from email, test them, send pull request". I also have no way to identify whether any differences that I might see if I disassembled the ACPI tables make sense, as that comment suggests I should be doing. The differences in the tables need to be checked by the people reviewing the patches, which will not be me for anything ACPI related -- I just don't know enough about the ACPI specs. Patches should be self contained, including updating test cases as required. The underlying problem here is that the 'golden master' data for the acpi tests is a pile of binary blobs rather than something that's human readable and reviewable as part of a patch. thanks -- PMM
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Mon, Jan 06, 2020 at 04:39:51AM -0500, Michael S. Tsirkin wrote:
> On Sun, Jan 05, 2020 at 04:54:20PM -0600, Corey Minyard wrote:
> > On Sun, Jan 05, 2020 at 07:33:55AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
> > > > According to ACPI spec, _ADR should be used for device which is on a
> > > > bus that has a standard enumeration algorithm. It does not make sense
> > > > to have a _ADR object for devices which already have _HID and will be
> > > > enumerated by OSPM.
> > > >
> > > > Signed-off-by: Heyi Guo
> > >
> > > Are you sure? I would think this depends on the ID and the device
> > > really. E.g. PCI devices all are expected to have _ADR and some of them
> > > have a _HID.
> >
> > That's my understanding, too.
> >
> > >
> > > CC Corey who added a device with both HID and ADR to x86 recenly.
> > >
> > > Apropos Corey, why was HID APP0005 chosen?
> >
> > I don't remember. I thought I had looked it up someplace, but I didn't
> > document it.
> >
> > >From reading the spec, I believe you could safely delete the _HID and it
> > would be fine. However, I don't see anything that says it can't be
> > there, either. But it is extraneous.
> >
> > Searching on the web a bit, I see that the APP0005 has confused windows.
>
> Could you post the link you found pls?
https://bugs.launchpad.net/qemu/+bug/1856724
>
> > It does look to be wrong. Maybe deleting it would be a good idea.
> >
> > -corey
> >
> > >
> > > > ---
> > > > Cc: Shannon Zhao
> > > > Cc: Peter Maydell
> > > > Cc: "Michael S. Tsirkin"
> > > > Cc: Igor Mammedov
> > > > Cc: qemu-...@nongnu.org
> > > > Cc: qemu-devel@nongnu.org
> > > > ---
> > > > hw/arm/virt-acpi-build.c | 8
> > > > tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> > > > tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> > > > tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> > > > 4 files changed, 8 deletions(-)
> > > >
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 9f4c7d1889..be752c0ad8 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> > > > MemMapEntry *uart_memmap,
> > > > AML_EXCLUSIVE, _irq, 1));
> > > > aml_append(dev, aml_name_decl("_CRS", crs));
> > > >
> > > > -/* The _ADR entry is used to link this device to the UART described
> > > > - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> > > > - */
> > > > -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > > > -
> > > > aml_append(scope, dev);
> > > > }
> > > >
> > > > @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> > > > MemMapEntry *memmap,
> > > > aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> > > > aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> > > > aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> > > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > > aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> > > > aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0
> > > > Device")));
> > > > aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > > > @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> > > > MemMapEntry *gpio_memmap,
> > > > {
> > > > Aml *dev = aml_device("GPO0");
> > > > aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> > > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > > >
> > > > Aml *crs = aml_resource_template();
> > > > @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > > {
> > > > Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> > > > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> > > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > > > aml_append(scope, dev);
> > > > }
> > > > diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> > >
> > >
> > > Please do not include binary changes in acpi patches.
> > >
> > > See comment at the top of tests/bios-tables-test.c for documentation
> > > on how to update these.
> > >
> > > --
> > > MST
> > >
>
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Sun, Jan 05, 2020 at 04:54:20PM -0600, Corey Minyard wrote:
> On Sun, Jan 05, 2020 at 07:33:55AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
> > > According to ACPI spec, _ADR should be used for device which is on a
> > > bus that has a standard enumeration algorithm. It does not make sense
> > > to have a _ADR object for devices which already have _HID and will be
> > > enumerated by OSPM.
> > >
> > > Signed-off-by: Heyi Guo
> >
> > Are you sure? I would think this depends on the ID and the device
> > really. E.g. PCI devices all are expected to have _ADR and some of them
> > have a _HID.
>
> That's my understanding, too.
>
> >
> > CC Corey who added a device with both HID and ADR to x86 recenly.
> >
> > Apropos Corey, why was HID APP0005 chosen?
>
> I don't remember. I thought I had looked it up someplace, but I didn't
> document it.
>
> >From reading the spec, I believe you could safely delete the _HID and it
> would be fine. However, I don't see anything that says it can't be
> there, either. But it is extraneous.
>
> Searching on the web a bit, I see that the APP0005 has confused windows.
Could you post the link you found pls?
> It does look to be wrong. Maybe deleting it would be a good idea.
>
> -corey
>
> >
> > > ---
> > > Cc: Shannon Zhao
> > > Cc: Peter Maydell
> > > Cc: "Michael S. Tsirkin"
> > > Cc: Igor Mammedov
> > > Cc: qemu-...@nongnu.org
> > > Cc: qemu-devel@nongnu.org
> > > ---
> > > hw/arm/virt-acpi-build.c | 8
> > > tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> > > tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> > > tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> > > 4 files changed, 8 deletions(-)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 9f4c7d1889..be752c0ad8 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> > > MemMapEntry *uart_memmap,
> > > AML_EXCLUSIVE, _irq, 1));
> > > aml_append(dev, aml_name_decl("_CRS", crs));
> > >
> > > -/* The _ADR entry is used to link this device to the UART described
> > > - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> > > - */
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > > -
> > > aml_append(scope, dev);
> > > }
> > >
> > > @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> > > MemMapEntry *memmap,
> > > aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> > > aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> > > aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
> > > aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > > @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> > > MemMapEntry *gpio_memmap,
> > > {
> > > Aml *dev = aml_device("GPO0");
> > > aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > >
> > > Aml *crs = aml_resource_template();
> > > @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > > {
> > > Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> > > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > > aml_append(scope, dev);
> > > }
> > > diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> >
> >
> > Please do not include binary changes in acpi patches.
> >
> > See comment at the top of tests/bios-tables-test.c for documentation
> > on how to update these.
> >
> > --
> > MST
> >
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
在 2020/1/5 20:53, Michael S. Tsirkin 写道:
On Sun, Jan 05, 2020 at 07:34:01AM -0500, Michael S. Tsirkin wrote:
On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
According to ACPI spec, _ADR should be used for device which is on a
bus that has a standard enumeration algorithm. It does not make sense
to have a _ADR object for devices which already have _HID and will be
enumerated by OSPM.
Signed-off-by: Heyi Guo
Are you sure? I would think this depends on the ID and the device
really. E.g. PCI devices all are expected to have _ADR and some of them
have a _HID.
To clarify I am not commenting on patches.
The spec says this:
6.1.5 _HID (Hardware ID)
This object is used to supply OSPM with the device’s PNP ID or ACPI ID.
1
When describing a platform, use of any _HID objects is optional.
However, a _HID object must be
used to describe any device that will be enumerated by OSPM. OSPM only
enumerates a device
when no bus enumerator can detect the device ID. For example, devices
on an ISA bus are
enumerated by OSPM. Use the _ADR object to describe devices enumerated
by bus enumerators
other than OSPM.
Note: "detect the device ID" not "enumerate the device" which I think
means there's a driver matching this vendor/device ID.
So it seems fine to have _ADR so device is enumerated, and still have
_HID e.g. so ACPI driver can be loaded as fallback if there's
no bus driver.
Note I am not saying the patch itself is not correct.
Maybe these devices are not on any standard bus and that
is why they should not have _ADR? I have not looked.
I am just saying that spec does not seem to imply _HID and _ADR
can't coexist.
That's true; I did't find such statement either. Maybe what we can say
is that the _ADR is senseless here.
Thanks,
Heyi
CC Corey who added a device with both HID and ADR to x86 recenly.
Apropos Corey, why was HID APP0005 chosen?
---
Cc: Shannon Zhao
Cc: Peter Maydell
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: qemu-...@nongnu.org
Cc: qemu-devel@nongnu.org
---
hw/arm/virt-acpi-build.c | 8
tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
4 files changed, 8 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9f4c7d1889..be752c0ad8 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry
*uart_memmap,
AML_EXCLUSIVE, _irq, 1));
aml_append(dev, aml_name_decl("_CRS", crs));
-/* The _ADR entry is used to link this device to the UART described
- * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
- */
-aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
-
aml_append(scope, dev);
}
@@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
@@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
MemMapEntry *gpio_memmap,
{
Aml *dev = aml_device("GPO0");
aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
Aml *crs = aml_resource_template();
@@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
{
Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
-aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
aml_append(dev, aml_name_decl("_UID", aml_int(0)));
aml_append(scope, dev);
}
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
Please do not include binary changes in acpi patches.
See comment at the top of tests/bios-tables-test.c for documentation
on how to update these.
--
MST
.
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Sun, Jan 05, 2020 at 07:33:55AM -0500, Michael S. Tsirkin wrote:
> On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
> > According to ACPI spec, _ADR should be used for device which is on a
> > bus that has a standard enumeration algorithm. It does not make sense
> > to have a _ADR object for devices which already have _HID and will be
> > enumerated by OSPM.
> >
> > Signed-off-by: Heyi Guo
>
> Are you sure? I would think this depends on the ID and the device
> really. E.g. PCI devices all are expected to have _ADR and some of them
> have a _HID.
That's my understanding, too.
>
> CC Corey who added a device with both HID and ADR to x86 recenly.
>
> Apropos Corey, why was HID APP0005 chosen?
I don't remember. I thought I had looked it up someplace, but I didn't
document it.
>From reading the spec, I believe you could safely delete the _HID and it
would be fine. However, I don't see anything that says it can't be
there, either. But it is extraneous.
Searching on the web a bit, I see that the APP0005 has confused windows.
It does look to be wrong. Maybe deleting it would be a good idea.
-corey
>
> > ---
> > Cc: Shannon Zhao
> > Cc: Peter Maydell
> > Cc: "Michael S. Tsirkin"
> > Cc: Igor Mammedov
> > Cc: qemu-...@nongnu.org
> > Cc: qemu-devel@nongnu.org
> > ---
> > hw/arm/virt-acpi-build.c | 8
> > tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> > tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> > tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> > 4 files changed, 8 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 9f4c7d1889..be752c0ad8 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> > MemMapEntry *uart_memmap,
> > AML_EXCLUSIVE, _irq, 1));
> > aml_append(dev, aml_name_decl("_CRS", crs));
> >
> > -/* The _ADR entry is used to link this device to the UART described
> > - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> > - */
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > -
> > aml_append(scope, dev);
> > }
> >
> > @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> > MemMapEntry *memmap,
> > aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> > aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> > aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> > aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
> > aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> > MemMapEntry *gpio_memmap,
> > {
> > Aml *dev = aml_device("GPO0");
> > aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> >
> > Aml *crs = aml_resource_template();
> > @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > {
> > Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > aml_append(scope, dev);
> > }
> > diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
>
>
> Please do not include binary changes in acpi patches.
>
> See comment at the top of tests/bios-tables-test.c for documentation
> on how to update these.
>
> --
> MST
>
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Sun, Jan 05, 2020 at 07:34:01AM -0500, Michael S. Tsirkin wrote:
> On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
> > According to ACPI spec, _ADR should be used for device which is on a
> > bus that has a standard enumeration algorithm. It does not make sense
> > to have a _ADR object for devices which already have _HID and will be
> > enumerated by OSPM.
> >
> > Signed-off-by: Heyi Guo
>
> Are you sure? I would think this depends on the ID and the device
> really. E.g. PCI devices all are expected to have _ADR and some of them
> have a _HID.
To clarify I am not commenting on patches.
The spec says this:
6.1.5 _HID (Hardware ID)
This object is used to supply OSPM with the device’s PNP ID or ACPI ID.
1
When describing a platform, use of any _HID objects is optional.
However, a _HID object must be
used to describe any device that will be enumerated by OSPM. OSPM only
enumerates a device
when no bus enumerator can detect the device ID. For example, devices
on an ISA bus are
enumerated by OSPM. Use the _ADR object to describe devices enumerated
by bus enumerators
other than OSPM.
Note: "detect the device ID" not "enumerate the device" which I think
means there's a driver matching this vendor/device ID.
So it seems fine to have _ADR so device is enumerated, and still have
_HID e.g. so ACPI driver can be loaded as fallback if there's
no bus driver.
Note I am not saying the patch itself is not correct.
Maybe these devices are not on any standard bus and that
is why they should not have _ADR? I have not looked.
I am just saying that spec does not seem to imply _HID and _ADR
can't coexist.
> CC Corey who added a device with both HID and ADR to x86 recenly.
>
> Apropos Corey, why was HID APP0005 chosen?
>
> > ---
> > Cc: Shannon Zhao
> > Cc: Peter Maydell
> > Cc: "Michael S. Tsirkin"
> > Cc: Igor Mammedov
> > Cc: qemu-...@nongnu.org
> > Cc: qemu-devel@nongnu.org
> > ---
> > hw/arm/virt-acpi-build.c | 8
> > tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> > tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> > tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> > 4 files changed, 8 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 9f4c7d1889..be752c0ad8 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> > MemMapEntry *uart_memmap,
> > AML_EXCLUSIVE, _irq, 1));
> > aml_append(dev, aml_name_decl("_CRS", crs));
> >
> > -/* The _ADR entry is used to link this device to the UART described
> > - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> > - */
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > -
> > aml_append(scope, dev);
> > }
> >
> > @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> > MemMapEntry *memmap,
> > aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> > aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> > aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> > aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
> > aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> > MemMapEntry *gpio_memmap,
> > {
> > Aml *dev = aml_device("GPO0");
> > aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> >
> > Aml *crs = aml_resource_template();
> > @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> > {
> > Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> > -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > aml_append(scope, dev);
> > }
> > diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
>
>
> Please do not include binary changes in acpi patches.
>
> See comment at the top of tests/bios-tables-test.c for documentation
> on how to update these.
>
> --
> MST
Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
On Thu, Dec 19, 2019 at 02:47:59PM +0800, Heyi Guo wrote:
> According to ACPI spec, _ADR should be used for device which is on a
> bus that has a standard enumeration algorithm. It does not make sense
> to have a _ADR object for devices which already have _HID and will be
> enumerated by OSPM.
>
> Signed-off-by: Heyi Guo
Are you sure? I would think this depends on the ID and the device
really. E.g. PCI devices all are expected to have _ADR and some of them
have a _HID.
CC Corey who added a device with both HID and ADR to x86 recenly.
Apropos Corey, why was HID APP0005 chosen?
> ---
> Cc: Shannon Zhao
> Cc: Peter Maydell
> Cc: "Michael S. Tsirkin"
> Cc: Igor Mammedov
> Cc: qemu-...@nongnu.org
> Cc: qemu-devel@nongnu.org
> ---
> hw/arm/virt-acpi-build.c | 8
> tests/data/acpi/virt/DSDT | Bin 18449 -> 18426 bytes
> tests/data/acpi/virt/DSDT.memhp | Bin 19786 -> 19763 bytes
> tests/data/acpi/virt/DSDT.numamem | Bin 18449 -> 18426 bytes
> 4 files changed, 8 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9f4c7d1889..be752c0ad8 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -78,11 +78,6 @@ static void acpi_dsdt_add_uart(Aml *scope, const
> MemMapEntry *uart_memmap,
> AML_EXCLUSIVE, _irq, 1));
> aml_append(dev, aml_name_decl("_CRS", crs));
>
> -/* The _ADR entry is used to link this device to the UART described
> - * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
> - */
> -aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> -
> aml_append(scope, dev);
> }
>
> @@ -170,7 +165,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> MemMapEntry *memmap,
> aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
> aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
> aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> @@ -334,7 +328,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
> MemMapEntry *gpio_memmap,
> {
> Aml *dev = aml_device("GPO0");
> aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>
> Aml *crs = aml_resource_template();
> @@ -364,7 +357,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)
> {
> Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> aml_append(scope, dev);
> }
> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
Please do not include binary changes in acpi patches.
See comment at the top of tests/bios-tables-test.c for documentation
on how to update these.
--
MST
