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 <marce...@redhat.com>
> >
> > 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.
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.
> >
> > > ---
> > > 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.
> >
> > > + 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?
Maybe if you split each case properly it will become
clear.
> >
> > > + 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.
> 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.
> >
> > > + 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
>
>
