|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH RFC v2 3/7] plat/common: Introduce new platform bus
Hi Justin,
Thanks fort he patch. Please find my comments inline.
Best,
Santiago
On 23.10.19, 05:52, "Minios-devel on behalf of Jia He"
<minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx on behalf of justin.he@xxxxxxx>
wrote:
platform bus is a simple bus interface for many devices, including
virtio_mmio device on arm64
Signed-off-by: Jia He <justin.he@xxxxxxx>
---
plat/common/include/platform_bus.h | 103 +++++++++++++
plat/common/platform_bus.c | 237 +++++++++++++++++++++++++++++
2 files changed, 340 insertions(+)
create mode 100644 plat/common/include/platform_bus.h
create mode 100644 plat/common/platform_bus.c
diff --git a/plat/common/include/platform_bus.h
b/plat/common/include/platform_bus.h
new file mode 100644
index 0000000..d0c14f2
--- /dev/null
+++ b/plat/common/include/platform_bus.h
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Jia He <justin.he@xxxxxxx>
+ *
+ * Copyright (c) 2018, Arm Ltd. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+
+#ifndef __UKPLAT_COMMON_PF_BUS_H__
+#define __UKPLAT_COMMON_PF_BUS_H__
+
+#include <uk/bus.h>
+#include <uk/alloc.h>
+
+/**
+ * A structure describing an ID for a Platform driver. Each driver
provides a
+ * table of these IDs for each device that it supports.
+ */
+#define PLATFORM_DEVICE_ID_START (0x100)
+#define VIRTIO_MMIO_ID PLATFORM_DEVICE_ID_START
+#define PLATFORM_DEVICE_ID_END (PLATFORM_DEVICE_ID_START + 0x100)
+
+#define UK_MAX_VIRTIO_MMIO_DEVICE (0x2)
+
+struct pf_device_id {
+ uint16_t device_id;
+};
+
+struct pf_device;
+
+typedef int (*pf_driver_add_func_t)(struct pf_device *);
+typedef int (*pf_driver_init_func_t)(struct uk_alloc *a);
+
+struct pf_driver {
+ UK_TAILQ_ENTRY(struct pf_driver) next;
+ const struct pf_device_id *device_ids;
+ pf_driver_init_func_t init; /* optional */
+ pf_driver_add_func_t add_dev;
+};
+UK_TAILQ_HEAD(pf_driver_list, struct pf_driver);
+
+enum pf_device_state {
+ PF_DEVICE_STATE_RESET = 0,
+ PF_DEVICE_STATE_RUNNING
+};
+
+struct pf_device {
+ UK_TAILQ_ENTRY(struct pf_device) next; /**< used by pf_bus_handler */
+ struct pf_device_id id;
+ struct pf_driver *drv;
+ enum pf_device_state state;
+
+ uint64_t base;
+ unsigned long irq;
+};
+UK_TAILQ_HEAD(pf_device_list, struct pf_device);
+
+#define PF_REGISTER_DRIVER(b) \
+ _PF_REGISTER_DRIVER(__LIBNAME__, b)
+
+#define _PF_REGFNNAME(x, y) x##y
+
+#define PF_REGISTER_CTOR(CTOR) \
+ UK_CTOR_FUNC(2, CTOR)
+
+#define _PF_REGISTER_DRIVER(libname, b) \
+ static void \
+ _PF_REGFNNAME(libname, _pf_register_driver)(void) \
+ { \
+ _pf_register_driver((b)); \
+ } \
+ PF_REGISTER_CTOR(_PF_REGFNNAME(libname, _pf_register_driver))
+
+/* Do not use this function directly: */
+void _pf_register_driver(struct pf_driver *drv);
+
+#endif /* __UKPLAT_COMMON_PF_BUS_H__ */
diff --git a/plat/common/platform_bus.c b/plat/common/platform_bus.c
new file mode 100644
index 0000000..6e6d61f
--- /dev/null
+++ b/plat/common/platform_bus.c
@@ -0,0 +1,237 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Jia He <justin.he@xxxxxxx>
+ *
+ * Copyright (c) 2018, Arm Ltd. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+
+#include <string.h>
+#include <uk/print.h>
+#include <cpu.h>
+#include <platform_bus.h>
+#include <libfdt.h>
+#include <ofw/fdt.h>
+#include <gic/gic-v2.h>
+#include <kvm/config.h>
+
+struct pf_bus_handler {
+ struct uk_bus b;
+ struct uk_alloc *a;
+ struct pf_driver_list drv_list; /**< List of platform drivers */
+ int drv_list_initialized;
+ struct pf_device_list dev_list; /**< List of platform devices */
+};
+static struct pf_bus_handler pfh;
+
You already have a "strcut pf_device_list" defined inside "struct
pf_bus_handler". Maybe have a different name for this list of strings?
+static char *pf_device_list[] = {
+ "virtio,mmio",
+};
+
+static inline int pf_device_id_match(const struct pf_device_id *id0,
+ const struct pf_device_id *id1)
+{
+ int rc = 0;
+
+ if (id0->device_id == id1->device_id)
+ rc = 1;
+
+ return rc;
+}
+
Parameter "struct pf_device_id *id" could also be "const"
+static inline struct pf_driver *pf_find_driver(struct pf_device_id *id)
+{
+ struct pf_driver *drv;
+
+ UK_TAILQ_FOREACH(drv, &pfh.drv_list, next) {
+ if (pf_device_id_match(id, drv->device_ids)) {
+ uk_pr_info("pf driver found devid=%hu\n",
+ drv->device_ids->device_id);
+ return drv; /* driver found */
+ }
+ }
+
+ uk_pr_info("no pf driver found\n");
+
+ return NULL; /* no driver found */
+}
+
+static inline int pf_driver_add_device(struct pf_driver *drv,
+ struct pf_device_id *devid,
+ __u64 dev_base,
+ int dev_irq)
dev->irq is of type "unsigned long", however parameter "dev_irq" is of type
int. Better change the type of dev->irq to "int"? Or otherwise this dev_irq
parameter to unsigned long? Please check this variable through the series, as
there are multiple assignments like this, sometimes using "int", sometimes
"unsigned long", sometimes "unsigned int". We should use something consistent.
+{
+ struct pf_device *dev;
+ int ret;
+
+ UK_ASSERT(drv != NULL);
+ UK_ASSERT(drv->add_dev != NULL);
+
I would prefer to have "sizeof(struct pf_device)" instead of " sizeof(*dev)".
It is more explicit and easier to understand.
+ dev = (struct pf_device *) uk_calloc(pfh.a, 1, sizeof(*dev));
+ if (!dev) {
+ uk_pr_err("Platform : Failed to initialize: Out of memory!\n");
+ return -ENOMEM;
+ }
+
+ memcpy(&dev->id, devid, sizeof(dev->id));
+ uk_pr_info("pf_driver_add_device device id=%hu\n", dev->id.device_id);
+
+ dev->base = dev_base;
+ dev->irq = dev_irq;
+
+ ret = drv->add_dev(dev);
+ if (ret < 0) {
+ uk_pr_err("Platform Failed to initialize device driver\n");
+ uk_free(pfh.a, dev);
+ }
+
+ return ret;
+}
+
+static int pf_probe(void)
+{
+ struct pf_device_id devid;
+ struct pf_driver *drv;
+ int i;
+ int end_offset = -1;
+ int ret = -ENODEV;
+ const fdt32_t *prop;
+ int type, hwirq, prop_len;
+ __u64 reg_base;
+ __phys_addr dev_base;
+ int dev_irq;
+ const void *fdt = _libkvmplat_cfg.dtb;
+
+ uk_pr_info("Probe PF\n");
+
+ /* We only support virtio_mmio as a platform device here.
+ * A loop here is needed for finding drivers if more devices
+ */
+ devid.device_id = VIRTIO_MMIO_ID;
+
+ drv = pf_find_driver(&devid);
+ if (!drv) {
+ uk_pr_info("<no driver>\n");
+ return -ENODEV;
+ }
+
+ uk_pr_info("driver %p\n", drv);
+
+ /* qemu creates virtio devices in reverse order */
+ for (i = 0; i < UK_MAX_VIRTIO_MMIO_DEVICE; i++) {
+ end_offset = fdt_get_last_node_by_compatible(fdt,
+ end_offset,
+ pf_device_list[0]);
+ if (end_offset == -FDT_ERR_NOTFOUND) {
+ uk_pr_info("device not found in fdt\n");
+ goto error_exit;
+ } else {
+ prop = fdt_getprop(fdt, end_offset, "interrupts",
+ &prop_len);
+ if (!prop) {
+ uk_pr_err("irq of device not found in fdt\n");
+ goto error_exit;
+ }
+
+ type = fdt32_to_cpu(prop[0]);
+ hwirq = fdt32_to_cpu(prop[1]);
+
+ prop = fdt_getprop(fdt, end_offset, "reg", &prop_len);
+ if (!prop) {
+ uk_pr_err("reg of device not found in fdt\n");
+ goto error_exit;
+ }
+
+ /* only care about base addr, ignore the size */
+ reg_base = fdt32_to_cpu(prop[0]);
+ reg_base = reg_base << 32 | fdt32_to_cpu(prop[1]);
+ }
+
+ dev_base = reg_base;
+ dev_irq = gic_irq_translate(type, hwirq);
"dev_base" is of type "__phys_addr", while "reg_base" is unit64_t. But then
"dev_base" is passed as parameter when calling " pf_driver_add_device", however
this function expects a "uint64_t" not a "__phys_addr". Maybe dev_base is not
needed at all in this case?
+
+ ret = pf_driver_add_device(drv, &devid, dev_base, dev_irq);
+ }
+
+ return ret;
+
+error_exit:
+ return -ENODEV;
+}
+
+
+static int pf_init(struct uk_alloc *a)
+{
+ struct pf_driver *drv, *drv_next;
+ int ret = 0;
+
+ UK_ASSERT(a != NULL);
+
+ pfh.a = a;
+
+ if (!pfh.drv_list_initialized) {
+ UK_TAILQ_INIT(&pfh.drv_list);
+ pfh.drv_list_initialized = 1;
+ }
+ UK_TAILQ_INIT(&pfh.dev_list);
+
+ UK_TAILQ_FOREACH_SAFE(drv, &pfh.drv_list, next, drv_next) {
+ if (drv->init) {
+ ret = drv->init(a);
+ if (ret == 0)
+ continue;
+ uk_pr_err("Failed to initialize pf driver %p: %d\n",
+ drv, ret);
+ UK_TAILQ_REMOVE(&pfh.drv_list, drv, next);
+ }
+ }
+ return 0;
+}
+
+void _pf_register_driver(struct pf_driver *drv)
+{
+ UK_ASSERT(drv != NULL);
+ uk_pr_info("pf_register_driver %p\n", drv);
+
+ if (!pfh.drv_list_initialized) {
+ UK_TAILQ_INIT(&pfh.drv_list);
+ pfh.drv_list_initialized = 1;
+ }
+ UK_TAILQ_INSERT_TAIL(&pfh.drv_list, drv, next);
+}
+
+
+/* Register this bus driver to libukbus:
+ */
+static struct pf_bus_handler pfh = {
+ .b.init = pf_init,
+ .b.probe = pf_probe
+};
+UK_BUS_REGISTER(&pfh.b);
+
--
2.17.1
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |