Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Paul Durrant
> Sent: 20 March 2017 11:50
> To: 'Jan Beulich' <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH 2/7] x86/viridian: fix xen-hvmcrash when
> vp_assist page is present
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 20 March 2017 11:36
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> > devel@lists.xenproject.org
> > Subject: Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist
> page
> > is present
> >
> > >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > > @@ -288,6 +304,14 @@ static void initialize_vp_assist(struct vcpu *v)
> > >       * enlightenment.
> > >       */
> > >
> > > +    if ( v->arch.hvm_vcpu.viridian.vp_assist.va )
> > > +    {
> > > +        if ( v->arch.hvm_vcpu.viridian.vp_assist.gmfn == gmfn )
> > > +            return;
> >
> > Is this shortcut valid? I.e. is it not valid for the guest to expect the
> > VP assist state to be fully reset if it calls this more than once on a
> > vCPU, yet possibly with the same GFN? (It also looks like this isn't
> > really part of the corrections you want to make here, according to
> > the description.)
> 
> Hmm. The spec is not clear. The problem is that doing a save-context
> followed by restore-context is going through this path. Maybe it's best to
> leave the teardown in the MSR right and special-case a restore when
> vp_assist_va is set.
> 
> >
> > > --- a/xen/include/asm-x86/hvm/viridian.h
> > > +++ b/xen/include/asm-x86/hvm/viridian.h
> > > @@ -23,6 +23,7 @@ struct viridian_vcpu
> > >  {
> > >      struct {
> > >          union viridian_vp_assist msr;
> > > +        unsigned long gmfn;
> >
> > gfn_t ?
> >
> 
> Yes, you're right. I should probably precede this with a patch fixing up the
> gmfn stack variables in viridian.c to use gfn_t for consistency though.

Actually, looking at this again, I'm not sure there's any point in making this a gfn_t. It's only stored for the purposes of an identity match and the thing it matches with is an unsigned long extracted from an MSR bit-field.

  Paul

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