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

Re: [Xen-devel] [PATCH v2 3/3] tools: introduce parameter max_wp_ram_ranges.



Thank you, Jan.

On 1/22/2016 4:01 PM, Jan Beulich wrote:
On 22.01.16 at 04:20, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -940,6 +940,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct
hvm_ioreq_server *s,
  {
      unsigned int i;
      int rc;
+    unsigned int max_wp_ram_ranges =
+        ( s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] > 0 ) 
?
+        s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] :
+        MAX_NR_IO_RANGES;

Besides this having stray blanks inside the parentheses it truncates
the value from 64 to 32 bits and would benefit from using the gcc
extension of omitting the middle operand of ?:. But even better
would imo be if you avoided the local variable and ...

After second thought, how about we define a default value for this
parameter in libx.h, and initialize the parameter when creating the
domain with default value if it's not configured.
About this local variable, we keep it, and ...

@@ -962,7 +966,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct 
hvm_ioreq_server *s,
          if ( !s->range[i] )
              goto fail;

-        rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
+        if ( i == HVMOP_IO_RANGE_WP_MEM )
+            rangeset_limit(s->range[i], max_wp_ram_ranges);
+        else
+            rangeset_limit(s->range[i], MAX_NR_IO_RANGES);

... did the entire computation here, using ?: for the second argument
of the function invocation.

... replace the if/else pair with sth. like:
        rangeset_limit(s->range[i],
                       ((i == HVMOP_IO_RANGE_WP_MEM)?
                        max_wp_ram_ranges:
                        MAX_NR_IO_RANGES));
This 'max_wp_ram_ranges' has no particular usages, but the string
"s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] "
is too lengthy, and can easily break the 80 column limitation. :)
Does this approach sounds OK? :)

@@ -6009,6 +6016,7 @@ static int hvm_allow_set_param(struct domain *d,
      case HVM_PARAM_IOREQ_SERVER_PFN:
      case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
      case HVM_PARAM_ALTP2M:
+    case HVM_PARAM_MAX_WP_RAM_RANGES:
          if ( value != 0 && a->value != value )
              rc = -EEXIST;
          break;

Is there a particular reason you want this limit to be unchangeable
after having got set once?

Well, not exactly. :)
I added this limit because by now we do not have any approach to
change the max range numbers inside ioreq server during run-time.
I can add another patch to introduce an xl command, which can change
it dynamically. But I doubt the necessity of this new command and
am also wonder if this new command would cause more confusion for
the user...
Jan


B.R.
Yu
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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