Re: [PATCH] x86/HVM: replace paging_mode_hap() uses

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Mon, Sep 21, 2026 at 03:45:29PM +0200, Alejandro Vallejo wrote:
> On Mon Sep 21, 2026 at 3:19 PM CEST, Jan Beulich wrote:
> > HVM guests cannot run without either HAP or shadow enabled. While
> > paging_mode_shadow() is compile-time-constant when SHADOW_PAGING=n,
> > paging_mode_hap() isn't. Hence the former is preferred to leverage DCE.
> >
> > In svm_update_guest_cr() combine two adjacent conditionals.
> >
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Yes, please.
> 
>   Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
> 
> A couple of nits below. Take them or leave them.
> 
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -113,7 +113,8 @@ static void cf_check svm_update_guest_cr
> >      switch ( cr )
> >      {
> >      case 0:
> > -        if ( paging_mode_hap(v->domain) )
> > +        value = v->arch.hvm.guest_cr[0];
> > +        if ( !paging_mode_shadow(v->domain) )
> >          {
> >              uint32_t intercepts = vmcb_get_cr_intercepts(vmcb);
> >  
> > @@ -122,9 +123,7 @@ static void cf_check svm_update_guest_cr
> >                   monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
> >                 vmcb_set_cr_intercepts(vmcb, intercepts | 
> > CR_INTERCEPT_CR3_WRITE);
> >          }
> > -
> > -        value = v->arch.hvm.guest_cr[0];
> > -        if ( paging_mode_shadow(v->domain) )
> > +        else
> >              value |= X86_CR0_PG | X86_CR0_WP;
> 
> nit: This would be clearer with the polarity reversed. Check shadow
> first and have hap later. It'd also make the diff (marginally) smaller too.
> 
> >          vmcb_set_cr0(vmcb, value);
> >          break;
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> 
> [snip]
> 
> >      v->arch.hvm.vmx.exception_bitmap = HVM_TRAP_MASK
> > -              | (paging_mode_hap(d) ? 0 : (1U << X86_EXC_PF));
> > +        | (!paging_mode_shadow(d) ? 0 : (1U << X86_EXC_PF));
> 
> nit: Shouldn't | be on the prior line? It was there before, but seeing
> how you're adjusting indentation might as well move that char.
> 
> In the same vein as before, it'd be a bit clearer with the polarity
> inverted (shadow() ? bit : 0 )

I agree with both suggestions in principle, always better if we can
avoid negations.

Acked-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>

Thanks, Roger.