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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 )
> +    {
> +        /*
> +         * ..., 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel