Re: [PATCH 1/5] confidential guest support: Introduce a 'check' class handler

[Philippe Mathieu-Daudé]

On 5/1/23 09:46, Thomas Huth wrote:
> On 04/01/2023 12.51, Cédric Le Goater wrote:
> > From: Cédric Le Goater <clg@redhat.com>
> >
> > Some machines have specific requirements to activate confidential
> > guest support. Add a class handler to the confidential guest support
> > interface to let the arch implementation perform extra checks.
> >
> > Cc: Eduardo Habkost <eduardo@habkost.net>
> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> > Cc: Yanan Wang <wangyanan55@huawei.com>
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> >   include/exec/confidential-guest-support.h |  4 +++-
> >   hw/core/machine.c                         | 11 ++++++-----
> >   2 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/exec/confidential-guest-support.h 
> > b/include/exec/confidential-guest-support.h
> > index ba2dd4b5df..9e6d362b26 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -23,7 +23,8 @@
> >   #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,6 +56,7 @@ struct ConfidentialGuestSupport {
> >   typedef struct ConfidentialGuestSupportClass {
> >       ObjectClass parent;
> > +    bool (*check)(const Object *obj, Error **errp);
> >   } ConfidentialGuestSupportClass;
> >   #endif /* !CONFIG_USER_ONLY */
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index f589b92909..bab43cd675 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -502,11 +502,12 @@ static void 
> > machine_check_confidential_guest_support(const Object *obj,
> >                                                        Object 
> > *new_target,
> >                                                        Error **errp)
> >   {
> > -    /*
> > -     * So far the only constraint is that the target has the
> > -     * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
> > -     * by the QOM core
> > -     */
> > +    ConfidentialGuestSupportClass *cgsc =
> > +        CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(new_target);
> > +
> > +    if (cgsc->check) {
> > +        cgsc->check(obj, errp);
>
> I assume the caller is checking *errp, so it's ok to ignore the return 
> value of the check function here?

Agreed, can we change by:

  if (cgsc->check && !cgsc->check(obj, errp)) {
      return;
  }

?

Also since the handler name is not very self-descriptive, can we
add a comment in its declaration in ConfidentialGuestSupportClass?

> > +    }
> >   }