> -----Original Message-----
> From: Jan Beulich <[email protected]>
> Sent: 16 September 2020 09:05
> To: Oleksandr Tyshchenko <[email protected]>; Paul Durrant <[email protected]>
> Cc: [email protected]; Oleksandr Tyshchenko
> <[email protected]>; Stefano
> Stabellini <[email protected]>; Julien Grall <[email protected]>; Volodymyr
> Babchuk
> <[email protected]>; Andrew Cooper <[email protected]>; Wei
> Liu <[email protected]>; Roger
> Pau Monné <[email protected]>; Julien Grall <[email protected]>
> 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 <[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 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
