[Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Paul Durrant]

From: George Dunlap <george.dunlap@citrix.com>

Xen used to have single, system-wide limits for the number of grant
frames and maptrack frames a guest was allowed to create. Increasing
or decreasing this single limit on the Xen command-line would change
the limit for all guests on the system.

Later, per-domain limits for these values was created. The system-wide
limits became strict limits: domains could not be created with higher
limits, but could be created with lower limits. However, that change
also introduced a range of different "default" values into various
places in the toolstack:

- The python libxc bindings hard-coded these values to 32 and 1024,
  respectively
- The libxl default values are 32 and 1024 respectively.
- xl will use the libxl default for maptrack, but does its own default
  calculation for grant frames: either 32 or 64, based on the max
  possible mfn.

These defaults interact poorly with the hypervisor command-line limit:

- The hypervisor command-line limit cannot be used to raise the limit
  for all guests anymore, as the default in the toolstack will
  effectively override this.
- If you use the hypervisor command-line limit to *reduce* the limit,
  then the "default" values generated by the toolstack are too high,
  and all guest creations will fail.

In other words, the toolstack defaults require any change to be
effected by having the admin explicitly specify a new value in every
guest.

In order to address this, have grant_table_init treat negative values
for max_grant_frames and max_maptrack_frames as instructions to use the
system-wide default, and have all the above toolstacks default to passing
-1 unless a different value is explicitly configured.

This restores the old behavior in that changing the hypervisor command-line
option can change the behavior for all guests, while retaining the ability
to set per-guest values.  It also removes the bug that reducing the
system-wide max will cause all domains without explicit limits to fail.

NOTE: The Ocaml bindings require the caller to always specify a value, and
      the code to start a xenstored stubdomain hard-codes these to 4 and
      128 respectively; this behavour will not be modified.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

v2:
 - re-worked George's original commit massage a little
 - fixed the text in xl.conf.5.pod
 - use -1 as the sentinel value for 'default' and < 0 for checking it
---
 docs/man/xl.conf.5.pod            |  6 ++++--
 tools/libxl/libxl.h               |  4 ++--
 tools/libxl/libxl_types.idl       |  4 ++--
 tools/python/xen/lowlevel/xc/xc.c |  4 ++--
 tools/xl/xl.c                     |  8 --------
 tools/xl/xl_parse.c               |  3 ++-
 xen/arch/arm/setup.c              |  2 +-
 xen/arch/x86/setup.c              |  4 ++--
 xen/common/grant_table.c          | 10 ++++++++--
 xen/include/public/domctl.h       | 10 ++++++----
 xen/include/xen/grant_table.h     |  8 ++++----
 11 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod
index 962144e38e..207ab3e77a 100644
--- a/docs/man/xl.conf.5.pod
+++ b/docs/man/xl.conf.5.pod
@@ -81,13 +81,15 @@ Default: C</var/lock/xl>
 
 Sets the default value for the C<max_grant_frames> domain config value.
 
-Default: C<32> on hosts up to 16TB of memory, C<64> on hosts larger than 16TB
+Default: value of Xen command line B<gnttab_max_frames> parameter (or its
+default value if unspecified).
 
 =item B<max_maptrack_frames=NUMBER>
 
 Sets the default value for the C<max_maptrack_frames> domain config value.
 
-Default: C<1024>
+Default: value of Xen command line B<gnttab_max_maptrack_frames>
+parameter (or its default value if unspecified).
 
 =item B<vif.default.script="PATH">
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 49b56fa1a3..a2a5d321c5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -364,8 +364,8 @@
  */
 #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
 
-#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
-#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
+#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
+#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
 
 /*
  * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0546d7865a..63e29bb2fb 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
 
-    ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
-    ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
+    ("max_grant_frames",    integer, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
+    ("max_maptrack_frames", integer, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 44d3606141..a751e85910 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -127,8 +127,8 @@ static PyObject *pyxc_domain_create(XcObject *self,
         },
         .max_vcpus = 1,
         .max_evtchn_port = -1, /* No limit. */
-        .max_grant_frames = 32,
-        .max_maptrack_frames = 1024,
+        .max_grant_frames = -1,
+        .max_maptrack_frames = -1,
     };
 
     static char *kwd_list[] = { "domid", "ssidref", "handle", "flags",
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index ddd29b3f1b..08f31cc90e 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -96,7 +96,6 @@ static void parse_global_config(const char *configfile,
     XLU_Config *config;
     int e;
     const char *buf;
-    libxl_physinfo physinfo;
 
     config = xlu_cfg_init(stderr, configfile);
     if (!config) {
@@ -199,13 +198,6 @@ static void parse_global_config(const char *configfile,
 
     if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0))
         max_grant_frames = l;
-    else {
-        libxl_physinfo_init(&physinfo);
-        max_grant_frames = (libxl_get_physinfo(ctx, &physinfo) != 0 ||
-                            !(physinfo.max_possible_mfn >> 32))
-                           ? 32 : 64;
-        libxl_physinfo_dispose(&physinfo);
-    }
     if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
         max_maptrack_frames = l;
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 112f8ee026..7554048f0b 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1415,9 +1415,10 @@ void parse_config_data(const char *config_source,
         b_info->max_grant_frames = l;
     else
         b_info->max_grant_frames = max_grant_frames;
+
     if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
         b_info->max_maptrack_frames = l;
-    else if (max_maptrack_frames != -1)
+    else
         b_info->max_maptrack_frames = max_maptrack_frames;
 
     libxl_defbool_set(&b_info->claim_mode, claim_mode);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 51d32106b7..3c899cd4a0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -789,7 +789,7 @@ void __init start_xen(unsigned long boot_phys_offset,
         .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
         .max_evtchn_port = -1,
         .max_grant_frames = gnttab_dom0_frames(),
-        .max_maptrack_frames = opt_max_maptrack_frames,
+        .max_maptrack_frames = -1,
     };
     int rc;
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 00ee87bde5..7d27f36053 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     struct xen_domctl_createdomain dom0_cfg = {
         .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
         .max_evtchn_port = -1,
-        .max_grant_frames = opt_max_grant_frames,
-        .max_maptrack_frames = opt_max_maptrack_frames,
+        .max_grant_frames = -1,
+        .max_maptrack_frames = -1,
     };
 
     /* Critical region without IDT or TSS.  Any fault is deadly! */
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index b34d520f6d..6972cef1de 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1837,12 +1837,18 @@ active_alloc_failed:
     return -ENOMEM;
 }
 
-int grant_table_init(struct domain *d, unsigned int max_grant_frames,
-                     unsigned int max_maptrack_frames)
+int grant_table_init(struct domain *d, int max_grant_frames,
+                     int max_maptrack_frames)
 {
     struct grant_table *gt;
     int ret = -ENOMEM;
 
+    /* Default to maximum value if no value was specified */
+    if ( max_grant_frames < 0 )
+        max_grant_frames = opt_max_grant_frames;
+    if ( max_maptrack_frames < 0 )
+        max_maptrack_frames = opt_max_maptrack_frames;
+
     if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
          max_grant_frames > opt_max_grant_frames ||
          max_maptrack_frames > opt_max_maptrack_frames )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 9f2cfd602c..e313da499f 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -82,13 +82,15 @@ struct xen_domctl_createdomain {
     uint32_t iommu_opts;
 
     /*
-     * Various domain limits, which impact the quantity of resources (global
-     * mapping space, xenheap, etc) a guest may consume.
+     * Various domain limits, which impact the quantity of resources
+     * (global mapping space, xenheap, etc) a guest may consume.  For
+     * max_grant_frames and max_maptrack_frames, < 0 means "use the
+     * default maximum value in the hypervisor".
      */
     uint32_t max_vcpus;
     uint32_t max_evtchn_port;
-    uint32_t max_grant_frames;
-    uint32_t max_maptrack_frames;
+    int32_t max_grant_frames;
+    int32_t max_maptrack_frames;
 
     struct xen_arch_domainconfig arch;
 };
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 6f9345d9ef..34886bb6f8 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -36,8 +36,8 @@ extern unsigned int opt_max_grant_frames;
 extern unsigned int opt_max_maptrack_frames;
 
 /* Create/destroy per-domain grant table context. */
-int grant_table_init(struct domain *d, unsigned int max_grant_frames,
-                     unsigned int max_maptrack_frames);
+int grant_table_init(struct domain *d, int max_grant_frames,
+                     int max_maptrack_frames);
 void grant_table_destroy(
     struct domain *d);
 void grant_table_init_vcpu(struct vcpu *v);
@@ -68,8 +68,8 @@ int gnttab_get_status_frame(struct domain *d, unsigned long idx,
 #define opt_max_maptrack_frames 0
 
 static inline int grant_table_init(struct domain *d,
-                                   unsigned int max_grant_frames,
-                                   unsigned int max_maptrack_frames)
+                                   int max_grant_frames,
+                                   int max_maptrack_frames)
 {
     return 0;
 }
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Jan Beulich]

On 27.11.2019 15:37, Paul Durrant wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>          .max_evtchn_port = -1,
>          .max_grant_frames = gnttab_dom0_frames(),
> -        .max_maptrack_frames = opt_max_maptrack_frames,
> +        .max_maptrack_frames = -1,
>      };
>      int rc;
>  
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      struct xen_domctl_createdomain dom0_cfg = {
>          .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
>          .max_evtchn_port = -1,
> -        .max_grant_frames = opt_max_grant_frames,
> -        .max_maptrack_frames = opt_max_maptrack_frames,
> +        .max_grant_frames = -1,
> +        .max_maptrack_frames = -1,
>      };

With these there's no need anymore for opt_max_maptrack_frames to
be non-static. Sadly Arm still wants opt_max_grant_frames
accessible in gnttab_dom0_frames().

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1837,12 +1837,18 @@ active_alloc_failed:
>      return -ENOMEM;
>  }
>  
> -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
> -                     unsigned int max_maptrack_frames)
> +int grant_table_init(struct domain *d, int max_grant_frames,
> +                     int max_maptrack_frames)
>  {
>      struct grant_table *gt;
>      int ret = -ENOMEM;
>  
> +    /* Default to maximum value if no value was specified */
> +    if ( max_grant_frames < 0 )
> +        max_grant_frames = opt_max_grant_frames;
> +    if ( max_maptrack_frames < 0 )
> +        max_maptrack_frames = opt_max_maptrack_frames;
> +
>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||

I take it we don't expect people to specify 2^^31 or more
frames for either option. It looks like almost everything
here would cope, except for this very comparison. Nevertheless
I wonder whether you wouldn't better confine both values to
[0, INT_MAX] now, including when adjusted at runtime.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 27 November 2019 15:56
> To: Durrant, Paul <pdurrant@amazon.com>; George Dunlap
> <george.dunlap@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@citrix.com>;
> Roger Pau Monné <roger.pau@citrix.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; George Dunlap <George.Dunlap@eu.citrix.com>;
> Ian Jackson <ian.jackson@eu.citrix.com>; Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>;
> Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On 27.11.2019 15:37, Paul Durrant wrote:
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
> boot_phys_offset,
> >          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> >          .max_evtchn_port = -1,
> >          .max_grant_frames = gnttab_dom0_frames(),
> > -        .max_maptrack_frames = opt_max_maptrack_frames,
> > +        .max_maptrack_frames = -1,
> >      };
> >      int rc;
> >
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
> >      struct xen_domctl_createdomain dom0_cfg = {
> >          .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity
> : 0,
> >          .max_evtchn_port = -1,
> > -        .max_grant_frames = opt_max_grant_frames,
> > -        .max_maptrack_frames = opt_max_maptrack_frames,
> > +        .max_grant_frames = -1,
> > +        .max_maptrack_frames = -1,
> >      };
> 
> With these there's no need anymore for opt_max_maptrack_frames to
> be non-static. Sadly Arm still wants opt_max_grant_frames
> accessible in gnttab_dom0_frames().
>

Yes, I was about to make them static until I saw what the ARM code did.
 
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1837,12 +1837,18 @@ active_alloc_failed:
> >      return -ENOMEM;
> >  }
> >
> > -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
> > -                     unsigned int max_maptrack_frames)
> > +int grant_table_init(struct domain *d, int max_grant_frames,
> > +                     int max_maptrack_frames)
> >  {
> >      struct grant_table *gt;
> >      int ret = -ENOMEM;
> >
> > +    /* Default to maximum value if no value was specified */
> > +    if ( max_grant_frames < 0 )
> > +        max_grant_frames = opt_max_grant_frames;
> > +    if ( max_maptrack_frames < 0 )
> > +        max_maptrack_frames = opt_max_maptrack_frames;
> > +
> >      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> 
> I take it we don't expect people to specify 2^^31 or more
> frames for either option. It looks like almost everything
> here would cope, except for this very comparison. Nevertheless
> I wonder whether you wouldn't better confine both values to
> [0, INT_MAX] now, including when adjusted at runtime.

I can certainly remove the 'U' from the definition of INITIAL_NR_GRANT_FRAMES, but do you want me to make opt_max_grant_frames and opt_max_maptrack_frames into signed ints and add signed parser code too? I also don't understand the 'adjusted at runtime' part.

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Jan Beulich]

On 27.11.2019 17:14,  Durrant, Paul  wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 27 November 2019 15:56
>>
>> On 27.11.2019 15:37, Paul Durrant wrote:
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
>> boot_phys_offset,
>>>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>>>          .max_evtchn_port = -1,
>>>          .max_grant_frames = gnttab_dom0_frames(),
>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
>>> +        .max_maptrack_frames = -1,
>>>      };
>>>      int rc;
>>>
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>>>      struct xen_domctl_createdomain dom0_cfg = {
>>>          .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity
>> : 0,
>>>          .max_evtchn_port = -1,
>>> -        .max_grant_frames = opt_max_grant_frames,
>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
>>> +        .max_grant_frames = -1,
>>> +        .max_maptrack_frames = -1,
>>>      };
>>
>> With these there's no need anymore for opt_max_maptrack_frames to
>> be non-static. Sadly Arm still wants opt_max_grant_frames
>> accessible in gnttab_dom0_frames().
> 
> Yes, I was about to make them static until I saw what the ARM code did.

But the one that Arm doesn't need should become static now.

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
>>>      return -ENOMEM;
>>>  }
>>>
>>> -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
>>> -                     unsigned int max_maptrack_frames)
>>> +int grant_table_init(struct domain *d, int max_grant_frames,
>>> +                     int max_maptrack_frames)
>>>  {
>>>      struct grant_table *gt;
>>>      int ret = -ENOMEM;
>>>
>>> +    /* Default to maximum value if no value was specified */
>>> +    if ( max_grant_frames < 0 )
>>> +        max_grant_frames = opt_max_grant_frames;
>>> +    if ( max_maptrack_frames < 0 )
>>> +        max_maptrack_frames = opt_max_maptrack_frames;
>>> +
>>>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>>
>> I take it we don't expect people to specify 2^^31 or more
>> frames for either option. It looks like almost everything
>> here would cope, except for this very comparison. Nevertheless
>> I wonder whether you wouldn't better confine both values to
>> [0, INT_MAX] now, including when adjusted at runtime.
> 
> I can certainly remove the 'U' from the definition of
> INITIAL_NR_GRANT_FRAMES,

Oh, I didn't pay attention that is has a U on it - in this case
the comparison above is fine.

> but do you want me to make opt_max_grant_frames and
> opt_max_maptrack_frames into signed ints and add signed parser
> code too?

Definitely not. They should remain unsigned quantities, but their
values may need sanity checking now.

> I also don't understand the 'adjusted at runtime' part.

Well, for a command line drive value you could adjust an out of
bounds value in some __init function. But for runtime modifiable
settings you won't get away this easily.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[George Dunlap]

On 11/27/19 4:20 PM, Jan Beulich wrote:
> On 27.11.2019 17:14,  Durrant, Paul  wrote:
>>> From: Jan Beulich <jbeulich@suse.com>
>>> Sent: 27 November 2019 15:56
>>>
>>> On 27.11.2019 15:37, Paul Durrant wrote:
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
>>> boot_phys_offset,
>>>>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>>>>          .max_evtchn_port = -1,
>>>>          .max_grant_frames = gnttab_dom0_frames(),
>>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
>>>> +        .max_maptrack_frames = -1,
>>>>      };
>>>>      int rc;
>>>>
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
>>> mbi_p)
>>>>      struct xen_domctl_createdomain dom0_cfg = {
>>>>          .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity
>>> : 0,
>>>>          .max_evtchn_port = -1,
>>>> -        .max_grant_frames = opt_max_grant_frames,
>>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
>>>> +        .max_grant_frames = -1,
>>>> +        .max_maptrack_frames = -1,
>>>>      };
>>>
>>> With these there's no need anymore for opt_max_maptrack_frames to
>>> be non-static. Sadly Arm still wants opt_max_grant_frames
>>> accessible in gnttab_dom0_frames().
>>
>> Yes, I was about to make them static until I saw what the ARM code did.
> 
> But the one that Arm doesn't need should become static now.
> 
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
>>>>      return -ENOMEM;
>>>>  }
>>>>
>>>> -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
>>>> -                     unsigned int max_maptrack_frames)
>>>> +int grant_table_init(struct domain *d, int max_grant_frames,
>>>> +                     int max_maptrack_frames)
>>>>  {
>>>>      struct grant_table *gt;
>>>>      int ret = -ENOMEM;
>>>>
>>>> +    /* Default to maximum value if no value was specified */
>>>> +    if ( max_grant_frames < 0 )
>>>> +        max_grant_frames = opt_max_grant_frames;
>>>> +    if ( max_maptrack_frames < 0 )
>>>> +        max_maptrack_frames = opt_max_maptrack_frames;
>>>> +
>>>>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>>>
>>> I take it we don't expect people to specify 2^^31 or more
>>> frames for either option. It looks like almost everything
>>> here would cope, except for this very comparison. Nevertheless
>>> I wonder whether you wouldn't better confine both values to
>>> [0, INT_MAX] now, including when adjusted at runtime.
>>
>> I can certainly remove the 'U' from the definition of
>> INITIAL_NR_GRANT_FRAMES,
> 
> Oh, I didn't pay attention that is has a U on it - in this case
> the comparison above is fine.
> 
>> but do you want me to make opt_max_grant_frames and
>> opt_max_maptrack_frames into signed ints and add signed parser
>> code too?
> 
> Definitely not. They should remain unsigned quantities, but their
> values may need sanity checking now.
> 
>> I also don't understand the 'adjusted at runtime' part.
> 
> Well, for a command line drive value you could adjust an out of
> bounds value in some __init function. But for runtime modifiable
> settings you won't get away this easily.

TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
long)(-1) or something, and explicitly compare to that.  That leaves
open the possibility of having more sentinel values if we decided we
wanted them.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Durrant, Paul]

