* Re: [RFC][PATCH resend 8/12] asm-generic: bitops: introduce le bit offset macro
From: Arnd Bergmann @ 2010-05-10 12:01 UTC (permalink / raw)
To: Takuya Yoshikawa
Cc: linux-arch, x86, kvm, kvm-ia64, fernando, mtosatti, agraf,
kvm-ppc, linux-kernel, linuxppc-dev, mingo, paulus, Avi Kivity,
hpa, tglx, Takuya Yoshikawa
In-Reply-To: <4BE7F22E.9070504@oss.ntt.co.jp>
On Monday 10 May 2010, Takuya Yoshikawa wrote:
> (2010/05/06 22:38), Arnd Bergmann wrote:
> > On Wednesday 05 May 2010, Takuya Yoshikawa wrote:
> >> There was a suggestion to propose set_le_bit_user() kind of macros.
> >> But what I thought was these have a constraint you two explained and seemed to be
> >> a little bit specific to some area, like KVM.
> >>
> >> So I decided to propose just the offset calculation macro.
> >
> > I'm not sure I understand how this macro is going to be used though.
> > If you are just using this in kernel space, that's fine, please go for
> > it.
>
> Yes, I'm just using in kernel space: qemu has its own endian related helpers.
>
> So if you allow us to place this macro in asm-generic/bitops/* it will help us.
No problem at all then. Thanks for the explanation.
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [RFC][PATCH resend 8/12] asm-generic: bitops: introduce le bit offset macro
From: Takuya Yoshikawa @ 2010-05-10 12:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arch, x86, kvm, kvm-ia64, fernando, mtosatti, agraf,
kvm-ppc, linux-kernel, linuxppc-dev, mingo, paulus, Avi Kivity,
hpa, tglx, Takuya Yoshikawa
In-Reply-To: <201005101401.52182.arnd@arndb.de>
>>
>> Yes, I'm just using in kernel space: qemu has its own endian related helpers.
>>
>> So if you allow us to place this macro in asm-generic/bitops/* it will help us.
>
> No problem at all then. Thanks for the explanation.
>
> Acked-by: Arnd Bergmann<arnd@arndb.de>
Thanks you both. I will add your Acked-by from now on!
Takuya
^ permalink raw reply
* Re: [RFC][PATCH resend 8/12] asm-generic: bitops: introduce le bit offset macro
From: Takuya Yoshikawa @ 2010-05-10 11:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arch, x86, kvm, kvm-ia64, fernando, mtosatti, agraf,
kvm-ppc, linux-kernel, linuxppc-dev, mingo, paulus, Avi Kivity,
hpa, tglx, Takuya Yoshikawa
In-Reply-To: <201005061538.54326.arnd@arndb.de>
(2010/05/06 22:38), Arnd Bergmann wrote:
> On Wednesday 05 May 2010, Takuya Yoshikawa wrote:
>> Date:
>> Yesterday 04:59:24
>>> That's why the bitmaps are defined as little endian u64 aligned, even on
>>> big endian 32-bit systems. Little endian bitmaps are wordsize agnostic,
>>> and u64 alignment ensures we can use long-sized bitops on mixed size
>>> systems.
>
> Ok, I see.
>
>> There was a suggestion to propose set_le_bit_user() kind of macros.
>> But what I thought was these have a constraint you two explained and seemed to be
>> a little bit specific to some area, like KVM.
>>
>> So I decided to propose just the offset calculation macro.
>
> I'm not sure I understand how this macro is going to be used though.
> If you are just using this in kernel space, that's fine, please go for
> it.
Yes, I'm just using in kernel space: qemu has its own endian related helpers.
So if you allow us to place this macro in asm-generic/bitops/* it will help us.
Avi, what do you think? Do you want to place it in kvm.h ?
>
> However, if the intention is to use the same macro in user space, putting
> it into asm-generic/bitops/* is not going to help, because those headers
> are not available in user space, and I wouldn't want to change that.
>
> The definition of the macro is not part of the ABI, so just duplicate
> it in KVM if you need it there.
>
> Arnd
^ permalink raw reply
* Re: [RFC][PATCH 0/12] KVM, x86, ppc, asm-generic: moving dirty bitmaps to user space
From: Avi Kivity @ 2010-05-10 12:06 UTC (permalink / raw)
To: Takuya Yoshikawa
Cc: linux-arch, x86, arnd, kvm, kvm-ia64, fernando, mtosatti, agraf,
kvm-ppc, linux-kernel, yoshikawa.takuya, linuxppc-dev, mingo,
paulus, hpa, tglx
In-Reply-To: <20100504215645.6448af8f.takuya.yoshikawa@gmail.com>
On 05/04/2010 03:56 PM, Takuya Yoshikawa wrote:
> [Performance test]
>
> We measured the tsc needed to the ioctl()s for getting dirty logs in
> kernel.
>
> Test environment
>
> AMD Phenom(tm) 9850 Quad-Core Processor with 8GB memory
>
>
> 1. GUI test (running Ubuntu guest in graphical mode)
>
> sudo qemu-system-x86_64 -hda dirtylog_test.img -boot c -m 4192 -net ...
>
> We show a relatively stable part to compare how much time is needed
> for the basic parts of dirty log ioctl.
>
> get.org get.opt switch.opt
>
> slots[7].len=32768 278379 66398 64024
> slots[8].len=32768 181246 270 160
> slots[7].len=32768 263961 64673 64494
> slots[8].len=32768 181655 265 160
> slots[7].len=32768 263736 64701 64610
> slots[8].len=32768 182785 267 160
> slots[7].len=32768 260925 65360 65042
> slots[8].len=32768 182579 264 160
> slots[7].len=32768 267823 65915 65682
> slots[8].len=32768 186350 271 160
>
> At a glance, we know our optimization improved significantly compared
> to the original get dirty log ioctl. This is true for both get.opt and
> switch.opt. This has a really big impact for the personal KVM users who
> drive KVM in GUI mode on their usual PCs.
>
> Next, we notice that switch.opt improved a hundred nano seconds or so for
> these slots. Although this may sound a bit tiny improvement, we can feel
> this as a difference of GUI's responses like mouse reactions.
>
100 ns... this is a bit on the low side (and if you can measure it
interactively you have much better reflexes than I).
> To feel the difference, please try GUI on your PC with our patch series!
>
No doubt get.org -> get.opt is measurable, but get.opt->switch.opt is
problematic. Have you tried profiling to see where the time is spent
(well I can guess, clearing the write access from the sptes).
>
> 2. Live-migration test (4GB guest, write loop with 1GB buf)
>
> We also did a live-migration test.
>
> get.org get.opt switch.opt
>
> slots[0].len=655360 797383 261144 222181
> slots[1].len=3757047808 2186721 1965244 1842824
> slots[2].len=637534208 1433562 1012723 1031213
> slots[3].len=131072 216858 331 331
> slots[4].len=131072 121635 225 164
> slots[5].len=131072 120863 356 164
> slots[6].len=16777216 121746 1133 156
> slots[7].len=32768 120415 230 278
> slots[8].len=32768 120368 216 149
> slots[0].len=655360 806497 194710 223582
> slots[1].len=3757047808 2142922 1878025 1895369
> slots[2].len=637534208 1386512 1021309 1000345
> slots[3].len=131072 221118 459 296
> slots[4].len=131072 121516 272 166
> slots[5].len=131072 122652 244 173
> slots[6].len=16777216 123226 99185 149
> slots[7].len=32768 121803 457 505
> slots[8].len=32768 121586 216 155
> slots[0].len=655360 766113 211317 213179
> slots[1].len=3757047808 2155662 1974790 1842361
> slots[2].len=637534208 1481411 1020004 1031352
> slots[3].len=131072 223100 351 295
> slots[4].len=131072 122982 436 164
> slots[5].len=131072 122100 300 503
> slots[6].len=16777216 123653 779 151
> slots[7].len=32768 122617 284 157
> slots[8].len=32768 122737 253 149
>
> For slots other than 0,1,2 we can see the similar improvement.
>
> Considering the fact that switch.opt does not depend on the bitmap length
> except for kvm_mmu_slot_remove_write_access(), this is the cause of some
> usec to msec time consumption: there might be some context switches.
>
> But note that this was done with the workload which dirtied the memory
> endlessly during the live-migration.
>
> In usual workload, the number of dirty pages varies a lot for each iteration
> and we should gain really a lot for relatively clean cases.
>
Can you post such a test, for an idle large guest?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [RFC][PATCH 0/12] KVM, x86, ppc, asm-generic: moving dirty bitmaps to user space
From: Takuya Yoshikawa @ 2010-05-10 12:26 UTC (permalink / raw)
To: Avi Kivity
Cc: linux-arch, x86, arnd, kvm, kvm-ia64, fernando, mtosatti, agraf,
kvm-ppc, linux-kernel, linuxppc-dev, mingo, paulus, hpa, tglx,
Takuya Yoshikawa
In-Reply-To: <4BE7F6D7.3060005@redhat.com>
>>
>> get.org get.opt switch.opt
>>
>> slots[7].len=32768 278379 66398 64024
>> slots[8].len=32768 181246 270 160
>> slots[7].len=32768 263961 64673 64494
>> slots[8].len=32768 181655 265 160
>> slots[7].len=32768 263736 64701 64610
>> slots[8].len=32768 182785 267 160
>> slots[7].len=32768 260925 65360 65042
>> slots[8].len=32768 182579 264 160
>> slots[7].len=32768 267823 65915 65682
>> slots[8].len=32768 186350 271 160
>>
>> At a glance, we know our optimization improved significantly compared
>> to the original get dirty log ioctl. This is true for both get.opt and
>> switch.opt. This has a really big impact for the personal KVM users who
>> drive KVM in GUI mode on their usual PCs.
>>
>> Next, we notice that switch.opt improved a hundred nano seconds or so for
>> these slots. Although this may sound a bit tiny improvement, we can feel
>> this as a difference of GUI's responses like mouse reactions.
>
> 100 ns... this is a bit on the low side (and if you can measure it
> interactively you have much better reflexes than I).
>
>> To feel the difference, please try GUI on your PC with our patch series!
>
> No doubt get.org -> get.opt is measurable, but get.opt->switch.opt is
> problematic. Have you tried profiling to see where the time is spent
> (well I can guess, clearing the write access from the sptes).
Sorry but no, and I agree with your guess.
Anyway, I want to do some profiling to confirm this guess.
BTW, If we only think about performance improvement of time, optimized
get(get.opt) may be enough at this stage.
But if we consider the future expansion like using user allocated bitmaps,
new API's introduced for switch.opt won't become waste, I think, because we
need a structure to get and export bitmap addresses.
>>
>> In usual workload, the number of dirty pages varies a lot for each
>> iteration
>> and we should gain really a lot for relatively clean cases.
>
> Can you post such a test, for an idle large guest?
OK, I'll do!
>
^ permalink raw reply
* Re: [PATCH 2/2] powerpc: Add power management support to VIO bus
From: Brian King @ 2010-05-10 13:40 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev
In-Reply-To: <1273458906.6591.4.camel@concordia>
On 05/09/2010 09:35 PM, Michael Ellerman wrote:
> On Fri, 2010-05-07 at 13:58 -0500, Brian King wrote:
>> Adds support for suspend/resume for VIO devices. This is needed for
>> support for HMC initiated hibernation.
>>
>> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>
>> arch/powerpc/kernel/vio.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff -puN arch/powerpc/kernel/vio.c~powerpc_vio_bus_pm arch/powerpc/kernel/vio.c
>> --- linux-2.6/arch/powerpc/kernel/vio.c~powerpc_vio_bus_pm 2010-05-07 13:49:16.000000000 -0500
>> +++ linux-2.6-bjking1/arch/powerpc/kernel/vio.c 2010-05-07 13:49:16.000000000 -0500
>> @@ -1358,6 +1358,29 @@ static int vio_hotplug(struct device *de
>> return 0;
>> }
>>
>> +static int vio_pm_suspend(struct device *dev)
>> +{
>> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> +
>> + if (pm && pm->suspend)
>> + return pm->suspend(dev);
>> + return 0;
>> +}
>> +
>> +static int vio_pm_resume(struct device *dev)
>> +{
>> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> +
>> + if (pm && pm->resume)
>> + return pm->resume(dev);
>> + return 0;
>> +}
>> +
>> +const struct dev_pm_ops vio_dev_pm_ops = {
>> + .suspend = vio_pm_suspend,
>> + .resume = vio_pm_resume,
>> +};
>
> That just looks like boiler plate, is there not a generic version
> somewhere?
Looks there is. I'll update the patch to use the generic helpers.
Thanks,
Brian
--
Brian King
Linux on Power Virtualization
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH] powerpc: fix userspace build of ptrace.h
From: Dave Kleikamp @ 2010-05-10 14:10 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev list, Andrey Volkov, Sam Ravnborg, LKML,
David Gibson
In-Reply-To: <1273390060.4861.230.camel@pasglop>
On Sun, 2010-05-09 at 17:27 +1000, Benjamin Herrenschmidt wrote:
> On Sun, 2010-05-09 at 08:59 +0200, Sam Ravnborg wrote:
> > >From ff056c080d2b0b93bac07ad71125fee701919f5e Mon Sep 17 00:00:00 2001
> > From: Sam Ravnborg <sam@ravnborg.org>
> > Date: Sun, 9 May 2010 08:52:31 +0200
> > Subject: [PATCH] powerpc: fix userspace build of ptrace.h
> >
> > Build of ptrace.h failed for assembly because it
> > pulls in stdint.h.
> > Use exportable types (__u32, __u64) to avoid the dependency
> > on stdint.h.
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Andrey Volkov <avolkov@varma-el.com>
> > Cc: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >
> > A better fix is to remove the use of stdint like the following patch does.
> > Note - I have not even build tested this patch!
>
> Ack, thanks, I'll test and apply.
Acked-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
--
Dave Kleikamp
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Partition hibernation support
From: Brian King @ 2010-05-10 14:37 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev
In-Reply-To: <1273474102.6591.258.camel@concordia>
On 05/10/2010 01:48 AM, Michael Ellerman wrote:
> On Fri, 2010-05-07 at 13:58 -0500, Brian King wrote:
>> diff -puN /dev/null arch/powerpc/platforms/pseries/suspend.c
>> --- /dev/null 2009-12-15 17:58:07.000000000 -0600
>> +++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/suspend.c 2010-05-07 13:49:12.000000000 -0500
>> @@ -0,0 +1,209 @@
>> +/*
>> + * Copyright (C) 2010 Brian King IBM Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/suspend.h>
>> +#include <asm/hvcall.h>
>> +#include <asm/machdep.h>
>> +#include <asm/mmu.h>
>> +#include <asm/rtas.h>
>> +
>> +static u64 stream_id;
>
> I don't see any reasons why this is a global?
In summary, I need to be able to access this from pseries_suspend_begin,
whose prototype is dictated by the prototype in struct platform_suspend_ops.
>
>> +static struct sys_device suspend_sysdev;
>> +static DECLARE_COMPLETION(suspend_work);
>> +static struct rtas_suspend_me_data suspend_data;
>> +static atomic_t suspending;
>> +
>> +/**
>> + * pseries_suspend_begin - First phase of hibernation
>> + *
>> + * Check to ensure we are in a valid state to hibernate
>> + *
>> + * Return value:
>> + * 0 on success / other on failure
>> + **/
>> +static int pseries_suspend_begin(suspend_state_t state)
>> +{
>> + long vs, rc;
>
> I read "vs" as "versus", whereas I think it's meant to be "vasi state".
> "state" might be a better name.
Since state is the function parameter, I'll update this to be called vasi_state.
>
>> + unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
>> +
>> + if (!rtas_service_present("ibm,suspend-me"))
>> + return -ENOSYS;
>> +
>> + /* Make sure the state is valid */
>> + rc = plpar_hcall(H_VASI_STATE, retbuf, stream_id);
>> +
>> + vs = retbuf[0];
>> +
>> + if (rc) {
>> + pr_err("pseries_suspend_begin: vasi_state returned %ld\n",rc);
>> + return rc;
>> + } else if (vs == H_VASI_ENABLED) {
>> + return RTAS_NOT_SUSPENDABLE;
>
> I'm sure if I read PAPR this would make sense, but I haven't, why does
> enabled mean we can't suspend?
It means that hibernation has been initiated. The OS is expected to continue
to call the H_VASI_STATE hcall as long as H_VASI_ENABLED is returned, since
firmware is in the process of preparing for the hibernation, but is not
yet ready for the OS to hibernate.
>
>> + } else if (vs != H_VASI_SUSPENDING) {
>> + pr_err("pseries_suspend_begin: vasi_state returned state %ld\n", vs);
>> + return -EIO;
>> + }
>
> The comment above "Make sure the state is valid" made me think that was
> just a preliminary check before we started suspending. But it seems that
> actually starts the suspend?
The suspension actually starts because of the HMC. The HMC sends a command
to firmware to start the hibernation. It also sends a command to the LPAR
with the stream id of the hibernation, which we use on the H_VASI_STATE
hcall. Firmware checks to make sure the stream ids match.
>> +/**
>> + * pseries_prepare_late - Prepare to suspend all other CPUs
>> + *
>> + * Return value:
>> + * 0 on success / other on failure
>> + **/
>> +static int pseries_prepare_late(void)
>> +{
>> + atomic_set(&suspending, 1);
>> + atomic_set(&suspend_data.working, 0);
>> + atomic_set(&suspend_data.done, 0);
>> + atomic_set(&suspend_data.error, 0);
>> + suspend_data.token = rtas_token("ibm,suspend-me");
>
> You could look this up at init time.
Agreed
>
>> + suspend_data.complete = &suspend_work;
>> + INIT_COMPLETION(suspend_work);
>> + return 0;
>> +}
>> +
>> +/**
>> + * store_hibernate - Initiate partition hibernation
>> + * @classdev: sysdev class struct
>> + * @attr: class device attribute struct
>> + * @buf: buffer
>> + * @count: buffer size
>> + *
>> + * Write the stream ID received from the HMC to this file
>> + * to trigger hibernating the partition
>> + *
>> + * Return value:
>> + * number of bytes printed to buffer / other on failure
>> + **/
>> +static ssize_t store_hibernate(struct sysdev_class *classdev,
>> + struct sysdev_class_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int rc;
>> +
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + stream_id = simple_strtoul(buf, NULL, 16);
>
> Error handling?
I'm relying on H_VASI_STATE for the error checking. If an invalid
stream ID is passed to H_VASI_STATE, we will get back H_PARAMETER.
>
>> + do {
>> + rc = pseries_suspend_begin(PM_SUSPEND_MEM);
>> + if (rc == RTAS_NOT_SUSPENDABLE)
>> + ssleep(1);
>
> Hmm OK. So if we're not suspendable, we just try again? That sounds more
> like -EAGAIN to me.
I'll change to use -EAGAIN for this return value
>
>> + } while (rc == RTAS_NOT_SUSPENDABLE);
>> +
>> + if (!rc)
>> + rc = pm_suspend(PM_SUSPEND_MEM);
>> +
>> + stream_id = 0;
>
> Same comment before about this being global.
>
>> + if (!rc)
>> + rc = count;
>> + return rc;
>> +}
>> +
>> +static SYSDEV_CLASS_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
>> +
>> +static struct sysdev_class suspend_sysdev_class = {
>> + .name = "power",
>
> "power" like powerpc, or like electricity? If it's the former should it
> be "pseries" instead.
Like electricty. This results in a sysfs file in the following directory:
/sys/devices/system/power/hibernate
Userspace then simply writes the stream id received from the HMC to this file
to start the hibernation. (This is typically done by the drmgr tool which is
invoked by the HMC.)
> ...
>> +/**
>> + * pseries_suspend_init - initcall for pSeries suspend
>> + *
>> + * Return value:
>> + * 0 on success / other on failure
>> + **/
>> +static int __init pseries_suspend_init(void)
>> +{
>> + int rc;
>> +
>> + if ((rc = pseries_suspend_sysfs_register(&suspend_sysdev)))
>> + return rc;
>> +
>> + ppc_md.suspend_disable_cpu = pseries_suspend_cpu;
>> + suspend_set_ops(&pseries_suspend_ops);
>
> This unconditionally sets the pseries suspend ops, regardless of whether
> we're actually running on a pseries machine, or if the required RTAS
> tokens were found.
>
> You should probably use the presence of ibm,suspend-me as the condition?
> And while you're looking it up anyway you can store it in
> suspend_data :)
Agreed
>
>> + return 0;
>> +}
>> +
>> +__initcall(pseries_suspend_init);
>> diff -puN arch/powerpc/kernel/rtas.c~powerpc_allarch_pseries_hibernation arch/powerpc/kernel/rtas.c
>> --- linux-2.6/arch/powerpc/kernel/rtas.c~powerpc_allarch_pseries_hibernation 2010-05-07 13:49:12.000000000 -0500
>> +++ linux-2.6-bjking1/arch/powerpc/kernel/rtas.c 2010-05-07 13:49:12.000000000 -0500
>> @@ -47,14 +47,6 @@ struct rtas_t rtas = {
>> };
>> EXPORT_SYMBOL(rtas);
>>
>> -struct rtas_suspend_me_data {
>> - atomic_t working; /* number of cpus accessing this struct */
>> - atomic_t done;
>> - int token; /* ibm,suspend-me */
>> - int error;
>> - struct completion *complete; /* wait on this until working == 0 */
>> -};
>> -
>> DEFINE_SPINLOCK(rtas_data_buf_lock);
>> EXPORT_SYMBOL(rtas_data_buf_lock);
>>
>> @@ -711,14 +703,54 @@ void rtas_os_term(char *str)
>>
>> static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
>> #ifdef CONFIG_PPC_PSERIES
>> -static void rtas_percpu_suspend_me(void *info)
>> +static int __rtas_suspend_last_cpu(struct rtas_suspend_me_data *data, int wake_when_done)
>> +{
>> + u16 slb_size = mmu_slb_size;
>> + int rc = H_MULTI_THREADS_ACTIVE;
>> + int cpu;
>> +
>> + atomic_inc(&data->working);
>> +
>> + slb_set_size(SLB_MIN_SIZE);
>> + printk(KERN_DEBUG "calling ibm,suspend-me on cpu %i\n", smp_processor_id());
>> +
>> + while (rc == H_MULTI_THREADS_ACTIVE && !atomic_read(&data->done) &&
>> + !atomic_read(&data->error))
>> + rc = rtas_call(data->token, 0, 1, NULL);
>> +
>> + if (rc || atomic_read(&data->error)) {
>> + printk(KERN_DEBUG "ibm,suspend-me returned %d\n", rc);
>> + slb_set_size(slb_size);
>> + }
>> +
>> + if (atomic_read(&data->error))
>> + rc = atomic_read(&data->error);
>> +
>> + atomic_set(&data->error, rc);
>> +
>> + if (wake_when_done) {
>> + atomic_set(&data->done, 1);
>> +
>> + for_each_online_cpu(cpu)
>> + plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
>> + }
>> +
>> + if (atomic_dec_return(&data->working) == 0)
>> + complete(data->complete);
>> +
>> + return rc;
>> +}
>> +
>> +int rtas_suspend_last_cpu(struct rtas_suspend_me_data *data)
>> +{
>> + return __rtas_suspend_last_cpu(data, 0);
>> +}
>> +
>> +static int __rtas_suspend_cpu(struct rtas_suspend_me_data *data, int wake_when_done)
>> {
>> long rc = H_SUCCESS;
>> unsigned long msr_save;
>> - u16 slb_size = mmu_slb_size;
>> int cpu;
>> - struct rtas_suspend_me_data *data =
>> - (struct rtas_suspend_me_data *)info;
>>
>> atomic_inc(&data->working);
>>
>> @@ -726,7 +758,7 @@ static void rtas_percpu_suspend_me(void
>> msr_save = mfmsr();
>> mtmsr(msr_save & ~(MSR_EE));
>>
>> - while (rc == H_SUCCESS && !atomic_read(&data->done))
>> + while (rc == H_SUCCESS && !atomic_read(&data->done) && !atomic_read(&data->error))
>> rc = plpar_hcall_norets(H_JOIN);
>>
>> mtmsr(msr_save);
>> @@ -738,33 +770,37 @@ static void rtas_percpu_suspend_me(void
>> /* All other cpus are in H_JOIN, this cpu does
>> * the suspend.
>> */
>> - slb_set_size(SLB_MIN_SIZE);
>> - printk(KERN_DEBUG "calling ibm,suspend-me on cpu %i\n",
>> - smp_processor_id());
>> - data->error = rtas_call(data->token, 0, 1, NULL);
>> -
>> - if (data->error) {
>> - printk(KERN_DEBUG "ibm,suspend-me returned %d\n",
>> - data->error);
>> - slb_set_size(slb_size);
>> - }
>> + return __rtas_suspend_last_cpu(data, wake_when_done);
>> } else {
>> printk(KERN_ERR "H_JOIN on cpu %i failed with rc = %ld\n",
>> smp_processor_id(), rc);
>> - data->error = rc;
>> + atomic_set(&data->error, rc);
>> }
>>
>> - atomic_set(&data->done, 1);
>> + if (wake_when_done) {
>> + atomic_set(&data->done, 1);
>>
>> - /* This cpu did the suspend or got an error; in either case,
>> - * we need to prod all other other cpus out of join state.
>> - * Extra prods are harmless.
>> - */
>> - for_each_online_cpu(cpu)
>> - plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
>> + /* This cpu did the suspend or got an error; in either case,
>> + * we need to prod all other other cpus out of join state.
>> + * Extra prods are harmless.
>> + */
>> + for_each_online_cpu(cpu)
>> + plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
>> + }
>> out:
>> if (atomic_dec_return(&data->working) == 0)
>> complete(data->complete);
>> + return rc;
>> +}
>> +
>> +int rtas_suspend_cpu(struct rtas_suspend_me_data *data)
>> +{
>> + return __rtas_suspend_cpu(data, 0);
>> +}
>> +
>> +static void rtas_percpu_suspend_me(void *info)
>> +{
>> + __rtas_suspend_cpu((struct rtas_suspend_me_data *)info, 1);
>> }
>
> What is the current path via rtas_ibm_suspend_me() used for? It's not
> clear to me that you've changed that in a way that is equivalent.
Its used for Live Partition Mobility. This patch does change this path
slightly, but it should be functionally equivalent.
>> static int rtas_ibm_suspend_me(struct rtas_args *args)
>> @@ -799,29 +835,41 @@ static int rtas_ibm_suspend_me(struct rt
>>
>> atomic_set(&data.working, 0);
>> atomic_set(&data.done, 0);
>> + atomic_set(&data.error, 0);
>> data.token = rtas_token("ibm,suspend-me");
>> - data.error = 0;
>> data.complete = &done;
>>
>> /* Call function on all CPUs. One of us will make the
>> * rtas call
>> */
>> if (on_each_cpu(rtas_percpu_suspend_me, &data, 0))
>> - data.error = -EINVAL;
>> + atomic_set(&data.error, -EINVAL);
>>
>> wait_for_completion(&done);
>>
>> - if (data.error != 0)
>> + if (atomic_read(&data.error) != 0)
>> printk(KERN_ERR "Error doing global join\n");
>>
>> - return data.error;
>> + return atomic_read(&data.error);
>
> So while moving the struct definition you've also changed error to be
> atomic, and fixed the code here. That would be nicer as a separate
> commit, at the very least you should call it out in your changelog.
Ok.
>
> And why does it need to be atomic?
Its potentially checked and set on different cpus. I considered creating
a lock for updating this data structure, which seemed like overkill and
would have complicated the code. Another option would be to simply sprinkle
memory barriers at the appropriate places. I thought the atomics were
simple, clean, and consistent with the existing code.
>> }
>> #else /* CONFIG_PPC_PSERIES */
>> static int rtas_ibm_suspend_me(struct rtas_args *args)
>> {
>> return -ENOSYS;
>> }
>> +
>> +int rtas_suspend_cpu(struct rtas_suspend_me_data *data)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> +int rtas_suspend_last_cpu(struct rtas_suspend_me_data *data)
>> +{
>> + return -ENOSYS;
>> +}
>
> Don't see why you need these empty versions, they're only ever called
> from pseries code aren't they?
True, but we still need to be able to link the code in a cross platform
kernel.
>> #endif
>> +EXPORT_SYMBOL_GPL(rtas_suspend_cpu);
>> +EXPORT_SYMBOL_GPL(rtas_suspend_last_cpu);
>
> Don't see why these need to be exported, CONFIG_SUSPEND is bool so
> pseries/suspend.c is only ever built-in.
Ok.
>
>> diff -puN arch/powerpc/include/asm/rtas.h~powerpc_allarch_pseries_hibernation arch/powerpc/include/asm/rtas.h
>> --- linux-2.6/arch/powerpc/include/asm/rtas.h~powerpc_allarch_pseries_hibernation 2010-05-07 13:49:12.000000000 -0500
>> +++ linux-2.6-bjking1/arch/powerpc/include/asm/rtas.h 2010-05-07 13:49:12.000000000 -0500
>> @@ -63,6 +63,14 @@ struct rtas_t {
>> struct device_node *dev; /* virtual address pointer */
>> };
>>
>> +struct rtas_suspend_me_data {
>> + atomic_t working; /* number of cpus accessing this struct */
>> + atomic_t done;
>> + int token; /* ibm,suspend-me */
>> + atomic_t error;
>> + struct completion *complete; /* wait on this until working == 0 */
>> +};
>> +
>> /* RTAS event classes */
>> #define RTAS_INTERNAL_ERROR 0x80000000 /* set bit 0 */
>> #define RTAS_EPOW_WARNING 0x40000000 /* set bit 1 */
>
>
>> diff -puN arch/powerpc/include/asm/hvcall.h~powerpc_allarch_pseries_hibernation arch/powerpc/include/asm/hvcall.h
>> --- linux-2.6/arch/powerpc/include/asm/hvcall.h~powerpc_allarch_pseries_hibernation 2010-05-07 13:49:12.000000000 -0500
>> +++ linux-2.6-bjking1/arch/powerpc/include/asm/hvcall.h 2010-05-07 13:49:12.000000000 -0500
>> @@ -74,6 +74,7 @@
>> #define H_NOT_ENOUGH_RESOURCES -44
>> #define H_R_STATE -45
>> #define H_RESCINDEND -46
>> +#define H_MULTI_THREADS_ACTIVE -9005
>
> Which means what?
When calling ibm,suspend-me, all other CPU threads must have called H_JOIN to ensure
that there are no other CPU threads executing in the LPAR. If this has not occurred,
the ibm,suspend-me RTAS call will fail with H_MULTI_THREADS_ACTIVE.
>
>> diff -puN arch/powerpc/include/asm/machdep.h~powerpc_allarch_pseries_hibernation arch/powerpc/include/asm/machdep.h
>> --- linux-2.6/arch/powerpc/include/asm/machdep.h~powerpc_allarch_pseries_hibernation 2010-05-07 13:49:12.000000000 -0500
>> +++ linux-2.6-bjking1/arch/powerpc/include/asm/machdep.h 2010-05-07 13:49:12.000000000 -0500
>> @@ -266,6 +266,7 @@ struct machdep_calls {
>> void (*suspend_disable_irqs)(void);
>> void (*suspend_enable_irqs)(void);
>> #endif
>> + int (*suspend_disable_cpu)(void);
>
> Should this be ifdef CONFIG_SUSPEND ?
I purposely did not do that so that the code in arch/powerpc/platforms/pseries/hotplug-cpu.c
wouldn't need to ifdef CONFIG_SUSPEND around checking this function pointer.
>
>> diff -puN arch/powerpc/platforms/pseries/hotplug-cpu.c~powerpc_allarch_pseries_hibernation arch/powerpc/platforms/pseries/hotplug-cpu.c
>> --- linux-2.6/arch/powerpc/platforms/pseries/hotplug-cpu.c~powerpc_allarch_pseries_hibernation 2010-05-07 13:49:12.000000000 -0500
>> +++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/hotplug-cpu.c 2010-05-07 13:49:12.000000000 -0500
>> @@ -116,6 +116,12 @@ static void pseries_mach_cpu_die(void)
>>
>> if (get_preferred_offline_state(cpu) == CPU_STATE_INACTIVE) {
>> set_cpu_current_state(cpu, CPU_STATE_INACTIVE);
>> + if (ppc_md.suspend_disable_cpu)
>> + ppc_md.suspend_disable_cpu();
>> + }
>> +
>> + if (get_preferred_offline_state(cpu) == CPU_STATE_INACTIVE) {
>> + set_cpu_current_state(cpu, CPU_STATE_INACTIVE);
>
> That looks weird? Do we expect preferred offline state to change?
After looking at this a bit closer, I don't think it can. I'll update in the next rev
of the patch. Thanks for reviewing!
-Brian
--
Brian King
Linux on Power Virtualization
IBM Linux Technology Center
^ permalink raw reply
* [PATCH] hwmon: (tmp421) Add nfactor support.
From: Jeff Angielski @ 2010-05-10 14:43 UTC (permalink / raw)
To: Linuxppc-dev, Andre Prendel, Jean Delvare
Add support for reading and writing the n-factor correction
registers. This is needed to compensate for the characteristics
of a particular sensor hanging off of the remote channels.
Signed-off-by: Jeff Angielski <jeff@theptrgroup.com>
---
drivers/hwmon/tmp421.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 738c472..c9e9855 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
+static const u8 TMP421_NFACTOR[3] = { 0x21, 0x22, 0x23 };
/* Flags */
#define TMP421_CONFIG_SHUTDOWN 0x40
@@ -157,6 +158,38 @@ static ssize_t show_fault(struct device *dev,
return sprintf(buf, "0\n");
}
+static ssize_t show_nfactor(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp421_data *data = i2c_get_clientdata(client);
+ int index = to_sensor_dev_attr(devattr)->index;
+ s8 nfactor;
+
+ mutex_lock(&data->update_lock);
+ nfactor = i2c_smbus_read_byte_data(client, TMP421_NFACTOR[index-1]);
+ mutex_unlock(&data->update_lock);
+
+ return sprintf(buf, "%d\n", nfactor);
+}
+
+static ssize_t set_nfactor(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp421_data *data = i2c_get_clientdata(client);
+ int index = to_sensor_dev_attr(devattr)->index;
+ int nfactor = simple_strtol(buf, NULL, 10);
+
+ mutex_lock(&data->update_lock);
+ i2c_smbus_write_byte_data(client, TMP421_NFACTOR[index-1],
+ SENSORS_LIMIT(nfactor, -128, 127));
+ mutex_unlock(&data->update_lock);
+
+ return count;
+}
+
static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
int n)
{
@@ -177,19 +210,28 @@ static mode_t tmp421_is_visible(struct kobject
*kobj, struct attribute *a,
static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_fault, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp2_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 1);
static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2);
static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_fault, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp3_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 2);
static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3);
static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_fault, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp4_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 3);
static struct attribute *tmp421_attr[] = {
&sensor_dev_attr_temp1_input.dev_attr.attr,
&sensor_dev_attr_temp2_input.dev_attr.attr,
&sensor_dev_attr_temp2_fault.dev_attr.attr,
+ &sensor_dev_attr_temp2_nfactor.dev_attr.attr,
&sensor_dev_attr_temp3_input.dev_attr.attr,
&sensor_dev_attr_temp3_fault.dev_attr.attr,
+ &sensor_dev_attr_temp3_nfactor.dev_attr.attr,
&sensor_dev_attr_temp4_input.dev_attr.attr,
&sensor_dev_attr_temp4_fault.dev_attr.attr,
+ &sensor_dev_attr_temp4_nfactor.dev_attr.attr,
NULL
};
--
1.7.0.4
--
Jeff Angielski
The PTR Group
www.theptrgroup.com
^ permalink raw reply related
* Re: [PATCH] serial: mpc52xx_uart: fix null pointer dereference
From: Greg KH @ 2010-05-10 16:25 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, leonardo.lists, Anatolij Gustschin, linux-kernel,
Andrew Morton
In-Reply-To: <i2xfa686aa41005091504ve2b35a09m23cd2dc13a331f86@mail.gmail.com>
On Mon, May 10, 2010 at 12:04:30AM +0200, Grant Likely wrote:
> On Wed, May 5, 2010 at 12:18 AM, Anatolij Gustschin <agust@denx.de> wrote:
> > Commit 6acc6833510db8f72b5ef343296d97480555fda9
> > introduced NULL pointer dereference and kernel crash
> > on ppc32 machines while booting. Fix this bug now.
> >
> > Reported-by: Leonardo Chiquitto <leonardo.lists@gmail.com>
> > Signed-off-by: Anatolij Gustschin <agust@denx.de>
>
> Thanks for being so quick to pick up on this Anatolij. I've been
> traveling and not able to respond to stuff. I see that Greg has
> already picked this up (thanks Greg!), but FWIW:
>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>
> Actually, now that I look, this fix needs to go into Linus' tree right
> away (not just linux-next) since the offending patch went in during
> the 2.6.34 merge window.
>
> Greg, I can either pick this up and push it out to Linus tomorrow, or
> let you do it. Whichever you prefer. Let me know so I don't cause
> conflicts in your tree.
I have 2 other tty/serial fixes to go to Linus for .34 along with this
one, so I'll be glad to send it to him. I've added your ack to the
patch.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] 85xx: Fix PCI-E interrupt mapping for slot 0 of P2020DS
From: Felix Radensky @ 2010-05-10 19:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Felix Radensky
Fix legacy PCI-E interrupt mapping for PCI-E slot 0 of
P2020DS evaluation board. The patch is based on P2020DS
device tree from Freescale BSP for this board.
Signed-off-by: Felix Radensky <felix@embedded-sol.com>
---
arch/powerpc/boot/dts/p2020ds.dts | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/boot/dts/p2020ds.dts b/arch/powerpc/boot/dts/p2020ds.dts
index 1101914..7c5fc07 100644
--- a/arch/powerpc/boot/dts/p2020ds.dts
+++ b/arch/powerpc/boot/dts/p2020ds.dts
@@ -507,10 +507,10 @@
interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
interrupt-map = <
/* IDSEL 0x0 */
- 0000 0x0 0x0 0x1 &mpic 0x8 0x1
- 0000 0x0 0x0 0x2 &mpic 0x9 0x1
- 0000 0x0 0x0 0x3 &mpic 0xa 0x1
- 0000 0x0 0x0 0x4 &mpic 0xb 0x1
+ 0000 0x0 0x0 0x1 &mpic 0x8 0x2
+ 0000 0x0 0x0 0x2 &mpic 0x9 0x2
+ 0000 0x0 0x0 0x3 &mpic 0xa 0x2
+ 0000 0x0 0x0 0x4 &mpic 0xb 0x2
>;
pcie@0 {
reg = <0x0 0x0 0x0 0x0 0x0>;
--
1.5.4.3
^ permalink raw reply related
* Re: [PATCH] serial: mpc52xx_uart: fix null pointer dereference
From: Grant Likely @ 2010-05-10 20:46 UTC (permalink / raw)
To: Greg KH
Cc: linuxppc-dev, leonardo.lists, Anatolij Gustschin, linux-kernel,
Andrew Morton
In-Reply-To: <20100510162535.GA15137@suse.de>
On Mon, May 10, 2010 at 6:25 PM, Greg KH <gregkh@suse.de> wrote:
> On Mon, May 10, 2010 at 12:04:30AM +0200, Grant Likely wrote:
>> Greg, I can either pick this up and push it out to Linus tomorrow, or
>> let you do it. =A0Whichever you prefer. =A0Let me know so I don't cause
>> conflicts in your tree.
>
> I have 2 other tty/serial fixes to go to Linus for .34 along with this
> one, so I'll be glad to send it to him. =A0I've added your ack to the
> patch.
Perfect. Thanks Greg.
g.
^ permalink raw reply
* [PATCHv2 3/3] powerpc: Partition hibernation support
From: Brian King @ 2010-05-10 20:53 UTC (permalink / raw)
To: benh; +Cc: brking, linuxppc-dev
Enables support for HMC initiated partition hibernation. This is
a firmware assisted hibernation, since the firmware handles writing
the memory out to disk, along with other partition information,
so we just mimic suspend to ram.
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
arch/powerpc/Kconfig | 2
arch/powerpc/include/asm/hvcall.h | 1
arch/powerpc/include/asm/machdep.h | 1
arch/powerpc/platforms/pseries/Makefile | 4
arch/powerpc/platforms/pseries/hotplug-cpu.c | 3
arch/powerpc/platforms/pseries/suspend.c | 214 +++++++++++++++++++++++++++
6 files changed, 224 insertions(+), 1 deletion(-)
diff -puN /dev/null arch/powerpc/platforms/pseries/suspend.c
--- /dev/null 2009-12-15 17:58:07.000000000 -0600
+++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/suspend.c 2010-05-10 14:18:44.000000000 -0500
@@ -0,0 +1,214 @@
+/*
+ * Copyright (C) 2010 Brian King IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/delay.h>
+#include <linux/suspend.h>
+#include <asm/firmware.h>
+#include <asm/hvcall.h>
+#include <asm/machdep.h>
+#include <asm/mmu.h>
+#include <asm/rtas.h>
+
+static u64 stream_id;
+static struct sys_device suspend_sysdev;
+static DECLARE_COMPLETION(suspend_work);
+static struct rtas_suspend_me_data suspend_data;
+static atomic_t suspending;
+
+/**
+ * pseries_suspend_begin - First phase of hibernation
+ *
+ * Check to ensure we are in a valid state to hibernate
+ *
+ * Return value:
+ * 0 on success / other on failure
+ **/
+static int pseries_suspend_begin(suspend_state_t state)
+{
+ long vasi_state, rc;
+ unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
+
+ /* Make sure the state is valid */
+ rc = plpar_hcall(H_VASI_STATE, retbuf, stream_id);
+
+ vasi_state = retbuf[0];
+
+ if (rc) {
+ pr_err("pseries_suspend_begin: vasi_state returned %ld\n",rc);
+ return rc;
+ } else if (vasi_state == H_VASI_ENABLED) {
+ return -EAGAIN;
+ } else if (vasi_state != H_VASI_SUSPENDING) {
+ pr_err("pseries_suspend_begin: vasi_state returned state %ld\n",
+ vasi_state);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/**
+ * pseries_suspend_cpu - Suspend a single CPU
+ *
+ * Makes the H_JOIN call to suspend the CPU
+ *
+ **/
+static int pseries_suspend_cpu(void)
+{
+ if (atomic_read(&suspending))
+ return rtas_suspend_cpu(&suspend_data);
+ return 0;
+}
+
+/**
+ * pseries_suspend_enter - Final phase of hibernation
+ *
+ * Return value:
+ * 0 on success / other on failure
+ **/
+static int pseries_suspend_enter(suspend_state_t state)
+{
+ int rc = rtas_suspend_last_cpu(&suspend_data);
+
+ atomic_set(&suspending, 0);
+ atomic_set(&suspend_data.done, 1);
+ return rc;
+}
+
+/**
+ * pseries_prepare_late - Prepare to suspend all other CPUs
+ *
+ * Return value:
+ * 0 on success / other on failure
+ **/
+static int pseries_prepare_late(void)
+{
+ atomic_set(&suspending, 1);
+ atomic_set(&suspend_data.working, 0);
+ atomic_set(&suspend_data.done, 0);
+ atomic_set(&suspend_data.error, 0);
+ suspend_data.complete = &suspend_work;
+ INIT_COMPLETION(suspend_work);
+ return 0;
+}
+
+/**
+ * store_hibernate - Initiate partition hibernation
+ * @classdev: sysdev class struct
+ * @attr: class device attribute struct
+ * @buf: buffer
+ * @count: buffer size
+ *
+ * Write the stream ID received from the HMC to this file
+ * to trigger hibernating the partition
+ *
+ * Return value:
+ * number of bytes printed to buffer / other on failure
+ **/
+static ssize_t store_hibernate(struct sysdev_class *classdev,
+ struct sysdev_class_attribute *attr,
+ const char *buf, size_t count)
+{
+ int rc;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ stream_id = simple_strtoul(buf, NULL, 16);
+
+ do {
+ rc = pseries_suspend_begin(PM_SUSPEND_MEM);
+ if (rc == -EAGAIN)
+ ssleep(1);
+ } while (rc == -EAGAIN);
+
+ if (!rc)
+ rc = pm_suspend(PM_SUSPEND_MEM);
+
+ stream_id = 0;
+
+ if (!rc)
+ rc = count;
+ return rc;
+}
+
+static SYSDEV_CLASS_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
+
+static struct sysdev_class suspend_sysdev_class = {
+ .name = "power",
+};
+
+static struct platform_suspend_ops pseries_suspend_ops = {
+ .valid = suspend_valid_only_mem,
+ .begin = pseries_suspend_begin,
+ .prepare_late = pseries_prepare_late,
+ .enter = pseries_suspend_enter,
+};
+
+/**
+ * pseries_suspend_sysfs_register - Register with sysfs
+ *
+ * Return value:
+ * 0 on success / other on failure
+ **/
+static int pseries_suspend_sysfs_register(struct sys_device *sysdev)
+{
+ int rc;
+
+ if ((rc = sysdev_class_register(&suspend_sysdev_class)))
+ return rc;
+
+ sysdev->id = 0;
+ sysdev->cls = &suspend_sysdev_class;
+
+ if ((rc = sysdev_class_create_file(&suspend_sysdev_class, &attr_hibernate)))
+ goto class_unregister;
+
+ return 0;
+
+class_unregister:
+ sysdev_class_unregister(&suspend_sysdev_class);
+ return rc;
+}
+
+/**
+ * pseries_suspend_init - initcall for pSeries suspend
+ *
+ * Return value:
+ * 0 on success / other on failure
+ **/
+static int __init pseries_suspend_init(void)
+{
+ int rc;
+
+ if (!machine_is(pseries) || !firmware_has_feature(FW_FEATURE_LPAR))
+ return 0;
+
+ suspend_data.token = rtas_token("ibm,suspend-me");
+ if (suspend_data.token == RTAS_UNKNOWN_SERVICE)
+ return 0;
+
+ if ((rc = pseries_suspend_sysfs_register(&suspend_sysdev)))
+ return rc;
+
+ ppc_md.suspend_disable_cpu = pseries_suspend_cpu;
+ suspend_set_ops(&pseries_suspend_ops);
+ return 0;
+}
+
+__initcall(pseries_suspend_init);
diff -puN arch/powerpc/Kconfig~powerpc_allarch_pseries_hibernation arch/powerpc/Kconfig
--- linux-2.6/arch/powerpc/Kconfig~powerpc_allarch_pseries_hibernation 2010-05-10 14:18:44.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/Kconfig 2010-05-10 14:18:44.000000000 -0500
@@ -217,7 +217,7 @@ config ARCH_HIBERNATION_POSSIBLE
config ARCH_SUSPEND_POSSIBLE
def_bool y
depends on ADB_PMU || PPC_EFIKA || PPC_LITE5200 || PPC_83xx || \
- PPC_85xx || PPC_86xx
+ PPC_85xx || PPC_86xx || PPC_PSERIES
config PPC_DCR_NATIVE
bool
diff -puN arch/powerpc/platforms/pseries/Makefile~powerpc_allarch_pseries_hibernation arch/powerpc/platforms/pseries/Makefile
--- linux-2.6/arch/powerpc/platforms/pseries/Makefile~powerpc_allarch_pseries_hibernation 2010-05-10 14:18:44.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/Makefile 2010-05-10 14:18:44.000000000 -0500
@@ -26,3 +26,7 @@ obj-$(CONFIG_HCALL_STATS) += hvCall_inst
obj-$(CONFIG_PHYP_DUMP) += phyp_dump.o
obj-$(CONFIG_CMM) += cmm.o
obj-$(CONFIG_DTL) += dtl.o
+
+ifeq ($(CONFIG_PPC_PSERIES),y)
+obj-$(CONFIG_SUSPEND) += suspend.o
+endif
diff -puN arch/powerpc/include/asm/hvcall.h~powerpc_allarch_pseries_hibernation arch/powerpc/include/asm/hvcall.h
--- linux-2.6/arch/powerpc/include/asm/hvcall.h~powerpc_allarch_pseries_hibernation 2010-05-10 14:18:44.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/include/asm/hvcall.h 2010-05-10 14:18:44.000000000 -0500
@@ -74,6 +74,7 @@
#define H_NOT_ENOUGH_RESOURCES -44
#define H_R_STATE -45
#define H_RESCINDEND -46
+#define H_MULTI_THREADS_ACTIVE -9005
/* Long Busy is a condition that can be returned by the firmware
diff -puN arch/powerpc/include/asm/machdep.h~powerpc_allarch_pseries_hibernation arch/powerpc/include/asm/machdep.h
--- linux-2.6/arch/powerpc/include/asm/machdep.h~powerpc_allarch_pseries_hibernation 2010-05-10 14:18:44.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/include/asm/machdep.h 2010-05-10 14:18:44.000000000 -0500
@@ -266,6 +266,7 @@ struct machdep_calls {
void (*suspend_disable_irqs)(void);
void (*suspend_enable_irqs)(void);
#endif
+ int (*suspend_disable_cpu)(void);
#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
ssize_t (*cpu_probe)(const char *, size_t);
diff -puN arch/powerpc/platforms/pseries/hotplug-cpu.c~powerpc_allarch_pseries_hibernation arch/powerpc/platforms/pseries/hotplug-cpu.c
--- linux-2.6/arch/powerpc/platforms/pseries/hotplug-cpu.c~powerpc_allarch_pseries_hibernation 2010-05-10 14:18:44.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/hotplug-cpu.c 2010-05-10 14:18:44.000000000 -0500
@@ -116,6 +116,9 @@ static void pseries_mach_cpu_die(void)
if (get_preferred_offline_state(cpu) == CPU_STATE_INACTIVE) {
set_cpu_current_state(cpu, CPU_STATE_INACTIVE);
+ if (ppc_md.suspend_disable_cpu)
+ ppc_md.suspend_disable_cpu();
+
cede_latency_hint = 2;
get_lppaca()->idle = 1;
_
^ permalink raw reply
* [PATCHv2 1/3] powerpc: Migration code reorganization / hibernation prep
From: Brian King @ 2010-05-10 20:53 UTC (permalink / raw)
To: benh; +Cc: brking, linuxppc-dev
Partition hibernation will use some of the same code as is
currently used for Live Partition Migration. This function
further abstracts this code such that code outside of rtas.c
can utilize it. It also changes the error field in the suspend
me data structure to be an atomic type, since it is set and
checked on different cpus without any barriers or locking.
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/rtas.h | 10 +++
arch/powerpc/kernel/rtas.c | 106 ++++++++++++++++++++++++++--------------
2 files changed, 81 insertions(+), 35 deletions(-)
diff -puN arch/powerpc/include/asm/rtas.h~powerpc_rtas_hibernate_prep arch/powerpc/include/asm/rtas.h
--- linux-2.6/arch/powerpc/include/asm/rtas.h~powerpc_rtas_hibernate_prep 2010-05-10 14:10:37.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/include/asm/rtas.h 2010-05-10 14:16:23.000000000 -0500
@@ -63,6 +63,14 @@ struct rtas_t {
struct device_node *dev; /* virtual address pointer */
};
+struct rtas_suspend_me_data {
+ atomic_t working; /* number of cpus accessing this struct */
+ atomic_t done;
+ int token; /* ibm,suspend-me */
+ atomic_t error;
+ struct completion *complete; /* wait on this until working == 0 */
+};
+
/* RTAS event classes */
#define RTAS_INTERNAL_ERROR 0x80000000 /* set bit 0 */
#define RTAS_EPOW_WARNING 0x40000000 /* set bit 1 */
@@ -174,6 +182,8 @@ extern int rtas_set_indicator(int indica
extern int rtas_set_indicator_fast(int indicator, int index, int new_value);
extern void rtas_progress(char *s, unsigned short hex);
extern void rtas_initialize(void);
+extern int rtas_suspend_cpu(struct rtas_suspend_me_data *data);
+extern int rtas_suspend_last_cpu(struct rtas_suspend_me_data *data);
struct rtc_time;
extern unsigned long rtas_get_boot_time(void);
diff -puN arch/powerpc/kernel/rtas.c~powerpc_rtas_hibernate_prep arch/powerpc/kernel/rtas.c
--- linux-2.6/arch/powerpc/kernel/rtas.c~powerpc_rtas_hibernate_prep 2010-05-10 14:10:37.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/kernel/rtas.c 2010-05-10 14:16:23.000000000 -0500
@@ -47,14 +47,6 @@ struct rtas_t rtas = {
};
EXPORT_SYMBOL(rtas);
-struct rtas_suspend_me_data {
- atomic_t working; /* number of cpus accessing this struct */
- atomic_t done;
- int token; /* ibm,suspend-me */
- int error;
- struct completion *complete; /* wait on this until working == 0 */
-};
-
DEFINE_SPINLOCK(rtas_data_buf_lock);
EXPORT_SYMBOL(rtas_data_buf_lock);
@@ -711,14 +703,54 @@ void rtas_os_term(char *str)
static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
#ifdef CONFIG_PPC_PSERIES
-static void rtas_percpu_suspend_me(void *info)
+static int __rtas_suspend_last_cpu(struct rtas_suspend_me_data *data, int wake_when_done)
+{
+ u16 slb_size = mmu_slb_size;
+ int rc = H_MULTI_THREADS_ACTIVE;
+ int cpu;
+
+ atomic_inc(&data->working);
+
+ slb_set_size(SLB_MIN_SIZE);
+ printk(KERN_DEBUG "calling ibm,suspend-me on cpu %i\n", smp_processor_id());
+
+ while (rc == H_MULTI_THREADS_ACTIVE && !atomic_read(&data->done) &&
+ !atomic_read(&data->error))
+ rc = rtas_call(data->token, 0, 1, NULL);
+
+ if (rc || atomic_read(&data->error)) {
+ printk(KERN_DEBUG "ibm,suspend-me returned %d\n", rc);
+ slb_set_size(slb_size);
+ }
+
+ if (atomic_read(&data->error))
+ rc = atomic_read(&data->error);
+
+ atomic_set(&data->error, rc);
+
+ if (wake_when_done) {
+ atomic_set(&data->done, 1);
+
+ for_each_online_cpu(cpu)
+ plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
+ }
+
+ if (atomic_dec_return(&data->working) == 0)
+ complete(data->complete);
+
+ return rc;
+}
+
+int rtas_suspend_last_cpu(struct rtas_suspend_me_data *data)
+{
+ return __rtas_suspend_last_cpu(data, 0);
+}
+
+static int __rtas_suspend_cpu(struct rtas_suspend_me_data *data, int wake_when_done)
{
long rc = H_SUCCESS;
unsigned long msr_save;
- u16 slb_size = mmu_slb_size;
int cpu;
- struct rtas_suspend_me_data *data =
- (struct rtas_suspend_me_data *)info;
atomic_inc(&data->working);
@@ -726,7 +758,7 @@ static void rtas_percpu_suspend_me(void
msr_save = mfmsr();
mtmsr(msr_save & ~(MSR_EE));
- while (rc == H_SUCCESS && !atomic_read(&data->done))
+ while (rc == H_SUCCESS && !atomic_read(&data->done) && !atomic_read(&data->error))
rc = plpar_hcall_norets(H_JOIN);
mtmsr(msr_save);
@@ -738,33 +770,37 @@ static void rtas_percpu_suspend_me(void
/* All other cpus are in H_JOIN, this cpu does
* the suspend.
*/
- slb_set_size(SLB_MIN_SIZE);
- printk(KERN_DEBUG "calling ibm,suspend-me on cpu %i\n",
- smp_processor_id());
- data->error = rtas_call(data->token, 0, 1, NULL);
-
- if (data->error) {
- printk(KERN_DEBUG "ibm,suspend-me returned %d\n",
- data->error);
- slb_set_size(slb_size);
- }
+ return __rtas_suspend_last_cpu(data, wake_when_done);
} else {
printk(KERN_ERR "H_JOIN on cpu %i failed with rc = %ld\n",
smp_processor_id(), rc);
- data->error = rc;
+ atomic_set(&data->error, rc);
}
- atomic_set(&data->done, 1);
+ if (wake_when_done) {
+ atomic_set(&data->done, 1);
- /* This cpu did the suspend or got an error; in either case,
- * we need to prod all other other cpus out of join state.
- * Extra prods are harmless.
- */
- for_each_online_cpu(cpu)
- plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
+ /* This cpu did the suspend or got an error; in either case,
+ * we need to prod all other other cpus out of join state.
+ * Extra prods are harmless.
+ */
+ for_each_online_cpu(cpu)
+ plpar_hcall_norets(H_PROD, get_hard_smp_processor_id(cpu));
+ }
out:
if (atomic_dec_return(&data->working) == 0)
complete(data->complete);
+ return rc;
+}
+
+int rtas_suspend_cpu(struct rtas_suspend_me_data *data)
+{
+ return __rtas_suspend_cpu(data, 0);
+}
+
+static void rtas_percpu_suspend_me(void *info)
+{
+ __rtas_suspend_cpu((struct rtas_suspend_me_data *)info, 1);
}
static int rtas_ibm_suspend_me(struct rtas_args *args)
@@ -799,22 +835,22 @@ static int rtas_ibm_suspend_me(struct rt
atomic_set(&data.working, 0);
atomic_set(&data.done, 0);
+ atomic_set(&data.error, 0);
data.token = rtas_token("ibm,suspend-me");
- data.error = 0;
data.complete = &done;
/* Call function on all CPUs. One of us will make the
* rtas call
*/
if (on_each_cpu(rtas_percpu_suspend_me, &data, 0))
- data.error = -EINVAL;
+ atomic_set(&data.error, -EINVAL);
wait_for_completion(&done);
- if (data.error != 0)
+ if (atomic_read(&data.error) != 0)
printk(KERN_ERR "Error doing global join\n");
- return data.error;
+ return atomic_read(&data.error);
}
#else /* CONFIG_PPC_PSERIES */
static int rtas_ibm_suspend_me(struct rtas_args *args)
_
^ permalink raw reply
* [PATCHv2 2/3] powerpc: Add power management support to VIO bus
From: Brian King @ 2010-05-10 20:53 UTC (permalink / raw)
To: benh; +Cc: brking, linuxppc-dev
Adds support for suspend/resume for VIO devices. This is needed for
support for HMC initiated hibernation.
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
arch/powerpc/kernel/vio.c | 1 +
1 file changed, 1 insertion(+)
diff -puN arch/powerpc/kernel/vio.c~powerpc_vio_bus_pm arch/powerpc/kernel/vio.c
--- linux-2.6/arch/powerpc/kernel/vio.c~powerpc_vio_bus_pm 2010-05-10 11:12:15.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/kernel/vio.c 2010-05-10 11:12:15.000000000 -0500
@@ -1365,6 +1365,7 @@ static struct bus_type vio_bus_type = {
.match = vio_bus_match,
.probe = vio_bus_probe,
.remove = vio_bus_remove,
+ .pm = GENERIC_SUBSYS_PM_OPS,
};
/**
_
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Partition hibernation support
From: Brian King @ 2010-05-10 20:57 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev
In-Reply-To: <4BE81A3E.8030501@linux.vnet.ibm.com>
On 05/10/2010 09:37 AM, Brian King wrote:
>
>>> }
>>> #else /* CONFIG_PPC_PSERIES */
>>> static int rtas_ibm_suspend_me(struct rtas_args *args)
>>> {
>>> return -ENOSYS;
>>> }
>>> +
>>> +int rtas_suspend_cpu(struct rtas_suspend_me_data *data)
>>> +{
>>> + return -ENOSYS;
>>> +}
>>> +
>>> +int rtas_suspend_last_cpu(struct rtas_suspend_me_data *data)
>>> +{
>>> + return -ENOSYS;
>>> +}
>>
>> Don't see why you need these empty versions, they're only ever called
>> from pseries code aren't they?
>
> True, but we still need to be able to link the code in a cross platform
> kernel.
I was able to fix this up as well with a makefile change to ensure
suspend.c only gets built when CONFIG_PPC_PSERIES=y
--
Brian King
Linux on Power Virtualization
IBM Linux Technology Center
^ permalink raw reply
* [PATCH] need check for devices with bad status status property in __of_scan_bus()
From: Sonny Rao @ 2010-05-11 1:13 UTC (permalink / raw)
To: benh; +Cc: sonnyrao, linuxppc-dev, anton
Hi Ben, we ran into an issue where it looks like we're not
properly ignoring a pci device with a non-good status property
when we walk the device tree and create our device nodes.
However, the EEH init code does look for the property and
disables EEH on these devices. This leaves us in an
inconsistent where we are poking at a supposedly bad
piece of hardware and RTAS will block our config cycles
because EEH isn't enabled anyway.
This has only been compile tested.
Signed-of-by: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
Index: common/arch/powerpc/kernel/pci_of_scan.c
===================================================================
--- common/arch/powerpc/kernel.orig/pci_of_scan.c 2010-05-10 20:00:40.000000000 -0500
+++ common/arch/powerpc/kernel/pci_of_scan.c 2010-05-10 20:03:04.000000000 -0500
@@ -310,6 +310,8 @@ static void __devinit __of_scan_bus(stru
/* Scan direct children */
for_each_child_of_node(node, child) {
pr_debug(" * %s\n", child->full_name);
+ if (!of_device_is_available(child))
+ continue;
reg = of_get_property(child, "reg", ®len);
if (reg == NULL || reglen < 20)
continue;
--
Sonny Rao, LTC OzLabs, BML team
^ permalink raw reply
* [PATCH] powerpc: eeh: Fix oops when probing in early boot
From: Anton Blanchard @ 2010-05-11 1:38 UTC (permalink / raw)
To: benh, linasvepstas, leitao, mmlnx, mikey; +Cc: linuxppc-dev
If we take an EEH early enough, we oops:
Call Trace:
[c000000010483770] [c000000000013ee4] .show_stack+0xd8/0x218 (unreliable)
[c000000010483850] [c000000000658940] .dump_stack+0x28/0x3c
[c0000000104838d0] [c000000000057a68] .eeh_dn_check_failure+0x2b8/0x304
[c000000010483990] [c0000000000259c8] .rtas_read_config+0x120/0x168
[c000000010483a40] [c000000000025af4] .rtas_pci_read_config+0xe4/0x124
[c000000010483af0] [c00000000037af18] .pci_bus_read_config_word+0xac/0x104
[c000000010483bc0] [c0000000008fec98] .pcibios_allocate_resources+0x7c/0x220
[c000000010483c90] [c0000000008feed8] .pcibios_resource_survey+0x9c/0x418
[c000000010483d80] [c0000000008fea10] .pcibios_init+0xbc/0xf4
[c000000010483e20] [c000000000009844] .do_one_initcall+0x98/0x1d8
[c000000010483ed0] [c0000000008f0560] .kernel_init+0x228/0x2e8
[c000000010483f90] [c000000000031a08] .kernel_thread+0x54/0x70
EEH: Detected PCI bus error on device <null>
EEH: This PCI device has failed 1 times in the last hour:
EEH: location=U78A5.001.WIH8464-P1 driver= pci addr=0001:00:01.0
EEH: of node=/pci@800000020000209/usb@1
EEH: PCI device/vendor: 00351033
EEH: PCI cmd/status register: 12100146
Unable to handle kernel paging request for data at address 0x00000468
Oops: Kernel access of bad area, sig: 11 [#1]
....
NIP [c000000000057610] .rtas_set_slot_reset+0x38/0x10c
LR [c000000000058724] .eeh_reset_device+0x5c/0x124
Call Trace:
[c00000000bc6bd00] [c00000000005a0e0] .pcibios_remove_pci_devices+0x7c/0xb0 (unreliable)
[c00000000bc6bd90] [c000000000058724] .eeh_reset_device+0x5c/0x124
[c00000000bc6be40] [c0000000000589c0] .handle_eeh_events+0x1d4/0x39c
[c00000000bc6bf00] [c000000000059124] .eeh_event_handler+0xf0/0x188
[c00000000bc6bf90] [c000000000031a08] .kernel_thread+0x54/0x70
We called rtas_set_slot_reset while scanning the bus and before the pci_dn
to pcidev mapping has been created. Since we only need the pcidev to work
out the type of reset and that only gets set after the module for the
device loads, lets just do a hot reset if the pcidev is NULL.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: linux-2.6/arch/powerpc/platforms/pseries/eeh.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/pseries/eeh.c 2010-05-10 17:25:10.703453565 +1000
+++ linux-2.6/arch/powerpc/platforms/pseries/eeh.c 2010-05-10 17:25:24.034323030 +1000
@@ -749,7 +749,7 @@ static void __rtas_set_slot_reset(struct
/* Determine type of EEH reset required by device,
* default hot reset or fundamental reset
*/
- if (dev->needs_freset)
+ if (dev && dev->needs_freset)
rtas_pci_slot_reset(pdn, 3);
else
rtas_pci_slot_reset(pdn, 1);
^ permalink raw reply
* [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code
From: Anton Blanchard @ 2010-05-11 2:23 UTC (permalink / raw)
To: benh, mikey, michael, miltonm; +Cc: linuxppc-dev
With sparse irqs we have to check if we have a descriptor before dereferencing
it.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
diff --git a/arch/powerpc/kernel/crash.c b/arch/powerpc/kernel/crash.c
index 6f4613d..5182439 100644
--- a/arch/powerpc/kernel/crash.c
+++ b/arch/powerpc/kernel/crash.c
@@ -375,6 +375,9 @@ void default_machine_crash_shutdown(struct pt_regs *regs)
for_each_irq(i) {
struct irq_desc *desc = irq_to_desc(i);
+ if (!desc)
+ continue;
+
if (desc->status & IRQ_INPROGRESS)
desc->chip->eoi(i);
^ permalink raw reply related
* [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU
From: Anton Blanchard @ 2010-05-11 2:25 UTC (permalink / raw)
To: benh, mikey, miltonm, michael, paulus; +Cc: linuxppc-dev
In-Reply-To: <20100511022329.GE12203@kryten>
We wrap the crash_shutdown_handles[] calls with longjmp/setjmp, so if any
of them fault we can recover. The problem is we add a hook to the debugger
fault handler hook which calls longjmp unconditionally.
This first part of kdump is run before we marshall the other CPUs, so there
is a very good chance some CPU on the box is going to page fault. And when
it does it hits the longjmp code and assumes the context of the oopsing CPU.
The machine gets very confused when it has 10 CPUs all with the same stack,
all thinking they have the same CPU id. I get even more confused trying
to debug it.
The patch below adds crash_shutdown_cpu and uses it to specify which cpu is
in the protected region. Since it can only be -1 or the oopsing CPU, we don't
need to use memory barriers since it is only valid on the local CPU - no other
CPU will ever see a value that matches it's local CPU id.
Eventually we should switch the order and marshall all CPUs before doing the
crash_shutdown_handles[] calls, but that is a bigger fix.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: linux-2.6/arch/powerpc/kernel/crash.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/crash.c 2010-05-10 23:48:35.775954185 +1000
+++ linux-2.6/arch/powerpc/kernel/crash.c 2010-05-11 08:39:14.423453660 +1000
@@ -281,10 +281,12 @@ int crash_shutdown_unregister(crash_shut
EXPORT_SYMBOL(crash_shutdown_unregister);
static unsigned long crash_shutdown_buf[JMP_BUF_LEN];
+static int crash_shutdown_cpu = -1;
static int handle_fault(struct pt_regs *regs)
{
- longjmp(crash_shutdown_buf, 1);
+ if (crash_shutdown_cpu == smp_processor_id())
+ longjmp(crash_shutdown_buf, 1);
return 0;
}
@@ -325,6 +327,7 @@ void default_machine_crash_shutdown(stru
*/
old_handler = __debugger_fault_handler;
__debugger_fault_handler = handle_fault;
+ crash_shutdown_cpu = smp_processor_id();
for (i = 0; crash_shutdown_handles[i]; i++) {
if (setjmp(crash_shutdown_buf) == 0) {
/*
@@ -338,6 +341,7 @@ void default_machine_crash_shutdown(stru
asm volatile("sync; isync");
}
}
+ crash_shutdown_cpu = -1;
__debugger_fault_handler = old_handler;
/*
^ permalink raw reply
* [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs
From: Anton Blanchard @ 2010-05-11 2:27 UTC (permalink / raw)
To: benh, mikey, miltonm, michael, paulus; +Cc: linuxppc-dev
In-Reply-To: <20100511022551.GF12203@kryten>
I saw this in a kdump kernel:
IOMMU table initialized, virtual merging enabled
Interrupt 155954 (real) is invalid, disabling it.
Interrupt 155953 (real) is invalid, disabling it.
ie we took some spurious interrupts. default_machine_crash_shutdown tries
to disable all interrupt sources but uses chip->disable which maps to
the default action of:
static void default_disable(unsigned int irq)
{
}
If we use chip->shutdown, then we actually mask the IRQ:
static void default_shutdown(unsigned int irq)
{
struct irq_desc *desc = irq_to_desc(irq);
desc->chip->mask(irq);
desc->status |= IRQ_MASKED;
}
Not sure why we don't implement a ->disable action for xics.c, or why
default_disable doesn't mask the interrupt.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: linux-2.6/arch/powerpc/kernel/crash.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/crash.c 2010-05-10 23:43:10.445953883 +1000
+++ linux-2.6/arch/powerpc/kernel/crash.c 2010-05-10 23:43:21.223454012 +1000
@@ -315,7 +315,7 @@ void default_machine_crash_shutdown(stru
desc->chip->eoi(i);
if (!(desc->status & IRQ_DISABLED))
- desc->chip->disable(i);
+ desc->chip->shutdown(i);
}
/*
^ permalink raw reply
* Re: [RFC][PATCH RFC 10/12] KVM: move dirty bitmaps to user space
From: Marcelo Tosatti @ 2010-05-11 3:28 UTC (permalink / raw)
To: Takuya Yoshikawa
Cc: linux-arch, arnd, kvm, kvm-ia64, fernando, x86, agraf, kvm-ppc,
linux-kernel, yoshikawa.takuya, linuxppc-dev, mingo, paulus, avi,
hpa, tglx
In-Reply-To: <20100504220702.f8ba6ccc.takuya.yoshikawa@gmail.com>
On Tue, May 04, 2010 at 10:07:02PM +0900, Takuya Yoshikawa wrote:
> We move dirty bitmaps to user space.
>
> - Allocation and destruction: we use do_mmap() and do_munmap().
> The new bitmap space is twice longer than the original one and we
> use the additional space for double buffering: this makes it
> possible to update the active bitmap while letting the user space
> read the other one safely. For x86, we can also remove the vmalloc()
> in kvm_vm_ioctl_get_dirty_log().
>
> - Bitmap manipulations: we replace all functions which access dirty
> bitmaps with *_user() functions.
>
> - For ia64: moving the dirty bitmaps of memory slots does not affect
> ia64 much because it's using a different place to store dirty logs
> rather than the dirty bitmaps of memory slots: all we have to change
> are sync and get of dirty log, so we don't need set_bit_user like
> functions for ia64.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> CC: Avi Kivity <avi@redhat.com>
> CC: Alexander Graf <agraf@suse.de>
> ---
> arch/ia64/kvm/kvm-ia64.c | 15 +++++++++-
> arch/powerpc/kvm/book3s.c | 5 +++-
> arch/x86/kvm/x86.c | 25 ++++++++----------
> include/linux/kvm_host.h | 3 +-
> virt/kvm/kvm_main.c | 62 +++++++++++++++++++++++++++++++++++++-------
> 5 files changed, 82 insertions(+), 28 deletions(-)
>
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index 17fd65c..03503e6 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -1823,11 +1823,19 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
> n = kvm_dirty_bitmap_bytes(memslot);
> base = memslot->base_gfn / BITS_PER_LONG;
>
> + r = -EFAULT;
> + if (!access_ok(VERIFY_WRITE, memslot->dirty_bitmap, n))
> + goto out;
> +
> for (i = 0; i < n/sizeof(long); ++i) {
> if (dirty_bitmap[base + i])
> memslot->is_dirty = true;
>
> - memslot->dirty_bitmap[i] = dirty_bitmap[base + i];
> + if (__put_user(dirty_bitmap[base + i],
> + &memslot->dirty_bitmap[i])) {
> + r = -EFAULT;
> + goto out;
> + }
> dirty_bitmap[base + i] = 0;
> }
> r = 0;
> @@ -1858,7 +1866,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> if (memslot->is_dirty) {
> kvm_flush_remote_tlbs(kvm);
> n = kvm_dirty_bitmap_bytes(memslot);
> - memset(memslot->dirty_bitmap, 0, n);
> + if (clear_user(memslot->dirty_bitmap, n)) {
> + r = -EFAULT;
> + goto out;
> + }
> memslot->is_dirty = false;
> }
> r = 0;
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 4b074f1..2a31d2f 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -1210,7 +1210,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> kvmppc_mmu_pte_pflush(vcpu, ga, ga_end);
>
> n = kvm_dirty_bitmap_bytes(memslot);
> - memset(memslot->dirty_bitmap, 0, n);
> + if (clear_user(memslot->dirty_bitmap, n)) {
> + r = -EFAULT;
> + goto out;
> + }
> memslot->is_dirty = false;
> }
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 023c7f8..32a3d94 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2760,40 +2760,37 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> /* If nothing is dirty, don't bother messing with page tables. */
> if (memslot->is_dirty) {
> struct kvm_memslots *slots, *old_slots;
> - unsigned long *dirty_bitmap;
> + unsigned long __user *dirty_bitmap;
> + unsigned long __user *dirty_bitmap_old;
>
> spin_lock(&kvm->mmu_lock);
> kvm_mmu_slot_remove_write_access(kvm, log->slot);
> spin_unlock(&kvm->mmu_lock);
>
> - r = -ENOMEM;
> - dirty_bitmap = vmalloc(n);
> - if (!dirty_bitmap)
> + dirty_bitmap = memslot->dirty_bitmap;
> + dirty_bitmap_old = memslot->dirty_bitmap_old;
> + r = -EFAULT;
> + if (clear_user(dirty_bitmap_old, n))
> goto out;
> - memset(dirty_bitmap, 0, n);
>
> r = -ENOMEM;
> slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
> - if (!slots) {
> - vfree(dirty_bitmap);
> + if (!slots)
> goto out;
> - }
> +
> memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots));
> - slots->memslots[log->slot].dirty_bitmap = dirty_bitmap;
> + slots->memslots[log->slot].dirty_bitmap = dirty_bitmap_old;
> + slots->memslots[log->slot].dirty_bitmap_old = dirty_bitmap;
> slots->memslots[log->slot].is_dirty = false;
>
> old_slots = kvm->memslots;
> rcu_assign_pointer(kvm->memslots, slots);
> synchronize_srcu_expedited(&kvm->srcu);
> - dirty_bitmap = old_slots->memslots[log->slot].dirty_bitmap;
> kfree(old_slots);
>
> r = -EFAULT;
> - if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) {
> - vfree(dirty_bitmap);
> + if (copy_in_user(log->dirty_bitmap, dirty_bitmap, n))
> goto out;
> - }
> - vfree(dirty_bitmap);
> } else {
> r = -EFAULT;
> if (clear_user(log->dirty_bitmap, n))
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0aa6ecb..c95e2b7 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -116,7 +116,8 @@ struct kvm_memory_slot {
> unsigned long npages;
> unsigned long flags;
> unsigned long *rmap;
> - unsigned long *dirty_bitmap;
> + unsigned long __user *dirty_bitmap;
> + unsigned long __user *dirty_bitmap_old;
> bool is_dirty;
> struct {
> unsigned long rmap_pde;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 3e3acad..ddcf65a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -437,8 +437,20 @@ out_err_nodisable:
>
> static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
> {
> - vfree(memslot->dirty_bitmap);
> + unsigned long user_addr;
> + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> +
> + if (!memslot->dirty_bitmap)
> + return;
> +
> + user_addr = min((unsigned long)memslot->dirty_bitmap,
> + (unsigned long)memslot->dirty_bitmap_old);
> + down_write(¤t->mm->mmap_sem);
> + do_munmap(current->mm, user_addr, 2 * n);
> + up_write(¤t->mm->mmap_sem);
> +
> memslot->dirty_bitmap = NULL;
> + memslot->dirty_bitmap_old = NULL;
> }
>
> /*
> @@ -472,8 +484,12 @@ void kvm_free_physmem(struct kvm *kvm)
> int i;
> struct kvm_memslots *slots = kvm->memslots;
>
> - for (i = 0; i < slots->nmemslots; ++i)
> + for (i = 0; i < slots->nmemslots; ++i) {
> + /* VM process will exit: we don't unmap by ourselves. */
> + slots->memslots[i].dirty_bitmap = NULL;
> + slots->memslots[i].dirty_bitmap_old = NULL;
> kvm_free_physmem_slot(&slots->memslots[i], NULL);
> + }
>
> kfree(kvm->memslots);
> }
> @@ -527,14 +543,35 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
>
> static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot)
> {
> - unsigned long dirty_bytes = kvm_dirty_bitmap_bytes(memslot);
> + int err;
> + unsigned long user_addr;
> + unsigned long n = kvm_dirty_bitmap_bytes(memslot);
>
> - memslot->dirty_bitmap = vmalloc(dirty_bytes);
> - if (!memslot->dirty_bitmap)
> - return -ENOMEM;
> + down_write(¤t->mm->mmap_sem);
> + user_addr = do_mmap(NULL, 0, 2 * n,
> + PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, 0);
> + up_write(¤t->mm->mmap_sem);
> +
> + if (IS_ERR((void *)user_addr)) {
> + err = PTR_ERR((void *)user_addr);
> + goto out;
> + }
> +
> + memslot->dirty_bitmap = (unsigned long __user *)user_addr;
> + memslot->dirty_bitmap_old = (unsigned long __user *)(user_addr + n);
> + if (clear_user(memslot->dirty_bitmap, 2 * n)) {
> + err = -EFAULT;
> + goto out_unmap;
> + }
>
> - memset(memslot->dirty_bitmap, 0, dirty_bytes);
> return 0;
> +out_unmap:
> + down_write(¤t->mm->mmap_sem);
> + do_munmap(current->mm, user_addr, 2 * n);
> + up_write(¤t->mm->mmap_sem);
> +out:
> + return err;
> }
>
> /*
> @@ -799,7 +836,7 @@ int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
> n = kvm_dirty_bitmap_bytes(memslot);
>
> r = -EFAULT;
> - if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
> + if (copy_in_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
> goto out;
>
> r = 0;
> @@ -1195,11 +1232,16 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
> gfn = unalias_gfn(kvm, gfn);
> memslot = gfn_to_memslot_unaliased(kvm, gfn);
> if (memslot && memslot->dirty_bitmap) {
> - unsigned long rel_gfn = gfn - memslot->base_gfn;
> + int nr = generic_le_bit_offset(gfn - memslot->base_gfn);
>
> - generic___set_le_bit(rel_gfn, memslot->dirty_bitmap);
> + if (kvm_set_bit_user(nr, memslot->dirty_bitmap))
> + goto out_fault;
mark_page_dirty is called with the mmu_lock spinlock held in set_spte.
Must find a way to move it outside of the spinlock section.
^ permalink raw reply
* Re: [RFC][PATCH 11/12] KVM: introduce new API for getting/switching dirty bitmaps
From: Marcelo Tosatti @ 2010-05-11 3:43 UTC (permalink / raw)
To: Takuya Yoshikawa
Cc: linux-arch, arnd, kvm, kvm-ia64, fernando, x86, agraf, kvm-ppc,
linux-kernel, yoshikawa.takuya, linuxppc-dev, mingo, paulus, avi,
hpa, tglx
In-Reply-To: <20100504220821.d68bde57.takuya.yoshikawa@gmail.com>
On Tue, May 04, 2010 at 10:08:21PM +0900, Takuya Yoshikawa wrote:
> Now that dirty bitmaps are accessible from user space, we export the
> addresses of these to achieve zero-copy dirty log check:
>
> KVM_GET_USER_DIRTY_LOG_ADDR
>
> We also need an API for triggering dirty bitmap switch to take the full
> advantage of double buffered bitmaps.
>
> KVM_SWITCH_DIRTY_LOG
>
> See the documentation in this patch for precise explanations.
>
> About performance improvement: the most important feature of switch API
> is the lightness. In our test, this appeared in the form of improved
> responses for GUI manipulations.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> CC: Avi Kivity <avi@redhat.com>
> CC: Alexander Graf <agraf@suse.de>
> ---
> Documentation/kvm/api.txt | 87 +++++++++++++++++++++++++++++++++++++++++++++
> arch/ia64/kvm/kvm-ia64.c | 27 +++++++++-----
> arch/powerpc/kvm/book3s.c | 18 +++++++--
> arch/x86/kvm/x86.c | 44 ++++++++++++++++-------
> include/linux/kvm.h | 11 ++++++
> include/linux/kvm_host.h | 4 ++-
> virt/kvm/kvm_main.c | 63 +++++++++++++++++++++++++++++----
> 7 files changed, 220 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
> index a237518..c106c83 100644
> --- a/Documentation/kvm/api.txt
> +++ b/Documentation/kvm/api.txt
> @@ -892,6 +892,93 @@ arguments.
> This ioctl is only useful after KVM_CREATE_IRQCHIP. Without an in-kernel
> irqchip, the multiprocessing state must be maintained by userspace.
>
> +4.39 KVM_GET_USER_DIRTY_LOG_ADDR
> +
> +Capability: KVM_CAP_USER_DIRTY_LOG (>=1 see below)
> +Architectures: all
> +Type: vm ioctl
> +Parameters: struct kvm_user_dirty_log (in/out)
> +Returns: 0 on success, -1 on error
> +
> +This ioctl makes it possible to use KVM_SWITCH_DIRTY_LOG (see 4.40) instead
> +of the old dirty log manipulation by KVM_GET_DIRTY_LOG.
> +
> +A bit about KVM_CAP_USER_DIRTY_LOG
> +
> +Before this ioctl was introduced, dirty bitmaps for dirty page logging were
> +allocated in the kernel's memory space. But we have now moved them to user
> +space to get more flexiblity and performance. To achive this move without
> +breaking the compatibility, we will split KVM_CAP_USER_DIRTY_LOG capability
> +into a few generations which can be identified by its check extension
> +return values.
> +
> +This KVM_GET_USER_DIRTY_LOG_ADDR belongs to the first generation with the
> +KVM_SWITCH_DIRTY_LOG (4.40) and must be supported by all generations.
> +
> +What you get
> +
> +By using this, you can get paired bitmap addresses which are accessible from
> +user space. See the explanation in 4.40 for the roles of these two bitmaps.
> +
> +How to Get
> +
> +Before calling this, you have to set the slot member of kvm_user_dirty_log
> +to indicate the target memory slot.
> +
> +struct kvm_user_dirty_log {
> + __u32 slot;
> + __u32 flags;
> + __u64 dirty_bitmap;
> + __u64 dirty_bitmap_old;
> +};
> +
> +The addresses will be set in the paired members: dirty_bitmap and _old.
Why not pass the bitmap address to the kernel, instead of having the
kernel allocate it. Because apparently you plan to do that in a new
generation anyway?
Also, why does the kernel need to know about different bitmaps?
One alternative would be:
KVM_SWITCH_DIRTY_LOG passing the address of a bitmap. If the active
bitmap was clean, it returns 0, no switch performed. If the active
bitmap was dirty, the kernel switches to the new bitmap and returns 1.
And the responsability of cleaning the new bitmap could also be left
for userspace.
^ permalink raw reply
* Re: [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs
From: Michael Ellerman @ 2010-05-11 4:11 UTC (permalink / raw)
To: Anton Blanchard; +Cc: mikey, paulus, miltonm, linuxppc-dev
In-Reply-To: <20100511022738.GG12203@kryten>
[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]
On Tue, 2010-05-11 at 12:27 +1000, Anton Blanchard wrote:
> I saw this in a kdump kernel:
>
> IOMMU table initialized, virtual merging enabled
> Interrupt 155954 (real) is invalid, disabling it.
> Interrupt 155953 (real) is invalid, disabling it.
>
> ie we took some spurious interrupts.
OK, but it should have still worked OK?
> default_machine_crash_shutdown tries
> to disable all interrupt sources but uses chip->disable which maps to
> the default action of:
>
> static void default_disable(unsigned int irq)
> {
> }
>
> If we use chip->shutdown, then we actually mask the IRQ:
>
> static void default_shutdown(unsigned int irq)
> {
> struct irq_desc *desc = irq_to_desc(irq);
>
> desc->chip->mask(irq);
> desc->status |= IRQ_MASKED;
> }
>
> Not sure why we don't implement a ->disable action for xics.c, or why
> default_disable doesn't mask the interrupt.
It used to mask, see 76d21601, I knew that would bite us somewhere.
Not 100% sure about the change to use shutdown, but it's probably sane
and you've tested it so cool :)
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU
From: Michael Ellerman @ 2010-05-11 4:14 UTC (permalink / raw)
To: Anton Blanchard; +Cc: mikey, paulus, miltonm, linuxppc-dev
In-Reply-To: <20100511022551.GF12203@kryten>
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
On Tue, 2010-05-11 at 12:25 +1000, Anton Blanchard wrote:
> We wrap the crash_shutdown_handles[] calls with longjmp/setjmp, so if any
> of them fault we can recover. The problem is we add a hook to the debugger
> fault handler hook which calls longjmp unconditionally.
>
> This first part of kdump is run before we marshall the other CPUs, so there
> is a very good chance some CPU on the box is going to page fault. And when
> it does it hits the longjmp code and assumes the context of the oopsing CPU.
> The machine gets very confused when it has 10 CPUs all with the same stack,
> all thinking they have the same CPU id. I get even more confused trying
> to debug it.
Lol, guess that one didn't get tested that well :)
Fix looks good.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox