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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <519CFF7602000078000D82E6@nat28.tlf.novell.com>
Date:	Wed, 22 May 2013 16:25:10 +0100
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Konrad Rzeszutek Wilk" <konrad.wilk@...cle.com>
Cc:	"David Vrabel" <david.vrabel@...rix.com>,
	"Stefano Stabellini" <stefano.stabellini@...citrix.com>,
	"Thomas Gleixner" <tglx@...utronix.de>,
	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	"Chien Yen" <chien.yen@...cle.com>,
	"Feng Jin" <joe.jin@...cle.com>,
	"Yuval Shaia" <yuval.shaia@...cle.com>,
	"Zhenzhong Duan" <zhenzhong.duan@...cle.com>,
	"Ingo Molnar" <mingo@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated
 when driver load first time

>>> On 22.05.13 at 17:14, Konrad Rzeszutek Wilk <konrad.wilk@...cle.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/