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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2021 16:52, Oleksandr wrote:
> On 18.01.21 12:44, Jan Beulich wrote:
>> 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?

You're in the best position to tell. The IOREQ_STATUS_* you
mention may require including from xen/ioreq.h, but as said,
...

>>>>> --- 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.

... it depends. If xen/ioreq.h needs nothing from asm/ioreq.h,
the I wouldn't see why it should include 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)?

As with about everything, introduce new things the first time you
need them, unless this results in overly big patches (in which
case suitably splitting up is desirable, but of course no always
possible). IOW if you introduce xen/ioreq.h and it needs to
include asm/ioreq.h, then of course at this point you also need
to introduce the asm-x86/ioreq.h wrapper.

Jan