[PATCH] discover: Add support for IPMI network override
Sam Mendoza-Jonas
sam at mendozajonas.com
Wed Feb 24 09:49:30 AEDT 2016
On Tue, Feb 23, 2016 at 09:24:52PM +1030, Joel Stanley wrote:
> On Tue, Feb 23, 2016 at 2:08 PM, Sam Mendoza-Jonas <sam at mendozajonas.com> wrote:
> > On BMC platforms the 'Get System Boot Options' command can also be used
> > to check for a temporary network interface config override. This is
> > implemented via the optional 'OEM Parameters' field defined in the IPMI
> > v2 spec. We define the actual format of the field as:
> > - 4 byte cookie value
> > - 2 byte version value
> > - 1 byte hardware address size
> > - 1 byte IP address size
> > - Hardware address
> > - 1 byte flags for 'ignore' and 'method'
> > And for static configs:
> > - IP Address
> > - 1 byte subnet value
> > - Gateway address
> >
> > If set the config override replaces any other interface config, forcing
> > the use of the specified configuration.
> >
> > Signed-off-by: Sam Mendoza-Jonas <sam at mendozajonas.com>
>
> Looks good. A few comments below.
>
> A test would be great :)
Agreed, I'll try and get one out today.
>
> > ---
> > discover/platform-powerpc.c | 173 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 173 insertions(+)
> >
> > diff --git a/discover/platform-powerpc.c b/discover/platform-powerpc.c
> > index 78b76d3..d8b4897 100644
> > --- a/discover/platform-powerpc.c
> > +++ b/discover/platform-powerpc.c
> > @@ -8,6 +8,8 @@
> > #include <sys/wait.h>
> > #include <sys/fcntl.h>
> > #include <sys/stat.h>
> > +#include <arpa/inet.h>
> > +#include <asm/byteorder.h>
> >
> > #include <file/file.h>
> > #include <talloc/talloc.h>
> > @@ -1113,6 +1115,174 @@ static void get_ipmi_bmc_versions(struct platform *p, struct system_info *info)
> > pb_log("Failed to retrieve Golden Device ID from IPMI\n");
> > }
> >
> > +static void get_ipmi_network_override(struct platform_powerpc *platform,
> > + struct config *config)
> > +{
> > + const uint32_t magic_value = 0x21706221;
> > + uint16_t min_len = 12, resp_len = 53, version;
> > + uint8_t hwsize, ipsize, resp[resp_len];
> > + struct interface_config *ifconf;
> > + uint8_t req[] = {
>
> const? I wonder if you could make ipmi_transaction take a const
> buffer. But the userspace IPMI API would require you to cast it away
> :(
Yeah eventually it gets thrown away but we could probably make it nicer
up until then - I'll look at it in a separate patch since the existing
users will need updating too.
>
> > + 0x61, /* parameter selector: OEM section (network) */
> > + 0x00, /* no set selector */
> > + 0x00, /* no block selector */
> > + };
> > + char *ipstr, *gatewaystr;
> > + int addr_type, i, rc;
> > + socklen_t addr_len;
> > + uint32_t cookie;
> > +
> > + rc = ipmi_transaction(platform->ipmi, IPMI_NETFN_CHASSIS,
> > + IPMI_CMD_CHASSIS_GET_SYSTEM_BOOT_OPTIONS,
> > + req, sizeof(req),
> > + resp, &resp_len,
> > + ipmi_timeout);
> > +
> > + pb_debug("IPMI net override resp [%d][%d]:\n", rc, resp_len);
> > + if (resp_len > 0) {
> > + for (i = 0; i < resp_len; i++) {
> > + pb_debug(" %02x", resp[i]);
> > + if (i && (i + 1) % 16 == 0 && i != resp_len - 1)
> > + pb_debug("\n");
> > + else if (i && (i + 1) % 8 == 0)
> > + pb_debug(" ");
> > + }
> > + pb_debug("\n");
> > + }
> > +
> > + if (rc) {
> > + pb_log("IPMI network config option unavailable\n");
> > + return;
> > + }
> > +
> > + if (resp_len < min_len) {
> > + pb_log("IPMI net response too small\n");
> > + return;
> > + }
> > +
> > + if (resp[0] != 0) {
> > + pb_log("platform: non-zero completion code %d from IPMI "
> > + "network req\n", resp[0]);
>
> I like it when the entire string is on the same line, so it's easier
> to grep for.
>
I was on the fence about it but that pushes me over.
> > + return;
> > + }
> > +
> > + /* Check for correct parameter version */
> > + if ((resp[1] & 0xf) != 0x1) {
> > + pb_log("platform: unexpected version (0x%x) in "
> > + "network override response\n", resp[0]);
>
> Again.
>
Same.
> > + return;
> > + }
> > +
> > + /* Check for valid parameters. For now ignore the persistent flag */
> > + if (resp[2] & 0x80) {
> > + pb_debug("platform: network override is invalid/locked\n");
> > + return;
> > + }
> > +
> > + /* Check for valid parameters in the boot flags section, ignoring the
> > + * persistent bit */
> > + if (!(resp[3] & 0x80)) {
> > + pb_debug("platform: network override valid flag not set\n");
> > + return;
> > + }
> > +
> > + /* Check 4-byte cookie value */
> > + i = 4;
> > + memcpy(&cookie, &resp[i], sizeof(cookie));
> > + cookie = __be32_to_cpu(cookie);
> > + if (cookie != magic_value) {
> > + pb_log("%s: Incorrect cookie %x\n", __func__, cookie);
> > + return;
> > + }
> > + i += sizeof(cookie);
> > +
> > + /* Check 2-byte version number */
> > + memcpy(&version, &resp[i], sizeof(version));
> > + version = __be16_to_cpu(version);
> > + i += sizeof(version);
> > + if (version != 1)
> > + pb_debug("Unexpected version: %u\n", version);
> > +
> > + /* Get 1-byte hardware address size and ip address size */
> > + memcpy(&hwsize, &resp[i], sizeof(hwsize));
> > + i += sizeof(hwsize);
> > + memcpy(&ipsize, &resp[i], sizeof(ipsize));
> > + i += sizeof(ipsize);
> > +
> > + if (!hwsize || !ipsize) {
> > + pb_log("%s: Empty response\n", __func__);
> > + return;
> > + }
> > +
> > + /* At the moment only support 6-byte MAC addresses */
> > + if (hwsize != sizeof(ifconf->hwaddr)) {
> > + pb_log("Unsupported HW address size in network override: %u\n",
> > + hwsize);
> > + return;
> > + }
> > +
> > + /* Sanity check the IP address size */
> > + if (ipsize == 4) {
> > + addr_type = AF_INET;
> > + addr_len = INET_ADDRSTRLEN;
> > + } else if (ipsize == 16) {
> > + addr_type = AF_INET6;
> > + addr_len = INET6_ADDRSTRLEN;
> > + } else {
> > + pb_log("Unsupported IP address size: %u\n", ipsize);
> > + return;
> > + }
> > +
> > + /* Everything past here is the interface config */
>
> This is a huge function. Perhaps this is a good spot to pass the data
> off to a helper?
>
Yeah it goes on a bit. I can split it out into another function - more
generally I want to have a look at splitting out some of the rest of
platform-powerpc and organising the FSP/BMC split a little better,
there's a lot of new logic being added recently.
> > + ifconf = talloc_zero(config, struct interface_config);
> > + if (!ifconf) {
> > + pb_log("Failed to allocate network override\n");
> > + return;
> > + }
> > +
> > + /* Hardware Address */
> > + memcpy(ifconf->hwaddr, &resp[i], hwsize);
>
> Do we need to fix the endian here?
>
I don't think so since it's just an array of uint8_t.
> > + i += hwsize;
> > +
> > + /* Check 1-byte ignore and method flags */
> > + ifconf->ignore = !!resp[i++];
> > + ifconf->method = !!resp[i++];
> > +
> > + if (ifconf->method == CONFIG_METHOD_STATIC) {
> > + /* IP address */
> > + ipstr = talloc_array(ifconf, char, addr_len);
> > + if (!inet_ntop(addr_type, &resp[i], ipstr, addr_len)) {
> > + pb_log("Failed to convert ipaddr: %m\n");
> > + talloc_free(ifconf);
> > + return;
> > + }
> > + i += ipsize;
> > +
> > + /* IP address subnet */
> > + ifconf->static_config.address = talloc_asprintf(ifconf,
> > + "%s/%u", ipstr, resp[i]);
> > + i++;
> > +
> > + /* Gateway address */
> > + gatewaystr = talloc_array(ifconf, char, addr_len);
> > + if (!inet_ntop(addr_type, &resp[i], gatewaystr, addr_len)) {
> > + pb_log("Failed to convert gateway: %m\n");
> > + talloc_free(ifconf);
> > + return;
> > + }
> > + ifconf->static_config.gateway = gatewaystr;
> > + i += ipsize;
> > + }
> > +
> > + pb_log("Applying IPMI network config\n");
> > +
> > + /* Replace any existing interface config */
> > + talloc_free(config->network.interfaces);
> > + config->network.n_interfaces = 1;
> > + config->network.interfaces = talloc(config, struct interface_config *);
> > + config->network.interfaces[0] = ifconf;
> > +}
> > +
> > static int load_config(struct platform *p, struct config *config)
> > {
> > struct platform_powerpc *platform = to_platform_powerpc(p);
> > @@ -1134,6 +1304,9 @@ static int load_config(struct platform *p, struct config *config)
> > }
> > }
> >
> > + if (platform->ipmi)
> > + get_ipmi_network_override(platform, config);
> > +
> > return 0;
> > }
> >
> > --
> > 2.7.1
> >
> > _______________________________________________
> > Petitboot mailing list
> > Petitboot at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/petitboot
More information about the Petitboot
mailing list