RE: [PATCH] xen/vm_event: introduce vm_event_is_enabled()

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, September 12, 2025 3:30 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>; Tamas K Lengyel
> <tamas@xxxxxxxxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>; Petre Pircalabu
> <ppircalabu@xxxxxxxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Oleksii Kurochko
> <oleksii.kurochko@xxxxxxxxx>
> Subject: Re: [PATCH] xen/vm_event: introduce vm_event_is_enabled()
>
> On 12.09.2025 06:52, Penny Zheng wrote:
> > @@ -2462,9 +2461,8 @@ int hvm_set_cr3(unsigned long value, bool noflush,
> bool may_defer)
> >      if ( may_defer && unlikely(currd->arch.monitor.write_ctrlreg_enabled &
> >                                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) )
> >      {
> > -        ASSERT(curr->arch.vm_event);
> > -
> > -        if ( hvm_monitor_crX(CR3, value, curr->arch.hvm.guest_cr[3]) )
> > +        if ( vm_event_is_enabled(curr) &&
> > +             hvm_monitor_crX(CR3, value, curr->arch.hvm.guest_cr[3])
> > + )
> >          {
> >              /* The actual write will occur in hvm_do_resume(), if 
> > permitted. */
> >              curr->arch.vm_event->write_data.do_write.cr3 = 1; @@
> > -2544,9 +2542,7 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
> >      if ( may_defer && 
> > unlikely(v->domain->arch.monitor.write_ctrlreg_enabled &
> >                                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR4)) )
> >      {
> > -        ASSERT(v->arch.vm_event);
> > -
> > -        if ( hvm_monitor_crX(CR4, value, old_cr) )
> > +        if ( vm_event_is_enabled(v) && hvm_monitor_crX(CR4, value,
> > + old_cr) )
> >          {
> >              /* The actual write will occur in hvm_do_resume(), if 
> > permitted. */
> >              v->arch.vm_event->write_data.do_write.cr4 = 1; @@ -3407,7
> > +3403,7 @@ static enum hvm_translation_result __hvm_copy(
> >              return HVMTRANS_bad_gfn_to_mfn;
> >          }
> >
> > -        if ( unlikely(v->arch.vm_event) &&
> > +        if ( unlikely(vm_event_is_enabled(v)) &&
> >               (flags & HVMCOPY_linear) &&
> >               v->arch.vm_event->send_event &&
> >               hvm_monitor_check_p2m(addr, gfn, pfec,
> > npfec_kind_with_gla) ) @@ -3538,6 +3534,7 @@ int hvm_vmexit_cpuid(struct
> cpu_user_regs *regs, unsigned int inst_len)
> >      struct vcpu *curr = current;
> >      unsigned int leaf = regs->eax, subleaf = regs->ecx;
> >      struct cpuid_leaf res;
> > +    int ret = 0;
> >
> >      if ( curr->arch.msrs->misc_features_enables.cpuid_faulting &&
> >           hvm_get_cpl(curr) > 0 )
> > @@ -3554,7 +3551,10 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs,
> unsigned int inst_len)
> >      regs->rcx = res.c;
> >      regs->rdx = res.d;
> >
> > -    return hvm_monitor_cpuid(inst_len, leaf, subleaf);
> > +    if ( vm_event_is_enabled(curr) )
> > +        ret = hvm_monitor_cpuid(inst_len, leaf, subleaf);
> > +
> > +    return ret;
> >  }
> >
> >  void hvm_rdtsc_intercept(struct cpu_user_regs *regs) @@ -3694,9
> > +3694,8 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t
> msr_content,
> >          if ( ret != X86EMUL_OKAY )
> >              return ret;
> >
> > -        ASSERT(v->arch.vm_event);
> > -
> > -        if ( hvm_monitor_msr(msr, msr_content, msr_old_content) )
> > +        if ( vm_event_is_enabled(v) &&
> > +             hvm_monitor_msr(msr, msr_content, msr_old_content) )
> >          {
> >              /* The actual write will occur in hvm_do_resume(), if 
> > permitted. */
> >              v->arch.vm_event->write_data.do_write.msr = 1; @@
> > -3854,12 +3853,10 @@ int hvm_descriptor_access_intercept(uint64_t exit_info,
> >      struct vcpu *curr = current;
> >      struct domain *currd = curr->domain;
> >
> > -    if ( currd->arch.monitor.descriptor_access_enabled )
> > -    {
> > -        ASSERT(curr->arch.vm_event);
> > +    if ( currd->arch.monitor.descriptor_access_enabled &&
> > +         vm_event_is_enabled(curr) )
> >          hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
> >                                        descriptor, is_write);
> > -    }
> >      else if ( !hvm_emulate_one_insn(is_sysdesc_access, "sysdesc access") )
> >          domain_crash(currd);
>
> Following "xen: consolidate CONFIG_VM_EVENT" this function is actually
> unreachable when VM_EVENT=n, so no change should be needed here. It's instead
> the unreachability which needs properly taking care of (to satisfy Misra
> requirements) there.
>

I'm a bit confused and may not understand you correctly here.
Just because that hvm_monitor_descriptor_access() will become unreachable codes 
when VM_EVENT=n, and to avoid writing stubs, we added the vm_event_xxx check 
here. Or maybe you want me to add description to say the new checking also 
helps compiling out unreachable codes?

>
> Jan