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

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

On 11/20/2017 7:25 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 19/11/17 07:45, Goel, Sameer wrote:
>> On 10/12/2017 10:36 AM, Julien Grall wrote:
>>>> +
>>>> +typedef paddr_t phys_addr_t;
>>>> +typedef paddr_t dma_addr_t;
>>>> +
>>>> +/* Alias to Xen device tree helpers */
>>>> +#define device_node dt_device_node
>>>> +#define of_phandle_args dt_phandle_args
>>>> +#define of_device_id dt_device_match
>>>> +#define of_match_node dt_match_node
>>>> +#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, 
>>>> pname, out))
>>>> +#define of_property_read_bool dt_property_read_bool
>>>> +#define of_parse_phandle_with_args dt_parse_phandle_with_args
>>>> +#define mutex spinlock_t
>>>> +#define mutex_init spin_lock_init
>>>> +#define mutex_lock spin_lock
>>>> +#define mutex_unlock spin_unlock
>>>
>>> mutex and spinlock are not the same. The former is sleeping whilst the 
>>> later is not.
>>>
>>> Can you please explain why this is fine and possibly add that in a comment?
>>>
>> Mutex is used to protect the access to smmu device internal data structure 
>> when setting up the s2 config and installing stes for a given device in 
>> Linux. The ste programming  operation can be competitively long but in the 
>> current testing, I did not see this blocking for too long. I will put in a 
>> comment.
> 
> Well, I don't think that this is a justification. You tested on one platform 
> and does not explain how you perform them.
> 
> If I understand correctly, that mutex is only used when assigning device. So 
> it might be ok to switch to spinlock. But that's not because the operation is 
> not too long, it just because it would be only perform by the toolstack 
> (domctl) and will not be issued by guest.
Ok. I agree ans will update the comment. 
> 
>>
>>>> +
>>>> +/* Xen: Helpers to get device MMIO and IRQs */
>>>> +struct resource {
>>>> +    u64 addr;
>>>> +    u64 size;
>>>> +    unsigned int type;
>>>> +};
>>>
>>> Likely we want a compat header for defining Linux helpers. This would avoid 
>>> replicating it everywhere.
>> Agreed.
>>
> That should be
>>>
>>>> +
>>>> +#define resource_size(res) ((res)->size)
>>>> +
>>>> +#define platform_device device
>>>> +
>>>> +#define IORESOURCE_MEM 0
>>>> +#define IORESOURCE_IRQ 1
>>>> +
>>>> +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;
>>>> +
>>>> +            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:
>>>> +        ret = platform_get_irq(dev_to_dt(pdev), num);
>>>
>>> No IRQ for ACPI?
>> For IRQs the code calls platform_get_irq_byname. So, the IORESOURCE_IRQ 
>> implementation is not needed at all. (DT or ACPI)
> 
> Please document it then.
Ok.
> 
> [...]
> 
>>>
>>>> +        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)
>>>> +
>>>> +/* Xen: Helpers for IRQ functions */
>>>> +#define request_irq(irq, func, flags, name, dev) request_irq(irq, flags, 
>>>> func, name, dev)
>>>> +#define free_irq release_irq
>>>> +
>>>> +enum irqreturn {
>>>> +    IRQ_NONE    = (0 << 0),
>>>> +    IRQ_HANDLED    = (1 << 0),
>>>> +};
>>>> +
>>>> +typedef enum irqreturn irqreturn_t;
>>>> +
>>>> +/* Device logger functions */
>>>> +#define dev_print(dev, lvl, fmt, ...)                        \
>>>> +     printk(lvl "smmu: " fmt, ## __VA_ARGS__)
>>>> +
>>>> +#define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## 
>>>> __VA_ARGS__)
>>>> +#define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## 
>>>> __VA_ARGS__)
>>>> +#define dev_warn(dev, fmt, ...) dev_print(dev, XENLOG_WARNING, fmt, ## 
>>>> __VA_ARGS__)
>>>> +#define dev_err(dev, fmt, ...) dev_print(dev, XENLOG_ERR, fmt, ## 
>>>> __VA_ARGS__)
>>>> +#define dev_info(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## 
>>>> __VA_ARGS__)
>>>> +
>>>> +#define dev_err_ratelimited(dev, fmt, ...)                    \
>>>> +     dev_print(dev, XENLOG_ERR, fmt, ## __VA_ARGS__)
>>>> +
>>>> +#define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
>>>> +
>>>> +/* Alias to Xen allocation helpers */
>>>> +#define kfree xfree
>>>> +#define kmalloc(size, flags)        _xmalloc(size, sizeof(void *))
>>>> +#define kzalloc(size, flags)        _xzalloc(size, sizeof(void *))
>>>> +#define devm_kzalloc(dev, size, flags)    _xzalloc(size, sizeof(void *))
>>>> +#define kmalloc_array(size, n, flags)    _xmalloc_array(size, sizeof(void 
>>>> *), n)
>>>> +
>>>> +/* Compatibility defines */
>>>> +#undef WARN_ON
>>>> +#define WARN_ON(cond) (!!cond)
>>>
>>> Why do you redefine WARN_ON?Need it to be a scalar value. The Xen 
>>> implementation is a do..while. Did not want a large impact hence the local 
>>> define. I can try proposing a new common define.
> 
> Well, I don't think it is acceptable to redefine WARN_ON. At least without 
> trying to modify the common version.

I will put this in the common folder.
> 
>>>
>>>> +#define WARN_ON_ONCE(cond) WARN_ON(cond)
>>>> Hmmm, can't we implement a common WARN_ON_ONCE?
>> Will not really be executed. Defining it to maintain compatibility. I think 
>> this file is the right place for this define. We can send it to a generic 
>> file if so needed.
> 
> I can't see why we wouldn't want to have a WARN_ON_ONCE in the common code. 
> If you disagree, please explain.
> 
> [...]
> 
>>>
>>>> +        else {
>>>>                cpu_relax();
>>>
>>> Hmmm I now see why you added cpu_relax() at the top. Well, on Xen cpu_relax 
>>> is just a barrier. On Linux it is used to yield.
>>>
>>> And that bit is worrying me. The Linux code will allow context switching to 
>>> another tasks if the code is taking too much time.
>>>
>>> Xen is not preemptible, so is it fine?
>> This is used when consuming the command queue and could be a potential 
>> performance issue if the queue is large. (This is never the case).
>> I am wondering if we should define a yeild in long run?
> 
> As I said before, Xen is not preemptible. In this particular case, there are 
> spinlock taken by the callers (e.g any function assigning device). So yield 
> would just make it worst.
> 
> [...]
I can keep the memory barrier as is. This should not impact much. This should 
not be a concern.
> 
>>>
>>>>        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 = 1 << ((5 + (desc->span - 1)));
>>>
>>> Do you mind explaining the 5? Also, does the shift will always be < 32?
>> Its from the "Level 1 Stream Table Descriptor" from SMMU spec. I can add the 
>> ref to the spec. Yes the span is hard coded in the driver and with any spec 
>> valid span values this cannot exceed 32.
>> I have added a comment tot he code
>>>
>>>> +    desc->l2ptr = _xzalloc(size, alignment);
>>>> +    desc->l2ptr_dma = virt_to_maddr(desc->l2ptr);
>>>
>>> _xzalloc can fail and virt_to_maddr will result to a panic.
>>>
>> I will move the check.
>>>> +
>>>>        if (!desc->l2ptr) {
>>>>            dev_err(smmu->dev,
>>>>                "failed to allocate l2 stream table for SID %u\n",
>>>> @@ -1158,7 +1418,7 @@ static int arm_smmu_init_l2_strtab(struct 
>>>> arm_smmu_device *smmu, u32 sid)
>>>>    }
>>>>      /* IRQ and event handlers */
>>>> -static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
>>>> +static void arm_smmu_evtq_thread(int irq, void *dev, struct cpu_user_regs 
>>>> *regs)
>>>
>>> Could you please introduce a wrapper instead as it was done in smmu.c?
>>>
>> Done.
>>
>>>>    {
>>>>        int i;
>>>>        struct arm_smmu_device *smmu = dev;
>>>> @@ -1186,7 +1446,6 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, 
>>>> void *dev)
>>>>          /* Sync our overflow flag, as we believe we're up to speed */
>>>>        q->cons = Q_OVF(q, q->prod) | Q_WRP(q, q->cons) | Q_IDX(q, q->cons);
>>>> -    return IRQ_HANDLED;
>>>>    }
>>>>      static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 
>>>> *evt)
>>>> @@ -1203,7 +1462,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 
>>>> 0x%016lx\n",
>>>
>>> Hmmm why?
>> The variable is defined as ungined long long in Linux. ( uses generic 
>> int-ll64.h) In Xen the definition changes to unsigned long for ARM_64 which 
>> actually makes sense.
> 
> u64 is a 64-bit variable. And therefore the format should be PRIx64 to 
> accomodate 64-bit and 32-bit.
Sure. I can change the format specifier. 
> 
> [...]
> 
>>>
>>>> +
>>>>      static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>>>>    {
>>>> @@ -1410,6 +1674,7 @@ static struct iommu_domain 
>>>> *arm_smmu_domain_alloc(unsigned type)
>>>>        return &smmu_domain->domain;
>>>>    }
>>>>    +#if 0
>>>
>>> Please explain the #if 0
>>>
>> Done.
>>
>>>>    static int arm_smmu_bitmap_alloc(unsigned long *map, int span)
>>>>    {
>>>>        int idx, size = 1 << span;
>>>> @@ -1427,36 +1692,20 @@ static void arm_smmu_bitmap_free(unsigned long 
>>>> *map, int idx)
>>>>    {
>>>>        clear_bit(idx, map);
>>>>    }
>>>> +#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;
>>>> -
>>>> -    iommu_put_dma_cookie(domain);
>>>> -    free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>>>> -
>>>> -    /* Free the CD and ASID, if we allocated them */
>>>> -    if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>>>> -        struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
>>>> -
>>>> -        if (cfg->cdptr) {
>>>> -            dmam_free_coherent(smmu_domain->smmu->dev,
>>>> -                       CTXDESC_CD_DWORDS << 3,
>>>> -                       cfg->cdptr,
>>>> -                       cfg->cdptr_dma);
>>>> -
>>>> -            arm_smmu_bitmap_free(smmu->asid_map, cfg->cd.asid);
>>>> -        }
>>>> -    } else {
>>>> -        struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
>>>> -        if (cfg->vmid)
>>>> -            arm_smmu_bitmap_free(smmu->vmid_map, cfg->vmid);
>>>> -    }
>>>> +    /*
>>>> +     * Xen: Remove the free functions that are not used and code related
>>>> +     * to S1 translation. We just need to free the domain here.
>>>> +     */
>>>
>>> Please use #if 0 rather than removing the code + comment on top. But I am 
>>> not sure why you drop the S2 free code. Shouldn't we allocate VMID from the 
>>> SMMU?
>> I am picking up the vmid from the domain I am sharing the page tables with. 
>> domain->arch.p2m.vmid. This seemed valid. Please let me know if we want to 
>> generate a different vmid?
> 
> The processor and the SMMU may support either 8 or 16bits VMID but I don't 
> think any thing in the spec says that both processor and SMMU must support 
> the same value.
> 
> So it would be possible to have a processor supporting 16-bits VMID and the 
> SMMU only 8-bits VMID.
> 
> Adding a check at the SMMU initialization might be a solution. But as the 
> code for allocating the VMID is already present, then I would prefer to we 
> stick with different the VMID.
Sure. For this iteration I will generate an SMMU specific VMID. Do you think it 
would be an optimization to reuse the CPU VMID if SMMU supports 16bit VMID?

