[PATCH v3 07/15] efi_loader: efi_net: add efi_net_set_addr, efi_net_get_addr

[Adriano Córdova]

Hi Ilias,

El lun, 18 nov 2024 a las 9:22, Ilias Apalodimas (<
ilias.apalodimas at linaro.org>) escribió:

> Hi Adriano,
>
> On Mon, 11 Nov 2024 at 23:10, Adriano Cordova <adrianox at gmail.com> wrote:
> >
> > Add the functions efi_net_set_addr and  efi_net_get_addr to set
> > and get the ip address from efi code in a network agnostic way.
> > This could also go in net_common, or be compiled conditionally
> > for each network stack.
> >
> > Signed-off-by: Adriano Cordova <adrianox at gmail.com>
> > ---
> >
> > (no changes since v2)
> >
> >  include/efi_loader.h     |  16 +++++++
> >  lib/efi_loader/efi_net.c | 100 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 116 insertions(+)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 39809eac1b..612bc42816 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -125,6 +125,22 @@ static inline void efi_set_bootdev(const char *dev,
> const char *devnr,
> >                                    size_t buffer_size) { }
> >  #endif
> >
> > +#if CONFIG_IS_ENABLED(NETDEVICES) && CONFIG_IS_ENABLED(EFI_LOADER)
> > +void efi_net_get_addr(struct efi_ipv4_address *ip,
> > +                     struct efi_ipv4_address *mask,
> > +                     struct efi_ipv4_address *gw);
> > +void efi_net_set_addr(struct efi_ipv4_address *ip,
> > +                     struct efi_ipv4_address *mask,
> > +                     struct efi_ipv4_address *gw);
> > +#else
> > +static inline void efi_net_get_addr(struct efi_ipv4_address *ip,
> > +                                    struct efi_ipv4_address *mask,
> > +                                    struct efi_ipv4_address *gw) { }
> > +static inline void efi_net_set_addr(struct efi_ipv4_address *ip,
> > +                                    struct efi_ipv4_address *mask,
> > +                                    struct efi_ipv4_address *gw) { }
> > +#endif
> > +
> >  /* Maximum number of configuration tables */
> >  #define EFI_MAX_CONFIGURATION_TABLES 16
> >
> > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> > index 7cd536705f..dd84b3f18d 100644
> > --- a/lib/efi_loader/efi_net.c
> > +++ b/lib/efi_loader/efi_net.c
> > @@ -17,6 +17,7 @@
> >
> >  #include <efi_loader.h>
> >  #include <malloc.h>
> > +#include <vsprintf.h>
> >  #include <net.h>
> >
> >  static const efi_guid_t efi_net_guid = EFI_SIMPLE_NETWORK_PROTOCOL_GUID;
> > @@ -997,3 +998,102 @@ out_of_resources:
> >         printf("ERROR: Out of memory\n");
> >         return EFI_OUT_OF_RESOURCES;
> >  }
> > +
> > +void efi_net_get_addr(struct efi_ipv4_address *ip,
> > +                     struct efi_ipv4_address *mask,
> > +                     struct efi_ipv4_address *gw)
> > +{
> > +#ifdef CONFIG_NET_LWIP
> > +       char *ipstr = "ipaddr\0\0";
> > +       char *maskstr = "netmask\0\0";
> > +       char *gwstr = "gatewayip\0\0";
>
> You generally can't define strings like that and expect to change
> them. The compiler places the string literal somewhere in memory which
> is immutable.
> If you want to keep the code as is, you need to define these as ipstr[].
>
> > +       int idx;
> > +       struct in_addr tmp;
> > +       char *env;
> > +
> > +       idx = dev_seq(eth_get_dev());
> > +
> > +       if (idx < 0 || idx > 99) {
> > +               log_err("unexpected idx %d\n", idx);
> > +               return;
> > +       }
> > +
> > +       if (idx) {
>
> Is 0 not legal here?
> And why is the behavior differnt with and without LWIP?
>

This is how the lwip code deals with the environment variables for the IP
address, see
get_udev_ipv4_info in
https://github.com/u-boot/u-boot/blob/a38390284ad4261723d3a2411ba988828e994535/net/lwip/net-lwip.c#L91
In here I am trying to be consistent with that. In legacy the network stack
just uses the variables net_ip and net_netmask.


> > +               sprintf(ipstr, "ipaddr%d", idx);
> > +               sprintf(maskstr, "netmask%d", idx);
> > +               sprintf(gwstr, "gatewayip%d", idx);
> > +       }
> > +
> > +       env = env_get(ipstr);
> > +       if (env && ip) {
> > +               tmp = string_to_ip(env);
> > +               memcpy((void *)ip, (void *)&tmp, 4);
> > +       }
> > +
> > +       env = env_get(maskstr);
> > +       if (env && mask) {
> > +               tmp = string_to_ip(env);
> > +               memcpy((void *)mask, (void *)&tmp, 4);
> > +       }
> > +       env = env_get(gwstr);
> > +       if (env && gw) {
> > +               tmp = string_to_ip(env);
> > +               memcpy((void *)gw, (void *)&tmp, 4);
> > +       }
> > +#else
> > +       if (ip)
> > +               memcpy((void *)ip, (void *)&net_ip, 4);
> > +       if (mask)
> > +               memcpy((void *)mask, (void *)&net_netmask, 4);
>
> Get rid of casts etc.
>
> > +#endif
> > +}
> > +
> > +void efi_net_set_addr(struct efi_ipv4_address *ip,
> > +                     struct efi_ipv4_address *mask,
> > +                     struct efi_ipv4_address *gw)
> > +{
> > +#ifdef CONFIG_NET_LWIP
> > +       char *ipstr = "ipaddr\0\0";
> > +       char *maskstr = "netmask\0\0";
> > +       char *gwstr = "gatewayip\0\0";
> > +       int idx;
> > +       struct in_addr *addr;
> > +       char tmp[46];
> > +
> > +       idx = dev_seq(eth_get_dev());
> > +
> > +       if (idx < 0 || idx > 99) {
> > +               log_err("unexpected idx %d\n", idx);
> > +               return;
> > +       }
> > +
> > +       if (idx) {
> > +               sprintf(ipstr, "ipaddr%d", idx);
> > +               sprintf(maskstr, "netmask%d", idx);
> > +               sprintf(gwstr, "gatewayip%d", idx);
> > +       }
> > +
> > +       if (ip) {
> > +               addr = (struct in_addr *)ip;
> > +               ip_to_string(*addr, tmp);
> > +               env_set(ipstr, tmp);
> > +       }
> > +
> > +       if (mask) {
> > +               addr = (struct in_addr *)mask;
> > +               ip_to_string(*addr, tmp);
> > +               env_set(maskstr, tmp);
> > +       }
> > +
> > +       if (gw) {
> > +               addr = (struct in_addr *)gw;
> > +               ip_to_string(*addr, tmp);
> > +               env_set(gwstr, tmp);
> > +       }
> > +#else
> > +       if (ip)
> > +               memcpy((void *)&net_ip, (void *)ip, 4);
> > +       if (mask)
> > +               memcpy((void *)&net_netmask, (void *)mask, 4);
> > +#endif
> > +}
> > --
> > 2.43.0
> >
>
> Cheers
> /Ilias
>

Thanks,
Adriano