Re: [Xen-devel] [Xen-users] ARM: "xen_add_mach_to_phys_entry: cannot add ... already exists and panics"

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 4 Jul 2014, David Vrabel wrote:
> On 04/07/14 17:36, David Vrabel wrote:
> > On 04/07/14 15:31, Stefano Stabellini wrote:
> >> On Fri, 4 Jul 2014, David Vrabel wrote:
> >>> On 04/07/14 15:12, Stefano Stabellini wrote:
> >>>> On Fri, 4 Jul 2014, David Vrabel wrote:
> >>>>> On 03/07/14 18:47, Stefano Stabellini wrote:
> >>>>>>
> >>>>>> At the moment I would like a way to disable grant mappings and go back
> >>>>>> to grant copies on demand. Maybe we could have a feature flag to change
> >>>>>> the behaviour of the network backend?
> >>>>>
> >>>>> You must fix the ARM code to handle this properly.
> >>>>>
> >>>>> blkback also uses grant maps and depending on the frontend blkback may
> >>>>> grant map the same machine frame multiple time.  We have see frontends
> >>>>> that send such requests.
> >>>>
> >>>> Indeed, it must be fixed properly. The workaround of disabling grant
> >>>> mappings would be just to buy us some time to come up with the fix.
> >>>
> >>> It's an expensive workaround though.
> >>
> >> In terms of performances or in terms of code?
> >> If it's the performances you are worried about, we could enable the
> >> workaround just for arm and arm64.
> > 
> > Expensive in terms of effort required to implement and maintain.
> > 
> >>>>> I can't remember how the ARM code works.  Where is the problematic m2p
> >>>>> lookup?
> >>>>
> >>>> arch/arm/xen/p2m.c
> >>>>
> >>>>
> >>>>> Perhaps this could be avoided for foreign frames?
> >>>>
> >>>> Unfortunately no. The whole point of p2m.c is to be able to translate
> >>>> foreign frames for dma operations.
> >>>
> >>> This is a p2m lookup though, which is fine, yes?  Where, specifically is
> >>> a mfn_to_pfn() lookup needed for a foreign MFN.
> >>  
> >> drivers/xen/swiotlb-xen.c:xen_unmap_single. xen_bus_to_phys is based on
> >> the value returned by mfn_to_pfn.
> > 
> > I think you can probably get away with not doing the cache operations on
> > foreign pages when DMA map/unmapping.  DMA mapped foreign pages are
> > (currently) either:
> > 
> > a) packets from a guest being Tx'd off host.  These are read-only and
> > are immediately freed after they're tranmitted.
> > 
> > b) block requests from blkback and these pages are never accessed.

Unfortunately it doesn't look like it is possible to skip the call to
__dma_page_dev_to_cpu
(xen_dma_unmap_page->arm_dma_unmap_page->__dma_page_dev_to_cpu), because
otherwise the data written by DMA to system memory won't be visible to
the CPU, at least on ARM32.

In fact the grant_unmap operation might issue flushes, but they are
inner flushes. While here we would need to issue an outer flush at the
very least. In addition to it, __dma_page_dev_to_cpu also calls
dmac_unmap_area, that on v7 becomes v7_dma_inv_range that issues a bunch
of DCCIMVAC.

So in order for this to work on grant_unmap Xen would have to:

- issue an outer flush
- DCCIMVAC the page

Also not all devices need this, only the non-coherent ones.

Given that we assume that Dom0 is mapped 1:1, one way to solve this
would be for Dom0 to map the grant on PFN=MFN too, then issue all the
required flushed on the machine address instead of the physical address.



> Something like:
> 
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -90,17 +90,18 @@ static inline dma_addr_t xen_phys_to_bus(phys_addr_t 
> paddr)
>       return dma;
>  }
>  
> -static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
> +/* Return true if baddr is the address of a foreign frame. */
> +static inline int xen_bus_to_phys(dma_addr_t baddr, phys_addr_t *paddr)
>  {
>       unsigned long pfn = mfn_to_pfn(PFN_DOWN(baddr));
>       dma_addr_t dma = (dma_addr_t)pfn << PAGE_SHIFT;
> -     phys_addr_t paddr = dma;
>  
> -     BUG_ON(paddr != dma); /* truncation has occurred, should never happen */
> +     if (pfn == INVALID_P2M_ENTRY)
> +             return true;
>  
> -     paddr |= baddr & ~PAGE_MASK;
> +     *paddr = dma | (baddr & ~PAGE_MASK);
>  
> -     return paddr;
> +     return false;
>  }
>  
>  static inline dma_addr_t xen_virt_to_bus(void *address)
> @@ -443,10 +444,30 @@ static void xen_unmap_single(struct device *hwdev, 
> dma_addr_t dev_addr,
>                            size_t size, enum dma_data_direction dir,
>                                struct dma_attrs *attrs)
>  {
> -     phys_addr_t paddr = xen_bus_to_phys(dev_addr);
> +     phys_addr_t paddr;
> +     bool is_foreign;
>  
>       BUG_ON(dir == DMA_NONE);
>  
> +     is_foreign = xen_bus_to_phys(dev_addr, &paddr);
> +
> +     /*
> +      * We can't get the appropriate PFN for a foreign frame since
> +      * it may be grant mapped multiple times.
> +      *
> +      * Assume that the grant unmap will do any appropriate cache
> +      * operations, and that the frontend will do any for its own
> +      * mappings.
> +      *
> +      * This does mean there is a window between the DMA unmap and
> +      * the grant unmap where the CPU may see stale data (for a
> +      * FROM_DEVICE operation), but this is not a problem in
> +      * practice with the current users of foreign mappings
> +      * (netback and blkback).
> +      */
> +     if (is_foreign)
> +             return;
> +
>       xen_dma_unmap_page(hwdev, paddr, size, dir, attrs);
>  
>       /* NOTE: We use dev_addr here, not paddr! */
> 

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