On 20.10.20 13:51, Paul Durrant wrote:
Hi Paul.
Sorry for the late response.
-----Original Message-----
From: Oleksandr Tyshchenko <olekst...@gmail.com>
Sent: 15 October 2020 17:44
To: xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>; Stefano Stabellini
<sstabell...@kernel.org>;
Julien Grall <jul...@xen.org>; Volodymyr Babchuk <volodymyr_babc...@epam.com>;
Andrew Cooper
<andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>; Ian
Jackson
<i...@xenproject.org>; Jan Beulich <jbeul...@suse.com>; Wei Liu <w...@xen.org>;
Paul Durrant
<p...@xen.org>; Julien Grall <julien.gr...@arm.com>
Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
This patch introduces a helper the main purpose of which is to check
if a domain is using IOREQ server(s).
On Arm the current benefit is to avoid calling handle_io_completion()
(which implies iterating over all possible IOREQ servers anyway)
on every return in leave_hypervisor_to_guest() if there is no active
servers for the particular domain.
Also this helper will be used by one of the subsequent patches on Arm.
This involves adding an extra per-domain variable to store the count
of servers in use.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
CC: Julien Grall <julien.gr...@arm.com>
---
Please note, this is a split/cleanup/hardening of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"
Changes RFC -> V1:
- new patch
Changes V1 -> V2:
- update patch description
- guard helper with CONFIG_IOREQ_SERVER
- remove "hvm" prefix
- modify helper to just return d->arch.hvm.ioreq_server.nr_servers
- put suitable ASSERT()s
- use ASSERT(d->ioreq_server.server[id] ? !s : !!s) in set_ioreq_server()
- remove d->ioreq_server.nr_servers = 0 from hvm_ioreq_init()
---
xen/arch/arm/traps.c | 15 +++++++++------
xen/common/ioreq.c | 7 ++++++-
xen/include/xen/ioreq.h | 14 ++++++++++++++
xen/include/xen/sched.h | 1 +
4 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 507c095..a8f5fdf 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2261,14 +2261,17 @@ static bool check_for_vcpu_work(void)
struct vcpu *v = current;
#ifdef CONFIG_IOREQ_SERVER
- bool handled;
+ if ( domain_has_ioreq_server(v->domain) )
+ {
+ bool handled;
- local_irq_enable();
- handled = handle_io_completion(v);
- local_irq_disable();
+ local_irq_enable();
+ handled = handle_io_completion(v);
+ local_irq_disable();
- if ( !handled )
- return true;
+ if ( !handled )
+ return true;
+ }
#endif
if ( likely(!v->arch.need_flush_to_ram) )
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index bcd4961..a72bc0e 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -39,9 +39,14 @@ static void set_ioreq_server(struct domain *d, unsigned int
id,
struct ioreq_server *s)
{
ASSERT(id < MAX_NR_IOREQ_SERVERS);
- ASSERT(!s || !d->ioreq_server.server[id]);
+ ASSERT(d->ioreq_server.server[id] ? !s : !!s);
That looks odd. How about ASSERT(!s ^ !d->ioreq_server.server[id])?
ok, looks like it will work.
Paul
d->ioreq_server.server[id] = s;
+
+ if ( s )
+ d->ioreq_server.nr_servers++;
+ else
+ d->ioreq_server.nr_servers--;
}
#define GET_IOREQ_SERVER(d, id) \
diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
index 7b03ab5..0679fef 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -55,6 +55,20 @@ struct ioreq_server {
uint8_t bufioreq_handling;
};
+#ifdef CONFIG_IOREQ_SERVER
+static inline bool domain_has_ioreq_server(const struct domain *d)
+{
+ ASSERT((current->domain == d) || atomic_read(&d->pause_count));
+
This seems like an odd place to put such an assertion.
I might miss something or interpreted incorrectly but these asserts are
the result of how I understood the review comment on previous version [1].
I will copy a comment here for the convenience:
"This is safe only when d == current->domain and it's not paused,
or when they're distinct and d is paused. Otherwise the result is
stale before the caller can inspect it. This wants documenting by
at least a comment, but perhaps better by suitable ASSERT()s."
+ return d->ioreq_server.nr_servers;
+}
+#else
+static inline bool domain_has_ioreq_server(const struct domain *d)
+{
+ return false;
+}
+#endif
+
Can this be any more compact? E.g.
return IS_ENABLED(CONFIG_IOREQ_SERVER) && d->ioreq_server.nr_servers;
?
I have got a compilation error this way (if CONFIG_IOREQ_SERVER is
disabled):
...xen/4.14.0+gitAUTOINC+ee22110219-r0/git/xen/include/xen/ioreq.h:62:48:
error: ‘const struct domain’ has no member named ‘ioreq_server’
return IS_ENABLED(CONFIG_IOREQ_SERVER) && d->ioreq_server.nr_servers;
^
as domain's ioreq_server struct is guarded by CONFIG_IOREQ_SERVER as well.
[1]
https://patchwork.kernel.org/project/xen-devel/patch/1599769330-17656-12-git-send-email-olekst...@gmail.com/#23618623
Thank you.
--
Regards,
Oleksandr Tyshchenko
