Re: [PATCH RFC 3/3] x86/mm: Simplify/correct l1_disallow_mask()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2024 18:14, Andrew Cooper wrote:
> l1_disallow_mask() yields L1_DISALLOW_MASK with PAGE_CACHE_ATTRS conditionally
> permitted.  First, rewrite it as a plain function.
> 
> Next, correct some dubious behaviours.
> 
> The use of is_pv_domain() is tautological; l1_disallow_mask() is only used in
> the PV pagetable code, and it return true for system domains as well.

Well, only quite. What you say is entirely true for mod_l1_entry(), but sadly
not for get_page_from_l1e(): That's also used by shadow_get_page_from_l1e(),
i.e. theoretically from a PVH Dom0 running in shadow mode. I don't think I
can spot anywhere we'd reject that combination.

> The use of has_arch_pdevs() is wonky; by making the use of cache attributes
> dependent on the instantanious property of whether any devices is assigned,
> means that a guest could have created a legal PTE which will fail validation
> at a later point when the device has been removed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> RFC.  I've not tested this, and I doubt it will work to start with owing to
> the removal of the dom_io special case which IIRC dom0 uses to map arbitrary
> MMIO.

I think that check has been dead for a long time. DomIO can't be the page
table owner; it can only be the owner of a page to be mapped. The check
likely was needed only before the proper splitting of which domain involved
plays which role. Dropping this may then want to be done as a separate
change, but it could of course remain here as long as properly explained in
the description. Maybe for the time being we want to retain an assertion to
this effect.

> Furthermore, the rangeset_is_empty() calls have the same problem that
> has_arch_pdevs() has; they're not invariants on the domain.  Also, VMs
> wanting/needing encrypted memory need fully working cacheability irrespective
> of device assignment.
> 
> I expect the way we actually want to fix this is to have a CDF flag for
> allowing reduced cahcebility, and for this expression to simplify to just:
> 
>     if ( d->options & XEN_DOMCTL_CDF_any_cacheability )
>         disallow &= ~PAGE_CACHE_ATTRS;
> 
> which is simpler still.

Perhaps. Or refuse altering the two rangesets post-creation for domains
without IOMMUs ("post-creation" here including the establishing of any
mapping on behalf of the domain).

Jan