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

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

[Stefano Stabellini]

On Thu, 3 Jul 2014, Ian Campbell wrote:
> On Thu, 2014-07-03 at 02:01 +0200, Denis Schneider wrote:
> > Hi an,
> > thank you for your reply.
> > 
> > 2014-07-02 11:56 GMT+02:00 Ian Campbell <Ian.Campbell@citrix.com>:
> > > First thing I would recommend would be to try the latest mainline stable
> > > 3.15.x release. I think everything needed for a usable sunxi system is
> > > in there already so no need for the sunxi-devel branch
> > 
> > I tried Linus' linux.git/master, which corresponds to 3.16 --
> > resulting in the same messages and panic.
> > Besides that, the mainline kernel works quite well.
> > BTW, git shows that sunxi-devel and mainline Linux v3.15.2 are the
> > same for drivers/net/xen-netback, though linux.git/master shows some
> > changes.
> > 
> > The bug can easily be triggered if you access blkback and netback in
> > parallel (thanks to Maximilian), e.g.
> > domU: iperf -s & cat /dev/xvda > /dev/null
> > dom0: iperf -t 3600 -c domU
> > 
> > It does not matter if the underlying dom0 block device is on a SATA,
> > USB or mmc device. The panic is similar.
> > 
> > > The reason I suggest the latest 3.15.x is that there were a few
> > > interesting netback bugs but I think they've all been backported to
> > > stable by now.
> > 
> > I hope that they are all included in linux.git/master @ 16874b2,
> > regarding xen-netback, those changes occurred from sunxi-devel to
> > 16874b2:
> > * xen-netback: bookkeep number of active queues in our own module
> > * net: xen-netback: include linux/vmalloc.h again
> > * xen-netback: Add support for multiple queues
> > * xen-netback: Factor queue-specific data into queue struct
> > * xen-netback: Move grant_copy_op array back into struct xenvif.
> > * net: get rid of SET_ETHTOOL_OPS
> > 
> > Interestingly, it takes some time until the bug triggers and the time
> > increased when I switched from linux-sunxi to mainline.
> > 
> > Do you have any idea what happens here? I am a bit clueless what's going on.
> 
> Me too. Since there are mach_to_phys messages perhaps Stefano (CCd) has
> a clue. Original logs are in
> http://lists.xen.org/archives/html/xen-users/2014-07/msg00004.html
> 
> Lots of these under network load:
> 
> [ 189.507495] xen_add_phys_to_mach_entry: cannot add pfn=0x0006930f ->
> mfn=0x0004c3bc: pfn=0x00069310 -> mfn=0x0004c3bc already exists
> [ 189.531185] xen_add_phys_to_mach_entry: cannot add pfn=0x0006921d ->
> mfn=0x0004c489: pfn=0x0006928f -> mfn=0x0004c489 already exists

Unfortunately this is a known issue without a proper solution at the
moment.  It is caused by Zoltan's patch series to switch from grant
copies to grant mappings in netfront/netback, that went in Linux
3.15-rc1.  If you use Linux 3.14 instead, you shouldn't have any
problems.

These are the details: ARM support in Linux cannot deal with multiple
foreign mappings of the same physical page. If the frontend decides to
grant the same page twice, using two different grant references, it is
going to cause problems to the accounting in arch/arm/xen/p2m.c on the
backend side. It is not trivial to come up with a solution because the
data structures in p2m.c are already pretty slow as they are, being able
to account for multiple mappings for a single mfn would slow things down
further.

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?

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

[David Vrabel]

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.

I can't remember how the ARM code works.  Where is the problematic m2p
lookup?  Perhaps this could be avoided for foreign frames?

David

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

[Stefano Stabellini]

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.


> 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.

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

[David Vrabel]

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.

>> 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.

FWIW, On x86, mfn_to_pfn() on a foreign MFN will return an invalid value
(unless the gntdev device made the mapping and the m2p_override is used).

David

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

[Stefano Stabellini]

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.


> >> 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.


> FWIW, On x86, mfn_to_pfn() on a foreign MFN will return an invalid value
> (unless the gntdev device made the mapping and the m2p_override is used).

Keep in mind that ARM domains are a bit like PVH without IOMMU.

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

[David Vrabel]

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.

David

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

[David Vrabel]

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

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

[Stefano Stabellini]

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