Re: [PATCHv4 4/8] x86/boot/compressed: Handle unaccepted memory

[Dave Hansen <dave.hansen@intel.com>]

On 4/9/22 13:20, Kirill A. Shutemov wrote:
> On Fri, Apr 08, 2022 at 10:57:17AM -0700, Dave Hansen wrote:
...
>> It's a real shame that we have to duplicate this code.  Is there
>> anything crazy we could do here like
>>
>> #include "../../../lib/find_bit.c"
>>
>> ?
> 
> Well, it would require fracturing source files on the kernel side.
> 
> __bitmap_set() and __bitmap_clear() are now in lib/bitmap.c.
> 
> _find_next_bit() is in lib/find_bit.c.
> 
> Both lib/bitmap.c and lib/find_bit.c have a lot of stuff that are not used
> here. I guess we would need to split them into few pieces to make it in
> sane way. Do you want me to go this path?

I'd be curious if others have any sane ideas for how to do it.

One idea would be to stick most of the implementation in a header that
we can #include.  Then, lib/find_bit.c #includes that header and does
something simple like:

#include "header.h"
int _find_next_bit(...)
{
	return _find_next_bit_from_header();
}
EXPORT_SYMBOL(_find_next_bit);


>>> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>>> index fa8969fad011..c1d9d71a6615 100644
>>> --- a/arch/x86/boot/compressed/misc.c
>>> +++ b/arch/x86/boot/compressed/misc.c
>>> @@ -18,6 +18,7 @@
>>>  #include "../string.h"
>>>  #include "../voffset.h"
>>>  #include <asm/bootparam_utils.h>
>>> +#include <asm/unaccepted_memory.h>
>>>  
>>>  /*
>>>   * WARNING!!
>>> @@ -43,6 +44,9 @@
>>>  void *memmove(void *dest, const void *src, size_t n);
>>>  #endif
>>>  
>>> +#undef __pa
>>> +#define __pa(x)	((unsigned long)(x))
>>
>> Those #undef's always worry me.  Why is this one needed?
> 
> arch/x86/boot/compressed/misc.c:47:9: warning: '__pa' macro redefined [-Wmacro-redefined]
> #define __pa(x) ((unsigned long)(x))
>         ^
> arch/x86/include/asm/page.h:47:9: note: previous definition is here
> #define __pa(x)         __phys_addr((unsigned long)(x))
> 
> Note that sev.c does the same. At least we are consistent :)

Ugh.  Please do look into fixing this properly.  The SEV folks will
thank you. :)

>>> +void accept_memory(phys_addr_t start, phys_addr_t end)
>>> +{
>>> +	unsigned long *unaccepted_memory;
>>> +	unsigned int rs, re;
>>> +
>>> +	unaccepted_memory = (unsigned long *)boot_params->unaccepted_memory;
>>> +	rs = start / PMD_SIZE;
>>
>> OK, so start is a physical address, PMD_SIZE is 2^21, and 'rs' is an
>> unsigned int.  That means 'rs' can, at most, represent a physical
>> address at 2^(21+32), or 2^53.  That's cutting it a *bit* close, don't
>> you think?
>>
>> Could we please just give 'rs' and 're' real names and make them
>> 'unsigned long's, please?  It will surely save at least one other person
>> from doing math.  The find_next_bit() functions seem to take ulongs anyway.
> 
> Okay. 'range_start' and 'range_end' are good enough names?

Yep, works for me.