Re: [Xen-devel] [PATCH v16 02/23] x86/VPMU: Don't globally disable VPMU if initialization fails.

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 12/18/2014 09:10 AM, Jan Beulich wrote:
> > > > On 17.12.14 at 16:38, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > The failure to initialize VPMU may be temporary so we shouldn'd disable VMPU
> > forever.
> Reported-by: Jan Beulich <jbeulich@xxxxxxxx>
> (or Suggested-by if you like that better)
>
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > ---
> >   xen/arch/x86/hvm/vpmu.c | 15 ++++++++-------
> >   1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> > index 1df74c2..b43ea80 100644
> > --- a/xen/arch/x86/hvm/vpmu.c
> > +++ b/xen/arch/x86/hvm/vpmu.c
> > @@ -218,6 +218,7 @@ void vpmu_initialise(struct vcpu *v)
> >   {
> >       struct vpmu_struct *vpmu = vcpu_vpmu(v);
> >       uint8_t vendor = current_cpu_data.x86_vendor;
> > +    int ret;
> >   
> >       if ( is_pvh_vcpu(v) )
> >           return;
> > @@ -230,21 +231,21 @@ void vpmu_initialise(struct vcpu *v)
> >       switch ( vendor )
> >       {
> >       case X86_VENDOR_AMD:
> > -        if ( svm_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> > -            opt_vpmu_enabled = 0;
> > +        ret = svm_vpmu_initialise(v, opt_vpmu_enabled);
> >           break;
> >   
> >       case X86_VENDOR_INTEL:
> > -        if ( vmx_vpmu_initialise(v, opt_vpmu_enabled) != 0 )
> > -            opt_vpmu_enabled = 0;
> > +        ret = vmx_vpmu_initialise(v, opt_vpmu_enabled);
> >           break;
> >   
> >       default:
> > -        printk("VPMU: Initialization failed. "
> > -               "Unknown CPU vendor %d\n", vendor);
> > -        opt_vpmu_enabled = 0;
> > +        ret = -EINVAL;
> > +        printk(XENLOG_G_WARNING "VPMU: Unknown CPU vendor %d\n", vendor);
> >           break;
> >       }
> > +
> > +    if ( ret )
> > +        printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v);
> Please avoid issuing two messages for the same problem. And the
> one in the default case probably doesn't make sense to be issued
> more than once (and then perhaps at boot time, if that doesn't
> happen already).
I see not doing this for default case but arch-specific inits may (at 
least in principle) fail for some VCPUs and not for others so I think I 
should keep warnings.
What I perhaps should do for default case (and in fact I do this in 
patch 14 where I moved some of this stuff into __initcalls) is to 
disable VPMU globally.
As for printing only once --- I often wondered whether we should have 
something similar to Linux' WARN_ONCE().
-boris


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