[PATCH v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[PATCH v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Lev Kujawski]

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 PIIX3 specification nor actual hardware (a Tyan S1686D system)
permit modification of the Memory Space Enable (MSE) bit, 1, and thus
the command register would be left in an unspecified state without
this patch.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
This revised patch uses QEMU's built-in PCI bit-masking support rather
than attempting to manually filter writes.  Thanks to Philippe Mathieu-
Daude and Michael S. Tsirkin for review and the pointer.

 hw/ide/piix.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 76ea8fd9f6..bd3f397de8 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,19 @@ 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 1), 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) is hardwired to 1, but this is contradicted by
+     * actual PIIX3 hardware, the datasheet itself (viz., Default
+     * Value: 0000h), and the PIIX4 datasheet [2].
+     */
+    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);
-- 
2.34.1

Re: [PATCH v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Bernhard Beschow]

Am 2. Juni 2022 20:47:31 UTC schrieb Lev Kujawski <lkujaw@member.fsf.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 PIIX3 specification nor actual hardware (a Tyan S1686D system)
>permit modification of the Memory Space Enable (MSE) bit, 1, and thus
>the command register would be left in an unspecified state without
>this patch.
>
>Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
>---
>This revised patch uses QEMU's built-in PCI bit-masking support rather
>than attempting to manually filter writes.  Thanks to Philippe Mathieu-
>Daude and Michael S. Tsirkin for review and the pointer.

Ping. Any news?

> hw/ide/piix.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
>diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>index 76ea8fd9f6..bd3f397de8 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,19 @@ 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 1), 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) is hardwired to 1, but this is contradicted by
>+     * actual PIIX3 hardware, the datasheet itself (viz., Default
>+     * Value: 0000h), and the PIIX4 datasheet [2].
>+     */
>+    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);

Re: [PATCH v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Michael S. Tsirkin]

On Thu, Jun 02, 2022 at 08:47:31PM +0000, Lev Kujawski wrote:
> 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 PIIX3 specification nor actual hardware (a Tyan S1686D system)
> permit modification of the Memory Space Enable (MSE) bit, 1, and thus
> the command register would be left in an unspecified state without
> this patch.
> 
> Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>


I don't think this is appropriate in trivial at all.



> ---
> This revised patch uses QEMU's built-in PCI bit-masking support rather
> than attempting to manually filter writes.  Thanks to Philippe Mathieu-
> Daude and Michael S. Tsirkin for review and the pointer.

But pls note I wrote:

	Might need machine compat machinery
	for this.

without said machinery, if guest set one of the other
bits, migration will fail.


>  hw/ide/piix.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 76ea8fd9f6..bd3f397de8 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,19 @@ 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 1), 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) is hardwired to 1, but this is contradicted by
> +     * actual PIIX3 hardware, the datasheet itself (viz., Default
> +     * Value: 0000h), and the PIIX4 datasheet [2].
> +     */
> +    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);
> -- 
> 2.34.1
> 
> 
> 

Re: [PATCH v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Michael S. Tsirkin]

On Tue, Sep 06, 2022 at 10:23:57AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 02, 2022 at 08:47:31PM +0000, Lev Kujawski wrote:
> > 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 PIIX3 specification nor actual hardware (a Tyan S1686D system)
> > permit modification of the Memory Space Enable (MSE) bit, 1, and thus
> > the command register would be left in an unspecified state without
> > this patch.
> > 
> > Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
> 
> 
> I don't think this is appropriate in trivial at all.
> 
> 
> 
> > ---
> > This revised patch uses QEMU's built-in PCI bit-masking support rather
> > than attempting to manually filter writes.  Thanks to Philippe Mathieu-
> > Daude and Michael S. Tsirkin for review and the pointer.
> 
> But pls note I wrote:
> 
> 	Might need machine compat machinery
> 	for this.
> 
> without said machinery, if guest set one of the other
> bits, migration will fail.

I assume v3 will be forthcoming, right?


