Re: [PATCH 3/9] backends/confidential-guest-support: Add functions to support IGVM

[Roy Hopkins]

On Fri, 2024-03-01 at 16:37 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:09PM +0000, Roy Hopkins wrote:
> > In preparation for supporting the processing of IGVM files to configure
> > guests, this adds a set of functions to ConfidentialGuestSupport
> > allowing configuration of secure virtual machines that can be
> > implemented for each supported isolation platform type such as Intel TDX
> > or AMD SEV-SNP. These functions will be called by IGVM processing code
> > in subsequent patches.
> > 
> > This commit provides a default implementation of the functions that
> > either perform no action or generate a warning or error when they are
> > called. Targets that support ConfidentalGuestSupport should override
> > these implementations.
> > 
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> >  backends/confidential-guest-support.c     | 26 ++++++++
> >  include/exec/confidential-guest-support.h | 76 +++++++++++++++++++++++
> >  2 files changed, 102 insertions(+)
> > 
> > diff --git a/backends/confidential-guest-support.c b/backends/confidential-
> > guest-support.c
> > index da436fb736..42628be8d7 100644
> > --- a/backends/confidential-guest-support.c
> > +++ b/backends/confidential-guest-support.c
> > @@ -14,6 +14,7 @@
> >  #include "qemu/osdep.h"
> >  
> >  #include "exec/confidential-guest-support.h"
> > +#include "qemu/error-report.h"
> >  
> >  OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> >                              confidential_guest_support,
> > @@ -45,8 +46,33 @@ static void
> > confidential_guest_support_class_init(ObjectClass *oc, void *data)
> >  #endif
> >  }
> >  
> > +static int check_support(ConfidentialGuestPlatformType platform,
> > +                         uint16_t platform_version, uint8_t highest_vtl,
> > +                         uint64_t shared_gpa_boundary)
> > +{
> > +    /* Default: no support. */
> > +    return 0;
> > +}
> > +
> > +static int set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
> > +                                 ConfidentialGuestPageType memory_type,
> > +                                 uint16_t cpu_index)
> > +{
> > +    warn_report("Confidential guest memory not supported");
> > +    return -1;
> > +}
> > +
> > +static int get_mem_map_entry(int index, ConfidentialGuestMemoryMapEntry
> > *entry)
> > +{
> > +    return 1;
> > +}
> 
> IIUC, all these can reports errors, and as such I think
> they should have an 'Error **errp' parameter, so we can
> report precise errors in these methods, rather than
> less useful generic errors in the caller.
> 
> The above 'warn_report' ought to be an error too, since
> it is returning an failure code (-1)

Yes, that makes sense. I've made a comprehensive rework of the error handling in
the patch series which addresses this and your suggestions in the other patches
regarding error handling. I'll submit these as a V2.

> 
> > +
> >  static void confidential_guest_support_init(Object *obj)
> >  {
> > +    ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > +    cgs->check_support = check_support;
> > +    cgs->set_guest_state = set_guest_state;
> > +    cgs->get_mem_map_entry = get_mem_map_entry;
> >  }
> >  
> >  static void confidential_guest_support_finalize(Object *obj)
> > diff --git a/include/exec/confidential-guest-support.h
> > b/include/exec/confidential-guest-support.h
> > index b08ad8de4d..c43a1a32f1 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -21,10 +21,46 @@
> >  #ifndef CONFIG_USER_ONLY
> >  
> >  #include "qom/object.h"
> > +#include "exec/hwaddr.h"
> > +
> > +#if defined(CONFIG_IGVM)
> > +#include "igvm/igvm.h"
> > +#endif
> >  
> >  #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> >  OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport,
> > CONFIDENTIAL_GUEST_SUPPORT)
> >  
> > +typedef enum ConfidentialGuestPlatformType {
> > +    CGS_PLATFORM_SEV,
> > +    CGS_PLATFORM_SEV_ES,
> > +    CGS_PLATFORM_SEV_SNP,
> > +    CGS_PLATFORM_TDX,
> 
> QEMU only has support for SEV & SEV_ES today. We should leave the
> others until we actually get an impl of SEV-SNP/TDX in QEMU that
> supports those platforms.

Ok. I'll remove the currently unsupported platforms.

> 
> > +} ConfidentialGuestPlatformType;
> > +
> > +typedef enum ConfidentialGuestMemoryType {
> > +    CGS_MEM_RAM,
> > +    CGS_MEM_RESERVED,
> > +    CGS_MEM_ACPI,
> > +    CGS_MEM_NVS,
> > +    CGS_MEM_UNUSABLE,
> > +} ConfidentialGuestMemoryType;
> > +
> > +typedef struct ConfidentialGuestMemoryMapEntry {
> > +    uint64_t gpa;
> > +    uint64_t size;
> > +    ConfidentialGuestMemoryType type;
> > +} ConfidentialGuestMemoryMapEntry;
> > +
> > +typedef enum ConfidentialGuestPageType {
> > +    CGS_PAGE_TYPE_NORMAL,
> > +    CGS_PAGE_TYPE_VMSA,
> > +    CGS_PAGE_TYPE_ZERO,
> > +    CGS_PAGE_TYPE_UNMEASURED,
> > +    CGS_PAGE_TYPE_SECRETS,
> > +    CGS_PAGE_TYPE_CPUID,
> > +    CGS_PAGE_TYPE_REQUIRED_MEMORY,
> > +} ConfidentialGuestPageType;
> > +
> >  struct ConfidentialGuestSupport {
> >      Object parent;
> >  
> > @@ -60,6 +96,46 @@ struct ConfidentialGuestSupport {
> >       */
> >      char *igvm_filename;
> >  #endif
> > +
> > +    /*
> > +     * The following virtual methods need to be implemented by systems that
> > +     * support confidential guests that can be configured with IGVM and are
> > +     * used during processing of the IGVM file with process_igvm().
> > +     */
> > +
> > +    /*
> > +     * Check for to see if this confidential guest supports a particular
> > +     * platform or configuration
> > +     */
> > +    int (*check_support)(ConfidentialGuestPlatformType platform,
> > +                         uint16_t platform_version, uint8_t highest_vtl,
> > +                         uint64_t shared_gpa_boundary);
> > +
> > +    /*
> > +     * Configure part of the state of a guest for a particular set of data,
> > page
> > +     * type and gpa. This can be used for example to pre-populate and
> > measure
> > +     * guest memory contents, define private ranges or set the initial CPU
> > state
> > +     * for one or more CPUs.
> > +     *
> > +     * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the initial
> > CPU
> > +     * context for a virtual CPU. The format of the data depends on the
> > type of
> > +     * confidential virtual machine. For example, for SEV-ES ptr will point
> > to a
> > +     * vmcb_save_area structure that should be copied into guest memory at
> > the
> > +     * address specified in gpa. The cpu_index parameter contains the index
> > of
> > +     * the CPU the VMSA applies to.
> > +     */
> > +    int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
> > +                                 ConfidentialGuestPageType memory_type,
> > +                                 uint16_t cpu_index);
> > +
> > +    /*
> > +     * Iterate the system memory map, getting the entry with the given
> > index
> > +     * that can be populated into guest memory.
> > +     *
> > +     * Returns 1 if the index is out of range.
> > +     */
> > +    int (*get_mem_map_entry)(int index,
> > +                              ConfidentialGuestMemoryMapEntry *entry);
> >  };
> >  
> >  typedef struct ConfidentialGuestSupportClass {
> > -- 
> > 2.43.0
> > 
> > 
> 
> With regards,
> Daniel