Re: [Qemu-devel] [PATCH v9 06/14] hw/arm/smmuv3: Queue helpers

[Auger Eric]

Hi Peter,

On 08/03/18 19:28, Peter Maydell wrote:
> On 17 February 2018 at 18:46, Eric Auger  wrote:
>> We introduce helpers to read/write into the command and event
>> circular queues.
>>
>> smmuv3_write_eventq and smmuv3_cmq_consume will become static
>> in subsequent patches.
>>
>> Invalidation commands are not yet dealt with. We do not cache
>> data that need to be invalidated. This will change with vhost
>> integration.
>>
>> Signed-off-by: Eric Auger 
>>
>> ---
>>
>> v8 -> v9:
>> - fix CMD_SSID & CMD_ADDR + some renamings
>> - do cons increment after the execution of the command
>> - add Q_INCONSISTENT()
>>
>> v7 -> v8
>> - use address_space_rw
>> - helpers inspired from spec
>> ---
>>  hw/arm/smmuv3-internal.h | 150 +++
>>  hw/arm/smmuv3.c  | 162 
>> +++
>>  hw/arm/trace-events  |   4 ++
>>  3 files changed, 316 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index 40b39a1..c0771ce 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -162,4 +162,154 @@ static inline uint64_t smmu_read64(uint64_t r, 
>> unsigned offset,
>>  void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask);
>>  void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t gerrorn);
>>
>> +/* Queue Handling */
>> +
>> +#define LOG2SIZE(q)extract64((q)->base, 0, 5)
>> +#define BASE(q)((q)->base & SMMU_BASE_ADDR_MASK)
renamed Q_LOG2SIZE and Q_BASE respectively
> 
> These are both very generic names for things in header files.
> 
> Looking at the required behaviour of the *_BASE registers,
> if the LOG2SIZE field is written to a value larger than the maximum,
> it is supposed to read back as the value written but must behave
> as if it was set to the maximum. That suggests to me that your
> SMMUQueue struct should have a "log2size" field which is set when
> the guest writes to *_BASE (which is where you can cap it to the
> max value).
done
> 
>> +#define WRAP_MASK(q)   (1 << LOG2SIZE(q))
>> +#define INDEX_MASK(q)  ((1 << LOG2SIZE(q)) - 1)
>> +#define WRAP_INDEX_MASK(q) ((1 << (LOG2SIZE(q) + 1)) - 1)
> 
> WRAP_INDEX_MASK is unused (but see below for a possible use)
kept and adopted your suggestions below
> 
>> +
>> +#define Q_CONS_ENTRY(q)  (BASE(q) + \
>> +  (q)->entry_size * ((q)->cons & INDEX_MASK(q)))
>> +#define Q_PROD_ENTRY(q)  (BASE(q) + \
>> +  (q)->entry_size * ((q)->prod & INDEX_MASK(q)))
>> +
>> +#define Q_CONS(q) ((q)->cons & INDEX_MASK(q))
>> +#define Q_PROD(q) ((q)->prod & INDEX_MASK(q))
> 
> If you put these a bit earlier you can use them in the definitions
> of Q_CONS_ENTRY and Q_PROD_ENTRY.
done
> 
>> +
>> +#define Q_CONS_WRAP(q) (((q)->cons & WRAP_MASK(q)) >> LOG2SIZE(q))
>> +#define Q_PROD_WRAP(q) (((q)->prod & WRAP_MASK(q)) >> LOG2SIZE(q))
>> +
>> +#define Q_FULL(q) \
>> +(q)->cons) & INDEX_MASK(q)) == \
>> +  (((q)->prod) & INDEX_MASK(q))) && \
>> + q)->cons) & WRAP_MASK(q)) != \
>> +  (((q)->prod) & WRAP_MASK(q
> 
> You could write this as
>((cons ^ prod) & WRAP_INDEX_MASK) == WRAP_MASK
done
> 
>> +
>> +#define Q_EMPTY(q) \
>> +(q)->cons) & INDEX_MASK(q)) == \
>> +  (((q)->prod) & INDEX_MASK(q))) && \
>> + q)->cons) & WRAP_MASK(q)) == \
>> +  (((q)->prod) & WRAP_MASK(q
> 
> and this as
>(cons & WRAP_INDEX_MASK) == (prod & WRAP_INDEX_MASK)
done
> 
> (or as ((cons ^ prod) & WRAP_INDEX_MASK) == 0, but that's unnecessarily
> obscure I think.)
> 
> 
> This is all a bit macro-heavy. Do these really all need to be macros
> rather than functions?
> 
>> +
>> +#define Q_INCONSISTENT(q) \
>> +((q)->prod) & INDEX_MASK(q)) > (((q)->cons) & INDEX_MASK(q))) && \
>> +q)->prod) & WRAP_MASK(q)) != (((q)->cons) & WRAP_MASK(q || \
>> +(q)->prod) & INDEX_MASK(q)) < (((q)->cons) & INDEX_MASK(q))) && \
>> +q)->prod) & WRAP_MASK(q)) == (((q)->cons) & WRAP_MASK(q) \
>> +
> 
> This never seems to be used. (Also it has a stray trailing '\',
> and isn't indented very clearly.
removed
> 
>> +#define SMMUV3_CMDQ_ENABLED(s) \
>> + (FIELD_EX32(s->cr[0], CR0, CMDQEN))
>> +
>> +#define SMMUV3_EVENTQ_ENABLED(s) \
>> + (FIELD_EX32(s->cr[0], CR0, EVENTQEN))

Those were moved to static inline functions
>> +
>> +static inline void smmu_write_cmdq_err(SMMUv3State *s, uint32_t err_type)
>> +{
>> +s->cmdq.cons = FIELD_DP32(s->cmdq.cons, CMDQ_CONS, ERR, err_type);
>> +}
>> +
>> +void smmuv3_write_eventq(SMMUv3State *s, Evt *evt);
>> +
>> +/* Commands */
>> +
>> +enum {
>> +SMMU_CMD_PREFETCH_CONFIG = 0x01,
>> +SMMU_CMD_PREFETCH_ADDR,
>> +SMMU_CMD_CFGI_STE,
>> +SMMU_CMD_CFGI_STE_RANGE,
>> +SMMU_CMD_CFGI_CD,
>> +SMMU_CMD_CFGI_CD_ALL,
>> +SMMU_CMD_CFGI_ALL,
>> +SMMU_CMD_TLBI_NH_ALL = 0x10,
>> +SMMU_CMD_TLBI_NH_ASID,
>> +

Re: [Qemu-devel] [PATCH v9 06/14] hw/arm/smmuv3: Queue helpers

[Peter Maydell]

On 17 February 2018 at 18:46, Eric Auger  wrote:
> We introduce helpers to read/write into the command and event
> circular queues.
>
> smmuv3_write_eventq and smmuv3_cmq_consume will become static
> in subsequent patches.
>
> Invalidation commands are not yet dealt with. We do not cache
> data that need to be invalidated. This will change with vhost
> integration.
>
> Signed-off-by: Eric Auger 
>
> ---
>
> v8 -> v9:
> - fix CMD_SSID & CMD_ADDR + some renamings
> - do cons increment after the execution of the command
> - add Q_INCONSISTENT()
>
> v7 -> v8
> - use address_space_rw
> - helpers inspired from spec
> ---
>  hw/arm/smmuv3-internal.h | 150 +++
>  hw/arm/smmuv3.c  | 162 
> +++
>  hw/arm/trace-events  |   4 ++
>  3 files changed, 316 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 40b39a1..c0771ce 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -162,4 +162,154 @@ static inline uint64_t smmu_read64(uint64_t r, unsigned 
> offset,
>  void smmuv3_trigger_irq(SMMUv3State *s, SMMUIrq irq, uint32_t gerror_mask);
>  void smmuv3_write_gerrorn(SMMUv3State *s, uint32_t gerrorn);
>
> +/* Queue Handling */
> +
> +#define LOG2SIZE(q)extract64((q)->base, 0, 5)
> +#define BASE(q)((q)->base & SMMU_BASE_ADDR_MASK)

These are both very generic names for things in header files.

Looking at the required behaviour of the *_BASE registers,
if the LOG2SIZE field is written to a value larger than the maximum,
it is supposed to read back as the value written but must behave
as if it was set to the maximum. That suggests to me that your
SMMUQueue struct should have a "log2size" field which is set when
the guest writes to *_BASE (which is where you can cap it to the
max value).

> +#define WRAP_MASK(q)   (1 << LOG2SIZE(q))
> +#define INDEX_MASK(q)  ((1 << LOG2SIZE(q)) - 1)
> +#define WRAP_INDEX_MASK(q) ((1 << (LOG2SIZE(q) + 1)) - 1)

WRAP_INDEX_MASK is unused (but see below for a possible use)

> +
> +#define Q_CONS_ENTRY(q)  (BASE(q) + \
> +  (q)->entry_size * ((q)->cons & INDEX_MASK(q)))
> +#define Q_PROD_ENTRY(q)  (BASE(q) + \
> +  (q)->entry_size * ((q)->prod & INDEX_MASK(q)))
> +
> +#define Q_CONS(q) ((q)->cons & INDEX_MASK(q))
> +#define Q_PROD(q) ((q)->prod & INDEX_MASK(q))

If you put these a bit earlier you can use them in the definitions
of Q_CONS_ENTRY and Q_PROD_ENTRY.

> +
> +#define Q_CONS_WRAP(q) (((q)->cons & WRAP_MASK(q)) >> LOG2SIZE(q))
> +#define Q_PROD_WRAP(q) (((q)->prod & WRAP_MASK(q)) >> LOG2SIZE(q))
> +
> +#define Q_FULL(q) \
> +(q)->cons) & INDEX_MASK(q)) == \
> +  (((q)->prod) & INDEX_MASK(q))) && \
> + q)->cons) & WRAP_MASK(q)) != \
> +  (((q)->prod) & WRAP_MASK(q

You could write this as
   ((cons ^ prod) & WRAP_INDEX_MASK) == WRAP_MASK

> +
> +#define Q_EMPTY(q) \
> +(q)->cons) & INDEX_MASK(q)) == \
> +  (((q)->prod) & INDEX_MASK(q))) && \
> + q)->cons) & WRAP_MASK(q)) == \
> +  (((q)->prod) & WRAP_MASK(q

and this as
   (cons & WRAP_INDEX_MASK) == (prod & WRAP_INDEX_MASK)

(or as ((cons ^ prod) & WRAP_INDEX_MASK) == 0, but that's unnecessarily
obscure I think.)


This is all a bit macro-heavy. Do these really all need to be macros
rather than functions?

> +
> +#define Q_INCONSISTENT(q) \
> +((q)->prod) & INDEX_MASK(q)) > (((q)->cons) & INDEX_MASK(q))) && \
> +q)->prod) & WRAP_MASK(q)) != (((q)->cons) & WRAP_MASK(q || \
> +(q)->prod) & INDEX_MASK(q)) < (((q)->cons) & INDEX_MASK(q))) && \
> +q)->prod) & WRAP_MASK(q)) == (((q)->cons) & WRAP_MASK(q) \
> +

This never seems to be used. (Also it has a stray trailing '\',
and isn't indented very clearly.

> +#define SMMUV3_CMDQ_ENABLED(s) \
> + (FIELD_EX32(s->cr[0], CR0, CMDQEN))
> +
> +#define SMMUV3_EVENTQ_ENABLED(s) \
> + (FIELD_EX32(s->cr[0], CR0, EVENTQEN))
> +
> +static inline void smmu_write_cmdq_err(SMMUv3State *s, uint32_t err_type)
> +{
> +s->cmdq.cons = FIELD_DP32(s->cmdq.cons, CMDQ_CONS, ERR, err_type);
> +}
> +
> +void smmuv3_write_eventq(SMMUv3State *s, Evt *evt);
> +
> +/* Commands */
> +
> +enum {
> +SMMU_CMD_PREFETCH_CONFIG = 0x01,
> +SMMU_CMD_PREFETCH_ADDR,
> +SMMU_CMD_CFGI_STE,
> +SMMU_CMD_CFGI_STE_RANGE,
> +SMMU_CMD_CFGI_CD,
> +SMMU_CMD_CFGI_CD_ALL,
> +SMMU_CMD_CFGI_ALL,
> +SMMU_CMD_TLBI_NH_ALL = 0x10,
> +SMMU_CMD_TLBI_NH_ASID,
> +SMMU_CMD_TLBI_NH_VA,
> +SMMU_CMD_TLBI_NH_VAA,
> +SMMU_CMD_TLBI_EL3_ALL= 0x18,
> +SMMU_CMD_TLBI_EL3_VA = 0x1a,
> +SMMU_CMD_TLBI_EL2_ALL= 0x20,
> +SMMU_CMD_TLBI_EL2_ASID,
> +SMMU_CMD_TLBI_EL2_VA,
> +SMMU_CMD_TLBI_EL2_VAA,  /* 0x23 */
> +SMMU_CMD_TLBI_S12_VMALL  = 0x28,
> +SMMU_CMD_TLBI_S2_IPA = 0x2a,
> +SMMU_CMD_TLBI_NSNH_ALL   =