Re: [PATCH v6 02/25] KVM: arm64: Save ID registers' sanitized value per guest

From: Oliver Upton <oupton@google.com>
To: Reiji Watanabe <reijiw@google.com>
Cc: Marc Zyngier <maz@kernel.org>,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Will Deacon <will@kernel.org>, Andrew Jones <drjones@redhat.com>,
	Fuad Tabba <tabba@google.com>,
	Peng Liang <liangpeng10@huawei.com>,
	Peter Shier <pshier@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>
Subject: Re: [PATCH v6 02/25] KVM: arm64: Save ID registers' sanitized value per guest
Date: Thu, 24 Mar 2022 17:54:48 +0000	[thread overview]
Message-ID: <YjywaFuHp8DL7Q9T@google.com> (raw)
In-Reply-To: <8adf6145-085e-9868-b2f8-65dfbdb5d88f@google.com>

Hi Reiji,

On Thu, Mar 24, 2022 at 09:23:10AM -0700, Reiji Watanabe wrote:

[...]

> > > + */
> > > +#define KVM_ARM_ID_REG_MAX_NUM	64
> > > +#define IDREG_IDX(id)		((sys_reg_CRm(id) << 3) | sys_reg_Op2(id))
> > > +
> > >   struct kvm_arch {
> > >   	struct kvm_s2_mmu mmu;
> > > @@ -137,6 +144,9 @@ struct kvm_arch {
> > >   	/* Memory Tagging Extension enabled for the guest */
> > >   	bool mte_enabled;
> > >   	bool ran_once;
> > > +
> > > +	/* ID registers for the guest. */
> > > +	u64 id_regs[KVM_ARM_ID_REG_MAX_NUM];
> > 
> > This is a decently large array. Should we embed it in kvm_arch or
> > allocate at init?
> 
> 
> What is the reason why you think you might want to allocate it at init ?

Well, its a 512 byte array of mostly cold data. We're probably
convinced that the guest is going to access these registers at most once
per vCPU at boot.

For the vCPU context at least, we only allocate space for registers we
actually care about (enum vcpu_sysreg). My impression of the feature
register ranges is that there are a lot of registers which are RAZ, so I
don't believe we need to make room for uninteresting values.

Additionally, struct kvm is visible to EL2 if running nVHE. I
don't believe hyp will ever need to look at these register values.

[...]

> > Which itself is dependent on whether KVM is going to sparsely or
> > verbosely define its feature filtering tables per the other thread. So
> > really only bother with this if that is the direction you're going.
> 
> Even just going through for ID register ranges, we should do the check
> to skip hidden/reserved ID registers (not to call read_sanitised_ftr_reg).
> 
> Yes, it's certainly possible to avoid walking the entire system register,
> and I will fix it.  The reason why I didn't care it so much was just
> because the code (walking the entire system register) will be removed by
> the following patches:)

Let me go through the series again and see how this flows. If there is a
way to avoid rewriting code introduced earlier in the series I would
suggest going that route.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel