[PATCH v2 0/4] target/arm: Support for Data Cache Clean up to PoP

[PATCH v2 0/4] target/arm: Support for Data Cache Clean up to PoP

[Beata Michalska]

ARMv8.2 introduced support for Data Cache Clean instructions to PoP
(point-of-persistence) and PoDP (point-of-deep-persistence):
ARMv8.2-DCCVAP &  ARMv8.2-DCCVADP respectively.
This patch set adds support for emulating both, though there is no
distinction between the two points: the PoDP is assumed to represent
the same point of persistence as PoP. Case there is no such point specified
for the considered memory system both will fall back to the DV CVAC inst
(clean up to the point of coherency).
The changes introduced include adding probe_read for validating read memory
access to allow verification for mandatory read access for both cache
clean instructions, along with support for writeback for requested memory
regions through msync, if one is available, based otherwise on fsyncdata.

As currently the virt platform is missing support for NVDIMM,
the changes have been tested  with [1] & [2]


[1] https://patchwork.kernel.org/cover/10830237/
[2] https://patchwork.kernel.org/project/qemu-devel/list/?series=159441

v2:
    - Moved the msync into a qemu wrapper with
      CONFIG_POSIX switch + additional comments
    - Fixed length alignment
    - Dropped treating the DC CVAP/CVADP as special case
      and moved those to conditional registration
    - Dropped needless locking for grabbing mem region


Beata Michalska (4):
  tcg: cputlb: Add probe_read
  Memory: Enable writeback for given memory region
  migration: ram: Switch to ram block writeback
  target/arm: Add support for DC CVAP & DC CVADP ins

 exec.c                  | 43 +++++++++++++++++++++++++++++++++++++
 include/exec/exec-all.h |  6 ++++++
 include/exec/memory.h   |  6 ++++++
 include/exec/ram_addr.h |  8 +++++++
 include/qemu/cutils.h   |  1 +
 linux-user/elfload.c    |  2 ++
 memory.c                | 12 +++++++++++
 migration/ram.c         |  5 +----
 target/arm/cpu.h        | 10 +++++++++
 target/arm/cpu64.c      |  1 +
 target/arm/helper.c     | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
 util/cutils.c           | 47 +++++++++++++++++++++++++++++++++++++++++
 12 files changed, 193 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] tcg: cputlb: Add probe_read

[Beata Michalska]

Add probe_read alongside the write probing equivalent.

Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/exec-all.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index d85e610..350c4b4 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -339,6 +339,12 @@ static inline void *probe_write(CPUArchState *env, target_ulong addr, int size,
     return probe_access(env, addr, size, MMU_DATA_STORE, mmu_idx, retaddr);
 }
 
+static inline void *probe_read(CPUArchState *env, target_ulong addr, int size,
+                               int mmu_idx, uintptr_t retaddr)
+{
+    return probe_access(env, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
+}
+
 #define CODE_GEN_ALIGN           16 /* must be >= of the size of a icache line */
 
 /* Estimated block size for TB allocation.  */
-- 
2.7.4

[PATCH v2 2/4] Memory: Enable writeback for given memory region

[Beata Michalska]

Add an option to trigger memory writeback to sync given memory region
with the corresponding backing store, case one is available.
This extends the support for persistent memory, allowing syncing on-demand.

Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
---
 exec.c                  | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/exec/memory.h   |  6 ++++++
 include/exec/ram_addr.h |  8 ++++++++
 include/qemu/cutils.h   |  1 +
 memory.c                | 12 ++++++++++++
 util/cutils.c           | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 117 insertions(+)

diff --git a/exec.c b/exec.c
index ffdb518..e1f06de 100644
--- a/exec.c
+++ b/exec.c
@@ -65,6 +65,8 @@
 #include "exec/ram_addr.h"
 #include "exec/log.h"
 
+#include "qemu/pmem.h"
+
 #include "migration/vmstate.h"
 
 #include "qemu/range.h"
@@ -2156,6 +2158,47 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
     return 0;
 }
 
+/*
+ * Trigger sync on the given ram block for range [start, start + length]
+ * with the backing store if one is available.
+ * Otherwise no-op.
+ * @Note: this is supposed to be a synchronous op.
+ */
+void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
+{
+    void *addr = ramblock_ptr(block, start);
+
+    /*
+     * The requested range might spread up to the very end of the block
+     */
+    if ((start + length) > block->used_length) {
+        qemu_log("%s: sync range outside the block boundaries: "
+                     "start: " RAM_ADDR_FMT " length: " RAM_ADDR_FMT
+                     " block length: " RAM_ADDR_FMT " Narrowing down ..." ,
+                     __func__, start, length, block->used_length);
+        length = block->used_length - start;
+    }
+
+#ifdef CONFIG_LIBPMEM
+    /* The lack of support for pmem should not block the sync */
+    if (ramblock_is_pmem(block)) {
+        pmem_persist(addr, length);
+    } else
+#endif
+    if (block->fd >= 0) {
+        /**
+         * Case there is no support for PMEM or the memory has not been
+         * specified as persistent (or is not one) - use the msync.
+         * Less optimal but still achieves the same goal
+         */
+        if (qemu_msync(addr, length, qemu_host_page_size, block->fd)) {
+            warn_report("%s: failed to sync memory range: start: "
+                    RAM_ADDR_FMT " length: " RAM_ADDR_FMT,
+                    __func__, start, length);
+        }
+    }
+}
+
 /* Called with ram_list.mutex held */
 static void dirty_memory_extend(ram_addr_t old_ram_size,
                                 ram_addr_t new_ram_size)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e499dc2..27a84e0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1265,6 +1265,12 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
  */
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize,
                               Error **errp);
+/**
+ * memory_region_do_writeback: Trigger writeback for selected address range
+ * [addr, addr + size]
+ *
+ */
+void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size);
 
 /**
  * memory_region_set_log: Turn dirty logging on or off for a region.
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index bed0554..5adebb0 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -174,6 +174,14 @@ void qemu_ram_free(RAMBlock *block);
 
 int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
 
+void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length);
+
+/* Clear whole block of mem */
+static inline void qemu_ram_block_writeback(RAMBlock *block)
+{
+    qemu_ram_writeback(block, 0, block->used_length);
+}
+
 #define DIRTY_CLIENTS_ALL     ((1 << DIRTY_MEMORY_NUM) - 1)
 #define DIRTY_CLIENTS_NOCODE  (DIRTY_CLIENTS_ALL & ~(1 << DIRTY_MEMORY_CODE))
 
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index b54c847..41c5fa9 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -130,6 +130,7 @@ const char *qemu_strchrnul(const char *s, int c);
 #endif
 time_t mktimegm(struct tm *tm);
 int qemu_fdatasync(int fd);
+int qemu_msync(void *addr, size_t length, size_t alignment, int fd);
 int fcntl_setfl(int fd, int flag);
 int qemu_parse_fd(const char *param);
 int qemu_strtoi(const char *nptr, const char **endptr, int base,
diff --git a/memory.c b/memory.c
index c952eab..15734a0 100644
--- a/memory.c
+++ b/memory.c
@@ -2214,6 +2214,18 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
     qemu_ram_resize(mr->ram_block, newsize, errp);
 }
 
+
+void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size)
+{
+    /*
+     * Might be extended case needed to cover
+     * different types of memory regions
+     */
+    if (mr->ram_block && mr->dirty_log_mask) {
+        qemu_ram_writeback(mr->ram_block, addr, size);
+    }
+}
+
 /*
  * Call proper memory listeners about the change on the newly
  * added/removed CoalescedMemoryRange.
diff --git a/util/cutils.c b/util/cutils.c
index fd591ca..7a50dbc 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -164,6 +164,53 @@ int qemu_fdatasync(int fd)
 #endif
 }
 
+/**
+ * Sync changes made to the memory mapped file back to the backing
+ * storage. For POSIX compliant systems this will simply fallback
+ * to regular msync call (thus the required alignment). Otherwise
+ * it will trigger whole file sync (including the metadata case
+ * there is no support to skip that otherwise)
+ *
+ * @addr   - start of the memory area to be synced
+ * @length - length of the are to be synced
+ * @align  - alignment (expected to be PAGE_SIZE)
+ * @fd     - file descriptor for the file to be synced
+ *           (mandatory only for POSIX non-compliant systems)
+ */
+int qemu_msync(void *addr, size_t length, size_t align, int fd)
+{
+#ifdef CONFIG_POSIX
+    size_t align_mask;
+
+    /* Bare minimum of sanity checks on the alignment */
+    /* The start address needs to be a multiple of PAGE_SIZE */
+    align = MAX(align, qemu_real_host_page_size);
+    align_mask = ~(qemu_real_host_page_size - 1);
+    align = (align + ~align_mask) & align_mask;
+
+    align_mask = ~(align - 1);
+    /**
+     * There are no strict reqs as per the length of mapping
+     * to be synced. Still the length needs to follow the address
+     * alignment changes. Additionally - round the size to the multiple
+     * of requested alignment (expected as PAGE_SIZE)
+     */
+    length += ((uintptr_t)addr & (align - 1));
+    length = (length + ~align_mask) & align_mask;
+
+    addr = (void *)((uintptr_t)addr & align_mask);
+
+    return msync(addr, length, MS_SYNC);
+#else /* CONFIG_POSIX */
+    /**
+     * Perform the sync based on the file descriptor
+     * The sync range will most probably be wider than the one
+     * requested - but it will still get the job done
+     */
+    return qemu_fdatasync(fd);
+#endif /* CONFIG_POSIX */
+}
+
 #ifndef _WIN32
 /* Sets a specific flag */
 int fcntl_setfl(int fd, int flag)
