Re: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and s2_cfg

2020-11-17 Thread Auger Eric
Hi Shameer,

On 11/17/20 12:39 PM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
> 
>> -Original Message-
>> From: Eric Auger [mailto:eric.au...@redhat.com]
>> Sent: 16 November 2020 10:43
>> To: eric.auger@gmail.com; eric.au...@redhat.com;
>> iommu@lists.linux-foundation.org; linux-ker...@vger.kernel.org;
>> k...@vger.kernel.org; kvm...@lists.cs.columbia.edu; w...@kernel.org;
>> j...@8bytes.org; m...@kernel.org; robin.mur...@arm.com
>> Cc: jean-phili...@linaro.org; zhangfei@linaro.org;
>> zhangfei@gmail.com; vivek.gau...@arm.com; Shameerali Kolothum
>> Thodi ;
>> alex.william...@redhat.com; jacob.jun@linux.intel.com;
>> yi.l....@intel.com; t...@semihalf.com; nicoleots...@gmail.com
>> Subject: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and
>> s2_cfg
>>
>> In preparation for the introduction of nested stages
>> let's turn s1_cfg and s2_cfg fields into pointers which are
>> dynamically allocated depending on the smmu_domain stage.
> 
> This will break compile if we have CONFIG_ARM_SMMU_V3_SVA
> because ,
> https://github.com/eauger/linux/blob/5.10-rc4-2stage-v12/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c#L40
> 
> Do we really need to make these pointers?

Thanks for reporting. I think I can do differently. Working on this now.

Thanks

