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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 16 May 2024 10:34:43 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>, <pbonzini@...hat.com>,
	<seanjc@...gle.com>, <kvm@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, <isaku.yamahata@...il.com>,
	<erdemaktas@...gle.com>, <sagis@...gle.com>, <yan.y.zhao@...el.com>,
	<dmatlack@...gle.com>
Subject: Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for
 TDX shared bit of GPA



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