Re: [Xen-devel] [PATCH] libxl: Disallow save or migrate when host devices are assigned to a guest.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Apr 14, 2015 at 08:15:52PM -0600, Jim Fehlig wrote:
> Konrad Rzeszutek Wilk wrote:
> > It is unhealthy. If the device is not doing any DMA operations
> > it would work - but if you are saving and there are DMA operations
> > happening the chance of corruption (outstanding DMAs) increase.
> >
> > As such re-use the check migration used.
> >   
> 
> s/used// ?

<nods>
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > ---
> >  src/libxl/libxl_domain.c    | 19 +++++++++++++++++++
> >  src/libxl/libxl_domain.h    |  3 +++
> >  src/libxl/libxl_driver.c    |  6 ++++++
> >  src/libxl/libxl_migration.c | 15 +--------------
> >  4 files changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > index 5e0ab56..c038989 100644
> > --- a/src/libxl/libxl_domain.c
> > +++ b/src/libxl/libxl_domain.c
> > @@ -178,6 +178,25 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver 
> > ATTRIBUTE_UNUSED,
> >      return virObjectUnref(obj);
> >  }
> >  
> > +/*
> > + * Checks whether the domain has host devices (PCIe) to disallow
> > + * migration or saving of guest - unless they are detached.
> > + *
> > + * Returns true if the domain has host devices, otherwise false.
> > + */
> > +bool
> > +libxlDomainHasHostDevices(virDomainDefPtr def)
> > +{
> > +    /* Migration is not allowed if definition contains any hostdevs */
> > +    if (def->nhostdevs > 0) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > +                       _("domain has assigned host devices"));
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static void *
> >  libxlDomainObjPrivateAlloc(void)
> >  {
> > diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> > index aa647b8..76c0c8d 100644
> > --- a/src/libxl/libxl_domain.h
> > +++ b/src/libxl/libxl_domain.h
> > @@ -95,6 +95,9 @@ char *
> >  libxlDomainManagedSavePath(libxlDriverPrivatePtr driver,
> >                             virDomainObjPtr vm);
> >  
> > +bool
> > +libxlDomainHasHostDevices(virDomainDefPtr def);
> > +
> >  int
> >  libxlDomainSaveImageOpen(libxlDriverPrivatePtr driver,
> >                           libxlDriverConfigPtr cfg,
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index 9eb071e..b9faba8 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> > @@ -1650,6 +1650,9 @@ libxlDomainSaveFlags(virDomainPtr dom, const char 
> > *to, const char *dxml,
> >          goto endjob;
> >      }
> >  
> > +    if (libxlDomainHasHostDevices(vm->def))
> > +        goto endjob;
> > +
> >      if (libxlDoDomainSave(driver, vm, to) < 0)
> >          goto endjob;
> >  
> > @@ -1876,6 +1879,9 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int 
> > flags)
> >          goto endjob;
> >      }
> >  
> > +    if (libxlDomainHasHostDevices(vm->def))
> > +        goto endjob;
> > +
> >      name = libxlDomainManagedSavePath(driver, vm);
> >      if (name == NULL)
> >          goto endjob;
> > diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> > index 4010506..aaed448c 100644
> > --- a/src/libxl/libxl_migration.c
> > +++ b/src/libxl/libxl_migration.c
> > @@ -209,19 +209,6 @@ libxlDoMigrateSend(libxlDriverPrivatePtr driver,
> >      return ret;
> >  }
> >  
> > -static bool
> > -libxlDomainMigrationIsAllowed(virDomainDefPtr def)
> > -{
> > -    /* Migration is not allowed if definition contains any hostdevs */
> > -    if (def->nhostdevs > 0) {
> > -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > -                       _("domain has assigned host devices"));
> > -        return false;
> > -    }
> > -
> > -    return true;
> > -}
> > -
> 
> This function was created with the intention of adding more checks, e.g.
> is the cpu and numa configuration migratable?  Are there pending
> snapshot or block jobs (once those are supported)? ...
> 
> I'd like to keep libxlDomainMigrationIsAllowed even though it currently
> only checks for assigned hostdevs.  I think it improves readability of
> the migration code.  For the time it could simply call
> libxlDomainHasHostDevices.

You got it.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel