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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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