Re: [PATCH v2 04/12] x86/hvm: Reduce stack usage from HVMTRACE_ND()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/09/2021 12:00, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> v2:
>>  * Adjust callers of HVMTRACE_ND() too
> What does this refer to? The sole difference to v1 that I can spot
> is ...

Oh - its me who was confused.

I thought I had failed to include the changes in vmx.c/svm.c in v1.  In
which case, no change to that in v2.

>>  * Drop _d[] for the 0 case.
> ... the one corresponding to this line, i.e. ...
>
>> --- a/xen/include/asm-x86/hvm/trace.h
>> +++ b/xen/include/asm-x86/hvm/trace.h
>> @@ -67,38 +67,30 @@
>>  #define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
>>      TRACE_6D(_e, d1, d2, d3, d4)
>>  
>> -#define HVMTRACE_ND(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6) \
>> +#define HVMTRACE_ND(evt, modifier, cycles, ...)                           \
>>      do {                                                                  \
>>          if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt )               \
>>          {                                                                 \
>> -            struct {                                                      \
>> -                u32 d[6];                                                 \
>> -            } _d;                                                         \
>> -            _d.d[0]=(d1);                                                 \
>> -            _d.d[1]=(d2);                                                 \
>> -            _d.d[2]=(d3);                                                 \
>> -            _d.d[3]=(d4);                                                 \
>> -            _d.d[4]=(d5);                                                 \
>> -            _d.d[5]=(d6);                                                 \
>> +            uint32_t _d[] = { __VA_ARGS__ };                              \
>>              __trace_var(TRC_HVM_ ## evt | (modifier), cycles,             \
>> -                        sizeof(*_d.d) * count, &_d);                      \
>> +                        sizeof(_d), sizeof(_d) ? _d : NULL);              \
> ... the addition of a conditional operator here (which I assume was
> something a particular compiler didn't like in v1).

And was covered in the commit message:

> The 0 case needs a little help.  All object in C must have a unique address
> and _d is passed by pointer.  Explicitly permit the optimiser to drop the
> array.



> FAOD - I'm fine
> with the change, but I fear I'm overlooking something (again).

Thanks,

~Andrew