[Qemu-devel] [PATCH for-2.10 0/2] mmio-execution and migration

[Qemu-devel] [PATCH for-2.10 0/2] mmio-execution and migration

[KONRAD Frederic]

mmio-execution has incompatibilities with migration so this informs the
potential developer that implementing request_ptr will break migration and
adds a migration blocker in xilinx-spips when using mmio-execution.

This bug affects the user which wants to execute code from SPI (which wasn't
possible before) and uses migration at the same time.

Let's fix that in the next stable release?

Thanks,
Fred

KONRAD Frederic (2):
  mmio-execution: warn the potential developer about migration
  xilinx-spips: add a migration blocker when using mmio_execution

 hw/ssi/xilinx_spips.c | 11 +++++++++++
 include/exec/memory.h |  4 ++++
 2 files changed, 15 insertions(+)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.10 1/2] mmio-execution: warn the potential developer about migration

[KONRAD Frederic]

There are two potential corner cases with mmio-execution:
 * It adds a mmio-interface device which will be migrated.
 * It modifies the RAMBlock list during live migration which odd
   side effects.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 include/exec/memory.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 400dd44..4ded02c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -154,6 +154,10 @@ struct MemoryRegionOps {
      * @offset is the location of the pointer inside @mr.
      *
      * Returns a pointer to a location which contains guest code.
+     *
+     * Warning: This breaks migration if used before or during migration
+     *          because mmio-interface device will be migrated and because
+     *          RAMBlock list _must_ be static during migration.
      */
     void *(*request_ptr)(void *opaque, hwaddr addr, unsigned *size,
                          unsigned *offset);
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[KONRAD Frederic]

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);
+    }
+
     lqspi_load_cache(opaque, offset_within_the_region);
     *size = LQSPI_CACHE_SIZE;
     *offset = offset_within_the_region;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[KONRAD Frederic]

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
> 

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[Edgar E. Iglesias]

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

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[KONRAD Frederic]

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

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[Edgar E. Iglesias]

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

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[KONRAD Frederic]

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