Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 19/03/2015 23:51, James Sullivan wrote: I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection So it seems to be the case that logical destination mode is used whenever DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is undefined, as was indicated in the closing response to the thread in https://software.intel.com/en-us/forums/topic/23 : So this means this patch is not necessary, right? Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-04-21 14:18+0200, Paolo Bonzini: On 19/03/2015 23:51, James Sullivan wrote: I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection So it seems to be the case that logical destination mode is used whenever DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is undefined, as was indicated in the closing response to the thread in https://software.intel.com/en-us/forums/topic/23 : So this means this patch is not necessary, right? Yes, and this patch was obsolete even before; the latest version is [PATCH v4 0/2] kvm: x86: Implement handling of RH=1 for MSI delivery in KVM which delivers DM=1+RH=1 as lowest priority. (Several unfortunate things made this quite confusing ...) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-04-02 16:08-0600, James Sullivan: On 03/24/2015 08:03 AM, Radim Krčmář wrote: 2015-03-23 16:46-0600, James Sullivan: On 03/23/2015 03:13 PM, Radim Krčmář wrote: I tested it and Linux got a lot of unexpected NMIs, so the emulation in your latest patch looks correct. Good to hear, thanks for testing that. Any other tweaks you think are necessary for the patch set? No, I thought it fell off my list because I reviewed it; will do so now. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/24/2015 08:03 AM, Radim Krčmář wrote: 2015-03-23 16:46-0600, James Sullivan: On 03/23/2015 03:13 PM, Radim Krčmář wrote: I meant if the delivery mode from data register isn't ignored with RH=1, and the message delivered as if lowest-priority was set there. (Decided by having something else than fixed or lowest-priority there.) Hmm, any thoughts on how I could test for that? Set the MSI data register's delivery mode to NMI/SMI/... The change below fails = hardware honors delivery mode. I tested it and Linux got a lot of unexpected NMIs, so the emulation in your latest patch looks correct. diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h index 4cc48af23fef..2270e459186b 100644 --- a/arch/x86/include/asm/msidef.h +++ b/arch/x86/include/asm/msidef.h @@ -17,6 +17,7 @@ #define MSI_DATA_DELIVERY_MODE_SHIFT 8 #define MSI_DATA_DELIVERY_FIXED (0 MSI_DATA_DELIVERY_MODE_SHIFT) #define MSI_DATA_DELIVERY_LOWPRI(1 MSI_DATA_DELIVERY_MODE_SHIFT) +#define MSI_DATA_DELIVERY_NMI (4 MSI_DATA_DELIVERY_MODE_SHIFT) #define MSI_DATA_LEVEL_SHIFT 14 #define MSI_DATA_LEVEL_DEASSERT(0 MSI_DATA_LEVEL_SHIFT) diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c index d6ba2d660dc5..4f71737c34eb 100644 --- a/arch/x86/kernel/apic/msi.c +++ b/arch/x86/kernel/apic/msi.c @@ -46,7 +46,7 @@ void native_compose_msi_msg(struct pci_dev *pdev, MSI_DATA_LEVEL_ASSERT | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_DATA_DELIVERY_FIXED : - MSI_DATA_DELIVERY_LOWPRI) | + MSI_DATA_DELIVERY_NMI) | MSI_DATA_VECTOR(cfg-vector); } Good to hear, thanks for testing that. Any other tweaks you think are necessary for the patch set? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/24/2015 08:03 AM, Radim Krčmář wrote: 2015-03-23 16:46-0600, James Sullivan: On 03/23/2015 03:13 PM, Radim Krčmář wrote: I meant if the delivery mode from data register isn't ignored with RH=1, and the message delivered as if lowest-priority was set there. (Decided by having something else than fixed or lowest-priority there.) Hmm, any thoughts on how I could test for that? Set the MSI data register's delivery mode to NMI/SMI/... The change below fails = hardware honors delivery mode. I tested it and Linux got a lot of unexpected NMIs, so the emulation in your latest patch looks correct. diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h index 4cc48af23fef..2270e459186b 100644 --- a/arch/x86/include/asm/msidef.h +++ b/arch/x86/include/asm/msidef.h @@ -17,6 +17,7 @@ #define MSI_DATA_DELIVERY_MODE_SHIFT 8 #define MSI_DATA_DELIVERY_FIXED (0 MSI_DATA_DELIVERY_MODE_SHIFT) #define MSI_DATA_DELIVERY_LOWPRI(1 MSI_DATA_DELIVERY_MODE_SHIFT) +#define MSI_DATA_DELIVERY_NMI (4 MSI_DATA_DELIVERY_MODE_SHIFT) #define MSI_DATA_LEVEL_SHIFT 14 #define MSI_DATA_LEVEL_DEASSERT(0 MSI_DATA_LEVEL_SHIFT) diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c index d6ba2d660dc5..4f71737c34eb 100644 --- a/arch/x86/kernel/apic/msi.c +++ b/arch/x86/kernel/apic/msi.c @@ -46,7 +46,7 @@ void native_compose_msi_msg(struct pci_dev *pdev, MSI_DATA_LEVEL_ASSERT | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_DATA_DELIVERY_FIXED : - MSI_DATA_DELIVERY_LOWPRI) | + MSI_DATA_DELIVERY_NMI) | MSI_DATA_VECTOR(cfg-vector); } Great, thanks. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-03-23 16:46-0600, James Sullivan: On 03/23/2015 03:13 PM, Radim Krčmář wrote: I meant if the delivery mode from data register isn't ignored with RH=1, and the message delivered as if lowest-priority was set there. (Decided by having something else than fixed or lowest-priority there.) Hmm, any thoughts on how I could test for that? Set the MSI data register's delivery mode to NMI/SMI/... The change below fails = hardware honors delivery mode. I tested it and Linux got a lot of unexpected NMIs, so the emulation in your latest patch looks correct. diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h index 4cc48af23fef..2270e459186b 100644 --- a/arch/x86/include/asm/msidef.h +++ b/arch/x86/include/asm/msidef.h @@ -17,6 +17,7 @@ #define MSI_DATA_DELIVERY_MODE_SHIFT 8 #define MSI_DATA_DELIVERY_FIXED (0 MSI_DATA_DELIVERY_MODE_SHIFT) #define MSI_DATA_DELIVERY_LOWPRI (1 MSI_DATA_DELIVERY_MODE_SHIFT) +#define MSI_DATA_DELIVERY_NMI (4 MSI_DATA_DELIVERY_MODE_SHIFT) #define MSI_DATA_LEVEL_SHIFT 14 #define MSI_DATA_LEVEL_DEASSERT(0 MSI_DATA_LEVEL_SHIFT) diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c index d6ba2d660dc5..4f71737c34eb 100644 --- a/arch/x86/kernel/apic/msi.c +++ b/arch/x86/kernel/apic/msi.c @@ -46,7 +46,7 @@ void native_compose_msi_msg(struct pci_dev *pdev, MSI_DATA_LEVEL_ASSERT | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_DATA_DELIVERY_FIXED : - MSI_DATA_DELIVERY_LOWPRI) | + MSI_DATA_DELIVERY_NMI) | MSI_DATA_VECTOR(cfg-vector); } -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-03-20 11:50-0600, James Sullivan: On 03/20/2015 09:22 AM, James Sullivan wrote: On 03/20/2015 09:15 AM, Radim Krčmář wrote: 2015-03-19 16:51-0600, James Sullivan: I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection Great! (What CPU family was that?) This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge'). Thanks, it's possible that the behavior of chipsets changed since the report on Intel's forum ... (Lowest priority behaved differently before QPI, so it might coincide.) I'm still wondering about last sentence from that link, the parenthesised part to be exact, The reference to the APIC ID being 0xff is because 0xff is broadcast and lowest priority (what the RH bit really is for X86) is illegal with broadcast. Can you also check if RH=1 does something to delivery mode? I haven't seen any changes in the MSI Data Register for any values of RH, but I don't have a great sample size (one machine with one set of PCI devices), so if anyone else can confirm that I would appreciate it. I meant if the delivery mode from data register isn't ignored with RH=1, and the message delivered as if lowest-priority was set there. (Decided by having something else than fixed or lowest-priority there.) Worth noting that low prio delivery was used across the board for my PCI devices regardless of RH=1 or 0, so it doesn't seem to be de facto the case that the RH bit's only purpose is for lowprio delivery on x86. Yeah, afaik, it can be done with lowest priority delivery mode on ia64 too, so I have a hard time finding RH's intended purpose. Again, need to have some more PCI devices to test against to confirm anything. It's impossible to test everything, and there is no conflict if we have at most one data point ;) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/23/2015 03:13 PM, Radim Krčmář wrote: 2015-03-20 11:50-0600, James Sullivan: On 03/20/2015 09:22 AM, James Sullivan wrote: On 03/20/2015 09:15 AM, Radim Krčmář wrote: 2015-03-19 16:51-0600, James Sullivan: I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection Great! (What CPU family was that?) This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge'). Thanks, it's possible that the behavior of chipsets changed since the report on Intel's forum ... (Lowest priority behaved differently before QPI, so it might coincide.) I'm still wondering about last sentence from that link, the parenthesised part to be exact, The reference to the APIC ID being 0xff is because 0xff is broadcast and lowest priority (what the RH bit really is for X86) is illegal with broadcast. Can you also check if RH=1 does something to delivery mode? I haven't seen any changes in the MSI Data Register for any values of RH, but I don't have a great sample size (one machine with one set of PCI devices), so if anyone else can confirm that I would appreciate it. I meant if the delivery mode from data register isn't ignored with RH=1, and the message delivered as if lowest-priority was set there. (Decided by having something else than fixed or lowest-priority there.) Hmm, any thoughts on how I could test for that? Worth noting that low prio delivery was used across the board for my PCI devices regardless of RH=1 or 0, so it doesn't seem to be de facto the case that the RH bit's only purpose is for lowprio delivery on x86. Yeah, afaik, it can be done with lowest priority delivery mode on ia64 too, so I have a hard time finding RH's intended purpose. Again, need to have some more PCI devices to test against to confirm anything. It's impossible to test everything, and there is no conflict if we have at most one data point ;) Very true :) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-03-19 16:51-0600, James Sullivan: I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection Great! (What CPU family was that?) So it seems to be the case that logical destination mode is used whenever DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is undefined, as was indicated in the closing response to the thread in https://software.intel.com/en-us/forums/topic/23 : DM=0+RH=1 might be defined to fail, but I think it's acceptable to treat it as undefined. (Deliver them in KVM if it improves something.) I'm still wondering about last sentence from that link, the parenthesised part to be exact, The reference to the APIC ID being 0xff is because 0xff is broadcast and lowest priority (what the RH bit really is for X86) is illegal with broadcast. Can you also check if RH=1 does something to delivery mode? Thanks. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/20/2015 09:15 AM, Radim Krčmář wrote: 2015-03-19 16:51-0600, James Sullivan: I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection Great! (What CPU family was that?) This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge'). So it seems to be the case that logical destination mode is used whenever DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is undefined, as was indicated in the closing response to the thread in https://software.intel.com/en-us/forums/topic/23 : DM=0+RH=1 might be defined to fail, but I think it's acceptable to treat it as undefined. (Deliver them in KVM if it improves something.) My thoughts as well. I'm still wondering about last sentence from that link, the parenthesised part to be exact, The reference to the APIC ID being 0xff is because 0xff is broadcast and lowest priority (what the RH bit really is for X86) is illegal with broadcast. Can you also check if RH=1 does something to delivery mode? Thanks. Sure, I'll look into that as well. -James -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/20/2015 09:22 AM, James Sullivan wrote: On 03/20/2015 09:15 AM, Radim Krčmář wrote: 2015-03-19 16:51-0600, James Sullivan: I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection Great! (What CPU family was that?) This was on Intel x86_64 (Core i5-3210m, 'Ivy Bridge'). So it seems to be the case that logical destination mode is used whenever DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is undefined, as was indicated in the closing response to the thread in https://software.intel.com/en-us/forums/topic/23 : DM=0+RH=1 might be defined to fail, but I think it's acceptable to treat it as undefined. (Deliver them in KVM if it improves something.) My thoughts as well. I'm still wondering about last sentence from that link, the parenthesised part to be exact, The reference to the APIC ID being 0xff is because 0xff is broadcast and lowest priority (what the RH bit really is for X86) is illegal with broadcast. Can you also check if RH=1 does something to delivery mode? Thanks. Sure, I'll look into that as well. -James I haven't seen any changes in the MSI Data Register for any values of RH, but I don't have a great sample size (one machine with one set of PCI devices), so if anyone else can confirm that I would appreciate it. Worth noting that low prio delivery was used across the board for my PCI devices regardless of RH=1 or 0, so it doesn't seem to be de facto the case that the RH bit's only purpose is for lowprio delivery on x86. Again, need to have some more PCI devices to test against to confirm anything. -James -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/19/2015 07:00 AM, Radim Krčmář wrote: 2015-03-18 22:09-0300, Marcelo Tosatti: See native_compose_msi_msg: ((apic-irq_dest_mode == 0) ? MSI_ADDR_DEST_MODE_PHYSICAL : MSI_ADDR_DEST_MODE_LOGICAL) | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_ADDR_REDIRECTION_CPU : MSI_ADDR_REDIRECTION_LOWPRI) | So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL and RH = MSI_ADDR_REDIRECTION_LOWPRI. ...and yet this is a good counterexample against my argument :) (It could be just to make the code nicer ... the developer might have known how real hardware will handle it.) What I think I'll do is revert this particular change so that dest_mode is set independently of RH. While I'm not entirely convinced that this is the intended interpretation, I do think that consistency with the existing logic is probably desirable for the time being. If I can get closure on the matter I'll re-submit that change, but for the time being I will undo it. Just write MSI-X table entries on real hardware (say: modify native_compose_msi_msg or MSI-X equivalent), with all RH/DM combinations, and see what behaviour is comes up? I second this idea. (We'd also get to know how RH interacts with delivery mode.) I played around with native_compose_msi_msg and discovered the following: * dm=0, rh=0 = Physical Destination Mode * dm=0, rh=1 = Failed delivery * dm=1, rh=0 = Logical Destination Mode, No Redirection * dm=1, rh=1 = Logical Destination Mode, Redirection So it seems to be the case that logical destination mode is used whenever DM=1, regardless of RH. Furthermore, the case where DM=0 and RH=1 is undefined, as was indicated in the closing response to the thread in https://software.intel.com/en-us/forums/topic/23 : ...X86 supports logical mode but does not support redirection of physical mode interrupts. I think they should have just said RH=1 is not defined in physical mode, but instead they are trying to tell you what the restrictions are if you set it on. The default config for PCI devices seems to be DM=1,RH=1. -James -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-03-18 22:09-0300, Marcelo Tosatti: See native_compose_msi_msg: ((apic-irq_dest_mode == 0) ? MSI_ADDR_DEST_MODE_PHYSICAL : MSI_ADDR_DEST_MODE_LOGICAL) | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_ADDR_REDIRECTION_CPU : MSI_ADDR_REDIRECTION_LOWPRI) | So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL and RH = MSI_ADDR_REDIRECTION_LOWPRI. ...and yet this is a good counterexample against my argument :) (It could be just to make the code nicer ... the developer might have known how real hardware will handle it.) What I think I'll do is revert this particular change so that dest_mode is set independently of RH. While I'm not entirely convinced that this is the intended interpretation, I do think that consistency with the existing logic is probably desirable for the time being. If I can get closure on the matter I'll re-submit that change, but for the time being I will undo it. Just write MSI-X table entries on real hardware (say: modify native_compose_msi_msg or MSI-X equivalent), with all RH/DM combinations, and see what behaviour is comes up? I second this idea. (We'd also get to know how RH interacts with delivery mode.) https://software.intel.com/en-us/forums/topic/23 said that DM=1+RH=0 delivers to physical: The exact quote from 10.11.1 is When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. This does not specify if physical or logical addressing mode is used. Experimentation shows that physical addressing mode is used with RH equal to zero. and it also mentioned a disturbing behavior, which I chose to ignore: 10.11.1 goes on to say that When RH is 1 and the physical destination mode is used [i.e., DM = 0], the Destination ID field must not be set to 0xFF; it must point to a processor that is present and enabled to receive the interrupt. This would seem to be the exact same case as RH equal to zero; there, DM is ignored: If RH is 0, then the DM bit is ignored and the message is sent ahead independent of whether the physical or logical destination mode is used. However, changing RH to 1 and DM to zero fails to send the message to the physical processor. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On Fri, Mar 13, 2015 at 09:14:35AM -0600, James Sullivan wrote: This patch adds a check for RH=1 in kvm_set_msi_irq. Currently the DM bit is the only thing used to decide irq-dest_mode (logical when DM set, physical when unset). Documentation indicates that the DM bit will be 'ignored' when the RH bit is unset, and physical destination mode is used in this case. Fixed this to set irq-dest_mode to APIC_DEST_LOGICAL just in case both RH=1/DM=1. This patch doesn't completely handle RH=1; if RH=1 then the delivery will behave as in low priority mode (deliver the interrupt to only the lowest priority processor), but the delivery mode may still used to specify the semantics of the delivery beyond its destination. I will be trying and comparing a few options to handle this fully (extension of struct kvm_lapic_irq, introduction of MSI specific delivery functions or helpers, etc) and hope to have some patches to show in the near future. Signed-off-by: James Sullivan sullivan.jame...@gmail.com The documentation states the following: * When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. * If RH is 0, then the DM bit is ignored and the message is sent ahead independent of whether the physical or logical destination mode is used. However, from the POV of a device writing to memory to generate an MSI interrupt, there is no (or i can't see any) other information that can be used to infer logical or physical mode for the interrupt message. Before your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 1 (dm, rh) = (1, 1) = irq-dest_mode = 1 After your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 0 (dm, rh) = (1, 1) = irq-dest_mode = 1 Am i missing some explicit documentation that refers to (dm, rh) = (1, 0) = irq-dest_mode = 0 ? See native_compose_msi_msg: msg-address_lo = MSI_ADDR_BASE_LO | ((apic-irq_dest_mode == 0) ? MSI_ADDR_DEST_MODE_PHYSICAL : MSI_ADDR_DEST_MODE_LOGICAL) | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_ADDR_REDIRECTION_CPU : MSI_ADDR_REDIRECTION_LOWPRI) | MSI_ADDR_DEST_ID(dest); So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL and RH = MSI_ADDR_REDIRECTION_LOWPRI. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On Wed, Mar 18, 2015 at 06:59:22PM -0600, James Sullivan wrote: The documentation states the following: * When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. * If RH is 0, then the DM bit is ignored and the message is sent ahead independent of whether the physical or logical destination mode is used. However, from the POV of a device writing to memory to generate an MSI interrupt, there is no (or i can't see any) other information that can be used to infer logical or physical mode for the interrupt message. Before your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 1 (dm, rh) = (1, 1) = irq-dest_mode = 1 After your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 0 (dm, rh) = (1, 1) = irq-dest_mode = 1 Am i missing some explicit documentation that refers to (dm, rh) = (1, 0) = irq-dest_mode = 0 ? From the IA32 manual (Vol. 3, 10.11.2): * When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. * When RH is 1 and the physical destination mode is used, the Destination ID field must not be set to FFH; it must point to a processor that is present and enabled to receive the interrupt. * When RH is 1 and the logical destination mode is active in a system using a flat addressing model, the Destination ID field must be set so that bits set to 1 identify processors that are present and enabled to receive the interrupt. * If RH is set to 1 and the logical destination mode is active in a system using cluster addressing model, then Destination ID field must not be set to FFH; the processors identified with this field must be present and enabled to receive the interrupt. My interpretation of this is that RH=0 indicates that the Dest. ID field contains an APIC ID, and as such destination mode is physical. When RH=1, depending on the value of DM, we either use physical or logical dest mode. The result of this is that logical dest mode is set just when RH=1/DM=1, as far as I understand. See native_compose_msi_msg: msg-address_lo = MSI_ADDR_BASE_LO | ((apic-irq_dest_mode == 0) ? MSI_ADDR_DEST_MODE_PHYSICAL : MSI_ADDR_DEST_MODE_LOGICAL) | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_ADDR_REDIRECTION_CPU : MSI_ADDR_REDIRECTION_LOWPRI) | MSI_ADDR_DEST_ID(dest); So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL and RH = MSI_ADDR_REDIRECTION_LOWPRI. ...and yet this is a good counterexample against my argument :) What I think I'll do is revert this particular change so that dest_mode is set independently of RH. While I'm not entirely convinced that this is the intended interpretation, I do think that consistency with the existing logic is probably desirable for the time being. If I can get closure on the matter I'll re-submit that change, but for the time being I will undo it. -James Just write MSI-X table entries on real hardware (say: modify native_compose_msi_msg or MSI-X equivalent), with all RH/DM combinations, and see what behaviour is comes up? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On Wed, Mar 18, 2015 at 06:59:22PM -0600, James Sullivan wrote: The documentation states the following: * When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. * If RH is 0, then the DM bit is ignored and the message is sent ahead independent of whether the physical or logical destination mode is used. However, from the POV of a device writing to memory to generate an MSI interrupt, there is no (or i can't see any) other information that can be used to infer logical or physical mode for the interrupt message. Before your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 1 (dm, rh) = (1, 1) = irq-dest_mode = 1 After your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 0 (dm, rh) = (1, 1) = irq-dest_mode = 1 Am i missing some explicit documentation that refers to (dm, rh) = (1, 0) = irq-dest_mode = 0 ? From the IA32 manual (Vol. 3, 10.11.2): * When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. * When RH is 1 and the physical destination mode is used, the Destination ID field must not be set to FFH; it must point to a processor that is present and enabled to receive the interrupt. * When RH is 1 and the logical destination mode is active in a system using a flat addressing model, the Destination ID field must be set so that bits set to 1 identify processors that are present and enabled to receive the interrupt. * If RH is set to 1 and the logical destination mode is active in a system using cluster addressing model, then Destination ID field must not be set to FFH; the processors identified with this field must be present and enabled to receive the interrupt. My interpretation of this is that RH=0 indicates that the Dest. ID field contains an APIC ID, and as such destination mode is physical. When RH=1, depending on the value of DM, we either use physical or logical dest mode. The result of this is that logical dest mode is set just when RH=1/DM=1, as far as I understand. See native_compose_msi_msg: msg-address_lo = MSI_ADDR_BASE_LO | ((apic-irq_dest_mode == 0) ? MSI_ADDR_DEST_MODE_PHYSICAL : MSI_ADDR_DEST_MODE_LOGICAL) | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_ADDR_REDIRECTION_CPU : MSI_ADDR_REDIRECTION_LOWPRI) | MSI_ADDR_DEST_ID(dest); So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL and RH = MSI_ADDR_REDIRECTION_LOWPRI. ...and yet this is a good counterexample against my argument :) What I think I'll do is revert this particular change so that dest_mode is set independently of RH. While I'm not entirely convinced that this is the intended interpretation, Where would the logical/physical information come from, if not from the DM bit ? I do think that consistency with the existing logic is probably desirable for the time being. If I can get closure on the matter I'll re-submit that change, but for the time being I will undo it. -James -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
The documentation states the following: * When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. * If RH is 0, then the DM bit is ignored and the message is sent ahead independent of whether the physical or logical destination mode is used. However, from the POV of a device writing to memory to generate an MSI interrupt, there is no (or i can't see any) other information that can be used to infer logical or physical mode for the interrupt message. Before your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 1 (dm, rh) = (1, 1) = irq-dest_mode = 1 After your patch: (dm, rh) = (0, 0) = irq-dest_mode = 0 (dm, rh) = (0, 1) = irq-dest_mode = 0 (dm, rh) = (1, 0) = irq-dest_mode = 0 (dm, rh) = (1, 1) = irq-dest_mode = 1 Am i missing some explicit documentation that refers to (dm, rh) = (1, 0) = irq-dest_mode = 0 ? From the IA32 manual (Vol. 3, 10.11.2): * When RH is 0, the interrupt is directed to the processor listed in the Destination ID field. * When RH is 1 and the physical destination mode is used, the Destination ID field must not be set to FFH; it must point to a processor that is present and enabled to receive the interrupt. * When RH is 1 and the logical destination mode is active in a system using a flat addressing model, the Destination ID field must be set so that bits set to 1 identify processors that are present and enabled to receive the interrupt. * If RH is set to 1 and the logical destination mode is active in a system using cluster addressing model, then Destination ID field must not be set to FFH; the processors identified with this field must be present and enabled to receive the interrupt. My interpretation of this is that RH=0 indicates that the Dest. ID field contains an APIC ID, and as such destination mode is physical. When RH=1, depending on the value of DM, we either use physical or logical dest mode. The result of this is that logical dest mode is set just when RH=1/DM=1, as far as I understand. See native_compose_msi_msg: msg-address_lo = MSI_ADDR_BASE_LO | ((apic-irq_dest_mode == 0) ? MSI_ADDR_DEST_MODE_PHYSICAL : MSI_ADDR_DEST_MODE_LOGICAL) | ((apic-irq_delivery_mode != dest_LowestPrio) ? MSI_ADDR_REDIRECTION_CPU : MSI_ADDR_REDIRECTION_LOWPRI) | MSI_ADDR_DEST_ID(dest); So it does configure DM = MSI_ADDR_DEST_MODE_LOGICAL and RH = MSI_ADDR_REDIRECTION_LOWPRI. ...and yet this is a good counterexample against my argument :) What I think I'll do is revert this particular change so that dest_mode is set independently of RH. While I'm not entirely convinced that this is the intended interpretation, I do think that consistency with the existing logic is probably desirable for the time being. If I can get closure on the matter I'll re-submit that change, but for the time being I will undo it. -James -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
N.B. This patch has been re-submitted in a larger patch, see 1426555822-3280-1-git-send-email-sullivan.jame...@gmail.com (The new patch relies on changes made in this patch, and as such it makes more sense to bundle them) On 03/13/2015 09:14 AM, James Sullivan wrote: This patch adds a check for RH=1 in kvm_set_msi_irq. Currently the DM bit is the only thing used to decide irq-dest_mode (logical when DM set, physical when unset). Documentation indicates that the DM bit will be 'ignored' when the RH bit is unset, and physical destination mode is used in this case. Fixed this to set irq-dest_mode to APIC_DEST_LOGICAL just in case both RH=1/DM=1. This patch doesn't completely handle RH=1; if RH=1 then the delivery will behave as in low priority mode (deliver the interrupt to only the lowest priority processor), but the delivery mode may still used to specify the semantics of the delivery beyond its destination. I will be trying and comparing a few options to handle this fully (extension of struct kvm_lapic_irq, introduction of MSI specific delivery functions or helpers, etc) and hope to have some patches to show in the near future. Signed-off-by: James Sullivan sullivan.jame...@gmail.com --- Changes since v2: * Added one time warning message when RH=1 * Documented conflict between RH=1 and delivery mode * Tidied code to check RH=1/DM=1 (remove bool phys, use if/else) Changes since v3: * Fixed logical error in RH=1/DM=1 check * Aligned quotation blocks in multiline pr_warn_once argument Changes since v4: * Put error message string on single line arch/x86/kvm/irq_comm.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c index 72298b3..f2887ea 100644 --- a/arch/x86/kvm/irq_comm.c +++ b/arch/x86/kvm/irq_comm.c @@ -103,12 +103,24 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e, MSI_ADDR_DEST_ID_MASK) MSI_ADDR_DEST_ID_SHIFT; irq-vector = (e-msi.data MSI_DATA_VECTOR_MASK) MSI_DATA_VECTOR_SHIFT; - irq-dest_mode = (1 MSI_ADDR_DEST_MODE_SHIFT) e-msi.address_lo; + /* + * TODO Deal with RH bit of MSI message address + * IF RH=1, then MSI delivers only to the processor with the + * lowest interrupt priority among processors that can receive + * the interrupt. + */ + if ((e-msi.address_lo MSI_ADDR_REDIRECTION_LOWPRI) + (e-msi.address_lo MSI_ADDR_DEST_MODE_LOGICAL)) + irq-dest_mode = APIC_DEST_LOGICAL; + else + irq-dest_mode = APIC_DEST_PHYSICAL; irq-trig_mode = (1 MSI_DATA_TRIGGER_SHIFT) e-msi.data; irq-delivery_mode = e-msi.data 0x700; + if (e-msi.address_lo MSI_ADDR_REDIRECTION_LOWPRI) + pr_warn_once( + kvm: MSIs may not be correctly delivered with RH set.\n); irq-level = 1; irq-shorthand = 0; - /* TODO Deal with RH bit of MSI message address */ } int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
This patch adds a check for RH=1 in kvm_set_msi_irq. Currently the DM bit is the only thing used to decide irq-dest_mode (logical when DM set, physical when unset). Documentation indicates that the DM bit will be 'ignored' when the RH bit is unset, and physical destination mode is used in this case. Fixed this to set irq-dest_mode to APIC_DEST_LOGICAL just in case both RH=1/DM=1. This patch doesn't completely handle RH=1; if RH=1 then the delivery will behave as in low priority mode (deliver the interrupt to only the lowest priority processor), but the delivery mode may still used to specify the semantics of the delivery beyond its destination. I will be trying and comparing a few options to handle this fully (extension of struct kvm_lapic_irq, introduction of MSI specific delivery functions or helpers, etc) and hope to have some patches to show in the near future. Signed-off-by: James Sullivan sullivan.jame...@gmail.com --- Changes since v2: * Added one time warning message when RH=1 * Documented conflict between RH=1 and delivery mode * Tidied code to check RH=1/DM=1 (remove bool phys, use if/else) Changes since v3: * Fixed logical error in RH=1/DM=1 check * Aligned quotation blocks in multiline pr_warn_once argument Changes since v4: * Put error message string on single line arch/x86/kvm/irq_comm.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c index 72298b3..f2887ea 100644 --- a/arch/x86/kvm/irq_comm.c +++ b/arch/x86/kvm/irq_comm.c @@ -103,12 +103,24 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e, MSI_ADDR_DEST_ID_MASK) MSI_ADDR_DEST_ID_SHIFT; irq-vector = (e-msi.data MSI_DATA_VECTOR_MASK) MSI_DATA_VECTOR_SHIFT; - irq-dest_mode = (1 MSI_ADDR_DEST_MODE_SHIFT) e-msi.address_lo; + /* +* TODO Deal with RH bit of MSI message address +* IF RH=1, then MSI delivers only to the processor with the +* lowest interrupt priority among processors that can receive +* the interrupt. +*/ + if ((e-msi.address_lo MSI_ADDR_REDIRECTION_LOWPRI) + (e-msi.address_lo MSI_ADDR_DEST_MODE_LOGICAL)) + irq-dest_mode = APIC_DEST_LOGICAL; + else + irq-dest_mode = APIC_DEST_PHYSICAL; irq-trig_mode = (1 MSI_DATA_TRIGGER_SHIFT) e-msi.data; irq-delivery_mode = e-msi.data 0x700; + if (e-msi.address_lo MSI_ADDR_REDIRECTION_LOWPRI) + pr_warn_once( + kvm: MSIs may not be correctly delivered with RH set.\n); irq-level = 1; irq-shorthand = 0; - /* TODO Deal with RH bit of MSI message address */ } int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, -- 2.3.1 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-03-13 09:14-0600, James Sullivan: --- Changes since v2: * Added one time warning message when RH=1 * Documented conflict between RH=1 and delivery mode * Tidied code to check RH=1/DM=1 (remove bool phys, use if/else) Changes since v3: * Fixed logical error in RH=1/DM=1 check * Aligned quotation blocks in multiline pr_warn_once argument Changes since v4: * Put error message string on single line Thanks, Reviewed-by: Radim Krčmář rkrc...@redhat.com diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c @@ -103,12 +103,24 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e, + if (e-msi.address_lo MSI_ADDR_REDIRECTION_LOWPRI) + pr_warn_once( + kvm: MSIs may not be correctly delivered with RH set.\n); (The joys of 80+ columns!) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
