Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

From: Oleksandr <olekstysh@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: xen-devel@lists.xenproject.org,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Julien Grall <julien@xen.org>
Subject: Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features
Date: Mon, 18 Jan 2021 17:52:26 +0200	[thread overview]
Message-ID: <758fea3f-8a67-7541-1fa6-cf9898b4c336@gmail.com> (raw)
In-Reply-To: <9904062b-cc59-c80e-50fa-ea932c8a9bd5@suse.com>

On 18.01.21 12:44, Jan Beulich wrote:

Hi Jan

> On 17.01.2021 18:11, Oleksandr wrote:
>> On 15.01.21 22:26, Julien Grall wrote:
>>> On 12/01/2021 21:52, Oleksandr Tyshchenko wrote:
>>>> --- a/xen/arch/arm/io.c
>>>> +++ b/xen/arch/arm/io.c
>>>> @@ -16,6 +16,7 @@
>>>>     * GNU General Public License for more details.
>>>>     */
>>>>    +#include <xen/ioreq.h>
>>>>    #include <xen/lib.h>
>>>>    #include <xen/spinlock.h>
>>>>    #include <xen/sched.h>
>>>> @@ -23,6 +24,7 @@
>>>>    #include <asm/cpuerrata.h>
>>>>    #include <asm/current.h>
>>>>    #include <asm/mmio.h>
>>>> +#include <asm/hvm/ioreq.h>

Note to self:

Remove obsolete bool ioreq_complete_mmio(void) from asm-arm/hvm/ioreq.h

>>> Shouldn't this have been included by "xen/ioreq.h"?
>> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it turned
>> out that there was nothing inside common header required arch one to be
>> included and
>> I was asked to include arch header where it was indeed needed (several
>> *.c files).
> I guess the general usage model of the two headers needs to be
> established first: If the per-arch header declares only stuff
> needed by the soon common/ioreq.c, then indeed it should be
> only that file and the producer(s) of the arch_*() functions
> which include that header; it should then in particular not be
> included by xen/ioreq.h.
>
> However, with the change request on patch 1 I think that usage
> model goes away at least for now, at which point the question
> is what exactly the per-arch header still declares, and based
> on that it would need to be decided whether xen/ioreq.h
> should include it.

ok, well.

x86's arch header now contains few IOREQ_STATUS_* #define-s, but Arm's 
contains more stuff
besides that:
- stuff which is needed by common/ioreq.c, mostly stubs which are not 
implemented yet (handle_pio, etc)
- stuff which is not needed by common/ioreq.c, internal Arm bits 
(handle_ioserv, try_fwd_ioserv)

Could we please decide based on the information above?

>
>>>> --- a/xen/include/asm-arm/domain.h
>>>> +++ b/xen/include/asm-arm/domain.h
>>>> @@ -10,6 +10,7 @@
>>>>    #include <asm/gic.h>
>>>>    #include <asm/vgic.h>
>>>>    #include <asm/vpl011.h>
>>>> +#include <public/hvm/dm_op.h>
>>> May I ask, why do you need to include dm_op.h here?
>> I needed to include that header to make some bits visible
>> (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a
>> really good question.
>> I don't remember exactly, probably I followed x86's domain.h which also
>> included it.
>> So, trying to remove the inclusion here, I get several build failures on
>> Arm which could be fixed if I include that header from dm.h and ioreq.h:
>>
>> Shall I do this way?
> The general rule ought to be that header include what they need,
> but not more. Header dependencies are quite problematic already,
> so every dependency we can avoid (or eliminate) will help. This
> goes as far as only forward declaring structure where possible.

I got it.

>
>>>> @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu
>>>> *v) {}
>>>>      #define arch_vm_assist_valid_mask(d) (1UL <<
>>>> VMASST_TYPE_runstate_update_flag)
>>>>    +#define has_vpci(d)    ({ (void)(d); false; })
>>>> +
>>>>    #endif /* __ASM_DOMAIN_H__ */
>>>>      /*
>>>> diff --git a/xen/include/asm-arm/hvm/ioreq.h
>>>> b/xen/include/asm-arm/hvm/ioreq.h
>>>> new file mode 100644
>>>> index 0000000..19e1247
>>>> --- /dev/null
>>>> +++ b/xen/include/asm-arm/hvm/ioreq.h
>>> Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ as
>>> the IOREQ is now meant to be agnostic?
>> Good question... The _common_ IOREQ code is indeed arch-agnostic. But,
>> can the _arch_ IOREQ code be treated as really subarch-agnostic?
>> I think, on Arm it can and it is most likely ok to keep it in
>> "asm-arm/", but how it would be correlated with x86's IOREQ code which
>> is HVM specific and located
>> in "hvm" subdir?
> I think for Arm's sake this should be used as asm/ioreq.h, where
> x86 would gain a new header consisting of just
>
> #include <asm/hvm/ioreq.h>
>
> as there the functionality is needed for HVM only.
For me this sounds perfectly fine. I think, this would also address 
Julien's question.
May I introduce that new header together with moving IOREQ to the common 
code (patch #4)?

-- 
Regards,

Oleksandr Tyshchenko