Re: [Xen-devel] [PATCH v3 1/2] xen/pt: fix some pass-thru devices don't work across reboot

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Dec 19, 2018 at 09:57:51AM +0100, Roger Pau Monné wrote:
>On Tue, Dec 18, 2018 at 10:43:37PM +0800, Chao Gao wrote:
>> I find some pass-thru devices don't work any more across guest
>> reboot. Assigning it to another domain also meets the same issue. And
>> the only way to make it work again is un-binding and binding it to
>> pciback. Someone reported this issue one year ago [1].
>> 
>> If the device's driver doesn't disable MSI-X during shutdown or qemu is
>> killed/crashed before the domain shutdown, this domain's pirq won't be
>> unmapped. Then xen takes over this work, unmapping all pirq-s, when
>> destroying guest. But as pciback has already disabled meory decoding before
>> xen unmapping pirq, Xen has to sets the host_maskall flag and maskall bit
>> to mask a MSI rather than sets maskbit in MSI-x table. The call trace of
>> this process is:
>> 
>> ->arch_domain_destroy
>>     ->free_domain_pirqs
>>         ->unmap_domain_pirq (if pirq isn't unmapped by qemu)
>>             ->pirq_guest_force_unbind
>>                 ->__pirq_guest_unbind
>>                     ->mask_msi_irq(=desc->handler->disable())
>>                         ->the warning in msi_set_mask_bit()
>> 
>> The host_maskall bit will prevent guests from clearing the maskall bit
>> even the device is assigned to another guest later. Then guests cannot
>> receive MSIs from this device.
>> 
>> To fix this issue, a pirq is unmapped before memory decoding is disabled by
>> pciback. Specifically, when a device is detached from a guest, all 
>> established
>> mappings between pirq and msi are destroying before changing the ownership.
>> 
>> [1]: 
>> https://lists.xenproject.org/archives/html/xen-devel/2017-09/msg02520.html
>> 
>> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
>> ---
>> Applied this patch, qemu would report the error below:
>>     [00:05.0] msi_msix_disable: Error: Unbinding of MSI-X failed. (err: 1, 
>> pirq: 302, gvec: 0xd5)
>>     [00:05.0] msi_msix_disable: Error: Unbinding of MSI-X failed. (err: 1, 
>> pirq: 301, gvec: 0xe5)
>>     [00:04.0] msi_msix_disable: Error: Unbinding of MSI-X failed. (err: 1, 
>> pirq: 359, gvec: 0x41)
>>     [00:04.0] msi_msix_disable: Error: Unbinding of MSI-X failed. (err: 1, 
>> pirq: 358, gvec: 0x51)
>> 
>> Despite of the error, guest shutdown or device hotplug finishs smoothly.
>> It seems to me that qemu tries to unbind a msi which is already unbound by
>> the code added by this patch. I am not sure whether it is acceptable to
>> leave this error there.
>
>So QEMU would try to unmap IRQs after unbinding the device? I think

It seems to me yes. I don't know the reason right now. maybe because it
is an asynchronous process?

>QEMU should be fixed to first unmap the IRQs and then unbind the
>device.

Yes. Agree.

>
>As long as this doesn't affect QEMU functionality I guess the Xen side
>can be committed, but ideally a QEMU patch to avoid those error
>messages should be committed at the same time.
>
>> ---
>>  xen/drivers/passthrough/io.c  | 57 
>> +++++++++++++++++++++++++++++--------------
>>  xen/drivers/passthrough/pci.c | 49 +++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/iommu.h       |  1 +
>>  3 files changed, 89 insertions(+), 18 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
>> index a6eb8a4..56ee1ef 100644
>> --- a/xen/drivers/passthrough/io.c
>> +++ b/xen/drivers/passthrough/io.c
>> @@ -619,6 +619,42 @@ int pt_irq_create_bind(
>>      return 0;
>>  }
>>  
>> +static void pt_irq_destroy_bind_common(struct domain *d, struct pirq *pirq)
>> +{
>> +    struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>> +
>> +    ASSERT(spin_is_locked(&d->event_lock));
>> +
>> +    if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
>> +         list_empty(&pirq_dpci->digl_list) )
>> +    {
>> +        pirq_guest_unbind(d, pirq);
>> +        msixtbl_pt_unregister(d, pirq);
>> +        if ( pt_irq_need_timer(pirq_dpci->flags) )
>> +            kill_timer(&pirq_dpci->timer);
>> +        pirq_dpci->flags = 0;
>> +        /*
>> +         * See comment in pt_irq_create_bind's PT_IRQ_TYPE_MSI before the
>> +         * call to pt_pirq_softirq_reset.
>> +         */
>> +        pt_pirq_softirq_reset(pirq_dpci);
>> +
>> +        pirq_cleanup_check(pirq, d);
>> +    }
>> +}
>> +
>> +void pt_irq_destroy_bind_msi(struct domain *d, struct pirq *pirq)
>> +{
>> +    struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>> +
>> +    ASSERT(spin_is_locked(&d->event_lock));
>> +
>> +    if ( pirq_dpci && pirq_dpci->gmsi.posted )
>> +        pi_update_irte(NULL, pirq, 0);
>> +
>> +    pt_irq_destroy_bind_common(d, pirq);
>> +}
>> +
>>  int pt_irq_destroy_bind(
>>      struct domain *d, const struct xen_domctl_bind_pt_irq *pt_irq_bind)
>>  {
>> @@ -727,26 +763,11 @@ int pt_irq_destroy_bind(
>>          }
>>          else
>>              what = "bogus";
>> -    }
>> -    else if ( pirq_dpci && pirq_dpci->gmsi.posted )
>> -        pi_update_irte(NULL, pirq, 0);
>> -
>> -    if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
>> -         list_empty(&pirq_dpci->digl_list) )
>> -    {
>> -        pirq_guest_unbind(d, pirq);
>> -        msixtbl_pt_unregister(d, pirq);
>> -        if ( pt_irq_need_timer(pirq_dpci->flags) )
>> -            kill_timer(&pirq_dpci->timer);
>> -        pirq_dpci->flags = 0;
>> -        /*
>> -         * See comment in pt_irq_create_bind's PT_IRQ_TYPE_MSI before the
>> -         * call to pt_pirq_softirq_reset.
>> -         */
>> -        pt_pirq_softirq_reset(pirq_dpci);
>>  
>> -        pirq_cleanup_check(pirq, d);
>> +        pt_irq_destroy_bind_common(d, pirq);
>>      }
>> +    else
>> +        pt_irq_destroy_bind_msi(d, pirq);
>>  
>>      spin_unlock(&d->event_lock);
>>  
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 1277ce2..88a8007 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -368,6 +368,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
>> u8 bus, u8 devfn)
>>              return NULL;
>>          }
>>          spin_lock_init(&msix->table_lock);
>> +        msix->warned = DOMID_INVALID;
>>          pdev->msix = msix;
>>      }
>>  
>> @@ -1514,6 +1515,52 @@ static int assign_device(struct domain *d, u16 seg, 
>> u8 bus, u8 devfn, u32 flag)
>>      return rc;
>>  }
>>  
>> +/*
>> + * Unmap established mappings between domain's pirq and device's MSI.
>> + * These mappings were set up by qemu/guest and are expected to be
>> + * destroyed when changing the device's ownership.
>> + */
>> +static void pci_unmap_msi(struct pci_dev *pdev)
>> +{
>> +    struct msi_desc *entry, *tmp;
>> +
>> +    ASSERT(pcidevs_locked());
>> +
>> +    if ( !pdev->domain )
>> +        return;
>> +
>> +    spin_lock(&pdev->domain->event_lock);
>> +    list_for_each_entry_safe( entry, tmp, &pdev->msi_list, list )
>
>Do you really need the _safe version here? Couldn't you even use:

Don't need the _safe version.

>
>while ( (entry = list_first_entry_or_null(...)) != NULL )
>...

I think it is the same with list_for_each_entry(). Any reason makes you think
this one would be better?

>
>> +    {
>> +        struct pirq *info;
>> +        struct hvm_pirq_dpci *pirq_dpci;
>> +        int pirq = domain_irq_to_pirq(pdev->domain, entry->irq), pirq_orig;
>> +
>> +        pirq_orig = pirq;
>> +
>> +        if ( !pirq )
>> +            continue;
>> +
>> +        /* For forcibly unmapped pirq, lookup radix tree with absolute 
>> value */
>> +        if ( pirq < 0)
>> +            pirq = -pirq;
>
>I'm not sure I follow, the pirq hasn't been unmapped at this point
>yet?

Qemu (i.e. compromised qemu) has the ability to do this. Right? we can't
assert the pirq hasn't been unmapped here.

>
>> +
>> +        info = pirq_info(pdev->domain, pirq);
>> +        if ( !info )
>> +            continue;
>> +        pirq_dpci = pirq_dpci(info);
>> +
>> +        if ( pirq_dpci &&
>> +             (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) &&
>> +             (pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI) )
>> +            pt_irq_destroy_bind_msi(pdev->domain, info);
>
>I think this is missing unbinding for group MSI interrupts, you should
>check the type and if it's MSI (not MSIX) iterate over the number of
>vectors in msi.nvec in order to unbind them?

Good catch.

Thanks
Chao

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