| Message ID | 1428363937-19003-6-git-send-email-sullivan.james.f@gmail.com |
|---|---|
| State | New |
| Headers | show |
2015-04-06 17:45-0600, James Sullivan: > Added argument to apic_get_delivery_bitmask() for msi_redir_hint, > and changed calls to the function accordingly (using 0 as a default > value for non-MSI interrupts). > > Modified the implementation of apic_get_delivery_bitmask() to account > for the RH bit of an MSI IRQ. The RH bit indicates that the message > should target only the lowest-priority processor among those specified > by the logical destination of the IRQ. > > Signed-off-by: James Sullivan <sullivan.james.f@gmail.com> > --- > diff --git a/hw/intc/apic.c b/hw/intc/apic.c > @@ -519,23 +521,27 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, > } > } else { > /* XXX: cluster mode */ > + int l = -1; > memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t)); > for(i = 0; i < MAX_APICS; i++) { > apic_iter = local_apics[i]; > + if (!apic_iter) { > + break; > + } (I wonder if QEMU would allow 'for(i = 0; i < MAX_APICS && (apic_iter = local_apics[i]); i++) {') > + if (apic_match_dest(apic_iter, dest_mode, dest)) { > + if (msi_redir_hint) { You could check for APIC_DM_LOWPRI here as well and save the apic_lowest_prio() loop in patch [1/4]. LOWPRI would be delivered like FIXED. > + if (l < 0 || > + apic_compare_prio(apic_iter, local_apics[l]) < 0) { > + l = i; (Btw. lowest priority has a lot of cases that are forbidden ... - in combination with physical broadcast - in combination with clustered logical broadcast - to invalid/disabled destinations These most likely won't work correctly in real hardware. Lowest priority is a bad concept for large systems, which is why Intel stopped implementing it.)
On 04/23/2015 08:14 AM, Radim Krčmář wrote: > 2015-04-06 17:45-0600, James Sullivan: >> Added argument to apic_get_delivery_bitmask() for msi_redir_hint, >> and changed calls to the function accordingly (using 0 as a default >> value for non-MSI interrupts). >> >> Modified the implementation of apic_get_delivery_bitmask() to account >> for the RH bit of an MSI IRQ. The RH bit indicates that the message >> should target only the lowest-priority processor among those specified >> by the logical destination of the IRQ. >> >> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com> >> --- >> diff --git a/hw/intc/apic.c b/hw/intc/apic.c >> @@ -519,23 +521,27 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, >> } >> } else { >> /* XXX: cluster mode */ >> + int l = -1; >> memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t)); >> for(i = 0; i < MAX_APICS; i++) { >> apic_iter = local_apics[i]; >> + if (!apic_iter) { >> + break; >> + } > > (I wonder if QEMU would allow > 'for(i = 0; i < MAX_APICS && (apic_iter = local_apics[i]); i++) {') > >> + if (apic_match_dest(apic_iter, dest_mode, dest)) { >> + if (msi_redir_hint) { > > You could check for APIC_DM_LOWPRI here as well and save the > apic_lowest_prio() loop in patch [1/4]. > LOWPRI would be delivered like FIXED. > I was considering doing that, saving the loop is a big plus. My concern was that it would change get_delivery_bitmask's semantics in a way that could be confusing (i.e. it is currently expected that the caller is responsible for doing arbitration after the fact, this would flip that responsibility). >> + if (l < 0 || >> + apic_compare_prio(apic_iter, local_apics[l]) < 0) { >> + l = i; > > (Btw. lowest priority has a lot of cases that are forbidden ... > - in combination with physical broadcast > - in combination with clustered logical broadcast > - to invalid/disabled destinations > > These most likely won't work correctly in real hardware. > Lowest priority is a bad concept for large systems, which is why Intel > stopped implementing it.) > Checking for such illegal cases should probably happen in the delivery routines before we set the delivery bitmask (in apic_bus_deliver()?)
2015-04-23 13:08-0600, James Sullivan: > On 04/23/2015 08:14 AM, Radim Krčmář wrote: >> 2015-04-06 17:45-0600, James Sullivan: >>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c >>> @@ -519,23 +521,27 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, >>> + if (apic_match_dest(apic_iter, dest_mode, dest)) { >>> + if (msi_redir_hint) { >> >> You could check for APIC_DM_LOWPRI here as well and save the >> apic_lowest_prio() loop in patch [1/4]. >> LOWPRI would be delivered like FIXED. > > I was considering doing that, saving the loop is a big plus. My concern > was that it would change get_delivery_bitmask's semantics in a way that > could be confusing (i.e. it is currently expected that the caller is > responsible for doing arbitration after the fact, this would flip that > responsibility). Good concern! In this case, I think it's ok to change semantics, as the function is static. All callers are going to be in this module and they call apic_bus_deliver() right after apic_get_delivery_bitmask() now. (It's not like we could break interrupt delivery with this change, only logging or some other meta-operation would be affected.) >> (Btw. lowest priority has a lot of cases that are forbidden ... >> - in combination with physical broadcast >> - in combination with clustered logical broadcast >> - to invalid/disabled destinations >> >> These most likely won't work correctly in real hardware. >> Lowest priority is a bad concept for large systems, which is why Intel >> stopped implementing it.) >> > > Checking for such illegal cases should probably happen in the delivery > routines before we set the delivery bitmask (in apic_bus_deliver()?) What we have now is fine. I presented them to lift constraints that might have prevented you from making different decisions; software mustn't configure them, so we're free to do whatever. (I think it would be better if they didn't work, but haven't read enough QEMU code to know what is the approach here.)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c index f3ec2c8..f4cc1df 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -37,7 +37,8 @@ static APICCommonState *local_apics[MAX_APICS + 1]; static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode); static void apic_update_irq(APICCommonState *s); static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, - uint8_t dest, uint8_t dest_mode); + uint8_t dest, uint8_t dest_mode, + uint8_t msi_redir_hint); static int apic_get_arb_pri(APICCommonState *s); /* Find first bit starting from msb */ @@ -326,7 +327,7 @@ void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode, trace_apic_deliver_irq(dest, dest_mode, delivery_mode, vector_num, trigger_mode, msi_redir_hint); - apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode); + apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode, msi_redir_hint); apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, trigger_mode); } @@ -503,7 +504,8 @@ static int apic_find_dest(uint8_t dest) } static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, - uint8_t dest, uint8_t dest_mode) + uint8_t dest, uint8_t dest_mode, + uint8_t msi_redir_hint) { APICCommonState *apic_iter; int i; @@ -519,23 +521,27 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, } } else { /* XXX: cluster mode */ + int l = -1; memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t)); for(i = 0; i < MAX_APICS; i++) { apic_iter = local_apics[i]; - if (apic_iter) { - if (apic_iter->dest_mode == 0xf) { - if (dest & apic_iter->log_dest) - apic_set_bit(deliver_bitmask, i); - } else if (apic_iter->dest_mode == 0x0) { - if ((dest & 0xf0) == (apic_iter->log_dest & 0xf0) && - (dest & apic_iter->log_dest & 0x0f)) { - apic_set_bit(deliver_bitmask, i); + if (!apic_iter) { + break; + } + if (apic_match_dest(apic_iter, dest_mode, dest)) { + if (msi_redir_hint) { + if (l < 0 || + apic_compare_prio(apic_iter, local_apics[l]) < 0) { + l = i; } + } else { + apic_set_bit(deliver_bitmask, i); } - } else { - break; } } + if (msi_redir_hint && l >= 0) { + apic_set_bit(deliver_bitmask, l); + } } } @@ -568,7 +574,7 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode, switch (dest_shorthand) { case 0: - apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode); + apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode, 0); break; case 1: memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
Added argument to apic_get_delivery_bitmask() for msi_redir_hint, and changed calls to the function accordingly (using 0 as a default value for non-MSI interrupts). Modified the implementation of apic_get_delivery_bitmask() to account for the RH bit of an MSI IRQ. The RH bit indicates that the message should target only the lowest-priority processor among those specified by the logical destination of the IRQ. Signed-off-by: James Sullivan <sullivan.james.f@gmail.com> --- hw/intc/apic.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)