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

["Michael S. Tsirkin" <mst@redhat.com>]

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