Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for TDX shared bit of GPA

[Huang, Kai]

On 15/05/2024 12:59 pm, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
>
> Introduce a "gfn_shared_mask" field in the kvm_arch structure to record GPA
> shared bit and provide address conversion helpers for TDX shared bit of
> GPA.
>
> TDX designates a specific GPA bit as the shared bit, which can be either
> bit 51 or bit 47 based on configuration.
>
> This GPA shared bit indicates whether the corresponding physical page is
> shared (if shared bit set) or private (if shared bit cleared).
>
> - GPAs with shared bit set will be mapped by VMM into conventional EPT,
>    which is pointed by shared EPTP in TDVMCS, resides in host VMM memory
>    and is managed by VMM.
> - GPAs with shared bit cleared will be mapped by VMM firstly into a
>    mirrored EPT, which resides in host VMM memory. Changes of the mirrored
>    EPT are then propagated into a private EPT, which resides outside of host
>    VMM memory and is managed by TDX module.
>
> Add the "gfn_shared_mask" field to the kvm_arch structure for each VM with
> a default value of 0. It will be set to the position of the GPA shared bit
> in GFN through TD specific initialization code.
>
> Provide helpers to utilize the gfn_shared_mask to determine whether a GPA
> is shared or private, retrieve the GPA shared bit value, and insert/strip
> shared bit to/from a GPA.

I am seriously thinking whether we should just abandon this whole 
kvm_gfn_shared_mask() thing.

We already have enough mechanisms around private memory and the mapping 
of it:

1) Xarray to query whether a given GFN is private or shared;
2) fault->is_private to indicate whether a faulting address is private 
or shared;
3) sp->is_private to indicate whether a "page table" is only for private 
mapping;

Consider this as 4) -- I also like to have a kvm->arch.has_mirrored_pt 
(or a better name) as I replied here:

https://lore.kernel.org/kvm/20240515005952.3410568-17-rick.p.edgecombe@xxxxxxxxx/T/#m49b37658f03e786c6aa43719cbf748215170980d

So I believe we really already have enough mechanisms in the *COMMON* 
code for private page/mapping support.  I intend to believe the whole 
GPA shared bit thing can be hidden in TDX specific operations.  If 
there's really a need to apply/strip GPA shared bit in the common code, 
we can do via kvm_x86_ops callback (I'll review other patches to see).

And btw, I think ...

[...]

> +
> +/*
> + *			default or SEV-SNP	TDX: where S = (47 or 51) - 12
> + * gfn_shared_mask	0			S bit
> + * is_private_gpa()	always false		true if GPA has S bit clear

.. this @is_private_gpa(), and ...

> + * gfn_to_shared()	nop			set S bit
> + * gfn_to_private()	nop			clear S bit
> + *
> + * fault.is_private means that host page should be gotten from guest_memfd
> + * is_private_gpa() means that KVM MMU should invoke private MMU hooks.
> + */

.. this invoking MMU hooks based on @is_private_gpa() makes no sense, 
because clearly for SEV-SNP @is_priavate_gpa() isn't report the fact 
when the GPA is indeed private, and the MMU hooks should be invoked 
based on whether the faulting GPA is private or not, but not this 
@is_private_gpa().