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

From: Wei Liu <wei.liu2@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Igor Druzhinin <igor.druzhinin@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	IanJackson <ian.jackson@eu.citrix.com>,
	xen-devel@lists.xenproject.org,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT
Date: Tue, 29 Aug 2017 10:42:50 +0100	[thread overview]
Message-ID: <20170829094250.nmtl65ruyyk2qu2p@citrix.com> (raw)
In-Reply-To: <59A552D10200007800174E02@prv-mh.provo.novell.com>

On Tue, Aug 29, 2017 at 03:41:05AM -0600, Jan Beulich wrote:
> >>> On 29.08.17 at 11:31, <roger.pau@citrix.com> wrote:
> > On Tue, Aug 29, 2017 at 10:25:23AM +0100, Wei Liu wrote:
> >> On Tue, Aug 29, 2017 at 10:17:24AM +0100, Roger Pau Monne wrote:
> >> > On Tue, Aug 29, 2017 at 03:08:57AM -0600, Jan Beulich wrote:
> >> > > >>> On 29.08.17 at 10:50, <roger.pau@citrix.com> 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@citrix.com>
> >> > > 
> >> > > The patch here looks good, i.e. could have my R-b, just that it is
> >> > > entirely unclear to me how things did work before the quoted
> >> > > commit: Ports used by Xen and qemu-trad must have been out of
> >> > > sync, or am I overlooking something?
> >> > 
> >> > Yes, the GPE port used by qemu-trad was out of sync with the one
> >> > reported in the FADT.
> >> > 
> >> > AFAICT the only thing that didn't work with qemu-trad was ACPI vCPU
> >> > hotplug, but we don't test that in osstest (not even sure if we test
> >> > xenstore vCPU hotplug).
> >> > 
> >> > PM1a and TMR worked fine because the V1 address was hardcoded in the
> >> > FADT, and HVM_PARAM_ACPI_IOPORTS_LOCATION was unconditionally set to 1
> >> > by hvmloader.
> >> 
> >> Do you maybe want to put some of the above into the commit message?
> >> 
> >> You can provide me a new one here in a reply, no need to resend. I want
> >> to cimmit this asap.
> > 
> > OK, I think the following is clearer:
> > 
> > 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.
> > 
> > The most notable issue caused by the above bug was that the QEMU
> > traditional GPE0 block was out of sync: the address provided in the
> > FADT didn't match the address QEMU was using.
> > 
> > Note that PM1a and TMR worked fine because the V1 address was
> > hardcoded in the FADT and HVM_PARAM_ACPI_IOPORTS_LOCATION was
> > unconditionally set to 1 by hvmloader.
> > 
> > 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.
> 
> LGTM

Pushed. Thanks.

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