lkml.org 
[lkml]   [2024]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v19 069/130] KVM: TDX: Require TDP MMU and mmio caching for TDX
    From
    On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote:
    > From: Isaku Yamahata <isaku.yamahata@intel.com>
    >
    > As TDP MMU is becoming main stream than the legacy MMU, the legacy MMU
    > support for TDX isn't implemented. TDX requires KVM mmio caching. Disable
    > TDX support when TDP MMU or mmio caching aren't supported.
    >
    > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
    > ---
    > arch/x86/kvm/mmu/mmu.c | 1 +
    > arch/x86/kvm/vmx/main.c | 13 +++++++++++++
    > 2 files changed, 14 insertions(+)
    >
    > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
    > index 0e0321ad9ca2..b8d6ce02e66d 100644
    > --- a/arch/x86/kvm/mmu/mmu.c
    > +++ b/arch/x86/kvm/mmu/mmu.c
    > @@ -104,6 +104,7 @@ module_param_named(flush_on_reuse, force_flush_and_sync_on_reuse, bool, 0644);
    > * If the hardware supports that we don't need to do shadow paging.
    > */
    > bool tdp_enabled = false;
    > +EXPORT_SYMBOL_GPL(tdp_enabled);

    I haven't looked at the rest of the series, but this should be unnecessary. Just
    use enable_ept. Ah, the code is wrong.

    > static bool __ro_after_init tdp_mmu_allowed;
    >
    > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
    > index 076a471d9aea..54df6653193e 100644
    > --- a/arch/x86/kvm/vmx/main.c
    > +++ b/arch/x86/kvm/vmx/main.c
    > @@ -3,6 +3,7 @@
    >
    > #include "x86_ops.h"
    > #include "vmx.h"
    > +#include "mmu.h"
    > #include "nested.h"
    > #include "pmu.h"
    > #include "tdx.h"
    > @@ -36,6 +37,18 @@ static __init int vt_hardware_setup(void)
    > if (ret)
    > return ret;
    >
    > + /* TDX requires KVM TDP MMU. */

    This is a useless comment (assuming the code is correctly written), it's quite
    obvious from:

    if (!tdp_mmu_enabled)
    enable_tdx = false;

    that the TDP MMU is required. Explaining *why* could be useful, but I'd probably
    just omit a comment. In the not too distant future, it will likely be common
    knowledge that the shadow MMU doesn't support newfangled features, and it _should_
    be very easy for someone to get the info from the changelog.

    > + if (enable_tdx && !tdp_enabled) {

    tdp_enabled can be true without the TDP MMU, you want tdp_mmu_enabled.

    > + enable_tdx = false;
    > + pr_warn_ratelimited("TDX requires TDP MMU. Please enable TDP MMU for TDX.\n");

    Drop the pr_warn(), TDX will presumably be on by default at some point, I don't
    want to get spam every time I test with TDP disabled.

    Also, ratelimiting this code is pointless (as is _once()), it should only ever
    be called once per module module, and the limiting/once protections are tied to
    the module, i.e. effectively get reset when a module is reloaded.

    > + }
    > +
    > + /* TDX requires MMIO caching. */
    > + if (enable_tdx && !enable_mmio_caching) {
    > + enable_tdx = false;
    > + pr_warn_ratelimited("TDX requires mmio caching. Please enable mmio caching for TDX.\n");

    Same comments here.

    > + }
    > +
    > enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);

    All of the above code belongs in tdx_hardware_setup(), especially since you need
    tdp_mmu_enabled, not enable_ept.

    > if (enable_tdx)
    > vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size,
    > --
    > 2.25.1
    >

    \
     
     \ /
      Last update: 2024-05-27 16:18    [W:2.427 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site