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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Oct 13, 2017 at 02:25:38AM -0600, Jan Beulich wrote:
>>>> 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?

Will update.

>
>> 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?

I believe it is likely to be the case. On the other hand, the
assertion can be triggered in above two cases; it needs to be
fixed.

>
>> --- 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).

Ok. I understand what you suggestion. Will give it a shot.

>
>> --- 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.

It is a good suggestion. But I incline to check the PIR bit and if the
bit is set, return true and otherwise return the state of the vIRR bit
in case PIR bits are already synced to vIRR.

Thanks
Chao

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