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 <lku...@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