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=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 AE40FC433E1 for ; Fri, 5 Jun 2020 15:18:17 +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 8BE88206A2 for ; Fri, 5 Jun 2020 15:18:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8BE88206A2 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com 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 1jhE6Z-0006Rm-IQ; Fri, 05 Jun 2020 15:17: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 1jhE6Y-0006Rh-Qt for xen-devel@lists.xenproject.org; Fri, 05 Jun 2020 15:17:58 +0000 X-Inumbo-ID: bc3f2d23-a73f-11ea-afd8-12813bfff9fa Received: from esa6.hc3370-68.iphmx.com (unknown [216.71.155.175]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id bc3f2d23-a73f-11ea-afd8-12813bfff9fa; Fri, 05 Jun 2020 15:17:57 +0000 (UTC) Authentication-Results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: D0PnSQOv/L+HoZMJ0K5wPBHnoIgB6sl+ZXbvRR+Fg38kWWX/HNk4Q0cgA+7yFRkyFBYsDDWPLw BXwz/PntUvbgkQNzYks57ubLj8+Tg/TUE4It1w6Zr7/t3XO74N8CEhKEOrd58Pt0j2EmKu7qJD Edc3xfhRxTrEGRAbvtN9uLB6eHsMT8XXWj1cd2+pj119uHTFL7UvEgrmeHsxT7P9mZwm2rZvIl usjoI79M38BumwXw0dqlddLKxMAcaAU33mEdIG3SO2yZ7Yqxz+RgXkuizV1bEcnViq0wHCJ4NE RsM= X-SBRS: 2.7 X-MesageID: 19686232 X-Ironport-Server: esa6.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.73,476,1583211600"; d="scan'208";a="19686232" Date: Fri, 5 Jun 2020 17:17:48 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Jan Beulich Subject: Re: [PATCH for-4.14 v2] x86/rtc: provide mediated access to RTC for PVH dom0 Message-ID: <20200605151748.GC660@Air-de-Roger> References: <20200605110240.52545-1-roger.pau@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) 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: xen-devel@lists.xenproject.org, Andrew Cooper , Wei Liu , paul@xen.org Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On Fri, Jun 05, 2020 at 04:44:32PM +0200, Jan Beulich wrote: > On 05.06.2020 13:02, Roger Pau Monne wrote: > > Mediated access to the RTC was provided for PVHv1 dom0 using the PV > > code paths (guest_io_{write/read}), but those accesses where never > > implemented for PVHv2 dom0. This patch provides such mediated accesses > > to the RTC for PVH dom0, just like it's provided for a classic PV > > dom0. > > > > Pull out some of the RTC logic from guest_io_{read/write} into > > specific helpers that can be used by both PV and HVM guests. The > > setup of the handlers for PVH is done in rtc_init, which is already > > used to initialize the fully emulated RTC. > > > > Without this a Linux PVH dom0 will read garbage when trying to access > > the RTC, and one vCPU will be constantly looping in > > rtc_timer_do_work. > > > > Note that such issue doesn't happen on domUs because the ACPI > > NO_CMOS_RTC flag is set in FADT, which prevents the OS from accessing > > the RTC. Also the X86_EMU_RTC flag is not set for PVH dom0, as the > > accesses are not emulated but rather forwarded to the physical > > hardware. > > > > No functional change expected for classic PV dom0. > > But there is, in whether (virtual) port 0x71 can be read/written (even > by a DomU). I'm afraid of being called guilty in splitting hair, though. Urg, OK, I realized that but considered it a harmless mistake. > > @@ -808,10 +809,43 @@ void rtc_reset(struct domain *d) > > s->pt.source = PTSRC_isa; > > } > > > > +/* RTC mediator for HVM hardware domain. */ > > +static int hw_rtc_io(int dir, unsigned int port, unsigned int size, > > + uint32_t *val) > > +{ > > + if ( dir == IOREQ_READ ) > > + *val = ~0; > > + > > + if ( size != 1 ) > > + { > > + gdprintk(XENLOG_WARNING, "bad RTC access size (%u)\n", size); > > + return X86EMUL_OKAY; > > + } > > + if ( !ioports_access_permitted(current->domain, port, port) ) > > This wants to move into the helper, such that the PV side can have > it moved too. > > > void rtc_init(struct domain *d) > > { > > RTCState *s = domain_vrtc(d); > > > > + if ( is_hardware_domain(d) ) > > + { > > + /* Hardware domain gets mediated access to the physical RTC. */ > > + register_portio_handler(d, RTC_PORT(0), 2, hw_rtc_io); > > + return; > > Any reason for this explicit return, rather than ... > > > + } > > + > > if ( !has_vrtc(d) ) > > return; > > ... making use of this one? In fact wouldn't it be more correct > to have > > if ( !has_vrtc(d) ) > { > /* Hardware domain gets mediated access to the physical RTC. */ > if ( is_hardware_domain(d) ) > register_portio_handler(d, RTC_PORT(0), 2, hw_rtc_io); > return; > } > > such that eventual (perhaps optional) enabling of vRTC for hwdom > would have it properly work without changing this function again? Right, that seems fine to me. > > --- a/xen/arch/x86/pv/emul-priv-op.c > > +++ b/xen/arch/x86/pv/emul-priv-op.c > > @@ -280,19 +280,10 @@ static uint32_t guest_io_read(unsigned int port, unsigned int bytes, > > { > > sub_data = pv_pit_handler(port, 0, 0); > > } > > - else if ( port == RTC_PORT(0) ) > > - { > > - sub_data = currd->arch.cmos_idx; > > Note how there was no permission check here. Having one or more > I/O ports that can be used to simply latch a value can, as I've > learned, be quite valuable as a debugging vehicle, and there > aren't many (if any) ports beyond this one that a PV DomU might > use for such a purpose. Arguably the value is somewhat limited > here, as the value wouldn't survive a crash, but I'd still > prefer if we could retain prior functionality. OK, as said above I considered this a harmless mistake, but seeing as you find it valuable I will make sure to keep the behavior. > > @@ -1110,6 +1111,64 @@ static unsigned long get_cmos_time(void) > > return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec); > > } > > > > +/* Helpers for guest accesses to the physical RTC. */ > > +unsigned int rtc_guest_read(unsigned int port) > > +{ > > + const struct domain *currd = current->domain; > > + unsigned long flags; > > + unsigned int data = ~0; > > + > > + ASSERT(port == RTC_PORT(0) || port == RTC_PORT(1)); > > Instead of this, how about ... > > > + if ( !ioports_access_permitted(currd, port, port) ) > > + { > > + ASSERT_UNREACHABLE(); > > + return data; > > + } > > + > > + switch ( port ) > > + { > > + case RTC_PORT(0): > > + data = currd->arch.cmos_idx; > > + break; > > + > > + case RTC_PORT(1): > > + spin_lock_irqsave(&rtc_lock, flags); > > + outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0)); > > + data = inb(RTC_PORT(1)); > > + spin_unlock_irqrestore(&rtc_lock, flags); > > + break; > > default: > ASSERT_UNREACHABLE(); > break; > > ? Sure. Thanks, Roger.