Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <82acae8f-6b27-928f-0c00-1df8fc9d26b8@redhat.com>
Date:   Tue, 28 Sep 2021 18:20:17 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Subject: Re: [RFC PATCH 2/3] nSVM: introduce smv->nested.save to cache save
 area fields
Content-Language: en-US
To:     Maxim Levitsky <mlevitsk@redhat.com>,
        Emanuele Giuseppe Esposito <eesposit@redhat.com>,
        kvm@vger.kernel.org
Cc:     Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        linux-kernel@vger.kernel.org
References: <20210903102039.55422-1-eesposit@redhat.com>
 <20210903102039.55422-3-eesposit@redhat.com>
 <fbb40bb8c12715c0aa9d6a113784f8a21603e2b3.camel@redhat.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <fbb40bb8c12715c0aa9d6a113784f8a21603e2b3.camel@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/09/21 12:39, Maxim Levitsky wrote:
> On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote:
>> This is useful in next patch, to avoid having temporary
>> copies of vmcb12 registers and passing them manually.
> 
> This is NOT what I had in mind, but I do like that idea very much,
> IMHO this is much better than what I had in mind!
> 
> The only thing that I would change is that I woudn't reuse 'struct vmcb_save_area'
> for the copy, as this both wastes space (minor issue),
> and introduces a chance of someone later using non copied
> fields from it (can cause a bug later on).
> 
> I would just define a new struct for that (but keep same names
> for readability)
> 
> Maybe something like 'struct vmcb_save_area_cached'?

I agree, I like this too.  However, it needs a comment that this new 
struct is not kept up-to-date, and is only valid until enter_svm_guest_mode.

I might even propose a

#ifdef CONFIG_DEBUG_KERNEL
	memset(&svm->nested.save, 0xaf, sizeof(svm->nested.save));
#endif

but there are no uses of CONFIG_DEBUG_KERNEL in all of Linux so it's 
probably not the way one should use that symbol.  Can anybody think of a 
similar alternative?  Or should the memset simply be unconditional?

Paolo