Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()

From: Jan Beulich <jbeulich@suse.com>
To: Oleksandr <olekstysh@gmail.com>
Cc: xen-devel@lists.xenproject.org,
	"'Oleksandr Tyshchenko'" <oleksandr_tyshchenko@epam.com>,
	"'Andrew Cooper'" <andrew.cooper3@citrix.com>,
	"'Roger Pau Monné'" <roger.pau@citrix.com>,
	"'Wei Liu'" <wl@xen.org>, "'Julien Grall'" <julien@xen.org>,
	"'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Julien Grall'" <julien.grall@arm.com>,
	paul@xen.org
Subject: Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
Date: Wed, 11 Nov 2020 09:16:19 +0100	[thread overview]
Message-ID: <2e677d01-bf2f-e569-95a2-490dc05102d9@suse.com> (raw)
In-Reply-To: <35d38051-129e-333d-ec94-aa36e68a3814@gmail.com>

On 11.11.2020 09:09, Oleksandr wrote:
> 
> On 11.11.20 09:27, Jan Beulich wrote:
> 
> Hi Jan
> 
>> On 10.11.2020 20:44, Oleksandr wrote:
>>> On 20.10.20 13:38, Paul Durrant wrote:
>>>
>>> Hi Jan, Paul
>>>
>>> Sorry for the late response.
>>>
>>>>> -----Original Message-----
>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>> Sent: 20 October 2020 11:05
>>>>> To: paul@xen.org
>>>>> Cc: 'Oleksandr Tyshchenko' <olekstysh@gmail.com>; xen-devel@lists.xenproject.org; 'Oleksandr
>>>>> Tyshchenko' <oleksandr_tyshchenko@epam.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Roger Pau
>>>>> Monné' <roger.pau@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Julien Grall' <julien@xen.org>; 'Stefano
>>>>> Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien.grall@arm.com>
>>>>> Subject: Re: [PATCH V2 04/23] xen/ioreq: Provide alias for the handle_mmio()
>>>>>
>>>>> On 20.10.2020 11:14, Paul Durrant wrote:
>>>>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko
>>>>>>> Sent: 15 October 2020 17:44
>>>>>>>
>>>>>>> --- a/xen/include/asm-x86/hvm/ioreq.h
>>>>>>> +++ b/xen/include/asm-x86/hvm/ioreq.h
>>>>>>> @@ -181,6 +181,8 @@ static inline bool arch_hvm_ioreq_destroy(struct domain *d)
>>>>>>>    #define IOREQ_STATUS_UNHANDLED   X86EMUL_UNHANDLEABLE
>>>>>>>    #define IOREQ_STATUS_RETRY       X86EMUL_RETRY
>>>>>>>
>>>>>>> +#define ioreq_complete_mmio   handle_mmio
>>>>>>> +
>>>>>> A #define? Really? Can we not have a static inline?
>>>>> I guess this would require further shuffling: handle_mmio() is
>>>>> an inline function in hvm/emulate.h, and hvm/ioreq.h has no
>>>>> need to include the former (and imo it also shouldn't have).
>>>>>
>>>> I see. I think we need an x86 ioreq.c anyway, to deal with the legacy use of magic pages, so it could be dealt with there instead.
>>> I am afraid I don't entirely understand the required changes. Could you
>>> please clarify where the "inline(?)" ioreq_complete_mmio() should
>>> live? I included hvm/emulate.h here not for the "handle_mmio()" reason
>>> only, but for "struct hvm_emulate_ctxt" also (see arch_io_completion()).
>> I'm sorry, but in the context of this patch there's no use of any
>> struct hvm_emulate_ctxt instance. I'm not going to wade through 23
>> patches to find what you mean.
> 
> Sorry for not being precise here. I meant arch_io_completion() added at [1]

At least some of the inlines you add there are way too large to be
inline functions, imo. But consensus appears to be now to retain a
per-arch ioreq.c anyway.

Jan

> [1] 
> https://patchwork.kernel.org/project/xen-devel/patch/1602780274-29141-2-git-send-email-olekstysh@gmail.com/
> 