Re: [PATCH 03/26] confidential guest support: Add kvm_init() and kvm_reset() in class

[Paolo Bonzini]

On Mon, Mar 25, 2024 at 9:33 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Xiaoyao,
>
> On 22/3/24 19:10, Paolo Bonzini wrote:
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> >
> > Different confidential VMs in different architectures all have the same
> > needs to do their specific initialization (and maybe resetting) stuffs
> > with KVM. Currently each of them exposes individual *_kvm_init()
> > functions and let machine code or kvm code to call it.
> >
> > To facilitate the introduction of confidential guest technology from
> > different x86 vendors, add two virtual functions, kvm_init() and kvm_reset()
> > in ConfidentialGuestSupportClass, and expose two helpers functions for
> > invodking them.
> >
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > Message-Id: <20240229060038.606591-1-xiaoyao.li@intel.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   include/exec/confidential-guest-support.h | 34 ++++++++++++++++++++++-
> >   1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/exec/confidential-guest-support.h 
> > b/include/exec/confidential-guest-support.h
> > index ba2dd4b5dfc..e5b188cffbf 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -23,7 +23,10 @@
> >   #include "qom/object.h"
> >
> >   #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> > -OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, 
> > CONFIDENTIAL_GUEST_SUPPORT)
> > +OBJECT_DECLARE_TYPE(ConfidentialGuestSupport,
> > +                    ConfidentialGuestSupportClass,
> > +                    CONFIDENTIAL_GUEST_SUPPORT)
> > +
> >
> >   struct ConfidentialGuestSupport {
> >       Object parent;
> > @@ -55,8 +58,37 @@ struct ConfidentialGuestSupport {
> >
> >   typedef struct ConfidentialGuestSupportClass {
> >       ObjectClass parent;
> > +
> > +    int (*kvm_init)(ConfidentialGuestSupport *cgs, Error **errp);
> > +    int (*kvm_reset)(ConfidentialGuestSupport *cgs, Error **errp);
>
> Can we get a docstring indicating what these functions return?
> Looking at the next patch, the KVM specific return value doesn't
> seem used, so can we return a boolean instead?

It is propagated all the way up to accel_init_machine().  _There_ it
isn't used, but I think it's not a good idea to return a "wrong" value
from kvm_arch_init() because we know that ultimately it isn't used. It
should be refactored top-down, though I admit that to be honest it
won't happen.

Paolo