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, &overlay_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_ATTACH 3
+#define XEN_DOMCTL_DT_OVERLAY_DETACH 4
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 <xen/device_tree.h>
#include <xen/list.h>
#include <xen/rangeset.h>
+#include <public/domctl.h>
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