> -----Original Message-----
> From: George Dunlap <george.dunlap@citrix.com>
> Sent: 27 November 2019 16:34
> To: Jan Beulich <jbeulich@suse.com>; Durrant, Paul <pdurrant@amazon.com>
> Cc: AndrewCooper <andrew.cooper3@citrix.com>; Anthony PERARD
> <anthony.perard@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>;
> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Julien Grall
> <julien@xen.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On 11/27/19 4:20 PM, Jan Beulich wrote:
> > On 27.11.2019 17:14,  Durrant, Paul  wrote:
> >>> From: Jan Beulich <jbeulich@suse.com>
> >>> Sent: 27 November 2019 15:56
> >>>
> >>> On 27.11.2019 15:37, Paul Durrant wrote:
> >>>> --- a/xen/arch/arm/setup.c
> >>>> +++ b/xen/arch/arm/setup.c
> >>>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
> >>> boot_phys_offset,
> >>>>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> >>>>          .max_evtchn_port = -1,
> >>>>          .max_grant_frames = gnttab_dom0_frames(),
> >>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
> >>>> +        .max_maptrack_frames = -1,
> >>>>      };
> >>>>      int rc;
> >>>>
> >>>> --- a/xen/arch/x86/setup.c
> >>>> +++ b/xen/arch/x86/setup.c
> >>>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
> >>> mbi_p)
> >>>>      struct xen_domctl_createdomain dom0_cfg = {
> >>>>          .flags = IS_ENABLED(CONFIG_TBOOT) ?
> XEN_DOMCTL_CDF_s3_integrity
> >>> : 0,
> >>>>          .max_evtchn_port = -1,
> >>>> -        .max_grant_frames = opt_max_grant_frames,
> >>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
> >>>> +        .max_grant_frames = -1,
> >>>> +        .max_maptrack_frames = -1,
> >>>>      };
> >>>
> >>> With these there's no need anymore for opt_max_maptrack_frames to
> >>> be non-static. Sadly Arm still wants opt_max_grant_frames
> >>> accessible in gnttab_dom0_frames().
> >>
> >> Yes, I was about to make them static until I saw what the ARM code did.
> >
> > But the one that Arm doesn't need should become static now.
> >
> >>>> --- a/xen/common/grant_table.c
> >>>> +++ b/xen/common/grant_table.c
> >>>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
> >>>>      return -ENOMEM;
> >>>>  }
> >>>>
> >>>> -int grant_table_init(struct domain *d, unsigned int
> max_grant_frames,
> >>>> -                     unsigned int max_maptrack_frames)
> >>>> +int grant_table_init(struct domain *d, int max_grant_frames,
> >>>> +                     int max_maptrack_frames)
> >>>>  {
> >>>>      struct grant_table *gt;
> >>>>      int ret = -ENOMEM;
> >>>>
> >>>> +    /* Default to maximum value if no value was specified */
> >>>> +    if ( max_grant_frames < 0 )
> >>>> +        max_grant_frames = opt_max_grant_frames;
> >>>> +    if ( max_maptrack_frames < 0 )
> >>>> +        max_maptrack_frames = opt_max_maptrack_frames;
> >>>> +
> >>>>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> >>>
> >>> I take it we don't expect people to specify 2^^31 or more
> >>> frames for either option. It looks like almost everything
> >>> here would cope, except for this very comparison. Nevertheless
> >>> I wonder whether you wouldn't better confine both values to
> >>> [0, INT_MAX] now, including when adjusted at runtime.
> >>
> >> I can certainly remove the 'U' from the definition of
> >> INITIAL_NR_GRANT_FRAMES,
> >
> > Oh, I didn't pay attention that is has a U on it - in this case
> > the comparison above is fine.
> >
> >> but do you want me to make opt_max_grant_frames and
> >> opt_max_maptrack_frames into signed ints and add signed parser
> >> code too?
> >
> > Definitely not. They should remain unsigned quantities, but their
> > values may need sanity checking now.
> >
> >> I also don't understand the 'adjusted at runtime' part.
> >
> > Well, for a command line drive value you could adjust an out of
> > bounds value in some __init function. But for runtime modifiable
> > settings you won't get away this easily.
> 
> TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
> long)(-1) or something, and explicitly compare to that.  That leaves
> open the possibility of having more sentinel values if we decided we
> wanted them.

I'm extremely confused now. What do you want me to compare and where?

I assume we're talking about the opt_XXX values. Am I supposed to stop >INT_MAX being assigned to them? Or should I define local unsigned values for max_maptrack/grant_frames and simply initialize them to the passed-in arg (if >= 0) or the opt_XXX value otherwise.

  Paul

> 
>  -George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[George Dunlap]

On 11/27/19 4:43 PM, Durrant, Paul wrote:
>> -----Original Message-----
>> From: George Dunlap <george.dunlap@citrix.com>
>> Sent: 27 November 2019 16:34
>> To: Jan Beulich <jbeulich@suse.com>; Durrant, Paul <pdurrant@amazon.com>
>> Cc: AndrewCooper <andrew.cooper3@citrix.com>; Anthony PERARD
>> <anthony.perard@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
>> <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>;
>> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org;
>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Julien Grall
>> <julien@xen.org>; Wei Liu <wl@xen.org>
>> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
>> max_maptrack_frames handling
>>
>> On 11/27/19 4:20 PM, Jan Beulich wrote:
>>> On 27.11.2019 17:14,  Durrant, Paul  wrote:
>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>> Sent: 27 November 2019 15:56
>>>>>
>>>>> On 27.11.2019 15:37, Paul Durrant wrote:
>>>>>> --- a/xen/arch/arm/setup.c
>>>>>> +++ b/xen/arch/arm/setup.c
>>>>>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
>>>>> boot_phys_offset,
>>>>>>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>>>>>>          .max_evtchn_port = -1,
>>>>>>          .max_grant_frames = gnttab_dom0_frames(),
>>>>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
>>>>>> +        .max_maptrack_frames = -1,
>>>>>>      };
>>>>>>      int rc;
>>>>>>
>>>>>> --- a/xen/arch/x86/setup.c
>>>>>> +++ b/xen/arch/x86/setup.c
>>>>>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
>>>>> mbi_p)
>>>>>>      struct xen_domctl_createdomain dom0_cfg = {
>>>>>>          .flags = IS_ENABLED(CONFIG_TBOOT) ?
>> XEN_DOMCTL_CDF_s3_integrity
>>>>> : 0,
>>>>>>          .max_evtchn_port = -1,
>>>>>> -        .max_grant_frames = opt_max_grant_frames,
>>>>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
>>>>>> +        .max_grant_frames = -1,
>>>>>> +        .max_maptrack_frames = -1,
>>>>>>      };
>>>>>
>>>>> With these there's no need anymore for opt_max_maptrack_frames to
>>>>> be non-static. Sadly Arm still wants opt_max_grant_frames
>>>>> accessible in gnttab_dom0_frames().
>>>>
>>>> Yes, I was about to make them static until I saw what the ARM code did.
>>>
>>> But the one that Arm doesn't need should become static now.
>>>
>>>>>> --- a/xen/common/grant_table.c
>>>>>> +++ b/xen/common/grant_table.c
>>>>>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
>>>>>>      return -ENOMEM;
>>>>>>  }
>>>>>>
>>>>>> -int grant_table_init(struct domain *d, unsigned int
>> max_grant_frames,
>>>>>> -                     unsigned int max_maptrack_frames)
>>>>>> +int grant_table_init(struct domain *d, int max_grant_frames,
>>>>>> +                     int max_maptrack_frames)
>>>>>>  {
>>>>>>      struct grant_table *gt;
>>>>>>      int ret = -ENOMEM;
>>>>>>
>>>>>> +    /* Default to maximum value if no value was specified */
>>>>>> +    if ( max_grant_frames < 0 )
>>>>>> +        max_grant_frames = opt_max_grant_frames;
>>>>>> +    if ( max_maptrack_frames < 0 )
>>>>>> +        max_maptrack_frames = opt_max_maptrack_frames;
>>>>>> +
>>>>>>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>>>>>
>>>>> I take it we don't expect people to specify 2^^31 or more
>>>>> frames for either option. It looks like almost everything
>>>>> here would cope, except for this very comparison. Nevertheless
>>>>> I wonder whether you wouldn't better confine both values to
>>>>> [0, INT_MAX] now, including when adjusted at runtime.
>>>>
>>>> I can certainly remove the 'U' from the definition of
>>>> INITIAL_NR_GRANT_FRAMES,
>>>
>>> Oh, I didn't pay attention that is has a U on it - in this case
>>> the comparison above is fine.
>>>
>>>> but do you want me to make opt_max_grant_frames and
>>>> opt_max_maptrack_frames into signed ints and add signed parser
>>>> code too?
>>>
>>> Definitely not. They should remain unsigned quantities, but their
>>> values may need sanity checking now.
>>>
>>>> I also don't understand the 'adjusted at runtime' part.
>>>
>>> Well, for a command line drive value you could adjust an out of
>>> bounds value in some __init function. But for runtime modifiable
>>> settings you won't get away this easily.
>>
>> TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
>> long)(-1) or something, and explicitly compare to that.  That leaves
>> open the possibility of having more sentinel values if we decided we
>> wanted them.
> 
> I'm extremely confused now. What do you want me to compare and where?
> 
> I assume we're talking about the opt_XXX values. Am I supposed to stop >INT_MAX being assigned to them? Or should I define local unsigned values for max_maptrack/grant_frames and simply initialize them to the passed-in arg (if >= 0) or the opt_XXX value otherwise.

In this version of the patch, you change the domctl arguments from
uint32_t to int32_t.  I would leave them uint32_t, and if (
max_grant_frames == XENSOMETHING_MAX_DEFAULT ) max_grant_frames = opt_&c.

Then the only invalid value we have to worry about is checking for
XENSOMETHING_MAX_DEFAULT.

This is a suggestion, and I wouldn't argue strongly if someone thought
it was a bad idea, but it seems like the most straightforward option to me.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Durrant, Paul]

> -----Original Message-----
> From: George Dunlap <george.dunlap@citrix.com>
> Sent: 27 November 2019 16:52
> To: Durrant, Paul <pdurrant@amazon.com>; Jan Beulich <jbeulich@suse.com>
> Cc: AndrewCooper <andrew.cooper3@citrix.com>; Anthony PERARD
> <anthony.perard@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>;
> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Julien Grall
> <julien@xen.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On 11/27/19 4:43 PM, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: George Dunlap <george.dunlap@citrix.com>
> >> Sent: 27 November 2019 16:34
> >> To: Jan Beulich <jbeulich@suse.com>; Durrant, Paul
> <pdurrant@amazon.com>
> >> Cc: AndrewCooper <andrew.cooper3@citrix.com>; Anthony PERARD
> >> <anthony.perard@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> >> <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>;
> >> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Stefano
> >> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org;
> >> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Julien Grall
> >> <julien@xen.org>; Wei Liu <wl@xen.org>
> >> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
> >> max_maptrack_frames handling
> >>
> >> On 11/27/19 4:20 PM, Jan Beulich wrote:
> >>> On 27.11.2019 17:14,  Durrant, Paul  wrote:
> >>>>> From: Jan Beulich <jbeulich@suse.com>
> >>>>> Sent: 27 November 2019 15:56
> >>>>>
> >>>>> On 27.11.2019 15:37, Paul Durrant wrote:
> >>>>>> --- a/xen/arch/arm/setup.c
> >>>>>> +++ b/xen/arch/arm/setup.c
> >>>>>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
> >>>>> boot_phys_offset,
> >>>>>>          .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> >>>>>>          .max_evtchn_port = -1,
> >>>>>>          .max_grant_frames = gnttab_dom0_frames(),
> >>>>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
> >>>>>> +        .max_maptrack_frames = -1,
> >>>>>>      };
> >>>>>>      int rc;
> >>>>>>
> >>>>>> --- a/xen/arch/x86/setup.c
> >>>>>> +++ b/xen/arch/x86/setup.c
> >>>>>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
> >>>>> mbi_p)
> >>>>>>      struct xen_domctl_createdomain dom0_cfg = {
> >>>>>>          .flags = IS_ENABLED(CONFIG_TBOOT) ?
> >> XEN_DOMCTL_CDF_s3_integrity
> >>>>> : 0,
> >>>>>>          .max_evtchn_port = -1,
> >>>>>> -        .max_grant_frames = opt_max_grant_frames,
> >>>>>> -        .max_maptrack_frames = opt_max_maptrack_frames,
> >>>>>> +        .max_grant_frames = -1,
> >>>>>> +        .max_maptrack_frames = -1,
> >>>>>>      };
> >>>>>
> >>>>> With these there's no need anymore for opt_max_maptrack_frames to
> >>>>> be non-static. Sadly Arm still wants opt_max_grant_frames
> >>>>> accessible in gnttab_dom0_frames().
> >>>>
> >>>> Yes, I was about to make them static until I saw what the ARM code
> did.
> >>>
> >>> But the one that Arm doesn't need should become static now.
> >>>
> >>>>>> --- a/xen/common/grant_table.c
> >>>>>> +++ b/xen/common/grant_table.c
> >>>>>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
> >>>>>>      return -ENOMEM;
> >>>>>>  }
> >>>>>>
> >>>>>> -int grant_table_init(struct domain *d, unsigned int
> >> max_grant_frames,
> >>>>>> -                     unsigned int max_maptrack_frames)
> >>>>>> +int grant_table_init(struct domain *d, int max_grant_frames,
> >>>>>> +                     int max_maptrack_frames)
> >>>>>>  {
> >>>>>>      struct grant_table *gt;
> >>>>>>      int ret = -ENOMEM;
> >>>>>>
> >>>>>> +    /* Default to maximum value if no value was specified */
> >>>>>> +    if ( max_grant_frames < 0 )
> >>>>>> +        max_grant_frames = opt_max_grant_frames;
> >>>>>> +    if ( max_maptrack_frames < 0 )
> >>>>>> +        max_maptrack_frames = opt_max_maptrack_frames;
> >>>>>> +
> >>>>>>      if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> >>>>>
> >>>>> I take it we don't expect people to specify 2^^31 or more
> >>>>> frames for either option. It looks like almost everything
> >>>>> here would cope, except for this very comparison. Nevertheless
> >>>>> I wonder whether you wouldn't better confine both values to
> >>>>> [0, INT_MAX] now, including when adjusted at runtime.
> >>>>
> >>>> I can certainly remove the 'U' from the definition of
> >>>> INITIAL_NR_GRANT_FRAMES,
> >>>
> >>> Oh, I didn't pay attention that is has a U on it - in this case
> >>> the comparison above is fine.
> >>>
> >>>> but do you want me to make opt_max_grant_frames and
> >>>> opt_max_maptrack_frames into signed ints and add signed parser
> >>>> code too?
> >>>
> >>> Definitely not. They should remain unsigned quantities, but their
> >>> values may need sanity checking now.
> >>>
> >>>> I also don't understand the 'adjusted at runtime' part.
> >>>
> >>> Well, for a command line drive value you could adjust an out of
> >>> bounds value in some __init function. But for runtime modifiable
> >>> settings you won't get away this easily.
> >>
> >> TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
> >> long)(-1) or something, and explicitly compare to that.  That leaves
> >> open the possibility of having more sentinel values if we decided we
> >> wanted them.
> >
> > I'm extremely confused now. What do you want me to compare and where?
> >
> > I assume we're talking about the opt_XXX values. Am I supposed to stop
> >INT_MAX being assigned to them? Or should I define local unsigned values
> for max_maptrack/grant_frames and simply initialize them to the passed-in
> arg (if >= 0) or the opt_XXX value otherwise.
> 
> In this version of the patch, you change the domctl arguments from
> uint32_t to int32_t.  I would leave them uint32_t, and if (
> max_grant_frames == XENSOMETHING_MAX_DEFAULT ) max_grant_frames = opt_&c.
> 
> Then the only invalid value we have to worry about is checking for
> XENSOMETHING_MAX_DEFAULT.
> 
> This is a suggestion, and I wouldn't argue strongly if someone thought
> it was a bad idea, but it seems like the most straightforward option to
> me.

AFAICT the definition of that invalid value is going to be needed by both the grant table code and the user-space toolstack code so I guess the logical place for the definition would be a tools-only section of the public grant table header? TBH I prefer the idea of any negative value being default though.
As long as the xl/libxl parts don't allow a *specified* value > INT_MAX then that should be fine, although for the full story a custom parser for the command line values should also be added to ensure the same semantics there.

  Paul

> 
>  -George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling

[Jan Beulich]

On 28.11.2019 11:26,  Durrant, Paul  wrote:
>> From: George Dunlap <george.dunlap@citrix.com>
>> Sent: 27 November 2019 16:52
>>
>> On 11/27/19 4:43 PM, Durrant, Paul wrote:
>>>> From: George Dunlap <george.dunlap@citrix.com>
>>>> Sent: 27 November 2019 16:34
>>>>
>>>> TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
>>>> long)(-1) or something, and explicitly compare to that.  That leaves
>>>> open the possibility of having more sentinel values if we decided we
>>>> wanted them.
>>>
>>> I'm extremely confused now. What do you want me to compare and where?
>>>
>>> I assume we're talking about the opt_XXX values. Am I supposed to stop
>>> INT_MAX being assigned to them? Or should I define local unsigned values
>> for max_maptrack/grant_frames and simply initialize them to the passed-in
>> arg (if >= 0) or the opt_XXX value otherwise.
>>
>> In this version of the patch, you change the domctl arguments from
>> uint32_t to int32_t.  I would leave them uint32_t, and if (
>> max_grant_frames == XENSOMETHING_MAX_DEFAULT ) max_grant_frames = opt_&c.
>>
>> Then the only invalid value we have to worry about is checking for
>> XENSOMETHING_MAX_DEFAULT.
>>
>> This is a suggestion, and I wouldn't argue strongly if someone thought
>> it was a bad idea, but it seems like the most straightforward option to
>> me.
> 
> AFAICT the definition of that invalid value is going to be needed by both
> the grant table code and the user-space toolstack code so I guess the
> logical place for the definition would be a tools-only section of the
> public grant table header? TBH I prefer the idea of any negative value
> being default though.

FWIW I agree, as I can't really see what other purposes we might need
sentinel values for down the road.

> As long as the xl/libxl parts don't allow a *specified* value > INT_MAX
> then that should be fine, although for the full story a custom parser
> for the command line values should also be added to ensure the same
> semantics there.

Right. While going a little far, I can't right now see easy alternatives
to a custom parser.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel