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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 14 March 2019 14:12
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; 
> Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v6 09/11] viridian: add implementation of synthetic 
> interrupt MSRs
> 
> >>> 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;
> 

Personally I find it a bit ugly, and I don't imagine having the stack variable 
will make much difference to the generated code.

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

Yes, it does look a little odd but because the viridian_vcpu is no longer 
inline the vcpu can indeed be const.

> 
> > @@ -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>
> 

Ok, thanks.

  Paul

> Jan
> 


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