Re: [PATCH] x86/EPT: relax iPAT for "invalid" MFNs

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 11, 2024 at 01:52:58PM +0200, Jan Beulich wrote:
> On 11.06.2024 13:08, Roger Pau Monné wrote:
> > On Tue, Jun 11, 2024 at 11:33:24AM +0200, Jan Beulich wrote:
> >> On 11.06.2024 11:02, Roger Pau Monné wrote:
> >>> On Tue, Jun 11, 2024 at 10:26:32AM +0200, Jan Beulich wrote:
> >>>> On 11.06.2024 09:41, Roger Pau Monné wrote:
> >>>>> On Mon, Jun 10, 2024 at 04:58:52PM +0200, Jan Beulich wrote:
> >>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
> >>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
> >>>>>> @@ -503,7 +503,8 @@ int epte_get_entry_emt(struct domain *d,
> >>>>>>  
> >>>>>>      if ( !mfn_valid(mfn) )
> >>>>>>      {
> >>>>>> -        *ipat = true;
> >>>>>> +        *ipat = type != p2m_mmio_direct ||
> >>>>>> +                (!is_iommu_enabled(d) && !cache_flush_permitted(d));
> >>>>>
> >>>>> Looking at this, shouldn't the !mfn_valid special case be removed, and
> >>>>> mfns without a valid page be processed normally, so that the guest
> >>>>> MTRR values are taken into account, and no iPAT is enforced?
> >>>>
> >>>> Such removal is what, in the post commit message remark, I'm referring to
> >>>> as "moving to too lax". Doing so might be okay, but will imo be hard to
> >>>> prove to be correct for all possible cases. Along these lines goes also
> >>>> that I'm adding the IOMMU-enabled and cache-flush checks: In principle
> >>>> p2m_mmio_direct should not be used when neither of these return true. Yet
> >>>> a similar consideration would apply to the immediately subsequent if().
> >>>>
> >>>> Removing this code would, in particular, result in INVALID_MFN getting a
> >>>> type of WB by way of the subsequent if(), unless the type there would
> >>>> also be p2m_mmio_direct (which, as said, it ought to never be for non-
> >>>> pass-through domains). That again _may_ not be a problem as long as such
> >>>> EPT entries would never be marked present, yet that's again difficult to
> >>>> prove.
> >>>
> >>> My understanding is that the !mfn_valid() check was a way to detect
> >>> MMIO regions in order to exit early and set those to UC.  I however
> >>> don't follow why the guest MTRR settings shouldn't also be applied to
> >>> those regions.
> >>
> >> It's unclear to me whether the original purpose of he check really was
> >> (just) MMIO. It could as well also have been to cover the (then not yet
> >> named that way) case of INVALID_MFN.
> >>
> >> As to ignoring guest MTRRs for MMIO: I think that's to be on the safe
> >> side. We don't want guests to map uncachable memory with a cachable
> >> memory type. Yet control isn't fine grained enough to prevent just
> >> that. Hence why we force UC, allowing merely to move to WC via PAT.
> > 
> > Would that be to cover up for guests bugs, or there's a coherency
> > reason for not allowing guests to access memory using fully guest
> > chosen cache attributes?
> 
> I think the main reason is that this way we don't need to bother thinking
> of whether MMIO regions may need caches flushed in order for us to be
> sure memory is all up-to-date. But I have no insight into what the
> original reasons here may have been.
> 
> > I really wonder whether Xen has enough information to figure out
> > whether a hole (MMIO region) is supposed to be accessed as UC or
> > something else.
> 
> It certainly hasn't, and hence is erring on the (safe) side of forcing
> UC.

Except that for the vesa framebuffer at least this is a bad choice :).

> > Your proposed patch already allows guest to set such attributes in
> > PAT, and hence I don't see why also taking guest MTRRs into account
> > would be any worse.
> 
> Whatever the guest sets in PAT, UC in EMT will win except fot the
> special case of WC.
> 
> >>>>> I also think this likely wants a:
> >>>>>
> >>>>> Fixes: 81fd0d3ca4b2 ('x86/hvm: simplify 'mmio_direct' check in 
> >>>>> epte_get_entry_emt()')
> >>>>
> >>>> Oh, indeed, I should have dug out when this broke. I didn't because I
> >>>> knew this mfn_valid() check was there forever, neglecting that it wasn't
> >>>> always (almost) first.
> >>>>
> >>>>> As AFAICT before that commit direct MMIO regions would set iPAT to WB,
> >>>>> which would result in the correct attributes (albeit guest MTRR was
> >>>>> still ignored).
> >>>>
> >>>> Two corrections here: First iPAT is a boolean; it can't be set to WB.
> >>>> And then what was happening prior to that change was that for the APIC
> >>>> access page iPAT was set to true, thus forcing WB there. iPAT was left
> >>>> set to false for all other p2m_mmio_direct pages, yielding (PAT-
> >>>> overridable) UC there.
> >>>
> >>> Right, that behavior was still dubious to me, as I would assume those
> >>> regions would also want to fetch the type from guest MTRRs.
> >>
> >> Well, for the APIC access page we want to prevent it becoming UC. It's MMIO
> >> from the guest's perspective, yet _we_ know it's really ordinary RAM. For
> >> actual MMIO see above; the only case where we probably ought to respect
> >> guest MTRRs is when they say WC (following from what I said further up).
> >> Yet that's again an independent change to (possibly) make.
> > 
> > For emulated devices we might map regular RAM into what the guest
> > otherwise thinks it's MMIO.
> 
> Right, and for non-pass-through domains we force everything to WB already.
> 
> >  Maybe the mfn_valid() check should be
> > inverted, and return WB when the underlying mfn is RAM, and otherwise
> > use the guest MTRRs to decide the cache attribute?
> 
> First: Whether WB is correct for RAM isn't known. With some peculiar device
> assigned, the guest may want/need part of its RAM be e.g. WC or WT. (It's
> only without any physical devices assigned that we can be quite sure that
> WB is good for all of RAM.) Therefore, second, I think respecting MTRRs for
> RAM is less likely to cause problems than respecting them for MMIO.
> 
> I think at this point the main question is: Do we want to do things at least
> along the lines of this v1, or do we instead feel certain enough to switch
> the mfn_valid() to a comparison against INVALID_MFN (and perhaps moving it
> up to almost the top of the function)?

My preferred option would be the later, as that would remove a special
casing.  However, I'm unsure how much fallout this could cause - those
caching changes are always tricky and lead to unexpected fallout.

OTOH the current !mfn_valid() check is very restrictive, as it forces
all MMIO to UC.  So by removing it we allow guest chosen types to take
effect, which are likely less restrictive than UC (whether those are
correct is another question).

> One caveat here that I forgot to
> mention before: MFNs taken out of EPT entries will never be INVALID_MFN, for
> the truncation that happens when populating entries. In that case we rely on
> mfn_valid() to be "rejecting" them.

The only caller where mfns from EPT entries are passed to
epte_get_entry_emt() is in resolve_misconfig() AFAICT, and in that
case the EPT entry must be present for epte_get_entry_emt() to be
called.  So it seems to me that epte_get_entry_emt() can never be
called from an mfn constructed from an INVALID_MFN EPT entry (but it's
worth adding an assert for it).

Thanks, Roger.