Re: [Xen-devel] [PATCH v3 8/9] viridian: add implementation of synthetic timers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.03.19 at 12:23, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 25 February 2019 14:54
>> 
>> >>> On 31.01.19 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
>> > @@ -118,14 +119,237 @@ static int64_t time_ref_count(struct domain *d)
>> >      return raw_trc_val(d) + trc->off;
>> >  }
>> >
>> > +static int64_t time_now(struct domain *d)
>> 
>> Why would this return a signed value? And can't the function
>> parameter be const?
> 
> The function parameter can be const, but I think the result needs to be 
> signed for the missed ticks logic in start_timer() to work correctly.

If something requires signed arithmetic, then this should be enforced
there, not by the return type of an otherwise sufficiently generic
function. Then again NOW() also produces a signed value ...

>> > +{
>> > +    const struct viridian_page *rt = &d->arch.hvm.viridian->reference_tsc;
>> > +    HV_REFERENCE_TSC_PAGE *p = rt->ptr;
>> > +    uint32_t start, end;
>> > +    __int128_t tsc;
>> > +    __int128_t scale;
>> 
>> I don't think you need both of them be 128 bits wide. I also don't
>> see why either would want to be of a signed type.
> 
> The spec says (as in the comment below):
> 
> "The partition reference time is computed by the following formula:
> 
> ReferenceTime = ((VirtualTsc * TscScale) >> 64) + TscOffset
> 
> The multiplication is a 64 bit multiplication, which results in a 128 bit 
> number which is then shifted 64 times to the right to obtain the high 64 
> bits.TscScale"

Well, yes, you want a 128-bit result. But for that you don't need to
multiply 128-bit quantities. See e.g. our own scale_delta() or
hvm_scale_tsc().

>> > +    case HV_X64_MSR_STIMER0_CONFIG:
>> > +    case HV_X64_MSR_STIMER1_CONFIG:
>> > +    case HV_X64_MSR_STIMER2_CONFIG:
>> > +    case HV_X64_MSR_STIMER3_CONFIG:
>> > +    {
>> > +        unsigned int stimerx = (idx - HV_X64_MSR_STIMER0_CONFIG) / 2;
>> > +        struct viridian_stimer *vs = 
>> > &v->arch.hvm.viridian->stimer[stimerx];
>> > +
>> > +        if ( !(viridian_feature_mask(d) & HVMPV_stimer) )
>> > +            return X86EMUL_EXCEPTION;
>> > +
>> > +        stop_stimer(vs);
>> > +
>> > +        vs->config.raw = val;
>> > +
>> > +        if ( !vs->config.fields.sintx )
>> > +            vs->config.fields.enabled = 0;
>> > +
>> > +        if ( vs->config.fields.enabled )
>> > +            start_stimer(vs);
>> > +
>> > +        break;
>> > +    }
>> > +    case HV_X64_MSR_STIMER0_COUNT:
>> 
>> Missing blank line again (and also further down here as well as in the
>> rdmsr code).
>> 
> 
> Ok. TBH I've always thought the normal style was to omit the blank line if 
> the case statement has braces.

Not sure about this specific sub-case.

>> > @@ -201,6 +476,32 @@ int viridian_time_rdmsr(const struct vcpu *v, 
>> > uint32_t idx, uint64_t *val)
>> >          break;
>> >      }
>> >
>> > +    case HV_X64_MSR_STIMER0_CONFIG:
>> > +    case HV_X64_MSR_STIMER1_CONFIG:
>> > +    case HV_X64_MSR_STIMER2_CONFIG:
>> > +    case HV_X64_MSR_STIMER3_CONFIG:
>> > +    {
>> > +        unsigned int stimerx = (idx - HV_X64_MSR_STIMER0_CONFIG) / 2;
>> > +
>> > +        if ( !(viridian_feature_mask(d) & HVMPV_stimer) )
>> > +            return X86EMUL_EXCEPTION;
>> > +
>> > +        *val = v->arch.hvm.viridian->stimer[stimerx].config.raw;
>> 
>> While more noticeable here and ...
>> 
>> > +        break;
>> > +    }
>> > +    case HV_X64_MSR_STIMER0_COUNT:
>> > +    case HV_X64_MSR_STIMER1_COUNT:
>> > +    case HV_X64_MSR_STIMER2_COUNT:
>> > +    case HV_X64_MSR_STIMER3_COUNT:
>> > +    {
>> > +        unsigned int stimerx = (idx - HV_X64_MSR_STIMER0_COUNT) / 2;
>> > +
>> > +        if ( !(viridian_feature_mask(d) & HVMPV_stimer) )
>> > +            return X86EMUL_EXCEPTION;
>> > +
>> > +        *val = v->arch.hvm.viridian->stimer[stimerx].count;
>> 
>> ... here, array_access_nospec() are probably needed not just here,
>> but also in the wrmsr logic.
> 
> Really? stimerx is calculated based on hitting the case statement in the 
> first place.

And any of the branches of the switch() can be (mis)speculated into.

>> > --- a/xen/include/asm-x86/hvm/viridian.h
>> > +++ b/xen/include/asm-x86/hvm/viridian.h
>> > @@ -40,6 +40,33 @@ union viridian_sint_msr
>> >      } fields;
>> >  };
>> >
>> > +union viridian_stimer_config_msr
>> > +{
>> > +    uint64_t raw;
>> > +    struct
>> > +    {
>> > +        uint64_t enabled:1;
>> > +        uint64_t periodic:1;
>> > +        uint64_t lazy:1;
>> > +        uint64_t auto_enable:1;
>> > +        uint64_t vector:8;
>> > +        uint64_t direct_mode:1;
>> > +        uint64_t reserved_zero1:3;
>> > +        uint64_t sintx:4;
>> > +        uint64_t reserved_zero2:44;
>> > +    } fields;
>> > +};
>> > +
>> > +struct viridian_stimer {
>> > +    struct vcpu *v;
>> 
>> Isn't a full 8-byte pointer a little too much overhead here? You could
>> instead store the timer index ...
> 
> I think I need it in stimer_expire() which can be called in any vcpu context 
> IIUC.
> 
>> 
>> > +    struct timer timer;
>> > +    union viridian_stimer_config_msr config;
>> > +    uint64_t count;
>> > +    int64_t expiration;
>> > +    s_time_t timeout;
>> > +    bool started;
>> 
>> ... in a field using the 7-byte padding here, and use container_of()
>> to get at the outer structure.
> 
> That would get me as far as viridian_vcpu, but there's no pointer to struct 
> vcpu in there, and I need one to call vcpu_kick().

Oh, I see.

Jan


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