Re: [Qemu-devel] [PATCH v5 12/37] target-arm: A64: Implement DC ZVA

[Peter Maydell]

On 28 March 2014 18:42, Richard Henderson <address@hidden> wrote:
> On 03/28/2014 09:09 AM, Peter Maydell wrote:
>> +            for (i = 0; i < maxidx; i++) {
>> +                hostaddr[i] = tlb_vaddr_to_host(env,
>> +                                                vaddr + TARGET_PAGE_SIZE * 
>> i,
>> +                                                1, cpu_mmu_index(env));
>> +                if (!hostaddr[i]) {
>> +                    break;
>> +                }
>> +            }
>> +            if (i == maxidx) {
>> +                /* If it's all in the TLB it's fair game for just writing 
>> to;
>> +                 * we know we don't need to update dirty status, etc.
>> +                 */
>> +                for (i = 0; i < maxidx - 1; i++) {
>> +                    memset(hostaddr[i], 0, TARGET_PAGE_SIZE);
>> +                }
>> +                memset(hostaddr[i], 0, blocklen - (i * TARGET_PAGE_SIZE));
>> +                return;
>> +            }
>
> Doesn't this fail if blocklen < TARGET_PAGE_SIZE?

I can't see where it does -- in that case, maxidx is 1,
and so we do the tlb_vaddr_to_host loopup once, then
(assuming the TLB hit) i == 1 so we take this if, and
the for() loop does nothing (except set i to 0); then we
do all the work via the final memset, where i == 0 and
so we memset blocklen bytes exactly.

> Since blocklen must be a power of 4, it's either less than TARGET_PAGE_SIZE or
> a multiple of TARGET_PAGE_SIZE, so that last memset looks suspect.

If blocklen is a multiple of TARGET_PAGE_SIZE then
blocklen - (i * TARGET_PAGE_SIZE) will always be
TARGET_PAGE_SIZE. As it happens the code will work
for any blocksize, but multiples of TARGET_PAGE_SIZE
are just a special case of that :-)

> I think all this would be easier to follow as two cases:
>
>     if (blocklen <= TARGET_PAGE_SIZE) {
>         // One look up and no hostaddr array
>     } else {
>         // Multiple pages; much of what you have now, only no partial pages
>     }

I'm not convinced. You get a dozen extra lines of code for the
<= TARGET_PAGE_SIZE case, and the only change in the
other case is that you get to delete that one memset because
you can adjust the upper bound of the loop. So it doesn't
seem to me like it's any easier to review, because it's
basically the same code plus extra code that isn't needed.

thanks
-- PMM