[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 5/5] vPCI/MSI-X: tidy init_msix()
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |