| Message ID | 20171115193414.7678-3-julien.grall@linaro.org |
|---|---|
| State | Accepted |
| Commit | ca4b2e52a894845f26fc5b784f465e31c4cef90b |
| Headers | show |
| Series | xen/arm: Add more debug in get_page_from_gva | expand |
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 )
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,
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 )
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 )
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 ) >
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 )
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(+)