RE: [Xen-devel] [PATCH 1/2] Xen acpi pad implement
From: Liu, Jinsong
Date: Tue Oct 30 2012 - 03:58:25 EST
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--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/