On Tue, May 19, 2020 at 09:55:38AM +0200, Jan Beulich wrote:
> On 18.05.2020 19:09, Roger Pau Monné wrote:
> > On Wed, Sep 25, 2019 at 05:23:11PM +0200, Jan Beulich wrote:
> >> @@ -310,7 +313,16 @@ int pv_domain_initialise(struct domain *
> >> /* 64-bit PV guest by default. */
> >> d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> >>
> >> - d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom :
> >> opt_xpti_domu;
> >> + if ( is_hardware_domain(d) && opt_xpti_hwdom )
> >> + {
> >> + d->arch.pv.xpti = true;
> >> + ++opt_xpti_hwdom;
> >> + }
> >> + if ( !is_hardware_domain(d) && opt_xpti_domu )
> >> + {
> >> + d->arch.pv.xpti = true;
> >> + opt_xpti_domu = 2;
> >
> > I wonder whether a store fence is needed here in order to guarantee
> > that opt_xpti_domu is visible to flush_area_local before proceeding
> > any further with domain creation.
>
> The changed behavior of flush_area_local() becomes relevant only
> once the new domain runs. This being x86 code, the write can't
> remain invisible for longer than the very latest when the function
> returns, as the store can't be deferred past that (in reality it
> can't be deferred even until after the next [real] function call
> or the next barrier()). And due to x86'es cache coherent nature
> (for WB memory) the moment the store insn completes the new value
> is visible to all other CPUs.
Yes, I think it's fine because this is x86 specific code. A comment
in that regard might be nice, but I'm not going to make this a strong
request.
Reviewed-by: Roger Pau Monné <roger....@citrix.com>
I also think that turning opt_xpti_domu into a proper atomic and
increasing/decreasing (maybe a cmpxg would be needed) upon PV domain
creation/destruction should be able to accurately keep track of PV
domUs and hence could be used to further reduce the flushes when no PV
domains are running?
Thanks, Roger.
