Re: [Qemu-devel] [PATCH 11/15] apb: split pci_pbm_map_irq() into separate functions for bus A and bus B

[Mark Cave-Ayland]

On 19/12/17 07:56, Artyom Tarasenko wrote:
> On Sun, Dec 17, 2017 at 12:09 PM, Mark Cave-Ayland
> <address@hidden> wrote:
> > On 19/11/17 11:06, Mark Cave-Ayland wrote:
> >
> > > On 17/11/17 14:33, Artyom Tarasenko wrote:
> > >
> > > > On Fri, Nov 17, 2017 at 2:42 PM, Mark Cave-Ayland
> > > > <address@hidden> wrote:
> > > > > After the previous refactoring it is now possible to use separate
> > > > > functions
> > > > > to improve clarity of the interrupt paths. Similarly by checking the PCI
> > > > > devnfn to identify busA during apb_pci_bridge_realize() it becomes
> > > > > possible
> > > > > to completely remove the busA property from the PBMPCIBridge state.
> > > > >
> > > > > Signed-off-by: Mark Cave-Ayland <address@hidden>
> > > > > ---
> > > > >    hw/pci-host/apb.c         |   54
> > > > > ++++++++++++++++++---------------------------
> > > > >    include/hw/pci-host/apb.h |    3 ---
> > > > >    2 files changed, 21 insertions(+), 36 deletions(-)
> > > > >
> > > > > diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> > > > > index 6c20285..268100e 100644
> > > > > --- a/hw/pci-host/apb.c
> > > > > +++ b/hw/pci-host/apb.c
> > > > > @@ -517,32 +517,27 @@ static int pci_apb_map_irq(PCIDevice *pci_dev, int
> > > > > irq_num)
> > > > >        return irq_num;
> > > > >    }
> > > > >
> > > > > -static int pci_pbm_map_irq(PCIDevice *pci_dev, int irq_num)
> > > > > +static int pci_pbmA_map_irq(PCIDevice *pci_dev, int irq_num)
> > > > >    {
> > > > > -    PBMPCIBridge *br = PBM_PCI_BRIDGE(pci_bridge_get_device(
> > > > > -
> > > > > PCI_BUS(qdev_get_parent_bus(DEVICE(pci_dev)))));
> > > > > -
> > > > > -    int bus_offset;
> > > > > -    if (br->busA) {
> > > > > -        bus_offset = 0x0;
> > > > > +    /* The on-board devices have fixed (legacy) OBIO intnos */
> > > > > +    switch (PCI_SLOT(pci_dev->devfn)) {
> > > > > +    case 1:
> > > > > +        /* Onboard NIC */
> > > > > +        return 0x21;
> > > > > +    case 3:
> > > > > +        /* Onboard IDE */
> > > > > +        return 0x20;
> > > > > +    default:
> > > > > +        /* Normal intno, fall through */
> > > > > +        break;
> > > > > +    }
> > > > >
> > > > > -        /* The on-board devices have fixed (legacy) OBIO intnos */
> > > > > -        switch (PCI_SLOT(pci_dev->devfn)) {
> > > > > -        case 1:
> > > > > -            /* Onboard NIC */
> > > > > -            return 0x21;
> > > > > -        case 3:
> > > > > -            /* Onboard IDE */
> > > > > -            return 0x20;
> > > > > +    return ((PCI_SLOT(pci_dev->devfn) << 2) + irq_num) & 0x1f;
> > > > > +}
> > > > >
> > > > > -        default:
> > > > > -            /* Normal intno, fall through */
> > > > > -            break;
> > > > > -        }
> > > > > -    } else {
> > > > > -        bus_offset = 0x10;
> > > > > -    }
> > > > > -    return (bus_offset + (PCI_SLOT(pci_dev->devfn) << 2) + irq_num) &
> > > > > 0x1f;
> > > > > +static int pci_pbmB_map_irq(PCIDevice *pci_dev, int irq_num)
> > > > > +{
> > > > > +    return (0x10 + (PCI_SLOT(pci_dev->devfn) << 2) + irq_num) & 0x1f;
> > > > >    }
> > > > >
> > > > >    static void pci_apb_set_irq(void *opaque, int irq_num, int level)
> > > > > @@ -593,7 +588,7 @@ static void apb_pci_bridge_realize(PCIDevice *dev,
> > > > > Error **errp)
> > > > >
> > > > >        /* If initialising busA, ensure that we allow IO transactions so
> > > > > that
> > > > >           we get the early serial console until OpenBIOS configures the
> > > > > bridge */
> > > > > -    if (br->busA) {
> > > > > +    if (dev->devfn == PCI_DEVFN(1, 1)) {
> > > >
> > > >
> > > > I think the previous syntax was more explicit here. A comment would be
> > > > nice.
> > >
> > >
> > > Yes it's definitely something that isn't immediately obvious, which is
> > > why I left the above comment in place explaining what the if() branch is
> > > doing. Is there something in the comment that isn't particularly clear?
> > >
> > > Note one of the reasons for wanting to remove the busA property is that
> > > where possible I'd like to reduce the code in the IRQ path, and while
> > > the existing code works I am still unsure of the additional overhead of
> > > the 2 levels of QOM type checking that the current approach requires for
> > > each IRQ.
> >
> >
> > Hi Artyom,
> >
> > Thinking about this a bit more during freeze, this is actually doing the
> > opposite of what we want, as it requires the device realise function to
> > behave differently depending upon how it is related to the PCI bus.
> >
> > How about swapping this out for a qdev bool property for APB named
> > "enable-early-pci-io-access", setting it just for the PCI_DEVFN(1, 1) device
> > containing the ebus and then alter the if() statement above to enable PCI IO
> > access if the qdev property is set?
>
> This does sound reasonable. I wonder if this has to be a qdev property though.
> Doesn't the physical bridge have a software visible bit/register for it?

Yes, we could enable the PCI IO transaction bit for that particular 
bridge after realize if that is acceptable?


ATB,

Mark.