Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign
@ 2019-10-09  8:33 Roger Pau Monne
  2019-10-09  9:49 ` Jan Beulich
  2019-10-09 11:13 ` Chao Gao
  0 siblings, 2 replies; 5+ messages in thread
From: Roger Pau Monne @ 2019-10-09  8:33 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Andrew Cooper, Spassov, Stanislav, Jan Beulich,
	Roger Pau Monne, Chao Gao

The current implementation of host_maskall makes it sticky across
assign and deassign calls, which means that once a guest forces Xen to
set host_maskall the maskall bit is not going to be cleared until a
call to PHYSDEVOP_prepare_msix is performed. Such call however
shouldn't be part of the normal flow when doing PCI passthrough, and
hence the flag needs to be cleared when assigning in order to prevent
host_maskall being carried over from previous assignations.

Note that the entry maskbit is reset when the msix capability is
initialized, and the guest_maskall field is also cleared so that the
hardware value matches Xen's internal state (hardware maskall =
host_maskall | guest_maskall).

Also note that doing the reset of host_maskall there would allow the
guest to reset such field by enabling and disabling MSIX, which is not
intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Chao Gao <chao.gao@intel.com>
Cc: "Spassov, Stanislav" <stanspas@amazon.de>
Cc: Pasi Kärkkäinen <pasik@iki.fi>
---
Chao, Stanislav, can you please check if this patch fixes your
issues?
---
Changes since v1:
 - Also set guest_maskall.
 - Place the code in a helper function in x86/msi.c
 - Add a comment to describe the expected state.
 - Test that maskall is not set on hardware.
---
 xen/arch/x86/msi.c            | 22 ++++++++++++++++++++++
 xen/drivers/passthrough/pci.c |  5 +++++
 xen/include/asm-x86/msi.h     |  1 +
 3 files changed, 28 insertions(+)

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 76d4034c4f..c239a00fb1 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1249,6 +1249,28 @@ void pci_cleanup_msi(struct pci_dev *pdev)
     msi_free_irqs(pdev);
 }
 
+int pci_reset_msix_state(struct pci_dev *pdev)
+{
+    unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus, pdev->sbdf.dev,
+                                           pdev->sbdf.fn, PCI_CAP_ID_MSIX);
+
+    ASSERT(pos);
+    /*
+     * Xen expects the device state to be the after reset one, and hence
+     * host_maskall = guest_maskall = false and all entries should have the
+     * mask bit set. Test that the maskall bit is not set, having it set could
+     * signal that the device hasn't been reset properly.
+     */
+    if ( pci_conf_read16(pdev->sbdf, msix_control_reg(pos)) &
+         PCI_MSIX_FLAGS_MASKALL )
+        return -EBUSY;
+
+    pdev->msix->host_maskall = false;
+    pdev->msix->guest_maskall = false;
+
+    return 0;
+}
+
 int pci_msi_conf_write_intercept(struct pci_dev *pdev, unsigned int reg,
                                  unsigned int size, uint32_t *data)
 {
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 90ccb8370b..bdcc482d81 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1505,7 +1505,12 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
     }
 
     if ( pdev->msix )
