Re: [PATCH V1 11/16] xen/ioreq: Introduce hvm_domain_has_ioreq_server()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> 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 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)?

> --- 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);

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).

> @@ -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.

> --- 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.

As in earlier patches I don't think a hvm_ prefix should be used
here.

Also as a nit: The parentheses here are unnecessary, and strictly
speaking the "> 0" is, too.

Jan