[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 11/15] apb: split pci_pbm_map_irq() into separat
From: |
Artyom Tarasenko |
Subject: |
Re: [Qemu-devel] [PATCH 11/15] apb: split pci_pbm_map_irq() into separate functions for bus A and bus B |
Date: |
Tue, 19 Dec 2017 08:56:05 +0100 |
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?
--
Regards,
Artyom Tarasenko
SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu