Date:   Sat, 4 Mar 2023 14:07:09 +0200
From:   Zhi Wang <zhi.wang.linux@gmail.com>
To:     Steven Price <steven.price@arm.com>
Cc:     kvm@vger.kernel.org, kvmarm@lists.linux.dev,
        Catalin Marinas <catalin.marinas@arm.com>,
        Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
        James Morse <james.morse@arm.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Joey Gouly <joey.gouly@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Christoffer Dall <christoffer.dall@arm.com>,
        Fuad Tabba <tabba@google.com>, linux-coco@lists.linux.dev
Subject: Re: [RFC PATCH 04/28] arm64: RME: Check for RME support at KVM init
Message-ID: <20230304140709.0000112e@gmail.com>
In-Reply-To: <748a6bcf-ec16-0870-8e33-bc29ab311211@arm.com>
References: <20230127112248.136810-1-suzuki.poulose@arm.com>
        <20230127112932.38045-1-steven.price@arm.com>
        <20230127112932.38045-5-steven.price@arm.com>
        <20230213174846.00003fad@gmail.com>
        <748a6bcf-ec16-0870-8e33-bc29ab311211@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 13 Feb 2023 15:59:05 +0000
Steven Price <steven.price@arm.com> wrote:

> On 13/02/2023 15:48, Zhi Wang wrote:
> > On Fri, 27 Jan 2023 11:29:08 +0000
> > Steven Price <steven.price@arm.com> wrote:
> > 
> >> Query the RMI version number and check if it is a compatible version. A
> >> static key is also provided to signal that a supported RMM is available.
> >>
> >> Functions are provided to query if a VM or VCPU is a realm (or rec)
> >> which currently will always return false.
> >>
> >> Signed-off-by: Steven Price <steven.price@arm.com>
> >> ---
> >>  arch/arm64/include/asm/kvm_emulate.h | 17 ++++++++++
> >>  arch/arm64/include/asm/kvm_host.h    |  4 +++
> >>  arch/arm64/include/asm/kvm_rme.h     | 22 +++++++++++++
> >>  arch/arm64/include/asm/virt.h        |  1 +
> >>  arch/arm64/kvm/Makefile              |  3 +-
> >>  arch/arm64/kvm/arm.c                 |  8 +++++
> >>  arch/arm64/kvm/rme.c                 | 49 ++++++++++++++++++++++++++++
> >>  7 files changed, 103 insertions(+), 1 deletion(-)
> >>  create mode 100644 arch/arm64/include/asm/kvm_rme.h
> >>  create mode 100644 arch/arm64/kvm/rme.c
> >>
> >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> >> index 9bdba47f7e14..5a2b7229e83f 100644
> >> --- a/arch/arm64/include/asm/kvm_emulate.h
> >> +++ b/arch/arm64/include/asm/kvm_emulate.h
> >> @@ -490,4 +490,21 @@ static inline bool vcpu_has_feature(struct kvm_vcpu *vcpu, int feature)
> >>  	return test_bit(feature, vcpu->arch.features);
> >>  }
> >>  
> >> +static inline bool kvm_is_realm(struct kvm *kvm)
> >> +{
> >> +	if (static_branch_unlikely(&kvm_rme_is_available))
> >> +		return kvm->arch.is_realm;
> >> +	return false;
> >> +}
> >> +
> >> +static inline enum realm_state kvm_realm_state(struct kvm *kvm)
> >> +{
> >> +	return READ_ONCE(kvm->arch.realm.state);
> >> +}
> >> +
> >> +static inline bool vcpu_is_rec(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return false;
> >> +}
> >> +
> >>  #endif /* __ARM64_KVM_EMULATE_H__ */
> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >> index 35a159d131b5..04347c3a8c6b 100644
> >> --- a/arch/arm64/include/asm/kvm_host.h
> >> +++ b/arch/arm64/include/asm/kvm_host.h
> >> @@ -26,6 +26,7 @@
> >>  #include <asm/fpsimd.h>
> >>  #include <asm/kvm.h>
> >>  #include <asm/kvm_asm.h>
> >> +#include <asm/kvm_rme.h>
> >>  
> >>  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
> >>  
> >> @@ -240,6 +241,9 @@ struct kvm_arch {
> >>  	 * the associated pKVM instance in the hypervisor.
> >>  	 */
> >>  	struct kvm_protected_vm pkvm;
> >> +
> >> +	bool is_realm;
> >                ^
> > It would be better to put more comments which really helps on the review.
> 
> Thanks for the feedback - I had thought "is realm" was fairly
> self-documenting, but perhaps I've just spent too much time with this code.
> 
> > I was looking for the user of this memeber to see when it is set. It seems
> > it is not in this patch. It would have been nice to have a quick answer from the
> > comments.
> 
> The usage is in the kvm_is_realm() function which is used in several of
> the later patches as a way to detect this kvm guest is a realm guest.
> 
> I think the main issue is that I've got the patches in the wrong other.
> Patch 7 "arm64: kvm: Allow passing machine type in KVM creation" should
> probably be before this one, then I could add the assignment of is_realm
> into this patch (potentially splitting out the is_realm parts into
> another patch).
> 

I agree the patch order seems a problem here. The name is self-documenting
but if the user of the variable is not in this patch, still needs to jump to
the related patch to confirm if the variable is used as expected. In that
situation, a comment would help to avoid jumping between patches (sometimes
finding the the user of a variable from a patch bundle really slows down
the review progress and eventually you have to open a terminal and check
it in the git tree).

> Thanks,
> 
> Steve
>