Re: [PATCH v1 08/14] iommu/arm-smmu-v3: Prepare for nested domain support

Robin Murphy
2023-03-10T20:40:34Z

On 2023-03-09 10:53, Nicolin Chen wrote:
> In a nested translation setup, the device is attached to a stage-1 domain
> that represents the guest-level Context Descriptor table. A Stream Table
> Entry for a 2-stage translation needs both the stage-1 Context Descriptor
> table info and the stage-2 Translation table information, i.e. a pair of
> s1_cfg and s2_cfg.
>
> Add an "s2" pointer in struct arm_smmu_domain, so a nested stage-1 domain
> can simply navigate its stage-2 domain for the s2_cfg pointer. Also, add
> a to_s2_cfg() helper for this purpose, and use it at proper places.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++++++--
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
> 2 files changed, 24 insertions(+), 2 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 21d819979865..fee5977feef3 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -100,6 +100,24 @@ static void parse_driver_options(struct arm_smmu_device *smmu)
> } while (arm_smmu_options[++i].opt);
> }
>
> +static struct arm_smmu_s2_cfg *to_s2_cfg(struct arm_smmu_domain *smmu_domain)
> +{
> + if (!smmu_domain)
> + return NULL;
> +
> + switch (smmu_domain->stage) {
> + case ARM_SMMU_DOMAIN_S1:
> + if (smmu_domain->s2)
> + return &smmu_domain->s2->s2_cfg;
> + return NULL;
> + case ARM_SMMU_DOMAIN_S2:
> + return &smmu_domain->s2_cfg;
> + case ARM_SMMU_DOMAIN_BYPASS:
> + default:
> + return NULL;
> + }
> +}
> +
> /* Low-level queue manipulation functions */
> static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
> {
> @@ -1277,6 +1295,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> switch (smmu_domain->stage) {
> case ARM_SMMU_DOMAIN_S1:
> s1_cfg = &smmu_domain->s1_cfg;
> + s2_cfg = to_s2_cfg(smmu_domain);

TBH I'd say you only need a 2-line change here. All the other cases
below are when the stage is guaranteed to be ARM_SMMU_DOMAIN_S2 (once
ARM_SMMU_DOMAIN_NESTED is gone), so pretending it might be otherwise
seems unnecessarily confusing.

Thanks,
Robin.

> break;
> case ARM_SMMU_DOMAIN_S2:
> s2_cfg = &smmu_domain->s2_cfg;
> @@ -1846,6 +1865,7 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
> static void arm_smmu_tlb_inv_context(void *cookie)
> {
> struct arm_smmu_domain *smmu_domain = cookie;
> + struct arm_smmu_s2_cfg *s2_cfg = to_s2_cfg(smmu_domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> struct arm_smmu_cmdq_ent cmd;
>
> @@ -1860,7 +1880,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> 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 = s2_cfg->vmid;
> arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> }
> arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
> @@ -1931,6 +1951,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
> size_t granule, bool leaf,
> struct arm_smmu_domain *smmu_domain)
> {
> + struct arm_smmu_s2_cfg *s2_cfg = to_s2_cfg(smmu_domain);
> struct arm_smmu_cmdq_ent cmd = {
> .tlbi = {
> .leaf = leaf,
> @@ -1943,7 +1964,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
> 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 = s2_cfg->vmid;
> }
> __arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 1a93eeb993ea..6cf516852721 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -709,6 +709,7 @@ enum arm_smmu_domain_stage {
> };
>
> struct arm_smmu_domain {
> + struct arm_smmu_domain *s2;
> struct arm_smmu_device *smmu;
> struct mutex init_mutex; /* Protects smmu pointer */