| Message ID | 20170324165419.oragzj3dcbs36smj@dhcp-3-128.uk.xensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: > As I understand it, for level triggered legacy PCI interrupts Xen sets up a > timer in order to perform the EOI if the guest takes too long in deasserting > the line. This is done in pt_irq_time_out. What I don't understand is why this > function also does a deassertion of the guest view of the PCI interrupt, ie: > why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending assert > in the guest, and thus the guest will end up loosing one interrupt. Especially with the comment next to the respective set_timer() it looks to me as if this was the intended effect: If the guest didn't care to at least start handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in order to not have it block other interrupts inside the guest (i.e. there's more to it than just guarding the host here). "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared interrupts") introducing this has no description at all. Let's see if Kevin remembers any further details ... Jan
> From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, March 27, 2017 4:00 PM > > >>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: > > As I understand it, for level triggered legacy PCI interrupts Xen sets > > up a timer in order to perform the EOI if the guest takes too long in > > deasserting the line. This is done in pt_irq_time_out. What I don't > > understand is why this function also does a deassertion of the guest view > of the PCI interrupt, ie: > > why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending > > assert in the guest, and thus the guest will end up loosing one interrupt. > > Especially with the comment next to the respective set_timer() it looks to me > as if this was the intended effect: If the guest didn't care to at least start > handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in > order to not have it block other interrupts inside the guest (i.e. there's more > to it than just guarding the host here). > > "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared > interrupts") introducing this has no description at all. Let's see if Kevin > remembers any further details ... > Sorry I don't remember more detail other than existing comments. Roger, did you encounter a problem now? Thanks Kevin
On Fri, Mar 31, 2017 at 05:05:44AM +0000, Tian, Kevin wrote: > > From: Jan Beulich [mailto:JBeulich@suse.com] > > Sent: Monday, March 27, 2017 4:00 PM > > > > >>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: > > > As I understand it, for level triggered legacy PCI interrupts Xen sets > > > up a timer in order to perform the EOI if the guest takes too long in > > > deasserting the line. This is done in pt_irq_time_out. What I don't > > > understand is why this function also does a deassertion of the guest view > > of the PCI interrupt, ie: > > > why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending > > > assert in the guest, and thus the guest will end up loosing one interrupt. > > > > Especially with the comment next to the respective set_timer() it looks to me > > as if this was the intended effect: If the guest didn't care to at least start > > handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in > > order to not have it block other interrupts inside the guest (i.e. there's more > > to it than just guarding the host here). > > > > "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared > > interrupts") introducing this has no description at all. Let's see if Kevin > > remembers any further details ... > > > > Sorry I don't remember more detail other than existing comments. > Roger, did you encounter a problem now? No, I didn't encounter any problems with this so far, any well behaved guest will deassert those lines anyway, it just seems to be against the spec. AFAIK on bare metal the line will be asserted until the OS deasserts it, so I was wondering if this was some kind of workaround? Note that the physical line is already automatically deasserted (on the PIRQ layer), so this just deasserts the guest line which IMHO Xen shouldn't care at all (at the end the guest is free to not deassert it, and it shouldn't affect Xen at all). Roger.
>>> On 31.03.17 at 10:07, <roger.pau@citrix.com> wrote: > On Fri, Mar 31, 2017 at 05:05:44AM +0000, Tian, Kevin wrote: >> > From: Jan Beulich [mailto:JBeulich@suse.com] >> > Sent: Monday, March 27, 2017 4:00 PM >> > >> > >>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: >> > > As I understand it, for level triggered legacy PCI interrupts Xen sets >> > > up a timer in order to perform the EOI if the guest takes too long in >> > > deasserting the line. This is done in pt_irq_time_out. What I don't >> > > understand is why this function also does a deassertion of the guest view >> > of the PCI interrupt, ie: >> > > why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending >> > > assert in the guest, and thus the guest will end up loosing one interrupt. >> > >> > Especially with the comment next to the respective set_timer() it looks to me >> > as if this was the intended effect: If the guest didn't care to at least start >> > handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in >> > order to not have it block other interrupts inside the guest (i.e. there's more >> > to it than just guarding the host here). >> > >> > "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared >> > interrupts") introducing this has no description at all. Let's see if Kevin >> > remembers any further details ... >> > >> >> Sorry I don't remember more detail other than existing comments. >> Roger, did you encounter a problem now? > > No, I didn't encounter any problems with this so far, any well behaved guest > will deassert those lines anyway, it just seems to be against the spec. AFAIK > on bare metal the line will be asserted until the OS deasserts it, so I was > wondering if this was some kind of workaround? "OS deasserts" is a term I don't understand. Aiui it's the origin device which would need to de-assert its interrupt, and I think it is not uncommon for devices to de-assert interrupts after a certain amount of time. If that wasn't the case, spurious interrupts could never occur. Jan
On Fri, Mar 31, 2017 at 04:46:27AM -0600, Jan Beulich wrote: > >>> On 31.03.17 at 10:07, <roger.pau@citrix.com> wrote: > > On Fri, Mar 31, 2017 at 05:05:44AM +0000, Tian, Kevin wrote: > >> > From: Jan Beulich [mailto:JBeulich@suse.com] > >> > Sent: Monday, March 27, 2017 4:00 PM > >> > > >> > >>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: > >> > > As I understand it, for level triggered legacy PCI interrupts Xen sets > >> > > up a timer in order to perform the EOI if the guest takes too long in > >> > > deasserting the line. This is done in pt_irq_time_out. What I don't > >> > > understand is why this function also does a deassertion of the guest view > >> > of the PCI interrupt, ie: > >> > > why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending > >> > > assert in the guest, and thus the guest will end up loosing one interrupt. > >> > > >> > Especially with the comment next to the respective set_timer() it looks to me > >> > as if this was the intended effect: If the guest didn't care to at least start > >> > handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in > >> > order to not have it block other interrupts inside the guest (i.e. there's more > >> > to it than just guarding the host here). > >> > > >> > "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared > >> > interrupts") introducing this has no description at all. Let's see if Kevin > >> > remembers any further details ... > >> > > >> > >> Sorry I don't remember more detail other than existing comments. > >> Roger, did you encounter a problem now? > > > > No, I didn't encounter any problems with this so far, any well behaved guest > > will deassert those lines anyway, it just seems to be against the spec. AFAIK > > on bare metal the line will be asserted until the OS deasserts it, so I was > > wondering if this was some kind of workaround? > > "OS deasserts" is a term I don't understand. Aiui it's the origin device > which would need to de-assert its interrupt, and I think it is not > uncommon for devices to de-assert interrupts after a certain amount > of time. If that wasn't the case, spurious interrupts could never occur. I recall Sander (CC-ed) here hitting this at some point. There was some device he had (legacy?) that would very much hit this path. But I can't recall the details, sorry. Sanders, it was in the context of the dpci softirq work I did if that helps. > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On 31/03/17 16:38, Konrad Rzeszutek Wilk wrote: > On Fri, Mar 31, 2017 at 04:46:27AM -0600, Jan Beulich wrote: >>>>> On 31.03.17 at 10:07, <roger.pau@citrix.com> wrote: >>> On Fri, Mar 31, 2017 at 05:05:44AM +0000, Tian, Kevin wrote: >>>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>>> Sent: Monday, March 27, 2017 4:00 PM >>>>> >>>>>>>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: >>>>>> As I understand it, for level triggered legacy PCI interrupts Xen sets >>>>>> up a timer in order to perform the EOI if the guest takes too long in >>>>>> deasserting the line. This is done in pt_irq_time_out. What I don't >>>>>> understand is why this function also does a deassertion of the guest view >>>>> of the PCI interrupt, ie: >>>>>> why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending >>>>>> assert in the guest, and thus the guest will end up loosing one interrupt. >>>>> >>>>> Especially with the comment next to the respective set_timer() it looks to me >>>>> as if this was the intended effect: If the guest didn't care to at least start >>>>> handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in >>>>> order to not have it block other interrupts inside the guest (i.e. there's more >>>>> to it than just guarding the host here). >>>>> >>>>> "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared >>>>> interrupts") introducing this has no description at all. Let's see if Kevin >>>>> remembers any further details ... >>>>> >>>> >>>> Sorry I don't remember more detail other than existing comments. >>>> Roger, did you encounter a problem now? >>> >>> No, I didn't encounter any problems with this so far, any well behaved guest >>> will deassert those lines anyway, it just seems to be against the spec. AFAIK >>> on bare metal the line will be asserted until the OS deasserts it, so I was >>> wondering if this was some kind of workaround? >> >> "OS deasserts" is a term I don't understand. Aiui it's the origin device >> which would need to de-assert its interrupt, and I think it is not >> uncommon for devices to de-assert interrupts after a certain amount >> of time. If that wasn't the case, spurious interrupts could never occur. > > I recall Sander (CC-ed) here hitting this at some point. There was some device > he had (legacy?) that would very much hit this path. > > But I can't recall the details, sorry. > > Sanders, it was in the context of the dpci softirq work I did if that helps. Hi Konrad, You mean these ? The issue leading up to this revert for xen-4.5: https://lists.xen.org/archives/html/xen-devel/2015-01/msg01025.html Where this seems to be the thread that started the conversation leading up to that revert: https://lists.xenproject.org/archives/html/xen-devel/2014-11/msg01330.html Which than for xen-4.6 continued in a thread with the subject "dpci: Put the dpci back on the list if scheduled from another CPU." which is spread out over several months, (this is somewhere in between https://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02102.html ). -- Sander >> >> Jan >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> https://lists.xen.org/xen-devel
On Mon, Apr 03, 2017 at 02:22:36PM +0200, Sander Eikelenboom wrote: > On 31/03/17 16:38, Konrad Rzeszutek Wilk wrote: > > On Fri, Mar 31, 2017 at 04:46:27AM -0600, Jan Beulich wrote: > >>>>> On 31.03.17 at 10:07, <roger.pau@citrix.com> wrote: > >>> On Fri, Mar 31, 2017 at 05:05:44AM +0000, Tian, Kevin wrote: > >>>>> From: Jan Beulich [mailto:JBeulich@suse.com] > >>>>> Sent: Monday, March 27, 2017 4:00 PM > >>>>> > >>>>>>>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: > >>>>>> As I understand it, for level triggered legacy PCI interrupts Xen sets > >>>>>> up a timer in order to perform the EOI if the guest takes too long in > >>>>>> deasserting the line. This is done in pt_irq_time_out. What I don't > >>>>>> understand is why this function also does a deassertion of the guest view > >>>>> of the PCI interrupt, ie: > >>>>>> why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending > >>>>>> assert in the guest, and thus the guest will end up loosing one interrupt. > >>>>> > >>>>> Especially with the comment next to the respective set_timer() it looks to me > >>>>> as if this was the intended effect: If the guest didn't care to at least start > >>>>> handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in > >>>>> order to not have it block other interrupts inside the guest (i.e. there's more > >>>>> to it than just guarding the host here). > >>>>> > >>>>> "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared > >>>>> interrupts") introducing this has no description at all. Let's see if Kevin > >>>>> remembers any further details ... > >>>>> > >>>> > >>>> Sorry I don't remember more detail other than existing comments. > >>>> Roger, did you encounter a problem now? > >>> > >>> No, I didn't encounter any problems with this so far, any well behaved guest > >>> will deassert those lines anyway, it just seems to be against the spec. AFAIK > >>> on bare metal the line will be asserted until the OS deasserts it, so I was > >>> wondering if this was some kind of workaround? > >> > >> "OS deasserts" is a term I don't understand. Aiui it's the origin device > >> which would need to de-assert its interrupt, and I think it is not > >> uncommon for devices to de-assert interrupts after a certain amount > >> of time. If that wasn't the case, spurious interrupts could never occur. > > > > I recall Sander (CC-ed) here hitting this at some point. There was some device > > he had (legacy?) that would very much hit this path. > > > > But I can't recall the details, sorry. > > > > Sanders, it was in the context of the dpci softirq work I did if that helps. > > Hi Konrad, > > You mean these ? Yes, but I can't seem to find in those threads the name of the device you had - the one that was triggering those legacy interrupts.. By any chance you recall what it was? > > The issue leading up to this revert for xen-4.5: > https://lists.xen.org/archives/html/xen-devel/2015-01/msg01025.html > > Where this seems to be the thread that started the conversation leading up to that revert: > https://lists.xenproject.org/archives/html/xen-devel/2014-11/msg01330.html > > Which than for xen-4.6 continued in a thread with the subject "dpci: Put the dpci back on the list if scheduled from another CPU." > which is spread out over several months, (this is somewhere in between https://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02102.html ). > > -- > Sander > > >> > >> Jan > >> > >> > >> _______________________________________________ > >> Xen-devel mailing list > >> Xen-devel@lists.xen.org > >> https://lists.xen.org/xen-devel >
On 04/04/17 18:07, Konrad Rzeszutek Wilk wrote: > On Mon, Apr 03, 2017 at 02:22:36PM +0200, Sander Eikelenboom wrote: >> On 31/03/17 16:38, Konrad Rzeszutek Wilk wrote: >>> On Fri, Mar 31, 2017 at 04:46:27AM -0600, Jan Beulich wrote: >>>>>>> On 31.03.17 at 10:07, <roger.pau@citrix.com> wrote: >>>>> On Fri, Mar 31, 2017 at 05:05:44AM +0000, Tian, Kevin wrote: >>>>>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>>>>> Sent: Monday, March 27, 2017 4:00 PM >>>>>>> >>>>>>>>>> On 24.03.17 at 17:54, <roger.pau@citrix.com> wrote: >>>>>>>> As I understand it, for level triggered legacy PCI interrupts Xen sets >>>>>>>> up a timer in order to perform the EOI if the guest takes too long in >>>>>>>> deasserting the line. This is done in pt_irq_time_out. What I don't >>>>>>>> understand is why this function also does a deassertion of the guest view >>>>>>> of the PCI interrupt, ie: >>>>>>>> why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending >>>>>>>> assert in the guest, and thus the guest will end up loosing one interrupt. >>>>>>> >>>>>>> Especially with the comment next to the respective set_timer() it looks to me >>>>>>> as if this was the intended effect: If the guest didn't care to at least start >>>>>>> handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in >>>>>>> order to not have it block other interrupts inside the guest (i.e. there's more >>>>>>> to it than just guarding the host here). >>>>>>> >>>>>>> "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared >>>>>>> interrupts") introducing this has no description at all. Let's see if Kevin >>>>>>> remembers any further details ... >>>>>>> >>>>>> >>>>>> Sorry I don't remember more detail other than existing comments. >>>>>> Roger, did you encounter a problem now? >>>>> >>>>> No, I didn't encounter any problems with this so far, any well behaved guest >>>>> will deassert those lines anyway, it just seems to be against the spec. AFAIK >>>>> on bare metal the line will be asserted until the OS deasserts it, so I was >>>>> wondering if this was some kind of workaround? >>>> >>>> "OS deasserts" is a term I don't understand. Aiui it's the origin device >>>> which would need to de-assert its interrupt, and I think it is not >>>> uncommon for devices to de-assert interrupts after a certain amount >>>> of time. If that wasn't the case, spurious interrupts could never occur. >>> >>> I recall Sander (CC-ed) here hitting this at some point. There was some device >>> he had (legacy?) that would very much hit this path. >>> >>> But I can't recall the details, sorry. >>> >>> Sanders, it was in the context of the dpci softirq work I did if that helps. >> >> Hi Konrad, >> >> You mean these ? > > Yes, but I can't seem to find in those threads the name of the > device you had - the one that was triggering those legacy > interrupts.. By any chance you recall what it was? From your analysis here: https://lists.xenproject.org/archives/html/xen-devel/2014-11/msg01550.html It's the "Multimedia video controller: Conexant Systems, Inc. Device 8210" which is a DVR videograbber. Since it is still in my system, if you need some patches tested with that hardware, i could give them a spin! lspci: 0d:00.0 Multimedia video controller [0400]: Conexant Systems, Inc. Device [14f1:8210] Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0, Cache Line Size: 64 bytes Interrupt: pin A routed to IRQ 28 Region 0: Memory at fe600000 (64-bit, non-prefetchable) [size=2M] Capabilities: [40] Express (v1) Endpoint, MSI 00 DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <2us, L1 <4us ClockPM- Surprise- LLActRep- BwNot- LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk- ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk- DLActive- BWMgmt- ABWMgmt- Capabilities: [80] Power Management version 3 Flags: PMEClk- DSI+ D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold-) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME- Capabilities: [90] Vital Product Data pcilib: sysfs_read_vpd: read failed: Input/output error Not readable Capabilities: [a0] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 00, GenCap- CGenEn- ChkCap- ChkEn- Capabilities: [200 v1] Virtual Channel Caps: LPEVC=1 RefClk=100ns PATEntryBits=1 Arb: Fixed+ WRR32+ WRR64+ WRR128- Ctrl: ArbSelect=WRR64 Status: InProgress- Port Arbitration Table [240] <?> VC0: Caps: PATOffset=00 MaxTimeSlots=1 RejSnoopTrans- Arb: Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256- Ctrl: Enable+ ID=0 ArbSelect=Fixed TC/VC=ff Status: NegoPending- InProgress- VC1: Caps: PATOffset=00 MaxTimeSlots=1 RejSnoopTrans- Arb: Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256- Ctrl: Enable- ID=1 ArbSelect=Fixed TC/VC=00 Status: NegoPending- InProgress- Kernel driver in use: pciback -- Sander >> >> The issue leading up to this revert for xen-4.5: >> https://lists.xen.org/archives/html/xen-devel/2015-01/msg01025.html >> >> Where this seems to be the thread that started the conversation leading up to that revert: >> https://lists.xenproject.org/archives/html/xen-devel/2014-11/msg01330.html >> >> Which than for xen-4.6 continued in a thread with the subject "dpci: Put the dpci back on the list if scheduled from another CPU." >> which is spread out over several months, (this is somewhere in between https://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02102.html ). >> >> -- >> Sander >> >>>> >>>> Jan >>>> >>>> >>>> _______________________________________________ >>>> Xen-devel mailing list >>>> Xen-devel@lists.xen.org >>>> https://lists.xen.org/xen-devel >>
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c index 080183ea31..e0b19ee8c9 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -182,7 +182,6 @@ static void pt_irq_time_out(void *data) pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; } - hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx); } pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL);