Re: [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations

2024-05-17 Thread Jan Beulich
On 17.05.2024 03:36, Henry Wang wrote:
> On 5/16/2024 8:31 PM, Jan Beulich wrote:
>> On 16.05.2024 12:03, Henry Wang wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
>>>   typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
>>>   DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
>>>   
>>> +#if defined(__arm__) || defined (__aarch64__)
>> Nit: Consistent use of blanks please (also again below).
> 
> Good catch. Will fix it.
> 
>>> +struct xen_domctl_dt_overlay {
>>> +XEN_GUEST_HANDLE_64(const_void) overlay_fdt;  /* IN: overlay fdt. */
>>> +uint32_t overlay_fdt_size;  /* IN: Overlay dtb size. */
>>> +#define XEN_DOMCTL_DT_OVERLAY_ATTACH3
>>> +#define XEN_DOMCTL_DT_OVERLAY_DETACH4
>> While the numbers don't really matter much, picking 3 and 4 rather than,
>> say, 1 and 2 still looks a little odd.
> 
> Well although I agree with you it is indeed a bit odd, the problem of 
> this is that, in current implementation I reused the libxl_dt_overlay() 
> (with proper backward compatible) to deliver the sysctl and domctl 
> depend on the op, and we have:
> #define LIBXL_DT_OVERLAY_ADD   1
> #define LIBXL_DT_OVERLAY_REMOVE    2
> #define LIBXL_DT_OVERLAY_ATTACH    3
> #define LIBXL_DT_OVERLAY_DETACH    4
> 
> Then the op-number is passed from the toolstack to Xen, and checked in 
> dt_overlay_domctl(). So with this implementation the attach/detach op 
> number should be 3 and 4 since 1 and 2 have different meanings.
> 
> But I realized that I can also implement a similar API, say 
> libxl_dt_overlay_domain() and that way we can reuse 1 and 2 and there is 
> not even need to provide backward compatible of libxl_dt_overlay(). So 
> would you mind sharing your preference on which approach would you like 
> more? Thanks!

While I think tying together libxl and domctl values isn't a very good idea,
if you really want to do so, you'll want to add suitable checking somewhere,
alongside comments. The comments ought to keep people from changing the
values then, while the checks would need to be there for people not paying
attention.

Jan



Re: [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations

2024-05-16 Thread Henry Wang

Hi Jan,

As usual, thanks for the review!

On 5/16/2024 8:31 PM, Jan Beulich wrote:

On 16.05.2024 12:03, Henry Wang wrote:

+/*
+ * First check if dtbo is correct i.e. it should one of the dtbo which was
+ * used when dynamically adding the node.
+ * Limitation: Cases with same node names but different property are not
+ * supported currently. We are relying on user to provide the same dtbo
+ * as it was used when adding the nodes.
+ */
+list_for_each_entry_safe( entry, temp, _tracker, entry )
+{
+if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
+{
+track = entry;
Random question (not doing a full review of the DT code): What use is
this (and the track variable itself)? It's never used further down afaics.
Same for attach.


I think you are correct, it is a copy paste of the existing code and the 
track variable is indeed useless. So in v3, I will simply drop it and 
mention this clean-up in commit message. Also I realized that the exact 
logic of finding the entry is duplicated third times, so I will also 
extract the logic to a function.



--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
  typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
  
+#if defined(__arm__) || defined (__aarch64__)

Nit: Consistent use of blanks please (also again below).


Good catch. Will fix it.


+struct xen_domctl_dt_overlay {
+XEN_GUEST_HANDLE_64(const_void) overlay_fdt;  /* IN: overlay fdt. */
+uint32_t overlay_fdt_size;  /* IN: Overlay dtb size. */
+#define XEN_DOMCTL_DT_OVERLAY_ATTACH3
+#define XEN_DOMCTL_DT_OVERLAY_DETACH4

While the numbers don't really matter much, picking 3 and 4 rather than,
say, 1 and 2 still looks a little odd.


Well although I agree with you it is indeed a bit odd, the problem of 
this is that, in current implementation I reused the libxl_dt_overlay() 
(with proper backward compatible) to deliver the sysctl and domctl 
depend on the op, and we have:

#define LIBXL_DT_OVERLAY_ADD   1
#define LIBXL_DT_OVERLAY_REMOVE    2
#define LIBXL_DT_OVERLAY_ATTACH    3
#define LIBXL_DT_OVERLAY_DETACH    4

Then the op-number is passed from the toolstack to Xen, and checked in 
dt_overlay_domctl(). So with this implementation the attach/detach op 
number should be 3 and 4 since 1 and 2 have different meanings.


But I realized that I can also implement a similar API, say 
libxl_dt_overlay_domain() and that way we can reuse 1 and 2 and there is 
not even need to provide backward compatible of libxl_dt_overlay(). So 
would you mind sharing your preference on which approach would you like 
more? Thanks!



--- a/xen/include/xen/dt-overlay.h
+++ b/xen/include/xen/dt-overlay.h
@@ -14,6 +14,7 @@
  #include 
  #include 
  #include 
+#include 

Why? All you need here ...


@@ -42,12 +43,18 @@ struct xen_sysctl_dt_overlay;
  
  #ifdef CONFIG_OVERLAY_DTB

  long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
+long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);

... is a forward declaration of struct xen_domctl_dt_overlay.


Oh indeed. Will fix this. Thanks!

Kind regards,
Henry



Jan





Re: [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations

2024-05-16 Thread Jan Beulich
On 16.05.2024 12:03, Henry Wang wrote:
> +static long handle_detach_overlay_nodes(struct domain *d,
> +const void *overlay_fdt,
> +uint32_t overlay_fdt_size)
> +{
> +int rc;
> +unsigned int j;
> +struct overlay_track *entry, *temp, *track;
> +bool found_entry = false;
> +
> +rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> +if ( rc )
> +return rc;
> +
> +spin_lock(_lock);
> +
> +/*
> + * First check if dtbo is correct i.e. it should one of the dtbo which 
> was
> + * used when dynamically adding the node.
> + * Limitation: Cases with same node names but different property are not
> + * supported currently. We are relying on user to provide the same dtbo
> + * as it was used when adding the nodes.
> + */
> +list_for_each_entry_safe( entry, temp, _tracker, entry )
> +{
> +if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> +{
> +track = entry;

Random question (not doing a full review of the DT code): What use is
this (and the track variable itself)? It's never used further down afaics.
Same for attach.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
>  typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
>  
> +#if defined(__arm__) || defined (__aarch64__)

Nit: Consistent use of blanks please (also again below).

> +struct xen_domctl_dt_overlay {
> +XEN_GUEST_HANDLE_64(const_void) overlay_fdt;  /* IN: overlay fdt. */
> +uint32_t overlay_fdt_size;  /* IN: Overlay dtb size. */
> +#define XEN_DOMCTL_DT_OVERLAY_ATTACH3
> +#define XEN_DOMCTL_DT_OVERLAY_DETACH4

While the numbers don't really matter much, picking 3 and 4 rather than,
say, 1 and 2 still looks a little odd.

> --- a/xen/include/xen/dt-overlay.h
> +++ b/xen/include/xen/dt-overlay.h
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

Why? All you need here ...

> @@ -42,12 +43,18 @@ struct xen_sysctl_dt_overlay;
>  
>  #ifdef CONFIG_OVERLAY_DTB
>  long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
> +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);

... is a forward declaration of struct xen_domctl_dt_overlay.

Jan



[PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations

2024-05-16 Thread Henry Wang
In order to support the dynamic dtbo device assignment to a running
VM, the add/remove of the DT overlay and the attach/detach of the
device from the DT overlay should happen separately. Therefore,
repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
overlay to Xen device tree, instead of assigning the device to the
hardware domain at the same time. Add the XEN_DOMCTL_dt_overlay with
operations XEN_DOMCTL_DT_OVERLAY_{ATTACH,DETACH} to do/undo the
device assignment to the domain.

The hypervisor firstly checks the DT overlay passed from the toolstack
is valid. Then the device nodes are retrieved from the overlay tracker
based on the DT overlay. The attach/detach of the device is implemented
by map/unmap the IRQ and IOMMU resources. Note that with these changes,
the device de-registration from the IOMMU driver should only happen at
the time when the DT overlay is removed from the Xen device tree.

Signed-off-by: Henry Wang 
Signed-off-by: Vikram Garhwal 
---
v2:
- New patch.
---
 xen/arch/arm/domctl.c|   3 +
 xen/common/dt-overlay.c  | 415 ---
 xen/include/public/domctl.h  |  15 ++
 xen/include/public/sysctl.h  |   7 +-
 xen/include/xen/dt-overlay.h |   7 +
 5 files changed, 366 insertions(+), 81 deletions(-)

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index ad56efb0f5..12a12ee781 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2012, Citrix Systems
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -176,6 +177,8 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
domain *d,
 
 return rc;
 }
+case XEN_DOMCTL_dt_overlay:
+return dt_overlay_domctl(d, >u.dt_overlay);
 default:
 return subarch_do_domctl(domctl, d, u_domctl);
 }
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 9cece79067..593e985949 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -356,24 +356,100 @@ static int overlay_get_nodes_info(const void *fdto, char 
**nodes_full_path)
 return 0;
 }
 
+static int remove_irq(unsigned long s, unsigned long e, void *data)
+{
+struct domain *d = data;
+int rc = 0;
+
+/*
+ * 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, s))
+return 0;
+/*
+ * 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, s);
+if ( rc )
+{
+printk(XENLOG_ERR "unable to revoke access for irq %ld\n", s);
+return rc;
+}
+
+rc = release_guest_irq(d, s);
+if ( rc )
+{
+printk(XENLOG_ERR "unable to release irq %ld\n", s);
+return rc;
+}
+
+return rc;
+}
+
+static int remove_all_irqs(struct rangeset *irq_ranges, struct domain *d)
+{
+return rangeset_report_ranges(irq_ranges, 0, ~0UL, remove_irq, d);
+}
+
+static int remove_iomem(unsigned long s, unsigned long e, void *data)
+{
+struct domain *d = data;
+int rc = 0;
+p2m_type_t t;
+mfn_t mfn;
+
+mfn = p2m_lookup(d, _gfn(s), );
+if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )
+return -EINVAL;
+
+rc = iomem_deny_access(d, s, e);
+if ( rc )
+{
+printk(XENLOG_ERR "Unable to remove %pd access to %#lx - %#lx\n",
+   d, s, e);
+return rc;
+}
+
+rc = unmap_mmio_regions(d, _gfn(s), e - s, _mfn(s));
+if ( rc )
+return rc;
+
+return rc;
+}
+
+static int remove_all_iomems(struct rangeset *iomem_ranges, struct domain *d)
+{
+return rangeset_report_ranges(iomem_ranges, 0, ~0UL, remove_iomem, d);
+}
+
 /* 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)
 {
 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);
+dt_dprintk("Checking if node %s is used by any domain\n",
+   device_node->full_name);
 
-/* 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;
+/*
+ * We also check if device is assigned to DOMID_IO as when a domain
+ * is destroyed device is assigned to DOMID_IO.
+