From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yu, Zhang" Subject: Re: [PATCH v2 3/3] tools: introduce parameter max_wp_ram_ranges. Date: Wed, 27 Jan 2016 15:01:18 +0800 Message-ID: <56A86B3E.50100@linux.intel.com> References: <1453432840-5319-1-git-send-email-yu.c.zhang@linux.intel.com> <1453432840-5319-4-git-send-email-yu.c.zhang@linux.intel.com> <56A1EFEE02000078000C9E1E@prv-mh.provo.novell.com> <56A7211C.5040204@linux.intel.com> <56A75FE002000078000CAFBD@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56A75FE002000078000CAFBD@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, Paul.Durrant@citrix.com, zhiyuan.lv@intel.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 1/26/2016 7:00 PM, Jan Beulich wrote: >>>> On 26.01.16 at 08:32, wrote: >> On 1/22/2016 4:01 PM, Jan Beulich wrote: >>>>>> On 22.01.16 at 04:20, 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. > > No, I don't think the tool stack should be determining the default > here (unless you want the default to be zero, and have zero > indeed mean zero). > Thank you, Jan. If we do not provide a default value in tool stack, the code above should be kept, to initialize the local variable with either the one set in the configuration file, or with MAX_NR_IO_RANGES. Is this OK? >> 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? :) > > Seems better than the original, so okay. > >>>> @@ -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... > > And I didn't say you need to expose this to the user. All I asked > was whether you really mean the value to be a set-once one. If > yes, the code above is fine. If no, the code above should be > changed, but there's then still no need to expose a way to > "manually" adjust the value until a need for such arises. > I see. The constraint is not necessary. And I'll remove this code. :) > Jan > > B.R. Yu