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

[Peter Maydell]

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() ?

-- PMM