From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes Date: Tue, 25 Mar 2014 12:51:51 +0000 Message-ID: <53317BE7.1080805@linaro.org> References: <1395712976-19454-1-git-send-email-avanzini.arianna@gmail.com> <1395712976-19454-3-git-send-email-avanzini.arianna@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1395712976-19454-3-git-send-email-avanzini.arianna@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Arianna Avanzini Cc: julien.grall@citrix.com, paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, tim@xen.org, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, Ian.Campbell@eu.citrix.com, etrudeau@broadcom.com, JBeulich@suse.com, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org Hi Arianna, Thank you for the patch, On 03/25/2014 02:02 AM, Arianna Avanzini wrote: > Currently, the REMOVE case of the switch in apply_p2m_changes() > does not perform any consistency check on the mapping to be removed. > More in detail, the code does not check if the guest address to be > unmapped is actually mapped to the machine address given as a > parameter. > This commit attempts to add the above-described consistency check > to the REMOVE path of apply_p2m_changes(). This is instrumental to > one of the following commits which implements the possibility to > trigger the removal of p2m ranges via the memory_mapping DOMCTL > for ARM. > > Signed-off-by: Arianna Avanzini > Cc: Dario Faggioli > Cc: Paolo Valente > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: Ian Campbell > Cc: Jan Beulich > Cc: Keir Fraser > Cc: Tim Deegan > Cc: Ian Jackson > Cc: Eric Trudeau > Cc: Viktor Kleinik > > --- > > v4: > - Remove useless and slow lookup and use already-available > data from pte instead. > - Correctly increment the local variable used to keep the > machine address whose mapping is currently being removed. > - Return with an error upon finding a mismatch between the > actual machine address mapped to the guest address and > the machine address passed as parameter, instead of just > skipping the page. > > --- > xen/arch/arm/p2m.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d00c882..bb0db16 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -243,12 +243,13 @@ static int apply_p2m_changes(struct domain *d, > int rc; > struct p2m_domain *p2m = &d->arch.p2m; > lpae_t *first = NULL, *second = NULL, *third = NULL; > - paddr_t addr; > + paddr_t addr, _maddr; > unsigned long cur_first_page = ~0, > cur_first_offset = ~0, > cur_second_offset = ~0; > unsigned long count = 0; > unsigned int flush = 0; > + unsigned long mfn; > bool_t populate = (op == INSERT || op == ALLOCATE); > lpae_t pte; > > @@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d, > p2m_load_VTTBR(d); > > addr = start_gpaddr; > + _maddr = maddr; You don't need a temporary variable to hold maddr and increment it. You can directly use maddr, but you will have to remove "maddr += PAGE_SIZE" in INSERT. You also need to increment maddr when first/second level are not valid. While reading the code, I'm wondering if we need to return an error if we are trying to remove a non-existent page. Ian, Stefano, any thoughs? > while ( addr < end_gpaddr ) > { > if ( cur_first_page != p2m_first_level_index(addr) ) > @@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d, > > flush |= pte.p2m.valid; > > + mfn = pte.p2m.base; I would move mfn = ... in REMOVE part because it may not be valid on some place and wrongly used in the future. > /* TODO: Handle other p2m type > * > * It's safe to do the put_page here because page_alloc will > @@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d, > */ > if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) ) > { > - unsigned long mfn = pte.p2m.base; > - > ASSERT(mfn_valid(mfn)); > put_page(mfn_to_page(mfn)); > } > @@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d, > maddr += PAGE_SIZE; > } > break; > - case RELINQUISH: > case REMOVE: > { > + ASSERT(pte.p2m.valid); Why did you add an ASSERT here? Regards, -- Julien Grall