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

[Oleksandr <olekstysh@gmail.com>]

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@arm.com>
>>>>
>>>> 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@arm.com>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> ---
>>>> 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