Re: [PATCH v2 2/3] RISC-V: Add kdump support

From: Nick Kossifidis <mick@ics.forth.gr>
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: david.abdurachmanov@sifive.com, anup@brainfault.org,
	Atish Patra <Atish.Patra@wdc.com>,
	yibin_liu@c-sky.com, zong.li@sifive.com,
	Paul Walmsley <paul.walmsley@sifive.com>,
	mick@ics.forth.gr, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v2 2/3] RISC-V: Add kdump support
Date: Thu, 16 Jul 2020 18:31:29 +0300	[thread overview]
Message-ID: <3e3af982459083a8467662ca1114007c@mailhost.ics.forth.gr> (raw)
In-Reply-To: <mhng-8b167c99-bbdf-420c-95a4-40ad9d706d45@palmerdabbelt-glaptop1>

Στις 2020-07-11 06:59, Palmer Dabbelt έγραψε:
> On Tue, 23 Jun 2020 08:05:11 PDT (-0700), mick@ics.forth.gr wrote:
>> 
>> +
>> +	/* Switch to physical addressing */
>> +	csrw	sptbr, zero
> 
> I guess I'm missing something, but I'd assume that we would need some 
> sort of
> tvec juggling here like we usually do when jumping between virtual and 
> physical
> spaces.  I get that we're not relocating the kernel that we're jumping 
> to, but
> I don't understand how PAGE_OFFSET works.
> 

We only clean up registers after this, interrupts are disabled, and jalr 
will be called with an absolute physical address after satp is zero. I 
don't see how we can end up with a trap but I can add a1 to stvec just 
in case if you want.

>> +
>> +	/* Pass the arguments to the next kernel  / Cleanup*/
> 
> Missing space in that comment.
> 

ACK

>> +	/* Add /memory regions to the resource tree */
>> +	for_each_memblock(memory, region) {
>> +		res = memblock_alloc(sizeof(struct resource), SMP_CACHE_BYTES);
>> +		if (!res)
>> +			panic("%s: Failed to allocate %zu bytes\n", __func__,
>> +			      sizeof(struct resource));
>> +
>> +		if (unlikely(memblock_is_nomap(region))) {
>> +			res->name = "Reserved";
>> +			res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> 
> This differs from what I ended up with in that I don't have 
> IORESOURCE_BUSY.  I
> just copied this blindly from arm64 so I don't actually know what the
> difference is.
> 

For now since we don't mark any /memory regions with the NOMAP flag (ARM 
for example does this for the kernel image region) this code won't be 
executed, I put it there for completeness in case we do so in the 
future. In any case the idea is to prevent a region marked with NOMAP to 
be accessible through /dev/mem when STRICT_DEVMEM & IO_STRICT_DEVMEM is 
enabled. On devmem_is_allowed(), the default page_is_ram() will check 
for IORESOURCE_SYSTEM_RAM flag so it will not prevent this region from 
being mapped through /dev/mem since it's flaged with IORESOURCE_MEM 
instead. The idea is to rely on iomem_is_exclusive() to do the work, and 
for that we need the region to be marked as busy 
(https://elixir.bootlin.com/linux/v5.8-rc4/source/kernel/resource.c#L1614).

I wanted to do something to also honor the no_map flag on the 
/reserved-memory regions that according to the device tree spec, 
shouldn't be available to any other than the driver (so we should also 
prevent them from being accessible through /dev/mem), and I noticed that 
other archs don't deal with this. However this was a bit off-topic since 
it had nothing to do with kexec/kdump.

> 
> If you split out init_resource() then I can take it before the rest of 
> the
> kexec stuff.

I'll send a patch for populating ioresources and also add the extra 
sauce for handling no_map for /reserved-memory regions. Then I'll send 
kdump / crashkernel on top. Is it OK if I remove the existing code from 
mm/init.c and add the new one on kernel/setup.c ? After all it's not 
related to mm, ioresources is a different thing and other archs also 
handle this on setup.c.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv