Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid

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

>
> At 11:41 +0100 on 16 Aug (1345117281), Jan Beulich wrote:
>> >>> On 16.08.12 at 12:31, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> >> >> >>> On 15.08.12 at 08:57, Xudong Hao <xudong.hao@intel.com> wrote:
>> >> >> > --- a/xen/arch/x86/mm/p2m-ept.c	Tue Jul 24 17:02:04 2012 +0200
>> >> >> > +++ b/xen/arch/x86/mm/p2m-ept.c	Thu Jul 26 15:40:01 2012 +0800
>> >> >> > @@ -428,7 +428,7 @@ ept_set_entry(struct p2m_domain *p2m, un
>> >> >> >      }
>> >> >> >
>> >> >> >      /* Track the highest gfn for which we have ever had a valid
>> mapping
>> >> */
>> >> >> > -    if ( mfn_valid(mfn_x(mfn)) &&
>> >> >> > +    if ( (mfn_x(mfn) != INVALID_MFN) &&
>> >> >> >           (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>> >> >> >          p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>> >> >>
>> >> >> Depending on how the above comment gets addressed (i.e.
>> >> >> whether MMIO MFNs are to be considered here at all), this
>> >> >> might need changing anyway, as this a huge max_mapped_pfn
>> >> >> value likely wouldn't be very useful anymore.
>> >> >
>> >> > Your viewpoint is similar with us. Here max_mapped_pfn value is for
>> memory
>> >> > but not for MMIO. I think this is a simple changes, do you have
>> another
>> >> > suggestion?
>> >>
>> >> The question is why this needs to be changed at all. If this is
>> >> only about RAM, then mfn_valid() is the right thing to use. If
>> >> this is about MMIO too, then the condition is wrong already
>> >> (since, as we appear to agree, even now there can be MMIO
>> >> above RAM, provided there's little enough RAM).
>> >>
>> >
>> > The original code considered EPT only, now for the device assignment,
>> it
>> > need to consider MMIO. So how about remove the mfn_valid() here?
>>
>> I don't think it's there without reason, but I'm not sure. Tim?
>
> max_mapped_pfn should be the highest entry that's even had a mapping in
> the p2m.  Its intent was to provide a fast path exit from p2m lookups in
> the (at the time) common case where _emulated_ MMIO addresses were
> higher than all the actual p2m mappings, and the cost of a failed lookup
> (on 32-bit) was a page fault in the linear map.  Also, at the time, the
> p2m wasn't typed and we didn't support direct MMIO, so mfn_valid() was
> equivalent to 'entry is present'.
>
> These days, I'm not sure how useful max_mapped_pfn is, since (a) for any
> VM with >3GB RAM the emulated MMIO lookups are not avoided, and (b) on
> 64-bit builds there's not pagefault for a failed lookup.  Also it seems to
> have been abused in a few places to do for() loops that touch every PFN
> instead of just walking the tries.  So I might get rid of it after 4.2
> is out.

max_mapped_pfn also helps keep XENMEM_maximum_gpfn O(1).
Andres

>
> In the meantime, the patch at the top of this thread is definitely an
> improvement.  However, I think this is a better fix:
>
> diff -r c887c30a0a35 xen/arch/x86/mm/p2m-ept.c
> --- a/xen/arch/x86/mm/p2m-ept.c	Thu Aug 16 10:16:19 2012 +0200
> +++ b/xen/arch/x86/mm/p2m-ept.c	Thu Aug 16 11:57:44 2012 +0100
> @@ -428,7 +428,7 @@ ept_set_entry(struct p2m_domain *p2m, un
>      }
>
>      /* Track the highest gfn for which we have ever had a valid mapping
> */
> -    if ( mfn_valid(mfn_x(mfn)) &&
> +    if ( p2mt != p2m_invalid &&
>           (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>          p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>
> diff -r c887c30a0a35 xen/arch/x86/mm/p2m-pt.c
> --- a/xen/arch/x86/mm/p2m-pt.c	Thu Aug 16 10:16:19 2012 +0200
> +++ b/xen/arch/x86/mm/p2m-pt.c	Thu Aug 16 11:57:44 2012 +0100
> @@ -454,7 +454,7 @@ p2m_set_entry(struct p2m_domain *p2m, un
>      }
>
>      /* Track the highest gfn for which we have ever had a valid mapping
> */
> -    if ( mfn_valid(mfn)
> +    if ( p2mt != p2m_invalid
>           && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
>          p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;
>
> and I'll commit it this afternoon or tomorrow.
>
> Tim.
>