[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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

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

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.