Re: [PATCH] KVM: SVM: Update tracked pCPU on pCPU switch

[Jim Mattson]

On Tue, Oct 13, 2026 at 11:11 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> On Tue, Oct 13, 2026 at 10:55 AM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> >
> > On Tue, Oct 13, 2026 at 7:49 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 12, 2026 at 4:58 PM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Oct 12, 2026 at 3:47 PM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
> > [ ... ]
> > > > > @@ -3892,13 +3893,22 @@ static int pre_svm_run(struct kvm_vcpu *vcpu)
> > > > >          * - The new VMCB does not detect a pCPU change and runs on pCPU A with
> > > > >          *   the new ASID allocated on pCPU B, which is potentially used by
> > > > >          *   another vCPU/VM.
> > > > > +        *
> > > > > +        * Note that dirtying both VMCBs is not strictly needed, but the per-CPU
> > > > > +        * tracking is shared for both ASID and clean bits, and that CPU must
> > > > > +        * match where the ASID was allocated.
> > > >
> > > > Is dirtying both VMCBs not strictly needed?
> > > >
> > > > Per APM vol. 2 (rev. 3.45), section 15.15.2:
> > > >
> > > > > The hypervisor must clear the entire VMCB [Clean] field to 0 for a guest, under the following circumstances:
> > > > > ...
> > > > >  - The hypervisor executes the guest on a different CPU core than one used the last time that guest was executed.
> > > > > ...
> > > > > Failure to clear the VMCB Clean bits to zero in these cases may result in undefined behavior.
> > > >
> > > > Suppose vmcb02 runs on pCPU A, migrates to (and runs on) pCPU B, and
> > > > then migrates back to pCPU A with some clean bits set. At the next
> > > > VMRUN, pCPU A will happily load stale state for anything marked clean.
> > > >
> > > > At some point before that VMRUN, something has to clear the clean bits!
> > >
> > > Dirtying both VMCBs is not needed in all cases covered with the new
> > > code here. IIUC, clean bits are VMCB-specific, unlike the ASID here
> > > which is shared.
> > >
> > > Because ASID is shared, we have to reset the ASID generation for both
> > > VMCBs when a new ASID is allocated through either, and update the cpu
> > > variable tracking which pCPU allocated that ASID. Since that cpu
> > > variable is also shared for tracking which pCPU ran the VMCB last, we
> > > also need to clear the clean bits every time we updated. IOW, if we
> > > had two variables, asid_cpu and vmcb_dirty_cpu, we would need to
> > > update asid_cpu on pCPU migration for both VMCBs, but only update
> > > vmcb_run_cpu for the current VMCB.
> > >
> > > This is all annoying and confusing, but that should go away in the
> > > static ASIDs series, and we should go back to only updating pCPU
> > > tracking, flushing the ASID, and clearing the clean bits only for the
> > > current VMCB.
> >
> > I haven't read the static ASIDs series, but in any case, this patch
> > should stand on its own, in case that series never gets committed or
> > gets committed much later.
>
> It does. I just didn't spend time trying to improve things beyond the fix.
>
> >
> > My complaint is with the claim, "dirtying both VMCBs is not strictly
> > needed," which seems to imply that dirtying the inactive VMCB here is
> > optional. In fact, once you update vmcb02.cpu/vmcb01.cpu for both
> > VMCBs, dirtying both *is* strictly needed so the inactive VMCB doesn't
> > later miss a pCPU switch.
>
> Yeah I guess it can be verbed more accurately, something like this:

I don't understand your prevarication. The comment, as written, isn't
"inaccurately verbed." It's wrong. With the current implementation,
dirtying both VMCBs *is* strictly needed.

> Dirtying both VMCBs is generally not required if only one VMCB
> switches pCPUs, but since the pCPU tracking is shared between ASID and
> VMCB clean bits, it must be done once pCPU tracking is updated.

Clear as mud. :)