[PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo

[PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo

[Henry Wang]

Hi all,

This is the remaining series for the full functional "dynamic node
programming using overlay dtbo" feature. The first part [1] has
already been merged.

Quoting from the original series, the first part has already made
Xen aware of new device tree node which means updating the dt_host
with overlay node information, and in this series, the goal is to
map IRQ and IOMMU during runtime, where we will do the actual IOMMU
and IRQ mapping to a running domain and will call unmap_mmio_regions()
to remove the mapping.

Also, documentation of the "dynamic node programming using overlay dtbo"
feature is added.

Patch 1 is a fix of [1] which is noticed during my local test, details
please see the commit message.

Gitlab CI for this series can be found in [2].

[1] https://lore.kernel.org/xen-devel/20230906011631.30310-1-vikram.garhwal@amd.com/
[2] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1265297506

Henry Wang (1):
  xen/commom/dt-overlay: Fix missing lock when remove the device

Vikram Garhwal (14):
  xen/arm/gic: Enable interrupt assignment to running VM
  xen/arm: Always enable IOMMU
  tools/libs/light: Always enable IOMMU
  tools/libs/light: Increase nr_spi to 160
  rangeset: Move struct range and struct rangeset to headerfile
  xen/overlay: Enable device tree overlay assignment to running domains
  tools: Add domain_id and expert mode for overlay operations
  tools/libs/light: Modify dtbo to domU linux dtbo format
  tools/xl: Share overlay with domU
  tools/helpers: Add get_overlay
  get_overlay: remove domU overlay
  xl/overlay: add remove operation to xenstore
  add a domU script to fetch overlays and applying them to linux
  docs: add device tree overlay documentation

 docs/misc/arm/overlay.txt           | 172 +++++++++
 tools/helpers/Makefile              |   8 +
 tools/helpers/get_overlay.c         | 507 ++++++++++++++++++++++++++
 tools/helpers/get_overlay.sh        |  81 +++++
 tools/include/libxl.h               |   8 +-
 tools/include/xenctrl.h             |   5 +-
 tools/libs/ctrl/xc_dt_overlay.c     |   7 +-
 tools/libs/light/libxl_arm.c        |   9 +-
 tools/libs/light/libxl_dt_overlay.c |  90 ++++-
 tools/xl/Makefile                   |   2 +-
 tools/xl/xl_cmdtable.c              |   2 +-
 tools/xl/xl_vmcontrol.c             | 539 +++++++++++++++++++++++++++-
 xen/arch/arm/device.c               |   8 +-
 xen/arch/arm/dom0less-build.c       |   3 +-
 xen/arch/arm/domain_build.c         |   2 +-
 xen/arch/arm/gic.c                  |   4 +
 xen/arch/arm/include/asm/setup.h    |   3 +-
 xen/common/dt-overlay.c             | 223 ++++++++++--
 xen/common/rangeset.c               |  31 +-
 xen/include/public/sysctl.h         |   4 +-
 xen/include/xen/rangeset.h          |  32 +-
 21 files changed, 1654 insertions(+), 86 deletions(-)
 create mode 100644 docs/misc/arm/overlay.txt
 create mode 100644 tools/helpers/get_overlay.c
 create mode 100755 tools/helpers/get_overlay.sh

-- 
2.34.1

[PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device

[Henry Wang]

If CONFIG_DEBUG=y, below assertion will be triggered:
(XEN) Assertion 'rw_is_locked(&dt_host_lock)' failed at drivers/passthrough/device_tree.c:146
(XEN) ----[ Xen-4.19-unstable  arm64  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     00000a0000257418 iommu_remove_dt_device+0x8c/0xd4
(XEN) LR:     00000a00002573a0
(XEN) SP:     00008000fff7fb30
(XEN) CPSR:   0000000000000249 MODE:64-bit EL2h (Hypervisor, handler)
[...]

(XEN) Xen call trace:
(XEN)    [<00000a0000257418>] iommu_remove_dt_device+0x8c/0xd4 (PC)
(XEN)    [<00000a00002573a0>] iommu_remove_dt_device+0x14/0xd4 (LR)
(XEN)    [<00000a000020797c>] dt-overlay.c#remove_node_resources+0x8c/0x90
(XEN)    [<00000a0000207f14>] dt-overlay.c#remove_nodes+0x524/0x648
(XEN)    [<00000a0000208460>] dt_overlay_sysctl+0x428/0xc68
(XEN)    [<00000a00002707f8>] arch_do_sysctl+0x1c/0x2c
(XEN)    [<00000a0000230b40>] do_sysctl+0x96c/0x9ec
(XEN)    [<00000a0000271e08>] traps.c#do_trap_hypercall+0x1e8/0x288
(XEN)    [<00000a0000273490>] do_trap_guest_sync+0x448/0x63c
(XEN)    [<00000a000025c480>] entry.o#guest_sync_slowpath+0xa8/0xd8
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion 'rw_is_locked(&dt_host_lock)' failed at drivers/passthrough/device_tree.c:146
(XEN) ****************************************

This is because iommu_remove_dt_device() is called without taking the
dt_host_lock. Fix the issue by taking and releasing the lock properly.

Fixes: 7e5c4a8b86f1 ("xen/arm: Implement device tree node removal functionalities")
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 xen/common/dt-overlay.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 1b197381f6..39e4ba59dd 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node *device_node)
     {
         if ( dt_device_is_protected(device_node) )
         {
+            write_lock(&dt_host_lock);
             rc = iommu_remove_dt_device(device_node);
             if ( rc < 0 )
+            {
+                write_unlock(&dt_host_lock);
                 return rc;
+            }
+            write_unlock(&dt_host_lock);
         }
     }
 
-- 
2.34.1

Re: [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device

[Jan Beulich]

On 24.04.2024 05:34, Henry Wang wrote:
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node *device_node)
>      {
>          if ( dt_device_is_protected(device_node) )
>          {
> +            write_lock(&dt_host_lock);
>              rc = iommu_remove_dt_device(device_node);

Any particular reason you add two call sites to the unlock function,
instead of putting it here?

Jan

>              if ( rc < 0 )
> +            {
> +                write_unlock(&dt_host_lock);
>                  return rc;
> +            }
> +            write_unlock(&dt_host_lock);
>          }
>      }
>  

Re: [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device

[Henry Wang]

Hi Jan,

On 4/24/2024 1:58 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> --- a/xen/common/dt-overlay.c
>> +++ b/xen/common/dt-overlay.c
>> @@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node *device_node)
>>       {
>>           if ( dt_device_is_protected(device_node) )
>>           {
>> +            write_lock(&dt_host_lock);
>>               rc = iommu_remove_dt_device(device_node);
> Any particular reason you add two call sites to the unlock function,
> instead of putting it here?

Oh...you are correct. It is indeed better to put the unlock here. If 
this is the only comment for this patch, can I respin this only patch as 
a v1.1 or would one of the committers be ok to fix on commit? Sorry for 
the trouble and thanks for the suggestion.

Kind regards,
Henry

> Jan
>
>>               if ( rc < 0 )
>> +            {
>> +                write_unlock(&dt_host_lock);
>>                   return rc;
>> +            }
>> +            write_unlock(&dt_host_lock);
>>           }
>>       }
>>   

[PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

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.

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.
      */
+#ifndef CONFIG_OVERLAY_DTB
     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
 
     desc->handler->shutdown(desc);
 
-- 
2.34.1

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

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

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

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,
>

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

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

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

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,
>

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

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

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

Hi Julien,

On 5/1/2024 4:13 AM, Julien Grall wrote:
> 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 :).

Thank you very much for spending your time on discussing this and 
provide these valuable comments!

>>
>>> 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.

I agree. I think IIUC, with your explanation here and below, could we 
simplify the problem to how to properly handle the removal of the IRQ 
from a running guest, if we always properly remove and clean up the 
information when remove the IRQ from the guest? In this way, the IRQ can 
always be viewed as a brand new one when we add it back. Then the only 
corner case that we need to take care of would be...

>> 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.

...same as this case, i.e.
test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED, 
&desc->status)) when we try to remove the IRQ from a running domain.

we have 3 possible states which can be read from LR for this case : 
active, pending, pending and active.
- I don't think we can do anything about the active state, so we should 
return -EBUSY and reject the whole operation of removing the IRQ from 
running guest, and user can always retry this operation.
- For the pending (and active) case, can we clear the LR and point the 
LR for the pending_irq to invalid?

Kind regards,
Henry

>
> Cheers,
>

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

Hi Henry,

On 06/05/2024 09:32, Henry Wang wrote:
> On 5/1/2024 4:13 AM, Julien Grall wrote:
>> 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 :).
> 
> Thank you very much for spending your time on discussing this and 
> provide these valuable comments!
> 
>>>
>>>> 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.
> 
> I agree. I think IIUC, with your explanation here and below, could we 
> simplify the problem to how to properly handle the removal of the IRQ 
> from a running guest, if we always properly remove and clean up the 
> information when remove the IRQ from the guest? In this way, the IRQ can 
> always be viewed as a brand new one when we add it back.

If we can make sure the virtual IRQ and physical IRQ is cleaned then yes.

> Then the only 
> corner case that we need to take care of would be...

Can you clarify whether you say the "only corner case" because you 
looked at the code? Or is it just because I mentioned only one?

> 
>>> 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.
> 
> ...same as this case, i.e.
> test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED, 
> &desc->status)) when we try to remove the IRQ from a running domain.

We already call ->shutdown() which will disable the IRQ. So don't we 
only need to take care of _IRQ_INPROGRESS?

[...]

> we have 3 possible states which can be read from LR for this case : 
> active, pending, pending and active.
> - I don't think we can do anything about the active state, so we should 
> return -EBUSY and reject the whole operation of removing the IRQ from 
> running guest, and user can always retry this operation.

This would mean a malicious/buggy guest would be able to prevent a 
device to be de-assigned. This is not a good idea in particular when the 
domain is dying.

That said, I think you can handle this case. The LR has a bit to 
indicate whether the pIRQ needs to be EOIed. You can clear it and this 
would prevent the guest to touch the pIRQ. There might be other clean-up 
to do in the vGIC datastructure.

Anyway, we don't have to handle removing an active IRQ when the domain 
is still running (although we do when the domain is destroying). But I 
think this would need to be solved before the feature is (security) 
supported.

> - For the pending (and active) case,

Shouldn't the pending and active case handled the same way as the active 
case?

> can we clear the LR and point the 
> LR for the pending_irq to invalid?

LRs can be cleared. You will need to find which vCPU was used for the 
injection and then pause it so the LR can be safely updated.

There will also be some private state to clear. I don't know how easy it 
will be. However, we decided to not do anything for ICPENDR (which 
requires a similar behavior) as this was complex (?) to do with the 
existing vGIC.

I vaguely remember we had some discussions on the ML. I didn't look for 
them though.

Anyway, same as above, this could possibly handled later on. But this 
would probably need to be solved before the feature is (security supported).

Cheers,

-- 
Julien Grall

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

Hi Julien,

On 5/8/2024 5:54 AM, Julien Grall wrote:
> Hi Henry,
>>> 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.
>>
>> I agree. I think IIUC, with your explanation here and below, could we 
>> simplify the problem to how to properly handle the removal of the IRQ 
>> from a running guest, if we always properly remove and clean up the 
>> information when remove the IRQ from the guest? In this way, the IRQ 
>> can always be viewed as a brand new one when we add it back.
>
> If we can make sure the virtual IRQ and physical IRQ is cleaned then yes.
>
>> Then the only corner case that we need to take care of would be...
>
> Can you clarify whether you say the "only corner case" because you 
> looked at the code? Or is it just because I mentioned only one?

Well, I indeed checked the code and to my best knowledge the corner case 
that you pointed out would be the only one I can think of.

>>> 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.
>>
>> ...same as this case, i.e.
>> test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED, 
>> &desc->status)) when we try to remove the IRQ from a running domain.
>
> We already call ->shutdown() which will disable the IRQ. So don't we 
> only need to take care of _IRQ_INPROGRESS?

Yes you are correct.

>> we have 3 possible states which can be read from LR for this case : 
>> active, pending, pending and active.
>> - I don't think we can do anything about the active state, so we 
>> should return -EBUSY and reject the whole operation of removing the 
>> IRQ from running guest, and user can always retry this operation.
>
> This would mean a malicious/buggy guest would be able to prevent a 
> device to be de-assigned. This is not a good idea in particular when 
> the domain is dying.
>
> That said, I think you can handle this case. The LR has a bit to 
> indicate whether the pIRQ needs to be EOIed. You can clear it and this 
> would prevent the guest to touch the pIRQ. There might be other 
> clean-up to do in the vGIC datastructure.

I probably misunderstood this sentence, do you mean the EOI bit in the 
pINTID field? I think this bit is only available when the HW bit of LR 
is 0, but in our case the HW is supposed to be 1 (as indicated as your 
previous comment). Would you mind clarifying a bit more? Thanks!

> Anyway, we don't have to handle removing an active IRQ when the domain 
> is still running (although we do when the domain is destroying). But I 
> think this would need to be solved before the feature is (security) 
> supported.
>
>> - For the pending (and active) case,
>
> Shouldn't the pending and active case handled the same way as the 
> active case?

Sorry, yes you are correct.

Kind regards,
Henry

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

Hi Henry,

On 08/05/2024 08:49, Henry Wang wrote:
> On 5/8/2024 5:54 AM, Julien Grall wrote:
>> Hi Henry,
>>>> 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.
>>>
>>> I agree. I think IIUC, with your explanation here and below, could we 
>>> simplify the problem to how to properly handle the removal of the IRQ 
>>> from a running guest, if we always properly remove and clean up the 
>>> information when remove the IRQ from the guest? In this way, the IRQ 
>>> can always be viewed as a brand new one when we add it back.
>>
>> If we can make sure the virtual IRQ and physical IRQ is cleaned then yes.
>>
>>> Then the only corner case that we need to take care of would be...
>>
>> Can you clarify whether you say the "only corner case" because you 
>> looked at the code? Or is it just because I mentioned only one?
> 
> Well, I indeed checked the code and to my best knowledge the corner case 
> that you pointed out would be the only one I can think of.

Ok :). I was just checking you had a look as well.

[...]

>>> we have 3 possible states which can be read from LR for this case : 
>>> active, pending, pending and active.
>>> - I don't think we can do anything about the active state, so we 
>>> should return -EBUSY and reject the whole operation of removing the 
>>> IRQ from running guest, and user can always retry this operation.
>>
>> This would mean a malicious/buggy guest would be able to prevent a 
>> device to be de-assigned. This is not a good idea in particular when 
>> the domain is dying.
>>
>> That said, I think you can handle this case. The LR has a bit to 
>> indicate whether the pIRQ needs to be EOIed. You can clear it and this 
>> would prevent the guest to touch the pIRQ. There might be other 
>> clean-up to do in the vGIC datastructure.
> 
> I probably misunderstood this sentence, do you mean the EOI bit in the 
> pINTID field? I think this bit is only available when the HW bit of LR 
> is 0, but in our case the HW is supposed to be 1 (as indicated as your 
> previous comment). Would you mind clarifying a bit more? Thanks!

You are right, ICH_LR.HW will be 1 for physical IRQ routed to a guest. 
What I was trying to explain is this bit could be cleared (with 
ICH_LR.pINTD adjusted).

Cheers,

-- 
Julien Grall

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

Hi Julien,

On 5/9/2024 4:46 AM, Julien Grall wrote:
> Hi Henry,
> [...]
>
>>>> we have 3 possible states which can be read from LR for this case : 
>>>> active, pending, pending and active.
>>>> - I don't think we can do anything about the active state, so we 
>>>> should return -EBUSY and reject the whole operation of removing the 
>>>> IRQ from running guest, and user can always retry this operation.
>>>
>>> This would mean a malicious/buggy guest would be able to prevent a 
>>> device to be de-assigned. This is not a good idea in particular when 
>>> the domain is dying.
>>>
>>> That said, I think you can handle this case. The LR has a bit to 
>>> indicate whether the pIRQ needs to be EOIed. You can clear it and 
>>> this would prevent the guest to touch the pIRQ. There might be other 
>>> clean-up to do in the vGIC datastructure.
>>
>> I probably misunderstood this sentence, do you mean the EOI bit in 
>> the pINTID field? I think this bit is only available when the HW bit 
>> of LR is 0, but in our case the HW is supposed to be 1 (as indicated 
>> as your previous comment). Would you mind clarifying a bit more? Thanks!
>
> You are right, ICH_LR.HW will be 1 for physical IRQ routed to a guest. 
> What I was trying to explain is this bit could be cleared (with 
> ICH_LR.pINTD adjusted).

Thank you for all the discussions. Based on that, would below diff make 
sense to you? I did a test of the dynamic dtbo adding/removing with a 
ethernet device with this patch applied. Test steps are:
(1) Use xl dt-overlay to add the ethernet device to Xen device tree and 
assign it to dom0.
(2) Create a domU.
(3) Use xl dt-overlay to de-assign the device from dom0 and assign it to 
domU.
(4) Destroy the domU.

The ethernet device is functional in the domain respectively when it is 
attached to a domain and I don't see errors when I destroy domU. But 
honestly I think the case we talked about is a quite unusual case so I 
am not sure if it was hit during my test.

```
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a775f886ed..d3f9cd2299 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -135,16 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, 
unsigned int virq,
      ASSERT(virq < vgic_num_irqs(d));
      ASSERT(!is_lpi(virq));

-    /*
-     * When routing an IRQ to guest, the virtual state is not synced
-     * back to the physical IRQ. To prevent get unsync, restrict the
-     * routing to when the Domain is been created.
-     */
-#ifndef CONFIG_OVERLAY_DTB
-    if ( d->creation_finished )
-        return -EBUSY;
-#endif
-
      ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
      if ( ret )
          return ret;
@@ -169,20 +159,40 @@ int gic_remove_irq_from_guest(struct domain *d, 
unsigned int virq,
      ASSERT(test_bit(_IRQ_GUEST, &desc->status));
      ASSERT(!is_lpi(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
-
      desc->handler->shutdown(desc);

      /* EOI the IRQ if it has not been done by the guest */
      if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
+    {
+        /*
+         * Handle the LR where the physical interrupt is de-assigned 
from the
+         * guest before it was EOIed
+         */
+        struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
+        struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
+        struct pending_irq *p = irq_to_pending(v_target, virq);
+        unsigned long flags;
+
+        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        /* LR allocated for the IRQ */
+        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) &&
+             test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+        {
+            gic_hw_ops->clear_lr(p->lr);
+            clear_bit(p->lr, &v_target->arch.lr_mask);
+
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+            clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
+            p->lr = GIC_INVALID_LR;
+        }
+        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+
+        vgic_lock_rank(v_target, rank, flags);
+        vgic_disable_irqs(v_target, (~rank->ienable) & rank->ienable, 
rank->index);
+        vgic_unlock_rank(v_target, rank, flags);
+
          gic_hw_ops->deactivate_irq(desc);
+    }
      clear_bit(_IRQ_INPROGRESS, &desc->status);

      ret = vgic_connect_hw_irq(d, NULL, virq, desc, false);
```

Kind regards,
Henry

>
> Cheers,
>

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

Hi,

On 09/05/2024 16:31, Henry Wang wrote:
> On 5/9/2024 4:46 AM, Julien Grall wrote:
>> Hi Henry,
>> [...]
>>
>>>>> we have 3 possible states which can be read from LR for this case : 
>>>>> active, pending, pending and active.
>>>>> - I don't think we can do anything about the active state, so we 
>>>>> should return -EBUSY and reject the whole operation of removing the 
>>>>> IRQ from running guest, and user can always retry this operation.
>>>>
>>>> This would mean a malicious/buggy guest would be able to prevent a 
>>>> device to be de-assigned. This is not a good idea in particular when 
>>>> the domain is dying.
>>>>
>>>> That said, I think you can handle this case. The LR has a bit to 
>>>> indicate whether the pIRQ needs to be EOIed. You can clear it and 
>>>> this would prevent the guest to touch the pIRQ. There might be other 
>>>> clean-up to do in the vGIC datastructure.
>>>
>>> I probably misunderstood this sentence, do you mean the EOI bit in 
>>> the pINTID field? I think this bit is only available when the HW bit 
>>> of LR is 0, but in our case the HW is supposed to be 1 (as indicated 
>>> as your previous comment). Would you mind clarifying a bit more? Thanks!
>>
>> You are right, ICH_LR.HW will be 1 for physical IRQ routed to a guest. 
>> What I was trying to explain is this bit could be cleared (with 
>> ICH_LR.pINTD adjusted).
> 
> Thank you for all the discussions. Based on that, would below diff make 
> sense to you? I did a test of the dynamic dtbo adding/removing with a 
> ethernet device with this patch applied. Test steps are:
> (1) Use xl dt-overlay to add the ethernet device to Xen device tree and 
> assign it to dom0.
> (2) Create a domU.
> (3) Use xl dt-overlay to de-assign the device from dom0 and assign it to 
> domU.
> (4) Destroy the domU.
> 
> The ethernet device is functional in the domain respectively when it is 
> attached to a domain and I don't see errors when I destroy domU. But 
> honestly I think the case we talked about is a quite unusual case so I 
> am not sure if it was hit during my test.

Correct, they are not errors that will happen in normal operations. You 
will want to tweak Linux or use XTF to trigger them.

We want to test the state active, pending and both together before the 
physical interrupt is routed and also removed.

> 
> ```
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index a775f886ed..d3f9cd2299 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -135,16 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, 
> unsigned int virq,
>       ASSERT(virq < vgic_num_irqs(d));
>       ASSERT(!is_lpi(virq));
> 
> -    /*
> -     * When routing an IRQ to guest, the virtual state is not synced
> -     * back to the physical IRQ. To prevent get unsync, restrict the
> -     * routing to when the Domain is been created.
> -     */
> -#ifndef CONFIG_OVERLAY_DTB
> -    if ( d->creation_finished )
> -        return -EBUSY;
> -#endif
> -
>       ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);

This is checking if the interrupt is already enabled. Do we also need to 
check for active/pending?

>       if ( ret )
>           return ret;
> @@ -169,20 +159,40 @@ int gic_remove_irq_from_guest(struct domain *d, 
> unsigned int virq,
>       ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>       ASSERT(!is_lpi(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
> -
>       desc->handler->shutdown(desc);
> 
>       /* EOI the IRQ if it has not been done by the guest */
>       if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> +    {

I assume this is just a PoC state, but I just want to point out that 
this will not work with the new vGIC (some of the functions doesn't 
exist there).

> +        /*
> +         * Handle the LR where the physical interrupt is de-assigned 
> from the
> +         * guest before it was EOIed
> +         */
> +        struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);

This will return a vCPU from the current affinity. This may not be where 
the interrupt was injected. From a brief look, I can't tell whether we 
have an easy way to know where the interrupt was injected (other than 
the pending_irq is in the list lr_queue/inflight)

> +        struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> +        struct pending_irq *p = irq_to_pending(v_target, virq);
> +        unsigned long flags;
> +
> +        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> +        /* LR allocated for the IRQ */
> +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) &&
> +             test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> +        {
> +            gic_hw_ops->clear_lr(p->lr);

This works on the current pCPU LR. However, the vCPU may not run on this 
pCPU.

> +            clear_bit(p->lr, &v_target->arch.lr_mask);
> +
> +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> +            clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> +            p->lr = GIC_INVALID_LR;

You also need to remove 'p' from the various list (e.g. inflight/lr_queue).

But as I wrote previously, I think it would be much easier if we simply 
clear the HW bit in the LR. So we don't have to try to mess up with the 
vGIC internal state which is quite complex.

I think it could be done as part of vgic_connect_hw_irq(). But I haven't 
fully investigate it.

> +        }
> +        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> +
> +        vgic_lock_rank(v_target, rank, flags);
> +        vgic_disable_irqs(v_target, (~rank->ienable) & rank->ienable, 
> rank->index);
> +        vgic_unlock_rank(v_target, rank, flags);

Why do you need to call vgic_disable_irqs()?

> +
>           gic_hw_ops->deactivate_irq(desc);
> +    }
>       clear_bit(_IRQ_INPROGRESS, &desc->status);
> 
>       ret = vgic_connect_hw_irq(d, NULL, virq, desc, false);
> ```
> 
> Kind regards,
> Henry
> 
>>
>> Cheers,
>>
> 

Cheers,

-- 
Julien Grall

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

Hi Julien,

