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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Aug 20, 2018 at 06:37:28AM -0600, Jan Beulich wrote:
> >>> 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)".

This patch can be dropped with adjustment to other files.

> 
> > +#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.

This is useful for developers.

I would certainly build with both PV and HVM disabled in the future to
catch any wrong assumptions that creep into x86 common code.

(Though in this instance the addition here is not needed)

Wei.

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