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

[Tom Rotenberg <tom.rotenberg@xxxxxxxxx>]

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?

Tom

On Tue, Dec 29, 2009 at 10:59 AM, Zhai, Edwin <edwin.zhai@xxxxxxxxx> wrote:
>
>
> Tom Rotenberg wrote:
>>
>> Well, i don't think that your patch will work for all of the cases, as
>> some platforms boot with non multi-function devices using interrupt
>> pin other than INTA... i myself encountered 2 such Lenovo platforms.
>>
>
> In general single function device should be linked to INTA, although most of
> OS can handle otherwise:)
>
> Back to the issue, even this abnormal machine can work with my patch, as our
> policy is handling _virtual_ INTx and providing consistent values to guest
> and xen, and physical INTx doesn't matter here.
>
> Could you pls. test my patch without yours to see if it can work?
> Thanks,
>
>
>> So, i think we will need to use both patches together. What do u say?
>>
>> On Tue, Dec 29, 2009 at 4:01 AM, Simon Horman <horms@xxxxxxxxxxxx> wrote:
>>
>>>
>>> On Tue, Dec 29, 2009 at 08:22:35AM +0800, Zhai, Edwin wrote:
>>>
>>>>
>>>> Your patch can also fix this issue, as using physical pin info for
>>>> both guest and xen.
>>>> Only potential issue is that guest will see a single function device
>>>> is not linked to INTA, say assign 00:1a.7 to guest as 00:4.0. It
>>>> should work, but seems doesn't comply with PCI spec.
>>>>
>>>
>>> I had a vague memory that was the case,
>>> I have been meaning to check the spec.
>>>
>>>
>>>>
>>>> We have 2 options here:
>>>> 1. always use INTA
>>>> 2. Use INTA for virtual function 0, and physical value for others.
>>>> Options 2 is more friendly to multiple-function device assignment,
>>>> and is current used.
>>>>
>>>
>>> 2 seems to be a much better solution to me.
>>>
>>> Tom, could you see if Edwin's patch, without your patch, resolves
>>> the problem that you were seeing?
>>>
>>>
>>>>
>>>> Tom Rotenberg wrote:
>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> I ran into similar problems last week, and i tried the following fix,
>>>>> which looks like it kind-of fixed it, does this do the same fix as
>>>>> your patch?
>>>>>
>>>>> Here is the patch i used:
>>>>>
>>>>> --- a/tools/ioemu/hw/pass-through.c  Sun Dec 27 11:56:08 2009 +0200
>>>>> +++ b/tools/ioemu/hw/pass-through.c  Sun Dec 27 11:56:08 2009 +0200
>>>>> @@ -4209,8 +4209,14 @@
>>>>>  */
>>>>> uint8_t pci_intx(struct pt_dev *ptdev)
>>>>> {
>>>>> +#if 0    /* FIX */
>>>>>     if (!PCI_FUNC(ptdev->dev.devfn))
>>>>>         return 0;
>>>>> +#endif /* FIX */
>>>>> +
>>>>>     return pci_read_intx(ptdev);
>>>>> }
>>>>>
>>>>> Tom
>>>>>
>>>>> On Mon, Dec 28, 2009 at 9:04 AM, Zhai, Edwin <edwin.zhai@xxxxxxxxx>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> Simon,
>>>>>> For the pass-through device's INTx emulation, we follow the policy: if
>>>>>> virtual function 0, use INTA#, otherwise use hardware value. However,
>>>>>> this
>>>>>> policy only apply when bind_pt_pci_irq to xen, and always use physical
>>>>>> value
>>>>>> when exporting to guest. This discrepancy cause different INTx, thus
>>>>>> different GSI in xen and guest, so that interrupts never got injected
>>>>>> to
>>>>>> guest. E.g. when assigning a USB controller with non-zero
>>>>>> function(00:1d.2)
>>>>>> to guest as 00:4.0, xen will see INTA#, while guest see INTC#.
>>>>>>
>>>>>> This simple patch can fix it. Could you pls. review it?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> --
>>>>>> best rgds,
>>>>>> edwin
>>>>>>
>>>>>>
>>>>>> Signed-Off-By: Zhai Edwin <edwin.zhai@xxxxxxxxx>
>>>>>>
>>>>>> diff --git a/hw/pass-through.c b/hw/pass-through.c
>>>>>> index e7bd386..a08c0bf 100644
>>>>>> --- a/hw/pass-through.c
>>>>>> +++ b/hw/pass-through.c
>>>>>> @@ -2710,7 +2710,8 @@ static uint32_t pt_status_reg_init(struct pt_dev
>>>>>> *ptdev,
>>>>>> 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];
>>>>>> +    /* Translate xen INTx value to hw */
>>>>>> +    return pci_intx(ptdev) + 1;
>>>>>> }
>>>>>>
>>>>>> /* initialize BAR */
>>>>>>
>>>>>> _______________________________________________
>>>>>> Xen-devel mailing list
>>>>>> Xen-devel@xxxxxxxxxxxxxxxxxxx
>>>>>> http://lists.xensource.com/xen-devel
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>> --
>>>> best rgds,
>>>> edwin
>>>>
>>
>>
>
> --
> best rgds,
> edwin
>
>

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