diff mbox series

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

Message ID 1602780274-29141-18-git-send-email-olekstysh@gmail.com (mailing list archive)
State New
Headers show
Series IOREQ feature (+ virtio-mmio) on Arm | expand

Commit Message

Oleksandr Oct. 15, 2020, 4:44 p.m. UTC
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(-)

Comments

Paul Durrant Oct. 20, 2020, 10:51 a.m. UTC | #1
> -----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])?

  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.

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

?

>  struct ioreq_server *get_ioreq_server(const struct domain *d,
>                                        unsigned int id);
> 
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index f9ce14c..290cddb 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -553,6 +553,7 @@ struct domain
>      struct {
>          spinlock_t              lock;
>          struct ioreq_server     *server[MAX_NR_IOREQ_SERVERS];
> +        unsigned int            nr_servers;
>      } ioreq_server;
>  #endif
>  };
> --
> 2.7.4
Oleksandr Nov. 10, 2020, 8:53 p.m. UTC | #2
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.
Jan Beulich Nov. 11, 2020, 8:08 a.m. UTC | #3
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. 

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

Jan
Oleksandr Nov. 11, 2020, 8:41 a.m. UTC | #4
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?


>   
>
>>>> +    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.
Jan Beulich Nov. 11, 2020, 1:27 p.m. UTC | #5
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
Paul Durrant Nov. 11, 2020, 4:28 p.m. UTC | #6
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 11 November 2020 13:28
> To: Oleksandr <olekstysh@gmail.com>
> 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>; 'Wei Liu' <wl@xen.org>; 'Julien Grall'
> <julien.grall@arm.com>; paul@xen.org; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
> 
> 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?

If it is indeed a question for me then yes, put the assertion where it is clear why it is needed. domain_has_ioreq_server() is essentially a trivial accessor function; it's not the appropriate place.

  Paul

> 
> Jan
Oleksandr Nov. 11, 2020, 5:31 p.m. UTC | #7
On 11.11.20 15:27, Jan Beulich wrote:

Hi Jan.

>
>>>>>>     }
>>>>>>
>>>>>>     #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?
Yes, it was primarily a question to Paul, but I wanted to hear your 
opinion as well. Sorry for the confusion.
Oleksandr Nov. 11, 2020, 5:33 p.m. UTC | #8
On 11.11.20 18:28, Paul Durrant wrote:

Hi 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?
> If it is indeed a question for me then yes, put the assertion where it is clear why it is needed. domain_has_ioreq_server() is essentially a trivial accessor function; it's not the appropriate place.

Got it. Thank you for the clarification.
diff mbox series

Patch

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);
 
     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));
+
+    return d->ioreq_server.nr_servers;
+}
+#else
+static inline bool domain_has_ioreq_server(const struct domain *d)
+{
+    return false;
+}
+#endif
+
 struct ioreq_server *get_ioreq_server(const struct domain *d,
                                       unsigned int id);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index f9ce14c..290cddb 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -553,6 +553,7 @@  struct domain
     struct {
         spinlock_t              lock;
         struct ioreq_server     *server[MAX_NR_IOREQ_SERVERS];
+        unsigned int            nr_servers;
     } ioreq_server;
 #endif
 };