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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

I am slightly confused. I receive your answer on this e-mail after you 
resend a version. So were the comments on RFC v4 was addressed?
On 02/09/2018 05:56 PM, Sameer Goel wrote:
> > >          /* Request interrupt lines */
> > >        irq = smmu->evtq.q.irq;
> > > @@ -2316,9 +2782,13 @@ 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,
> > > +        /*
> > > +         * Xen: Does not support threaded irqs, so serialise the setup.
> > > +         * This is the same for pris and event interrupt lines on other
> > > +         * systems
> > > +         */
> > Above you did implemented a dummy implementation of 
> > devm_request_threaded_irq(...). So why did you replace the code here?
> The replacement worked well for other functions, where the handler was not 
> defined. So, the wrapper function calls devm_request_irq with the
> argument passed in as thread. In this case really the handler hits first and it 
> calls the thread in response. I can modify the code to make this fit into the 
> api
> but in that case I will need to swap around the functions so number of line 
> changes will stay the same. Tell me your preference.
I don't understand what you mean here. Would it be possible to give a 
concrete example?
> > > +        ret = devm_request_irq(smmu->dev, irq,
> > >                        arm_smmu_combined_irq_handler,
> > > -                    arm_smmu_combined_irq_thread,
> > >                        IRQF_ONESHOT,
> > >                        "arm-smmu-v3-combined-irq", smmu);
> > >            if (ret < 0)
> > > @@ -2542,8 +3012,14 @@ static int arm_smmu_device_hw_probe(struct 
> > > arm_smmu_device *smmu)
> > >            smmu->features |= ARM_SMMU_FEAT_STALLS;
> > >        }
> > >    +/*
> > > + * Xen: Block stage 1 translations. By doing this here we do not need to set 
> > > the
> > > + * domain->stage explicitly.
> > > + */
> > > +#if 0
> > >        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 +3092,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) {
> > > @@ -2680,7 +3158,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 +3182,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,6 +3195,7 @@ static int arm_smmu_device_dt_probe(struct 
> > > platform_device *pdev,
> > >          parse_driver_options(smmu);
> > >    +    /* Xen: of_dma_is_coherent is a stub till dt support is introduced */
> > I think this comment matters more on top of the stub. I would also add an BUG() 
> > in the stub to catch it.
> I have just returned a 0 here. Putting a BUG in might not have the desired 
> impact, since, this will execute every time.
How about a WARN_ON_ONCE in the implementation?

Cheers,

--
Julien Grall

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