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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> 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