Re: [RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Haibo Xu]

On Thu, 25 Mar 2021 at 23:37, Juan Quintela <quintela@redhat.com> wrote:
>
> Haibo Xu <haibo.xu@linaro.org> wrote:
> > To make it easier to keep the page tags sync with
> > the page data, tags for one page are appended to
> > the data during ram save iteration.
> >
> > This patch only add the pre-copy migration support.
> > Post-copy and compress as well as zero page saving
> > are not supported yet.
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
>
>
> >  #define RAM_SAVE_FLAG_XBZRLE   0x40
> >  /* 0x80 is reserved in migration.h start with 0x100 next */
> >  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> > +#define RAM_SAVE_FLAG_MTE              0x200
>
> Flags are really a scarce resource.  You are using one here, when you
> know that you will always have the feature enable (or not), so you can
> do better during negotiation IMHO.
>

Yes, any suggestions are welcomed to finalize the MTE migration support!

>
> > +void precopy_enable_metadata_migration(void)
> > +{
> > +    if (!ram_state) {
> > +        return;
> > +    }
> > +
> > +    ram_state->metadata_enabled = true;
> > +}
>
> My understanding is that in your following patch, if mte is enabled, you
> will always sent mte tags, for all pages needed, right?

Yes, for the current kernel support, we can't tell whether the MTE was enabled
for a specific page.

>
> > +static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > +{
> > +    uint8_t *tag_buf = NULL;
> > +    uint64_t ipa;
> > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> > +
> > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > +        /* Buffer for the page tags(one byte per tag) */
> > +        tag_buf = g_try_malloc0(size);
>
> size of the buffer is known at start of migration.  Just get a buffer
> and reuse it?

Yes, we can pre-allocate the buffer during the migration setup time

>
> Do zero pages have mte tags?  From migration point of view, a zero page
> is a page that is just full of zeros, i.e. nothing else special.
> Because you are not sending any for them.
>

Yes, I think we can do some optimization for the zero page migration support.

>
>
> > @@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock 
> > *block, ram_addr_t offset,
> >  static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t 
> > offset,
> >                              uint8_t *buf, bool async)
> >  {
> > +    if (rs->metadata_enabled) {
> > +        offset |= RAM_SAVE_FLAG_MTE;
>
> You don't really need the flag, for you normal pages are just
> TARGET_PAGE_SIZE + (TARGET_PAGE_SIZE/MTE_)
>

So you suggest to use the size field to indicate whether tags are available?

>
> > +    }
> > +
> >      ram_counters.transferred += save_page_header(rs, rs->f, block,
> >                                                   offset | 
> > RAM_SAVE_FLAG_PAGE);
> >      if (async) {
> > @@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock 
> > *block, ram_addr_t offset,
> >      }
> >      ram_counters.transferred += TARGET_PAGE_SIZE;
> >      ram_counters.normal++;
> > +
> > +    if (rs->metadata_enabled) {
>
> See?  You are not checking the flag, you are checking the bool setup at
> the beggining of migration.

The idea is to only migrate the memory tags when MTE was enabled for the VM.
Could you be more elaborate on "You are not checking the flag"?

>
> > +        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
> > +    }
> > +
> >      return 1;
> >  }
> >
> > @@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
> >      rs->last_version = ram_list.version;
> >      rs->ram_bulk_stage = true;
> >      rs->fpo_enabled = false;
> > +    rs->metadata_enabled = false;
> >  }
> >
> >  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> > @@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
> >              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
> >          }
> >
> > -        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> > +        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {
>
> Creating the flag is hurting you here also.
>
> >          case RAM_SAVE_FLAG_MEM_SIZE:
> >              /* Synchronize RAM block list */
> >              total_ram_bytes = addr;
> > @@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
> >
> >          case RAM_SAVE_FLAG_PAGE:
> >              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > +            if (flags & RAM_SAVE_FLAG_MTE) {
> > +                load_normal_page_mte_tags(f, host);
> > +            }
>
> I don't claim to understand the MTE, but my understanding is that if we
> are using MTE, all pages have to have MTE flags, right?
>
> So, somtehing like
>
> is_mte_enabled()
>
> that I told in the other thread looks like a good idea.
>

Yes, we do need a function like this.

> Later, Juan.
>
> >              break;
> >
> >          case RAM_SAVE_FLAG_COMPRESS_PAGE:
>