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

["Zhai, Edwin" <edwin.zhai@intel.com>]

Simon/Tom,
We have 3 optinos:
A. always INTA
B. always physical INTx
C. INTA if virtual function is 0, physical INTx for otherwise.

Difference of option B and C is that guest will see a function 0 on a 
single function device is linked to a PIN rather than INTA, say assign 
0:1a.7 to guest as 0:4.0. Most of OS should handle this. so I'm okay 
with both.

For option A, I'm not sure the issue for the multiple-function device. 
Can we assign a multiple function device to a guest as it is? E.g. 
assign physical 0:a.0/0:a.1to guest as 0:4.0/0:4.1. In this way, with 
option A, there are performance issue when injecting intr as they share 
same virtual GSI.
If we can't do this now, I think option A is also good. Is any specific 
reason that we change to C? Does some specific multiple function driver 
assumes specific pin other than INTA?

BTW, pls. send your patch in attachment as I couldn't get it from your 
mail:(

Thanks,

Simon Horman wrote:
> 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@verge.net.au>
>
>
> 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@gmail.com>
> Cc: Edwin Zhai <edwin.zhai@intel.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> 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);
> -}
>
>   

-- 
best rgds,
edwin