[Qemu-devel] [PATCH 0/3] Drop old_mmio accessor support

[Qemu-devel] [PATCH 0/3] Drop old_mmio accessor support

[Peter Maydell]

Hi; this patchset removes support for the old_mmio accessors
from the memory core code, now that we have fixed all the
devices that were using it.

Patch 1 does that removal.
Patch 2 fixes up a wart in fw_cfg that was only needed while
we had the old_mmio accessors (spotted because of the comment
coming up in my grep for 'old_mmio'...)
Patch 3 is a bonus that adds documentation for the _with_attrs
accessors, since I noticed while writing patch 1 that we
hadn't actually written any.

Based-on: <20180802174042.29234-1-peter.maydell@linaro.org>
("[PATCH 0/2] hw/net/pcnet-pci: Convert away from old_mmio accessors")

The pcnet-pci patches are the only "convert away from old_mmio"
patches still not yet in master; they've been reviewed, they just
haven't been picked up by anybody yet.

thanks
-- PMM

Peter Maydell (3):
  memory: Remove old_mmio accessors
  hw/nvram/fw_cfg: Use memberwise copy of MemoryRegionOps struct
  docs/devel/memory.txt: Document _with_attrs accessors

 docs/devel/memory.txt | 13 ++++++---
 include/exec/memory.h |  5 ----
 hw/nvram/fw_cfg.c     |  7 +----
 memory.c              | 64 ++-----------------------------------------
 4 files changed, 12 insertions(+), 77 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/3] memory: Remove old_mmio accessors

[Peter Maydell]

Now that all the users of old_mmio MemoryRegion accessors
have been converted, we can remove the core code support.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/memory.txt |  2 --
 include/exec/memory.h |  5 ----
 memory.c              | 64 ++-----------------------------------------
 3 files changed, 2 insertions(+), 69 deletions(-)

diff --git a/docs/devel/memory.txt b/docs/devel/memory.txt
index c1dee1252c2..4fff0d56031 100644
--- a/docs/devel/memory.txt
+++ b/docs/devel/memory.txt
@@ -342,5 +342,3 @@ various constraints can be supplied to control how these callbacks are called:
  - .impl.unaligned specifies that the *implementation* supports unaligned
    accesses; if false, unaligned accesses will be emulated by two aligned
    accesses.
- - .old_mmio eases the porting of code that was formerly using
-   cpu_register_io_memory(). It should not be used in new code.
diff --git a/include/exec/memory.h b/include/exec/memory.h
index eb4f2fb249c..f43a0d7e36a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -201,11 +201,6 @@ struct MemoryRegionOps {
          */
         bool unaligned;
     } impl;
-
-    /* If .read and .write are not present, old_mmio may be used for
-     * backwards compatibility with old mmio registration
-     */
-    const MemoryRegionMmio old_mmio;
 };
 
 enum IOMMUMemoryRegionAttr {
diff --git a/memory.c b/memory.c
index 9b73892768d..fa915a88c08 100644
--- a/memory.c
+++ b/memory.c
@@ -396,32 +396,6 @@ static int get_cpu_index(void)
     return -1;
 }
 
-static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
-                                                       hwaddr addr,
-                                                       uint64_t *value,
-                                                       unsigned size,
-                                                       unsigned shift,
-                                                       uint64_t mask,
-                                                       MemTxAttrs attrs)
-{
-    uint64_t tmp;
-
-    tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
-    if (mr->subpage) {
-        trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
-    } else if (mr == &io_mem_notdirty) {
-        /* Accesses to code which has previously been translated into a TB show
-         * up in the MMIO path, as accesses to the io_mem_notdirty
-         * MemoryRegion. */
-        trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size);
-    } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
-        hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size);
-    }
-    *value |= (tmp & mask) << shift;
-    return MEMTX_OK;
-}
-
 static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
                                                 hwaddr addr,
                                                 uint64_t *value,
@@ -475,32 +449,6 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr,
     return r;
 }
 
-static MemTxResult memory_region_oldmmio_write_accessor(MemoryRegion *mr,
-                                                        hwaddr addr,
-                                                        uint64_t *value,
-                                                        unsigned size,
-                                                        unsigned shift,
-                                                        uint64_t mask,
-                                                        MemTxAttrs attrs)
-{
-    uint64_t tmp;
-
-    tmp = (*value >> shift) & mask;
-    if (mr->subpage) {
-        trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size);
-    } else if (mr == &io_mem_notdirty) {
-        /* Accesses to code which has previously been translated into a TB show
-         * up in the MMIO path, as accesses to the io_mem_notdirty
-         * MemoryRegion. */
-        trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size);
-    } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
-        hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
-        trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size);
-    }
-    mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, tmp);
-    return MEMTX_OK;
-}
-
 static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
                                                 hwaddr addr,
                                                 uint64_t *value,
