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
