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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 1/4] x86/guest: Introduce {get,set}_reg() infrastructure
Date: Wed, 19 Jan 2022 14:28:28 +0100	[thread overview]
Message-ID: <752485db-deec-1011-f65a-d277fc3e404a@suse.com> (raw)
In-Reply-To: <20220117183415.11150-2-andrew.cooper3@citrix.com>

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@suse.com>

Jan