Thanks,
Sameer
> 
>>
>>
>>>
>>>>          kfree(smmu_domain);
>>>>    }
>>>>    +#if 0
>>>>    static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain 
>>>> *smmu_domain,
>>>>                           struct io_pgtable_cfg *pgtbl_cfg)
>>>>    {
>>>> @@ -1488,33 +1737,30 @@ out_free_asid:
>>>>        arm_smmu_bitmap_free(smmu->asid_map, asid);
>>>>        return ret;
>>>>    }
>>>> +#endif
>>>>    -static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain 
>>>> *smmu_domain,
>>>> -                       struct io_pgtable_cfg *pgtbl_cfg)
>>>> +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: Set the values as needed */
>>>>    -    cfg->vmid    = (u16)vmid;
>>>> -    cfg->vttbr    = pgtbl_cfg->arm_lpae_s2_cfg.vttbr;
>>>> -    cfg->vtcr    = pgtbl_cfg->arm_lpae_s2_cfg.vtcr;
>>>> +    cfg->vmid    = cfg->domain->arch.p2m.vmid;
>>>
>>> See my comment above.
>> I am wondering why we are considering this value invalid? Since the page 
>> tables are shared we can use the vmid from the domain. Is the concern 
>> regarding the size?
> 
> See above.
> 
> Cheers,
> 

-- 
 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel