* Re: [PATCH v2 06/10] powerpc/smp: Generalize 2nd sched domain
From: peterz @ 2020-07-22 8:54 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Ingo Molnar, Nathan Lynch, Oliver OHalloran, Michael Neuling,
Michael Ellerman, Gautham R Shenoy, Jordan Niethe,
Anton Blanchard, LKML, Valentin Schneider, linuxppc-dev,
Nick Piggin
In-Reply-To: <20200722084854.GL10769@hirez.programming.kicks-ass.net>
On Wed, Jul 22, 2020 at 10:48:54AM +0200, Peter Zijlstra wrote:
> But reading your explanation, it looks like the Linux topology setup
> could actually unravel the fused-faux-SMT8 into two independent SMT4
> parts, negating that firmware option.
Ah, it looks like that's exactly what you're doing. Nice!
^ permalink raw reply
* Re: [v4 1/5] KVM: PPC: Book3S HV: Disable page merging in H_SVM_INIT_START
From: Bharata B Rao @ 2020-07-22 8:52 UTC (permalink / raw)
To: Ram Pai
Cc: ldufour, cclaudio, kvm-ppc, sathnaga, aneesh.kumar, sukadev,
linuxppc-dev, bauerman, david
In-Reply-To: <1594972827-13928-2-git-send-email-linuxram@us.ibm.com>
On Fri, Jul 17, 2020 at 01:00:23AM -0700, Ram Pai wrote:
> Page-merging of pages in memory-slots associated with a Secure VM,
> is disabled in H_SVM_PAGE_IN handler.
>
> This operation should have been done much earlier; the moment the VM
> is initiated for secure-transition. Delaying this operation, increases
> the probability for those pages to acquire new references , making it
> impossible to migrate those pages.
>
> Disable page-migration in H_SVM_INIT_START handling.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Bharata B Rao <bharata@linux.ibm.com>
with a few observations below...
> ---
> Documentation/powerpc/ultravisor.rst | 1 +
> arch/powerpc/kvm/book3s_hv_uvmem.c | 98 +++++++++++++++++++++++++++---------
> 2 files changed, 76 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/powerpc/ultravisor.rst b/Documentation/powerpc/ultravisor.rst
> index df136c8..a1c8c37 100644
> --- a/Documentation/powerpc/ultravisor.rst
> +++ b/Documentation/powerpc/ultravisor.rst
> @@ -895,6 +895,7 @@ Return values
> One of the following values:
>
> * H_SUCCESS on success.
> + * H_STATE if the VM is not in a position to switch to secure.
>
> Description
> ~~~~~~~~~~~
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index e6f76bc..0baa293 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -211,6 +211,65 @@ static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn, struct kvm *kvm,
> return false;
> }
>
> +static int kvmppc_memslot_page_merge(struct kvm *kvm,
> + struct kvm_memory_slot *memslot, bool merge)
> +{
> + unsigned long gfn = memslot->base_gfn;
> + unsigned long end, start = gfn_to_hva(kvm, gfn);
> + int ret = 0;
> + struct vm_area_struct *vma;
> + int merge_flag = (merge) ? MADV_MERGEABLE : MADV_UNMERGEABLE;
> +
> + if (kvm_is_error_hva(start))
> + return H_STATE;
> +
> + end = start + (memslot->npages << PAGE_SHIFT);
> +
> + mmap_write_lock(kvm->mm);
> + do {
> + vma = find_vma_intersection(kvm->mm, start, end);
> + if (!vma) {
> + ret = H_STATE;
> + break;
> + }
> + ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> + merge_flag, &vma->vm_flags);
> + if (ret) {
> + ret = H_STATE;
> + break;
> + }
> + start = vma->vm_end + 1;
This should be start = vma->vm_end I believe.
> + } while (end > vma->vm_end);
> +
> + mmap_write_unlock(kvm->mm);
> + return ret;
> +}
> +
> +static int __kvmppc_page_merge(struct kvm *kvm, bool merge)
> +{
> + struct kvm_memslots *slots;
> + struct kvm_memory_slot *memslot;
> + int ret = 0;
> +
> + slots = kvm_memslots(kvm);
> + kvm_for_each_memslot(memslot, slots) {
> + ret = kvmppc_memslot_page_merge(kvm, memslot, merge);
> + if (ret)
> + break;
> + }
> + return ret;
> +}
You walk through all the slots here to issue kvm_madvise, but...
> +
> +static inline int kvmppc_disable_page_merge(struct kvm *kvm)
> +{
> + return __kvmppc_page_merge(kvm, false);
> +}
> +
> +static inline int kvmppc_enable_page_merge(struct kvm *kvm)
> +{
> + return __kvmppc_page_merge(kvm, true);
> +}
> +
> unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> {
> struct kvm_memslots *slots;
> @@ -232,11 +291,18 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> return H_AUTHORITY;
>
> srcu_idx = srcu_read_lock(&kvm->srcu);
> +
> + /* disable page-merging for all memslot */
> + ret = kvmppc_disable_page_merge(kvm);
> + if (ret)
> + goto out;
> +
> + /* register the memslot */
> slots = kvm_memslots(kvm);
> kvm_for_each_memslot(memslot, slots) {
... you are walking thro' the same set of slots here anyway. I think
it makes sense to issue merge advices from here itself. That will
help you to share code with kvmppc_memslot_create() in 5/5.
All the below 3 calls are common to both the code paths, I think
they can be carved out into a separate function if you prefer.
kvmppc_uvmem_slot_init
kvmppc_memslot_page_merge
uv_register_mem_slot
Regards,
Bharata.
^ permalink raw reply
* Re: [PATCH v2 06/10] powerpc/smp: Generalize 2nd sched domain
From: Peter Zijlstra @ 2020-07-22 8:48 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Ingo Molnar, Nathan Lynch, Oliver OHalloran, Michael Neuling,
Michael Ellerman, Gautham R Shenoy, Jordan Niethe,
Anton Blanchard, LKML, Valentin Schneider, linuxppc-dev,
Nick Piggin
In-Reply-To: <20200722081822.GG9290@linux.vnet.ibm.com>
On Wed, Jul 22, 2020 at 01:48:22PM +0530, Srikar Dronamraju wrote:
> * peterz@infradead.org <peterz@infradead.org> [2020-07-22 09:46:24]:
>
> > On Tue, Jul 21, 2020 at 05:08:10PM +0530, Srikar Dronamraju wrote:
> > > Currently "CACHE" domain happens to be the 2nd sched domain as per
> > > powerpc_topology. This domain will collapse if cpumask of l2-cache is
> > > same as SMT domain. However we could generalize this domain such that it
> > > could mean either be a "CACHE" domain or a "BIGCORE" domain.
> >
> > What's the whole smallcore vs bigcore thing?
> >
> > Would it make sense to have an actual overview of the various topology
> > layers somewhere? Because I'm getting lost and can't really make sense
> > of the code due to that.
>
> To quote with an example: using (Power 9)
>
> Power 9 is an SMT 8 core by design. However this 8 thread core can run as 2
> independent core with threads 0,2,4 and 6 acting like a core, while threads
> 1,3,5,7 acting as another core.
>
> The firmware can decide to showcase them as 2 independent small cores or as
> one big core. However the LLC (i.e last level of cache) is shared between
> all the threads of the SMT 8 core. Future power chips, LLC might change, it
> may be expanded to share with another SMT 8 core or it could be made
> specific to SMT 4 core.
>
> The smt 8 core is also known as fused core/ Big core.
> The smaller smt 4 core is known as small core.
>
> So on a Power9 Baremetal, the firmware would show up as SMT4 core.
> and we have a CACHE level at SMT 8. CACHE level would be very very similar
> to MC domain in X86.
>
> Hope this is clear.
Ooh, that thing. I thought P9 was in actual fact an SMT4 hardware part,
but to be compatible with P8 it has this fused option where two cores
act like a single SMT8 part.
The interleaving enumeration is done due to P7 asymmetric cores,
resuting in schedulers having the preference to use the lower threads.
Combined that results in a P9-fused configuration using two independent
full cores when there's only 2 runnable threads.
Which is a subtly different story from yours.
But reading your explanation, it looks like the Linux topology setup
could actually unravel the fused-faux-SMT8 into two independent SMT4
parts, negating that firmware option.
Anyway, a few comments just around there might be helpfull.
Thanks!
^ permalink raw reply
* Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
From: David Gibson @ 2020-07-22 7:51 UTC (permalink / raw)
To: Bharata B Rao; +Cc: Nathan Lynch, linuxppc-dev, Aneesh Kumar K.V
In-Reply-To: <20200722060506.GO7902@in.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]
On Wed, Jul 22, 2020 at 11:35:06AM +0530, Bharata B Rao wrote:
> On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
> > Bharata B Rao <bharata@linux.ibm.com> writes:
> > > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> > >> Nathan Lynch <nathanl@linux.ibm.com> writes:
> > >> > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> > >> >> This is the next version of the fixes for memory unplug on radix.
> > >> >> The issues and the fix are described in the actual patches.
> > >> >
> > >> > I guess this isn't actually causing problems at runtime right now, but I
> > >> > notice calls to resize_hpt_for_hotplug() from arch_add_memory() and
> > >> > arch_remove_memory(), which ought to be mmu-agnostic:
> > >> >
> > >> > int __ref arch_add_memory(int nid, u64 start, u64 size,
> > >> > struct mhp_params *params)
> > >> > {
> > >> > unsigned long start_pfn = start >> PAGE_SHIFT;
> > >> > unsigned long nr_pages = size >> PAGE_SHIFT;
> > >> > int rc;
> > >> >
> > >> > resize_hpt_for_hotplug(memblock_phys_mem_size());
> > >> >
> > >> > start = (unsigned long)__va(start);
> > >> > rc = create_section_mapping(start, start + size, nid,
> > >> > params->pgprot);
> > >> > ...
> > >>
> > >> Hmm well spotted.
> > >>
> > >> That does return early if the ops are not setup:
> > >>
> > >> int resize_hpt_for_hotplug(unsigned long new_mem_size)
> > >> {
> > >> unsigned target_hpt_shift;
> > >>
> > >> if (!mmu_hash_ops.resize_hpt)
> > >> return 0;
> > >>
> > >>
> > >> And:
> > >>
> > >> void __init hpte_init_pseries(void)
> > >> {
> > >> ...
> > >> if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
> > >> mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;
> > >>
> > >> And that comes in via ibm,hypertas-functions:
> > >>
> > >> {FW_FEATURE_HPT_RESIZE, "hcall-hpt-resize"},
> > >>
> > >>
> > >> But firmware is not necessarily going to add/remove that call based on
> > >> whether we're using hash/radix.
> > >
> > > Correct but hpte_init_pseries() will not be called for radix guests.
> >
> > Yeah, duh. You'd think the function name would have been a sufficient
> > clue for me :)
> >
> > >> So I think a follow-up patch is needed to make this more robust.
> > >>
> > >> Aneesh/Bharata what platform did you test this series on? I'm curious
> > >> how this didn't break.
> > >
> > > I have tested memory hotplug/unplug for radix guest on zz platform and
> > > sanity-tested this for hash guest on P8.
> > >
> > > As noted above, mmu_hash_ops.resize_hpt will not be set for radix
> > > guest and hence we won't see any breakage.
> >
> > OK.
> >
> > That's probably fine as it is then. Or maybe just a comment in
> > resize_hpt_for_hotplug() pointing out that resize_hpt will be NULL if
> > we're using radix.
>
> Or we could move these calls to hpt-only routines like below?
>
> David - Do you remember if there was any particular reason to have
> these two hpt-resize calls within powerpc-generic memory hotplug code?
I don't remember, sorry.
--
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 06/10] powerpc/smp: Generalize 2nd sched domain
From: Srikar Dronamraju @ 2020-07-22 8:18 UTC (permalink / raw)
To: peterz
Cc: Ingo Molnar, Nathan Lynch, Oliver OHalloran, Michael Neuling,
Michael Ellerman, Gautham R Shenoy, Jordan Niethe,
Anton Blanchard, LKML, Valentin Schneider, linuxppc-dev,
Nick Piggin
In-Reply-To: <20200722074624.GP119549@hirez.programming.kicks-ass.net>
* peterz@infradead.org <peterz@infradead.org> [2020-07-22 09:46:24]:
> On Tue, Jul 21, 2020 at 05:08:10PM +0530, Srikar Dronamraju wrote:
> > Currently "CACHE" domain happens to be the 2nd sched domain as per
> > powerpc_topology. This domain will collapse if cpumask of l2-cache is
> > same as SMT domain. However we could generalize this domain such that it
> > could mean either be a "CACHE" domain or a "BIGCORE" domain.
>
> What's the whole smallcore vs bigcore thing?
>
> Would it make sense to have an actual overview of the various topology
> layers somewhere? Because I'm getting lost and can't really make sense
> of the code due to that.
To quote with an example: using (Power 9)
Power 9 is an SMT 8 core by design. However this 8 thread core can run as 2
independent core with threads 0,2,4 and 6 acting like a core, while threads
1,3,5,7 acting as another core.
The firmware can decide to showcase them as 2 independent small cores or as
one big core. However the LLC (i.e last level of cache) is shared between
all the threads of the SMT 8 core. Future power chips, LLC might change, it
may be expanded to share with another SMT 8 core or it could be made
specific to SMT 4 core.
The smt 8 core is also known as fused core/ Big core.
The smaller smt 4 core is known as small core.
So on a Power9 Baremetal, the firmware would show up as SMT4 core.
and we have a CACHE level at SMT 8. CACHE level would be very very similar
to MC domain in X86.
Hope this is clear.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v2 01/10] powerpc/smp: Cache node for reuse
From: Srikar Dronamraju @ 2020-07-22 8:04 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ingo Molnar, Nathan Lynch, Oliver OHalloran, Michael Neuling,
Gautham R Shenoy, Peter Zijlstra, Jordan Niethe, Anton Blanchard,
LKML, Nick Piggin, linuxppc-dev, Valentin Schneider
In-Reply-To: <87imegq9my.fsf@mpe.ellerman.id.au>
* Michael Ellerman <michaele@au1.ibm.com> [2020-07-22 17:41:41]:
> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > While cpu_to_node is inline function with access to per_cpu variable.
> > However when using repeatedly, it may be cleaner to cache it in a local
> > variable.
>
> It's not clear what "cleaner" is supposed to mean. Are you saying it
> makes the source clearer, or the generated code?
>
> I'm not sure it will make any difference to the latter.
I meant the source code, I am okay dropping the hunks that try to cache
cpu_to_node.
>
> > Also fix a build error in a some weird config.
> > "error: _numa_cpu_lookup_table_ undeclared"
>
> Separate patch please.
Okay, will do.
>
> > No functional change
>
> The ifdef change means that's not true.
Okay
> > @@ -854,20 +854,24 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> > cpu_callin_map[boot_cpuid] = 1;
> >
> > for_each_possible_cpu(cpu) {
> > + int node = cpu_to_node(cpu);
> > +
>
> Does cpu_to_node() even work here?
Except in the case where NUMA is not enabled, (when cpu_to_node would return
-1), It should work here since numa initialization would have happened by
now. It cpu_to_node(cpu) should work once numa_setup_cpu() /
map_cpu_to_node() gets called. And those are being called before this.
>
> Doesn't look like it to me.
>
> More fallout from 8c272261194d ("powerpc/numa: Enable USE_PERCPU_NUMA_NODE_ID") ?
>
> > }
> >
> > /* Init the cpumasks so the boot CPU is related to itself */
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* RE: [RFC PATCH] powerpc/pseries/svm: capture instruction faulting on MMIO access, in sprg0 register
From: Ram Pai @ 2020-07-22 7:49 UTC (permalink / raw)
To: Michael Ellerman
Cc: ldufour, aik, kvm-ppc, bharata, sathnaga, sukadev, linuxppc-dev,
bauerman, david
In-Reply-To: <875zags3qp.fsf@mpe.ellerman.id.au>
On Wed, Jul 22, 2020 at 12:06:06PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > An instruction accessing a mmio address, generates a HDSI fault. This fault is
> > appropriately handled by the Hypervisor. However in the case of secureVMs, the
> > fault is delivered to the ultravisor.
> >
> > Unfortunately the Ultravisor has no correct-way to fetch the faulting
> > instruction. The PEF architecture does not allow Ultravisor to enable MMU
> > translation. Walking the two level page table to read the instruction can race
> > with other vcpus modifying the SVM's process scoped page table.
>
> You're trying to read the guest's kernel text IIUC, that mapping should
> be stable. Possibly permissions on it could change over time, but the
> virtual -> real mapping should not.
Actually the code does not capture the address of the instruction in the
sprg0 register. It captures the instruction itself. So should the mapping
matter?
>
> > This problem can be correctly solved with some help from the kernel.
> >
> > Capture the faulting instruction in SPRG0 register, before executing the
> > faulting instruction. This enables the ultravisor to easily procure the
> > faulting instruction and emulate it.
>
> This is not something I'm going to merge. Sorry.
Ok. Will consider other approaches.
RP
^ permalink raw reply
* Re: [PATCH v2 06/10] powerpc/smp: Generalize 2nd sched domain
From: peterz @ 2020-07-22 7:46 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Ingo Molnar, Nathan Lynch, Oliver OHalloran, Michael Neuling,
Michael Ellerman, Gautham R Shenoy, Jordan Niethe,
Anton Blanchard, LKML, Valentin Schneider, linuxppc-dev,
Nick Piggin
In-Reply-To: <20200721113814.32284-7-srikar@linux.vnet.ibm.com>
On Tue, Jul 21, 2020 at 05:08:10PM +0530, Srikar Dronamraju wrote:
> Currently "CACHE" domain happens to be the 2nd sched domain as per
> powerpc_topology. This domain will collapse if cpumask of l2-cache is
> same as SMT domain. However we could generalize this domain such that it
> could mean either be a "CACHE" domain or a "BIGCORE" domain.
What's the whole smallcore vs bigcore thing?
Would it make sense to have an actual overview of the various topology
layers somewhere? Because I'm getting lost and can't really make sense
of the code due to that.
^ permalink raw reply
* RE: [RFC PATCH] powerpc/pseries/svm: capture instruction faulting on MMIO access, in sprg0 register
From: Ram Pai @ 2020-07-22 7:45 UTC (permalink / raw)
To: Paul Mackerras
Cc: ldufour, aik, kvm-ppc, bharata, sathnaga, sukadev, linuxppc-dev,
bauerman, david
In-Reply-To: <20200722074205.GH7339@oc0525413822.ibm.com>
On Wed, Jul 22, 2020 at 12:42:05AM -0700, Ram Pai wrote:
> On Wed, Jul 22, 2020 at 03:02:32PM +1000, Paul Mackerras wrote:
> > On Thu, Jul 16, 2020 at 01:32:13AM -0700, Ram Pai wrote:
> > > An instruction accessing a mmio address, generates a HDSI fault. This fault is
> > > appropriately handled by the Hypervisor. However in the case of secureVMs, the
> > > fault is delivered to the ultravisor.
> > >
> > > Unfortunately the Ultravisor has no correct-way to fetch the faulting
> > > instruction. The PEF architecture does not allow Ultravisor to enable MMU
> > > translation. Walking the two level page table to read the instruction can race
> > > with other vcpus modifying the SVM's process scoped page table.
> > >
> > > This problem can be correctly solved with some help from the kernel.
> > >
> > > Capture the faulting instruction in SPRG0 register, before executing the
> > > faulting instruction. This enables the ultravisor to easily procure the
> > > faulting instruction and emulate it.
> >
> > Just a comment on the approach of putting the instruction in SPRG0:
> > these I/O accessors can be used in interrupt routines, which means
> > that if these accessors are ever used with interrupts enabled, there
> > is the possibility of an external interrupt occurring between the
> > instruction that sets SPRG0 and the load/store instruction that
> > faults. If the handler for that interrupt itself does an I/O access,
> > it will overwrite SPRG0, corrupting the value set by the interrupted
> > code.
>
> Acutally my proposed code restores the value of SPRG0 before returning back to
> the interrupted instruction. So here is the sequence. I think it works.
>
> (1) store sprg0 in register Rx (lets say srpg0 had 0xc. Rx now contains 0xc)
>
> (2) save faulting instruction address in sprg0 (lets say the value is 0xa.
> sprg0 will contain 0xa).
Small correction. sprg0 does not store the address of the faulting
instruction. It stores the isntruction itself. Regardless, the code
should work, I think.
RP
^ permalink raw reply
* Re: Re: [RFC PATCH] powerpc/pseries/svm: capture instruction faulting on MMIO access, in sprg0 register
From: Ram Pai @ 2020-07-22 7:42 UTC (permalink / raw)
To: Paul Mackerras
Cc: ldufour, aik, kvm-ppc, bharata, sathnaga, sukadev, linuxppc-dev,
bauerman, david
In-Reply-To: <20200722050232.GD3878639@thinks.paulus.ozlabs.org>
On Wed, Jul 22, 2020 at 03:02:32PM +1000, Paul Mackerras wrote:
> On Thu, Jul 16, 2020 at 01:32:13AM -0700, Ram Pai wrote:
> > An instruction accessing a mmio address, generates a HDSI fault. This fault is
> > appropriately handled by the Hypervisor. However in the case of secureVMs, the
> > fault is delivered to the ultravisor.
> >
> > Unfortunately the Ultravisor has no correct-way to fetch the faulting
> > instruction. The PEF architecture does not allow Ultravisor to enable MMU
> > translation. Walking the two level page table to read the instruction can race
> > with other vcpus modifying the SVM's process scoped page table.
> >
> > This problem can be correctly solved with some help from the kernel.
> >
> > Capture the faulting instruction in SPRG0 register, before executing the
> > faulting instruction. This enables the ultravisor to easily procure the
> > faulting instruction and emulate it.
>
> Just a comment on the approach of putting the instruction in SPRG0:
> these I/O accessors can be used in interrupt routines, which means
> that if these accessors are ever used with interrupts enabled, there
> is the possibility of an external interrupt occurring between the
> instruction that sets SPRG0 and the load/store instruction that
> faults. If the handler for that interrupt itself does an I/O access,
> it will overwrite SPRG0, corrupting the value set by the interrupted
> code.
Acutally my proposed code restores the value of SPRG0 before returning back to
the interrupted instruction. So here is the sequence. I think it works.
(1) store sprg0 in register Rx (lets say srpg0 had 0xc. Rx now contains 0xc)
(2) save faulting instruction address in sprg0 (lets say the value is 0xa.
sprg0 will contain 0xa).
(3) <----- interrupt arrives
(4) store sprg0 in register Ry ( Ry now contains 0xa )
(5) save faulting instruction address in sprg0 (lets say the value is 0xb).
(6) 0xb: execute faulting instruction
(7) restore Ry into sprg0 ( sprg0 now contains 0xa )
(8) <-- return from interrupt
(9) 0xa: execute faulting instruction
(10) restore Rx into sprg0 (sprg0 now contains 0xc)
>
> The choices to fix that would seem to be (a) disable interrupts around
> all I/O accesses, (b) have the accessor save and restore SPRG0, or (c)
> solve the problem another way, such as by doing a H_LOGICAL_CI_LOAD
> or H_LOGICAL_CI_STORE hypercall.
Ok. Will explore (c).
Thanks,
RP
--
Ram Pai
^ permalink raw reply
* Re: [PATCH v2 01/10] powerpc/smp: Cache node for reuse
From: Michael Ellerman @ 2020-07-22 7:41 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Oliver OHalloran, Michael Neuling,
Srikar Dronamraju, Peter Zijlstra, Jordan Niethe, Anton Blanchard,
LKML, Valentin Schneider, Nick Piggin, linuxppc-dev, Ingo Molnar
In-Reply-To: <20200721113814.32284-2-srikar@linux.vnet.ibm.com>
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> While cpu_to_node is inline function with access to per_cpu variable.
> However when using repeatedly, it may be cleaner to cache it in a local
> variable.
It's not clear what "cleaner" is supposed to mean. Are you saying it
makes the source clearer, or the generated code?
I'm not sure it will make any difference to the latter.
> Also fix a build error in a some weird config.
> "error: _numa_cpu_lookup_table_ undeclared"
Separate patch please.
> No functional change
The ifdef change means that's not true.
> Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: Michael Ellerman <michaele@au1.ibm.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Nick Piggin <npiggin@au1.ibm.com>
> Cc: Oliver OHalloran <oliveroh@au1.ibm.com>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Michael Neuling <mikey@linux.ibm.com>
> Cc: Anton Blanchard <anton@au1.ibm.com>
> Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
> Cc: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>
> Cc: Jordan Niethe <jniethe5@gmail.com>
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/smp.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 73199470c265..680c0edcc59d 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -843,7 +843,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>
> DBG("smp_prepare_cpus\n");
>
> - /*
> + /*
> * setup_cpu may need to be called on the boot cpu. We havent
> * spun any cpus up but lets be paranoid.
> */
> @@ -854,20 +854,24 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> cpu_callin_map[boot_cpuid] = 1;
>
> for_each_possible_cpu(cpu) {
> + int node = cpu_to_node(cpu);
> +
Does cpu_to_node() even work here?
Doesn't look like it to me.
More fallout from 8c272261194d ("powerpc/numa: Enable USE_PERCPU_NUMA_NODE_ID") ?
> zalloc_cpumask_var_node(&per_cpu(cpu_sibling_map, cpu),
> - GFP_KERNEL, cpu_to_node(cpu));
> + GFP_KERNEL, node);
> zalloc_cpumask_var_node(&per_cpu(cpu_l2_cache_map, cpu),
> - GFP_KERNEL, cpu_to_node(cpu));
> + GFP_KERNEL, node);
> zalloc_cpumask_var_node(&per_cpu(cpu_core_map, cpu),
> - GFP_KERNEL, cpu_to_node(cpu));
> + GFP_KERNEL, node);
> +#ifdef CONFIG_NEED_MULTIPLE_NODES
> /*
> * numa_node_id() works after this.
> */
> if (cpu_present(cpu)) {
> - set_cpu_numa_node(cpu, numa_cpu_lookup_table[cpu]);
> - set_cpu_numa_mem(cpu,
> - local_memory_node(numa_cpu_lookup_table[cpu]));
> + node = numa_cpu_lookup_table[cpu];
> + set_cpu_numa_node(cpu, node);
> + set_cpu_numa_mem(cpu, local_memory_node(node));
> }
> +#endif
> }
>
> /* Init the cpumasks so the boot CPU is related to itself */
> --
> 2.17.1
cheers
^ permalink raw reply
* Re: [PATCH v2 06/10] powerpc/smp: Generalize 2nd sched domain
From: Srikar Dronamraju @ 2020-07-22 7:39 UTC (permalink / raw)
To: Gautham R Shenoy
Cc: Nathan Lynch, Oliver OHalloran, Michael Neuling, Michael Ellerman,
Peter Zijlstra, Jordan Niethe, Anton Blanchard, LKML, Ingo Molnar,
Nick Piggin, linuxppc-dev, Valentin Schneider
In-Reply-To: <20200722065640.GE31038@in.ibm.com>
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-22 12:26:40]:
> Hello Srikar,
>
> On Tue, Jul 21, 2020 at 05:08:10PM +0530, Srikar Dronamraju wrote:
> > Currently "CACHE" domain happens to be the 2nd sched domain as per
> > powerpc_topology. This domain will collapse if cpumask of l2-cache is
> > same as SMT domain. However we could generalize this domain such that it
> > could mean either be a "CACHE" domain or a "BIGCORE" domain.
> >
> > While setting up the "CACHE" domain, check if shared_cache is already
> > set.
> > @@ -1339,14 +1345,20 @@ void start_secondary(void *unused)
> > /* Update topology CPU masks */
> > add_cpu_to_masks(cpu);
> >
> > - if (has_big_cores)
> > - sibling_mask = cpu_smallcore_mask;
> > /*
> > * Check for any shared caches. Note that this must be done on a
> > * per-core basis because one core in the pair might be disabled.
> > */
> > - if (!cpumask_equal(cpu_l2_cache_mask(cpu), sibling_mask(cpu)))
> > - shared_caches = true;
> > + if (!shared_caches) {
> > + struct cpumask *(*sibling_mask)(int) = cpu_sibling_mask;
> > + struct cpumask *mask = cpu_l2_cache_mask(cpu);
> > +
> > + if (has_big_cores)
> > + sibling_mask = cpu_smallcore_mask;
> > +
> > + if (cpumask_weight(mask) > cpumask_weight(sibling_mask(cpu)))
> > + shared_caches = true;
>
> At the risk of repeating my comment to the v1 version of the patch, we
> have shared caches only l2_cache_mask(cpu) is a strict superset of
> sibling_mask(cpu).
>
> "cpumask_weight(mask) > cpumask_weight(sibling_mask(cpu))" does not
> capture this.
Why would it not? We are setting shared_caches if and only if l2_cache_mask
is a strict superset of sibling/smallcore mask.
> Could we please use
>
> if (!cpumask_equal(sibling_mask(cpu), mask) &&
> cpumask_subset(sibling_mask(cpu), mask) {
> }
>
Scheduler mandates that two cpumasks for the same CPU would either have to
be equal or one of them has to be a strict superset of the other. If not the
scheduler would mark our domains as broken. That being the case,
cpumask_weight will correctly capture what we are looking for. That said
your condition is also correct but slightly heavier and doesn't provide us
with any more information or correctness.
>
> Otherwise the patch looks good to me.
>
> --
> Thanks and Regards
> gautham.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* [PATCH] powerpc/64s: Fix irq tracing corruption in interrupt/syscall return caused by perf interrupts
From: Nicholas Piggin @ 2020-07-22 7:34 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Nicholas Piggin
Alexey reports lockdep_assert_irqs_enabled() warnings when stress testing perf, e.g.,
WARNING: CPU: 0 PID: 1556 at kernel/softirq.c:169 __local_bh_enable_ip+0x258/0x270
CPU: 0 PID: 1556 Comm: syz-executor
NIP: c0000000001ec888 LR: c0000000001ec884 CTR: c000000000ef0610
REGS: c000000022d4f8a0 TRAP: 0700 Not tainted (5.8.0-rc3-x)
MSR: 8000000000029033 <SF,EE,ME,IR,DR,RI,LE> CR: 28008844 XER: 20040000
CFAR: c0000000001dc1d0 IRQMASK: 0
The interesting thing is MSR[EE] and IRQMASK shows interrupts are enabled,
suggesting the current->hardirqs_enabled irq tracing state is going out of sync
with the actual interrupt enable state.
The cause is a window in interrupt/syscall return where irq tracing state is being
adjusted for an irqs-enabled return while MSR[EE] is still enabled. A perf
interrupt hits and ends up calling trace_hardirqs_off() when restoring
interrupt flags to a disable state.
Fix this by disabling perf interrupts as well while adjusting irq tracing state.
Add a debug check that catches the condition sooner.
Fixes: 68b34588e202 ("powerpc/64/sycall: Implement syscall entry/exit logic in C")
Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
I can reproduce similar symptoms and this patch fixes my test case,
still trying to confirm Alexey's test case or whether there's another
similar bug causing it.
arch/powerpc/kernel/syscall_64.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 79edba3ab312..6c6f88eff915 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -107,8 +107,13 @@ notrace long system_call_exception(long r3, long r4, long r5,
*/
static notrace inline bool prep_irq_for_enabled_exit(void)
{
- /* This must be done with RI=1 because tracing may touch vmaps */
- trace_hardirqs_on();
+ if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS)) {
+ /* Prevent perf interrupts hitting and messing up the trace_hardirqs state */
+ irq_soft_mask_set(IRQS_ALL_DISABLED);
+
+ /* This must be done with RI=1 because tracing may touch vmaps */
+ trace_hardirqs_on();
+ }
/* This pattern matches prep_irq_for_idle */
__hard_EE_RI_disable();
@@ -123,6 +128,8 @@ static notrace inline bool prep_irq_for_enabled_exit(void)
local_paca->irq_happened = 0;
irq_soft_mask_set(IRQS_ENABLED);
+ lockdep_assert_irqs_enabled();
+
return true;
}
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v2 09/10] Powerpc/smp: Create coregroup domain
From: Srikar Dronamraju @ 2020-07-22 7:29 UTC (permalink / raw)
To: Gautham R Shenoy
Cc: Nathan Lynch, Oliver OHalloran, Michael Neuling, Michael Ellerman,
Peter Zijlstra, Jordan Niethe, Anton Blanchard, LKML, Ingo Molnar,
Nick Piggin, linuxppc-dev, Valentin Schneider
In-Reply-To: <20200722070436.GF31038@in.ibm.com>
> > @@ -1386,6 +1421,9 @@ int setup_profiling_timer(unsigned int multiplier)
> >
> > static void fixup_topology(void)
> > {
> > + if (!has_coregroup_support())
> > + powerpc_topology[mc_idx].mask = cpu_bigcore_mask;
> > +
>
> Shouldn't we move this condition after doing the fixup for shared
> caches ? Because if we have shared_caches, but not core_group, then we
> want the coregroup domain to degenerate correctly.
>
Currently we aren't consolidating, and hence the order doesn't matter for
degeneration. However even if in future, if we want to consolidate the
domains before calling set_sched_topology(), this order would be ideal.
>
> > if (shared_caches) {
> > pr_info("Using shared cache scheduler topology\n");
> > powerpc_topology[bigcore_idx].mask = shared_cache_mask;
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v2 2/2] KVM: PPC: Book3S HV: rework secure mem slot dropping
From: Laurent Dufour @ 2020-07-22 7:18 UTC (permalink / raw)
To: Ram Pai, paulus
Cc: linux-kernel, kvm-ppc, bharata, sukadev, linuxppc-dev, bauerman
In-Reply-To: <20200721213736.GG7339@oc0525413822.ibm.com>
Le 21/07/2020 à 23:37, Ram Pai a écrit :
> On Tue, Jul 21, 2020 at 12:42:02PM +0200, Laurent Dufour wrote:
>> When a secure memslot is dropped, all the pages backed in the secure device
>> (aka really backed by secure memory by the Ultravisor) should be paged out
>> to a normal page. Previously, this was achieved by triggering the page
>> fault mechanism which is calling kvmppc_svm_page_out() on each pages.
>>
>> This can't work when hot unplugging a memory slot because the memory slot
>> is flagged as invalid and gfn_to_pfn() is then not trying to access the
>> page, so the page fault mechanism is not triggered.
>>
>> Since the final goal is to make a call to kvmppc_svm_page_out() it seems
>> simpler to directly calling it instead of triggering such a mechanism. This
> ^^ call directly instead of triggering..
>
>> way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a
>> memslot.
>>
>> Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock,
>> the call to __kvmppc_svm_page_out() is made.
>> As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the
>> VMA is fetched in a lazy way, to not trigger find_vma() all the time. In
>> addition, the mmap_sem is help in read mode during that time, not in write
> ^^ held
>
>> mode since the virual memory layout is not impacted, and
>> kvm->arch.uvmem_lock prevents concurrent operation on the secure device.
>>
>> Cc: Ram Pai <linuxram@us.ibm.com>
>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
Thanks for reviewing this series.
Regarding the wordsmithing, Paul, could you manage that when pulling the series?
Thanks,
Laurent.
^ permalink raw reply
* Re: [PATCH v2 10/10] powerpc/smp: Implement cpu_to_coregroup_id
From: Gautham R Shenoy @ 2020-07-22 7:06 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Oliver OHalloran, Michael Neuling,
Michael Ellerman, Peter Zijlstra, Jordan Niethe, Anton Blanchard,
LKML, Ingo Molnar, Nick Piggin, linuxppc-dev, Valentin Schneider
In-Reply-To: <20200721113814.32284-11-srikar@linux.vnet.ibm.com>
On Tue, Jul 21, 2020 at 05:08:14PM +0530, Srikar Dronamraju wrote:
> Lookup the coregroup id from the associativity array.
>
> If unable to detect the coregroup id, fallback on the core id.
> This way, ensure sched_domain degenerates and an extra sched domain is
> not created.
>
> Ideally this function should have been implemented in
> arch/powerpc/kernel/smp.c. However if its implemented in mm/numa.c, we
> don't need to find the primary domain again.
>
> If the device-tree mentions more than one coregroup, then kernel
> implements only the last or the smallest coregroup, which currently
> corresponds to the penultimate domain in the device-tree.
>
> Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: Michael Ellerman <michaele@au1.ibm.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Nick Piggin <npiggin@au1.ibm.com>
> Cc: Oliver OHalloran <oliveroh@au1.ibm.com>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Michael Neuling <mikey@linux.ibm.com>
> Cc: Anton Blanchard <anton@au1.ibm.com>
> Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
> Cc: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>
> Cc: Jordan Niethe <jniethe5@gmail.com>
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Looks good to me.
Reviewed-by : Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> Changelog v1 -> v2:
> powerpc/smp: Implement cpu_to_coregroup_id
> Move coregroup_enabled before getting associativity (Gautham)
>
> arch/powerpc/mm/numa.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index ef8aa580da21..ae57b68beaee 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -1218,6 +1218,26 @@ int find_and_online_cpu_nid(int cpu)
>
> int cpu_to_coregroup_id(int cpu)
> {
> + __be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};
> + int index;
> +
> + if (cpu < 0 || cpu > nr_cpu_ids)
> + return -1;
> +
> + if (!coregroup_enabled)
> + goto out;
> +
> + if (!firmware_has_feature(FW_FEATURE_VPHN))
> + goto out;
> +
> + if (vphn_get_associativity(cpu, associativity))
> + goto out;
> +
> + index = of_read_number(associativity, 1);
> + if (index > min_common_depth + 1)
> + return of_read_number(&associativity[index - 1], 1);
> +
> +out:
> return cpu_to_core_id(cpu);
> }
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v2 09/10] Powerpc/smp: Create coregroup domain
From: Gautham R Shenoy @ 2020-07-22 7:04 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Oliver OHalloran, Michael Neuling,
Michael Ellerman, Peter Zijlstra, Jordan Niethe, Anton Blanchard,
LKML, Ingo Molnar, Nick Piggin, linuxppc-dev, Valentin Schneider
In-Reply-To: <20200721113814.32284-10-srikar@linux.vnet.ibm.com>
Hi Srikar,
On Tue, Jul 21, 2020 at 05:08:13PM +0530, Srikar Dronamraju wrote:
> Add percpu coregroup maps and masks to create coregroup domain.
> If a coregroup doesn't exist, the coregroup domain will be degenerated
> in favour of SMT/CACHE domain.
>
> Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: Michael Ellerman <michaele@au1.ibm.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Nick Piggin <npiggin@au1.ibm.com>
> Cc: Oliver OHalloran <oliveroh@au1.ibm.com>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Michael Neuling <mikey@linux.ibm.com>
> Cc: Anton Blanchard <anton@au1.ibm.com>
> Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
> Cc: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>
> Cc: Jordan Niethe <jniethe5@gmail.com>
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
A query below.
> ---
> Changelog v1 -> v2:
> Powerpc/smp: Create coregroup domain
> Moved coregroup topology fixup to fixup_topology (Gautham)
>
> arch/powerpc/include/asm/topology.h | 10 ++++++++
> arch/powerpc/kernel/smp.c | 38 +++++++++++++++++++++++++++++
> arch/powerpc/mm/numa.c | 5 ++++
> 3 files changed, 53 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index f0b6300e7dd3..6609174918ab 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
[..snip..]
> @@ -91,6 +92,7 @@ enum {
> smt_idx,
> #endif
> bigcore_idx,
> + mc_idx,
> die_idx,
> };
>
[..snip..]
> @@ -879,6 +896,7 @@ static struct sched_domain_topology_level powerpc_topology[] = {
> { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
> #endif
> { cpu_bigcore_mask, SD_INIT_NAME(BIGCORE) },
> + { cpu_mc_mask, SD_INIT_NAME(MC) },
> { cpu_cpu_mask, SD_INIT_NAME(DIE) },
> { NULL, },
> };
[..snip..]
> @@ -1386,6 +1421,9 @@ int setup_profiling_timer(unsigned int multiplier)
>
> static void fixup_topology(void)
> {
> + if (!has_coregroup_support())
> + powerpc_topology[mc_idx].mask = cpu_bigcore_mask;
> +
Shouldn't we move this condition after doing the fixup for shared
caches ? Because if we have shared_caches, but not core_group, then we
want the coregroup domain to degenerate correctly.
> if (shared_caches) {
> pr_info("Using shared cache scheduler topology\n");
> powerpc_topology[bigcore_idx].mask = shared_cache_mask;
--
Thanks and regards
gautham.
^ permalink raw reply
* Re: [PATCH v2 04/10] powerpc/smp: Enable small core scheduling sooner
From: Srikar Dronamraju @ 2020-07-22 6:59 UTC (permalink / raw)
To: Gautham R Shenoy
Cc: Nathan Lynch, Oliver OHalloran, Michael Neuling, Michael Ellerman,
Peter Zijlstra, Jordan Niethe, Anton Blanchard, LKML, Ingo Molnar,
Nick Piggin, linuxppc-dev, Valentin Schneider
In-Reply-To: <20200722055925.GC31038@in.ibm.com>
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-22 11:29:25]:
> Hello Srikar,
>
> On Tue, Jul 21, 2020 at 05:08:08PM +0530, Srikar Dronamraju wrote:
> > Enable small core scheduling as soon as we detect that we are in a
> > system that supports thread group. Doing so would avoid a redundant
> > check.
>
> The patch looks good to me. However, I think the commit message still
> reflect the v1 code where we were moving the functionality from
> smp_cpus_done() to init_big_cores().
>
> In this we are moving it to a helper function to collate all fixups to
> topology.
>
Right.
Thanks for pointing , will correct this.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v2 05/10] powerpc/smp: Dont assume l2-cache to be superset of sibling
From: Srikar Dronamraju @ 2020-07-22 6:57 UTC (permalink / raw)
To: Gautham R Shenoy
Cc: Nathan Lynch, Oliver OHalloran, Michael Neuling, Michael Ellerman,
Peter Zijlstra, Jordan Niethe, Anton Blanchard, LKML, Ingo Molnar,
Nick Piggin, linuxppc-dev, Valentin Schneider
In-Reply-To: <20200722062114.GD31038@in.ibm.com>
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-22 11:51:14]:
> Hi Srikar,
>
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 72f16dc0cb26..57468877499a 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -1196,6 +1196,7 @@ static bool update_mask_by_l2(int cpu, struct cpumask *(*mask_fn)(int))
> > if (!l2_cache)
> > return false;
> >
> > + cpumask_set_cpu(cpu, mask_fn(cpu));
>
>
> Ok, we need to do this because "cpu" is not yet set in the
> cpu_online_mask. Prior to your patch the "cpu" was getting set in
> cpu_l2_cache_map(cpu) as a side-effect of the code that is removed in
> the patch.
>
Right.
>
> > for_each_cpu(i, cpu_online_mask) {
> > /*
> > * when updating the marks the current CPU has not been marked
> > @@ -1278,29 +1279,30 @@ static void add_cpu_to_masks(int cpu)
> > * add it to it's own thread sibling mask.
> > */
> > cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
> > + cpumask_set_cpu(cpu, cpu_core_mask(cpu));
Note: Above, we are explicitly setting the cpu_core_mask.
> >
> > for (i = first_thread; i < first_thread + threads_per_core; i++)
> > if (cpu_online(i))
> > set_cpus_related(i, cpu, cpu_sibling_mask);
> >
> > add_cpu_to_smallcore_masks(cpu);
> > - /*
> > - * Copy the thread sibling mask into the cache sibling mask
> > - * and mark any CPUs that share an L2 with this CPU.
> > - */
> > - for_each_cpu(i, cpu_sibling_mask(cpu))
> > - set_cpus_related(cpu, i, cpu_l2_cache_mask);
> > update_mask_by_l2(cpu, cpu_l2_cache_mask);
> >
> > - /*
> > - * Copy the cache sibling mask into core sibling mask and mark
> > - * any CPUs on the same chip as this CPU.
> > - */
> > - for_each_cpu(i, cpu_l2_cache_mask(cpu))
> > - set_cpus_related(cpu, i, cpu_core_mask);
> > + if (pkg_id == -1) {
>
> I suppose this "if" condition is an optimization, since if pkg_id != -1,
> we anyway set these CPUs in the cpu_core_mask below.
>
> However...
This is not just an optimization.
The hunk removed would only work if cpu_l2_cache_mask is bigger than
cpu_sibling_mask. (this was the previous assumption that we want to break)
If the cpu_sibling_mask is bigger than cpu_l2_cache_mask and pkg_id is -1,
then setting only cpu_l2_cache_mask in cpu_core_mask will result in a broken
topology.
>
> > + struct cpumask *(*mask)(int) = cpu_sibling_mask;
> > +
> > + /*
> > + * Copy the sibling mask into core sibling mask and
> > + * mark any CPUs on the same chip as this CPU.
> > + */
> > + if (shared_caches)
> > + mask = cpu_l2_cache_mask;
> > +
> > + for_each_cpu(i, mask(cpu))
> > + set_cpus_related(cpu, i, cpu_core_mask);
> >
> > - if (pkg_id == -1)
> > return;
> > + }
>
>
> ... since "cpu" is not yet set in the cpu_online_mask, do we not miss setting
> "cpu" in the cpu_core_mask(cpu) in the for-loop below ?
>
>
As noted above, we are setting before. So we don't missing the cpu and hence
have not different from before.
> --
> Thanks and Regards
> gautham.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* [PATCH v2 16/16] powerpc/powernv/sriov: Remove vfs_expanded
From: Oliver O'Halloran @ 2020-07-22 6:57 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20200722065715.1432738-1-oohall@gmail.com>
Previously iov->vfs_expanded was used for two purposes.
1) To work out how much we need to multiple the per-VF BAR size to figure
out the total space required for the IOV BAR.
2) To indicate that IOV is not usable with this device (vfs_expanded == 0).
We don't really need the field for either since the multiple in 1) is
always the number PEs supported by the PHB. Similarly, we don't really need
it in 2) either since the IOV data field will be NULL if we can't use IOV
with the device.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
v2: New
---
arch/powerpc/platforms/powernv/pci-sriov.c | 9 +++------
arch/powerpc/platforms/powernv/pci.h | 3 ---
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
index 76215d01405b..5742215b4093 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -213,8 +213,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
iov->need_shift = true;
}
- iov->vfs_expanded = mul;
-
return;
disable_iov:
@@ -256,6 +254,7 @@ void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
int resno)
{
+ struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
struct pnv_iov_data *iov = pnv_iov_get(pdev);
resource_size_t align;
@@ -267,8 +266,6 @@ resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
*/
if (!iov)
return align;
- if (!iov->vfs_expanded)
- return align;
align = pci_iov_resource_size(pdev, resno);
@@ -290,7 +287,7 @@ resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
* If the M64 BAR is in Single PE mode, return the VF BAR size or
* M64 segment size if IOV BAR size is less.
*/
- return iov->vfs_expanded * align;
+ return phb->ioda.total_pe_num * align;
}
static int pnv_pci_vf_release_m64(struct pci_dev *pdev, u16 num_vfs)
@@ -708,7 +705,7 @@ static int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
return -ENXIO;
}
- if (!iov->vfs_expanded) {
+ if (!iov) {
dev_info(&pdev->dev, "don't support this SRIOV device"
" with non 64bit-prefetchable IOV BAR\n");
return -ENOSPC;
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 902e928c7c22..c8cc152bdf52 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -234,9 +234,6 @@ void pnv_ioda_free_pe(struct pnv_ioda_pe *pe);
* and this structure is used to keep track of it all.
*/
struct pnv_iov_data {
- /* number of VFs IOV BAR expanded. FIXME: rename this to something less bad */
- u16 vfs_expanded;
-
/* number of VFs enabled */
u16 num_vfs;
--
2.26.2
^ permalink raw reply related
* [PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
From: Oliver O'Halloran @ 2020-07-22 6:57 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20200722065715.1432738-1-oohall@gmail.com>
Using single PE BARs to map an SR-IOV BAR is really a choice about what
strategy to use when mapping a BAR. It doesn't make much sense for this to
be a global setting since a device might have one large BAR which needs to
be mapped with single PE windows and another smaller BAR that can be mapped
with a regular segmented window. Make the segmented vs single decision a
per-BAR setting and clean up the logic that decides which mode to use.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
v2: Dropped unused total_vfs variables in pnv_pci_ioda_fixup_iov_resources()
Dropped bar_no from pnv_pci_iov_resource_alignment()
Minor re-wording of comments.
---
arch/powerpc/platforms/powernv/pci-sriov.c | 131 ++++++++++-----------
arch/powerpc/platforms/powernv/pci.h | 11 +-
2 files changed, 73 insertions(+), 69 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
index ce8ad6851d73..76215d01405b 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -146,21 +146,17 @@
static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
{
struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
- const resource_size_t gate = phb->ioda.m64_segsize >> 2;
struct resource *res;
int i;
- resource_size_t size, total_vf_bar_sz;
+ resource_size_t vf_bar_sz;
struct pnv_iov_data *iov;
- int mul, total_vfs;
+ int mul;
iov = kzalloc(sizeof(*iov), GFP_KERNEL);
if (!iov)
goto disable_iov;
pdev->dev.archdata.iov_data = iov;
-
- total_vfs = pci_sriov_get_totalvfs(pdev);
mul = phb->ioda.total_pe_num;
- total_vf_bar_sz = 0;
for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
res = &pdev->resource[i + PCI_IOV_RESOURCES];
@@ -173,50 +169,50 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
goto disable_iov;
}
- total_vf_bar_sz += pci_iov_resource_size(pdev,
- i + PCI_IOV_RESOURCES);
+ vf_bar_sz = pci_iov_resource_size(pdev, i + PCI_IOV_RESOURCES);
/*
- * If bigger than quarter of M64 segment size, just round up
- * power of two.
+ * Generally, one segmented M64 BAR maps one IOV BAR. However,
+ * if a VF BAR is too large we end up wasting a lot of space.
+ * If each VF needs more than 1/4 of the default m64 segment
+ * then each VF BAR should be mapped in single-PE mode to reduce
+ * the amount of space required. This does however limit the
+ * number of VFs we can support.
*
- * Generally, one M64 BAR maps one IOV BAR. To avoid conflict
- * with other devices, IOV BAR size is expanded to be
- * (total_pe * VF_BAR_size). When VF_BAR_size is half of M64
- * segment size , the expanded size would equal to half of the
- * whole M64 space size, which will exhaust the M64 Space and
- * limit the system flexibility. This is a design decision to
- * set the boundary to quarter of the M64 segment size.
+ * The 1/4 limit is arbitrary and can be tweaked.
*/
- if (total_vf_bar_sz > gate) {
- mul = roundup_pow_of_two(total_vfs);
- dev_info(&pdev->dev,
- "VF BAR Total IOV size %llx > %llx, roundup to %d VFs\n",
- total_vf_bar_sz, gate, mul);
- iov->m64_single_mode = true;
- break;
- }
- }
+ if (vf_bar_sz > (phb->ioda.m64_segsize >> 2)) {
+ /*
+ * On PHB3, the minimum size alignment of M64 BAR in
+ * single mode is 32MB. If this VF BAR is smaller than
+ * 32MB, but still too large for a segmented window
+ * then we can't map it and need to disable SR-IOV for
+ * this device.
+ */
+ if (vf_bar_sz < SZ_32M) {
+ pci_err(pdev, "VF BAR%d: %pR can't be mapped in single PE mode\n",
+ i, res);
+ goto disable_iov;
+ }
- for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
- res = &pdev->resource[i + PCI_IOV_RESOURCES];
- if (!res->flags || res->parent)
+ iov->m64_single_mode[i] = true;
continue;
+ }
- size = pci_iov_resource_size(pdev, i + PCI_IOV_RESOURCES);
/*
- * On PHB3, the minimum size alignment of M64 BAR in single
- * mode is 32MB.
+ * This BAR can be mapped with one segmented window, so adjust
+ * te resource size to accommodate.
*/
- if (iov->m64_single_mode && (size < SZ_32M))
- goto disable_iov;
+ pci_dbg(pdev, " Fixing VF BAR%d: %pR to\n", i, res);
+ res->end = res->start + vf_bar_sz * mul - 1;
+ pci_dbg(pdev, " %pR\n", res);
- dev_dbg(&pdev->dev, " Fixing VF BAR%d: %pR to\n", i, res);
- res->end = res->start + size * mul - 1;
- dev_dbg(&pdev->dev, " %pR\n", res);
- dev_info(&pdev->dev, "VF BAR%d: %pR (expanded to %d VFs for PE alignment)",
+ pci_info(pdev, "VF BAR%d: %pR (expanded to %d VFs for PE alignment)",
i, res, mul);
+
+ iov->need_shift = true;
}
+
iov->vfs_expanded = mul;
return;
@@ -260,42 +256,40 @@ void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
int resno)
{
- struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
struct pnv_iov_data *iov = pnv_iov_get(pdev);
resource_size_t align;
+ /*
+ * iov can be null if we have an SR-IOV device with IOV BAR that can't
+ * be placed in the m64 space (i.e. The BAR is 32bit or non-prefetch).
+ * In that case we don't allow VFs to be enabled since one of their
+ * BARs would not be placed in the correct PE.
+ */
+ if (!iov)
+ return align;
+ if (!iov->vfs_expanded)
+ return align;
+
+ align = pci_iov_resource_size(pdev, resno);
+
+ /*
+ * If we're using single mode then we can just use the native VF BAR
+ * alignment. We validated that it's possible to use a single PE
+ * window above when we did the fixup.
+ */
+ if (iov->m64_single_mode[resno - PCI_IOV_RESOURCES])
+ return align;
+
/*
* On PowerNV platform, IOV BAR is mapped by M64 BAR to enable the
* SR-IOV. While from hardware perspective, the range mapped by M64
* BAR should be size aligned.
*
- * When IOV BAR is mapped with M64 BAR in Single PE mode, the extra
- * powernv-specific hardware restriction is gone. But if just use the
- * VF BAR size as the alignment, PF BAR / VF BAR may be allocated with
- * in one segment of M64 #15, which introduces the PE conflict between
- * PF and VF. Based on this, the minimum alignment of an IOV BAR is
- * m64_segsize.
- *
* This function returns the total IOV BAR size if M64 BAR is in
* Shared PE mode or just VF BAR size if not.
* If the M64 BAR is in Single PE mode, return the VF BAR size or
* M64 segment size if IOV BAR size is less.
*/
- align = pci_iov_resource_size(pdev, resno);
-
- /*
- * iov can be null if we have an SR-IOV device with IOV BAR that can't
- * be placed in the m64 space (i.e. The BAR is 32bit or non-prefetch).
- * In that case we don't allow VFs to be enabled so just return the
- * default alignment.
- */
- if (!iov)
- return align;
- if (!iov->vfs_expanded)
- return align;
- if (iov->m64_single_mode)
- return max(align, (resource_size_t)phb->ioda.m64_segsize);
-
return iov->vfs_expanded * align;
}
@@ -450,7 +444,7 @@ static int pnv_pci_vf_assign_m64(struct pci_dev *pdev, u16 num_vfs)
continue;
/* don't need single mode? map everything in one go! */
- if (!iov->m64_single_mode) {
+ if (!iov->m64_single_mode[i]) {
win = pnv_pci_alloc_m64_bar(phb, iov);
if (win < 0)
goto m64_failed;
@@ -543,6 +537,8 @@ static int pnv_pci_vf_resource_shift(struct pci_dev *dev, int offset)
res = &dev->resource[i + PCI_IOV_RESOURCES];
if (!res->flags || !res->parent)
continue;
+ if (iov->m64_single_mode[i])
+ continue;
/*
* The actual IOV BAR range is determined by the start address
@@ -574,6 +570,8 @@ static int pnv_pci_vf_resource_shift(struct pci_dev *dev, int offset)
res = &dev->resource[i + PCI_IOV_RESOURCES];
if (!res->flags || !res->parent)
continue;
+ if (iov->m64_single_mode[i])
+ continue;
size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
res2 = *res;
@@ -619,8 +617,8 @@ static void pnv_pci_sriov_disable(struct pci_dev *pdev)
/* Release VF PEs */
pnv_ioda_release_vf_PE(pdev);
- /* Un-shift the IOV BAR resources */
- if (!iov->m64_single_mode)
+ /* Un-shift the IOV BARs if we need to */
+ if (iov->need_shift)
pnv_pci_vf_resource_shift(pdev, -base_pe);
/* Release M64 windows */
@@ -738,9 +736,8 @@ static int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
* the IOV BAR according to the PE# allocated to the VFs.
* Otherwise, the PE# for the VF will conflict with others.
*/
- if (!iov->m64_single_mode) {
- ret = pnv_pci_vf_resource_shift(pdev,
- base_pe->pe_number);
+ if (iov->need_shift) {
+ ret = pnv_pci_vf_resource_shift(pdev, base_pe->pe_number);
if (ret)
goto shift_failed;
}
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 41a6f4e938e4..902e928c7c22 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -243,8 +243,15 @@ struct pnv_iov_data {
/* pointer to the array of VF PEs. num_vfs long*/
struct pnv_ioda_pe *vf_pe_arr;
- /* Did we map the VF BARs with single-PE IODA BARs? */
- bool m64_single_mode;
+ /* Did we map the VF BAR with single-PE IODA BARs? */
+ bool m64_single_mode[PCI_SRIOV_NUM_BARS];
+
+ /*
+ * True if we're using any segmented windows. In that case we need
+ * shift the start of the IOV resource the segment corresponding to
+ * the allocated PE.
+ */
+ bool need_shift;
/*
* Bit mask used to track which m64 windows are used to map the
--
2.26.2
^ permalink raw reply related
* [PATCH v2 14/16] powerpc/powernv/sriov: Refactor M64 BAR setup
From: Oliver O'Halloran @ 2020-07-22 6:57 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Oliver O'Halloran
In-Reply-To: <20200722065715.1432738-1-oohall@gmail.com>
Split up the logic so that we have one branch that handles setting up a
segmented window and another that handles setting up single PE windows for
each VF.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
v2: no changes
---
arch/powerpc/platforms/powernv/pci-sriov.c | 57 ++++++++++------------
1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
index f9aa5773dc6d..ce8ad6851d73 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -438,52 +438,49 @@ static int pnv_pci_vf_assign_m64(struct pci_dev *pdev, u16 num_vfs)
struct resource *res;
int i, j;
int64_t rc;
- int total_vfs;
resource_size_t size, start;
- int m64_bars;
+ int base_pe_num;
phb = pci_bus_to_pnvhb(pdev->bus);
iov = pnv_iov_get(pdev);
- total_vfs = pci_sriov_get_totalvfs(pdev);
-
- if (iov->m64_single_mode)
- m64_bars = num_vfs;
- else
- m64_bars = 1;
for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
res = &pdev->resource[i + PCI_IOV_RESOURCES];
if (!res->flags || !res->parent)
continue;
- for (j = 0; j < m64_bars; j++) {
+ /* don't need single mode? map everything in one go! */
+ if (!iov->m64_single_mode) {
win = pnv_pci_alloc_m64_bar(phb, iov);
if (win < 0)
goto m64_failed;
- if (iov->m64_single_mode) {
- int pe_num = iov->vf_pe_arr[j].pe_number;
-
- size = pci_iov_resource_size(pdev,
- PCI_IOV_RESOURCES + i);
- start = res->start + size * j;
- rc = pnv_ioda_map_m64_single(phb, win,
- pe_num,
- start,
- size);
- } else {
- size = resource_size(res);
- start = res->start;
-
- rc = pnv_ioda_map_m64_segmented(phb, win, start,
- size);
- }
+ size = resource_size(res);
+ start = res->start;
- if (rc != OPAL_SUCCESS) {
- dev_err(&pdev->dev, "Failed to map M64 window #%d: %lld\n",
- win, rc);
+ rc = pnv_ioda_map_m64_segmented(phb, win, start, size);
+ if (rc)
+ goto m64_failed;
+
+ continue;
+ }
+
+ /* otherwise map each VF with single PE BARs */
+ size = pci_iov_resource_size(pdev, PCI_IOV_RESOURCES + i);
+ base_pe_num = iov->vf_pe_arr[0].pe_number;
+
+ for (j = 0; j < num_vfs; j++) {
+ win = pnv_pci_alloc_m64_bar(phb, iov);
+ if (win < 0)
+ goto m64_failed;
+
+ start = res->start + size * j;
+ rc = pnv_ioda_map_m64_single(phb, win,
+ base_pe_num + j,
+ start,
+ size);
+ if (rc)
goto m64_failed;
- }
}
}
return 0;
--
2.26.2
^ permalink raw reply related
* [PATCH v2 13/16] powerpc/powernv/sriov: Move M64 BAR allocation into a helper
From: Oliver O'Halloran @ 2020-07-22 6:57 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Oliver O'Halloran
In-Reply-To: <20200722065715.1432738-1-oohall@gmail.com>
I want to refactor the loop this code is currently inside of. Hoist it on
out.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
v2: no change
---
arch/powerpc/platforms/powernv/pci-sriov.c | 31 ++++++++++++++--------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
index b60d8a054a61..f9aa5773dc6d 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -413,6 +413,23 @@ static int64_t pnv_ioda_map_m64_single(struct pnv_phb *phb,
return rc;
}
+static int pnv_pci_alloc_m64_bar(struct pnv_phb *phb, struct pnv_iov_data *iov)
+{
+ int win;
+
+ do {
+ win = find_next_zero_bit(&phb->ioda.m64_bar_alloc,
+ phb->ioda.m64_bar_idx + 1, 0);
+
+ if (win >= phb->ioda.m64_bar_idx + 1)
+ return -1;
+ } while (test_and_set_bit(win, &phb->ioda.m64_bar_alloc));
+
+ set_bit(win, iov->used_m64_bar_mask);
+
+ return win;
+}
+
static int pnv_pci_vf_assign_m64(struct pci_dev *pdev, u16 num_vfs)
{
struct pnv_iov_data *iov;
@@ -440,17 +457,9 @@ static int pnv_pci_vf_assign_m64(struct pci_dev *pdev, u16 num_vfs)
continue;
for (j = 0; j < m64_bars; j++) {
-
- /* allocate a window ID for this BAR */
- do {
- win = find_next_zero_bit(&phb->ioda.m64_bar_alloc,
- phb->ioda.m64_bar_idx + 1, 0);
-
- if (win >= phb->ioda.m64_bar_idx + 1)
- goto m64_failed;
- } while (test_and_set_bit(win, &phb->ioda.m64_bar_alloc));
- set_bit(win, iov->used_m64_bar_mask);
-
+ win = pnv_pci_alloc_m64_bar(phb, iov);
+ if (win < 0)
+ goto m64_failed;
if (iov->m64_single_mode) {
int pe_num = iov->vf_pe_arr[j].pe_number;
--
2.26.2
^ permalink raw reply related
* [PATCH v2 12/16] powerpc/powernv/sriov: De-indent setup and teardown
From: Oliver O'Halloran @ 2020-07-22 6:57 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20200722065715.1432738-1-oohall@gmail.com>
Remove the IODA2 PHB checks. We already assume IODA2 in several places so
there's not much point in wrapping most of the setup and teardown process
in an if block.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
v2: Added a note that iov->vf_pe_arr is a pointer into the PHB's PE array
rather than something we allocate.
---
arch/powerpc/platforms/powernv/pci-sriov.c | 86 ++++++++++++----------
arch/powerpc/platforms/powernv/pci.h | 5 +-
2 files changed, 50 insertions(+), 41 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
index 5981323cd9a6..b60d8a054a61 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -607,16 +607,18 @@ static void pnv_pci_sriov_disable(struct pci_dev *pdev)
num_vfs = iov->num_vfs;
base_pe = iov->vf_pe_arr[0].pe_number;
+ if (WARN_ON(!iov))
+ return;
+
/* Release VF PEs */
pnv_ioda_release_vf_PE(pdev);
- if (phb->type == PNV_PHB_IODA2) {
- if (!iov->m64_single_mode)
- pnv_pci_vf_resource_shift(pdev, -base_pe);
+ /* Un-shift the IOV BAR resources */
+ if (!iov->m64_single_mode)
+ pnv_pci_vf_resource_shift(pdev, -base_pe);
- /* Release M64 windows */
- pnv_pci_vf_release_m64(pdev, num_vfs);
- }
+ /* Release M64 windows */
+ pnv_pci_vf_release_m64(pdev, num_vfs);
}
static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
@@ -690,41 +692,51 @@ static int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
phb = pci_bus_to_pnvhb(pdev->bus);
iov = pnv_iov_get(pdev);
- if (phb->type == PNV_PHB_IODA2) {
- if (!iov->vfs_expanded) {
- dev_info(&pdev->dev, "don't support this SRIOV device"
- " with non 64bit-prefetchable IOV BAR\n");
- return -ENOSPC;
- }
+ /*
+ * There's a calls to IODA2 PE setup code littered throughout. We could
+ * probably fix that, but we'd still have problems due to the
+ * restriction inherent on IODA1 PHBs.
+ *
+ * NB: We class IODA3 as IODA2 since they're very similar.
+ */
+ if (phb->type != PNV_PHB_IODA2) {
+ pci_err(pdev, "SR-IOV is not supported on this PHB\n");
+ return -ENXIO;
+ }
- /* allocate a contigious block of PEs for our VFs */
- base_pe = pnv_ioda_alloc_pe(phb, num_vfs);
- if (!base_pe) {
- pci_err(pdev, "Unable to allocate PEs for %d VFs\n", num_vfs);
- return -EBUSY;
- }
+ if (!iov->vfs_expanded) {
+ dev_info(&pdev->dev, "don't support this SRIOV device"
+ " with non 64bit-prefetchable IOV BAR\n");
+ return -ENOSPC;
+ }
- iov->vf_pe_arr = base_pe;
- iov->num_vfs = num_vfs;
+ /* allocate a contigious block of PEs for our VFs */
+ base_pe = pnv_ioda_alloc_pe(phb, num_vfs);
+ if (!base_pe) {
+ pci_err(pdev, "Unable to allocate PEs for %d VFs\n", num_vfs);
+ return -EBUSY;
+ }
- /* Assign M64 window accordingly */
- ret = pnv_pci_vf_assign_m64(pdev, num_vfs);
- if (ret) {
- dev_info(&pdev->dev, "Not enough M64 window resources\n");
- goto m64_failed;
- }
+ iov->vf_pe_arr = base_pe;
+ iov->num_vfs = num_vfs;
- /*
- * When using one M64 BAR to map one IOV BAR, we need to shift
- * the IOV BAR according to the PE# allocated to the VFs.
- * Otherwise, the PE# for the VF will conflict with others.
- */
- if (!iov->m64_single_mode) {
- ret = pnv_pci_vf_resource_shift(pdev,
- base_pe->pe_number);
- if (ret)
- goto shift_failed;
- }
+ /* Assign M64 window accordingly */
+ ret = pnv_pci_vf_assign_m64(pdev, num_vfs);
+ if (ret) {
+ dev_info(&pdev->dev, "Not enough M64 window resources\n");
+ goto m64_failed;
+ }
+
+ /*
+ * When using one M64 BAR to map one IOV BAR, we need to shift
+ * the IOV BAR according to the PE# allocated to the VFs.
+ * Otherwise, the PE# for the VF will conflict with others.
+ */
+ if (!iov->m64_single_mode) {
+ ret = pnv_pci_vf_resource_shift(pdev,
+ base_pe->pe_number);
+ if (ret)
+ goto shift_failed;
}
/* Setup VF PEs */
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index f76923f44f66..41a6f4e938e4 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -240,10 +240,7 @@ struct pnv_iov_data {
/* number of VFs enabled */
u16 num_vfs;
- /*
- * Pointer to the IODA PE state of each VF. Note that this is a pointer
- * into the PHB's PE array (phb->ioda.pe_array).
- */
+ /* pointer to the array of VF PEs. num_vfs long*/
struct pnv_ioda_pe *vf_pe_arr;
/* Did we map the VF BARs with single-PE IODA BARs? */
--
2.26.2
^ permalink raw reply related
* [PATCH v2 11/16] powerpc/powernv/sriov: Drop iov->pe_num_map[]
From: Oliver O'Halloran @ 2020-07-22 6:57 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Oliver O'Halloran
In-Reply-To: <20200722065715.1432738-1-oohall@gmail.com>
Currently the iov->pe_num_map[] does one of two things depending on
whether single PE mode is being used or not. When it is, this contains an
array which maps a vf_index to the corresponding PE number. When single PE
mode is not being used this contains a scalar which is the base PE for the
set of enabled VFs (for for VFn is base + n).
The array was necessary because when calling pnv_ioda_alloc_pe() there is
no guarantee that the allocated PEs would be contigious. We can now
allocate contigious blocks of PEs so this is no longer an issue. This
allows us to drop the if (single_mode) {} .. else {} block scattered
through the SR-IOV code which is a nice clean up.
This also fixes a bug in pnv_pci_sriov_disable() which is the non-atomic
bitmap_clear() to manipulate the PE allocation map. Other users of the map
assume it will be accessed with atomic ops.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
v2: Added a note to the commit message about bitmap_clear()
---
arch/powerpc/platforms/powernv/pci-sriov.c | 109 +++++----------------
arch/powerpc/platforms/powernv/pci.h | 7 +-
2 files changed, 28 insertions(+), 88 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
index d90e11218add..5981323cd9a6 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -453,11 +453,13 @@ static int pnv_pci_vf_assign_m64(struct pci_dev *pdev, u16 num_vfs)
if (iov->m64_single_mode) {
+ int pe_num = iov->vf_pe_arr[j].pe_number;
+
size = pci_iov_resource_size(pdev,
PCI_IOV_RESOURCES + i);
start = res->start + size * j;
rc = pnv_ioda_map_m64_single(phb, win,
- iov->pe_num_map[j],
+ pe_num,
start,
size);
} else {
@@ -596,38 +598,24 @@ static int pnv_pci_vf_resource_shift(struct pci_dev *dev, int offset)
static void pnv_pci_sriov_disable(struct pci_dev *pdev)
{
+ u16 num_vfs, base_pe;
struct pnv_phb *phb;
- struct pnv_ioda_pe *pe;
struct pnv_iov_data *iov;
- u16 num_vfs, i;
phb = pci_bus_to_pnvhb(pdev->bus);
iov = pnv_iov_get(pdev);
num_vfs = iov->num_vfs;
+ base_pe = iov->vf_pe_arr[0].pe_number;
/* Release VF PEs */
pnv_ioda_release_vf_PE(pdev);
if (phb->type == PNV_PHB_IODA2) {
if (!iov->m64_single_mode)
- pnv_pci_vf_resource_shift(pdev, -*iov->pe_num_map);
+ pnv_pci_vf_resource_shift(pdev, -base_pe);
/* Release M64 windows */
pnv_pci_vf_release_m64(pdev, num_vfs);
-
- /* Release PE numbers */
- if (iov->m64_single_mode) {
- for (i = 0; i < num_vfs; i++) {
- if (iov->pe_num_map[i] == IODA_INVALID_PE)
- continue;
-
- pe = &phb->ioda.pe_array[iov->pe_num_map[i]];
- pnv_ioda_free_pe(pe);
- }
- } else
- bitmap_clear(phb->ioda.pe_alloc, *iov->pe_num_map, num_vfs);
- /* Releasing pe_num_map */
- kfree(iov->pe_num_map);
}
}
@@ -653,13 +641,7 @@ static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
int vf_bus = pci_iov_virtfn_bus(pdev, vf_index);
struct pci_dn *vf_pdn;
- if (iov->m64_single_mode)
- pe_num = iov->pe_num_map[vf_index];
- else
- pe_num = *iov->pe_num_map + vf_index;
-
- pe = &phb->ioda.pe_array[pe_num];
- pe->pe_number = pe_num;
+ pe = &iov->vf_pe_arr[vf_index];
pe->phb = phb;
pe->flags = PNV_IODA_PE_VF;
pe->pbus = NULL;
@@ -667,6 +649,7 @@ static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
pe->mve_number = -1;
pe->rid = (vf_bus << 8) | vf_devfn;
+ pe_num = pe->pe_number;
pe_info(pe, "VF %04d:%02d:%02d.%d associated with PE#%x\n",
pci_domain_nr(pdev->bus), pdev->bus->number,
PCI_SLOT(vf_devfn), PCI_FUNC(vf_devfn), pe_num);
@@ -698,9 +681,9 @@ static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
static int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
{
+ struct pnv_ioda_pe *base_pe;
struct pnv_iov_data *iov;
struct pnv_phb *phb;
- struct pnv_ioda_pe *pe;
int ret;
u16 i;
@@ -714,55 +697,14 @@ static int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
return -ENOSPC;
}
- /*
- * When M64 BARs functions in Single PE mode, the number of VFs
- * could be enabled must be less than the number of M64 BARs.
- */
- if (iov->m64_single_mode && num_vfs > phb->ioda.m64_bar_idx) {
- dev_info(&pdev->dev, "Not enough M64 BAR for VFs\n");
+ /* allocate a contigious block of PEs for our VFs */
+ base_pe = pnv_ioda_alloc_pe(phb, num_vfs);
+ if (!base_pe) {
+ pci_err(pdev, "Unable to allocate PEs for %d VFs\n", num_vfs);
return -EBUSY;
}
- /* Allocating pe_num_map */
- if (iov->m64_single_mode)
- iov->pe_num_map = kmalloc_array(num_vfs,
- sizeof(*iov->pe_num_map),
- GFP_KERNEL);
- else
- iov->pe_num_map = kmalloc(sizeof(*iov->pe_num_map), GFP_KERNEL);
-
- if (!iov->pe_num_map)
- return -ENOMEM;
-
- if (iov->m64_single_mode)
- for (i = 0; i < num_vfs; i++)
- iov->pe_num_map[i] = IODA_INVALID_PE;
-
- /* Calculate available PE for required VFs */
- if (iov->m64_single_mode) {
- for (i = 0; i < num_vfs; i++) {
- pe = pnv_ioda_alloc_pe(phb);
- if (!pe) {
- ret = -EBUSY;
- goto m64_failed;
- }
-
- iov->pe_num_map[i] = pe->pe_number;
- }
- } else {
- mutex_lock(&phb->ioda.pe_alloc_mutex);
- *iov->pe_num_map = bitmap_find_next_zero_area(
- phb->ioda.pe_alloc, phb->ioda.total_pe_num,
- 0, num_vfs, 0);
- if (*iov->pe_num_map >= phb->ioda.total_pe_num) {
- mutex_unlock(&phb->ioda.pe_alloc_mutex);
- dev_info(&pdev->dev, "Failed to enable VF%d\n", num_vfs);
- kfree(iov->pe_num_map);
- return -EBUSY;
- }
- bitmap_set(phb->ioda.pe_alloc, *iov->pe_num_map, num_vfs);
- mutex_unlock(&phb->ioda.pe_alloc_mutex);
- }
+ iov->vf_pe_arr = base_pe;
iov->num_vfs = num_vfs;
/* Assign M64 window accordingly */
@@ -778,9 +720,10 @@ static int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
* Otherwise, the PE# for the VF will conflict with others.
*/
if (!iov->m64_single_mode) {
- ret = pnv_pci_vf_resource_shift(pdev, *iov->pe_num_map);
+ ret = pnv_pci_vf_resource_shift(pdev,
+ base_pe->pe_number);
if (ret)
- goto m64_failed;
+ goto shift_failed;
}
}
@@ -789,20 +732,12 @@ static int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
return 0;
-m64_failed:
- if (iov->m64_single_mode) {
- for (i = 0; i < num_vfs; i++) {
- if (iov->pe_num_map[i] == IODA_INVALID_PE)
- continue;
-
- pe = &phb->ioda.pe_array[iov->pe_num_map[i]];
- pnv_ioda_free_pe(pe);
- }
- } else
- bitmap_clear(phb->ioda.pe_alloc, *iov->pe_num_map, num_vfs);
+shift_failed:
+ pnv_pci_vf_release_m64(pdev, num_vfs);
- /* Releasing pe_num_map */
- kfree(iov->pe_num_map);
+m64_failed:
+ for (i = 0; i < num_vfs; i++)
+ pnv_ioda_free_pe(&iov->vf_pe_arr[i]);
return ret;
}
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 06431a452130..f76923f44f66 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -239,7 +239,12 @@ struct pnv_iov_data {
/* number of VFs enabled */
u16 num_vfs;
- unsigned int *pe_num_map; /* PE# for the first VF PE or array */
+
+ /*
+ * Pointer to the IODA PE state of each VF. Note that this is a pointer
+ * into the PHB's PE array (phb->ioda.pe_array).
+ */
+ struct pnv_ioda_pe *vf_pe_arr;
/* Did we map the VF BARs with single-PE IODA BARs? */
bool m64_single_mode;
--
2.26.2
^ permalink raw reply related
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