Re: [PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/11/2015 07:43 AM, Radim Krčmář wrote: 2015-03-10 16:39-0600, James Sullivan: On 03/10/2015 08:47 AM, Radim Krčmář wrote: + irq-dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL); (Should be APIC_DEST_LOGICAL. All works because it is a boolean and we only checked for APIC_DEST_PHYSICAL, which is 0.) Thank you, that should be as follows: irq-dest_mode = phys ? (APIC_DEST_PHYSICAL) : (APIC_DEST_LOGICAL); ? Yes, thanks. (No need for parentheses around macros, we agreed to cover that in definition and they don't make this more readable.) I'll tidy that up too. Your patch improves the situation, but removing the TODO is not warranted -- RH still doesn't do what it should: How does DM=1/RH=1 work on real hardware? (There seem to be interesting corner cases with irq-delivery_mode like APIC_DM_NMI.) The IA32 manual says that if DM=1/RH=1, we operate in logical destination mode similarly to other IPIs. I don't believe this patch introduces any invalid settings listed in section 10-21, Vol. 3, so this shouldn't create any weirdness. Quoting SDM Jan 2015, 10.11.1 Message Address Register Format: This bit [RH] indicates whether the message should be directed to the processor with the lowest interrupt priority among processors that can receive the interrupt. = it should behave like lowest priority delivery. Deliver to just one APIC -- we don't do that. KVM interrupt delivery functions can only specify lowest priority though delivery_mode and we would have to rework KVM if MSI can set something else in the delivery_mode (like NMI). I see-- That will pose issues when you want some particular semantics of one delivery mode (such as ignoring vector info in NMI) but want low-pri delivery. Short of writing an MSI specific delivery function and calling it in kvm_set_msi, I can't think of a way to get this. Do you think that's warranted? I'll document this and resubmit this patch with the TODO for now. Thanks for the insight. -- 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] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-03-10 16:39-0600, James Sullivan:
On 03/10/2015 08:47 AM, Radim Krčmář wrote:
+ irq-dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);
(Should be APIC_DEST_LOGICAL. All works because it is a boolean and we
only checked for APIC_DEST_PHYSICAL, which is 0.)
Thank you, that should be as follows:
irq-dest_mode = phys ? (APIC_DEST_PHYSICAL) : (APIC_DEST_LOGICAL);
?
Yes, thanks. (No need for parentheses around macros, we agreed to
cover that in definition and they don't make this more readable.)
- /* TODO Deal with RH bit of MSI message address */
RH bit still isn't deal with -- we do not use lowest-priority-like
delivery in logical destination mode ...
Just want to make sure I understand this comment-
Isn't low-pri delivery mode used in kvm_irq_delivery_to_apic_fast
when irq-dest_mode APIC_DEST_PHYSICAL (ie, logical)? (See below.)
Not necessarily -- we have 'dest_mode' and 'delivery_mode';
- dest_mode is either logical or physical
- delivery_mode is fixed/lowest-priority/NMI/...
Logical destination can be used with multiple delivery modes and
lowest-priority is just one of them:
bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
| [...]
if (irq-dest_mode == APIC_DEST_PHYSICAL) {
| [...]
} else {
| [...]
if (irq-delivery_mode == APIC_DM_LOWEST) {
Without lowest priority, it delivers to all matching APICs.
Do you mean that this patch will interfere with this? As long as
irq-dest_mode is still APIC_DEST_LOGICAL this shouldn't change.
Your patch improves the situation, but removing the TODO is not
warranted -- RH still doesn't do what it should:
How does DM=1/RH=1 work on real hardware?
(There seem to be interesting corner cases with irq-delivery_mode like
APIC_DM_NMI.)
The IA32 manual says that if DM=1/RH=1, we operate in logical destination mode
similarly to other IPIs. I don't believe this patch introduces any invalid
settings listed in section 10-21, Vol. 3, so this shouldn't create any
weirdness.
Quoting SDM Jan 2015, 10.11.1 Message Address Register Format:
This bit [RH] indicates whether the message should be directed to the
processor with the lowest interrupt priority among processors that can
receive the interrupt.
= it should behave like lowest priority delivery.
Deliver to just one APIC -- we don't do that.
KVM interrupt delivery functions can only specify lowest priority though
delivery_mode and we would have to rework KVM if MSI can set something
else in the delivery_mode (like NMI).
--
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] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
2015-03-09 23:05-0600, James Sullivan:
Hi folks,
This is a small patch that implements logic to handle the RH bit
being set in the msi message address register. 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.
(I found that https://software.intel.com/en-us/forums/topic/23 agrees
with this definition of ignored; and it explains the weird design at
the bottom.)
Fixed this to set irq-dest_mode to logical just when both bits are set,
and physical otherwise.
Signed-off-by: James Sullivan sullivan.jame...@gmail.com
---
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
@@ -97,18 +97,26 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct
kvm_lapic *src,
static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
struct kvm_lapic_irq *irq)
{
+ bool phys;
trace_kvm_msi_set_irq(e-msi.address_lo, e-msi.data);
irq-dest_id = (e-msi.address_lo
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;
+ /*
+ * Set dest_mode to logical just in case both the RH and DM
+ * bits are set, otherwise default to physical.
+ */
+ phys = ((e-msi.address_lo (MSI_ADDR_REDIRECTION_LOWPRI |
+ MSI_ADDR_DEST_MODE_LOGICAL)) !=
+ (MSI_ADDR_REDIRECTION_LOWPRI |
+ MSI_ADDR_DEST_MODE_LOGICAL));
+ irq-dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);
(Should be APIC_DEST_LOGICAL. All works because it is a boolean and we
only checked for APIC_DEST_PHYSICAL, which is 0.)
irq-trig_mode = (1 MSI_DATA_TRIGGER_SHIFT) e-msi.data;
irq-delivery_mode = e-msi.data 0x700;
irq-level = 1;
irq-shorthand = 0;
- /* TODO Deal with RH bit of MSI message address */
RH bit still isn't deal with -- we do not use lowest-priority-like
delivery in logical destination mode ...
How does DM=1/RH=1 work on real hardware?
(There seem to be interesting corner cases with irq-delivery_mode like
APIC_DM_NMI.)
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] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
On 03/10/2015 08:47 AM, Radim Krčmář wrote:
...
+/*
+ * Set dest_mode to logical just in case both the RH and DM
+ * bits are set, otherwise default to physical.
+ */
+phys = ((e-msi.address_lo (MSI_ADDR_REDIRECTION_LOWPRI |
+MSI_ADDR_DEST_MODE_LOGICAL)) !=
+(MSI_ADDR_REDIRECTION_LOWPRI |
+MSI_ADDR_DEST_MODE_LOGICAL));
+irq-dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);
(Should be APIC_DEST_LOGICAL. All works because it is a boolean and we
only checked for APIC_DEST_PHYSICAL, which is 0.)
Thank you, that should be as follows:
irq-dest_mode = phys ? (APIC_DEST_PHYSICAL) : (APIC_DEST_LOGICAL);
?
irq-trig_mode = (1 MSI_DATA_TRIGGER_SHIFT) e-msi.data;
irq-delivery_mode = e-msi.data 0x700;
irq-level = 1;
irq-shorthand = 0;
-/* TODO Deal with RH bit of MSI message address */
RH bit still isn't deal with -- we do not use lowest-priority-like
delivery in logical destination mode ...
Just want to make sure I understand this comment-
Isn't low-pri delivery mode used in kvm_irq_delivery_to_apic_fast
when irq-dest_mode APIC_DEST_PHYSICAL (ie, logical)? (See below.)
Do you mean that this patch will interfere with this? As long as
irq-dest_mode is still APIC_DEST_LOGICAL this shouldn't change.
bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
{
.
if (irq-dest_mode == APIC_DEST_PHYSICAL) {
if (irq-dest_id = ARRAY_SIZE(map-phys_map))
goto out;
dst = map-phys_map[irq-dest_id];
} else {
u32 mda = irq-dest_id (32 - map-ldr_bits);
u16 cid = apic_cluster_id(map, mda);
if (cid = ARRAY_SIZE(map-logical_map))
goto out;
dst = map-logical_map[cid];
bitmap = apic_logical_id(map, mda);
if (irq-delivery_mode == APIC_DM_LOWEST) {
int l = -1;
for_each_set_bit(i, bitmap, 16) {
if (!dst[i])
continue;
if (l 0)
l = i;
else if (kvm_apic_compare_prio(dst[i]-vcpu,
dst[l]-vcpu) 0)
l = i;
}
bitmap = (l = 0) ? 1 l : 0;
}
}
.
}
How does DM=1/RH=1 work on real hardware?
(There seem to be interesting corner cases with irq-delivery_mode like
APIC_DM_NMI.)
The IA32 manual says that if DM=1/RH=1, we operate in logical destination mode
similarly to other IPIs. I don't believe this patch introduces any invalid
settings listed in section 10-21, Vol. 3, so this shouldn't create any
weirdness.
Thanks
-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
[PATCH] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq
Hi folks,
This is a small patch that implements logic to handle the RH bit
being set in the msi message address register. 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 logical just when both bits are set,
and physical otherwise.
Signed-off-by: James Sullivan sullivan.jame...@gmail.com
---
arch/x86/kvm/irq_comm.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 72298b3..26bbab8 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -97,18 +97,26 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct
kvm_lapic *src,
static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
struct kvm_lapic_irq *irq)
{
+ bool phys;
trace_kvm_msi_set_irq(e-msi.address_lo, e-msi.data);
irq-dest_id = (e-msi.address_lo
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;
+ /*
+* Set dest_mode to logical just in case both the RH and DM
+* bits are set, otherwise default to physical.
+*/
+ phys = ((e-msi.address_lo (MSI_ADDR_REDIRECTION_LOWPRI |
+ MSI_ADDR_DEST_MODE_LOGICAL)) !=
+ (MSI_ADDR_REDIRECTION_LOWPRI |
+ MSI_ADDR_DEST_MODE_LOGICAL));
+ irq-dest_mode = phys ? 0 : (MSI_ADDR_DEST_MODE_LOGICAL);
irq-trig_mode = (1 MSI_DATA_TRIGGER_SHIFT) e-msi.data;
irq-delivery_mode = e-msi.data 0x700;
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
