Re: [Qemu-devel] [PATCH RFC v2 2/9] hw/pci: add pci wrappers for allocating and asserting irqs

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Wed, 2013-10-02 at 15:54 +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 02, 2013 at 03:41:27PM +0300, Marcel Apfelbaum wrote:
> > Interrupt pin is selected and saved into PCI_INTERRUPT_PIN
> > register during device initialization. Devices should not call
> > directly qemu_set_irq and specify the INTx pin on each call.
> > 
> > Added pci_* wrappers to replace qemu_set_irq, qemu_irq_raise,
> > qemu_irq_lower and qemu_irq_pulse, setting the irq
> > based on PCI_INTERRUPT_PIN.
> > 
> > Added pci_allocate_irq wrapper to be used by devices that
> > still need PCIDevice infrastructure to assert irqs.
> > 
> > Renamed a static method which was named already pci_set_irq.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > Changes from v1:
> >  - Added pci irq wrappers for all qemu methods
> >    setting irq and not only qemu_set_irq 
> >  - Added pci wrappers to allocate and pci irq
> > 
> >  hw/pci/pci.c         | 26 ++++++++++++++++++++++----
> >  include/hw/pci/pci.h | 19 +++++++++++++++++++
> >  2 files changed, 41 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 00554a0..fbfd8f7 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -83,7 +83,7 @@ static const TypeInfo pcie_bus_info = {
> >  
> >  static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
> >  static void pci_update_mappings(PCIDevice *d);
> > -static void pci_set_irq(void *opaque, int irq_num, int level);
> > +static void pci_irq_handler(void *opaque, int irq_num, int level);
> >  static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom);
> >  static void pci_del_option_rom(PCIDevice *pdev);
> >  
> > @@ -161,7 +161,7 @@ void pci_device_deassert_intx(PCIDevice *dev)
> >  {
> >      int i;
> >      for (i = 0; i < PCI_NUM_PINS; ++i) {
> > -        qemu_set_irq(dev->irq[i], 0);
> > +        pci_irq_handler(dev, i, 0);
> >      }
> >  }
> >  
> > @@ -863,7 +863,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >      pci_dev->config_read = config_read;
> >      pci_dev->config_write = config_write;
> >      bus->devices[devfn] = pci_dev;
> > -    pci_dev->irq = qemu_allocate_irqs(pci_set_irq, pci_dev, PCI_NUM_PINS);
> > +    pci_dev->irq = qemu_allocate_irqs(pci_irq_handler, pci_dev, PCI_NUM_PINS);
> >      pci_dev->version_id = 2; /* Current pci device vmstate version */
> >      return pci_dev;
> >  }
> > @@ -1175,7 +1175,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> >  /* generic PCI irq support */
> >  
> >  /* 0 <= irq_num <= 3. level must be 0 or 1 */
> > -static void pci_set_irq(void *opaque, int irq_num, int level)
> > +static void pci_irq_handler(void *opaque, int irq_num, int level)
> >  {
> >      PCIDevice *pci_dev = opaque;
> >      int change;
> > @@ -1191,6 +1191,24 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
> >      pci_change_irq_level(pci_dev, irq_num, change);
> >  }
> >  
> > +static inline int pci_intx(PCIDevice *pci_dev)
> > +{
> > +    return pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1;
> > +}
> > +
> > +qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
> > +{
> > +    int intx = pci_intx(pci_dev);
> > +
> > +    return qemu_allocate_irq(pci_irq_handler, pci_dev, intx);
> > +}
> > +
> > +void pci_set_irq(PCIDevice *pci_dev, int level)
> > +{
> > +    int intx = pci_intx(pci_dev);
> > +    pci_irq_handler(pci_dev, intx, level);
> > +}
> > +
> >  /* Special hooks used by device assignment */
> >  void pci_bus_set_route_irq_fn(PCIBus *bus, pci_route_irq_fn route_intx_to_irq)
> >  {
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 4b90e5d..df7d316 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -632,6 +632,25 @@ PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
> >  PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
> >  PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
> >  
> > +qemu_irq pci_allocate_irq(PCIDevice *pci_dev);
> > +void pci_set_irq(PCIDevice *pci_dev, int level);
> > +
> > +static inline void pci_irq_raise(PCIDevice *pci_dev)
> > +{
> > +    pci_set_irq(pci_dev, 1);
> > +}
> > +
> > +static inline void pci_irq_lower(PCIDevice *pci_dev)
> > +{
> > +    pci_set_irq(pci_dev, 0);
> > +}
> > +
> 
> I'd like to add that any users of pci_irq_pulse
> are immediately suspect. PCI does not work this way.
> If you resend this, maybe add a comment that all users
> of this should be fixed.
Sure, thanks!
Marcel

> 
> > +static inline void pci_irq_pulse(PCIDevice *pci_dev)
> > +{
> > +    pci_irq_lower(pci_dev);
> > +    pci_irq_raise(pci_dev);
> > +}
> > +
> >  static inline int pci_is_express(const PCIDevice *d)
> >  {
> >      return d->cap_present & QEMU_PCI_CAP_EXPRESS;
> > -- 
> > 1.8.3.1