Re: [PATCH v3 09/16] x86: provide stubs, declarations and macros in hvm.h

[Wei Liu <wei.liu2@citrix.com>]

On Fri, Sep 07, 2018 at 01:02:02AM -0600, Jan Beulich wrote:
> >>> On 04.09.18 at 18:15, <wei.liu2@citrix.com> wrote:
> > @@ -675,6 +678,100 @@ static inline bool altp2m_vcpu_emulate_ve(struct vcpu *v)
> >          d_->arch.hvm.pi_ops.vcpu_block(v_);                     \
> >  })
> >  
> > +#else  /* CONFIG_HVM */
> > +
> > +#define hvm_enabled false
> > +
> > +/*
> > + * List of inline functions above, of which only declarations are
> > + * needed because DCE will kick in.
> > + */
> 
> With this comment I think ...
> 
> > +int hvm_guest_x86_mode(struct vcpu *v);
> > +unsigned long hvm_get_shadow_gs_base(struct vcpu *v);
> > +void hvm_set_info_guest(struct vcpu *v);
> > +void hvm_cpuid_policy_changed(struct vcpu *v);
> > +void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, uint64_t at_tsc);
> > +
> > +static inline bool hvm_is_singlestep_supported(void)
> 
> ... there should be another comment above here to sort of
> terminate that first comment's effect.

OK.

> 
> > +static inline int hvm_cpu_up(void)
> > +{
> > +    return 0;
> > +}
> > +
> > +static inline void hvm_cpu_down(void) {}
> > +
> > +static inline void hvm_flush_guest_tlbs(void) {}
> > +
> > +static inline void hvm_update_host_cr3(struct vcpu *v)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +}
> 
> Here and below - why ASSERT_UNREACHABLE() instead of the declaration
> only approach above? (If it really needs to be this way, I think it would
> help if the patch description said why.)
> 

Shadow code has some code paths which are HVM only but haven't been
cleaned up. I will add a comment to that effect.


> > +static inline int hvm_event_pending(struct vcpu *v)
> > +{
> > +    return 0;
> > +}
> 
> Would there be an issue if you made this return bool and take pointer
> to const right away, even without touching the "full" function? Perhaps
> the const part would apply to other stubs here as well.

I wanted to keep make them have the same prototype. I'm happy to make
the changes here.

> 
> > +#define is_viridian_domain(d) ({(void)(d); false;})
> > +#define has_viridian_time_ref_count(d) ({(void)(d); false;})
> > +#define hvm_long_mode_active(v) ({(void)(v); false;})
> > +#define hvm_pae_enabled(v) ({(void)(v); false;})
> > +#define hvm_get_guest_time(v) ({(void)(v); 0;})
> 
> Perhaps simply without the need to use a gcc extension
> 
> #define is_viridian_domain(d) ((void)(d), false)

Done (for this and others).

Wei.

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