On 16.07.2025 22:15, Petr Beneš wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2686,8 +2686,8 @@ static int cf_check hvmemul_tlb_op(
> return rc;
> }
>
> -static int cf_check hvmemul_vmfunc(
> - struct x86_emulate_ctxt *ctxt)
> +#ifdef CONFIG_ALTP2M
> +static int cf_check hvmemul_vmfunc(struct x86_emulate_ctxt *ctxt)
Please don't needlessly alter formatting, and ...
> {
> int rc;
>
> @@ -2699,6 +2699,12 @@ static int cf_check hvmemul_vmfunc(
>
> return rc;
> }
> +#else
> +static int cf_check hvmemul_vmfunc(struct x86_emulate_ctxt *ctxt)
> +{
> + return X86EMUL_UNHANDLEABLE;
> +}
> +#endif
... please don't duplicate function headers when such #ifdef-s can easily
be put inside the function body. Much like you do ...
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4608,6 +4608,7 @@ static int hvmop_get_param(
> static int do_altp2m_op(
> XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> +#ifdef CONFIG_ALTP2M
> struct xen_hvm_altp2m_op a;
> struct domain *d = NULL;
> int rc = 0;
> @@ -4944,6 +4945,9 @@ static int do_altp2m_op(
> rcu_unlock_domain(d);
>
> return rc;
> +#else /* !CONFIG_ALTP2M */
> + return -EOPNOTSUPP;
> +#endif /* CONFIG_ALTP2M */
> }
... here.
> @@ -5261,6 +5269,7 @@ void hvm_toggle_singlestep(struct vcpu *v)
>
> void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx)
> {
> +#ifdef CONFIG_ALTP2M
> ASSERT(atomic_read(&v->pause_count));
>
> if ( !hvm_is_singlestep_supported() )
> @@ -5272,6 +5281,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t
> p2midx)
> v->arch.hvm.single_step = true;
> v->arch.hvm.fast_single_step.enabled = true;
> v->arch.hvm.fast_single_step.p2midx = p2midx;
> +#endif
> }
This function would better be unreachable when ALTP2M=n, imo. Putting an #ifdef
in vm_event_toggle_singlestep() would be the simple solution, but maybe we could
to better.
> @@ -707,6 +709,7 @@ static inline bool hvm_nested_virt_supported(void)
> return hvm_funcs.caps.nested_virt;
> }
>
> +#ifdef CONFIG_ALTP2M
> /* updates the current hardware p2m */
> static inline void altp2m_vcpu_update_p2m(struct vcpu *v)
> {
> @@ -731,6 +734,11 @@ static inline bool altp2m_vcpu_emulate_ve(struct vcpu *v)
> }
> return false;
> }
> +#else /* !CONFIG_ALTP2M */
> +void altp2m_vcpu_update_p2m(struct vcpu *v);
> +void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v);
> +bool altp2m_vcpu_emulate_ve(struct vcpu *v);
> +#endif /* CONFIG_ALTP2M */
Why would the altp2m_vcpu_update_{p2m,vmfunc_ve}() declarations be needed? All
uses are from altp2m.c.
Jan
