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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.11.18 at 18:33, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel