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.
> > 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.
Paolo