On 5/10/2024 4:54 PM, Julien Grall wrote:
> Hi,
>
> On 09/05/2024 16:31, Henry Wang wrote:
>> On 5/9/2024 4:46 AM, Julien Grall wrote:
>>> Hi Henry,
>>> [...]
>>> ```
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index a775f886ed..d3f9cd2299 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -135,16 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, 
>>> unsigned int virq,
>>>       ASSERT(virq < vgic_num_irqs(d));
>>>       ASSERT(!is_lpi(virq));
>>>
>>> -    /*
>>> -     * When routing an IRQ to guest, the virtual state is not synced
>>> -     * back to the physical IRQ. To prevent get unsync, restrict the
>>> -     * routing to when the Domain is been created.
>>> -     */
>>> -#ifndef CONFIG_OVERLAY_DTB
>>> -    if ( d->creation_finished )
>>> -        return -EBUSY;
>>> -#endif
>>> -
>>>       ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>
> This is checking if the interrupt is already enabled. Do we also need 
> to check for active/pending?

Thank you for raising this! I assume you meant this?
@@ -444,7 +444,9 @@ int vgic_connect_hw_irq(struct domain *d, struct 
vcpu *v, unsigned int virq,
      {
          /* The VIRQ should not be already enabled by the guest */
          if ( !p->desc &&
-             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
              p->desc = desc;
          else
              ret = -EBUSY;

I think adding the check for active/pending check at the time of routing 
the IRQ makes sense, so I will add them (both for old and new vGIC 
implementation).

>>       if ( ret )
>>           return ret;
>> @@ -169,20 +159,40 @@ int gic_remove_irq_from_guest(struct domain *d, 
>> unsigned int virq,
>>       ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>>       ASSERT(!is_lpi(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
>> -
>>       desc->handler->shutdown(desc);
>>
>>       /* EOI the IRQ if it has not been done by the guest */
>>       if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
>> +    {
>
> I assume this is just a PoC state, but I just want to point out that 
> this will not work with the new vGIC (some of the functions doesn't 
> exist there).

Thank you. Yes currently we can discuss for the old vGIC implementation. 
After we reach the final conclusion I will do the changes for both old 
and new vGIC.

>> +        /*
>> +         * Handle the LR where the physical interrupt is de-assigned 
>> from the
>> +         * guest before it was EOIed
>> +         */
>> +        struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>
> This will return a vCPU from the current affinity. This may not be 
> where the interrupt was injected. From a brief look, I can't tell 
> whether we have an easy way to know where the interrupt was injected 
> (other than the pending_irq is in the list lr_queue/inflight)

I doubt if we need to handle more than this - I think if the pending_irq 
is not in the lr_queue/inflight list, it would not belong to the corner 
case we are talking about (?).

>> +        }
>> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>> +
>> +        vgic_lock_rank(v_target, rank, flags);
>> +        vgic_disable_irqs(v_target, (~rank->ienable) & 
>> rank->ienable, rank->index);
>> +        vgic_unlock_rank(v_target, rank, flags);
>
> Why do you need to call vgic_disable_irqs()?

I will drop this part.

Kind regards,
Henry

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall]

Hi Henry,

On 11/05/2024 08:29, Henry Wang wrote:
>>> +        /*
>>> +         * Handle the LR where the physical interrupt is de-assigned 
>>> from the
>>> +         * guest before it was EOIed
>>> +         */
>>> +        struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>>
>> This will return a vCPU from the current affinity. This may not be 
>> where the interrupt was injected. From a brief look, I can't tell 
>> whether we have an easy way to know where the interrupt was injected 
>> (other than the pending_irq is in the list lr_queue/inflight)
> 
> I doubt if we need to handle more than this - I think if the pending_irq 
> is not in the lr_queue/inflight list, it would not belong to the corner 
> case we are talking about (?).

I didn't suggest we would need to handle the case where the pending_irq 
is not any of the queues. I was pointing out that I think we don't 
directly store the vCPU ID where we injected the IRQ. Instead, the 
pending_irq is just in list, so we will possibly need to store the vCPU 
ID for convenience.

Cheers,

-- 
Julien Grall

Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Henry Wang]

Hi Julien,

On 5/11/2024 4:22 PM, Julien Grall wrote:
> Hi Henry,
>
> On 11/05/2024 08:29, Henry Wang wrote:
>>>> +        /*
>>>> +         * Handle the LR where the physical interrupt is 
>>>> de-assigned from the
>>>> +         * guest before it was EOIed
>>>> +         */
>>>> +        struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], 
>>>> virq);
>>>
>>> This will return a vCPU from the current affinity. This may not be 
>>> where the interrupt was injected. From a brief look, I can't tell 
>>> whether we have an easy way to know where the interrupt was injected 
>>> (other than the pending_irq is in the list lr_queue/inflight)
>>
>> I doubt if we need to handle more than this - I think if the 
>> pending_irq is not in the lr_queue/inflight list, it would not belong 
>> to the corner case we are talking about (?).
>
> I didn't suggest we would need to handle the case where the 
> pending_irq is not any of the queues. I was pointing out that I think 
> we don't directly store the vCPU ID where we injected the IRQ. 
> Instead, the pending_irq is just in list, so we will possibly need to 
> store the vCPU ID for convenience.

Sorry for misunderstanding. Yeah you are definitely correct. Also thank 
you so much for the suggestion! Before seeing this suggestion, I was 
struggling in finding the correct vCPU by "for_each_vcpus" and 
comparison... but now I realized your suggestion is way more clever :)

Kind regards,
Henry

[PATCH 03/15] xen/arm: Always enable IOMMU

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

For overlay with iommu functionality to work with running VMs, we need to enable
IOMMU by default for the domains.

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/dom0less-build.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index fb63ec6fd1..2d1fd1e214 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -894,7 +894,8 @@ void __init create_domUs(void)
             panic("Missing property 'cpus' for domain %s\n",
                   dt_node_name(node));
 
-        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
+        if ( (IS_ENABLED(CONFIG_OVERLAY_DTB) ||
+              dt_find_compatible_node(node, NULL, "multiboot,device-tree")) &&
              iommu_enabled )
             d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
-- 
2.34.1

Re: [PATCH 03/15] xen/arm: Always enable IOMMU

[Julien Grall]

Hi Henry,

On 24/04/2024 04:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
> 
> For overlay with iommu functionality to work with running VMs, we need to enable
> IOMMU by default for the domains.
> 
> 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/dom0less-build.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index fb63ec6fd1..2d1fd1e214 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -894,7 +894,8 @@ void __init create_domUs(void)
>               panic("Missing property 'cpus' for domain %s\n",
>                     dt_node_name(node));
>   
> -        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
> +        if ( (IS_ENABLED(CONFIG_OVERLAY_DTB) ||

Similar to the first patch, building Xen with the DTB overlay doesn't 
mean the user will want to use it (think of distros that may want to 
provide a generic Xen).

Instead, we should introduce a new DT property "passthrough" that would 
indicate whether the IOMMU should be used.

To be futureproof, I would match the values used by xl.cfg (see 
docs/man/xl.cfg.5.pod.in).

> +              dt_find_compatible_node(node, NULL, "multiboot,device-tree")) &&
>                iommu_enabled )
>               d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>   

Cheers,

-- 
Julien Grall

Re: [PATCH 03/15] xen/arm: Always enable IOMMU

[Henry Wang]

Hi Julien,

On 4/24/2024 9:03 PM, Julien Grall wrote:
> Hi Henry,
>
> On 24/04/2024 04:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> For overlay with iommu functionality to work with running VMs, we 
>> need to enable
>> IOMMU by default for the domains.
>>
>> 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/dom0less-build.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c 
>> b/xen/arch/arm/dom0less-build.c
>> index fb63ec6fd1..2d1fd1e214 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -894,7 +894,8 @@ void __init create_domUs(void)
>>               panic("Missing property 'cpus' for domain %s\n",
>>                     dt_node_name(node));
>>   -        if ( dt_find_compatible_node(node, NULL, 
>> "multiboot,device-tree") &&
>> +        if ( (IS_ENABLED(CONFIG_OVERLAY_DTB) ||
>
> Similar to the first patch, building Xen with the DTB overlay doesn't 
> mean the user will want to use it (think of distros that may want to 
> provide a generic Xen).
>
> Instead, we should introduce a new DT property "passthrough" that 
> would indicate whether the IOMMU should be used.
>
> To be futureproof, I would match the values used by xl.cfg (see 
> docs/man/xl.cfg.5.pod.in).

That sounds good. I can introduce a new DT property as suggested. Thanks 
for the suggestion!

Kind regards,
Henry

>
>> + dt_find_compatible_node(node, NULL, "multiboot,device-tree")) &&
>>                iommu_enabled )
>>               d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> Cheers,
>

[PATCH 04/15] tools/libs/light: Always enable IOMMU

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

For overlay with iommu functionality to work with running VMs, we need
to enable IOMMU when iomem presents for the domains.

Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 tools/libs/light/libxl_arm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 1cb89fa584..dd5c9f4917 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
     }
 
+#ifdef LIBXL_HAVE_DT_OVERLAY
+    if (d_config->b_info.num_iomem) {
+        config->flags |= XEN_DOMCTL_CDF_iommu;
+    }
+#endif
+
     return 0;
 }
 
-- 
2.34.1

Re: [PATCH 04/15] tools/libs/light: Always enable IOMMU

[Anthony PERARD]

On Wed, Apr 24, 2024 at 11:34:38AM +0800, Henry Wang wrote:
> For overlay with iommu functionality to work with running VMs, we need
> to enable IOMMU when iomem presents for the domains.
> 
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
>  tools/libs/light/libxl_arm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 1cb89fa584..dd5c9f4917 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>          config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
>      }
>  
> +#ifdef LIBXL_HAVE_DT_OVERLAY

libxl_arm.c is only build on Arm, so this should be defined, so no need
to check.

> +    if (d_config->b_info.num_iomem) {
> +        config->flags |= XEN_DOMCTL_CDF_iommu;

Is this doing the same thing as the previous patch?

Thanks,

-- 
Anthony PERARD

Re: [PATCH 04/15] tools/libs/light: Always enable IOMMU

[Henry Wang]

Hi Anthony,

On 5/1/2024 9:47 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:38AM +0800, Henry Wang wrote:
>> For overlay with iommu functionality to work with running VMs, we need
>> to enable IOMMU when iomem presents for the domains.
>>
>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>>   tools/libs/light/libxl_arm.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 1cb89fa584..dd5c9f4917 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>           config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
>>       }
>>   
>> +#ifdef LIBXL_HAVE_DT_OVERLAY
> libxl_arm.c is only build on Arm, so this should be defined, so no need
> to check.

Ah sure, I was just thought in the future RISC-V/PPC may have the same, 
but you are correct. I will remove the check.

>> +    if (d_config->b_info.num_iomem) {
>> +        config->flags |= XEN_DOMCTL_CDF_iommu;
> Is this doing the same thing as the previous patch?

I think so, yes, we need the IOMMU flag to be set if we want to assign a 
device from a DT node protected by IOMMU.

Kind regards,
Henry

>
> Thanks,
>

[PATCH 05/15] tools/libs/light: Increase nr_spi to 160

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32.
This was done to allocate and assign IRQs to a running 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>
---
 tools/libs/light/libxl_arm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index dd5c9f4917..50dbd0f2a9 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
 
     LOG(DEBUG, "Configure the domain");
 
-    config->arch.nr_spis = nr_spis;
+    /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */
+    config->arch.nr_spis = MAX(nr_spis, 160);
     LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
 
     switch (d_config->b_info.arch_arm.gic_version) {
-- 
2.34.1

Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160

[Anthony PERARD]

On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32.
> This was done to allocate and assign IRQs to a running 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>
> ---
>  tools/libs/light/libxl_arm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index dd5c9f4917..50dbd0f2a9 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>  
>      LOG(DEBUG, "Configure the domain");
>  
> -    config->arch.nr_spis = nr_spis;
> +    /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */
> +    config->arch.nr_spis = MAX(nr_spis, 160);

Is there a way that that Xen or libxl could find out what the minimum
number of SPI needs to be? Are we going to have to increase that minimum
number every time a new platform comes along?

It doesn't appear that libxl is using that `nr_spis` value and it is
probably just given to Xen. So my guess is that Xen could simply take
care of the minimum value, gic_number_lines() seems to be a Xen
function.

Thanks,

-- 
Anthony PERARD

Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160

[Henry Wang]

Hi Anthony,

(+Arm maintainers)

On 5/1/2024 9:58 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32.
>> This was done to allocate and assign IRQs to a running 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>
>> ---
>>   tools/libs/light/libxl_arm.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index dd5c9f4917..50dbd0f2a9 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>   
>>       LOG(DEBUG, "Configure the domain");
>>   
>> -    config->arch.nr_spis = nr_spis;
>> +    /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */
>> +    config->arch.nr_spis = MAX(nr_spis, 160);
> Is there a way that that Xen or libxl could find out what the minimum
> number of SPI needs to be?

I am afraid currently there is none.

> Are we going to have to increase that minimum
> number every time a new platform comes along?
>
> It doesn't appear that libxl is using that `nr_spis` value and it is
> probably just given to Xen. So my guess is that Xen could simply take
> care of the minimum value, gic_number_lines() seems to be a Xen
> function.

Xen will take care of the value of nr_spis for dom0 in create_dom0()
dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
and also for dom0less domUs in create_domUs().

However, it looks like Xen will not take care of the mininum value for 
libxl guests, the value from config->arch.nr_spis in guest config file 
will be directly passed to the domain_vgic_init() function from 
arch_domain_create().

I agree with you that we shouldn't just bump the number everytime when 
we have a new platform. Therefore, would it be a good idea to move the 
logic in this patch to arch_sanitise_domain_config()?

Kind regards,
Henry

>
> Thanks,
>

Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160

[Julien Grall]

Hi,

On 06/05/2024 06:17, Henry Wang wrote:
> On 5/1/2024 9:58 PM, Anthony PERARD wrote:
>> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
>>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx 
>>> ZynqMP - 32.
>>> This was done to allocate and assign IRQs to a running 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>
>>> ---
>>>   tools/libs/light/libxl_arm.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>> index dd5c9f4917..50dbd0f2a9 100644
>>> --- a/tools/libs/light/libxl_arm.c
>>> +++ b/tools/libs/light/libxl_arm.c
>>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>>       LOG(DEBUG, "Configure the domain");
>>> -    config->arch.nr_spis = nr_spis;
>>> +    /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 
>>> 192 - 32. */
>>> +    config->arch.nr_spis = MAX(nr_spis, 160);
>> Is there a way that that Xen or libxl could find out what the minimum
>> number of SPI needs to be?
> 
> I am afraid currently there is none.
> 
>> Are we going to have to increase that minimum
>> number every time a new platform comes along?
>>
>> It doesn't appear that libxl is using that `nr_spis` value and it is
>> probably just given to Xen. So my guess is that Xen could simply take
>> care of the minimum value, gic_number_lines() seems to be a Xen
>> function.
> 
> Xen will take care of the value of nr_spis for dom0 in create_dom0()
> dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
> and also for dom0less domUs in create_domUs().
> 
> However, it looks like Xen will not take care of the mininum value for 
> libxl guests, the value from config->arch.nr_spis in guest config file 
> will be directly passed to the domain_vgic_init() function from 
> arch_domain_create().
> 
> I agree with you that we shouldn't just bump the number everytime when 
> we have a new platform. Therefore, would it be a good idea to move the 
> logic in this patch to arch_sanitise_domain_config()?

Xen domains are supposed to be platform agnostics and therefore the 
numbers of SPIs should not be based on the HW.

Furthermore, with your proposal we would end up to allocate data 
structure for N SPIs when a domain may never needs any SPIs (such as if 
passthrough is not in-use). This is more likely for domain created by 
the toolstack than from Xen directly.

Instead, we should introduce a new XL configuration to let the user 
decide the number of SPIs. I would suggest to name "nr_spis" to match 
the DT bindings.

Cheers,

-- 
Julien Grall

Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160

[Henry Wang]

Hi Julien,

On 5/7/2024 10:35 PM, Julien Grall wrote:
> Hi,
>
> On 06/05/2024 06:17, Henry Wang wrote:
>> On 5/1/2024 9:58 PM, Anthony PERARD wrote:
>>> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
>>>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx 
>>>> ZynqMP - 32.
>>>> This was done to allocate and assign IRQs to a running 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>
>>>> ---
>>>>   tools/libs/light/libxl_arm.c | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/libs/light/libxl_arm.c 
>>>> b/tools/libs/light/libxl_arm.c
>>>> index dd5c9f4917..50dbd0f2a9 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc 
>>>> *gc,
>>>>       LOG(DEBUG, "Configure the domain");
>>>> -    config->arch.nr_spis = nr_spis;
>>>> +    /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 
>>>> 192 - 32. */
>>>> +    config->arch.nr_spis = MAX(nr_spis, 160);
>>> Is there a way that that Xen or libxl could find out what the minimum
>>> number of SPI needs to be?
>>
>> I am afraid currently there is none.
>>
>>> Are we going to have to increase that minimum
>>> number every time a new platform comes along?
>>>
>>> It doesn't appear that libxl is using that `nr_spis` value and it is
>>> probably just given to Xen. So my guess is that Xen could simply take
>>> care of the minimum value, gic_number_lines() seems to be a Xen
>>> function.
>>
>> Xen will take care of the value of nr_spis for dom0 in create_dom0()
>> dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 
>> 32;
>> and also for dom0less domUs in create_domUs().
>>
>> However, it looks like Xen will not take care of the mininum value 
>> for libxl guests, the value from config->arch.nr_spis in guest config 
>> file will be directly passed to the domain_vgic_init() function from 
>> arch_domain_create().
>>
>> I agree with you that we shouldn't just bump the number everytime 
>> when we have a new platform. Therefore, would it be a good idea to 
>> move the logic in this patch to arch_sanitise_domain_config()?
>
> Xen domains are supposed to be platform agnostics and therefore the 
> numbers of SPIs should not be based on the HW.
>
> Furthermore, with your proposal we would end up to allocate data 
> structure for N SPIs when a domain may never needs any SPIs (such as 
> if passthrough is not in-use). This is more likely for domain created 
> by the toolstack than from Xen directly.

Agreed on both comments.

> Instead, we should introduce a new XL configuration to let the user 
> decide the number of SPIs. I would suggest to name "nr_spis" to match 
> the DT bindings.

Sure, I will introduce a new xl config for this to replace this patch. 
Thank you for the suggestion.

Kind regards,
Henry

>
> Cheers,
>

[PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile

[Henry Wang]

From: Vikram Garhwal <vikram.garhwal@amd.com>

Move struct range, rangeset and removed static from first_range and next_range().
IRQs and IOMEMs for nodes are stored as rangeset in the dynamic node addition
part. While removing the nodes we need to access every IRQ and IOMEM ranges to
unmap IRQ and IOMEM from the domain.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 xen/common/rangeset.c      | 31 ++-----------------------------
 xen/include/xen/rangeset.h | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index b75590f907..d3f4297e41 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -12,31 +12,6 @@
 #include <xen/rangeset.h>
 #include <xsm/xsm.h>
 
-/* An inclusive range [s,e] and pointer to next range in ascending order. */
-struct range {
-    struct list_head list;
-    unsigned long s, e;
-};
-
-struct rangeset {
-    /* Owning domain and threaded list of rangesets. */
-    struct list_head rangeset_list;
-    struct domain   *domain;
-
-    /* Ordered list of ranges contained in this set, and protecting lock. */
-    struct list_head range_list;
-
-    /* Number of ranges that can be allocated */
-    long             nr_ranges;
-    rwlock_t         lock;
-
-    /* Pretty-printing name. */
-    char             name[32];
-
-    /* RANGESETF flags. */
-    unsigned int     flags;
-};
-
 /*****************************
  * Private range functions hide the underlying linked-list implemnetation.
  */
@@ -57,8 +32,7 @@ static struct range *find_range(
     return x;
 }
 
-/* Return the lowest range in the set r, or NULL if r is empty. */
-static struct range *first_range(
+struct range *first_range(
     struct rangeset *r)
 {
     if ( list_empty(&r->range_list) )
@@ -66,8 +40,7 @@ static struct range *first_range(
     return list_entry(r->range_list.next, struct range, list);
 }
 
-/* Return range following x in ascending order, or NULL if x is the highest. */
-static struct range *next_range(
+struct range *next_range(
     struct rangeset *r, struct range *x)
 {
     if ( x->list.next == &r->range_list )
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 96c9180825..cd80fd9179 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -13,7 +13,37 @@
 #include <xen/types.h>
 
 struct domain;
-struct rangeset;
+
+struct rangeset {
+    /* Owning domain and threaded list of rangesets. */
+    struct list_head rangeset_list;
+    struct domain   *domain;
+
+    /* Ordered list of ranges contained in this set, and protecting lock. */
+    struct list_head range_list;
+
+    /* Number of ranges that can be allocated */
+    long             nr_ranges;
+    rwlock_t         lock;
+
+    /* Pretty-printing name. */
+    char             name[32];
+
+    /* RANGESETF flags. */
+    unsigned int     flags;
+};
+
+/* An inclusive range [s,e] and pointer to next range in ascending order. */
+struct range {
+    struct list_head list;
+    unsigned long s, e;
+};
+
+/* Return the lowest range in the set r, or NULL if r is empty. */
+struct range *first_range(struct rangeset *r);
+
+/* Return range following x in ascending order, or NULL if x is the highest. */
+struct range *next_range(struct rangeset *r, struct range *x);
 
 /*
  * Initialise/destroy per-domain rangeset information.
-- 
2.34.1

Re: [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile

[Jan Beulich]

On 24.04.2024 05:34, Henry Wang wrote:
> From: Vikram Garhwal <vikram.garhwal@amd.com>
> 
> Move struct range, rangeset and removed static from first_range and next_range().

NAK, for going against what we do elsewhere (limiting exposure of internals).
At least as long as the justification isn't any better than ...

> IRQs and IOMEMs for nodes are stored as rangeset in the dynamic node addition
> part. While removing the nodes we need to access every IRQ and IOMEM ranges to
> unmap IRQ and IOMEM from the domain.

... this. You're aware of rangeset_report_ranges() and rangeset_consume_ranges(),
aren't you? If neither is suitable for your purpose, can you explain what you
need in addition?

Jan

Re: [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile

[Henry Wang]

Hi Jan,

On 4/24/2024 2:22 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> From: Vikram Garhwal <vikram.garhwal@amd.com>
>>
>> Move struct range, rangeset and removed static from first_range and next_range().
> NAK, for going against what we do elsewhere (limiting exposure of internals).
> At least as long as the justification isn't any better than ...
>
>> IRQs and IOMEMs for nodes are stored as rangeset in the dynamic node addition
>> part. While removing the nodes we need to access every IRQ and IOMEM ranges to
>> unmap IRQ and IOMEM from the domain.
> ... this. You're aware of rangeset_report_ranges() and rangeset_consume_ranges(),
> aren't you? If neither is suitable for your purpose, can you explain what you
> need in addition?

I understand your concern. I will check if I can refactor this patch 
using the suggested helpers. Thanks!

Kind regards,
Henry


>
> Jan

[PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Following changes are done to enable dtbo assignment to a running vm with given
domain_id:
1. Support for irq map and unmap for running domain. We store the IRQ nums for
    each overlay and while removing those IRQ nums are removed.
2. Support iommu assign/reassign to running domains.
3. Added "map_nodes" options to support two modes:
    a. Add the nodes in Xen device tree and map the nodes to given VM
    b. Just add them in xen device tree without mapping.

Change device.c: handle_device() function with input domain_mapping flag.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 xen/arch/arm/device.c            |   8 +-
 xen/arch/arm/domain_build.c      |   2 +-
 xen/arch/arm/include/asm/setup.h |   3 +-
 xen/common/dt-overlay.c          | 218 ++++++++++++++++++++++++++-----
 xen/include/public/sysctl.h      |   4 +-
 5 files changed, 199 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 3e02cff008..df5035f9a8 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -317,7 +317,8 @@ static int map_device_children(const struct dt_device_node *dev,
  *  - Map the IRQs and iomem regions to DOM0
  */
 int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
-                  struct rangeset *iomem_ranges, struct rangeset *irq_ranges)
+                  struct rangeset *iomem_ranges, struct rangeset *irq_ranges,
+                  bool device_mapping)
 {
     unsigned int naddr;
     unsigned int i;
@@ -334,9 +335,10 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
     struct map_range_data mr_data = {
         .d = d,
         .p2mt = p2mt,
-        .skip_mapping = !own_device ||
+        .skip_mapping = (!own_device ||
                         (is_pci_passthrough_enabled() &&
-                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
+                        (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))) &&
+                        !device_mapping,
         .iomem_ranges = iomem_ranges,
         .irq_ranges = irq_ranges
     };
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 54232ed4cb..a525aed175 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1699,7 +1699,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
                "WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
                path);
 
-    res = handle_device(d, node, p2mt, NULL, NULL);
+    res = handle_device(d, node, p2mt, NULL, NULL, false);
     if ( res)
         return res;
 
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index d15a88d2e0..d6d8c28d02 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -172,7 +172,8 @@ u32 device_tree_get_u32(const void *fdt, int node,
                         const char *prop_name, u32 dflt);
 
 int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
-                  struct rangeset *iomem_ranges, struct rangeset *irq_ranges);
+                  struct rangeset *iomem_ranges, struct rangeset *irq_ranges,
+                  bool device_mapping);
 
 int map_device_irqs_to_domain(struct domain *d, struct dt_device_node *dev,
                               bool need_mapping, struct rangeset *irq_ranges);
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 39e4ba59dd..8840ea756b 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -356,24 +356,133 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
     return 0;
 }
 
+static int remove_all_irqs(struct rangeset *irq_ranges,
+                           struct domain *d, bool domain_mapping)
+{
+    struct range *x;
+    int rc = 0;
+
+    read_lock(&irq_ranges->lock);
+
+    for ( x = first_range(irq_ranges); x != NULL;
+          x = next_range(irq_ranges, x) )
+    {
+        /*
+         * Handle invalid use cases 1:
+         * Where user assigned the nodes to dom0 along with their irq
+         * mappings but now just wants to remove the nodes entry from Xen device
+         * device tree without unmapping the irq.
+         */
+        if ( !domain_mapping && vgic_get_hw_irq_desc(d, NULL, x->s) )
+        {
+            printk(XENLOG_ERR "Removing node from device tree without releasing it's IRQ/IOMMU is not allowed\n");
+            rc = -EINVAL;
+            goto out;
+        }
+
+        /*
+         * IRQ should always have access unless there are duplication of
+         * of irqs in device tree. There are few cases of xen device tree
+         * where there are duplicate interrupts for the same node.
+         */
+        if (!irq_access_permitted(d, x->s))
+            continue;
+        /*
+         * TODO: We don't handle shared IRQs for now. So, it is assumed that
+         * the IRQs was not shared with another domain.
+         */
+        rc = irq_deny_access(d, x->s);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "unable to revoke access for irq %ld\n", x->s);
+            goto out;
+        }
+
+        if ( domain_mapping )
+        {
+            rc = release_guest_irq(d, x->s);
+            if ( rc )
+            {
+                printk(XENLOG_ERR "unable to release irq %ld\n", x->s);
+                goto out;
+            }
+        }
+    }
+
+out:
+    read_unlock(&irq_ranges->lock);
+    return rc;
+}
+
+static int remove_all_iomems(struct rangeset *iomem_ranges,
+                             struct domain *d,
+                             bool domain_mapping)
+{
+    struct range *x;
+    int rc = 0;
+    p2m_type_t t;
+    mfn_t mfn;
+
+    read_lock(&iomem_ranges->lock);
+
+    for ( x = first_range(iomem_ranges); x != NULL;
+          x = next_range(iomem_ranges, x) )
+    {
+        mfn = p2m_lookup(d, _gfn(x->s), &t);
+        if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )
+        {
+            rc = -EINVAL;
+            goto out;
+        }
+
+        rc = iomem_deny_access(d, x->s, x->e);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "Unable to remove dom%d access to %#lx - %#lx\n",
+                   d->domain_id,
+                   x->s, x->e);
+            goto out;
+        }
+
+        rc = unmap_mmio_regions(d, _gfn(x->s), x->e - x->s,
+                                _mfn(x->s));
+        if ( rc )
+            goto out;
+    }
+
+out:
+    read_unlock(&iomem_ranges->lock);
+    return rc;
+}
+
 /* Check if node itself can be removed and remove node from IOMMU. */
-static int remove_node_resources(struct dt_device_node *device_node)
+static int remove_node_resources(struct dt_device_node *device_node,
+                                 struct domain *d, bool domain_mapping)
 {
     int rc = 0;
     unsigned int len;
     domid_t domid;
 
-    domid = dt_device_used_by(device_node);
+    if ( !d )
+    {
+        domid = dt_device_used_by(device_node);
 
-    dt_dprintk("Checking if node %s is used by any domain\n",
-               device_node->full_name);
+        /*
+         * We also check if device is assigned to DOMID_IO as when a domain
+         * is destroyed device is assigned to DOMID_IO or for the case when
+         * device was never mapped to a running domain.
+         */
+        if ( domid != hardware_domain->domain_id && domid != DOMID_IO )
+        {
+            printk(XENLOG_ERR "Device %s is being assigned to %u. Device is assigned to %d\n",
+                   device_node->full_name, DOMID_IO, domid);
+            return -EINVAL;
+        }
 
-    /* Remove the node if only it's assigned to hardware domain or domain io. */
-    if ( domid != hardware_domain->domain_id && domid != DOMID_IO )
-    {
-        printk(XENLOG_ERR "Device %s is being used by domain %u. Removing nodes failed\n",
-               device_node->full_name, domid);
-        return -EINVAL;
+        /*
+         * Device is assigned to hardware domain.
+         */
+         d = hardware_domain;
     }
 
     /* Check if iommu property exists. */
@@ -382,6 +491,16 @@ static int remove_node_resources(struct dt_device_node *device_node)
         if ( dt_device_is_protected(device_node) )
         {
             write_lock(&dt_host_lock);
+            if ( domain_mapping && !list_empty(&device_node->domain_list) )
+            {
+                rc = iommu_deassign_dt_device(d, device_node);
+                if ( rc < 0 )
+                {
+                    write_unlock(&dt_host_lock);
+                    return rc;
+                }
+            }
+
             rc = iommu_remove_dt_device(device_node);
             if ( rc < 0 )
             {
@@ -397,7 +516,8 @@ static int remove_node_resources(struct dt_device_node *device_node)
 
 /* Remove all descendants from IOMMU. */
 static int
-remove_descendant_nodes_resources(const struct dt_device_node *device_node)
+remove_descendant_nodes_resources(const struct dt_device_node *device_node,
+                                  struct domain *d, bool domain_mapping)
 {
     int rc = 0;
     struct dt_device_node *child_node;
@@ -407,12 +527,13 @@ remove_descendant_nodes_resources(const struct dt_device_node *device_node)
     {
         if ( child_node->child )
         {
-            rc = remove_descendant_nodes_resources(child_node);
+            rc = remove_descendant_nodes_resources(child_node, d,
+                                                   domain_mapping);
             if ( rc )
                 return rc;
         }
 
-        rc = remove_node_resources(child_node);
+        rc = remove_node_resources(child_node, d, domain_mapping);
         if ( rc )
             return rc;
     }
@@ -421,12 +542,13 @@ remove_descendant_nodes_resources(const struct dt_device_node *device_node)
 }
 
 /* Remove nodes from dt_host. */
-static int remove_nodes(const struct overlay_track *tracker)
+static int remove_nodes(const struct overlay_track *tracker,
+                        struct domain *d)
 {
     int rc = 0;
     struct dt_device_node *overlay_node;
     unsigned int j;
-    struct domain *d = hardware_domain;
+    bool domain_mapping = (d != NULL);
 
     for ( j = 0; j < tracker->num_nodes; j++ )
     {
@@ -434,11 +556,19 @@ static int remove_nodes(const struct overlay_track *tracker)
         if ( overlay_node == NULL )
             return -EINVAL;
 
-        rc = remove_descendant_nodes_resources(overlay_node);
+        rc = remove_descendant_nodes_resources(overlay_node, d, domain_mapping);
+        if ( rc )
+            return rc;
+
+        rc = remove_node_resources(overlay_node, d, domain_mapping);
+        if ( rc )
+            return rc;
+
+        rc = remove_all_irqs(tracker->irq_ranges, d, domain_mapping);
         if ( rc )
             return rc;
 
-        rc = remove_node_resources(overlay_node);
+        rc = remove_all_iomems(tracker->iomem_ranges, d, domain_mapping);
         if ( rc )
             return rc;
 
@@ -481,7 +611,8 @@ static int remove_nodes(const struct overlay_track *tracker)
  * while destroying the domain.
  */
 static long handle_remove_overlay_nodes(const void *overlay_fdt,
-                                        uint32_t overlay_fdt_size)
+                                        uint32_t overlay_fdt_size,
+                                        struct domain *d)
 {
     int rc;
     struct overlay_track *entry, *temp, *track;
@@ -520,7 +651,7 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
 
     }
 
-    rc = remove_nodes(entry);
+    rc = remove_nodes(entry, d);
     if ( rc )
     {
         printk(XENLOG_ERR "Removing node failed\n");
@@ -560,12 +691,21 @@ static void free_nodes_full_path(unsigned int num_nodes, char **nodes_full_path)
     xfree(nodes_full_path);
 }
 
-static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
+static long add_nodes(struct overlay_track *tr, char **nodes_full_path,
+                      struct domain *d)
 
 {
     int rc;
     unsigned int j;
     struct dt_device_node *overlay_node;
+    bool domain_mapping = (d != NULL);
+
+    /*
+     * If domain is NULL, then we add the devices into hardware domain and skip
+     * IRQs/IOMMUs mapping.
+     */
+    if ( d == NULL )
+        d = hardware_domain;
 
     for ( j = 0; j < tr->num_nodes; j++ )
     {
@@ -609,8 +749,6 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
             return rc;
         }
 
-        write_unlock(&dt_host_lock);
-
         prev_node->allnext = next_node;
 
         overlay_node = dt_find_node_by_path(overlay_node->full_name);
@@ -618,18 +756,23 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
         {
             /* Sanity check. But code will never come here. */
             ASSERT_UNREACHABLE();
+            write_unlock(&dt_host_lock);
             return -EFAULT;
         }
 
-        rc = handle_device(hardware_domain, overlay_node, p2m_mmio_direct_c,
+        rc = handle_device(d, overlay_node, p2m_mmio_direct_c,
                            tr->iomem_ranges,
-                           tr->irq_ranges);
+                           tr->irq_ranges,
+                           domain_mapping);
         if ( rc )
         {
             printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
+            write_unlock(&dt_host_lock);
             return rc;
         }
 
+        write_unlock(&dt_host_lock);
+
         /* Keep overlay_node address in tracker. */
         tr->nodes_address[j] = (unsigned long)overlay_node;
     }
@@ -643,7 +786,8 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
  * to hardware domain done by handle_node().
  */
 static long handle_add_overlay_nodes(void *overlay_fdt,
-                                     uint32_t overlay_fdt_size)
+                                     uint32_t overlay_fdt_size,
+                                     struct domain *d)
 {
     int rc;
     unsigned int j;
@@ -788,7 +932,7 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
         goto err;
     }
 
-    rc = add_nodes(tr, nodes_full_path);
+    rc = add_nodes(tr, nodes_full_path, d);
     if ( rc )
     {
         printk(XENLOG_ERR "Adding nodes failed. Removing the partially added nodes.\n");
@@ -810,7 +954,7 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
  */
  remove_node:
     tr->num_nodes = j;
-    rc = remove_nodes(tr);
+    rc = remove_nodes(tr, d);
 
     if ( rc )
     {
@@ -855,6 +999,7 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
 {
     long ret;
     void *overlay_fdt;
+    struct domain *d = NULL;
 
     if ( op->overlay_op != XEN_SYSCTL_DT_OVERLAY_ADD &&
          op->overlay_op != XEN_SYSCTL_DT_OVERLAY_REMOVE )
@@ -863,7 +1008,7 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
     if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
         return -EINVAL;
 
-    if ( op->pad[0] || op->pad[1] || op->pad[2] )
+    if ( op->pad[0] || op->pad[1] )
         return -EINVAL;
 
     overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
@@ -880,13 +1025,26 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
         return -EFAULT;
     }
 
+    /*
+     * If domain_mapping == false, domain_id can be ignored as we don't need to
+     * map resource to any domain.
+     *
+     * If domain_mapping == true, domain_id, get the target domain for the
+     * mapping.
+     */
+    if ( op->domain_mapping )
+        d = rcu_lock_domain_by_id(op->domain_id);
+
     if ( op->overlay_op == XEN_SYSCTL_DT_OVERLAY_REMOVE )
-        ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+        ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size, d);
     else
-        ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+        ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size, d);
 
     xfree(overlay_fdt);
 
+    if ( op->domain_mapping )
+        rcu_unlock_domain(d);
+
     return ret;
 }
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9b19679cae..f5435f8890 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
 #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
 #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
     uint8_t overlay_op;                     /* IN: Add or remove. */
-    uint8_t pad[3];                         /* IN: Must be zero. */
+    bool domain_mapping;                    /* IN: True of False. */
+    uint8_t pad[2];                         /* IN: Must be zero. */
+    uint32_t domain_id;
 };
 #endif
 
-- 
2.34.1

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Jan Beulich]

On 24.04.2024 05:34, Henry Wang wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>  #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>  #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>      uint8_t overlay_op;                     /* IN: Add or remove. */
> -    uint8_t pad[3];                         /* IN: Must be zero. */
> +    bool domain_mapping;                    /* IN: True of False. */
> +    uint8_t pad[2];                         /* IN: Must be zero. */
> +    uint32_t domain_id;
>  };

If you merely re-purposed padding fields, all would be fine without
bumping the interface version. Yet you don't, albeit for an unclear
reason: Why uint32_t rather than domid_t? And on top of that - why a
separate boolean when you could use e.g. DOMID_INVALID to indicate
"no domain mapping"?

That said - anything taking a domain ID is certainly suspicious in a
sysctl. Judging from the description you really mean this to be a
domctl. Anything else will require extra justification.

Jan

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Henry Wang]

Hi Jan, Julien, Stefano,

On 4/24/2024 2:05 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>>   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>>       uint8_t overlay_op;                     /* IN: Add or remove. */
>> -    uint8_t pad[3];                         /* IN: Must be zero. */
>> +    bool domain_mapping;                    /* IN: True of False. */
>> +    uint8_t pad[2];                         /* IN: Must be zero. */
>> +    uint32_t domain_id;
>>   };
> If you merely re-purposed padding fields, all would be fine without
> bumping the interface version. Yet you don't, albeit for an unclear
> reason: Why uint32_t rather than domid_t? And on top of that - why a
> separate boolean when you could use e.g. DOMID_INVALID to indicate
> "no domain mapping"?

I think both of your suggestion make great sense. I will follow the 
suggestion in v2.

> That said - anything taking a domain ID is certainly suspicious in a
> sysctl. Judging from the description you really mean this to be a
> domctl. Anything else will require extra justification.

I also think a domctl is better. I had a look at the history of the 
already merged series, it looks like in the first version of merged part 
1 [1], the hypercall was implemented as the domctl in the beginning but 
later in v2 changed to sysctl. I think this makes sense as the scope of 
that time is just to make Xen aware of the device tree node via Xen 
device tree.

However this is now a problem for the current part where the scope (and 
the end goal) is extended to assign the added device to Linux Dom0/DomU 
via device tree overlays. I am not sure which way is better, should we 
repurposing the sysctl to domctl or maybe add another domctl (I am 
worrying about the duplication because basically we need the same sysctl 
functionality but now with a domid in it)? What do you think?

@Stefano: Since I am not 100% if I understand the whole story behind 
this feature, would you mind checking if I am providing correct 
information above and sharing your opinions on this? Thank you very much!

[1] 
https://lore.kernel.org/xen-devel/13240b69-f7bb-6a64-b89c-b7c2cbb7e465@xen.org/

Kind regards,
Henry

> Jan

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Jan Beulich]

On 29.04.2024 05:36, Henry Wang wrote:
> Hi Jan, Julien, Stefano,
> 
> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>>>   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>>>       uint8_t overlay_op;                     /* IN: Add or remove. */
>>> -    uint8_t pad[3];                         /* IN: Must be zero. */
>>> +    bool domain_mapping;                    /* IN: True of False. */
>>> +    uint8_t pad[2];                         /* IN: Must be zero. */
>>> +    uint32_t domain_id;
>>>   };
>> If you merely re-purposed padding fields, all would be fine without
>> bumping the interface version. Yet you don't, albeit for an unclear
>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>> "no domain mapping"?
> 
> I think both of your suggestion make great sense. I will follow the 
> suggestion in v2.
> 
>> That said - anything taking a domain ID is certainly suspicious in a
>> sysctl. Judging from the description you really mean this to be a
>> domctl. Anything else will require extra justification.
> 
> I also think a domctl is better. I had a look at the history of the 
> already merged series, it looks like in the first version of merged part 
> 1 [1], the hypercall was implemented as the domctl in the beginning but 
> later in v2 changed to sysctl. I think this makes sense as the scope of 
> that time is just to make Xen aware of the device tree node via Xen 
> device tree.
> 
> However this is now a problem for the current part where the scope (and 
> the end goal) is extended to assign the added device to Linux Dom0/DomU 
> via device tree overlays. I am not sure which way is better, should we 
> repurposing the sysctl to domctl or maybe add another domctl (I am 
> worrying about the duplication because basically we need the same sysctl 
> functionality but now with a domid in it)? What do you think?

I'm taking it that what is a sysctl right now legitimately is. Therefore
folding both into domctl would at least be bending the rules of what
should be sysctl and what domctl. It would need clarifying what (fake)
domain such a (folded) domctl ought to operate on for the case that's
currently a sysctl. That then may (or may not) be justification for such
folding.

Jan

> @Stefano: Since I am not 100% if I understand the whole story behind 
> this feature, would you mind checking if I am providing correct 
> information above and sharing your opinions on this? Thank you very much!
> 
> [1] 
> https://lore.kernel.org/xen-devel/13240b69-f7bb-6a64-b89c-b7c2cbb7e465@xen.org/
> 
> Kind regards,
> Henry
> 
>> Jan
> 

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Julien Grall]

On 29/04/2024 04:36, Henry Wang wrote:
> Hi Jan, Julien, Stefano,

Hi Henry,

> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>>>   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>>>       uint8_t overlay_op;                     /* IN: Add or remove. */
>>> -    uint8_t pad[3];                         /* IN: Must be zero. */
>>> +    bool domain_mapping;                    /* IN: True of False. */
>>> +    uint8_t pad[2];                         /* IN: Must be zero. */
>>> +    uint32_t domain_id;
>>>   };
>> If you merely re-purposed padding fields, all would be fine without
>> bumping the interface version. Yet you don't, albeit for an unclear
>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>> "no domain mapping"?
> 
> I think both of your suggestion make great sense. I will follow the 
> suggestion in v2.
> 
>> That said - anything taking a domain ID is certainly suspicious in a
>> sysctl. Judging from the description you really mean this to be a
>> domctl. Anything else will require extra justification.
> 
> I also think a domctl is better. I had a look at the history of the 
> already merged series, it looks like in the first version of merged part 
> 1 [1], the hypercall was implemented as the domctl in the beginning but 
> later in v2 changed to sysctl. I think this makes sense as the scope of 
> that time is just to make Xen aware of the device tree node via Xen 
> device tree.
> 
> However this is now a problem for the current part where the scope (and 
> the end goal) is extended to assign the added device to Linux Dom0/DomU 
> via device tree overlays. I am not sure which way is better, should we 
> repurposing the sysctl to domctl or maybe add another domctl (I am 
> worrying about the duplication because basically we need the same sysctl 
> functionality but now with a domid in it)? What do you think?

I am not entirely sure this is a good idea to try to add the device in 
Xen and attach it to the guests at the same time. Imagine the following 
situation:

1) Add and attach devices
2) The domain is rebooted
3) Detach and remove devices

After step 2, you technically have a new domain. You could have also a 
case where this is a completely different guest. So the flow would look 
a little bit weird (you create the DT overlay with domain A but remove 
with domain B).

So, at the moment, it feels like the add/attach (resp detech/remove) 
operations should happen separately.

Can you clarify why you want to add devices to Xen and attach to a guest 
within a single hypercall?

Cheers,

-- 
Julien Grall

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Henry Wang]

Hi Julien,

On 4/30/2024 1:34 AM, Julien Grall wrote:
> On 29/04/2024 04:36, Henry Wang wrote:
>> Hi Jan, Julien, Stefano,
>
> Hi Henry,
>
>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>> --- a/xen/include/public/sysctl.h
>>>> +++ b/xen/include/public/sysctl.h
>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>>   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>>>>   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>>>>       uint8_t overlay_op;                     /* IN: Add or remove. */
>>>> -    uint8_t pad[3];                         /* IN: Must be zero. */
>>>> +    bool domain_mapping;                    /* IN: True of False. */
>>>> +    uint8_t pad[2];                         /* IN: Must be zero. */
>>>> +    uint32_t domain_id;
>>>>   };
>>> If you merely re-purposed padding fields, all would be fine without
>>> bumping the interface version. Yet you don't, albeit for an unclear
>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>> "no domain mapping"?
>>
>> I think both of your suggestion make great sense. I will follow the 
>> suggestion in v2.
>>
>>> That said - anything taking a domain ID is certainly suspicious in a
>>> sysctl. Judging from the description you really mean this to be a
>>> domctl. Anything else will require extra justification.
>>
>> I also think a domctl is better. I had a look at the history of the 
>> already merged series, it looks like in the first version of merged 
>> part 1 [1], the hypercall was implemented as the domctl in the 
>> beginning but later in v2 changed to sysctl. I think this makes sense 
>> as the scope of that time is just to make Xen aware of the device 
>> tree node via Xen device tree.
>>
>> However this is now a problem for the current part where the 
>> scope (and the end goal) is extended to assign the added device to 
>> Linux Dom0/DomU via device tree overlays. I am not sure which way is 
>> better, should we repurposing the sysctl to domctl or maybe add 
>> another domctl (I am worrying about the duplication because basically 
>> we need the same sysctl functionality but now with a domid in it)? 
>> What do you think?
>
> I am not entirely sure this is a good idea to try to add the device in 
> Xen and attach it to the guests at the same time. 
> Imagine the following situation:
>
> 1) Add and attach devices
> 2) The domain is rebooted
> 3) Detach and remove devices
>
> After step 2, you technically have a new domain. You could have also a 
> case where this is a completely different guest. So the flow would 
> look a little bit weird (you create the DT overlay with domain A but 
> remove with domain B).
>
> So, at the moment, it feels like the add/attach (resp detech/remove) 
> operations should happen separately.
>
> Can you clarify why you want to add devices to Xen and attach to a 
> guest within a single hypercall?

Sorry I don't know if there is any specific thoughts on the design of 
using a single hypercall to do both add devices to Xen device tree and 
assign the device to the guest. In fact seeing your above comments, I 
think separating these two functionality to two xl commands using 
separated hypercalls would indeed be a better idea. Thank you for the 
suggestion!

To make sure I understand correctly, would you mind confirming if below 
actions for v2 make sense to you? Thanks!
- Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove 
overlay to Xen device tree
- Introduce the xl dt-overlay attach <domid> command and respective 
domctls to do the device assignment for the overlay to domain.

Kind regards,
Henry

>
> Cheers,
>

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Julien Grall]

On 30/04/2024 05:00, Henry Wang wrote:
> Hi Julien,

Hi Henry,

> On 4/30/2024 1:34 AM, Julien Grall wrote:
>> On 29/04/2024 04:36, Henry Wang wrote:
>>> Hi Jan, Julien, Stefano,
>>
>> Hi Henry,
>>
>>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>>> --- a/xen/include/public/sysctl.h
>>>>> +++ b/xen/include/public/sysctl.h
>>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>>>   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>>>>>   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>>>>>       uint8_t overlay_op;                     /* IN: Add or remove. */
>>>>> -    uint8_t pad[3];                         /* IN: Must be zero. */
>>>>> +    bool domain_mapping;                    /* IN: True of False. */
>>>>> +    uint8_t pad[2];                         /* IN: Must be zero. */
>>>>> +    uint32_t domain_id;
>>>>>   };
>>>> If you merely re-purposed padding fields, all would be fine without
>>>> bumping the interface version. Yet you don't, albeit for an unclear
>>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>>> "no domain mapping"?
>>>
>>> I think both of your suggestion make great sense. I will follow the 
>>> suggestion in v2.
>>>
>>>> That said - anything taking a domain ID is certainly suspicious in a
>>>> sysctl. Judging from the description you really mean this to be a
>>>> domctl. Anything else will require extra justification.
>>>
>>> I also think a domctl is better. I had a look at the history of the 
>>> already merged series, it looks like in the first version of merged 
>>> part 1 [1], the hypercall was implemented as the domctl in the 
>>> beginning but later in v2 changed to sysctl. I think this makes sense 
>>> as the scope of that time is just to make Xen aware of the device 
>>> tree node via Xen device tree.
>>>
>>> However this is now a problem for the current part where the 
>>> scope (and the end goal) is extended to assign the added device to 
>>> Linux Dom0/DomU via device tree overlays. I am not sure which way is 
>>> better, should we repurposing the sysctl to domctl or maybe add 
>>> another domctl (I am worrying about the duplication because basically 
>>> we need the same sysctl functionality but now with a domid in it)? 
>>> What do you think?
>>
>> I am not entirely sure this is a good idea to try to add the device in 
>> Xen and attach it to the guests at the same time. Imagine the 
>> following situation:
>>
>> 1) Add and attach devices
>> 2) The domain is rebooted
>> 3) Detach and remove devices
>>
>> After step 2, you technically have a new domain. You could have also a 
>> case where this is a completely different guest. So the flow would 
>> look a little bit weird (you create the DT overlay with domain A but 
>> remove with domain B).
>>
>> So, at the moment, it feels like the add/attach (resp detech/remove) 
>> operations should happen separately.

Thinking a bit more about it, there is another problem with the single 
hypercall appproach. The MMIOs will be mapped 1:1 to the guest. These 
region may clash with other part of the layout for domain created by the 
toolstack
and dom0less (if the 1:1 option has not been enabled).

I guess for that add, it would be possible to specify the mapping in the 
Device-Tree. But that would not work for the removal (this may be a 
different domain).

On a somewhat similar topic, the number of IRQs supported by the vGIC is 
fixed at boot. How would that work with this patch?

>>
>> Can you clarify why you want to add devices to Xen and attach to a 
>> guest within a single hypercall?
> 
> Sorry I don't know if there is any specific thoughts on the design of 
> using a single hypercall to do both add devices to Xen device tree and 
> assign the device to the guest. In fact seeing your above comments, I 
> think separating these two functionality to two xl commands using 
> separated hypercalls would indeed be a better idea. Thank you for the 
> suggestion!
> 
> To make sure I understand correctly, would you mind confirming if below 
> actions for v2 make sense to you? Thanks!
> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove 
> overlay to Xen device tree

Note that this would attach the devices to dom0 first. Maybe this is why 
it was decided to merge the two operations? An option would be to allow 
the devices to be attached to no-one.

> - Introduce the xl dt-overlay attach <domid> command and respective 
> domctls to do the device assignment for the overlay to domain.

We already have domctls to route IRQs and map MMIOs. So do we actually 
need new domctls?

Cheers,

-- 
Julien Grall

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Henry Wang]

Hi Julien,

On 4/30/2024 5:47 PM, Julien Grall wrote:
> On 30/04/2024 05:00, Henry Wang wrote:
>> Hi Julien,
>
> Hi Henry,
>
>> On 4/30/2024 1:34 AM, Julien Grall wrote:
>>> On 29/04/2024 04:36, Henry Wang wrote:
>>>> Hi Jan, Julien, Stefano,
>>>
>>> Hi Henry,
>>>
>>>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>>>> --- a/xen/include/public/sysctl.h
>>>>>> +++ b/xen/include/public/sysctl.h
>>>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>>>>   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>>>>>>   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>>>>>>       uint8_t overlay_op;                     /* IN: Add or 
>>>>>> remove. */
>>>>>> -    uint8_t pad[3];                         /* IN: Must be zero. */
>>>>>> +    bool domain_mapping;                    /* IN: True of 
>>>>>> False. */
>>>>>> +    uint8_t pad[2];                         /* IN: Must be zero. */
>>>>>> +    uint32_t domain_id;
>>>>>>   };
>>>>> If you merely re-purposed padding fields, all would be fine without
>>>>> bumping the interface version. Yet you don't, albeit for an unclear
>>>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>>>> "no domain mapping"?
>>>>
>>>> I think both of your suggestion make great sense. I will follow the 
>>>> suggestion in v2.
>>>>
>>>>> That said - anything taking a domain ID is certainly suspicious in a
>>>>> sysctl. Judging from the description you really mean this to be a
>>>>> domctl. Anything else will require extra justification.
>>>>
>>>> I also think a domctl is better. I had a look at the history of the 
>>>> already merged series, it looks like in the first version of merged 
>>>> part 1 [1], the hypercall was implemented as the domctl in the 
>>>> beginning but later in v2 changed to sysctl. I think this makes 
>>>> sense as the scope of that time is just to make Xen aware of the 
>>>> device tree node via Xen device tree.
>>>>
>>>> However this is now a problem for the current part where the 
>>>> scope (and the end goal) is extended to assign the added device to 
>>>> Linux Dom0/DomU via device tree overlays. I am not sure which way 
>>>> is better, should we repurposing the sysctl to domctl or maybe add 
>>>> another domctl (I am worrying about the duplication because 
>>>> basically we need the same sysctl functionality but now with a 
>>>> domid in it)? What do you think?
>>>
>>> I am not entirely sure this is a good idea to try to add the device 
>>> in Xen and attach it to the guests at the same time. Imagine the 
>>> following situation:
>>>
>>> 1) Add and attach devices
>>> 2) The domain is rebooted
>>> 3) Detach and remove devices
>>>
>>> After step 2, you technically have a new domain. You could have also 
>>> a case where this is a completely different guest. So the flow would 
>>> look a little bit weird (you create the DT overlay with domain A but 
>>> remove with domain B).
>>>
>>> So, at the moment, it feels like the add/attach (resp detech/remove) 
>>> operations should happen separately.
>
> Thinking a bit more about it, there is another problem with the single 
> hypercall appproach. The MMIOs will be mapped 1:1 to the guest. These 
> region may clash with other part of the layout for domain created by 
> the toolstack
> and dom0less (if the 1:1 option has not been enabled).
>
> I guess for that add, it would be possible to specify the mapping in 
> the Device-Tree. But that would not work for the removal (this may be 
> a different domain).
>
> On a somewhat similar topic, the number of IRQs supported by the vGIC 
> is fixed at boot. How would that work with this patch?

Seeing your comment here I now realized patch #5 is to address this 
issue. But I think we need to have a complete rework of the original 
patch to make the feature portable. We can continue the discussion in 
patch 5.

>>>
>>> Can you clarify why you want to add devices to Xen and attach to a 
>>> guest within a single hypercall?
>>
>> Sorry I don't know if there is any specific thoughts on the design of 
>> using a single hypercall to do both add devices to Xen device tree 
>> and assign the device to the guest. In fact seeing your above 
>> comments, I think separating these two functionality to two xl 
>> commands using separated hypercalls would indeed be a better idea. 
>> Thank you for the suggestion!
>>
>> To make sure I understand correctly, would you mind confirming if 
>> below actions for v2 make sense to you? Thanks!
>> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to 
>> add/remove overlay to Xen device tree
>
> Note that this would attach the devices to dom0 first. Maybe this is 
> why it was decided to merge the two operations? An option would be to 
> allow the devices to be attached to no-one.
>
>> - Introduce the xl dt-overlay attach <domid> command and respective 
>> domctls to do the device assignment for the overlay to domain.
>
> We already have domctls to route IRQs and map MMIOs. So do we actually 
> need new domctls?

No I don't think so, like you and Stefano said in the other thread, I 
think I need to split the command to different hypercalls instead of 
only one hypercall and reuse the existing domctl.

Kind regards,
Henry

>
> Cheers,
>

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4302 bytes --]

On Tue, 30 Apr 2024, Henry Wang wrote:
> Hi Julien,
> 
> On 4/30/2024 1:34 AM, Julien Grall wrote:
> > On 29/04/2024 04:36, Henry Wang wrote:
> > > Hi Jan, Julien, Stefano,
> > 
> > Hi Henry,
> > 
> > > On 4/24/2024 2:05 PM, Jan Beulich wrote:
> > > > On 24.04.2024 05:34, Henry Wang wrote:
> > > > > --- a/xen/include/public/sysctl.h
> > > > > +++ b/xen/include/public/sysctl.h
> > > > > @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
> > > > >   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
> > > > >   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
> > > > >       uint8_t overlay_op;                     /* IN: Add or remove. */
> > > > > -    uint8_t pad[3];                         /* IN: Must be zero. */
> > > > > +    bool domain_mapping;                    /* IN: True of False. */
> > > > > +    uint8_t pad[2];                         /* IN: Must be zero. */
> > > > > +    uint32_t domain_id;
> > > > >   };
> > > > If you merely re-purposed padding fields, all would be fine without
> > > > bumping the interface version. Yet you don't, albeit for an unclear
> > > > reason: Why uint32_t rather than domid_t? And on top of that - why a
> > > > separate boolean when you could use e.g. DOMID_INVALID to indicate
> > > > "no domain mapping"?
> > > 
> > > I think both of your suggestion make great sense. I will follow the
> > > suggestion in v2.
> > > 
> > > > That said - anything taking a domain ID is certainly suspicious in a
> > > > sysctl. Judging from the description you really mean this to be a
> > > > domctl. Anything else will require extra justification.
> > > 
> > > I also think a domctl is better. I had a look at the history of the
> > > already merged series, it looks like in the first version of merged part 1
> > > [1], the hypercall was implemented as the domctl in the beginning but
> > > later in v2 changed to sysctl. I think this makes sense as the scope of
> > > that time is just to make Xen aware of the device tree node via Xen device
> > > tree.
> > > 
> > > However this is now a problem for the current part where the scope (and
> > > the end goal) is extended to assign the added device to Linux Dom0/DomU
> > > via device tree overlays. I am not sure which way is better, should we
> > > repurposing the sysctl to domctl or maybe add another domctl (I am
> > > worrying about the duplication because basically we need the same sysctl
> > > functionality but now with a domid in it)? What do you think?
> > 
> > I am not entirely sure this is a good idea to try to add the device in Xen
> > and attach it to the guests at the same time. Imagine the following
> > situation:
> > 
> > 1) Add and attach devices
> > 2) The domain is rebooted
> > 3) Detach and remove devices
> > 
> > After step 2, you technically have a new domain. You could have also a case
> > where this is a completely different guest. So the flow would look a little
> > bit weird (you create the DT overlay with domain A but remove with domain
> > B).
> > 
> > So, at the moment, it feels like the add/attach (resp detech/remove)
> > operations should happen separately.
> > 
> > Can you clarify why you want to add devices to Xen and attach to a guest
> > within a single hypercall?
> 
> Sorry I don't know if there is any specific thoughts on the design of using a
> single hypercall to do both add devices to Xen device tree and assign the
> device to the guest. In fact seeing your above comments, I think separating
> these two functionality to two xl commands using separated hypercalls would
> indeed be a better idea. Thank you for the suggestion!
> 
> To make sure I understand correctly, would you mind confirming if below
> actions for v2 make sense to you? Thanks!
> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove
> overlay to Xen device tree
> - Introduce the xl dt-overlay attach <domid> command and respective domctls to
> do the device assignment for the overlay to domain.

I think two hypercalls is OK. The original idea was to have a single xl
command to do the operation for user convenience (even that is not a
hard requirement) but that can result easily in two hypercalls.

Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Henry Wang]

Hi Stefano, Julien,

On 5/3/2024 2:02 AM, Stefano Stabellini wrote:
> On Tue, 30 Apr 2024, Henry Wang wrote:
>> Hi Julien,
>>
>> On 4/30/2024 1:34 AM, Julien Grall wrote:
>>> On 29/04/2024 04:36, Henry Wang wrote:
>>>> Hi Jan, Julien, Stefano,
>>> Hi Henry,
>>>
>>>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>>>> --- a/xen/include/public/sysctl.h
>>>>>> +++ b/xen/include/public/sysctl.h
>>>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>>>>    #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>>>>>>    #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>>>>>>        uint8_t overlay_op;                     /* IN: Add or remove. */
>>>>>> -    uint8_t pad[3];                         /* IN: Must be zero. */
>>>>>> +    bool domain_mapping;                    /* IN: True of False. */
>>>>>> +    uint8_t pad[2];                         /* IN: Must be zero. */
>>>>>> +    uint32_t domain_id;
>>>>>>    };
>>>>> If you merely re-purposed padding fields, all would be fine without
>>>>> bumping the interface version. Yet you don't, albeit for an unclear
>>>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>>>> "no domain mapping"?
>>>> I think both of your suggestion make great sense. I will follow the
>>>> suggestion in v2.
>>>>
>>>>> That said - anything taking a domain ID is certainly suspicious in a
>>>>> sysctl. Judging from the description you really mean this to be a
>>>>> domctl. Anything else will require extra justification.
>>>> I also think a domctl is better. I had a look at the history of the
>>>> already merged series, it looks like in the first version of merged part 1
>>>> [1], the hypercall was implemented as the domctl in the beginning but
>>>> later in v2 changed to sysctl. I think this makes sense as the scope of
>>>> that time is just to make Xen aware of the device tree node via Xen device
>>>> tree.
>>>>
>>>> However this is now a problem for the current part where the scope (and
>>>> the end goal) is extended to assign the added device to Linux Dom0/DomU
>>>> via device tree overlays. I am not sure which way is better, should we
>>>> repurposing the sysctl to domctl or maybe add another domctl (I am
>>>> worrying about the duplication because basically we need the same sysctl
>>>> functionality but now with a domid in it)? What do you think?
>>> I am not entirely sure this is a good idea to try to add the device in Xen
>>> and attach it to the guests at the same time. Imagine the following
>>> situation:
>>>
>>> 1) Add and attach devices
>>> 2) The domain is rebooted
>>> 3) Detach and remove devices
>>>
>>> After step 2, you technically have a new domain. You could have also a case
>>> where this is a completely different guest. So the flow would look a little
>>> bit weird (you create the DT overlay with domain A but remove with domain
>>> B).
>>>
>>> So, at the moment, it feels like the add/attach (resp detech/remove)
>>> operations should happen separately.
>>>
>>> Can you clarify why you want to add devices to Xen and attach to a guest
>>> within a single hypercall?
>> Sorry I don't know if there is any specific thoughts on the design of using a
>> single hypercall to do both add devices to Xen device tree and assign the
>> device to the guest. In fact seeing your above comments, I think separating
>> these two functionality to two xl commands using separated hypercalls would
>> indeed be a better idea. Thank you for the suggestion!
>>
>> To make sure I understand correctly, would you mind confirming if below
>> actions for v2 make sense to you? Thanks!
>> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove
>> overlay to Xen device tree
>> - Introduce the xl dt-overlay attach <domid> command and respective domctls to
>> do the device assignment for the overlay to domain.
> I think two hypercalls is OK. The original idea was to have a single xl
> command to do the operation for user convenience (even that is not a
> hard requirement) but that can result easily in two hypercalls.

Ok, sounds good. I will break the command to two hypercalls and try to 
reuse the existing domctls for assign/remove IRQ/MMIO ranges.

Kind regards,
Henry

[PATCH 08/15] tools: Add domain_id and expert mode for overlay operations

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Add domain_id and expert mode for overlay assignment. This enables dynamic
programming of nodes during runtime.

Take the opportunity to fix the name mismatch in the xl command, the
command name should be "dt-overlay" instead of "dt_overlay".

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>
---
 tools/include/libxl.h               |  8 +++++--
 tools/include/xenctrl.h             |  5 +++--
 tools/libs/ctrl/xc_dt_overlay.c     |  7 ++++--
 tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++----
 tools/xl/xl_vmcontrol.c             | 34 ++++++++++++++++++++++++++---
 5 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 62cb07dea6..59a3e1b37c 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2549,8 +2549,12 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
 void libxl_device_pci_list_free(libxl_device_pci* list, int num);
 
 #if defined(__arm__) || defined(__aarch64__)
-int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
-                     uint32_t overlay_size, uint8_t overlay_op);
+#define LIBXL_DT_OVERLAY_ADD                   1
+#define LIBXL_DT_OVERLAY_REMOVE                2
+
+int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
+                     uint32_t overlay_size, uint8_t overlay_op, bool auto_mode,
+                     bool domain_mapping);
 #endif
 
 /*
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e054..3861d0a23f 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2654,8 +2654,9 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
                          xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
 
 #if defined(__arm__) || defined(__aarch64__)
-int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
-                  uint32_t overlay_fdt_size, uint8_t overlay_op);
+int xc_dt_overlay(xc_interface *xch, uint32_t domain_id, void *overlay_fdt,
+                  uint32_t overlay_fdt_size, uint8_t overlay_op,
+                  bool domain_mapping);
 #endif
 
 /* Compat shims */
diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
index c2224c4d15..a1dc549915 100644
--- a/tools/libs/ctrl/xc_dt_overlay.c
+++ b/tools/libs/ctrl/xc_dt_overlay.c
@@ -20,15 +20,18 @@
 
 #include "xc_private.h"
 
-int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
-                  uint32_t overlay_fdt_size, uint8_t overlay_op)
+int xc_dt_overlay(xc_interface *xch, uint32_t domid, void *overlay_fdt,
+                  uint32_t overlay_fdt_size, uint8_t overlay_op,
+                  bool domain_mapping)
 {
     int err;
     struct xen_sysctl sysctl = {
         .cmd = XEN_SYSCTL_dt_overlay,
         .u.dt_overlay = {
+            .domain_id = domid,
             .overlay_op = overlay_op,
             .overlay_fdt_size = overlay_fdt_size,
+            .domain_mapping = domain_mapping,
         }
     };
 
diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
index a6c709a6dc..cdb62b28cf 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -41,8 +41,9 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
     return 0;
 }
 
-int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
-                     uint8_t overlay_op)
+int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
+                     uint32_t overlay_dt_size, uint8_t overlay_op,
+                     bool auto_mode, bool domain_mapping)
 {
     int rc;
     int r;
@@ -57,10 +58,18 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
         rc = 0;
     }
 
-    r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
+    /* Check if user entered a valid domain id. */
+    rc = libxl_domain_info(CTX, NULL, domid);
+    if (rc == ERROR_DOMAIN_NOTFOUND) {
+        LOGD(ERROR, domid, "Non-existant domain.");
+        return ERROR_FAIL;
+    }
+
+    r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op,
+                      domain_mapping);
 
     if (r) {
-        LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
+        LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid);
         rc = ERROR_FAIL;
     }
 
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 98f6bd2e76..9674383ec3 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv)
 {
     const char *overlay_ops = NULL;
     const char *overlay_config_file = NULL;
+    uint32_t domain_id = 0;
     void *overlay_dtb = NULL;
     int rc;
+    bool auto_mode = true;
+    bool domain_mapping = false;
     uint8_t op;
     int overlay_dtb_size = 0;
     const int overlay_add_op = 1;
     const int overlay_remove_op = 2;
 
-    if (argc < 2) {
-        help("dt_overlay");
+    if (argc < 3) {
+        help("dt-overlay");
         return EXIT_FAILURE;
     }
 
+    if (argc > 5) {
+        fprintf(stderr, "Too many arguments\n");
+        return ERROR_FAIL;
+    }
+
     overlay_ops = argv[1];
     overlay_config_file = argv[2];
 
+    if (!strcmp(argv[argc - 1], "-e"))
+        auto_mode = false;
+
+    if (argc == 4 || !auto_mode) {
+        domain_id = find_domain(argv[argc-1]);
+        domain_mapping = true;
+    }
+
+    if (argc == 5 || !auto_mode) {
+        domain_id = find_domain(argv[argc-2]);
+        domain_mapping = true;
+    }
+
+    /* User didn't prove any overlay operation. */
+    if (overlay_ops == NULL) {
+        fprintf(stderr, "No overlay operation mode provided\n");
+        return ERROR_FAIL;
+    }
+
     if (strcmp(overlay_ops, "add") == 0)
         op = overlay_add_op;
     else if (strcmp(overlay_ops, "remove") == 0)
@@ -1309,7 +1336,8 @@ int main_dt_overlay(int argc, char **argv)
         return ERROR_FAIL;
     }
 
-    rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+    rc = libxl_dt_overlay(ctx, domain_id, overlay_dtb, overlay_dtb_size, op,
+                          auto_mode, domain_mapping);
 
     free(overlay_dtb);
 
-- 
2.34.1

Re: [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations

[Anthony PERARD]

On Wed, Apr 24, 2024 at 11:34:42AM +0800, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
> 
> Add domain_id and expert mode for overlay assignment. This enables dynamic
> programming of nodes during runtime.
> 
> Take the opportunity to fix the name mismatch in the xl command, the
> command name should be "dt-overlay" instead of "dt_overlay".

I don't like much these unrelated / opportunistic changes in a patch,
I'd rather have a separate patch. And in this case, if it was on a
separate patch, that separated patch could gain: Fixes: 61765a07e3d8
("tools/xl: Add new xl command overlay for device tree overlay support")
and potentially backported.

> 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>
> ---
>  tools/include/libxl.h               |  8 +++++--
>  tools/include/xenctrl.h             |  5 +++--
>  tools/libs/ctrl/xc_dt_overlay.c     |  7 ++++--
>  tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++----
>  tools/xl/xl_vmcontrol.c             | 34 ++++++++++++++++++++++++++---
>  5 files changed, 58 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 62cb07dea6..59a3e1b37c 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -2549,8 +2549,12 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
>  void libxl_device_pci_list_free(libxl_device_pci* list, int num);
>  
>  #if defined(__arm__) || defined(__aarch64__)
> -int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
> -                     uint32_t overlay_size, uint8_t overlay_op);
> +#define LIBXL_DT_OVERLAY_ADD                   1
> +#define LIBXL_DT_OVERLAY_REMOVE                2
> +
> +int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
> +                     uint32_t overlay_size, uint8_t overlay_op, bool auto_mode,
> +                     bool domain_mapping);

Sorry, you cannot change the API of an existing libxl function without
providing something backward compatible. We have already a few example
of this changes in libxl.h, e.g.: fded24ea8315 ("libxl: Make libxl_set_vcpuonline async")
So, providing a wrapper called libxl_dt_overlay_0x041800() which call
the new function.

>  #endif
>  
>  /*
> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
> index a6c709a6dc..cdb62b28cf 100644
> --- a/tools/libs/light/libxl_dt_overlay.c
> +++ b/tools/libs/light/libxl_dt_overlay.c
> @@ -57,10 +58,18 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
>          rc = 0;
>      }
>  
> -    r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
> +    /* Check if user entered a valid domain id. */
> +    rc = libxl_domain_info(CTX, NULL, domid);
> +    if (rc == ERROR_DOMAIN_NOTFOUND) {

Why do you check specifically for "domain not found", what about other
error?

> +        LOGD(ERROR, domid, "Non-existant domain.");
> +        return ERROR_FAIL;

Use `goto out`, and you can let the function return
ERROR_DOMAIN_NOTFOUND if that the error, we can just propagate the `rc`
from libxl_domain_info().

> +    }
> +
> +    r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op,
> +                      domain_mapping);
>  
>      if (r) {
> -        LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
> +        LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid);

You could replace the macro by LOGD, instead of handwriting "domain%d".

>          rc = ERROR_FAIL;
>      }
>  
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 98f6bd2e76..9674383ec3 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv)
>  {
>      const char *overlay_ops = NULL;
>      const char *overlay_config_file = NULL;
> +    uint32_t domain_id = 0;
>      void *overlay_dtb = NULL;
>      int rc;
> +    bool auto_mode = true;
> +    bool domain_mapping = false;
>      uint8_t op;
>      int overlay_dtb_size = 0;
>      const int overlay_add_op = 1;
>      const int overlay_remove_op = 2;
>  
> -    if (argc < 2) {
> -        help("dt_overlay");
> +    if (argc < 3) {
> +        help("dt-overlay");
>          return EXIT_FAILURE;
>      }
>  
> +    if (argc > 5) {
> +        fprintf(stderr, "Too many arguments\n");
> +        return ERROR_FAIL;
> +    }
> +
>      overlay_ops = argv[1];
>      overlay_config_file = argv[2];
>  
> +    if (!strcmp(argv[argc - 1], "-e"))
> +        auto_mode = false;
> +
> +    if (argc == 4 || !auto_mode) {
> +        domain_id = find_domain(argv[argc-1]);
> +        domain_mapping = true;
> +    }
> +
> +    if (argc == 5 || !auto_mode) {
> +        domain_id = find_domain(argv[argc-2]);
> +        domain_mapping = true;
> +    }

Sorry, I can't review that changes, this needs a change in the help
message of dt-overlay, and something in the man page. (and that argument
parsing looks convoluted).

> +
> +    /* User didn't prove any overlay operation. */

I guess you meant "provide" instead of prove. But the comment can be
discarded, it doesn't explain anything useful that the error message
doesn't already explain.

> +    if (overlay_ops == NULL) {
> +        fprintf(stderr, "No overlay operation mode provided\n");
> +        return ERROR_FAIL;

That should be EXIT_FAILURE instead. (and I realise that the function
already return ERROR_FAIL by mistake in several places. (ERROR_FAIL is a
libxl error return value of -3, which isn't really a good exit value for
CLI programmes.))

Thanks,

-- 
Anthony PERARD

Re: [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations

[Henry Wang]

Hi Anthony,

On 5/1/2024 10:46 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:42AM +0800, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Add domain_id and expert mode for overlay assignment. This enables dynamic
>> programming of nodes during runtime.
>>
>> Take the opportunity to fix the name mismatch in the xl command, the
>> command name should be "dt-overlay" instead of "dt_overlay".
> I don't like much these unrelated / opportunistic changes in a patch,
> I'd rather have a separate patch. And in this case, if it was on a
> separate patch, that separated patch could gain: Fixes: 61765a07e3d8
> ("tools/xl: Add new xl command overlay for device tree overlay support")
> and potentially backported.

Ok. I can split this part to a separated commit.

>> 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>
>> ---
>>   tools/include/libxl.h               |  8 +++++--
>>   tools/include/xenctrl.h             |  5 +++--
>>   tools/libs/ctrl/xc_dt_overlay.c     |  7 ++++--
>>   tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++----
>>   tools/xl/xl_vmcontrol.c             | 34 ++++++++++++++++++++++++++---
>>   5 files changed, 58 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index 62cb07dea6..59a3e1b37c 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -2549,8 +2549,12 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
>>   void libxl_device_pci_list_free(libxl_device_pci* list, int num);
>>   
>>   #if defined(__arm__) || defined(__aarch64__)
>> -int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
>> -                     uint32_t overlay_size, uint8_t overlay_op);
>> +#define LIBXL_DT_OVERLAY_ADD                   1
>> +#define LIBXL_DT_OVERLAY_REMOVE                2
>> +
>> +int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
>> +                     uint32_t overlay_size, uint8_t overlay_op, bool auto_mode,
>> +                     bool domain_mapping);
> Sorry, you cannot change the API of an existing libxl function without
> providing something backward compatible. We have already a few example
> of this changes in libxl.h, e.g.: fded24ea8315 ("libxl: Make libxl_set_vcpuonline async")
> So, providing a wrapper called libxl_dt_overlay_0x041800() which call
> the new function.

Ok, I will add an wrapper.

>>   #endif
>>   
>>   /*
>> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
>> index a6c709a6dc..cdb62b28cf 100644
>> --- a/tools/libs/light/libxl_dt_overlay.c
>> +++ b/tools/libs/light/libxl_dt_overlay.c
>> @@ -57,10 +58,18 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
>>           rc = 0;
>>       }
>>   
>> -    r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
>> +    /* Check if user entered a valid domain id. */
>> +    rc = libxl_domain_info(CTX, NULL, domid);
>> +    if (rc == ERROR_DOMAIN_NOTFOUND) {
> Why do you check specifically for "domain not found", what about other
> error?

I agree this is indeed very confusing...I will rewrite this part 
properly in the next version.

>> +        LOGD(ERROR, domid, "Non-existant domain.");
>> +        return ERROR_FAIL;
> Use `goto out`, and you can let the function return
> ERROR_DOMAIN_NOTFOUND if that the error, we can just propagate the `rc`
> from libxl_domain_info().

Sure, will do the suggested way.

>> +    }
>> +
>> +    r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op,
>> +                      domain_mapping);
>>   
>>       if (r) {
>> -        LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
>> +        LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid);
> You could replace the macro by LOGD, instead of handwriting "domain%d".

Great suggestion. I will use LOGD.

>>           rc = ERROR_FAIL;
>>       }
>>   
>> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
>> index 98f6bd2e76..9674383ec3 100644
>> --- a/tools/xl/xl_vmcontrol.c
>> +++ b/tools/xl/xl_vmcontrol.c
>> @@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv)
>>   {
>>       const char *overlay_ops = NULL;
>>       const char *overlay_config_file = NULL;
>> +    uint32_t domain_id = 0;
>>       void *overlay_dtb = NULL;
>>       int rc;
>> +    bool auto_mode = true;
>> +    bool domain_mapping = false;
>>       uint8_t op;
>>       int overlay_dtb_size = 0;
>>       const int overlay_add_op = 1;
>>       const int overlay_remove_op = 2;
>>   
>> -    if (argc < 2) {
>> -        help("dt_overlay");
>> +    if (argc < 3) {
>> +        help("dt-overlay");
>>           return EXIT_FAILURE;
>>       }
>>   
>> +    if (argc > 5) {
>> +        fprintf(stderr, "Too many arguments\n");
>> +        return ERROR_FAIL;
>> +    }
>> +
>>       overlay_ops = argv[1];
>>       overlay_config_file = argv[2];
>>   
>> +    if (!strcmp(argv[argc - 1], "-e"))
>> +        auto_mode = false;
>> +
>> +    if (argc == 4 || !auto_mode) {
>> +        domain_id = find_domain(argv[argc-1]);
>> +        domain_mapping = true;
>> +    }
>> +
>> +    if (argc == 5 || !auto_mode) {
>> +        domain_id = find_domain(argv[argc-2]);
>> +        domain_mapping = true;
>> +    }
> Sorry, I can't review that changes, this needs a change in the help
> message of dt-overlay, and something in the man page. (and that argument
> parsing looks convoluted).

I will rework this part to see if I can make it better.

>> +
>> +    /* User didn't prove any overlay operation. */
> I guess you meant "provide" instead of prove. But the comment can be
> discarded, it doesn't explain anything useful that the error message
> doesn't already explain.

I think your comment is correct, I will just drop it.

>> +    if (overlay_ops == NULL) {
>> +        fprintf(stderr, "No overlay operation mode provided\n");
>> +        return ERROR_FAIL;
> That should be EXIT_FAILURE instead. (and I realise that the function
> already return ERROR_FAIL by mistake in several places. (ERROR_FAIL is a
> libxl error return value of -3, which isn't really a good exit value for
> CLI programmes.))

Thanks. Will use EXIT_FAILURE.

Kind regards,
Henry

>
> Thanks,
>

[PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Add support for modifying dtbo to make it suitable for DomU Linux. This is
done for '-e expert' mode.

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>
---
 tools/libs/light/libxl_dt_overlay.c | 73 +++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
index cdb62b28cf..eaf11a0f9c 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -17,6 +17,7 @@
 #include "libxl_internal.h"
 #include <libfdt.h>
 #include <xenctrl.h>
+#include <xen/device_tree_defs.h>
 
 static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
 {
@@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
     return 0;
 }
 
+static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
+                                   size_t size)
+{
+    int rc = 0;
+    int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
+    const struct fdt_property *fdt_prop_node = NULL;
+    int overlay;
+    int prop_len = 0;
+    int subnode = 0;
+    int fragment;
+    const char *prop_name;
+    const char *target_path = "/";
+
+    fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
+        prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
+                                &prop_len);
+        if (prop_name == NULL) {
+            LOG(ERROR, "target-path property not found\n");
+            rc = ERROR_FAIL;
+            goto err;
+        }
+
+        /* Change target path for domU dtb. */
+        rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",
+                                target_path);
+        if (rc) {
+            LOG(ERROR, "Setting interrupt parent property failed for %s\n",
+                prop_name);
+            goto err;
+        }
+
+        overlay = fdt_subnode_offset(overlay_dt_domU, fragment, "__overlay__");
+
+        fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
+        {
+            const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
+                                                 NULL);
+
+            fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
+                                        "interrupt-parent", &prop_len);
+            if (fdt_prop_node == NULL) {
+                LOG(DETAIL, "%s property not found for %s. Skip to next node\n",
+                    "interrupt-parent", node_name);
+                continue;
+            }
+
+            rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
+                                         "interrupt-parent",
+                                         virtual_interrupt_parent);
+            if (rc) {
+                LOG(ERROR, "Setting interrupt parent property failed for %s\n",
+                    "interrupt-parent");
+                goto err;
+            }
+        }
+    }
+
+return 0;
+
+err:
+    return rc;
+}
+
 int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
                      uint32_t overlay_dt_size, uint8_t overlay_op,
                      bool auto_mode, bool domain_mapping)
@@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
         rc = ERROR_FAIL;
     }
 
+    /*
+     * auto_mode doesn't apply to dom0 as dom0 can get the physical
+     * description of the hardware.
+     */
+    if (domid && auto_mode) {
+        if (overlay_op == LIBXL_DT_OVERLAY_ADD)
+            rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
+    }
+
 out:
     GC_FREE;
     return rc;
-- 
2.34.1

Re: [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format

[Anthony PERARD]

On Wed, Apr 24, 2024 at 11:34:43AM +0800, Henry Wang wrote:
> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
> index cdb62b28cf..eaf11a0f9c 100644
> --- a/tools/libs/light/libxl_dt_overlay.c
> +++ b/tools/libs/light/libxl_dt_overlay.c
> @@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
>      return 0;
>  }
>  
> +static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
> +                                   size_t size)
> +{
> +    int rc = 0;
> +    int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
> +    const struct fdt_property *fdt_prop_node = NULL;
> +    int overlay;
> +    int prop_len = 0;
> +    int subnode = 0;
> +    int fragment;
> +    const char *prop_name;
> +    const char *target_path = "/";
> +
> +    fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
> +        prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
> +                                &prop_len);
> +        if (prop_name == NULL) {
> +            LOG(ERROR, "target-path property not found\n");

LOG* macros already takes care of adding \n, no need to add an extra
one.

> +            rc = ERROR_FAIL;
> +            goto err;
> +        }
> +
> +        /* Change target path for domU dtb. */
> +        rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",

fdt_setprop_string() isn't a libxl function, store the return value in a
variable named `r` instead.

> +                                target_path);
> +        if (rc) {
> +            LOG(ERROR, "Setting interrupt parent property failed for %s\n",
> +                prop_name);
> +            goto err;
> +        }
> +
> +        overlay = fdt_subnode_offset(overlay_dt_domU, fragment, "__overlay__");
> +
> +        fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
> +        {
> +            const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
> +                                                 NULL);
> +
> +            fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
> +                                        "interrupt-parent", &prop_len);
> +            if (fdt_prop_node == NULL) {
> +                LOG(DETAIL, "%s property not found for %s. Skip to next node\n",
> +                    "interrupt-parent", node_name);

Why do you have "interrupt-parent" in a separate argument? Do you meant
to do something like
    const char *some_name = "interrupt-parent";
and use that in the 4 different places that this string is used? (Using
a variable mean that we (or the compiler) can make sure that they are
all spelled correctly.

> +                continue;
> +            }
> +
> +            rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
> +                                         "interrupt-parent",
> +                                         virtual_interrupt_parent);
> +            if (rc) {
> +                LOG(ERROR, "Setting interrupt parent property failed for %s\n",
> +                    "interrupt-parent");
> +                goto err;
> +            }
> +        }
> +    }
> +
> +return 0;

Missed indentation.

> +
> +err:
> +    return rc;

A few things, looks like `rc` is always going to be ERROR_FAIL here,
unless you find an libxl_error code that better describe the error, so
you could forgo the `rc` variable.

Also, if you don't need to clean up anything in the function or have a
generic error message, you could simply "return " instead of using the
"goto" style.

> +}
> +
>  int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
>                       uint32_t overlay_dt_size, uint8_t overlay_op,
>                       bool auto_mode, bool domain_mapping)
> @@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
>          rc = ERROR_FAIL;
>      }
>  
> +    /*
> +     * auto_mode doesn't apply to dom0 as dom0 can get the physical
> +     * description of the hardware.
> +     */
> +    if (domid && auto_mode) {
> +        if (overlay_op == LIBXL_DT_OVERLAY_ADD)

Shouldn't libxl complain if the operation is different?

> +            rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
> +    }
> +
>  out:
>      GC_FREE;
>      return rc;

Thanks,

-- 
Anthony PERARD

Re: [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format

[Henry Wang]

Hi Anthony,

On 5/1/2024 11:09 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:43AM +0800, Henry Wang wrote:
>> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
>> index cdb62b28cf..eaf11a0f9c 100644
>> --- a/tools/libs/light/libxl_dt_overlay.c
>> +++ b/tools/libs/light/libxl_dt_overlay.c
>> @@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
>>       return 0;
>>   }
>>   
>> +static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
>> +                                   size_t size)
>> +{
>> +    int rc = 0;
>> +    int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
>> +    const struct fdt_property *fdt_prop_node = NULL;
>> +    int overlay;
>> +    int prop_len = 0;
>> +    int subnode = 0;
>> +    int fragment;
>> +    const char *prop_name;
>> +    const char *target_path = "/";
>> +
>> +    fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
>> +        prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
>> +                                &prop_len);
>> +        if (prop_name == NULL) {
>> +            LOG(ERROR, "target-path property not found\n");
> LOG* macros already takes care of adding \n, no need to add an extra
> one.

Sure, I will remove the "\n".

>
>> +            rc = ERROR_FAIL;
>> +            goto err;
>> +        }
>> +
>> +        /* Change target path for domU dtb. */
>> +        rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",
> fdt_setprop_string() isn't a libxl function, store the return value in a
> variable named `r` instead.'

Thanks for spotting this. Will change it to `r`.

>> +                                target_path);
>> +        if (rc) {
>> +            LOG(ERROR, "Setting interrupt parent property failed for %s\n",
>> +                prop_name);
>> +            goto err;
>> +        }
>> +
>> +        overlay = fdt_subnode_offset(overlay_dt_domU, fragment, "__overlay__");
>> +
>> +        fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
>> +        {
>> +            const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
>> +                                                 NULL);
>> +
>> +            fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
>> +                                        "interrupt-parent", &prop_len);
>> +            if (fdt_prop_node == NULL) {
>> +                LOG(DETAIL, "%s property not found for %s. Skip to next node\n",
>> +                    "interrupt-parent", node_name);
> Why do you have "interrupt-parent" in a separate argument? Do you meant
> to do something like
>      const char *some_name = "interrupt-parent";
> and use that in the 4 different places that this string is used? (Using
> a variable mean that we (or the compiler) can make sure that they are
> all spelled correctly.

Great suggestion! I will do this way.

>> +                continue;
>> +            }
>> +
>> +            rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
>> +                                         "interrupt-parent",
>> +                                         virtual_interrupt_parent);
>> +            if (rc) {
>> +                LOG(ERROR, "Setting interrupt parent property failed for %s\n",
>> +                    "interrupt-parent");
>> +                goto err;
>> +            }
>> +        }
>> +    }
>> +
>> +return 0;
> Missed indentation.

Will correct it.

>> +
>> +err:
>> +    return rc;
> A few things, looks like `rc` is always going to be ERROR_FAIL here,
> unless you find an libxl_error code that better describe the error, so
> you could forgo the `rc` variable.
>
> Also, if you don't need to clean up anything in the function or have a
> generic error message, you could simply "return " instead of using the
> "goto" style.

Sure, I will simply use return because I don't really think there is 
anything to be cleaned up.

>> +}
>> +
>>   int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
>>                        uint32_t overlay_dt_size, uint8_t overlay_op,
>>                        bool auto_mode, bool domain_mapping)
>> @@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
>>           rc = ERROR_FAIL;
>>       }
>>   
>> +    /*
>> +     * auto_mode doesn't apply to dom0 as dom0 can get the physical
>> +     * description of the hardware.
>> +     */
>> +    if (domid && auto_mode) {
>> +        if (overlay_op == LIBXL_DT_OVERLAY_ADD)
> Shouldn't libxl complain if the operation is different?

I will add corresponding error handling code here. Thanks!

Kind regards,
Henry

>> +            rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
>> +    }
>> +
>>   out:
>>       GC_FREE;
>>       return rc;
> Thanks,
>

[PATCH 10/15] tools/xl: Share overlay with domU

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Add domid to enable assigning the node to a running VMs.

Add expert mode option to share the overlay dtbo with domU. In this mode dom0
communicates with domid domain to share the overlay dtbo. This is done via
xenstore.

Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 tools/xl/Makefile       |   2 +-
 tools/xl/xl_cmdtable.c  |   2 +-
 tools/xl/xl_vmcontrol.c | 343 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 342 insertions(+), 5 deletions(-)

diff --git a/tools/xl/Makefile b/tools/xl/Makefile
index d742e96a5b..122dfb9346 100644
--- a/tools/xl/Makefile
+++ b/tools/xl/Makefile
@@ -32,7 +32,7 @@ $(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h # libxl_json.h needs i
 all: xl
 
 xl: $(XL_OBJS)
-	$(CC) $(LDFLAGS) -o $@ $(XL_OBJS) $(LDLIBS_libxenutil) $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) -lyajl $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) -o $@ $(XL_OBJS) $(LDLIBS_libxenutil) $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxengnttab) -lyajl $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: all
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 62bdb2aeaa..2531e8517b 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -635,7 +635,7 @@ const struct cmd_spec cmd_table[] = {
     { "dt-overlay",
       &main_dt_overlay, 0, 1,
       "Add/Remove a device tree overlay",
-      "add/remove <.dtbo>"
+      "add/remove <.dtbo> domain_id -e[expert_mode]",
       "-h print this help\n"
     },
 #endif
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 9674383ec3..2bf76dd389 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -16,6 +16,7 @@
 #include <inttypes.h>
 #include <limits.h>
 #include <stdlib.h>
+#include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/utsname.h>
@@ -29,6 +30,8 @@
 #include "xl.h"
 #include "xl_utils.h"
 #include "xl_parse.h"
+#include <xenstore.h>
+#include <xengnttab.h>
 
 static int fd_lock = -1;
 
@@ -1266,6 +1269,335 @@ int main_create(int argc, char **argv)
 }
 
 #ifdef LIBXL_HAVE_DT_OVERLAY
+static int copy_overlay_to_domU(uint32_t domain_id, void *overlay_dt_domU,
+                                uint32_t overlay_dt_size, uint32_t *grant_ref,
+                                uint32_t num_pages)
+{
+    void *buffer = NULL;
+    xengnttab_handle *gnttab = xengnttab_open(NULL, 0);
+
+    if (!gnttab) {
+        fprintf(stderr,"opening gnttab failed for domain %d\n", domain_id);
+        return -1;
+    }
+
+    buffer = xengnttab_map_domain_grant_refs(gnttab, num_pages, domain_id,
+                                             grant_ref, PROT_READ|PROT_WRITE);
+
+    if (buffer == NULL) {
+        fprintf(stderr, "Getting the buffer failed for grant_refs\n");
+
+        return -1;
+    }
+
+    /* buffer is contiguous allocated. */
+    memcpy(buffer, overlay_dt_domU, overlay_dt_size);
+
+    xengnttab_unmap(gnttab, buffer, num_pages);
+
+    return 0;
+}
+
+static bool wait_for_status(struct xs_handle *xs, int fd, char *status_path,
+                            const char *status)
+{
+    unsigned int num_strings;
+    char *buf = NULL;
+    char **vec = NULL;
+    bool ret = false;
+    unsigned int len;
+    int rc = 0;
+    fd_set set;
+
+    while (1)
+    {
+        FD_ZERO(&set);
+        FD_SET(fd, &set);
+
+        rc = select(fd + 1, &set, NULL, NULL, NULL);
+        /* Poll for data: Blocking. */
+        if (rc <= 0)
+            break;
+
+        if (FD_ISSET(fd, &set)) {
+            /*
+             * num_strings will be set to the number of elements in vec
+             * (2 - the watched path and the overlay_watch)
+             */
+            vec = xs_read_watch(xs, &num_strings);
+            if (!vec) {
+                break;
+            }
+
+            /* do a read. */
+            buf = xs_read(xs, XBT_NULL, status_path, &len);
+            if (buf) {
+                if (!strcmp(buf, status)) {
+                    ret = true;
+                    break;
+                }
+            }
+        }
+    }
+
+    free(vec);
+    free(buf);
+
+    return ret;
+}
+
+static bool write_overlay_size(struct xs_handle *xs, uint32_t overlay_size,
+                               char *path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+    char buf[128];
+    char ref[16];
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        return false;
+
+    snprintf(ref, sizeof(ref), "%u", overlay_size);
+    snprintf(buf, sizeof(buf), "%s/overlay-size", path);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        return false;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return false;
+    }
+
+    return true;
+}
+
+static bool write_status(struct xs_handle *xs, char *status, char *status_path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        return false;
+
+    if (!xs_write(xs, xs_trans, status_path, status, strlen(status)))
+        return false;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return false;
+    }
+
+    return true;
+}
+
+static uint32_t *get_page_ref(struct xs_handle *xs, const char *xs_path,
+                              uint32_t num_pages)
+{
+    char buf[128];
+    uint32_t *ref = NULL;
+    char *data;
+    char temp[16] = { };
+    unsigned int len;
+    int i = 0;
+    int j = 0;
+    int start_ind = 0;
+
+    snprintf(buf, sizeof(buf), "%s/page-ref", xs_path);
+
+    /* do a read. */
+    data = xs_read(xs, XBT_NULL, buf, &len);
+
+    if (data) {
+        /* Caller will free this. */
+        ref = (uint32_t *)calloc(num_pages, sizeof(uint32_t));
+
+        for (i = 0; data[i] != '\0'; i++) {
+            if (data[i] == ',') {
+                /* Next page_ref data. */
+                memset(temp, '\0', sizeof(temp));
+
+                /* Copy the data between previous ',' to current. */
+                memcpy(temp, &data[start_ind], i - start_ind);
+
+                /* Convert to int. */
+                ref[j] = atoi(temp);
+                start_ind = i + 1;
+                j++;
+            }
+
+            if (j == num_pages)
+                break;
+        }
+    }
+
+    if (j != num_pages) {
+        fprintf(stderr, "Number of page_refs are not equal to num_pages\n");
+        free(ref);
+        ref = NULL;
+    }
+
+    free(data);
+    return ref;
+}
+
+static uint32_t get_num_pages(struct xs_handle *xs, const char *xs_path)
+{
+    char buf[128];
+    char *ref;
+    unsigned int len;
+    uint32_t num_pages = 0;
+
+    snprintf(buf, sizeof(buf), "%s/num-pages", xs_path);
+
+    /* do a read. */
+    ref = xs_read(xs, XBT_NULL, buf, &len);
+
+    if (ref)
+        num_pages = atoi(ref);
+
+    free(ref);
+
+    return num_pages;
+}
+
+static int share_overlay_with_domu(void *overlay_dt_domU, int overlay_dt_size,
+                                   int domain_id)
+{
+    struct xs_handle *xs = NULL;
+    char *path = NULL;
+    char receiver_status_path[64] = { };
+    char sender_status_path[64] = { };
+    int fd = 0;
+    int err;
+    uint32_t num_pages= 1;
+    uint32_t *page_ref = NULL;
+
+    /* XS_watch part. */
+    /* Open a connection to the xenstore. */
+    xs = xs_open(0);
+    if (xs == NULL) {
+        fprintf(stderr, "Deamon open for domain%d failed\n", domain_id);
+        err = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Get the local domain path */
+    path = xs_get_domain_path(xs, domain_id);
+    if (path == NULL) {
+        fprintf(stderr, "Getting domain%d path failed\n", domain_id);
+        err = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Make space for our node on the path */
+    path = realloc(path, strlen(path) + strlen("/data/overlay") + 1);
+    if (path == NULL) {
+        fprintf(stderr, "Path allocation failed\n");
+        err = ERROR_NOMEM;
+        goto out;
+    }
+
+    strcat(path, "/data/overlay");
+    strcpy(receiver_status_path, path);
+    strcat(receiver_status_path, "/receiver-status");
+
+    /*
+     * Watch a node for changes (poll on fd to detect).
+     * When the node (or any child) changes, fd will become readable.
+     */
+    err = xs_watch(xs, receiver_status_path, "overlay_watch");
+    if (!err) {
+        fprintf(stderr, "Creating watch failed\n");
+        err = ERROR_FAIL;
+        goto out;
+    }
+
+    /*
+     * We are notified of read availability on the watch via the
+     * file descriptor.
+     */
+    fd = xs_fileno(xs);
+
+    /* Wait for "waiting" status from other domain. */
+    if (!wait_for_status(xs, fd, receiver_status_path, "waiting")) {
+        err = ERROR_NOT_READY;
+        goto out;
+    }
+
+    /* Share the dtb size with domain. */
+    if (!write_overlay_size(xs, overlay_dt_size, path)) {
+        err = ERROR_FAIL;
+        fprintf(stderr,"Writing page ref failed\n");
+        goto out;
+    }
+
+    strcpy(sender_status_path, path);
+    strcat(sender_status_path, "/sender-status");
+
+    /* Write the status "ready". */
+    if (!write_status(xs, "ready", sender_status_path)) {
+        err = ERROR_FAIL;
+        fprintf(stderr,"Writing status ready failed\n");
+        goto out;
+    }
+
+    /* Wait for "page_ref" status from other domain. */
+    if (!wait_for_status(xs, fd, receiver_status_path, "page_ref")) {
+        err = ERROR_NOT_READY;
+        goto out;
+    }
+
+    num_pages = get_num_pages(xs, path);
+    if (num_pages == 0) {
+        fprintf(stderr, "no pages allocated\n");
+        err = ERROR_FAIL;
+        goto out;
+    }
+
+    page_ref = get_page_ref(xs, path, num_pages);
+    if (page_ref == NULL) {
+        fprintf(stderr,"page ref is null.\n");
+        err = ERROR_FAIL;
+        goto out;
+    }
+
+    if (copy_overlay_to_domU(domain_id, overlay_dt_domU, overlay_dt_size,
+                             page_ref, num_pages)) {
+        fprintf(stderr,"Copy overlay failed\n");
+        err = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Write the status "done". */
+    if (!write_status(xs, "done", sender_status_path)) {
+        fprintf(stderr,"Writing status DONE failed\n");
+        err = ERROR_FAIL;
+        goto out;
+    }
+
+/* Cleanup */
+out:
+    if (xs) {
+        close(fd);
+        xs_unwatch(xs, path, "overlay_watch");
+        xs_close(xs);
+    }
+
+    if (path)
+        free(path);
+
+    if (page_ref)
+        free(page_ref);
+
+    return err;
+}
+
 int main_dt_overlay(int argc, char **argv)
 {
     const char *overlay_ops = NULL;
@@ -1339,11 +1671,16 @@ int main_dt_overlay(int argc, char **argv)
     rc = libxl_dt_overlay(ctx, domain_id, overlay_dtb, overlay_dtb_size, op,
                           auto_mode, domain_mapping);
 
-    free(overlay_dtb);
-
-    if (rc)
+    if (rc) {
+        free(overlay_dtb);
         return EXIT_FAILURE;
+    }
+
+    if (domain_id && auto_mode && (op == LIBXL_DT_OVERLAY_ADD))
+        rc = share_overlay_with_domu(overlay_dtb, overlay_dtb_size,
+                                     domain_id);
 
+    free(overlay_dtb);
     return rc;
 }
 #endif
-- 
2.34.1

[PATCH 11/15] tools/helpers: Add get_overlay

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

This user level application copies the overlay dtbo shared by dom0 while doing
overlay node assignment operation. It uses xenstore to communicate with dom0.
More information on the protocol is writtien in docs/misc/overlay.txt file.

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>
---
 tools/helpers/Makefile      |   8 +
 tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
 2 files changed, 401 insertions(+)
 create mode 100644 tools/helpers/get_overlay.c

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 09590eb5b6..dfe17ef269 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
 endif
 ifeq ($(CONFIG_ARM),y)
 TARGETS += init-dom0less
+TARGETS += get_overlay
 endif
 endif
 
@@ -39,6 +40,9 @@ $(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
 $(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
 init-dom0less: LDLIBS += $(call xenlibs-ldlibs,ctrl evtchn toollog store light guest foreignmemory)
 
+SHARE_OVERLAY_OBJS = get_overlay.o
+$(SHARE_OVERLAY_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenstore) $(CFLAGS_libxenctrl)
+
 .PHONY: all
 all: $(TARGETS)
 
@@ -51,6 +55,10 @@ init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
 init-dom0less: $(INIT_DOM0LESS_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS) $(APPEND_LDFLAGS)
 
+get_overlay: $(SHARE_OVERLAY_OBJS)
+	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
+
+
 .PHONY: install
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
diff --git a/tools/helpers/get_overlay.c b/tools/helpers/get_overlay.c
new file mode 100644
index 0000000000..ca3007570e
--- /dev/null
+++ b/tools/helpers/get_overlay.c
@@ -0,0 +1,393 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <limits.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <xenstore.h>
+#include <libxenvchan.h>
+#include <xengnttab.h>
+
+#define PAGE_SIZE 4096
+
+static int xs_create_overlay_node(int domain, const char *xs_base,
+                                  struct xs_handle *xs)
+{
+    int ret = -1;
+    struct xs_permissions perms[2];
+    char buf[64];
+    char ref[16];
+    char *domid_str = NULL;
+    int overlay_size = 0;
+
+    xs_transaction_t xs_trans = XBT_NULL;
+
+    domid_str = xs_read(xs, XBT_NULL, "domid", NULL);
+
+    if (!domid_str)
+        return ret;
+
+    /* owner domain is us */
+    perms[0].id = atoi(domid_str);
+    /* permissions for domains not listed = none. */
+    perms[0].perms = XS_PERM_NONE;
+    /* other domains i.e. domain provided by user gets r/w permissions. */
+    perms[1].id = domain;
+    perms[1].perms =  XS_PERM_READ | XS_PERM_WRITE;
+
+retry_transaction:
+
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        goto fail_xs_transaction;
+
+    /* Create overlay-size node. */
+    snprintf(ref, sizeof(ref), "%d", overlay_size);
+    snprintf(buf, sizeof(buf), "%s/overlay-size", xs_base);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        goto fail_xs_transaction;
+    if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+        goto fail_xs_transaction;
+
+    /* Create domU status node. */
+    snprintf(ref, sizeof(ref), "%s", "waiting");
+    snprintf(buf, sizeof(buf), "%s/receiver-status", xs_base);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        goto fail_xs_transaction;
+    if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+        goto fail_xs_transaction;
+
+    /* Create dom0 status node. */
+    snprintf(ref, sizeof(ref), "%s", "not_ready");
+    snprintf(buf, sizeof(buf), "%s/sender-status", xs_base);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        goto fail_xs_transaction;
+    if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+        goto fail_xs_transaction;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            goto fail_xs_transaction;
+    } else
+        ret = 0;
+
+fail_xs_transaction:
+    free(domid_str);
+
+    return ret;
+}
+
+static int get_overlay_size(struct xs_handle *xs, int domain,
+                            const char *xs_path)
+{
+    char buf[128];
+    char *ref;
+    unsigned int len;
+    int dt_size = 0;
+
+    snprintf(buf, sizeof(buf), "%s/overlay-size", xs_path);
+
+    ref = xs_read(xs, XBT_NULL, buf, &len);
+
+    if (!ref)
+        return dt_size;
+
+    dt_size = atoi(ref);
+
+    free(ref);
+
+    return dt_size;
+}
+
+static uint32_t get_num_pages(int dtbo_size)
+{
+    int num_pages = 1;
+
+    while (dtbo_size > PAGE_SIZE) {
+        dtbo_size = dtbo_size - PAGE_SIZE;
+        num_pages++;
+    }
+
+    return num_pages;
+}
+
+static void *create_shared_buffer(int domain, uint32_t *refs, uint32_t pages,
+                                  xengntshr_handle *gntshr)
+{
+    return xengntshr_share_pages(gntshr, domain, pages, refs, 1);
+}
+
+static bool wait_for_status(struct xs_handle *xs, int fd, char *status_path,
+                            const char *status)
+{
+    unsigned int num_strings;
+    char *buf = NULL;
+    char **vec = NULL;
+    bool ret = false;
+    unsigned int len;
+    int rc = 0;
+    fd_set set;
+
+    while (1)
+    {
+        FD_ZERO(&set);
+        FD_SET(fd, &set);
+
+        rc = select(fd + 1, &set, NULL, NULL, NULL);
+        /* Poll for data: Blocking. */
+        if (rc <= 0)
+            break;
+
+        if (FD_ISSET(fd, &set)) {
+            /*
+             * num_strings will be set to the number of elements in vec
+             * (2 - the watched path and the overlay_watch)
+             */
+            vec = xs_read_watch(xs, &num_strings);
+            if (!vec) {
+                break;
+            }
+
+            /* do a read. */
+            buf = xs_read(xs, XBT_NULL, status_path, &len);
+            if (buf) {
+                if (!strcmp(buf, status)) {
+                    ret = true;
+                    break;
+                }
+            }
+        }
+    }
+
+    free(vec);
+    free(buf);
+
+    return ret;
+}
+
+static bool write_page_ref(struct xs_handle *xs, uint32_t *page_ref,
+                           uint32_t num_pages, char *path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+    char buf[128];
+    char *ref = NULL;
+    char tmp[16];
+    int i = 0;
+    bool rc = false;
+
+    /* Caller will free this. */
+    ref = (char *)calloc(num_pages * 16, sizeof(char)); /* For each number. */
+    if (ref == NULL) {
+        fprintf(stderr, "Memory calloc for ref failed\n");
+        return rc;
+    }
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        goto out;
+
+    for (i = 0; i < num_pages; i++) {
+        snprintf(tmp, sizeof(tmp), "%d,", page_ref[i]);
+        strcat(ref, tmp);
+    }
+
+    snprintf(buf, sizeof(buf), "%s/page-ref", path);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        goto out;
+
+    snprintf(buf, sizeof(buf), "%s/num-pages", path);
+    snprintf(tmp, sizeof(tmp), "%u", num_pages);
+    if (!xs_write(xs, xs_trans, buf, tmp, strlen(tmp)))
+        goto out;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            goto out;
+    }
+
+    rc = true;
+
+out:
+    if (ref)
+        free(ref);
+
+    return rc;
+}
+
+static bool write_status(struct xs_handle *xs, const char *status,
+                         const char *status_path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        return false;
+
+    if (!xs_write(xs, xs_trans, status_path, status, strlen(status)))
+        return false;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return false;
+    }
+
+    return true;
+}
+
+int main(int argc, char **argv)
+{
+    void *buffer = NULL;
+    int domain ;
+    uint32_t *page_refs = NULL;
+    FILE *fptr;
+    int dtbo_size = 0;
+    const char *path = "data/overlay";
+    char receiver_status_path[64] = { };
+    char sender_status_path[64] = { };
+    struct xs_handle *xs = NULL;
+    int rc = 0;
+    int fd = 0;
+    uint32_t num_pages = 0;
+    xengntshr_handle *gntshr;
+
+    if (argc < 2) {
+       fprintf(stderr,"Please enter domain_id.\n");
+        return 0;
+    }
+
+    domain = atoi(argv[1]);
+
+    xs = xs_open(0);
+    if (xs == NULL) {
+        fprintf(stderr, "Xenstore open for domain%d failed\n", domain);
+        goto out;
+    }
+
+    rc = xs_create_overlay_node(domain, path, xs);
+    if (rc) {
+        fprintf(stderr,"Creating overlay nodes failed\n");
+        goto out;
+    }
+
+    strcpy(receiver_status_path, path);
+    strcat(receiver_status_path, "/receiver-status");
+
+    strcpy(sender_status_path, path);
+    strcat(sender_status_path, "/sender-status");
+
+    /*
+     * Watch a node for changes (poll on fd to detect).
+     * When the node changes, fd will become readable.
+     */
+    rc = xs_watch(xs, sender_status_path, "overlay_watch");
+    if (rc == 0) {
+        fprintf(stderr, "Creating watch failed\n");
+        goto out;
+    }
+
+    /* We are notified of read availability on the watch via the
+     * file descriptor.
+     */
+    fd = xs_fileno(xs);
+
+    /* Wait for ready. */
+    if (!wait_for_status(xs, fd, sender_status_path, "ready")) {
+        fprintf(stderr, "dom0 not ready.\n");
+        goto out;
+    }
+
+    dtbo_size = get_overlay_size(xs, domain, path);
+    if (dtbo_size == 0) {
+        fprintf(stderr,"Overlay data size is zero. Exiting the application\n");
+        goto out;
+    }
+
+    gntshr = xengntshr_open(NULL, 0);
+    if (!gntshr) {
+        fprintf(stderr,"Error in opening gntshr\n");
+        goto out;
+    }
+
+    num_pages = get_num_pages(dtbo_size);
+
+    page_refs =(uint32_t *)malloc(num_pages * sizeof(int));
+    if (page_refs == NULL) {
+        fprintf(stderr, "Allocating page_ref array failed\n");
+        goto out;
+    }
+
+    /* Allocate memory for data size and share with domain. */
+    buffer = create_shared_buffer(domain, page_refs, num_pages,
+                                  gntshr);
+    if (buffer == NULL) {
+        fprintf(stderr,"Buffer allocation failed\n");
+        goto out;
+    }
+
+    /* Created the buffer and got page_ref. Share the page_ref with domain. */
+    if (!write_page_ref(xs, page_refs, num_pages, path)) {
+        fprintf(stderr,"Writing page ref failed\n");
+        goto out;
+    }
+
+    /* Write the status "page_ref". */
+    if (!write_status(xs, "page_ref", receiver_status_path)) {
+        fprintf(stderr,"Writing status DONE failed\n");
+        goto out;
+    }
+
+    /* Wait for done. This means other domain done copying the dtb to buffer. */
+    if (!wait_for_status(xs, fd, sender_status_path, "done")) {
+        fprintf(stderr, "dom0 status not done\n");
+        goto out;
+    }
+
+    if ((fptr = fopen("overlay.dtbo","wb")) == NULL) {
+        fprintf(stderr,"Error! opening file");
+        goto out;
+    }
+
+    printf("Writing to file overlay.dtbo.\n");
+
+    fwrite(buffer, dtbo_size, 1, fptr);
+
+    printf("Done writing to file overlay.dtbo \n");
+
+out:
+    if (fptr)
+        fclose(fptr);
+
+    if (page_refs)
+        free(page_refs);
+
+    if (xs) {
+        close(fd);
+
+        xs_unwatch(xs, path, "overlay_watch");
+
+        xs_close(xs);
+    }
+
+    if (buffer)
+        xengntshr_unshare(gntshr, buffer, num_pages);
+
+    if (gntshr)
+         xengntshr_close(gntshr);
+
+    return 0;
+}
-- 
2.34.1

Re: [PATCH 11/15] tools/helpers: Add get_overlay

[Jan Beulich]

On 24.04.2024 05:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
> 
> This user level application copies the overlay dtbo shared by dom0 while doing
> overlay node assignment operation. It uses xenstore to communicate with dom0.
> More information on the protocol is writtien in docs/misc/overlay.txt file.
> 
> 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>
> ---
>  tools/helpers/Makefile      |   8 +
>  tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 401 insertions(+)
>  create mode 100644 tools/helpers/get_overlay.c

As mentioned before on various occasions - new files preferably use dashes as
separators in preference to underscores. You not doing so is particularly
puzzling seeing ...

> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>  endif
>  ifeq ($(CONFIG_ARM),y)
>  TARGETS += init-dom0less
> +TARGETS += get_overlay

... patch context here (demonstrating a whopping 3 dashes used in similar
cases).

Jan

Re: [PATCH 11/15] tools/helpers: Add get_overlay

[Henry Wang]

Hi Jan,

On 4/24/2024 2:08 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> This user level application copies the overlay dtbo shared by dom0 while doing
>> overlay node assignment operation. It uses xenstore to communicate with dom0.
>> More information on the protocol is writtien in docs/misc/overlay.txt file.
>>
>> 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>
>> ---
>>   tools/helpers/Makefile      |   8 +
>>   tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 401 insertions(+)
>>   create mode 100644 tools/helpers/get_overlay.c
> As mentioned before on various occasions - new files preferably use dashes as
> separators in preference to underscores. You not doing so is particularly
> puzzling seeing ...
>
>> --- a/tools/helpers/Makefile
>> +++ b/tools/helpers/Makefile
>> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>>   endif
>>   ifeq ($(CONFIG_ARM),y)
>>   TARGETS += init-dom0less
>> +TARGETS += get_overlay
> ... patch context here (demonstrating a whopping 3 dashes used in similar
> cases).

I am not very sure why Vikram used "_" in the original patch. However I 
agree you are correct. Since I am currently doing the follow up of this 
series, I will use "-" in v2 as suggested. Thanks.

Kind regards,
Henry

>
> Jan

Re: [PATCH 11/15] tools/helpers: Add get_overlay

[Stewart Hildebrand]

On 4/24/24 20:43, Henry Wang wrote:
> Hi Jan,
> 
> On 4/24/2024 2:08 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>
>>> This user level application copies the overlay dtbo shared by dom0 while doing
>>> overlay node assignment operation. It uses xenstore to communicate with dom0.
>>> More information on the protocol is writtien in docs/misc/overlay.txt file.
>>>
>>> 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>
>>> ---
>>>   tools/helpers/Makefile      |   8 +
>>>   tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 401 insertions(+)
>>>   create mode 100644 tools/helpers/get_overlay.c
>> As mentioned before on various occasions - new files preferably use dashes as
>> separators in preference to underscores. You not doing so is particularly
>> puzzling seeing ...
>>
>>> --- a/tools/helpers/Makefile
>>> +++ b/tools/helpers/Makefile
>>> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>>>   endif
>>>   ifeq ($(CONFIG_ARM),y)
>>>   TARGETS += init-dom0less
>>> +TARGETS += get_overlay
>> ... patch context here (demonstrating a whopping 3 dashes used in similar
>> cases).
> 
> I am not very sure why Vikram used "_" in the original patch. However I agree you are correct. Since I am currently doing the follow up of this series, I will use "-" in v2 as suggested. Thanks.

Please also add tools/helpers/get-overlay to .gitignore

Re: [PATCH 11/15] tools/helpers: Add get_overlay

[Henry Wang]

Hi Stewart,

On 4/26/2024 9:45 AM, Stewart Hildebrand wrote:
> On 4/24/24 20:43, Henry Wang wrote:
>> Hi Jan,
>>
>> On 4/24/2024 2:08 PM, Jan Beulich wrote:
>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>>
>>>> This user level application copies the overlay dtbo shared by dom0 while doing
>>>> overlay node assignment operation. It uses xenstore to communicate with dom0.
>>>> More information on the protocol is writtien in docs/misc/overlay.txt file.
>>>>
>>>> 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>
>>>> ---
>>>>    tools/helpers/Makefile      |   8 +
>>>>    tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 401 insertions(+)
>>>>    create mode 100644 tools/helpers/get_overlay.c
>>> As mentioned before on various occasions - new files preferably use dashes as
>>> separators in preference to underscores. You not doing so is particularly
>>> puzzling seeing ...
>>>
>>>> --- a/tools/helpers/Makefile
>>>> +++ b/tools/helpers/Makefile
>>>> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>>>>    endif
>>>>    ifeq ($(CONFIG_ARM),y)
>>>>    TARGETS += init-dom0less
>>>> +TARGETS += get_overlay
>>> ... patch context here (demonstrating a whopping 3 dashes used in similar
>>> cases).
>> I am not very sure why Vikram used "_" in the original patch. However I agree you are correct. Since I am currently doing the follow up of this series, I will use "-" in v2 as suggested. Thanks.
> Please also add tools/helpers/get-overlay to .gitignore

Thanks for the reminder! Yes sure I will add it.

Kind regards,
Henry

[PATCH 12/15] get_overlay: remove domU overlay

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Retrieve 4 new parameters from xenstore: overlay name, type, whether it
is a partial overlay and operation. Operation can be "add" or "remove".

Add correspond to existing mode of operation. Remove introduces support
for removing an overlay from a domU.

Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
 tools/helpers/get_overlay.c | 132 +++++++++++++++++++++++++++++++++---
 1 file changed, 123 insertions(+), 9 deletions(-)

diff --git a/tools/helpers/get_overlay.c b/tools/helpers/get_overlay.c
index ca3007570e..daa697ca04 100644
--- a/tools/helpers/get_overlay.c
+++ b/tools/helpers/get_overlay.c
@@ -66,6 +66,33 @@ retry_transaction:
     snprintf(ref, sizeof(ref), "%s", "not_ready");
     snprintf(buf, sizeof(buf), "%s/sender-status", xs_base);
 
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        goto fail_xs_transaction;
+    if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+        goto fail_xs_transaction;
+
+    /* Create overlay-name node. */
+    snprintf(ref, sizeof(ref), "%s", "overlay_node");
+    snprintf(buf, sizeof(buf), "%s/overlay-name", xs_base);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        goto fail_xs_transaction;
+    if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+        goto fail_xs_transaction;
+
+    /* Create overlay-type node. */
+    snprintf(ref, sizeof(ref), "%s", "type");
+    snprintf(buf, sizeof(buf), "%s/overlay-type", xs_base);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        goto fail_xs_transaction;
+    if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+        goto fail_xs_transaction;
+
+    /* Create overlay-partial node. */
+    snprintf(ref, sizeof(ref), "%d", 0);
+    snprintf(buf, sizeof(buf), "%s/overlay-partial", xs_base);
+
     if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
         goto fail_xs_transaction;
     if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
@@ -174,7 +201,7 @@ static bool wait_for_status(struct xs_handle *xs, int fd, char *status_path,
 }
 
 static bool write_page_ref(struct xs_handle *xs, uint32_t *page_ref,
-                           uint32_t num_pages, char *path)
+                           uint32_t num_pages, const char *path)
 {
     xs_transaction_t xs_trans = XBT_NULL;
     char buf[128];
@@ -249,12 +276,69 @@ retry_transaction:
     return true;
 }
 
+static char *get_overlay_ops(struct xs_handle *xs, const char *xs_path)
+{
+    char buf[128];
+    char *ref = NULL;
+    unsigned int len;
+
+    snprintf(buf, sizeof(buf), "%s/overlay-operation", xs_path);
+
+    ref = xs_read(xs, XBT_NULL, buf, &len);
+
+    return ref;
+}
+static char *get_overlay_name(struct xs_handle *xs, const char *xs_path)
+{
+    char buf[128];
+    char *ref = NULL;
+    unsigned int len;
+
+    snprintf(buf, sizeof(buf), "%s/overlay-name", xs_path);
+
+    ref = xs_read(xs, XBT_NULL, buf, &len);
+
+    return ref;
+}
+
+static char *get_overlay_type(struct xs_handle *xs, const char *xs_path)
+{
+    char buf[128];
+    char *ref = NULL;
+    unsigned int len;
+
+    snprintf(buf, sizeof(buf), "%s/overlay-type", xs_path);
+
+    ref = xs_read(xs, XBT_NULL, buf, &len);
+
+    return ref;
+}
+
+static bool get_overlay_partial(struct xs_handle *xs, const char *xs_path)
+{
+    char buf[128];
+    char *ref = NULL;
+    unsigned int len;
+
+    snprintf(buf, sizeof(buf), "%s/overlay-partial", xs_path);
+
+    ref = xs_read(xs, XBT_NULL, buf, &len);
+
+    if (ref) {
+        bool is_partial = atoi(ref);
+        free(ref);
+        return is_partial;
+    }
+
+    return false;
+}
+
 int main(int argc, char **argv)
 {
     void *buffer = NULL;
     int domain ;
     uint32_t *page_refs = NULL;
-    FILE *fptr;
+    FILE *fptr = NULL;
     int dtbo_size = 0;
     const char *path = "data/overlay";
     char receiver_status_path[64] = { };
@@ -263,7 +347,11 @@ int main(int argc, char **argv)
     int rc = 0;
     int fd = 0;
     uint32_t num_pages = 0;
-    xengntshr_handle *gntshr;
+    xengntshr_handle *gntshr = NULL;
+    char *overlay_ops = NULL;
+    char *name = NULL;
+    char *type = NULL;
+    bool is_partial = false;
 
     if (argc < 2) {
        fprintf(stderr,"Please enter domain_id.\n");
@@ -357,16 +445,33 @@ int main(int argc, char **argv)
         goto out;
     }
 
-    if ((fptr = fopen("overlay.dtbo","wb")) == NULL) {
-        fprintf(stderr,"Error! opening file");
+    overlay_ops = get_overlay_ops(xs, path);
+    name = get_overlay_name(xs, path);
+    type = get_overlay_type(xs, path);
+    is_partial = get_overlay_partial(xs, path);
+
+    if (overlay_ops == NULL || name == NULL || type == NULL)
         goto out;
-    }
 
-    printf("Writing to file overlay.dtbo.\n");
+    printf("%s %s %s", overlay_ops, name, type);
+    if (is_partial)
+        printf(" %d", is_partial);
+
+    printf("\n");
 
-    fwrite(buffer, dtbo_size, 1, fptr);
+    if (!strcmp(overlay_ops, "add")) {
 
-    printf("Done writing to file overlay.dtbo \n");
+        if ((fptr = fopen("overlay.dtbo","wb")) == NULL) {
+            fprintf(stderr,"Error! opening file");
+            goto out;
+        }
+
+        printf("Writing to file overlay.dtbo.\n");
+
+        fwrite(buffer, dtbo_size, 1, fptr);
+
+        printf("Done writing to file overlay.dtbo \n");
+    }
 
 out:
     if (fptr)
@@ -375,6 +480,15 @@ out:
     if (page_refs)
         free(page_refs);
 
+    if (overlay_ops)
+        free(overlay_ops);
+
+    if (name)
+        free(name);
+
+    if (type)
+        free(type);
+
     if (xs) {
         close(fd);
 
-- 
2.34.1

[PATCH 13/15] xl/overlay: add remove operation to xenstore

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Add 3 new command line parameters to the xl overlay command: overlay
name, type and partial. Pass these paramters to the domU via xenstore.

Also introduce support for "operation" in xenstore: it can be "add" or
"remove". In case of "remove", the overlay is to be removed from the
domU device tree.

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>
---
 tools/xl/xl_vmcontrol.c | 184 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 173 insertions(+), 11 deletions(-)

diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 2bf76dd389..ddd6e9e370 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1466,8 +1466,123 @@ static uint32_t get_num_pages(struct xs_handle *xs, const char *xs_path)
     return num_pages;
 }
 
+static bool write_overlay_operation(struct xs_handle *xs, char *operation,
+                               char *path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+    char buf[128];
+    char ref[64];
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        return false;
+
+    snprintf(ref, sizeof(ref), "%s", operation);
+    snprintf(buf, sizeof(buf), "%s/overlay-operation", path);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        return false;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return false;
+    }
+
+    return true;
+}
+
+static bool write_overlay_name(struct xs_handle *xs, char *name,
+                               char *path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+    char buf[128];
+    char ref[64];
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        return false;
+
+    snprintf(ref, sizeof(ref), "%s", name);
+    snprintf(buf, sizeof(buf), "%s/overlay-name", path);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        return false;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return false;
+    }
+
+    return true;
+}
+
+static bool write_overlay_type(struct xs_handle *xs, char *type,
+                               char *path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+    char buf[128];
+    char ref[64];
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        return false;
+
+    snprintf(ref, sizeof(ref), "%s", type);
+    snprintf(buf, sizeof(buf), "%s/overlay-type", path);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        return false;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return false;
+    }
+
+    return true;
+}
+
+static bool write_overlay_partial(struct xs_handle *xs, bool is_partial,
+                                  char *path)
+{
+    xs_transaction_t xs_trans = XBT_NULL;
+    char buf[128];
+    char ref[4];
+
+retry_transaction:
+    xs_trans = xs_transaction_start(xs);
+    if (!xs_trans)
+        return false;
+
+    snprintf(ref, sizeof(ref), "%d", is_partial);
+    snprintf(buf, sizeof(buf), "%s/overlay-partial", path);
+
+    if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+        return false;
+
+    if (!xs_transaction_end(xs, xs_trans, 0)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return false;
+    }
+
+    return true;
+}
+
+
 static int share_overlay_with_domu(void *overlay_dt_domU, int overlay_dt_size,
-                                   int domain_id)
+                                   int domain_id, char *overlay_ops,
+                                   char *overlay_name,
+                                   char *overlay_type, bool is_overlay_partial)
 {
     struct xs_handle *xs = NULL;
     char *path = NULL;
@@ -1574,6 +1689,34 @@ static int share_overlay_with_domu(void *overlay_dt_domU, int overlay_dt_size,
         goto out;
     }
 
+    /* write overlay ops */
+    if (!write_overlay_operation(xs, overlay_ops, path)) {
+        err = ERROR_FAIL;
+        fprintf(stderr,"Writing overlay_ops ready failed\n");
+        goto out;
+    }
+
+    /* Write the overlay-name. */
+    if (!write_overlay_name(xs, overlay_name, path)) {
+        err = ERROR_FAIL;
+        fprintf(stderr,"Writing overlay_name ready failed\n");
+        goto out;
+    }
+
+    /* Write the overlay-type. */
+    if (!write_overlay_type(xs, overlay_type, path)) {
+        err = ERROR_FAIL;
+        fprintf(stderr,"Writing overlay_type ready failed\n");
+        goto out;
+    }
+
+    /* Write the overlay-partial. */
+    if (!write_overlay_partial(xs, is_overlay_partial, path)) {
+        err = ERROR_FAIL;
+        fprintf(stderr,"Writing overlay_partial ready failed\n");
+        goto out;
+    }
+
     /* Write the status "done". */
     if (!write_status(xs, "done", sender_status_path)) {
         fprintf(stderr,"Writing status DONE failed\n");
@@ -1611,13 +1754,16 @@ int main_dt_overlay(int argc, char **argv)
     int overlay_dtb_size = 0;
     const int overlay_add_op = 1;
     const int overlay_remove_op = 2;
+    char *overlay_name = "overlay";
+    char *overlay_type = "normal";
+    bool is_overlay_partial = false;
 
     if (argc < 3) {
         help("dt-overlay");
         return EXIT_FAILURE;
     }
 
-    if (argc > 5) {
+    if (argc > 7) {
         fprintf(stderr, "Too many arguments\n");
         return ERROR_FAIL;
     }
@@ -1625,17 +1771,22 @@ int main_dt_overlay(int argc, char **argv)
     overlay_ops = argv[1];
     overlay_config_file = argv[2];
 
-    if (!strcmp(argv[argc - 1], "-e"))
-        auto_mode = false;
-
-    if (argc == 4 || !auto_mode) {
+    if (argc == 4 ) {
         domain_id = find_domain(argv[argc-1]);
         domain_mapping = true;
-    }
-
-    if (argc == 5 || !auto_mode) {
-        domain_id = find_domain(argv[argc-2]);
+    } else if (argc == 5 && !strcmp(argv[4], "-e")) {
+        domain_id = find_domain(argv[3]);
+        auto_mode = false;
         domain_mapping = true;
+    } else if (argc == 7) {
+        domain_id = find_domain(argv[3]);
+        domain_mapping = true;
+        overlay_name = argv[4];
+        overlay_type = argv[5];
+        is_overlay_partial = atoi(argv[6]);
+    } else {
+        fprintf(stderr, "Invalid arguments\n");
+        return ERROR_FAIL;
     }
 
     /* User didn't prove any overlay operation. */
@@ -1678,7 +1829,18 @@ int main_dt_overlay(int argc, char **argv)
 
     if (domain_id && auto_mode && (op == LIBXL_DT_OVERLAY_ADD))
         rc = share_overlay_with_domu(overlay_dtb, overlay_dtb_size,
-                                     domain_id);
+                                     domain_id, "add", overlay_name,
+                                     overlay_type, is_overlay_partial);
+
+    if (rc) {
+        free(overlay_dtb);
+        return rc;
+    }
+
+    if (domain_id && auto_mode && (op == LIBXL_DT_OVERLAY_REMOVE))
+        rc = share_overlay_with_domu(overlay_dtb, overlay_dtb_size,
+                                     domain_id, "remove", overlay_name,
+                                     overlay_type, is_overlay_partial);
 
     free(overlay_dtb);
     return rc;
-- 
2.34.1

[PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

Introduce a shell script that runs in the background and calls
get_overlay to retrive overlays and add them (or remove them) to Linux
device tree (running as a domU).

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>
---
 tools/helpers/Makefile       |  2 +-
 tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100755 tools/helpers/get_overlay.sh

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index dfe17ef269..2d0558aeb8 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
 get_overlay: $(SHARE_OVERLAY_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
 
-
 .PHONY: install
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
@@ -67,6 +66,7 @@ install: all
 .PHONY: uninstall
 uninstall:
 	for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
+	$(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
 
 .PHONY: clean
 clean:
diff --git a/tools/helpers/get_overlay.sh b/tools/helpers/get_overlay.sh
new file mode 100755
index 0000000000..2e8c6ecefd
--- /dev/null
+++ b/tools/helpers/get_overlay.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+modprobe xen_gntalloc
+modprobe xen_gntdev
+
+while :
+do
+    overlay_node_name=""
+    type_overlay="normal"
+    is_partial_dtb=""
+
+    output=`/usr/lib/xen/bin/get_overlay 0`
+
+    if test $? -ne 0
+    then
+        echo error
+        exit 1
+    fi
+
+    if test -z "$output"
+    then
+        echo ""
+        exit 1
+    fi
+
+    # output: add overlay-name normal partial
+    operation=`echo $output | cut -d " " -f 1`
+    overlay_node_name=`echo $output | cut -d " " -f 2`
+    type_overlay=`echo $output | cut -d " " -f 3`
+    is_partial_dtb=`echo $output | cut -d " " -f 4`
+
+    if test -z "$operation" || test -z "$overlay_node_name"
+    then
+        echo "invalid ops"
+        exit 1
+    fi
+
+    if test $operation = "add"
+    then
+        echo "Overlay received"
+
+        if test "$type_overlay" = "normal"
+        then
+            final_path="/sys/kernel/config/device-tree/overlays/$overlay_node_name"
+            mkdir -p $final_path
+            cat overlay.dtbo > $final_path/dtbo
+        else
+            # fpga overlay
+            cp overlay.dtbo lib/firmware/
+            mkdir /configfs
+            mount -t configfs configfs /configfs
+            cd /configfs/device-tree/overlays/
+
+            if test "$is_partial_dtb"
+            then
+                mkdir partial
+                echo 1 > /sys/class/fpga_manager/fpga0/flags
+                echo -n "overlay.dtbo" > /configfs/device-tree/overlays/partial
+            else
+                mkdir full
+                echo -n "overlay.dtbo" > /configfs/device-tree/overlays/full
+            fi
+        fi
+    elif test $operation = "remove"
+    then
+        if test "$type_overlay" = "normal"
+        then
+            # implement remove
+            path=/sys/kernel/config/device-tree/overlays/$overlay_node_name/dtbo
+            if ! test -f $path
+            then
+                echo "error: path doesn't exist"
+                exit 1
+            fi
+            rm $path
+        fi
+    else
+        echo "operation unsupported"
+        exit 1
+    fi
+done
-- 
2.34.1

Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Jan Beulich]

On 24.04.2024 05:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
> 
> Introduce a shell script that runs in the background and calls
> get_overlay to retrive overlays and add them (or remove them) to Linux
> device tree (running as a domU).
> 
> 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>
> ---
>  tools/helpers/Makefile       |  2 +-
>  tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100755 tools/helpers/get_overlay.sh

Besides the same naming issue as in the earlier patch, the script also
looks very Linux-ish. Yet ...

> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>  get_overlay: $(SHARE_OVERLAY_OBJS)
>  	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>  
> -
>  .PHONY: install
>  install: all
>  	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> @@ -67,6 +66,7 @@ install: all
>  .PHONY: uninstall
>  uninstall:
>  	for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
> +	$(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>  
>  .PHONY: clean
>  clean:

... you touching only the uninstall target, it's not even clear to me
how (and under what conditions) the script is going to make it into
$(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
alongside the earlier added get-overlay binary?

Jan

Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Henry Wang]

Hi Jan,

On 4/24/2024 2:16 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Introduce a shell script that runs in the background and calls
>> get_overlay to retrive overlays and add them (or remove them) to Linux
>> device tree (running as a domU).
>>
>> 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>
>> ---
>>   tools/helpers/Makefile       |  2 +-
>>   tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 82 insertions(+), 1 deletion(-)
>>   create mode 100755 tools/helpers/get_overlay.sh
> Besides the same naming issue as in the earlier patch, the script also
> looks very Linux-ish. Yet ...

I will fix the naming issue in v2. Would you mind elaborating a bit more 
about the "Linux-ish" concern? I guess this is because the original use 
case is on Linux, should I do anything about this?

>> --- a/tools/helpers/Makefile
>> +++ b/tools/helpers/Makefile
>> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>>   get_overlay: $(SHARE_OVERLAY_OBJS)
>>   	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>>   
>> -
>>   .PHONY: install
>>   install: all
>>   	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>> @@ -67,6 +66,7 @@ install: all
>>   .PHONY: uninstall
>>   uninstall:
>>   	for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
>> +	$(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>>   
>>   .PHONY: clean
>>   clean:
> ... you touching only the uninstall target, it's not even clear to me
> how (and under what conditions) the script is going to make it into
> $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
> alongside the earlier added get-overlay binary?

You are right, I think the get-overlay binary and this script should be 
installed if DTB overlay is supported. Checking the code, I found 
LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature 
supported in libxl. Do you think it is a good idea to use it to install 
these two files in Makefile? Thanks.

Kind regards,
Henry

>
> Jan

Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Jan Beulich]

On 25.04.2024 02:54, Henry Wang wrote:
> On 4/24/2024 2:16 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>
>>> Introduce a shell script that runs in the background and calls
>>> get_overlay to retrive overlays and add them (or remove them) to Linux
>>> device tree (running as a domU).
>>>
>>> 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>
>>> ---
>>>   tools/helpers/Makefile       |  2 +-
>>>   tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 82 insertions(+), 1 deletion(-)
>>>   create mode 100755 tools/helpers/get_overlay.sh
>> Besides the same naming issue as in the earlier patch, the script also
>> looks very Linux-ish. Yet ...
> 
> I will fix the naming issue in v2. Would you mind elaborating a bit more 
> about the "Linux-ish" concern? I guess this is because the original use 
> case is on Linux, should I do anything about this?

Well, the script won't work on other than Linux, will it? Therefore ...

>>> --- a/tools/helpers/Makefile
>>> +++ b/tools/helpers/Makefile
>>> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>>>   get_overlay: $(SHARE_OVERLAY_OBJS)
>>>   	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>>>   
>>> -
>>>   .PHONY: install
>>>   install: all
>>>   	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>>> @@ -67,6 +66,7 @@ install: all
>>>   .PHONY: uninstall
>>>   uninstall:
>>>   	for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
>>> +	$(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>>>   
>>>   .PHONY: clean
>>>   clean:
>> ... you touching only the uninstall target, it's not even clear to me
>> how (and under what conditions) the script is going to make it into
>> $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
>> alongside the earlier added get-overlay binary?

... it first of needs to become clear under what conditions it is actually
going to be installed.

> You are right, I think the get-overlay binary and this script should be 
> installed if DTB overlay is supported. Checking the code, I found 
> LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature 
> supported in libxl. Do you think it is a good idea to use it to install 
> these two files in Makefile? Thanks.

Counter question: If it's not going to be installed, how are people going
to make use of it? If the script is intended for manual use only, I think
that would want saying in the description. Yet then I couldn't see why
the uninstall goal would need modifying.

As to LIBXL_HAVE_DT_OVERLAY - that's not accessible from a Makefile, I
guess?

Jan

Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux

[Henry Wang]

Hi Jan,

On 4/25/2024 2:46 PM, Jan Beulich wrote:
> On 25.04.2024 02:54, Henry Wang wrote:
>> On 4/24/2024 2:16 PM, Jan Beulich wrote:
>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>>
>>>> Introduce a shell script that runs in the background and calls
>>>> get_overlay to retrive overlays and add them (or remove them) to Linux
>>>> device tree (running as a domU).
>>>>
>>>> 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>
>>>> ---
>>>>    tools/helpers/Makefile       |  2 +-
>>>>    tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 82 insertions(+), 1 deletion(-)
>>>>    create mode 100755 tools/helpers/get_overlay.sh
>>> Besides the same naming issue as in the earlier patch, the script also
>>> looks very Linux-ish. Yet ...
>> I will fix the naming issue in v2. Would you mind elaborating a bit more
>> about the "Linux-ish" concern? I guess this is because the original use
>> case is on Linux, should I do anything about this?
> Well, the script won't work on other than Linux, will it? Therefore ...
>
>>>> --- a/tools/helpers/Makefile
>>>> +++ b/tools/helpers/Makefile
>>>> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>>>>    get_overlay: $(SHARE_OVERLAY_OBJS)
>>>>    	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>>>>    
>>>> -
>>>>    .PHONY: install
>>>>    install: all
>>>>    	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>>>> @@ -67,6 +66,7 @@ install: all
>>>>    .PHONY: uninstall
>>>>    uninstall:
>>>>    	for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
>>>> +	$(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>>>>    
>>>>    .PHONY: clean
>>>>    clean:
>>> ... you touching only the uninstall target, it's not even clear to me
>>> how (and under what conditions) the script is going to make it into
>>> $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
>>> alongside the earlier added get-overlay binary?
> ... it first of needs to become clear under what conditions it is actually
> going to be installed.
>
>> You are right, I think the get-overlay binary and this script should be
>> installed if DTB overlay is supported. Checking the code, I found
>> LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature
>> supported in libxl. Do you think it is a good idea to use it to install
>> these two files in Makefile? Thanks.
> Counter question: If it's not going to be installed, how are people going
> to make use of it? If the script is intended for manual use only, I think
> that would want saying in the description. Yet then I couldn't see why
> the uninstall goal would need modifying.

Checking the code again, I feel like this is a mistake actually. I think 
this script should be installed together with the get-overlay 
application as the script actually calls get-overlay. The uninstall goal 
should remain untouched. I will fix this in v2.

> As to LIBXL_HAVE_DT_OVERLAY - that's not accessible from a Makefile, I
> guess?

Yes.

Kind regards,
Henry

>
> Jan

[PATCH 15/15] docs: add device tree overlay documentation

[Henry Wang]

From: Vikram Garhwal <fnu.vikram@xilinx.com>

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>
---
 docs/misc/arm/overlay.txt | 172 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 172 insertions(+)
 create mode 100644 docs/misc/arm/overlay.txt

diff --git a/docs/misc/arm/overlay.txt b/docs/misc/arm/overlay.txt
new file mode 100644
index 0000000000..0351f82a19
--- /dev/null
+++ b/docs/misc/arm/overlay.txt
@@ -0,0 +1,172 @@
+# Device Tree Overlays support in Xen
+
+Xen now supports dynamic device assignment to running domains i.e.
+adding/removing nodes (using .dtbo) in Xen device tree, and assigning
+them to a running domain with given $domid.
+
+Xen supports two modes of operation: normal mode and expert mode for
+assigning nodes to domU. More on this below.
+
+Dynamic node assignment works in following ways:
+
+1. Xen tools check the dtbo given and parse all other user provided arguments
+2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
+3. Xen hypervisor applies the dtbo to Xen device tree and assign the
+   dbto's node resources to the user-provided $domid.
+4. For normal mode,  Xen tools share the modified dtbo with domU. domU needs
+   to run get_overlay.sh to get the dtbo from dom0 and apply the
+   overlay. get_overlay.sh uses get_overlay application for data transfer
+   between dom0 and domU.
+
+
+# Overlay Sharing protocol based on Xenstore
+
+The overlay sharing protocol with domU works in the following ways:
+
+1. get_overlay creates Xenstore path data/overlay and creates the
+   following nodes under data/overlay path:
+       a. receiver-status
+       b. sender-status
+       c. overlay-size
+       d. overlay-name
+       e. overlay-type
+       f. overlay-partial
+   and write "waiting" on receiver-status and "not_ready" to sender_status.
+
+2. libxl waits for "waiting" status on receiver-status, then writes
+   "overlay-size" with dtbo size and "ready" on "sender-status".
+
+3. get_overlay waits for "sender-status" to "ready", then allocates the
+   pages, next it shares the pages with dom0 (the page-ref num) by creating
+   page-ref node under /data/overlay and finally writes "page-refs" to
+   "receiver_status".
+
+4. libxl waits for "receiver-status" to become "page-refs" and copies
+   the data to buffer allocated with page_refs. libxl also writes the
+   "overlay-name", "overlay-type", and "overlay-partial" nodes with
+   user-provided information.  Lastly, libxl writes "done" to
+   "sender-status".
+
+6. get_overlay waits for "sender-status" to be "done".
+
+7. get_overlay copies the data and writes it to file.
+
+8. Finally, get_overlay unshares the pages with dom0.
+
+Note: get_overlay application needs two drivers xen_gntdev and xen_gntalloc in
+Linux. These can be loaded using modprobe xen_gntalloc and modprobe xen_gntdev.
+
+
+# Examples
+
+Here are a few examples on how to use it.
+
+## Dom0 device add
+
+For assigning a device tree overlay to dom0, enter the following:
+
+    (dom0) xl dt-overlay add overlay.dtbo 0
+
+This will allocate the devices mentioned in overlay.dtbo to Xen device
+tree and will assign these devices to dom0.
+
+Next, if the user wants to add the same device tree overlay to dom0
+Linux, execute the following:
+
+    (dom0) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
+    (dom0) cat overlay.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
+
+Finally if needed, the relevant Linux kernel drive can be loaded using:
+
+    (dom0) modprobe module_name.ko
+
+
+## Dom0 device remove
+
+For removing the device from dom0, do the following:
+
+    (dom0) xl dt-overlay remove overlay.dtbo
+
+NOTE: The user is expected to unload any Linux kernel modules which
+might be accessing the devices in overlay.dtbo. Removing devices without
+unloading the modules might result in a crash.
+
+The following is an incorrect sequence:
+
+    (dom0) xl dt-overlay add overlay.dtbo 0
+    (dom0) xl dt-overlay remove overlay.dtbo
+
+The last command only removed the nodes from the Xen dtb but it did not
+deassigning irq/iommus from dom0. This will result in unhandled
+behavior. The correct sequence is to deassign the nodes from dom0:
+
+    (dom0) xl dt-overlay remove overlay.dtbo 0
+
+
+## DomU device add/remove
+
+There are two modes supported for domU use cases: expert mode and normal
+mode.
+
+
+### Expert mode
+
+All the nodes in dtbo will be assigned to a domain; the user will need
+to prepare the dtb for the domU. User will also need to modprobe the
+relevant drivers.
+
+Example for domU device add:
+
+    (dom0) xl dt-overlay add overlay.dtbo $domid
+    (dom0) xl console $domid  # to access $domid console
+
+Next, if the user needs to modify/prepare the overlay.dtbo suitable for
+the domU:
+
+    (domU) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
+    (domU) cat overlay_linux.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
+
+Finally, if needed, the relevant Linux kernel drive can be probed:
+
+    (domU) modprobe module_name.ko
+
+Example for domU overlay remove:
+
+    (dom0) xl dt-overlay remove overlay.dtbo $domid
+
+
+### Normal mode
+
+Libxl modifies the dtbo suitable for the domU. Currently, it does basic
+modifications like updating "target-path" and "interrupt-parent" to make
+them compatible with the domU device tree. Please note that this might
+not work for nodes which need more complex adjustments. The user needs
+to make any required changes for complex overlays and modprobe the
+required Linux modules.
+
+For normal mode, the user is also required to input below three parameters:
+
+a. overlay_node: the name
+b. overlay_type: whether the nodes are fpga nodes or normal nodes
+c. partial or full overlay type: only needed for fpga overlays
+
+Example for domU overlay add:
+
+    (dom0) xl dt-overlay add overlay.dtbo $domid overlay_node_name overlay_type is_partial
+    (dom0) xl console $domid  # go to $domid console
+    (domU) ./usr/lib/xen/bin/get_overlay.sh
+
+Finally if needed, the relevant Linux kernel drive can be loaded:
+
+    (domU) modprobe module_name.ko
+
+Example for domU overlay remove:
+
+    (dom0) xl dt-overlay remove overlay.dtbo $domid overlay_node_name overlay_type is_partial
+
+The get_overlay.sh script automates the following:
+
+a. gets the modified overlay.dtbo from dom0 using Xenstore transactions
+b. applies overlay.dtbo to the domU Linux device tree depending on what
+   type of overlay it is
+c. removes the overlay nodes from device tree when the user requests it
-- 
2.34.1

Re: [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo

[Jan Beulich]

On 24.04.2024 05:34, Henry Wang wrote:
> Hi all,
> 
> This is the remaining series for the full functional "dynamic node
> programming using overlay dtbo" feature. The first part [1] has
> already been merged.
> 
> Quoting from the original series, the first part has already made
> Xen aware of new device tree node which means updating the dt_host
> with overlay node information, and in this series, the goal is to
> map IRQ and IOMMU during runtime, where we will do the actual IOMMU
> and IRQ mapping to a running domain and will call unmap_mmio_regions()
> to remove the mapping.
> 
> Also, documentation of the "dynamic node programming using overlay dtbo"
> feature is added.
> 
> Patch 1 is a fix of [1] which is noticed during my local test, details
> please see the commit message.
> 
> Gitlab CI for this series can be found in [2].
> 
> [1] https://lore.kernel.org/xen-devel/20230906011631.30310-1-vikram.garhwal@amd.com/
> [2] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1265297506
> 
> Henry Wang (1):
>   xen/commom/dt-overlay: Fix missing lock when remove the device
> 
> Vikram Garhwal (14):
>   xen/arm/gic: Enable interrupt assignment to running VM
>   xen/arm: Always enable IOMMU
>   tools/libs/light: Always enable IOMMU
>   tools/libs/light: Increase nr_spi to 160
>   rangeset: Move struct range and struct rangeset to headerfile
>   xen/overlay: Enable device tree overlay assignment to running domains
>   tools: Add domain_id and expert mode for overlay operations
>   tools/libs/light: Modify dtbo to domU linux dtbo format
>   tools/xl: Share overlay with domU
>   tools/helpers: Add get_overlay
>   get_overlay: remove domU overlay
>   xl/overlay: add remove operation to xenstore
>   add a domU script to fetch overlays and applying them to linux
>   docs: add device tree overlay documentation

For all the replies I sent, Vikram's addresses - Xilinx or AMD - bounced.
What's the value of Cc-ing dead addresses?

Jan