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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-09-23 at 09:45 +0100, Paul Durrant wrote:
> > Can we not just either include an explicit MAX case in the enum itself
> > or provide a libxl function to allocate a suitably sized bitmap like we
> > do with libxl_cpu_bitmap_alloc (which lets us keep all this internal so
> > we don't need to think about the API implications too hard).
> > 
> 
> Is there a definite problem with what I've done?

It's adding a new public API which would need more careful
consideration/review.

>  Man and max values for enumerations seem like useful things to have.

This case is a bit different because they happen to represent bitmaps.
but in general we want users to be using the enum names/values and the
to/from string conversion routines, exposing the incidental min/max may
encourage them to try and do strange things.

>  I could put an upper limit on the enlightenments but using this mechanism 
> seems neater.
> 
> > > +libxl_viridian_enlightenment = Enumeration("viridian_enlightenment",
> > > [
> > > +    (0, "base"),
> > > +    (1, "freq"),
> > > +    ])
> > 
> > Does this need to be kept in sync with the hvm.h values?.
> > 
> 
> It doesn't need to be in sync. Obviously, if another enlightenment is
> added then the chances are that something would have to be added here,
> but there's no necessarily a 1:1 correspondence.

1:1 correspondence was what I was trying to ask about (like the
SHUTDOWN_* values, which must match).

> 
> > > +/* 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)
> > 
> > Given that these have no backwards compatiblity reqs can't we avoid the
> > negative feature at the hypercall and DTRT in libxl?
> > 
> 
> Not really. There are implications on migrate. A value of 1 in the HVM
> param needs to represent the set of enlightenments in Xen 4.4. This
> has already been discussed.

So the answer is that this is transparently migrated via the hvmparam
and not taken care of by the toolstack, ok then.

I'd have though declaring 0x1 to be the "legacy set" and starting the
new mechanism at 0x2 onwards would have been neater in the long run, but
if the h/visor chaps are happy then fine.

Ian.


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