qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH RFC v2 2/2] hw/pci: handle unassigned pci addresses
Date: Mon, 09 Sep 2013 15:43:49 +0300

On Mon, 2013-09-09 at 15:23 +0300, Michael S. Tsirkin wrote:
> 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.
The current implementation uses only one MemoryRegion that spans
over all pci address space. It catches all the accesses both to
primary bus addresses (bus 0), or to the regions covered by the bridges.
The callback ALWAYS receive a pointer to the primary bus.

What would be a better approach?
1. Remain with a single MemoryRegion and check if the address
   belongs to a bridge address range and recursively travel
   the bridges tree and update the registers
2. Model a MemoryRegion for each bridge that represents
   the address range behind the bridge (not existing today).
   Add a MemoryRegion child to catch accesses to unassigned
   addresses behind the bridge, then recursively travel the
   bridges to top and update the registers. 

> 
> > > 
> > > > +}
> > > > +
> > > > +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?
1. Because it states what happens when there is a an access to
   unassigned memory and not that IS an unassigned memory.
2. This name is already used for unassigned IO ports and
   maybe is better to follow this convension
> 
> > > 
> > > 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.
The scenario is covered only for the primary bus and not for buses
behind the PCI bridge (the later being handled differently.)
In this case, isn't the Host Bridge always device 00.0?
If not, can we find a way to scan all bus devices and find
the host bridge so we will not have to manually add it to each
host bridge?

> 
> And this won't work for bridges at all.
For bridges I follow a different path, please see above. 

Thanks!
Marcel

> 
> > 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
> > > 
> > 





reply via email to

[Prev in Thread] Current Thread [Next in Thread]