From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v10 7/8] arm/arm64: vgic: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl Date: Mon, 23 Jan 2017 14:42:05 +0100 Message-ID: <20170123134205.GE15850@cbox> References: <1480576187-5012-1-git-send-email-vijay.kilari@gmail.com> <1480576187-5012-8-git-send-email-vijay.kilari@gmail.com> <20170120195338.GH13531@cbox> <20170123110639.GA15850@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id BCF7840296 for ; Mon, 23 Jan 2017 08:42:07 -0500 (EST) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xbtI-OyxpQmQ for ; Mon, 23 Jan 2017 08:42:06 -0500 (EST) Received: from mail-lf0-f50.google.com (mail-lf0-f50.google.com [209.85.215.50]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 783EF40194 for ; Mon, 23 Jan 2017 08:42:06 -0500 (EST) Received: by mail-lf0-f50.google.com with SMTP id n124so93359275lfd.2 for ; Mon, 23 Jan 2017 05:42:11 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Peter Maydell Cc: Marc Zyngier , Andre Przywara , Vijaya Kumar K , "kvmarm@lists.cs.columbia.edu" , arm-mail-list List-Id: kvmarm@lists.cs.columbia.edu On Mon, Jan 23, 2017 at 11:41:41AM +0000, Peter Maydell wrote: > On 23 January 2017 at 11:06, Christoffer Dall > wrote: > > ok, I think you're right that it can be done this way, but it has the > > unfortunate consequence of having to change a lot of the implementation. > > > > The reason is that we store the latch state in two different variables, > > depening on whether it's an edge- or level-triggered interrupts. > > > > We use the irq->pending field for the computed result (using above > > calculaiton) for level interrupts based on irq->line_level and > > irq->soft_pending. soft_pending is the latch state for level interrupts > > only. > > > > For edge triggered interrupts the computed result and the latch state > > are alway the same (right?) so we we only use the irq->pending field for > > that. > > > > But unless I didn't have enough coffee this morning, this only works if > > you have a-priori knowledge of which interrupts are level and which are > > edge. When this is not the case, as in the case of order-free > > save/restore, then I think the only solution is to rework the logic so > > that the soft_pending field is used for the latch state for both edge > > and level interrupts, and the pending field is just the internal > > computed value. > > I *think* you could fudge it by saying "when the config changes > from edge to level, copy the current irq->pending into irq->soft_pending". > Then you can always read the latch state (it's soft_pending > if level, otherwise pending), and writing it is "set both if > level, just irq->pending if edge". That in turn gives you enough > information I think to cope with restores of all of (config, > level, pending-latch) in any order. It feels a bit fragile, though. Right, thanks for working this out. I agree it's fragile and I cannot seem to easily convince myself it would be correct, so I sent out a patch to get rid of the pending cached state which should simplify the save/restore patches as well. Assuming the other VGIC suspects don't object to that, I suggest we base these patches on top of that one and see if we can convince ourselves that it's correct then. Thanks, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Mon, 23 Jan 2017 14:42:05 +0100 Subject: [PATCH v10 7/8] arm/arm64: vgic: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl In-Reply-To: References: <1480576187-5012-1-git-send-email-vijay.kilari@gmail.com> <1480576187-5012-8-git-send-email-vijay.kilari@gmail.com> <20170120195338.GH13531@cbox> <20170123110639.GA15850@cbox> Message-ID: <20170123134205.GE15850@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Jan 23, 2017 at 11:41:41AM +0000, Peter Maydell wrote: > On 23 January 2017 at 11:06, Christoffer Dall > wrote: > > ok, I think you're right that it can be done this way, but it has the > > unfortunate consequence of having to change a lot of the implementation. > > > > The reason is that we store the latch state in two different variables, > > depening on whether it's an edge- or level-triggered interrupts. > > > > We use the irq->pending field for the computed result (using above > > calculaiton) for level interrupts based on irq->line_level and > > irq->soft_pending. soft_pending is the latch state for level interrupts > > only. > > > > For edge triggered interrupts the computed result and the latch state > > are alway the same (right?) so we we only use the irq->pending field for > > that. > > > > But unless I didn't have enough coffee this morning, this only works if > > you have a-priori knowledge of which interrupts are level and which are > > edge. When this is not the case, as in the case of order-free > > save/restore, then I think the only solution is to rework the logic so > > that the soft_pending field is used for the latch state for both edge > > and level interrupts, and the pending field is just the internal > > computed value. > > I *think* you could fudge it by saying "when the config changes > from edge to level, copy the current irq->pending into irq->soft_pending". > Then you can always read the latch state (it's soft_pending > if level, otherwise pending), and writing it is "set both if > level, just irq->pending if edge". That in turn gives you enough > information I think to cope with restores of all of (config, > level, pending-latch) in any order. It feels a bit fragile, though. Right, thanks for working this out. I agree it's fragile and I cannot seem to easily convince myself it would be correct, so I sent out a patch to get rid of the pending cached state which should simplify the save/restore patches as well. Assuming the other VGIC suspects don't object to that, I suggest we base these patches on top of that one and see if we can convince ourselves that it's correct then. Thanks, -Christoffer