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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.18 at 16:07, <roger.pau@xxxxxxxxxx> wrote:
> +void 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);
> +    }

In case you have 2k entries to print, you surely want to make sure
events get processed every once in a while.

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

> @@ -293,8 +298,7 @@ void vpci_dump_msi(void)
>  
>              if ( msi )
>              {
> -                printk("%04x:%02x:%02x.%u\n", pdev->seg, pdev->bus,
> -                       PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +                printk(" MSI\n");

For this as well as ...

> @@ -306,6 +310,16 @@ void vpci_dump_msi(void)
>                  vpci_msi_arch_print(msi);
>              }
>  
> +            if ( msix )
> +            {
> +                printk(" MSI-X\n");

... this: Can we please avoid logging such short (and meaningless
on their own) lines? MSI and MSI-X shouldn't be enabled at the
same time, so what's wrong with adding this to the line with the
device SBDF?

> +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.

Jan


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