diff mbox series

[RESEND] xhci: Fix Renesas PCIe controllers passthrough issue

Message ID 20221020103914.262202-1-wangwenmei168@163.com (mailing list archive)
State New, archived
Headers show
Series [RESEND] xhci: Fix Renesas PCIe controllers passthrough issue | expand

Commit Message

wangwenmei168@163.com Oct. 20, 2022, 10:39 a.m. UTC 
From: gehao <gehao@kylinos.cn>

When we use uPD720201 USB 3.0 Host Controller passthrough to VM
guest os will report follow errors and it can not working.

xhci_hcd 0000:09:00.0: Host took too long to start, waited 16000
microseconds.
xhci_hcd 0000:09:00.0: startup error -19.

Because when we passthroug some device to our guest os,
dev->iommu_group =NULL,so it will return from this function,
Actually it still control by host os.
I think that this condition is not necessary.

For host os with IOMMU,it is safe.
For host os with noiommu,doing anything when there is no
iommu is definitely.
For guest os,the addresses we can access are restricted.

After add this path,they all work well.

Signed-off-by: gehao <gehao@kylinos.cn>
---
 drivers/usb/host/xhci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Greg KH Oct. 20, 2022, 11:11 a.m. UTC | #1 
On Thu, Oct 20, 2022 at 06:39:14PM +0800, wangwenmei168@163.com wrote:
> From: gehao <gehao@kylinos.cn>

