Re: [PATCH v2] KVM: x86: some apic broadcast modes does not work
Il 06/10/2014 21:08, Radim Krčmář ha scritto: > 2014-10-06 18:29+0300, Nadav Amit: >> On Oct 3, 2014, at 3:49 PM, Radim Krčmář wrote: >>> 2014-10-03 00:30+0300, Nadav Amit: >>> Reviewed-by: Radim Krčmář >>> +#define X2APIC_BROADCAST 0xul >>> >>> (int is better -- using long introduces an interesting feature with >>> implicit retyping: (int)X2APIC_BROADCAST != X2APIC_BROADCAST, and we >>> don't compile with -Wtype-limits to notice it. It poses no problem >>> now, so I can change it in an inevitable cleanup series / convince lkml >>> to endorse stricter warnings.) >> Would unsigned int ease your mind? > > That would be perfect. > -int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest) +static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest) >>> >>> (bool is better.) >> Ok. I will do it for v3. > > I'm fine with v2, which is why the nitpicking was in parentheses. I will fix up v2 with 0xu and static bool kvm_apic_broadcast. 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 v2] KVM: x86: some apic broadcast modes does not work
On Fri, Oct 03, 2014 at 12:30:52AM +0300, Nadav Amit wrote:
>KVM does not deliver x2APIC broadcast messages with physical mode. Intel SDM
>(10.12.9 ICR Operation in x2APIC Mode) states: "A destination ID value of
>_H is used for broadcast of interrupts in both logical destination and
>physical destination modes."
>
>In addition, the local-apic enables cluster mode broadcast. As Intel SDM
>10.6.2.2 says: "Broadcast to all local APICs is achieved by setting all
>destination bits to one." This patch enables cluster mode broadcast.
>
>The fix tries to combine broadcast in different modes through a unified code.
>
>One rare case occurs when the source of IPI has its APIC disabled. In such
>case, the source can still issue IPIs, but since the source is not obliged to
>have the same LAPIC mode as the enabled ones, we cannot rely on it.
>Since it is a rare case, it is unoptimized and done on the slow-path.
>
>---
>
>Changes v1->v2:
>- Follow Radim's review: setting constants, preferring simplicity to marginal
> performance gain, etc.
>- Combine the cluster mode and x2apic mode patches
>
>Signed-off-by: Nadav Amit
>---
Reviewed-by: Wanpeng Li
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/lapic.c| 28 ++--
> arch/x86/kvm/lapic.h| 4 ++--
> virt/kvm/ioapic.h | 2 +-
> 4 files changed, 26 insertions(+), 10 deletions(-)
>
>diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>index 7d603a7..6f2be83 100644
>--- a/arch/x86/include/asm/kvm_host.h
>+++ b/arch/x86/include/asm/kvm_host.h
>@@ -542,7 +542,7 @@ struct kvm_apic_map {
> struct rcu_head rcu;
> u8 ldr_bits;
> /* fields bellow are used to decode ldr values in different modes */
>- u32 cid_shift, cid_mask, lid_mask;
>+ u32 cid_shift, cid_mask, lid_mask, broadcast;
> struct kvm_lapic *phys_map[256];
> /* first index is cluster id second is cpu id in a cluster */
> struct kvm_lapic *logical_map[16][16];
>diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>index b8345dd..ee04adf 100644
>--- a/arch/x86/kvm/lapic.c
>+++ b/arch/x86/kvm/lapic.c
>@@ -68,6 +68,9 @@
> #define MAX_APIC_VECTOR 256
> #define APIC_VECTORS_PER_REG 32
>
>+#define APIC_BROADCAST0xFF
>+#define X2APIC_BROADCAST 0xul
>+
> #define VEC_POS(v) ((v) & (32 - 1))
> #define REG_POS(v) (((v) >> 5) << 4)
>
>@@ -149,6 +152,7 @@ static void recalculate_apic_map(struct kvm *kvm)
> new->cid_shift = 8;
> new->cid_mask = 0;
> new->lid_mask = 0xff;
>+ new->broadcast = APIC_BROADCAST;
>
> kvm_for_each_vcpu(i, vcpu, kvm) {
> struct kvm_lapic *apic = vcpu->arch.apic;
>@@ -170,6 +174,7 @@ static void recalculate_apic_map(struct kvm *kvm)
> new->cid_shift = 16;
> new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
> new->lid_mask = 0x;
>+ new->broadcast = X2APIC_BROADCAST;
> } else if (kvm_apic_sw_enabled(apic) &&
> !new->cid_mask /* flat mode */ &&
> kvm_apic_get_reg(apic, APIC_DFR) ==
> APIC_DFR_CLUSTER) {
>@@ -558,16 +563,25 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
> apic_update_ppr(apic);
> }
>
>-int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest)
>+static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
>+{
>+ return dest == (apic_x2apic_mode(apic) ?
>+ X2APIC_BROADCAST : APIC_BROADCAST);
>+}
>+
>+int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
> {
>- return dest == 0xff || kvm_apic_id(apic) == dest;
>+ return kvm_apic_id(apic) == dest || kvm_apic_broadcast(apic, dest);
> }
>
>-int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
>+int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
> {
> int result = 0;
> u32 logical_id;
>
>+ if (kvm_apic_broadcast(apic, mda))
>+ return 1;
>+
> if (apic_x2apic_mode(apic)) {
> logical_id = kvm_apic_get_reg(apic, APIC_LDR);
> return logical_id & mda;
>@@ -595,7 +609,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8
>mda)
> }
>
> int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>- int short_hand, int dest, int dest_mode)
>+ int short_hand, unsigned int dest, int dest_mode)
> {
> int result = 0;
> struct kvm_lapic *target = vcpu->arch.apic;
>@@ -657,9 +671,11 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm,
>struct kvm_lapic *src,
> if (!map)
> goto out;
>
>+ if (irq->dest_id == map->broadcast)
>+ goto out;
>+
> if (irq->dest_mode == 0) { /* physical mode */
>- if (irq->delivery_mode == APIC_DM_
Re: [PATCH v2] KVM: x86: some apic broadcast modes does not work
2014-10-06 18:29+0300, Nadav Amit: > On Oct 3, 2014, at 3:49 PM, Radim Krčmář wrote: > > 2014-10-03 00:30+0300, Nadav Amit: > > Reviewed-by: Radim Krčmář > > > >> +#define X2APIC_BROADCAST 0xul > > > > (int is better -- using long introduces an interesting feature with > > implicit retyping: (int)X2APIC_BROADCAST != X2APIC_BROADCAST, and we > > don't compile with -Wtype-limits to notice it. It poses no problem > > now, so I can change it in an inevitable cleanup series / convince lkml > > to endorse stricter warnings.) > Would unsigned int ease your mind? That would be perfect. > >> -int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest) > >> +static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest) > > > > (bool is better.) > Ok. I will do it for v3. I'm fine with v2, which is why the nitpicking was in parentheses. -- 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 v2] KVM: x86: some apic broadcast modes does not work
On Oct 3, 2014, at 3:49 PM, Radim Krčmář wrote: > 2014-10-03 00:30+0300, Nadav Amit: >> KVM does not deliver x2APIC broadcast messages with physical mode. Intel SDM >> (10.12.9 ICR Operation in x2APIC Mode) states: "A destination ID value of >> _H is used for broadcast of interrupts in both logical destination >> and >> physical destination modes." >> >> In addition, the local-apic enables cluster mode broadcast. As Intel SDM >> 10.6.2.2 says: "Broadcast to all local APICs is achieved by setting all >> destination bits to one." This patch enables cluster mode broadcast. >> >> The fix tries to combine broadcast in different modes through a unified code. >> >> One rare case occurs when the source of IPI has its APIC disabled. In such >> case, the source can still issue IPIs, but since the source is not obliged to >> have the same LAPIC mode as the enabled ones, we cannot rely on it. >> Since it is a rare case, it is unoptimized and done on the slow-path. >> >> --- > > Thanks! > > Reviewed-by: Radim Krčmář > >> Changes v1->v2: >> - Follow Radim's review: setting constants, preferring simplicity to marginal >> performance gain, etc. >> - Combine the cluster mode and x2apic mode patches >> >> Signed-off-by: Nadav Amit >> --- >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index b8345dd..ee04adf 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -68,6 +68,9 @@ >> #define MAX_APIC_VECTOR 256 >> #define APIC_VECTORS_PER_REG 32 >> >> +#define APIC_BROADCAST 0xFF >> +#define X2APIC_BROADCAST0xul > > (int is better -- using long introduces an interesting feature with > implicit retyping: (int)X2APIC_BROADCAST != X2APIC_BROADCAST, and we > don't compile with -Wtype-limits to notice it. It poses no problem > now, so I can change it in an inevitable cleanup series / convince lkml > to endorse stricter warnings.) Would unsigned int ease your mind? > >> + >> #define VEC_POS(v) ((v) & (32 - 1)) >> #define REG_POS(v) (((v) >> 5) << 4) >> >> @@ -558,16 +563,25 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 >> tpr) >> apic_update_ppr(apic); >> } >> >> -int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest) >> +static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest) > > (bool is better.) Ok. I will do it for v3. Nadav signature.asc Description: Message signed with OpenPGP using GPGMail
Re: [PATCH v2] KVM: x86: some apic broadcast modes does not work
2014-10-03 00:30+0300, Nadav Amit:
> KVM does not deliver x2APIC broadcast messages with physical mode. Intel SDM
> (10.12.9 ICR Operation in x2APIC Mode) states: "A destination ID value of
> _H is used for broadcast of interrupts in both logical destination and
> physical destination modes."
>
> In addition, the local-apic enables cluster mode broadcast. As Intel SDM
> 10.6.2.2 says: "Broadcast to all local APICs is achieved by setting all
> destination bits to one." This patch enables cluster mode broadcast.
>
> The fix tries to combine broadcast in different modes through a unified code.
>
> One rare case occurs when the source of IPI has its APIC disabled. In such
> case, the source can still issue IPIs, but since the source is not obliged to
> have the same LAPIC mode as the enabled ones, we cannot rely on it.
> Since it is a rare case, it is unoptimized and done on the slow-path.
>
> ---
Thanks!
Reviewed-by: Radim Krčmář
> Changes v1->v2:
> - Follow Radim's review: setting constants, preferring simplicity to marginal
> performance gain, etc.
> - Combine the cluster mode and x2apic mode patches
>
> Signed-off-by: Nadav Amit
> ---
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index b8345dd..ee04adf 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -68,6 +68,9 @@
> #define MAX_APIC_VECTOR 256
> #define APIC_VECTORS_PER_REG 32
>
> +#define APIC_BROADCAST 0xFF
> +#define X2APIC_BROADCAST 0xul
(int is better -- using long introduces an interesting feature with
implicit retyping: (int)X2APIC_BROADCAST != X2APIC_BROADCAST, and we
don't compile with -Wtype-limits to notice it. It poses no problem
now, so I can change it in an inevitable cleanup series / convince lkml
to endorse stricter warnings.)
> +
> #define VEC_POS(v) ((v) & (32 - 1))
> #define REG_POS(v) (((v) >> 5) << 4)
>
> @@ -558,16 +563,25 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32
> tpr)
> apic_update_ppr(apic);
> }
>
> -int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest)
> +static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
(bool is better.)
> +{
> + return dest == (apic_x2apic_mode(apic) ?
> + X2APIC_BROADCAST : APIC_BROADCAST);
> +}
--
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
