Re: [Xen-devel] [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 04 August 2014 14:31
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxx
> Cc: Keir (Xen.org); Ian Campbell; Stefano Stabellini; Ian Jackson; Jan Beulich
> Subject: Re: [Xen-devel] [PATCH v2 1/3] x86/viridian: Re-purpose the HVM
> parameter to be a feature mask
> 
> On 04/08/14 14:12, Paul Durrant wrote:
> > The following commits introduced the time reference counter MSR and
> > TSC/APIC frequency MSRs into the viridian feature set respectively:
> >
> > e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b
> > 84657efd9116f40924aa13c9d5a349e007da716f
> >
> > The time reference counter MSR feature was then reverted by commit
> >
> > 1cd4fab14ce25859efa4a2af13475e6650a5506c
> >
> > because a flaw in the implementation meant the counter was reset on
> > migration.
> >
> > All of these changes were made without any addtional options being
> > added to the VM configuration, or any compatibility checks being made
> > in the domain save/restore code. Hence setting the single boolean
> > 'viridian' option in the VM configuration yields a different set of
> > features depending on which version of Xen the VM is started on, and the
> > feature set can change across migration (so new MSRs can magically
> appear).
> >
> > This patch grandfathers in the current viridian features set and calls them
> > the 'base' plus 'freq' feature set. HVM_PARAM_VIRIDIAN is re-purposed as
> > a feature mask. It has only ever been legally set to 0 or 1, so the presence
> > of the base set and freq set are indicated by setting bit 0. The 'freq' set
> > can then be turned off by setting bit 1, thus restoring the pre-Xen-4.4
> > 'base' set. Newly implemented viridian features can be optionally enabled
> > in future by setting further bits.
> >
> > The viridian option in xl.cfg(5) has also been changed to a string list so
> > that the sets can be individually sepcified. For compatibility, if the
> > option is specified as a boolean, then a true (1) value will be translated
> > to a string list containing "base" and "freq".
> >
> > This patch also alters the allowed write accesses to
> HVM_PARAM_VIRIDIAN.
> > Currently there is nothing to stop the guest writing this value (which,
> > while harmless to anything else, should not happen) and nothing to
> > stop a toolstack from setting the value back to zero whilst the guest is
> > running, causing CPUID leaves to disappear and MSR accesses to start
> > causing GPFs in the guest. Both of these possibilities are now disallowed:
> > Once the parameter is set to a non-zero value it may not be cleared, and
> > guests no longer have any write access.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Keir Fraser <keir@xxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  docs/man/xl.cfg.pod.5           |   53 ++++++++++++++++++++++++++++++--
> -------
> >  tools/libxc/xc_domain_restore.c |    4 +--
> >  tools/libxl/libxl.h             |    6 +++++
> >  tools/libxl/libxl_create.c      |    1 -
> >  tools/libxl/libxl_dom.c         |   46 ++++++++++++++++++++++++++-------
> >  tools/libxl/libxl_types.idl     |    2 +-
> >  tools/libxl/xl_cmdimpl.c        |   24 +++++++++++++++++-
> >  tools/libxl/xl_sxp.c            |    8 ++++--
> >  xen/arch/x86/hvm/hvm.c          |   18 +++++++++++--
> >  xen/arch/x86/hvm/viridian.c     |   21 ++++++++++++++--
> >  xen/include/asm-x86/hvm/hvm.h   |    7 ++++--
> >  xen/include/public/hvm/params.h |   33 +++++++++++++++++++++++-
> >  12 files changed, 188 insertions(+), 35 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index ff9ea77..a05e831 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1084,18 +1084,47 @@ Windows
> L<http://wiki.xen.org/wiki/XenWindowsGplPv>.
> >  Setting B<xen_platform_pci=0> with the default device_model "qemu-
> xen"
> >  requires at least QEMU 1.6.
> >
> > -=item B<viridian=BOOLEAN>
> > -
> > -Turns on or off the exposure of MicroSoft Hyper-V (AKA viridian)
> > -compatible enlightenments to the guest.  These can improve performance
> > -of Microsoft Windows guests from Windows Vista and Windows 2008
> > -onwards and setting this option for such guests is strongly
> > -recommended. This option should be harmless for other versions of
> > -Windows (although it will not give any benefit) and the majority of
> > -other non-Windows OSes. However it is known to be incompatible with
> > -some other Operating Systems and in some circumstance can prevent
> > -Xen's own paravirtualisation interfaces for HVM guests from being
> > -used.
> > +=item B<viridian=[ "SET", "SET", ...]>
> > +
> > +The sets of Microsoft Hyper-V (AKA viridian) compatible enlightenments
> > +exposed to the guest. The following sets of enlightenments may be
> > +specified:
> > +
> > +=over 4
> > +
> > +=item B<base>
> > +
> > +This set incorporates the Hypercall MSRs, Virtual processor index MSR,
> > +and APIC access MSRs. This set is a pre-requisite for all other sets.
> > +If it is not specified then all enlightenments will be disabled.
> > +
> > +These enlightenments can improve performance of Windows Vista and
> Windows
> > +Server 2008 onwards and setting this option for such guests is strongly
> > +recommended.
> > +
> > +=item B<freq>
> > +
> > +This set incorporates the TSC and APIC frequency MSRs.
> > +
> > +This enlightenment can improve performance of Windows 7 and Windows
> > +Server 2008 R2 onwards.
> > +
> > +=back
> > +
> > +See the latest version of Microsoft's Hypervisor Top-Level Functional
> > +Specification for more details.
> > +
> > +The enlightenments should be harmless for other versions of Windows
> > +(although they will not give any benefit) and the majority of other
> > +non-Windows OSes.
> > +However it is known that they are incompatible with some other
> Operating
> > +Systems and in some circumstance can prevent Xen's own
> paravirtualisation
> > +interfaces for HVM guests from being used.
> > +
> > +Specifying the viridian option as a boolean is deprecated. However, for
> > +backwards compatibility, if it is specified as a boolean a value of
> > +true (1) will cause both the 'base' and 'freq' sets to be exposed to
> > +the guest, and a value of false (0) will disable all enlightenments.
> >
> >  =back
> >
> > diff --git a/tools/libxc/xc_domain_restore.c
> b/tools/libxc/xc_domain_restore.c
> > index e73e0a2..5e7e62d 100644
> > --- a/tools/libxc/xc_domain_restore.c
> > +++ b/tools/libxc/xc_domain_restore.c
> > @@ -922,7 +922,7 @@ static int pagebuf_get_one(xc_interface *xch,
> struct restore_ctx *ctx,
> >          if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> >               RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> >          {
> > -            PERROR("error read the viridian flag");
> > +            PERROR("error read the viridian flags");
> >              return -1;
> >          }
> >          return pagebuf_get_one(xch, ctx, buf, fd, dom);
> > @@ -1733,7 +1733,7 @@ int xc_domain_restore(xc_interface *xch, int
> io_fd, uint32_t dom,
> >      }
> >
> >      if (pagebuf.viridian != 0)
> > -        xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> > +        xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN,
> pagebuf.viridian);
> >
> >      /*
> >       * If we are migrating in from a host that does not support
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 5ae6532..b985f49 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -128,6 +128,12 @@
> >  #define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
> >
> >  /*
> > + * The libxl_domain_build_info u.hvm.viridian field is a string list
> > + */
> > +#define LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_STRING_LIST 1
> > +
> > +
> > +/*
> >   * libxl ABI compatibility
> >   *
> >   * The only guarantee which libxl makes regarding ABI compatibility
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 0686f96..ee19fc8 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -273,7 +273,6 @@ int libxl__domain_build_info_setdefault(libxl__gc
> *gc,
> >          libxl_defbool_setdefault(&b_info->u.hvm.acpi_s3,            true);
> >          libxl_defbool_setdefault(&b_info->u.hvm.acpi_s4,            true);
> >          libxl_defbool_setdefault(&b_info->u.hvm.nx,                 true);
> > -        libxl_defbool_setdefault(&b_info->u.hvm.viridian,           false);
> >          libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
> >          libxl_defbool_setdefault(&b_info->u.hvm.vpt_align,          true);
> >          libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm,         false);
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 83eb29a..a15c185 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -209,21 +209,49 @@ static unsigned long timer_mode(const
> libxl_domain_build_info *info)
> >      return ((unsigned long)mode);
> >  }
> >
> > -static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
> > +#if defined(__i386__) || defined(__x86_64__)
> > +static void hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
> > +                                      libxl_domain_build_info *const info)
> > +{
> > +    uint64_t feature_mask = HVMPV_no_freq;
> > +
> > +    char **p = info->u.hvm.viridian;
> > +    while (*p) {
> > +        LOG(DETAIL, "+%s", *p);
> > +        if (strcmp(*p, "base") == 0)
> > +            feature_mask |= HVMPV_base_freq;
> > +        else if (strcmp(*p, "freq") == 0)
> > +            feature_mask &= ~HVMPV_no_freq;
> > +        p++;
> > +    }
> > +
> > +    if (xc_hvm_param_set(CTX->xch,
> > +                         domid,
> > +                         HVM_PARAM_VIRIDIAN,
> > +                         feature_mask) != 0)
> > +        LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_WARNING,
> > +                         "Couldn't set viridian feature mask (0x%lx)",
> 
> PRIx64, or this will break 32bit builds.
> 

Ok. Done.

> > +                         feature_mask);
> > +}
> > +#endif
> > +
> > +static void hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
> >                                  libxl_domain_build_info *const info)
> 
> const libxl_domain_build_info *info ?
> 

I didn't actually change this - it's code movement.

> >  {
> > -    xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED,
> > +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_PAE_ENABLED,
> >                      libxl_defbool_val(info->u.hvm.pae));
> >  #if defined(__i386__) || defined(__x86_64__)
> > -    xc_hvm_param_set(handle, domid, HVM_PARAM_VIRIDIAN,
> > -                    libxl_defbool_val(info->u.hvm.viridian));
> > -    xc_hvm_param_set(handle, domid, HVM_PARAM_HPET_ENABLED,
> > +    if (libxl_string_list_length(&info->u.hvm.viridian) != 0)
> > +        hvm_set_viridian_features(gc, domid, info);
> > +
> > +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_HPET_ENABLED,
> >                      libxl_defbool_val(info->u.hvm.hpet));
> >  #endif
> > -    xc_hvm_param_set(handle, domid, HVM_PARAM_TIMER_MODE,
> timer_mode(info));
> > -    xc_hvm_param_set(handle, domid, HVM_PARAM_VPT_ALIGN,
> > +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_TIMER_MODE,
> > +                     timer_mode(info));
> > +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_VPT_ALIGN,
> >                      libxl_defbool_val(info->u.hvm.vpt_align));
> > -    xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
> > +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_NESTEDHVM,
> >                      libxl_defbool_val(info->u.hvm.nested_hvm));
> >  }
> >
> > @@ -306,7 +334,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >      state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid,
> state->console_domid);
> >
> >      if (info->type == LIBXL_DOMAIN_TYPE_HVM)
> > -        hvm_set_conf_params(ctx->xch, domid, info);
> > +        hvm_set_conf_params(gc, domid, info);
> >
> >      rc = libxl__arch_domain_create(gc, d_config, domid);
> >
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index a412f9c..6c5d7e0 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -363,7 +363,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
> >                                         ("acpi_s3",          libxl_defbool),
> >                                         ("acpi_s4",          libxl_defbool),
> >                                         ("nx",               libxl_defbool),
> > -                                       ("viridian",         libxl_defbool),
> > +                                       ("viridian",         
> > libxl_string_list),
> >                                         ("timeoffset",       string),
> >                                         ("hpet",             libxl_defbool),
> >                                         ("vpt_align",        libxl_defbool),
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 01bce2f..960f626 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -951,10 +951,32 @@ static void parse_config_data(const char
> *config_source,
> >          xlu_cfg_get_defbool(config, "acpi_s3", &b_info->u.hvm.acpi_s3, 0);
> >          xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
> >          xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
> > -        xlu_cfg_get_defbool(config, "viridian", &b_info->u.hvm.viridian, 
> > 0);
> >          xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
> >          xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align,
> 0);
> >
> > +        switch (xlu_cfg_get_list_as_string_list(config, "viridian",
> > +                                                &b_info->u.hvm.viridian, 
> > 1))
> > +        {
> > +        case 0: break; /* Success */
> > +        case ESRCH: break; /* Option not present */
> > +        case EINVAL: {
> > +            libxl_defbool b;
> > +
> > +            xlu_cfg_get_defbool(config, "viridian", &b, 1);
> > +            if (libxl_defbool_val(b)) {
> > +                fprintf(stderr, "WARNING: Specifying \"viridian\""
> > +                        " as a boolean is deprecated. "
> > +                        "Please use a list of features.\n");
> > +                split_string_into_string_list("base freq", " \n",
> > +                                              &b_info->u.hvm.viridian);
> > +            }
> > +            break;
> > +        }
> > +        default:
> > +            fprintf(stderr,"xl: Unable to parse bootloader_args.\n");
> > +            exit(-ERROR_FAIL);
> > +        }
> > +
> >          if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
> >              const char *s = libxl_timer_mode_to_string(l);
> >              fprintf(stderr, "WARNING: specifying \"timer_mode\" as an 
> > integer
> is deprecated. "
> > diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
> > index 48eb608..347526f 100644
> > --- a/tools/libxl/xl_sxp.c
> > +++ b/tools/libxl/xl_sxp.c
> > @@ -97,8 +97,12 @@ void printf_info_sexp(int domid, libxl_domain_config
> *d_config)
> >          printf("\t\t\t(acpi %s)\n",
> >                 libxl_defbool_to_string(b_info->u.hvm.acpi));
> >          printf("\t\t\t(nx %s)\n", 
> > libxl_defbool_to_string(b_info->u.hvm.nx));
> > -        printf("\t\t\t(viridian %s)\n",
> > -               libxl_defbool_to_string(b_info->u.hvm.viridian));
> > +        if (b_info->u.hvm.viridian) {
> > +            printf("\t\t\t(viridian");
> > +            for (i=0; b_info->u.hvm.viridian[i]; i++)
> > +                printf(" %s", b_info->u.hvm.viridian[i]);
> > +            printf(")\n");
> > +        }
> >          printf("\t\t\t(hpet %s)\n",
> >                 libxl_defbool_to_string(b_info->u.hvm.hpet));
> >          printf("\t\t\t(vpt_align %s)\n",
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index fba13e0..04d43d0 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> >                      rc = -EINVAL;
> >                  break;
> >              case HVM_PARAM_VIRIDIAN:
> > -                if ( a.value > 1 )
> > -                    rc = -EINVAL;
> > +                /* This should only ever be set once by the tools and read 
> > by the
> guest. */
> > +                rc = -EPERM;
> > +                if ( curr_d == d )
> > +                    break;
> > +
> > +                rc = -EPERM;
> > +                if ( d->arch.hvm_domain.params[a.index] &&
> > +                     a.value != d->arch.hvm_domain.params[a.index] )
> > +                    break;
> 
> Setting it twice should be an error, even if it is set to the same value
> again.  Also, EEXIST would be a better error.
> 

I've opted for EEXIST but, as Jan requested, setting to the same value is 
allowed.

> > +
> > +                rc = -EINVAL;
> > +                if ( a.value & ~HVMPV_feature_mask ||
> > +                     !(a.value & HVMPV_base_freq) )
> > +                    break;
> > +
> > +                rc = 0;
> 
> To save bloating the code here if/when new combinations are introduced,
> would it be better to have a "rc = viridian_initialise(d, a.value);"
> which validates the featureset and sets it if necessary.  The above
> EEXISTs check could be included as well.

This statement won't grow with new features (since they are added to the mask), 
so probably not worth it.

> 
> >                  break;
> >              case HVM_PARAM_IDENT_PT:
> >                  /* Not reflexive, as we must domain_pause(). */
> > diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> > index 0fcbfd8..31c9656 100644
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -90,8 +90,9 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned
> int *eax,
> >          /* Which hypervisor MSRs are available to the guest */
> >          *eax = (CPUID3A_MSR_APIC_ACCESS |
> >                  CPUID3A_MSR_HYPERCALL   |
> > -                CPUID3A_MSR_VP_INDEX    |
> > -                CPUID3A_MSR_FREQ);
> > +                CPUID3A_MSR_VP_INDEX);
> > +        if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> > +            *eax |= CPUID3A_MSR_FREQ;
> >          break;
> >      case 4:
> >          /* Recommended hypercall usage. */
> > @@ -312,11 +313,17 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t
> *val)
> >          break;
> >
> >      case VIRIDIAN_MSR_TSC_FREQUENCY:
> > +        if ( viridian_feature_mask(d) & HVMPV_no_freq )
> > +            return 0;
> > +
> >          perfc_incr(mshv_rdmsr_tsc_frequency);
> >          *val = (uint64_t)d->arch.tsc_khz * 1000ull;
> >          break;
> >
> >      case VIRIDIAN_MSR_APIC_FREQUENCY:
> > +        if ( viridian_feature_mask(d) & HVMPV_no_freq )
> > +            return 0;
> > +
> >          perfc_incr(mshv_rdmsr_apic_frequency);
> >          *val = 1000000000ull / APIC_BUS_CYCLE_NS;
> >          break;
> > @@ -489,3 +496,13 @@ static int viridian_load_vcpu_ctxt(struct domain
> *d, hvm_domain_context_t *h)
> >
> >  HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU,
> viridian_save_vcpu_ctxt,
> >                            viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> > index 0ebd478..24e513b 100644
> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -344,8 +344,11 @@ static inline unsigned long
> hvm_get_shadow_gs_base(struct vcpu *v)
> >      return hvm_funcs.get_shadow_gs_base(v);
> >  }
> >
> > -#define is_viridian_domain(_d)                                             
> > \
> > - (is_hvm_domain(_d) && ((_d)-
> >arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
> > +#define viridian_feature_mask(d) \
> > +    ((d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
> > +
> > +#define is_viridian_domain(d) \
> > +    (is_hvm_domain(d) && (viridian_feature_mask(d) &
> HVMPV_base_freq))
> >
> >  void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
> >                                 uint32_t *eax, uint32_t *ebx,
> > diff --git a/xen/include/public/hvm/params.h
> b/xen/include/public/hvm/params.h
> > index 614ff5f..68d26fd 100644
> > --- a/xen/include/public/hvm/params.h
> > +++ b/xen/include/public/hvm/params.h
> > @@ -56,9 +56,40 @@
> >
> >  #if defined(__i386__) || defined(__x86_64__)
> >
> > -/* Expose Viridian interfaces to this HVM guest? */
> > +/*
> > + * Viridian enlightenments
> > + *
> > + * (See http://download.microsoft.com/download/A/B/4/AB43A34E-
> BDD0-4FA6-BDEF-
> 79EEF16E880B/Hypervisor%20Top%20Level%20Functional%20Specification%2
> 0v4.0.docx)
> > + *
> > + * To expose viridian enlightenments to the guest set this parameter
> > + * to the desired feature mask. The base feature set must be present
> > + * in any valid feature mask.
> > + */
> >  #define HVM_PARAM_VIRIDIAN     9
> >
> > +/* Base+Freq viridian feature sets:
> > + *
> > + * - Hypercall MSRs (HV_X64_MSR_GUEST_OS_ID and
> HV_X64_MSR_HYPERCALL)
> > + * - APIC access MSRs (HV_X64_MSR_EOI, HV_X64_MSR_ICR and
> HV_X64_MSR_TPR)
> > + * - Virtual Processor index MSR (HV_X64_MSR_VP_INDEX)
> > + * - Timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> > + *   HV_X64_MSR_APIC_FREQUENCY)
> > + */
> > +#define _HVMPV_base_freq 0
> > +#define HVMPV_base_freq  (1 << _HVMPV_base_freq)
> > +
> > +/* Feature set modifications */
> > +
> > +/* Disable timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> > + * HV_X64_MSR_APIC_FREQUENCY).
> > + * This modification restores the viridian feature set to the
> > + * original 'base' set exposed in releases prior to Xen 4.4.
> > + */
> > +#define _HVMPV_no_freq 1
> > +#define HVMPV_no_freq  (1 << _HVMPV_no_freq)
> > +
> > +#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)
> 
> Please could we avoid introducing new negated-sense booleans, especially
> when their predominant use is "if ( !$FOO_no_BAR )"
> 

As Jan points out, I have no choice if I am to maintain compatibility but still 
allow the frequency MSRs to be optional - as I think they should be.

  Paul

> ~Andrew
> 
> > +
> >  #endif
> >
> >  /*


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel