[SLOF] [PATCH v2 07/11] libnet: Wire up pxelinux.cfg network booting

[Alexey Kardashevskiy]

On 25/5/18 12:39 am, Thomas Huth wrote:
> On 24.05.2018 11:01, Alexey Kardashevskiy wrote:
>> On 19/5/18 1:45 am, Thomas Huth wrote:
>>> In case the normal network loading failed, try to load a pxelinux.cfg
>>> config file. If that succeeds, load the kernel and initrd with the
>>> information that could be found in this file.
>>>
>>> Signed-off-by: Thomas Huth <thuth at redhat.com>
>>> ---
>>>  include/helpers.h    |  2 ++
>>>  lib/libnet/netload.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  slof/helpers.c       | 15 ++++++++++---
>>>  3 files changed, 73 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/helpers.h b/include/helpers.h
>>> index 04ee771..9dfe3ae 100644
>>> --- a/include/helpers.h
>>> +++ b/include/helpers.h
>>> @@ -36,6 +36,8 @@ extern void SLOF_pci_config_write16(long offset, long value);
>>>  extern void SLOF_pci_config_write8(long offset, long value);
>>>  extern void *SLOF_translate_my_address(void *addr);
>>>  extern int write_mm_log(char *data, unsigned int len, unsigned short type);
>>> +extern void SLOF_set_chosen_int(const char *s, long val);
>>> +extern void SLOF_set_chosen_bytes(const char *s, const char *addr, size_t size);
>>>  extern void SLOF_encode_bootp_response(void *addr, size_t size);
>>>  extern void SLOF_encode_dhcp_response(void *addr, size_t size);
>>>  
>>> diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c
>>> index ba1008c..2a2a7c5 100644
>>> --- a/lib/libnet/netload.c
>>> +++ b/lib/libnet/netload.c
>>> @@ -26,6 +26,7 @@
>>>  #include <helpers.h>
>>>  #include "args.h"
>>>  #include "netapps.h"
>>> +#include "pxelinux.h"
>>>  
>>>  #define IP_INIT_DEFAULT 5
>>>  #define IP_INIT_NONE    0
>>> @@ -425,6 +426,60 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
>>>  	return rc;
>>>  }
>>>  
>>> +#define CFG_BUF_SIZE 2048
>>> +#define MAX_LKIA_ENTRIES 16
>>> +static int net_pxelinux_cfg_load(filename_ip_t *fnip, char *loadbase,
>>> +                                 int maxloadlen, uint8_t *mac)
>>
>>
>> The name confused me, there is already pxelinux_load_cfg().
>> net_pxelinux_load() may be?
> 
> Sure, I'll rename it.
> 
>>> +{
>>> +	static char cfgbuf[CFG_BUF_SIZE];
>>
>> This belongs to pxelinux_load_parse_cfg().
> 
> The pointer in the struct lkia entries[] below will point to the
> original location in the above buffer, so this cfgbuf needs to remain
> valid until the end of this function. That means, the memory for the
> buffer should be managed in the calling function instead of doing it in
> pxelinux_load_parse_cfg(). I should maybe update the comments there to
> make this clear... Alternatively, I could also "strdup" the strings for
> the struct lkia entries, but then this caller here has to make sure to
> free the strings again once the information from struct lkia is not
> needed anymore - and IMHO that's uglier and more error-prone than to
> allocate the cfgbuf buffer here on the calling site.


Open coding pxelinux_load_parse_cfg() here would have eliminated the
question in the first place. You export pxelinux_parse_cfg() anyway even
though you do not use it outside lib/libnet/pxelinux.c.


> 
>>  And why static?
> 
> Allocating big buffers on the stack is always critical within paflof,
> we've seen more than enough buffer overruns in the past already. But I
> could malloc() the buffer instead if you prefer that.


It is just 2KB, is it still too much for slof? And when it is static - this
space is always taken even if pxelinux is not used so malloc seems
appropriate here.



> 
>>> +	struct lkia entries[MAX_LKIA_ENTRIES];
>>> +	int def, num_entries, rc, ilen;
>>> +
>>> +	num_entries = pxelinux_load_parse_cfg(fnip, mac, NULL, cfgbuf,
>>> +	                                      CFG_BUF_SIZE, entries,
>>> +	                                      MAX_LKIA_ENTRIES, &def);
>>> +	if (num_entries <= 0)
>>> +		return -1;
>>> +
>>> +	/* Load kernel */
>>> +	strncpy(fnip->filename, entries[def].kernel,
>>> +		sizeof(fnip->filename) - 1);
>>> +	fnip->filename[sizeof(fnip->filename) - 1] = 0;
>>> +	rc = tftp_load(fnip, loadbase, maxloadlen);
>>> +	if (rc < 0)
>>> +		return rc;
>>> +
>>> +	/* Load ramdisk */
>>> +	if (entries[def].initrd) {
>>> +		loadbase += rc;
>>> +		maxloadlen -= rc;
>>> +		if (maxloadlen <= 0) {
>>> +			puts("  Not enough space for loading the initrd!");
>>> +			return -1;
>>> +		}
>>> +		strncpy(fnip->filename, entries[def].initrd,
>>> +			sizeof(fnip->filename) - 1);
>>> +		ilen = tftp_load(fnip, loadbase, maxloadlen);
>>> +		if (ilen < 0)
>>> +			return ilen;
>>> +		/* The ELF loader will move the kernel to some spot in low mem
>>> +		 * later, thus move the initrd to the end of the RAM instead */
>>> +		memmove(loadbase + maxloadlen - ilen, loadbase, ilen);
>>> +		/* Encode the initrd information in the device tree */
>>> +		SLOF_set_chosen_int("linux,initrd-start",
>>> +		                    (long)loadbase + maxloadlen - ilen);
>>> +		SLOF_set_chosen_int("linux,initrd-end",
>>> +		                    (long)loadbase + maxloadlen);
>>> +	}
>>> +
>>> +	if (entries[def].append) {
>>> +		SLOF_set_chosen_bytes("bootargs", entries[def].append,
>>> +		                      strlen(entries[def].append) + 1);
>>> +	}
>>> +
>>> +	return rc;
>>> +}
> 
>  Thomas
> 


-- 
Alexey