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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.02.17 at 08:31, <kevin.tian@xxxxxxxxx> wrote:
>>  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.

From an abstract perspective the two should always correlate. We
may want to take an intermediate step though and first only add
checking, and perhaps a release later remove the extra parameter
if the check never triggered for anyone.

Jan

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