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

[Xiaoyao Li]

On 3/18/2022 10:07 AM, Isaku Yamahata wrote:
> 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?

the cast is to get TdxGuest pointer for later usage. I can move it the 
patch that really uses tdx pointer.

Thanks,
-Xiaoyao

>