* Re: [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
From: Claire Chang @ 2021-06-24 6:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: heikki.krogerus, linux-devicetree, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, Frank Rowand, mingo,
Jianxiong Gao, Stefano Stabellini, Saravana Kannan, Joerg Roedel,
Rafael J . Wysocki, Bartosz Golaszewski, bskeggs, linux-pci,
xen-devel, Marek Szyprowski, Dan Williams, matthew.auld,
Nicolas Boichat, thomas.hellstrom, Jim Quinlan, Will Deacon,
Konrad Rzeszutek Wilk, intel-gfx, maarten.lankhorst, Robin Murphy,
jani.nikula, Rob Herring, rodrigo.vivi, Bjorn Helgaas,
boris.ostrovsky, Andy Shevchenko, jgross, airlied, Thierry Reding,
Greg KH, Randy Dunlap, Qian Cai, lkml, list@263.net:IOMMU DRIVERS,
Daniel Vetter, xypron.glpk, Tom Lendacky, linuxppc-dev, bauerman
In-Reply-To: <20210624054315.GA25381@lst.de>
On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
> > is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
> >
> > is_swiotlb_force_bounce() was the new function introduced in this patch here.
> >
> > +static inline bool is_swiotlb_force_bounce(struct device *dev)
> > +{
> > + return dev->dma_io_tlb_mem->force_bounce;
> > +}
>
> To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
> turn this into :
>
> return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
>
> for a quick debug check?
I just realized that dma_io_tlb_mem might be NULL like Christoph
pointed out since swiotlb might not get initialized.
However, `Unable to handle kernel paging request at virtual address
dfff80000000000e` looks more like the address is garbage rather than
NULL?
I wonder if that's because dev->dma_io_tlb_mem is not assigned
properly (which means device_initialize is not called?).
^ permalink raw reply
* Re: [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
From: Christoph Hellwig @ 2021-06-24 5:43 UTC (permalink / raw)
To: Qian Cai
Cc: heikki.krogerus, linux-devicetree, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, Frank Rowand, mingo,
jxgao, sstabellini, Saravana Kannan, Joerg Roedel,
Rafael J . Wysocki, Christoph Hellwig, Bartosz Golaszewski,
bskeggs, linux-pci, xen-devel, Marek Szyprowski, Dan Williams,
matthew.auld, Nicolas Boichat, thomas.hellstrom, Jim Quinlan,
Will Deacon, Konrad Rzeszutek Wilk, intel-gfx, maarten.lankhorst,
Robin Murphy, jani.nikula, Rob Herring, rodrigo.vivi, bhelgaas,
Claire Chang, boris.ostrovsky, Andy Shevchenko, jgross, airlied,
Thierry Reding, Greg KH, Randy Dunlap, lkml,
list@263.net:IOMMU DRIVERS, daniel, xypron.glpk, thomas.lendacky,
linuxppc-dev, bauerman
In-Reply-To: <19d4c7a2-744d-21e0-289c-a576e1f0e6f3@quicinc.com>
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
> is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
>
> is_swiotlb_force_bounce() was the new function introduced in this patch here.
>
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return dev->dma_io_tlb_mem->force_bounce;
> +}
To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
turn this into :
return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
for a quick debug check?
^ permalink raw reply
* Re: [PATCH 6/6] drm/i915/gvt: use gfn_to_pfn's page instead of pfn
From: David Stevens @ 2021-06-24 4:28 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: David Stevens, intel-gvt-dev, Wanpeng Li, kvm, Will Deacon,
Suzuki K Poulose, Sean Christopherson, Joerg Roedel, linuxppc-dev,
open list, kvm-ppc, linux-mips, James Morse, ML dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
In-Reply-To: <20210624035749.4054934-7-stevensd@google.com>
Please ignore this last patch. It was put together as an afterthought
and wasn't properly tested.
-David
On Thu, Jun 24, 2021 at 12:59 PM David Stevens <stevensd@chromium.org> wrote:
>
> Return struct page instead of pfn from gfn_to_mfn. This function is only
> used to determine if the page is a transparent hugepage, to enable 2MB
> huge gtt shadowing. Returning the page directly avoids the risk of
> calling pfn_to_page on a VM_IO|VM_PFNMAP pfn.
>
> This change also properly releases the reference on the page returned by
> gfn_to_pfn.
>
> Signed-off-by: David Stevens <stevensd@google.com>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 12 ++++++++----
> drivers/gpu/drm/i915/gvt/hypercall.h | 3 ++-
> drivers/gpu/drm/i915/gvt/kvmgt.c | 12 ++++--------
> drivers/gpu/drm/i915/gvt/mpt.h | 8 ++++----
> 4 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 9478c132d7b6..b2951c560582 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -1160,16 +1160,20 @@ static int is_2MB_gtt_possible(struct intel_vgpu *vgpu,
> struct intel_gvt_gtt_entry *entry)
> {
> struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
> - unsigned long pfn;
> + struct page *page;
> + bool is_trans_huge;
>
> if (!HAS_PAGE_SIZES(vgpu->gvt->gt->i915, I915_GTT_PAGE_SIZE_2M))
> return 0;
>
> - pfn = intel_gvt_hypervisor_gfn_to_mfn(vgpu, ops->get_pfn(entry));
> - if (pfn == INTEL_GVT_INVALID_ADDR)
> + page = intel_gvt_hypervisor_gfn_to_mfn_page(vgpu, ops->get_pfn(entry));
> + if (!page)
> return -EINVAL;
>
> - return PageTransHuge(pfn_to_page(pfn));
> + is_trans_huge = PageTransHuge(page);
> + put_page(page);
> +
> + return is_trans_huge;
> }
>
> static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> index b79da5124f83..017190ff52d5 100644
> --- a/drivers/gpu/drm/i915/gvt/hypercall.h
> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> @@ -60,7 +60,8 @@ struct intel_gvt_mpt {
> unsigned long len);
> int (*write_gpa)(unsigned long handle, unsigned long gpa, void *buf,
> unsigned long len);
> - unsigned long (*gfn_to_mfn)(unsigned long handle, unsigned long gfn);
> + struct page *(*gfn_to_mfn_page)(unsigned long handle,
> + unsigned long gfn);
>
> int (*dma_map_guest_page)(unsigned long handle, unsigned long gfn,
> unsigned long size, dma_addr_t *dma_addr);
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index b829ff67e3d9..1e97ae813ed0 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1928,21 +1928,17 @@ static int kvmgt_inject_msi(unsigned long handle, u32 addr, u16 data)
> return -EFAULT;
> }
>
> -static unsigned long kvmgt_gfn_to_pfn(unsigned long handle, unsigned long gfn)
> +static struct page *kvmgt_gfn_to_page(unsigned long handle, unsigned long gfn)
> {
> struct kvmgt_guest_info *info;
> kvm_pfn_t pfn;
>
> if (!handle_valid(handle))
> - return INTEL_GVT_INVALID_ADDR;
> + return NULL;
>
> info = (struct kvmgt_guest_info *)handle;
>
> - pfn = kvm_pfn_page_unwrap(gfn_to_pfn(info->kvm, gfn));
> - if (is_error_noslot_pfn(pfn))
> - return INTEL_GVT_INVALID_ADDR;
> -
> - return pfn;
> + return gfn_to_pfn(info->kvm, gfn).page;
> }
>
> static int kvmgt_dma_map_guest_page(unsigned long handle, unsigned long gfn,
> @@ -2112,7 +2108,7 @@ static const struct intel_gvt_mpt kvmgt_mpt = {
> .disable_page_track = kvmgt_page_track_remove,
> .read_gpa = kvmgt_read_gpa,
> .write_gpa = kvmgt_write_gpa,
> - .gfn_to_mfn = kvmgt_gfn_to_pfn,
> + .gfn_to_mfn_page = kvmgt_gfn_to_page,
> .dma_map_guest_page = kvmgt_dma_map_guest_page,
> .dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
> .dma_pin_guest_page = kvmgt_dma_pin_guest_page,
> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
> index 550a456e936f..9169b83cf0f6 100644
> --- a/drivers/gpu/drm/i915/gvt/mpt.h
> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> @@ -214,17 +214,17 @@ static inline int intel_gvt_hypervisor_write_gpa(struct intel_vgpu *vgpu,
> }
>
> /**
> - * intel_gvt_hypervisor_gfn_to_mfn - translate a GFN to MFN
> + * intel_gvt_hypervisor_gfn_to_mfn_page - translate a GFN to MFN page
> * @vgpu: a vGPU
> * @gpfn: guest pfn
> *
> * Returns:
> - * MFN on success, INTEL_GVT_INVALID_ADDR if failed.
> + * struct page* on success, NULL if failed.
> */
> -static inline unsigned long intel_gvt_hypervisor_gfn_to_mfn(
> +static inline unsigned long intel_gvt_hypervisor_gfn_to_mfn_page(
> struct intel_vgpu *vgpu, unsigned long gfn)
> {
> - return intel_gvt_host.mpt->gfn_to_mfn(vgpu->handle, gfn);
> + return intel_gvt_host.mpt->gfn_to_mfn_page(vgpu->handle, gfn);
> }
>
> /**
> --
> 2.32.0.93.g670b81a890-goog
>
^ permalink raw reply
* [PATCH 6/6] drm/i915/gvt: use gfn_to_pfn's page instead of pfn
From: David Stevens @ 2021-06-24 3:57 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: David Stevens, intel-gvt-dev, Wanpeng Li, kvm, Will Deacon,
Suzuki K Poulose, Sean Christopherson, Joerg Roedel, linuxppc-dev,
linux-kernel, kvm-ppc, linux-mips, James Morse, dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
In-Reply-To: <20210624035749.4054934-1-stevensd@google.com>
Return struct page instead of pfn from gfn_to_mfn. This function is only
used to determine if the page is a transparent hugepage, to enable 2MB
huge gtt shadowing. Returning the page directly avoids the risk of
calling pfn_to_page on a VM_IO|VM_PFNMAP pfn.
This change also properly releases the reference on the page returned by
gfn_to_pfn.
Signed-off-by: David Stevens <stevensd@google.com>
---
drivers/gpu/drm/i915/gvt/gtt.c | 12 ++++++++----
drivers/gpu/drm/i915/gvt/hypercall.h | 3 ++-
drivers/gpu/drm/i915/gvt/kvmgt.c | 12 ++++--------
drivers/gpu/drm/i915/gvt/mpt.h | 8 ++++----
4 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
index 9478c132d7b6..b2951c560582 100644
--- a/drivers/gpu/drm/i915/gvt/gtt.c
+++ b/drivers/gpu/drm/i915/gvt/gtt.c
@@ -1160,16 +1160,20 @@ static int is_2MB_gtt_possible(struct intel_vgpu *vgpu,
struct intel_gvt_gtt_entry *entry)
{
struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
- unsigned long pfn;
+ struct page *page;
+ bool is_trans_huge;
if (!HAS_PAGE_SIZES(vgpu->gvt->gt->i915, I915_GTT_PAGE_SIZE_2M))
return 0;
- pfn = intel_gvt_hypervisor_gfn_to_mfn(vgpu, ops->get_pfn(entry));
- if (pfn == INTEL_GVT_INVALID_ADDR)
+ page = intel_gvt_hypervisor_gfn_to_mfn_page(vgpu, ops->get_pfn(entry));
+ if (!page)
return -EINVAL;
- return PageTransHuge(pfn_to_page(pfn));
+ is_trans_huge = PageTransHuge(page);
+ put_page(page);
+
+ return is_trans_huge;
}
static int split_2MB_gtt_entry(struct intel_vgpu *vgpu,
diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index b79da5124f83..017190ff52d5 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -60,7 +60,8 @@ struct intel_gvt_mpt {
unsigned long len);
int (*write_gpa)(unsigned long handle, unsigned long gpa, void *buf,
unsigned long len);
- unsigned long (*gfn_to_mfn)(unsigned long handle, unsigned long gfn);
+ struct page *(*gfn_to_mfn_page)(unsigned long handle,
+ unsigned long gfn);
int (*dma_map_guest_page)(unsigned long handle, unsigned long gfn,
unsigned long size, dma_addr_t *dma_addr);
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index b829ff67e3d9..1e97ae813ed0 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1928,21 +1928,17 @@ static int kvmgt_inject_msi(unsigned long handle, u32 addr, u16 data)
return -EFAULT;
}
-static unsigned long kvmgt_gfn_to_pfn(unsigned long handle, unsigned long gfn)
+static struct page *kvmgt_gfn_to_page(unsigned long handle, unsigned long gfn)
{
struct kvmgt_guest_info *info;
kvm_pfn_t pfn;
if (!handle_valid(handle))
- return INTEL_GVT_INVALID_ADDR;
+ return NULL;
info = (struct kvmgt_guest_info *)handle;
- pfn = kvm_pfn_page_unwrap(gfn_to_pfn(info->kvm, gfn));
- if (is_error_noslot_pfn(pfn))
- return INTEL_GVT_INVALID_ADDR;
-
- return pfn;
+ return gfn_to_pfn(info->kvm, gfn).page;
}
static int kvmgt_dma_map_guest_page(unsigned long handle, unsigned long gfn,
@@ -2112,7 +2108,7 @@ static const struct intel_gvt_mpt kvmgt_mpt = {
.disable_page_track = kvmgt_page_track_remove,
.read_gpa = kvmgt_read_gpa,
.write_gpa = kvmgt_write_gpa,
- .gfn_to_mfn = kvmgt_gfn_to_pfn,
+ .gfn_to_mfn_page = kvmgt_gfn_to_page,
.dma_map_guest_page = kvmgt_dma_map_guest_page,
.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
.dma_pin_guest_page = kvmgt_dma_pin_guest_page,
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index 550a456e936f..9169b83cf0f6 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -214,17 +214,17 @@ static inline int intel_gvt_hypervisor_write_gpa(struct intel_vgpu *vgpu,
}
/**
- * intel_gvt_hypervisor_gfn_to_mfn - translate a GFN to MFN
+ * intel_gvt_hypervisor_gfn_to_mfn_page - translate a GFN to MFN page
* @vgpu: a vGPU
* @gpfn: guest pfn
*
* Returns:
- * MFN on success, INTEL_GVT_INVALID_ADDR if failed.
+ * struct page* on success, NULL if failed.
*/
-static inline unsigned long intel_gvt_hypervisor_gfn_to_mfn(
+static inline unsigned long intel_gvt_hypervisor_gfn_to_mfn_page(
struct intel_vgpu *vgpu, unsigned long gfn)
{
- return intel_gvt_host.mpt->gfn_to_mfn(vgpu->handle, gfn);
+ return intel_gvt_host.mpt->gfn_to_mfn_page(vgpu->handle, gfn);
}
/**
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* [PATCH 5/6] KVM: mmu: remove over-aggressive warnings
From: David Stevens @ 2021-06-24 3:57 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: intel-gvt-dev, Wanpeng Li, kvm, Will Deacon, Suzuki K Poulose,
Sean Christopherson, Joerg Roedel, linuxppc-dev, linux-kernel,
kvm-ppc, linux-mips, David Stevens, James Morse, dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
In-Reply-To: <20210624035749.4054934-1-stevensd@google.com>
From: David Stevens <stevensd@chromium.org>
Remove two warnings that require ref counts for pages to be non-zero, as
mapped pfns from follow_pfn may not have an initialized ref count.
Signed-off-by: David Stevens <stevensd@chromium.org>
---
arch/x86/kvm/mmu/mmu.c | 7 -------
virt/kvm/kvm_main.c | 2 +-
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 8fa4a4a411ba..19249ad4b5b8 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -546,13 +546,6 @@ static int mmu_spte_clear_track_bits(u64 *sptep)
pfn = spte_to_pfn(old_spte);
- /*
- * KVM does not hold the refcount of the page used by
- * kvm mmu, before reclaiming the page, we should
- * unmap it from mmu first.
- */
- WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn)));
-
if (is_accessed_spte(old_spte))
kvm_set_pfn_accessed(pfn);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 898e90be4d0e..671361f30476 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -168,7 +168,7 @@ bool kvm_is_zone_device_pfn(kvm_pfn_t pfn)
* the device has been pinned, e.g. by get_user_pages(). WARN if the
* page_count() is zero to help detect bad usage of this helper.
*/
- if (!pfn_valid(pfn) || WARN_ON_ONCE(!page_count(pfn_to_page(pfn))))
+ if (!pfn_valid(pfn) || !page_count(pfn_to_page(pfn)))
return false;
return is_zone_device_page(pfn_to_page(pfn));
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* [PATCH 4/6] KVM: arm64/mmu: avoid struct page in MMU
From: David Stevens @ 2021-06-24 3:57 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: intel-gvt-dev, Wanpeng Li, kvm, Will Deacon, Suzuki K Poulose,
Sean Christopherson, Joerg Roedel, linuxppc-dev, linux-kernel,
kvm-ppc, linux-mips, David Stevens, James Morse, dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
In-Reply-To: <20210624035749.4054934-1-stevensd@google.com>
From: David Stevens <stevensd@chromium.org>
Avoid converting pfns returned by follow_fault_pfn to struct pages to
transiently take a reference. The reference was originally taken to
match the reference taken by gup. However, pfns returned by
follow_fault_pfn may not have a struct page set up for reference
counting.
Signed-off-by: David Stevens <stevensd@chromium.org>
---
arch/arm64/kvm/mmu.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 896b3644b36f..a741972cb75f 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -779,17 +779,17 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot,
*/
static unsigned long
transparent_hugepage_adjust(struct kvm_memory_slot *memslot,
- unsigned long hva, kvm_pfn_t *pfnp,
+ unsigned long hva, struct kvm_pfn_page *pfnpgp,
phys_addr_t *ipap)
{
- kvm_pfn_t pfn = *pfnp;
+ kvm_pfn_t pfn = pfnpgp->pfn;
/*
* Make sure the adjustment is done only for THP pages. Also make
* sure that the HVA and IPA are sufficiently aligned and that the
* block map is contained within the memslot.
*/
- if (kvm_is_transparent_hugepage(pfn) &&
+ if (pfnpgp->page && kvm_is_transparent_hugepage(pfn) &&
fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
/*
* The address we faulted on is backed by a transparent huge
@@ -810,10 +810,11 @@ transparent_hugepage_adjust(struct kvm_memory_slot *memslot,
* page accordingly.
*/
*ipap &= PMD_MASK;
- kvm_release_pfn_clean(pfn);
+ put_page(pfnpgp->page);
pfn &= ~(PTRS_PER_PMD - 1);
- kvm_get_pfn(pfn);
- *pfnp = pfn;
+ pfnpgp->pfn = pfn;
+ pfnpgp->page = pfn_to_page(pfnpgp->pfn);
+ get_page(pfnpgp->page);
return PMD_SIZE;
}
@@ -836,7 +837,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
struct vm_area_struct *vma;
short vma_shift;
gfn_t gfn;
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
bool logging_active = memslot_is_logging(memslot);
unsigned long fault_level = kvm_vcpu_trap_get_fault_level(vcpu);
unsigned long vma_pagesize, fault_granule;
@@ -933,17 +934,16 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
*/
smp_rmb();
- pfn = kvm_pfn_page_unwrap(__gfn_to_pfn_memslot(memslot, gfn, false,
- NULL, write_fault,
- &writable, NULL));
- if (pfn == KVM_PFN_ERR_HWPOISON) {
+ pfnpg = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
+ write_fault, &writable, NULL);
+ if (pfnpg.pfn == KVM_PFN_ERR_HWPOISON) {
kvm_send_hwpoison_signal(hva, vma_shift);
return 0;
}
- if (is_error_noslot_pfn(pfn))
+ if (is_error_noslot_pfn(pfnpg.pfn))
return -EFAULT;
- if (kvm_is_device_pfn(pfn)) {
+ if (kvm_is_device_pfn(pfnpg.pfn)) {
device = true;
force_pte = true;
} else if (logging_active && !write_fault) {
@@ -968,16 +968,16 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
*/
if (vma_pagesize == PAGE_SIZE && !force_pte)
vma_pagesize = transparent_hugepage_adjust(memslot, hva,
- &pfn, &fault_ipa);
+ &pfnpg, &fault_ipa);
if (writable)
prot |= KVM_PGTABLE_PROT_W;
if (fault_status != FSC_PERM && !device)
- clean_dcache_guest_page(pfn, vma_pagesize);
+ clean_dcache_guest_page(pfnpg.pfn, vma_pagesize);
if (exec_fault) {
prot |= KVM_PGTABLE_PROT_X;
- invalidate_icache_guest_page(pfn, vma_pagesize);
+ invalidate_icache_guest_page(pfnpg.pfn, vma_pagesize);
}
if (device)
@@ -994,20 +994,23 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
ret = kvm_pgtable_stage2_relax_perms(pgt, fault_ipa, prot);
} else {
ret = kvm_pgtable_stage2_map(pgt, fault_ipa, vma_pagesize,
- __pfn_to_phys(pfn), prot,
+ __pfn_to_phys(pfnpg.pfn), prot,
memcache);
}
/* Mark the page dirty only if the fault is handled successfully */
if (writable && !ret) {
- kvm_set_pfn_dirty(pfn);
+ if (pfnpg.page)
+ kvm_set_pfn_dirty(pfnpg.pfn);
mark_page_dirty_in_slot(kvm, memslot, gfn);
}
out_unlock:
spin_unlock(&kvm->mmu_lock);
- kvm_set_pfn_accessed(pfn);
- kvm_release_pfn_clean(pfn);
+ if (pfnpg.page) {
+ kvm_set_pfn_accessed(pfnpg.pfn);
+ put_page(pfnpg.page);
+ }
return ret != -EAGAIN ? ret : 0;
}
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* [PATCH 3/6] KVM: x86/mmu: avoid struct page in MMU
From: David Stevens @ 2021-06-24 3:57 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: intel-gvt-dev, Wanpeng Li, kvm, Will Deacon, Suzuki K Poulose,
Sean Christopherson, Joerg Roedel, linuxppc-dev, linux-kernel,
kvm-ppc, linux-mips, David Stevens, James Morse, dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
In-Reply-To: <20210624035749.4054934-1-stevensd@google.com>
From: David Stevens <stevensd@chromium.org>
Avoid converting pfns returned by follow_fault_pfn to struct pages to
transiently take a reference. The reference was originally taken to
match the reference taken by gup. However, pfns returned by
follow_fault_pfn may not have a struct page set up for reference
counting.
Signed-off-by: David Stevens <stevensd@chromium.org>
---
arch/x86/kvm/mmu/mmu.c | 56 +++++++++++++++++++--------------
arch/x86/kvm/mmu/mmu_audit.c | 13 ++++----
arch/x86/kvm/mmu/mmu_internal.h | 3 +-
arch/x86/kvm/mmu/paging_tmpl.h | 36 ++++++++++++---------
arch/x86/kvm/mmu/tdp_mmu.c | 7 +++--
arch/x86/kvm/mmu/tdp_mmu.h | 4 +--
arch/x86/kvm/x86.c | 9 +++---
7 files changed, 73 insertions(+), 55 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 84913677c404..8fa4a4a411ba 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2610,16 +2610,16 @@ static int mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
return ret;
}
-static kvm_pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
- bool no_dirty_log)
+static struct kvm_pfn_page pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu,
+ gfn_t gfn, bool no_dirty_log)
{
struct kvm_memory_slot *slot;
slot = gfn_to_memslot_dirty_bitmap(vcpu, gfn, no_dirty_log);
if (!slot)
- return KVM_PFN_ERR_FAULT;
+ return KVM_PFN_PAGE_ERR(KVM_PFN_ERR_FAULT);
- return kvm_pfn_page_unwrap(gfn_to_pfn_memslot_atomic(slot, gfn));
+ return gfn_to_pfn_memslot_atomic(slot, gfn);
}
static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
@@ -2748,7 +2748,8 @@ int kvm_mmu_max_mapping_level(struct kvm *kvm,
int kvm_mmu_hugepage_adjust(struct kvm_vcpu *vcpu, gfn_t gfn,
int max_level, kvm_pfn_t *pfnp,
- bool huge_page_disallowed, int *req_level)
+ struct page *page, bool huge_page_disallowed,
+ int *req_level)
{
struct kvm_memory_slot *slot;
kvm_pfn_t pfn = *pfnp;
@@ -2760,6 +2761,9 @@ int kvm_mmu_hugepage_adjust(struct kvm_vcpu *vcpu, gfn_t gfn,
if (unlikely(max_level == PG_LEVEL_4K))
return PG_LEVEL_4K;
+ if (!page)
+ return PG_LEVEL_4K;
+
if (is_error_noslot_pfn(pfn) || kvm_is_reserved_pfn(pfn))
return PG_LEVEL_4K;
@@ -2814,7 +2818,8 @@ void disallowed_hugepage_adjust(u64 spte, gfn_t gfn, int cur_level,
}
static int __direct_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
- int map_writable, int max_level, kvm_pfn_t pfn,
+ int map_writable, int max_level,
+ const struct kvm_pfn_page *pfnpg,
bool prefault, bool is_tdp)
{
bool nx_huge_page_workaround_enabled = is_nx_huge_page_enabled();
@@ -2826,11 +2831,12 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
int level, req_level, ret;
gfn_t gfn = gpa >> PAGE_SHIFT;
gfn_t base_gfn = gfn;
+ kvm_pfn_t pfn = pfnpg->pfn;
if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root_hpa)))
return RET_PF_RETRY;
- level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn,
+ level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn, pfnpg->page,
huge_page_disallowed, &req_level);
trace_kvm_mmu_spte_requested(gpa, level, pfn);
@@ -3672,8 +3678,8 @@ static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
}
static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
- gpa_t cr2_or_gpa, kvm_pfn_t *pfn, hva_t *hva,
- bool write, bool *writable)
+ gpa_t cr2_or_gpa, struct kvm_pfn_page *pfnpg,
+ hva_t *hva, bool write, bool *writable)
{
struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
bool async;
@@ -3688,17 +3694,16 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
/* Don't expose private memslots to L2. */
if (is_guest_mode(vcpu) && !kvm_is_visible_memslot(slot)) {
- *pfn = KVM_PFN_NOSLOT;
+ *pfnpg = KVM_PFN_PAGE_ERR(KVM_PFN_NOSLOT);
*writable = false;
return false;
}
async = false;
- *pfn = kvm_pfn_page_unwrap(__gfn_to_pfn_memslot(slot, gfn, false,
- &async, write,
- writable, hva));
+ *pfnpg = __gfn_to_pfn_memslot(slot, gfn, false, &async,
+ write, writable, hva);
if (!async)
- return false; /* *pfn has correct page already */
+ return false; /* *pfnpg has correct page already */
if (!prefault && kvm_can_do_async_pf(vcpu)) {
trace_kvm_try_async_get_page(cr2_or_gpa, gfn);
@@ -3710,8 +3715,8 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
return true;
}
- *pfn = kvm_pfn_page_unwrap(__gfn_to_pfn_memslot(slot, gfn, false, NULL,
- write, writable, hva));
+ *pfnpg = __gfn_to_pfn_memslot(slot, gfn, false, NULL,
+ write, writable, hva);
return false;
}
@@ -3723,7 +3728,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
gfn_t gfn = gpa >> PAGE_SHIFT;
unsigned long mmu_seq;
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
hva_t hva;
int r;
@@ -3743,11 +3748,12 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
mmu_seq = vcpu->kvm->mmu_notifier_seq;
smp_rmb();
- if (try_async_pf(vcpu, prefault, gfn, gpa, &pfn, &hva,
+ if (try_async_pf(vcpu, prefault, gfn, gpa, &pfnpg, &hva,
write, &map_writable))
return RET_PF_RETRY;
- if (handle_abnormal_pfn(vcpu, is_tdp ? 0 : gpa, gfn, pfn, ACC_ALL, &r))
+ if (handle_abnormal_pfn(vcpu, is_tdp ? 0 : gpa,
+ gfn, pfnpg.pfn, ACC_ALL, &r))
return r;
r = RET_PF_RETRY;
@@ -3757,7 +3763,8 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
else
write_lock(&vcpu->kvm->mmu_lock);
- if (!is_noslot_pfn(pfn) && mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, hva))
+ if (!is_noslot_pfn(pfnpg.pfn) &&
+ mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, hva))
goto out_unlock;
r = make_mmu_pages_available(vcpu);
if (r)
@@ -3765,17 +3772,18 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
r = kvm_tdp_mmu_map(vcpu, gpa, error_code, map_writable, max_level,
- pfn, prefault);
+ &pfnpg, prefault);
else
- r = __direct_map(vcpu, gpa, error_code, map_writable, max_level, pfn,
- prefault, is_tdp);
+ r = __direct_map(vcpu, gpa, error_code, map_writable, max_level,
+ &pfnpg, prefault, is_tdp);
out_unlock:
if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
read_unlock(&vcpu->kvm->mmu_lock);
else
write_unlock(&vcpu->kvm->mmu_lock);
- kvm_release_pfn_clean(pfn);
+ if (pfnpg.page)
+ put_page(pfnpg.page);
return r;
}
diff --git a/arch/x86/kvm/mmu/mmu_audit.c b/arch/x86/kvm/mmu/mmu_audit.c
index 3f983dc6e0f1..72b470b892da 100644
--- a/arch/x86/kvm/mmu/mmu_audit.c
+++ b/arch/x86/kvm/mmu/mmu_audit.c
@@ -94,7 +94,7 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
{
struct kvm_mmu_page *sp;
gfn_t gfn;
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
hpa_t hpa;
sp = sptep_to_sp(sptep);
@@ -111,18 +111,19 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
return;
gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
- pfn = kvm_pfn_page_unwrap(kvm_vcpu_gfn_to_pfn_atomic(vcpu, gfn));
+ pfnpg = kvm_vcpu_gfn_to_pfn_atomic(vcpu, gfn);
- if (is_error_pfn(pfn))
+ if (is_error_pfn(pfnpg.pfn))
return;
- hpa = pfn << PAGE_SHIFT;
+ hpa = pfnpg.pfn << PAGE_SHIFT;
if ((*sptep & PT64_BASE_ADDR_MASK) != hpa)
audit_printk(vcpu->kvm, "levels %d pfn %llx hpa %llx "
- "ent %llxn", vcpu->arch.mmu->root_level, pfn,
+ "ent %llxn", vcpu->arch.mmu->root_level, pfnpg.pfn,
hpa, *sptep);
- kvm_release_pfn_clean(pfn);
+ if (pfnpg.page)
+ put_page(pfnpg.page);
}
static void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
index d64ccb417c60..db4d878fde4e 100644
--- a/arch/x86/kvm/mmu/mmu_internal.h
+++ b/arch/x86/kvm/mmu/mmu_internal.h
@@ -154,7 +154,8 @@ int kvm_mmu_max_mapping_level(struct kvm *kvm,
kvm_pfn_t pfn, int max_level);
int kvm_mmu_hugepage_adjust(struct kvm_vcpu *vcpu, gfn_t gfn,
int max_level, kvm_pfn_t *pfnp,
- bool huge_page_disallowed, int *req_level);
+ struct page *page, bool huge_page_disallowed,
+ int *req_level);
void disallowed_hugepage_adjust(u64 spte, gfn_t gfn, int cur_level,
kvm_pfn_t *pfnp, int *goal_levelp);
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 823a5919f9fa..db13efd4b62d 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -535,7 +535,7 @@ FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
{
unsigned pte_access;
gfn_t gfn;
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
if (FNAME(prefetch_invalid_gpte)(vcpu, sp, spte, gpte))
return false;
@@ -545,19 +545,20 @@ FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
gfn = gpte_to_gfn(gpte);
pte_access = sp->role.access & FNAME(gpte_access)(gpte);
FNAME(protect_clean_gpte)(vcpu->arch.mmu, &pte_access, gpte);
- pfn = pte_prefetch_gfn_to_pfn(vcpu, gfn,
+ pfnpg = pte_prefetch_gfn_to_pfn(vcpu, gfn,
no_dirty_log && (pte_access & ACC_WRITE_MASK));
- if (is_error_pfn(pfn))
+ if (is_error_pfn(pfnpg.pfn))
return false;
/*
* we call mmu_set_spte() with host_writable = true because
* pte_prefetch_gfn_to_pfn always gets a writable pfn.
*/
- mmu_set_spte(vcpu, spte, pte_access, false, PG_LEVEL_4K, gfn, pfn,
+ mmu_set_spte(vcpu, spte, pte_access, false, PG_LEVEL_4K, gfn, pfnpg.pfn,
true, true);
- kvm_release_pfn_clean(pfn);
+ if (pfnpg.page)
+ put_page(pfnpg.page);
return true;
}
@@ -637,8 +638,8 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
*/
static int FNAME(fetch)(struct kvm_vcpu *vcpu, gpa_t addr,
struct guest_walker *gw, u32 error_code,
- int max_level, kvm_pfn_t pfn, bool map_writable,
- bool prefault)
+ int max_level, struct kvm_pfn_page *pfnpg,
+ bool map_writable, bool prefault)
{
bool nx_huge_page_workaround_enabled = is_nx_huge_page_enabled();
bool write_fault = error_code & PFERR_WRITE_MASK;
@@ -649,6 +650,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, gpa_t addr,
unsigned int direct_access, access;
int top_level, level, req_level, ret;
gfn_t base_gfn = gw->gfn;
+ kvm_pfn_t pfn = pfnpg->pfn;
direct_access = gw->pte_access;
@@ -695,7 +697,8 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, gpa_t addr,
}
level = kvm_mmu_hugepage_adjust(vcpu, gw->gfn, max_level, &pfn,
- huge_page_disallowed, &req_level);
+ pfnpg->page, huge_page_disallowed,
+ &req_level);
trace_kvm_mmu_spte_requested(addr, gw->level, pfn);
@@ -801,7 +804,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gpa_t addr, u32 error_code,
bool user_fault = error_code & PFERR_USER_MASK;
struct guest_walker walker;
int r;
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
hva_t hva;
unsigned long mmu_seq;
bool map_writable, is_self_change_mapping;
@@ -853,11 +856,12 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gpa_t addr, u32 error_code,
mmu_seq = vcpu->kvm->mmu_notifier_seq;
smp_rmb();
- if (try_async_pf(vcpu, prefault, walker.gfn, addr, &pfn, &hva,
+ if (try_async_pf(vcpu, prefault, walker.gfn, addr, &pfnpg, &hva,
write_fault, &map_writable))
return RET_PF_RETRY;
- if (handle_abnormal_pfn(vcpu, addr, walker.gfn, pfn, walker.pte_access, &r))
+ if (handle_abnormal_pfn(vcpu, addr, walker.gfn, pfnpg.pfn,
+ walker.pte_access, &r))
return r;
/*
@@ -866,7 +870,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gpa_t addr, u32 error_code,
*/
if (write_fault && !(walker.pte_access & ACC_WRITE_MASK) &&
!is_write_protection(vcpu) && !user_fault &&
- !is_noslot_pfn(pfn)) {
+ !is_noslot_pfn(pfnpg.pfn)) {
walker.pte_access |= ACC_WRITE_MASK;
walker.pte_access &= ~ACC_USER_MASK;
@@ -882,20 +886,22 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gpa_t addr, u32 error_code,
r = RET_PF_RETRY;
write_lock(&vcpu->kvm->mmu_lock);
- if (!is_noslot_pfn(pfn) && mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, hva))
+ if (!is_noslot_pfn(pfnpg.pfn) &&
+ mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, hva))
goto out_unlock;
kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
r = make_mmu_pages_available(vcpu);
if (r)
goto out_unlock;
- r = FNAME(fetch)(vcpu, addr, &walker, error_code, max_level, pfn,
+ r = FNAME(fetch)(vcpu, addr, &walker, error_code, max_level, &pfnpg,
map_writable, prefault);
kvm_mmu_audit(vcpu, AUDIT_POST_PAGE_FAULT);
out_unlock:
write_unlock(&vcpu->kvm->mmu_lock);
- kvm_release_pfn_clean(pfn);
+ if (pfnpg.page)
+ put_page(pfnpg.page);
return r;
}
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 237317b1eddd..b0e6d63f0fe1 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -960,8 +960,8 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, int write,
* page tables and SPTEs to translate the faulting guest physical address.
*/
int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
- int map_writable, int max_level, kvm_pfn_t pfn,
- bool prefault)
+ int map_writable, int max_level,
+ const struct kvm_pfn_page *pfnpg, bool prefault)
{
bool nx_huge_page_workaround_enabled = is_nx_huge_page_enabled();
bool write = error_code & PFERR_WRITE_MASK;
@@ -976,13 +976,14 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
gfn_t gfn = gpa >> PAGE_SHIFT;
int level;
int req_level;
+ kvm_pfn_t pfn = pfnpg->pfn;
if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root_hpa)))
return RET_PF_RETRY;
if (WARN_ON(!is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa)))
return RET_PF_RETRY;
- level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn,
+ level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn, pfnpg->page,
huge_page_disallowed, &req_level);
trace_kvm_mmu_spte_requested(gpa, level, pfn);
diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index 5fdf63090451..f78681b9dcb7 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -52,8 +52,8 @@ void kvm_tdp_mmu_invalidate_all_roots(struct kvm *kvm);
void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm);
int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
- int map_writable, int max_level, kvm_pfn_t pfn,
- bool prefault);
+ int map_writable, int max_level,
+ const struct kvm_pfn_page *pfnpg, bool prefault);
bool kvm_tdp_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range,
bool flush);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d31797e0cb6e..86d66c765190 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7311,7 +7311,7 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
int emulation_type)
{
gpa_t gpa = cr2_or_gpa;
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
if (!(emulation_type & EMULTYPE_ALLOW_RETRY_PF))
return false;
@@ -7341,16 +7341,17 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
* retry instruction -> write #PF -> emulation fail -> retry
* instruction -> ...
*/
- pfn = kvm_pfn_page_unwrap(gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa)));
+ pfnpg = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
/*
* If the instruction failed on the error pfn, it can not be fixed,
* report the error to userspace.
*/
- if (is_error_noslot_pfn(pfn))
+ if (is_error_noslot_pfn(pfnpg.pfn))
return false;
- kvm_release_pfn_clean(pfn);
+ if (pfnpg.page)
+ put_page(pfnpg.page);
/* The instructions are well-emulated on direct mmu. */
if (vcpu->arch.mmu->direct_map) {
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* [PATCH 2/6] KVM: mmu: also return page from gfn_to_pfn
From: David Stevens @ 2021-06-24 3:57 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: intel-gvt-dev, Wanpeng Li, kvm, Will Deacon, Suzuki K Poulose,
Sean Christopherson, Joerg Roedel, linuxppc-dev, linux-kernel,
kvm-ppc, linux-mips, David Stevens, James Morse, dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
In-Reply-To: <20210624035749.4054934-1-stevensd@google.com>
From: David Stevens <stevensd@chromium.org>
Return a struct kvm_pfn_page containing both a pfn and an optional
struct page from the gfn_to_pfn family of functions. This differentiates
the gup and follow_fault_pfn cases, which allows callers that only need
a pfn to avoid touching the page struct in the latter case. For callers
that need a struct page, introduce a helper function that unwraps a
struct kvm_pfn_page into a struct page. This helper makes the call to
kvm_get_pfn which had previously been in hva_to_pfn_remapped.
For now, wrap all calls to gfn_to_pfn functions in the new helper
function. Callers which don't need the page struct will be updated in
follow-up patches.
Signed-off-by: David Stevens <stevensd@chromium.org>
---
arch/arm64/kvm/mmu.c | 5 +-
arch/mips/kvm/mmu.c | 3 +-
arch/powerpc/kvm/book3s.c | 3 +-
arch/powerpc/kvm/book3s_64_mmu_hv.c | 5 +-
arch/powerpc/kvm/book3s_64_mmu_radix.c | 5 +-
arch/powerpc/kvm/book3s_hv_uvmem.c | 4 +-
arch/powerpc/kvm/e500_mmu_host.c | 2 +-
arch/x86/kvm/mmu/mmu.c | 11 ++-
arch/x86/kvm/mmu/mmu_audit.c | 2 +-
arch/x86/kvm/x86.c | 2 +-
drivers/gpu/drm/i915/gvt/kvmgt.c | 2 +-
include/linux/kvm_host.h | 27 ++++--
include/linux/kvm_types.h | 5 +
virt/kvm/kvm_main.c | 121 +++++++++++++------------
14 files changed, 109 insertions(+), 88 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index c10207fed2f3..896b3644b36f 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -933,8 +933,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
*/
smp_rmb();
- pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
- write_fault, &writable, NULL);
+ pfn = kvm_pfn_page_unwrap(__gfn_to_pfn_memslot(memslot, gfn, false,
+ NULL, write_fault,
+ &writable, NULL));
if (pfn == KVM_PFN_ERR_HWPOISON) {
kvm_send_hwpoison_signal(hva, vma_shift);
return 0;
diff --git a/arch/mips/kvm/mmu.c b/arch/mips/kvm/mmu.c
index 6d1f68cf4edf..f4e5e48bc6bf 100644
--- a/arch/mips/kvm/mmu.c
+++ b/arch/mips/kvm/mmu.c
@@ -630,7 +630,8 @@ static int kvm_mips_map_page(struct kvm_vcpu *vcpu, unsigned long gpa,
smp_rmb();
/* Slow path - ask KVM core whether we can access this GPA */
- pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writeable);
+ pfn = kvm_pfn_page_unwrap(gfn_to_pfn_prot(kvm, gfn,
+ write_fault, &writeable));
if (is_error_noslot_pfn(pfn)) {
err = -EFAULT;
goto out;
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 2b691f4d1f26..2dff01d0632a 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -417,7 +417,8 @@ kvm_pfn_t kvmppc_gpa_to_pfn(struct kvm_vcpu *vcpu, gpa_t gpa, bool writing,
return pfn;
}
- return gfn_to_pfn_prot(vcpu->kvm, gfn, writing, writable);
+ return kvm_pfn_page_unwrap(gfn_to_pfn_prot(vcpu->kvm, gfn,
+ writing, writable));
}
EXPORT_SYMBOL_GPL(kvmppc_gpa_to_pfn);
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 2d9193cd73be..ba094b9f87a9 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -590,8 +590,9 @@ int kvmppc_book3s_hv_page_fault(struct kvm_vcpu *vcpu,
write_ok = true;
} else {
/* Call KVM generic code to do the slow-path check */
- pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
- writing, &write_ok, NULL);
+ pfn = kvm_pfn_page_unwrap(
+ __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
+ writing, &write_ok, NULL));
if (is_error_noslot_pfn(pfn))
return -EFAULT;
page = NULL;
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index d909c069363e..e7892f148222 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -821,8 +821,9 @@ int kvmppc_book3s_instantiate_page(struct kvm_vcpu *vcpu,
unsigned long pfn;
/* Call KVM generic code to do the slow-path check */
- pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
- writing, upgrade_p, NULL);
+ pfn = kvm_pfn_page_unwrap(
+ __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
+ writing, upgrade_p, NULL));
if (is_error_noslot_pfn(pfn))
return -EFAULT;
page = NULL;
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 84e5a2dc8be5..0eee0d98b055 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -890,7 +890,7 @@ static unsigned long kvmppc_share_page(struct kvm *kvm, unsigned long gpa,
retry:
mutex_unlock(&kvm->arch.uvmem_lock);
- pfn = gfn_to_pfn(kvm, gfn);
+ pfn = kvm_pfn_page_unwrap(gfn_to_pfn(kvm, gfn));
if (is_error_noslot_pfn(pfn))
goto out;
@@ -1075,7 +1075,7 @@ int kvmppc_send_page_to_uv(struct kvm *kvm, unsigned long gfn)
unsigned long pfn;
int ret = U_SUCCESS;
- pfn = gfn_to_pfn(kvm, gfn);
+ pfn = kvm_pfn_page_unwrap(gfn_to_pfn(kvm, gfn));
if (is_error_noslot_pfn(pfn))
return -EFAULT;
diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
index 7f16afc331ef..be7ca5788a05 100644
--- a/arch/powerpc/kvm/e500_mmu_host.c
+++ b/arch/powerpc/kvm/e500_mmu_host.c
@@ -446,7 +446,7 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
if (likely(!pfnmap)) {
tsize_pages = 1UL << (tsize + 10 - PAGE_SHIFT);
- pfn = gfn_to_pfn_memslot(slot, gfn);
+ pfn = kvm_pfn_page_unwrap(gfn_to_pfn_memslot(slot, gfn));
if (is_error_noslot_pfn(pfn)) {
if (printk_ratelimit())
pr_err("%s: real page not found for gfn %lx\n",
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 8d5876dfc6b7..84913677c404 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2619,7 +2619,7 @@ static kvm_pfn_t pte_prefetch_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn,
if (!slot)
return KVM_PFN_ERR_FAULT;
- return gfn_to_pfn_memslot_atomic(slot, gfn);
+ return kvm_pfn_page_unwrap(gfn_to_pfn_memslot_atomic(slot, gfn));
}
static int direct_pte_prefetch_many(struct kvm_vcpu *vcpu,
@@ -3694,8 +3694,9 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
}
async = false;
- *pfn = __gfn_to_pfn_memslot(slot, gfn, false, &async,
- write, writable, hva);
+ *pfn = kvm_pfn_page_unwrap(__gfn_to_pfn_memslot(slot, gfn, false,
+ &async, write,
+ writable, hva));
if (!async)
return false; /* *pfn has correct page already */
@@ -3709,8 +3710,8 @@ static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
return true;
}
- *pfn = __gfn_to_pfn_memslot(slot, gfn, false, NULL,
- write, writable, hva);
+ *pfn = kvm_pfn_page_unwrap(__gfn_to_pfn_memslot(slot, gfn, false, NULL,
+ write, writable, hva));
return false;
}
diff --git a/arch/x86/kvm/mmu/mmu_audit.c b/arch/x86/kvm/mmu/mmu_audit.c
index 97ff184084b4..3f983dc6e0f1 100644
--- a/arch/x86/kvm/mmu/mmu_audit.c
+++ b/arch/x86/kvm/mmu/mmu_audit.c
@@ -111,7 +111,7 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
return;
gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
- pfn = kvm_vcpu_gfn_to_pfn_atomic(vcpu, gfn);
+ pfn = kvm_pfn_page_unwrap(kvm_vcpu_gfn_to_pfn_atomic(vcpu, gfn));
if (is_error_pfn(pfn))
return;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6d425310054b..d31797e0cb6e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7341,7 +7341,7 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
* retry instruction -> write #PF -> emulation fail -> retry
* instruction -> ...
*/
- pfn = gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa));
+ pfn = kvm_pfn_page_unwrap(gfn_to_pfn(vcpu->kvm, gpa_to_gfn(gpa)));
/*
* If the instruction failed on the error pfn, it can not be fixed,
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 65ff43cfc0f7..b829ff67e3d9 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1938,7 +1938,7 @@ static unsigned long kvmgt_gfn_to_pfn(unsigned long handle, unsigned long gfn)
info = (struct kvmgt_guest_info *)handle;
- pfn = gfn_to_pfn(info->kvm, gfn);
+ pfn = kvm_pfn_page_unwrap(gfn_to_pfn(info->kvm, gfn));
if (is_error_noslot_pfn(pfn))
return INTEL_GVT_INVALID_ADDR;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 8583ed3ff344..d3731790a967 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -138,6 +138,8 @@ static inline bool is_error_page(struct page *page)
return IS_ERR(page);
}
+#define KVM_PFN_PAGE_ERR(e) ((struct kvm_pfn_page) { .pfn = (e), .page = NULL })
+
#define KVM_REQUEST_MASK GENMASK(7,0)
#define KVM_REQUEST_NO_WAKEUP BIT(8)
#define KVM_REQUEST_WAIT BIT(9)
@@ -795,14 +797,19 @@ void kvm_release_page_clean(struct page *page);
void kvm_release_page_dirty(struct page *page);
void kvm_set_page_accessed(struct page *page);
-kvm_pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn);
-kvm_pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
- bool *writable);
-kvm_pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn);
-kvm_pfn_t gfn_to_pfn_memslot_atomic(struct kvm_memory_slot *slot, gfn_t gfn);
-kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
- bool atomic, bool *async, bool write_fault,
- bool *writable, hva_t *hva);
+struct kvm_pfn_page gfn_to_pfn(struct kvm *kvm, gfn_t gfn);
+struct kvm_pfn_page gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn,
+ bool write_fault, bool *writable);
+struct kvm_pfn_page gfn_to_pfn_memslot(struct kvm_memory_slot *slot,
+ gfn_t gfn);
+struct kvm_pfn_page gfn_to_pfn_memslot_atomic(struct kvm_memory_slot *slot,
+ gfn_t gfn);
+struct kvm_pfn_page __gfn_to_pfn_memslot(struct kvm_memory_slot *slot,
+ gfn_t gfn, bool atomic, bool *async,
+ bool write_fault, bool *writable,
+ hva_t *hva);
+
+kvm_pfn_t kvm_pfn_page_unwrap(struct kvm_pfn_page pfnpg);
void kvm_release_pfn_clean(kvm_pfn_t pfn);
void kvm_release_pfn_dirty(kvm_pfn_t pfn);
@@ -883,8 +890,8 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn);
struct kvm_memslots *kvm_vcpu_memslots(struct kvm_vcpu *vcpu);
struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn);
-kvm_pfn_t kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn);
-kvm_pfn_t kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn);
+struct kvm_pfn_page kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn);
+struct kvm_pfn_page kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn);
int kvm_vcpu_map(struct kvm_vcpu *vcpu, gpa_t gpa, struct kvm_host_map *map);
int kvm_map_gfn(struct kvm_vcpu *vcpu, gfn_t gfn, struct kvm_host_map *map,
struct gfn_to_pfn_cache *cache, bool atomic);
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index a7580f69dda0..471f4b329f46 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -45,6 +45,11 @@ typedef u64 hfn_t;
typedef hfn_t kvm_pfn_t;
+struct kvm_pfn_page {
+ kvm_pfn_t pfn;
+ struct page *page;
+};
+
struct gfn_to_hva_cache {
u64 generation;
gpa_t gpa;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6a6bc7af0e28..898e90be4d0e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1982,9 +1982,8 @@ static inline int check_user_page_hwpoison(unsigned long addr)
* only part that runs if we can in atomic context.
*/
static bool hva_to_pfn_fast(unsigned long addr, bool write_fault,
- bool *writable, kvm_pfn_t *pfn)
+ bool *writable, struct kvm_pfn_page *pfnpg)
{
- struct page *page[1];
/*
* Fast pin a writable pfn only if it is a write fault request
@@ -1994,8 +1993,8 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault,
if (!(write_fault || writable))
return false;
- if (get_user_page_fast_only(addr, FOLL_WRITE, page)) {
- *pfn = page_to_pfn(page[0]);
+ if (get_user_page_fast_only(addr, FOLL_WRITE, &pfnpg->page)) {
+ pfnpg->pfn = page_to_pfn(pfnpg->page);
if (writable)
*writable = true;
@@ -2010,10 +2009,9 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault,
* 1 indicates success, -errno is returned if error is detected.
*/
static int hva_to_pfn_slow(unsigned long addr, bool *async, bool write_fault,
- bool *writable, kvm_pfn_t *pfn)
+ bool *writable, struct kvm_pfn_page *pfnpg)
{
unsigned int flags = FOLL_HWPOISON;
- struct page *page;
int npages = 0;
might_sleep();
@@ -2026,7 +2024,7 @@ static int hva_to_pfn_slow(unsigned long addr, bool *async, bool write_fault,
if (async)
flags |= FOLL_NOWAIT;
- npages = get_user_pages_unlocked(addr, 1, &page, flags);
+ npages = get_user_pages_unlocked(addr, 1, &pfnpg->page, flags);
if (npages != 1)
return npages;
@@ -2036,11 +2034,11 @@ static int hva_to_pfn_slow(unsigned long addr, bool *async, bool write_fault,
if (get_user_page_fast_only(addr, FOLL_WRITE, &wpage)) {
*writable = true;
- put_page(page);
- page = wpage;
+ put_page(pfnpg->page);
+ pfnpg->page = wpage;
}
}
- *pfn = page_to_pfn(page);
+ pfnpg->pfn = page_to_pfn(pfnpg->page);
return npages;
}
@@ -2058,7 +2056,7 @@ static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
static int hva_to_pfn_remapped(struct vm_area_struct *vma,
unsigned long addr, bool *async,
bool write_fault, bool *writable,
- kvm_pfn_t *p_pfn)
+ struct kvm_pfn_page *p_pfn)
{
kvm_pfn_t pfn;
pte_t *ptep;
@@ -2094,22 +2092,12 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
*writable = pte_write(*ptep);
pfn = pte_pfn(*ptep);
- /*
- * Get a reference here because callers of *hva_to_pfn* and
- * *gfn_to_pfn* ultimately call kvm_release_pfn_clean on the
- * returned pfn. This is only needed if the VMA has VM_MIXEDMAP
- * set, but the kvm_get_pfn/kvm_release_pfn_clean pair will
- * simply do nothing for reserved pfns.
- *
- * Whoever called remap_pfn_range is also going to call e.g.
- * unmap_mapping_range before the underlying pages are freed,
- * causing a call to our MMU notifier.
- */
- kvm_get_pfn(pfn);
-
out:
pte_unmap_unlock(ptep, ptl);
- *p_pfn = pfn;
+
+ p_pfn->pfn = pfn;
+ p_pfn->page = NULL;
+
return 0;
}
@@ -2127,30 +2115,30 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
* 2): @write_fault = false && @writable, @writable will tell the caller
* whether the mapping is writable.
*/
-static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
- bool write_fault, bool *writable)
+static struct kvm_pfn_page hva_to_pfn(unsigned long addr, bool atomic,
+ bool *async, bool write_fault, bool *writable)
{
struct vm_area_struct *vma;
- kvm_pfn_t pfn = 0;
+ struct kvm_pfn_page pfnpg = {};
int npages, r;
/* we can do it either atomically or asynchronously, not both */
BUG_ON(atomic && async);
- if (hva_to_pfn_fast(addr, write_fault, writable, &pfn))
- return pfn;
+ if (hva_to_pfn_fast(addr, write_fault, writable, &pfnpg))
+ return pfnpg;
if (atomic)
- return KVM_PFN_ERR_FAULT;
+ return KVM_PFN_PAGE_ERR(KVM_PFN_ERR_FAULT);
- npages = hva_to_pfn_slow(addr, async, write_fault, writable, &pfn);
+ npages = hva_to_pfn_slow(addr, async, write_fault, writable, &pfnpg);
if (npages == 1)
- return pfn;
+ return pfnpg;
mmap_read_lock(current->mm);
if (npages == -EHWPOISON ||
(!async && check_user_page_hwpoison(addr))) {
- pfn = KVM_PFN_ERR_HWPOISON;
+ pfnpg.pfn = KVM_PFN_ERR_HWPOISON;
goto exit;
}
@@ -2158,26 +2146,27 @@ static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
vma = find_vma_intersection(current->mm, addr, addr + 1);
if (vma == NULL)
- pfn = KVM_PFN_ERR_FAULT;
+ pfnpg.pfn = KVM_PFN_ERR_FAULT;
else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
- r = hva_to_pfn_remapped(vma, addr, async, write_fault, writable, &pfn);
+ r = hva_to_pfn_remapped(vma, addr, async, write_fault, writable, &pfnpg);
if (r == -EAGAIN)
goto retry;
if (r < 0)
- pfn = KVM_PFN_ERR_FAULT;
+ pfnpg.pfn = KVM_PFN_ERR_FAULT;
} else {
if (async && vma_is_valid(vma, write_fault))
*async = true;
- pfn = KVM_PFN_ERR_FAULT;
+ pfnpg.pfn = KVM_PFN_ERR_FAULT;
}
exit:
mmap_read_unlock(current->mm);
- return pfn;
+ return pfnpg;
}
-kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
- bool atomic, bool *async, bool write_fault,
- bool *writable, hva_t *hva)
+struct kvm_pfn_page __gfn_to_pfn_memslot(struct kvm_memory_slot *slot,
+ gfn_t gfn, bool atomic, bool *async,
+ bool write_fault, bool *writable,
+ hva_t *hva)
{
unsigned long addr = __gfn_to_hva_many(slot, gfn, NULL, write_fault);
@@ -2187,13 +2176,13 @@ kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
if (addr == KVM_HVA_ERR_RO_BAD) {
if (writable)
*writable = false;
- return KVM_PFN_ERR_RO_FAULT;
+ return KVM_PFN_PAGE_ERR(KVM_PFN_ERR_RO_FAULT);
}
if (kvm_is_error_hva(addr)) {
if (writable)
*writable = false;
- return KVM_PFN_NOSLOT;
+ return KVM_PFN_PAGE_ERR(KVM_PFN_NOSLOT);
}
/* Do not map writable pfn in the readonly memslot. */
@@ -2207,44 +2196,55 @@ kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
}
EXPORT_SYMBOL_GPL(__gfn_to_pfn_memslot);
-kvm_pfn_t gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn, bool write_fault,
- bool *writable)
+struct kvm_pfn_page gfn_to_pfn_prot(struct kvm *kvm, gfn_t gfn,
+ bool write_fault, bool *writable)
{
return __gfn_to_pfn_memslot(gfn_to_memslot(kvm, gfn), gfn, false, NULL,
write_fault, writable, NULL);
}
EXPORT_SYMBOL_GPL(gfn_to_pfn_prot);
-kvm_pfn_t gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn)
+struct kvm_pfn_page gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn)
{
return __gfn_to_pfn_memslot(slot, gfn, false, NULL, true, NULL, NULL);
}
EXPORT_SYMBOL_GPL(gfn_to_pfn_memslot);
-kvm_pfn_t gfn_to_pfn_memslot_atomic(struct kvm_memory_slot *slot, gfn_t gfn)
+struct kvm_pfn_page gfn_to_pfn_memslot_atomic(struct kvm_memory_slot *slot,
+ gfn_t gfn)
{
return __gfn_to_pfn_memslot(slot, gfn, true, NULL, true, NULL, NULL);
}
EXPORT_SYMBOL_GPL(gfn_to_pfn_memslot_atomic);
-kvm_pfn_t kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn)
+struct kvm_pfn_page kvm_vcpu_gfn_to_pfn_atomic(struct kvm_vcpu *vcpu, gfn_t gfn)
{
return gfn_to_pfn_memslot_atomic(kvm_vcpu_gfn_to_memslot(vcpu, gfn), gfn);
}
EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_pfn_atomic);
-kvm_pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
+struct kvm_pfn_page gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
{
return gfn_to_pfn_memslot(gfn_to_memslot(kvm, gfn), gfn);
}
EXPORT_SYMBOL_GPL(gfn_to_pfn);
-kvm_pfn_t kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn)
+struct kvm_pfn_page kvm_vcpu_gfn_to_pfn(struct kvm_vcpu *vcpu, gfn_t gfn)
{
return gfn_to_pfn_memslot(kvm_vcpu_gfn_to_memslot(vcpu, gfn), gfn);
}
EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_pfn);
+kvm_pfn_t kvm_pfn_page_unwrap(struct kvm_pfn_page pfnpg)
+{
+ if (pfnpg.page)
+ return pfnpg.pfn;
+
+ kvm_get_pfn(pfnpg.pfn);
+ return pfnpg.pfn;
+}
+EXPORT_SYMBOL_GPL(kvm_pfn_page_unwrap);
+
int gfn_to_page_many_atomic(struct kvm_memory_slot *slot, gfn_t gfn,
struct page **pages, int nr_pages)
{
@@ -2277,11 +2277,14 @@ static struct page *kvm_pfn_to_page(kvm_pfn_t pfn)
struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)
{
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
+
+ pfnpg = gfn_to_pfn(kvm, gfn);
- pfn = gfn_to_pfn(kvm, gfn);
+ if (pfnpg.page)
+ return pfnpg.page;
- return kvm_pfn_to_page(pfn);
+ return kvm_pfn_to_page(kvm_pfn_page_unwrap(pfnpg));
}
EXPORT_SYMBOL_GPL(gfn_to_page);
@@ -2304,7 +2307,7 @@ static void kvm_cache_gfn_to_pfn(struct kvm_memory_slot *slot, gfn_t gfn,
{
kvm_release_pfn(cache->pfn, cache->dirty, cache);
- cache->pfn = gfn_to_pfn_memslot(slot, gfn);
+ cache->pfn = kvm_pfn_page_unwrap(gfn_to_pfn_memslot(slot, gfn));
cache->gfn = gfn;
cache->dirty = false;
cache->generation = gen;
@@ -2335,7 +2338,7 @@ static int __kvm_map_gfn(struct kvm_memslots *slots, gfn_t gfn,
} else {
if (atomic)
return -EAGAIN;
- pfn = gfn_to_pfn_memslot(slot, gfn);
+ pfn = kvm_pfn_page_unwrap(gfn_to_pfn_memslot(slot, gfn));
}
if (is_error_noslot_pfn(pfn))
return -EINVAL;
@@ -2435,11 +2438,11 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_unmap);
struct page *kvm_vcpu_gfn_to_page(struct kvm_vcpu *vcpu, gfn_t gfn)
{
- kvm_pfn_t pfn;
+ struct kvm_pfn_page pfnpg;
- pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
+ pfnpg = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
- return kvm_pfn_to_page(pfn);
+ return kvm_pfn_to_page(kvm_pfn_page_unwrap(pfnpg));
}
EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_page);
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* [PATCH 1/6] KVM: x86/mmu: release audited pfns
From: David Stevens @ 2021-06-24 3:57 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: intel-gvt-dev, Wanpeng Li, kvm, Will Deacon, Suzuki K Poulose,
Sean Christopherson, Joerg Roedel, linuxppc-dev, linux-kernel,
kvm-ppc, linux-mips, David Stevens, James Morse, dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
In-Reply-To: <20210624035749.4054934-1-stevensd@google.com>
From: David Stevens <stevensd@chromium.org>
Signed-off-by: David Stevens <stevensd@chromium.org>
---
arch/x86/kvm/mmu/mmu_audit.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kvm/mmu/mmu_audit.c b/arch/x86/kvm/mmu/mmu_audit.c
index cedc17b2f60e..97ff184084b4 100644
--- a/arch/x86/kvm/mmu/mmu_audit.c
+++ b/arch/x86/kvm/mmu/mmu_audit.c
@@ -121,6 +121,8 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)
audit_printk(vcpu->kvm, "levels %d pfn %llx hpa %llx "
"ent %llxn", vcpu->arch.mmu->root_level, pfn,
hpa, *sptep);
+
+ kvm_release_pfn_clean(pfn);
}
static void inspect_spte_has_rmap(struct kvm *kvm, u64 *sptep)
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* [PATCH 0/6] KVM: Remove uses of struct page from x86 and arm64 MMU
From: David Stevens @ 2021-06-24 3:57 UTC (permalink / raw)
To: Marc Zyngier, Huacai Chen, Aleksandar Markovic, Paul Mackerras,
Paolo Bonzini, Zhenyu Wang, Zhi Wang
Cc: David Stevens, intel-gvt-dev, Wanpeng Li, kvm, Will Deacon,
Suzuki K Poulose, Sean Christopherson, Joerg Roedel, linuxppc-dev,
linux-kernel, kvm-ppc, linux-mips, James Morse, dri-devel,
intel-gfx, Vitaly Kuznetsov, Alexandru Elisei, kvmarm,
linux-arm-kernel, Jim Mattson
KVM supports mapping VM_IO and VM_PFNMAP memory into the guest by using
follow_pte in gfn_to_pfn. However, the resolved pfns may not have
assoicated struct pages, so they should not be passed to pfn_to_page.
This series removes such calls from the x86 and arm64 secondary MMU. To
do this, this series modifies gfn_to_pfn to return a struct page in
addition to a pfn, if the hva was resolved by gup. This allows the
caller to call put_page only when necessated by gup.
This series provides a helper function that unwraps the new return type
of gfn_to_pfn to provide behavior identical to the old behavior. As I
have no hardware to test powerpc/mips changes, the function is used
there for minimally invasive changes. Additionally, as gfn_to_page and
gfn_to_pfn_cache are not integrated with mmu notifier, they cannot be
easily changed over to only use pfns.
This addresses CVE-2021-22543 on x86 and arm64.
David Stevens (6):
KVM: x86/mmu: release audited pfns
KVM: mmu: also return page from gfn_to_pfn
KVM: x86/mmu: avoid struct page in MMU
KVM: arm64/mmu: avoid struct page in MMU
KVM: mmu: remove over-aggressive warnings
drm/i915/gvt: use gfn_to_pfn's page instead of pfn
arch/arm64/kvm/mmu.c | 42 +++++----
arch/mips/kvm/mmu.c | 3 +-
arch/powerpc/kvm/book3s.c | 3 +-
arch/powerpc/kvm/book3s_64_mmu_hv.c | 5 +-
arch/powerpc/kvm/book3s_64_mmu_radix.c | 5 +-
arch/powerpc/kvm/book3s_hv_uvmem.c | 4 +-
arch/powerpc/kvm/e500_mmu_host.c | 2 +-
arch/x86/kvm/mmu/mmu.c | 60 ++++++------
arch/x86/kvm/mmu/mmu_audit.c | 13 ++-
arch/x86/kvm/mmu/mmu_internal.h | 3 +-
arch/x86/kvm/mmu/paging_tmpl.h | 36 +++++---
arch/x86/kvm/mmu/tdp_mmu.c | 7 +-
arch/x86/kvm/mmu/tdp_mmu.h | 4 +-
arch/x86/kvm/x86.c | 9 +-
drivers/gpu/drm/i915/gvt/gtt.c | 12 ++-
drivers/gpu/drm/i915/gvt/hypercall.h | 3 +-
drivers/gpu/drm/i915/gvt/kvmgt.c | 12 +--
drivers/gpu/drm/i915/gvt/mpt.h | 8 +-
include/linux/kvm_host.h | 27 ++++--
include/linux/kvm_types.h | 5 +
virt/kvm/kvm_main.c | 123 +++++++++++++------------
21 files changed, 212 insertions(+), 174 deletions(-)
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply
* [PATCH v16 4/4] kasan: use MAX_PTRS_PER_* for early shadow tables
From: Daniel Axtens @ 2021-06-24 3:40 UTC (permalink / raw)
To: linux-kernel, linux-mm, kasan-dev, elver, akpm, andreyknvl
Cc: aneesh.kumar, linuxppc-dev, Daniel Axtens
In-Reply-To: <20210624034050.511391-1-dja@axtens.net>
powerpc has a variable number of PTRS_PER_*, set at runtime based
on the MMU that the kernel is booted under.
This means the PTRS_PER_* are no longer constants, and therefore
breaks the build. Switch to using MAX_PTRS_PER_*, which are constant.
Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Suggested-by: Balbir Singh <bsingharora@gmail.com>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Balbir Singh <bsingharora@gmail.com>
Reviewed-by: Marco Elver <elver@google.com>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
include/linux/kasan.h | 6 +++---
mm/kasan/init.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 768d7d342757..5310e217bd74 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -41,9 +41,9 @@ struct kunit_kasan_expectation {
#endif
extern unsigned char kasan_early_shadow_page[PAGE_SIZE];
-extern pte_t kasan_early_shadow_pte[PTRS_PER_PTE + PTE_HWTABLE_PTRS];
-extern pmd_t kasan_early_shadow_pmd[PTRS_PER_PMD];
-extern pud_t kasan_early_shadow_pud[PTRS_PER_PUD];
+extern pte_t kasan_early_shadow_pte[MAX_PTRS_PER_PTE + PTE_HWTABLE_PTRS];
+extern pmd_t kasan_early_shadow_pmd[MAX_PTRS_PER_PMD];
+extern pud_t kasan_early_shadow_pud[MAX_PTRS_PER_PUD];
extern p4d_t kasan_early_shadow_p4d[MAX_PTRS_PER_P4D];
int kasan_populate_early_shadow(const void *shadow_start,
diff --git a/mm/kasan/init.c b/mm/kasan/init.c
index 348f31d15a97..cc64ed6858c6 100644
--- a/mm/kasan/init.c
+++ b/mm/kasan/init.c
@@ -41,7 +41,7 @@ static inline bool kasan_p4d_table(pgd_t pgd)
}
#endif
#if CONFIG_PGTABLE_LEVELS > 3
-pud_t kasan_early_shadow_pud[PTRS_PER_PUD] __page_aligned_bss;
+pud_t kasan_early_shadow_pud[MAX_PTRS_PER_PUD] __page_aligned_bss;
static inline bool kasan_pud_table(p4d_t p4d)
{
return p4d_page(p4d) == virt_to_page(lm_alias(kasan_early_shadow_pud));
@@ -53,7 +53,7 @@ static inline bool kasan_pud_table(p4d_t p4d)
}
#endif
#if CONFIG_PGTABLE_LEVELS > 2
-pmd_t kasan_early_shadow_pmd[PTRS_PER_PMD] __page_aligned_bss;
+pmd_t kasan_early_shadow_pmd[MAX_PTRS_PER_PMD] __page_aligned_bss;
static inline bool kasan_pmd_table(pud_t pud)
{
return pud_page(pud) == virt_to_page(lm_alias(kasan_early_shadow_pmd));
@@ -64,7 +64,7 @@ static inline bool kasan_pmd_table(pud_t pud)
return false;
}
#endif
-pte_t kasan_early_shadow_pte[PTRS_PER_PTE + PTE_HWTABLE_PTRS]
+pte_t kasan_early_shadow_pte[MAX_PTRS_PER_PTE + PTE_HWTABLE_PTRS]
__page_aligned_bss;
static inline bool kasan_pte_table(pmd_t pmd)
--
2.30.2
^ permalink raw reply related
* [PATCH v16 3/4] mm: define default MAX_PTRS_PER_* in include/pgtable.h
From: Daniel Axtens @ 2021-06-24 3:40 UTC (permalink / raw)
To: linux-kernel, linux-mm, kasan-dev, elver, akpm, andreyknvl
Cc: aneesh.kumar, linuxppc-dev, Daniel Axtens
In-Reply-To: <20210624034050.511391-1-dja@axtens.net>
Commit c65e774fb3f6 ("x86/mm: Make PGDIR_SHIFT and PTRS_PER_P4D variable")
made PTRS_PER_P4D variable on x86 and introduced MAX_PTRS_PER_P4D as a
constant for cases which need a compile-time constant (e.g. fixed-size
arrays).
powerpc likewise has boot-time selectable MMU features which can cause
other mm "constants" to vary. For KASAN, we have some static
PTE/PMD/PUD/P4D arrays so we need compile-time maximums for all these
constants. Extend the MAX_PTRS_PER_ idiom, and place default definitions
in include/pgtable.h. These define MAX_PTRS_PER_x to be PTRS_PER_x unless
an architecture has defined MAX_PTRS_PER_x in its arch headers.
Clean up pgtable-nop4d.h and s390's MAX_PTRS_PER_P4D definitions while
we're at it: both can just pick up the default now.
Acked-by: Andrey Konovalov <andreyknvl@gmail.com>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Marco Elver <elver@google.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
s390 was compile tested only.
---
arch/s390/include/asm/pgtable.h | 2 --
include/asm-generic/pgtable-nop4d.h | 1 -
include/linux/pgtable.h | 22 ++++++++++++++++++++++
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 79742f497cb5..dcac7b2df72c 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -343,8 +343,6 @@ static inline int is_module_addr(void *addr)
#define PTRS_PER_P4D _CRST_ENTRIES
#define PTRS_PER_PGD _CRST_ENTRIES
-#define MAX_PTRS_PER_P4D PTRS_PER_P4D
-
/*
* Segment table and region3 table entry encoding
* (R = read-only, I = invalid, y = young bit):
diff --git a/include/asm-generic/pgtable-nop4d.h b/include/asm-generic/pgtable-nop4d.h
index 2f1d0aad645c..03b7dae47dd4 100644
--- a/include/asm-generic/pgtable-nop4d.h
+++ b/include/asm-generic/pgtable-nop4d.h
@@ -9,7 +9,6 @@
typedef struct { pgd_t pgd; } p4d_t;
#define P4D_SHIFT PGDIR_SHIFT
-#define MAX_PTRS_PER_P4D 1
#define PTRS_PER_P4D 1
#define P4D_SIZE (1UL << P4D_SHIFT)
#define P4D_MASK (~(P4D_SIZE-1))
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index fb20c57de2ce..d147480cdefc 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1634,4 +1634,26 @@ typedef unsigned int pgtbl_mod_mask;
#define pte_leaf_size(x) PAGE_SIZE
#endif
+/*
+ * Some architectures have MMUs that are configurable or selectable at boot
+ * time. These lead to variable PTRS_PER_x. For statically allocated arrays it
+ * helps to have a static maximum value.
+ */
+
+#ifndef MAX_PTRS_PER_PTE
+#define MAX_PTRS_PER_PTE PTRS_PER_PTE
+#endif
+
+#ifndef MAX_PTRS_PER_PMD
+#define MAX_PTRS_PER_PMD PTRS_PER_PMD
+#endif
+
+#ifndef MAX_PTRS_PER_PUD
+#define MAX_PTRS_PER_PUD PTRS_PER_PUD
+#endif
+
+#ifndef MAX_PTRS_PER_P4D
+#define MAX_PTRS_PER_P4D PTRS_PER_P4D
+#endif
+
#endif /* _LINUX_PGTABLE_H */
--
2.30.2
^ permalink raw reply related
* [PATCH v16 2/4] kasan: allow architectures to provide an outline readiness check
From: Daniel Axtens @ 2021-06-24 3:40 UTC (permalink / raw)
To: linux-kernel, linux-mm, kasan-dev, elver, akpm, andreyknvl
Cc: aneesh.kumar, linuxppc-dev, Daniel Axtens
In-Reply-To: <20210624034050.511391-1-dja@axtens.net>
Allow architectures to define a kasan_arch_is_ready() hook that bails
out of any function that's about to touch the shadow unless the arch
says that it is ready for the memory to be accessed. This is fairly
uninvasive and should have a negligible performance penalty.
This will only work in outline mode, so an arch must specify
ARCH_DISABLE_KASAN_INLINE if it requires this.
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Marco Elver <elver@google.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
--
Both previous RFCs for ppc64 - by 2 different people - have
needed this trick! See:
- https://lore.kernel.org/patchwork/patch/592820/ # ppc64 hash series
- https://patchwork.ozlabs.org/patch/795211/ # ppc radix series
Build tested on arm64 with SW_TAGS and x86 with INLINE: the error fires
if I add a kasan_arch_is_ready define.
---
mm/kasan/common.c | 3 +++
mm/kasan/generic.c | 3 +++
mm/kasan/kasan.h | 6 ++++++
mm/kasan/shadow.c | 6 ++++++
4 files changed, 18 insertions(+)
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 10177cc26d06..2baf121fb8c5 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -331,6 +331,9 @@ static inline bool ____kasan_slab_free(struct kmem_cache *cache, void *object,
u8 tag;
void *tagged_object;
+ if (!kasan_arch_is_ready())
+ return false;
+
tag = get_tag(object);
tagged_object = object;
object = kasan_reset_tag(object);
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 53cbf28859b5..c3f5ba7a294a 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -163,6 +163,9 @@ static __always_inline bool check_region_inline(unsigned long addr,
size_t size, bool write,
unsigned long ret_ip)
{
+ if (!kasan_arch_is_ready())
+ return true;
+
if (unlikely(size == 0))
return true;
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 8f450bc28045..4dbc8def64f4 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -449,6 +449,12 @@ static inline void kasan_poison_last_granule(const void *address, size_t size) {
#endif /* CONFIG_KASAN_GENERIC */
+#ifndef kasan_arch_is_ready
+static inline bool kasan_arch_is_ready(void) { return true; }
+#elif !defined(CONFIG_KASAN_GENERIC) || !defined(CONFIG_KASAN_OUTLINE)
+#error kasan_arch_is_ready only works in KASAN generic outline mode!
+#endif
+
/*
* Exported functions for interfaces called from assembly or from generated
* code. Declarations here to avoid warning about missing declarations.
diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
index 082ee5b6d9a1..8d95ee52d019 100644
--- a/mm/kasan/shadow.c
+++ b/mm/kasan/shadow.c
@@ -73,6 +73,9 @@ void kasan_poison(const void *addr, size_t size, u8 value, bool init)
{
void *shadow_start, *shadow_end;
+ if (!kasan_arch_is_ready())
+ return;
+
/*
* Perform shadow offset calculation based on untagged address, as
* some of the callers (e.g. kasan_poison_object_data) pass tagged
@@ -99,6 +102,9 @@ EXPORT_SYMBOL(kasan_poison);
#ifdef CONFIG_KASAN_GENERIC
void kasan_poison_last_granule(const void *addr, size_t size)
{
+ if (!kasan_arch_is_ready())
+ return;
+
if (size & KASAN_GRANULE_MASK) {
u8 *shadow = (u8 *)kasan_mem_to_shadow(addr + size);
*shadow = size & KASAN_GRANULE_MASK;
--
2.30.2
^ permalink raw reply related
* [PATCH v16 1/4] kasan: allow an architecture to disable inline instrumentation
From: Daniel Axtens @ 2021-06-24 3:40 UTC (permalink / raw)
To: linux-kernel, linux-mm, kasan-dev, elver, akpm, andreyknvl
Cc: aneesh.kumar, linuxppc-dev, Daniel Axtens
In-Reply-To: <20210624034050.511391-1-dja@axtens.net>
For annoying architectural reasons, it's very difficult to support inline
instrumentation on powerpc64.*
Add a Kconfig flag to allow an arch to disable inline. (It's a bit
annoying to be 'backwards', but I'm not aware of any way to have
an arch force a symbol to be 'n', rather than 'y'.)
We also disable stack instrumentation in this case as it does things that
are functionally equivalent to inline instrumentation, namely adding
code that touches the shadow directly without going through a C helper.
* on ppc64 atm, the shadow lives in virtual memory and isn't accessible in
real mode. However, before we turn on virtual memory, we parse the device
tree to determine which platform and MMU we're running under. That calls
generic DT code, which is instrumented. Inline instrumentation in DT would
unconditionally attempt to touch the shadow region, which we won't have
set up yet, and would crash. We can make outline mode wait for the arch to
be ready, but we can't change what the compiler inserts for inline mode.
Reviewed-by: Marco Elver <elver@google.com>
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
lib/Kconfig.kasan | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index cffc2ebbf185..c3b228828a80 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -12,6 +12,13 @@ config HAVE_ARCH_KASAN_HW_TAGS
config HAVE_ARCH_KASAN_VMALLOC
bool
+config ARCH_DISABLE_KASAN_INLINE
+ bool
+ help
+ An architecture might not support inline instrumentation.
+ When this option is selected, inline and stack instrumentation are
+ disabled.
+
config CC_HAS_KASAN_GENERIC
def_bool $(cc-option, -fsanitize=kernel-address)
@@ -130,6 +137,7 @@ config KASAN_OUTLINE
config KASAN_INLINE
bool "Inline instrumentation"
+ depends on !ARCH_DISABLE_KASAN_INLINE
help
Compiler directly inserts code checking shadow memory before
memory accesses. This is faster than outline (in some workloads
@@ -141,6 +149,7 @@ endchoice
config KASAN_STACK
bool "Enable stack instrumentation (unsafe)" if CC_IS_CLANG && !COMPILE_TEST
depends on KASAN_GENERIC || KASAN_SW_TAGS
+ depends on !ARCH_DISABLE_KASAN_INLINE
default y if CC_IS_GCC
help
The LLVM stack address sanitizer has a know problem that
@@ -154,6 +163,9 @@ config KASAN_STACK
but clang users can still enable it for builds without
CONFIG_COMPILE_TEST. On gcc it is assumed to always be safe
to use and enabled by default.
+ If the architecture disables inline instrumentation, stack
+ instrumentation is also disabled as it adds inline-style
+ instrumentation that is run unconditionally.
config KASAN_SW_TAGS_IDENTIFY
bool "Enable memory corruption identification"
--
2.30.2
^ permalink raw reply related
* [PATCH v16 0/4] KASAN core changes for ppc64 radix KASAN
From: Daniel Axtens @ 2021-06-24 3:40 UTC (permalink / raw)
To: linux-kernel, linux-mm, kasan-dev, elver, akpm, andreyknvl
Cc: aneesh.kumar, linuxppc-dev, Daniel Axtens
Building on the work of Christophe, Aneesh and Balbir, I've ported
KASAN to 64-bit Book3S kernels running on the Radix MMU. I've been
trying this for a while, but we keep having collisions between the
kasan code in the mm tree and the code I want to put in to the ppc
tree.
This series just contains the kasan core changes that we need. These
can go in via the mm tree. I will then propose the powerpc changes for
a later cycle. (The most recent RFC for the powerpc changes is in the
v12 series at
https://lore.kernel.org/linux-mm/20210615014705.2234866-1-dja@axtens.net/
)
v16 applies to next-20210622. There should be no noticeable changes to
other platforms.
Changes since v15: Review comments from Andrey. Thanks Andrey.
Changes since v14: Included a bunch of Reviewed-by:s, thanks
Christophe and Marco. Cleaned up the build time error #ifdefs, thanks
Christophe.
Changes since v13: move the MAX_PTR_PER_* definitions out of kasan and
into pgtable.h. Add a build time error to hopefully prevent any
confusion about when the new hook is applicable. Thanks Marco and
Christophe.
Changes since v12: respond to Marco's review comments - clean up the
help for ARCH_DISABLE_KASAN_INLINE, and add an arch readiness check to
the new granule poisioning function. Thanks Marco.
Daniel Axtens (4):
kasan: allow an architecture to disable inline instrumentation
kasan: allow architectures to provide an outline readiness check
mm: define default MAX_PTRS_PER_* in include/pgtable.h
kasan: use MAX_PTRS_PER_* for early shadow tables
arch/s390/include/asm/pgtable.h | 2 --
include/asm-generic/pgtable-nop4d.h | 1 -
include/linux/kasan.h | 6 +++---
include/linux/pgtable.h | 22 ++++++++++++++++++++++
lib/Kconfig.kasan | 12 ++++++++++++
mm/kasan/common.c | 3 +++
mm/kasan/generic.c | 3 +++
mm/kasan/init.c | 6 +++---
mm/kasan/kasan.h | 6 ++++++
mm/kasan/shadow.c | 6 ++++++
10 files changed, 58 insertions(+), 9 deletions(-)
--
2.30.2
^ permalink raw reply
* Re: [RFC PATCH 8/8] powerpc/papr_scm: Use FORM2 associativity details
From: David Gibson @ 2021-06-24 3:16 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <f88ac56a-646b-7c9b-1d01-5db38dc2a789@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3286 bytes --]
On Thu, Jun 17, 2021 at 04:29:01PM +0530, Aneesh Kumar K.V wrote:
> On 6/17/21 1:16 PM, David Gibson wrote:
> > On Tue, Jun 15, 2021 at 12:35:17PM +0530, Aneesh Kumar K.V wrote:
> > > David Gibson <david@gibson.dropbear.id.au> writes:
> > >
> > > > On Tue, Jun 15, 2021 at 11:27:50AM +0530, Aneesh Kumar K.V wrote:
> > > > > David Gibson <david@gibson.dropbear.id.au> writes:
>
> ...
>
> > > > It's weird to me that you'd want to consider them in different nodes
> > > > for those different purposes.
> > >
> > >
> > > --------------------------------------
> > > | NUMA node0 |
> > > | ProcA -----> MEMA |
> > > | | |
> > > | | |
> > > | -------------------> PMEMB |
> > > | |
> > > ---------------------------------------
> > >
> > > ---------------------------------------
> > > | NUMA node1 |
> > > | |
> > > | ProcB -------> MEMC |
> > > | | |
> > > | -------------------> PMEMD |
> > > | |
> > > | |
> > > ---------------------------------------
> > >
> > > For a topology like the above application running of ProcA wants to find out
> > > persistent memory mount local to its NUMA node. Hence when using it as
> > > pmem fsdax mount or devdax device we want PMEMB to have associativity
> > > of NUMA node0 and PMEMD to have associativity of NUMA node 1. But when
> > > we want to use it as memory using dax kmem driver, we want both PMEMB
> > > and PMEMD to appear as memory only NUMA node at a distance that is
> > > derived based on the latency of the media.
> >
> > I'm still not understanding why the latency we care about is different
> > in the two cases. Can you give an example of when this would result
> > in different actual node assignments for the two different cases?
> >
>
> In the above example in order allow use of PMEMB and PMEMD as memory only
> NUMA nodes
> we need platform to represent them in its own domainID. Let's assume that
> platform assigned id 40 and 41 and hence both PMEMB and PMEMD will have
> associativity array like below
>
> { 4, 6, 0} -> PROCA/MEMA
> { 4, 6, 40} -> PMEMB
> { 4, 6, 41} -> PMEMD
> { 4, 6, 1} -> PROCB/MEMB
>
> When we want to use this device PMEMB and PMEMD as fsdax/devdax devices, we
> essentially look for the first nearest online node. Which means both PMEMB
> and PMEMD will appear as devices attached to node0. That is not ideal for
> for many applications.
Not if you actually look at the distance table which tells you that
PMEMB is closer to node0 and PMEMD is closer to node1. That's exactly
what the distance table is for - making this information explicit,
rather than intuited from a confusing set of nested domains.
--
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 v4 4/7] powerpc/pseries: Consolidate DLPAR NUMA distance update
From: David Gibson @ 2021-06-24 3:11 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Nathan Lynch, nvdimm, Daniel Henrique Barboza, dan.j.williams,
linuxppc-dev
In-Reply-To: <20210617165105.574178-5-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 4131 bytes --]
On Thu, Jun 17, 2021 at 10:21:02PM +0530, Aneesh Kumar K.V wrote:
> The associativity details of the newly added resourced are collected from
> the hypervisor via "ibm,configure-connector" rtas call. Update the numa
> distance details of the newly added numa node after the above call. In
> later patch we will remove updating NUMA distance when we are looking
> for node id from associativity array.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
I think this patch and the next would be easier to review if merged
together. That would make the fact that this is (half of) a code
motion clearer.
> ---
> arch/powerpc/mm/numa.c | 41 +++++++++++++++++++
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 +
> .../platforms/pseries/hotplug-memory.c | 2 +
> arch/powerpc/platforms/pseries/pseries.h | 1 +
> 4 files changed, 46 insertions(+)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 0ec16999beef..645a95e3a7ea 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -287,6 +287,47 @@ int of_node_to_nid(struct device_node *device)
> }
> EXPORT_SYMBOL(of_node_to_nid);
>
> +static void __initialize_form1_numa_distance(const __be32 *associativity)
> +{
> + int i, nid;
> +
> + if (of_read_number(associativity, 1) >= primary_domain_index) {
> + nid = of_read_number(&associativity[primary_domain_index], 1);
> +
> + for (i = 0; i < max_domain_index; i++) {
> + const __be32 *entry;
> +
> + entry = &associativity[be32_to_cpu(distance_ref_points[i])];
> + distance_lookup_table[nid][i] = of_read_number(entry, 1);
> + }
> + }
> +}
> +
> +static void initialize_form1_numa_distance(struct device_node *node)
> +{
> + const __be32 *associativity;
> +
> + associativity = of_get_associativity(node);
> + if (!associativity)
> + return;
> +
> + __initialize_form1_numa_distance(associativity);
> + return;
> +}
> +
> +/*
> + * Used to update distance information w.r.t newly added node.
> + */
> +void update_numa_distance(struct device_node *node)
> +{
> + if (affinity_form == FORM0_AFFINITY)
> + return;
> + else if (affinity_form == FORM1_AFFINITY) {
> + initialize_form1_numa_distance(node);
> + return;
> + }
> +}
> +
> static int __init find_primary_domain_index(void)
> {
> int index;
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 7e970f81d8ff..778b6ab35f0d 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -498,6 +498,8 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
> return saved_rc;
> }
>
> + update_numa_distance(dn);
> +
> rc = dlpar_online_cpu(dn);
> if (rc) {
> saved_rc = rc;
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 8377f1f7c78e..0e602c3b01ea 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -180,6 +180,8 @@ static int update_lmb_associativity_index(struct drmem_lmb *lmb)
> return -ENODEV;
> }
>
> + update_numa_distance(lmb_node);
> +
> dr_node = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> if (!dr_node) {
> dlpar_free_cc_nodes(lmb_node);
> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> index 1f051a786fb3..663a0859cf13 100644
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -113,4 +113,5 @@ extern u32 pseries_security_flavor;
> void pseries_setup_security_mitigations(void);
> void pseries_lpar_read_hblkrm_characteristics(void);
>
> +void update_numa_distance(struct device_node *node);
> #endif /* _PSERIES_PSERIES_H */
--
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 v4 2/7] powerpc/pseries: rename distance_ref_points_depth to max_associativity_domain_index
From: David Gibson @ 2021-06-24 3:10 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Nathan Lynch, nvdimm, Daniel Henrique Barboza, dan.j.williams,
linuxppc-dev
In-Reply-To: <20210617165105.574178-3-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 4088 bytes --]
On Thu, Jun 17, 2021 at 10:21:00PM +0530, Aneesh Kumar K.V wrote:
> No functional change in this patch
I've been convinced of your other rename, but I'm not yet convinced
this one actually clarifies anything.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/mm/numa.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 8365b298ec48..132813dd1a6c 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -56,7 +56,7 @@ static int n_mem_addr_cells, n_mem_size_cells;
> static int form1_affinity;
>
> #define MAX_DISTANCE_REF_POINTS 4
> -static int distance_ref_points_depth;
> +static int max_associativity_domain_index;
> static const __be32 *distance_ref_points;
> static int distance_lookup_table[MAX_NUMNODES][MAX_DISTANCE_REF_POINTS];
>
> @@ -169,7 +169,7 @@ int cpu_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
>
> int i, index;
>
> - for (i = 0; i < distance_ref_points_depth; i++) {
> + for (i = 0; i < max_associativity_domain_index; i++) {
> index = be32_to_cpu(distance_ref_points[i]);
> if (cpu1_assoc[index] == cpu2_assoc[index])
> break;
> @@ -193,7 +193,7 @@ int __node_distance(int a, int b)
> if (!form1_affinity)
> return ((a == b) ? LOCAL_DISTANCE : REMOTE_DISTANCE);
>
> - for (i = 0; i < distance_ref_points_depth; i++) {
> + for (i = 0; i < max_associativity_domain_index; i++) {
> if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
> break;
>
> @@ -213,7 +213,7 @@ static void initialize_distance_lookup_table(int nid,
> if (!form1_affinity)
> return;
>
> - for (i = 0; i < distance_ref_points_depth; i++) {
> + for (i = 0; i < max_associativity_domain_index; i++) {
> const __be32 *entry;
>
> entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
> @@ -240,7 +240,7 @@ static int associativity_to_nid(const __be32 *associativity)
> nid = NUMA_NO_NODE;
>
> if (nid > 0 &&
> - of_read_number(associativity, 1) >= distance_ref_points_depth) {
> + of_read_number(associativity, 1) >= max_associativity_domain_index) {
> /*
> * Skip the length field and send start of associativity array
> */
> @@ -310,14 +310,14 @@ static int __init find_primary_domain_index(void)
> */
> distance_ref_points = of_get_property(root,
> "ibm,associativity-reference-points",
> - &distance_ref_points_depth);
> + &max_associativity_domain_index);
>
> if (!distance_ref_points) {
> dbg("NUMA: ibm,associativity-reference-points not found.\n");
> goto err;
> }
>
> - distance_ref_points_depth /= sizeof(int);
> + max_associativity_domain_index /= sizeof(int);
>
> if (firmware_has_feature(FW_FEATURE_OPAL) ||
> firmware_has_feature(FW_FEATURE_TYPE1_AFFINITY)) {
> @@ -328,7 +328,7 @@ static int __init find_primary_domain_index(void)
> if (form1_affinity) {
> index = of_read_number(distance_ref_points, 1);
> } else {
> - if (distance_ref_points_depth < 2) {
> + if (max_associativity_domain_index < 2) {
> printk(KERN_WARNING "NUMA: "
> "short ibm,associativity-reference-points\n");
> goto err;
> @@ -341,10 +341,10 @@ static int __init find_primary_domain_index(void)
> * Warn and cap if the hardware supports more than
> * MAX_DISTANCE_REF_POINTS domains.
> */
> - if (distance_ref_points_depth > MAX_DISTANCE_REF_POINTS) {
> + if (max_associativity_domain_index > MAX_DISTANCE_REF_POINTS) {
> printk(KERN_WARNING "NUMA: distance array capped at "
> "%d entries\n", MAX_DISTANCE_REF_POINTS);
> - distance_ref_points_depth = MAX_DISTANCE_REF_POINTS;
> + max_associativity_domain_index = MAX_DISTANCE_REF_POINTS;
> }
>
> of_node_put(root);
--
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 v4 7/7] powerpc/pseries: Add support for FORM2 associativity
From: David Gibson @ 2021-06-24 3:08 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Nathan Lynch, nvdimm, Daniel Henrique Barboza, dan.j.williams,
linuxppc-dev
In-Reply-To: <20210617165105.574178-8-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 22772 bytes --]
On Thu, Jun 17, 2021 at 10:21:05PM +0530, Aneesh Kumar K.V wrote:
> PAPR interface currently supports two different ways of communicating resource
> grouping details to the OS. These are referred to as Form 0 and Form 1
> associativity grouping. Form 0 is the older format and is now considered
> deprecated. This patch adds another resource grouping named FORM2.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> Documentation/powerpc/associativity.rst | 135 ++++++++++++++++++++
> arch/powerpc/include/asm/firmware.h | 3 +-
> arch/powerpc/include/asm/prom.h | 1 +
> arch/powerpc/kernel/prom_init.c | 3 +-
> arch/powerpc/mm/numa.c | 149 +++++++++++++++++++++-
> arch/powerpc/platforms/pseries/firmware.c | 1 +
> 6 files changed, 286 insertions(+), 6 deletions(-)
> create mode 100644 Documentation/powerpc/associativity.rst
>
> diff --git a/Documentation/powerpc/associativity.rst b/Documentation/powerpc/associativity.rst
> new file mode 100644
> index 000000000000..93be604ac54d
> --- /dev/null
> +++ b/Documentation/powerpc/associativity.rst
> @@ -0,0 +1,135 @@
> +============================
> +NUMA resource associativity
> +=============================
> +
> +Associativity represents the groupings of the various platform resources into
> +domains of substantially similar mean performance relative to resources outside
> +of that domain. Resources subsets of a given domain that exhibit better
> +performance relative to each other than relative to other resources subsets
> +are represented as being members of a sub-grouping domain. This performance
> +characteristic is presented in terms of NUMA node distance within the Linux kernel.
> +From the platform view, these groups are also referred to as domains.
> +
> +PAPR interface currently supports different ways of communicating these resource
> +grouping details to the OS. These are referred to as Form 0, Form 1 and Form2
> +associativity grouping. Form 0 is the older format and is now considered deprecated.
> +
> +Hypervisor indicates the type/form of associativity used via "ibm,arcitecture-vec-5 property".
> +Bit 0 of byte 5 in the "ibm,architecture-vec-5" property indicates usage of Form 0 or Form 1.
> +A value of 1 indicates the usage of Form 1 associativity. For Form 2 associativity
> +bit 2 of byte 5 in the "ibm,architecture-vec-5" property is used.
> +
> +Form 0
> +-----
> +Form 0 associativity supports only two NUMA distance (LOCAL and REMOTE).
> +
> +Form 1
> +-----
> +With Form 1 a combination of ibm,associativity-reference-points and ibm,associativity
> +device tree properties are used to determine the NUMA distance between resource groups/domains.
> +
> +The “ibm,associativity” property contains one or more lists of numbers (domainID)
> +representing the resource’s platform grouping domains.
> +
> +The “ibm,associativity-reference-points” property contains one or more list of numbers
> +(domainID index) that represents the 1 based ordinal in the associativity lists.
> +The list of domainID index represnets increasing hierachy of
> resource grouping.
Typo "represnets". Also s/hierachy/hierarchy/
> +
> +ex:
> +{ primary domainID index, secondary domainID index, tertiary domainID index.. }
> +Linux kernel uses the domainID at the primary domainID index as the NUMA node id.
> +Linux kernel computes NUMA distance between two domains by recursively comparing
> +if they belong to the same higher-level domains. For mismatch at every higher
> +level of the resource group, the kernel doubles the NUMA distance between the
> +comparing domains.
The Form1 description is still kinda confusing, but I don't really
care. Form1 *is* confusing, it's Form2 that I hope will be clearer.
> +
> +Form 2
> +-------
> +Form 2 associativity format adds separate device tree properties representing NUMA node distance
> +thereby making the node distance computation flexible. Form 2 also allows flexible primary
> +domain numbering. With numa distance computation now detached from the index value of
> +"ibm,associativity" property, Form 2 allows a large number of primary domain ids at the
> +same domainID index representing resource groups of different performance/latency characteristics.
So, see you've removed the special handling of secondary IDs for pmem
- big improvement, thanks. IIUC, in this revised version, for Form2
there's really no reason for ibm,associativity-reference-points to
have >1 entry. Is that right?
In Form2 everything revolves around the primary domain ID - so much
that I suggest we come up with a short name for it. How about just
"node id" since that's how Linux uses it.
> +Hypervisor indicates the usage of FORM2 associativity using bit 2 of byte 5 in the
> +"ibm,architecture-vec-5" property.
> +
> +"ibm,numa-lookup-index-table" property contains one or more list numbers representing
> +the domainIDs present in the system. The offset of the domainID in this property is considered
> +the domainID index.
The lookup-index-table is all about compactifying the representation
of the distance matrix. Because node ids are sparse, the matrix of
distances is also effectively sparse. You don't want to have a huge
matrix in the DT with mostly meaningless entries, so you use the
lookup table to compact the node ids.
It's a bit confusing though, because you now have two representations
of exactly the same information the "full" node id (== primary
domainID) and the "short" node id (==domainID lookup table offset).
I can see three ways to clarify this:
A) Give the short node id an explicit name rather than difficult to
parse "domainID index" or "domainID offset" which gets easily
muddled with other uses of index and offset. Use that name
throughout.
B) Eliminate the short ID entirely, and use an explicit sparse
matrix representation for the distance table e.g. a list of
(nodeA, nodeB, distance) tuples
That does have the disadvantage that it's not structurally
guaranteed that you have entries for every pair of "active" node
ids.
C) Eliminate the "long ID" entirely. In the Form2 world, is there
actually any point allowing sparse node ids? In this case you'd
have the node id read from associativity and use that directly to
index the distance table
> +prop-encoded-array: The number N of the domainIDs encoded as with encode-int, followed by
> +N domainID encoded as with encode-int
> +
> +For ex:
> +ibm,numa-lookup-index-table = {4, 0, 8, 250, 252}, domainID index
> for domainID 8 is 1.
As noted "domainID index" is confusing here, because it's different
from the "domainID index" values in reference-points.
> +
> +"ibm,numa-distance-table" property contains one or more list of numbers representing the NUMA
> +distance between resource groups/domains present in the system.
> +
> +prop-encoded-array: The number N of the distance values encoded as with encode-int, followed by
> +N distance values encoded as with encode-bytes. The max distance value we could encode is 255.
The N here always has to be the square of the N in the
lookup-index-table, yes? In which case do we actually need to include
it?
> +For ex:
> +ibm,numa-lookup-index-table = {3, 0, 8, 40}
> +ibm,numa-distance-table = {9, 10, 20, 80, 20, 10, 160, 80, 160, 10}
> +
> + | 0 8 40
> +--|------------
> + |
> +0 | 10 20 80
> + |
> +8 | 20 10 160
> + |
> +40| 80 160 10
> +
> +
> +"ibm,associativity" property for resources in node 0, 8 and 40
> +
> +{ 3, 6, 7, 0 }
> +{ 3, 6, 9, 8 }
> +{ 3, 6, 7, 40}
> +
> +With "ibm,associativity-reference-points" { 0x3 }
> +
> +Each resource (drcIndex) now also supports additional optional device tree properties.
> +These properties are marked optional because the platform can choose not to export
> +them and provide the system topology details using the earlier defined device tree
> +properties alone. The optional device tree properties are used when adding new resources
> +(DLPAR) and when the platform didn't provide the topology details of the domain which
> +contains the newly added resource during boot.
In what circumstances would you envisage a hot-add creating a new NUMA
node, as opposed to adding resources to an existing (possibly empty)
node? Do you need any provision for removing NUMA nodes again?
> +"ibm,numa-lookup-index" property contains a number representing the domainID index to be used
> +when building the NUMA distance of the numa node to which this resource belongs. This can
> +be looked at as the index at which this new domainID would have appeared in
> +"ibm,numa-lookup-index-table" if the domain was present during boot. The domainID
Again, confusing use of "index" where it's used both for offsets in
ibm,associativity properties and for offsets in ibm,numa-lookup-index-table
> +of the new resource can be obtained from the existing "ibm,associativity" property. This
> +can be used to build distance information of a newly onlined NUMA node via DLPAR operation.
> +The value is 1 based array index value.
> +
> +prop-encoded-array: An integer encoded as with encode-int specifying the domainID index
> +
> +"ibm,numa-distance" property contains one or more list of numbers presenting the NUMA distance
> +from this resource domain to other resources.
> +
> +prop-encoded-array: The number N of the distance values encoded as with encode-int, followed by
> +N distance values encoded as with encode-bytes. The max distance value we could encode is 255.
Again, does N have to equal 2 * (existing number of nodes + 1)? If so
you should say so, if not you should specify how incomplete
information is interpreted.
> +For ex:
> +ibm,associativity = { 4, 5, 10, 50}
> +ibm,numa-lookup-index = { 4 }
> +ibm,numa-distance = {8, 160, 255, 80, 10, 160, 255, 80, 10}
> +
> +resulting in a new toplogy as below.
> + | 0 8 40 50
> +--|------------------
> + |
> +0 | 10 20 80 160
> + |
> +8 | 20 10 160 255
> + |
> +40| 80 160 10 80
> + |
> +50| 160 255 80 10
> +
> diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
> index 60b631161360..97a3bd9ffeb9 100644
> --- a/arch/powerpc/include/asm/firmware.h
> +++ b/arch/powerpc/include/asm/firmware.h
> @@ -53,6 +53,7 @@
> #define FW_FEATURE_ULTRAVISOR ASM_CONST(0x0000004000000000)
> #define FW_FEATURE_STUFF_TCE ASM_CONST(0x0000008000000000)
> #define FW_FEATURE_RPT_INVALIDATE ASM_CONST(0x0000010000000000)
> +#define FW_FEATURE_FORM2_AFFINITY ASM_CONST(0x0000020000000000)
>
> #ifndef __ASSEMBLY__
>
> @@ -73,7 +74,7 @@ enum {
> FW_FEATURE_HPT_RESIZE | FW_FEATURE_DRMEM_V2 |
> FW_FEATURE_DRC_INFO | FW_FEATURE_BLOCK_REMOVE |
> FW_FEATURE_PAPR_SCM | FW_FEATURE_ULTRAVISOR |
> - FW_FEATURE_RPT_INVALIDATE,
> + FW_FEATURE_RPT_INVALIDATE | FW_FEATURE_FORM2_AFFINITY,
> FW_FEATURE_PSERIES_ALWAYS = 0,
> FW_FEATURE_POWERNV_POSSIBLE = FW_FEATURE_OPAL | FW_FEATURE_ULTRAVISOR,
> FW_FEATURE_POWERNV_ALWAYS = 0,
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index df9fec9d232c..5c80152e8f18 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -149,6 +149,7 @@ extern int of_read_drc_info_cell(struct property **prop,
> #define OV5_XCMO 0x0440 /* Page Coalescing */
> #define OV5_FORM1_AFFINITY 0x0580 /* FORM1 NUMA affinity */
> #define OV5_PRRN 0x0540 /* Platform Resource Reassignment */
> +#define OV5_FORM2_AFFINITY 0x0520 /* Form2 NUMA affinity */
> #define OV5_HP_EVT 0x0604 /* Hot Plug Event support */
> #define OV5_RESIZE_HPT 0x0601 /* Hash Page Table resizing */
> #define OV5_PFO_HW_RNG 0x1180 /* PFO Random Number Generator */
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 64b9593038a7..496fdac54c29 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1070,7 +1070,8 @@ static const struct ibm_arch_vec ibm_architecture_vec_template __initconst = {
> #else
> 0,
> #endif
> - .associativity = OV5_FEAT(OV5_FORM1_AFFINITY) | OV5_FEAT(OV5_PRRN),
> + .associativity = OV5_FEAT(OV5_FORM1_AFFINITY) | OV5_FEAT(OV5_PRRN) |
> + OV5_FEAT(OV5_FORM2_AFFINITY),
> .bin_opts = OV5_FEAT(OV5_RESIZE_HPT) | OV5_FEAT(OV5_HP_EVT),
> .micro_checkpoint = 0,
> .reserved0 = 0,
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index d32729f235b8..5a7d94960fb7 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -56,12 +56,17 @@ static int n_mem_addr_cells, n_mem_size_cells;
>
> #define FORM0_AFFINITY 0
> #define FORM1_AFFINITY 1
> +#define FORM2_AFFINITY 2
> static int affinity_form;
>
> #define MAX_DISTANCE_REF_POINTS 4
> static int max_associativity_domain_index;
> static const __be32 *distance_ref_points;
> static int distance_lookup_table[MAX_NUMNODES][MAX_DISTANCE_REF_POINTS];
> +static int numa_distance_table[MAX_NUMNODES][MAX_NUMNODES] = {
> + [0 ... MAX_NUMNODES - 1] = { [0 ... MAX_NUMNODES - 1] = -1 }
> +};
> +static int numa_id_index_table[MAX_NUMNODES];
>
> /*
> * Allocate node_to_cpumask_map based on number of available nodes
> @@ -166,6 +171,27 @@ static void unmap_cpu_from_node(unsigned long cpu)
> }
> #endif /* CONFIG_HOTPLUG_CPU || CONFIG_PPC_SPLPAR */
>
> +/*
> + * With FORM2 if we are not using logical domain ids, we will find
> + * both primary and seconday domains with same value. Hence always
> + * start comparison from secondary domains
IIUC, in this new draft, secondary domains are no longer a meaningful
thing in Form2, so this comment and code seem outdated.
> + */
> +static int __cpu_form2_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> +{
> + int dist = 0;
> +
> + int i, index;
> +
> + for (i = 1; i < max_associativity_domain_index; i++) {
> + index = be32_to_cpu(distance_ref_points[i]);
> + if (cpu1_assoc[index] == cpu2_assoc[index])
> + break;
> + dist++;
> + }
> +
> + return dist;
> +}
> +
> static int __cpu_form1_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> {
> int dist = 0;
> @@ -178,7 +204,6 @@ static int __cpu_form1_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> break;
> dist++;
> }
> -
> return dist;
> }
>
> @@ -186,8 +211,9 @@ int cpu_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> {
> /* We should not get called with FORM0 */
> VM_WARN_ON(affinity_form == FORM0_AFFINITY);
> -
> - return __cpu_form1_distance(cpu1_assoc, cpu2_assoc);
> + if (affinity_form == FORM1_AFFINITY)
> + return __cpu_form1_distance(cpu1_assoc, cpu2_assoc);
> + return __cpu_form2_distance(cpu1_assoc, cpu2_assoc);
> }
>
> /* must hold reference to node during call */
> @@ -201,7 +227,9 @@ int __node_distance(int a, int b)
> int i;
> int distance = LOCAL_DISTANCE;
>
> - if (affinity_form == FORM0_AFFINITY)
> + if (affinity_form == FORM2_AFFINITY)
> + return numa_distance_table[a][b];
> + else if (affinity_form == FORM0_AFFINITY)
> return ((a == b) ? LOCAL_DISTANCE : REMOTE_DISTANCE);
>
> for (i = 0; i < max_associativity_domain_index; i++) {
> @@ -303,15 +331,116 @@ static void initialize_form1_numa_distance(struct device_node *node)
>
> /*
> * Used to update distance information w.r.t newly added node.
> + * ibm,numa-lookup-index -> 4
> + * ibm,numa-distance -> {5, 20, 40, 60, 80, 10 }
> */
> void update_numa_distance(struct device_node *node)
> {
> + int i, nid, other_nid, other_nid_index = 0;
> + const __be32 *numa_indexp;
> + const __u8 *numa_distancep;
> + int numa_index, max_numa_index, numa_distance;
> +
> if (affinity_form == FORM0_AFFINITY)
> return;
> else if (affinity_form == FORM1_AFFINITY) {
> initialize_form1_numa_distance(node);
> return;
> }
> + /* FORM2 affinity */
> +
> + nid = of_node_to_nid_single(node);
> + if (nid == NUMA_NO_NODE)
> + return;
> +
> + /* Already initialized */
> + if (numa_distance_table[nid][nid] != -1)
IIUC, this should exactly correspond to the case where the new
resource lies in an existing NUMA node, yes?
> + return;
> + /*
> + * update node distance if not already populated.
> + */
> + numa_distancep = of_get_property(node, "ibm,numa-distance", NULL);
> + if (!numa_distancep)
> + return;
> +
> + numa_indexp = of_get_property(node, "ibm,numa-lookup-index", NULL);
> + if (!numa_indexp)
> + return;
> +
> + numa_index = of_read_number(numa_indexp, 1);
> + /*
> + * update the numa_id_index_table. Device tree look at index table as
> + * 1 based array indexing.
> + */
> + numa_id_index_table[numa_index - 1] = nid;
> +
> + max_numa_index = of_read_number((const __be32 *)numa_distancep, 1);
> + VM_WARN_ON(max_numa_index != 2 * numa_index);
You WARN_ON(), but you don't actually bail out to avoid indexing past
the end of the property below.
AFAICT none of this really works unless numa_index == (previous number
of numa nodes + 1), which makes all the use of these different
variables kind of confusing. If you had a variable that you just set
equal to the new number of numa nodes (previous number plus the one
being added), then I think you can replace all numa_index and
max_numa_index references with that and it will be clearer.
> + /* Skip the size which is encoded int */
> + numa_distancep += sizeof(__be32);
> +
> + /*
> + * First fill the distance information from other node to this node.
> + */
> + other_nid_index = 0;
> + for (i = 0; i < numa_index; i++) {
> + numa_distance = numa_distancep[i];
> + other_nid = numa_id_index_table[other_nid_index++];
> + numa_distance_table[other_nid][nid] = numa_distance;
> + }
> +
> + other_nid_index = 0;
> + for (; i < max_numa_index; i++) {
Again, splitting this loop and carrying i over seems a confusing way
to code this. It's basically two loops of N, one writing a row of the
distance matrix, one writing a column (other_nid will even go through
the same values in each loop).
> + numa_distance = numa_distancep[i];
> + other_nid = numa_id_index_table[other_nid_index++];
> + numa_distance_table[nid][other_nid] = numa_distance;
> + }
> +}
> +
> +/*
> + * ibm,numa-lookup-index-table= {N, domainid1, domainid2, ..... domainidN}
> + * ibm,numa-distance-table = { N, 1, 2, 4, 5, 1, 6, .... N elements}
> + */
> +static void initialize_form2_numa_distance_lookup_table(struct device_node *root)
> +{
> + const __u8 *numa_dist_table;
> + const __be32 *numa_lookup_index;
> + int numa_dist_table_length;
> + int max_numa_index, distance_index;
> + int i, curr_row = 0, curr_column = 0;
> +
> + numa_lookup_index = of_get_property(root, "ibm,numa-lookup-index-table", NULL);
> + max_numa_index = of_read_number(&numa_lookup_index[0], 1);
max_numa_index here has a different meaning to max_numa_index in the
previous function, which is pointlessly confusing.
> + /* first element of the array is the size and is encode-int */
> + numa_dist_table = of_get_property(root, "ibm,numa-distance-table", NULL);
> + numa_dist_table_length = of_read_number((const __be32 *)&numa_dist_table[0], 1);
> + /* Skip the size which is encoded int */
> + numa_dist_table += sizeof(__be32);
> +
> + pr_debug("numa_dist_table_len = %d, numa_dist_indexes_len = %d \n",
> + numa_dist_table_length, max_numa_index);
> +
> + for (i = 0; i < max_numa_index; i++)
> + /* +1 skip the max_numa_index in the property */
> + numa_id_index_table[i] = of_read_number(&numa_lookup_index[i + 1], 1);
> +
> +
> + VM_WARN_ON(numa_dist_table_length != max_numa_index * max_numa_index);
Again, you don't actually bail out in this case. And if it has to
have this value, what's the point of encoding it into the property.
> + for (distance_index = 0; distance_index < numa_dist_table_length; distance_index++) {
> + int nodeA = numa_id_index_table[curr_row];
> + int nodeB = numa_id_index_table[curr_column++];
You've already (sort of) verified that the distance table has size
N^2, in which case you can just to a simple two dimensional loop
rather than having to do ugly calculations of row and column.
> +
> + numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index];
> +
> + pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
> + if (curr_column >= max_numa_index) {
> + curr_row++;
> + /* reset the column */
> + curr_column = 0;
> + }
> + }
> }
>
> static int __init find_primary_domain_index(void)
> @@ -324,6 +453,9 @@ static int __init find_primary_domain_index(void)
> */
> if (firmware_has_feature(FW_FEATURE_OPAL)) {
> affinity_form = FORM1_AFFINITY;
> + } else if (firmware_has_feature(FW_FEATURE_FORM2_AFFINITY)) {
> + dbg("Using form 2 affinity\n");
> + affinity_form = FORM2_AFFINITY;
> } else if (firmware_has_feature(FW_FEATURE_FORM1_AFFINITY)) {
> dbg("Using form 1 affinity\n");
> affinity_form = FORM1_AFFINITY;
> @@ -368,8 +500,17 @@ static int __init find_primary_domain_index(void)
>
> index = of_read_number(&distance_ref_points[1], 1);
> } else {
> + /*
> + * Both FORM1 and FORM2 affinity find the primary domain details
> + * at the same offset.
> + */
> index = of_read_number(distance_ref_points, 1);
> }
> + /*
> + * If it is FORM2 also initialize the distance table here.
> + */
> + if (affinity_form == FORM2_AFFINITY)
> + initialize_form2_numa_distance_lookup_table(root);
>
> /*
> * Warn and cap if the hardware supports more than
> diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
> index 5d4c2bc20bba..f162156b7b68 100644
> --- a/arch/powerpc/platforms/pseries/firmware.c
> +++ b/arch/powerpc/platforms/pseries/firmware.c
> @@ -123,6 +123,7 @@ vec5_fw_features_table[] = {
> {FW_FEATURE_PRRN, OV5_PRRN},
> {FW_FEATURE_DRMEM_V2, OV5_DRMEM_V2},
> {FW_FEATURE_DRC_INFO, OV5_DRC_INFO},
> + {FW_FEATURE_FORM2_AFFINITY, OV5_FORM2_AFFINITY},
> };
>
> static void __init fw_vec5_feature_init(const char *vec5, unsigned long len)
--
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 v4 1/7] powerpc/pseries: rename min_common_depth to primary_domain_index
From: David Gibson @ 2021-06-24 1:46 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Nathan Lynch, nvdimm, Daniel Henrique Barboza, dan.j.williams,
linuxppc-dev
In-Reply-To: <20210617165105.574178-2-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 4977 bytes --]
On Thu, Jun 17, 2021 at 10:20:59PM +0530, Aneesh Kumar K.V wrote:
> No functional change in this patch.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> arch/powerpc/mm/numa.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index f2bf98bdcea2..8365b298ec48 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -51,7 +51,7 @@ EXPORT_SYMBOL(numa_cpu_lookup_table);
> EXPORT_SYMBOL(node_to_cpumask_map);
> EXPORT_SYMBOL(node_data);
>
> -static int min_common_depth;
> +static int primary_domain_index;
> static int n_mem_addr_cells, n_mem_size_cells;
> static int form1_affinity;
>
> @@ -232,8 +232,8 @@ static int associativity_to_nid(const __be32 *associativity)
> if (!numa_enabled)
> goto out;
>
> - if (of_read_number(associativity, 1) >= min_common_depth)
> - nid = of_read_number(&associativity[min_common_depth], 1);
> + if (of_read_number(associativity, 1) >= primary_domain_index)
> + nid = of_read_number(&associativity[primary_domain_index], 1);
>
> /* POWER4 LPAR uses 0xffff as invalid node */
> if (nid == 0xffff || nid >= nr_node_ids)
> @@ -284,9 +284,9 @@ int of_node_to_nid(struct device_node *device)
> }
> EXPORT_SYMBOL(of_node_to_nid);
>
> -static int __init find_min_common_depth(void)
> +static int __init find_primary_domain_index(void)
> {
> - int depth;
> + int index;
> struct device_node *root;
>
> if (firmware_has_feature(FW_FEATURE_OPAL))
> @@ -326,7 +326,7 @@ static int __init find_min_common_depth(void)
> }
>
> if (form1_affinity) {
> - depth = of_read_number(distance_ref_points, 1);
> + index = of_read_number(distance_ref_points, 1);
> } else {
> if (distance_ref_points_depth < 2) {
> printk(KERN_WARNING "NUMA: "
> @@ -334,7 +334,7 @@ static int __init find_min_common_depth(void)
> goto err;
> }
>
> - depth = of_read_number(&distance_ref_points[1], 1);
> + index = of_read_number(&distance_ref_points[1], 1);
> }
>
> /*
> @@ -348,7 +348,7 @@ static int __init find_min_common_depth(void)
> }
>
> of_node_put(root);
> - return depth;
> + return index;
>
> err:
> of_node_put(root);
> @@ -437,16 +437,16 @@ int of_drconf_to_nid_single(struct drmem_lmb *lmb)
> int nid = default_nid;
> int rc, index;
>
> - if ((min_common_depth < 0) || !numa_enabled)
> + if ((primary_domain_index < 0) || !numa_enabled)
> return default_nid;
>
> rc = of_get_assoc_arrays(&aa);
> if (rc)
> return default_nid;
>
> - if (min_common_depth <= aa.array_sz &&
> + if (primary_domain_index <= aa.array_sz &&
> !(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index < aa.n_arrays) {
> - index = lmb->aa_index * aa.array_sz + min_common_depth - 1;
> + index = lmb->aa_index * aa.array_sz + primary_domain_index - 1;
> nid = of_read_number(&aa.arrays[index], 1);
>
> if (nid == 0xffff || nid >= nr_node_ids)
> @@ -708,18 +708,18 @@ static int __init parse_numa_properties(void)
> return -1;
> }
>
> - min_common_depth = find_min_common_depth();
> + primary_domain_index = find_primary_domain_index();
>
> - if (min_common_depth < 0) {
> + if (primary_domain_index < 0) {
> /*
> - * if we fail to parse min_common_depth from device tree
> + * if we fail to parse primary_domain_index from device tree
> * mark the numa disabled, boot with numa disabled.
> */
> numa_enabled = false;
> - return min_common_depth;
> + return primary_domain_index;
> }
>
> - dbg("NUMA associativity depth for CPU/Memory: %d\n", min_common_depth);
> + dbg("NUMA associativity depth for CPU/Memory: %d\n", primary_domain_index);
>
> /*
> * Even though we connect cpus to numa domains later in SMP
> @@ -919,14 +919,14 @@ static void __init find_possible_nodes(void)
> goto out;
> }
>
> - max_nodes = of_read_number(&domains[min_common_depth], 1);
> + max_nodes = of_read_number(&domains[primary_domain_index], 1);
> for (i = 0; i < max_nodes; i++) {
> if (!node_possible(i))
> node_set(i, node_possible_map);
> }
>
> prop_length /= sizeof(int);
> - if (prop_length > min_common_depth + 2)
> + if (prop_length > primary_domain_index + 2)
> coregroup_enabled = 1;
>
> out:
> @@ -1259,7 +1259,7 @@ int cpu_to_coregroup_id(int cpu)
> goto out;
>
> index = of_read_number(associativity, 1);
> - if (index > min_common_depth + 1)
> + if (index > primary_domain_index + 1)
> return of_read_number(&associativity[index - 1], 1);
>
> out:
--
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 2/3] powerpc: Define swapper_pg_dir[] in C
From: Daniel Axtens @ 2021-06-24 2:36 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Benjamin Herrenschmidt,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87czsc21st.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Daniel Axtens <dja@axtens.net> writes:
>> Hi Christophe,
>>
>> This breaks booting a radix KVM guest with 4k pages for me:
>>
>> make pseries_le_defconfig
>> scripts/config -d CONFIG_PPC_64K_PAGES
>> scripts/config -e CONFIG_PPC_4K_PAGES
>> make vmlinux
>> sudo qemu-system-ppc64 -enable-kvm -M pseries -m 1G -nographic -vga none -smp 4 -cpu host -kernel vmlinux
>>
>> Boot hangs after printing 'Booting Linux via __start()' and qemu's 'info
>> registers' reports that it's stuck at the instruction fetch exception.
>>
>> My host is Power9, 64k page size radix, and
>> gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34
>>
>
> ...
>>> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
>>> index 730838c7ca39..79f2d1e61abd 100644
>>> --- a/arch/powerpc/kernel/head_64.S
>>> +++ b/arch/powerpc/kernel/head_64.S
>>> @@ -997,18 +997,3 @@ start_here_common:
>>> 0: trap
>>> EMIT_BUG_ENTRY 0b, __FILE__, __LINE__, 0
>>> .previous
>>> -
>>> -/*
>>> - * We put a few things here that have to be page-aligned.
>>> - * This stuff goes at the beginning of the bss, which is page-aligned.
>>> - */
>>> - .section ".bss"
>>> -/*
>>> - * pgd dir should be aligned to PGD_TABLE_SIZE which is 64K.
>>> - * We will need to find a better way to fix this
>>> - */
>>> - .align 16
>>> -
>>> - .globl swapper_pg_dir
>>> -swapper_pg_dir:
>>> - .space PGD_TABLE_SIZE
>
> This is now 4K aligned whereas it used to be 64K.
>
> This fixes it and is not completely ugly?
>
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index 1707ab580ee2..298469beaa90 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -28,7 +28,13 @@
> #include <asm/hugetlb.h>
> #include <asm/pte-walk.h>
>
> -pgd_t swapper_pg_dir[MAX_PTRS_PER_PGD] __page_aligned_bss;
> +#ifdef CONFIG_PPC64
> +#define PGD_ALIGN 0x10000
> +#else
> +#define PGD_ALIGN PAGE_SIZE
> +#endif
> +
> +pgd_t swapper_pg_dir[MAX_PTRS_PER_PGD] __section(".bss..page_aligned") __aligned(PGD_ALIGN);
The fix works for me, thank you.
Kind regards,
Daniel
>
> static inline int is_exec_fault(void)
> {
>
>
> cheers
^ permalink raw reply
* Re: [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
From: Qian Cai @ 2021-06-23 18:44 UTC (permalink / raw)
To: Will Deacon
Cc: heikki.krogerus, linux-devicetree, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, Frank Rowand, mingo,
jxgao, sstabellini, Saravana Kannan, Joerg Roedel,
Rafael J . Wysocki, Christoph Hellwig, Bartosz Golaszewski,
bskeggs, linux-pci, xen-devel, Marek Szyprowski, Dan Williams,
matthew.auld, Nicolas Boichat, thomas.hellstrom, Jim Quinlan,
Konrad Rzeszutek Wilk, intel-gfx, maarten.lankhorst, Robin Murphy,
jani.nikula, Rob Herring, rodrigo.vivi, bhelgaas, Claire Chang,
boris.ostrovsky, Andy Shevchenko, jgross, airlied, Thierry Reding,
Greg KH, Randy Dunlap, lkml, list@263.net:IOMMU DRIVERS, daniel,
xypron.glpk, thomas.lendacky, linuxppc-dev, bauerman
In-Reply-To: <20210623183736.GA472@willie-the-truck>
On 6/23/2021 2:37 PM, Will Deacon wrote:
> On Wed, Jun 23, 2021 at 12:39:29PM -0400, Qian Cai wrote:
>>
>>
>> On 6/18/2021 11:40 PM, Claire Chang wrote:
>>> Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
>>> use it to determine whether to bounce the data or not. This will be
>>> useful later to allow for different pools.
>>>
>>> Signed-off-by: Claire Chang <tientzu@chromium.org>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Tested-by: Stefano Stabellini <sstabellini@kernel.org>
>>> Tested-by: Will Deacon <will@kernel.org>
>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> Reverting the rest of the series up to this patch fixed a boot crash with NVMe on today's linux-next.
>
> Hmm, so that makes patch 7 the suspicious one, right?
Will, no. It is rather patch #6 (this patch). Only the patch from #6 to #12 were reverted to fix the issue. Also, looking at this offset of the crash,
pc : dma_direct_map_sg+0x304/0x8f0
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
>
> Looking at that one more closely, it looks like swiotlb_find_slots() takes
> 'alloc_size + offset' as its 'alloc_size' parameter from
> swiotlb_tbl_map_single() and initialises 'mem->slots[i].alloc_size' based
> on 'alloc_size + offset', which looks like a change in behaviour from the
> old code, which didn't include the offset there.
>
> swiotlb_release_slots() then adds the offset back on afaict, so we end up
> accounting for it twice and possibly unmap more than we're supposed to?
>
> Will
>
^ permalink raw reply
* Re: [PATCH v14 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
From: Will Deacon @ 2021-06-23 18:37 UTC (permalink / raw)
To: Qian Cai
Cc: heikki.krogerus, linux-devicetree, peterz, joonas.lahtinen,
dri-devel, chris, grant.likely, paulus, Frank Rowand, mingo,
jxgao, sstabellini, Saravana Kannan, Joerg Roedel,
Rafael J . Wysocki, Christoph Hellwig, Bartosz Golaszewski,
bskeggs, linux-pci, xen-devel, Marek Szyprowski, Dan Williams,
matthew.auld, Nicolas Boichat, thomas.hellstrom, Jim Quinlan,
Konrad Rzeszutek Wilk, intel-gfx, maarten.lankhorst, Robin Murphy,
jani.nikula, Rob Herring, rodrigo.vivi, bhelgaas, Claire Chang,
boris.ostrovsky, Andy Shevchenko, jgross, airlied, Thierry Reding,
Greg KH, Randy Dunlap, lkml, list@263.net:IOMMU DRIVERS, daniel,
xypron.glpk, thomas.lendacky, linuxppc-dev, bauerman
In-Reply-To: <76c3343d-72e5-9df3-8924-5474ee698ef4@quicinc.com>
On Wed, Jun 23, 2021 at 12:39:29PM -0400, Qian Cai wrote:
>
>
> On 6/18/2021 11:40 PM, Claire Chang wrote:
> > Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
> > use it to determine whether to bounce the data or not. This will be
> > useful later to allow for different pools.
> >
> > Signed-off-by: Claire Chang <tientzu@chromium.org>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Tested-by: Stefano Stabellini <sstabellini@kernel.org>
> > Tested-by: Will Deacon <will@kernel.org>
> > Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>
> Reverting the rest of the series up to this patch fixed a boot crash with NVMe on today's linux-next.
Hmm, so that makes patch 7 the suspicious one, right?
Looking at that one more closely, it looks like swiotlb_find_slots() takes
'alloc_size + offset' as its 'alloc_size' parameter from
swiotlb_tbl_map_single() and initialises 'mem->slots[i].alloc_size' based
on 'alloc_size + offset', which looks like a change in behaviour from the
old code, which didn't include the offset there.
swiotlb_release_slots() then adds the offset back on afaict, so we end up
accounting for it twice and possibly unmap more than we're supposed to?
Will
^ permalink raw reply
* Re: [powerpc][next-20210621] WARNING at kernel/sched/fair.c:3277 during boot
From: Odin Ugedal @ 2021-06-23 17:37 UTC (permalink / raw)
To: Vincent Guittot
Cc: Sachin Sant, linuxppc-dev, Linux Next Mailing List, Odin Ugedal,
open list
In-Reply-To: <CAKfTPtDFUBe+qD9z0YYb7yyup_mhdNNX+zopFwcnyh+G41viAw@mail.gmail.com>
ons. 23. jun. 2021 kl. 19:27 skrev Vincent Guittot <vincent.guittot@linaro.org>:
>
> On Wed, 23 Jun 2021 at 18:55, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Wed, 23 Jun 2021 at 18:46, Sachin Sant <sachinp@linux.vnet.ibm.com> wrote:
> > >
> > >
> > > > Ok. This becomes even more weird. Could you share your config file and more details about
> > > > you setup ?
> > > >
> > > > Have you applied the patch below ?
> > > > https://lore.kernel.org/lkml/20210621174330.11258-1-vincent.guittot@linaro.org/
> > > >
> > > > Regarding the load_avg warning, I can see possible problem during attach. Could you add
> > > > the patch below. The load_avg warning seems to happen during boot and sched_entity
> > > > creation.
> > > >
> > >
> > > Here is a summary of my testing.
> > >
> > > I have a POWER box with PowerVM hypervisor. On this box I have a logical partition(LPAR) or guest
> > > (allocated with 32 cpus 90G memory) running linux-next.
> > >
> > > I started with a clean slate.
> > > Moved to linux-next 5.13.0-rc7-next-20210622 as base code.
> > > Applied patch #1 from Vincent which contains changes to dequeue_load_avg()
> > > Applied patch #2 from Vincent which contains changes to enqueue_load_avg()
> > > Applied patch #3 from Vincent which contains changes to attach_entity_load_avg()
> > > Applied patch #4 from https://lore.kernel.org/lkml/20210621174330.11258-1-vincent.guittot@linaro.org/
> > >
> > > With these changes applied I was still able to recreate the issue. I could see kernel warning
> > > during boot.
> > >
> > > I then applied patch #5 from Odin which contains changes to update_cfs_rq_load_avg()
> > >
> > > With all the 5 patches applied I was able to boot the kernel without any warning messages.
> > > I also ran scheduler related tests from ltp (./runltp -f sched) . All tests including cfs_bandwidth01
> > > ran successfully. No kernel warnings were observed.
> >
> > ok so Odin's patch fixes the problem which highlights that we
> > overestimate _sum or don't sync _avg and _sum correctly
> >
> > I'm going to look at this further
>
> The problem is "_avg * divider" makes the assumption that all pending
> contrib are not null contributions whereas they can be null.
Yeah.
> Odin patch is the right way to fix this. Other patches should not be
> useful for your problem
Ack. As I see it, given how PELT works now, it is the only way to
mitigate it (without doing a lot of extra PELT stuff).
Will post it as a patch together with a proper message later today or tomorrow.
>
> >
> > >
> > > Have also attached .config in case it is useful. config has CONFIG_HZ_100=y
> >
> > Thanks, i will have a look
> >
> > >
> > > Thanks
> > > -Sachin
> > >
Thanks for reporting Sachin!
Thanks
Odin
^ permalink raw reply
* Re: [powerpc][next-20210621] WARNING at kernel/sched/fair.c:3277 during boot
From: Vincent Guittot @ 2021-06-23 17:27 UTC (permalink / raw)
To: Sachin Sant; +Cc: linuxppc-dev, Linux Next Mailing List, Odin Ugedal, open list
In-Reply-To: <CAKfTPtBKn27=jryS_sxsVb+0yHDze_PMcLuyFtDkDo0H9Nzqww@mail.gmail.com>
On Wed, 23 Jun 2021 at 18:55, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 23 Jun 2021 at 18:46, Sachin Sant <sachinp@linux.vnet.ibm.com> wrote:
> >
> >
> > > Ok. This becomes even more weird. Could you share your config file and more details about
> > > you setup ?
> > >
> > > Have you applied the patch below ?
> > > https://lore.kernel.org/lkml/20210621174330.11258-1-vincent.guittot@linaro.org/
> > >
> > > Regarding the load_avg warning, I can see possible problem during attach. Could you add
> > > the patch below. The load_avg warning seems to happen during boot and sched_entity
> > > creation.
> > >
> >
> > Here is a summary of my testing.
> >
> > I have a POWER box with PowerVM hypervisor. On this box I have a logical partition(LPAR) or guest
> > (allocated with 32 cpus 90G memory) running linux-next.
> >
> > I started with a clean slate.
> > Moved to linux-next 5.13.0-rc7-next-20210622 as base code.
> > Applied patch #1 from Vincent which contains changes to dequeue_load_avg()
> > Applied patch #2 from Vincent which contains changes to enqueue_load_avg()
> > Applied patch #3 from Vincent which contains changes to attach_entity_load_avg()
> > Applied patch #4 from https://lore.kernel.org/lkml/20210621174330.11258-1-vincent.guittot@linaro.org/
> >
> > With these changes applied I was still able to recreate the issue. I could see kernel warning
> > during boot.
> >
> > I then applied patch #5 from Odin which contains changes to update_cfs_rq_load_avg()
> >
> > With all the 5 patches applied I was able to boot the kernel without any warning messages.
> > I also ran scheduler related tests from ltp (./runltp -f sched) . All tests including cfs_bandwidth01
> > ran successfully. No kernel warnings were observed.
>
> ok so Odin's patch fixes the problem which highlights that we
> overestimate _sum or don't sync _avg and _sum correctly
>
> I'm going to look at this further
The problem is "_avg * divider" makes the assumption that all pending
contrib are not null contributions whereas they can be null.
Odin patch is the right way to fix this. Other patches should not be
useful for your problem
>
> >
> > Have also attached .config in case it is useful. config has CONFIG_HZ_100=y
>
> Thanks, i will have a look
>
> >
> > Thanks
> > -Sachin
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox