lkml.org 
[lkml]   [2021]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6 1/5] x86/kvm: Add AMD SEV specific Hypercall3
    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.

    --
    Regards/Gruss,
    Boris.

    https://people.kernel.org/tglx/notes-about-netiquette

    \
     
     \ /
      Last update: 2021-09-22 11:39    [W:2.400 / U:0.288 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site