Re: [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Chao Gao <chao.gao@intel.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Ian Jackson <ian.jackson@eu.citrix.com>,
	xen-devel@lists.xen.org, Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
Date: Thu, 5 Apr 2018 12:25:26 +0100	[thread overview]
Message-ID: <20180405112526.6f6dzepc4qpd4ve7@MacBook-Pro-de-Roger.local> (raw)
In-Reply-To: <20180405110040.GA141879@skl-4s-chao.sh.intel.com>

On Thu, Apr 05, 2018 at 07:00:41PM +0800, Chao Gao wrote:
> On Thu, Apr 05, 2018 at 10:34:39AM +0100, Roger Pau Monné wrote:
> >On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote:
> >> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> >> index 5567990..2abf2cf 100644
> >> --- a/xen/arch/x86/msi.c
> >> +++ b/xen/arch/x86/msi.c
> >> @@ -992,7 +992,9 @@ static int msix_capability_init(struct pci_dev *dev,
> >>          if ( rangeset_add_range(mmio_ro_ranges, msix->table.first,
> >>                                  msix->table.last) )
> >>              WARN();
> >> -        if ( rangeset_add_range(mmio_ro_ranges, msix->pba.first,
> >> +
> >> +        if ( !msix->pba_quirk_enabled &&
> >> +             rangeset_add_range(mmio_ro_ranges, msix->pba.first,
> >>                                  msix->pba.last) )
> >>              WARN();
> >
> >This will work fine as long as the PBA is not in the same page as the
> >MSI-X table. In such case you will also need changes to QEMU (see
> >pci_msix_write), so that writes to the memory in the same page as the
> >MSI-X/PBA tables are forwarded to the underlying hardware.
> >
> >You should add least add something like:
> >
> >if ( msix->pba_quirk_enabled &&
> >     msix->table.first <= msix->pba.last &&
> >     msix->pba.first <= msix->table.last )
> >{
> >    printk("PBA write not allowed to dev %04x:%02x:%02x.%u due to MSI-X table overlap\n");
> >    return -ENXIO;
> >}
> >
> >Or similar if the QEMU side is not fixed.
> >
> >Note that in order to fix the QEMU side you would probably have to add
> >a flag to xl 'pci' config option and pass it to both QEMU and Xen.
> 
> Thanks for your comments.
> 
> First of all, I don't intend to also support devices which has MSI-X
> table, MSI-X PBA and other MSI-X irrelevant registers in the same page.
> Because as you said, it cleary violates MSI-X spec. IMO, we can extend
> the workaround when we found such a device.
>
> I also had the same concern with yours. But after careful thinking, I
> found it wouldn't be a problem. If MSI-X table resides the same pages
> with MSI-X PBA, we will mark the pages as RO when handling MSI-X table.
> As a consequence, guest isn't able to write MSI-X table directly in this
> case. Hence, it won't affect MSI-X table emulation and furthermore the
> quirk won't override the decision, marking the page RO, made for other
> reasons.

My suggestion is not because it would be dangerous from a security
PoV, it's simply because the quirk won't be applied, and hence we need
to notify the user that the desired quirk has not been applied.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel