Patchwork [for-4.10,2/2] xen/arm: p2m: Add more debug in get_page_from_gva

login
register
mail settings
Submitter Julien Grall
Date Nov. 15, 2017, 7:34 p.m.
Message ID <20171115193414.7678-3-julien.grall@linaro.org>
Download mbox | patch
Permalink /patch/384709/
State New
Headers show

Comments

Julien Grall - Nov. 15, 2017, 7:34 p.m.
The function get_page_from_gva is used by copy_*_guest helpers to
translate a guest virtual address to a machine physical address and take
reference on the page.

There are a couple of errors path that will return the same value making
difficult to know the exact error. Add more debug in each error patch
only for debug-build.

This should help narrowing down the intermittent failure with the
hypercall GNTTABOP_copy (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-11/msg00942.html

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Andrew Cooper - Nov. 15, 2017, 7:43 p.m.
On 15/11/17 19:34, Julien Grall wrote:
> The function get_page_from_gva is used by copy_*_guest helpers to
> translate a guest virtual address to a machine physical address and take
> reference on the page.
>
> There are a couple of errors path that will return the same value making
> difficult to know the exact error. Add more debug in each error patch
> only for debug-build.
>
> This should help narrowing down the intermittent failure with the
> hypercall GNTTABOP_copy (see [1]).
>
> [1] https://lists.xen.org/archives/html/xen-devel/2017-11/msg00942.html
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/p2m.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f6b3d8e421..417609ede2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1428,16 +1428,29 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>      par = gvirt_to_maddr(va, &maddr, flags);
>  
>      if ( par )
> +    {
> +        dprintk(XENLOG_G_DEBUG,
> +                "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx par=%#"PRIx64"\n",
> +                v, va, flags, par);

Given the long round-trip time on debugging output, how about trying to
dump the guest and/or second stage table walk?

~Andrew

>          goto err;
> +    }
>  
>      if ( !mfn_valid(maddr_to_mfn(maddr)) )
> +    {
> +        dprintk(XENLOG_G_DEBUG, "%pv: Invalid MFN %#"PRI_mfn"\n",
> +                v, mfn_x(maddr_to_mfn(maddr)));
>          goto err;
> +    }
>  
>      page = mfn_to_page(maddr_to_mfn(maddr));
>      ASSERT(page);
>  
>      if ( unlikely(!get_page(page, d)) )
> +    {
> +        dprintk(XENLOG_G_DEBUG, "%pv: Failing to acquire the MFN %#"PRI_mfn"\n",
> +                v, mfn_x(maddr_to_mfn(maddr)));
>          page = NULL;
> +    }
>  
>  err:
>      if ( !page && p2m->mem_access_enabled )
Julien Grall - Nov. 15, 2017, 9:42 p.m.
Hi Andrew,

On 11/15/2017 07:43 PM, Andrew Cooper wrote:
> On 15/11/17 19:34, Julien Grall wrote:
>> The function get_page_from_gva is used by copy_*_guest helpers to
>> translate a guest virtual address to a machine physical address and take
>> reference on the page.
>>
>> There are a couple of errors path that will return the same value making
>> difficult to know the exact error. Add more debug in each error patch
>> only for debug-build.
>>
>> This should help narrowing down the intermittent failure with the
>> hypercall GNTTABOP_copy (see [1]).
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2017-11/msg00942.html
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/p2m.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index f6b3d8e421..417609ede2 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1428,16 +1428,29 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>>       par = gvirt_to_maddr(va, &maddr, flags);
>>   
>>       if ( par )
>> +    {
>> +        dprintk(XENLOG_G_DEBUG,
>> +                "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx par=%#"PRIx64"\n",
>> +                v, va, flags, par);
> 
> Given the long round-trip time on debugging output, how about trying to
> dump the guest and/or second stage table walk?

I thought about it, however at the moment dump_s1_guest_walk() is very 
minimal and would be add much value here. Thought, Now that we have code 
to do first-stage walk (see guest_walk_tables), we might be able to get 
a better dump here. Thought I am not sure it would be 4.10 material.

However, I think we could try to translate the guest VA to a guest PA 
using hardware instruction and then do the second-stage walk using 
dump_p2m_lookup.

Let me have a look.

Cheers,
Stefano Stabellini - Nov. 16, 2017, 1:36 a.m.
On Wed, 15 Nov 2017, Julien Grall wrote:
> The function get_page_from_gva is used by copy_*_guest helpers to
> translate a guest virtual address to a machine physical address and take
> reference on the page.
> 
> There are a couple of errors path that will return the same value making
                               ^ paths

> difficult to know the exact error. Add more debug in each error patch
^ it difficult


> only for debug-build.
> 
> This should help narrowing down the intermittent failure with the
> hypercall GNTTABOP_copy (see [1]).
> 
> [1] https://lists.xen.org/archives/html/xen-devel/2017-11/msg00942.html
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

fixed on commit


> ---
>  xen/arch/arm/p2m.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f6b3d8e421..417609ede2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1428,16 +1428,29 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>      par = gvirt_to_maddr(va, &maddr, flags);
>  
>      if ( par )
> +    {
> +        dprintk(XENLOG_G_DEBUG,
> +                "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx par=%#"PRIx64"\n",
> +                v, va, flags, par);
>          goto err;
> +    }
>  
>      if ( !mfn_valid(maddr_to_mfn(maddr)) )
> +    {
> +        dprintk(XENLOG_G_DEBUG, "%pv: Invalid MFN %#"PRI_mfn"\n",
> +                v, mfn_x(maddr_to_mfn(maddr)));
>          goto err;
> +    }
>  
>      page = mfn_to_page(maddr_to_mfn(maddr));
>      ASSERT(page);
>  
>      if ( unlikely(!get_page(page, d)) )
> +    {
> +        dprintk(XENLOG_G_DEBUG, "%pv: Failing to acquire the MFN %#"PRI_mfn"\n",
> +                v, mfn_x(maddr_to_mfn(maddr)));
>          page = NULL;
> +    }
>  
>  err:
>      if ( !page && p2m->mem_access_enabled )
Julien Grall - Nov. 16, 2017, 8:50 a.m.
On 11/16/2017 01:36 AM, Stefano Stabellini wrote:
> On Wed, 15 Nov 2017, Julien Grall wrote:
>> The function get_page_from_gva is used by copy_*_guest helpers to
>> translate a guest virtual address to a machine physical address and take
>> reference on the page.
>>
>> There are a couple of errors path that will return the same value making
>                                 ^ paths
> 
>> difficult to know the exact error. Add more debug in each error patch
> ^ it difficult
> 
> 
>> only for debug-build.
>>
>> This should help narrowing down the intermittent failure with the
>> hypercall GNTTABOP_copy (see [1]).
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2017-11/msg00942.html
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> fixed on commit

I am not sure why this was merged given Andrew gave some comments...

> 
> 
>> ---
>>   xen/arch/arm/p2m.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index f6b3d8e421..417609ede2 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1428,16 +1428,29 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>>       par = gvirt_to_maddr(va, &maddr, flags);
>>   
>>       if ( par )
>> +    {
>> +        dprintk(XENLOG_G_DEBUG,
>> +                "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx par=%#"PRIx64"\n",
>> +                v, va, flags, par);
>>           goto err;
>> +    }
>>   
>>       if ( !mfn_valid(maddr_to_mfn(maddr)) )
>> +    {
>> +        dprintk(XENLOG_G_DEBUG, "%pv: Invalid MFN %#"PRI_mfn"\n",
>> +                v, mfn_x(maddr_to_mfn(maddr)));
>>           goto err;
>> +    }
>>   
>>       page = mfn_to_page(maddr_to_mfn(maddr));
>>       ASSERT(page);
>>   
>>       if ( unlikely(!get_page(page, d)) )
>> +    {
>> +        dprintk(XENLOG_G_DEBUG, "%pv: Failing to acquire the MFN %#"PRI_mfn"\n",
>> +                v, mfn_x(maddr_to_mfn(maddr)));
>>           page = NULL;
>> +    }
>>   
>>   err:
>>       if ( !page && p2m->mem_access_enabled )
Stefano Stabellini - Nov. 16, 2017, 7 p.m.
On Thu, 16 Nov 2017, Julien Grall wrote:
> On 11/16/2017 01:36 AM, Stefano Stabellini wrote:
> > On Wed, 15 Nov 2017, Julien Grall wrote:
> > > The function get_page_from_gva is used by copy_*_guest helpers to
> > > translate a guest virtual address to a machine physical address and take
> > > reference on the page.
> > > 
> > > There are a couple of errors path that will return the same value making
> >                                 ^ paths
> > 
> > > difficult to know the exact error. Add more debug in each error patch
> > ^ it difficult
> > 
> > 
> > > only for debug-build.
> > > 
> > > This should help narrowing down the intermittent failure with the
> > > hypercall GNTTABOP_copy (see [1]).
> > > 
> > > [1] https://lists.xen.org/archives/html/xen-devel/2017-11/msg00942.html
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > fixed on commit
> 
> I am not sure why this was merged given Andrew gave some comments...

Sorry Andrew! I missed your reply!

 
> > 
> > > ---
> > >   xen/arch/arm/p2m.c | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index f6b3d8e421..417609ede2 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -1428,16 +1428,29 @@ struct page_info *get_page_from_gva(struct vcpu
> > > *v, vaddr_t va,
> > >       par = gvirt_to_maddr(va, &maddr, flags);
> > >         if ( par )
> > > +    {
> > > +        dprintk(XENLOG_G_DEBUG,
> > > +                "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx
> > > par=%#"PRIx64"\n",
> > > +                v, va, flags, par);
> > >           goto err;
> > > +    }
> > >         if ( !mfn_valid(maddr_to_mfn(maddr)) )
> > > +    {
> > > +        dprintk(XENLOG_G_DEBUG, "%pv: Invalid MFN %#"PRI_mfn"\n",
> > > +                v, mfn_x(maddr_to_mfn(maddr)));
> > >           goto err;
> > > +    }
> > >         page = mfn_to_page(maddr_to_mfn(maddr));
> > >       ASSERT(page);
> > >         if ( unlikely(!get_page(page, d)) )
> > > +    {
> > > +        dprintk(XENLOG_G_DEBUG, "%pv: Failing to acquire the MFN
> > > %#"PRI_mfn"\n",
> > > +                v, mfn_x(maddr_to_mfn(maddr)));
> > >           page = NULL;
> > > +    }
> > >     err:
> > >       if ( !page && p2m->mem_access_enabled )
>

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f6b3d8e421..417609ede2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1428,16 +1428,29 @@  struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     par = gvirt_to_maddr(va, &maddr, flags);
 
     if ( par )
+    {
+        dprintk(XENLOG_G_DEBUG,
+                "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx par=%#"PRIx64"\n",
+                v, va, flags, par);
         goto err;
+    }
 
     if ( !mfn_valid(maddr_to_mfn(maddr)) )
+    {
+        dprintk(XENLOG_G_DEBUG, "%pv: Invalid MFN %#"PRI_mfn"\n",
+                v, mfn_x(maddr_to_mfn(maddr)));
         goto err;
+    }
 
     page = mfn_to_page(maddr_to_mfn(maddr));
     ASSERT(page);
 
     if ( unlikely(!get_page(page, d)) )
+    {
+        dprintk(XENLOG_G_DEBUG, "%pv: Failing to acquire the MFN %#"PRI_mfn"\n",
+                v, mfn_x(maddr_to_mfn(maddr)));
         page = NULL;
+    }
 
 err:
     if ( !page && p2m->mem_access_enabled )