| Message ID | 1602780274-29141-18-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
> -----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
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.
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
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.
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
> -----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
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.
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 --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 };