[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/3] x86/irq: don't disable domain MSI IRQ on force unbind


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 26 Jan 2021 17:21:57 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=ZSAkpewS/T7g/TCWm/Xp18FU4aJB+e6JQoCvcrbfApk=; b=H2saXHR/W05siwJcUp/psQBulyfM2XHxs4Z1Ymt/uYIrBn6pYCixHM7HtOoAIRWNS+3xaOKdmb24GnaO/b4mTItuSeJULtSK+PFX+YmsR0BdzmBf+Mztgd6YXznmMqIroiqZavy7+tCt6n0pCeTxsYW5jkVZ4mHPrs7/nXJeXjbmPEVcmHT/K3HxUAGI8HHttCUHAei0ewWE9OXd7BfRQYRLKVdzLoQKF9nMLuZA7y6PLM1BVsorxyr/1qkg4FUwopj/ajSnPf8jwlKlXGdaeij3l5ylvMI2zgcj8JiGvvBPprkxvjDZOVQo+jMT6MXWsjbK7i8l5yIv/q93mdYf1A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MDaE77cf5xEIBn8EbdBjcRiZV6J/5AAFjGf8/z8whrs/ll6S8pB70FxceTMVPzWHYR+D7ZuPJS9eKVfsg4UsGkf9l1eytWCwjluuJX1zZa7Ezr6PKFVRTqHyOC3EaCYz6ErrVUEHfX/EWS2lHxGF+QTG/o3TKS3Cd3QL0iv8/qme3ZOULjngeDWtvRAMgFoivU3SzlxzZpjtksDhROZEAGRG+UPTaRfciSLyrb8zccCG3inECaNPt9lMQxe1X2T82TorRZ3iIiiLxAVwqfo6CQcJFCnvz0dn/v4zSvu+kg89QT+N4SIFA2S9rvI8Jk107NonzZyDD2i4FXxYIjrofQ==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 26 Jan 2021 16:22:28 +0000
  • Ironport-sdr: xTLkb/BL6aLnMGwG8Y6t0sQW1MouWy/F7zPfMltAL/wPU079/0CWlvZFVwJqQF43D6OiPyT7jP 9xpgbQlKFvnxh0TiTmAM4eN0WD8G4Rh8CL/ktk1A1Y7qAd5S/kbMWrgrDM7L/U/qCmKhHp3KkA lRCyvkv9pFIrWGV6Z53eEuweApwQF7gCsfFJUgnQHW+yZFYiZPUWTX/jJJaqgAMTvT3WawxlDP KWNgps4Zvn0YvzxroYKu66ukPJRj070VNYtnFNAo+VS+VvCZqo4eeJg1VhCao5icyS4Aq/bN6C j4I=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Jan 26, 2021 at 03:52:54PM +0100, Jan Beulich wrote:
> On 26.01.2021 12:06, Roger Pau Monne wrote:
> > When force unbinding a MSI the used IRQ would get added to the domain
> > irq-pirq tree as -irq -pirq,
> 
> I think it's -pirq at index irq, i.e. I don't think IRQ gets
> negated as far as the radix tree goes. info->arch.irq gets a
> negative value stored, yes.

Right, and this then prevents the IRQ to be used at all by the domain.
Doiong a domain_irq_to_pirq with that IRQ will get -pirq, but that
seems pretty arbitrary for MSI IRQs, that get allocated on demand.

At the end of unmap_domain_pirq the IRQ will get freed if it was
assigned to an MSI source, and hence it seem pointless to add irq ->
-pirq to the domain irq tree.

> > thus preventing the same IRQ to be used by the domain.
> 
> Iirc this (answering your post-commit-message question here)
> is for cleaning up _after_ the domain, i.e. there's no goal
> to allow re-use of this IRQ. The comment ahead of
> unmap_domain_pirq() validly says "The pirq should have been
> unbound before this call." The only time we can't make
> ourselves dependent upon this is when the guest is being
> cleaned up. During normal operation I think we actually
> _want_ to enforce correct behavior of the guest here.

OK, so that might be fine for legacy PCI IRQs, that are fixed, but
quite pointless for allocated on demand MSI IRQs that can change
between allocations.

> > It's not clear to me why we add the irq as -irq -pirq to the irq-pirq
> > domain tree on forced unbind, as allowing to bind the irq again should
> > just work regardless of whether it has been previously forcefully
> > unbound?
> 
> To continue from the above, see pirq_guest_unbind() where
> we have
> 
>     if ( desc == NULL )
>     {
>         irq = -pirq->arch.irq;
>         BUG_ON(irq <= 0);
>         desc = irq_to_desc(irq);
>         spin_lock_irq(&desc->lock);
>         clear_domain_irq_pirq(d, irq, pirq);
>     }
> 
> as the alternative to going the normal path through
> __pirq_guest_unbind(). Again iirc that's to cover for the
> unbind request arriving after the unmap one (i.e. having
> caused the unmap to force-unbind the IRQ).

Oh, so that's the point. Do you think you could add some comments to
explain the indented behaviour? I think I get it now, but it's hard to
follow without some pointers.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.