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=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 0755DC4320A for ; Tue, 31 Aug 2021 11:28:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D567B60FED for ; Tue, 31 Aug 2021 11:28:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241376AbhHaL31 (ORCPT ); Tue, 31 Aug 2021 07:29:27 -0400 Received: from mail.kernel.org ([198.145.29.99]:40226 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234065AbhHaL30 (ORCPT ); Tue, 31 Aug 2021 07:29:26 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (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 2D00B60F91; Tue, 31 Aug 2021 11:28:31 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mL1wL-008CVw-2D; Tue, 31 Aug 2021 12:28:29 +0100 Date: Tue, 31 Aug 2021 12:28:28 +0100 Message-ID: <87wno1ontv.wl-maz@kernel.org> From: Marc Zyngier To: Andre Przywara Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Alexandru Elisei , Will Deacon Subject: Re: [PATCH][kvmtool] virtio/pci: Size the MSI-X bar according to the number of MSI-X In-Reply-To: <20210831121035.6b5c993b@slackpad.fritz.box> References: <20210827115405.1981529-1-maz@kernel.org> <20210831121035.6b5c993b@slackpad.fritz.box> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: andre.przywara@arm.com, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, alexandru.elisei@arm.com, will@kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Hi Andre, On Tue, 31 Aug 2021 12:10:35 +0100, Andre Przywara wrote: > > On Fri, 27 Aug 2021 12:54:05 +0100 > Marc Zyngier wrote: > > Hi Marc, > > > Since 45d3b59e8c45 ("kvm tools: Increase amount of possible interrupts > > per PCI device"), the number of MSI-S has gone from 4 to 33. > > > > However, the corresponding storage hasn't been upgraded, and writing > > to the MSI-X table is a pretty risky business. Now that the Linux > > kernel writes to *all* MSI-X entries before doing anything else > > with the device, kvmtool dies a horrible death. > > > > Fix it by properly defining the size of the MSI-X bar, and make > > Linux great again. > > > > This includes some fixes the PBA region decoding, as well as minor > > cleanups to make this code a bit more maintainable. > > > > Signed-off-by: Marc Zyngier > > Many thanks for fixing this, it looks good to me now. Just some > questions below: > > > --- > > virtio/pci.c | 42 ++++++++++++++++++++++++++++++------------ > > 1 file changed, 30 insertions(+), 12 deletions(-) > > > > diff --git a/virtio/pci.c b/virtio/pci.c > > index eb91f512..41085291 100644 > > --- a/virtio/pci.c > > +++ b/virtio/pci.c > > @@ -7,6 +7,7 @@ > > #include "kvm/irq.h" > > #include "kvm/virtio.h" > > #include "kvm/ioeventfd.h" > > +#include "kvm/util.h" > > > > #include > > #include > > @@ -14,6 +15,13 @@ > > #include > > #include > > > > +#define ALIGN_UP(x, s) ALIGN((x) + (s) - 1, (s)) > > +#define VIRTIO_NR_MSIX (VIRTIO_PCI_MAX_VQ + VIRTIO_PCI_MAX_CONFIG) > > +#define VIRTIO_MSIX_TABLE_SIZE (VIRTIO_NR_MSIX * 16) > > +#define VIRTIO_MSIX_PBA_SIZE (ALIGN_UP(VIRTIO_MSIX_TABLE_SIZE, 64) / 8) > > +#define VIRTIO_MSIX_BAR_SIZE (1UL << fls_long(VIRTIO_MSIX_TABLE_SIZE + \ > > + VIRTIO_MSIX_PBA_SIZE)) > > + > > static u16 virtio_pci__port_addr(struct virtio_pci *vpci) > > { > > return pci__bar_address(&vpci->pci_hdr, 0); > > @@ -333,18 +341,27 @@ static void virtio_pci__msix_mmio_callback(struct kvm_cpu *vcpu, > > struct virtio_pci *vpci = vdev->virtio; > > struct msix_table *table; > > u32 msix_io_addr = virtio_pci__msix_io_addr(vpci); > > + u32 pba_offset; > > int vecnum; > > size_t offset; > > > > - if (addr > msix_io_addr + PCI_IO_SIZE) { > > Ouch, the missing "=" looks like another long standing bug you fixed, I > wonder how this ever worked before? Looking deeper it looks like the > whole PBA code was quite broken (allowing writes, for instance, and > mixing with the code for the MSIX table)? I don't think it ever worked. And to be fair, no known guest ever reads from it either. It just that as I was reworking it, some of the pitfalls became obvious. > > > + BUILD_BUG_ON(VIRTIO_NR_MSIX > (sizeof(vpci->msix_pba) * 8)); > > + > > + pba_offset = vpci->pci_hdr.msix.pba_offset & ~PCI_MSIX_TABLE_BIR; > > Any particular reason you read back the offset from the MSIX capability > instead of just using VIRTIO_MSIX_TABLE_SIZE here? Is that to avoid > accidentally diverging in the future, by having just one place of > definition? Exactly. My first version of this patch actually failed to update the offset advertised to the guest, so I decided to just have a single location for this. At least, we won't have to touch this code again if we change the number of MSI-X. > > > + if (addr >= msix_io_addr + pba_offset) { > > + /* Read access to PBA */ > > if (is_write) > > return; > > - table = (struct msix_table *)&vpci->msix_pba; > > - offset = addr - (msix_io_addr + PCI_IO_SIZE); > > - } else { > > - table = vpci->msix_table; > > - offset = addr - msix_io_addr; > > + offset = addr - (msix_io_addr + pba_offset); > > + if ((offset + len) > sizeof (vpci->msix_pba)) > > + return; > > + memcpy(data, (void *)&vpci->msix_pba + offset, len); > > Should this be a char* cast, since pointer arithmetic on void* is > somewhat frowned upon (aka "forbidden in the C standard, but allowed as > a GCC extension")? I am trying to be consistent. A quick grep shows at least 19 occurrences of pointer arithmetic with '(void *)', and none with '(char *)'. Happy for someone to go and repaint this, but I don't think this should be the purpose of this patch. Thanks, M. -- Without deviation from the norm, progress is not possible.