lkml.org 
[lkml]   [2022]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/2] x86/sev: Put PSC struct on the stack in prep for unaccepted memory support
From
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;
>> }
>>
>

\
 
 \ /
  Last update: 2022-08-09 00:19    [W:0.549 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site