WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] Re: [PATCH 11/11] xen/m2p: Check whether the MFN has IDE

To: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] Re: [PATCH 11/11] xen/m2p: Check whether the MFN has IDENTITY_FRAME bit set..
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Wed, 2 Feb 2011 11:43:07 -0500
Cc: "jeremy@xxxxxxxx" <jeremy@xxxxxxxx>, "Xen-devel@xxxxxxxxxxxxxxxxxxx" <Xen-devel@xxxxxxxxxxxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>, "konrad@xxxxxxxxxx" <konrad@xxxxxxxxxx>, "hpa@xxxxxxxxx" <hpa@xxxxxxxxx>
Delivery-date: Wed, 02 Feb 2011 08:45:41 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <alpine.DEB.2.00.1102021128350.7277@kaball-desktop>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1296513876-31415-1-git-send-email-konrad.wilk@xxxxxxxxxx> <1296513876-31415-12-git-send-email-konrad.wilk@xxxxxxxxxx> <alpine.DEB.2.00.1102011513190.7277@kaball-desktop> <20110201202923.GC18656@xxxxxxxxxxxx> <alpine.DEB.2.00.1102021128350.7277@kaball-desktop>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.20 (2009-06-14)
> > Not sure I understand.
> 
> Considering that getting in this function with an identity mfn passed as
> argument shouldn't happen at all, and considering that there are cases

True, unless a developer wants to check that p2m(m2p(mfn))=mfn and 
supplies the MFNs that cover the PCI BAR spaces.. but that is mostly
done when trying to troubleshoot something and normal folks won't do this.


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

> corresponding to the granted mfn, that is the right answer because we
> shouldn't even be here if the argument was an identity mfn.
> 
> 
> > > 3) the returned pfn might be 0xfffff or 0xeeeee.

>From the machine_to_phys_mapping array. OK.
> > > We should use the mfn value directly as pfn value to check for possible
> > > identity mappings.
> > 
> > Aren't we doing that via 'get_phys_to_machine' ? It returns the value
> > and if it has the IDENTITY_FRAME_BIT it is an identity.
> > 
> > Or are you thinking of abolishing the IDENTITY_FRAME_BIT and check the
> > M2P in conjunction with the P2M to see if the mfn is a 1-1 mapping?
> > 
> 
> Not at all.
> The problem is the following:
> 
> pfn = m2p(mfn);

You are assuming that the 'mfn' is outside the scope off our machine_to_phys 
array
or that it is "garbage", such as 0xdeadbeef for example, I think.

In which case the pfn value is either 0xdeadbeef or INVALID_P2M_ENTRY.

If you provide that to get_phys_to_machine(0xdeadbeef) you get 
INVALID_P2M_ENTRY,
and you would call:
  pfn = m2p_find_override_pfn(mfn, pfn);

So we still end up checking the M2P override. If the pfn supplied
was INVALID_P2M_ENTRY, you would get the same value and still check the P2M 
override.


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

> get_phys_to_machine(pfn) will return INVALID_P2M_ENTRY that is not what
> we want.
> So we check for
> 
> get_phys_to_machine(mfn) == IDENTITY_FRAME(mfn)
>                     ---
> if it is true than it means that the mfn passed as argument belongs to
> an identity mapping.

<nods> Yup.
> 
> 
> 
> > > 
> > > 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 &&
> > 
> > You should also check for 0x55555... then.
> 
> that is not necessary because pfn == ~0 at this point, this is the code
> path:
> 
> 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.
> 
> 
> > > +                 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?

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel