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

Re: [Xen-devel] [RFC v3 4/4] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver




On 12/5/2017 7:17 AM, Julien Grall wrote:
> Hello,
> 
> On 05/12/17 03:59, Sameer Goel wrote:
>> This driver follows an approach similar to smmu driver. The intent here
>> is to reuse as much Linux code as possible.
>> - Glue code has been introduced in headers to bridge the API calls.
>> - Called Linux functions from the Xen IOMMU function calls.
>> - Xen modifications are preceded by /*Xen: comment */
>> - New config items for SMMUv3 and legacy SMMU have been defined.
> 
> There are no reason to touch legacy SMMU in this patch. Please move that 
> outside of it.
Ok.
> 
>>
>> Signed-off-by: Sameer Goel <sgoel@xxxxxxxxxxxxxx>
>> ---
>>   xen/drivers/Kconfig                    |   2 +
>>   xen/drivers/passthrough/arm/Kconfig    |  14 +
>>   xen/drivers/passthrough/arm/Makefile   |   3 +-
>>   xen/drivers/passthrough/arm/arm_smmu.h | 189 ++++++++++
>>   xen/drivers/passthrough/arm/smmu-v3.c  | 619 
>> ++++++++++++++++++++++++++++++---
>>   5 files changed, 768 insertions(+), 59 deletions(-)
>>   create mode 100644 xen/drivers/passthrough/arm/Kconfig
>>   create mode 100644 xen/drivers/passthrough/arm/arm_smmu.h
>>
>> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
>> index bc3a54f..6126553 100644
>> --- a/xen/drivers/Kconfig
>> +++ b/xen/drivers/Kconfig
>> @@ -12,4 +12,6 @@ source "drivers/pci/Kconfig"
>>     source "drivers/video/Kconfig"
>>   +source "drivers/passthrough/arm/Kconfig"
>> +
>>   endmenu
>> diff --git a/xen/drivers/passthrough/arm/Kconfig 
>> b/xen/drivers/passthrough/arm/Kconfig
>> new file mode 100644
>> index 0000000..9ac4cea
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/arm/Kconfig
>> @@ -0,0 +1,14 @@
>> +
>> +config ARM_SMMU
>> +    bool "ARM SMMU v1/2 support"
>> +    depends on ARM_64
> 
> Why? SMMUv1 and SMMUv2 supports Arm 32-bit.
> 
>> +    help
>> +     Support for implementations of the ARM System MMU architecture. (1/2)
> 
> I am not sure to understand the (1/2) after the final point.
> 
>> +
>> +config ARM_SMMU_v3
>> +    bool "ARM SMMUv3 Support"
>> +    depends on ARM_64
>> +    help
>> +     Support for implementations of the ARM System MMU architecture
>> +     version 3.
>> +
>> diff --git a/xen/drivers/passthrough/arm/Makefile 
>> b/xen/drivers/passthrough/arm/Makefile
>> index f4cd26e..5b3eb15 100644
>> --- a/xen/drivers/passthrough/arm/Makefile
>> +++ b/xen/drivers/passthrough/arm/Makefile
>> @@ -1,2 +1,3 @@
>>   obj-y += iommu.o
>> -obj-y += smmu.o
>> +obj-$(CONFIG_ARM_SMMU) += smmu.o
>> +obj-$(CONFIG_ARM_SMMU_v3) += smmu-v3.o
>> diff --git a/xen/drivers/passthrough/arm/arm_smmu.h 
>> b/xen/drivers/passthrough/arm/arm_smmu.h
>> new file mode 100644
>> index 0000000..b5e161f
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/arm/arm_smmu.h
> 
> I don't think there are any value to use Linux coding style in this header. 
> It contains Xen stubs.
> 
> I would also have expected this new file to come in a separate patch with the 
> modification associated in SMMUv2. This would make easier to see what could 
> be common.
That makes sense. I was holding it back till I post the first actual patch and 
just wanted to put out the SMMUv3patches.

