[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4] vpci/msix: fix PBA accesses


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 8 Mar 2022 11:46:20 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=evRIC5qTSl1HD3cmRzPh9aztqVRrw9cv5kUjLlqScTg=; b=Ly2GyA1C+j5d6MZRzlj9cj0RO6yXn1Fa6LtAXl8HjJ46+oCR9M3Hltd9WPwuRJdpO2BC6cZgNeGYNMoFvq7KTjcnqbZSYIxPe67Wtk1d72z47I2yp2CT00zp5le2MWgQp+dlBeUFzxaI+Ywcw9YMoX1Ncfm/9m36BVfF+u1YOl1TSCedGB27fioZHy0i3CPPUFiIUSp35k2FCrQb7G0BAF6TRtBQu8g+IWI87/mHqyDBhEHb2r7QLaadlahQccs665o+G5aFGWWwKvxdnBbOFe0REG9m/84jOrpPvApycCbaZxzWuO9eyyyrVs7Lpu/G+3YesdzzTgPMaqM7VLIeUQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=F5FxWul2jJEoUBwP9Mbvr+iKLkX4zAIM1G7Ay8pdFVMl3xM17BlnPEFqRYSn/32fWN9/+ymtwYP8um3VH854QO+4MECibVhUGUahcKg++H0hLGD2n8nsq4zvbp+CWF32TvaQiU2s1BgWe0UPgDYoOpMjbvqD5YkVpSxS61AYHTQ9B03AdUySGS73jX/96JznUJ77roVc9Ae2uF7rDxS0sBtbM9of4SEbHp9sKkNgtAjYJ0dhepCBXKFelXNaN2mOxRqXhkNdt+WUWTsNfCCFQZfQYN4JRdzh9jFN2aQgkYLoQbTvzSazR34Il5mQoHGGmhDRfGy6vBhc9pHgFmRm+w==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Alex Olson <this.is.a0lson@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 08 Mar 2022 10:46:43 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.