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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@intel.com> wrote:
>>>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: Monday, November 23, 2015 10:28 PM
>>>>
>>>>>>> On 21.10.15 at 05:16, <kevin.tian@intel.com> wrote:
>>>>>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>> Sent: Tuesday, October 20, 2015 6:36 PM
>>>>>>>>> On 20.10.15 at 12:12, <andrew.cooper3@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.tian@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.

Actually it turns out that we already have an appropriate sentinel.

include/asm-x86/types.h:34:#define INVALID_PADDR (~0UL)

~Andrew