diff mbox series

[v3,01/15] hw: encode accessing CPU index in MemTxAttrs

Message ID 20220927141504.3886314-2-alex.bennee@linaro.org
State New
Headers show
Series gdbstub/next (MemTxAttrs, re-factoring) | expand

Commit Message

Alex Bennée Sept. 27, 2022, 2:14 p.m. UTC
We currently have hacks across the hw/ to reference current_cpu to
work out what the current accessing CPU is. This breaks in some cases
including using gdbstub to access HW state. As we have MemTxAttrs to
describe details about the access lets extend it so CPU accesses can
be explicitly marked.

To achieve this we create a new requester_type which indicates to
consumers how requester_id it to be consumed. We absorb the existing
unspecified:1 bitfield into this type and also document a potential
machine specific encoding which will be useful to (currently)
out-of-tree extensions.

There are a number of places we need to fix up including:

  CPU helpers directly calling address_space_*() fns
  models in hw/ fishing the data out of current_cpu
  hypervisors offloading device emulation to QEMU

I'll start addressing some of these in following patches.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use separate field cpu_index
  - bool for requester_is_cpu
v3
  - switch to enum MemTxRequesterType
  - move helper #define to patch
  - revert to overloading requester_id
  - mention hypervisors in commit message
  - drop cputlb tweaks, they will move to target specific code
v4
  - merge unspecified:1 into MTRT_UNSPECIFIED
  - document a MTRT_MACHINE for more complex encoding
  - ensure existing users of requester_id set MTRT_PCI
  - ensure existing consumers of requester_id check type is MTRT_PCI
  - have MEMTXATTRS_CPU take CPUState * directly
---
 include/exec/memattrs.h | 39 +++++++++++++++++++++++++++++++--------
 hw/i386/amd_iommu.c     |  3 ++-
 hw/i386/intel_iommu.c   |  2 +-
 hw/misc/tz-mpc.c        |  2 +-
 hw/misc/tz-msc.c        |  8 ++++----
 hw/pci/pci.c            |  7 ++++---
 6 files changed, 43 insertions(+), 18 deletions(-)

Comments

Richard Henderson Sept. 28, 2022, 4:42 p.m. UTC | #1
On 9/27/22 07:14, Alex Bennée wrote:
> +/*
> + * Every memory transaction comes from a specific place which defines
> + * how requester_id should be handled. For CPU's the requester_id is
> + * the global cpu_index which needs further processing if you need to
> + * work out which socket or complex it comes from. UNSPECIFIED is the
> + * default for otherwise undefined MemTxAttrs. PCI indicates the
> + * requester_id is a PCI id. MACHINE indicates a machine specific
> + * encoding which needs further processing to decode into its
> + * constituent parts.
> + */
> +typedef enum MemTxRequesterType {
> +    MTRT_CPU = 0,
> +    MTRT_UNSPECIFIED,
> +    MTRT_PCI,
> +    MTRT_MACHINE
> +} MemTxRequesterType;


I wonder if UNSPECIFIED should be zero, or even ILLEGAL, attempting to catch MemTxAttrs 
foo = { }, which is now ill-formed as it doesn't initialize .requester_id to match CPU.

Perhaps a preceeding patch, should introduce

#define MEMTXATTRS_CPU(cpu)  ((MemTxAttrs){ })

and modify all uses of "= { }", at least under target/.

> --- a/hw/misc/tz-msc.c
> +++ b/hw/misc/tz-msc.c
> @@ -137,11 +137,11 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
>           return MEMTX_OK;
>       case MSCAllowSecure:
>           attrs.secure = 1;
> -        attrs.unspecified = 0;
> +        attrs.requester_type = MTRT_CPU;
>           break;
>       case MSCAllowNonSecure:
>           attrs.secure = 0;
> -        attrs.unspecified = 0;
> +        attrs.requester_type = MTRT_CPU;
>           break;

This is surely incomplete.  You can't just set "cpu" without saying where it's from.

Since this device is only used by the ARMSSE machine, I would hope that attrs.unspecified 
should never be set before the patch, and thus MTRT_CPU should be set afterward.


