Re: [PATCH RESEND 4/9] hw/arm/smmu: Introduce SMMUTLBEntry for PTW and IOTLB value

From: Peter Maydell <peter.maydell@linaro.org>
To: Eric Auger <eric.auger@redhat.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Robin Murphy <robin.murphy@arm.com>,
	zhangfei.gao@foxmail.com, QEMU Developers <qemu-devel@nongnu.org>,
	Peter Xu <peterx@redhat.com>, qemu-arm <qemu-arm@nongnu.org>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	Will Deacon <will@kernel.org>, Rob Herring <robh@kernel.org>,
	Eric Auger <eric.auger.pro@gmail.com>
Subject: Re: [PATCH RESEND 4/9] hw/arm/smmu: Introduce SMMUTLBEntry for PTW and IOTLB value
Date: Thu, 25 Jun 2020 16:13:32 +0100	[thread overview]
Message-ID: <CAFEAcA9BR4KJQP-M5QswYkWAmTy5OsAdOa6wF3oPMa_EbeBXPg@mail.gmail.com> (raw)
In-Reply-To: <20200611161500.23580-5-eric.auger@redhat.com>

On Thu, 11 Jun 2020 at 17:15, Eric Auger <eric.auger@redhat.com> wrote:
>
> Introduce a specialized SMMUTLBEntry to store the result of
> the PTW and cache in the IOTLB. This structure extends the
> generic IOMMUTLBEntry struct with the level of the entry and
> the granule size.
>
> Those latter will be useful when implementing range invalidation.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/hw/arm/smmu-common.h | 14 +++++++++++---
>  hw/arm/smmu-common.c         | 30 ++++++++++++++++--------------
>  hw/arm/smmuv3.c              | 10 +++++-----
>  3 files changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 7b9d2f0eb7..f6ee78e16c 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -52,6 +52,14 @@ typedef struct SMMUTransTableInfo {
>      uint8_t granule_sz;        /* granule page shift */
>  } SMMUTransTableInfo;
>
> +struct SMMUTLBEntry {
> +    IOMMUTLBEntry entry;
> +    uint8_t level;
> +    uint8_t granule;
> +};
> +
> +typedef struct SMMUTLBEntry SMMUTLBEntry;

Every other typedef in this header uses
  typedef struct Foo {
      ...
  } Foo;

rather than a separate typedef line; could we be consistent with that?

> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 3101043540..aa88b62efb 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -47,11 +47,11 @@ uint64_t smmu_get_iotlb_key(uint16_t asid, uint64_t iova)
>      return iova >> 12 | (uint64_t)(asid) << SMMU_IOTLB_ASID_SHIFT;
>  }
>
> -IOMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
>                                   hwaddr iova)

Indent on this second line needs to move in one space now.

>  {
>      uint64_t key = smmu_get_iotlb_key(cfg->asid, iova);
> -    IOMMUTLBEntry *entry = g_hash_table_lookup(bs->iotlb, &key);
> +    SMMUTLBEntry *entry = g_hash_table_lookup(bs->iotlb, &key);
>
>      if (entry) {
>          cfg->iotlb_hits++;

> @@ -303,7 +305,7 @@ error:
>   * return 0 on success
>   */
>  inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> -             IOMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +             SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)

might as well fix this indent while we're editing the line...

>  {
>      if (!cfg->aa64) {
>          /*

otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM