[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH 2/5] plat/linuxu: Add initrd memory region
On 28.01.20 05:01, Robert Hrusecky wrote:
Add a new library parameter (initrd_file). The parameter can be used to
map a file on the host filesystem to to a new memory region on boot.
Signed-off-by: Robert Hrusecky <roberth@xxxxxxxxxxxxx>
Signed-off-by: Omar Jamil <omarj2898@xxxxxxxxx>
Signed-off-by: Sachin Beldona <sachinbeldona@xxxxxxxxxx>
---
plat/linuxu/include/linuxu/setup.h | 11 +--
plat/linuxu/memory.c | 105 ++++++++++++++++++++++++-----
2 files changed, 96 insertions(+), 20 deletions(-)
diff --git a/plat/linuxu/include/linuxu/setup.h
b/plat/linuxu/include/linuxu/setup.h
index 571d66c..5d2c3a6 100644
--- a/plat/linuxu/include/linuxu/setup.h
+++ b/plat/linuxu/include/linuxu/setup.h
@@ -38,11 +38,14 @@
#include <sys/types.h>
+struct liblinuxuplat_memregion {
+ void *base;
+ size_t len;
+};
+
struct liblinuxuplat_opts {
- struct {
- void *base;
- size_t len;
- } heap;
+ struct liblinuxuplat_memregion heap;
+ struct liblinuxuplat_memregion initrd;
};
extern struct liblinuxuplat_opts _liblinuxuplat_opts;
diff --git a/plat/linuxu/memory.c b/plat/linuxu/memory.c
index 9b5479e..842debe 100644
--- a/plat/linuxu/memory.c
+++ b/plat/linuxu/memory.c
@@ -42,11 +42,19 @@
#include <uk/plat/memory.h>
#include <uk/libparam.h>
-#define MB2B (1024 * 1024)
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
Don't include in particular these headers. The linuxu platform is
actually really tricky in this terms. The reason is that those headers
are provided by a given libc and its API definition might be
incompatible with Linux's ones (e.g., libc could be BSD style, not Linux
style). Standard int types aren't an issue usually, but VFS ones. See
also my comments in patch 1.
+
+#define MB2B (1024 * 1024)
static __u32 heap_size = CONFIG_LINUXU_DEFAULT_HEAPMB;
UK_LIB_PARAM(heap_size, __u32);
+static const char *initrd_file = NULL;
+UK_LIB_PARAM_STR(initrd_file);
Maybe call it just initrd. This is maybe more user friendly because the
command line parameter would be shorter.
+
static int __linuxu_plat_heap_init(void)
{
void *pret;
@@ -63,32 +71,86 @@ static int __linuxu_plat_heap_init(void)
if (PTRISERR(pret)) {
rc = PTR2ERR(pret);
uk_pr_err("Failed to allocate memory for heap: %d\n",
- rc);
+ rc);
} else
_liblinuxuplat_opts.heap.base = pret;
}
return rc;
+}
+static int __linuxu_plat_initrd_init(void)
+{
+ void *pret;
+ int rc = 0;
+ struct stat file_info;
You should use a copy of `struct stat`: `struct k_stat` (see comments
patch 1).
+
+ if (initrd_file == NULL) {
+ uk_pr_debug("No initrd present.\n");
+ } else {
+ uk_pr_debug("Mapping in initrd file: %s\n", initrd_file);
+ int initrd_fd = sys_open(initrd_file, O_RDONLY, 0);
K_O_RDONLY (see patch 1)
+
+ if (initrd_fd < 0) {
+ uk_pr_err("Failed to open intrd file");
Use uk_pr_crit("Failed to open %s for initrd\n", initrd_file);
+ return -1 > + }
+
+ /**
+ * Find initrd file size
+ */
+ if (sys_fstat(initrd_fd, &file_info) < 0) {
+ uk_pr_err("sys_fstat failed for initrd file");
Use uk_pr_crit, remember newline '\n' at the end of the string.
+ close(initrd_fd);
You have to define and use sys_close(). close() is going to be mapped
internally to libvfscore. The file descriptor you got here was coming
from the Linux kernel. libvfscore isn't aware of it.
+ return -1;
+ }
+ _liblinuxuplat_opts.initrd.len = file_info.st_size;
+ /**
+ * Allocate initrd memory
+ */
+ if (_liblinuxuplat_opts.initrd.len > 0) {
+ pret = sys_mmap((void *)_liblinuxuplat_opts.heap.len,
Don't give it an address for mapping it to. Let the kernel figure out an
target address which is okay.
+ _liblinuxuplat_opts.initrd.len,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_PRIVATE, initrd_fd, 0);
+ if (PTRISERR(pret)) {
+ rc = PTR2ERR(pret);
+ uk_pr_err("Failed to allocate memory for initrd:
%d\n",
+ rc);
The mmap operation actually failed. I would put the following message:
uk_pr_crit("Failed to memory-map initrd: %d\n", rc);
+ close(initrd_fd);
sys_close()
+ return -1;
+ }
+ _liblinuxuplat_opts.initrd.base = pret;
+ } else {
+ uk_pr_err("Empty initrd file given.\n");
I would change this error message to the following info message:
uk_pr_info("Ignoring empty initrd file.\n");
I would treat the case as no initrd was given. So I would return 0
instead of a failure.
+ close(initrd_fd);
sys_close()
+ return -1;
+ }
+ }
+ return rc;
}
int ukplat_memregion_count(void)
{
static int have_heap = 0;
+ static int have_initrd = 0;
int rc = 0;
+ /*
+ * NOTE: The heap size and initrd file can be changed by a
+ * library parameter. We assume that those ones are processed
+ * by the boot library shortly before memory regions are
+ * scanned. This is why we initialize the heap here.
+ */
if (!have_heap) {
- /*
- * NOTE: The heap size can be changed by a library parameter.
- * We assume that those ones are processed by the boot library
- * shortly before memory regions are scanned. This is why
- * we initialize the heap here.
- */
rc = __linuxu_plat_heap_init();
have_heap = (rc == 0) ? 1 : 0;
}
-
- return (have_heap) ? 1 : 0;
+ if (!have_initrd) {
+ rc = __linuxu_plat_initrd_init();
+ have_initrd = (rc == 0) ? 1 : 0;
+ }
+ return have_heap + have_initrd;
}
int ukplat_memregion_get(int i, struct ukplat_memregion_desc *m)
@@ -98,20 +160,31 @@ int ukplat_memregion_get(int i, struct
ukplat_memregion_desc *m)
UK_ASSERT(m);
if (i == 0 && _liblinuxuplat_opts.heap.base) {
- m->base = _liblinuxuplat_opts.heap.base;
- m->len = _liblinuxuplat_opts.heap.len;
+ m->base = _liblinuxuplat_opts.heap.base;
+ m->len = _liblinuxuplat_opts.heap.len;
Usually, try to keep your patch minimal, this simplifies the review
process. If there is no good reason, those little changes (for instance,
indention, white spaces) just bloat your patch.
m->flags = UKPLAT_MEMRF_ALLOCATABLE;
#if CONFIG_UKPLAT_MEMRNAME
- m->name = "heap";
+ m->name = "heap";
Same here...
+#endif
+ ret = 0;
+ } else if ((i == 0 && !_liblinuxuplat_opts.heap.base
+ && _liblinuxuplat_opts.initrd.base)
+ || (i == 1 && _liblinuxuplat_opts.heap.base
+ && _liblinuxuplat_opts.initrd.base)) {
+ m->base = _liblinuxuplat_opts.initrd.base;
+ m->len = _liblinuxuplat_opts.initrd.len;
+ m->flags = UKPLAT_MEMRF_INITRD | UKPLAT_MEMRF_WRITABLE;
+#if CONFIG_UKPLAT_MEMRNAME
+ m->name = "initrd";
#endif
ret = 0;
} else {
/* invalid memory region index or no heap allocated */
- m->base = __NULL;
- m->len = 0;
+ m->base = __NULL;
+ m->len = 0;
m->flags = 0x0;
#if CONFIG_UKPLAT_MEMRNAME
- m->name = __NULL;
+ m->name = __NULL;
#endif
ret = -1;
}
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
