Re: [Xen-devel] [PATCH v2] new config option vtsc_khz_tolerance to avoid TSC emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.18 at 12:35, <olaf@xxxxxxxxx> wrote:

One thing I'm missing in the description (or the added documentation)
is a discussion of the conditions under which it is safe to make use of
the new setting.

> @@ -954,11 +955,21 @@ long arch_do_domctl(
>              tsc_set_info(d, domctl->u.tsc_info.tsc_mode,
>                           domctl->u.tsc_info.elapsed_nsec,
>                           domctl->u.tsc_info.gtsc_khz,
> +                         domctl->u.tsc_info.vtsc_khz_tolerance,
>                           domctl->u.tsc_info.incarnation);
>              domain_unpause(d);
>          }
>          break;
>  
> +    case XEN_DOMCTL_set_vtsc_khz_tolerance:
> +        if ( d == currd )
> +            ret = -EINVAL;

Why? There's e.g. no domain_pause() involved here. And without
that there's no difference between changing the value behind the
back of a foreign domain, of for oneself. Granted Dom0 isn't likely
to want to do that, but such checks should have a reason.

> +        else
> +        {
> +            d->arch.vtsc_khz_tolerance = 
> domctl->u.tsc_info.vtsc_khz_tolerance;
> +        }

Stray braces (the more that the if() side doesn't have them).

Also throughout the patch I wonder if it wasn't more natural to
put the unit last in the parameter / field names.

> @@ -2122,7 +2123,8 @@ void tsc_get_info(struct domain *d, uint32_t *tsc_mode,
>   */
>  void tsc_set_info(struct domain *d,
>                    uint32_t tsc_mode, uint64_t elapsed_nsec,
> -                  uint32_t gtsc_khz, uint32_t incarnation)
> +                  uint32_t gtsc_khz, uint16_t vtsc_khz_tolerance,
> +                  uint32_t incarnation)

For the sake of consistency with the other types here I'm not
going to demand to replace uint16_t here, but it's really not
necessary to use that type here (other than on the read path).

> @@ -2147,8 +2152,24 @@ void tsc_set_info(struct domain *d,
>           * When a guest is created, gtsc_khz is passed in as zero, making
>           * d->arch.tsc_khz == cpu_khz. Thus no need to check incarnation.
>           */
> +        diff_tolerated = d->arch.tsc_khz == cpu_khz;
> +
> +        if ( tsc_mode == TSC_MODE_DEFAULT && gtsc_khz ) {

Style.

> +            uint32_t khz_diff;
> +
> +            khz_diff = cpu_khz > gtsc_khz ?
> +                       cpu_khz - gtsc_khz : gtsc_khz - cpu_khz;
> +            if (vtsc_khz_tolerance)

Again.

> +                diff_tolerated = khz_diff <= vtsc_khz_tolerance;
> +
> +            printk(XENLOG_WARNING "%s: d%u: host has %lu kHz,"
> +                   " domU expects %u kHz,"
> +                   " difference of %u is %s tolerance of %u\n",
> +                   __func__, d->domain_id, cpu_khz, gtsc_khz, khz_diff,
> +                   diff_tolerated ? "within" : "outside", 
> vtsc_khz_tolerance);

Leftover debugging message?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -697,12 +697,14 @@ struct xen_domctl_disable_migrate {
>  
>  /* XEN_DOMCTL_gettscinfo */
>  /* XEN_DOMCTL_settscinfo */
> +/* XEN_DOMCTL_set_vtsc_khz_tolerance */
>  struct xen_domctl_tsc_info {
>      /* IN/OUT */
>      uint32_t tsc_mode;
>      uint32_t gtsc_khz;
>      uint32_t incarnation;
> -    uint32_t pad;
> +    uint16_t vtsc_khz_tolerance;
> +    uint16_t pad;

Generally with the prior pad field not being checked anywhere this
isn't a valid extension. However, this being a domctl (and the
interface version suitably bumped already) plus looking at how the
libxc code works, I think you can get away doing it like this.

Jan


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