Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
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_tbufioreq_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.
--
Regards,
Oleksandr Tyshchenko
Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
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_tbufioreq_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.
--
Regards,
Oleksandr Tyshchenko
RE: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
> -Original Message-
> From: Jan Beulich
> Sent: 11 November 2020 13:28
> To: Oleksandr
> Cc: 'Oleksandr Tyshchenko' ; 'Stefano
> Stabellini'
> ; 'Julien Grall' ; 'Volodymyr Babchuk'
> ; 'Andrew Cooper' ;
> 'George Dunlap'
> ; 'Ian Jackson' ; 'Wei Liu'
> ; 'Julien Grall'
> ; p...@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
> >>>>> Sent: 15 October 2020 17:44
> >>>>> To: xen-devel@lists.xenproject.org
> >>>>> Cc: Oleksandr Tyshchenko ; Stefano
> >>>>> Stabellini
> ;
> >>>>> Julien Grall ; Volodymyr Babchuk
> >>>>> ; Andrew Cooper
> >>>>> ; George Dunlap ;
> >>>>> Ian Jackson
> >>>>> ; Jan Beulich ; Wei Liu
> >>>>> ; Paul Durrant
> >>>>> ; Julien Grall
> >>>>> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
> >>>>>
> >>>>> From: Oleksandr Tyshchenko
> >>>>>
> >>>>> 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
> >>>>> CC: Julien Grall
> >>>>>
> >>>>> ---
> >>>>> 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
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
>>>>> Sent: 15 October 2020 17:44
>>>>> To: xen-devel@lists.xenproject.org
>>>>> Cc: Oleksandr Tyshchenko ; Stefano
>>>>> Stabellini ;
>>>>> Julien Grall ; Volodymyr Babchuk
>>>>> ; Andrew Cooper
>>>>> ; George Dunlap ;
>>>>> Ian Jackson
>>>>> ; Jan Beulich ; Wei Liu
>>>>> ; Paul Durrant
>>>>> ; Julien Grall
>>>>> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
>>>>>
>>>>> From: Oleksandr Tyshchenko
>>>>>
>>>>> 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
>>>>> CC: Julien Grall
>>>>>
>>>>> ---
>>>>> 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)
>>>>>{
>>
Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
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
Sent: 15 October 2020 17:44
To: xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko ; Stefano Stabellini
;
Julien Grall ; Volodymyr Babchuk ;
Andrew Cooper
; George Dunlap ; Ian
Jackson
; Jan Beulich ; Wei Liu ;
Paul Durrant
; Julien Grall
Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
From: Oleksandr Tyshchenko
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
CC: Julien Grall
---
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_tbufioreq_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/i
Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
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
>>> Sent: 15 October 2020 17:44
>>> To: xen-devel@lists.xenproject.org
>>> Cc: Oleksandr Tyshchenko ; Stefano
>>> Stabellini ;
>>> Julien Grall ; Volodymyr Babchuk
>>> ; Andrew Cooper
>>> ; George Dunlap ; Ian
>>> Jackson
>>> ; Jan Beulich ; Wei Liu
>>> ; Paul Durrant
>>> ; Julien Grall
>>> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
>>>
>>> From: Oleksandr Tyshchenko
>>>
>>> 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
>>> CC: Julien Grall
>>>
>>> ---
>>> 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_
Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
On 20.10.20 13:51, Paul Durrant wrote:
Hi Paul.
Sorry for the late response.
-Original Message-
From: Oleksandr Tyshchenko
Sent: 15 October 2020 17:44
To: xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko ; Stefano Stabellini
;
Julien Grall ; Volodymyr Babchuk ;
Andrew Cooper
; George Dunlap ; Ian
Jackson
; Jan Beulich ; Wei Liu ;
Paul Durrant
; Julien Grall
Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
From: Oleksandr Tyshchenko
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
CC: Julien Grall
---
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_tbufioreq_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]
RE: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
> -Original Message-
> From: Oleksandr Tyshchenko
> Sent: 15 October 2020 17:44
> To: xen-devel@lists.xenproject.org
> Cc: Oleksandr Tyshchenko ; Stefano Stabellini
> ;
> Julien Grall ; Volodymyr Babchuk
> ; Andrew Cooper
> ; George Dunlap ; Ian
> Jackson
> ; Jan Beulich ; Wei Liu
> ; Paul Durrant
> ; Julien Grall
> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
>
> From: Oleksandr Tyshchenko
>
> 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
> CC: Julien Grall
>
> ---
> 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_tbufioreq_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 intnr_servers;
> } ioreq_server;
> #endif
> };
> --
> 2.7.4
[PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
From: Oleksandr Tyshchenko
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
CC: Julien Grall
---
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);
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_tbufioreq_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 intnr_servers;
} ioreq_server;
#endif
};
--
2.7.4
