* Re: [PATCH 6/7] powerpc/eeh: Allow disabling recovery
From: Michael Ellerman @ 2019-02-08 9:58 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-6-oohall@gmail.com>
Oliver O'Halloran <oohall@gmail.com> writes:
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index d1f0bdf41fac..92809b137e39 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1810,7 +1817,11 @@ static int __init eeh_init_proc(void)
> &eeh_enable_dbgfs_ops);
> debugfs_create_u32("eeh_max_freezes", 0600,
> powerpc_debugfs_root, &eeh_max_freezes);
> + debugfs_create_bool("eeh_disable_recovery", 0600,
> + powerpc_debugfs_root,
> + &eeh_debugfs_no_recover);
> eeh_cache_debugfs_init();
> +#endif
There's that endif.
Whem I'm doing rebasing and think I might have broken bisectability I
build every commit with:
https://github.com/mpe/misc-scripts/blob/master/git/for-each-commit
cheers
^ permalink raw reply
* Re: [PATCH 5/7] powerpc/pci: Add pci_find_hose_for_domain()
From: Michael Ellerman @ 2019-02-08 9:57 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-5-oohall@gmail.com>
Oliver O'Halloran <oohall@gmail.com> writes:
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index aee4fcc24990..149053b7f481 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -274,6 +274,8 @@ extern int pcibios_map_io_space(struct pci_bus *bus);
> extern struct pci_controller *pci_find_hose_for_OF_device(
> struct device_node* node);
>
> +extern struct pci_controller *pci_find_hose_for_domain(uint32_t domain_nr);
I know we use "hose" a lot in the PCI code, but it's a stupid name. Can
we not introduce new usages?
It returns a pci_controller so pci_find_controller_for_domain() ?
cheers
^ permalink raw reply
* Re: [PATCH 3/7] powerpc/eeh_cache: Add a way to dump the EEH address cache
From: Michael Ellerman @ 2019-02-08 9:47 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-3-oohall@gmail.com>
Oliver O'Halloran <oohall@gmail.com> writes:
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index f6e65375a8de..d1f0bdf41fac 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1810,7 +1810,7 @@ static int __init eeh_init_proc(void)
> &eeh_enable_dbgfs_ops);
> debugfs_create_u32("eeh_max_freezes", 0600,
> powerpc_debugfs_root, &eeh_max_freezes);
> -#endif
> + eeh_cache_debugfs_init();
Oops :)
> diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
> index b2c320e0fcef..dba421a577e7 100644
> --- a/arch/powerpc/kernel/eeh_cache.c
> +++ b/arch/powerpc/kernel/eeh_cache.c
> @@ -298,9 +299,34 @@ void eeh_addr_cache_build(void)
> eeh_addr_cache_insert_dev(dev);
> eeh_sysfs_add_device(dev);
> }
> +}
>
> -#ifdef DEBUG
> - /* Verify tree built up above, echo back the list of addrs. */
> - eeh_addr_cache_print(&pci_io_addr_cache_root);
> -#endif
> +static int eeh_addr_cache_show(struct seq_file *s, void *v)
> +{
> + struct rb_node *n = rb_first(&pci_io_addr_cache_root.rb_root);
> + struct pci_io_addr_range *piar;
> + int cnt = 0;
> +
> + spin_lock(&pci_io_addr_cache_root.piar_lock);
> + while (n) {
> + piar = rb_entry(n, struct pci_io_addr_range, rb_node);
> +
> + seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n",
> + (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", cnt,
> + &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev));
> +
> + n = rb_next(n);
> + cnt++;
> + }
You can write that as a for loop can't you?
struct rb_node *n;
int i = 0;
for (n = rb_first(&pci_io_addr_cache_root.rb_root); n; n = rb_next(n), i++) {
piar = rb_entry(n, struct pci_io_addr_range, rb_node);
seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n",
(piar->flags & IORESOURCE_IO) ? "i/o" : "mem", i,
&piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev));
}
cheers
^ permalink raw reply
* Re: [PATCH 1/7] powerpc/eeh: Use debugfs_create_u32 for eeh_max_freezes
From: Michael Ellerman @ 2019-02-08 9:38 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-1-oohall@gmail.com>
Oliver O'Halloran <oohall@gmail.com> writes:
> There's no need to the custom getter/setter functions so we should remove
> them in favour of using the generic one. While we're here, change the type
> of eeh_max_freeze to uint32_t and print the value in decimal rather than
Please use kernel types, ie. u32.
Look fine otherwise.
cheers
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> arch/powerpc/include/asm/eeh.h | 2 +-
> arch/powerpc/kernel/eeh.c | 21 +++------------------
> 2 files changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 8b596d096ebe..c003628441cc 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -219,7 +219,7 @@ struct eeh_ops {
> };
>
> extern int eeh_subsystem_flags;
> -extern int eeh_max_freezes;
> +extern uint32_t eeh_max_freezes;
> extern struct eeh_ops *eeh_ops;
> extern raw_spinlock_t confirm_error_lock;
>
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index ae05203eb4de..f6e65375a8de 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -109,7 +109,7 @@ EXPORT_SYMBOL(eeh_subsystem_flags);
> * frozen count in last hour exceeds this limit, the PE will
> * be forced to be offline permanently.
> */
> -int eeh_max_freezes = 5;
> +uint32_t eeh_max_freezes = 5;
>
> /* Platform dependent EEH operations */
> struct eeh_ops *eeh_ops = NULL;
> @@ -1796,22 +1796,8 @@ static int eeh_enable_dbgfs_get(void *data, u64 *val)
> return 0;
> }
>
> -static int eeh_freeze_dbgfs_set(void *data, u64 val)
> -{
> - eeh_max_freezes = val;
> - return 0;
> -}
> -
> -static int eeh_freeze_dbgfs_get(void *data, u64 *val)
> -{
> - *val = eeh_max_freezes;
> - return 0;
> -}
> -
> DEFINE_DEBUGFS_ATTRIBUTE(eeh_enable_dbgfs_ops, eeh_enable_dbgfs_get,
> eeh_enable_dbgfs_set, "0x%llx\n");
> -DEFINE_DEBUGFS_ATTRIBUTE(eeh_freeze_dbgfs_ops, eeh_freeze_dbgfs_get,
> - eeh_freeze_dbgfs_set, "0x%llx\n");
> #endif
>
> static int __init eeh_init_proc(void)
> @@ -1822,9 +1808,8 @@ static int __init eeh_init_proc(void)
> debugfs_create_file_unsafe("eeh_enable", 0600,
> powerpc_debugfs_root, NULL,
> &eeh_enable_dbgfs_ops);
> - debugfs_create_file_unsafe("eeh_max_freezes", 0600,
> - powerpc_debugfs_root, NULL,
> - &eeh_freeze_dbgfs_ops);
> + debugfs_create_u32("eeh_max_freezes", 0600,
> + powerpc_debugfs_root, &eeh_max_freezes);
> #endif
> }
>
> --
> 2.20.1
^ permalink raw reply
* Re: use generic DMA mapping code in powerpc V4
From: Christoph Hellwig @ 2019-02-08 9:18 UTC (permalink / raw)
To: Christian Zigotzky
Cc: linux-arch, Darren Stevens, linux-kernel, Julian Margetson,
linux-mm, iommu, Paul Mackerras, Olof Johansson, linuxppc-dev,
Christoph Hellwig
In-Reply-To: <7c1f208b-6909-3b0a-f9f9-38ff1ac3d617@xenosoft.de>
On Fri, Feb 08, 2019 at 10:01:46AM +0100, Christian Zigotzky wrote:
> Hi Christoph,
>
> Your new patch fixes the problems with the P.A. Semi Ethernet! :-)
Thanks a lot once again for testing!
Now can you test with this patch and the whole series?
I've updated the powerpc-dma.6 branch to include this fix.
^ permalink raw reply
* Re: use generic DMA mapping code in powerpc V4
From: Christian Zigotzky @ 2019-02-08 9:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Darren Stevens, linux-kernel, Julian Margetson,
linux-mm, iommu, Paul Mackerras, Olof Johansson, linuxppc-dev
In-Reply-To: <61EC67B1-12EF-42B6-B69B-B59F9E4FC474@xenosoft.de>
Hi Christoph,
Your new patch fixes the problems with the P.A. Semi Ethernet! :-)
Thanks,
Christian
On 07 February 2019 at 05:34AM, Christian Zigotzky wrote:
> Hi Christoph,
>
> I also didn’t notice the 32-bit DMA mask in your patch. I have to read your patches and descriptions carefully in the future. I will test your new patch at the weekend.
>
> Thanks,
> Christian
>
> Sent from my iPhone
>
>> On 6. Feb 2019, at 16:16, Christoph Hellwig <hch@lst.de> wrote:
>>
>>> On Wed, Feb 06, 2019 at 04:15:05PM +0100, Christoph Hellwig wrote:
>>> The last good one was 29e7e2287e196f48fe5d2a6e017617723ea979bf
>>> ("dma-direct: we might need GFP_DMA for 32-bit dma masks"), if I
>>> remember correctly. powerpc/dma: use the dma_direct mapping routines
>>> was the one that you said makes the pasemi ethernet stop working.
>>>
>>> Can you post the dmesg from the failing runs?
>> But I just noticed I sent you a wrong patch - the pasemi ethernet
>> should set a 64-bit DMA mask, not 32-bit. Updated version below,
>> 32-bit would just keep the previous status quo.
>>
>> commit 6c8f88045dee35933337b9ce2ea5371eee37073a
>> Author: Christoph Hellwig <hch@lst.de>
>> Date: Mon Feb 4 13:38:22 2019 +0100
>>
>> pasemi WIP
>>
>> diff --git a/drivers/net/ethernet/pasemi/pasemi_mac.c b/drivers/net/ethernet/pasemi/pasemi_mac.c
>> index 8a31a02c9f47..2d7d1589490a 100644
>> --- a/drivers/net/ethernet/pasemi/pasemi_mac.c
>> +++ b/drivers/net/ethernet/pasemi/pasemi_mac.c
>> @@ -1716,6 +1716,7 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> err = -ENODEV;
>> goto out;
>> }
>> + dma_set_mask(&mac->dma_pdev->dev, DMA_BIT_MASK(64));
>>
>> mac->iob_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa001, NULL);
>> if (!mac->iob_pdev) {
^ permalink raw reply
* Re: [PATCH 3/7] powerpc/eeh_cache: Add a way to dump the EEH address cache
From: kbuild test robot @ 2019-02-08 9:00 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: Oliver O'Halloran, linuxppc-dev, kbuild-all
In-Reply-To: <20190208030802.10805-3-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]
Hi Oliver,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.0-rc4 next-20190207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oliver-O-Halloran/powerpc-eeh-Use-debugfs_create_u32-for-eeh_max_freezes/20190208-145918
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=powerpc
Note: the linux-review/Oliver-O-Halloran/powerpc-eeh-Use-debugfs_create_u32-for-eeh_max_freezes/20190208-145918 HEAD a8dcd44575537e3e67a44fe3139b273a64c0f620 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
>> arch/powerpc/kernel/eeh.c:1840: error: unterminated #ifdef
#ifdef CONFIG_DEBUG_FS
vim +1840 arch/powerpc/kernel/eeh.c
7f52a526f arch/powerpc/kernel/eeh.c Gavin Shan 2014-04-24 1835
^1da177e4 arch/ppc64/kernel/eeh.c Linus Torvalds 2005-04-16 1836 static int __init eeh_init_proc(void)
^1da177e4 arch/ppc64/kernel/eeh.c Linus Torvalds 2005-04-16 1837 {
7f52a526f arch/powerpc/kernel/eeh.c Gavin Shan 2014-04-24 1838 if (machine_is(pseries) || machine_is(powernv)) {
3f3942aca arch/powerpc/kernel/eeh.c Christoph Hellwig 2018-05-15 1839 proc_create_single("powerpc/eeh", 0, NULL, proc_eeh_show);
7f52a526f arch/powerpc/kernel/eeh.c Gavin Shan 2014-04-24 @1840 #ifdef CONFIG_DEBUG_FS
:::::: The code at line 1840 was first introduced by commit
:::::: 7f52a526f64c69c913f0027fbf43821ff0b3a7d7 powerpc/eeh: Allow to disable EEH
:::::: TO: Gavin Shan <gwshan@linux.vnet.ibm.com>
:::::: CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59719 bytes --]
^ permalink raw reply
* Re: [PATCH 06/19] KVM: PPC: Book3S HV: add a GET_ESB_FD control to the XIVE native device
From: Cédric Le Goater @ 2019-02-08 7:58 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190208051523.GD2688@umbus.fritz.box>
On 2/8/19 6:15 AM, David Gibson wrote:
> On Thu, Feb 07, 2019 at 10:03:15AM +0100, Cédric Le Goater wrote:
>> On 2/7/19 3:49 AM, David Gibson wrote:
>>> On Wed, Feb 06, 2019 at 08:21:10AM +0100, Cédric Le Goater wrote:
>>>> On 2/6/19 2:23 AM, David Gibson wrote:
>>>>> On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
>>>>>> On 2/5/19 6:28 AM, David Gibson wrote:
>>>>>>> On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
>>>>>>>> On 2/4/19 5:45 AM, David Gibson wrote:
>>>>>>>>> On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
>>>>>>>>>> This will let the guest create a memory mapping to expose the ESB MMIO
>>>>>>>>>> regions used to control the interrupt sources, to trigger events, to
>>>>>>>>>> EOI or to turn off the sources.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>>>>>> ---
>>>>>>>>>> arch/powerpc/include/uapi/asm/kvm.h | 4 ++
>>>>>>>>>> arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
>>>>>>>>>> 2 files changed, 101 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>>>>>>>>>> index 8c876c166ef2..6bb61ba141c2 100644
>>>>>>>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>>>>>>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>>>>>>>>> @@ -675,4 +675,8 @@ struct kvm_ppc_cpu_char {
>>>>>>>>>> #define KVM_XICS_PRESENTED (1ULL << 43)
>>>>>>>>>> #define KVM_XICS_QUEUED (1ULL << 44)
>>>>>>>>>>
>>>>>>>>>> +/* POWER9 XIVE Native Interrupt Controller */
>>>>>>>>>> +#define KVM_DEV_XIVE_GRP_CTRL 1
>>>>>>>>>> +#define KVM_DEV_XIVE_GET_ESB_FD 1
>>>>>>>>>
>>>>>>>>> Introducing a new FD for ESB and TIMA seems overkill. Can't you get
>>>>>>>>> to both with an mmap() directly on the xive device fd? Using the
>>>>>>>>> offset to distinguish which one to map, obviously.
>>>>>>>>
>>>>>>>> The page offset would define some sort of user API. It seems feasible.
>>>>>>>> But I am not sure this would be practical in the future if we need to
>>>>>>>> tune the length.
>>>>>>>
>>>>>>> Um.. why not? I mean, yes the XIVE supports rather a lot of
>>>>>>> interrupts, but we have 64-bits of offset we can play with - we can
>>>>>>> leave room for billions of ESB slots and still have room for billions
>>>>>>> of VPs.
>>>>>>
>>>>>> So the first 4 pages could be the TIMA pages and then would come
>>>>>> the pages for the interrupt ESBs. I think that we can have different
>>>>>> vm_fault handler for each mapping.
>>>>>
>>>>> Um.. no, I'm saying you don't need to tightly pack them. So you could
>>>>> have the ESB pages at 0, the TIMA at, say offset 2^60.
>>>>
>>>> Well, we know that the TIMA is 4 pages wide and is "directly" related
>>>> with the KVM interrupt device. So being at offset 0 seems a good idea.
>>>> While the ESB segment is of a variable size depending on the number
>>>> of IRQs and it can come after I think.
>>>>
>>>>>> I wonder how this will work out with pass-through. As Paul said in
>>>>>> a previous email, it would be better to let QEMU request a new
>>>>>> mapping to handle the ESB pages of the device being passed through.
>>>>>> I guess this is not a special case, just another offset and length.
>>>>>
>>>>> Right, if we need multiple "chunks" of ESB pages we can given them
>>>>> each their own terabyte or several. No need to be stingy with address
>>>>> space.
>>>>
>>>> You can not put them anywhere. They should map the same interrupt range
>>>> of ESB pages, overlapping with the underlying segment of IPI ESB pages.
>>>
>>> I don't really follow what you're saying here.
>>
>>
>> What we want the guest to access in terms of ESB pages is something like
>> below, VMA0 being the initial mapping done by QEMU at offset 0x0, the IPI
>> ESB pages being populated on the demand with the loads and the stores from
>> the guest :
>>
>>
>> 0x0 0x1100 0x1200 0x1300
>>
>> ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
>>
>> +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
>> VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
>> pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
>>
>>
>>
>> A device is passed through and the driver requests MSIs.
>>
>> We now want the guest to access the HW ESB pages for the requested IRQs
>> but still the initial IPI ESB pages for the others. Something like below :
>>
>>
>> 0x0 0x1100 0x1200 0x1300
>>
>> ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
>>
>> +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
>> VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
>> pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
>>
>> VMA1 PHB ESB +-------+
>> pages | | | | |
>> +-------+
>
> Right, except of course VMA0 will be split into two pieces by
> performing the mmap() over it.
>
>> The VMA1 is the result of a new mmap() being done at an offset depending on
>> the first IRQ number requested by the driver.
>
> Right... that's one way we could do it. But the irq numbers are all
> dynamically allocated here, so could we instead just put the
> passthrough MSIs in a separate range?
Hmm, yes. These are still MSIs. I am not sure of the benefits. See below.
> We'd still need a separate
> mmap() for them, but we wouldn't have to deal with mapping over and
> unmapping if the device is removed or whatever.
How would we handle multiples devices being hot-plugged, hot-unplugged
and hot-replugged ? The ESB pages would be populated the first time
they are touched and might not be the correct ones if a new device is
hot-plugged to the machine.
>> This is because the vm_fault handler uses the page offset to find the
>> associated KVM IRQ struct containing the addresses of the EOI and trigger
>> pages in the underlying hardware, which will be the PHB in case of a
>> passthrough device.
>>
>> From there, the VMA1 mmap() pointer will be used to create a 'ram device'
>> memory region which will be mapped on top of the initial ESB memory region
>> in QEMU. This will override the initial IPI ESB pages with the PHB ESB pages
>> in the guest ESB address space.
>
> Um.. what? If that qemu memory range is already mapped into the guest
> we don't need to create new RAM devices or anything for the
> overmapping. If we overmap in qemu that will just get carried into
> the guest.
yes, that's the goal.
When the guest accesses the region, the vm_fault handler will be invoked
and the VMA will be populated with the ESB pages of the device being
passthrough. When the device is removed from the machine, we only need
to delete the region from QEMU and munmap() the VMA to clear the mappings.
The underlying pages will be the ones for the XIVE IC IPIs.
And the IRQ numbers can be safely recycled for another passthrough device.
>> That's the plan I have in mind as suggested by Paul if I understood it well.
>> The mechanics are more complex than the patch zapping the PTEs from the VMA
>> but it's also safer.
>
> Well, yes, where "safer" means "has the possibility to be correct".
Well, the only problem with the kernel approach is keeping a pointer on
the VMA. If we could call find_vma(), it would be perfectly safe and much
more simpler.
C.
^ permalink raw reply
* Re: [PATCH 00/19] KVM: PPC: Book3S HV: add XIVE native exploitation mode
From: Cédric Le Goater @ 2019-02-08 7:38 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, linuxppc-dev
In-Reply-To: <20190208050747.GC2688@umbus.fritz.box>
>> With the dual mode, the interrupt mode
>> is negotiated at CAS time and when merged, the KVM device will be created
>> at reset. In case of failure, QEMU will abort.
>>
>> I am not saying it is not possible but we will need some internal
>> infrastructure to handle dynamically the fall back to userspace
>> emulation.
>
> Uh.. we do? I think in all cases we need to make the XICS vs. XIVE
> decision first (i.e. what we present to the guest), then we should
> decide how to implement it (userspace, KVM accelerated, impossible and
> give up).
I am changing things with the addition of KM support for dual mode but
that might not be the right approach. Let's talk over it when you reach
the end of the QEMU patchset.
I will keep in mind that we should know exactly what KVM supports
before the machine starts. That is : not to abort QEMU if we can not
satisfy the interrupt mode chosen at CAS time. It might be possible
to fallback to XIVE emulated mode, I think that is where the problem
is but I haven't looked at it closely.
C.
^ permalink raw reply
* Re: [PATCH 15/19] KVM: PPC: Book3S HV: add get/set accessors for the source configuration
From: David Gibson @ 2019-02-08 5:15 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <9f03f232-1c47-6e27-6d79-3bcc900fe943@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 7088 bytes --]
On Thu, Feb 07, 2019 at 10:13:48AM +0100, Cédric Le Goater wrote:
> On 2/7/19 3:48 AM, David Gibson wrote:
> > On Wed, Feb 06, 2019 at 08:07:36AM +0100, Cédric Le Goater wrote:
> >> On 2/6/19 2:24 AM, David Gibson wrote:
> >>> On Wed, Feb 06, 2019 at 12:23:29PM +1100, David Gibson wrote:
> >>>> On Tue, Feb 05, 2019 at 02:03:11PM +0100, Cédric Le Goater wrote:
> >>>>> On 2/5/19 6:32 AM, David Gibson wrote:
> >>>>>> On Mon, Feb 04, 2019 at 05:07:28PM +0100, Cédric Le Goater wrote:
> >>>>>>> On 2/4/19 6:21 AM, David Gibson wrote:
> >>>>>>>> On Mon, Jan 07, 2019 at 07:43:27PM +0100, Cédric Le Goater wrote:
> >>>>>>>>> Theses are use to capure the XIVE EAS table of the KVM device, the
> >>>>>>>>> configuration of the source targets.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>>>>>>> ---
> >>>>>>>>> arch/powerpc/include/uapi/asm/kvm.h | 11 ++++
> >>>>>>>>> arch/powerpc/kvm/book3s_xive_native.c | 87 +++++++++++++++++++++++++++
> >>>>>>>>> 2 files changed, 98 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >>>>>>>>> index 1a8740629acf..faf024f39858 100644
> >>>>>>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >>>>>>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >>>>>>>>> @@ -683,9 +683,20 @@ struct kvm_ppc_cpu_char {
> >>>>>>>>> #define KVM_DEV_XIVE_SAVE_EQ_PAGES 4
> >>>>>>>>> #define KVM_DEV_XIVE_GRP_SOURCES 2 /* 64-bit source attributes */
> >>>>>>>>> #define KVM_DEV_XIVE_GRP_SYNC 3 /* 64-bit source attributes */
> >>>>>>>>> +#define KVM_DEV_XIVE_GRP_EAS 4 /* 64-bit eas attributes */
> >>>>>>>>>
> >>>>>>>>> /* Layout of 64-bit XIVE source attribute values */
> >>>>>>>>> #define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
> >>>>>>>>> #define KVM_XIVE_LEVEL_ASSERTED (1ULL << 1)
> >>>>>>>>>
> >>>>>>>>> +/* Layout of 64-bit eas attribute values */
> >>>>>>>>> +#define KVM_XIVE_EAS_PRIORITY_SHIFT 0
> >>>>>>>>> +#define KVM_XIVE_EAS_PRIORITY_MASK 0x7
> >>>>>>>>> +#define KVM_XIVE_EAS_SERVER_SHIFT 3
> >>>>>>>>> +#define KVM_XIVE_EAS_SERVER_MASK 0xfffffff8ULL
> >>>>>>>>> +#define KVM_XIVE_EAS_MASK_SHIFT 32
> >>>>>>>>> +#define KVM_XIVE_EAS_MASK_MASK 0x100000000ULL
> >>>>>>>>> +#define KVM_XIVE_EAS_EISN_SHIFT 33
> >>>>>>>>> +#define KVM_XIVE_EAS_EISN_MASK 0xfffffffe00000000ULL
> >>>>>>>>> +
> >>>>>>>>> #endif /* __LINUX_KVM_POWERPC_H */
> >>>>>>>>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> >>>>>>>>> index f2de1bcf3b35..0468b605baa7 100644
> >>>>>>>>> --- a/arch/powerpc/kvm/book3s_xive_native.c
> >>>>>>>>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> >>>>>>>>> @@ -525,6 +525,88 @@ static int kvmppc_xive_native_sync(struct kvmppc_xive *xive, long irq, u64 addr)
> >>>>>>>>> return 0;
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> +static int kvmppc_xive_native_set_eas(struct kvmppc_xive *xive, long irq,
> >>>>>>>>> + u64 addr)
> >>>>>>>>
> >>>>>>>> I'd prefer to avoid the name "EAS" here. IIUC these aren't "raw" EAS
> >>>>>>>> values, but rather essentially the "source config" in the terminology
> >>>>>>>> of the PAPR hcalls. Which, yes, is basically implemented by setting
> >>>>>>>> the EAS, but since it's the PAPR architected state that we need to
> >>>>>>>> preserve across migration, I'd prefer to stick as close as we can to
> >>>>>>>> the PAPR terminology.
> >>>>>>>
> >>>>>>> But we don't have an equivalent name in the PAPR specs for the tuple
> >>>>>>> (prio, server). We could use the generic 'target' name may be ? even
> >>>>>>> if this is usually referring to a CPU number.
> >>>>>>
> >>>>>> Um.. what? That's about terminology for one of the fields in this
> >>>>>> thing, not about the name for the thing itself.
> >>>>>>
> >>>>>>> Or, IVE (Interrupt Vector Entry) ? which makes some sense.
> >>>>>>> This is was the former name in HW. I think we recycle it for KVM.
> >>>>>>
> >>>>>> That's a terrible idea, which will make a confusing situation even
> >>>>>> more confusing.
> >>>>>
> >>>>> Let's use SOURCE_CONFIG and QUEUE_CONFIG. The KVM ioctls are very
> >>>>> similar to the hcalls anyhow.
> >>>>
> >>>> Yes, I think that's a good idea.
> >>>
> >>> Actually... AIUI the SET_CONFIG hcalls shouldn't be a fast path.
> >>
> >> No indeed. I have move them to standard hcalls in the current version.
> >>
> >>> Can
> >>> we simplify things further by removing the hcall implementation from
> >>> the kernel entirely, and have qemu implement them by basically just
> >>> forwarding them to the appropriate SET_CONFIG ioctl()?
> >>
> >> Yes. I think we could.
> >
> > Great!
> >
> >> The hcalls H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG and
> >> the KVM ioctls to set the EQ and the SOURCE configuration have a
> >> lot in common. I need to look at how we can plug the KVM ioctl in
> >> the hcalls under QEMU.
> >>
> >> We will have to convert the returned error to respect the PAPR
> >> specs or have the ioctls return H_* errors.
> >
> > I don't think returning H_* values from a kernel call is a good idea.
> > Converting errors is kinda ugly, but I still think it's the better
> > option. Note that we already have something like this for the HPT
> > resizing hcalls.
>
> ok.
>
> >> Let's dig that idea. If we choose that path, QEMU will have an
> >> up-to-date EAT and so we won't need to synchronize its state anymore
> >> for migration.
> >
> > I guess so, though I don't see that as essential.
> >
> >> H_INT_GET_SOURCE_CONFIG can be implemented in QEMU without any KVM
> >> ioctl.
> >>
> >> H_INT_GET_QUEUE_INFO could be implemented in QEMU. I need to check
> >> how we return the address of the END ESB in sPAPR. We haven't paid
> >> much attention to these pages because they are not used under Linux
> >> and today the address is returned by OPAL.
> >>
> >> H_INT_GET_QUEUE_CONFIG is a little more problematic because we need
> >> to query into the XIVE HW the EQ index and toggle bit. OPAL support
> >> is required for that. But we could reduce the KVM support to the
> >> ioctl querying these EQ information.
> >
> > Right, and we'd need an ioctl() like that for migration anyway, yes?
>
> Yes. it is the same need.
>
> >> H_INT_ESB could be entirely done under QEMU.
> >
> > This one can actually happen on fairly hot paths, so I think doing
> > that in qemu probably isn't a good idea.
>
> I agree It would nice to have some performance.
>
> This hcall is used when LSIs are involved, which is not really a common
> configuration. There are no OPAL calls involved. And we are duplicating
> code at the KVM level to retrigger the interrupt when the level is still
> asserted.
>
> I will benchmark the two options before making a choice.
Ok.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 06/19] KVM: PPC: Book3S HV: add a GET_ESB_FD control to the XIVE native device
From: David Gibson @ 2019-02-08 5:15 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <d20bbaca-5b49-66e4-9d99-21d991c2c345@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 6973 bytes --]
On Thu, Feb 07, 2019 at 10:03:15AM +0100, Cédric Le Goater wrote:
> On 2/7/19 3:49 AM, David Gibson wrote:
> > On Wed, Feb 06, 2019 at 08:21:10AM +0100, Cédric Le Goater wrote:
> >> On 2/6/19 2:23 AM, David Gibson wrote:
> >>> On Tue, Feb 05, 2019 at 01:55:40PM +0100, Cédric Le Goater wrote:
> >>>> On 2/5/19 6:28 AM, David Gibson wrote:
> >>>>> On Mon, Feb 04, 2019 at 12:30:39PM +0100, Cédric Le Goater wrote:
> >>>>>> On 2/4/19 5:45 AM, David Gibson wrote:
> >>>>>>> On Mon, Jan 07, 2019 at 07:43:18PM +0100, Cédric Le Goater wrote:
> >>>>>>>> This will let the guest create a memory mapping to expose the ESB MMIO
> >>>>>>>> regions used to control the interrupt sources, to trigger events, to
> >>>>>>>> EOI or to turn off the sources.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>>>>>> ---
> >>>>>>>> arch/powerpc/include/uapi/asm/kvm.h | 4 ++
> >>>>>>>> arch/powerpc/kvm/book3s_xive_native.c | 97 +++++++++++++++++++++++++++
> >>>>>>>> 2 files changed, 101 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >>>>>>>> index 8c876c166ef2..6bb61ba141c2 100644
> >>>>>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >>>>>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >>>>>>>> @@ -675,4 +675,8 @@ struct kvm_ppc_cpu_char {
> >>>>>>>> #define KVM_XICS_PRESENTED (1ULL << 43)
> >>>>>>>> #define KVM_XICS_QUEUED (1ULL << 44)
> >>>>>>>>
> >>>>>>>> +/* POWER9 XIVE Native Interrupt Controller */
> >>>>>>>> +#define KVM_DEV_XIVE_GRP_CTRL 1
> >>>>>>>> +#define KVM_DEV_XIVE_GET_ESB_FD 1
> >>>>>>>
> >>>>>>> Introducing a new FD for ESB and TIMA seems overkill. Can't you get
> >>>>>>> to both with an mmap() directly on the xive device fd? Using the
> >>>>>>> offset to distinguish which one to map, obviously.
> >>>>>>
> >>>>>> The page offset would define some sort of user API. It seems feasible.
> >>>>>> But I am not sure this would be practical in the future if we need to
> >>>>>> tune the length.
> >>>>>
> >>>>> Um.. why not? I mean, yes the XIVE supports rather a lot of
> >>>>> interrupts, but we have 64-bits of offset we can play with - we can
> >>>>> leave room for billions of ESB slots and still have room for billions
> >>>>> of VPs.
> >>>>
> >>>> So the first 4 pages could be the TIMA pages and then would come
> >>>> the pages for the interrupt ESBs. I think that we can have different
> >>>> vm_fault handler for each mapping.
> >>>
> >>> Um.. no, I'm saying you don't need to tightly pack them. So you could
> >>> have the ESB pages at 0, the TIMA at, say offset 2^60.
> >>
> >> Well, we know that the TIMA is 4 pages wide and is "directly" related
> >> with the KVM interrupt device. So being at offset 0 seems a good idea.
> >> While the ESB segment is of a variable size depending on the number
> >> of IRQs and it can come after I think.
> >>
> >>>> I wonder how this will work out with pass-through. As Paul said in
> >>>> a previous email, it would be better to let QEMU request a new
> >>>> mapping to handle the ESB pages of the device being passed through.
> >>>> I guess this is not a special case, just another offset and length.
> >>>
> >>> Right, if we need multiple "chunks" of ESB pages we can given them
> >>> each their own terabyte or several. No need to be stingy with address
> >>> space.
> >>
> >> You can not put them anywhere. They should map the same interrupt range
> >> of ESB pages, overlapping with the underlying segment of IPI ESB pages.
> >
> > I don't really follow what you're saying here.
>
>
> What we want the guest to access in terms of ESB pages is something like
> below, VMA0 being the initial mapping done by QEMU at offset 0x0, the IPI
> ESB pages being populated on the demand with the loads and the stores from
> the guest :
>
>
> 0x0 0x1100 0x1200 0x1300
>
> ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
>
> +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
> VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
> pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
>
>
>
> A device is passed through and the driver requests MSIs.
>
> We now want the guest to access the HW ESB pages for the requested IRQs
> but still the initial IPI ESB pages for the others. Something like below :
>
>
> 0x0 0x1100 0x1200 0x1300
>
> ranges | CPU IPIs .. | VIO | PCI LSI | MSIs
>
> +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
> VMA0 IPI ESB | | | | | | | | | | | | | | | | | | | | | | | | |
> pages +-+-+-+-+-+-+-...-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- ....
>
> VMA1 PHB ESB +-------+
> pages | | | | |
> +-------+
Right, except of course VMA0 will be split into two pieces by
performing the mmap() over it.
> The VMA1 is the result of a new mmap() being done at an offset depending on
> the first IRQ number requested by the driver.
Right... that's one way we could do it. But the irq numbers are all
dynamically allocated here, so could we instead just put the
passthrough MSIs in a separate range? We'd still need a separate
mmap() for them, but we wouldn't have to deal with mapping over and
unmapping if the device is removed or whatever.
> This is because the vm_fault handler uses the page offset to find the
> associated KVM IRQ struct containing the addresses of the EOI and trigger
> pages in the underlying hardware, which will be the PHB in case of a
> passthrough device.
>
> >From there, the VMA1 mmap() pointer will be used to create a 'ram device'
> memory region which will be mapped on top of the initial ESB memory region
> in QEMU. This will override the initial IPI ESB pages with the PHB ESB pages
> in the guest ESB address space.
Um.. what? If that qemu memory range is already mapped into the guest
we don't need to create new RAM devices or anything for the
overmapping. If we overmap in qemu that will just get carried into
the guest.
> That's the plan I have in mind as suggested by Paul if I understood it well.
> The mechanics are more complex than the patch zapping the PTEs from the VMA
> but it's also safer.
Well, yes, where "safer" means "has the possibility to be correct".
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] powerpc/64: Fix memcmp reading past the end of src/dest
From: Michael Ellerman @ 2019-02-08 6:12 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, chandan, npiggin, daniel
In-Reply-To: <20190207125256.GW14180@gate.crashing.org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Feb 07, 2019 at 10:53:13PM +1100, Michael Ellerman wrote:
>> Chandan reported that fstests' generic/026 test hit a crash:
>
>> The instruction dump decodes as:
>> subfic r6,r5,8
>> rlwinm r6,r6,3,0,28
>> ldbrx r9,0,r3
>> ldbrx r10,0,r4 <-
>>
>> Which shows us doing an 8 byte load from c00000062ac3fff9, which
>> crosses the page boundary at c00000062ac40000 and faults.
>>
>> It's not OK for memcmp to read past the end of the source or
>> destination buffers.
>
> It's not okay to access memory pages unsolicited. Reading past the end
> is fine per se.
Yeah I guess that's true.
Things like KASAN/valgrind probably disagree, but KASAN at least
overrides memcmp AIUI.
I guess I feel better about it not reading past the end of the buffers,
but maybe I'm being paranoid.
The other complication is we support multiple page sizes, so detecting a
page boundary is more complicated than it could be.
So I guess I'm inclined to stick with this approach, but I can update
the change log.
cheers
^ permalink raw reply
* Re: [PATCH v03] powerpc/numa: Perform full re-add of CPU for PRRN/VPHN topology update
From: Srikar Dronamraju @ 2019-02-08 5:44 UTC (permalink / raw)
To: Michael Bringmann
Cc: Rob Herring, Kees Cook, Nicholas Piggin, linux-kernel, Al Viro,
Juliet Kim, Oliver O'Halloran, Tyrel Datwyler, Nathan Lynch,
Thomas Falcon, linuxppc-dev, Guenter Roeck, Corentin Labbe
In-Reply-To: <305ed693-ea85-8a70-1d3c-ae405aebc0ad@linux.vnet.ibm.com>
>
> int arch_update_cpu_topology(void)
> {
> - return numa_update_cpu_topology(true);
> + int changed = topology_changed;
> +
> + topology_changed = 0;
> + return changed;
> }
>
Do we need Powerpc override for arch_update_cpu_topology() now? That
topology_changed sometime back doesn't seem to have help. The scheduler
atleast now is neglecting whether the topology changed or not.
Also we can do away with the new topology_changed.
> static void topology_work_fn(struct work_struct *work)
> {
> - rebuild_sched_domains();
> + lock_device_hotplug();
> + if (numa_update_cpu_topology(true))
> + rebuild_sched_domains();
> + unlock_device_hotplug();
> }
Should this hunk be a separate patch by itself to say why
rebuild_sched_domains with a changelog that explains why it should be under
lock_device_hotplug? rebuild_sched_domains already takes cpuset_mutex.
So I am not sure if we need to take device_hotplug_lock.
> static DECLARE_WORK(topology_work, topology_work_fn);
>
> -static void topology_schedule_update(void)
> +void topology_schedule_update(void)
> {
> - schedule_work(&topology_work);
> + if (!topology_update_in_progress)
> + schedule_work(&topology_work);
> }
>
> static void topology_timer_fn(struct timer_list *unused)
> {
> + bool sdo = false;
Is sdo any abbrevation?
> +
> + if (topology_scans < 1)
> + bitmap_fill(cpumask_bits(&cpu_associativity_changes_mask),
> + nr_cpumask_bits);
Why do we need topology_scan? Just to make sure
cpu_associativity_changes_mask is populated only once?
cant we use a static bool inside the function for the same?
> +
> if (prrn_enabled && cpumask_weight(&cpu_associativity_changes_mask))
> - topology_schedule_update();
> - else if (vphn_enabled) {
> + sdo = true;
> + if (vphn_enabled) {
Any reason to remove the else above?
> if (update_cpu_associativity_changes_mask() > 0)
> - topology_schedule_update();
> + sdo = true;
> reset_topology_timer();
> }
> + if (sdo)
> + topology_schedule_update();
> + topology_scans++;
> }
Are the above two hunks necessary? Not getting how the current changes are
different from the previous.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH 00/19] KVM: PPC: Book3S HV: add XIVE native exploitation mode
From: David Gibson @ 2019-02-08 5:07 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, linuxppc-dev
In-Reply-To: <2d623da8-31e4-bdc9-c5e4-ea3689a795dd@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 3139 bytes --]
On Thu, Feb 07, 2019 at 09:31:06AM +0100, Cédric Le Goater wrote:
> On 2/7/19 3:51 AM, David Gibson wrote:
> > On Wed, Feb 06, 2019 at 08:35:24AM +0100, Cédric Le Goater wrote:
> >> On 2/6/19 2:18 AM, David Gibson wrote:
> >>> On Wed, Feb 06, 2019 at 09:13:15AM +1100, Paul Mackerras wrote:
> >>>> On Tue, Feb 05, 2019 at 12:31:28PM +0100, Cédric Le Goater wrote:
> >>>>>>>> As for nesting, I suggest for the foreseeable future we stick to XICS
> >>>>>>>> emulation in nested guests.
> >>>>>>>
> >>>>>>> ok. so no kernel_irqchip at all. hmm.
> >>>>>
> >>>>> I was confused with what Paul calls 'XICS emulation'. It's not the QEMU
> >>>>> XICS emulated device but the XICS-over-XIVE KVM device, the KVM XICS
> >>>>> device KVM uses when under a P9 processor.
> >>>>
> >>>> Actually there are two separate implementations of XICS emulation in
> >>>> KVM. The first (older) one is almost entirely a software emulation
> >>>> but does have some cases where it accesses an underlying XICS device
> >>>> in order to make some things faster (IPIs and pass-through of a device
> >>>> interrupt to a guest). The other, newer one is the XICS-on-XIVE
> >>>> emulation that Ben wrote, which uses the XIVE hardware pretty heavily.
> >>>> My patch was about making the the older code work when there is no
> >>>> XICS available to the host.
> >>>
> >>> Ah, right. To clarify my earlier statements in light of this:
> >>>
> >>> * We definitely want some sort of kernel-XICS available in a nested
> >>> guest. AIUI, this is now accomplished, so, Yay!
> >>>
> >>> * Implementing the L2 XICS in terms of L1's PAPR-XIVE would be a
> >>> bonus, but it's a much lower priority.
> >>
> >> Yes. In this case, the L1 KVM-HV should not advertise KVM_CAP_PPC_IRQ_XIVE
> >> to QEMU which will restrict CAS to the XICS only interrupt mode.
> >
> > Uh... no... we shouldn't change what's available to the guest based on
> > host configuration only. We should just stop advertising the CAP
> > saying that *KVM implemented* is available
>
> yes. that is what I meant.
>
> > so that qemu will fall back to userspace XIVE emulation.
>
> even if kernel_irqchip is required ?
Well, no, but if we don't specify.
> Today, QEMU just fails to start.
If we specify kernel_irqchip=on but the kernel can't support that I
think that's the right thing to do.
> With the dual mode, the interrupt mode
> is negotiated at CAS time and when merged, the KVM device will be created
> at reset. In case of failure, QEMU will abort.
>
> I am not saying it is not possible but we will need some internal
> infrastructure to handle dynamically the fall back to userspace
> emulation.
Uh.. we do? I think in all cases we need to make the XICS vs. XIVE
decision first (i.e. what we present to the guest), then we should
decide how to implement it (userspace, KVM accelerated, impossible and
give up).
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/1] Start conversion of PowerPC docs
From: Michael Ellerman @ 2019-02-08 3:40 UTC (permalink / raw)
To: Jonathan Corbet, Tobin C. Harding; +Cc: linuxppc-dev, linux-kernel, linux-doc
In-Reply-To: <20190207170131.1fd2bb03@lwn.net>
Jonathan Corbet <corbet@lwn.net> writes:
> On Thu, 7 Feb 2019 17:03:15 +1100
> "Tobin C. Harding" <tobin@kernel.org> wrote:
>
>> As discussed at LCA here is the start to the docs conversion for PowerPC
>> to RST.
>>
>> This applies cleanly on top of the mainline (5.20-rc5) and Jon's tree
>> (docs-next branch).
>>
>> I'm guessing it should go in through the PowerPC tree because I doubt
>> you want to review this Jon, it's one big single patch (all blame for
>> that falls on mpe ;)
>
> Well, I went and took a look anyway, being a glutton for punishment. So
> naturally I do have some comments...
>
> - I don't think this should be a top-level directory full of docs; the top
> level is already rather overpopulated. At worst, we should create an
> arch/ directory for architecture-specific docs.
We currently have arch specific directories for arm, arm64, ia64, m68k,
nios2, openrisc, parisc, powerpc, s390, sh, sparc, x86, xtensa.
Do you mean they should all be moved to Documentation/arch ?
> I kind of think that
> this should be thought through a bit more, though, with an eye toward
> who the audience is. Some of it is clearly developer documentation, and
> some of it is aimed at admins; ptrace.rst is user-space API stuff.
> Nobody ever welcomes me saying this, but we should really split things
> into the appropriate manuals according to audience.
I don't think any of it's aimed at admins, but I haven't read every
word. I see it as aimed at kernel devs or people writing directly to the
kernel API, eg. gdb developers reading ptrace.rst.
If Documentation/ wants to be more user focused and nicely curated
perhaps we need arch/foo/docs/ for these developer centric docs?
> - It would be good to know how much of this stuff is still relevant.
> bootwrapper.txt hasn't been modified since it was added in 2008.
It hasn't been modified but AFAIK it's still pretty much accurate and
definitely something we want to have documented.
> cpu_features.txt predates the git era
But so does the code it's documenting.
> as does mpc52xx.txt; hvcs.txt is nearly as old. And so on. Can we
> perhaps stop dragging some of those docs around?
We support some hardware that is ~25 years old, so we have some old
documentation too, and I'd rather we didn't drop things just because
they're old.
> - The use of flat-table in isa-versions.rst totally wrecks the readability
> of those tables in the plain-text version. Said tables are pretty close
> to being RST in their original form; it would be far better to just fix
> anything needing fixing but to keep that form.
Yes agree, I'd like the docs to be as readable as possible as plain text.
> - I'm glad you're adding SPDX lines, but do you know that the license is
> correct in each case? It's best to be careful with such things.
None of the files have licenses so I think we just fall back to COPYING
don't we? In which case GPL-2.0 is correct for all files.
cheers
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
From: Andrew Donnellan @ 2019-02-08 3:11 UTC (permalink / raw)
To: Michael Ellerman, Segher Boessenkool, Nicholas Piggin
Cc: syzkaller, linuxppc-dev, dvyukov
In-Reply-To: <87h8dfj83j.fsf@concordia.ellerman.id.au>
On 8/2/19 2:02 pm, Michael Ellerman wrote:> I'd prefer a minimal fix
that we can backport. How about:
>
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 8be3721d9302..a1acccd25839 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
> }
> }
>
> - if (!known && enable_unknown) {
> - if (!feat_try_enable_unknown(f)) {
> - pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
> - f->name);
> - return false;
> - }
> + if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
> + pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
> + f->name);
> + return false;
> }
>
> if (m->cpu_ftr_bit_mask)
>
Sure, I can send a v2 with this fix and your signoff?
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* [PATCH 7/7] powerpc/eeh: Add eeh_force_recover to debugfs
From: Oliver O'Halloran @ 2019-02-08 3:08 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-1-oohall@gmail.com>
This patch adds a debugfs interface to force scheduling a recovery event.
This can be used to recover a specific PE or schedule a "special" recovery
even that checks for errors at the PHB level.
To force a recovery of a normal PE, use:
echo '<#pe>:<#phb>' > /sys/kernel/debug/powerpc/eeh_force_recover
To force a scan broken PHBs:
echo 'null' > /sys/kernel/debug/powerpc/eeh_force_recover
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh_event.h | 1 +
arch/powerpc/kernel/eeh.c | 60 ++++++++++++++++++++++++++++
arch/powerpc/kernel/eeh_event.c | 25 +++++++-----
3 files changed, 76 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh_event.h b/arch/powerpc/include/asm/eeh_event.h
index 9884e872686f..6d0412b846ac 100644
--- a/arch/powerpc/include/asm/eeh_event.h
+++ b/arch/powerpc/include/asm/eeh_event.h
@@ -33,6 +33,7 @@ struct eeh_event {
int eeh_event_init(void);
int eeh_send_failure_event(struct eeh_pe *pe);
+int __eeh_send_failure_event(struct eeh_pe *pe);
void eeh_remove_event(struct eeh_pe *pe, bool force);
void eeh_handle_normal_event(struct eeh_pe *pe);
void eeh_handle_special_event(void);
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 92809b137e39..63b91a4918c9 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1805,6 +1805,63 @@ static int eeh_enable_dbgfs_get(void *data, u64 *val)
DEFINE_DEBUGFS_ATTRIBUTE(eeh_enable_dbgfs_ops, eeh_enable_dbgfs_get,
eeh_enable_dbgfs_set, "0x%llx\n");
+
+static ssize_t eeh_force_recover_write(struct file *filp,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct pci_controller *hose;
+ uint32_t phbid, pe_no;
+ struct eeh_pe *pe;
+ char buf[20];
+ int ret;
+
+ ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
+ if (!ret)
+ return -EFAULT;
+
+ /*
+ * When PE is NULL the event is a "special" event. Rather than
+ * recovering a specific PE it forces the EEH core to scan for failed
+ * PHBs and recovers each. This needs to be done before any device
+ * recoveries can occur.
+ */
+ if (!strncmp(buf, "null", 4)) {
+ pr_err("sending failure event\n");
+ __eeh_send_failure_event(NULL);
+ return count;
+ }
+
+ ret = sscanf(buf, "%x:%x", &phbid, &pe_no);
+ if (ret != 2)
+ return -EINVAL;
+
+ hose = pci_find_hose_for_domain(phbid);
+ if (!hose)
+ return -ENODEV;
+
+ /* Retrieve PE */
+ pe = eeh_pe_get(hose, pe_no, 0);
+ if (!pe)
+ return -ENODEV;
+
+ /*
+ * We don't do any state checking here since the detection
+ * process is async to the recovery process. The recovery
+ * thread *should* not break even if we schedule a recovery
+ * from an odd state (e.g. PE removed, or recovery of a
+ * non-isolated PE)
+ */
+ __eeh_send_failure_event(pe);
+
+ return ret < 0 ? ret : count;
+}
+
+static const struct file_operations eeh_force_recover_fops = {
+ .open = simple_open,
+ .llseek = no_llseek,
+ .write = eeh_force_recover_write,
+};
#endif
static int __init eeh_init_proc(void)
@@ -1820,6 +1877,9 @@ static int __init eeh_init_proc(void)
debugfs_create_bool("eeh_disable_recovery", 0600,
powerpc_debugfs_root,
&eeh_debugfs_no_recover);
+ debugfs_create_file_unsafe("eeh_force_recover", 0600,
+ powerpc_debugfs_root, NULL,
+ &eeh_force_recover_fops);
eeh_cache_debugfs_init();
#endif
}
diff --git a/arch/powerpc/kernel/eeh_event.c b/arch/powerpc/kernel/eeh_event.c
index 19837798bb1d..539aca055d70 100644
--- a/arch/powerpc/kernel/eeh_event.c
+++ b/arch/powerpc/kernel/eeh_event.c
@@ -121,20 +121,11 @@ int eeh_event_init(void)
* the actual event will be delivered in a normal context
* (from a workqueue).
*/
-int eeh_send_failure_event(struct eeh_pe *pe)
+int __eeh_send_failure_event(struct eeh_pe *pe)
{
unsigned long flags;
struct eeh_event *event;
- /*
- * If we've manually supressed recovery events via debugfs
- * then just drop it on the floor.
- */
- if (eeh_debugfs_no_recover) {
- pr_err("EEH: Event dropped due to no_recover setting\n");
- return 0;
- }
-
event = kzalloc(sizeof(*event), GFP_ATOMIC);
if (!event) {
pr_err("EEH: out of memory, event not handled\n");
@@ -153,6 +144,20 @@ int eeh_send_failure_event(struct eeh_pe *pe)
return 0;
}
+int eeh_send_failure_event(struct eeh_pe *pe)
+{
+ /*
+ * If we've manually supressed recovery events via debugfs
+ * then just drop it on the floor.
+ */
+ if (eeh_debugfs_no_recover) {
+ pr_err("EEH: Event dropped due to no_recover setting\n");
+ return 0;
+ }
+
+ return __eeh_send_failure_event(pe);
+}
+
/**
* eeh_remove_event - Remove EEH event from the queue
* @pe: Event binding to the PE
--
2.20.1
^ permalink raw reply related
* [PATCH 6/7] powerpc/eeh: Allow disabling recovery
From: Oliver O'Halloran @ 2019-02-08 3:08 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-1-oohall@gmail.com>
Currently when we detect an error we automatically invoke the EEH recovery
handler. This can be annoying when debugging EEH problems, or when working
on EEH itself so this patch adds a debugfs knob that will prevent a
recovery event from being queued up when an issue is detected.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 1 +
arch/powerpc/kernel/eeh.c | 11 +++++++++++
arch/powerpc/kernel/eeh_event.c | 9 +++++++++
3 files changed, 21 insertions(+)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index fc21b6e78e91..6f6721561302 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -220,6 +220,7 @@ struct eeh_ops {
extern int eeh_subsystem_flags;
extern uint32_t eeh_max_freezes;
+extern bool eeh_debugfs_no_recover;
extern struct eeh_ops *eeh_ops;
extern raw_spinlock_t confirm_error_lock;
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index d1f0bdf41fac..92809b137e39 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -111,6 +111,13 @@ EXPORT_SYMBOL(eeh_subsystem_flags);
*/
uint32_t eeh_max_freezes = 5;
+/*
+ * Controls whether a recovery event should be scheduled when an
+ * isolated device is discovered. This is only really useful for
+ * debugging problems with the EEH core.
+ */
+bool eeh_debugfs_no_recover;
+
/* Platform dependent EEH operations */
struct eeh_ops *eeh_ops = NULL;
@@ -1810,7 +1817,11 @@ static int __init eeh_init_proc(void)
&eeh_enable_dbgfs_ops);
debugfs_create_u32("eeh_max_freezes", 0600,
powerpc_debugfs_root, &eeh_max_freezes);
+ debugfs_create_bool("eeh_disable_recovery", 0600,
+ powerpc_debugfs_root,
+ &eeh_debugfs_no_recover);
eeh_cache_debugfs_init();
+#endif
}
return 0;
diff --git a/arch/powerpc/kernel/eeh_event.c b/arch/powerpc/kernel/eeh_event.c
index 227e57f980df..19837798bb1d 100644
--- a/arch/powerpc/kernel/eeh_event.c
+++ b/arch/powerpc/kernel/eeh_event.c
@@ -126,6 +126,15 @@ int eeh_send_failure_event(struct eeh_pe *pe)
unsigned long flags;
struct eeh_event *event;
+ /*
+ * If we've manually supressed recovery events via debugfs
+ * then just drop it on the floor.
+ */
+ if (eeh_debugfs_no_recover) {
+ pr_err("EEH: Event dropped due to no_recover setting\n");
+ return 0;
+ }
+
event = kzalloc(sizeof(*event), GFP_ATOMIC);
if (!event) {
pr_err("EEH: out of memory, event not handled\n");
--
2.20.1
^ permalink raw reply related
* [PATCH 5/7] powerpc/pci: Add pci_find_hose_for_domain()
From: Oliver O'Halloran @ 2019-02-08 3:08 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-1-oohall@gmail.com>
Add a helper to find the pci_controller structure based on the domain
number / phb id.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/pci-bridge.h | 2 ++
arch/powerpc/kernel/pci-common.c | 11 +++++++++++
2 files changed, 13 insertions(+)
diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index aee4fcc24990..149053b7f481 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -274,6 +274,8 @@ extern int pcibios_map_io_space(struct pci_bus *bus);
extern struct pci_controller *pci_find_hose_for_OF_device(
struct device_node* node);
+extern struct pci_controller *pci_find_hose_for_domain(uint32_t domain_nr);
+
/* Fill up host controller resources from the OF node */
extern void pci_process_bridge_OF_ranges(struct pci_controller *hose,
struct device_node *dev, int primary);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 88e4f69a09e5..958f38c698da 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -357,6 +357,17 @@ struct pci_controller* pci_find_hose_for_OF_device(struct device_node* node)
return NULL;
}
+struct pci_controller *pci_find_hose_for_domain(uint32_t domain_nr)
+{
+ struct pci_controller *hose;
+
+ list_for_each_entry(hose, &hose_list, list_node)
+ if (hose->global_number == domain_nr)
+ return hose;
+
+ return NULL;
+}
+
/*
* Reads the interrupt pin to determine if interrupt is use by card.
* If the interrupt is used, then gets the interrupt line from the
--
2.20.1
^ permalink raw reply related
* [PATCH 4/7] powerpc/eeh_cache: Bump log level of eeh_addr_cache_print()
From: Oliver O'Halloran @ 2019-02-08 3:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-1-oohall@gmail.com>
To use this function at all #define DEBUG needs to be set in eeh_cache.c.
Considering that printing at pr_debug is probably not all that useful since
it adds the additional hurdle of requiring you to enable the debug print if
dynamic_debug is in use so this patch bumps it to pr_info.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/kernel/eeh_cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
index dba421a577e7..7de278f88e3d 100644
--- a/arch/powerpc/kernel/eeh_cache.c
+++ b/arch/powerpc/kernel/eeh_cache.c
@@ -114,7 +114,7 @@ static void eeh_addr_cache_print(struct pci_io_addr_cache *cache)
while (n) {
struct pci_io_addr_range *piar;
piar = rb_entry(n, struct pci_io_addr_range, rb_node);
- pr_debug("PCI: %s addr range %d [%pap-%pap]: %s\n",
+ pr_info("PCI: %s addr range %d [%pap-%pap]: %s\n",
(piar->flags & IORESOURCE_IO) ? "i/o" : "mem", cnt,
&piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev));
cnt++;
--
2.20.1
^ permalink raw reply related
* [PATCH 3/7] powerpc/eeh_cache: Add a way to dump the EEH address cache
From: Oliver O'Halloran @ 2019-02-08 3:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-1-oohall@gmail.com>
Adds a debugfs file that can be read to view the contents of the EEH
address cache. This is pretty similar to the existing
eeh_addr_cache_print() function, but that function is intended to debug
issues inside of the kernel since it's #ifdef`ed out by default, and writes
into the kernel log.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 3 +++
arch/powerpc/kernel/eeh.c | 2 +-
arch/powerpc/kernel/eeh_cache.c | 34 +++++++++++++++++++++++++++++----
3 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index c003628441cc..fc21b6e78e91 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -460,6 +460,9 @@ static inline void eeh_readsl(const volatile void __iomem *addr, void * buf,
eeh_check_failure(addr);
}
+
+void eeh_cache_debugfs_init(void);
+
#endif /* CONFIG_PPC64 */
#endif /* __KERNEL__ */
#endif /* _POWERPC_EEH_H */
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index f6e65375a8de..d1f0bdf41fac 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1810,7 +1810,7 @@ static int __init eeh_init_proc(void)
&eeh_enable_dbgfs_ops);
debugfs_create_u32("eeh_max_freezes", 0600,
powerpc_debugfs_root, &eeh_max_freezes);
-#endif
+ eeh_cache_debugfs_init();
}
return 0;
diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
index b2c320e0fcef..dba421a577e7 100644
--- a/arch/powerpc/kernel/eeh_cache.c
+++ b/arch/powerpc/kernel/eeh_cache.c
@@ -26,6 +26,7 @@
#include <linux/spinlock.h>
#include <linux/atomic.h>
#include <asm/pci-bridge.h>
+#include <asm/debugfs.h>
#include <asm/ppc-pci.h>
@@ -298,9 +299,34 @@ void eeh_addr_cache_build(void)
eeh_addr_cache_insert_dev(dev);
eeh_sysfs_add_device(dev);
}
+}
-#ifdef DEBUG
- /* Verify tree built up above, echo back the list of addrs. */
- eeh_addr_cache_print(&pci_io_addr_cache_root);
-#endif
+static int eeh_addr_cache_show(struct seq_file *s, void *v)
+{
+ struct rb_node *n = rb_first(&pci_io_addr_cache_root.rb_root);
+ struct pci_io_addr_range *piar;
+ int cnt = 0;
+
+ spin_lock(&pci_io_addr_cache_root.piar_lock);
+ while (n) {
+ piar = rb_entry(n, struct pci_io_addr_range, rb_node);
+
+ seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n",
+ (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", cnt,
+ &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev));
+
+ n = rb_next(n);
+ cnt++;
+ }
+ spin_unlock(&pci_io_addr_cache_root.piar_lock);
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(eeh_addr_cache);
+
+void eeh_cache_debugfs_init(void)
+{
+ debugfs_create_file_unsafe("eeh_address_cache", 0400,
+ powerpc_debugfs_root, NULL,
+ &eeh_addr_cache_fops);
}
--
2.20.1
^ permalink raw reply related
* [PATCH 2/7] powerpc/eeh_cache: Add pr_debug() prints for insert/remove
From: Oliver O'Halloran @ 2019-02-08 3:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20190208030802.10805-1-oohall@gmail.com>
The EEH address cache is used to map a physical MMIO address back to a PCI
device. It's useful to know when it's being manipulated, but currently this
requires recompiling with #define DEBUG set. This is pointless since we
have dynamic_debug nowdays, so remove the #ifdef guard and add a pr_debug()
for the remove case too.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/kernel/eeh_cache.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
index 201943d54a6e..b2c320e0fcef 100644
--- a/arch/powerpc/kernel/eeh_cache.c
+++ b/arch/powerpc/kernel/eeh_cache.c
@@ -157,10 +157,8 @@ eeh_addr_cache_insert(struct pci_dev *dev, resource_size_t alo,
piar->pcidev = dev;
piar->flags = flags;
-#ifdef DEBUG
pr_debug("PIAR: insert range=[%pap:%pap] dev=%s\n",
&alo, &ahi, pci_name(dev));
-#endif
rb_link_node(&piar->rb_node, parent, p);
rb_insert_color(&piar->rb_node, &pci_io_addr_cache_root.rb_root);
@@ -240,6 +238,8 @@ static inline void __eeh_addr_cache_rmv_dev(struct pci_dev *dev)
piar = rb_entry(n, struct pci_io_addr_range, rb_node);
if (piar->pcidev == dev) {
+ pr_debug("PIAR: remove range=[%pap:%pap] dev=%s\n",
+ &piar->addr_lo, &piar->addr_hi, pci_name(dev));
rb_erase(n, &pci_io_addr_cache_root.rb_root);
kfree(piar);
goto restart;
--
2.20.1
^ permalink raw reply related
* [PATCH 1/7] powerpc/eeh: Use debugfs_create_u32 for eeh_max_freezes
From: Oliver O'Halloran @ 2019-02-08 3:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
There's no need to the custom getter/setter functions so we should remove
them in favour of using the generic one. While we're here, change the type
of eeh_max_freeze to uint32_t and print the value in decimal rather than
hex because printing it in hex makes no sense.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 2 +-
arch/powerpc/kernel/eeh.c | 21 +++------------------
2 files changed, 4 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 8b596d096ebe..c003628441cc 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -219,7 +219,7 @@ struct eeh_ops {
};
extern int eeh_subsystem_flags;
-extern int eeh_max_freezes;
+extern uint32_t eeh_max_freezes;
extern struct eeh_ops *eeh_ops;
extern raw_spinlock_t confirm_error_lock;
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index ae05203eb4de..f6e65375a8de 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -109,7 +109,7 @@ EXPORT_SYMBOL(eeh_subsystem_flags);
* frozen count in last hour exceeds this limit, the PE will
* be forced to be offline permanently.
*/
-int eeh_max_freezes = 5;
+uint32_t eeh_max_freezes = 5;
/* Platform dependent EEH operations */
struct eeh_ops *eeh_ops = NULL;
@@ -1796,22 +1796,8 @@ static int eeh_enable_dbgfs_get(void *data, u64 *val)
return 0;
}
-static int eeh_freeze_dbgfs_set(void *data, u64 val)
-{
- eeh_max_freezes = val;
- return 0;
-}
-
-static int eeh_freeze_dbgfs_get(void *data, u64 *val)
-{
- *val = eeh_max_freezes;
- return 0;
-}
-
DEFINE_DEBUGFS_ATTRIBUTE(eeh_enable_dbgfs_ops, eeh_enable_dbgfs_get,
eeh_enable_dbgfs_set, "0x%llx\n");
-DEFINE_DEBUGFS_ATTRIBUTE(eeh_freeze_dbgfs_ops, eeh_freeze_dbgfs_get,
- eeh_freeze_dbgfs_set, "0x%llx\n");
#endif
static int __init eeh_init_proc(void)
@@ -1822,9 +1808,8 @@ static int __init eeh_init_proc(void)
debugfs_create_file_unsafe("eeh_enable", 0600,
powerpc_debugfs_root, NULL,
&eeh_enable_dbgfs_ops);
- debugfs_create_file_unsafe("eeh_max_freezes", 0600,
- powerpc_debugfs_root, NULL,
- &eeh_freeze_dbgfs_ops);
+ debugfs_create_u32("eeh_max_freezes", 0600,
+ powerpc_debugfs_root, &eeh_max_freezes);
#endif
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
From: Michael Ellerman @ 2019-02-08 3:02 UTC (permalink / raw)
To: Andrew Donnellan, Segher Boessenkool, Nicholas Piggin
Cc: syzkaller, linuxppc-dev, dvyukov
In-Reply-To: <e1e1fc85-6ab6-ba91-c91f-21c3f51ee6ee@au1.ibm.com>
Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
> (+ Nick)
>
> On 7/2/19 6:49 pm, Segher Boessenkool wrote:
>> On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:
>>> On 7/2/19 5:37 pm, Segher Boessenkool wrote:
>>>> On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
>>>>> Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
>>>>> cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
>>>>>
>>>>> arch/powerpc/kernel/dt_cpu_ftrs.c: In function
>>>>> ‘cpufeatures_process_feature’:
>>>>> arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used
>>>>> uninitialized in this function [-Wmaybe-uninitialized]
>>>>> if (m->cpu_ftr_bit_mask)
>>>>
>>>> It seems to me the warning is correct? If enable_unknown is false and no
>>>> cpu_feature is found, it will in
>>>>
>>>> if (m->cpu_ftr_bit_mask)
>>>> cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>>
>>>> enable random features (whatever was last in the table), or indeed access
>>>> via NULL if the table is length 0? So maybe this should be
>>>>
>>>> if (known && m->cpu_ftr_bit_mask)
>>>> cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>>
>>>> instead? (The code would be much clearer if all the known vs. !known
>>>> codepath was fully separated here).
>>>
>>> The table is never length 0, it's a statically defined array.
>>
>> Sure, and presumably that is why newer GCC doesn't warn. But what about
>> the other point? Is this code ever correct? Enabling random features
>> (in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.
>
> Now that I'm replying without being 2 minutes before a meeting :)
>
> The warning is still spurious, but the logic looks very suspicious.
>
> I think your solution looks correct, though the whole function could be
> cleaned up a bit.
>
> I also notice that if enable_unknown == false, then I think an unknown
> feature will still print "enabling" and return true, which seems wrong.
>
> How does something like the following look, which I could send instead
> and will probably solve the spurious warnings issues anyway?
I'd prefer a minimal fix that we can backport. How about:
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8be3721d9302..a1acccd25839 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
}
}
- if (!known && enable_unknown) {
- if (!feat_try_enable_unknown(f)) {
- pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
- f->name);
- return false;
- }
+ if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
+ pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
+ f->name);
+ return false;
}
if (m->cpu_ftr_bit_mask)
cheers
^ permalink raw reply related
* Re: [PATCH v3 1/2] mm: add probe_user_read()
From: Michael Ellerman @ 2019-02-08 3:01 UTC (permalink / raw)
To: Jann Horn, Christophe Leroy
Cc: Kees Cook, kernel list, Mike Rapoport, Linux-MM, Paul Mackerras,
Andrew Morton, linuxppc-dev
In-Reply-To: <CAG48ez1gXgsBG6bYGG5+B4Dqkhk_iVaYLqt63RaxURxE0yt9eA@mail.gmail.com>
Jann Horn <jannh@google.com> writes:
> On Thu, Feb 7, 2019 at 10:22 AM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>> In powerpc code, there are several places implementing safe
>> access to user data. This is sometimes implemented using
>> probe_kernel_address() with additional access_ok() verification,
>> sometimes with get_user() enclosed in a pagefault_disable()/enable()
>> pair, etc. :
>> show_user_instructions()
>> bad_stack_expansion()
>> p9_hmi_special_emu()
>> fsl_pci_mcheck_exception()
>> read_user_stack_64()
>> read_user_stack_32() on PPC64
>> read_user_stack_32() on PPC32
>> power_pmu_bhrb_to()
>>
>> In the same spirit as probe_kernel_read(), this patch adds
>> probe_user_read().
>>
>> probe_user_read() does the same as probe_kernel_read() but
>> first checks that it is really a user address.
>>
>> The patch defines this function as a static inline so the "size"
>> variable can be examined for const-ness by the check_object_size()
>> in __copy_from_user_inatomic()
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
>
>
>> ---
>> v3: Moved 'Returns:" comment after description.
>> Explained in the commit log why the function is defined static inline
>>
>> v2: Added "Returns:" comment and removed probe_user_address()
>>
>> include/linux/uaccess.h | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
>> index 37b226e8df13..ef99edd63da3 100644
>> --- a/include/linux/uaccess.h
>> +++ b/include/linux/uaccess.h
>> @@ -263,6 +263,40 @@ extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count);
>> #define probe_kernel_address(addr, retval) \
>> probe_kernel_read(&retval, addr, sizeof(retval))
>>
>> +/**
>> + * probe_user_read(): safely attempt to read from a user location
>> + * @dst: pointer to the buffer that shall take the data
>> + * @src: address to read from
>> + * @size: size of the data chunk
>> + *
>> + * Safely read from address @src to the buffer at @dst. If a kernel fault
>> + * happens, handle that and return -EFAULT.
>> + *
>> + * We ensure that the copy_from_user is executed in atomic context so that
>> + * do_page_fault() doesn't attempt to take mmap_sem. This makes
>> + * probe_user_read() suitable for use within regions where the caller
>> + * already holds mmap_sem, or other locks which nest inside mmap_sem.
>> + *
>> + * Returns: 0 on success, -EFAULT on error.
>> + */
>> +
>> +#ifndef probe_user_read
>> +static __always_inline long probe_user_read(void *dst, const void __user *src,
>> + size_t size)
>> +{
>> + long ret;
>> +
>> + if (!access_ok(src, size))
>> + return -EFAULT;
>
> If this happens in code that's running with KERNEL_DS, the access_ok()
> is a no-op. If this helper is only intended for accessing real
> userspace memory, it would be more robust to add
> set_fs(USER_DS)/set_fs(oldfs) around this thing. Looking at the
> functions you're referring to in the commit message, e.g.
> show_user_instructions() does an explicit `__access_ok(pc,
> NR_INSN_TO_PRINT * sizeof(int), USER_DS)` to get the same effect.
Yeah I raised the same question up thread.
I think we're both right :) - it should explicitly set USER_DS.
There's precedent for that in the code you mentioned and also in the
perf code, eg:
88b0193d9418 ("perf/callchain: Force USER_DS when invoking perf_callchain_user()")
cheers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox