Re: [PATCH v5 2/2] x86/hvm/viridian: Enable APIC assist enlightenment

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 18 March 2016 10:25
> To: Paul Durrant
> Cc: Andrew Cooper; Ian Jackson; Stefano Stabellini; Wei Liu; xen-
> devel@lists.xenproject.org; Keir (Xen.org)
> Subject: RE: [PATCH v5 2/2] x86/hvm/viridian: Enable APIC assist
> enlightenment
> 
> >>> On 18.03.16 at 11:06, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 17 March 2016 16:43
> >> >>> On 17.03.16 at 13:13, <paul.durrant@citrix.com> wrote:
> >> > @@ -1170,10 +1183,27 @@ int vlapic_has_pending_irq(struct vcpu *v)
> >> >           !nestedhvm_vcpu_in_guestmode(v) )
> >> >          return irr;
> >> >
> >> > +    /*
> >> > +     * If APIC assist was used then there may have been no EOI so
> >> > +     * we need to clear the requisite bit from the ISR here, before
> >> > +     * comparing with the IRR.
> >> > +     */
> >> > +    if ( viridian_complete_apic_assist(v, &vector) &&
> >> > +         vector != -1 )
> >>
> >> Afaict "vector" is uninitialized here when initialize_apic_assist()
> >> didn't run for that vCPU yet (which includes the case where no
> >> Viridian emulation is active at all).
> >>
> >
> > Yes, vector will be uninitialized in that case but viridian
> > _complete_apic_assist() will return 0 (because the va will be zero) and so
> > the second clause of the if will not be evaluated.
> 
> Ah, true. But raises the question why viridian_complete_apic_assist()
> doesn't return the vector then rather then using indirection.
> 

Yes, I guess that would be more obvious.

> >> > +    /*
> >> > +     * This vector is edge triggered and there are no lower priority
> >> > +     * vectors pending, so we can use APIC assist to avoid exiting
> >> > +     * for EOI.
> >> > +     */
> >> > +    viridian_start_apic_assist(v, vector);
> >> >
> >> > +done:
> >>
> >> Labels indented by at least one space please.
> >
> > OK, sorry, emacs keeps moving them back.
> 
> That's very unfriendly of it.
> 

Indeed.

  Paul

> Jan


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