| Message ID | 20230304010632.2127470-22-quic_eberman@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Drivers for gunyah hypervisor | expand |
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 <quic_pheragu@quicinc.com> > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> 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 > --- > 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); . . .
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; + 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, + }; + 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); + +int gh_vm_add_io_handler(struct gh_vm *ghvm, struct gh_vm_io_handler *io_hdlr) +{ + struct rb_node *found; + + if (io_hdlr->datamatch && (!io_hdlr->len || io_hdlr->len > sizeof(io_hdlr->data))) + return -EINVAL; + + down_write(&ghvm->mmio_handler_lock); + found = rb_find_add(&io_hdlr->node, &ghvm->mmio_handler_root, gh_vm_io_handler_compare); + up_write(&ghvm->mmio_handler_lock); + + return found ? -EEXIST : 0; +} +EXPORT_SYMBOL_GPL(gh_vm_add_io_handler); + +void gh_vm_remove_io_handler(struct gh_vm *ghvm, struct gh_vm_io_handler *io_hdlr) +{ + down_write(&ghvm->mmio_handler_lock); + rb_erase(&io_hdlr->node, &ghvm->mmio_handler_root); + up_write(&ghvm->mmio_handler_lock); +} +EXPORT_SYMBOL_GPL(gh_vm_remove_io_handler); + static int gh_vm_rm_notification_status(struct gh_vm *ghvm, void *data) { struct gh_rm_vm_status_payload *payload = data; diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h index 18d0e1effd25..9c1046af80ed 100644 --- a/drivers/virt/gunyah/vm_mgr.h +++ b/drivers/virt/gunyah/vm_mgr.h @@ -55,6 +55,8 @@ struct gh_vm { struct mutex resources_lock; struct list_head resources; struct list_head resource_tickets; + struct rb_root mmio_handler_root; + struct rw_semaphore mmio_handler_lock; }; int gh_vm_mem_alloc(struct gh_vm *ghvm, struct gh_userspace_memory_region *region); @@ -63,4 +65,6 @@ int gh_vm_mem_free(struct gh_vm *ghvm, u32 label); struct gh_vm_mem *gh_vm_mem_find_by_label(struct gh_vm *ghvm, u32 label); struct gh_vm_mem *gh_vm_mem_find_by_addr(struct gh_vm *ghvm, u64 guest_phys_addr, u32 size); +int gh_vm_mmio_write(struct gh_vm *ghvm, u64 addr, u32 len, u64 data); + #endif diff --git a/include/linux/gunyah_vm_mgr.h b/include/linux/gunyah_vm_mgr.h index 01b1761b5923..2dbf5e5f4037 100644 --- a/include/linux/gunyah_vm_mgr.h +++ b/include/linux/gunyah_vm_mgr.h @@ -84,4 +84,29 @@ struct gh_vm_resource_ticket { int gh_vm_add_resource_ticket(struct gh_vm *ghvm, struct gh_vm_resource_ticket *ticket); void gh_vm_remove_resource_ticket(struct gh_vm *ghvm, struct gh_vm_resource_ticket *ticket); +/* + * gh_vm_io_handler contains the info about an io device and its associated + * addr and the ops associated with the io device. + */ +struct gh_vm_io_handler { + struct rb_node node; + u64 addr; + + bool datamatch; + u8 len; + u64 data; + struct gh_vm_io_handler_ops *ops; +}; + +/* + * gh_vm_io_handler_ops contains function pointers associated with an iodevice. + */ +struct gh_vm_io_handler_ops { + int (*read)(struct gh_vm_io_handler *io_dev, u64 addr, u32 len, u64 data); + int (*write)(struct gh_vm_io_handler *io_dev, u64 addr, u32 len, u64 data); +}; + +int gh_vm_add_io_handler(struct gh_vm *ghvm, struct gh_vm_io_handler *io_dev); +void gh_vm_remove_io_handler(struct gh_vm *ghvm, struct gh_vm_io_handler *io_dev); + #endif