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

[Alex Bennée]

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


-- 
Alex Bennée