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 9FE09C433DF for ; Fri, 7 Aug 2020 21:51:22 +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 6CF072224D for ; Fri, 7 Aug 2020 21:51:22 +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="h7kqbL5Y" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6CF072224D 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 1k4AGR-0004ke-Cr; Fri, 07 Aug 2020 21:50:59 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1k4AGQ-0004kZ-EI for xen-devel@lists.xenproject.org; Fri, 07 Aug 2020 21:50:58 +0000 X-Inumbo-ID: ffb5e211-c1d8-426a-9af0-6e8b40d0b556 Received: from mail.kernel.org (unknown [198.145.29.99]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id ffb5e211-c1d8-426a-9af0-6e8b40d0b556; Fri, 07 Aug 2020 21:50:57 +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 3742B2177B; Fri, 7 Aug 2020 21:50:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1596837056; bh=CG9QwhsLJfw504BN17t/b6iO0h5SQDJspA7AuuEQLzE=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=h7kqbL5Y5/HQcE/JUcQ320EUR2zVtfQQjXwZXHyGFzmg8VqZgbuoXUU7rZwm3Bu3Z wJUtq78ZjrKyN7vYfoN6y7suSCmz3kcptDBqm6PC9sBhdM2h+/vIpcgI3Vg7vq4Mdd KKc9YAkATqT2L/+E/C+vG0e0onh93BsnczQ6NfUs= Date: Fri, 7 Aug 2020 14:50:55 -0700 (PDT) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-T480s To: Jan Beulich Subject: Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op In-Reply-To: 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> 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 , Julien Grall , Wei Liu , Andrew Cooper , Ian Jackson , George Dunlap , Oleksandr Tyshchenko , Oleksandr Tyshchenko , Julien Grall , xen-devel@lists.xenproject.org, Volodymyr Babchuk Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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. 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 a rising or falling.