Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Baoquan He]

On 05/03/16 at 11:12am, Russell King - ARM Linux wrote:
> On Tue, May 03, 2016 at 12:24:41PM +0800, Baoquan He wrote:
> > Could you please help tell why arm PAE kernel can be put above 4G?
> > Since the change is related to common code, I am curious about how
> > it's so different with other ARCHs.
> 
> This is explained in the covering email to the series.
> 
> The explanation given by Pratyush was incomplete.

Yeah, he said non LPAE also has PHYS_OFFSET later to me. In fact this
patch is totally unharmful to other ARCHes, I just can't stand the
curiosity. Sorry about this.

> 
> -- 
> RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Russell King - ARM Linux]

On Tue, May 03, 2016 at 12:24:41PM +0800, Baoquan He wrote:
> Could you please help tell why arm PAE kernel can be put above 4G?
> Since the change is related to common code, I am curious about how
> it's so different with other ARCHs.

This is explained in the covering email to the series.

The explanation given by Pratyush was incomplete.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Baoquan He]

On 05/03/16 at 11:23am, Pratyush Anand wrote:
> Hi Baoquan,
> 
> On 03/05/2016:12:24:41 PM, Baoquan He wrote:
> > Hi Pratyush,
> > 
> > Could you please help tell why arm PAE kernel can be put above 4G?
> 
> PAE system can have physical addresses above 4G. So, if a CPU is supporting 
> the
> LPAE page table format then we should be able to access physical addresses
> beyond 4G.

OK, after patient explanation from Pratush on irc, I finally understand on
arm LPAE there's PHYS_OFFSET which indicats the physical address of main
memory since arm could map some amount of device memory to cpu address space
below 4G. Then physical ram has to be started from above 4G form cpu
point of view. That's why kernel need be put above 4G. It's fair enough to
fix it in this patch. Certainly if this explanation is also added into
patch log, non arm developer will understand the change better.

Thanks
Baoquan

> 
> > Since the change is related to common code, I am curious about how
> > it's so different with other ARCHs.
> 
> paddr_vmcoreinfo_note() returns a physical address, so returning phys_addr_t
> seems most appropriate. So, kernel variable may land into above 4G locations,
> even with the platform in other architecture (with PAE support), if start of 
> RAM
> is located very high,
> 
> So, in my opinion it would be safer to have these changes for other ARCHs as
> well.
> 
> ~Pratyush
> 
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Pratyush Anand]

Hi Baoquan,

On 03/05/2016:12:24:41 PM, Baoquan He wrote:
> Hi Pratyush,
> 
> Could you please help tell why arm PAE kernel can be put above 4G?

PAE system can have physical addresses above 4G. So, if a CPU is supporting the
LPAE page table format then we should be able to access physical addresses
beyond 4G.

> Since the change is related to common code, I am curious about how
> it's so different with other ARCHs.

paddr_vmcoreinfo_note() returns a physical address, so returning phys_addr_t
seems most appropriate. So, kernel variable may land into above 4G locations,
even with the platform in other architecture (with PAE support), if start of RAM
is located very high,

So, in my opinion it would be safer to have these changes for other ARCHs as
well.

~Pratyush
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Baoquan He]

This patch is clearly related to kdump. The prefix of subject should be
changed to kdump. Kexec doesn't need to handle vmcore things. 

And patches realted to kexec/kdump should be CCed to Andrew, he usually
picks up and add them into akpm tree.

Hi Pratyush,

Could you please help tell why arm PAE kernel can be put above 4G?
Since the change is related to common code, I am curious about how
it's so different with other ARCHs.

Thanks
Baoquan


On 04/29/16 at 09:17pm, Pratyush Anand wrote:
> On Fri, Apr 29, 2016 at 8:46 PM, Mark Rutland  wrote:
> > On Fri, Apr 29, 2016 at 08:36:43PM +0530, Pratyush Anand wrote:
> >> > +   phys_addr_t vmcore_base = paddr_vmcoreinfo_note();
> >> > +   return sprintf(buf, "%pa %x\n", _base,
> >>
> >> Why do we pass _base? Shouldn't it be vmcore_base?
> >
> > The %pa* printk format specifiers take the value by reference (as
> > phys_addr_t and friends are not necessarily the same width as a
> > pointer). Per Documentation/printk-formats.txt:
> >
> > Physical addresses types phys_addr_t:
> >
> > %pa[p]  0x01234567 or 0x0123456789abcdef
> >
> > For printing a phys_addr_t type (and its derivatives, such as
> > resource_size_t) which can vary based on build options, regardless 
> > of
> > the width of the CPU data path. Passed by reference.
> >
> > So the above prints the value of vmcore_base as expected.
> 
> Thanks a lot Mark for explaining :-)
> 
> Reviewed-by: Pratyush Anand 
> 
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Pratyush Anand]

On Fri, Apr 29, 2016 at 11:36 PM, Russell King - ARM Linux
 wrote:
> On Fri, Apr 29, 2016 at 08:36:43PM +0530, Pratyush Anand wrote:
>> On Thu, Apr 28, 2016 at 2:58 PM, Russell King
>>  wrote:
>> > diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
>> > index 152da4a48867..9f1920d2d0c6 100644
>> > --- a/kernel/ksysfs.c
>> > +++ b/kernel/ksysfs.c
>> > @@ -128,8 +128,8 @@ KERNEL_ATTR_RW(kexec_crash_size);
>> >  static ssize_t vmcoreinfo_show(struct kobject *kobj,
>> >struct kobj_attribute *attr, char *buf)
>> >  {
>> > -   return sprintf(buf, "%lx %x\n",
>> > -  paddr_vmcoreinfo_note(),
>> > +   phys_addr_t vmcore_base = paddr_vmcoreinfo_note();
>> > +   return sprintf(buf, "%pa %x\n", _base,
>>
>> Why do we pass _base? Shouldn't it be vmcore_base?
>
> You seem to not know what the "%pa" format string means.

Apologies, Yes, yes.. it was my negligence.

>
> %p always takes a _pointer_ as per C standard, so the printf argument
> must be a pointer.  However, the kernel format strings are extended
> with additional suffixes - in this case 'a', which means that we want
> to print the contents of a _pointer_ to a phys_addr_t.
>
> Full details in Documentation/printk-formats.txt in the kernel.

Thanks for explaining :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Russell King - ARM Linux]

On Fri, Apr 29, 2016 at 08:36:43PM +0530, Pratyush Anand wrote:
> On Thu, Apr 28, 2016 at 2:58 PM, Russell King
>  wrote:
> > diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> > index 152da4a48867..9f1920d2d0c6 100644
> > --- a/kernel/ksysfs.c
> > +++ b/kernel/ksysfs.c
> > @@ -128,8 +128,8 @@ KERNEL_ATTR_RW(kexec_crash_size);
> >  static ssize_t vmcoreinfo_show(struct kobject *kobj,
> >struct kobj_attribute *attr, char *buf)
> >  {
> > -   return sprintf(buf, "%lx %x\n",
> > -  paddr_vmcoreinfo_note(),
> > +   phys_addr_t vmcore_base = paddr_vmcoreinfo_note();
> > +   return sprintf(buf, "%pa %x\n", _base,
> 
> Why do we pass _base? Shouldn't it be vmcore_base?

You seem to not know what the "%pa" format string means.

%p always takes a _pointer_ as per C standard, so the printf argument
must be a pointer.  However, the kernel format strings are extended
with additional suffixes - in this case 'a', which means that we want
to print the contents of a _pointer_ to a phys_addr_t.

Full details in Documentation/printk-formats.txt in the kernel.

The code above is correct.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Pratyush Anand]

On Fri, Apr 29, 2016 at 8:46 PM, Mark Rutland  wrote:
> On Fri, Apr 29, 2016 at 08:36:43PM +0530, Pratyush Anand wrote:
>> > +   phys_addr_t vmcore_base = paddr_vmcoreinfo_note();
>> > +   return sprintf(buf, "%pa %x\n", _base,
>>
>> Why do we pass _base? Shouldn't it be vmcore_base?
>
> The %pa* printk format specifiers take the value by reference (as
> phys_addr_t and friends are not necessarily the same width as a
> pointer). Per Documentation/printk-formats.txt:
>
> Physical addresses types phys_addr_t:
>
> %pa[p]  0x01234567 or 0x0123456789abcdef
>
> For printing a phys_addr_t type (and its derivatives, such as
> resource_size_t) which can vary based on build options, regardless of
> the width of the CPU data path. Passed by reference.
>
> So the above prints the value of vmcore_base as expected.

Thanks a lot Mark for explaining :-)

Reviewed-by: Pratyush Anand 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Pratyush Anand]

On Thu, Apr 28, 2016 at 2:58 PM, Russell King
 wrote:
> On PAE systems (eg, ARM LPAE) the vmcore note may be located above
> 4GB physical on 32-bit architectures, so we need a wider type than
> "unsigned long" here.  Arrange for paddr_vmcoreinfo_note() to return
> a phys_addr_t, thereby allowing it to be located above 4GB.
>
> This makes no difference for kexec-tools, as they already assume a
> 64-bit type when reading from this file.
>
> Signed-off-by: Russell King 
> ---
>  arch/ia64/kernel/machine_kexec.c | 2 +-
>  include/linux/kexec.h| 2 +-
>  kernel/kexec_core.c  | 2 +-
>  kernel/ksysfs.c  | 4 ++--
>  4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/ia64/kernel/machine_kexec.c 
> b/arch/ia64/kernel/machine_kexec.c
> index b72cd7a07222..599507bcec91 100644
> --- a/arch/ia64/kernel/machine_kexec.c
> +++ b/arch/ia64/kernel/machine_kexec.c
> @@ -163,7 +163,7 @@ void arch_crash_save_vmcoreinfo(void)
>  #endif
>  }
>
> -unsigned long paddr_vmcoreinfo_note(void)
> +phys_addr_t paddr_vmcoreinfo_note(void)
>  {
> return ia64_tpa((unsigned long)(char *)_note);
>  }
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 1b32ab587f66..52a3a221bcb2 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -235,7 +235,7 @@ void crash_unmap_reserved_pages(void);
>  void arch_crash_save_vmcoreinfo(void);
>  __printf(1, 2)
>  void vmcoreinfo_append_str(const char *fmt, ...);
> -unsigned long paddr_vmcoreinfo_note(void);
> +phys_addr_t paddr_vmcoreinfo_note(void);
>
>  #define VMCOREINFO_OSRELEASE(value) \
> vmcoreinfo_append_str("OSRELEASE=%s\n", value)
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index d719a4d0ef55..f9847e5822e6 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1377,7 +1377,7 @@ void vmcoreinfo_append_str(const char *fmt, ...)
>  void __weak arch_crash_save_vmcoreinfo(void)
>  {}
>
> -unsigned long __weak paddr_vmcoreinfo_note(void)
> +phys_addr_t __weak paddr_vmcoreinfo_note(void)
>  {
> return __pa((unsigned long)(char *)_note);
>  }
> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> index 152da4a48867..9f1920d2d0c6 100644
> --- a/kernel/ksysfs.c
> +++ b/kernel/ksysfs.c
> @@ -128,8 +128,8 @@ KERNEL_ATTR_RW(kexec_crash_size);
>  static ssize_t vmcoreinfo_show(struct kobject *kobj,
>struct kobj_attribute *attr, char *buf)
>  {
> -   return sprintf(buf, "%lx %x\n",
> -  paddr_vmcoreinfo_note(),
> +   phys_addr_t vmcore_base = paddr_vmcoreinfo_note();
> +   return sprintf(buf, "%pa %x\n", _base,

Why do we pass _base? Shouldn't it be vmcore_base?

~Pratyush
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/12] kexec: arrange for paddr_vmcoreinfo_note() to return phys_addr_t

[Russell King]

On PAE systems (eg, ARM LPAE) the vmcore note may be located above
4GB physical on 32-bit architectures, so we need a wider type than
"unsigned long" here.  Arrange for paddr_vmcoreinfo_note() to return
a phys_addr_t, thereby allowing it to be located above 4GB.

This makes no difference for kexec-tools, as they already assume a
64-bit type when reading from this file.

Signed-off-by: Russell King 
---
 arch/ia64/kernel/machine_kexec.c | 2 +-
 include/linux/kexec.h| 2 +-
 kernel/kexec_core.c  | 2 +-
 kernel/ksysfs.c  | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/ia64/kernel/machine_kexec.c b/arch/ia64/kernel/machine_kexec.c
index b72cd7a07222..599507bcec91 100644
--- a/arch/ia64/kernel/machine_kexec.c
+++ b/arch/ia64/kernel/machine_kexec.c
@@ -163,7 +163,7 @@ void arch_crash_save_vmcoreinfo(void)
 #endif
 }
 
-unsigned long paddr_vmcoreinfo_note(void)
+phys_addr_t paddr_vmcoreinfo_note(void)
 {
return ia64_tpa((unsigned long)(char *)_note);
 }
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 1b32ab587f66..52a3a221bcb2 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -235,7 +235,7 @@ void crash_unmap_reserved_pages(void);
 void arch_crash_save_vmcoreinfo(void);
 __printf(1, 2)
 void vmcoreinfo_append_str(const char *fmt, ...);
-unsigned long paddr_vmcoreinfo_note(void);
+phys_addr_t paddr_vmcoreinfo_note(void);
 
 #define VMCOREINFO_OSRELEASE(value) \
vmcoreinfo_append_str("OSRELEASE=%s\n", value)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index d719a4d0ef55..f9847e5822e6 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -1377,7 +1377,7 @@ void vmcoreinfo_append_str(const char *fmt, ...)
 void __weak arch_crash_save_vmcoreinfo(void)
 {}
 
-unsigned long __weak paddr_vmcoreinfo_note(void)
+phys_addr_t __weak paddr_vmcoreinfo_note(void)
 {
return __pa((unsigned long)(char *)_note);
 }
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 152da4a48867..9f1920d2d0c6 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -128,8 +128,8 @@ KERNEL_ATTR_RW(kexec_crash_size);
 static ssize_t vmcoreinfo_show(struct kobject *kobj,
   struct kobj_attribute *attr, char *buf)
 {
-   return sprintf(buf, "%lx %x\n",
-  paddr_vmcoreinfo_note(),
+   phys_addr_t vmcore_base = paddr_vmcoreinfo_note();
+   return sprintf(buf, "%pa %x\n", _base,
   (unsigned int)sizeof(vmcoreinfo_note));
 }
 KERNEL_ATTR_RO(vmcoreinfo);
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html