Eric
> 
> Thanks,
> Shameer
>  
>> In nested mode, both stages will coexist and s1_cfg will
>> be allocated when the guest configuration gets passed.
>>
>> Signed-off-by: Eric Auger 
>> ---
>>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 83 -
>>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  6 +-
>>  2 files changed, 48 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index d828d6cbeb0e..4baf9fafe462 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -953,9 +953,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct
>> arm_smmu_domain *smmu_domain,
>>  unsigned int idx;
>>  struct arm_smmu_l1_ctx_desc *l1_desc;
>>  struct arm_smmu_device *smmu = smmu_domain->smmu;
>> -struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg;
>> +struct arm_smmu_ctx_desc_cfg *cdcfg =
>> _domain->s1_cfg->cdcfg;
>>
>> -if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
>> +if (smmu_domain->s1_cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
>>  return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS;
>>
>>  idx = ssid >> CTXDESC_SPLIT;
>> @@ -990,7 +990,7 @@ int arm_smmu_write_ctx_desc(struct
>> arm_smmu_domain *smmu_domain, int ssid,
>>  __le64 *cdptr;
>>  struct arm_smmu_device *smmu = smmu_domain->smmu;
>>
>> -if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg.s1cdmax)))
>> +if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg->s1cdmax)))
>>  return -E2BIG;
>>
>>  cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid);
>> @@ -1056,7 +1056,7 @@ static int arm_smmu_alloc_cd_tables(struct
>> arm_smmu_domain *smmu_domain)
>>  size_t l1size;
>>  size_t max_contexts;
>>  struct arm_smmu_device *smmu = smmu_domain->smmu;
>> -struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg;
>> +struct arm_smmu_s1_cfg *cfg = smmu_domain->s1_cfg;
>>  struct arm_smmu_ctx_desc_cfg *cdcfg = >cdcfg;
>>
>>  max_contexts = 1 << cfg->s1cdmax;
>> @@ -1104,7 +1104,7 @@ static void arm_smmu_free_cd_tables(struct
>> arm_smmu_domain *smmu_domain)
>>  int i;
>>  size_t size, l1size;
>>  struct arm_smmu_device *smmu = smmu_domain->smmu;
>> -struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg;
>> +struct arm_smmu_ctx_desc_cfg *cdcfg =
>> _domain->s1_cfg->cdcfg;
>>
>>  if (cdcfg->l1_desc) {
>>  size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3);
>> @@ -1211,17 +1211,8 @@ static void arm_smmu_write_strtab_ent(struct
>> arm_smmu_master *master, u32 sid,
>>  }
>>
>>  if (smmu_domain) {
>> -switch (smmu_domain->stage) {
>> -case ARM_SMMU_DOMAIN_S1:
>> -s1_cfg = _domain->s1_cfg;
>> -break;
>> -case ARM_SMMU_DOMAIN_S2:
>> -case ARM_SMMU_DOMAIN_NESTED:
>> -s2_cfg = _domain->s2_cfg;
>> -   

RE: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and s2_cfg

2020-11-17 Thread Shameerali Kolothum Thodi
Hi Eric,

> -Original Message-
> From: Eric Auger [mailto:eric.au...@redhat.com]
> Sent: 16 November 2020 10:43
> To: eric.auger@gmail.com; eric.au...@redhat.com;
> iommu@lists.linux-foundation.org; linux-ker...@vger.kernel.org;
> k...@vger.kernel.org; kvm...@lists.cs.columbia.edu; w...@kernel.org;
> j...@8bytes.org; m...@kernel.org; robin.mur...@arm.com
> Cc: jean-phili...@linaro.org; zhangfei@linaro.org;
> zhangfei@gmail.com; vivek.gau...@arm.com; Shameerali Kolothum
> Thodi ;
> alex.william...@redhat.com; jacob.jun@linux.intel.com;
> yi.l@intel.com; t...@semihalf.com; nicoleots...@gmail.com
> Subject: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and
> s2_cfg
> 
> In preparation for the introduction of nested stages
> let's turn s1_cfg and s2_cfg fields into pointers which are
> dynamically allocated depending on the smmu_domain stage.

This will break compile if we have CONFIG_ARM_SMMU_V3_SVA
because ,
https://github.com/eauger/linux/blob/5.10-rc4-2stage-v12/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c#L40

Do we really need to make these pointers?

Thanks,
Shameer
 
> In nested mode, both stages will coexist and s1_cfg will
> be allocated when the guest configuration gets passed.
> 
> Signed-off-by: Eric Auger 
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 83 -
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  6 +-
>  2 files changed, 48 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index d828d6cbeb0e..4baf9fafe462 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -953,9 +953,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct
> arm_smmu_domain *smmu_domain,
>   unsigned int idx;
>   struct arm_smmu_l1_ctx_desc *l1_desc;
>   struct arm_smmu_device *smmu = smmu_domain->smmu;
> - struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg;
> + struct arm_smmu_ctx_desc_cfg *cdcfg =
> _domain->s1_cfg->cdcfg;
> 
> - if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
> + if (smmu_domain->s1_cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
>   return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS;
> 
>   idx = ssid >> CTXDESC_SPLIT;
> @@ -990,7 +990,7 @@ int arm_smmu_write_ctx_desc(struct
> arm_smmu_domain *smmu_domain, int ssid,
>   __le64 *cdptr;
>   struct arm_smmu_device *smmu = smmu_domain->smmu;
> 
> - if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg.s1cdmax)))
> + if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg->s1cdmax)))
>   return -E2BIG;
> 
>   cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid);
> @@ -1056,7 +1056,7 @@ static int arm_smmu_alloc_cd_tables(struct
> arm_smmu_domain *smmu_domain)
>   size_t l1size;
>   size_t max_contexts;
>   struct arm_smmu_device *smmu = smmu_domain->smmu;
> - struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg;
> + struct arm_smmu_s1_cfg *cfg = smmu_domain->s1_cfg;
>   struct arm_smmu_ctx_desc_cfg *cdcfg = >cdcfg;
> 
>   max_contexts = 1 << cfg->s1cdmax;
> @@ -1104,7 +1104,7 @@ static void arm_smmu_free_cd_tables(struct
> arm_smmu_domain *smmu_domain)
>   int i;
>   size_t size, l1size;
>   struct arm_smmu_device *smmu = smmu_domain->smmu;
> - struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg;
> + struct arm_smmu_ctx_desc_cfg *cdcfg =
> _domain->s1_cfg->cdcfg;
> 
>   if (cdcfg->l1_desc) {
>   size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3);
> @@ -1211,17 +1211,8 @@ static void arm_smmu_write_strtab_ent(struct
> arm_smmu_master *master, u32 sid,
>   }
> 
>   if (smmu_domain) {
> - switch (smmu_domain->stage) {
> - case ARM_SMMU_DOMAIN_S1:
> - s1_cfg = _domain->s1_cfg;
> - break;
> - case ARM_SMMU_DOMAIN_S2:
> - case ARM_SMMU_DOMAIN_NESTED:
> - s2_cfg = _domain->s2_cfg;
> - break;
> - default:
> - break;
> - }
> + s1_cfg = smmu_domain->s1_cfg;
> + s2_cfg = smmu_domain->s2_cfg;
>   }
> 
>   if (val & STRTAB_STE_0_V) {
> @@ -1664,10 +1655,10 @@ static void arm_smmu_tlb_inv_context(void
> *cookie)
>* careful, 007.
>*/
>   if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> - arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid);
> +  

[PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and s2_cfg

2020-11-16 Thread Eric Auger
In preparation for the introduction of nested stages
let's turn s1_cfg and s2_cfg fields into pointers which are
dynamically allocated depending on the smmu_domain stage.

In nested mode, both stages will coexist and s1_cfg will
be allocated when the guest configuration gets passed.

Signed-off-by: Eric Auger 
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 83 -
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  6 +-
 2 files changed, 48 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d828d6cbeb0e..4baf9fafe462 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -953,9 +953,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain 
*smmu_domain,
unsigned int idx;
struct arm_smmu_l1_ctx_desc *l1_desc;
struct arm_smmu_device *smmu = smmu_domain->smmu;
-   struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg;
+   struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg->cdcfg;
 
-   if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
+   if (smmu_domain->s1_cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS;
 
idx = ssid >> CTXDESC_SPLIT;
@@ -990,7 +990,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain 
*smmu_domain, int ssid,
__le64 *cdptr;
struct arm_smmu_device *smmu = smmu_domain->smmu;
 
-   if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg.s1cdmax)))
+   if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg->s1cdmax)))
return -E2BIG;
 
cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid);
@@ -1056,7 +1056,7 @@ static int arm_smmu_alloc_cd_tables(struct 
arm_smmu_domain *smmu_domain)
size_t l1size;
size_t max_contexts;
struct arm_smmu_device *smmu = smmu_domain->smmu;
-   struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg;
+   struct arm_smmu_s1_cfg *cfg = smmu_domain->s1_cfg;
struct arm_smmu_ctx_desc_cfg *cdcfg = >cdcfg;
 
max_contexts = 1 << cfg->s1cdmax;
@@ -1104,7 +1104,7 @@ static void arm_smmu_free_cd_tables(struct 
arm_smmu_domain *smmu_domain)
int i;
size_t size, l1size;
struct arm_smmu_device *smmu = smmu_domain->smmu;
-   struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg;
+   struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg->cdcfg;
 
if (cdcfg->l1_desc) {
size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3);
@@ -1211,17 +1211,8 @@ static void arm_smmu_write_strtab_ent(struct 
arm_smmu_master *master, u32 sid,
}
 
if (smmu_domain) {
-   switch (smmu_domain->stage) {
-   case ARM_SMMU_DOMAIN_S1:
-   s1_cfg = _domain->s1_cfg;
-   break;
-   case ARM_SMMU_DOMAIN_S2:
-   case ARM_SMMU_DOMAIN_NESTED:
-   s2_cfg = _domain->s2_cfg;
-   break;
-   default:
-   break;
-   }
+   s1_cfg = smmu_domain->s1_cfg;
+   s2_cfg = smmu_domain->s2_cfg;
}
 
if (val & STRTAB_STE_0_V) {
@@ -1664,10 +1655,10 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 * careful, 007.
 */
if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-   arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid);
+   arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg->cd.asid);
} else {
cmd.opcode  = CMDQ_OP_TLBI_S12_VMALL;
-   cmd.tlbi.vmid   = smmu_domain->s2_cfg.vmid;
+   cmd.tlbi.vmid   = smmu_domain->s2_cfg->vmid;
arm_smmu_cmdq_issue_cmd(smmu, );
arm_smmu_cmdq_issue_sync(smmu);
}
@@ -1693,10 +1684,10 @@ static void arm_smmu_tlb_inv_range(unsigned long iova, 
size_t size,
 
if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
cmd.opcode  = CMDQ_OP_TLBI_NH_VA;
-   cmd.tlbi.asid   = smmu_domain->s1_cfg.cd.asid;
+   cmd.tlbi.asid   = smmu_domain->s1_cfg->cd.asid;
} else {
cmd.opcode  = CMDQ_OP_TLBI_S2_IPA;
-   cmd.tlbi.vmid   = smmu_domain->s2_cfg.vmid;
+   cmd.tlbi.vmid   = smmu_domain->s2_cfg->vmid;
}
 
if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
@@ -1846,24 +1837,25 @@ static void arm_smmu_domain_free(struct iommu_domain 
*domain)
 {
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;
+   struct arm_smmu_s1_cfg *s1_cfg = smmu_domain->s1_cfg;
+   struct arm_smmu_s2_cfg *s2_cfg = smmu_domain->s2_cfg;
 
iommu_put_dma_cookie(domain);
free_io_pgtable_ops(smmu_domain->pgtbl_ops);
 
/* Free the CD and ASID, if we allocated them */
-