Changeset
hw/ssi/xilinx_spips.c | 41 +++++++++++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 8 deletions(-)
Git apply log
Switched to a new branch '1502367698-4539-1-git-send-email-frederic.konrad@adacore.com'
Applying: xlnx-qspi: add a property for mmio-execution
To https://github.com/patchew-project/qemu
 * [new tag]               patchew/1502367698-4539-1-git-send-email-frederic.konrad@adacore.com -> patchew/1502367698-4539-1-git-send-email-frederic.konrad@adacore.com
Test passed: FreeBSD

loading

Test passed: docker

loading

Test passed: s390x

loading

Test passed: checkpatch

loading

[Qemu-devel] [PATCH for-2.10] xlnx-qspi: add a property for mmio-execution
Posted by KONRAD Frederic, 1 week ago
This adds mmio-exec property to workaround the migration bug.
When enabled the migration is blocked and will return an error.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 hw/ssi/xilinx_spips.c | 41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index e833028..f763bc3 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,8 @@ typedef struct {
 
     uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
     hwaddr lqspi_cached_addr;
+    Error *migration_blocker;
+    bool mmio_execution_enabled;
 } XilinxQSPIPS;
 
 typedef struct XilinxSPIPSClass {
@@ -500,12 +504,13 @@ static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
 {
     XilinxSPIPS *s = &q->parent_obj;
 
-    if (q->lqspi_cached_addr != ~0ULL) {
+    if ((q->mmio_execution_enabled) && (q->lqspi_cached_addr != ~0ULL)) {
         /* Invalidate the current mapped mmio */
         memory_region_invalidate_mmio_ptr(&s->mmlqspi, q->lqspi_cached_addr,
                                           LQSPI_CACHE_SIZE);
-        q->lqspi_cached_addr = ~0ULL;
     }
+
+    q->lqspi_cached_addr = ~0ULL;
 }
 
 static void xilinx_qspips_write(void *opaque, hwaddr addr,
@@ -601,12 +606,17 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size,
                                     unsigned *offset)
 {
     XilinxQSPIPS *q = opaque;
-    hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
-
-    lqspi_load_cache(opaque, offset_within_the_region);
-    *size = LQSPI_CACHE_SIZE;
-    *offset = offset_within_the_region;
-    return q->lqspi_buf;
+    hwaddr offset_within_the_region;
+
+    if (q->mmio_execution_enabled) {
+        offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
+        lqspi_load_cache(opaque, offset_within_the_region);
+        *size = LQSPI_CACHE_SIZE;
+        *offset = offset_within_the_region;
+        return q->lqspi_buf;
+    } else {
+        return NULL;
+    }
 }
 
 static uint64_t
@@ -693,6 +703,15 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->mmlqspi);
 
     q->lqspi_cached_addr = ~0ULL;
+
+    /* mmio_execution breaks migration better aborting than having strange
+     * bugs.
+     */
+    if (q->mmio_execution_enabled) {
+        error_setg(&q->migration_blocker,
+                   "enabling mmio_execution breaks migration");
+        migrate_add_blocker(q->migration_blocker, &error_fatal);
+    }
 }
 
 static int xilinx_spips_post_load(void *opaque, int version_id)
@@ -716,6 +735,11 @@ static const VMStateDescription vmstate_xilinx_spips = {
     }
 };
 
