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

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

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?

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?

-- 
dwmw2

Attachment: smime.p7s
Description: S/MIME cryptographic signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel