| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <58817A25.6080305@linux.intel.com>
Date: Fri, 20 Jan 2017 10:47:01 +0800
From: Lu Baolu <baolu.lu@...ux.intel.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Mathias Nyman <mathias.nyman@...ux.intel.com>,
Ingo Molnar <mingo@...hat.com>, tglx@...utronix.de,
peterz@...radead.org, linux-usb@...r.kernel.org, x86@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo,
I'm very appreciated for your review comments. I've put my
replies in lines.
On 01/19/2017 05:37 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@...ux.intel.com> wrote:
>
>> xHCI debug capability (DbC) is an optional but standalone
>> functionality provided by an xHCI host controller. Software
>> learns this capability by walking through the extended
>> capability list of the host. xHCI specification describes
>> DbC in section 7.6.
>>
>> This patch introduces the code to probe and initialize the
>> debug capability hardware during early boot. With hardware
>> initialized, the debug target (system on which this code is
>> running) will present a debug device through the debug port
>> (normally the first USB3 port). The debug device is fully
>> compliant with the USB framework and provides the equivalent
>> of a very high performance (USB3) full-duplex serial link
>> between the debug host and target. The DbC functionality is
>> independent of xHCI host. There isn't any precondition from
>> xHCI host side for DbC to work.
>>
>> This patch also includes bulk out and bulk in interfaces.
>> These interfaces could be used to implement early printk
>> bootconsole or hook to various system debuggers.
>>
>> This code is designed to be only used for kernel debugging
>> when machine crashes very early before the console code is
>> initialized. For normal operation it is not recommended.
>>
>> Cc: Mathias Nyman <mathias.nyman@...ux.intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> ---
>> arch/x86/Kconfig.debug | 14 +
>> drivers/usb/Kconfig | 3 +
>> drivers/usb/Makefile | 2 +-
>> drivers/usb/early/Makefile | 1 +
>> drivers/usb/early/xhci-dbc.c | 1068 +++++++++++++++++++++++++++++++++++++++++
>> drivers/usb/early/xhci-dbc.h | 205 ++++++++
>> include/linux/usb/xhci-dbgp.h | 22 +
>> 7 files changed, 1314 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/usb/early/xhci-dbc.c
>> create mode 100644 drivers/usb/early/xhci-dbc.h
>> create mode 100644 include/linux/usb/xhci-dbgp.h
>>
>> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
>> index 67eec55..13e85b7 100644
>> --- a/arch/x86/Kconfig.debug
>> +++ b/arch/x86/Kconfig.debug
>> @@ -29,6 +29,7 @@ config EARLY_PRINTK
>> config EARLY_PRINTK_DBGP
>> bool "Early printk via EHCI debug port"
>> depends on EARLY_PRINTK && PCI
>> + select USB_EARLY_PRINTK
>> ---help---
>> Write kernel log output directly into the EHCI debug port.
>>
>> @@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
>> This is useful for kernel debugging when your machine crashes very
>> early before the console code is initialized.
>>
>> +config EARLY_PRINTK_XDBC
>> + bool "Early printk via xHCI debug port"
>> + depends on EARLY_PRINTK && PCI
>> + select USB_EARLY_PRINTK
>> + ---help---
>> + Write kernel log output directly into the xHCI debug port.
>> +
>> + This is useful for kernel debugging when your machine crashes very
>> + early before the console code is initialized. For normal operation
>> + it is not recommended because it looks ugly and doesn't cooperate
>> + with klogd/syslogd or the X server. You should normally N here,
>> + unless you want to debug such a crash.
> Could we please do this rename:
>
> s/EARLY_PRINTK_XDBC
> EARLY_PRINTK_USB_XDBC
>
> ?
>
> As many people will not realize what 'xdbc' means, standalone - while "it's an
> USB serial logging variant" is a lot more natural.
>
>
>> +config USB_EARLY_PRINTK
>> + bool
> Also, could we standardize the nomencalture to not be a mixture of prefixes and
> postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig space)
> and rename this one to EARLY_PRINTK_USB or so?
>
> You can see the prefix/postfix inconsistency here already:
Sure. I will fix the names. Thanks.
>
>> -obj-$(CONFIG_EARLY_PRINTK_DBGP) += early/
>> +obj-$(CONFIG_USB_EARLY_PRINTK) += early/
>> +obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
>> +static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
>> +{
>> + u32 val, sz;
>> + u64 val64, sz64, mask64;
>> + u8 byte;
>> + void __iomem *base;
>> +
>> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
>> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, ~0);
>> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
>> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, val);
>> + if (val == 0xffffffff || sz == 0xffffffff) {
>> + pr_notice("invalid mmio bar\n");
>> + return NULL;
>> + }
>> + if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
>> + PCI_BASE_ADDRESS_MEM_TYPE_64) {
> Please don't break the line here.
Sure. Will fix it.
>
>> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
>> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
>> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
>> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, val);
>> +
>> + val64 |= ((u64)val << 32);
>> + sz64 |= ((u64)sz << 32);
>> + mask64 |= ((u64)~0 << 32);
> Unnecessary parentheses.
Sure. Will fix it.
>
>> + }
>> +
>> + sz64 &= mask64;
>> +
>> + if (sizeof(dma_addr_t) < 8 || !sz64) {
>> + pr_notice("invalid mmio address\n");
>> + return NULL;
>> + }
> So this doesn't work on regular 32-bit kernels?
I will run my code on a 32-bit kernel and remove this check
if it passes the test.
>
>> +static u32 __init xdbc_find_dbgp(int xdbc_num, u32 *b, u32 *d, u32 *f)
>> +{
>> + u32 bus, dev, func, class;
>> +
>> + for (bus = 0; bus < XDBC_PCI_MAX_BUSES; bus++) {
>> + for (dev = 0; dev < XDBC_PCI_MAX_DEVICES; dev++) {
>> + for (func = 0; func < XDBC_PCI_MAX_FUNCTION; func++) {
>> + class = read_pci_config(bus, dev, func,
>> + PCI_CLASS_REVISION);
> Please no ugly linebreaks.
Sorry. I will fix it.
>
>> +static void xdbc_runtime_delay(unsigned long count)
>> +{
>> + udelay(count);
>> +}
>> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
> Is this udelay() complication really necessary? udelay() should work fine even in
> early code. It might not be precisely calibrated, but should be good enough.
I tried udelay() in the early code. It's not precise enough for the
hardware handshaking.
>
>> +static int handshake(void __iomem *ptr, u32 mask, u32 done,
>> + int wait, int delay)
> Please break lines more intelligently:
>
> static int
> handshake(void __iomem *ptr, u32 mask, u32 done, int wait, int delay)
Sure. I will fix it.
>
>> + ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
>> + 0, XHCI_EXT_CAPS_LEGACY);
> No ugly linebreaks please. There's a ton more in other parts of this patch and
> other patches: please review all the other linebreaks (and ignore checkpatch.pl).
>
> For example this:
>
>> + xdbc.erst_base = xdbc.table_base +
>> + index * XDBC_TABLE_ENTRY_SIZE;
>> + xdbc.erst_dma = xdbc.table_dma +
>> + index * XDBC_TABLE_ENTRY_SIZE;
> should be:
>
> xdbc.erst_base = xdbc.table_base + index*XDBC_TABLE_ENTRY_SIZE;
> xdbc.erst_dma = xdbc.table_dma + index*XDBC_TABLE_ENTRY_SIZE;
>
> which makes it much more readable, etc.
Sure.
These line breaks were added to make checkpatch.pl happy.
I will review all the line breaks and make them more readable.
>
>> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
>> +{
>> + int chunk, ret;
>> + static char buf[XDBC_MAX_PACKET];
>> + int use_cr = 0;
>> +
>> + if (!xdbc.xdbc_reg)
>> + return;
>> + memset(buf, 0, XDBC_MAX_PACKET);
>> + while (n > 0) {
>> + for (chunk = 0; chunk < XDBC_MAX_PACKET && n > 0;
>> + str++, chunk++, n--) {
>> + if (!use_cr && *str == '\n') {
>> + use_cr = 1;
>> + buf[chunk] = '\r';
>> + str--;
>> + n++;
>> + continue;
>> + }
>> + if (use_cr)
>> + use_cr = 0;
>> + buf[chunk] = *str;
> Hm, why are newlines converted to \r\n unconditionally? Makes for a crappy minicom
> log on the other side ...
Yes. The usb ehci (usb2) debug port driver (drivers/usb/early/ehci-dbgp.c)
does this. I kept the same for xhci (usb3). It turns out to be good for display
on host side.
>
>> +static int __init xdbc_init(void)
>> +{
>> + unsigned long flags;
>> + void __iomem *base;
>> + u32 offset;
>> + int ret = 0;
>> +
>> + if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
>> + return 0;
>> +
>> + xdbc_delay = xdbc_runtime_delay;
>> +
>> + /*
>> + * It's time to shutdown DbC, so that the debug
>> + * port could be reused by the host controller.
> s/shutdown DbC
> /shut down the DbC
>
> s/could be reused
> /can be reused
>
> ?
>
Sure. I will fix it. Thanks.
>> + */
>> + if (early_xdbc_console.index == -1 ||
>> + (early_xdbc_console.flags & CON_BOOT)) {
>> + xdbc_trace("hardware not used any more\n");
> s/any more
> anymore
Sure. I will fix it. Thanks.
>
>> + raw_spin_lock_irqsave(&xdbc.lock, flags);
>> + base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
> Ugh, ioremap() can sleep ...
Oh, right. I will remove the remapping code and let it use the
previously mapped one.
>
>> +/**
>> + * struct xdbc_regs - xHCI Debug Capability Register interface.
>> + */
>> +struct xdbc_regs {
>> + __le32 capability;
>> + __le32 doorbell;
>> + __le32 ersts; /* Event Ring Segment Table Size*/
>> + __le32 rvd0; /* 0c~0f reserved bits */
> Yeah, so thsbbrvtnssck. (these abbreviations suck)
>
> Why 'rvd0' - did we run out of letters? Please name it __reserved_0 and
> __reserved_1 like we typically do in kernel code.
Sure. I will fix it. Thanks.
>
>> + __le32 rsvd;
>> + __le32 rsvdz[7];
>> + __le32 rsvd0[11];
> ditto.
I will fix them.
>
>> +#define XDBC_INFO_CONTEXT_SIZE 48
>> +
>> +#define XDBC_MAX_STRING_LENGTH 64
>> +#define XDBC_STRING_MANUFACTURE "Linux"
>> +#define XDBC_STRING_PRODUCT "Remote GDB"
>> +#define XDBC_STRING_SERIAL "0001"
>> +struct xdbc_strings {
> Please put a newline between different types of definitions.
Sure.
>
>> + char string0[XDBC_MAX_STRING_LENGTH];
>> + char manufacture[XDBC_MAX_STRING_LENGTH];
>> + char product[XDBC_MAX_STRING_LENGTH];
>> + char serial[XDBC_MAX_STRING_LENGTH];
> s/manufacture/manufacturer
>
> ?
Sure.
>
>> +};
>> +
>> +#define XDBC_PROTOCOL 1 /* GNU Remote Debug Command Set */
>> +#define XDBC_VENDOR_ID 0x1d6b /* Linux Foundation 0x1d6b */
>> +#define XDBC_PRODUCT_ID 0x0004 /* __le16 idProduct; device 0004 */
>> +#define XDBC_DEVICE_REV 0x0010 /* 0.10 */
>> +
>> +/*
>> + * software state structure
>> + */
>> +struct xdbc_segment {
>> + struct xdbc_trb *trbs;
>> + dma_addr_t dma;
>> +};
>> +
>> +#define XDBC_TRBS_PER_SEGMENT 256
>> +
>> +struct xdbc_ring {
>> + struct xdbc_segment *segment;
>> + struct xdbc_trb *enqueue;
>> + struct xdbc_trb *dequeue;
>> + u32 cycle_state;
>> +};
>> +
>> +#define XDBC_EPID_OUT 2
>> +#define XDBC_EPID_IN 3
>> +
>> +struct xdbc_state {
>> + /* pci device info*/
>> + u16 vendor;
>> + u16 device;
>> + u32 bus;
>> + u32 dev;
>> + u32 func;
>> + void __iomem *xhci_base;
>> + u64 xhci_start;
>> + size_t xhci_length;
>> + int port_number;
>> +#define XDBC_PCI_MAX_BUSES 256
>> +#define XDBC_PCI_MAX_DEVICES 32
>> +#define XDBC_PCI_MAX_FUNCTION 8
>> +
>> + /* DbC register base */
>> + struct xdbc_regs __iomem *xdbc_reg;
>> +
>> + /* DbC table page */
>> + dma_addr_t table_dma;
>> + void *table_base;
>> +
>> +#define XDBC_TABLE_ENTRY_SIZE 64
>> +#define XDBC_ERST_ENTRY_NUM 1
>> +#define XDBC_DBCC_ENTRY_NUM 3
>> +#define XDBC_STRING_ENTRY_NUM 4
>> +
>> + /* event ring segment table */
>> + dma_addr_t erst_dma;
>> + size_t erst_size;
>> + void *erst_base;
>> +
>> + /* event ring segments */
>> + struct xdbc_ring evt_ring;
>> + struct xdbc_segment evt_seg;
>> +
>> + /* debug capability contexts */
>> + dma_addr_t dbcc_dma;
>> + size_t dbcc_size;
>> + void *dbcc_base;
>> +
>> + /* descriptor strings */
>> + dma_addr_t string_dma;
>> + size_t string_size;
>> + void *string_base;
>> +
>> + /* bulk OUT endpoint */
>> + struct xdbc_ring out_ring;
>> + struct xdbc_segment out_seg;
>> + void *out_buf;
>> + dma_addr_t out_dma;
>> +
>> + /* bulk IN endpoint */
>> + struct xdbc_ring in_ring;
>> + struct xdbc_segment in_seg;
>> + void *in_buf;
>> + dma_addr_t in_dma;
> Please make the vertical tabulation of the fields consistent throughout the
> structure. Look at it in a terminal and convince yourself that it's nice and
> beautiful to look at!
>
> Also, if you mix CPP #defines into structure definitions then tabulate them in a
> similar fashion.
Sure. I will fix this. Thank you.
Best regards,
Lu Baolu
Powered by blists - more mailing lists