> 
> >  hw/ide/piix.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index 76ea8fd9f6..bd3f397de8 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,19 @@ 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 1), 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) is hardwired to 1, but this is contradicted by
> > +     * actual PIIX3 hardware, the datasheet itself (viz., Default
> > +     * Value: 0000h), and the PIIX4 datasheet [2].
> > +     */
> > +    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);
> > -- 
> > 2.34.1
> > 
> > 
> > 

[PATCH v3 0/2] Re: hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Lev Kujawski]

> On Tue, Sep 06, 2022 at 10:23:57AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 02, 2022 at 08:47:31PM +0000, Lev Kujawski wrote:
> > > ---
> > > This revised patch uses QEMU's built-in PCI bit-masking support rather
> > > than attempting to manually filter writes.  Thanks to Philippe Mathieu-
> > > Daude and Michael S. Tsirkin for review and the pointer.
> >
> > But pls note I wrote:
> >
> > 	Might need machine compat machinery
> > 	for this.
> >
> > without said machinery, if guest set one of the other
> > bits, migration will fail.
>
> I assume v3 will be forthcoming, right?

Thanks for your review and my apologies for the delay.  I hope this revised
patch appropriately handles the machine state migration case.

Kind regards,
Lev Kujawski

Lev Kujawski (2):
  qpci_device_enable: Allow for command bits hardwired to 0
  hw/ide/piix: Ignore writes of hardwired PCI command register bits

 hw/ide/pci.c             |  5 +++++
 hw/ide/piix.c            | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/ide/pci.h     |  7 ++++++-
 tests/qtest/ide-test.c   |  1 +
 tests/qtest/libqos/pci.c | 13 +++++++------
 tests/qtest/libqos/pci.h |  1 +
 6 files changed, 59 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] qpci_device_enable: Allow for command bits hardwired to 0

[Lev Kujawski]

Devices like the PIIX3/4 IDE controller do not support certain modes
of operation, such as memory space accesses, and indicate this lack of
support by hardwiring the applicable bits to zero.  Extend the QEMU
PCI device testing framework to accommodate such devices.

* tests/qtest/libqos/pci.h: Add the command_disabled word to indicate
  bits hardwired to 0.
* tests/qtest/libqos/pci.c: Verify that hardwired bits are actually
  hardwired.

Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>
---
 tests/qtest/libqos/pci.c | 13 +++++++------
 tests/qtest/libqos/pci.h |  1 +
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index b23d72346b..4f3d28d8d9 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -220,18 +220,19 @@ int qpci_secondary_buses_init(QPCIBus *bus)
 
 void qpci_device_enable(QPCIDevice *dev)
 {
-    uint16_t cmd;
+    const uint16_t enable_bits =
+        PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    uint16_t cmd, new_cmd;
 
     /* FIXME -- does this need to be a bus callout? */
     cmd = qpci_config_readw(dev, PCI_COMMAND);
-    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    cmd |= enable_bits;
     qpci_config_writew(dev, PCI_COMMAND, cmd);
 
     /* Verify the bits are now set. */
-    cmd = qpci_config_readw(dev, PCI_COMMAND);
-    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
-    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
-    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
+    new_cmd = qpci_config_readw(dev, PCI_COMMAND);
+    new_cmd &= enable_bits;
+    g_assert_cmphex(new_cmd, ==, enable_bits & ~dev->command_disabled);
 }
 
 /**
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 8389614523..eaedb98588 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -68,6 +68,7 @@ struct QPCIDevice
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
+    uint16_t command_disabled;
 };
 
 struct QPCIAddress {
-- 
2.34.1

Re: [PATCH v3 1/2] qpci_device_enable: Allow for command bits hardwired to 0

[Michael S. Tsirkin]

On Sun, Sep 25, 2022 at 09:37:58AM +0000, Lev Kujawski wrote:
> Devices like the PIIX3/4 IDE controller do not support certain modes
> of operation, such as memory space accesses, and indicate this lack of
> support by hardwiring the applicable bits to zero.  Extend the QEMU
> PCI device testing framework to accommodate such devices.
> 
> * tests/qtest/libqos/pci.h: Add the command_disabled word to indicate
>   bits hardwired to 0.
> * tests/qtest/libqos/pci.c: Verify that hardwired bits are actually
>   hardwired.
> 
> Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>
> ---
>  tests/qtest/libqos/pci.c | 13 +++++++------
>  tests/qtest/libqos/pci.h |  1 +
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index b23d72346b..4f3d28d8d9 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -220,18 +220,19 @@ int qpci_secondary_buses_init(QPCIBus *bus)
>  
>  void qpci_device_enable(QPCIDevice *dev)
>  {
> -    uint16_t cmd;
> +    const uint16_t enable_bits =
> +        PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> +    uint16_t cmd, new_cmd;
>  
>      /* FIXME -- does this need to be a bus callout? */
>      cmd = qpci_config_readw(dev, PCI_COMMAND);
> -    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> +    cmd |= enable_bits;
>      qpci_config_writew(dev, PCI_COMMAND, cmd);
>  
>      /* Verify the bits are now set. */
> -    cmd = qpci_config_readw(dev, PCI_COMMAND);
> -    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
> -    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
> -    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
> +    new_cmd = qpci_config_readw(dev, PCI_COMMAND);
> +    new_cmd &= enable_bits;
> +    g_assert_cmphex(new_cmd, ==, enable_bits & ~dev->command_disabled);
>  }
>  
>  /**
> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
> index 8389614523..eaedb98588 100644
> --- a/tests/qtest/libqos/pci.h
> +++ b/tests/qtest/libqos/pci.h
> @@ -68,6 +68,7 @@ struct QPCIDevice
>      bool msix_enabled;
>      QPCIBar msix_table_bar, msix_pba_bar;
>      uint64_t msix_table_off, msix_pba_off;
> +    uint16_t command_disabled;


Can we get this from device's wmask?

>  };
>  
>  struct QPCIAddress {
> -- 
> 2.34.1

Re: [PATCH v3 1/2] qpci_device_enable: Allow for command bits hardwired to 0

[Lev Kujawski]

Michael S. Tsirkin writes:

> On Sun, Sep 25, 2022 at 09:37:58AM +0000, Lev Kujawski wrote:
>> Devices like the PIIX3/4 IDE controller do not support certain modes
>> of operation, such as memory space accesses, and indicate this lack of
>> support by hardwiring the applicable bits to zero.  Extend the QEMU
>> PCI device testing framework to accommodate such devices.
>> 
>> * tests/qtest/libqos/pci.h: Add the command_disabled word to indicate
>>   bits hardwired to 0.
>> * tests/qtest/libqos/pci.c: Verify that hardwired bits are actually
>>   hardwired.
>> 
>> Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>
>> ---
>>  tests/qtest/libqos/pci.c | 13 +++++++------
>>  tests/qtest/libqos/pci.h |  1 +
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
>> index b23d72346b..4f3d28d8d9 100644
>> --- a/tests/qtest/libqos/pci.c
>> +++ b/tests/qtest/libqos/pci.c
>> @@ -220,18 +220,19 @@ int qpci_secondary_buses_init(QPCIBus *bus)
>>  
>>  void qpci_device_enable(QPCIDevice *dev)
>>  {
>> -    uint16_t cmd;
>> +    const uint16_t enable_bits =
>> +        PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
>> +    uint16_t cmd, new_cmd;
>>  
>>      /* FIXME -- does this need to be a bus callout? */
>>      cmd = qpci_config_readw(dev, PCI_COMMAND);
>> -    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
>> +    cmd |= enable_bits;
>>      qpci_config_writew(dev, PCI_COMMAND, cmd);
>>  
>>      /* Verify the bits are now set. */
>> -    cmd = qpci_config_readw(dev, PCI_COMMAND);
>> -    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
>> -    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
>> -    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
>> +    new_cmd = qpci_config_readw(dev, PCI_COMMAND);
>> +    new_cmd &= enable_bits;
>> +    g_assert_cmphex(new_cmd, ==, enable_bits & ~dev->command_disabled);
>>  }
>>  
>>  /**
>> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
>> index 8389614523..eaedb98588 100644
>> --- a/tests/qtest/libqos/pci.h
>> +++ b/tests/qtest/libqos/pci.h
>> @@ -68,6 +68,7 @@ struct QPCIDevice
>>      bool msix_enabled;
>>      QPCIBar msix_table_bar, msix_pba_bar;
>>      uint64_t msix_table_off, msix_pba_off;
>> +    uint16_t command_disabled;
>
>
> Can we get this from device's wmask?
>

I have not seen any way to pass the wmask from the underlying PCI device
without violating the abstraction of the driver testing framework.

Another approach might be to omit the verification of the PCI command
bits in the assumption that some filtering mechanism like wmask is
active, but I think the advantage of this patch is that it makes the
expected (albeit abnormal) behavior explicit in the device test.

Kind regards,
Lev Kujawski

[PATCH v3 2/2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Lev Kujawski]

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 PIIX3 specification nor actual hardware (a Tyan S1686D system)
permit modification of the Memory Space Enable (MSE) bit, 1, and thus
the command register would be left in an unspecified state without
this patch.

* hw/ide/pci.c
  Call post_load if provided by derived IDE controller.
* hw/ide/piix.c
  a) Add references to the PIIX data sheets.
  b) Mask the MSE bit using the QEMU PCI device wmask field.
  c) Add a post_load function to mask bits from saved machine states.
  d) Specify post_load for both the PIIX3/4 IDE controllers.
* include/hw/ide/pci.h
  Switch from SIMPLE_TYPE to TYPE, explicitly create a PCIIDEClass
  that includes the post_load function pointer.
* tests/qtest/ide-test.c
  Use the command_disabled field of the QPCIDevice testing model to
  indicate that PCI_COMMAND_MEMORY is hardwired in the PIIX3/4 IDE
  controller.

Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>
---
(v2) Use QEMU's built-in PCI bit-masking support rather than attempting
     to manually filter writes.  Thanks to Philippe Mathieu-Daude and
     Michael S. Tsirkin for review and the pointer.
(v3) Handle migration of older machine states, which may have set bits
     masked by this patch, via a new post_load method of PCIIDEClass.
     Thanks to Michael S. Tsirkin for catching this via review.

 hw/ide/pci.c           |  5 +++++
 hw/ide/piix.c          | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/ide/pci.h   |  7 ++++++-
 tests/qtest/ide-test.c |  1 +
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 84ba733548..e42c7b9415 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -447,6 +447,7 @@ static const VMStateDescription vmstate_bmdma = {
 
 static int ide_pci_post_load(void *opaque, int version_id)
 {
+    PCIIDEClass *dc = PCI_IDE_GET_CLASS(opaque);
     PCIIDEState *d = opaque;
     int i;
 
@@ -457,6 +458,10 @@ static int ide_pci_post_load(void *opaque, int version_id)
         ide_bmdma_post_load(&d->bmdma[i], -1);
     }
 
+    if (dc->post_load) {
+        dc->post_load(d, version_id);
+    }
+
     return 0;
 }
 
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9a9b28078e..fd55ecbd36 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -21,6 +21,12 @@
  * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
+ *
+ * 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"
@@ -159,6 +165,19 @@ 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].
+     */
+    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);
@@ -184,11 +203,28 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
     }
 }
 
