RE: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
> Sent: 15 October 2020 17:44
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Volodymyr Babchuk 
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Ian 
> Jackson
> <iwj@xxxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Wei Liu <wl@xxxxxxx>; 
> Paul Durrant
> <paul@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> 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_tyshchenko@xxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> 
> ---
> 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