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

[David Hildenbrand]

On 23.08.21 18:41, 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).

s/not/no/

> The MEMTXPERM_UNRESTRICTED case works as an allow list. Devices
> using it won't be checked by flatview_access_allowed().

Well, and MEMTXPERM_UNSPECIFIED. Another indication that the split 
should better be avoided.

> 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;
> +        }

I'm a bit confused why it's called MEMTXPERM_RAM_DEVICE, yet we also 
allow access to !memory_region_is_ram_device(mr).

Can we find a more expressive name?

Also, I wonder if we'd actually want to have "flags" instead of pure 
values. Like having

#define MEMTXPERM_RAM           1
#define MEMTXPERM_RAM_DEVICE    2

and map them cleanly to the two similar, but different types of memory 
backends.


> +        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;
> +        }
> +        return false;
> +    } else {
> +        /* MEMTXPERM_UNRESTRICTED and MEMTXPERM_UNSPECIFIED cases */
> +        return true;
> +    }
>   }
>   
>   /* Called within RCU critical section.  */

Do we have any target user examples / prototypes?

--
Thanks,

David / dhildenb