[PATCH 1/4] hw/ide/atapi.c: Correct typos (CD-CDROM -> CD-ROM)

[PATCH 1/4] hw/ide/atapi.c: Correct typos (CD-CDROM -> CD-ROM)

[Lev Kujawski]

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 hw/ide/atapi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index b626199e3d..88b2890faf 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -318,7 +318,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
     }
 }
 
-/* start a CD-CDROM read command */
+/* start a CD-ROM read command */
 static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
                                    int sector_size)
 {
@@ -417,7 +417,7 @@ eot:
     ide_set_inactive(s, false);
 }
 
-/* start a CD-CDROM read command with DMA */
+/* start a CD-ROM read command with DMA */
 /* XXX: test if DMA is available */
 static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
                                    int sector_size)
-- 
2.34.1

[PATCH 2/4] hw/ide/core: Clear LBA and drive bits for EXECUTE DEVICE DIAGNOSTIC

[Lev Kujawski]

Prior to this patch, cmd_exec_dev_diagnostic relied upon
ide_set_signature to clear the device register.  While the
preservation of the drive bit by ide_set_signature is necessary for
the DEVICE RESET, IDENTIFY DEVICE, and READ SECTOR commands,
ATA/ATAPI-6 specifies that "DEV shall be cleared to zero" for EXECUTE
DEVICE DIAGNOSTIC.

This deviation was uncovered by the ATACT Device Testing Program
written by Hale Landis.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 hw/ide/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c2caa54285..5a24547e49 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1704,8 +1704,14 @@ static bool cmd_identify_packet(IDEState *s, uint8_t cmd)
     return false;
 }
 
+/* EXECUTE DEVICE DIAGNOSTIC */
 static bool cmd_exec_dev_diagnostic(IDEState *s, uint8_t cmd)
 {
+    /*
+     * Clear the device register per the ATA (v6) specification,
+     * because ide_set_signature does not clear LBA or drive bits.
+     */
+    s->select = (ATA_DEV_ALWAYS_ON);
     ide_set_signature(s);
 
     if (s->drive_kind == IDE_CD) {
-- 
2.34.1

[PATCH 3/4] piix_ide_reset: Use pci_set_* functions instead of direct access

[Lev Kujawski]

Eliminates the remaining TODOs in hw/ide/piix.c by:
- Using pci_set_{size} functions to write the PIIX PCI configuration
  space instead of manipulating it directly as an array; and
- Documenting default register values by reference to the controlling
  specification.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 hw/ide/piix.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..76ea8fd9f6 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -21,6 +21,10 @@
  * 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.
  */
 
 #include "qemu/osdep.h"
@@ -114,14 +118,11 @@ static void piix_ide_reset(DeviceState *dev)
         ide_bus_reset(&d->bus[i]);
     }
 
-    /* TODO: this is the default. do not override. */
-    pci_conf[PCI_COMMAND] = 0x00;
-    /* TODO: this is the default. do not override. */
-    pci_conf[PCI_COMMAND + 1] = 0x00;
-    /* TODO: use pci_set_word */
-    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
-    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
-    pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
+    /* PCI command register default value (0000h) per [1, p.48].  */
+    pci_set_word(pci_conf + PCI_COMMAND, 0x0000);
+    pci_set_word(pci_conf + PCI_STATUS,
+                 PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_FAST_BACK);
+    pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
 }
 
 static int pci_piix_init_ports(PCIIDEState *d)
-- 
2.34.1

Re: [PATCH 3/4] piix_ide_reset: Use pci_set_* functions instead of direct access

[Philippe Mathieu-Daudé via]

On 28/5/22 22:47, Lev Kujawski wrote:
> Eliminates the remaining TODOs in hw/ide/piix.c by:
> - Using pci_set_{size} functions to write the PIIX PCI configuration
>    space instead of manipulating it directly as an array; and
> - Documenting default register values by reference to the controlling
>    specification.
> 
> Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
> ---
>   hw/ide/piix.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 4/4] 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>
---
 hw/ide/piix.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 76ea8fd9f6..f1d1168ecd 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"
@@ -32,6 +34,7 @@
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/range.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/dma.h"
@@ -220,6 +223,26 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
     }
 }
 
+static void piix_pci_config_write(PCIDevice *d, uint32_t addr,
+                                  uint32_t val, int l)
+{
+    /*
+     * 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].
+     */
+    if (range_covers_byte(addr, l, PCI_COMMAND)) {
+        val &= ~(0xfffa << ((PCI_COMMAND - addr) << 3));
+    }
+
+    pci_default_write_config(d, addr, val, l);
+}
+
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
@@ -232,6 +255,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;
+    k->config_write = piix_pci_config_write;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
 }
@@ -260,6 +284,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;
+    k->config_write = piix_pci_config_write;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
 }
-- 
2.34.1

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

[Philippe Mathieu-Daudé via]

On 28/5/22 22:47, 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>
> ---
>   hw/ide/piix.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 76ea8fd9f6..f1d1168ecd 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"
> @@ -32,6 +34,7 @@
>   #include "migration/vmstate.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> +#include "qemu/range.h"
>   #include "sysemu/block-backend.h"
>   #include "sysemu/blockdev.h"
>   #include "sysemu/dma.h"
> @@ -220,6 +223,26 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>       }
>   }
>   
> +static void piix_pci_config_write(PCIDevice *d, uint32_t addr,
> +                                  uint32_t val, int l)
> +{
> +    /*
> +     * 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].
> +     */
> +    if (range_covers_byte(addr, l, PCI_COMMAND)) {
> +        val &= ~(0xfffa << ((PCI_COMMAND - addr) << 3));

Watch out, len can be 1/2/4.

> +    }
> +
> +    pci_default_write_config(d, addr, val, l);
> +}
> +
>   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>   static void piix3_ide_class_init(ObjectClass *klass, void *data)
>   {
> @@ -232,6 +255,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;
> +    k->config_write = piix_pci_config_write;
>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>       dc->hotpluggable = false;
>   }
> @@ -260,6 +284,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;
> +    k->config_write = piix_pci_config_write;
>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>       dc->hotpluggable = false;
>   }

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

[Michael S. Tsirkin]

On Mon, May 30, 2022 at 03:33:18PM +0200, Philippe Mathieu-Daudé wrote:
> On 28/5/22 22:47, 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>
> > ---
> >   hw/ide/piix.c | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> > 
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index 76ea8fd9f6..f1d1168ecd 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"
> > @@ -32,6 +34,7 @@
> >   #include "migration/vmstate.h"
> >   #include "qapi/error.h"
> >   #include "qemu/module.h"
> > +#include "qemu/range.h"
> >   #include "sysemu/block-backend.h"
> >   #include "sysemu/blockdev.h"
> >   #include "sysemu/dma.h"
> > @@ -220,6 +223,26 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
> >       }
> >   }
> > +static void piix_pci_config_write(PCIDevice *d, uint32_t addr,
> > +                                  uint32_t val, int l)
> > +{
> > +    /*
> > +     * 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].
> > +     */
> > +    if (range_covers_byte(addr, l, PCI_COMMAND)) {
> > +        val &= ~(0xfffa << ((PCI_COMMAND - addr) << 3));
> 
> Watch out, len can be 1/2/4.


If there are bits hardwired to 0 the right way to do it is
by clearing a bit in wmask. Might need machine compat machinery
for this.

> > +    }
> > +
> > +    pci_default_write_config(d, addr, val, l);
> > +}
> > +
> >   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> >   static void piix3_ide_class_init(ObjectClass *klass, void *data)
> >   {
> > @@ -232,6 +255,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;
> > +    k->config_write = piix_pci_config_write;
> >       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> >       dc->hotpluggable = false;
> >   }
> > @@ -260,6 +284,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;
> > +    k->config_write = piix_pci_config_write;
> >       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> >       dc->hotpluggable = false;
> >   }

Re: [PATCH 1/4] hw/ide/atapi.c: Correct typos (CD-CDROM -> CD-ROM)

[Philippe Mathieu-Daudé via]

On 28/5/22 22:46, Lev Kujawski wrote:
> Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
> ---
>   hw/ide/atapi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>