Re: [Xen-devel] [PATCH] x86/altp2m: add altp2m_vcpu_disable_notify

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 12/18/18 4:10 PM, Jan Beulich wrote:
>>>> On 14.12.18 at 18:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> Allow altp2m users to disable #VE/VMFUNC alone. Currently it is
>> only possible to disable this functionality when we disable altp2m
>> completely; #VE/VMFUNC can only be enabled once per altp2m session.
>>
>> In addition to making things complete, disabling #VE is also a
>> workaround for CFW116 ("When Virtualization Exceptions are Enabled,
>> EPT Violations May Generate Erroneous Virtualization Exceptions")
>> on Xeon CPUs.
> 
> "Xeon CPUs" is overly generic. Yes, the CFW erratum prefix allows
> to identify which one you mean, but only (afaik) by going through
> the spec updates until you've found the right one. Can you please
> be more specific here?

Of course, sorry for the ambiguity. I was referring to the E-2100s:

https://www.intel.com/content/www/us/en/products/docs/processors/xeon/xeon-e-2100-specification-update.html

I will update the patch description.

>> @@ -4602,6 +4603,36 @@ static int do_altp2m_op(
>>          break;
>>      }
>>  
>> +    case HVMOP_altp2m_vcpu_disable_notify:
>> +    {
>> +        struct vcpu *v;
>> +
>> +        if ( a.u.disable_notify.pad ||
>> +             a.u.disable_notify.vcpu_id >= d->max_vcpus )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        if ( !cpu_has_vmx_virt_exceptions )
>> +        {
>> +            rc = -EOPNOTSUPP;
>> +            break;
>> +        }
>> +
>> +        v = d->vcpu[a.u.enable_notify.vcpu_id];
>> +
>> +        if ( gfn_eq(vcpu_altp2m(v).veinfo_gfn, INVALID_GFN) )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
> 
> Why? Disabling what is already disabled is not wrong, and hence
> could easily be treated as a no-op.

Just tought I'd actually report that this would be a no-op. But it's not
important, so I'll make it a no-op (simply break instead of setting rc
to -EINVAL).

>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -232,6 +232,13 @@ struct xen_hvm_altp2m_vcpu_enable_notify {
>>  typedef struct xen_hvm_altp2m_vcpu_enable_notify 
>> xen_hvm_altp2m_vcpu_enable_notify_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_vcpu_enable_notify_t);
>>  
>> +struct xen_hvm_altp2m_vcpu_disable_notify {
>> +    uint32_t vcpu_id;
>> +    uint32_t pad;
> 
> Why the pad field? There's no hole left.

Sorry, that was an oversight. I'll correct it.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel