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

["Goel, Sameer" <sgoel@xxxxxxxxxxxxxx>]

On 5/30/2018 10:10 PM, Manish Jaggi wrote:
> 
> 
> On 05/31/2018 01:16 AM, Sameer Goel wrote:
>> Manish, I'll take another look at the variable names. I might not have 
>> enough time :).
>>>>
>>>> On 05/23/2018 10:48 PM, Manish Jaggi wrote:
>>>>> Hi Sameer,
>>>>>
>>>>> General Comment, please use appropriate variable names for XXX_domain 
>>>>> structures in code which is xen specific.
>>>> I thought that we had discussed this before on one of the RFCs.
>>> Yes and no one replied to my last comment that the var names have to be non 
>>> confusing in Xen specific code.
>> I'm sorry about that. I just assumed that since the policy on ported 
>> variable names was consistent we so not nee to change the names.
>>
>>>> At this point we are just using the format used for smmu-v2.
>>> smmu-v2 has a lot of confusing variable names with _domain.
>>> I believe that file needs to be fixed as well.
>>>> I don't think that the variable names are inappropriate. Unless there is a 
>>>> very specific issue with the variable names,
>>> The issue is in code readability and understanding the flow.
>>> It is confusing so many _domain variable names are used which are not 
>>> verbose.
>> These names are coming from the original Linux code. So, we do not change 
>> them as per the policy. That being said I can add a comment section to 
>> explain the confusing variables in the Xen specific code? Please let me know 
>> if this will help us resolve this issue?
>>   
> If it is in Xen specific functions, then it can be changed, this is what 
> julien also confirmed.

