Re: [Xen-devel] [PATCH] xen/mm: Alter is_iomem_page() to use mfn_t

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, February 06, 2017 11:38 PM
> 
> >>> On 06.02.17 at 15:48, <dwmw2@xxxxxxxxxxxxx> wrote:
> > On Mon, 2017-02-06 at 07:26 -0700, Jan Beulich wrote:
> >> >
> >> > >
> >> > > >
> >> > > > On 06.02.17 at 14:55, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> > Switch its return type to bool to match its use, and simplify the
> >> > ARM
> >> > implementation slightly.
> >> >
> >> > No functional change.
> >> >
> >> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> And perhaps that's what should be used in epte_get_entry_emt()
> >> instead of !mfn_valid() in David's patch. David, would you be okay
> >> with your patch changed to that effect upon commit?
> >
> > I don't think that works, at least not literally
> > s/!mfn_valid()/is_iomem_page()/
> >
> > In my patch, mfn_valid() is checked *after* we've processed the
> > 'direct_mmio' case that all MMIO should hit. In a sane world I think
> > it's *only* actually catching INVALID_MFN, and probably should never
> > match on any other value of mfn.
> >
> > I don't quite understand why we pass 'direct_mmio' in as a separate
> > argument. Perhaps there's scope for doing a sanity check that
> > 'direct_mmio == is_iomem_page(mfn)' — because when would that *not* be
> > true?
> 
> Likely never. The reasons for why this was done the way it is
> escape me. Adding VMX maintainers once again.

I'm not the original author of that code. Here is what I found from the code:

There are two places caring direct_mmio: resolve_misconfig and ept_
set_entry. It's decided upon p2m type:

      int emt = epte_get_entry_emt(p2m->domain, gfn, _mfn(e.mfn),
                                    level * EPT_TABLE_ORDER, &ipat,
                                    e.sa_p2mt == p2m_mmio_direct);

I'm not sure whether p2m_mmio_direct always makes is_iomem_page
true. If true then yes we may not require this parameter at all.

> 
> > But sanity checks are of dubious utility because it must be noted that
> > we have no way to return failure for order==0 anyway; that 'return -1'
> > idiom is dangerous in epte_get_entry_emt(), and that's why I gave it an
> > extra comment.
> >
> > So if we can use is_iomem_page() do we ditch the direct_mmio argument
> > to the function entirely?
> 
> I guess we could, yes. I didn't carefully check though yet what the
> caller(s) derive(s) it from.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel