On 8/24/21 16:21, Peter Maydell wrote:
> 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() ?
You are right, this is not the case here, so we can simplify as
Stefan suggested. Thanks for clarifying the examples.
