Re: [PATCH v2 2/2] kexec: Consider crashk_low_res in sanity_check_segment_list()

[Xunlei Pang]

On 2016/08/17 at 15:24, Dave Young wrote:
> Hi, Xunlei,
>
> On 08/17/16 at 09:50am, Xunlei Pang wrote:
>> We have crashk_res only in most cases, but sometimes we have
>> crashk_low_res.
>>
>> For example, on 64-bit x86 systems, when "crashkernel=32M,high"
>> combined with "crashkernel=128M,low" is used, so some segments
>> may have the chance to be loaded into crashk_low_res area. We
>> can't fail it as a memory violation in these cases.
>>
>> Thus, we add the case to regard the segment as valid if it is
>> within crashk_low_res.
> crashkernel low is meant for swiotlb, it can be reserved automaticlly
> in case there's only crashkernel high specified in cmdline, I'm not
> sure it is useful to use crashk_res_low for other purpose and
> likely kdump can fail in the case. 
>
> I'm not sure it is really necessary to add this check now, we may
> handle it only when there is an actual use case and bug report in
> the future.

Thanks for the review.
The reason I added this is that crashk_res is allowed to be shrunk, so the 
segment
will surely fall into crashk_low_res if crashk_res was shrunk to be a small 
range.

But yes, this should be a corner case, but seems it does no harm adding this 
check.
Anyway, if you think it's not necessary, let's simply ignore it :-)

Regards,
Xunlei

>
> Thanks
> Dave
>> Signed-off-by: Xunlei Pang 
>> ---
>>  kernel/kexec_core.c | 11 ---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 707d18e..9012a60 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -248,9 +248,14 @@ int sanity_check_segment_list(struct kimage *image)
>>  mstart = image->segment[i].mem;
>>  mend = mstart + image->segment[i].memsz - 1;
>>  /* Ensure we are within the crash kernel limits */
>> -if ((mstart < phys_to_boot_phys(crashk_res.start)) ||
>> -(mend > phys_to_boot_phys(crashk_res.end)))
>> -return -EADDRNOTAVAIL;
>> +if ((mstart >= phys_to_boot_phys(crashk_res.start)) &&
>> +(mend <= phys_to_boot_phys(crashk_res.end)))
>> +continue;
>> +if ((mstart >= phys_to_boot_phys(crashk_low_res.start)) 
>> &&
>> +(mend <= phys_to_boot_phys(crashk_low_res.end)))
>> +continue;
>> +
>> +return -EADDRNOTAVAIL;
>>  }
>>  }
>>  
>> -- 
>> 1.8.3.1
>>
> ___
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 2/2] kexec: Consider crashk_low_res in sanity_check_segment_list()

[Dave Young]

Hi, Xunlei,

On 08/17/16 at 09:50am, Xunlei Pang wrote:
> We have crashk_res only in most cases, but sometimes we have
> crashk_low_res.
> 
> For example, on 64-bit x86 systems, when "crashkernel=32M,high"
> combined with "crashkernel=128M,low" is used, so some segments
> may have the chance to be loaded into crashk_low_res area. We
> can't fail it as a memory violation in these cases.
> 
> Thus, we add the case to regard the segment as valid if it is
> within crashk_low_res.

crashkernel low is meant for swiotlb, it can be reserved automaticlly
in case there's only crashkernel high specified in cmdline, I'm not
sure it is useful to use crashk_res_low for other purpose and
likely kdump can fail in the case. 

I'm not sure it is really necessary to add this check now, we may
handle it only when there is an actual use case and bug report in
the future.

Thanks
Dave
> 
> Signed-off-by: Xunlei Pang 
> ---
>  kernel/kexec_core.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 707d18e..9012a60 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -248,9 +248,14 @@ int sanity_check_segment_list(struct kimage *image)
>   mstart = image->segment[i].mem;
>   mend = mstart + image->segment[i].memsz - 1;
>   /* Ensure we are within the crash kernel limits */
> - if ((mstart < phys_to_boot_phys(crashk_res.start)) ||
> - (mend > phys_to_boot_phys(crashk_res.end)))
> - return -EADDRNOTAVAIL;
> + if ((mstart >= phys_to_boot_phys(crashk_res.start)) &&
> + (mend <= phys_to_boot_phys(crashk_res.end)))
> + continue;
> + if ((mstart >= phys_to_boot_phys(crashk_low_res.start)) 
> &&
> + (mend <= phys_to_boot_phys(crashk_low_res.end)))
> + continue;
> +
> + return -EADDRNOTAVAIL;
>   }
>   }
>  
> -- 
> 1.8.3.1
> 

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec