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

[Peter Maydell]

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