Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device

[Simon Horman <horms@xxxxxxxxxxxx>]

On Wed, Dec 30, 2009 at 10:20:39AM +0200, Tom Rotenberg wrote:
> Hi,
> 
> Just tested your patch (without my patch), and it works.
> So, now the question is, which approach is better and why? i think
> your approach in the patch sounds a little bit better, but i am not
> sure why (other than reflecting IntA for multi-function devs).
> Can u please explain why?

Hi Tom, Hi Edwin,

As I see things, the problem is that there is a discrepancy in the
way that two different sections of code calculate the same value -
the virtual INTx.

On the one hand there is uint32_t pt_irqpin_reg_init(), which
always uses the hw value.

And on the other hand there is pci_intx() which uses the hw value
if for PCI functions other than zero, and INTA for PCI function zero.
(pci_read_intx() also mangles the value but thats not relevant to the
problem).

Furthermore we can observe that almost always the result of these
two methods is the same, as function zero should use INTA. But some
real HW doesn't, and thats a problem.

The thing that is really annoying here is that there are two bits
of code calculating the same thing. For that reason Ediwin's patch
is nice in that it centralises the code. But otherwise I prefer
Tom's approach of always using the hw value - that just seems nicer
to me. IIRC, the only reason that I put in the use INA for function 0
logic was because prior to multi-function INTA was always used. But
that extra little bit of logic is biting us now.

I propose a) always using the hw value for INTx and b) always calculating
this in the same place.

From: Simon Horman <horms@xxxxxxxxxxxx>


qemu-xen: pass-through: always use hw intx

pass-through: always use hw intx and always get it from the same place

Cc: Tom Rotenberg <tom.rotenberg@xxxxxxxxx>
Cc: Edwin Zhai <edwin.zhai@xxxxxxxxx>
Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx>

Index: ioemu-remote/hw/pass-through.c
===================================================================
--- ioemu-remote.orig/hw/pass-through.c 2009-12-31 13:10:06.000000000 +0900
+++ ioemu-remote/hw/pass-through.c      2009-12-31 13:24:20.000000000 +0900
@@ -2710,7 +2710,7 @@ static uint32_t pt_status_reg_init(struc
 static uint32_t pt_irqpin_reg_init(struct pt_dev *ptdev,
         struct pt_reg_info_tbl *reg, uint32_t real_offset)
 {
-    return ptdev->dev.config[real_offset];
+    return pci_read_intx(ptdev);
 }
 
 /* initialize BAR */
@@ -4471,6 +4471,11 @@ int pt_init(PCIBus *e_bus)
     return 0;
 }
 
+static uint8_t pci_read_intx(struct pt_dev *ptdev)
+{
+    return pci_read_byte(ptdev->pci_dev, PCI_INTERRUPT_PIN);
+}
+
 /* The PCI Local Bus Specification, Rev. 3.0,
  * Section 6.2.4 Miscellaneous Registers, pp 223
  * outlines 5 valid values for the intertupt pin (intx).
@@ -4499,9 +4504,9 @@ int pt_init(PCIBus *e_bus)
  * 4               | 3   | INTD#
  * any other value | 0   | This should never happen, log error message
  */
-static uint8_t pci_read_intx(struct pt_dev *ptdev)
+uint8_t pci_intx(struct pt_dev *ptdev)
 {
-    uint8_t r_val = pci_read_byte(ptdev->pci_dev, PCI_INTERRUPT_PIN);
+    uint8_t r_val = pci_read_intx(ptdev);
 
     PT_LOG("intx=%i\n", r_val);
     if (r_val < 1 || r_val > 4)
@@ -4517,14 +4522,3 @@ static uint8_t pci_read_intx(struct pt_d
 
     return r_val;
 }
-
-/*
- * For virtual function 0, always use INTA#,
- * otherwise use the hardware value
- */
-uint8_t pci_intx(struct pt_dev *ptdev)
-{
-    if (!PCI_FUNC(ptdev->dev.devfn))
-        return 0;
-    return pci_read_intx(ptdev);
-}


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel