Re: [Qemu-devel] [PATCH] hw/pci: completed master-abort emulation

[Michael S. Tsirkin]

On Tue, Sep 24, 2013 at 11:07:19AM +0300, Marcel Apfelbaum wrote:
> On Mon, 2013-09-23 at 21:45 +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 23, 2013 at 08:49:53PM +0300, Marcel Apfelbaum wrote:
> > > On Mon, 2013-09-23 at 18:10 +0300, Michael S. Tsirkin wrote:
> > > > On Mon, Sep 23, 2013 at 05:43:38PM +0300, Marcel Apfelbaum wrote:
> > > > > On Mon, 2013-09-23 at 16:45 +0300, Michael S. Tsirkin wrote:
> > > > > > On Mon, Sep 23, 2013 at 03:37:43PM +0300, Marcel Apfelbaum wrote:
> > > > > > > On Mon, 2013-09-23 at 14:27 +0300, Michael S. Tsirkin wrote:
> > > > > > > > On Mon, Sep 23, 2013 at 02:01:17PM +0300, Marcel Apfelbaum 
> > > > > > > > wrote:
> > > > > > > > > This patch is implemented on top of series:
> > > > > > > > > [PATCH v5 0/3] pci: implement upstream master abort protocol
> > > > > > > > > 
> > > > > > > > > Added "master abort io" background region for PCIBus.
> > > > > > > > > 
> > > > > > > > > Added "master abort mem" region to catch transactions 
> > > > > > > > > initiated
> > > > > > > > > by pci devices targeted to unassigned addresses.
> > > > > > > > > 
> > > > > > > > > Enabled "master abort" regions for PCI-2-PCI bridge's 
> > > > > > > > > secondary bus.
> > > > > > > > > 
> > > > > > > > > Set "Received Master Abort" Bit on Status/Secondary Status
> > > > > > > > > register as defined in the PCI Spec.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > > > > > > > 
> > > > > > > > I think it will be easier to review the complete
> > > > > > > > series, not an incremental patch.
> > > > > > > Should I combine this with the prev series without the
> > > > > > > memory patches?
> > > > > > > 
> > > > > > > > 
> > > > > > > > We probably need to think what's the right thing
> > > > > > > > to do for master abort mode since we do not
> > > > > > > > emulate SERR. Do we make it writeable at all?
> > > > > > > Master abort mode can be enabled. (Tested)
> > > > > > > > 
> > > > > > > > It's a pci only bit:
> > > > > > > >         When the Master-Abort Mode bit is cleared and a posted 
> > > > > > > > write transaction
> > > > > > > >         forwarded by the
> > > > > > > >         bridge terminates with a Master-Abort, no error is 
> > > > > > > > reported (note that
> > > > > > > >         the Master-Abort bit
> > > > > > > >         is still set). When the Master-Abort Mode bit is set 
> > > > > > > > and a posted write
> > > > > > > >         transaction forwarded
> > > > > > > >         by the bridge terminates with a Master-Abort on the 
> > > > > > > > destination bus, the
> > > > > > > >         bridge must:
> > > > > > > >         Assert SERR# on the primary interfaceCommand register)
> > > > > > > >         (if enabled by the SERR# Enable bitin the
> > > > > > > >         Set the Signaled System Errorbit in the Command 
> > > > > > > > register)
> > > > > > > >         bitin the Status register (if enabled by the SERR# 
> > > > > > > > Enable)
> > > > > > > > 
> > > > > > > > one way to do this would be not to set Master-Abort bit even
> > > > > > > > though spec says we should, and pretend there was no error.
> > > > > > > Maybe we can just not allow to set "Master abort mode"
> > > > > > > in BRIDGE_CONTROL register. It is a cleaner way (I think)
> > > > > > > considering we don't emulate SERR.
> > > > > > 
> > > > > > I'm not sure P2P spec allows this - want to check.
> > > > > I will check this too, thanks
> > > > > 
> > > > > > It's the right thing to do for express.
> > > > > > 
> > > > > > Also pls note that cross-version migration
> > > > > > requires that we keep it writable for old
> > > > > > machine types.
> > > > > Yes :(. It seems we need another solution.
> > > > 
> > > > Not really. Just set a flag "don't assert master abort"
> > > > for old machine types.
> > > OK, thanks
> > > 
> > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >  hw/pci/pci.c             | 115 
> > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > > >  hw/pci/pci_bridge.c      |  10 +++++
> > > > > > > > >  include/hw/pci/pci.h     |   3 ++
> > > > > > > > >  include/hw/pci/pci_bus.h |   1 +
> > > > > > > > >  4 files changed, 118 insertions(+), 11 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > > > > > index d8a1b11..1f4e707 100644
> > > > > > > > > --- a/hw/pci/pci.c
> > > > > > > > > +++ b/hw/pci/pci.c
> > > > > > > > > @@ -283,17 +283,76 @@ const char *pci_root_bus_path(PCIDevice 
> > > > > > > > > *dev)
> > > > > > > > >      return rootbus->qbus.name;
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > > +static PCIDevice *pci_bus_find_host(PCIBus *bus)
> > > > > > > > > +{
> > > > > > > > > +    PCIDevice *dev;
> > > > > > > > > +    int i;
> > > > > > > > > +
> > > > > > > > > +    for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> > > > > > > > > +        dev = bus->devices[i];
> > > > > > > > > +        if (dev) {
> > > > > > > > > +            PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > > > > > > > > +            if (pc->class_id == PCI_CLASS_BRIDGE_HOST) {
> > > > > > > > 
> > > > > > > > In fact you can do this easier:
> > > > > > > >          pci_get_word(dev->config + PCI_CLASS_DEVICE) == 
> > > > > > > > PCI_CLASS_BRIDGE_HOST
> > > > > > > Thanks, I'll change.
> > > > > > > > 
> > > > > > > > > +                return dev;
> > > > > > > > > +            }
> > > > > > > > > +        }
> > > > > > > > > +    }
> > > > > > > > > +
> > > > > > > > > +    return NULL;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void master_abort(void *opaque)
> > > > > > > > > +{
> > > > > > > > > +    PCIDevice *master_dev = NULL;
> > > > > > > > > +    PCIDeviceClass *pc;
> > > > > > > > > +    PCIBus *bus;
> > > > > > > > > +    int downstream;
> > > > > > > > 
> > > > > > > > bool?
> > > > > > > Yes, I'll change
> > > > > > > 
> > > > > > > > > +
> > > > > > > > > +    if (object_dynamic_cast(OBJECT(opaque), 
> > > > > > > > > TYPE_PCI_DEVICE)) {
> > > > > > > > 
> > > > > > > > Please don't do dynamic cast just because you can.
> > > > > > > > You know very well what kind of object you pass
> > > > > > > > when you create the MR.
> > > > > > > > So just call the appropriate function.
> > > > > > > I wanted to merge the code for all 3 entities involved:
> > > > > > > root PCIBus, PCIBus behind a bridge, and PCIDevice.
> > > > > > > The region for merge was to use the same master_abort_mem_ops
> > > > > > > because there are the same operations that must be done:
> > > > > > > return -1 (0xFFF...) and set master abort received bit. 
> > > > > > 
> > > > > > return -1 is not a lot of common code, and MA bit
> > > > > > is in a different place each time.
> > > > > Ack
> > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > +        master_dev = PCI_DEVICE(opaque);
> > > > > > > > > +        bus = master_dev->bus;
> > > > > > > > > +        while (!pci_bus_is_root(bus)) {
> > > > > > > > > +            master_dev = bus->parent_dev;
> > > > > > > > > +            bus = master_dev->bus;
> > > > > > > > > +        }
> > > > > > > > > +        downstream = 0;
> > > > > > > > > +    }
> > > > > > > > > +
> > > > > > > > > +    if (object_dynamic_cast(OBJECT(opaque), TYPE_PCI_BUS)) {
> > > > > > > > > +        bus = PCI_BUS(opaque);
> > > > > > > > > +        if (pci_bus_is_root(bus)) {
> > > > > > > > > +            master_dev = pci_bus_find_host(bus);
> > > > > > > > > +        } else { /* bus behind a PCI-2-PCI bridge */
> > > > > > > > > +            master_dev = bus->parent_dev;
> > > > > > > > > +        }
> > > > > > > > 
> > > > > > > > So the reason for this is that device to device MA
> > > > > > > > is still not implemented here, correct?
> > > > > > > Nope, the above code differentiate 2 scenarios:
> > > > > > > 1. the bus is the root bus => look for host bridge
> > > > > > >    to update STATUS register
> > > > > > > 2. the bus is bridge's secondary bus =>
> > > > > > >    update bridge STATUS register
> > > > > > > "Device 2 Device" is handled implicitly because we have
> > > > > > > a background region covering all device's target memory
> > > > > > > 
> > > > > > > > If it was it would be just one instance of it.
> > > > > > > > I'm not saying this must block this patch, however
> > > > > > > > there should be a code comment to this effect,
> > > > > > > > and commit log should mention this explicitly.
> > > > > > > As above, "Device 2 Device" is handled implicit
> > > > > > 
> > > > > > Implicit => confused.
> > > > > > 
> > > > > > I'm confused. Where's the code that sets MA bit on
> > > > > > the initiator device?
> > > > > Is the code that adds the background bus_master_abort_mem region to
> > > > > bus_master_enable_region.
> > > > > bus_master_enable_region is the target for all PCIDevice
> > > > > reads and writes(if I understand correctly). So it doesn't matter if
> > > > > a device initiate a transaction to another device or to memory,
> > > > > it will do it using the bus_master_enable_region.
> > > > > The master_abort() method will recognize this as an
> > > > > upstream transaction. 
> > > > 
> > > > But if target is behind a bridge then master does
> > > > not detect the MA, it's the bridge that does it
> > > > (in PCI, express seems to be different).
> > > The current code handles upstream transactions as follows:
> > > 1. If the PCI device is behind a bridge, finds
> > >    recursively the corresponding bridge
> > >    on the root bus and modifies its status register.
> > 
> > Why recursively?
> > I think you need to set MA bit on the bit in bridge right
> > above the device.
> If a PCI Device behind a bridge issues a transaction:
>  - From my understanding, any address outside the bridge address range
>    will be claimed by the bridge.

By parent bridge, yes.

>  - This will happen recursively until we reach the root bus (assuming
>    that the target is not another device).

Not necessarily. Another bridge can claim it then
terminate with MA.

Example:

-[0000:00]-+-00.0
           +-02.0
           +-16.0
           +-16.3
           +-19.0
           +-1a.0
           +-1b.0
           +-1c.0-[02]--
           +-1c.1-[03]----00.0
           +-1c.3-[05-0c]--
           +-1c.4-[0d]--+-00.0
           |            \-00.3



Device 03:00.0 writes to within memory window of 00:1c.4,
but outside BARs of both 0d:00.0 and 0d:00.3.

On PCI, I think MA is set in sec status register of 00:1c.4.


>  - The bridge on the bus will set MA bit.


See above.

> > 
> > > 2. If the PCI device is not behind the bridge, directly
> > >    modifies the device status register
> > 
> > 
> > The issue really isn't whether the PCI device
> > (which PCI device?) is behind the bridge.
> > Rather it's whether the address is claimed by some bridge
> > on the same bus as the master, or by a parent bridge.
> This can be recursive, no?
> 
> > 
> > If it is not claimed, MA bit in the original master
> > is set.
> > 
> > If it is claimed, master becomes some bridge:
> > either bridge on the secondary bus (forwarding downstream) or a parent 
> > bridge
> > (forwarding upstream) of the bus, then repeat the logic, until we find a
> > bus where it is not claimed.
> I agree, the "repeat logic" is the response to your question "Why 
> recursively?"
> 
> > 
> > At that point to know where in bridge config to set the bit, look at the 
> > last step we
> > took.  If it was a parent bus (transaction was forwarded upstream) set
> > primary status, otherwise (it was a bridge on the secondary bus)
> > set secondary status.
> > 
> > The last piece of puzzle is that for transactions coming from
> > outside the PCI domain, pci host bridge is the original master.
> > 
> > 
> > This applies to memory,io and configuration cycles.
> > 
> > This is for PCI.
> I agree.
> 
> > 
> > > Regarding PCI express, I'll check this issue.
> > > Thanks,
> > > Marcel
> > 
> > As far as I can tell, for express instead of the last bridge
> > detecting UR (and setting MA), all bridges on the path detect UR and the
> > original master detects UR, in all cases.
> Thanks
> 
> > 
> > Also, UR apparently can only be detected by non-posted transactions:
> > mem/io reads and configuration. Even on the same "bus".
> Also "Posted Write Transactions", PCI-2-PCI bridge Spec (6.3.2)
> How can we differentiate in Qemu posted from non-posted transcations
> anyway?
> 
> > 
> > If you cross a pci to express bridge (direct bridge)
> > then the bridge is the original master and
> > UR is detected on path from it on.
> > 
> > If you cross an express to pci bridge (reversed bridge)
> > you lose this info and only the last master sets MA.
> Thanks, I'll look into it,
> Marcel
> 
> > 
> > > > 
> > > > 
> > > > > > Maybe if you split each case properly it will become
> > > > > > clear.
> > > > > I hope it will.
> > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > +        downstream = 1;
> > > > > > > > > +    }
> > > > > > > > > +
> > > > > > > > > +    assert(master_dev);
> > > > > > > > > +    pc = PCI_DEVICE_GET_CLASS(master_dev);
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > There's very little common code between devices and
> > > > > > > > buses. So just use 2 functions.
> > > > > > > As stated above:
> > > > > > > The region for merge was to use the same master_abort_mem_ops
> > > > > > > because there are the same operations that must be done:
> > > > > > > return -1 (0xFFF...) and set master abort received bit.
> > > > > > > Separating them will result in some code duplication.
> > > > > > 
> > > > > > About 2 lines. But you'll lose the nasty dynamic casts.
> > > > > > opaque pointers are bad enough.
> > > > > Ack
> > > > > 
> > > > > > 
> > > > > > > The read and write methods are the same, but would call different
> > > > > > > "master_abort" methods.
> > > > > > > If it does bother you the down casting anyway, I'll change.
> > > > > > 
> > > > > > Pls do.
> > > > > Ack
> > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > +    if (downstream && pc->is_bridge) {
> > > > > > > > > +        pci_word_test_and_set_mask(master_dev->config + 
> > > > > > > > > PCI_SEC_STATUS,
> > > > > > > > > +                                   
> > > > > > > > > PCI_STATUS_REC_MASTER_ABORT);
> > > > > > > > > +    } else {
> > > > > > > > > +        pci_word_test_and_set_mask(master_dev->config + 
> > > > > > > > > PCI_STATUS,
> > > > > > > > > +                                   
> > > > > > > > > PCI_STATUS_REC_MASTER_ABORT);
> > > > > > > > > +    }
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static uint64_t master_abort_mem_read(void *opaque, hwaddr 
> > > > > > > > > addr, unsigned size)
> > > > > > > > >  {
> > > > > > > > > -   return -1ULL;
> > > > > > > > 
> > > > > > > > I didn't notice but this means there were 3 spaces here in 
> > > > > > > > previous
> > > > > > > > patch?
> > > > > > > yes :(, checkpatch didn't catch it
> > > > > > > 
> > > > > > > Thanks for the review,
> > > > > > > Marcel
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +    master_abort(opaque);
> > > > > > > > > +    return -1ULL;
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > >  static void master_abort_mem_write(void *opaque, hwaddr 
> > > > > > > > > addr, uint64_t val,
> > > > > > > > >                                     unsigned size)
> > > > > > > > >  {
> > > > > > > > > +    master_abort(opaque);
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > > -static const MemoryRegionOps master_abort_mem_ops = {
> > > > > > > > > +static const MemoryRegionOps master_abort_ops = {
> > > > > > > > >      .read = master_abort_mem_read,
> > > > > > > > >      .write = master_abort_mem_write,
> > > > > > > > >      .endianness = DEVICE_LITTLE_ENDIAN,
> > > > > > > > > @@ -301,6 +360,23 @@ static const MemoryRegionOps 
> > > > > > > > > master_abort_mem_ops = {
> > > > > > > > >  
> > > > > > > > >  #define MASTER_ABORT_MEM_PRIORITY INT_MIN
> > > > > > > > >  
> > > > > > > > > +void pci_bus_master_abort_init(PCIBus *bus, const char *mem, 
> > > > > > > > > const char *io)
> > > > > > > > > +{
> > > > > > > > > +    memory_region_init_io(&bus->master_abort_mem, 
> > > > > > > > > OBJECT(bus),
> > > > > > > > > +                          &master_abort_ops, bus, mem,
> > > > > > > > > +                          
> > > > > > > > > memory_region_size(bus->address_space_mem));
> > > > > > > > > +    
> > > > > > > > > memory_region_add_subregion_overlap(bus->address_space_mem,
> > > > > > > > > +                                        0, 
> > > > > > > > > &bus->master_abort_mem,
> > > > > > > > > +                                        
> > > > > > > > > MASTER_ABORT_MEM_PRIORITY);
> > > > > > > > > +
> > > > > > > > > +    memory_region_init_io(&bus->master_abort_io, OBJECT(bus),
> > > > > > > > > +                          &master_abort_ops, bus, io,
> > > > > > > > > +                          
> > > > > > > > > memory_region_size(bus->address_space_io));
> > > > > > > > > +    
> > > > > > > > > memory_region_add_subregion_overlap(bus->address_space_io,
> > > > > > > > > +                                        0, 
> > > > > > > > > &bus->master_abort_io,
> > > > > > > > > +                                        
> > > > > > > > > MASTER_ABORT_MEM_PRIORITY);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> > > > > > > > >                           const char *name,
> > > > > > > > >                           MemoryRegion *address_space_mem,
> > > > > > > > > @@ -312,13 +388,8 @@ static void pci_bus_init(PCIBus *bus, 
> > > > > > > > > DeviceState *parent,
> > > > > > > > >      bus->address_space_mem = address_space_mem;
> > > > > > > > >      bus->address_space_io = address_space_io;
> > > > > > > > >  
> > > > > > > > > -
> > > > > > > > > -    memory_region_init_io(&bus->master_abort_mem, 
> > > > > > > > > OBJECT(bus),
> > > > > > > > > -                          &master_abort_mem_ops, bus, 
> > > > > > > > > "pci-master-abort",
> > > > > > > > > -                          
> > > > > > > > > memory_region_size(bus->address_space_mem));
> > > > > > > > > -    
> > > > > > > > > memory_region_add_subregion_overlap(bus->address_space_mem,
> > > > > > > > > -                                        0, 
> > > > > > > > > &bus->master_abort_mem,
> > > > > > > > > -                                        
> > > > > > > > > MASTER_ABORT_MEM_PRIORITY);
> > > > > > > > > +    pci_bus_master_abort_init(bus, "pci-master-abort-mem",
> > > > > > > > > +                              "pci-master-abort-io");
> > > > > > > > >  
> > > > > > > > >      /* host bridge */
> > > > > > > > >      QLIST_INIT(&bus->child);
> > > > > > > > > @@ -840,9 +911,24 @@ static PCIDevice 
> > > > > > > > > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> > > > > > > > >      pci_dev->bus = bus;
> > > > > > > > >      dma_as = pci_device_iommu_address_space(pci_dev);
> > > > > > > > >  
> > > > > > > > > -    
> > > > > > > > > memory_region_init_alias(&pci_dev->bus_master_enable_region,
> > > > > > > > > -                             OBJECT(pci_dev), "bus master",
> > > > > > > > > +    memory_region_init(&pci_dev->bus_master_enable_region, 
> > > > > > > > > NULL,
> > > > > > > > > +                       "bus master", UINT64_MAX);
> > > > > > > > > +
> > > > > > > > > +    memory_region_init_io(&pci_dev->bus_master_abort_mem, 
> > > > > > > > > OBJECT(bus),
> > > > > > > > > +                          &master_abort_ops, OBJECT(pci_dev),
> > > > > > > > > +                          "bus master master-abort",
> > > > > > > > > +                          
> > > > > > > > > memory_region_size(&pci_dev->bus_master_enable_region));
> > > > > > > > > +    
> > > > > > > > > memory_region_add_subregion_overlap(&pci_dev->bus_master_enable_region,
> > > > > > > > > +                                        0, 
> > > > > > > > > &pci_dev->bus_master_abort_mem,
> > > > > > > > > +                                        
> > > > > > > > > MASTER_ABORT_MEM_PRIORITY);
> > > > > > > > > +
> > > > > > > > > +    memory_region_init_alias(&pci_dev->bus_master_dma_mem,
> > > > > > > > > +                             OBJECT(pci_dev), "bus master 
> > > > > > > > > dma",
> > > > > > > > >                               dma_as->root, 0, 
> > > > > > > > > memory_region_size(dma_as->root));
> > > > > > > > > +    
> > > > > > > > > memory_region_add_subregion_overlap(&pci_dev->bus_master_enable_region,
> > > > > > > > > +                                        0, 
> > > > > > > > > &pci_dev->bus_master_dma_mem, 0);
> > > > > > > > > +
> > > > > > > > > +
> > > > > > > > >      
> > > > > > > > > memory_region_set_enabled(&pci_dev->bus_master_enable_region, 
> > > > > > > > > false);
> > > > > > > > >      address_space_init(&pci_dev->bus_master_as, 
> > > > > > > > > &pci_dev->bus_master_enable_region,
> > > > > > > > >                         name);
> > > > > > > > > @@ -901,6 +987,13 @@ static void 
> > > > > > > > > do_pci_unregister_device(PCIDevice *pci_dev)
> > > > > > > > >      pci_config_free(pci_dev);
> > > > > > > > >  
> > > > > > > > >      address_space_destroy(&pci_dev->bus_master_as);
> > > > > > > > > +    
> > > > > > > > > memory_region_del_subregion(&pci_dev->bus_master_enable_region,
> > > > > > > > > +                                
> > > > > > > > > &pci_dev->bus_master_dma_mem);
> > > > > > > > > +    
> > > > > > > > > memory_region_del_subregion(&pci_dev->bus_master_enable_region,
> > > > > > > > > +                                
> > > > > > > > > &pci_dev->bus_master_abort_mem);
> > > > > > > > > +    memory_region_destroy(&pci_dev->bus_master_dma_mem);
> > > > > > > > > +    memory_region_destroy(&pci_dev->bus_master_abort_mem);
> > > > > > > > > +
> > > > > > > > >      
> > > > > > > > > memory_region_destroy(&pci_dev->bus_master_enable_region);
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > > > > > > > > index e6b22b8..56b682f 100644
> > > > > > > > > --- a/hw/pci/pci_bridge.c
> > > > > > > > > +++ b/hw/pci/pci_bridge.c
> > > > > > > > > @@ -376,6 +376,10 @@ int pci_bridge_initfn(PCIDevice *dev, 
> > > > > > > > > const char *typename)
> > > > > > > > >      sec_bus->address_space_io = &br->address_space_io;
> > > > > > > > >      memory_region_init(&br->address_space_io, OBJECT(br), 
> > > > > > > > > "pci_bridge_io", 65536);
> > > > > > > > >      br->windows = pci_bridge_region_init(br);
> > > > > > > > > +
> > > > > > > > > +    pci_bus_master_abort_init(sec_bus, 
> > > > > > > > > "pci_bridge_master_abort_mem",
> > > > > > > > > +                              "pci_bridge_master_abort_io");
> > > > > > > > > +
> > > > > > > > >      QLIST_INIT(&sec_bus->child);
> > > > > > > > >      QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> > > > > > > > >      return 0;
> > > > > > > > > @@ -389,6 +393,12 @@ void pci_bridge_exitfn(PCIDevice 
> > > > > > > > > *pci_dev)
> > > > > > > > >      QLIST_REMOVE(&s->sec_bus, sibling);
> > > > > > > > >      pci_bridge_region_del(s, s->windows);
> > > > > > > > >      pci_bridge_region_cleanup(s, s->windows);
> > > > > > > > > +    memory_region_del_subregion(s->sec_bus.address_space_mem,
> > > > > > > > > +                                
> > > > > > > > > &s->sec_bus.master_abort_mem);
> > > > > > > > > +    memory_region_del_subregion(s->sec_bus.address_space_io,
> > > > > > > > > +                                &s->sec_bus.master_abort_io);
> > > > > > > > > +    memory_region_destroy(&s->sec_bus.master_abort_mem);
> > > > > > > > > +    memory_region_destroy(&s->sec_bus.master_abort_io);
> > > > > > > > >      memory_region_destroy(&s->address_space_mem);
> > > > > > > > >      memory_region_destroy(&s->address_space_io);
> > > > > > > > >      /* qbus_free() is called automatically by qdev_free() */
> > > > > > > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > > > > > > > index 37979aa..d69e06d 100644
> > > > > > > > > --- a/include/hw/pci/pci.h
> > > > > > > > > +++ b/include/hw/pci/pci.h
> > > > > > > > > @@ -242,6 +242,8 @@ struct PCIDevice {
> > > > > > > > >      PCIIORegion io_regions[PCI_NUM_REGIONS];
> > > > > > > > >      AddressSpace bus_master_as;
> > > > > > > > >      MemoryRegion bus_master_enable_region;
> > > > > > > > > +    MemoryRegion bus_master_dma_mem;
> > > > > > > > > +    MemoryRegion bus_master_abort_mem;
> > > > > > > > >  
> > > > > > > > >      /* do not access the following fields */
> > > > > > > > >      PCIConfigReadFunc *config_read;
> > > > > > > > > @@ -357,6 +359,7 @@ PCIBus *pci_bus_new(DeviceState *parent, 
> > > > > > > > > const char *name,
> > > > > > > > >                      MemoryRegion *address_space_mem,
> > > > > > > > >                      MemoryRegion *address_space_io,
> > > > > > > > >                      uint8_t devfn_min, const char *typename);
> > > > > > > > > +void pci_bus_master_abort_init(PCIBus *bus, const char *mem, 
> > > > > > > > > const char *io);
> > > > > > > > >  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, 
> > > > > > > > > pci_map_irq_fn map_irq,
> > > > > > > > >                    void *irq_opaque, int nirq);
> > > > > > > > >  int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> > > > > > > > > diff --git a/include/hw/pci/pci_bus.h 
> > > > > > > > > b/include/hw/pci/pci_bus.h
> > > > > > > > > index 2ad5edb..57b1541 100644
> > > > > > > > > --- a/include/hw/pci/pci_bus.h
> > > > > > > > > +++ b/include/hw/pci/pci_bus.h
> > > > > > > > > @@ -24,6 +24,7 @@ struct PCIBus {
> > > > > > > > >      MemoryRegion *address_space_mem;
> > > > > > > > >      MemoryRegion *address_space_io;
> > > > > > > > >      MemoryRegion master_abort_mem;
> > > > > > > > > +    MemoryRegion master_abort_io;
> > > > > > > > >  
> > > > > > > > >      QLIST_HEAD(, PCIBus) child; /* this will be replaced by 
> > > > > > > > > qdev later */
> > > > > > > > >      QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by 
> > > > > > > > > qdev later */
> > > > > > > > > -- 
> > > > > > > > > 1.8.3.1
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> 
>