Re: [PATCH v2 1/4] x86/guest: Introduce {get,set}_reg() infrastructure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.01.2022 19:34, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3744,6 +3744,28 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t 
> msr_content,
>      return X86EMUL_EXCEPTION;
>  }
>  
> +uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg)
> +{
> +    ASSERT(v == current || !vcpu_runnable(v));
> +
> +    switch ( reg )
> +    {
> +    default:
> +        return alternative_call(hvm_funcs.get_reg, v, reg);
> +    }
> +}
> +
> +void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
> +{
> +    ASSERT(v == current || !vcpu_runnable(v));
> +
> +    switch ( reg )
> +    {
> +    default:
> +        return alternative_vcall(hvm_funcs.set_reg, v, reg, val);

I'm inclined to ask to drop "return" from here.

Also, for both functions, without it being clear for what kind of
registers beyond MSRs this may want using down the road, I wonder
whether uint64_t is actually wide enough.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2469,6 +2469,33 @@ static bool svm_get_pending_event(struct vcpu *v, 
> struct x86_event *info)
>      return true;
>  }
>  
> +static uint64_t svm_get_reg(struct vcpu *v, unsigned int reg)
> +{
> +    struct domain *d = v->domain;
> +
> +    switch ( reg )
> +    {
> +    default:
> +        printk(XENLOG_G_ERR "%s(%pv, 0x%08x) Bad register\n",
> +               __func__, v, reg);

Is __func__ actually of much use here and in similar further places?

> @@ -852,6 +867,15 @@ static inline int hvm_vmtrace_get_option(
>      return -EOPNOTSUPP;
>  }
>  
> +static inline uint64_t pv_get_reg(struct vcpu *v, unsigned int reg)
> +{
> +    ASSERT_UNREACHABLE();
> +}
> +static inline void pv_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
> +{
> +    ASSERT_UNREACHABLE();
> +}

Were these meant to have hvm_ prefixes?

With at least this last aspect addressed
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan