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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 11.11.20 10:08, Jan Beulich wrote:

Hi Jan

> On 10.11.2020 21:53, Oleksandr wrote:
> > On 20.10.20 13:51, Paul Durrant wrote:
> >
> > Hi Paul.
> >
> > Sorry for the late response.
> >
> > > > -----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])?
> > ok, looks like it will work.
> >
> >
> > >     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.
> > I might miss something or interpreted incorrectly but these asserts are
> > the result of how I understood the review comment on previous version [1].
> >
> > I will copy a comment here for the convenience:
> > "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."
> The way his reply was worded, I think Paul was wondering about the
> place where you put the assertion, not what you actually assert.
Shall I put the assertion at the call sites of this helper instead?


>   
>
> > > > +    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;
> > >
> > > ?
> > I have got a compilation error this way (if CONFIG_IOREQ_SERVER is
> > disabled):
> >
> > ...xen/4.14.0+gitAUTOINC+ee22110219-r0/git/xen/include/xen/ioreq.h:62:48:
> > error: ‘const struct domain’ has no member named ‘ioreq_server’
> >        return IS_ENABLED(CONFIG_IOREQ_SERVER) && d->ioreq_server.nr_servers;
> >                                                   ^
> > as domain's ioreq_server struct is guarded by CONFIG_IOREQ_SERVER as well.
> The #ifdef is unavoidable here, I agree, but it should be inside
> the function's body. There's no need to duplicate the rest of it.

Got it, will do.


--
Regards,

Oleksandr Tyshchenko