On Wed, Dec 06, 2023 at 04:04:43PM +0100, Paolo Bonzini wrote:
> On Wed, Dec 6, 2023 at 3:46 PM Michael Roth <michael.roth@xxxxxxx> wrote:
> > > There is no need to check cr0_old or sev_es_enabled(); EFER.LMA is
> > > simply EFER.LME && CR0.PG.
> >
> > Yah, I originally had it like that, but svm_set_cr0() in the kernel only
> > sets it in vcpu->arch.efer it when setting CR0.PG, so I thought it might
> > be safer to be more selective and mirror that handling on the QEMU side
> > so we can leave as much of any other sanity checks on kernel/QEMU side
> > intact as possible. E.g., if some other bug in the kernel ends up
> > unsetting EFER.LMA while paging is still enabled, we'd still notice that
> > when passing it back in via KVM_SET_SREGS*.
> >
> > But agree it's simpler to just always set it based on CR0.PG and EFER.LMA
> > and can send a v3 if that's preferred.
>
> Yeah, in this case I think the chance of something breaking is really,
> really small.
>
> The behavior of svm_set_cr0() is more due to how the surrounding code
> looks like, than anything else.
Ok, seems reasonable. I'll plan to send a v3 with the old_cr0 stuff
dropped.
>
> > > Alternatively, sev_es_enabled() could be an assertion, that is:
> > >
> > > if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK) &&
> > > !(env->efer & MSR_EFER_LMA)) {
> > > /* Workaround for... */
> > > assert(sev_es_enabled());
> > > env->efer |= MSR_EFER_LMA;
> > > }
> > >
> > > What do you think?
> >
> > I'm a little apprehensive about this approach for similar reasons as
> > above
>
> I agree on this. I think it's worth in general to have clear
> expectations, though. If you think it's worrisome, we can commit it
> without assertion now and add it in 9.0.
I think that seems like a good approach. That would give us more time to
discuss the fix/handling on the kernel side, and then as a follow-up we
can tighten down the QEMU handling/expectations based on that.
Thanks,
Mike
>
> Paolo
>
>
