Hi Boris,
On 10/29/2024 11:13 PM, Borislav Petkov wrote:
> On Mon, Oct 28, 2024 at 11:04:19AM +0530, Nikunj A Dadhania wrote:
>> 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. To add Secure TSC guest support, these initialization
>> routines need to be available during early boot.
The above paragraph explains why the change is being done.
>>
>> 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.
>> 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.
>
> Do not talk about *what* the patch is doing in the commit message - that
> should be obvious from the diff itself. Rather, concentrate on the *why*
> it needs to be done.
>
> Imagine one fine day you're doing git archeology, you find the place in
> the code about which you want to find out why it was changed the way it
> is now.
>
> You do git annotate <filename> ... find the line, see the commit id and
> you do:
>
> git show <commit id>
>
> You read the commit message and there's just gibberish and nothing's
> explaining *why* that change was done. And you start scratching your
> head, trying to figure out why.
>
> See what I mean?
People have different styles of writing, as long as we are capturing the
required information, IMHO, it should be fine.
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.
Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>
>> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> ---
>> arch/x86/include/asm/sev.h | 71 ++++++++-
>> arch/x86/coco/sev/core.c | 133 +++++++++++++++-
>> drivers/virt/coco/sev-guest/sev-guest.c | 195 +++---------------------
>> 3 files changed, 215 insertions(+), 184 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index 2e49c4a9e7fe..63c30f4d44d7 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -14,6 +14,7 @@
>> #include <asm/insn.h>
>> #include <asm/sev-common.h>
>> #include <asm/coco.h>
>> +#include <asm/set_memory.h>
>>
>> #define GHCB_PROTOCOL_MIN 1ULL
>> #define GHCB_PROTOCOL_MAX 2ULL
>> @@ -170,10 +171,6 @@ struct snp_guest_msg {
>> u8 payload[PAGE_SIZE - sizeof(struct snp_guest_msg_hdr)];
>> } __packed;
>>
>> -struct sev_guest_platform_data {
>> - u64 secrets_gpa;
>> -};
>> -
>> struct snp_guest_req {
>> void *req_buf;
>> size_t req_sz;
>> @@ -253,6 +250,7 @@ struct snp_msg_desc {
>>
>> u32 *os_area_msg_seqno;
>> u8 *vmpck;
>> + int vmpck_id;
>> };
>>
>> /*
>> @@ -438,6 +436,63 @@ u64 sev_get_status(void);
>> void sev_show_status(void);
>> void snp_update_svsm_ca(void);
>>
>> +static inline void free_shared_pages(void *buf, size_t sz)
>
> A function with a generic name exported in a header?!
>
> First of all, why is it in a header?
>
> Then, why isn't it called something "sev_" or so...?
>
> Same holds true for all the below.
Sure, will update it accordingly in my next version.
>
>> + unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
>> + int ret;
>> +
>> + if (!buf)
>> + return;
>> +
>> + ret = set_memory_encrypted((unsigned long)buf, npages);
>> + if (ret) {
>> + WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");
>> + return;
>> + }
>> +
>> + __free_pages(virt_to_page(buf), get_order(sz));
>> +}
>
> ...
>
>> +static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t keylen)
>> +{
>> + struct aesgcm_ctx *ctx;
>> +
>> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL_ACCOUNT);
>> + if (!ctx)
>> + return NULL;
>> +
>> + if (aesgcm_expandkey(ctx, key, keylen, AUTHTAG_LEN)) {
>
> 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
help
Say yes to enable support for the encryption of system memory.
This requires an AMD processor that supports Secure Memory
diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
index 0b772bd921d8..a6405ab6c2c3 100644
--- a/drivers/virt/coco/sev-guest/Kconfig
+++ b/drivers/virt/coco/sev-guest/Kconfig
@@ -2,7 +2,6 @@ config SEV_GUEST
tristate "AMD SEV Guest driver"
default m
depends on AMD_MEM_ENCRYPT
- select CRYPTO_LIB_AESGCM
select TSM_REPORTS
help
SEV-SNP firmware provides the guest a mechanism to communicate with
> Btw, tip patches are done against tip/master - not against the branch they get
> queued in.
Sure, I will keep that in mind.
Thanks for the review,
Nikunj
1. https://lore.kernel.org/all/20241028053431.3439593-3-nikunj@xxxxxxx/#Z31arch:x86:Kconfig
