[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [RFC for-2.12 4/8] spapr: Validate capabilities on migrat
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [RFC for-2.12 4/8] spapr: Validate capabilities on migration |
Date: |
Tue, 12 Dec 2017 11:31:12 +1100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Mon, Dec 11, 2017 at 06:05:39PM -0600, Michael Roth wrote:
> Quoting David Gibson (2017-12-11 01:08:04)
> > Now that the "pseries" machine type implements optional capabilities (well,
> > one so far) there's the possibility of having different capabilities
> > available at either end of a migration. Although arguably a user error,
> > it would be nice to catch this situation and fail as gracefully as we can.
> >
> > This adds code to migrate the capabilities flags. These aren't pulled
> > directly into the destination's configuration since what the user has
> > specified on the destination command line should take precedence. However,
> > they are checked against the destination capabilities.
> >
> > If the source was using a capability which is absent on the destination,
> > we fail the migration, since that could easily cause a guest crash or other
> > bad behaviour. If the source lacked a capability which is present on the
> > destination we warn, but allow the migration to proceed.
> >
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> > hw/ppc/spapr.c | 6 ++++
> > hw/ppc/spapr_caps.c | 94
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > include/hw/ppc/spapr.h | 6 ++++
> > 3 files changed, 103 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0487d67894..4e4cf35587 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1587,6 +1587,11 @@ static int spapr_post_load(void *opaque, int
> > version_id)
> > sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > int err = 0;
> >
> > + err = spapr_caps_post_migration(spapr);
> > + if (err) {
> > + return err;
> > + }
> > +
> > if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> > CPUState *cs;
> > CPU_FOREACH(cs) {
> > @@ -1753,6 +1758,7 @@ static const VMStateDescription vmstate_spapr = {
> > &vmstate_spapr_ov5_cas,
> > &vmstate_spapr_patb_entry,
> > &vmstate_spapr_pending_events,
> > + &vmstate_spapr_caps,
> > NULL
> > }
> > };
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 5afb4d9d5f..6bba04d60e 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -22,6 +22,7 @@
> > * THE SOFTWARE.
> > */
> > #include "qemu/osdep.h"
> > +#include "qemu/error-report.h"
> > #include "qapi/error.h"
> > #include "qapi/visitor.h"
> > #include "sysemu/hw_accel.h"
> > @@ -61,6 +62,91 @@ static sPAPRCapabilities
> > default_caps_with_cpu(sPAPRMachineState *spapr, CPUStat
> > return caps;
> > }
> >
> > +static bool spapr_caps_needed(void *opaque)
> > +{
> > + sPAPRMachineState *spapr = opaque;
> > +
> > + return (spapr->forced_caps.mask != 0) || (spapr->forbidden_caps.mask
> > != 0);
> > +}
> > +
> > +/* This has to be called from the top-level spapr post_load, not the
> > + * caps specific one. Otherwise it wouldn't be called when the source
> > + * caps are all defaults, which could still conflict with overridden
> > + * caps on the destination */
> > +int spapr_caps_post_migration(sPAPRMachineState *spapr)
> > +{
> > + uint64_t allcaps = 0;
> > + int i;
> > + bool ok = true;
> > + sPAPRCapabilities dstcaps = spapr->effective_caps;
> > + sPAPRCapabilities srccaps;
> > +
> > + srccaps = default_caps_with_cpu(spapr, first_cpu);
> > + srccaps.mask |= spapr->mig_forced_caps.mask;
> > + srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> > +
> > + for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > + sPAPRCapabilityInfo *info = &capability_table[i];
> > +
> > + allcaps |= info->bit;
> > +
> > + if ((srccaps.mask & info->bit) && !(dstcaps.mask & info->bit)) {
> > + error_report("cap-%s=on in incoming stream, but off in
> > destination",
> > + info->name);
> > + ok = false;
> > + }
> > +
> > + if (!(srccaps.mask & info->bit) && (dstcaps.mask & info->bit)) {
> > + error_report("Warning: cap-%s=off in incoming stream, but on
> > in destination",
> > + info->name);
> > + }
> > + }
> > +
> > + if (spapr->mig_forced_caps.mask & ~allcaps) {
> > + error_report("Unknown capabilities 0x%"PRIx64" enabled in incoming
> > stream",
> > + spapr->forced_caps.mask & ~allcaps);
>
> Shouldn't these be spapr->mig_forced_caps.mask,
>
> > + ok = false;
> > + }
> > + if (spapr->mig_forbidden_caps.mask & ~allcaps) {
> > + error_report("Warning: Unknown capabilities 0x%"PRIx64" disabled
> > in incoming stream",
> > + spapr->forbidden_caps.mask & ~allcaps);
>
> and spapr->mig_forbidden_caps.mask?
Yes, they should. Nice catch.
>
> Looks good otherwise.
>
> > + }
> > +
> > + return ok ? 0 : -EINVAL;
> > +}
> > +
> > +static int spapr_caps_pre_save(void *opaque)
> > +{
> > + sPAPRMachineState *spapr = opaque;
> > +
> > + spapr->mig_forced_caps = spapr->forced_caps;
> > + spapr->mig_forbidden_caps = spapr->forbidden_caps;
> > + return 0;
> > +}
> > +
> > +static int spapr_caps_pre_load(void *opaque)
> > +{
> > + sPAPRMachineState *spapr = opaque;
> > +
> > + spapr->mig_forced_caps = spapr_caps(0);
> > + spapr->mig_forbidden_caps = spapr_caps(0);
> > + return 0;
> > +}
> > +
> > +const VMStateDescription vmstate_spapr_caps = {
> > + .name = "spapr/caps",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .needed = spapr_caps_needed,
> > + .pre_save = spapr_caps_pre_save,
> > + .pre_load = spapr_caps_pre_load,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_UINT64(mig_forced_caps.mask, sPAPRMachineState),
> > + VMSTATE_UINT64(mig_forbidden_caps.mask, sPAPRMachineState),
> > + VMSTATE_END_OF_LIST()
> > + },
> > +};
> > +
> > static void spapr_allow_caps(sPAPRMachineState *spapr, Error **errp)
> > {
> > Error *err = NULL;
> > @@ -98,6 +184,11 @@ void spapr_caps_reset(sPAPRMachineState *spapr)
> >
> > caps = default_caps_with_cpu(spapr, first_cpu);
> >
> > + /* Remove unnecessary forced/forbidden bits (this will help us
> > + * with migration) */
> > + spapr->forced_caps.mask &= ~caps.mask;
> > + spapr->forbidden_caps.mask &= caps.mask;
> > +
> > caps.mask |= spapr->forced_caps.mask;
> > caps.mask &= ~spapr->forbidden_caps.mask;
> >
> > @@ -168,9 +259,6 @@ void spapr_validate_caps(sPAPRMachineState *spapr,
> > Error **errp)
> > error_setg(&local_err, "Some sPAPR capabilities set both on and
> > off");
> > return;
> > }
> > -
> > - /* Check for any caps incompatible with other caps. Nothing to do
> > - * yet */
> > }
> >
> > void spapr_capability_properties(sPAPRMachineClass *smc, Error **errp)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 4215b113f4..9b80fe72ed 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -54,6 +54,8 @@ typedef enum {
> > * Capabilities
> > */
> >
> > +/* These bits go in the migration stream, so they can't be reassigned */
> > +
> > /* Hardware Transactional Memory */
> > #define SPAPR_CAP_HTM 0x0000000000000001ULL
> >
> > @@ -142,6 +144,7 @@ struct sPAPRMachineState {
> > const char *icp_type;
> >
> > sPAPRCapabilities forced_caps, forbidden_caps;
> > + sPAPRCapabilities mig_forced_caps, mig_forbidden_caps;
> > sPAPRCapabilities effective_caps;
> > };
> >
> > @@ -743,6 +746,8 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> > /*
> > * Handling of optional capabilities
> > */
> > +extern const VMStateDescription vmstate_spapr_caps;
> > +
> > static inline sPAPRCapabilities spapr_caps(uint64_t mask)
> > {
> > sPAPRCapabilities caps = { mask };
> > @@ -757,5 +762,6 @@ static inline bool spapr_has_cap(sPAPRMachineState
> > *spapr, uint64_t cap)
> > void spapr_caps_reset(sPAPRMachineState *spapr);
> > void spapr_validate_caps(sPAPRMachineState *spapr, Error **errp);
> > void spapr_capability_properties(sPAPRMachineClass *smc, Error **errp);
> > +int spapr_caps_post_migration(sPAPRMachineState *spapr);
> >
> > #endif /* HW_SPAPR_H */
>
--
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
signature.asc
Description: PGP signature
- Re: [Qemu-ppc] [RFC for-2.12 7/8] spapr: Handle VMX/VSX presence as an spapr capability flag, (continued)
[Qemu-ppc] [RFC for-2.12 6/8] target/ppc: Clean up probing of VMX, VSX and DFP availability on KVM, David Gibson, 2017/12/11
[Qemu-ppc] [RFC for-2.12 4/8] spapr: Validate capabilities on migration, David Gibson, 2017/12/11
[Qemu-ppc] [RFC for-2.12 8/8] spapr: Handle Decimal Floating Point (DFP) as an optional capability, David Gibson, 2017/12/11
Re: [Qemu-ppc] [RFC for-2.12 0/8] spapr: Add optional capabilities, Laurent Vivier, 2017/12/15