Re: [PATCH 1/2] hw/arm/smmuv3: Cache event fault record

[Eric Auger <eric.auger@redhat.com>]

Hi Jean,

On 4/27/22 13:15, Jean-Philippe Brucker wrote:
> The Record bit in the Context Descriptor tells the SMMU to report fault
> events to the event queue. Since we don't cache the Record bit at the
> moment, access faults from a cached Context Descriptor are never
> reported. Store the Record bit in the cached SMMUTransCfg.
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks!

Eric
>
> Fixes: 9bde7f0674fe ("hw/arm/smmuv3: Implement translate callback")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/smmuv3-internal.h     |  1 -
>  include/hw/arm/smmu-common.h |  1 +
>  hw/arm/smmuv3.c              | 14 +++++++-------
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index d1885ae3f2..6de52bbf4d 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -387,7 +387,6 @@ typedef struct SMMUEventInfo {
>      SMMUEventType type;
>      uint32_t sid;
>      bool recorded;
> -    bool record_trans_faults;
>      bool inval_ste_allowed;
>      union {
>          struct {
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 706be3c6d0..21e62342e9 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -71,6 +71,7 @@ typedef struct SMMUTransCfg {
>      bool disabled;             /* smmu is disabled */
>      bool bypassed;             /* translation is bypassed */
>      bool aborted;              /* translation is aborted */
> +    bool record_faults;        /* record fault events */
>      uint64_t ttb;              /* TT base address */
>      uint8_t oas;               /* output address width */
>      uint8_t tbi;               /* Top Byte Ignore */
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 707eb430c2..8b1d8103dc 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -527,7 +527,7 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
>          trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb, tt->granule_sz, tt->had);
>      }
>  
> -    event->record_trans_faults = CD_R(cd);
> +    cfg->record_faults = CD_R(cd);
>  
>      return 0;
>  
> @@ -680,7 +680,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>  
>      tt = select_tt(cfg, addr);
>      if (!tt) {
> -        if (event.record_trans_faults) {
> +        if (cfg->record_faults) {
>              event.type = SMMU_EVT_F_TRANSLATION;
>              event.u.f_translation.addr = addr;
>              event.u.f_translation.rnw = flag & 0x1;
> @@ -696,7 +696,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      if (cached_entry) {
>          if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
>              status = SMMU_TRANS_ERROR;
> -            if (event.record_trans_faults) {
> +            if (cfg->record_faults) {
>                  event.type = SMMU_EVT_F_PERMISSION;
>                  event.u.f_permission.addr = addr;
>                  event.u.f_permission.rnw = flag & 0x1;
> @@ -720,28 +720,28 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>              event.u.f_walk_eabt.addr2 = ptw_info.addr;
>              break;
>          case SMMU_PTW_ERR_TRANSLATION:
> -            if (event.record_trans_faults) {
> +            if (cfg->record_faults) {
>                  event.type = SMMU_EVT_F_TRANSLATION;
>                  event.u.f_translation.addr = addr;
>                  event.u.f_translation.rnw = flag & 0x1;
>              }
>              break;
>          case SMMU_PTW_ERR_ADDR_SIZE:
> -            if (event.record_trans_faults) {
> +            if (cfg->record_faults) {
>                  event.type = SMMU_EVT_F_ADDR_SIZE;
>                  event.u.f_addr_size.addr = addr;
>                  event.u.f_addr_size.rnw = flag & 0x1;
>              }
>              break;
>          case SMMU_PTW_ERR_ACCESS:
> -            if (event.record_trans_faults) {
> +            if (cfg->record_faults) {
>                  event.type = SMMU_EVT_F_ACCESS;
>                  event.u.f_access.addr = addr;
>                  event.u.f_access.rnw = flag & 0x1;
>              }
>              break;
>          case SMMU_PTW_ERR_PERMISSION:
> -            if (event.record_trans_faults) {
> +            if (cfg->record_faults) {
>                  event.type = SMMU_EVT_F_PERMISSION;
>                  event.u.f_permission.addr = addr;
>                  event.u.f_permission.rnw = flag & 0x1;