On 24/11/15 07:50, Jan Beulich wrote: >>>> On 24.11.15 at 06:04, <kevin.t...@intel.com> wrote: >>> From: Jan Beulich [mailto:jbeul...@suse.com] >>> Sent: Monday, November 23, 2015 10:28 PM >>> >>>>>> On 21.10.15 at 05:16, <kevin.t...@intel.com> wrote: >>>>> From: Jan Beulich [mailto:jbeul...@suse.com] >>>>> Sent: Tuesday, October 20, 2015 6:36 PM >>>>>>>> On 20.10.15 at 12:12, <andrew.coop...@citrix.com> wrote: >>>>>> On 19/10/15 16:22, Jan Beulich wrote: >>>>>>> @@ -580,7 +583,7 @@ int vmx_cpu_up_prepare(unsigned int cpu) >>>>>>> void vmx_cpu_dead(unsigned int cpu) >>>>>>> { >>>>>>> vmx_free_vmcs(per_cpu(vmxon_region, cpu)); >>>>>>> - per_cpu(vmxon_region, cpu) = NULL; >>>>>>> + per_cpu(vmxon_region, cpu) = 0; >>>>>> While this is currently safe (as pa 0 is not part of the available heap >>>>>> allocation range), might it be worth introducing a named sentential? I >>>>>> can forsee a DMLite nested Xen scenario where we definitely don't need >>>>>> to treat the low 1MB magically. >>>>> I guess there are more things to adjust if we ever cared to recover >>>>> the few hundred kb below 1Mb. And then I don't see why nested >>>>> Xen would matter here: One major main reason for reserving that >>>>> space is that we want to put the trampoline there. Do you think >>>>> DMlite would allow us to get away without? But even if so, this >>>>> would again fall under what I've said in the first sentence. >>>> Could you at least introduce a macro first? Regardless of how much >>>> things to adjust, this way makes future change simple. >>> So I've made an attempt, but this is really getting unwieldy: Setting >>> per-CPU data to non-zero initial values is not possible; making sure >>> cleanup code avoids assuming such variables got initialized is quite >>> error prone. Same goes at least to a certain extent for struct vcpu >>> members (see e.g. nvmx_vcpu_destroy(), which currently is >>> correct no matter whether nvmx_vcpu_initialise() ran at all, or to >>> completion). >>> >>> I also don't see what a macro would help here, or how/where it >>> should be used. paddr_valid()? Yes, I could do this, but it wouldn't >>> simplify much when later wanting to convert to a non-zero value >>> for above reasons (it would instead give the wrong impression that >>> changing the value is all it takes). >>> >> Thanks for looking into this attempt. Based on your explanation >> I think your original code is reasonable to go. Here is my ack: >> >> Acked-by: Kevin Tian <kevin.t...@intel.com> > Thanks Kevin. Andrew - please indicate whether your previous > comment is to be considered a NAK, or "just a comment".
I would prefer a sentinel value being introduced, but can live without it being changed. It is definitely not the only area which uses 0 as a sentinel and cleanup will have to happen, one way or another. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
