On Fri, Aug 31, 2012 at 11:42:27AM +0300, Michael S. Tsirkin wrote:
> Some minor nits below. If you dont get to it I will tweak this patch
> when I apply it early next week.
>
> On Thu, Aug 30, 2012 at 01:51:15PM -0400, Jason Baron wrote:
> > The Advanced Error Interrupt Message Number (bits 31:27 of the Root
> > Error Status Register) is updated when the number of msi messages assigned
> > to a
> > device changes. Migration of windows 7 on q35 chipset failed because the
> > check
> > in get_pci_config_device() fails due to wmask being set on these bits.
>
> I think you actually mean 'not being set on these bits'?
>
No, the check is:
static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
{
PCIDevice *s = container_of(pv, PCIDevice, config);
uint8_t *config;
int i;
assert(size == pci_config_size(s));
config = g_malloc(size);
qemu_get_buffer(f, config, size);
for (i = 0; i < size; ++i) {
if ((config[i] ^ s->config[i]) &
s->cmask[i] & ~s->wmask[i] & ~s->w1cmask[i]) {
g_free(config);
return -EINVAL;
}
}
.....
So because cmask is set and these bits differ in config space (due to
them being updated before migration), the migration aborts.
> > Its valid
> > to update these bits and we must restore this state across migration.
> >
> > Signed-off-by: Jason Baron <jba...@redhat.com>
> > ---
> > hw/pcie_aer.c | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
> > index 3b6981c..6edcd79 100644
> > --- a/hw/pcie_aer.c
> > +++ b/hw/pcie_aer.c
> > @@ -738,6 +738,12 @@ void pcie_aer_root_init(PCIDevice *dev)
> > PCI_ERR_ROOT_CMD_EN_MASK);
> > pci_set_long(dev->w1cmask + pos + PCI_ERR_ROOT_STATUS,
> > PCI_ERR_ROOT_STATUS_REPORT_MASK);
> > + /* Bits 31:27 - Advanced Error Interrupt Message Number
>
> This line is better moved to near definition of PCI_ERR_ROOT_IRQ.
> Then here we can say 'PCI_ERR_ROOT_IRQ is RO but devices
> change it using a device-specific method.'
ok.
>
> > + * These bits are updated when the number of MSI messages changes.
> > + * By clearing the cmask, pcie devices can be migrated.
> > + */
> > + pci_set_long(dev->cmask + pos + PCI_ERR_ROOT_STATUS,
> > + (1 << PCI_ERR_ROOT_IRQ_SHIFT) - 1);
>
> ~PCI_ERR_ROOT_IRQ would be clearer I think.
>
agreed.
I will re-spin and post a v2.
Thanks,
-Jason
