On 16.09.20 11:04, Jan Beulich wrote:
Hi Jan
On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
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 benefit is to avoid calling handle_hvm_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.
This involves adding an extra per-domain variable to store the count
of servers in use.
Since only Arm needs the variable (and the helper), perhaps both should
be Arm-specific (which looks to be possible without overly much hassle)?
Please note that the whole ioreq_server struct are going be moved to
"common" domain and new variable is going to go into it.
I am wondering whether this single-line helper could be used on x86 or
potential new arch ...
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -38,9 +38,15 @@ static void set_ioreq_server(struct domain *d, unsigned int
id,
struct hvm_ioreq_server *s)
{
ASSERT(id < MAX_NR_IOREQ_SERVERS);
- ASSERT(!s || !d->arch.hvm.ioreq_server.server[id]);
+ ASSERT((!s && d->arch.hvm.ioreq_server.server[id]) ||
+ (s && !d->arch.hvm.ioreq_server.server[id]));
For one, this can be had with less redundancy (and imo even improved
clarity, but I guess this latter aspect my depend on personal
preferences):
ASSERT(d->arch.hvm.ioreq_server.server[id] ? !s : !!s);
This construction indeed better.
But then I wonder whether the original intention wasn't rather such
that replacing NULL by NULL is permissible. Paul?
d->arch.hvm.ioreq_server.server[id] = s;
+
+ if ( s )
+ d->arch.hvm.ioreq_server.nr_servers ++;
+ else
+ d->arch.hvm.ioreq_server.nr_servers --;
Nit: Stray blanks (should be there only with binary operators).
ok
@@ -1395,6 +1401,7 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool
buffered)
void hvm_ioreq_init(struct domain *d)
{
spin_lock_init(&d->arch.hvm.ioreq_server.lock);
+ d->arch.hvm.ioreq_server.nr_servers = 0;
There's no need for this - struct domain instances start out all
zero anyway.
ok
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -57,6 +57,11 @@ struct hvm_ioreq_server {
uint8_t bufioreq_handling;
};
+static inline bool hvm_domain_has_ioreq_server(const struct domain *d)
+{
+ return (d->arch.hvm.ioreq_server.nr_servers > 0);
+}
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.
Agree, will use ASSERT()s.
As in earlier patches I don't think a hvm_ prefix should be used
here.
ok
Also as a nit: The parentheses here are unnecessary, and strictly
speaking the "> 0" is, too.
ok
--
Regards,
Oleksandr Tyshchenko
