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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 16.09.20 11:04, Jan Beulich wrote:

Hi Jan

> 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)?
Please note that the whole ioreq_server struct are going be moved to 
"common" domain and new variable is going to go into it.
I am wondering whether this single-line helper could be used on x86 or 
potential new arch ...

> > --- 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);
This construction indeed better.


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


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


> > --- 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.
Agree, will use ASSERT()s.


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


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


--
Regards,

Oleksandr Tyshchenko