Re: [Xen-devel] [PATCH v6 09/11] viridian: add implementation of synthetic interrupt MSRs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.03.19 at 12:25, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -131,13 +238,69 @@ int viridian_synic_rdmsr(const struct vcpu *v, uint32_t 
> idx, uint64_t *val)
>          *val = ((uint64_t)icr2 << 32) | icr;
>          break;
>      }
> +
>      case HV_X64_MSR_TPR:
>          *val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
>          break;
>  
>      case HV_X64_MSR_VP_ASSIST_PAGE:
> -        *val = v->arch.hvm.viridian->vp_assist.msr.raw;
> +        *val = vv->vp_assist.msr.raw;
> +        break;
> +
> +    case HV_X64_MSR_SCONTROL:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        *val = vv->scontrol;
> +        break;
> +
> +    case HV_X64_MSR_SVERSION:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        /*
> +         * The specification says that the version number is 0x00000001
> +         * and should be in the lower 32-bits of the MSR, while the
> +         * upper 32-bits are reserved... but it doesn't say what they
> +         * should be set to. Assume everything but the bottom bit
> +         * should be zero.
> +         */
> +        *val = 1ul;
> +        break;
> +
> +    case HV_X64_MSR_SIEFP:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        *val = vv->siefp;
> +        break;
> +
> +    case HV_X64_MSR_SIMP:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        *val = vv->simp.msr.raw;
> +        break;
> +
> +    case HV_X64_MSR_EOM:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        *val = 0;
> +        break;
> +
> +    case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
> +    {
> +        unsigned int sintx = idx - HV_X64_MSR_SINT0;
> +        const union viridian_sint_msr *vs =
> +            &array_access_nospec(vv->sint, sintx);
> +
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        *val = vs->raw;
>          break;

Without this necessarily being a request to change, I still don't
understand why you don't omit vs as a variable and simply do

        *val = array_access_nospec(vv->sint, sintx).raw;

> @@ -149,6 +312,20 @@ int viridian_synic_rdmsr(const struct vcpu *v, uint32_t 
> idx, uint64_t *val)
>  
>  int viridian_synic_vcpu_init(const struct vcpu *v)
>  {

FTR while I'm in favor of adding const wherever it is possible
and makes sense, I consider it quite odd for an init function
to take a pointer to const. Perhaps the deinit one would also
fall into that category.

> @@ -1328,9 +1340,13 @@ int vlapic_has_pending_irq(struct vcpu *v)
>           (irr & 0xf0) <= (isr & 0xf0) )
>      {
>          viridian_apic_assist_clear(v);
> -        return -1;
> +        irr = -1;
>      }
>  
> +out:

The label still lacks proper indentation. With at least this fixed (which
is fine to be done while committing if this is the only piece to change)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



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