+    {
+        rc = pci_reset_msix_state(pdev);
+        if ( rc )
+            goto done;
         msixtbl_init(d);
+    }
 
     pdev->fault.count = 0;
 
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index d0b0045d0d..6e35713ec7 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -92,6 +92,7 @@ extern int __setup_msi_irq(struct irq_desc *, struct msi_desc *,
 extern void teardown_msi_irq(int irq);
 extern int msi_free_vector(struct msi_desc *entry);
 extern int pci_restore_msi_state(struct pci_dev *pdev);
+extern int pci_reset_msix_state(struct pci_dev *pdev);
 
 struct msi_desc {
 	struct msi_attrib {
-- 
2.23.0


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign
  2019-10-09  8:33 [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign Roger Pau Monne
@ 2019-10-09  9:49 ` Jan Beulich
  2019-10-10  8:47   ` Jürgen Groß
  2019-10-09 11:13 ` Chao Gao
  1 sibling, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2019-10-09  9:49 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: Juergen Gross, Wei Liu, Andrew Cooper, Stanislav Spassov,
	xen-devel, Chao Gao

On 09.10.2019 10:33, Roger Pau Monne wrote:
> The current implementation of host_maskall makes it sticky across
> assign and deassign calls, which means that once a guest forces Xen to
> set host_maskall the maskall bit is not going to be cleared until a
> call to PHYSDEVOP_prepare_msix is performed. Such call however
> shouldn't be part of the normal flow when doing PCI passthrough, and
> hence the flag needs to be cleared when assigning in order to prevent
> host_maskall being carried over from previous assignations.
> 
> Note that the entry maskbit is reset when the msix capability is
> initialized, and the guest_maskall field is also cleared so that the
> hardware value matches Xen's internal state (hardware maskall =
> host_maskall | guest_maskall).
> 
> Also note that doing the reset of host_maskall there would allow the
> guest to reset such field by enabling and disabling MSIX, which is not
> intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm also Cc-ing Jürgen for a possible release ack for 4.13, but
I'd also like to point out that I'd prefer to wait a little with
committing this to get at least one Tested-by.

Jan

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign
  2019-10-09  8:33 [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign Roger Pau Monne
  2019-10-09  9:49 ` Jan Beulich
@ 2019-10-09 11:13 ` Chao Gao
  2019-10-10 10:23   ` Wei Liu
  1 sibling, 1 reply; 5+ messages in thread
From: Chao Gao @ 2019-10-09 11:13 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: Wei Liu, Andrew Cooper, Spassov, Stanislav, Jan Beulich, xen-devel

On Wed, Oct 09, 2019 at 10:33:21AM +0200, Roger Pau Monne wrote:
>The current implementation of host_maskall makes it sticky across
>assign and deassign calls, which means that once a guest forces Xen to
>set host_maskall the maskall bit is not going to be cleared until a
>call to PHYSDEVOP_prepare_msix is performed. Such call however
>shouldn't be part of the normal flow when doing PCI passthrough, and
>hence the flag needs to be cleared when assigning in order to prevent
>host_maskall being carried over from previous assignations.
>
>Note that the entry maskbit is reset when the msix capability is
>initialized, and the guest_maskall field is also cleared so that the
>hardware value matches Xen's internal state (hardware maskall =
>host_maskall | guest_maskall).
>
>Also note that doing the reset of host_maskall there would allow the
>guest to reset such field by enabling and disabling MSIX, which is not
>intended.
>
>Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>---
>Cc: Chao Gao <chao.gao@intel.com>
>Cc: "Spassov, Stanislav" <stanspas@amazon.de>
>Cc: Pasi Kärkkäinen <pasik@iki.fi>
>---
>Chao, Stanislav, can you please check if this patch fixes your
>issues?

Tested-by: Chao Gao <chao.gao@intel.com>

I got the assertion failure below when starting xencommons with the
newest staging:

Setting domain 0 name, domid and JSON config...
xen-init-dom0: _libxl_types.c:2163: libxl_domain_build_info_init_type: Assertion `p->type == LIBXL_DOMAIN_TYPE_INVALID' failed.
/etc/init.d/xencommons: line 54:  2006 Aborted                 (core dumped) ${LIBEXEC_BIN}/xen-init-dom0 ${XEN_DOM0_UUID}

It should be irrelated to this patch. So I apply this patch on
cd93953538aac and it works.

Thanks
Chao

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign
  2019-10-09  9:49 ` Jan Beulich
@ 2019-10-10  8:47   ` Jürgen Groß
  0 siblings, 0 replies; 5+ messages in thread
From: Jürgen Groß @ 2019-10-10  8:47 UTC (permalink / raw)
  To: Jan Beulich, Roger Pau Monne
  Cc: Wei Liu, Andrew Cooper, Stanislav Spassov, xen-devel, Chao Gao

On 09.10.19 11:49, Jan Beulich wrote:
> On 09.10.2019 10:33, Roger Pau Monne wrote:
>> The current implementation of host_maskall makes it sticky across
>> assign and deassign calls, which means that once a guest forces Xen to
>> set host_maskall the maskall bit is not going to be cleared until a
>> call to PHYSDEVOP_prepare_msix is performed. Such call however
>> shouldn't be part of the normal flow when doing PCI passthrough, and
>> hence the flag needs to be cleared when assigning in order to prevent
>> host_maskall being carried over from previous assignations.
>>
>> Note that the entry maskbit is reset when the msix capability is
>> initialized, and the guest_maskall field is also cleared so that the
>> hardware value matches Xen's internal state (hardware maskall =
>> host_maskall | guest_maskall).
>>
>> Also note that doing the reset of host_maskall there would allow the
>> guest to reset such field by enabling and disabling MSIX, which is not
>> intended.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm also Cc-ing Jürgen for a possible release ack for 4.13, but
> I'd also like to point out that I'd prefer to wait a little with
> committing this to get at least one Tested-by.

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign
  2019-10-09 11:13 ` Chao Gao
@ 2019-10-10 10:23   ` Wei Liu
  0 siblings, 0 replies; 5+ messages in thread
From: Wei Liu @ 2019-10-10 10:23 UTC (permalink / raw)
  To: Chao Gao
  Cc: Wei Liu, Andrew Cooper, Spassov, Stanislav, Jan Beulich,
	xen-devel, Roger Pau Monne

On Wed, Oct 09, 2019 at 07:13:45PM +0800, Chao Gao wrote:
> On Wed, Oct 09, 2019 at 10:33:21AM +0200, Roger Pau Monne wrote:
> >The current implementation of host_maskall makes it sticky across
> >assign and deassign calls, which means that once a guest forces Xen to
> >set host_maskall the maskall bit is not going to be cleared until a
> >call to PHYSDEVOP_prepare_msix is performed. Such call however
> >shouldn't be part of the normal flow when doing PCI passthrough, and
> >hence the flag needs to be cleared when assigning in order to prevent
> >host_maskall being carried over from previous assignations.
> >
> >Note that the entry maskbit is reset when the msix capability is
> >initialized, and the guest_maskall field is also cleared so that the
> >hardware value matches Xen's internal state (hardware maskall =
> >host_maskall | guest_maskall).
> >
> >Also note that doing the reset of host_maskall there would allow the
> >guest to reset such field by enabling and disabling MSIX, which is not
> >intended.
> >
> >Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >---
> >Cc: Chao Gao <chao.gao@intel.com>
> >Cc: "Spassov, Stanislav" <stanspas@amazon.de>
> >Cc: Pasi Kärkkäinen <pasik@iki.fi>
> >---
> >Chao, Stanislav, can you please check if this patch fixes your
> >issues?
> 
> Tested-by: Chao Gao <chao.gao@intel.com>
> 
> I got the assertion failure below when starting xencommons with the
> newest staging:
> 
> Setting domain 0 name, domid and JSON config...
> xen-init-dom0: _libxl_types.c:2163: libxl_domain_build_info_init_type: Assertion `p->type == LIBXL_DOMAIN_TYPE_INVALID' failed.
> /etc/init.d/xencommons: line 54:  2006 Aborted                 (core dumped) ${LIBEXEC_BIN}/xen-init-dom0 ${XEN_DOM0_UUID}

What is your setup like?

Did you perhaps have some stale libraries?

Wei.

> 
> It should be irrelated to this patch. So I apply this patch on
> cd93953538aac and it works.
> 
> Thanks
> Chao

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09  8:33 [Xen-devel] [PATCH v2] pci: clear {host/guest}_maskall field on assign Roger Pau Monne
2019-10-09  9:49 ` Jan Beulich
2019-10-10  8:47   ` Jürgen Groß
2019-10-09 11:13 ` Chao Gao
2019-10-10 10:23   ` Wei Liu

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox