Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a291399a-2889-0c4c-8524-e5d0a2da2e9e@linaro.org>
Date:   Fri, 31 Mar 2023 09:27:23 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
From:   Alex Elder <elder@linaro.org>
Subject: Re: [PATCH v11 21/26] virt: gunyah: Add IO handlers
To:     Elliot Berman <quic_eberman@quicinc.com>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Cc:     Murali Nalajala <quic_mnalajal@quicinc.com>,
        Trilok Soni <quic_tsoni@quicinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
        Carl van Schaik <quic_cvanscha@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@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>,
        Will Deacon <will@kernel.org>, Andy Gross <agross@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Jassi Brar <jassisinghbrar@gmail.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: <20230304010632.2127470-1-quic_eberman@quicinc.com>
 <20230304010632.2127470-22-quic_eberman@quicinc.com>
Content-Language: en-US
In-Reply-To: <20230304010632.2127470-22-quic_eberman@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

. . .