[PATCH v3 06/15] KVM: arm64: Restore mdcr_el2 from vcpu

[Will Deacon]

Hey Fuad,

On Thu, Aug 12, 2021 at 11:28:50AM +0200, Fuad Tabba wrote:
> On Thu, Aug 12, 2021 at 10:46 AM Will Deacon <will at kernel.org> wrote:
> >
> > On Wed, Jul 21, 2021 at 08:37:21AM +0100, Fuad Tabba wrote:
> > > On Tue, Jul 20, 2021 at 3:53 PM Andrew Jones <drjones at redhat.com> wrote:
> > > >
> > > > On Mon, Jul 19, 2021 at 05:03:37PM +0100, Fuad Tabba wrote:
> > > > > On deactivating traps, restore the value of mdcr_el2 from the
> > > > > newly created and preserved host value vcpu context, rather than
> > > > > directly reading the hardware register.
> > > > >
> > > > > Up until and including this patch the two values are the same,
> > > > > i.e., the hardware register and the vcpu one. A future patch will
> > > > > be changing the value of mdcr_el2 on activating traps, and this
> > > > > ensures that its value will be restored.
> > > > >
> > > > > No functional change intended.
> > > >
> > > > I'm probably missing something, but I can't convince myself that the host
> > > > will end up with the same mdcr_el2 value after deactivating traps after
> > > > this patch as before. We clearly now restore whatever we had when
> > > > activating traps (presumably whatever we configured at init_el2_state
> > > > time), but is that equivalent to what we had before with the masking and
> > > > ORing that this patch drops?
> > >
> > > You're right. I thought that these were actually being initialized to
> > > the same values, but having a closer look at the code the mdcr values
> > > are not the same as pre-patch. I will fix this.
> >
> > Can you elaborate on the issue here, please? I was just looking at this
> > but aren't you now relying on __init_el2_debug to configure this, which
> > should be fine?
> 
> I *think* that it should be fine, but as Drew pointed out, the host
> does not end up with the same mdcr_el2 value after the deactivation in
> this patch as it did after deactivation before this patch. In my v4
> (not sent out yet), I have fixed it to ensure that the host does end
> up with the same value as the one before this patch. That should make
> it easier to check that there's no functional change.
> 
> I'll look into it further, and if I can convince myself that there
> aren't any issues and that this patch makes the code cleaner, I will
> add it as a separate patch instead to make reviewing easier.

Cheers. I think the new code might actually be better, as things like
MDCR_EL2.E2PB are RES0 if SPE is not implemented. The init code takes care
to set those only if if probes SPE first, whereas the code you're removing
doesn't seem to check that.

Will