Re: [PATCH] xen: xen-pciback: Reset MSI-X state when exposing a device

[Chao Gao]

On Wed, Dec 12, 2018 at 01:51:01AM -0700, Jan Beulich wrote:
>>>> On 12.12.18 at 08:06, <chao.gao@xxxxxxxxx> wrote:
>> On Wed, Dec 05, 2018 at 09:01:33AM -0500, Boris Ostrovsky wrote:
>>>On 12/5/18 4:32 AM, Roger Pau Monné wrote:
>>>> On Wed, Dec 05, 2018 at 10:19:17AM +0800, Chao Gao wrote:
>>>>> I find some pass-thru devices don't work any more across guest reboot.
>>>>> Assigning it to another guest 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]. More detail also can be
>>>>> found in [2].
>>>>>
>>>>> The root-cause is Xen's internal MSI-X state isn't reset properly
>>>>> during reboot or re-assignment. In the above case, Xen set maskall bit
>>>>> to mask all MSI interrupts after it detected a potential security
>>>>> issue. Even after device reset, Xen didn't reset its internal maskall
>>>>> bit. As a result, maskall bit would be set again in next write to
>>>>> MSI-X message control register.
>>>>>
>>>>> Given that PHYSDEVOPS_prepare_msix() also triggers Xen resetting MSI-X
>>>>> internal state of a device, we employ it to fix this issue rather than
>>>>> introducing another dedicated sub-hypercall.
>>>>>
>>>>> Note that PHYSDEVOPS_release_msix() will fail if the mapping between
>>>>> the device's msix and pirq has been created. This limitation prevents
>>>>> us calling this function when detaching a device from a guest during
>>>>> guest shutdown. Thus it is called right before calling
>>>>> PHYSDEVOPS_prepare_msix().
>>>> s/PHYSDEVOPS/PHYSDEVOP/ (no final S). And then I would also drop the
>>>> () at the end of the hypercall name since it's not a function.
>>>>
>>>> I'm also wondering why the release can't be done when the device is
>>>> detached from the guest (or the guest has been shut down). This makes
>>>> me worry about the raciness of the attach/detach procedure: if there's
>>>> a state where pciback assumes the device has been detached from the
>>>> guest, but there are still pirqs bound, an attempt to attach to
>>>> another guest in such state will fail.
>>>
>>>I wonder whether this additional reset functionality could be done out
>>>of xen_pcibk_xenbus_remove(). We first do a (best effort) device reset
>>>and then do the extra things that are not properly done there.
>> 
>> No. It cannot be done in xen_pcibk_xenbus_remove() without modifying
>> the handler of PHYSDEVOP_release_msix. To do a successful Xen internal
>> MSI-X state reset, PHYSDEVOP_{release, prepare}_msix should be finished
>> without error. But ATM, xen expects that no msi is bound to pirq when
>> doing PHYSDEVOP_release_msix. Otherwise it fails with error code -EBUSY.
>> However, the expectation isn't guaranteed in xen_pcibk_xenbus_remove().
>> In some cases, if qemu fails to unmap MSIs, MSIs are unmapped by Xen
>> at last minute, which happens after device reset in 
>> xen_pcibk_xenbus_remove().
>
>But that may need taking care of: I don't think it is a good idea to have
>anything left from the prior owning domain when the device gets reset.
>I.e. left over IRQ bindings should perhaps be forcibly cleared before
>invoking the reset;

Agree. How about pciback to track the established IRQ bindings? Then
pciback can clear irq binding before invoking the reset.

>in fact I'd expect this to happen in the course of
>domain destruction, and I'd expect the device reset to come after the
>domain was cleaned up. Perhaps simply an ordering issue in the tool
>stack?

I don't think reversing the sequences of device reset and domain
destruction would be simple. Furthermore, during device hot-unplug,
device reset is done when the owner is alive. So if we use domain
destruction to enforce all irq binding cleared, in theory, it won't be
applicable to hot-unplug case (if qemu's hot-unplug logic is
compromised).

Thanks
Chao