All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
@ 2012-08-15  6:57 Xudong Hao
  2012-08-15  9:21 ` Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Xudong Hao @ 2012-08-15  6:57 UTC (permalink / raw)
  To: xen-devel; +Cc: Xudong Hao, tim, Xiantao Zhang

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;

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-15  6:57 [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid Xudong Hao
@ 2012-08-15  9:21 ` Jan Beulich
  2012-08-16 10:05   ` Hao, Xudong
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2012-08-15  9:21 UTC (permalink / raw)
  To: Xudong Hao; +Cc: tim, Xiantao Zhang, xen-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-15  9:21 ` Jan Beulich
@ 2012-08-16 10:05   ` Hao, Xudong
  2012-08-16 10:12     ` Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Hao, Xudong @ 2012-08-16 10:05 UTC (permalink / raw)
  To: Jan Beulich; +Cc: tim, Zhang, Xiantao, xen-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-16 10:05   ` Hao, Xudong
@ 2012-08-16 10:12     ` Jan Beulich
  2012-08-16 10:31       ` Hao, Xudong
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2012-08-16 10:12 UTC (permalink / raw)
  To: Xudong Hao; +Cc: tim, Xiantao Zhang, xen-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-16 10:12     ` Jan Beulich
@ 2012-08-16 10:31       ` Hao, Xudong
  2012-08-16 10:41         ` Jan Beulich
  2012-08-16 19:14         ` Mukesh Rathor
  0 siblings, 2 replies; 11+ messages in thread
From: Hao, Xudong @ 2012-08-16 10:31 UTC (permalink / raw)
  To: Jan Beulich; +Cc: tim, Zhang, Xiantao, xen-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-16 10:31       ` Hao, Xudong
@ 2012-08-16 10:41         ` Jan Beulich
  2012-08-16 11:01           ` Tim Deegan
  2012-08-16 19:14         ` Mukesh Rathor
  1 sibling, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2012-08-16 10:41 UTC (permalink / raw)
  To: Xudong Hao, tim; +Cc: Xiantao Zhang, xen-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-16 10:41         ` Jan Beulich
@ 2012-08-16 11:01           ` Tim Deegan
  0 siblings, 0 replies; 11+ messages in thread
From: Tim Deegan @ 2012-08-16 11:01 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xudong Hao, Xiantao Zhang, xen-devel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-16 10:31       ` Hao, Xudong
  2012-08-16 10:41         ` Jan Beulich
@ 2012-08-16 19:14         ` Mukesh Rathor
  1 sibling, 0 replies; 11+ messages in thread
From: Mukesh Rathor @ 2012-08-16 19:14 UTC (permalink / raw)
  To: Hao, Xudong; +Cc: tim, Zhang, Xiantao, Jan Beulich, xen-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
  2012-08-16 17:17 ` Andres Lagar-Cavilla
@ 2012-08-16 17:47   ` Tim Deegan
  0 siblings, 0 replies; 11+ messages in thread
From: Tim Deegan @ 2012-08-16 17:47 UTC (permalink / raw)
  To: Andres Lagar-Cavilla; +Cc: xudong.hao, xiantao.zhang, Jan Beulich, xen-devel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
       [not found] <mailman.10813.1345115327.1399.xen-devel@lists.xen.org>
@ 2012-08-16 17:17 ` Andres Lagar-Cavilla
  2012-08-16 17:47   ` Tim Deegan
  0 siblings, 1 reply; 11+ messages in thread
From: Andres Lagar-Cavilla @ 2012-08-16 17:17 UTC (permalink / raw)
  To: xen-devel; +Cc: xudong.hao, tim, xiantao.zhang, Jan Beulich

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid
@ 2012-08-15  6:55 Xudong Hao
  0 siblings, 0 replies; 11+ messages in thread
From: Xudong Hao @ 2012-08-15  6:55 UTC (permalink / raw)
  To: xen-devel; +Cc: , Xudong Hao, tim, Xiantao Zhang

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;

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-08-16 19:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-15  6:57 [PATCH 2/3] xen/p2m: Using INVALID_MFN instead of mfn_valid Xudong Hao
2012-08-15  9:21 ` Jan Beulich
2012-08-16 10:05   ` Hao, Xudong
2012-08-16 10:12     ` Jan Beulich
2012-08-16 10:31       ` Hao, Xudong
2012-08-16 10:41         ` Jan Beulich
2012-08-16 11:01           ` Tim Deegan
2012-08-16 19:14         ` Mukesh Rathor
     [not found] <mailman.10813.1345115327.1399.xen-devel@lists.xen.org>
2012-08-16 17:17 ` Andres Lagar-Cavilla
2012-08-16 17:47   ` Tim Deegan
  -- strict thread matches above, loose matches on Subject: below --
2012-08-15  6:55 Xudong Hao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.