+static int pci_piix_ide_post_load(PCIIDEState *s, int version_id)
+{
+    PCIDevice *dev = PCI_DEVICE(s);
+    uint8_t *pci_conf = dev->config;
+
+    /*
+     * To preserve backward compatibility, handle saved machine states
+     * with reserved bits set (see comment in pci_piix_ide_realize()).
+     */
+    pci_set_word(pci_conf + PCI_COMMAND,
+                 pci_get_word(pci_conf + PCI_COMMAND) &
+                 (PCI_COMMAND_MASTER | PCI_COMMAND_IO));
+
+    return 0;
+}
+
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    PCIIDEClass *ic = PCI_IDE_CLASS(klass);
 
     dc->reset = piix_ide_reset;
     k->realize = pci_piix_ide_realize;
@@ -196,6 +232,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
+    ic->post_load = pci_piix_ide_post_load;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
 }
@@ -211,6 +248,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    PCIIDEClass *ic = PCI_IDE_CLASS(klass);
 
     dc->reset = piix_ide_reset;
     k->realize = pci_piix_ide_realize;
@@ -218,6 +256,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
     k->class_id = PCI_CLASS_STORAGE_IDE;
+    ic->post_load = pci_piix_ide_post_load;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
 }
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..727c748a0f 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -40,7 +40,12 @@ typedef struct BMDMAState {
 } BMDMAState;
 
 #define TYPE_PCI_IDE "pci-ide"
-OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
+OBJECT_DECLARE_TYPE(PCIIDEState, PCIIDEClass, PCI_IDE)
+
+struct PCIIDEClass {
+    IDEDeviceClass parent_class;
+    int (*post_load)(PCIIDEState *s, int version_id);
+};
 
 struct PCIIDEState {
     /*< private >*/
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 5bcb75a7e5..85a3967063 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -173,6 +173,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;
-- 
2.34.1

Re: [PATCH v3 2/2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Michael S. Tsirkin]

On Sun, Sep 25, 2022 at 09:37:59AM +0000, Lev Kujawski wrote:
> 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 PIIX3 specification nor actual hardware (a Tyan S1686D system)
> permit modification of the Memory Space Enable (MSE) bit, 1, and thus
> the command register would be left in an unspecified state without
> this patch.
> 
> * hw/ide/pci.c
>   Call post_load if provided by derived IDE controller.
> * hw/ide/piix.c
>   a) Add references to the PIIX data sheets.
>   b) Mask the MSE bit using the QEMU PCI device wmask field.
>   c) Add a post_load function to mask bits from saved machine states.
>   d) Specify post_load for both the PIIX3/4 IDE controllers.
> * include/hw/ide/pci.h
>   Switch from SIMPLE_TYPE to TYPE, explicitly create a PCIIDEClass
>   that includes the post_load function pointer.
> * tests/qtest/ide-test.c
>   Use the command_disabled field of the QPCIDevice testing model to
>   indicate that PCI_COMMAND_MEMORY is hardwired in the PIIX3/4 IDE
>   controller.
> 
> Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>


I guess this cna work but what I had in mind is much
simpler. Add an internal property (name starting with "x-")
enabling the buggy behaviour and set it in hw compat array.
If set - do not touch the wmask register.

post load hooks are harder to reason about.

Sorry about not being clear originally.

> ---
> (v2) Use QEMU's built-in PCI bit-masking support rather than attempting
>      to manually filter writes.  Thanks to Philippe Mathieu-Daude and
>      Michael S. Tsirkin for review and the pointer.
> (v3) Handle migration of older machine states, which may have set bits
>      masked by this patch, via a new post_load method of PCIIDEClass.
>      Thanks to Michael S. Tsirkin for catching this via review.
> 
>  hw/ide/pci.c           |  5 +++++
>  hw/ide/piix.c          | 39 +++++++++++++++++++++++++++++++++++++++
>  include/hw/ide/pci.h   |  7 ++++++-
>  tests/qtest/ide-test.c |  1 +
>  4 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 84ba733548..e42c7b9415 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -447,6 +447,7 @@ static const VMStateDescription vmstate_bmdma = {
>  
>  static int ide_pci_post_load(void *opaque, int version_id)
>  {
> +    PCIIDEClass *dc = PCI_IDE_GET_CLASS(opaque);
>      PCIIDEState *d = opaque;
>      int i;
>  
> @@ -457,6 +458,10 @@ static int ide_pci_post_load(void *opaque, int version_id)
>          ide_bmdma_post_load(&d->bmdma[i], -1);
>      }
>  
> +    if (dc->post_load) {
> +        dc->post_load(d, version_id);
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 9a9b28078e..fd55ecbd36 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -21,6 +21,12 @@
>   * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>   * THE SOFTWARE.
> + *
> + * 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"
> @@ -159,6 +165,19 @@ 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].
> +     */
> +    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);
> @@ -184,11 +203,28 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> +static int pci_piix_ide_post_load(PCIIDEState *s, int version_id)
> +{
> +    PCIDevice *dev = PCI_DEVICE(s);
> +    uint8_t *pci_conf = dev->config;
> +
> +    /*
> +     * To preserve backward compatibility, handle saved machine states
> +     * with reserved bits set (see comment in pci_piix_ide_realize()).
> +     */
> +    pci_set_word(pci_conf + PCI_COMMAND,
> +                 pci_get_word(pci_conf + PCI_COMMAND) &
> +                 (PCI_COMMAND_MASTER | PCI_COMMAND_IO));
> +
> +    return 0;
> +}
> +
>  /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>  static void piix3_ide_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    PCIIDEClass *ic = PCI_IDE_CLASS(klass);
>  
>      dc->reset = piix_ide_reset;
>      k->realize = pci_piix_ide_realize;
> @@ -196,6 +232,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
> +    ic->post_load = pci_piix_ide_post_load;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      dc->hotpluggable = false;
>  }
> @@ -211,6 +248,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    PCIIDEClass *ic = PCI_IDE_CLASS(klass);
>  
>      dc->reset = piix_ide_reset;
>      k->realize = pci_piix_ide_realize;
> @@ -218,6 +256,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
> +    ic->post_load = pci_piix_ide_post_load;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      dc->hotpluggable = false;
>  }
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..727c748a0f 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -40,7 +40,12 @@ typedef struct BMDMAState {
>  } BMDMAState;
>  
>  #define TYPE_PCI_IDE "pci-ide"
> -OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
> +OBJECT_DECLARE_TYPE(PCIIDEState, PCIIDEClass, PCI_IDE)
> +
> +struct PCIIDEClass {
> +    IDEDeviceClass parent_class;
> +    int (*post_load)(PCIIDEState *s, int version_id);
> +};
>  
>  struct PCIIDEState {
>      /*< private >*/
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index 5bcb75a7e5..85a3967063 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -173,6 +173,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;
> -- 
> 2.34.1

Re: [PATCH v3 2/2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Lev Kujawski]

> I guess this cna work but what I had in mind is much
> simpler. Add an internal property (name starting with "x-")
> enabling the buggy behaviour and set it in hw compat array.
> If set - do not touch the wmask register.
>
> post load hooks are harder to reason about.

Thanks again for the review and clarification, please find attached an
updated patch.  My only concern with the internal property approach is
a potential proliferation of similar boolean values if someone else
encounters an incompatibility.  I have not conducted a thorough audit
of all the PIIX 3/4 IDE registers for hardwired bits (only what I
encountered testing proprietary firmware - PCICMD), and I do not have
access to my PIIX 3 system at the moment.

Kind regards,
Lev Kujawski

Lev Kujawski (2):
  qpci_device_enable: Allow for command bits hardwired to 0
  hw/ide/piix: Ignore writes of hardwired PCI command register bits

 hw/core/machine.c        |  5 ++++-
 hw/ide/piix.c            | 24 ++++++++++++++++++++++++
 include/hw/ide/pci.h     |  1 +
 tests/qtest/ide-test.c   |  1 +
 tests/qtest/libqos/pci.c | 13 +++++++------
 tests/qtest/libqos/pci.h |  1 +
 6 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] qpci_device_enable: Allow for command bits hardwired to 0

[Lev Kujawski]

Devices like the PIIX3/4 IDE controller do not support certain modes
of operation, such as memory space accesses, and indicate this lack of
support by hardwiring the applicable bits to zero.  Extend the QEMU
PCI device testing framework to accommodate such devices.

* tests/qtest/libqos/pci.h: Add the command_disabled word to indicate
  bits hardwired to 0.
* tests/qtest/libqos/pci.c: Verify that hardwired bits are actually
  hardwired.

Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>
---
 tests/qtest/libqos/pci.c | 13 +++++++------
 tests/qtest/libqos/pci.h |  1 +
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index b23d72346b..4f3d28d8d9 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -220,18 +220,19 @@ int qpci_secondary_buses_init(QPCIBus *bus)
 
 void qpci_device_enable(QPCIDevice *dev)
 {
-    uint16_t cmd;
+    const uint16_t enable_bits =
+        PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    uint16_t cmd, new_cmd;
 
     /* FIXME -- does this need to be a bus callout? */
     cmd = qpci_config_readw(dev, PCI_COMMAND);
-    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    cmd |= enable_bits;
     qpci_config_writew(dev, PCI_COMMAND, cmd);
 
     /* Verify the bits are now set. */
-    cmd = qpci_config_readw(dev, PCI_COMMAND);
-    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
-    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
-    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
+    new_cmd = qpci_config_readw(dev, PCI_COMMAND);
+    new_cmd &= enable_bits;
+    g_assert_cmphex(new_cmd, ==, enable_bits & ~dev->command_disabled);
 }
 
 /**
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 8389614523..eaedb98588 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -68,6 +68,7 @@ struct QPCIDevice
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
+    uint16_t command_disabled;
 };
 
 struct QPCIAddress {
-- 
2.34.1

Re: [PATCH 1/2] qpci_device_enable: Allow for command bits hardwired to 0

[Michael S. Tsirkin]

On Mon, Oct 24, 2022 at 09:46:20AM +0000, Lev Kujawski wrote:
> Devices like the PIIX3/4 IDE controller do not support certain modes
> of operation, such as memory space accesses, and indicate this lack of
> support by hardwiring the applicable bits to zero.  Extend the QEMU
> PCI device testing framework to accommodate such devices.
> 
> * tests/qtest/libqos/pci.h: Add the command_disabled word to indicate
>   bits hardwired to 0.
> * tests/qtest/libqos/pci.c: Verify that hardwired bits are actually
>   hardwired.
> 
> Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>


This patch makes the fuzzer unhappy with qpci_device_enable():
https://gitlab.com/qemu-project/qemu/-/jobs/3253817499

Will drop this patchset for now, pls address and resubmit.



> ---
>  tests/qtest/libqos/pci.c | 13 +++++++------
>  tests/qtest/libqos/pci.h |  1 +
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index b23d72346b..4f3d28d8d9 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -220,18 +220,19 @@ int qpci_secondary_buses_init(QPCIBus *bus)
>  
>  void qpci_device_enable(QPCIDevice *dev)
>  {
> -    uint16_t cmd;
> +    const uint16_t enable_bits =
> +        PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> +    uint16_t cmd, new_cmd;
>  
>      /* FIXME -- does this need to be a bus callout? */
>      cmd = qpci_config_readw(dev, PCI_COMMAND);
> -    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> +    cmd |= enable_bits;
>      qpci_config_writew(dev, PCI_COMMAND, cmd);
>  
>      /* Verify the bits are now set. */
> -    cmd = qpci_config_readw(dev, PCI_COMMAND);
> -    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
> -    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
> -    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
> +    new_cmd = qpci_config_readw(dev, PCI_COMMAND);
> +    new_cmd &= enable_bits;
> +    g_assert_cmphex(new_cmd, ==, enable_bits & ~dev->command_disabled);
>  }
>  
>  /**
> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
> index 8389614523..eaedb98588 100644
> --- a/tests/qtest/libqos/pci.h
> +++ b/tests/qtest/libqos/pci.h
> @@ -68,6 +68,7 @@ struct QPCIDevice
>      bool msix_enabled;
>      QPCIBar msix_table_bar, msix_pba_bar;
>      uint64_t msix_table_off, msix_pba_off;
> +    uint16_t command_disabled;
>  };
>  
>  struct QPCIAddress {
> -- 
> 2.34.1

[PATCH 2/2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Lev Kujawski]

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>
---
 hw/core/machine.c      |  5 ++++-
 hw/ide/piix.c          | 24 ++++++++++++++++++++++++
 include/hw/ide/pci.h   |  1 +
 tests/qtest/ide-test.c |  1 +
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..8e8e69c081 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,7 +40,10 @@
 #include "hw/virtio/virtio-pci.h"
 #include "qom/object_interfaces.h"
 
-GlobalProperty hw_compat_7_1[] = {};
+GlobalProperty hw_compat_7_1[] = {
+    { "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);
 
 GlobalProperty hw_compat_7_0[] = {
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/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/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;
-- 
2.34.1

Re: [PATCH v3 2/2] hw/ide/piix: Ignore writes of hardwired PCI command register bits

[Michael S. Tsirkin]

On Mon, Oct 24, 2022 at 09:46:19AM +0000, Lev Kujawski wrote:
> > I guess this cna work but what I had in mind is much
> > simpler. Add an internal property (name starting with "x-")
> > enabling the buggy behaviour and set it in hw compat array.
> > If set - do not touch the wmask register.
> >
> > post load hooks are harder to reason about.
> 
> Thanks again for the review and clarification, please find attached an
> updated patch.  My only concern with the internal property approach is
> a potential proliferation of similar boolean values if someone else
> encounters an incompatibility.

Yes they do tend to proliferate. We might drop some down the road
after the affected machine type is dropped.

>  I have not conducted a thorough audit
> of all the PIIX 3/4 IDE registers for hardwired bits (only what I
> encountered testing proprietary firmware - PCICMD), and I do not have
> access to my PIIX 3 system at the moment.
> 
> Kind regards,
> Lev Kujawski
> 
> Lev Kujawski (2):
>   qpci_device_enable: Allow for command bits hardwired to 0
>   hw/ide/piix: Ignore writes of hardwired PCI command register bits
> 
>  hw/core/machine.c        |  5 ++++-
>  hw/ide/piix.c            | 24 ++++++++++++++++++++++++
>  include/hw/ide/pci.h     |  1 +
>  tests/qtest/ide-test.c   |  1 +
>  tests/qtest/libqos/pci.c | 13 +++++++------
>  tests/qtest/libqos/pci.h |  1 +
>  6 files changed, 38 insertions(+), 7 deletions(-)
> 
> -- 
> 2.34.1

[PATCH 1/2] qpci_device_enable: Allow for command bits hardwired to 0

[Lev Kujawski]

Devices like the PIIX3/4 IDE controller do not support certain modes
of operation, such as memory space accesses, and indicate this lack of
support by hardwiring the applicable bits to zero. The QEMU PCI device
testing framework is hereby extended to accommodate such devices.

* tests/qtest/libqos/pci.h: Add the command_disabled word to indicate
  bits hardwired to 0.
* tests/qtest/libqos/pci.c: Verify that hardwired bits are actually
  hardwired.
* tests/qtest/ide-test.c: Use the new command_disabled member to
  indicate that PCI_COMMAND_MEMORY is hardwired in the PIIX3/4
  IDE controller.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 tests/qtest/ide-test.c   |  1 +
 tests/qtest/libqos/pci.c | 13 +++++++------
 tests/qtest/libqos/pci.h |  1 +
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 5bcb75a7e5..85a3967063 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -173,6 +173,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;
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index b23d72346b..4f3d28d8d9 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -220,18 +220,19 @@ int qpci_secondary_buses_init(QPCIBus *bus)
 
 void qpci_device_enable(QPCIDevice *dev)
 {
-    uint16_t cmd;
+    const uint16_t enable_bits =
+        PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    uint16_t cmd, new_cmd;
 
     /* FIXME -- does this need to be a bus callout? */
     cmd = qpci_config_readw(dev, PCI_COMMAND);
-    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    cmd |= enable_bits;
     qpci_config_writew(dev, PCI_COMMAND, cmd);
 
     /* Verify the bits are now set. */
-    cmd = qpci_config_readw(dev, PCI_COMMAND);
-    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
-    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
-    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
+    new_cmd = qpci_config_readw(dev, PCI_COMMAND);
+    new_cmd &= enable_bits;
+    g_assert_cmphex(new_cmd, ==, enable_bits & ~dev->command_disabled);
 }
 
 /**
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 8389614523..eaedb98588 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -68,6 +68,7 @@ struct QPCIDevice
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
+    uint16_t command_disabled;
 };
 
 struct QPCIAddress {
-- 
2.34.1