[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

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.


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.


Julien Grall



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