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 09:35
> To: Ian Jackson
> Cc: Paul Durrant; 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 Mon, 2014-09-22 at 17:48 +0100, Ian Jackson wrote:
> > Paul Durrant writes ("[PATCH v11 1/2] x86/viridian: Re-purpose the HVM
> parameter to be a feature mask"):
> > > The following commits introduced the time reference counter MSR and
> > > TSC/APIC frequency MSRs into the viridian feature set respectively:
> >
> > The libxl and libxc parts of this, and the corresponding docs, look
> > good to me, apart from one part that I would like Ian C to comment on
> > - see below.
> >
> > If you had separated that out into a patch of its own, I would have
> > given a tools ack to this one :-).
> 
> AFAICT only the max_val is actually used, min_val I suppose is just for
> completeness.
> 

Yes.

> 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? Man and max values for 
enumerations seem like useful things to have. 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.

> > +/* 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.

  Paul

> Ian.

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