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
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
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
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
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
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
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 >
在 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 > > .
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 > >
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 > > > >
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
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
>>
>> .
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_ >
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 >
在 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_ >> > > .
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_ > > > > > > > . > >
在 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_ >>>> >>> . >> > > .
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
>
> .
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"
在 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" > > > .
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" > > > > > > .
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
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_ >
在 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 > > .
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
在 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 >> > > .
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 [...]
在 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 > [...] > > > .
在 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 > > .
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
>>
>
> .
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 > >
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
在 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 > > .