Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On Thu, Apr 05, 2018 at 11:08:59AM +0100, George Dunlap wrote: >On 04/05/2018 10:59 AM, Roger Pau Monné wrote: >> On Thu, Apr 05, 2018 at 10:52:09AM +0100, George Dunlap wrote: >>> On 04/05/2018 10:46 AM, Roger Pau Monné wrote: On Thu, Apr 05, 2018 at 10:40:37AM +0100, George Dunlap wrote: > On 04/05/2018 10:34 AM, Roger Pau Monné wrote: >> On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: >>> ... the same page with other registers which are not relevant to MSI-X. >>> Xen >>> marks pages where PBA resides as read-only. When assigning such devices >>> to >>> guest, device driver writes MSI-X irrelevant registers on those pages >>> would >>> lead to an EPT violation and the guest is destroyed because no handler >>> is >>> registered for those address range. In order to make guest capable to >>> use such >>> kind of devices, trapping very frequent write accesses is not a good >>> idea for >>> it would significantly impact the performance. >>> >>> This patch provides a workaround with caveat. Specifically, an option is >>> introduced to specify a list of devices. For those devices, Xen doesn't >>> control the access right to pages where PBA resides. Hence, guest device >>> driver is able to write those pages and functions well. Note that >>> adding an >>> untrusted device to this option may endanger security of the entire >>> system. >>> >>> Signed-off-by: Chao Gao>>> --- >>> docs/misc/xen-command-line.markdown | 10 + >>> xen/arch/x86/msi.c | 7 -- >>> xen/drivers/passthrough/pci.c | 45 >>> +++-- >>> xen/include/asm-x86/msi.h | 1 + >>> 4 files changed, 59 insertions(+), 4 deletions(-) >>> >>> diff --git a/docs/misc/xen-command-line.markdown >>> b/docs/misc/xen-command-line.markdown >>> index b353352..e382513 100644 >>> --- a/docs/misc/xen-command-line.markdown >>> +++ b/docs/misc/xen-command-line.markdown >>> @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. >>> >>> > Default: `on` >>> >>> +### pba\_quirk >> >> pba_write_allowed would be better, pba_quirk is too generic IMO. > > 'quirk' was I think requested by Jan; and my understanding is that the > word clearly indicates that the behavior in question is a workaround for > hardware which is not compliant with the appropriate specification. If > you grep the source tree for 'quirk' you'll find a fairly large number. > > pba_shared_quirk might be slightly more descriptive. pba_write_quirk? I just think it should be slightly more descriptive than pba_quirk in case Xen has to add further PBA-related quirks in the future. >>> >>> "shared" tells you something about the quirk itself: The PBA is shared >>> across multiple devices. "write" tells you about the work-around: >>> unsafe writes to the PBA region are allowed. >> >> I don't think the PBA page is shared with multiple devices in any >> case. The problem here is that the PBA page contains other registers >> (from the same device as the PBA) that must be RW instead of RO. > >Yes, I realized that after I'd clicked 'send'. "Shared" still makes >sense though: the pba shares a page with registers which must be kept RO. pba_shared_quirk is fine with me. I will use it. Thanks Chao ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On Thu, Apr 05, 2018 at 12:25:26PM +0100, Roger Pau Monné wrote: >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. Got it. It is reasonable. Thanks Chao ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
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: >> ... the same page with other registers which are not relevant to MSI-X. Xen >> marks pages where PBA resides as read-only. When assigning such devices to >> guest, device driver writes MSI-X irrelevant registers on those pages would >> lead to an EPT violation and the guest is destroyed because no handler is >> registered for those address range. In order to make guest capable to use >> such >> kind of devices, trapping very frequent write accesses is not a good idea for >> it would significantly impact the performance. >> >> This patch provides a workaround with caveat. Specifically, an option is >> introduced to specify a list of devices. For those devices, Xen doesn't >> control the access right to pages where PBA resides. Hence, guest device >> driver is able to write those pages and functions well. Note that adding an >> untrusted device to this option may endanger security of the entire system. >> >> Signed-off-by: Chao Gao>> --- >> docs/misc/xen-command-line.markdown | 10 + >> xen/arch/x86/msi.c | 7 -- >> xen/drivers/passthrough/pci.c | 45 >> +++-- >> xen/include/asm-x86/msi.h | 1 + >> 4 files changed, 59 insertions(+), 4 deletions(-) >> >> diff --git a/docs/misc/xen-command-line.markdown >> b/docs/misc/xen-command-line.markdown >> index b353352..e382513 100644 >> --- a/docs/misc/xen-command-line.markdown >> +++ b/docs/misc/xen-command-line.markdown >> @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. >> >> > Default: `on` >> >> +### pba\_quirk > >pba_write_allowed would be better, pba_quirk is too generic IMO. > >> +> `= List of [:]:. >> + >> +Specify a list of SBDF of devices. When assigning devices in this list to >> +guest, reading or writing the page where MSI-X PBA resides are allowed. >> +This option provides a workaround for nonstandard PCI devices whose >> +MSI-X PBA shares the same 4K-byte page with other registers. Note that >> +adding an untrusted device to this option would undermine security of >> +the entire system. >> + >> ### pci >> > `= {no-}serr | {no-}perr` >> >> 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. > >> >> @@ -1139,7 +1141,8 @@ static void _pci_cleanup_msix(struct arch_msix *msix) >> if ( rangeset_remove_range(mmio_ro_ranges, msix->table.first, >> msix->table.last) ) >> WARN(); >> -if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first, >> +if ( !msix->pba_quirk_enabled && >> + rangeset_remove_range(mmio_ro_ranges, msix->pba.first, >> msix->pba.last) ) >> WARN(); >> } >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >>
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On 04/05/2018 10:59 AM, Roger Pau Monné wrote: > On Thu, Apr 05, 2018 at 10:52:09AM +0100, George Dunlap wrote: >> On 04/05/2018 10:46 AM, Roger Pau Monné wrote: >>> On Thu, Apr 05, 2018 at 10:40:37AM +0100, George Dunlap wrote: On 04/05/2018 10:34 AM, Roger Pau Monné wrote: > On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: >> ... the same page with other registers which are not relevant to MSI-X. >> Xen >> marks pages where PBA resides as read-only. When assigning such devices >> to >> guest, device driver writes MSI-X irrelevant registers on those pages >> would >> lead to an EPT violation and the guest is destroyed because no handler is >> registered for those address range. In order to make guest capable to >> use such >> kind of devices, trapping very frequent write accesses is not a good >> idea for >> it would significantly impact the performance. >> >> This patch provides a workaround with caveat. Specifically, an option is >> introduced to specify a list of devices. For those devices, Xen doesn't >> control the access right to pages where PBA resides. Hence, guest device >> driver is able to write those pages and functions well. Note that adding >> an >> untrusted device to this option may endanger security of the entire >> system. >> >> Signed-off-by: Chao Gao>> --- >> docs/misc/xen-command-line.markdown | 10 + >> xen/arch/x86/msi.c | 7 -- >> xen/drivers/passthrough/pci.c | 45 >> +++-- >> xen/include/asm-x86/msi.h | 1 + >> 4 files changed, 59 insertions(+), 4 deletions(-) >> >> diff --git a/docs/misc/xen-command-line.markdown >> b/docs/misc/xen-command-line.markdown >> index b353352..e382513 100644 >> --- a/docs/misc/xen-command-line.markdown >> +++ b/docs/misc/xen-command-line.markdown >> @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. >> >> > Default: `on` >> >> +### pba\_quirk > > pba_write_allowed would be better, pba_quirk is too generic IMO. 'quirk' was I think requested by Jan; and my understanding is that the word clearly indicates that the behavior in question is a workaround for hardware which is not compliant with the appropriate specification. If you grep the source tree for 'quirk' you'll find a fairly large number. pba_shared_quirk might be slightly more descriptive. >>> >>> pba_write_quirk? >>> >>> I just think it should be slightly more descriptive than pba_quirk in >>> case Xen has to add further PBA-related quirks in the future. >> >> "shared" tells you something about the quirk itself: The PBA is shared >> across multiple devices. "write" tells you about the work-around: >> unsafe writes to the PBA region are allowed. > > I don't think the PBA page is shared with multiple devices in any > case. The problem here is that the PBA page contains other registers > (from the same device as the PBA) that must be RW instead of RO. Yes, I realized that after I'd clicked 'send'. "Shared" still makes sense though: the pba shares a page with registers which must be kept RO. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On Thu, Apr 05, 2018 at 10:52:09AM +0100, George Dunlap wrote: > On 04/05/2018 10:46 AM, Roger Pau Monné wrote: > > On Thu, Apr 05, 2018 at 10:40:37AM +0100, George Dunlap wrote: > >> On 04/05/2018 10:34 AM, Roger Pau Monné wrote: > >>> On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: > ... the same page with other registers which are not relevant to MSI-X. > Xen > marks pages where PBA resides as read-only. When assigning such devices > to > guest, device driver writes MSI-X irrelevant registers on those pages > would > lead to an EPT violation and the guest is destroyed because no handler is > registered for those address range. In order to make guest capable to > use such > kind of devices, trapping very frequent write accesses is not a good > idea for > it would significantly impact the performance. > > This patch provides a workaround with caveat. Specifically, an option is > introduced to specify a list of devices. For those devices, Xen doesn't > control the access right to pages where PBA resides. Hence, guest device > driver is able to write those pages and functions well. Note that adding > an > untrusted device to this option may endanger security of the entire > system. > > Signed-off-by: Chao Gao> --- > docs/misc/xen-command-line.markdown | 10 + > xen/arch/x86/msi.c | 7 -- > xen/drivers/passthrough/pci.c | 45 > +++-- > xen/include/asm-x86/msi.h | 1 + > 4 files changed, 59 insertions(+), 4 deletions(-) > > diff --git a/docs/misc/xen-command-line.markdown > b/docs/misc/xen-command-line.markdown > index b353352..e382513 100644 > --- a/docs/misc/xen-command-line.markdown > +++ b/docs/misc/xen-command-line.markdown > @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. > > > Default: `on` > > +### pba\_quirk > >>> > >>> pba_write_allowed would be better, pba_quirk is too generic IMO. > >> > >> 'quirk' was I think requested by Jan; and my understanding is that the > >> word clearly indicates that the behavior in question is a workaround for > >> hardware which is not compliant with the appropriate specification. If > >> you grep the source tree for 'quirk' you'll find a fairly large number. > >> > >> pba_shared_quirk might be slightly more descriptive. > > > > pba_write_quirk? > > > > I just think it should be slightly more descriptive than pba_quirk in > > case Xen has to add further PBA-related quirks in the future. > > "shared" tells you something about the quirk itself: The PBA is shared > across multiple devices. "write" tells you about the work-around: > unsafe writes to the PBA region are allowed. I don't think the PBA page is shared with multiple devices in any case. The problem here is that the PBA page contains other registers (from the same device as the PBA) that must be RW instead of RO. Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On 04/05/2018 10:46 AM, Roger Pau Monné wrote: > On Thu, Apr 05, 2018 at 10:40:37AM +0100, George Dunlap wrote: >> On 04/05/2018 10:34 AM, Roger Pau Monné wrote: >>> On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: ... the same page with other registers which are not relevant to MSI-X. Xen marks pages where PBA resides as read-only. When assigning such devices to guest, device driver writes MSI-X irrelevant registers on those pages would lead to an EPT violation and the guest is destroyed because no handler is registered for those address range. In order to make guest capable to use such kind of devices, trapping very frequent write accesses is not a good idea for it would significantly impact the performance. This patch provides a workaround with caveat. Specifically, an option is introduced to specify a list of devices. For those devices, Xen doesn't control the access right to pages where PBA resides. Hence, guest device driver is able to write those pages and functions well. Note that adding an untrusted device to this option may endanger security of the entire system. Signed-off-by: Chao Gao--- docs/misc/xen-command-line.markdown | 10 + xen/arch/x86/msi.c | 7 -- xen/drivers/passthrough/pci.c | 45 +++-- xen/include/asm-x86/msi.h | 1 + 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index b353352..e382513 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. > Default: `on` +### pba\_quirk >>> >>> pba_write_allowed would be better, pba_quirk is too generic IMO. >> >> 'quirk' was I think requested by Jan; and my understanding is that the >> word clearly indicates that the behavior in question is a workaround for >> hardware which is not compliant with the appropriate specification. If >> you grep the source tree for 'quirk' you'll find a fairly large number. >> >> pba_shared_quirk might be slightly more descriptive. > > pba_write_quirk? > > I just think it should be slightly more descriptive than pba_quirk in > case Xen has to add further PBA-related quirks in the future. "shared" tells you something about the quirk itself: The PBA is shared across multiple devices. "write" tells you about the work-around: unsafe writes to the PBA region are allowed. I think it makes more sense for the name to describe the quirk itself rather than the work-around. The description says what the work-around does and why it's unsafe. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On Thu, Apr 05, 2018 at 10:40:37AM +0100, George Dunlap wrote: > On 04/05/2018 10:34 AM, Roger Pau Monné wrote: > > On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: > >> ... the same page with other registers which are not relevant to MSI-X. Xen > >> marks pages where PBA resides as read-only. When assigning such devices to > >> guest, device driver writes MSI-X irrelevant registers on those pages would > >> lead to an EPT violation and the guest is destroyed because no handler is > >> registered for those address range. In order to make guest capable to use > >> such > >> kind of devices, trapping very frequent write accesses is not a good idea > >> for > >> it would significantly impact the performance. > >> > >> This patch provides a workaround with caveat. Specifically, an option is > >> introduced to specify a list of devices. For those devices, Xen doesn't > >> control the access right to pages where PBA resides. Hence, guest device > >> driver is able to write those pages and functions well. Note that adding an > >> untrusted device to this option may endanger security of the entire system. > >> > >> Signed-off-by: Chao Gao> >> --- > >> docs/misc/xen-command-line.markdown | 10 + > >> xen/arch/x86/msi.c | 7 -- > >> xen/drivers/passthrough/pci.c | 45 > >> +++-- > >> xen/include/asm-x86/msi.h | 1 + > >> 4 files changed, 59 insertions(+), 4 deletions(-) > >> > >> diff --git a/docs/misc/xen-command-line.markdown > >> b/docs/misc/xen-command-line.markdown > >> index b353352..e382513 100644 > >> --- a/docs/misc/xen-command-line.markdown > >> +++ b/docs/misc/xen-command-line.markdown > >> @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. > >> > >> > Default: `on` > >> > >> +### pba\_quirk > > > > pba_write_allowed would be better, pba_quirk is too generic IMO. > > 'quirk' was I think requested by Jan; and my understanding is that the > word clearly indicates that the behavior in question is a workaround for > hardware which is not compliant with the appropriate specification. If > you grep the source tree for 'quirk' you'll find a fairly large number. > > pba_shared_quirk might be slightly more descriptive. pba_write_quirk? I just think it should be slightly more descriptive than pba_quirk in case Xen has to add further PBA-related quirks in the future. Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On 04/05/2018 10:34 AM, Roger Pau Monné wrote: > On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: >> ... the same page with other registers which are not relevant to MSI-X. Xen >> marks pages where PBA resides as read-only. When assigning such devices to >> guest, device driver writes MSI-X irrelevant registers on those pages would >> lead to an EPT violation and the guest is destroyed because no handler is >> registered for those address range. In order to make guest capable to use >> such >> kind of devices, trapping very frequent write accesses is not a good idea for >> it would significantly impact the performance. >> >> This patch provides a workaround with caveat. Specifically, an option is >> introduced to specify a list of devices. For those devices, Xen doesn't >> control the access right to pages where PBA resides. Hence, guest device >> driver is able to write those pages and functions well. Note that adding an >> untrusted device to this option may endanger security of the entire system. >> >> Signed-off-by: Chao Gao>> --- >> docs/misc/xen-command-line.markdown | 10 + >> xen/arch/x86/msi.c | 7 -- >> xen/drivers/passthrough/pci.c | 45 >> +++-- >> xen/include/asm-x86/msi.h | 1 + >> 4 files changed, 59 insertions(+), 4 deletions(-) >> >> diff --git a/docs/misc/xen-command-line.markdown >> b/docs/misc/xen-command-line.markdown >> index b353352..e382513 100644 >> --- a/docs/misc/xen-command-line.markdown >> +++ b/docs/misc/xen-command-line.markdown >> @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. >> >> > Default: `on` >> >> +### pba\_quirk > > pba_write_allowed would be better, pba_quirk is too generic IMO. 'quirk' was I think requested by Jan; and my understanding is that the word clearly indicates that the behavior in question is a workaround for hardware which is not compliant with the appropriate specification. If you grep the source tree for 'quirk' you'll find a fairly large number. pba_shared_quirk might be slightly more descriptive. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: > ... the same page with other registers which are not relevant to MSI-X. Xen > marks pages where PBA resides as read-only. When assigning such devices to > guest, device driver writes MSI-X irrelevant registers on those pages would > lead to an EPT violation and the guest is destroyed because no handler is > registered for those address range. In order to make guest capable to use such > kind of devices, trapping very frequent write accesses is not a good idea for > it would significantly impact the performance. > > This patch provides a workaround with caveat. Specifically, an option is > introduced to specify a list of devices. For those devices, Xen doesn't > control the access right to pages where PBA resides. Hence, guest device > driver is able to write those pages and functions well. Note that adding an > untrusted device to this option may endanger security of the entire system. > > Signed-off-by: Chao Gao> --- > docs/misc/xen-command-line.markdown | 10 + > xen/arch/x86/msi.c | 7 -- > xen/drivers/passthrough/pci.c | 45 > +++-- > xen/include/asm-x86/msi.h | 1 + > 4 files changed, 59 insertions(+), 4 deletions(-) > > diff --git a/docs/misc/xen-command-line.markdown > b/docs/misc/xen-command-line.markdown > index b353352..e382513 100644 > --- a/docs/misc/xen-command-line.markdown > +++ b/docs/misc/xen-command-line.markdown > @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. > > > Default: `on` > > +### pba\_quirk pba_write_allowed would be better, pba_quirk is too generic IMO. > +> `= List of [:]:. > + > +Specify a list of SBDF of devices. When assigning devices in this list to > +guest, reading or writing the page where MSI-X PBA resides are allowed. > +This option provides a workaround for nonstandard PCI devices whose > +MSI-X PBA shares the same 4K-byte page with other registers. Note that > +adding an untrusted device to this option would undermine security of > +the entire system. > + > ### pci > > `= {no-}serr | {no-}perr` > > 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. > > @@ -1139,7 +1141,8 @@ static void _pci_cleanup_msix(struct arch_msix *msix) > if ( rangeset_remove_range(mmio_ro_ranges, msix->table.first, > msix->table.last) ) > WARN(); > -if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first, > +if ( !msix->pba_quirk_enabled && > + rangeset_remove_range(mmio_ro_ranges, msix->pba.first, > msix->pba.last) ) > WARN(); > } > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 1db69d5..cd765ef 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -184,6 +184,38 @@ static int __init parse_phantom_dev(const char *str) > } > custom_param("pci-phantom", parse_phantom_dev); > > +static struct pba_quirk_dev { > +uint32_t sbdf; > +} pba_quirk_devs[8]; We have a sbdf type now, see 514f58. Also, I would prefer that you use a list here. I know it's not likely to have a huge number of devices in the system that require this quirk, but I also see no reason to limit this to 8 (or any other arbitrary value). > +static unsigned int nr_pba_quirk_devs; > + > +static int __init parse_pba_quirk(const char *str) > +{ > +unsigned int seg, bus, dev, func; > + > +for ( ; ; ) > +{ > +if ( nr_pba_quirk_devs >= ARRAY_SIZE(pba_quirk_devs) ) > +return -E2BIG; > + > +str = parse_pci(str, , ,
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On Wed, Apr 04, 2018 at 04:45:32PM +0100, Roger Pau Monné wrote: >On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: >> ... the same page with other registers which are not relevant to MSI-X. Xen >> marks pages where PBA resides as read-only. When assigning such devices to >> guest, device driver writes MSI-X irrelevant registers on those pages would >> lead to an EPT violation and the guest is destroyed because no handler is >> registered for those address range. In order to make guest capable to use >> such >> kind of devices, trapping very frequent write accesses is not a good idea for >> it would significantly impact the performance. >> >> This patch provides a workaround with caveat. Specifically, an option is >> introduced to specify a list of devices. For those devices, Xen doesn't >> control the access right to pages where PBA resides. Hence, guest device >> driver is able to write those pages and functions well. Note that adding an >> untrusted device to this option may endanger security of the entire system. > >This is a clear violation of the MSI-X spec. Out of curiosity, which Yes, that's why we have this patch -- to workaround a hardware issue. >device is it that places random registers in the same page as the PBA? According to the commit [1], Mellanox MT27500 series, ConnectX-3 VF. And, a generation of Intel's Omni-Path. [1]:https://git.qemu.org/?p=qemu.git;a=commit;h=95239e162518dc6577164be3d9a789aba7f591a3 Could you help to give some comments? :). Thanks Chao ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
On Wed, Apr 04, 2018 at 11:29:39PM +0800, Chao Gao wrote: > ... the same page with other registers which are not relevant to MSI-X. Xen > marks pages where PBA resides as read-only. When assigning such devices to > guest, device driver writes MSI-X irrelevant registers on those pages would > lead to an EPT violation and the guest is destroyed because no handler is > registered for those address range. In order to make guest capable to use such > kind of devices, trapping very frequent write accesses is not a good idea for > it would significantly impact the performance. > > This patch provides a workaround with caveat. Specifically, an option is > introduced to specify a list of devices. For those devices, Xen doesn't > control the access right to pages where PBA resides. Hence, guest device > driver is able to write those pages and functions well. Note that adding an > untrusted device to this option may endanger security of the entire system. This is a clear violation of the MSI-X spec. Out of curiosity, which device is it that places random registers in the same page as the PBA? Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] pci: a workaround for nonstandard PCI devices whose PBA shares
... the same page with other registers which are not relevant to MSI-X. Xen marks pages where PBA resides as read-only. When assigning such devices to guest, device driver writes MSI-X irrelevant registers on those pages would lead to an EPT violation and the guest is destroyed because no handler is registered for those address range. In order to make guest capable to use such kind of devices, trapping very frequent write accesses is not a good idea for it would significantly impact the performance. This patch provides a workaround with caveat. Specifically, an option is introduced to specify a list of devices. For those devices, Xen doesn't control the access right to pages where PBA resides. Hence, guest device driver is able to write those pages and functions well. Note that adding an untrusted device to this option may endanger security of the entire system. Signed-off-by: Chao Gao--- docs/misc/xen-command-line.markdown | 10 + xen/arch/x86/msi.c | 7 -- xen/drivers/passthrough/pci.c | 45 +++-- xen/include/asm-x86/msi.h | 1 + 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index b353352..e382513 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors. > Default: `on` +### pba\_quirk +> `= List of [:]:. + +Specify a list of SBDF of devices. When assigning devices in this list to +guest, reading or writing the page where MSI-X PBA resides are allowed. +This option provides a workaround for nonstandard PCI devices whose +MSI-X PBA shares the same 4K-byte page with other registers. Note that +adding an untrusted device to this option would undermine security of +the entire system. + ### pci > `= {no-}serr | {no-}perr` 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(); @@ -1139,7 +1141,8 @@ static void _pci_cleanup_msix(struct arch_msix *msix) if ( rangeset_remove_range(mmio_ro_ranges, msix->table.first, msix->table.last) ) WARN(); -if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first, +if ( !msix->pba_quirk_enabled && + rangeset_remove_range(mmio_ro_ranges, msix->pba.first, msix->pba.last) ) WARN(); } diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 1db69d5..cd765ef 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -184,6 +184,38 @@ static int __init parse_phantom_dev(const char *str) } custom_param("pci-phantom", parse_phantom_dev); +static struct pba_quirk_dev { +uint32_t sbdf; +} pba_quirk_devs[8]; +static unsigned int nr_pba_quirk_devs; + +static int __init parse_pba_quirk(const char *str) +{ +unsigned int seg, bus, dev, func; + +for ( ; ; ) +{ +if ( nr_pba_quirk_devs >= ARRAY_SIZE(pba_quirk_devs) ) +return -E2BIG; + +str = parse_pci(str, , , , ); +if ( !str ) +return -EINVAL; + +pba_quirk_devs[nr_pba_quirk_devs++].sbdf = PCI_SBDF(seg, bus, dev, +func); +if ( *str == ',' ) +str++; +else if ( !*str ) +break; +else +return -EINVAL; +} + +return 0; +} +custom_param("pba_quirk", parse_pba_quirk); + static u16 __read_mostly command_mask; static u16 __read_mostly bridge_ctl_mask; @@ -300,6 +332,7 @@ static void check_pdev(const struct pci_dev *pdev) static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) { +unsigned int i; struct pci_dev *pdev; list_for_each_entry ( pdev, >alldevs_list, alldevs_list ) @@ -328,6 +361,16 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) } spin_lock_init(>table_lock); pdev->msix = msix; + +for ( i = 0; i < nr_pba_quirk_devs; i++ ) +{ +if ( pba_quirk_devs[i].sbdf == PCI_SBDF3(pseg->nr, bus, devfn) ) +{ +pdev->msix->pba_quirk_enabled = true; +printk(XENLOG_WARNING "Enable PBA quirk for %04x:%02x:%02x.%u\n", + pseg->nr, bus, PCI_SLOT(devfn),