|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/2] ns16550: enable use of PCI MSI
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@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |