* Re: [PATCH 15/15] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
From: Alexey Kardashevskiy @ 2020-07-15 5:24 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200710052340.737567-16-oohall@gmail.com>
On 10/07/2020 15:23, Oliver O'Halloran wrote:
> 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>
> ---
> arch/powerpc/platforms/powernv/pci-sriov.c | 131 +++++++++++----------
> arch/powerpc/platforms/powernv/pci.h | 10 +-
> 2 files changed, 75 insertions(+), 66 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> index 8de03636888a..87377d95d648 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -146,10 +146,9 @@
> 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;
>
> @@ -158,9 +157,9 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
> goto disable_iov;
> pdev->dev.archdata.iov_data = iov;
>
> + /* FIXME: totalvfs > phb->ioda.total_pe_num is going to be a problem */
WARN_ON_ONCE() then?
> 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 +172,51 @@ 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 we've got a BAR that's bigger than greater than 1/4 of the
bigger, greater, huger? :)
Also, a nit: s/got a BAR/got a VF BAR/
> + * default window's segment size then switch to using single PE
> + * windows. This limits the total number of VFs we can support.
Just to get idea about absolute numbers here.
On my P9:
./pciex@600c3c0300000/ibm,opal-m64-window
00060200 00000000 00060200 00000000 00000040 00000000
so that default window's segment size is 0x40.0000.0000/512 = 512MB?
> *
> - * 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.
Why not use single PE mode for such BAR? Better than nothing.
> + */
> + 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;
> }
> +
> + // what should this be?
> iov->vfs_expanded = mul;
>
> return;
> @@ -260,42 +260,42 @@ 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;
>
> - /*
> - * 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);
> + int bar_no = resno - PCI_IOV_RESOURCES;
>
> /*
> * 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.
> + * 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;
> - if (iov->m64_single_mode)
> - return max(align, (resource_size_t)phb->ioda.m64_segsize);
>
> + 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[bar_no])
> + 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.
> + *
> + * 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.
> + */
> return iov->vfs_expanded * align;
> }
>
> @@ -453,7 +453,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;
> @@ -546,6 +546,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
> @@ -577,6 +579,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;
> @@ -622,8 +626,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 */
> @@ -741,9 +745,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 13555bc549f4..a78d1feb8fb8 100644
> --- a/arch/powerpc/platforms/powernv/pci.h
> +++ b/arch/powerpc/platforms/powernv/pci.h
> @@ -236,14 +236,20 @@ struct pnv_iov_data {
> /* number of VFs IOV BAR expanded. FIXME: rename this to something less bad */
> u16 vfs_expanded;
>
> + /*
> + * indicates if we need to move our IOV BAR to account for our
> + * allocated PE number when enabling VFs.
> + */
> + bool need_shift;
> +
> /* number of VFs enabled */
> u16 num_vfs;
>
> /* 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];
>
> /*
> * Bit mask used to track which m64 windows that we used to map the
>
--
Alexey
^ permalink raw reply
* Re: [v3 1/5] KVM: PPC: Book3S HV: Disable page merging in H_SVM_INIT_START
From: Ram Pai @ 2020-07-15 5:16 UTC (permalink / raw)
To: Bharata B Rao
Cc: ldufour, cclaudio, kvm-ppc, sathnaga, aneesh.kumar, sukadev,
linuxppc-dev, bauerman, david
In-Reply-To: <20200713052941.GF7902@in.ibm.com>
On Mon, Jul 13, 2020 at 10:59:41AM +0530, Bharata B Rao wrote:
> On Sat, Jul 11, 2020 at 02:13:43AM -0700, Ram Pai wrote:
> > Merging of pages associated with each memslot of a SVM is
> > disabled the page is migrated 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 in H_SVM_PAGE_IN handler.
> >
> > Disable page-migration in H_SVM_INIT_START handling.
>
> While it is a good idea to disable KSM merging for all VMAs during
> H_SVM_INIT_START, I am curious if you did observe an actual case of
> ksm_madvise() failing which resulted in subsequent H_SVM_PAGE_IN
> failing to migrate?
No. I did not find any ksm_madvise() failing. But it did not make sense
to ksm_madvise() everytime a page_in was requested. Hence i proposed
this patch. H_SVM_INIT_START is the right place for ksm_advise().
>
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> > arch/powerpc/kvm/book3s_hv_uvmem.c | 96 +++++++++++++++++++++++++++++---------
> > 1 file changed, 74 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> > index 3d987b1..bfc3841 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;
>
> This and other cases below seem to be a new return value from
> H_SVM_INIT_START. May be update the documentation too along with
> this patch?
ok.
>
> > +
> > + end = start + (memslot->npages << PAGE_SHIFT);
> > +
> > + down_write(&kvm->mm->mmap_sem);
>
> When you rebase the patches against latest upstream you may want to
> replace the above and other instances by mmap_write/read_lock().
ok.
>
> > + 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;
> > + } while (end > vma->vm_end);
> > +
> > + up_write(&kvm->mm->mmap_sem);
> > + 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;
> > +}
> > +
> > +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) {
> > if (kvmppc_uvmem_slot_init(kvm, memslot)) {
> > ret = H_PARAMETER;
> > - goto out;
> > + break;
> > }
> > ret = uv_register_mem_slot(kvm->arch.lpid,
> > memslot->base_gfn << PAGE_SHIFT,
> > @@ -245,9 +311,12 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> > if (ret < 0) {
> > kvmppc_uvmem_slot_free(kvm, memslot);
> > ret = H_PARAMETER;
> > - goto out;
> > + break;
> > }
> > }
> > +
> > + if (ret)
> > + kvmppc_enable_page_merge(kvm);
>
> Is there any use of enabling KSM merging in the failure path here?
> Won't UV terminate the VM if H_SVM_INIT_START fails? If there is no need,
> you can do away with some extra routines above.
UV will terminate it. But I did not want to tie that assumption into
this function.
>
> > out:
> > srcu_read_unlock(&kvm->srcu, srcu_idx);
> > return ret;
> > @@ -384,7 +453,7 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
> > */
> > static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
> > unsigned long end, unsigned long gpa, struct kvm *kvm,
> > - unsigned long page_shift, bool *downgrade)
> > + unsigned long page_shift)
> > {
> > unsigned long src_pfn, dst_pfn = 0;
> > struct migrate_vma mig;
> > @@ -400,18 +469,6 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
> > mig.src = &src_pfn;
> > mig.dst = &dst_pfn;
> >
> > - /*
> > - * We come here with mmap_sem write lock held just for
> > - * ksm_madvise(), otherwise we only need read mmap_sem.
> > - * Hence downgrade to read lock once ksm_madvise() is done.
> > - */
> > - ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> > - MADV_UNMERGEABLE, &vma->vm_flags);
>
> I haven't seen the subsequent patches yet, but guess you are
> taking care of disabling KSM mering for hot-plugged memory too.
No. This is a good catch. The hotplugged memory patch needs to disable
KSM aswell.
RP
^ permalink raw reply
* Re: [v3 4/5] KVM: PPC: Book3S HV: retry page migration before erroring-out H_SVM_PAGE_IN
From: Ram Pai @ 2020-07-15 5:09 UTC (permalink / raw)
To: Bharata B Rao
Cc: ldufour, cclaudio, kvm-ppc, sathnaga, aneesh.kumar, sukadev,
linuxppc-dev, bauerman, david
In-Reply-To: <20200713095043.GH7902@in.ibm.com>
On Mon, Jul 13, 2020 at 03:20:43PM +0530, Bharata B Rao wrote:
> On Sat, Jul 11, 2020 at 02:13:46AM -0700, Ram Pai wrote:
> > The page requested for page-in; sometimes, can have transient
> > references, and hence cannot migrate immediately. Retry a few times
> > before returning error.
>
> As I noted in the previous patch, we need to understand what are these
> transient errors and they occur on what type of pages?
Its not clear when they occur. But they occur quite irregularly and they
do occur on pages that were shared in the previous boot.
>
> The previous patch also introduced a bit of retry logic in the
> page-in path. Can you consolidate the retry logic into a separate
> patch?
yes. having the retry logic in a seperate patch gives us the flexibility
to drop it, if needed. The retry patch is not the core element of this
patch series.
RP
^ permalink raw reply
* Re: [v3 3/5] KVM: PPC: Book3S HV: migrate remaining normal-GFNs to secure-GFNs in H_SVM_INIT_DONE
From: Ram Pai @ 2020-07-15 5:05 UTC (permalink / raw)
To: Bharata B Rao
Cc: ldufour, cclaudio, kvm-ppc, sathnaga, aneesh.kumar, sukadev,
linuxppc-dev, bauerman, david
In-Reply-To: <20200713094506.GG7902@in.ibm.com>
On Mon, Jul 13, 2020 at 03:15:06PM +0530, Bharata B Rao wrote:
> On Sat, Jul 11, 2020 at 02:13:45AM -0700, Ram Pai wrote:
> > The Ultravisor is expected to explicitly call H_SVM_PAGE_IN for all the pages
> >
> > if (!(*mig.src & MIGRATE_PFN_MIGRATE)) {
> > - ret = -1;
> > + ret = -2;
>
> migrate_vma_setup() has marked that this pfn can't be migrated. What
> transient errors are you observing which will disappear within 10
> retries?
>
> Also till now when UV used to pull in all the pages, we never seemed to
> have hit these transient errors. But now when HV is pushing the same
> pages, we see these errors which are disappearing after 10 retries.
> Can you explain this more please? What sort of pages are these?
We did see them even before this patch. The retry alleviates the
problem, but does not entirely eliminate it. If the chance of seeing
the issue without the patch is 1%, the chance of seeing this issue
with this patch becomes 0.25%.
>
> > goto out_finalize;
> > }
> > + bool retry = 0;
...snip...
> > +
> > + *ret = 0;
> > + while (kvmppc_next_nontransitioned_gfn(memslot, kvm, &gfn)) {
> > +
> > + down_write(&kvm->mm->mmap_sem);
>
> Acquiring and releasing mmap_sem in a loop? Any reason?
>
> Now that you have moved ksm_madvise() calls to init time, any specific
> reason to take write mmap_sem here?
The semaphore protects the vma. right?
And its acquired/released in the loop, to provide the ability to relinquish
the cpu without getting into a tight loop and cause softlockups.
>
> > + start = gfn_to_hva(kvm, gfn);
> > + if (kvm_is_error_hva(start)) {
> > + *ret = H_STATE;
> > + goto next;
> > + }
> > +
> > + end = start + (1UL << PAGE_SHIFT);
> > + vma = find_vma_intersection(kvm->mm, start, end);
> > + if (!vma || vma->vm_start > start || vma->vm_end < end) {
> > + *ret = H_STATE;
> > + goto next;
> > + }
> > +
> > + mutex_lock(&kvm->arch.uvmem_lock);
> > + *ret = kvmppc_svm_migrate_page(vma, start, end,
> > + (gfn << PAGE_SHIFT), kvm, PAGE_SHIFT, false);
> > + mutex_unlock(&kvm->arch.uvmem_lock);
> > +
> > +next:
> > + up_write(&kvm->mm->mmap_sem);
> > +
> > + if (*ret == -2) {
> > + retry = 1;
>
> Using true or false assignment makes it easy to understand the intention
> of this 'retry' variable.
ok. next version will have that change.
Thanks for the comments,
RP
^ permalink raw reply
* Re: [PATCH 12/15] powerpc/powernv/sriov: De-indent setup and teardown
From: Alexey Kardashevskiy @ 2020-07-15 4:58 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <CAOSf1CGdGQ0cFNmfL=gVNcqX+JhT_W=73UWYd6aVourvtEs4ow@mail.gmail.com>
On 15/07/2020 14:46, Oliver O'Halloran wrote:
> On Wed, Jul 15, 2020 at 2:41 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>
>>
>> On 15/07/2020 14:21, Oliver O'Halloran wrote:
>>> On Wed, Jul 15, 2020 at 2:00 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>
>>>>
>>>> or we could just skip setting
>>>>
>>>> ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable;
>>>>
>>>> for uninteresting platforms in pnv_pci_init_ioda_phb().
>>>
>>> I don't think so. ppc_md is per-platform, not per-PHB andw e still
>>> have to deal with a mixture of IODA/NVLink/OpenCAPI PHBs on a single
>>> system.
>>
>> NVLink/OpenCAPI won't have SRIOV devices.
>
> ...OR WILL THEY?
NO!
>> Other types won't appear on
>> the same platform simultaneously. It is not too clean, yes.
>
> Sure, my point is that's a per-PHB setting rather than a per-platform
> one so we should set it up like that.
and my point is that you did too good job getting rid of IODA1 vs IODA2
checks to keep this check. But ok.
>
>>> We could make it a callback in pnv_phb, but it seemed like
>>> more indirection than it's worth.
>>
>> I genuinely dislike how we use ppc_md so removing things from it is
>> definitely a good thing.
>
> you wouldn't be able to get rid of it. We'd have something like what
> we have for the existing pcibios calls where there's a "generic" one
> that bounces it to a member of pci_controller_ops, which then bounces
> it to the pnv_phb method. It's bad and I hate it.
No argument here...
--
Alexey
^ permalink raw reply
* Re: [PATCH 14/15] powerpc/powernv/sriov: Refactor M64 BAR setup
From: Alexey Kardashevskiy @ 2020-07-15 4:50 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200710052340.737567-15-oohall@gmail.com>
On 10/07/2020 15:23, Oliver O'Halloran wrote:
> 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>
> ---
> This patch could be folded into the previous one. I've kept it
> seperate mainly because the diff is *horrific* when they're merged.
> ---
> 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 2f967aa4fbf5..8de03636888a 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -441,52 +441,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_accordion(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_accordion(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;
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 12/15] powerpc/powernv/sriov: De-indent setup and teardown
From: Oliver O'Halloran @ 2020-07-15 4:46 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: linuxppc-dev
In-Reply-To: <f97edf44-0029-5a33-0cff-7067a81f10ba@ozlabs.ru>
On Wed, Jul 15, 2020 at 2:41 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
>
> On 15/07/2020 14:21, Oliver O'Halloran wrote:
> > On Wed, Jul 15, 2020 at 2:00 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>
> >>
> >> or we could just skip setting
> >>
> >> ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable;
> >>
> >> for uninteresting platforms in pnv_pci_init_ioda_phb().
> >
> > I don't think so. ppc_md is per-platform, not per-PHB andw e still
> > have to deal with a mixture of IODA/NVLink/OpenCAPI PHBs on a single
> > system.
>
> NVLink/OpenCAPI won't have SRIOV devices.
...OR WILL THEY?
> Other types won't appear on
> the same platform simultaneously. It is not too clean, yes.
Sure, my point is that's a per-PHB setting rather than a per-platform
one so we should set it up like that.
> > We could make it a callback in pnv_phb, but it seemed like
> > more indirection than it's worth.
>
> I genuinely dislike how we use ppc_md so removing things from it is
> definitely a good thing.
you wouldn't be able to get rid of it. We'd have something like what
we have for the existing pcibios calls where there's a "generic" one
that bounces it to a member of pci_controller_ops, which then bounces
it to the pnv_phb method. It's bad and I hate it.
>
>
> --
> Alexey
^ permalink raw reply
* Re: [PATCH 12/15] powerpc/powernv/sriov: De-indent setup and teardown
From: Alexey Kardashevskiy @ 2020-07-15 4:41 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <CAOSf1CGbDWFzuh5pD3CYayTXOFsw18E0kFn052xtxrdes_2_zw@mail.gmail.com>
On 15/07/2020 14:21, Oliver O'Halloran wrote:
> On Wed, Jul 15, 2020 at 2:00 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>
>> or we could just skip setting
>>
>> ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable;
>>
>> for uninteresting platforms in pnv_pci_init_ioda_phb().
>
> I don't think so. ppc_md is per-platform, not per-PHB andw e still
> have to deal with a mixture of IODA/NVLink/OpenCAPI PHBs on a single
> system.
NVLink/OpenCAPI won't have SRIOV devices. Other types won't appear on
the same platform simultaneously. It is not too clean, yes.
> We could make it a callback in pnv_phb, but it seemed like
> more indirection than it's worth.
I genuinely dislike how we use ppc_md so removing things from it is
definitely a good thing.
--
Alexey
^ permalink raw reply
* Re: [PATCH 12/15] powerpc/powernv/sriov: De-indent setup and teardown
From: Oliver O'Halloran @ 2020-07-15 4:21 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: linuxppc-dev
In-Reply-To: <ece39d29-6ab0-dcf4-0561-3c488c7921f9@ozlabs.ru>
On Wed, Jul 15, 2020 at 2:00 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
> or we could just skip setting
>
> ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable;
>
> for uninteresting platforms in pnv_pci_init_ioda_phb().
I don't think so. ppc_md is per-platform, not per-PHB andw e still
have to deal with a mixture of IODA/NVLink/OpenCAPI PHBs on a single
system. We could make it a callback in pnv_phb, but it seemed like
more indirection than it's worth.
^ permalink raw reply
* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Oliver O'Halloran @ 2020-07-15 4:18 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Greg Kroah-Hartman, linux-pci, bjorn, Paul Mackerras, sparclinux,
Toan Le, Christoph Hellwig, Marek Vasut, Rob Herring,
Lorenzo Pieralisi, Sagi Grimberg, Kevin Hilman, Russell King,
Ley Foon Tan, Greg Ungerer, Geert Uytterhoeven, Jakub Kicinski,
Matt Turner, linux-kernel-mentees, Guenter Roeck, Bjorn Helgaas,
Ray Jui, linuxppc-dev, Jens Axboe, Ivan Kokshaysky, Shuah Khan,
Keith Busch, Boris Ostrovsky, Richard Henderson, Juergen Gross,
Thomas Bogendoerfer, Scott Branden, Jingoo Han,
linux-kernel@vger.kernel.org, Philipp Zabel, Saheed O. Bolarinwa,
Gustavo Pimentel, Bjorn Helgaas, David S. Miller, Heiner Kallweit
In-Reply-To: <CAK8P3a3EZX8=649R9cYF6_=ivh1Xyrgsc5mUtS=d5yvQ3doZaQ@mail.gmail.com>
On Wed, Jul 15, 2020 at 8:03 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> - Most error checking is static: PCIBIOS_BAD_REGISTER_NUMBER
> only happens if you pass an invalid register number, but most
> callers pass a compile-time constant register number that is
> known to be correct, or the driver would never work. Similarly,
> PCIBIOS_DEVICE_NOT_FOUND wouldn't normally happen
> since you pass a valid pci_device pointer that was already
> probed.
Having some feedback about obvious programming errors is still useful
when doing driver development. Reporting those via printk() would
probably be more useful to those who care though.
> - config space accesses are very rare compared to memory
> space access and on the hardware side the error handling
> would be similar, but readl/writel don't return errors, they just
> access wrong registers or return 0xffffffff.
> arch/powerpc/kernel/eeh.c has a ton extra code written to
> deal with it, but no other architectures do.
TBH the EEH MMIO hooks were probably a mistake to begin with. Errors
detected via MMIO are almost always asynchronous to the error itself
so you usually just wind up with a misleading stack trace rather than
any kind of useful synchronous error reporting. It seems like most
drivers don't bother checking for 0xFFs either and rely on the
asynchronous reporting via .error_detected() instead, so I have to
wonder what the point is. I've been thinking of removing the MMIO
hooks and using a background poller to check for errors on each PHB
periodically (assuming we don't have an EEH interrupt) instead. That
would remove the requirement for eeh_dev_check_failure() to be
interrupt safe too, so it might even let us fix all the godawful races
in EEH.
> - If we add code to detect errors in pci_read_config_*
> and do some of the stuff from powerpc's
> eeh_dev_check_failure(), we are more likely to catch
> intermittent failures when drivers don't check, or bugs
> with invalid arguments in device drivers than relying on
> drivers to get their error handling right when those code
> paths don't ever get covered in normal testing.
Adding some kind of error detection to the generic config accessors
wouldn't hurt, but detection is only half the problem. The main job of
eeh_dev_check_failure() is waking up the EEH recovery thread which
actually handles notifying drivers, device resets, etc and you'd want
something in the PCI core. Right now there's two implementations of
that reporting logic: one for EEH in arch/powerpc/eeh_driver.c and one
for AER/DPC in drivers/pci/pcie/err.c. I think the latter could be
moved into the PCI core easily enough since there's not much about it
that's really specific to PCIe. Ideally we could drop the EEH specific
one too, but I'm not sure how to implement that without it devolving
into callback spaghetti.
Oliver
^ permalink raw reply
* Re: [PATCH 3/3] ASoC: fsl-asoc-card: Support Headphone and Microphone Jack detection
From: Shengjiu Wang @ 2020-07-15 4:14 UTC (permalink / raw)
To: Nicolin Chen
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, kuninori.morimoto.gx, Timur Tabi, samuel, katsuhiro,
Fabio Estevam, Shengjiu Wang, Xiubo Li, linux-kernel,
Liam Girdwood, Rob Herring, Mark Brown, Takashi Iwai,
linuxppc-dev
In-Reply-To: <20200714211432.GA10818@Asurada-Nvidia>
On Wed, Jul 15, 2020 at 5:16 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> Hi Shengjiu,
>
> The whole series looks good to me. Just a couple of small
> questions inline:
>
> On Tue, Jul 14, 2020 at 05:05:36PM +0800, Shengjiu Wang wrote:
> > Use asoc_simple_init_jack function from simple card to implement
> > the Headphone and Microphone detection.
> > Register notifier to disable Speaker when Headphone is plugged in
> > and enable Speaker when Headphone is unplugged.
> > Register notifier to disable Digital Microphone when Analog Microphone
> > is plugged in and enable DMIC when Analog Microphone is unplugged.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > sound/soc/fsl/Kconfig | 1 +
> > sound/soc/fsl/fsl-asoc-card.c | 69 ++++++++++++++++++++++++++++++++++-
> > 2 files changed, 68 insertions(+), 2 deletions(-)
>
> > static int fsl_asoc_card_late_probe(struct snd_soc_card *card)
> > {
> > struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(card);
> > @@ -745,8 +789,29 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
> > snd_soc_card_set_drvdata(&priv->card, priv);
> >
> > ret = devm_snd_soc_register_card(&pdev->dev, &priv->card);
> > - if (ret && ret != -EPROBE_DEFER)
> > - dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
> > + if (ret) {
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
>
> I think we may move this EPROBE_DEFER to the asrc_fail label.
If we move this to asrc_fail label, then it will be hard to define the
error message.
There are many places that goto asrc_fail.
>
> > + goto asrc_fail;
> > + }
> > +
> > + if (of_property_read_bool(np, "hp-det-gpio")) {
>
> Could we move this check inside asoc_simple_init_jack? There's no
> problem with doing it here though, yet I got a bit confused by it
> as I thought it's a boolean type property, which would be against
> the DT bindings until I saw asoc_simple_init_jack() uses the same
> string to get the GPIO. Just it probably would be a bit tricky as
> we need it to be optional here.
>
> Otherwise, I think we may add a line of comments to indicate that
> the API would use the same string to get the GPIO.
In asoc_simple_init_jack, gpio_is_valid() will be invalid when there is
no "hp-det-gpio" property, and asoc_simple_init_jack will return 0.
The reason why I add a check here is mostly for
snd_soc_jack_notifier_register().
when there is no jack created, there will be a kernel dump.
or I can use this code:
- if (of_property_read_bool(np, "hp-det-gpio")) {
- ret = asoc_simple_init_jack(&priv->card, &priv->hp_jack,
- 1, NULL, "Headphone Jack");
- if (ret)
- goto asrc_fail;
+ ret = asoc_simple_init_jack(&priv->card, &priv->hp_jack,
+ 1, NULL, "Headphone Jack");
+ if (ret)
+ goto asrc_fail;
+ if (priv->hp_jack.jack.jack)
snd_soc_jack_notifier_register(&priv->hp_jack.jack,
&hp_jack_nb);
- }
what do you think?
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH v3 2/9] powerpc/watchpoint: Fix DAWR exception constraint
From: Ravi Bangoria @ 2020-07-15 4:08 UTC (permalink / raw)
To: Jordan Niethe
Cc: Christophe Leroy, apopple, mikey, miltonm, peterz, oleg,
Nicholas Piggin, linux-kernel, Paul Mackerras, jolsa, fweisbec,
pedromfc, naveen.n.rao, linuxppc-dev, mingo, Ravi Bangoria
In-Reply-To: <CACzsE9qSjWKWGDQPGSk-c5f5pxUyWWtUFW+AzzB5M76qFcQ-Cw@mail.gmail.com>
Hi Jordan,
>> @@ -536,7 +538,12 @@ static bool check_dawrx_constraints(struct pt_regs *regs, int type,
>> if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
>> return false;
>>
>> - if (OP_IS_STORE(type) && !(info->type & HW_BRK_TYPE_WRITE))
>> + /*
>> + * The Cache Management instructions other than dcbz never
>> + * cause a match. i.e. if type is CACHEOP, the instruction
>> + * is dcbz, and dcbz is treated as Store.
>> + */
>> + if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
>> return false;
> This change seems seperate to this commit?
I also thought about it but was not sure. See below ...
>>
>> if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
>> @@ -553,7 +560,8 @@ static bool check_dawrx_constraints(struct pt_regs *regs, int type,
>> * including extraneous exception. Otherwise return false.
>> */
>> static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
>> - int type, int size, struct arch_hw_breakpoint *info)
>> + unsigned long ea, int type, int size,
>> + struct arch_hw_breakpoint *info)
>> {
>> bool in_user_range = dar_in_user_range(regs->dar, info);
>> bool dawrx_constraints;
>> @@ -569,11 +577,10 @@ static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
>> }
>>
>> if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
>> - if (in_user_range)
>> - return true;
>> -
>> - if (dar_in_hw_range(regs->dar, info)) {
>> - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> + if (dar_in_hw_range(regs->dar, info))
>> + return true;
>> + } else {
>> return true;
> I think this would be clearer as:
> if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> !(dar_in_hw_range(regs->dar, info)))
> return false;
> else
> return true;
ok
>
>> }
>> return false;
>> @@ -581,10 +588,20 @@ static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
>>
>> dawrx_constraints = check_dawrx_constraints(regs, type, info);
>>
>> - if (dar_user_range_overlaps(regs->dar, size, info))
>> + if (type == UNKNOWN) {
>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> + if (dar_in_hw_range(regs->dar, info))
>> + return dawrx_constraints;
>> + } else {
>> + return dawrx_constraints;
>> + }
>> + return false;
>> + }
> Similar thing here, it could be:
> if ((cpu_has_feature(CPU_FTR_ARCH_31)) &&
> !(dar_in_hw_range(regs->dar, info)))
> return false;
> else
> return dawrx_constraints;
ok
>> +
>> + if (ea_user_range_overlaps(ea, size, info))
>> return dawrx_constraints;
>>
>> - if (dar_hw_range_overlaps(regs->dar, size, info)) {
>> + if (ea_hw_range_overlaps(ea, size, info)) {
>> if (dawrx_constraints) {
>> info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
>> return true;
>> @@ -593,8 +610,17 @@ static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
>> return false;
>> }
>>
>> +static int cache_op_size(void)
>> +{
>> +#ifdef __powerpc64__
>> + return ppc64_caches.l1d.block_size;
>> +#else
>> + return L1_CACHE_BYTES;
>> +#endif
>> +}
>> +
>> static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
>> - int *type, int *size, bool *larx_stcx)
>> + int *type, int *size, unsigned long *ea)
>> {
>> struct instruction_op op;
>>
>> @@ -602,16 +628,23 @@ static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
>> return;
>>
>> analyse_instr(&op, regs, *instr);
>> -
>> - /*
>> - * Set size = 8 if analyse_instr() fails. If it's a userspace
>> - * watchpoint(valid or extraneous), we can notify user about it.
>> - * If it's a kernel watchpoint, instruction emulation will fail
>> - * in stepping_handler() and watchpoint will be disabled.
>> - */
>> *type = GETTYPE(op.type);
>> - *size = !(*type == UNKNOWN) ? GETSIZE(op.type) : 8;
>> - *larx_stcx = (*type == LARX || *type == STCX);
>> + *ea = op.ea;
>> +#ifdef __powerpc64__
>> + if (!(regs->msr & MSR_64BIT))
>> + *ea &= 0xffffffffUL;
>> +#endif
>> +
>> + *size = GETSIZE(op.type);
>> + if (*type == CACHEOP) {
>> + *size = cache_op_size();
>> + *ea &= ~(*size - 1);
>> + }
> Again related to CACHEOP, should these changes be mentioned in the
> commit message?
For CACHEOP, ea returned by analyse_instr() needs to be aligned down to cache
block size manually. Also, for CACHEOP, size returned by analyse_instr() is 0
and thus size also needs to be calculated manually. This was missed in
27985b2a640e. So it kind of relates to other changes of the patch but needs
special treatment as well. Will see if I can split it.
Thanks for the review,
Ravi
^ permalink raw reply
* Re: [PATCH 13/15] powerpc/powernv/sriov: Move M64 BAR allocation into a helper
From: Alexey Kardashevskiy @ 2020-07-15 4:02 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200710052340.737567-14-oohall@gmail.com>
On 10/07/2020 15:23, Oliver O'Halloran wrote:
> 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>
> ---
> 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 d5699cd2ab7a..2f967aa4fbf5 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -416,6 +416,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;
> @@ -443,17 +460,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;
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 12/15] powerpc/powernv/sriov: De-indent setup and teardown
From: Alexey Kardashevskiy @ 2020-07-15 4:00 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200710052340.737567-13-oohall@gmail.com>
On 10/07/2020 15:23, Oliver O'Halloran wrote:
> 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>
> ---
> arch/powerpc/platforms/powernv/pci-sriov.c | 86 ++++++++++++----------
> 1 file changed, 49 insertions(+), 37 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> index 08f88187d65a..d5699cd2ab7a 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -610,16 +610,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)
> @@ -693,41 +695,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;
> + }
or we could just skip setting
ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable;
for uninteresting platforms in pnv_pci_init_ioda_phb().
>
> - /* 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);
This can be a single line now. Thanks,
> + if (ret)
> + goto shift_failed;
> }
>
> /* Setup VF PEs */
>
--
Alexey
^ permalink raw reply
* Re: [PATCH] powerpc: Fix inconsistent function names
From: Michael Ellerman @ 2020-07-15 3:54 UTC (permalink / raw)
To: YueHaibing, benh, paulus, haren, dave.hansen, npiggin
Cc: YueHaibing, linuxppc-dev, linux-kernel
In-Reply-To: <20200715031939.31968-1-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> writes:
> The stub helpers name should be consistent with prototypes.
>
> mm_context_add_vas_windows() --> mm_context_add_vas_window()
> mm_context_remove_vas_windows() --> mm_context_remove_vas_window()
>
> Fixes: c420644c0a8f ("powerpc: Use mm_context vas_windows counter to issue CP_ABORT")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> arch/powerpc/include/asm/mmu_context.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> index 1a474f6b1992..00fd1d44731a 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -218,8 +218,8 @@ static inline void inc_mm_active_cpus(struct mm_struct *mm) { }
> static inline void dec_mm_active_cpus(struct mm_struct *mm) { }
> static inline void mm_context_add_copro(struct mm_struct *mm) { }
> static inline void mm_context_remove_copro(struct mm_struct *mm) { }
> -static inline void mm_context_add_vas_windows(struct mm_struct *mm) { }
> -static inline void mm_context_remove_vas_windows(struct mm_struct *mm) { }
> +static inline void mm_context_add_vas_window(struct mm_struct *mm) { }
> +static inline void mm_context_remove_vas_window(struct mm_struct *mm) { }
> #endif
Both of those functions are only called from 64-bit only code, so the
stubs should not be needed at all. Which explains why we haven't seen a
build break.
So just dropping them would be better IMO.
cheers
^ permalink raw reply
* Re: [PATCH 05/13] cpufreq/arch: powerpc: pasemi: Move prototypes to shared header
From: Viresh Kumar @ 2020-07-15 3:51 UTC (permalink / raw)
To: Olof Johansson
Cc: linux-pm, linuxppc-dev, Rafael J. Wysocki,
Linux Kernel Mailing List, Paul Mackerras, Lee Jones,
Linux ARM Mailing List
In-Reply-To: <CAOesGMi1dfqPbFJ8YoUoJ75NdU1=XiNoYx+6+JLu44a4LuuYGA@mail.gmail.com>
On 14-07-20, 20:49, Olof Johansson wrote:
> On Tue, Jul 14, 2020 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 14-07-20, 15:50, Lee Jones wrote:
> > > If function callers and providers do not share the same prototypes the
> > > compiler complains of missing prototypes. Fix this by moving the
> > > already existing prototypes out to a mutually convenient location.
> > >
> > > Fixes the following W=1 kernel build warning(s):
> > >
> > > drivers/cpufreq/pasemi-cpufreq.c:109:5: warning: no previous prototype for ‘check_astate’ [-Wmissing-prototypes]
> > > 109 | int check_astate(void)
> > > | ^~~~~~~~~~~~
> > > drivers/cpufreq/pasemi-cpufreq.c:114:6: warning: no previous prototype for ‘restore_astate’ [-Wmissing-prototypes]
> > > 114 | void restore_astate(int cpu)
> > > | ^~~~~~~~~~~~~~
> > >
> > > Cc: Olof Johansson <olof@lixom.net>
> > > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Cc: Paul Mackerras <paulus@samba.org>
> > > Cc: linuxppc-dev@lists.ozlabs.org
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > > arch/powerpc/platforms/pasemi/pasemi.h | 15 ------------
> >
> > Is there no sane way we can include this file directly to the cpufreq
> > file ?
>
> Yep. arch/powerpc seems to be in the search path for modules on powerpc, so:
>
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
> index c66f566a854cb..815645170c4de 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -22,6 +22,8 @@
> #include <asm/time.h>
> #include <asm/smp.h>
>
> +#include <platforms/pasemi/pasemi.h>
> +
> #define SDCASR_REG 0x0100
> #define SDCASR_REG_STRIDE 0x1000
> #define SDCPWR_CFGA0_REG 0x0100
Fantastic. Thanks.
--
viresh
^ permalink raw reply
* Re: [PATCH v3 05/12] powerpc/drmem: make lmb walk a bit more flexible
From: Thiago Jung Bauermann @ 2020-07-15 3:50 UTC (permalink / raw)
To: Hari Bathini
Cc: Pingfan Liu, Nayna Jain, Kexec-ml, Mahesh J Salgaonkar,
Mimi Zohar, lkml, linuxppc-dev, Sourabh Jain, Petr Tesarik,
Andrew Morton, Dave Young, Vivek Goyal, Eric Biederman
In-Reply-To: <159466090332.24747.9255471295044653085.stgit@hbathini.in.ibm.com>
Hari Bathini <hbathini@linux.ibm.com> writes:
> @@ -534,7 +537,7 @@ static int __init early_init_dt_scan_memory_ppc(unsigned long node,
> #ifdef CONFIG_PPC_PSERIES
> if (depth == 1 &&
> strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
> - walk_drmem_lmbs_early(node, early_init_drmem_lmb);
> + walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
walk_drmem_lmbs_early() can now fail. Should this failure be propagated
as a return value of early_init_dt_scan_memory_ppc()?
> return 0;
> }
> #endif
<snip>
> @@ -787,7 +790,7 @@ static int __init parse_numa_properties(void)
> */
> memory = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> if (memory) {
> - walk_drmem_lmbs(memory, numa_setup_drmem_lmb);
> + walk_drmem_lmbs(memory, NULL, numa_setup_drmem_lmb);
Similarly here. Now that this call can fail, should
parse_numa_properties() handle or propagate the failure?
> of_node_put(memory);
> }
>
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH 05/13] cpufreq/arch: powerpc: pasemi: Move prototypes to shared header
From: Olof Johansson @ 2020-07-15 3:49 UTC (permalink / raw)
To: Viresh Kumar
Cc: linux-pm, linuxppc-dev, Rafael J. Wysocki,
Linux Kernel Mailing List, Paul Mackerras, Lee Jones,
Linux ARM Mailing List
In-Reply-To: <20200715030706.prxya7fyylscoy25@vireshk-i7>
On Tue, Jul 14, 2020 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 14-07-20, 15:50, Lee Jones wrote:
> > If function callers and providers do not share the same prototypes the
> > compiler complains of missing prototypes. Fix this by moving the
> > already existing prototypes out to a mutually convenient location.
> >
> > Fixes the following W=1 kernel build warning(s):
> >
> > drivers/cpufreq/pasemi-cpufreq.c:109:5: warning: no previous prototype for ‘check_astate’ [-Wmissing-prototypes]
> > 109 | int check_astate(void)
> > | ^~~~~~~~~~~~
> > drivers/cpufreq/pasemi-cpufreq.c:114:6: warning: no previous prototype for ‘restore_astate’ [-Wmissing-prototypes]
> > 114 | void restore_astate(int cpu)
> > | ^~~~~~~~~~~~~~
> >
> > Cc: Olof Johansson <olof@lixom.net>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> > arch/powerpc/platforms/pasemi/pasemi.h | 15 ------------
>
> Is there no sane way we can include this file directly to the cpufreq
> file ?
Yep. arch/powerpc seems to be in the search path for modules on powerpc, so:
diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
index c66f566a854cb..815645170c4de 100644
--- a/drivers/cpufreq/pasemi-cpufreq.c
+++ b/drivers/cpufreq/pasemi-cpufreq.c
@@ -22,6 +22,8 @@
#include <asm/time.h>
#include <asm/smp.h>
+#include <platforms/pasemi/pasemi.h>
+
#define SDCASR_REG 0x0100
#define SDCASR_REG_STRIDE 0x1000
#define SDCPWR_CFGA0_REG 0x0100
-Olof
^ permalink raw reply related
* Re: [PATCH -next] cpufreq: powernv: Make some symbols static
From: Viresh Kumar @ 2020-07-15 3:41 UTC (permalink / raw)
To: Wei Yongjun, lee.jones
Cc: Hulk Robot, linux-pm, Rafael J. Wysocki, linuxppc-dev
In-Reply-To: <20200714142355.29819-1-weiyongjun1@huawei.com>
On 14-07-20, 22:23, Wei Yongjun wrote:
> The sparse tool complains as follows:
>
> drivers/cpufreq/powernv-cpufreq.c:88:1: warning:
> symbol 'pstate_revmap' was not declared. Should it be static?
> drivers/cpufreq/powernv-cpufreq.c:383:18: warning:
> symbol 'cpufreq_freq_attr_cpuinfo_nominal_freq' was not declared. Should it be static?
> drivers/cpufreq/powernv-cpufreq.c:669:6: warning:
> symbol 'gpstate_timer_handler' was not declared. Should it be static?
> drivers/cpufreq/powernv-cpufreq.c:902:6: warning:
> symbol 'powernv_cpufreq_work_fn' was not declared. Should it be static?
>
> Those symbols are not used outside of this file, so mark
> them static.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/cpufreq/powernv-cpufreq.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Lee also sent a fix for this, but yours look complete :)
https://lore.kernel.org/lkml/20200714145049.2496163-7-lee.jones@linaro.org/
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH 03/15] powerpc/powernv/pci: Add explicit tracking of the DMA setup state
From: Alexey Kardashevskiy @ 2020-07-15 3:33 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev, Cédric Le Goater
In-Reply-To: <CAOSf1CGhc1i64_DS18evMhwoXvMWyafOk3r2czUz49F6gERtfw@mail.gmail.com>
On 15/07/2020 11:38, Oliver O'Halloran wrote:
> On Tue, Jul 14, 2020 at 5:21 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>> On 14/07/2020 15:58, Oliver O'Halloran wrote:
>>> On Tue, Jul 14, 2020 at 3:37 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>
>>>> On 10/07/2020 15:23, Oliver O'Halloran wrote:
>>>>> This also means the only remaining user of the old "DMA Weight" code is
>>>>> the IODA1 DMA setup code that it was originally added for, which is good.
>>>>
>>>>
>>>> Is ditching IODA1 in the plan? :)
>>>
>>> That or separating out the pci_controller_ops for IODA1 and IODA2 so
>>> we can stop any IODA2 specific changes from breaking it.
>>
>> Is IODA1 tested at all these days? Or, is anyone running upstream
>> kernels anywhere and keeps shouting when it does not work on IODA1? Thanks,
>
> Cedric has a P7 with OPAL. That's probably the one left though.
Has he tried these patches on that box? Or we hope for the best here? :)
--
Alexey
^ permalink raw reply
* Re: [PATCH 11/15] powerpc/powernv/sriov: Drop iov->pe_num_map[]
From: Alexey Kardashevskiy @ 2020-07-15 3:31 UTC (permalink / raw)
To: Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20200710052340.737567-12-oohall@gmail.com>
On 10/07/2020 15:23, Oliver O'Halloran wrote:
> 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
s/contigious/contiguous/ here and below.
> 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.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> arch/powerpc/platforms/powernv/pci-sriov.c | 109 +++++----------------
> arch/powerpc/platforms/powernv/pci.h | 4 +-
> 2 files changed, 25 insertions(+), 88 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> index d53a85ccb538..08f88187d65a 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -456,11 +456,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 {
> @@ -599,38 +601,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);
> }
> }
>
> @@ -656,13 +644,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;
> @@ -670,6 +652,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);
> @@ -701,9 +684,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;
>
> @@ -717,55 +700,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 */
> @@ -781,9 +723,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;
> }
> }
>
> @@ -792,20 +735,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 b4c9bdba7217..13555bc549f4 100644
> --- a/arch/powerpc/platforms/powernv/pci.h
> +++ b/arch/powerpc/platforms/powernv/pci.h
> @@ -238,7 +238,9 @@ 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 array of VF PEs. num_vfs long*/
I read the comment and for a second I thought that now you are storing
pnv_ioda_pe structs in pnv_iov_data which is not true: vf_pe_arr
actually points inside phb->ioda.pe_array[]. May be add this to the
comment please.
Otherwise good,
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> + struct pnv_ioda_pe *vf_pe_arr;
>
> /* Did we map the VF BARs with single-PE IODA BARs? */
> bool m64_single_mode;
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 05/13] cpufreq/arch: powerpc: pasemi: Move prototypes to shared header
From: Olof Johansson @ 2020-07-15 3:26 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pm, Rafael J. Wysocki, Linux Kernel Mailing List,
Paul Mackerras, viresh kumar, linuxppc-dev,
Linux ARM Mailing List
In-Reply-To: <20200714145049.2496163-6-lee.jones@linaro.org>
On Tue, Jul 14, 2020 at 7:50 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> If function callers and providers do not share the same prototypes the
> compiler complains of missing prototypes. Fix this by moving the
> already existing prototypes out to a mutually convenient location.
>
> Fixes the following W=1 kernel build warning(s):
>
> drivers/cpufreq/pasemi-cpufreq.c:109:5: warning: no previous prototype for ‘check_astate’ [-Wmissing-prototypes]
> 109 | int check_astate(void)
> | ^~~~~~~~~~~~
> drivers/cpufreq/pasemi-cpufreq.c:114:6: warning: no previous prototype for ‘restore_astate’ [-Wmissing-prototypes]
> 114 | void restore_astate(int cpu)
> | ^~~~~~~~~~~~~~
>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> arch/powerpc/platforms/pasemi/pasemi.h | 15 ------------
> arch/powerpc/platforms/pasemi/powersave.S | 2 ++
> drivers/cpufreq/pasemi-cpufreq.c | 1 +
> include/linux/platform_data/pasemi.h | 28 +++++++++++++++++++++++
> 4 files changed, 31 insertions(+), 15 deletions(-)
> create mode 100644 include/linux/platform_data/pasemi.h
>
> diff --git a/arch/powerpc/platforms/pasemi/pasemi.h b/arch/powerpc/platforms/pasemi/pasemi.h
> index 70b56048ed1be..528d81ef748ad 100644
> --- a/arch/powerpc/platforms/pasemi/pasemi.h
> +++ b/arch/powerpc/platforms/pasemi/pasemi.h
> @@ -15,21 +15,6 @@ extern void __init pasemi_map_registers(void);
> extern void idle_spin(void);
> extern void idle_doze(void);
>
> -/* Restore astate to last set */
> -#ifdef CONFIG_PPC_PASEMI_CPUFREQ
> -extern int check_astate(void);
> -extern void restore_astate(int cpu);
> -#else
> -static inline int check_astate(void)
> -{
> - /* Always return >0 so we never power save */
> - return 1;
> -}
> -static inline void restore_astate(int cpu)
> -{
> -}
> -#endif
> -
> extern struct pci_controller_ops pasemi_pci_controller_ops;
>
> #endif /* _PASEMI_PASEMI_H */
> diff --git a/arch/powerpc/platforms/pasemi/powersave.S b/arch/powerpc/platforms/pasemi/powersave.S
> index d0215d5329ca7..7747b48963286 100644
> --- a/arch/powerpc/platforms/pasemi/powersave.S
> +++ b/arch/powerpc/platforms/pasemi/powersave.S
> @@ -5,6 +5,8 @@
> * Maintained by: Olof Johansson <olof@lixom.net>
> */
>
> +#include <linux/platform_data/pasemi.h>
> +
> #include <asm/processor.h>
> #include <asm/page.h>
> #include <asm/ppc_asm.h>
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
> index c66f566a854cb..c6bb3ecc90ef3 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -15,6 +15,7 @@
> #include <linux/timer.h>
> #include <linux/module.h>
> #include <linux/of_address.h>
> +#include <linux/platform_data/pasemi.h>
>
> #include <asm/hw_irq.h>
> #include <asm/io.h>
> diff --git a/include/linux/platform_data/pasemi.h b/include/linux/platform_data/pasemi.h
> new file mode 100644
> index 0000000000000..3fed0687fcc9a
> --- /dev/null
> +++ b/include/linux/platform_data/pasemi.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 Linaro Ltd.
> + *
> + * Author: Lee Jones <lee.jones@linaro.org>
> + */
Absolutely not. It's neither your copyright, nor your authorship.
-Olof
^ permalink raw reply
* [PATCH] powerpc: Fix inconsistent function names
From: YueHaibing @ 2020-07-15 3:19 UTC (permalink / raw)
To: mpe, benh, paulus, haren, dave.hansen, npiggin
Cc: YueHaibing, linuxppc-dev, linux-kernel
The stub helpers name should be consistent with prototypes.
mm_context_add_vas_windows() --> mm_context_add_vas_window()
mm_context_remove_vas_windows() --> mm_context_remove_vas_window()
Fixes: c420644c0a8f ("powerpc: Use mm_context vas_windows counter to issue CP_ABORT")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
arch/powerpc/include/asm/mmu_context.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 1a474f6b1992..00fd1d44731a 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -218,8 +218,8 @@ static inline void inc_mm_active_cpus(struct mm_struct *mm) { }
static inline void dec_mm_active_cpus(struct mm_struct *mm) { }
static inline void mm_context_add_copro(struct mm_struct *mm) { }
static inline void mm_context_remove_copro(struct mm_struct *mm) { }
-static inline void mm_context_add_vas_windows(struct mm_struct *mm) { }
-static inline void mm_context_remove_vas_windows(struct mm_struct *mm) { }
+static inline void mm_context_add_vas_window(struct mm_struct *mm) { }
+static inline void mm_context_remove_vas_window(struct mm_struct *mm) { }
#endif
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 10/15] powerpc/powernv/pci: Refactor pnv_ioda_alloc_pe()
From: Alexey Kardashevskiy @ 2020-07-15 3:15 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <CAOSf1CGkQBL5caG7tgCUzC7OP55qVk86_XbAx9L43+hy7YNXSw@mail.gmail.com>
On 15/07/2020 12:53, Oliver O'Halloran wrote:
> On Wed, Jul 15, 2020 at 12:29 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>
>>
>> On 10/07/2020 15:23, Oliver O'Halloran wrote:
>>> Rework the PE allocation logic to allow allocating blocks of PEs rather
>>> than individually. We'll use this to allocate contigious blocks of PEs for
>>> the SR-IOVs.
>>
>> The patch does not do just this, it also adds missing mutexes (which is
>> good) but still misses them in pnv_pci_sriov_disable() and
>> pnv_pci_ioda_pe_dump().
>
> The current implementation doesn't need the mutex because alloc,
> reserve and free all use atomic bit ops.
Ah, ok.
> The mutex has been there
> forever with nothing actually using it, but with the change we need to
> prevent modifications to the bitmap while alloc() is scanning it. I
> probably should have mentioned that in the commit message.
but bitmap_clear() (from pnv_pci_sriov_disable()) is not atomic. It
probably does not matter as the next patch gets rid of it anyway.
--
Alexey
^ permalink raw reply
* [PATCH -next] powerpc/smp: Remove unused inline functions
From: YueHaibing @ 2020-07-15 3:09 UTC (permalink / raw)
To: mpe, benh, paulus; +Cc: YueHaibing, linuxppc-dev, linux-kernel
commit 441c19c8a290 ("powerpc/kvm/book3s_hv: Rework the secondary inhibit code")
left behind this, remove it.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
arch/powerpc/include/asm/smp.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index 49a25e2400f2..5d5a7596d144 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -176,8 +176,6 @@ extern void __cpu_die(unsigned int cpu);
/* for UP */
#define hard_smp_processor_id() get_hard_smp_processor_id(0)
#define smp_setup_cpu_maps()
-static inline void inhibit_secondary_onlining(void) {}
-static inline void uninhibit_secondary_onlining(void) {}
static inline const struct cpumask *cpu_sibling_mask(int cpu)
{
return cpumask_of(cpu);
--
2.17.1
^ 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