Re: [PATCH v4 3/8] VMX: tertiary execution control infrastructure

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 01, 2024 at 01:09:11PM +0100, Jan Beulich wrote:
> On 01.02.2024 12:50, Roger Pau Monné wrote:
> > On Thu, Jan 11, 2024 at 10:00:10AM +0100, Jan Beulich wrote:
> >> @@ -503,6 +538,9 @@ static int vmx_init_vmcs_config(bool bsp
> >>              "Secondary Exec Control",
> >>              vmx_secondary_exec_control, _vmx_secondary_exec_control);
> >>          mismatch |= cap_check(
> >> +            "Tertiary Exec Control",
> >> +            vmx_tertiary_exec_control, _vmx_tertiary_exec_control);
> > 
> > I know it's done to match the surrounding style, but couldn't you move
> > the name parameter one line up, and then limit the call to two lines?
> > 
> > (I don't think it will compromise readability).
> 
> You mean like this:
> 
>         mismatch |= cap_check("Tertiary Exec Control",
>             vmx_tertiary_exec_control, _vmx_tertiary_exec_control);
> 
> ? No, I view this as a mix of two possible styles. If the string literal
> was moved up, the other legitimate style would only be
> 
>         mismatch |= cap_check("Tertiary Exec Control",
>                               vmx_tertiary_exec_control,
>                               _vmx_tertiary_exec_control);
> 
> aiui (again extending over 3 lines). Yet none of this is written down
> anywhere.
> 
> But anyway - consistency with surrounding code trumps here, I think.

I was hoping it could still fit on 2 lines, but if you need 3 never
mind then.

> >> @@ -2068,10 +2111,12 @@ void vmcs_dump_vcpu(struct vcpu *v)
> >>                 vmr(HOST_PERF_GLOBAL_CTRL));
> >>  
> >>      printk("*** Control State ***\n");
> >> -    printk("PinBased=%08x CPUBased=%08x SecondaryExec=%08x\n",
> >> +    printk("PinBased=%08x CPUBased=%08x\n",
> >>             vmr32(PIN_BASED_VM_EXEC_CONTROL),
> >> -           vmr32(CPU_BASED_VM_EXEC_CONTROL),
> >> -           vmr32(SECONDARY_VM_EXEC_CONTROL));
> >> +           vmr32(CPU_BASED_VM_EXEC_CONTROL));
> >> +    printk("SecondaryExec=%08x TertiaryExec=%08lx\n",
> > 
> > For consistency, shouldn't TertiaryExec use 016 instead of 08 (as it's
> > a 64bit filed).
> 
> Perhaps, assuming we'll gets bits 32 and populated sooner or later.
> However, I view 16-digit literal numbers as hard to read, so I'd be
> inclined to insert a separator (e.g. an underscore) between the low
> and high halves. Thoughts?

Works for me.

Thanks, Roger.