[PATCH v2 0/2] ns16550: enable use of PCI MSI

[PATCH v2 0/2] ns16550: enable use of PCI MSI

[Jan Beulich]

Patch 2 is no longer RFC, now that I have a device where MSI actually
works (suggesting that it was really the other device's fault that things
didn't work).

1: console: adjust IRQ initialization
2: ns16550: enable use of PCI MSI

Jan



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

[PATCH v2 1/2] console: adjust IRQ initialization

[Jan Beulich]

In order for a Xen internal PCI device driver to enable MSI on the
device, we need another hook which the driver can use to create the IRQ
(doing this in the init_preirq hook is too early, since IRQ code hasn't
got initialized at that time yet, and doing it in init_postirq is too
late because at least on x86 smp_intr_init() needs to know the IRQ
number).

On x86 this additionally requires a slight ordering change to IRQ
initialization, to facilitate calling the new hook between basic
initialization and the call path leading to smp_intr_init().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base.

--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -341,8 +341,6 @@ void __init init_IRQ(void)
 
     init_8259A(0);
 
-    BUG_ON(init_irq_data() < 0);
-
     for (irq = 0; platform_legacy_irq(irq); irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -677,6 +677,7 @@ void __init noreturn __start_xen(unsigne
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
     int i, j, e820_warn = 0, bytes = 0;
     bool acpi_boot_table_init_done = false, relocated = false;
+    int ret;
     struct ns16550_defaults ns16550 = {
         .data_bits = 8,
         .parity    = 'n',
@@ -1559,6 +1560,12 @@ void __init noreturn __start_xen(unsigne
 
     x2apic_bsp_setup();
 
+    ret = init_irq_data();
+    if ( ret < 0 )
+        panic("Error %d setting up IRQ data\n", ret);
+
+    console_init_irq();
+
     init_IRQ();
 
     module_map = xmalloc_array(unsigned long, BITS_TO_LONGS(mbi->mods_count));
@@ -1661,7 +1668,7 @@ void __init noreturn __start_xen(unsigne
             if ( (park_offline_cpus || num_online_cpus() < max_cpus) &&
                  !cpu_online(i) )
             {
-                int ret = cpu_up(i);
+                ret = cpu_up(i);
                 if ( ret != 0 )
                     printk("Failed to bring up CPU %u (error %d)\n", i, ret);
                 else if ( num_online_cpus() > max_cpus ||
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -909,6 +909,11 @@ void __init console_init_ring(void)
     printk("Allocated console ring of %u KiB.\n", opt_conring_size >> 10);
 }
 
+void __init console_init_irq(void)
+{
+    serial_init_irq();
+}
+
 void __init console_init_postirq(void)
 {
     serial_init_postirq();
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -503,6 +503,15 @@ void __init serial_init_preirq(void)
             com[i].driver->init_preirq(&com[i]);
 }
 
+void __init serial_init_irq(void)
+{
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(com); i++ )
+        if ( com[i].driver && com[i].driver->init_irq )
+            com[i].driver->init_irq(&com[i]);
+}
+
 void __init serial_init_postirq(void)
 {
     int i;
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -15,6 +15,7 @@ long read_console_ring(struct xen_sysctl
 
 void console_init_preirq(void);
 void console_init_ring(void);
+void console_init_irq(void);
 void console_init_postirq(void);
 void console_endboot(void);
 int console_has(const char *device);
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -64,6 +64,7 @@ struct serial_port {
 struct uart_driver {
     /* Driver initialisation (pre- and post-IRQ subsystem setup). */
     void (*init_preirq)(struct serial_port *);
+    void (*init_irq)(struct serial_port *);
     void (*init_postirq)(struct serial_port *);
     /* Hook to clean up after Xen bootstrap (before domain 0 runs). */
     void (*endboot)(struct serial_port *);
@@ -99,8 +100,9 @@ struct uart_driver {
 #define SERHND_LO       (1<<3) /* Ditto, except that the MSB is cleared.  */
 #define SERHND_COOKED   (1<<4) /* Newline/carriage-return translation?    */
 
-/* Two-stage initialisation (before/after IRQ-subsystem initialisation). */
+/* Three-stage initialisation (before/during/after IRQ-subsystem setup). */
 void serial_init_preirq(void);
+void serial_init_irq(void);
 void serial_init_postirq(void);
 
 /* Clean-up hook before domain 0 runs. */




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

[PATCH v2 2/2] ns16550: enable use of PCI MSI

[Jan Beulich]

Which, on x86, requires fiddling with the INTx bit in PCI config space,
since for internally used MSI we can't delegate this to Dom0.

ns16550_init_postirq() also needs (benign) re-ordering of its
operations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base.

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -307,7 +307,7 @@ Flag to indicate whether to probe for a
 ACPI indicating none to be there.
 
 ### com1,com2
-> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
+> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
@@ -328,7 +328,7 @@ Both option `com1` and `com2` follow the
 * `<io-base>` is an integer which specifies the IO base port for UART
   registers.
 * `<irq>` is the IRQ number to use, or `0` to use the UART in poll
-  mode only.
+  mode only, or `msi` to set up a Message Signaled Interrupt.
 * `<port-bdf>` is the PCI location of the UART, in
   `<bus>:<device>.<function>` notation.
 * `<bridge-bdf>` is the PCI bridge behind which is the UART, in
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
 
     *desc = entry;
     /* Restore the original MSI enabled bits  */
+    if ( !hardware_domain )
+    {
+        /*
+         * ..., except for internal requests (before Dom0 starts), in which
+         * case we rather need to behave "normally", i.e. not follow the split
+         * brain model where Dom0 actually enables MSI (and disables INTx).
+         */
+        pci_intx(dev, 0);
+        control |= PCI_MSI_FLAGS_ENABLE;
+    }
     pci_conf_write16(seg, bus, slot, func, msi_control_reg(pos), control);
 
     return 0;
@@ -1019,6 +1029,18 @@ static int msix_capability_init(struct p
     ++msix->used_entries;
 
     /* Restore MSI-X enabled bits */
+    if ( !hardware_domain )
+    {
+        /*
+         * ..., except for internal requests (before Dom0 starts), in which
+         * case we rather need to behave "normally", i.e. not follow the split
+         * brain model where Dom0 actually enables MSI (and disables INTx).
+         */
+        pci_intx(dev, 0);
+        control |= PCI_MSIX_FLAGS_ENABLE;
+        control &= ~PCI_MSIX_FLAGS_MASKALL;
+        maskall = 0;
+    }
     msix->host_maskall = maskall;
     pci_conf_write16(seg, bus, slot, func, msix_control_reg(pos), control);
 
@@ -1073,6 +1095,8 @@ static void __pci_disable_msi(struct msi
 
     dev = entry->dev;
     msi_set_enable(dev, 0);
+    if ( entry->irq > 0 && !(irq_to_desc(entry->irq)->status & IRQ_GUEST) )
+        pci_intx(dev, 1);
 
     BUG_ON(list_empty(&dev->msi_list));
 }
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -92,6 +92,7 @@ static struct ns16550 {
     u32 bar64;
     u16 cr;
     u8 bar_idx;
+    bool_t msi;
     const struct ns16550_config_param *param; /* Points into .init.*! */
 #endif
 } ns16550_com[2] = { { 0 } };
@@ -712,6 +713,16 @@ static void __init ns16550_init_preirq(s
         uart->fifo_size = 16;
 }
 
+static void __init ns16550_init_irq(struct serial_port *port)
+{
+#ifdef CONFIG_HAS_PCI
+    struct ns16550 *uart = port->uart;
+
+    if ( uart->msi )
+        uart->irq = create_irq(0);
+#endif
+}
+
 static void ns16550_setup_postirq(struct ns16550 *uart)
 {
     if ( uart->irq > 0 )
@@ -746,17 +757,6 @@ static void __init ns16550_init_postirq(
     uart->timeout_ms = max_t(
         unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
 
-    if ( uart->irq > 0 )
-    {
-        uart->irqaction.handler = ns16550_interrupt;
-        uart->irqaction.name    = "ns16550";
-        uart->irqaction.dev_id  = port;
-        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
-            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
-    }
-
-    ns16550_setup_postirq(uart);
-
 #ifdef CONFIG_HAS_PCI
     if ( uart->bar || uart->ps_bdf_enable )
     {
@@ -777,8 +777,65 @@ static void __init ns16550_init_postirq(
                                     uart->ps_bdf[0], uart->ps_bdf[1],
                                     uart->ps_bdf[2]);
         }
+
+        if ( uart->msi )
+        {
+            struct msi_info msi = {
+                .bus = uart->ps_bdf[0],
+                .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
+                .irq = rc = uart->irq,
+                .entry_nr = 1
+            };
+
+            if ( rc > 0 )
+            {
+                struct msi_desc *msi_desc = NULL;
+
+                pcidevs_lock();
+
+                rc = pci_enable_msi(&msi, &msi_desc);
+                if ( !rc )
+                {
+                    struct irq_desc *desc = irq_to_desc(msi.irq);
+                    unsigned long flags;
+
+                    spin_lock_irqsave(&desc->lock, flags);
+                    rc = setup_msi_irq(desc, msi_desc);
+                    spin_unlock_irqrestore(&desc->lock, flags);
+                    if ( rc )
+                        pci_disable_msi(msi_desc);
+                }
+
+                pcidevs_unlock();
+
+                if ( rc )
+                {
+                    uart->irq = 0;
+                    if ( msi_desc )
+                        msi_free_irq(msi_desc);
+                    else
+                        destroy_irq(msi.irq);
+                }
+            }
+
+            if ( rc )
+                printk(XENLOG_WARNING
+                       "MSI setup failed (%d) for %02x:%02x.%o\n",
+                       rc, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]);
+        }
     }
 #endif
+
+    if ( uart->irq > 0 )
+    {
+        uart->irqaction.handler = ns16550_interrupt;
+        uart->irqaction.name    = "ns16550";
+        uart->irqaction.dev_id  = port;
+        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
+            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
+    }
+
+    ns16550_setup_postirq(uart);
 }
 
 static void ns16550_suspend(struct serial_port *port)
@@ -908,6 +965,7 @@ static const struct vuart_info *ns16550_
 
 static struct uart_driver __read_mostly ns16550_driver = {
     .init_preirq  = ns16550_init_preirq,
+    .init_irq     = ns16550_init_irq,
     .init_postirq = ns16550_init_postirq,
     .endboot      = ns16550_endboot,
     .suspend      = ns16550_suspend,
@@ -1261,7 +1319,18 @@ static bool __init parse_positional(stru
     }
 
     if ( *conf == ',' && *++conf != ',' )
-        uart->irq = simple_strtol(conf, &conf, 10);
+    {
+#ifdef CONFIG_HAS_PCI
+        if ( strncmp(conf, "msi", 3) == 0 )
+        {
+            conf += 3;
+            uart->msi = 1;
+            uart->irq = 0;
+        }
+        else
+#endif
+            uart->irq = simple_strtol(conf, &conf, 10);
+    }
 
 #ifdef CONFIG_HAS_PCI
     if ( *conf == ',' && *++conf != ',' )
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
     return 0;
 }
 
+void pci_intx(const struct pci_dev *pdev, bool_t enable)
+{
+    uint16_t seg = pdev->seg;
+    uint8_t bus = pdev->bus;
+    uint8_t slot = PCI_SLOT(pdev->devfn);
+    uint8_t func = PCI_FUNC(pdev->devfn);
+    uint16_t cmd = pci_conf_read16(seg, bus, slot, func, PCI_COMMAND);
+
+    if ( enable )
+        cmd &= ~PCI_COMMAND_INTX_DISABLE;
+    else
+        cmd |= PCI_COMMAND_INTX_DISABLE;
+    pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
+}
+
 const char *__init parse_pci(const char *s, unsigned int *seg_p,
                              unsigned int *bus_p, unsigned int *dev_p,
                              unsigned int *func_p)
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -197,6 +197,7 @@ unsigned int pci_size_mem_bar(pci_sbdf_t
                               uint64_t *paddr, uint64_t *psize,
                               unsigned int flags);
 
+void pci_intx(const struct pci_dev *, bool_t enable);
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
 
 struct pirq;




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

Ping: [PATCH v2 0/2] ns16550: enable use of PCI MSI

[Jan Beulich]

>>> On 01.10.18 at 18:03, <JBeulich@suse.com> wrote:
> Patch 2 is no longer RFC, now that I have a device where MSI actually
> works (suggesting that it was really the other device's fault that things
> didn't work).
> 
> 1: console: adjust IRQ initialization
> 2: ns16550: enable use of PCI MSI
> 
> Jan
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org 
> https://lists.xenproject.org/mailman/listinfo/xen-devel 





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

Ping#2: [PATCH v2 0/2] ns16550: enable use of PCI MSI

[Jan Beulich]

>>> On 26.10.18 at 13:06, <JBeulich@suse.com> wrote:
>>>> On 01.10.18 at 18:03, <JBeulich@suse.com> wrote:
>> Patch 2 is no longer RFC, now that I have a device where MSI actually
>> works (suggesting that it was really the other device's fault that things
>> didn't work).
>> 
>> 1: console: adjust IRQ initialization
>> 2: ns16550: enable use of PCI MSI

Unless I hear back any objections, I'm going to commit this pair of
patches as well as "ns16550/PCI: fix skipping of devices" in a week's
time, irrespective of the lack of acks.

Jan



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

Re: [PATCH v2 1/2] console: adjust IRQ initialization

[Roger Pau Monné]

On Mon, Oct 01, 2018 at 10:25:33AM -0600, Jan Beulich wrote:
> In order for a Xen internal PCI device driver to enable MSI on the
> device, we need another hook which the driver can use to create the IRQ
> (doing this in the init_preirq hook is too early, since IRQ code hasn't
> got initialized at that time yet, and doing it in init_postirq is too
> late because at least on x86 smp_intr_init() needs to know the IRQ
> number).
> 
> On x86 this additionally requires a slight ordering change to IRQ
> initialization, to facilitate calling the new hook between basic
> initialization and the call path leading to smp_intr_init().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

[Roger Pau Monné]

On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
> Which, on x86, requires fiddling with the INTx bit in PCI config space,
> since for internally used MSI we can't delegate this to Dom0.
> 
> ns16550_init_postirq() also needs (benign) re-ordering of its
> operations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Re-base.
> 
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -307,7 +307,7 @@ Flag to indicate whether to probe for a
>  ACPI indicating none to be there.
>  
>  ### com1,com2
> -> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
> +> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
>  
>  Both option `com1` and `com2` follow the same format.
>  
> @@ -328,7 +328,7 @@ Both option `com1` and `com2` follow the
>  * `<io-base>` is an integer which specifies the IO base port for UART
>    registers.
>  * `<irq>` is the IRQ number to use, or `0` to use the UART in poll
> -  mode only.
> +  mode only, or `msi` to set up a Message Signaled Interrupt.
>  * `<port-bdf>` is the PCI location of the UART, in
>    `<bus>:<device>.<function>` notation.
>  * `<bridge-bdf>` is the PCI bridge behind which is the UART, in
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
>  
>      *desc = entry;
>      /* Restore the original MSI enabled bits  */
> +    if ( !hardware_domain )

Wouldn't it be better to assign the device to dom_xen (pdev->domain =
dom_xen), and then check if the owner is dom_xen here?

Or at the point where this is called from the serial console driver is
too early for dom_xen to exist?

> +    {
> +        /*
> +         * ..., except for internal requests (before Dom0 starts), in which
> +         * case we rather need to behave "normally", i.e. not follow the split
> +         * brain model where Dom0 actually enables MSI (and disables INTx).
> +         */
> +        pci_intx(dev, 0);

If you use bool with pci_intx then you can just pass false here.

> +        control |= PCI_MSI_FLAGS_ENABLE;
> +    }
>      pci_conf_write16(seg, bus, slot, func, msi_control_reg(pos), control);
>  
>      return 0;
> @@ -1019,6 +1029,18 @@ static int msix_capability_init(struct p
>      ++msix->used_entries;
>  
>      /* Restore MSI-X enabled bits */
> +    if ( !hardware_domain )
> +    {
> +        /*
> +         * ..., except for internal requests (before Dom0 starts), in which
> +         * case we rather need to behave "normally", i.e. not follow the split
> +         * brain model where Dom0 actually enables MSI (and disables INTx).
> +         */
> +        pci_intx(dev, 0);
> +        control |= PCI_MSIX_FLAGS_ENABLE;
> +        control &= ~PCI_MSIX_FLAGS_MASKALL;
> +        maskall = 0;
> +    }
>      msix->host_maskall = maskall;
>      pci_conf_write16(seg, bus, slot, func, msix_control_reg(pos), control);
>  
> @@ -1073,6 +1095,8 @@ static void __pci_disable_msi(struct msi
>  
>      dev = entry->dev;
>      msi_set_enable(dev, 0);
> +    if ( entry->irq > 0 && !(irq_to_desc(entry->irq)->status & IRQ_GUEST) )
> +        pci_intx(dev, 1);
>  
>      BUG_ON(list_empty(&dev->msi_list));
>  }
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -92,6 +92,7 @@ static struct ns16550 {
>      u32 bar64;
>      u16 cr;
>      u8 bar_idx;
> +    bool_t msi;
>      const struct ns16550_config_param *param; /* Points into .init.*! */
>  #endif
>  } ns16550_com[2] = { { 0 } };
> @@ -712,6 +713,16 @@ static void __init ns16550_init_preirq(s
>          uart->fifo_size = 16;
>  }
>  
> +static void __init ns16550_init_irq(struct serial_port *port)
> +{
> +#ifdef CONFIG_HAS_PCI
> +    struct ns16550 *uart = port->uart;
> +
> +    if ( uart->msi )
> +        uart->irq = create_irq(0);
> +#endif
> +}
> +
>  static void ns16550_setup_postirq(struct ns16550 *uart)
>  {
>      if ( uart->irq > 0 )
> @@ -746,17 +757,6 @@ static void __init ns16550_init_postirq(
>      uart->timeout_ms = max_t(
>          unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>  
> -    if ( uart->irq > 0 )
> -    {
> -        uart->irqaction.handler = ns16550_interrupt;
> -        uart->irqaction.name    = "ns16550";
> -        uart->irqaction.dev_id  = port;
> -        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
> -            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
> -    }
> -
> -    ns16550_setup_postirq(uart);
> -
>  #ifdef CONFIG_HAS_PCI
>      if ( uart->bar || uart->ps_bdf_enable )
>      {
> @@ -777,8 +777,65 @@ static void __init ns16550_init_postirq(
>                                      uart->ps_bdf[0], uart->ps_bdf[1],
>                                      uart->ps_bdf[2]);
>          }
> +
> +        if ( uart->msi )
> +        {
> +            struct msi_info msi = {
> +                .bus = uart->ps_bdf[0],
> +                .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
> +                .irq = rc = uart->irq,
> +                .entry_nr = 1
> +            };
> +
> +            if ( rc > 0 )
> +            {
> +                struct msi_desc *msi_desc = NULL;
> +
> +                pcidevs_lock();
> +
> +                rc = pci_enable_msi(&msi, &msi_desc);

Before attempting to enable MSI, shouldn't you make sure memory
decoding is enabled in case the device uses MSIX?

I think this code assumes the device will always use MSI? (in which
case my above question is likely moot).

> +                if ( !rc )
> +                {
> +                    struct irq_desc *desc = irq_to_desc(msi.irq);
> +                    unsigned long flags;
> +
> +                    spin_lock_irqsave(&desc->lock, flags);
> +                    rc = setup_msi_irq(desc, msi_desc);
> +                    spin_unlock_irqrestore(&desc->lock, flags);
> +                    if ( rc )
> +                        pci_disable_msi(msi_desc);
> +                }
> +
> +                pcidevs_unlock();
> +
> +                if ( rc )
> +                {
> +                    uart->irq = 0;
> +                    if ( msi_desc )
> +                        msi_free_irq(msi_desc);
> +                    else
> +                        destroy_irq(msi.irq);
> +                }
> +            }
> +
> +            if ( rc )
> +                printk(XENLOG_WARNING
> +                       "MSI setup failed (%d) for %02x:%02x.%o\n",
> +                       rc, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]);
> +        }
>      }
>  #endif
> +
> +    if ( uart->irq > 0 )
> +    {
> +        uart->irqaction.handler = ns16550_interrupt;
> +        uart->irqaction.name    = "ns16550";
> +        uart->irqaction.dev_id  = port;
> +        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
> +            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
> +    }
> +
> +    ns16550_setup_postirq(uart);
>  }
>  
>  static void ns16550_suspend(struct serial_port *port)
> @@ -908,6 +965,7 @@ static const struct vuart_info *ns16550_
>  
>  static struct uart_driver __read_mostly ns16550_driver = {
>      .init_preirq  = ns16550_init_preirq,
> +    .init_irq     = ns16550_init_irq,
>      .init_postirq = ns16550_init_postirq,
>      .endboot      = ns16550_endboot,
>      .suspend      = ns16550_suspend,
> @@ -1261,7 +1319,18 @@ static bool __init parse_positional(stru
>      }
>  
>      if ( *conf == ',' && *++conf != ',' )
> -        uart->irq = simple_strtol(conf, &conf, 10);
> +    {
> +#ifdef CONFIG_HAS_PCI
> +        if ( strncmp(conf, "msi", 3) == 0 )
> +        {
> +            conf += 3;
> +            uart->msi = 1;
> +            uart->irq = 0;
> +        }
> +        else
> +#endif
> +            uart->irq = simple_strtol(conf, &conf, 10);
> +    }
>  
>  #ifdef CONFIG_HAS_PCI
>      if ( *conf == ',' && *++conf != ',' )
> --- a/xen/drivers/pci/pci.c
> +++ b/xen/drivers/pci/pci.c
> @@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
>      return 0;
>  }
>  
> +void pci_intx(const struct pci_dev *pdev, bool_t enable)

Please use bool.

Thanks, Roger.

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

Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

[Jan Beulich]

>>> On 29.11.18 at 18:33, <roger.pau@citrix.com> wrote:
> On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
>>  
>>      *desc = entry;
>>      /* Restore the original MSI enabled bits  */
>> +    if ( !hardware_domain )
> 
> Wouldn't it be better to assign the device to dom_xen (pdev->domain =
> dom_xen), and then check if the owner is dom_xen here?

I'm not sure this couldn't be wrong in the general case (and we
sit on a generic code path here): It depends on whether Dom0
can modify the device's config space, and I wouldn't want to
(here) introduce a connection between dom_xen ownership and
whether Dom0 can control INTX. The comment below here is
specifically worded to the effect of why I use hardware_domain
here.

If we ever get into the situation of wanting to enable MSI on an
internally used device _after_ Dom0 has started, this would need
careful re-considering.

> Or at the point where this is called from the serial console driver is
> too early for dom_xen to exist?

ns16550_init_postirq() is where both MSI setup and hiding of the
device happen, so in principle this would seem to be possible for
the specific case of a serial card.

>> @@ -777,8 +777,65 @@ static void __init ns16550_init_postirq(
>>                                      uart->ps_bdf[0], uart->ps_bdf[1],
>>                                      uart->ps_bdf[2]);
>>          }
>> +
>> +        if ( uart->msi )
>> +        {
>> +            struct msi_info msi = {
>> +                .bus = uart->ps_bdf[0],
>> +                .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
>> +                .irq = rc = uart->irq,
>> +                .entry_nr = 1
>> +            };
>> +
>> +            if ( rc > 0 )
>> +            {
>> +                struct msi_desc *msi_desc = NULL;
>> +
>> +                pcidevs_lock();
>> +
>> +                rc = pci_enable_msi(&msi, &msi_desc);
> 
> Before attempting to enable MSI, shouldn't you make sure memory
> decoding is enabled in case the device uses MSIX?
> 
> I think this code assumes the device will always use MSI? (in which
> case my above question is likely moot).

I've yet to see serial cards using MSI-X. If we get to the point where
this is needed, we also may need to first set up the BAR for the MSI-X
table. Furthermore pci_enable_msi() won't even try to enable MSI-X
with msi->table_base being zero.

>> --- a/xen/drivers/pci/pci.c
>> +++ b/xen/drivers/pci/pci.c
>> @@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
>>      return 0;
>>  }
>>  
>> +void pci_intx(const struct pci_dev *pdev, bool_t enable)
> 
> Please use bool.

See how old this patch is. V1 was posted long before bool came into
existence, and I had refrained from posting v2 until I actually had a
device where MSI would indeed function (the first two I tried this
with claimed to be MSI capable, but no interrupts ever surfaced
when MSI was enabled on them, yet I couldn't be sure the code
was doing something wrong). Obviously I then forgot to switch this,
which I've now done.

Jan



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

Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

[Roger Pau Monné]

On Fri, Nov 30, 2018 at 01:52:39AM -0700, Jan Beulich wrote:
> >>> On 29.11.18 at 18:33, <roger.pau@citrix.com> wrote:
> > On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
> >> --- a/xen/arch/x86/msi.c
> >> +++ b/xen/arch/x86/msi.c
> >> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
> >>  
> >>      *desc = entry;
> >>      /* Restore the original MSI enabled bits  */
> >> +    if ( !hardware_domain )
> > 
> > Wouldn't it be better to assign the device to dom_xen (pdev->domain =
> > dom_xen), and then check if the owner is dom_xen here?
> 
> I'm not sure this couldn't be wrong in the general case (and we
> sit on a generic code path here): It depends on whether Dom0
> can modify the device's config space, and I wouldn't want to
> (here) introduce a connection between dom_xen ownership and
> whether Dom0 can control INTX. The comment below here is
> specifically worded to the effect of why I use hardware_domain
> here.

Well, I think Dom0 shouldn't be allowed to interact with devices owned
by dom_xen. That being set, at least the current vPCI code will allow
PVH Dom0 to do so by passing through any accesses to registers not
explicitly handled by vPCI.

> If we ever get into the situation of wanting to enable MSI on an
> internally used device _after_ Dom0 has started, this would need
> careful re-considering.

OK, I'm fine with this. Maybe using system_state would be clearer to
note that this code path is only to be used during early boot?

> > Or at the point where this is called from the serial console driver is
> > too early for dom_xen to exist?
> 
> ns16550_init_postirq() is where both MSI setup and hiding of the
> device happen, so in principle this would seem to be possible for
> the specific case of a serial card.

IMO it's clear from a conceptual PoV to check against the ownership of
the device rather than the system state at the point of the function
call. Devices assigned to Dom0 use the split model, devices assigned
to Xen don't.

> >> @@ -777,8 +777,65 @@ static void __init ns16550_init_postirq(
> >>                                      uart->ps_bdf[0], uart->ps_bdf[1],
> >>                                      uart->ps_bdf[2]);
> >>          }
> >> +
> >> +        if ( uart->msi )
> >> +        {
> >> +            struct msi_info msi = {
> >> +                .bus = uart->ps_bdf[0],
> >> +                .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
> >> +                .irq = rc = uart->irq,
> >> +                .entry_nr = 1
> >> +            };
> >> +
> >> +            if ( rc > 0 )
> >> +            {
> >> +                struct msi_desc *msi_desc = NULL;
> >> +
> >> +                pcidevs_lock();
> >> +
> >> +                rc = pci_enable_msi(&msi, &msi_desc);
> > 
> > Before attempting to enable MSI, shouldn't you make sure memory
> > decoding is enabled in case the device uses MSIX?
> > 
> > I think this code assumes the device will always use MSI? (in which
> > case my above question is likely moot).
> 
> I've yet to see serial cards using MSI-X. If we get to the point where
> this is needed, we also may need to first set up the BAR for the MSI-X
> table. Furthermore pci_enable_msi() won't even try to enable MSI-X
> with msi->table_base being zero.

Yes, that's how I figured out it was restricted to MSI because
table_base was always 0. Note sure if a comment to note this code is
not capable of handling MSIX would be helpful.

> >> --- a/xen/drivers/pci/pci.c
> >> +++ b/xen/drivers/pci/pci.c
> >> @@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
> >>      return 0;
> >>  }
> >>  
> >> +void pci_intx(const struct pci_dev *pdev, bool_t enable)
> > 
> > Please use bool.
> 
> See how old this patch is. V1 was posted long before bool came into
> existence, and I had refrained from posting v2 until I actually had a
> device where MSI would indeed function (the first two I tried this
> with claimed to be MSI capable, but no interrupts ever surfaced
> when MSI was enabled on them, yet I couldn't be sure the code
> was doing something wrong). Obviously I then forgot to switch this,
> which I've now done.

Thanks.

With the bool change:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I don't want to hold this any longer for the msi_capability_init
change, we can always switch to check device ownership in a later
patch.

Roger.

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

Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

[Roger Pau Monné]

On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
> Which, on x86, requires fiddling with the INTx bit in PCI config space,
> since for internally used MSI we can't delegate this to Dom0.
> 
> ns16550_init_postirq() also needs (benign) re-ordering of its
> operations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Re-base.
> 
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -307,7 +307,7 @@ Flag to indicate whether to probe for a
>  ACPI indicating none to be there.
>  
>  ### com1,com2
> -> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
> +> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
>  
>  Both option `com1` and `com2` follow the same format.
>  
> @@ -328,7 +328,7 @@ Both option `com1` and `com2` follow the
>  * `<io-base>` is an integer which specifies the IO base port for UART
>    registers.
>  * `<irq>` is the IRQ number to use, or `0` to use the UART in poll
> -  mode only.
> +  mode only, or `msi` to set up a Message Signaled Interrupt.
>  * `<port-bdf>` is the PCI location of the UART, in
>    `<bus>:<device>.<function>` notation.
>  * `<bridge-bdf>` is the PCI bridge behind which is the UART, in
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
>  
>      *desc = entry;
>      /* Restore the original MSI enabled bits  */
> +    if ( !hardware_domain )
> +    {
> +        /*
> +         * ..., except for internal requests (before Dom0 starts), in which
> +         * case we rather need to behave "normally", i.e. not follow the split
> +         * brain model where Dom0 actually enables MSI (and disables INTx).
> +         */
> +        pci_intx(dev, 0);
> +        control |= PCI_MSI_FLAGS_ENABLE;

Sorry for the split reply, I've been wondering about the MSI enabling
and INTX disabling done here. Xen already owns other PCI devices (AMD
IOMMU for example, see set_iommu_interrupt_handler) that use MSI, yet
they seem to manage to work without this by doing a manual MSI enable
(and I cannot figure out where the INTX disable is done).

Shouldn't Xen have a more uniform way of dealing with MSI interrupt
setup for such devices?

And doesn't your change here imply that some code from the current
internal MSI users should be dropped? There's a call to
__msi_set_enable in the AMD IOMMU code (amd_iommu_msi_enable) that I
guess can be dropped?

Thanks, Roger.

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

Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

[Jan Beulich]

>>> On 30.11.18 at 13:40, <roger.pau@citrix.com> wrote:
> On Fri, Nov 30, 2018 at 01:52:39AM -0700, Jan Beulich wrote:
>> >>> On 29.11.18 at 18:33, <roger.pau@citrix.com> wrote:
>> > On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
>> >> --- a/xen/arch/x86/msi.c
>> >> +++ b/xen/arch/x86/msi.c
>> >> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
>> >>  
>> >>      *desc = entry;
>> >>      /* Restore the original MSI enabled bits  */
>> >> +    if ( !hardware_domain )
>> > 
>> > Wouldn't it be better to assign the device to dom_xen (pdev->domain =
>> > dom_xen), and then check if the owner is dom_xen here?
>> 
>> I'm not sure this couldn't be wrong in the general case (and we
>> sit on a generic code path here): It depends on whether Dom0
>> can modify the device's config space, and I wouldn't want to
>> (here) introduce a connection between dom_xen ownership and
>> whether Dom0 can control INTX. The comment below here is
>> specifically worded to the effect of why I use hardware_domain
>> here.
> 
> Well, I think Dom0 shouldn't be allowed to interact with devices owned
> by dom_xen. That being set, at least the current vPCI code will allow
> PVH Dom0 to do so by passing through any accesses to registers not
> explicitly handled by vPCI.

Well, the r/o devices we expose don't match this desire of yours.
I also think that we ought to be very careful with completely hiding
devices from Dom0, as there may be implications between the
existence of devices (whether such implications are always
conceptually correct would be an orthogonal question).

>> If we ever get into the situation of wanting to enable MSI on an
>> internally used device _after_ Dom0 has started, this would need
>> careful re-considering.
> 
> OK, I'm fine with this. Maybe using system_state would be clearer to
> note that this code path is only to be used during early boot?

I don't think so, as a hardware domain cannot possibly fail to
exist at normal runtime. And system_state gets set to
SYS_STATE_active too late for my taste, while tying the logic
to SYS_STATE_smp_boot seems inappropriate to me.

>> > Or at the point where this is called from the serial console driver is
>> > too early for dom_xen to exist?
>> 
>> ns16550_init_postirq() is where both MSI setup and hiding of the
>> device happen, so in principle this would seem to be possible for
>> the specific case of a serial card.
> 
> IMO it's clear from a conceptual PoV to check against the ownership of
> the device rather than the system state at the point of the function
> call. Devices assigned to Dom0 use the split model, devices assigned
> to Xen don't.

Except aforementioned r/o devices, which Dom0 can see but not
(generally) fiddle with.

>> >> --- a/xen/drivers/pci/pci.c
>> >> +++ b/xen/drivers/pci/pci.c
>> >> @@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
>> >>      return 0;
>> >>  }
>> >>  
>> >> +void pci_intx(const struct pci_dev *pdev, bool_t enable)
>> > 
>> > Please use bool.
>> 
>> See how old this patch is. V1 was posted long before bool came into
>> existence, and I had refrained from posting v2 until I actually had a
>> device where MSI would indeed function (the first two I tried this
>> with claimed to be MSI capable, but no interrupts ever surfaced
>> when MSI was enabled on them, yet I couldn't be sure the code
>> was doing something wrong). Obviously I then forgot to switch this,
>> which I've now done.
> 
> Thanks.
> 
> With the bool change:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks!

Jan


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

Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

[Jan Beulich]

>>> On 30.11.18 at 17:33, <roger.pau@citrix.com> wrote:
> On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
>>  
>>      *desc = entry;
>>      /* Restore the original MSI enabled bits  */
>> +    if ( !hardware_domain )
>> +    {
>> +        /*
>> +         * ..., except for internal requests (before Dom0 starts), in which
>> +         * case we rather need to behave "normally", i.e. not follow the split
>> +         * brain model where Dom0 actually enables MSI (and disables INTx).
>> +         */
>> +        pci_intx(dev, 0);
>> +        control |= PCI_MSI_FLAGS_ENABLE;
> 
> Sorry for the split reply, I've been wondering about the MSI enabling
> and INTX disabling done here. Xen already owns other PCI devices (AMD
> IOMMU for example, see set_iommu_interrupt_handler) that use MSI, yet
> they seem to manage to work without this by doing a manual MSI enable
> (and I cannot figure out where the INTX disable is done).

That's because IOMMUs don't normally have a means to signal
interrupts via a pin. Hence there's nothing to disable.

> Shouldn't Xen have a more uniform way of dealing with MSI interrupt
> setup for such devices?

Perhaps, but the crude way of setting up IOMMU interrupts was
invented by the CPU vendor engineers; on the AMD side later I
then re-worked this to re-use at least some of the generic MSI
code we have.

> And doesn't your change here imply that some code from the current
> internal MSI users should be dropped? There's a call to
> __msi_set_enable in the AMD IOMMU code (amd_iommu_msi_enable) that I
> guess can be dropped?

Quite possible (I admit I didn't check in detail), but here I didn't
want to fiddle with unrelated code.

Jan



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

Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

[Roger Pau Monné]

On Mon, Dec 03, 2018 at 01:40:06AM -0700, Jan Beulich wrote:
> >>> On 30.11.18 at 17:33, <roger.pau@citrix.com> wrote:
> > On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
> >> --- a/xen/arch/x86/msi.c
> >> +++ b/xen/arch/x86/msi.c
> >> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
> >>  
> >>      *desc = entry;
> >>      /* Restore the original MSI enabled bits  */
> >> +    if ( !hardware_domain )
> >> +    {
> >> +        /*
> >> +         * ..., except for internal requests (before Dom0 starts), in which
> >> +         * case we rather need to behave "normally", i.e. not follow the split
> >> +         * brain model where Dom0 actually enables MSI (and disables INTx).
> >> +         */
> >> +        pci_intx(dev, 0);
> >> +        control |= PCI_MSI_FLAGS_ENABLE;
> > 
> > Sorry for the split reply, I've been wondering about the MSI enabling
> > and INTX disabling done here. Xen already owns other PCI devices (AMD
> > IOMMU for example, see set_iommu_interrupt_handler) that use MSI, yet
> > they seem to manage to work without this by doing a manual MSI enable
> > (and I cannot figure out where the INTX disable is done).
> 
> That's because IOMMUs don't normally have a means to signal
> interrupts via a pin. Hence there's nothing to disable.
> 
> > Shouldn't Xen have a more uniform way of dealing with MSI interrupt
> > setup for such devices?
> 
> Perhaps, but the crude way of setting up IOMMU interrupts was
> invented by the CPU vendor engineers; on the AMD side later I
> then re-worked this to re-use at least some of the generic MSI
> code we have.
> 
> > And doesn't your change here imply that some code from the current
> > internal MSI users should be dropped? There's a call to
> > __msi_set_enable in the AMD IOMMU code (amd_iommu_msi_enable) that I
> > guess can be dropped?
> 
> Quite possible (I admit I didn't check in detail), but here I didn't
> want to fiddle with unrelated code.

Ack, I guess it's something to add to the list of nice-to-have.

Thanks, Roger.

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