Zhai, Edwin
2009-Dec-28 07:04 UTC
[Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Horman
2009-Dec-28 07:54 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
On Mon, Dec 28, 2009 at 03:04:59PM +0800, Zhai, Edwin 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?Hi Edwin, that seems reasonable to me. I will have time to review this more thoroughly tomorrow.> > Thanks, > > -- > best rgds, > edwin >> Signed-Off-By: Zhai Edwin <edwin.zhai@intel.com> > > 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@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tom Rotenberg
2009-Dec-28 14:33 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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@intel.com>
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@intel.com>
>
> 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@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Zhai, Edwin
2009-Dec-29 00:22 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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. 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. 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@intel.com> 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@intel.com> >> >> 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@lists.xensource.com >> http://lists.xensource.com/xen-devel >> >> >> > >-- best rgds, edwin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Horman
2009-Dec-29 02:01 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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@intel.com> 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@intel.com> > >> > >>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@lists.xensource.com > >>http://lists.xensource.com/xen-devel > >> > >> > > > > -- > best rgds, > edwin_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tom Rotenberg
2009-Dec-29 08:51 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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. 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@verge.net.au> 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@intel.com> 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@intel.com> >> >> >> >>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@lists.xensource.com >> >>http://lists.xensource.com/xen-devel >> >> >> >> >> > >> >> -- >> best rgds, >> edwin >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhai, Edwin
2009-Dec-29 08:59 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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@verge.net.au> 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@intel.com> 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@intel.com> >>>>> >>>>> 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@lists.xensource.com >>>>> http://lists.xensource.com/xen-devel >>>>> >>>>> >>>>> >>> -- >>> best rgds, >>> edwin >>> > >-- best rgds, edwin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tom Rotenberg
2009-Dec-30 08:20 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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@intel.com> 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@verge.net.au> 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@intel.com> >>>>> 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@intel.com> >>>>>> >>>>>> 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@lists.xensource.com >>>>>> http://lists.xensource.com/xen-devel >>>>>> >>>>>> >>>>>> >>>> >>>> -- >>>> best rgds, >>>> edwin >>>> >> >> > > -- > best rgds, > edwin > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Horman
2009-Dec-31 04:32 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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); -} _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhai, Edwin
2009-Dec-31 06:08 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Horman
2009-Dec-31 06:45 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
On Thu, Dec 31, 2009 at 02:08:03PM +0800, Zhai, Edwin wrote:> 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.Yes, the ability to make assignments like that is the crux of the multi-function work that I did earlier in the year. And the idea of not always using INTA was to avoid the performance penalty of reusing the 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?... so A isn''t such a good option (it was before and thats what was used). I think that I chose C when I added multi-function because it avoided introducing any incompatibility for single-function pass-through. But at this point I think C just introduces complexity, so I now prefer B.> BTW, pls. send your patch in attachment as I couldn''t get it from > your mail:(Sure. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhai, Edwin
2009-Dec-31 07:40 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
Regarding this patch, I think we need move the pci_read_intx up, or I would get compilation error with the reference in pt_irqpin_reg_init. I have tested this path, both linux and windows can work, so Let''s use physical PIN policy. Could you pls. send a new patch to Ian Jakson in a new thread, so that he can check in it after holiday without tracking this long discussion:) BTW, could you pls. tell me how to assign the multiple function device to guest as it is? Setting the same virtual PCI slot? Thanks, Simon Horman wrote:> Yes, the ability to make assignments like that is the crux > of the multi-function work that I did earlier in the year. > And the idea of not always using INTA was to avoid the > performance penalty of reusing the 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? >> > > ... so A isn''t such a good option (it was before and thats what was used). > I think that I chose C when I added multi-function because it > avoided introducing any incompatibility for single-function pass-through. > But at this point I think C just introduces complexity, so I now prefer B. > > >> BTW, pls. send your patch in attachment as I couldn''t get it from >> your mail:( >> > > Sure. > >-- best rgds, edwin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Horman
2009-Dec-31 07:56 UTC
Re: [Xen-devel] [PATCH] [IOEMU] Fix wrong INTx for pass-through device
On Thu, Dec 31, 2009 at 03:40:53PM +0800, Zhai, Edwin wrote:> Regarding this patch, I think we need move the pci_read_intx up, or > I would get compilation error with the reference in > pt_irqpin_reg_init.Ok, I''ll fix that.> I have tested this path, both linux and windows can work, so Let''s > use physical PIN policy. Could you pls. send a new patch to Ian > Jakson in a new thread, so that he can check in it after holiday > without tracking this long discussion:)Sure.> BTW, could you pls. tell me how to assign the multiple function > device to guest as it is? Setting the same virtual PCI slot?Single-function assignment works like this: pci=00:1d.0 Multi-function extends that syntax by allowing multiple functions to be specified. For example: pci=0:1d.0,1 Details of other incantations are detailed at: http://wiki.xensource.com/xenwiki/BDFNotation _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel