sh: movca.l cancel by ocbi

Current sh4's "movca.l" instruction is implemented in a same way as "mov.l". Then, write to memory cannot be canceled by "ocbi" cache control instrunction. This makes text area broken on cache flush by linux kernel.

This patch delays "movca.l" execution and provide the chance to cancel it for "ocbi".
# Thank you Edgar, for your advice!

This patch does,
- on executing "movca.l", just records what and where movca should store data.
- on executing "ocbi", find the corresponding record by movca, and delete it to cancel.
- lets TCG produce "delayed_movca" instruction at the first instruction which is neither "movca.l" nor "ocbi".
- on executing "delayed_movca", does the data store task, according to the record.

Signed-off-by Shin-ichiro KAWASAKI <[email protected]>

Index: trunk/target-sh4/op_helper.c
===================================================================

target-sh4/op_helper.c +1 -0

@@ -20,6 +20,7 @@
 #include <assert.h>
 #include "exec.h"
 #include "helper.h"
+void *qemu_mallocz(size_t size);
 
 #ifndef CONFIG_USER_ONLY
 

target-sh4/op_helper.c +21 -0

+	    cur->valid = 1;
+	    cur->value = val;
+	    cur->addr = addr;
+	    return;
+	}
+	prev = cur;
+	cur = cur->next;
+    }
+
+    /* movca entry shortage. allocate it. */
+    prev->next = cur = qemu_mallocz(sizeof(delayed_movca_t));
+    if (cur == NULL) {
+	printf("out of memory for delayed movca. @%08x\n", addr);
+	return;
+    }
+    cur->valid = 1;
+    cur->value = val;
+    cur->addr = addr;
+}
+
+void helper_ocbi(uint32_t addr)

Quoted 8 lines · Shin-ichiro KAWASAKI

Current sh4's "movca.l" instruction is implemented in a same way as "mov.l". Then, write to memory cannot be canceled by "ocbi" cache control instrunction. This makes text area broken on cache flush by linux kernel.

This patch delays "movca.l" execution and provide the chance to cancel it for "ocbi". # Thank you Edgar, for your advice!

Quoted 10 lines

This patch does,
- on executing "movca.l", just records what and where movca should store data.
- on executing "ocbi", find the corresponding record by movca, and delete it to cancel.
- lets TCG produce "delayed_movca" instruction at the first instruction which is neither "movca.l" nor "ocbi".
- on executing "delayed_movca", does the data store task, according to the record.

Hello,

I think your patch will catch the linux dflush sequence but I've got a few comments.

There is a complication with the delayed stores and potential mmu exceptions that they might generate. I think with this approach you'll take those faults at the next instruction which might not even be a load/store. Not sure if that can cause problems for sh. Maybe you could avoid that by making movca a load + store and the ocbi a store with the original value loaded at movca. But you'll still get into trouble with exceptions that might break the movca/ocbi sequence.

I don't it's worth having a dynamically sized movca delay buffer. The movca insn has a restricted addressing mode making it a bit hard to use back-to-back without alu insns in between. IIU your code correctly you flush the buffer before every non movca/ocbi insn so I guess that for most realistic use-cases you'll only need very few (maybe only 1) entries in the delay buffer.

Quoted 15 lines

Signed-off-by Shin-ichiro KAWASAKI <[email protected]>

Index: trunk/target-sh4/op_helper.c
===================================================================

target-sh4/op_helper.c +1 -0

@@ -20,6 +20,7 @@
 #include <assert.h>
 #include "exec.h"
 #include "helper.h"
+void *qemu_mallocz(size_t size);
 
 #ifndef CONFIG_USER_ONLY
 

target-sh4/op_helper.c line 614

Quoted 10 lines

target-sh4/op_helper.c +7 -0

@@ -604,3 +605,57 @@
     d.ll = t0;
     return float64_to_int32_round_to_zero(d.d, &env->fp_status);
 }
+
+void helper_movca(uint32_t val, uint32_t addr)
+{
+    delayed_movca_t *cur = &env->movca_list;
+    delayed_movca_t *prev = NULL;
+    while (cur) {
+	if (!cur->valid) {

Might be overkill but you can merge here, i.e:

	if (!cur->valid || cur->addr == addr) {

Quoted 21 lines

target-sh4/op_helper.c +21 -0

+	    cur->valid = 1;
+	    cur->value = val;
+	    cur->addr = addr;
+	    return;
+	}
+	prev = cur;
+	cur = cur->next;
+    }
+
+    /* movca entry shortage. allocate it. */
+    prev->next = cur = qemu_mallocz(sizeof(delayed_movca_t));
+    if (cur == NULL) {
+	printf("out of memory for delayed movca. @%08x\n", addr);
+	return;
+    }
+    cur->valid = 1;
+    cur->value = val;
+    cur->addr = addr;
+}
+
+void helper_ocbi(uint32_t addr)

target-sh4/op_helper.c line 645

Quoted 10 lines

target-sh4/op_helper.c +10 -0

+{
+    delayed_movca_t *cur = &env->movca_list;
+
+    while (cur) {
+	if (cur->valid && cur->addr == addr) { /* found! */
+	    cur->valid = 0;
+	    return;
+	}
+	cur = cur->next;
+    }

I think you can quite easy catch a few more movca use cases if you dont compare line offsets, i.e for sh4 you'd mask the addr & ~(32 - 1). Also, you should not return until you've scanned the entire movca delay buffer as there might be multiple stores to the same line that need to be ignored.

example:

    addr &= ~31;
    while (cur) {
       if (cur->valid && (cur->addr & ~31) == addr)
           cur->valid = 0;
       cur = cur->next;
    }

Thank you for your commmnts!

Quoted 24 lines · Edgar E. Iglesias

Quoted 8 lines · Shin-ichiro KAWASAKI

Current sh4's "movca.l" instruction is implemented in a same way as "mov.l". Then, write to memory cannot be canceled by "ocbi" cache control instrunction. This makes text area broken on cache flush by linux kernel.

This patch delays "movca.l" execution and provide the chance to cancel it for "ocbi". # Thank you Edgar, for your advice!

Quoted 9 lines

This patch does,
- on executing "movca.l", just records what and where movca should
store data.
- on executing "ocbi", find the corresponding record by movca, and delete it to cancel.
- lets TCG produce "delayed_movca" instruction at the first instruction which is neither "movca.l" nor "ocbi".
- on executing "delayed_movca", does the data store task, according to the record.

Hello,

I think your patch will catch the linux dflush sequence but I've got a few comments.

Quoted 8 lines

There is a complication with the delayed stores and potential mmu exceptions that they might generate. I think with this approach you'll take those faults at the next instruction which might not even be a load/store. Not sure if that can cause problems for sh. Maybe you could avoid that by making movca a load + store and the ocbi a store with the original value loaded at movca. But you'll still get into trouble with exceptions that might break the movca/ocbi sequence.

That's an idea. Current implementation is "delay and do-or-cancel" way. But your idea is "do and commit-or-rollback" way.
I'll try to implement it to avoid mmu fault by movca.
The mmu fault by movca will cause invoke exception handler, and break movca/ocbi sequence. It will be a quite rare case, however, I'll consider it for next version.

Quoted 6 lines

I don't it's worth having a dynamically sized movca delay buffer. The movca insn has a restricted addressing mode making it a bit hard to use back-to-back without alu insns in between. IIU your code correctly you flush the buffer before every non movca/ocbi insn so I guess that for most realistic use-cases you'll only need very few (maybe only 1) entries in the delay buffer.

That is a point I wavered.
I investigated linux kernel and found following lines in "__flush_dcache_segment_4way()" in "arch/sh/mm/cache-sh4.c".

    a0 = base_addr;
    a1 = a0 + way_incr;
    a2 = a1 + way_incr;
    a3 = a2 + way_incr;
    a0e = base_addr + extent_per_way;
    do {
        asm volatile("ldc %0, sr" : : "r" (sr_with_bl));
        asm volatile("movca.l r0, @%0\n\t"
                 "movca.l r0, @%1\n\t"
                 "movca.l r0, @%2\n\t"
                 "movca.l r0, @%3\n\t"
                 "ocbi @%0\n\t"
                 "ocbi @%1\n\t"
                 "ocbi @%2\n\t"
                 "ocbi @%3\n\t" : :
                 "r" (a0), "r" (a1), "r" (a2), "r" (a3));

Multiple movca.l insns executed consequently, and they access different lines. At least, 4 entries are needed for it. This code shows kernel implementation decides how many number of movcas are invoked consequently. That's the reason why I introudcued dynamic size change.

Quoted 69 lines

Quoted 15 lines

Signed-off-by Shin-ichiro KAWASAKI <[email protected]>

Index: trunk/target-sh4/op_helper.c
===================================================================

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +1 -0

@@ -20,6 +20,7 @@
 #include <assert.h>
 #include "exec.h"
 #include "helper.h"
+void *qemu_mallocz(size_t size);
 
 #ifndef CONFIG_USER_ONLY
 

target-sh4/op_helper.c (working copy) line 614

Quoted 10 lines

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +7 -0

@@ -604,3 +605,57 @@
     d.ll = t0;
     return float64_to_int32_round_to_zero(d.d, &env->fp_status);
 }
+
+void helper_movca(uint32_t val, uint32_t addr)
+{
+    delayed_movca_t *cur = &env->movca_list;
+    delayed_movca_t *prev = NULL;
+    while (cur) {
+    if (!cur->valid) {

Might be overkill but you can merge here, i.e:

    if (!cur->valid || cur->addr == addr) {

Quoted 21 lines

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +21 -0

+        cur->valid = 1;
+        cur->value = val;
+        cur->addr = addr;
+        return;
+    }
+    prev = cur;
+    cur = cur->next;
+    }
+
+    /* movca entry shortage. allocate it. */
+    prev->next = cur = qemu_mallocz(sizeof(delayed_movca_t));
+    if (cur == NULL) {
+    printf("out of memory for delayed movca. @%08x\n", addr);
+    return;
+    }
+    cur->valid = 1;
+    cur->value = val;
+    cur->addr = addr;
+}
+
+void helper_ocbi(uint32_t addr)

target-sh4/op_helper.c (working copy) line 645

Quoted 10 lines

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +10 -0

+{
+    delayed_movca_t *cur = &env->movca_list;
+
+    while (cur) {
+    if (cur->valid && cur->addr == addr) { /* found! */
+        cur->valid = 0;
+        return;
+    }
+    cur = cur->next;
+    }

I think you can quite easy catch a few more movca use cases if you dont compare line offsets, i.e for sh4 you'd mask the addr & ~(32 - 1). Also, you should not return until you've scanned the entire movca delay buffer as there might be multiple stores to the same line that need to be ignored.

Quoted 7 lines

example:

    addr &= ~31;
    while (cur) {
       if (cur->valid && (cur->addr & ~31) == addr)
           cur->valid = 0;
       cur = cur->next;
    }

That's a good advice. I'll try to implement it for next version.

Thanks again for your review!

Regards,
Shin-ichiro KAWASAKI

I've completely passed over Vladimir-san's nice patch on movca-ocbi, even though it is already in qemu-sh staging repository.

http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg00605.html

But now, I can review the patch. I'll do it later.

Regards,
Shin-ichiro KAWASAKI

Quoted 150 lines · Shin-ichiro KAWASAKI

Thank you for your commmnts!

Quoted 25 lines · Edgar E. Iglesias

Quoted 9 lines · Shin-ichiro KAWASAKI

Current sh4's "movca.l" instruction is implemented in a same way as "mov.l". Then, write to memory cannot be canceled by "ocbi" cache control instrunction. This makes text area broken on cache flush by linux kernel.

This patch delays "movca.l" execution and provide the chance to cancel it for "ocbi".
# Thank you Edgar, for your advice!

Quoted 9 lines

This patch does,
- on executing "movca.l", just records what and where movca should
store data.
- on executing "ocbi", find the corresponding record by movca, and delete it to cancel.
- lets TCG produce "delayed_movca" instruction at the first instruction which is neither "movca.l" nor "ocbi".
- on executing "delayed_movca", does the data store task, according to the record.

Hello,

I think your patch will catch the linux dflush sequence but I've got a few comments.

Quoted 8 lines

There is a complication with the delayed stores and potential mmu exceptions that they might generate. I think with this approach you'll take those faults at the next instruction which might not even be a load/store. Not sure if that can cause problems for sh. Maybe you could avoid that by making movca a load + store and the ocbi a store with the original value loaded at movca. But you'll still get into trouble with exceptions that might break the movca/ocbi sequence.

That's an idea. Current implementation is "delay and do-or-cancel" way. But your idea is "do and commit-or-rollback" way.
I'll try to implement it to avoid mmu fault by movca.
The mmu fault by movca will cause invoke exception handler, and break movca/ocbi sequence. It will be a quite rare case, however, I'll consider it for next version.

Quoted 6 lines

I don't it's worth having a dynamically sized movca delay buffer. The movca insn has a restricted addressing mode making it a bit hard to use back-to-back without alu insns in between. IIU your code correctly you flush the buffer before every non movca/ocbi insn so I guess that for most realistic use-cases you'll only need very few (maybe only 1) entries in the delay buffer.

That is a point I wavered.
I investigated linux kernel and found following lines in "__flush_dcache_segment_4way()" in "arch/sh/mm/cache-sh4.c".

    a0 = base_addr;
    a1 = a0 + way_incr;
    a2 = a1 + way_incr;
    a3 = a2 + way_incr;
    a0e = base_addr + extent_per_way;
    do {
        asm volatile("ldc %0, sr" : : "r" (sr_with_bl));
        asm volatile("movca.l r0, @%0\n\t"
                 "movca.l r0, @%1\n\t"
                 "movca.l r0, @%2\n\t"
                 "movca.l r0, @%3\n\t"
                 "ocbi @%0\n\t"
                 "ocbi @%1\n\t"
                 "ocbi @%2\n\t"
                 "ocbi @%3\n\t" : :
                 "r" (a0), "r" (a1), "r" (a2), "r" (a3));

Multiple movca.l insns executed consequently, and they access different lines.
At least, 4 entries are needed for it. This code shows kernel implementation
decides how many number of movcas are invoked consequently. That's the reason why I introudcued dynamic size change.

Quoted 67 lines

Quoted 16 lines

Signed-off-by Shin-ichiro KAWASAKI <[email protected]>

Index: trunk/target-sh4/op_helper.c
===================================================================

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +1 -0

@@ -20,6 +20,7 @@
 #include <assert.h>
 #include "exec.h"
 #include "helper.h"
+void *qemu_mallocz(size_t size);
  #ifndef CONFIG_USER_ONLY
 @@ -604,3 +605,57 @@
     d.ll = t0;

     return float64_to_int32_round_to_zero(d.d, &env->fp_status);

}

target-sh4/op_helper.c (working copy)

Quoted 7 lines

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +7 -0

+
+void helper_movca(uint32_t val, uint32_t addr)
+{
+    delayed_movca_t *cur = &env->movca_list;
+    delayed_movca_t *prev = NULL;
+    while (cur) {
+    if (!cur->valid) {

Might be overkill but you can merge here, i.e:

    if (!cur->valid || cur->addr == addr) {

Quoted 21 lines

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +21 -0

+        cur->valid = 1;
+        cur->value = val;
+        cur->addr = addr;
+        return;
+    }
+    prev = cur;
+    cur = cur->next;
+    }
+
+    /* movca entry shortage. allocate it. */
+    prev->next = cur = qemu_mallocz(sizeof(delayed_movca_t));
+    if (cur == NULL) {
+    printf("out of memory for delayed movca. @%08x\n", addr);
+    return;
+    }
+    cur->valid = 1;
+    cur->value = val;
+    cur->addr = addr;
+}
+
+void helper_ocbi(uint32_t addr)

target-sh4/op_helper.c (working copy)

Quoted 10 lines

target-sh4/op_helper.c (working copy) renamed from target-sh4/op_helper.c (revision 6133) +10 -0

+{
+    delayed_movca_t *cur = &env->movca_list;
+
+    while (cur) {
+    if (cur->valid && cur->addr == addr) { /* found! */
+        cur->valid = 0;
+        return;
+    }
+    cur = cur->next;
+    }

I think you can quite easy catch a few more movca use cases if you dont compare line offsets, i.e for sh4 you'd mask the addr & ~(32 - 1). Also, you should not return until you've scanned the entire movca delay buffer as there might be multiple stores to the same line that need to be ignored.

Quoted 7 lines

example:

    addr &= ~31;
    while (cur) {
       if (cur->valid && (cur->addr & ~31) == addr)
           cur->valid = 0;
       cur = cur->next;
    }

That's a good advice. I'll try to implement it for next version. Thanks again for your review!
Regards,
Shin-ichiro KAWASAKI

SE7750 uses SCI := Serial Communication Interface for one of consoles. This patch completes the SCI implementation, and makes SCI available for a console.

# Tabs and spaces are mixed in "hw/sh_serial.c" so much. # Some clean up might be useful.

SE7750 uses SCI := Serial Communication Interface for one of consoles. This patch completes the SCI implementation, and makes SCI available for a console.

# Tabs and spaces are mixed in "hw/sh_serial.c" so much. # Some clean up might be useful.

Signed-off-by Shin-ichiro KAWASAKI <[email protected]>