Re: [PATCH v9 23/27] virt: gunyah: Add IO handlers

Elliot Berman
2023-02-07T03:59:50Z

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);