diff mbox series

tools/ocaml: Add missing vmtrace_buf_kb field

Message ID 9b9909c9e93cb540b3488c784935acc2bc9e071e.1707343396.git.w1benny@gmail.com (mailing list archive)
State New, archived
Headers show
Series tools/ocaml: Add missing vmtrace_buf_kb field | expand

Commit Message

Petr Beneš Feb. 7, 2024, 10:04 p.m. UTC 
From: Petr Beneš <w1benny@gmail.com>

Add the missing `vmtrace_buf_kb` field to the OCaml bindings to match the
vm.cfg configuration, correcting an oversight from its initial introduction.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/ocaml/libs/xc/xenctrl.ml      |  1 +
 tools/ocaml/libs/xc/xenctrl.mli     |  1 +
 tools/ocaml/libs/xc/xenctrl_stubs.c | 13 +++++++++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

Comments

Christian Lindig Feb. 8, 2024, 9:13 a.m. UTC | #1 
> On 7 Feb 2024, at 22:04, Petr Beneš <w1benny@gmail.com> wrote:
> 
> 
> Add the missing `vmtrace_buf_kb` field to the OCaml bindings to match the
> vm.cfg configuration, correcting an oversight from its initial introduction.
> 
> Signed-off-by: Petr Beneš <w1benny@gmail.com>

Acked-by: Christian Lindig <christian.lindig@cloud.com>

This looks correct from an OCaml perspective. Why was the new field added in the middle of the record type domctl_create_config and thus forcing changes to the index of fields coming later in the record versus just appending the new field to the record type?

The critical bit is using the correct type in "Int32_val(VAL_VMTRACE_BUF_KB)” that matches the type "vmtrace_buf_kb: int32;” - which it does.

—- C
Petr Beneš Feb. 8, 2024, 9:35 p.m. UTC | #2 
> On Thu, Feb 8, 2024 at 10:14 AM Christian Lindig <christian.lindig@cloud.com> wrote:
>
> > On 7 Feb 2024, at 22:04, Petr Beneš <w1benny@gmail.com> wrote:
> >
> >
> > Add the missing `vmtrace_buf_kb` field to the OCaml bindings to match the
> > vm.cfg configuration, correcting an oversight from its initial introduction.
> >
> > Signed-off-by: Petr Beneš <w1benny@gmail.com>
>
> Acked-by: Christian Lindig <christian.lindig@cloud.com>
>
> This looks correct from an OCaml perspective. Why was the new field added in the middle of the record type domctl_create_config and thus forcing changes to the index of fields coming later in the record versus just appending the new field to the record type?
>

To match the position of the field inside of xen_domctl_createdomain.

P.
Jan Beulich Feb. 14, 2024, 7:11 a.m. UTC | #3 
On 08.02.2024 10:13, Christian Lindig wrote:
>> On 7 Feb 2024, at 22:04, Petr Beneš <w1benny@gmail.com> wrote:
>> Add the missing `vmtrace_buf_kb` field to the OCaml bindings to match the
>> vm.cfg configuration, correcting an oversight from its initial introduction.
>>
>> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> 
> Acked-by: Christian Lindig <christian.lindig@cloud.com>
> 
> This looks correct from an OCaml perspective. Why was the new field added in the middle of the record type domctl_create_config and thus forcing changes to the index of fields coming later in the record versus just appending the new field to the record type?
> 
> The critical bit is using the correct type in "Int32_val(VAL_VMTRACE_BUF_KB)” that matches the type "vmtrace_buf_kb: int32;” - which it does.

Is this then perhaps also lacking a

Fixes: 45ba9a7d7688 ("tools/[lib]xl: Add vmtrace_buf_size parameter")

and hence wanting backporting?

Jan
Petr Beneš Feb. 14, 2024, 11:30 a.m. UTC | #4 
On Wed, Feb 14, 2024 at 8:12 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.02.2024 10:13, Christian Lindig wrote:
> >> On 7 Feb 2024, at 22:04, Petr Beneš <w1benny@gmail.com> wrote:
> >> Add the missing `vmtrace_buf_kb` field to the OCaml bindings to match the
> >> vm.cfg configuration, correcting an oversight from its initial introduction.
> >>
> >> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> >
> > Acked-by: Christian Lindig <christian.lindig@cloud.com>
> >
> > This looks correct from an OCaml perspective. Why was the new field added in the middle of the record type domctl_create_config and thus forcing changes to the index of fields coming later in the record versus just appending the new field to the record type?
> >
> > The critical bit is using the correct type in "Int32_val(VAL_VMTRACE_BUF_KB)” that matches the type "vmtrace_buf_kb: int32;” - which it does.
>
> Is this then perhaps also lacking a
>
> Fixes: 45ba9a7d7688 ("tools/[lib]xl: Add vmtrace_buf_size parameter")
>
> and hence wanting backporting?
>
> Jan

