Re: [Xen-devel] [PATCH] x86/vpt: fix a bug in pt_update_irq()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.17 at 23:32, <chao.gao@xxxxxxxxx> wrote:

First of all - please use a better subject. If someone finds another
bug in this function in, say, half a year's time, how will we tell apart
the two patches from looking at just the list of titles several years
later?

> pt_update_irq() is expected to return the vector number of periodic
> timer interrupt, which should be set in vIRR of vlapic. Otherwise it
> would trigger the assertion in vmx_intr_assist(), please seeing
> https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
> 
> But it fails to achieve that in the following two case:
> 1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
> mask field of IOAPIC RTE is set. Please refer to the call tree
> vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
> assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
> checks whether the vector is set or not in vIRR of vlapic before
> returning.
> 
> 2. someone changes the vector field of IOAPIC RTE between asserting
> the irq and getting the vector of the irq, leading to setting the
> old vector number but returning a different vector number. This patch
> holds the irq_lock when doing the two operations to prevent the case.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>

Point 2 is very unlikely to be the cause of the failed assertion that
osstest keeps hitting once in a while. Did your analysis yield
indication that point 1 is what is happening there?

> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -168,20 +168,23 @@ void hvm_gsi_deassert(struct domain *d, unsigned int 
> gsi)
>      spin_unlock(&d->arch.hvm_domain.irq_lock);
>  }
>  
> -void hvm_isa_irq_assert(
> -    struct domain *d, unsigned int isa_irq)
> +void hvm_isa_irq_assert_locked(struct domain *d, unsigned int isa_irq)

Please don't introduce a non-static function like this. Instead I
would suggest you introduce a new helper function doing what
you introduce as replacement to the call to
hvm_isa_irq_assert(). That'll presumably involve passing a
get_vector() callback to a wrapper of pt_irq_vector() (or to an
abbreviated form of it, as "src" is hvm_intsrc_lapic), since I
understand you need this called with the lock held.

And once you do this I don't think it'll be worthwhile breaking
out hvm_isa_irq_assert_locked() at all - you'll just have a
sibling to hvm_isa_irq_assert(). Or, considering the few callers
the function has, simply giving that function itself an optional
callback parameter might be even better (eliminating any code
duplication).

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -137,6 +137,17 @@ static void vlapic_error(struct vlapic *vlapic, unsigned 
> int errmask)
>      spin_unlock_irqrestore(&vlapic->esr_lock, flags);
>  }
>  
> +bool vlapic_test_irq(struct vlapic *vlapic, uint8_t vec)

The way the function is named, the pointer should be const
qualified. However, the function does more than just testing
current state:

> +{
> +    if ( unlikely(vec < 16) )
> +        return false;
> +
> +    if ( hvm_funcs.sync_pir_to_irr )
> +        hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));

Question is whether this is really necessary, of whether instead
you could just return the state of the respective PIR bit here. I'd
prefer that over giving the function a name no longer suggesting
it leaves all state alone.

Jan


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