> 
>> @@ -0,0 +1,189 @@
>> +/******************************************************************************
>> + * ./arm_smmu.h
>> + *
>> + * Common compatibility defines and data_structures for porting arm smmu
>> + * drivers from Linux.
> 
> [...]
> 
>> +static struct resource *platform_get_resource(struct platform_device *pdev,
>> +                          unsigned int type,
>> +                          unsigned int num)
>> +{
>> +    /*
>> +     * The resource is only used between 2 calls of platform_get_resource.
>> +     * It's quite ugly but it's avoid to add too much code in the part
>> +     * imported from Linux
>> +     */
>> +    static struct resource res;
>> +    struct acpi_iort_node *iort_node;
>> +    struct acpi_iort_smmu_v3 *node_smmu_data;
>> +    int ret = 0;
>> +
>> +    res.type = type;
>> +
>> +    switch (type) {
>> +    case IORESOURCE_MEM:
>> +        if (pdev->type == DEV_ACPI) {
>> +            ret = 1;
>> +            iort_node = pdev->acpi_node;
>> +            node_smmu_data =
>> +                (struct acpi_iort_smmu_v3 *)iort_node->node_data;
> 
> Above you say: "Common compatibility defines and data_structures for porting 
> arm smmu driver from Linux". But this code is clearly SMMUv3.
> 
It is. I will pull this in the SMMUv3 driver.
>> +
>> +            if (node_smmu_data != NULL) {
>> +                res.addr = node_smmu_data->base_address;
>> +                res.size = SZ_128K;
>> +                ret = 0;
>> +            }
>> +        } else {
>> +            ret = dt_device_get_address(dev_to_dt(pdev), num,
>> +                            &res.addr, &res.size);
>> +        }
>> +
>> +        return ((ret) ? NULL : &res);
>> +
>> +    case IORESOURCE_IRQ:
>> +        /* ACPI case not implemented as there is no use case for it */
>> +        ret = platform_get_irq(dev_to_dt(pdev), num);
>> +
>> +        if (ret < 0)
>> +            return NULL;
>> +
>> +        res.addr = ret;
>> +        res.size = 1;
>> +
>> +        return &res;
>> +
>> +    default:
>> +        return NULL;
>> +    }
>> +}
>> +
>> +static int platform_get_irq_byname(struct platform_device *pdev, const char 
>> *name)
>> +{
>> +    const struct dt_property *dtprop;
>> +    struct acpi_iort_node *iort_node;
>> +    struct acpi_iort_smmu_v3 *node_smmu_data;
>> +    int ret = 0;
>> +
>> +    if (pdev->type == DEV_ACPI) {
>> +        iort_node = pdev->acpi_node;
>> +        node_smmu_data = (struct acpi_iort_smmu_v3 *)iort_node->node_data;
> 
> Ditto.
> 
>> +
>> +        if (node_smmu_data != NULL) {
>> +            if (!strcmp(name, "eventq"))
>> +                ret = node_smmu_data->event_gsiv;
>> +            else if (!strcmp(name, "priq"))
>> +                ret = node_smmu_data->pri_gsiv;
>> +            else if (!strcmp(name, "cmdq-sync"))
>> +                ret = node_smmu_data->sync_gsiv;
>> +            else if (!strcmp(name, "gerror"))
>> +                ret = node_smmu_data->gerr_gsiv;
>> +            else
>> +                ret = -EINVAL;
>> +        }
>> +    } else {
>> +        dtprop = dt_find_property(dev_to_dt(pdev), "interrupt-names", NULL);
>> +        if (!dtprop)
>> +            return -EINVAL;
>> +
>> +        if (!dtprop->value)
>> +            return -ENODATA;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +/* Xen: Stub out DMA domain related functions */
> 
> I don't think 'Xen:' is necessary as this file contains Xen stubs.
Ok.
> 
>> +#define iommu_get_dma_cookie(dom) 0
>> +#define iommu_put_dma_cookie(dom) 0
>> +
>> +static void __iomem *devm_ioremap_resource(struct device *dev,
>> +                       struct resource *res)
>> +{
>> +    void __iomem *ptr;
>> +
>> +    if (!res || res->type != IORESOURCE_MEM) {
>> +        dev_err(dev, "Invalid resource\n");
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    ptr = ioremap_nocache(res->addr, res->size);
>> +    if (!ptr) {
>> +        dev_err(dev,
>> +            "ioremap failed (addr 0x%"PRIx64" size 0x%"PRIx64")\n",
>> +            res->addr, res->size);
>> +        return ERR_PTR(-ENOMEM);
>> +    }
>> +
>> +    return ptr;
>> +}
>> +
>> +/* Xen: Dummy iommu_domain */
>> +struct iommu_domain {
>> +    /* Runtime SMMU configuration for this iommu_domain */
>> +    struct arm_smmu_domain        *priv;
>> +    unsigned int            type;
> 
> What are the values for type?
> 
>> +
>> +    atomic_t ref;
>> +    /* Used to link iommu_domain contexts for a same domain.
> 
> /*
>  * Used ...
>  */
> 
>> +     * There is at least one per-SMMU to used by the domain.
>> +     */
>> +    struct list_head        list;
>> +};
>> +/* Xen: Domain type definitions. Not really needed for Xen, defining to port
> 
> /*
>  * Xen: ...
> 
>> + * Linux code as-is
>> + */
>> +#define IOMMU_DOMAIN_UNMANAGED 0
>> +#define IOMMU_DOMAIN_DMA 1
>> +#define IOMMU_DOMAIN_IDENTITY 2
>> +
>> +/* Xen: Describes information required for a Xen domain */
>> +struct arm_smmu_xen_domain {
>> +    spinlock_t            lock;
>> +    /* List of iommu domains associated to this domain */
>> +    struct list_head        iommu_domains;
>> +};
>> +
>> +/*
>> + * Xen: Information about each device stored in dev->archdata.iommu
>> + *
>> + * The dev->archdata.iommu stores the iommu_domain (runtime configuration of
>> + * the SMMU).
>> + */
>> +struct arm_smmu_xen_device {
>> +    struct iommu_domain *domain;
>> +};
>> +
>> +#endif /* __ARM_SMMU_H__ */
> 
> Missing emacs magic.
> 
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index e67ba6c..c6c1b99 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -18,28 +18,38 @@
>>    * Author: Will Deacon <will.deacon@xxxxxxx>
>>    *
>>    * This driver is powered by bad coffee and bombay mix.
>> + *
>> + *
>> + * Based on Linux drivers/iommu/arm-smmu-v3.c
>> + * => commit 7aa8619a66aea52b145e04cbab4f8d6a4e5f3f3b
>> + *
>> + * Xen modifications:
>> + * Sameer Goel <sameer.goel@xxxxxxxxxx>
>> + * Copyright (C) 2017, The Linux Foundation, All rights reserved.
>> + *
>>    */
>>   -#include <linux/acpi.h>
>> -#include <linux/acpi_iort.h>
>> -#include <linux/delay.h>
>> -#include <linux/dma-iommu.h>
>> -#include <linux/err.h>
>> -#include <linux/interrupt.h>
>> -#include <linux/iommu.h>
>> -#include <linux/iopoll.h>
>> -#include <linux/module.h>
>> -#include <linux/msi.h>
>> -#include <linux/of.h>
>> -#include <linux/of_address.h>
>> -#include <linux/of_iommu.h>
>> -#include <linux/of_platform.h>
>> -#include <linux/pci.h>
>> -#include <linux/platform_device.h>
>> -
>> -#include <linux/amba/bus.h>
>> -
>> -#include "io-pgtable.h"
>> +#include <xen/acpi.h>
>> +#include <xen/config.h>
>> +#include <xen/delay.h>
>> +#include <xen/errno.h>
>> +#include <xen/err.h>
>> +#include <xen/irq.h>
>> +#include <xen/lib.h>
>> +#include <xen/linux_compat.h>
>> +#include <xen/list.h>
>> +#include <xen/mm.h>
>> +#include <xen/rbtree.h>
>> +#include <xen/sched.h>
>> +#include <xen/sizes.h>
>> +#include <xen/vmap.h>
>> +#include <acpi/acpi_iort.h>
>> +#include <asm/atomic.h>
>> +#include <asm/device.h>
>> +#include <asm/io.h>
>> +#include <asm/platform.h>
>> +
>> +#include "arm_smmu.h" /* Not a self contained header. So last in the list */
>>     /* MMIO registers */
>>   #define ARM_SMMU_IDR0            0x0
>> @@ -423,9 +433,12 @@
>>   #endif
>>     static bool disable_bypass;
>> +
>> +#if 0 /* Xen: Not applicable for Xen */
>>   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.");
>> +#endif
> 
> Can't you stub module_param_namde and MODULE_PARM_DESC to avoid #if 0?
> 
>>     enum pri_resp {
>>       PRI_RESP_DENY,
>> @@ -433,6 +446,7 @@ enum pri_resp {
>>       PRI_RESP_SUCC,
>>   };
>>   +#if 0 /* Xen: No MSI support in this iteration */
>>   enum arm_smmu_msi_index {
>>       EVTQ_MSI_INDEX,
>>       GERROR_MSI_INDEX,
>> @@ -457,6 +471,7 @@ static phys_addr_t 
>> arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
>>           ARM_SMMU_PRIQ_IRQ_CFG2,
>>       },
>>   };
>> +#endif
>>     struct arm_smmu_cmdq_ent {
>>       /* Common fields */
>> @@ -561,6 +576,8 @@ struct arm_smmu_s2_cfg {
>>       u16                vmid;
>>       u64                vttbr;
>>       u64                vtcr;
>> +    /* Xen: Domain associated to this configuration */
>> +    struct domain            *domain;
>>   };
>>     struct arm_smmu_strtab_ent {
>> @@ -635,9 +652,21 @@ struct arm_smmu_device {
>>       struct arm_smmu_strtab_cfg    strtab_cfg;
>>         /* IOMMU core code handle */
>> +#if 0 /*Xen: Generic iommu_device ref not needed here */
>>       struct iommu_device        iommu;
>> +#endif
>> +    /* Xen: Need to keep a list of SMMU devices */
>> +    struct list_head                devices;
>>   };
>>   +/* Xen: Keep a list of devices associated with this driver */
>> +static DEFINE_SPINLOCK(arm_smmu_devices_lock);
>> +static LIST_HEAD(arm_smmu_devices);
>> +/* Xen: Helper for finding a device using fwnode */
>> +static
>> +struct arm_smmu_device *arm_smmu_get_by_fwnode(struct fwnode_handle 
>> *fwnode);
>> +
>> +
>>   /* SMMU private data for each master */
>>   struct arm_smmu_master_data {
>>       struct arm_smmu_device        *smmu;
>> @@ -654,7 +683,7 @@ enum arm_smmu_domain_stage {
>>     struct arm_smmu_domain {
>>       struct arm_smmu_device        *smmu;
>> -    struct mutex            init_mutex; /* Protects smmu pointer */
>> +    mutex            init_mutex; /* Protects smmu pointer */
>>         struct io_pgtable_ops        *pgtbl_ops;
>>   @@ -961,6 +990,7 @@ static void arm_smmu_cmdq_issue_cmd(struct 
>> arm_smmu_device *smmu,
>>       spin_unlock_irqrestore(&smmu->cmdq.lock, flags);
>>   }
>>   +#if 0 /*Xen: Comment out functions that set up S1 translations */
> 
> Why? I do agree that the code will not be used by Xen, but I would prefer if 
> you minimize the number of #ifdef.
> 
>>   /* Context descriptor manipulation functions */
>>   static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr)
>>   {
>> @@ -1003,6 +1033,7 @@ static void arm_smmu_write_ctx_desc(struct 
>> arm_smmu_device *smmu,
>>         cfg->cdptr[3] = cpu_to_le64(cfg->cd.mair << CTXDESC_CD_3_MAIR_SHIFT);
>>   }
>> +#endif
>>     /* Stream table manipulation functions */
>>   static void
>> @@ -1164,6 +1195,7 @@ static int arm_smmu_init_l2_strtab(struct 
>> arm_smmu_device *smmu, u32 sid)
>>       void *strtab;
>>       struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>>       struct arm_smmu_strtab_l1_desc *desc = &cfg->l1_desc[sid >> 
>> STRTAB_SPLIT];
>> +    u32 alignment = 0;
> 
> It is not necassary to initialize alignment. Also we are trying to limit the 
> use of u* in favor of uint32_t.
> 
>>         if (desc->l2ptr)
>>           return 0;
>> @@ -1172,14 +1204,16 @@ static int arm_smmu_init_l2_strtab(struct 
>> arm_smmu_device *smmu, u32 sid)
>>       strtab = &cfg->strtab[(sid >> STRTAB_SPLIT) * STRTAB_L1_DESC_DWORDS];
>>         desc->span = STRTAB_SPLIT + 1;
>> -    desc->l2ptr = dmam_alloc_coherent(smmu->dev, size, &desc->l2ptr_dma,
>> -                      GFP_KERNEL | __GFP_ZERO);
>> +    /* Alignment picked from ARM SMMU arch version 3.x. L1ST.L2Ptr */
>> +    alignment = 1 << ((5 + (desc->span - 1)));
>> +    desc->l2ptr = _xzalloc(size, alignment);
>>       if (!desc->l2ptr) {
>>           dev_err(smmu->dev,
>>               "failed to allocate l2 stream table for SID %u\n",
>>               sid);
>>           return -ENOMEM;
>>       }
>> +    desc->l2ptr_dma = virt_to_maddr(desc->l2ptr);
>>         arm_smmu_init_bypass_stes(desc->l2ptr, 1 << STRTAB_SPLIT);
>>       arm_smmu_write_strtab_l1_desc(strtab, desc);
>> @@ -1232,7 +1266,7 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device 
>> *smmu, u64 *evt)
>>         dev_info(smmu->dev, "unexpected PRI request received:\n");
>>       dev_info(smmu->dev,
>> -         "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova 
>> 0x%016llx\n",
>> +         "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova 
>> %#" PRIx64 "\n",
>>            sid, ssid, grpid, last ? "L" : "",
>>            evt[0] & PRIQ_0_PERM_PRIV ? "" : "un",
>>            evt[0] & PRIQ_0_PERM_READ ? "R" : "",
>> @@ -1346,6 +1380,8 @@ static irqreturn_t arm_smmu_combined_irq_handler(int 
>> irq, void *dev)
>>   {
>>       arm_smmu_gerror_handler(irq, dev);
>>       arm_smmu_cmdq_sync_handler(irq, dev);
>> +    /*Xen: No threaded irq. So call the required function from here */
>> +    arm_smmu_combined_irq_thread(irq, dev);
>>       return IRQ_WAKE_THREAD;
>>   }
>>   @@ -1358,11 +1394,49 @@ static void __arm_smmu_tlb_sync(struct 
>> arm_smmu_device *smmu)
>>       arm_smmu_cmdq_issue_cmd(smmu, &cmd);
>>   }
>>   +static void arm_smmu_evtq_thread_xen(int irq, void *dev,
>> +                       struct cpu_user_regs *regs)
>> +{
>> +    arm_smmu_evtq_thread(irq, dev);
>> +}
>> +
>> +static void arm_smmu_priq_thread_xen(int irq, void *dev,
>> +                       struct cpu_user_regs *regs)
>> +{
>> +    arm_smmu_priq_thread(irq, dev);
>> +}
>> +
>> +static void arm_smmu_cmdq_sync_handler_xen(int irq, void *dev,
>> +                       struct cpu_user_regs *regs)
>> +{
>> +    arm_smmu_cmdq_sync_handler(irq, dev);
>> +}
>> +
>> +static void arm_smmu_gerror_handler_xen(int irq, void *dev,
>> +                       struct cpu_user_regs *regs)
>> +{
>> +    arm_smmu_gerror_handler(irq, dev);
>> +}
>> +
>> +static void arm_smmu_combined_irq_handler_xen(int irq, void *dev,
>> +                       struct cpu_user_regs *regs)
>> +{
>> +    arm_smmu_combined_irq_handler(irq, dev);
>> +}
>> +
> 
> Missing:
> /* Xen: .... */
> 
>> +#define arm_smmu_evtq_thread arm_smmu_evtq_thread_xen
>> +#define arm_smmu_priq_thread arm_smmu_priq_thread_xen
>> +#define arm_smmu_cmdq_sync_handler arm_smmu_cmdq_sync_handler_xen
>> +#define arm_smmu_gerror_handler arm_smmu_gerror_handler_xen
>> +#define arm_smmu_combined_irq_handler arm_smmu_combined_irq_handler_xen
>> +
>> +#if 0 /*Xen: Unused function */
>>   static void arm_smmu_tlb_sync(void *cookie)
>>   {
>>       struct arm_smmu_domain *smmu_domain = cookie;
>>       __arm_smmu_tlb_sync(smmu_domain->smmu);
>>   }
>> +#endif
>>     static void arm_smmu_tlb_inv_context(void *cookie)
>>   {
>> @@ -1383,6 +1457,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
>>       __arm_smmu_tlb_sync(smmu);
>>   }
>>   +#if 0 /*Xen: Unused functionality */
>>   static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>>                         size_t granule, bool leaf, void *cookie)
>>   {
>> @@ -1427,6 +1502,7 @@ static bool arm_smmu_capable(enum iommu_cap cap)
>>           return false;
>>       }
>>   }
>> +#endif
>>     static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>>   {
>> @@ -1474,6 +1550,7 @@ static void arm_smmu_bitmap_free(unsigned long *map, 
>> int idx)
>>       clear_bit(idx, map);
>>   }
>>   +#if 0
>>   static void arm_smmu_domain_free(struct iommu_domain *domain)
>>   {
>>       struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> @@ -1502,7 +1579,23 @@ static void arm_smmu_domain_free(struct iommu_domain 
>> *domain)
>>         kfree(smmu_domain);
>>   }
>> +#endif
>> +
>> +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_s2_cfg *cfg = &smmu_domain->s2_cfg;
>> +    /*
>> +     * Xen: Remove the free functions that are not used and code related
>> +     * to S1 translation. We just need to free the domain and vmid here.
>> +     */
> 
> Can you please give a reason to remove stage-1 code? This is not in the 
> spririt of a verbatim port and I still can't see why you can't keep it.

I was just clearing it out as it was not used. I will put it back in.

> 
>> +    if (cfg->vmid)
>> +        arm_smmu_bitmap_free(smmu->vmid_map, cfg->vmid);
>> +    kfree(smmu_domain);
>> +}
>>   +#if 0 /*Xen: The finalize domain functions are not needed in current form 
>> */
>>   static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
>>                          struct io_pgtable_cfg *pgtbl_cfg)
>>   {
>> @@ -1551,16 +1644,41 @@ static int arm_smmu_domain_finalise_s2(struct 
>> arm_smmu_domain *smmu_domain,
>>       cfg->vtcr    = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>>       return 0;
>>   }
>> +#endif
>> +
>> +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;
>> +
>> +    vmid = arm_smmu_bitmap_alloc(smmu->vmid_map, smmu->vmid_bits);
>> +    if (vmid < 0)
>> +        return vmid;
>> +
>> +    /* Xen: Get the ttbr and vtcr values
> 
> /*
>  * Xen: ...
> 
> But why do you need to duplicate the function when you can just replace the 2 
> lines that needs to be modified?
> 
>> +     * vttbr: This is a shared value with the domain page table
>> +     * vtcr: The TCR settings are the same as CPU since he page
> s/he/the/
> 
>> +     * tables are shared
>> +     */
>> +
>> +    cfg->vmid    = vmid;
>> +    cfg->vttbr    = page_to_maddr(cfg->domain->arch.p2m.root);
>> +    cfg->vtcr    = READ_SYSREG32(VTCR_EL2) & STRTAB_STE_2_VTCR_MASK;
> 
> I still think this is really fragile. You at least need a comment on the 
> other side (e.g where VTCR_EL2 is written) to explain you are relying the 
> value in other places.
I can add the comment. 

> 
>> +    return 0;
>> +}
>>     static int arm_smmu_domain_finalise(struct iommu_domain *domain)
>>   {
>>       int ret;
>> +#if 0    /* Xen: pgtbl_cfg not needed. So modify the function as needed */
>>       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 io_pgtable_cfg *);
>> +#endif
>>       struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>       struct arm_smmu_device *smmu = smmu_domain->smmu;
>>   @@ -1575,6 +1693,7 @@ static int arm_smmu_domain_finalise(struct 
>> iommu_domain *domain)
>>       if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>>           smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>>   +#if 0
>>       switch (smmu_domain->stage) {
>>       case ARM_SMMU_DOMAIN_S1:
>>           ias = VA_BITS;
>> @@ -1616,7 +1735,9 @@ static int arm_smmu_domain_finalise(struct 
>> iommu_domain *domain)
>>       ret = finalise_stage_fn(smmu_domain, &pgtbl_cfg);
>>       if (ret < 0)
>>           free_io_pgtable_ops(pgtbl_ops);
>> +#endif
>>   +    ret = arm_smmu_domain_finalise_s2(smmu_domain);
>>       return ret;
>>   }
>>   @@ -1709,7 +1830,9 @@ static int arm_smmu_attach_dev(struct iommu_domain 
>> *domain, struct device *dev)
>>       } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>>           ste->s1_cfg = &smmu_domain->s1_cfg;
>>           ste->s2_cfg = NULL;
>> +#if 0 /*Xen: S1 configuratio not needed */
> 
> What would be the issue to let this code uncommented
> 
>>           arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);
>> +#endif
>>       } else {
>>           ste->s1_cfg = NULL;
>>           ste->s2_cfg = &smmu_domain->s2_cfg;
>> @@ -1721,6 +1844,7 @@ out_unlock:
>>       return ret;
>>   }
>>    > +#if 0
> 
> /* Xen: ... */
> 
>>   static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>               phys_addr_t paddr, size_t size, int prot)
>>   {
>> @@ -1772,6 +1896,7 @@ struct arm_smmu_device *arm_smmu_get_by_fwnode(struct 
>> fwnode_handle *fwnode)
>>       put_device(dev);
>>       return dev ? dev_get_drvdata(dev) : NULL;
>>   }
>> +#endif
>>     static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>>   {
>> @@ -1782,8 +1907,9 @@ static bool arm_smmu_sid_in_range(struct 
>> arm_smmu_device *smmu, u32 sid)
>>         return sid < limit;
>>   }
>> -
> 
> Please don't remove newline.
> 
>> +#if 0
>>   static struct iommu_ops arm_smmu_ops;
>> +#endif
>>     static int arm_smmu_add_device(struct device *dev)
>>   {
>> @@ -1791,9 +1917,12 @@ static int arm_smmu_add_device(struct device *dev)
>>       struct arm_smmu_device *smmu;
>>       struct arm_smmu_master_data *master;
>>       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +#if 0 /*Xen: iommu_group is not needed */
>>       struct iommu_group *group;
>> +#endif
>>   -    if (!fwspec || fwspec->ops != &arm_smmu_ops)
>> +    /* Xen: fwspec->ops are not needed */
>> +    if (!fwspec)
>>           return -ENODEV;
>>       /*
>>        * We _can_ actually withstand dodgy bus code re-calling add_device()
>> @@ -1830,6 +1959,12 @@ static int arm_smmu_add_device(struct device *dev)
>>           }
>>       }
>>   +#if 0
>> +/*
>> + * Xen: Do not need an iommu group as the stream data is carried by the SMMU
>> + * master device object
>> + */
> 
> This is better to put before #if 0. So IDE will still show the comment even 
> when #if 0 is fold.
> 
>> +
>>       group = iommu_group_get_for_dev(dev);
>>       if (!IS_ERR(group)) {
>>           iommu_group_put(group);
>> @@ -1837,8 +1972,16 @@ static int arm_smmu_add_device(struct device *dev)
>>       }
>>         return PTR_ERR_OR_ZERO(group);
>> +#endif
>> +    return 0;
>>   }
>>   +/*
>> + * Xen: We can potentially support this function and destroy a device. This
>> + * will be relevant for PCI hotplug. So, will be implemented as needed after
>> + * passthrough support is available.
>> + */
>> +#if 0
>>   static void arm_smmu_remove_device(struct device *dev)
>>   {
>>       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> @@ -1974,7 +2117,7 @@ static struct iommu_ops arm_smmu_ops = {
>>       .put_resv_regions    = arm_smmu_put_resv_regions,
>>       .pgsize_bitmap        = -1UL, /* Restricted during device attach */
>>   };
>> -
> 
> Ditto for the newline. I know I didn't mention it in every place in the 
> previous series. But I would have expected you to apply my comments 
> everywhere.
> 
>> +#endif
>>   /* Probing and initialisation functions */
>>   static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>>                      struct arm_smmu_queue *q,
>> @@ -1984,13 +2127,19 @@ static int arm_smmu_init_one_queue(struct 
>> arm_smmu_device *smmu,
>>   {
>>       size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
>>   -    q->base = dmam_alloc_coherent(smmu->dev, qsz, &q->base_dma, 
>> GFP_KERNEL);
>> +    /* The SMMU cache coherency property is always set. Since we are 
>> sharing the CPU translation tables
> 
> /*
>  * ...
> 
>> +     * just make a regular allocation.
> 
> I am not sure to understand it. AFAIU, q is for the command queue. So how 
> sharing the CPU translation tables will help here?
> 
> Furthermore, I don't understand how you can say cache coherency property is 
> always set? When I look at the driver, it seems to be able to handle 
> non-coherent memory. So where do you modify that?
> 
>> +     */
>> +    q->base = _xzalloc(qsz, sizeof(void *));
>> +
>>       if (!q->base) {
>>           dev_err(smmu->dev, "failed to allocate queue (0x%zx bytes)\n",
>>               qsz);
>>           return -ENOMEM;
>>       }
>>   +    q->base_dma = virt_to_maddr(q->base);
>> +
>>       q->prod_reg    = arm_smmu_page1_fixup(prod_off, smmu);
>>       q->cons_reg    = arm_smmu_page1_fixup(cons_off, smmu);
>>       q->ent_dwords    = dwords;
>> @@ -2056,6 +2205,7 @@ static int arm_smmu_init_strtab_2lvl(struct 
>> arm_smmu_device *smmu)
>>       u64 reg;
>>       u32 size, l1size;
>>       struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>> +    u32 alignment;
>>         /* Calculate the L1 size, capped to the SIDSIZE. */
>>       size = STRTAB_L1_SZ_SHIFT - (ilog2(STRTAB_L1_DESC_DWORDS) + 3);
>> @@ -2069,14 +2219,17 @@ static int arm_smmu_init_strtab_2lvl(struct 
>> arm_smmu_device *smmu)
>>                size, smmu->sid_bits);
>>         l1size = cfg->num_l1_ents * (STRTAB_L1_DESC_DWORDS << 3);
>> -    strtab = dmam_alloc_coherent(smmu->dev, l1size, &cfg->strtab_dma,
>> -                     GFP_KERNEL | __GFP_ZERO);
>> +    alignment = max_t(u32, cfg->num_l1_ents, 64);
> 
> Same as before. I know I didn't go through the rest of the code. But you 
> could have at least applied my comments on alignment here too. E.g where does 
> the 64 come from?
> 
> But, it looks like to me you want to create a function dmam_alloc_coherent 
> that will do the allocation for you. This could be used in a few places 
> within file driver...
dmam_alloc_coherent uses the allocation size as the alignment. This is not as 
per spec. But that being said I am fine replicating the code from Linux. That 
will make my life easier :).

> 
>> +    strtab = _xzalloc(l1size, l1size);
>> +
>>       if (!strtab) {
>>           dev_err(smmu->dev,
>>               "failed to allocate l1 stream table (%u bytes)\n",
>>               size);
>>           return -ENOMEM;
>>       }
>> +
>> +    cfg->strtab_dma = virt_to_maddr(strtab);
>>       cfg->strtab = strtab;
>>         /* Configure strtab_base_cfg for 2 levels */
>> @@ -2098,14 +2251,16 @@ static int arm_smmu_init_strtab_linear(struct 
>> arm_smmu_device *smmu)
>>       struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>>         size = (1 << smmu->sid_bits) * (STRTAB_STE_DWORDS << 3);
>> -    strtab = dmam_alloc_coherent(smmu->dev, size, &cfg->strtab_dma,
>> -                     GFP_KERNEL | __GFP_ZERO);
> 
> ... such as here.
> 
>> +    strtab = _xzalloc(size, size);
> 
> Hmmm, _xzalloc contains the following assert:
> 
> ASSERT((align & (align - 1)) == 0);
> 
> How are you sure the size will always honor this check?
I can add another check or add a comment. Till now the size has passed this 
check.
> 
>> +
>>       if (!strtab) {
>>           dev_err(smmu->dev,
>>               "failed to allocate linear stream table (%u bytes)\n",
>>               size);
>>           return -ENOMEM;
>>       }
>> +
>> +    cfg->strtab_dma = virt_to_maddr(strtab);
>>       cfg->strtab = strtab;
>>       cfg->num_l1_ents = 1 << smmu->sid_bits;
>>   @@ -2182,6 +2337,7 @@ static int arm_smmu_update_gbpa(struct 
>> arm_smmu_device *smmu, u32 set, u32 clr)
>>                         1, ARM_SMMU_POLL_TIMEOUT_US);
>>   }
>>   +#if 0 /* Xen: There is no MSI support as yet */
>>   static void arm_smmu_free_msis(void *data)
>>   {
>>       struct device *dev = data;
>> @@ -2247,36 +2403,39 @@ static void arm_smmu_setup_msis(struct 
>> arm_smmu_device *smmu)
>>       /* Add callback to free MSIs on teardown */
>>       devm_add_action(dev, arm_smmu_free_msis, dev);
>>   }
>> +#endif
>>     static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>>   {
>>       int irq, ret;
>>   +#if 0 /*Xen: Cannot setup msis for now */
>>       arm_smmu_setup_msis(smmu);
>> +#endif
>>         /* Request interrupt lines */
>>       irq = smmu->evtq.q.irq;
>>       if (irq) {
>> -        ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>> -                        arm_smmu_evtq_thread,
>> -                        IRQF_ONESHOT,
>> -                        "arm-smmu-v3-evtq", smmu);
>> +        irq_set_type(irq, IRQ_TYPE_EDGE_BOTH);
> 
> Why do you need to set the IRQ type? Can't it be found from the firmware 
> tables?
> 
>> +        ret = request_irq(irq, arm_smmu_evtq_thread,
>> +                        0, "arm-smmu-v3-evtq", smmu);
> 
> Please create a stub for devm_request_threaded_irq.
> 
>>           if (ret < 0)
>>               dev_warn(smmu->dev, "failed to enable evtq irq\n");
>>       }
>>         irq = smmu->cmdq.q.irq;
>>       if (irq) {
>> -        ret = devm_request_irq(smmu->dev, irq,
>> -                       arm_smmu_cmdq_sync_handler, 0,
>> -                       "arm-smmu-v3-cmdq-sync", smmu);
>> +        irq_set_type(irq, IRQ_TYPE_EDGE_BOTH);
>> +        ret = request_irq(irq, arm_smmu_cmdq_sync_handler,
>> +                0, "arm-smmu-v3-cmdq-sync", smmu);
> 
> Ditto.
> 
>>           if (ret < 0)
>>               dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n");
>>       }
>>         irq = smmu->gerr_irq;
>>       if (irq) {
>> -        ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
>> +        irq_set_type(irq, IRQ_TYPE_EDGE_BOTH);
>> +        ret = request_irq(irq, arm_smmu_gerror_handler,
>>                          0, "arm-smmu-v3-gerror", smmu);
> 
> Ditto.
> 
>>           if (ret < 0)
>>               dev_warn(smmu->dev, "failed to enable gerror irq\n");
>> @@ -2284,12 +2443,13 @@ static void arm_smmu_setup_unique_irqs(struct 
>> arm_smmu_device *smmu)
>>         if (smmu->features & ARM_SMMU_FEAT_PRI) {
>>           irq = smmu->priq.q.irq;
>> +        irq_set_type(irq, IRQ_TYPE_EDGE_BOTH);
>>           if (irq) {
>> -            ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>> -                            arm_smmu_priq_thread,
>> -                            IRQF_ONESHOT,
>> -                            "arm-smmu-v3-priq",
>> -                            smmu);
>> +            ret = request_irq(irq,
>> +                      arm_smmu_priq_thread,
>> +                      0,
>> +                      "arm-smmu-v3-priq",
>> +                      smmu);
> 
> Ditto.
> 
>>               if (ret < 0)
>>                   dev_warn(smmu->dev,
>>                        "failed to enable priq irq\n");
>> @@ -2316,11 +2476,11 @@ static int arm_smmu_setup_irqs(struct 
>> arm_smmu_device *smmu)
>>            * Cavium ThunderX2 implementation doesn't not support unique
>>            * irq lines. Use single irq line for all the SMMUv3 interrupts.
>>            */
>> -        ret = devm_request_threaded_irq(smmu->dev, irq,
>> -                    arm_smmu_combined_irq_handler,
>> -                    arm_smmu_combined_irq_thread,
>> -                    IRQF_ONESHOT,
>> -                    "arm-smmu-v3-combined-irq", smmu);
>> +        ret = request_irq(irq,
>> +                  arm_smmu_combined_irq_handler,
>> +                  0,
>> +                  "arm-smmu-v3-combined-irq",
>> +                  smmu);
> 
> Ditto. And here a good example where I a stub is good. You set the IRQ type 
> everywere but not for this one.
> 
>>           if (ret < 0)
>>               dev_warn(smmu->dev, "failed to enable combined irq\n");
>>       } else
>> @@ -2542,8 +2702,11 @@ static int arm_smmu_device_hw_probe(struct 
>> arm_smmu_device *smmu)
>>           smmu->features |= ARM_SMMU_FEAT_STALLS;
>>       }
>>   +#if 0/* Xen: Do not enable Stage 1 translations */
> 
> This is just saying stage-1 is available. So why do you care so much to 
> disable it? This is just adding more #if 0, we managed to get away in SMMUv1 
> by leaving the code as it.
> 
>> +
>>       if (reg & IDR0_S1P)
>>           smmu->features |= ARM_SMMU_FEAT_TRANS_S1;
>> +#endif
>>         if (reg & IDR0_S2P)
>>           smmu->features |= ARM_SMMU_FEAT_TRANS_S2;
>> @@ -2616,10 +2779,12 @@ static int arm_smmu_device_hw_probe(struct 
>> arm_smmu_device *smmu)
>>       if (reg & IDR5_GRAN4K)
>>           smmu->pgsize_bitmap |= SZ_4K | SZ_2M | SZ_1G;
>>   +#if 0 /* Xen: SMMU ops do not have a pgsize_bitmap member for Xen */
>>       if (arm_smmu_ops.pgsize_bitmap == -1UL)
>>           arm_smmu_ops.pgsize_bitmap = smmu->pgsize_bitmap;
>>       else
>>           arm_smmu_ops.pgsize_bitmap |= smmu->pgsize_bitmap;
>> +#endif
>>         /* Output address size */
>>       switch (reg & IDR5_OAS_MASK << IDR5_OAS_SHIFT) {
>> @@ -2646,10 +2811,12 @@ static int arm_smmu_device_hw_probe(struct 
>> arm_smmu_device *smmu)
>>           smmu->oas = 48;
>>       }
>>   +#if 0 /* Xen: There is no support for DMA mask */
> 
> Stub it?
> 
>>       /* 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");
>> +#endif
>>         smmu->ias = max(smmu->ias, smmu->oas);
>>   @@ -2680,7 +2847,8 @@ static int arm_smmu_device_acpi_probe(struct 
>> platform_device *pdev,
>>       struct device *dev = smmu->dev;
>>       struct acpi_iort_node *node;
>>   -    node = *(struct acpi_iort_node **)dev_get_platdata(dev);
>> +    /* Xen: Modification to get iort_node */
>> +    node = (struct acpi_iort_node *)dev->acpi_node;
>>         /* Retrieve SMMUv3 specific data */
>>       iort_smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>> @@ -2703,7 +2871,7 @@ static inline int arm_smmu_device_acpi_probe(struct 
>> platform_device *pdev,
>>   static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>>                       struct arm_smmu_device *smmu)
>>   {
>> -    struct device *dev = &pdev->dev;
>> +    struct device *dev = pdev;
>>       u32 cells;
>>       int ret = -EINVAL;
>>   @@ -2716,8 +2884,8 @@ 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;
>> +    /* Xen: Set the COHERNECY feature */
>> +    smmu->features |= ARM_SMMU_FEAT_COHERENCY;
> 
> This looks like completely wrong. You should only do it when the firmware 
> tables say it is fine.
> 
>>         return ret;
>>   }
>> @@ -2734,9 +2902,11 @@ static int arm_smmu_device_probe(struct 
>> platform_device *pdev)
>>   {
>>       int irq, ret;
>>       struct resource *res;
>> +#if 0 /*Xen: Do not need to setup sysfs */
>>       resource_size_t ioaddr;
>> +#endif
>>       struct arm_smmu_device *smmu;
>> -    struct device *dev = &pdev->dev;
>> +    struct device *dev = pdev;/* Xen: dev is ignored */
>>       bool bypass;
>>         smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>> @@ -2763,8 +2933,9 @@ static int arm_smmu_device_probe(struct 
>> platform_device *pdev)
>>           dev_err(dev, "MMIO region too small (%pr)\n", res);
>>           return -EINVAL;
>>       }
>> +#if 0 /*Xen: Do not need to setup sysfs */
>>       ioaddr = res->start;
>> -
> 
> Again the newline.
> 
>> +#endif
>>       smmu->base = devm_ioremap_resource(dev, res);
>>       if (IS_ERR(smmu->base))
>>           return PTR_ERR(smmu->base);
>> @@ -2802,13 +2973,16 @@ static int arm_smmu_device_probe(struct 
>> platform_device *pdev)
>>           return ret;
>>         /* Record our private device structure */
>> +#if 0 /* Xen: SMMU is not treated a a platform device*/
>>       platform_set_drvdata(pdev, smmu);
>> -
> 
> Again the newline.
> 
>> +#endif
>>       /* Reset the device */
>>       ret = arm_smmu_device_reset(smmu, bypass);
>>       if (ret)
>>           return ret;
>>   +/* Xen: Not creating an IOMMU device list for Xen */
>> +#if 0
>>       /* And we're up. Go go go! */
>>       ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
>>                        "smmu3.%pa", &ioaddr);
>> @@ -2844,9 +3018,18 @@ static int arm_smmu_device_probe(struct 
>> platform_device *pdev)
>>           if (ret)
>>               return ret;
>>       }
>> +#endif
>> +    /*
>> +     * Xen: Keep a list of all probed devices. This will be used to query
>> +     * the smmu devices based on the fwnode.
>> +     */
>> +    INIT_LIST_HEAD(&smmu->devices);
>> +    spin_lock(&arm_smmu_devices_lock);
>> +    list_add(&smmu->devices, &arm_smmu_devices);
>> +    spin_unlock(&arm_smmu_devices_lock);
>>       return 0;
>>   }
>> -
> 
> Again the newline removed and /* Xen ... */
>> +#if 0
>>   static int arm_smmu_device_remove(struct platform_device *pdev)
>>   {
>>       struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
>> @@ -2860,6 +3043,10 @@ static void arm_smmu_device_shutdown(struct 
>> platform_device *pdev)
>>   {
>>       arm_smmu_device_remove(pdev);
>>   }
>> +#endif
>> +
>> +#define MODULE_DEVICE_TABLE(type, name)
>> +#define of_device_id dt_device_match
> 
> That should be define on top.
> 
>>     static const struct of_device_id arm_smmu_of_match[] = {
>>       { .compatible = "arm,smmu-v3", },
>> @@ -2867,6 +3054,7 @@ static const struct of_device_id arm_smmu_of_match[] = 
>> {
>>   };
>>   MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>   +#if 0
>>   static struct platform_driver arm_smmu_driver = {
>>       .driver    = {
>>           .name        = "arm-smmu-v3",
>> @@ -2883,3 +3071,318 @@ IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", NULL);
>>   MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
>>   MODULE_AUTHOR("Will Deacon <will.deacon@xxxxxxx>");
>>   MODULE_LICENSE("GPL v2");
>> +#endif
>> +
>> +/***** Start of Xen specific code *****/
>> +
>> +static int __must_check arm_smmu_iotlb_flush_all(struct domain *d)
>> +{
>> +    struct arm_smmu_xen_domain *smmu_domain = dom_iommu(d)->arch.priv;
>> +    struct iommu_domain *cfg;
>> +
>> +    spin_lock(&smmu_domain->lock);
>> +    list_for_each_entry(cfg, &smmu_domain->iommu_domains, list) {
>> +        /*
>> +         * Only invalidate the context when SMMU is present.
>> +         * This is because the context initialization is delayed
>> +         * until a master has been added.
>> +         */
>> +        if (unlikely(!ACCESS_ONCE(cfg->priv->smmu)))
>> +            continue;
>> +        arm_smmu_tlb_inv_context(cfg->priv);
>> +    }
>> +    spin_unlock(&smmu_domain->lock);
>> +    return 0;
>> +}
>> +
>> +static int __must_check arm_smmu_iotlb_flush(struct domain *d,
>> +                         unsigned long gfn,
>> +                         unsigned int page_count)
>> +{
>> +    return arm_smmu_iotlb_flush_all(d);
>> +}
>> +
>> +static struct iommu_domain *arm_smmu_get_domain(struct domain *d,
>> +                        struct device *dev)
>> +{
>> +    struct iommu_domain *domain;
>> +    struct arm_smmu_xen_domain *xen_domain;
>> +    struct arm_smmu_device *smmu;
>> +    struct arm_smmu_domain *smmu_domain;
>> +
>> +    xen_domain = dom_iommu(d)->arch.priv;
>> +
>> +    smmu = arm_smmu_get_by_fwnode(dev->iommu_fwspec->iommu_fwnode);
>> +    if (!smmu)
>> +        return NULL;
>> +
>> +    /*
>> +     * Loop through the &xen_domain->contexts to locate a context
>> +     * assigned to this SMMU
>> +     */
>> +    list_for_each_entry(domain, &xen_domain->iommu_domains, list) {
>> +        smmu_domain = to_smmu_domain(domain);
>> +        if (smmu_domain->smmu == smmu)
>> +            return domain;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static void arm_smmu_destroy_iommu_domain(struct iommu_domain *domain)
>> +{
>> +    list_del(&domain->list);
>> +    arm_smmu_domain_free(domain);
>> +}
>> +
>> +static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
>> +                   struct device *dev, u32 flag)
>> +{
>> +    int ret = 0;
>> +    struct iommu_domain *domain;
>> +    struct arm_smmu_xen_domain *xen_domain;
>> +    struct arm_smmu_domain *arm_smmu;
>> +
>> +    xen_domain = dom_iommu(d)->arch.priv;
>> +
>> +    if (!dev->archdata.iommu) {
>> +        dev->archdata.iommu = xzalloc(struct arm_smmu_xen_device);
>> +        if (!dev->archdata.iommu)
>> +            return -ENOMEM;
>> +    }
>> +
>> +    ret = arm_smmu_add_device(dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    spin_lock(&xen_domain->lock);
>> +
>> +    /*
>> +     * Check to see if an iommu_domain already exists for this xen domain
>> +     * under the same SMMU
>> +     */
>> +    domain = arm_smmu_get_domain(d, dev);
>> +    if (!domain) {
>> +
>> +        domain = arm_smmu_domain_alloc(IOMMU_DOMAIN_DMA);
>> +        if (!domain) {
>> +            ret = -ENOMEM;
>> +            goto out;
>> +        }
>> +
>> +        arm_smmu = to_smmu_domain(domain);
>> +        arm_smmu->s2_cfg.domain = d;
>> +
>> +        /* Chain the new context to the domain */
>> +        list_add(&domain->list, &xen_domain->iommu_domains);
>> +
>> +    }
>> +
>> +    ret = arm_smmu_attach_dev(domain, dev);
>> +    if (ret) {
>> +        if (domain->ref.counter == 0)
>> +            arm_smmu_destroy_iommu_domain(domain);
>> +    } else {
>> +        atomic_inc(&domain->ref);
>> +    }
>> +
>> +out:
>> +    spin_unlock(&xen_domain->lock);
>> +    return ret;
>> +}
>> +
>> +static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
>> +{
>> +    struct iommu_domain *domain = arm_smmu_get_domain(d, dev);
>> +    struct arm_smmu_xen_domain *xen_domain;
>> +    struct arm_smmu_domain *arm_smmu = to_smmu_domain(domain);
>> +
>> +    xen_domain = dom_iommu(d)->arch.priv;
>> +
>> +    if (!arm_smmu || arm_smmu->s2_cfg.domain != d) {
>> +        dev_err(dev, " not attached to domain %d\n", d->domain_id);
>> +        return -ESRCH;
>> +    }
>> +
>> +    spin_lock(&xen_domain->lock);
>> +
>> +    arm_smmu_detach_dev(dev);
>> +    atomic_dec(&domain->ref);
>> +
>> +    if (domain->ref.counter == 0)
>> +        arm_smmu_destroy_iommu_domain(domain);
>> +
>> +    spin_unlock(&xen_domain->lock);
>> +
>> +
>> +
>> +    return 0;
>> +}
>> +
>> +static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
>> +                 u8 devfn,  struct device *dev)
>> +{
>> +    int ret = 0;
>> +
>> +    /* Don't allow remapping on other domain than hwdom */
>> +    if (t && t != hardware_domain)
>> +        return -EPERM;
>> +
>> +    if (t == s)
>> +        return 0;
>> +
>> +    ret = arm_smmu_deassign_dev(s, dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (t) {
>> +        /* No flags are defined for ARM. */
>> +        ret = arm_smmu_assign_dev(t, devfn, dev, 0);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int arm_smmu_iommu_domain_init(struct domain *d)
>> +{
>> +    struct arm_smmu_xen_domain *xen_domain;
>> +
>> +    xen_domain = xzalloc(struct arm_smmu_xen_domain);
>> +    if (!xen_domain)
>> +        return -ENOMEM;
>> +
>> +    spin_lock_init(&xen_domain->lock);
>> +    INIT_LIST_HEAD(&xen_domain->iommu_domains);
>> +
>> +    dom_iommu(d)->arch.priv = xen_domain;
>> +
>> +    return 0;
>> +}
>> +
>> +static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>> +{
>> +}
>> +
>> +static void arm_smmu_iommu_domain_teardown(struct domain *d)
>> +{
>> +    struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
>> +
>> +    ASSERT(list_empty(&xen_domain->iommu_domains));
>> +    xfree(xen_domain);
>> +}
>> +
>> +static int __must_check arm_smmu_map_page(struct domain *d, unsigned long 
>> gfn,
>> +            unsigned long mfn, unsigned int flags)
>> +{
>> +    p2m_type_t t;
>> +
>> +    /*
>> +     * Grant mappings can be used for DMA requests. The dev_bus_addr
>> +     * returned by the hypercall is the MFN (not the IPA). For device
>> +     * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
>> +     * p2m to allow DMA request to work.
>> +     * This is only valid when the domain is directed mapped. Hence this
>> +     * function should only be used by gnttab code with gfn == mfn.
>> +     */
>> +    BUG_ON(!is_domain_direct_mapped(d));
>> +    BUG_ON(mfn != gfn);
>> +
>> +    /* We only support readable and writable flags */
>> +    if (!(flags & (IOMMUF_readable | IOMMUF_writable)))
>> +        return -EINVAL;
>> +
>> +    t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
>> +
>> +    /*
>> +     * The function guest_physmap_add_entry replaces the current mapping
>> +     * if there is already one...
>> +     */
>> +    return guest_physmap_add_entry(d, _gfn(gfn), _mfn(mfn), 0, t);
>> +}
>> +
>> +static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long 
>> gfn)
>> +{
>> +    /*
>> +     * This function should only be used by gnttab code when the domain
>> +     * is direct mapped
>> +     */
>> +    if (!is_domain_direct_mapped(d))
>> +        return -EINVAL;
>> +
>> +    return guest_physmap_remove_page(d, _gfn(gfn), _mfn(gfn), 0);
>> +}
>> +
>> +static const struct iommu_ops arm_smmu_iommu_ops = {
>> +    .init = arm_smmu_iommu_domain_init,
>> +    .hwdom_init = arm_smmu_iommu_hwdom_init,
>> +    .teardown = arm_smmu_iommu_domain_teardown,
>> +    .iotlb_flush = arm_smmu_iotlb_flush,
>> +    .iotlb_flush_all = arm_smmu_iotlb_flush_all,
>> +    .assign_device = arm_smmu_assign_dev,
>> +    .reassign_device = arm_smmu_reassign_dev,
>> +    .map_page = arm_smmu_map_page,
>> +    .unmap_page = arm_smmu_unmap_page,
>> +};
>> +
>> +static
>> +struct arm_smmu_device *arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
>> +{
>> +    struct arm_smmu_device *smmu = NULL;
>> +
>> +    spin_lock(&arm_smmu_devices_lock);
>> +    list_for_each_entry(smmu, &arm_smmu_devices, devices) {
>> +        if (smmu->dev->fwnode == fwnode)
>> +            break;
>> +    }
>> +    spin_unlock(&arm_smmu_devices_lock);
>> +
>> +    return smmu;
>> +}
>> +
>> +static __init int arm_smmu_dt_init(struct dt_device_node *dev,
>> +                   const void *data)
>> +{
>> +    int rc;
>> +
>> +    /*
>> +     * Even if the device can't be initialized, we don't want to
>> +     * give the SMMU device to dom0.
>> +     */
>> +    dt_device_set_used_by(dev, DOMID_XEN);
>> +
>> +    rc = arm_smmu_device_probe(dt_to_dev(dev));
>> +    if (rc)
>> +        return rc;
>> +
>> +    iommu_set_ops(&arm_smmu_iommu_ops);
>> +
>> +    return 0;
>> +}
>> +
>> +DT_DEVICE_START(smmuv3, "ARM SMMU V3", DEVICE_IOMMU)
>> +    .dt_match = arm_smmu_of_match,
>> +    .init = arm_smmu_dt_init,
>> +DT_DEVICE_END
>> +
>> +#ifdef CONFIG_ACPI
>> +/* Set up the IOMMU */
>> +static int __init arm_smmu_acpi_init(const void *data)
>> +{
>> +    int rc;
>> +    rc = arm_smmu_device_probe((struct device *)data);
>> +
>> +    if (rc)
>> +        return rc;
>> +
>> +    iommu_set_ops(&arm_smmu_iommu_ops);
>> +    return 0;
>> +}
>> +
>> +ACPI_DEVICE_START(asmmuv3, "ARM SMMU V3", DEVICE_IOMMU)
>> +    .class_type = ACPI_IORT_NODE_SMMU_V3,
>> +    .init = arm_smmu_acpi_init,
>> +ACPI_DEVICE_END
>> +
>> +#endif
>>
> Cheers,
> 

I'll fix the newlines as needed. I thought I had got them all but it seems a 
few were still missed. 
Thanks,
Sameer
-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

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