Re: [Qemu-devel] [PATCH] migration: discard RAMBlocks of type ram_device

[Dr. David Alan Gilbert]

* Peter Maydell (address@hidden) wrote:
> On 12 April 2018 at 11:18, Cédric Le Goater <address@hidden> wrote:
> > On the POWER9 processor, the XIVE interrupt controller can control
> > interrupt sources using MMIO to trigger events, to EOI or to turn off
> > the sources. Priority management and interrupt acknowledgment is also
> > controlled by MMIO in the presenter sub-engine.
> >
> > These MMIO regions are exposed to guests in QEMU with a set of 'ram
> > device' memory mappings, similarly to VFIO, and the VMAs are populated
> > dynamically with the appropriate pages using a fault handler.
> >
> > But, these regions are an issue for migration. We need to discard the
> > associated RAMBlocks from the RAM state on the source VM and let the
> > destination VM rebuild the memory mappings on the new host in the
> > post_load() operation just before resuming the system.
> >
> > To achieve this goal, the following introduces a new helper,
> > ram_block_is_migratable(), which identifies RAMBlocks to discard on
> > the source. Some checks are also performed on the destination to make
> > sure nothing invalid was sent.
> 
> David suggested on IRC that we would want a flag on the ramblock
> for "not migratable", because there are other uses for "don't
> migrate this" than just "is this a ram device".

My original suggestion to your series was with a flag, but I'd forgotten
about that by the time I'd made the suggestion to Cédric.
In your case would just adding an extra term to the
ram_block_is_migratable function work, or do you really need a flag?

Dave

> > Signed-off-by: Cédric Le Goater <address@hidden>
> > ---
> >
> >  I am not sure we want to taker into account patchew complaint :
> >
> >    ERROR: braces {} are necessary for all arms of this statement
> >    #52: FILE: migration/ram.c:203:
> >    +        if (ram_block_is_migratable(block))
> >    [...]
> >
> >    total: 1 errors, 0 warnings, 136 lines checked
> >
> >  migration/ram.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 42 insertions(+), 10 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 0e90efa09236..32371950865b 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -188,6 +188,21 @@ void ramblock_recv_bitmap_set_range(RAMBlock *rb, void 
> > *host_addr,
> >  }
> >
> >  /*
> > + * Identifies RAM blocks which should be discarded from migration. For
> > + * the moment, it only applies to blocks backed by a 'ram_device'
> > + * memory region.
> > + */
> > +static inline bool ram_block_is_migratable(RAMBlock *block)
> > +{
> > +    return !memory_region_is_ram_device(block->mr);
> > +}
> > +
> > +/* Should be holding either ram_list.mutex, or the RCU lock. */
> > +#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
> > +    RAMBLOCK_FOREACH(block)                            \
> > +        if (ram_block_is_migratable(block))
> 
> This will mishandle some uses, like:
> 
>     if (foo)
>         RAMBLOCK_FOREACH_MIGRATABLE(block)
>             stuff;
>     else
>         morestuff;
> 
> as the if() inside the macro will capture the else clause.
> (The lack of braces in the calling code would be against our
> coding style, of course, so not very likely.)
> 
> Eric, is there a 'standard' trick for this? I thought of
> maybe
> 
> #define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
>     RAMBLOCK_FOREACH(block)                            \
>         if (!ram_block_is_migratable(block)) {} else
> 
> ?
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK