Message-ID: <20241030101005.GAZyIF_bKVpe9gpHrn@fat_crate.local>
Date: Wed, 30 Oct 2024 11:10:05 +0100
From: Borislav Petkov <bp@...en8.de>
To: "Nikunj A. Dadhania" <nikunj@....com>
Cc: linux-kernel@...r.kernel.org, thomas.lendacky@....com, x86@...nel.org,
	kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
	dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
	pbonzini@...hat.com
Subject: Re: [PATCH v14 01/13] x86/sev: Carve out and export SNP guest
 messaging init routines

On Wed, Oct 30, 2024 at 10:14:45AM +0530, Nikunj A. Dadhania wrote:
> >> Carve out common SNP guest messaging buffer allocations and message
> >> initialization routines to core/sev.c and export them. These newly added
> >> APIs set up the SNP message context (snp_msg_desc), which contains all the
> >> necessary details for sending SNP guest messages.
> 
> This explains how it is being done, which I think is useful. However, if you
> think otherwise, I can remove.

Useful why? It should be visible from the diff itself. Why do you have to
explain your diff?

If you feel the need to have to explain it, then *maybe* it needs splitting or
simplifying or whatnot.

> >> At present, the SEV guest platform data structure is used to pass the
> >> secrets page physical address to SEV guest driver. Since the secrets page
> >> address is locally available to the initialization routine, use the cached
> >> address. Remove the unused SEV guest platform data structure.
> 
> In the above paragraph I tried to explains why I have removed
> sev_guest_platform_data.

Probably ok.

> People have different styles of writing, as long as we are capturing the
> required information, IMHO, it should be fine.

"... as long we are not writing what the patch does - which is visible from
the diff itself, it should be fine." is what I would say.

> Let me try to repharse the commit message again:
> 
> x86/sev: Carve out and export SNP guest messaging init routines
> 
> Currently, the sev-guest driver is the only user of SNP guest messaging.
> All routines for initializing SNP guest messaging are implemented within
> the sev-guest driver and are not available during early boot. In
> prepratation for adding Secure TSC guest support, carve out APIs to
> allocate and initialize guest messaging descriptor context and make it part
> of coco/sev/core.c. As there is no user of sev_guest_platform_data anymore,
> remove the structure.

Yap, better.

Btw, you can even use AI for help rephrasing it:

-> Please simpliify this code change description by concentrating only on why
the change is being done:

<insert commit message here>

ChatGPT: "This change is being made to allow the Secure TSC guest feature to
access SNP guest messaging routines earlier in the boot process. By moving
common SNP message initialization routines to a central location (core/sev.c)
and exporting them, these routines become accessible for early boot needs.
Additionally, since the secrets page address is now accessible within the
initialization, the redundant SEV guest platform data structure has been
removed."

-> Do the same as the above but write it in imperative tone

ChatGPT: "Enable Secure TSC guest support by moving the SNP guest messaging
routines to core/sev.c, making them available during early boot. Export these
routines to set up the SNP message context (snp_msg_desc) needed for guest
messaging. Use the cached secrets page address in the initialization routine,
and remove the unused SEV guest platform data structure."

I think that's a good help to use at least as a starter for the final commit
message.

To sum up: I'd like the commit message to contain enough information to know
*why* a change has been done. No rambling about what the patch does.

> > ld: vmlinux.o: in function `snp_init_crypto':
> > /home/boris/kernel/2nd/linux/arch/x86/coco/sev/core.c:2700:(.text+0x1fa3): undefined reference to `aesgcm_expandkey'
> > make[2]: *** [scripts/Makefile.vmlinux:34: vmlinux] Error 1
> > make[1]: *** [/mnt/kernel/kernel/2nd/linux/Makefile:1166: vmlinux] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > make: *** [Makefile:224: __sub-make] Error 2
> > 
> > I'll stop here until you fix those.
> 
> Sorry for this, I had sev-guest driver as in-built module in my config, so wasn't
> able to catch this in my per patch build script. The corresponding fix is in the 
> following patch[1], during patch juggling it had landed there:
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2852fcd82cbd..6426b6d469a4 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1556,6 +1556,7 @@ config AMD_MEM_ENCRYPT
>  	select ARCH_HAS_CC_PLATFORM
>  	select X86_MEM_ENCRYPT
>  	select UNACCEPTED_MEMORY
> +	select CRYPTO_LIB_AESGCM

Right, that is debatable. AMD memory encryption support cannot really depend
on a crypto library - you can get it without it too - just won't get secure
TSC but for simplicity's sake let's leave it that way for now.

Because looking at this:

config AMD_MEM_ENCRYPT          
        bool "AMD Secure Memory Encryption (SME) support"
        depends on X86_64 && CPU_SUP_AMD
        depends on EFI_STUB 
        select DMA_COHERENT_POOL
        select ARCH_USE_MEMREMAP_PROT
        select INSTRUCTION_DECODER
        select ARCH_HAS_CC_PLATFORM
        select X86_MEM_ENCRYPT
        select UNACCEPTED_MEMORY
        select CRYPTO_LIB_AESGCM

that symbol is pulling in a lot of other stuff. I guess it is ok for now...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists