Support xen-driven USB3 debug capability

PATCH In review Connor Davis 5 participants 14 messages May 14, 2021 – May 18, 2021

Subsystems (3)

USB Xen hypervisor interface USB xHCI driver

lore

Share & tools

lore

Connor Davis

author

2021-05-14 00:57 UTC #0 lore raw

v2 cover Support xen-driven USB3 debug capability

Hi all,

This goal of this series is to allow the USB3 debug capability (DbC) to be safely used by xen while linux runs as dom0.

The first patch prevents the early DbC driver from using an already-running DbC.

The second exports xen_dbgp_reset_prep and xen_dbgp_external_startup functions when CONFIG_XEN_DOM0 is enabled so they may be used by the xHCI driver.

The third ensures that xen_dbgp_reset_prep/xen_dbgp_external_startup return consistent values in failure cases. This inconsistency illustrated another issue: dbgp_reset_prep returned the value of xen_dbgp_reset_prep if it was nonzero, but callers of dbgp_reset_prep interpret nonzero as "keep using the debug port" and would eventually (needlessly) call dbgp_external_startup. Patch three _should_ fix this issue, but I don't have any EHCI hardware available to test unfortunately.

The last uses the xen_dbgp_* functions to notify xen of unsafe periods (e.g. reset and D3hot transition).

Thanks,
Connor

--
Changes since v1:
- Added patch for dbgp return value fixes
- Return -EPERM when !xen_initial_domain() in xen_dbgp_op
- Moved #ifdef-ary out of xhci.c into xhci-dbgcap.h

--
Connor Davis (4):
usb: early: Avoid using DbC if already enabled
xen: Export dbgp functions when CONFIG_XEN_DOM0 is enabled
usb: dbgp: Fix return values for reset prep and startup
usb: xhci: Notify xen when DbC is unsafe to use

 drivers/usb/early/ehci-dbgp.c  |  9 ++++---
 drivers/usb/early/xhci-dbc.c   | 10 ++++++++
 drivers/usb/host/xhci-dbgcap.h | 19 ++++++++++++++
 drivers/usb/host/xhci.c        | 47 ++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h        |  1 +
 drivers/xen/dbgp.c             |  4 +--
 include/linux/usb/ehci-dbgp.h  | 14 ++++++----
 7 files changed, 94 insertions(+), 10 deletions(-)

base-commit: 88b06399c9c766c283e070b022b5ceafa4f63f19

Signature · 1 line

2.31.1

Connor Davis

author

2021-05-14 00:57 UTC #1 lore raw

PATCH v2 1/4 usb: early: Avoid using DbC if already enabled

Check if the debug capability is enabled in early_xdbc_parse_parameter, and if it is, return with an error. This avoids collisions with whatever enabled the DbC prior to linux starting.

Signed-off-by Connor Davis <[email protected]>

---
 drivers/usb/early/xhci-dbc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/usb/early/xhci-dbc.c +10 -0

index be4ecbabdd58..ca67fddc2d36 100644
@@ -642,6 +642,16 @@ int __init early_xdbc_parse_parameter(char *s)
 	}
 	xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
 
+	if (readl(&xdbc.xdbc_reg->control) & CTRL_DBC_ENABLE) {
+		pr_notice("xhci debug capability already in use\n");
+		early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+		xdbc.xdbc_reg = NULL;
+		xdbc.xhci_base = NULL;
+		xdbc.xhci_length = 0;
+
+		return -ENODEV;
+	}
+
 	return 0;
 }

Signature · 1 line

2.31.1

Connor Davis

author

2021-05-14 00:57 UTC #4 lore raw

PATCH v2 3/4 usb: dbgp: Fix return values for reset prep and startup

Callers of dbgp_reset_prep treat a 0 return value as "stop using the debug port", which means they don't make any subsequent calls to dbgp_reset_prep or dbgp_external_startup.

