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

[Oleksandr <olekstysh@xxxxxxxxx>]

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


--
Regards,

Oleksandr Tyshchenko