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

[Oleksandr <olekstysh@xxxxxxxxx>]

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