Re: [Qemu-devel] [PATCH 2/3] sh: movca.l cancel by ocbi

[Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>]

Thank you for your commmnts!

Edgar E. Iglesias wrote:
> On Sun, Jan 11, 2009 at 06:14:40PM +0900, Shin-ichiro KAWASAKI wrote:
>> 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. 
>>
> 
> 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.
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.

> 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.  


>> Signed-off-by: Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>
>>
>> Index: trunk/target-sh4/op_helper.c
>> ===================================================================
>> --- trunk/target-sh4/op_helper.c	(revision 6133)
>> +++ trunk/target-sh4/op_helper.c	(working copy)
>> @@ -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);
>>  }
>> +
>> +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) {
> 
> 
>> +	    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)
>> +{
>> +    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;
>     }

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

Thanks again for your review!

Regards,
Shin-ichiro KAWASAKI