[2/2] arm/virt/acpi: remove _ADR from devices identified by _HID

Commit Message

Heyi Guo Dec. 19, 2019, 6:47 a.m. UTC

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 <[e-mail]>

---
Cc: Shannon Zhao <[e-mail]>
Cc: Peter Maydell <[e-mail]>
Cc: "Michael S. Tsirkin" <[e-mail]>
Cc: Igor Mammedov <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 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/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_

Comments

Michael S. Tsirkin Jan. 5, 2020, 12:33 p.m. UTC | #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 <[e-mail]>

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 <[e-mail]>
> Cc: Peter Maydell <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Igor Mammedov <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  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.

Michael S. Tsirkin Jan. 5, 2020, 12:53 p.m. UTC | #2

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 <[e-mail]>
> 
> 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 <[e-mail]>
> > Cc: Peter Maydell <[e-mail]>
> > Cc: "Michael S. Tsirkin" <[e-mail]>
> > Cc: Igor Mammedov <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  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

Corey Minyard Jan. 5, 2020, 10:54 p.m. UTC | #3

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 <[e-mail]>
> 
> 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 <[e-mail]>
> > Cc: Peter Maydell <[e-mail]>
> > Cc: "Michael S. Tsirkin" <[e-mail]>
> > Cc: Igor Mammedov <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  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
>

Heyi Guo Jan. 6, 2020, 2:10 a.m. UTC | #4

在 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 <[e-mail]>
>> 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 <[e-mail]>
>>> Cc: Peter Maydell <[e-mail]>
>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>> Cc: Igor Mammedov <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> ---
>>>   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
>
> .

Michael S. Tsirkin Jan. 6, 2020, 9:39 a.m. UTC | #5

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > > Cc: Peter Maydell <[e-mail]>
> > > Cc: "Michael S. Tsirkin" <[e-mail]>
> > > Cc: Igor Mammedov <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > ---
> > >  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
> >

Corey Minyard Jan. 6, 2020, 1:07 p.m. UTC | #6

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 <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > > Cc: Peter Maydell <[e-mail]>
> > > > Cc: "Michael S. Tsirkin" <[e-mail]>
> > > > Cc: Igor Mammedov <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > ---
> > > >  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
> > > 
>

Peter Maydell Jan. 6, 2020, 3:51 p.m. UTC | #7

On Sun, 5 Jan 2020 at 12:34, Michael S. Tsirkin <[e-mail]> 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

Heyi Guo Jan. 13, 2020, 8:46 a.m. UTC | #8

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 <[e-mail]>
>>> 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 <[e-mail]>
>>>> Cc: Peter Maydell <[e-mail]>
>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>>> Cc: Igor Mammedov <[e-mail]>
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> ---
>>>>   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
>>
>> .

Igor Mammedov Jan. 13, 2020, 12:08 p.m. UTC | #9

On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo <[e-mail]> 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 <[e-mail]>

Are you sure it's does not make sense?
Have you checked commit f264d51d8, that added _ADR?

> ---
> Cc: Shannon Zhao <[e-mail]>
> Cc: Peter Maydell <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Igor Mammedov <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  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_
>

Heyi Guo Jan. 13, 2020, 1:57 p.m. UTC | #10

在 2020/1/13 20:08, Igor Mammedov 写道:
> On Thu, 19 Dec 2019 14:47:59 +0800
> Heyi Guo <[e-mail]> 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 <[e-mail]>
> 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 <[e-mail]>
>> Cc: Peter Maydell <[e-mail]>
>> Cc: "Michael S. Tsirkin" <[e-mail]>
>> Cc: Igor Mammedov <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>   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_
>>
>
> .

Andrew Jones Jan. 13, 2020, 3:26 p.m. UTC | #11

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 <[e-mail]> 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 <[e-mail]>
> > 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 <[e-mail]>
> > > Cc: Peter Maydell <[e-mail]>
> > > Cc: "Michael S. Tsirkin" <[e-mail]>
> > > Cc: Igor Mammedov <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > ---
> > >   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_
> > > 
> > 
> > .
> 
>

Heyi Guo Jan. 15, 2020, 1:25 a.m. UTC | #12

在 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 <[e-mail]> 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 <[e-mail]>
>>> 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 <[e-mail]>
>>>> Cc: Peter Maydell <[e-mail]>
>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>>> Cc: Igor Mammedov <[e-mail]>
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> ---
>>>>    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_
>>>>
>>> .
>>
>
> .

Heyi Guo Jan. 15, 2020, 2:03 a.m. UTC | #13

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 <[e-mail]> 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
>
> .

Michael S. Tsirkin Jan. 15, 2020, 6:30 a.m. UTC | #14

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 <[e-mail]>


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"

Heyi Guo Jan. 15, 2020, 9:25 a.m. UTC | #15

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

Michael S. Tsirkin Jan. 15, 2020, 10:53 a.m. UTC | #16

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 <[e-mail]>
> > 
> > 
> > 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"
> > 
> > 
> > .

Michael S. Tsirkin Jan. 15, 2020, 10:55 a.m. UTC | #17

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 <[e-mail]>
> > 
> > 
> > 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 <[e-mail]>


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

Igor Mammedov Jan. 15, 2020, 12:08 p.m. UTC | #18

On Thu, 19 Dec 2019 14:47:59 +0800
Heyi Guo <[e-mail]> 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 <[e-mail]>
Acked-by: Igor Mammedov <[e-mail]>

> 
> ---
> Cc: Shannon Zhao <[e-mail]>
> Cc: Peter Maydell <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Igor Mammedov <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  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_
>

Heyi Guo Jan. 16, 2020, 11:56 a.m. UTC | #19

在 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 <[e-mail]>
>> 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 <[e-mail]>
>>> Cc: Peter Maydell <[e-mail]>
>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>> Cc: Igor Mammedov <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> ---
>>>   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
>
> .

Peter Maydell Jan. 16, 2020, 12:24 p.m. UTC | #20

On Wed, 15 Jan 2020 at 10:55, Michael S. Tsirkin <[e-mail]> wrote:
>
> Here's a hopefully better patch. Peter does this address the issue?
>
> Signed-off-by: Michael S. Tsirkin <[e-mail]>
>
>
> 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

Igor Mammedov Jan. 16, 2020, 1:25 p.m. UTC | #21

On Thu, 16 Jan 2020 19:56:19 +0800
Guoheyi <[e-mail]> 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 <[e-mail]>  
> >> 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

[...]

Heyi Guo Jan. 17, 2020, 1:54 a.m. UTC | #22

在 2020/1/16 21:25, Igor Mammedov 写道:
> On Thu, 16 Jan 2020 19:56:19 +0800
> Guoheyi <[e-mail]> 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 <[e-mail]>
>>>> 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
> [...]
>
>
> .

Heyi Guo Jan. 17, 2020, 2:08 a.m. UTC | #23

在 2020/1/16 20:24, Peter Maydell 写道:
> On Wed, 15 Jan 2020 at 10:55, Michael S. Tsirkin <[e-mail]> wrote:
>> Here's a hopefully better patch. Peter does this address the issue?
>>
>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
>>
>>
>> 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
>
> .

Patch

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);
 }