Re: Re: changeset 22526:7a5ee3800417

[George Dunlap <george.dunlap@citrix.com>]

On Mon, 2011-03-07 at 16:48 +0000, Tim Deegan wrote:
> Hi, 
> 
> At 16:41 +0000 on 07 Mar (1299516062), George Dunlap wrote:
> > diff -r 809ea782d2e6 -r 7ddd021a2079 xen/arch/x86/mm/hap/p2m-ept.c
> > --- a/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:33:45 2011 +0000
> > +++ b/xen/arch/x86/mm/hap/p2m-ept.c	Mon Mar 07 16:41:02 2011 +0000
> > @@ -390,7 +390,7 @@
> >              new_entry.access = p2ma;
> >              new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
> >  
> > -            if ( new_entry.mfn == mfn_x(mfn) )
> > +            if ( ept_entry->mfn == mfn_x(mfn) )
> 
> Better to use old_entry.mfn, in the spirit of the original cset
> ("access-once semantics")?  

I started to do that, but the one below didn't have an old_entry
already.  

> In fact, I suspect that to be safe, you need
> to do an atomic RMW instead of just an atomic set, and then decide
> whether the VT-d tables will need to be synced.

Are we not holding the p2m lock when writing entries?

Reading should be safe here if we are holding the p2m lock.  When
holding the lock, we only need write-once semantics (to avoid racing the
read-once semantics of someone reading without the lock).

 -George

> 
> Cheers,
> 
> Tim. 
> 
> >                  need_modify_vtd_table = 0;
> >              else
> >                  new_entry.mfn = mfn_x(mfn);
> > @@ -438,7 +438,7 @@
> >          new_entry.access = p2ma;
> >          new_entry.rsvd2_snp = (iommu_enabled && iommu_snoop);
> >  
> > -        if ( new_entry.mfn == mfn_x(mfn) )
> > +        if ( ept_entry->mfn == mfn_x(mfn) )
> >               need_modify_vtd_table = 0;
> >          else /* the caller should take care of the previous page */
> >              new_entry.mfn = mfn_x(mfn);
> 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
>