Re: [Xen-devel] [PATCH 1/2] Xen acpi pad implement

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 29, 2012 at 03:38:14AM +0000, Liu, Jinsong wrote:
>> It's doable to register a stub for xen. However, it's not preferred,
>> say, to make xen pad as module, considering the case 'rmmod
>> xen_acpi_pad' then 'insmod acpi_pad'? Under such case there is risk
>> of mwait #UD, if we want to remove mwait check as 'patch 2/2:
>> Revert-pad-config-check-in-xen_check_mwait.patch' did, advantages of
>> which is to enjoy deep Cx.     
> 
> You are missing my point. This is what I was thinking off:
> 
> 
> From 5f30320b8a1c21d60a2c22e98bcf013b7773938b Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date: Mon, 29 Oct 2012 08:38:22 -0400
> Subject: [PATCH] xen/acpi: Provide a stub function to register for
>  ACPI PAD module.
> 
> TODO: Give more details.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  drivers/xen/Kconfig             |    5 ++
>  drivers/xen/Makefile            |    3 +-
>  drivers/xen/xen-acpi-pad-stub.c |  128
>  +++++++++++++++++++++++++++++++++++++++ drivers/xen/xen-acpi.h      
>  |    2 + 4 files changed, 137 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/xen/xen-acpi-pad-stub.c
>  create mode 100644 drivers/xen/xen-acpi.h
> 
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index d4dffcd..9156a08 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -204,4 +204,9 @@ config XEN_MCE_LOG
>         Allow kernel fetching MCE error from Xen platform and
>         converting it into Linux mcelog format for mcelog tools
> 
> +config XEN_ACPI_PAD_STUB
> +     bool
> +     depends on XEN_DOM0 && X86_64 && ACPI
> +     default n
> +

This Kconfig is pointless, if CONFIG_XEN_ACPI_PAD_STUB = n, native pad would 
successfully registerred, and then mwait #UD (we would revert 
df88b2d96e36d9a9e325bfcd12eb45671cbbc937, right?). So xen stub logic should 
unconditionally built-in kernel.

>  endmenu
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 0e863703..d1895d9 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -9,9 +9,10 @@ nostackp := $(call cc-option, -fno-stack-protector)
>  CFLAGS_features.o                    := $(nostackp)
> 
>  dom0-$(CONFIG_PCI) += pci.o
> -dom0-$(CONFIG_USB_SUPPORT) += dbgp.o
> +dom0-$(CONFIG_USB_EHCI_HCD) += dbgp.o
>  dom0-$(CONFIG_ACPI) += acpi.o
>  dom0-$(CONFIG_X86) += pcpu.o
> +dom0-$(CONFIG_XEN_ACPI_PAD_STUB)     += xen-acpi-pad-stub.o
>  obj-$(CONFIG_XEN_DOM0)                       += $(dom0-y)
>  obj-$(CONFIG_BLOCK)                  += biomerge.o
>  obj-$(CONFIG_XEN_XENCOMM)            += xencomm.o
> diff --git a/drivers/xen/xen-acpi-pad-stub.c
> b/drivers/xen/xen-acpi-pad-stub.c new file mode 100644
> index 0000000..cac9a39
> --- /dev/null
> +++ b/drivers/xen/xen-acpi-pad-stub.c
> @@ -0,0 +1,128 @@
> +/*
> + * Copyright 2012 by Oracle Inc
> + * Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> + *
> + * This code borrows ideas from https://lkml.org/lkml/2011/11/30/249
> + * so many thanks go to Kevin Tian <kevin.tian@xxxxxxxxx>
> + * and Yu Ke <ke.yu@xxxxxxxxx>.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it + * under the terms and conditions of the GNU General
> Public License, + * version 2, as published by the Free Software
> Foundation. + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> General Public License for + * more details.
> + *
> + */
> +
> +#include <linux/cpumask.h>
> +#include <linux/freezer.h>
> +#include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +#include <acpi/processor.h>
> +
> +#include <xen/xen.h>
> +#include <xen/interface/platform.h>
> +#include <xen/interface/version.h>
> +#include <asm/xen/hypercall.h>
> +
> +/* TODO: Needs comments. */
> +static struct acpi_device_ops *xen_acpi_pad_ops;
> +static bool __read_mostly xen_acpi_pad_registered;
> +static DEFINE_SPINLOCK(xen_acpi_pad_spinlock);
> +
> +int xen_register_acpi_pad_owner(struct acpi_device_ops *ops)
> +{
> +     if (xen_acpi_pad_ops)
> +             return -EEXIST;
> +
> +     spin_lock(&xen_acpi_pad_spinlock);
> +     xen_acpi_pad_ops = ops;
> +     spin_unlock(&xen_acpi_pad_spinlock);
> +     return 0;
> +}
> +int xen_unregister_acpi_pad_owner(struct acpi_device_ops *ops)
> +{
> +     spin_lock(&xen_acpi_pad_spinlock);
> +     if (xen_acpi_pad_ops != ops) {
> +             spin_unlock(&xen_acpi_pad_spinlock);
> +             return -ENODEV;
> +     }
> +     xen_acpi_pad_ops = NULL;
> +     spin_unlock(&xen_acpi_pad_spinlock);
> +     return 0;
> +}
> +
> +static int xen_acpi_pad_remove(struct acpi_device *device, int type)
> +{
> +     int ret = -ENOSYS;
> +
> +     spin_lock(&xen_acpi_pad_spinlock);
> +     if (xen_acpi_pad_ops && xen_acpi_pad_ops->remove)
> +             ret = xen_acpi_pad_ops->remove(device, type);
> +     spin_unlock(&xen_acpi_pad_spinlock);
> +     return ret;
> +}
> +static int xen_acpi_pad_add(struct acpi_device *device)
> +{
> +     int ret = -ENOSYS;
> +     spin_lock(&xen_acpi_pad_spinlock);
> +     if (xen_acpi_pad_ops && xen_acpi_pad_ops->add)
> +             ret = xen_acpi_pad_ops->add(device);
> +     spin_unlock(&xen_acpi_pad_spinlock);
> +     return ret;
> +}
> +
> +static const struct acpi_device_id pad_device_ids[] = {
> +     {"ACPI000C", 0},
> +     {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, pad_device_ids);
> +#define ACPI_PROCESSOR_AGGREGATOR_CLASS "acpi_pad"
> +
> +static struct acpi_driver xen_acpi_pad_driver = {
> +     .name = "processor_aggregator",
> +     .class = ACPI_PROCESSOR_AGGREGATOR_CLASS,
> +     .ids = pad_device_ids,
> +     .ops = {
> +             .add = xen_acpi_pad_add,
> +             .remove = xen_acpi_pad_remove,
> +     },
> +};
> +
> +static int __init xen_acpi_pad_stub_init(void)
> +{
> +     int ret = -ENOSYS;
> +     unsigned version;
> +
> +     if (!xen_initial_domain())
> +             return -ENODEV;
> +
> +     version = HYPERVISOR_xen_version(XENVER_version, NULL);
> +
> +     if ((version >> 16 >= 4) && ((version & 0xffff) >= 2)) {
> +             ret = acpi_bus_register_driver(&xen_acpi_pad_driver);
> +             if (!ret)
> +                     xen_acpi_pad_registered = true;
> +     }
> +     return ret;
> +}
> +#if 0
> +/* For testing purposes. */
> +static void __exit xen_acpi_pad_stub_exit(void)
> +{
> +     if (xen_acpi_pad_registered)
> +             acpi_bus_unregister_driver(&xen_acpi_pad_driver);
> +     /* The driver should have unregistered first ! */
> +     if (WARN_ON(xen_acpi_pad_ops))
> +             xen_acpi_pad_ops = NULL;
> +}
> +#endif
> +subsys_initcall(xen_acpi_pad_stub_init);

I'm still confused. In this way there are xen-acpi-pad-stub.c and 
xen-acpi-pad.c, and you want to let xen-acpi-pad loaded as module, right? how 
can xen-acpi-pad logic work when it was insmoded?

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