Re: [RFC 02/16] x86/kvm: Introduce KVM memory protection feature

[Vitaly Kuznetsov]

"Huang, Kai" <kai.huang@xxxxxxxxx> writes:

> On Wed, 2020-05-27 at 10:39 +0200, Vitaly Kuznetsov wrote:
>> Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
>> 
>> > On Mon, May 25, 2020 at 06:15:25PM +0300, Kirill A. Shutemov wrote:
>> > > On Mon, May 25, 2020 at 04:58:51PM +0200, Vitaly Kuznetsov wrote:
>> > > > > @@ -727,6 +734,15 @@ static void __init kvm_init_platform(void)
>> > > > >  {
>> > > > >  	kvmclock_init();
>> > > > >  	x86_platform.apic_post_init = kvm_apic_init;
>> > > > > +
>> > > > > +	if (kvm_para_has_feature(KVM_FEATURE_MEM_PROTECTED)) {
>> > > > > +		if (kvm_hypercall0(KVM_HC_ENABLE_MEM_PROTECTED)) {
>> > > > > +			pr_err("Failed to enable KVM memory
>> > > > > protection\n");
>> > > > > +			return;
>> > > > > +		}
>> > > > > +
>> > > > > +		mem_protected = true;
>> > > > > +	}
>> > > > >  }
>> > > > 
>> > > > Personally, I'd prefer to do this via setting a bit in a KVM-specific
>> > > > MSR instead. The benefit is that the guest doesn't need to remember if
>> > > > it enabled the feature or not, it can always read the config msr. May
>> > > > come handy for e.g. kexec/kdump.
>> > > 
>> > > I think we would need to remember it anyway. Accessing MSR is somewhat
>> > > expensive. But, okay, I can rework it MSR if needed.
>> > 
>> > I think Vitaly is talking about the case where the kernel can't easily get
>> > at its cached state, e.g. after booting into a new kernel.  The kernel would
>> > still have an X86_FEATURE bit or whatever, providing a virtual MSR would be
>> > purely for rare slow paths.
>> > 
>> > That being said, a hypercall plus CPUID bit might be better, e.g. that'd
>> > allow the guest to query the state without risking a #GP.
>> 
>> We have rdmsr_safe() for that! :-) MSR (and hypercall to that matter)
>> should have an associated CPUID feature bit of course.
>> 
>> Yes, hypercall + CPUID would do but normally we treat CPUID data as
>> static and in this case we'll make it a dynamically flipping
>> bit. Especially if we introduce 'KVM_HC_DISABLE_MEM_PROTECTED' later.
>
> Not sure why is KVM_HC_DISABLE_MEM_PROTECTED needed?
>

I didn't put much thought in it but we may need it to support 'kexec'
case when no reboot is performed but we either need to pass  the data
about which regions are protected from old kernel to the new one or
'unprotect exerything'.

-- 
Vitaly