Re: [Qemu-devel] [PATCH v9 6/6] migration: Include migration support for machine check handling

[Greg Kurz]

On Fri, 7 Jun 2019 10:22:40 +1000
David Gibson <address@hidden> wrote:

> On Thu, Jun 06, 2019 at 02:10:48PM +0200, Greg Kurz wrote:
> > On Thu, 6 Jun 2019 16:45:30 +0530
> > Aravinda Prasad <address@hidden> wrote:
> >   
> > > On Thursday 06 June 2019 11:36 AM, Greg Kurz wrote:  
> > > > On Thu, 6 Jun 2019 13:06:14 +1000
> > > > David Gibson <address@hidden> wrote:
> > > >     
> > > >> On Wed, May 29, 2019 at 11:10:57AM +0530, Aravinda Prasad wrote:    
> > > >>> This patch includes migration support for machine check
> > > >>> handling. Especially this patch blocks VM migration
> > > >>> requests until the machine check error handling is
> > > >>> complete as (i) these errors are specific to the source
> > > >>> hardware and is irrelevant on the target hardware,
> > > >>> (ii) these errors cause data corruption and should
> > > >>> be handled before migration.
> > > >>>
> > > >>> Signed-off-by: Aravinda Prasad <address@hidden>
> > > >>> ---
> > > >>>  hw/ppc/spapr.c         |   20 ++++++++++++++++++++
> > > >>>  hw/ppc/spapr_events.c  |   17 +++++++++++++++++
> > > >>>  hw/ppc/spapr_rtas.c    |    4 ++++
> > > >>>  include/hw/ppc/spapr.h |    2 ++
> > > >>>  4 files changed, 43 insertions(+)
> > > >>>
> > > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > >>> index e8a77636..31c4850 100644
> > > >>> --- a/hw/ppc/spapr.c
> > > >>> +++ b/hw/ppc/spapr.c
> > > >>> @@ -2104,6 +2104,25 @@ static const VMStateDescription 
> > > >>> vmstate_spapr_dtb = {
> > > >>>      },
> > > >>>  };
> > > >>>  
> > > >>> +static bool spapr_fwnmi_needed(void *opaque)
> > > >>> +{
> > > >>> +    SpaprMachineState *spapr = (SpaprMachineState *)opaque;
> > > >>> +
> > > >>> +    return (spapr->guest_machine_check_addr == -1) ? 0 : 1;      
> > > >>
> > > >> Since we're introducing a PAPR capability to enable this, it would
> > > >> actually be better to check that here, rather than the runtime state.
> > > >> That leads to less cases and easier to understand semantics for the
> > > >> migration stream.
> > > >>    
> > > > 
> > > > Hmmm... the purpose of needed() VMState callbacks is precisely about
> > > > runtime state: the subsection should only be migrated if an MCE is
> > > > pending, ie. spapr->guest_machine_check_addr != -1.    
> > > 
> > > spapr->guest_machine_check_addr is set when fwnmi is registered. Hence
> > > an MCE might not be pending if it is set.
> > >   
> > 
> > Oops sorry, got confused... should have written "if the guest has
> > registered FWNMI", but the idea is the same. We only need to migrate
> > the subsection if the state is different from reset. This is the way
> > needed() callbacks are generally implemented.  
> 
> Yes, but usually that's because we need to omit the section if it's
> not actively in use in order to maintain backwards compatiblity with
> old migration streams.  If the cap is enabled we already need
> something that implements it at both ends to have a sane migration.
> 

I see it the opposite way. A subsection is something that is optional
for the destination only. In the present case, an older QEMU wont send
the "spapr_machine_check" subsection, which is interpreted by a newer
QEMU as "the guest didn't register FWNMI".

On the source side, if some internal state got used it should always
be migrated. We maintain backwards compatibility to an older QEMU
by not using the new state at all, thanks to the versioned machine
property.

> > > I am fine with both the approaches (checking for
> > > guest_machine_check_addr or for SPAPR_CAP_FWNMI_MCE).
> > >   
> > 
> > I would find ackward to migrate this always for new machine types,
> > even if the guest doesn't register FWNMI...  
> 
> How so?
> 

Well, I just don't see the point in migrating something that
is not state since its value is the reset default that the
destination already knows.

This being said, I won't make more fuss about it, as long as
it works :)

pgpz3FxUS767l.pgp
Description: OpenPGP digital signature