diff mbox

[Xen-devel] xen/arm: dump guest stack even if not the current VCPU

Message ID CAHt6W4dS+v_3RExtstzfwRWCEKfd3Z+WFXLN1Q4EQbErdb6XzQ@mail.gmail.com
State New
Headers show

Commit Message

Frediano Ziglio Oct. 20, 2014, 3:39 p.m. UTC 
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   | 7 ++++++-
 xen/arch/arm/traps.c | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

Tested manually on a D01 board. I actually have only dom0 so EL1 is
already pointing to the right domain. I have some doubt about the
is_idle_domain test inside p2m_load_VTTBR.
I used the "unlikely" to avoid decreasing performances on the normal
cases, I'm fine if you remove them.

         printk("Failed to convert stack to physical address\n");

Comments

Julien Grall Oct. 20, 2014, 5:42 p.m. UTC | #1 
Hi Frediano,

Thank you for the patch.

On 20/10/2014 16:39, 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   | 7 ++++++-
>   xen/arch/arm/traps.c | 2 +-
>   2 files changed, 7 insertions(+), 2 deletions(-)
>
> Tested manually on a D01 board. I actually have only dom0 so EL1 is
> already pointing to the right domain. I have some doubt about the
> is_idle_domain test inside p2m_load_VTTBR.

I guess this patch could be consider as a bug fix for Xen 4.5. It makes 
dump stack working when the dump domain key is hit.

> I used the "unlikely" to avoid decreasing performances on the normal
> cases, I'm fine if you remove them.

I'm fine with keeping the unlikely.

>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 1585d35..cf872fc 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1178,7 +1178,8 @@ struct page_info *get_page_from_gva(struct
> domain *d, vaddr_t va,
>       struct page_info *page = NULL;
>       paddr_t maddr;
>
> -    ASSERT(d == current->domain);
> +    if ( unlikely(d != current->domain) )
> +        p2m_load_VTTBR(d);

The code between the 2 p2m_load_VTTBR should not interrupted by an 
interrupt. I send a similar patch (https://patches.linaro.org/38895/) 
for flush_tlb_domain.

>       spin_lock(&p2m->lock);

I think taking the lock for the whole function is a bit too much here.
IHMO, the lock is only necessary for gvirt_to_maddr.

But this is 4.6 material if the patch reaches 4.5.

Regards,
diff mbox

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1585d35..cf872fc 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1178,7 +1178,8 @@  struct page_info *get_page_from_gva(struct
domain *d, vaddr_t va,
     struct page_info *page = NULL;
     paddr_t maddr;

-    ASSERT(d == current->domain);
+    if ( unlikely(d != current->domain) )
+        p2m_load_VTTBR(d);

     spin_lock(&p2m->lock);

@@ -1196,6 +1197,10 @@  struct page_info *get_page_from_gva(struct
domain *d, vaddr_t va,

 err:
     spin_unlock(&p2m->lock);
+
+    if ( unlikely(d != current->domain) )
+        p2m_load_VTTBR(current->domain);
+
     return page;
 }

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 )
     {