Re: [RFC PATCH v3 05/36] i386/tdx: Implement tdx_kvm_init() to initialize TDX VM context

[Isaku Yamahata]

On Thu, Mar 17, 2022 at 09:58:42PM +0800,
Xiaoyao Li <xiaoyao.li@intel.com> wrote:

> Introduce tdx_kvm_init() and invoke it in kvm_confidential_guest_init()
> if it's a TDX VM. More initialization will be added later.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/kvm/kvm.c       | 15 ++++++---------
>  target/i386/kvm/meson.build |  2 +-
>  target/i386/kvm/tdx-stub.c  |  9 +++++++++
>  target/i386/kvm/tdx.c       | 13 +++++++++++++
>  target/i386/kvm/tdx.h       |  2 ++
>  5 files changed, 31 insertions(+), 10 deletions(-)
>  create mode 100644 target/i386/kvm/tdx-stub.c
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 70454355f3bf..26ed5faf07b8 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -54,6 +54,7 @@
>  #include "migration/blocker.h"
>  #include "exec/memattrs.h"
>  #include "trace.h"
> +#include "tdx.h"
>  
>  //#define DEBUG_KVM
>  
> @@ -2360,6 +2361,8 @@ static int kvm_confidential_guest_init(MachineState 
> *ms, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(ms->cgs), TYPE_SEV_GUEST)) {
>          return sev_kvm_init(ms->cgs, errp);
> +    } else if (object_dynamic_cast(OBJECT(ms->cgs), TYPE_TDX_GUEST)) {
> +        return tdx_kvm_init(ms, errp);
>      }
>  
>      return 0;
> @@ -2374,16 +2377,10 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      Error *local_err = NULL;
>  
>      /*
> -     * Initialize SEV context, if required
> +     * Initialize confidential guest (SEV/TDX) context, if required
>       *
> -     * If no memory encryption is requested (ms->cgs == NULL) this is
> -     * a no-op.
> -     *
> -     * It's also a no-op if a non-SEV confidential guest support
> -     * mechanism is selected.  SEV is the only mechanism available to
> -     * select on x86 at present, so this doesn't arise, but if new
> -     * mechanisms are supported in future (e.g. TDX), they'll need
> -     * their own initialization either here or elsewhere.
> +     * It's a no-op if a non-SEV/non-tdx confidential guest support
> +     * mechanism is selected, i.e., ms->cgs == NULL
>       */
>      ret = kvm_confidential_guest_init(ms, &local_err);
>      if (ret < 0) {
> diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
> index b2d7d41acde2..fd30b93ecec9 100644
> --- a/target/i386/kvm/meson.build
> +++ b/target/i386/kvm/meson.build
> @@ -9,7 +9,7 @@ i386_softmmu_kvm_ss.add(files(
>  
>  i386_softmmu_kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
>  
> -i386_softmmu_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'))
> +i386_softmmu_kvm_ss.add(when: 'CONFIG_TDX', if_true: files('tdx.c'), 
> if_false: files('tdx-stub.c'))
>  
>  i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), 
> if_false: files('hyperv-stub.c'))
>  
> diff --git a/target/i386/kvm/tdx-stub.c b/target/i386/kvm/tdx-stub.c
> new file mode 100644
> index 000000000000..1df24735201e
> --- /dev/null
> +++ b/target/i386/kvm/tdx-stub.c
> @@ -0,0 +1,9 @@
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +
> +#include "tdx.h"
> +
> +int tdx_kvm_init(MachineState *ms, Error **errp)
> +{
> +    return -EINVAL;
> +}
> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
> index d3792d4a3d56..e3b94373b316 100644
> --- a/target/i386/kvm/tdx.c
> +++ b/target/i386/kvm/tdx.c
> @@ -12,10 +12,23 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "qom/object_interfaces.h"
>  
> +#include "hw/i386/x86.h"
>  #include "tdx.h"
>  
> +int tdx_kvm_init(MachineState *ms, Error **errp)
> +{
> +    TdxGuest *tdx = (TdxGuest *)object_dynamic_cast(OBJECT(ms->cgs),
> +                                                    TYPE_TDX_GUEST);

The caller already checks it.  This is redundant. Maybe assert?


-- 
Isaku Yamahata <isaku.yamahata@gmail.com>