Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.11.2022 21:56, Andrew Cooper wrote:
> On 29/11/2022 14:55, Jan Beulich wrote:
>> By defining the constant to zero when !SHADOW_PAGING we give compilers
>> the chance to eliminate a little more dead code elsewhere in the tree.
>> Plus, as a minor benefit, the general reference count can be one bit
>> wider. (To simplify things, have PGC_page_table change places with
>> PGC_extra.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Ahead of making this change, can we please rename it to something less
> confusing, and fix it's comment which is wrong.
> 
> PGC_shadowed_pt is the best I can think of.

Can do, sure.

>> ---
>> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
>> no-op even prior to this change when !SHADOW_PAGING, which can't be
>> quite right. If (guest) page tables are relevant to include in the
>> verification, shouldn't this look for PGT_l<N>_page_table as well? How
>> to deal with HAP guests there is entirely unclear.
> 
> Considering the caller, it MACs every domheap page for domains with
> CDF_s3_integrity.
> 
> The tboot logical also blindly assumes that any non-idle domain has an
> Intel IOMMU context with it.  This only doesn't (trivially) expose
> because struct domain_iommu is embedded in struct domain (rather than
> allocated separately), and reaching into the wrong part of the arch
> union is only mitigated by the tboot logic not being invoked on
> non-intel systems.  (Also the idle domain check is useless, given that
> it's in a for_each_domain() loop).
> 
> It does look a little like the caller is wanting to MAC all Xen data
> that describes the guest, but doing this unilaterally for all shadowed
> guests seems wrong beside the per-domain s3_integrity setting.

Question is - do we care about addressing this (when, as said, it's
unclear how to deal with HAP domains; maybe their actively used p2m
pages would need including instead)? Or should we rather consider
ripping out tboot support?

Jan