Re: [PATCH v11 21/26] virt: gunyah: Add IO handlers
From: Alex Elder <hidden>
Date: 2023-03-31 14:28:22
Also in:
linux-arm-msm, linux-devicetree, linux-doc, lkml
On 3/3/23 7:06 PM, Elliot Berman wrote:
Add framework for VM functions to handle stage-2 write faults from Gunyah guest virtual machines. IO handlers have a range of addresses which they apply to. Optionally, they may apply to only when the value written matches the IO handler's value. Co-developed-by: Prakruthi Deepak Heragu <redacted> Signed-off-by: Prakruthi Deepak Heragu <redacted> Signed-off-by: Elliot Berman <redacted>
Two (related) bugs and a suggestion that might help avoid adding the same problem in the future. (Or maybe I made that suggestion elsewhere? Anyway, you'll see.) -Alex
quoted hunk ↗ jump to hunk
--- drivers/virt/gunyah/vm_mgr.c | 94 +++++++++++++++++++++++++++++++++++ drivers/virt/gunyah/vm_mgr.h | 4 ++ include/linux/gunyah_vm_mgr.h | 25 ++++++++++ 3 files changed, 123 insertions(+)diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c index 0269bcdaf692..b31fac15ff45 100644 --- a/drivers/virt/gunyah/vm_mgr.c +++ b/drivers/virt/gunyah/vm_mgr.c@@ -233,6 +233,100 @@ static void gh_vm_add_resource(struct gh_vm *ghvm, struct gh_resource *ghrsc) mutex_unlock(&ghvm->resources_lock); } +static int _gh_vm_io_handler_compare(const struct rb_node *node, const struct rb_node *parent) +{ + struct gh_vm_io_handler *n = container_of(node, struct gh_vm_io_handler, node); + struct gh_vm_io_handler *p = container_of(parent, struct gh_vm_io_handler, node); + + if (n->addr < p->addr) + return -1; + if (n->addr > p->addr) + return 1; + if ((n->len && !p->len) || (!n->len && p->len)) + return 0; + if (n->len < p->len) + return -1; + if (n->len > p->len) + return 1;
The datamatch field in a gh_vm_io_handler structure is Boolean. If this is what you intend, it would be better to not treat them as integer values (i.e., don't use < and >). However I *think* what you want is to be comparing the data fields here. If so, this is a BUG. I think you should maybe use "data" in the gh_fn_ioeventfd_arg structure rather than "datamatch". And then use "datamatch" consistently as a Boolean indicating whether to do matching, and "data" to be the value used in matching.
+ if (n->datamatch < p->datamatch)
+ return -1;
+ if (n->datamatch > p->datamatch)
+ return 1;
+ return 0;
+}
+
+static int gh_vm_io_handler_compare(struct rb_node *node, const struct rb_node *parent)
+{
+ return _gh_vm_io_handler_compare(node, parent);
+}
+
+static int gh_vm_io_handler_find(const void *key, const struct rb_node *node)
+{
+ const struct gh_vm_io_handler *k = key;
+
+ return _gh_vm_io_handler_compare(&k->node, node);
+}
+
+static struct gh_vm_io_handler *gh_vm_mgr_find_io_hdlr(struct gh_vm *ghvm, u64 addr,
+ u64 len, u64 data)
+{
+ struct gh_vm_io_handler key = {
+ .addr = addr,
+ .len = len,
+ .datamatch = data,The datamatch field here is Boolean. I'm pretty sure you want to assign the data field instead, in which case, this is a BUG. If you *do* intend to treat the data assigned as Boolean, please use !!data to make this obvious.
+ };
+ struct rb_node *node;
+
+ node = rb_find(&key, &ghvm->mmio_handler_root, gh_vm_io_handler_find);
+ if (!node)
+ return NULL;
+
+ return container_of(node, struct gh_vm_io_handler, node);
+}
+
+int gh_vm_mmio_write(struct gh_vm *ghvm, u64 addr, u32 len, u64 data)
+{
+ struct gh_vm_io_handler *io_hdlr = NULL;
+ int ret;
+
+ down_read(&ghvm->mmio_handler_lock);
+ io_hdlr = gh_vm_mgr_find_io_hdlr(ghvm, addr, len, data);
+ if (!io_hdlr || !io_hdlr->ops || !io_hdlr->ops->write) {
+ ret = -ENODEV;
+ goto out;
+ }
+
+ ret = io_hdlr->ops->write(io_hdlr, addr, len, data);
+
+out:
+ up_read(&ghvm->mmio_handler_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gh_vm_mmio_write);. . . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel