Re: [Part1 PATCH v6 16/17] X86/KVM: Decrypt shared per-cpu variables when SEV is active

[Brijesh Singh <brijesh.singh@amd.com>]

On 10/16/17 5:24 PM, Borislav Petkov wrote:
...

>>  
>> +static inline void __set_percpu_decrypted(void *ptr, unsigned long size)
>> +{
>> +	early_set_memory_decrypted(slow_virt_to_phys(ptr), size);
>> +}
> Ok, so this looks like useless conversion:
>
> you pass in a virtual address, it gets converted to a physical address
> with slow_virt_to_phys() and then in early_set_memory_enc_dec() gets
> converted to a virtual address again.
>
> Why? Why not pass the virtual address directly?


Actually, I worked to enable the kvmclock support before the
kvm-stealtime, eoi and apf_reason. The kvmclock uses memblock_alloc() to
allocate the shared memory and since the memblock_alloc() returns the
physical address hence I used the same input type as a argument to the
early_set_memory_decrypted(). If you want me to change the input to
accept the virtual address then I have no issue doing so. But the
changes need to propagated to kvmclock (i.e PATCH 17/17) to use __va().
Please let me know if you want me to pass the virtual address.


>> +/*
>> + * Iterate through all possible CPUs and map the memory region pointed
>> + * by apf_reason, steal_time and kvm_apic_eoi as decrypted at once.
>> + *
>> + * Note: we iterate through all possible CPUs to ensure that CPUs
>> + * hotplugged will have their per-cpu variable already mapped as
>> + * decrypted.
>> + */
>> +static void __init sev_map_percpu_data(void)
>> +{
>> +	int cpu;
>> +
>> +	if (!sev_active())
>> +		return;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		__set_percpu_decrypted(&per_cpu(apf_reason, cpu), sizeof(apf_reason));
>> +		__set_percpu_decrypted(&per_cpu(steal_time, cpu), sizeof(steal_time));
>> +		__set_percpu_decrypted(&per_cpu(kvm_apic_eoi, cpu), sizeof(kvm_apic_eoi));
>> +	}
>> +}
>> +
>>  #ifdef CONFIG_SMP
>>  static void __init kvm_smp_prepare_boot_cpu(void)
>>  {
>> +	sev_map_percpu_data();
>>  	kvm_guest_cpu_init();
>>  	native_smp_prepare_boot_cpu();
>>  	kvm_spinlock_init();
>> @@ -496,6 +524,7 @@ void __init kvm_guest_init(void)
>>  				      kvm_cpu_online, kvm_cpu_down_prepare) < 0)
>>  		pr_err("kvm_guest: Failed to install cpu hotplug callbacks\n");
>>  #else
>> +	sev_map_percpu_data();
>>  	kvm_guest_cpu_init();
>>  #endif
> Why isn't it enough to call
>
> 	sev_map_percpu_data()
>
> at the end of kvm_guest_init() only but you have to call it in
> kvm_smp_prepare_boot_cpu() too? I mean, once you map those things
> decrypted, there's no need to do them again...
>

IIRC, we tried clearing C bit in kvm_guest_init() but since the
kvm_guest_init() is called before setup_per_cpu_areas() hence
per_cpu_ptr(var, cpu_id) was not able to get another processors copy of
the variable.