Re: [PATCH v4 04/14] confidential guest support: introduce ConfidentialGuestMemoryEncryptionOps for encrypted VMs
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 > > > > 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 > > Co-developed-by: Ashish Kalra > > Signed-off-by: Ashish Kalra > > --- > > 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 > > #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 */ > >
Re: [PATCH v4 04/14] confidential guest support: introduce ConfidentialGuestMemoryEncryptionOps for encrypted VMs
On 04/08/2021 14:55, Ashish Kalra wrote: > From: Brijesh Singh > > 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? > > Signed-off-by: Brijesh Singh > Co-developed-by: Ashish Kalra > Signed-off-by: Ashish Kalra > --- > 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 > #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 */ >
[PATCH v4 04/14] confidential guest support: introduce ConfidentialGuestMemoryEncryptionOps for encrypted VMs
From: Brijesh Singh 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. Signed-off-by: Brijesh Singh Co-developed-by: Ashish Kalra Signed-off-by: Ashish Kalra --- 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 #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 { +/* 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); + +/* 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; } ConfidentialGuestSupportClass; #endif /* !CONFIG_USER_ONLY */ -- 2.17.1