* Re: [PATCH 3/3] KVM: PPC: Book3S: Add support for hwrng found on some powernv systems
From: Gleb Natapov @ 2013-10-01 9:57 UTC (permalink / raw)
To: Paul Mackerras
Cc: tytso, kvm, linuxppc-dev, agraf, kvm-ppc, linux-kernel, herbert,
mpm, Paolo Bonzini
In-Reply-To: <20131001092320.GA2000@iris.ozlabs.ibm.com>
On Tue, Oct 01, 2013 at 07:23:20PM +1000, Paul Mackerras wrote:
> On Tue, Oct 01, 2013 at 11:39:08AM +0300, Gleb Natapov wrote:
> > On Tue, Oct 01, 2013 at 06:34:26PM +1000, Michael Ellerman wrote:
> > > On Thu, Sep 26, 2013 at 11:06:59AM +0200, Paolo Bonzini wrote:
> > > > Il 26/09/2013 08:31, Michael Ellerman ha scritto:
> > > > > Some powernv systems include a hwrng. Guests can access it via the
> > > > > H_RANDOM hcall.
> > > >
> > > > Is there any reason to do this in the kernel?
> > >
> > > It's less code, and it's faster :)
> > >
> > > > It does not have to be a particularly fast path;
> > >
> > > Sure, but do we have to make it slow on purpose?
> > >
> > We do not put non performance critical devices into the kernel.
>
> It's not a device, it's a single hypercall, specified by PAPR, which
> is the moral equivalent of x86's RDRAND.
>
OK. A couple of general questions. How guest knows when this hypercall
is available? Is QEMU enable it when KVM_CAP_PPC_HWRNG is available (I
haven't seen userspace patch that uses KVM_CAP_PPC_HWRNG)? What about
QEMU TCG does it implement this hypercall or it emulates hwrng directly?
--
Gleb.
^ permalink raw reply
* Re: [PATCH 3/3] KVM: PPC: Book3S: Add support for hwrng found on some powernv systems
From: Paolo Bonzini @ 2013-10-01 9:58 UTC (permalink / raw)
To: Michael Ellerman
Cc: tytso, herbert, gleb, linux-kernel, kvm-ppc, agraf, linuxppc-dev,
Paul Mackerras, kvm, mpm
In-Reply-To: <20131001083426.GB27484@concordia>
Il 01/10/2013 10:34, Michael Ellerman ha scritto:
>> If you really want to have the hypercall, implementing it in QEMU means
>> that you can support it on all systems, in fact even when running
>> without KVM.
>
> Sure, I can add a fallback to /dev/hwrng for full emulation.
>
>> The QEMU command line would be something like "-object
>> rng-random,filename=/dev/random,id=rng0 -device spapr-rng,rng=rng0".
>
> We can't use /dev/random like that. The PAPR specification, which is
> what we're implementing, implies that H_RANDOM provides data from a
> hardware source.
Then use /dev/hwrng.
I don't have POWER machines, but I still want to be able to test as much
as possible using emulation.
Paolo
^ permalink raw reply
* Re: [PATCH 3/3] KVM: PPC: Book3S: Add support for hwrng found on some powernv systems
From: Benjamin Herrenschmidt @ 2013-10-01 9:38 UTC (permalink / raw)
To: Gleb Natapov
Cc: tytso, kvm, linuxppc-dev, linux-kernel, kvm-ppc, agraf, herbert,
Paul Mackerras, mpm, Paolo Bonzini
In-Reply-To: <20131001083908.GA17294@redhat.com>
On Tue, 2013-10-01 at 11:39 +0300, Gleb Natapov wrote:
> On Tue, Oct 01, 2013 at 06:34:26PM +1000, Michael Ellerman wrote:
> > On Thu, Sep 26, 2013 at 11:06:59AM +0200, Paolo Bonzini wrote:
> > > Il 26/09/2013 08:31, Michael Ellerman ha scritto:
> > > > Some powernv systems include a hwrng. Guests can access it via the
> > > > H_RANDOM hcall.
> > >
> > > Is there any reason to do this in the kernel?
> >
> > It's less code, and it's faster :)
> >
> > > It does not have to be a particularly fast path;
> >
> > Sure, but do we have to make it slow on purpose?
> >
> We do not put non performance critical devices into the kernel.
So for the sake of that dogma you are going to make us do something that
is about 100 times slower ? (and possibly involves more lines of code)
It's not just speed ... H_RANDOM is going to be called by the guest
kernel. A round trip to qemu is going to introduce a kernel jitter
(complete stop of operations of the kernel on that virtual processor) of
a full exit + round trip to qemu + back to the kernel to get to some
source of random number ... this is going to be in the dozens of ns at
least.
This makes no sense.
Ben.
^ permalink raw reply
* [PATCH v2] powerpc/kernel/sysfs: cleanup set up macros for PMC/non-PMC sprs
From: Madhavan Srinivasan @ 2013-10-01 9:23 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, Madhavan Srinivasan
Currently PMC (Performance Monitor Counter) setup macros are used
for other sprs. Since not all sprs are PMC related, this patch
modifies the exisiting macro and uses it to setup both PMC and
non PMC sprs accordingly.
V2 changes:
1) Modified SYSFS_PMCSETUP to a generic macro with additional parameter
2) Added PMC and SPR macro to call the generic macro
3) Changes in the comment to explain better.
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
arch/powerpc/kernel/sysfs.c | 73 +++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 27a90b9..cb971c4 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -107,14 +107,14 @@ void ppc_enable_pmcs(void)
}
EXPORT_SYMBOL(ppc_enable_pmcs);
-#define SYSFS_PMCSETUP(NAME, ADDRESS) \
+#define _SYSFS_SPRSETUP(NAME, ADDRESS, EXTRA) \
static void read_##NAME(void *val) \
{ \
*(unsigned long *)val = mfspr(ADDRESS); \
} \
static void write_##NAME(void *val) \
{ \
- ppc_enable_pmcs(); \
+ EXTRA; \
mtspr(ADDRESS, *(unsigned long *)val); \
} \
static ssize_t show_##NAME(struct device *dev, \
@@ -139,6 +139,11 @@ static ssize_t __used \
return count; \
}
+#define SYSFS_EMPTY
+#define SYSFS_PMCSETUP(NAME, ADDRESS) \
+ _SYSFS_SPRSETUP(NAME, ADDRESS, ppc_enable_pmcs())
+#define SYSFS_SPRSETUP(NAME, ADDRESS) \
+ _SYSFS_SPRSETUP(NAME, ADDRESS, SYSFS_EMPTY)
/* Let's define all possible registers, we'll only hook up the ones
* that are implemented on the current processor
@@ -174,10 +179,10 @@ SYSFS_PMCSETUP(pmc7, SPRN_PMC7);
SYSFS_PMCSETUP(pmc8, SPRN_PMC8);
SYSFS_PMCSETUP(mmcra, SPRN_MMCRA);
-SYSFS_PMCSETUP(purr, SPRN_PURR);
-SYSFS_PMCSETUP(spurr, SPRN_SPURR);
-SYSFS_PMCSETUP(dscr, SPRN_DSCR);
-SYSFS_PMCSETUP(pir, SPRN_PIR);
+SYSFS_SPRSETUP(purr, SPRN_PURR);
+SYSFS_SPRSETUP(spurr, SPRN_SPURR);
+SYSFS_SPRSETUP(dscr, SPRN_DSCR);
+SYSFS_SPRSETUP(pir, SPRN_PIR);
static DEVICE_ATTR(mmcra, 0600, show_mmcra, store_mmcra);
static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
@@ -238,34 +243,34 @@ SYSFS_PMCSETUP(pa6t_pmc3, SPRN_PA6T_PMC3);
SYSFS_PMCSETUP(pa6t_pmc4, SPRN_PA6T_PMC4);
SYSFS_PMCSETUP(pa6t_pmc5, SPRN_PA6T_PMC5);
#ifdef CONFIG_DEBUG_KERNEL
-SYSFS_PMCSETUP(hid0, SPRN_HID0);
-SYSFS_PMCSETUP(hid1, SPRN_HID1);
-SYSFS_PMCSETUP(hid4, SPRN_HID4);
-SYSFS_PMCSETUP(hid5, SPRN_HID5);
-SYSFS_PMCSETUP(ima0, SPRN_PA6T_IMA0);
-SYSFS_PMCSETUP(ima1, SPRN_PA6T_IMA1);
-SYSFS_PMCSETUP(ima2, SPRN_PA6T_IMA2);
-SYSFS_PMCSETUP(ima3, SPRN_PA6T_IMA3);
-SYSFS_PMCSETUP(ima4, SPRN_PA6T_IMA4);
-SYSFS_PMCSETUP(ima5, SPRN_PA6T_IMA5);
-SYSFS_PMCSETUP(ima6, SPRN_PA6T_IMA6);
-SYSFS_PMCSETUP(ima7, SPRN_PA6T_IMA7);
-SYSFS_PMCSETUP(ima8, SPRN_PA6T_IMA8);
-SYSFS_PMCSETUP(ima9, SPRN_PA6T_IMA9);
-SYSFS_PMCSETUP(imaat, SPRN_PA6T_IMAAT);
-SYSFS_PMCSETUP(btcr, SPRN_PA6T_BTCR);
-SYSFS_PMCSETUP(pccr, SPRN_PA6T_PCCR);
-SYSFS_PMCSETUP(rpccr, SPRN_PA6T_RPCCR);
-SYSFS_PMCSETUP(der, SPRN_PA6T_DER);
-SYSFS_PMCSETUP(mer, SPRN_PA6T_MER);
-SYSFS_PMCSETUP(ber, SPRN_PA6T_BER);
-SYSFS_PMCSETUP(ier, SPRN_PA6T_IER);
-SYSFS_PMCSETUP(sier, SPRN_PA6T_SIER);
-SYSFS_PMCSETUP(siar, SPRN_PA6T_SIAR);
-SYSFS_PMCSETUP(tsr0, SPRN_PA6T_TSR0);
-SYSFS_PMCSETUP(tsr1, SPRN_PA6T_TSR1);
-SYSFS_PMCSETUP(tsr2, SPRN_PA6T_TSR2);
-SYSFS_PMCSETUP(tsr3, SPRN_PA6T_TSR3);
+SYSFS_SPRSETUP(hid0, SPRN_HID0);
+SYSFS_SPRSETUP(hid1, SPRN_HID1);
+SYSFS_SPRSETUP(hid4, SPRN_HID4);
+SYSFS_SPRSETUP(hid5, SPRN_HID5);
+SYSFS_SPRSETUP(ima0, SPRN_PA6T_IMA0);
+SYSFS_SPRSETUP(ima1, SPRN_PA6T_IMA1);
+SYSFS_SPRSETUP(ima2, SPRN_PA6T_IMA2);
+SYSFS_SPRSETUP(ima3, SPRN_PA6T_IMA3);
+SYSFS_SPRSETUP(ima4, SPRN_PA6T_IMA4);
+SYSFS_SPRSETUP(ima5, SPRN_PA6T_IMA5);
+SYSFS_SPRSETUP(ima6, SPRN_PA6T_IMA6);
+SYSFS_SPRSETUP(ima7, SPRN_PA6T_IMA7);
+SYSFS_SPRSETUP(ima8, SPRN_PA6T_IMA8);
+SYSFS_SPRSETUP(ima9, SPRN_PA6T_IMA9);
+SYSFS_SPRSETUP(imaat, SPRN_PA6T_IMAAT);
+SYSFS_SPRSETUP(btcr, SPRN_PA6T_BTCR);
+SYSFS_SPRSETUP(pccr, SPRN_PA6T_PCCR);
+SYSFS_SPRSETUP(rpccr, SPRN_PA6T_RPCCR);
+SYSFS_SPRSETUP(der, SPRN_PA6T_DER);
+SYSFS_SPRSETUP(mer, SPRN_PA6T_MER);
+SYSFS_SPRSETUP(ber, SPRN_PA6T_BER);
+SYSFS_SPRSETUP(ier, SPRN_PA6T_IER);
+SYSFS_SPRSETUP(sier, SPRN_PA6T_SIER);
+SYSFS_SPRSETUP(siar, SPRN_PA6T_SIAR);
+SYSFS_SPRSETUP(tsr0, SPRN_PA6T_TSR0);
+SYSFS_SPRSETUP(tsr1, SPRN_PA6T_TSR1);
+SYSFS_SPRSETUP(tsr2, SPRN_PA6T_TSR2);
+SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
#endif /* CONFIG_DEBUG_KERNEL */
#endif /* HAS_PPC_PMC_PA6T */
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 3/3] KVM: PPC: Book3S: Add support for hwrng found on some powernv systems
From: Paul Mackerras @ 2013-10-01 9:23 UTC (permalink / raw)
To: Gleb Natapov
Cc: tytso, kvm, linuxppc-dev, agraf, kvm-ppc, linux-kernel, herbert,
mpm, Paolo Bonzini
In-Reply-To: <20131001083908.GA17294@redhat.com>
On Tue, Oct 01, 2013 at 11:39:08AM +0300, Gleb Natapov wrote:
> On Tue, Oct 01, 2013 at 06:34:26PM +1000, Michael Ellerman wrote:
> > On Thu, Sep 26, 2013 at 11:06:59AM +0200, Paolo Bonzini wrote:
> > > Il 26/09/2013 08:31, Michael Ellerman ha scritto:
> > > > Some powernv systems include a hwrng. Guests can access it via the
> > > > H_RANDOM hcall.
> > >
> > > Is there any reason to do this in the kernel?
> >
> > It's less code, and it's faster :)
> >
> > > It does not have to be a particularly fast path;
> >
> > Sure, but do we have to make it slow on purpose?
> >
> We do not put non performance critical devices into the kernel.
It's not a device, it's a single hypercall, specified by PAPR, which
is the moral equivalent of x86's RDRAND.
Paul.
^ permalink raw reply
* Re: [PATCH] Revert "powerpc: 52xx: provide a default in mpc52xx_irqhost_map()"
From: Wolfram Sang @ 2013-10-01 9:11 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linuxppc-dev, Anatolij Gustschin, linux-rt-users
In-Reply-To: <524A7FCB.3020406@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]
Hi,
On Tue, Oct 01, 2013 at 09:54:51AM +0200, Sebastian Andrzej Siewior wrote:
> On 10/01/2013 09:26 AM, Wolfram Sang wrote:
> > This reverts commit 6391f697d4892a6f233501beea553e13f7745a23. The
> > compiler warning it wants to fix does not appear with my gcc 4.6.2. IMO
> > we don't need superfluous (and here even misleading) code to make old
> > compilers happy. Fixing the printout was bogus, too. We want to know
> > WHICH critical irq failed, not which level it had.
>
> According to minimal Doc*/Changes minimal gcc is 3.2. Mine was 4.3.5.
Well, if you insist, I'd prefer the following patch.
From: Wolfram Sang <wsa@the-dreams.de>
Subject: [PATCH] ppc: mpc52xx: silence false positive from old GCC
So people can compile with -Werror (RT patchset).
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
arch/powerpc/platforms/52xx/mpc52xx_pic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pic.c b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
index b89ef65..ad3c9b0 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_pic.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
@@ -340,7 +340,7 @@ static int mpc52xx_irqhost_map(struct irq_domain *h, unsigned int virq,
{
int l1irq;
int l2irq;
- struct irq_chip *irqchip;
+ struct irq_chip *irqchip = NULL; /* pet old compilers */
void *hndlr;
int type;
u32 reg;
> Why miss leading code? Default here does the same as unhandled and crit
> where it does nothing.
People not realizing 'default' is a no-op might wonder why unknown
levels are mapped to critical.
> Any why do you want to see l2irq since it was
> not in the case statement? l2 holds the number, l1 the level.
We know which level it was, since the printout is only for that level.
We probably want to know which requested IRQ was causing this, so we can
fix the assorted driver. Otherwise we only know that some critical IRQ
was requested somewhere.
> You were but your email bounced. I wasn't aware of this new email
> address you are using now.
Ah, I see, pity.
Thanks,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
From: Michael Ellerman @ 2013-10-01 8:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Madhavan Srinivasan, linuxppc-dev
In-Reply-To: <1380613830.645.18.camel@pasglop>
On Tue, Oct 01, 2013 at 05:50:30PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2013-10-01 at 16:31 +1000, Michael Ellerman wrote:
>
> > > 1)Changed the test for to hypervisor mode instead of platform
> >
> > I think Ben's wrong about that.
> >
> > Almost all existing code uses FW_FEATURE_LPAR to differentiate
> > hypervisor vs guest mode, so I think we should do the same here.
>
> I didn't object to using the FW test, it's a reasonable way to do it, I
> objected to using the platform as an indication (powernv vs. pseries)
Yeah sorry I wasn't clear. You are right that it shouldn't use the
platform. So I think we agree FW_FEATURE is the right way to go.
cheers
^ permalink raw reply
* Re: [PATCH 3/3] KVM: PPC: Book3S: Add support for hwrng found on some powernv systems
From: Gleb Natapov @ 2013-10-01 8:39 UTC (permalink / raw)
To: Michael Ellerman
Cc: tytso, kvm, linuxppc-dev, agraf, kvm-ppc, linux-kernel, herbert,
Paul Mackerras, mpm, Paolo Bonzini
In-Reply-To: <20131001083426.GB27484@concordia>
On Tue, Oct 01, 2013 at 06:34:26PM +1000, Michael Ellerman wrote:
> On Thu, Sep 26, 2013 at 11:06:59AM +0200, Paolo Bonzini wrote:
> > Il 26/09/2013 08:31, Michael Ellerman ha scritto:
> > > Some powernv systems include a hwrng. Guests can access it via the
> > > H_RANDOM hcall.
> >
> > Is there any reason to do this in the kernel?
>
> It's less code, and it's faster :)
>
> > It does not have to be a particularly fast path;
>
> Sure, but do we have to make it slow on purpose?
>
We do not put non performance critical devices into the kernel.
--
Gleb.
^ permalink raw reply
* Re: [PATCH] powerpc/powernv: Reduce panic timeout from 180s to 10s
From: Michael Ellerman @ 2013-10-01 8:39 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev
In-Reply-To: <20130926211719.7b99740a@kryten>
On Thu, Sep 26, 2013 at 09:17:19PM +1000, Anton Blanchard wrote:
>
> We made this change to pseries in 2011 and I think it makes
> sense to do the same on powernv.
I'd vote we set it to 10s for all 64-bit machines in
arch/powerpc/kernel/setup_64.c.
cheers
^ permalink raw reply
* Re: [PATCH 3/3] KVM: PPC: Book3S: Add support for hwrng found on some powernv systems
From: Michael Ellerman @ 2013-10-01 8:36 UTC (permalink / raw)
To: Anshuman Khandual
Cc: tytso, herbert, gleb, agraf, kvm-ppc, linux-kernel, linuxppc-dev,
Paul Mackerras, kvm, mpm, pbonzini
In-Reply-To: <52459311.4010104@linux.vnet.ibm.com>
On Fri, Sep 27, 2013 at 07:45:45PM +0530, Anshuman Khandual wrote:
> On 09/26/2013 12:01 PM, Michael Ellerman wrote:
> > +int powernv_hwrng_present(void)
> > +{
> > + return __raw_get_cpu_var(powernv_rng) != NULL;
> > +}
> > +
> > static unsigned long rng_whiten(struct powernv_rng *rng, unsigned long val)
> > {
> > unsigned long parity;
> > @@ -42,6 +48,17 @@ static unsigned long rng_whiten(struct powernv_rng *rng, unsigned long val)
> > return val;
> > }
> >
> > +int powernv_get_random_real_mode(unsigned long *v)
> > +{
> > + struct powernv_rng *rng;
> > +
> > + rng = __raw_get_cpu_var(powernv_rng);
> > +
> > + *v = rng_whiten(rng, in_rm64(rng->regs_real));
> > +
>
> Will it be in_be64() instead of in_rm64() ? Its failing the build here. Except this
> all individual patches build correctly.
No it's definitely not in_be64() - that will checkstop your machine :)
I added in_rm64() in a previous patch, "Add real mode cache inhibited
IO accessors" - I just didn't want to spam the KVM guys with those
patches as well.
Thanks for the review & testing.
cheers
^ permalink raw reply
* Re: [PATCH 3/3] KVM: PPC: Book3S: Add support for hwrng found on some powernv systems
From: Michael Ellerman @ 2013-10-01 8:34 UTC (permalink / raw)
To: Paolo Bonzini
Cc: tytso, herbert, gleb, linux-kernel, kvm-ppc, agraf, linuxppc-dev,
Paul Mackerras, kvm, mpm
In-Reply-To: <5243F933.7000907@redhat.com>
On Thu, Sep 26, 2013 at 11:06:59AM +0200, Paolo Bonzini wrote:
> Il 26/09/2013 08:31, Michael Ellerman ha scritto:
> > Some powernv systems include a hwrng. Guests can access it via the
> > H_RANDOM hcall.
>
> Is there any reason to do this in the kernel?
It's less code, and it's faster :)
> It does not have to be a particularly fast path;
Sure, but do we have to make it slow on purpose?
> on x86, we are simply forwarding /dev/hwrng or
> /dev/random data to the guest. You can simply use virtio-rng.
Not all guests support virtio-rng.
> If you really want to have the hypercall, implementing it in QEMU means
> that you can support it on all systems, in fact even when running
> without KVM.
Sure, I can add a fallback to /dev/hwrng for full emulation.
> The QEMU command line would be something like "-object
> rng-random,filename=/dev/random,id=rng0 -device spapr-rng,rng=rng0".
We can't use /dev/random like that. The PAPR specification, which is
what we're implementing, implies that H_RANDOM provides data from a
hardware source.
cheers
^ permalink raw reply
* Re: [PATCH 2/3] hwrng: Add a driver for the hwrng found in power7+ systems
From: Michael Ellerman @ 2013-10-01 8:25 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: tytso, kvm, gleb, linuxppc-dev, linux-kernel, kvm-ppc, agraf,
herbert, Paul Mackerras, mpm, pbonzini
In-Reply-To: <1380182487.28561.22.camel@pasglop>
On Thu, Sep 26, 2013 at 06:01:27PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-09-26 at 16:31 +1000, Michael Ellerman wrote:
>
> > + pr_info("registered powernv hwrng.\n");
>
> First letter of a line should get a capital :-) Also since
> it's per-device, at least indicate the OF path or the chip number or
> something ...
It's not the first letter, it looks like this:
powernv-rng: registered powernv hwrng.
Which looks nice to me, but I can make it this if you prefer:
powernv-rng: Registered powernv hwrng.
And it's not per-device due to the trick in the probe routine. It's a
bit sneaky but we need to make it a proper driver for module loading to
work, but we only want to register one hwrng device.
cheers
^ permalink raw reply
* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
From: Madhavan Srinivasan @ 2013-10-01 8:09 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <20131001063129.GF17966@concordia>
On Tuesday 01 October 2013 12:01 PM, Michael Ellerman wrote:
> Hi Maddy,
>
> On Fri, Sep 27, 2013 at 05:03:54PM +0530, Madhavan Srinivasan wrote:
>> powerpc/kernel/sysfs.c exports purr with write permission.
>
> PURR
>
>> This is only valid for kernel in hypervisor mode.
>> But writing to the file in PowerVM lpar causes crash.
>
> In the kernel history/source we refer to it as "phyp". However in this
Will follow the convention.
> case it's not an issue with phyp, it's simply that you are not in
> hypervisor mode, ie. the same crash would occur under KVM.
>
> So you should just say "writing to the file in guest mode .."
>
Sure. Will make the changes.
>> # echo 0 > purr
>> cpu 0x0: Vector: 700 (Program Check) at [c000000000d072b0]
>> pc: c00000000001770c: .write_purr+0x1c/0x40
>> lr: c000000000017708: .write_purr+0x18/0x40
>> sp: c000000000d07530
>> msr: 8000000000049032
>> current = 0xc000000000c53de0
>> paca = 0xc00000000ec70000 softe: 0 irq_happened: 0x01
>> pid = 0, comm = swapper/0
>> enter ? for help
>> [c000000000d075b0] c0000000000fba64
>> .generic_smp_call_function_single_interrupt+0x104/0x190
>> [c000000000d07650] c000000000037748 .smp_ipi_demux+0xa8/0xf0
>> [c000000000d076e0] c000000000035314 .doorbell_exception+0x74/0xb0
>> [c000000000d07760] c000000000002950 doorbell_super_common+0x150/0x180
>> --- Exception: a01 (Doorbell) at c000000000060904
>> .plpar_hcall_norets+0x84/0xd4
>> [link register ] c00000000006dbd4 .check_and_cede_processor+0x24/0x40
>> [c000000000d07a50] c000000001002558 (unreliable)
>> [c000000000d07ac0] c00000000006dd0c .shared_cede_loop+0x2c/0x70
>> [c000000000d07b40] c0000000006ae954 .cpuidle_enter_state+0x64/0x150
>> [c000000000d07c00] c0000000006aeb30 .cpuidle_idle_call+0xf0/0x300
>> [c000000000d07cb0] c000000000062fa0 .pseries_lpar_idle+0x10/0x50
>> [c000000000d07d20] c000000000016d14 .arch_cpu_idle+0x64/0x150
>> [c000000000d07da0] c0000000000e0060 .cpu_startup_entry+0x1a0/0x2c0
>> [c000000000d07e80] c00000000000bca4 .rest_init+0x94/0xb0
>> [c000000000d07ef0] c000000000b54530 .start_kernel+0x478/0x494
>> [c000000000d07f90] c000000000009be0 .start_here_common+0x20/0x40
>> 0:mon>
>>
>> Changes:
>>
>> 1)Changed the test for to hypervisor mode instead of platform
>
> I think Ben's wrong about that.
>
> Almost all existing code uses FW_FEATURE_LPAR to differentiate
> hypervisor vs guest mode, so I think we should do the same here.
>
> So it would be:
>
>> + if (cpu_has_feature(CPU_FTR_PURR)) {
>> + if (!firmware_has_feature(FW_FEATURE_LPAR))
>> + add_write_permission_dev_attr((void *)&dev_attr_purr);
>> device_create_file(s, &dev_attr_purr);
>> + }
>
Will modify the check.
>
>> +static void add_write_permission_dev_attr(void *ptr)
>> +{
>> + struct device_attribute *attr = (struct device_attribute *)ptr;
>> +
>> + attr->attr.mode |= (unsigned short) 0200;
>> +}
>
> Why does it take a void *, which then requires a cast at the call site?
>
just prefered to send the address as void.
> And do you need the cast to short? If so shouldn't you use umode_t
> directly?
No, not really.
Will make the changes and will resend the patch.
>
> cheers
>
Thanks for feedback.
Maddy
^ permalink raw reply
* Re: [PATCH] Revert "powerpc: 52xx: provide a default in mpc52xx_irqhost_map()"
From: Sebastian Andrzej Siewior @ 2013-10-01 7:54 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linuxppc-dev, Anatolij Gustschin, linux-rt-users
In-Reply-To: <1380612366-13504-1-git-send-email-wsa@the-dreams.de>
On 10/01/2013 09:26 AM, Wolfram Sang wrote:
> This reverts commit 6391f697d4892a6f233501beea553e13f7745a23. The
> compiler warning it wants to fix does not appear with my gcc 4.6.2. IMO
> we don't need superfluous (and here even misleading) code to make old
> compilers happy. Fixing the printout was bogus, too. We want to know
> WHICH critical irq failed, not which level it had.
According to minimal Doc*/Changes minimal gcc is 3.2. Mine was 4.3.5.
Why miss leading code? Default here does the same as unhandled and crit
where it does nothing. Any why do you want to see l2irq since it was
not in the case statement? l2 holds the number, l1 the level.
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
>
> Have I been on CC when the original patch was sent?
You were but your email bounced. I wasn't aware of this new email
address you are using now.
>
> arch/powerpc/platforms/52xx/mpc52xx_pic.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pic.c b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
> index b69221b..b89ef65 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_pic.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
> @@ -373,9 +373,8 @@ static int mpc52xx_irqhost_map(struct irq_domain *h, unsigned int virq,
> case MPC52xx_IRQ_L1_PERP: irqchip = &mpc52xx_periph_irqchip; break;
> case MPC52xx_IRQ_L1_SDMA: irqchip = &mpc52xx_sdma_irqchip; break;
> case MPC52xx_IRQ_L1_CRIT:
> - default:
> pr_warn("%s: Critical IRQ #%d is unsupported! Nopping it.\n",
> - __func__, l1irq);
> + __func__, l2irq);
> irq_set_chip(virq, &no_irq_chip);
> return 0;
> }
>
Sebastian
^ permalink raw reply
* Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Michael Ellerman @ 2013-10-01 7:51 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linuxppc-dev, Joerg Roedel, x86@kernel.org,
linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org,
Jan Beulich, linux-pci@vger.kernel.org, Tejun Heo, Bjorn Helgaas,
Ingo Molnar
In-Reply-To: <20130918094759.GA2353@dhcp-26-207.brq.redhat.com>
On Wed, Sep 18, 2013 at 11:48:00AM +0200, Alexander Gordeev wrote:
> On Wed, Sep 18, 2013 at 12:30:23AM +1000, Michael Ellerman wrote:
> > How about no?
> >
> > We have a small number of MSIs available, limited by hardware &
> > firmware, if we don't impose a quota then the first device that probes
> > will get most/all of the MSIs and other devices miss out.
>
> Out of curiosity - how pSeries has had done it without quotas before
> 448e2ca ("powerpc/pseries: Implement a quota system for MSIs")?
>
> > Anyway I don't see what problem you're trying to solve? I agree the
> > -ve/0/+ve return value pattern is ugly, but it's hardly the end of the
> > world.
>
> Well, the interface recently has been re-classified from "ugly" to
> "unnecessarily complex and actively harmful" in Tejun's words ;)
>
> Indeed, I checked most of the drivers and it is incredible how people
> are creative in misusing the interface: from innocent pci_disable_msix()
> calls when if pci_enable_msix() failed to assuming MSI-Xs were enabled
> if pci_enable_msix() returned a positive value (apparently untested).
OK, but we have the source to the drivers, we could just fix them.
We could even add:
pci_enable_msix_i_am_stupid()
Which swallows the positive return and just gives back -ve/0.
> Roughly third of the drivers just do not care and bail out once
> pci_enable_msix() has not succeeded. Not sure how many of these are
> mandated by the hardware.
Sure, that's fine if those drivers do that, it's up to the drivers after
all.
> Another quite common pattern is a call to pci_enable_msix() to figure out
> the number of MSI-Xs available and a repeated call of pci_enable_msix()
> to enable those MSI-Xs, this time.
Also fine, though as the documentation suggests a loop is the best
construct rather than two explicit calls.
> The recommended practice would be:
>
> /*
> * Retrieving 'nvec' by means other than pci_msix_table_size()
> */
>
> rc = pci_get_msix_limit(pdev);
> if (rc < 0)
> return rc;
>
> /*
> * nvec = min(rc, nvec);
> */
>
> for (i = 0; i < nvec; i++)
> msix_entry[i].entry = i;
>
> rc = pci_enable_msix(pdev, msix_entry, nvec);
> if (rc)
> return rc;
>
> Thoughts?
We could probably make that work.
The disadvantage is that any restriction imposed on us above the quota
can only be reported as an error from pci_enable_msix().
The quota code, called from pci_get_msix_limit(), can only do so much to
interogate firmware about the limitations. The ultimate way to check if
firmware will give us enough MSIs is to try and allocate them. But we
can't do that from pci_get_msix_limit() because the driver is not asking
us to enable MSIs, just query them.
You'll also need to add another arch hook, for the quota check, and
we'll have to add it to our per-platform indirection as well.
All a lot of bother for no real gain IMHO.
cheers
^ permalink raw reply
* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
From: Benjamin Herrenschmidt @ 2013-10-01 7:50 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Madhavan Srinivasan, linuxppc-dev
In-Reply-To: <20131001063129.GF17966@concordia>
On Tue, 2013-10-01 at 16:31 +1000, Michael Ellerman wrote:
> > 1)Changed the test for to hypervisor mode instead of platform
>
> I think Ben's wrong about that.
>
> Almost all existing code uses FW_FEATURE_LPAR to differentiate
> hypervisor vs guest mode, so I think we should do the same here.
I didn't object to using the FW test, it's a reasonable way to do it, I
objected to using the platform as an indication (powernv vs. pseries)
Ben.
> So it would be:
>
> > + if (cpu_has_feature(CPU_FTR_PURR)) {
> > + if (!firmware_has_feature(FW_FEATURE_LPAR))
> > + add_write_permission_dev_attr((void *)&dev_attr_purr);
> > device_create_file(s, &dev_attr_purr);
> > + }
>
>
> > +static void add_write_permission_dev_attr(void *ptr)
> > +{
> > + struct device_attribute *attr = (struct device_attribute *)ptr;
> > +
> > + attr->attr.mode |= (unsigned short) 0200;
> > +}
>
> Why does it take a void *, which then requires a cast at the call site?
>
> And do you need the cast to short? If so shouldn't you use umode_t
> directly?
>
> cheers
^ permalink raw reply
* Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Michael Ellerman @ 2013-10-01 7:35 UTC (permalink / raw)
To: Tejun Heo
Cc: Joerg Roedel, x86@kernel.org, linux-kernel@vger.kernel.org,
linux-ide@vger.kernel.org, Alexander Gordeev, Jan Beulich,
linux-pci@vger.kernel.org, Bjorn Helgaas, linuxppc-dev,
Ingo Molnar
In-Reply-To: <20130918142231.GA21650@mtj.dyndns.org>
On Wed, Sep 18, 2013 at 09:22:31AM -0500, Tejun Heo wrote:
> Hello,
>
> On Wed, Sep 18, 2013 at 11:48:00AM +0200, Alexander Gordeev wrote:
> > On Wed, Sep 18, 2013 at 12:30:23AM +1000, Michael Ellerman wrote:
> > > How about no?
> > >
> > > We have a small number of MSIs available, limited by hardware &
> > > firmware, if we don't impose a quota then the first device that probes
> > > will get most/all of the MSIs and other devices miss out.
> >
> > Out of curiosity - how pSeries has had done it without quotas before
> > 448e2ca ("powerpc/pseries: Implement a quota system for MSIs")?
>
> Hmmm... do we need to treat this any differently? If the platform
> can't allocate full range of requested MSIs, just failing should be
> enough regardless of why such allocation can't be met, no?
>
> > > Anyway I don't see what problem you're trying to solve? I agree the
> > > -ve/0/+ve return value pattern is ugly, but it's hardly the end of the
> > > world.
> >
> > Well, the interface recently has been re-classified from "ugly" to
> > "unnecessarily complex and actively harmful" in Tejun's words ;)
>
> LOL. :)
>
> > Indeed, I checked most of the drivers and it is incredible how people
> > are creative in misusing the interface: from innocent pci_disable_msix()
> > calls when if pci_enable_msix() failed to assuming MSI-Xs were enabled
> > if pci_enable_msix() returned a positive value (apparently untested).
> >
> > Roughly third of the drivers just do not care and bail out once
> > pci_enable_msix() has not succeeded. Not sure how many of these are
> > mandated by the hardware.
>
> Yeah, I mean, this type of interface is a trap. People have to
> actively resist to avoid doing silly stuff which is a lot to ask.
I really think you're overstating the complexity here.
Functions typically return a boolean -> nothing to see here
This function returns a tristate value -> brain explosion!
> > /*
> > * Retrieving 'nvec' by means other than pci_msix_table_size()
> > */
> >
> > rc = pci_get_msix_limit(pdev);
> > if (rc < 0)
> > return rc;
> >
> > /*
> > * nvec = min(rc, nvec);
> > */
> >
> > for (i = 0; i < nvec; i++)
> > msix_entry[i].entry = i;
> >
> > rc = pci_enable_msix(pdev, msix_entry, nvec);
> > if (rc)
> > return rc;
>
> I really think what we should do is
>
> * Determine the number of MSIs the controller wants. Don't worry
> about quotas or limits or anything. Just determine the number
> necessary to enable enhanced interrupt handling.
>
> * Try allocating that number of MSIs. If it fails, then just revert
> to single interrupt mode. It's not the end of the world and mostly
> guaranteed to work. Let's please not even try to do partial
> multiple interrupts. I really don't think it's worth the risk or
> complexity.
It will potentially break existing setups on our hardware.
Can I make that any clearer?
cheers
^ permalink raw reply
* Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Michael Ellerman @ 2013-10-01 7:26 UTC (permalink / raw)
To: Tejun Heo
Cc: Joerg Roedel, x86@kernel.org, linux-kernel@vger.kernel.org,
linux-ide@vger.kernel.org, Alexander Gordeev, Jan Beulich,
linux-pci@vger.kernel.org, Bjorn Helgaas, linuxppc-dev,
Ingo Molnar
In-Reply-To: <20130920122603.GC7630@mtj.dyndns.org>
On Fri, Sep 20, 2013 at 07:26:03AM -0500, Tejun Heo wrote:
> Hello,
>
> On Wed, Sep 18, 2013 at 06:50:45PM +0200, Alexander Gordeev wrote:
> > Actually, I do not see much contradiction with what I proposed. The
> > key words here "determine the number of MSIs the controller wants".
> >
> > In general case it is not what pci_msix_table_size() returns (or at
> > least we should not limit ourselves to it) - there could be non-
> > standard means to report number of MSIs: hardcoded, version-dependant,
> > device-specific registers etc.
> >
> > Next, if we opt to determine the number of MSIs by non-MSI standard
> > means then there is no reason not to call pci_get_msix_limit() (or
> > whatever) at this step.
>
> Yeah, that's all fine. My point is that we shouldn't try to use
> "degraded" multiple MSI mode where the number of MSIs allocated is
> smaller than performing full multiple MSI operation. How that number
> is determined doesn't really matter but that number is a property
> which is solely decided by the device driver, right? If a device
> needs full multiple MSI mode, given specific configuration, it needs
> >= X number of MSIs and that's the number it should request.
Sure, the driver is in full control. If it can ONLY work with N MSIs
then it should try for N, else fallback to 1.
But some drivers are able to work with a range of values for N, and
performance is improved vs using a single MSI.
> > Being Captain Obvious here, but it is up to the device driver to handle
> > a failure. There could be no such option as single MSI mode after all :)
>
> I don't think there actually is a mainstream device which can't
> fallback to single interrupt. Anyways, the point is the same, let's
> please not try to create an interface which encourages complex retry
> logic in its users which are likely to involve less traveled and
> tested paths in both the driver and firmware.
Why support > 1 MSI at all? It just adds complex logic and less travelled
paths in the driver and firmware.
cheers
^ permalink raw reply
* [PATCH] Revert "powerpc: 52xx: provide a default in mpc52xx_irqhost_map()"
From: Wolfram Sang @ 2013-10-01 7:26 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sebastian Andrzej Siewior, Anatolij Gustschin, linux-rt-users,
Wolfram Sang
This reverts commit 6391f697d4892a6f233501beea553e13f7745a23. The
compiler warning it wants to fix does not appear with my gcc 4.6.2. IMO
we don't need superfluous (and here even misleading) code to make old
compilers happy. Fixing the printout was bogus, too. We want to know
WHICH critical irq failed, not which level it had.
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Anatolij Gustschin <agust@denx.de>
---
Have I been on CC when the original patch was sent?
arch/powerpc/platforms/52xx/mpc52xx_pic.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pic.c b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
index b69221b..b89ef65 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_pic.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
@@ -373,9 +373,8 @@ static int mpc52xx_irqhost_map(struct irq_domain *h, unsigned int virq,
case MPC52xx_IRQ_L1_PERP: irqchip = &mpc52xx_periph_irqchip; break;
case MPC52xx_IRQ_L1_SDMA: irqchip = &mpc52xx_sdma_irqchip; break;
case MPC52xx_IRQ_L1_CRIT:
- default:
pr_warn("%s: Critical IRQ #%d is unsupported! Nopping it.\n",
- __func__, l1irq);
+ __func__, l2irq);
irq_set_chip(virq, &no_irq_chip);
return 0;
}
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Michael Ellerman @ 2013-10-01 7:19 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-ide@vger.kernel.org, linux-pci@vger.kernel.org,
Joerg Roedel, x86@kernel.org, linux-kernel@vger.kernel.org,
Bjorn Helgaas, Jan Beulich, Tejun Heo, linuxppc-dev, Ingo Molnar
In-Reply-To: <20130926143901.GE16774@dhcp-26-207.brq.redhat.com>
On Thu, Sep 26, 2013 at 04:39:02PM +0200, Alexander Gordeev wrote:
> On Thu, Sep 26, 2013 at 09:11:47AM -0400, Tejun Heo wrote:
> > > Because otherwise we will re-introduce a problem described by Michael:
> > > "We have a small number of MSIs available, limited by hardware &
> > > firmware, if we don't impose a quota then the first device that probes
> > > will get most/all of the MSIs and other devices miss out."
> >
> > Still not following. Why wouldn't just letting the drivers request
> > the optimal number they want and falling back to single interrupt mode
> > work? ie. why can't we just have an all or nothing interface?
>
> I can imagine a scenario where the first device probes in, requests its
> optimal number, acquires that number and exhausts MSIs in pSeries firmware.
> The next few devices possibly end up with single MSI, since no MSIs left
> to satisfy their optimal numbers. If one of those single-MSI'ed devices
> happened to be a high-performance HBA hitting a degraded performance that
> alone would force (IBM) to introduce the quotas.
Yes that's exactly the scenario, and I didn't imagine it, our test
people actually hit it and yelled at me.
I don't remember exactly which adapters it was, I might be able to find
the details if I looked hard, a quick search through my mail archive
didn't find it - it might have come in via irc / bugzilla etc.
cheers
^ permalink raw reply
* Re: [PATCH 2/2] iommu: Update platform initialisation of iommu to use it_page_shift
From: Alistair Popple @ 2013-10-01 7:13 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131001041228.GB17966@concordia>
On Tue, 1 Oct 2013 14:12:29 Michael Ellerman wrote:
> On Tue, Oct 01, 2013 at 01:54:10PM +1000, Alistair Popple wrote:
> > This patch initialises the iommu page size used for vio, cell, powernv
> > and pseries platforms to 4K. It has been boot tested on a pseries
> > machine with vio.
>
> This patch fixes the build errors introduced by the previous patch
> right?
>
> In which case you need to rework it so that you introduce the new
> constants, use them everywhere, before removing the old constants. ie.
> the build should never break.
Thanks, I'll rework and resubmit.
Alistair
> cheers
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* [PATCH] Restore registers on error exit from csum_partial_copy_generic()
From: Anton Blanchard @ 2013-10-01 7:11 UTC (permalink / raw)
To: benh, paulus, paulmck; +Cc: linuxppc-dev
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
The csum_partial_copy_generic() function saves the PowerPC non-volatile
r14, r15, and r16 registers for the main checksum-and-copy loop.
Unfortunately, it fails to restore them upon error exit from this loop,
which results in silent corruption of these registers in the presumably
rare event of an access exception within that loop.
This commit therefore restores these register on error exit from the loop.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: stable@vger.kernel.org
---
Index: b/arch/powerpc/lib/checksum_64.S
===================================================================
--- a/arch/powerpc/lib/checksum_64.S
+++ b/arch/powerpc/lib/checksum_64.S
@@ -226,19 +226,35 @@ _GLOBAL(csum_partial)
blr
- .macro source
+ .macro srcnr
100:
.section __ex_table,"a"
.align 3
- .llong 100b,.Lsrc_error
+ .llong 100b,.Lsrc_error_nr
.previous
.endm
- .macro dest
+ .macro source
+150:
+ .section __ex_table,"a"
+ .align 3
+ .llong 150b,.Lsrc_error
+ .previous
+ .endm
+
+ .macro dstnr
200:
.section __ex_table,"a"
.align 3
- .llong 200b,.Ldest_error
+ .llong 200b,.Ldest_error_nr
+ .previous
+ .endm
+
+ .macro dest
+250:
+ .section __ex_table,"a"
+ .align 3
+ .llong 250b,.Ldest_error
.previous
.endm
@@ -274,11 +290,11 @@ _GLOBAL(csum_partial_copy_generic)
mtctr r6
1:
-source; lhz r6,0(r3) /* align to doubleword */
+srcnr; lhz r6,0(r3) /* align to doubleword */
subi r5,r5,2
addi r3,r3,2
adde r0,r0,r6
-dest; sth r6,0(r4)
+dstnr; sth r6,0(r4)
addi r4,r4,2
bdnz 1b
@@ -392,10 +408,10 @@ dest; std r16,56(r4)
mtctr r6
3:
-source; ld r6,0(r3)
+srcnr; ld r6,0(r3)
addi r3,r3,8
adde r0,r0,r6
-dest; std r6,0(r4)
+dstnr; std r6,0(r4)
addi r4,r4,8
bdnz 3b
@@ -405,10 +421,10 @@ dest; std r6,0(r4)
srdi. r6,r5,2
beq .Lcopy_tail_halfword
-source; lwz r6,0(r3)
+srcnr; lwz r6,0(r3)
addi r3,r3,4
adde r0,r0,r6
-dest; stw r6,0(r4)
+dstnr; stw r6,0(r4)
addi r4,r4,4
subi r5,r5,4
@@ -416,10 +432,10 @@ dest; stw r6,0(r4)
srdi. r6,r5,1
beq .Lcopy_tail_byte
-source; lhz r6,0(r3)
+srcnr; lhz r6,0(r3)
addi r3,r3,2
adde r0,r0,r6
-dest; sth r6,0(r4)
+dstnr; sth r6,0(r4)
addi r4,r4,2
subi r5,r5,2
@@ -427,10 +443,10 @@ dest; sth r6,0(r4)
andi. r6,r5,1
beq .Lcopy_finish
-source; lbz r6,0(r3)
+srcnr; lbz r6,0(r3)
sldi r9,r6,8 /* Pad the byte out to 16 bits */
adde r0,r0,r9
-dest; stb r6,0(r4)
+dstnr; stb r6,0(r4)
.Lcopy_finish:
addze r0,r0 /* add in final carry */
@@ -440,6 +456,11 @@ dest; stb r6,0(r4)
blr
.Lsrc_error:
+ ld r14,STK_REG(R14)(r1)
+ ld r15,STK_REG(R15)(r1)
+ ld r16,STK_REG(R16)(r1)
+ addi r1,r1,STACKFRAMESIZE
+.Lsrc_error_nr:
cmpdi 0,r7,0
beqlr
li r6,-EFAULT
@@ -447,6 +468,11 @@ dest; stb r6,0(r4)
blr
.Ldest_error:
+ ld r14,STK_REG(R14)(r1)
+ ld r15,STK_REG(R15)(r1)
+ ld r16,STK_REG(R16)(r1)
+ addi r1,r1,STACKFRAMESIZE
+.Ldest_error_nr:
cmpdi 0,r8,0
beqlr
li r6,-EFAULT
^ permalink raw reply
* [PATCH] powerpc: Fix parameter clobber in csum_partial_copy_generic()
From: Anton Blanchard @ 2013-10-01 6:54 UTC (permalink / raw)
To: benh, paulus, paulmck; +Cc: linuxppc-dev
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
The csum_partial_copy_generic() uses register r7 to adjust the remaining
bytes to process. Unfortunately, r7 also holds a parameter, namely the
address of the flag to set in case of access exceptions while reading
the source buffer. Lacking a quantum implementation of PowerPC, this
commit instead uses register r9 to do the adjusting, leaving r7's
pointer uncorrupted.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: stable@vger.kernel.org
---
Index: b/arch/powerpc/lib/checksum_64.S
===================================================================
--- a/arch/powerpc/lib/checksum_64.S
+++ b/arch/powerpc/lib/checksum_64.S
@@ -269,8 +269,8 @@ _GLOBAL(csum_partial_copy_generic)
rldicl. r6,r3,64-1,64-2 /* r6 = (r3 & 0x3) >> 1 */
beq .Lcopy_aligned
- li r7,4
- sub r6,r7,r6
+ li r9,4
+ sub r6,r9,r6
mtctr r6
1:
^ permalink raw reply
* Re: [PATCH] powerpc/kernel/sysfs: cleanup set up macros for pmc/non pmc sprs
From: Madhavan Srinivasan @ 2013-10-01 6:44 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <20131001045035.GC17966@concordia>
On Tuesday 01 October 2013 10:20 AM, Michael Ellerman wrote:
> On Mon, Sep 30, 2013 at 04:47:29PM +0530, Madhavan Srinivasan wrote:
>> Currently pmc setup macros are used for non pmc sprs. This patch
>> add new set of macros and cleans up the code to use the new setup macro
>> for non pmc sprs.
>
> Hi Maddy,
>
> Firstly you should use "PMC" not pmc, it's an acronym. You should also
> spell out what it means the first time you use it, eg:
>
> Currently the PMC (Performance Monitor Counter) macros are used ..
>
> Secondly you need to say _why_ it is a bad idea to use the PMC macros
> for non-PMC SPRs.
>
Will change it.
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 27a90b9..73b6f9f 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -139,6 +139,37 @@ static ssize_t __used \
>> return count; \
>> }
>>
>> +#define SYSFS_SPRSETUP(NAME, ADDRESS) \
>> +static void read_##NAME(void *val) \
>> +{ \
>> + *(unsigned long *)val = mfspr(ADDRESS); \
>> +} \
>> +static void write_##NAME(void *val) \
>> +{ \
>> + mtspr(ADDRESS, *(unsigned long *)val); \
>> +} \
>> +static ssize_t show_##NAME(struct device *dev, \
>> + struct device_attribute *attr, \
>> + char *buf) \
>> +{ \
>> + struct cpu *cpu = container_of(dev, struct cpu, dev); \
>> + unsigned long val; \
>> + smp_call_function_single(cpu->dev.id, read_##NAME, &val, 1); \
>> + return sprintf(buf, "%lx\n", val); \
>> +} \
>> +static ssize_t __used \
>> + store_##NAME(struct device *dev, struct device_attribute *attr, \
>> + const char *buf, size_t count) \
>> +{ \
>> + struct cpu *cpu = container_of(dev, struct cpu, dev); \
>> + unsigned long val; \
>> + int ret = sscanf(buf, "%lx", &val); \
>> + if (ret != 1) \
>> + return -EINVAL; \
>> + smp_call_function_single(cpu->dev.id, write_##NAME, &val, 1); \
>> + return count; \
>> +}
>
> This is basically a complete copy of the SYSFS_PMCSETUP() macro, except
> for the one line removal of the call to ppc_enable_pmcs().
>
> You should be able to do better, by defining a macro that does all the
> boiler plate and takes an "extra" argument, which for PMCs is
> "ppc_enable_pmcs()" and for regular SPRs is empty.
>
Sorry about that. Will make the changes.
> cheers
>
Thanks for the feedback
^ permalink raw reply
* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
From: Michael Ellerman @ 2013-10-01 6:31 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: linuxppc-dev
In-Reply-To: <1380281634-9921-1-git-send-email-maddy@linux.vnet.ibm.com>
Hi Maddy,
On Fri, Sep 27, 2013 at 05:03:54PM +0530, Madhavan Srinivasan wrote:
> powerpc/kernel/sysfs.c exports purr with write permission.
PURR
> This is only valid for kernel in hypervisor mode.
> But writing to the file in PowerVM lpar causes crash.
In the kernel history/source we refer to it as "phyp". However in this
case it's not an issue with phyp, it's simply that you are not in
hypervisor mode, ie. the same crash would occur under KVM.
So you should just say "writing to the file in guest mode .."
> # echo 0 > purr
> cpu 0x0: Vector: 700 (Program Check) at [c000000000d072b0]
> pc: c00000000001770c: .write_purr+0x1c/0x40
> lr: c000000000017708: .write_purr+0x18/0x40
> sp: c000000000d07530
> msr: 8000000000049032
> current = 0xc000000000c53de0
> paca = 0xc00000000ec70000 softe: 0 irq_happened: 0x01
> pid = 0, comm = swapper/0
> enter ? for help
> [c000000000d075b0] c0000000000fba64
> .generic_smp_call_function_single_interrupt+0x104/0x190
> [c000000000d07650] c000000000037748 .smp_ipi_demux+0xa8/0xf0
> [c000000000d076e0] c000000000035314 .doorbell_exception+0x74/0xb0
> [c000000000d07760] c000000000002950 doorbell_super_common+0x150/0x180
> --- Exception: a01 (Doorbell) at c000000000060904
> .plpar_hcall_norets+0x84/0xd4
> [link register ] c00000000006dbd4 .check_and_cede_processor+0x24/0x40
> [c000000000d07a50] c000000001002558 (unreliable)
> [c000000000d07ac0] c00000000006dd0c .shared_cede_loop+0x2c/0x70
> [c000000000d07b40] c0000000006ae954 .cpuidle_enter_state+0x64/0x150
> [c000000000d07c00] c0000000006aeb30 .cpuidle_idle_call+0xf0/0x300
> [c000000000d07cb0] c000000000062fa0 .pseries_lpar_idle+0x10/0x50
> [c000000000d07d20] c000000000016d14 .arch_cpu_idle+0x64/0x150
> [c000000000d07da0] c0000000000e0060 .cpu_startup_entry+0x1a0/0x2c0
> [c000000000d07e80] c00000000000bca4 .rest_init+0x94/0xb0
> [c000000000d07ef0] c000000000b54530 .start_kernel+0x478/0x494
> [c000000000d07f90] c000000000009be0 .start_here_common+0x20/0x40
> 0:mon>
>
> Changes:
>
> 1)Changed the test for to hypervisor mode instead of platform
I think Ben's wrong about that.
Almost all existing code uses FW_FEATURE_LPAR to differentiate
hypervisor vs guest mode, so I think we should do the same here.
So it would be:
> + if (cpu_has_feature(CPU_FTR_PURR)) {
> + if (!firmware_has_feature(FW_FEATURE_LPAR))
> + add_write_permission_dev_attr((void *)&dev_attr_purr);
> device_create_file(s, &dev_attr_purr);
> + }
> +static void add_write_permission_dev_attr(void *ptr)
> +{
> + struct device_attribute *attr = (struct device_attribute *)ptr;
> +
> + attr->attr.mode |= (unsigned short) 0200;
> +}
Why does it take a void *, which then requires a cast at the call site?
And do you need the cast to short? If so shouldn't you use umode_t
directly?
cheers
^ 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