Re: [PATCH v4 10/16] cputlb: Partially inline memory_region_section_get_iotlb

[Richard Henderson]

On 9/24/19 12:59 AM, David Hildenbrand wrote:
>> +    is_ram = memory_region_is_ram(section->mr);
>> +    is_romd = memory_region_is_romd(section->mr);
>> +
>> +    if (is_ram || is_romd) {
>> +        /* RAM and ROMD both have associated host memory. */
>>          addend = (uintptr_t)memory_region_get_ram_ptr(section->mr) + xlat;
>> +    } else {
>> +        /* I/O does not; force the host address to NULL. */
>> +        addend = 0;
>> +    }
>> +
>> +    write_address = address;
> 
> I guess the only "suboptimal" change is that you now have two checks for
> "prot & PAGE_WRITE" twice in the case of ram instead of one.

It's a single bit test on a register operand -- as cheap as can be.  If you
look at the entire code, there *must* be more than one test.  You can rearrange
the code to choose exactly where those tests are, but you'll have to have them
somewhere.

>> +        /* I/O or ROMD */
>> +        iotlb = memory_region_section_get_iotlb(cpu, section) + xlat;
>> +        /*
>> +         * Writes to romd devices must go through MMIO to enable write.
>> +         * Reads to romd devices go through the ram_ptr found above,
>> +         * but of course reads to I/O must go through MMIO.
>> +         */
>> +        write_address |= TLB_MMIO;
> 
> ... and here you calculate write_address even if probably unused.

Well... while the page might not be writable (but I'd bet that it is -- I/O
memory is almost never read-only), and therefore write_address is technically
unused, the variable is practically used in the next line:

    if (!is_romd) {
        address = write_address
    }

which will compile to a conditional move.

> Can your move the calculation of the write_address completely into the
> "prot & PAGE_WRITE" case below?

We'd prefer not to, since the code below is within the cpu tlb lock region.
We'd prefer to keep all of the expensive operations outside that.


r~