[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/2] hw/ide/piix: Ignore writes of hardwired PCI command r
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v3 2/2] hw/ide/piix: Ignore writes of hardwired PCI command register bits |
Date: |
Fri, 7 Oct 2022 09:54:35 -0400 |
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,
Michael S. Tsirkin <=