Re: [Xen-devel] [PATCH 3/8] ACPI: processor: add __acpi_processor_[un]register_driver helpers.

[Konrad Rzeszutek Wilk <konrad@darnok.org>]

On Tue, Dec 20, 2011 at 10:29:12AM +0800, Tian, Kevin wrote:
> > From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> > Sent: Monday, December 19, 2011 10:26 PM
> > 
> > On Mon, Dec 19, 2011 at 01:48:01PM +0800, Tian, Kevin wrote:
> > > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > > > Sent: Saturday, December 17, 2011 6:04 AM
> > > >
> > > > On Wed, Nov 30, 2011 at 12:20:59PM -0500, Konrad Rzeszutek Wilk wrote:
> > > > > From: Tang Liang <liang.tang@oracle.com>
> > > > >
> > > > > This patch implement __acpi_processor_[un]register_driver helper,
> > > > > so we can registry override processor driver function. Specifically
> > > > > the Xen processor driver.
> > > >
> > > > Liang,
> > > >
> > > > Is the reason we are doing this b/c we need to call acpi_bus_register_driver
> > > > and inhibit the registration of 'acpi_processor_driver' ?
> > > >
> > > > And the reason we don't want 'acpi_processor_driver' to run is b/c of what?
> > > > If the cpuidle is disabled what is the harm of running the
> > > > 'acpi_processor_driver'
> > > > driver?
> > >
> > > IIRC, the reason why we want a Xen specific driver is because current driver
> > > is integrated with OS logic too tightly, e.g. the various checks on VCPU related
> > > structures. Long time ago the 1st version in Xen was to use same driver, by
> > > adding various tweaking lines inside which makes it completely messed. Then
> > > later it's found that it's clearer to create a Xen specific wrapping driver, by
> > > invoking some exported functions from existing one.
> > 
> > What I am asking is does it matter "if the current driver is integrated
> > with OS logic to tighly" - as it is not running anyhow (b/c cpuidle is
> > disabled).
> > 
> > And if Xen specific driver can run along-side the generic one - since
> > the generic one is not doing any work (b/c cpuidle is disabled).
> > 
> > That is what I am trying to figure out - since this patch purpose is to
> > thwart the generic driver from running and allowing the xen one to run.
> 
> It's a separate issue from cpuidle case. Here we're talking about acpi processor
> driver, not the acpi cpuidle driver. ACPI processor driver is responsible for 
> discovering ACPI processor projects, and then enumerate various methods 
> such as _PPC, _CST, etc. under those objects. So far this driver depends on 
> VCPU presence in various places, which causes trouble when dom0 is configured
> with less VCPU number than physical present one.
> 
> One example you can see from acpi_processor_add:
> 
>         result = acpi_processor_get_info(device); // call acpi_get_cpuid
>         if (result) {
>                 /* Processor is physically not present */
>                 return 0;
>         }
> 
> #ifdef CONFIG_SMP
>         if (pr->id >= setup_max_cpus && pr->id != 0)
>                 return 0;
> #endif 
> 
>         BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
> 
> The binding between ACPI processor objects and VCPU presence would only parse
> partial objects to Xen. And there're various places within driver validating pr->id
> making it messy to workaround for Xen within same driver.
> 
> That's the major reason for coming up a Xen specific driver, which always parses
> all present objects regardless of VCPU presence. :-)

OK. Lets put the # VCPU != PCPU aside. Say dom0 will boot with all
CPUs and then later on the admin starts unplugging them.

With that in mind, could we use a slim driver (like the one in: "ACPI:
 add processor driver for Xen virtual CPUs.") that would just
take the _Pxx, Cxx data and shove them to the hypervisor (handwaving
aside the checking for quirks and such). And lets assume that we use the
unmodified acpi_processor_[add|remove|notify] code that is present in
processor_driver.c.

Oh, and the processor-driver.c did try to load (with the understanding
that it would load, but won't do much since cpuidle is turned off).

Would that still get the Pxx, Cxx data to the hypervisor?