Re: [PATCH 5/5] vPCI/MSI-X: tidy init_msix()

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

On Mon, Dec 07, 2020 at 11:38:42AM +0100, Jan Beulich wrote:
> First of all introduce a local variable for the to be allocated struct.
> The compiler can't CSE all the occurrences (I'm observing 80 bytes of
> code saved with gcc 10). Additionally, while the caller can cope and
> there was no memory leak, globally "announce" the struct only once done
> initializing it. This also removes the dependency of the function on
> the caller cleaning up after it in case of an error.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just a couple of comments.

> ---
> I was heavily tempted to also move up the call to vpci_add_register(),
> such that there would be no pointless init done in case of an error
> coming back from there.

Feel free to do so.

> 
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -436,6 +436,7 @@ static int init_msix(struct pci_dev *pde
>      uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
>      unsigned int msix_offset, i, max_entries;
>      uint16_t control;
> +    struct vpci_msix *msix;
>      int rc;
>  
>      msix_offset = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
> @@ -447,34 +448,37 @@ static int init_msix(struct pci_dev *pde
>  
>      max_entries = msix_table_size(control);
>  
> -    pdev->vpci->msix = xzalloc_flex_struct(struct vpci_msix, entries,
> -                                           max_entries);
> -    if ( !pdev->vpci->msix )
> +    msix = xzalloc_flex_struct(struct vpci_msix, entries, max_entries);
> +    if ( !msix )
>          return -ENOMEM;
>  
> -    pdev->vpci->msix->max_entries = max_entries;
> -    pdev->vpci->msix->pdev = pdev;
> +    msix->max_entries = max_entries;
> +    msix->pdev = pdev;
>  
> -    pdev->vpci->msix->tables[VPCI_MSIX_TABLE] =
> +    msix->tables[VPCI_MSIX_TABLE] =
>          pci_conf_read32(pdev->sbdf, msix_table_offset_reg(msix_offset));
> -    pdev->vpci->msix->tables[VPCI_MSIX_PBA] =
> +    msix->tables[VPCI_MSIX_PBA] =
>          pci_conf_read32(pdev->sbdf, msix_pba_offset_reg(msix_offset));
>  
> -    for ( i = 0; i < pdev->vpci->msix->max_entries; i++)
> +    for ( i = 0; i < msix->max_entries; i++)

Feel free to just use max_entries directly here.

>      {
> -        pdev->vpci->msix->entries[i].masked = true;
> -        vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
> +        msix->entries[i].masked = true;

I think we should also set msix->entries[i].updated = true; for
correctness? Albeit this will never lead to a working configuration,
as the address field will be 0 and thus cause and error to trigger if
enabled without prior setup.

Maybe on a different patch anyway.

Thanks, Roger.