Re: [Qemu-devel] [PATCH v3 07/11] versatile_pci: Implement the correct PCI IRQ mapping

[Peter Maydell]

On 4 April 2013 18:03, Michael S. Tsirkin <address@hidden> wrote:
> On Thu, Apr 04, 2013 at 01:58:29PM +0100, Peter Maydell wrote:
>> Implement the correct IRQ mapping for the Versatile PCI controller; it
>> differs between realview and versatile boards, but the previous QEMU
>> implementation was correct only for the first PCI card on a versatile
>> board, since we weren't swizzling IRQs based on the slot number.
>>
>> Since this change would otherwise break any uses of PCI on Linux kernels
>> which have an equivalent bug (since they have effectively only been
>> tested against QEMU, not real hardware), we implement a mechanism
>> for automatically detecting those broken kernels and switching back
>> to the old mapping. This works by looking at the values the kernel
>> writes to the PCI_INTERRUPT_LINE register in the config space, which
>> is effectively the interrupt number the kernel expects the device
>> to be using.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>
> Looks good a couple of suggestion on the implementation.
>
>> ---
>>  hw/versatile_pci.c |  105 
>> +++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 99 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
>> index 576e619..859fbee 100644
>> --- a/hw/versatile_pci.c
>> +++ b/hw/versatile_pci.c
>> @@ -13,6 +13,28 @@
>>  #include "hw/pci/pci_host.h"
>>  #include "exec/address-spaces.h"
>>
>> +/* Old and buggy versions of QEMU used the wrong mapping from
>> + * PCI IRQs to system interrupt lines. Unfortunately the Linux
>> + * kernel also had the corresponding bug in setting up interrupts
>> + * (so older kernels work on QEMU and not on real hardware).
>> + * We automatically detect these broken kernels and flip back
>> + * to the broken irq mapping by spotting guest writes to the
>> + * PCI_INTERRUPT_LINE register to see where the guest thinks
>> + * interrupts are going to be routed. So we start in state
>> + * ASSUME_OK on reset, and transition to either BROKEN or
>> + * FORCE_OK at the first write to an INTERRUPT_LINE register for
>> + * a slot where broken and correct interrupt mapping would differ.
>> + * Once in either BROKEN or FORCE_OK we never transition again;
>> + * this allows a newer kernel to use the INTERRUPT_LINE
>> + * registers arbitrarily once it has indicated that it isn't
>> + * broken in its init code somewhere.
>> + */
>> +enum {
>> +    IRQ_MAPPING_ASSUME_OK = 0,
>> +    IRQ_MAPPING_BROKEN = 1,
>> +    IRQ_MAPPING_FORCE_OK = 2,
>
> Better to prefix with PCI_VPB_ or something.

That makes them even longer and more unwieldy, which isn't very
friendly for something that's really only in a single source
file.

> And we don't really care what the values are,
> can let enum assign what it wants.

True.

>> +};
>> +
>>  typedef struct {
>>      PCIHostState parent_obj;
>>
>> @@ -26,6 +48,9 @@ typedef struct {
>>
>>      /* Constant for life of device: */
>>      int realview;
>> +
>> +    /* Variable state: */
>> +    uint8_t irq_mapping;
>>  } PCIVPBState;
>>
>>  #define TYPE_VERSATILE_PCI "versatile_pci"
>> @@ -44,14 +69,27 @@ static inline uint32_t vpb_pci_config_addr(hwaddr addr)
>>  static void pci_vpb_config_write(void *opaque, hwaddr addr,
>>                                   uint64_t val, unsigned size)
>>  {
>> -    pci_data_write(opaque, vpb_pci_config_addr(addr), val, size);
>> +    PCIVPBState *s = (PCIVPBState *)opaque;
>
> Please don't cast void *.

There are quite a lot of casts of void* in hw/, but I'm
happy to help postpone the evil day when we try to build
QEMU with a C++ compiler :-)

>> +    if (!s->realview && (addr & 0xff) == PCI_INTERRUPT_LINE
>
>> +        && s->irq_mapping == IRQ_MAPPING_ASSUME_OK) {
>> +        uint8_t devfn = addr >> 8;
>> +        if ((PCI_SLOT(devfn) % PCI_NUM_PINS) != 2) {
>
> Would it be enough to just detect this for devfn 0?

Nope, because devfn 0 is almost never used. We start at
devfn 11 (for reasons that aren't entirely clear to me).

> If yes this happens to be our device here

You mean the PCI controller itself? It's at devfn 29, not 0.
In any case, the kernel doesn't write to PCI_INTERRUPT_LINE
for the controller, only for actual PCI cards.

> , so we could
> cleanly implement a config_write callback, like this:
>
>         pci_default_write_config
>         if (!ranger_cover_byte(addr, size, PCI_INTERRUPT_LINE))
>                 return;
>         if (dev->config[PCI_INTERRUPT_LINE] == 27) {
>                 s->irq_mapping = IRQ_MAPPING_BROKEN;
>         } else {
>                 s->irq_mapping = IRQ_MAPPING_FORCE_OK;
>         }
>
>
>
>> +            if (val == 27) {
>> +                s->irq_mapping = IRQ_MAPPING_BROKEN;
>> +            } else {
>> +                s->irq_mapping = IRQ_MAPPING_FORCE_OK;
>> +            }
>> +        }
>> +    }
>> +    pci_data_write(&s->pci_bus, vpb_pci_config_addr(addr), val, size);
>

thanks
-- PMM