* Re: [PATCH v2 2/3] powerpc/kvm/book3s_hv: Enable CPUs to run guest after waking up from fast-sleep
From: Shreyas B Prabhu @ 2014-10-02 16:39 UTC (permalink / raw)
To: linux-kernel
Cc: linux-pm, Rafael J. Wysocki, Paul Mackerras, Preeti U Murthy,
linuxppc-dev
In-Reply-To: <1412149560-2953-3-git-send-email-shreyas@linux.vnet.ibm.com>
CCing Rafael J. Wysocki and linux-pm@vger.kernel.org
On Wednesday 01 October 2014 01:15 PM, Shreyas B. Prabhu wrote:
> When guests have to be launched, the secondary threads which are offline
> are woken up to run the guests. Today these threads wake up from nap
> and check if they have to run guests. Now that the offline secondary
> threads can go to fastsleep or going ahead a deeper idle state such as winkle,
> add this check in the wakeup from any of the deep idle states path as well.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Suggested-by: "Srivatsa S. Bhat" <srivatsa@mit.edu>
> Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
> [ Changelog added by <preeti@linux.vnet.ibm.com> ]
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 35 ++++++++++++++++-------------------
> 1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 050f79a..c64f3cc0 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -100,25 +100,8 @@ system_reset_pSeries:
> SET_SCRATCH0(r13)
> #ifdef CONFIG_PPC_P7_NAP
> BEGIN_FTR_SECTION
> - /* Running native on arch 2.06 or later, check if we are
> - * waking up from nap. We only handle no state loss and
> - * supervisor state loss. We do -not- handle hypervisor
> - * state loss at this time.
> - */
> - mfspr r13,SPRN_SRR1
> - rlwinm. r13,r13,47-31,30,31
> - beq 9f
>
> - /* waking up from powersave (nap) state */
> - cmpwi cr1,r13,2
> - /* Total loss of HV state is fatal, we could try to use the
> - * PIR to locate a PACA, then use an emergency stack etc...
> - * OPAL v3 based powernv platforms have new idle states
> - * which fall in this catagory.
> - */
> - bgt cr1,8f
> GET_PACA(r13)
> -
> #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> li r0,KVM_HWTHREAD_IN_KERNEL
> stb r0,HSTATE_HWTHREAD_STATE(r13)
> @@ -131,13 +114,27 @@ BEGIN_FTR_SECTION
> 1:
> #endif
>
> + /* Running native on arch 2.06 or later, check if we are
> + * waking up from nap. We only handle no state loss and
> + * supervisor state loss. We do -not- handle hypervisor
> + * state loss at this time.
> + */
> + mfspr r13,SPRN_SRR1
> + rlwinm. r13,r13,47-31,30,31
> + beq 9f
> +
> + /* waking up from powersave (nap) state */
> + cmpwi cr1,r13,2
> + GET_PACA(r13)
> +
> + bgt cr1,8f
> +
> beq cr1,2f
> b power7_wakeup_noloss
> 2: b power7_wakeup_loss
>
> /* Fast Sleep wakeup on PowerNV */
> -8: GET_PACA(r13)
> - b power7_wakeup_tb_loss
> +8: b power7_wakeup_tb_loss
>
> 9:
> END_FTR_SECTION_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
>
^ permalink raw reply
* [PATCH] powerpc: fix sys_call_table declaration
From: Romeo Cane @ 2014-10-02 14:41 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Declaring sys_call_table as a pointer causes the compiler to generate the wrong lookup code in arch_syscall_addr
Signed-off-by: Romeo Cane <romeo.cane.ext@coriant.com>
---
arch/powerpc/include/asm/syscall.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index b54b2ad..528ba9d 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -17,7 +17,7 @@
/* ftrace syscalls requires exporting the sys_call_table */
#ifdef CONFIG_FTRACE_SYSCALLS
-extern const unsigned long *sys_call_table;
+extern const unsigned long sys_call_table[];
#endif /* CONFIG_FTRACE_SYSCALLS */
static inline long syscall_get_nr(struct task_struct *task,
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH V7 00/17] Enable SRIOV on POWER8
From: Bjorn Helgaas @ 2014-10-02 15:59 UTC (permalink / raw)
To: Wei Yang
Cc: Benjamin Herrenschmidt, linux-pci@vger.kernel.org, Gavin Shan,
Mike Qiu, Guo Chao, linuxppc-dev
In-Reply-To: <20140820033546.GA7669@richard>
On Wed, Aug 20, 2014 at 11:35:46AM +0800, Wei Yang wrote:
> On Tue, Aug 19, 2014 at 10:12:27PM -0500, Bjorn Helgaas wrote:
> >On Tue, Aug 19, 2014 at 9:34 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> >> On Tue, Aug 19, 2014 at 03:19:42PM -0600, Bjorn Helgaas wrote:
> >>>On Thu, Jul 24, 2014 at 02:22:10PM +0800, Wei Yang wrote:
> >>>> This patch set enables the SRIOV on POWER8.
> >>>>
> >>>> The gerneral idea is put each VF into one individual PE and allocate required
> >>>> resources like DMA/MSI.
> >>>>
> >>>> One thing special for VF PE is we use M64BT to cover the IOV BAR. M64BT is one
> >>>> hardware on POWER platform to map MMIO address to PE. By using M64BT, we could
> >>>> map one individual VF to a VF PE, which introduce more flexiblity to users.
> >>>>
> >>>> To achieve this effect, we need to do some hack on pci devices's resources.
> >>>> 1. Expand the IOV BAR properly.
> >>>> Done by pnv_pci_ioda_fixup_iov_resources().
> >>>> 2. Shift the IOV BAR properly.
> >>>> Done by pnv_pci_vf_resource_shift().
> >>>> 3. IOV BAR alignment is the total size instead of an individual size on
> >>>> powernv platform.
> >>>> Done by pnv_pcibios_sriov_resource_alignment().
> >>>> 4. Take the IOV BAR alignment into consideration in the sizing and assigning.
> >>>> This is achieved by commit: "PCI: Take additional IOV BAR alignment in
> >>>> sizing and assigning"
> >>>>
> >>>> Test Environment:
> >>>> The SRIOV device tested is Emulex Lancer and Mellanox ConnectX-3 on
> >>>> POWER8.
> >>>>
> >>>> Examples on pass through a VF to guest through vfio:
> >>>> 1. install necessary modules
> >>>> modprobe vfio
> >>>> modprobe vfio-pci
> >>>> 2. retrieve the iommu_group the device belongs to
> >>>> readlink /sys/bus/pci/devices/0000:06:0d.0/iommu_group
> >>>> ../../../../kernel/iommu_groups/26
> >>>> This means it belongs to group 26
> >>>> 3. see how many devices under this iommu_group
> >>>> ls /sys/kernel/iommu_groups/26/devices/
> >>>> 4. unbind the original driver and bind to vfio-pci driver
> >>>> echo 0000:06:0d.0 > /sys/bus/pci/devices/0000:06:0d.0/driver/unbind
> >>>> echo 1102 0002 > /sys/bus/pci/drivers/vfio-pci/new_id
> >>>> Note: this should be done for each device in the same iommu_group
> >>>> 5. Start qemu and pass device through vfio
> >>>> /home/ywywyang/git/qemu-impreza/ppc64-softmmu/qemu-system-ppc64 \
> >>>> -M pseries -m 2048 -enable-kvm -nographic \
> >>>> -drive file=/home/ywywyang/kvm/fc19.img \
> >>>> -monitor telnet:localhost:5435,server,nowait -boot cd \
> >>>> -device "spapr-pci-vfio-host-bridge,id=CXGB3,iommu=26,index=6"
> >>>>
> >>>> Verify this is the exact VF response:
> >>>> 1. ping from a machine in the same subnet(the broadcast domain)
> >>>> 2. run arp -n on this machine
> >>>> 9.115.251.20 ether 00:00:c9:df:ed:bf C eth0
> >>>> 3. ifconfig in the guest
> >>>> # ifconfig eth1
> >>>> eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
> >>>> inet 9.115.251.20 netmask 255.255.255.0 broadcast 9.115.251.255
> >>>> inet6 fe80::200:c9ff:fedf:edbf prefixlen 64 scopeid 0x20<link>
> >>>> ether 00:00:c9:df:ed:bf txqueuelen 1000 (Ethernet)
> >>>> RX packets 175 bytes 13278 (12.9 KiB)
> >>>> RX errors 0 dropped 0 overruns 0 frame 0
> >>>> TX packets 58 bytes 9276 (9.0 KiB)
> >>>> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
> >>>> 4. They have the same MAC address
> >>>>
> >>>> Note: make sure you shutdown other network interfaces in guest.
> >>>>
> >>>> ---
> >>>> v6 -> v7:
> >>>> 1. add IORESOURCE_ARCH flag for IOV BAR on powernv platform.
> >>>> 2. when IOV BAR has IORESOURCE_ARCH flag, the size is retrieved from
> >>>> hardware directly. If not, calculate as usual.
> >>>> 3. reorder the patch set, group them by subsystem:
> >>>> PCI, powerpc, powernv
> >>>> 4. rebase it on 3.16-rc6
> >>>
> >>>This doesn't apply for me on v3.16-rc6:
> >>>
> >>> 02:48:57 ~/linux$ stg rebase v3.16-rc6
> >>> Checking for changes in the working directory ... done
> >>> Rebasing to "v3.16-rc6" ... done
> >>> No patches applied
> >>> 02:49:14 ~/linux$ stg import -M --sign m/wy
> >>> Checking for changes in the working directory ... done
> >>> Importing patch "pci-iov-export-interface-for" ... done
> >>> Importing patch "pci-iov-get-vf-bar-size-from" ... done
> >>> Importing patch "pci-add-weak" ... done
> >>> Importing patch "pci-take-additional-iov-bar" ... done
> >>> Importing patch "powerpc-pci-don-t-unset-pci" ... done
> >>> Importing patch "powerpc-pci-define" ... done
> >>> Importing patch "powrepc-pci-refactor-pci_dn" ... done
> >>> Importing patch "powerpc-powernv-use-pci_dn-in" ... error: patch failed:
> >>> arch/powerpc/platforms/powernv/pci.c:376
> >>> error: arch/powerpc/platforms/powernv/pci.c: patch does not apply
> >>> stg import: Diff does not apply cleanly
> >>>
> >>>What am I missing?
> >>>
> >>>I assume you intend these all to go through my tree just to keep them all
> >>>together. The ideal rebase target for me would be v3.17-rc1.
> >>
> >> Ok, I will rebase it on v3.17-rc1 upstream. While I guess the conflict is due
> >> to some patches from Gavin, which is not merged at that moment. I will make
> >> sure it applies to v3.17-rc1.
> >
> >I tried applying them on v3.16-rc6 as well as on every change to
> >arch/powerpc/platforms/powernv/pci.c between v3.16-rc6 and v3.17-rc1,
> >and none applied cleanly. Patches you post should be based on some
> >upstream tag, not on something that includes unmerged patches.
>
> Sorry about this, I will pay attention to this next time.
I haven't seen any more on this series, and I'm assuming you'll post a
rebased series (maybe you're waiting for v3.18-rc1?). I'm just checking to
make sure you're not waiting for something from me...
Bjorn
^ permalink raw reply
* Re: [PATCH v2 2/4] gpu/radeon: Move 64-bit MSI quirk from arch to driver
From: Bjorn Helgaas @ 2014-10-02 15:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Dave Airlie, linux-pci, Anton Blanchard, Brian King,
Yijing Wang, Takashi Iwai, Alex Deucher
In-Reply-To: <1412210062.10667.1.camel@pasglop>
On Thu, Oct 02, 2014 at 10:34:22AM +1000, Benjamin Herrenschmidt wrote:
>
> A number of radeon cards have a HW limitation causing them to be
> unable to generate the full 64-bit of address bits for MSIs. This
> breaks MSIs on some platforms such as POWER machines.
>
> We used to have a powerpc specific quirk to address that on a
> single card, but this doesn't scale very well, this is better
> put under control of the drivers who know precisely what a given
> HW revision can do.
>
> This moves the setting of the quirk flag to the radeon driver
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> v2: This is just adjusted to the new flag name
I'm sorta confused because I got two "v2 2/4" emails a minute or so apart.
I assume they're the same.
>
> arch/powerpc/kernel/pci_64.c | 1 -
> drivers/gpu/drm/radeon/radeon_irq_kms.c | 10 ++++++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> index d41a831..5330f6d 100644
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -271,5 +271,4 @@ static void quirk_radeon_32bit_msi(struct pci_dev *dev)
> {
> dev->no_64bit_msi = true;
> }
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
Why do we keep the 0xaa68 quirk? Shouldn't that be made generic, too?
> diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> index 16807af..e760671 100644
> --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> @@ -202,6 +202,16 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
> if (rdev->flags & RADEON_IS_AGP)
> return false;
>
> + /*
> + * Older chips have a HW limitation, they can only generate 40 bits
> + * of address for "64-bit" MSIs which breaks on some platforms, notably
> + * IBM POWER servers, so we limit them
> + */
> + if (rdev->family < CHIP_BONAIRE) {
> + dev_info(rdev->dev, "radeon: MSI limited to 32-bit\n");
> + rdev->pdev->no_64bit_msi = true;
> + }
> +
> /* force MSI on */
> if (radeon_msi == 1)
> return true;
>
>
>
^ permalink raw reply
* Re: [PATCH v2 1/4] pci/msi: Move "no_64bit_msi" flag from powerpc to generic pci_dev
From: Bjorn Helgaas @ 2014-10-02 15:31 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Dave Airlie, linux-pci, Anton Blanchard, Brian King,
Yijing Wang, Takashi Iwai, Alex Deucher
In-Reply-To: <1412210052.10667.0.camel@pasglop>
On Thu, Oct 02, 2014 at 10:34:12AM +1000, Benjamin Herrenschmidt wrote:
>
> Some devices have broken 64-bit MSI support which only support some
> address bits (40 to 48 typically). This doesn't work on some platforms
> such as POWER servers, so we need a quirk.
>
> Currently we keep a flag in a powerpc specific data structure which we
> have per PCI device. However this is impractical as we really want the
> driver to set that flag appropriately (and the driver shouldn't touch
> that arch specific data structure).
>
> It's also not unlikely that this limitation will affect other architectures
> in the long run.
>
> So this moves the flag to struct pci_dev instead and adjusts the
> corresponding arch/powerpc code to look for it there. At this point,
> there is no attempt at making other architectures honor it just yet,
> though it appears that x86 doesn't care and always generates 32-bit
> MSI addresses.
I thought you were going to add something in drivers/pci to make this more
generic?
Is it feasible to structure this series as follows?
- add generic stuff (struct pci_dev bit, msi.c checks)
- add quirks to drivers to set the pci_dev bit
- change powerpc to check pci_dev bit instead of pdn bit
- remove powerpc-specific quirks since nobody looks as the pdn bit
I think it would be easier to follow if the powerpc-specific parts weren't
intertwined with the rest.
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: <stable@vger.kernel.org>
> ---
>
> v2: Rename flag to "no_64bit_msi" as suggested by Bjorn
>
> arch/powerpc/include/asm/pci-bridge.h | 2 --
> arch/powerpc/kernel/pci_64.c | 5 +----
> arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
> arch/powerpc/platforms/powernv/pci.c | 3 +--
> arch/powerpc/platforms/pseries/msi.c | 2 +-
> include/linux/pci.h | 1 +
> 6 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index 4ca90a3..725247b 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -159,8 +159,6 @@ struct pci_dn {
>
> int pci_ext_config_space; /* for pci devices */
>
> - bool force_32bit_msi;
> -
> struct pci_dev *pcidev; /* back-pointer to the pci device */
> #ifdef CONFIG_EEH
> struct eeh_dev *edev; /* eeh device */
> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> index 155013d..d41a831 100644
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
>
> static void quirk_radeon_32bit_msi(struct pci_dev *dev)
> {
> - struct pci_dn *pdn = pci_get_pdn(dev);
> -
> - if (pdn)
> - pdn->force_32bit_msi = true;
> + dev->no_64bit_msi = true;
> }
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index df241b1..a188bb8 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
> unsigned int is_64, struct msi_msg *msg)
> {
> struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
> - struct pci_dn *pdn = pci_get_pdn(dev);
> struct irq_data *idata;
> struct irq_chip *ichip;
> unsigned int xive_num = hwirq - phb->msi_base;
> @@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
> return -ENXIO;
>
> /* Force 32-bit MSI on some broken devices */
> - if (pdn && pdn->force_32bit_msi)
> + if (dev->no_64bit_msi)
> is_64 = 0;
>
> /* Assign XIVE to PE */
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index b854b57..89c6608 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
> {
> struct pci_controller *hose = pci_bus_to_host(pdev->bus);
> struct pnv_phb *phb = hose->private_data;
> - struct pci_dn *pdn = pci_get_pdn(pdev);
>
> - if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
> + if (pdev->no_64bit_msi && !phb->msi32_support)
> return -ENODEV;
>
> return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 18ff462..6fd96d8 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
> */
> again:
> if (type == PCI_CAP_ID_MSI) {
> - if (pdn->force_32bit_msi) {
> + if (pdev->no_64bit_msi) {
> rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
> if (rc < 0) {
> /*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 96453f9..fc938003 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -331,6 +331,7 @@ struct pci_dev {
> unsigned int is_added:1;
> unsigned int is_busmaster:1; /* device is busmaster */
> unsigned int no_msi:1; /* device may not use msi */
> + unsigned int no_64bit_msi:1; /* Device has broken 64-bit MSIs */
> unsigned int block_cfg_access:1; /* config space access is blocked */
> unsigned int broken_parity_status:1; /* Device generates false positive parity */
> unsigned int irq_reroute_variant:2; /* device needs IRQ rerouting variant */
>
>
>
^ permalink raw reply
* Re: [PATCH v2] powerpc: Speed up clear_page by unrolling it
From: Segher Boessenkool @ 2014-10-02 14:17 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev
In-Reply-To: <20141002154421.62073027@kryten>
On Thu, Oct 02, 2014 at 03:44:21PM +1000, Anton Blanchard wrote:
> This assumes cacheline sizes won't grow beyond 512 bytes or
> page sizes wont drop below 1kB,
Or a combination of those.
> Michael found that some versions of gcc produce quite bad code
> (all multiplies), so we give gcc a hand by using shifts and adds.
You can make the code a lot less cluttered as well as making the
generated code independent of compiler version by writing the setup
of twox..eightx in the asm block itself.
Segher
^ permalink raw reply
* [PATCH] powerpc/boot: Makefile cleanup
From: Michal Marek @ 2014-10-02 13:49 UTC (permalink / raw)
To: linuxppc-dev; +Cc: yamada.m, linux-kernel
The $(image-n) variable will never exist, because unset Kconfig options
are '' and not 'n'.
Signed-off-by: Michal Marek <mmarek@suse.cz>
---
arch/powerpc/boot/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index ccc25ed..c456d07 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -317,7 +317,7 @@ endif
# Allow extra targets to be added to the defconfig
image-y += $(subst ",,$(CONFIG_EXTRA_TARGETS))
-initrd- := $(patsubst zImage%, zImage.initrd%, $(image-n) $(image-))
+initrd- := $(patsubst zImage%, zImage.initrd%, $(image-))
initrd-y := $(patsubst zImage%, zImage.initrd%, \
$(patsubst dtbImage%, dtbImage.initrd%, \
$(patsubst simpleImage%, simpleImage.initrd%, \
--
1.8.4.5
^ permalink raw reply related
* Re: [PATCH v2 15/17] cxl: Userspace header file.
From: Benjamin Herrenschmidt @ 2014-10-02 12:42 UTC (permalink / raw)
To: Ian Munsie
Cc: cbe-oss-dev, Michael Neuling, arnd, Aneesh Kumar K.V,
linux-kernel, linuxppc-dev, anton, greg, jk
In-Reply-To: <1412243942-sup-5336@delenn.ozlabs.ibm.com>
On Thu, 2014-10-02 at 20:28 +1000, Ian Munsie wrote:
> Hey Michael,
>
> Excerpts from Michael Ellerman's message of 2014-10-02 16:02:37 +1000:
> > > +/* ioctls */
> > > +struct cxl_ioctl_start_work {
> > > + __u64 wed;
> > > + __u64 amr;
> > > + __u64 reserved1;
> > > + __u32 reserved2;
> > > + __s16 num_interrupts; /* -1 = use value from afu descriptor */
> > > + __u16 process_element; /* returned from kernel */
> > > + __u64 reserved3;
> > > + __u64 reserved4;
> > > + __u64 reserved5;
> > > + __u64 reserved6;
> >
> > Why so many reserved fields?
>
> The first two are reserved for the context save area (reserved1) and
> size (reserved2) of the "shared" (AKA time sliced) virtualisation model,
> which we don't yet support. That only leaves us with four reserved
> fields for anything that we haven't thought of or that the hardware team
> hasn't come up with yet ;-)
>
> > What mechanism is there that will allow you to ever unreserve them?
> >
> > ie. how does a new userspace detect that the kernel it's running on supports
> > new fields?
>
> The ioctl will return -EINVAL if any of them are set to non-zero values,
> so userspace can easily tell if it's running on an old kernel.
Not good enough in my experience. Throw in a flags field I'd say..
> > Or conversely how does a new kernel detect that userspace has passed it a
> > meaningful value in one of the previously reserved fields?
>
> They would have to be non-zero (certainly true of the context save
> area's size), or one could turn into a flags field or api version.
If you go that way you need to negociate as well latest compatible
etc...
> > > +#define CXL_MAGIC 0xCA
> > > +#define CXL_IOCTL_START_WORK _IOWR(CXL_MAGIC, 0x00, struct cxl_ioctl_start_work)
> >
> > What happened to 0x1 ?
>
> That was used to dynamically program the FPGA with a new AFU image, but
> we don't have anything to test it on yet and I'm not convinced that the
> procedure won't change by the time we do, so we pulled the code.
>
> We can repack the ioctl numbers easily enough... Will do :)
>
> > > +enum cxl_event_type {
> > > + CXL_EVENT_READ_FAIL = -1,
> >
> > I don't see this used?
>
> That was used in the userspace library to mark it's buffer as bad if the
> read() call failed for whatever reason... but you're right - it isn't
> used by the kernel and doesn't belong in this header. Will remove.
>
> > > +struct cxl_event_header {
> > > + __u32 type;
> > > + __u16 size;
> > > + __u16 process_element;
> > > + __u64 reserved1;
> > > + __u64 reserved2;
> > > + __u64 reserved3;
> > > +};
> >
> > Again lots of reserved fields?
>
> Figured it was better to have a bit more than we expect we might need
> just in case... We can reduce this if you feel it is excessive?
>
> In an earlier version of the code the kernel would fill out the header
> and not clear an event if a buffer was passed in that was too small, so
> userspace could realloc a larger buffer and try again. This made the API
> a bit more complex and our internal users weren't too keen on it, so we
> decided to use a fixed-size buffer and make it larger than we strictly
> needed so we have plenty of room for further expansion.
>
> > Rather than having the header included in every event, would it be clearer if
> > the cxl_event was:
> >
> > struct cxl_event {
> > struct cxl_event_header header;
> > union {
> > struct cxl_event_afu_interrupt irq;
> > struct cxl_event_data_storage fault;
> > struct cxl_event_afu_error afu_err;
> > };
> > };
>
> Sounds like a good idea to me :)
>
> Cheers,
> -Ian
^ permalink raw reply
* Re: [PATCH v2 3/4] sound/radeon: Move 64-bit MSI quirk from arch to driver
From: Benjamin Herrenschmidt @ 2014-10-02 12:09 UTC (permalink / raw)
To: Takashi Iwai
Cc: linuxppc-dev, Dave Airlie, Linux PCI, Brian King, Anton Blanchard,
Bjorn Helgaas, Yijing Wang, Alex Deucher
In-Reply-To: <s5hlhoy52c3.wl-tiwai@suse.de>
On Thu, 2014-10-02 at 09:59 +0200, Takashi Iwai wrote:
> > The attached updated patch only flags the affected asics.
>
> Alex's patch looks more comprehensive. For either patch,
>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
Absolutely. I'll give it a spin tomorrow to be 100% sure. From there,
it will need the previous patch moving the quirk so I'm thinking of
merging it via Bjorn or my tree unless objection...
Cheers,
Ben.
>
> thanks,
>
> Takashi
>
> >
> > Alex
> >
> > > ---
> > > arch/powerpc/kernel/pci_64.c | 6 ------
> > > sound/pci/hda/hda_intel.c | 10 ++++++++--
> > > sound/pci/hda/hda_priv.h | 1 +
> > > 3 files changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> > > index 5330f6d..b15194e 100644
> > > --- a/arch/powerpc/kernel/pci_64.c
> > > +++ b/arch/powerpc/kernel/pci_64.c
> > > @@ -266,9 +266,3 @@ int pcibus_to_node(struct pci_bus *bus)
> > > }
> > > EXPORT_SYMBOL(pcibus_to_node);
> > > #endif
> > > -
> > > -static void quirk_radeon_32bit_msi(struct pci_dev *dev)
> > > -{
> > > - dev->no_64bit_msi = true;
> > > -}
> > > -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index aa302fb..f91ba7f 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -296,7 +296,8 @@ enum {
> > >
> > > /* quirks for ATI/AMD HDMI */
> > > #define AZX_DCAPS_PRESET_ATI_HDMI \
> > > - (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB)
> > > + (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB|\
> > > + AZX_DCAPS_NO_MSI64)
> > >
> > > /* quirks for Nvidia */
> > > #define AZX_DCAPS_PRESET_NVIDIA \
> > > @@ -1505,9 +1506,14 @@ static int azx_first_init(struct azx *chip)
> > > return -ENXIO;
> > > }
> > >
> > > - if (chip->msi)
> > > + if (chip->msi) {
> > > + if (chip->driver_caps & AZX_DCAPS_NO_MSI64) {
> > > + dev_dbg(card->dev, "Disabling 64bit MSI\n");
> > > + pci->no_64bit_msi = true;
> > > + }
> > > if (pci_enable_msi(pci) < 0)
> > > chip->msi = 0;
> > > + }
> > >
> > > if (azx_acquire_irq(chip, 0) < 0)
> > > return -EBUSY;
> > > diff --git a/sound/pci/hda/hda_priv.h b/sound/pci/hda/hda_priv.h
> > > index 949cd43..5016014 100644
> > > --- a/sound/pci/hda/hda_priv.h
> > > +++ b/sound/pci/hda/hda_priv.h
> > > @@ -171,6 +171,7 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
> > > #define AZX_DCAPS_PM_RUNTIME (1 << 26) /* runtime PM support */
> > > #define AZX_DCAPS_I915_POWERWELL (1 << 27) /* HSW i915 powerwell support */
> > > #define AZX_DCAPS_CORBRP_SELF_CLEAR (1 << 28) /* CORBRP clears itself after reset */
> > > +#define AZX_DCAPS_NO_MSI64 (1 << 29) /* Stick to 32-bit MSIs */
> > >
> > > /* HD Audio class code */
> > > #define PCI_CLASS_MULTIMEDIA_HD_AUDIO 0x0403
> > >
> > >
> > >
> > >
^ permalink raw reply
* Re: [PATCH v2 15/17] cxl: Userspace header file.
From: Ian Munsie @ 2014-10-02 10:28 UTC (permalink / raw)
To: Michael Ellerman
Cc: cbe-oss-dev, Michael Neuling, arnd, Aneesh Kumar K.V, greg,
linux-kernel, linuxppc-dev, anton, jk
In-Reply-To: <20141002060237.E9963140180@ozlabs.org>
Hey Michael,
Excerpts from Michael Ellerman's message of 2014-10-02 16:02:37 +1000:
> > +/* ioctls */
> > +struct cxl_ioctl_start_work {
> > + __u64 wed;
> > + __u64 amr;
> > + __u64 reserved1;
> > + __u32 reserved2;
> > + __s16 num_interrupts; /* -1 = use value from afu descriptor */
> > + __u16 process_element; /* returned from kernel */
> > + __u64 reserved3;
> > + __u64 reserved4;
> > + __u64 reserved5;
> > + __u64 reserved6;
>
> Why so many reserved fields?
The first two are reserved for the context save area (reserved1) and
size (reserved2) of the "shared" (AKA time sliced) virtualisation model,
which we don't yet support. That only leaves us with four reserved
fields for anything that we haven't thought of or that the hardware team
hasn't come up with yet ;-)
> What mechanism is there that will allow you to ever unreserve them?
>
> ie. how does a new userspace detect that the kernel it's running on supports
> new fields?
The ioctl will return -EINVAL if any of them are set to non-zero values,
so userspace can easily tell if it's running on an old kernel.
> Or conversely how does a new kernel detect that userspace has passed it a
> meaningful value in one of the previously reserved fields?
They would have to be non-zero (certainly true of the context save
area's size), or one could turn into a flags field or api version.
> > +#define CXL_MAGIC 0xCA
> > +#define CXL_IOCTL_START_WORK _IOWR(CXL_MAGIC, 0x00, struct cxl_ioctl_start_work)
>
> What happened to 0x1 ?
That was used to dynamically program the FPGA with a new AFU image, but
we don't have anything to test it on yet and I'm not convinced that the
procedure won't change by the time we do, so we pulled the code.
We can repack the ioctl numbers easily enough... Will do :)
> > +enum cxl_event_type {
> > + CXL_EVENT_READ_FAIL = -1,
>
> I don't see this used?
That was used in the userspace library to mark it's buffer as bad if the
read() call failed for whatever reason... but you're right - it isn't
used by the kernel and doesn't belong in this header. Will remove.
> > +struct cxl_event_header {
> > + __u32 type;
> > + __u16 size;
> > + __u16 process_element;
> > + __u64 reserved1;
> > + __u64 reserved2;
> > + __u64 reserved3;
> > +};
>
> Again lots of reserved fields?
Figured it was better to have a bit more than we expect we might need
just in case... We can reduce this if you feel it is excessive?
In an earlier version of the code the kernel would fill out the header
and not clear an event if a buffer was passed in that was too small, so
userspace could realloc a larger buffer and try again. This made the API
a bit more complex and our internal users weren't too keen on it, so we
decided to use a fixed-size buffer and make it larger than we strictly
needed so we have plenty of room for further expansion.
> Rather than having the header included in every event, would it be clearer if
> the cxl_event was:
>
> struct cxl_event {
> struct cxl_event_header header;
> union {
> struct cxl_event_afu_interrupt irq;
> struct cxl_event_data_storage fault;
> struct cxl_event_afu_error afu_err;
> };
> };
Sounds like a good idea to me :)
Cheers,
-Ian
^ permalink raw reply
* Re: [PATCH v2 3/4] sound/radeon: Move 64-bit MSI quirk from arch to driver
From: Takashi Iwai @ 2014-10-02 7:59 UTC (permalink / raw)
To: Alex Deucher
Cc: linuxppc-dev, Dave Airlie, Brian King, Anton Blanchard,
Yijing Wang, Linux PCI, Bjorn Helgaas
In-Reply-To: <CADnq5_PhuFbRPgzAa_hEA1czTQjY9xdLFOWhL393DN4CYDnFng@mail.gmail.com>
At Wed, 1 Oct 2014 22:13:38 -0400,
Alex Deucher wrote:
>
> On Wed, Oct 1, 2014 at 8:34 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > A number of radeon cards have a HW limitation causing them to be
> > unable to generate the full 64-bit of address bits for MSIs. This
> > breaks MSIs on some platforms such as POWER machines.
> >
> > We used to have a powerpc specific quirk to address that on a
> > single card, but this doesn't scale very well, this is better
> > put under control of the drivers who know precisely what a given
> > HW revision can do.
> >
> > This moves the setting of the quirk flag to the audio driver.
> >
> > While recent ASICs have that problem fixed, they don't seem to
> > be listed in the PCI IDs of the current driver, so let's quirk all
> > the ATI HDMI for now. The consequences are nil on x86 anyway.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > CC: <stable@vger.kernel.org>
>
> The attached updated patch only flags the affected asics.
Alex's patch looks more comprehensive. For either patch,
Reviewed-by: Takashi Iwai <tiwai@suse.de>
thanks,
Takashi
>
> Alex
>
> > ---
> > arch/powerpc/kernel/pci_64.c | 6 ------
> > sound/pci/hda/hda_intel.c | 10 ++++++++--
> > sound/pci/hda/hda_priv.h | 1 +
> > 3 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> > index 5330f6d..b15194e 100644
> > --- a/arch/powerpc/kernel/pci_64.c
> > +++ b/arch/powerpc/kernel/pci_64.c
> > @@ -266,9 +266,3 @@ int pcibus_to_node(struct pci_bus *bus)
> > }
> > EXPORT_SYMBOL(pcibus_to_node);
> > #endif
> > -
> > -static void quirk_radeon_32bit_msi(struct pci_dev *dev)
> > -{
> > - dev->no_64bit_msi = true;
> > -}
> > -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index aa302fb..f91ba7f 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -296,7 +296,8 @@ enum {
> >
> > /* quirks for ATI/AMD HDMI */
> > #define AZX_DCAPS_PRESET_ATI_HDMI \
> > - (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB)
> > + (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB|\
> > + AZX_DCAPS_NO_MSI64)
> >
> > /* quirks for Nvidia */
> > #define AZX_DCAPS_PRESET_NVIDIA \
> > @@ -1505,9 +1506,14 @@ static int azx_first_init(struct azx *chip)
> > return -ENXIO;
> > }
> >
> > - if (chip->msi)
> > + if (chip->msi) {
> > + if (chip->driver_caps & AZX_DCAPS_NO_MSI64) {
> > + dev_dbg(card->dev, "Disabling 64bit MSI\n");
> > + pci->no_64bit_msi = true;
> > + }
> > if (pci_enable_msi(pci) < 0)
> > chip->msi = 0;
> > + }
> >
> > if (azx_acquire_irq(chip, 0) < 0)
> > return -EBUSY;
> > diff --git a/sound/pci/hda/hda_priv.h b/sound/pci/hda/hda_priv.h
> > index 949cd43..5016014 100644
> > --- a/sound/pci/hda/hda_priv.h
> > +++ b/sound/pci/hda/hda_priv.h
> > @@ -171,6 +171,7 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
> > #define AZX_DCAPS_PM_RUNTIME (1 << 26) /* runtime PM support */
> > #define AZX_DCAPS_I915_POWERWELL (1 << 27) /* HSW i915 powerwell support */
> > #define AZX_DCAPS_CORBRP_SELF_CLEAR (1 << 28) /* CORBRP clears itself after reset */
> > +#define AZX_DCAPS_NO_MSI64 (1 << 29) /* Stick to 32-bit MSIs */
> >
> > /* HD Audio class code */
> > #define PCI_CLASS_MULTIMEDIA_HD_AUDIO 0x0403
> >
> >
> >
> >
^ permalink raw reply
* Re: [PATCH v2 09/17] powerpc/mm: Add new hash_page_mm()
From: Michael Neuling @ 2014-10-02 7:39 UTC (permalink / raw)
To: Michael Ellerman
Cc: cbe-oss-dev, arnd, Aneesh Kumar K.V, greg, linux-kernel, imunsie,
linuxppc-dev, anton, jk
In-Reply-To: <20141002034855.8E3D514017E@ozlabs.org>
On Thu, 2014-10-02 at 13:48 +1000, Michael Ellerman wrote:
> On Tue, 2014-30-09 at 10:34:58 UTC, Michael Neuling wrote:
> > From: Ian Munsie <imunsie@au1.ibm.com>
> >=20
> > This adds a new function hash_page_mm() based on the existing hash_page=
().
> > This version allows any struct mm to be passed in, rather than assuming
> > current. This is useful for servicing co-processor faults which are no=
t in the
> > context of the current running process.
>=20
> I'm not a big fan. hash_page() is already a train wreck, and this doesn't=
make
> it any better.
I can document it to make the situation a bit better. It's certainly
not clear which one to use here and under what circumstances. It's
basically ask benh territory. =20
> > diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_uti=
ls_64.c
> > index bbdb054..0a5c8c0 100644
> > --- a/arch/powerpc/mm/hash_utils_64.c
> > +++ b/arch/powerpc/mm/hash_utils_64.c
> > @@ -904,7 +904,7 @@ void demote_segment_4k(struct mm_struct *mm, unsign=
ed long addr)
> > return;
> > slice_set_range_psize(mm, addr, 1, MMU_PAGE_4K);
> > copro_flush_all_slbs(mm);
> > - if (get_paca_psize(addr) !=3D MMU_PAGE_4K) {
> > + if ((get_paca_psize(addr) !=3D MMU_PAGE_4K) && (current->mm =3D=3D mm=
)) {
> > get_paca()->context =3D mm->context;
> > slb_flush_and_rebolt();
>=20
> This is a bit fishy.
>=20
> If that mm is currently running on another cpu you just failed to update =
it's
> paca. But I think the call to check_paca_psize() in hash_page() will save=
you
> on that cpu.
>=20
> In fact we might be able to remove that synchronisation from
> demote_segment_4k() and always leave it up to check_paca_psize()?
Aneesh asked the same thing for v1 and we convinced ourselves it was ok.
I said this at the time...
I had a chat to benh offline about this and he thinks it's fine. A
running process in the same mm context will either have hit this mapping
or not. If it's hit it, the page will be invalidated and it'll come in
via hash_page and have it's segment demoted also (and paca updated). If
it hasn't hit, again it'll come into hash_page() and get demoted also.
> > @@ -989,26 +989,24 @@ static void check_paca_psize(unsigned long ea, st=
ruct mm_struct *mm,
> > * -1 - critical hash insertion error
> > * -2 - access not permitted by subpage protection mechanism
> > */
> > -int hash_page(unsigned long ea, unsigned long access, unsigned long tr=
ap)
> > +int hash_page_mm(struct mm_struct *mm, unsigned long ea, unsigned long=
access, unsigned long trap)
> > {
> > enum ctx_state prev_state =3D exception_enter();
> > pgd_t *pgdir;
> > unsigned long vsid;
> > - struct mm_struct *mm;
> > pte_t *ptep;
> > unsigned hugeshift;
> > const struct cpumask *tmp;
> > int rc, user_region =3D 0, local =3D 0;
> > int psize, ssize;
> > =20
> > - DBG_LOW("hash_page(ea=3D%016lx, access=3D%lx, trap=3D%lx\n",
> > - ea, access, trap);
> > + DBG_LOW("%s(ea=3D%016lx, access=3D%lx, trap=3D%lx\n",
> > + __func__, ea, access, trap);
> > =20
> > /* Get region & vsid */
> > switch (REGION_ID(ea)) {
> > case USER_REGION_ID:
> > user_region =3D 1;
> > - mm =3D current->mm;
> > if (! mm) {
> > DBG_LOW(" user region with no mm !\n");
> > rc =3D 1;
>=20
> What about the VMALLOC case where we do:
> mm =3D &init_mm;
> =09
> Is that what you want? It seems odd that you pass an mm to the routine, b=
ut
> then potentially it ends up using a different mm after all depending on t=
he
> address.
Good point. We have hash_page() still. I can make that check in there
and decide which mm to use and pass that to hash_page_mm(). Then we
always use mm in hash_page_mm(). hash_page() will then look like this:=20
int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
{
struct mm_struct *mm =3D current->mm;
if (REGION_ID(ea) =3D=3D VMALLOC_REGION_ID)
mm =3D &init_mm;
return hash_page_mm(mm, ea, access, trap);
}
Mikey
^ permalink raw reply
* Re: powerpc: Print instruction when logging unhandled exceptions
From: Michael Ellerman @ 2014-10-02 7:14 UTC (permalink / raw)
To: Anton Blanchard, benh, paulus; +Cc: linuxppc-dev
In-Reply-To: <1411621545-19310-1-git-send-email-anton@samba.org>
On Thu, 2014-25-09 at 05:05:45 UTC, Anton Blanchard wrote:
> It is often useful to see the instruction that caused an unhandled
> exception.
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 0dc43f9..27e30c8 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -252,9 +252,18 @@ void _exception(int signr, struct pt_regs *regs, int code, unsigned long addr)
> }
>
> if (show_unhandled_signals && unhandled_signal(current, signr)) {
> + u32 __user *nia = (u32 __user *)regs->nip;
> + u32 insn = 0;
> +
> + pagefault_disable();
> + if (!access_ok(VERIFY_READ, nia, sizeof(*nia)) ||
> + __get_user_inatomic(insn, nia))
> + insn = 0xffffffffUL;
> + pagefault_enable();
Can you add a comment explaining that interesting construct?
Looks like we do something similar in the perf callchain code, though without
the access_ok() check?
cheers
^ permalink raw reply
* Re: [PATCH v2 09/17] powerpc/mm: Add new hash_page_mm()
From: Michael Neuling @ 2014-10-02 7:10 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: cbe-oss-dev, arnd, greg, linux-kernel, linuxppc-dev, anton,
imunsie, jk
In-Reply-To: <8761g4nn12.fsf@linux.vnet.ibm.com>
On Wed, 2014-10-01 at 15:13 +0530, Aneesh Kumar K.V wrote:
> Michael Neuling <mikey@neuling.org> writes:
>=20
> > From: Ian Munsie <imunsie@au1.ibm.com>
> >
> > This adds a new function hash_page_mm() based on the existing hash_page=
().
> > This version allows any struct mm to be passed in, rather than assuming
> > current. This is useful for servicing co-processor faults which are no=
t in the
> > context of the current running process.
> >
> > We need to be careful here as the current hash_page() assumes current i=
n a few
> > places.
>=20
> It would be nice to document the rules here. So when we try to add a hash
> page entry, and if that result in demotion of the segment are we suppose =
to
> flush slbs ?=20
Yeah, we found it sucky to understand. The current documentation is
"buy benh a beer and ask him" which doesn't scale very well unless
you're benh and you like beer.
> Also why would one want to hash anything other
> than current->mm ? How will this get called ?=20
We are calling this on behalf of a co-processor (eg cxl). The mm this
is currently associated with may not be running on a cpu. =20
> May be they are explained in later patches. But can we also explain it
> here.=20
Ok, I'll add something (mpe had the same question).
Mikey
>=20
> >
> > Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > ---
> > arch/powerpc/include/asm/mmu-hash64.h | 1 +
> > arch/powerpc/mm/hash_utils_64.c | 22 ++++++++++++++--------
> > 2 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/mmu-hash64.h b/arch/powerpc/inclu=
de/asm/mmu-hash64.h
> > index 6d0b7a2..f84e5a5 100644
> > --- a/arch/powerpc/include/asm/mmu-hash64.h
> > +++ b/arch/powerpc/include/asm/mmu-hash64.h
> > @@ -322,6 +322,7 @@ extern int __hash_page_64K(unsigned long ea, unsign=
ed long access,
> > unsigned int local, int ssize);
> > struct mm_struct;
> > unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int =
trap);
> > +extern int hash_page_mm(struct mm_struct *mm, unsigned long ea, unsign=
ed long access, unsigned long trap);
> > extern int hash_page(unsigned long ea, unsigned long access, unsigned =
long trap);
> > int __hash_page_huge(unsigned long ea, unsigned long access, unsigned =
long vsid,
> > pte_t *ptep, unsigned long trap, int local, int ssize,
> > diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_uti=
ls_64.c
> > index bbdb054..0a5c8c0 100644
> > --- a/arch/powerpc/mm/hash_utils_64.c
> > +++ b/arch/powerpc/mm/hash_utils_64.c
> > @@ -904,7 +904,7 @@ void demote_segment_4k(struct mm_struct *mm, unsign=
ed long addr)
> > return;
> > slice_set_range_psize(mm, addr, 1, MMU_PAGE_4K);
> > copro_flush_all_slbs(mm);
> > - if (get_paca_psize(addr) !=3D MMU_PAGE_4K) {
> > + if ((get_paca_psize(addr) !=3D MMU_PAGE_4K) && (current->mm =3D=3D mm=
)) {
> > get_paca()->context =3D mm->context;
> > slb_flush_and_rebolt();
> > }
> > @@ -989,26 +989,24 @@ static void check_paca_psize(unsigned long ea, st=
ruct mm_struct *mm,
> > * -1 - critical hash insertion error
> > * -2 - access not permitted by subpage protection mechanism
> > */
> > -int hash_page(unsigned long ea, unsigned long access, unsigned long tr=
ap)
> > +int hash_page_mm(struct mm_struct *mm, unsigned long ea, unsigned long=
access, unsigned long trap)
> > {
> > enum ctx_state prev_state =3D exception_enter();
> > pgd_t *pgdir;
> > unsigned long vsid;
> > - struct mm_struct *mm;
> > pte_t *ptep;
> > unsigned hugeshift;
> > const struct cpumask *tmp;
> > int rc, user_region =3D 0, local =3D 0;
> > int psize, ssize;
> > =20
> > - DBG_LOW("hash_page(ea=3D%016lx, access=3D%lx, trap=3D%lx\n",
> > - ea, access, trap);
> > + DBG_LOW("%s(ea=3D%016lx, access=3D%lx, trap=3D%lx\n",
> > + __func__, ea, access, trap);
> > =20
> > /* Get region & vsid */
> > switch (REGION_ID(ea)) {
> > case USER_REGION_ID:
> > user_region =3D 1;
> > - mm =3D current->mm;
> > if (! mm) {
> > DBG_LOW(" user region with no mm !\n");
> > rc =3D 1;
> > @@ -1104,7 +1102,8 @@ int hash_page(unsigned long ea, unsigned long acc=
ess, unsigned long trap)
> > WARN_ON(1);
> > }
> > #endif
> > - check_paca_psize(ea, mm, psize, user_region);
> > + if (current->mm =3D=3D mm)
> > + check_paca_psize(ea, mm, psize, user_region);
> > =20
> > goto bail;
> > }
> > @@ -1145,7 +1144,8 @@ int hash_page(unsigned long ea, unsigned long acc=
ess, unsigned long trap)
> > }
> > }
> > =20
> > - check_paca_psize(ea, mm, psize, user_region);
> > + if (current->mm =3D=3D mm)
> > + check_paca_psize(ea, mm, psize, user_region);
> > #endif /* CONFIG_PPC_64K_PAGES */
> > =20
> > #ifdef CONFIG_PPC_HAS_HASH_64K
> > @@ -1180,6 +1180,12 @@ bail:
> > exception_exit(prev_state);
> > return rc;
> > }
> > +EXPORT_SYMBOL_GPL(hash_page_mm);
> > +
> > +int hash_page(unsigned long ea, unsigned long access, unsigned long tr=
ap)
> > +{
> > + return hash_page_mm(current->mm, ea, access, trap);
> > +}
> > EXPORT_SYMBOL_GPL(hash_page);
> > =20
> > void hash_preload(struct mm_struct *mm, unsigned long ea,
> > --=20
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel"=
in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>=20
^ permalink raw reply
* Re: [PATCH v2 14/17] cxl: Driver code for powernv PCIe based cards for userspace access
From: Michael Ellerman @ 2014-10-02 7:02 UTC (permalink / raw)
To: Michael Neuling, greg, arnd, benh
Cc: cbe-oss-dev, mikey, Aneesh Kumar K.V, imunsie, linux-kernel,
linuxppc-dev, jk, anton
In-Reply-To: <1412073306-13812-15-git-send-email-mikey@neuling.org>
On Tue, 2014-30-09 at 10:35:03 UTC, Michael Neuling wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This is the core of the cxl driver.
>
> It adds support for using cxl cards in the powernv environment only (no guest
> support).
Which means on bare metal on power8 for the peanut gallery.
> It allows access to cxl accelerators by userspace using
> /dev/cxl/afu0.0 char device.
devices ?
> The kernel driver has no knowledge of the acceleration function.
.. has no knowledge of the function implemented by the accelerator ?
> It only provides services to userspace via the /dev/cxl/afu0.0 device.
Provides what services?
> This will compile to two modules. cxl.ko provides the core cxl functionality
> and userspace API. cxl-pci.ko provides the PCI driver driver functionality the
> powernv environment.
Last sentence doesn't hold together.
> Documentation of the cxl hardware architecture and userspace API is provided in
> subsequent patches.
Partial review below.
So some meta comments.
Can you get rid of all the foo_t's. That should just be a search and replace.
Can we drop the indirection layers for now. They make it quite a bit harder to
follow the code, and it sounds like you're not 100% sure they're the right
abstraction anyway. When you add another backend/driver/whatever you can readd
just the abstractions you need.
/*
* Block comments look like this.
*/
> diff --git a/drivers/misc/cxl/context.c b/drivers/misc/cxl/context.c
> new file mode 100644
> index 0000000..9206ca4
> --- /dev/null
> +++ b/drivers/misc/cxl/context.c
> @@ -0,0 +1,171 @@
> +/*
> + * Copyright 2014 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#undef DEBUG
Drop this please.
Instead can you add:
#define pr_fmt(fmt) "cxl: " fmt
To each file, so it's clear where your pr_xxxs() come from.
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/bitmap.h>
> +#include <linux/sched.h>
> +#include <linux/pid.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/debugfs.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
> +#include <asm/cputable.h>
> +#include <asm/current.h>
> +#include <asm/copro.h>
> +
> +#include "cxl.h"
> +
> +/*
> + * Allocates space for a CXL context.
> + */
> +struct cxl_context_t *cxl_context_alloc(void)
> +{
> + return kzalloc(sizeof(struct cxl_context_t), GFP_KERNEL);
> +}
> +/*
> + * Initialises a CXL context.
> + */
> +int cxl_context_init(struct cxl_context_t *ctx, struct cxl_afu_t *afu, bool master)
> +{
> + int i;
> +
> + spin_lock_init(&ctx->sst_lock);
> + ctx->sstp = NULL;
> + ctx->afu = afu;
> + ctx->master = master;
> + ctx->pid = get_pid(get_task_pid(current, PIDTYPE_PID));
> +
> + INIT_WORK(&ctx->fault_work, cxl_handle_fault);
> +
> + init_waitqueue_head(&ctx->wq);
> + spin_lock_init(&ctx->lock);
> +
> + ctx->irq_bitmap = NULL;
> + ctx->pending_irq = false;
> + ctx->pending_fault = false;
> + ctx->pending_afu_err = false;
> +
> + ctx->status = OPENED;
> +
> + idr_preload(GFP_KERNEL);
> + spin_lock(&afu->contexts_lock);
> + i = idr_alloc(&ctx->afu->contexts_idr, ctx, 0,
> + ctx->afu->num_procs, GFP_NOWAIT);
> + spin_unlock(&afu->contexts_lock);
> + idr_preload_end();
> + if (i < 0)
> + return i;
> +
> + ctx->ph = i;
> + ctx->elem = &ctx->afu->spa[i];
> + ctx->pe_inserted = false;
> + return 0;
> +}
> +
> +/*
> + * Map a per-context mmio space into the given vma.
> + */
> +int cxl_context_iomap(struct cxl_context_t *ctx, struct vm_area_struct *vma)
> +{
> + u64 len = vma->vm_end - vma->vm_start;
> + len = min(len, ctx->psn_size);
> +
> + if (ctx->afu->current_model == CXL_MODEL_DEDICATED) {
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + return vm_iomap_memory(vma, ctx->afu->psn_phys, ctx->afu->adapter->ps_size);
Why don't we use len here?
> + }
> +
> + /* make sure there is a valid per process space for this AFU */
> + if ((ctx->master && !ctx->afu->psa) || (!ctx->afu->pp_psa)) {
What the hell are psa and pp_psa ?
> + pr_devel("AFU doesn't support mmio space\n");
> + return -EINVAL;
> + }
> +
> + /* Can't mmap until the AFU is enabled */
> + if (!ctx->afu->enabled)
> + return -EBUSY;
afu_mmap() already checked status == STARTED.
Is EBUSY the right return code?
> + pr_devel("%s: mmio physical: %llx pe: %i master:%i\n", __func__,
> + ctx->psn_phys, ctx->ph , ctx->master);
> +
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + return vm_iomap_memory(vma, ctx->psn_phys, len);
> +}
> +
> +/*
> + * Detach a context from the hardware. This disables interrupts and doesn't
> + * return until all outstanding interrupts for this context have completed. The
> + * hardware should no longer access *ctx after this has returned.
> + */
> +static void __detach_context(struct cxl_context_t *ctx)
> +{
> + unsigned long flags;
> + enum cxl_context_status status;
> +
> + spin_lock_irqsave(&ctx->sst_lock, flags);
> + status = ctx->status;
> + ctx->status = CLOSED;
> + spin_unlock_irqrestore(&ctx->sst_lock, flags);
You take sst_lock here, before manipulating ctx->status. But I see lots of
places where you check status without taking any lock. So I'm a bit confused by
that.
At first glance it looks like we could race with afu_ioctl_start_work(), which
sets status to STARTED. But the only place we're called from is
cxl_context_detach(), from afu_release(), and that should only run once the
ioctl is finished AIUI. So that looks OK.
But some commentary would be good, especially if this is ever called via a
different path.
> + if (status != STARTED)
> + return;
> +
> + WARN_ON(cxl_ops->detach_process(ctx));
As discussed offline, this can fail, and the device might continue generating
interrupts even though we asked it not to.
Once you release the irqs below you'll get warnings from the xics code. Until
those virq numbers are handed out to someone else, at which point hilarity will
ensue.
It might be better to just warn and bail if detach fails, and leave the ctx
lying around?
> + afu_release_irqs(ctx);
> + flush_work(&ctx->fault_work); /* Only needed for dedicated process */
> + wake_up_all(&ctx->wq);
> +}
> +
> +/*
> + * Detach the given context from the AFU. This doesn't actually
> + * free the context but it should stop the context running in hardware
> + * (ie. prevent this context from generating any further interrupts
> + * so that it can be freed).
> + */
> +void cxl_context_detach(struct cxl_context_t *ctx)
> +{
> + __detach_context(ctx);
> +}
Why does this exist, or why does __detach_context() exist?
> +
> +/*
> + * Detach all contexts on the given AFU.
> + */
> +void cxl_context_detach_all(struct cxl_afu_t *afu)
> +{
> + struct cxl_context_t *ctx;
> + int tmp;
> +
Some commentary on why you're using rcu_read_lock() would be good. I know, but
I'll have forgotten by next week.
> + rcu_read_lock();
> + idr_for_each_entry(&afu->contexts_idr, ctx, tmp)
> + __detach_context(ctx);
> + rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(cxl_context_detach_all);
> +static int afu_release(struct inode *inode, struct file *file)
> +{
> + struct cxl_context_t *ctx = file->private_data;
> +
> + pr_devel("%s: closing cxl file descriptor. pe: %i\n",
> + __func__, ctx->ph);
> + cxl_context_detach(ctx);
> +
> + module_put(ctx->afu->adapter->driver->module);
This potentially drops the last reference on cxl-pci.ko.
cxl_remove() (in cxl-pci.ko) calls back into cxl_remove_afu() and
cxl_remove_adapter().
I *think* that's OK, because you won't be able to finish cxl_remove() until you
remove the afu cdev, and presumably that can't happen until you return from
release.
> + put_device(&ctx->afu->dev);
> +
> + /* It should be safe to remove the context now */
> + cxl_context_free(ctx);
My other worry is that it's not until here that you remove the ctx from the
idr. And so up until this point the ctx could be found in the idr and used by
someone.
I think it would be better to remove the ctx from the idr earlier, before you
start tearing things down. Perhaps even in cxl_context_detach().
> + cxl_ctx_put();
> + return 0;
> +}
> +static ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> + loff_t *off)
> +{
> + struct cxl_context_t *ctx = file->private_data;
> + struct cxl_event event;
> + unsigned long flags;
> + ssize_t size;
> + DEFINE_WAIT(wait);
> +
> + if (count < sizeof(struct cxl_event_header))
> + return -EINVAL;
This could use some love. The locking in here is pretty funky, and not good
funky.
> + while (1) {
> + spin_lock_irqsave(&ctx->lock, flags);
> + if (ctx->pending_irq || ctx->pending_fault ||
> + ctx->pending_afu_err || (ctx->status == CLOSED))
> + break;
> + spin_unlock_irqrestore(&ctx->lock, flags);
> +
> + if (file->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + prepare_to_wait(&ctx->wq, &wait, TASK_INTERRUPTIBLE);
> + if (!(ctx->pending_irq || ctx->pending_fault ||
> + ctx->pending_afu_err || (ctx->status == CLOSED))) {
> + pr_devel("afu_read going to sleep...\n");
> + schedule();
> + pr_devel("afu_read woken up\n");
> + }
> + finish_wait(&ctx->wq, &wait);
> +
> + if (signal_pending(current))
> + return -ERESTARTSYS;
> + }
> +
> + memset(&event, 0, sizeof(event));
> + event.header.process_element = ctx->ph;
> + if (ctx->pending_irq) {
> + pr_devel("afu_read delivering AFU interrupt\n");
> + event.header.size = sizeof(struct cxl_event_afu_interrupt);
> + event.header.type = CXL_EVENT_AFU_INTERRUPT;
> + event.irq.irq = find_first_bit(ctx->irq_bitmap, ctx->irq_count) + 1;
> +
> + /* Only clear the IRQ if we can send the whole event: */
> + if (count >= event.header.size) {
> + clear_bit(event.irq.irq - 1, ctx->irq_bitmap);
> + if (bitmap_empty(ctx->irq_bitmap, ctx->irq_count))
> + ctx->pending_irq = false;
> + }
> + } else if (ctx->pending_fault) {
> + pr_devel("afu_read delivering data storage fault\n");
> + event.header.size = sizeof(struct cxl_event_data_storage);
> + event.header.type = CXL_EVENT_DATA_STORAGE;
> + event.fault.addr = ctx->fault_addr;
> +
> + /* Only clear the fault if we can send the whole event: */
> + if (count >= event.header.size)
> + ctx->pending_fault = false;
> + } else if (ctx->pending_afu_err) {
> + pr_devel("afu_read delivering afu error\n");
> + event.header.size = sizeof(struct cxl_event_afu_error);
> + event.header.type = CXL_EVENT_AFU_ERROR;
> + event.afu_err.err = ctx->afu_err;
> +
> + /* Only clear the fault if we can send the whole event: */
> + if (count >= event.header.size)
> + ctx->pending_afu_err = false;
> + } else if (ctx->status == CLOSED) {
> + pr_devel("afu_read fatal error\n");
> + spin_unlock_irqrestore(&ctx->lock, flags);
> + return -EIO;
> + } else
> + WARN(1, "afu_read must be buggy\n");
> +
> + spin_unlock_irqrestore(&ctx->lock, flags);
> +
> + size = min_t(size_t, count, event.header.size);
> + copy_to_user(buf, &event, size);
> +
> + return size;
> +}
> diff --git a/drivers/misc/cxl/irq.c b/drivers/misc/cxl/irq.c
> new file mode 100644
> index 0000000..3e01e1d
> --- /dev/null
> +++ b/drivers/misc/cxl/irq.c
...
> +
> +void afu_release_irqs(struct cxl_context_t *ctx)
> +{
> + irq_hw_number_t hwirq;
> + unsigned int virq;
> + int r, i;
> +
> + for (r = 1; r < CXL_IRQ_RANGES; r++) {
> + hwirq = ctx->irqs.offset[r];
> + for (i = 0; i < ctx->irqs.range[r]; hwirq++, i++) {
> + virq = irq_find_mapping(NULL, hwirq);
> + if (virq)
> + cxl_unmap_irq(virq, ctx);
> + }
> + }
> +
> + ctx->afu->adapter->driver->release_irq_ranges(&ctx->irqs, ctx->afu->adapter);
Do we need this many levels of indirection?
cheers
^ permalink raw reply
* Re: [PATCH v2] drivers: char: hw_random: printk replacement
From: Herbert Xu @ 2014-10-02 6:53 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Olof Johansson, linuxppc-dev, linux-kernel, linux-geode,
Matt Mackall
In-Reply-To: <1410793281-30305-1-git-send-email-sudipm.mukherjee@gmail.com>
On Mon, Sep 15, 2014 at 08:31:20PM +0530, Sudip Mukherjee wrote:
> as pr_* macros are more preffered over printk, so printk replaced with corresponding pr_* macros
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>
> The replacement was done by a bash script to avoid copy paste error. The script is as follows :
>
> OLD="printk(KERN_ERR \?"
> OLD1="printk(KERN_NOTICE \?"
> OLD2="printk(KERN_WARNING \?"
> OLD3="printk(KERN_INFO \?"
> NEW="pr_err("
> NEW1="pr_notice("
> NEW2="pr_warn("
> NEW3="pr_info("
> TFILE="/tmp/out.tmp.$$"
> for f in *.c
> do
> sed -e "s/$OLD/$NEW/g" -e "s/$OLD1/$NEW1/g" -e "s/$OLD2/$NEW2/g" -e "s/$OLD3/$NEW3/g" "$f" > $TFILE && mv $TFILE "$f"
> done
>
> exception is intel-rng.c .
> in v1 the build failed due to intel-rng.c .
> it has been corrected and has been carefully build tested.
>
> drivers/char/hw_random/amd-rng.c | 4 ++--
> drivers/char/hw_random/geode-rng.c | 4 ++--
> drivers/char/hw_random/intel-rng.c | 13 ++++++-------
> drivers/char/hw_random/pasemi-rng.c | 2 +-
> drivers/char/hw_random/pseries-rng.c | 2 +-
> drivers/char/hw_random/via-rng.c | 8 ++++----
> 6 files changed, 16 insertions(+), 17 deletions(-)
Patch applied.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v2 10/17] powerpc/mm: Merge vsid calculation in hash_page() and copro_data_segment()
From: Michael Neuling @ 2014-10-02 6:44 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: cbe-oss-dev, arnd, greg, linux-kernel, linuxppc-dev, anton,
imunsie, jk
In-Reply-To: <87wq8km7w0.fsf@linux.vnet.ibm.com>
On Wed, 2014-10-01 at 15:25 +0530, Aneesh Kumar K.V wrote:
> Michael Neuling <mikey@neuling.org> writes:
>=20
> > From: Ian Munsie <imunsie@au1.ibm.com>
> >
> > The vsid calculation between hash_page() and copro_data_segment() are v=
ery
> > similar. This merges these two different versions.
> >
> > Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > ---
> > arch/powerpc/include/asm/mmu-hash64.h | 2 ++
> > arch/powerpc/mm/copro_fault.c | 45 ++++++--------------------
> > arch/powerpc/mm/hash_utils_64.c | 61 ++++++++++++++++++++++-----=
--------
> > 3 files changed, 50 insertions(+), 58 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/mmu-hash64.h b/arch/powerpc/inclu=
de/asm/mmu-hash64.h
> > index f84e5a5..bf43fb0 100644
> > --- a/arch/powerpc/include/asm/mmu-hash64.h
> > +++ b/arch/powerpc/include/asm/mmu-hash64.h
> > @@ -322,6 +322,8 @@ extern int __hash_page_64K(unsigned long ea, unsign=
ed long access,
> > unsigned int local, int ssize);
> > struct mm_struct;
> > unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int =
trap);
> > +int calculate_vsid(struct mm_struct *mm, u64 ea,
> > + u64 *vsid, int *psize, int *ssize);
> > extern int hash_page_mm(struct mm_struct *mm, unsigned long ea, unsign=
ed long access, unsigned long trap);
> > extern int hash_page(unsigned long ea, unsigned long access, unsigned =
long trap);
> > int __hash_page_huge(unsigned long ea, unsigned long access, unsigned =
long vsid,
> > diff --git a/arch/powerpc/mm/copro_fault.c b/arch/powerpc/mm/copro_faul=
t.c
> > index 939abdf..ba8bf8e 100644
> > --- a/arch/powerpc/mm/copro_fault.c
> > +++ b/arch/powerpc/mm/copro_fault.c
> > @@ -94,45 +94,18 @@ EXPORT_SYMBOL_GPL(copro_handle_mm_fault);
> > =20
> > int copro_data_segment(struct mm_struct *mm, u64 ea, u64 *esid, u64 *v=
sid)
> > {
> > - int psize, ssize;
> > + int psize, ssize, rc;
> > =20
> > *esid =3D (ea & ESID_MASK) | SLB_ESID_V;
> > =20
> > - switch (REGION_ID(ea)) {
> > - case USER_REGION_ID:
> > - pr_devel("copro_data_segment: 0x%llx -- USER_REGION_ID\n", ea);
> > -#ifdef CONFIG_PPC_MM_SLICES
> > - psize =3D get_slice_psize(mm, ea);
> > -#else
> > - psize =3D mm->context.user_psize;
> > -#endif
> > - ssize =3D user_segment_size(ea);
> > - *vsid =3D (get_vsid(mm->context.id, ea, ssize)
> > - << slb_vsid_shift(ssize)) | SLB_VSID_USER;
> > - break;
> > - case VMALLOC_REGION_ID:
> > - pr_devel("copro_data_segment: 0x%llx -- VMALLOC_REGION_ID\n", ea);
> > - if (ea < VMALLOC_END)
> > - psize =3D mmu_vmalloc_psize;
> > - else
> > - psize =3D mmu_io_psize;
> > - ssize =3D mmu_kernel_ssize;
> > - *vsid =3D (get_kernel_vsid(ea, mmu_kernel_ssize)
> > - << SLB_VSID_SHIFT) | SLB_VSID_KERNEL;
> > - break;
> > - case KERNEL_REGION_ID:
> > - pr_devel("copro_data_segment: 0x%llx -- KERNEL_REGION_ID\n", ea);
> > - psize =3D mmu_linear_psize;
> > - ssize =3D mmu_kernel_ssize;
> > - *vsid =3D (get_kernel_vsid(ea, mmu_kernel_ssize)
> > - << SLB_VSID_SHIFT) | SLB_VSID_KERNEL;
> > - break;
> > - default:
> > - /* Future: support kernel segments so that drivers can use the
> > - * CoProcessors */
> > - pr_debug("invalid region access at %016llx\n", ea);
> > - return 1;
> > - }
> > + rc =3D calculate_vsid(mm, ea, vsid, &psize, &ssize);
> > + if (rc)
> > + return rc;
> > + if (REGION_ID(ea) =3D=3D USER_REGION_ID)
> > + *vsid =3D (*vsid << slb_vsid_shift(ssize)) | SLB_VSID_USER;
> > + else
> > + *vsid =3D (*vsid << SLB_VSID_SHIFT) | SLB_VSID_KERNEL;
> > +
> > *vsid |=3D mmu_psize_defs[psize].sllp |
> > ((ssize =3D=3D MMU_SEGSIZE_1T) ? SLB_VSID_B_1T : 0);
> > =20
> > diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_uti=
ls_64.c
> > index 0a5c8c0..3fa81ca 100644
> > --- a/arch/powerpc/mm/hash_utils_64.c
> > +++ b/arch/powerpc/mm/hash_utils_64.c
> > @@ -983,6 +983,38 @@ static void check_paca_psize(unsigned long ea, str=
uct mm_struct *mm,
> > }
> > }
> > =20
> > +int calculate_vsid(struct mm_struct *mm, u64 ea,
> > + u64 *vsid, int *psize, int *ssize)
> > +{
> > + switch (REGION_ID(ea)) {
> > + case USER_REGION_ID:
> > + pr_devel("%s: 0x%llx -- USER_REGION_ID\n", __func__, ea);
> > + *psize =3D get_slice_psize(mm, ea);
> > + *ssize =3D user_segment_size(ea);
> > + *vsid =3D get_vsid(mm->context.id, ea, *ssize);
> > + return 0;
> > + case VMALLOC_REGION_ID:
> > + pr_devel("%s: 0x%llx -- VMALLOC_REGION_ID\n", __func__, ea);
> > + if (ea < VMALLOC_END)
> > + *psize =3D mmu_vmalloc_psize;
> > + else
> > + *psize =3D mmu_io_psize;
> > + *ssize =3D mmu_kernel_ssize;
> > + *vsid =3D get_kernel_vsid(ea, mmu_kernel_ssize);
> > + return 0;
> > + case KERNEL_REGION_ID:
> > + pr_devel("%s: 0x%llx -- KERNEL_REGION_ID\n", __func__, ea);
> > + *psize =3D mmu_linear_psize;
> > + *ssize =3D mmu_kernel_ssize;
> > + *vsid =3D get_kernel_vsid(ea, mmu_kernel_ssize);
> > + return 0;
> > + default:
> > + pr_debug("%s: invalid region access at %016llx\n", __func__, ea);
> > + return 1;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(calculate_vsid);
> > +
> > /* Result code is:
> > * 0 - handled
> > * 1 - normal page fault
> > @@ -993,7 +1025,7 @@ int hash_page_mm(struct mm_struct *mm, unsigned lo=
ng ea, unsigned long access, u
> > {
> > enum ctx_state prev_state =3D exception_enter();
> > pgd_t *pgdir;
> > - unsigned long vsid;
> > + u64 vsid;
> > pte_t *ptep;
> > unsigned hugeshift;
> > const struct cpumask *tmp;
> > @@ -1003,35 +1035,20 @@ int hash_page_mm(struct mm_struct *mm, unsigned=
long ea, unsigned long access, u
> > DBG_LOW("%s(ea=3D%016lx, access=3D%lx, trap=3D%lx\n",
> > __func__, ea, access, trap);
> > =20
> > - /* Get region & vsid */
> > - switch (REGION_ID(ea)) {
> > - case USER_REGION_ID:
> > + /* Get region */
> > + if (REGION_ID(ea) =3D=3D USER_REGION_ID) {
> > user_region =3D 1;
> > if (! mm) {
> > DBG_LOW(" user region with no mm !\n");
> > rc =3D 1;
> > goto bail;
> > }
> > - psize =3D get_slice_psize(mm, ea);
> > - ssize =3D user_segment_size(ea);
> > - vsid =3D get_vsid(mm->context.id, ea, ssize);
> > - break;
> > - case VMALLOC_REGION_ID:
> > + } else
> > mm =3D &init_mm;
> > - vsid =3D get_kernel_vsid(ea, mmu_kernel_ssize);
> > - if (ea < VMALLOC_END)
> > - psize =3D mmu_vmalloc_psize;
> > - else
> > - psize =3D mmu_io_psize;
> > - ssize =3D mmu_kernel_ssize;
> > - break;
> > - default:
> > - /* Not a valid range
> > - * Send the problem up to do_page_fault=20
> > - */
> > - rc =3D 1;
>=20
>=20
> That part is different now. We now handle kernel_region_id in case of
> hash_page. Earlier we used consider it a problem.=20
Yeah, that's going to be the kernel linear mapping. We should probably
continue to barf as we shouldn't fault on that. Thanks.
I'll fix.
Mikey
>=20
> > + rc =3D calculate_vsid(mm, ea, &vsid, &psize, &ssize);
> > + if (rc)
> > goto bail;
> > - }
> > +
> > DBG_LOW(" mm=3D%p, mm->pgdir=3D%p, vsid=3D%016lx\n", mm, mm->pgd, vsi=
d);
> > =20
> > /* Bad address. */
> > --=20
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel"=
in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>=20
^ permalink raw reply
* Re: [PATCH v2 08/17] powerpc/powerpc: Add new PCIe functions for allocating cxl interrupts
From: Michael Neuling @ 2014-10-02 6:09 UTC (permalink / raw)
To: Michael Ellerman
Cc: cbe-oss-dev, arnd, Aneesh Kumar K.V, greg, linux-kernel, imunsie,
linuxppc-dev, anton, jk
In-Reply-To: <20141002031659.DB564140188@ozlabs.org>
On Thu, 2014-10-02 at 13:16 +1000, Michael Ellerman wrote:
> On Tue, 2014-30-09 at 10:34:57 UTC, Michael Neuling wrote:
> > From: Ian Munsie <imunsie@au1.ibm.com>
> >=20
> > This adds a number of functions for allocating IRQs under powernv PCIe =
for cxl.
> >=20
> > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/p=
latforms/powernv/pci-ioda.c
> > index 329164f..b0b96f0 100644
> > --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> > @@ -503,6 +505,138 @@ static struct pnv_ioda_pe *pnv_ioda_get_pe(struct=
pci_dev *dev)
> > return NULL;
> > return &phb->ioda.pe_array[pdn->pe_number];
> > }
> > +
> > +struct device_node *pnv_pci_to_phb_node(struct pci_dev *dev)
> > +{
> > + struct pci_controller *hose =3D pci_bus_to_host(dev->bus);
> > +
> > + return hose->dn;
> > +}
> > +EXPORT_SYMBOL(pnv_pci_to_phb_node);
> > +
> > +#ifdef CONFIG_CXL_BASE
> > +int pnv_phb_to_cxl(struct pci_dev *dev)
> > +{
> > + struct pci_controller *hose =3D pci_bus_to_host(dev->bus);
> > + struct pnv_phb *phb =3D hose->private_data;
> > + struct pnv_ioda_pe *pe;
> > + int rc;
> > +
> > + if (!(pe =3D pnv_ioda_get_pe(dev))) {
> > + rc =3D -ENODEV;
> > + goto out;
> > + }
>=20
> That'd be a lot simpler as:
>=20
> pe =3D pnv_ioda_get_pe(dev);
> if (!pe)
> return -ENODEV;
OK
> > + pe_info(pe, "switch PHB to CXL\n");
> > + pe_info(pe, "PHB-ID : 0x%016llx\n", phb->opal_id);
> > + pe_info(pe, " pe : %i\n", pe->pe_number);
>=20
> Spacing is a bit weird but maybe it matches something else?
Actually, we switched this to pe_info() based on one of Gavin's reviews,
so the pe_number and opal_id being printed here are not needed anymore.
I'm simplifying this into one line.
pe_info(pe, "Switching PHB to CXL\n");
>=20
> > +
> > + if ((rc =3D opal_pci_set_phb_cxl_mode(phb->opal_id, 1, pe->pe_number)=
))
> > + dev_err(&dev->dev, "opal_pci_set_phb_cxl_mode failed: %i\n", rc);
>=20
> Again why not:
>=20
> rc =3D opal_pci_set_phb_cxl_mode(phb->opal_id, 1, pe->pe_number);
> if (rc)
> dev_err(&dev->dev, "opal_pci_set_phb_cxl_mode failed: %i\n", rc);
Ok.
> > +out:
> > + return rc;
> > +}
> > +EXPORT_SYMBOL(pnv_phb_to_cxl);
> > +
>=20
> > +int pnv_cxl_alloc_hwirq_ranges(struct cxl_irq_ranges *irqs,
> > + struct pci_dev *dev, int num)
>=20
> This could use some documentation.
>=20
> It seems to be that it allocates num irqs in some number of ranges, up to
> CXL_IRQ_RANGES?
OK
>=20
> > +{
> > + struct pci_controller *hose =3D pci_bus_to_host(dev->bus);
> > + struct pnv_phb *phb =3D hose->private_data;
> > + int range =3D 0;
>=20
> You reinitialise to 1 below?
Oops
>=20
> > + int hwirq;
> > + int try;
>=20
> So these can be:
>=20
> int hwirq, try, range;
>=20
> > + memset(irqs, 0, sizeof(struct cxl_irq_ranges));
> > +
> > + for (range =3D 1; range < CXL_IRQ_RANGES && num; range++) {
>=20
> I think this would be clearer if range was just called "i" as usual.
OK
> Why does it start at 1 ?
0 is used by the data storage interrupt. I'll add a comment to clarify.
>=20
> > + try =3D num;
> > + while (try) {
> > + hwirq =3D msi_bitmap_alloc_hwirqs(&phb->msi_bmp, try);
> > + if (hwirq >=3D 0)
> > + break;
> > + try /=3D 2;
> > + }
> > + if (!try)
> > + goto fail;
> > +
> > + irqs->offset[range] =3D phb->msi_base + hwirq;
> > + irqs->range[range] =3D try;
>=20
> irqs->range is irq_hw_number_t but looks like it should just be uint.
>=20
> > + pr_devel("cxl alloc irq range 0x%x: offset: 0x%lx limit: %li\n",
> > + range, irqs->offset[range], irqs->range[range]);
> > + num -=3D try;
> > + }
> > + if (num)
> > + goto fail;
> > +
> > + return 0;
> > +fail:
> > + for (range--; range >=3D 0; range--) {
> > + hwirq =3D irqs->offset[range] - phb->msi_base;
> > + msi_bitmap_free_hwirqs(&phb->msi_bmp, hwirq,
> > + irqs->range[range]);
> > + irqs->range[range] =3D 0;
> > + }
>=20
> Because you zero ranges at the top I think you can replace all of the fai=
l
> logic with a call to pnv_cxl_release_hwirq_ranges().
Nice. Will change.
>=20
>=20
> > + return -ENOSPC;
> > +}
> > +EXPORT_SYMBOL(pnv_cxl_alloc_hwirq_ranges);
> > +
> > +void pnv_cxl_release_hwirq_ranges(struct cxl_irq_ranges *irqs,
> > + struct pci_dev *dev)
> > +{
> > + struct pci_controller *hose =3D pci_bus_to_host(dev->bus);
> > + struct pnv_phb *phb =3D hose->private_data;
> > + int range =3D 0;
>=20
> Unnecessary init again.
Yep. I'll change to 'i' too.
> > + int hwirq;
> > +
> > + for (range =3D 0; range < 4; range++) {
>=20
> Shouldn't 4 be CXL_IRQ_RANGES ?
Yep.
>=20
> > + hwirq =3D irqs->offset[range] - phb->msi_base;
>=20
> That should be inside the if.
Yep.
>=20
> Or better do:
> if (!irqs->range[range])
> continue;
> ...
Nice.
>=20
> > + if (irqs->range[range]) {
> > + pr_devel("cxl release irq range 0x%x: offset: 0x%lx limit: %ld\n",
> > + range, irqs->offset[range],
> > + irqs->range[range]);
> > + msi_bitmap_free_hwirqs(&phb->msi_bmp, hwirq,
> > + irqs->range[range]);
> > + }
> > + }
> > +}
> > +EXPORT_SYMBOL(pnv_cxl_release_hwirq_ranges);
> > +
> > +int pnv_cxl_get_irq_count(struct pci_dev *dev)
> > +{
> > + struct pci_controller *hose =3D pci_bus_to_host(dev->bus);
> > + struct pnv_phb *phb =3D hose->private_data;
>=20
> Indentation is fubar.
OK
>=20
> > + return phb->msi_bmp.irq_count;
> > +}
> > +EXPORT_SYMBOL(pnv_cxl_get_irq_count);
> > +
> > +#endif /* CONFIG_CXL_BASE */
> > #endif /* CONFIG_PCI_MSI */
> > =20
> > static int pnv_ioda_configure_pe(struct pnv_phb *phb, struct pnv_ioda_=
pe *pe)
> > @@ -1330,6 +1464,33 @@ static void set_msi_irq_chip(struct pnv_phb *phb=
, unsigned int virq)
> > irq_set_chip(virq, &phb->ioda.irq_chip);
> > }
> > =20
> > +#ifdef CONFIG_CXL_BASE
>=20
> Why is this here and not in the previous #ifdef CONFIG_CXL_BASE block ?
I can actually move the rest of the cxl code down here too. So I'll do
that.
>=20
> > +int pnv_cxl_ioda_msi_setup(struct pci_dev *dev, unsigned int hwirq,
> > + unsigned int virq)
> > +{
> > + struct pci_controller *hose =3D pci_bus_to_host(dev->bus);
> > + struct pnv_phb *phb =3D hose->private_data;
> > + unsigned int xive_num =3D hwirq - phb->msi_base;
> > + struct pnv_ioda_pe *pe;
> > + int rc;
> > +
> > + if (!(pe =3D pnv_ioda_get_pe(dev)))
> > + return -ENODEV;
> > +
> > + /* Assign XIVE to PE */
> > + rc =3D opal_pci_set_xive_pe(phb->opal_id, pe->pe_number, xive_num);
> > + if (rc) {
> > + pr_warn("%s: OPAL error %d setting msi_base 0x%x hwirq 0x%x XIVE 0x%=
x PE\n",
> > + pci_name(dev), rc, phb->msi_base, hwirq, xive_num);
>=20
> dev_warn() ?
I'm going to move it to the pe_warn() we have here.
Cheers,
Mikey
^ permalink raw reply
* Re: [PATCH v2 15/17] cxl: Userspace header file.
From: Michael Ellerman @ 2014-10-02 6:02 UTC (permalink / raw)
To: Michael Neuling, greg, arnd, benh
Cc: cbe-oss-dev, mikey, Aneesh Kumar K.V, imunsie, linux-kernel,
linuxppc-dev, jk, anton
In-Reply-To: <1412073306-13812-16-git-send-email-mikey@neuling.org>
On Tue, 2014-30-09 at 10:35:04 UTC, Michael Neuling wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This defines structs and magic numbers required for userspace to interact with
> the kernel cxl driver via /dev/cxl/afu0.0.
>
> diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
> new file mode 100644
> index 0000000..6a394b5
> --- /dev/null
> +++ b/include/uapi/misc/cxl.h
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright 2014 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _UAPI_ASM_CXL_H
> +#define _UAPI_ASM_CXL_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* ioctls */
> +struct cxl_ioctl_start_work {
> + __u64 wed;
> + __u64 amr;
> + __u64 reserved1;
> + __u32 reserved2;
> + __s16 num_interrupts; /* -1 = use value from afu descriptor */
> + __u16 process_element; /* returned from kernel */
> + __u64 reserved3;
> + __u64 reserved4;
> + __u64 reserved5;
> + __u64 reserved6;
Why so many reserved fields?
What mechanism is there that will allow you to ever unreserve them?
ie. how does a new userspace detect that the kernel it's running on supports
new fields?
Or conversely how does a new kernel detect that userspace has passed it a
meaningful value in one of the previously reserved fields?
> +#define CXL_MAGIC 0xCA
> +#define CXL_IOCTL_START_WORK _IOWR(CXL_MAGIC, 0x00, struct cxl_ioctl_start_work)
What happened to 0x1 ?
> +#define CXL_IOCTL_CHECK_ERROR _IO(CXL_MAGIC, 0x02)
> +
> +/* events from read() */
> +
> +enum cxl_event_type {
> + CXL_EVENT_READ_FAIL = -1,
I don't see this used?
> + CXL_EVENT_RESERVED = 0,
> + CXL_EVENT_AFU_INTERRUPT = 1,
> + CXL_EVENT_DATA_STORAGE = 2,
> + CXL_EVENT_AFU_ERROR = 3,
> +};
> +
> +struct cxl_event_header {
> + __u32 type;
> + __u16 size;
> + __u16 process_element;
> + __u64 reserved1;
> + __u64 reserved2;
> + __u64 reserved3;
> +};
Again lots of reserved fields?
> +struct cxl_event_afu_interrupt {
> + struct cxl_event_header header;
> + __u16 irq; /* Raised AFU interrupt number */
> + __u16 reserved1;
> + __u32 reserved2;
> + __u64 reserved3;
> + __u64 reserved4;
> + __u64 reserved5;
> +};
> +
> +struct cxl_event_data_storage {
> + struct cxl_event_header header;
> + __u64 addr;
> + __u64 reserved1;
> + __u64 reserved2;
> + __u64 reserved3;
> +};
> +
> +struct cxl_event_afu_error {
> + struct cxl_event_header header;
> + __u64 err;
> + __u64 reserved1;
> + __u64 reserved2;
> + __u64 reserved3;
> +};
> +
> +struct cxl_event {
> + union {
> + struct cxl_event_header header;
> + struct cxl_event_afu_interrupt irq;
> + struct cxl_event_data_storage fault;
> + struct cxl_event_afu_error afu_err;
> + };
> +};
Rather than having the header included in every event, would it be clearer if
the cxl_event was:
struct cxl_event {
struct cxl_event_header header;
union {
struct cxl_event_afu_interrupt irq;
struct cxl_event_data_storage fault;
struct cxl_event_afu_error afu_err;
};
};
cheers
^ permalink raw reply
* [PATCH v2] powerpc: Speed up clear_page by unrolling it
From: Anton Blanchard @ 2014-10-02 5:44 UTC (permalink / raw)
To: benh, paulus, mpe; +Cc: linuxppc-dev
Unroll clear_page 8 times. A simple microbenchmark which
allocates and frees a zeroed page:
for (i = 0; i < iterations; i++) {
unsigned long p = __get_free_page(GFP_KERNEL | __GFP_ZERO);
free_page(p);
}
improves 20% on POWER8.
This assumes cacheline sizes won't grow beyond 512 bytes or
page sizes wont drop below 1kB, which is unlikely, but we could
add a runtime check during early init if it makes people nervous.
Michael found that some versions of gcc produce quite bad code
(all multiplies), so we give gcc a hand by using shifts and adds.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
arch/powerpc/include/asm/page_64.h | 42 ++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/page_64.h b/arch/powerpc/include/asm/page_64.h
index d0d6afb..d908a46 100644
--- a/arch/powerpc/include/asm/page_64.h
+++ b/arch/powerpc/include/asm/page_64.h
@@ -42,20 +42,40 @@
typedef unsigned long pte_basic_t;
-static __inline__ void clear_page(void *addr)
+static inline void clear_page(void *addr)
{
- unsigned long lines, line_size;
-
- line_size = ppc64_caches.dline_size;
- lines = ppc64_caches.dlines_per_page;
-
- __asm__ __volatile__(
+ unsigned long iterations;
+ unsigned long onex, twox, fourx, eightx;
+
+ iterations = ppc64_caches.dlines_per_page / 8;
+
+ /*
+ * Some verisions of gcc use multiply instructions to
+ * calculate the offsets so lets give it a hand to
+ * do better.
+ */
+ onex = ppc64_caches.dline_size;
+ twox = onex << 1;
+ fourx = onex << 2;
+ eightx = onex << 3;
+
+ asm volatile(
"mtctr %1 # clear_page\n\
-1: dcbz 0,%0\n\
- add %0,%0,%3\n\
+ .balign 16\n\
+1: dcbz 0,%0\n\
+ dcbz %3,%0\n\
+ dcbz %4,%0\n\
+ dcbz %5,%0\n\
+ dcbz %6,%0\n\
+ dcbz %7,%0\n\
+ dcbz %8,%0\n\
+ dcbz %9,%0\n\
+ add %0,%0,%10\n\
bdnz+ 1b"
- : "=r" (addr)
- : "r" (lines), "0" (addr), "r" (line_size)
+ : "=&r" (addr)
+ : "r" (iterations), "0" (addr), "b" (onex), "b" (twox),
+ "b" (twox+onex), "b" (fourx), "b" (fourx+onex),
+ "b" (twox+fourx), "b" (eightx-onex), "r" (eightx)
: "ctr", "memory");
}
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2 06/17] powerpc/powernv: Split out set MSI IRQ chip code
From: Michael Neuling @ 2014-10-02 5:22 UTC (permalink / raw)
To: Michael Ellerman
Cc: cbe-oss-dev, arnd, Aneesh Kumar K.V, greg, linux-kernel, imunsie,
linuxppc-dev, anton, jk
In-Reply-To: <20141002015729.D379414016B@ozlabs.org>
On Thu, 2014-10-02 at 11:57 +1000, Michael Ellerman wrote:
> On Tue, 2014-30-09 at 10:34:55 UTC, Michael Neuling wrote:
> > From: Ian Munsie <imunsie@au1.ibm.com>
> >=20
> > Some of the MSI IRQ code in pnv_pci_ioda_msi_setup() is generically use=
ful so
> > split it out.
> >=20
> > This will be used by some of the cxl PCIe code later.
> >=20
> > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/p=
latforms/powernv/pci-ioda.c
> > index df241b1..329164f 100644
> > --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> > @@ -1306,14 +1306,36 @@ static void pnv_ioda2_msi_eoi(struct irq_data *=
d)
> > icp_native_eoi(d);
> > }
> > =20
> > +
> > +static void set_msi_irq_chip(struct pnv_phb *phb, unsigned int virq)
> > +{
> > + struct irq_data *idata;
> > + struct irq_chip *ichip;
> > +
> > + if (phb->type !=3D PNV_PHB_IODA2)
> > + return;
> > +
> > + /*
> > + * Change the IRQ chip for the MSI interrupts on PHB3.
> > + * The corresponding IRQ chip should be populated for
> > + * the first time.
>=20
> Seeing as you're moving this comment can you clarify the wording.
Ok.
Mikey
^ permalink raw reply
* Re: [PATCH v2 09/17] powerpc/mm: Add new hash_page_mm()
From: Michael Ellerman @ 2014-10-02 3:48 UTC (permalink / raw)
To: Michael Neuling, greg, arnd, benh
Cc: cbe-oss-dev, mikey, Aneesh Kumar K.V, imunsie, linux-kernel,
linuxppc-dev, jk, anton
In-Reply-To: <1412073306-13812-10-git-send-email-mikey@neuling.org>
On Tue, 2014-30-09 at 10:34:58 UTC, Michael Neuling wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This adds a new function hash_page_mm() based on the existing hash_page().
> This version allows any struct mm to be passed in, rather than assuming
> current. This is useful for servicing co-processor faults which are not in the
> context of the current running process.
I'm not a big fan. hash_page() is already a train wreck, and this doesn't make
it any better.
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index bbdb054..0a5c8c0 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -904,7 +904,7 @@ void demote_segment_4k(struct mm_struct *mm, unsigned long addr)
> return;
> slice_set_range_psize(mm, addr, 1, MMU_PAGE_4K);
> copro_flush_all_slbs(mm);
> - if (get_paca_psize(addr) != MMU_PAGE_4K) {
> + if ((get_paca_psize(addr) != MMU_PAGE_4K) && (current->mm == mm)) {
> get_paca()->context = mm->context;
> slb_flush_and_rebolt();
This is a bit fishy.
If that mm is currently running on another cpu you just failed to update it's
paca. But I think the call to check_paca_psize() in hash_page() will save you
on that cpu.
In fact we might be able to remove that synchronisation from
demote_segment_4k() and always leave it up to check_paca_psize()?
> @@ -989,26 +989,24 @@ static void check_paca_psize(unsigned long ea, struct mm_struct *mm,
> * -1 - critical hash insertion error
> * -2 - access not permitted by subpage protection mechanism
> */
> -int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
> +int hash_page_mm(struct mm_struct *mm, unsigned long ea, unsigned long access, unsigned long trap)
> {
> enum ctx_state prev_state = exception_enter();
> pgd_t *pgdir;
> unsigned long vsid;
> - struct mm_struct *mm;
> pte_t *ptep;
> unsigned hugeshift;
> const struct cpumask *tmp;
> int rc, user_region = 0, local = 0;
> int psize, ssize;
>
> - DBG_LOW("hash_page(ea=%016lx, access=%lx, trap=%lx\n",
> - ea, access, trap);
> + DBG_LOW("%s(ea=%016lx, access=%lx, trap=%lx\n",
> + __func__, ea, access, trap);
>
> /* Get region & vsid */
> switch (REGION_ID(ea)) {
> case USER_REGION_ID:
> user_region = 1;
> - mm = current->mm;
> if (! mm) {
> DBG_LOW(" user region with no mm !\n");
> rc = 1;
What about the VMALLOC case where we do:
mm = &init_mm;
Is that what you want? It seems odd that you pass an mm to the routine, but
then potentially it ends up using a different mm after all depending on the
address.
cheers
^ permalink raw reply
* Re: [PATCH v2 13/17] cxl: Add base builtin support
From: Michael Neuling @ 2014-10-02 3:43 UTC (permalink / raw)
To: Michael Ellerman
Cc: cbe-oss-dev, arnd, Aneesh Kumar K.V, greg, linux-kernel, imunsie,
linuxppc-dev, anton, jk
In-Reply-To: <20141001120010.A834E140170@ozlabs.org>
On Wed, 2014-10-01 at 22:00 +1000, Michael Ellerman wrote:
> On Tue, 2014-30-09 at 10:35:02 UTC, Michael Neuling wrote:
> > This also adds the cxl_ctx_in_use() function for use in the mm code to =
see if
> > any cxl contexts are currently in use. This is used by the tlbie() to
> > determine if it can do local TLB invalidations or not. This also adds =
get/put
> > calls for the cxl driver module to refcount the active cxl contexts.
>=20
> > diff --git a/drivers/misc/cxl/base.c b/drivers/misc/cxl/base.c
> > new file mode 100644
> > index 0000000..f4cbcfb
> > --- /dev/null
> > +++ b/drivers/misc/cxl/base.c
> > @@ -0,0 +1,102 @@
> > +/*
> > + * Copyright 2014 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/rcupdate.h>
> > +#include <asm/errno.h>
> > +#include <misc/cxl.h>
> > +#include "cxl.h"
> > +
> > +/* protected by rcu */
> > +static struct cxl_calls *cxl_calls;
> > +
> > +static atomic_t use_count =3D ATOMIC_INIT(0);
> ...
>=20
> > +void cxl_ctx_get(void)
> > +{
> > + atomic_inc(&use_count);
> > +}
> > +EXPORT_SYMBOL(cxl_ctx_get);
> > +
> > +void cxl_ctx_put(void)
> > +{
> > + atomic_dec(&use_count);
> > +}
> > +EXPORT_SYMBOL(cxl_ctx_put);
> > +
> > +bool cxl_ctx_in_use(void)
> > +{
> > + return (atomic_read(&use_count) !=3D 0);
> > +}
> > +EXPORT_SYMBOL(cxl_ctx_in_use);
>=20
> So as written this results in a function call for every tlbie(), even whe=
n no
> one has ever used a CAPI adapter, or when none are even in the machine.
Yep.
> I think the patch below is a better trade off. It makes the use_count glo=
bal,
> but that's not a biggy. The benefit is that the additional code in tlbie(=
)
> becomes:
>=20
> ld r10,-29112(r2)
> lwz r10,0(r10)
> cmpwi cr7,r10,0
>=20
> Which is about as good as it can get.
Nice.. I'll add. Thanks.
Mikey
>=20
> cheers
>=20
>=20
> diff --git a/drivers/misc/cxl/base.c b/drivers/misc/cxl/base.c
> index f4cbcfbd8dbc..4401d1c2dd33 100644
> --- a/drivers/misc/cxl/base.c
> +++ b/drivers/misc/cxl/base.c
> @@ -16,7 +16,7 @@
> /* protected by rcu */
> static struct cxl_calls *cxl_calls;
> =20
> -static atomic_t use_count =3D ATOMIC_INIT(0);
> +atomic_t cxl_use_count =3D ATOMIC_INIT(0);
> =20
> #ifdef CONFIG_CXL_MODULE
> =20
> @@ -65,24 +65,6 @@ void cxl_slbia(struct mm_struct *mm)
> }
> EXPORT_SYMBOL(cxl_slbia);
> =20
> -void cxl_ctx_get(void)
> -{
> - atomic_inc(&use_count);
> -}
> -EXPORT_SYMBOL(cxl_ctx_get);
> -
> -void cxl_ctx_put(void)
> -{
> - atomic_dec(&use_count);
> -}
> -EXPORT_SYMBOL(cxl_ctx_put);
> -
> -bool cxl_ctx_in_use(void)
> -{
> - return (atomic_read(&use_count) !=3D 0);
> -}
> -EXPORT_SYMBOL(cxl_ctx_in_use);
> -
> int register_cxl_calls(struct cxl_calls *calls)
> {
> if (cxl_calls)
> diff --git a/include/misc/cxl.h b/include/misc/cxl.h
> index bde46a330881..6e43dca6a792 100644
> --- a/include/misc/cxl.h
> +++ b/include/misc/cxl.h
> @@ -18,12 +18,24 @@ struct cxl_irq_ranges {
> };
> =20
> #ifdef CONFIG_CXL_BASE
> +extern atomic_t cxl_use_count;
> =20
> void cxl_slbia(struct mm_struct *mm);
> -void cxl_ctx_get(void);
> -void cxl_ctx_put(void);
> -bool cxl_ctx_in_use(void);
> =20
> +static inline bool cxl_ctx_in_use(void)
> +{
> + return (atomic_read(&cxl_use_count) !=3D 0);
> +}
> +
> +static inline void cxl_ctx_get(void)
> +{
> + atomic_inc(&cxl_use_count);
> +}
> +
> +static inline void cxl_ctx_put(void)
> +{
> + atomic_dec(&cxl_use_count);
> +}
> #else /* CONFIG_CXL_BASE */
> =20
> #define cxl_slbia(...) do { } while (0)
>=20
^ permalink raw reply
* Re: [PATCH v2 07/17] cxl: Add new header for call backs and structs
From: Michael Neuling @ 2014-10-02 3:37 UTC (permalink / raw)
To: Michael Ellerman
Cc: cbe-oss-dev, arnd, Aneesh Kumar K.V, greg, linux-kernel, imunsie,
linuxppc-dev, anton, jk
In-Reply-To: <20141001120010.4813614016A@ozlabs.org>
On Wed, 2014-10-01 at 22:00 +1000, Michael Ellerman wrote:
> On Tue, 2014-30-09 at 10:34:56 UTC, Michael Neuling wrote:
> > From: Ian Munsie <imunsie@au1.ibm.com>
> >=20
> > This new header add defines for callbacks and structs needed by the res=
t of the
> adds
> > kernel to hook into the cxl infrastructure.
> >=20
> > Empty functions are provided when CONFIG CXL_BASE is not enabled.
> >=20
> > Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > ---
> > include/misc/cxl.h | 34 ++++++++++++++++++++++++++++++++++
>=20
> include/misc is kind of weird. I guess it's a misc device.
>=20
> Any reason not to have it in arch/powerpc/include ?
We can do either way. We did consider it a driver so putting it in
arch/powerpc didn't seem quite right.
I might leave it here unless you really object.
>=20
> > diff --git a/include/misc/cxl.h b/include/misc/cxl.h
> > new file mode 100644
> > index 0000000..bde46a3
> > --- /dev/null
> > +++ b/include/misc/cxl.h
> > @@ -0,0 +1,34 @@
> > +/*
> > + * Copyright 2014 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#ifndef _MISC_ASM_CXL_H
>=20
> No ASM.
Oops, yes.
>=20
> > +#define _MISC_ASM_CXL_H
> > +
> > +#define CXL_IRQ_RANGES 4
> > +
> > +struct cxl_irq_ranges {
> > + irq_hw_number_t offset[CXL_IRQ_RANGES];
> > + irq_hw_number_t range[CXL_IRQ_RANGES];
> > +};
> > +
> > +#ifdef CONFIG_CXL_BASE
> > +
> > +void cxl_slbia(struct mm_struct *mm);
> > +void cxl_ctx_get(void);
> > +void cxl_ctx_put(void);
> > +bool cxl_ctx_in_use(void);
> > +
> > +#else /* CONFIG_CXL_BASE */
> > +
> > +#define cxl_slbia(...) do { } while (0)
> > +#define cxl_ctx_in_use(...) false
>=20
> Any reason these shouldn't be static inlines?
No, I'll change.
Mikey
^ permalink raw reply
* Re: [PATCH v2 08/17] powerpc/powerpc: Add new PCIe functions for allocating cxl interrupts
From: Michael Ellerman @ 2014-10-02 3:16 UTC (permalink / raw)
To: Michael Neuling, greg, arnd, benh
Cc: cbe-oss-dev, mikey, Aneesh Kumar K.V, imunsie, linux-kernel,
linuxppc-dev, jk, anton
In-Reply-To: <1412073306-13812-9-git-send-email-mikey@neuling.org>
On Tue, 2014-30-09 at 10:34:57 UTC, Michael Neuling wrote:
> From: Ian Munsie <imunsie@au1.ibm.com>
>
> This adds a number of functions for allocating IRQs under powernv PCIe for cxl.
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 329164f..b0b96f0 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -503,6 +505,138 @@ static struct pnv_ioda_pe *pnv_ioda_get_pe(struct pci_dev *dev)
> return NULL;
> return &phb->ioda.pe_array[pdn->pe_number];
> }
> +
> +struct device_node *pnv_pci_to_phb_node(struct pci_dev *dev)
> +{
> + struct pci_controller *hose = pci_bus_to_host(dev->bus);
> +
> + return hose->dn;
> +}
> +EXPORT_SYMBOL(pnv_pci_to_phb_node);
> +
> +#ifdef CONFIG_CXL_BASE
> +int pnv_phb_to_cxl(struct pci_dev *dev)
> +{
> + struct pci_controller *hose = pci_bus_to_host(dev->bus);
> + struct pnv_phb *phb = hose->private_data;
> + struct pnv_ioda_pe *pe;
> + int rc;
> +
> + if (!(pe = pnv_ioda_get_pe(dev))) {
> + rc = -ENODEV;
> + goto out;
> + }
That'd be a lot simpler as:
pe = pnv_ioda_get_pe(dev);
if (!pe)
return -ENODEV;
> + pe_info(pe, "switch PHB to CXL\n");
> + pe_info(pe, "PHB-ID : 0x%016llx\n", phb->opal_id);
> + pe_info(pe, " pe : %i\n", pe->pe_number);
Spacing is a bit weird but maybe it matches something else?
> +
> + if ((rc = opal_pci_set_phb_cxl_mode(phb->opal_id, 1, pe->pe_number)))
> + dev_err(&dev->dev, "opal_pci_set_phb_cxl_mode failed: %i\n", rc);
Again why not:
rc = opal_pci_set_phb_cxl_mode(phb->opal_id, 1, pe->pe_number);
if (rc)
dev_err(&dev->dev, "opal_pci_set_phb_cxl_mode failed: %i\n", rc);
> +out:
> + return rc;
> +}
> +EXPORT_SYMBOL(pnv_phb_to_cxl);
> +
> +int pnv_cxl_alloc_hwirq_ranges(struct cxl_irq_ranges *irqs,
> + struct pci_dev *dev, int num)
This could use some documentation.
It seems to be that it allocates num irqs in some number of ranges, up to
CXL_IRQ_RANGES?
> +{
> + struct pci_controller *hose = pci_bus_to_host(dev->bus);
> + struct pnv_phb *phb = hose->private_data;
> + int range = 0;
You reinitialise to 1 below?
> + int hwirq;
> + int try;
So these can be:
int hwirq, try, range;
> + memset(irqs, 0, sizeof(struct cxl_irq_ranges));
> +
> + for (range = 1; range < CXL_IRQ_RANGES && num; range++) {
I think this would be clearer if range was just called "i" as usual.
Why does it start at 1 ?
> + try = num;
> + while (try) {
> + hwirq = msi_bitmap_alloc_hwirqs(&phb->msi_bmp, try);
> + if (hwirq >= 0)
> + break;
> + try /= 2;
> + }
> + if (!try)
> + goto fail;
> +
> + irqs->offset[range] = phb->msi_base + hwirq;
> + irqs->range[range] = try;
irqs->range is irq_hw_number_t but looks like it should just be uint.
> + pr_devel("cxl alloc irq range 0x%x: offset: 0x%lx limit: %li\n",
> + range, irqs->offset[range], irqs->range[range]);
> + num -= try;
> + }
> + if (num)
> + goto fail;
> +
> + return 0;
> +fail:
> + for (range--; range >= 0; range--) {
> + hwirq = irqs->offset[range] - phb->msi_base;
> + msi_bitmap_free_hwirqs(&phb->msi_bmp, hwirq,
> + irqs->range[range]);
> + irqs->range[range] = 0;
> + }
Because you zero ranges at the top I think you can replace all of the fail
logic with a call to pnv_cxl_release_hwirq_ranges().
> + return -ENOSPC;
> +}
> +EXPORT_SYMBOL(pnv_cxl_alloc_hwirq_ranges);
> +
> +void pnv_cxl_release_hwirq_ranges(struct cxl_irq_ranges *irqs,
> + struct pci_dev *dev)
> +{
> + struct pci_controller *hose = pci_bus_to_host(dev->bus);
> + struct pnv_phb *phb = hose->private_data;
> + int range = 0;
Unnecessary init again.
> + int hwirq;
> +
> + for (range = 0; range < 4; range++) {
Shouldn't 4 be CXL_IRQ_RANGES ?
> + hwirq = irqs->offset[range] - phb->msi_base;
That should be inside the if.
Or better do:
if (!irqs->range[range])
continue;
...
> + if (irqs->range[range]) {
> + pr_devel("cxl release irq range 0x%x: offset: 0x%lx limit: %ld\n",
> + range, irqs->offset[range],
> + irqs->range[range]);
> + msi_bitmap_free_hwirqs(&phb->msi_bmp, hwirq,
> + irqs->range[range]);
> + }
> + }
> +}
> +EXPORT_SYMBOL(pnv_cxl_release_hwirq_ranges);
> +
> +int pnv_cxl_get_irq_count(struct pci_dev *dev)
> +{
> + struct pci_controller *hose = pci_bus_to_host(dev->bus);
> + struct pnv_phb *phb = hose->private_data;
Indentation is fubar.
> + return phb->msi_bmp.irq_count;
> +}
> +EXPORT_SYMBOL(pnv_cxl_get_irq_count);
> +
> +#endif /* CONFIG_CXL_BASE */
> #endif /* CONFIG_PCI_MSI */
>
> static int pnv_ioda_configure_pe(struct pnv_phb *phb, struct pnv_ioda_pe *pe)
> @@ -1330,6 +1464,33 @@ static void set_msi_irq_chip(struct pnv_phb *phb, unsigned int virq)
> irq_set_chip(virq, &phb->ioda.irq_chip);
> }
>
> +#ifdef CONFIG_CXL_BASE
Why is this here and not in the previous #ifdef CONFIG_CXL_BASE block ?
> +int pnv_cxl_ioda_msi_setup(struct pci_dev *dev, unsigned int hwirq,
> + unsigned int virq)
> +{
> + struct pci_controller *hose = pci_bus_to_host(dev->bus);
> + struct pnv_phb *phb = hose->private_data;
> + unsigned int xive_num = hwirq - phb->msi_base;
> + struct pnv_ioda_pe *pe;
> + int rc;
> +
> + if (!(pe = pnv_ioda_get_pe(dev)))
> + return -ENODEV;
> +
> + /* Assign XIVE to PE */
> + rc = opal_pci_set_xive_pe(phb->opal_id, pe->pe_number, xive_num);
> + if (rc) {
> + pr_warn("%s: OPAL error %d setting msi_base 0x%x hwirq 0x%x XIVE 0x%x PE\n",
> + pci_name(dev), rc, phb->msi_base, hwirq, xive_num);
dev_warn() ?
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