Re: [PATCH v3 08/15] hw/intc/gic: use MxTxAttrs to divine accessing CPU

[Richard Henderson <richard.henderson@linaro.org>]

On 9/27/22 07:14, Alex Bennée wrote:
> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
> This solves edge cases like accessing via gdbstub or qtest. As we
> should only be processing accesses from CPU cores we can push the CPU
> extraction logic out to the main access functions. If the access does
> not come from a CPU we log it and fail the transaction with
> MEMTX_ACCESS_ERROR.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
> 
> ---
> v2
>    - update for new field
>    - bool asserts
> v3
>    - fail non-CPU transactions
> ---
>   hw/intc/arm_gic.c | 174 +++++++++++++++++++++++++++++++---------------
>   1 file changed, 118 insertions(+), 56 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 492b2421ab..7b4f3fb81a 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -56,17 +56,42 @@ static const uint8_t gic_id_gicv2[] = {
>       0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>   };
>   
> -static inline int gic_get_current_cpu(GICState *s)
> +
> +/*
> + * The GIC should only be accessed by the CPU so if it is not we
> + * should fail the transaction (it would either be a bug in how we've
> + * wired stuff up, a limitation of the translator or the guest doing
> + * something weird like programming a DMA master to write to the MMIO
> + * region).
> + *
> + * Note the cpu_index is global and we currently don't have any models
> + * with multiple SoC's with different CPUs. However if we did we would
> + * need to transform the cpu_index into the socket core.
> + */
> +typedef struct {
> +    bool valid;
> +    int cpu_index;
> +} GicCPU;
> +
> +static inline GicCPU gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
>   {
> -    if (!qtest_enabled() && s->num_cpu > 1) {
> -        return current_cpu->cpu_index;
> +    if (attrs.requester_type != MTRT_CPU) {
> +        qemu_log_mask(LOG_UNIMP | LOG_GUEST_ERROR,
> +                      "%s: saw non-CPU transaction", __func__);
> +        return (GicCPU) { .valid = false };
>       }
> -    return 0;
> +    g_assert(attrs.requester_id < s->num_cpu);
> +
> +    return (GicCPU) { .valid = true, .cpu_index = attrs.requester_id };
>   }

I think you should split this function, and do away with the struct.

>   static MemTxResult gic_dist_read(void *opaque, hwaddr offset, uint64_t *data,
>                                    unsigned size, MemTxAttrs attrs)
>   {
> +    GICState *s = (GICState *)opaque;
> +    GicCPU cpu = gic_get_current_cpu(s, attrs);
> +
> +    if (!cpu.valid) {
> +        return MEMTX_ACCESS_ERROR;
> +    }

This would become

     if (!gic_valid_cpu(attrs)) {
         return MEMTX_ACCESS_ERROR;
     }
     cpu = gic_get_cpu(attrs);



r~