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 <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: Sun, 27 Mar 2022 22:57:03 +0000	[thread overview]
Message-ID: <YkDrv2JdZhVFnGMk@google.com> (raw)
In-Reply-To: <CAAeT=FwkSUb59Uc35CJgerJdBM5ZCUExNnz2Zs2oHFLn0Jjbsg@mail.gmail.com>

On Fri, Mar 25, 2022 at 07:35:39PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> > > > > + */
> > > > > +#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.
> 
> As saving/restoring breakpoint/watchpoint registers for guests
> might need a special handling when AA64DFR0_EL1.BRPs get changed,
> next version of the series will use the data in the array at
> EL2 nVHE.  They are cold data, and almost half of the entries
> will be RAZ at the moment though:)

Shouldn't we always be doing a full context switch based on what
registers are present in hardware? We probably don't want to leave host
watchpoints/breakpoints visible to the guest.

Additionally, if we are narrowing the guest's view of the debug
hardware, are we going to need to start trapping debug register
accesses? Unexposed breakpoint registers are supposed to UNDEF if we
obey the Arm ARM to the letter. Even if we decide to not care about
unexposed regs, I believe certain combinations of ID_AA64DF0_EL1.{BRPs,
CTX_CMPs} might require that we emulate.

--
Thanks,
Oliver

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