[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 7/8] xen/arm: Remove Linux specific code that is not usable in XEN


  • To: Julien Grall <julien@xxxxxxx>
  • From: Rahul Singh <Rahul.Singh@xxxxxxx>
  • Date: Thu, 3 Dec 2020 14:33:49 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=83I9tKpsKyeU0i3y+i565ekiUkZAPReSxob8mTNCB2c=; b=fQsvru/4hGkM7BNnzB2yLmjXvr6a2Xy9KMsYo5xNJitOPVXJk6F8E5Dx3THf2Q0/1/8MG1WM0sY8O32fjZJSLLSXMqa7YgYsksZsFgm3dZVyVHuFTsHHM1JNFUHHBJWmTcS3NHK4Ej9W6z1fPDDYhRETbTsfHnaRRQf17gdftNuAn1APgb7SWkAt0MyO67+bk2bjjTmJS57lWI91oYSEbqbzwPDPihTreyOjpiGzV4kAAAffRaTM1LNOZdIyRACmw8WRyBoYIIAAhR2bKTFMKtXOeI2Arn08xILouW6mXre3BqfnqRd9WtSwadViGkHZr9XaaBy30OOlGCvGEa2BBw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XVyj+TiTc5qlY8WPXIqXlGQ+p3xKbiqIR9XTFlC1KB9pm+vFXeiROwVZZW44bgQUjShCeJjPNwboUMV4vz5tGjeGi20fX+NF5E/gJ+dMr5NoSJgKRDkDAsy3Op/ZUjYWGtzd2a9FGJtiznJ9eNtq5kLuOaD/1GZCXcG9V9ldyMYS0wziBCQR3AT2veIkvuhVKJ8lW5ULzy1ouuck+1l3TPH6kQg9dbiAMNADVW7iXyyP7/5d9F4fbrXYH/ppMnBJu3CwEWrQhvoLUtxym9XBmvCqsvNho2/c0ghQN4U5DxevdsafrYF6tcBw1Zsdy6JyCLHLZF6+AF5aDSBhDqjHQQ==
  • Authentication-results-original: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 03 Dec 2020 14:34:48 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHWxBX830xFguE4YkK2klATz/ucYKnj64aAgAGO7QA=
  • Thread-topic: [PATCH v2 7/8] xen/arm: Remove Linux specific code that is not usable in XEN

Hello Julien,

> On 2 Dec 2020, at 2:45 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 26/11/2020 17:02, Rahul Singh wrote:
>> struct io_pgtable_ops, struct io_pgtable_cfg, struct iommu_flush_ops,
>> and struct iommu_ops related code are linux specific.
> 
> So the assumption is we are going to support only sharing with the P2M and 
> the IOMMU. That's probably fine short term, but long-term we are going to 
> need unsharing the page-tables (there are issues on some platforms if the ITS 
> doorbell is accessed by the processors).
> 
> I am ok with removing anything related to the unsharing code. But I think it 
> should be clarified here.

Ok I will add in commit message what is removed from the code.
> 
>> Remove code related to above struct as code is dead code in XEN.
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>>  xen/drivers/passthrough/arm/smmu-v3.c | 457 --------------------------
>>  1 file changed, 457 deletions(-)
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index 40e3890a58..55d1cba194 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -402,13 +402,7 @@
>>  #define ARM_SMMU_CMDQ_SYNC_TIMEOUT_US       1000000 /* 1s! */
>>  #define ARM_SMMU_CMDQ_SYNC_SPIN_COUNT       10
>>  -#define MSI_IOVA_BASE                      0x8000000
>> -#define MSI_IOVA_LENGTH                     0x100000
>> -
>>  static bool disable_bypass = 1;
>> -module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
>> -MODULE_PARM_DESC(disable_bypass,
>> -    "Disable bypass streams such that incoming transactions from devices 
>> that are not attached to an iommu domain will report an abort back to the 
>> device and will not be allowed to pass through the SMMU.");
> 
> Per your commit message, this doesn't look related to what you intend to 
> remove. Maybe your commit message should be updated?

Ok.
> 
>>    enum pri_resp {
>>      PRI_RESP_DENY = 0,
>> @@ -599,7 +593,6 @@ struct arm_smmu_domain {
>>      struct arm_smmu_device          *smmu;
>>      struct mutex                    init_mutex; /* Protects smmu pointer */
>>  -   struct io_pgtable_ops           *pgtbl_ops;
>>      bool                            non_strict;
>>      enum arm_smmu_domain_stage      stage;
>> @@ -1297,74 +1290,6 @@ static void arm_smmu_tlb_inv_context(void *cookie)
>>      arm_smmu_cmdq_issue_sync(smmu);
>>  }
>>  -static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>> -                                      size_t granule, bool leaf, void 
>> *cookie)
>> -{
>> -    struct arm_smmu_domain *smmu_domain = cookie;
>> -    struct arm_smmu_device *smmu = smmu_domain->smmu;
>> -    struct arm_smmu_cmdq_ent cmd = {
>> -            .tlbi = {
>> -                    .leaf   = leaf,
>> -                    .addr   = iova,
>> -            },
>> -    };
>> -
>> -    cmd.opcode      = CMDQ_OP_TLBI_S2_IPA;
>> -    cmd.tlbi.vmid   = smmu_domain->s2_cfg.vmid;
>> -
>> -    do {
>> -            arm_smmu_cmdq_issue_cmd(smmu, &cmd);
>> -            cmd.tlbi.addr += granule;
>> -    } while (size -= granule);
>> -}
>> -
>> -static void arm_smmu_tlb_inv_page_nosync(struct iommu_iotlb_gather *gather,
>> -                                     unsigned long iova, size_t granule,
>> -                                     void *cookie)
>> -{
>> -    arm_smmu_tlb_inv_range_nosync(iova, granule, granule, true, cookie);
>> -}
>> -
>> -static void arm_smmu_tlb_inv_walk(unsigned long iova, size_t size,
>> -                              size_t granule, void *cookie)
>> -{
>> -    struct arm_smmu_domain *smmu_domain = cookie;
>> -    struct arm_smmu_device *smmu = smmu_domain->smmu;
>> -
>> -    arm_smmu_tlb_inv_range_nosync(iova, size, granule, false, cookie);
>> -    arm_smmu_cmdq_issue_sync(smmu);
>> -}
>> -
>> -static void arm_smmu_tlb_inv_leaf(unsigned long iova, size_t size,
>> -                              size_t granule, void *cookie)
>> -{
>> -    struct arm_smmu_domain *smmu_domain = cookie;
>> -    struct arm_smmu_device *smmu = smmu_domain->smmu;
>> -
>> -    arm_smmu_tlb_inv_range_nosync(iova, size, granule, true, cookie);
>> -    arm_smmu_cmdq_issue_sync(smmu);
>> -}
>> -
>> -static const struct iommu_flush_ops arm_smmu_flush_ops = {
>> -    .tlb_flush_all  = arm_smmu_tlb_inv_context,
>> -    .tlb_flush_walk = arm_smmu_tlb_inv_walk,
>> -    .tlb_flush_leaf = arm_smmu_tlb_inv_leaf,
>> -    .tlb_add_page   = arm_smmu_tlb_inv_page_nosync,
>> -};
>> -
>> -/* IOMMU API */
>> -static bool arm_smmu_capable(enum iommu_cap cap)
>> -{
>> -    switch (cap) {
>> -    case IOMMU_CAP_CACHE_COHERENCY:
>> -            return true;
>> -    case IOMMU_CAP_NOEXEC:
>> -            return true;
>> -    default:
>> -            return false;
>> -    }
>> -}
>> -
>>  static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>>  {
>>      struct arm_smmu_domain *smmu_domain;
>> @@ -1421,7 +1346,6 @@ static void arm_smmu_domain_free(struct iommu_domain 
>> *domain)
>>      struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
>>      iommu_put_dma_cookie(domain);
>> -    free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>>      if (cfg->vmid)
>>              arm_smmu_bitmap_free(smmu->vmid_map, cfg->vmid);
>> @@ -1429,7 +1353,6 @@ static void arm_smmu_domain_free(struct iommu_domain 
>> *domain)
>>      kfree(smmu_domain);
>>  }
>>  -
> 
> Looks like a spurious change.

Ack. 
> 
>>  static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
>>                                     struct arm_smmu_master *master,
>>                                     struct io_pgtable_cfg *pgtbl_cfg)
> 
> You commit message leads to think that all the use of struct io_pgtable_cfg 
> will be removed.

Ok I will remove it.
> 
>> @@ -1437,7 +1360,6 @@ static int arm_smmu_domain_finalise_s2(struct 
>> arm_smmu_domain *smmu_domain,
>>      int vmid;
>>      struct arm_smmu_device *smmu = smmu_domain->smmu;
>>      struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
>> -    typeof(&pgtbl_cfg->arm_lpae_s2_cfg.vtcr) vtcr;
> 
> It feels a bit odd that the definition of 'vtcr' is removed but there are 
> still users.

Ack. Will fix that.
> 
>>      vmid = arm_smmu_bitmap_alloc(smmu->vmid_map, smmu->vmid_bits);
>>      if (vmid < 0)
>> @@ -1461,20 +1383,12 @@ static int arm_smmu_domain_finalise(struct 
>> iommu_domain *domain,
>>  {
>>      int ret;
>>      unsigned long ias, oas;
>> -    enum io_pgtable_fmt fmt;
>> -    struct io_pgtable_cfg pgtbl_cfg;
>> -    struct io_pgtable_ops *pgtbl_ops;
>>      int (*finalise_stage_fn)(struct arm_smmu_domain *,
>>                               struct arm_smmu_master *,
>>                               struct io_pgtable_cfg *);
>>      struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>      struct arm_smmu_device *smmu = smmu_domain->smmu;
>>  -   if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>> -            smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
>> -            return 0;
>> -    }
>> -
> 
> Per your commit message, this doesn't look related to what you intend to 
> remove. Maybe your commit message should be updated?

Ok I will update the commit message.
> 
>>      /* Restrict the stage to what we can actually support */
>>      smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
>>  @@ -1483,40 +1397,17 @@ static int arm_smmu_domain_finalise(struct 
>> iommu_domain *domain,
>>      case ARM_SMMU_DOMAIN_S2:
>>              ias = smmu->ias;
>>              oas = smmu->oas;
>> -            fmt = ARM_64_LPAE_S2;
>>              finalise_stage_fn = arm_smmu_domain_finalise_s2;
>>              break;
>>      default:
>>              return -EINVAL;
>>      }
>>  -   pgtbl_cfg = (struct io_pgtable_cfg) {
>> -            .pgsize_bitmap  = smmu->pgsize_bitmap,
>> -            .ias            = ias,
>> -            .oas            = oas,
>> -            .coherent_walk  = smmu->features & ARM_SMMU_FEAT_COHERENCY,
>> -            .tlb            = &arm_smmu_flush_ops,
>> -            .iommu_dev      = smmu->dev,
>> -    };
>> -
>> -    if (smmu_domain->non_strict)
>> -            pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
>> -
>> -    pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
>> -    if (!pgtbl_ops)
>> -            return -ENOMEM;
>> -
>> -    domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
>> -    domain->geometry.aperture_end = (1UL << pgtbl_cfg.ias) - 1;
>> -    domain->geometry.force_aperture = true;
>> -
>>      ret = finalise_stage_fn(smmu_domain, master, &pgtbl_cfg);
>>      if (ret < 0) {
>> -            free_io_pgtable_ops(pgtbl_ops);
>>              return ret;
>>      }
>>  -   smmu_domain->pgtbl_ops = pgtbl_ops;
>>      return 0;
>>  }
>>  @@ -1626,71 +1517,6 @@ out_unlock:
>>      return ret;
>>  }
>>  -static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>> -                    phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
>> -{
>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>> -
>> -    if (!ops)
>> -            return -ENODEV;
>> -
>> -    return ops->map(ops, iova, paddr, size, prot, gfp);
>> -}
>> -
>> -static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long 
>> iova,
>> -                         size_t size, struct iommu_iotlb_gather *gather)
>> -{
>> -    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> -    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>> -
>> -    if (!ops)
>> -            return 0;
>> -
>> -    return ops->unmap(ops, iova, size, gather);
>> -}
>> -
>> -static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain)
>> -{
>> -    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> -
>> -    if (smmu_domain->smmu)
>> -            arm_smmu_tlb_inv_context(smmu_domain);
>> -}
>> -
>> -static void arm_smmu_iotlb_sync(struct iommu_domain *domain,
>> -                            struct iommu_iotlb_gather *gather)
>> -{
>> -    struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
>> -
>> -    if (smmu)
>> -            arm_smmu_cmdq_issue_sync(smmu);
>> -}
>> -
>> -static phys_addr_t
>> -arm_smmu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
>> -{
>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>> -
>> -    if (domain->type == IOMMU_DOMAIN_IDENTITY)
>> -            return iova;
>> -
>> -    if (!ops)
>> -            return 0;
>> -
>> -    return ops->iova_to_phys(ops, iova);
>> -}
>> -
>> -static struct platform_driver arm_smmu_driver;
>> -
>> -static
>> -struct arm_smmu_device *arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
>> -{
>> -    struct device *dev = 
>> driver_find_device_by_fwnode(&arm_smmu_driver.driver,
>> -                                                      fwnode);
>> -    put_device(dev);
>> -    return dev ? dev_get_drvdata(dev) : NULL;
>> -}
>> -
>>  static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>>  {
>>      unsigned long limit = smmu->strtab_cfg.num_l1_ents;
>> @@ -1701,206 +1527,6 @@ static bool arm_smmu_sid_in_range(struct 
>> arm_smmu_device *smmu, u32 sid)
>>      return sid < limit;
>>  }
>>  -static struct iommu_ops arm_smmu_ops;
>> -
>> -static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>> -{
> 
> Most of the code here looks useful to Xen. I think you want to keep the code 
> and re-use it afterwards.

Ok. I removed the code here and added the XEN compatible code to add devices in 
next patch.
I will keep it in this patch and will modifying the code to make XEN compatible.

> 
>> -    int i, ret;
>> -    struct arm_smmu_device *smmu;
>> -    struct arm_smmu_master *master;
>> -    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> -
>> -    if (!fwspec || fwspec->ops != &arm_smmu_ops)
>> -            return ERR_PTR(-ENODEV);
>> -
>> -    if (WARN_ON_ONCE(dev_iommu_priv_get(dev)))
>> -            return ERR_PTR(-EBUSY);
>> -
>> -    smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
>> -    if (!smmu)
>> -            return ERR_PTR(-ENODEV);
>> -
>> -    master = kzalloc(sizeof(*master), GFP_KERNEL);
>> -    if (!master)
>> -            return ERR_PTR(-ENOMEM);
>> -
>> -    master->dev = dev;
>> -    master->smmu = smmu;
>> -    master->sids = fwspec->ids;
>> -    master->num_sids = fwspec->num_ids;
>> -    dev_iommu_priv_set(dev, master);
>> -
>> -    /* Check the SIDs are in range of the SMMU and our stream table */
>> -    for (i = 0; i < master->num_sids; i++) {
>> -            u32 sid = master->sids[i];
>> -
>> -            if (!arm_smmu_sid_in_range(smmu, sid)) {
>> -                    ret = -ERANGE;
>> -                    goto err_free_master;
>> -            }
>> -
>> -            /* Ensure l2 strtab is initialised */
>> -            if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
>> -                    ret = arm_smmu_init_l2_strtab(smmu, sid);
>> -                    if (ret)
>> -                            goto err_free_master;
>> -            }
>> -    }
>> -
>> -    return &smmu->iommu;
>> -
>> -err_free_master:
>> -    kfree(master);
>> -    dev_iommu_priv_set(dev, NULL);
>> -    return ERR_PTR(ret);
>> -}
>> -
>> -static void arm_smmu_release_device(struct device *dev)
>> -{
>> -    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> -    struct arm_smmu_master *master;
>> -
>> -    if (!fwspec || fwspec->ops != &arm_smmu_ops)
>> -            return;
>> -
>> -    master = dev_iommu_priv_get(dev);
>> -    arm_smmu_detach_dev(master);
>> -    kfree(master);
>> -    iommu_fwspec_free(dev);
>> -}
>> -
>> -static struct iommu_group *arm_smmu_device_group(struct device *dev)
>> -{
>> -    struct iommu_group *group;
>> -
>> -    /*
>> -     * We don't support devices sharing stream IDs other than PCI RID
>> -     * aliases, since the necessary ID-to-device lookup becomes rather
>> -     * impractical given a potential sparse 32-bit stream ID space.
>> -     */
>> -    if (dev_is_pci(dev))
>> -            group = pci_device_group(dev);
>> -    else
>> -            group = generic_device_group(dev);
>> -
>> -    return group;
>> -}
>> -
>> -static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
>> -                                enum iommu_attr attr, void *data)
>> -{
>> -    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> -
>> -    switch (domain->type) {
>> -    case IOMMU_DOMAIN_UNMANAGED:
>> -            switch (attr) {
>> -            case DOMAIN_ATTR_NESTING:
>> -                    *(int *)data = (smmu_domain->stage == 
>> ARM_SMMU_DOMAIN_NESTED);
>> -                    return 0;
>> -            default:
>> -                    return -ENODEV;
>> -            }
>> -            break;
>> -    case IOMMU_DOMAIN_DMA:
>> -            switch (attr) {
>> -            case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
>> -                    *(int *)data = smmu_domain->non_strict;
>> -                    return 0;
>> -            default:
>> -                    return -ENODEV;
>> -            }
>> -            break;
>> -    default:
>> -            return -EINVAL;
>> -    }
>> -}
>> -
>> -static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
>> -                                enum iommu_attr attr, void *data)
>> -{
>> -    int ret = 0;
>> -    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> -
>> -    mutex_lock(&smmu_domain->init_mutex);
>> -
>> -    switch (domain->type) {
>> -    case IOMMU_DOMAIN_UNMANAGED:
>> -            switch (attr) {
>> -            case DOMAIN_ATTR_NESTING:
>> -                    if (smmu_domain->smmu) {
>> -                            ret = -EPERM;
>> -                            goto out_unlock;
>> -                    }
>> -
>> -                    if (*(int *)data)
>> -                            smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
>> -                    else
>> -                            smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>> -                    break;
>> -            default:
>> -                    ret = -ENODEV;
>> -            }
>> -            break;
>> -    case IOMMU_DOMAIN_DMA:
>> -            switch(attr) {
>> -            case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
>> -                    smmu_domain->non_strict = *(int *)data;
>> -                    break;
>> -            default:
>> -                    ret = -ENODEV;
>> -            }
>> -            break;
>> -    default:
>> -            ret = -EINVAL;
>> -    }
>> -
>> -out_unlock:
>> -    mutex_unlock(&smmu_domain->init_mutex);
>> -    return ret;
>> -}
>> -
>> -static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args 
>> *args)
>> -{
>> -    return iommu_fwspec_add_ids(dev, args->args, 1);
>> -}
>> -
>> -static void arm_smmu_get_resv_regions(struct device *dev,
>> -                                  struct list_head *head)
>> -{
>> -    struct iommu_resv_region *region;
>> -    int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>> -
>> -    region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
>> -                                     prot, IOMMU_RESV_SW_MSI);
>> -    if (!region)
>> -            return;
>> -
>> -    list_add_tail(&region->list, head);
>> -
>> -    iommu_dma_get_resv_regions(dev, head);
>> -}
>> -
>> -static struct iommu_ops arm_smmu_ops = {
>> -    .capable                = arm_smmu_capable,
>> -    .domain_alloc           = arm_smmu_domain_alloc,
>> -    .domain_free            = arm_smmu_domain_free,
>> -    .attach_dev             = arm_smmu_attach_dev,
>> -    .map                    = arm_smmu_map,
>> -    .unmap                  = arm_smmu_unmap,
>> -    .flush_iotlb_all        = arm_smmu_flush_iotlb_all,
>> -    .iotlb_sync             = arm_smmu_iotlb_sync,
>> -    .iova_to_phys           = arm_smmu_iova_to_phys,
>> -    .probe_device           = arm_smmu_probe_device,
>> -    .release_device         = arm_smmu_release_device,
>> -    .device_group           = arm_smmu_device_group,
>> -    .domain_get_attr        = arm_smmu_domain_get_attr,
>> -    .domain_set_attr        = arm_smmu_domain_set_attr,
>> -    .of_xlate               = arm_smmu_of_xlate,
>> -    .get_resv_regions       = arm_smmu_get_resv_regions,
>> -    .put_resv_regions       = generic_iommu_put_resv_regions,
>> -    .pgsize_bitmap          = -1UL, /* Restricted during device attach */
>> -};
>> -
>>  /* Probing and initialisation functions */
>>  static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>>                                 struct arm_smmu_queue *q,
>> @@ -2406,7 +2032,6 @@ static int arm_smmu_device_hw_probe(struct 
>> arm_smmu_device *smmu)
>>      switch (FIELD_GET(IDR0_STALL_MODEL, reg)) {
>>      case IDR0_STALL_MODEL_FORCE:
>>              smmu->features |= ARM_SMMU_FEAT_STALL_FORCE;
>> -            fallthrough;
> 
> We should keep all the fallthrough documented. So I think we want to 
> introduce the fallthrough in Xen as well.

Ok I will keep fallthrough documented in this patch. 

fallthrough implementation in XEN should be another patch. I am not sure when 
we can implement but we will try to implement.

> 
>>      case IDR0_STALL_MODEL_STALL:
>>              smmu->features |= ARM_SMMU_FEAT_STALLS;
>>      }
>> @@ -2426,7 +2051,6 @@ static int arm_smmu_device_hw_probe(struct 
>> arm_smmu_device *smmu)
>>      switch (FIELD_GET(IDR0_TTF, reg)) {
>>      case IDR0_TTF_AARCH32_64:
>>              smmu->ias = 40;
>> -            fallthrough;
>>      case IDR0_TTF_AARCH64:
>>              break;
>>      default:
>> @@ -2515,21 +2139,10 @@ static int arm_smmu_device_hw_probe(struct 
>> arm_smmu_device *smmu)
>>      default:
>>              dev_info(smmu->dev,
>>                      "unknown output address size. Truncating to 48-bit\n");
>> -            fallthrough;
>>      case IDR5_OAS_48_BIT:
>>              smmu->oas = 48;
>>      }
>>  -   if (arm_smmu_ops.pgsize_bitmap == -1UL)
>> -            arm_smmu_ops.pgsize_bitmap = smmu->pgsize_bitmap;
>> -    else
>> -            arm_smmu_ops.pgsize_bitmap |= smmu->pgsize_bitmap;
>> -
>> -    /* Set the DMA mask for our table walker */
>> -    if (dma_set_mask_and_coherent(smmu->dev, DMA_BIT_MASK(smmu->oas)))
>> -            dev_warn(smmu->dev,
>> -                     "failed to set DMA mask for table walker\n");
>> -
>>      smmu->ias = max(smmu->ias, smmu->oas);
>>      dev_info(smmu->dev, "ias %lu-bit, oas %lu-bit (features 0x%08x)\n",
>> @@ -2595,9 +2208,6 @@ static int arm_smmu_device_dt_probe(struct 
>> platform_device *pdev,
>>      parse_driver_options(smmu);
>>  -   if (of_dma_is_coherent(dev->of_node))
>> -            smmu->features |= ARM_SMMU_FEAT_COHERENCY;
>> -
>>      return ret;
>>  }
>>  @@ -2609,55 +2219,6 @@ static unsigned long arm_smmu_resource_size(struct 
>> arm_smmu_device *smmu)
>>              return SZ_128K;
>>  }
>>  -static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
>> -{
>> -    int err;
>> -
>> -#ifdef CONFIG_PCI
>> -    if (pci_bus_type.iommu_ops != ops) {
>> -            err = bus_set_iommu(&pci_bus_type, ops);
>> -            if (err)
>> -                    return err;
>> -    }
>> -#endif
>> -#ifdef CONFIG_ARM_AMBA
>> -    if (amba_bustype.iommu_ops != ops) {
>> -            err = bus_set_iommu(&amba_bustype, ops);
>> -            if (err)
>> -                    goto err_reset_pci_ops;
>> -    }
>> -#endif
>> -    if (platform_bus_type.iommu_ops != ops) {
>> -            err = bus_set_iommu(&platform_bus_type, ops);
>> -            if (err)
>> -                    goto err_reset_amba_ops;
>> -    }
>> -
>> -    return 0;
>> -
>> -err_reset_amba_ops:
>> -#ifdef CONFIG_ARM_AMBA
>> -    bus_set_iommu(&amba_bustype, NULL);
>> -#endif
>> -err_reset_pci_ops: __maybe_unused;
>> -#ifdef CONFIG_PCI
>> -    bus_set_iommu(&pci_bus_type, NULL);
>> -#endif
>> -    return err;
>> -}
>> -
>> -static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t 
>> start,
>> -                                  resource_size_t size)
> 
> This seems a bit odd that you are removing the function but not the callers. 
> Shouldn't you keep this function until next patch?
> 
Ok I will keep this in this patch and will remove in next patch.

