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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

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

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

> > [...]
> >
> > > > >   #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.

Cheers,

--
Julien Grall