* [PATCH 0/2] Some cleanup in arm/virt/acpi
@ 2019-12-19 6:47 Heyi Guo
2019-12-19 6:47 ` [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0 Heyi Guo
` (2 more replies)
0 siblings, 3 replies; 33+ messages in thread
From: Heyi Guo @ 2019-12-19 6:47 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Michael S. Tsirkin, Shannon Zhao, Igor Mammedov,
Heyi Guo, wanghaibin.wang
Remove useless device node and conflict _ADR objects in ACPI/DSDT.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
Cc: qemu-arm@nongnu.org
Cc: qemu-devel@nongnu.org
Heyi Guo (2):
arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
arm/virt/acpi: remove _ADR from devices identified by _HID
hw/arm/virt-acpi-build.c | 12 ------------
tests/data/acpi/virt/DSDT | Bin 18462 -> 18426 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19763 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18426 bytes
4 files changed, 12 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
2019-12-19 6:47 [PATCH 0/2] Some cleanup in arm/virt/acpi Heyi Guo
@ 2019-12-19 6:47 ` Heyi Guo
2020-01-13 12:37 ` Igor Mammedov
2020-01-22 5:39 ` Michael S. Tsirkin
2019-12-19 6:47 ` [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID Heyi Guo
2019-12-19 12:34 ` [PATCH 0/2] Some cleanup in arm/virt/acpi Michael S. Tsirkin
2 siblings, 2 replies; 33+ messages in thread
From: Heyi Guo @ 2019-12-19 6:47 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Michael S. Tsirkin, Shannon Zhao, Igor Mammedov,
Heyi Guo, wanghaibin.wang
The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
so simply remote it.
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
Cc: qemu-arm@nongnu.org
Cc: qemu-devel@nongnu.org
---
hw/arm/virt-acpi-build.c | 4 ----
tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19786 bytes
tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
4 files changed, 4 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index bd5f771e9b..9f4c7d1889 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
aml_append(method, aml_return(buf));
aml_append(dev, method);
- Aml *dev_rp0 = aml_device("%s", "RP0");
- aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
- aml_append(dev, dev_rp0);
-
Aml *dev_res0 = aml_device("%s", "RES0");
aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
crs = aml_resource_template();
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
GIT binary patch
delta 39
vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
delta 50
zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
G{V4!>hYx%J
diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
index 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce 100644
GIT binary patch
delta 40
wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
delta 51
zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
HyBr$;t7;Fc
diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
GIT binary patch
delta 39
vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
delta 50
zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
G{V4!>hYx%J
--
2.19.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2019-12-19 6:47 [PATCH 0/2] Some cleanup in arm/virt/acpi Heyi Guo
2019-12-19 6:47 ` [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0 Heyi Guo
@ 2019-12-19 6:47 ` Heyi Guo
2020-01-05 12:33 ` Michael S. Tsirkin
` (2 more replies)
2019-12-19 12:34 ` [PATCH 0/2] Some cleanup in arm/virt/acpi Michael S. Tsirkin
2 siblings, 3 replies; 33+ messages in thread
From: Heyi Guo @ 2019-12-19 6:47 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Michael S. Tsirkin, Shannon Zhao, Igor Mammedov,
Heyi Guo, wanghaibin.wang
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 <guoheyi@huawei.com>
---
Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: qemu-arm@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, &uart_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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
delta 94
zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
tnY@hCfEg&X`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<R6@E|9Scn?n(-T=P<17`zg2CxPo8S3f6006qZ7#siq
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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
delta 94
zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
tnY@hCfEg&X`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
--
2.19.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 0/2] Some cleanup in arm/virt/acpi
2019-12-19 6:47 [PATCH 0/2] Some cleanup in arm/virt/acpi Heyi Guo
2019-12-19 6:47 ` [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0 Heyi Guo
2019-12-19 6:47 ` [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID Heyi Guo
@ 2019-12-19 12:34 ` Michael S. Tsirkin
2 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-12-19 12:34 UTC (permalink / raw)
To: Heyi Guo
Cc: Peter Maydell, qemu-devel, Shannon Zhao, qemu-arm, Igor Mammedov,
wanghaibin.wang
On Thu, Dec 19, 2019 at 02:47:57PM +0800, Heyi Guo wrote:
> Remove useless device node and conflict _ADR objects in ACPI/DSDT.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-devel@nongnu.org
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Pls feel free to merge through ARM tree.
> Heyi Guo (2):
> arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
> arm/virt/acpi: remove _ADR from devices identified by _HID
>
> hw/arm/virt-acpi-build.c | 12 ------------
> tests/data/acpi/virt/DSDT | Bin 18462 -> 18426 bytes
> tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19763 bytes
> tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18426 bytes
> 4 files changed, 12 deletions(-)
>
> --
> 2.19.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2019-12-19 6:47 ` [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID Heyi Guo
@ 2020-01-05 12:33 ` Michael S. Tsirkin
2020-01-05 12:53 ` Michael S. Tsirkin
` (2 more replies)
2020-01-13 12:08 ` Igor Mammedov
2020-01-15 12:08 ` Igor Mammedov
2 siblings, 3 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-05 12:33 UTC (permalink / raw)
To: Heyi Guo
Cc: Peter Maydell, Corey Minyard, qemu-devel, Shannon Zhao, qemu-arm,
Igor Mammedov, wanghaibin.wang
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 <guoheyi@huawei.com>
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 <shannon.zhaosl@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-arm@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, &uart_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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-05 12:33 ` Michael S. Tsirkin
@ 2020-01-05 12:53 ` Michael S. Tsirkin
2020-01-06 2:10 ` Guoheyi
2020-01-16 11:56 ` Guoheyi
2020-01-05 22:54 ` Corey Minyard
2020-01-06 15:51 ` Peter Maydell
2 siblings, 2 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-05 12:53 UTC (permalink / raw)
To: Heyi Guo
Cc: Peter Maydell, Corey Minyard, qemu-devel, Shannon Zhao, qemu-arm,
Igor Mammedov, wanghaibin.wang
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 <guoheyi@huawei.com>
>
> 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 <shannon.zhaosl@gmail.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: qemu-arm@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, &uart_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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-05 12:33 ` Michael S. Tsirkin
2020-01-05 12:53 ` Michael S. Tsirkin
@ 2020-01-05 22:54 ` Corey Minyard
2020-01-06 9:39 ` Michael S. Tsirkin
2020-01-06 15:51 ` Peter Maydell
2 siblings, 1 reply; 33+ messages in thread
From: Corey Minyard @ 2020-01-05 22:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, qemu-devel, Shannon Zhao, qemu-arm, Igor Mammedov,
Heyi Guo, wanghaibin.wang
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 <guoheyi@huawei.com>
>
> 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 <shannon.zhaosl@gmail.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: qemu-arm@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, &uart_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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-05 12:53 ` Michael S. Tsirkin
@ 2020-01-06 2:10 ` Guoheyi
2020-01-13 8:46 ` Guoheyi
2020-01-16 11:56 ` Guoheyi
1 sibling, 1 reply; 33+ messages in thread
From: Guoheyi @ 2020-01-06 2:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Corey Minyard, qemu-devel, Shannon Zhao, qemu-arm,
Igor Mammedov, wanghaibin.wang
在 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 <guoheyi@huawei.com>
>> 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 <shannon.zhaosl@gmail.com>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: qemu-arm@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, &uart_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
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-05 22:54 ` Corey Minyard
@ 2020-01-06 9:39 ` Michael S. Tsirkin
2020-01-06 13:07 ` Corey Minyard
0 siblings, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-06 9:39 UTC (permalink / raw)
To: Corey Minyard
Cc: Peter Maydell, qemu-devel, Shannon Zhao, qemu-arm, Igor Mammedov,
Heyi Guo, wanghaibin.wang
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 <guoheyi@huawei.com>
> >
> > 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 <shannon.zhaosl@gmail.com>
> > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Cc: qemu-arm@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, &uart_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
> >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-06 9:39 ` Michael S. Tsirkin
@ 2020-01-06 13:07 ` Corey Minyard
0 siblings, 0 replies; 33+ messages in thread
From: Corey Minyard @ 2020-01-06 13:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, qemu-devel, Shannon Zhao, qemu-arm, Igor Mammedov,
Heyi Guo, wanghaibin.wang
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 <guoheyi@huawei.com>
> > >
> > > 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 <shannon.zhaosl@gmail.com>
> > > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > > Cc: qemu-arm@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, &uart_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
> > >
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-05 12:33 ` Michael S. Tsirkin
2020-01-05 12:53 ` Michael S. Tsirkin
2020-01-05 22:54 ` Corey Minyard
@ 2020-01-06 15:51 ` Peter Maydell
2020-01-15 2:03 ` Guoheyi
2 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2020-01-06 15:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Corey Minyard, QEMU Developers, Shannon Zhao, qemu-arm,
Igor Mammedov, Heyi Guo, wanghaibin.wang
On Sun, 5 Jan 2020 at 12:34, Michael S. Tsirkin <mst@redhat.com> 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-06 2:10 ` Guoheyi
@ 2020-01-13 8:46 ` Guoheyi
0 siblings, 0 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-13 8:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Corey Minyard, qemu-devel, Shannon Zhao, qemu-arm,
Igor Mammedov, wanghaibin.wang
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 <guoheyi@huawei.com>
>>> 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 <shannon.zhaosl@gmail.com>
>>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: qemu-arm@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, &uart_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
>>
>> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2019-12-19 6:47 ` [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID Heyi Guo
2020-01-05 12:33 ` Michael S. Tsirkin
@ 2020-01-13 12:08 ` Igor Mammedov
2020-01-13 13:57 ` Guoheyi
2020-01-15 12:08 ` Igor Mammedov
2 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2020-01-13 12:08 UTC (permalink / raw)
To: Heyi Guo
Cc: Peter Maydell, Andrew Jones, Michael S. Tsirkin, qemu-devel,
Shannon Zhao, qemu-arm, wanghaibin.wang
On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo <guoheyi@huawei.com> 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 <guoheyi@huawei.com>
Are you sure it's does not make sense?
Have you checked commit f264d51d8, that added _ADR?
> ---
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-arm@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, &uart_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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg&X`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<R6@E|9Scn?n(-T=P<17`zg2CxPo8S3f6006qZ7#siq
>
> 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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg&X`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
2019-12-19 6:47 ` [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0 Heyi Guo
@ 2020-01-13 12:37 ` Igor Mammedov
2020-01-16 12:36 ` Guoheyi
` (2 more replies)
2020-01-22 5:39 ` Michael S. Tsirkin
1 sibling, 3 replies; 33+ messages in thread
From: Igor Mammedov @ 2020-01-13 12:37 UTC (permalink / raw)
To: Heyi Guo
Cc: Peter Maydell, Michael S. Tsirkin, Julia Suvorova, qemu-devel,
Shannon Zhao, qemu-arm, wanghaibin.wang
On Thu, 19 Dec 2019 14:47:58 +0800
Heyi Guo <guoheyi@huawei.com> wrote:
> The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
> so simply remote it.
Could you make commit message more concrete so it would say
why it doesn't make any sense.
It seems to be there to describe root port,
I'd rather have PCI folk ack if it's ok to remove it.
>
> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
>
> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-devel@nongnu.org
> ---
> hw/arm/virt-acpi-build.c | 4 ----
> tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
> tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19786 bytes
> tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
> 4 files changed, 4 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index bd5f771e9b..9f4c7d1889 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> aml_append(method, aml_return(buf));
> aml_append(dev, method);
>
> - Aml *dev_rp0 = aml_device("%s", "RP0");
> - aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
> - aml_append(dev, dev_rp0);
> -
> Aml *dev_res0 = aml_device("%s", "RES0");
> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
> crs = aml_resource_template();
> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
> GIT binary patch
> delta 39
> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>
> delta 50
> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
> G{V4!>hYx%J
>
> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
> index 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce 100644
> GIT binary patch
> delta 40
> wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
>
> delta 51
> zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
> HyBr$;t7;Fc
>
> diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
> GIT binary patch
> delta 39
> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>
> delta 50
> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
> G{V4!>hYx%J
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-13 12:08 ` Igor Mammedov
@ 2020-01-13 13:57 ` Guoheyi
2020-01-13 15:26 ` Andrew Jones
0 siblings, 1 reply; 33+ messages in thread
From: Guoheyi @ 2020-01-13 13:57 UTC (permalink / raw)
To: Igor Mammedov, Andrew Jones
Cc: Peter Maydell, Andrew Jones, Michael S. Tsirkin, qemu-devel,
Shannon Zhao, qemu-arm, wanghaibin.wang
在 2020/1/13 20:08, Igor Mammedov 写道:
> On Thu, 19 Dec 2019 14:47:59 +0800
> Heyi Guo <guoheyi@huawei.com> 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 <guoheyi@huawei.com>
> 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 <shannon.zhaosl@gmail.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: qemu-arm@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, &uart_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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
>> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>>
>> delta 94
>> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
>> tnY@hCfEg&X`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<R6@E|9Scn?n(-T=P<17`zg2CxPo8S3f6006qZ7#siq
>>
>> 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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
>> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>>
>> delta 94
>> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
>> tnY@hCfEg&X`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>>
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-13 13:57 ` Guoheyi
@ 2020-01-13 15:26 ` Andrew Jones
2020-01-15 1:25 ` Guoheyi
0 siblings, 1 reply; 33+ messages in thread
From: Andrew Jones @ 2020-01-13 15:26 UTC (permalink / raw)
To: Guoheyi
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, wanghaibin.wang, Igor Mammedov
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 <guoheyi@huawei.com> 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 <guoheyi@huawei.com>
> > 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 <shannon.zhaosl@gmail.com>
> > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Cc: qemu-arm@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, &uart_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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
> > > c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
> > >
> > > delta 94
> > > zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> > > tnY@hCfEg&X`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<R6@E|9Scn?n(-T=P<17`zg2CxPo8S3f6006qZ7#siq
> > >
> > > 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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
> > > c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
> > >
> > > delta 94
> > > zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> > > tnY@hCfEg&X`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
> > >
> >
> > .
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-13 15:26 ` Andrew Jones
@ 2020-01-15 1:25 ` Guoheyi
0 siblings, 0 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-15 1:25 UTC (permalink / raw)
To: Andrew Jones
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, wanghaibin.wang, Igor Mammedov
在 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 <guoheyi@huawei.com> 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 <guoheyi@huawei.com>
>>> 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 <shannon.zhaosl@gmail.com>
>>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: qemu-arm@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, &uart_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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
>>>> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>>>>
>>>> delta 94
>>>> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
>>>> tnY@hCfEg&X`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<R6@E|9Scn?n(-T=P<17`zg2CxPo8S3f6006qZ7#siq
>>>>
>>>> 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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
>>>> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>>>>
>>>> delta 94
>>>> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
>>>> tnY@hCfEg&X`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>>>>
>>> .
>>
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-06 15:51 ` Peter Maydell
@ 2020-01-15 2:03 ` Guoheyi
2020-01-15 6:30 ` Michael S. Tsirkin
0 siblings, 1 reply; 33+ messages in thread
From: Guoheyi @ 2020-01-15 2:03 UTC (permalink / raw)
To: Peter Maydell, Michael S. Tsirkin
Cc: Corey Minyard, QEMU Developers, Shannon Zhao, qemu-arm,
Igor Mammedov, wanghaibin.wang
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 <mst@redhat.com> 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
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-15 2:03 ` Guoheyi
@ 2020-01-15 6:30 ` Michael S. Tsirkin
2020-01-15 9:25 ` Guoheyi
0 siblings, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-15 6:30 UTC (permalink / raw)
To: Guoheyi
Cc: Peter Maydell, Corey Minyard, QEMU Developers, Shannon Zhao,
qemu-arm, wanghaibin.wang, Igor Mammedov
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 <mst@redhat.com>
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"
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-15 6:30 ` Michael S. Tsirkin
@ 2020-01-15 9:25 ` Guoheyi
2020-01-15 10:53 ` Michael S. Tsirkin
2020-01-15 10:55 ` Michael S. Tsirkin
0 siblings, 2 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-15 9:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Corey Minyard, QEMU Developers, Shannon Zhao,
qemu-arm, wanghaibin.wang, Igor Mammedov
在 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 <mst@redhat.com>
>
>
> 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"
>
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-15 9:25 ` Guoheyi
@ 2020-01-15 10:53 ` Michael S. Tsirkin
2020-01-15 10:55 ` Michael S. Tsirkin
1 sibling, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-15 10:53 UTC (permalink / raw)
To: Guoheyi
Cc: Peter Maydell, Corey Minyard, QEMU Developers, Shannon Zhao,
qemu-arm, wanghaibin.wang, Igor Mammedov
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 <mst@redhat.com>
> >
> >
> > 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"
> >
> >
> > .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-15 9:25 ` Guoheyi
2020-01-15 10:53 ` Michael S. Tsirkin
@ 2020-01-15 10:55 ` Michael S. Tsirkin
2020-01-16 12:24 ` Peter Maydell
1 sibling, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-15 10:55 UTC (permalink / raw)
To: Guoheyi
Cc: Peter Maydell, Corey Minyard, QEMU Developers, Shannon Zhao,
qemu-arm, wanghaibin.wang, Igor Mammedov
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 <mst@redhat.com>
> >
> >
> > 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 <mst@redhat.com>
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
*/
#include "qemu/osdep.h"
diff --git a/roms/seabios b/roms/seabios
index f21b5a4aeb..c9ba5276e3 160000
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit f21b5a4aeb020f2a5e2c6503f906a9349dd2f069
+Subproject commit c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2019-12-19 6:47 ` [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID Heyi Guo
2020-01-05 12:33 ` Michael S. Tsirkin
2020-01-13 12:08 ` Igor Mammedov
@ 2020-01-15 12:08 ` Igor Mammedov
2 siblings, 0 replies; 33+ messages in thread
From: Igor Mammedov @ 2020-01-15 12:08 UTC (permalink / raw)
To: Heyi Guo
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, wanghaibin.wang
On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo <guoheyi@huawei.com> 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 <guoheyi@huawei.com>
Acked-by: Igor Mammedov <imammedo@redhat.com>
>
> ---
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-arm@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, &uart_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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg&X`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<R6@E|9Scn?n(-T=P<17`zg2CxPo8S3f6006qZ7#siq
>
> 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&G1_q{66S<_BT5Bh&t1wzk^tIeLL4lL8ZSqD=gU!!5x$Pt+
> c1HyxxIO07#U3dfh0t}oDoEbRcLp@y>07w882mk;8
>
> delta 94
> zcmey>&p2@cBbQ6CONgKc0|V26iCof5J#`b+RhV2^Ci+-%al|{i1o1F1FmP^cRp4ao
> tnY@hCfEg&X`7$S;oxFTNc#soEyoaX?Z-8HbfwO@#16Tu)4E1zj005fm7mWY_
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-05 12:53 ` Michael S. Tsirkin
2020-01-06 2:10 ` Guoheyi
@ 2020-01-16 11:56 ` Guoheyi
2020-01-16 13:25 ` Igor Mammedov
1 sibling, 1 reply; 33+ messages in thread
From: Guoheyi @ 2020-01-16 11:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, Corey Minyard, qemu-devel, Shannon Zhao, qemu-arm,
Igor Mammedov, wanghaibin.wang
在 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 <guoheyi@huawei.com>
>> 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 <shannon.zhaosl@gmail.com>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: qemu-arm@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, &uart_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
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-15 10:55 ` Michael S. Tsirkin
@ 2020-01-16 12:24 ` Peter Maydell
2020-01-17 2:08 ` Guoheyi
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2020-01-16 12:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Corey Minyard, QEMU Developers, Shannon Zhao, qemu-arm,
Igor Mammedov, Guoheyi, wanghaibin.wang
On Wed, 15 Jan 2020 at 10:55, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> Here's a hopefully better patch. Peter does this address the issue?
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> 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 160000
> --- 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
2020-01-13 12:37 ` Igor Mammedov
@ 2020-01-16 12:36 ` Guoheyi
2020-01-21 3:48 ` Guoheyi
2020-01-21 6:35 ` Michael S. Tsirkin
2 siblings, 0 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-16 12:36 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, Michael S. Tsirkin, Julia Suvorova, qemu-devel,
Shannon Zhao, qemu-arm, wanghaibin.wang
在 2020/1/13 20:37, Igor Mammedov 写道:
> On Thu, 19 Dec 2019 14:47:58 +0800
> Heyi Guo <guoheyi@huawei.com> wrote:
>
>> The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
>> so simply remote it.
> Could you make commit message more concrete so it would say
> why it doesn't make any sense.
The reason I'd like to remove it is that I never see a reason that it
should be there :)
1. I searched through ACPI spec and PCI firmware spec, but didn't find
any reason for its existance (might miss something).
2. We don't have such "PR0" device on PCI host bridge object on physical
arm64 machines and these platforms work well.
3. The VM also works well after removing this device.
4. This device looks strange to only have an identity (_ADR) but not any
other attribute; also this device was introduced in the initial commit
without any special comment.
Thanks,
Heyi
>
> It seems to be there to describe root port,
> I'd rather have PCI folk ack if it's ok to remove it.
>
>> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
>>
>> ---
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
>> Cc: qemu-arm@nongnu.org
>> Cc: qemu-devel@nongnu.org
>> ---
>> hw/arm/virt-acpi-build.c | 4 ----
>> tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
>> tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19786 bytes
>> tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
>> 4 files changed, 4 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index bd5f771e9b..9f4c7d1889 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>> aml_append(method, aml_return(buf));
>> aml_append(dev, method);
>>
>> - Aml *dev_rp0 = aml_device("%s", "RP0");
>> - aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
>> - aml_append(dev, dev_rp0);
>> -
>> Aml *dev_res0 = aml_device("%s", "RES0");
>> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>> crs = aml_resource_template();
>> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
>> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
>> GIT binary patch
>> delta 39
>> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>>
>> delta 50
>> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
>> G{V4!>hYx%J
>>
>> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
>> index 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce 100644
>> GIT binary patch
>> delta 40
>> wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
>>
>> delta 51
>> zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
>> HyBr$;t7;Fc
>>
>> diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
>> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
>> GIT binary patch
>> delta 39
>> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>>
>> delta 50
>> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
>> G{V4!>hYx%J
>>
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-16 11:56 ` Guoheyi
@ 2020-01-16 13:25 ` Igor Mammedov
2020-01-17 1:54 ` Guoheyi
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2020-01-16 13:25 UTC (permalink / raw)
To: Guoheyi
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, Corey Minyard, wanghaibin.wang
On Thu, 16 Jan 2020 19:56:19 +0800
Guoheyi <guoheyi@huawei.com> 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 <guoheyi@huawei.com>
> >> 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
[...]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-16 13:25 ` Igor Mammedov
@ 2020-01-17 1:54 ` Guoheyi
0 siblings, 0 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-17 1:54 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, Corey Minyard, wanghaibin.wang
在 2020/1/16 21:25, Igor Mammedov 写道:
> On Thu, 16 Jan 2020 19:56:19 +0800
> Guoheyi <guoheyi@huawei.com> 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 <guoheyi@huawei.com>
>>>> 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
> [...]
>
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID
2020-01-16 12:24 ` Peter Maydell
@ 2020-01-17 2:08 ` Guoheyi
0 siblings, 0 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-17 2:08 UTC (permalink / raw)
To: Peter Maydell, Michael S. Tsirkin
Cc: Corey Minyard, QEMU Developers, Shannon Zhao, qemu-arm,
Igor Mammedov, wanghaibin.wang
在 2020/1/16 20:24, Peter Maydell 写道:
> On Wed, 15 Jan 2020 at 10:55, Michael S. Tsirkin <mst@redhat.com> wrote:
>> Here's a hopefully better patch. Peter does this address the issue?
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>>
>> 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 160000
>> --- 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
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
2020-01-13 12:37 ` Igor Mammedov
2020-01-16 12:36 ` Guoheyi
@ 2020-01-21 3:48 ` Guoheyi
2020-01-21 6:35 ` Michael S. Tsirkin
2 siblings, 0 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-21 3:48 UTC (permalink / raw)
To: Igor Mammedov, Julia Suvorova
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel, Shannon Zhao,
qemu-arm, wanghaibin.wang
Hi Julia,
Could you provide some comments or advice?
Thanks,
Heyi
在 2020/1/13 20:37, Igor Mammedov 写道:
> On Thu, 19 Dec 2019 14:47:58 +0800
> Heyi Guo <guoheyi@huawei.com> wrote:
>
>> The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
>> so simply remote it.
> Could you make commit message more concrete so it would say
> why it doesn't make any sense.
>
> It seems to be there to describe root port,
> I'd rather have PCI folk ack if it's ok to remove it.
>
>> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
>>
>> ---
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
>> Cc: qemu-arm@nongnu.org
>> Cc: qemu-devel@nongnu.org
>> ---
>> hw/arm/virt-acpi-build.c | 4 ----
>> tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
>> tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19786 bytes
>> tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
>> 4 files changed, 4 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index bd5f771e9b..9f4c7d1889 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>> aml_append(method, aml_return(buf));
>> aml_append(dev, method);
>>
>> - Aml *dev_rp0 = aml_device("%s", "RP0");
>> - aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
>> - aml_append(dev, dev_rp0);
>> -
>> Aml *dev_res0 = aml_device("%s", "RES0");
>> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>> crs = aml_resource_template();
>> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
>> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
>> GIT binary patch
>> delta 39
>> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>>
>> delta 50
>> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
>> G{V4!>hYx%J
>>
>> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
>> index 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce 100644
>> GIT binary patch
>> delta 40
>> wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
>>
>> delta 51
>> zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
>> HyBr$;t7;Fc
>>
>> diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
>> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
>> GIT binary patch
>> delta 39
>> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>>
>> delta 50
>> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
>> G{V4!>hYx%J
>>
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
2020-01-13 12:37 ` Igor Mammedov
2020-01-16 12:36 ` Guoheyi
2020-01-21 3:48 ` Guoheyi
@ 2020-01-21 6:35 ` Michael S. Tsirkin
2 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-21 6:35 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, Julia Suvorova, qemu-devel, Shannon Zhao,
qemu-arm, Heyi Guo, wanghaibin.wang
On Mon, Jan 13, 2020 at 01:37:02PM +0100, Igor Mammedov wrote:
> On Thu, 19 Dec 2019 14:47:58 +0800
> Heyi Guo <guoheyi@huawei.com> wrote:
>
> > The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
> > so simply remote it.
> Could you make commit message more concrete so it would say
> why it doesn't make any sense.
>
> It seems to be there to describe root port,
> I'd rather have PCI folk ack if it's ok to remove it.
An empty device like this doesn't really do anything useful I think.
commit log needs to be fixed up though.
> >
> > Signed-off-by: Heyi Guo <guoheyi@huawei.com>
> >
> > ---
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> > Cc: qemu-arm@nongnu.org
> > Cc: qemu-devel@nongnu.org
> > ---
> > hw/arm/virt-acpi-build.c | 4 ----
> > tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
> > tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19786 bytes
> > tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
> > 4 files changed, 4 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index bd5f771e9b..9f4c7d1889 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> > aml_append(method, aml_return(buf));
> > aml_append(dev, method);
> >
> > - Aml *dev_rp0 = aml_device("%s", "RP0");
> > - aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
> > - aml_append(dev, dev_rp0);
> > -
> > Aml *dev_res0 = aml_device("%s", "RES0");
> > aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
> > crs = aml_resource_template();
> > diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> > index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
> > GIT binary patch
> > delta 39
> > vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
> >
> > delta 50
> > zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
> > G{V4!>hYx%J
> >
> > diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
> > index 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce 100644
> > GIT binary patch
> > delta 40
> > wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
> >
> > delta 51
> > zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
> > HyBr$;t7;Fc
> >
> > diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
> > index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
> > GIT binary patch
> > delta 39
> > vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
> >
> > delta 50
> > zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
> > G{V4!>hYx%J
> >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
2019-12-19 6:47 ` [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0 Heyi Guo
2020-01-13 12:37 ` Igor Mammedov
@ 2020-01-22 5:39 ` Michael S. Tsirkin
2020-01-22 6:38 ` Guoheyi
1 sibling, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2020-01-22 5:39 UTC (permalink / raw)
To: Heyi Guo
Cc: Peter Maydell, qemu-devel, Shannon Zhao, qemu-arm, Igor Mammedov,
wanghaibin.wang
On Thu, Dec 19, 2019 at 02:47:58PM +0800, Heyi Guo wrote:
> The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
> so simply remote it.
>
> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
So given this has no methods except _ADR, I think it's
safe to remove:
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-devel@nongnu.org
> ---
> hw/arm/virt-acpi-build.c | 4 ----
> tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
> tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19786 bytes
> tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
> 4 files changed, 4 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index bd5f771e9b..9f4c7d1889 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> aml_append(method, aml_return(buf));
> aml_append(dev, method);
>
> - Aml *dev_rp0 = aml_device("%s", "RP0");
> - aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
> - aml_append(dev, dev_rp0);
> -
> Aml *dev_res0 = aml_device("%s", "RES0");
> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
> crs = aml_resource_template();
> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
> GIT binary patch
> delta 39
> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>
> delta 50
> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
> G{V4!>hYx%J
>
> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
> index 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce 100644
> GIT binary patch
> delta 40
> wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
>
> delta 51
> zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
> HyBr$;t7;Fc
>
> diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
> GIT binary patch
> delta 39
> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>
> delta 50
> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
> G{V4!>hYx%J
>
> --
> 2.19.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0
2020-01-22 5:39 ` Michael S. Tsirkin
@ 2020-01-22 6:38 ` Guoheyi
0 siblings, 0 replies; 33+ messages in thread
From: Guoheyi @ 2020-01-22 6:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, qemu-devel, Shannon Zhao, qemu-arm, Igor Mammedov,
wanghaibin.wang
在 2020/1/22 13:39, Michael S. Tsirkin 写道:
> On Thu, Dec 19, 2019 at 02:47:58PM +0800, Heyi Guo wrote:
>> The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
>> so simply remote it.
>>
>> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
> So given this has no methods except _ADR, I think it's
> safe to remove:
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Thanks.
I'll send v2 to fix up the commit message, and follow the new work flow
we disscussed earlier. I found some more issues in ARM ACPI, which will
also be included in v2.
Heyi
>
>
>> ---
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
>> Cc: qemu-arm@nongnu.org
>> Cc: qemu-devel@nongnu.org
>> ---
>> hw/arm/virt-acpi-build.c | 4 ----
>> tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
>> tests/data/acpi/virt/DSDT.memhp | Bin 19799 -> 19786 bytes
>> tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
>> 4 files changed, 4 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index bd5f771e9b..9f4c7d1889 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>> aml_append(method, aml_return(buf));
>> aml_append(dev, method);
>>
>> - Aml *dev_rp0 = aml_device("%s", "RP0");
>> - aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
>> - aml_append(dev, dev_rp0);
>> -
>> Aml *dev_res0 = aml_device("%s", "RES0");
>> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>> crs = aml_resource_template();
>> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
>> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
>> GIT binary patch
>> delta 39
>> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>>
>> delta 50
>> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
>> G{V4!>hYx%J
>>
>> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
>> index 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce 100644
>> GIT binary patch
>> delta 40
>> wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
>>
>> delta 51
>> zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
>> HyBr$;t7;Fc
>>
>> diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
>> index d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c 100644
>> GIT binary patch
>> delta 39
>> vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)
>>
>> delta 50
>> zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?&k^tF5;R%I
>> G{V4!>hYx%J
>>
>> --
>> 2.19.1
>
> .
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2020-01-22 6:40 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-19 6:47 [PATCH 0/2] Some cleanup in arm/virt/acpi Heyi Guo
2019-12-19 6:47 ` [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0 Heyi Guo
2020-01-13 12:37 ` Igor Mammedov
2020-01-16 12:36 ` Guoheyi
2020-01-21 3:48 ` Guoheyi
2020-01-21 6:35 ` Michael S. Tsirkin
2020-01-22 5:39 ` Michael S. Tsirkin
2020-01-22 6:38 ` Guoheyi
2019-12-19 6:47 ` [PATCH 2/2] arm/virt/acpi: remove _ADR from devices identified by _HID Heyi Guo
2020-01-05 12:33 ` Michael S. Tsirkin
2020-01-05 12:53 ` Michael S. Tsirkin
2020-01-06 2:10 ` Guoheyi
2020-01-13 8:46 ` Guoheyi
2020-01-16 11:56 ` Guoheyi
2020-01-16 13:25 ` Igor Mammedov
2020-01-17 1:54 ` Guoheyi
2020-01-05 22:54 ` Corey Minyard
2020-01-06 9:39 ` Michael S. Tsirkin
2020-01-06 13:07 ` Corey Minyard
2020-01-06 15:51 ` Peter Maydell
2020-01-15 2:03 ` Guoheyi
2020-01-15 6:30 ` Michael S. Tsirkin
2020-01-15 9:25 ` Guoheyi
2020-01-15 10:53 ` Michael S. Tsirkin
2020-01-15 10:55 ` Michael S. Tsirkin
2020-01-16 12:24 ` Peter Maydell
2020-01-17 2:08 ` Guoheyi
2020-01-13 12:08 ` Igor Mammedov
2020-01-13 13:57 ` Guoheyi
2020-01-13 15:26 ` Andrew Jones
2020-01-15 1:25 ` Guoheyi
2020-01-15 12:08 ` Igor Mammedov
2019-12-19 12:34 ` [PATCH 0/2] Some cleanup in arm/virt/acpi Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).