>> -{
>> -    struct resource res = {
>> -            .flags = IORESOURCE_MEM,
>> -            .start = start,
>> -            .end = start + size - 1,
>> -    };
>> -
>> -    return devm_ioremap_resource(dev, &res);
>> -}
>> -
>>  static int arm_smmu_device_probe(struct platform_device *pdev)
>>  {
>>      int irq, ret;
>> @@ -2785,21 +2346,3 @@ static const struct of_device_id arm_smmu_of_match[] 
>> = {
>>      { .compatible = "arm,smmu-v3", },
>>      { },
>>  };
>> -MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>> -
>> -static struct platform_driver arm_smmu_driver = {
>> -    .driver = {
>> -            .name                   = "arm-smmu-v3",
>> -            .of_match_table         = arm_smmu_of_match,
>> -            .suppress_bind_attrs    = true,
>> -    },
>> -    .probe  = arm_smmu_device_probe,
>> -    .remove = arm_smmu_device_remove,
>> -    .shutdown = arm_smmu_device_shutdown,
>> -};
>> -module_platform_driver(arm_smmu_driver);
>> -
>> -MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
>> -MODULE_AUTHOR("Will Deacon <will@xxxxxxxxxx>");
>> -MODULE_ALIAS("platform:arm-smmu-v3");
>> -MODULE_LICENSE("GPL v2");
> 
> -- 
> Julien Grall




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.