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

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

On 9/12/2017 5:25 AM, Julien Grall wrote:
> 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.
>
Agreed.
 
> [...]
> 
>>>>   #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.
Agreed. I will add the new config in the next patch set.
> 
> [...]
> 
>>>
>>> [...]
>>>
>>>> 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.

Agreed. Since this is more of a Linux compat function, I do not want to change 
the system-wide behavior. I will move this in the IORT code.
> 
> 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