On 17/01/2021 17:11, Oleksandr wrote:
On 15.01.21 22:26, Julien Grall wrote:
Hi Julien
Hi Oleksandr,
+
PROGRESS(xen):
ret = relinquish_memory(d, &d->xenpage_list);
if ( ret )
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index ae7ef96..9814481 100644
--- 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>
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).
Fair enough.
[...]
If you return IO_HANDLED here, then it means the we will take care of
previous I/O but the current one is going to be ignored.
Which current one? As I understand, if try_fwd_ioserv() gets called with
vio->req.state == STATE_IORESP_READY then this is a second round after
emulator completes the emulation (the first round was when
we returned IO_RETRY down the function and claimed that we would need a
completion), so we are still dealing with previous I/O.
vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() ->
try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv()
And after we return IO_HANDLED here, handle_ioserv() will be called to
complete the handling of this previous I/O emulation.
Or I really missed something?
Hmmm... I somehow thought try_fw_ioserv() would only be called the first
time. Do you have a branch with your code applied? This would help to
follow the different paths.
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?
[...]
#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?
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".
Cheers,
--
Julien Grall
