Re: [PATCH v2 for-4.19 3/3] x86/EPT: drop questionable mfn_valid() from epte_get_entry_emt()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2024 17:00, Roger Pau Monné wrote:
> On Wed, Jun 12, 2024 at 03:17:38PM +0200, Jan Beulich wrote:
>> mfn_valid() is RAM-focused; it will often return false for MMIO. Yet
>> access to actual MMIO space should not generally be restricted to UC
>> only; especially video frame buffer accesses are unduly affected by such
>> a restriction.
>>
>> Since, as of ???????????? ("x86/EPT: avoid marking non-present entries
>> for re-configuring"), the function won't be called with INVALID_MFN or,
>> worse, truncated forms thereof anymore, we call fully drop that check.
>>
>> Fixes: 81fd0d3ca4b2 ("x86/hvm: simplify 'mmio_direct' check in 
>> epte_get_entry_emt()")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> I do think this is the way to go (removing quirks from
> epte_get_entry_emt()), however it's a risky change to make at this
> point in the release.
> 
> If this turns out to cause some unexpected damage, it would only
> affect HVM guests with PCI passthrough and PVH dom0, which I consider
> not great, but tolerable.
> 
> I would be more comfortable with making the change just not so close
> to the release, but that's where we are.

Certainly, and I could live with Oleksii revoking his R-a-b (or simply
not offering it for either of the two prereq changes). Main thing for
me is - PVH Dom0 finally isn't so horribly slow anymore. However, if it
doesn't go into the release, then I'd also be unsure about eventual
backporting.

> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> I wonder if you should explicitly mention that if adding the
> mfn_valid() check was done to ensure all mappings to MMIO are created
> with effective UC caching attribute it won't be fully correct either.
> Xen could map those using a different effective caching attribute by
> virtue of host MTRRs being in effect plus Xen chosen PAT attributes.

Well, the mfn_valid() can't have been there to cover _all_ MMIO. It was
maybe a flawed initial attempt at doing so, and then wasn't properly
adjusted / dropped. So overall - no, I don't think extending the
description with anything along the lines of the above would make a lot
of sense.

Jan