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

[Evgeny Yakovlev]

On 22.05.2017 20:44, Michael S. Tsirkin wrote:
> 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?

As far as i know this is one of the proposed solutions we consider. Will 
have to test and see.

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