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

[Michael S. Tsirkin]

On Tue, Sep 06, 2022 at 10:23:57AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 02, 2022 at 08:47:31PM +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.
> > 
> > Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
> 
> 
> 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: 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);
> > -- 
> > 2.34.1
> > 
> > 
> >