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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.12.2020 18:58, Roger Pau Monné wrote:
> 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>

Thanks.

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

Indeed, I'd prefer to not make such a change right here.

Jan