Re: [Xen-devel] [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-01-17 at 18:09 +0000, Stefano Stabellini wrote:
> On Fri, 17 Jan 2014, Ian Campbell wrote:
> > The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
> > causes us to lose the top bits of the DMA address if the size of a DMA 
> > address is not the same as the size of the phyiscal address.
> > 
> > This can happen in practice on ARM where foreign pages can be above 4GB even
> > though the local kernel does not have LPAE page tables enabled (which is
> > totally reasonable if the guest does not itself have >4GB of RAM). In this
> > case the kernel still maps the foreign pages at a phys addr below 4G (as it
> > must) but the resulting DMA address (returned by the grant map operation) is
> > much higher.
> 
> We are lucky that LPAE only supports 40 bits otherwise we would need to
> change any other functions that use unsigned long for mfn, starting from
> set_phys_to_machine.

Yes, the use of unsigned long for pfn is a larger problem for the kernel
on any 32-bit arch with LPAE type extensions, I think.

> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 1eac073..b626c79 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -77,12 +77,22 @@ static u64 start_dma_addr;
> >  
> >  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
> >  {
> > -   return phys_to_machine(XPADDR(paddr)).maddr;
> > +   unsigned long mfn = pfn_to_mfn(PFN_DOWN(paddr));
> > +   dma_addr_t dma = (dma_addr_t)mfn << PAGE_SHIFT;
> 
> I'd add this comment:
> 
> /* avoid PFN_PHYS because phys_addr_t can be 32bit when dma_addr_t is
>  * 64bit leading to a loss in information if the shift is done before
>  * casting to 64bit. */
> 
> > +   dma |= paddr & ~PAGE_MASK;
> > +   return dma;
> >  }
> >  
> >  static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
> >  {
> > -   return machine_to_phys(XMADDR(baddr)).paddr;
> > +   dma_addr_t dma = PFN_PHYS(mfn_to_pfn(PFN_DOWN(baddr)));
> > +   phys_addr_t paddr = dma;
> > +
> > +   BUG_ON(paddr != dma); /* truncation has occurred, should never happen */
> 
> This check is useless because PFN_PHYS contains a cast to (phys_addr_t)
> that is 32 bit. I think you'll have to:
> 
> dma_addr_t dma = ((dma_addr_t)mfn_to_pfn(PFN_DOWN(baddr))) << PAGE_SHIFT;

That's what I originally had, then I spotted PFN_PHYS and decided it
would be a nice cleanup -- oops!

v2 coming up.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel