Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers

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

On Tue, Mar 20, 2018 at 05:03:09AM -0600, Jan Beulich wrote:
> >>> On 16.03.18 at 14:30, <roger.pau@xxxxxxxxxx> wrote:
> > +int vpci_msix_arch_print(const struct vpci_msix *msix)
> > +{
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < msix->max_entries; i++ )
> > +    {
> > +        const struct vpci_msix_entry *entry = &msix->entries[i];
> > +
> > +        printk("%6u vec=%02x%7s%6s%3sassert%5s%7s dest_id=%lu mask=%u 
> > pirq: %d\n",
> > +               i, MASK_EXTR(entry->data, MSI_DATA_VECTOR_MASK),
> > +               entry->data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
> > +               entry->data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
> > +               entry->data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
> > +               entry->addr & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
> > +               entry->addr & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" : 
> > "fixed",
> > +               MASK_EXTR(entry->addr, MSI_ADDR_DEST_ID_MASK),
> > +               entry->masked, entry->arch.pirq);
> > +        if ( i && !(i % 64) )
> > +        {
> > +            struct pci_dev *pdev = msix->pdev;
> > +
> > +            spin_unlock(&msix->pdev->vpci->lock);
> > +            process_pending_softirqs();
> > +            /* NB: we assume that pdev cannot go away for an alive domain. 
> > */
> > +            if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) )
> > +                return -EBUSY;
> > +            msix = pdev->vpci->msix;
> 
> I disagree with resuming with a potentially changed msix here: This
> can only lead to confusion of the consumer of the produced output.

OK, I will check that the previous msix pointer matches the current
one.

> > @@ -231,6 +232,23 @@ static int modify_bars(const struct pci_dev *pdev, 
> > bool map, bool rom_only)
> >          }
> >      }
> >  
> > +    /* Remove any MSIX regions if present. */
> > +    for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> > +    {
> > +        paddr_t start = vmsix_table_addr(pdev->vpci, i);
> > +        paddr_t end = start + vmsix_table_size(pdev->vpci, i) - 1;
> > +
> > +        rc = rangeset_remove_range(mem, PFN_DOWN(start), PFN_DOWN(end));
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "Failed to remove MSIX table [%" PRI_gfn ", %" PRI_gfn 
> > "]: %d\n",
> > +                   PFN_DOWN(start), PFN_DOWN(end), rc);
> 
> In cases like this (where you don't use plain start/end anywhere,
> but you do use the same calculation on them twice each), it's
> certainly more efficient for the local variables to be frame numbers
> right away.

Right, I've fixed it and also changed the printf formatters to lu, I
guess at some point in the series I used to print gfn values.

> 
> Considering that I didn't notice this earlier, I won't insist on the
> latter change to be made, i.e. with at least the former issue
> addressed
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks. FWIW I've fixed both your comments.

Since you only had comments on patch 7 and 11 and there's the extra
fix for the test harness, should I just send those and provide you
with a git branch that contains the rest?

I can also wait if you want to commit the start of the series
(provided I get all the relevant Acks) and rebase on top of that.

Roger.

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