This does not match your from: line in the email at all, so we can't
take this :(

Please work with your company's email system to make it possible to send
kernel patches if you wish to be able to contribute.

> 
> When we use uPD720201 USB 3.0 Host Controller passthrough to VM
> guest os will report follow errors and it can not working.
> 
> xhci_hcd 0000:09:00.0: Host took too long to start, waited 16000
> microseconds.
> xhci_hcd 0000:09:00.0: startup error -19.
> 
> Because when we passthroug some device to our guest os,
> dev->iommu_group =NULL,so it will return from this function,
> Actually it still control by host os.
> I think that this condition is not necessary.
> 
> For host os with IOMMU,it is safe.
> For host os with noiommu,doing anything when there is no
> iommu is definitely.
> For guest os,the addresses we can access are restricted.
> 
> After add this path,they all work well.

This line isn't needed in a changelog, right?

> 
> Signed-off-by: gehao <gehao@kylinos.cn>
> ---
>  drivers/usb/host/xhci.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 5176765c4013..e8f4c4ee3ea3 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -241,12 +241,8 @@ static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
>  	 * changing the programming leads to extra accesses even if the
>  	 * controller is supposed to be halted. The controller ends up with
>  	 * a fatal fault, and is then ripe for being properly reset.
> -	 *
> -	 * Special care is taken to only apply this if the device is behind
> -	 * an iommu. Doing anything when there is no iommu is definitely
> -	 * unsafe...

How many different systems did you test this on to verify that this is
now ok to do?

thanks,

greg k-h
kernel test robot Oct. 20, 2022, 3:13 p.m. UTC | #2 
Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v6.1-rc1 next-20221020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/wangwenmei168-163-com/xhci-Fix-Renesas-PCIe-controllers-passthrough-issue/20221020-183913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20221020103914.262202-1-wangwenmei168%40163.com
patch subject: [RESEND PATCH] xhci: Fix Renesas PCIe controllers passthrough issue
config: i386-randconfig-a003
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/8398c1621f41a99e7fe38f1c7c705ea1c61f2411
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review wangwenmei168-163-com/xhci-Fix-Renesas-PCIe-controllers-passthrough-issue/20221020-183913
        git checkout 8398c1621f41a99e7fe38f1c7c705ea1c61f2411
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/host/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/host/xhci.c: In function 'xhci_zero_64b_regs':
>> drivers/usb/host/xhci.c:230:24: warning: unused variable 'dev' [-Wunused-variable]
     230 |         struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
         |                        ^~~


vim +/dev +230 drivers/usb/host/xhci.c

66d4eadd8d0672 drivers/usb/host/xhci-hcd.c Sarah Sharp   2009-04-27  227  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  228  static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  229  {
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23 @230  	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  231  	int err, i;
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  232  	u64 val;
286fd02fd54b6a drivers/usb/host/xhci.c     Mathias Nyman 2021-04-06  233  	u32 intrs;
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  234  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  235  	/*
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  236  	 * Some Renesas controllers get into a weird state if they are
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  237  	 * reset while programmed with 64bit addresses (they will preserve
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  238  	 * the top half of the address in internal, non visible
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  239  	 * registers). You end up with half the address coming from the
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  240  	 * kernel, and the other half coming from the firmware. Also,
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  241  	 * changing the programming leads to extra accesses even if the
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  242  	 * controller is supposed to be halted. The controller ends up with
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  243  	 * a fatal fault, and is then ripe for being properly reset.
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  244  	 */
8398c1621f41a9 drivers/usb/host/xhci.c     gehao         2022-10-20  245  	if (!(xhci->quirks & XHCI_ZERO_64B_REGS))
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  246  		return;
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  247  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  248  	xhci_info(xhci, "Zeroing 64bit base registers, expecting fault\n");
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  249  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  250  	/* Clear HSEIE so that faults do not get signaled */
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  251  	val = readl(&xhci->op_regs->command);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  252  	val &= ~CMD_HSEIE;
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  253  	writel(val, &xhci->op_regs->command);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  254  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  255  	/* Clear HSE (aka FATAL) */
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  256  	val = readl(&xhci->op_regs->status);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  257  	val |= STS_FATAL;
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  258  	writel(val, &xhci->op_regs->status);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  259  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  260  	/* Now zero the registers, and brace for impact */
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  261  	val = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  262  	if (upper_32_bits(val))
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  263  		xhci_write_64(xhci, 0, &xhci->op_regs->dcbaa_ptr);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  264  	val = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  265  	if (upper_32_bits(val))
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  266  		xhci_write_64(xhci, 0, &xhci->op_regs->cmd_ring);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  267  
286fd02fd54b6a drivers/usb/host/xhci.c     Mathias Nyman 2021-04-06  268  	intrs = min_t(u32, HCS_MAX_INTRS(xhci->hcs_params1),
286fd02fd54b6a drivers/usb/host/xhci.c     Mathias Nyman 2021-04-06  269  		      ARRAY_SIZE(xhci->run_regs->ir_set));
286fd02fd54b6a drivers/usb/host/xhci.c     Mathias Nyman 2021-04-06  270  
286fd02fd54b6a drivers/usb/host/xhci.c     Mathias Nyman 2021-04-06  271  	for (i = 0; i < intrs; i++) {
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  272  		struct xhci_intr_reg __iomem *ir;
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  273  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  274  		ir = &xhci->run_regs->ir_set[i];
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  275  		val = xhci_read_64(xhci, &ir->erst_base);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  276  		if (upper_32_bits(val))
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  277  			xhci_write_64(xhci, 0, &ir->erst_base);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  278  		val= xhci_read_64(xhci, &ir->erst_dequeue);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  279  		if (upper_32_bits(val))
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  280  			xhci_write_64(xhci, 0, &ir->erst_dequeue);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  281  	}
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  282  
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  283  	/* Wait for the fault to appear. It will be cleared on reset */
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  284  	err = xhci_handshake(&xhci->op_regs->status,
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  285  			     STS_FATAL, STS_FATAL,
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  286  			     XHCI_MAX_HALT_USEC);
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  287  	if (!err)
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  288  		xhci_info(xhci, "Fault detected\n");
12de0a35c996c3 drivers/usb/host/xhci.c     Marc Zyngier  2018-05-23  289  }
43b86af83da7db drivers/usb/host/xhci.c     Dong Nguyen   2010-07-21  290
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5176765c4013..e8f4c4ee3ea3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -241,12 +241,8 @@  static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
 	 * changing the programming leads to extra accesses even if the
 	 * controller is supposed to be halted. The controller ends up with
 	 * a fatal fault, and is then ripe for being properly reset.
-	 *
-	 * Special care is taken to only apply this if the device is behind
-	 * an iommu. Doing anything when there is no iommu is definitely
-	 * unsafe...
 	 */
-	if (!(xhci->quirks & XHCI_ZERO_64B_REGS) || !device_iommu_mapped(dev))
+	if (!(xhci->quirks & XHCI_ZERO_64B_REGS))
 		return;
 
 	xhci_info(xhci, "Zeroing 64bit base registers, expecting fault\n");