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 <jbeul...@suse.com>
Reviewed-by: Roger Pau Monné <roger....@citrix.com>
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.
