[PATCH v3 0/3] Android bootimg support

[PATCH v3 0/3] Android bootimg support

[Shea Levy]

This patch series adds support for booting from android bootimgs. A set
of helper functions is added to read the kernel, command line, and
initrd from a bootimg partition, the initrd command is updated to
respect the android_bootimg:<disk-path> syntax for loading the initrd
from the bootimg at <disk-path>, and on i386 the linux command is
update to respect the android_bootimg:<disk-path> syntax for loading the
kernel and kernel command line from the bootimg.

Shea Levy (3):
  Add helper functions to interact with android bootimg disks.
  i386: Add support for loading from android bootimg
  Add support for loading initrd from android bootimg

 grub-core/Makefile.core.def        |   1 +
 grub-core/loader/android_bootimg.c | 253 +++++++++++++++++++++++++++++++++++++
 grub-core/loader/i386/linux.c      |  27 +++-
 grub-core/loader/linux.c           |   7 +-
 include/grub/android_bootimg.h     |  12 ++
 5 files changed, 293 insertions(+), 7 deletions(-)
 create mode 100644 grub-core/loader/android_bootimg.c
 create mode 100644 include/grub/android_bootimg.h

-- 
2.7.0

[PATCH v3 1/3] Add helper functions to interact with android bootimg disks.

[Shea Levy]

Currently, the kernel, command line, and ramdisk can be extracted.
---
 grub-core/Makefile.core.def        |   1 +
 grub-core/loader/android_bootimg.c | 253 +++++++++++++++++++++++++++++++++++++
 include/grub/android_bootimg.h     |  12 ++
 3 files changed, 266 insertions(+)
 create mode 100644 grub-core/loader/android_bootimg.c
 create mode 100644 include/grub/android_bootimg.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 0cc40bb..991adad 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1669,6 +1669,7 @@ module = {
   arm64 = loader/arm64/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
+  common = loader/android_bootimg.c;
   enable = noemu;
 };
 
diff --git a/grub-core/loader/android_bootimg.c b/grub-core/loader/android_bootimg.c
new file mode 100644
index 0000000..bbee915
--- /dev/null
+++ b/grub-core/loader/android_bootimg.c
@@ -0,0 +1,253 @@
+/* android_bootimg.c - Helpers for interacting with the android bootimg format. */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2016 Free Software Foundation, Inc.
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/file.h>
+#include <grub/misc.h>
+#include <grub/android_bootimg.h>
+
+/* from https://android.googlesource.com/platform/system/core/+/506d233e7ac8ca4efa80768153d842c296477f99/mkbootimg/bootimg.h */
+/* From here until the end of struct boot_img_hdr available under the following terms:
+ *
+ * Copyright 2007, The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#define BOOT_MAGIC	"ANDROID!"
+#define BOOT_MAGIC_SIZE	8
+#define BOOT_NAME_SIZE	16
+#define BOOT_EXTRA_ARGS_SIZE	1024
+
+struct boot_img_hdr
+{
+  grub_uint8_t magic[BOOT_MAGIC_SIZE];
+
+  grub_uint32_t kernel_size;
+  grub_uint32_t kernel_addr;
+
+  grub_uint32_t ramdisk_size;
+  grub_uint32_t ramdisk_addr;
+
+  grub_uint32_t second_size;
+  grub_uint32_t second_addr;
+
+  grub_uint32_t tags_addr;
+  grub_uint32_t page_size;
+  grub_uint32_t unused[2];
+
+  grub_uint8_t name[BOOT_NAME_SIZE];
+
+  grub_uint8_t cmdline[BOOT_ARGS_SIZE];
+
+  grub_uint32_t id[8];
+
+  grub_uint8_t extra_cmdline[BOOT_EXTRA_ARGS_SIZE];
+} GRUB_PACKED;
+
+static grub_err_t
+read_hdr (grub_disk_t disk, struct boot_img_hdr *hd)
+{
+  if (grub_disk_read (disk, 0, 0, sizeof *hd, hd))
+    goto fail;
+
+  if (grub_memcmp (hd->magic, BOOT_MAGIC, BOOT_MAGIC_SIZE))
+    goto fail;
+
+  if (hd->unused[0] || hd->unused[1])
+    goto fail;
+
+  hd->kernel_size = grub_le_to_cpu32 (hd->kernel_size);
+  hd->kernel_addr = grub_le_to_cpu32 (hd->kernel_addr);
+  hd->ramdisk_size = grub_le_to_cpu32 (hd->ramdisk_size);
+  hd->ramdisk_addr = grub_le_to_cpu32 (hd->ramdisk_addr);
+  hd->second_size = grub_le_to_cpu32 (hd->second_size);
+  hd->second_addr = grub_le_to_cpu32 (hd->second_addr);
+  hd->tags_addr = grub_le_to_cpu32 (hd->tags_addr);
+  hd->page_size = grub_le_to_cpu32 (hd->page_size);
+
+  grub_size_t i;
+  for (i = 0; i < sizeof hd->id / sizeof hd->id[0]; ++i)
+    {
+      hd->id[i] = grub_le_to_cpu32 (hd->id[i]);
+    }
+
+  return GRUB_ERR_NONE;
+
+fail:
+  return grub_error (GRUB_ERR_BAD_FS, N_("%s not an android bootimg"),
+                     disk->name);
+}
+
+static grub_ssize_t
+grub_android_bootimg_read (grub_file_t file, char *buf, grub_size_t len)
+{
+  grub_size_t len_left = file->size - file->offset;
+  len = len > len_left ? len_left : len;
+  grub_off_t *begin_offset = file->data;
+  grub_off_t actual_offset = *begin_offset + file->offset;
+  file->device->disk->read_hook = file->read_hook;
+  file->device->disk->read_hook_data = file->read_hook_data;
+  grub_errno = grub_disk_read (file->device->disk, 0, actual_offset, len, buf);
+  file->device->disk->read_hook = 0;
+
+  if (grub_errno)
+    return -1;
+  else
+    return len;
+}
+
+static grub_err_t
+grub_android_bootimg_close (grub_file_t file)
+{
+  grub_free (file->data);
+  return GRUB_ERR_NONE;
+}
+
+static struct grub_fs grub_android_bootimg_fs =
+  {
+    .name = "android_bootimg",
+    .read = grub_android_bootimg_read,
+    .close = grub_android_bootimg_close
+  };
+
+grub_err_t
+grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
+                                  char *cmdline)
+{
+  grub_err_t ret = GRUB_ERR_NONE;
+  struct grub_file *f = 0;
+  grub_disk_t disk = grub_disk_open (disk_path);
+  if (!disk)
+    goto err;
+
+  struct boot_img_hdr hd;
+  if (read_hdr (disk, &hd))
+    goto err;
+
+  f = grub_zalloc (sizeof *f);
+  if (!f)
+    goto err;
+
+  f->fs = &grub_android_bootimg_fs;
+  f->device = grub_zalloc (sizeof *(f->device));
+  if (!f->device)
+    goto err;
+  f->device->disk = disk;
+
+  f->name = grub_malloc (sizeof "kernel");
+  if (!f->name)
+    goto err;
+  grub_memcpy (f->name, "kernel", sizeof "kernel");
+  grub_off_t *begin_offset = grub_malloc (sizeof *begin_offset);
+  if (!begin_offset)
+    goto err;
+  *begin_offset = hd.page_size;
+  f->data = begin_offset;
+  f->size = hd.kernel_size;
+
+  *file = f;
+  grub_memcpy (cmdline, hd.cmdline, BOOT_ARGS_SIZE);
+
+  return ret;
+
+err:
+  ret = grub_errno;
+  if (disk)
+    grub_disk_close (disk);
+
+  if (f)
+    {
+      grub_free (f->device);
+      grub_free (f->name);
+      grub_free (f);
+    }
+
+  return ret;
+}
+
+grub_err_t
+grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file)
+{
+  grub_err_t ret = GRUB_ERR_NONE;
+  struct grub_file *f = 0;
+  grub_disk_t disk = grub_disk_open (disk_path);
+  if (!disk)
+    goto err;
+
+  struct boot_img_hdr hd;
+  if (read_hdr (disk, &hd))
+    goto err;
+
+  if (!hd.ramdisk_size)
+    {
+      grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("no ramdisk in `%s'"), disk_path);
+      goto err;
+    }
+
+  f = grub_zalloc (sizeof *f);
+  if (!f)
+    goto err;
+
+  f->fs = &grub_android_bootimg_fs;
+  f->device = grub_zalloc (sizeof *(f->device));
+  if (!f->device)
+    goto err;
+  f->device->disk = disk;
+
+  f->name = grub_malloc (sizeof "ramdisk");
+  if (!f->name)
+    goto err;
+  grub_memcpy (f->name, "ramdisk", sizeof "ramdisk");
+  grub_off_t *begin_offset = grub_malloc (sizeof *begin_offset);
+  if (!begin_offset)
+    goto err;
+  *begin_offset =
+    hd.page_size * (1 + (hd.kernel_size + hd.page_size - 1) / hd.page_size);
+  f->data = begin_offset;
+  f->size = hd.ramdisk_size;
+
+  *file = f;
+
+  return ret;
+
+err:
+  ret = grub_errno;
+  if (disk)
+    grub_disk_close (disk);
+
+  if (f)
+    {
+      grub_free (f->device);
+
+      grub_free (f->name);
+
+      grub_free (f);
+    }
+
+  return ret;
+}
diff --git a/include/grub/android_bootimg.h b/include/grub/android_bootimg.h
new file mode 100644
index 0000000..ff14df7
--- /dev/null
+++ b/include/grub/android_bootimg.h
@@ -0,0 +1,12 @@
+#include <grub/file.h>
+
+#define BOOT_ARGS_SIZE 512
+
+/* Load a kernel from a bootimg. cmdline must be at least BOOT_ARGS_SIZE */
+grub_err_t
+grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
+                                  char *cmdline);
+
+/* Load an initrd from a bootimg. */
+grub_err_t
+grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file);
-- 
2.7.0

Re: [PATCH v3 1/3] Add helper functions to interact with android bootimg disks.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 8577 bytes --]

On 08.02.2016 21:47, Shea Levy wrote:
> Currently, the kernel, command line, and ramdisk can be extracted.
> ---
>  grub-core/Makefile.core.def        |   1 +
>  grub-core/loader/android_bootimg.c | 253 +++++++++++++++++++++++++++++++++++++
>  include/grub/android_bootimg.h     |  12 ++
>  3 files changed, 266 insertions(+)
>  create mode 100644 grub-core/loader/android_bootimg.c
>  create mode 100644 include/grub/android_bootimg.h
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 0cc40bb..991adad 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1669,6 +1669,7 @@ module = {
>    arm64 = loader/arm64/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
> +  common = loader/android_bootimg.c;
>    enable = noemu;
>  };
>  
> diff --git a/grub-core/loader/android_bootimg.c b/grub-core/loader/android_bootimg.c
> new file mode 100644
> index 0000000..bbee915
> --- /dev/null
> +++ b/grub-core/loader/android_bootimg.c
> @@ -0,0 +1,253 @@
> +/* android_bootimg.c - Helpers for interacting with the android bootimg format. */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2016 Free Software Foundation, Inc.
> + *
> + *  This program is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/file.h>
> +#include <grub/misc.h>
> +#include <grub/android_bootimg.h>
> +
> +/* from https://android.googlesource.com/platform/system/core/+/506d233e7ac8ca4efa80768153d842c296477f99/mkbootimg/bootimg.h */
Parts of the file cannot be effectively under different licenses. Please
put this into separate header file.
> +/* From here until the end of struct boot_img_hdr available under the following terms:
> + *
> + * Copyright 2007, The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +#define BOOT_MAGIC	"ANDROID!"
> +#define BOOT_MAGIC_SIZE	8
> +#define BOOT_NAME_SIZE	16
> +#define BOOT_EXTRA_ARGS_SIZE	1024
> +
> +struct boot_img_hdr
> +{
> +  grub_uint8_t magic[BOOT_MAGIC_SIZE];
> +
> +  grub_uint32_t kernel_size;
> +  grub_uint32_t kernel_addr;
> +
> +  grub_uint32_t ramdisk_size;
> +  grub_uint32_t ramdisk_addr;
> +
> +  grub_uint32_t second_size;
> +  grub_uint32_t second_addr;
> +
> +  grub_uint32_t tags_addr;
> +  grub_uint32_t page_size;
> +  grub_uint32_t unused[2];
> +
> +  grub_uint8_t name[BOOT_NAME_SIZE];
> +
> +  grub_uint8_t cmdline[BOOT_ARGS_SIZE];
> +
> +  grub_uint32_t id[8];
> +
> +  grub_uint8_t extra_cmdline[BOOT_EXTRA_ARGS_SIZE];
> +} GRUB_PACKED;
> +
> +static grub_err_t
> +read_hdr (grub_disk_t disk, struct boot_img_hdr *hd)
> +{
> +  if (grub_disk_read (disk, 0, 0, sizeof *hd, hd))
> +    goto fail;
> +
> +  if (grub_memcmp (hd->magic, BOOT_MAGIC, BOOT_MAGIC_SIZE))
> +    goto fail;
> +
> +  if (hd->unused[0] || hd->unused[1])
> +    goto fail;
> +
> +  hd->kernel_size = grub_le_to_cpu32 (hd->kernel_size);
> +  hd->kernel_addr = grub_le_to_cpu32 (hd->kernel_addr);
> +  hd->ramdisk_size = grub_le_to_cpu32 (hd->ramdisk_size);
> +  hd->ramdisk_addr = grub_le_to_cpu32 (hd->ramdisk_addr);
> +  hd->second_size = grub_le_to_cpu32 (hd->second_size);
> +  hd->second_addr = grub_le_to_cpu32 (hd->second_addr);
> +  hd->tags_addr = grub_le_to_cpu32 (hd->tags_addr);
> +  hd->page_size = grub_le_to_cpu32 (hd->page_size);
> +
> +  grub_size_t i;
> +  for (i = 0; i < sizeof hd->id / sizeof hd->id[0]; ++i)
> +    {
> +      hd->id[i] = grub_le_to_cpu32 (hd->id[i]);
> +    }
Why do you need this byteswap? Why not byteswap when it's used?
Also in loaders you can avoid byteswap usually as you know that you load
the file for the same arch as you currently run. Exceptions are arm-be,
ppc-le and fat binary headers.
> +
> +  return GRUB_ERR_NONE;
> +
> +fail:
> +  return grub_error (GRUB_ERR_BAD_FS, N_("%s not an android bootimg"),
> +                     disk->name);
BAD_FS is not correct anymore. BAD_KERNEL would be more correct.
> +}
> +
> +static grub_ssize_t
> +grub_android_bootimg_read (grub_file_t file, char *buf, grub_size_t len)
> +{
> +  grub_size_t len_left = file->size - file->offset;
> +  len = len > len_left ? len_left : len;
> +  grub_off_t *begin_offset = file->data;
> +  grub_off_t actual_offset = *begin_offset + file->offset;
> +  file->device->disk->read_hook = file->read_hook;
> +  file->device->disk->read_hook_data = file->read_hook_data;
> +  grub_errno = grub_disk_read (file->device->disk, 0, actual_offset, len, buf);
> +  file->device->disk->read_hook = 0;
> +
> +  if (grub_errno)
> +    return -1;
> +  else
> +    return len;
> +}
> +
> +static grub_err_t
> +grub_android_bootimg_close (grub_file_t file)
> +{
> +  grub_free (file->data);
> +  return GRUB_ERR_NONE;
> +}
> +
> +static struct grub_fs grub_android_bootimg_fs =
> +  {
> +    .name = "android_bootimg",
> +    .read = grub_android_bootimg_read,
> +    .close = grub_android_bootimg_close
> +  };
> +
please use grub_file_offset_open rather than reimplementing it if you
need it at all. I've looked in linux.c and you'll need only to adjust
prot_file_size calculation and grub_file_seek (add a call and adjust one
call). You can easily extract grub_cmd_linux essence into load_linux
(grub_file_t file, grub_off_t off, grub_off_t size)

> +grub_err_t
> +grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
> +                                  char *cmdline)
> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  struct grub_file *f = 0;
> +  grub_disk_t disk = grub_disk_open (disk_path);
> +  if (!disk)
> +    goto err;
> +
> +  struct boot_img_hdr hd;
> +  if (read_hdr (disk, &hd))
> +    goto err;
> +
> +  f = grub_zalloc (sizeof *f);
> +  if (!f)
> +    goto err;
> +
> +  f->fs = &grub_android_bootimg_fs;
> +  f->device = grub_zalloc (sizeof *(f->device));
> +  if (!f->device)
> +    goto err;
> +  f->device->disk = disk;
> +
> +  f->name = grub_malloc (sizeof "kernel");
> +  if (!f->name)
> +    goto err;
> +  grub_memcpy (f->name, "kernel", sizeof "kernel");
> +  grub_off_t *begin_offset = grub_malloc (sizeof *begin_offset);
> +  if (!begin_offset)
> +    goto err;
> +  *begin_offset = hd.page_size;
> +  f->data = begin_offset;
> +  f->size = hd.kernel_size;
> +
> +  *file = f;
All of this will go away, so I won't comment on it.
> +grub_err_t
> +grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file)
> +{

Instead you can extract grub_cmd_initrd essence into sth like
void *prepare_initrd (grub_size_t size)
which returns a pointer to buffer where the initrd would do.
> diff --git a/include/grub/android_bootimg.h b/include/grub/android_bootimg.h
> new file mode 100644
> index 0000000..ff14df7
> --- /dev/null
> +++ b/include/grub/android_bootimg.h
> @@ -0,0 +1,12 @@
> +#include <grub/file.h>
> +
> +#define BOOT_ARGS_SIZE 512
> +
> +/* Load a kernel from a bootimg. cmdline must be at least BOOT_ARGS_SIZE */
> +grub_err_t
> +grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
> +                                  char *cmdline);
> +
> +/* Load an initrd from a bootimg. */
> +grub_err_t
> +grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH v3 1/3] Add helper functions to interact with android bootimg disks.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 9072 bytes --]

On 12.02.2016 18:42, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 08.02.2016 21:47, Shea Levy wrote:
>> Currently, the kernel, command line, and ramdisk can be extracted.
>> ---
>>  grub-core/Makefile.core.def        |   1 +
>>  grub-core/loader/android_bootimg.c | 253 +++++++++++++++++++++++++++++++++++++
>>  include/grub/android_bootimg.h     |  12 ++
>>  3 files changed, 266 insertions(+)
>>  create mode 100644 grub-core/loader/android_bootimg.c
>>  create mode 100644 include/grub/android_bootimg.h
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 0cc40bb..991adad 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1669,6 +1669,7 @@ module = {
>>    arm64 = loader/arm64/linux.c;
>>    common = loader/linux.c;
>>    common = lib/cmdline.c;
>> +  common = loader/android_bootimg.c;
>>    enable = noemu;
>>  };
>>  
>> diff --git a/grub-core/loader/android_bootimg.c b/grub-core/loader/android_bootimg.c
>> new file mode 100644
>> index 0000000..bbee915
>> --- /dev/null
>> +++ b/grub-core/loader/android_bootimg.c
>> @@ -0,0 +1,253 @@
>> +/* android_bootimg.c - Helpers for interacting with the android bootimg format. */
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2016 Free Software Foundation, Inc.
>> + *
>> + *  This program is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <grub/file.h>
>> +#include <grub/misc.h>
>> +#include <grub/android_bootimg.h>
>> +
>> +/* from https://android.googlesource.com/platform/system/core/+/506d233e7ac8ca4efa80768153d842c296477f99/mkbootimg/bootimg.h */
> Parts of the file cannot be effectively under different licenses. Please
> put this into separate header file.
>> +/* From here until the end of struct boot_img_hdr available under the following terms:
>> + *
>> + * Copyright 2007, The Android Open Source Project
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +#define BOOT_MAGIC	"ANDROID!"
>> +#define BOOT_MAGIC_SIZE	8
>> +#define BOOT_NAME_SIZE	16
>> +#define BOOT_EXTRA_ARGS_SIZE	1024
>> +
>> +struct boot_img_hdr
>> +{
>> +  grub_uint8_t magic[BOOT_MAGIC_SIZE];
>> +
>> +  grub_uint32_t kernel_size;
>> +  grub_uint32_t kernel_addr;
>> +
>> +  grub_uint32_t ramdisk_size;
>> +  grub_uint32_t ramdisk_addr;
>> +
>> +  grub_uint32_t second_size;
>> +  grub_uint32_t second_addr;
>> +
>> +  grub_uint32_t tags_addr;
>> +  grub_uint32_t page_size;
>> +  grub_uint32_t unused[2];
>> +
>> +  grub_uint8_t name[BOOT_NAME_SIZE];
>> +
>> +  grub_uint8_t cmdline[BOOT_ARGS_SIZE];
>> +
>> +  grub_uint32_t id[8];
>> +
>> +  grub_uint8_t extra_cmdline[BOOT_EXTRA_ARGS_SIZE];
>> +} GRUB_PACKED;
>> +
>> +static grub_err_t
>> +read_hdr (grub_disk_t disk, struct boot_img_hdr *hd)
>> +{
>> +  if (grub_disk_read (disk, 0, 0, sizeof *hd, hd))
>> +    goto fail;
>> +
>> +  if (grub_memcmp (hd->magic, BOOT_MAGIC, BOOT_MAGIC_SIZE))
>> +    goto fail;
>> +
>> +  if (hd->unused[0] || hd->unused[1])
>> +    goto fail;
>> +
>> +  hd->kernel_size = grub_le_to_cpu32 (hd->kernel_size);
>> +  hd->kernel_addr = grub_le_to_cpu32 (hd->kernel_addr);
>> +  hd->ramdisk_size = grub_le_to_cpu32 (hd->ramdisk_size);
>> +  hd->ramdisk_addr = grub_le_to_cpu32 (hd->ramdisk_addr);
>> +  hd->second_size = grub_le_to_cpu32 (hd->second_size);
>> +  hd->second_addr = grub_le_to_cpu32 (hd->second_addr);
>> +  hd->tags_addr = grub_le_to_cpu32 (hd->tags_addr);
>> +  hd->page_size = grub_le_to_cpu32 (hd->page_size);
>> +
>> +  grub_size_t i;
>> +  for (i = 0; i < sizeof hd->id / sizeof hd->id[0]; ++i)
>> +    {
>> +      hd->id[i] = grub_le_to_cpu32 (hd->id[i]);
>> +    }
> Why do you need this byteswap? Why not byteswap when it's used?
> Also in loaders you can avoid byteswap usually as you know that you load
> the file for the same arch as you currently run. Exceptions are arm-be,
> ppc-le and fat binary headers.
>> +
>> +  return GRUB_ERR_NONE;
>> +
>> +fail:
>> +  return grub_error (GRUB_ERR_BAD_FS, N_("%s not an android bootimg"),
>> +                     disk->name);
> BAD_FS is not correct anymore. BAD_KERNEL would be more correct.
>> +}
>> +
>> +static grub_ssize_t
>> +grub_android_bootimg_read (grub_file_t file, char *buf, grub_size_t len)
>> +{
>> +  grub_size_t len_left = file->size - file->offset;
>> +  len = len > len_left ? len_left : len;
>> +  grub_off_t *begin_offset = file->data;
>> +  grub_off_t actual_offset = *begin_offset + file->offset;
>> +  file->device->disk->read_hook = file->read_hook;
>> +  file->device->disk->read_hook_data = file->read_hook_data;
>> +  grub_errno = grub_disk_read (file->device->disk, 0, actual_offset, len, buf);
>> +  file->device->disk->read_hook = 0;
>> +
>> +  if (grub_errno)
>> +    return -1;
>> +  else
>> +    return len;
>> +}
>> +
>> +static grub_err_t
>> +grub_android_bootimg_close (grub_file_t file)
>> +{
>> +  grub_free (file->data);
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +static struct grub_fs grub_android_bootimg_fs =
>> +  {
>> +    .name = "android_bootimg",
>> +    .read = grub_android_bootimg_read,
>> +    .close = grub_android_bootimg_close
>> +  };
>> +
> please use grub_file_offset_open rather than reimplementing it if you
> need it at all. I've looked in linux.c and you'll need only to adjust
> prot_file_size calculation and grub_file_seek (add a call and adjust one
> call). You can easily extract grub_cmd_linux essence into load_linux
> (grub_file_t file, grub_off_t off, grub_off_t size)
> 
To clarify: I'm not against using grub_file_offset_open if it makes code
simpler. Just choose between those 2 alternatives whichever results in
simpler code.
>> +grub_err_t
>> +grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
>> +                                  char *cmdline)
>> +{
>> +  grub_err_t ret = GRUB_ERR_NONE;
>> +  struct grub_file *f = 0;
>> +  grub_disk_t disk = grub_disk_open (disk_path);
>> +  if (!disk)
>> +    goto err;
>> +
>> +  struct boot_img_hdr hd;
>> +  if (read_hdr (disk, &hd))
>> +    goto err;
>> +
>> +  f = grub_zalloc (sizeof *f);
>> +  if (!f)
>> +    goto err;
>> +
>> +  f->fs = &grub_android_bootimg_fs;
>> +  f->device = grub_zalloc (sizeof *(f->device));
>> +  if (!f->device)
>> +    goto err;
>> +  f->device->disk = disk;
>> +
>> +  f->name = grub_malloc (sizeof "kernel");
>> +  if (!f->name)
>> +    goto err;
>> +  grub_memcpy (f->name, "kernel", sizeof "kernel");
>> +  grub_off_t *begin_offset = grub_malloc (sizeof *begin_offset);
>> +  if (!begin_offset)
>> +    goto err;
>> +  *begin_offset = hd.page_size;
>> +  f->data = begin_offset;
>> +  f->size = hd.kernel_size;
>> +
>> +  *file = f;
> All of this will go away, so I won't comment on it.
>> +grub_err_t
>> +grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file)
>> +{
> 
> Instead you can extract grub_cmd_initrd essence into sth like
> void *prepare_initrd (grub_size_t size)
> which returns a pointer to buffer where the initrd would do.
>> diff --git a/include/grub/android_bootimg.h b/include/grub/android_bootimg.h
>> new file mode 100644
>> index 0000000..ff14df7
>> --- /dev/null
>> +++ b/include/grub/android_bootimg.h
>> @@ -0,0 +1,12 @@
>> +#include <grub/file.h>
>> +
>> +#define BOOT_ARGS_SIZE 512
>> +
>> +/* Load a kernel from a bootimg. cmdline must be at least BOOT_ARGS_SIZE */
>> +grub_err_t
>> +grub_android_bootimg_load_kernel (const char *disk_path, grub_file_t *file,
>> +                                  char *cmdline);
>> +
>> +/* Load an initrd from a bootimg. */
>> +grub_err_t
>> +grub_android_bootimg_load_initrd (const char *disk_path, grub_file_t *file);
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

[PATCH v3 2/3] i386: Add support for loading from android bootimg

[Shea Levy]

---
 grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index fddcc46..6ab8d3c 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -35,6 +35,7 @@
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
 #include <grub/linux.h>
+#include <grub/android_bootimg.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       goto fail;
     }
 
-  file = grub_file_open (argv[0]);
+  char android_cmdline[BOOT_ARGS_SIZE];
+  android_cmdline[0] = '\0';
+  if (grub_memcmp (argv[0], "android_bootimg:", sizeof "android_bootimg:" - 1) == 0)
+    grub_android_bootimg_load_kernel (argv[0] + sizeof "android_bootimg:" - 1,
+                                      &file, android_cmdline);
+  else
+      file = grub_file_open (argv[0]);
   if (! file)
     goto fail;
 
@@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
   if (!linux_cmdline)
     goto fail;
-  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
+  grub_size_t cmdline_offset = 0;
+  if (android_cmdline[0])
+    {
+      cmdline_offset = grub_strlen (android_cmdline) + 1;
+      grub_memcpy (linux_cmdline, android_cmdline, cmdline_offset - 1);
+      linux_cmdline[cmdline_offset - 1] = ' ';
+    }
+  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, sizeof (LINUX_IMAGE));
+  cmdline_offset += sizeof LINUX_IMAGE - 1;
   grub_create_loader_cmdline (argc, argv,
-			      linux_cmdline
-			      + sizeof (LINUX_IMAGE) - 1,
-			      maximal_cmdline_size
-			      - (sizeof (LINUX_IMAGE) - 1));
+                              linux_cmdline
+                              + cmdline_offset,
+                              maximal_cmdline_size
+                              - cmdline_offset);
 
   len = prot_file_size;
   if (grub_file_read (file, prot_mode_mem, len) != len && !grub_errno)
-- 
2.7.0

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2730 bytes --]

On 08.02.2016 21:47, Shea Levy wrote:
> ---
>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index fddcc46..6ab8d3c 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -35,6 +35,7 @@
>  #include <grub/i18n.h>
>  #include <grub/lib/cmdline.h>
>  #include <grub/linux.h>
> +#include <grub/android_bootimg.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>        goto fail;
>      }
>  
> -  file = grub_file_open (argv[0]);
> +  char android_cmdline[BOOT_ARGS_SIZE];
> +  android_cmdline[0] = '\0';
> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof "android_bootimg:" - 1) == 0)
> +    grub_android_bootimg_load_kernel (argv[0] + sizeof "android_bootimg:" - 1,
> +                                      &file, android_cmdline);
> +  else
> +      file = grub_file_open (argv[0]);
I hoped more for autodetection. This gets a bit hairy and proper
separation is better. Sorry for confusion. I think it's simpler with
commands like
android_bootimg [--no-cmdline] [--no-initrd] IMAGE [EXTRA_ARGUMENTS]
by default it will load both IMAGE, with cmdline and initrd. With
--no-initrd you can use initrd for custom initrd.
>    if (! file)
>      goto fail;
>  
> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>    if (!linux_cmdline)
>      goto fail;
> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
> +  grub_size_t cmdline_offset = 0;
> +  if (android_cmdline[0])
> +    {
> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
> +      grub_memcpy (linux_cmdline, android_cmdline, cmdline_offset - 1);
> +      linux_cmdline[cmdline_offset - 1] = ' ';
> +    }
> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, sizeof (LINUX_IMAGE));
> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
LINUX_IMAGE must be at the beginning. don't forget brackets around sizeof.
>    grub_create_loader_cmdline (argc, argv,
> -			      linux_cmdline
> -			      + sizeof (LINUX_IMAGE) - 1,
> -			      maximal_cmdline_size
> -			      - (sizeof (LINUX_IMAGE) - 1));
> +                              linux_cmdline
> +                              + cmdline_offset,
> +                              maximal_cmdline_size
> +                              - cmdline_offset);
>  
>    len = prot_file_size;
>    if (grub_file_read (file, prot_mode_mem, len) != len && !grub_errno)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Shea Levy]

On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 08.02.2016 21:47, Shea Levy wrote:
>> ---
>>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/grub-core/loader/i386/linux.c 
>> b/grub-core/loader/i386/linux.c
>> index fddcc46..6ab8d3c 100644
>> --- a/grub-core/loader/i386/linux.c
>> +++ b/grub-core/loader/i386/linux.c
>> @@ -35,6 +35,7 @@
>>  #include <grub/i18n.h>
>>  #include <grub/lib/cmdline.h>
>>  #include <grub/linux.h>
>> +#include <grub/android_bootimg.h>
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd 
>> __attribute__ ((unused)),
>>        goto fail;
>>      }
>>
>> -  file = grub_file_open (argv[0]);
>> +  char android_cmdline[BOOT_ARGS_SIZE];
>> +  android_cmdline[0] = '\0';
>> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof 
>> "android_bootimg:" - 1) == 0)
>> +    grub_android_bootimg_load_kernel (argv[0] + sizeof 
>> "android_bootimg:" - 1,
>> +                                      &file, android_cmdline);
>> +  else
>> +      file = grub_file_open (argv[0]);
> I hoped more for autodetection. This gets a bit hairy and proper
> separation is better. Sorry for confusion. I think it's simpler with
> commands like
> android_bootimg [--no-cmdline] [--no-initrd] IMAGE [EXTRA_ARGUMENTS]
> by default it will load both IMAGE, with cmdline and initrd. With
> --no-initrd you can use initrd for custom initrd.

Autodetection would be possible actually, I didn't think of that. If 
grub_file_open fails, we can try grub_android_bootimg_load_kernel on the 
same file. Would that be preferable or do we still want a separate 
command?

>
>>    if (! file)
>>      goto fail;
>>
>> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd 
>> __attribute__ ((unused)),
>>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>>    if (!linux_cmdline)
>>      goto fail;
>> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
>> +  grub_size_t cmdline_offset = 0;
>> +  if (android_cmdline[0])
>> +    {
>> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
>> +      grub_memcpy (linux_cmdline, android_cmdline, cmdline_offset - 
>> 1);
>> +      linux_cmdline[cmdline_offset - 1] = ' ';
>> +    }
>> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, sizeof 
>> (LINUX_IMAGE));
>> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
> LINUX_IMAGE must be at the beginning. don't forget brackets around 
> sizeof.
>>    grub_create_loader_cmdline (argc, argv,
>> -			      linux_cmdline
>> -			      + sizeof (LINUX_IMAGE) - 1,
>> -			      maximal_cmdline_size
>> -			      - (sizeof (LINUX_IMAGE) - 1));
>> +                              linux_cmdline
>> +                              + cmdline_offset,
>> +                              maximal_cmdline_size
>> +                              - cmdline_offset);
>>
>>    len = prot_file_size;
>>    if (grub_file_read (file, prot_mode_mem, len) != len && 
>> !grub_errno)
>>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3972 bytes --]

Separate command is better as it keeps interface tidy and unpoluted,
decreasing maintenance cost. Correct me if I'm wrong but it should be clear
from context of file is Android image or usual linux one?

Le ven. 12 févr. 2016 20:19, Shea Levy <shea@shealevy.com> a écrit :

> On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > On 08.02.2016 21:47, Shea Levy wrote:
> >> ---
> >>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
> >>  1 file changed, 21 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/grub-core/loader/i386/linux.c
> >> b/grub-core/loader/i386/linux.c
> >> index fddcc46..6ab8d3c 100644
> >> --- a/grub-core/loader/i386/linux.c
> >> +++ b/grub-core/loader/i386/linux.c
> >> @@ -35,6 +35,7 @@
> >>  #include <grub/i18n.h>
> >>  #include <grub/lib/cmdline.h>
> >>  #include <grub/linux.h>
> >> +#include <grub/android_bootimg.h>
> >>
> >>  GRUB_MOD_LICENSE ("GPLv3+");
> >>
> >> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd
> >> __attribute__ ((unused)),
> >>        goto fail;
> >>      }
> >>
> >> -  file = grub_file_open (argv[0]);
> >> +  char android_cmdline[BOOT_ARGS_SIZE];
> >> +  android_cmdline[0] = '\0';
> >> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof
> >> "android_bootimg:" - 1) == 0)
> >> +    grub_android_bootimg_load_kernel (argv[0] + sizeof
> >> "android_bootimg:" - 1,
> >> +                                      &file, android_cmdline);
> >> +  else
> >> +      file = grub_file_open (argv[0]);
> > I hoped more for autodetection. This gets a bit hairy and proper
> > separation is better. Sorry for confusion. I think it's simpler with
> > commands like
> > android_bootimg [--no-cmdline] [--no-initrd] IMAGE [EXTRA_ARGUMENTS]
> > by default it will load both IMAGE, with cmdline and initrd. With
> > --no-initrd you can use initrd for custom initrd.
>
> Autodetection would be possible actually, I didn't think of that. If
> grub_file_open fails, we can try grub_android_bootimg_load_kernel on the
> same file. Would that be preferable or do we still want a separate
> command?
>
> >
> >>    if (! file)
> >>      goto fail;
> >>
> >> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd
> >> __attribute__ ((unused)),
> >>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
> >>    if (!linux_cmdline)
> >>      goto fail;
> >> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
> >> +  grub_size_t cmdline_offset = 0;
> >> +  if (android_cmdline[0])
> >> +    {
> >> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
> >> +      grub_memcpy (linux_cmdline, android_cmdline, cmdline_offset -
> >> 1);
> >> +      linux_cmdline[cmdline_offset - 1] = ' ';
> >> +    }
> >> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, sizeof
> >> (LINUX_IMAGE));
> >> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
> > LINUX_IMAGE must be at the beginning. don't forget brackets around
> > sizeof.
> >>    grub_create_loader_cmdline (argc, argv,
> >> -                          linux_cmdline
> >> -                          + sizeof (LINUX_IMAGE) - 1,
> >> -                          maximal_cmdline_size
> >> -                          - (sizeof (LINUX_IMAGE) - 1));
> >> +                              linux_cmdline
> >> +                              + cmdline_offset,
> >> +                              maximal_cmdline_size
> >> +                              - cmdline_offset);
> >>
> >>    len = prot_file_size;
> >>    if (grub_file_read (file, prot_mode_mem, len) != len &&
> >> !grub_errno)
> >>
> >
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 5443 bytes --]

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Shea Levy]

OK. Do you have any thoughts on how best to extract the "load the 
kernel and set the command line to a specific string" functionality out 
of the linux command, then?

On 2016-02-12 14:22, Vladimir 'phcoder' Serbinenko wrote:
> Separate command is better as it keeps interface tidy and unpoluted,
> decreasing maintenance cost. Correct me if I'm wrong but it should be
> clear from context of file is Android image or usual linux one?
>
> Le ven. 12 févr. 2016 20:19, Shea Levy <shea@shealevy.com> a écrit :
>
>> On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> > On 08.02.2016 21:47, Shea Levy wrote:
>> >> ---
>> >>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
>> >>  1 file changed, 21 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/grub-core/loader/i386/linux.c
>> >> b/grub-core/loader/i386/linux.c
>> >> index fddcc46..6ab8d3c 100644
>> >> --- a/grub-core/loader/i386/linux.c
>> >> +++ b/grub-core/loader/i386/linux.c
>> >> @@ -35,6 +35,7 @@
>> >>  #include <grub/i18n.h>
>> >>  #include <grub/lib/cmdline.h>
>> >>  #include <grub/linux.h>
>> >> +#include <grub/android_bootimg.h>
>> >>
>> >>  GRUB_MOD_LICENSE ("GPLv3+");
>> >>
>> >> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd
>> >> __attribute__ ((unused)),
>> >>        goto fail;
>> >>      }
>> >>
>> >> -  file = grub_file_open (argv[0]);
>> >> +  char android_cmdline[BOOT_ARGS_SIZE];
>> >> +  android_cmdline[0] = '';
>> >> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof
>> >> "android_bootimg:" - 1) == 0)
>> >> +    grub_android_bootimg_load_kernel (argv[0] + sizeof
>> >> "android_bootimg:" - 1,
>> >> +                                      &file, android_cmdline);
>> >> +  else
>> >> +      file = grub_file_open (argv[0]);
>> > I hoped more for autodetection. This gets a bit hairy and proper
>> > separation is better. Sorry for confusion. I think it's simpler 
>> with
>> > commands like
>> > android_bootimg [--no-cmdline] [--no-initrd] IMAGE 
>> [EXTRA_ARGUMENTS]
>> > by default it will load both IMAGE, with cmdline and initrd. With
>> > --no-initrd you can use initrd for custom initrd.
>>
>> Autodetection would be possible actually, I didn't think of that. If
>> grub_file_open fails, we can try grub_android_bootimg_load_kernel on 
>> the
>> same file. Would that be preferable or do we still want a separate
>> command?
>>
>> >
>> >>    if (! file)
>> >>      goto fail;
>> >>
>> >> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd
>> >> __attribute__ ((unused)),
>> >>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>> >>    if (!linux_cmdline)
>> >>      goto fail;
>> >> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof 
>> (LINUX_IMAGE));
>> >> +  grub_size_t cmdline_offset = 0;
>> >> +  if (android_cmdline[0])
>> >> +    {
>> >> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
>> >> +      grub_memcpy (linux_cmdline, android_cmdline, 
>> cmdline_offset -
>> >> 1);
>> >> +      linux_cmdline[cmdline_offset - 1] = ' ';
>> >> +    }
>> >> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, 
>> sizeof
>> >> (LINUX_IMAGE));
>> >> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
>> > LINUX_IMAGE must be at the beginning. don't forget brackets around
>> > sizeof.
>> >>    grub_create_loader_cmdline (argc, argv,
>> >> -                          linux_cmdline
>> >> -                          + sizeof (LINUX_IMAGE) - 1,
>> >> -                          maximal_cmdline_size
>> >> -                          - (sizeof (LINUX_IMAGE) - 1));
>> >> +                              linux_cmdline
>> >> +                              + cmdline_offset,
>> >> +                              maximal_cmdline_size
>> >> +                              - cmdline_offset);
>> >>
>> >>    len = prot_file_size;
>> >>    if (grub_file_read (file, prot_mode_mem, len) != len &&
>> >> !grub_errno)
>> >>
>> >
>> >
>> >
>> > _______________________________________________
>> > Grub-devel mailing list
>> > Grub-devel@gnu.org
>> > https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>
>
> Links:
> ------
> [1] https://lists.gnu.org/mailman/listinfo/grub-devel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 5034 bytes --]

On 12.02.2016 20:34, Shea Levy wrote:
> OK. Do you have any thoughts on how best to extract the "load the kernel
> and set the command line to a specific string" functionality out of the
> linux command, then?
> 
At first I wanted to write a short skeleton patch to show what I meant
but once skeleton is done, there is little actual code involved. Please
try attached patch
> On 2016-02-12 14:22, Vladimir 'phcoder' Serbinenko wrote:
>> Separate command is better as it keeps interface tidy and unpoluted,
>> decreasing maintenance cost. Correct me if I'm wrong but it should be
>> clear from context of file is Android image or usual linux one?
>>
>> Le ven. 12 févr. 2016 20:19, Shea Levy <shea@shealevy.com> a écrit :
>>
>>> On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>> > On 08.02.2016 21:47, Shea Levy wrote:
>>> >> ---
>>> >>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
>>> >>  1 file changed, 21 insertions(+), 6 deletions(-)
>>> >>
>>> >> diff --git a/grub-core/loader/i386/linux.c
>>> >> b/grub-core/loader/i386/linux.c
>>> >> index fddcc46..6ab8d3c 100644
>>> >> --- a/grub-core/loader/i386/linux.c
>>> >> +++ b/grub-core/loader/i386/linux.c
>>> >> @@ -35,6 +35,7 @@
>>> >>  #include <grub/i18n.h>
>>> >>  #include <grub/lib/cmdline.h>
>>> >>  #include <grub/linux.h>
>>> >> +#include <grub/android_bootimg.h>
>>> >>
>>> >>  GRUB_MOD_LICENSE ("GPLv3+");
>>> >>
>>> >> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd
>>> >> __attribute__ ((unused)),
>>> >>        goto fail;
>>> >>      }
>>> >>
>>> >> -  file = grub_file_open (argv[0]);
>>> >> +  char android_cmdline[BOOT_ARGS_SIZE];
>>> >> +  android_cmdline[0] = '';
>>> >> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof
>>> >> "android_bootimg:" - 1) == 0)
>>> >> +    grub_android_bootimg_load_kernel (argv[0] + sizeof
>>> >> "android_bootimg:" - 1,
>>> >> +                                      &file, android_cmdline);
>>> >> +  else
>>> >> +      file = grub_file_open (argv[0]);
>>> > I hoped more for autodetection. This gets a bit hairy and proper
>>> > separation is better. Sorry for confusion. I think it's simpler with
>>> > commands like
>>> > android_bootimg [--no-cmdline] [--no-initrd] IMAGE [EXTRA_ARGUMENTS]
>>> > by default it will load both IMAGE, with cmdline and initrd. With
>>> > --no-initrd you can use initrd for custom initrd.
>>>
>>> Autodetection would be possible actually, I didn't think of that. If
>>> grub_file_open fails, we can try grub_android_bootimg_load_kernel on the
>>> same file. Would that be preferable or do we still want a separate
>>> command?
>>>
>>> >
>>> >>    if (! file)
>>> >>      goto fail;
>>> >>
>>> >> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd
>>> >> __attribute__ ((unused)),
>>> >>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>>> >>    if (!linux_cmdline)
>>> >>      goto fail;
>>> >> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
>>> >> +  grub_size_t cmdline_offset = 0;
>>> >> +  if (android_cmdline[0])
>>> >> +    {
>>> >> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
>>> >> +      grub_memcpy (linux_cmdline, android_cmdline, cmdline_offset -
>>> >> 1);
>>> >> +      linux_cmdline[cmdline_offset - 1] = ' ';
>>> >> +    }
>>> >> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, sizeof
>>> >> (LINUX_IMAGE));
>>> >> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
>>> > LINUX_IMAGE must be at the beginning. don't forget brackets around
>>> > sizeof.
>>> >>    grub_create_loader_cmdline (argc, argv,
>>> >> -                          linux_cmdline
>>> >> -                          + sizeof (LINUX_IMAGE) - 1,
>>> >> -                          maximal_cmdline_size
>>> >> -                          - (sizeof (LINUX_IMAGE) - 1));
>>> >> +                              linux_cmdline
>>> >> +                              + cmdline_offset,
>>> >> +                              maximal_cmdline_size
>>> >> +                              - cmdline_offset);
>>> >>
>>> >>    len = prot_file_size;
>>> >>    if (grub_file_read (file, prot_mode_mem, len) != len &&
>>> >> !grub_errno)
>>> >>
>>> >
>>> >
>>> >
>>> > _______________________________________________
>>> > Grub-devel mailing list
>>> > Grub-devel@gnu.org
>>> > https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>>
>>
>> Links:
>> ------
>> [1] https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: android.diff --]
[-- Type: text/x-diff; name="android.diff", Size: 15380 bytes --]

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index fddcc46..d3870ae 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -35,6 +35,7 @@
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
 #include <grub/linux.h>
+#include <grub/android.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -673,11 +674,11 @@ grub_linux_unload (void)
   return GRUB_ERR_NONE;
 }
 
+
 static grub_err_t
-grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
-		int argc, char *argv[])
+linux_load (grub_file_t file, grub_off_t fileoffset, grub_off_t filesize,
+	    const char *extra_cmdline, int argc, char *argv[])
 {
-  grub_file_t file = 0;
   struct linux_kernel_header lh;
   grub_uint8_t setup_sects;
   grub_size_t real_size, prot_size, prot_file_size;
@@ -689,15 +690,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 
   grub_dl_ref (my_mod);
 
-  if (argc == 0)
-    {
-      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
-      goto fail;
-    }
-
-  file = grub_file_open (argv[0]);
-  if (! file)
-    goto fail;
+  grub_file_seek (file, fileoffset);
 
   if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
     {
@@ -757,7 +750,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
     setup_sects = GRUB_LINUX_DEFAULT_SETUP_SECTS;
 
   real_size = setup_sects << GRUB_DISK_SECTOR_BITS;
-  prot_file_size = grub_file_size (file) - real_size - GRUB_DISK_SECTOR_SIZE;
+  prot_file_size = filesize - real_size - GRUB_DISK_SECTOR_SIZE;
 
   if (grub_le_to_cpu16 (lh.version) >= 0x205
       && lh.kernel_alignment != 0
@@ -871,7 +864,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 
   /* The other parameters are filled when booting.  */
 
-  grub_file_seek (file, real_size + GRUB_DISK_SECTOR_SIZE);
+  grub_file_seek (file, fileoffset + real_size + GRUB_DISK_SECTOR_SIZE);
 
   grub_dprintf ("linux", "bzImage, setup=0x%x, size=0x%x\n",
 		(unsigned) real_size, (unsigned) prot_size);
@@ -1008,12 +1001,32 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
   if (!linux_cmdline)
     goto fail;
-  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
-  grub_create_loader_cmdline (argc, argv,
-			      linux_cmdline
-			      + sizeof (LINUX_IMAGE) - 1,
-			      maximal_cmdline_size
-			      - (sizeof (LINUX_IMAGE) - 1));
+  char *cmdline_ptr = linux_cmdline;
+  char *cmdline_ptr_end = linux_cmdline + maximal_cmdline_size;
+  /* Construct BOOT_IMAGE=/boot/... */
+  cmdline_ptr = grub_stpcpy (cmdline_ptr, LINUX_IMAGE);
+  grub_create_loader_cmdline (1, argv, cmdline_ptr,
+			      cmdline_ptr_end - cmdline_ptr);
+  cmdline_ptr += grub_strlen (cmdline_ptr);
+  /* Extra */
+  if (cmdline_ptr < cmdline_ptr_end - 1 && *extra_cmdline)
+    {
+      *cmdline_ptr++ = ' ';
+      cmdline_ptr = grub_stpncpy (cmdline_ptr, extra_cmdline,
+				  cmdline_ptr_end - cmdline_ptr);
+    }
+
+  /* Rest of command line.  */
+  if (cmdline_ptr < cmdline_ptr_end - 1 && argc > 1)
+    {
+      *cmdline_ptr++ = ' ';
+      grub_create_loader_cmdline (argc - 1, argv + 1, cmdline_ptr,
+				  cmdline_ptr_end - cmdline_ptr);
+    }
+  else
+    {
+      *cmdline_ptr = '\0';
+    }
 
   len = prot_file_size;
   if (grub_file_read (file, prot_mode_mem, len) != len && !grub_errno)
@@ -1029,9 +1042,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 
  fail:
 
-  if (file)
-    grub_file_close (file);
-
   if (grub_errno != GRUB_ERR_NONE)
     {
       grub_dl_unref (my_mod);
@@ -1042,31 +1052,32 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 }
 
 static grub_err_t
-grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
-		 int argc, char *argv[])
+grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
+		int argc, char *argv[])
 {
-  grub_size_t size = 0, aligned_size = 0;
-  grub_addr_t addr_min, addr_max;
-  grub_addr_t addr;
+  grub_file_t file = 0;
   grub_err_t err;
-  struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
-
   if (argc == 0)
-    {
-      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
-      goto fail;
-    }
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
-  if (! loaded)
-    {
-      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("you need to load the kernel first"));
-      goto fail;
-    }
+  file = grub_file_open (argv[0]);
+  if (! file)
+    return grub_errno;
 
-  if (grub_initrd_init (argc, argv, &initrd_ctx))
-    goto fail;
+  err = linux_load (file, 0, grub_file_size (file), "", argc, argv);
+  grub_file_close (file);
+  return err;
+}
+
+static grub_err_t
+load_initrd (struct grub_linux_initrd_context *initrd_ctx, char *filenames[])
+{
+  grub_size_t size = 0, aligned_size = 0;
+  grub_addr_t addr_min, addr_max;
+  grub_addr_t addr;
+  grub_err_t err;
 
-  size = grub_get_initrd_size (&initrd_ctx);
+  size = grub_get_initrd_size (initrd_ctx);
   aligned_size = ALIGN_UP (size, 4096);
 
   /* Get the highest address available for the initrd.  */
@@ -1098,10 +1109,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
   addr = (addr_max - aligned_size) & ~0xFFF;
 
   if (addr < addr_min)
-    {
-      grub_error (GRUB_ERR_OUT_OF_RANGE, "the initrd is too big");
-      goto fail;
-    }
+    return grub_error (GRUB_ERR_OUT_OF_RANGE, "the initrd is too big");
 
   {
     grub_relocator_chunk_t ch;
@@ -1116,8 +1124,9 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
     initrd_mem_target = get_physical_target_address (ch);
   }
 
-  if (grub_initrd_load (&initrd_ctx, argv, initrd_mem))
-    goto fail;
+  err = grub_initrd_load (initrd_ctx, filenames, initrd_mem);
+  if (err)
+    return err;
 
   grub_dprintf ("linux", "Initrd, addr=0x%x, size=0x%x\n",
 		(unsigned) addr, (unsigned) size);
@@ -1126,18 +1135,123 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
   linux_params.ramdisk_size = size;
   linux_params.root_dev = 0x0100; /* XXX */
 
- fail:
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
+		 int argc, char *argv[])
+{
+  struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
+  grub_err_t err;
+
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  if (! loaded)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("you need to load the kernel first"));
+
+  err = grub_initrd_init (argc, argv, &initrd_ctx);
+
+  if (!err)
+    err = load_initrd (&initrd_ctx, argv);
+
   grub_initrd_close (&initrd_ctx);
 
+  return err;
+}
+
+static grub_err_t
+grub_cmd_android_bootimg (grub_command_t cmd __attribute__ ((unused)),
+			  int argc, char *argv[])
+{
+  grub_file_t file = 0;
+  grub_err_t err  = GRUB_ERR_NONE;
+  int do_load_initrd = 1, do_load_cmdline = 1;
+  struct grub_android_boot_img_hdr boot_img_header;
+  char *cmdline = NULL;
+
+  for (; argc > 0; argc--, argv++)
+    {
+      if (grub_strcmp (argv[0], "--no-cmdline") == 0)
+	{
+	  do_load_cmdline = 0;
+	  continue;
+	}
+      if (grub_strcmp (argv[0], "--no-initrd") == 0)
+	{
+	  do_load_initrd = 0;
+	  continue;
+	}
+      break;
+    }
+      
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  file = grub_file_open (argv[0]);
+  if (! file)
+    return grub_errno;
+
+  if (grub_file_read (file, &boot_img_header, sizeof (boot_img_header)) != sizeof (boot_img_header))
+    {
+      if (!grub_errno)
+	grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
+		    argv[0]);
+      err = grub_errno;
+      goto fail;
+    }
+
+  if (grub_memcmp (boot_img_header.magic, GRUB_ANDROID_BOOT_MAGIC, GRUB_ANDROID_BOOT_MAGIC_SIZE) != 0)
+    {
+      err = grub_error (GRUB_ERR_BAD_OS, "invalid android magic");
+      goto fail;
+    }
+
+  if (do_load_cmdline)
+    {
+      cmdline = grub_malloc (GRUB_ANDROID_BOOT_EXTRA_ARGS_SIZE
+			     + GRUB_ANDROID_BOOT_ARGS_SIZE + 1);
+      if (!cmdline)
+	goto fail;
+      grub_memcpy (cmdline, boot_img_header.cmdline, GRUB_ANDROID_BOOT_ARGS_SIZE);
+      grub_memcpy (cmdline + GRUB_ANDROID_BOOT_ARGS_SIZE, boot_img_header.extra_cmdline, GRUB_ANDROID_BOOT_EXTRA_ARGS_SIZE);
+      cmdline[GRUB_ANDROID_BOOT_EXTRA_ARGS_SIZE + GRUB_ANDROID_BOOT_ARGS_SIZE] = '\0';
+    }
+
+  err = linux_load (file, boot_img_header.page_size, boot_img_header.kernel_size, cmdline ? : "", argc, argv);
+  if (err)
+    goto fail;
+
+  if (do_load_initrd && boot_img_header.ramdisk_size)
+    {
+      struct grub_linux_initrd_component component = {
+	.size = boot_img_header.ramdisk_size,
+	.newc_name = 0,
+	.file = file,
+	.fileoffset = ALIGN_UP (boot_img_header.kernel_size, boot_img_header.page_size)
+      };
+      struct grub_linux_initrd_context initrd_ctx = {
+	.nfiles = 1,
+	.size = boot_img_header.ramdisk_size,
+	.components = &component
+      };
+      err = load_initrd (&initrd_ctx, argv);
+    }
+ fail:
+  grub_file_close (file);
   return grub_errno;
 }
 
-static grub_command_t cmd_linux, cmd_initrd;
+
+static grub_command_t cmd_linux, cmd_initrd, cmd_android_bootimg;
 
 GRUB_MOD_INIT(linux)
 {
   cmd_linux = grub_register_command ("linux", grub_cmd_linux,
 				     0, N_("Load Linux."));
+  cmd_android_bootimg = grub_register_command ("android_bootimg", grub_cmd_android_bootimg,
+					       "[--no-initrd] [--no-cmdline] IMAGE [OPTIONS]", N_("Load Android bootimg."));
   cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd,
 				      0, N_("Load initrd."));
   my_mod = mod;
@@ -1147,4 +1261,5 @@ GRUB_MOD_FINI(linux)
 {
   grub_unregister_command (cmd_linux);
   grub_unregister_command (cmd_initrd);
+  grub_unregister_command (cmd_android_bootimg);
 }
diff --git a/grub-core/loader/linux.c b/grub-core/loader/linux.c
index be6fa0f..dea0299 100644
--- a/grub-core/loader/linux.c
+++ b/grub-core/loader/linux.c
@@ -23,13 +23,6 @@ struct newc_head
   char check[8];
 } GRUB_PACKED;
 
-struct grub_linux_initrd_component
-{
-  grub_file_t file;
-  char *newc_name;
-  grub_off_t size;
-};
-
 struct dir
 {
   char *name;
@@ -208,6 +201,7 @@ grub_initrd_init (int argc, char *argv[],
       initrd_ctx->nfiles++;
       initrd_ctx->components[i].size
 	= grub_file_size (initrd_ctx->components[i].file);
+      initrd_ctx->components[i].fileoffset = 0;
       initrd_ctx->size += initrd_ctx->components[i].size;
     }
 
@@ -279,6 +273,8 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx,
 	}
 
       cursize = initrd_ctx->components[i].size;
+      grub_file_seek (initrd_ctx->components[i].file,
+		      initrd_ctx->components[i].fileoffset);
       if (grub_file_read (initrd_ctx->components[i].file, ptr, cursize)
 	  != cursize)
 	{
diff --git a/include/grub/android.h b/include/grub/android.h
new file mode 100644
index 0000000..7cd25f4
--- /dev/null
+++ b/include/grub/android.h
@@ -0,0 +1,73 @@
+/* tools/mkbootimg/bootimg.h
+**
+** Copyright 2007, The Android Open Source Project
+**
+** Licensed under the Apache License, Version 2.0 (the "License"); 
+** you may not use this file except in compliance with the License. 
+** You may obtain a copy of the License at 
+**
+**     http://www.apache.org/licenses/LICENSE-2.0 
+**
+** Unless required by applicable law or agreed to in writing, software 
+** distributed under the License is distributed on an "AS IS" BASIS, 
+** WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. 
+** See the License for the specific language governing permissions and 
+** limitations under the License.
+*/
+#ifndef _GRUB_ANDROID_BOOT_IMAGE_H_
+#define _GRUB_ANDROID_BOOT_IMAGE_H_
+
+#include <grub/types.h>
+
+#define GRUB_ANDROID_BOOT_MAGIC "ANDROID!"
+#define GRUB_ANDROID_BOOT_MAGIC_SIZE 8
+#define GRUB_ANDROID_BOOT_NAME_SIZE 16
+#define GRUB_ANDROID_BOOT_ARGS_SIZE 512
+#define GRUB_ANDROID_BOOT_EXTRA_ARGS_SIZE 1024
+struct grub_android_boot_img_hdr
+{
+    grub_uint8_t magic[GRUB_ANDROID_BOOT_MAGIC_SIZE];
+    grub_uint32_t kernel_size;  /* size in bytes */
+    grub_uint32_t kernel_addr;  /* physical load addr */
+    grub_uint32_t ramdisk_size; /* size in bytes */
+    grub_uint32_t ramdisk_addr; /* physical load addr */
+    grub_uint32_t second_size;  /* size in bytes */
+    grub_uint32_t second_addr;  /* physical load addr */
+    grub_uint32_t tags_addr;    /* physical addr for kernel tags */
+    grub_uint32_t page_size;    /* flash page size we assume */
+    grub_uint32_t unused[2];    /* future expansion: should be 0 */
+    grub_uint8_t name[GRUB_ANDROID_BOOT_NAME_SIZE]; /* asciiz product name */
+    grub_uint8_t cmdline[GRUB_ANDROID_BOOT_ARGS_SIZE];
+    grub_uint32_t id[8]; /* timestamp / checksum / sha1 / etc */
+    /* Supplemental command line data; kept here to maintain
+     * binary compatibility with older versions of mkbootimg */
+    grub_uint8_t extra_cmdline[GRUB_ANDROID_BOOT_EXTRA_ARGS_SIZE];
+} __attribute__((packed));
+/*
+** +-----------------+ 
+** | boot header     | 1 page
+** +-----------------+
+** | kernel          | n pages  
+** +-----------------+
+** | ramdisk         | m pages  
+** +-----------------+
+** | second stage    | o pages
+** +-----------------+
+**
+** n = (kernel_size + page_size - 1) / page_size
+** m = (ramdisk_size + page_size - 1) / page_size
+** o = (second_size + page_size - 1) / page_size
+**
+** 0. all entities are page_size aligned in flash
+** 1. kernel and ramdisk are required (size != 0)
+** 2. second is optional (second_size == 0 -> no second)
+** 3. load each element (kernel, ramdisk, second) at
+**    the specified physical address (kernel_addr, etc)
+** 4. prepare tags at tag_addr.  kernel_args[] is
+**    appended to the kernel commandline in the tags.
+** 5. r0 = 0, r1 = MACHINE_TYPE, r2 = tags_addr
+** 6. if second_size != 0: jump to second_addr
+**    else: jump to kernel_addr
+*/
+
+#endif
diff --git a/include/grub/linux.h b/include/grub/linux.h
index 594a3f3..4a99d78 100644
--- a/include/grub/linux.h
+++ b/include/grub/linux.h
@@ -9,6 +9,14 @@ struct grub_linux_initrd_context
   grub_size_t size;
 };
 
+struct grub_linux_initrd_component
+{
+  grub_file_t file;
+  char *newc_name;
+  grub_off_t fileoffset;
+  grub_off_t size;
+};
+
 grub_err_t
 grub_initrd_init (int argc, char *argv[],
 		  struct grub_linux_initrd_context *ctx);
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 2a9f87c..ef9c9b6 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -64,6 +64,17 @@ grub_stpcpy (char *dest, const char *src)
   return d - 1;
 }
 
+static inline char *
+grub_stpncpy (char *dest, const char *src, int c)
+{
+  char *p = dest;
+
+  while ((*p++ = *src++) != '\0' && --c)
+    ;
+
+  return p - 1;
+}
+
 /* XXX: If grub_memmove is too slow, we must implement grub_memcpy.  */
 static inline void *
 grub_memcpy (void *dest, const void *src, grub_size_t n)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Andrei Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 3983 bytes --]

13.02.2016 00:16, Vladimir 'φ-coder/phcoder' Serbinenko пишет:
> On 12.02.2016 20:34, Shea Levy wrote:
>> > OK. Do you have any thoughts on how best to extract the "load the kernel
>> > and set the command line to a specific string" functionality out of the
>> > linux command, then?
>> > 
> At first I wanted to write a short skeleton patch to show what I meant
> but once skeleton is done, there is little actual code involved. Please
> try attached patch

Yes, exactly what I meant. Nitpicks below.

> android.diff
> 
> 
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index fddcc46..d3870ae 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -35,6 +35,7 @@
>  #include <grub/i18n.h>
>  #include <grub/lib/cmdline.h>
>  #include <grub/linux.h>
> +#include <grub/android.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -673,11 +674,11 @@ grub_linux_unload (void)
>    return GRUB_ERR_NONE;
>  }
>  
> +
>  static grub_err_t
> -grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> -		int argc, char *argv[])
> +linux_load (grub_file_t file, grub_off_t fileoffset, grub_off_t filesize,
> +	    const char *extra_cmdline, int argc, char *argv[])
>  {
> -  grub_file_t file = 0;
>    struct linux_kernel_header lh;
>    grub_uint8_t setup_sects;
>    grub_size_t real_size, prot_size, prot_file_size;
> @@ -689,15 +690,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>  
>    grub_dl_ref (my_mod);
>  
> -  if (argc == 0)
> -    {
> -      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> -      goto fail;
> -    }
> -
> -  file = grub_file_open (argv[0]);
> -  if (! file)
> -    goto fail;
> +  grub_file_seek (file, fileoffset);
>  
>    if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
>      {
> @@ -757,7 +750,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>      setup_sects = GRUB_LINUX_DEFAULT_SETUP_SECTS;
>  
>    real_size = setup_sects << GRUB_DISK_SECTOR_BITS;
> -  prot_file_size = grub_file_size (file) - real_size - GRUB_DISK_SECTOR_SIZE;
> +  prot_file_size = filesize - real_size - GRUB_DISK_SECTOR_SIZE;
>  
>    if (grub_le_to_cpu16 (lh.version) >= 0x205
>        && lh.kernel_alignment != 0
> @@ -871,7 +864,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>  
>    /* The other parameters are filled when booting.  */
>  
> -  grub_file_seek (file, real_size + GRUB_DISK_SECTOR_SIZE);
> +  grub_file_seek (file, fileoffset + real_size + GRUB_DISK_SECTOR_SIZE);
>  
>    grub_dprintf ("linux", "bzImage, setup=0x%x, size=0x%x\n",
>  		(unsigned) real_size, (unsigned) prot_size);
> @@ -1008,12 +1001,32 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>    if (!linux_cmdline)
>      goto fail;
> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
> -  grub_create_loader_cmdline (argc, argv,
> -			      linux_cmdline
> -			      + sizeof (LINUX_IMAGE) - 1,
> -			      maximal_cmdline_size
> -			      - (sizeof (LINUX_IMAGE) - 1));
> +  char *cmdline_ptr = linux_cmdline;
> +  char *cmdline_ptr_end = linux_cmdline + maximal_cmdline_size;
> +  /* Construct BOOT_IMAGE=/boot/... */
> +  cmdline_ptr = grub_stpcpy (cmdline_ptr, LINUX_IMAGE);
> +  grub_create_loader_cmdline (1, argv, cmdline_ptr,

What if argc is zero? We checked this before, but check is removed. This
became library function now, it should check input for sanity.
...

> diff --git a/include/grub/misc.h b/include/grub/misc.h
> index 2a9f87c..ef9c9b6 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -64,6 +64,17 @@ grub_stpcpy (char *dest, const char *src)
>    return d - 1;
>  }
>  
> +static inline char *
> +grub_stpncpy (char *dest, const char *src, int c)

"Official" prototype has size_t here.




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Shea Levy]

This patch uses grub_file_open, but the android bootimg is a disk, not 
a file. You mentioned something about file_offset_open, but that also 
expects an input file, not a disk. Should I modify your patch with my 
code I wrote to create a grub_file_t from an android_bootimg disk 
device, or is there another approach?

On 2016-02-12 16:16, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 12.02.2016 20:34, Shea Levy wrote:
>> OK. Do you have any thoughts on how best to extract the "load the 
>> kernel
>> and set the command line to a specific string" functionality out of 
>> the
>> linux command, then?
>>
> At first I wanted to write a short skeleton patch to show what I 
> meant
> but once skeleton is done, there is little actual code involved. 
> Please
> try attached patch
>> On 2016-02-12 14:22, Vladimir 'phcoder' Serbinenko wrote:
>>> Separate command is better as it keeps interface tidy and 
>>> unpoluted,
>>> decreasing maintenance cost. Correct me if I'm wrong but it should 
>>> be
>>> clear from context of file is Android image or usual linux one?
>>>
>>> Le ven. 12 févr. 2016 20:19, Shea Levy <shea@shealevy.com> a écrit 
>>> :
>>>
>>>> On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>> > On 08.02.2016 21:47, Shea Levy wrote:
>>>> >> ---
>>>> >>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
>>>> >>  1 file changed, 21 insertions(+), 6 deletions(-)
>>>> >>
>>>> >> diff --git a/grub-core/loader/i386/linux.c
>>>> >> b/grub-core/loader/i386/linux.c
>>>> >> index fddcc46..6ab8d3c 100644
>>>> >> --- a/grub-core/loader/i386/linux.c
>>>> >> +++ b/grub-core/loader/i386/linux.c
>>>> >> @@ -35,6 +35,7 @@
>>>> >>  #include <grub/i18n.h>
>>>> >>  #include <grub/lib/cmdline.h>
>>>> >>  #include <grub/linux.h>
>>>> >> +#include <grub/android_bootimg.h>
>>>> >>
>>>> >>  GRUB_MOD_LICENSE ("GPLv3+");
>>>> >>
>>>> >> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd
>>>> >> __attribute__ ((unused)),
>>>> >>        goto fail;
>>>> >>      }
>>>> >>
>>>> >> -  file = grub_file_open (argv[0]);
>>>> >> +  char android_cmdline[BOOT_ARGS_SIZE];
>>>> >> +  android_cmdline[0] = '';
>>>> >> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof
>>>> >> "android_bootimg:" - 1) == 0)
>>>> >> +    grub_android_bootimg_load_kernel (argv[0] + sizeof
>>>> >> "android_bootimg:" - 1,
>>>> >> +                                      &file, android_cmdline);
>>>> >> +  else
>>>> >> +      file = grub_file_open (argv[0]);
>>>> > I hoped more for autodetection. This gets a bit hairy and proper
>>>> > separation is better. Sorry for confusion. I think it's simpler 
>>>> with
>>>> > commands like
>>>> > android_bootimg [--no-cmdline] [--no-initrd] IMAGE 
>>>> [EXTRA_ARGUMENTS]
>>>> > by default it will load both IMAGE, with cmdline and initrd. 
>>>> With
>>>> > --no-initrd you can use initrd for custom initrd.
>>>>
>>>> Autodetection would be possible actually, I didn't think of that. 
>>>> If
>>>> grub_file_open fails, we can try grub_android_bootimg_load_kernel 
>>>> on the
>>>> same file. Would that be preferable or do we still want a separate
>>>> command?
>>>>
>>>> >
>>>> >>    if (! file)
>>>> >>      goto fail;
>>>> >>
>>>> >> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd
>>>> >> __attribute__ ((unused)),
>>>> >>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>>>> >>    if (!linux_cmdline)
>>>> >>      goto fail;
>>>> >> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof 
>>>> (LINUX_IMAGE));
>>>> >> +  grub_size_t cmdline_offset = 0;
>>>> >> +  if (android_cmdline[0])
>>>> >> +    {
>>>> >> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
>>>> >> +      grub_memcpy (linux_cmdline, android_cmdline, 
>>>> cmdline_offset -
>>>> >> 1);
>>>> >> +      linux_cmdline[cmdline_offset - 1] = ' ';
>>>> >> +    }
>>>> >> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, 
>>>> sizeof
>>>> >> (LINUX_IMAGE));
>>>> >> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
>>>> > LINUX_IMAGE must be at the beginning. don't forget brackets 
>>>> around
>>>> > sizeof.
>>>> >>    grub_create_loader_cmdline (argc, argv,
>>>> >> -                          linux_cmdline
>>>> >> -                          + sizeof (LINUX_IMAGE) - 1,
>>>> >> -                          maximal_cmdline_size
>>>> >> -                          - (sizeof (LINUX_IMAGE) - 1));
>>>> >> +                              linux_cmdline
>>>> >> +                              + cmdline_offset,
>>>> >> +                              maximal_cmdline_size
>>>> >> +                              - cmdline_offset);
>>>> >>
>>>> >>    len = prot_file_size;
>>>> >>    if (grub_file_read (file, prot_mode_mem, len) != len &&
>>>> >> !grub_errno)
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > Grub-devel mailing list
>>>> > Grub-devel@gnu.org
>>>> > https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>>>
>>>
>>> Links:
>>> ------
>>> [1] https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 6621 bytes --]

Le dim. 14 févr. 2016 14:21, Shea Levy <shea@shealevy.com> a écrit :

> This patch uses grub_file_open, but the android bootimg is a disk, not
> a file. You mentioned something about file_offset_open, but that also
> expects an input file, not a disk. Should I modify your patch with my
> code I wrote to create a grub_file_t from an android_bootimg disk
> device, or is there another approach?
>
We already have syntax (hd0,1)+<number of sectors> that we use for i.a.
chainloader perhaps we should extend it to have (hd0,1)+ meaning whole disk
as file? Or even allow the disk to be opened with GRUB_file_open? I'd like
a second opinion on this. Andrei, what do you think?

>
> On 2016-02-12 16:16, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > On 12.02.2016 20:34, Shea Levy wrote:
> >> OK. Do you have any thoughts on how best to extract the "load the
> >> kernel
> >> and set the command line to a specific string" functionality out of
> >> the
> >> linux command, then?
> >>
> > At first I wanted to write a short skeleton patch to show what I
> > meant
> > but once skeleton is done, there is little actual code involved.
> > Please
> > try attached patch
> >> On 2016-02-12 14:22, Vladimir 'phcoder' Serbinenko wrote:
> >>> Separate command is better as it keeps interface tidy and
> >>> unpoluted,
> >>> decreasing maintenance cost. Correct me if I'm wrong but it should
> >>> be
> >>> clear from context of file is Android image or usual linux one?
> >>>
> >>> Le ven. 12 févr. 2016 20:19, Shea Levy <shea@shealevy.com> a écrit
> >>> :
> >>>
> >>>> On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >>>> > On 08.02.2016 21:47, Shea Levy wrote:
> >>>> >> ---
> >>>> >>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
> >>>> >>  1 file changed, 21 insertions(+), 6 deletions(-)
> >>>> >>
> >>>> >> diff --git a/grub-core/loader/i386/linux.c
> >>>> >> b/grub-core/loader/i386/linux.c
> >>>> >> index fddcc46..6ab8d3c 100644
> >>>> >> --- a/grub-core/loader/i386/linux.c
> >>>> >> +++ b/grub-core/loader/i386/linux.c
> >>>> >> @@ -35,6 +35,7 @@
> >>>> >>  #include <grub/i18n.h>
> >>>> >>  #include <grub/lib/cmdline.h>
> >>>> >>  #include <grub/linux.h>
> >>>> >> +#include <grub/android_bootimg.h>
> >>>> >>
> >>>> >>  GRUB_MOD_LICENSE ("GPLv3+");
> >>>> >>
> >>>> >> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd
> >>>> >> __attribute__ ((unused)),
> >>>> >>        goto fail;
> >>>> >>      }
> >>>> >>
> >>>> >> -  file = grub_file_open (argv[0]);
> >>>> >> +  char android_cmdline[BOOT_ARGS_SIZE];
> >>>> >> +  android_cmdline[0] = '';
> >>>> >> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof
> >>>> >> "android_bootimg:" - 1) == 0)
> >>>> >> +    grub_android_bootimg_load_kernel (argv[0] + sizeof
> >>>> >> "android_bootimg:" - 1,
> >>>> >> +                                      &file, android_cmdline);
> >>>> >> +  else
> >>>> >> +      file = grub_file_open (argv[0]);
> >>>> > I hoped more for autodetection. This gets a bit hairy and proper
> >>>> > separation is better. Sorry for confusion. I think it's simpler
> >>>> with
> >>>> > commands like
> >>>> > android_bootimg [--no-cmdline] [--no-initrd] IMAGE
> >>>> [EXTRA_ARGUMENTS]
> >>>> > by default it will load both IMAGE, with cmdline and initrd.
> >>>> With
> >>>> > --no-initrd you can use initrd for custom initrd.
> >>>>
> >>>> Autodetection would be possible actually, I didn't think of that.
> >>>> If
> >>>> grub_file_open fails, we can try grub_android_bootimg_load_kernel
> >>>> on the
> >>>> same file. Would that be preferable or do we still want a separate
> >>>> command?
> >>>>
> >>>> >
> >>>> >>    if (! file)
> >>>> >>      goto fail;
> >>>> >>
> >>>> >> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd
> >>>> >> __attribute__ ((unused)),
> >>>> >>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
> >>>> >>    if (!linux_cmdline)
> >>>> >>      goto fail;
> >>>> >> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof
> >>>> (LINUX_IMAGE));
> >>>> >> +  grub_size_t cmdline_offset = 0;
> >>>> >> +  if (android_cmdline[0])
> >>>> >> +    {
> >>>> >> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
> >>>> >> +      grub_memcpy (linux_cmdline, android_cmdline,
> >>>> cmdline_offset -
> >>>> >> 1);
> >>>> >> +      linux_cmdline[cmdline_offset - 1] = ' ';
> >>>> >> +    }
> >>>> >> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE,
> >>>> sizeof
> >>>> >> (LINUX_IMAGE));
> >>>> >> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
> >>>> > LINUX_IMAGE must be at the beginning. don't forget brackets
> >>>> around
> >>>> > sizeof.
> >>>> >>    grub_create_loader_cmdline (argc, argv,
> >>>> >> -                          linux_cmdline
> >>>> >> -                          + sizeof (LINUX_IMAGE) - 1,
> >>>> >> -                          maximal_cmdline_size
> >>>> >> -                          - (sizeof (LINUX_IMAGE) - 1));
> >>>> >> +                              linux_cmdline
> >>>> >> +                              + cmdline_offset,
> >>>> >> +                              maximal_cmdline_size
> >>>> >> +                              - cmdline_offset);
> >>>> >>
> >>>> >>    len = prot_file_size;
> >>>> >>    if (grub_file_read (file, prot_mode_mem, len) != len &&
> >>>> >> !grub_errno)
> >>>> >>
> >>>> >
> >>>> >
> >>>> >
> >>>> > _______________________________________________
> >>>> > Grub-devel mailing list
> >>>> > Grub-devel@gnu.org
> >>>> > https://lists.gnu.org/mailman/listinfo/grub-devel [1]
> >>>>
> >>>> _______________________________________________
> >>>> Grub-devel mailing list
> >>>> Grub-devel@gnu.org
> >>>> https://lists.gnu.org/mailman/listinfo/grub-devel [1]
> >>>
> >>>
> >>> Links:
> >>> ------
> >>> [1] https://lists.gnu.org/mailman/listinfo/grub-devel
> >>>
> >>> _______________________________________________
> >>> Grub-devel mailing list
> >>> Grub-devel@gnu.org
> >>> https://lists.gnu.org/mailman/listinfo/grub-devel
> >>
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> Grub-devel@gnu.org
> >> https://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 10711 bytes --]

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Andrei Borzenkov]

14.02.2016 16:26, Vladimir 'phcoder' Serbinenko пишет:
> Le dim. 14 févr. 2016 14:21, Shea Levy <shea@shealevy.com> a écrit :
> 
>> This patch uses grub_file_open, but the android bootimg is a disk, not
>> a file. You mentioned something about file_offset_open, but that also
>> expects an input file, not a disk. Should I modify your patch with my
>> code I wrote to create a grub_file_t from an android_bootimg disk
>> device, or is there another approach?
>>
> We already have syntax (hd0,1)+<number of sectors> that we use for i.a.
> chainloader perhaps we should extend it to have (hd0,1)+ meaning whole disk
> as file? Or even allow the disk to be opened with GRUB_file_open? I'd like
> a second opinion on this. Andrei, what do you think?
> 

Yes, it was discussed just recently on help-grub. I'd prefer ($dev)+ as
explicit indication that we want blocklists.

The practical problem is that we must allow unknown file size. I am not
sure how deep changes are required. But as the trivial example, what "ls
-l ($dev)+" is going to output? Is "test -s ($dev)+" true or false when
size is unknown?

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Seth Goldberg]

[-- Attachment #1: Type: text/plain, Size: 7207 bytes --]



> On Feb 14, 2016, at 5:26 AM, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com> wrote:
> 
> 
> 
> Le dim. 14 févr. 2016 14:21, Shea Levy <shea@shealevy.com> a écrit :
>> This patch uses grub_file_open, but the android bootimg is a disk, not
>> a file. You mentioned something about file_offset_open, but that also
>> expects an input file, not a disk. Should I modify your patch with my
>> code I wrote to create a grub_file_t from an android_bootimg disk
>> device, or is there another approach?
> 
> We already have syntax (hd0,1)+<number of sectors> that we use for i.a. chainloader perhaps we should extend it to have (hd0,1)+ meaning whole disk as file? Or even allow the disk to be opened with GRUB_file_open? I'd like a second opinion on this. Andrei, what do you think?

  I think syntax that allows a whole disk to be specified (e.g. To the multiboot module command so a disk image can be passed that way) is a great idea.

  --S


>> 
>> On 2016-02-12 16:16, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> > On 12.02.2016 20:34, Shea Levy wrote:
>> >> OK. Do you have any thoughts on how best to extract the "load the
>> >> kernel
>> >> and set the command line to a specific string" functionality out of
>> >> the
>> >> linux command, then?
>> >>
>> > At first I wanted to write a short skeleton patch to show what I
>> > meant
>> > but once skeleton is done, there is little actual code involved.
>> > Please
>> > try attached patch
>> >> On 2016-02-12 14:22, Vladimir 'phcoder' Serbinenko wrote:
>> >>> Separate command is better as it keeps interface tidy and
>> >>> unpoluted,
>> >>> decreasing maintenance cost. Correct me if I'm wrong but it should
>> >>> be
>> >>> clear from context of file is Android image or usual linux one?
>> >>>
>> >>> Le ven. 12 févr. 2016 20:19, Shea Levy <shea@shealevy.com> a écrit
>> >>> :
>> >>>
>> >>>> On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> >>>> > On 08.02.2016 21:47, Shea Levy wrote:
>> >>>> >> ---
>> >>>> >>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
>> >>>> >>  1 file changed, 21 insertions(+), 6 deletions(-)
>> >>>> >>
>> >>>> >> diff --git a/grub-core/loader/i386/linux.c
>> >>>> >> b/grub-core/loader/i386/linux.c
>> >>>> >> index fddcc46..6ab8d3c 100644
>> >>>> >> --- a/grub-core/loader/i386/linux.c
>> >>>> >> +++ b/grub-core/loader/i386/linux.c
>> >>>> >> @@ -35,6 +35,7 @@
>> >>>> >>  #include <grub/i18n.h>
>> >>>> >>  #include <grub/lib/cmdline.h>
>> >>>> >>  #include <grub/linux.h>
>> >>>> >> +#include <grub/android_bootimg.h>
>> >>>> >>
>> >>>> >>  GRUB_MOD_LICENSE ("GPLv3+");
>> >>>> >>
>> >>>> >> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd
>> >>>> >> __attribute__ ((unused)),
>> >>>> >>        goto fail;
>> >>>> >>      }
>> >>>> >>
>> >>>> >> -  file = grub_file_open (argv[0]);
>> >>>> >> +  char android_cmdline[BOOT_ARGS_SIZE];
>> >>>> >> +  android_cmdline[0] = '';
>> >>>> >> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof
>> >>>> >> "android_bootimg:" - 1) == 0)
>> >>>> >> +    grub_android_bootimg_load_kernel (argv[0] + sizeof
>> >>>> >> "android_bootimg:" - 1,
>> >>>> >> +                                      &file, android_cmdline);
>> >>>> >> +  else
>> >>>> >> +      file = grub_file_open (argv[0]);
>> >>>> > I hoped more for autodetection. This gets a bit hairy and proper
>> >>>> > separation is better. Sorry for confusion. I think it's simpler
>> >>>> with
>> >>>> > commands like
>> >>>> > android_bootimg [--no-cmdline] [--no-initrd] IMAGE
>> >>>> [EXTRA_ARGUMENTS]
>> >>>> > by default it will load both IMAGE, with cmdline and initrd.
>> >>>> With
>> >>>> > --no-initrd you can use initrd for custom initrd.
>> >>>>
>> >>>> Autodetection would be possible actually, I didn't think of that.
>> >>>> If
>> >>>> grub_file_open fails, we can try grub_android_bootimg_load_kernel
>> >>>> on the
>> >>>> same file. Would that be preferable or do we still want a separate
>> >>>> command?
>> >>>>
>> >>>> >
>> >>>> >>    if (! file)
>> >>>> >>      goto fail;
>> >>>> >>
>> >>>> >> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd
>> >>>> >> __attribute__ ((unused)),
>> >>>> >>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>> >>>> >>    if (!linux_cmdline)
>> >>>> >>      goto fail;
>> >>>> >> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof
>> >>>> (LINUX_IMAGE));
>> >>>> >> +  grub_size_t cmdline_offset = 0;
>> >>>> >> +  if (android_cmdline[0])
>> >>>> >> +    {
>> >>>> >> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
>> >>>> >> +      grub_memcpy (linux_cmdline, android_cmdline,
>> >>>> cmdline_offset -
>> >>>> >> 1);
>> >>>> >> +      linux_cmdline[cmdline_offset - 1] = ' ';
>> >>>> >> +    }
>> >>>> >> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE,
>> >>>> sizeof
>> >>>> >> (LINUX_IMAGE));
>> >>>> >> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
>> >>>> > LINUX_IMAGE must be at the beginning. don't forget brackets
>> >>>> around
>> >>>> > sizeof.
>> >>>> >>    grub_create_loader_cmdline (argc, argv,
>> >>>> >> -                          linux_cmdline
>> >>>> >> -                          + sizeof (LINUX_IMAGE) - 1,
>> >>>> >> -                          maximal_cmdline_size
>> >>>> >> -                          - (sizeof (LINUX_IMAGE) - 1));
>> >>>> >> +                              linux_cmdline
>> >>>> >> +                              + cmdline_offset,
>> >>>> >> +                              maximal_cmdline_size
>> >>>> >> +                              - cmdline_offset);
>> >>>> >>
>> >>>> >>    len = prot_file_size;
>> >>>> >>    if (grub_file_read (file, prot_mode_mem, len) != len &&
>> >>>> >> !grub_errno)
>> >>>> >>
>> >>>> >
>> >>>> >
>> >>>> >
>> >>>> > _______________________________________________
>> >>>> > Grub-devel mailing list
>> >>>> > Grub-devel@gnu.org
>> >>>> > https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>> >>>>
>> >>>> _______________________________________________
>> >>>> Grub-devel mailing list
>> >>>> Grub-devel@gnu.org
>> >>>> https://lists.gnu.org/mailman/listinfo/grub-devel [1]
>> >>>
>> >>>
>> >>> Links:
>> >>> ------
>> >>> [1] https://lists.gnu.org/mailman/listinfo/grub-devel
>> >>>
>> >>> _______________________________________________
>> >>> Grub-devel mailing list
>> >>> Grub-devel@gnu.org
>> >>> https://lists.gnu.org/mailman/listinfo/grub-devel
>> >>
>> >>
>> >> _______________________________________________
>> >> Grub-devel mailing list
>> >> Grub-devel@gnu.org
>> >> https://lists.gnu.org/mailman/listinfo/grub-devel
>> >
>> >
>> > _______________________________________________
>> > Grub-devel mailing list
>> > Grub-devel@gnu.org
>> > https://lists.gnu.org/mailman/listinfo/grub-devel
>> 
>> 
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 12360 bytes --]

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Andrei Borzenkov]

14.02.2016 23:58, Seth Goldberg пишет:
> 
> 
>> On Feb 14, 2016, at 5:26 AM, Vladimir 'phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>> 
>> 
>> 
>> Le dim. 14 févr. 2016 14:21, Shea Levy <shea@shealevy.com> a écrit
>> :
>>> This patch uses grub_file_open, but the android bootimg is a
>>> disk, not a file. You mentioned something about file_offset_open,
>>> but that also expects an input file, not a disk. Should I modify
>>> your patch with my code I wrote to create a grub_file_t from an
>>> android_bootimg disk device, or is there another approach?
>> 
>> We already have syntax (hd0,1)+<number of sectors> that we use for
>> i.a. chainloader perhaps we should extend it to have (hd0,1)+
>> meaning whole disk as file? Or even allow the disk to be opened
>> with GRUB_file_open? I'd like a second opinion on this. Andrei,
>> what do you think?
> 
> I think syntax that allows a whole disk to be specified (e.g. To the
> multiboot module command so a disk image can be passed that way) is a
> great idea.
> 

The problem is that "whole" disk may not have define length which in
turn means quite a lot of rewrite everywhere (most loaders assume that
file they get has size and this size is what they load).

I assume you do know in advance whether you have file or device. In
which case what about extending probe to return size and do

probe --set size --size $dev
multiboot ($dev)+$size

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Seth Goldberg]

> On Feb 14, 2016, at 7:41 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 
> 14.02.2016 23:58, Seth Goldberg пишет:
>> 
>> 
>>> On Feb 14, 2016, at 5:26 AM, Vladimir 'phcoder' Serbinenko
>>> <phcoder@gmail.com> wrote:
>>> 
>>> 
>>> 
>>> Le dim. 14 févr. 2016 14:21, Shea Levy <shea@shealevy.com> a écrit
>>> :
>>>> This patch uses grub_file_open, but the android bootimg is a
>>>> disk, not a file. You mentioned something about file_offset_open,
>>>> but that also expects an input file, not a disk. Should I modify
>>>> your patch with my code I wrote to create a grub_file_t from an
>>>> android_bootimg disk device, or is there another approach?
>>> 
>>> We already have syntax (hd0,1)+<number of sectors> that we use for
>>> i.a. chainloader perhaps we should extend it to have (hd0,1)+
>>> meaning whole disk as file? Or even allow the disk to be opened
>>> with GRUB_file_open? I'd like a second opinion on this. Andrei,
>>> what do you think?
>> 
>> I think syntax that allows a whole disk to be specified (e.g. To the
>> multiboot module command so a disk image can be passed that way) is a
>> great idea.
> 
> The problem is that "whole" disk may not have define length which in
> turn means quite a lot of rewrite everywhere (most loaders assume that
> file they get has size and this size is what they load).
> 
> I assume you do know in advance whether you have file or device. In
> which case what about extending probe to return size and do
> 
> probe --set size --size $dev
> multiboot ($dev)+$size

  Yes I'm referring to devices for which size is
known mostly, though it would also be interesting to support on-demand loading of devices that don't have a fully known size.  The probe syntax is 
also acceptable though some syntactic sugar would also be appreciated ;).

  Thanks,
  --S

> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

On Mon, 15 Feb 2016 04:42 Andrei Borzenkov <arvidjaar@gmail.com> wrote:

> 14.02.2016 23:58, Seth Goldberg пишет:
> >
> >
> >> On Feb 14, 2016, at 5:26 AM, Vladimir 'phcoder' Serbinenko
> >> <phcoder@gmail.com> wrote:
> >>
> >>
> >>
> >> Le dim. 14 févr. 2016 14:21, Shea Levy <shea@shealevy.com> a écrit
> >> :
> >>> This patch uses grub_file_open, but the android bootimg is a
> >>> disk, not a file. You mentioned something about file_offset_open,
> >>> but that also expects an input file, not a disk. Should I modify
> >>> your patch with my code I wrote to create a grub_file_t from an
> >>> android_bootimg disk device, or is there another approach?
> >>
> >> We already have syntax (hd0,1)+<number of sectors> that we use for
> >> i.a. chainloader perhaps we should extend it to have (hd0,1)+
> >> meaning whole disk as file? Or even allow the disk to be opened
> >> with GRUB_file_open? I'd like a second opinion on this. Andrei,
> >> what do you think?
> >
> > I think syntax that allows a whole disk to be specified (e.g. To the
> > multiboot module command so a disk image can be passed that way) is a
> > great idea.
> >
>
> The problem is that "whole" disk may not have define length which in
> turn means quite a lot of rewrite everywhere (most loaders assume that
> file they get has size and this size is what they load).
>
Actually we already have support for unsized files. It's marginal due to
issues you detailed. As long as we error or correctly, it's fine. I looked
thorough the code and there is some bad code around this issue which needs
to be fixed anyway like overflows. I'll prepare a patch

>
>
> I assume you do know in advance whether you have file or device. In
> which case what about extending probe to return size and do
>
> probe --set size --size $dev
> multiboot ($dev)+$size
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2947 bytes --]

[PATCH v3 3/3] Add support for loading initrd from android bootimg

[Shea Levy]

---
 grub-core/loader/linux.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/linux.c b/grub-core/loader/linux.c
index be6fa0f..4c5537a 100644
--- a/grub-core/loader/linux.c
+++ b/grub-core/loader/linux.c
@@ -4,6 +4,7 @@
 #include <grub/misc.h>
 #include <grub/file.h>
 #include <grub/mm.h>
+#include <grub/android_bootimg.h>
 
 struct newc_head
 {
@@ -199,7 +200,11 @@ grub_initrd_init (int argc, char *argv[],
 	  newc = 0;
 	}
       grub_file_filter_disable_compression ();
-      initrd_ctx->components[i].file = grub_file_open (fname);
+      if (grub_memcmp (fname, "android_bootimg:", sizeof "android_bootimg:" - 1) == 0)
+        grub_android_bootimg_load_initrd (fname + sizeof "android_bootimg:" - 1,
+                                          &initrd_ctx->components[i].file);
+      else
+        initrd_ctx->components[i].file = grub_file_open (fname);
       if (!initrd_ctx->components[i].file)
 	{
 	  grub_initrd_close (initrd_ctx);
-- 
2.7.0