From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen/arm: dump guest stack even if not the current VCPU Date: Mon, 20 Oct 2014 18:42:59 +0100 Message-ID: <544549A3.7060402@linaro.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Frediano Ziglio , Ian Campbell Cc: Tim Deegan , Stefano Stabellini , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org Hi Frediano, Thank you for the patch. On 20/10/2014 16:39, Frediano Ziglio wrote: > From: Frediano Ziglio > > 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 > --- > 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, -- Julien Grall