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.
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.
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 <[email protected]>
---
arch/x86/include/asm/sev-common.h | 2 +-
arch/x86/kernel/sev.c | 24 ++++++++++++++++--------
2 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b8357d6ecd47..6f7268a817fc 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -107,7 +107,7 @@ enum psc_op {
#define GHCB_HV_FT_SNP_AP_CREATION BIT_ULL(1)
/* SNP Page State Change NAE event */
-#define VMGEXIT_PSC_MAX_ENTRY 253
+#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..275aa890611f 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -66,6 +66,9 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
*/
static struct ghcb *boot_ghcb __section(".data");
+/* Flag to indicate when the first per-CPU GHCB is registered */
+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 +663,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;
@@ -868,11 +871,16 @@ 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;
+
+ /*
+ * 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);
- desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
- if (!desc)
- panic("SNP: failed to allocate memory for PSC descriptor\n");
+ memset(&desc, 0, sizeof(desc));
vaddr = vaddr & PAGE_MASK;
vaddr_end = vaddr + (npages << PAGE_SHIFT);
@@ -882,12 +890,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 +1260,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.1
On 8/8/22 10:16, Tom Lendacky wrote:
...
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index b8357d6ecd47..6f7268a817fc 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -107,7 +107,7 @@ enum psc_op {
> #define GHCB_HV_FT_SNP_AP_CREATION BIT_ULL(1)
>
> /* SNP Page State Change NAE event */
> -#define VMGEXIT_PSC_MAX_ENTRY 253
> +#define VMGEXIT_PSC_MAX_ENTRY 64
In general, the stack-based allocation looks fine. It might be worth a
comment in there to make it clear that this can consume stack space.
> struct psc_hdr {
> u16 cur_entry;
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index c05f0124c410..275aa890611f 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -66,6 +66,9 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
> */
> static struct ghcb *boot_ghcb __section(".data");
>
> +/* Flag to indicate when the first per-CPU GHCB is registered */
> +static bool ghcb_percpu_ready __section(".data");
So, there's a code path that can't be entered until this is set? Seems
like the least we can do it annotate that path with a
WARN_ON_ONCE(!ghcb_percpu_ready).
Also, how does having _one_ global variable work for indicating the
state of multiple per-cpu structures? The code doesn't seem to delay
setting this variable until after _all_ of the per-cpu state is ready.
> /* Bitmap of SEV features supported by the hypervisor */
> static u64 sev_hv_features __ro_after_init;
>
> @@ -660,7 +663,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;
> @@ -868,11 +871,16 @@ 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;
> +
> + /*
> + * 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);
>
> - desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
> - if (!desc)
> - panic("SNP: failed to allocate memory for PSC descriptor\n");
> + memset(&desc, 0, sizeof(desc));
Why is this using memset()? The compiler should be smart enough to
delay initializing 'desc' until after the return with this kind of
construct:
struct snp_psc_desc desc = {};
if (foo)
return;
// use 'desc' here
The compiler *knows* there is no access to 'desc' before the if().
> vaddr = vaddr & PAGE_MASK;
> vaddr_end = vaddr + (npages << PAGE_SHIFT);
> @@ -882,12 +890,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 +1260,8 @@ void setup_ghcb(void)
> if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> snp_register_per_cpu_ghcb();
>
> + ghcb_percpu_ready = true;
> +
> return;
> }
>
On 8/8/22 15:18, Tom Lendacky wrote:
>>> +/* Flag to indicate when the first per-CPU GHCB is registered */
>>> +static bool ghcb_percpu_ready __section(".data");
>>
>> So, there's a code path that can't be entered until this is set? Seems
>> like the least we can do it annotate that path with a
>> WARN_ON_ONCE(!ghcb_percpu_ready).
>
> Sure, that can be added. Right now the only function that calls
> vmgexit_psc() is covered (set_pages_state()/__set_pages_state()) and is
> doing the right thing. But I guess if anything is added in the future,
> that will provide details on what happened.
>
>>
>> Also, how does having _one_ global variable work for indicating the
>> state of multiple per-cpu structures? The code doesn't seem to delay
>> setting this variable until after _all_ of the per-cpu state is ready.
>
> All of the per-CPU GHCBs are allocated during the BSP boot, before any
> AP is started. The APs only ever run the kernel_exc_vmm_communication()
> #VC handler and only ever use the per-CPU version of the GHCB, never the
> early boot version. This is based on the initial_vc_handler being
> switched to the runtime #VC handler, kernel_exc_vmm_communication.
>
> The trigger for the switch over for the BSP from the early boot GHCB to
> the per-CPU GHCB is during setup_ghcb() after the initial_vc_handler has
> been switched to kernel_exc_vmm_communication, which is just after the
> per-CPU allocations. By putting the setting of the ghcb_percpu_ready in
> setup_ghcb(), it indicates that the BSP per-CPU GHCB has been registered
> and can be used.
That description makes the proposed comment even more confusing:
/* Flag to indicate when the first per-CPU GHCB is registered */
The important thing is that this variable is only _useful_ for the boot
CPU. After the boot CPU has allocated space for _itself_, it can then
go and stop using the MSR-based method.
The reason it's set after "the first" is because "the first" is also the
boot CPU, but referring to it as the "the first" is a bit oblique.
Maybe something like this:
/*
* Set after the boot CPU's GHCB is registered. At that point,
* it can be used for calls instead of MSRs.
*/
On 8/8/22 17:33, Dave Hansen wrote:
> On 8/8/22 15:18, Tom Lendacky wrote:
>>>> +/* Flag to indicate when the first per-CPU GHCB is registered */
>>>> +static bool ghcb_percpu_ready __section(".data");
>>>
>>> So, there's a code path that can't be entered until this is set? Seems
>>> like the least we can do it annotate that path with a
>>> WARN_ON_ONCE(!ghcb_percpu_ready).
>>
>> Sure, that can be added. Right now the only function that calls
>> vmgexit_psc() is covered (set_pages_state()/__set_pages_state()) and is
>> doing the right thing. But I guess if anything is added in the future,
>> that will provide details on what happened.
>>
>>>
>>> Also, how does having _one_ global variable work for indicating the
>>> state of multiple per-cpu structures? The code doesn't seem to delay
>>> setting this variable until after _all_ of the per-cpu state is ready.
>>
>> All of the per-CPU GHCBs are allocated during the BSP boot, before any
>> AP is started. The APs only ever run the kernel_exc_vmm_communication()
>> #VC handler and only ever use the per-CPU version of the GHCB, never the
>> early boot version. This is based on the initial_vc_handler being
>> switched to the runtime #VC handler, kernel_exc_vmm_communication.
>>
>> The trigger for the switch over for the BSP from the early boot GHCB to
>> the per-CPU GHCB is during setup_ghcb() after the initial_vc_handler has
>> been switched to kernel_exc_vmm_communication, which is just after the
>> per-CPU allocations. By putting the setting of the ghcb_percpu_ready in
>> setup_ghcb(), it indicates that the BSP per-CPU GHCB has been registered
>> and can be used.
>
> That description makes the proposed comment even more confusing:
>
> /* Flag to indicate when the first per-CPU GHCB is registered */
>
> The important thing is that this variable is only _useful_ for the boot
> CPU. After the boot CPU has allocated space for _itself_, it can then
> go and stop using the MSR-based method.
>
> The reason it's set after "the first" is because "the first" is also the
> boot CPU, but referring to it as the "the first" is a bit oblique.
> Maybe something like this:
>
> /*
> * Set after the boot CPU's GHCB is registered. At that point,
> * it can be used for calls instead of MSRs.
> */
Sure, I'll work on the wording.
Thanks,
Tom
On 8/8/22 16:43, Dave Hansen wrote:
> On 8/8/22 10:16, Tom Lendacky wrote:
> ...
>> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
>> index b8357d6ecd47..6f7268a817fc 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -107,7 +107,7 @@ enum psc_op {
>> #define GHCB_HV_FT_SNP_AP_CREATION BIT_ULL(1)
>>
>> /* SNP Page State Change NAE event */
>> -#define VMGEXIT_PSC_MAX_ENTRY 253
>> +#define VMGEXIT_PSC_MAX_ENTRY 64
>
> In general, the stack-based allocation looks fine. It might be worth a
> comment in there to make it clear that this can consume stack space.
I'll add that.
>
>> struct psc_hdr {
>> u16 cur_entry;
>> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
>> index c05f0124c410..275aa890611f 100644
>> --- a/arch/x86/kernel/sev.c
>> +++ b/arch/x86/kernel/sev.c
>> @@ -66,6 +66,9 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
>> */
>> static struct ghcb *boot_ghcb __section(".data");
>>
>> +/* Flag to indicate when the first per-CPU GHCB is registered */
>> +static bool ghcb_percpu_ready __section(".data");
>
> So, there's a code path that can't be entered until this is set? Seems
> like the least we can do it annotate that path with a
> WARN_ON_ONCE(!ghcb_percpu_ready).
Sure, that can be added. Right now the only function that calls
vmgexit_psc() is covered (set_pages_state()/__set_pages_state()) and is
doing the right thing. But I guess if anything is added in the future,
that will provide details on what happened.
>
> Also, how does having _one_ global variable work for indicating the
> state of multiple per-cpu structures? The code doesn't seem to delay
> setting this variable until after _all_ of the per-cpu state is ready.
All of the per-CPU GHCBs are allocated during the BSP boot, before any AP
is started. The APs only ever run the kernel_exc_vmm_communication() #VC
handler and only ever use the per-CPU version of the GHCB, never the early
boot version. This is based on the initial_vc_handler being switched to
the runtime #VC handler, kernel_exc_vmm_communication.
The trigger for the switch over for the BSP from the early boot GHCB to
the per-CPU GHCB is during setup_ghcb() after the initial_vc_handler has
been switched to kernel_exc_vmm_communication, which is just after the
per-CPU allocations. By putting the setting of the ghcb_percpu_ready in
setup_ghcb(), it indicates that the BSP per-CPU GHCB has been registered
and can be used.
>
>> /* Bitmap of SEV features supported by the hypervisor */
>> static u64 sev_hv_features __ro_after_init;
>>
>> @@ -660,7 +663,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;
>> @@ -868,11 +871,16 @@ 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;
>> +
>> + /*
>> + * 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);
>>
>> - desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
>> - if (!desc)
>> - panic("SNP: failed to allocate memory for PSC descriptor\n");
>> + memset(&desc, 0, sizeof(desc));
>
> Why is this using memset()? The compiler should be smart enough to
> delay initializing 'desc' until after the return with this kind of
> construct:
>
> struct snp_psc_desc desc = {};
> if (foo)
> return;
> // use 'desc' here
>
> The compiler *knows* there is no access to 'desc' before the if().
Yup, I can change that.
Thanks,
Tom
>
>
>> vaddr = vaddr & PAGE_MASK;
>> vaddr_end = vaddr + (npages << PAGE_SHIFT);
>> @@ -882,12 +890,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 +1260,8 @@ void setup_ghcb(void)
>> if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>> snp_register_per_cpu_ghcb();
>>
>> + ghcb_percpu_ready = true;
>> +
>> return;
>> }
>>
>
On Mon, Aug 08, 2022 at 12:16:24PM -0500, Tom Lendacky wrote:
> 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.
I don't understand: kmalloc() allocates memory which is unaccepted?
> 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.
Why can't you simply allocate that one PSC page once at boot, accept the
memory for it and use it throughout? Under locking, ofc, if multiple PSC
calls need to happen in parallel...
Instead of limiting the PSC req size.
> @@ -1254,6 +1260,8 @@ void setup_ghcb(void)
> if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> snp_register_per_cpu_ghcb();
>
> + ghcb_percpu_ready = true;
You know how I can't stand those random boolean vars stating something
has been initialized?
Can't you at least use some field in struct ghcb.reserved_1[] or so
which the spec can provide to OS use so that FW doesn't touch it?
And then stick a "percpu_ready" bit there.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 8/12/22 08:03, Borislav Petkov wrote:
> On Mon, Aug 08, 2022 at 12:16:24PM -0500, Tom Lendacky wrote:
>> 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.
>
> I don't understand: kmalloc() allocates memory which is unaccepted?
In order to satisfy the kmalloc() some memory has to be accepted. So it
tries to accept some additional memory, but we're already in the accept
memory path... deadlock.
>
>> 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.
>
> Why can't you simply allocate that one PSC page once at boot, accept the
> memory for it and use it throughout? Under locking, ofc, if multiple PSC
> calls need to happen in parallel...
>
> Instead of limiting the PSC req size.
There was a whole discussion on this and I would prefer to keep the
ability to parallelize PSC without locking.
>
>> @@ -1254,6 +1260,8 @@ void setup_ghcb(void)
>> if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>> snp_register_per_cpu_ghcb();
>>
>> + ghcb_percpu_ready = true;
>
> You know how I can't stand those random boolean vars stating something
> has been initialized?
>
> Can't you at least use some field in struct ghcb.reserved_1[] or so
> which the spec can provide to OS use so that FW doesn't touch it?
Well when we don't know which GHCB is in use, using that reserved area in
the GHCB doesn't help. Also, I don't want to update the GHCB specification
for a single bit that is only required because of the way Linux went about
establishing the GHCB usage.
Thanks,
Tom
>
> And then stick a "percpu_ready" bit there.
>
> Thx.
>
On Fri, Aug 12, 2022 at 09:11:25AM -0500, Tom Lendacky wrote:
> There was a whole discussion on this
Pointer to it?
> and I would prefer to keep the ability to parallelize PSC without
> locking.
So smaller, on-stack PSC but lockless is still better than a bigger one
but with synchronized accesses to it?
> Well when we don't know which GHCB is in use, using that reserved area in
> the GHCB doesn't help.
What do you mean?
The one which you read with
data = this_cpu_read(runtime_data);
in snp_register_per_cpu_ghcb() is the one you register.
> Also, I don't want to update the GHCB specification for a single bit
> that is only required because of the way Linux went about establishing
> the GHCB usage.
Linux?
You mean, you did it this way: 885689e47dfa1499b756a07237eb645234d93cf9
:-)
"The runtime handler needs one GHCB per-CPU. Set them up and map them
unencrypted."
Why does that handler need one GHCB per CPU?
As to the field, I was thinking along the lines of
struct ghcb.vendor_flags
field which each virt vendor can use however they like.
It might be overkill but a random bool ain't pretty either. Especially
if those things start getting added for all kinds of other things.
If anything, you could make this a single u64 sev_flags which can at
least collect all that gunk in one variable ... at least...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 8/12/22 09:33, Borislav Petkov wrote:
> On Fri, Aug 12, 2022 at 09:11:25AM -0500, Tom Lendacky wrote:
>> There was a whole discussion on this
>
> Pointer to it?
It starts here:
https://lore.kernel.org/lkml/658c455c40e8950cb046dd885dd19dc1c52d060a.1659103274.git.thomas.lendacky@amd.com/
>
>> and I would prefer to keep the ability to parallelize PSC without
>> locking.
>
> So smaller, on-stack PSC but lockless is still better than a bigger one
> but with synchronized accesses to it?
>
>> Well when we don't know which GHCB is in use, using that reserved area in
>> the GHCB doesn't help.
>
> What do you mean?
>
> The one which you read with
>
> data = this_cpu_read(runtime_data);
Memory acceptance is called before the per-CPU GHCBs have been allocated
and so you would be actually be using early boot GHCB. And that is decided
based on the #VC handler that is invoked - but in this case we're not
coming through the #VC handler to accept memory.
>
> in snp_register_per_cpu_ghcb() is the one you register.
>
>> Also, I don't want to update the GHCB specification for a single bit
>> that is only required because of the way Linux went about establishing
>> the GHCB usage.
>
> Linux?
>
> You mean, you did it this way: 885689e47dfa1499b756a07237eb645234d93cf9
>
> :-)
Well Joerg re-worked all that quite a bit. And with the SNP support, the
added requirement of registering the GHCB changed which GHCB could be
used. So even when the per-CPU GHCB is allocated, it can't be used until
it is registered, which depends on when the #VC handler is changed from
the boot #VC handler to the runtime #VC handler.
>
> "The runtime handler needs one GHCB per-CPU. Set them up and map them
> unencrypted."
>
> Why does that handler need one GHCB per CPU?
Each vCPU can be handling a #VC and you don't want to be serializing on a
single GHCB.
Thanks,
Tom
>
> As to the field, I was thinking along the lines of
>
> struct ghcb.vendor_flags
>
> field which each virt vendor can use however they like.
>
> It might be overkill but a random bool ain't pretty either. Especially
> if those things start getting added for all kinds of other things.
>
> If anything, you could make this a single u64 sev_flags which can at
> least collect all that gunk in one variable ... at least...
>
> Thx.
>
On Fri, Aug 12, 2022 at 09:51:41AM -0500, Tom Lendacky wrote:
> On 8/12/22 09:33, Borislav Petkov wrote:
> > On Fri, Aug 12, 2022 at 09:11:25AM -0500, Tom Lendacky wrote:
> > > There was a whole discussion on this
> >
> > Pointer to it?
>
> It starts here: https://lore.kernel.org/lkml/658c455c40e8950cb046dd885dd19dc1c52d060a.1659103274.git.thomas.lendacky@amd.com/
So how come none of the rationale for the on-stack decision vs a single
buffer with a spinlock protection hasn't made it to this patch?
We need to have the reason why this thing is changed documented
somewhere.
> > So smaller, on-stack PSC but lockless is still better than a bigger one
> > but with synchronized accesses to it?
That thing.
That decision for on-stack buffer needs explaining why.
> > > Well when we don't know which GHCB is in use, using that reserved area in
> > > the GHCB doesn't help.
> >
> > What do you mean?
> >
> > The one which you read with
> >
> > data = this_cpu_read(runtime_data);
>
> Memory acceptance is called before the per-CPU GHCBs have been allocated
> and so you would be actually be using early boot GHCB. And that is decided
> based on the #VC handler that is invoked - but in this case we're not
> coming through the #VC handler to accept memory.
But then ghcb_percpu_ready needs to be a per-CPU variable too! Because
it is set right after snp_register_per_cpu_ghcb() which works on the
*per-CPU* GHCB.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 8/13/22 14:40, Borislav Petkov wrote:
> On Fri, Aug 12, 2022 at 09:51:41AM -0500, Tom Lendacky wrote:
>> On 8/12/22 09:33, Borislav Petkov wrote:
>>> On Fri, Aug 12, 2022 at 09:11:25AM -0500, Tom Lendacky wrote:
>>>> There was a whole discussion on this
>>>
>>> Pointer to it?
>>
>> It starts here: https://lore.kernel.org/lkml/658c455c40e8950cb046dd885dd19dc1c52d060a.1659103274.git.thomas.lendacky@amd.com/
>
> So how come none of the rationale for the on-stack decision vs a single
> buffer with a spinlock protection hasn't made it to this patch?
>
> We need to have the reason why this thing is changed documented
> somewhere.
Yup, was all being addressed in v3 based on Dave's comments.
>
>>> So smaller, on-stack PSC but lockless is still better than a bigger one
>>> but with synchronized accesses to it?
>
> That thing.
>
> That decision for on-stack buffer needs explaining why.
>
>>>> Well when we don't know which GHCB is in use, using that reserved area in
>>>> the GHCB doesn't help.
>>>
>>> What do you mean?
>>>
>>> The one which you read with
>>>
>>> data = this_cpu_read(runtime_data);
>>
>> Memory acceptance is called before the per-CPU GHCBs have been allocated
>> and so you would be actually be using early boot GHCB. And that is decided
>> based on the #VC handler that is invoked - but in this case we're not
>> coming through the #VC handler to accept memory.
>
> But then ghcb_percpu_ready needs to be a per-CPU variable too! Because
> it is set right after snp_register_per_cpu_ghcb() which works on the
> *per-CPU* GHCB.
No, and the code comment will explain this. Since the APs only ever use
the per-CPU GHCB there is no concern as to when there is a switch over
from the early boot GHCB to the per-CPU GHCB, so a single global variable
is all that is needed.
I'll send out v3 soon.
Thanks,
Tom
>