Re: [PATCH v2 2/8] xen/x86: Simplify header dependencies in x86/hvm

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, May 08, 2024 at 01:39:21PM +0100, Alejandro Vallejo wrote:
> Otherwise it's not possible to call functions described in hvm/vlapic.h from the
> inline functions of hvm/hvm.h.
> 
> This is because a static inline in vlapic.h depends on hvm.h, and pulls it
> transitively through vpt.h. The ultimate cause is having hvm.h included in any
> of the "v*.h" headers, so break the cycle moving the guilty inline into hvm.h.
> 
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

One cosmetic comment below.

> ---
> v2:
>   * New patch. Prereq to moving vlapic_cpu_policy_changed() onto hvm.h
> ---
>  xen/arch/x86/hvm/irq.c                | 6 +++---
>  xen/arch/x86/hvm/vlapic.c             | 4 ++--
>  xen/arch/x86/include/asm/hvm/hvm.h    | 6 ++++++
>  xen/arch/x86/include/asm/hvm/vlapic.h | 6 ------
>  xen/arch/x86/include/asm/hvm/vpt.h    | 1 -
>  5 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index 4a9fe82cbd8d..4f5479b12c98 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -512,13 +512,13 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
>      int vector;
>  
>      /*
> -     * Always call vlapic_sync_pir_to_irr so that PIR is synced into IRR when
> -     * using posted interrupts. Note this is also done by
> +     * Always call hvm_vlapic_sync_pir_to_irr so that PIR is synced into IRR
> +     * when using posted interrupts. Note this is also done by
>       * vlapic_has_pending_irq but depending on which interrupts are pending
>       * hvm_vcpu_has_pending_irq will return early without calling
>       * vlapic_has_pending_irq.
>       */
> -    vlapic_sync_pir_to_irr(v);
> +    hvm_vlapic_sync_pir_to_irr(v);
>  
>      if ( unlikely(v->arch.nmi_pending) )
>          return hvm_intack_nmi;
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 61a96474006b..8a244100009c 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -98,7 +98,7 @@ static void vlapic_clear_irr(int vector, struct vlapic *vlapic)
>  
>  static int vlapic_find_highest_irr(struct vlapic *vlapic)
>  {
> -    vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic));
> +    hvm_vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic));
>  
>      return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]);
>  }
> @@ -1516,7 +1516,7 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h)
>      if ( !has_vlapic(v->domain) )
>          return 0;
>  
> -    vlapic_sync_pir_to_irr(v);
> +    hvm_vlapic_sync_pir_to_irr(v);
>  
>      return hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, vcpu_vlapic(v)->regs);
>  }
> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
> index e1f0585d75a9..84911f3ebcb4 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -798,6 +798,12 @@ static inline void hvm_update_vlapic_mode(struct vcpu *v)
>          alternative_vcall(hvm_funcs.update_vlapic_mode, v);
>  }
>  
> +static inline void hvm_vlapic_sync_pir_to_irr(struct vcpu *v)
> +{
> +    if ( hvm_funcs.sync_pir_to_irr )
> +        alternative_vcall(hvm_funcs.sync_pir_to_irr, v);

Nit: for consistency the wrappers are usually named hvm_<hook_name>,
so in this case it would be hvm_sync_pir_to_irr(), or the hvm_funcs
field should be renamed to vlapic_sync_pir_to_irr.

Thanks, Roger.