Messages in this thread | | | Date | Mon, 6 Feb 2023 19:59:30 -0800 | Subject | Re: [PATCH v9 23/27] virt: gunyah: Add IO handlers | From | Elliot Berman <> |
| |
On 2/6/2023 2:46 AM, Srivatsa Vaddagiri wrote: > * Elliot Berman <quic_eberman@quicinc.com> [2023-01-20 14:46:22]: > >> +static inline bool gh_vm_io_handler_matches(struct gunyah_vm_io_handler *io_hdlr, u64 addr, >> + u64 len, u64 data) >> +{ >> + u64 mask = BIT_ULL(io_hdlr->len * BITS_PER_BYTE) - 1; >> + >> + if (io_hdlr->addr != addr) > > Isn't this test redundant (given that caller would have performed same test)? >
Done.
>> + return false; >> + >> + if (!io_hdlr->datamatch) >> + return true; >> + >> + if (io_hdlr->len != len) >> + return false; >> + >> + return (data & mask) == (io_hdlr->data & mask); >> +} >> + >> +static struct gunyah_vm_io_handler *gh_vm_mgr_find_io_hdlr(struct gunyah_vm *ghvm, u64 addr, >> + u64 len, u64 data) >> +{ >> + struct gunyah_vm_io_handler *io_hdlr = NULL; >> + struct rb_node *root = NULL; >> + >> + root = ghvm->mmio_handler_root.rb_node; >> + while (root) { >> + io_hdlr = rb_entry(root, struct gunyah_vm_io_handler, node); >> + if (addr < io_hdlr->addr) >> + root = root->rb_left; >> + else if (addr > io_hdlr->addr) >> + root = root->rb_right; >> + else if (gh_vm_io_handler_matches(io_hdlr, addr, len, data)) > > In case of handler not matching, don't we need to modify root? > Otherwise we can be stuck in infinite loop here AFAICS. >
Done.
>> + return io_hdlr; >> + } >> + return NULL; >> +} > > // snip > >> +int gh_vm_mgr_add_io_handler(struct gunyah_vm *ghvm, struct gunyah_vm_io_handler *io_hdlr) >> +{ >> + struct rb_node **root, *parent = NULL; >> + >> + if (io_hdlr->datamatch && >> + (!io_hdlr->len || io_hdlr->len > (sizeof(io_hdlr->data) * BITS_PER_BYTE))) >> + return -EINVAL; >> + >> + root = &ghvm->mmio_handler_root.rb_node; >> + while (*root) { >> + struct gunyah_vm_io_handler *curr = rb_entry(*root, struct gunyah_vm_io_handler, >> + node); >> + >> + parent = *root; >> + if (io_hdlr->addr < curr->addr) >> + root = &((*root)->rb_left); >> + else if (io_hdlr->addr > curr->addr) >> + root = &((*root)->rb_right); >> + else > > We should allow two io_handlers on the same addr, but with different data > matches I think. >
Done.
>> + return -EEXIST; >> + } >> + >> + rb_link_node(&io_hdlr->node, parent, root); >> + rb_insert_color(&io_hdlr->node, &ghvm->mmio_handler_root); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(gh_vm_mgr_add_io_handler);
| |