Re: [PULL 68/86] hw/ide/piix: Ignore writes of hardwired PCI command register bits

From: "Michael S. Tsirkin" <mst@redhat.com>
To: qemu-devel@nongnu.org
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Lev Kujawski" <lkujaw@mailbox.org>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Yanan Wang" <wangyanan55@huawei.com>,
	"John Snow" <jsnow@redhat.com>, "Thomas Huth" <thuth@redhat.com>,
	"Laurent Vivier" <lvivier@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	qemu-block@nongnu.org
Subject: Re: [PULL 68/86] hw/ide/piix: Ignore writes of hardwired PCI command register bits
Date: Fri, 4 Nov 2022 12:36:31 -0400	[thread overview]
Message-ID: <20221104123545-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20221031124928.128475-69-mst@redhat.com>

On Mon, Oct 31, 2022 at 08:53:57AM -0400, Michael S. Tsirkin wrote:
> From: Lev Kujawski <lkujaw@mailbox.org>
> 
> One method to enable PCI bus mastering for IDE controllers, often used
> by x86 firmware, is to write 0x7 to the PCI command register.  Neither
> the PIIX 3/4 specifications nor actual PIIX 3 hardware (a Tyan S1686D
> system) permit setting the Memory Space Enable (MSE) bit, thus the
> command register would be left in an unspecified state without this
> patch.
> 
> * hw/core/machine.c
>   Facilitate migration by not masking writes to the PIIX 3/4 PCICMD
>   register for machine states of QEMU versions prior to 7.2.
> * hw/ide/piix.c
>   a) Add a reference to the PIIX 4 data sheet.
>   b) Mask the MSE bit using the QEMU PCI device wmask field.
>   c) Define a new boolean property, x-filter-pcicmd, to control
>      whether the write mask is enabled and enable it by default
>      for both the PIIX 3 and PIIX 4 IDE controllers.
> * include/hw/ide/pci.h
>   Add the boolean filter_pcicmd field to the PCIIDEState structure,
>   because the PIIX IDE controllers do not define their own state.
> * tests/qtest/ide-test.c
>   Use the command_disabled field of the QPCIDevice testing model to
>   indicate that PCI_COMMAND_MEMORY is hardwired within PIIX 3/4 IDE
>   controllers.
> 
> Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>
> Message-Id: <20221024094621.512806-3-lkujaw@mailbox.org>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


Caused test asserts reported by Stefan. Dropped for now,
pls figure the issue out and resubmit. Thanks!
> ---
>  include/hw/ide/pci.h   |  1 +
>  hw/core/machine.c      |  2 ++
>  hw/ide/piix.c          | 24 ++++++++++++++++++++++++
>  tests/qtest/ide-test.c |  1 +
>  4 files changed, 28 insertions(+)
> 
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..5424b00a9e 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -53,6 +53,7 @@ struct PCIIDEState {
>      MemoryRegion bmdma_bar;
>      MemoryRegion cmd_bar[2];
>      MemoryRegion data_bar[2];
> +    bool filter_pcicmd; /* used only for piix3/4 */
>  };
>  
>  static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 907fa78ff0..65fdfe2fed 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -42,6 +42,8 @@
>  
>  GlobalProperty hw_compat_7_1[] = {
>      { "virtio-device", "queue_reset", "false" },
> +    { "piix3-ide", "x-filter-pcicmd", "false" },
> +    { "piix4-ide", "x-filter-pcicmd", "false" },
>  };
>  const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
>  
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index de1f4f0efb..a3af32e126 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -25,6 +25,8 @@
>   * References:
>   *  [1] 82371FB (PIIX) AND 82371SB (PIIX3) PCI ISA IDE XCELERATOR,
>   *      290550-002, Intel Corporation, April 1997.
> + *  [2] 82371AB PCI-TO-ISA / IDE XCELERATOR (PIIX4), 290562-001,
> + *      Intel Corporation, April 1997.
>   */
>  
>  #include "qemu/osdep.h"
> @@ -160,6 +162,21 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>      uint8_t *pci_conf = dev->config;
>      int rc;
>  
> +    /*
> +     * Mask all IDE PCI command register bits except for Bus Master
> +     * Function Enable (bit 2) and I/O Space Enable (bit 0), as the
> +     * remainder are hardwired to 0 [1, p.48] [2, p.89-90].
> +     *
> +     * NOTE: According to the PIIX3 datasheet [1], the Memory Space
> +     * Enable (MSE, bit 1) is hardwired to 1, but this is contradicted
> +     * by actual PIIX3 hardware, the datasheet itself (viz., Default
> +     * Value: 0000h), and the PIIX4 datasheet [2].
> +     */
> +    if (d->filter_pcicmd) {
> +        pci_set_word(dev->wmask + PCI_COMMAND,
> +                     PCI_COMMAND_MASTER | PCI_COMMAND_IO);
> +    }
> +
>      pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>  
>      bmdma_setup_bar(d);
> @@ -185,6 +202,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> +static Property pci_piix_ide_properties[] = {
> +    DEFINE_PROP_BOOL("x-filter-pcicmd", PCIIDEState, filter_pcicmd, TRUE),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>  static void piix3_ide_class_init(ObjectClass *klass, void *data)
>  {
> @@ -198,6 +220,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +    device_class_set_props(dc, pci_piix_ide_properties);
>      dc->hotpluggable = false;
>  }
>  
> @@ -220,6 +243,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +    device_class_set_props(dc, pci_piix_ide_properties);
>      dc->hotpluggable = false;
>  }
>  
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index dbe1563b23..d5cec7c14c 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -174,6 +174,7 @@ static QPCIDevice *get_pci_device(QTestState *qts, QPCIBar *bmdma_bar,
>  
>      *ide_bar = qpci_legacy_iomap(dev, IDE_BASE);
>  
> +    dev->command_disabled = PCI_COMMAND_MEMORY;
>      qpci_device_enable(dev);
>  
>      return dev;
> -- 
> MST
>