Re: [Xen-devel] [PATCH 10/34] x86: stub out has_* testing for emulation flags

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.08.18 at 17:12, <wei.liu2@xxxxxxxxxx> wrote:
> Most are all HVM only. Provide stubs for !CONFIG_HVM.
> 
> One exception is PIT emulation, which is available to both PV and HVM.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  xen/include/asm-x86/domain.h | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 09f6b3d..4c18054 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -440,6 +440,8 @@ struct arch_domain
>      uint32_t emulation_flags;
>  } __cacheline_aligned;
>  
> +#ifdef CONFIG_HVM
> +
>  #define has_vlapic(d)      (!!((d)->arch.emulation_flags & 
> XEN_X86_EMU_LAPIC))
>  #define has_vhpet(d)       (!!((d)->arch.emulation_flags & XEN_X86_EMU_HPET))
>  #define has_vpm(d)         (!!((d)->arch.emulation_flags & XEN_X86_EMU_PM))
> @@ -448,11 +450,31 @@ struct arch_domain
>  #define has_vpic(d)        (!!((d)->arch.emulation_flags & XEN_X86_EMU_PIC))
>  #define has_vvga(d)        (!!((d)->arch.emulation_flags & XEN_X86_EMU_VGA))
>  #define has_viommu(d)      (!!((d)->arch.emulation_flags & 
> XEN_X86_EMU_IOMMU))
> -#define has_vpit(d)        (!!((d)->arch.emulation_flags & XEN_X86_EMU_PIT))
>  #define has_pirq(d)        (!!((d)->arch.emulation_flags & \
>                              XEN_X86_EMU_USE_PIRQ))
>  #define has_vpci(d)        (!!((d)->arch.emulation_flags & XEN_X86_EMU_VPCI))
>  
> +#else
> +
> +#define has_vlapic(d)     (0)
> +#define has_vhpet(d)      (0)
> +#define has_vpm(d)        (0)
> +#define has_vrtc(d)       (0)
> +#define has_vioapic(d)    (0)
> +#define has_vpic(d)       (0)
> +#define has_vvga(d)       (0)
> +#define has_viommu(d)     (0)
> +#define has_pirq(d)       (0)
> +#define has_vpci(d)       (0)
> +
> +#endif

I'm not convinced - this introduces disconnected duplicate logic
(between here and emulation_flags_ok()). If we were to go this
route, I think comments would need to be added on both sides,
each referring to the other one.

Furthermore, if we go this route, then "false" please instead of
"(0)".

> +#if CONFIG_HVM || CONFIG_PV
> +#define has_vpit(d)        (!!((d)->arch.emulation_flags & XEN_X86_EMU_PIT))
> +#else
> +#define has_vpit(d)       (0)
> +#endif

What purpose would a Xen serve with both HVM and PV disabled?
IOW - I don't think any #if is warranted here.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel