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

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 16 September 2020 09:05
> To: Oleksandr Tyshchenko <olekstysh@gmail.com>; Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger
> Pau Monné <roger.pau@citrix.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH V1 11/16] xen/ioreq: Introduce hvm_domain_has_ioreq_server()
> 
> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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.
> >

Is this really worth it? The limit on the number of ioreq serves is small... just 8. I doubt you'd be able measure the difference.

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

Yikes, that's a long time ago.. but I can't see why the check for !s would be there unless it was indeed intended to allow replacing NULL with NULL.

  Paul