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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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.

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