Re: [Xen-devel] [PATCH] x86/vvmx: Fix nested virt on VMCS-Shadow capable hardware

[Jan Beulich <JBeulich@xxxxxxxx>]

On 05.08.2019 15:14, Andrew Cooper wrote:
> On 05/08/2019 13:52, Jan Beulich wrote:
>> On 30.07.2019 16:42, Andrew Cooper wrote:
>>> c/s e9986b0dd "x86/vvmx: Simplify per-CPU memory allocations" had the wrong
>>> indirection on its pointer check in nvmx_cpu_up_prepare(), causing the
>>> VMCS-shadowing buffer never be allocated.  Fix it.
>>>
>>> This in turn results in a massive quantity of logspam, as every virtual
>>> vmentry/exit hits both gdprintk()s in the *_bulk() functions.
>> The "in turn" here applies to the original bug (which gets fixed here)
>> aiui,
> 
> Correct.
> 
>>   i.e. there isn't any log spam with the fix in place anymore, is
>> there?
> 
> Incorrect, because...
> 
>>   If so, ...
>>
>>> Switch these to using printk_once().  The size of the buffer is chosen at
>>> compile time, so complaining about it repeatedly is of no benefit.
>> ... I'm not sure I'd agree with this move: Why would it be of interest
>> only the first time that we (would have) overrun the buffer?
> 
> ... we will either never overrun it, or overrun it on every virtual
> vmentry/exit.
> 
>> After all
>> it's not only the compile time choice of buffer size that matters here,
>> but also the runtime aspect of what value "n" has got passed into the
>> functions.
> 
> The few choices of "n" are fixed at compile time as well, which is why...

Oh - I should have looked at the callers. It's all ARRAY_SIZE(),
indeed.

>> If this is on the assumption that we'd want to know merely
>> of the fact, not how often it occurs, then I'd think this ought to
>> remain a debugging printk().
> 
> ... this still ends up as a completely unusable system, when the problem
> occurs.
> 
>>
>>> Finally, drop the runtime NULL pointer checks.  It is not terribly 
>>> appropriate
>>> to be repeatedly checking infrastructure which is set up from start-of-day,
>>> and in this case, actually hid the above bug.
>> I don't see how the repeated checking would have hidden any bug:
> 
> Without this check, Xen would have crashed with a NULL deference on the
> original change, and highlighted the fact that the change was totally
> broken.
> 
> I didn't spot the issue because it was tested with a release build,
> which is another reason why the replacement printk() is deliberately not
> a debug-time-only.

Taking this as a basis, there shouldn't be any debug-only printk()s.

Anyway, with your explanations
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel