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

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

On Wed, 2 Feb 2011, Konrad Rzeszutek Wilk wrote:
> > in which the same mfn can correspond to a normal mapping, a 1:1 mapping
> > or even a granted page, it is a good idea to check the m2p_override
> > *before* checking if the mfn is an identity mfn.
> > So that if there are two identical mfns, one granted and the other
> > one belonging to an identity mapping, we would return the pfn
> 
> How can you have that? Are you passing through a PCI device to
> a PV domain, making the PV domain have the netback/blkback device,
> and then granting those pages to another domain?

it is an hypothetical scenario: we have two domUs dA and dB; dA grants a
page P (the corresponding mfn will be called mfn_P) to dB.  mfn_P
happens to be normal valid ram in dA but in dB another mfn called mfn_Q
belonging to a 1:1 region exists so that mfn_P == mfn_Q.
I think this scenario is possible, we just need two domU with different
e820's.


> > let's suppose that pfn is garbage (0xFFFFF.. or 0x55555..), in this case
> 
> 0xFFFF or 0x5555 are not garbage. They are valid values. Garbage is the
> value zero. 0xFFFF.. has two meanings: It is a missing page that can be
> balloon-ed in, or it belongs to the DOMID_IO. The 0x5555.. means it is
> a shared DOMID_IO page.

I see, this is interesting.


> > ret = __get_user(pfn, &machine_to_phys_mapping[mfn]); /* ret is >= 0 and
> >                                                        pfn is 0x55555.. */
> > get_phys_to_machine(0x55555) != mfn /* true, I am assuming there are no
> >                                      valid p2m mappings at 0x55555.. */
> 
> > pfn = m2p_find_override_pfn(mfn, ~0); /* the m2p_ovverride doesn't find
> >                                          anything so it returns ~0 (the
> >                                          second argument), pfn == ~0 now */
> > if (pfn == ~0 && /* true */
> > 
> > 
> > maybe I should add a comment (and drink less caffeine)?
> 
> The first time I saw the patch I missed that you passed in 'mfn' to
> the second get_phys_to_machine .. Comments are good here I think.

I'll do.


> > 
> > 
> > > > +			get_phys_to_machine(mfn) == IDENTITY_FRAME(mfn))
> > > > +		pfn = mfn;
> > > 
> > > So for identity type mfns we end up calling 'get_phys_to_machine(mfn)' twice
> > > I think?
> > > 
> > > Would it make sense to save the result of 'get_phys_to_machine(mfn)'
> > > the first call?
> > 
> > the first call is get_phys_to_machine(pfn), with pfn potentially
> > garbage; this call is get_phys_to_machine(mfn).
> 
> I think what you are telling me is that it is pointless to check for
> the IDENTITY_FRAME b/c it won't happen often or at all.
> 
> So moving the code so that we do the hot-paths first makes more
> sense and we should structure the code as so, right?
> 

Yes.
My point is that it makes sense to check the m2p_override first
because it is more likely to get a valid result than checking for an
identity mapping.
Besides if both are available for the same mfn (case described above) we
want to return the m2p_override result here.


> I agree with that sentiment, do you want to prep another patch that
> has this patch and also some more comments?

yes, appended.

---


diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index be118d8..16ba2a8 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;
+	int ret = 0;
 
 	if (xen_feature(XENFEAT_auto_translated_physmap))
 		return mfn;
@@ -95,15 +96,29 @@ 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.
+	/* ret might be < 0 if there are no entries in the m2p for mfn */
+	if (ret < 0)
+		pfn = ~0;
+	else if (get_phys_to_machine(pfn) != mfn)
+		/*
+		 * 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.
+		 *
+		 * m2p_find_override_pfn returns ~0 if it doesn't find anything.
+		 */
+		pfn = m2p_find_override_pfn(mfn, ~0);
+
+	/* 
+	 * pfn is ~0 if there are no entries in the m2p for mfn or if the
+	 * entry doesn't map back to the mfn and m2p_override doesn't have a
+	 * valid entry for it.
 	 */
-	if (get_phys_to_machine(pfn) != mfn)
-		pfn = m2p_find_override_pfn(mfn, pfn);
+	if (pfn == ~0 &&
+			get_phys_to_machine(mfn) == IDENTITY_FRAME(mfn))
+		pfn = mfn;
 
 	return pfn;
 }