Re: [RFC PATCH 1/6] x86/virt/tdx: Add SEAMCALL wrappers for TDX KeyID management

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Fri, 2024-11-22 at 16:08 -0800, Dave Hansen wrote:
> On 11/22/24 15:55, Sean Christopherson wrote:
> > On Fri, Nov 22, 2024, Dave Hansen wrote:
> > I don't know the full context, but working with "struct page" is a pain when every
> > user just wants the physical address.  KVM SVM had a few cases where pointers were
> > tracked as "struct page", and it was generally unpleasant to read and work with.
> 
> I'm not super convinced. page_to_phys(foo) is all it takes
> 
> > I also don't like conflating the kernel's "struct page" with the architecture's
> > definition of a 4KiB page.
> 
> That's fair, although it's pervasively conflated across our entire
> codebase. But 'struct page' is substantially better than a hpa_t,
> phys_addr_t or u64 that can store a full 64-bits of address. Those
> conflate a physical address with a physical page, which is *FAR* worse.

In the case of tdh_mem_page_aug(), etc the caller only has a kvm_pfn_t passed
from a TDP MMU callback, for the page to be mapped in the guest TD. It is
probably not nice to assume that this kvm_pfn_t will have a struct page. So we
shouldn't always use struct pages for the SEAMCALL wrappers in any case.

What if we just move these members from hpa_t to pfn_t? It keeps us off struct
page, but addresses some of Dave's concerns about hpa_t looking like a specific
address.

> 
> > > You know that 'tdr' is not just some random physical address.  It's a
> > > whole physical page.  It's page-aligned.  It was allocated, from the
> > > allocator.  It doesn't point to special memory.
> > 
> > Oh, but it does point to special memory.  If it *didn't* point at special memory
> > that is completely opaque and untouchable, then KVM could use a struct overlay,
> > which would give contextual information and some amount of type safety.  E.g.
> > an equivalent without TDX is "struct vmcs *".
> > 
> > Rather than "struct page", what if we add an address_space (in the Sparse sense),
> > and a typedef for a TDX pages?  Maybe __firmware?  E.g.
> > 
> >    # define __firmware	__attribute__((noderef, address_space(__firmware)))
> > 
> >    typedef u64 __firmware *tdx_page_t;
> > 
> > That doesn't give as much compile-time safety, but in some ways it provides more
> > type safety since KVM (or whatever else cares) would need to make an explicit and
> > ugly cast to misuse the pointer.
> 
> It's better than nothing. But I still vastly prefer to have a type that
> tells you that something is physically-allocated out of the buddy, RAM,
> and page-aligned.
> 
> I'd be better to have:
> 
> struct tdx_page {
> 	u64 page_phys_addr;
> };
> 
> than depend on sparse, IMNHO.
> 
> Do you run sparse every time you compile the kernel, btw? ;)

Hmm, I'm trying to think of specific scenarios that "tdx page" types could make
big safety difference on.

Sean, do you happen to recall any specific bugs on the SEV side that this would
have helped with?

I hear the intuition, but without specific problems, it doesn't seem worth extra
code to me. Not a strong objection though.