Re: Violations of MISRA C Rule 20.7 in xen/arch/x86/include/asm/hvm/trace.h

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/03/2024 11:43 am, Nicola Vetrini wrote:
> Reporting the discussion that took place on Matrix on the matter, so
> that it can carry on here with all interested parties:
>
>> Hi everyone. I was looking at MISRA violations for Rule 20.7
>> ("Expressions resulting from the expansion of macro parameters shall
>> be enclosed in parentheses") generated by
>>
>>     #define TRC_PAR_LONG(par) ((uint32_t)(par)), ((par) >> 32)
>>
>> The thing here is this: the simplest fix is
>>
>>     -#define TRC_PAR_LONG(par) ((uint32_t)(par)), ((par) >> 32)
>>     +#define TRC_PAR_LO(par) ((uint32_t)(par))
>>     +#define TRC_PAR_HI(par) ((par) >> 32)
>>
>> and then replace all call sites accordingly. However, in certain
>> places (e.g., svm.c:1445), this causes a build error:
>>
>>     arch/x86/hvm/svm/svm.c: In function ‘svm_inject_event’:
>>     arch/x86/hvm/svm/svm.c:1445:1: error: macro "HVMTRACE_3D"
>> requires 4 arguments, but only 3 given
>>     1445 | TRC_PAR_LO(_event.cr2), TRC_PAR_HI(_event.cr2));
>>     | ^
>>     In file included from arch/x86/hvm/svm/svm.c:31:
>>
>> this is due to the somewhat strange definition of HVMTRACE_3D which
>> works with the previous form of the macro, but not the current one,
>> as the macro argument in HVMTRACE_LONG_2D is now split (_LO is d2 and
>> _HI is variadic), and therefore it's not passed to HVMTRACE_3D
>> anymore as two arguments.
>> I have a proposal: introduce a d3 argument to HVMTRACE_LONG_2D to
>> supply the additional argument and/or make HVMTRACE_LONG_2D
>> non-variadic. This would of course apply to the similarly named
>> macros in xen/arch/x86/include/asm/hvm/trace.h
>
> Andrew Cooper:
>> sigh - I'm half way through deleting all of that
>> guess I ought to finish
>
> Andrew Cooper:
>> @Nicola Vetrini:
>> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/xen-trace
>> On second thoughts I'm not sure it fixes the problem
>> just changes it.  The problem is the use of macros to split a 64bit
>> quantity across two 32bit fields
>
> Nicola Vetrini:
>> It seems so, yes. But afaict this can be fixed by splitting the macro
>> definition in two, as done above, which wouldn't incur in the
>> compilation error on the new API
>
> Andrew Cooper:
>> given the users of TRACE_PARAM64() by the end, I'd prefer to do that
>> with real structs rather than perpetuating the macro mess
>
> George Dunlap:
>> It's been a long time since I looked at all this, but FWIW I
>> inherited all the weird macro stuff, never liked it, and myself used
>> structs for all new traces. So without having looked at the code at
>> all, I'm predisposed to agree w/ Andy's assessment that we should
>> just rip out the offending macros and replace them with structs.
>
> @gwd: I believe the file that I was concerned about does not fall
> under the XENTRACE entry in MAINTAINERS; you may want to rectify that.
>

My vague plan to fix this is to still take
https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=8636f0eaa0f163f8a433eb17b9e0d2e87db284b6
but not introduce TRACE_PARAM64

The users are as follows:

xen.git/xen$ git grep TRACE_PARAM64
arch/x86/hvm/emulate.c:2183:        TRACE(TRC_HVM_CR_READ64, reg,
TRACE_PARAM64(*val));
arch/x86/hvm/emulate.c:2199:    TRACE(TRC_HVM_CR_WRITE64, reg,
TRACE_PARAM64(val));
arch/x86/hvm/emulate.c:2244:        TRACE(TRC_HVM_XCR_READ64, reg,
TRACE_PARAM64(*val));
arch/x86/hvm/emulate.c:2254:    TRACE(TRC_HVM_XCR_WRITE64, reg,
TRACE_PARAM64(val));
arch/x86/hvm/hpet.c:308:               TRACE_PARAM64(diff_ns),
TRACE_PARAM64(period_ns));
arch/x86/hvm/hvm.c:2155:    TRACE(TRC_HVM_CR_WRITE64, cr,
TRACE_PARAM64(val));
arch/x86/hvm/hvm.c:2220:    TRACE(TRC_HVM_CR_READ64, cr,
TRACE_PARAM64(val));
arch/x86/hvm/hvm.c:3631:    TRACE(TRC_HVM_MSR_READ, msr,
TRACE_PARAM64(*msr_content));
arch/x86/hvm/hvm.c:3647:    TRACE(TRC_HVM_MSR_WRITE, msr,
TRACE_PARAM64(msr_content));
arch/x86/hvm/svm/svm.c:1442:              TRACE_PARAM64(_event.cr2));
arch/x86/hvm/svm/svm.c:2402:    TRACE(TRC_HVM_INVLPG64, 0,
TRACE_PARAM64(linear));
arch/x86/hvm/svm/svm.c:2630:                   exit_reason,
TRACE_PARAM64(regs->rip));
arch/x86/hvm/svm/svm.c:2826:                TRACE(TRC_HVM_PF_XEN64,
regs->error_code, TRACE_PARAM64(va));
arch/x86/hvm/vlapic.c:738:       
TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(delta),
arch/x86/hvm/vlapic.c:739:                   TRACE_PARAM64(is_periodic ?
period : 0), vlapic->pt.irq);
arch/x86/hvm/vlapic.c:1209:       
TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(delta),
arch/x86/hvm/vlapic.c:1210:                   TRACE_PARAM64(0LL),
vlapic->pt.irq);
arch/x86/hvm/vlapic.c:1223:           
TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(0LL),
arch/x86/hvm/vlapic.c:1224:                       TRACE_PARAM64(0LL),
vlapic->pt.irq);
arch/x86/hvm/vlapic.c:1477:   
TRACE_TIME(TRC_HVM_EMUL_LAPIC_START_TIMER, TRACE_PARAM64(period),
arch/x86/hvm/vlapic.c:1478:               TRACE_PARAM64(is_periodic ?
period : 0LL), s->pt.irq);
arch/x86/hvm/vmx/vmx.c:2093:             
TRACE_PARAM64(curr->arch.hvm.guest_cr[2]));
arch/x86/hvm/vmx/vmx.c:3099:    TRACE(TRC_HVM_INVLPG64, /*invlpga=*/ 0,
TRACE_PARAM64(linear));
arch/x86/hvm/vmx/vmx.c:3160:        TRACE(TRC_HVM_LMSW64,
TRACE_PARAM64(value));
arch/x86/hvm/vmx/vmx.c:4064:        TRACE_TIME(TRC_HVM_VMEXIT64,
exit_reason, TRACE_PARAM64(regs->rip));
arch/x86/hvm/vmx/vmx.c:4346:                         
TRACE_PARAM64(exit_qualification));
include/xen/trace.h:87:#define TRACE_PARAM64(p)    (uint32_t)(p), ((p)
>> 32)

and there are probably enough examples of "reg32, val64" to justify
making a helper.  We're touching every site anyway, so adjudging for the
next version of the series is easy.

I might repost the first few patches right now.  All the fixes in the
scheduler have been reviewed and ready to go for 3 release of Xen already...

~Andrew