On 20.01.21 17:50, Julien Grall wrote:
Hi Oleksandr,
Hi Julien
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.
Yes, I assume you meant in "ioreq.h"
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?
Including it directly doesn't solve build issue.
If I am not mistaken in order to follow requirements how to include
headers (alphabetic order, public* should be included after xen* and
asm* ones, etc)
the dm.h gets included the first in dm.c, and dm_op.h gets included the
last. We can avoid build issue if we change inclusion order a bit,
what I mean is to include dm.h after hypercall.h at least (because
hypercall.h already includes dm_op.h). But this breaks the requirements
and is not way to go.
Now I am in doubt how to overcome this.
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?
The same, please ignore them, sorry for the confusion.
[...]
#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.
Thank you, I think, no. Everything is clear at the moment.
Cheers,
--
Regards,
Oleksandr Tyshchenko