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.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, UNWANTED_LANGUAGE_BODY,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 B2FC9C433E0 for ; Tue, 16 Feb 2021 14:24:05 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 4AA2664E07 for ; Tue, 16 Feb 2021 14:24:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4AA2664E07 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=ddWwwj/L0Bm0+HOVoudr1SGsOnQi2uUfm2KIcImsrkc=; b=oK/OqYwmX0pjAC6D8y8WEn6EE 60mF+1/6AXaK9FLirUjmYh1a6D73ozlKGFRE4h+q5PwSpoPfQON1T5sFBGH7Tq77zYRCFtH5Yj5S5 XGfpGxjuIMmrnSjOS4gnzWyym2BDTzgdyfWo9y2MLU48S4AVG9qCpUZFhZLjV6/4eIrKRyHaeOTAI Yo6fZ773iwxLMqEWABEulCcSCwzMCZHMpcuJp0PPIfUxztMCtDyPq5im7d2E7AAdV6Zo4LxVSjqBT A/6h1BB1nIOctU5hNYCmb9of5iJXQFNwjQRqbfCwuvcIjNi+Ru0ZPWPLrZfbLDrTaksRSlGe+/Tyv wgnU5sf3w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lC1Ey-00016c-Dw; Tue, 16 Feb 2021 14:22:12 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lC1Eu-00016A-UY for linux-arm-kernel@lists.infradead.org; Tue, 16 Feb 2021 14:22:10 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E120A31B; Tue, 16 Feb 2021 06:22:00 -0800 (PST) Received: from [192.168.0.110] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E571D3F694; Tue, 16 Feb 2021 06:21:59 -0800 (PST) Subject: Re: [PATCH kvmtool 13/21] hw/serial: Refactor trap handler To: Andre Przywara , Will Deacon , Julien Thierry References: <20201210142908.169597-1-andre.przywara@arm.com> <20201210142908.169597-14-andre.przywara@arm.com> From: Alexandru Elisei Message-ID: <5c91e8f2-7bdf-4c4a-1da3-08dac79fd9a4@arm.com> Date: Tue, 16 Feb 2021 14:22:05 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20201210142908.169597-14-andre.przywara@arm.com> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210216_092209_116030_3C34E96F X-CRM114-Status: GOOD ( 23.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm-kernel@lists.infradead.org, Marc Zyngier , kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Andre, Patch looks good, nitpicks below. On 12/10/20 2:29 PM, Andre Przywara wrote: > With the planned retirement of the special ioport emulation code, we > need to provide an emulation function compatible with the MMIO prototype. > > Adjust the trap handler to use that new function, and provide shims to > implement the old ioport interface, for now. > > Signed-off-by: Andre Przywara > --- > hw/serial.c | 97 +++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 65 insertions(+), 32 deletions(-) > > diff --git a/hw/serial.c b/hw/serial.c > index b0465d99..2907089c 100644 > --- a/hw/serial.c > +++ b/hw/serial.c > @@ -242,36 +242,31 @@ void serial8250__inject_sysrq(struct kvm *kvm, char sysrq) > sysrq_pending = sysrq; > } > > -static bool serial8250_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, > - void *data, int size) > +static bool serial8250_out(struct serial8250_device *dev, struct kvm_cpu *vcpu, > + u16 offset, u8 data) > { > - struct serial8250_device *dev = ioport->priv; > - u16 offset; > bool ret = true; > - char *addr = data; > > mutex_lock(&dev->mutex); > > - offset = port - dev->iobase; > - > switch (offset) { > case UART_TX: > if (dev->lcr & UART_LCR_DLAB) { > - dev->dll = ioport__read8(data); > + dev->dll = data; > break; > } > > /* Loopback mode */ > if (dev->mcr & UART_MCR_LOOP) { > if (dev->rxcnt < FIFO_LEN) { > - dev->rxbuf[dev->rxcnt++] = *addr; > + dev->rxbuf[dev->rxcnt++] = data; > dev->lsr |= UART_LSR_DR; > } > break; > } > > if (dev->txcnt < FIFO_LEN) { > - dev->txbuf[dev->txcnt++] = *addr; > + dev->txbuf[dev->txcnt++] = data; > dev->lsr &= ~UART_LSR_TEMT; > if (dev->txcnt == FIFO_LEN / 2) > dev->lsr &= ~UART_LSR_THRE; > @@ -283,18 +278,18 @@ static bool serial8250_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port > break; > case UART_IER: > if (!(dev->lcr & UART_LCR_DLAB)) > - dev->ier = ioport__read8(data) & 0x0f; > + dev->ier = data & 0x0f; > else > - dev->dlm = ioport__read8(data); > + dev->dlm = data; > break; > case UART_FCR: > - dev->fcr = ioport__read8(data); > + dev->fcr = data; > break; > case UART_LCR: > - dev->lcr = ioport__read8(data); > + dev->lcr = data; > break; > case UART_MCR: > - dev->mcr = ioport__read8(data); > + dev->mcr = data; > break; > case UART_LSR: > /* Factory test */ > @@ -303,7 +298,7 @@ static bool serial8250_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port > /* Not used */ > break; > case UART_SCR: > - dev->scr = ioport__read8(data); > + dev->scr = data; > break; > default: > ret = false; > @@ -336,46 +331,43 @@ static void serial8250_rx(struct serial8250_device *dev, void *data) > } > } > > -static bool serial8250_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size) > +static bool serial8250_in(struct serial8250_device *dev, struct kvm_cpu *vcpu, > + u16 offset, u8 *data) > { > - struct serial8250_device *dev = ioport->priv; > - u16 offset; > bool ret = true; > > mutex_lock(&dev->mutex); > > - offset = port - dev->iobase; > - > switch (offset) { > case UART_RX: > if (dev->lcr & UART_LCR_DLAB) > - ioport__write8(data, dev->dll); > + *data = dev->dll; > else > serial8250_rx(dev, data); > break; > case UART_IER: > if (dev->lcr & UART_LCR_DLAB) > - ioport__write8(data, dev->dlm); > + *data = dev->dlm; > else > - ioport__write8(data, dev->ier); > + *data = dev->ier; > break; > case UART_IIR: > - ioport__write8(data, dev->iir | UART_IIR_TYPE_BITS); > + *data = dev->iir | UART_IIR_TYPE_BITS; > break; > case UART_LCR: > - ioport__write8(data, dev->lcr); > + *data = dev->lcr; > break; > case UART_MCR: > - ioport__write8(data, dev->mcr); > + *data = dev->mcr; > break; > case UART_LSR: > - ioport__write8(data, dev->lsr); > + *data = dev->lsr; > break; > case UART_MSR: > - ioport__write8(data, dev->msr); > + *data = dev->msr; > break; > case UART_SCR: > - ioport__write8(data, dev->scr); > + *data = dev->scr; > break; > default: > ret = false; > @@ -389,6 +381,47 @@ static bool serial8250_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, > return ret; > } > > +static void serial8250_mmio(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, > + u8 is_write, void *ptr) > +{ > + struct serial8250_device *dev = ptr; > + u8 value = 0; > + > + if (is_write) { > + value = *data; Extra space before value. > + > + serial8250_out(dev, vcpu, addr - dev->iobase, value); > + } else { > + if (serial8250_in(dev, vcpu, addr - dev->iobase, &value)) > + *data = value; > + } > +} > + > +static bool serial8250_ioport_out(struct ioport *ioport, struct kvm_cpu *vcpu, > + u16 port, void *data, int size) > +{ > + struct serial8250_device *dev = ioport->priv; > + u8 value = ioport__read8(data); > + > + serial8250_mmio(vcpu, port, &value, 1, true, dev); > + > + return true; > +} > + > +static bool serial8250_ioport_in(struct ioport *ioport, struct kvm_cpu *vcpu, > + u16 port, void *data, int size) > +{ > + struct serial8250_device *dev = ioport->priv; > + u8 value = 0; > + > + > + serial8250_mmio(vcpu, port, &value, 1, false, dev); > + > + ioport__write8(data, value); This is correct, but confusing. You pass the address of a local variable as *data to serial8250_mmio, serial8250_mmio conditionally updates the value at data (which is &value from here), and then here we update the *data unconditionally. Why not pass data directly to serial8250_mmio and skip the local variable? Am I missing something? Thanks, Alex > + > + return true; > +} > + > #ifdef CONFIG_HAS_LIBFDT > > char *fdt_stdout_path = NULL; > @@ -427,8 +460,8 @@ void serial8250_generate_fdt_node(void *fdt, struct device_header *dev_hdr, > #endif > > static struct ioport_operations serial8250_ops = { > - .io_in = serial8250_in, > - .io_out = serial8250_out, > + .io_in = serial8250_ioport_in, > + .io_out = serial8250_ioport_out, > }; > > static int serial8250__device_init(struct kvm *kvm, _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel