On Tue, Dec 19, 2017 at 10:27 AM, Mark Cave-Ayland
<mark.cave-ayl...@ilande.co.uk> wrote:
> On 19/12/17 07:56, Artyom Tarasenko wrote:
>>
>> On Sun, Dec 17, 2017 at 12:09 PM, Mark Cave-Ayland
>> <mark.cave-ayl...@ilande.co.uk> 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
>>>>> <mark.cave-ayl...@ilande.co.uk> 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 <mark.cave-ayl...@ilande.co.uk>
>>>>>> ---
>>>>>> 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?
Yes, please. I think it would be pretty close to what the real hw does.
--
Regards,
Artyom Tarasenko
SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu
