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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]

On Thu, Jun 06, 2019 at 02:10:48PM +0200, Greg Kurz wrote:
> On Thu, 6 Jun 2019 16:45:30 +0530
> Aravinda Prasad <aravinda@linux.vnet.ibm.com> wrote:
> 
> > On Thursday 06 June 2019 11:36 AM, Greg Kurz wrote:
> > > On Thu, 6 Jun 2019 13:06:14 +1000
> > > David Gibson <david@gibson.dropbear.id.au> 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 <aravinda@linux.vnet.ibm.com>
> > >>> ---
> > >>>  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 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?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]