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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 18.01.21 18:00, Jan Beulich wrote:

Hi Jan

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

Thank you for the clarification.

--
Regards,

Oleksandr Tyshchenko