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

[Jan Beulich <jbeulich@suse.com>]

On 11.11.2020 09:41, Oleksandr wrote:
> 
> 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@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."
>> 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?

Since Paul raised the question, I expect this is a question to him
rather than me?

Jan