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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.18 at 18:15, <wei.liu2@xxxxxxxxxx> 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.

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

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

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

etc? Otherwise please add blanks inside the figure braces.

> +#define hvm_paging_enabled(v) ({(void)(v); false;})
> +#define hvm_wp_enabled(v) ({(void)(v); false;})
> +#define hvm_pcid_enabled(v) ({(void)(v); false;})
> +#define hvm_pae_enabled(v) ({(void)(v); false;})
> +#define hvm_smep_enabled(v) ({(void)(v); false;})
> +#define hvm_smap_enabled(v) ({(void)(v); false;})
> +#define hvm_nx_enabled(v) ({(void)(v); false;})
> +#define hvm_pku_enabled(v) ({(void)(v); false;})

Same here.

Jan



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