Re: [Xen-devel] [PATCH 2/3] VMX: allocate VMCS pages from domain heap

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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".

Jan


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