* 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
* Re: [PATCH v2 05/17] powerpc/mm: Export mmu_kernel_ssize and mmu_linear_psize
From: Michael Neuling @ 2014-10-02 3:13 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: <20141001071332.A61A614017E@ozlabs.org>
On Wed, 2014-10-01 at 17:13 +1000, Michael Ellerman wrote:
> On Tue, 2014-30-09 at 10:34:54 UTC, Michael Neuling wrote:
> > From: Ian Munsie <imunsie@au1.ibm.com>
>=20
> Mind explaining why ? :)
Sure.
Mikey
^ permalink raw reply
* Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off
From: Guenter Roeck @ 2014-10-02 2:51 UTC (permalink / raw)
To: Alexander Graf, Scott Wood
Cc: Arnd Bergmann, Geoff Levand, Alistair Popple, Anatolij Gustschin,
linuxppc-dev
In-Reply-To: <542C91D5.3030305@suse.de>
On 10/01/2014 04:44 PM, Alexander Graf wrote:
>
>
> On 02.10.14 01:28, Scott Wood wrote:
>> On Thu, 2014-10-02 at 01:21 +0200, Alexander Graf wrote:
>>>
>>> On 02.10.14 00:39, Scott Wood wrote:
>>>> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
>>>>> The generic Linux framework to power off the machine is a function pointer
>>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>>> potentially implement it rather than board files.
>>>>>
>>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>>> off.
>>>>>
>>>>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>>>>> this card house falls apart. That driver only registers itself if pm_power_off
>>>>> is NULL to ensure it doesn't override board specific logic. However, since we
>>>>> always set pm_power_off to the generic power off logic (which will just not
>>>>> power off the machine if no ppc_md.power_off call is implemented), we can't
>>>>> implement power off via the generic GPIO power off driver.
>>>>>
>>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>>> driver can implement power off logic via that function pointer.
>>>>>
>>>>> With this patch set applied and a few patches on top of QEMU that implement a
>>>>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>>>>> machine after halt.
>>>>
>>>> Are there any plans to handle restart similarly?
>>>
Guess I am missing some context here. Support for a restart handler call chain
will be added in 3.18. Currently its primary goal is to replace arm_pm_restart,
but it would be a one-liner to add support for it to powerpc (see below).
>>> I don't see an immediate need for it, as we do have access to reset via
>>> the guts device.
>>
>> The guts device is problematic from a hardware description perspective,
>> as we advertise a bunch of things to the guest that we don't implement.
>> E.g. the only reason we don't currently have a problem with Linux trying
>> to use guts to sync the timebase across cores is that by chance we
>> happen to expose a guts that corresponds to a single-cpu SoC.
>
> True, and it is slightly ugly to expose a specific SoC's guts in our
> generic pv machine type.
>
>>
>>> I also don't see any generic driver in Linux that would implement reset
>>> via a gpio controller device, so apparently it's not incredibly common
>>> to implement reset via gpio.
>>
Again, I may be missing something. How about drivers/power/reset/gpio-restart.c ?
It would be really simple make it work with powerpc; all you would have to do
is to add a call to do_kernel_restart() to the powerpc machine_restart() function.
>> There wasn't a generic gpio-power-off driver either until a couple years
>> ago... Regardless of whether it's done via gpio, using ppc_md for it is
>> bad for the same reasons as for power-off.
>
> I agree. Today, only ARM has a comparable mechanism for drivers to hook
> a reset handler into the machine specific reset path.
>
Not anymore ...
> I'd say let's wait for Guenter to finish the power off rework and then
> tackle a generic reset call chain based approach next.
>
Please have a look into the restart handler; the code is in linux-next.
That should probably solve your restart related problems.
A branch on top of v3.17-rc2 is at
https://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=restart-handler
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off
From: Guenter Roeck @ 2014-10-02 2:36 UTC (permalink / raw)
To: Alexander Graf
Cc: Arnd Bergmann, Geoff Levand, Alistair Popple, Geert Uytterhoeven,
Scott Wood, Anatolij Gustschin, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <542C714C.4020009@suse.de>
On 10/01/2014 02:25 PM, Alexander Graf wrote:
>
>
> On 01.10.14 17:54, Guenter Roeck wrote:
>> On Wed, Oct 01, 2014 at 04:47:23PM +0200, Alexander Graf wrote:
>>>
>>>
>>> On 01.10.14 16:33, Geert Uytterhoeven wrote:
>>>> Hi Alex,
>>>>
>>>> On Wed, Oct 1, 2014 at 3:27 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> The generic Linux framework to power off the machine is a function pointer
>>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>>> potentially implement it rather than board files.
>>>>>
>>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>>> off.
>>>>>
>>>>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>>>>> this card house falls apart. That driver only registers itself if pm_power_off
>>>>> is NULL to ensure it doesn't override board specific logic. However, since we
>>>>> always set pm_power_off to the generic power off logic (which will just not
>>>>> power off the machine if no ppc_md.power_off call is implemented), we can't
>>>>> implement power off via the generic GPIO power off driver.
>>>>>
>>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>>> driver can implement power off logic via that function pointer.
>>>>>
>>>>> With this patch set applied and a few patches on top of QEMU that implement a
>>>>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>>>>> machine after halt.
>>>>
>>>> This is touching the same area as last night's
>>>> "[RFC PATCH 00/16] kernel: Add support for poweroff handler call chain"
>>>> https://lkml.org/lkml/2014/9/30/575
>>>
>>> I agree, and I think your patch set is walking into a reasonable
>>> direction. However, I really think it should convert all users of
>>> pm_power_off - at which point you'll probably get to the same conclusion
>>> that ppc_md.power_off is a bad idea :).
>>>
>> Yes, that would be the ultimate goal.
>>
>>> So in a way, this patch set is semantically a prerequisite to the full
>>> conversion you'd probably like to do :).
>>>
>>> Also, in your cover letter you describe that some methods power off the
>>> CPU power while others power off the system power. How do you
>>> distinguish between them with a call chain? You probably won't get
>>> around to trigger the system power off callback after the CPU power off
>>> callback ran ;).
>>>
>> Those are examples. Don't get hung up on it. I may actually replace the
>> CPU example with something better in the next version; it is not really
>> a good example and may get people stuck on "why on earth would anyone want
>> or need a means to turn off the CPU power" instead of focusing on the problem
>> the patch set tries to solve.
>>
>> The basic problem is that there can be different poweroff handlers,
>> some of which may not be available on some systems, and some may not
>> be as desirable as others for various reasons. The code registering
>> those poweroff handlers does not specify the poweroff method, but its
>> priority. It would be up to the programmer (hopefully together with
>> the board designer) to determine which method should have higher priority.
>> Taking the above example, the callback to turn off CPU power would presumably
>> be one of last resort, and have a very low priority.
>>
>> A better example may actually be patch 15/16 of the series. The affected
>> driver (drivers/power/reset/restart-poweroff.c) does not really power off
>> the system, but restarts it instead. Obviously that would only be a poweroff
>> handler of last resort, which should only be executed if no other means
>> to power off the system is available.
>
> Sounds like a good plan :). You probably want to have some global list
> of priority numbers like "try this first" or "this is a non-optimal, but
> working method" and "only ever do this as last resort".
>
Yes, this is already in the patch set, similar to the restart handler.
> Maybe you could as a first step convert every user of pm_power_off to
> this new framework with a global notifier_block, similar to how
> pm_power_off is a global today? Then we can at least get rid of
> pm_power_off altogether and move to only notifiers, whereas new
> notifiers can come before or after the old machine set implementations.
>
> As a nice bonus this automatically converts every user of pm_power_off()
> to instead call the notifier chain.
>
Interesting idea, but I am not really sure if it would make much of
a difference. I would still have to implement that handler for each
platform. I might as well convert all users directly; at least this would
ensure that all users are converted.
Guenter
^ permalink raw reply
* Re: [PATCH v2 2/4] gpu/radeon: Move 64-bit MSI quirk from arch to driver
From: Alex Deucher @ 2014-10-02 2:18 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Dave Airlie, Linux PCI, Anton Blanchard, Brian King,
Yijing Wang, Takashi Iwai, Bjorn Helgaas
In-Reply-To: <1412210019.4285.251.camel@pasglop>
On Wed, Oct 1, 2014 at 8:33 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 radeon driver
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: <stable@vger.kernel.org>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>
> v2: This is just adjusted to the new flag name
>
> 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);
> 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 4/4] ALSA: hda - Limit 40bit DMA for AMD HDMI controllers
From: Alex Deucher @ 2014-10-02 2:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Dave Airlie, Linux PCI, Anton Blanchard, Brian King,
Yijing Wang, Takashi Iwai, Bjorn Helgaas
In-Reply-To: <1412210092.10667.3.camel@pasglop>
On Wed, Oct 1, 2014 at 8:34 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> AMD/ATI HDMI controller chip models, we already have a filter to lower
> to 32bit DMA, but the rest are supposed to be working with 64bit
> although the hardware doesn't really work with 63bit but only with 40
> or 48bit DMA. In this patch, we take 40bit DMA for safety for the
> AMD/ATI controllers as the graphics drivers does.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: <stable@vger.kernel.org>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>
> Tested, works fine. This patch is actually independent of the rest
> of the series
>
> sound/pci/hda/hda_intel.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index f91ba7f..48d0f30 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1483,6 +1483,7 @@ static int azx_first_init(struct azx *chip)
> struct snd_card *card = chip->card;
> int err;
> unsigned short gcap;
> + unsigned int dma_bits = 64;
>
> #if BITS_PER_LONG != 64
> /* Fix up base address on ULI M5461 */
> @@ -1524,9 +1525,14 @@ static int azx_first_init(struct azx *chip)
> gcap = azx_readw(chip, GCAP);
> dev_dbg(card->dev, "chipset global capabilities = 0x%x\n", gcap);
>
> + /* AMD devices support 40 or 48bit DMA, take the safe one */
> + if (chip->pci->vendor == PCI_VENDOR_ID_AMD)
> + dma_bits = 40;
> +
> /* disable SB600 64bit support for safety */
> if (chip->pci->vendor == PCI_VENDOR_ID_ATI) {
> struct pci_dev *p_smbus;
> + dma_bits = 40;
> p_smbus = pci_get_device(PCI_VENDOR_ID_ATI,
> PCI_DEVICE_ID_ATI_SBX00_SMBUS,
> NULL);
> @@ -1556,9 +1562,11 @@ static int azx_first_init(struct azx *chip)
> }
>
> /* allow 64bit DMA address if supported by H/W */
> - if ((gcap & AZX_GCAP_64OK) && !pci_set_dma_mask(pci, DMA_BIT_MASK(64)))
> - pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(64));
> - else {
> + if (!(gcap & AZX_GCAP_64OK))
> + dma_bits = 32;
> + if (!pci_set_dma_mask(pci, DMA_BIT_MASK(dma_bits))) {
> + pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(dma_bits));
> + } else {
> pci_set_dma_mask(pci, DMA_BIT_MASK(32));
> pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(32));
> }
>
>
>
>
^ 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