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

Huang, Kai
2024-05-15T22:34:56Z

On 15/05/2024 12:59 pm, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> 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@intel.com/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().