Re: [PATCH v6 1/5] x86/kvm: Add AMD SEV specific Hypercall3

[Ashish Kalra <ashish.kalra@amd.com>]

Hello Boris,

On Wed, Sep 22, 2021 at 11:38:08AM +0200, Borislav Petkov wrote:
> On Tue, Sep 21, 2021 at 04:07:03PM +0000, Sean Christopherson wrote:
> > init_hypervisor_platform(), after x86_init.hyper.init_platform() so that the
> > PV support can set the desired feature flags.  Since kvm_hypercall*() is only
> > used by KVM guests, set_cpu_cap(c, X86_FEATURE_VMMCALL) can be moved out of
> > early_init_amd/hygon() and into kvm_init_platform().
> 
> See below.
> 
> > Another option would be to refactor apply_alternatives() to allow
> > the caller to provide a different feature check mechanism than
> > boot_cpu_has(), which I think would let us drop X86_FEATURE_VMMCALL,
> > X86_FEATURE_VMCALL, and X86_FEATURE_VMW_VMMCALL from cpufeatures. That
> > might get more than a bit gross though.
> 
> Uuuf.
> 
> So here's what I'm seeing (line numbers given to show when stuff
> happens):
> 
> start_kernel
> |-> 953: setup_arch
>     |-> 794: early_cpu_init
>     |-> 936: init_hypervisor_platform
> |
> |-> 1134: check_bugs
> 	  |-> alternative_instructions
> 
> at line 794 setup_arch() calls early_cpu_init() which would end up
> setting X86_FEATURE_VMMCALL on an AMD guest, based on CPUID information.
> 
> init_hypervisor_platform() happens after that.
> 
> The alternatives patching happens in check_bugs() at line 1134. Which
> means, if one would consider moving the patching up, one would have
> to audit all the code between line 953 and 1134, whether it does
> set_cpu_cap() or some of the other helpers to set or clear bits in
> boot_cpu_data which controls the patching.
> 
> So for that I have only one thing to say: can'o'worms. We tried to move
> the memblock allocations placement in the boot process and generated at
> least 4 regressions. I'm still testing the fix for the fix for the 4th
> regression.
> 
> So moving stuff in the fragile boot process makes my hair stand up.
> 
> Refactoring apply_alternatives() to patch only for X86_FEATURE_VMMCALL
> and then patch again, I dunno, this stuff is fragile and it might cause
> some other similarly nasty fallout. And those are hard to debug because
> one does not see immediately when boot_cpu_data features are missing and
> functionality is behaving differently because of that.
> 
> So what's wrong with:
> 
> kvm_hypercall3:
> 
> 	if (cpu_feature_enabled(X86_FEATURE_VMMCALL))
> 		return kvm_sev_hypercall3(...);
> 
> 	/* rest of code */
> 
> ?
> 
> Dunno we probably had that already in those old versions and maybe that
> was shot down for another reason but it should get you what you want
> without having to test the world and more for regressions possibly
> happening from disturbing the house of cards called x86 boot order.
> 
> IMHO, I'd say.
> 

Thanks for the above explanation.

If we have to do this:
 	if (cpu_feature_enabled(X86_FEATURE_VMMCALL))
 		return kvm_sev_hypercall3(...);

Then isn't it cleaner to simply do it via the paravirt_ops interface,
i.e, pv_ops.mmu.notify_page_enc_status_changed() where the callback
is only set when SEV and live migration feature are supported and
invoked through early_set_memory_decrypted()/encrypted().

Another memory encryption platform can set it's callback accordingly.

Thanks,
Ashish

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cashish.kalra%40amd.com%7C02217ac26c444833d50208d97dacb5f0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637679003031781718%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1oXxchRABGifVoLwnXwQxQ7%2F%2FZpwGLqpGdma4Yz5sjw%3D&reserved=0