Re: [PATCH v5 5/8] KVM: nSVM: Use emulator bytes for synthesized nested #NPF

[Tina Zhang]

On 9/5/2026 8:17 AM, Jim Mattson wrote:
> On Mon, Aug 24, 2026 at 5:40 AM Tina Zhang <zhang_wei@xxxxxxxxxxxxxx> wrote:
> > +
> > +static void nested_svm_prepare_synthesized_insn_bytes(struct kvm_vcpu *vcpu)
> > +{
> > +       struct vcpu_svm *svm = to_svm(vcpu);
> > +       struct nested_svm_insn_bytes *synthesized =
> > +               &svm->nested.synthesized_insn_bytes;
> > +       struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
> > +
> > +       static_assert(sizeof(synthesized->insn_bytes) >=
> > +                     sizeof(ctxt->fetch.data));
> > +
> > +       nested_svm_clear_synthesized_insn_bytes(svm);
> > +
> > +       if (!guest_cpu_cap_has(vcpu, X86_FEATURE_DECODEASSISTS))
> > +               return;
> > +
> > +       if (!ctxt || ctxt->eip != kvm_rip_read(vcpu) ||
> > +           ctxt->fetch.end < ctxt->fetch.data ||
> > +           ctxt->fetch.end > ctxt->fetch.data + sizeof(ctxt->fetch.data))
> > +               return;
>
> Peeking into emulator internals violates encapsulation. Rather than
> performing arithmetic on fetch.data and fetch.end, the emulator should
> provide a function for accessing the fetch cache.

Agreed. I will add an emulator helper for this.

> > +       synthesized->insn_len = ctxt->fetch.end - ctxt->fetch.data;
> > +       memcpy(synthesized->insn_bytes, ctxt->fetch.data,
> > +              synthesized->insn_len);
> > +       synthesized->prepared = true;
> > +}
> > +
> >   static void nested_svm_update_vmcb12_insn_bytes(struct kvm_vcpu *vcpu,
> >                                                  struct vmcb *vmcb12,
> >                                                  const struct vmcb *vmcb02)
> >   {
> >          struct vcpu_svm *svm = to_svm(vcpu);
> > +       struct nested_svm_insn_bytes *synthesized =
> > +               &svm->nested.synthesized_insn_bytes;
> >
> >          nested_svm_clear_insn_bytes(vmcb12);
> >
> > @@ -93,11 +127,20 @@ static void nested_svm_update_vmcb12_insn_bytes(struct kvm_vcpu *vcpu,
> >              !nested_svm_vmexit_has_insn_bytes(vmcb02))
> >                  goto out;
> >
> > -       if (svm->nested.vmcb02_insn_bytes_fresh)
> > +       if (svm->nested.vmcb02_insn_bytes_fresh) {
> >                  nested_svm_copy_insn_bytes(vmcb12, vmcb02);
> > +               goto out;
> > +       }
> > +
> > +       if (synthesized->prepared) {
> > +               vmcb12->control.insn_len = synthesized->insn_len;
> > +               memcpy(vmcb12->control.insn_bytes, synthesized->insn_bytes,
> > +                      vmcb12->control.insn_len);
>
> These instruction bytes may be short. Although this is fixed in the
> next commit, introducing a bug in one patch and fixing it in the next
> is bad practice.

DecodeAssists is advertised to L1 only later in the series, so this path 
is not normally active at this point.  That said, I agree that each
patch should be self-contained and provide complete behavior.

I will reorder and split the changes.

> >   struct svm_nested_state {
> >          struct kvm_vmcb_info vmcb02;
> >          u64 hsave_msr;
> > @@ -245,6 +251,12 @@ struct svm_nested_state {
> >
> >          /* True if VMCB02 has instruction bytes from the current hardware exit. */
> >          bool vmcb02_insn_bytes_fresh;
> > +
> > +       /*
> > +        * Cached instruction bytes for the current synthesized nested #NPF.
> > +        * Valid until the corresponding nested VM-Exit is constructed.
> > +        */
> > +       struct nested_svm_insn_bytes synthesized_insn_bytes;
> >   };
>
> I think the implementatiom would be much cleaner if you wrote the
> synthesized instruction bytes directly to the vmcb02, rather than
> going through this staging buffer.
Sounds good. I'll remove the staging buffer and write the synthesized 
instruction bytes directly to VMCB02 in the next version.


Thanks,
Tina