Re: [XEN PATCH v6 2/3] ioreq: do not build arch_vcpu_ioreq_completion() for non-VMX configurations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2024 12:10, 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

Nit: The rename of the option wants to be reflected here, too.

> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -95,4 +95,10 @@ config LTO
>  config ARCH_SUPPORTS_INT128
>       bool
>  
> +#
> +# For platforms that require specific handling of ioreq completion events
> +#
> +config ARCH_VCPU_IOREQ_COMPLETION
> +     bool

If already you add a comment here, then it similarly wants to disambiguate
things by saying "per-vCPU ioreq completion events" or something along these
lines.

> --- 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_VCPU_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;
> +}

My prior comment here remains: Despite pre-existing behavior being to return
"true" here, I question that to be in line with coding-best-practices.pandoc.
Imo the generalization of the stub is a good opportunity to adjust that. But
yes, it could also be done in a separate change. If you really don't want to
do so right here, then
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
with the two cosmetic adjustments (which likely could also be done while
committing).

Jan