* Re: [PATCH v6 14/17] net: can: mscan: adjust to common clock support for mpc512x
From: Anatolij Gustschin @ 2013-12-07 9:57 UTC (permalink / raw)
To: Gerhard Sittig
Cc: Marc Kleine-Budde, linuxppc-dev, Wolfgang Grandegger, linux-can
In-Reply-To: <1385851897-23475-15-git-send-email-gsi@denx.de>
On Sat, 30 Nov 2013 23:51:34 +0100
Gerhard Sittig <gsi@denx.de> wrote:
> implement a .get_clock() callback for the MPC512x platform which uses
> the common clock infrastructure (eliminating direct access to the clock
> control registers from within the CAN network driver), and provide the
> corresponding .put_clock() callback to release resources after use
>
> acquire both the clock items for register access ("ipg") as well as for
> wire communication ("can")
>
> keep the previous implementation of MPC512x support in place during
> migration, this results in a readable diff of the change
>
> this change is neutral to the MPC5200 platform
>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: linux-can@vger.kernel.org
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
> drivers/net/can/mscan/mpc5xxx_can.c | 179 +++++++++++++++++++++++++++++++++++
> 1 file changed, 179 insertions(+)
applied to next. Thanks!
Anatolij
^ permalink raw reply
* Re: [PATCH v6 15/17] net: can: mscan: remove non-CCF code for MPC512x
From: Anatolij Gustschin @ 2013-12-07 9:58 UTC (permalink / raw)
To: Gerhard Sittig
Cc: Marc Kleine-Budde, linuxppc-dev, Wolfgang Grandegger, linux-can
In-Reply-To: <1385851897-23475-16-git-send-email-gsi@denx.de>
On Sat, 30 Nov 2013 23:51:35 +0100
Gerhard Sittig <gsi@denx.de> wrote:
> transition to the common clock framework has completed and the PPC_CLOCK
> is no longer available for the MPC512x platform, remove the now obsolete
> code path of the mpc5xxx mscan driver which accessed clock control module
> registers directly
>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: linux-can@vger.kernel.org
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
> drivers/net/can/mscan/mpc5xxx_can.c | 141 -----------------------------------
> 1 file changed, 141 deletions(-)
applied to next. Thanks!
Anatolij
^ permalink raw reply
* Re: [PATCH v6 16/17] powerpc/mpc512x: improve DIU related clock setup
From: Anatolij Gustschin @ 2013-12-07 9:59 UTC (permalink / raw)
To: Gerhard Sittig; +Cc: Scott Wood, linuxppc-dev
In-Reply-To: <1385851897-23475-17-git-send-email-gsi@denx.de>
On Sat, 30 Nov 2013 23:51:36 +0100
Gerhard Sittig <gsi@denx.de> wrote:
> adapt the DIU clock initialization to the COMMON_CLK approach:
> device tree based clock lookup, prepare and unprepare for clocks,
> work with frequencies not dividers, call the appropriate clk_*()
> routines and don't access CCM registers
>
> the "best clock" determination now completely relies on the
> platform's clock driver to pick a frequency close to what the
> caller requests, and merely checks whether the desired frequency
> was met (fits the tolerance of the monitor)
>
> this approach shall succeed upon first try in the usual case,
> will test a few less desirable yet acceptable frequencies in
> edge cases, and will fallback to "best effort" if none of the
> previously tried frequencies pass the test
>
> provide a fallback clock lookup approach in case the OF based clock
> lookup for the DIU fails, this allows for successful operation in
> the presence of an outdated device tree which lacks clock specs
>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
> arch/powerpc/platforms/512x/mpc512x_shared.c | 169 ++++++++++++++------------
> 1 file changed, 92 insertions(+), 77 deletions(-)
applied to next. Thanks!
Anatolij
^ permalink raw reply
* Re: [PATCH] powerpc 8xx: Loading kernels over 8Mbytes without CONFIG_PIN_TLB
From: christophe leroy @ 2013-12-07 9:39 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <1386356996.7375.120.camel@snotra.buserror.net>
Le 06/12/2013 20:09, Scott Wood a écrit :
> On Wed, 2013-11-27 at 12:04 +0100, Christophe Leroy wrote:
>> Today, the only way to load kernels whose size is greater than 8Mbytes is to
>> activate CONFIG_PIN_TLB. Otherwise, the physical memory initially mapped is
>> limited to 8Mbytes. This patch sets up 24 Mbytes of initial memory regardless
>> of whether CONFIG_PIN_TLB is active or not. It allows to load "big" kernels
>> (for instance when activating CONFIG_LOCKDEP_SUPPORT) without having
>> to activate CONFIG_PIN_TLB.
> So, what happens on boards with less than 24M memory present? Even if
> you avoid explicitly referencing those addresses, what if there is a
> speculative access -- or does 8xx not do that?
>
> -Scott
>
Function setup_initial_memory_limit() in mm/init_32.c defines the limits
based on the parameters given by the bootloader.
As far as I know, the 8xx doesn't do speculative access just because an
area is loaded in a TLB Entry.
Christophe
---
Ce courrier électronique ne contient aucun virus ou logiciel malveillant parce que la protection avast! Antivirus est active.
http://www.avast.com
^ permalink raw reply
* Re: [PATCH v6 17/17] clk: mpc512x: remove migration support workarounds
From: Anatolij Gustschin @ 2013-12-07 10:05 UTC (permalink / raw)
To: Gerhard Sittig; +Cc: Mike Turquette, linuxppc-dev
In-Reply-To: <1385851897-23475-18-git-send-email-gsi@denx.de>
On Sat, 30 Nov 2013 23:51:37 +0100
Gerhard Sittig <gsi@denx.de> wrote:
> this change removes workarounds which have become obsolete after
> migration to common clock support has completed
> - remove clkdev registration calls (compatibility clock item aliases)
> after all peripheral drivers were adjusted for device tree based
> clock lookup
> - remove pre-enable workarounds after all peripheral drivers were
> adjusted to acquire their respective clock items
>
> workarounds for these clock items get removed: FEC (ethernet), I2C,
> PSC (UART, SPI), PSC FIFO, USB, NFC (NAND flash), VIU (video capture),
> BDLC (CAN), CAN MCLK, DIU (video output)
>
> these clkdev registered names won't be provided any longer by the
> MPC512x platform's clock driver: "psc%d_mclk", "mscan%d_mclk",
> "usb%d_clk", "nfc_clk", "viu_clk", "sys_clk", "ref_clk"
>
> the pre-enable workaround for PCI remains, but depends on the presence
> of PCI related device tree nodes (disables the PCI clock in the absence
> of PCI nodes, keeps the PCI clock enabled in the presence of nodes) --
> moving clock acquisition into the peripheral driver isn't possible for
> PCI because its initialization takes place before the platform clock
> driver gets initialized, thus the clock provider isn't available then
>
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
> arch/powerpc/platforms/512x/clock-commonclk.c | 50 ++++++++-----------------
> 1 file changed, 16 insertions(+), 34 deletions(-)
applied to next. Thanks!
Anatolij
^ permalink raw reply
* Re: [PATCH] powernv: fix VFIO support with PHB3
From: Thadeu Lima de Souza Cascardo @ 2013-12-07 11:58 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: linuxppc-dev, shangw, paulus, linux-kernel
In-Reply-To: <52A272C4.1060303@ozlabs.ru>
On Sat, Dec 07, 2013 at 11:58:44AM +1100, Alexey Kardashevskiy wrote:
> On 12/06/2013 11:21 PM, Thadeu Lima de Souza Cascardo wrote:
> > I have recently found out that no iommu_groups could be found under
> > /sys/ on a P8. That prevents PCI passthrough from working.
> >
> > During my investigation, I found out there seems to be a missing
> > iommu_register_group for PHB3. The following patch seems to fix the
> > problem. After applying it, I see iommu_groups under
> > /sys/kernel/iommu_groups/, and can also bind vfio-pci to an adapter,
> > which gives me a device at /dev/vfio/.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/platforms/powernv/pci-ioda.c | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> > index 084cdfa..2c6d173 100644
> > --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> > @@ -720,6 +720,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> > tbl->it_type = TCE_PCI_SWINV_CREATE | TCE_PCI_SWINV_FREE;
> > }
> > iommu_init_table(tbl, phb->hose->node);
> > + iommu_register_group(tbl, pci_domain_nr(pe->pbus), pe->pe_number);
> >
> > if (pe->pdev)
> > set_iommu_table_base(&pe->pdev->dev, tbl);
>
>
> This does not seem absolutely right - normally set_iommu_table_base() is
> replaced with set_iommu_table_base_and_group() or you will not see some
> devices in a group and this may make VFIO unhappy.
Alexey,
Your patch "[PATCH v9] PPC: POWERNV: move iommu_add_device earlier" is
not upstream yet. It calls set_iommu_table_base_and_group properly.
However, without calling iommu_register_group, there is no group to add
the device to.
Compare to pnv_pci_ioda_setup_dma_pe which calls iommu_register_group
too. My patch fixes this discrepancy between IODA and IODA2.
Regards.
Cascardo.
>
> But - if every single device gets assigned to some group, then we can push
> this to Frobisher and let people test in on power8.
>
>
>
> --
> Alexey
^ permalink raw reply
* questions: second of the 2 pcie controllers does not scan the bus.
From: Ruchika @ 2013-12-07 13:26 UTC (permalink / raw)
Cc: linuxppc-dev
In-Reply-To: <52A2706A.7030205@servergy.com>
Any ideas/pointers anyone ?
Thank you
On 12/6/2013 6:48 PM, Ruchika wrote:
>
> Hi,
> I am working with an p4080 based board. I am trying to get 2 PCIE
> controllers probed properly.
>
> In uboot I have no problems scanning and discovering what is connected
> to both controllers/PCI bridges.
>
> For both PCIE1/2 uboot sets up the Primary, secondary and Subordinate
> bus numbers to 0,1,1 respectively.
>
> When linux boots up and probes the controllers, PCIE1 is probed and the
> bridge scanned properly but PCIE2 is probed at the bridge but not
> attempted a scan.
> I see this message
> "pci 0001:02:00.0: bridge configuration invalid ([bus 01-01]),
> reconfiguring
> "
>
> I updated uboot to set the secondary and subordinate numbers to 2 (left
> the primary number to 0) and a subsequent kernel boot scanned the bus
> for PCIE2 successfully.
> I found these numbers to be very critical since the device tree blob
> (bus-range) for pci is also based off these.
>
> I'd like to get a good fix rather than the uboot hack and get better
> understanding of the problem. If there are any pointers someone could
> provide it would be awesome.
>
> Thank you
> Regards
> Ruchika
>
>
>
^ permalink raw reply
* Re: [PATCH] powerpc: Fix PTE page address mismatch in pgtable ctor/dtor
From: Hong H. Pham @ 2013-12-07 14:05 UTC (permalink / raw)
To: Aneesh Kumar K.V, linux-rt-users, linuxppc-dev
Cc: Paul Mackerras, linux-stable
In-Reply-To: <52A1F804.5000205@windriver.com>
There is a typo in the v2 patch, please disregard it. The v3 patch with
the correction will follow.
Thanks,
Hong
On 12/06/2013 11:15 AM, Hong H. Pham wrote:
> Hi Aneesh,
>
> On 12/06/2013 05:38 AM, Aneesh Kumar K.V wrote:
>
>> can you also specifiy the config details here. ie, 4K page size functions
>> are broken ?
>
> My PPC64 config has SMP and 4K page size enabled. I re-tested with 64K page size,
> and the problem is not present.
>
> I have added a note that this problem affects only PPC32 and PPC64 SMP kernels.
> On PPC64, the problem is limited to 4K page size.
>
>> make it closer to what it was before,
>>
>> pgtable_page_dtor(table);
>> pgtable_free_tlb(tlb, page_address(table), 0);
>>
>> This is what we had before
>>
>> -static inline void __pte_free_tlb(struct mmu_gather *tlb, struct page *ptepage,
>> - unsigned long address)
>> -{
>> - tlb_flush_pgtable(tlb, address);
>> - pgtable_page_dtor(ptepage);
>> - pgtable_free_tlb(tlb, page_address(ptepage), 0);
>> -}
>>
>>
>> -aneesh
>>
>
> Done.
>
> Thanks,
> Hong
^ permalink raw reply
* [PATCH v3] powerpc: Fix PTE page address mismatch in pgtable ctor/dtor
From: Hong H. Pham @ 2013-12-07 14:06 UTC (permalink / raw)
To: linux-rt-users, linuxppc-dev
Cc: Paul Mackerras, Hong H. Pham, Aneesh Kumar K.V, linux-stable
In-Reply-To: <874n6muuw4.fsf@linux.vnet.ibm.com>
From: "Hong H. Pham" <hong.pham@windriver.com>
In pte_alloc_one(), pgtable_page_ctor() is passed an address that has
not been converted by page_address() to the newly allocated PTE page.
When the PTE is freed, __pte_free_tlb() calls pgtable_page_dtor()
with an address to the PTE page that has been converted by page_address().
The mismatch in the PTE's page address causes pgtable_page_dtor() to access
invalid memory, so resources for that PTE (such as the page lock) is not
properly cleaned up.
On PPC32, only SMP kernels are affected.
On PPC64, only SMP kernels with 4K page size are affected.
This bug was introduced by commit d614bb041209fd7cb5e4b35e11a7b2f6ee8f62b8
"powerpc: Move the pte free routines from common header".
On a preempt-rt kernel, a spinlock is dynamically allocated for each
PTE in pgtable_page_ctor(). When the PTE is freed, calling
pgtable_page_dtor() with a mismatched page address causes a memory leak,
as the pointer to the PTE's spinlock is bogus.
On mainline, there isn't any immediately obvious symptoms, but the
problem still exists here.
Fixes: d614bb041209fd7c "powerpc: Move the pte free routes from common header"
Cc: Paul Mackerras <paulus@samba.org>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linux-stable <stable@vger.kernel.org> # v3.10+
Signed-off-by: Hong H. Pham <hong.pham@windriver.com>
---
arch/powerpc/include/asm/pgalloc-32.h | 6 ++----
arch/powerpc/include/asm/pgalloc-64.h | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/pgalloc-32.h b/arch/powerpc/include/asm/pgalloc-32.h
index 27b2386..842846c 100644
--- a/arch/powerpc/include/asm/pgalloc-32.h
+++ b/arch/powerpc/include/asm/pgalloc-32.h
@@ -84,10 +84,8 @@ static inline void pgtable_free_tlb(struct mmu_gather *tlb,
static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t table,
unsigned long address)
{
- struct page *page = page_address(table);
-
tlb_flush_pgtable(tlb, address);
- pgtable_page_dtor(page);
- pgtable_free_tlb(tlb, page, 0);
+ pgtable_page_dtor(table);
+ pgtable_free_tlb(tlb, page_address(table), 0);
}
#endif /* _ASM_POWERPC_PGALLOC_32_H */
diff --git a/arch/powerpc/include/asm/pgalloc-64.h b/arch/powerpc/include/asm/pgalloc-64.h
index f65e27b..256d6f8 100644
--- a/arch/powerpc/include/asm/pgalloc-64.h
+++ b/arch/powerpc/include/asm/pgalloc-64.h
@@ -144,11 +144,9 @@ static inline void pgtable_free_tlb(struct mmu_gather *tlb,
static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t table,
unsigned long address)
{
- struct page *page = page_address(table);
-
tlb_flush_pgtable(tlb, address);
- pgtable_page_dtor(page);
- pgtable_free_tlb(tlb, page, 0);
+ pgtable_page_dtor(table);
+ pgtable_free_tlb(tlb, page_address(table), 0);
}
#else /* if CONFIG_PPC_64K_PAGES */
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH v3] powerpc: Fix PTE page address mismatch in pgtable ctor/dtor
From: Benjamin Herrenschmidt @ 2013-12-07 20:27 UTC (permalink / raw)
To: Hong H. Pham
Cc: Paul Mackerras, linuxppc-dev, linux-rt-users, linux-stable,
Aneesh Kumar K.V
In-Reply-To: <1386425193-24015-1-git-send-email-hong.pham@windriver.com>
On Sat, 2013-12-07 at 09:06 -0500, Hong H. Pham wrote:
> diff --git a/arch/powerpc/include/asm/pgalloc-32.h b/arch/powerpc/include/asm/pgalloc-32.h
> index 27b2386..842846c 100644
> --- a/arch/powerpc/include/asm/pgalloc-32.h
> +++ b/arch/powerpc/include/asm/pgalloc-32.h
> @@ -84,10 +84,8 @@ static inline void pgtable_free_tlb(struct mmu_gather *tlb,
> static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t table,
> unsigned long address)
> {
> - struct page *page = page_address(table);
> -
> tlb_flush_pgtable(tlb, address);
> - pgtable_page_dtor(page);
> - pgtable_free_tlb(tlb, page, 0);
> + pgtable_page_dtor(table);
> + pgtable_free_tlb(tlb, page_address(table), 0);
> }
Ok so your description of the problem confused me a bit, but I see that
in the !64K page, pgtable_t is already a struct page so yes, the
page_address() call here is bogus.
However, I also noticed that in the 64k page case, we don't call the dto
at all. Is that a problem ?
Also, Aneesh, shouldn't we just fix the disconnect here and have
pgtable_t always be the same type ? The way this is now is confusing
and error prone...
> #endif /* _ASM_POWERPC_PGALLOC_32_H */
> diff --git a/arch/powerpc/include/asm/pgalloc-64.h b/arch/powerpc/include/asm/pgalloc-64.h
> index f65e27b..256d6f8 100644
> --- a/arch/powerpc/include/asm/pgalloc-64.h
> +++ b/arch/powerpc/include/asm/pgalloc-64.h
> @@ -144,11 +144,9 @@ static inline void pgtable_free_tlb(struct mmu_gather *tlb,
> static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t table,
> unsigned long address)
> {
> - struct page *page = page_address(table);
> -
> tlb_flush_pgtable(tlb, address);
> - pgtable_page_dtor(page);
> - pgtable_free_tlb(tlb, page, 0);
> + pgtable_page_dtor(table);
> + pgtable_free_tlb(tlb, page_address(table), 0);
> }
>
> #else /* if CONFIG_PPC_64K_PAGES */
Ben.
^ permalink raw reply
* Re: [PATCH v3] powerpc: Fix PTE page address mismatch in pgtable ctor/dtor
From: Benjamin Herrenschmidt @ 2013-12-07 20:32 UTC (permalink / raw)
To: Hong H. Pham
Cc: linuxppc-dev, Paul Mackerras, linux-rt-users, linux-stable,
Aneesh Kumar K.V
In-Reply-To: <1386448079.21910.105.camel@pasglop>
On Sun, 2013-12-08 at 07:27 +1100, Benjamin Herrenschmidt wrote:
> On Sat, 2013-12-07 at 09:06 -0500, Hong H. Pham wrote:
>
> > diff --git a/arch/powerpc/include/asm/pgalloc-32.h b/arch/powerpc/include/asm/pgalloc-32.h
> > index 27b2386..842846c 100644
> > --- a/arch/powerpc/include/asm/pgalloc-32.h
> > +++ b/arch/powerpc/include/asm/pgalloc-32.h
> > @@ -84,10 +84,8 @@ static inline void pgtable_free_tlb(struct mmu_gather *tlb,
> > static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t table,
> > unsigned long address)
> > {
> > - struct page *page = page_address(table);
> > -
> > tlb_flush_pgtable(tlb, address);
> > - pgtable_page_dtor(page);
> > - pgtable_free_tlb(tlb, page, 0);
> > + pgtable_page_dtor(table);
> > + pgtable_free_tlb(tlb, page_address(table), 0);
> > }
>
> Ok so your description of the problem confused me a bit, but I see that
> in the !64K page, pgtable_t is already a struct page so yes, the
> page_address() call here is bogus.
>
> However, I also noticed that in the 64k page case, we don't call the dto
> at all. Is that a problem ?
Actually we do, just elsewhere... ignore the above.
> Also, Aneesh, shouldn't we just fix the disconnect here and have
> pgtable_t always be the same type ? The way this is now is confusing
> and error prone...
>
> > #endif /* _ASM_POWERPC_PGALLOC_32_H */
> > diff --git a/arch/powerpc/include/asm/pgalloc-64.h b/arch/powerpc/include/asm/pgalloc-64.h
> > index f65e27b..256d6f8 100644
> > --- a/arch/powerpc/include/asm/pgalloc-64.h
> > +++ b/arch/powerpc/include/asm/pgalloc-64.h
> > @@ -144,11 +144,9 @@ static inline void pgtable_free_tlb(struct mmu_gather *tlb,
> > static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t table,
> > unsigned long address)
> > {
> > - struct page *page = page_address(table);
> > -
> > tlb_flush_pgtable(tlb, address);
> > - pgtable_page_dtor(page);
> > - pgtable_free_tlb(tlb, page, 0);
> > + pgtable_page_dtor(table);
> > + pgtable_free_tlb(tlb, page_address(table), 0);
> > }
> >
> > #else /* if CONFIG_PPC_64K_PAGES */
>
> Ben.
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH] powerpc 8xx: Loading kernels over 8Mbytes without CONFIG_PIN_TLB
From: Benjamin Herrenschmidt @ 2013-12-07 20:58 UTC (permalink / raw)
To: christophe leroy; +Cc: Scott Wood, linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <52A2ECE4.3050407@c-s.fr>
On Sat, 2013-12-07 at 10:39 +0100, christophe leroy wrote:
> Function setup_initial_memory_limit() in mm/init_32.c defines the
> limits
> based on the parameters given by the bootloader.
> As far as I know, the 8xx doesn't do speculative access just because
> an area is loaded in a TLB Entry.
Speculative accesses are ... speculative :-) The address used for such
an access can be anything really. So yes, architecturally, powerpc
processors can access *anything* speculatively just because there's a
valid non-garded translation.
Whether the 8xx does it at all, I don't know. 44x originally did but
that was so buggy that we had to force G on all mappings (until later
versions of the core just burned the feature out). Pretty much all other
powerpc's do it.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect
From: Benjamin Herrenschmidt @ 2013-12-09 0:31 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Paul Mackerras, linuxppc-dev, Liu Ping Fan
In-Reply-To: <87a9gfva9n.fsf@linux.vnet.ibm.com>
On Thu, 2013-12-05 at 16:23 +0530, Aneesh Kumar K.V wrote:
> Liu Ping Fan <kernelfans@gmail.com> writes:
>
> > To enable the do_numa_page(), we should not fix _PAGE_NUMA in
> > hash_page(), so bail out for the case of pte_numa().
For some reason I don't have 2/3 and 3/3 in my mbox (though I do have
them on patchwork) so I'll reply to this one.
Overall, your statement that this is a faster path needs to be backed up
with numbers.
The code is complicated enough as it-is, such additional mess in the low
level hashing code requires a good justification, and also a
demonstration that it doesn't add overhead to the normal hash path.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH V4 00/10] perf: New conditional branch filter
From: Michael Ellerman @ 2013-12-09 0:41 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, peterz, mingo
Cc: mikey, ak, linux-kernel, eranian, linuxppc-dev, sukadev,
Anshuman Khandual
In-Reply-To: <20131206131817.GB2279@ghostprotocols.net>
On Fri, 2013-12-06 at 10:18 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 05, 2013 at 03:47:54PM +1100, Michael Ellerman escreveu:
> > On Wed, 2013-12-04 at 16:02 +0530, Anshuman Khandual wrote:
> > > This patchset is the re-spin of the original branch stack sampling
> > > patchset which introduced new PERF_SAMPLE_BRANCH_COND branch filter. This patchset
> > > also enables SW based branch filtering support for book3s powerpc platforms which
> > > have PMU HW backed branch stack sampling support.
> > >
> > > Summary of code changes in this patchset:
> > >
> > > (1) Introduces a new PERF_SAMPLE_BRANCH_COND branch filter
> > > (2) Add the "cond" branch filter options in the "perf record" tool
> > > (3) Enable PERF_SAMPLE_BRANCH_COND in X86 platforms
> > > (4) Enable PERF_SAMPLE_BRANCH_COND in POWER8 platform
> > > (5) Update the documentation regarding "perf record" tool
> >
> >
> > Hi Arnaldo,
> >
> > Can you please take just patches 1-5 into the perf tree? And do you mind
> > putting them in a topic branch so Benh can merge that.
>
> This is mostly kernel code, I process the userspace ones, so I think either
> Ingo or PeterZ should pick these, Ingo, Peter?
Urgh, sorry. MAINTAINERS just lists all of you in a block.
Added PeterZ to CC.
Peter/Ingo can you please take just patches 1-5 into the perf tree? And
do you mind putting them in a topic branch so Benh can merge that.
The generic & x86 changes have a Reviewed-by from Stephane, and the change to
tools/perf has an ack-of-sorts from Arnaldo:
> Only:
>
> Subject: [PATCH V4 03/10] perf, tool: Conditional branch filter 'cond' added to perf record
>
> Which is a one liner, touches tools/perf/, and I'm ok with it.
cheers
^ permalink raw reply
* [PATCH] drivers/tty: ehv_bytechan fails to build as a module
From: Anton Blanchard @ 2013-12-09 5:03 UTC (permalink / raw)
To: timur, gregkh, benh, paulus, dvaleev; +Cc: linuxppc-dev
ehv_bytechan is marked tristate but fails to build as a module:
drivers/tty/ehv_bytechan.c:363:1: error: type defaults to =E2=80=98int=E2=
=80=99 in declaration of =E2=80=98console_initcall=E2=80=99 [-Werror=3Dimpl=
icit-int]
It doesn't make much sense for a console driver to be built as
a module, so change it to a bool.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: b/drivers/tty/Kconfig
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -366,7 +366,7 @@ config TRACE_SINK
"Trace data router for MIPI P1149.7 cJTAG standard".
=20
config PPC_EPAPR_HV_BYTECHAN
- tristate "ePAPR hypervisor byte channel driver"
+ bool "ePAPR hypervisor byte channel driver"
depends on PPC
select EPAPR_PARAVIRT
help
^ permalink raw reply
* [PATCH][RESEND] powerpc: remove unused REDBOOT Kconfig parameter
From: Michael Opdenacker @ 2013-12-09 5:27 UTC (permalink / raw)
To: benh, paulus, galak, vitb, marcelo
Cc: Michael Opdenacker, linuxppc-dev, linux-kernel
In-Reply-To: <1383554336-3829-1-git-send-email-michael.opdenacker@free-electrons.com>
This removes the REDBOOT Kconfig parameter,
which was no longer used anywhere in the source code
and Makefiles.
Signed-off-by: Michael Opdenacker <michael.opdenacker@free-electrons.com>
---
arch/powerpc/Kconfig | 3 ---
arch/powerpc/platforms/83xx/Kconfig | 1 -
arch/powerpc/platforms/8xx/Kconfig | 1 -
3 files changed, 5 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index b44b52c0a8f0..70dc283050b5 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -209,9 +209,6 @@ config DEFAULT_UIMAGE
Used to allow a board to specify it wants a uImage built by default
default n
-config REDBOOT
- bool
-
config ARCH_HIBERNATION_POSSIBLE
bool
default y
diff --git a/arch/powerpc/platforms/83xx/Kconfig b/arch/powerpc/platforms/83xx/Kconfig
index 670a033264c0..2bdc8c862c46 100644
--- a/arch/powerpc/platforms/83xx/Kconfig
+++ b/arch/powerpc/platforms/83xx/Kconfig
@@ -99,7 +99,6 @@ config SBC834x
config ASP834x
bool "Analogue & Micro ASP 834x"
select PPC_MPC834x
- select REDBOOT
help
This enables support for the Analogue & Micro ASP 83xx
board.
diff --git a/arch/powerpc/platforms/8xx/Kconfig b/arch/powerpc/platforms/8xx/Kconfig
index 8dec3c0911ad..bd6f1a1cf922 100644
--- a/arch/powerpc/platforms/8xx/Kconfig
+++ b/arch/powerpc/platforms/8xx/Kconfig
@@ -45,7 +45,6 @@ config PPC_EP88XC
config PPC_ADDER875
bool "Analogue & Micro Adder 875"
select CPM1
- select REDBOOT
help
This enables support for the Analogue & Micro Adder 875
board.
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect
From: Liu ping fan @ 2013-12-09 6:17 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul Mackerras, linuxppc-dev, Aneesh Kumar K.V
In-Reply-To: <1386549112.5159.4.camel@pasglop>
On Mon, Dec 9, 2013 at 8:31 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2013-12-05 at 16:23 +0530, Aneesh Kumar K.V wrote:
>> Liu Ping Fan <kernelfans@gmail.com> writes:
>>
>> > To enable the do_numa_page(), we should not fix _PAGE_NUMA in
>> > hash_page(), so bail out for the case of pte_numa().
>
> For some reason I don't have 2/3 and 3/3 in my mbox (though I do have
> them on patchwork) so I'll reply to this one.
>
> Overall, your statement that this is a faster path needs to be backed up
> with numbers.
>
> The code is complicated enough as it-is, such additional mess in the low
> level hashing code requires a good justification, and also a
> demonstration that it doesn't add overhead to the normal hash path.
>
For the test, is it ok to have an user application to copy page where
all page are PG_mlocked?
Thanks and regards,
Pingfan
^ permalink raw reply
* Re: [PATCH][RESEND] powerpc: remove unused REDBOOT Kconfig parameter
From: Benjamin Herrenschmidt @ 2013-12-09 6:17 UTC (permalink / raw)
To: Michael Opdenacker; +Cc: marcelo, linux-kernel, paulus, linuxppc-dev
In-Reply-To: <1386566860-19785-1-git-send-email-michael.opdenacker@free-electrons.com>
On Mon, 2013-12-09 at 06:27 +0100, Michael Opdenacker wrote:
> This removes the REDBOOT Kconfig parameter,
> which was no longer used anywhere in the source code
> and Makefiles.
It hasn't been lost :-) It's still in patchwork and it's even in my
queue.
Cheers,
Ben.
> Signed-off-by: Michael Opdenacker <michael.opdenacker@free-electrons.com>
> ---
> arch/powerpc/Kconfig | 3 ---
> arch/powerpc/platforms/83xx/Kconfig | 1 -
> arch/powerpc/platforms/8xx/Kconfig | 1 -
> 3 files changed, 5 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index b44b52c0a8f0..70dc283050b5 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -209,9 +209,6 @@ config DEFAULT_UIMAGE
> Used to allow a board to specify it wants a uImage built by default
> default n
>
> -config REDBOOT
> - bool
> -
> config ARCH_HIBERNATION_POSSIBLE
> bool
> default y
> diff --git a/arch/powerpc/platforms/83xx/Kconfig b/arch/powerpc/platforms/83xx/Kconfig
> index 670a033264c0..2bdc8c862c46 100644
> --- a/arch/powerpc/platforms/83xx/Kconfig
> +++ b/arch/powerpc/platforms/83xx/Kconfig
> @@ -99,7 +99,6 @@ config SBC834x
> config ASP834x
> bool "Analogue & Micro ASP 834x"
> select PPC_MPC834x
> - select REDBOOT
> help
> This enables support for the Analogue & Micro ASP 83xx
> board.
> diff --git a/arch/powerpc/platforms/8xx/Kconfig b/arch/powerpc/platforms/8xx/Kconfig
> index 8dec3c0911ad..bd6f1a1cf922 100644
> --- a/arch/powerpc/platforms/8xx/Kconfig
> +++ b/arch/powerpc/platforms/8xx/Kconfig
> @@ -45,7 +45,6 @@ config PPC_EP88XC
> config PPC_ADDER875
> bool "Analogue & Micro Adder 875"
> select CPM1
> - select REDBOOT
> help
> This enables support for the Analogue & Micro Adder 875
> board.
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: mm: make _PAGE_NUMA take effect
From: Benjamin Herrenschmidt @ 2013-12-09 6:19 UTC (permalink / raw)
To: Liu ping fan; +Cc: Paul Mackerras, linuxppc-dev, Aneesh Kumar K.V
In-Reply-To: <CAFgQCTtJh1-2wuLueREbA6ru5cje4EcxN5yT+cU-YNSrZi9O=Q@mail.gmail.com>
On Mon, 2013-12-09 at 14:17 +0800, Liu ping fan wrote:
> On Mon, Dec 9, 2013 at 8:31 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Thu, 2013-12-05 at 16:23 +0530, Aneesh Kumar K.V wrote:
> >> Liu Ping Fan <kernelfans@gmail.com> writes:
> >>
> >> > To enable the do_numa_page(), we should not fix _PAGE_NUMA in
> >> > hash_page(), so bail out for the case of pte_numa().
> >
> > For some reason I don't have 2/3 and 3/3 in my mbox (though I do have
> > them on patchwork) so I'll reply to this one.
> >
> > Overall, your statement that this is a faster path needs to be backed up
> > with numbers.
> >
> > The code is complicated enough as it-is, such additional mess in the low
> > level hashing code requires a good justification, and also a
> > demonstration that it doesn't add overhead to the normal hash path.
> >
> For the test, is it ok to have an user application to copy page where
> all page are PG_mlocked?
If that specific scenario is relevant in practice, then yes, though also
demonstrate the lack of regression with some more normal path such as a
kernel compile.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH V4 07/10] powerpc, lib: Add new branch instruction analysis support functions
From: Michael Ellerman @ 2013-12-09 6:21 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel, khandual
Cc: mikey, ak, eranian, acme, sukadev, mingo
In-Reply-To: <1386153162-11225-8-git-send-email-khandual@linux.vnet.ibm.com>
On Wed, 2013-04-12 at 10:32:39 UTC, Anshuman Khandual wrote:
> Generic powerpc branch instruction analysis support added in the code
> patching library which will help the subsequent patch on SW based
> filtering of branch records in perf. This patch also converts and
> exports some of the existing local static functions through the header
> file to be used else where.
>
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index a6f8c7a..8bab417 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -22,6 +22,36 @@
> #define BRANCH_SET_LINK 0x1
> #define BRANCH_ABSOLUTE 0x2
>
> +#define XL_FORM_LR 0x4C000020
> +#define XL_FORM_CTR 0x4C000420
> +#define XL_FORM_TAR 0x4C000460
> +
> +#define BO_ALWAYS 0x02800000
> +#define BO_CTR 0x02000000
> +#define BO_CRBI_OFF 0x00800000
> +#define BO_CRBI_ON 0x01800000
> +#define BO_CRBI_HINT 0x00400000
> +
> +/* Forms of branch instruction */
> +int instr_is_branch_iform(unsigned int instr);
> +int instr_is_branch_bform(unsigned int instr);
> +int instr_is_branch_xlform(unsigned int instr);
> +
> +/* Classification of XL-form instruction */
> +int is_xlform_lr(unsigned int instr);
> +int is_xlform_ctr(unsigned int instr);
> +int is_xlform_tar(unsigned int instr);
> +
> +/* Branch instruction is a call */
> +int is_branch_link_set(unsigned int instr);
> +
> +/* BO field analysis (B-form or XL-form) */
> +int is_bo_always(unsigned int instr);
> +int is_bo_ctr(unsigned int instr);
> +int is_bo_crbi_off(unsigned int instr);
> +int is_bo_crbi_on(unsigned int instr);
> +int is_bo_crbi_hint(unsigned int instr);
I think this is the wrong API.
We end up with all these micro checks, which don't actually encapsulate much,
and don't implement the logic perf needs. If we had another user for this level
of detail then it might make sense, but for a single user I think we're better
off just implementing the semantics it wants.
So that would be something more like:
bool instr_is_return_branch(unsigned int instr);
bool instr_is_conditional_branch(unsigned int instr);
bool instr_is_func_call(unsigned int instr);
bool instr_is_indirect_func_call(unsigned int instr);
These would then encapsulate something like the logic in your 8/10 patch. You
can hopefully also optimise the checking logic in each routine because you know
the exact semantics you're implementing.
cheers
^ permalink raw reply
* Re: [PATCH V4 08/10] powerpc, perf: Enable SW filtering in branch stack sampling framework
From: Michael Ellerman @ 2013-12-09 6:21 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel, khandual
Cc: mikey, ak, eranian, acme, sukadev, mingo
In-Reply-To: <1386153162-11225-9-git-send-email-khandual@linux.vnet.ibm.com>
On Wed, 2013-04-12 at 10:32:40 UTC, Anshuman Khandual wrote:
> This patch enables SW based post processing of BHRB captured branches
> to be able to meet more user defined branch filtration criteria in perf
> branch stack sampling framework. These changes increase the number of
> branch filters and their valid combinations on any powerpc64 server
> platform with BHRB support. Find the summary of code changes here.
>
> (1) struct cpu_hw_events
>
> Introduced two new variables track various filter values and mask
>
> (a) bhrb_sw_filter Tracks SW implemented branch filter flags
> (b) filter_mask Tracks both (SW and HW) branch filter flags
The name 'filter_mask' doesn't mean much to me. I'd rather it was 'bhrb_filter'.
> (2) Event creation
>
> Kernel will figure out supported BHRB branch filters through a PMU call
> back 'bhrb_filter_map'. This function will find out how many of the
> requested branch filters can be supported in the PMU HW. It will not
> try to invalidate any branch filter combinations. Event creation will not
> error out because of lack of HW based branch filters. Meanwhile it will
> track the overall supported branch filters in the "filter_mask" variable.
>
> Once the PMU call back returns kernel will process the user branch filter
> request against available SW filters while looking at the "filter_mask".
> During this phase all the branch filters which are still pending from the
> user requested list will have to be supported in SW failing which the
> event creation will error out.
>
> (3) SW branch filter
>
> During the BHRB data capture inside the PMU interrupt context, each
> of the captured 'perf_branch_entry.from' will be checked for compliance
> with applicable SW branch filters. If the entry does not conform to the
> filter requirements, it will be discarded from the final perf branch
> stack buffer.
>
> (4) Supported SW based branch filters
>
> (a) PERF_SAMPLE_BRANCH_ANY_RETURN
> (b) PERF_SAMPLE_BRANCH_IND_CALL
> (c) PERF_SAMPLE_BRANCH_ANY_CALL
> (d) PERF_SAMPLE_BRANCH_COND
>
> Please refer patch to understand the classification of instructions into
> these branch filter categories.
>
> (5) Multiple branch filter semantics
>
> Book3 sever implementation follows the same OR semantics (as implemented in
> x86) while dealing with multiple branch filters at any point of time. SW
> branch filter analysis is carried on the data set captured in the PMU HW.
> So the resulting set of data (after applying the SW filters) will inherently
> be an AND with the HW captured set. Hence any combination of HW and SW branch
> filters will be invalid. HW based branch filters are more efficient and faster
> compared to SW implemented branch filters. So at first the PMU should decide
> whether it can support all the requested branch filters itself or not. In case
> it can support all the branch filters in an OR manner, we dont apply any SW
> branch filter on top of the HW captured set (which is the final set). This
> preserves the OR semantic of multiple branch filters as required. But in case
> where the PMU cannot support all the requested branch filters in an OR manner,
> it should not apply any it's filters and leave it upto the SW to handle them
> all. Its the PMU code's responsibility to uphold this protocol to be able to
> conform to the overall OR semantic of perf branch stack sampling framework.
I'd prefer this level of commentary was in a block comment in the code. It's
much more likely to be seen by a future hacker than here in the commit log.
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 2de7d48..54d39a5 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -48,6 +48,8 @@ struct cpu_hw_events {
>
> /* BHRB bits */
> u64 bhrb_hw_filter; /* BHRB HW branch filter */
> + u64 bhrb_sw_filter; /* BHRB SW branch filter */
> + u64 filter_mask; /* Branch filter mask */
> int bhrb_users;
> void *bhrb_context;
> struct perf_branch_stack bhrb_stack;
> @@ -400,6 +402,228 @@ static __u64 power_pmu_bhrb_to(u64 addr)
> return target - (unsigned long)&instr + addr;
> }
>
> +/*
> + * Instruction opcode analysis
> + *
> + * Analyse instruction opcodes and classify them
> + * into various branch filter options available.
> + * This follows the standard semantics of OR which
> + * means that instructions which conforms to `any`
> + * of the requested branch filters get picked up.
> + */
> +static bool validate_instruction(unsigned int *addr, u64 bhrb_sw_filter)
> +{
"validate" is not a good name here. That implies that this routine identifies
"valid" and "invalid" instructions - but that's not really correct.
Also it's preferable to not use the same variable name for the local as for the
cpuhw->bhrb_sw_filter global. Although technically it doesn't shadow the global
it can still be confusing to a human, ie. me. A good name for the local would
just be "sw_filter" because we know in this code that we're dealing with the
BHRB.
> + bool result = false;
> +
> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_RETURN) {
> +
> + /* XL-form instruction */
> + if (instr_is_branch_xlform(*addr)) {
> +
> + /* LR should not be set */
> + /*
> + * Conditional and unconditional
> + * branch to LR register.
> + */
> + if (is_xlform_lr(*addr))
> + result = true;
> + }
> + }
> + }
is_xform_lr() implies instr_is_branch_xlform(), and once you get a hit you can
short-circuit and exit the function, so this should boil down to just:
if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_RETURN)
if (is_xlform_lr(*addr) && !is_branch_link_set(*addr))
return true;
Having said that I think it should move into a routine in code-patching as I
said in the comments to the previous patch.
> +
> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_IND_CALL) {
> + /* XL-form instruction */
> + if (instr_is_branch_xlform(*addr)) {
> +
> + /* LR should be set */
> + if (is_branch_link_set(*addr)) {
> + /*
> + * Conditional and unconditional
> + * branch to CTR.
> + */
> + if (is_xlform_ctr(*addr))
> + result = true;
> +
> + /*
> + * Conditional and unconditional
> + * branch to LR.
> + */
> + if (is_xlform_lr(*addr))
> + result = true;
> +
> + /*
> + * Conditional and unconditional
> + * branch to TAR.
> + */
> + if (is_xlform_tar(*addr))
> + result = true;
What other kind of XL-Form branch is there?
> + }
> + }
> + }
The comments above all have a bogus leading space.
> +
> + /* Any-form branch */
> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_CALL) {
> + /* LR should be set */
> + if (is_branch_link_set(*addr))
> + result = true;
Short circuit.
> + }
> +
> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_COND) {
> +
> + /* I-form instruction - excluded */
> + if (instr_is_branch_iform(*addr))
> + goto out;
> +
> + /* B-form or XL-form instruction */
> + if (instr_is_branch_bform(*addr) || instr_is_branch_xlform(*addr)) {
> +
> + /* Not branch always */
> + if (!is_bo_always(*addr)) {
> +
> + /* Conditional branch to CTR register */
> + if (is_bo_ctr(*addr))
> + goto out;
We might have discussed this but why not?
> +
> + /* CR[BI] conditional branch with static hint */
A conditional branch with a static hint is still a conditional branch?
> + if (is_bo_crbi_off(*addr) || is_bo_crbi_on(*addr)) {
> + if (is_bo_crbi_hint(*addr))
> + goto out;
> + }
> +
> + result = true;
> + }
> + }
> + }
> +out:
> + return result;
> +}
> +
> +static bool check_instruction(u64 addr, u64 bhrb_sw_filter)
> +{
"check" is not a very descriptive name here, especially when "check" calls
"validate".
"filter" is also not good because a filter keeps some things and rejects others,
and the directionality is not clear.
I'd suggest "filter_selects_branch()" or just "keep_branch()".
> + unsigned int instr;
> + bool ret;
> +
> + if (bhrb_sw_filter == 0)
> + return true;
> +
> + if (is_kernel_addr(addr)) {
> + ret = validate_instruction((unsigned int *) addr, bhrb_sw_filter);
No reason not to return directly here.
That would then remove the need for an else block.
> + } else {
> + /*
> + * Userspace address needs to be
> + * copied first before analysis.
> + */
> + pagefault_disable();
> + ret = __get_user_inatomic(instr, (unsigned int __user *)addr);
I suspect you borrowed this incantation from the callchain code. Unlike that
code you don't fallback to reading the page tables directly.
I'd rather see the accessor in the callchain code made generic and have you
call it here.
> +
> + /*
> + * If the instruction could not be accessible
> + * from user space, we still 'okay' the entry.
> + */
> + if (ret) {
> + pagefault_enable();
> + return true;
> + }
> + pagefault_enable();
> + ret = validate_instruction(&instr, bhrb_sw_filter);
No reason not to return directly here.
> + }
> + return ret;
> +}
> +
> +/*
> + * Validate whether all requested branch filters
> + * are getting processed either in the PMU or in SW.
> + */
> +static int match_filters(u64 branch_sample_type, u64 filter_mask)
I don't really understand why we have this routine?
We should implement the filter in HW if we can, or in SW. Which filters can't we
implement in SW?
> +{
> + u64 x;
> +
> + if (filter_mask == PERF_SAMPLE_BRANCH_ANY)
> + return true;
> +
> + for_each_branch_sample_type(x) {
> + if (!(branch_sample_type & x))
> + continue;
> + /*
> + * Privilege filter requests have been already
> + * taken care during the base PMU configuration.
> + */
> + if (x == PERF_SAMPLE_BRANCH_USER)
> + continue;
> + if (x == PERF_SAMPLE_BRANCH_KERNEL)
> + continue;
> + if (x == PERF_SAMPLE_BRANCH_HV)
> + continue;
> +
> + /*
> + * Requested filter not available either
> + * in PMU or in SW.
> + */
> + if (!(filter_mask & x))
> + return false;
> + }
> + return true;
> +}
> +
> +/*
> + * Required SW based branch filters
> + *
> + * This is called after figuring out what all branch filters the
> + * PMU HW supports for the requested branch filter set. Here we
> + * will go through all the SW implemented branch filters one by
> + * one and pick them up if its not already supported in the PMU.
> + */
> +static u64 branch_filter_map(u64 branch_sample_type, u64 pmu_bhrb_filter,
> + u64 *filter_mask)
Whitespace is foobar here ^
This function deals exclusively with the software filter IIUI, but the name
doesn't indicate that in any way.
As far as the logic goes, you return the software filter value, as well as
mutating the *filter_mask. And in all cases you make the same modification to
both. That seems very dubious.
Shouldn't this routine just setup the software filter, and leave the upper
level code to deal with the HW & SW filter values?
> +{
> + u64 branch_sw_filter = 0;
> +
> + /* No branch filter requested */
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY) {
> + WARN_ON(pmu_bhrb_filter != 0);
> + WARN_ON(*filter_mask != PERF_SAMPLE_BRANCH_ANY);
> + return branch_sw_filter;
> + }
> +
> + /*
> + * PMU supported branch filters must also be implemented in SW
> + * in the event when the PMU is unable to process them for some
> + * reason. This all those branch filters can be satisfied with
> + * SW implemented filters. But right now, there is now way to
> + * initimate the user about this decision.
Please proof read these comments, I don't entirely follow this one.
You say "must also be implemented in SW" - but I think it's actually "must be
implemented in SW", ie. the HW is not "also" implementing the filter.
You say "in the event when" but I think you just mean "when" - the word "event"
has a particular meaning in this code so you should only use it for that if at
all possible.
I don't follow "This all those".
You should just drop the last sentence, there is never going to be any way to
notify the user that their filter is implemented in HW vs SW, that's an
implementation detail.
> + */
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_ANY_CALL)) {
> + branch_sw_filter |= PERF_SAMPLE_BRANCH_ANY_CALL;
> + *filter_mask |= PERF_SAMPLE_BRANCH_ANY_CALL;
> + }
> + }
> +
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_COND)) {
> + branch_sw_filter |= PERF_SAMPLE_BRANCH_COND;
> + *filter_mask |= PERF_SAMPLE_BRANCH_COND;
> + }
> + }
> +
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN) {
> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_ANY_RETURN)) {
> + branch_sw_filter |= PERF_SAMPLE_BRANCH_ANY_RETURN;
> + *filter_mask |= PERF_SAMPLE_BRANCH_ANY_RETURN;
> + }
> + }
> +
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL) {
> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_IND_CALL)) {
> + branch_sw_filter |= PERF_SAMPLE_BRANCH_IND_CALL;
> + *filter_mask |= PERF_SAMPLE_BRANCH_IND_CALL;
> + }
> + }
> +
> + return branch_sw_filter;
> +}
> +
> /* Processing BHRB entries */
> void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
> {
> @@ -459,17 +683,29 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
> addr = 0;
> }
> cpuhw->bhrb_entries[u_index].from = addr;
> +
> + if (!check_instruction(cpuhw->
> + bhrb_entries[u_index].from,
> + cpuhw->bhrb_sw_filter))
> + u_index--;
> } else {
> /* Branches to immediate field
> (ie I or B form) */
> cpuhw->bhrb_entries[u_index].from = addr;
> - cpuhw->bhrb_entries[u_index].to =
> - power_pmu_bhrb_to(addr);
> - cpuhw->bhrb_entries[u_index].mispred = pred;
> - cpuhw->bhrb_entries[u_index].predicted = ~pred;
> + if (check_instruction(cpuhw->
> + bhrb_entries[u_index].from,
> + cpuhw->bhrb_sw_filter)) {
> + cpuhw->bhrb_entries[u_index].
> + to = power_pmu_bhrb_to(addr);
> + cpuhw->bhrb_entries[u_index].
> + mispred = pred;
> + cpuhw->bhrb_entries[u_index].
> + predicted = ~pred;
> + } else {
> + u_index--;
> + }
> }
> u_index++;
This code was already in need of some unindentation, and now it's just
ridiculous.
To start with at the beginning of this routine we have:
while (..) {
if (!val)
break;
else {
// Bulk of the logic
...
}
}
That should almost always become:
while (..) {
if (!val)
break;
// Bulk of the logic
...
}
But in this case that's not enough. Please send a precursor patch which moves
this logic out into a helper function.
> -
> }
> }
> cpuhw->bhrb_stack.nr = u_index;
> @@ -1255,7 +1491,11 @@ nocheck:
> if (has_branch_stack(event)) {
> power_pmu_bhrb_enable(event);
> cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map(
> - event->attr.branch_sample_type);
> + event->attr.branch_sample_type,
> + &cpuhw->filter_mask);
> + cpuhw->bhrb_sw_filter = branch_filter_map
> + (event->attr.branch_sample_type,
> + cpuhw->bhrb_hw_filter, &cpuhw->filter_mask);
> }
>
> perf_pmu_enable(event->pmu);
> @@ -1637,10 +1877,16 @@ static int power_pmu_event_init(struct perf_event *event)
> err = power_check_constraints(cpuhw, events, cflags, n + 1);
>
> if (has_branch_stack(event)) {
> - cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map(
> - event->attr.branch_sample_type);
> -
> - if(cpuhw->bhrb_hw_filter == -1)
> + cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map
> + (event->attr.branch_sample_type,
> + &cpuhw->filter_mask);
> + cpuhw->bhrb_sw_filter = branch_filter_map
> + (event->attr.branch_sample_type,
> + cpuhw->bhrb_hw_filter,
> + &cpuhw->filter_mask);
> +
> + if(!match_filters(event->attr.branch_sample_type,
> + cpuhw->filter_mask))
> return -EOPNOTSUPP;
The above two hunks look too similar for my liking.
cheers
^ permalink raw reply
* Re: [PATCH V4 09/10] power8, perf: Change BHRB branch filter configuration
From: Michael Ellerman @ 2013-12-09 6:21 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel, khandual
Cc: mikey, ak, eranian, acme, sukadev, mingo
In-Reply-To: <1386153162-11225-10-git-send-email-khandual@linux.vnet.ibm.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4825 bytes --]
On Wed, 2013-04-12 at 10:32:41 UTC, Anshuman Khandual wrote:
> Powerpc kernel now supports SW based branch filters for book3s systems with some
> specifc requirements while dealing with HW supported branch filters in order to
> achieve overall OR semantics prevailing in perf branch stack sampling framework.
> This patch adapts the BHRB branch filter configuration to meet those protocols.
> POWER8 PMU does support 3 branch filters (out of which two are getting used in
> perf branch stack) which are mutually exclussive and cannot be ORed with each
> other. This implies that PMU can only handle one HW based branch filter request
> at any point of time. For all other combinations PMU will pass it on to the SW.
>
> Also the combination of PERF_SAMPLE_BRANCH_ANY_CALL and PERF_SAMPLE_BRANCH_COND
> can now be handled in SW, hence we dont error them out anymore.
>
> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
> index 03c5b8d..6021349 100644
> --- a/arch/powerpc/perf/power8-pmu.c
> +++ b/arch/powerpc/perf/power8-pmu.c
> @@ -561,7 +561,56 @@ static int power8_generic_events[] = {
>
> static u64 power8_bhrb_filter_map(u64 branch_sample_type, u64 *filter_mask)
> {
> - u64 pmu_bhrb_filter = 0;
> + u64 x, tmp, pmu_bhrb_filter = 0;
> + *filter_mask = 0;
> +
> + /* No branch filter requested */
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY) {
> + *filter_mask = PERF_SAMPLE_BRANCH_ANY;
> + return pmu_bhrb_filter;
> + }
> +
> + /*
> + * P8 does not support oring of PMU HW branch filters. Hence
> + * if multiple branch filters are requested which includes filters
> + * supported in PMU, still go ahead and clear the PMU based HW branch
> + * filter component as in this case all the filters will be processed
> + * in SW.
Leading space there.
> + */
> + tmp = branch_sample_type;
> +
> + /* Remove privilege filters before comparison */
> + tmp &= ~PERF_SAMPLE_BRANCH_USER;
> + tmp &= ~PERF_SAMPLE_BRANCH_KERNEL;
> + tmp &= ~PERF_SAMPLE_BRANCH_HV;
> +
> + for_each_branch_sample_type(x) {
> + /* Ignore privilege requests */
> + if ((x == PERF_SAMPLE_BRANCH_USER) || (x == PERF_SAMPLE_BRANCH_KERNEL) || (x == PERF_SAMPLE_BRANCH_HV))
> + continue;
> +
> + if (!(tmp & x))
> + continue;
> +
> + /* Supported HW PMU filters */
> + if (tmp & PERF_SAMPLE_BRANCH_ANY_CALL) {
> + tmp &= ~PERF_SAMPLE_BRANCH_ANY_CALL;
> + if (tmp) {
> + pmu_bhrb_filter = 0;
> + *filter_mask = 0;
> + return pmu_bhrb_filter;
> + }
> + }
> +
> + if (tmp & PERF_SAMPLE_BRANCH_COND) {
> + tmp &= ~PERF_SAMPLE_BRANCH_COND;
> + if (tmp) {
> + pmu_bhrb_filter = 0;
> + *filter_mask = 0;
> + return pmu_bhrb_filter;
> + }
> + }
> + }
>
> /* BHRB and regular PMU events share the same privilege state
> * filter configuration. BHRB is always recorded along with a
> @@ -570,34 +619,20 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type, u64 *filter_mask)
> * PMU event, we ignore any separate BHRB specific request.
> */
>
> - /* No branch filter requested */
> - if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY)
> - return pmu_bhrb_filter;
> -
> - /* Invalid branch filter options - HW does not support */
> - if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
> - return -1;
> -
> - if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
> - return -1;
> -
> + /* Supported individual branch filters */
> if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
> pmu_bhrb_filter |= POWER8_MMCRA_IFM1;
> + *filter_mask |= PERF_SAMPLE_BRANCH_ANY_CALL;
> return pmu_bhrb_filter;
> }
>
> if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
> pmu_bhrb_filter |= POWER8_MMCRA_IFM3;
> + *filter_mask |= PERF_SAMPLE_BRANCH_COND;
> return pmu_bhrb_filter;
> }
>
> - /* PMU does not support ANY combination of HW BHRB filters */
> - if ((branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) &&
> - (branch_sample_type & PERF_SAMPLE_BRANCH_COND))
> - return -1;
> -
> - /* Every thing else is unsupported */
> - return -1;
> + return pmu_bhrb_filter;
> }
As I said in my comments on version 3 which you ignored:
I think it would be clearer if we actually checked for the possibilities we
allow and let everything else fall through, eg:
/* Ignore user/kernel/hv bits */
branch_sample_type &= ~PERF_SAMPLE_BRANCH_PLM_ALL;
if (branch_sample_type == PERF_SAMPLE_BRANCH_ANY)
return 0;
if (branch_sample_type == PERF_SAMPLE_BRANCH_ANY_CALL)
return POWER8_MMCRA_IFM1;
if (branch_sample_type == PERF_SAMPLE_BRANCH_COND)
return POWER8_MMCRA_IFM3;
return -1;
cheers
^ permalink raw reply
* Re: [PATCH V4 10/10] powerpc, perf: Cleanup SW branch filter list look up
From: Michael Ellerman @ 2013-12-09 6:21 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel, khandual
Cc: mikey, ak, eranian, acme, sukadev, mingo
In-Reply-To: <1386153162-11225-11-git-send-email-khandual@linux.vnet.ibm.com>
On Wed, 2013-04-12 at 10:32:42 UTC, Anshuman Khandual wrote:
> This patch adds enumeration for all available SW branch filters
> in powerpc book3s code and also streamlines the look for the
> SW branch filter entries while trying to figure out which all
> branch filters can be supported in SW.
This appears to patch code that was only added in 8/10 ?
Was there any reason not to do it the right way from the beginning?
cheers
^ permalink raw reply
* linux-next: build failure after merge of the final tree (powerpc tree related)
From: Stephen Rothwell @ 2013-12-09 6:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt, linuxppc-dev
Cc: Mahesh Salgaonkar, linux-next, Paul Mackerras, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]
Hi all,
After merging the final tree, today's linux-next build (powerpc
allyesconfig) failed like this:
arch/powerpc/kernel/exceptions-64s.S: Assembler messages:
arch/powerpc/kernel/exceptions-64s.S:958: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:959: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:983: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:984: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:1003: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:1013: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:1014: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:1015: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:1016: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:1017: Error: attempt to move .org backwards
arch/powerpc/kernel/exceptions-64s.S:1018: Error: attempt to move .org backwards
Caused by commit 1e9b4507ed98 ("powerpc/book3s: handle machine check in
Linux host").
I have reverted these commits (possibly some of these reverts are
unnecessary):
b63a0ffe35de "powerpc/powernv: Machine check exception handling"
28446de2ce99 "powerpc/powernv: Remove machine check handling in OPAL"
b5ff4211a829 "powerpc/book3s: Queue up and process delayed MCE events"
36df96f8acaf "powerpc/book3s: Decode and save machine check event"
ae744f3432d3 "powerpc/book3s: Flush SLB/TLBs if we get SLB/TLB machine check errors on power8"
e22a22740c1a "powerpc/book3s: Flush SLB/TLBs if we get SLB/TLB machine check errors on power7"
0440705049b0 "powerpc/book3s: Add flush_tlb operation in cpu_spec"
4c703416efc0 "powerpc/book3s: Introduce a early machine check hook in cpu_spec"
1c51089f777b "powerpc/book3s: Return from interrupt if coming from evil context"
1e9b4507ed98 "powerpc/book3s: handle machine check in Linux host"
--
Cheers,
Stephen Rothwell <sfr@canb.auug.org.au>
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH V2 0/3] powerpc iommu: Remove hardcoded page sizes
From: Alistair Popple @ 2013-12-09 7:17 UTC (permalink / raw)
To: benh, linuxppc-dev
The series doesn't actually change the iommu page size as each platform continues to
initialise the iommu page size to a hardcoded value of 4K.
At this stage testing has only been carried out on a pSeries machine, other platforms
including cell have yet to be tested.
Changes from V1:
* Rebased on Ben's next tree
* Updated constants in 1/3 that were not present in V1 (thanks Alexy!)
* Added initialisation for pasemi platform that was missed in V1
^ 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