| Message ID | 20210823164157.751807-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | physmem: Have flaview API check bus permission from MemTxAttrs argument | expand |
On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote: > +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */ > +enum { > + MEMTXPERM_UNSPECIFIED = 0, > + MEMTXPERM_UNRESTRICTED = 1, > + MEMTXPERM_RAM_DEVICE = 2, > +}; Is there a difference between UNSPECIFIED and UNRESTRICTED? If no, should we merge them?
On 23.08.21 20:41, Peter Xu wrote: > On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote: >> +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */ >> +enum { >> + MEMTXPERM_UNSPECIFIED = 0, >> + MEMTXPERM_UNRESTRICTED = 1, >> + MEMTXPERM_RAM_DEVICE = 2, >> +}; > > Is there a difference between UNSPECIFIED and UNRESTRICTED? > > If no, should we merge them? > I'd assume MEMTXPERM_UNSPECIFIED has to be treated like MEMTXPERM_UNRESTRICTED, so I'd also think we should just squash them.
On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote: > Add the 'direct_access' bit to the memory attributes to restrict > bus master access to ROM/RAM. > Have read/write accessors return MEMTX_BUS_ERROR if an access is > restricted and the region is not ROM/RAM ('direct'). > Add corresponding trace events. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/exec/memattrs.h | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h > index 95f2d20d55b..7a94ee75a88 100644 > --- a/include/exec/memattrs.h > +++ b/include/exec/memattrs.h > @@ -14,6 +14,13 @@ > #ifndef MEMATTRS_H > #define MEMATTRS_H > > +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */ > +enum { > + MEMTXPERM_UNSPECIFIED = 0, > + MEMTXPERM_UNRESTRICTED = 1, > + MEMTXPERM_RAM_DEVICE = 2, > +}; > + > /* Every memory transaction has associated with it a set of > * attributes. Some of these are generic (such as the ID of > * the bus master); some are specific to a particular kind of > @@ -35,6 +42,19 @@ typedef struct MemTxAttrs { > unsigned int secure:1; > /* Memory access is usermode (unprivileged) */ > unsigned int user:1; > + /* > + * Bus memory access permission. > + * > + * Some devices (such DMA) might be restricted to only access > + * some type of device, such RAM devices. By default memory > + * accesses are unspecified (MEMTXPERM_UNSPECIFIED), but could be > + * unrestricted (MEMTXPERM_UNRESTRICTED, similar to an allow list) > + * or restricted to a type of devices (similar to a deny list). > + * Currently only RAM devices can be restricted (MEMTXPERM_RAM_DEVICE). I don't understand these 3 categories. MEMTXPERM_UNSPECIFIED means any MemoryRegion can be accessed? What does MEMTXPERM_UNRESTRICTED mean? How does this differ from MEMTXPERM_UNSPECIFIED? What exactly does MEMTXPERM_RAM_DEVICE mean? Maybe that only MemoryRegions where memory_region_is_ram() is true can be accessed?
On 8/24/21 15:08, Stefan Hajnoczi wrote: > On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote: >> Add the 'direct_access' bit to the memory attributes to restrict >> bus master access to ROM/RAM. >> Have read/write accessors return MEMTX_BUS_ERROR if an access is >> restricted and the region is not ROM/RAM ('direct'). >> Add corresponding trace events. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/exec/memattrs.h | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h >> index 95f2d20d55b..7a94ee75a88 100644 >> --- a/include/exec/memattrs.h >> +++ b/include/exec/memattrs.h >> @@ -14,6 +14,13 @@ >> #ifndef MEMATTRS_H >> #define MEMATTRS_H >> >> +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */ >> +enum { >> + MEMTXPERM_UNSPECIFIED = 0, >> + MEMTXPERM_UNRESTRICTED = 1, >> + MEMTXPERM_RAM_DEVICE = 2, >> +}; >> + >> /* Every memory transaction has associated with it a set of >> * attributes. Some of these are generic (such as the ID of >> * the bus master); some are specific to a particular kind of >> @@ -35,6 +42,19 @@ typedef struct MemTxAttrs { >> unsigned int secure:1; >> /* Memory access is usermode (unprivileged) */ >> unsigned int user:1; >> + /* >> + * Bus memory access permission. >> + * >> + * Some devices (such DMA) might be restricted to only access >> + * some type of device, such RAM devices. By default memory >> + * accesses are unspecified (MEMTXPERM_UNSPECIFIED), but could be >> + * unrestricted (MEMTXPERM_UNRESTRICTED, similar to an allow list) >> + * or restricted to a type of devices (similar to a deny list). >> + * Currently only RAM devices can be restricted (MEMTXPERM_RAM_DEVICE). > > I don't understand these 3 categories. > > MEMTXPERM_UNSPECIFIED means any MemoryRegion can be accessed? MEMTXPERM_UNSPECIFIED means no change in the current behavior. IOW we haven't reviewed the device, and don't know whether it has to be restricted or not. > What does MEMTXPERM_UNRESTRICTED mean? How does this differ from > MEMTXPERM_UNSPECIFIED? We set MEMTXPERM_UNRESTRICTED when we reviewed a device and are sure it can be accessed any region (even being re-entrant on itself). I understand it like connected via a dual-port on the bus, and allowing read *and* write accesses at the same time... So the device is allowed to access itself, i.e. it can re-program itself in loop and so on. IIUC this is a requested feature for this API. > What exactly does MEMTXPERM_RAM_DEVICE mean? Maybe that only > MemoryRegions where memory_region_is_ram() is true can be accessed? No, it means, while we don't know which bus owner will access the device, the device itself can only access RAMd regions on the bus. I added MEMTXPERM_RAM_DEVICE as the first example, thinking it is the more generic use case. But maybe it is too generic and unuseful, and we need a real bus permission matrix?
On 8/23/21 21:04, David Hildenbrand wrote: > On 23.08.21 20:41, Peter Xu wrote: >> On Mon, Aug 23, 2021 at 06:41:55PM +0200, Philippe Mathieu-Daudé wrote: >>> +/* Permission to restrict bus memory accesses. See >>> MemTxAttrs::bus_perm */ >>> +enum { >>> + MEMTXPERM_UNSPECIFIED = 0, >>> + MEMTXPERM_UNRESTRICTED = 1, >>> + MEMTXPERM_RAM_DEVICE = 2, >>> +}; >> >> Is there a difference between UNSPECIFIED and UNRESTRICTED? >> >> If no, should we merge them? >> > > I'd assume MEMTXPERM_UNSPECIFIED has to be treated like > MEMTXPERM_UNRESTRICTED, so I'd also think we should just squash them. For now they are treated the same way, but ideally we should explicitly classify bus accesses and remove the MEMTXPERM_UNSPECIFIED. While we can use the same definition with comments, I think having different definitions ease maintainance (thinking of git-grep), but if we know we will never classify/convert the devices, then indeed having MEMTXPERM_UNSPECIFIED is pointless and confusing.
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h index 95f2d20d55b..7a94ee75a88 100644 --- a/include/exec/memattrs.h +++ b/include/exec/memattrs.h @@ -14,6 +14,13 @@ #ifndef MEMATTRS_H #define MEMATTRS_H +/* Permission to restrict bus memory accesses. See MemTxAttrs::bus_perm */ +enum { + MEMTXPERM_UNSPECIFIED = 0, + MEMTXPERM_UNRESTRICTED = 1, + MEMTXPERM_RAM_DEVICE = 2, +}; + /* Every memory transaction has associated with it a set of * attributes. Some of these are generic (such as the ID of * the bus master); some are specific to a particular kind of @@ -35,6 +42,19 @@ typedef struct MemTxAttrs { unsigned int secure:1; /* Memory access is usermode (unprivileged) */ unsigned int user:1; + /* + * Bus memory access permission. + * + * Some devices (such DMA) might be restricted to only access + * some type of device, such RAM devices. By default memory + * accesses are unspecified (MEMTXPERM_UNSPECIFIED), but could be + * unrestricted (MEMTXPERM_UNRESTRICTED, similar to an allow list) + * or restricted to a type of devices (similar to a deny list). + * Currently only RAM devices can be restricted (MEMTXPERM_RAM_DEVICE). + * + * Memory accesses to restricted addresses return MEMTX_BUS_ERROR. + */ + unsigned int bus_perm:2; /* Requester ID (for MSI for example) */ unsigned int requester_id:16; /* Invert endianness for this page */ @@ -66,6 +86,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
Add the 'direct_access' bit to the memory attributes to restrict bus master access to ROM/RAM. Have read/write accessors return MEMTX_BUS_ERROR if an access is restricted and the region is not ROM/RAM ('direct'). Add corresponding trace events. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/exec/memattrs.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)