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

[Marcel Apfelbaum]

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.
2. If the PCI device is not behind the bridge, directly
   modifies the device status register
Regarding PCI express, I'll check this issue.
Thanks,
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
> > > > 
> > > > 
> > 
> >