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

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi Jan.


On 07.01.21 18:49, Oleksandr wrote:
> On 07.01.21 17:01, Jan Beulich wrote:
>
> Hi Jan, all
>
> > 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.
> Well, let's wait a bit for other opinions... @Julien, @Paul what do 
> you think on that?
>
> > > 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.
> I got it. It seems yes, at least the following could became static:
>
> ioreq_server_create
> ioreq_server_get_info
> ioreq_server_map_io_range
> ioreq_server_unmap_io_range
> ioreq_server_set_state
> ioreq_server_destroy

Well, I have already pushed V4 with this "alternative" approach, let's 
discuss there [1].

[1] 
https://lore.kernel.org/xen-devel/1610488352-18494-10-git-send-email-olekstysh@xxxxxxxxx/T/#mb08f75657f43df869596c5c9c30e2395a9e35c7a

--
Regards,

Oleksandr Tyshchenko