| Message ID | 1487676368-22356-4-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pl011 emulation support in Xen | expand |
On Tue, Feb 21, 2017 at 04:56:00PM +0530, Bhupinder Thakur wrote: > Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, > there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel > is bound to a xen consumer then event generation is not allowed for that channel. > This check is to disallow a guest from raising an event via this channel. However, Did any code archeology help in idenfitying why this was done this way? You should explain why it was done - what was the use case , and why your change will not change this semantic. > it should allow Xen to send a event via this channel as it is required for sending > vpl011 event to the dom0. > > This change introduces a new function raw_evtchn_send() which sends the event > without this check. The current evtchn_send() calls this function after doing the .. and without taking a lock? Why? > xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus > bypassing the check. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------ > xen/include/xen/event.h | 6 ++++++ > 2 files changed, 42 insertions(+), 13 deletions(-) > > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c > index 638dc5e..4b039f3 100644 > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -27,6 +27,7 @@ > #include <xen/keyhandler.h> > #include <xen/event_fifo.h> > #include <asm/current.h> > +#include <xen/domain_page.h> > > #include <public/xen.h> > #include <public/event_channel.h> > @@ -650,25 +651,21 @@ static long evtchn_close(struct domain *d1, int port1, bool_t guest) > return rc; > } > > -int evtchn_send(struct domain *ld, unsigned int lport) > +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data) > { > struct evtchn *lchn, *rchn; > struct domain *rd; > - int rport, ret = 0; > + int rport, ret=0; Please that code as is. > > - if ( !port_is_valid(ld, lport) ) > - return -EINVAL; > - > - lchn = evtchn_from_port(ld, lport); > - > - spin_lock(&lchn->lock); > - > - /* Guest cannot send via a Xen-attached event channel. */ > - if ( unlikely(consumer_is_xen(lchn)) ) > + if ( !data ) > { > - ret = -EINVAL; > - goto out; > + if ( !port_is_valid(ld, lport) ) > + return -EINVAL; > + lchn = evtchn_from_port(ld, lport); > + spin_lock(&lchn->lock); That won't do. Please keep the format of the old code as much as possible (hint: Those newlines). > } > + else > + lchn = (struct evtchn *)data; > > ret = xsm_evtchn_send(XSM_HOOK, ld, lchn); > if ( ret ) > @@ -696,6 +693,32 @@ int evtchn_send(struct domain *ld, unsigned int lport) > } > > out: > + if ( !data ) > + spin_unlock(&lchn->lock); > + > + return ret; > +} > + > +int evtchn_send(struct domain *ld, unsigned int lport) > +{ > + struct evtchn *lchn; > + int ret; > + > + if ( !port_is_valid(ld, lport) ) > + return -EINVAL; > + > + lchn = evtchn_from_port(ld, lport); > + > + spin_lock(&lchn->lock); > + > + if ( unlikely(consumer_is_xen(lchn)) ) > + { > + printk("evtchn_send failed to send via xen event channel\n"); No. Please do not. > + return -EINVAL; > + } > + > + ret = raw_evtchn_send(ld, lport, lchn); > + > spin_unlock(&lchn->lock); > > return ret; > diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h > index 5008c80..9bd17db 100644 > --- a/xen/include/xen/event.h > +++ b/xen/include/xen/event.h > @@ -45,6 +45,12 @@ void send_guest_pirq(struct domain *, const struct pirq *); > /* Send a notification from a given domain's event-channel port. */ > int evtchn_send(struct domain *d, unsigned int lport); > > +/* > + * This function is same as evntchn_send() except it does not do xen consumer check > + * to allow the events to be sent from xen bound channels. And it also looks to ignore the locking? Could you explain why? > + */ > +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data); > + > /* Bind a local event-channel port to the specified VCPU. */ > long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id); > > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
Hi Bhupinder, My knowledge is limited for this code. So I've just CCed "The REST" maintainers. Please do CC them in the future. On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: > Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, > there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel > is bound to a xen consumer then event generation is not allowed for that channel. > This check is to disallow a guest from raising an event via this channel. However, > it should allow Xen to send a event via this channel as it is required for sending > vpl011 event to the dom0. > > This change introduces a new function raw_evtchn_send() which sends the event > without this check. The current evtchn_send() calls this function after doing the > xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus > bypassing the check. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------ > xen/include/xen/event.h | 6 ++++++ > 2 files changed, 42 insertions(+), 13 deletions(-) > > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c > index 638dc5e..4b039f3 100644 > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -27,6 +27,7 @@ > #include <xen/keyhandler.h> > #include <xen/event_fifo.h> > #include <asm/current.h> > +#include <xen/domain_page.h> Why did you include xen/domain_page.h? > > #include <public/xen.h> > #include <public/event_channel.h> > @@ -650,25 +651,21 @@ static long evtchn_close(struct domain *d1, int port1, bool_t guest) > return rc; > } > > -int evtchn_send(struct domain *ld, unsigned int lport) > +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data) > { > struct evtchn *lchn, *rchn; > struct domain *rd; > - int rport, ret = 0; > + int rport, ret=0; > > - if ( !port_is_valid(ld, lport) ) > - return -EINVAL; > - > - lchn = evtchn_from_port(ld, lport); > - > - spin_lock(&lchn->lock); > - > - /* Guest cannot send via a Xen-attached event channel. */ > - if ( unlikely(consumer_is_xen(lchn)) ) > + if ( !data ) > { > - ret = -EINVAL; > - goto out; > + if ( !port_is_valid(ld, lport) ) > + return -EINVAL; > + lchn = evtchn_from_port(ld, lport); > + spin_lock(&lchn->lock); > } > + else > + lchn = (struct evtchn *)data; > > ret = xsm_evtchn_send(XSM_HOOK, ld, lchn); > if ( ret ) > @@ -696,6 +693,32 @@ int evtchn_send(struct domain *ld, unsigned int lport) > } > > out: > + if ( !data ) > + spin_unlock(&lchn->lock); > + > + return ret; > +} > + > +int evtchn_send(struct domain *ld, unsigned int lport) > +{ > + struct evtchn *lchn; > + int ret; > + > + if ( !port_is_valid(ld, lport) ) > + return -EINVAL; > + > + lchn = evtchn_from_port(ld, lport); > + > + spin_lock(&lchn->lock); > + > + if ( unlikely(consumer_is_xen(lchn)) ) > + { > + printk("evtchn_send failed to send via xen event channel\n"); > + return -EINVAL; > + } > + > + ret = raw_evtchn_send(ld, lport, lchn); > + > spin_unlock(&lchn->lock); > > return ret; > diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h > index 5008c80..9bd17db 100644 > --- a/xen/include/xen/event.h > +++ b/xen/include/xen/event.h > @@ -45,6 +45,12 @@ void send_guest_pirq(struct domain *, const struct pirq *); > /* Send a notification from a given domain's event-channel port. */ > int evtchn_send(struct domain *d, unsigned int lport); > > +/* > + * This function is same as evntchn_send() except it does not do xen consumer check > + * to allow the events to be sent from xen bound channels. > + */ > +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data); > + > /* Bind a local event-channel port to the specified VCPU. */ > long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id); > > Cheers,
>>> On 05.03.17 at 13:39, <julien.grall@arm.com> wrote: > My knowledge is limited for this code. So I've just CCed "The REST" > maintainers. Please do CC them in the future. Indeed. > On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: >> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, >> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel >> is bound to a xen consumer then event generation is not allowed for that channel. >> This check is to disallow a guest from raising an event via this channel. However, >> it should allow Xen to send a event via this channel as it is required for sending >> vpl011 event to the dom0. >> >> This change introduces a new function raw_evtchn_send() which sends the event >> without this check. The current evtchn_send() calls this function after doing the >> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus >> bypassing the check. Why would Xen want to send an event it is itself the consumer of? Surely there are better ways to communicate state internally? The more that you say you want the event sent to Dom0... >> @@ -650,25 +651,21 @@ static long evtchn_close(struct domain *d1, int port1, bool_t guest) >> return rc; >> } >> >> -int evtchn_send(struct domain *ld, unsigned int lport) >> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data) >> { >> struct evtchn *lchn, *rchn; >> struct domain *rd; >> - int rport, ret = 0; >> + int rport, ret=0; There's only whitespace change here, and just the break existing formatting. Please avoid stray changes like this. >> @@ -696,6 +693,32 @@ int evtchn_send(struct domain *ld, unsigned int lport) >> } >> >> out: >> + if ( !data ) >> + spin_unlock(&lchn->lock); >> + >> + return ret; >> +} >> + >> +int evtchn_send(struct domain *ld, unsigned int lport) >> +{ >> + struct evtchn *lchn; >> + int ret; >> + >> + if ( !port_is_valid(ld, lport) ) >> + return -EINVAL; >> + >> + lchn = evtchn_from_port(ld, lport); >> + >> + spin_lock(&lchn->lock); >> + >> + if ( unlikely(consumer_is_xen(lchn)) ) >> + { >> + printk("evtchn_send failed to send via xen event channel\n"); >> + return -EINVAL; As a general remark: Splitting out functionality into a new function should _never_ be accompanied by silent behavioral changes for pre-existing code paths. In the case here there was no printk() before, and I see no justification for one to be added. Jan
Hi Konrad, On 4 March 2017 at 02:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Tue, Feb 21, 2017 at 04:56:00PM +0530, Bhupinder Thakur wrote: >> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, >> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel >> is bound to a xen consumer then event generation is not allowed for that channel. >> This check is to disallow a guest from raising an event via this channel. However, > > Did any code archeology help in idenfitying why this was done this way? > > You should explain why it was done - what was the use case , and why > your change will not change this semantic. I think there was never a use case earlier to generate events from the Xen itself to the guest. The event generation always happened on behalf of a guest via evtchn_send(), which was invoked through a hypercall by the guest. To disallow a guest from sending an event via an event channel which is bound to Xen, there is a check in evtchn_send() to check if it is a xen bound channel and if it is then disallow sending an event via that channel because that channel is meant only for Xen. For pl011 emulation, there is a requirement for Xen to generate the event to dom0 when there is data for dom0 to read in the ring buffer. Since I am using a Xen bound channel so that Xen can receive events from dom0 when there is data for Xen to read, the above mentioned check would not allow the event to be sent to dom0. Since this event is required to be generated from Xen only, it is safe to allow to send the event from a xen bound channel. That is why I introduced a new function raw_evtchn_send() to allow Xen to do that while still keeping that restriction for the guests who use evtchn_send() as it is today. >> it should allow Xen to send a event via this channel as it is required for sending >> vpl011 event to the dom0. >> >> This change introduces a new function raw_evtchn_send() which sends the event >> without this check. The current evtchn_send() calls this function after doing the > > .. and without taking a lock? Why? > The third parameter to raw_evtchn_send() is the event channel structure pointer. When raw_evtchn_send() is called from inside evtchn_send(), it would be passed a pointer to the local event channel structure and lock already taken (which would be case most of the times). But when it is called directly by Xen to send an event to dom0 (as in case of pl011), the third parameter will be passed as NULL. In this case, the lock and a reference to local channel structure would be taken inside raw_evtchn_send(). > >> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus >> bypassing the check. >> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> >> --- >> xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------ >> xen/include/xen/event.h | 6 ++++++ >> 2 files changed, 42 insertions(+), 13 deletions(-) >> >> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c >> index 638dc5e..4b039f3 100644 >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -27,6 +27,7 @@ >> #include <xen/keyhandler.h> >> #include <xen/event_fifo.h> >> #include <asm/current.h> >> +#include <xen/domain_page.h> >> >> #include <public/xen.h> >> #include <public/event_channel.h> >> @@ -650,25 +651,21 @@ static long evtchn_close(struct domain *d1, int port1, bool_t guest) >> return rc; >> } >> >> -int evtchn_send(struct domain *ld, unsigned int lport) >> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data) >> { >> struct evtchn *lchn, *rchn; >> struct domain *rd; >> - int rport, ret = 0; >> + int rport, ret=0; > > Please that code as is. > I will remove these cosmetic changes in the next patch. >> >> - if ( !port_is_valid(ld, lport) ) >> - return -EINVAL; >> - >> - lchn = evtchn_from_port(ld, lport); >> - >> - spin_lock(&lchn->lock); >> - >> - /* Guest cannot send via a Xen-attached event channel. */ >> - if ( unlikely(consumer_is_xen(lchn)) ) >> + if ( !data ) >> { >> - ret = -EINVAL; >> - goto out; >> + if ( !port_is_valid(ld, lport) ) >> + return -EINVAL; >> + lchn = evtchn_from_port(ld, lport); >> + spin_lock(&lchn->lock); > > That won't do. Please keep the format of the old code as much > as possible (hint: Those newlines). > I will remove these cosmetic changes in the next patch. >> } >> + else >> + lchn = (struct evtchn *)data; >> >> ret = xsm_evtchn_send(XSM_HOOK, ld, lchn); >> if ( ret ) >> @@ -696,6 +693,32 @@ int evtchn_send(struct domain *ld, unsigned int lport) >> } >> >> out: >> + if ( !data ) >> + spin_unlock(&lchn->lock); >> + >> + return ret; >> +} >> + >> +int evtchn_send(struct domain *ld, unsigned int lport) >> +{ >> + struct evtchn *lchn; >> + int ret; >> + >> + if ( !port_is_valid(ld, lport) ) >> + return -EINVAL; >> + >> + lchn = evtchn_from_port(ld, lport); >> + >> + spin_lock(&lchn->lock); >> + >> + if ( unlikely(consumer_is_xen(lchn)) ) >> + { >> + printk("evtchn_send failed to send via xen event channel\n"); > > No. Please do not. I will remove the failure print. >> + return -EINVAL; >> + } >> + >> + ret = raw_evtchn_send(ld, lport, lchn); >> + >> spin_unlock(&lchn->lock); >> >> return ret; >> diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h >> index 5008c80..9bd17db 100644 >> --- a/xen/include/xen/event.h >> +++ b/xen/include/xen/event.h >> @@ -45,6 +45,12 @@ void send_guest_pirq(struct domain *, const struct pirq *); >> /* Send a notification from a given domain's event-channel port. */ >> int evtchn_send(struct domain *d, unsigned int lport); >> >> +/* >> + * This function is same as evntchn_send() except it does not do xen consumer check >> + * to allow the events to be sent from xen bound channels. > > And it also looks to ignore the locking? Could you explain why? > It will take the lock when invoked directly (explained above). >> + */ >> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data); >> + >> /* Bind a local event-channel port to the specified VCPU. */ >> long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id); >> >> -- >> 2.7.4 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> https://lists.xen.org/xen-devel
>>> On 06.03.17 at 11:16, <bhupinder.thakur@linaro.org> wrote: > On 4 March 2017 at 02:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> On Tue, Feb 21, 2017 at 04:56:00PM +0530, Bhupinder Thakur wrote: >>> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, >>> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel >>> is bound to a xen consumer then event generation is not allowed for that channel. >>> This check is to disallow a guest from raising an event via this channel. However, >> >> Did any code archeology help in idenfitying why this was done this way? >> >> You should explain why it was done - what was the use case , and why >> your change will not change this semantic. > > I think there was never a use case earlier to generate events from the > Xen itself to the guest. The event generation always happened on > behalf of a guest via evtchn_send(), which was invoked through a > hypercall by the guest. This is not true: All interrupts being converted to events go this route, without any hypercall involved. Same for signaling qemu to do emulation of an emulated device (memory of port) access. > To disallow a guest from sending an event via > an event channel which is bound to Xen, there is a check in > evtchn_send() to check if it is a xen bound channel and if it is then > disallow sending an event via that channel because that channel is > meant only for Xen. As said in reply to your patch - I think you got directions mixed up here: You talk about Xen sending an event to a guest, but at the same time you fiddle with code preventing guests from sending events to Xen. > For pl011 emulation, there is a requirement for Xen to generate the > event to dom0 when there is data for dom0 to read in the ring buffer. This is not different from hardware signaling the need for service via an interrupt, which - always for PV guests, sometimes for HVM ones - is being converted to an event. > Since I am using a Xen bound channel so that Xen can receive events > from dom0 when there is data for Xen to read, the above mentioned > check would not allow the event to be sent to dom0. Now here you come to talk about the direction which indeed is not allowed. But breaking out part of the function won't help you, as the new (raw) function then mustn't be called as a result of some guest's request (hypercall or whatever), which includes Dom0. > Since this event is required to be generated from Xen only, it is safe > to allow to send the event from a xen bound channel. That is why I > introduced a new function raw_evtchn_send() to allow Xen to do that > while still keeping that restriction for the guests who use > evtchn_send() as it is today. And now it becomes confusing again: Yet another time you talk about Xen sending the event. Please can you cleanly separate both directions of event flow, and talk only about the relevant one in the respective contexts? Jan
Hi Jan, On 6 March 2017 at 13:45, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 05.03.17 at 13:39, <julien.grall@arm.com> wrote: >> My knowledge is limited for this code. So I've just CCed "The REST" >> maintainers. Please do CC them in the future. > > Indeed. > I will take care of this in the future. >> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: >>> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, >>> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel >>> is bound to a xen consumer then event generation is not allowed for that channel. >>> This check is to disallow a guest from raising an event via this channel. However, >>> it should allow Xen to send a event via this channel as it is required for sending >>> vpl011 event to the dom0. >>> >>> This change introduces a new function raw_evtchn_send() which sends the event >>> without this check. The current evtchn_send() calls this function after doing the >>> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus >>> bypassing the check. > > Why would Xen want to send an event it is itself the consumer of? > Surely there are better ways to communicate state internally? The > more that you say you want the event sent to Dom0... > As a consumer, Xen receives event from dom0. It also needs to send events to dom0 to indicate that there is data in the ring buffer for dom0 to read. I am using a xen bound event channel for sending/receiving events to/from dom0. I added a new function raw_evtchn_send() to allow Xen to send events to dom0 without doing the is_xen_consumer check. Note that this check is still there in evtchn_send() to disallow guests to raise events on the xen bound channel. >>> @@ -650,25 +651,21 @@ static long evtchn_close(struct domain *d1, int port1, bool_t guest) >>> return rc; >>> } >>> >>> -int evtchn_send(struct domain *ld, unsigned int lport) >>> +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data) >>> { >>> struct evtchn *lchn, *rchn; >>> struct domain *rd; >>> - int rport, ret = 0; >>> + int rport, ret=0; > > There's only whitespace change here, and just the break existing > formatting. Please avoid stray changes like this. > I will fix this in the next version. >>> @@ -696,6 +693,32 @@ int evtchn_send(struct domain *ld, unsigned int lport) >>> } >>> >>> out: >>> + if ( !data ) >>> + spin_unlock(&lchn->lock); >>> + >>> + return ret; >>> +} >>> + >>> +int evtchn_send(struct domain *ld, unsigned int lport) >>> +{ >>> + struct evtchn *lchn; >>> + int ret; >>> + >>> + if ( !port_is_valid(ld, lport) ) >>> + return -EINVAL; >>> + >>> + lchn = evtchn_from_port(ld, lport); >>> + >>> + spin_lock(&lchn->lock); >>> + >>> + if ( unlikely(consumer_is_xen(lchn)) ) >>> + { >>> + printk("evtchn_send failed to send via xen event channel\n"); >>> + return -EINVAL; > > As a general remark: Splitting out functionality into a new function > should _never_ be accompanied by silent behavioral changes for > pre-existing code paths. In the case here there was no printk() > before, and I see no justification for one to be added. > Ok. I will remove the printk. > Jan >
>>> On 06.03.17 at 11:44, <bhupinder.thakur@linaro.org> wrote: > On 6 March 2017 at 13:45, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 05.03.17 at 13:39, <julien.grall@arm.com> wrote: >>> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: >>>> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, >>>> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel >>>> is bound to a xen consumer then event generation is not allowed for that channel. >>>> This check is to disallow a guest from raising an event via this channel. However, >>>> it should allow Xen to send a event via this channel as it is required for sending >>>> vpl011 event to the dom0. >>>> >>>> This change introduces a new function raw_evtchn_send() which sends the event >>>> without this check. The current evtchn_send() calls this function after doing the >>>> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus >>>> bypassing the check. >> >> Why would Xen want to send an event it is itself the consumer of? >> Surely there are better ways to communicate state internally? The >> more that you say you want the event sent to Dom0... >> > As a consumer, Xen receives event from dom0. It also needs to send > events to dom0 to indicate that there is data in the ring buffer for > dom0 to read. I am using a xen bound event channel for > sending/receiving events to/from dom0. I added a new function > raw_evtchn_send() to allow Xen to send events to dom0 without doing > the is_xen_consumer check. Note that this check is still there in > evtchn_send() to disallow guests to raise events on the xen bound > channel. I can see why Xen needs to send events; I can't see why Dom0 couldn't simply make a hypercall instead of sending an event if it needs to signal something. Jan
Hi, On 6 March 2017 at 16:24, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 06.03.17 at 11:44, <bhupinder.thakur@linaro.org> wrote: >> On 6 March 2017 at 13:45, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 05.03.17 at 13:39, <julien.grall@arm.com> wrote: >>>> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: >>>>> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, >>>>> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel >>>>> is bound to a xen consumer then event generation is not allowed for that channel. >>>>> This check is to disallow a guest from raising an event via this channel. However, >>>>> it should allow Xen to send a event via this channel as it is required for sending >>>>> vpl011 event to the dom0. >>>>> >>>>> This change introduces a new function raw_evtchn_send() which sends the event >>>>> without this check. The current evtchn_send() calls this function after doing the >>>>> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus >>>>> bypassing the check. >>> >>> Why would Xen want to send an event it is itself the consumer of? >>> Surely there are better ways to communicate state internally? The >>> more that you say you want the event sent to Dom0... >>> >> As a consumer, Xen receives event from dom0. It also needs to send >> events to dom0 to indicate that there is data in the ring buffer for >> dom0 to read. I am using a xen bound event channel for >> sending/receiving events to/from dom0. I added a new function >> raw_evtchn_send() to allow Xen to send events to dom0 without doing >> the is_xen_consumer check. Note that this check is still there in >> evtchn_send() to disallow guests to raise events on the xen bound >> channel. > > I can see why Xen needs to send events; I can't see why Dom0 couldn't > simply make a hypercall instead of sending an event if it needs to signal > something. > We decided to reuse the same PV console interface for pl011 as used for PV console in xenconsole running on dom0, which is events/ring buffer based. From xenconsole point of view, there is no difference in terms of handling a pl011 console and a PV console. > Jan >
Hi, Now I am using notify_via_xen_event_channel() to send the vpl011 events to dom0. So no changes are required in evtchn_send() and this patch can be dropped. Regards, Bhupinder On 6 March 2017 at 16:42, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote: > Hi, > > On 6 March 2017 at 16:24, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 06.03.17 at 11:44, <bhupinder.thakur@linaro.org> wrote: >>> On 6 March 2017 at 13:45, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 05.03.17 at 13:39, <julien.grall@arm.com> wrote: >>>>> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: >>>>>> Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, >>>>>> there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel >>>>>> is bound to a xen consumer then event generation is not allowed for that channel. >>>>>> This check is to disallow a guest from raising an event via this channel. However, >>>>>> it should allow Xen to send a event via this channel as it is required for sending >>>>>> vpl011 event to the dom0. >>>>>> >>>>>> This change introduces a new function raw_evtchn_send() which sends the event >>>>>> without this check. The current evtchn_send() calls this function after doing the >>>>>> xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus >>>>>> bypassing the check. >>>> >>>> Why would Xen want to send an event it is itself the consumer of? >>>> Surely there are better ways to communicate state internally? The >>>> more that you say you want the event sent to Dom0... >>>> >>> As a consumer, Xen receives event from dom0. It also needs to send >>> events to dom0 to indicate that there is data in the ring buffer for >>> dom0 to read. I am using a xen bound event channel for >>> sending/receiving events to/from dom0. I added a new function >>> raw_evtchn_send() to allow Xen to send events to dom0 without doing >>> the is_xen_consumer check. Note that this check is still there in >>> evtchn_send() to disallow guests to raise events on the xen bound >>> channel. >> >> I can see why Xen needs to send events; I can't see why Dom0 couldn't >> simply make a hypercall instead of sending an event if it needs to signal >> something. >> > We decided to reuse the same PV console interface for pl011 as used > for PV console in xenconsole running on dom0, which is events/ring > buffer based. From xenconsole point of view, there is no difference in > terms of handling a pl011 console and a PV console. > >> Jan >>
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 638dc5e..4b039f3 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -27,6 +27,7 @@ #include <xen/keyhandler.h> #include <xen/event_fifo.h> #include <asm/current.h> +#include <xen/domain_page.h> #include <public/xen.h> #include <public/event_channel.h> @@ -650,25 +651,21 @@ static long evtchn_close(struct domain *d1, int port1, bool_t guest) return rc; } -int evtchn_send(struct domain *ld, unsigned int lport) +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data) { struct evtchn *lchn, *rchn; struct domain *rd; - int rport, ret = 0; + int rport, ret=0; - if ( !port_is_valid(ld, lport) ) - return -EINVAL; - - lchn = evtchn_from_port(ld, lport); - - spin_lock(&lchn->lock); - - /* Guest cannot send via a Xen-attached event channel. */ - if ( unlikely(consumer_is_xen(lchn)) ) + if ( !data ) { - ret = -EINVAL; - goto out; + if ( !port_is_valid(ld, lport) ) + return -EINVAL; + lchn = evtchn_from_port(ld, lport); + spin_lock(&lchn->lock); } + else + lchn = (struct evtchn *)data; ret = xsm_evtchn_send(XSM_HOOK, ld, lchn); if ( ret ) @@ -696,6 +693,32 @@ int evtchn_send(struct domain *ld, unsigned int lport) } out: + if ( !data ) + spin_unlock(&lchn->lock); + + return ret; +} + +int evtchn_send(struct domain *ld, unsigned int lport) +{ + struct evtchn *lchn; + int ret; + + if ( !port_is_valid(ld, lport) ) + return -EINVAL; + + lchn = evtchn_from_port(ld, lport); + + spin_lock(&lchn->lock); + + if ( unlikely(consumer_is_xen(lchn)) ) + { + printk("evtchn_send failed to send via xen event channel\n"); + return -EINVAL; + } + + ret = raw_evtchn_send(ld, lport, lchn); + spin_unlock(&lchn->lock); return ret; diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index 5008c80..9bd17db 100644 --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -45,6 +45,12 @@ void send_guest_pirq(struct domain *, const struct pirq *); /* Send a notification from a given domain's event-channel port. */ int evtchn_send(struct domain *d, unsigned int lport); +/* + * This function is same as evntchn_send() except it does not do xen consumer check + * to allow the events to be sent from xen bound channels. + */ +int raw_evtchn_send(struct domain *ld, unsigned int lport, void *data); + /* Bind a local event-channel port to the specified VCPU. */ long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id);
Breakup evtchn_send() to allow sending events for a Xen bound channel. Currently, there is a check in evtchn_send() i.e. is_consumer_xen() that if the event channel is bound to a xen consumer then event generation is not allowed for that channel. This check is to disallow a guest from raising an event via this channel. However, it should allow Xen to send a event via this channel as it is required for sending vpl011 event to the dom0. This change introduces a new function raw_evtchn_send() which sends the event without this check. The current evtchn_send() calls this function after doing the xen consumer check. Xen uses the raw_evtchm_send() version to send the event thus bypassing the check. Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- xen/common/event_channel.c | 49 ++++++++++++++++++++++++++++++++++------------ xen/include/xen/event.h | 6 ++++++ 2 files changed, 42 insertions(+), 13 deletions(-)