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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 18 January 2018 16:21
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] x86/hvm: re-work viridian APIC assist code
> 
> >>> On 18.01.18 at 16:10, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel