Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driver load first time

From: "Jan Beulich" <JBeulich@suse.com>
To: "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
Cc: "David Vrabel" <david.vrabel@citrix.com>,
	"Stefano Stabellini" <stefano.stabellini@eu.citrix.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
	"Chien Yen" <chien.yen@oracle.com>,
	"Feng Jin" <joe.jin@oracle.com>,
	"Yuval Shaia" <yuval.shaia@oracle.com>,
	"Zhenzhong Duan" <zhenzhong.duan@oracle.com>,
	"Ingo Molnar" <mingo@redhat.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driver load first time
Date: Wed, 22 May 2013 16:25:10 +0100	[thread overview]
Message-ID: <519CFF7602000078000D82E6@nat28.tlf.novell.com> (raw)
In-Reply-To: <20130522151437.GC8162@phenom.dumpdata.com>

>>> On 22.05.13 at 17:14, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> The physdev_unmap_pirq (from PHYSDEVOP_unmap_pirq), only has this
> check:
>  if (domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND)
> 
> and since the arch.hvm.emuirq is IRQ_UNBOUND (-1), it does not
> call unmap_domain_pirq_emuirq. It probably shouldn't, but it should
> at least remove the info->arch.pirq = PIRQ_ALLOCATED as we are
> telling the hypervisor: "hey, I am done with this, return to the
> pool." But since that is not cleared, the PHYSDEVOP_get_free_pirq
> will skip this pirq as arch.pirq is still set to PIRQ_ALLOCATED.

Okay, that clarifies it quite a bit. For one, I'll leave any of the
emuirq stuff to Stefano, who wrote this originally. And then, from
the beginning of this thread, I'm not convinced that freeing a pirq
is really the right thing here: unmap_pirq() is the counterpart of
map_pirq(), not get_free_pirq(). I would think that is a guest
allocates a pirq and then unmaps it without first mapping it, it's
the guest's fault that it now lost one pirq resource. It should not
have allocated one in the first place if it didn't mean to use it for
anything.

>> I see none at all, unmap_domain_pirq() has a <= 0 check, and
>> unmap_domain_pirq_emuirq() again doesn't appear to have any.
> 
> The 'unmap_domain_pirq' path would be if dom0 (so QEMU) did the
> unmap for the guest. That is via the PHYSDEVOP_unmap_pirq. And
> I think if that path was taken (as Stefano suggests QEMU should
> do when a MSI or MSI-X driver is unloaded and zero is writen as
> an PIRQ), we would end up calling clear_domain_irq_pirq, which
> would set arch.pirq = 0.
> 
> Or to a negative value as you pointed out later. Which then
> means we won't be ever able to re-use the PIRQ (as
> PHYSDEVOP_get_free_pirq or rather get_free_pirq would skip over it
> as arch.pirq != 0).

That setting to a negative value is not causing the slot to be
permanently lost, it merely defers its freeing. It was the only
way I could find back then to reasonably handle an unmap
being done before the matched unbind.

Jan