Re: [PATCH V3 09/23] xen/dm: Make x86's DM feature common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2021 15:38, Oleksandr wrote:
>>> Well, on v2 you replied you didn't consider the alternative. I would
>>> have expected that you would at least go through this consideration
>>> process, and see whether there are better reasons to stick with the
>>> apparently backwards arrangement than to change to the more
>>> conventional one. If there are such reasons, I would expect them to
>>> be called out in reply and perhaps also in the commit message; the
>>> latter because down the road more people may wonder why the more
>>> narrow / special set of cases gets handled at a higher layer than
>>> the wider set of remaining ones, and they would then be able to find
>>> an explanation without having to resort to searching through list
>>> archives.
>> Ah, will investigate. Sorry for not paying enough attention to it.
>> Yes, IOREQ (I mean "common") ops are 7 out of 18 right now. The 
>> subsequent patch is adding one more DM op - XEN_DMOP_set_irq_level.
>> There are several PCI related ops which might want to be common in the 
>> future (but I am not sure).
> I think, I can say that I have considered the alternative (doing it the 
> other way around), of course if I got your suggestion for V2 correctly.
> Agree, the alternative is more natural, also compat_dm_op() was left in 
> x86 code. For me the downside is in code duplication. With the 
> alternative both arches have to duplicate do_dm_op() and "common" part 
> of dm_op()
> (only "switch ( op.op )" is unique).

Yes, this duplication is the main downside.

> Now the question is which approach to take ("current" or "alternative") 
> for me to prepare for V4. Personally, I would be OK with the both (with 
> a little preference for "alternative").

Same here. I don't think I've seen anyone else voice an opinion.

> Also, If we decide to go with the alternative, should the common files 
> still be named dm.*?

I think this should live in ioreq.c, just like e.g.
iommu_do_pci_domctl() lives in passthrough/pci.c. This would then
allow quite a few things to become static in that file, I believe.

Jan