Re: [XEN PATCH v5 11/13] ioreq: do not build arch_vcpu_ioreq_completion() for non-VMX configurations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2024 12:37, Sergiy Kibrik wrote:
> From: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> 
> VIO_realmode_completion is specific to vmx realmode and thus the function
> arch_vcpu_ioreq_completion() has actual handling work only in VMX-enabled 
> build,
> as for the rest x86 and ARM build configurations it is basically a stub.
> 
> Here a separate configuration option ARCH_IOREQ_COMPLETION introduced that 
> tells
> whether the platform we're building for requires any specific ioreq completion
> handling. As of now only VMX has such requirement, so the option is selected
> by INTEL_VMX, for other configurations a generic default stub is provided
> (it is ARM's version of arch_vcpu_ioreq_completion() moved to common header).
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> changes in v5:
>  - introduce ARCH_IOREQ_COMPLETION option & put arch_vcpu_ioreq_completion() 
> under it
>  - description changed

I'm worried by this naming inconsistency: We also have 
arch_ioreq_complete_mmio(),
and who know what else we'll gain. I think the Kconfig identifier wants to 
equally
include VCPU.

> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -95,4 +95,7 @@ config LTO
>  config ARCH_SUPPORTS_INT128
>       bool
>  
> +config ARCH_IOREQ_COMPLETION
> +     bool

Please maintain alphabetic sorting with the other ARCH_*.

> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -111,7 +111,17 @@ 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);
> +
> +#ifdef CONFIG_ARCH_IOREQ_COMPLETION
>  bool arch_vcpu_ioreq_completion(enum vio_completion completion);
> +#else
> +static inline bool arch_vcpu_ioreq_completion(enum vio_completion completion)
> +{
> +    ASSERT_UNREACHABLE();
> +    return true;
> +}

I understand this is how the Arm stub had it, but is "true" really appropriate
here? Looking at the sole vcpu_ioreq_handle_completion() call site in x86 code,
I'm inclined to say "false" would be better: We shouldn't resume a vCPU when
such an (internal) error has been encountered.

Jan