Re: [XEN PATCH v3 14/16] ioreq: make arch_vcpu_ioreq_completion() an optional callback

[Julien Grall <julien@xxxxxxx>]

Hi Sergiy,

On 03/06/2024 12:34, Sergiy Kibrik wrote:
> For the most cases arch_vcpu_ioreq_completion() routine is just an empty stub,
> except when handling VIO_realmode_completion, which only happens on HVM
> domains running on VT-x machine. When VT-x is disabled in build configuration,
> both x86 & arm version of routine become empty stubs.
> To dispose of these useless stubs we can do optional call to arch-specific
> ioreq completion handler, if it's present, and drop arm and generic x86 
> handlers.
> Actual handling of VIO_realmore_completion can be done by VMX code then.
>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> ---
>   xen/arch/arm/ioreq.c       |  6 ------
>   xen/arch/x86/hvm/ioreq.c   | 23 -----------------------
>   xen/arch/x86/hvm/vmx/vmx.c | 16 ++++++++++++++++
>   xen/common/ioreq.c         |  5 ++++-
>   xen/include/xen/ioreq.h    |  2 +-
>   5 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
> index 5df755b48b..2e829d2e7f 100644
> --- a/xen/arch/arm/ioreq.c
> +++ b/xen/arch/arm/ioreq.c
> @@ -135,12 +135,6 @@ bool arch_ioreq_complete_mmio(void)
>       return false;
>   }
>   
> -bool arch_vcpu_ioreq_completion(enum vio_completion completion)
> -{
> -    ASSERT_UNREACHABLE();
> -    return true;
> -}
> -
>   /*
>    * The "legacy" mechanism of mapping magic pages for the IOREQ servers
>    * is x86 specific, so the following hooks don't need to be implemented on 
> Arm:
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 4eb7a70182..088650e007 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -29,29 +29,6 @@ bool arch_ioreq_complete_mmio(void)
>       return handle_mmio();
>   }
>   
> -bool arch_vcpu_ioreq_completion(enum vio_completion completion)
> -{
> -    switch ( completion )
> -    {
> -    case VIO_realmode_completion:
> -    {
> -        struct hvm_emulate_ctxt ctxt;
> -
> -        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> -        vmx_realmode_emulate_one(&ctxt);
> -        hvm_emulate_writeback(&ctxt);
> -
> -        break;
> -    }
> -
> -    default:
> -        ASSERT_UNREACHABLE();
> -        break;
> -    }
> -
> -    return true;
> -}
> -
>   static gfn_t hvm_alloc_legacy_ioreq_gfn(struct ioreq_server *s)
>   {
>       struct domain *d = s->target;
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index f16faa6a61..7187d1819c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -10,6 +10,7 @@
>   #include <xen/param.h>
>   #include <xen/trace.h>
>   #include <xen/sched.h>
> +#include <xen/ioreq.h>
>   #include <xen/irq.h>
>   #include <xen/softirq.h>
>   #include <xen/domain_page.h>
> @@ -2749,6 +2750,20 @@ static void cf_check vmx_set_reg(struct vcpu *v, 
> unsigned int reg, uint64_t val)
>       vmx_vmcs_exit(v);
>   }
>   
> +bool realmode_vcpu_ioreq_completion(enum vio_completion completion)
No one seems to call this function outside of vmx.c. So can it be 'static'?

> +{
> +    struct hvm_emulate_ctxt ctxt;
> +
> +    if ( completion != VIO_realmode_completion )
> +        ASSERT_UNREACHABLE();
> +
> +    hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> +    vmx_realmode_emulate_one(&ctxt);
> +    hvm_emulate_writeback(&ctxt);
> +
> +    return true;
> +}
> +
>   static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
>       .name                 = "VMX",
>       .cpu_up_prepare       = vmx_cpu_up_prepare,
> @@ -3070,6 +3085,7 @@ const struct hvm_function_table * __init start_vmx(void)
>       lbr_tsx_fixup_check();
>       ler_to_fixup_check();
>   
> +    arch_vcpu_ioreq_completion = realmode_vcpu_ioreq_completion;
>       return &vmx_function_table;
>   }
>   
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index 1257a3d972..94fde97ece 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -33,6 +33,8 @@
>   #include <public/hvm/ioreq.h>
>   #include <public/hvm/params.h>
>   
> +bool (*arch_vcpu_ioreq_completion)(enum vio_completion completion) = NULL;
I don't think this should be allowed to be modified after boot. So I 
woudl add __ro_after_init.
> +
>   void ioreq_request_mapcache_invalidate(const struct domain *d)
>   {
>       struct vcpu *v = current;
> @@ -244,7 +246,8 @@ bool vcpu_ioreq_handle_completion(struct vcpu *v)
>           break;
>   
>       default:
> -        res = arch_vcpu_ioreq_completion(completion);
> +        if ( arch_vcpu_ioreq_completion )
> +            res = arch_vcpu_ioreq_completion(completion);
I think this wants an:

else {
  ASSERT_UNREACHABLE();
}

So this match the existing code. But I am not fully convinced that this 
is the right approach. Arch_vcpu_ioreq_completion is not meant to change 
across boot (or even at compile time for Arm).
Reading the previous thread, I think something like below would work:

static arch_vcpu_ioreq_completion(...)
{
#ifdef CONFIG_VMX
/* Existing code */
#else
ASSERT_UNREACHABLE();
return true;
#endif
}

If we want to avoid stub, then I think it would be better to use

#ifdef CONFIG_VMX
static  arch_vcpu_ioreq...
{
}
#endif CONFIG_VMX

Then in the x86 header.

#ifdef CONFIG_VMX
static arch_vcpu_ioreq..();
#define arch_vcpu_ioreq...
#endif

And then in common/ioreq.c

#ifdef arch_vcpu_ioreq
res = arch_vcpu_ioreq(...)
#else
ASSERT_UNREACHABLE();
#endif

>           break;
>       }
>   
> diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
> index cd399adf17..880214ec41 100644
> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -111,7 +111,7 @@ void ioreq_domain_init(struct domain *d);
>   int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool 
> *const_op);
>   
>   bool arch_ioreq_complete_mmio(void);
> -bool arch_vcpu_ioreq_completion(enum vio_completion completion);
> +extern bool (*arch_vcpu_ioreq_completion)(enum vio_completion completion);
>   int arch_ioreq_server_map_pages(struct ioreq_server *s);
>   void arch_ioreq_server_unmap_pages(struct ioreq_server *s);
>   void arch_ioreq_server_enable(struct ioreq_server *s);
--
Julien Grall