Mailing List Archive

Hi Henry,

On 24/04/2024 04:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>
> Currently, irq_route and mapping is only allowed at the domain creation. Adding
> exception for CONFIG_OVERLAY_DTB.

AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when a
physical IRQ can be routed/removed from/to a domain").

>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> xen/arch/arm/gic.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 44c40e86de..a775f886ed 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> * back to the physical IRQ. To prevent get unsync, restrict the
> * routing to when the Domain is been created.
> */

The above comment explains why the check was added. But the commit
message doesn't explain why this can be disregarded for your use-case.

Looking at the history, I don't think you can simply remove the checks.

Regardless that...

> +#ifndef CONFIG_OVERLAY_DTB

... I am against such #ifdef. A distros may want to have OVERLAY_DTB
enabled, yet the user will not use it.

Instead, you want to remove the check once the code can properly handle
routing an IRQ the domain is created or ...

> if ( d->creation_finished )
> return -EBUSY;
> +#endif
>
> ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
> if ( ret )
> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> * Removing an interrupt while the domain is running may have
> * undesirable effect on the vGIC emulation.
> */
> +#ifndef CONFIG_OVERLAY_DTB
> if ( !d->is_dying )
> return -EBUSY;
> +#endif

... removed before they domain is destroyed.

>
> desc->handler->shutdown(desc);
>

Cheers,

--
Julien Grall

Hi Julien,

On 4/24/2024 8:58 PM, Julien Grall wrote:
> Hi Henry,
>
> On 24/04/2024 04:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>>
>> Currently, irq_route and mapping is only allowed at the domain
>> creation. Adding
>> exception for CONFIG_OVERLAY_DTB.
>
> AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when
> a physical IRQ can be routed/removed from/to a domain").
>
>>
>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>>   xen/arch/arm/gic.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 44c40e86de..a775f886ed 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d,
>> unsigned int virq,
>>        * back to the physical IRQ. To prevent get unsync, restrict the
>>        * routing to when the Domain is been created.
>>        */
>
> The above comment explains why the check was added. But the commit
> message doesn't explain why this can be disregarded for your use-case.
>
> Looking at the history, I don't think you can simply remove the checks.
>
> Regardless that...
>
>> +#ifndef CONFIG_OVERLAY_DTB
>
> ... I am against such #ifdef. A distros may want to have OVERLAY_DTB
> enabled, yet the user will not use it.
>
> Instead, you want to remove the check once the code can properly
> handle routing an IRQ the domain is created or ...
>
>>       if ( d->creation_finished )
>>           return -EBUSY;
>> +#endif
>>         ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>>       if ( ret )
>> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d,
>> unsigned int virq,
>>        * Removing an interrupt while the domain is running may have
>>        * undesirable effect on the vGIC emulation.
>>        */
>> +#ifndef CONFIG_OVERLAY_DTB
>>       if ( !d->is_dying )
>>           return -EBUSY;
>> +#endif
>
> ... removed before they domain is destroyed.

Thanks for your feeedback. After checking the b8577547236f commit
message I think I now understand your point. Do you have any suggestion
about how can I properly add the support to route/remove the IRQ to
running domains? Thanks.

Kind regards,
Henry

>
>> desc->handler->shutdown(desc);
>
> Cheers,
>

Hi,

On 25/04/2024 08:06, Henry Wang wrote:
> Hi Julien,
>
> On 4/24/2024 8:58 PM, Julien Grall wrote:
>> Hi Henry,
>>
>> On 24/04/2024 04:34, Henry Wang wrote:
>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>
>>> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>>>
>>> Currently, irq_route and mapping is only allowed at the domain
>>> creation. Adding
>>> exception for CONFIG_OVERLAY_DTB.
>>
>> AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when
>> a physical IRQ can be routed/removed from/to a domain").
>>
>>>
>>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>>> ---
>>>   xen/arch/arm/gic.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 44c40e86de..a775f886ed 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d,
>>> unsigned int virq,
>>>        * back to the physical IRQ. To prevent get unsync, restrict the
>>>        * routing to when the Domain is been created.
>>>        */
>>
>> The above comment explains why the check was added. But the commit
>> message doesn't explain why this can be disregarded for your use-case.
>>
>> Looking at the history, I don't think you can simply remove the checks.
>>
>> Regardless that...
>>
>>> +#ifndef CONFIG_OVERLAY_DTB
>>
>> ... I am against such #ifdef. A distros may want to have OVERLAY_DTB
>> enabled, yet the user will not use it.
>>
>> Instead, you want to remove the check once the code can properly
>> handle routing an IRQ the domain is created or ...
>>
>>>       if ( d->creation_finished )
>>>           return -EBUSY;
>>> +#endif
>>>         ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>>>       if ( ret )
>>> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d,
>>> unsigned int virq,
>>>        * Removing an interrupt while the domain is running may have
>>>        * undesirable effect on the vGIC emulation.
>>>        */
>>> +#ifndef CONFIG_OVERLAY_DTB
>>>       if ( !d->is_dying )
>>>           return -EBUSY;
>>> +#endif
>>
>> ... removed before they domain is destroyed.
>
> Thanks for your feeedback. After checking the b8577547236f commit
> message I think I now understand your point. Do you have any suggestion
> about how can I properly add the support to route/remove the IRQ to
> running domains? Thanks.

I haven't really look at that code in quite a while. I think we need to
make sure that the virtual and physical IRQ state matches at the time we
do the routing.

I am undecided on whether we want to simply prevent the action to happen
or try to reset the state.

There is also the question of what to do if the guest is enabling the
vIRQ before it is routed.

Overall, someone needs to spend some time reading the code and then make
a proposal (this could be just documentation if we believe it is safe to
do). Both the current vGIC and the new one may need an update.

Cheers,

--
Julien Grall

Hi Julien,

Sorry for the late reply,

On 4/25/2024 10:28 PM, Julien Grall wrote:
> Hi,
>
> On 25/04/2024 08:06, Henry Wang wrote:
>> Hi Julien,
>>
>> On 4/24/2024 8:58 PM, Julien Grall wrote:
>>> Hi Henry,
>>>
>>> On 24/04/2024 04:34, Henry Wang wrote:
>>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>>
>>>> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>>>>
>>>> Currently, irq_route and mapping is only allowed at the domain
>>>> creation. Adding
>>>> exception for CONFIG_OVERLAY_DTB.
>>>
>>> AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict
>>> when a physical IRQ can be routed/removed from/to a domain").
>>>
>>>>
>>>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>>>> ---
>>>>   xen/arch/arm/gic.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index 44c40e86de..a775f886ed 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d,
>>>> unsigned int virq,
>>>>        * back to the physical IRQ. To prevent get unsync, restrict the
>>>>        * routing to when the Domain is been created.
>>>>        */
>>>
>>> The above comment explains why the check was added. But the commit
>>> message doesn't explain why this can be disregarded for your use-case.
>>>
>>> Looking at the history, I don't think you can simply remove the checks.
>>>
>>> Regardless that...
>>>
>>>> +#ifndef CONFIG_OVERLAY_DTB
>>>
>>> ... I am against such #ifdef. A distros may want to have OVERLAY_DTB
>>> enabled, yet the user will not use it.
>>>
>>> Instead, you want to remove the check once the code can properly
>>> handle routing an IRQ the domain is created or ...
>>>
>>>>       if ( d->creation_finished )
>>>>           return -EBUSY;
>>>> +#endif
>>>>         ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>>>>       if ( ret )
>>>> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain
>>>> *d, unsigned int virq,
>>>>        * Removing an interrupt while the domain is running may have
>>>>        * undesirable effect on the vGIC emulation.
>>>>        */
>>>> +#ifndef CONFIG_OVERLAY_DTB
>>>>       if ( !d->is_dying )
>>>>           return -EBUSY;
>>>> +#endif
>>>
>>> ... removed before they domain is destroyed.
>>
>> Thanks for your feeedback. After checking the b8577547236f commit
>> message I think I now understand your point. Do you have any
>> suggestion about how can I properly add the support to route/remove
>> the IRQ to running domains? Thanks.

I spent some time going through the GIC/vGIC code and had some
discussions with Stefano and Stewart during the last couple of days, let
me see if I can describe the use case properly now to continue the
discussion:

We have some use cases that requires assigning devices to domains after
domain boot time. For example, suppose there is an FPGA on the board
which can simulate a device, and the bitstream for the FPGA is provided
and programmed after domain boot. So we need a way to assign the device
to the running domain. This series tries to implement this use case by
using device tree overlay - users can firstly add the overlay to Xen
dtb, assign the device in the overlay to a domain by the xl command,
then apply the overlay to Linux.

> I haven't really look at that code in quite a while. I think we need
> to make sure that the virtual and physical IRQ state matches at the
> time we do the routing.
>
> I am undecided on whether we want to simply prevent the action to
> happen or try to reset the state.
>
> There is also the question of what to do if the guest is enabling the
> vIRQ before it is routed.

Sorry for bothering, would you mind elaborating a bit more about the two
cases that you mentioned above? Commit b8577547236f ("xen/arm: Restrict
when a physical IRQ can be routed/removed from/to a domain") only said
there will be undesirable effects, so I am not sure if I understand the
concerns raised above and the consequences of these two use cases. I am
probably wrong, I think when we add the overlay, we are probably fine as
the interrupt is not being used before. Also since we only load the
device driver after the IRQ is routed to the guest, I am not sure the
guest can enable the vIRQ before it is routed.

Kind regards,
Henry

> Overall, someone needs to spend some time reading the code and then
> make a proposal (this could be just documentation if we believe it is
> safe to do). Both the current vGIC and the new one may need an update.
>
> Cheers,
>

Hi Henry,

On 30/04/2024 04:50, Henry Wang wrote:
> On 4/25/2024 10:28 PM, Julien Grall wrote:
>>> Thanks for your feeedback. After checking the b8577547236f commit
>>> message I think I now understand your point. Do you have any
>>> suggestion about how can I properly add the support to route/remove
>>> the IRQ to running domains? Thanks.
>
> I spent some time going through the GIC/vGIC code and had some
> discussions with Stefano and Stewart during the last couple of days, let
> me see if I can describe the use case properly now to continue the
> discussion:
>
> We have some use cases that requires assigning devices to domains after
> domain boot time. For example, suppose there is an FPGA on the board
> which can simulate a device, and the bitstream for the FPGA is provided
> and programmed after domain boot. So we need a way to assign the device
> to the running domain. This series tries to implement this use case by
> using device tree overlay - users can firstly add the overlay to Xen
> dtb, assign the device in the overlay to a domain by the xl command,
> then apply the overlay to Linux.

Thanks for the description! This helps to understand your goal :).

>
>> I haven't really look at that code in quite a while. I think we need
>> to make sure that the virtual and physical IRQ state matches at the
>> time we do the routing.
>>
>> I am undecided on whether we want to simply prevent the action to
>> happen or try to reset the state.
>>
>> There is also the question of what to do if the guest is enabling the
>> vIRQ before it is routed.
>
> Sorry for bothering, would you mind elaborating a bit more about the two
> cases that you mentioned above? Commit b8577547236f ("xen/arm: Restrict
> when a physical IRQ can be routed/removed from/to a domain") only said
> there will be undesirable effects, so I am not sure if I understand the
> concerns raised above and the consequences of these two use cases.

I will try to explain them below after I answer the rest.

> I am
> probably wrong, I think when we add the overlay, we are probably fine as
> the interrupt is not being used before.

What if the DT overlay is unloaded and then reloaded? Wouldn't the same
interrupt be re-used? As a more generic case, this could also be a new
bitstream for the FPGA.

But even if the interrupt is brand new every time for the DT overlay,
you are effectively relaxing the check for every user (such as
XEN_DOMCTL_bind_pt_irq). So the interrupt re-use case needs to be taken
into account.

> Also since we only load the
> device driver after the IRQ is routed to the guest,

This is what a well-behave guest will do. However, we need to think what
will happen if a guest misbehaves. I am not concerned about a guest only
impacting itself, I am more concerned about the case where the rest of
the system is impacted.

> I am not sure the
> guest can enable the vIRQ before it is routed.

Xen allows the guest to enable a vIRQ even if there is no pIRQ assigned.
Thanksfully, it looks like the vgic_connect_hw_irq(), in both the
current and new vGIC, will return an error if we are trying to route a
pIRQ to an already enabled vIRQ.

But we need to investigate all the possible scenarios to make sure that
any inconsistencies between the physical state and virtual state
(including the LRs) will not result to bigger problem.

The one that comes to my mind is: The physical interrupt is de-assigned
from the guest before it was EOIed. In this case, the interrupt will
still be in the LR with the HW bit set. This would allow the guest to
EOI the interrupt even if it is routed to someone else. It is unclear
what would be the impact on the other guest.

Cheers,

--
Julien Grall