Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask

["Jan Beulich" <JBeulich@suse.com>]

>>> On 04.08.14 at 15:31, <andrew.cooper3@citrix.com> wrote:
> On 04/08/14 14:12, Paul Durrant wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>>                      rc = -EINVAL;
>>                  break;
>>              case HVM_PARAM_VIRIDIAN:
>> -                if ( a.value > 1 )
>> -                    rc = -EINVAL;
>> +                /* This should only ever be set once by the tools and read by the guest. */
>> +                rc = -EPERM;
>> +                if ( curr_d == d )
>> +                    break;
>> +
>> +                rc = -EPERM;
>> +                if ( d->arch.hvm_domain.params[a.index] &&
>> +                     a.value != d->arch.hvm_domain.params[a.index] )
>> +                    break;
> 
> Setting it twice should be an error, even if it is set to the same value
> again.

I specifically asked for it to be done this way, such that redundant
calls wouldn't needlessly fail. Remember that we're altering an
existing interface, and hence should be careful about breaking
existing callers.

>  Also, EEXIST would be a better error.

I agree with this part.

>> +/* Disable timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
>> + * HV_X64_MSR_APIC_FREQUENCY).
>> + * This modification restores the viridian feature set to the
>> + * original 'base' set exposed in releases prior to Xen 4.4.
>> + */
>> +#define _HVMPV_no_freq 1
>> +#define HVMPV_no_freq  (1 << _HVMPV_no_freq)
>> +
>> +#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)
> 
> Please could we avoid introducing new negated-sense booleans, especially
> when their predominant use is "if ( !$FOO_no_BAR )"

What would your alternative proposal be, again keeping in mind that
existing callers must not observe a behavioral change?

Jan