r~
Peter Maydell Sept. 28, 2022, 6:56 p.m. UTC | #2
On Wed, 28 Sept 2022 at 17:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/27/22 07:14, Alex Bennée wrote:
> > --- a/hw/misc/tz-msc.c
> > +++ b/hw/misc/tz-msc.c
> > @@ -137,11 +137,11 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
> >           return MEMTX_OK;
> >       case MSCAllowSecure:
> >           attrs.secure = 1;
> > -        attrs.unspecified = 0;
> > +        attrs.requester_type = MTRT_CPU;
> >           break;
> >       case MSCAllowNonSecure:
> >           attrs.secure = 0;
> > -        attrs.unspecified = 0;
> > +        attrs.requester_type = MTRT_CPU;
> >           break;
>
> This is surely incomplete.  You can't just set "cpu" without saying where it's from.
>
> Since this device is only used by the ARMSSE machine, I would hope that attrs.unspecified
> should never be set before the patch, and thus MTRT_CPU should be set afterward.

The MSC is in the address map like most other stuff, and thus there is
no restriction on whether it can be accessed by other things than CPUs
(DMAing to it would be silly but is perfectly possible).

The intent of the code is "pass this transaction through, but force
it to be Secure/NonSecure regardless of what it was before". That
should not involve a change of the requester type.

thanks
-- PMM
Alex Bennée Oct. 4, 2022, 1:32 p.m. UTC | #3
Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 28 Sept 2022 at 17:42, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 9/27/22 07:14, Alex Bennée wrote:
>> > --- a/hw/misc/tz-msc.c
>> > +++ b/hw/misc/tz-msc.c
>> > @@ -137,11 +137,11 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
>> >           return MEMTX_OK;
>> >       case MSCAllowSecure:
>> >           attrs.secure = 1;
>> > -        attrs.unspecified = 0;
>> > +        attrs.requester_type = MTRT_CPU;
>> >           break;
>> >       case MSCAllowNonSecure:
>> >           attrs.secure = 0;
>> > -        attrs.unspecified = 0;
>> > +        attrs.requester_type = MTRT_CPU;
>> >           break;
>>
>> This is surely incomplete.  You can't just set "cpu" without saying where it's from.
>>
>> Since this device is only used by the ARMSSE machine, I would hope that attrs.unspecified
>> should never be set before the patch, and thus MTRT_CPU should be set afterward.
>
> The MSC is in the address map like most other stuff, and thus there is
> no restriction on whether it can be accessed by other things than CPUs
> (DMAing to it would be silly but is perfectly possible).
>
> The intent of the code is "pass this transaction through, but force
> it to be Secure/NonSecure regardless of what it was before". That
> should not involve a change of the requester type.

Should we assert (or warn) when the requester_type is unspecified?

>
> thanks
> -- PMM
Peter Maydell Oct. 4, 2022, 2:54 p.m. UTC | #4
On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > The MSC is in the address map like most other stuff, and thus there is
> > no restriction on whether it can be accessed by other things than CPUs
> > (DMAing to it would be silly but is perfectly possible).
> >
> > The intent of the code is "pass this transaction through, but force
> > it to be Secure/NonSecure regardless of what it was before". That
> > should not involve a change of the requester type.
>
> Should we assert (or warn) when the requester_type is unspecified?

Not in the design of MemTxAttrs that's currently in git, no:
in that design it's perfectly fine for something generating
memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
to meaning a bunch of things including "not secure").

thanks
-- PMM
Philippe Mathieu-Daudé Oct. 31, 2022, 12:08 p.m. UTC | #5
On 4/10/22 16:54, Peter Maydell wrote:
> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> The MSC is in the address map like most other stuff, and thus there is
>>> no restriction on whether it can be accessed by other things than CPUs
>>> (DMAing to it would be silly but is perfectly possible).
>>>
>>> The intent of the code is "pass this transaction through, but force
>>> it to be Secure/NonSecure regardless of what it was before". That
>>> should not involve a change of the requester type.
>>
>> Should we assert (or warn) when the requester_type is unspecified?
> 
> Not in the design of MemTxAttrs that's currently in git, no:
> in that design it's perfectly fine for something generating
> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
> to meaning a bunch of things including "not secure").

In tz_mpc_handle_block():

static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs 
attrs)
{
     /* Handle a blocked transaction: raise IRQ, capture info, etc */
     if (!s->int_stat) {

         s->int_info1 = addr;
         s->int_info2 = 0;
         s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
                                   attrs.requester_id & 0xffff);
         s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
                                   ~attrs.secure);
         s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
                                   tz_mpc_cfg_ns(s, addr));


