Re: [Qemu-devel] [PATCH] acpi: change CPU container node _HID to compatible PNP0A05

[Michael S. Tsirkin]

On Mon, May 22, 2017 at 02:58:56PM +0200, Igor Mammedov wrote:
> On Mon, 22 May 2017 14:01:15 +0300
> Evgeny Yakovlev <address@hidden> wrote:
> 
> > On 22.05.2017 13:35, Igor Mammedov wrote:
> > > On Mon, 22 May 2017 12:50:30 +0300
> > > Evgeny Yakovlev <address@hidden> wrote:
> > >  
> > >> When running windows 2016 server guests we have encountered a problem
> > >> with ACPI representation of CPU devices. This windows version contains a
> > >> hidinterrupt.sys driver which looks for ACPI device node with _HID set
> > >> to "ACPI0010" and "ACPI0011". ACPI0010 is also a valid id for CPU
> > >> container device which qemu uses.
> > >>
> > >> hidinterrupt driver takes over (even though it fails) ACPI0010 node and
> > >> thus hides its children -- the CPUs -- from the regular ACPI
> > >> enumeration.  So there are no processors in the Windows device tree
> > >> Device Manager or "!devnode 0 1" in the debugger.
> > >>
> > >> hidinterrupt.inf as shipped with Windows 2016 has both ACPI0011 and
> > >> ACPI0010; the record for the latter is preceded with a comment "This Id
> > >> is not to be used. It will be removed once everyone has stopped using
> > >> it."  So I guess the typo was not in the driver but in the ACPI tables
> > >> of some device(s) which the driver wanted to support despite the bug.
> > >>
> > >> For reference this is a known issue:
> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1377155#c31
> > >>
> > >> This change works around this problem by setting qemu CPU container ACPI
> > >> _HID to compatible PNP0A05.  
> > > I'd say NACK to it.
> > > It's Windows server 2016 bug that violates spec and
> > > it's been reported to them before RTM has been released.
> > > So complain to Microsoft support and make them fix it.
> > >
> > > Meanwhile there is MS suggested workaround to fix issue,
> > > one can use https://bugzilla.redhat.com/show_bug.cgi?id=1377155#c17
> > >
> > > Perhaps I should add workaround link as comment above line:
> > >
> > > aml_append(cpus_dev, aml_name_decl("_HID", aml_string("ACPI0010")));  
> > 
> > Thanks for replying!
> > 
> > Strictly speaking, this is not a Windows bug. Comment in INF file says 
> > that they had to support a hardware device which erroneously picked this 
> > ACPI HID.
> in linux kernel we use device specific quirks to workaround broken hardware,
> there is no point to break generic code for all hardware configurations
> for the sake of one broken firmware.
> So it's Windows problem that they prefer to violate spec and cannibalize
> ACPI0010 for the sake of broken firmware. Open support case with Windows,
> the more complains they get, the more chances that they'll fix it.
> 
> > I understand that this is not a qemu problem either. Then again other 
> > emulators we've tested this on (VirtualBox, Parallels, HyperV) provide a 
> > compatible ACPI configuration and don't trigger this conflict. If this 
> we provide it, that's what '_CID' field is for, so OS that doesn't know
> about ACPI0010 would be happy (any Windows prior Win10/2016)
> 
> > fix is not a big deal and does not affect anything negatively then maybe 
> > we pay a relatively small price to improve compatibility with some guest 
> > systems?
> we plan to reuse this code for ARM in future and it might use
> ACPI0010 container as per spec to describe modeled topology.
> Hence no workaround in QEMU, just use MS suggested workaround
> to remove wrong device binding in WS2016 until they actually
> fix it.

I wonder whether adding an INF with a stub driver
on the install disk can also help as a work-around.
Thoughts?


> 
> > >> Signed-off-by: Evgeny Yakovlev <address@hidden>
> > >> ---
> > >>   hw/acpi/cpu.c | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > >> index a233fe1..b93db40 100644
> > >> --- a/hw/acpi/cpu.c
> > >> +++ b/hw/acpi/cpu.c
> > >> @@ -397,7 +397,7 @@ void build_cpus_aml(Aml *table, MachineState 
> > >> *machine, CPUHotplugFeatures opts,
> > >>           Aml *rm_evt = aml_name("%s.%s", cphp_res_path, 
> > >> CPU_REMOVE_EVENT);
> > >>           Aml *ej_evt = aml_name("%s.%s", cphp_res_path, 
> > >> CPU_EJECT_EVENT);
> > >>   
> > >> -        aml_append(cpus_dev, aml_name_decl("_HID", 
> > >> aml_string("ACPI0010")));
> > >> +        aml_append(cpus_dev, aml_name_decl("_HID", 
> > >> aml_string("PNP0A05")));
> > >>           aml_append(cpus_dev, aml_name_decl("_CID", 
> > >> aml_eisaid("PNP0A05")));
> > >>   
> > >>           method = aml_method(CPU_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);  
> > 
> >