lkml.org 
[lkml]   [2023]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v10 021/108] KVM: TDX: initialize VM with TDX specific parameters
    On Wed, Jan 04, 2023 at 03:59:08PM +0800,
    "Wang, Lei" <lei4.wang@intel.com> wrote:

    > On 10/30/2022 2:22 PM, isaku.yamahata@intel.com wrote:
    > > From: Xiaoyao Li <xiaoyao.li@intel.com>
    > >
    > > TDX requires additional parameters for TDX VM for confidential execution to
    > > protect its confidentiality of its memory contents and its CPU state from
    > > any other software, including VMM. When creating guest TD VM before
    > > creating vcpu, the number of vcpu, TSC frequency (that is same among
    > > vcpus. and it can't be changed.) CPUIDs which is emulated by the TDX
    > > module. It means guest can trust those CPUIDs. and sha384 values for
    > > measurement.
    > >
    > > Add new subcommand, KVM_TDX_INIT_VM, to pass parameters for TDX guest. It
    > > assigns encryption key to the TDX guest for memory encryption. TDX
    > > encrypts memory per-guest bases. It assigns device model passes per-VM
    > > parameters for the TDX guest. The maximum number of vcpus, tsc frequency
    > > (TDX guest has fised VM-wide TSC frequency. not per-vcpu. The TDX guest
    > > can not change it.), attributes (production or debug), available extended
    > > features (which is reflected into guest XCR0, IA32_XSS MSR), cpuids, sha384
    > > measurements, and etc.
    > >
    > > This subcommand is called before creating vcpu and KVM_SET_CPUID2, i.e.
    > > cpuids configurations aren't available yet. So CPUIDs configuration values
    > > needs to be passed in struct kvm_init_vm. It's device model responsibility
    >
    > I suppose this should be kvm_tdx_init_vm.
    >
    > > to make this cpuid config for KVM_TDX_INIT_VM and KVM_SET_CPUID2.
    > >
    > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
    > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
    > > ---
    > > arch/x86/include/asm/tdx.h | 3 +
    > > arch/x86/include/uapi/asm/kvm.h | 31 +++
    > > arch/x86/kvm/vmx/tdx.c | 296 ++++++++++++++++++++++----
    > > arch/x86/kvm/vmx/tdx.h | 22 ++
    > > tools/arch/x86/include/uapi/asm/kvm.h | 33 +++
    > > 5 files changed, 347 insertions(+), 38 deletions(-)
    > >
    > > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
    > > index cd304d323d33..05ac4bfc8f8a 100644
    > > --- a/arch/x86/include/asm/tdx.h
    > > +++ b/arch/x86/include/asm/tdx.h
    > > @@ -131,6 +131,9 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
    > > #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
    > >
    > > #ifdef CONFIG_INTEL_TDX_HOST
    > > +
    > > +/* -1 indicates CPUID leaf with no sub-leaves. */
    > > +#define TDX_CPUID_NO_SUBLEAF ((u32)-1)
    > > struct tdx_cpuid_config {
    > > u32 leaf;
    > > u32 sub_leaf;
    > > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
    > > index 2ad9666e02a5..26661879c031 100644
    > > --- a/arch/x86/include/uapi/asm/kvm.h
    > > +++ b/arch/x86/include/uapi/asm/kvm.h
    > > @@ -538,6 +538,7 @@ struct kvm_pmu_event_filter {
    > > /* Trust Domain eXtension sub-ioctl() commands. */
    > > enum kvm_tdx_cmd_id {
    > > KVM_TDX_CAPABILITIES = 0,
    > > + KVM_TDX_INIT_VM,
    > >
    > > KVM_TDX_CMD_NR_MAX,
    > > };
    > > @@ -583,4 +584,34 @@ struct kvm_tdx_capabilities {
    > > struct kvm_tdx_cpuid_config cpuid_configs[0];
    > > };
    > >
    > > +struct kvm_tdx_init_vm {
    > > + __u64 attributes;
    > > + __u64 mrconfigid[6]; /* sha384 digest */
    > > + __u64 mrowner[6]; /* sha384 digest */
    > > + __u64 mrownerconfig[6]; /* sha348 digest */
    > > + union {
    > > + /*
    > > + * KVM_TDX_INIT_VM is called before vcpu creation, thus before
    > > + * KVM_SET_CPUID2. CPUID configurations needs to be passed.
    > > + *
    > > + * This configuration supersedes KVM_SET_CPUID{,2}.
    > > + * The user space VMM, e.g. qemu, should make them consistent
    > > + * with this values.
    > > + * sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES(256)
    > > + * = 8KB.
    > > + */
    > > + struct {
    > > + struct kvm_cpuid2 cpuid;
    > > + /* 8KB with KVM_MAX_CPUID_ENTRIES. */
    > > + struct kvm_cpuid_entry2 entries[];
    > > + };
    > > + /*
    > > + * For future extensibility.
    > > + * The size(struct kvm_tdx_init_vm) = 16KB.
    > > + * This should be enough given sizeof(TD_PARAMS) = 1024
    > > + */
    > > + __u64 reserved[2029];
    > > + };
    > > +};
    > > +
    > > #endif /* _ASM_X86_KVM_H */
    > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
    > > index d77709a6da51..54045e0576e7 100644
    > > --- a/arch/x86/kvm/vmx/tdx.c
    > > +++ b/arch/x86/kvm/vmx/tdx.c
    > > @@ -284,6 +284,205 @@ static int tdx_do_tdh_mng_key_config(void *param)
    > > int tdx_vm_init(struct kvm *kvm)
    > > {
    > > struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
    > > +
    > > + kvm_tdx->hkid = -1;
    > > +
    > > + /*
    > > + * This function initializes only KVM software construct. It doesn't
    > > + * initialize TDX stuff, e.g. TDCS, TDR, TDCX, HKID etc.
    > > + * It is handled by KVM_TDX_INIT_VM, __tdx_td_init().
    > > + */
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +int tdx_dev_ioctl(void __user *argp)
    > > +{
    > > + struct kvm_tdx_capabilities __user *user_caps;
    > > + struct kvm_tdx_capabilities caps;
    > > + struct kvm_tdx_cmd cmd;
    > > +
    > > + BUILD_BUG_ON(sizeof(struct kvm_tdx_cpuid_config) !=
    > > + sizeof(struct tdx_cpuid_config));
    > > +
    > > + if (copy_from_user(&cmd, argp, sizeof(cmd)))
    > > + return -EFAULT;
    > > + if (cmd.flags || cmd.error || cmd.unused)
    > > + return -EINVAL;
    > > + /*
    > > + * Currently only KVM_TDX_CAPABILITIES is defined for system-scoped
    > > + * mem_enc_ioctl().
    > > + */
    > > + if (cmd.id != KVM_TDX_CAPABILITIES)
    > > + return -EINVAL;
    > > +
    > > + user_caps = (void __user *)cmd.data;
    > > + if (copy_from_user(&caps, user_caps, sizeof(caps)))
    > > + return -EFAULT;
    > > +
    > > + if (caps.nr_cpuid_configs < tdx_caps.nr_cpuid_configs)
    > > + return -E2BIG;
    > > +
    > > + caps = (struct kvm_tdx_capabilities) {
    > > + .attrs_fixed0 = tdx_caps.attrs_fixed0,
    > > + .attrs_fixed1 = tdx_caps.attrs_fixed1,
    > > + .xfam_fixed0 = tdx_caps.xfam_fixed0,
    > > + .xfam_fixed1 = tdx_caps.xfam_fixed1,
    > > + .nr_cpuid_configs = tdx_caps.nr_cpuid_configs,
    > > + .padding = 0,
    > > + };
    > > +
    > > + if (copy_to_user(user_caps, &caps, sizeof(caps)))
    > > + return -EFAULT;
    > > + if (copy_to_user(user_caps->cpuid_configs, &tdx_caps.cpuid_configs,
    > > + tdx_caps.nr_cpuid_configs *
    > > + sizeof(struct tdx_cpuid_config)))
    > > + return -EFAULT;
    > > +
    > > + return 0;
    > > +}
    >
    > tdx_dev_ioctl() is introduced in previous patch with the same code added here,
    > which means this is just a place change and it will confuse reviewers. Is it
    > neccesary to do so?

    Right. Somehow diff was also confused. I'll try to twist the previous patch to
    introduce __tdx_td_init() with the previous patch.


    ...

    > > +static inline bool is_td_initialized(struct kvm *kvm)
    > > +{
    > > + return to_kvm_tdx(kvm)->hkid > 0;
    > > +}
    >
    > There is a similar function, is_hkid_assigned(), which is previously defined. Do
    > you think redefining a new function here will bring code redundency?

    I'll drop this function and replace it with is_hkid_assigned().
    --
    Isaku Yamahata <isaku.yamahata@gmail.com>

    \
     
     \ /
      Last update: 2023-03-26 23:37    [W:7.481 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site