| Message ID | 1501575048-13485-3-git-send-email-frederic.konrad@adacore.com |
|---|---|
| State | New |
| Headers | show |
On 1 August 2017 at 09:10, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > This adds a migration blocker when mmio_execution has been used. > > Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> > --- > hw/ssi/xilinx_spips.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c > index e833028..d46491f 100644 > --- a/hw/ssi/xilinx_spips.c > +++ b/hw/ssi/xilinx_spips.c > @@ -31,6 +31,8 @@ > #include "hw/ssi/ssi.h" > #include "qemu/bitops.h" > #include "hw/ssi/xilinx_spips.h" > +#include "qapi/error.h" > +#include "migration/blocker.h" > > #ifndef XILINX_SPIPS_ERR_DEBUG > #define XILINX_SPIPS_ERR_DEBUG 0 > @@ -139,6 +141,7 @@ typedef struct { > > uint8_t lqspi_buf[LQSPI_CACHE_SIZE]; > hwaddr lqspi_cached_addr; > + Error *migration_blocker; > } XilinxQSPIPS; > > typedef struct XilinxSPIPSClass { > @@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, > XilinxQSPIPS *q = opaque; > hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); > > + /* mmio_execution breaks migration better aborting than having strange > + * bugs. > + */ > + if (!q->migration_blocker) { > + error_setg(&q->migration_blocker, "booting from SPI breaks migration"); > + migrate_add_blocker(q->migration_blocker, &error_fatal); > + } > + This doesn't handle the case when migration is already in progress and this function is called (which will cause migrate_add_blocker to fail). thanks -- PMM
On 08/01/2017 11:00 AM, Peter Maydell wrote: > On 1 August 2017 at 09:10, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >> This adds a migration blocker when mmio_execution has been used. >> >> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> >> --- >> hw/ssi/xilinx_spips.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c >> index e833028..d46491f 100644 >> --- a/hw/ssi/xilinx_spips.c >> +++ b/hw/ssi/xilinx_spips.c >> @@ -31,6 +31,8 @@ >> #include "hw/ssi/ssi.h" >> #include "qemu/bitops.h" >> #include "hw/ssi/xilinx_spips.h" >> +#include "qapi/error.h" >> +#include "migration/blocker.h" >> >> #ifndef XILINX_SPIPS_ERR_DEBUG >> #define XILINX_SPIPS_ERR_DEBUG 0 >> @@ -139,6 +141,7 @@ typedef struct { >> >> uint8_t lqspi_buf[LQSPI_CACHE_SIZE]; >> hwaddr lqspi_cached_addr; >> + Error *migration_blocker; >> } XilinxQSPIPS; >> >> typedef struct XilinxSPIPSClass { >> @@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, >> XilinxQSPIPS *q = opaque; >> hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); >> >> + /* mmio_execution breaks migration better aborting than having strange >> + * bugs. >> + */ >> + if (!q->migration_blocker) { >> + error_setg(&q->migration_blocker, "booting from SPI breaks migration"); >> + migrate_add_blocker(q->migration_blocker, &error_fatal); >> + } >> + > > This doesn't handle the case when migration is already in progress > and this function is called (which will cause migrate_add_blocker > to fail). Maybe I can make the request_ptr to fail if migration is in progress.. But is that safe or do I risk a race. Fred > > thanks > -- PMM >
On Tue, Aug 01, 2017 at 11:13:56AM +0200, KONRAD Frederic wrote: > > > On 08/01/2017 11:00 AM, Peter Maydell wrote: > >On 1 August 2017 at 09:10, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > >>This adds a migration blocker when mmio_execution has been used. > >> > >>Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> > >>--- > >> hw/ssi/xilinx_spips.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >>diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c > >>index e833028..d46491f 100644 > >>--- a/hw/ssi/xilinx_spips.c > >>+++ b/hw/ssi/xilinx_spips.c > >>@@ -31,6 +31,8 @@ > >> #include "hw/ssi/ssi.h" > >> #include "qemu/bitops.h" > >> #include "hw/ssi/xilinx_spips.h" > >>+#include "qapi/error.h" > >>+#include "migration/blocker.h" > >> > >> #ifndef XILINX_SPIPS_ERR_DEBUG > >> #define XILINX_SPIPS_ERR_DEBUG 0 > >>@@ -139,6 +141,7 @@ typedef struct { > >> > >> uint8_t lqspi_buf[LQSPI_CACHE_SIZE]; > >> hwaddr lqspi_cached_addr; > >>+ Error *migration_blocker; > >> } XilinxQSPIPS; > >> > >> typedef struct XilinxSPIPSClass { > >>@@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, > >> XilinxQSPIPS *q = opaque; > >> hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); > >> > >>+ /* mmio_execution breaks migration better aborting than having strange > >>+ * bugs. > >>+ */ > >>+ if (!q->migration_blocker) { > >>+ error_setg(&q->migration_blocker, "booting from SPI breaks migration"); > >>+ migrate_add_blocker(q->migration_blocker, &error_fatal); > >>+ } > >>+ > > > >This doesn't handle the case when migration is already in progress > >and this function is called (which will cause migrate_add_blocker > >to fail). > > Maybe I can make the request_ptr to fail if migration is in > progress.. But is that safe or do I risk a race. > Hi Fred, At this stage, perhaps we should just register the blocker when this dev realizes. If a request_ptr comes in during migration, the VM will fail either way... Cheers, Edgar
On 08/01/2017 11:30 AM, Edgar E. Iglesias wrote: > On Tue, Aug 01, 2017 at 11:13:56AM +0200, KONRAD Frederic wrote: >> >> >> On 08/01/2017 11:00 AM, Peter Maydell wrote: >>> On 1 August 2017 at 09:10, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >>>> This adds a migration blocker when mmio_execution has been used. >>>> >>>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> >>>> --- >>>> hw/ssi/xilinx_spips.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c >>>> index e833028..d46491f 100644 >>>> --- a/hw/ssi/xilinx_spips.c >>>> +++ b/hw/ssi/xilinx_spips.c >>>> @@ -31,6 +31,8 @@ >>>> #include "hw/ssi/ssi.h" >>>> #include "qemu/bitops.h" >>>> #include "hw/ssi/xilinx_spips.h" >>>> +#include "qapi/error.h" >>>> +#include "migration/blocker.h" >>>> >>>> #ifndef XILINX_SPIPS_ERR_DEBUG >>>> #define XILINX_SPIPS_ERR_DEBUG 0 >>>> @@ -139,6 +141,7 @@ typedef struct { >>>> >>>> uint8_t lqspi_buf[LQSPI_CACHE_SIZE]; >>>> hwaddr lqspi_cached_addr; >>>> + Error *migration_blocker; >>>> } XilinxQSPIPS; >>>> >>>> typedef struct XilinxSPIPSClass { >>>> @@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, >>>> XilinxQSPIPS *q = opaque; >>>> hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); >>>> >>>> + /* mmio_execution breaks migration better aborting than having strange >>>> + * bugs. >>>> + */ >>>> + if (!q->migration_blocker) { >>>> + error_setg(&q->migration_blocker, "booting from SPI breaks migration"); >>>> + migrate_add_blocker(q->migration_blocker, &error_fatal); >>>> + } >>>> + >>> >>> This doesn't handle the case when migration is already in progress >>> and this function is called (which will cause migrate_add_blocker >>> to fail). >> >> Maybe I can make the request_ptr to fail if migration is in >> progress.. But is that safe or do I risk a race. >> > > Hi Fred, > > At this stage, perhaps we should just register the blocker when this dev realizes. > > If a request_ptr comes in during migration, the VM will fail either way... > > Cheers, > Edgar > Hi Edgar, Yes but this will breaks migration for the spips device everytime and not only when mmio-execution is used? Thanks, Fred
On 1 August 2017 at 10:35, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > > > On 08/01/2017 11:30 AM, Edgar E. Iglesias wrote: >> At this stage, perhaps we should just register the blocker when this dev >> realizes. >> >> If a request_ptr comes in during migration, the VM will fail either way... > Yes but this will breaks migration for the spips device everytime > and not only when mmio-execution is used? This line of thought is why I ended up suggesting just disabling the exec-in-place feature -- that way we just don't introduce what would be a new-in-2.10 feature, rather than breaking something that used to work in 2.9. thanks -- PMM
On 1 August 2017 at 10:41, Peter Maydell <peter.maydell@linaro.org> wrote: > On 1 August 2017 at 10:35, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >> >> >> On 08/01/2017 11:30 AM, Edgar E. Iglesias wrote: >>> At this stage, perhaps we should just register the blocker when this dev >>> realizes. >>> >>> If a request_ptr comes in during migration, the VM will fail either way... > >> Yes but this will breaks migration for the spips device everytime >> and not only when mmio-execution is used? > > This line of thought is why I ended up suggesting just disabling > the exec-in-place feature -- that way we just don't introduce > what would be a new-in-2.10 feature, rather than breaking something > that used to work in 2.9. OK, so what's the plan here? We have several options: * just disable exec-from-spips for 2.10 (I sent a patch for that) * disable exec-from-spips for 2.10 but with a device x-property to allow the user to turn it on again if they really want it * this patch or variants on it which try to only disable migration if exec-from-spips is actually used by the guest (I don't like these because of the awkward corner cases if migration and the guest using exec-from-spips happen at the same time) So my current view remains "we should just disable this feature for 2.10 and we can implement it properly with handling of migration for 2.11", unless somebody cares enough to implement the x-property thing within the next day or so. thanks -- PMM
On Thu, Aug 10, 2017 at 10:11:13AM +0100, Peter Maydell wrote: > On 1 August 2017 at 10:41, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 1 August 2017 at 10:35, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > >> > >> > >> On 08/01/2017 11:30 AM, Edgar E. Iglesias wrote: > >>> At this stage, perhaps we should just register the blocker when this dev > >>> realizes. > >>> > >>> If a request_ptr comes in during migration, the VM will fail either way... > > > >> Yes but this will breaks migration for the spips device everytime > >> and not only when mmio-execution is used? > > > > This line of thought is why I ended up suggesting just disabling > > the exec-in-place feature -- that way we just don't introduce > > what would be a new-in-2.10 feature, rather than breaking something > > that used to work in 2.9. > > OK, so what's the plan here? We have several options: > * just disable exec-from-spips for 2.10 (I sent a patch for that) > * disable exec-from-spips for 2.10 but with a device x-property > to allow the user to turn it on again if they really want it > * this patch or variants on it which try to only disable > migration if exec-from-spips is actually used by the guest > (I don't like these because of the awkward corner cases if > migration and the guest using exec-from-spips happen at the > same time) > > So my current view remains "we should just disable this feature > for 2.10 and we can implement it properly with handling of > migration for 2.11", unless somebody cares enough to implement > the x-property thing within the next day or so. Hi Peter, I think the x-property sounds good. Fred, would you like to send a patch for that? Otherwise, I can do it later today. Cheers, Edgar
On 08/10/2017 11:22 AM, Edgar E. Iglesias wrote: > On Thu, Aug 10, 2017 at 10:11:13AM +0100, Peter Maydell wrote: >> On 1 August 2017 at 10:41, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 1 August 2017 at 10:35, KONRAD Frederic <frederic.konrad@adacore.com> wrote: >>>> >>>> >>>> On 08/01/2017 11:30 AM, Edgar E. Iglesias wrote: >>>>> At this stage, perhaps we should just register the blocker when this dev >>>>> realizes. >>>>> >>>>> If a request_ptr comes in during migration, the VM will fail either way... >>> >>>> Yes but this will breaks migration for the spips device everytime >>>> and not only when mmio-execution is used? >>> >>> This line of thought is why I ended up suggesting just disabling >>> the exec-in-place feature -- that way we just don't introduce >>> what would be a new-in-2.10 feature, rather than breaking something >>> that used to work in 2.9. >> >> OK, so what's the plan here? We have several options: >> * just disable exec-from-spips for 2.10 (I sent a patch for that) >> * disable exec-from-spips for 2.10 but with a device x-property >> to allow the user to turn it on again if they really want it >> * this patch or variants on it which try to only disable >> migration if exec-from-spips is actually used by the guest >> (I don't like these because of the awkward corner cases if >> migration and the guest using exec-from-spips happen at the >> same time) >> >> So my current view remains "we should just disable this feature >> for 2.10 and we can implement it properly with handling of >> migration for 2.11", unless somebody cares enough to implement >> the x-property thing within the next day or so. > > Hi Peter, > > I think the x-property sounds good. > Fred, would you like to send a patch for that? > Otherwise, I can do it later today. > > Cheers, > Edgar > Hi, Ok I will do. Thanks, Fred
diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c index e833028..d46491f 100644 --- a/hw/ssi/xilinx_spips.c +++ b/hw/ssi/xilinx_spips.c @@ -31,6 +31,8 @@ #include "hw/ssi/ssi.h" #include "qemu/bitops.h" #include "hw/ssi/xilinx_spips.h" +#include "qapi/error.h" +#include "migration/blocker.h" #ifndef XILINX_SPIPS_ERR_DEBUG #define XILINX_SPIPS_ERR_DEBUG 0 @@ -139,6 +141,7 @@ typedef struct { uint8_t lqspi_buf[LQSPI_CACHE_SIZE]; hwaddr lqspi_cached_addr; + Error *migration_blocker; } XilinxQSPIPS; typedef struct XilinxSPIPSClass { @@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size, XilinxQSPIPS *q = opaque; hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1); + /* mmio_execution breaks migration better aborting than having strange + * bugs. + */ + if (!q->migration_blocker) { + error_setg(&q->migration_blocker, "booting from SPI breaks migration"); + migrate_add_blocker(q->migration_blocker, &error_fatal); + } + lqspi_load_cache(opaque, offset_within_the_region); *size = LQSPI_CACHE_SIZE; *offset = offset_within_the_region;
This adds a migration blocker when mmio_execution has been used. Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> --- hw/ssi/xilinx_spips.c | 11 +++++++++++ 1 file changed, 11 insertions(+)