On Tue, 24 Aug 2021 at 14:50, Philippe Mathieu-Daudé <phi...@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é <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;
> }
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
