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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Stefano,

> On 3 Dec 2020, at 4:13 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 2 Dec 2020, Julien Grall wrote:
>> On 02/12/2020 02:51, Stefano Stabellini wrote:
>>> On Thu, 26 Nov 2020, Rahul Singh wrote:
>>>> +/* Alias to Xen device tree helpers */
>>>> +#define device_node dt_device_node
>>>> +#define of_phandle_args dt_phandle_args
>>>> +#define of_device_id dt_device_match
>>>> +#define of_match_node dt_match_node
>>>> +#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np,
>>>> pname, out))
>>>> +#define of_property_read_bool dt_property_read_bool
>>>> +#define of_parse_phandle_with_args dt_parse_phandle_with_args
>>> 
>>> Given all the changes to the file by the previous patches we are
>>> basically fully (or almost fully) adapting this code to Xen.
>>> 
>>> So at that point I wonder if we should just as well make these changes
>>> (e.g. s/of_phandle_args/dt_phandle_args/g) to the code too.
>> 
>> I have already accepted the fact that keeping Linux code as-is is nearly
>> impossible without much workaround :). The benefits tends to also limited as
>> we noticed for the SMMU driver.
>> 
>> I would like to point out that this may make quite difficult (if not
>> impossible) to revert the previous patches which remove support for some
>> features (e.g. atomic, MSI, ATS).
>> 
>> If we are going to adapt the code to Xen (I'd like to keep Linux code style
>> though), then I think we should consider to keep code that may be useful in
>> the near future (at least MSI, ATS).
> 
> (I am fine with keeping the Linux code style.)
> 
> We could try to keep the code as similar to Linux as possible. This
> didn't work out in the past.
> 
> Otherwise, we could fully adapt the driver to Xen. If we fully adapt the
> driver to Xen (code style aside) it is better to be consistent and also
> do substitutions like s/of_phandle_args/dt_phandle_args/g. Then the
> policy becomes clear: the code comes from Linux but it is 100% adapted
> to Xen.
> 
> 
> Now the question about what to do about the MSI and ATS code is a good
> one. We know that we are going to want that code at some point in the
> next 2 years. Like you wrote, if we fully adapt the code to Xen and
> remove MSI and ATS code, then it is going to be harder to add it back.
> 
> So maybe keeping the MSI and ATS code for now, even if it cannot work,
> would be better. I think this strategy works well if the MSI and ATS
> code can be disabled easily, i.e. with a couple of lines of code in the
> init function rather than #ifdef everywhere. It doesn't work well if we
> have to add #ifdef everywhere.
> 
> It looks like MSI could be disabled adding a couple of lines to
> arm_smmu_setup_msis.
> 
> Similarly ATS seems to be easy to disable by forcing ats_enabled to
> false.
> 
> So yes, this looks like a good way forward. Rahul, what do you think?


I am ok to have the PCI ATS and MSI functionality in the code. 
As per the discussion next version of the patch will include below 
modification:Please let me know if there are any suggestion overall that should 
be added in next version.

1. Keep the PCI ATS and MSI functionality code.
2. Make the code with XEN compatible ( remove linux compatibility functions)
3. Keep the Linux coding style for code imported from Linux.
4. Fix all comments.

I have one query what will be coding style for new code to make driver work  
for XEN ? 


> 
> 
> 
>>>> +#define FIELD_GET(_mask, _reg)          \
>>>> +    (typeof(_mask))(((_reg) & (_mask)) >> (__builtin_ffsll(_mask) - 1))
>>>> +
>>>> +#define WRITE_ONCE(x, val)                  \
>>>> +do {                                        \
>>>> +    *(volatile typeof(x) *)&(x) = (val);    \
>>>> +} while (0)
>>> 
>>> maybe we should define this in xen/include/xen/lib.h
>> 
>> I have attempted such discussion in the past and this resulted to more
>> bikeshed than it is worth it. So I would suggest to re-implement WRITE_ONCE()
>> as write_atomic() for now.
> 
> Good suggestion, less discussions more code :-)

I will use the write_atomic.

Regards,
Rahul