Re: [Xen-devel] [PATCH] x86/pt: skip setup of posted format IRTE when gvec is 0

[Chao Gao <chao.gao@xxxxxxxxx>]

On Tue, Apr 30, 2019 at 01:56:31AM -0600, Jan Beulich wrote:
>>>> On 30.04.19 at 07:19, <chao.gao@xxxxxxxxx> wrote:
>> When testing with an UP guest with a pass-thru device with vt-d pi
>> enabled in host, we observed that guest couldn't receive interrupts
>> from that pass-thru device. Dumping IRTE, we found the corresponding
>> IRTE is set to posted format with "vector" field as 0.
>> 
>> We would fall into this issue when guest used the pirq format of MSI
>> (see the comment xen_msi_compose_msg() in linux kernel). As 'dest_id'
>> is repurposed, skip migration which is based on 'dest_id'.
>
>I've gone through all uses of gvec, and I couldn't find any existing
>special casing of it being zero. I assume this is actually communication
>between the kernel and qemu,

Yes. 

>in which case I'd like to see an
>explanation of why the issue needs to be addressed in Xen rather
>than qemu.

To call pirq_guest_bind() to configure irq_desc properly.
Especially, we append a pointer of struct domain to 'action->guest' in
pirq_guest_bind(). Then __do_IRQ_guest() knows domains that are interested
in this interrupt and injects an interrupt to those domains.

>Otherwise, if I've overlooked something, would you
>mind pointing out where such special casing lives in Xen?
>
>In any event it doesn't look correct to skip migration altogether in
>that case. I'd rather expect it to require getting done differently.
>After all there still is a (CPU, vector) tuple associated with that
>{,p}IRQ if it's not posted, and hvm_migrate_pirq() is a no-op if it is
>posted.

Here, we try to set irq's target cpu to the cpu which the vmsi's target vcpu
is running on to reduce IPI. But the 'dest_id' field which used to
indicate the vmsi's target vcpu is missing, we don't know which cpu we should
migrate the irq to. One possible choice is the 'chn->notify_vcpu_id'
used in send_guest_pirq(). Do you think this choice is fine?

>
>Finally it hasn't become clear to me why this is a UP-guest issue
>only.

For SMP case, it happens to work. hvm_girq_dest_2_vcpu_id() would return
-1 for SMP in most cases. And then we won't use VT-d PI if there is no
dest vcpu. For UP case, hvm_girq_dest_2_vcpu_id() returns 0 without matching.

>
>> --- a/xen/drivers/passthrough/io.c
>> +++ b/xen/drivers/passthrough/io.c
>> @@ -413,34 +413,52 @@ int pt_irq_create_bind(
>>                  pirq_dpci->gmsi.gflags = gflags;
>>              }
>>          }
>> -        /* Calculate dest_vcpu_id for MSI-type pirq migration. */
>> -        dest = MASK_EXTR(pirq_dpci->gmsi.gflags,
>> -                         XEN_DOMCTL_VMSI_X86_DEST_ID_MASK);
>> -        dest_mode = pirq_dpci->gmsi.gflags & XEN_DOMCTL_VMSI_X86_DM_MASK;
>> -        delivery_mode = MASK_EXTR(pirq_dpci->gmsi.gflags,
>> -                                  XEN_DOMCTL_VMSI_X86_DELIV_MASK);
>> -
>> -        dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>> -        pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
>> -        spin_unlock(&d->event_lock);
>> -
>> -        pirq_dpci->gmsi.posted = false;
>> -        vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
>> -        if ( iommu_intpost )
>> +        /*
>> +         * Migrate pirq and create posted format IRTE only if we know the 
>> gmsi's
>> +         * dest_id and vector.
>> +         */
>> +        if ( pirq_dpci->gmsi.gvec )
>
>If we're to go with this hypervisor side change, please insert a
>blank line above the comment.

Will do

Thanks
Chao

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