Agreed. Changing the var names.
> 
>>> Two functions different and confusing variable names
>>>>>> +    struct iommu_domain *domain;
>>>>>> +};
>>>>>> +
>>>>> As this is a xen specific code, can the variable names be used 
>>>>> appropriately.
>>>>> Repeating my comment  from earlier version.
>>>>> a domain is  usually a VM in Xen. So it is a bit confusing to use domain 
>>>>> for iommu_domain.
>> The struct is fine but you have an issue with the var name right?
> Yes.
>>
>>>>>> +/*
>>>>>> + * Xen: io_pgtable compatibility defines.
>>>>>> + * Most of these are to port in the S1 translation code as is.
>>>>>> + */
>>>>>> +struct io_pgtable_ops {
>>>>>> +};
>>>>>> +
>>>>>> +struct iommu_gather_ops {
>>>>>> +    void (*tlb_flush_all)(void *cookie);
>>>>>> +    void (*tlb_add_flush)(unsigned long iova, size_t size, size_t 
>>>>>> granule,
>>>>>> +                  bool leaf, void *cookie);
>>>>>> +    void (*tlb_sync)(void *cookie);
>>>>>> +};
>>>>>> +
>>>>>> +struct io_pgtable_cfg {
>>>>>> +    /*
>>>>>> +     * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
>>>>>> +     *    stage 1 PTEs, for hardware which insists on validating them
>>>>>> +     *    even in    non-secure state where they should normally be 
>>>>>> ignored.
>>>>>> +     *
>>>>>> +     * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
>>>>>> +     *    IOMMU_NOEXEC flags and map everything with full access, for
>>>>>> +     *    hardware which does not implement the permissions of a given
>>>>>> +     *    format, and/or requires some format-specific default value.
>>>>>> +     *
>>>>>> +     * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching 
>>>>>> invalid
>>>>>> +     *    (unmapped) entries but the hardware might do so anyway, 
>>>>>> perform
>>>>>> +     *    TLB maintenance when mapping as well as when unmapping.
>>>>>> +     *
>>>>>> +     * IO_PGTABLE_QUIRK_ARM_MTK_4GB: (ARM v7s format) Set bit 9 in all
>>>>>> +     *    PTEs, for Mediatek IOMMUs which treat it as a 33rd address bit
>>>>>> +     *    when the SoC is in "4GB mode" and they can only access the 
>>>>>> high
>>>>>> +     *    remap of DRAM (0x1_00000000 to 0x1_ffffffff).
>>>>>> +     *
>>>>>> +     * IO_PGTABLE_QUIRK_NO_DMA: Guarantees that the tables will only 
>>>>>> ever
>>>>>> +     *    be accessed by a fully cache-coherent IOMMU or CPU (e.g. for a
>>>>>> +     *    software-emulated IOMMU), such that pagetable updates need not
>>>>>> +     *    be treated as explicit DMA data.
>>>>>> +     */
>>>>>> +    #define IO_PGTABLE_QUIRK_ARM_NS        BIT(0)
>>>>>> +    #define IO_PGTABLE_QUIRK_NO_PERMS    BIT(1)
>>>>>> +    #define IO_PGTABLE_QUIRK_TLBI_ON_MAP    BIT(2)
>>>>>> +    #define IO_PGTABLE_QUIRK_ARM_MTK_4GB    BIT(3)
>>>>>> +    #define IO_PGTABLE_QUIRK_NO_DMA        BIT(4)
>>>>>> +    unsigned long            quirks;
>>>>>> +    unsigned long            pgsize_bitmap;
>>>>>> +    unsigned int            ias;
>>>>>> +    unsigned int            oas;
>>>>>> +    const struct iommu_gather_ops    *tlb;
>>>>>> +    struct device            *iommu_dev;
>>>>>> +
>>>>>> +    /* Low-level data specific to the table format */
>>>>>> +    union {
>>>>>> +        struct {
>>>>>> +            u64    ttbr[2];
>>>>>> +            u64    tcr;
>>>>>> +            u64    mair[2];
>>>>>> +        } arm_lpae_s1_cfg;
>>>>>> +
>>>>>> +        struct {
>>>>>> +            u64    vttbr;
>>>>>> +            u64    vtcr;
>>>>>> +        } arm_lpae_s2_cfg;
>>>>>> +
>>>>>> +        struct {
>>>>>> +            u32    ttbr[2];
>>>>>> +            u32    tcr;
>>>>>> +            u32    nmrr;
>>>>>> +            u32    prrr;
>>>>>> +        } arm_v7s_cfg;
>>>>>> +    };
>>>>>> +};
>>>>>> +
>>>>>> +enum io_pgtable_fmt {
>>>>>> +    ARM_32_LPAE_S1,
>>>>>> +    ARM_32_LPAE_S2,
>>>>>> +    ARM_64_LPAE_S1,
>>>>>> +    ARM_64_LPAE_S2,
>>>>>> +    ARM_V7S,
>>>>>> +    IO_PGTABLE_NUM_FMTS,
>>>>>> +};
>>>>>> +
>>>>>> +/*
>>>>>> + * Xen: The pgtable_ops are used by the S1 translations, so return the 
>>>>>> dummy
>>>>>> + * address.
>>>>>> + */
>>>>>> +#define alloc_io_pgtable_ops(f, c, o) ((struct io_pgtable_ops *)0x1)
>>>>>> +#define free_io_pgtable_ops(o)
>>>>>> +
>>>>>> +/* Xen: Define wrapper for requesting IRQs */
>>>>>> +#define IRQF_ONESHOT 0
>>>>>> +
>>>>>> +typedef void (*irq_handler_t)(int, void *, struct cpu_user_regs *);
>>>>>> +
>>>>>> +static inline int devm_request_irq(struct device *dev, unsigned int irq,
>>>>>> +                   irq_handler_t handler, unsigned long irqflags,
>>>>>> +                   const char *devname, void *dev_id)
>>>>>> +{
>>>>>> +    /*
>>>>>> +     * SMMUv3 implementation can support wired interrupt outputs that 
>>>>>> are
>>>>>> +     * edge-triggered. Set the irq type as per the spec.
>>>>>> +     */
>>>>>> +    irq_set_type(irq, IRQ_TYPE_EDGE_BOTH);
>>>>>> +    return request_irq(irq, irqflags, handler, devname, dev_id);
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Xen does not have a concept of threaded irq, but we can use tasklets 
>>>>>> to
>>>>>> + * achieve the desired functionality as needed.
>>>>>> + */
>>>>>> +int devm_request_threaded_irq(struct device *dev, unsigned int irq, 
>>>>>> irq_handler_t handler,
>>>>>> +                  irq_handler_t thread_fn, unsigned long irqflags,
>>>>>> +                  const char *devname, void *dev_id)
>>>>>> +{
>>>>>> +    return devm_request_irq(dev, irq, thread_fn, irqflags, devname, 
>>>>>> dev_id);
>>>>>> +}
>>>>>> +
>>>>>> +/* Xen: The mutex is used only during initialization so the typecast is 
>>>>>> safe */
>>>>>> +#define mutex spinlock
>>>>>> +#define mutex_init spin_lock_init
>>>>>> +#define mutex_lock spin_lock
>>>>>> +#define mutex_unlock spin_unlock
>>>>>> +
>>>>>> +#define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us) \
>>>>>> +({ \
>>>>>> +    s_time_t deadline = NOW() + MICROSECS(timeout_us); \
>>>>>> +    for (;;) { \
>>>>>> +        (val) = op(addr); \
>>>>>> +        if (cond) \
>>>>>> +            break; \
>>>>>> +        if (NOW() > deadline) { \
>>>>>> +            (val) = op(addr); \
>>>>>> +            break; \
>>>>>> +        } \
>>>>>> +    udelay(sleep_us); \
>>>>>> +    } \
>>>>>> +    (cond) ? 0 : -ETIMEDOUT; \
>>>>>> +})
>>>>>> +
>>>>>> +#define readl_relaxed_poll_timeout(addr, val, cond, delay_us, 
>>>>>> timeout_us) \
>>>>>> +    readx_poll_timeout(readl_relaxed, addr, val, cond, delay_us, 
>>>>>> timeout_us)
>>>>>> +
>>>>>> +#define VA_BITS 0 /* Only needed for S1 translations */
>>>>>>      /* MMIO registers */
>>>>>>    #define ARM_SMMU_IDR0            0x0
>>>>>> @@ -433,6 +819,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 +844,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 +949,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 +1025,25 @@ 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
>>>>> Is it clean to put #if 0, can this line be deleted or used with a proper 
>>>>> macro
>> Manish this is a var name from original struct from Linux. I have just 
>> followed the prior standard of adding #if 0.
> It does not look clean, can we use something other than 0, say a macro 
> NA_CODE ...
That's true. Adding NA_CODE will probably make things clearer. I'll incorporate 
the change.

>>>>>> +    /* Xen: Need to keep a list of SMMU devices */
>>>>>> +    struct list_head                devices;
>>>>>> +    /* Xen: Tasklets for handling evts/faults and pci page request 
>>>>>> IRQs*/
>>>>>> +    struct tasklet            evtq_tasklet;
>>>>>> +    struct tasklet            priq_tasklet;
>>>>>> +    struct tasklet            combined_irq_tasklet;
.....
>>>>>> +                        struct device *dev)
>>>>> This is returning iommu_domain and not a domain.
>>>>> Please change the name of function.
>> Same as smmu-v2.c. I think with the complexity of the code we should try to 
>> get this in as the prior code. Once the set that has been pending for some 
>> time is in. We can start on the variable cleanup activity. I say this so 
>> that we can make the names consistent across the drivers and this will be a 
>> lot easier to do when the majority of the other code is in.
> smmu-v2.c had this issue and I had hard time understanding the code, the 
> confusing variable names break the understanding flow, especially when you 
> are debugging something and have lost touch of code.
> So let not do same mistake as in smmu-v2.c
Agreed. Will change the variable name.
>>>>>> +{
>>>>>> +    struct iommu_domain *domain;
>>>>>> +    struct arm_smmu_xen_domain *xen_domain;
>>>>> A suggestion
>>>>> 1. as you have used in above function smmu_domain variable for 
>>>>> arm_smmu_xen_domain
>>>>> Can similar logic be used for iommu_domain.
>> I am fine changing the var name.
>>>>> 2. When smmu_domain variable name is used in above function why 
>>>>> xen_domain is used in this function.
>>>>> It is quite confusing.
>>>>> logically xen_domain should mean a VM.
>> You lost me man. xen_domain is referring to the page tables of the the VM 
>> currently using the SMMU. What is the concern?
> in xen when you read basic documentation a domain is a VM, right?
> Now what would xen_domain variable should mean.
> How would you know it has something to do with SMMU.
> 
> So it is confusing to someone looking at the code.
I agree that you would think about the VMs when you see xen_domain. In this 
case the xen_domain variable has a 1:1 correspondence with the SMMU. If I call 
this smmu_xen_domain will it be easier to read?
>>>>>> +    struct arm_smmu_device *smmu;
>>>>>> +    struct arm_smmu_domain *smmu_domain;
>>>>> smmu_domain was used for arm_smmu_xen_domain.
>> Failed to see it. But, I'll have another look.
>>
>>>>> consistency of variable names is necessary for code clarity
>>>>>> +
>>>>>> +    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->contexts, 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)
>>>>> same
>> Ok
>>>>>> +{
>>>>>> +    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;
>>>>> same
>> xen_domain is fine. You have an issue with the iommu_domioan here?
> domain has a variable name d, and iommu_domain has a variable name domain.
> Isnt it confusing.
> Why cant we use
> struct iommu_domain *io_domain ...
I agree. I am changing this all to iommu_domain. io_domain is fine too. What do 
you prefer?
> 
>>>>>> +    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->contexts);
>>>>>> +
>>>>>> +    }
>>>>>> +
>>>>>> +    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);
>>>>> same
>>>>>> +
>>>>>> +    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)
>>>>> Where is iommu_domain initialized?
>>>>> The function does not use a iommu_domain * variable
>>>>>> +{
>>>>>> +    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->contexts);
>>>>>> +
>>>>>> +    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->contexts));
>>>>>> +    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

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

-- 
 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