@@ -1394,16 +1342,12 @@ static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
                                          mr->ops->impl.max_access_size,
                                          memory_region_read_accessor,
                                          mr, attrs);
-    } else if (mr->ops->read_with_attrs) {
+    } else {
         return access_with_adjusted_size(addr, pval, size,
                                          mr->ops->impl.min_access_size,
                                          mr->ops->impl.max_access_size,
                                          memory_region_read_with_attrs_accessor,
                                          mr, attrs);
-    } else {
-        return access_with_adjusted_size(addr, pval, size, 1, 4,
-                                         memory_region_oldmmio_read_accessor,
-                                         mr, attrs);
     }
 }
 
@@ -1475,17 +1419,13 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
                                          mr->ops->impl.max_access_size,
                                          memory_region_write_accessor, mr,
                                          attrs);
-    } else if (mr->ops->write_with_attrs) {
+    } else {
         return
             access_with_adjusted_size(addr, &data, size,
                                       mr->ops->impl.min_access_size,
                                       mr->ops->impl.max_access_size,
                                       memory_region_write_with_attrs_accessor,
                                       mr, attrs);
-    } else {
-        return access_with_adjusted_size(addr, &data, size, 1, 4,
-                                         memory_region_oldmmio_write_accessor,
-                                         mr, attrs);
     }
 }
 
-- 
2.18.0

[Qemu-devel] [PATCH 2/3] hw/nvram/fw_cfg: Use memberwise copy of MemoryRegionOps struct

[Peter Maydell]

We've now removed the 'old_mmio' member from MemoryRegionOps,
so we can perform the copy as a simple struct copy rather
than having to do it via a memberwise copy.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/nvram/fw_cfg.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d79a568f540..b3c9b066239 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1109,12 +1109,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->ctl_iomem);
 
     if (s->data_width > data_ops->valid.max_access_size) {
-        /* memberwise copy because the "old_mmio" member is const */
-        s->wide_data_ops.read       = data_ops->read;
-        s->wide_data_ops.write      = data_ops->write;
-        s->wide_data_ops.endianness = data_ops->endianness;
-        s->wide_data_ops.valid      = data_ops->valid;
-        s->wide_data_ops.impl       = data_ops->impl;
+        s->wide_data_ops = *data_ops;
 
         s->wide_data_ops.valid.max_access_size = s->data_width;
         s->wide_data_ops.impl.max_access_size  = s->data_width;
-- 
2.18.0

[Qemu-devel] [PATCH 3/3] docs/devel/memory.txt: Document _with_attrs accessors

[Peter Maydell]

When we added the _with_attrs accessors we forgot to mention
them in the documentation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/memory.txt | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/docs/devel/memory.txt b/docs/devel/memory.txt
index 4fff0d56031..42577e1d860 100644
--- a/docs/devel/memory.txt
+++ b/docs/devel/memory.txt
@@ -326,8 +326,15 @@ visible as the pci-hole alias clips it to a 0.5GB range.
 MMIO Operations
 ---------------
 
-MMIO regions are provided with ->read() and ->write() callbacks; in addition
-various constraints can be supplied to control how these callbacks are called:
+MMIO regions are provided with ->read() and ->write() callbacks,
+which are sufficient for most devices. Some devices change behaviour
+based on the attributes used for the memory transaction, or need
+to be able to respond that the access should provoke a bus error
+rather than completing successfully; those devices can use the
+->read_with_attrs() and ->write_with_attrs() callbacks instead.
+
+In addition various constraints can be supplied to control how these
+callbacks are called:
 
  - .valid.min_access_size, .valid.max_access_size define the access sizes
    (in bytes) which the device accepts; accesses outside this range will
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/3] memory: Remove old_mmio accessors

[Richard Henderson]

On 08/24/2018 10:04 AM, Peter Maydell wrote:
> Now that all the users of old_mmio MemoryRegion accessors
> have been converted, we can remove the core code support.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  docs/devel/memory.txt |  2 --
>  include/exec/memory.h |  5 ----
>  memory.c              | 64 ++-----------------------------------------
>  3 files changed, 2 insertions(+), 69 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/3] hw/nvram/fw_cfg: Use memberwise copy of MemoryRegionOps struct

[Richard Henderson]

On 08/24/2018 10:04 AM, Peter Maydell wrote:
> We've now removed the 'old_mmio' member from MemoryRegionOps,
> so we can perform the copy as a simple struct copy rather
> than having to do it via a memberwise copy.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/nvram/fw_cfg.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~