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 */
> >