Should we check whether the requester is MTRT_CPU?
Peter Maydell Oct. 31, 2022, 1:03 p.m. UTC | #6
On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 4/10/22 16:54, Peter Maydell wrote:
> > On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >>
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>> The MSC is in the address map like most other stuff, and thus there is
> >>> no restriction on whether it can be accessed by other things than CPUs
> >>> (DMAing to it would be silly but is perfectly possible).
> >>>
> >>> The intent of the code is "pass this transaction through, but force
> >>> it to be Secure/NonSecure regardless of what it was before". That
> >>> should not involve a change of the requester type.
> >>
> >> Should we assert (or warn) when the requester_type is unspecified?
> >
> > Not in the design of MemTxAttrs that's currently in git, no:
> > in that design it's perfectly fine for something generating
> > memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
> > to meaning a bunch of things including "not secure").
>
> In tz_mpc_handle_block():
>
> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
> attrs)
> {
>      /* Handle a blocked transaction: raise IRQ, capture info, etc */
>      if (!s->int_stat) {
>
>          s->int_info1 = addr;
>          s->int_info2 = 0;
>          s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
>                                    attrs.requester_id & 0xffff);
>          s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
>                                    ~attrs.secure);
>          s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
>                                    tz_mpc_cfg_ns(s, addr));
>
>
> Should we check whether the requester is MTRT_CPU?

That code is basically assuming that the requester_id is the AMBA AHB
'HMASTER' field (i.e. something hopefully unique to all things that
send out transactions, not necessarily limited only to CPUs), which is a
somewhat bogus assumption given that it isn't currently any such thing...

I'm not sure if/how this patchset plans to model generic "ID of transaction
generator".

-- PMM
Philippe Mathieu-Daudé Nov. 11, 2022, 1:23 p.m. UTC | #7
On 31/10/22 14:03, Peter Maydell wrote:
> On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 4/10/22 16:54, Peter Maydell wrote:
>>> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>>
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>> The MSC is in the address map like most other stuff, and thus there is
>>>>> no restriction on whether it can be accessed by other things than CPUs
>>>>> (DMAing to it would be silly but is perfectly possible).
>>>>>
>>>>> The intent of the code is "pass this transaction through, but force
>>>>> it to be Secure/NonSecure regardless of what it was before". That
>>>>> should not involve a change of the requester type.
>>>>
>>>> Should we assert (or warn) when the requester_type is unspecified?
>>>
>>> Not in the design of MemTxAttrs that's currently in git, no:
>>> in that design it's perfectly fine for something generating
>>> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
>>> to meaning a bunch of things including "not secure").
>>
>> In tz_mpc_handle_block():
>>
>> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
>> attrs)
>> {
>>       /* Handle a blocked transaction: raise IRQ, capture info, etc */
>>       if (!s->int_stat) {
>>
>>           s->int_info1 = addr;
>>           s->int_info2 = 0;
>>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
>>                                     attrs.requester_id & 0xffff);
>>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
>>                                     ~attrs.secure);
>>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
>>                                     tz_mpc_cfg_ns(s, addr));
>>
>>
>> Should we check whether the requester is MTRT_CPU?
> 
> That code is basically assuming that the requester_id is the AMBA AHB
> 'HMASTER' field (i.e. something hopefully unique to all things that
> send out transactions, not necessarily limited only to CPUs), which is a
> somewhat bogus assumption given that it isn't currently any such thing...
> 
> I'm not sure if/how this patchset plans to model generic "ID of transaction
> generator".

Does your 'generic "ID of transaction generator"' fit into MTRT_MACHINE 
described as "for more complex encoding":

   'MACHINE indicates a machine specific encoding which needs further
    processing to decode into its constituent parts.'

?

> 
> -- PMM
Alex Bennée Nov. 11, 2022, 1:58 p.m. UTC | #8
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 31/10/22 14:03, Peter Maydell wrote:
>> On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> On 4/10/22 16:54, Peter Maydell wrote:
>>>> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>
>>>>>
>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>> The MSC is in the address map like most other stuff, and thus there is
>>>>>> no restriction on whether it can be accessed by other things than CPUs
>>>>>> (DMAing to it would be silly but is perfectly possible).
>>>>>>
>>>>>> The intent of the code is "pass this transaction through, but force
>>>>>> it to be Secure/NonSecure regardless of what it was before". That
>>>>>> should not involve a change of the requester type.
>>>>>
>>>>> Should we assert (or warn) when the requester_type is unspecified?
>>>>
>>>> Not in the design of MemTxAttrs that's currently in git, no:
>>>> in that design it's perfectly fine for something generating
>>>> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
>>>> to meaning a bunch of things including "not secure").
>>>
>>> In tz_mpc_handle_block():
>>>
>>> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
>>> attrs)
>>> {
>>>       /* Handle a blocked transaction: raise IRQ, capture info, etc */
>>>       if (!s->int_stat) {
>>>
>>>           s->int_info1 = addr;
>>>           s->int_info2 = 0;
>>>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
>>>                                     attrs.requester_id & 0xffff);
>>>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
>>>                                     ~attrs.secure);
>>>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
>>>                                     tz_mpc_cfg_ns(s, addr));
>>>
>>>
>>> Should we check whether the requester is MTRT_CPU?
>> That code is basically assuming that the requester_id is the AMBA
>> AHB
>> 'HMASTER' field (i.e. something hopefully unique to all things that
>> send out transactions, not necessarily limited only to CPUs), which is a
>> somewhat bogus assumption given that it isn't currently any such thing...
>> I'm not sure if/how this patchset plans to model generic "ID of
>> transaction
>> generator".
>
> Does your 'generic "ID of transaction generator"' fit into
> MTRT_MACHINE described as "for more complex encoding":
>
>   'MACHINE indicates a machine specific encoding which needs further
>    processing to decode into its constituent parts.'
>
> ?

Yes - I've just done something similar to model the IOAPIC on x86.
Currently that uses a magic number of requester_id that is unique to the
"machine bus" but it could multiplex multiple bits of data on more
complex topologies.

I'll post v5 soon now I have x86 working.
Peter Maydell Nov. 14, 2022, 10:06 a.m. UTC | #9
On Fri, 11 Nov 2022 at 13:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 31/10/22 14:03, Peter Maydell wrote:
> > On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> On 4/10/22 16:54, Peter Maydell wrote:
> >>> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>>>
> >>>>
> >>>> Peter Maydell <peter.maydell@linaro.org> writes:
> >>>>> The MSC is in the address map like most other stuff, and thus there is
> >>>>> no restriction on whether it can be accessed by other things than CPUs
> >>>>> (DMAing to it would be silly but is perfectly possible).
> >>>>>
> >>>>> The intent of the code is "pass this transaction through, but force
> >>>>> it to be Secure/NonSecure regardless of what it was before". That
> >>>>> should not involve a change of the requester type.
> >>>>
> >>>> Should we assert (or warn) when the requester_type is unspecified?
> >>>
> >>> Not in the design of MemTxAttrs that's currently in git, no:
> >>> in that design it's perfectly fine for something generating
> >>> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
> >>> to meaning a bunch of things including "not secure").
> >>
> >> In tz_mpc_handle_block():
> >>
> >> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
> >> attrs)
> >> {
> >>       /* Handle a blocked transaction: raise IRQ, capture info, etc */
> >>       if (!s->int_stat) {
> >>
> >>           s->int_info1 = addr;
> >>           s->int_info2 = 0;
> >>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
> >>                                     attrs.requester_id & 0xffff);
> >>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
> >>                                     ~attrs.secure);
> >>           s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
> >>                                     tz_mpc_cfg_ns(s, addr));
> >>
> >>
> >> Should we check whether the requester is MTRT_CPU?
> >
> > That code is basically assuming that the requester_id is the AMBA AHB
> > 'HMASTER' field (i.e. something hopefully unique to all things that
> > send out transactions, not necessarily limited only to CPUs), which is a
> > somewhat bogus assumption given that it isn't currently any such thing...
> >
> > I'm not sure if/how this patchset plans to model generic "ID of transaction
> > generator".
>
> Does your 'generic "ID of transaction generator"' fit into MTRT_MACHINE
> described as "for more complex encoding":
>
>    'MACHINE indicates a machine specific encoding which needs further
>     processing to decode into its constituent parts.'
>
> ?

Not really, because "generic ID of a transaction generator" is a
superset of "ID per CPU", not something separate that sits
alongside it...

-- PMM
diff mbox series

Patch

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..67625c6344 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -14,6 +14,24 @@ 
 #ifndef MEMATTRS_H
 #define MEMATTRS_H
 
