| Message ID | 20230104115111.3240594-2-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/pv: Improve protected VM support | expand |
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? > + } > } > > static bool machine_get_nvdimm(Object *obj, Error **errp) Reviewed-by: Thomas Huth <thuth@redhat.com>
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? >> + } >> }
On 1/5/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? yes. routine machine_check_confidential_guest_support() is a direct parameter of object_class_property_add_link() call, which checks *errp. See routine object_set_link_property(). Thanks, C. > >> + } >> } >> static bool machine_get_nvdimm(Object *obj, Error **errp) > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
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); + } } static bool machine_get_nvdimm(Object *obj, Error **errp)