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 X-Spam-Level: X-Spam-Status: No, score=-11.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 50F2DC433E0 for ; Tue, 11 Aug 2020 22:48:34 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0AE8C206DA for ; Tue, 11 Aug 2020 22:48:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="qmq6goTs" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0AE8C206DA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1k5d49-00049q-5p; Tue, 11 Aug 2020 22:48:21 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1k5d48-00049Z-9v for xen-devel@lists.xenproject.org; Tue, 11 Aug 2020 22:48:20 +0000 X-Inumbo-ID: d3bb426b-4776-488a-b72b-bbc9fa167df3 Received: from mail.kernel.org (unknown [198.145.29.99]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id d3bb426b-4776-488a-b72b-bbc9fa167df3; Tue, 11 Aug 2020 22:48:19 +0000 (UTC) Received: from localhost (c-67-164-102-47.hsd1.ca.comcast.net [67.164.102.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 6C1BC206DA; Tue, 11 Aug 2020 22:48:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597186099; bh=HYApckcaU1bYSPOMdjQi+3PIeVXodndk6gnv1ggGNVY=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=qmq6goTsaxUXC4slFLbjg37DXsh6O51QAm5/IInaWb6HanEPp0GRoeiUr59+1YrJT JoGqk4ThV4KmkK9ITof6T8CC/NMKxQL8E2qp5Gg6rkWBc1SKrBanhIU07WcdZt0BY2 rCJTgtrDiS7euS0RZPMbXsgUE2OvvQ7S8s2YKWd8= Date: Tue, 11 Aug 2020 15:48:17 -0700 (PDT) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-T480s To: Julien Grall Subject: Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op In-Reply-To: <97b477a9-3945-9c5d-671d-ab5cbb2d0468@xen.org> Message-ID: References: <1596478888-23030-1-git-send-email-olekstysh@gmail.com> <1596478888-23030-6-git-send-email-olekstysh@gmail.com> <00e261e0-295a-9cd8-ed11-7e3801a4eb58@xen.org> <92e2b136-8468-2877-0e8c-c13ff2a0a1fb@xen.org> <97b477a9-3945-9c5d-671d-ab5cbb2d0468@xen.org> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Stefano Stabellini , Wei Liu , Andrew Cooper , Ian Jackson , George Dunlap , Oleksandr Tyshchenko , Oleksandr Tyshchenko , Julien Grall , Jan Beulich , xen-devel , Volodymyr Babchuk , Julien Grall Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On Tue, 11 Aug 2020, Julien Grall wrote: > On 11/08/2020 00:34, Stefano Stabellini wrote: > > On Sat, 8 Aug 2020, Julien Grall wrote: > > > On Fri, 7 Aug 2020 at 22:51, Stefano Stabellini > > > wrote: > > > > > > > > On Fri, 7 Aug 2020, Jan Beulich wrote: > > > > > On 07.08.2020 01:49, Stefano Stabellini wrote: > > > > > > On Thu, 6 Aug 2020, Julien Grall wrote: > > > > > > > On 06/08/2020 01:37, Stefano Stabellini wrote: > > > > > > > > On Wed, 5 Aug 2020, Julien Grall wrote: > > > > > > > > > On 05/08/2020 00:22, Stefano Stabellini wrote: > > > > > > > > > > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote: > > > > > > > > > > > From: Oleksandr Tyshchenko > > > > > > > > > > > > > > > > > > > > > > This patch adds ability to the device emulator to notify > > > > > > > > > > > otherend > > > > > > > > > > > (some entity running in the guest) using a SPI and > > > > > > > > > > > implements Arm > > > > > > > > > > > specific bits for it. Proposed interface allows emulator > > > > > > > > > > > to set > > > > > > > > > > > the logical level of a one of a domain's IRQ lines. > > > > > > > > > > > > > > > > > > > > > > Please note, this is a split/cleanup of Julien's PoC: > > > > > > > > > > > "Add support for Guest IO forwarding to a device emulator" > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Julien Grall > > > > > > > > > > > Signed-off-by: Oleksandr Tyshchenko > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > tools/libs/devicemodel/core.c | 18 > > > > > > > > > > > ++++++++++++++++++ > > > > > > > > > > > tools/libs/devicemodel/include/xendevicemodel.h | 4 > > > > > > > > > > > ++++ > > > > > > > > > > > tools/libs/devicemodel/libxendevicemodel.map | 1 + > > > > > > > > > > > xen/arch/arm/dm.c | 22 > > > > > > > > > > > +++++++++++++++++++++- > > > > > > > > > > > xen/common/hvm/dm.c | 1 + > > > > > > > > > > > xen/include/public/hvm/dm_op.h | 15 > > > > > > > > > > > +++++++++++++++ > > > > > > > > > > > 6 files changed, 60 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/tools/libs/devicemodel/core.c > > > > > > > > > > > b/tools/libs/devicemodel/core.c > > > > > > > > > > > index 4d40639..30bd79f 100644 > > > > > > > > > > > --- a/tools/libs/devicemodel/core.c > > > > > > > > > > > +++ b/tools/libs/devicemodel/core.c > > > > > > > > > > > @@ -430,6 +430,24 @@ int xendevicemodel_set_isa_irq_level( > > > > > > > > > > > return xendevicemodel_op(dmod, domid, 1, &op, > > > > > > > > > > > sizeof(op)); > > > > > > > > > > > } > > > > > > > > > > > +int xendevicemodel_set_irq_level( > > > > > > > > > > > + xendevicemodel_handle *dmod, domid_t domid, uint32_t > > > > > > > > > > > irq, > > > > > > > > > > > + unsigned int level) > > > > > > > > > > > > > > > > > > > > It is a pity that having xen_dm_op_set_pci_intx_level and > > > > > > > > > > xen_dm_op_set_isa_irq_level already we need to add a third > > > > > > > > > > one, but from > > > > > > > > > > the names alone I don't think we can reuse either of them. > > > > > > > > > > > > > > > > > > The problem is not the name... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It is very similar to set_isa_irq_level. We could almost > > > > > > > > > > rename > > > > > > > > > > xendevicemodel_set_isa_irq_level to > > > > > > > > > > xendevicemodel_set_irq_level or, > > > > > > > > > > better, just add an alias to it so that > > > > > > > > > > xendevicemodel_set_irq_level is > > > > > > > > > > implemented by calling xendevicemodel_set_isa_irq_level. > > > > > > > > > > Honestly I am > > > > > > > > > > not sure if it is worth doing it though. Any other opinions? > > > > > > > > > > > > > > > > > > ... the problem is the interrupt field is only 8-bit. So we > > > > > > > > > would only be > > > > > > > > > able > > > > > > > > > to cover IRQ 0 - 255. > > > > > > > > > > > > > > > > Argh, that's not going to work :-( I wasn't sure if it was a > > > > > > > > good idea > > > > > > > > anyway. > > > > > > > > > > > > > > > > > > > > > > > > > It is not entirely clear how the existing subop could be > > > > > > > > > extended without > > > > > > > > > breaking existing callers. > > > > > > > > > > > > > > > > > > > But I think we should plan for not needing two calls (one to > > > > > > > > > > set level > > > > > > > > > > to 1, and one to set it to 0): > > > > > > > > > > https://marc.info/?l=xen-devel&m=159535112027405 > > > > > > > > > > > > > > > > > > I am not sure to understand your suggestion here? Are you > > > > > > > > > suggesting to > > > > > > > > > remove > > > > > > > > > the 'level' parameter? > > > > > > > > > > > > > > > > My hope was to make it optional to call the hypercall with level > > > > > > > > = 0, > > > > > > > > not necessarily to remove 'level' from the struct. > > > > > > > > > > > > > > From my understanding, the hypercall is meant to represent the > > > > > > > status of the > > > > > > > line between the device and the interrupt controller (either low > > > > > > > or high). > > > > > > > > > > > > > > This is then up to the interrupt controller to decide when the > > > > > > > interrupt is > > > > > > > going to be fired: > > > > > > > - For edge interrupt, this will fire when the line move from > > > > > > > low to high (or > > > > > > > vice versa). > > > > > > > - For level interrupt, this will fire when line is high > > > > > > > (assuming level > > > > > > > trigger high) and will keeping firing until the device decided to > > > > > > > lower the > > > > > > > line. > > > > > > > > > > > > > > For a device, it is common to keep the line high until an OS wrote > > > > > > > to a > > > > > > > specific register. > > > > > > > > > > > > > > Furthermore, technically, the guest OS is in charge to configure > > > > > > > how an > > > > > > > interrupt is triggered. Admittely this information is part of the > > > > > > > DT, but > > > > > > > nothing prevent a guest to change it. > > > > > > > > > > > > > > As side note, we have a workaround in Xen for some buggy DT (see > > > > > > > the arch > > > > > > > timer) exposing the wrong trigger type. > > > > > > > > > > > > > > Because of that, I don't really see a way to make optional. Maybe > > > > > > > you have > > > > > > > something different in mind? > > > > > > > > > > > > For level, we need the level parameter. For edge, we are only > > > > > > interested > > > > > > in the "edge", right? > > > > > > > > > > I don't think so, unless Arm has special restrictions. Edges can be > > > > > both rising and falling ones. > > > > > > > > And the same is true for level interrupts too: they could be active-low > > > > or active-high. > > > > > > > > > > > > Instead of modelling the state of the line, which seems to be a bit > > > > error prone especially in the case of a single-device emulator that > > > > might not have enough information about the rest of the system (it might > > > > not know if the interrupt is active-high or active-low), we could model > > > > the triggering of the interrupt instead. > > > > > > I am not sure to understand why the single (or event multiple) device > > > emulator needs to know the trigger type. The information of the > > > trigger type of the interrupt would be described in the firmware table > > > and it is expected to be the same as what the emulator expects. > > > > > > If the guest OS decided to configure wrongly the interrupt trigger > > > type, then it may not work properly. But, from my understanding, this > > > doesn't differ from the HW behavior. > > > > > > > > > > > In the case of level=1, it would mean that the interrupt line is active, > > > > no matter if it is active-low or active-high. In the case of level=0, it > > > > would mean that it is inactive. > > > > > > > > Similarly, in the case of an edge interrupt edge=1 or level=1 would mean > > > > that there is an edge, no matter if it is rising or falling. > > > > > > TBH, I think your approach is only going to introduce more headache in > > > Xen if a guest OS decides to change the trigger type. > > > > > > It feels much easier to just ask the emulator to let us know the level > > > of the line. Then if the guest OS decides to change the trigger type, > > > we only need to resample the line. > > > > Emulators, at least the ones in QEMU, don't model the hardware so > > closely to care about trigger type. The only thing they typically care > > about is to fire a notification. > > I don't think I agree with this. Devices in QEMU will set the level (high or > low) of the line. This is then up to the interrupt controller to decide how to > act with it. See the function qemu_set_irq(). > > In the case of active-high level interrupt, the interrupt would fire until the > line has been lowered. > > > > > The trigger type only comes into the picture when there is a bug or a > > disagreement between Xen and QEMU. Imagine a device that can be both > > level active-high or active-low, if the guest kernel changes the > > configuration, Xen would know about it, but QEMU wouldn't. > > Lets take a step back. From my understanding, on real HW, the OS will have to > configure the device *and* the interrupt controller in order to switch from > level active-low to level active-high. Otherwise, there would be discrepancy > between the two. > > In our situation, Xen is basically the interrupt controller and QEMU the > device. So both should be aware of any change here. Did I miss anything? What you wrote looks correct. So now I wonder how they went out of sync that time. Maybe it was something x86 specific and cannot happen on ARM? Or maybe just a bug in the interrupt controller emulator or QEMU. > > I vaguely > > recall a bug 10+ years ago about this with QEMU on x86 and a line that > > could be both active-high and active-low. So QEMU would raise the > > interrupt but Xen would actually think that QEMU stopped the interrupt. > > > > To do this right, we would have to introduce an interface between Xen > > and QEMU to propagate the trigger type. Xen would have to tell QEMU when > > the guest changed the configuration. That would work, but it would be > > better if we can figure out a way to do without it to reduce complexity. > Per above, I don't think this is necessary. > > > > > Instead, given that QEMU and other emulators don't actually care about > > active-high or active-low, if we have a Xen interface that just says > > "fire the interrupt" we get away from this kind of troubles. It would > > also be more efficient because the total number of hypercalls required > > would be lower. > > I read "fire interrupt" the interrupt as "Please generate an interrupt once". > Is it what you definition you expect? Yes, that is the idea. It would have to take into account the edge/level semantic difference: level would have "start it" and a "stop it".