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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 8 Mar 2022 13:37:20 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=q3PjM7YjxFPOmv6OkEMZsIGB/W/MEnG0q9AmT8FGzjE=; b=S+fQcbCBjgq9x2pTIgSaZi4aGf48msugbmTdrbmq5XlsZU7SowWeYGlPJ36wLk+I0psjbpKi2gt74NvgWb1KJd25m0+CJs5MFWTf7eeiGUnIslv3OlHkdaS1vHcDd4QKup2ji4Eg4m2coJYtEvi0fxqhREcQZ2hqejmiwnhSTal2Lux0QAoPCcLamJI6ji3kEnGmigAa+MpGdBltlbRK85XhB6/TGpVXRnrKmIaH2nvhwgBWEXNpZMYSqrAcHdRAObIharChNnM4GstAtkHJbq9XjNdadHJNxVBiQiuivfZdFtmUlAkXxwHqFeyGLH3a3hLT1PG2UMgih6o35TwKkA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=IxnVvxQQ1507b7j/NXDe7kalrvvxrp0+ZfvV6e2/ux53HF7IR2qktU04uDqbO0kBjK9n/4mLShQpwGjF//CWaRgLkw4x0EWXwBcUEuwPju/qk9HcV708Etnjo7GNOH/VwaKgF+vC3Y9YxqiFPLy8b97pE0o/jmUwUEeGpOTm7jPzBJbkrJZZmGCPezREFdo7gzRbKXcOKFqiHUTZMktNpcWcSErJkhBhI1fuaeo5V1Ej/6GmiUA3LF2l/JnHgSwDabxJ+iLmgouswr0vX4prDKv99bV11taKd7J6YDaDqmob8nCZYEJmnVZyqmdFWDRYh9a3NSCUTZexFOFypCTBYw==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Alex Olson <this.is.a0lson@xxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 08 Mar 2022 12:37:34 +0000
  • Ironport-data: A9a23:FvsUY67MYGqHOVbWRajErAxRtBzHchMFZxGqfqrLsTDasY5as4F+v mZLDW2BbvqPNGLzLY0gOoS38RkOuJfXndQwQQNk/3pkHi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuVGuG96yE6j8lkf5KkYAL+EnkZqTRMFWFw0XqPp8Zj2tQy2YPhU1vU0 T/Pi5a31GGNimYc3l08s8pvmDs31BglkGpF1rCWTakjUG72zxH5PrpGTU2CByKQrr1vNvy7X 47+IISRpQs1yfuP5uSNyd4XemVSKlLb0JPnZnB+A8BOiTAazsA+PzpS2FPxpi67hh3Q9+2dx umhurSvZgsFZ43iqt0HcEBSTzp+M58YyLrudC3XXcy7lyUqclPpyvRqSko3IZcZ6qB8BmQmG f4wcW5XKErZ3qTvnez9GrIEascLdaEHOKsFvX5t13fBBOsOSpHfWaTao9Rf2V/cg+gQQK+BO ZZFOFKDajyZUhldFw80Jqthv83xlHv0KjgFl3W88P9fD2/7k1UqjemF3MDuUsyHQ4BZk1iVo krC/n/lGVcKOdqH0z2H/3mwwOjVkkvTRIITD/u57bhj2FmIwWo7BxgfVF/9qv684ma8Ud9CL 00f+gI1sLM/skesS7HAswaQ+SDe+ERGApwJTrN8uFrlJrfoDxixGUcIYmdhRu4fruxvT2Ax1 lG7wePsLGk62FGKck61+rCRpDK0HCEaK24eeCMJJTc4D8nfTJIb1UyWEIs6eEKhppisQGyrn WjWxMQrr+hL1aY2O7OHEUcrat5GjrzAVUYL6wreRQpJBSspNdf+N+REBbU2hMuszbp1rHHc5 BDoeODEtYji6K1hcgTXGI3h+5nzu5643MX02wIHInXY323FF4SfVY5R+ipiA0xiL9wJfzTkC GeK518PusADYSDzPfIsC25UNyjM5fK7fTgCfqqIBuein7ArLFPXlM2QTRT4M5/RfLgEzvhkZ MbznTeEBncGE6V3pAdatM9GuYLHMhsWnDuJLbiilkzP+ePHOBa9FOdUWHPTP7tRxP7V/23oH yN3apLiJ+N3C7alPEE6MOc7cDg3EJTMLcyu+5wNKL/ZeVYO9aNII6a5/I7NsrdNxsx9vuzJ4 mu8Sglfzl/+jmfAMgKEdjZob7aHYHq1hStT0fAEVbpw50UeXA==
  • Ironport-hdrordr: A9a23:nYfhK64OMw0qobQi9gPXwSqBI+orL9Y04lQ7vn2ZFiY7TiXIra yTdaoguCMc6AxxZJkh8erwX5VoZUmsj6KdhrNhQItKPTOWw1dASbsN0WKM+UyDJ8STzJ856U 4kSdkDNDSSNykKsS+Z2njALz9I+rDum8rJ9ITjJjVWPHlXgslbnnlE422gYytLrWd9dP4E/M 323Ls5m9PsQwVeUu2LQl0+G8TTrdzCk5zrJTYAGh4c8QGLyRel8qTzHRS01goXF2on+8ZpzU H11yjCoomzufCyzRHRk0fV8pRtgdPkjv9OHtaFhMQ5IijlziyoeINicbufuy1dmpDl1H8a1P 335zswNcV67H3cOkmzvBvWwgHllA0j7nfzoGXo9kfLkIjcfnYXGsBBjYVWfl/y8Ew7puxx16 pNwiawq4dXJQmoplWz2/H4EzVR0makq3srluAey1ZFV5EFVbNXpYsDuGtIDZY7Gj7g4oxPKp gjMCjl3ocWTbqmVQGYgoE2q+bcHUjbXy32D3Tqg/blnQS/xxtCvgklLM92pAZ1yHtycegA2w 3+CNUaqFh/dL5nUUtDPpZyfSKWMB26ffueChPaHbzYfJt3Tk4l7aSHpIkI2A==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Mar 08, 2022 at 11:46:20AM +0100, Jan Beulich wrote:
> 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.

I don't really have a strong opinion. I agree the duplicated
spin_unlock() call is not nice, but I think the logic is easier to
follow by using a single if ... else ... section.

Feel free to adjust at commit, or else I can resend if you prefer.

Thanks, Roger.



 


Rackspace

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