Re: [Qemu-devel] [PATCH v11 08/17] hw/arm/smmuv3: Event queue recording helper
Hi Peter,
On 04/16/2018 06:51 PM, Peter Maydell wrote:
> On 12 April 2018 at 08:37, Eric Auger wrote:
>> Let's introduce a helper function aiming at recording an
>> event in the event queue.
>>
>> Signed-off-by: Eric Auger
>>
>> ---
>> v9 -> v10:
>> - rework SMMU_EVENT_STRING
>> - trigger a GERROR EVENTQ_ABT_ERR in case of eventq write failure
>>
>> v8 -> v9:
>> - add SMMU_EVENT_STRING
>>
>> v7 -> v8:
>> - use dma_addr_t instead of hwaddr in smmuv3_record_event()
>> - introduce struct SMMUEventInfo
>> - add event_stringify + helpers for all fields
>> ---
>> hw/arm/smmuv3-internal.h | 142
>> ++-
>> hw/arm/smmuv3.c | 108 +--
>> hw/arm/trace-events | 1 +
>> 3 files changed, 243 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index 8550be0..8e546bf 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -205,8 +205,6 @@ 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 */
>>
>> typedef enum SMMUCommandType {
>> @@ -308,4 +306,144 @@ enum { /* Command completion notification */
>>
>> #define SMMU_FEATURE_2LVL_STE (1 << 0)
>>
>> +/* Events */
>> +
>> +typedef enum SMMUEventType {
>> +SMMU_EVT_OK = 0x00,
>> +SMMU_EVT_F_UUT = 0x01,
>> +SMMU_EVT_C_BAD_STREAMID = 0x02,
>> +SMMU_EVT_F_STE_FETCH= 0x03,
>> +SMMU_EVT_C_BAD_STE = 0x04,
>> +SMMU_EVT_F_BAD_ATS_TREQ = 0x05,
>> +SMMU_EVT_F_STREAM_DISABLED = 0x06,
>> +SMMU_EVT_F_TRANS_FORBIDDEN = 0x07,
>> +SMMU_EVT_C_BAD_SUBSTREAMID = 0x08,
>> +SMMU_EVT_F_CD_FETCH = 0x09,
>> +SMMU_EVT_C_BAD_CD = 0x0a,
>> +SMMU_EVT_F_WALK_EABT= 0x0b,
>
> Most of the other enums you let the auto-increment deal
> with long runs of consecutive integers like this. I don't
> much care which but being consistent is generally nicer.
>
>> +SMMU_EVT_F_TRANSLATION = 0x10,
>> +SMMU_EVT_F_ADDR_SIZE= 0x11,
>> +SMMU_EVT_F_ACCESS = 0x12,
>> +SMMU_EVT_F_PERMISSION = 0x13,
>> +SMMU_EVT_F_TLB_CONFLICT = 0x20,
>> +SMMU_EVT_F_CFG_CONFLICT = 0x21,
>> +SMMU_EVT_E_PAGE_REQ = 0x24,
>> +} SMMUEventType;
>> +
>> +static const char *event_stringify[] = {
>> +[SMMU_EVT_OK] = "SMMU_EVT_OK",
>> +[SMMU_EVT_F_UUT]= "SMMU_EVT_F_UUT",
>> +[SMMU_EVT_C_BAD_STREAMID] = "SMMU_EVT_C_BAD_STREAMID",
>> +[SMMU_EVT_F_STE_FETCH] = "SMMU_EVT_F_STE_FETCH",
>> +[SMMU_EVT_C_BAD_STE]= "SMMU_EVT_C_BAD_STE",
>> +[SMMU_EVT_F_BAD_ATS_TREQ] = "SMMU_EVT_F_BAD_ATS_TREQ",
>> +[SMMU_EVT_F_STREAM_DISABLED]= "SMMU_EVT_F_STREAM_DISABLED",
>> +[SMMU_EVT_F_TRANS_FORBIDDEN]= "SMMU_EVT_F_TRANS_FORBIDDEN",
>> +[SMMU_EVT_C_BAD_SUBSTREAMID]= "SMMU_EVT_C_BAD_SUBSTREAMID",
>> +[SMMU_EVT_F_CD_FETCH] = "SMMU_EVT_F_CD_FETCH",
>> +[SMMU_EVT_C_BAD_CD] = "SMMU_EVT_C_BAD_CD",
>> +[SMMU_EVT_F_WALK_EABT] = "SMMU_EVT_F_WALK_EABT",
>> +[SMMU_EVT_F_TRANSLATION]= "SMMU_EVT_F_TRANSLATION",
>> +[SMMU_EVT_F_ADDR_SIZE] = "SMMU_EVT_F_ADDR_SIZE",
>> +[SMMU_EVT_F_ACCESS] = "SMMU_EVT_F_ACCESS",
>> +[SMMU_EVT_F_PERMISSION] = "SMMU_EVT_F_PERMISSION",
>> +[SMMU_EVT_F_TLB_CONFLICT] = "SMMU_EVT_F_TLB_CONFLICT",
>> +[SMMU_EVT_F_CFG_CONFLICT] = "SMMU_EVT_F_CFG_CONFLICT",
>> +[SMMU_EVT_E_PAGE_REQ] = "SMMU_EVT_E_PAGE_REQ",
>> +};
>> +
>> +static inline const char *smmu_event_string(SMMUEventType type)
>> +{
>> +return event_stringify[type] ? event_stringify[type] : "UNKNOWN";
>
> Same remarks about being defensive about out of range values
> apply here, I expect.
I think this one is safe as called from smmuv3_record_event(). Type is
is sanitized in that case. Adding the check against ARRAY_SIZE though.
Thanks
Eric
>
>> +}
>> +
>> +/* Encode an event record */
>> +typedef struct SMMUEventInfo {
>> +SMMUEventType type;
>> +uint32_t sid;
>> +bool recorded;
>> +bool record_trans_faults;
>> +union {
>> +struct {
>> +uint32_t ssid;
>> +bool ssv;
>> +dma_addr_t addr;
>> +bool rnw;
>> +bool pnu;
>> +bool ind;
>> + } f_uut;
>> + struct ssid_info {
>> +uint32_t ssid;
>> +bool ssv;
>> + } c_bad_streamid;
>
> If we were being really picky about coding style these
> embedded struct names like ssid_info ought to be camelcase.
>
>> + struct ssid_addr_info {
>> +
Re: [Qemu-devel] [PATCH v11 08/17] hw/arm/smmuv3: Event queue recording helper
On 12 April 2018 at 08:37, Eric Auger wrote:
> Let's introduce a helper function aiming at recording an
> event in the event queue.
>
> Signed-off-by: Eric Auger
>
> ---
> v9 -> v10:
> - rework SMMU_EVENT_STRING
> - trigger a GERROR EVENTQ_ABT_ERR in case of eventq write failure
>
> v8 -> v9:
> - add SMMU_EVENT_STRING
>
> v7 -> v8:
> - use dma_addr_t instead of hwaddr in smmuv3_record_event()
> - introduce struct SMMUEventInfo
> - add event_stringify + helpers for all fields
> ---
> hw/arm/smmuv3-internal.h | 142
> ++-
> hw/arm/smmuv3.c | 108 +--
> hw/arm/trace-events | 1 +
> 3 files changed, 243 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 8550be0..8e546bf 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -205,8 +205,6 @@ 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 */
>
> typedef enum SMMUCommandType {
> @@ -308,4 +306,144 @@ enum { /* Command completion notification */
>
> #define SMMU_FEATURE_2LVL_STE (1 << 0)
>
> +/* Events */
> +
> +typedef enum SMMUEventType {
> +SMMU_EVT_OK = 0x00,
> +SMMU_EVT_F_UUT = 0x01,
> +SMMU_EVT_C_BAD_STREAMID = 0x02,
> +SMMU_EVT_F_STE_FETCH= 0x03,
> +SMMU_EVT_C_BAD_STE = 0x04,
> +SMMU_EVT_F_BAD_ATS_TREQ = 0x05,
> +SMMU_EVT_F_STREAM_DISABLED = 0x06,
> +SMMU_EVT_F_TRANS_FORBIDDEN = 0x07,
> +SMMU_EVT_C_BAD_SUBSTREAMID = 0x08,
> +SMMU_EVT_F_CD_FETCH = 0x09,
> +SMMU_EVT_C_BAD_CD = 0x0a,
> +SMMU_EVT_F_WALK_EABT= 0x0b,
Most of the other enums you let the auto-increment deal
with long runs of consecutive integers like this. I don't
much care which but being consistent is generally nicer.
> +SMMU_EVT_F_TRANSLATION = 0x10,
> +SMMU_EVT_F_ADDR_SIZE= 0x11,
> +SMMU_EVT_F_ACCESS = 0x12,
> +SMMU_EVT_F_PERMISSION = 0x13,
> +SMMU_EVT_F_TLB_CONFLICT = 0x20,
> +SMMU_EVT_F_CFG_CONFLICT = 0x21,
> +SMMU_EVT_E_PAGE_REQ = 0x24,
> +} SMMUEventType;
> +
> +static const char *event_stringify[] = {
> +[SMMU_EVT_OK] = "SMMU_EVT_OK",
> +[SMMU_EVT_F_UUT]= "SMMU_EVT_F_UUT",
> +[SMMU_EVT_C_BAD_STREAMID] = "SMMU_EVT_C_BAD_STREAMID",
> +[SMMU_EVT_F_STE_FETCH] = "SMMU_EVT_F_STE_FETCH",
> +[SMMU_EVT_C_BAD_STE]= "SMMU_EVT_C_BAD_STE",
> +[SMMU_EVT_F_BAD_ATS_TREQ] = "SMMU_EVT_F_BAD_ATS_TREQ",
> +[SMMU_EVT_F_STREAM_DISABLED]= "SMMU_EVT_F_STREAM_DISABLED",
> +[SMMU_EVT_F_TRANS_FORBIDDEN]= "SMMU_EVT_F_TRANS_FORBIDDEN",
> +[SMMU_EVT_C_BAD_SUBSTREAMID]= "SMMU_EVT_C_BAD_SUBSTREAMID",
> +[SMMU_EVT_F_CD_FETCH] = "SMMU_EVT_F_CD_FETCH",
> +[SMMU_EVT_C_BAD_CD] = "SMMU_EVT_C_BAD_CD",
> +[SMMU_EVT_F_WALK_EABT] = "SMMU_EVT_F_WALK_EABT",
> +[SMMU_EVT_F_TRANSLATION]= "SMMU_EVT_F_TRANSLATION",
> +[SMMU_EVT_F_ADDR_SIZE] = "SMMU_EVT_F_ADDR_SIZE",
> +[SMMU_EVT_F_ACCESS] = "SMMU_EVT_F_ACCESS",
> +[SMMU_EVT_F_PERMISSION] = "SMMU_EVT_F_PERMISSION",
> +[SMMU_EVT_F_TLB_CONFLICT] = "SMMU_EVT_F_TLB_CONFLICT",
> +[SMMU_EVT_F_CFG_CONFLICT] = "SMMU_EVT_F_CFG_CONFLICT",
> +[SMMU_EVT_E_PAGE_REQ] = "SMMU_EVT_E_PAGE_REQ",
> +};
> +
> +static inline const char *smmu_event_string(SMMUEventType type)
> +{
> +return event_stringify[type] ? event_stringify[type] : "UNKNOWN";
Same remarks about being defensive about out of range values
apply here, I expect.
> +}
> +
> +/* Encode an event record */
> +typedef struct SMMUEventInfo {
> +SMMUEventType type;
> +uint32_t sid;
> +bool recorded;
> +bool record_trans_faults;
> +union {
> +struct {
> +uint32_t ssid;
> +bool ssv;
> +dma_addr_t addr;
> +bool rnw;
> +bool pnu;
> +bool ind;
> + } f_uut;
> + struct ssid_info {
> +uint32_t ssid;
> +bool ssv;
> + } c_bad_streamid;
If we were being really picky about coding style these
embedded struct names like ssid_info ought to be camelcase.
> + struct ssid_addr_info {
> +uint32_t ssid;
> +bool ssv;
> +dma_addr_t addr;
> + } f_ste_fetch;
> + struct ssid_info c_bad_ste;
> + struct {
> +dma_addr_t addr;
> +bool rnw;
> + } f_transl_forbidden;
> + struct {
> +uint32_t ssid;
> + } c_bad_substream;
> + struct ssid_addr_i
