| Message ID | 91656930b5bfd49e40ff5a9d060d7643e6311f4f.1655903088.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/evtchn: implement static event channel signaling | expand |
Hi, On 22/06/2022 15:38, Rahul Singh wrote: > Guest can request the Xen to close the event channels. Ignore the > request from guest to close the static channels as static event channels > should not be closed. Why do you want to prevent the guest to close static ports? The problem I can see is... [...] > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c > index 84f0055a5a..cedc98ccaf 100644 > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn) > * If port is zero get the next free port and allocate. If port is non-zero > * allocate the specified port. > */ > -int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) > +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port, > + bool is_static) > { > struct evtchn *chn; > struct domain *d; > @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) > evtchn_write_lock(chn); > > chn->state = ECS_UNBOUND; > + chn->is_static = is_static; > if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF ) > chn->u.unbound.remote_domid = current->domain->domain_id; > evtchn_port_init(d, chn); > @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn) > * allocate the specified lport. > */ > int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, > - evtchn_port_t lport) > + evtchn_port_t lport, bool is_static) > { > struct evtchn *lchn, *rchn; > struct domain *rd; > @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, > lchn->u.interdomain.remote_dom = rd; > lchn->u.interdomain.remote_port = rport; > lchn->state = ECS_INTERDOMAIN; > + lchn->is_static = is_static; > evtchn_port_init(ld, lchn); > > rchn->u.interdomain.remote_dom = ld; > @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest) > rc = -EINVAL; > goto out; > } > + /* Guest cannot close a static event channel. */ > + if ( chn1->is_static && guest ) > + goto out; ... at least the interdomain structure store pointer to the domain. I am a bit concerned that we would end up to leave dangling pointers (such as chn->u.interdomain.remote_domain) as evtchn_close() is also used while destroying the domain. Also, AFAICT Xen will return 0 (i.e. success) to the caller. I think this is a mistake because we didn't close the port as requested. Cheers,
Hi Julien, > On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 22/06/2022 15:38, Rahul Singh wrote: >> Guest can request the Xen to close the event channels. Ignore the >> request from guest to close the static channels as static event channels >> should not be closed. > > Why do you want to prevent the guest to close static ports? The problem I can see is... As a static event channel should be available during the lifetime of the guest we want to prevent the guest to close the static ports. I tested this series to send/receive event notification from the Linux user-space application via "/dev/xen/evtchn” interface and ioctl ( IOCTL_EVTCHN_*) calls. When we close the "/dev/xen/evtchn” interface Linux event channel driver will try to close the static event channel also, that why we need this patch to avoid guests to close the event channel as we don’t want to close the static event channel. > > [...] > >> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c >> index 84f0055a5a..cedc98ccaf 100644 >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn) >> * If port is zero get the next free port and allocate. If port is non-zero >> * allocate the specified port. >> */ >> -int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) >> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port, >> + bool is_static) >> { >> struct evtchn *chn; >> struct domain *d; >> @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) >> evtchn_write_lock(chn); >> chn->state = ECS_UNBOUND; >> + chn->is_static = is_static; >> if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF ) >> chn->u.unbound.remote_domid = current->domain->domain_id; >> evtchn_port_init(d, chn); >> @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn) >> * allocate the specified lport. >> */ >> int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, >> - evtchn_port_t lport) >> + evtchn_port_t lport, bool is_static) >> { >> struct evtchn *lchn, *rchn; >> struct domain *rd; >> @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, >> lchn->u.interdomain.remote_dom = rd; >> lchn->u.interdomain.remote_port = rport; >> lchn->state = ECS_INTERDOMAIN; >> + lchn->is_static = is_static; >> evtchn_port_init(ld, lchn); >> rchn->u.interdomain.remote_dom = ld; >> @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest) >> rc = -EINVAL; >> goto out; >> } >> + /* Guest cannot close a static event channel. */ >> + if ( chn1->is_static && guest ) >> + goto out; > > ... at least the interdomain structure store pointer to the domain. I am a bit concerned that we would end up to leave dangling pointers (such as chn->u.interdomain.remote_domain) as evtchn_close() is also used while destroying the domain. Let me have a look again if we have to do the cleanup when we destroy the guest and close the static event channel. > > Also, AFAICT Xen will return 0 (i.e. success) to the caller. I think this is a mistake because we didn't close the port as requested. If we return non-zero to guest (in particular if linux guest), Linux will report the BUG(). Therefore I decided to return 0. if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0) BUG(); Regards, Rahul
On 23/06/2022 16:10, Rahul Singh wrote: > Hi Julien, > > >> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 22/06/2022 15:38, Rahul Singh wrote: >>> Guest can request the Xen to close the event channels. Ignore the >>> request from guest to close the static channels as static event channels >>> should not be closed. >> >> Why do you want to prevent the guest to close static ports? The problem I can see is... > > As a static event channel should be available during the lifetime of the guest we want to prevent > the guest to close the static ports. I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen. > > I tested this series to send/receive event notification from the Linux user-space application via "/dev/xen/evtchn” interface and > ioctl ( IOCTL_EVTCHN_*) calls. When we close the "/dev/xen/evtchn” interface Linux event channel > driver will try to close the static event channel also, that why we need this patch to avoid guests to close > the event channel as we don’t want to close the static event channel. To me, this reads as Linux should be modified in order to avoid closing static event channel. In fact... >> >> [...] >> >>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c >>> index 84f0055a5a..cedc98ccaf 100644 >>> --- a/xen/common/event_channel.c >>> +++ b/xen/common/event_channel.c >>> @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn) >>> * If port is zero get the next free port and allocate. If port is non-zero >>> * allocate the specified port. >>> */ >>> -int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) >>> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port, >>> + bool is_static) >>> { >>> struct evtchn *chn; >>> struct domain *d; >>> @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) >>> evtchn_write_lock(chn); >>> chn->state = ECS_UNBOUND; >>> + chn->is_static = is_static; >>> if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF ) >>> chn->u.unbound.remote_domid = current->domain->domain_id; >>> evtchn_port_init(d, chn); >>> @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn) >>> * allocate the specified lport. >>> */ >>> int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, >>> - evtchn_port_t lport) >>> + evtchn_port_t lport, bool is_static) >>> { >>> struct evtchn *lchn, *rchn; >>> struct domain *rd; >>> @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, >>> lchn->u.interdomain.remote_dom = rd; >>> lchn->u.interdomain.remote_port = rport; >>> lchn->state = ECS_INTERDOMAIN; >>> + lchn->is_static = is_static; >>> evtchn_port_init(ld, lchn); >>> rchn->u.interdomain.remote_dom = ld; >>> @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest) >>> rc = -EINVAL; >>> goto out; >>> } >>> + /* Guest cannot close a static event channel. */ >>> + if ( chn1->is_static && guest ) >>> + goto out; >> >> ... at least the interdomain structure store pointer to the domain. I am a bit concerned that we would end up to leave dangling pointers (such as chn->u.interdomain.remote_domain) as evtchn_close() is also used while destroying the domain. > > Let me have a look again if we have to do the cleanup when we destroy the guest and close the static event channel. >> >> Also, AFAICT Xen will return 0 (i.e. success) to the caller. I think this is a mistake because we didn't close the port as requested. > > If we return non-zero to guest (in particular if linux guest), Linux will report the BUG(). Therefore I decided to return 0. ... this shows that we are papering over a bigger problem: Linux is not ready for static event channels. > > if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0) > BUG(); The BUG() in Linux is definitely not a reason to lie and claim the port was closed. If you tell that to an OS, it may validly think that it know need to call bind interdomain in order to "re-open" the port. So your Linux will already need some information to know that the port is "static". At which point, you can modify Linux to also prevent the port to be closed. Cheers,
On 23.06.2022 17:30, Julien Grall wrote: > On 23/06/2022 16:10, Rahul Singh wrote: >>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: >>> On 22/06/2022 15:38, Rahul Singh wrote: >>>> Guest can request the Xen to close the event channels. Ignore the >>>> request from guest to close the static channels as static event channels >>>> should not be closed. >>> >>> Why do you want to prevent the guest to close static ports? The problem I can see is... >> >> As a static event channel should be available during the lifetime of the guest we want to prevent >> the guest to close the static ports. > I don't think it is Xen job to prevent a guest to close a static port. > If the guest decide to do it, then it will just break itself and not Xen. +1, fwiw. Jan
Hi Julien > On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote: > > > > On 23/06/2022 16:10, Rahul Singh wrote: >> Hi Julien, >>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: >>> >>> Hi, >>> >>> On 22/06/2022 15:38, Rahul Singh wrote: >>>> Guest can request the Xen to close the event channels. Ignore the >>>> request from guest to close the static channels as static event channels >>>> should not be closed. >>> >>> Why do you want to prevent the guest to close static ports? The problem I can see is... >> As a static event channel should be available during the lifetime of the guest we want to prevent >> the guest to close the static ports. > I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen. It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel. Xen has nothing to do for close the static event channel and just return 0. Regards, Rahul
On 28/06/2022 14:53, Rahul Singh wrote: > Hi Julien Hi Rahul, >> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 23/06/2022 16:10, Rahul Singh wrote: >>> Hi Julien, >>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi, >>>> >>>> On 22/06/2022 15:38, Rahul Singh wrote: >>>>> Guest can request the Xen to close the event channels. Ignore the >>>>> request from guest to close the static channels as static event channels >>>>> should not be closed. >>>> >>>> Why do you want to prevent the guest to close static ports? The problem I can see is... >>> As a static event channel should be available during the lifetime of the guest we want to prevent >>> the guest to close the static ports. >> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen. > > It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel. As I wrote before, the OS will need to know that the port is statically allocated when initializing the port (we don't want to call the hypercall to bind the event channel). By extend, the OS should be able to know that when closing it and skip the hypercall. > Xen has nothing to do for close the static event channel and just return 0. Xen would not need to be modified if the OS was doing the right (i.e. no calling close). So it is still unclear why papering over the issue in Xen is the best solution. Cheers,
Hi Julien, > On 28 Jun 2022, at 15:26, Julien Grall <julien@xen.org> wrote: > > > > On 28/06/2022 14:53, Rahul Singh wrote: >> Hi Julien > > Hi Rahul, > >>> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote: >>> >>> >>> >>> On 23/06/2022 16:10, Rahul Singh wrote: >>>> Hi Julien, >>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On 22/06/2022 15:38, Rahul Singh wrote: >>>>>> Guest can request the Xen to close the event channels. Ignore the >>>>>> request from guest to close the static channels as static event channels >>>>>> should not be closed. >>>>> >>>>> Why do you want to prevent the guest to close static ports? The problem I can see is... >>>> As a static event channel should be available during the lifetime of the guest we want to prevent >>>> the guest to close the static ports. >>> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen. >> It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel. > As I wrote before, the OS will need to know that the port is statically allocated when initializing the port (we don't want to call the hypercall to bind the event channel). By extend, the OS should be able to know that when closing it and skip the hypercall. > >> Xen has nothing to do for close the static event channel and just return 0. > > Xen would not need to be modified if the OS was doing the right (i.e. no calling close). > > So it is still unclear why papering over the issue in Xen is the best solution. It is not that a static event channel cannot be closed, it is just that during a close there is nothing to do for Xen as the event channel is static and hence is never removed so none of the operations to be done for a non static one are needed (maybe some day some will be, who knows). Why requiring the OS to have the knowledge of the fact that an event channel is static or not and introduce some complexity on guest code if we can prevent it ? Doing so would need to have a specific binding in device tree (not to mention the issue on ACPI), a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature. Cheers Bertrand
On 28/06/2022 15:52, Bertrand Marquis wrote: > Hi Julien, > >> On 28 Jun 2022, at 15:26, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 28/06/2022 14:53, Rahul Singh wrote: >>> Hi Julien >> >> Hi Rahul, >> >>>> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote: >>>> >>>> >>>> >>>> On 23/06/2022 16:10, Rahul Singh wrote: >>>>> Hi Julien, >>>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 22/06/2022 15:38, Rahul Singh wrote: >>>>>>> Guest can request the Xen to close the event channels. Ignore the >>>>>>> request from guest to close the static channels as static event channels >>>>>>> should not be closed. >>>>>> >>>>>> Why do you want to prevent the guest to close static ports? The problem I can see is... >>>>> As a static event channel should be available during the lifetime of the guest we want to prevent >>>>> the guest to close the static ports. >>>> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen. >>> It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel. >> As I wrote before, the OS will need to know that the port is statically allocated when initializing the port (we don't want to call the hypercall to bind the event channel). By extend, the OS should be able to know that when closing it and skip the hypercall. >> >>> Xen has nothing to do for close the static event channel and just return 0. >> >> Xen would not need to be modified if the OS was doing the right (i.e. no calling close). >> >> So it is still unclear why papering over the issue in Xen is the best solution. > > It is not that a static event channel cannot be closed, it is just that during a close there is nothing to do for Xen as the event channel is static and hence is never removed so none of the operations to be done for a non static one are needed (maybe some day some will be, who knows). I feel there are some disagreement on the meaning of "close" here. In the context of event channel, "close" means that the port is marked as ECS_FREE. So I think this is wrong to say that there is nothing to do for "close" because after this operation the port will still be "open" (the port state will be ECS_INTERDOMAIN). In fact, to me, a "static" port is the same as if the event channel was allocated from the toolstack (for instance this is the case for Xenstored). In such case, we are still allowing the guest to close the port and then re-opening. So I don't really see why we should diverge here. > > Why requiring the OS to have the knowledge of the fact that an event channel is static or not and introduce some complexity on guest code if we can prevent it ? I am confused. Your OS already need to know that this is a static port (so it doesn't call the hypercall to "open" the port). So why is it a non-issue for "opening" but one for "closing" ? > > Doing so would need to have a specific binding in device tree (not to mention the issue on ACPI), You already need to create a Device-Tree binding to expose the static event-channel. So why is this a new problem? Likewise for ACPI, you already have this issue with your current proposal. > a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature. I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it". Cheers,
On 28.06.2022 17:18, Julien Grall wrote: > In fact, to me, a "static" port is the same as if the event channel was > allocated from the toolstack (for instance this is the case for > Xenstored). In such case, we are still allowing the guest to close the > port and then re-opening. So I don't really see why we should diverge here. Fwiw, I agree with Julien's view here. Jan
Hi Julien, > On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote: > > > > On 28/06/2022 15:52, Bertrand Marquis wrote: >> Hi Julien, >>> On 28 Jun 2022, at 15:26, Julien Grall <julien@xen.org> wrote: >>> >>> >>> >>> On 28/06/2022 14:53, Rahul Singh wrote: >>>> Hi Julien >>> >>> Hi Rahul, >>> >>>>> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> >>>>> >>>>> On 23/06/2022 16:10, Rahul Singh wrote: >>>>>> Hi Julien, >>>>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On 22/06/2022 15:38, Rahul Singh wrote: >>>>>>>> Guest can request the Xen to close the event channels. Ignore the >>>>>>>> request from guest to close the static channels as static event channels >>>>>>>> should not be closed. >>>>>>> >>>>>>> Why do you want to prevent the guest to close static ports? The problem I can see is... >>>>>> As a static event channel should be available during the lifetime of the guest we want to prevent >>>>>> the guest to close the static ports. >>>>> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen. >>>> It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel. >>> As I wrote before, the OS will need to know that the port is statically allocated when initializing the port (we don't want to call the hypercall to bind the event channel). By extend, the OS should be able to know that when closing it and skip the hypercall. >>> >>>> Xen has nothing to do for close the static event channel and just return 0. >>> >>> Xen would not need to be modified if the OS was doing the right (i.e. no calling close). >>> >>> So it is still unclear why papering over the issue in Xen is the best solution. >> It is not that a static event channel cannot be closed, it is just that during a close there is nothing to do for Xen as the event channel is static and hence is never removed so none of the operations to be done for a non static one are needed (maybe some day some will be, who knows). > > I feel there are some disagreement on the meaning of "close" here. In the context of event channel, "close" means that the port is marked as ECS_FREE. > > So I think this is wrong to say that there is nothing to do for "close" because after this operation the port will still be "open" (the port state will be ECS_INTERDOMAIN). > > In fact, to me, a "static" port is the same as if the event channel was allocated from the toolstack (for instance this is the case for Xenstored). In such case, we are still allowing the guest to close the port and then re-opening. So I don't really see why we should diverge here. > >> Why requiring the OS to have the knowledge of the fact that an event channel is static or not and introduce some complexity on guest code if we can prevent it ? > > I am confused. Your OS already need to know that this is a static port (so it doesn't call the hypercall to "open" the port). So why is it a non-issue for "opening" but one for "closing" ? > >> Doing so would need to have a specific binding in device tree (not to mention the issue on ACPI), > > You already need to create a Device-Tree binding to expose the static event-channel. So why is this a new problem? > > Likewise for ACPI, you already have this issue with your current proposal. > >> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature. > > I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it". I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the event channel state to ECS_STATIC when Xen allocate and create the static event channels. From guest OS we can check if the event channel is static (via EVTCHNOP_status() hypercall ), if the event channel is static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed. diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index 46d9295d9a6e..c5ca29b8ed70 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -815,8 +815,17 @@ static void xen_free_irq(unsigned irq) static void xen_evtchn_close(evtchn_port_t port) { + struct evtchn_status status; struct evtchn_close close; + status.dom = DOMID_SELF; + status.port = port; + if (HYPERVISOR_event_channel_op(EVTCHNOP_status, &status) != 0) + BUG(); + + if (status.status == EVTCHNSTAT_static) + return; + close.port = port; if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0) BUG(); Regards, Rahul > > Cheers, > > -- > Julien Grall
On 05/07/2022 14:28, Rahul Singh wrote: > Hi Julien, Hi Rahul, >> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote: >>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature. >> >> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it". > > I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the > event channel state to ECS_STATIC when Xen allocate and create the static event channels. From what you wrote, ECS_STATIC is just an interdomain behind but where you want Xen to prevent closing the port. From Xen PoV, it is still not clear why this is a problem to let Linux closing such port. From the guest PoV, there are other way to pass this information (see below). > > From guest OS we can check if the event channel is static (via EVTCHNOP_status() hypercall ), if the event channel is > static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed. Why do you need this? You already need a binding indicating which ports will be pre-allocated. So you could update your binding to pass a flag telling Linux "don't close it". I have already proposed that before and I haven't seen any explanation why this is not a viable solution. Cheers,
On 22.06.2022 16:38, Rahul Singh wrote: > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -119,6 +119,7 @@ struct evtchn > unsigned char priority; /* FIFO event channels only. */ > unsigned short notify_vcpu_id; /* VCPU for local delivery notification */ > uint32_t fifo_lastq; /* Data for identifying last queue. */ > + bool is_static; /* Static event channels. */ > > #ifdef CONFIG_XSM > union { _If_ this is the behavior we want in the first place (which I'm unconvinced of, seeing your discussion with Julien), then please be conservative with growing the structure. There are available (padding) bits, so you should first try to use any of those. If that's impossible (or undesirable), please at least briefly say why in the description. Jan
On 22.06.2022 16:38, Rahul Singh wrote: > --- a/xen/include/xen/event.h > +++ b/xen/include/xen/event.h > @@ -73,12 +73,12 @@ int evtchn_allocate_port(struct domain *d, unsigned int port); > > /* Allocate a new event channel */ > int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, > - evtchn_port_t port); > + evtchn_port_t port, bool is_static); > > /* Bind an event channel port to interdomain */ > int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, > struct domain *ld, > - evtchn_port_t port); > + evtchn_port_t port, bool is_static); Didn't even pay attention to this the first time through: You're again touching functions you did alter already in earlier patches, and with them their pre-existing call sites. This is not only unnecessary code churn but also makes it harder to follow where a change came from when (perhaps much later) using "git blame" or alike. Please bring these functions into their intended shape in a single step (each). Jan
On 05.07.2022 17:26, Jan Beulich wrote: > On 22.06.2022 16:38, Rahul Singh wrote: >> --- a/xen/include/xen/event.h >> +++ b/xen/include/xen/event.h >> @@ -73,12 +73,12 @@ int evtchn_allocate_port(struct domain *d, unsigned int port); >> >> /* Allocate a new event channel */ >> int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, >> - evtchn_port_t port); >> + evtchn_port_t port, bool is_static); >> >> /* Bind an event channel port to interdomain */ >> int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, >> struct domain *ld, >> - evtchn_port_t port); >> + evtchn_port_t port, bool is_static); > > Didn't even pay attention to this the first time through: You're > again touching functions you did alter already in earlier patches, > and with them their pre-existing call sites. This is not only > unnecessary code churn but also makes it harder to follow where a > change came from when (perhaps much later) using "git blame" or > alike. Please bring these functions into their intended shape in > a single step (each). One more thing: Especially "bind" now has quite a few parameters for which without dom0less (i.e. particularly on x86) only a single value would ever be passed. Without LTO the compiler could still deal with this if the function remained static in all non-dom0less cases. Please consider whether you want to do so, or whether you want to find another solution to address this concern. Jan
Hi Julien, > On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote: > > > > On 05/07/2022 14:28, Rahul Singh wrote: >> Hi Julien, > > Hi Rahul, > >>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote: >>>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature. >>> >>> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it". >> I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the >> event channel state to ECS_STATIC when Xen allocate and create the static event channels. > > From what you wrote, ECS_STATIC is just an interdomain behind but where you want Xen to prevent closing the port. > > From Xen PoV, it is still not clear why this is a problem to let Linux closing such port. From the guest PoV, there are other way to pass this information (see below). If Linux closes the port, the static event channel created by Xen associated with such port will not be available to use afterward. When I started implemented the static event channel series, I thought the static event channel has to be available for use during the lifetime of the guest. This patch avoids closing the port if the Linux user-space application wants to use the event channel again. This patch is fixing the problem for Linux OS, and I agree with you that we should not modify the Xen to fix the Linux problem. Therefore, If the guest decided to close the static event channel, Xen will close the port. Event Chanel associated with the port will not be available for use after that.I will discard this patch in the next series. > >> From guest OS we can check if the event channel is static (via EVTCHNOP_status() hypercall ), if the event channel is >> static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed. > Why do you need this? You already need a binding indicating which ports will be pre-allocated. So you could update your binding to pass a flag telling Linux "don't close it". > > I have already proposed that before and I haven't seen any explanation why this is not a viable solution. Sorry I didn’t mention this earlier, I started with your suggestion to fix the issue but after going through the Linux evtchn driver code it is not straight forward to tell Linux don’t close the port. Let me try to explain. In Linux, struct user_evtchn {} is the struct that hold the information for each user evtchn opened. We can add one bool parameter in this struct to tell Linux driver via IOCTL if evtchn is static. When user application close the fd "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call evtchn_unbind_from_user() for each evtchn. evtchn_unbind_from_user() will call __unbind_from_irq(irq) that will call xen_evtchn_close() . We need references to "struct user_evtchn” in function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to close the static event channel. I am not able to find any way to get struct user_evtchn in function __unbind_from_irq() , without modifying the other Linux structure. Regards, Rahul
(+ Juergen for the Linux question) On 06/07/2022 11:42, Rahul Singh wrote: > Hi Julien, > >> On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 05/07/2022 14:28, Rahul Singh wrote: >>> Hi Julien, >> >> Hi Rahul, >> >>>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote: >>>>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature. >>>> >>>> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it". >>> I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the >>> event channel state to ECS_STATIC when Xen allocate and create the static event channels. >> >> From what you wrote, ECS_STATIC is just an interdomain behind but where you want Xen to prevent closing the port. >> >> From Xen PoV, it is still not clear why this is a problem to let Linux closing such port. From the guest PoV, there are other way to pass this information (see below). > > If Linux closes the port, the static event channel created by Xen associated with such port will not be available to use afterward. > > When I started implemented the static event channel series, I thought the static event channel has to be available for use during > the lifetime of the guest. This patch avoids closing the port if the Linux user-space application wants to use the event channel again. > > This patch is fixing the problem for Linux OS, and I agree with you that we should not modify the Xen to fix the Linux problem. > Therefore, If the guest decided to close the static event channel, Xen will close the port. Event Chanel associated with the port > will not be available for use after that.I will discard this patch in the next series. > >> >>> From guest OS we can check if the event channel is static (via EVTCHNOP_status() hypercall ), if the event channel is >>> static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed. >> Why do you need this? You already need a binding indicating which ports will be pre-allocated. So you could update your binding to pass a flag telling Linux "don't close it". >> >> I have already proposed that before and I haven't seen any explanation why this is not a viable solution. > > Sorry I didn’t mention this earlier, I started with your suggestion to fix the issue but after going through the Linux evtchn driver code > it is not straight forward to tell Linux don’t close the port. Let me try to explain. > > In Linux, struct user_evtchn {} is the struct that hold the information for each user evtchn opened. We can add one bool parameter in this struct to tell Linux driver > via IOCTL if evtchn is static. When user application close the fd "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call evtchn_unbind_from_user() > for each evtchn. evtchn_unbind_from_user() will call __unbind_from_irq(irq) that will call xen_evtchn_close() . We need references to "struct user_evtchn” in > function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to close the static event channel. I am not able to find any way to get > struct user_evtchn in function __unbind_from_irq() , without modifying the other Linux structure. > > Regards, > Rahul >
On 06.07.22 13:04, Julien Grall wrote: > (+ Juergen for the Linux question) > > On 06/07/2022 11:42, Rahul Singh wrote: >> Hi Julien, >> >>> On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote: >>> >>> >>> >>> On 05/07/2022 14:28, Rahul Singh wrote: >>>> Hi Julien, >>> >>> Hi Rahul, >>> >>>>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote: >>>>>> a new driver in linux kernel, etc where right now we just need to >>>>>> introduce an extra IOCTL in linux to support this feature. >>>>> >>>>> I don't understand why would need a new driver, etc. Given that you are >>>>> introducing a new IOCTL you could pass a flag to say "This is a static >>>>> event channel so don't close it". >>>> I tried to implement other solutions to this issue. We can introduce a new >>>> event channel state “ECS_STATIC” and set the >>>> event channel state to ECS_STATIC when Xen allocate and create the static >>>> event channels. >>> >>> From what you wrote, ECS_STATIC is just an interdomain behind but where you >>> want Xen to prevent closing the port. >>> >>> From Xen PoV, it is still not clear why this is a problem to let Linux >>> closing such port. From the guest PoV, there are other way to pass this >>> information (see below). >> >> If Linux closes the port, the static event channel created by Xen associated >> with such port will not be available to use afterward. >> >> When I started implemented the static event channel series, I thought the >> static event channel has to be available for use during >> the lifetime of the guest. This patch avoids closing the port if the Linux >> user-space application wants to use the event channel again. >> >> This patch is fixing the problem for Linux OS, and I agree with you that we >> should not modify the Xen to fix the Linux problem. >> Therefore, If the guest decided to close the static event channel, Xen will >> close the port. Event Chanel associated with the port >> will not be available for use after that.I will discard this patch in the next >> series. >> >>> >>>> From guest OS we can check if the event channel is static (via >>>> EVTCHNOP_status() hypercall ), if the event channel is >>>> static don’t try to close the event channel. If guest OS try to close the >>>> static event channel Xen will return error as static event channel can’t be >>>> closed. >>> Why do you need this? You already need a binding indicating which ports will >>> be pre-allocated. So you could update your binding to pass a flag telling >>> Linux "don't close it". >>> >>> I have already proposed that before and I haven't seen any explanation why >>> this is not a viable solution. >> >> Sorry I didn’t mention this earlier, I started with your suggestion to fix the >> issue but after going through the Linux evtchn driver code >> it is not straight forward to tell Linux don’t close the port. Let me try to >> explain. >> >> In Linux, struct user_evtchn {} is the struct that hold the information for >> each user evtchn opened. We can add one bool parameter in this struct to tell >> Linux driver >> via IOCTL if evtchn is static. When user application close the fd >> "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call >> evtchn_unbind_from_user() >> for each evtchn. evtchn_unbind_from_user() will call __unbind_from_irq(irq) >> that will call xen_evtchn_close() . We need references to "struct user_evtchn” in >> function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to >> close the static event channel. I am not able to find any way to get >> struct user_evtchn in function __unbind_from_irq() , without modifying the >> other Linux structure. The "static" flag should be added to struct irq_info. In case all relevant event channels are really user ones, we could easily add another "static" flag to evtchn_make_refcounted(), which is already used to set a user event channel specific value into struct irq_info when binding the event channel. Juergen
Hi Juergen, > On 6 Jul 2022, at 12:33 pm, Juergen Gross <jgross@suse.com> wrote: > > On 06.07.22 13:04, Julien Grall wrote: >> (+ Juergen for the Linux question) >> On 06/07/2022 11:42, Rahul Singh wrote: >>> Hi Julien, >>> >>>> On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote: >>>> >>>> >>>> >>>> On 05/07/2022 14:28, Rahul Singh wrote: >>>>> Hi Julien, >>>> >>>> Hi Rahul, >>>> >>>>>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote: >>>>>>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature. >>>>>> >>>>>> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it". >>>>> I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the >>>>> event channel state to ECS_STATIC when Xen allocate and create the static event channels. >>>> >>>> From what you wrote, ECS_STATIC is just an interdomain behind but where you want Xen to prevent closing the port. >>>> >>>> From Xen PoV, it is still not clear why this is a problem to let Linux closing such port. From the guest PoV, there are other way to pass this information (see below). >>> >>> If Linux closes the port, the static event channel created by Xen associated with such port will not be available to use afterward. >>> >>> When I started implemented the static event channel series, I thought the static event channel has to be available for use during >>> the lifetime of the guest. This patch avoids closing the port if the Linux user-space application wants to use the event channel again. >>> >>> This patch is fixing the problem for Linux OS, and I agree with you that we should not modify the Xen to fix the Linux problem. >>> Therefore, If the guest decided to close the static event channel, Xen will close the port. Event Chanel associated with the port >>> will not be available for use after that.I will discard this patch in the next series. >>> >>>> >>>>> From guest OS we can check if the event channel is static (via EVTCHNOP_status() hypercall ), if the event channel is >>>>> static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed. >>>> Why do you need this? You already need a binding indicating which ports will be pre-allocated. So you could update your binding to pass a flag telling Linux "don't close it". >>>> >>>> I have already proposed that before and I haven't seen any explanation why this is not a viable solution. >>> >>> Sorry I didn’t mention this earlier, I started with your suggestion to fix the issue but after going through the Linux evtchn driver code >>> it is not straight forward to tell Linux don’t close the port. Let me try to explain. >>> >>> In Linux, struct user_evtchn {} is the struct that hold the information for each user evtchn opened. We can add one bool parameter in this struct to tell Linux driver >>> via IOCTL if evtchn is static. When user application close the fd "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call evtchn_unbind_from_user() >>> for each evtchn. evtchn_unbind_from_user() will call __unbind_from_irq(irq) that will call xen_evtchn_close() . We need references to "struct user_evtchn” in >>> function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to close the static event channel. I am not able to find any way to get >>> struct user_evtchn in function __unbind_from_irq() , without modifying the other Linux structure. > > The "static" flag should be added to struct irq_info. In case all relevant > event channels are really user ones, we could easily add another "static" > flag to evtchn_make_refcounted(), which is already used to set a user > event channel specific value into struct irq_info when binding the event > channel. > As suggested by you, I modified the Linux Kernel by adding “static" flag in struct irq_info and it works fine. We can skip the closing of static channel if required. I will send the patch for review once I will send the patch for new ioctl for static event channel. Regards, Rahul
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 5f97d9d181..89195b042c 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3171,7 +3171,7 @@ static int __init alloc_xenstore_evtchn(struct domain *d) alloc.dom = d->domain_id; alloc.remote_dom = hardware_domain->domain_id; - rc = evtchn_alloc_unbound(&alloc, 0); + rc = evtchn_alloc_unbound(&alloc, 0, false); if ( rc ) { printk("Failed allocating event channel for domain\n"); diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 84f0055a5a..cedc98ccaf 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn) * If port is zero get the next free port and allocate. If port is non-zero * allocate the specified port. */ -int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port, + bool is_static) { struct evtchn *chn; struct domain *d; @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port) evtchn_write_lock(chn); chn->state = ECS_UNBOUND; + chn->is_static = is_static; if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF ) chn->u.unbound.remote_domid = current->domain->domain_id; evtchn_port_init(d, chn); @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn) * allocate the specified lport. */ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, - evtchn_port_t lport) + evtchn_port_t lport, bool is_static) { struct evtchn *lchn, *rchn; struct domain *rd; @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, lchn->u.interdomain.remote_dom = rd; lchn->u.interdomain.remote_port = rport; lchn->state = ECS_INTERDOMAIN; + lchn->is_static = is_static; evtchn_port_init(ld, lchn); rchn->u.interdomain.remote_dom = ld; @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest) rc = -EINVAL; goto out; } + /* Guest cannot close a static event channel. */ + if ( chn1->is_static && guest ) + goto out; switch ( chn1->state ) { @@ -1238,7 +1244,7 @@ long cf_check do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) struct evtchn_alloc_unbound alloc_unbound; if ( copy_from_guest(&alloc_unbound, arg, 1) != 0 ) return -EFAULT; - rc = evtchn_alloc_unbound(&alloc_unbound, 0); + rc = evtchn_alloc_unbound(&alloc_unbound, 0, false); if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) ) rc = -EFAULT; /* Cleaning up here would be a mess! */ break; @@ -1248,7 +1254,8 @@ long cf_check do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) struct evtchn_bind_interdomain bind_interdomain; if ( copy_from_guest(&bind_interdomain, arg, 1) != 0 ) return -EFAULT; - rc = evtchn_bind_interdomain(&bind_interdomain, current->domain, 0); + rc = evtchn_bind_interdomain(&bind_interdomain, current->domain, + 0, false); if ( !rc && __copy_to_guest(arg, &bind_interdomain, 1) ) rc = -EFAULT; /* Cleaning up here would be a mess! */ break; diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index 8eae9984a9..71ad4c5bfd 100644 --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -73,12 +73,12 @@ int evtchn_allocate_port(struct domain *d, unsigned int port); /* Allocate a new event channel */ int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, - evtchn_port_t port); + evtchn_port_t port, bool is_static); /* Bind an event channel port to interdomain */ int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld, - evtchn_port_t port); + evtchn_port_t port, bool is_static); /* Unmask a local event-channel port. */ int evtchn_unmask(unsigned int port); diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 463d41ffb6..da823c8091 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -119,6 +119,7 @@ struct evtchn unsigned char priority; /* FIFO event channels only. */ unsigned short notify_vcpu_id; /* VCPU for local delivery notification */ uint32_t fifo_lastq; /* Data for identifying last queue. */ + bool is_static; /* Static event channels. */ #ifdef CONFIG_XSM union {
Guest can request the Xen to close the event channels. Ignore the request from guest to close the static channels as static event channels should not be closed. Add the new bool variable "is_static" in "struct evtchn" to mark the event channel static when creating the event channel. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- xen/arch/arm/domain_build.c | 2 +- xen/common/event_channel.c | 15 +++++++++++---- xen/include/xen/event.h | 4 ++-- xen/include/xen/sched.h | 1 + 4 files changed, 15 insertions(+), 7 deletions(-)