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

Adriano Córdova
2024-11-18T12:36:15Z

Hi Ilias,

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

> Hi Adriano,
>
> On Mon, 11 Nov 2024 at 23:10, Adriano Cordova <adrianox@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@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