Re: [RFC PATCH v2 5/5] softmmu/physmem: Have flaview API check MemTxAttrs::bus_perm field

[Philippe Mathieu-Daudé]

On 8/24/21 3:15 PM, Stefan Hajnoczi wrote:
> On Mon, Aug 23, 2021 at 06:41:57PM +0200, Philippe Mathieu-Daudé wrote:
>> Check bus permission in flatview_access_allowed() before
>> running any bus transaction.
>>
>> There is not change for the default case (MEMTXPERM_UNSPECIFIED).
>>
>> The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices
>> using it won't be checked by flatview_access_allowed().
>>
>> The only deny list equivalent is MEMTXPERM_RAM_DEVICE: devices
>> using this flag will reject transactions and set the optional
>> MemTxResult to MEMTX_BUS_ERROR.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> ---
>>  softmmu/physmem.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 0d31a2f4199..329542dba75 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -2772,7 +2772,22 @@ static inline bool 
>> flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
>>                                             hwaddr addr, hwaddr len,
>>                                             MemTxResult *result)
>>  {
>> -    return true;
>> +    if (unlikely(attrs.bus_perm == MEMTXPERM_RAM_DEVICE)) {
>> +        if (memory_region_is_ram(mr) || memory_region_is_ram_device(mr)) {
>> +            return true;
>> +        }
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "Invalid access to non-RAM device at "
>> +                      "addr 0x%" HWADDR_PRIX ", size %" HWADDR_PRIu ", "
>> +                      "region '%s'\n", addr, len, memory_region_name(mr));
>> +        if (result) {
>> +            *result |= MEMTX_BUS_ERROR;
> 
> Why bitwise OR?

MemTxResult is not an enum but used as a bitfield.

See access_with_adjusted_size():

    MemTxResult r = MEMTX_OK;
    ...
    if (memory_region_big_endian(mr)) {
        for (i = 0; i < size; i += access_size) {
            r |= access_fn(mr, addr + i, value, access_size,
                        (size - access_size - i) * 8,
                        access_mask, attrs);
        }
    } else {
        for (i = 0; i < size; i += access_size) {
            r |= access_fn(mr, addr + i, value, access_size, i * 8,
                        access_mask, attrs);
        }
    }
    return r;
}

and flatview_read_continue() / flatview_write_continue():

    for (;;) {
        if (!memory_access_is_direct(mr, true)) {
            release_lock |= prepare_mmio_access(mr);
            l = memory_access_size(mr, l, addr1);
            val = ldn_he_p(buf, l);
            result |= memory_region_dispatch_write(mr, addr1, val,
                                                   size_memop(l),
                                                   attrs);
    ...
    return result;
}