Re: the P2M/VP patch merge plan (was Re: [Xen-ia64-devel] [PATCH][RFC][TAKE5] the P2M/VP patches)

[Tristan Gingold <Tristan.Gingold@xxxxxxxx>]

Le Mercredi 17 Mai 2006 15:53, Isaku Yamahata a écrit :
> On Wed, May 17, 2006 at 09:34:23AM +0200, Tristan Gingold wrote:
> > Le Mercredi 17 Mai 2006 04:36, Isaku Yamahata a écrit :
> > > On Tue, May 16, 2006 at 05:30:17PM +0200, Tristan Gingold wrote:
> > > > [...]
> > > >
> > > > > > > The following is what I notice now.
> > > > > > >
> > > > > > > - pgd_populate(), pud_populate(), pmd_populate()
> > > > > > >   What if two cpu try to populate same virtual address?
> > > > > > >   Given that page allocation on demand is now removed, it might
> > > > > > > be possible to all necessary pgd/pud/pmd/pte page is allocate
> > > > > > > at domain creation.
> > > >
> > > > This could be easily fixed.  However there is also no lock in the
> > > > current Xen, so I think the kernel never does that.
> > >
> > > Although current linux kernel doesn't,
> > > I will add such a code to avoid oom-kiler in dom0 when vt-i domain
> > > is created.
> > > Hmm, it doesn't seem that you have the necessity of protecting the p2m
> > > table. On the other hand I have.
> > > I'll work on adding protection the p2m table. Is it okay?
> >
> > I still do not understand why we need to protect p2m table.
> > I don't have your deep understanding, but why two cpus can try to fill
> > the same p2m entry.
>
> Your approach seems just band-aiding found races.
> I don't believe that such a approach is good and
> that we can achieve stability.
I'd just like to understand clearly why and where locks must be added.
Adding locks has a cost and must be understood in order to be done correctly.

> The essence is that tlb insert/purge virtulalization by xen
> isn't serialized.
Yes.
tlb insert/purge only reads p2m.  If p2m is always coherent, there is no need 
to lock reads.

> A big lock should be introduced as a first step.
> It is O.K. that its performance may be poor at first.
> Then the big lock should be relaxed later making sure that no race exists.
>
> > > > > > > - guest_physmap_add_page()
> > > > > > >     assign_domain_page_replace()
> > > > > > >       ptep_get_and_clear()
> > > > > > >                       <<<<<<<<<<<< what if another cpu does
> > > > > > > set_pte() here? set_pte()
> > > > > > >     set_gpfn_from_mfn()
> > > >
> > > > We should create a ptep_get_and_replace.
> > > > Is it enough ?
> > >
> > > Just for the integrity of the p2m table, it might be enough.
> > > But I'm not sure.
> > >
> > > > Since kernel is not supposed to access to pages being replaced, this
> > > > shouldn't happen, should it ?
> > >
> > > What do you mean by 'not supposed'?
> > > I think that nothing guarantees it.
> > > Xen shouldn't rely on the fact that xenLinux just doesn't but a guest
> > > domain surely can.
> >
> > If a guest domain is still writing (or reading) a dying page, it
> > miss-behaves.
>
> Yes.
>
> > For sure, we must protect Xen against misbehavior, but this is only a
> > security issue (ie, if SMP-g doesn't work yet this is not due to this).
>
> It maybe true.
> However fixing this requires a kind of exclusion, it will make a big impact
> on code complexity and performance.
> It would be very difficult to introduce a exclusion after performance
> tuning. And tuning must be done again.
Anyway, my previously submitted patch does an atomic xchg for pte.
I was able to compile 10 times linux on dom0 without any fault.

Tristan.

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel