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

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

On Wed, Feb 28, 2018 at 07:33:55AM -0700, Jan Beulich wrote:
> >>> On 23.01.18 at 16:07, <roger.pau@xxxxxxxxxx> wrote:
> > @@ -255,6 +256,23 @@ static void modify_bars(const struct pci_dev *pdev, 
> > bool map, bool rom)
> >          }
> >      }
> >  
> > +    /* 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_UP(end));
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "Failed to remove MSIX table [%" PRI_gfn ", %" PRI_gfn 
> > "): %d\n",
> > +                   PFN_DOWN(start), PFN_UP(end), rc);
> > +            rangeset_destroy(mem);
> > +            return;
> 
> On v7 I had said "Silent discarding of an error also needs an
> explanation in a comment." You've added a printk() instead of a
> comment; the discarding of the error remains silent that way
> from the perspective of the caller (it's only verbose as far as the
> admin goes), and still provides no explanation.

Right, cmd_write and rom_write don't really care about the error
because there's nothing those functions can do about it.

However init_bars cares about the error, in which case this function
should return and the error should be propagated to the caller of
init_bars.

> > +static int update_entry(struct vpci_msix_entry *entry,
> > +                        const struct pci_dev *pdev, unsigned int nr)
> > +{
> > +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> > +    int rc = vpci_msix_arch_disable_entry(entry, pdev);
> > +
> > +    /* Ignore ENOENT, it means the entry wasn't setup. */
> > +    if ( rc && rc != -ENOENT )
> > +    {
> > +        gprintk(XENLOG_WARNING,
> > +                "%04x:%02x:%02x.%u: unable to disable entry %u for update: 
> > %d\n",
> > +                pdev->seg, pdev->bus, slot, func, nr, rc);
> > +        return rc;
> > +    }
> > +
> > +    rc = vpci_msix_arch_enable_entry(entry, pdev,
> > +                                     vmsix_table_base(pdev->vpci,
> > +                                                      VPCI_MSIX_TABLE));
> > +    if ( rc )
> > +    {
> > +        gprintk(XENLOG_WARNING,
> > +                "%04x:%02x:%02x.%u: unable to enable entry %u: %d\n",
> > +                pdev->seg, pdev->bus, slot, func, nr, rc);
> > +        /* Entry is likely not properly configured, skip it. */
> > +        return rc;
> 
> The "skip" part of the comment isn't really applicable here.

Correct. This code was probably moved and the comment is now stall.

Thanks, Roger.

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