| Message ID | 1526658340-1992-8-git-send-email-thuth@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support network booting with pxelinux.cfg files | expand |
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@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? > +{ > + static char cfgbuf[CFG_BUF_SIZE]; This belongs to pxelinux_load_parse_cfg(). And why static? > + 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; > +} > + > static void encode_response(char *pkt_buffer, size_t size, int ip_init) > { > switch(ip_init) { > @@ -689,6 +744,10 @@ int netload(char *buffer, int len, char *args_fs, int alen) > /* Do the TFTP load and print error message if necessary */ > rc = tftp_load(&fn_ip, buffer, len); > > + if (rc <= 0 && !obp_tftp_args.filename[0]) { > + rc = net_pxelinux_cfg_load(&fn_ip, buffer, len, own_mac); > + } > + > if (obp_tftp_args.ip_init == IP_INIT_DHCP) > dhcp_send_release(fn_ip.fd); > > diff --git a/slof/helpers.c b/slof/helpers.c > index a8d575c..bd0742e 100644 > --- a/slof/helpers.c > +++ b/slof/helpers.c > @@ -181,7 +181,16 @@ int write_mm_log(char *data, unsigned int len, unsigned short type) > return forth_eval_pop("write-mm-log"); > } > > -static void SLOF_encode_response(void *addr, size_t size,char *s) > +void SLOF_set_chosen_int(const char *s, long val) > +{ > + forth_push(val); > + forth_eval("encode-int"); > + forth_push((unsigned long)s); > + forth_push(strlen(s)); > + forth_eval("set-chosen"); > +} > + > +void SLOF_set_chosen_bytes(const char *s, const char *addr, size_t size) > { > forth_push((unsigned long)addr); > forth_push(size); > @@ -193,10 +202,10 @@ static void SLOF_encode_response(void *addr, size_t size,char *s) > > void SLOF_encode_bootp_response(void *addr, size_t size) > { > - SLOF_encode_response(addr, size, "bootp-response"); > + SLOF_set_chosen_bytes("bootp-response", addr, size); > } > > void SLOF_encode_dhcp_response(void *addr, size_t size) > { > - SLOF_encode_response(addr, size, "dhcp-response"); > + SLOF_set_chosen_bytes("dhcp-response", addr, size); > } >
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@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. > 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. >> + 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
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@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 >
On 25.05.2018 04:44, Alexey Kardashevskiy wrote: > 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@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. Please keep in mind that the s390-ccw bios is using now libnet, too. That's the reason for putting the code in pxelinux.c, while keeping the "controlling" code in netload.c. Think of pxelinux.c being the library, and netload.c the "program" that uses the library. I also plan to call pxelinux_parse_cfg() from the outside in the s390-ccw bios, so that function needs to be non-static. Thomas
On 25.05.2018 04:44, Alexey Kardashevskiy wrote: > On 25/5/18 12:39 am, Thomas Huth wrote: >> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: [...] >>> 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. Yeah, stack size is very, very sensitive, since a lot of code is calling engine() recursively, and this uses quite some bit of stack space each time. See commits 8c41240bc4e9, 271fd45605c11, baa834884c90, ... for example. So I'll try to switch to malloc here instead. Thomas PS: By the way, something for our TODO lists: We really should add some logic to the libc free() function to merge freed memory blocks again if possible...
On 25/5/18 2:42 pm, Thomas Huth wrote: > On 25.05.2018 04:44, Alexey Kardashevskiy wrote: >> On 25/5/18 12:39 am, Thomas Huth wrote: >>> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: > [...] >>>> 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. > > Yeah, stack size is very, very sensitive, since a lot of code is calling > engine() recursively, and this uses quite some bit of stack space each > time. See commits 8c41240bc4e9, 271fd45605c11, baa834884c90, ... for > example. > > So I'll try to switch to malloc here instead. May be then embed cfgbuf/cfgsize into "lkia" so it is obvious lkia's entries point into this cfgbuf? struct pxe_cfg { struct { const char *label; const char *kernel; const char *initrd; const char *append; } lkia[MAX_LKIA_ENTRIES]; unsigned entries; unsigned default; char buf[CFG_BUF_SIZE]; }; and malloc/free that? /me wonders if I am asking too much :) > > Thomas > > > PS: > By the way, something for our TODO lists: We really should add some > logic to the libc free() function to merge freed memory blocks again if > possible... Do you have any measurements of how much loose on this?
On 25.05.2018 07:27, Alexey Kardashevskiy wrote: > On 25/5/18 2:42 pm, Thomas Huth wrote: >> On 25.05.2018 04:44, Alexey Kardashevskiy wrote: >>> On 25/5/18 12:39 am, Thomas Huth wrote: >>>> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: >> [...] >>>>> 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. >> >> Yeah, stack size is very, very sensitive, since a lot of code is calling >> engine() recursively, and this uses quite some bit of stack space each >> time. See commits 8c41240bc4e9, 271fd45605c11, baa834884c90, ... for >> example. >> >> So I'll try to switch to malloc here instead. > > > May be then embed cfgbuf/cfgsize into "lkia" so it is obvious lkia's > entries point into this cfgbuf? > > struct pxe_cfg { > struct { > const char *label; > const char *kernel; > const char *initrd; > const char *append; > } lkia[MAX_LKIA_ENTRIES]; > unsigned entries; > unsigned default; > char buf[CFG_BUF_SIZE]; > }; > > and malloc/free that? No, I'd like to keep the buffer and lkia handling separate (since that'll fit the handling in the s390-ccw bios better). Just let me add a proper comment in the code that the buffer needs to remain valid for the lkia pointers, then I think it should be fine. >> PS: >> By the way, something for our TODO lists: We really should add some >> logic to the libc free() function to merge freed memory blocks again if >> possible... > > Do you have any measurements of how much loose on this? No. Since we scarcely used malloc and free in the SLOF code in the past, this also was never an issue. But if we now add more and more code that uses malloc + free, we should keep this in mind and be prepared to face out-of-memory problems one day due to the memory fragmentation here. Thomas
On Fri, 25 May 2018 07:42:53 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 25.05.2018 07:27, Alexey Kardashevskiy wrote: > > On 25/5/18 2:42 pm, Thomas Huth wrote: > >> On 25.05.2018 04:44, Alexey Kardashevskiy wrote: > >>> On 25/5/18 12:39 am, Thomas Huth wrote: > >>>> On 24.05.2018 11:01, Alexey Kardashevskiy wrote: > >> [...] > >>>>> 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. > >> > >> Yeah, stack size is very, very sensitive, since a lot of code is calling > >> engine() recursively, and this uses quite some bit of stack space each > >> time. See commits 8c41240bc4e9, 271fd45605c11, baa834884c90, ... for > >> example. > >> > >> So I'll try to switch to malloc here instead. > > > > > > May be then embed cfgbuf/cfgsize into "lkia" so it is obvious lkia's > > entries point into this cfgbuf? > > > > struct pxe_cfg { > > struct { > > const char *label; > > const char *kernel; > > const char *initrd; > > const char *append; > > } lkia[MAX_LKIA_ENTRIES]; > > unsigned entries; > > unsigned default; > > char buf[CFG_BUF_SIZE]; > > }; > > > > and malloc/free that? > > No, I'd like to keep the buffer and lkia handling separate (since > that'll fit the handling in the s390-ccw bios better). > > Just let me add a proper comment in the code that the buffer needs to > remain valid for the lkia pointers, then I think it should be fine. > > >> PS: > >> By the way, something for our TODO lists: We really should add some > >> logic to the libc free() function to merge freed memory blocks again if > >> possible... > > > > Do you have any measurements of how much loose on this? > > No. Since we scarcely used malloc and free in the SLOF code in the past, > this also was never an issue. But if we now add more and more code that > uses malloc + free, we should keep this in mind and be prepared to face > out-of-memory problems one day due to the memory fragmentation here. > It looks like some memory defragmentation logic already exists but it is only used during malloc() when the heap is full. Isn't this enough ? > Thomas
On 25.05.2018 08:10, Greg Kurz wrote: > On Fri, 25 May 2018 07:42:53 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 25.05.2018 07:27, Alexey Kardashevskiy wrote: >>> On 25/5/18 2:42 pm, Thomas Huth wrote: [...] >>>> By the way, something for our TODO lists: We really should add some >>>> logic to the libc free() function to merge freed memory blocks again if >>>> possible... >>> >>> Do you have any measurements of how much loose on this? >> >> No. Since we scarcely used malloc and free in the SLOF code in the past, >> this also was never an issue. But if we now add more and more code that >> uses malloc + free, we should keep this in mind and be prepared to face >> out-of-memory problems one day due to the memory fragmentation here. >> > > It looks like some memory defragmentation logic already exists but it is > only used during malloc() when the heap is full. Isn't this enough ? Oh, you're right. I only looked at the free() code and did not notice the clean() function in the malloc.c code. I didn't expect that... Intuitively, I'd rather implement that during free() instead. But during malloc() should be ok, too, so never mind, fragmentation is then likely not such a big issue as I originally thought. Thomas
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. Without that, my test setup would report error like this: Trying to load: from: /pci@800000020000000/ethernet@1 ... Initializing NIC Reading MAC address from device: c0:41:49:4b:00:30 Requesting information via DHCP: done Using IPv4 address: 10.61.191.48 Requesting file "image.aiktest48" via TFTP from 10.61.2.7 Receiving data: 0 KBytes E3009 (net) file not found: image.aiktest48 E3407: Load failed With this patch applied: Trying to load: from: /pci@800000020000000/ethernet@1 ... Initializing NIC Reading MAC address from device: c0:41:49:4b:00:30 Requesting information via DHCP: done Using IPv4 address: 10.61.191.48 Requesting file "image.aiktest48" via TFTP from 10.61.2.7 Receiving data: 0 KBytes E3009 (net) file not found: image.aiktest48 Trying pxelinux.cfg files... TFTP error: TFTP buffer of 2047 bytes is too small for pxelinux.cfg/default E3407: Load failed this is because.... > > Signed-off-by: Thomas Huth <thuth@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) > +{ > + static char cfgbuf[CFG_BUF_SIZE]; > + 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; > +} > + > static void encode_response(char *pkt_buffer, size_t size, int ip_init) > { > switch(ip_init) { > @@ -689,6 +744,10 @@ int netload(char *buffer, int len, char *args_fs, int alen) > /* Do the TFTP load and print error message if necessary */ > rc = tftp_load(&fn_ip, buffer, len); > > + if (rc <= 0 && !obp_tftp_args.filename[0]) { .... obp_tftp_args.filename == "" in this particular case as tftp_load() does not seem to store the filename if loading fails. > + rc = net_pxelinux_cfg_load(&fn_ip, buffer, len, own_mac); > + } > + > if (obp_tftp_args.ip_init == IP_INIT_DHCP) > dhcp_send_release(fn_ip.fd); > > diff --git a/slof/helpers.c b/slof/helpers.c > index a8d575c..bd0742e 100644 > --- a/slof/helpers.c > +++ b/slof/helpers.c > @@ -181,7 +181,16 @@ int write_mm_log(char *data, unsigned int len, unsigned short type) > return forth_eval_pop("write-mm-log"); > } > > -static void SLOF_encode_response(void *addr, size_t size,char *s) > +void SLOF_set_chosen_int(const char *s, long val) > +{ > + forth_push(val); > + forth_eval("encode-int"); > + forth_push((unsigned long)s); > + forth_push(strlen(s)); > + forth_eval("set-chosen"); > +} > + > +void SLOF_set_chosen_bytes(const char *s, const char *addr, size_t size) > { > forth_push((unsigned long)addr); > forth_push(size); > @@ -193,10 +202,10 @@ static void SLOF_encode_response(void *addr, size_t size,char *s) > > void SLOF_encode_bootp_response(void *addr, size_t size) > { > - SLOF_encode_response(addr, size, "bootp-response"); > + SLOF_set_chosen_bytes("bootp-response", addr, size); > } > > void SLOF_encode_dhcp_response(void *addr, size_t size) > { > - SLOF_encode_response(addr, size, "dhcp-response"); > + SLOF_set_chosen_bytes("dhcp-response", addr, size); > } >
On 25.05.2018 09:08, 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. > > Without that, my test setup would report error like this: > > > Trying to load: from: /pci@800000020000000/ethernet@1 ... > Initializing NIC > Reading MAC address from device: c0:41:49:4b:00:30 > Requesting information via DHCP: done > Using IPv4 address: 10.61.191.48 > Requesting file "image.aiktest48" via TFTP from 10.61.2.7 > Receiving data: 0 KBytes > E3009 (net) file not found: image.aiktest48 > > E3407: Load failed > > > > With this patch applied: > > Trying to load: from: /pci@800000020000000/ethernet@1 ... > Initializing NIC > Reading MAC address from device: c0:41:49:4b:00:30 > Requesting information via DHCP: done > Using IPv4 address: 10.61.191.48 > Requesting file "image.aiktest48" via TFTP from 10.61.2.7 > Receiving data: 0 KBytes > E3009 (net) file not found: image.aiktest48 > Trying pxelinux.cfg files... > TFTP error: TFTP buffer of 2047 bytes is too small for pxelinux.cfg/default > > E3407: Load failed > > > this is because.... [...] >> @@ -689,6 +744,10 @@ int netload(char *buffer, int len, char *args_fs, int alen) >> /* Do the TFTP load and print error message if necessary */ >> rc = tftp_load(&fn_ip, buffer, len); >> >> + if (rc <= 0 && !obp_tftp_args.filename[0]) { > > .... obp_tftp_args.filename == "" in this particular case as tftp_load() > does not seem to store the filename if loading fails. > >> + rc = net_pxelinux_cfg_load(&fn_ip, buffer, len, own_mac); >> + } Sorry, I don't quite understand what you want to say here? How big is your pxelinux.cfg/default file in this case? Was it smaller than 2047 bytes, so that the above error message is not right? Or do you rather complain that SLOF is trying to boot via pxelinux at all in case you've set a bootfile name? Thomas
On 25/5/18 5:29 pm, Thomas Huth wrote: > On 25.05.2018 09:08, 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. >> >> Without that, my test setup would report error like this: >> >> >> Trying to load: from: /pci@800000020000000/ethernet@1 ... >> Initializing NIC >> Reading MAC address from device: c0:41:49:4b:00:30 >> Requesting information via DHCP: done >> Using IPv4 address: 10.61.191.48 >> Requesting file "image.aiktest48" via TFTP from 10.61.2.7 >> Receiving data: 0 KBytes >> E3009 (net) file not found: image.aiktest48 >> >> E3407: Load failed >> >> >> >> With this patch applied: >> >> Trying to load: from: /pci@800000020000000/ethernet@1 ... >> Initializing NIC >> Reading MAC address from device: c0:41:49:4b:00:30 >> Requesting information via DHCP: done >> Using IPv4 address: 10.61.191.48 >> Requesting file "image.aiktest48" via TFTP from 10.61.2.7 >> Receiving data: 0 KBytes >> E3009 (net) file not found: image.aiktest48 >> Trying pxelinux.cfg files... >> TFTP error: TFTP buffer of 2047 bytes is too small for pxelinux.cfg/default >> >> E3407: Load failed >> >> >> this is because.... > [...] >>> @@ -689,6 +744,10 @@ int netload(char *buffer, int len, char *args_fs, int alen) >>> /* Do the TFTP load and print error message if necessary */ >>> rc = tftp_load(&fn_ip, buffer, len); >>> >>> + if (rc <= 0 && !obp_tftp_args.filename[0]) { >> >> .... obp_tftp_args.filename == "" in this particular case as tftp_load() >> does not seem to store the filename if loading fails. >> >>> + rc = net_pxelinux_cfg_load(&fn_ip, buffer, len, own_mac); >>> + } > > Sorry, I don't quite understand what you want to say here? > > How big is your pxelinux.cfg/default file in this case? Was it smaller > than 2047 bytes, so that the above error message is not right? Or do > you rather complain that SLOF is trying to boot via pxelinux at all in > case you've set a bootfile name? I thought we should stop and not try PXE if a filename is set. But now I wonder what do others do in this case? Keep trying all PXE options if they are present (these 209 and 210 DHCP options)?
On 25.05.2018 10:22, Alexey Kardashevskiy wrote: > On 25/5/18 5:29 pm, Thomas Huth wrote: >> On 25.05.2018 09:08, 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. >>> >>> Without that, my test setup would report error like this: >>> >>> >>> Trying to load: from: /pci@800000020000000/ethernet@1 ... >>> Initializing NIC >>> Reading MAC address from device: c0:41:49:4b:00:30 >>> Requesting information via DHCP: done >>> Using IPv4 address: 10.61.191.48 >>> Requesting file "image.aiktest48" via TFTP from 10.61.2.7 >>> Receiving data: 0 KBytes >>> E3009 (net) file not found: image.aiktest48 >>> >>> E3407: Load failed >>> >>> >>> >>> With this patch applied: >>> >>> Trying to load: from: /pci@800000020000000/ethernet@1 ... >>> Initializing NIC >>> Reading MAC address from device: c0:41:49:4b:00:30 >>> Requesting information via DHCP: done >>> Using IPv4 address: 10.61.191.48 >>> Requesting file "image.aiktest48" via TFTP from 10.61.2.7 >>> Receiving data: 0 KBytes >>> E3009 (net) file not found: image.aiktest48 >>> Trying pxelinux.cfg files... >>> TFTP error: TFTP buffer of 2047 bytes is too small for pxelinux.cfg/default >>> >>> E3407: Load failed >>> >>> >>> this is because.... >> [...] >>>> @@ -689,6 +744,10 @@ int netload(char *buffer, int len, char *args_fs, int alen) >>>> /* Do the TFTP load and print error message if necessary */ >>>> rc = tftp_load(&fn_ip, buffer, len); >>>> >>>> + if (rc <= 0 && !obp_tftp_args.filename[0]) { >>> >>> .... obp_tftp_args.filename == "" in this particular case as tftp_load() >>> does not seem to store the filename if loading fails. >>> >>>> + rc = net_pxelinux_cfg_load(&fn_ip, buffer, len, own_mac); >>>> + } >> >> Sorry, I don't quite understand what you want to say here? >> >> How big is your pxelinux.cfg/default file in this case? Was it smaller >> than 2047 bytes, so that the above error message is not right? Or do >> you rather complain that SLOF is trying to boot via pxelinux at all in >> case you've set a bootfile name? > > I thought we should stop and not try PXE if a filename is set. But now I > wonder what do others do in this case? Keep trying all PXE options if they > are present (these 209 and 210 DHCP options)? I think we should continue. x86 folks are used to set the pxelinux.0 binary in the DHCP options and then expect that the pxelinux loader continues loading with the appropriate base directory. Since we do not have a dedicated pxelinux.0 loader binary on ppc, we should just take the information from DHCP to get the base directory (by the way, I think I still got that slightly wrong and should insert a "pxelinux.cfg" directory in there...). See here for details: https://www.syslinux.org/wiki/index.php?title=PXELINUX#Configuration_filename Alternatively, we also might only continue if the file name ends in a slash - so we are really sure that the user wanted to configure a base directory in this case... Thomas
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) +{ + static char cfgbuf[CFG_BUF_SIZE]; + 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; +} + static void encode_response(char *pkt_buffer, size_t size, int ip_init) { switch(ip_init) { @@ -689,6 +744,10 @@ int netload(char *buffer, int len, char *args_fs, int alen) /* Do the TFTP load and print error message if necessary */ rc = tftp_load(&fn_ip, buffer, len); + if (rc <= 0 && !obp_tftp_args.filename[0]) { + rc = net_pxelinux_cfg_load(&fn_ip, buffer, len, own_mac); + } + if (obp_tftp_args.ip_init == IP_INIT_DHCP) dhcp_send_release(fn_ip.fd); diff --git a/slof/helpers.c b/slof/helpers.c index a8d575c..bd0742e 100644 --- a/slof/helpers.c +++ b/slof/helpers.c @@ -181,7 +181,16 @@ int write_mm_log(char *data, unsigned int len, unsigned short type) return forth_eval_pop("write-mm-log"); } -static void SLOF_encode_response(void *addr, size_t size,char *s) +void SLOF_set_chosen_int(const char *s, long val) +{ + forth_push(val); + forth_eval("encode-int"); + forth_push((unsigned long)s); + forth_push(strlen(s)); + forth_eval("set-chosen"); +} + +void SLOF_set_chosen_bytes(const char *s, const char *addr, size_t size) { forth_push((unsigned long)addr); forth_push(size); @@ -193,10 +202,10 @@ static void SLOF_encode_response(void *addr, size_t size,char *s) void SLOF_encode_bootp_response(void *addr, size_t size) { - SLOF_encode_response(addr, size, "bootp-response"); + SLOF_set_chosen_bytes("bootp-response", addr, size); } void SLOF_encode_dhcp_response(void *addr, size_t size) { - SLOF_encode_response(addr, size, "dhcp-response"); + SLOF_set_chosen_bytes("dhcp-response", addr, size); }
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@redhat.com> --- include/helpers.h | 2 ++ lib/libnet/netload.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ slof/helpers.c | 15 ++++++++++--- 3 files changed, 73 insertions(+), 3 deletions(-)