[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4] vpci/msix: fix PBA accesses
On 08.03.2022 10:05, Roger Pau Monné wrote: > On Tue, Mar 08, 2022 at 09:31:34AM +0100, Jan Beulich wrote: >> On 07.03.2022 17:37, Roger Pau Monne wrote: >>> Map the PBA in order to access it from the MSI-X read and write >>> handlers. Note that previously the handlers would pass the physical >>> host address into the {read,write}{l,q} handlers, which is wrong as >>> those expect a linear address. >>> >>> Map the PBA using ioremap when the first access is performed. Note >>> that 32bit arches might want to abstract the call to ioremap into a >>> vPCI arch handler, so they can use a fixmap range to map the PBA. >>> >>> Reported-by: Jan Beulich <jbeulich@xxxxxxxx> >>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> >> >> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> >> >>> Cc: Alex Olson <this.is.a0lson@xxxxxxxxx> >> >> I'll wait a little with committing, in the hope for Alex to re-provide >> a Tested-by. >> >>> --- a/xen/drivers/vpci/msix.c >>> +++ b/xen/drivers/vpci/msix.c >>> @@ -182,6 +182,38 @@ static struct vpci_msix_entry *get_entry(struct >>> vpci_msix *msix, >>> return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE]; >>> } >>> >>> +static void __iomem *get_pba(struct vpci *vpci) >>> +{ >>> + struct vpci_msix *msix = vpci->msix; >>> + /* >>> + * PBA will only be unmapped when the device is deassigned, so access >>> it >>> + * without holding the vpci lock. >>> + */ >>> + void __iomem *pba = read_atomic(&msix->pba); >>> + >>> + if ( likely(pba) ) >>> + return pba; >>> + >>> + pba = ioremap(vmsix_table_addr(vpci, VPCI_MSIX_PBA), >>> + vmsix_table_size(vpci, VPCI_MSIX_PBA)); >>> + if ( !pba ) >>> + return read_atomic(&msix->pba); >>> + >>> + spin_lock(&vpci->lock); >>> + if ( !msix->pba ) >>> + { >>> + write_atomic(&msix->pba, pba); >>> + spin_unlock(&vpci->lock); >>> + } >>> + else >>> + { >>> + spin_unlock(&vpci->lock); >>> + iounmap(pba); >>> + } >> >> TBH I had been hoping for just a single spin_unlock(), but you're >> the maintainer of this code ... > > Would you prefer something like: > > spin_lock(&vpci->lock); > if ( !msix->pba ) > write_atomic(&msix->pba, pba); > spin_unlock(&vpci->lock); > > if ( read_atomic(&msix->pba) != pba ) > iounmap(pba); This or (to avoid the re-read) spin_lock(&vpci->lock); if ( !msix->pba ) { write_atomic(&msix->pba, pba); pba = NULL; } spin_unlock(&vpci->lock); if ( pba ) iounmap(pba); Iirc we have similar constructs elsewhere in a few places. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |