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

From: Oleksandr <olekstysh@gmail.com>
To: Jan Beulich <JBeulich@suse.com>,
	Julien Grall <julien.grall@arm.com>, Paul Durrant <paul@xen.org>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Daniel De Graaf" <dgdegra@tycho.nsa.gov>,
	"Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH V3 09/23] xen/dm: Make x86's DM feature common
Date: Thu, 7 Jan 2021 18:49:26 +0200	[thread overview]
Message-ID: <7ca63ab6-85c8-2b5a-2be9-dcb4aaed7ac3@gmail.com> (raw)
In-Reply-To: <318580a5-d02f-9663-cc58-dfad8a4f7836@suse.com>

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