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 <address@hidden>
> >>>
> >>> 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:
> > address@hidden seabios]$ git grep ACI0007
> > address@hidden 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, address@hidden, +49 - 351 - 41 88 86 14
> >>
> >> Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
> >> Geschäftsführer: Dr.-Ing. Michael Hohmuth
> 
> 
> -- 
> Matthias Lange, address@hidden, +49 - 351 - 41 88 86 14
> 
> Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
> Geschäftsführer: Dr.-Ing. Michael Hohmuth