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

[Oleksandr <olekstysh@gmail.com>]

On 20.10.20 13:51, Paul Durrant wrote:

Hi Paul.

Sorry for the late response.

>
>> -----Original Message-----
>> From: Oleksandr Tyshchenko <olekstysh@gmail.com>
>> Sent: 15 October 2020 17:44
>> To: xen-devel@lists.xenproject.org
>> Cc: 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>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
>> <iwj@xenproject.org>; Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>; Paul Durrant
>> <paul@xen.org>; Julien Grall <julien.grall@arm.com>
>> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
>>
>> 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 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@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> 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."


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


[1] 
https://patchwork.kernel.org/project/xen-devel/patch/1599769330-17656-12-git-send-email-olekstysh@gmail.com/#23618623

Thank you.

-- 
Regards,

Oleksandr Tyshchenko