Re: [PATCH v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits
On Tue, Sep 06, 2022 at 10:23:57AM -0400, Michael S. Tsirkin wrote: > On Thu, Jun 02, 2022 at 08:47:31PM +, 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 > > > 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: h), 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
Am 2. Juni 2022 20:47:31 UTC schrieb 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 >--- >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: h), 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
On Thu, Jun 02, 2022 at 08:47:31PM +, 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 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: h), 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 v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits
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 --- 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: h), 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
