[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/3] VMX: allocate VMCS pages from domain heap
On 24/11/15 11:04, Jan Beulich wrote: >>>> On 24.11.15 at 11:59, <andrew.cooper3@xxxxxxxxxx> wrote: >> On 24/11/15 10:55, Andrew Cooper wrote: >>> On 24/11/15 07:50, Jan Beulich wrote: >>>>>>> On 24.11.15 at 06:04, <kevin.tian@xxxxxxxxx> wrote: >>>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] >>>>>> Sent: Monday, November 23, 2015 10:28 PM >>>>>> >>>>>>>>> On 21.10.15 at 05:16, <kevin.tian@xxxxxxxxx> wrote: >>>>>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] >>>>>>>> Sent: Tuesday, October 20, 2015 6:36 PM >>>>>>>>>>> On 20.10.15 at 12:12, <andrew.cooper3@xxxxxxxxxx> 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.tian@xxxxxxxxx> >>>> 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. >> Actually it turns out that we already have an appropriate sentinel. >> >> include/asm-x86/types.h:34:#define INVALID_PADDR (~0UL) > Yes, that's what I had tried to use in above mentioned attempt. Right. Lets not block the change on another infrastructure improvement. Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |