* Re: [PATCH v3 2/7] mm/memory_hotplug: Rename mhp_restrictions to mhp_params
From: Michal Hocko @ 2020-03-03 9:50 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-ia64, linux-sh, Peter Zijlstra, Catalin Marinas,
Dave Hansen, platform-driver-x86, linux-mm, Will Deacon,
Christoph Hellwig, linux-s390, David Hildenbrand, Ingo Molnar,
Dan Williams, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
linux-arm-kernel, Eric Badger, linux-kernel, Andrew Morton,
linuxppc-dev
In-Reply-To: <20200221182503.28317-3-logang@deltatee.com>
On Fri 21-02-20 11:24:58, Logan Gunthorpe wrote:
> The mhp_restrictions struct really doesn't specify anything resembling
> a restriction anymore so rename it to be mhp_params as it is a list
> of extended parameters.
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> arch/arm64/mm/mmu.c | 4 ++--
> arch/ia64/mm/init.c | 4 ++--
> arch/powerpc/mm/mem.c | 4 ++--
> arch/s390/mm/init.c | 6 +++---
> arch/sh/mm/init.c | 4 ++--
> arch/x86/mm/init_32.c | 4 ++--
> arch/x86/mm/init_64.c | 8 ++++----
> include/linux/memory_hotplug.h | 16 ++++++++--------
> mm/memory_hotplug.c | 8 ++++----
> mm/memremap.c | 8 ++++----
> 10 files changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 128f70852bf3..ee37bca8aba8 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr)
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> int arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> int flags = 0;
>
> @@ -1063,7 +1063,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> memblock_clear_nomap(start, size);
>
> return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> - restrictions);
> + params);
> }
> void arch_remove_memory(int nid, u64 start, u64 size,
> struct vmem_altmap *altmap)
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index b01d68a2d5d9..97bbc23ea1e3 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -670,13 +670,13 @@ mem_init (void)
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> int arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> unsigned long start_pfn = start >> PAGE_SHIFT;
> unsigned long nr_pages = size >> PAGE_SHIFT;
> int ret;
>
> - ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
> + ret = __add_pages(nid, start_pfn, nr_pages, params);
> if (ret)
> printk("%s: Problem encountered in __add_pages() as ret=%d\n",
> __func__, ret);
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index ef7b1119b2e2..b4bece53bec0 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -128,7 +128,7 @@ static void flush_dcache_range_chunked(unsigned long start, unsigned long stop,
> }
>
> int __ref arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> unsigned long start_pfn = start >> PAGE_SHIFT;
> unsigned long nr_pages = size >> PAGE_SHIFT;
> @@ -144,7 +144,7 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
> return -EFAULT;
> }
>
> - return __add_pages(nid, start_pfn, nr_pages, restrictions);
> + return __add_pages(nid, start_pfn, nr_pages, params);
> }
>
> void __ref arch_remove_memory(int nid, u64 start, u64 size,
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index ac44bd76db4b..e9e4a7abd0cc 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -268,20 +268,20 @@ device_initcall(s390_cma_mem_init);
> #endif /* CONFIG_CMA */
>
> int arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> unsigned long start_pfn = PFN_DOWN(start);
> unsigned long size_pages = PFN_DOWN(size);
> int rc;
>
> - if (WARN_ON_ONCE(restrictions->altmap))
> + if (WARN_ON_ONCE(params->altmap))
> return -EINVAL;
>
> rc = vmem_add_mapping(start, size);
> if (rc)
> return rc;
>
> - rc = __add_pages(nid, start_pfn, size_pages, restrictions);
> + rc = __add_pages(nid, start_pfn, size_pages, params);
> if (rc)
> vmem_remove_mapping(start, size);
> return rc;
> diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
> index d1b1ff2be17a..e5114c053364 100644
> --- a/arch/sh/mm/init.c
> +++ b/arch/sh/mm/init.c
> @@ -406,14 +406,14 @@ void __init mem_init(void)
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> int arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> unsigned long start_pfn = PFN_DOWN(start);
> unsigned long nr_pages = size >> PAGE_SHIFT;
> int ret;
>
> /* We only have ZONE_NORMAL, so this is easy.. */
> - ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
> + ret = __add_pages(nid, start_pfn, nr_pages, params);
> if (unlikely(ret))
> printk("%s: Failed, __add_pages() == %d\n", __func__, ret);
>
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 23df4885bbed..3ec3dac7c268 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -853,12 +853,12 @@ void __init mem_init(void)
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> int arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> unsigned long start_pfn = start >> PAGE_SHIFT;
> unsigned long nr_pages = size >> PAGE_SHIFT;
>
> - return __add_pages(nid, start_pfn, nr_pages, restrictions);
> + return __add_pages(nid, start_pfn, nr_pages, params);
> }
>
> void arch_remove_memory(int nid, u64 start, u64 size,
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index abbdecb75fad..87977a8bfbbf 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -844,11 +844,11 @@ static void update_end_of_memory_vars(u64 start, u64 size)
> }
>
> int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> int ret;
>
> - ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
> + ret = __add_pages(nid, start_pfn, nr_pages, params);
> WARN_ON_ONCE(ret);
>
> /* update max_pfn, max_low_pfn and high_memory */
> @@ -859,14 +859,14 @@ int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
> }
>
> int arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> unsigned long start_pfn = start >> PAGE_SHIFT;
> unsigned long nr_pages = size >> PAGE_SHIFT;
>
> init_memory_mapping(start, start + size);
>
> - return add_pages(nid, start_pfn, nr_pages, restrictions);
> + return add_pages(nid, start_pfn, nr_pages, params);
> }
>
> #define PAGE_INUSE 0xFD
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 69ff3037528d..c5df1b3dada0 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -54,10 +54,10 @@ enum {
> };
>
> /*
> - * Restrictions for the memory hotplug:
> - * altmap: alternative allocator for memmap array
> + * Extended parameters for memory hotplug:
> + * altmap: alternative allocator for memmap array (optional)
> */
> -struct mhp_restrictions {
> +struct mhp_params {
> struct vmem_altmap *altmap;
> };
>
> @@ -108,7 +108,7 @@ extern int restore_online_page_callback(online_page_callback_t callback);
> extern int try_online_node(int nid);
>
> extern int arch_add_memory(int nid, u64 start, u64 size,
> - struct mhp_restrictions *restrictions);
> + struct mhp_params *params);
> extern u64 max_mem_size;
>
> extern bool memhp_auto_online;
> @@ -126,17 +126,17 @@ extern void __remove_pages(unsigned long start_pfn, unsigned long nr_pages,
>
> /* reasonably generic interface to expand the physical pages */
> extern int __add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
> - struct mhp_restrictions *restrictions);
> + struct mhp_params *params);
>
> #ifndef CONFIG_ARCH_HAS_ADD_PAGES
> static inline int add_pages(int nid, unsigned long start_pfn,
> - unsigned long nr_pages, struct mhp_restrictions *restrictions)
> + unsigned long nr_pages, struct mhp_params *params)
> {
> - return __add_pages(nid, start_pfn, nr_pages, restrictions);
> + return __add_pages(nid, start_pfn, nr_pages, params);
> }
> #else /* ARCH_HAS_ADD_PAGES */
> int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
> - struct mhp_restrictions *restrictions);
> + struct mhp_params *params);
> #endif /* ARCH_HAS_ADD_PAGES */
>
> #ifdef CONFIG_NUMA
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 0a54ffac8c68..c69469e1b40e 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -299,11 +299,11 @@ static int check_hotplug_memory_addressable(unsigned long pfn,
> * add the new pages.
> */
> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> - struct mhp_restrictions *restrictions)
> + struct mhp_params *params)
> {
> int err;
> unsigned long nr, start_sec, end_sec;
> - struct vmem_altmap *altmap = restrictions->altmap;
> + struct vmem_altmap *altmap = params->altmap;
>
> err = check_hotplug_memory_addressable(pfn, nr_pages);
> if (err)
> @@ -993,7 +993,7 @@ static int online_memory_block(struct memory_block *mem, void *arg)
> */
> int __ref add_memory_resource(int nid, struct resource *res)
> {
> - struct mhp_restrictions restrictions = {};
> + struct mhp_params params = {};
> u64 start, size;
> bool new_node = false;
> int ret;
> @@ -1021,7 +1021,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
> new_node = ret;
>
> /* call arch's memory hotadd */
> - ret = arch_add_memory(nid, start, size, &restrictions);
> + ret = arch_add_memory(nid, start, size, ¶ms);
> if (ret < 0)
> goto error;
>
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 09b5b7adc773..6891a503a078 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -161,7 +161,7 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
> {
> struct resource *res = &pgmap->res;
> struct dev_pagemap *conflict_pgmap;
> - struct mhp_restrictions restrictions = {
> + struct mhp_params params = {
> /*
> * We do not want any optional features only our own memmap
> */
> @@ -275,7 +275,7 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
> */
> if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
> error = add_pages(nid, PHYS_PFN(res->start),
> - PHYS_PFN(resource_size(res)), &restrictions);
> + PHYS_PFN(resource_size(res)), ¶ms);
> } else {
> error = kasan_add_zero_shadow(__va(res->start), resource_size(res));
> if (error) {
> @@ -284,7 +284,7 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
> }
>
> error = arch_add_memory(nid, res->start, resource_size(res),
> - &restrictions);
> + ¶ms);
> }
>
> if (!error) {
> @@ -292,7 +292,7 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
>
> zone = &NODE_DATA(nid)->node_zones[ZONE_DEVICE];
> move_pfn_range_to_zone(zone, PHYS_PFN(res->start),
> - PHYS_PFN(resource_size(res)), restrictions.altmap);
> + PHYS_PFN(resource_size(res)), params.altmap);
> }
>
> mem_hotplug_done();
> --
> 2.20.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 3/7] x86/mm: Thread pgprot_t through init_memory_mapping()
From: Michal Hocko @ 2020-03-03 9:52 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: x86, linux-ia64, linux-sh, Peter Zijlstra, Catalin Marinas,
Dave Hansen, platform-driver-x86, linux-mm, H. Peter Anvin,
Will Deacon, Christoph Hellwig, linux-s390, David Hildenbrand,
Ingo Molnar, Dan Williams, Borislav Petkov, Andy Lutomirski,
Thomas Gleixner, linux-arm-kernel, Eric Badger, linux-kernel,
Andrew Morton, linuxppc-dev
In-Reply-To: <20200221182503.28317-4-logang@deltatee.com>
On Fri 21-02-20 11:24:59, Logan Gunthorpe wrote:
> In prepartion to support a pgprot_t argument for arch_add_memory().
>
> It's required to move the prototype of init_memory_mapping() seeing
> the original location came before the definition of pgprot_t.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> arch/x86/include/asm/page_types.h | 3 ---
> arch/x86/include/asm/pgtable.h | 3 +++
> arch/x86/kernel/amd_gart_64.c | 3 ++-
> arch/x86/mm/init.c | 9 +++++----
> arch/x86/mm/init_32.c | 3 ++-
> arch/x86/mm/init_64.c | 32 +++++++++++++++++--------------
> arch/x86/mm/mm_internal.h | 3 ++-
> arch/x86/platform/uv/bios_uv.c | 3 ++-
> 8 files changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> index c85e15010f48..bf7aa2e290ef 100644
> --- a/arch/x86/include/asm/page_types.h
> +++ b/arch/x86/include/asm/page_types.h
> @@ -73,9 +73,6 @@ static inline phys_addr_t get_max_mapped(void)
>
> bool pfn_range_is_mapped(unsigned long start_pfn, unsigned long end_pfn);
>
> -extern unsigned long init_memory_mapping(unsigned long start,
> - unsigned long end);
> -
> extern void initmem_init(void);
>
> #endif /* !__ASSEMBLY__ */
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 7e118660bbd9..48d6a5960f28 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -1046,6 +1046,9 @@ static inline void __meminit init_trampoline_default(void)
>
> void __init poking_init(void);
>
> +unsigned long init_memory_mapping(unsigned long start,
> + unsigned long end, pgprot_t prot);
> +
> # ifdef CONFIG_RANDOMIZE_MEMORY
> void __meminit init_trampoline(void);
> # else
> diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
> index 4e5f50236048..16133819415c 100644
> --- a/arch/x86/kernel/amd_gart_64.c
> +++ b/arch/x86/kernel/amd_gart_64.c
> @@ -744,7 +744,8 @@ int __init gart_iommu_init(void)
>
> start_pfn = PFN_DOWN(aper_base);
> if (!pfn_range_is_mapped(start_pfn, end_pfn))
> - init_memory_mapping(start_pfn<<PAGE_SHIFT, end_pfn<<PAGE_SHIFT);
> + init_memory_mapping(start_pfn<<PAGE_SHIFT, end_pfn<<PAGE_SHIFT,
> + PAGE_KERNEL);
>
> pr_info("PCI-DMA: using GART IOMMU.\n");
> iommu_size = check_iommu_size(info.aper_base, aper_size);
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index e7bb483557c9..1bba16c5742b 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -467,7 +467,7 @@ bool pfn_range_is_mapped(unsigned long start_pfn, unsigned long end_pfn)
> * the physical memory. To access them they are temporarily mapped.
> */
> unsigned long __ref init_memory_mapping(unsigned long start,
> - unsigned long end)
> + unsigned long end, pgprot_t prot)
> {
> struct map_range mr[NR_RANGE_MR];
> unsigned long ret = 0;
> @@ -481,7 +481,8 @@ unsigned long __ref init_memory_mapping(unsigned long start,
>
> for (i = 0; i < nr_range; i++)
> ret = kernel_physical_mapping_init(mr[i].start, mr[i].end,
> - mr[i].page_size_mask);
> + mr[i].page_size_mask,
> + prot);
>
> add_pfn_range_mapped(start >> PAGE_SHIFT, ret >> PAGE_SHIFT);
>
> @@ -521,7 +522,7 @@ static unsigned long __init init_range_memory_mapping(
> */
> can_use_brk_pgt = max(start, (u64)pgt_buf_end<<PAGE_SHIFT) >=
> min(end, (u64)pgt_buf_top<<PAGE_SHIFT);
> - init_memory_mapping(start, end);
> + init_memory_mapping(start, end, PAGE_KERNEL);
> mapped_ram_size += end - start;
> can_use_brk_pgt = true;
> }
> @@ -661,7 +662,7 @@ void __init init_mem_mapping(void)
> #endif
>
> /* the ISA range is always mapped regardless of memory holes */
> - init_memory_mapping(0, ISA_END_ADDRESS);
> + init_memory_mapping(0, ISA_END_ADDRESS, PAGE_KERNEL);
>
> /* Init the trampoline, possibly with KASLR memory offset */
> init_trampoline();
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 3ec3dac7c268..e25a4218e6ff 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -253,7 +253,8 @@ static inline int is_kernel_text(unsigned long addr)
> unsigned long __init
> kernel_physical_mapping_init(unsigned long start,
> unsigned long end,
> - unsigned long page_size_mask)
> + unsigned long page_size_mask,
> + pgprot_t prot)
> {
> int use_pse = page_size_mask == (1<<PG_LEVEL_2M);
> unsigned long last_map_addr = end;
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 87977a8bfbbf..9e7692080dda 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -585,7 +585,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned long paddr, unsigned long paddr_end,
> */
> static unsigned long __meminit
> phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
> - unsigned long page_size_mask, bool init)
> + unsigned long page_size_mask, pgprot_t _prot, bool init)
> {
> unsigned long pages = 0, paddr_next;
> unsigned long paddr_last = paddr_end;
> @@ -595,7 +595,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
> for (; i < PTRS_PER_PUD; i++, paddr = paddr_next) {
> pud_t *pud;
> pmd_t *pmd;
> - pgprot_t prot = PAGE_KERNEL;
> + pgprot_t prot = _prot;
>
> vaddr = (unsigned long)__va(paddr);
> pud = pud_page + pud_index(vaddr);
> @@ -644,9 +644,12 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
> if (page_size_mask & (1<<PG_LEVEL_1G)) {
> pages++;
> spin_lock(&init_mm.page_table_lock);
> +
> + prot = __pgprot(pgprot_val(prot) | __PAGE_KERNEL_LARGE);
> +
> set_pte_init((pte_t *)pud,
> pfn_pte((paddr & PUD_MASK) >> PAGE_SHIFT,
> - PAGE_KERNEL_LARGE),
> + prot),
> init);
> spin_unlock(&init_mm.page_table_lock);
> paddr_last = paddr_next;
> @@ -669,7 +672,7 @@ phys_pud_init(pud_t *pud_page, unsigned long paddr, unsigned long paddr_end,
>
> static unsigned long __meminit
> phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
> - unsigned long page_size_mask, bool init)
> + unsigned long page_size_mask, pgprot_t prot, bool init)
> {
> unsigned long vaddr, vaddr_end, vaddr_next, paddr_next, paddr_last;
>
> @@ -679,7 +682,7 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
>
> if (!pgtable_l5_enabled())
> return phys_pud_init((pud_t *) p4d_page, paddr, paddr_end,
> - page_size_mask, init);
> + page_size_mask, prot, init);
>
> for (; vaddr < vaddr_end; vaddr = vaddr_next) {
> p4d_t *p4d = p4d_page + p4d_index(vaddr);
> @@ -702,13 +705,13 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
> if (!p4d_none(*p4d)) {
> pud = pud_offset(p4d, 0);
> paddr_last = phys_pud_init(pud, paddr, __pa(vaddr_end),
> - page_size_mask, init);
> + page_size_mask, prot, init);
> continue;
> }
>
> pud = alloc_low_page();
> paddr_last = phys_pud_init(pud, paddr, __pa(vaddr_end),
> - page_size_mask, init);
> + page_size_mask, prot, init);
>
> spin_lock(&init_mm.page_table_lock);
> p4d_populate_init(&init_mm, p4d, pud, init);
> @@ -722,7 +725,7 @@ static unsigned long __meminit
> __kernel_physical_mapping_init(unsigned long paddr_start,
> unsigned long paddr_end,
> unsigned long page_size_mask,
> - bool init)
> + pgprot_t prot, bool init)
> {
> bool pgd_changed = false;
> unsigned long vaddr, vaddr_start, vaddr_end, vaddr_next, paddr_last;
> @@ -743,13 +746,13 @@ __kernel_physical_mapping_init(unsigned long paddr_start,
> paddr_last = phys_p4d_init(p4d, __pa(vaddr),
> __pa(vaddr_end),
> page_size_mask,
> - init);
> + prot, init);
> continue;
> }
>
> p4d = alloc_low_page();
> paddr_last = phys_p4d_init(p4d, __pa(vaddr), __pa(vaddr_end),
> - page_size_mask, init);
> + page_size_mask, prot, init);
>
> spin_lock(&init_mm.page_table_lock);
> if (pgtable_l5_enabled())
> @@ -778,10 +781,10 @@ __kernel_physical_mapping_init(unsigned long paddr_start,
> unsigned long __meminit
> kernel_physical_mapping_init(unsigned long paddr_start,
> unsigned long paddr_end,
> - unsigned long page_size_mask)
> + unsigned long page_size_mask, pgprot_t prot)
> {
> return __kernel_physical_mapping_init(paddr_start, paddr_end,
> - page_size_mask, true);
> + page_size_mask, prot, true);
> }
>
> /*
> @@ -796,7 +799,8 @@ kernel_physical_mapping_change(unsigned long paddr_start,
> unsigned long page_size_mask)
> {
> return __kernel_physical_mapping_init(paddr_start, paddr_end,
> - page_size_mask, false);
> + page_size_mask, PAGE_KERNEL,
> + false);
> }
>
> #ifndef CONFIG_NUMA
> @@ -864,7 +868,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> unsigned long start_pfn = start >> PAGE_SHIFT;
> unsigned long nr_pages = size >> PAGE_SHIFT;
>
> - init_memory_mapping(start, start + size);
> + init_memory_mapping(start, start + size, PAGE_KERNEL);
>
> return add_pages(nid, start_pfn, nr_pages, params);
> }
> diff --git a/arch/x86/mm/mm_internal.h b/arch/x86/mm/mm_internal.h
> index eeae142062ed..3f37b5c80bb3 100644
> --- a/arch/x86/mm/mm_internal.h
> +++ b/arch/x86/mm/mm_internal.h
> @@ -12,7 +12,8 @@ void early_ioremap_page_table_range_init(void);
>
> unsigned long kernel_physical_mapping_init(unsigned long start,
> unsigned long end,
> - unsigned long page_size_mask);
> + unsigned long page_size_mask,
> + pgprot_t prot);
> unsigned long kernel_physical_mapping_change(unsigned long start,
> unsigned long end,
> unsigned long page_size_mask);
> diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> index 607f58147311..c60255da5a6c 100644
> --- a/arch/x86/platform/uv/bios_uv.c
> +++ b/arch/x86/platform/uv/bios_uv.c
> @@ -352,7 +352,8 @@ void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
> if (type == EFI_MEMORY_MAPPED_IO)
> return ioremap(phys_addr, size);
>
> - last_map_pfn = init_memory_mapping(phys_addr, phys_addr + size);
> + last_map_pfn = init_memory_mapping(phys_addr, phys_addr + size,
> + PAGE_KERNEL);
> if ((last_map_pfn << PAGE_SHIFT) < phys_addr + size) {
> unsigned long top = last_map_pfn << PAGE_SHIFT;
> efi_ioremap(top, size - (top - phys_addr), type, attribute);
> --
> 2.20.1
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH 6/6] powerpc: powernv: no need to check return value of debugfs_create functions
From: Michael Ellerman @ 2020-03-03 9:52 UTC (permalink / raw)
To: Oliver O'Halloran, Greg Kroah-Hartman
Cc: Paul Mackerras, Sukadev Bhattiprolu, linuxppc-dev,
Linux Kernel Mailing List, Anju T Sudhakar
In-Reply-To: <CAOSf1CEKwjDkp-=SMjmJfQirxdGCkadougZbdDS6FK1muNNCZw@mail.gmail.com>
"Oliver O'Halloran" <oohall@gmail.com> writes:
> On Mon, Feb 10, 2020 at 12:12 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> When calling debugfs functions, there is no need to ever check the
>> return value. The function can work or not, but the code logic should
>> never do something different based on this.
>
> For memtrace debugfs is the only way to actually use the feature. It'd
> be nice if it still printed out *something* if it failed to create the
> files rather than just being mysteriously absent
That's true, but the current code doesn't actually do that anyway.
>> diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
>> index eb2e75dac369..d6d64f8718e6 100644
>> --- a/arch/powerpc/platforms/powernv/memtrace.c
>> +++ b/arch/powerpc/platforms/powernv/memtrace.c
>> @@ -187,11 +187,6 @@ static int memtrace_init_debugfs(void)
>>
>> snprintf(ent->name, 16, "%08x", ent->nid);
>> dir = debugfs_create_dir(ent->name, memtrace_debugfs_dir);
>> - if (!dir) {
>> - pr_err("Failed to create debugfs directory for node %d\n",
>> - ent->nid);
>> - return -1;
>> - }
debugfs_create_dir() doesn't return NULL on error, it returns
ERR_PTR(-ENOMEM), which will not trigger that pr_err().
So I've merged this and if someone wants to they can send a follow-up to
do proper error checking in memtrace.c
cheers
^ permalink raw reply
* Re: [PATCH 2/6] powerpc: kvm: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2020-03-03 9:58 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <87d09tsrf0.fsf@mpe.ellerman.id.au>
On Tue, Mar 03, 2020 at 08:45:23PM +1100, Michael Ellerman wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > On Tue, Mar 03, 2020 at 06:46:23PM +1100, Michael Ellerman wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> > When calling debugfs functions, there is no need to ever check the
> >> > return value. The function can work or not, but the code logic should
> >> > never do something different based on this.
> >>
> >> Except it does need to do something different, if the file was created
> >> it needs to be removed in the remove path.
> >>
> >> > diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
> >> > index bfe4f106cffc..8e4791c6f2af 100644
> >> > --- a/arch/powerpc/kvm/timing.c
> >> > +++ b/arch/powerpc/kvm/timing.c
> >> > @@ -207,19 +207,12 @@ static const struct file_operations kvmppc_exit_timing_fops = {
> >> > void kvmppc_create_vcpu_debugfs(struct kvm_vcpu *vcpu, unsigned int id)
> >> > {
> >> > static char dbg_fname[50];
> >> > - struct dentry *debugfs_file;
> >> >
> >> > snprintf(dbg_fname, sizeof(dbg_fname), "vm%u_vcpu%u_timing",
> >> > current->pid, id);
> >> > - debugfs_file = debugfs_create_file(dbg_fname, 0666,
> >> > - kvm_debugfs_dir, vcpu,
> >> > - &kvmppc_exit_timing_fops);
> >> > -
> >> > - if (!debugfs_file) {
> >> > - printk(KERN_ERR"%s: error creating debugfs file %s\n",
> >> > - __func__, dbg_fname);
> >> > - return;
> >> > - }
> >> > + debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir, vcpu,
> >> > + &kvmppc_exit_timing_fops);
> >> > +
> >> >
> >> > vcpu->arch.debugfs_exit_timing = debugfs_file;
> >
> > Ugh, you are right, how did I miss that? How is 0-day missing this?
> > It's been in my tree for a long time, odd.
>
> This code isn't enabled by default, or in any defconfig. So it's only
> allmodconfig that would trip it, I guess 0-day isn't doing powerpc
> allmodconfig builds.
>
> >> I squashed this in, which seems to work:
> ...
> >>
> >> void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
> >> {
> >> - if (vcpu->arch.debugfs_exit_timing) {
> >> + if (!IS_ERR_OR_NULL(vcpu->arch.debugfs_exit_timing)) {
> >> debugfs_remove(vcpu->arch.debugfs_exit_timing);
> >> vcpu->arch.debugfs_exit_timing = NULL;
> >> }
> >
> > No, this can just be:
> > debugfs_remove(vcpu->arch.debugfs_exit_timing);
> >
> > No need to check anything, just call it and the debugfs code can handle
> > it just fine.
>
> Oh duh, of course, I should have checked.
>
> I'd still like to NULL out the debugfs_exit_timing member, so I'll do:
>
> void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
> {
> debugfs_remove(vcpu->arch.debugfs_exit_timing);
> vcpu->arch.debugfs_exit_timing = NULL;
> }
Fair enough, but I doubt it ever matters :)
Thanks for the fixups, sorry for sending a broken patch, my fault.
greg k-h
^ permalink raw reply
* Re: eh_frame confusion
From: Michael Ellerman @ 2020-03-03 10:28 UTC (permalink / raw)
To: Naveen N. Rao, Linux Kbuild mailing list, LKML,
linuxppc-dev@lists.ozlabs.org, Rasmus Villemoes
In-Reply-To: <1583168442.ovqnxu16tp.naveen@linux.ibm.com>
"Naveen N. Rao" <naveen.n.rao@linux.ibm.com> writes:
> Rasmus Villemoes wrote:
>> I'm building a ppc32 kernel, and noticed that after upgrading from gcc-7
>> to gcc-8 all object files now end up having .eh_frame section. For
>> vmlinux, that's not a problem, because they all get discarded in
>> arch/powerpc/kernel/vmlinux.lds.S . However, they stick around in
>> modules, which doesn't seem to be useful - given that everything worked
>> just fine with gcc-7, and I don't see anything in the module loader that
>> handles .eh_frame.
>>
>> The reason I care is that my target has a rather tight rootfs budget,
>> and the .eh_frame section seem to occupy 10-30% of the file size
>> (obviously very depending on the particular module).
>>
>> Comparing the .foo.o.cmd files, I don't see change in options that might
>> explain this (there's a bunch of new -Wno-*, and the -mspe=no spelling
>> is apparently no longer supported in gcc-8). Both before and after, there's
>>
>> -fno-dwarf2-cfi-asm
>>
>> about which gcc's documentation says
>>
>> '-fno-dwarf2-cfi-asm'
>> Emit DWARF unwind info as compiler generated '.eh_frame' section
>> instead of using GAS '.cfi_*' directives.
>>
>> Looking into where that comes from got me even more confused, because
>> both arm and unicore32 say
>>
>> # Never generate .eh_frame
>> KBUILD_CFLAGS += $(call cc-option,-fno-dwarf2-cfi-asm)
>>
>> while the ppc32 case at hand says
>>
>> # FIXME: the module load should be taught about the additional relocs
>> # generated by this.
>> # revert to pre-gcc-4.4 behaviour of .eh_frame
>
> Michael opened a task to look into this recently and I had spent some
> time last week on this. The original commit/discussion adding
> -fno-dwarf2-cfi-asm refers to R_PPC64_REL32 relocations not being
> handled by our module loader:
> http://lkml.kernel.org/r/20090224065112.GA6690@bombadil.infradead.org
I opened that issue purely based on noticing the wart in the Makefile,
not because I'd actually tested it.
> However, that is now handled thanks to commit 9f751b82b491d:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f751b82b491d
Haha, written by me, what an idiot.
So the Makefile hack can presumably be dropped, because the module
loader can handle the relocations.
And then maybe we also want to turn off the unwind tables, but that
would be a separate patch.
> I did a test build and a simple module loaded fine, so I think
> -fno-dwarf2-cfi-asm is not required anymore, unless Michael has seen
> some breakages with it. Michael?
No, as I said above it was just reading the Makefile.
cheers
^ permalink raw reply
* Re: [PATCH 2/6] powerpc: kvm: no need to check return value of debugfs_create functions
From: Michael Ellerman @ 2020-03-03 10:32 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <20200303095849.GA1399072@kroah.com>
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Tue, Mar 03, 2020 at 08:45:23PM +1100, Michael Ellerman wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> > On Tue, Mar 03, 2020 at 06:46:23PM +1100, Michael Ellerman wrote:
>> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> >> > When calling debugfs functions, there is no need to ever check the
>> >> > return value. The function can work or not, but the code logic should
>> >> > never do something different based on this.
>> >>
>> >> Except it does need to do something different, if the file was created
>> >> it needs to be removed in the remove path.
>> >>
>> >> > diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
>> >> > index bfe4f106cffc..8e4791c6f2af 100644
>> >> > --- a/arch/powerpc/kvm/timing.c
>> >> > +++ b/arch/powerpc/kvm/timing.c
>> >> > @@ -207,19 +207,12 @@ static const struct file_operations kvmppc_exit_timing_fops = {
>> >> > void kvmppc_create_vcpu_debugfs(struct kvm_vcpu *vcpu, unsigned int id)
>> >> > {
>> >> > static char dbg_fname[50];
>> >> > - struct dentry *debugfs_file;
>> >> >
>> >> > snprintf(dbg_fname, sizeof(dbg_fname), "vm%u_vcpu%u_timing",
>> >> > current->pid, id);
>> >> > - debugfs_file = debugfs_create_file(dbg_fname, 0666,
>> >> > - kvm_debugfs_dir, vcpu,
>> >> > - &kvmppc_exit_timing_fops);
>> >> > -
>> >> > - if (!debugfs_file) {
>> >> > - printk(KERN_ERR"%s: error creating debugfs file %s\n",
>> >> > - __func__, dbg_fname);
>> >> > - return;
>> >> > - }
>> >> > + debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir, vcpu,
>> >> > + &kvmppc_exit_timing_fops);
>> >> > +
>> >> >
>> >> > vcpu->arch.debugfs_exit_timing = debugfs_file;
>> >
>> > Ugh, you are right, how did I miss that? How is 0-day missing this?
>> > It's been in my tree for a long time, odd.
>>
>> This code isn't enabled by default, or in any defconfig. So it's only
>> allmodconfig that would trip it, I guess 0-day isn't doing powerpc
>> allmodconfig builds.
>>
>> >> I squashed this in, which seems to work:
>> ...
>> >>
>> >> void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
>> >> {
>> >> - if (vcpu->arch.debugfs_exit_timing) {
>> >> + if (!IS_ERR_OR_NULL(vcpu->arch.debugfs_exit_timing)) {
>> >> debugfs_remove(vcpu->arch.debugfs_exit_timing);
>> >> vcpu->arch.debugfs_exit_timing = NULL;
>> >> }
>> >
>> > No, this can just be:
>> > debugfs_remove(vcpu->arch.debugfs_exit_timing);
>> >
>> > No need to check anything, just call it and the debugfs code can handle
>> > it just fine.
>>
>> Oh duh, of course, I should have checked.
>>
>> I'd still like to NULL out the debugfs_exit_timing member, so I'll do:
>>
>> void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
>> {
>> debugfs_remove(vcpu->arch.debugfs_exit_timing);
>> vcpu->arch.debugfs_exit_timing = NULL;
>> }
>
> Fair enough, but I doubt it ever matters :)
Yeah, but I'm paranoid and I have no way to test this code :)
> Thanks for the fixups, sorry for sending a broken patch, my fault.
No worries, we have too many CONFIG options.
cheers
^ permalink raw reply
* Re: [PATCH v3 17/27] powerpc/powernv/pmem: Implement the Read Error Log command
From: Frederic Barrat @ 2020-03-03 10:36 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <20200221032720.33893-18-alastair@au1.ibm.com>
Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> From: Alastair D'Silva <alastair@d-silva.org>
>
> The read error log command extracts information from the controller's
> internal error log.
>
> This patch exposes this information in 2 ways:
> - During probe, if an error occurs & a log is available, print it to the
> console
> - After probe, make the error log available to userspace via an IOCTL.
> Userspace is notified of pending error logs in a later patch
> ("powerpc/powernv/pmem: Forward events to userspace")
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> arch/powerpc/platforms/powernv/pmem/ocxl.c | 269 ++++++++++++++++++
> .../platforms/powernv/pmem/ocxl_internal.h | 1 +
> include/uapi/nvdimm/ocxl-pmem.h | 46 +++
> 3 files changed, 316 insertions(+)
> create mode 100644 include/uapi/nvdimm/ocxl-pmem.h
>
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> index 63109a870d2c..2b64504f9129 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> @@ -447,10 +447,219 @@ static int file_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> +/**
> + * error_log_header_parse() - Parse the first 64 bits of the error log command response
> + * @ocxlpmem: the device metadata
> + * @length: out, returns the number of bytes in the response (excluding the 64 bit header)
> + */
> +static int error_log_header_parse(struct ocxlpmem *ocxlpmem, u16 *length)
> +{
> + int rc;
> + u64 val;
> +
Empty line in the middle of declarations
> + u16 data_identifier;
> + u32 data_length;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + data_identifier = val >> 48;
> + data_length = val & 0xFFFF;
> +
> + if (data_identifier != 0x454C) { // 'EL'
> + dev_err(&ocxlpmem->dev,
> + "Bad data identifier for error log data, expected 'EL', got '%2s' (%#x), data_length=%u\n",
> + (char *)&data_identifier,
> + (unsigned int)data_identifier, data_length);
> + return -EINVAL;
> + }
> +
> + *length = data_length;
> + return 0;
> +}
> +
> +static int error_log_offset_0x08(struct ocxlpmem *ocxlpmem,
> + u32 *log_identifier, u32 *program_ref_code)
> +{
> + int rc;
> + u64 val;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + 0x08,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + return rc;
> +
> + *log_identifier = val >> 32;
> + *program_ref_code = val & 0xFFFFFFFF;
> +
> + return 0;
> +}
> +
> +static int read_error_log(struct ocxlpmem *ocxlpmem,
> + struct ioctl_ocxl_pmem_error_log *log, bool buf_is_user)
> +{
> + u64 val;
> + u16 user_buf_length;
> + u16 buf_length;
> + u16 i;
> + int rc;
> +
> + if (log->buf_size % 8)
> + return -EINVAL;
> +
> + rc = ocxlpmem_chi(ocxlpmem, &val);
> + if (rc)
> + goto out;
"out" will unlock a mutex not yet taken.
> +
> + if (!(val & GLOBAL_MMIO_CHI_ELA))
> + return -EAGAIN;
> +
> + user_buf_length = log->buf_size;
> +
> + mutex_lock(&ocxlpmem->admin_command.lock);
> +
> + rc = admin_command_request(ocxlpmem, ADMIN_COMMAND_ERRLOG);
> + if (rc)
> + goto out;
> +
> + rc = admin_command_execute(ocxlpmem);
> + if (rc)
> + goto out;
> +
> + rc = admin_command_complete_timeout(ocxlpmem, ADMIN_COMMAND_ERRLOG);
> + if (rc < 0) {
> + dev_warn(&ocxlpmem->dev, "Read error log timed out\n");
> + goto out;
> + }
> +
> + rc = admin_response(ocxlpmem);
> + if (rc < 0)
> + goto out;
> + if (rc != STATUS_SUCCESS) {
> + warn_status(ocxlpmem, "Unexpected status from retrieve error log", rc);
> + goto out;
> + }
> +
> +
> + rc = error_log_header_parse(ocxlpmem, &log->buf_size);
> + if (rc)
> + goto out;
> + // log->buf_size now contains the returned buffer size, not the user size
> +
> + rc = error_log_offset_0x08(ocxlpmem, &log->log_identifier,
> + &log->program_reference_code);
> + if (rc)
> + goto out;
Offset 0x08 gets a preferential treatment compared to 0x10 below and
it's not clear why.
I would create a subfonction which parses all the fields linearly.
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + 0x10,
> + OCXL_LITTLE_ENDIAN, &val);
> + if (rc)
> + goto out;
> +
> + log->error_log_type = val >> 56;
> + log->action_flags = (log->error_log_type == OCXL_PMEM_ERROR_LOG_TYPE_GENERAL) ?
> + (val >> 32) & 0xFFFFFF : 0;
> + log->power_on_seconds = val & 0xFFFFFFFF;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + 0x18,
> + OCXL_LITTLE_ENDIAN, &log->timestamp);
> + if (rc)
> + goto out;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + 0x20,
> + OCXL_HOST_ENDIAN, &log->wwid[0]);
A bit of a moot point, but is there a reason why some of those MMIO ops
use OCXL_LITTLE_ENDIAN and the others OCXL_HOST_ENDIAN?
> + if (rc)
> + goto out;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + 0x28,
> + OCXL_HOST_ENDIAN, &log->wwid[1]);
> + if (rc)
> + goto out;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + 0x30,
> + OCXL_HOST_ENDIAN, (u64 *)log->fw_revision);
> + if (rc)
> + goto out;
> + log->fw_revision[8] = '\0';
> +
> + buf_length = (user_buf_length < log->buf_size) ?
> + user_buf_length : log->buf_size;
> + for (i = 0; i < buf_length + 0x48; i += 8) {
> + u64 val;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + i,
> + OCXL_HOST_ENDIAN, &val);
> + if (rc)
> + goto out;
> +
> + if (buf_is_user) {
> + if (copy_to_user(&log->buf[i], &val, sizeof(u64))) {
> + rc = -EFAULT;
> + goto out;
> + }
> + } else
> + log->buf[i] = val;
> + }
I think it could be a bit simplified by keeping the handling of the user
buffer out of this function. Always call it with a kernel buffer. And
have only one copy_to_user() call on the ioctl() path. You'd need to
allocate a kernel buf on the ioctl path, but you're already doing it on
the probe() path, so it should be doable to share code.
> +
> + rc = admin_response_handled(ocxlpmem);
> + if (rc)
> + goto out;
> +
> +out:
> + mutex_unlock(&ocxlpmem->admin_command.lock);
> + return rc;
> +
> +}
> +
> +static int ioctl_error_log(struct ocxlpmem *ocxlpmem,
> + struct ioctl_ocxl_pmem_error_log __user *uarg)
> +{
> + struct ioctl_ocxl_pmem_error_log args;
> + int rc;
> +
> + if (copy_from_user(&args, uarg, sizeof(args)))
> + return -EFAULT;
> +
> + rc = read_error_log(ocxlpmem, &args, true);
> + if (rc)
> + return rc;
> +
> + if (copy_to_user(uarg, &args, sizeof(args)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static long file_ioctl(struct file *file, unsigned int cmd, unsigned long args)
> +{
> + struct ocxlpmem *ocxlpmem = file->private_data;
> + int rc = -EINVAL;
> +
> + switch (cmd) {
> + case IOCTL_OCXL_PMEM_ERROR_LOG:
> + rc = ioctl_error_log(ocxlpmem,
> + (struct ioctl_ocxl_pmem_error_log __user *)args);
> + break;
> + }
> + return rc;
> +}
> +
> static const struct file_operations fops = {
> .owner = THIS_MODULE,
> .open = file_open,
> .release = file_release,
> + .unlocked_ioctl = file_ioctl,
> + .compat_ioctl = file_ioctl,
> };
>
> /**
> @@ -527,6 +736,60 @@ static int read_device_metadata(struct ocxlpmem *ocxlpmem)
> return 0;
> }
>
> +static const char *decode_error_log_type(u8 error_log_type)
> +{
> + switch (error_log_type) {
> + case 0x00:
> + return "general";
> + case 0x01:
> + return "predictive failure";
> + case 0x02:
> + return "thermal warning";
> + case 0x03:
> + return "data loss";
> + case 0x04:
> + return "health & performance";
> + default:
> + return "unknown";
> + }
> +}
> +
> +static void dump_error_log(struct ocxlpmem *ocxlpmem)
> +{
> + struct ioctl_ocxl_pmem_error_log log;
> + u32 buf_size;
> + u8 *buf;
> + int rc;
> +
> + if (ocxlpmem->admin_command.data_size == 0)
> + return;
> +
> + buf_size = ocxlpmem->admin_command.data_size - 0x48;
> + buf = kzalloc(buf_size, GFP_KERNEL);
> + if (!buf)
> + return;
> +
> + log.buf = buf;
> + log.buf_size = buf_size;
> +
> + rc = read_error_log(ocxlpmem, &log, false);
> + if (rc < 0)
> + goto out;
> +
> + dev_warn(&ocxlpmem->dev,
> + "OCXL PMEM Error log: WWID=0x%016llx%016llx LID=0x%x PRC=%x type=0x%x %s, Uptime=%u seconds timestamp=0x%llx\n",
> + log.wwid[0], log.wwid[1],
> + log.log_identifier, log.program_reference_code,
> + log.error_log_type,
> + decode_error_log_type(log.error_log_type),
> + log.power_on_seconds, log.timestamp);
> + print_hex_dump(KERN_WARNING, "buf", DUMP_PREFIX_OFFSET, 16, 1, buf,
> + log.buf_size, false);
dev_warn already logs a warning. Isn't KERN_DEBUG more appropriate for
the hex dump?
> +
> +out:
> + kfree(buf);
> +}
> +
> /**
> * probe_function0() - Set up function 0 for an OpenCAPI persistent memory device
> * This is important as it enables templates higher than 0 across all other functions,
> @@ -568,6 +831,7 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> struct ocxlpmem *ocxlpmem;
> int rc;
> u16 elapsed, timeout;
> + u64 chi;
>
> if (PCI_FUNC(pdev->devfn) == 0)
> return probe_function0(pdev);
> @@ -667,6 +931,11 @@ static int probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> return 0;
>
> err:
> + if (ocxlpmem &&
> + (ocxlpmem_chi(ocxlpmem, &chi) == 0) &&
> + (chi & GLOBAL_MMIO_CHI_ELA))
> + dump_error_log(ocxlpmem);
> +
> /*
> * Further cleanup is done in the release handler via free_ocxlpmem()
> * This allows us to keep the character device live to handle IOCTLs to
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> index d2d81fec7bb1..b953ee522ed4 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> @@ -5,6 +5,7 @@
> #include <linux/cdev.h>
> #include <misc/ocxl.h>
> #include <linux/libnvdimm.h>
> +#include <uapi/nvdimm/ocxl-pmem.h>
Can't we limit the extra include to ocxl.c?
Completely unrelated, but ocxl.c contains most of the code for this
driver. We should consider renaming it to ocxlpmem.c or something along
those lines, since it does a lot more than just interfacing with the
opencapi interface. And would avoid confusion with an other already
existing ocxl.c file.
> #include <linux/mm.h>
>
> #define LABEL_AREA_SIZE (1UL << PA_SECTION_SHIFT)
> diff --git a/include/uapi/nvdimm/ocxl-pmem.h b/include/uapi/nvdimm/ocxl-pmem.h
> new file mode 100644
> index 000000000000..b10f8ac0c20f
> --- /dev/null
> +++ b/include/uapi/nvdimm/ocxl-pmem.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/* Copyright 2017 IBM Corp. */
> +#ifndef _UAPI_OCXL_SCM_H
> +#define _UAPI_OCXL_SCM_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define OCXL_PMEM_ERROR_LOG_ACTION_RESET (1 << (32-32))
> +#define OCXL_PMEM_ERROR_LOG_ACTION_CHKFW (1 << (53-32))
> +#define OCXL_PMEM_ERROR_LOG_ACTION_REPLACE (1 << (54-32))
> +#define OCXL_PMEM_ERROR_LOG_ACTION_DUMP (1 << (55-32))
> +
> +#define OCXL_PMEM_ERROR_LOG_TYPE_GENERAL (0x00)
> +#define OCXL_PMEM_ERROR_LOG_TYPE_PREDICTIVE_FAILURE (0x01)
> +#define OCXL_PMEM_ERROR_LOG_TYPE_THERMAL_WARNING (0x02)
> +#define OCXL_PMEM_ERROR_LOG_TYPE_DATA_LOSS (0x03)
> +#define OCXL_PMEM_ERROR_LOG_TYPE_HEALTH_PERFORMANCE (0x04)
> +
> +struct ioctl_ocxl_pmem_error_log {
> + __u32 log_identifier; /* out */
> + __u32 program_reference_code; /* out */
> + __u32 action_flags; /* out, recommended course of action */
> + __u32 power_on_seconds; /* out, Number of seconds the controller has been on when the error occurred */
> + __u64 timestamp; /* out, relative time since the current IPL */
> + __u64 wwid[2]; /* out, the NAA formatted WWID associated with the controller */
> + char fw_revision[8+1]; /* out, firmware revision as null terminated text */
The 8+1 size will make the compiler add some padding here. Are we
confident that all the compilers, at least on powerpc, will do the same
thing and we can guarantee a kernel ABI? I would play it safe and have a
discussion with folks who understand compilers better.
> + __u16 buf_size; /* in/out, buffer size provided/required.
> + * If required is greater than provided, the buffer
> + * will be truncated to the amount provided. If its
> + * less, then only the required bytes will be populated.
> + * If it is 0, then there are no more error log entries.
> + */
> + __u8 error_log_type;
> + __u8 reserved1;
> + __u32 reserved2;
> + __u64 reserved3[2];
> + __u8 *buf; /* pointer to output buffer */
> +};
> +
> +/* ioctl numbers */
> +#define OCXL_PMEM_MAGIC 0x5C
Randomly picked?
See (and add entry in) Documentation/userspace-api/ioctl/ioctl-number.rst
Fred
> +/* SCM devices */
> +#define IOCTL_OCXL_PMEM_ERROR_LOG _IOWR(OCXL_PMEM_MAGIC, 0x01, struct ioctl_ocxl_pmem_error_log)
> +
> +#endif /* _UAPI_OCXL_SCM_H */
>
^ permalink raw reply
* Re: [PATCH] macintosh: therm_windtunnel: fix regression when instantiating devices
From: Michael Ellerman @ 2020-03-03 11:08 UTC (permalink / raw)
To: Wolfram Sang, linuxppc-dev
Cc: Mathieu Malaterre, Erhard Furtner, debian-powerpc, linux-i2c
In-Reply-To: <20200228170033.GB1130@ninjato>
Wolfram Sang <wsa@the-dreams.de> writes:
> On Tue, Feb 25, 2020 at 03:12:29PM +0100, Wolfram Sang wrote:
>> Removing attach_adapter from this driver caused a regression for at
>> least some machines. Those machines had the sensors described in their
>> DT, too, so they didn't need manual creation of the sensor devices. The
>> old code worked, though, because manual creation came first. Creation of
>> DT devices then failed later and caused error logs, but the sensors
>> worked nonetheless because of the manually created devices.
>>
>> When removing attach_adaper, manual creation now comes later and loses
>> the race. The sensor devices were already registered via DT, yet with
>> another binding, so the driver could not be bound to it.
>>
>> This fix refactors the code to remove the race and only manually creates
>> devices if there are no DT nodes present. Also, the DT binding is updated
>> to match both, the DT and manually created devices. Because we don't
>> know which device creation will be used at runtime, the code to start
>> the kthread is moved to do_probe() which will be called by both methods.
>>
>> Fixes: 3e7bed52719d ("macintosh: therm_windtunnel: drop using attach_adapter")
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=201723
>> Reported-by: Erhard Furtner <erhard_f@mailbox.org>
>> Tested-by: Erhard Furtner <erhard_f@mailbox.org>
>> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
>
> Applied to for-current, thanks!
Thanks.
cheers
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/powernv: Wire up OPAL address lookups
From: Michael Ellerman @ 2020-03-03 11:43 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
Cc: linux-arch, skiboot, linux-kernel, Nicholas Piggin
In-Reply-To: <20200228031027.271510-2-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> Use ARCH_HAS_ADDRESS_LOOKUP to look up the opal symbol table. This
> allows crashes and xmon debugging to print firmware symbols.
>
> Oops: System Reset, sig: 6 [#1]
> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.0-rc2-dirty #903
> NIP: 0000000030020434 LR: 000000003000378c CTR: 0000000030020414
> REGS: c0000000fffc3d70 TRAP: 0100 Not tainted (5.6.0-rc2-dirty)
> MSR: 9000000002101002 <SF,HV,VEC,ME,RI> CR: 28022284 XER: 20040000
> CFAR: 0000000030003788 IRQMASK: 3
> GPR00: 000000003000378c 0000000031c13c90 0000000030136200 c0000000012cfa10
> GPR04: c0000000012cfa10 0000000000000010 0000000000000000 0000000031c10060
> GPR08: c0000000012cfaaf 0000000030003640 0000000000000000 0000000000000001
> GPR12: 00000000300e0000 c000000001490000 0000000000000000 c00000000139c588
> GPR16: 0000000031c10000 c00000000125a900 0000000000000000 c0000000012076a8
> GPR20: c0000000012a3950 0000000000000001 0000000031c10060 c0000000012cfaaf
> GPR24: 0000000000000019 0000000030003640 0000000000000000 0000000000000000
> GPR28: 0000000000000010 c0000000012cfa10 0000000000000000 0000000000000000
> NIP [0000000030020434] .dummy_console_write_buffer_space+0x20/0x64 [OPAL]
> LR [000000003000378c] opal_entry+0x14c/0x17c [OPAL]
>
> This won't unwind the firmware stack (or its Linux caller) properly if
> firmware and kernel endians don't match, but that problem could be solved
> in powerpc's unwinder.
How well does this work if we're tracing opal calls at the time we oops :)
Though it looks like that's already fishy because we don't do anything
to disable tracing of opal_console_write().
I guess I'm a bit wary of adding numerous further opal calls in the oops
path, I'm sure the opal symbol lookup code is bug free, but still.
Could we instead suck in the opal symbols early on, and search them in
Linux? I suspect you've thought of that and rejected it, but it would be
good to document why.
cheers
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 497b7d0b2d7e..4d32b02d35e8 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -115,6 +115,7 @@ config PPC
> # Please keep this list sorted alphabetically.
> #
> select ARCH_32BIT_OFF_T if PPC32
> + select ARCH_HAS_ADDRESS_LOOKUP if PPC_POWERNV
> select ARCH_HAS_DEBUG_VIRTUAL
> select ARCH_HAS_DEVMEM_IS_ALLOWED
> select ARCH_HAS_ELF_RANDOMIZE
> diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
> index c1f25a760eb1..c3a2a797177a 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -214,7 +214,11 @@
> #define OPAL_SECVAR_GET 176
> #define OPAL_SECVAR_GET_NEXT 177
> #define OPAL_SECVAR_ENQUEUE_UPDATE 178
> -#define OPAL_LAST 178
> +#define OPAL_PHB_SET_OPTION 179
> +#define OPAL_PHB_GET_OPTION 180
Only pull in the calls you need for this patch.
> +#define OPAL_GET_SYMBOL 181
> +#define OPAL_LOOKUP_SYMBOL 182
> +#define OPAL_LAST 182
>
> #define QUIESCE_HOLD 1 /* Spin all calls at entry */
> #define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 9986ac34b8e2..ef2d9273f06f 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -312,6 +312,9 @@ s64 opal_mpipl_query_tag(enum opal_mpipl_tags tag, u64 *addr);
> s64 opal_signal_system_reset(s32 cpu);
> s64 opal_quiesce(u64 shutdown_type, s32 cpu);
>
> +int64_t opal_get_symbol(uint64_t addr, __be64 *symaddr, __be64 *symsize, char *namebuf, uint64_t buflen);
> +int64_t opal_lookup_symbol(const char *name, __be64 *symaddr, __be64 *symsize);
> +
> /* Internal functions */
> extern int early_init_dt_scan_opal(unsigned long node, const char *uname,
> int depth, void *data);
> diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
> index 5cd0f52d258f..ba11112d94df 100644
> --- a/arch/powerpc/platforms/powernv/opal-call.c
> +++ b/arch/powerpc/platforms/powernv/opal-call.c
> @@ -293,3 +293,5 @@ OPAL_CALL(opal_mpipl_query_tag, OPAL_MPIPL_QUERY_TAG);
> OPAL_CALL(opal_secvar_get, OPAL_SECVAR_GET);
> OPAL_CALL(opal_secvar_get_next, OPAL_SECVAR_GET_NEXT);
> OPAL_CALL(opal_secvar_enqueue_update, OPAL_SECVAR_ENQUEUE_UPDATE);
> +OPAL_CALL(opal_get_symbol, OPAL_GET_SYMBOL);
> +OPAL_CALL(opal_lookup_symbol, OPAL_LOOKUP_SYMBOL);
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 2b3dfd0b6cdd..fdf6c4e6f7f9 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -107,6 +107,46 @@ void opal_configure_cores(void)
> cur_cpu_spec->cpu_restore();
> }
>
> +const char *arch_address_lookup(unsigned long addr,
> + unsigned long *symbolsize,
> + unsigned long *offset,
> + char **modname, char *namebuf)
> +{
> + __be64 symaddr;
> + __be64 symsize;
> +
> + if (!firmware_has_feature(FW_FEATURE_OPAL))
> + return NULL;
> +
> + if (opal_get_symbol(addr, &symaddr, &symsize, namebuf,
> + cpu_to_be64(KSYM_NAME_LEN)) != OPAL_SUCCESS)
> + return NULL;
> +
> + *symbolsize = be64_to_cpu(symsize);
> + *offset = addr - be64_to_cpu(symaddr);
> + *modname = "OPAL";
> +
> + return namebuf;
> +}
> +
> +unsigned long arch_address_lookup_name(const char *name)
> +{
> + __be64 addr;
> + __be64 size;
> +
> + if (!firmware_has_feature(FW_FEATURE_OPAL))
> + return 0;
> +
> + /* opal: prefix allows lookup of symbols that clash with kernel */
> + if (!strncasecmp(name, "opal:", strlen("opal:")))
> + name += strlen("opal:");
> +
> + if (opal_lookup_symbol(name, &addr, &size) != OPAL_SUCCESS)
> + return 0;
> +
> + return be64_to_cpu(addr);
> +}
> +
> int __init early_init_dt_scan_opal(unsigned long node,
> const char *uname, int depth, void *data)
> {
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v4 1/8] ASoC: dt-bindings: fsl_asrc: Change asrc-width to asrc-format
From: Mark Brown @ 2020-03-03 12:47 UTC (permalink / raw)
To: Shengjiu Wang
Cc: Mark Rutland, Rob Herring, Linux-ALSA, Liam Girdwood, Timur Tabi,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
shawnguo, Shengjiu Wang, Xiubo Li, linux-kernel, Takashi Iwai,
Nicolin Chen, linux-imx, kernel, Fabio Estevam, linuxppc-dev,
s.hauer, linux-arm-kernel
In-Reply-To: <CAA+D8ANgECaz=tRtRwNP=jMXBD0XciAE0HUYROH8uuo03iDejg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 773 bytes --]
On Tue, Mar 03, 2020 at 11:59:30AM +0800, Shengjiu Wang wrote:
> On Tue, Mar 3, 2020 at 9:43 AM Rob Herring <robh@kernel.org> wrote:
> > > - - fsl,asrc-width : Defines a mutual sample width used by DPCM Back Ends.
> > > + - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> > > + Ends. The value is one of SNDRV_PCM_FORMAT_XX in
> > > + "include/uapi/sound/asound.h"
> > You can't just change properties. They are an ABI.
> I have updated all the things related with this ABI in this patch series.
> What else should I do?
Like Nicolin says you should continue to support the old stuff. The
kernel should work with people's out of tree DTs too so simply updating
everything in the tree isn't enough.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH] macintosh: windfarm: fix MODINFO regression
From: Wolfram Sang @ 2020-03-03 12:50 UTC (permalink / raw)
To: linuxppc-dev
Cc: Erhard Furtner, Wolfram Sang, Mathieu Malaterre, debian-powerpc,
linux-i2c, John Paul Adrian Glaubitz
Commit af503716ac14 made sure OF devices get an OF style modalias with
I2C events. It assumed all in-tree users were converted, yet it missed
some Macintosh drivers.
Add an OF module device table for all windfarm drivers to make them
automatically load again.
Fixes: af503716ac14 ("i2c: core: report OF style module alias for devices registered via OF")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=199471
Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Tested-by: Erhard Furtner <erhard_f@mailbox.org>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
This should also help with this: https://lists.debian.org/debian-powerpc/2020/01/msg00062.html
Some more testing would be appreciated because lm75 also has some code
changes I can't test myself obviusly.
By grepping, I found some more potential candidates (using a "MAC,"
prefix but not defining a OF MODULE DEVICE TABLE). Does someone know
about bugreports filed for those? I don't want to change them for no
reason:
drivers/macintosh/ams/ams-i2c.c
drivers/macintosh/therm_adt746x.c
sound/aoa/codecs/onyx.c
sound/aoa/codecs/tas.c
sound/ppc/keywest.c
Happy hacking,
Wolfram
drivers/macintosh/windfarm_ad7417_sensor.c | 7 +++++++
drivers/macintosh/windfarm_fcu_controls.c | 7 +++++++
drivers/macintosh/windfarm_lm75_sensor.c | 16 +++++++++++++++-
drivers/macintosh/windfarm_lm87_sensor.c | 7 +++++++
drivers/macintosh/windfarm_max6690_sensor.c | 7 +++++++
drivers/macintosh/windfarm_smu_sat.c | 7 +++++++
6 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/macintosh/windfarm_ad7417_sensor.c b/drivers/macintosh/windfarm_ad7417_sensor.c
index 125605987b44..e7dec328c7cf 100644
--- a/drivers/macintosh/windfarm_ad7417_sensor.c
+++ b/drivers/macintosh/windfarm_ad7417_sensor.c
@@ -312,9 +312,16 @@ static const struct i2c_device_id wf_ad7417_id[] = {
};
MODULE_DEVICE_TABLE(i2c, wf_ad7417_id);
+static const struct of_device_id wf_ad7417_of_id[] = {
+ { .compatible = "ad7417", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, wf_ad7417_of_id);
+
static struct i2c_driver wf_ad7417_driver = {
.driver = {
.name = "wf_ad7417",
+ .of_match_table = wf_ad7417_of_id,
},
.probe = wf_ad7417_probe,
.remove = wf_ad7417_remove,
diff --git a/drivers/macintosh/windfarm_fcu_controls.c b/drivers/macintosh/windfarm_fcu_controls.c
index 67daeec94b44..2470e5a725c8 100644
--- a/drivers/macintosh/windfarm_fcu_controls.c
+++ b/drivers/macintosh/windfarm_fcu_controls.c
@@ -580,9 +580,16 @@ static const struct i2c_device_id wf_fcu_id[] = {
};
MODULE_DEVICE_TABLE(i2c, wf_fcu_id);
+static const struct of_device_id wf_fcu_of_id[] = {
+ { .compatible = "fcu", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, wf_fcu_of_id);
+
static struct i2c_driver wf_fcu_driver = {
.driver = {
.name = "wf_fcu",
+ .of_match_table = wf_fcu_of_id,
},
.probe = wf_fcu_probe,
.remove = wf_fcu_remove,
diff --git a/drivers/macintosh/windfarm_lm75_sensor.c b/drivers/macintosh/windfarm_lm75_sensor.c
index 282c28a17ea1..1e5fa09845e7 100644
--- a/drivers/macintosh/windfarm_lm75_sensor.c
+++ b/drivers/macintosh/windfarm_lm75_sensor.c
@@ -14,6 +14,7 @@
#include <linux/init.h>
#include <linux/wait.h>
#include <linux/i2c.h>
+#include <linux/of_device.h>
#include <asm/prom.h>
#include <asm/machdep.h>
#include <asm/io.h>
@@ -91,9 +92,14 @@ static int wf_lm75_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct wf_lm75_sensor *lm;
- int rc, ds1775 = id->driver_data;
+ int rc, ds1775;
const char *name, *loc;
+ if (id)
+ ds1775 = id->driver_data;
+ else
+ ds1775 = !!of_device_get_match_data(&client->dev);
+
DBG("wf_lm75: creating %s device at address 0x%02x\n",
ds1775 ? "ds1775" : "lm75", client->addr);
@@ -164,9 +170,17 @@ static const struct i2c_device_id wf_lm75_id[] = {
};
MODULE_DEVICE_TABLE(i2c, wf_lm75_id);
+static const struct of_device_id wf_lm75_of_id[] = {
+ { .compatible = "lm75", .data = (void *)0},
+ { .compatible = "ds1775", .data = (void *)1 },
+ { }
+};
+MODULE_DEVICE_TABLE(of, wf_lm75_of_id);
+
static struct i2c_driver wf_lm75_driver = {
.driver = {
.name = "wf_lm75",
+ .of_match_table = wf_lm75_of_id,
},
.probe = wf_lm75_probe,
.remove = wf_lm75_remove,
diff --git a/drivers/macintosh/windfarm_lm87_sensor.c b/drivers/macintosh/windfarm_lm87_sensor.c
index b03a33b803b7..d011899c0a8a 100644
--- a/drivers/macintosh/windfarm_lm87_sensor.c
+++ b/drivers/macintosh/windfarm_lm87_sensor.c
@@ -166,9 +166,16 @@ static const struct i2c_device_id wf_lm87_id[] = {
};
MODULE_DEVICE_TABLE(i2c, wf_lm87_id);
+static const struct of_device_id wf_lm87_of_id[] = {
+ { .compatible = "lm87cimt", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, wf_lm87_of_id);
+
static struct i2c_driver wf_lm87_driver = {
.driver = {
.name = "wf_lm87",
+ .of_match_table = wf_lm87_of_id,
},
.probe = wf_lm87_probe,
.remove = wf_lm87_remove,
diff --git a/drivers/macintosh/windfarm_max6690_sensor.c b/drivers/macintosh/windfarm_max6690_sensor.c
index e666cc020683..1e7b03d44ad9 100644
--- a/drivers/macintosh/windfarm_max6690_sensor.c
+++ b/drivers/macintosh/windfarm_max6690_sensor.c
@@ -120,9 +120,16 @@ static const struct i2c_device_id wf_max6690_id[] = {
};
MODULE_DEVICE_TABLE(i2c, wf_max6690_id);
+static const struct of_device_id wf_max6690_of_id[] = {
+ { .compatible = "max6690", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, wf_max6690_of_id);
+
static struct i2c_driver wf_max6690_driver = {
.driver = {
.name = "wf_max6690",
+ .of_match_table = wf_max6690_of_id,
},
.probe = wf_max6690_probe,
.remove = wf_max6690_remove,
diff --git a/drivers/macintosh/windfarm_smu_sat.c b/drivers/macintosh/windfarm_smu_sat.c
index c84ec49c3741..cb75dc035616 100644
--- a/drivers/macintosh/windfarm_smu_sat.c
+++ b/drivers/macintosh/windfarm_smu_sat.c
@@ -341,9 +341,16 @@ static const struct i2c_device_id wf_sat_id[] = {
};
MODULE_DEVICE_TABLE(i2c, wf_sat_id);
+static const struct of_device_id wf_sat_of_id[] = {
+ { .compatible = "smu-sat", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, wf_sat_of_id);
+
static struct i2c_driver wf_sat_driver = {
.driver = {
.name = "wf_smu_sat",
+ .of_match_table = wf_sat_of_id,
},
.probe = wf_sat_probe,
.remove = wf_sat_remove,
--
2.20.1
^ permalink raw reply related
* [Bug 199471] [Bisected][Regression] windfarm_pm* no longer gets automatically loaded when CONFIG_I2C_POWERMAC=y is set
From: bugzilla-daemon @ 2020-03-03 12:54 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-199471-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=199471
--- Comment #19 from Wolfram Sang (wsa@the-dreams.de) ---
Well, yes, the lm75 code gets loaded yet it is not clear to me if the device is
now created by DT or not. Well, we will see...
Patch sent out: http://patchwork.ozlabs.org/patch/1248349/
Let's discuss this one. The proof-of-concept here had a missing line but worked
enough.
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [PATCH] powerpc/mm: Fix missing KUAP disable in flush_coherent_icache()
From: Michael Ellerman @ 2020-03-03 12:59 UTC (permalink / raw)
To: linuxppc-dev
We received a report of strange kernel faults which turned out to be
due to a missing KUAP disable in flush_coherent_icache() called
from flush_icache_range().
The fault looks like:
Kernel attempted to access user page (7fffc30d9c00) - exploit attempt? (uid: 1009)
BUG: Unable to handle kernel data access on read at 0x7fffc30d9c00
Faulting instruction address: 0xc00000000007232c
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV
CPU: 35 PID: 5886 Comm: sigtramp Not tainted 5.6.0-rc2-gcc-8.2.0-00003-gfc37a1632d40 #79
NIP: c00000000007232c LR: c00000000003b7fc CTR: 0000000000000000
REGS: c000001e11093940 TRAP: 0300 Not tainted (5.6.0-rc2-gcc-8.2.0-00003-gfc37a1632d40)
MSR: 900000000280b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28000884 XER: 00000000
CFAR: c0000000000722fc DAR: 00007fffc30d9c00 DSISR: 08000000 IRQMASK: 0
GPR00: c00000000003b7fc c000001e11093bd0 c0000000023ac200 00007fffc30d9c00
GPR04: 00007fffc30d9c18 0000000000000000 c000001e11093bd4 0000000000000000
GPR08: 0000000000000000 0000000000000001 0000000000000000 c000001e1104ed80
GPR12: 0000000000000000 c000001fff6ab380 c0000000016be2d0 4000000000000000
GPR16: c000000000000000 bfffffffffffffff 0000000000000000 0000000000000000
GPR20: 00007fffc30d9c00 00007fffc30d8f58 00007fffc30d9c18 00007fffc30d9c20
GPR24: 00007fffc30d9c18 0000000000000000 c000001e11093d90 c000001e1104ed80
GPR28: c000001e11093e90 0000000000000000 c0000000023d9d18 00007fffc30d9c00
NIP flush_icache_range+0x5c/0x80
LR handle_rt_signal64+0x95c/0xc2c
Call Trace:
0xc000001e11093d90 (unreliable)
handle_rt_signal64+0x93c/0xc2c
do_notify_resume+0x310/0x430
ret_from_except_lite+0x70/0x74
Instruction dump:
409e002c 7c0802a6 3c62ff31 3863f6a0 f8010080 48195fed 60000000 48fe4c8d
60000000 e8010080 7c0803a6 7c0004ac <7c00ffac> 7c0004ac 4c00012c 38210070
This path through handle_rt_signal64() to setup_trampoline() and
flush_icache_range() is only triggered by 64-bit processes that have
unmapped their VDSO, which is rare.
flush_icache_range() takes a range of addresses to flush. In
flush_coherent_icache() we implement an optimisation for CPUs where we
know we don't actually have to flush the whole range, we just need to
do a single icbi.
However we still execute the icbi on the user address of the start of
the range we're flushing. On CPUs that also implement KUAP (Power9)
that leads to the spurious fault above.
We should be able to pass any address, including a kernel address, to
the icbi on these CPUs, which would avoid any interaction with KUAP.
But I don't want to make that change in a bug fix, just in case it
surfaces some strange behaviour on some CPU.
So for now just disable KUAP around the icbi. Note the icbi is treated
as a load, so we allow read access, not write as you'd expect.
Fixes: 890274c2dc4c ("powerpc/64s: Implement KUAP for Radix MMU")
Cc: stable@vger.kernel.org # v5.2+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/mm/mem.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index ef7b1119b2e2..184850d9d000 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -373,7 +373,9 @@ static inline bool flush_coherent_icache(unsigned long addr)
*/
if (cpu_has_feature(CPU_FTR_COHERENT_ICACHE)) {
mb(); /* sync */
+ allow_read_from_user((void *)addr, 4);
icbi((void *)addr);
+ prevent_read_from_user((void *)addr, 4);
mb(); /* sync */
isync();
return true;
--
2.21.1
^ permalink raw reply related
* Re: [PATCH] powerpc/mm: Fix missing KUAP disable in flush_coherent_icache()
From: Christophe Leroy @ 2020-03-03 13:10 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
In-Reply-To: <20200303125949.27172-1-mpe@ellerman.id.au>
Le 03/03/2020 à 13:59, Michael Ellerman a écrit :
> We received a report of strange kernel faults which turned out to be
> due to a missing KUAP disable in flush_coherent_icache() called
> from flush_icache_range().
>
> The fault looks like:
>
> Kernel attempted to access user page (7fffc30d9c00) - exploit attempt? (uid: 1009)
> BUG: Unable to handle kernel data access on read at 0x7fffc30d9c00
> Faulting instruction address: 0xc00000000007232c
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV
> CPU: 35 PID: 5886 Comm: sigtramp Not tainted 5.6.0-rc2-gcc-8.2.0-00003-gfc37a1632d40 #79
> NIP: c00000000007232c LR: c00000000003b7fc CTR: 0000000000000000
> REGS: c000001e11093940 TRAP: 0300 Not tainted (5.6.0-rc2-gcc-8.2.0-00003-gfc37a1632d40)
> MSR: 900000000280b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28000884 XER: 00000000
> CFAR: c0000000000722fc DAR: 00007fffc30d9c00 DSISR: 08000000 IRQMASK: 0
> GPR00: c00000000003b7fc c000001e11093bd0 c0000000023ac200 00007fffc30d9c00
> GPR04: 00007fffc30d9c18 0000000000000000 c000001e11093bd4 0000000000000000
> GPR08: 0000000000000000 0000000000000001 0000000000000000 c000001e1104ed80
> GPR12: 0000000000000000 c000001fff6ab380 c0000000016be2d0 4000000000000000
> GPR16: c000000000000000 bfffffffffffffff 0000000000000000 0000000000000000
> GPR20: 00007fffc30d9c00 00007fffc30d8f58 00007fffc30d9c18 00007fffc30d9c20
> GPR24: 00007fffc30d9c18 0000000000000000 c000001e11093d90 c000001e1104ed80
> GPR28: c000001e11093e90 0000000000000000 c0000000023d9d18 00007fffc30d9c00
> NIP flush_icache_range+0x5c/0x80
> LR handle_rt_signal64+0x95c/0xc2c
> Call Trace:
> 0xc000001e11093d90 (unreliable)
> handle_rt_signal64+0x93c/0xc2c
> do_notify_resume+0x310/0x430
> ret_from_except_lite+0x70/0x74
> Instruction dump:
> 409e002c 7c0802a6 3c62ff31 3863f6a0 f8010080 48195fed 60000000 48fe4c8d
> 60000000 e8010080 7c0803a6 7c0004ac <7c00ffac> 7c0004ac 4c00012c 38210070
>
> This path through handle_rt_signal64() to setup_trampoline() and
> flush_icache_range() is only triggered by 64-bit processes that have
> unmapped their VDSO, which is rare.
>
> flush_icache_range() takes a range of addresses to flush. In
> flush_coherent_icache() we implement an optimisation for CPUs where we
> know we don't actually have to flush the whole range, we just need to
> do a single icbi.
>
> However we still execute the icbi on the user address of the start of
> the range we're flushing. On CPUs that also implement KUAP (Power9)
> that leads to the spurious fault above.
>
> We should be able to pass any address, including a kernel address, to
> the icbi on these CPUs, which would avoid any interaction with KUAP.
> But I don't want to make that change in a bug fix, just in case it
> surfaces some strange behaviour on some CPU.
>
> So for now just disable KUAP around the icbi. Note the icbi is treated
> as a load, so we allow read access, not write as you'd expect.
>
> Fixes: 890274c2dc4c ("powerpc/64s: Implement KUAP for Radix MMU")
> Cc: stable@vger.kernel.org # v5.2+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/mm/mem.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index ef7b1119b2e2..184850d9d000 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -373,7 +373,9 @@ static inline bool flush_coherent_icache(unsigned long addr)
> */
> if (cpu_has_feature(CPU_FTR_COHERENT_ICACHE)) {
> mb(); /* sync */
> + allow_read_from_user((void *)addr, 4);
I know that's ignored on Radix, but shouldn't we use L1_CACHE_BYTES as a
length ?
> icbi((void *)addr);
> + prevent_read_from_user((void *)addr, 4);
> mb(); /* sync */
> isync();
> return true;
>
Christophe
^ permalink raw reply
* [PATCH v3 14/18] docs: powerpc: convert vcpudispatch_stats.txt to ReST
From: Mauro Carvalho Chehab @ 2020-03-03 13:59 UTC (permalink / raw)
Cc: Jonathan Corbet, Mauro Carvalho Chehab, linux-doc, Paul Mackerras,
linuxppc-dev
In-Reply-To: <cover.1583243826.git.mchehab+huawei@kernel.org>
- Add a SPDX header;
- Use standard markup for document title;
- Adjust identation on lists and add blank lines where
needed;
- Add it to the powerpc index.rst file.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
Documentation/powerpc/index.rst | 1 +
...ispatch_stats.txt => vcpudispatch_stats.rst} | 17 ++++++++++++-----
2 files changed, 13 insertions(+), 5 deletions(-)
rename Documentation/powerpc/{vcpudispatch_stats.txt => vcpudispatch_stats.rst} (94%)
diff --git a/Documentation/powerpc/index.rst b/Documentation/powerpc/index.rst
index 0d45f0fc8e57..29b90b1b6f20 100644
--- a/Documentation/powerpc/index.rst
+++ b/Documentation/powerpc/index.rst
@@ -30,6 +30,7 @@ powerpc
syscall64-abi
transactional_memory
ultravisor
+ vcpudispatch_stats
.. only:: subproject and html
diff --git a/Documentation/powerpc/vcpudispatch_stats.txt b/Documentation/powerpc/vcpudispatch_stats.rst
similarity index 94%
rename from Documentation/powerpc/vcpudispatch_stats.txt
rename to Documentation/powerpc/vcpudispatch_stats.rst
index e21476bfd78c..5704657a5987 100644
--- a/Documentation/powerpc/vcpudispatch_stats.txt
+++ b/Documentation/powerpc/vcpudispatch_stats.rst
@@ -1,5 +1,8 @@
-VCPU Dispatch Statistics:
-=========================
+.. SPDX-License-Identifier: GPL-2.0
+
+========================
+VCPU Dispatch Statistics
+========================
For Shared Processor LPARs, the POWER Hypervisor maintains a relatively
static mapping of the LPAR processors (vcpus) to physical processor
@@ -20,25 +23,29 @@ The statistics themselves are available by reading the procfs file
a vcpu as represented by the first field, followed by 8 numbers.
The first number corresponds to:
+
1. total vcpu dispatches since the beginning of statistics collection
The next 4 numbers represent vcpu dispatch dispersions:
+
2. number of times this vcpu was dispatched on the same processor as last
time
3. number of times this vcpu was dispatched on a different processor core
as last time, but within the same chip
4. number of times this vcpu was dispatched on a different chip
5. number of times this vcpu was dispatches on a different socket/drawer
-(next numa boundary)
+ (next numa boundary)
The final 3 numbers represent statistics in relation to the home node of
the vcpu:
+
6. number of times this vcpu was dispatched in its home node (chip)
7. number of times this vcpu was dispatched in a different node
8. number of times this vcpu was dispatched in a node further away (numa
-distance)
+ distance)
+
+An example output::
-An example output:
$ sudo cat /proc/powerpc/vcpudispatch_stats
cpu0 6839 4126 2683 30 0 6821 18 0
cpu1 2515 1274 1229 12 0 2509 6 0
--
2.24.1
^ permalink raw reply related
* Re: [RFC 02/11] perf/core: Data structure to present hazard data
From: Ravi Bangoria @ 2020-03-03 14:31 UTC (permalink / raw)
To: Mark Rutland
Cc: Ravi Bangoria, ak, maddy, peterz, jolsa, Madhavan Srinivasan,
linux-kernel, eranian, adrian.hunter, alexander.shishkin, yao.jin,
mingo, paulus, acme, robert.richter, namhyung, kim.phillips,
linuxppc-dev, alexey.budankov, kan.liang
In-Reply-To: <20200302145434.GE56497@lakrids.cambridge.arm.com>
On 3/2/20 8:24 PM, Mark Rutland wrote:
>> @@ -870,6 +871,13 @@ enum perf_event_type {
>> * { u64 phys_addr;} && PERF_SAMPLE_PHYS_ADDR
>> * { u64 size;
>> * char data[size]; } && PERF_SAMPLE_AUX
>> + * { u8 itype;
>> + * u8 icache;
>> + * u8 hazard_stage;
>> + * u8 hazard_reason;
>> + * u8 stall_stage;
>> + * u8 stall_reason;
>> + * u16 pad;} && PERF_SAMPLE_PIPELINE_HAZ
>> * };
>
> The existing comment shows the aux data *immediately* after ther
> phys_addr field, where you've placed struct perf_pipeline_haz_data.
>
> If adding to struct perf_sample_data is fine, this needs to come before
> the aux data in this comment. If adding to struct perf_sample_data is
> not fine. struct perf_pipeline_haz_data cannot live there.
>
> I suspect the latter is true, but you're getting away with it because
> you're not using both PERF_SAMPLE_AUX and PERF_SAMPLE_PIPELINE_HAZ
> simultaneously.
Right. Thanks for pointing it out. Will change it.
Ravi
^ permalink raw reply
* Re: [RFC 02/11] perf/core: Data structure to present hazard data
From: Ravi Bangoria @ 2020-03-03 14:32 UTC (permalink / raw)
To: Mark Rutland
Cc: Ravi Bangoria, ak, maddy, peterz, jolsa, Madhavan Srinivasan,
linux-kernel, eranian, adrian.hunter, alexander.shishkin, yao.jin,
mingo, paulus, acme, robert.richter, namhyung, kim.phillips,
linuxppc-dev, alexey.budankov, kan.liang
In-Reply-To: <20200302144842.GD56497@lakrids.cambridge.arm.com>
On 3/2/20 8:18 PM, Mark Rutland wrote:
>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>> index 377d794d3105..ff252618ca93 100644
>> --- a/include/uapi/linux/perf_event.h
>> +++ b/include/uapi/linux/perf_event.h
>> @@ -142,8 +142,9 @@ enum perf_event_sample_format {
>> PERF_SAMPLE_REGS_INTR = 1U << 18,
>> PERF_SAMPLE_PHYS_ADDR = 1U << 19,
>> PERF_SAMPLE_AUX = 1U << 20,
>> + PERF_SAMPLE_PIPELINE_HAZ = 1U << 21,
>
> Can we please have perf_event_open() reject this sample flag for PMUs
> without the new callback (introduced in the next patch)?
>
> That way it'll be possible to detect whether the PMU exposes this.
Sure. Will change it.
Ravi
^ permalink raw reply
* RE: [RFC PATCH v1] powerpc/prom_init: disable XIVE in Secure VM.
From: Ram Pai @ 2020-03-03 17:02 UTC (permalink / raw)
To: Cédric Le Goater
Cc: aik, andmike, groug, kvm-ppc, sukadev, linuxppc-dev, bauerman,
David Gibson
In-Reply-To: <8f0c3d41-d1f9-7e6d-276b-b95238715979@fr.ibm.com>
On Tue, Mar 03, 2020 at 07:50:08AM +0100, Cédric Le Goater wrote:
> On 3/3/20 12:32 AM, David Gibson wrote:
> > On Fri, Feb 28, 2020 at 11:54:04PM -0800, Ram Pai wrote:
> >> XIVE is not correctly enabled for Secure VM in the KVM Hypervisor yet.
> >>
> >> Hence Secure VM, must always default to XICS interrupt controller.
> >>
> >> If XIVE is requested through kernel command line option "xive=on",
> >> override and turn it off.
> >>
> >> If XIVE is the only supported platform interrupt controller; specified
> >> through qemu option "ic-mode=xive", simply abort. Otherwise default to
> >> XICS.
> >
> > Uh... the discussion thread here seems to have gotten oddly off
> > track.
>
> There seem to be multiple issues. It is difficult to have a clear status.
>
> > So, to try to clean up some misunderstandings on both sides:
> >
> > 1) The guest is the main thing that knows that it will be in secure
> > mode, so it's reasonable for it to conditionally use XIVE based
> > on that
>
> FW support is required AFAIUI.
> > 2) The mechanism by which we do it here isn't quite right. Here the
> > guest is checking itself that the host only allows XIVE, but we
> > can't do XIVE and is panic()ing. Instead, in the SVM case we
> > should force support->xive to false, and send that in the CAS
> > request to the host. We expect the host to just terminate
> > us because of the mismatch, but this will interact better with
> > host side options setting policy for panic states and the like.
> > Essentially an SVM kernel should behave like an old kernel with
> > no XIVE support at all, at least w.r.t. the CAS irq mode flags.
>
> Yes. XIVE shouldn't be requested by the guest.
Ok.
> This is the last option
> I proposed but I thought there was some negotiation with the hypervisor
> which is not the case.
>
> > 3) Although there are means by which the hypervisor can kind of know
> > a guest is in secure mode, there's not really an "svm=on" option
> > on the host side. For the most part secure mode is based on
> > discussion directly between the guest and the ultravisor with
> > almost no hypervisor intervention.
>
> Is there a negotiation with the ultravisor ?
The VM has no negotiation with the ultravisor w.r.t CAS.
>
> > 4) I'm guessing the problem with XIVE in SVM mode is that XIVE needs
> > to write to event queues in guest memory, which would have to be
> > explicitly shared for secure mode. That's true whether it's KVM
> > or qemu accessing the guest memory, so kernel_irqchip=on/off is
> > entirely irrelevant.
>
> This problem should be already fixed.
> The XIVE event queues are shared
Yes i have a patch for the guest kernel that shares the event
queue page with the hypervisor. This is done using the
UV_SHARE_PAGE ultracall. This patch is not sent out to any any mailing
lists yet. However the patch by itself does not solve the xive problem
for secure VM.
> and the remaining problem with XIVE is the KVM page fault handler
> populating the TIMA and ESB pages. Ultravisor doesn't seem to support
> this feature and this breaks interrupt management in the guest.
Yes. This is the bigger issue that needs to be fixed. When the secure guest
accesses the page associated with the xive memslot, a page fault is
generated, which the ultravisor reflects to the hypervisor. Hypervisor
seems to be mapping Hardware-page to that GPA. Unforatunately it is not
informing the ultravisor of that map. I am trying to understand the
root cause. But since I am not sure what more issues I might run into
after chasing down that issue, I figured its better to disable xive
support in SVM in the interim.
**** BTW: I figured, I dont need this intermin patch to disable xive for
secure VM. Just doing "svm=on xive=off" on the kernel command line is
sufficient for now. *****
>
> But, kernel_irqchip=off should work out of the box. It seems it doesn't.
> Something to investigate.
Dont know why.
Does this option, disable the chip from interrupting the
guest directly; instead mediates the interrupt through the hypervisor?
>
> >
> > 5) All the above said, having to use XICS is pretty crappy. You
> > should really get working on XIVE support for secure VMs.
>
> Yes.
and yes too.
Summary: I am dropping this patch for now.
>
> Thanks,
>
> C.
--
Ram Pai
^ permalink raw reply
* Re: [RFC 1/3] mm/vma: Define a default value for VM_DATA_DEFAULT_FLAGS
From: Vlastimil Babka @ 2020-03-03 17:25 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm
Cc: Rich Felker, linux-ia64, linux-sh, Catalin Marinas,
Heiko Carstens, linux-mips, James E.J. Bottomley, Guo Ren,
sparclinux, linux-hexagon, linux-riscv, Jonas Bonn, linux-s390,
linux-snps-arc, linux-c6x-dev, Brian Cain, Russell King,
Ley Foon Tan, Geert Uytterhoeven, linux-parisc, Mark Salter,
Paul Burton, uclinux-h8-devel, linux-xtensa, Jeff Dike, linux-um,
linux-m68k, openrisc, Paul Walmsley, Thomas Gleixner, Guan Xuetao,
linux-arm-kernel, Richard Henderson, Chris Zankel, Michal Simek,
Tony Luck, Yoshinori Sato, Nick Hu, Vineet Gupta, linux-kernel,
Ralf Baechle, linux-alpha, nios2-dev, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <1583131666-15531-2-git-send-email-anshuman.khandual@arm.com>
On 3/2/20 7:47 AM, Anshuman Khandual wrote:
> There are many platforms with exact same value for VM_DATA_DEFAULT_FLAGS
> This creates a default value for VM_DATA_DEFAULT_FLAGS in line with the
> existing VM_STACK_DEFAULT_FLAGS. While here, also define some more macros
> with standard VMA access flag combinations that are used frequently across
> many platforms. Apart from simplification, this reduces code duplication
> as well.
>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Salter <msalter@redhat.com>
> Cc: Guo Ren <guoren@kernel.org>
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Brian Cain <bcain@codeaurora.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Michal Simek <monstr@monstr.eu>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Nick Hu <nickhu@andestech.com>
> Cc: Ley Foon Tan <ley.foon.tan@intel.com>
> Cc: Jonas Bonn <jonas@southpole.se>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Rich Felker <dalias@libc.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Guan Xuetao <gxt@pku.edu.cn>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jeff Dike <jdike@addtoit.com>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-alpha@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-snps-arc@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-c6x-dev@linux-c6x.org
> Cc: uclinux-h8-devel@lists.sourceforge.jp
> Cc: linux-hexagon@vger.kernel.org
> Cc: linux-ia64@vger.kernel.org
> Cc: linux-m68k@lists.linux-m68k.org
> Cc: linux-mips@vger.kernel.org
> Cc: nios2-dev@lists.rocketboards.org
> Cc: openrisc@lists.librecores.org
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: sparclinux@vger.kernel.org
> Cc: linux-um@lists.infradead.org
> Cc: linux-xtensa@linux-xtensa.org
> Cc: linux-mm@kvack.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Nit:
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b0e53ef13ff1..7a764ae6ab68 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -342,6 +342,21 @@ extern unsigned int kobjsize(const void *objp);
> /* Bits set in the VMA until the stack is in its final location */
> #define VM_STACK_INCOMPLETE_SETUP (VM_RAND_READ | VM_SEQ_READ)
>
> +#define TASK_EXEC ((current->personality & READ_IMPLIES_EXEC) ? VM_EXEC : 0)
> +
> +/* Common data flag combinations */
> +#define VM_DATA_FLAGS_TSK_EXEC (VM_READ | VM_WRITE | TASK_EXEC | \
> + VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
> +#define VM_DATA_FLAGS_NON_EXEC (VM_READ | VM_WRITE | VM_MAYREAD | \
> + VM_MAYWRITE | VM_MAYEXEC)
> +#define VM_DATA_FLAGS_EXEC (VM_READ | VM_WRITE | VM_EXEC | \
> + VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
> +
> +#ifndef VM_DATA_DEFAULT_FLAGS /* arch can override this */
> +#define VM_DATA_DEFAULT_FLAGS (VM_READ | VM_WRITE | VM_EXEC | \
> + VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
Should you use VM_DATA_FLAGS_EXEC here? Yeah one more macro to expand, but it's
right above this.
> +#endif
> +
> #ifndef VM_STACK_DEFAULT_FLAGS /* arch can override this */
> #define VM_STACK_DEFAULT_FLAGS VM_DATA_DEFAULT_FLAGS
> #endif
>
^ permalink raw reply
* Re: [RFC 2/3] mm/vma: Introduce VM_ACCESS_FLAGS
From: Vlastimil Babka @ 2020-03-03 17:48 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm
Cc: linux-sh, Catalin Marinas, Dave Hansen, Heiko Carstens, devel,
linux-s390, linux-c6x-dev, Yoshinori Sato, Russell King,
Ley Foon Tan, Mark Salter, Rob Springer, Thomas Gleixner,
Guan Xuetao, linux-arm-kernel, Nick Hu, Greg Kroah-Hartman,
linux-kernel, nios2-dev, Andrew Morton, linuxppc-dev
In-Reply-To: <1583131666-15531-3-git-send-email-anshuman.khandual@arm.com>
On 3/2/20 7:47 AM, Anshuman Khandual wrote:
> There are many places where all basic VMA access flags (read, write, exec)
> are initialized or checked against as a group. One such example is during
> page fault. Existing vma_is_accessible() wrapper already creates the notion
> of VMA accessibility as a group access permissions. Hence lets just create
> VM_ACCESS_FLAGS (VM_READ|VM_WRITE|VM_EXEC) which will not only reduce code
> duplication but also extend the VMA accessibility concept in general.
>
> Cc: Russell King <linux@armlinux.org.uk>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Mark Salter <msalter@redhat.com>
> Cc: Nick Hu <nickhu@andestech.com>
> CC: Ley Foon Tan <ley.foon.tan@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Guan Xuetao <gxt@pku.edu.cn>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Rob Springer <rspringer@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-c6x-dev@linux-c6x.org
> Cc: nios2-dev@lists.rocketboards.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: devel@driverdev.osuosl.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Dunno. Such mask seems ok for testing flags, but it's a bit awkward when
initializing flags, where it covers just one of many combinations that seem
used. But no strong opinions, patch looks correct.
^ permalink raw reply
* Re: [PATCH v3 18/27] powerpc/powernv/pmem: Add controller dump IOCTLs
From: Frederic Barrat @ 2020-03-03 18:04 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <20200221032720.33893-19-alastair@au1.ibm.com>
Le 21/02/2020 à 04:27, Alastair D'Silva a écrit :
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This patch adds IOCTLs to allow userspace to request & fetch dumps
> of the internal controller state.
>
> This is useful during debugging or when a fatal error on the controller
> has occurred.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> arch/powerpc/platforms/powernv/pmem/ocxl.c | 132 +++++++++++++++++++++
> include/uapi/nvdimm/ocxl-pmem.h | 15 +++
> 2 files changed, 147 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> index 2b64504f9129..2cabafe1fc58 100644
> --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c
> @@ -640,6 +640,124 @@ static int ioctl_error_log(struct ocxlpmem *ocxlpmem,
> return 0;
> }
>
> +static int ioctl_controller_dump_data(struct ocxlpmem *ocxlpmem,
> + struct ioctl_ocxl_pmem_controller_dump_data __user *uarg)
> +{
> + struct ioctl_ocxl_pmem_controller_dump_data args;
> + u16 i;
> + u64 val;
> + int rc;
> +
> + if (copy_from_user(&args, uarg, sizeof(args)))
> + return -EFAULT;
> +
> + if (args.buf_size % 8)
> + return -EINVAL;
> +
> + if (args.buf_size > ocxlpmem->admin_command.data_size)
> + return -EINVAL;
> +
> + mutex_lock(&ocxlpmem->admin_command.lock);
> +
> + rc = admin_command_request(ocxlpmem, ADMIN_COMMAND_CONTROLLER_DUMP);
> + if (rc)
> + goto out;
> +
> + val = ((u64)args.offset) << 32;
> + val |= args.buf_size;
> + rc = ocxl_global_mmio_write64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.request_offset + 0x08,
> + OCXL_LITTLE_ENDIAN, val);
> + if (rc)
> + goto out;
> +
> + rc = admin_command_execute(ocxlpmem);
> + if (rc)
> + goto out;
> +
> + rc = admin_command_complete_timeout(ocxlpmem,
> + ADMIN_COMMAND_CONTROLLER_DUMP);
> + if (rc < 0) {
> + dev_warn(&ocxlpmem->dev, "Controller dump timed out\n");
> + goto out;
> + }
> +
> + rc = admin_response(ocxlpmem);
> + if (rc < 0)
> + goto out;
> + if (rc != STATUS_SUCCESS) {
> + warn_status(ocxlpmem,
> + "Unexpected status from retrieve error log",
> + rc);
> + goto out;
> + }
It would help if there was a comment indicating how the 3 ioctls are
used. My understanding is that the userland is:
- requesting the controller to prepare a state dump
- then one or more ioctls to fetch the data. The number of calls
required to get the full state really depends on the size of the buffer
passed by user
- a last ioctl to tell the controller that we're done, presumably to let
it free some resources.
> +
> + for (i = 0; i < args.buf_size; i += 8) {
> + u64 val;
> +
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + ocxlpmem->admin_command.data_offset + i,
> + OCXL_HOST_ENDIAN, &val);
> + if (rc)
> + goto out;
> +
> + if (copy_to_user(&args.buf[i], &val, sizeof(u64))) {
> + rc = -EFAULT;
> + goto out;
> + }
> + }
> +
> + if (copy_to_user(uarg, &args, sizeof(args))) {
> + rc = -EFAULT;
> + goto out;
> + }
> +
> + rc = admin_response_handled(ocxlpmem);
> + if (rc)
> + goto out;
> +
> +out:
> + mutex_unlock(&ocxlpmem->admin_command.lock);
> + return rc;
> +}
> +
> +int request_controller_dump(struct ocxlpmem *ocxlpmem)
> +{
> + int rc;
> + u64 busy = 1;
> +
> + rc = ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHIC,
> + OCXL_LITTLE_ENDIAN,
> + GLOBAL_MMIO_CHI_CDA);
> +
rc is not checked here.
> +
> + rc = ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_HCI,
> + OCXL_LITTLE_ENDIAN,
> + GLOBAL_MMIO_HCI_CONTROLLER_DUMP);
> + if (rc)
> + return rc;
> +
> + while (busy) {
> + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> + GLOBAL_MMIO_HCI,
> + OCXL_LITTLE_ENDIAN, &busy);
> + if (rc)
> + return rc;
> +
> + busy &= GLOBAL_MMIO_HCI_CONTROLLER_DUMP;
Setting 'busy' doesn't hurt, but it's not really useful, is it?
We should add some kind of timeout so that if the controller hits an
issue, we don't spin in kernel space endlessly.
> + cond_resched();
> + }
> +
> + return 0;
> +}
> +
> +static int ioctl_controller_dump_complete(struct ocxlpmem *ocxlpmem)
> +{
> + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_HCI,
> + OCXL_LITTLE_ENDIAN,
> + GLOBAL_MMIO_HCI_CONTROLLER_DUMP_COLLECTED);
> +}
> +
> static long file_ioctl(struct file *file, unsigned int cmd, unsigned long args)
> {
> struct ocxlpmem *ocxlpmem = file->private_data;
> @@ -650,7 +768,21 @@ static long file_ioctl(struct file *file, unsigned int cmd, unsigned long args)
> rc = ioctl_error_log(ocxlpmem,
> (struct ioctl_ocxl_pmem_error_log __user *)args);
> break;
> +
> + case IOCTL_OCXL_PMEM_CONTROLLER_DUMP:
> + rc = request_controller_dump(ocxlpmem);
> + break;
> +
> + case IOCTL_OCXL_PMEM_CONTROLLER_DUMP_DATA:
> + rc = ioctl_controller_dump_data(ocxlpmem,
> + (struct ioctl_ocxl_pmem_controller_dump_data __user *)args);
> + break;
> +
> + case IOCTL_OCXL_PMEM_CONTROLLER_DUMP_COMPLETE:
> + rc = ioctl_controller_dump_complete(ocxlpmem);
> + break;
> }
> +
> return rc;
> }
>
> diff --git a/include/uapi/nvdimm/ocxl-pmem.h b/include/uapi/nvdimm/ocxl-pmem.h
> index b10f8ac0c20f..d4d8512d03f7 100644
> --- a/include/uapi/nvdimm/ocxl-pmem.h
> +++ b/include/uapi/nvdimm/ocxl-pmem.h
> @@ -38,9 +38,24 @@ struct ioctl_ocxl_pmem_error_log {
> __u8 *buf; /* pointer to output buffer */
> };
>
> +struct ioctl_ocxl_pmem_controller_dump_data {
> + __u8 *buf; /* pointer to output buffer */
We only support 64-bit user app on powerpc, but using a pointer type in
a kernel ABI is unusual. We should use a know size like __u64.
(also applies to buf pointer in struct ioctl_ocxl_pmem_error_log from
previous patch too)
The rest of the structure will also be padded by the compiler, which we
should avoid.
Fred
> + __u16 buf_size; /* in/out, buffer size provided/required.
> + * If required is greater than provided, the buffer
> + * will be truncated to the amount provided. If its
> + * less, then only the required bytes will be populated.
> + * If it is 0, then there is no more dump data available.
> + */
> + __u32 offset; /* in, Offset within the dump */
> + __u64 reserved[8];
> +};
> +
> /* ioctl numbers */
> #define OCXL_PMEM_MAGIC 0x5C
> /* SCM devices */
> #define IOCTL_OCXL_PMEM_ERROR_LOG _IOWR(OCXL_PMEM_MAGIC, 0x01, struct ioctl_ocxl_pmem_error_log)
> +#define IOCTL_OCXL_PMEM_CONTROLLER_DUMP _IO(OCXL_PMEM_MAGIC, 0x02)
> +#define IOCTL_OCXL_PMEM_CONTROLLER_DUMP_DATA _IOWR(OCXL_PMEM_MAGIC, 0x03, struct ioctl_ocxl_pmem_controller_dump_data)
> +#define IOCTL_OCXL_PMEM_CONTROLLER_DUMP_COMPLETE _IO(OCXL_PMEM_MAGIC, 0x04)
>
> #endif /* _UAPI_OCXL_SCM_H */
>
^ permalink raw reply
* RE: [RFC PATCH v1] powerpc/prom_init: disable XIVE in Secure VM.
From: Ram Pai @ 2020-03-03 18:56 UTC (permalink / raw)
To: Greg Kurz
Cc: aik, andmike, kvm-ppc, Cédric Le Goater, sukadev,
linuxppc-dev, bauerman, David Gibson
In-Reply-To: <20200303184520.632be270@bahia.home>
On Tue, Mar 03, 2020 at 06:45:20PM +0100, Greg Kurz wrote:
> On Tue, 3 Mar 2020 09:02:05 -0800
> Ram Pai <linuxram@us.ibm.com> wrote:
>
> > On Tue, Mar 03, 2020 at 07:50:08AM +0100, Cédric Le Goater wrote:
> > > On 3/3/20 12:32 AM, David Gibson wrote:
> > > > On Fri, Feb 28, 2020 at 11:54:04PM -0800, Ram Pai wrote:
> > > >> XIVE is not correctly enabled for Secure VM in the KVM Hypervisor yet.
> > > >>
> > > >> Hence Secure VM, must always default to XICS interrupt controller.
> > > >>
> > > >> If XIVE is requested through kernel command line option "xive=on",
> > > >> override and turn it off.
> > > >>
> > > >> If XIVE is the only supported platform interrupt controller; specified
> > > >> through qemu option "ic-mode=xive", simply abort. Otherwise default to
> > > >> XICS.
> > > >
> > > > Uh... the discussion thread here seems to have gotten oddly off
> > > > track.
> > >
> > > There seem to be multiple issues. It is difficult to have a clear status.
> > >
> > > > So, to try to clean up some misunderstandings on both sides:
> > > >
> > > > 1) The guest is the main thing that knows that it will be in secure
> > > > mode, so it's reasonable for it to conditionally use XIVE based
> > > > on that
> > >
> > > FW support is required AFAIUI.
> > > > 2) The mechanism by which we do it here isn't quite right. Here the
> > > > guest is checking itself that the host only allows XIVE, but we
> > > > can't do XIVE and is panic()ing. Instead, in the SVM case we
> > > > should force support->xive to false, and send that in the CAS
> > > > request to the host. We expect the host to just terminate
> > > > us because of the mismatch, but this will interact better with
> > > > host side options setting policy for panic states and the like.
> > > > Essentially an SVM kernel should behave like an old kernel with
> > > > no XIVE support at all, at least w.r.t. the CAS irq mode flags.
> > >
> > > Yes. XIVE shouldn't be requested by the guest.
> >
> > Ok.
> >
> > > This is the last option
> > > I proposed but I thought there was some negotiation with the hypervisor
> > > which is not the case.
> > >
> > > > 3) Although there are means by which the hypervisor can kind of know
> > > > a guest is in secure mode, there's not really an "svm=on" option
> > > > on the host side. For the most part secure mode is based on
> > > > discussion directly between the guest and the ultravisor with
> > > > almost no hypervisor intervention.
> > >
> > > Is there a negotiation with the ultravisor ?
> >
> > The VM has no negotiation with the ultravisor w.r.t CAS.
> >
> > >
> > > > 4) I'm guessing the problem with XIVE in SVM mode is that XIVE needs
> > > > to write to event queues in guest memory, which would have to be
> > > > explicitly shared for secure mode. That's true whether it's KVM
> > > > or qemu accessing the guest memory, so kernel_irqchip=on/off is
> > > > entirely irrelevant.
> > >
> > > This problem should be already fixed.
> > > The XIVE event queues are shared
> >
> > Yes i have a patch for the guest kernel that shares the event
> > queue page with the hypervisor. This is done using the
> > UV_SHARE_PAGE ultracall. This patch is not sent out to any any mailing
> > lists yet.
>
> Why ?
At this point I am not sure if this is the only change, I need to the
guest kernel. I also need changes to KVM and to the ultravisor. Its bit
premature to send the patch without having figured out everything
to get xive working on a Secure VM.
>
> > However the patch by itself does not solve the xive problem
> > for secure VM.
> >
>
> This patch would allow at least to answer Cedric's question about
> kernel_irqchip=off, since this looks like the only thing needed
> to make it work.
hmm.. I am not sure. Are you saying
(a) patch the guest kernel to share the event queue page
(b) run the qemu with "kernel_irqchip=off"
(c) and the guest kernel with "svm=on"
and it should all work?
RP
^ permalink raw reply
* Re: [PATCH] macintosh: windfarm: fix MODINFO regression
From: Andreas Schwab @ 2020-03-03 18:58 UTC (permalink / raw)
To: Wolfram Sang
Cc: Erhard Furtner, Mathieu Malaterre, debian-powerpc, linux-i2c,
John Paul Adrian Glaubitz, linuxppc-dev
In-Reply-To: <20200303125046.5157-1-wsa@the-dreams.de>
On Mär 03 2020, Wolfram Sang wrote:
> sound/aoa/codecs/onyx.c
> sound/aoa/codecs/tas.c
These are loaded explicitly via request_module (as snd-aoa-codec-%s).
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: [EXTERNAL] Re: [RFC PATCH v1] powerpc/prom_init: disable XIVE in Secure VM.
From: Cédric Le Goater @ 2020-03-03 19:08 UTC (permalink / raw)
To: Ram Pai
Cc: aik, andmike, groug, kvm-ppc, sukadev, linuxppc-dev, bauerman,
David Gibson
In-Reply-To: <20200303170205.GA5416@oc0525413822.ibm.com>
>>> 4) I'm guessing the problem with XIVE in SVM mode is that XIVE needs
>>> to write to event queues in guest memory, which would have to be
>>> explicitly shared for secure mode. That's true whether it's KVM
>>> or qemu accessing the guest memory, so kernel_irqchip=on/off is
>>> entirely irrelevant.
>>
>> This problem should be already fixed.
>> The XIVE event queues are shared
>
> Yes i have a patch for the guest kernel that shares the event
> queue page with the hypervisor. This is done using the
> UV_SHARE_PAGE ultracall. This patch is not sent out to any any mailing
> lists yet. However the patch by itself does not solve the xive problem
> for secure VM.
yes because you also need to share the XIVE TIMA and ESB pages mapped
in xive_native_esb_fault() and xive_native_tima_fault().
>> and the remaining problem with XIVE is the KVM page fault handler
>> populating the TIMA and ESB pages. Ultravisor doesn't seem to support
>> this feature and this breaks interrupt management in the guest.
>
> Yes. This is the bigger issue that needs to be fixed. When the secure guest
> accesses the page associated with the xive memslot, a page fault is
> generated, which the ultravisor reflects to the hypervisor. Hypervisor
> seems to be mapping Hardware-page to that GPA. Unforatunately it is not
> informing the ultravisor of that map. I am trying to understand the
> root cause. But since I am not sure what more issues I might run into
> after chasing down that issue, I figured its better to disable xive
> support in SVM in the interim.
Is it possible to call uv_share_page() from the hypervisor ?
> **** BTW: I figured, I dont need this intermin patch to disable xive for
> secure VM. Just doing "svm=on xive=off" on the kernel command line is
> sufficient for now. *****
Yes.
>> But, kernel_irqchip=off should work out of the box. It seems it doesn't.
>> Something to investigate.
>
> Dont know why.
We need to understand why.
You still need the patch to share the event queue page allocated by the
guest OS because QEMU will enqueue events. But you should not need anything
else.
> Does this option, disable the chip from interrupting the
> guest directly; instead mediates the interrupt through the hypervisor?
Yes. The KVM backend is unused, the XIVE interrupt controller is deactivated
for the guest and QEMU notifies the vCPUs directly.
The TIMA and ESB pages belong the QEMU process and the guest OS will do
some load and store operations onto them for interrupt management. Is that
OK from a UV perspective ?
Thanks,
C.
^ permalink raw reply
* Re: [EXTERNAL] Re: [RFC PATCH v1] powerpc/prom_init: disable XIVE in Secure VM.
From: Cédric Le Goater @ 2020-03-03 19:18 UTC (permalink / raw)
To: Greg Kurz, Ram Pai
Cc: aik, andmike, kvm-ppc, sukadev, linuxppc-dev, bauerman,
David Gibson
In-Reply-To: <20200303184520.632be270@bahia.home>
>> **** BTW: I figured, I dont need this intermin patch to disable xive for
>> secure VM. Just doing "svm=on xive=off" on the kernel command line is
>> sufficient for now. *****
>>
>
> No it is not. If the hypervisor doesn't propose XIVE (ie. ic-mode=xive
> on the QEMU command line), the kernel simply ignores "xive=off".
If I am correct, with the option ic-mode=xive, the hypervisor will
propose only 'xive' in OV5 and not both 'xive' and 'xics'. But the
result is the same because xive can not be turned off and "xive=off"
is ignored.
Anyway, it's not the most common case of usage of the QEMU command
like. I think it's OK to use "xive=off" on the kernel command line
for now.
C.
^ 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