In advance of providing support for unaccepted memory, switch from using
kmalloc() for allocating the Page State Change (PSC) structure to using a
local variable that lives on the stack. This is needed to avoid a possible
recursive call into set_pages_state() if the kmalloc() call requires
(more) memory to be accepted, which would result in a hang.
The current size of the PSC struct is 2,032 bytes. To make the struct more
stack friendly, reduce the number of PSC entries from 253 down to 64,
resulting in a size of 520 bytes. This is a nice compromise on struct size
and total PSC requests while still allowing parallel PSC operations across
vCPUs.
Also, since set_pages_state() uses the per-CPU GHCB, add a static variable
that indicates when per-CPU GHCBs are available. Until they are available,
the GHCB MSR protocol is used to perform page state changes. For APs, the
per-CPU GHCB is created before they are started and registered upon
startup, so this flag can be used globally for the BSP and APs instead of
creating a per-CPU flag.
If either the reduction in PSC entries or the use of the MSR protocol
until the per-CPU GHCBs are available, result in any kind of performance
issue (that is not seen at the moment), use of a larger static PSC struct,
with fallback to the smaller stack version, or use the boot GHCB prior to
per-CPU GHCBs, can be investigated.
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
arch/x86/include/asm/sev-common.h | 9 +++++++--
arch/x86/kernel/sev.c | 32 +++++++++++++++++++++++--------
2 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b8357d6ecd47..6c3d61c5f6a3 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -106,8 +106,13 @@ enum psc_op {
#define GHCB_HV_FT_SNP BIT_ULL(0)
#define GHCB_HV_FT_SNP_AP_CREATION BIT_ULL(1)
-/* SNP Page State Change NAE event */
-#define VMGEXIT_PSC_MAX_ENTRY 253
+/*
+ * SNP Page State Change NAE event
+ * The VMGEXIT_PSC_MAX_ENTRY determines the size of the PSC structure,
+ * which is a local variable (stack usage) in set_pages_state(). Do not
+ * increase this value without evaluating the impact to stack usage.
+ */
+#define VMGEXIT_PSC_MAX_ENTRY 64
struct psc_hdr {
u16 cur_entry;
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index c05f0124c410..40268ce97aad 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -66,6 +66,17 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
*/
static struct ghcb *boot_ghcb __section(".data");
+/*
+ * A flag used by set_pages_state() that indicates when the per-CPU GHCB has
+ * been created and registered and thus can be used instead of using the MSR
+ * protocol. The set_pages_state() function eventually invokes vmgexit_psc(),
+ * which only works with a per-CPU GHCB.
+ *
+ * For APs, the per-CPU GHCB is created before they are started and registered
+ * upon startup, so this flag can be used globally for the BSP and APs.
+ */
+static bool ghcb_percpu_ready __section(".data");
+
/* Bitmap of SEV features supported by the hypervisor */
static u64 sev_hv_features __ro_after_init;
@@ -660,7 +671,7 @@ static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool valid
}
}
-static void __init early_set_pages_state(unsigned long paddr, unsigned int npages, enum psc_op op)
+static void early_set_pages_state(unsigned long paddr, unsigned int npages, enum psc_op op)
{
unsigned long paddr_end;
u64 val;
@@ -751,6 +762,8 @@ static int vmgexit_psc(struct snp_psc_desc *desc)
unsigned long flags;
struct ghcb *ghcb;
+ WARN_ON_ONCE(!ghcb_percpu_ready);
+
/*
* __sev_get_ghcb() needs to run with IRQs disabled because it is using
* a per-CPU GHCB.
@@ -868,11 +881,14 @@ static void __set_pages_state(struct snp_psc_desc *data, unsigned long vaddr,
static void set_pages_state(unsigned long vaddr, unsigned int npages, int op)
{
unsigned long vaddr_end, next_vaddr;
- struct snp_psc_desc *desc;
+ struct snp_psc_desc desc = {};
- desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
- if (!desc)
- panic("SNP: failed to allocate memory for PSC descriptor\n");
+ /*
+ * Use the MSR protocol when the per-CPU GHCBs are not yet registered,
+ * since vmgexit_psc() uses the per-CPU GHCB.
+ */
+ if (!ghcb_percpu_ready)
+ return early_set_pages_state(__pa(vaddr), npages, op);
vaddr = vaddr & PAGE_MASK;
vaddr_end = vaddr + (npages << PAGE_SHIFT);
@@ -882,12 +898,10 @@ static void set_pages_state(unsigned long vaddr, unsigned int npages, int op)
next_vaddr = min_t(unsigned long, vaddr_end,
(VMGEXIT_PSC_MAX_ENTRY * PAGE_SIZE) + vaddr);
- __set_pages_state(desc, vaddr, next_vaddr, op);
+ __set_pages_state(&desc, vaddr, next_vaddr, op);
vaddr = next_vaddr;
}
-
- kfree(desc);
}
void snp_set_memory_shared(unsigned long vaddr, unsigned int npages)
@@ -1254,6 +1268,8 @@ void setup_ghcb(void)
if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
snp_register_per_cpu_ghcb();
+ ghcb_percpu_ready = true;
+
return;
}
--
2.36.1On Mon, Aug 15, 2022 at 10:57:42AM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index c05f0124c410..40268ce97aad 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -66,6 +66,17 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
> */
> static struct ghcb *boot_ghcb __section(".data");
>
> +/*
> + * A flag used by set_pages_state() that indicates when the per-CPU GHCB has
> + * been created and registered and thus can be used instead of using the MSR
> + * protocol. The set_pages_state() function eventually invokes vmgexit_psc(),
> + * which only works with a per-CPU GHCB.
> + *
> + * For APs, the per-CPU GHCB is created before they are started and registered
> + * upon startup, so this flag can be used globally for the BSP and APs.
> + */
Ok, better, thanks!
> +static bool ghcb_percpu_ready __section(".data");
However, it reads really weird if you have "percpu" in the name of a
variable which is not per CPU...
Let's just call it "ghcbs_initialized" and be done with it.
And I still hate the whole thing ofc.
Do this ontop (and I knew we had a flags thing already):
(And yes, __read_mostly is in the .data section too).
---
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 40268ce97aad..5b3afbf26349 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -66,17 +66,6 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
*/
static struct ghcb *boot_ghcb __section(".data");
-/*
- * A flag used by set_pages_state() that indicates when the per-CPU GHCB has
- * been created and registered and thus can be used instead of using the MSR
- * protocol. The set_pages_state() function eventually invokes vmgexit_psc(),
- * which only works with a per-CPU GHCB.
- *
- * For APs, the per-CPU GHCB is created before they are started and registered
- * upon startup, so this flag can be used globally for the BSP and APs.
- */
-static bool ghcb_percpu_ready __section(".data");
-
/* Bitmap of SEV features supported by the hypervisor */
static u64 sev_hv_features __ro_after_init;
@@ -128,7 +117,18 @@ static DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa);
struct sev_config {
__u64 debug : 1,
- __reserved : 63;
+
+ /*
+ * A flag used by set_pages_state() that indicates when the per-CPU GHCB has
+ * been created and registered and thus can be used instead of using the MSR
+ * protocol. The set_pages_state() function eventually invokes vmgexit_psc(),
+ * which only works with a per-CPU GHCB.
+ *
+ * For APs, the per-CPU GHCB is created before they are started and registered
+ * upon startup, so this flag can be used globally for the BSP and APs.
+ */
+ ghcbs_initialized : 1,
+ __reserved : 62;
};
static struct sev_config sev_cfg __read_mostly;
@@ -762,7 +762,7 @@ static int vmgexit_psc(struct snp_psc_desc *desc)
unsigned long flags;
struct ghcb *ghcb;
- WARN_ON_ONCE(!ghcb_percpu_ready);
+ WARN_ON_ONCE(!sev_cfg.ghcbs_initialized);
/*
* __sev_get_ghcb() needs to run with IRQs disabled because it is using
@@ -887,7 +887,7 @@ static void set_pages_state(unsigned long vaddr, unsigned int npages, int op)
* Use the MSR protocol when the per-CPU GHCBs are not yet registered,
* since vmgexit_psc() uses the per-CPU GHCB.
*/
- if (!ghcb_percpu_ready)
+ if (!sev_cfg.ghcbs_initialized)
return early_set_pages_state(__pa(vaddr), npages, op);
vaddr = vaddr & PAGE_MASK;
@@ -1268,7 +1268,7 @@ void setup_ghcb(void)
if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
snp_register_per_cpu_ghcb();
- ghcb_percpu_ready = true;
+ sev_cfg.ghcbs_initialized = true;
return;
}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 8/17/22 11:08, Borislav Petkov wrote:
> On Mon, Aug 15, 2022 at 10:57:42AM -0500, Tom Lendacky wrote:
>> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
>> index c05f0124c410..40268ce97aad 100644
>> --- a/arch/x86/kernel/sev.c
>> +++ b/arch/x86/kernel/sev.c
>> @@ -66,6 +66,17 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
>> */
>> static struct ghcb *boot_ghcb __section(".data");
>>
>> +/*
>> + * A flag used by set_pages_state() that indicates when the per-CPU GHCB has
>> + * been created and registered and thus can be used instead of using the MSR
>> + * protocol. The set_pages_state() function eventually invokes vmgexit_psc(),
>> + * which only works with a per-CPU GHCB.
>> + *
>> + * For APs, the per-CPU GHCB is created before they are started and registered
>> + * upon startup, so this flag can be used globally for the BSP and APs.
>> + */
>
> Ok, better, thanks!
>
>> +static bool ghcb_percpu_ready __section(".data");
>
> However, it reads really weird if you have "percpu" in the name of a
> variable which is not per CPU...
>
> Let's just call it "ghcbs_initialized" and be done with it.
>
> And I still hate the whole thing ofc.
>
> Do this ontop (and I knew we had a flags thing already):
>
> (And yes, __read_mostly is in the .data section too).
Cool, will do.
Thanks,
Tom
>
> ---
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 40268ce97aad..5b3afbf26349 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -66,17 +66,6 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
> */
> static struct ghcb *boot_ghcb __section(".data");
>
> -/*
> - * A flag used by set_pages_state() that indicates when the per-CPU GHCB has
> - * been created and registered and thus can be used instead of using the MSR
> - * protocol. The set_pages_state() function eventually invokes vmgexit_psc(),
> - * which only works with a per-CPU GHCB.
> - *
> - * For APs, the per-CPU GHCB is created before they are started and registered
> - * upon startup, so this flag can be used globally for the BSP and APs.
> - */
> -static bool ghcb_percpu_ready __section(".data");
> -
> /* Bitmap of SEV features supported by the hypervisor */
> static u64 sev_hv_features __ro_after_init;
>
> @@ -128,7 +117,18 @@ static DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa);
>
> struct sev_config {
> __u64 debug : 1,
> - __reserved : 63;
> +
> + /*
> + * A flag used by set_pages_state() that indicates when the per-CPU GHCB has
> + * been created and registered and thus can be used instead of using the MSR
> + * protocol. The set_pages_state() function eventually invokes vmgexit_psc(),
> + * which only works with a per-CPU GHCB.
> + *
> + * For APs, the per-CPU GHCB is created before they are started and registered
> + * upon startup, so this flag can be used globally for the BSP and APs.
> + */
> + ghcbs_initialized : 1,
> + __reserved : 62;
> };
>
> static struct sev_config sev_cfg __read_mostly;
> @@ -762,7 +762,7 @@ static int vmgexit_psc(struct snp_psc_desc *desc)
> unsigned long flags;
> struct ghcb *ghcb;
>
> - WARN_ON_ONCE(!ghcb_percpu_ready);
> + WARN_ON_ONCE(!sev_cfg.ghcbs_initialized);
>
> /*
> * __sev_get_ghcb() needs to run with IRQs disabled because it is using
> @@ -887,7 +887,7 @@ static void set_pages_state(unsigned long vaddr, unsigned int npages, int op)
> * Use the MSR protocol when the per-CPU GHCBs are not yet registered,
> * since vmgexit_psc() uses the per-CPU GHCB.
> */
> - if (!ghcb_percpu_ready)
> + if (!sev_cfg.ghcbs_initialized)
> return early_set_pages_state(__pa(vaddr), npages, op);
>
> vaddr = vaddr & PAGE_MASK;
> @@ -1268,7 +1268,7 @@ void setup_ghcb(void)
> if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> snp_register_per_cpu_ghcb();
>
> - ghcb_percpu_ready = true;
> + sev_cfg.ghcbs_initialized = true;
>
> return;
> }
>
© 2016 - 2026 Red Hat, Inc.