Mailing List Archive

1 2 3  View All
Re: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted [ In reply to ]
On 3/9/23 05:59, Borislav Petkov wrote:
> First of all,
>
> thanks for proactively pointing that out instead of simply using what's
> there and we get to find out later, only by chance.
>
> Much appreciated. :-)
>
> On Thu, Mar 09, 2023 at 11:12:10AM +0000, David Woodhouse wrote:
>>> Right, I think we're ok with the following basic rules:
>>>
>>> - pure arch/x86/ code should use the x86_platform function pointers to
>>>   query hypervisor capabilities/peculiarities
>>>
>>> - cc_platform_has() should be used in generic/driver code as it
>>>   abstracts away the underlying platform better. IOW, querying
>>>   x86_platform.... in generic, platform-agnostic driver code looks weird to
>>>   say the least
>>>
>>> The hope is that those two should be enough to support most guest types
>>> and not let the zoo get too much out of hand...
>>>
>>> Thx.
>>
>> In
>> https://lore.kernel.org/all/20230308171328.1562857-13-usama.arif@bytedance.com/
>> I added an sev_es_active() helper for x86 code.
>>
>> Is that consistent with the vision here, or should I do something different?
>
> So looking at sev_es_init_vc_handling() where we set that key, I'm
> *thinking* that key can be removed now and the code should check
>
> cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)
>
> instead.
>
> Because if some of the checks in that function below fail, the guest
> will terminate anyway.
>
> Jörg, Tom?

I believe Joerg added that key for performance reasons, since it is used
on the exception path and can avoid all the calls to cc_platform_has(). I
think that key should stay.

Maybe David can introduce an CC_ATTR_GUEST_SEV_ES attribute that returns
true if the guest is an ES or SNP guest. Or do we introduce a
CC_ATTR_PARALLEL_BOOT attribute that returns true for any SEV guest.

Then the "if cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) && !has_sev_es"
check in arch/x86/kernel/smpboot.c can be removed and the following check
can become if (x2apic_mode || cc_platform_has(CC_ATTR_PARALLEL_BOOT))

Not sure how that affects a TDX guest, though.

Thanks,
Tom

>
Re: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted [ In reply to ]
On Thu, Mar 09, 2023 at 08:19:58AM -0600, Tom Lendacky wrote:
> I believe Joerg added that key for performance reasons, since it is used on
> the exception path and can avoid all the calls to cc_platform_has(). I think
> that key should stay.

Yes, that is right. The key is mainly for the NMI entry path which can
be performance relevant in some situations. For SEV-ES some special
handling is needed there to re-enable NMIs and adjust the #VC stack in
case it was raised on the VC-handlers entry path.

Regards,

Joerg
Re: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted [ In reply to ]
On Thu, Mar 09, 2023 at 03:36:45PM +0100, Jörg Rödel wrote:
> Yes, that is right. The key is mainly for the NMI entry path which can
> be performance relevant in some situations. For SEV-ES some special
> handling is needed there to re-enable NMIs and adjust the #VC stack in
> case it was raised on the VC-handlers entry path.

So the performance argument is meh. That key will be replaced by

if (cc_vendor == CC_VENDOR_AMD &&
cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)

which is something like 4 insns or so. Tops.

Haven't looked yet but it should be cheap.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted [ In reply to ]
On Thu, 2023-03-09 at 15:45 +0100, Borislav Petkov wrote:
> On Thu, Mar 09, 2023 at 03:36:45PM +0100, Jörg Rödel wrote:
> > Yes, that is right. The key is mainly for the NMI entry path which can
> > be performance relevant in some situations. For SEV-ES some special
> > handling is needed there to re-enable NMIs and adjust the #VC stack in
> > case it was raised on the VC-handlers entry path.
>
> So the performance argument is meh. That key will be replaced by
>
>         if (cc_vendor == CC_VENDOR_AMD &&
>             cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)
>
> which is something like 4 insns or so. Tops.
>
> Haven't looked yet but it should be cheap.


cc_vendor isn't yet exposed. As we discussed this in IRC, I've been
updating the parallel bringup support for SEV-ES, including adding a
cc_get_vendor() function, in the top of my tree at
https://git.infradead.org/users/dwmw2/linux.git/commitdiff/parallel-6.2-v15
and it now looks like this:

/*
* Encrypted guests other than SEV-ES (in the future) will need to
* implement an early way of finding the APIC ID, since they will
* presumably block direct CPUID too. Be kind to our future selves
* by warning here instead of just letting them break. Parallel
* startup doesn't have to be in the first round of enabling patches
* for any such technology.
*/
if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
switch (cc_get_vendor()) {
case CC_VENDOR_AMD:
has_sev_es = true;
break;

default:
pr_info("Disabling parallel bringup due to guest state encryption\n");
return false;
}
}

Using an explicit CC_ATTR_NO_EARLY_CPUID flag instead of
CC_ATTR_GUEST_STATE_ENCRYPT which is merely an approximation of that,
might be interesting.
Re: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted [ In reply to ]
On Thu, Mar 09, 2023 at 03:45:35PM +0000, David Woodhouse wrote:
> cc_vendor isn't yet exposed. As we discussed this in IRC, I've been

Right, and we might just as well expose it because having
a setter/getter is kinda silly for a __ro_after_init variable, see
below.

So here's what I was able to scratch up quickly to remove the static
key.

The asm looks like this:

# ./arch/x86/include/asm/sev.h:156: if (cc_vendor == CC_VENDOR_AMD &&
cmpl $1, cc_vendor(%rip) #, cc_vendor
je .L204 #,

...

.L204:
# ./arch/x86/include/asm/sev.h:157: cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
movl $3, %edi #,
call cc_platform_has #
# ./arch/x86/include/asm/sev.h:156: if (cc_vendor == CC_VENDOR_AMD &&
testb %al, %al # tmp134
je .L158 #,

and so I doubt that this is at all measureable comparing that to the
rest of the code that gets executed in the NMI handler. And it lets us
get rid of yet another static key and use only the CC APIs.

Oh, and it bitches because cc_platform_has() is being called in noinstr
region but we can mark it noinstr or add the drop-noinstr markers around
it, if needed. Not that important as that function and what it calls
don't do anything magical.

Thx.

---
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index 49b44f881484..34446383e68b 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -13,7 +13,7 @@
#include <asm/coco.h>
#include <asm/processor.h>

-static enum cc_vendor vendor __ro_after_init;
+enum cc_vendor cc_vendor __ro_after_init;
static u64 cc_mask __ro_after_init;

static bool intel_cc_platform_has(enum cc_attr attr)
@@ -83,7 +83,7 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)

bool cc_platform_has(enum cc_attr attr)
{
- switch (vendor) {
+ switch (cc_vendor) {
case CC_VENDOR_AMD:
return amd_cc_platform_has(attr);
case CC_VENDOR_INTEL:
@@ -105,7 +105,7 @@ u64 cc_mkenc(u64 val)
* - for AMD, bit *set* means the page is encrypted
* - for Intel *clear* means encrypted.
*/
- switch (vendor) {
+ switch (cc_vendor) {
case CC_VENDOR_AMD:
return val | cc_mask;
case CC_VENDOR_INTEL:
@@ -118,7 +118,7 @@ u64 cc_mkenc(u64 val)
u64 cc_mkdec(u64 val)
{
/* See comment in cc_mkenc() */
- switch (vendor) {
+ switch (cc_vendor) {
case CC_VENDOR_AMD:
return val & ~cc_mask;
case CC_VENDOR_INTEL:
@@ -131,7 +131,7 @@ EXPORT_SYMBOL_GPL(cc_mkdec);

__init void cc_set_vendor(enum cc_vendor v)
{
- vendor = v;
+ cc_vendor = v;
}

__init void cc_set_mask(u64 mask)
diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index 3d98c3a60d34..0563e07a1002 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -11,6 +11,8 @@ enum cc_vendor {
CC_VENDOR_INTEL,
};

+extern enum cc_vendor cc_vendor;
+
void cc_set_vendor(enum cc_vendor v);
void cc_set_mask(u64 mask);

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index ebc271bb6d8e..1335781e4976 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -12,6 +12,7 @@
#include <asm/insn.h>
#include <asm/sev-common.h>
#include <asm/bootparam.h>
+#include <asm/coco.h>

#define GHCB_PROTOCOL_MIN 1ULL
#define GHCB_PROTOCOL_MAX 2ULL
@@ -134,24 +135,26 @@ struct snp_secrets_page_layout {
} __packed;

#ifdef CONFIG_AMD_MEM_ENCRYPT
-extern struct static_key_false sev_es_enable_key;
extern void __sev_es_ist_enter(struct pt_regs *regs);
extern void __sev_es_ist_exit(void);
static __always_inline void sev_es_ist_enter(struct pt_regs *regs)
{
- if (static_branch_unlikely(&sev_es_enable_key))
+ if (cc_vendor == CC_VENDOR_AMD &&
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
__sev_es_ist_enter(regs);
}
static __always_inline void sev_es_ist_exit(void)
{
- if (static_branch_unlikely(&sev_es_enable_key))
+ if (cc_vendor == CC_VENDOR_AMD &&
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
__sev_es_ist_exit();
}
extern int sev_es_setup_ap_jump_table(struct real_mode_header *rmh);
extern void __sev_es_nmi_complete(void);
static __always_inline void sev_es_nmi_complete(void)
{
- if (static_branch_unlikely(&sev_es_enable_key))
+ if (cc_vendor == CC_VENDOR_AMD &&
+ cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
__sev_es_nmi_complete();
}
extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 679026a640ef..7d873bffbd8e 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -111,7 +111,6 @@ struct ghcb_state {
};

static DEFINE_PER_CPU(struct sev_es_runtime_data*, runtime_data);
-DEFINE_STATIC_KEY_FALSE(sev_es_enable_key);

static DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa);

@@ -1393,9 +1392,6 @@ void __init sev_es_init_vc_handling(void)
sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
}

- /* Enable SEV-ES special handling */
- static_branch_enable(&sev_es_enable_key);
-
/* Initialize per-cpu GHCB pages */
for_each_possible_cpu(cpu) {
alloc_runtime_data(cpu);

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v5 06/14] x86/ioremap: Support hypervisor specified range to map as encrypted [ In reply to ]
On Thu, 2023-03-09 at 17:34 +0100, Borislav Petkov wrote:
> On Thu, Mar 09, 2023 at 03:45:35PM +0000, David Woodhouse wrote:
> > cc_vendor isn't yet exposed. As we discussed this in IRC, I've been
>
> Right, and we might just as well expose it because having
> a setter/getter is kinda silly for a __ro_after_init variable, see
> below.
>
> So here's what I was able to scratch up quickly to remove the static
> key.
>
> The asm looks like this:

Seems reasonable. I might just let you iterate on that, drop the SNP-ES
part from the parallel boot series, then let Tom or someone send it in
later once the dust settles.

1 2 3  View All