Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Nick Piggin]

On Tuesday 09 October 2007 16:40, Huang, Ying wrote:

> +unsigned long copy_from_phys(void *to, unsigned long from_phys,
> +			     unsigned long n)
> +{
> +	struct page *page;
> +	void *from;
> +	unsigned long remain = n, offset, trunck;
> +
> +	while (remain) {
> +		page = pfn_to_page(from_phys >> PAGE_SHIFT);
> +		from = kmap_atomic(page, KM_USER0);
> +		offset = from_phys & ~PAGE_MASK;
> +		if (remain > PAGE_SIZE - offset)
> +			trunck = PAGE_SIZE - offset;
> +		else
> +			trunck = remain;
> +		memcpy(to, from + offset, trunck);
> +		kunmap_atomic(from, KM_USER0);
> +		to += trunck;
> +		from_phys += trunck;
> +		remain -= trunck;
> +	}
> +	return n;
> +}


I suppose that's not unreasonable to put in mm/memory.c, although
it's not really considered a problem to do this kind of stuff in
a low level arch file...

You have no kernel virtual mapping for the source data?

Should it be __init?

Care to add a line of documentation if you keep it in mm/memory.c?

Thanks,
Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Nick Piggin]

On Tuesday 09 October 2007 18:22, Huang, Ying wrote:
> On Tue, 2007-10-09 at 01:25 +1000, Nick Piggin wrote:
> > On Tuesday 09 October 2007 16:40, Huang, Ying wrote:
> > > +unsigned long copy_from_phys(void *to, unsigned long from_phys,
> > > +			     unsigned long n)

> > I suppose that's not unreasonable to put in mm/memory.c, although
> > it's not really considered a problem to do this kind of stuff in
> > a low level arch file...
> >
> > You have no kernel virtual mapping for the source data?
>
> On 32-bit platform such as i386. Some memory zones have no kernel
> virtual mapping (highmem region etc).

I'm just wondering whether you really need to access highmem in
boot code...


> So I think this may be useful as a 
> universal way to access physical memory. But it can be more efficient to
> implement it in arch file for some arch. Should this implementation be
> used as a fall back implementation with attribute "weak"?

Definitely on most architectures it would just amount to
memcpy(dst, __va(phys), n);, right? However I don't know if
it's worth the trouble of overriding it unless there is some
non-__init user of it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Nick Piggin]

On Tuesday 09 October 2007 18:55, Huang, Ying wrote:
> On Tue, 2007-10-09 at 02:06 +1000, Nick Piggin wrote:

> > I'm just wondering whether you really need to access highmem in
> > boot code...
>
> Because the zero page (boot_parameters) of i386 boot protocol has 4k
> limitation, a linked list style boot parameter passing mechanism (struct
> setup_data) is proposed by Peter Anvin. The linked list is provided by
> bootloader, so it is possible to be in highmem region.

OK. I don't really know the code, but I trust you ;)


> > Definitely on most architectures it would just amount to
> > memcpy(dst, __va(phys), n);, right? However I don't know if
>
> Yes.
>
> > it's worth the trouble of overriding it unless there is some
> > non-__init user of it.
>
> To support debugging and kexec, the boot parameters include the linked
> list above are exported into sysfs. This function is used there too. The
> patch is the No. 2 of the series.

Ah, I see. I missed that.

OK, well rather than make it weak, and have everyone except
i386 override it, can you just ifdef CONFIG_HIGHMEM?

After that, I guess most other architectures wouldn't even
use the function. Maybe it can go into lib/ instead so that
it can be discarded by the linker if it isn't used?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Huang, Ying]

On Tue, 2007-10-09 at 01:25 +1000, Nick Piggin wrote:
> On Tuesday 09 October 2007 16:40, Huang, Ying wrote:
> 
> > +unsigned long copy_from_phys(void *to, unsigned long from_phys,
> > +			     unsigned long n)
> > +{
> > +	struct page *page;
> > +	void *from;
> > +	unsigned long remain = n, offset, trunck;
> > +
> > +	while (remain) {
> > +		page = pfn_to_page(from_phys >> PAGE_SHIFT);
> > +		from = kmap_atomic(page, KM_USER0);
> > +		offset = from_phys & ~PAGE_MASK;
> > +		if (remain > PAGE_SIZE - offset)
> > +			trunck = PAGE_SIZE - offset;
> > +		else
> > +			trunck = remain;
> > +		memcpy(to, from + offset, trunck);
> > +		kunmap_atomic(from, KM_USER0);
> > +		to += trunck;
> > +		from_phys += trunck;
> > +		remain -= trunck;
> > +	}
> > +	return n;
> > +}
> 
> 
> I suppose that's not unreasonable to put in mm/memory.c, although
> it's not really considered a problem to do this kind of stuff in
> a low level arch file...
> 
> You have no kernel virtual mapping for the source data?
> 

On 32-bit platform such as i386. Some memory zones have no kernel
virtual mapping (highmem region etc). So I think this may be useful as a
universal way to access physical memory. But it can be more efficient to
implement it in arch file for some arch. Should this implementation be
used as a fall back implementation with attribute "weak"?

> Should it be __init?
> 
> Care to add a line of documentation if you keep it in mm/memory.c?
> 

OK, I will add the document in the next version.

Best Regards,
Huang Ying

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Huang, Ying]

On Tue, 2007-10-09 at 02:06 +1000, Nick Piggin wrote:
> On Tuesday 09 October 2007 18:22, Huang, Ying wrote:
> > On Tue, 2007-10-09 at 01:25 +1000, Nick Piggin wrote:
> > > On Tuesday 09 October 2007 16:40, Huang, Ying wrote:
> > > > +unsigned long copy_from_phys(void *to, unsigned long from_phys,
> > > > +			     unsigned long n)
> 
> > > I suppose that's not unreasonable to put in mm/memory.c, although
> > > it's not really considered a problem to do this kind of stuff in
> > > a low level arch file...
> > >
> > > You have no kernel virtual mapping for the source data?
> >
> > On 32-bit platform such as i386. Some memory zones have no kernel
> > virtual mapping (highmem region etc).
> 
> I'm just wondering whether you really need to access highmem in
> boot code...

Because the zero page (boot_parameters) of i386 boot protocol has 4k
limitation, a linked list style boot parameter passing mechanism (struct
setup_data) is proposed by Peter Anvin. The linked list is provided by
bootloader, so it is possible to be in highmem region.

> 
> > So I think this may be useful as a 
> > universal way to access physical memory. But it can be more efficient to
> > implement it in arch file for some arch. Should this implementation be
> > used as a fall back implementation with attribute "weak"?
> 
> Definitely on most architectures it would just amount to
> memcpy(dst, __va(phys), n);, right? However I don't know if

Yes.

> it's worth the trouble of overriding it unless there is some
> non-__init user of it.

To support debugging and kexec, the boot parameters include the linked
list above are exported into sysfs. This function is used there too. The
patch is the No. 2 of the series.

Best Regards,
Huang Ying

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Huang, Ying]

On Tue, 2007-10-09 at 02:28 +1000, Nick Piggin wrote:
> On Tuesday 09 October 2007 18:55, Huang, Ying wrote:
> > On Tue, 2007-10-09 at 02:06 +1000, Nick Piggin wrote:
> 
> > > I'm just wondering whether you really need to access highmem in
> > > boot code...
> >
> > Because the zero page (boot_parameters) of i386 boot protocol has 4k
> > limitation, a linked list style boot parameter passing mechanism (struct
> > setup_data) is proposed by Peter Anvin. The linked list is provided by
> > bootloader, so it is possible to be in highmem region.
> 
> OK. I don't really know the code, but I trust you ;)

Thank you :)

> 
> > > Definitely on most architectures it would just amount to
> > > memcpy(dst, __va(phys), n);, right? However I don't know if
> >
> > Yes.
> >
> > > it's worth the trouble of overriding it unless there is some
> > > non-__init user of it.
> >
> > To support debugging and kexec, the boot parameters include the linked
> > list above are exported into sysfs. This function is used there too. The
> > patch is the No. 2 of the series.
> 
> Ah, I see. I missed that.
> 
> OK, well rather than make it weak, and have everyone except
> i386 override it, can you just ifdef CONFIG_HIGHMEM?

Yes. This is better. I will do it. Maybe it can be defined as a macro
for these architectures, as follow:

/* in linux/mm.h */
#ifdef CONFIG_HIGHMEM
void *copy_from_phys(void *to, unsigned long from_phys, size_t n);
#else
#define copy_from_phys(dst, phys, n)	memcpy(dst, __va(phys), n)
#endif

> After that, I guess most other architectures wouldn't even
> use the function. Maybe it can go into lib/ instead so that
> it can be discarded by the linker if it isn't used?

Yes. I will do it.

Best Regards,
Huang Ying

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Andi Kleen]

> Care to add a line of documentation if you keep it in mm/memory.c?

It would be better to just use early_ioremap() (or ioremap())

That is how ACPI who has similar issues accessing its tables solves this.

-Andi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Oleg Verych]

* Tue, 09 Oct 2007 16:55:23 +0800
>
> On Tue, 2007-10-09 at 02:06 +1000, Nick Piggin wrote:
>> On Tuesday 09 October 2007 18:22, Huang, Ying wrote:
[]
>> I'm just wondering whether you really need to access highmem in
>> boot code...
>
> Because the zero page (boot_parameters) of i386 boot protocol has 4k
> limitation, a linked list style boot parameter passing mechanism (struct
> setup_data) is proposed by Peter Anvin. The linked list is provided by
> bootloader, so it is possible to be in highmem region.

Can it be explained, why boot protocol and boot line must be expanded?
This amount of code for what?

 arch/i386/Kconfig            |    3 -
 arch/i386/boot/header.S      |    8 +++
 arch/i386/kernel/setup.c     |   92 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86_64/kernel/setup.c   |   37 +++++++++++++++++
 include/asm-i386/bootparam.h |   15 +++++++
 include/asm-i386/io.h        |    7 +++
 include/linux/mm.h           |    2
 mm/memory.c                  |   24 +++++++++++
 

If it is proposed for passing ACPI makeup language bugfixes by boot
line for ACPI parser in the kernel, or "telling to kernel what to do
via EFI" then it's kind of very nasty red flag.

I'd suggest to have initramfs image ready with all possible
data/options/actions based on very small amount of possible boot line
information.

Any _right_ use-cases explained for dummies are appreciated.

Thanks.
--
-o--=O`C
 #oo'L O
<___=E M

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[huang ying]

On 10/9/07, Andi Kleen <ak@suse.de> wrote:
>
> > Care to add a line of documentation if you keep it in mm/memory.c?
>
> It would be better to just use early_ioremap() (or ioremap())
>
> That is how ACPI who has similar issues accessing its tables solves this.

Yes. That is another solution. But there is some problem about
early_ioremap (boot_ioremap, bt_ioremap for i386) or ioremap.

- ioremap can not be used before mem_init.
- For i386, boot_ioremap can map at most 4 pages, bt_ioremap can map
at most 16 pages. This will be an unnecessary constrains for size of
setup_data.
- For i386, the virtual memory space of ioremap is limited too.

Best Regards,
Huang Ying

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm -v4 1/3] i386/x86_64 boot: setup data

[Andi Kleen]

On Tuesday 09 October 2007 16:00:57 huang ying wrote:
> On 10/9/07, Andi Kleen <ak@suse.de> wrote:
> >
> > > Care to add a line of documentation if you keep it in mm/memory.c?
> >
> > It would be better to just use early_ioremap() (or ioremap())
> >
> > That is how ACPI who has similar issues accessing its tables solves this.
> 
> Yes. That is another solution. But there is some problem about
> early_ioremap (boot_ioremap, bt_ioremap for i386) or ioremap.
> 
> - ioremap can not be used before mem_init.
> - For i386, boot_ioremap can map at most 4 pages, bt_ioremap can map
> at most 16 pages. This will be an unnecessary constrains for size of
> setup_data.

That could be easily extended if needed. But I don't see why we would
need that much setup data anyways. Limiting it to let's say 16KB
seems entirely reasonable. And if some kernel ever needs more it can 
be still extended.

The biggest item is probably the command line and i don't see why
that should be more than a one or two KB.

> - For i386, the virtual memory space of ioremap is limited too.

That will be all freed and again the data shouldn't be that big.

-Andi

 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>