Patchwork [v3,07/11] versatile_pci: Implement the correct PCI IRQ mapping

login
register
mail settings
Submitter Peter Maydell
Date April 4, 2013, 12:58 p.m.
Message ID <1365080313-20875-8-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/233771/
State New
Headers show

Comments

Peter Maydell - April 4, 2013, 12:58 p.m.
Implement the correct IRQ mapping for the Versatile PCI controller; it
differs between realview and versatile boards, but the previous QEMU
implementation was correct only for the first PCI card on a versatile
board, since we weren't swizzling IRQs based on the slot number.

Since this change would otherwise break any uses of PCI on Linux kernels
which have an equivalent bug (since they have effectively only been
tested against QEMU, not real hardware), we implement a mechanism
for automatically detecting those broken kernels and switching back
to the old mapping. This works by looking at the values the kernel
writes to the PCI_INTERRUPT_LINE register in the config space, which
is effectively the interrupt number the kernel expects the device
to be using.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/versatile_pci.c |  105 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 6 deletions(-)
Michael S. Tsirkin - April 4, 2013, 5:03 p.m.
On Thu, Apr 04, 2013 at 01:58:29PM +0100, Peter Maydell wrote:
> Implement the correct IRQ mapping for the Versatile PCI controller; it
> differs between realview and versatile boards, but the previous QEMU
> implementation was correct only for the first PCI card on a versatile
> board, since we weren't swizzling IRQs based on the slot number.
> 
> Since this change would otherwise break any uses of PCI on Linux kernels
> which have an equivalent bug (since they have effectively only been
> tested against QEMU, not real hardware), we implement a mechanism
> for automatically detecting those broken kernels and switching back
> to the old mapping. This works by looking at the values the kernel
> writes to the PCI_INTERRUPT_LINE register in the config space, which
> is effectively the interrupt number the kernel expects the device
> to be using.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Looks good a couple of suggestion on the implementation.

> ---
>  hw/versatile_pci.c |  105 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index 576e619..859fbee 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -13,6 +13,28 @@
>  #include "hw/pci/pci_host.h"
>  #include "exec/address-spaces.h"
>  
> +/* Old and buggy versions of QEMU used the wrong mapping from
> + * PCI IRQs to system interrupt lines. Unfortunately the Linux
> + * kernel also had the corresponding bug in setting up interrupts
> + * (so older kernels work on QEMU and not on real hardware).
> + * We automatically detect these broken kernels and flip back
> + * to the broken irq mapping by spotting guest writes to the
> + * PCI_INTERRUPT_LINE register to see where the guest thinks
> + * interrupts are going to be routed. So we start in state
> + * ASSUME_OK on reset, and transition to either BROKEN or
> + * FORCE_OK at the first write to an INTERRUPT_LINE register for
> + * a slot where broken and correct interrupt mapping would differ.
> + * Once in either BROKEN or FORCE_OK we never transition again;
> + * this allows a newer kernel to use the INTERRUPT_LINE
> + * registers arbitrarily once it has indicated that it isn't
> + * broken in its init code somewhere.
> + */
> +enum {
> +    IRQ_MAPPING_ASSUME_OK = 0,
> +    IRQ_MAPPING_BROKEN = 1,
> +    IRQ_MAPPING_FORCE_OK = 2,

Better to prefix with PCI_VPB_ or something.
And we don't really care what the values are,
can let enum assign what it wants.

> +};
> +
>  typedef struct {
>      PCIHostState parent_obj;
>  
> @@ -26,6 +48,9 @@ typedef struct {
>  
>      /* Constant for life of device: */
>      int realview;
> +
> +    /* Variable state: */
> +    uint8_t irq_mapping;
>  } PCIVPBState;
>  
>  #define TYPE_VERSATILE_PCI "versatile_pci"
> @@ -44,14 +69,27 @@ static inline uint32_t vpb_pci_config_addr(hwaddr addr)
>  static void pci_vpb_config_write(void *opaque, hwaddr addr,
>                                   uint64_t val, unsigned size)
>  {
> -    pci_data_write(opaque, vpb_pci_config_addr(addr), val, size);
> +    PCIVPBState *s = (PCIVPBState *)opaque;

Please don't cast void *.

> +    if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE

> +        && s->irq_mapping == IRQ_MAPPING_ASSUME_OK) {
> +        uint8_t devfn = addr >> 8;
> +        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {

Would it be enough to just detect this for devfn 0?
If yes this happens to be our device here, so we could
cleanly implement a config_write callback, like this:

	pci_default_write_config
	if (!ranger_cover_byte(addr, size, PCI_INTERRUPT_LINE))
		return;
	if (dev->config[PCI_INTERRUPT_LINE] == 27) {
                s->irq_mapping = IRQ_MAPPING_BROKEN;
        } else {
                s->irq_mapping = IRQ_MAPPING_FORCE_OK;
        }
		


> +            if (val == 27) {
> +                s->irq_mapping = IRQ_MAPPING_BROKEN;
> +            } else {
> +                s->irq_mapping = IRQ_MAPPING_FORCE_OK;
> +            }
> +        }
> +    }
> +    pci_data_write(&s->pci_bus, vpb_pci_config_addr(addr), val, size);


>  }
>  
>  static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr,
>                                      unsigned size)
>  {
> +    PCIVPBState *s = (PCIVPBState *)opaque;
>      uint32_t val;
> -    val = pci_data_read(opaque, vpb_pci_config_addr(addr), size);
> +    val = pci_data_read(&s->pci_bus, vpb_pci_config_addr(addr), size);

Seems cleaner but don't cast void * pointer pls.

>      return val;
>  }
>  
> @@ -63,7 +101,47 @@ static const MemoryRegionOps pci_vpb_config_ops = {
>  
>  static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
>  {
> -    return irq_num;
> +    PCIVPBState *s = container_of(d->bus, PCIVPBState, pci_bus);
> +
> +    if (s->irq_mapping == IRQ_MAPPING_BROKEN) {
> +        /* Legacy broken IRQ mapping for compatibility with old and
> +         * buggy Linux guests
> +         */
> +        return irq_num;
> +    }
> +
> +    /* Slot to IRQ mapping for RealView Platform Baseboard 926 backplane
> +     *      name    slot    IntA    IntB    IntC    IntD
> +     *      A       31      IRQ28   IRQ29   IRQ30   IRQ27
> +     *      B       30      IRQ27   IRQ28   IRQ29   IRQ30
> +     *      C       29      IRQ30   IRQ27   IRQ28   IRQ29
> +     * Slot C is for the host bridge; A and B the peripherals.
> +     * Our output irqs 0..3 correspond to the baseboard's 27..30.
> +     *
> +     * This mapping function takes account of an oddity in the PB926
> +     * board wiring, where the FPGA's P_nINTA input is connected to
> +     * the INTB connection on the board PCI edge connector, P_nINTB
> +     * is connected to INTC, and so on, so everything is one number
> +     * further round from where you might expect.
> +     */
> +    return pci_swizzle_map_irq_fn(d, irq_num + 2);
> +}
> +
> +static int pci_vpb_rv_map_irq(PCIDevice *d, int irq_num)
> +{
> +    /* Slot to IRQ mapping for RealView EB and PB1176 backplane
> +     *      name    slot    IntA    IntB    IntC    IntD
> +     *      A       31      IRQ50   IRQ51   IRQ48   IRQ49
> +     *      B       30      IRQ49   IRQ50   IRQ51   IRQ48
> +     *      C       29      IRQ48   IRQ49   IRQ50   IRQ51
> +     * Slot C is for the host bridge; A and B the peripherals.
> +     * Our output irqs 0..3 correspond to the baseboard's 48..51.
> +     *
> +     * The PB1176 and EB boards don't have the PB926 wiring oddity
> +     * described above; P_nINTA connects to INTA, P_nINTB to INTB
> +     * and so on, which is why this mapping function is different.
> +     */
> +    return pci_swizzle_map_irq_fn(d, irq_num + 3);
>  }
>  
>  static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
> @@ -73,6 +151,13 @@ static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
>      qemu_set_irq(pic[irq_num], level);
>  }
>  
> +static void pci_vpb_reset(DeviceState *d)
> +{
> +    PCIVPBState *s = PCI_VPB(d);
> +
> +    s->irq_mapping = IRQ_MAPPING_ASSUME_OK;
> +}
> +
>  static void pci_vpb_init(Object *obj)
>  {
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
> @@ -95,13 +180,20 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
>  {
>      PCIVPBState *s = PCI_VPB(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    pci_map_irq_fn mapfn;
>      int i;
>  
>      for (i = 0; i < 4; i++) {
>          sysbus_init_irq(sbd, &s->irq[i]);
>      }
>  
> -    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, pci_vpb_map_irq, s->irq, 4);
> +    if (s->realview) {
> +        mapfn = pci_vpb_rv_map_irq;
> +    } else {
> +        mapfn = pci_vpb_map_irq;
> +    }
> +
> +    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
>  
>      /* ??? Register memory space.  */
>  
> @@ -110,10 +202,10 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
>       * 1 : PCI config window
>       * 2 : PCI IO window
>       */
> -    memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, &s->pci_bus,
> +    memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, s,
>                            "pci-vpb-selfconfig", 0x1000000);
>      sysbus_init_mmio(sbd, &s->mem_config);
> -    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, &s->pci_bus,
> +    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, s,
>                            "pci-vpb-config", 0x1000000);
>      sysbus_init_mmio(sbd, &s->mem_config2);
>  
> @@ -159,6 +251,7 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = pci_vpb_realize;
> +    dc->reset = pci_vpb_reset;
>  }
>  
>  static const TypeInfo pci_vpb_info = {
> -- 
> 1.7.9.5
Peter Maydell - April 4, 2013, 7:47 p.m.
On 4 April 2013 18:03, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Apr 04, 2013 at 01:58:29PM +0100, Peter Maydell wrote:
>> Implement the correct IRQ mapping for the Versatile PCI controller; it
>> differs between realview and versatile boards, but the previous QEMU
>> implementation was correct only for the first PCI card on a versatile
>> board, since we weren't swizzling IRQs based on the slot number.
>>
>> Since this change would otherwise break any uses of PCI on Linux kernels
>> which have an equivalent bug (since they have effectively only been
>> tested against QEMU, not real hardware), we implement a mechanism
>> for automatically detecting those broken kernels and switching back
>> to the old mapping. This works by looking at the values the kernel
>> writes to the PCI_INTERRUPT_LINE register in the config space, which
>> is effectively the interrupt number the kernel expects the device
>> to be using.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Looks good a couple of suggestion on the implementation.
>
>> ---
>>  hw/versatile_pci.c |  105 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 99 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
>> index 576e619..859fbee 100644
>> --- a/hw/versatile_pci.c
>> +++ b/hw/versatile_pci.c
>> @@ -13,6 +13,28 @@
>>  #include "hw/pci/pci_host.h"
>>  #include "exec/address-spaces.h"
>>
>> +/* Old and buggy versions of QEMU used the wrong mapping from
>> + * PCI IRQs to system interrupt lines. Unfortunately the Linux
>> + * kernel also had the corresponding bug in setting up interrupts
>> + * (so older kernels work on QEMU and not on real hardware).
>> + * We automatically detect these broken kernels and flip back
>> + * to the broken irq mapping by spotting guest writes to the
>> + * PCI_INTERRUPT_LINE register to see where the guest thinks
>> + * interrupts are going to be routed. So we start in state
>> + * ASSUME_OK on reset, and transition to either BROKEN or
>> + * FORCE_OK at the first write to an INTERRUPT_LINE register for
>> + * a slot where broken and correct interrupt mapping would differ.
>> + * Once in either BROKEN or FORCE_OK we never transition again;
>> + * this allows a newer kernel to use the INTERRUPT_LINE
>> + * registers arbitrarily once it has indicated that it isn't
>> + * broken in its init code somewhere.
>> + */
>> +enum {
>> +    IRQ_MAPPING_ASSUME_OK = 0,
>> +    IRQ_MAPPING_BROKEN = 1,
>> +    IRQ_MAPPING_FORCE_OK = 2,
>
> Better to prefix with PCI_VPB_ or something.

That makes them even longer and more unwieldy, which isn't very
friendly for something that's really only in a single source
file.

> And we don't really care what the values are,
> can let enum assign what it wants.

True.

>> +};
>> +
>>  typedef struct {
>>      PCIHostState parent_obj;
>>
>> @@ -26,6 +48,9 @@ typedef struct {
>>
>>      /* Constant for life of device: */
>>      int realview;
>> +
>> +    /* Variable state: */
>> +    uint8_t irq_mapping;
>>  } PCIVPBState;
>>
>>  #define TYPE_VERSATILE_PCI "versatile_pci"
>> @@ -44,14 +69,27 @@ static inline uint32_t vpb_pci_config_addr(hwaddr addr)
>>  static void pci_vpb_config_write(void *opaque, hwaddr addr,
>>                                   uint64_t val, unsigned size)
>>  {
>> -    pci_data_write(opaque, vpb_pci_config_addr(addr), val, size);
>> +    PCIVPBState *s = (PCIVPBState *)opaque;
>
> Please don't cast void *.

There are quite a lot of casts of void* in hw/, but I'm
happy to help postpone the evil day when we try to build
QEMU with a C++ compiler :-)

>> +    if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE
>
>> +        && s->irq_mapping == IRQ_MAPPING_ASSUME_OK) {
>> +        uint8_t devfn = addr >> 8;
>> +        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {
>
> Would it be enough to just detect this for devfn 0?

Nope, because devfn 0 is almost never used. We start at
devfn 11 (for reasons that aren't entirely clear to me).

> If yes this happens to be our device here

You mean the PCI controller itself? It's at devfn 29, not 0.
In any case, the kernel doesn't write to PCI_INTERRUPT_LINE
for the controller, only for actual PCI cards.

> , so we could
> cleanly implement a config_write callback, like this:
>
>         pci_default_write_config
>         if (!ranger_cover_byte(addr, size, PCI_INTERRUPT_LINE))
>                 return;
>         if (dev->config[PCI_INTERRUPT_LINE] == 27) {
>                 s->irq_mapping = IRQ_MAPPING_BROKEN;
>         } else {
>                 s->irq_mapping = IRQ_MAPPING_FORCE_OK;
>         }
>
>
>
>> +            if (val == 27) {
>> +                s->irq_mapping = IRQ_MAPPING_BROKEN;
>> +            } else {
>> +                s->irq_mapping = IRQ_MAPPING_FORCE_OK;
>> +            }
>> +        }
>> +    }
>> +    pci_data_write(&s->pci_bus, vpb_pci_config_addr(addr), val, size);
>

thanks
-- PMM

Patch

diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 576e619..859fbee 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -13,6 +13,28 @@ 
 #include "hw/pci/pci_host.h"
 #include "exec/address-spaces.h"
 
+/* Old and buggy versions of QEMU used the wrong mapping from
+ * PCI IRQs to system interrupt lines. Unfortunately the Linux
+ * kernel also had the corresponding bug in setting up interrupts
+ * (so older kernels work on QEMU and not on real hardware).
+ * We automatically detect these broken kernels and flip back
+ * to the broken irq mapping by spotting guest writes to the
+ * PCI_INTERRUPT_LINE register to see where the guest thinks
+ * interrupts are going to be routed. So we start in state
+ * ASSUME_OK on reset, and transition to either BROKEN or
+ * FORCE_OK at the first write to an INTERRUPT_LINE register for
+ * a slot where broken and correct interrupt mapping would differ.
+ * Once in either BROKEN or FORCE_OK we never transition again;
+ * this allows a newer kernel to use the INTERRUPT_LINE
+ * registers arbitrarily once it has indicated that it isn't
+ * broken in its init code somewhere.
+ */
+enum {
+    IRQ_MAPPING_ASSUME_OK = 0,
+    IRQ_MAPPING_BROKEN = 1,
+    IRQ_MAPPING_FORCE_OK = 2,
+};
+
 typedef struct {
     PCIHostState parent_obj;
 
@@ -26,6 +48,9 @@  typedef struct {
 
     /* Constant for life of device: */
     int realview;
+
+    /* Variable state: */
+    uint8_t irq_mapping;
 } PCIVPBState;
 
 #define TYPE_VERSATILE_PCI "versatile_pci"
@@ -44,14 +69,27 @@  static inline uint32_t vpb_pci_config_addr(hwaddr addr)
 static void pci_vpb_config_write(void *opaque, hwaddr addr,
                                  uint64_t val, unsigned size)
 {
-    pci_data_write(opaque, vpb_pci_config_addr(addr), val, size);
+    PCIVPBState *s = (PCIVPBState *)opaque;
+    if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE
+        && s->irq_mapping == IRQ_MAPPING_ASSUME_OK) {
+        uint8_t devfn = addr >> 8;
+        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {
+            if (val == 27) {
+                s->irq_mapping = IRQ_MAPPING_BROKEN;
+            } else {
+                s->irq_mapping = IRQ_MAPPING_FORCE_OK;
+            }
+        }
+    }
+    pci_data_write(&s->pci_bus, vpb_pci_config_addr(addr), val, size);
 }
 
 static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr,
                                     unsigned size)
 {
+    PCIVPBState *s = (PCIVPBState *)opaque;
     uint32_t val;
-    val = pci_data_read(opaque, vpb_pci_config_addr(addr), size);
+    val = pci_data_read(&s->pci_bus, vpb_pci_config_addr(addr), size);
     return val;
 }
 
@@ -63,7 +101,47 @@  static const MemoryRegionOps pci_vpb_config_ops = {
 
 static int pci_vpb_map_irq(PCIDevice *d, int irq_num)
 {
-    return irq_num;
+    PCIVPBState *s = container_of(d->bus, PCIVPBState, pci_bus);
+
+    if (s->irq_mapping == IRQ_MAPPING_BROKEN) {
+        /* Legacy broken IRQ mapping for compatibility with old and
+         * buggy Linux guests
+         */
+        return irq_num;
+    }
+
+    /* Slot to IRQ mapping for RealView Platform Baseboard 926 backplane
+     *      name    slot    IntA    IntB    IntC    IntD
+     *      A       31      IRQ28   IRQ29   IRQ30   IRQ27
+     *      B       30      IRQ27   IRQ28   IRQ29   IRQ30
+     *      C       29      IRQ30   IRQ27   IRQ28   IRQ29
+     * Slot C is for the host bridge; A and B the peripherals.
+     * Our output irqs 0..3 correspond to the baseboard's 27..30.
+     *
+     * This mapping function takes account of an oddity in the PB926
+     * board wiring, where the FPGA's P_nINTA input is connected to
+     * the INTB connection on the board PCI edge connector, P_nINTB
+     * is connected to INTC, and so on, so everything is one number
+     * further round from where you might expect.
+     */
+    return pci_swizzle_map_irq_fn(d, irq_num + 2);
+}
+
+static int pci_vpb_rv_map_irq(PCIDevice *d, int irq_num)
+{
+    /* Slot to IRQ mapping for RealView EB and PB1176 backplane
+     *      name    slot    IntA    IntB    IntC    IntD
+     *      A       31      IRQ50   IRQ51   IRQ48   IRQ49
+     *      B       30      IRQ49   IRQ50   IRQ51   IRQ48
+     *      C       29      IRQ48   IRQ49   IRQ50   IRQ51
+     * Slot C is for the host bridge; A and B the peripherals.
+     * Our output irqs 0..3 correspond to the baseboard's 48..51.
+     *
+     * The PB1176 and EB boards don't have the PB926 wiring oddity
+     * described above; P_nINTA connects to INTA, P_nINTB to INTB
+     * and so on, which is why this mapping function is different.
+     */
+    return pci_swizzle_map_irq_fn(d, irq_num + 3);
 }
 
 static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
@@ -73,6 +151,13 @@  static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(pic[irq_num], level);
 }
 
+static void pci_vpb_reset(DeviceState *d)
+{
+    PCIVPBState *s = PCI_VPB(d);
+
+    s->irq_mapping = IRQ_MAPPING_ASSUME_OK;
+}
+
 static void pci_vpb_init(Object *obj)
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
@@ -95,13 +180,20 @@  static void pci_vpb_realize(DeviceState *dev, Error **errp)
 {
     PCIVPBState *s = PCI_VPB(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    pci_map_irq_fn mapfn;
     int i;
 
     for (i = 0; i < 4; i++) {
         sysbus_init_irq(sbd, &s->irq[i]);
     }
 
-    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, pci_vpb_map_irq, s->irq, 4);
+    if (s->realview) {
+        mapfn = pci_vpb_rv_map_irq;
+    } else {
+        mapfn = pci_vpb_map_irq;
+    }
+
+    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
 
     /* ??? Register memory space.  */
 
@@ -110,10 +202,10 @@  static void pci_vpb_realize(DeviceState *dev, Error **errp)
      * 1 : PCI config window
      * 2 : PCI IO window
      */
-    memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, &s->pci_bus,
+    memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, s,
                           "pci-vpb-selfconfig", 0x1000000);
     sysbus_init_mmio(sbd, &s->mem_config);
-    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, &s->pci_bus,
+    memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, s,
                           "pci-vpb-config", 0x1000000);
     sysbus_init_mmio(sbd, &s->mem_config2);
 
@@ -159,6 +251,7 @@  static void pci_vpb_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pci_vpb_realize;
+    dc->reset = pci_vpb_reset;
 }
 
 static const TypeInfo pci_vpb_info = {