Re: [PATCH v3 7/8] xen/arm: Add support for SMMUv3 driver

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Julien,

> On 11 Dec 2020, at 2:25 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 10/12/2020 16:57, Rahul Singh wrote:
>>  struct arm_smmu_strtab_cfg {
>> @@ -613,8 +847,13 @@ struct arm_smmu_device {
>>              u64                     padding;
>>      };
>>  -   /* IOMMU core code handle */
>> -    struct iommu_device             iommu;
>> +    /* Need to keep a list of SMMU devices */
>> +    struct list_head                devices;
>> +
>> +    /* Tasklets for handling evts/faults and pci page request IRQs*/
>> +    struct tasklet          evtq_irq_tasklet;
>> +    struct tasklet          priq_irq_tasklet;
>> +    struct tasklet          combined_irq_tasklet;
>>  };
>>    /* SMMU private data for each master */
>> @@ -638,7 +877,6 @@ enum arm_smmu_domain_stage {
>>    struct arm_smmu_domain {
>>      struct arm_smmu_device          *smmu;
>> -    struct mutex                    init_mutex; /* Protects smmu pointer */
> 
> Hmmm... Your commit message says the mutex would be replaced by a spinlock. 
> However, you are dropping the lock. What I did miss?

Linux code using the mutex in the function arm_smmu_attach_dev() but in XEN 
this function is called from arm_smmu_assign_dev() which already has the 
spin_lock when arm_smmu_attach_dev() function I called so I drop the mutex to 
avoid nested spinlock. 
Timing analysis of using spin lock in place of mutex as compared to linux  when 
attaching a  device to SMMU is still valid.

> 
> [...]
> 
>> @@ -1578,6 +1841,17 @@ static int arm_smmu_domain_finalise_s2(struct 
>> arm_smmu_domain *smmu_domain,
>>      struct arm_smmu_device *smmu = smmu_domain->smmu;
>>      struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
>>      typeof(&arm_lpae_s2_cfg.vtcr) vtcr = &arm_lpae_s2_cfg.vtcr;
>> +    uint64_t reg = READ_SYSREG64(VTCR_EL2);
> 
> Please don't use VTCR_EL2 here. You should be able to infer the parameter 
> from the p2m_ipa_bits.

Ok.

> 
> Also, I still don't see code that will restrict the IPA bits because on the 
> support for the SMMU.

Ok I will add the code to restrict the IPA bits in next version.
> 
>> +
>> +    vtcr->tsz       = FIELD_GET(STRTAB_STE_2_VTCR_S2T0SZ, reg);
>> +    vtcr->sl        = FIELD_GET(STRTAB_STE_2_VTCR_S2SL0, reg);
>> +    vtcr->irgn      = FIELD_GET(STRTAB_STE_2_VTCR_S2IR0, reg);
>> +    vtcr->orgn      = FIELD_GET(STRTAB_STE_2_VTCR_S2OR0, reg);
>> +    vtcr->sh        = FIELD_GET(STRTAB_STE_2_VTCR_S2SH0, reg);
>> +    vtcr->tg        = FIELD_GET(STRTAB_STE_2_VTCR_S2TG, reg);
>> +    vtcr->ps        = FIELD_GET(STRTAB_STE_2_VTCR_S2PS, reg);
>> +
>> +    arm_lpae_s2_cfg.vttbr  = page_to_maddr(cfg->domain->arch.p2m.root);
>>      vmid = arm_smmu_bitmap_alloc(smmu->vmid_map, smmu->vmid_bits);
>>      if (vmid < 0)
>> @@ -1592,6 +1866,11 @@ static int arm_smmu_domain_finalise_s2(struct 
>> arm_smmu_domain *smmu_domain,
>>                        FIELD_PREP(STRTAB_STE_2_VTCR_S2SH0, vtcr->sh) |
>>                        FIELD_PREP(STRTAB_STE_2_VTCR_S2TG, vtcr->tg) |
>>                        FIELD_PREP(STRTAB_STE_2_VTCR_S2PS, vtcr->ps);
>> +
>> +    printk(XENLOG_DEBUG
>> +               "SMMUv3: d%u: vmid 0x%x vtcr 0x%"PRIpaddr" p2maddr 
>> 0x%"PRIpaddr"\n",
>> +               cfg->domain->domain_id, cfg->vmid, cfg->vtcr, cfg->vttbr);
>> +
>>      return 0;
>>  }
> 
> [...]
> 
>> @@ -1923,8 +2239,8 @@ static int arm_smmu_init_one_queue(struct 
>> arm_smmu_device *smmu,
>>              return -ENOMEM;
>>      }
>>  -   if (!WARN_ON(q->base_dma & (qsz - 1))) { > -            
>> dev_info(smmu->dev, "allocated %u entries for %s\n",
>> +    if (unlikely(q->base_dma & (qsz - 1))) {
>> +            dev_warn(smmu->dev, "allocated %u entries for %s\n",
> dev_warn() is not the same as WARN_ON(). But really, the first step is for 
> you to try to change behavior of WARN_ON() in Xen.

Ok. I will make sure we have the same behaviour as linux by modifying the code 
as below.

WARN_ON(q->base_dma & (qsz - 1));
if (!unlikely(q->base_dma & (qsz - 1))) {
        dev_info(smmu->dev, "allocated %u entries for %s\n",
                1 << q->llq.max_n_shift, name);
}

Regards,
Rahul

> 
> If this doesn't go through then we can discuss about approach to mitigate it.
> 
> Cheers,
> 
> -- 
> Julien Grall