[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

Hi Sameer,

On 12/05/2017 11:26 PM, Goel, Sameer wrote:
On 12/5/2017 7:17 AM, Julien Grall wrote:
On 05/12/17 03:59, Sameer Goel wrote:
+     * 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.

Yes please in both side.

+     */
+    q->base = _xzalloc(qsz, sizeof(void *));
       if (!q->base) {
           dev_err(smmu->dev, "failed to allocate queue (0x%zx bytes)\n",
           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 
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 :).

I am a bit confused. Does it mean Linux driver violate the spec? If so, that should be fixed in both and not only Xen.

+    strtab = _xzalloc(l1size, l1size);
       if (!strtab) {
               "failed to allocate l1 stream table (%u bytes)\n",
           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 

I was not able to convince myself that:

size = (1 << smmu->sid_bits) * (STRTAB_STE_DWORDS << 3)

will always honor the check. I would be ok with a comment explain why it should always work.


Julien Grall

Xen-devel mailing list



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