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
