Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring
Roger Pau Monné writes: > On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote: >> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote: >> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote: >> > > On Wed, 30 Nov 2022, Roger Pau Monne wrote: >> > > > The hvc machinery registers both a console and a tty device based on >> > > > the hv ops provided by the specific implementation. Those two >> > > > interfaces however have different locks, and there's no single locks >> > > > that's shared between the tty and the console implementations, hence >> > > > the driver needs to protect itself against concurrent accesses. >> > > > Otherwise concurrent calls using the split interfaces are likely to >> > > > corrupt the ring indexes, leaving the console unusable. >> > > > >> > > > Introduce a lock to xencons_info to serialize accesses to the shared >> > > > ring. This is only required when using the shared memory console, >> > > > concurrent accesses to the hypercall based console implementation are >> > > > not an issue. >> > > > >> > > > Note the conditional logic in domU_read_console() is slightly modified >> > > > so the notify_daemon() call can be done outside of the locked region: >> > > > it's an hypercall and there's no need for it to be done with the lock >> > > > held. >> > > >> > > For domU_read_console: I don't mean to block this patch but we need to >> > > be sure about the semantics of hv_ops.get_chars. Either it is expected >> > > to be already locked, then we definitely shouldn't add another lock to >> > > domU_read_console. Or it is not expected to be already locked, then we >> > > should add the lock. >> > > >> > > My impression is that it is expected to be already locked, but I think >> > > we need Greg or Jiri to confirm one way or the other. >> > >> > Let me move both to the 'To:' field then. >> > >> > My main concern is the usage of hv_ops.get_chars hook in >> > hvc_poll_get_char(), as it's not obvious to me that callers of >> > tty->poll_get_char hook as returned by tty_find_polling_driver() will >> > always do so with the tty lock held (in fact the only user right now >> > doesn't seem to hold the tty lock). >> > >> > > Aside from that the rest looks fine. > > I guess I could reluctantly remove the lock in the get_chars hook, > albeit I'm not convinced at all the lock is not needed there. I don't know the xen driver, but other HVC backends have a lock around their private state in their get_chars() implementations. See eg. hvterm_raw_get_chars(). cheers
Re: [PATCH v6 13/41] mm: Make pte_mkwrite() take a VMA
Rick Edgecombe writes: > The x86 Control-flow Enforcement Technology (CET) feature includes a new > type of memory called shadow stack. This shadow stack memory has some > unusual properties, which requires some core mm changes to function > properly. ... > --- > Hi Non-x86 Arch’s, > > x86 has a feature that allows for the creation of a special type of > writable memory (shadow stack) that is only writable in limited specific > ways. Previously, changes were proposed to core MM code to teach it to > decide when to create normally writable memory or the special shadow stack > writable memory, but David Hildenbrand suggested[0] to change > pXX_mkwrite() to take a VMA, so awareness of shadow stack memory can be > moved into x86 code. > > Since pXX_mkwrite() is defined in every arch, it requires some tree-wide > changes. So that is why you are seeing some patches out of a big x86 > series pop up in your arch mailing list. There is no functional change. > After this refactor, the shadow stack series goes on to use the arch > helpers to push shadow stack memory details inside arch/x86. ... > --- > Documentation/mm/arch_pgtable_helpers.rst| 9 ++--- > arch/alpha/include/asm/pgtable.h | 6 +- > arch/arc/include/asm/hugepage.h | 2 +- > arch/arc/include/asm/pgtable-bits-arcv2.h| 7 ++- > arch/arm/include/asm/pgtable-3level.h| 7 ++- > arch/arm/include/asm/pgtable.h | 2 +- > arch/arm64/include/asm/pgtable.h | 4 ++-- > arch/csky/include/asm/pgtable.h | 2 +- > arch/hexagon/include/asm/pgtable.h | 2 +- > arch/ia64/include/asm/pgtable.h | 2 +- > arch/loongarch/include/asm/pgtable.h | 4 ++-- > arch/m68k/include/asm/mcf_pgtable.h | 2 +- > arch/m68k/include/asm/motorola_pgtable.h | 6 +- > arch/m68k/include/asm/sun3_pgtable.h | 6 +- > arch/microblaze/include/asm/pgtable.h| 2 +- > arch/mips/include/asm/pgtable.h | 6 +++--- > arch/nios2/include/asm/pgtable.h | 2 +- > arch/openrisc/include/asm/pgtable.h | 2 +- > arch/parisc/include/asm/pgtable.h| 6 +- > arch/powerpc/include/asm/book3s/32/pgtable.h | 2 +- > arch/powerpc/include/asm/book3s/64/pgtable.h | 4 ++-- > arch/powerpc/include/asm/nohash/32/pgtable.h | 2 +- > arch/powerpc/include/asm/nohash/32/pte-8xx.h | 2 +- > arch/powerpc/include/asm/nohash/64/pgtable.h | 2 +- Looks like you discovered the joys of ppc's at-least 5 different MMU implementations, sorry :) Acked-by: Michael Ellerman (powerpc) cheers
Re: [PATCH 08/30] powerpc/setup: Refactor/untangle panic notifiers
"Guilherme G. Piccoli" writes: > On 05/05/2022 15:55, Hari Bathini wrote: >> [...] >> The change looks good. I have tested it on an LPAR (ppc64). >> >> Reviewed-by: Hari Bathini >> > > Hi Michael. do you think it's possible to add this one to powerpc/next > (or something like that), or do you prefer a V2 with his tag? Ah sorry, I assumed it was going in as part of the whole series. I guess I misread the cover letter. So you want me to take this patch on its own via the powerpc tree? cheers
Re: [patch V2 01/23] powerpc/4xx: Remove MSI support which never worked
Cédric Le Goater writes:
> Hello Thomas,
>
> On 12/6/21 23:27, Thomas Gleixner wrote:
>> This code is broken since day one. ppc4xx_setup_msi_irqs() has the
>> following gems:
>>
>> 1) The handling of the result of msi_bitmap_alloc_hwirqs() is completely
>> broken:
>>
>> When the result is greater than or equal 0 (bitmap allocation
>> successful) then the loop terminates and the function returns 0
>> (success) despite not having installed an interrupt.
>>
>> When the result is less than 0 (bitmap allocation fails), it prints an
>> error message and continues to "work" with that error code which would
>> eventually end up in the MSI message data.
>>
>> 2) On every invocation the file global pp4xx_msi::msi_virqs bitmap is
>> allocated thereby leaking the previous one.
>>
>> IOW, this has never worked and for more than 10 years nobody cared. Remove
>> the gunk.
>>
>> Fixes: 3fb7933850fa ("powerpc/4xx: Adding PCIe MSI support")
>
> Shouldn't we remove all of it ? including the updates in the device trees
> and the Kconfig changes under :
>
> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
> arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
> arch/powerpc/platforms/40x/Kconfig: select PPC4xx_MSI
This patch should drop those selects I guess. Can you send an
incremental diff for Thomas to squash in?
Removing all the tendrils in various device tree files will probably
require some archaeology, and it should be perfectly safe to leave those
in the tree with the driver gone. So I think we can do that as a
subsequent patch, rather than in this series.
cheers
Re: [PATCH v2 3/7] mm/memory_hotplug: prepare passing flags to add_memory() and friends
David Hildenbrand writes: > On 09.09.20 09:17, Greg Kroah-Hartman wrote: >> On Tue, Sep 08, 2020 at 10:10:08PM +0200, David Hildenbrand wrote: >>> We soon want to pass flags, e.g., to mark added System RAM resources. >>> mergeable. Prepare for that. >> >> What are these random "flags", and how do we know what should be passed >> to them? >> >> Why not make this an enumerated type so that we know it all works >> properly, like the GPF_* flags are? Passing around a random unsigned >> long feels very odd/broken... > > Agreed, an enum (mhp_flags) seems to give a better hint what can > actually be passed. Thanks! You probably know this but ... Just using a C enum doesn't get you any type safety. You can get some checking via sparse by using __bitwise, which is what gfp_t does. You don't actually have to use an enum for that, it works with #defines also. Or you can wrap the flag in a struct, the way atomic_t does, and then the compiler will prevent passing plain integers in place of your custom type. cheers
Re: [Xen-devel] [GIT PULL] dma-mapping updates for 5.4
On 20 September 2019 6:33:50 am AEST, Linus Torvalds wrote: >On Wed, Sep 18, 2019 at 8:27 AM Christoph Hellwig >wrote: >> >> please pull the dma-mapping updates for 5.4. > >Pulled. > >> In addition to the usual Kconfig conflics where you just want to keep >> both edits there are a few more interesting merge issues this time: >> >> - most importanly powerpc and microblaze add new callers of >>dma_atomic_pool_init, while this tree marks the function static >>and calls it from a common postcore_initcall(). The trivial >>functions added in powerpc and microblaze adding the calls >>need to be removed for the code to compile. This will not show up >>as a merge conflict and needs to be dealt with manually! > >So I haven't gotten the powerpc or microblaze pull requests yet, so >I'm not able to fix that part up yet. > >Intead, I'm cc'ing Michael Ellerman and Michal Simek to ask them to >remind me when they _do_ send those pull requests, since otherwise I >may well forget and miss it. Without an actual data conflict, and >since this won't show up in my build tests either, it would be very >easy for me to forget. > >Micha[e]l, can you both please make sure to remind me? Yeah I was aware of it, and will make sure to remind you in my pull request. cheers -- Sent from my Android phone with K-9 Mail. Please excuse my brevity. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 38/41] powerpc: convert put_page() to put_user_page*()
John Hubbard writes:
> On 8/7/19 10:42 PM, Michael Ellerman wrote:
>> Hi John,
>>
>> john.hubb...@gmail.com writes:
>>> diff --git a/arch/powerpc/mm/book3s64/iommu_api.c
>>> b/arch/powerpc/mm/book3s64/iommu_api.c
>>> index b056cae3388b..e126193ba295 100644
>>> --- a/arch/powerpc/mm/book3s64/iommu_api.c
>>> +++ b/arch/powerpc/mm/book3s64/iommu_api.c
>>> @@ -203,6 +202,7 @@ static void mm_iommu_unpin(struct
>>> mm_iommu_table_group_mem_t *mem)
>>> {
>>> long i;
>>> struct page *page = NULL;
>>> + bool dirty = false;
>>
>> I don't think you need that initialisation do you?
>>
>
> Nope, it can go. Fixed locally, thanks.
Thanks.
> Did you get a chance to look at enough of the other bits to feel comfortable
> with the patch, overall?
Mostly :) It's not really my area, but all the conversions looked
correct to me as best as I could tell.
So I'm fine for it to go in as part of the series:
Acked-by: Michael Ellerman (powerpc)
cheers
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 38/41] powerpc: convert put_page() to put_user_page*()
Hi John,
john.hubb...@gmail.com writes:
> diff --git a/arch/powerpc/mm/book3s64/iommu_api.c
> b/arch/powerpc/mm/book3s64/iommu_api.c
> index b056cae3388b..e126193ba295 100644
> --- a/arch/powerpc/mm/book3s64/iommu_api.c
> +++ b/arch/powerpc/mm/book3s64/iommu_api.c
> @@ -203,6 +202,7 @@ static void mm_iommu_unpin(struct
> mm_iommu_table_group_mem_t *mem)
> {
> long i;
> struct page *page = NULL;
> + bool dirty = false;
I don't think you need that initialisation do you?
> if (!mem->hpas)
> return;
> @@ -215,10 +215,9 @@ static void mm_iommu_unpin(struct
> mm_iommu_table_group_mem_t *mem)
> if (!page)
> continue;
>
> - if (mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY)
> - SetPageDirty(page);
> + dirty = mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY;
> - put_page(page);
> + put_user_pages_dirty_lock(&page, 1, dirty);
> mem->hpas[i] = 0;
> }
> }
cheers
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 10/21] memblock: refactor internal allocation functions
Mike Rapoport writes: > Currently, memblock has several internal functions with overlapping > functionality. They all call memblock_find_in_range_node() to find free > memory and then reserve the allocated range and mark it with kmemleak. > However, there is difference in the allocation constraints and in fallback > strategies. > > The allocations returning physical address first attempt to find free > memory on the specified node within mirrored memory regions, then retry on > the same node without the requirement for memory mirroring and finally fall > back to all available memory. > > The allocations returning virtual address start with clamping the allowed > range to memblock.current_limit, attempt to allocate from the specified > node from regions with mirroring and with user defined minimal address. If > such allocation fails, next attempt is done with node restriction lifted. > Next, the allocation is retried with minimal address reset to zero and at > last without the requirement for mirrored regions. > > Let's consolidate various fallbacks handling and make them more consistent > for physical and virtual variants. Most of the fallback handling is moved > to memblock_alloc_range_nid() and it now handles node and mirror fallbacks. > > The memblock_alloc_internal() uses memblock_alloc_range_nid() to get a > physical address of the allocated range and converts it to virtual address. > > The fallback for allocation below the specified minimal address remains in > memblock_alloc_internal() because memblock_alloc_range_nid() is used by CMA > with exact requirement for lower bounds. This is causing problems on some of my machines. I see NODE_DATA allocations falling back to node 0 when they shouldn't, or didn't previously. eg, before: 57990190: (116011251): numa: NODE_DATA [mem 0xfffe4980-0xfffebfff] 58152042: (116373087): numa: NODE_DATA [mem 0x8fff90980-0x8fff97fff] after: 16356872061562: (6296877055): numa: NODE_DATA [mem 0xfffe4980-0xfffebfff] 16356872079279: (6296894772): numa: NODE_DATA [mem 0xfffcd300-0xfffd497f] 16356872096376: (6296911869): numa: NODE_DATA(1) on node 0 On some of my other systems it does that, and then panics because it can't allocate anything at all: [0.00] numa: NODE_DATA [mem 0x7ffcaee80-0x7ffcb3fff] [0.00] numa: NODE_DATA [mem 0x7ffc99d00-0x7ffc9ee7f] [0.00] numa: NODE_DATA(1) on node 0 [0.00] Kernel panic - not syncing: Cannot allocate 20864 bytes for node 16 data [0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc4-gccN-next-20190201-gdc4c899 #1 [0.00] Call Trace: [0.00] [c11cfca0] [c0c11044] dump_stack+0xe8/0x164 (unreliable) [0.00] [c11cfcf0] [c00fdd6c] panic+0x17c/0x3e0 [0.00] [c11cfd90] [c0f61bc8] initmem_init+0x128/0x260 [0.00] [c11cfe60] [c0f57940] setup_arch+0x398/0x418 [0.00] [c11cfee0] [c0f50a94] start_kernel+0xa0/0x684 [0.00] [c11cff90] [c000af70] start_here_common+0x1c/0x52c [0.00] Rebooting in 180 seconds.. So there's something going wrong there, I haven't had time to dig into it though (Sunday night here). cheers ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 09/21] memblock: drop memblock_alloc_base()
Mike Rapoport writes: > The memblock_alloc_base() function tries to allocate a memory up to the > limit specified by its max_addr parameter and panics if the allocation > fails. Replace its usage with memblock_phys_alloc_range() and make the > callers check the return value and panic in case of error. > > Signed-off-by: Mike Rapoport > --- > arch/powerpc/kernel/rtas.c | 6 +- > arch/powerpc/mm/hash_utils_64.c | 8 ++-- > arch/s390/kernel/smp.c | 6 +- > drivers/macintosh/smu.c | 2 +- > include/linux/memblock.h| 2 -- > mm/memblock.c | 14 -- > 6 files changed, 17 insertions(+), 21 deletions(-) Acked-by: Michael Ellerman (powerpc) cheers ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 06/21] memblock: memblock_phys_alloc_try_nid(): don't panic
Michael Ellerman writes:
> Mike Rapoport writes:
>
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index ae34e3a..2c61ea4 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -237,6 +237,10 @@ static void __init setup_node_data(int nid, u64
>> start_pfn, u64 end_pfn)
>> pr_info("Initmem setup node %d []\n", nid);
>>
>> nd_pa = memblock_phys_alloc_try_nid(nd_size, SMP_CACHE_BYTES, nid);
>> +if (!nd_pa)
>> +panic("Cannot allocate %zu bytes for node %d data\n",
>> + nd_size, nid);
>> +
>> nd = __va(nd_pa);
Wrong hunk, O_o
> Acked-by: Michael Ellerman (powerpc)
You know what I mean though :)
cheers
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 06/21] memblock: memblock_phys_alloc_try_nid(): don't panic
Mike Rapoport writes:
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index ae34e3a..2c61ea4 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -237,6 +237,10 @@ static void __init setup_node_data(int nid, u64
> start_pfn, u64 end_pfn)
> pr_info("Initmem setup node %d []\n", nid);
>
> nd_pa = memblock_phys_alloc_try_nid(nd_size, SMP_CACHE_BYTES, nid);
> + if (!nd_pa)
> + panic("Cannot allocate %zu bytes for node %d data\n",
> + nd_size, nid);
> +
> nd = __va(nd_pa);
Acked-by: Michael Ellerman (powerpc)
cheers
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 02/21] powerpc: use memblock functions returning virtual address
Mike Rapoport writes: > From: Christophe Leroy > > Since only the virtual address of allocated blocks is used, > lets use functions returning directly virtual address. > > Those functions have the advantage of also zeroing the block. > > [ MR: > - updated error message in alloc_stack() to be more verbose > - convereted several additional call sites ] > > Signed-off-by: Christophe Leroy > Signed-off-by: Mike Rapoport > --- > arch/powerpc/kernel/dt_cpu_ftrs.c | 3 +-- > arch/powerpc/kernel/irq.c | 5 - > arch/powerpc/kernel/paca.c| 6 +- > arch/powerpc/kernel/prom.c| 5 - > arch/powerpc/kernel/setup_32.c| 26 -- > 5 files changed, 26 insertions(+), 19 deletions(-) LGTM. Acked-by: Michael Ellerman cheers ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
