Re: [Xen-ia64-devel][PATCH][VTD] small patches for VTD

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Wed, Oct 22, 2008 at 01:56:05PM +0800, Xu, Anthony wrote:
> >> diff -r e1ed3e5cd001 xen/arch/ia64/xen/mm.c
> >> --- a/xen/arch/ia64/xen/mm.c  Tue Oct 21 18:55:22 2008 +0800
> >> +++ b/xen/arch/ia64/xen/mm.c  Tue Oct 21 19:13:45 2008 +0800 @@
> >> -189,6 +189,10 @@
> >>
> >>  static void __xencomm_mark_dirty(struct domain *d,
> >>                                   unsigned long addr, unsigned int
> >> len); + +static void
> >> +zap_domain_page_one(struct domain *d, unsigned long mpaddr,
> >> +                    int clear_PGC_allocate, unsigned long mfn);
> >>
> >>  extern unsigned long ia64_iobase;
> >>
> >> @@ -908,20 +912,21 @@
> >>                       unsigned long flags)
> >>  {
> >>      volatile pte_t *pte;
> >> -    pte_t old_pte;
> >>      pte_t new_pte;
> >>      pte_t ret_pte;
> >>      unsigned long prot = flags_to_prot(flags);
> >>
> >>      pte = lookup_alloc_domain_pte(d, mpaddr);
> >> +    new_pte = pfn_pte(physaddr >> PAGE_SHIFT, __pgprot(prot));
> >> +    if(pte_val(new_pte) == pte_val(*pte))
> >> +        return 0;
> >>
> >> -    old_pte = __pte(0);
> >> -    new_pte = pfn_pte(physaddr >> PAGE_SHIFT, __pgprot(prot));
> >> -    ret_pte = ptep_cmpxchg_rel(&d->arch.mm, mpaddr, pte, old_pte,
> >> new_pte);
> >> -    if (pte_val(ret_pte) == pte_val(old_pte)) {
> >> -        smp_mb();
> >> -        return 0;
> >> -    }
> >> +    /* for assigned MMIO, the old pte may be set to _PAGE_IO
> >> attribute, +     * so zap it first, then set up it.
> >> +     */
> >> +
> >> +    zap_domain_page_one(d, mpaddr, 1, INVALID_MFN);
> >> +    ret_pte = ptep_xchg(&d->arch.mm, mpaddr, pte, new_pte);
> >>
> >>      // dom0 tries to map real machine's I/O region, but failed.
> >>      // It is very likely that dom0 doesn't boot correctly because
> >
> > Hmmm, are you really sure that the above is SMP-safe?
> > We are touching p2m table locklessly so we must be extremely
> > careful. The above hunk split the atomic operation into two phase
> > and makes the following logic not make sense.
> >
> >
> 
> Yes, it is not SMP-safe there is lock for p2m.
> Modifying p2m is not a frequent operation, why not add a lock for it?

It is frequent to read p2m table. So lockless approach was adopted
for scalability.
It doesn't make sense to lock around only writer side.

thanks,
-- 
yamahata

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