Message-ID: <5b4b0c3b-0d1f-5f6c-d541-744c9a7173f8@quicinc.com>
Date:   Mon, 6 Feb 2023 19:59:30 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v9 23/27] virt: gunyah: Add IO handlers
Content-Language: en-US
To:     Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
CC:     Bjorn Andersson <quic_bjorande@quicinc.com>,
        Alex Elder <elder@linaro.org>,
        Murali Nalajala <quic_mnalajal@quicinc.com>,
        Trilok Soni <quic_tsoni@quicinc.com>,
        Carl van Schaik <quic_cvanscha@quicinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Jassi Brar <jassisinghbrar@gmail.com>,
        Sudeep Holla <sudeep.holla@arm.com>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
References: <20230120224627.4053418-1-quic_eberman@quicinc.com>
 <20230120224627.4053418-24-quic_eberman@quicinc.com>
 <20230206104637.GG332@quicinc.com>
From:   Elliot Berman <quic_eberman@quicinc.com>
In-Reply-To: <20230206104637.GG332@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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