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 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 67AD5C4332F for ; Mon, 6 Nov 2023 14:13:14 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r00LF-00062H-I4; Mon, 06 Nov 2023 09:12:37 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r00LD-000620-US for qemu-devel@nongnu.org; Mon, 06 Nov 2023 09:12:36 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r00LC-0003TE-1A for qemu-devel@nongnu.org; Mon, 06 Nov 2023 09:12:35 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699279952; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=7jK1Nxg3c3H7AngHacIf8ZSjq97OjxP4Qxp0sKBH3ZE=; b=ChpuBzSoEWRFQjKeSqymAJh4m1GMKfKMfdEs3VFRMOMMnXr5snKR4epiZC0IUlPh8apsXr KjFaX7/60xQctvD7bJkiaN1ce5Q3mHQ/a0ZIXMW/73a1ZlVk6LoflOA2mJlLmtFW8026ti BdC8Of8uy0MMvHeIrj/x9N1xOMbcWd4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-644-6FZCcC44MEymkkMTHzJAYg-1; Mon, 06 Nov 2023 09:12:29 -0500 X-MC-Unique: 6FZCcC44MEymkkMTHzJAYg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3A0DD101A52D; Mon, 6 Nov 2023 14:12:29 +0000 (UTC) Received: from redhat.com (unknown [10.39.195.35]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2D19C2026D66; Mon, 6 Nov 2023 14:12:28 +0000 (UTC) Date: Mon, 6 Nov 2023 15:12:27 +0100 From: Kevin Wolf To: Mark Cave-Ayland Cc: jsnow@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, balaton@eik.bme.hu, philmd@linaro.org, shentey@gmail.com Subject: Re: [PATCH v2 1/3] ide/pci.c: introduce pci_ide_update_mode() function Message-ID: References: <20231024224056.842607-1-mark.cave-ayland@ilande.co.uk> <20231024224056.842607-2-mark.cave-ayland@ilande.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231024224056.842607-2-mark.cave-ayland@ilande.co.uk> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 Received-SPF: pass client-ip=170.10.133.124; envelope-from=kwolf@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben: > This function reads the value of the PCI_CLASS_PROG register for PCI IDE > controllers and configures the PCI BARs and/or IDE ioports accordingly. > > In the case where we switch to legacy mode, the PCI BARs are set to return zero > (as suggested in the "PCI IDE Controller" specification), the legacy IDE ioports > are enabled, and the PCI interrupt pin cleared to indicate legacy IRQ routing. > > Conversely when we switch to native mode, the legacy IDE ioports are disabled > and the PCI interrupt pin set to indicate native IRQ routing. The contents of > the PCI BARs are unspecified, but this is not an issue since if a PCI IDE > controller has been switched to native mode then its BARs will need to be > programmed. > > Signed-off-by: Mark Cave-Ayland > Tested-by: BALATON Zoltan > Tested-by: Bernhard Beschow > --- > hw/ide/pci.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ > include/hw/ide/pci.h | 1 + > 2 files changed, 91 insertions(+) > > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index a25b352537..5be643b460 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = { > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > +static const MemoryRegionPortio ide_portio_list[] = { > + { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write }, > + { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew }, > + { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel }, > + PORTIO_END_OF_LIST(), > +}; > + > +static const MemoryRegionPortio ide_portio2_list[] = { > + { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write }, > + PORTIO_END_OF_LIST(), > +}; This is duplicated from hw/ide/ioport.c. I think it would be better to use the arrays already defined there, ideally by calling ioport.c functions to setup and release the I/O ports. > +void pci_ide_update_mode(PCIIDEState *s) > +{ > + PCIDevice *d = PCI_DEVICE(s); > + uint8_t mode = d->config[PCI_CLASS_PROG]; > + > + switch (mode & 0xf) { > + case 0xa: > + /* Both channels legacy mode */ Why is it ok to handle only the case where both channels are set to the same mode? The spec describes mixed-mode setups, too, and doesn't seem to allow ignoring a mode change if it's only for one of the channels. > + > + /* Zero BARs */ > + pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0); > + pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0); > + pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0); > + pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0); Here I'm not sure what the spec really implies. Disabling the BAR (i.e. making it read-only and returning 0) while in compatibility mode doesn't necessarily mean that the value of the register is lost. In other words, are we sure that a driver can't expect that the old value is still there when it re-enables native mode? > + /* Clear interrupt pin */ > + pci_config_set_interrupt_pin(d->config, 0); Unlike for the BARs, I don't see anything in the spec that allows disabling this byte. I doubt it hurts in practice, but did you see any drivers requiring this? According to the spec, we just must not use the PCI interrupt in compatbility mode, but the registers stay accessible. As far as I can see, the whole PCI interrupt configuration is currently unused anyway, and nothing in this series seems to change it. So won't we incorrectly continue to use the legacy interrupt even in native mode? (Actually, cmd646 seems to get it wrong the other way around and uses the PCI interrupt even in compatibility mode.) I think this means that BMDMAState needs to have two irq lines (a legacy and a PCI one) and select the right one in bmdma_irq() depending on which mode we're in currently. > + /* Add legacy IDE ports */ > + if (!s->bus[0].portio_list.owner) { > + portio_list_init(&s->bus[0].portio_list, OBJECT(d), > + ide_portio_list, &s->bus[0], "ide"); > + portio_list_add(&s->bus[0].portio_list, > + pci_address_space_io(d), 0x1f0); > + } > + > + if (!s->bus[0].portio2_list.owner) { > + portio_list_init(&s->bus[0].portio2_list, OBJECT(d), > + ide_portio2_list, &s->bus[0], "ide"); > + portio_list_add(&s->bus[0].portio2_list, > + pci_address_space_io(d), 0x3f6); > + } > + > + if (!s->bus[1].portio_list.owner) { > + portio_list_init(&s->bus[1].portio_list, OBJECT(d), > + ide_portio_list, &s->bus[1], "ide"); > + portio_list_add(&s->bus[1].portio_list, > + pci_address_space_io(d), 0x170); > + } > + > + if (!s->bus[1].portio2_list.owner) { > + portio_list_init(&s->bus[1].portio2_list, OBJECT(d), > + ide_portio2_list, &s->bus[1], "ide"); > + portio_list_add(&s->bus[1].portio2_list, > + pci_address_space_io(d), 0x376); > + } > + break; This is essentially ide_init_ioport(), except that it handles the case where it is already initialised. Let's reuse it. > + case 0xf: > + /* Both channels native mode */ > + > + /* Set interrupt pin */ > + pci_config_set_interrupt_pin(d->config, 1); > + > + /* Remove legacy IDE ports */ > + if (s->bus[0].portio_list.owner) { > + portio_list_del(&s->bus[0].portio_list); > + portio_list_destroy(&s->bus[0].portio_list); > + } > + > + if (s->bus[0].portio2_list.owner) { > + portio_list_del(&s->bus[0].portio2_list); > + portio_list_destroy(&s->bus[0].portio2_list); > + } > + > + if (s->bus[1].portio_list.owner) { > + portio_list_del(&s->bus[1].portio_list); > + portio_list_destroy(&s->bus[1].portio_list); > + } > + > + if (s->bus[1].portio2_list.owner) { > + portio_list_del(&s->bus[1].portio2_list); > + portio_list_destroy(&s->bus[1].portio2_list); > + } > + break; And this part could be an ioport.c function as well. > + } > +} Kevin