Re: [PATCH v2] x86/hvm: re-work viridian APIC assist code

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 18 January 2018 16:21
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2] x86/hvm: re-work viridian APIC assist code
> 
> >>> On 18.01.18 at 16:10, <paul.durrant@citrix.com> wrote:
> > -int viridian_complete_apic_assist(struct vcpu *v)
> > +bool viridian_apic_assist_completed(struct vcpu *v)
> >  {
> >      uint32_t *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
> > -    int vector;
> >
> >      if ( !va )
> > -        return 0;
> > +        return false;
> >
> > -    if ( *va & 1u )
> > -        return 0; /* Interrupt not yet processed by the guest. */
> > -
> > -    vector = v->arch.hvm_vcpu.viridian.vp_assist.vector;
> > -    v->arch.hvm_vcpu.viridian.vp_assist.vector = 0;
> > +    if ( v->arch.hvm_vcpu.viridian.vp_assist.pending &&
> > +         !(*va & 1u) )
> > +    {
> > +        /* An EOI has been avoided */
> > +        v->arch.hvm_vcpu.viridian.vp_assist.pending = false;
> > +        return true;
> > +    }
> 
> Especially with such a non-atomic update, please remind me: Despite
> them having struct vcpu * parameters, these functions are all only
> ever called with v == current? If the answer is yes, then
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks. The answer is yes. They are only called from the vlapic code for the current vlapic AFAICT.

> with one more cosmetic remark:
> 
> > @@ -120,9 +120,9 @@ void viridian_time_ref_count_thaw(struct domain
> *d);
> >  void viridian_vcpu_deinit(struct vcpu *v);
> >  void viridian_domain_deinit(struct domain *d);
> >
> > -void viridian_start_apic_assist(struct vcpu *v, int vector);
> > -int viridian_complete_apic_assist(struct vcpu *v);
> > -void viridian_abort_apic_assist(struct vcpu *v);
> > +void viridian_set_apic_assist(struct vcpu *v);
> > +bool viridian_apic_assist_completed(struct vcpu *v);
> > +void viridian_clear_apic_assist(struct vcpu *v);
> 
> Could I talk you into naming them all viridian_apic_assist_...()?
> 

Sure. I'll send a v3 with that fixed and your R-b.

  Paul

> Jan


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