Re: [Qemu-devel] [PATCH RFC v2 2/2] hw/pci: handle unassigned pci addresses

[Michael S. Tsirkin]

On Mon, Sep 09, 2013 at 03:11:55PM +0300, Marcel Apfelbaum wrote:
> On Mon, 2013-09-09 at 14:40 +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 09, 2013 at 02:11:54PM +0300, Marcel Apfelbaum wrote:
> > > Created a MemoryRegion with negative priority that
> > > spans over all the pci address space.
> > > It "intercepts" the accesses to unassigned pci
> > > address space and will follow the pci spec:
> > >  1. returns -1 on read
> > >  2. does nothing on write
> > >  3. sets the RECEIVED MASTER ABORT bit in the STATUS register
> > >     of the device that initiated the transaction
> > > 
> > > Note: This implementation assumes that all the reads/writes to
> > > the pci address space are done by the cpu.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > > ---
> > > Changes from v1:
> > >  - "pci-unassigned-mem" MemoryRegion resides now in PCIBus and not on
> > >     various Host Bridges
> > >  - "pci-unassgined-mem" does not have a ".valid.accept" field and
> > >     implements read write methods
> > > 
> > >  hw/pci/pci.c             | 46 
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/pci/pci_bus.h |  1 +
> > >  2 files changed, 47 insertions(+)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index d00682e..b6a8026 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -283,6 +283,43 @@ const char *pci_root_bus_path(PCIDevice *dev)
> > >      return rootbus->qbus.name;
> > >  }
> > >  
> > > +static void unassigned_mem_access(PCIBus *bus)
> > > +{
> > > +    /* FIXME assumption: memory access to the pci address
> > > +     * space is always initiated by the host bridge
> > 
> > /* Always
> >  * like
> >  * this
> >  */
> > 
> > /* Not
> >  * like this */
> OK
> 
> > 
> > > +     * (device 0 on the bus) */
> > 
> > Can't we pass the master device in?
> > We are still left with the assumption that
> > there's a single master, but at least
> > we get rid of the assumption that it's
> > always device 0 function 0.
> > 
> > 
> > > +    PCIDevice *d = bus->devices[0];
> > > +    if (!d) {
> > > +        return;
> > > +    }
> > > +
> > > +    pci_word_test_and_set_mask(d->config + PCI_STATUS,
> > > +                               PCI_STATUS_REC_MASTER_ABORT);
> > 
> > Probably should check and set secondary status for
> > a bridge device.
> I wanted to add the support for bridges later,
> I am struggling with some issues with PCI bridges.

Shouldn't be very different.

> > 
> > > +}
> > > +
> > > +static uint64_t unassigned_mem_read(void *opaque, hwaddr addr, unsigned 
> > > size)
> > > +{
> > > +    PCIBus *bus = opaque;
> > > +    unassigned_mem_access(bus);
> > > +
> > > +    return -1ULL;
> > > +}
> > > +
> > > +static void unassigned_mem_write(void *opaque, hwaddr addr, uint64_t val,
> > > +                                unsigned size)
> > > +{
> > > +    PCIBus *bus = opaque;
> > > +    unassigned_mem_access(bus);
> > > +}
> > > +
> > > +static const MemoryRegionOps unassigned_mem_ops = {
> > > +    .read = unassigned_mem_read,
> > > +    .write = unassigned_mem_write,
> > > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > > +};
> > > +
> > > +#define UNASSIGNED_MEM_PRIORITY -1
> > > +
> > >  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> > >                           const char *name,
> > >                           MemoryRegion *address_space_mem,
> > 
> > 
> > I would rename "unassigned" to "pci_master_Abort_" everywhere.
> Are you sure about the capital "A"?

Ugh no, that's a typo.

> 
> For the memory ops I suppose is OK, but the MemoryRegion name
> I think it should remain "unassigned_mem"; it will be strange
> to name it pci_master_Abort_mem.

Why would it be strange?

> > 
> > Call things by what they do not how they are used.
> > 
> > > @@ -294,6 +331,15 @@ 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->unassigned_mem, OBJECT(bus),
> > > +                          &unassigned_mem_ops, bus, "pci-unassigned",
> > > +                          memory_region_size(bus->address_space_mem));
> > > +    memory_region_add_subregion_overlap(bus->address_space_mem,
> > > +                                        bus->address_space_mem->addr,
> > > +                                        &bus->unassigned_mem,
> > > +                                        UNASSIGNED_MEM_PRIORITY);
> > > +
> > >      /* host bridge */
> > >      QLIST_INIT(&bus->child);
> > >
> > 
> > It seems safer to add an API for enabling this functionality.
> > Something like
> > 
> > pci_set_master_for_master_abort(PCIBus *, PCIDevice *);
> I started to implement that way, but it would be strange (I think) 
> to see in the code the above method because almost all the pci devices
> can be master devices.

In theory yes in practice no one programs devices
with addresses that trigger master abort.

Addressing this theorectical issue would require
memory ops to get the bus master pointer.
When this happens the new API can go away,
but we can do this gradually.

> I selected an "implicit" implementation so for this reason exactly.
> We do not really select a master, but a "master for writes/reads on
> pci address space". Selecting device 0 on the bus automatically for
> this makes more sense to me.

No because you don't know that device 0 function 0 is the
pci host bridge.

And this won't work for bridges at all.

> What do you think?
> Marcel 
> 
> > 
> >   
> > > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > > index 9df1788..4cc25a3 100644
> > > --- a/include/hw/pci/pci_bus.h
> > > +++ b/include/hw/pci/pci_bus.h
> > > @@ -23,6 +23,7 @@ struct PCIBus {
> > >      PCIDevice *parent_dev;
> > >      MemoryRegion *address_space_mem;
> > >      MemoryRegion *address_space_io;
> > > +    MemoryRegion unassigned_mem;
> > >  
> > >      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
> > 
>