* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Rusty Russell @ 2014-05-20 1:14 UTC (permalink / raw)
To: Hugh Dickins, Madhavan Srinivasan
Cc: linux-arch, riel, ak, dave.hansen, peterz, x86, linux-kernel,
linux-mm, paulus, mgorman, akpm, linuxppc-dev, mingo,
Kirill A. Shutemov
In-Reply-To: <alpine.LSU.2.11.1405191531150.1317@eggly.anvils>
Hugh Dickins <hughd@google.com> writes:
> On Mon, 19 May 2014, Madhavan Srinivasan wrote:
>> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
>> > Hugh Dickins <hughd@google.com> writes:
>> >> On Thu, 15 May 2014, Madhavan Srinivasan wrote:
>> >>>
>> >>> Hi Ingo,
>> >>>
>> >>> Do you have any comments for the latest version of the patchset. If
>> >>> not, kindly can you pick it up as is.
>> >>>
>> >>>
>> >>> With regards
>> >>> Maddy
>> >>>
>> >>>> Kirill A. Shutemov with 8c6e50b029 commit introduced
>> >>>> vm_ops->map_pages() for mapping easy accessible pages around
>> >>>> fault address in hope to reduce number of minor page faults.
>> >>>>
>> >>>> This patch creates infrastructure to modify the FAULT_AROUND_ORDER
>> >>>> value using mm/Kconfig. This will enable architecture maintainers
>> >>>> to decide on suitable FAULT_AROUND_ORDER value based on
>> >>>> performance data for that architecture. First patch also defaults
>> >>>> FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
>> >>>> out the performance numbers for powerpc (platform pseries) and
>> >>>> initialize the fault around order variable for pseries platform of
>> >>>> powerpc.
>> >>
>> >> Sorry for not commenting earlier - just reminded by this ping to Ingo.
>> >>
>> >> I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
>> >>
>> >> arch/powerpc/Kconfig suggests that Power supports base page size of
>> >> 4k, 16k, 64k or 256k.
>> >>
>> >> I would expect your optimal fault_around_order to depend very much on
>> >> the base page size.
>> >
>> > It was 64k, which is what PPC64 uses on all the major distributions.
>> > You really only get a choice of 4k and 64k with 64 bit power.
>> >
>> This is true. PPC64 support multiple pagesize and yes the default page
>> size of 64k, is taken as base pagesize for the tests.
>>
>> >> Perhaps fault_around_size would provide a more useful default?
>> >
>> > That seems to fit. With 4k pages and order 4, you're asking for 64k.
>> > Maddy's result shows 64k is also reasonable for 64k pages.
>> >
>> > Perhaps we try to generalize from two data points (a slight improvement
>> > over doing it from 1!), eg:
>> >
>> > /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
>> > unsigned int fault_around_order __read_mostly =
>> > (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
>
> Rusty's bimodal answer doesn't seem the right starting point to me.
? It's not bimodal, it's graded. I think you misread?
> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> the order of the fault-around size in bytes, and fault_around_pages()
> use 1UL << (fault_around_order - PAGE_SHIFT)
> - when that doesn't wrap, of course!
>
> That would at least have a better chance of being appropriate for
> architectures with 8k and 16k pages (Itanium springs to mind).
Well, from our two data points it seems that we want to fault in
64k at a time whatever our page size. Perhaps it's clearer if the
code expresses itself that way.
> Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
> Did other architectures, with other page sizes, back that default?
> Clearly not powerpc.
Yeah, BenH flagged it as "we should test this" for powerpc, which is
what Maddy then did.
>> and also this will remove the
>> compile time option to disable the feature?
>
> Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
> for v3.16?
>
> I'm not sure whether Rusty was arguing against that or not. I think
> we are all three concerned to have a more sensible default than what's
> there at present. I don't feel very strongly about your Kconfig
> option: I've no objection, if it were to default to byte order 16.
I don't mind either.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Hugh Dickins @ 2014-05-20 2:34 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-arch, riel, Madhavan Srinivasan, dave.hansen, peterz, x86,
linux-kernel, linux-mm, ak, paulus, mgorman, akpm, linuxppc-dev,
mingo, Kirill A. Shutemov
In-Reply-To: <87d2f9jlpd.fsf@rustcorp.com.au>
On Tue, 20 May 2014, Rusty Russell wrote:
> Hugh Dickins <hughd@google.com> writes:
> >> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
> >> >
> >> > Perhaps we try to generalize from two data points (a slight improvement
> >> > over doing it from 1!), eg:
> >> >
> >> > /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
> >> > unsigned int fault_around_order __read_mostly =
> >> > (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
> >
> > Rusty's bimodal answer doesn't seem the right starting point to me.
>
> ? It's not bimodal, it's graded. I think you misread?
Yikes, worse than misread, more like I was too rude even to read: sorry!
Hugh
^ permalink raw reply
* PowerPC Build error with patch: powerpc/ppc64: Allow allmodconfig to build (finally !)
From: Mike Qiu @ 2014-05-20 6:36 UTC (permalink / raw)
To: linuxppc
Hi all,
I face one build error in linux-next git tree, see below:
The platform is IBM P7.
[root@cena01 linux-next]# make -j60
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CALL scripts/checksyscalls.sh
<stdin>:1232:2: warning: #warning syscall renameat2 not implemented [-Wcpp]
CHK include/generated/compile.h
CALL arch/powerpc/kernel/systbl_chk.sh
CALL arch/powerpc/kernel/prom_init_check.sh
AS arch/powerpc/kernel/head_64.o
arch/powerpc/kernel/exceptions-64s.S: Assembler messages:
arch/powerpc/kernel/exceptions-64s.S:269: Error: operand out of range
(0x000000000000814c is not between 0xffffffffffff8000 and
0x0000000000007ffc)
arch/powerpc/kernel/exceptions-64s.S:729: Error: operand out of range
(0x000000000000814c is not between 0xffffffffffff8000 and
0x0000000000007ffc)
make[1]: *** [arch/powerpc/kernel/head_64.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [arch/powerpc/kernel] Error 2
make: *** Waiting for unfinished jobs....
Finally, I find out that it is the commit
"0be9d8b61c0c1f3c8f86292c6e237ff26acd392d powerpc/ppc64: Allow
allmodconfig to build (finally !)" case this error.
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Rusty Russell @ 2014-05-20 6:22 UTC (permalink / raw)
To: Kirill A. Shutemov, Andrew Morton
Cc: linux-arch, riel, Madhavan Srinivasan, dave.hansen, peterz, x86,
Hugh Dickins, linux-kernel, linux-mm, ak, paulus, mgorman,
linuxppc-dev, mingo, Kirill A. Shutemov
In-Reply-To: <20140520004429.E660AE009B@blue.fi.intel.com>
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> Andrew Morton wrote:
>> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
>>
>> > Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
>> > the order of the fault-around size in bytes, and fault_around_pages()
>> > use 1UL << (fault_around_order - PAGE_SHIFT)
>>
>> Yes. And shame on me for missing it (this time!) at review.
>>
>> There's still time to fix this. Patches, please.
>
> Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH V4 2/2] powerpc/pseries: init fault_around_order for pseries
From: Andrew Morton @ 2014-05-20 7:28 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: linux-arch, riel, rusty, dave.hansen, peterz, x86, linux-kernel,
linux-mm, ak, paulus, mgorman, linuxppc-dev, mingo,
kirill.shutemov
In-Reply-To: <1399541296-18810-3-git-send-email-maddy@linux.vnet.ibm.com>
On Thu, 8 May 2014 14:58:16 +0530 Madhavan Srinivasan <maddy@linux.vnet.ibm.com> wrote:
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -17,6 +17,8 @@ struct device_node;
> extern void request_event_sources_irqs(struct device_node *np,
> irq_handler_t handler, const char *name);
>
> +extern unsigned int fault_around_order;
This isn't an appropriate header file for exporting something from core
mm - what happens if arch/mn10300 wants it?.
I guess include/linux/mm.h is the place.
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Andrew Morton @ 2014-05-20 7:32 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-arch, riel, Madhavan Srinivasan, dave.hansen, peterz, x86,
Hugh Dickins, linux-kernel, linux-mm, ak, paulus, mgorman,
linuxppc-dev, mingo, Kirill A. Shutemov
In-Reply-To: <87oaythsvk.fsf@rustcorp.com.au>
On Tue, 20 May 2014 15:52:07 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> > Andrew Morton wrote:
> >> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> >>
> >> > Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> >> > the order of the fault-around size in bytes, and fault_around_pages()
> >> > use 1UL << (fault_around_order - PAGE_SHIFT)
> >>
> >> Yes. And shame on me for missing it (this time!) at review.
> >>
> >> There's still time to fix this. Patches, please.
> >
> > Here it is. Made at 3.30 AM, build tested only.
>
> Prefer on top of Maddy's patch which makes it always a variable, rather
> than CONFIG_DEBUG_FS. It's got enough hair as it is.
>
We're at 3.15-rc5 and this interface should be finalised for 3.16. So
Kirrill's patch is pretty urgent and should come first.
Well. It's only a debugfs interface at this stage so we are allowed to
change it later, but it's better not to.
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Madhavan Srinivasan @ 2014-05-20 7:53 UTC (permalink / raw)
To: Andrew Morton, Rusty Russell
Cc: linux-arch, riel, ak, dave.hansen, peterz, x86, Hugh Dickins,
linux-kernel, linux-mm, paulus, mgorman, linuxppc-dev, mingo,
Kirill A. Shutemov
In-Reply-To: <20140520003201.a2360d5d.akpm@linux-foundation.org>
On Tuesday 20 May 2014 01:02 PM, Andrew Morton wrote:
> On Tue, 20 May 2014 15:52:07 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>
>> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
>>> Andrew Morton wrote:
>>>> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
>>>>
>>>>> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
>>>>> the order of the fault-around size in bytes, and fault_around_pages()
>>>>> use 1UL << (fault_around_order - PAGE_SHIFT)
>>>>
>>>> Yes. And shame on me for missing it (this time!) at review.
>>>>
>>>> There's still time to fix this. Patches, please.
>>>
>>> Here it is. Made at 3.30 AM, build tested only.
>>
>> Prefer on top of Maddy's patch which makes it always a variable, rather
>> than CONFIG_DEBUG_FS. It's got enough hair as it is.
>>
>
> We're at 3.15-rc5 and this interface should be finalised for 3.16. So
> Kirrill's patch is pretty urgent and should come first.
>
> Well. It's only a debugfs interface at this stage so we are allowed to
> change it later, but it's better not to.
>
My patchset does not change the interface, but uses the current fault
around order variable from CONFIG_DEBUG_FS block to allow changes at
runtime, instead of having a constant and some cleanup.
Thanks for review
Regards
--Maddy
^ permalink raw reply
* Re: [PATCH V4 2/2] powerpc/pseries: init fault_around_order for pseries
From: Madhavan Srinivasan @ 2014-05-20 8:03 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-arch, riel, rusty, dave.hansen, peterz, x86, linux-kernel,
linux-mm, ak, paulus, mgorman, linuxppc-dev, mingo,
kirill.shutemov
In-Reply-To: <20140520002834.aefb5a90.akpm@linux-foundation.org>
On Tuesday 20 May 2014 12:58 PM, Andrew Morton wrote:
> On Thu, 8 May 2014 14:58:16 +0530 Madhavan Srinivasan <maddy@linux.vnet.ibm.com> wrote:
>
>> --- a/arch/powerpc/platforms/pseries/pseries.h
>> +++ b/arch/powerpc/platforms/pseries/pseries.h
>> @@ -17,6 +17,8 @@ struct device_node;
>> extern void request_event_sources_irqs(struct device_node *np,
>> irq_handler_t handler, const char *name);
>>
>> +extern unsigned int fault_around_order;
>
> This isn't an appropriate header file for exporting something from core
> mm - what happens if arch/mn10300 wants it?.
>
> I guess include/linux/mm.h is the place.
>
Rusty already suggested this. My bad. Reason for adding it here was
that, I did the performance test for this platform. Will change and send
it out.
Thanks for review
Regards
Maddy
^ permalink raw reply
* Re: questions on CONFIG_PPC_ADV_DEBUG_REGS, DBCR0_BRT, and DBCR0_ACTIVE_EVENTS
From: Deepak Pandian @ 2014-05-20 8:09 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <5167457E.1050205@genband.com>
unsubcribe linuxppc-dev
On Fri, Apr 12, 2013 at 4:51 AM, Chris Friesen
<chris.friesen@genband.com> wrote:
> Hi all,
>
> Sorry for the bunch of emails, I'm working on some new stuff and running
> into issues.
>
> For CONFIG_BOOKE it appears that DBCR0_ACTIVE_EVENTS includes DBCR0_ICMP but
> not DBCR0_BRT. Is that just because none of the code paths currently using
> DBCR0_ACTIVE_EVENTS need to check DBCR0_BT?
>
> Also, in sys_debug_setcontext() why does SIG_DBG_BRANCH_TRACING return
> -EINVAL if CONFIG_PPC_ADV_DEBUG_REGS is set? Would it not be possible to
> use DBCR0_BRT?
>
> Thanks,
> Chris
>
> --
>
> Chris Friesen
> Software Designer
>
> 500 Palladium Drive, Suite 2100
> Ottawa, Ontario K2N 1C2, Canada
> www.genband.com
> office:+1.343.883.2717
> chris.friesen@genband.com
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
With Regards,
Deepak Pandian
"Deconstructing world one piece at a time"
^ permalink raw reply
* Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets
From: Anshuman Khandual @ 2014-05-20 8:14 UTC (permalink / raw)
To: Pedro Alves; +Cc: mikey, avagin, linux-kernel, oleg, michael, linuxppc-dev
In-Reply-To: <537A1889.8030801@redhat.com>
On 05/19/2014 08:13 PM, Pedro Alves wrote:
> On 05/19/2014 12:46 PM, Anshuman Khandual wrote:
>
>>>> I couldn't actually find any arch that currently returns -ENODEV in
>>>> the "active" hook. I see that binfmt_elf.c doesn't handle
>>>> regset->active() returning < 0. Guess that may be why. Looks like
>>>> something that could be cleaned up, to me.
>>>>
>> Also it does not consider the return value of regset->active(t->task, regset)
>> (whose objective is to figure out whether we need to request regset->n number
>> of elements or less than that) in the subsequent call to regset->get function.
>
> Indeed.
>
> TBC, do you plan on fixing this? Otherwise ...
Sure, thinking something like this as mentioned below. But still not sure how to use
the return type of -ENODEV from the function regset->active(). Right now if any
regset does have the active hook and it returns anything but positive value, it will
be ignored and the control moves to the next regset in view. This prevents the thread
core note type being written to the core dump.
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index aa3cb62..80672fb 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1553,7 +1553,15 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
if (regset->core_note_type && regset->get &&
(!regset->active || regset->active(t->task, regset))) {
int ret;
- size_t size = regset->n * regset->size;
+ size_t size;
+
+ /* Request only the active elements in the regset */
+ if (!regset->active)
+ size = regset->n * regset->size;
+ else
+ size = regset->active(t->task, regset)
+ * regset->size;
+
void *data = kmalloc(size, GFP_KERNEL);
if (unlikely(!data))
return 0;
>
>> Now coming to the installation of the .active hooks part for all the new regsets, it
>> should be pretty straight forward as well. Though its optional and used for elf_core_dump
>> purpose only, its worth adding them here. Example of an active function should be something
>> like this. The function is inexpensive as required.
>>
>> +static int tm_spr_active(struct task_struct *target,
>> + const struct user_regset *regset)
>> +{
>> + if (!cpu_has_feature(CPU_FTR_TM))
>> + return -ENODEV;
>
> ... unfortunately this will do the wrong thing.
I am not sure whether I understand this correctly. Are you saying that its wrong to return
-ENODEV in this case as above ?
^ permalink raw reply related
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Gavin Shan @ 2014-05-20 8:28 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, Gavin Shan, kvm-ppc, agraf, qiudayu, linuxppc-dev
In-Reply-To: <1400546244.3289.341.camel@ul30vt.home>
On Mon, May 19, 2014 at 06:37:24PM -0600, Alex Williamson wrote:
>On Tue, 2014-05-20 at 10:22 +1000, Gavin Shan wrote:
>> On Mon, May 19, 2014 at 04:33:10PM -0600, Alex Williamson wrote:
>> >On Wed, 2014-05-14 at 14:11 +1000, Gavin Shan wrote:
>> >> The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
>> >> to support EEH functionality for PCI devices, which have been
>> >> passed from host to guest via VFIO.
>>
>> Thanks for your comments, Alex.W :-)
>>
>> >
>> >Some comments throughout, but overall this seems to forgo every bit of
>> >the device ownership and protection model used by VFIO and lets the user
>> >pick arbitrary host devices and do various operations, mostly unchecked.
>> >That's not acceptable.
>> >
>>
>> As what I replied to patch[2], I'm going to let VFIO-PCI-dev fd handle
>> the newly introduced IOCTL command. That way, we should follow the VFIO
>> design principles (ownership and protection) because VFIO-PCI-dev fd
>> is owned by QEMU process usually.
>>
>> Also, the address mapping maintained in EEH will be removed.
>>
>> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> >> ---
>> >> arch/powerpc/platforms/powernv/Makefile | 1 +
>> >> arch/powerpc/platforms/powernv/eeh-vfio.c | 593 ++++++++++++++++++++++++++++++
>> >> drivers/vfio/vfio_iommu_spapr_tce.c | 12 +
>> >> include/uapi/linux/vfio.h | 57 +++
>> >> 4 files changed, 663 insertions(+)
>> >> create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>> >>
>> >> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
>> >> index 63cebb9..2b15a03 100644
>> >> --- a/arch/powerpc/platforms/powernv/Makefile
>> >> +++ b/arch/powerpc/platforms/powernv/Makefile
>> >> @@ -6,5 +6,6 @@ obj-y += opal-msglog.o
>> >> obj-$(CONFIG_SMP) += smp.o
>> >> obj-$(CONFIG_PCI) += pci.o pci-p5ioc2.o pci-ioda.o
>> >> obj-$(CONFIG_EEH) += eeh-ioda.o eeh-powernv.o
>> >> +obj-$(CONFIG_VFIO_EEH) += eeh-vfio.o
>> >> obj-$(CONFIG_PPC_SCOM) += opal-xscom.o
>> >> obj-$(CONFIG_MEMORY_FAILURE) += opal-memory-errors.o
>> >> diff --git a/arch/powerpc/platforms/powernv/eeh-vfio.c b/arch/powerpc/platforms/powernv/eeh-vfio.c
>> >> new file mode 100644
>> >> index 0000000..69d5f2d
>> >> --- /dev/null
>> >> +++ b/arch/powerpc/platforms/powernv/eeh-vfio.c
>> >> @@ -0,0 +1,593 @@
>> >> +/*
>> >> + * The file intends to support EEH funtionality for those PCI devices,
>> >> + * which have been passed through from host to guest via VFIO. So this
>> >> + * file is naturally part of VFIO implementation on PowerNV platform.
>> >> + *
>> >> + * Copyright Benjamin Herrenschmidt & Gavin Shan, IBM Corporation 2014.
>> >> + *
>> >> + * 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/init.h>
>> >> +#include <linux/io.h>
>> >> +#include <linux/irq.h>
>> >> +#include <linux/kernel.h>
>> >> +#include <linux/kvm_host.h>
>> >> +#include <linux/msi.h>
>> >> +#include <linux/pci.h>
>> >> +#include <linux/string.h>
>> >> +#include <linux/vfio.h>
>> >> +
>> >> +#include <asm/eeh.h>
>> >> +#include <asm/eeh_event.h>
>> >> +#include <asm/io.h>
>> >> +#include <asm/iommu.h>
>> >> +#include <asm/opal.h>
>> >> +#include <asm/msi_bitmap.h>
>> >> +#include <asm/pci-bridge.h>
>> >> +#include <asm/ppc-pci.h>
>> >> +#include <asm/tce.h>
>> >> +#include <asm/uaccess.h>
>> >> +
>> >> +#include "powernv.h"
>> >> +#include "pci.h"
>> >> +
>> >> +static int powernv_eeh_vfio_map(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pci_bus *bus, *pe_bus;
>> >> + struct pci_dev *pdev;
>> >> + struct eeh_dev *edev;
>> >> + struct eeh_pe *pe;
>> >> + int domain, bus_no, devfn;
>> >> +
>> >> + /* Host address */
>> >> + domain = info->map.host_domain;
>> >> + bus_no = (info->map.host_cfg_addr >> 8) & 0xff;
>> >> + devfn = info->map.host_cfg_addr & 0xff;
>> >
>> >Where are we validating that the user has any legitimate claim to be
>> >touching this device?
>> >
>>
>> I'll let VFIO-PCI-dev fd handle the IOCTL command. With that, we shouldn't
>> have the problem.
>>
>> >> + /* Find PCI bus */
>> >> + bus = pci_find_bus(domain, bus_no);
>> >> + if (!bus) {
>> >> + pr_warn("%s: PCI bus %04x:%02x not found\n",
>> >> + __func__, domain, bus_no);
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* Find PCI device */
>> >> + pdev = pci_get_slot(bus, devfn);
>> >> + if (!pdev) {
>> >> + pr_warn("%s: PCI device %04x:%02x:%02x.%01x not found\n",
>> >> + __func__, domain, bus_no,
>> >> + PCI_SLOT(devfn), PCI_FUNC(devfn));
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* No EEH device - almost impossible */
>> >> + edev = pci_dev_to_eeh_dev(pdev);
>> >> + if (unlikely(!edev)) {
>> >> + pci_dev_put(pdev);
>> >> + pr_warn("%s: No EEH dev for PCI device %s\n",
>> >> + __func__, pci_name(pdev));
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* Doesn't support PE migration between different PHBs */
>> >> + pe = edev->pe;
>> >> + if (!eeh_pe_passed(pe)) {
>> >> + pe_bus = eeh_pe_bus_get(pe);
>> >> + BUG_ON(!pe_bus);
>> >
>> >Can a user trigger this maliciously?
>> >
>> >> +
>> >> + /* PE# has format 00BBSS00 */
>> >> + pe->guest_addr.buid = info->map.guest_buid;
>> >> + pe->guest_addr.pe_addr = pe_bus->number << 16;
>> >> + eeh_pe_set_passed(pe, true);
>> >> + } else if (pe->guest_addr.buid != info->map.guest_buid) {
>> >> + pci_dev_put(pdev);
>> >> + pr_warn("%s: Mismatched PHB BUID (0x%llx, 0x%llx)\n",
>> >> + __func__, pe->guest_addr.buid, info->map.guest_buid);
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + edev->guest_addr.buid = info->map.guest_buid;
>> >> + edev->guest_addr.config_addr = info->map.guest_cfg_addr;
>> >> + eeh_dev_set_passed(edev, true);
>> >> +
>> >> + pr_debug("EEH: Host PCI dev %s to %llx-%02x:%02x.%01x\n",
>> >> + pci_name(pdev), info->map.guest_buid,
>> >> + (info->map.guest_cfg_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(info->map.guest_cfg_addr & 0xFF),
>> >> + PCI_FUNC(info->map.guest_cfg_addr & 0xFF));
>> >> +
>> >> + pci_dev_put(pdev);
>> >> + return 0;
>> >> +}
>> >
>> >So the effect of this function is that a user gets to setup an arbitrary
>> >guest mapping for an arbitrary host device and associated pe. Is that
>> >right? It seems bad.
>> >
>>
>> I'm going to remove this mapping in next revision.
>>
>> >> +
>> >> +static int powernv_eeh_vfio_unmap(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + struct pci_dev *pdev;
>> >> + struct eeh_dev *edev, *tmp;
>> >> + struct eeh_pe *pe;
>> >> + bool passed;
>> >> +
>> >> + /* Get EEH device */
>> >> + addr.buid = info->unmap.buid;
>> >> + addr.config_addr = info->unmap.cfg_addr;
>> >> + edev = eeh_vfio_dev_get(&addr);
>> >
>> >eeh_vfio_dev_get() just looks for a "passed" dev and a match for a well
>> >known address space. Seems very exploitable.
>> >
>> >> + if (!edev) {
>> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> >> + __func__, info->unmap.buid,
>> >> + (info->unmap.cfg_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(info->unmap.cfg_addr & 0xFF),
>> >> + PCI_FUNC(info->unmap.cfg_addr & 0xFF));
>> >> + return -ENODEV;
>> >> + }
>> >> +
>> >> + /* Return EEH device */
>> >> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
>> >> + eeh_dev_set_passed(edev, false);
>> >> + pdev = eeh_dev_to_pci_dev(edev);
>> >> + pr_debug("EEH: Host PCI dev %s returned\n",
>> >> + pdev ? pci_name(pdev) : "NULL");
>> >> +
>> >> + /* Return PE if no EEH device is owned by guest */
>> >> + pe = edev->pe;
>> >> + passed = false;
>> >> + eeh_pe_for_each_dev(pe, edev, tmp) {
>> >> + pdev = eeh_dev_to_pci_dev(edev);
>> >> + if (pdev && pdev->subordinate)
>> >> + continue;
>> >> +
>> >> + if (eeh_dev_passed(edev)) {
>> >> + passed = true;
>> >> + break;
>> >> + }
>> >> + }
>> >> +
>> >> + if (!passed) {
>> >> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
>> >> + eeh_pe_set_passed(pe, false);
>> >> + pr_debug("EEH: PHB#%x-PE#%x returned to host\n",
>> >> + pe->phb->global_number, pe->addr);
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_set_option(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_dev *edev;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int opcode = info->option.option;
>> >> + int ret = 0;
>> >> +
>> >> + /* Check opcode */
>> >> + if (opcode < EEH_OPT_DISABLE || opcode > EEH_OPT_THAW_DMA) {
>> >> + pr_warn("%s: opcode %d out of range (%d, %d)\n",
>> >> + __func__, opcode, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
>> >> + ret = 3;
>> >
>> >Please don't make up arbitrary return values.
>> >
>>
>> Nope, it will be turned to "-3" eventually by QEMU.
>
>Don't assume QEMU is your userspace.
>
>> That means "Invalid Parameter"
>> defined in PAPR spec.
>
>Is there value in matching the PAPR spec (which most people can't read)?
>If there is...
>
>> The IOCTL command handler return 3 values:
>>
>> < 0: Linux kernel error. For example, error from copy_from_user().
>> > 0: Error code to the EEH RTAS request, which will be returned to guest.
>> = 0: Success
>
>Maybe the ioctl return should match normal ioctl return values and the
>EEH error code can be stored somewhere in the structure.
>
Good idea and will apply it to next revision. Thanks.
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Option "enable" uses PCI config address */
>> >> + if (opcode == EEH_OPT_ENABLE) {
>> >> + addr.buid = info->option.buid;
>> >> + addr.config_addr = (info->option.addr >> 8) & 0xFFFF;
>> >> + edev = eeh_vfio_dev_get(&addr);
>> >> + if (!edev) {
>> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> >> + __func__, addr.buid,
>> >> + (addr.config_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(addr.config_addr & 0xFF),
>> >> + PCI_FUNC(addr.config_addr & 0xFF));
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> + phb = edev->phb->private_data;
>> >> + } else {
>> >> + addr.buid = info->option.buid;
>> >> + addr.pe_addr = info->option.addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot find PE %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> + }
>> >> +
>> >> + /* Insure that the EEH stuff has been initialized */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * The EEH functionality has been enabled on all PEs
>> >> + * by default. So just return success. The same situation
>> >> + * would be applied while we disable EEH functionality.
>> >> + * However, the guest isn't expected to disable that
>> >> + * at all.
>> >> + */
>> >> + if (opcode == EEH_OPT_DISABLE ||
>> >> + opcode == EEH_OPT_ENABLE) {
>> >> + ret = 0;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * Call into the IODA dependent backend in order
>> >> + * to enable DMA or MMIO for the indicated PE.
>> >> + */
>> >> + if (phb->eeh_ops && phb->eeh_ops->set_option) {
>> >> + if (phb->eeh_ops->set_option(pe, opcode)) {
>> >> + pr_warn("%s: Failure from backend\n",
>> >> + __func__);
>> >> + ret = 1;
>> >> + }
>> >> + } else {
>> >> + pr_warn("%s: Unsupported request\n",
>> >> + __func__);
>> >> + ret = 7;
>> >> + }
>> >> +
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_get_addr(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_dev *edev;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int opcode = info->addr.option;
>> >> + int ret = 0;
>> >> +
>> >> + /* Check opcode */
>> >> + if (opcode != 0 && opcode != 1) {
>> >> + pr_warn("%s: opcode %d out of range (0, 1)\n",
>> >> + __func__, opcode);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Find EEH device */
>> >> + addr.buid = info->addr.buid;
>> >> + addr.config_addr = (info->addr.cfg_addr >> 8 ) & 0xFFFF;
>> >> + edev = eeh_vfio_dev_get(&addr);
>> >> + if (!edev) {
>> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> >> + __func__, addr.buid,
>> >> + (addr.config_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(addr.config_addr & 0xFF),
>> >> + PCI_FUNC(addr.config_addr & 0xFF));
>> >> + ret = 7;
>> >> + goto out;
>> >> + }
>> >> + phb = edev->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* EEH device passed ? */
>> >> + if (!eeh_dev_passed(edev)) {
>> >> + pr_warn("%s: EEH dev %llx:%02x:%02x.%01x owned by host\n",
>> >> + __func__, addr.buid,
>> >> + (addr.config_addr >> 8) & 0xFF,
>> >> + PCI_SLOT(addr.config_addr & 0xFF),
>> >> + PCI_FUNC(addr.config_addr & 0xFF));
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * Fill result according to opcode. We don't differentiate
>> >> + * PCI bus and device sensitive PE here.
>> >> + */
>> >> + if (opcode == 0)
>> >> + info->addr.ret = edev->pe->guest_addr.pe_addr;
>> >> + else
>> >> + info->addr.ret = 1;
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_get_state(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int result, ret = 0;
>> >> +
>> >> + /* Locate the PE */
>> >> + addr.buid = info->state.buid;
>> >> + addr.pe_addr = info->state.pe_addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot locate %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Call to the IOC dependent function */
>> >> + if (phb->eeh_ops && phb->eeh_ops->get_state) {
>> >> + result = phb->eeh_ops->get_state(pe);
>> >> +
>> >> + if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> >> + (result & EEH_STATE_DMA_ENABLED) &&
>> >> + (result & EEH_STATE_MMIO_ENABLED))
>> >> + info->state.state = 0;
>> >> + else if (result & EEH_STATE_RESET_ACTIVE)
>> >> + info->state.state = 1;
>> >> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> >> + !(result & EEH_STATE_DMA_ENABLED) &&
>> >> + !(result & EEH_STATE_MMIO_ENABLED))
>> >> + info->state.state = 2;
>> >> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> >> + (result & EEH_STATE_DMA_ENABLED) &&
>> >> + !(result & EEH_STATE_MMIO_ENABLED))
>> >> + info->state.state = 4;
>> >> + else
>> >> + info->state.state = 5;
>> >> +
>> >> + ret = 0;
>> >> + } else {
>> >> + pr_warn("%s: Unsupported request\n", __func__);
>> >> + ret = 3;
>> >> + }
>> >> +
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_pe_reset(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int opcode = info->reset.option;
>> >> + int ret = 0;
>> >> +
>> >> + /* Check opcode */
>> >> + if (opcode != EEH_RESET_DEACTIVATE &&
>> >> + opcode != EEH_RESET_HOT &&
>> >> + opcode != EEH_RESET_FUNDAMENTAL) {
>> >> + pr_warn("%s: Unsupported opcode %d\n",
>> >> + __func__, opcode);
>> >
>> >Console warnings are exploitable DoS attacks.
>> >
>>
>> Yep. I'll change all pr_warn() to pr_debug() in next revision.
>>
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Locate the PE */
>> >> + addr.buid = info->reset.buid;
>> >> + addr.pe_addr = info->reset.pe_addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot locate %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /* Call into the IODA dependent backend to do the reset */
>> >> + if (!phb->eeh_ops ||
>> >> + !phb->eeh_ops->set_option ||
>> >> + !phb->eeh_ops->reset) {
>> >> + pr_warn("%s: Unsupported request\n",
>> >> + __func__);
>> >> + ret = 7;
>> >> + } else {
>> >> + /*
>> >> + * The frozen PE might be caused by the mechanism called
>> >> + * PAPR error injection, which is supposed to be one-shot
>> >> + * without "sticky" bit as being stated by the spec. But
>> >> + * the reality isn't that, at least on P7IOC. So we have
>> >> + * to clear that to avoid recrusive error, which fails the
>> >> + * recovery eventually.
>> >> + */
>> >> + if (opcode == EEH_RESET_DEACTIVATE)
>> >> + opal_pci_reset(phb->opal_id,
>> >> + OPAL_PHB_ERROR,
>> >> + OPAL_ASSERT_RESET);
>> >> +
>> >> + if (phb->eeh_ops->reset(pe, opcode)) {
>> >> + pr_warn("%s: Failure from backend\n", __func__);
>> >> + ret = 1;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * The PE is still in frozen state and we need clear that.
>> >> + * It's good to clear frozen state after deassert to avoid
>> >> + * messy IO access during reset, which might cause recrusive
>> >> + * frozen PE.
>> >> + */
>> >> + if (opcode == EEH_RESET_DEACTIVATE) {
>> >> + if (phb->eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO) ||
>> >> + phb->eeh_ops->set_option(pe, EEH_OPT_THAW_DMA)) {
>> >> + pr_warn("%s: Cannot clear frozen state\n",
>> >> + __func__);
>> >> + ret = 1;
>> >> + }
>> >> +
>> >> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
>> >> + }
>> >> + }
>> >> +
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +static int powernv_eeh_vfio_pe_config(struct vfio_eeh_info *info)
>> >> +{
>> >> + struct pnv_phb *phb;
>> >> + struct eeh_pe *pe;
>> >> + struct eeh_vfio_pci_addr addr;
>> >> + int ret = 0;
>> >> +
>> >> + /* Locate the PE */
>> >> + addr.buid = info->config.buid;
>> >> + addr.pe_addr = info->config.pe_addr;
>> >> + pe = eeh_vfio_pe_get(&addr);
>> >> + if (!pe) {
>> >> + pr_warn("%s: Cannot locate %llx:%x\n",
>> >> + __func__, addr.buid, addr.pe_addr);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> + phb = pe->phb->private_data;
>> >> +
>> >> + /* EEH enabled ? */
>> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> >> + __func__, phb->hose->global_number);
>> >> + ret = 3;
>> >> + goto out;
>> >> + }
>> >> +
>> >> + /*
>> >> + * The access to PCI config space on VFIO device has some
>> >> + * limitations. Part of PCI config space, including BAR
>> >> + * registers are not readable and writable. So the guest
>> >> + * should have stale values for those registers and we have
>> >> + * to restore them in host side.
>> >> + */
>> >> + eeh_pe_restore_bars(pe);
>> >> +out:
>> >> + return ret;
>> >> +}
>> >> +
>> >> +void eeh_vfio_release(struct iommu_table *tbl)
>> >> +{
>> >> + struct pnv_ioda_pe *pnv_pe = container_of(tbl, struct pnv_ioda_pe,
>> >> + tce32_table);
>> >> + struct pnv_phb *phb = pnv_pe->phb;
>> >> + struct eeh_pe *phb_pe, *pe;
>> >> + struct eeh_dev dev, *edev, *tmp;
>> >> +
>> >> + /* Find PHB PE */
>> >> + phb_pe = eeh_phb_pe_get(phb->hose);
>> >> + if (unlikely(!phb_pe)) {
>> >> + pr_warn("%s: Cannot find PHB#%d PE\n",
>> >> + __func__, phb->hose->global_number);
>> >> + return;
>> >> + }
>> >> +
>> >> + /* Find PE */
>> >> + memset(&dev, 0, sizeof(struct eeh_dev));
>> >> + dev.phb = phb->hose;
>> >> + dev.pe_config_addr = pnv_pe->pe_number;
>> >> + pe = eeh_pe_get(&dev);
>> >> + if (unlikely(!pe)) {
>> >> + pr_warn("%s: Cannot find PE instance for PHB#%d-PE#%d\n",
>> >> + __func__, phb->hose->global_number,
>> >> + pnv_pe->pe_number);
>> >> + return;
>> >> + }
>> >> +
>> >> + /* Release it to host */
>> >> + if (!eeh_pe_passed(pe))
>> >> + return;
>> >> +
>> >> + eeh_pe_for_each_dev(pe, edev, tmp) {
>> >> + if (!eeh_dev_passed(edev))
>> >> + continue;
>> >> +
>> >> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
>> >
>> >Is guest_addr = { 0 } not valid? As agraf already mentioned, there are
>> >a number of issues with using a guest_address for a token.
>> >
>>
>> For now, PHB BUID can't be "0". Originally, I was planing to have some code
>> in QEMU to have unique PHB BUID across the system so that guest_address could
>> be the unique token. But I'm going to remove the address mapping in next revision
>> as Alex.G suggested.
>>
>> >> + eeh_dev_set_passed(edev, false);
>> >> + }
>> >> +
>> >> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
>> >> + eeh_pe_set_passed(pe, false);
>> >> +}
>> >> +EXPORT_SYMBOL(eeh_vfio_release);
>> >> +
>> >> +int eeh_vfio_ioctl(unsigned long arg)
>> >> +{
>> >> + struct vfio_eeh_info info;
>> >> + int ret = -EINVAL;
>> >> +
>> >> + /* Copy over user argument */
>> >> + if (copy_from_user(&info, (void __user *)arg, sizeof(info))) {
>> >> + pr_warn("%s: Cannot copy user argument 0x%lx\n",
>> >> + __func__, arg);
>> >> + return -EFAULT;
>> >> + }
>> >> +
>> >> + /* Sanity check */
>> >> + if (info.argsz != sizeof(info)) {
>> >
>> >This breaks compatibility if you need to later add a new ops with a
>> >larger footprint.
>> >
>>
>> Ok. I'll fix it in next revision. Thanks for pointing it out.
>>
>> >> + pr_warn("%s: Invalid argument size (%d, %ld)\n",
>> >> + __func__, info.argsz, sizeof(info));
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + /* Route according to operation */
>> >> + switch (info.op) {
>> >> + case VFIO_EEH_OP_MAP:
>> >> + ret = powernv_eeh_vfio_map(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_UNMAP:
>> >> + ret = powernv_eeh_vfio_unmap(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_SET_OPTION:
>> >> + ret = powernv_eeh_vfio_set_option(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_GET_ADDR:
>> >> + ret = powernv_eeh_vfio_get_addr(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_GET_STATE:
>> >> + ret = powernv_eeh_vfio_get_state(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_PE_RESET:
>> >> + ret = powernv_eeh_vfio_pe_reset(&info);
>> >> + break;
>> >> + case VFIO_EEH_OP_PE_CONFIG:
>> >> + ret = powernv_eeh_vfio_pe_config(&info);
>> >> + break;
>> >> + default:
>> >> + pr_info("%s: Cannot handle op#%d\n",
>> >> + __func__, info.op);
>> >> + }
>> >> +
>> >> + /* Copy data back */
>> >> + if (!ret && copy_to_user((void __user *)arg, &info, sizeof(info))) {
>> >> + pr_warn("%s: Cannot copy to user 0x%lx\n",
>> >> + __func__, arg);
>> >> + return -EFAULT;
>> >> + }
>> >> +
>> >> + return ret;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(eeh_vfio_ioctl);
>> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> index a84788b..c45dece 100644
>> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> @@ -26,6 +26,11 @@
>> >> #define DRIVER_AUTHOR "aik@ozlabs.ru"
>> >> #define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
>> >>
>> >> +#ifdef CONFIG_VFIO_EEH
>> >> +extern void eeh_vfio_release(struct iommu_table *tbl);
>> >> +extern int eeh_vfio_ioctl(unsigned long arg);
>> >> +#endif
>> >> +
>> >> static void tce_iommu_detach_group(void *iommu_data,
>> >> struct iommu_group *iommu_group);
>> >>
>> >> @@ -283,6 +288,10 @@ static long tce_iommu_ioctl(void *iommu_data,
>> >> tce_iommu_disable(container);
>> >> mutex_unlock(&container->lock);
>> >> return 0;
>> >> +#ifdef CONFIG_VFIO_EEH
>> >
>> >I'm not a fan of all these #ifdefs, hide it in eeh_vfio_ioctl() and
>> >eeh_vfio_release() if needed.
>> >
>>
>> Ok. Will do it in next revision.
>>
>> >> + case VFIO_EEH_INFO:
>> >> + return eeh_vfio_ioctl(arg);
>> >> +#endif
>> >> }
>> >>
>> >> return -ENOTTY;
>> >> @@ -342,6 +351,9 @@ static void tce_iommu_detach_group(void *iommu_data,
>> >> /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
>> >> iommu_group_id(iommu_group), iommu_group); */
>> >> container->tbl = NULL;
>> >> +#ifdef CONFIG_VFIO_EEH
>> >> + eeh_vfio_release(tbl);
>> >> +#endif
>> >> iommu_release_ownership(tbl);
>> >> }
>> >> mutex_unlock(&container->lock);
>> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> >> index cb9023d..1fd1bfb 100644
>> >> --- a/include/uapi/linux/vfio.h
>> >> +++ b/include/uapi/linux/vfio.h
>> >> @@ -455,6 +455,63 @@ struct vfio_iommu_spapr_tce_info {
>> >>
>> >> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>> >>
>> >> +/*
>> >> + * The VFIO EEH info struct provides way to support EEH functionality
>> >> + * for PCI device that is passed from host to guest via VFIO.
>> >> + */
>> >> +#define VFIO_EEH_OP_MAP 0
>> >> +#define VFIO_EEH_OP_UNMAP 1
>> >> +#define VFIO_EEH_OP_SET_OPTION 2
>> >> +#define VFIO_EEH_OP_GET_ADDR 3
>> >> +#define VFIO_EEH_OP_GET_STATE 4
>> >> +#define VFIO_EEH_OP_PE_RESET 5
>> >> +#define VFIO_EEH_OP_PE_CONFIG 6
>> >
>> >Is this really an "info" ioctl?
>> >
>>
>> Yeah, "VFIO_EEH_INFO" isn't a good name. How about to have "VFIO_EEH_HANDLER" ?
>
>VFIO_EEH_OP perhaps. Thanks,
>
Ok. Will rename it to VFIO_EEH_OP in next revision.
>Alex
>
>> >> +
>> >> +struct vfio_eeh_info {
>> >> + __u32 argsz;
>> >> + __u32 op;
>> >> +
>> >> + union {
>> >> + struct vfio_eeh_map {
>> >> + __u32 host_domain;
>> >> + __u16 host_cfg_addr;
>> >> + __u64 guest_buid;
>> >> + __u16 guest_cfg_addr;
>> >> + } map;
>> >> + struct vfio_eeh_unmap {
>> >> + __u64 buid;
>> >> + __u16 cfg_addr;
>> >> + } unmap;
>> >> + struct vfio_eeh_set_option {
>> >> + __u64 buid;
>> >> + __u32 addr;
>> >> + __u32 option;
>> >> + } option;
>> >> + struct vfio_eeh_pe_addr {
>> >> + __u64 buid;
>> >> + __u32 cfg_addr;
>> >> + __u32 option;
>> >> + __u32 ret;
>> >> + } addr;
>> >> + struct vfio_eeh_state {
>> >> + __u64 buid;
>> >> + __u32 pe_addr;
>> >> + __u32 state;
>> >> + } state;
>> >> + struct vfio_eeh_reset {
>> >> + __u64 buid;
>> >> + __u32 pe_addr;
>> >> + __u32 option;
>> >> + } reset;
>> >> + struct vfio_eeh_config {
>> >> + __u64 buid;
>> >> + __u32 pe_addr;
>> >> + } config;
>> >> + };
>> >> +};
>> >> +
>> >> +#define VFIO_EEH_INFO _IO(VFIO_TYPE, VFIO_BASE + 21)
>> >> +
>> >> /* ***************************************************************** */
>> >>
>> >> #endif /* _UAPIVFIO_H */
>>
Thanks,
Gavin
>
^ permalink raw reply
* [PATCH RFCv4 0/4] EEH Support for VFIO PCI device
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
The series of patches intends to support EEH for PCI devices, which are
passed through to PowerKVM based guest via VFIO. The implementation is
straightforward based on the issues or problems we have to resolve to
support EEH for PowerKVM based guest.
- Emulation for EEH RTAS requests. All EEH RTAS requests goes to QEMU firstly.
If QEMU can't handle it, the request will be sent to host via newly introduced
VFIO container IOCTL command (VFIO_EEH_OP) and gets handled in host kernel.
The series of patches requires corresponding QEMU changes.
Change log
==========
v1 -> v2:
* EEH RTAS requests are routed to QEMU, and then possiblly to host kerenl.
The mechanism KVM in-kernel handling is dropped.
* Error injection is reimplemented based syscall, instead of KVM in-kerenl
handling. The logic for error injection token management is moved to
QEMU. The error injection request is routed to QEMU and then possiblly
to host kernel.
v2 -> v3:
* Make the fields in struct eeh_vfio_pci_addr, struct vfio_eeh_info based
on the comments from Alexey.
* Define macros for EEH VFIO operations (Alexey).
* Clear frozen state after successful PE reset.
* Merge original [PATCH 1/2/3] to one.
v3 -> v4:
* Remove the error injection from the patchset. Mike or I will work on that
later.
* Rename CONFIG_VFIO_EEH to VFIO_PCI_EEH.
* Rename the IOCTL command to VFIO_EEH_OP and it's handled by VFIO-PCI device
instead of VFIO container.
* Rename the IOCTL argument structure to "vfio_eeh_op" accordingly. Also, more
fields added to hold return values for RTAS requests.
* The address mapping stuff is totally removed. When opening or releasing VFIO
PCI device, notification sent to EEH to update the flags indicates the device
is passed to guest or not.
* Change pr_warn() to pr_debug() to avoid DOS as pointed by Alex.W
* Argument size check issue pointed by Alex.W.
Testing on P7
=============
- Emulex adapter
Testing on P8
=============
Need testing for more after the code is finalized. The logic is required by P7/P8
machines shouldn't be different.
Gavin Shan (4):
drivers/vfio: Introduce CONFIG_VFIO_PCI_EEH
powerpc/eeh: Flags for passed device and PE
drivers/vfio: New IOCTL command VFIO_EEH_INFO
powerpc/eeh: Avoid event on passed PE
arch/powerpc/include/asm/eeh.h | 32 +++
arch/powerpc/kernel/eeh.c | 8 +
arch/powerpc/platforms/powernv/Makefile | 1 +
arch/powerpc/platforms/powernv/eeh-ioda.c | 3 +-
arch/powerpc/platforms/powernv/eeh-vfio.c | 445 ++++++++++++++++++++++++++++++
drivers/vfio/pci/Kconfig | 6 +
drivers/vfio/pci/vfio_pci.c | 24 +-
drivers/vfio/pci/vfio_pci_private.h | 16 ++
include/uapi/linux/vfio.h | 43 +++
9 files changed, 571 insertions(+), 7 deletions(-)
create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
--
1.8.3.2
^ permalink raw reply
* [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
to support EEH functionality for PCI devices, which have been
passed from host to guest via VFIO.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/Makefile | 1 +
arch/powerpc/platforms/powernv/eeh-vfio.c | 445 ++++++++++++++++++++++++++++++
drivers/vfio/pci/vfio_pci.c | 24 +-
drivers/vfio/pci/vfio_pci_private.h | 16 ++
include/uapi/linux/vfio.h | 43 +++
5 files changed, 523 insertions(+), 6 deletions(-)
create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
index 63cebb9..45cd833 100644
--- a/arch/powerpc/platforms/powernv/Makefile
+++ b/arch/powerpc/platforms/powernv/Makefile
@@ -6,5 +6,6 @@ obj-y += opal-msglog.o
obj-$(CONFIG_SMP) += smp.o
obj-$(CONFIG_PCI) += pci.o pci-p5ioc2.o pci-ioda.o
obj-$(CONFIG_EEH) += eeh-ioda.o eeh-powernv.o
+obj-$(CONFIG_VFIO_PCI_EEH) += eeh-vfio.o
obj-$(CONFIG_PPC_SCOM) += opal-xscom.o
obj-$(CONFIG_MEMORY_FAILURE) += opal-memory-errors.o
diff --git a/arch/powerpc/platforms/powernv/eeh-vfio.c b/arch/powerpc/platforms/powernv/eeh-vfio.c
new file mode 100644
index 0000000..11adc55
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/eeh-vfio.c
@@ -0,0 +1,445 @@
+/*
+ * The file intends to support EEH funtionality for those PCI devices,
+ * which have been passed through from host to guest via VFIO. So this
+ * file is naturally part of VFIO implementation on PowerNV platform.
+ *
+ * Copyright Benjamin Herrenschmidt & Gavin Shan, IBM Corporation 2014.
+ *
+ * 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/init.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/kvm_host.h>
+#include <linux/msi.h>
+#include <linux/pci.h>
+#include <linux/string.h>
+#include <linux/vfio.h>
+
+#include <asm/eeh.h>
+#include <asm/eeh_event.h>
+#include <asm/io.h>
+#include <asm/iommu.h>
+#include <asm/opal.h>
+#include <asm/msi_bitmap.h>
+#include <asm/pci-bridge.h>
+#include <asm/ppc-pci.h>
+#include <asm/tce.h>
+#include <asm/uaccess.h>
+
+#include "powernv.h"
+#include "pci.h"
+
+static int powernv_eeh_vfio_check_dev(struct pci_dev *pdev,
+ struct eeh_dev **pedev,
+ struct eeh_pe **ppe,
+ struct pnv_phb **pphb)
+{
+ struct eeh_dev *edev;
+ struct pnv_phb *phb;
+
+ /* No device ? */
+ if (!pdev)
+ return -ENODEV;
+
+ edev = pci_dev_to_eeh_dev(pdev);
+ if (!edev || !eeh_dev_passed(edev) ||
+ !edev->pe || !eeh_pe_passed(edev->pe))
+ return -ENODEV;
+
+ /* EEH isn't supported ? */
+ phb = edev->phb->private_data;
+ if (!(phb->flags & PNV_PHB_FLAG_EEH))
+ return -EACCES;
+
+ if (pedev)
+ *pedev = edev;
+ if (ppe)
+ *ppe = edev->pe;
+ if (pphb)
+ *pphb = phb;
+
+ return 0;
+}
+
+static int powernv_eeh_vfio_set_option(struct pci_dev *pdev,
+ struct vfio_eeh_op *info)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ struct pnv_phb *phb;
+ int opcode = info->option.option;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+ if (ret) {
+ pr_debug("%s: Cannot find device\n",
+ __func__);
+ info->option.ret = -7;
+ goto out;
+ }
+
+ /* Invalid opcode ? */
+ if (opcode < EEH_OPT_DISABLE ||
+ opcode > EEH_OPT_THAW_DMA) {
+ pr_debug("%s: Opcode#%d out of range (%d, %d)\n",
+ __func__, opcode, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
+ info->option.ret = -3;
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (opcode == EEH_OPT_DISABLE ||
+ opcode == EEH_OPT_ENABLE) {
+ info->option.ret = 0;
+ } else {
+ if (!phb->eeh_ops || !phb->eeh_ops->set_option) {
+ info->option.ret = -7;
+ ret = -ENOENT;
+ goto out;
+ }
+
+ ret = phb->eeh_ops->set_option(pe, opcode);
+ if (ret) {
+ pr_debug("%s: Failure %d from backend\n",
+ __func__, ret);
+ info->option.ret = -3;
+ goto out;
+ }
+
+ info->option.ret = 0;
+ }
+out:
+ return ret;
+}
+
+static int powernv_eeh_vfio_get_addr(struct pci_dev *pdev,
+ struct vfio_eeh_op *info)
+{
+ struct pci_bus *bus;
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ struct pnv_phb *phb;
+ int opcode = info->addr.option;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+ if (ret) {
+ info->addr.ret = -3;
+ goto out;
+ }
+
+ /* Invalid opcode ? */
+ if (opcode != 0 && opcode != 1) {
+ pr_debug("%s: opcode %d out of range (0, 1)\n",
+ __func__, opcode);
+ info->addr.ret = -3;
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /*
+ * Fill result according to opcode. We don't differentiate
+ * PCI bus and device sensitive PE here.
+ */
+ if (opcode == 0) {
+ bus = eeh_pe_bus_get(pe);
+ if (!bus) {
+ info->addr.ret = -3;
+ ret = -ENODEV;
+ goto out;
+ }
+
+ info->addr.ret = 0;
+ info->addr.info = bus->number << 16;
+ } else {
+ info->addr.info = 1;
+ info->addr.ret = 1;
+ }
+out:
+ return ret;
+}
+
+static int powernv_eeh_vfio_get_state(struct pci_dev *pdev,
+ struct vfio_eeh_op *info)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ struct pnv_phb *phb;
+ int result, ret = 0;
+
+ /* Device existing ? */
+ ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+ if (ret) {
+ info->state.ret = -3;
+ goto out;
+ }
+
+ if (!phb->eeh_ops || !phb->eeh_ops->get_state) {
+ pr_debug("%s: Unsupported request\n",
+ __func__);
+ ret = -ENOENT;
+ info->state.ret = -3;
+ goto out;
+ }
+
+ result = phb->eeh_ops->get_state(pe);
+
+ if (!(result & EEH_STATE_RESET_ACTIVE) &&
+ (result & EEH_STATE_DMA_ENABLED) &&
+ (result & EEH_STATE_MMIO_ENABLED))
+ info->state.reset_state = 0;
+ else if (result & EEH_STATE_RESET_ACTIVE)
+ info->state.reset_state = 1;
+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
+ !(result & EEH_STATE_DMA_ENABLED) &&
+ !(result & EEH_STATE_MMIO_ENABLED))
+ info->state.reset_state = 2;
+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
+ (result & EEH_STATE_DMA_ENABLED) &&
+ !(result & EEH_STATE_MMIO_ENABLED))
+ info->state.reset_state = 4;
+ else
+ info->state.reset_state = 5;
+
+ info->state.ret = 0;
+ info->state.cfg_cap = 1;
+ info->state.pe_unavail_info = 1000;
+ info->state.pe_recovery_info = 0;
+
+out:
+ return ret;
+}
+
+static int powernv_eeh_vfio_pe_reset(struct pci_dev *pdev,
+ struct vfio_eeh_op *info)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ struct pnv_phb *phb;
+ int opcode = info->reset.option;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+ if (ret) {
+ info->addr.ret = -3;
+ goto out;
+ }
+
+ /* Invalid opcode ? */
+ if (opcode != EEH_RESET_DEACTIVATE &&
+ opcode != EEH_RESET_HOT &&
+ opcode != EEH_RESET_FUNDAMENTAL) {
+ pr_debug("%s: Unsupported opcode %d\n",
+ __func__, opcode);
+ ret = -EINVAL;
+ info->reset.ret = -3;
+ goto out;
+ }
+
+ /* Call into the IODA dependent backend to do the reset */
+ if (!phb->eeh_ops ||
+ !phb->eeh_ops->set_option ||
+ !phb->eeh_ops->reset) {
+ pr_debug("%s: Unsupported request\n",
+ __func__);
+ ret = -ENOENT;
+ info->reset.ret = -7;
+ goto out;
+ }
+
+ /*
+ * The frozen PE might be caused by the mechanism called
+ * PAPR error injection, which is supposed to be one-shot
+ * without "sticky" bit as being stated by the spec. But
+ * the reality isn't that, at least on P7IOC. So we have
+ * to clear that to avoid recrusive error, which fails the
+ * recovery eventually.
+ */
+ if (opcode == EEH_RESET_DEACTIVATE)
+ opal_pci_reset(phb->opal_id,
+ OPAL_PHB_ERROR,
+ OPAL_ASSERT_RESET);
+
+ ret = phb->eeh_ops->reset(pe, opcode);
+ if (ret) {
+ pr_debug("%s: Failure %d from backend\n",
+ __func__, ret);
+ info->reset.ret = -1;
+ goto out;
+ }
+
+ /*
+ * The PE is still in frozen state and we need clear that.
+ * It's good to clear frozen state after deassert to avoid
+ * messy IO access during reset, which might cause recrusive
+ * frozen PE.
+ */
+ if (opcode == EEH_RESET_DEACTIVATE) {
+ ret = phb->eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
+ if (ret) {
+ pr_debug("%s: Cannot enable DMA for PHB#%d-PE#%d (%d)\n",
+ __func__, pe->phb->global_number, pe->addr, ret);
+ info->reset.ret = -1;
+ goto out;
+ }
+
+ ret = phb->eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
+ if (ret) {
+ pr_debug("%s: Cannot enable IO for PHB#%d-PE#%d (%d)\n",
+ __func__, pe->phb->global_number, pe->addr, ret);
+ info->reset.ret = -1;
+ goto out;
+ }
+
+ eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
+ }
+
+ info->reset.ret = 0;
+out:
+ return ret;
+}
+
+static int powernv_eeh_vfio_pe_config(struct pci_dev *pdev,
+ struct vfio_eeh_op *info)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ struct pnv_phb *phb;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = powernv_eeh_vfio_check_dev(pdev, &edev, &pe, &phb);
+ if (ret) {
+ info->config.ret = -3;
+ goto out;
+ }
+
+ /*
+ * The access to PCI config space on VFIO device has some
+ * limitations. Part of PCI config space, including BAR
+ * registers are not readable and writable. So the guest
+ * should have stale values for those registers and we have
+ * to restore them in host side.
+ */
+ eeh_pe_restore_bars(pe);
+ info->config.ret = 0;
+
+out:
+ return ret;
+}
+
+int eeh_vfio_pci_open(struct pci_dev *pdev)
+{
+ struct eeh_dev *edev;
+
+ /* No PCI device ? */
+ if (!pdev)
+ return -ENODEV;
+
+ /* No EEH device ? */
+ edev = pci_dev_to_eeh_dev(pdev);
+ if (!edev || !edev->pe)
+ return -ENODEV;
+
+ eeh_dev_set_passed(edev, true);
+ eeh_pe_set_passed(edev->pe, true);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(eeh_vfio_pci_open);
+
+void eeh_vfio_pci_release(struct pci_dev *pdev)
+{
+ bool release_pe = true;
+ struct eeh_pe *pe = NULL;
+ struct eeh_dev *tmp, *edev;
+
+ /* No PCI device ? */
+ if (!pdev)
+ return;
+
+ /* No EEH device ? */
+ edev = pci_dev_to_eeh_dev(pdev);
+ if (!edev || !eeh_dev_passed(edev) ||
+ !edev->pe || !eeh_pe_passed(pe))
+ return;
+
+ /* Release device */
+ pe = edev->pe;
+ eeh_dev_set_passed(edev, false);
+
+ /* Release PE */
+ eeh_pe_for_each_dev(pe, edev, tmp) {
+ if (eeh_dev_passed(edev)) {
+ release_pe = false;
+ break;
+ }
+ }
+
+ if (release_pe)
+ eeh_pe_set_passed(pe, false);
+}
+EXPORT_SYMBOL(eeh_vfio_pci_release);
+
+int eeh_vfio_pci_ioctl(struct pci_dev *pdev,
+ unsigned long arg)
+{
+ struct vfio_eeh_op info;
+ unsigned long minsz = sizeof(info);
+ int ret = -EINVAL;
+
+ /* Copy over user argument */
+ if (copy_from_user(&info, (void __user *)arg, minsz)) {
+ pr_debug("%s: Cannot copy parameter 0x%lx\n",
+ __func__, arg);
+ return -EFAULT;
+ }
+
+ /* Sanity check */
+ if (info.argsz < minsz) {
+ pr_debug("%s: Invalid size (%d, %ld)\n",
+ __func__, info.argsz, minsz);
+ return -EINVAL;
+ }
+
+ /* Route according to operation */
+ switch (info.op) {
+ case VFIO_EEH_OP_SET_OPTION:
+ ret = powernv_eeh_vfio_set_option(pdev, &info);
+ break;
+ case VFIO_EEH_OP_GET_ADDR:
+ ret = powernv_eeh_vfio_get_addr(pdev, &info);
+ break;
+ case VFIO_EEH_OP_GET_STATE:
+ ret = powernv_eeh_vfio_get_state(pdev, &info);
+ break;
+ case VFIO_EEH_OP_PE_RESET:
+ ret = powernv_eeh_vfio_pe_reset(pdev, &info);
+ break;
+ case VFIO_EEH_OP_PE_CONFIG:
+ ret = powernv_eeh_vfio_pe_config(pdev, &info);
+ break;
+ default:
+ pr_debug("%s: Cannot handle op#%d\n",
+ __func__, info.op);
+ }
+
+ /* Copy data back */
+ if (copy_to_user((void __user *)arg, &info, minsz)) {
+ pr_debug("%s: Cannot copy parameter to user 0x%lx\n",
+ __func__, arg);
+ return -EFAULT;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(eeh_vfio_pci_ioctl);
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 7ba0424..ee82c7f 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -156,8 +156,11 @@ static void vfio_pci_release(void *device_data)
{
struct vfio_pci_device *vdev = device_data;
- if (atomic_dec_and_test(&vdev->refcnt))
+
+ if (atomic_dec_and_test(&vdev->refcnt)) {
+ eeh_vfio_pci_release(vdev->pdev);
vfio_pci_disable(vdev);
+ }
module_put(THIS_MODULE);
}
@@ -165,19 +168,26 @@ static void vfio_pci_release(void *device_data)
static int vfio_pci_open(void *device_data)
{
struct vfio_pci_device *vdev = device_data;
+ int ret;
if (!try_module_get(THIS_MODULE))
return -ENODEV;
if (atomic_inc_return(&vdev->refcnt) == 1) {
- int ret = vfio_pci_enable(vdev);
- if (ret) {
- module_put(THIS_MODULE);
- return ret;
- }
+ ret = vfio_pci_enable(vdev);
+ if (ret)
+ goto error;
+
+ ret = eeh_vfio_pci_open(vdev->pdev);
+ if (ret)
+ goto error;
}
return 0;
+
+error:
+ module_put(THIS_MODULE);
+ return ret;
}
static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
@@ -682,6 +692,8 @@ hot_reset_release:
kfree(groups);
return ret;
+ } else if (cmd == VFIO_EEH_OP) {
+ return eeh_vfio_pci_ioctl(vdev->pdev, arg);
}
return -ENOTTY;
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 9c6d5d0..1273bb6 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -90,4 +90,20 @@ extern void vfio_pci_virqfd_exit(void);
extern int vfio_config_init(struct vfio_pci_device *vdev);
extern void vfio_config_free(struct vfio_pci_device *vdev);
+
+#ifdef CONFIG_VFIO_PCI_EEH
+extern int eeh_vfio_pci_open(struct pci_dev *pdev);
+extern void eeh_vfio_pci_release(struct pci_dev *pdev);
+extern int eeh_vfio_pci_ioctl(struct pci_dev *pdev, unsigned long arg);
+#else
+static inline int eeh_vfio_pci_open(struct pci_dev *pdev)
+{
+ return 0;
+}
+static inline eeh_vfio_pci_release(struct pci_dev *pdev) { }
+static int eeh_vfio_pci_ioctl(struct pci_dev *pdev, unsigned long arg)
+{
+ return -ENOENT;
+}
+#endif /* COFNIG_VFIO_PCI_EEH */
#endif /* VFIO_PCI_PRIVATE_H */
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index cb9023d..6e7f033 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -455,6 +455,49 @@ struct vfio_iommu_spapr_tce_info {
#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
+/*
+ * The VFIO operation struct provides way to support EEH functionality
+ * for PCI device that is passed from host to guest via VFIO.
+ */
+#define VFIO_EEH_OP_SET_OPTION 0
+#define VFIO_EEH_OP_GET_ADDR 1
+#define VFIO_EEH_OP_GET_STATE 2
+#define VFIO_EEH_OP_PE_RESET 3
+#define VFIO_EEH_OP_PE_CONFIG 4
+
+struct vfio_eeh_op {
+ __u32 argsz;
+ __u32 op;
+
+ union {
+ struct vfio_eeh_set_option {
+ __u32 option;
+ __s32 ret;
+ } option;
+ struct vfio_eeh_pe_addr {
+ __u32 option;
+ __s32 ret;
+ __u32 info;
+ } addr;
+ struct vfio_eeh_pe_state {
+ __s32 ret;
+ __u32 reset_state;
+ __u32 cfg_cap;
+ __u32 pe_unavail_info;
+ __u32 pe_recovery_info;
+ } state;
+ struct vfio_eeh_reset {
+ __u32 option;
+ __s32 ret;
+ } reset;
+ struct vfio_eeh_config {
+ __s32 ret;
+ } config;
+ };
+};
+
+#define VFIO_EEH_OP _IO(VFIO_TYPE, VFIO_BASE + 21)
+
/* ***************************************************************** */
#endif /* _UAPIVFIO_H */
--
1.8.3.2
^ permalink raw reply related
* [PATCH 4/4] powerpc/eeh: Avoid event on passed PE
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
If we detects frozen state on PE that has been passed to guest, we
needn't handle it. Instead, we rely on the guest to detect and recover
it. The patch avoid EEH event on the frozen passed PE so that the guest
can have chance to handle that.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
arch/powerpc/kernel/eeh.c | 8 ++++++++
arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 9c6b899..6543f05 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -400,6 +400,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
if (ret > 0)
return ret;
+ /*
+ * If the PE has been passed to guest, we won't check the
+ * state. Instead, let the guest handle it if the PE has
+ * been frozen.
+ */
+ if (eeh_pe_passed(pe))
+ return 0;
+
/* If we already have a pending isolation event for this
* slot, we know it's bad already, we don't need to check.
* Do this checking under a lock; as multiple PCI devices
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 1b5982f..03a3ed2 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
ret = EEH_NEXT_ERR_NONE;
- } else if ((*pe)->state & EEH_PE_ISOLATED) {
+ } else if ((*pe)->state & EEH_PE_ISOLATED ||
+ eeh_pe_passed(*pe)) {
ret = EEH_NEXT_ERR_NONE;
} else {
pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/4] powerpc/eeh: Flags for passed device and PE
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
The patch introduces new flags for EEH device and PE to indicate
that the device or PE has been passed through to guest. In turn,
we will deliver EEH errors to guest for further handling, which
will be done in subsequent patches.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/eeh.h | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 7782056..34a2d83 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -72,6 +72,7 @@ struct device_node;
#define EEH_PE_RESET (1 << 2) /* PE reset in progress */
#define EEH_PE_KEEP (1 << 8) /* Keep PE on hotplug */
+#define EEH_PE_PASSTHROUGH (1 << 9) /* PE owned by guest */
struct eeh_pe {
int type; /* PE type: PHB/Bus/Device */
@@ -93,6 +94,21 @@ struct eeh_pe {
#define eeh_pe_for_each_dev(pe, edev, tmp) \
list_for_each_entry_safe(edev, tmp, &pe->edevs, list)
+static inline bool eeh_pe_passed(struct eeh_pe *pe)
+{
+ return pe ? !!(pe->state & EEH_PE_PASSTHROUGH) : false;
+}
+
+static inline void eeh_pe_set_passed(struct eeh_pe *pe, bool passed)
+{
+ if (pe) {
+ if (passed)
+ pe->state |= EEH_PE_PASSTHROUGH;
+ else
+ pe->state &= ~EEH_PE_PASSTHROUGH;
+ }
+}
+
/*
* The struct is used to trace EEH state for the associated
* PCI device node or PCI device. In future, it might
@@ -110,6 +126,7 @@ struct eeh_pe {
#define EEH_DEV_SYSFS (1 << 9) /* Sysfs created */
#define EEH_DEV_REMOVED (1 << 10) /* Removed permanently */
#define EEH_DEV_FRESET (1 << 11) /* Fundamental reset */
+#define EEH_DEV_PASSTHROUGH (1 << 12) /* Owned by guest */
struct eeh_dev {
int mode; /* EEH mode */
@@ -138,6 +155,21 @@ static inline struct pci_dev *eeh_dev_to_pci_dev(struct eeh_dev *edev)
return edev ? edev->pdev : NULL;
}
+static inline bool eeh_dev_passed(struct eeh_dev *dev)
+{
+ return dev ? !!(dev->mode & EEH_DEV_PASSTHROUGH) : false;
+}
+
+static inline void eeh_dev_set_passed(struct eeh_dev *dev, bool passed)
+{
+ if (dev) {
+ if (passed)
+ dev->mode |= EEH_DEV_PASSTHROUGH;
+ else
+ dev->mode &= ~EEH_DEV_PASSTHROUGH;
+ }
+}
+
/* Return values from eeh_ops::next_error */
enum {
EEH_NEXT_ERR_NONE = 0,
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/4] drivers/vfio: Introduce CONFIG_VFIO_PCI_EEH
From: Gavin Shan @ 2014-05-20 8:30 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-1-git-send-email-gwshan@linux.vnet.ibm.com>
The patch introduces CONFIG_VFIO_PCI_EEH for more IOCTL commands
on VFIO PCI device support EEH funtionality for PCI devices that
are passed through from host to guest.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
drivers/vfio/pci/Kconfig | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index c41b01e..dd0e0f0 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -1,6 +1,7 @@
config VFIO_PCI
tristate "VFIO support for PCI devices"
depends on VFIO && PCI && EVENTFD
+ select VFIO_PCI_EEH if PPC_POWERNV
help
Support for the PCI VFIO bus driver. This is required to make
use of PCI drivers using the VFIO framework.
@@ -16,3 +17,8 @@ config VFIO_PCI_VGA
BIOS and generic video drivers.
If you don't know what to do here, say N.
+
+config VFIO_PCI_EEH
+ tristate
+ depends on EEH && VFIO_IOMMU_SPAPR_TCE
+ default n
--
1.8.3.2
^ permalink raw reply related
* [PATCH] powerpc/powernv: Fix build error when CONFIG_SMP=n
From: Shreyas B. Prabhu @ 2014-05-20 9:55 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Geert Uytterhoeven, linux-kernel, Shreyas B. Prabhu
Fix the following build error when compiled with CONFIG_SMP=n
arch/powerpc/platforms/powernv/setup.c: In function ‘pnv_kexec_wait_secondaries_down’:
arch/powerpc/platforms/powernv/setup.c:179:4: error: implicit declaration of function ‘get_hard_smp_processor_id’ [-Werror=implicit-function-declaration]
rc = opal_query_cpu_status(get_hard_smp_processor_id(i),
The usage of get_hard_smp_processor_id() needs the declaration from <asm/smp.h>.
The file setup.c includes <linux/sched.h>, which in-turn includes <linux/smp.h>.
However, <linux/smp.h> includes <asm/smp.h> only on SMP configs and hence UP
builds fail. Fix this by directly including <asm/smp.h> in setup.c
unconditionally.
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/setup.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index 8723d32..e6bde98 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -34,6 +34,7 @@
#include <asm/rtas.h>
#include <asm/opal.h>
#include <asm/kexec.h>
+#include <asm/smp.h>
#include "powernv.h"
--
1.9.0
^ permalink raw reply related
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Alexander Graf @ 2014-05-20 10:02 UTC (permalink / raw)
To: Gavin Shan, Alex Williamson; +Cc: aik, qiudayu, linuxppc-dev, kvm-ppc
In-Reply-To: <20140520082856.GA16050@shangw>
On 20.05.14 10:28, Gavin Shan wrote:
> On Mon, May 19, 2014 at 06:37:24PM -0600, Alex Williamson wrote:
>> On Tue, 2014-05-20 at 10:22 +1000, Gavin Shan wrote:
>>> On Mon, May 19, 2014 at 04:33:10PM -0600, Alex Williamson wrote:
>>>> On Wed, 2014-05-14 at 14:11 +1000, Gavin Shan wrote:
>>>>> The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
>>>>> to support EEH functionality for PCI devices, which have been
>>>>> passed from host to guest via VFIO.
[...]
>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>> index cb9023d..1fd1bfb 100644
>>> --- a/include/uapi/linux/vfio.h
>>> +++ b/include/uapi/linux/vfio.h
>>> @@ -455,6 +455,63 @@ struct vfio_iommu_spapr_tce_info {
>>>
>>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>>
>>> +/*
>>> + * The VFIO EEH info struct provides way to support EEH functionality
>>> + * for PCI device that is passed from host to guest via VFIO.
>>> + */
>>> +#define VFIO_EEH_OP_MAP 0
>>> +#define VFIO_EEH_OP_UNMAP 1
>>> +#define VFIO_EEH_OP_SET_OPTION 2
>>> +#define VFIO_EEH_OP_GET_ADDR 3
>>> +#define VFIO_EEH_OP_GET_STATE 4
>>> +#define VFIO_EEH_OP_PE_RESET 5
>>> +#define VFIO_EEH_OP_PE_CONFIG 6
>>>> Is this really an "info" ioctl?
>>>>
>>> Yeah, "VFIO_EEH_INFO" isn't a good name. How about to have "VFIO_EEH_HANDLER" ?
>> VFIO_EEH_OP perhaps. Thanks,
>>
> Ok. Will rename it to VFIO_EEH_OP in next revision.
Is there any benefit of a multiplexing EEH ioctl over just 7 individual
ioctls?
Alex
^ permalink raw reply
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Gavin Shan @ 2014-05-20 10:23 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, Alex Williamson, qiudayu, linuxppc-dev
In-Reply-To: <537B282E.6040107@suse.de>
On Tue, May 20, 2014 at 12:02:22PM +0200, Alexander Graf wrote:
>
>On 20.05.14 10:28, Gavin Shan wrote:
>>On Mon, May 19, 2014 at 06:37:24PM -0600, Alex Williamson wrote:
>>>On Tue, 2014-05-20 at 10:22 +1000, Gavin Shan wrote:
>>>>On Mon, May 19, 2014 at 04:33:10PM -0600, Alex Williamson wrote:
>>>>>On Wed, 2014-05-14 at 14:11 +1000, Gavin Shan wrote:
>>>>>>The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
>>>>>>to support EEH functionality for PCI devices, which have been
>>>>>>passed from host to guest via VFIO.
>
>[...]
>
>>>>diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>>index cb9023d..1fd1bfb 100644
>>>>--- a/include/uapi/linux/vfio.h
>>>>+++ b/include/uapi/linux/vfio.h
>>>>@@ -455,6 +455,63 @@ struct vfio_iommu_spapr_tce_info {
>>>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>>>+/*
>>>>+ * The VFIO EEH info struct provides way to support EEH functionality
>>>>+ * for PCI device that is passed from host to guest via VFIO.
>>>>+ */
>>>>+#define VFIO_EEH_OP_MAP 0
>>>>+#define VFIO_EEH_OP_UNMAP 1
>>>>+#define VFIO_EEH_OP_SET_OPTION 2
>>>>+#define VFIO_EEH_OP_GET_ADDR 3
>>>>+#define VFIO_EEH_OP_GET_STATE 4
>>>>+#define VFIO_EEH_OP_PE_RESET 5
>>>>+#define VFIO_EEH_OP_PE_CONFIG 6
>>>>>Is this really an "info" ioctl?
>>>>>
>>>>Yeah, "VFIO_EEH_INFO" isn't a good name. How about to have "VFIO_EEH_HANDLER" ?
>>>VFIO_EEH_OP perhaps. Thanks,
>>>
>>Ok. Will rename it to VFIO_EEH_OP in next revision.
>
>Is there any benefit of a multiplexing EEH ioctl over just 7
>individual ioctls?
>
One benefit is pass one data struct to the real handler implemented
in arch/powerpc/platforms/powernv/eeh-vfio.c. With 7 commands, we
either passing "void *arg" + "command" to the handler, or export
7 functions with EXPORT_SYMBOL_GPL().
I just send RFCv4 out. VFIO_EEH_OP_{MAP, UNMAP} is removed there.
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Kirill A. Shutemov @ 2014-05-20 10:27 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-arch, riel, Madhavan Srinivasan, dave.hansen, peterz, x86,
Hugh Dickins, linux-kernel, linux-mm, ak, paulus, mgorman,
Andrew Morton, linuxppc-dev, mingo, Kirill A. Shutemov
In-Reply-To: <87oaythsvk.fsf@rustcorp.com.au>
Rusty Russell wrote:
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> > Andrew Morton wrote:
> >> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> >>
> >> > Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> >> > the order of the fault-around size in bytes, and fault_around_pages()
> >> > use 1UL << (fault_around_order - PAGE_SHIFT)
> >>
> >> Yes. And shame on me for missing it (this time!) at review.
> >>
> >> There's still time to fix this. Patches, please.
> >
> > Here it is. Made at 3.30 AM, build tested only.
>
> Prefer on top of Maddy's patch which makes it always a variable, rather
> than CONFIG_DEBUG_FS. It's got enough hair as it is.
Something like this?
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Tue, 20 May 2014 13:02:03 +0300
Subject: [PATCH] mm: nominate faultaround area in bytes rather then page order
There are evidences that faultaround feature is less relevant on
architectures with page size bigger then 4k. Which makes sense since
page fault overhead per byte of mapped area should be less there.
Let's rework the feature to specify faultaround area in bytes instead of
page order. It's 64 kilobytes for now.
The patch effectively disables faultaround on architectures with
page size >= 64k (like ppc64).
It's possible that some other size of faultaround area is relevant for a
platform. We can expose `fault_around_bytes' variable to arch-specific
code once such platforms will be found.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
mm/memory.c | 62 +++++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index 037b812a9531..252b319e8cdf 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3402,63 +3402,47 @@ void do_set_pte(struct vm_area_struct *vma, unsigned long address,
update_mmu_cache(vma, address, pte);
}
-#define FAULT_AROUND_ORDER 4
+static unsigned long fault_around_bytes = 65536;
+
+static inline unsigned long fault_around_pages(void)
+{
+ return rounddown_pow_of_two(fault_around_bytes) / PAGE_SIZE;
+}
+
+static inline unsigned long fault_around_mask(void)
+{
+ return ~(rounddown_pow_of_two(fault_around_bytes) - 1) & PAGE_MASK;
+}
-#ifdef CONFIG_DEBUG_FS
-static unsigned int fault_around_order = FAULT_AROUND_ORDER;
-static int fault_around_order_get(void *data, u64 *val)
+#ifdef CONFIG_DEBUG_FS
+static int fault_around_bytes_get(void *data, u64 *val)
{
- *val = fault_around_order;
+ *val = fault_around_bytes;
return 0;
}
-static int fault_around_order_set(void *data, u64 val)
+static int fault_around_bytes_set(void *data, u64 val)
{
- BUILD_BUG_ON((1UL << FAULT_AROUND_ORDER) > PTRS_PER_PTE);
- if (1UL << val > PTRS_PER_PTE)
+ if (val / PAGE_SIZE > PTRS_PER_PTE)
return -EINVAL;
- fault_around_order = val;
+ fault_around_bytes = val;
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fault_around_order_fops,
- fault_around_order_get, fault_around_order_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fault_around_bytes_fops,
+ fault_around_bytes_get, fault_around_bytes_set, "%llu\n");
static int __init fault_around_debugfs(void)
{
void *ret;
- ret = debugfs_create_file("fault_around_order", 0644, NULL, NULL,
- &fault_around_order_fops);
+ ret = debugfs_create_file("fault_around_bytes", 0644, NULL, NULL,
+ &fault_around_bytes_fops);
if (!ret)
- pr_warn("Failed to create fault_around_order in debugfs");
+ pr_warn("Failed to create fault_around_bytes in debugfs");
return 0;
}
late_initcall(fault_around_debugfs);
-
-static inline unsigned long fault_around_pages(void)
-{
- return 1UL << fault_around_order;
-}
-
-static inline unsigned long fault_around_mask(void)
-{
- return ~((1UL << (PAGE_SHIFT + fault_around_order)) - 1);
-}
-#else
-static inline unsigned long fault_around_pages(void)
-{
- unsigned long nr_pages;
-
- nr_pages = 1UL << FAULT_AROUND_ORDER;
- BUILD_BUG_ON(nr_pages > PTRS_PER_PTE);
- return nr_pages;
-}
-
-static inline unsigned long fault_around_mask(void)
-{
- return ~((1UL << (PAGE_SHIFT + FAULT_AROUND_ORDER)) - 1);
-}
#endif
static void do_fault_around(struct vm_area_struct *vma, unsigned long address,
@@ -3515,7 +3499,7 @@ static int do_read_fault(struct mm_struct *mm, struct vm_area_struct *vma,
* if page by the offset is not ready to be mapped (cold cache or
* something).
*/
- if (vma->vm_ops->map_pages) {
+ if (vma->vm_ops->map_pages && fault_around_pages() > 1) {
pte = pte_offset_map_lock(mm, pmd, address, &ptl);
do_fault_around(vma, address, pte, pgoff, flags);
if (!pte_same(*pte, orig_pte))
--
Kirill A. Shutemov
^ permalink raw reply related
* Re: [PATCH] powerpc/powernv: Fix build error when CONFIG_SMP=n
From: Srivatsa S. Bhat @ 2014-05-20 10:31 UTC (permalink / raw)
To: Shreyas B. Prabhu; +Cc: linuxppc-dev, linux-kernel, Geert Uytterhoeven
In-Reply-To: <1400579726-26206-1-git-send-email-shreyas@linux.vnet.ibm.com>
On 05/20/2014 03:25 PM, Shreyas B. Prabhu wrote:
> Fix the following build error when compiled with CONFIG_SMP=n
> arch/powerpc/platforms/powernv/setup.c: In function ‘pnv_kexec_wait_secondaries_down’:
> arch/powerpc/platforms/powernv/setup.c:179:4: error: implicit declaration of function ‘get_hard_smp_processor_id’ [-Werror=implicit-function-declaration]
> rc = opal_query_cpu_status(get_hard_smp_processor_id(i),
>
> The usage of get_hard_smp_processor_id() needs the declaration from <asm/smp.h>.
> The file setup.c includes <linux/sched.h>, which in-turn includes <linux/smp.h>.
> However, <linux/smp.h> includes <asm/smp.h> only on SMP configs and hence UP
> builds fail. Fix this by directly including <asm/smp.h> in setup.c
> unconditionally.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Regards,
Srivatsa S. Bhat
> ---
> arch/powerpc/platforms/powernv/setup.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index 8723d32..e6bde98 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -34,6 +34,7 @@
> #include <asm/rtas.h>
> #include <asm/opal.h>
> #include <asm/kexec.h>
> +#include <asm/smp.h>
>
> #include "powernv.h"
>
^ permalink raw reply
* Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets
From: Pedro Alves @ 2014-05-20 10:33 UTC (permalink / raw)
To: Anshuman Khandual
Cc: mikey, avagin, linux-kernel, oleg, michael, linuxppc-dev
In-Reply-To: <537B0EE0.4080406@linux.vnet.ibm.com>
On 05/20/2014 09:14 AM, Anshuman Khandual wrote:
> On 05/19/2014 08:13 PM, Pedro Alves wrote:
>> On 05/19/2014 12:46 PM, Anshuman Khandual wrote:
>>
>>>>> I couldn't actually find any arch that currently returns -ENODEV in
>>>>> the "active" hook. I see that binfmt_elf.c doesn't handle
>>>>> regset->active() returning < 0. Guess that may be why. Looks like
>>>>> something that could be cleaned up, to me.
>>>>>
>>> Also it does not consider the return value of regset->active(t->task, regset)
>>> (whose objective is to figure out whether we need to request regset->n number
>>> of elements or less than that) in the subsequent call to regset->get function.
>>
>> Indeed.
>>
>> TBC, do you plan on fixing this? Otherwise ...
>
> Sure, thinking something like this as mentioned below. But still not sure how to use
> the return type of -ENODEV from the function regset->active(). Right now if any
> regset does have the active hook and it returns anything but positive value, it will
> be ignored and the control moves to the next regset in view. This prevents the thread
> core note type being written to the core dump.
Looks to me that that's exactly what should happen for -ENODEV too. The regset
should be ignored. If regset->active() returns -ENODEV, then the machine
doesn't have the registers at all, so what makes sense to me is to not write the
corresponding core note in the dump. IOW, on such a machine, the kernel
generates a core exactly like if the support for these registers that don't
make sense for this machine wasn't compiled in at all. And generates a core
exactly like an older kernel that didn't know about that regset
(which is fine for that same machine) yet.
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index aa3cb62..80672fb 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1553,7 +1553,15 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
> if (regset->core_note_type && regset->get &&
> (!regset->active || regset->active(t->task, regset))) {
> int ret;
So, here, this ?
(!regset->active || regset->active(t->task, regset) > 0)) {
> - size_t size = regset->n * regset->size;
> + size_t size;
> +
> + /* Request only the active elements in the regset */
> + if (!regset->active)
> + size = regset->n * regset->size;
> + else
> + size = regset->active(t->task, regset)
> + * regset->size;
> +
I wonder if it wouldn't be cleaner to add a function like:
int
regset_active (tast *task, regseg *regset)
{
if (!regset->active)
return regset->n * regset->size;
else
return regset->active(task, regset);
}
And then use it like
if (regset->core_note_type && regset->get) {
int size = regset_active (t->task, regset);
if (size > 0) {
...
}
Though at this point, we don't actually make use of
the distinction between -ENODEV vs 0. Guess that's what
we should be thinking about. Seems like there some details that
need to be sorted out, and some verification that consumers aren't
broken by outputting smaller notes -- e.g., ia64 makes me
wonder that.
Maybe we should leave this for another day, and have tm_spr_active
return 0 instead of -ENODEV when the machine doesn't have the hardware,
or not install that hook at all. Seems like the effect will be the same,
as the note isn't output if ->get fails.
> void *data = kmalloc(size, GFP_KERNEL);
> if (unlikely(!data))
> return 0;
>
>>
>>> Now coming to the installation of the .active hooks part for all the new regsets, it
>>> should be pretty straight forward as well. Though its optional and used for elf_core_dump
>>> purpose only, its worth adding them here. Example of an active function should be something
>>> like this. The function is inexpensive as required.
>>>
>>> +static int tm_spr_active(struct task_struct *target,
>>> + const struct user_regset *regset)
>>> +{
>>> + if (!cpu_has_feature(CPU_FTR_TM))
>>> + return -ENODEV;
>>
>> ... unfortunately this will do the wrong thing.
>
> I am not sure whether I understand this correctly. Are you saying that its wrong to return
> -ENODEV in this case as above ?
No, sorry for not being clear. The (...)'s were connected:
"do you plan on fixing this? Otherwise ... ... unfortunately
this will do the wrong thing."
--
Pedro Alves
^ permalink raw reply
* Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Alexander Graf @ 2014-05-20 11:21 UTC (permalink / raw)
To: Gavin Shan, kvm-ppc; +Cc: aik, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-4-git-send-email-gwshan@linux.vnet.ibm.com>
On 20.05.14 10:30, Gavin Shan wrote:
> The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
> to support EEH functionality for PCI devices, which have been
> passed from host to guest via VFIO.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
> arch/powerpc/platforms/powernv/Makefile | 1 +
> arch/powerpc/platforms/powernv/eeh-vfio.c | 445 ++++++++++++++++++++++++++++++
> drivers/vfio/pci/vfio_pci.c | 24 +-
> drivers/vfio/pci/vfio_pci_private.h | 16 ++
> include/uapi/linux/vfio.h | 43 +++
> 5 files changed, 523 insertions(+), 6 deletions(-)
> create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
Why doesn't this code live inside the vfio module? If I don't load the
vfio module, I don't need that code to waste memory in my kernel, no?
Alex
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/eeh: Avoid event on passed PE
From: Alexander Graf @ 2014-05-20 11:25 UTC (permalink / raw)
To: Gavin Shan, kvm-ppc; +Cc: aik, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400574612-19411-5-git-send-email-gwshan@linux.vnet.ibm.com>
On 20.05.14 10:30, Gavin Shan wrote:
> If we detects frozen state on PE that has been passed to guest, we
> needn't handle it. Instead, we rely on the guest to detect and recover
> it. The patch avoid EEH event on the frozen passed PE so that the guest
> can have chance to handle that.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
How does the guest learn about this failure? We'd need to inject an
error into it, no?
I think what you want is an irqfd that the in-kernel eeh code notifies
when it sees a failure. When such an fd exists, the kernel skips its own
error handling.
Alex
> ---
> arch/powerpc/kernel/eeh.c | 8 ++++++++
> arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 9c6b899..6543f05 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -400,6 +400,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
> if (ret > 0)
> return ret;
>
> + /*
> + * If the PE has been passed to guest, we won't check the
> + * state. Instead, let the guest handle it if the PE has
> + * been frozen.
> + */
> + if (eeh_pe_passed(pe))
> + return 0;
> +
> /* If we already have a pending isolation event for this
> * slot, we know it's bad already, we don't need to check.
> * Do this checking under a lock; as multiple PCI devices
> diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
> index 1b5982f..03a3ed2 100644
> --- a/arch/powerpc/platforms/powernv/eeh-ioda.c
> +++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
> @@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
> opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
> OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
> ret = EEH_NEXT_ERR_NONE;
> - } else if ((*pe)->state & EEH_PE_ISOLATED) {
> + } else if ((*pe)->state & EEH_PE_ISOLATED ||
> + eeh_pe_passed(*pe)) {
> ret = EEH_NEXT_ERR_NONE;
> } else {
> pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
^ permalink raw reply
* Re: [PATCH 3/4] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Alexander Graf @ 2014-05-20 11:28 UTC (permalink / raw)
To: Gavin Shan, kvm-ppc; +Cc: aik, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <537B3AA7.7040106@suse.de>
On 20.05.14 13:21, Alexander Graf wrote:
>
> On 20.05.14 10:30, Gavin Shan wrote:
>> The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>> to support EEH functionality for PCI devices, which have been
>> passed from host to guest via VFIO.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/platforms/powernv/Makefile | 1 +
>> arch/powerpc/platforms/powernv/eeh-vfio.c | 445
>> ++++++++++++++++++++++++++++++
>> drivers/vfio/pci/vfio_pci.c | 24 +-
>> drivers/vfio/pci/vfio_pci_private.h | 16 ++
>> include/uapi/linux/vfio.h | 43 +++
>> 5 files changed, 523 insertions(+), 6 deletions(-)
>> create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>
> Why doesn't this code live inside the vfio module? If I don't load the
> vfio module, I don't need that code to waste memory in my kernel, no?
So I think from a modeling point of view, you want VFIO code that calls
reasonably generic helpers inside the kernel to deal with errors.
The "generic helpers" don't have anything to do with VFIO. Everything
that interfaces via ioctls with user space is 100% VFIO code.
The latter should be tristate inside vfio.ko, the former can be =y.
Alex
^ 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