On 01.04.2025 02:52, dm...@proton.me wrote:
> From: Denis Mukhin <dmuk...@ford.com>
>
> Rewrite emulation_flags_ok() using XEN_X86_EMU_{OPTIONAL,BASELINE}
> to simplify future modifications.
>
> Signed-off-by: Denis Mukhin <dmuk...@ford.com>
> ---
> Came in the context of NS16550 emulator v3 series:
>
> https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-0-c5d36b31d...@ford.com/
>
> 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
