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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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>

Jan


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