* Re: [PATCH 7/7] [v6] drivers/virt: introduce Freescale hypervisor management driver
From: Tabi Timur-B04825 @ 2011-07-01 1:18 UTC (permalink / raw)
To: Arnd Bergmann
Cc: konrad.wilk@oracle.com, linux-kernel@vger.kernel.org,
cmetcalf@tilera.com, akpm@kernel.org, dsaxena@linaro.org,
linux-console@vger.kernel.org, greg@kroah.com, Gala Kumar-B11780,
virtualization@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, alan@lxorguk.ukuu.org.uk
In-Reply-To: <201106092304.55479.arnd@arndb.de>
On Thu, Jun 9, 2011 at 4:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 09 June 2011 22:52:06 Timur Tabi wrote:
>> Add the drivers/virt directory, which houses drivers that support
>> virtualization environments, and add the Freescale hypervisor management
>> driver.
....
>>
>> Signed-off-by: Timur Tabi <timur@freescale.com>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
So I've made the changes that people have asked for, and Arnd has
acked this driver. Who's going to pick it up? Who will be the
maintainer for drivers/virt? I'd really like to see this driver in
3.1, and there's not much time left.
--=20
Timur Tabi
Linux kernel developer at Freescale=
^ permalink raw reply
* [PATCH] powerpc: Use -mtraceback=no
From: Anton Blanchard @ 2011-06-30 23:55 UTC (permalink / raw)
To: benh, linuxppc-dev
gcc 4.7 will be more strict about parsing the -mtraceback option:
gcc: error: unrecognized argument in option '-mtraceback=none'
gcc: note: valid arguments to '-mtraceback=' are: full no part
gcc used to do a 2 char compare so both "no" and "none" would
match. Switch to using -mtraceback=no should work everywhere.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: linux-powerpc/arch/powerpc/Makefile
===================================================================
--- linux-powerpc.orig/arch/powerpc/Makefile 2011-07-01 09:40:57.902255472 +1000
+++ linux-powerpc/arch/powerpc/Makefile 2011-07-01 09:41:13.512533634 +1000
@@ -67,7 +67,7 @@ LDFLAGS_vmlinux-yy := -Bstatic
LDFLAGS_vmlinux-$(CONFIG_PPC64)$(CONFIG_RELOCATABLE) := -pie
LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-yy)
-CFLAGS-$(CONFIG_PPC64) := -mminimal-toc -mtraceback=none -mcall-aixdesc
+CFLAGS-$(CONFIG_PPC64) := -mminimal-toc -mtraceback=no -mcall-aixdesc
CFLAGS-$(CONFIG_PPC32) := -ffixed-r2 -mmultiple
KBUILD_CPPFLAGS += -Iarch/$(ARCH)
KBUILD_AFLAGS += -Iarch/$(ARCH)
^ permalink raw reply
* Re: [RFC PATCH 17/17] KVM: PPC: Add an ioctl for userspace to select which platform to emulate
From: Benjamin Herrenschmidt @ 2011-06-30 23:13 UTC (permalink / raw)
To: Avi Kivity
Cc: kvm, Alexander Graf, kvm-ppc, linuxppc-dev, Paul Mackerras,
Scott Wood
In-Reply-To: <4E0C9339.2080601@redhat.com>
> Regarding that. There's another option - the ioctl code embeds the
> structure size. So if we extend the ioctl parsing to pad up (or
> truncate down) from the user's size to our size, and similarly in the
> other direction, we can get away from this ugliness.
I don't like relying on that much ... I prefer having an explicit
version in the structure head (or use a flag).
> Some years ago I posted a generic helper that did this (and also
> kmalloc'ed and kfree'd the data itself), but it wasn't received
> favourably. Maybe I should try again (and we can possibly use it in kvm
> even if it is rejected for general use, though that's against our
> principles of pushing all generic infrastructure to the wider kernel).
Cheers,
Ben.
^ permalink raw reply
* broken g5?
From: kevin diggs @ 2011-06-30 19:11 UTC (permalink / raw)
To: linuxppc-dev
Hi,
I would like to figure out if my G5 is broken. And if so how to fix it?
As I have previously posted it behaves strangely when using the cpufreq driver.
Anyone have suggestions to figure out what is going on?
Thanks!
kevin
^ permalink raw reply
* Re: [PATCH 2/5] hugetlb: add phys addr to struct huge_bootmem_page
From: Becky Bruce @ 2011-06-30 18:50 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, List linux-kernel@vger.kernel.org Mailing,
David Gibson
In-Reply-To: <1309297166.32158.461.camel@pasglop>
On Jun 28, 2011, at 4:39 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2011-06-28 at 14:54 -0500, Becky Bruce wrote:
>> struct page *alloc_huge_page_node(struct hstate *h, int nid);
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 6402458..2db81ea 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1105,8 +1105,14 @@ static void __init
>> gather_bootmem_prealloc(void)
>> struct huge_bootmem_page *m;
>>=20
>> list_for_each_entry(m, &huge_boot_pages, list) {
>> - struct page *page =3D virt_to_page(m);
>> struct hstate *h =3D m->hstate;
>> +#ifdef CONFIG_HIGHMEM
>> + struct page *page =3D pfn_to_page(m->phys >>
>> PAGE_SHIFT);
>> + free_bootmem_late((unsigned long)m,
>> + sizeof(struct huge_bootmem_page));
>> +#else
>> + struct page *page =3D virt_to_page(m);
>> +#endif
>> __ClearPageReserved(page);
>=20
> Why do you add free_bootmem_late() in the highmem case and not the
> normal case ?
Because there was no bootmem allocation in the normal case - the =
non-highmem version stores data structure in the huge page itself. This =
is perfectly fine as long as you have a mapping. Since this isn't true =
for HIGHMEM pages, I allocate bootmem to store the early data structure =
that stores information about the hugepage (this happens in =
arch-specific code in alloc_bootmem_huge_page).
-Becky
^ permalink raw reply
* Re: [PATCH 2/2] Add cpufreq driver for Momentum Maple boards
From: Dave Jones @ 2011-06-30 18:30 UTC (permalink / raw)
To: kevin diggs
Cc: Dmitry Eremin-Solenikov, Paul Mackerras, linuxppc-dev, cpufreq
In-Reply-To: <BANLkTinciPSnJHHsMo8nfvsYS1AeDov5Tw@mail.gmail.com>
On Thu, Jun 30, 2011 at 01:23:03PM -0500, kevin diggs wrote:
> Hi,
>
> On Wed, Jun 29, 2011 at 3:58 PM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
> >
> > drivers/cpufreq/powerpc. However my current version (as suggested by Ben)
> > goes directly to drivers/cpufreq
> >
> Uh ... Just curious ... why is arch specific code now being put
> outside of the arch directories? When I wrote the 750GX stuff
> (~2.6.28) I put in a location similar to what x86 was doing? When did
> this change?
last release, ARM moved their cpufreq drivers. I moved the x86 ones afterwards.
There's precedent for other arch specific drivers in drivers/ too, but the
cpufreq move is a recent thing.
Dave
^ permalink raw reply
* Re: [PATCH 2/2] Add cpufreq driver for Momentum Maple boards
From: kevin diggs @ 2011-06-30 18:23 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov; +Cc: Paul Mackerras, linuxppc-dev, cpufreq, Dave Jones
In-Reply-To: <BANLkTimjBfMBXEdH7k-6sTjb1P+Fq3-Fvg@mail.gmail.com>
Hi,
On Wed, Jun 29, 2011 at 3:58 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
>
> drivers/cpufreq/powerpc. However my current version (as suggested by Ben)
> goes directly to drivers/cpufreq
>
Uh ... Just curious ... why is arch specific code now being put
outside of the arch directories? When I wrote the 750GX stuff
(~2.6.28) I put in a location similar to what x86 was doing? When did
this change?
kevin
^ permalink raw reply
* Re: [RFC PATCH 17/17] KVM: PPC: Add an ioctl for userspace to select which platform to emulate
From: Alexander Graf @ 2011-06-30 16:33 UTC (permalink / raw)
To: Avi Kivity
Cc: Scott Wood, linuxppc-dev@ozlabs.org, Paul Mackerras,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
In-Reply-To: <4E0C9D98.5000904@redhat.com>
On 30.06.2011, at 18:00, Avi Kivity <avi@redhat.com> wrote:
> On 06/30/2011 06:22 PM, Alexander Graf wrote:
>>> Regarding that. There's another option - the ioctl code embeds the stru=
cture size. So if we extend the ioctl parsing to pad up (or truncate down) f=
rom the user's size to our size, and similarly in the other direction, we ca=
n get away from this ugliness.
>>>=20
>>> Some years ago I posted a generic helper that did this (and also kmalloc=
'ed and kfree'd the data itself), but it wasn't received favourably. Maybe I=
should try again (and we can possibly use it in kvm even if it is rejected f=
or general use, though that's against our principles of pushing all generic i=
nfrastructure to the wider kernel).
>>=20
>>=20
>> That does sound interesting, but requires a lot more thought to be put in=
to the actual code, as we basically need to read out the feature bitmap, the=
n provide a minimum size for the chosen features and then decide if they fit=
in.
>=20
>=20
> Why? just put the things you want in the structure.
>=20
> old userspace -> new kernel: we auto-zero the parts userspace left out, an=
d zero means old behaviour, so everthing works
> new userspace -> old kernel: truncate. Userspace shouldn't have used any n=
ew features (KVM_CAP), and we can -EINVAL if the truncated section contains a=
nonzero bit.
Yup, which requires knowledge in the code on what actually fits :). Logic we=
don't have today.
Alex
^ permalink raw reply
* Re: [PATCH 1/2] mtd/nand : don't free the global data fsl_lbc_ctrl_dev->nand in fsl_elbc_chip_remove()
From: Scott Wood @ 2011-06-30 16:26 UTC (permalink / raw)
To: dedekind1; +Cc: linuxppc-dev, b35362, dwmw2, linux-mtd
In-Reply-To: <1309434797.23597.178.camel@sauron>
On Thu, 30 Jun 2011 14:53:13 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-06-29 at 11:45 -0500, Scott Wood wrote:
> > If we're freeing fsl_lbc_ctrl, we'd better get rid of references to it...
>
> Yes, on the one hand this is a good defensive programming practice, on
> the other hand it hides double-free bugs. Like this patch fixes a
> double-free bug, and why it was noticed before? I thought may be because
> of this NULL assignment?
I'm not sure how the NULL assignment was hiding anything here. It was
probably hidden only because nobody tested it with suitable debug options
enabled since the code was last reorganized.
If the NULL assignment is dropped, consider what happens if the
fsl_elbc_nand module is removed then reinserted. On reinsertion, it will
see a non-NULL fsl_lbc_ctrl_dev->nand, and will skip allocating a new one.
Then you're referencing freed memory.
Looking more closely, the MAX_BANKS loop should be removed. Since the
reorganization, the platform device represents one chip, not the
controller, so we should only be removing that one chip.
-Scott
^ permalink raw reply
* Re: [RFC PATCH 17/17] KVM: PPC: Add an ioctl for userspace to select which platform to emulate
From: Avi Kivity @ 2011-06-30 16:00 UTC (permalink / raw)
To: Alexander Graf; +Cc: Scott Wood, linuxppc-dev, Paul Mackerras, kvm-ppc, kvm
In-Reply-To: <4E0C94A0.3090301@suse.de>
On 06/30/2011 06:22 PM, Alexander Graf wrote:
>> Regarding that. There's another option - the ioctl code embeds the
>> structure size. So if we extend the ioctl parsing to pad up (or
>> truncate down) from the user's size to our size, and similarly in the
>> other direction, we can get away from this ugliness.
>>
>> Some years ago I posted a generic helper that did this (and also
>> kmalloc'ed and kfree'd the data itself), but it wasn't received
>> favourably. Maybe I should try again (and we can possibly use it in
>> kvm even if it is rejected for general use, though that's against our
>> principles of pushing all generic infrastructure to the wider kernel).
>
>
> That does sound interesting, but requires a lot more thought to be put
> into the actual code, as we basically need to read out the feature
> bitmap, then provide a minimum size for the chosen features and then
> decide if they fit in.
Why? just put the things you want in the structure.
old userspace -> new kernel: we auto-zero the parts userspace left out,
and zero means old behaviour, so everthing works
new userspace -> old kernel: truncate. Userspace shouldn't have used
any new features (KVM_CAP), and we can -EINVAL if the truncated section
contains a nonzero bit.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [RFC PATCH 17/17] KVM: PPC: Add an ioctl for userspace to select which platform to emulate
From: Alexander Graf @ 2011-06-30 15:22 UTC (permalink / raw)
To: Avi Kivity; +Cc: Scott Wood, linuxppc-dev, Paul Mackerras, kvm-ppc, kvm
In-Reply-To: <4E0C9339.2080601@redhat.com>
On 06/30/2011 05:16 PM, Avi Kivity wrote:
> On 06/30/2011 06:04 PM, Alexander Graf wrote:
>>> +4.64 KVM_PPC_SET_PLATFORM
>>> +
>>> +Capability: none
>>> +Architectures: powerpc
>>> +Type: vm ioctl
>>> +Parameters: struct kvm_ppc_set_platform (in)
>>> +Returns: 0, or -1 on error
>>> +
>>> +This is used by userspace to tell KVM what sort of platform it should
>>> +emulate. The return value of the ioctl tells userspace whether the
>>> +emulation it is requesting is supported by KVM.
>>> +
>>> +struct kvm_ppc_set_platform {
>>> + __u16 platform; /* defines the OS/hypervisor ABI */
>>> + __u16 guest_arch; /* e.g. decimal 206 for v2.06 */
>>> + __u32 flags;
>>
>>
>> Please add some padding so we can extend it later if necessary.
>
> Regarding that. There's another option - the ioctl code embeds the
> structure size. So if we extend the ioctl parsing to pad up (or
> truncate down) from the user's size to our size, and similarly in the
> other direction, we can get away from this ugliness.
>
> Some years ago I posted a generic helper that did this (and also
> kmalloc'ed and kfree'd the data itself), but it wasn't received
> favourably. Maybe I should try again (and we can possibly use it in
> kvm even if it is rejected for general use, though that's against our
> principles of pushing all generic infrastructure to the wider kernel).
That does sound interesting, but requires a lot more thought to be put
into the actual code, as we basically need to read out the feature
bitmap, then provide a minimum size for the chosen features and then
decide if they fit in.
But yes, that'd still be a lot better than shoving around useless data
all the time :)
Alex
^ permalink raw reply
* Re: [RFC PATCH 17/17] KVM: PPC: Add an ioctl for userspace to select which platform to emulate
From: Avi Kivity @ 2011-06-30 15:16 UTC (permalink / raw)
To: Alexander Graf; +Cc: Scott Wood, linuxppc-dev, Paul Mackerras, kvm-ppc, kvm
In-Reply-To: <4E0C9077.2060608@suse.de>
On 06/30/2011 06:04 PM, Alexander Graf wrote:
>> +4.64 KVM_PPC_SET_PLATFORM
>> +
>> +Capability: none
>> +Architectures: powerpc
>> +Type: vm ioctl
>> +Parameters: struct kvm_ppc_set_platform (in)
>> +Returns: 0, or -1 on error
>> +
>> +This is used by userspace to tell KVM what sort of platform it should
>> +emulate. The return value of the ioctl tells userspace whether the
>> +emulation it is requesting is supported by KVM.
>> +
>> +struct kvm_ppc_set_platform {
>> + __u16 platform; /* defines the OS/hypervisor ABI */
>> + __u16 guest_arch; /* e.g. decimal 206 for v2.06 */
>> + __u32 flags;
>
>
> Please add some padding so we can extend it later if necessary.
Regarding that. There's another option - the ioctl code embeds the
structure size. So if we extend the ioctl parsing to pad up (or
truncate down) from the user's size to our size, and similarly in the
other direction, we can get away from this ugliness.
Some years ago I posted a generic helper that did this (and also
kmalloc'ed and kfree'd the data itself), but it wasn't received
favourably. Maybe I should try again (and we can possibly use it in kvm
even if it is rejected for general use, though that's against our
principles of pushing all generic infrastructure to the wider kernel).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [RFC PATCH 17/17] KVM: PPC: Add an ioctl for userspace to select which platform to emulate
From: Alexander Graf @ 2011-06-30 15:04 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Scott Wood, linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <20110629104103.GR25406@bloggs.ozlabs.ibm.com>
On 06/29/2011 12:41 PM, Paul Mackerras wrote:
> This new ioctl allows userspace to specify what paravirtualization
> interface (if any) KVM should implement, what architecture version
> the guest virtual processors should conform to, and whether the guest
> can be permitted to use a real supervisor mode.
>
> At present the only effect of the ioctl is to indicate whether the
> requested emulation is available, but in future it may be used to
> select between different emulation techniques (book3s_pr vs. book3s_hv)
> or set the CPU compatibility mode for the guest.
>
> If book3s_pr KVM is enabled in the kernel config, then this new
> ioctl accepts platform values of KVM_PPC_PV_NONE and KVM_PPC_PV_KVM,
> but not KVM_PPC_PV_SPAPR. If book3s_hv KVM is enabled, then this
> ioctl requires that the platform is KVM_PPC_PV_SPAPR and the
> guest_arch field contains one of 201 or 206 (for architecture versions
> 2.01 and 2.06) -- when running on a PPC970, it must contain 201, and
> when running on a POWER7, it must contain 206.
>
> Signed-off-by: Paul Mackerras<paulus@samba.org>
> ---
> Documentation/virtual/kvm/api.txt | 35 +++++++++++++++++++++++++++++++++++
> arch/powerpc/include/asm/kvm.h | 15 +++++++++++++++
> arch/powerpc/include/asm/kvm_host.h | 1 +
> arch/powerpc/kvm/powerpc.c | 28 ++++++++++++++++++++++++++++
> include/linux/kvm.h | 1 +
> 5 files changed, 80 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index b0e4b9c..3ab012c 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1430,6 +1430,41 @@ is supported; 2 if the processor requires all virtual machines to have
> an RMA, or 1 if the processor can use an RMA but doesn't require it,
> because it supports the Virtual RMA (VRMA) facility.
>
> +4.64 KVM_PPC_SET_PLATFORM
> +
> +Capability: none
> +Architectures: powerpc
> +Type: vm ioctl
> +Parameters: struct kvm_ppc_set_platform (in)
> +Returns: 0, or -1 on error
> +
> +This is used by userspace to tell KVM what sort of platform it should
> +emulate. The return value of the ioctl tells userspace whether the
> +emulation it is requesting is supported by KVM.
> +
> +struct kvm_ppc_set_platform {
> + __u16 platform; /* defines the OS/hypervisor ABI */
> + __u16 guest_arch; /* e.g. decimal 206 for v2.06 */
> + __u32 flags;
Please add some padding so we can extend it later if necessary.
> +};
> +
> +/* Values for platform */
> +#define KVM_PPC_PV_NONE 0 /* bare-metal, non-paravirtualized */
> +#define KVM_PPC_PV_KVM 1 /* as defined in kvm_para.h */
> +#define KVM_PPC_PV_SPAPR 2 /* IBM Server PAPR (a la PowerVM) */
We also support BookE which would be useful to also include in the list.
Furthermore, KVM is more of a feature flag than a platform. We can
easily support KVM extensions on an SPAPR platform, no?
This whole interface also could deprecate the PVR setting one, so we can
simply include PVR as well and not require kernel space to jump through
hoops to figure out its capabilities.
And we need to identify 32-bit BookS processors, so we can go into
32-bit mode when necessary. That should also be a different guest_arch,
right?
> +
> +/* Values for flags */
> +#define KVM_PPC_CROSS_ARCH 1 /* guest architecture != host */
User space shouldn't have to worry about this one. It's up to the kernel
to decide that it's cross.
Alex
^ permalink raw reply
* Re: [PATCH 1/2] mtd/nand : don't free the global data fsl_lbc_ctrl_dev->nand in fsl_elbc_chip_remove()
From: Artem Bityutskiy @ 2011-06-30 11:53 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, b35362, dwmw2, linux-mtd
In-Reply-To: <20110629114558.13975519@schlenkerla.am.freescale.net>
On Wed, 2011-06-29 at 11:45 -0500, Scott Wood wrote:
> On Wed, 29 Jun 2011 09:20:25 +0300
> Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> > On Tue, 2011-06-28 at 09:50 +0800, b35362@freescale.com wrote:
> > > From: Liu Shuo <b35362@freescale.com>
> > >
> > > The global data fsl_lbc_ctrl_dev->nand don't have to be freed in
> > > fsl_elbc_chip_remove(). The right place to do that is in fsl_elbc_nand_remove()
> > > if elbc_fcm_ctrl->counter is zero.
> > >
> > > Signed-off-by: Liu Shuo <b35362@freescale.com>
> > > ---
> > > drivers/mtd/nand/fsl_elbc_nand.c | 1 -
> > > 1 files changed, 0 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> > > index 0bb254c..a212116 100644
> > > --- a/drivers/mtd/nand/fsl_elbc_nand.c
> > > +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> > > @@ -829,7 +829,6 @@ static int fsl_elbc_chip_remove(struct fsl_elbc_mtd *priv)
> > >
> > > elbc_fcm_ctrl->chips[priv->bank] = NULL;
> > > kfree(priv);
> > > - kfree(elbc_fcm_ctrl);
> > > return 0;
> > > }
> >
> > Do we have to assign fsl_lbc_ctrl_dev->nand to NULL in
> > fsl_elbc_nand_remove() then? I think that assignment can be killed then.
> >
> > if (!elbc_fcm_ctrl->counter) {
> > fsl_lbc_ctrl_dev->nand = NULL;
> > kfree(elbc_fcm_ctrl);
> > }
> >
>
> If we're freeing fsl_lbc_ctrl, we'd better get rid of references to it...
Yes, on the one hand this is a good defensive programming practice, on
the other hand it hides double-free bugs. Like this patch fixes a
double-free bug, and why it was noticed before? I thought may be because
of this NULL assignment?
I do not insist though, that was just a suggestion/question.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply
* Re: [PATCH 2/2] mtd/nand : workaround for Freescale FCM to support large-page Nand chip
From: Artem Bityutskiy @ 2011-06-30 11:51 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, b35362, dwmw2, linux-mtd
In-Reply-To: <20110629114329.154aaef2@schlenkerla.am.freescale.net>
On Wed, 2011-06-29 at 11:43 -0500, Scott Wood wrote:
> On Wed, 29 Jun 2011 09:22:04 +0300
> Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> > On Tue, 2011-06-28 at 09:50 +0800, b35362@freescale.com wrote:
> > > + /* Hack for supporting the flash chip whose writesize is
> > > + * larger than 2K bytes.
> > > + */
> >
> > Please, use proper kernel multi-line comments. Please, make sure
> > checkpatch.pl does not generate 13 errors with this patch.
>
> Most of the checkpatch complaints are about existing style in the file --
> particularly, the use of tabs only for indentation, with spaces used for
> alignment beyond the indentation point.
OK, fair enough, but the multi-line comment is a valid complain,
although minor. And you guys could just fix the style issues as well in
a separate patch-set.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply
* Problem with a driver in Linux-2.6.33
From: MohanReddy koppula @ 2011-06-30 9:20 UTC (permalink / raw)
To: Linuxppc-dev
Hi,
I am writing a device driver for DSP Embedded Line and Port Interface Controller
PEB 20570 (Delic) from infineon. I am using MPC885 custom board. First we wrote
a driver without OS and tested transmission and reception, they are
working fine.
But when I port the same code to the Linux-2.6.33, the chip
initialization is done properly but
transmission and reception are not happening properly. Some times it
receives and transmits the
data but some times it doen't.
Please let me know where could the problem be and how I should
proceed. Is there any open source
driver available for this chip?.
Thanks,
Venkat.
^ permalink raw reply
* Re: [RFC PATCH 17/17] KVM: PPC: Add an ioctl for userspace to select which platform to emulate
From: Avi Kivity @ 2011-06-30 8:34 UTC (permalink / raw)
To: Josh Boyer; +Cc: kvm-ppc, linuxppc-dev, Paul Mackerras, Alexander Graf, kvm
In-Reply-To: <20110629115857.GB17551@zod.rchland.ibm.com>
On 06/29/2011 02:58 PM, Josh Boyer wrote:
> >> This makes me wonder if a similar thing might eventually be usable for
> >> running an i686 or x32 guest on an x86_64 KVM host. I have no idea if
> >> that is even theoretically possible, but if it is it might be better to
> >> rename the ioctl to be architecture agnostic.
> >
> >On x86 this is not required unless we want to "virtualize" pre-CPUID CPUs. Everything as of Pentium has a full bitmap of feature capabilities that KVM gets from user space, including information such as "Can we do 64-bit mode?".
>
> Ah. Thank you for the explanation.
To clarify a bit further, running an i686 guest on an x86_64 host is not
only theoretically possible, but is done regularly. First, x86_64 is
backwards compatible with i686 (so you can install a 32-bit OS on 64-bit
hardware), and second, you can impersonate 32-bit guest hardware on a
64-bit host.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: linux-next: build failure after merge of the final tree (powerpc tree related)
From: Benjamin Herrenschmidt @ 2011-06-30 7:09 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linuxppc-dev, linux-next, Paul Mackerras, linux-kernel
In-Reply-To: <20110630163659.374db7f9.sfr@canb.auug.org.au>
On Thu, 2011-06-30 at 16:36 +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the final tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> drivers/tty/hvc/hvsi.c:701:12: error: conflicting types for 'hvsi_put_chars'
> arch/powerpc/include/asm/hvsi.h:92:12: note: previous declaration of 'hvsi_put_chars' was here
> drivers/tty/hvc/hvsi.c:736:12: error: conflicting types for 'hvsi_open'
> arch/powerpc/include/asm/hvsi.h:86:12: note: previous declaration of 'hvsi_open' was here
> drivers/tty/hvc/hvsi.c:802:13: error: conflicting types for 'hvsi_close'
> arch/powerpc/include/asm/hvsi.h:87:13: note: previous declaration of 'hvsi_close' was here
> drivers/tty/hvc/hvsi.c:1083:19: error: conflicting types for 'hvsi_init'
> arch/powerpc/include/asm/hvsi.h:81:13: note: previous declaration of 'hvsi_init' was here
>
> Caused by commit 17bdc6c0e979 ("powerpc/pseries: Move hvsi support into a
> library").
>
> I have reverted that commit for today.
Ooops, that seems to be entirely my fault, not sure what happened, I
might have merged the wrong patch version (I had some problem locally
with screwing up the git repo where those patches were originally).
I'll push a fix tomorrow.
Cheers,
Ben.
^ permalink raw reply
* Re: [BUG?]3.0-rc4+ftrace+kprobe: set kprobe at instruction 'stwu' lead to system crash/freeze
From: Yong Zhang @ 2011-06-30 7:08 UTC (permalink / raw)
To: ananth
Cc: Jim Keniston, linux-kernel, Steven Rostedt, paulus,
yrl.pp-manager.tt, Masami Hiramatsu, linuxppc-dev
In-Reply-To: <20110629064635.GB678@in.ibm.com>
On Wed, Jun 29, 2011 at 2:46 PM, Ananth N Mavinakayanahalli
<ananth@in.ibm.com> wrote:
>
> Certain functions are off limits for probing -- look for __kprobe
Yup.
> annotations in the kernel. Some such functions are arch specific, but
> show_interrupts() would definitely not be one of them. It works fine on
> my (64bit) test box.
>
> At this time, I think your best bet is to work with the eldk folks to
> narrow down the problem.
I'll give a try :)
> Given the current set of data, I am inclined to
> think it could be an eldk bug, not a kernel one.
Maybe, but the fact is if I don't use kprobe, things works
very well.
I'll be back if there is any update :)
Thanks,
Yong
--
Only stand for myself
^ permalink raw reply
* [PATCH] powerpc: software invalidate TCEs on pseries
From: Michael Neuling @ 2011-06-30 6:58 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, miltonm
From: Milton Miller <miltonm@bga.com>
Some pseries IOMMUs cache TCEs but don't snoop when the TCEs are changed
in memory, hence we need manually invalidate in software.
This adds code to do the invalidate. It keys off a device tree property
to say where the to do the MMIO for the invalidate and some information
on what the format of the invalidate including some magic routing info.
it_busno get overloaded with this magic routing info and it_index with
the MMIO address for the invalidate command.
This then gets hooked into the building and freeing of TCEs.
This is only useful on bare metal pseries. pHyp takes care of this when
virtualised.
Based on patch from Milton with cleanups from Mikey.
Signed-off-by: Milton Miller <miltonm@bga.com>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/tce.h | 8 ++--
arch/powerpc/platforms/pseries/iommu.c | 61 ++++++++++++++++++++++++++++++---
2 files changed, 61 insertions(+), 8 deletions(-)
Index: linux-ozlabs/arch/powerpc/include/asm/tce.h
===================================================================
--- linux-ozlabs.orig/arch/powerpc/include/asm/tce.h
+++ linux-ozlabs/arch/powerpc/include/asm/tce.h
@@ -26,10 +26,12 @@
/*
* Tces come in two formats, one for the virtual bus and a different
- * format for PCI
+ * format for PCI. PCI TCEs can have hardware or software maintianed
+ * coherency.
*/
-#define TCE_VB 0
-#define TCE_PCI 1
+#define TCE_VB 0
+#define TCE_PCI 1
+#define TCE_PCI_SW_INVAL 2
/* TCE page size is 4096 bytes (1 << 12) */
Index: linux-ozlabs/arch/powerpc/platforms/pseries/iommu.c
===================================================================
--- linux-ozlabs.orig/arch/powerpc/platforms/pseries/iommu.c
+++ linux-ozlabs/arch/powerpc/platforms/pseries/iommu.c
@@ -51,13 +51,42 @@
#include "plpar_wrappers.h"
+static void tce_invalidate_pSeries_sw(struct iommu_table *tbl,
+ u64 *startp, u64 *endp)
+{
+ u64 __iomem *invalidate = (u64 __iomem *)tbl->it_index;
+ unsigned long start, end, inc;
+
+ start = __pa(startp);
+ end = __pa(endp);
+ inc = L1_CACHE_BYTES; /* invalidate a cacheline of TCEs at a time */
+
+ /* If this is non-zero, change the format. We shift the
+ * address and or in the magic from the device tree. */
+ if (tbl->it_busno) {
+ start <<= 12;
+ end <<= 12;
+ inc <<= 12;
+ start |= tbl->it_busno;
+ end |= tbl->it_busno;
+ }
+
+ end |= inc - 1; /* round up end to be different than start */
+
+ mb(); /* Make sure TCEs in memory are written */
+ while (start <= end) {
+ out_be64(invalidate, start);
+ start += inc;
+ }
+}
+
static int tce_build_pSeries(struct iommu_table *tbl, long index,
long npages, unsigned long uaddr,
enum dma_data_direction direction,
struct dma_attrs *attrs)
{
u64 proto_tce;
- u64 *tcep;
+ u64 *tcep, *tces;
u64 rpn;
proto_tce = TCE_PCI_READ; // Read allowed
@@ -65,7 +94,7 @@ static int tce_build_pSeries(struct iomm
if (direction != DMA_TO_DEVICE)
proto_tce |= TCE_PCI_WRITE;
- tcep = ((u64 *)tbl->it_base) + index;
+ tces = tcep = ((u64 *)tbl->it_base) + index;
while (npages--) {
/* can't move this out since we might cross MEMBLOCK boundary */
@@ -75,18 +104,24 @@ static int tce_build_pSeries(struct iomm
uaddr += TCE_PAGE_SIZE;
tcep++;
}
+
+ if (tbl->it_type == TCE_PCI_SW_INVAL)
+ tce_invalidate_pSeries_sw(tbl, tces, tcep - 1);
return 0;
}
static void tce_free_pSeries(struct iommu_table *tbl, long index, long npages)
{
- u64 *tcep;
+ u64 *tcep, *tces;
- tcep = ((u64 *)tbl->it_base) + index;
+ tces = tcep = ((u64 *)tbl->it_base) + index;
while (npages--)
*(tcep++) = 0;
+
+ if (tbl->it_type == TCE_PCI_SW_INVAL)
+ tce_invalidate_pSeries_sw(tbl, tces, tcep - 1);
}
static unsigned long tce_get_pseries(struct iommu_table *tbl, long index)
@@ -424,7 +459,7 @@ static void iommu_table_setparms(struct
struct iommu_table *tbl)
{
struct device_node *node;
- const unsigned long *basep;
+ const unsigned long *basep, *sw_inval;
const u32 *sizep;
node = phb->dn;
@@ -461,6 +496,22 @@ static void iommu_table_setparms(struct
tbl->it_index = 0;
tbl->it_blocksize = 16;
tbl->it_type = TCE_PCI;
+
+ sw_inval = of_get_property(node, "linux,tce-sw-invalidate-info", NULL);
+ if (sw_inval) {
+ /*
+ * This property contains information on how to
+ * invalidate the TCE entry. The first property is
+ * the base MMIO address used to invalidate entries.
+ * The second property tells us the format of the TCE
+ * invalidate (whether it needs to be shifted) and
+ * some magic routing info to add to our invalidate
+ * command.
+ */
+ tbl->it_index = (unsigned long) ioremap(sw_inval[0], 8);
+ tbl->it_busno = sw_inval[1]; /* overload this with magic */
+ tbl->it_type = TCE_PCI_SW_INVAL;
+ }
}
/*
^ permalink raw reply
* linux-next: build failure after merge of the final tree (powerpc tree related)
From: Stephen Rothwell @ 2011-06-30 6:36 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev
Cc: linux-next, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]
Hi all,
After merging the final tree, today's linux-next build (powerpc
allyesconfig) failed like this:
drivers/tty/hvc/hvsi.c:701:12: error: conflicting types for 'hvsi_put_chars'
arch/powerpc/include/asm/hvsi.h:92:12: note: previous declaration of 'hvsi_put_chars' was here
drivers/tty/hvc/hvsi.c:736:12: error: conflicting types for 'hvsi_open'
arch/powerpc/include/asm/hvsi.h:86:12: note: previous declaration of 'hvsi_open' was here
drivers/tty/hvc/hvsi.c:802:13: error: conflicting types for 'hvsi_close'
arch/powerpc/include/asm/hvsi.h:87:13: note: previous declaration of 'hvsi_close' was here
drivers/tty/hvc/hvsi.c:1083:19: error: conflicting types for 'hvsi_init'
arch/powerpc/include/asm/hvsi.h:81:13: note: previous declaration of 'hvsi_init' was here
Caused by commit 17bdc6c0e979 ("powerpc/pseries: Move hvsi support into a
library").
I have reverted that commit for today.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* [PATCH v2] powerpc: Add jump label support
From: Benjamin Herrenschmidt @ 2011-06-30 5:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Steven Rostedt
From: Michael Ellerman <michael@ellerman.id.au>
This patch adds support for the new "jump label" feature.
Unlike x86 and sparc we just merrily patch the code with no locks etc,
as far as I know this is safe, but I'm not really sure what the x86/sparc
code is protecting against so maybe it's not.
I also don't see any reason for us to implement the poke_early() routine,
even though sparc does.
[BenH: Updated the patch to upstream generic changes]
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
If no objection I'll stick that in powerpc -next tomorrow, it's been around
for a while, and it seems to just work so ...
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/jump_label.h | 47 +++++++++++++++++++++++++++++++++
arch/powerpc/kernel/Makefile | 1 +
arch/powerpc/kernel/jump_label.c | 23 ++++++++++++++++
4 files changed, 72 insertions(+), 0 deletions(-)
create mode 100644 arch/powerpc/include/asm/jump_label.h
create mode 100644 arch/powerpc/kernel/jump_label.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2729c66..c15f2e6 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -134,6 +134,7 @@ config PPC
select GENERIC_IRQ_SHOW_LEVEL
select HAVE_RCU_TABLE_FREE if SMP
select HAVE_SYSCALL_TRACEPOINTS
+ select HAVE_ARCH_JUMP_LABEL
config EARLY_PRINTK
bool
diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
new file mode 100644
index 0000000..1f780b9
--- /dev/null
+++ b/arch/powerpc/include/asm/jump_label.h
@@ -0,0 +1,47 @@
+#ifndef _ASM_POWERPC_JUMP_LABEL_H
+#define _ASM_POWERPC_JUMP_LABEL_H
+
+/*
+ * Copyright 2010 Michael Ellerman, IBM Corp.
+ *
+ * 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.
+ */
+
+#include <linux/types.h>
+
+#include <asm/feature-fixups.h>
+
+#define JUMP_ENTRY_TYPE stringify_in_c(FTR_ENTRY_LONG)
+#define JUMP_LABEL_NOP_SIZE 4
+
+static __always_inline bool arch_static_branch(struct jump_label_key *key)
+{
+ asm goto("1:\n\t"
+ "nop\n\t"
+ ".pushsection __jump_table, \"aw\"\n\t"
+ ".align 4\n\t"
+ JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
+ ".popsection \n\t"
+ : : "i" (key) : : l_yes);
+ return false;
+l_yes:
+ return true;
+}
+
+#ifdef CONFIG_PPC64
+typedef u64 jump_label_t;
+#else
+typedef u32 jump_label_t;
+#endif
+
+struct jump_entry {
+ jump_label_t code;
+ jump_label_t target;
+ jump_label_t key;
+ jump_label_t pad;
+};
+
+#endif /* _ASM_POWERPC_JUMP_LABEL_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index e8b9818..ce4f7f1 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_MODULES) += module.o module_$(CONFIG_WORD_SIZE).o
obj-$(CONFIG_44x) += cpu_setup_44x.o
obj-$(CONFIG_PPC_FSL_BOOK3E) += cpu_setup_fsl_booke.o dbell.o
obj-$(CONFIG_PPC_BOOK3E_64) += dbell.o
+obj-$(CONFIG_JUMP_LABEL) += jump_label.o
extra-y := head_$(CONFIG_WORD_SIZE).o
extra-$(CONFIG_40x) := head_40x.o
diff --git a/arch/powerpc/kernel/jump_label.c b/arch/powerpc/kernel/jump_label.c
new file mode 100644
index 0000000..368d158
--- /dev/null
+++ b/arch/powerpc/kernel/jump_label.c
@@ -0,0 +1,23 @@
+/*
+ * Copyright 2010 Michael Ellerman, IBM Corp.
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/jump_label.h>
+#include <asm/code-patching.h>
+
+void arch_jump_label_transform(struct jump_entry *entry,
+ enum jump_label_type type)
+{
+ u32 *addr = (u32 *)(unsigned long)entry->code;
+
+ if (type == JUMP_LABEL_ENABLE)
+ patch_branch(addr, entry->target, 0);
+ else
+ patch_instruction(addr, PPC_INST_NOP);
+}
^ permalink raw reply related
* Re: [openmcapi-dev] Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API
From: Benjamin Herrenschmidt @ 2011-06-30 4:19 UTC (permalink / raw)
To: Meador Inge
Cc: openmcapi-dev, Hollis Blanchard, devicetree-discuss, linuxppc-dev
In-Reply-To: <4E0BE7B4.1020901@mentor.com>
On Wed, 2011-06-29 at 22:04 -0500, Meador Inge wrote:
> I posted a more detailed response a few days back:
> http://patchwork.ozlabs.org/patch/98075/. In
> that response, I tried to put forth the rationale
> for allocating the registers statically due to
> the AMP use case. With that in mind, do you
> still disagree with the design? If so, do
> you have any suggestions for how it could be
> better?
No, not really, please repost with my other comments addressed.
Cheers,
Ben.
^ permalink raw reply
* Re: [openmcapi-dev] Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API
From: Meador Inge @ 2011-06-30 3:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: openmcapi-dev, Hollis Blanchard, devicetree-discuss, linuxppc-dev
In-Reply-To: <1308288784.32158.30.camel@pasglop>
On 06/17/2011 12:33 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
>> Some MPIC implementations contain one or more blocks of message registers
>> that are used to send messages between cores via IPIs. A simple API has
>> been added to access (get/put, read, write, etc ...) these message registers.
>> The available message registers are initially discovered via nodes in the
>> device tree. A separate commit contains a binding for the message register
>> nodes.
<snip>
>
> Ok, so we have another scheme of:
>
> - Count all devices in the system of a given type
> - Assign them numbers
> - API uses number
>
> That sucks... unless you have an allocator. And even then.
>
> I'd rather clients use something like struct mpic_msgr (or msg_reg or
> message_reg) as the "handle" to one of these things.
>
> It can be obtained via an allocator or a device tree parsing routine if
> there's a fixed relationship between clients and registers, I don't
> really know how that stuff is to be used, but in any case, the whole
> thing stinks as it is.
Ben,
I posted a more detailed response a few days back:
http://patchwork.ozlabs.org/patch/98075/. In
that response, I tried to put forth the rationale
for allocating the registers statically due to
the AMP use case. With that in mind, do you
still disagree with the design? If so, do
you have any suggestions for how it could be
better?
--
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software
^ permalink raw reply
* Re: [PATCH] powerpc/timebase_read: don't return time older than cycle_last
From: Benjamin Herrenschmidt @ 2011-06-30 0:29 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <20110629141940.2c930411@schlenkerla.am.freescale.net>
On Wed, 2011-06-29 at 14:19 -0500, Scott Wood wrote:
> On Wed, 29 Jun 2011 11:06:36 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > I don't think we ever want to "fix" userspace... how would you "fix" the
> > vDSO gettimeofday implementation for example since the vDSO has no
> > storage ?
>
> Hmm... I guess you could at least make the libc interface monotonic, though
> that wouldn't help if multiple processes are doing some shared memory
> communication.
>
> Ideally, anything that uses timestamps should be robust against small jumps
> backwards (i.e. not blow it up into some huge jump forward or similar
> breakage), because reality just isn't perfect. Especially if you're writing
> userspace code that could run on all sorts of hardware.
Well, powerpc hardware so far has been "perfect" in that regard and I'm
unwilling to bend on that one without throwing a lot of shame at your
face for screwing up :-)
Point is, it's an assumption made by Linux, but also a pile of userspace
code, and I wouldn't be surprised if it extends way beyond just glibc.
So even if it's not spelled out in the PPC_AS (tho it is in PAPR), we
can define it as a Linux requirement :-)
> > We base this assumption on what I believe is an architectural
> > requirement tho of course it's not worded very explicitely, and probably
> > just "derived" from the architecture statement that the timebase can
> > always be used as a monotonic source of time.
>
> Is it making any statement about monotonicity across CPUs?
I think PAPR does actually, I suspect it's been mostly implied and never
written down from a more generic arch perspective.
> > smp-tbsync.c is and has always been a "workaround" for broken HW.
>
> As is this (or for broken firmware, or broken emulation).
>
> > Anybody with half a clue should follow the recommendation of the
> > architecture (this one is actually spelled out, but as a recommendation
> > only) to have a TB enable pin and use it to perform a perfect sync at
> > boot time.
>
> Where in the ISA is this?
In the TB chapter of 2.06
> Closest I see from a quick scan is "There must be a method for getting all
> Time Bases in the system to start incrementing with values that are
> identical or almost identical." (7.2 S, 9.2.1 E). Note the "almost".
>
> And while we do provide this beginning with the e500mc-based chips, e500v2
> isn't dead yet. Kexec is also currently breaking the boot sync, requiring
> smp-tbsync -- though ideally kexec could be reworked to not need to
> physically reset the core.
Right, fixable :-)
> > > We had a bug in U-Boot's timebase sync where the boot core would sometimes
> > > be one tick faster than the other cores.
> >
> > It's scary to think that your cores TBs seem to be soured from different
> > clock sources...
>
> They're not. We use the boot core's timebase for a while, then disable it,
> reset it to zero, and enable all the timebases at once. The bug was a
> missing readback to ensure that it was stopped before it was reset, so
> sometimes it would tick up to 1 on the boot core after reset, before being
> enabled again. This resulted in the boot core's timebase always reading
> one greater than the other cores'.
>
> It can also be an issue when running on simulated hardware.
Ok.
> > ie even if you fix uBoot, can you guarantee they won't
> > drift ? I hope so ...
>
> It shouldn't drift even with the old U-boot -- it's just a constant offset.
That's easier to fix.
> > I would consider that an unfixable architecture
> > violation and I am not at this stage keen on implementing the necessary
> > "workarounds" in Linux (the userspace case is nasty, really nasty).
> >
> > PowerPC always prided itself on having a "sane" time base mechanism
> > unlike x86, please don't tell me that you guys are now breaking that
> > assumption.
>
> If you mean "are we introducing new chips that have timebase problems", no.
>
> I'm questioning whether the assumption was ever fully valid under all
> circumstances.
I think it was, and regardless, the majority of code out there was
written with that assumption.
> > > It's been fixed, but there are
> > > probably people still running the old U-Boot. It seems like the kind of
> > > thing where defensive robustness is called for, like timing out instead of
> > > hanging if a hardware register never flips the bit we're waiting for.
> >
> > No, you'll just "hide" the problem from the kernel and horrible &
> > unexplainable things will happen in userspace. At the VERY LEAST you
> > must warn very loudly if you detect this is happening.
>
> A warning message is OK.
>
> The current situation hides it as well, since it appears to work fine
> until you hit the race, and suddenly things get stuck and it takes a lot of
> digging to find out why.
Yes but you can't hide it completely, so you can't "contain" the damage.
> > > > We make hard assumptions here and in various places actually.
> > >
> > > Are there any in the kernel that this doesn't cover?
> >
> > Check gtod implementation, I'm not sure whether that's enough at this
> > stage or not for it, and then there's the vDSO of course.
>
> I think the in-kernel gtod is OK after this patch (it breaks if it reads a
> timestamp that is less than cycle_last).
>
> The 32-bit vdso looks OK as is since it doesn't convert to nsec until after
> adding to xtime. Userspace will still see time go backwards a bit if the
> timebase does, but it shouldn't get a wildly wrong answer.
But it might calculate one. There's no such thing as a "small problem"
here. If userspace sees the time go backward, even by 1ns, horrible
unpredictable things will happen.
It's either fully correct or not correct at all, and our userspace makes
the assumption that it's always fully correct.
> The 64-bit vdso
> uses srdi instead of sradi, and does so before adding to the upper
> half of xtime, so if the race is hit the returned time will be too high by
> 2^32 seconds.
>
> The problem in the in-kernel gettimeofday that causes it to be sensitive to
> times less than cycles_last appears to be the same thing --
> clocksource_cyc2ns() does an unsigned shift, even though it claims to be
> returning a signed result.
>
> > Not sure what's up with sched_clock() and whether that has similar constraints.
>
> sched_clock() should be OK as long as the timestamp is always greater than
> boot_tb, and hopefully any timebase skew is less than the delay from when
> boot_tb is set to when secondary cores start up.
>
> Delay loops could end early if they use unsigned comparisons, but only if
> the timebase skew is greater than the time it takes to move a thread from
> one cpu to another.
Cheers,
Ben.
^ 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