Re: [Xen-devel] [PATCH v2 04/11] vpci/msix: add teardown cleanup

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.18 at 11:48, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -436,11 +436,6 @@ static int init_msix(struct pci_dev *pdev)
>          vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
>      }
>  
> -    rc = vpci_add_register(pdev->vpci, control_read, control_write,
> -                           msix_control_reg(msix_offset), 2, 
> pdev->vpci->msix);
> -    if ( rc )
> -        return rc;
> -
>      write_lock(&d->arch.hvm_domain.msix_lock);
>      if ( list_empty(&d->arch.hvm_domain.msix_tables) )
>          register_mmio_handler(d, &vpci_msix_table_ops);
> @@ -448,9 +443,57 @@ static int init_msix(struct pci_dev *pdev)
>      list_add(&pdev->vpci->msix->next, &d->arch.hvm_domain.msix_tables);
>      write_unlock(&d->arch.hvm_domain.msix_lock);
>  
> +    rc = vpci_add_register(pdev->vpci, control_read, control_write,
> +                           msix_control_reg(msix_offset), 2, 
> pdev->vpci->msix);

Without the description saying why, I can't figure or guess why
this wants/needs moving.

> +    if ( rc )
> +        /* The teardown function will free the msix struct. */
> +        return rc;
> +
>      return 0;

This can be a single return statement now, without even a need
for going through rc.

> +static void teardown_msix(struct pci_dev *pdev)
> +{
> +    struct vpci_msix *msix = pdev->vpci->msix;
> +    unsigned int i, pos;
> +    uint16_t control;
> +
> +    if ( !msix )
> +        return;
> +
> +    write_lock(&pdev->domain->arch.hvm_domain.msix_lock);
> +    list_del(&pdev->vpci->msix->next);
> +    write_unlock(&pdev->domain->arch.hvm_domain.msix_lock);
> +
> +    if ( !msix->enabled )
> +        goto out;
> +
> +    /* Disable MSIX. */
> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
> +    ASSERT(pos);
> +    control = pci_conf_read16(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), msix_control_reg(pos));
> +    pci_conf_write16(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                     PCI_FUNC(pdev->devfn), msix_control_reg(pos),
> +                     (control & ~PCI_MSIX_FLAGS_ENABLE));

To avoid subsequent surprises, perhaps better also clear
PCI_MSIX_FLAGS_MASKALL?

> +    for ( i = 0; i < msix->max_entries; i++ )
> +    {
> +        int rc = vpci_msix_arch_disable_entry(&msix->entries[i], pdev);
> +
> +        if ( rc && rc != -ENOENT )
> +            gprintk(XENLOG_WARNING,
> +                    "%04x:%02x:%02x.%u: unable to disable MSIX entry %u: 
> %d\n",
> +                    pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                    PCI_FUNC(pdev->devfn), i, rc);
> +    }
> +
> +out:

Labels indented by at least one blank please.

> +    xfree(msix);
> +    pdev->vpci->msix = NULL;

Perhaps better to zap the field before freeing the structure?

Jan



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