-- 
2.7.4

Re: [PATCH v2 2/4] Memory: Enable writeback for given memory region

[Richard Henderson]

On 11/6/19 12:40 AM, Beata Michalska wrote:
> +void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
> +{
> +    void *addr = ramblock_ptr(block, start);
> +
> +    /*
> +     * The requested range might spread up to the very end of the block
> +     */
> +    if ((start + length) > block->used_length) {
> +        qemu_log("%s: sync range outside the block boundaries: "
> +                     "start: " RAM_ADDR_FMT " length: " RAM_ADDR_FMT
> +                     " block length: " RAM_ADDR_FMT " Narrowing down ..." ,
> +                     __func__, start, length, block->used_length);
> +        length = block->used_length - start;
> +    }

qemu_log_mask w/ GUEST_ERROR?  How do we expect the length to overflow?

> +#ifdef CONFIG_LIBPMEM
> +    /* The lack of support for pmem should not block the sync */
> +    if (ramblock_is_pmem(block)) {
> +        pmem_persist(addr, length);
> +    } else
> +#endif

Perhaps better to return out of that if block than have the dangling else.

> +/**
> + * Sync changes made to the memory mapped file back to the backing
> + * storage. For POSIX compliant systems this will simply fallback
> + * to regular msync call (thus the required alignment). Otherwise
> + * it will trigger whole file sync (including the metadata case
> + * there is no support to skip that otherwise)
> + *
> + * @addr   - start of the memory area to be synced
> + * @length - length of the are to be synced
> + * @align  - alignment (expected to be PAGE_SIZE)
> + * @fd     - file descriptor for the file to be synced
> + *           (mandatory only for POSIX non-compliant systems)
> + */
> +int qemu_msync(void *addr, size_t length, size_t align, int fd)
> +{
> +#ifdef CONFIG_POSIX
> +    size_t align_mask;
> +
> +    /* Bare minimum of sanity checks on the alignment */
> +    /* The start address needs to be a multiple of PAGE_SIZE */
> +    align = MAX(align, qemu_real_host_page_size);
> +    align_mask = ~(qemu_real_host_page_size - 1);
> +    align = (align + ~align_mask) & align_mask;
> +
> +    align_mask = ~(align - 1);

I don't understand what you're trying to do with align.

You pass in qemu_host_page_size from the one caller, and then adjust it for
qemu_real_host_page_size?

Why pass in anything at all, and just use qemu_real_host_page_mask?

> +    /**
> +     * There are no strict reqs as per the length of mapping
> +     * to be synced. Still the length needs to follow the address
> +     * alignment changes. Additionally - round the size to the multiple
> +     * of requested alignment (expected as PAGE_SIZE)
> +     */
> +    length += ((uintptr_t)addr & (align - 1));
> +    length = (length + ~align_mask) & align_mask;
> +
> +    addr = (void *)((uintptr_t)addr & align_mask);
> +
> +    return msync(addr, length, MS_SYNC);
> +#else /* CONFIG_POSIX */
> +    /**
> +     * Perform the sync based on the file descriptor
> +     * The sync range will most probably be wider than the one
> +     * requested - but it will still get the job done
> +     */
> +    return qemu_fdatasync(fd);
> +#endif /* CONFIG_POSIX */
> +}


r~

Re: [PATCH v2 2/4] Memory: Enable writeback for given memory region

[Beata Michalska]

On Wed, 6 Nov 2019 at 12:20, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/6/19 12:40 AM, Beata Michalska wrote:
> > +void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
> > +{
> > +    void *addr = ramblock_ptr(block, start);
> > +
> > +    /*
> > +     * The requested range might spread up to the very end of the block
> > +     */
> > +    if ((start + length) > block->used_length) {
> > +        qemu_log("%s: sync range outside the block boundaries: "
> > +                     "start: " RAM_ADDR_FMT " length: " RAM_ADDR_FMT
> > +                     " block length: " RAM_ADDR_FMT " Narrowing down ..." ,
> > +                     __func__, start, length, block->used_length);
> > +        length = block->used_length - start;
> > +    }
>
> qemu_log_mask w/ GUEST_ERROR?  How do we expect the length to overflow?

In theory it shouldn't, at least with current usage.
I guess the probe_access will make sure of that.
This was more of a precaution to enable catching potential/future misuses
aka debugging purpose. I can get rid of that it that's playing too safe.

>
> > +#ifdef CONFIG_LIBPMEM
> > +    /* The lack of support for pmem should not block the sync */
> > +    if (ramblock_is_pmem(block)) {
> > +        pmem_persist(addr, length);
> > +    } else
> > +#endif
>
> Perhaps better to return out of that if block than have the dangling else.

Good idea
>
> > +/**
> > + * Sync changes made to the memory mapped file back to the backing
> > + * storage. For POSIX compliant systems this will simply fallback
> > + * to regular msync call (thus the required alignment). Otherwise
> > + * it will trigger whole file sync (including the metadata case
> > + * there is no support to skip that otherwise)
> > + *
> > + * @addr   - start of the memory area to be synced
> > + * @length - length of the are to be synced
> > + * @align  - alignment (expected to be PAGE_SIZE)
> > + * @fd     - file descriptor for the file to be synced
> > + *           (mandatory only for POSIX non-compliant systems)
> > + */
> > +int qemu_msync(void *addr, size_t length, size_t align, int fd)
> > +{
> > +#ifdef CONFIG_POSIX
> > +    size_t align_mask;
> > +
> > +    /* Bare minimum of sanity checks on the alignment */
> > +    /* The start address needs to be a multiple of PAGE_SIZE */
> > +    align = MAX(align, qemu_real_host_page_size);
> > +    align_mask = ~(qemu_real_host_page_size - 1);
> > +    align = (align + ~align_mask) & align_mask;
> > +
> > +    align_mask = ~(align - 1);
>
> I don't understand what you're trying to do with align.
>
> You pass in qemu_host_page_size from the one caller, and then adjust it for
> qemu_real_host_page_size?
>
> Why pass in anything at all, and just use qemu_real_host_page_mask?

The qemu_msync was supposed to be generic and not tied to current use case
without any assumptions on the alignment and whether that would  be an actual
host page size. So that was just to make sure it will be a multiple of that.
I can get rid of that with assumption all will be using the same alignment.

BR
Beata
>
> > +    /**
> > +     * There are no strict reqs as per the length of mapping
> > +     * to be synced. Still the length needs to follow the address
> > +     * alignment changes. Additionally - round the size to the multiple
> > +     * of requested alignment (expected as PAGE_SIZE)
> > +     */
> > +    length += ((uintptr_t)addr & (align - 1));
> > +    length = (length + ~align_mask) & align_mask;
> > +
> > +    addr = (void *)((uintptr_t)addr & align_mask);
> > +
> > +    return msync(addr, length, MS_SYNC);
> > +#else /* CONFIG_POSIX */
> > +    /**
> > +     * Perform the sync based on the file descriptor
> > +     * The sync range will most probably be wider than the one
> > +     * requested - but it will still get the job done
> > +     */
> > +    return qemu_fdatasync(fd);
> > +#endif /* CONFIG_POSIX */
> > +}
>
>
> r~
>

Re: [PATCH v2 2/4] Memory: Enable writeback for given memory region

[Alex Bennée]

Beata Michalska <beata.michalska@linaro.org> writes:

> On Wed, 6 Nov 2019 at 12:20, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 11/6/19 12:40 AM, Beata Michalska wrote:
>> > +void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
>> > +{
>> > +    void *addr = ramblock_ptr(block, start);
>> > +
>> > +    /*
>> > +     * The requested range might spread up to the very end of the block
>> > +     */
>> > +    if ((start + length) > block->used_length) {
>> > +        qemu_log("%s: sync range outside the block boundaries: "
>> > +                     "start: " RAM_ADDR_FMT " length: " RAM_ADDR_FMT
>> > +                     " block length: " RAM_ADDR_FMT " Narrowing down ..." ,
>> > +                     __func__, start, length, block->used_length);
>> > +        length = block->used_length - start;
>> > +    }
>>
>> qemu_log_mask w/ GUEST_ERROR?  How do we expect the length to overflow?
>
> In theory it shouldn't, at least with current usage.
> I guess the probe_access will make sure of that.
> This was more of a precaution to enable catching potential/future misuses
> aka debugging purpose. I can get rid of that it that's playing too
> safe.

If the internal code might get it wrong and that would be a bug then the
g_assert(), if the values are ultimately from the guest then log with
GUEST_ERROR as Richard suggests.

<snip>

--
Alex Bennée

Re: [PATCH v2 2/4] Memory: Enable writeback for given memory region

[Peter Maydell]

On Thu, 7 Nov 2019 at 16:57, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Beata Michalska <beata.michalska@linaro.org> writes:
>
> > On Wed, 6 Nov 2019 at 12:20, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >> qemu_log_mask w/ GUEST_ERROR?  How do we expect the length to overflow?
> >
> > In theory it shouldn't, at least with current usage.
> > I guess the probe_access will make sure of that.
> > This was more of a precaution to enable catching potential/future misuses
> > aka debugging purpose. I can get rid of that it that's playing too
> > safe.
>
> If the internal code might get it wrong and that would be a bug then the
> g_assert(), if the values are ultimately from the guest then log with
> GUEST_ERROR as Richard suggests.

...or consider asserting at this level and making the target
specific calling code sanitize and do the GUEST_ERROR logging
(it can do a better job of it because it knows what the
target-specific operation that the guest just got wrong was).

thanks
-- PMM

[PATCH v2 3/4] migration: ram: Switch to ram block writeback

[Beata Michalska]

Switch to ram block writeback for pmem migration.

Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5078f94..38070f1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -33,7 +33,6 @@
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
 #include "qemu/main-loop.h"
-#include "qemu/pmem.h"
 #include "xbzrle.h"
 #include "ram.h"
 #include "migration.h"
@@ -3981,9 +3980,7 @@ static int ram_load_cleanup(void *opaque)
     RAMBlock *rb;
 
     RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
-        if (ramblock_is_pmem(rb)) {
-            pmem_persist(rb->host, rb->used_length);
-        }
+        qemu_ram_block_writeback(rb);
     }
 
     xbzrle_load_cleanup();
-- 
2.7.4

Re: [PATCH v2 3/4] migration: ram: Switch to ram block writeback

[Alex Bennée]

Beata Michalska <beata.michalska@linaro.org> writes:

> Switch to ram block writeback for pmem migration.
>
> Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  migration/ram.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 5078f94..38070f1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -33,7 +33,6 @@
>  #include "qemu/bitops.h"
>  #include "qemu/bitmap.h"
>  #include "qemu/main-loop.h"
> -#include "qemu/pmem.h"
>  #include "xbzrle.h"
>  #include "ram.h"
>  #include "migration.h"
> @@ -3981,9 +3980,7 @@ static int ram_load_cleanup(void *opaque)
>      RAMBlock *rb;
>
>      RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
> -        if (ramblock_is_pmem(rb)) {
> -            pmem_persist(rb->host, rb->used_length);
> -        }
> +        qemu_ram_block_writeback(rb);
>      }
>
>      xbzrle_load_cleanup();


--
Alex Bennée

[PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins

[Beata Michalska]

ARMv8.2 introduced support for Data Cache Clean instructions
to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
- DV CVADP. Both specify conceptual points in a memory system where all writes
that are to reach them are considered persistent.
The support provided considers both to be actually the same so there is no
distinction between the two. If none is available (there is no backing store
for given memory) both will result in Data Cache Clean up to the point of
coherency. Otherwise sync for the specified range shall be performed.

Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
---
 linux-user/elfload.c |  2 ++
 target/arm/cpu.h     | 10 ++++++++++
 target/arm/cpu64.c   |  1 +
 target/arm/helper.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f6693e5..07b16cc 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -656,6 +656,7 @@ static uint32_t get_elf_hwcap(void)
     GET_FEATURE_ID(aa64_jscvt, ARM_HWCAP_A64_JSCVT);
     GET_FEATURE_ID(aa64_sb, ARM_HWCAP_A64_SB);
     GET_FEATURE_ID(aa64_condm_4, ARM_HWCAP_A64_FLAGM);
+    GET_FEATURE_ID(aa64_dcpop, ARM_HWCAP_A64_DCPOP);
 
     return hwcaps;
 }
@@ -665,6 +666,7 @@ static uint32_t get_elf_hwcap2(void)
     ARMCPU *cpu = ARM_CPU(thread_cpu);
     uint32_t hwcaps = 0;
 
+    GET_FEATURE_ID(aa64_dcpodp, ARM_HWCAP2_A64_DCPODP);
     GET_FEATURE_ID(aa64_condm_5, ARM_HWCAP2_A64_FLAGM2);
     GET_FEATURE_ID(aa64_frint, ARM_HWCAP2_A64_FRINT);
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e1a66a2..0dc22c6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3617,6 +3617,16 @@ static inline bool isar_feature_aa64_frint(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, FRINTTS) != 0;
 }
 
+static inline bool isar_feature_aa64_dcpop(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, DPB) != 0;
+}
+
+static inline bool isar_feature_aa64_dcpodp(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, DPB) >= 2;
+}
+
 static inline bool isar_feature_aa64_fp16(const ARMISARegisters *id)
 {
     /* We always set the AdvSIMD and FP fields identically wrt FP16.  */
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 68baf04..e6a033e 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -661,6 +661,7 @@ static void aarch64_max_initfn(Object *obj)
         cpu->isar.id_aa64isar0 = t;
 
         t = cpu->isar.id_aa64isar1;
+        t = FIELD_DP64(t, ID_AA64ISAR1, DPB, 2);
         t = FIELD_DP64(t, ID_AA64ISAR1, JSCVT, 1);
         t = FIELD_DP64(t, ID_AA64ISAR1, FCMA, 1);
         t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); /* PAuth, architected only */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index be67e2c..00c72e4 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5924,6 +5924,52 @@ static const ARMCPRegInfo rndr_reginfo[] = {
       .access = PL0_R, .readfn = rndr_readfn },
     REGINFO_SENTINEL
 };
+
+#ifndef CONFIG_USER_ONLY
+static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
+                          uint64_t value)
+{
+    ARMCPU *cpu = env_archcpu(env);
+    /* CTR_EL0 System register -> DminLine, bits [19:16] */
+    uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
+    uint64_t vaddr_in = (uint64_t) value;
+    uint64_t vaddr = vaddr_in & ~(dline_size - 1);
+    void *haddr;
+    int mem_idx = cpu_mmu_index(env, false);
+
+    /* This won't be crossing page boundaries */
+    haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC());
+    if (haddr) {
+
+        ram_addr_t offset;
+        MemoryRegion *mr;
+
+        /* RCU lock is already being held */
+        mr = memory_region_from_host(haddr, &offset);
+
+        if (mr) {
+            memory_region_do_writeback(mr, offset, dline_size);
+        }
+    }
+}
+
+static const ARMCPRegInfo dcpop_reg[] = {
+    { .name = "DC_CVAP", .state = ARM_CP_STATE_AA64,
+      .opc0 = 1, .opc1 = 3, .crn = 7, .crm = 12, .opc2 = 1,
+      .access = PL0_W, .type = ARM_CP_NO_RAW | ARM_CP_SUPPRESS_TB_END,
+      .accessfn = aa64_cacheop_access, .writefn = dccvap_writefn },
+    REGINFO_SENTINEL
+};
+
+static const ARMCPRegInfo dcpodp_reg[] = {
+    { .name = "DC_CVADP", .state = ARM_CP_STATE_AA64,
+      .opc0 = 1, .opc1 = 3, .crn = 7, .crm = 13, .opc2 = 1,
+      .access = PL0_W, .type = ARM_CP_NO_RAW | ARM_CP_SUPPRESS_TB_END,
+      .accessfn = aa64_cacheop_access, .writefn = dccvap_writefn },
+    REGINFO_SENTINEL
+};
+#endif /*CONFIG_USER_ONLY*/
+
 #endif
 
 static CPAccessResult access_predinv(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -6884,6 +6930,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
     if (cpu_isar_feature(aa64_rndr, cpu)) {
         define_arm_cp_regs(cpu, rndr_reginfo);
     }
+#ifndef CONFIG_USER_ONLY
+    /* Data Cache clean instructions up to PoP */
+    if (cpu_isar_feature(aa64_dcpop, cpu)) {
+        define_one_arm_cp_reg(cpu, dcpop_reg);
+
+        if (cpu_isar_feature(aa64_dcpodp, cpu)) {
+            define_one_arm_cp_reg(cpu, dcpodp_reg);
+        }
+    }
+#endif /*CONFIG_USER_ONLY*/
 #endif
 
     /*
-- 
2.7.4

Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins

[Richard Henderson]

On 11/6/19 12:41 AM, Beata Michalska wrote:
> ARMv8.2 introduced support for Data Cache Clean instructions
> to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
> - DV CVADP. Both specify conceptual points in a memory system where all writes
> that are to reach them are considered persistent.
> The support provided considers both to be actually the same so there is no
> distinction between the two. If none is available (there is no backing store
> for given memory) both will result in Data Cache Clean up to the point of
> coherency. Otherwise sync for the specified range shall be performed.
> 
> Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> ---
>  linux-user/elfload.c |  2 ++
>  target/arm/cpu.h     | 10 ++++++++++
>  target/arm/cpu64.c   |  1 +
>  target/arm/helper.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 69 insertions(+)

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


r~

Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins

[Philippe Mathieu-Daudé]

Hi,

On 6/11/19 00:41, Beata Michalska wrote:
> ARMv8.2 introduced support for Data Cache Clean instructions
> to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
> - DV CVADP. Both specify conceptual points in a memory system where all writes
> that are to reach them are considered persistent.
> The support provided considers both to be actually the same so there is no
> distinction between the two. If none is available (there is no backing store
> for given memory) both will result in Data Cache Clean up to the point of
> coherency. Otherwise sync for the specified range shall be performed.
> 
> Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> ---
>   linux-user/elfload.c |  2 ++
>   target/arm/cpu.h     | 10 ++++++++++
>   target/arm/cpu64.c   |  1 +
>   target/arm/helper.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 69 insertions(+)


> +#ifndef CONFIG_USER_ONLY
> +static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
> +                          uint64_t value)
> +{
> +    ARMCPU *cpu = env_archcpu(env);
> +    /* CTR_EL0 System register -> DminLine, bits [19:16] */
> +    uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
> +    uint64_t vaddr_in = (uint64_t) value;
> +    uint64_t vaddr = vaddr_in & ~(dline_size - 1);
> +    void *haddr;
> +    int mem_idx = cpu_mmu_index(env, false);
> +
> +    /* This won't be crossing page boundaries */
> +    haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC());
> +    if (haddr) {
> +
> +        ram_addr_t offset;
> +        MemoryRegion *mr;
> +
> +        /* RCU lock is already being held */
> +        mr = memory_region_from_host(haddr, &offset);
> +
> +        if (mr) {
> +            memory_region_do_writeback(mr, offset, dline_size);
> +        }
> +    }
> +}


> +#ifndef CONFIG_USER_ONLY
> +    /* Data Cache clean instructions up to PoP */
> +    if (cpu_isar_feature(aa64_dcpop, cpu)) {

Am I correct understanding this is a TCG-only feature?


> +        define_one_arm_cp_reg(cpu, dcpop_reg);
> +
> +        if (cpu_isar_feature(aa64_dcpodp, cpu)) {
> +            define_one_arm_cp_reg(cpu, dcpodp_reg);
> +        }
> +    }
> +#endif /*CONFIG_USER_ONLY*/
>   #endif
>   
>       /*

Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins

[Peter Maydell]

On Tue, 28 Nov 2023 at 11:24, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> On 6/11/19 00:41, Beata Michalska wrote:
> > ARMv8.2 introduced support for Data Cache Clean instructions
> > to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
> > - DV CVADP. Both specify conceptual points in a memory system where all writes
> > that are to reach them are considered persistent.
> > The support provided considers both to be actually the same so there is no
> > distinction between the two. If none is available (there is no backing store
> > for given memory) both will result in Data Cache Clean up to the point of
> > coherency. Otherwise sync for the specified range shall be performed.
> >
> > Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> > ---
> >   linux-user/elfload.c |  2 ++
> >   target/arm/cpu.h     | 10 ++++++++++
> >   target/arm/cpu64.c   |  1 +
> >   target/arm/helper.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 69 insertions(+)
>
>
> > +#ifndef CONFIG_USER_ONLY
> > +static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
> > +                          uint64_t value)
> > +{
> > +    ARMCPU *cpu = env_archcpu(env);
> > +    /* CTR_EL0 System register -> DminLine, bits [19:16] */
> > +    uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
> > +    uint64_t vaddr_in = (uint64_t) value;
> > +    uint64_t vaddr = vaddr_in & ~(dline_size - 1);
> > +    void *haddr;
> > +    int mem_idx = cpu_mmu_index(env, false);
> > +
> > +    /* This won't be crossing page boundaries */
> > +    haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC());
> > +    if (haddr) {
> > +
> > +        ram_addr_t offset;
> > +        MemoryRegion *mr;
> > +
> > +        /* RCU lock is already being held */
> > +        mr = memory_region_from_host(haddr, &offset);
> > +
> > +        if (mr) {
> > +            memory_region_do_writeback(mr, offset, dline_size);
> > +        }
> > +    }
> > +}
>
>
> > +#ifndef CONFIG_USER_ONLY
> > +    /* Data Cache clean instructions up to PoP */
> > +    if (cpu_isar_feature(aa64_dcpop, cpu)) {
>
> Am I correct understanding this is a TCG-only feature?

For KVM, whether the vCPU implements these cache
maintenance instructions is up to it -- like all insns,
QEMU doesn't ever see if the guest executes them or not
(either the host CPU just implements them, or the host
kernel traps and handles them). The code in this patch is
specifically for the QEMU TCG emulation of them.

thanks
-- PMM

Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins

[Philippe Mathieu-Daudé]

On 28/11/23 12:34, Peter Maydell wrote:
> On Tue, 28 Nov 2023 at 11:24, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi,
>>
>> On 6/11/19 00:41, Beata Michalska wrote:
>>> ARMv8.2 introduced support for Data Cache Clean instructions
>>> to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
>>> - DV CVADP. Both specify conceptual points in a memory system where all writes
>>> that are to reach them are considered persistent.
>>> The support provided considers both to be actually the same so there is no
>>> distinction between the two. If none is available (there is no backing store
>>> for given memory) both will result in Data Cache Clean up to the point of
>>> coherency. Otherwise sync for the specified range shall be performed.
>>>
>>> Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
>>> ---
>>>    linux-user/elfload.c |  2 ++
>>>    target/arm/cpu.h     | 10 ++++++++++
>>>    target/arm/cpu64.c   |  1 +
>>>    target/arm/helper.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    4 files changed, 69 insertions(+)
>>
>>
>>> +#ifndef CONFIG_USER_ONLY
>>> +static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
>>> +                          uint64_t value)
>>> +{
>>> +    ARMCPU *cpu = env_archcpu(env);
>>> +    /* CTR_EL0 System register -> DminLine, bits [19:16] */
>>> +    uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
>>> +    uint64_t vaddr_in = (uint64_t) value;
>>> +    uint64_t vaddr = vaddr_in & ~(dline_size - 1);
>>> +    void *haddr;
>>> +    int mem_idx = cpu_mmu_index(env, false);
>>> +
>>> +    /* This won't be crossing page boundaries */
>>> +    haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC());
>>> +    if (haddr) {
>>> +
>>> +        ram_addr_t offset;
>>> +        MemoryRegion *mr;
>>> +
>>> +        /* RCU lock is already being held */
>>> +        mr = memory_region_from_host(haddr, &offset);
>>> +
>>> +        if (mr) {
>>> +            memory_region_do_writeback(mr, offset, dline_size);
>>> +        }
>>> +    }
>>> +}
>>
>>
>>> +#ifndef CONFIG_USER_ONLY
>>> +    /* Data Cache clean instructions up to PoP */
>>> +    if (cpu_isar_feature(aa64_dcpop, cpu)) {
>>
>> Am I correct understanding this is a TCG-only feature?
> 
> For KVM, whether the vCPU implements these cache
> maintenance instructions is up to it -- like all insns,
> QEMU doesn't ever see if the guest executes them or not
> (either the host CPU just implements them, or the host
> kernel traps and handles them). The code in this patch is
> specifically for the QEMU TCG emulation of them.

Thank you Peter. In this case I'm compiling HVF, but this is the
same reasoning. I'll add #ifdef'ry similar to ats_write() (commit
9fb005b02d "target/arm: Restrict the Address Translate write operation
to TCG accel"):

-- >8 --
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 99c7da9ca4..a05e613e10 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7629,6 +7629,7 @@ static const ARMCPRegInfo rndr_reginfo[] = {
  static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
                            uint64_t value)
  {
+#ifdef CONFIG_TCG
      ARMCPU *cpu = env_archcpu(env);
      /* CTR_EL0 System register -> DminLine, bits [19:16] */
      uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
@@ -7653,6 +7654,10 @@ static void dccvap_writefn(CPUARMState *env, 
const ARMCPRegInfo *opaque,
          }
  #endif /*CONFIG_USER_ONLY*/
      }
+#else
+    /* Handled by hardware accelerator. */
+    g_assert_not_reached();
+#endif /* CONFIG_TCG */
  }
---

Regards,

Phil.

Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins

[Richard Henderson]

On 11/28/23 05:44, Philippe Mathieu-Daudé wrote:
> Thank you Peter. In this case I'm compiling HVF, but this is the
> same reasoning. I'll add #ifdef'ry similar to ats_write() (commit
> 9fb005b02d "target/arm: Restrict the Address Translate write operation
> to TCG accel"):
> 
> -- >8 --
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 99c7da9ca4..a05e613e10 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7629,6 +7629,7 @@ static const ARMCPRegInfo rndr_reginfo[] = {
>   static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
>                             uint64_t value)
>   {
> +#ifdef CONFIG_TCG
>       ARMCPU *cpu = env_archcpu(env);
>       /* CTR_EL0 System register -> DminLine, bits [19:16] */
>       uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
> @@ -7653,6 +7654,10 @@ static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo 
> *opaque,
>           }
>   #endif /*CONFIG_USER_ONLY*/
>       }
> +#else
> +    /* Handled by hardware accelerator. */
> +    g_assert_not_reached();
> +#endif /* CONFIG_TCG */
>   }

Yep.

r~