Re: [PATCH 11/11] xen/m2p: Check whether the MFN has IDENTITY_FRAME bit set..

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Mon, 31 Jan 2011, Konrad Rzeszutek Wilk wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> If we have the IDENTITY_FRAME bit set from the P2M, there
> is no point in looking up MFN in the M2P override. This is
> b/c the MFN is a physical MFN.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/include/asm/xen/page.h |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index ed46ec2..e6f7f37 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -81,6 +81,7 @@ static inline int phys_to_machine_mapping_valid(unsigned long pfn)
>  static inline unsigned long mfn_to_pfn(unsigned long mfn)
>  {
>  	unsigned long pfn;
> +	unsigned long p2m_mfn;
>  
>  	if (xen_feature(XENFEAT_auto_translated_physmap))
>  		return mfn;
> @@ -102,7 +103,12 @@ try_override:
>  	 * doesn't map back to the mfn), then check the local override
>  	 * table to see if there's a better pfn to use.
>  	 */
> -	if (get_phys_to_machine(pfn) != mfn)
> +	p2m_mfn = get_phys_to_machine(pfn);
> +
> +	if (p2m_mfn == IDENTITY_FRAME(mfn))
> +		return pfn;
> +
> +	if (p2m_mfn != mfn)
>  		pfn = m2p_find_override_pfn(mfn, pfn);
>  
>  	return pfn;
 

I have been thinking some more about it and now I have few questions:

1) is it possible for a single domain to have a valid mfn with the same
number as an identity mfn (apart from the IDENTITY_FRAME bit)?

2) is it true that mfn_to_pfn should never be called passing an identity
mfn (because we set _PAGE_IOMAP)?

3) what is the value returned by m2p(identity_mfn)? Is it a correct pfn
or may be something like 0xfffff or 0xeeeee?


These are my guessed answers:

1) yes, it is possible.
For example mfn=0xc0100 might be a valid ram mfn present in the mfn_list
of a domU and also be present as 1:1 mfn of the 3G-4G region.
For this reason I think we should look in m2p_override first and check
for possible identity mapping later.
We might want to avoid these situations but the only way I can see to do
it would be to make sure that the 1:1 regions are always subset of
the host reserved regions, even for domUs.

2) yes indeed.
One more reason to look in the m2p_override first.

3) the returned pfn might be 0xfffff or 0xeeeee.
We should use the mfn value directly as pfn value to check for possible
identity mappings.


The resulting patch looks like the following:

---


diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index ed46ec2..7f9bae2 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -80,6 +80,7 @@ static inline int phys_to_machine_mapping_valid(unsigned long pfn)
 
 static inline unsigned long mfn_to_pfn(unsigned long mfn)
 {
+	int ret = 0;
 	unsigned long pfn;
 
 	if (xen_feature(XENFEAT_auto_translated_physmap))
@@ -95,15 +96,21 @@ static inline unsigned long mfn_to_pfn(unsigned long mfn)
 	 * In such cases it doesn't matter what we return (we return garbage),
 	 * but we must handle the fault without crashing!
 	 */
-	__get_user(pfn, &machine_to_phys_mapping[mfn]);
+	ret = __get_user(pfn, &machine_to_phys_mapping[mfn]);
 try_override:
 	/*
 	 * If this appears to be a foreign mfn (because the pfn
 	 * doesn't map back to the mfn), then check the local override
 	 * table to see if there's a better pfn to use.
 	 */
-	if (get_phys_to_machine(pfn) != mfn)
-		pfn = m2p_find_override_pfn(mfn, pfn);
+	if (ret < 0)
+		pfn = ~0;
+	else if (get_phys_to_machine(pfn) != mfn)
+		pfn = m2p_find_override_pfn(mfn, ~0);
+
+	if (pfn == ~0 &&
+			get_phys_to_machine(mfn) == IDENTITY_FRAME(mfn))
+		pfn = mfn;
 
 	return pfn;
 }