Re: [PATCH v1 3/3] xen/domain: rewrite emulation_flags_ok()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2025 02:52, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Rewrite emulation_flags_ok() using XEN_X86_EMU_{OPTIONAL,BASELINE}
> to simplify future modifications.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Came in the context of NS16550 emulator v3 series:
>   
> https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-0-c5d36b31d66c@xxxxxxxx/
> 
> After modifying emulation_flags_ok() with a new NS16550 vUART
> configuration switch passed from the toolstack for the HVM
> case, I decided to look into how to improve emulation_flags_ok().
> ---
>  xen/arch/x86/domain.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)

There is a readability win, yes.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -750,25 +750,18 @@ static bool emulation_flags_ok(const struct domain *d, 
> uint32_t emflags)
>      BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);
>  #endif
>  
> -    if ( is_hvm_domain(d) )
> -    {
> -        if ( is_hardware_domain(d) &&
> -             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
> -            return false;
> -        if ( !is_hardware_domain(d) &&
> -             /* HVM PIRQ feature is user-selectable. */
> -             (emflags & ~X86_EMU_USE_PIRQ) !=
> -             (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) &&
> -             emflags != X86_EMU_LAPIC )
> -            return false;
> -    }
> -    else if ( emflags != 0 && emflags != X86_EMU_PIT )
> -    {
> -        /* PV or classic PVH. */
> -        return false;
> -    }
> +    /* PV or classic PVH */
> +    if ( !is_hvm_domain(d) )
> +        return emflags == 0 || emflags == XEN_X86_EMU_PIT;

What's "classic PVH" here? This got to be PVHv1, which is dead. As you touch /
move such a comment, you want to adjust it so it's at least no longer stale.

> -    return true;
> +    /* HVM */
> +    if ( is_hardware_domain(d) )
> +        return emflags == (XEN_X86_EMU_LAPIC |
> +                           XEN_X86_EMU_IOAPIC |
> +                           XEN_X86_EMU_VPCI);
> +
> +    return (emflags & ~XEN_X86_EMU_OPTIONAL) == XEN_X86_EMU_BASELINE ||
> +            emflags == XEN_X86_EMU_LAPIC;

There was a comment about X86_EMU_USE_PIRQ being optional, which you've lost
together with (only) that (i.e. not at the same time including vPCI) 
optionality.

Furthermore you move from X86_EMU_* namespace to XEN_X86_EMU_* one without even
mentioning that (and the reason to do so) in the description. Aiui the function
deliberately uses the internal names, not the public interface ones.

Jan