Xen devel - Aug 2012 - [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid

64 bits big bar''s MMIO address may out of the highest gfn, then
mfn_valid
may return failure, so using INVALID_MFN to measure.

Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
Signed-off-by: Xudong Hao <xudong.hao@intel.com>

diff -r 663eb766cdde xen/arch/x86/mm/p2m-ept.c
--- 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;

64 bits big bar''s MMIO address may out of the highest gfn, then
mfn_valid
may return failure, so using INVALID_MFN to measure.

Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
Signed-off-by: Xudong Hao <xudong.hao@intel.com>

diff -r 663eb766cdde xen/arch/x86/mm/p2m-ept.c
--- 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;

>>> On 15.08.12 at 08:57, Xudong Hao <xudong.hao@intel.com>
wrote:
> 64 bits big bar''s MMIO address may out of the highest gfn, then
mfn_valid
> may return failure, so using INVALID_MFN to measure.
Hmm, that can be true for 32-bit BARs too (on systems with less
than 4Gb).
> Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> 
> diff -r 663eb766cdde xen/arch/x86/mm/p2m-ept.c
> --- 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.

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, August 15, 2012 5:22 PM
> To: Hao, Xudong
> Cc: Zhang, Xiantao; xen-devel@lists.xen.org; tim@xen.org
> Subject: Re: [Xen-devel] [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of
> mfn_valid
> 
> >>> On 15.08.12 at 08:57, Xudong Hao <xudong.hao@intel.com>
wrote:
> > 64 bits big bar''s MMIO address may out of the highest gfn,
then mfn_valid
> > may return failure, so using INVALID_MFN to measure.
> 
> Hmm, that can be true for 32-bit BARs too (on systems with less
> than 4Gb).
> 
Exactly right, thanks comments.
> > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> >
> > diff -r 663eb766cdde xen/arch/x86/mm/p2m-ept.c
> > --- 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.
> 
Jan,

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?
> Jan
>

>>> On 16.08.12 at 12:05, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> 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).

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, August 16, 2012 6:12 PM
> To: Hao, Xudong
> Cc: Zhang, Xiantao; xen-devel@lists.xen.org; tim@xen.org
> Subject: RE: [Xen-devel] [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of
> mfn_valid
> 
> >>> On 16.08.12 at 12:05, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >>> 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?
> Jan

>>> On 16.08.12 at 12:31, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, August 16, 2012 6:12 PM
>> To: Hao, Xudong
>> Cc: Zhang, Xiantao; xen-devel@lists.xen.org; tim@xen.org 
>> Subject: RE: [Xen-devel] [PATCH 2/3] xen/p2m: Using INVALID_MFN instead
of
>> mfn_valid
>> 
>> >>> On 16.08.12 at 12:05, "Hao, Xudong"
<xudong.hao@intel.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> >>> 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?

Jan

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.

>
> 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.
>

At 10:17 -0700 on 16 Aug (1345112267), Andres Lagar-Cavilla
wrote:
> > 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).
Ergh, true.  With a little tidying up in the tree update code (to
eliminate empty nodes) it will still be O(1) - just walking
rightmost-first down a trie of fixed height.  But that''s a little
more work than I hoped for. :)

Tim.

On Thu, 16 Aug 2012 10:31:30 +0000
"Hao, Xudong" <xudong.hao@intel.com> wrote:
> > */
> > >> > -    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;
> > >>

BTW, here''s the change in my PVH/hybrid tree that Tim D had suggested 
couple months ago:

     /* Track the highest gfn for which we have ever had a valid mapping */
     -    if ( mfn_valid(mfn_x(mfn)) &&
     +    if ( p2mt != p2m_invalid && p2mt != p2m_mmio_dm &&
               (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
	                p2m->max_mapped_pfn = gfn + (1UL << order) - 1;

thanks,
Mukesh