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

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

On Fri, Nov 30, 2018 at 01:52:39AM -0700, Jan Beulich wrote:
> >>> 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.

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@xxxxxxxxxx>

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