[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <JBeulich@xxxxxxxx>
  • Date: Mon, 5 Aug 2019 13:32:44 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1;spf=pass smtp.mailfrom=suse.com;dmarc=pass action=none header.from=suse.com;dkim=pass header.d=suse.com;arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=UQuXPml0OzgvqGA+qnqlrqmcNzuZcF5K7ADqGvRfCmA=; b=C0pLFdClWPqpjcC+UkshA1eVRU1kg/ciUjYBLMDmkeO36KindMqT/VKb8qlLbN0sEhvEx/QqWOn4Ossn9icvt7YVmUS4itNr6MFEPuQ4i1sXlukKnP1sOERrzf+mTomnqo5oIu0Z7pwKqrlGGomSloRCcxC2jslqrGBfkbk73tuGacrXXqC7i57m6UdszVCnwqDeqf2Lupu9P48nMzc8N3TfUvKIWfuWM1T6vxRsQafUz2J/DxxXf6orNs/KDp3BF83XP6W8v2HsvDQ9ceuepD8RpUkt+aROvgB4FQ3Hvdgf/OtcRFKLDyENIJbwYy9FLqSU61CF6utxiXCA8b9JyQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OYO+2VUJYuikS3J58PNgMlR8si35Hr2BybTV1crGritwWR208ea8hfPB8pyV0u77JQDpZCH6bjoeeqkU8RBsSk4WTz+2W6LUL4ghMWVaXIJ7f0nw1WkEHn9U08jJ8hGjhmMu3IQAn6eUpSusf0zhrkeWslKAvf1h4qfI/F6YNVdd2rfWYjAtH4l8D+EWkDSJH+KTUTtb0r66Jzah54Ogo19eEhAdlIMTltLXnjhrlO2CJ6m6tdZfO9GmMkuCQfeCvTdj5zr485DHfCbtOCsoWrzggiS/t7EDgye4/g6ua6ZO+PgbZI8aDg9Qxg5Lsyx5SxIGevbdzfkRXWZ6N0dKKw==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=JBeulich@xxxxxxxx;
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Mon, 05 Aug 2019 13:34:26 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVRuU3MpytGLjiWki4k1cIRnKVnabsi3qAgAAGLQ6AAAUMgA==
  • Thread-topic: [Xen-devel] [PATCH] x86/vvmx: Fix nested virt on VMCS-Shadow capable hardware

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(),

>> 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>

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.