xen: fix interrupt routing

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Match the routing informations built by seabios:

- remove i440fx_write_config_xen
we don't need to intercept pci config writes to i440FX;

- introduce piix3_write_config_xen
we do need to intercept pci config write to the PCI-ISA bridge to update
the PCI link routing;

- remove xen_pci_slot_get_pirq
we are now using the same link routing as seabios and qemu so we don't
need a diffirent get_pirq function;

- fix xen_piix3_set_irq
we always inject one of the 4 pci intx, so we can use
xc_hvm_set_isa_irq_level to inject the interrupt. Use pci_irqs as
initialized by seabios to map a pirq into an ISA irq. This has the
benefit of removing all the calls to xc_hvm_set_pci_intx_level that
doesn't work correctly anymore because from the same device number and
intx Xen calculates a different PCI link compared to Qemu and Seabios.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/piix_pci.c |   21 ++++++++++++---------
 xen-all.c     |   16 ++++++++--------
 2 files changed, 20 insertions(+), 17 deletions(-)

On Wed, May 18, 2011 at 06:53:40PM +0100, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Match the routing informations built by seabios:
> 
> - remove i440fx_write_config_xen
> we don't need to intercept pci config writes to i440FX;
> 
> - introduce piix3_write_config_xen
> we do need to intercept pci config write to the PCI-ISA bridge to update
> the PCI link routing;

So "i440FX-xen" isn't needed anymore, and something like PIIX3-xen
is necessary instead of overwriting piix3->dev.config_write.

thanks,

> 
> - remove xen_pci_slot_get_pirq
> we are now using the same link routing as seabios and qemu so we don't
> need a diffirent get_pirq function;
> 
> - fix xen_piix3_set_irq
> we always inject one of the 4 pci intx, so we can use
> xc_hvm_set_isa_irq_level to inject the interrupt. Use pci_irqs as
> initialized by seabios to map a pirq into an ISA irq. This has the
> benefit of removing all the calls to xc_hvm_set_pci_intx_level that
> doesn't work correctly anymore because from the same device number and
> intx Xen calculates a different PCI link compared to Qemu and Seabios.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/piix_pci.c |   21 ++++++++++++---------
>  xen-all.c     |   16 ++++++++--------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 7f1c4cc..aae6a48 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -78,6 +78,8 @@ struct PCII440FXState {
>  #define I440FX_SMRAM    0x72
>  
>  static void piix3_set_irq(void *opaque, int pirq, int level);
> +static void piix3_write_config_xen(PCIDevice *dev,
> +                               uint32_t address, uint32_t val, int len);
>  
>  /* return the global irq number corresponding to a given device irq
>     pin. We could also use the bus number to have a more precise
> @@ -173,13 +175,6 @@ static void i440fx_write_config(PCIDevice *dev,
>      }
>  }
>  
> -static void i440fx_write_config_xen(PCIDevice *dev,
> -                                    uint32_t address, uint32_t val, int len)
> -{
> -    xen_piix_pci_write_config_client(address, val, len);
> -    i440fx_write_config(dev, address, val, len);
> -}
> -
>  static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
>  {
>      PCII440FXState *d = opaque;
> @@ -301,7 +296,8 @@ PCIBus *i440fx_xen_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>      PCIBus *b;
>  
>      b = i440fx_common_init("i440FX-xen", pi440fx_state, piix3_devfn, pic, ram_size);
> -    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> +    (*pi440fx_state)->piix3->dev.config_write = piix3_write_config_xen;
> +    pci_bus_irqs(b, xen_piix3_set_irq, pci_slot_get_pirq,
>                   (*pi440fx_state)->piix3, PIIX_NUM_PIRQS);
>  
>      return b;
> @@ -365,6 +361,13 @@ static void piix3_write_config(PCIDevice *dev,
>      }
>  }
>  
> +static void piix3_write_config_xen(PCIDevice *dev,
> +                               uint32_t address, uint32_t val, int len)
> +{
> +    xen_piix_pci_write_config_client(address, val, len);
> +    piix3_write_config(dev, address, val, len);
> +}
> +
>  static void piix3_reset(void *opaque)
>  {
>      PIIX3State *d = opaque;
> @@ -471,7 +474,7 @@ static PCIDeviceInfo i440fx_info[] = {
>          .qdev.vmsd    = &vmstate_i440fx,
>          .qdev.no_user = 1,
>          .init         = i440fx_initfn,
> -        .config_write = i440fx_write_config_xen,
> +        .config_write = i440fx_write_config,
>      },{
>          .qdev.name    = "PIIX3",
>          .qdev.desc    = "ISA bridge",
> diff --git a/xen-all.c b/xen-all.c
> index 0eac202..7d7863f 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -70,17 +70,16 @@ typedef struct XenIOState {
>      Notifier exit;
>  } XenIOState;
>  
> -/* Xen specific function for piix pci */
> +/* pci_irqs as initialized by seabios */
> +uint8_t pci_irqs[4] = {
> +    10, 10, 11, 11
> +};
>  
> -int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> -{
> -    return irq_num + ((pci_dev->devfn >> 3) << 2);
> -}
> +/* Xen specific function for piix pci */
>  
> -void xen_piix3_set_irq(void *opaque, int irq_num, int level)
> +void xen_piix3_set_irq(void *opaque, int pirq, int level)
>  {
> -    xc_hvm_set_pci_intx_level(xen_xc, xen_domid, 0, 0, irq_num >> 2,
> -                              irq_num & 3, level);
> +    xc_hvm_set_isa_irq_level(xen_xc, xen_domid, pci_irqs[pirq], level);
>  }
>  
>  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
> @@ -95,6 +94,7 @@ void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
>          }
>          v &= 0xf;
>          if (((address + i) >= 0x60) && ((address + i) <= 0x63)) {
> +            pci_irqs[address + i - 0x60] = v;
>              xc_hvm_set_pci_link_route(xen_xc, xen_domid, address + i - 0x60, v);
>          }
>      }
> -- 
> 1.7.2.3
> 
>

On Wed, 2011-05-18 at 18:53 +0100, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Match the routing informations built by seabios:
> 
> - remove i440fx_write_config_xen
> we don't need to intercept pci config writes to i440FX;
> 
> - introduce piix3_write_config_xen
> we do need to intercept pci config write to the PCI-ISA bridge to update
> the PCI link routing;
> 
> - remove xen_pci_slot_get_pirq
> we are now using the same link routing as seabios and qemu so we don't
> need a diffirent get_pirq function;
> 
> - fix xen_piix3_set_irq
> we always inject one of the 4 pci intx, so we can use
> xc_hvm_set_isa_irq_level to inject the interrupt. Use pci_irqs as
> initialized by seabios to map a pirq into an ISA irq. This has the
> benefit of removing all the calls to xc_hvm_set_pci_intx_level that
> doesn't work correctly anymore because from the same device number and
> intx Xen calculates a different PCI link compared to Qemu and Seabios.

Why does Xen calculate a different PCI link? What are the other
downsides of this disparity?

For example, in the hypervisor I see that hvm_pci_intx_link is used in
the passthrough code as well as in the HVMOP_set_pci_intx_level, is that
safe/sane in conjunction with this change?

IOW -- are we hiding other problems for later by doing an end run around
the interface instead of addressing the disconnect directly? It seems
like it would be a relatively contained change on the SeaBIOS end to
allow it to use the routing which Xen expects.

Ian.

On Thu, 19 May 2011, Ian Campbell wrote:
> On Wed, 2011-05-18 at 18:53 +0100, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Match the routing informations built by seabios:
> > 
> > - remove i440fx_write_config_xen
> > we don't need to intercept pci config writes to i440FX;
> > 
> > - introduce piix3_write_config_xen
> > we do need to intercept pci config write to the PCI-ISA bridge to update
> > the PCI link routing;
> > 
> > - remove xen_pci_slot_get_pirq
> > we are now using the same link routing as seabios and qemu so we don't
> > need a diffirent get_pirq function;
> > 
> > - fix xen_piix3_set_irq
> > we always inject one of the 4 pci intx, so we can use
> > xc_hvm_set_isa_irq_level to inject the interrupt. Use pci_irqs as
> > initialized by seabios to map a pirq into an ISA irq. This has the
> > benefit of removing all the calls to xc_hvm_set_pci_intx_level that
> > doesn't work correctly anymore because from the same device number and
> > intx Xen calculates a different PCI link compared to Qemu and Seabios.
> 
> Why does Xen calculate a different PCI link? What are the other
> downsides of this disparity?
>
> For example, in the hypervisor I see that hvm_pci_intx_link is used in
> the passthrough code as well as in the HVMOP_set_pci_intx_level, is that
> safe/sane in conjunction with this change?
> 
> IOW -- are we hiding other problems for later by doing an end run around
> the interface instead of addressing the disconnect directly? It seems
> like it would be a relatively contained change on the SeaBIOS end to
> allow it to use the routing which Xen expects.


If you give a look at __hvm_pci_intx_assert in xen, it uses
hvm_pci_intx_link to calculate the link from device number and intx:

#define hvm_pci_intx_link(dev, intx) \
    (((dev) + (intx)) & 3)

while qemu and seabios calculate the link as follow:

static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
{
    int slot_addend;
    slot_addend = (pci_dev->devfn >> 3) - 1;
    return (pci_intx + slot_addend) & 3;
}

they are similar but unfortunately not indentical (the "- 1" is the
critical difference there).
That means that the routing for ISA irqs is different but if we always
use xc_hvm_set_isa_irq_level we avoid the issue because it takes an ISA
irq as parameter so Xen won't notice.

Unfortunately like you wrote, we would still have a problem with PCI
passthrough because Xen calls hvm_pci_intx_link internally.
We would need to add an option in Xen to switch to the new interrupt
routing.

I think we need to figure out if we want to keep the old MP tables and
the old ACPI _PRT entries or we want to start using Seabios' MP tables
and ACPI _PRT entries.
In both cases the impact on Qemu is pretty limited: it is just a matter
of changing the implementation of piix3_set_irq and pci_slot_get_pirq
(both have a xen specific implementation already).

It is interesting that the old PIIX3 in qemu_xen had 128 interrupt
lines, the routing informations of which are present in the ACPI PRTA
routing table.
The PIIX3 in the new qemu has only 4 lines so there is no need for
xc_hvm_set_pci_intx_level.
Considering that the next update of this patch will add PIIX3-xen there
is scope for increasing the interrupt lines again if we want to.

On Thu, 19 May 2011, Isaku Yamahata wrote:
> On Wed, May 18, 2011 at 06:53:40PM +0100, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Match the routing informations built by seabios:
> > 
> > - remove i440fx_write_config_xen
> > we don't need to intercept pci config writes to i440FX;
> > 
> > - introduce piix3_write_config_xen
> > we do need to intercept pci config write to the PCI-ISA bridge to update
> > the PCI link routing;
> 
> So "i440FX-xen" isn't needed anymore, and something like PIIX3-xen
> is necessary instead of overwriting piix3->dev.config_write.

That is a good idea. I am also going to remove i440fx_xen_init to get rid of
a 'if (xen_enabled())' but to do that I have to move the call to
pci_bus_irqs in i440fx_common_init.

I'll send an update of the patch as soon as we figure out what to do
with the BIOS tables.