To ensure the callers' interpretation is correct, first return -EPERM from xen_dbgp_op if !xen_initial_domain(). This ensures that both xen_dbgp_reset_prep and xen_dbgp_external_startup return 0 iff the PHYSDEVOP_DBGP_RESET_{PREPARE,DONE} hypercalls succeed. Also update xen_dbgp_reset_prep and xen_dbgp_external_startup to return -EPERM when !CONFIG_XEN_DOM0 for consistency.

Next, return nonzero from dbgp_reset_prep if xen_dbgp_reset_prep returns
0. The nonzero value ensures that callers of dbgp_reset_prep will subsequently call dbgp_external_startup when it is safe to do so.

Also invert the return values from dbgp_external_startup for consistency with dbgp_reset_prep (this inversion has no functional change since no callers actually check the value).

Signed-off-by Connor Davis <[email protected]>

---
 drivers/usb/early/ehci-dbgp.c |  9 ++++++---
 drivers/xen/dbgp.c            |  2 +-
 include/linux/usb/ehci-dbgp.h | 14 +++++++++-----
 3 files changed, 16 insertions(+), 9 deletions(-)

drivers/usb/early/ehci-dbgp.c +6 -3

index 45b42d8f6453..ff993d330c01 100644
@@ -970,8 +970,8 @@ int dbgp_reset_prep(struct usb_hcd *hcd)
 	int ret = xen_dbgp_reset_prep(hcd);
 	u32 ctrl;
 
-	if (ret)
-		return ret;
+	if (!ret)
+		return 1;
 
 	dbgp_not_safe = 1;
 	if (!ehci_debug)

@@ -995,7 +995,10 @@ EXPORT_SYMBOL_GPL(dbgp_reset_prep);
 
 int dbgp_external_startup(struct usb_hcd *hcd)
 {
-	return xen_dbgp_external_startup(hcd) ?: _dbgp_external_startup();
+	if (!xen_dbgp_external_startup(hcd))
+		return 1;
+
+	return !_dbgp_external_startup();
 }
 EXPORT_SYMBOL_GPL(dbgp_external_startup);
 #endif /* USB */

drivers/xen/dbgp.c +1 -1

index fef32dd1a5dc..d54f98380e6f 100644
@@ -15,7 +15,7 @@ static int xen_dbgp_op(struct usb_hcd *hcd, int op)
 	struct physdev_dbgp_op dbgp;
 
 	if (!xen_initial_domain())
-		return 0;
+		return -EPERM;
 
 	dbgp.op = op;

include/linux/usb/ehci-dbgp.h +9 -5

index 62ab3805172d..c0e98557efc0 100644
@@ -56,28 +56,32 @@ extern int xen_dbgp_external_startup(struct usb_hcd *);
 #else
 static inline int xen_dbgp_reset_prep(struct usb_hcd *hcd)
 {
-	return 1; /* Shouldn't this be 0? */
+	return -EPERM;
 }
 
 static inline int xen_dbgp_external_startup(struct usb_hcd *hcd)
 {
-	return -1;
+	return -EPERM;
 }
 #endif
 
 #ifdef CONFIG_EARLY_PRINTK_DBGP
-/* Call backs from ehci host driver to ehci debug driver */
+/*
+ * Call backs from ehci host driver to ehci debug driver.
+ * Returns 0 if the debug port should stopped being used,
+ * nonzero otherwise.
+ */
 extern int dbgp_external_startup(struct usb_hcd *);
 extern int dbgp_reset_prep(struct usb_hcd *);
 #else
 static inline int dbgp_reset_prep(struct usb_hcd *hcd)
 {
-	return xen_dbgp_reset_prep(hcd);
+	return !xen_dbgp_reset_prep(hcd);
 }
 
 static inline int dbgp_external_startup(struct usb_hcd *hcd)
 {
-	return xen_dbgp_external_startup(hcd);
+	return !xen_dbgp_external_startup(hcd);
 }
 #endif

Signature · 1 line

2.31.1

Connor Davis

author

2021-05-14 00:57 UTC #2 lore raw

PATCH v2 2/4 xen: Export dbgp functions when CONFIG_XEN_DOM0 is enabled

In reply to Connor Davis

Export xen_dbgp_reset_prep and xen_dbgp_external_startup when CONFIG_XEN_DOM0 is defined. This allows use of these symbols even if CONFIG_EARLY_PRINK_DBGP is defined.

Signed-off-by Connor Davis <[email protected]>

Acked-by Juergen Gross <[email protected]>

---
 drivers/xen/dbgp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/xen/dbgp.c +1 -1

index cfb5de31d860..fef32dd1a5dc 100644
@@ -44,7 +44,7 @@ int xen_dbgp_external_startup(struct usb_hcd *hcd)
 	return xen_dbgp_op(hcd, PHYSDEVOP_DBGP_RESET_DONE);
 }
 
-#ifndef CONFIG_EARLY_PRINTK_DBGP
+#if defined(CONFIG_XEN_DOM0) || !defined(CONFIG_EARLY_PRINTK_DBGP)
 #include <linux/export.h>
 EXPORT_SYMBOL_GPL(xen_dbgp_reset_prep);
 EXPORT_SYMBOL_GPL(xen_dbgp_external_startup);

Signature · 1 line

2.31.1

Connor Davis

author

2021-05-14 00:57 UTC #6 lore raw

PATCH v2 4/4 usb: xhci: Notify xen when DbC is unsafe to use

In reply to Connor Davis

When running as a dom0 guest on Xen, check if the USB3 debug capability is enabled before xHCI reset, suspend, and resume. If it is, call xen_dbgp_reset_prep() to notify Xen that it is unsafe to touch MMIO registers until the next xen_dbgp_external_startup().

This notification allows Xen to avoid undefined behavior resulting from MMIO access when the host controller's CNR bit is set or when the device transitions to D3hot.

Signed-off-by Connor Davis <[email protected]>

---
 drivers/usb/host/xhci-dbgcap.h | 19 ++++++++++++++
 drivers/usb/host/xhci.c        | 47 ++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h        |  1 +
 3 files changed, 67 insertions(+)

drivers/usb/host/xhci-dbgcap.h +19 -0

index c70b78d504eb..332be531bebd 100644
@@ -227,4 +227,23 @@ static inline int xhci_dbc_resume(struct xhci_hcd *xhci)
 	return 0;
 }
 #endif /* CONFIG_USB_XHCI_DBGCAP */
+
+#ifdef CONFIG_XEN_DOM0
+#include <xen/xen.h>
+
+int xen_dbgp_reset_prep(struct usb_hcd *hcd);
+int xen_dbgp_external_startup(struct usb_hcd *hcd);
+
+void xhci_dbc_external_reset_prep(struct xhci_hcd *xhci);
+void xhci_dbc_external_reset_done(struct xhci_hcd *xhci);
+#else
+static inline void xhci_dbc_external_reset_prep(struct xhci_hcd *xhci)
+{
+}
+
+static inline void xhci_dbc_external_reset_done(struct xhci_hcd *xhci)
+{
+}
+#endif /* CONFIG_XEN_DOM0 */
+
 #endif /* __LINUX_XHCI_DBGCAP_H */

drivers/usb/host/xhci.c +47 -0

77 lines not on this page · raw message

drivers/usb/host/xhci.h +1 -0

index 2595a8f057c4..61d8efc9eef2 100644
@@ -1920,6 +1920,7 @@ struct xhci_hcd {
 	struct list_head	regset_list;
 
 	void			*dbc;
+	int			external_dbc;
 	/* platform-specific data -- must come last */
 	unsigned long		priv[] __aligned(sizeof(s64));
 };

Signature · 1 line

2.31.1

Greg Kroah-Hartman

maintainer

2021-05-14 06:46 UTC #12 lore raw

In reply to Connor Davis

Quoted 4 lines · Connor Davis

Hi all,
This goal of this series is to allow the USB3 debug capability (DbC) to be safely used by xen while linux runs as dom0.

Patch 2/4 does not seem to be showing up anywhere, did it get lost?

thanks,

greg k-h

Connor Davis

author

2021-05-14 14:06 UTC #3 lore raw

Re: [PATCH v2 2/4] xen: Export dbgp functions when CONFIG_XEN_DOM0 is enabled

In reply to Connor Davis

Adding Greg and linux-usb

Quoted 23 lines · Connor Davis

Export xen_dbgp_reset_prep and xen_dbgp_external_startup when CONFIG_XEN_DOM0 is defined. This allows use of these symbols even if CONFIG_EARLY_PRINK_DBGP is defined.
Signed-off-by Connor Davis <[email protected]>
Acked-by Juergen Gross <[email protected]>
---
  drivers/xen/dbgp.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/xen/dbgp.c +1 -1
index cfb5de31d860..fef32dd1a5dc 100644
@@ -44,7 +44,7 @@ int xen_dbgp_external_startup(struct usb_hcd *hcd)
  	return xen_dbgp_op(hcd, PHYSDEVOP_DBGP_RESET_DONE);
  }
 
-#ifndef CONFIG_EARLY_PRINTK_DBGP
+#if defined(CONFIG_XEN_DOM0) || !defined(CONFIG_EARLY_PRINTK_DBGP)
  #include <linux/export.h>
  EXPORT_SYMBOL_GPL(xen_dbgp_reset_prep);
  EXPORT_SYMBOL_GPL(xen_dbgp_external_startup);

Connor Davis

author

2021-05-14 14:07 UTC #13 lore raw

In reply to Greg Kroah-Hartman

Quoted 6 lines · Greg Kroah-Hartman

Quoted 4 lines · Connor Davis
Hi all,
This goal of this series is to allow the USB3 debug capability (DbC) to be safely used by xen while linux runs as dom0.
Patch 2/4 does not seem to be showing up anywhere, did it get lost?

Yep, just added you, sorry about that

Thanks,

Connor

Quoted 3 lines

thanks,
greg k-h

Boris Ostrovsky

Reviewed-by on v2 3/4

2021-05-14 15:34 UTC #5 lore raw

Re: [PATCH v2 3/4] usb: dbgp: Fix return values for reset prep and startup

In reply to Connor Davis

Quoted 15 lines · Connor Davis

Callers of dbgp_reset_prep treat a 0 return value as "stop using the debug port", which means they don't make any subsequent calls to dbgp_reset_prep or dbgp_external_startup.
To ensure the callers' interpretation is correct, first return -EPERM from xen_dbgp_op if !xen_initial_domain(). This ensures that both xen_dbgp_reset_prep and xen_dbgp_external_startup return 0 iff the PHYSDEVOP_DBGP_RESET_{PREPARE,DONE} hypercalls succeed. Also update xen_dbgp_reset_prep and xen_dbgp_external_startup to return -EPERM when !CONFIG_XEN_DOM0 for consistency.
Next, return nonzero from dbgp_reset_prep if xen_dbgp_reset_prep returns
0. The nonzero value ensures that callers of dbgp_reset_prep will subsequently call dbgp_external_startup when it is safe to do so.

Quoted 5 lines

Also invert the return values from dbgp_external_startup for consistency with dbgp_reset_prep (this inversion has no functional change since no callers actually check the value).
Signed-off-by Connor Davis <[email protected]>

For Xen bits:

Reviewed-by Boris Ostrovsky <[email protected]>

For the rest it seems to me that error code passing could be improved: if it's 0 or 1 it should be bool. Or pass actual error code, with zero for no-error case, such as ...

Quoted 11 lines

---
 drivers/usb/early/ehci-dbgp.c |  9 ++++++---
 drivers/xen/dbgp.c            |  2 +-
 include/linux/usb/ehci-dbgp.h | 14 +++++++++-----
 3 files changed, 16 insertions(+), 9 deletions(-)

drivers/usb/early/ehci-dbgp.c +0 -0

index 45b42d8f6453..ff993d330c01 100644

drivers/usb/early/ehci-dbgp.c int dbgp_reset_prep(struct usb_hcd *hcd) · line 974

Quoted 7 lines

drivers/usb/early/ehci-dbgp.c +2 -2

@@ -970,8 +970,8 @@ int dbgp_reset_prep(struct usb_hcd *hcd)
 	int ret = xen_dbgp_reset_prep(hcd);
 	u32 ctrl;
 
-	if (ret)
-		return ret;
+	if (!ret)
+		return 1;

... here or ...

drivers/usb/early/ehci-dbgp.c EXPORT_SYMBOL_GPL(dbgp_reset_prep); · line 999

Quoted 10 lines

drivers/usb/early/ehci-dbgp.c +2 -1

 	dbgp_not_safe = 1;
 	if (!ehci_debug)

@@ -995,7 +995,10 @@ EXPORT_SYMBOL_GPL(dbgp_reset_prep);
 
 int dbgp_external_startup(struct usb_hcd *hcd)
 {
-	return xen_dbgp_external_startup(hcd) ?: _dbgp_external_startup();
+	if (!xen_dbgp_external_startup(hcd))
+		return 1;

... here.

-boris

Jan Beulich

2021-05-17 09:31 UTC #7 lore raw

Re: [PATCH v2 1/4] usb: early: Avoid using DbC if already enabled

In reply to Connor Davis

Quoted 3 lines · Connor Davis

Check if the debug capability is enabled in early_xdbc_parse_parameter, and if it is, return with an error. This avoids collisions with whatever enabled the DbC prior to linux starting.

Doesn't this go too far and prevent use even if firmware (perhaps mistakenly) left it enabled?

Jan

Quoted 27 lines

Signed-off-by Connor Davis <[email protected]>

---
 drivers/usb/early/xhci-dbc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/usb/early/xhci-dbc.c +10 -0

index be4ecbabdd58..ca67fddc2d36 100644
@@ -642,6 +642,16 @@ int __init early_xdbc_parse_parameter(char *s)
 	}
 	xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
 
+	if (readl(&xdbc.xdbc_reg->control) & CTRL_DBC_ENABLE) {
+		pr_notice("xhci debug capability already in use\n");
+		early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+		xdbc.xdbc_reg = NULL;
+		xdbc.xhci_base = NULL;
+		xdbc.xhci_length = 0;
+
+		return -ENODEV;
+	}
+
 	return 0;
 }

Connor Davis

author

2021-05-17 13:48 UTC #8 lore raw

Re: [PATCH v2 1/4] usb: early: Avoid using DbC if already enabled

Quoted 8 lines · Jan Beulich

Quoted 3 lines · Connor Davis
Check if the debug capability is enabled in early_xdbc_parse_parameter, and if it is, return with an error. This avoids collisions with whatever enabled the DbC prior to linux starting.
Doesn't this go too far and prevent use even if firmware (perhaps mistakenly) left it enabled?
Jan

Yes, but how is one supposed to distinguish the broken firmware and non-broken

firmware cases?

Quoted 14 lines

Signed-off-by Connor Davis <[email protected]>

---
  drivers/usb/early/xhci-dbc.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

drivers/usb/early/xhci-dbc.c +0 -0

index be4ecbabdd58..ca67fddc2d36 100644
@@ -642,6 +642,16 @@ int __init early_xdbc_parse_parameter(char *s)
  	}
  	xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);

Quoted 14 lines

Quoted 4 lines
drivers/usb/early/xhci-dbc.c +3 -0
+	if (readl(&xdbc.xdbc_reg->control) & CTRL_DBC_ENABLE) {
+		pr_notice("xhci debug capability already in use\n");
+		early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
drivers/usb/early/xhci-dbc.c int __init early_xdbc_parse_parameter(char *s) · line 657
Quoted 10 lines
drivers/usb/early/xhci-dbc.c +7 -0
+		xdbc.xdbc_reg = NULL;
+		xdbc.xhci_base = NULL;
+		xdbc.xhci_length = 0;
+
+		return -ENODEV;
+	}
+
  	return 0;
  }
 

Thanks,

Connor

Jan Beulich

2021-05-17 14:14 UTC #9 lore raw

Re: [PATCH v2 1/4] usb: early: Avoid using DbC if already enabled

Quoted 6 lines · Connor Davis

Quoted 4 lines · Jan Beulich
Quoted 3 lines · Connor Davis
Check if the debug capability is enabled in early_xdbc_parse_parameter, and if it is, return with an error. This avoids collisions with whatever enabled the DbC prior to linux starting.

Quoted 7 lines

Quoted 2 lines
Doesn't this go too far and prevent use even if firmware (perhaps mistakenly) left it enabled?
Yes, but how is one supposed to distinguish the broken firmware and non-broken
firmware cases?

Well, a first step might be to only check if running virtualized. And then if your running virtualized, there might be a way to inquire the hypervisor?

Jan

Connor Davis

author

2021-05-17 14:24 UTC #10 lore raw

Re: [PATCH v2 1/4] usb: early: Avoid using DbC if already enabled

Quoted 8 lines · Jan Beulich

Quoted 7 lines · Connor Davis
Quoted 6 lines · Jan Beulich
Quoted 3 lines · Connor Davis
Check if the debug capability is enabled in early_xdbc_parse_parameter, and if it is, return with an error. This avoids collisions with whatever enabled the DbC prior to linux starting.
Doesn't this go too far and prevent use even if firmware (perhaps mistakenly) left it enabled?

Quoted 7 lines

Quoted 4 lines
Yes, but how is one supposed to distinguish the broken firmware and non-broken
firmware cases?
Well, a first step might be to only check if running virtualized. And then if your running virtualized, there might be a way to inquire the hypervisor?

Right, but if it was enabled by something other than a hypervisor,

or you're not running virtualized, how do you distinguish then? IMO

the proper thing to do in any case is to simply not use the DbC in linux.

Thanks,

Connor

Mathias Nyman

2021-05-18 21:48 UTC #11 lore raw

Re: [PATCH v2 1/4] usb: early: Avoid using DbC if already enabled

Quoted 18 lines · Connor Davis

Quoted 8 lines · Jan Beulich
Quoted 7 lines · Connor Davis
Quoted 6 lines · Jan Beulich
Quoted 3 lines · Connor Davis
Check if the debug capability is enabled in early_xdbc_parse_parameter, and if it is, return with an error. This avoids collisions with whatever enabled the DbC prior to linux starting.
Doesn't this go too far and prevent use even if firmware (perhaps mistakenly) left it enabled?
Quoted 7 lines
Quoted 4 lines
Yes, but how is one supposed to distinguish the broken firmware and non-broken
firmware cases?
Well, a first step might be to only check if running virtualized. And then if your running virtualized, there might be a way to inquire the hypervisor?

Quoted 6 lines

Right, but if it was enabled by something other than a hypervisor,
or you're not running virtualized, how do you distinguish then? IMO
the proper thing to do in any case is to simply not use the DbC in linux.

Sounds reasonable.

We can address "broken firmware" during the xHC handoff from BIOS to OS. Only first OS that loads after BIOS should see the
"HC BIOS owned semaphore" bit set in xHCI MMIO.

If it's set then OS requests ownership, which clears BIOS owned bit. If DbC is running here we can stop it.

-Mathias