diff mbox series

[RFC,v3,3/3] softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR

Message ID 20211215182421.418374-4-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series physmem: Have flaview API check bus permission from MemTxAttrs argument | expand

Commit Message

Philippe Mathieu-Daudé Dec. 15, 2021, 6:24 p.m. UTC
Add the 'memory' bit to the memory attributes to restrict bus
controller accesses to memories.

Introduce flatview_access_allowed() to check bus permission
before running any bus transaction.

Have read/write accessors return MEMTX_BUS_ERROR if an access is
restricted.

There is no change for the default case where 'memory' is not set.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memattrs.h |  9 +++++++++
 softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 2 deletions(-)

Comments

Richard Henderson Dec. 17, 2021, 7:46 p.m. UTC | #1
On 12/15/21 10:24 AM, Philippe Mathieu-Daudé wrote:
> +static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
> +                                           hwaddr addr, hwaddr len)

There's no need to mark this inline. Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Peter Maydell Dec. 17, 2021, 10:34 p.m. UTC | #2
On Wed, 15 Dec 2021 at 18:24, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Add the 'memory' bit to the memory attributes to restrict bus
> controller accesses to memories.
>
> Introduce flatview_access_allowed() to check bus permission
> before running any bus transaction.
>
> Have read/write accessors return MEMTX_BUS_ERROR if an access is
> restricted.
>
> There is no change for the default case where 'memory' is not set.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/memattrs.h |  9 +++++++++
>  softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 95f2d20d55b..f0063583ee2 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -35,6 +35,14 @@ typedef struct MemTxAttrs {
>      unsigned int secure:1;
>      /* Memory access is usermode (unprivileged) */
>      unsigned int user:1;
> +    /*
> +     * Bus interconnect and peripherals can access anything (memories,
> +     * devices) by default. By setting the 'memory' bit, bus transaction
> +     * are restricted to "normal" memories (per the AMBA documentation)
> +     * versus devices. Access to devices will be logged and rejected
> +     * (see MEMTX_BUS_ERROR).
> +     */
> +    unsigned int memory:1;
>      /* Requester ID (for MSI for example) */
>      unsigned int requester_id:16;
>      /* Invert endianness for this page */
> @@ -66,6 +74,7 @@ typedef struct MemTxAttrs {
>  #define MEMTX_OK 0
>  #define MEMTX_ERROR             (1U << 0) /* device returned an error */
>  #define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
> +#define MEMTX_BUS_ERROR         (1U << 2) /* bus returned an error */

This is kind of odd naming, because MEMTX_DECODE_ERROR already means
"bus/interconnect returned an error" and it generally translates
into what at the OS level gets called a "bus error"...

-- PMM
Philippe Mathieu-Daudé Dec. 17, 2021, 11:18 p.m. UTC | #3
On 12/17/21 23:34, Peter Maydell wrote:
> On Wed, 15 Dec 2021 at 18:24, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Add the 'memory' bit to the memory attributes to restrict bus
>> controller accesses to memories.
>>
>> Introduce flatview_access_allowed() to check bus permission
>> before running any bus transaction.
>>
>> Have read/write accessors return MEMTX_BUS_ERROR if an access is
>> restricted.
>>
>> There is no change for the default case where 'memory' is not set.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/exec/memattrs.h |  9 +++++++++
>>  softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index 95f2d20d55b..f0063583ee2 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -35,6 +35,14 @@ typedef struct MemTxAttrs {
>>      unsigned int secure:1;
>>      /* Memory access is usermode (unprivileged) */
>>      unsigned int user:1;
>> +    /*
>> +     * Bus interconnect and peripherals can access anything (memories,
>> +     * devices) by default. By setting the 'memory' bit, bus transaction
>> +     * are restricted to "normal" memories (per the AMBA documentation)
>> +     * versus devices. Access to devices will be logged and rejected
>> +     * (see MEMTX_BUS_ERROR).
>> +     */
>> +    unsigned int memory:1;
>>      /* Requester ID (for MSI for example) */
>>      unsigned int requester_id:16;
>>      /* Invert endianness for this page */
>> @@ -66,6 +74,7 @@ typedef struct MemTxAttrs {
>>  #define MEMTX_OK 0
>>  #define MEMTX_ERROR             (1U << 0) /* device returned an error */
>>  #define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
>> +#define MEMTX_BUS_ERROR         (1U << 2) /* bus returned an error */
> 
> This is kind of odd naming, because MEMTX_DECODE_ERROR already means
> "bus/interconnect returned an error" and it generally translates
> into what at the OS level gets called a "bus error"...

MEMTX_DECODE_ERROR is "nothing at that address". We want a name
for "there is something, but you don't have access to it".
Maybe MEMTX_ILLEGAL_ERROR?
Richard Henderson Dec. 18, 2021, 2:07 a.m. UTC | #4
On 12/17/21 3:18 PM, Philippe Mathieu-Daudé wrote:
>>>   #define MEMTX_OK 0
>>>   #define MEMTX_ERROR             (1U << 0) /* device returned an error */
>>>   #define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
>>> +#define MEMTX_BUS_ERROR         (1U << 2) /* bus returned an error */
>>
>> This is kind of odd naming, because MEMTX_DECODE_ERROR already means
>> "bus/interconnect returned an error" and it generally translates
>> into what at the OS level gets called a "bus error"...
> 
> MEMTX_DECODE_ERROR is "nothing at that address". We want a name
> for "there is something, but you don't have access to it".
> Maybe MEMTX_ILLEGAL_ERROR?

ILLEGAL doesn't convey much.  MEMTX_ACCESS_ERROR?

r~
Philippe Mathieu-Daudé Dec. 18, 2021, 11:26 a.m. UTC | #5
On 12/18/21 03:07, Richard Henderson wrote:
> On 12/17/21 3:18 PM, Philippe Mathieu-Daudé wrote:
>>>>   #define MEMTX_OK 0
>>>>   #define MEMTX_ERROR             (1U << 0) /* device returned an
>>>> error */
>>>>   #define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that
>>>> address */
>>>> +#define MEMTX_BUS_ERROR         (1U << 2) /* bus returned an error */
>>>
>>> This is kind of odd naming, because MEMTX_DECODE_ERROR already means
>>> "bus/interconnect returned an error" and it generally translates
>>> into what at the OS level gets called a "bus error"...
>>
>> MEMTX_DECODE_ERROR is "nothing at that address". We want a name
>> for "there is something, but you don't have access to it".
>> Maybe MEMTX_ILLEGAL_ERROR?
> 
> ILLEGAL doesn't convey much.  MEMTX_ACCESS_ERROR?

OK, such:

  #define MEMTX_ACCESS_ERROR      (1U << 2) /* access denied */

alternatively MEMTX_PERM_ERROR.
Stefan Hajnoczi Jan. 24, 2022, 4:16 p.m. UTC | #6
On Wed, Dec 15, 2021 at 07:24:21PM +0100, Philippe Mathieu-Daudé wrote:
> Add the 'memory' bit to the memory attributes to restrict bus
> controller accesses to memories.
> 
> Introduce flatview_access_allowed() to check bus permission
> before running any bus transaction.
> 
> Have read/write accessors return MEMTX_BUS_ERROR if an access is
> restricted.
> 
> There is no change for the default case where 'memory' is not set.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/memattrs.h |  9 +++++++++
>  softmmu/physmem.c       | 43 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox series

Patch

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 95f2d20d55b..f0063583ee2 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -35,6 +35,14 @@  typedef struct MemTxAttrs {
     unsigned int secure:1;
     /* Memory access is usermode (unprivileged) */
     unsigned int user:1;
+    /*
+     * Bus interconnect and peripherals can access anything (memories,
+     * devices) by default. By setting the 'memory' bit, bus transaction
+     * are restricted to "normal" memories (per the AMBA documentation)
+     * versus devices. Access to devices will be logged and rejected
+     * (see MEMTX_BUS_ERROR).
+     */
+    unsigned int memory:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
     /* Invert endianness for this page */
@@ -66,6 +74,7 @@  typedef struct MemTxAttrs {
 #define MEMTX_OK 0
 #define MEMTX_ERROR             (1U << 0) /* device returned an error */
 #define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
+#define MEMTX_BUS_ERROR         (1U << 2) /* bus returned an error */
 typedef uint32_t MemTxResult;
 
 #endif
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 6c97a20107a..c03abcc0362 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2759,6 +2759,33 @@  static bool prepare_mmio_access(MemoryRegion *mr)
     return release_lock;
 }
 
+/**
+ * flatview_access_allowed
+ * @mr: #MemoryRegion to be accessed
+ * @attrs: memory transaction attributes
+ * @addr: address within that memory region
+ * @len: the number of bytes to access
+ *
+ * Check if a memory transaction is allowed.
+ *
+ * Returns: true if transaction is allowed, false if denied.
+ */
+static inline bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
+                                           hwaddr addr, hwaddr len)
+{
+    if (likely(!attrs.memory)) {
+        return true;
+    }
+    if (memory_region_is_ram(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));
+    return false;
+}
+
 /* Called within RCU critical section.  */
 static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                                            MemTxAttrs attrs,
@@ -2773,7 +2800,10 @@  static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
     const uint8_t *buf = ptr;
 
     for (;;) {
-        if (!memory_access_is_direct(mr, true)) {
+        if (!flatview_access_allowed(mr, attrs, addr1, l)) {
+            result |= MEMTX_BUS_ERROR;
+            /* Keep going. */
+        } else if (!memory_access_is_direct(mr, true)) {
             release_lock |= prepare_mmio_access(mr);
             l = memory_access_size(mr, l, addr1);
             /* XXX: could force current_cpu to NULL to avoid
@@ -2818,6 +2848,9 @@  static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
+    if (!flatview_access_allowed(mr, attrs, addr, len)) {
+        return MEMTX_BUS_ERROR;
+    }
     return flatview_write_continue(fv, addr, attrs, buf, len,
                                    addr1, l, mr);
 }
@@ -2836,7 +2869,10 @@  MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
 
     fuzz_dma_read_cb(addr, len, mr);
     for (;;) {
-        if (!memory_access_is_direct(mr, false)) {
+        if (!flatview_access_allowed(mr, attrs, addr1, l)) {
+            result |= MEMTX_BUS_ERROR;
+            /* Keep going. */
+        } else if (!memory_access_is_direct(mr, false)) {
             /* I/O case */
             release_lock |= prepare_mmio_access(mr);
             l = memory_access_size(mr, l, addr1);
@@ -2879,6 +2915,9 @@  static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
+    if (!flatview_access_allowed(mr, attrs, addr, len)) {
+        return MEMTX_BUS_ERROR;
+    }
     return flatview_read_continue(fv, addr, attrs, buf, len,
                                   addr1, l, mr);
 }