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(+)
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index be4ecbabdd58..ca67fddc2d36 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -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;
}
--
2.31.1
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(-)
diff --git a/drivers/xen/dbgp.c b/drivers/xen/dbgp.c
index cfb5de31d860..fef32dd1a5dc 100644
--- a/drivers/xen/dbgp.c
+++ b/drivers/xen/dbgp.c
@@ -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);
--
2.31.1
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(-)
diff --git a/drivers/usb/early/ehci-dbgp.c b/drivers/usb/early/ehci-dbgp.c
index 45b42d8f6453..ff993d330c01 100644
--- a/drivers/usb/early/ehci-dbgp.c
+++ b/drivers/usb/early/ehci-dbgp.c
@@ -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 */
diff --git a/drivers/xen/dbgp.c b/drivers/xen/dbgp.c
index fef32dd1a5dc..d54f98380e6f 100644
--- a/drivers/xen/dbgp.c
+++ b/drivers/xen/dbgp.c
@@ -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;
diff --git a/include/linux/usb/ehci-dbgp.h b/include/linux/usb/ehci-dbgp.h
index 62ab3805172d..c0e98557efc0 100644
--- a/include/linux/usb/ehci-dbgp.h
+++ b/include/linux/usb/ehci-dbgp.h
@@ -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
--
2.31.1
Adding Greg and linux-usb
On 5/13/21 6:56 PM, Connor Davis wrote:
> 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(-)
>
> diff --git a/drivers/xen/dbgp.c b/drivers/xen/dbgp.c
> index cfb5de31d860..fef32dd1a5dc 100644
> --- a/drivers/xen/dbgp.c
> +++ b/drivers/xen/dbgp.c
> @@ -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);
On 5/13/21 8:56 PM, Connor Davis wrote:
> 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]>
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 ...
> ---
> 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(-)
>
> diff --git a/drivers/usb/early/ehci-dbgp.c b/drivers/usb/early/ehci-dbgp.c
> index 45b42d8f6453..ff993d330c01 100644
> --- a/drivers/usb/early/ehci-dbgp.c
> +++ b/drivers/usb/early/ehci-dbgp.c
> @@ -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 ...
>
> 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
On 14.05.2021 02:56, Connor Davis wrote:
> 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
> Signed-off-by: Connor Davis <[email protected]>
> ---
> drivers/usb/early/xhci-dbc.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
> index be4ecbabdd58..ca67fddc2d36 100644
> --- a/drivers/usb/early/xhci-dbc.c
> +++ b/drivers/usb/early/xhci-dbc.c
> @@ -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;
> }
>
>
On 5/17/21 3:32 AM, Jan Beulich wrote:
> On 14.05.2021 02:56, Connor Davis wrote:
>> 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?
>
>> Signed-off-by: Connor Davis <[email protected]>
>> ---
>> drivers/usb/early/xhci-dbc.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>> index be4ecbabdd58..ca67fddc2d36 100644
>> --- a/drivers/usb/early/xhci-dbc.c
>> +++ b/drivers/usb/early/xhci-dbc.c
>> @@ -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;
>> }
>>
>>
Thanks,
Connor
On 17.05.2021 15:48, Connor Davis wrote:
>
> On 5/17/21 3:32 AM, Jan Beulich wrote:
>> On 14.05.2021 02:56, Connor Davis wrote:
>>> 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?
>
> 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
On 5/17/21 8:13 AM, Jan Beulich wrote:
> On 17.05.2021 15:48, Connor Davis wrote:
>> On 5/17/21 3:32 AM, Jan Beulich wrote:
>>> On 14.05.2021 02:56, Connor Davis wrote:
>>>> 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?
>> 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
On 17.5.2021 17.24, Connor Davis wrote:
>
> On 5/17/21 8:13 AM, Jan Beulich wrote:
>> On 17.05.2021 15:48, Connor Davis wrote:
>>> On 5/17/21 3:32 AM, Jan Beulich wrote:
>>>> On 14.05.2021 02:56, Connor Davis wrote:
>>>>> 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?
>>> 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.
>
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