From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A0C4FC5475B for ; Fri, 8 Mar 2024 09:45:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=VfHAqNS5yP4jTiZIRhF0chWtAMqSitp7PSdBXgCwL1U=; b=rh7yKbvdEAAGYD zlBphV6ec3ryycFmIB/L1faOWvX3YPbrk7Y9hmbj5ggFvj207uTWYIdg2k0h8+hSsZRuWfWhhN1FG 8ZrbilY4jcXd413utIWDolcCZV62v4Getv3hW8PdM+n71lHkslxOB04zDFcRjfWNISWdqyQwm3kcQ B0kcWpUPZU73WPsVOHfJr4A0MHhm/DWf8tMSR0hEogu+ZUhRJ1fXTpQzd9q62owxVzLTWhNQX7bSa hsUWbKIc7xyu12Jdti75BEKxglaTp8ql8+CLQWACuS3capT4Jy+pbQqtjxEMPeHpwB8rDZn8kE7cI Mni7QJlGXum6oIim6hiw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1riWma-00000008cLR-0kvv; Fri, 08 Mar 2024 09:44:52 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1riWmV-00000008cJ6-4Avb for linux-arm-kernel@lists.infradead.org; Fri, 08 Mar 2024 09:44:50 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 27D7DCE282D; Fri, 8 Mar 2024 09:44:46 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2CAF4C433F1; Fri, 8 Mar 2024 09:44:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1709891085; bh=dhCRHUOf+No5thmyInV2kItMPlwlkJ39kGLp27sVq+s=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BjKtxVRaIV/9UKxC/7pMrd8ucfWvFjfyVCEM2bwcSPO+4LK0Z5DboJ8GDP0K4YG+E CblwNV1U9CoVUAmW0MSfaqcmTSFD0HZ5RcEBqj6w78dUmyrCMMYyPpkwYYZ95hOfow 5dG1EDmI7UQjmOWUP/1SEjGHictZ6OKyc3lCR0raZcaA2Hzkxn8SC7iEm7bMQ5fnhi /Ih4g/5W8crbOh1hrj95YHEdBzEjp6H1BfQ42Pzt7ev7F/wZPkjEOzBXQ6JweAxa5M wrJtwQWdjiu+Dq0BSW1Eydi5jz7KvViG56q50Xiw/+rics0XLW4dwlykb6KIyJBEy/ PQATOQe6jDERw== Date: Fri, 8 Mar 2024 10:44:40 +0100 From: Lorenzo Pieralisi To: Jens Wiklander Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Sudeep Holla , Marc Bonnici , Olivier Deprez Subject: Re: [PATCH] firmware: arm_ffa: support running as a guest in a vm Message-ID: References: <20240307092132.943881-1-jens.wiklander@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240307092132.943881-1-jens.wiklander@linaro.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240308_014448_445513_CF4E0E14 X-CRM114-Status: GOOD ( 33.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Mar 07, 2024 at 10:21:32AM +0100, Jens Wiklander wrote: > Add support for running the driver in a guest to a hypervisor. The main > difference is that the notification interrupt is retrieved > with FFA_FEAT_NOTIFICATION_PENDING_INT and that > FFA_NOTIFICATION_BITMAP_CREATE doesn't need to be called. I have a couple of questions about these changes, comments below. > FFA_FEAT_NOTIFICATION_PENDING_INT gives the interrupt the hypervisor has > chosen to notify its guest of pending notifications. > > Signed-off-by: Jens Wiklander > --- > drivers/firmware/arm_ffa/driver.c | 45 ++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c > index f2556a8e9401..c183c7d39c0f 100644 > --- a/drivers/firmware/arm_ffa/driver.c > +++ b/drivers/firmware/arm_ffa/driver.c > @@ -1306,17 +1306,28 @@ static void ffa_sched_recv_irq_work_fn(struct work_struct *work) > ffa_notification_info_get(); > } > > +static int ffa_get_notif_intid(int *intid) > +{ > + int ret; > + > + ret = ffa_features(FFA_FEAT_SCHEDULE_RECEIVER_INT, 0, intid, NULL); > + if (!ret) > + return 0; > + ret = ffa_features(FFA_FEAT_NOTIFICATION_PENDING_INT, 0, intid, NULL); > + if (!ret) > + return 0; I think that both interrupts should be probed in eg a host and the actions their handlers should take are different. > + > + pr_err("Failed to retrieve one of scheduler Rx or notif pending interrupts\n"); > + return ret; > +} > + > static int ffa_sched_recv_irq_map(void) > { > - int ret, irq, sr_intid; > + int ret, irq, intid; > > - /* The returned sr_intid is assumed to be SGI donated to NS world */ > - ret = ffa_features(FFA_FEAT_SCHEDULE_RECEIVER_INT, 0, &sr_intid, NULL); > - if (ret < 0) { > - if (ret != -EOPNOTSUPP) > - pr_err("Failed to retrieve scheduler Rx interrupt\n"); > + ret = ffa_get_notif_intid(&intid); > + if (ret) > return ret; > - } > > if (acpi_disabled) { > struct of_phandle_args oirq = {}; > @@ -1329,12 +1340,12 @@ static int ffa_sched_recv_irq_map(void) > > oirq.np = gic; > oirq.args_count = 1; > - oirq.args[0] = sr_intid; > + oirq.args[0] = intid; > irq = irq_create_of_mapping(&oirq); > of_node_put(gic); > #ifdef CONFIG_ACPI > } else { > - irq = acpi_register_gsi(NULL, sr_intid, ACPI_EDGE_SENSITIVE, > + irq = acpi_register_gsi(NULL, intid, ACPI_EDGE_SENSITIVE, > ACPI_ACTIVE_HIGH); > #endif This means that for both schedule receiver interrupt and notification pending interrupt we would end up calling FFA_NOTIFICATION_INFO_GET (?), which is not correct AFAIK, for many reasons. If there is a pending notification for a VM, a scheduler receiver interrupt is triggered in the host. This would end up calling FFA_NOTIFICATION_INFO_GET(), that is destructive (calling it again in the notified *guest* - in the interrupt handler triggered by the hypervisor - would not return the pending notifications for it). Therefore, the action for the pending notification interrupt should be different and should just call FFA_NOTIFICATION_GET. Please let me know if that matches your understanding, this hunk is unclear to me. > } > @@ -1442,17 +1453,15 @@ static void ffa_notifications_setup(void) > int ret, irq; > > ret = ffa_features(FFA_NOTIFICATION_BITMAP_CREATE, 0, NULL, NULL); > - if (ret) { > - pr_info("Notifications not supported, continuing with it ..\n"); > - return; > - } > + if (!ret) { > > - ret = ffa_notification_bitmap_create(); > - if (ret) { > - pr_info("Notification bitmap create error %d\n", ret); > - return; > + ret = ffa_notification_bitmap_create(); > + if (ret) { > + pr_err("notification_bitmap_create error %d\n", ret); > + return; > + } > + drv_info->bitmap_created = true; > } > - drv_info->bitmap_created = true; This boils down to saying that FFA_NOTIFICATION_BITMAP_CREATE is not implemented for a VM (because the hypervisor should take care of issuing that call before the VM is created), so if the feature is not present that does not mean that notifications aren't supported. It is just about removing a spurious log. Is that correct ? Thanks, Lorenzo > > irq = ffa_sched_recv_irq_map(); > if (irq <= 0) { > -- > 2.34.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel