Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

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

On 13/06/17 12:44, Manish Jaggi wrote:
> On 6/13/2017 4:58 PM, Julien Grall wrote:
>> On 13/06/17 12:02, Manish Jaggi wrote:
>>>>> Will the below code be ok?
>>>>
>>>> If you noticed, I didn't say this code is wrong. Instead I asked why
>>>> you use the same ID. Meaning, is there anything in the DSDT requiring
>>>> this value?
>>>>
>>>>> + int tras_id = 0;
>>>>
>>>> unsigned.
>>>>
>>>>> + list_for_each_entry(its_data, &host_its_list, entry)
>>>>> + {
>>>>> +    gic_its->translation_id = ++trans_id;
>>>>
>>>> You start the translation ID at 1. Why?
>>>>
>>> as per the ACPI spec the value should be unique to each GIC ITS unit.
>>> Does starting with 1 break anything? Or should I start with a magic
>>> number?
>>
>> Rather than arguing on the start value here, you should have first
>> answer to the question regarding the usage of translation_id.
> in v1 I assumed that it would be the same as read from host its tables,
> so it would have a unique value as programmed by host firmware.
>>
>> I understand that nobody is using it today. However, when I asked
>> around me nobody ruled out to any future usage of GIC ITS ID and
>> request this to be kept as it is.
>>
>> This means that you can simply copy over the ACPI tables. Rather
>> regenerating them.
>>
> I dont follow your comment, a bit confused
> In v1 you mentioned that "Please explain why you need to have the same
> ID as the host."
> now when you say we copy over the translation_id would be same as that
> of host?

Usually when I say: "Please explain..." it means I want more 
documentation in the code because I am not sure to follow why it is 
necessary. It does not mean "The code is wrong". If it was, I would have 
clearly wrote it and give justification on it.

Furthermore, this kind of documentation will help a reader to understand 
your code and avoid spending hours to find a justification.

The contributor should be able to justify any code he wrote and help the 
reviewers to understand the patch quickly.

Cheers,

-- 
Julien Grall

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