> -----Original Message-----
> From: Oleksandr Tyshchenko <[email protected]>
> Sent: 15 October 2020 17:44
> To: [email protected]
> Cc: Oleksandr Tyshchenko <[email protected]>; Stefano Stabellini
> <[email protected]>;
> Julien Grall <[email protected]>; Volodymyr Babchuk
> <[email protected]>; Andrew Cooper
> <[email protected]>; George Dunlap <[email protected]>; Ian
> Jackson
> <[email protected]>; Jan Beulich <[email protected]>; Wei Liu
> <[email protected]>; Paul Durrant
> <[email protected]>; Julien Grall <[email protected]>
> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
>
> From: Oleksandr Tyshchenko <[email protected]>
>
> 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 <[email protected]>
> CC: Julien Grall <[email protected]>
>
> ---
> 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])?
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.
> + 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;
?
> struct ioreq_server *get_ioreq_server(const struct domain *d,
> unsigned int id);
>
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index f9ce14c..290cddb 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -553,6 +553,7 @@ struct domain
> struct {
> spinlock_t lock;
> struct ioreq_server *server[MAX_NR_IOREQ_SERVERS];
> + unsigned int nr_servers;
> } ioreq_server;
> #endif
> };
> --
> 2.7.4
