Re: [Xen-devel] [PATCH v2 13/23] x86: provide stubs, declarations and macros in hvm.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.08.18 at 14:19, <wei.liu2@xxxxxxxxxx> wrote:
> Make sure hvm_enabled evaluate to false then provide necessary stubs,
> declarations and macros to make Xen build.
> 
> The is_viridian_domain macro can't be turned into an inline function
> easily, so instead its caller is modified to avoid unused variable
> warning.

I assume that's because struct domain hasn't been fully declared
yet at that point. Some preliminary looking around suggests that
it may be possible to address this by moving it plus a few more
nearby items into hvm/viridian.h, and avoiding other headers in
hvm/ to include hvm/viridian.h. This last item would in turn seem
to require to convert struct viridian_domain the instance in
struct hvm_domain to a pointer, which I personally think would
be desirable anyway. Paul?

Otoh I'm not really sure all of this is worth the effort.

> @@ -268,8 +271,8 @@ u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc);
>  #define hvm_tsc_scaling_ratio(d) \
>      ((d)->arch.hvm_domain.tsc_scaling_ratio)
>  
> -u64 hvm_scale_tsc(const struct domain *d, u64 tsc);
> -u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz);
> +uint64_t hvm_scale_tsc(const struct domain *d, uint64_t tsc);
> +uint64_t hvm_get_tsc_scaling_ratio(uint32_t gtsc_khz);

I think this change doesn't really belong here. I don't mind if you
want to keep it, but then I question the need for uint32_t (rather
than unsigned int) for the second prototype's parameter.

> @@ -675,6 +678,146 @@ unsigned long hvm_cr4_guest_valid_bits(const struct 
> domain *d, bool restore);
>          d_->arch.hvm_domain.pi_ops.vcpu_block(v_);          \
>  })
>  
> +#else /* CONFIG_HVM */
> +
> +#define hvm_enabled false
> +
> +static inline int hvm_guest_x86_mode(struct vcpu *v)
> +{
> +    ASSERT_UNREACHABLE();
> +    return -1;
> +}

Hmm, there's a whole lot of things down from here. I'd really like to
see this minimized quite a bit. A first thought: On top of my indirect
call patching series, instead of directly using the macros provided
by the alternatives.h additions, an extra layer of abstraction could
introduce a macro which evaluates to ASSERT_UNREACHABLE()
without HVM, and to alternative_{,v}call() otherwise. I think this
would eliminate quite a few of the newly added inline functions.
I'll therefore skip some of the additions here.

> +#define hvm_long_mode_active(v) (false)
> +#define hvm_pae_enabled(v) (false)
> +#define hvm_get_guest_time(v) (0)
> +#define is_viridian_domain(d) (false)
> +#define has_viridian_time_ref_count(d) (false)
> +#define hvm_tsc_scaling_supported (false)
> +#define hap_has_1gb (false)
> +#define hap_has_2mb (false)
> +
> +#define hvm_paging_enabled(v) (false)
> +#define hvm_nx_enabled(v) (false)
> +#define hvm_wp_enabled(v) (false)
> +#define hvm_smap_enabled(v) (false)
> +#define hvm_smep_enabled(v) (false)
> +#define hvm_pku_enabled(v) (false)
> +
> +#define arch_vcpu_block(v) ((void)v)

Parenthesization is odd here: Just like for hvm_enabled, false
and 0 don't need parentheses, yet arguments should (a) be
properly parenthesized when used and (b) be consistently
evaluated (or not).

> +int hvm_vcpu_initialise(struct vcpu *v);
> +void hvm_vcpu_destroy(struct vcpu *v);
> +int hvm_domain_initialise(struct domain *d);
> +void hvm_domain_destroy(struct domain *d);
> +void hvm_domain_soft_reset(struct domain *d);
> +void hvm_domain_relinquish_resources(struct domain *d);
> +uint64_t hvm_scale_tsc(const struct domain *d, uint64_t tsc);
> +uint64_t hvm_get_tsc_scaling_ratio(uint32_t gtsc_khz);
> +
> +void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
> +                              struct segment_register *reg);
> +
> +void hvm_set_rdtsc_exiting(struct domain *d, bool_t enable);
> +void hvm_toggle_singlestep(struct vcpu *v);
> +void hvm_mapped_guest_frames_mark_dirty(struct domain *);
> +void hvm_hypercall_page_initialise(struct domain *d,
> +                                   void *hypercall_page);

Many if not all of these already have declarations. They shouldn't
be repeated (and risk to go out of sync) in the #if and #else
sections of this conditional. All external declarations are fine to
remain visible regardless of CONFIG_HVM - the linker will complain
if any are referenced without there being a definition anywhere.

Jan


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