[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 11/36] xen/include: define hypercall parameter for coloring



Hi,

On 04/03/2022 17:46, Marco Solieri wrote:
From: Luca Miccio <lucmiccio@xxxxxxxxx>

During domU creation process the colors selection has to be passed to
the Xen hypercall.
This is generally done using what Xen calls GUEST_HANDLE_PARAMS. In this
case a simple bitmask for the coloring configuration suffices.
Currently the maximum amount of supported colors is 128.
Add a new parameter that allows us to pass both the colors bitmask
and the number of elements in it.

Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
---
  xen/arch/arm/include/asm/coloring.h | 2 --
  xen/include/public/arch-arm.h       | 8 ++++++++
I would prefer if the structure is defined in the same patch that will use it. This would make easier to figure out if the structure is indeed suitable.

  2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/coloring.h 
b/xen/arch/arm/include/asm/coloring.h
index fdd46448d7..1f7e0dde79 100644
--- a/xen/arch/arm/include/asm/coloring.h
+++ b/xen/arch/arm/include/asm/coloring.h
@@ -23,8 +23,6 @@
  #ifndef __ASM_ARM_COLORING_H__
  #define __ASM_ARM_COLORING_H__
-#define MAX_COLORS_CELLS 4
-

In general, we should avoid moving code that was introduced within the same series.

In this case, I am not convinced we should use a static array to communicate the information between the toolstack and Xen.

This would make more difficult for the user to tweak update the number of colors.

Instead, I think it should be better to expose to the toolstack the number of color supported and allocate a dynamic array.

  #ifdef CONFIG_COLORING
  #include <xen/sched.h>
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 94b31511dd..627cc42164 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -303,6 +303,12 @@ struct vcpu_guest_context {
  typedef struct vcpu_guest_context vcpu_guest_context_t;
  DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
+#define MAX_COLORS_CELLS 4
+struct color_guest_config {
+    uint32_t max_colors;
+    uint32_t colors[MAX_COLORS_CELLS];
+};

This looks like an open-coded version of xenctl_bitmap. Can you have a look to use it?

I would expect this will reduce how much code you introduced in the next patch.

+
  /*
   * struct xen_arch_domainconfig's ABI is covered by
   * XEN_DOMCTL_INTERFACE_VERSION.
@@ -335,6 +341,8 @@ struct xen_arch_domainconfig {
       *
       */
      uint32_t clock_frequency;
+    /* IN */
+    struct color_guest_config colors;
  };
  #endif /* __XEN__ || __XEN_TOOLS__ */

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.