Re: [PATCH 2 of 3] x86/mm: Teach paging to page table-based p2m

["Andres Lagar-Cavilla" <andres@lagarcavilla.org>]

> At 22:15 -0500 on 29 Feb (1330553757), Andres Lagar-Cavilla wrote:
>> --- a/xen/arch/x86/mm/p2m-pt.c
>> +++ b/xen/arch/x86/mm/p2m-pt.c
>> @@ -53,6 +53,20 @@
>>  #define P2M_BASE_FLAGS \
>>          (_PAGE_PRESENT | _PAGE_USER | _PAGE_DIRTY | _PAGE_ACCESSED)
>>
>> +#ifdef __x86_64__
>> +/* l1e_from_pfn is not designed to have INVALID_MFN stored. The
>> 0xff..ff
>> + * value tramples over the higher-order bits used for flags (NX, p2mt,
>> + * etc.) This happens for paging entries. Thus we do this clip/unclip
>> + * juggle for l1 entries only (no paging superpages!) */
>> +#define EFF_MFN_WIDTH       (PADDR_BITS-PAGE_SHIFT) /* 40 bits */
>> +#define clipped_mfn(mfn)    ((mfn) & ((1UL << EFF_MFN_WIDTH) - 1))
>> +#define unclip_mfn(mfn)     (((mfn) == clipped_mfn(INVALID_MFN)) ? \
>> +                                INVALID_MFN : (mfn))
>> +#else
>> +#define clipped_mfn(mfn)    (mfn)
>> +#define unclip_mfn(mfn)     (mfn)
>> +#endif /* __x86_64__ */
>
> Hmmm.  If we need to have this, we should probably have it in the main
> l*_from_pfn and l*_get_pfn code rather than needing to scatter it around
> in the callers.  And we need a check in the e820 map to make sure we
> don't ever use MFN 0xffffffff (once CPUs start supporting it).
>
> The alternative would be to add another type so we don't have to store
> INVALID_MFN in p2m_ram_paging_in entries.

A lot of callers store INVALID_MFN into p2m entries (clear_mmio_p2m_entry,
p2m_remove_page, more). For all of them, as well as for paging eviction,
what matters is the type stored, not the mfn.

That is why retrieving the INVALID_MFN is not the problem. The ept code
itself clips the INVALID_MFN (typecast to bitfield in ept_entry union) and
everything seems to work fine when returning the clipped INVALID_MFN: no
one actually cares about that mfn.

Because of that, I believe it's neither necessary to unclip on the
extraction path, nor to take any special precautions in the e820.

But for p2m-pt, all the callers that set INVALID_MFN seem to work purely
by chance. In all cases INVALID_MFN will trample over the higher order
bits that are supposed to store the type. When reading the entry, the
p2m-pt code will not understand, default to p2m_mmio_dm type, and that
seems to be good enough (but not good enough when the type was supposed to
be paged_out).

So, I could submit one patch to clip the INVALID_MFN for p2m-pt in the 4k
page case of set_entry. That is the only place where we need it, and
avoids subtly changing semantics for a zillion other callers.

Or, we could change the paging code to store _mfn(0). This is the way of
PoD. I get the feeling George got lucky, or knew about this all along :)
The key, again, is not to trample over the high order bits.

Then, the rest of this patch, adding if's and changing asserts, can be
dealt with separately.

Thanks,
Andres

>
> Cheers,
>
> Tim.
>