+static Property xilinx_qspips_properties[] = {
+    DEFINE_PROP_BOOL("mmio-exec", XilinxQSPIPS, mmio_execution_enabled, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static Property xilinx_spips_properties[] = {
     DEFINE_PROP_UINT8("num-busses", XilinxSPIPS, num_busses, 1),
     DEFINE_PROP_UINT8("num-ss-bits", XilinxSPIPS, num_cs, 4),
@@ -729,6 +753,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
     XilinxSPIPSClass *xsc = XILINX_SPIPS_CLASS(klass);
 
     dc->realize = xilinx_qspips_realize;
+    dc->props = xilinx_qspips_properties;
     xsc->reg_ops = &qspips_ops;
     xsc->rx_fifo_size = RXFF_A_Q;
     xsc->tx_fifo_size = TXFF_A_Q;
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH for-2.10] xlnx-qspi: add a property for mmio-execution
Posted by Peter Maydell, 1 week ago
On 10 August 2017 at 13:21, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> This adds mmio-exec property to workaround the migration bug.
> When enabled the migration is blocked and will return an error.
>
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  hw/ssi/xilinx_spips.c | 41 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index e833028..f763bc3 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,8 @@ typedef struct {
>
>      uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
>      hwaddr lqspi_cached_addr;
> +    Error *migration_blocker;
> +    bool mmio_execution_enabled;
>  } XilinxQSPIPS;
>
>  typedef struct XilinxSPIPSClass {
> @@ -500,12 +504,13 @@ static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
>  {
>      XilinxSPIPS *s = &q->parent_obj;
>
> -    if (q->lqspi_cached_addr != ~0ULL) {
> +    if ((q->mmio_execution_enabled) && (q->lqspi_cached_addr != ~0ULL)) {
>          /* Invalidate the current mapped mmio */
>          memory_region_invalidate_mmio_ptr(&s->mmlqspi, q->lqspi_cached_addr,
>                                            LQSPI_CACHE_SIZE);
> -        q->lqspi_cached_addr = ~0ULL;
>      }
> +
> +    q->lqspi_cached_addr = ~0ULL;
>  }

This is safe, so it's probably the right thing to do for 2.10, but it
does indicate that there's something weird in the mmio-enabled
code. I was expecting this hunk not to be needed at all (on
the basis that the lqspi_cached_addr should always be ~0ULL
if there's no mapped mmio region), but it looks like calls to
lqspi_read() will call load_cache which will set the
lqspi_cached_addr even though we haven't actually set that up
as an mmio_ptr backed region. Then the next time we call
lqspi_load_cache() we'll call memory_region_invalidate_mmio_ptr()
on a pointer value that we never returned from the
request_mmio_ptr callback. The doc comment on the
memory_region_invalidate_mmio_ptr() function doesn't say that's
permitted.

Looking at the implementation it seems like this will work in
practice (because the invalidate code in memory.c checks that
the MR it's about to drop really is an MMIO_INTERFACE), but
if so we should document this. However, I'm not totally convinced
that it really will work in complicated cases like where you
have device A part of whose memory range is a container which
holds a device B which is also using the mmio_pointer API:
in that case if device A calls invalidate_mmio_ptr with an
address that's in the part of A's memory region that is the
device B container it could end up invalidating an mmio-interface
that's actually inside device B. So it would be safer to say
"the caller may only invalidate pointers it's actually told
the memory system about".

(Conversely if we're convinced that passing bogus pointers
to memory_region_invalidate_mmio_ptr() is fine then we
don't need to add the q->mmio_execution_enabled flag check.)

>  static void xilinx_qspips_write(void *opaque, hwaddr addr,
> @@ -601,12 +606,17 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size,
>                                      unsigned *offset)
>  {
>      XilinxQSPIPS *q = opaque;
> -    hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
> -
> -    lqspi_load_cache(opaque, offset_within_the_region);
> -    *size = LQSPI_CACHE_SIZE;
> -    *offset = offset_within_the_region;
> -    return q->lqspi_buf;
> +    hwaddr offset_within_the_region;
> +
> +    if (q->mmio_execution_enabled) {
> +        offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
> +        lqspi_load_cache(opaque, offset_within_the_region);
> +        *size = LQSPI_CACHE_SIZE;
> +        *offset = offset_within_the_region;
> +        return q->lqspi_buf;
> +    } else {
> +        return NULL;
> +    }

Rather than making the diffstat complicated like this, you can just say
   if (!q->mmio_execution_enabled) {
       return NULL;
   }

>  }
>
>  static uint64_t
> @@ -693,6 +703,15 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(sbd, &s->mmlqspi);
>
>      q->lqspi_cached_addr = ~0ULL;
> +
> +    /* mmio_execution breaks migration better aborting than having strange
> +     * bugs.
> +     */
> +    if (q->mmio_execution_enabled) {
> +        error_setg(&q->migration_blocker,
> +                   "enabling mmio_execution breaks migration");
> +        migrate_add_blocker(q->migration_blocker, &error_fatal);
> +    }
>  }
>
>  static int xilinx_spips_post_load(void *opaque, int version_id)
> @@ -716,6 +735,11 @@ static const VMStateDescription vmstate_xilinx_spips = {
>      }
>  };
>
> +static Property xilinx_qspips_properties[] = {
> +    DEFINE_PROP_BOOL("mmio-exec", XilinxQSPIPS, mmio_execution_enabled, false),

This needs to be an x-property, ie to have a name beginning "x-",
to indicate that it is experimental and will go away when we
fix the underlying bug. It would also be helpful to comment
here with the underlying reason why we've had to turn this off
in 2.10, and what the property does (ie enable execution directly
from the device, at the cost of preventing VM migration).

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static Property xilinx_spips_properties[] = {
>      DEFINE_PROP_UINT8("num-busses", XilinxSPIPS, num_busses, 1),
>      DEFINE_PROP_UINT8("num-ss-bits", XilinxSPIPS, num_cs, 4),
> @@ -729,6 +753,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>      XilinxSPIPSClass *xsc = XILINX_SPIPS_CLASS(klass);
>
>      dc->realize = xilinx_qspips_realize;
> +    dc->props = xilinx_qspips_properties;
>      xsc->reg_ops = &qspips_ops;
>      xsc->rx_fifo_size = RXFF_A_Q;
>      xsc->tx_fifo_size = TXFF_A_Q;
> --
> 1.8.3.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.10] xlnx-qspi: add a property for mmio-execution
Posted by KONRAD Frederic, 1 week ago

On 08/10/2017 02:43 PM, Peter Maydell wrote:
> On 10 August 2017 at 13:21, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> This adds mmio-exec property to workaround the migration bug.
>> When enabled the migration is blocked and will return an error.
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>> ---
>>   hw/ssi/xilinx_spips.c | 41 +++++++++++++++++++++++++++++++++--------
>>   1 file changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>> index e833028..f763bc3 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,8 @@ typedef struct {
>>
>>       uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
>>       hwaddr lqspi_cached_addr;
>> +    Error *migration_blocker;
>> +    bool mmio_execution_enabled;
>>   } XilinxQSPIPS;
>>
>>   typedef struct XilinxSPIPSClass {
>> @@ -500,12 +504,13 @@ static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
>>   {
>>       XilinxSPIPS *s = &q->parent_obj;
>>
>> -    if (q->lqspi_cached_addr != ~0ULL) {
>> +    if ((q->mmio_execution_enabled) && (q->lqspi_cached_addr != ~0ULL)) {
>>           /* Invalidate the current mapped mmio */
>>           memory_region_invalidate_mmio_ptr(&s->mmlqspi, q->lqspi_cached_addr,
>>                                             LQSPI_CACHE_SIZE);
>> -        q->lqspi_cached_addr = ~0ULL;
>>       }
>> +
>> +    q->lqspi_cached_addr = ~0ULL;
>>   }
> 
> This is safe, so it's probably the right thing to do for 2.10, but it
> does indicate that there's something weird in the mmio-enabled
> code. I was expecting this hunk not to be needed at all (on
> the basis that the lqspi_cached_addr should always be ~0ULL
> if there's no mapped mmio region), but it looks like calls to
> lqspi_read() will call load_cache which will set the
> lqspi_cached_addr even though we haven't actually set that up
> as an mmio_ptr backed region. Then the next time we call
> lqspi_load_cache() we'll call memory_region_invalidate_mmio_ptr()
> on a pointer value that we never returned from the
> request_mmio_ptr callback. The doc comment on the
> memory_region_invalidate_mmio_ptr() function doesn't say that's
> permitted.

Yes you're right: The device load a cache even if mmio-execution
isn't enabled this was the behavior before mmio-execution.

> 
> Looking at the implementation it seems like this will work in
> practice (because the invalidate code in memory.c checks that
> the MR it's about to drop really is an MMIO_INTERFACE), but
> if so we should document this. However, I'm not totally convinced
> that it really will work in complicated cases like where you
> have device A part of whose memory range is a container which
> holds a device B which is also using the mmio_pointer API:
> in that case if device A calls invalidate_mmio_ptr with an
> address that's in the part of A's memory region that is the
> device B container it could end up invalidating an mmio-interface
> that's actually inside device B. So it would be safer to say
> "the caller may only invalidate pointers it's actually told
> the memory system about".
> 

I see what you mean but I'm not sure this will happen because the
mmio-interface is mapped on @mr which is passed to invalidate.

So if device A doesn't have any mmio-interface mapped it can't
find the device B mmio-interface as we pass device A
MemoryRegion.

But I agree the doc is a little confusing about that.

> (Conversely if we're convinced that passing bogus pointers
> to memory_region_invalidate_mmio_ptr() is fine then we
> don't need to add the q->mmio_execution_enabled flag check.)

True but this will trigger an async_safe_work with all the
overhead.

> 
>>   static void xilinx_qspips_write(void *opaque, hwaddr addr,
>> @@ -601,12 +606,17 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size,
>>                                       unsigned *offset)
>>   {
>>       XilinxQSPIPS *q = opaque;
>> -    hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
>> -
>> -    lqspi_load_cache(opaque, offset_within_the_region);
>> -    *size = LQSPI_CACHE_SIZE;
>> -    *offset = offset_within_the_region;
>> -    return q->lqspi_buf;
>> +    hwaddr offset_within_the_region;
>> +
>> +    if (q->mmio_execution_enabled) {
>> +        offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
>> +        lqspi_load_cache(opaque, offset_within_the_region);
>> +        *size = LQSPI_CACHE_SIZE;
>> +        *offset = offset_within_the_region;
>> +        return q->lqspi_buf;
>> +    } else {
>> +        return NULL;
>> +    }
> 
> Rather than making the diffstat complicated like this, you can just say
>     if (!q->mmio_execution_enabled) {
>         return NULL;
>     }

Ok

> 
>>   }
>>
>>   static uint64_t
>> @@ -693,6 +703,15 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
>>       sysbus_init_mmio(sbd, &s->mmlqspi);
>>
>>       q->lqspi_cached_addr = ~0ULL;
>> +
>> +    /* mmio_execution breaks migration better aborting than having strange
>> +     * bugs.
>> +     */
>> +    if (q->mmio_execution_enabled) {
>> +        error_setg(&q->migration_blocker,
>> +                   "enabling mmio_execution breaks migration");
>> +        migrate_add_blocker(q->migration_blocker, &error_fatal);
>> +    }
>>   }
>>
>>   static int xilinx_spips_post_load(void *opaque, int version_id)
>> @@ -716,6 +735,11 @@ static const VMStateDescription vmstate_xilinx_spips = {
>>       }
>>   };
>>
>> +static Property xilinx_qspips_properties[] = {
>> +    DEFINE_PROP_BOOL("mmio-exec", XilinxQSPIPS, mmio_execution_enabled, false),
> 
> This needs to be an x-property, ie to have a name beginning "x-",
> to indicate that it is experimental and will go away when we
> fix the underlying bug. It would also be helpful to comment
> here with the underlying reason why we've had to turn this off
> in 2.10, and what the property does (ie enable execution directly
> from the device, at the cost of preventing VM migration).

Ok will do.

Thanks,
Fred

> 
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>   static Property xilinx_spips_properties[] = {
>>       DEFINE_PROP_UINT8("num-busses", XilinxSPIPS, num_busses, 1),
>>       DEFINE_PROP_UINT8("num-ss-bits", XilinxSPIPS, num_cs, 4),
>> @@ -729,6 +753,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>>       XilinxSPIPSClass *xsc = XILINX_SPIPS_CLASS(klass);
>>
>>       dc->realize = xilinx_qspips_realize;
>> +    dc->props = xilinx_qspips_properties;
>>       xsc->reg_ops = &qspips_ops;
>>       xsc->rx_fifo_size = RXFF_A_Q;
>>       xsc->tx_fifo_size = TXFF_A_Q;
>> --
>> 1.8.3.1
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH for-2.10] xlnx-qspi: add a property for mmio-execution
Posted by Peter Maydell, 1 week ago
On 10 August 2017 at 14:08, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>
>
> On 08/10/2017 02:43 PM, Peter Maydell wrote:
>>
>> On 10 August 2017 at 13:21, KONRAD Frederic <frederic.konrad@adacore.com>
>> wrote:
>>>

>> Looking at the implementation it seems like this will work in
>> practice (because the invalidate code in memory.c checks that
>> the MR it's about to drop really is an MMIO_INTERFACE), but
>> if so we should document this. However, I'm not totally convinced
>> that it really will work in complicated cases like where you
>> have device A part of whose memory range is a container which
>> holds a device B which is also using the mmio_pointer API:
>> in that case if device A calls invalidate_mmio_ptr with an
>> address that's in the part of A's memory region that is the
>> device B container it could end up invalidating an mmio-interface
>> that's actually inside device B. So it would be safer to say
>> "the caller may only invalidate pointers it's actually told
>> the memory system about".
>>
>
> I see what you mean but I'm not sure this will happen because the
> mmio-interface is mapped on @mr which is passed to invalidate.
>
> So if device A doesn't have any mmio-interface mapped it can't
> find the device B mmio-interface as we pass device A
> MemoryRegion.

I think what you're saying is (1) you can't create an mmio-ptr
for a container region (2) when you call invalidate_mmio_ptr
you have to pass exactly the MR that has the memory-region-ops
that the have the request_ptr set. This isn't entirely clear
from the docs, but OTOH it seems reasonable and I agree that
in that case you can't get the situation I suggested above.

> But I agree the doc is a little confusing about that.
>
>> (Conversely if we're convinced that passing bogus pointers
>> to memory_region_invalidate_mmio_ptr() is fine then we
>> don't need to add the q->mmio_execution_enabled flag check.)
>
>
> True but this will trigger an async_safe_work with all the
> overhead.

If we care about that then we should have a "have we used the
cached region as an mmio_ptr" flag separately anyway, so that
we don't trigger async_safe_work for the case of "not executing
from the memory, but we had to call lqspi_load_cache() because
the guest did a read from some address that was outside the
cached section".

thanks
-- PMM