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

[Philippe Mathieu-Daudé]

On 8/24/21 16:21, Peter Maydell wrote:
> On Tue, 24 Aug 2021 at 14:50, Philippe Mathieu-Daudé <philmd@redhat.com> 
> wrote:
>> 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é <philmd@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;
>> }
> 
> In these two examples we OR together the MemTxResults because
> we are looping over multiple accesses and combining all the
> results together; we want to return a "not OK" result if any
> of the individual results failed. Is that the case for
> flatview_access_allowed() ?

You are right, this is not the case here, so we can simplify as
Stefan suggested. Thanks for clarifying the examples.