Re: [PATCH] x86/vmx: add support for virtualize SPEC_CTRL

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2024 11:45, Roger Pau Monné wrote:
> On Thu, Feb 08, 2024 at 02:40:53PM +0100, Jan Beulich wrote:
>> On 06.02.2024 15:25, Roger Pau Monne wrote:
>>> @@ -2086,6 +2091,9 @@ void vmcs_dump_vcpu(struct vcpu *v)
>>>      if ( v->arch.hvm.vmx.secondary_exec_control &
>>>           SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY )
>>>          printk("InterruptStatus = %04x\n", vmr16(GUEST_INTR_STATUS));
>>> +    if ( cpu_has_vmx_virt_spec_ctrl )
>>> +        printk("SPEC_CTRL mask = %#016lx  shadow = %#016lx\n",
>>> +               vmr(SPEC_CTRL_MASK), vmr(SPEC_CTRL_SHADOW));
>>
>> #0... doesn't make a lot of sense; only e.g. %#lx does. Seeing context
>> there's no 0x prefix there anyway. Having looked at the function the
>> other day, I know though that there's a fair mix of 0x-prefixed and
>> unprefixed hex numbers that are output.
> 
> For consistency with how other MSRs are printed I should use the '0x'
> prefix.

MSRs? It's VMCS fields which are printed here.

>> Personally I'd prefer if all
>> 0x prefixes were omitted here. If you and Andrew think otherwise, I can
>> live with that, so long as we're at least striving towards consistent
>> output (I may be able to get to doing a conversion patch, once I know
>> which way the conversion should be).
> 
> I usually prefer the '0x' to avoid ambiguity.  However this being all
> hardware registers, I might be fine with dropping the '0x' on the
> grounds that all registers are always printed as hex.
> 
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -823,18 +823,28 @@ static void cf_check vmx_cpuid_policy_changed(struct 
>>> vcpu *v)
>>>      {
>>>          vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>  
>>> -        rc = vmx_add_guest_msr(v, MSR_SPEC_CTRL, 0);
>>> -        if ( rc )
>>> -            goto out;
>>> +        if ( !cpu_has_vmx_virt_spec_ctrl )
>>> +        {
>>> +            rc = vmx_add_guest_msr(v, MSR_SPEC_CTRL, 0);
>>> +            if ( rc )
>>> +                goto out;
>>> +        }
>>
>> I'm certainly okay with you doing it this way, but generally I'd prefer
>> if code churn was limited whjere possible. Here leveraging that rc is 0
>> on entry, a smaller change would be to
>>
>>         if ( !cpu_has_vmx_virt_spec_ctrl )
>>             rc = vmx_add_guest_msr(v, MSR_SPEC_CTRL, 0);
>>         if ( rc )
>>             goto out;
>>
>> (similarly below then).
> 
> That looks odd to me, and is not how I would write that code.  I can
> however adjust if you insist.  Given it's just a two line difference I
> think it was worth having the more usual form.

As said, I'm okay with the change as is.

Jan