Re: [Xen-devel] [PATCH v3 10/38] arm/p2m: Move hostp2m init/teardown to individual functions

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 05/09/16 11:23, Sergej Proskurin wrote:
> On 09/02/2016 12:51 PM, Julien Grall wrote:
> > On 02/09/16 10:09, Sergej Proskurin wrote:
> > > On 09/01/2016 07:36 PM, Julien Grall wrote:
> > > > On 16/08/16 23:16, Sergej Proskurin wrote:
> > > > Clearing live page table like that is not safe. Each entry (64-bit)
> > > > should be written atomically to avoid breaking coherency (e.g if the
> > > > MMU
> > > > is walking the page table at the same time). However you don't know the
> > > > implementation of clear_and_clean_page.
> > > The function p2m_flush_table gets called by the altp2m subsystem
> > > indrectly through the function p2m_teardown_one when the associated view
> > > gets destroyed. In this case we should not worry about crashing the
> > > domain, as the altp2m views are not active anyway.
> > >
> > > The function altp2m_reset calls the function p2m_flush_table directly
> > > (with active altp2m views), however, locks the p2m before flushing the
> > > table. I did not find any locks on page-granularity, so please provide
> > > me with further information about the solution you had in mind.
> > I never mentioned any locking problem. As said in my previous mail,
> > the altp2m may still be in use by another vCPU. So the MMU (i.e the
> > hardware) may want do a table walk whilst you modify the entry.
> >
> > The MMU is reading the entry (64-bit) at once so it also expects the
> > entry to be modified atomically. However, you don't know the
> > implementation of clean_and_clean_page. The function might write
> > 32-bit at the time, which means that the MMU will see bogus entry. At
> > best it will lead to a crash, at worst open a security issue.
> I see your point. Not sure how to fix this, though. I believe that the
> described issue would remain if we would use free_domheap_pages.
> Instead, maybe we should manually set the value in the translation tables?
This is the right way to go.

> Or, what if we flush the TLBs immediately before unmapping the root
> pages? This would cause the system to load the mappings from memory and
> delay a MMU table walk so that it would potentially resolve the
> atomicity issue.
I am not sure to fully understand this suggestion. Anyway, the first 
suggestion (i.e invalidating the entry one by one is the right way to go).
> > > > >      radix_tree_destroy(&p2m->mem_access_settings, NULL);
> > > > >  }
> > > > >
> > > > > -int p2m_init(struct domain *d)
> > > > > +int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
> > > > >  {
> > > > > -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > > >      int rc = 0;
> > > > >
> > > > >      rwlock_init(&p2m->lock);
> > > > > @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d)
> > > > >          return rc;
> > > > >
> > > > >      p2m->max_mapped_gfn = _gfn(0);
> > > > > -    p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
> > > > > +    p2m->lowest_mapped_gfn = INVALID_GFN;
> > > > Why this change?
> > > Since we compare the gfn's with INVALID_GFN throughout the code it makes
> > > sense to use the macro instead of a hardcoded value.
> > Please don't do unnecessary change. This patch is complex enough to
> > review.
> Ok.
If you want to do the change, then it should be done separately.

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel