| Message ID | 1402935486-29136-2-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 16 Jun 2014, Julien Grall wrote: > The function guest_physmap_remove_page does't have a return value. With > the change "arch/arm: add consistency check to REMOVE p2m changes", > apply_p2m_changes can unlikely fail. Warn the user in this case. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > xen/arch/arm/p2m.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index f93f99a..51f9225 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -607,10 +607,16 @@ void guest_physmap_remove_page(struct domain *d, > unsigned long gpfn, > unsigned long mfn, unsigned int page_order) > { > - apply_p2m_changes(d, REMOVE, > - pfn_to_paddr(gpfn), > - pfn_to_paddr(gpfn + (1<<page_order)), > - pfn_to_paddr(mfn), NULL, MATTR_MEM, p2m_invalid); > + int ret; > + > + ret = apply_p2m_changes(d, REMOVE, > + pfn_to_paddr(gpfn), > + pfn_to_paddr(gpfn + (1<<page_order)), > + pfn_to_paddr(mfn), NULL, MATTR_MEM, p2m_invalid); > + if ( ret ) > + dprintk(XENLOG_G_WARNING, > + "DOM%u: Unable to unmap region GPFN 0x%lx - 0x%lx MFN 0x%lx\n", > + d->domain_id, gpfn, gpfn + (1 << page_order), mfn); > } > > int p2m_alloc_table(struct domain *d) > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
On Mon, 2014-06-16 at 17:17 +0100, Julien Grall wrote: > The function guest_physmap_remove_page does't have a return value. With > the change "arch/arm: add consistency check to REMOVE p2m changes", > apply_p2m_changes can unlikely fail. Looking at v9 of that patch I don't see it adding any new failures, is this comment (I suppose written against an older version) still accurate? > Warn the user in this case. Given that apply_p2m_changes can fail it seems reasonable to log regardless of the above: Acked-by: Ian Campbell <ian.campbell@citrix.com> Two questions: Would it be better to instead ensure that apply_p2m_changes always logs on failure? I suppose it would be more able to give a specific message. On failure do we retain any reference counts to prevent these pages getting reused? Ian.
Hi Ian, On 07/03/2014 11:52 AM, Ian Campbell wrote: > On Mon, 2014-06-16 at 17:17 +0100, Julien Grall wrote: >> The function guest_physmap_remove_page does't have a return value. With >> the change "arch/arm: add consistency check to REMOVE p2m changes", >> apply_p2m_changes can unlikely fail. > > Looking at v9 of that patch I don't see it adding any new failures, is > this comment (I suppose written against an older version) still > accurate? It doesn't look like accurate for v9. I can change the commit message to explain that apply_p2m_changes may fail. So it's better to log it/ >> Warn the user in this case. > > Given that apply_p2m_changes can fail it seems reasonable to log > regardless of the above: > Acked-by: Ian Campbell <ian.campbell@citrix.com> > Two questions: > > Would it be better to instead ensure that apply_p2m_changes always logs > on failure? I suppose it would be more able to give a specific message. I didn't though about this solution. It may be easier for debugging later. > On failure do we retain any reference counts to prevent these pages > getting reused? We don't have any mapping reference count so it's fine. I've asked few changes in Arianna's series to clear the p2m even if the mapping doesn't correspond. Otherwise we may have issue with foreign mapping. I will drop this patch from the series and send a separate patch. Regards,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index f93f99a..51f9225 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -607,10 +607,16 @@ void guest_physmap_remove_page(struct domain *d, unsigned long gpfn, unsigned long mfn, unsigned int page_order) { - apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gpfn), - pfn_to_paddr(gpfn + (1<<page_order)), - pfn_to_paddr(mfn), NULL, MATTR_MEM, p2m_invalid); + int ret; + + ret = apply_p2m_changes(d, REMOVE, + pfn_to_paddr(gpfn), + pfn_to_paddr(gpfn + (1<<page_order)), + pfn_to_paddr(mfn), NULL, MATTR_MEM, p2m_invalid); + if ( ret ) + dprintk(XENLOG_G_WARNING, + "DOM%u: Unable to unmap region GPFN 0x%lx - 0x%lx MFN 0x%lx\n", + d->domain_id, gpfn, gpfn + (1 << page_order), mfn); } int p2m_alloc_table(struct domain *d)
The function guest_physmap_remove_page does't have a return value. With the change "arch/arm: add consistency check to REMOVE p2m changes", apply_p2m_changes can unlikely fail. Warn the user in this case. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/p2m.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)