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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: 23 September 2014 10:01
> To: Paul Durrant
> Cc: Ian Jackson; xen-devel@xxxxxxxxxxxxx; Keir (Xen.org); Stefano Stabellini;
> Dave Scott
> Subject: Re: [PATCH v11 1/2] x86/viridian: Re-purpose the HVM parameter to
> be a feature mask
> 
> 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.

Ok, I guess I'll hardcode then. Where though, directly in libxl.h? Or would you 
prefer an enumeration member in the idl that's guaranteed to be the highest 
value member?

> 
> >  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.
> 

Yes, that's right.

> 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.
> 

But that's basically what I've done. The problem is that an addition was made 
to the legacy set in Xen 4.4 and the only way to optionally turn this off again 
is to add a negated feature. It's ugly, but it's the only way I can see to add 
control and stop MSRs from disappearing on migrate.

  Paul

> Ian.

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