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 <olekst...@gmail.com>
>>> Sent: 15 October 2020 17:44
>>> To: xen-devel@lists.xenproject.org
>>> Cc: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>; Stefano
>>> Stabellini <sstabell...@kernel.org>;
>>> Julien Grall <jul...@xen.org>; Volodymyr Babchuk
>>> <volodymyr_babc...@epam.com>; Andrew Cooper
>>> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>; Ian
>>> Jackson
>>> <i...@xenproject.org>; Jan Beulich <jbeul...@suse.com>; Wei Liu
>>> <w...@xen.org>; Paul Durrant
>>> <p...@xen.org>; Julien Grall <julien.gr...@arm.com>
>>> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
>>>
>>> From: Oleksandr Tyshchenko <oleksandr_tyshche...@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_tyshche...@epam.com>
>>> CC: Julien Grall <julien.gr...@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
