Re: [Xen-devel] [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Tue, Aug 29, 2017 at 03:26:11PM +0100, Igor Druzhinin wrote:
> 
> 
> On 29/08/17 14:42, Jan Beulich wrote:
> >>>> On 29.08.17 at 15:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> On 29/08/17 09:50, Roger Pau Monne wrote:
> >>> Commit 149c6b unmasked an issue long present in Xen: the control/event
> >>> block addresses provided in the ACPI FADT table where hardcoded to the
> >>> V1 version. This was papered over because hvmloader would also always
> >>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> >>> version.
> >>>
> >>> Fix this by passing the address of the control/event blocks to
> >>> acpi_build_tables, so the values can be properly set in the FADT
> >>> table provided to the guest.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>> ---
> >>> Cc: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> >>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >>> ---
> >>> This commit should fix the qumu-trad Windows errors seen by osstest.
> >>
> >> This changes windows behaviour, but does not fix windows.  Windows now
> >> boots, but waits forever while trying to reboot after installing PV
> >> drivers.  There is no hint in the qemu log that the ACPI shutdown event
> >> was received.
> > 
> > Sounds to me like matching exactly the question I've raised: It
> > would help to understand why things have worked originally.
> > While PM1a/b are generally meant to help split brain environments
> > like our Xen/qemu one, iirc we don't make use of PM1b, and hence
> > it seems quite likely that both Xen and qemu monitor PM1a port
> > accesses. If that's the case and things have worked before, it's
> > quite possible that qemu-trad is no servicing the wrong port.
> > 
> 
> That's what actually happen. It seems that modern versions of qemu-trad
> service V1 port location. I was initially confused by comments in Xen
> suggesting that V0 is the right choice for qemu-trad. Seems they need to
> be updated or removed.

Right, I just came to the same conclusion after looking at qemu-trad
code. Can you also fix the comments in ioreq.h so that in the future
we don't fall into the same trap.

> I'll prepare an updated version of the fix today. And it looks like we
> don't need Roger's fix in that case.

Agreed, my fix is not needed, and you just need to unconditionally set
HVM_PARAM_ACPI_IOPORTS_LOCATION to 1, and there's no need to set it
based on BIOS version.

Wei has already reverted your patch and my fix, so please fix/expand
your original change accordingly, and post it against current staging.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel