On 25.09.19 19:55, Richard Henderson wrote:
> 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.
Makes all sense to me then and looks sane :)
>
>
> r~
>
--
Thanks,
David / dhildenb
