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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 08.12.20 11:30, Jan Beulich wrote:

Hi Jan

> On 07.12.2020 21:23, Oleksandr wrote:
> > On 07.12.20 14:08, Jan Beulich wrote:
> > > On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:
> > > > From: Julien Grall <julien.grall@xxxxxxx>
> > > >
> > > > As a lot of x86 code can be re-used on Arm later on, this patch
> > > > splits devicemodel support into common and arch specific parts.
> > > >
> > > > The common DM feature is supposed to be built with IOREQ_SERVER
> > > > option enabled (as well as the IOREQ feature), which is selected
> > > > for x86's config HVM for now.
> > > >
> > > > Also update XSM code a bit to let DM op be used on Arm.
> > > >
> > > > This support is going to be used on Arm to be able run device
> > > > emulator outside of Xen hypervisor.
> > > >
> > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > >
> > > > ---
> > > > Please note, this is a split/cleanup/hardening of Julien's PoC:
> > > > "Add support for Guest IO forwarding to a device emulator"
> > > >
> > > > Changes RFC -> V1:
> > > >      - update XSM, related changes were pulled from:
> > > >        [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM 
> > > > features
> > > >
> > > > Changes V1 -> V2:
> > > >      - update the author of a patch
> > > >      - update patch description
> > > >      - introduce xen/dm.h and move definitions here
> > > >
> > > > Changes V2 -> V3:
> > > >      - no changes
> > > And my concern regarding the common vs arch nesting also hasn't
> > > changed.
> > I am sorry, I might misread your comment, but I failed to see any
> > obvious to me request(s) for changes.
> > I have just re-read previous discussion...
> > So the question about considering doing it the other way around (top
> > level dm-op handling arch-specific
> > and call into e.g. ioreq_server_dm_op() for otherwise unhandled ops) is
> > exactly a concern which I should have addressed?
> 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).

--
Regards,

Oleksandr Tyshchenko