Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,


> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 28/06/2022 15:52, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 28 Jun 2022, at 15:26, Julien Grall <julien@xxxxxxx> 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@xxxxxxx> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 23/06/2022 16:10, Rahul Singh wrote:
>>>>>> Hi Julien,
>>>>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xxxxxxx> 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