Re: [V10 PATCH 3/4] pvh dom0: Add and remove foreign pages

["Jan Beulich" <JBeulich@suse.com>]

>>> On 03.05.14 at 01:35, <mukesh.rathor@oracle.com> wrote:
> On Fri, 2 May 2014 10:55:55 +0200
> Tim Deegan <tim@xen.org> wrote:
>> At 18:45 -0700 on 01 May (1398966318), Mukesh Rathor wrote:
>> > On Thu, 1 May 2014 18:19:08 +0200
>> > Tim Deegan <tim@xen.org> wrote:
>> > > At 18:06 -0700 on 29 Apr (1398791207), Mukesh Rathor wrote:
>> > > > diff --git a/xen/arch/x86/mm/p2m-ept.c
>> > > > b/xen/arch/x86/mm/p2m-ept.c index c0bfc50..11474e8 100644
>> > > > --- a/xen/arch/x86/mm/p2m-ept.c
>> > > > +++ b/xen/arch/x86/mm/p2m-ept.c
>> > > > @@ -36,8 +36,6 @@
>> > > >  
>> > > >  #define
>> > > > atomic_read_ept_entry(__pepte)                              \
>> > > > ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } )
>> > > > -#define atomic_write_ept_entry(__pepte,
>> > > > __epte)                     \
>> > > > -    write_atomic(&(__pepte)->epte, (__epte).epte)
>> > > >  
>> > > >  #define is_epte_present(ept_entry)      ((ept_entry)->epte &
>> > > > 0x7) #define is_epte_superpage(ept_entry)    ((ept_entry)->sp)
>> > > > @@ -46,6 +44,46 @@ static inline bool_t
>> > > > is_epte_valid(ept_entry_t *e) return (e->epte != 0 &&
>> > > > e->sa_p2mt != p2m_invalid); }
>> > > >  
>> > > > +/* returns : 0 for success, -errno otherwise */
>> > > > +static int atomic_write_ept_entry(ept_entry_t *entryptr,
>> > > > ept_entry_t new,
>> > > > +                                  int level)
>> > > > +{
>> > > > +    bool_t same_mfn = (new.mfn == entryptr->mfn);
>> > > > +    unsigned long oldmfn = INVALID_MFN;
>> > > > +
>> > > > +    if ( level )
>> > > > +    {
>> > > 
>> > > ASSERT(!(new.sp && p2m_is_foreign(new.sa_p2mt))) here?
>> > 
>> > Yeah, I debated adding p2m_is_foreign ASSERT, but didn't because
>> > iirc Jan had said he wanted to use up the non-leaf bits for
>> > something else in future. 
>> 
>> Well, if new.sp is set it's not non-leaf.  Though I suppose maybe the
>> test should be for _valid_ + superpage + foreign.
> 
> Ok, so looks like:
> 
>     ASSERT(new.sa_p2mt == p2m_invalid || 
>            !(new.sp && p2m_is_foreign(new.sa_p2mt)));
> 
> should do it?

Decoding this to

    ASSERT(new.sa_p2mt == p2m_invalid || !new.sp || !p2m_is_foreign(new.sa_p2mt));

makes already clear that the first check is redundant with the last one.
I.e. just

    ASSERT(!new.sp || !p2m_is_foreign(new.sa_p2mt));

would seem to suffice, but then again wouldn't cover intermediate
levels (which right not appears to not be a problem).

Jan