From: "Jan Beulich" <JBeulich@suse.com>
To: Chao Gao <chao.gao@intel.com>
Cc: Juergen Gross <jgross@suse.com>,
Stefano Stabellini <sstabellini@kernel.org>,
linux-kernel@vger.kernel.org, Jia-Ju Bai <baijiaju1990@gmail.com>,
xen-devel <xen-devel@lists.xenproject.org>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH] xen: xen-pciback: Reset MSI-X state when exposing a device
Date: Thu, 13 Dec 2018 00:54:52 -0700 [thread overview]
Message-ID: <5C12104C0200007800205B4B__33160.3981824175$1544687620$gmane$org@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20181213034657.GA26926@gao-cwp>
>>> On 13.12.18 at 04:46, <chao.gao@intel.com> wrote:
> On Wed, Dec 12, 2018 at 08:21:39AM -0700, Jan Beulich wrote:
>>>>> On 12.12.18 at 16:18, <chao.gao@intel.com> wrote:
>>> On Wed, Dec 12, 2018 at 01:51:01AM -0700, Jan Beulich wrote:
>>>>>>> On 12.12.18 at 08:06, <chao.gao@intel.com> 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.
>>
>>How would pciback even know of those mappings, when it's qemu
>>who establishes (and manages) them?
>
> I meant to expose some interfaces from pciback. And pciback serves
> as the proxy of IRQ (un)binding APIs.
If at all possible we should avoid having to change more parties (qemu,
libxc, kernel, hypervisor) than really necessary. Remember that such
a bug fix may want backporting, and making sure affected people have
all relevant components updated is increasingly difficult with their
number growing.
>>>>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).
>>
>>Even in the hot-unplug case the tool stack could issue unbind
>>requests, behind the back of the possibly compromised qemu,
>>once neither the guest nor qemu have access to the device
>>anymore.
>
> But currently, tool stack doesn't know the remaining IRQ bindings.
> If tool stack can maintaine IRQ binding information of a pass-thru
> device (stored in Xenstore?), we can come up with a clean solution
> without modifying linux kernel and Xen.
If there's no way for the tool stack to either find out the bindings
or "blindly" issue unbind requests (accepting them to fail), then a
"wildcard" unbind operation may want adding. Or, perhaps even
better, XEN_DOMCTL_deassign_device could unbind anything left
in place for the specified device. I actually wonder why that's not
already the case.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2018-12-13 7:54 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1543976357-1053-1-git-send-email-chao.gao@intel.com>
2018-12-05 9:32 ` [PATCH] xen: xen-pciback: Reset MSI-X state when exposing a device Roger Pau Monné
2018-12-05 16:26 ` Jan Beulich
[not found] ` <20181205093223.dncg4nq4dh6xmrhk@mac>
2018-12-05 14:01 ` Boris Ostrovsky
2018-12-06 2:18 ` Chao Gao
[not found] ` <c27236d3-6125-4049-6268-3d9c93cf3ef2@oracle.com>
2018-12-12 7:06 ` Chao Gao
[not found] ` <20181212070654.GA13411@gao-cwp>
2018-12-12 8:51 ` Jan Beulich
[not found] ` <5C10CBF50200007800205596@prv1-mh.provo.novell.com>
2018-12-12 15:18 ` Chao Gao
[not found] ` <20181212151824.GA17227@gao-cwp>
2018-12-12 15:21 ` Jan Beulich
[not found] ` <5C112783020000780020589C@prv1-mh.provo.novell.com>
2018-12-13 3:46 ` Chao Gao
[not found] ` <20181213034657.GA26926@gao-cwp>
2018-12-13 7:54 ` Jan Beulich [this message]
[not found] ` <5C12104C0200007800205B4B@prv1-mh.provo.novell.com>
2018-12-13 13:17 ` Chao Gao
2019-09-13 10:02 ` [Xen-devel] " Spassov, Stanislav
2019-09-13 15:28 ` Chao Gao
2019-09-26 10:13 ` Pasi Kärkkäinen
2019-09-26 10:54 ` Spassov, Stanislav
2020-01-17 18:57 ` Rich Persaud
2020-01-18 1:13 ` Chao Gao
2018-12-05 2:19 Chao Gao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='5C12104C0200007800205B4B__33160.3981824175$1544687620$gmane$org@prv1-mh.provo.novell.com' \
--to=jbeulich@suse.com \
--cc=baijiaju1990@gmail.com \
--cc=boris.ostrovsky@oracle.com \
--cc=chao.gao@intel.com \
--cc=jgross@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).