In my opinion, yes.

P.
Andrew Cooper Feb. 14, 2024, 11:45 a.m. UTC | #5 
On 14/02/2024 7:11 am, Jan Beulich wrote:
> On 08.02.2024 10:13, Christian Lindig wrote:
>>> On 7 Feb 2024, at 22:04, Petr Beneš <w1benny@gmail.com> wrote:
>>> Add the missing `vmtrace_buf_kb` field to the OCaml bindings to match the
>>> vm.cfg configuration, correcting an oversight from its initial introduction.
>>>
>>> Signed-off-by: Petr Beneš <w1benny@gmail.com>
>> Acked-by: Christian Lindig <christian.lindig@cloud.com>
>>
>> This looks correct from an OCaml perspective. Why was the new field added in the middle of the record type domctl_create_config and thus forcing changes to the index of fields coming later in the record versus just appending the new field to the record type?
>>
>> The critical bit is using the correct type in "Int32_val(VAL_VMTRACE_BUF_KB)” that matches the type "vmtrace_buf_kb: int32;” - which it does.
> Is this then perhaps also lacking a
>
> Fixes: 45ba9a7d7688 ("tools/[lib]xl: Add vmtrace_buf_size parameter")
>
> and hence wanting backporting?

It ought to have been part of that at the time, but I suggest not
backporting.

API changes in Ocaml are all breaking (module compatibility is checked
by hash).

Xapi is the only consumer of this interface.  I've fixed up the build
against staging, but we're not going to be running KFX under Xapi any
time soon.

Ultimately it's Christian's call.

~Andrew
Christian Lindig Feb. 14, 2024, 1:30 p.m. UTC | #6 
> On 14 Feb 2024, at 11:45, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> Xapi is the only consumer of this interface.  I've fixed up the build
> against staging, but we're not going to be running KFX under Xapi any
> time soon.
> 
> Ultimately it's Christian's call.


After a discussion with Andrew, we will not back port for now as we expect Xapi to use a new version that has it when it becomes relevant.

— C
diff mbox series

Patch

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index d6c6eb73db..55923857ec 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -85,6 +85,7 @@  type domctl_create_config =
     max_grant_frames: int;
     max_maptrack_frames: int;
     max_grant_version: int;
+    vmtrace_buf_kb: int32;
     cpupool_id: int32;
     arch: arch_domainconfig;
   }
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 3bfc16edba..9b4b45db3a 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -77,6 +77,7 @@  type domctl_create_config = {
   max_grant_frames: int;
   max_maptrack_frames: int;
   max_grant_version: int;
+  vmtrace_buf_kb: int32;
   cpupool_id: int32;
   arch: arch_domainconfig;
 }
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 3703f48c74..2b6d3c09df 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -210,10 +210,17 @@  CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 #define VAL_MAX_GRANT_FRAMES    Field(config, 6)
 #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
 #define VAL_MAX_GRANT_VERSION   Field(config, 8)
-#define VAL_CPUPOOL_ID          Field(config, 9)
-#define VAL_ARCH                Field(config, 10)
+#define VAL_VMTRACE_BUF_KB      Field(config, 9)
+#define VAL_CPUPOOL_ID          Field(config, 10)
+#define VAL_ARCH                Field(config, 11)
 
 	uint32_t domid = Int_val(wanted_domid);
+	uint64_t vmtrace_size = Int32_val(VAL_VMTRACE_BUF_KB);
+
+	vmtrace_size = ROUNDUP(vmtrace_size << 10, XC_PAGE_SHIFT);
+	if ( vmtrace_size != (uint32_t)vmtrace_size )
+		caml_invalid_argument("vmtrace_buf_kb");
+
 	int result;
 	struct xen_domctl_createdomain cfg = {
 		.ssidref = Int32_val(VAL_SSIDREF),
@@ -223,6 +230,7 @@  CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 		.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
 		.grant_opts =
 		    XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
+		.vmtrace_size = vmtrace_size,
 		.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
 	};
 
@@ -279,6 +287,7 @@  CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 
 #undef VAL_ARCH
 #undef VAL_CPUPOOL_ID
+#undef VAL_VMTRACE_BUF_KB
 #undef VAL_MAX_GRANT_VERSION
 #undef VAL_MAX_MAPTRACK_FRAMES
 #undef VAL_MAX_GRANT_FRAMES