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




 


Rackspace

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