Re: [Qemu-devel] [PATCH] pci: honor PCI_COMMAND_MEMORY

[Marcel Apfelbaum]

On 17/07/2017 15:48, Dmitry Fleytman wrote:
> On 16 Jul 2017, at 19:56 PM, Marcel Apfelbaum <address@hidden 
> <mailto:address@hidden>> wrote:
> > On 16/07/2017 11:29, Dmitry Fleytman wrote:
> > > According to PCI spec. bit 1 of command
> > > register (PCI_COMMAND_MEMORY) controls
> > > a device's response to memory space accesses.
> > > A value of 0 disables the device response.
> > > A value of 1 allows the device to respond
> > > to memory space accesses.
> >
> > Hi Dmitry,
>
> Hi Marcel,
>
> > > Current behavior introduced by commit
> > >   commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d
> > >   Author: Avi Kivity <address@hidden <mailto:address@hidden>>
> > >   Date:   Wed Oct 3 17:42:58 2012 +0200
> > >       pci: honor PCI_COMMAND_MASTER
> > > is to ignore device memory space accesses unless
> > > bit 2 (PCI_COMMAND_MASTER) is set.
> > > Aforementioned commit introduced regression of
> > > Windows hibernation (S4) functionality support
> > > because on resume Windows kernel sets bits 0 and 1
> > > (PCI_COMMAND_MEMORY | PCI_COMMAND_IO) of boot
> > > device's (piix3-ide in our specific case) command
> > > register and tries to work with the device.
> > > > Since PCI_COMMAND_MASTER bit is not set, device
> > > does not answer and Windows fails to resume from
> > > hibernation.
> >
> > As far as I am aware "Bus master" is needed for a device
> > to start PCI transactions, while PCI_COMMAND_MEMORY/IO
> > controls the device ability to respond to memory/IO accesses,
> > not to start them.
> > If "Bus master" is needed for DMA access or MSI, the OS should
> > explicitly set the  PCI_COMMAND_MASTER, right?
> >
> >
> >
> > > As a result following BSOD happens:
> > >   BugCheck A0, {10e, a, aa00, 418}
> > >   Probably caused by : ntkrnlmp.exe ( 
> > > nt!PopHiberChecksumHiberFileData+b346 )
> > >   Followup:     MachineOwner
> > >   ---------
> > >   0: kd> !analyze -v
> > >   
> > > *******************************************************************************
> > >   * 
> > >                                                                             *
> > >   *                        Bugcheck Analysis 
> > >                                    *
> > >   * 
> > >                                                                             *
> > >   
> > > *******************************************************************************
> > >   INTERNAL_POWER_ERROR (a0)
> > >   The power policy manager experienced a fatal error.
> > >   Arguments:
> > >   Arg1: 000000000000010e, The disk subsystem returned corrupt data 
> > > while reading from the
> > >       hibernation file.
> > >   Arg2: 000000000000000a
> > >   Arg3: 000000000000aa00, Incorrect checksum
> > >   Arg4: 0000000000000418, Previous disk read's checksum
> >
> > Does piix3-ide fail to respond to IO/MEM accesses if
> > PCI_COMMAND_MASTER is not set? Or is it a DMA issue?
>
> It is a DMA issue.
>
> After some additional research I see that this patch is wrong, please 
> ignore it.
>
> Windows expects DMA transfers from device but does not set
> bus master bit in PCI command register.
>
> Am I understand correctly that there are no special cases for
> IDE controllers, i.e. bus master bit must be set by SW same
> way as for other PCI devices?

If is a PCI device it has to comply with the same set of rules.
The interesting part is the hibernation. Maybe there is a mismatch
with Windows expectations regarding the state of the command register
after hybernation?


Thanks,
Marcel

> Thanks,
> Dmitry.
>
> > Thanks,
> > Marcel
> >
> > > According to our tests this problem happens at least on
> > > Windows 8/8.1/2012/2012R2/10/2016.
> > > This patch solves https://bugzilla.redhat.com/show_bug.cgi?id=988351
> > > Signed-off-by: Dmitry Fleytman <address@hidden 
> > > <mailto:address@hidden>>
> > > ---
> > >  hw/pci/pci.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 0c6f74a..10af82f 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -480,7 +480,7 @@ static int get_pci_config_device(QEMUFile *f, 
> > > void *pv, size_t size,
> > >        memory_region_set_enabled(&s->bus_master_enable_region,
> > >                                pci_get_word(s->config + PCI_COMMAND)
> > > -                              & PCI_COMMAND_MASTER);
> > > +                              & (PCI_COMMAND_MASTER | 
> > > PCI_COMMAND_MEMORY));
> > >        g_free(config);
> > >      return 0;
> > > @@ -1356,7 +1356,7 @@ void pci_default_write_config(PCIDevice *d, 
> > > uint32_t addr, uint32_t val_in, int
> > >          pci_update_irq_disabled(d, was_irq_disabled);
> > >          memory_region_set_enabled(&d->bus_master_enable_region,
> > >                                    pci_get_word(d->config + PCI_COMMAND)
> > > -                                    & PCI_COMMAND_MASTER);
> > > +                                  & (PCI_COMMAND_MASTER | 
> > > PCI_COMMAND_MEMORY));
> > >      }
> > >        msi_write_config(d, addr, val_in, l);