[Qemu-devel] [PATCH] i386/acpi: add _HID to processor nodes

[Qemu-devel] [PATCH] i386/acpi: add _HID to processor nodes

[Matthias Lange]

Processor nodes created via the acpi/aml framework currently don't
feature a _HID string. This patch appends "ACPI0007" as the _HID
string to each processor node.

Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>
---
 hw/i386/acpi-build.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..95f7bf9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1153,6 +1153,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         for (i = 0; i < acpi_cpus; i++) {
             dev = aml_processor(i, 0, 0, "CP%.02X", i);
 
+            aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
             method = aml_method("_MAT", 0);
             aml_append(method, aml_return(aml_call1("CPMA", aml_int(i))));
             aml_append(dev, method);
-- 
1.9.1

Re: [Qemu-devel] [PATCH] i386/acpi: add _HID to processor nodes

[Michael S. Tsirkin]

On Thu, Oct 22, 2015 at 12:38:11PM +0200, Matthias Lange wrote:
> Processor nodes created via the acpi/aml framework currently don't
> feature a _HID string. This patch appends "ACPI0007" as the _HID
> string to each processor node.
> 
> Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>

Indeed,  it does that.  But why?

ACPI0007 Processor Device. This device provides an alternative to
declaring processors using the
Processor ASL statement.


And “Declaring Processors” says


	Each processor in the system must be declared in the ACPI namespace in
	either the \_SB or \_PR
	scope but not both. Declaration of processors in the \_PR scope is
	required for platforms desiring
	compatibility with ACPI 1.0-based OSPM implementations. Processors are
	declared either via the
	ASL Processor statement or the ASL Device statement. A Processor
	definition declares a processor
	object that provides processor configuration information and points to
	the processor register block
	(P_BLK). A Device definition for a processor is declared using the
	ACPI0007 hardware identifier
	(HID). In this case, processor configuration information is provided
	exclusively by objects in the
	processor device’s object list.

In other words, Processor directive does not need a HID.
In fact, ACPI 1.0 didn't include the ACPI0007 HID at all.


Please include this info in both the commit log and a code comment.


> ---
>  hw/i386/acpi-build.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..95f7bf9 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1153,6 +1153,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          for (i = 0; i < acpi_cpus; i++) {
>              dev = aml_processor(i, 0, 0, "CP%.02X", i);
>  
> +            aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
>              method = aml_method("_MAT", 0);
>              aml_append(method, aml_return(aml_call1("CPMA", aml_int(i))));
>              aml_append(dev, method);
> -- 
> 1.9.1

Re: [Qemu-devel] [PATCH] i386/acpi: add _HID to processor nodes

[Matthias Lange]

On 10/22/2015 01:48 PM, Michael S. Tsirkin wrote:
> On Thu, Oct 22, 2015 at 12:38:11PM +0200, Matthias Lange wrote:
>> Processor nodes created via the acpi/aml framework currently don't
>> feature a _HID string. This patch appends "ACPI0007" as the _HID
>> string to each processor node.
>>
>> Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>
> 
> Indeed,  it does that.  But why?
> 
> ACPI0007 Processor Device. This device provides an alternative to
> declaring processors using the
> Processor ASL statement.
>  
> And “Declaring Processors” says
> 
> 
> 	Each processor in the system must be declared in the ACPI namespace in
> 	either the \_SB or \_PR
> 	scope but not both. Declaration of processors in the \_PR scope is
> 	required for platforms desiring
> 	compatibility with ACPI 1.0-based OSPM implementations. Processors are
> 	declared either via the
> 	ASL Processor statement or the ASL Device statement. A Processor
> 	definition declares a processor
> 	object that provides processor configuration information and points to
> 	the processor register block
> 	(P_BLK). A Device definition for a processor is declared using the
> 	ACPI0007 hardware identifier
> 	(HID). In this case, processor configuration information is provided
> 	exclusively by objects in the
> 	processor device’s object list.

Thanks for pointing me to the right section.

> In other words, Processor directive does not need a HID.
> In fact, ACPI 1.0 didn't include the ACPI0007 HID at all.

Right. But, does Qemu strive for ACPI 1.0 compatibility? And e.g.
seabios also defines processors using the ALS processor statement
including the HID.

> Please include this info in both the commit log and a code comment.

Will do.

Matthias.

>> ---
>>  hw/i386/acpi-build.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 95e0c65..95f7bf9 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1153,6 +1153,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>          for (i = 0; i < acpi_cpus; i++) {
>>              dev = aml_processor(i, 0, 0, "CP%.02X", i);
>>  
>> +            aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
>>              method = aml_method("_MAT", 0);
>>              aml_append(method, aml_return(aml_call1("CPMA", aml_int(i))));
>>              aml_append(dev, method);
>> -- 
>> 1.9.1


-- 
Matthias Lange, matthias.lange@kernkonzept.com, +49 - 351 - 41 88 86 14

Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
Geschäftsführer: Dr.-Ing. Michael Hohmuth

Re: [Qemu-devel] [PATCH] i386/acpi: add _HID to processor nodes

[Michael S. Tsirkin]

On Thu, Oct 22, 2015 at 02:42:05PM +0200, Matthias Lange wrote:
> On 10/22/2015 01:48 PM, Michael S. Tsirkin wrote:
> > On Thu, Oct 22, 2015 at 12:38:11PM +0200, Matthias Lange wrote:
> >> Processor nodes created via the acpi/aml framework currently don't
> >> feature a _HID string. This patch appends "ACPI0007" as the _HID
> >> string to each processor node.
> >>
> >> Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>
> > 
> > Indeed,  it does that.  But why?
> > 
> > ACPI0007 Processor Device. This device provides an alternative to
> > declaring processors using the
> > Processor ASL statement.
> >  
> > And “Declaring Processors” says
> > 
> > 
> > 	Each processor in the system must be declared in the ACPI namespace in
> > 	either the \_SB or \_PR
> > 	scope but not both. Declaration of processors in the \_PR scope is
> > 	required for platforms desiring
> > 	compatibility with ACPI 1.0-based OSPM implementations. Processors are
> > 	declared either via the
> > 	ASL Processor statement or the ASL Device statement. A Processor
> > 	definition declares a processor
> > 	object that provides processor configuration information and points to
> > 	the processor register block
> > 	(P_BLK). A Device definition for a processor is declared using the
> > 	ACPI0007 hardware identifier
> > 	(HID). In this case, processor configuration information is provided
> > 	exclusively by objects in the
> > 	processor device’s object list.
> 
> Thanks for pointing me to the right section.
> 
> > In other words, Processor directive does not need a HID.
> > In fact, ACPI 1.0 didn't include the ACPI0007 HID at all.
> 
> Right. But, does Qemu strive for ACPI 1.0 compatibility?

Where possible - it's handy for old guests.

> And e.g.
> seabios also defines processors using the ALS processor statement
> including the HID.

I don't see it there:
[mst@robin seabios]$ git grep ACI0007
[mst@robin seabios]$

> > Please include this info in both the commit log and a code comment.
> 
> Will do.
> 
> Matthias.

Not the ACPI spec info. I have that. The answer to my question - the
actual motivation for the patch.

> >> ---
> >>  hw/i386/acpi-build.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 95e0c65..95f7bf9 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -1153,6 +1153,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>          for (i = 0; i < acpi_cpus; i++) {
> >>              dev = aml_processor(i, 0, 0, "CP%.02X", i);
> >>  
> >> +            aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
> >>              method = aml_method("_MAT", 0);
> >>              aml_append(method, aml_return(aml_call1("CPMA", aml_int(i))));
> >>              aml_append(dev, method);
> >> -- 
> >> 1.9.1
> 
> 
> -- 
> Matthias Lange, matthias.lange@kernkonzept.com, +49 - 351 - 41 88 86 14
> 
> Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
> Geschäftsführer: Dr.-Ing. Michael Hohmuth

Re: [Qemu-devel] [PATCH] i386/acpi: add _HID to processor nodes

[Matthias Lange]

On 10/22/2015 03:02 PM, Michael S. Tsirkin wrote:
> On Thu, Oct 22, 2015 at 02:42:05PM +0200, Matthias Lange wrote:
>> On 10/22/2015 01:48 PM, Michael S. Tsirkin wrote:
>>> On Thu, Oct 22, 2015 at 12:38:11PM +0200, Matthias Lange wrote:
>>>> Processor nodes created via the acpi/aml framework currently don't
>>>> feature a _HID string. This patch appends "ACPI0007" as the _HID
>>>> string to each processor node.
>>>>
>>>> Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>
>>>
>>> Indeed,  it does that.  But why?
>>>
>>> ACPI0007 Processor Device. This device provides an alternative to
>>> declaring processors using the
>>> Processor ASL statement.
>>>  
>>> And “Declaring Processors” says
>>>
>>>
>>> 	Each processor in the system must be declared in the ACPI namespace in
>>> 	either the \_SB or \_PR
>>> 	scope but not both. Declaration of processors in the \_PR scope is
>>> 	required for platforms desiring
>>> 	compatibility with ACPI 1.0-based OSPM implementations. Processors are
>>> 	declared either via the
>>> 	ASL Processor statement or the ASL Device statement. A Processor
>>> 	definition declares a processor
>>> 	object that provides processor configuration information and points to
>>> 	the processor register block
>>> 	(P_BLK). A Device definition for a processor is declared using the
>>> 	ACPI0007 hardware identifier
>>> 	(HID). In this case, processor configuration information is provided
>>> 	exclusively by objects in the
>>> 	processor device’s object list.
>>
>> Thanks for pointing me to the right section.
>>
>>> In other words, Processor directive does not need a HID.
>>> In fact, ACPI 1.0 didn't include the ACPI0007 HID at all.
>>
>> Right. But, does Qemu strive for ACPI 1.0 compatibility?
> 
> Where possible - it's handy for old guests.

I understand. But then, why are processors declared under the \_SB scope
and not under \_PR?

>> And e.g.
>> seabios also defines processors using the ALS processor statement
>> including the HID.
> 
> I don't see it there:
> [mst@robin seabios]$ git grep ACI0007
> [mst@robin seabios]$

It's a typo in your command. Have a look into

seabios/src/fw/ssdt-proc.dsl

Matthias.

>>> Please include this info in both the commit log and a code comment.
>>
>> Will do.
>>
> Not the ACPI spec info. I have that. The answer to my question - the
> actual motivation for the patch.
> 
>>>> ---
>>>>  hw/i386/acpi-build.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 95e0c65..95f7bf9 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -1153,6 +1153,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>>          for (i = 0; i < acpi_cpus; i++) {
>>>>              dev = aml_processor(i, 0, 0, "CP%.02X", i);
>>>>  
>>>> +            aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
>>>>              method = aml_method("_MAT", 0);
>>>>              aml_append(method, aml_return(aml_call1("CPMA", aml_int(i))));
>>>>              aml_append(dev, method);
>>>> -- 
>>>> 1.9.1
>>
>>
>> -- 
>> Matthias Lange, matthias.lange@kernkonzept.com, +49 - 351 - 41 88 86 14
>>
>> Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
>> Geschäftsführer: Dr.-Ing. Michael Hohmuth


-- 
Matthias Lange, matthias.lange@kernkonzept.com, +49 - 351 - 41 88 86 14

Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
Geschäftsführer: Dr.-Ing. Michael Hohmuth

Re: [Qemu-devel] [PATCH] i386/acpi: add _HID to processor nodes

[Michael S. Tsirkin]

On Thu, Oct 22, 2015 at 03:34:46PM +0200, Matthias Lange wrote:
> On 10/22/2015 03:02 PM, Michael S. Tsirkin wrote:
> > On Thu, Oct 22, 2015 at 02:42:05PM +0200, Matthias Lange wrote:
> >> On 10/22/2015 01:48 PM, Michael S. Tsirkin wrote:
> >>> On Thu, Oct 22, 2015 at 12:38:11PM +0200, Matthias Lange wrote:
> >>>> Processor nodes created via the acpi/aml framework currently don't
> >>>> feature a _HID string. This patch appends "ACPI0007" as the _HID
> >>>> string to each processor node.
> >>>>
> >>>> Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>
> >>>
> >>> Indeed,  it does that.  But why?
> >>>
> >>> ACPI0007 Processor Device. This device provides an alternative to
> >>> declaring processors using the
> >>> Processor ASL statement.
> >>>  
> >>> And “Declaring Processors” says
> >>>
> >>>
> >>> 	Each processor in the system must be declared in the ACPI namespace in
> >>> 	either the \_SB or \_PR
> >>> 	scope but not both. Declaration of processors in the \_PR scope is
> >>> 	required for platforms desiring
> >>> 	compatibility with ACPI 1.0-based OSPM implementations. Processors are
> >>> 	declared either via the
> >>> 	ASL Processor statement or the ASL Device statement. A Processor
> >>> 	definition declares a processor
> >>> 	object that provides processor configuration information and points to
> >>> 	the processor register block
> >>> 	(P_BLK). A Device definition for a processor is declared using the
> >>> 	ACPI0007 hardware identifier
> >>> 	(HID). In this case, processor configuration information is provided
> >>> 	exclusively by objects in the
> >>> 	processor device’s object list.
> >>
> >> Thanks for pointing me to the right section.
> >>
> >>> In other words, Processor directive does not need a HID.
> >>> In fact, ACPI 1.0 didn't include the ACPI0007 HID at all.
> >>
> >> Right. But, does Qemu strive for ACPI 1.0 compatibility?
> > 
> > Where possible - it's handy for old guests.
> 
> I understand. But then, why are processors declared under the \_SB scope
> and not under \_PR?

Because even very old guests don't seem to care.
We can consider moving it there.

> >> And e.g.
> >> seabios also defines processors using the ALS processor statement
> >> including the HID.
> > 
> > I don't see it there:
> > [mst@robin seabios]$ git grep ACI0007
> > [mst@robin seabios]$
> 
> It's a typo in your command. Have a look into
> 
> seabios/src/fw/ssdt-proc.dsl
> 
> Matthias.

Right. qemu used to have it too.
This probably means it's safe to add.
Was dropped in commit 20843d16632c4da3d3d35ddc8d5eb047167693ce
without an explanation.
Doesn't mean we'll add it back without an explanation too :)


> >>> Please include this info in both the commit log and a code comment.
> >>
> >> Will do.
> >>
> > Not the ACPI spec info. I have that. The answer to my question - the
> > actual motivation for the patch.
> > 
> >>>> ---
> >>>>  hw/i386/acpi-build.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>> index 95e0c65..95f7bf9 100644
> >>>> --- a/hw/i386/acpi-build.c
> >>>> +++ b/hw/i386/acpi-build.c
> >>>> @@ -1153,6 +1153,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>>          for (i = 0; i < acpi_cpus; i++) {
> >>>>              dev = aml_processor(i, 0, 0, "CP%.02X", i);
> >>>>  
> >>>> +            aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
> >>>>              method = aml_method("_MAT", 0);
> >>>>              aml_append(method, aml_return(aml_call1("CPMA", aml_int(i))));
> >>>>              aml_append(dev, method);
> >>>> -- 
> >>>> 1.9.1
> >>
> >>
> >> -- 
> >> Matthias Lange, matthias.lange@kernkonzept.com, +49 - 351 - 41 88 86 14
> >>
> >> Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
> >> Geschäftsführer: Dr.-Ing. Michael Hohmuth
> 
> 
> -- 
> Matthias Lange, matthias.lange@kernkonzept.com, +49 - 351 - 41 88 86 14
> 
> Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
> Geschäftsführer: Dr.-Ing. Michael Hohmuth