Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Julien Grall <julien.grall@arm.com>]

Hi Sameer,

On 28/08/17 23:21, Goel, Sameer wrote:
> On 6/12/2017 7:24 AM, Julien Grall wrote:
>>>   static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>>                      struct fwnode_handle *fwnode,
>>>                      const struct iommu_ops *ops)
>>> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>>       return ret;
>>>   }
>>>
>>> -static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
>>> -                    struct acpi_iort_node *node,
>>> -                    u32 streamid)
>>> +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
>>> +                u32 streamid)
>>>   {
>>> -    const struct iommu_ops *ops = NULL;
>>>       int ret = -ENODEV;
>>>       struct fwnode_handle *iort_fwnode;
>>>
>>>       if (node) {
>>>           iort_fwnode = iort_get_fwnode(node);
>>>           if (!iort_fwnode)
>>> -            return NULL;
>>> -
>>> -        ops = iommu_ops_from_fwnode(iort_fwnode);
>>> -        if (!ops)
>>> -            return NULL;
>>> +            return ret;
>>>
>>> -        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
>>> +        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);
>>
>> Why don't you get the IOMMU ops here? This would avoid unecessary change.
>  From the linux definition it seems that there will be eventually used to override the ops
> set by the bus. I did not find a use for this here, so removed it to simplify code. I can
> add these back, but see this as dead code.

You will always have dead code if you import code as it is from Linux. 
This is the price to pay to help rebase the code in the future.

In that specific case, I think return the ops is the right thing to do. 
Potentially it will allow us to support different IOMMU at the same time.

[...]

>>>   #define IORT_IRQ_MASK(irq)        (irq & 0xffffffffULL)
>>>   #define IORT_IRQ_TRIGGER_MASK(irq)    ((irq >> 32) & 0xffffffffULL)
>>>
>>>   int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
>>>   void iort_deregister_domain_token(int trans_id);
>>>   struct fwnode_handle *iort_find_domain_token(int trans_id);
>>> -#ifdef CONFIG_ACPI_IORT
>>> +#endif
>>> +
>>> +#ifdef CONFIG_ARM_64
>>
>> Why #ifdef CONFIG_ARM_64?
> Was trying to keep the impact low for this RFC iteration (My use-case is arm64 only). Looking for the right recommendation?

IORT is not specific to ARM64. Even though ACPI is only supported for 
ARM64 on Xen today, we try to keep the code as generic as possible.

In that case, you could turn CONFIG_ACPI_IORT on in the Kconfig when 
ACPI is enabled.

[...]

>>
>> [...]
>>
>>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>>> index 995a85a..3785fae 100644
>>> --- a/xen/include/xen/lib.h
>>> +++ b/xen/include/xen/lib.h
>>> @@ -9,7 +9,12 @@
>>>   #include <asm/bug.h>
>>>
>>>   #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>> +#define WARN_ON(p) ({                                      \
>>> +    int __ret_warn_on = !!(p);                             \
>>> +    if (unlikely(__ret_warn_on))                           \
>>> +        WARN();                                            \
>>> +    unlikely(__ret_warn_on);                               \
>>> +})
>>
>> hmmmm. Why this change?
> Was getting a compilation error when I was using WARN_ON as a conditional
> in an if statement regarding the return value. So removed the loop. This
> looks similar to Linux now.

This should belong to a separate patch with a commit message explaining 
why the change.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel