Re: [PATCH v4 04/14] confidential guest support: introduce ConfidentialGuestMemoryEncryptionOps for encrypted VMs

[Ashish Kalra]

Hello Dov,

On Thu, Aug 05, 2021 at 03:20:50PM +0300, Dov Murik wrote:
> 
> 
> On 04/08/2021 14:55, Ashish Kalra wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > When memory encryption is enabled in VM, the guest RAM will be encrypted
> > with the guest-specific key, to protect the confidentiality of data while
> > in transit we need to platform specific hooks to save or migrate the
> > guest RAM.
> > 
> > Introduce the new ConfidentialGuestMemoryEncryptionOps in this patch
> > which will be later used by the encrypted guest for migration.
> 
> Do we already have SEV / ConfidentialGuest debug operations? (for
> reading SEV guest memory from gdb if debug is allowed in policy)
> 
> Are they supposed to be in the same Ops struct? Another?
> 

Not currently, the SEV debug patches were submitted as an independent
patch-set way before the ConfidentialGuestSupport was introduced, in the
future they may need to be rebased on this support.

Thanks,
Ashish

> 
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  include/exec/confidential-guest-support.h | 27 +++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/include/exec/confidential-guest-support.h 
> > b/include/exec/confidential-guest-support.h
> > index ba2dd4b5df..d8b4bd4c42 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -20,6 +20,7 @@
> > 
> >  #ifndef CONFIG_USER_ONLY
> > 
> > +#include <qapi/qapi-types-migration.h>
> >  #include "qom/object.h"
> > 
> >  #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> > @@ -53,8 +54,34 @@ struct ConfidentialGuestSupport {
> >      bool ready;
> >  };
> > 
> > +/**
> > + * The functions registers with ConfidentialGuestMemoryEncryptionOps will 
> > be
> > + * used during the encrypted guest migration.
> > + */
> > +struct ConfidentialGuestMemoryEncryptionOps {
> 
> [style] in QEMU you should add a 'typedef' at the beginning and call the
> type ConfidentialGuestMemoryEncryptionOps, and then you don't use the
> keyword 'struct' when you refer to it.  See for example the definition
> of ConfidentialGuestSupportClass below.
> 
> 
> > +    /* Initialize the platform specific state before starting the 
> > migration */
> > +    int (*save_setup)(MigrationParameters *p);
> > +
> > +    /* Write the encrypted page and metadata associated with it */
> > +    int (*save_outgoing_page)(QEMUFile *f, uint8_t *ptr, uint32_t size,
> > +                              uint64_t *bytes_sent);
> > +
> > +    /* Load the incoming encrypted page into guest memory */
> > +    int (*load_incoming_page)(QEMUFile *f, uint8_t *ptr);
> > +
> > +    /* Check if gfn is in shared/unencrypted region */
> > +    bool (*is_gfn_in_unshared_region)(unsigned long gfn);
> 
> The comment says "shared/unencrypted", but the function name talks about
> "unshared".  I prefer:
> 
>     /* Check if gfn is in encrypted region */
>     bool (*is_gfn_in_encrypted_region)(unsigned long gfn);
> 
> (and then maybe the comment is useless?)
> 
> 
> > +
> > +    /* Write the shared regions list */
> > +    int (*save_outgoing_shared_regions_list)(QEMUFile *f);
> > +
> > +    /* Load the shared regions list */
> > +    int (*load_incoming_shared_regions_list)(QEMUFile *f);
> > +};
> > +
> >  typedef struct ConfidentialGuestSupportClass {
> >      ObjectClass parent;
> > +    struct ConfidentialGuestMemoryEncryptionOps *memory_encryption_ops;
> 
> per above, remove 'struct'.
> 
> 
> >  } ConfidentialGuestSupportClass;
> > 
> >  #endif /* !CONFIG_USER_ONLY */
> >