[PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Frediano Ziglio]

From: Frediano Ziglio <frediano.ziglio@huawei.com>

If show_guest_stack was called from Xen context (for instance hitting
'0' key on Xen console) get_page_from_gva was not able to get the
page returning NULL.
Detecting different domain and changing VTTBR register make
get_page_from_gva works for different domains.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/p2m.c   | 22 +++++++++++++++++++---
 xen/arch/arm/traps.c |  2 +-
 2 files changed, 20 insertions(+), 4 deletions(-)

This is a bug fix to fix guest stack dumps.

The function get_page_from_gva is used in hot path (see
arch/arm/guestcopy.c) but always with the current domain. The function
will be used with another domain than current only when the stack of
the guest will be dumped. The code added is self-containted.

Changed from v2:
- add comment suggested by Julien Grall;

- modify code path to avoid keeping IRQ disabled too much (Julien Grall);
- collapse change in a single if to improve performances.

Tested manually.

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1585d35..2345199 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1177,12 +1177,28 @@ struct page_info *get_page_from_gva(struct
domain *d, vaddr_t va,
     struct p2m_domain *p2m = &d->arch.p2m;
     struct page_info *page = NULL;
     paddr_t maddr;
-
-    ASSERT(d == current->domain);
+    int rc;

     spin_lock(&p2m->lock);

-    if ( gvirt_to_maddr(va, &maddr, flags) )
+    if ( unlikely(d != current->domain) )
+    {
+        unsigned long irq_flags;
+
+        local_irq_save(irq_flags);
+        p2m_load_VTTBR(d);
+
+        rc = gvirt_to_maddr(va, &maddr, flags);
+
+        p2m_load_VTTBR(current->domain);
+        local_irq_restore(irq_flags);
+    }
+    else
+    {
+        rc = gvirt_to_maddr(va, &maddr, flags);
+    }
+
+    if ( rc )
         goto err;

     if ( !mfn_valid(maddr >> PAGE_SHIFT) )
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6fc8f8..4c93250 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -892,7 +892,7 @@ static void show_guest_stack(struct vcpu *v,
struct cpu_user_regs *regs)
         return;
     }

-    page = get_page_from_gva(current->domain, sp, GV2M_READ);
+    page = get_page_from_gva(v->domain, sp, GV2M_READ);
     if ( page == NULL )
     {
         printk("Failed to convert stack to physical address\n");
--
1.9.1

Re: [PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Ian Campbell]

On Thu, 2014-10-23 at 09:46 +0100, Frediano Ziglio wrote:
> From: Frediano Ziglio <frediano.ziglio@huawei.com>
> 
> If show_guest_stack was called from Xen context (for instance hitting
> '0' key on Xen console) get_page_from_gva was not able to get the
> page returning NULL.
> Detecting different domain and changing VTTBR register make
> get_page_from_gva works for different domains.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>

Acked + applied (since Konrad said ok in v2).

> - collapse change in a single if to improve performances.

Not 100% keen on the duplicated call to gvirt_to_maddr, but it'll do.

Ian.

Re: [PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Ian Campbell]

On Thu, 2014-10-23 at 09:46 +0100, Frediano Ziglio wrote:
> From: Frediano Ziglio <frediano.ziglio@huawei.com>
> 
> If show_guest_stack was called from Xen context (for instance hitting
> '0' key on Xen console) get_page_from_gva was not able to get the
> page returning NULL.
> Detecting different domain and changing VTTBR register make
> get_page_from_gva works for different domains.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>

Acked + applied (since Konrad said ok in v2).

> - collapse change in a single if to improve performances.

Not 100% keen on the duplicated call to gvirt_to_maddr, but it'll do.

Ian.

Re: [PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Ian Campbell]

On Thu, 2014-10-23 at 12:30 +0100, Ian Campbell wrote:
> On Thu, 2014-10-23 at 09:46 +0100, Frediano Ziglio wrote:
> > From: Frediano Ziglio <frediano.ziglio@huawei.com>
> > 
> > If show_guest_stack was called from Xen context (for instance hitting
> > '0' key on Xen console) get_page_from_gva was not able to get the
> > page returning NULL.
> > Detecting different domain and changing VTTBR register make
> > get_page_from_gva works for different domains.
> > 
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> 
> Acked + applied (since Konrad said ok in v2).

Um, apparently I wrote the mail but didn't actually queue the patch! It
will be in the next batch (which I'm about to kick off)

Ian.

Re: [PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Julien Grall]

On 10/23/2014 12:31 PM, Ian Campbell wrote:
> On Thu, 2014-10-23 at 09:46 +0100, Frediano Ziglio wrote:
>> From: Frediano Ziglio <frediano.ziglio@huawei.com>
>>
>> If show_guest_stack was called from Xen context (for instance hitting
>> '0' key on Xen console) get_page_from_gva was not able to get the
>> page returning NULL.
>> Detecting different domain and changing VTTBR register make
>> get_page_from_gva works for different domains.
>>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> 
> Acked + applied (since Konrad said ok in v2).
> 
>> - collapse change in a single if to improve performances.
> 
> Not 100% keen on the duplicated call to gvirt_to_maddr, but it'll do.

I think having two if with gvirt_to_maddr in the middle would have been
fine. My main concern was disabling the IRQ before taking the lock.

Anyway, I plan to send a patch for 4.6 to improve this function. The p2m
is taken for a long time for nothing.

Regards,

-- 
Julien Grall

Re: [PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Frediano Ziglio]

Agreed, but this is a performance improvement and not a fix.

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2345199..fab0f57 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1198,6 +1198,8 @@ struct page_info *get_page_from_gva(struct
domain *d, vaddr_t va,
         rc = gvirt_to_maddr(va, &maddr, flags);
     }

+    spin_unlock(&p2m->lock);
+
     if ( rc )
         goto err;

@@ -1211,7 +1213,6 @@ struct page_info *get_page_from_gva(struct
domain *d, vaddr_t va,
         page = NULL;

 err:
-    spin_unlock(&p2m->lock);
     return page;
 }


Frediano

Frediano


2014-10-23 12:56 GMT+01:00 Julien Grall <julien.grall@linaro.org>:
> On 10/23/2014 12:31 PM, Ian Campbell wrote:
>> On Thu, 2014-10-23 at 09:46 +0100, Frediano Ziglio wrote:
>>> From: Frediano Ziglio <frediano.ziglio@huawei.com>
>>>
>>> If show_guest_stack was called from Xen context (for instance hitting
>>> '0' key on Xen console) get_page_from_gva was not able to get the
>>> page returning NULL.
>>> Detecting different domain and changing VTTBR register make
>>> get_page_from_gva works for different domains.
>>>
>>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>>
>> Acked + applied (since Konrad said ok in v2).
>>
>>> - collapse change in a single if to improve performances.
>>
>> Not 100% keen on the duplicated call to gvirt_to_maddr, but it'll do.
>
> I think having two if with gvirt_to_maddr in the middle would have been
> fine. My main concern was disabling the IRQ before taking the lock.
>
> Anyway, I plan to send a patch for 4.6 to improve this function. The p2m
> is taken for a long time for nothing.
>
> Regards,
>
> --
> Julien Grall

Re: [PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Ian Campbell]

On Thu, 2014-10-23 at 09:46 +0100, Frediano Ziglio wrote:
> From: Frediano Ziglio <frediano.ziglio@huawei.com>
> 
> If show_guest_stack was called from Xen context (for instance hitting
> '0' key on Xen console) get_page_from_gva was not able to get the
> page returning NULL.
> Detecting different domain and changing VTTBR register make
> get_page_from_gva works for different domains.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> ---
>  xen/arch/arm/p2m.c   | 22 +++++++++++++++++++---
>  xen/arch/arm/traps.c |  2 +-
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> This is a bug fix to fix guest stack dumps.
> 
> The function get_page_from_gva is used in hot path (see
> arch/arm/guestcopy.c) but always with the current domain. The function
> will be used with another domain than current only when the stack of
> the guest will be dumped. The code added is self-containted.
> 
> Changed from v2:
> - add comment suggested by Julien Grall;
> 
> - modify code path to avoid keeping IRQ disabled too much (Julien Grall);
> - collapse change in a single if to improve performances.
> 
> Tested manually.

I succeeding in applying for real this time but:
p2m.c: In function ‘get_page_from_gva’:
p2m.c:1185:45: error: ‘maddr’ may be used uninitialized in this function [-Werror=uninitialized]
cc1: all warnings being treated as errors
make[4]: *** [p2m.o] Error 1

This was on arm32. So not pushed.

> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 1585d35..2345199 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1177,12 +1177,28 @@ struct page_info *get_page_from_gva(struct
> domain *d, vaddr_t va,

Please avoid word wrapping patches in the future.
http://wiki.xen.org/wiki/Submitting_Xen_Patches has some guidance for
different mail clients. I fixed it by hand this time.

Ian.


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

Re: [PATCH for-xen-4-5 v3] xen/arm: dump guest stack even if not the current VCPU

[Frediano Ziglio]

2014-10-23 13:14 GMT+01:00 Ian Campbell <Ian.Campbell@citrix.com>:
> On Thu, 2014-10-23 at 09:46 +0100, Frediano Ziglio wrote:
>> From: Frediano Ziglio <frediano.ziglio@huawei.com>
>>
>> If show_guest_stack was called from Xen context (for instance hitting
>> '0' key on Xen console) get_page_from_gva was not able to get the
>> page returning NULL.
>> Detecting different domain and changing VTTBR register make
>> get_page_from_gva works for different domains.
>>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>> ---
>>  xen/arch/arm/p2m.c   | 22 +++++++++++++++++++---
>>  xen/arch/arm/traps.c |  2 +-
>>  2 files changed, 20 insertions(+), 4 deletions(-)
>>
>> This is a bug fix to fix guest stack dumps.
>>
>> The function get_page_from_gva is used in hot path (see
>> arch/arm/guestcopy.c) but always with the current domain. The function
>> will be used with another domain than current only when the stack of
>> the guest will be dumped. The code added is self-containted.
>>
>> Changed from v2:
>> - add comment suggested by Julien Grall;
>>
>> - modify code path to avoid keeping IRQ disabled too much (Julien Grall);
>> - collapse change in a single if to improve performances.
>>
>> Tested manually.
>
> I succeeding in applying for real this time but:
> p2m.c: In function ‘get_page_from_gva’:
> p2m.c:1185:45: error: ‘maddr’ may be used uninitialized in this function [-Werror=uninitialized]
> cc1: all warnings being treated as errors
> make[4]: *** [p2m.o] Error 1
>
> This was on arm32. So not pushed.
>

Mmm... probably your compiler is detecting a false error. maddr is set
on both if paths. Should I rewrite the patch to workaround this
compiler problem? Obviously if we pretend to support this compiler. Or
perhaps we should silence this warning
(http://stackoverflow.com/questions/19374122/gcc-removing-is-used-uninitialized-in-this-function-warning).
I think setting variable to 0 at declaration would be quite a fine
solution.

>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 1585d35..2345199 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1177,12 +1177,28 @@ struct page_info *get_page_from_gva(struct
>> domain *d, vaddr_t va,
>
> Please avoid word wrapping patches in the future.
> http://wiki.xen.org/wiki/Submitting_Xen_Patches has some guidance for
> different mail clients. I fixed it by hand this time.
>
> Ian.
>

I'll keep more attention. Perhaps I did some wrong copy&paste.

Frediano

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