> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 14 March 2019 14:12
> To: Paul Durrant <[email protected]>
> Cc: Julien Grall <[email protected]>; Andrew Cooper
> <[email protected]>; Roger Pau Monne
> <[email protected]>; Wei Liu <[email protected]>; George Dunlap
> <[email protected]>; Ian
> Jackson <[email protected]>; Stefano Stabellini
> <[email protected]>; xen-devel <xen-
> [email protected]>; Konrad Rzeszutek Wilk <[email protected]>;
> Tim (Xen.org)
> <[email protected]>
> Subject: Re: [PATCH v6 09/11] viridian: add implementation of synthetic
> interrupt MSRs
>
> >>> On 14.03.19 at 12:25, <[email protected]> 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 <[email protected]>
>
Ok, thanks.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
