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=-12.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,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 80C83C433ED for ; Mon, 3 May 2021 15:59:38 +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 2714261164 for ; Mon, 3 May 2021 15:59:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2714261164 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.121742.229622 (Exim 4.92) (envelope-from ) id 1ldayh-0003hK-Os; Mon, 03 May 2021 15:59:23 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 121742.229622; Mon, 03 May 2021 15:59:23 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1ldayh-0003hD-KU; Mon, 03 May 2021 15:59:23 +0000 Received: by outflank-mailman (input) for mailman id 121742; Mon, 03 May 2021 15:59:22 +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 1ldayg-0003h4-6l for xen-devel@lists.xenproject.org; Mon, 03 May 2021 15:59:22 +0000 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id f22c889c-682e-4513-b001-5a76cd2e7cdc; Mon, 03 May 2021 15:59:20 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 23295B011; Mon, 3 May 2021 15:59:20 +0000 (UTC) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: f22c889c-682e-4513-b001-5a76cd2e7cdc X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1620057560; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3zjsJslUplCDLd2j5GYvhm/B4wkceDi5tiQCLODoQQw=; b=RpVqDnKpw/wxx5k4D6kbw3iqh/lvIXGDTRPoBKZL80VxFrWXfjB3MjLix+NSkLGwhMoi09 5DLNVFT9i3esBKdkdcb/m4JpaTisMHc635bF/t/ddIoyohU84FR9tb0F7GVsfDrvHHWuaL 2Rn2ptwZawd3eH8k+Z9GdShyoqIjIQc= Subject: Re: [PATCH v4 01/12] x86/rtc: drop code related to strict mode To: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Cc: Andrew Cooper , Wei Liu , xen-devel@lists.xenproject.org References: <20210420140723.65321-1-roger.pau@citrix.com> <20210420140723.65321-2-roger.pau@citrix.com> <5b06565e-1f2e-3498-c18f-e7eac0042761@suse.com> From: Jan Beulich Message-ID: Date: Mon, 3 May 2021 17:59:19 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 03.05.2021 17:28, Roger Pau Monné wrote: > On Mon, May 03, 2021 at 04:58:07PM +0200, Jan Beulich wrote: >> On 03.05.2021 16:47, Roger Pau Monné wrote: >>> On Mon, May 03, 2021 at 02:26:51PM +0200, Jan Beulich wrote: >>>> On 03.05.2021 11:28, Roger Pau Monné wrote: >>>>> On Thu, Apr 29, 2021 at 04:53:07PM +0200, Jan Beulich wrote: >>>>>> On 20.04.2021 16:07, Roger Pau Monne wrote: >>>>>>> @@ -337,8 +336,7 @@ int pt_update_irq(struct vcpu *v) >>>>>>> { >>>>>>> if ( pt->pending_intr_nr ) >>>>>>> { >>>>>>> - /* RTC code takes care of disabling the timer itself. */ >>>>>>> - if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) && >>>>>>> + if ( pt_irq_masked(pt) && >>>>>>> /* Level interrupts should be asserted even if masked. */ >>>>>>> !pt->level ) >>>>>>> { >>>>>> >>>>>> I'm struggling to relate this to any other part of the patch. In >>>>>> particular I can't find the case where a periodic timer would be >>>>>> registered with RTC_IRQ and a NULL private pointer. The only use >>>>>> I can find is with a non-NULL pointer, which would mean the "else" >>>>>> path is always taken at present for the RTC case (which you now >>>>>> change). >>>>> >>>>> Right, the else case was always taken because as the comment noted RTC >>>>> would take care of disabling itself (by calling destroy_periodic_time >>>>> from the callback when using strict_mode). When no_ack mode was >>>>> implemented this wasn't taken into account AFAICT, and thus the RTC >>>>> was never removed from the list even when masked. >>>>> >>>>> I think with no_ack mode the RTC shouldn't have this specific handling >>>>> in pt_update_irq, as it should behave like any other virtual timer. >>>>> I could try to split this as a separate bugfix, but then I would have >>>>> to teach pt_update_irq to differentiate between strict_mode and no_ack >>>>> mode. >>>> >>>> A fair part of my confusion was about "&& !pt->priv". >>> >>> I think you meant "|| !pt->priv"? >> >> Oops, indeed. >> >>>> I've looked back >>>> at 9607327abbd3 ("x86/HVM: properly handle RTC periodic timer even when >>>> !RTC_PIE"), where this was added. It was, afaict, to cover for >>>> hpet_set_timer() passing NULL with RTC_IRQ. >>> >>> That's tricky, as hpet_set_timer hardcodes 8 instead of using RTC_IRQ >>> which makes it really easy to miss. >>> >>>> Which makes me suspect that >>>> be07023be115 ("x86/vhpet: add support for level triggered interrupts") >>>> may have subtly broken things. >>> >>> Right - as that would have made the RTC irq when generated from the >>> HPET no longer be suspended when masked (as pt->priv would no longer >>> be NULL). Could be fixed with: >>> >>> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c >>> index ca94e8b4538..f2cbd12f400 100644 >>> --- a/xen/arch/x86/hvm/hpet.c >>> +++ b/xen/arch/x86/hvm/hpet.c >>> @@ -318,7 +318,8 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, >>> hpet_tick_to_ns(h, diff), >>> oneshot ? 0 : hpet_tick_to_ns(h, h->hpet.period[tn]), >>> irq, timer_level(h, tn) ? hpet_timer_fired : NULL, >>> - (void *)(unsigned long)tn, timer_level(h, tn)); >>> + timer_level(h, tn) ? (void *)(unsigned long)tn : NULL, >>> + timer_level(h, tn)); >>> } >>> >>> static inline uint64_t hpet_fixup_reg( >>> >>> Passing again NULL as the callback private data for edge triggered >>> interrupts. >> >> Right, plus perhaps at the same time replacing the hardcoded 8. > > Right, but if you agree to take this patch and remove strict_mode then > the emulated RTC won't disable itself anymore, and hence needs to be > handled as any other virtual timer? I'm still trying to become convinced, both of the removal of the mode in general and the particular part of the change I've been struggling with. Jan