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

[Tim Deegan <tim@xen.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.

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.