+/*
+ * Every memory transaction comes from a specific place which defines
+ * how requester_id should be handled. For CPU's the requester_id is
+ * the global cpu_index which needs further processing if you need to
+ * work out which socket or complex it comes from. UNSPECIFIED is the
+ * default for otherwise undefined MemTxAttrs. PCI indicates the
+ * requester_id is a PCI id. MACHINE indicates a machine specific
+ * encoding which needs further processing to decode into its
+ * constituent parts.
+ */
+typedef enum MemTxRequesterType {
+    MTRT_CPU = 0,
+    MTRT_UNSPECIFIED,
+    MTRT_PCI,
+    MTRT_MACHINE
+} MemTxRequesterType;
+
+
 /* 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
@@ -23,12 +41,6 @@ 
  * different semantics.
  */
 typedef struct MemTxAttrs {
-    /* Bus masters which don't specify any attributes will get this
-     * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
-     * distinguish "all attributes deliberately clear" from
-     * "didn't specify" if necessary.
-     */
-    unsigned int unspecified:1;
     /* ARM/AMBA: TrustZone Secure access
      * x86: System Management Mode access
      */
@@ -43,7 +55,9 @@  typedef struct MemTxAttrs {
      * (see MEMTX_ACCESS_ERROR).
      */
     unsigned int memory:1;
-    /* Requester ID (for MSI for example) */
+    /* Requester type (e.g. CPU or PCI MSI) */
+    MemTxRequesterType requester_type:2;
+    /* Requester ID */
     unsigned int requester_id:16;
     /* Invert endianness for this page */
     unsigned int byte_swap:1;
@@ -64,7 +78,16 @@  typedef struct MemTxAttrs {
  * (so that we can distinguish "all attributes deliberately clear"
  * from "didn't specify" if necessary).
  */
-#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
+#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) \
+                                { .requester_type = MTRT_UNSPECIFIED })
+
+/*
+ * Helper for setting a basic CPU sourced transaction, it expects a
+ * CPUState *
+ */
+#define MEMTXATTRS_CPU(cs) ((MemTxAttrs) \
+                            {.requester_type = MTRT_CPU, \
+                             .requester_id = cs->cpu_index})
 
 /* New-style MMIO accessors can indicate that the transaction failed.
  * A zero (MEMTX_OK) response means success; anything else is a failure
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 725f69095b..8db2b6b692 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -154,6 +154,7 @@  static void amdvi_generate_msi_interrupt(AMDVIState *s)
 {
     MSIMessage msg = {};
     MemTxAttrs attrs = {
+        .requester_type = MTRT_PCI,
         .requester_id = pci_requester_id(&s->pci.dev)
     };
 
@@ -1356,7 +1357,7 @@  static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
 
     trace_amdvi_mem_ir_write_req(addr, value, size);
 
-    if (!attrs.unspecified) {
+    if (attrs.requester_type == MTRT_PCI) {
         /* We have explicit Source ID */
         sid = attrs.requester_id;
     }
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 05d53a1aa9..89b9b9a3e6 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3394,7 +3394,7 @@  static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
     from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
     from.data = (uint32_t) value;
 
-    if (!attrs.unspecified) {
+    if (attrs.requester_type == MTRT_PCI) {
         /* We have explicit Source ID */
         sid = attrs.requester_id;
     }
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index 30481e1c90..4beb5daa1a 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -461,7 +461,7 @@  static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
      * All the real during-emulation transactions from the CPU will
      * specify attributes.
      */
-    return (attrs.unspecified || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
+    return ((attrs.requester_type == MTRT_UNSPECIFIED) || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
 }
 
 static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
diff --git a/hw/misc/tz-msc.c b/hw/misc/tz-msc.c
index acbe94400b..0b47972a46 100644
--- a/hw/misc/tz-msc.c
+++ b/hw/misc/tz-msc.c
@@ -137,11 +137,11 @@  static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
         return MEMTX_OK;
     case MSCAllowSecure:
         attrs.secure = 1;
-        attrs.unspecified = 0;
+        attrs.requester_type = MTRT_CPU;
         break;
     case MSCAllowNonSecure:
         attrs.secure = 0;
-        attrs.unspecified = 0;
+        attrs.requester_type = MTRT_CPU;
         break;
     }
 
@@ -179,11 +179,11 @@  static MemTxResult tz_msc_write(void *opaque, hwaddr addr, uint64_t val,
         return MEMTX_OK;
     case MSCAllowSecure:
         attrs.secure = 1;
-        attrs.unspecified = 0;
+        attrs.requester_type = MTRT_CPU;
         break;
     case MSCAllowNonSecure:
         attrs.secure = 0;
-        attrs.unspecified = 0;
+        attrs.requester_type = MTRT_CPU;
         break;
     }
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2f450f6a72..ccdd71e4ba 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -319,9 +319,10 @@  void pci_device_deassert_intx(PCIDevice *dev)
 
 static void pci_msi_trigger(PCIDevice *dev, MSIMessage msg)
 {
-    MemTxAttrs attrs = {};
-
-    attrs.requester_id = pci_requester_id(dev);
+    MemTxAttrs attrs = {
+        .requester_type = MTRT_PCI,
+        .requester_id = pci_requester_id(dev)
+    };
     address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
                          attrs, NULL);
 }