Re: [PATCH v2] xen/arm: dump guest stack even if not the current VCPU

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Oct 22, 2014 at 01:56:41PM +0100, Julien Grall wrote:
> Hi Frediano,
> 
> On 10/22/2014 11:05 AM, 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   | 14 +++++++++++++-
> >  xen/arch/arm/traps.c |  2 +-
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > Changes from v1:
> > - disable IRQ if different domain (as suggested by Julien Grall)
> 
> If you want this patch applied for Xen 4.5, you need to explain why it
> will be useful and what are the drawbacks.
> 
> Though, I'd like to see this patch for Xen 4.5 so... The function
> get_page_from_gva is used in hotpath (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.
> 
> (Can you keep this explanation or write your own in case you want this
> for Xen 4.5).

This is a bug-fix though? So if the maintainers (Stefano, Ian) are OK
with this they can put in the tree before this Friday. After Friday
the bars goes up.

Also pls have in the patch title 'PATCH for-xen-4-5' so that the maintainers
can prioritize this email before other ones.

Thank you.
> 
> > 
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 1585d35..6956eab 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1177,8 +1177,13 @@ 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;
> > +    unsigned long irq_flags = 0;
> > 
> > -    ASSERT(d == current->domain);
> > +    if ( unlikely(d != current->domain) )
> > +    {
> > +        local_irq_save(irq_flags);
> > +        p2m_load_VTTBR(d);
> > +    }
> > 
> >      spin_lock(&p2m->lock);
> 
> I though a bit more about the code path. I would first take the lock,
> then switch to the VTTBR and disable IRQ if necessary.
> 
> This would avoid to disable the IRQ for a long time if the lock is
> already taken.
> 
> Regards,
> 
> -- 
> Julien Grall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel