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

[Richard Henderson]

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~