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;
Date:   Wed, 13 Oct 2021 22:04:05 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Joerg Roedel <joro@8bytes.org>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>, x86@kernel.org,
        Brijesh Singh <brijesh.singh@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Joerg Roedel <jroedel@suse.de>
Subject: Re: [PATCH v4 3/5] KVM: SVM: Add support to handle AP reset MSR
 protocol
Message-ID: <YWdX1WXE3AOPFC6d@google.com>
References: <20210929155330.5597-1-joro@8bytes.org>
 <20210929155330.5597-4-joro@8bytes.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210929155330.5597-4-joro@8bytes.org>
Precedence: bulk

On Wed, Sep 29, 2021, Joerg Roedel wrote:
>  #define PFERR_PRESENT_BIT 0
>  #define PFERR_WRITE_BIT 1
> @@ -908,6 +913,8 @@ struct kvm_vcpu_arch {
>  #if IS_ENABLED(CONFIG_HYPERV)
>  	hpa_t hv_root_tdp;
>  #endif
> +
> +	enum ap_reset_hold_type reset_hold_type;


Apologies for very belated feedback...

This living in kvm_vcpu_arch came about from feedback (see bottom) that _if_
kvm_emulate_ap_reset_hold() is in x86.c, so should the hold type information.


But clearing the hold in SEV here...

>  void sev_es_unmap_ghcb(struct vcpu_svm *svm)
>  {
> +	/* Clear any indication that the vCPU is in a type of AP Reset Hold */
> +	svm->vcpu.arch.reset_hold_type = AP_RESET_HOLD_NONE;
> +
>  	if (!svm->ghcb)
>  		return;

makes this completely unbalanced, i.e. common x86 doesn't clear the reset_hold_type
when the vCPU is awakened, despite it being set in common x86.  More at the end.

> -int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu)
> +int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu,
> +			      enum ap_reset_hold_type type)
>  {
>  	int ret = kvm_skip_emulated_instruction(vcpu);
>  
> +	vcpu->arch.reset_hold_type = type;
> +
>  	return __kvm_vcpu_halt(vcpu, KVM_MP_STATE_AP_RESET_HOLD, KVM_EXIT_AP_RESET_HOLD) && ret;
>  }
>  EXPORT_SYMBOL_GPL(kvm_emulate_ap_reset_hold);

...

On Thu, Jul 15, 2021, Tom Lendacky wrote:
> On 7/15/21 10:45 AM, Sean Christopherson wrote:
> > On Thu, Jul 15, 2021, Tom Lendacky wrote:
> >> On 7/14/21 3:17 PM, Sean Christopherson wrote:
> >>>> +        case GHCB_MSR_AP_RESET_HOLD_REQ:
> >>>> +                svm->ap_reset_hold_type = AP_RESET_HOLD_MSR_PROTO;
> >>>> +                ret = kvm_emulate_ap_reset_hold(&svm->vcpu);
> >>>
> >>> The hold type feels like it should be a param to kvm_emulate_ap_reset_hold().
> >>
> >> I suppose it could be, but then the type would have to be tracked in the
> >> kvm_vcpu_arch struct instead of the vcpu_svm struct, so I opted for the
> >> latter. Maybe a helper function, sev_ap_reset_hold(), that sets the type
> >> and then calls kvm_emulate_ap_reset_hold(), but I'm not seeing a big need
> >> for it.
> >
> > Huh.  Why is kvm_emulate_ap_reset_hold() in x86.c?  That entire concept is very
> > much SEV specific.  And if anyone argues its not SEV specific, then the hold type
> > should also be considered generic, i.e. put in kvm_vcpu_arch.
>
> That was based on review comments where it was desired that the halt be
> identified as specifically from the AP reset hold vs a normal halt.

The reason I emphasized "if", is that IMO this patch goes in the wrong direction.
My feedback here was that kvm_emulate_ap_reset_hold() and reset_hold_type should
tied together.  I completely agree with the review comments Tom mentioned, but IMO
adding a common kvm_emulate_ap_reset_hold() was the wrong solution.  That's very
much an SEV specific concept, as demonstrated by this patch.

Rather than put more stuff into x86 that really belongs to SEV, what about moving
kvm_emulate_ap_reset_hold() into sev.c and instead exporting __kvm_vcpu_halt()?

Note, there's a conflict there with a proposed function rename[*], but it's minor
and should be trivial to resolve depending how which series wins the race.

[*] https://lkml.kernel.org/r/20211009021236.4122790-13-seanjc@google.com