Re: [PATCH v4 10/14] migration: add support to migrate shared regions list

[Ashish Kalra]

Hello Wang, 

On Fri, Sep 10, 2021 at 07:54:10AM +0000, Wang, Wei W wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > When memory encryption is enabled, the hypervisor maintains a shared
> > regions list which is referred by hypervisor during migration to check if 
> > page is
> > private or shared. This list is built during the VM bootup and must be 
> > migrated
> > to the target host so that hypervisor on target host can use it for future
> > migration.
> > 
> > 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/sysemu/sev.h |  2 ++
> >  target/i386/sev.c    | 43
> > +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h index
> > 3b913518c0..118ee66406 100644
> > --- a/include/sysemu/sev.h
> > +++ b/include/sysemu/sev.h
> > @@ -32,5 +32,7 @@ void sev_es_set_reset_vector(CPUState *cpu);  int
> > sev_remove_shared_regions_list(unsigned long gfn_start,
> >                                     unsigned long gfn_end);  int
> > sev_add_shared_regions_list(unsigned long gfn_start, unsigned long gfn_end);
> > +int sev_save_outgoing_shared_regions_list(QEMUFile *f); int
> > +sev_load_incoming_shared_regions_list(QEMUFile *f);
> > 
> >  #endif
> > diff --git a/target/i386/sev.c b/target/i386/sev.c index
> > 6d44b7ad21..789051f7b4 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -135,10 +135,15 @@ static const char *const sev_fw_errlist[] = {
> > 
> >  #define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB
> > */
> > 
> > +#define SHARED_REGION_LIST_CONT     0x1
> > +#define SHARED_REGION_LIST_END      0x2
> > +
> >  static struct ConfidentialGuestMemoryEncryptionOps
> > sev_memory_encryption_ops = {
> >      .save_setup = sev_save_setup,
> >      .save_outgoing_page = sev_save_outgoing_page,
> >      .load_incoming_page = sev_load_incoming_page,
> > +    .save_outgoing_shared_regions_list =
> > sev_save_outgoing_shared_regions_list,
> > +    .load_incoming_shared_regions_list =
> > + sev_load_incoming_shared_regions_list,
> 
> Hi Ashish,
> I have some questions about the callbacks:
> 
> 1) why using a list of shared regions, instead of bitmaps to record 
> private/shared state?
> I saw that the KVM side implementation used bitmaps in the first place and 
> changed to
> shared regions since v10, but don't find the reason.
> 

There has been a long discussion on this implementation on KVM mailing
list. Tracking shared memory via a list of ranges instead of using bitmap
is more optimal. Most of the guest memory will be private and the
unencrypted/shared regions are basically ranges/intervals, so easy to
implement and maintain using lists.

A list will consume much less memory than a bitmap. 

The bitmap will consume more memory as it will need to be sized as per
guest RAM size and will remain sparsely populated due to limited amount
of shared/unencrypted guest memory regions.

> 2) why is the save/load of shared region list (or bitmap) made vendor 
> specific?
> I think it can be a common interface and data structure, e.g. KVM maintains a 
> per memory slot
> bitmap to be obtained by QEMU.
> 

As the shared regions list current implementation is vendor specific,
it's migration is also vendor specific.

But you are right, this can be implemented as a common interface and
data stucture in qemu, but it is a separate data structure and not
maintained as the per memory slot bitmap in KVM and obtained as such by
qemu.

Thanks,
Ashish