Re: [PATCH 1/3] [RFC] x86/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once

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

On 08/05/15 14:46, Jan Beulich wrote:
>>>> On 08.05.15 at 14:54, <andrew.cooper3@citrix.com> wrote:
>> There is no conceptual problem with setting this parameter more than once.
>> Checkpointed migration streams will typically set it once per checkpoint to
>> the same value.
>>
>> The parameter is only actually needed on early-generation VT-x which lacked
>> the unrestricted guest capability, although it could plausibly be used on
>> newer VT-x with unusual execution control settings.  Short circuit the
>> expensive operations on non VT-x hardware.
>>
>> The parameter itself must always be latched to avoid issues if the VM
>> eventually migrates to a host which needs to use the pagetable.
> Reads all plausible, except that I think the way you carry out this
> last aspect needs adjustment:
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5730,12 +5730,13 @@ static int hvmop_set_param(
>>              rc = -EINVAL;
>>          break;
>>      case HVM_PARAM_IDENT_PT:
>> -        rc = -EINVAL;
>> -        if ( d->arch.hvm_domain.params[a.index] != 0 )
>> -            break;
>> -
>>          rc = 0;
>> -        if ( !paging_mode_hap(d) )
>> +        d->arch.hvm_domain.params[a.index] = a.value;
>> +        /*
>> +         * Only actually required for VT-x lacking unrestricted_guest
>> +         * capabilities.  Short circuit the pause if possible.
>> +         */
>> +        if ( !paging_mode_hap(d) || !cpu_has_vmx )
>>              break;
> You should latch the new value inside this if()'s body and ...
>
>> @@ -5749,7 +5750,6 @@ static int hvmop_set_param(
>>  
>>          rc = 0;
>>          domain_pause(d);
>> -        d->arch.hvm_domain.params[a.index] = a.value;
>>          for_each_vcpu ( d, v )
>>              paging_update_cr3(v);
>>          domain_unpause(d);
> ... leave this one alone so the use site in VMX code won't see the
> new value prematurely.

Good point - I will fix this up.

~Andrew