RE: x86/irq: Unbreak interrupt affinity setting
From: Alexander Graf > Sent: 26 August 2020 23:53 > > On 26.08.20 23:47, David Laight wrote: > > > > From: David Laight > >> Sent: 26 August 2020 22:37 > >> > >> From: Thomas Gleixner > >>> Sent: 26 August 2020 21:22 > >> ... > >>> Moving interrupts on x86 happens in several steps. A new vector on a > >>> different CPU is allocated and the relevant interrupt source is > >>> reprogrammed to that. But that's racy and there might be an interrupt > >>> already in flight to the old vector. So the old vector is preserved until > >>> the first interrupt arrives on the new vector and the new target CPU. Once > >>> that happens the old vector is cleaned up, but this cleanup still depends > >>> on the vector number being stored in pt_regs::orig_ax, which is now -1. > >> > >> I suspect that it is much more 'racy' than that for PCI-X interrupts. > >> On the hardware side there is an interrupt disable bit, and address > >> and a value. > >> To raise an interrupt the hardware must write the value to the address. > >> > >> If the cpu needs to move an interrupt both the address and value > >> need changing, but the cpu wont write the address and value using > >> the same TLP, so the hardware could potentially write a value to > >> the wrong address. > >> Worse than that, the hardware could easily only look at the address > >> and value in the clocks after checking the interrupt is enabled. > >> So masking the interrupt immediately prior to changing the vector > >> info may not be enough. > >> > >> It is likely that a read-back of the mask before updating the vector > >> is enough. > > > > But not enough to assume you won't receive an interrupt after reading > > back that interrupts are masked. > > > > (I've implemented the hardware side for an fpga ...) > > Do we actually care in this context? All we want to know here is whether > a device (or irqchip in between) has actually noticed that it should > post to a new vector. If we get interrupts on random other vectors in > between, they would simply show up as spurious, no? > > So I don't quite see where this patch makes the situation any worse than > before. Oh, it won't make it any worse. It just might be rather worse than anyone imagined. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
RE: x86/irq: Unbreak interrupt affinity setting
From: Thomas Gleixner > Sent: 26 August 2020 23:08 ... > > I suspect that it is much more 'racy' than that for PCI-X interrupts. > > On the hardware side there is an interrupt disable bit, and address > > and a value. > > To raise an interrupt the hardware must write the value to the > > address. > > Really? Yep, anyone with write access to the msi-x table can get the device to write to any physical location (allowed by any IOMMU) instead of raising an interrupt. > > If the cpu needs to move an interrupt both the address and value > > need changing, but the cpu wont write the address and value using > > the same TLP, so the hardware could potentially write a value to > > the wrong address. > > Now I understand finally why msi_set_affinity() in x86 has to be so > convoluted. Updating the registers should be much the same on all architectures. I probably should have looked at what msi_set_affinity() does before deciding which order the fpga logic should read the four 32bit registers in; but they are read in increasing order - so enable bit last. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: x86/irq: Unbreak interrupt affinity setting
On 26.08.20 23:47, David Laight wrote: From: David Laight Sent: 26 August 2020 22:37 From: Thomas Gleixner Sent: 26 August 2020 21:22 ... Moving interrupts on x86 happens in several steps. A new vector on a different CPU is allocated and the relevant interrupt source is reprogrammed to that. But that's racy and there might be an interrupt already in flight to the old vector. So the old vector is preserved until the first interrupt arrives on the new vector and the new target CPU. Once that happens the old vector is cleaned up, but this cleanup still depends on the vector number being stored in pt_regs::orig_ax, which is now -1. I suspect that it is much more 'racy' than that for PCI-X interrupts. On the hardware side there is an interrupt disable bit, and address and a value. To raise an interrupt the hardware must write the value to the address. If the cpu needs to move an interrupt both the address and value need changing, but the cpu wont write the address and value using the same TLP, so the hardware could potentially write a value to the wrong address. Worse than that, the hardware could easily only look at the address and value in the clocks after checking the interrupt is enabled. So masking the interrupt immediately prior to changing the vector info may not be enough. It is likely that a read-back of the mask before updating the vector is enough. But not enough to assume you won't receive an interrupt after reading back that interrupts are masked. (I've implemented the hardware side for an fpga ...) Do we actually care in this context? All we want to know here is whether a device (or irqchip in between) has actually noticed that it should post to a new vector. If we get interrupts on random other vectors in between, they would simply show up as spurious, no? So I don't quite see where this patch makes the situation any worse than before. Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
RE: x86/irq: Unbreak interrupt affinity setting
On Wed, Aug 26 2020 at 21:37, David Laight wrote: > From: Thomas Gleixner >> Sent: 26 August 2020 21:22 > ... >> Moving interrupts on x86 happens in several steps. A new vector on a >> different CPU is allocated and the relevant interrupt source is >> reprogrammed to that. But that's racy and there might be an interrupt >> already in flight to the old vector. So the old vector is preserved until >> the first interrupt arrives on the new vector and the new target CPU. Once >> that happens the old vector is cleaned up, but this cleanup still depends >> on the vector number being stored in pt_regs::orig_ax, which is now -1. > > I suspect that it is much more 'racy' than that for PCI-X interrupts. > On the hardware side there is an interrupt disable bit, and address > and a value. > To raise an interrupt the hardware must write the value to the > address. Really? > If the cpu needs to move an interrupt both the address and value > need changing, but the cpu wont write the address and value using > the same TLP, so the hardware could potentially write a value to > the wrong address. Now I understand finally why msi_set_affinity() in x86 has to be so convoluted. Thanks a lot for the enlightment! tglx
RE: x86/irq: Unbreak interrupt affinity setting
From: David Laight > Sent: 26 August 2020 22:37 > > From: Thomas Gleixner > > Sent: 26 August 2020 21:22 > ... > > Moving interrupts on x86 happens in several steps. A new vector on a > > different CPU is allocated and the relevant interrupt source is > > reprogrammed to that. But that's racy and there might be an interrupt > > already in flight to the old vector. So the old vector is preserved until > > the first interrupt arrives on the new vector and the new target CPU. Once > > that happens the old vector is cleaned up, but this cleanup still depends > > on the vector number being stored in pt_regs::orig_ax, which is now -1. > > I suspect that it is much more 'racy' than that for PCI-X interrupts. > On the hardware side there is an interrupt disable bit, and address > and a value. > To raise an interrupt the hardware must write the value to the address. > > If the cpu needs to move an interrupt both the address and value > need changing, but the cpu wont write the address and value using > the same TLP, so the hardware could potentially write a value to > the wrong address. > Worse than that, the hardware could easily only look at the address > and value in the clocks after checking the interrupt is enabled. > So masking the interrupt immediately prior to changing the vector > info may not be enough. > > It is likely that a read-back of the mask before updating the vector > is enough. But not enough to assume you won't receive an interrupt after reading back that interrupts are masked. (I've implemented the hardware side for an fpga ...) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
RE: x86/irq: Unbreak interrupt affinity setting
From: Thomas Gleixner > Sent: 26 August 2020 21:22 ... > Moving interrupts on x86 happens in several steps. A new vector on a > different CPU is allocated and the relevant interrupt source is > reprogrammed to that. But that's racy and there might be an interrupt > already in flight to the old vector. So the old vector is preserved until > the first interrupt arrives on the new vector and the new target CPU. Once > that happens the old vector is cleaned up, but this cleanup still depends > on the vector number being stored in pt_regs::orig_ax, which is now -1. I suspect that it is much more 'racy' than that for PCI-X interrupts. On the hardware side there is an interrupt disable bit, and address and a value. To raise an interrupt the hardware must write the value to the address. If the cpu needs to move an interrupt both the address and value need changing, but the cpu wont write the address and value using the same TLP, so the hardware could potentially write a value to the wrong address. Worse than that, the hardware could easily only look at the address and value in the clocks after checking the interrupt is enabled. So masking the interrupt immediately prior to changing the vector info may not be enough. It is likely that a read-back of the mask before updating the vector is enough. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
