Re: [Xen-devel] [PATCH] x86/hvm/viridian: save APIC assist vector

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant
> Sent: 29 March 2016 09:57
> To: 'Jan Beulich'
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] x86/hvm/viridian: save APIC assist vector
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: 29 March 2016 09:42
> > To: Paul Durrant
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] x86/hvm/viridian: save APIC assist vector
> >
> > >>> On 24.03.16 at 18:19, <paul.durrant@xxxxxxxxxx> wrote:
> > > @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v,
> int
> > vector)
> > >       * wrong and the VM will most likely hang so force a crash now
> > >       * to make the problem clear.
> > >       */
> > > -    if ( v->arch.hvm_vcpu.viridian.apic_assist.vector >= 0 )
> > > +    if ( v->arch.hvm_vcpu.viridian.apic_assist.vector != 0 )
> > >          domain_crash(v->domain);
> > >
> > > -    v->arch.hvm_vcpu.viridian.apic_assist.vector = vector;
> > > +    /* Increment the value so that zero is always invalid. */
> > > +    v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1;
> >
> > From an APIC perspective, aren't vectors below 0x10 invalid
> > anyway? I.e. can't zero serve the "none" indication just as much
> > as -1 did without this new biasing? Or otherwise I'd still expect ...
> >
> 
> I can do that, if you're happy with it. If I'm going to do it this way though 
> I will
> also put in an explicit check to make sure APIC assist is not used for 
> vectors <
> 0x16.

Obviously I meant 0x10 there ;-)

  Paul

> 
>   Paul
> 
> > > @@ -829,13 +830,21 @@ static int viridian_load_vcpu_ctxt(struct domain
> > *d, hvm_domain_context_t *h)
> > >          return -EINVAL;
> > >      }
> > >
> > > -    if ( hvm_load_entry(VIRIDIAN_VCPU, h, &ctxt) != 0 )
> > > +    if ( hvm_load_entry_zeroextend(VIRIDIAN_VCPU, h, &ctxt) != 0 )
> > >          return -EINVAL;
> > >
> > > -    v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist;
> > > +    v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist_msr;
> > >      if ( v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.enabled )
> > >          initialize_apic_assist(v);
> > >
> > > +    /*
> > > +     * Guests that were saved on a host that did not use APIC assist
> > > +     * will clearly never have a pending assist, so reading the zero
> > > +     * value for apic_assist_vector from the zero extended save record
> > > +     * will always be correct.
> > > +     */
> > > +    v->arch.hvm_vcpu.viridian.apic_assist.vector =
> ctxt.apic_assist_vector;
> >
> > ... the bias to get accounted for here, instead of quite a bit of
> > other code getting adjusted, and the meaning of the "vector"
> > field getting slightly mis-used.
> >
> > Jan


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