Re: [PATCH v2 6/9] x86/vmx: move declations used only by vmx code from vmx.h to private header

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/02/2023 10:47 am, Jan Beulich wrote:
> On 22.02.2023 13:00, Xenia Ragiadakou wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmx/vmx.h
>> @@ -0,0 +1,459 @@
>> +#include <xen/sched.h>

Like svm.h, SPDX tag and guards please.

>
>> +extern int8_t opt_ept_exec_sp;
>> +
>> +#define PI_xAPIC_NDST_MASK      0xFF00
>> +
>> +void vmx_asm_vmexit_handler(struct cpu_user_regs);
> Even if it was like this originally, this is bogus. This not-really-a-
> function doesn't take any parameters. It's declaration also looks to be
> missing cf_check - Andrew, was this deliberate?

Yes, it's deliberate.  As you identified, it's never called, and
therefore doesn't undergo any CFI typechecking.

It's an address written into the VMCS's HOST_RIP field, which also
explains why it has a bogus type and no-one's noticed in 18 years.  (Yup
really - c/s 9c3118a825, December 2004)


But it does bring us back to a general question which has come up a
couple of times recently - how to represent asm symbols in C.

This ought to be

void nocall vmx_asm_vmexit_handler(void);

to identify that it is a function-like thing without a normal calling
convention.  IMO i could live in vmcs.c rather than being declared publicly.

I see nothing in MISRA that objects to this.  Rule 8.5 states that an
external object or function shall be declared once, and only in one
file.  There is no mandate that this single file is a header file.

The point of moving this to a .c file is to indicate that the asm symbol
isn't legitimate to reference anywhere else.


Such a change probably wants backporting.  Let me do a specific fix, to
separate it from the cleanup.

~Andrew