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

[David Vrabel <david.vrabel@citrix.com>]

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! */