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

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien@xen.org>
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>
Subject: Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features
Date: Thu, 21 Jan 2021 10:50:08 +0200	[thread overview]
Message-ID: <e0bc7f80-974e-945d-4605-173bd05302af@gmail.com> (raw)
In-Reply-To: <57d95c18-5215-03e7-7849-73c9fe968e75@xen.org>

On 20.01.21 17:50, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien

>
> On 17/01/2021 18:52, Oleksandr wrote:
>>>>>>   diff --git a/xen/include/asm-arm/domain.h 
>>>>>> b/xen/include/asm-arm/domain.h
>>>>>> index 6819a3b..c235e5b 100644
>>>>>> --- 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?
>>>
>>> If the failure are indeded because ioreq.h and dm.h use definition 
>>> from public/hvm/dm_op.h, then yes. Can you post the errors?
>> Please see attached, although I built for Arm32 (and the whole 
>> series), I think errors are valid for Arm64 also.
>
> Thanks!
>
>> error1.txt - when remove #include <public/hvm/dm_op.h> from 
>> asm-arm/domain.h
>
> For this one, I agree that including <public/hvm/dm_op.h> in xen.h 
> looks the best solution.

Yes, I assume you meant in "ioreq.h"

>
>
>> error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h
>
> It looks like the error is happening in dm.c rather than xen/dm.h. Any 
> reason to not include <public/hvm/dm_op.h> in dm.c directly?
Including it directly doesn't solve build issue.
If I am not mistaken in order to follow requirements how to include 
headers (alphabetic order, public* should be included after xen* and 
asm* ones, etc)
the dm.h gets included the first in dm.c, and dm_op.h gets included the 
last. We can avoid build issue if we change inclusion order a bit,
what I mean is to include dm.h after hypercall.h at least (because 
hypercall.h already includes dm_op.h). But this breaks the requirements 
and is not way to go.
Now I am in doubt how to overcome this.

>
>
>
>> error3.txt - when add #include <public/hvm/dm_op.h> to xen/dm.h
>
> I am a bit confused with this one. Isn't it the same as error1.txt?

The same, please ignore them, sorry for the confusion.

>
>
>>
>>
>>>
>>>
>>> [...]
>>>
>>>>>>   #include <public/hvm/params.h>
>>>>>>     struct hvm_domain
>>>>>> @@ -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?
>>>
>>> Sorry, I don't understand your answer/questions. So let me ask the 
>>> question differently, is asm-arm/hvm/ioreq.h going to be included 
>>> from common code?
>>
>> Sorry if I was unclear.
>>
>>
>>>
>>> If the answer is no, then I see no reason to follow the x86 here.
>>> If the answer is yes, then I am quite confused why half of the 
>>> series tried to remove "hvm" from the function name but we still 
>>> include "asm/hvm/ioreq.h".
>>
>> Answer is yes. Even if we could to avoid including that header from 
>> the common code somehow, we would still have #include <public/hvm/*>, 
>> is_hvm_domain().
>
> I saw Jan answered about this one. Let me know if you need more input.

Thank you, I think, no. Everything is clear at the moment.

>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko