Re: [Xen-devel] [PATCH 7/7] x86/viridian: implement the crash MSRs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.17 at 10:57, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -234,6 +247,10 @@ void cpuid_viridian_leaves(const struct vcpu *v, 
> uint32_t leaf,
>  
>          res->a = u.lo;
>          res->b = u.hi;
> +
> +        if ( viridian_feature_mask(d) & HVMPV_crash_ctl )
> +            res->d = CPUID3D_CRASH_MSRS;

|= (for consistency with other code as well as to avoid the need
to touch this line again going forward)

> @@ -603,6 +620,37 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>              update_reference_tsc(d, 1);
>          break;
>  
> +    case HV_X64_MSR_CRASH_P0:
> +    case HV_X64_MSR_CRASH_P1:
> +    case HV_X64_MSR_CRASH_P2:
> +    case HV_X64_MSR_CRASH_P3:
> +    case HV_X64_MSR_CRASH_P4:
> +        BUILD_BUG_ON(HV_X64_MSR_CRASH_P4 - HV_X64_MSR_CRASH_P0 >
> +                     ARRAY_SIZE(d->arch.hvm_domain.viridian.crash_param));

>= as it looks.

> +        idx -= HV_X64_MSR_CRASH_P0;
> +        d->arch.hvm_domain.viridian.crash_param[idx] = val;
> +        break;
> +
> +    case HV_X64_MSR_CRASH_CTL:
> +    {
> +        HV_CRASH_CTL_REG_CONTENTS ctl;
> +
> +        ctl.AsUINT64 = val;
> +
> +        if ( !ctl.CrashNotify )
> +            break;
> +
> +        printk(XENLOG_G_INFO "d%d: VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
> +               d->domain_id,
> +               d->arch.hvm_domain.viridian.crash_param[0],
> +               d->arch.hvm_domain.viridian.crash_param[1],
> +               d->arch.hvm_domain.viridian.crash_param[2],
> +               d->arch.hvm_domain.viridian.crash_param[3],
> +               d->arch.hvm_domain.viridian.crash_param[4]);
> +        break;
> +    }
> +
>      default:
>          if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
>              gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> @@ -730,6 +778,25 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
>          break;
>      }
>  
> +    case HV_X64_MSR_CRASH_P0:
> +    case HV_X64_MSR_CRASH_P1:
> +    case HV_X64_MSR_CRASH_P2:
> +    case HV_X64_MSR_CRASH_P3:
> +    case HV_X64_MSR_CRASH_P4:
> +        idx -= HV_X64_MSR_CRASH_P0;
> +        *val = d->arch.hvm_domain.viridian.crash_param[idx];
> +        break;

I think it would be better to reproduce the BUILD_BUG_ON() here.

> +    case HV_X64_MSR_CRASH_CTL:
> +    {
> +        HV_CRASH_CTL_REG_CONTENTS ctl;
> +
> +        ctl.CrashNotify = 1;

You leak 63 bits of hypervisor stack here.

> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -96,6 +96,7 @@ struct viridian_domain
>      union viridian_hypercall_gpa hypercall_gpa;
>      struct viridian_time_ref_count time_ref_count;
>      union viridian_reference_tsc reference_tsc;
> +    uint64_t crash_param[5];

Are these really per-domain values (normally MSRs are per-vCPU)?
And don't they need migrating?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel