* [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug
@ 2012-07-10 20:22 Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [PATCH 1/7] cpus.h: include cpu-common.h Eduardo Habkost
` (8 more replies)
0 siblings, 9 replies; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
Hi,
This is more a call for discussion than a request for comments in the actual
code.
Our problem today is:
- Lots of QEMU and Seabios code have the assumption that Initial
APIC ID == CPU "index" (in other words, that APIC IDs are always contiguous
and start at 0);
- However, the Initial APIC IDs may be non-contiguous depending on the
requested CPU topology (core/thread counts), so we have to break that
assumption (see [1]).
This series is how a fix could look like if we had the following assumptions:
- Seabios can't run any code in hotplugged CPUs;
- We don't change the current CPU hotplug interface between the Seabios SSDT
code and QEMU (the interface used by method PRSC at acpi-dsdt.dsl in Seabios)
Note that I am more than willing to break any of the assumptions above, and get
rid of FW_CFG_LAPIC_INFO. But I don't know what's the best way to do it.
For the non-hotplug CPUs, it would be quite easy: just make Seabios get the
initial APIC ID from the AP processors on boot, at smp.c:smp_ap_boot_code.
The hotplug case is a bit more complex: we need to either:
- have a mechanism to let the ACPI SSDT code know what's the APIC ID of
hotplugged CPUs; or
- make Seabios run some code in the hotplugged CPU (I am assuming that this is
simply not possible).
I am hoping people have suggestions to solve this. I don't know where the
interface used by acpi-dsdt.dsl:PRSC comes from, and if it can be easily
extended or changed.
All that said, I have one question: is it acceptable to apply a fix for the
APIC- ID/topology bug that knowingly breaks CPU hotplug, by now? (not exactly
the fix in this series, but a fix that simply makes Seabios query the APIC IDs
directly from the CPUs).
Note that this series is incomplete and not completely tested. Know issues:
- It breaks CPU hotplug because the SSDT code in Seabios is still incorrect;
- It breaks live-migration because the initial APIC ID will be different
even for older machine-types (and the initial APIC ID is not part of the
CPU state sent during migration);
- The MPTABLE code in Seabios may also need changes to use the right APIC IDs
(I am not sure yet).
[1] http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 1/7] cpus.h: include cpu-common.h
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h Eduardo Habkost
` (7 subsequent siblings)
8 siblings, 0 replies; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
Needed for the definition of fprint_function.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
cpus.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/cpus.h b/cpus.h
index 81bd817..061ff7f 100644
--- a/cpus.h
+++ b/cpus.h
@@ -1,6 +1,8 @@
#ifndef QEMU_CPUS_H
#define QEMU_CPUS_H
+#include "qemu-common.h"
+
/* cpus.c */
void qemu_init_cpu_loop(void);
void resume_all_vcpus(void);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [PATCH 1/7] cpus.h: include cpu-common.h Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-12 19:24 ` Blue Swirl
2012-07-10 20:22 ` [Qemu-devel] [QEMU PATCH 3/7] kvm: set vcpu_id to APIC ID instead of CPU index Eduardo Habkost
` (6 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/apic.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/hw/apic.c b/hw/apic.c
index 60552df..d322fe3 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -50,7 +50,7 @@ static int ffs_bit(uint32_t value)
return ctz32(value);
}
-static inline void set_bit(uint32_t *tab, int index)
+static inline void apic_set_bit(uint32_t *tab, int index)
{
int i, mask;
i = index >> 5;
@@ -58,7 +58,7 @@ static inline void set_bit(uint32_t *tab, int index)
tab[i] |= mask;
}
-static inline void reset_bit(uint32_t *tab, int index)
+static inline void apic_reset_bit(uint32_t *tab, int index)
{
int i, mask;
i = index >> 5;
@@ -66,7 +66,7 @@ static inline void reset_bit(uint32_t *tab, int index)
tab[i] &= ~mask;
}
-static inline int get_bit(uint32_t *tab, int index)
+static inline int apic_get_bit(uint32_t *tab, int index)
{
int i, mask;
i = index >> 5;
@@ -183,7 +183,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
case APIC_DM_FIXED:
if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
break;
- reset_bit(s->irr, lvt & 0xff);
+ apic_reset_bit(s->irr, lvt & 0xff);
/* fall through */
case APIC_DM_EXTINT:
cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
@@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
{
- apic_report_irq_delivered(!get_bit(s->irr, vector_num));
+ apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
- set_bit(s->irr, vector_num);
+ apic_set_bit(s->irr, vector_num);
if (trigger_mode)
- set_bit(s->tmr, vector_num);
+ apic_set_bit(s->tmr, vector_num);
else
- reset_bit(s->tmr, vector_num);
+ apic_reset_bit(s->tmr, vector_num);
if (s->vapic_paddr) {
apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
/*
@@ -405,8 +405,8 @@ static void apic_eoi(APICCommonState *s)
isrv = get_highest_priority_int(s->isr);
if (isrv < 0)
return;
- reset_bit(s->isr, isrv);
- if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
+ apic_reset_bit(s->isr, isrv);
+ if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && apic_get_bit(s->tmr, isrv)) {
ioapic_eoi_broadcast(isrv);
}
apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
@@ -445,7 +445,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
int idx = apic_find_dest(dest);
memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
if (idx >= 0)
- set_bit(deliver_bitmask, idx);
+ apic_set_bit(deliver_bitmask, idx);
}
} else {
/* XXX: cluster mode */
@@ -455,11 +455,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
if (apic_iter) {
if (apic_iter->dest_mode == 0xf) {
if (dest & apic_iter->log_dest)
- set_bit(deliver_bitmask, i);
+ 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)) {
- set_bit(deliver_bitmask, i);
+ apic_set_bit(deliver_bitmask, i);
}
}
} else {
@@ -502,14 +502,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
break;
case 1:
memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
- set_bit(deliver_bitmask, s->idx);
+ apic_set_bit(deliver_bitmask, s->idx);
break;
case 2:
memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
break;
case 3:
memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
- reset_bit(deliver_bitmask, s->idx);
+ apic_reset_bit(deliver_bitmask, s->idx);
break;
}
@@ -557,8 +557,8 @@ int apic_get_interrupt(DeviceState *d)
apic_sync_vapic(s, SYNC_TO_VAPIC);
return s->spurious_vec & 0xff;
}
- reset_bit(s->irr, intno);
- set_bit(s->isr, intno);
+ apic_reset_bit(s->irr, intno);
+ apic_set_bit(s->isr, intno);
apic_sync_vapic(s, SYNC_TO_VAPIC);
apic_update_irq(s);
return intno;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [QEMU PATCH 3/7] kvm: set vcpu_id to APIC ID instead of CPU index
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [PATCH 1/7] cpus.h: include cpu-common.h Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 4/7] i386: create apic_id_for_cpu() function Eduardo Habkost
` (5 subsequent siblings)
8 siblings, 0 replies; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
The CPU ID in KVM is supposed to be the APIC ID, so change the
KVM_CREATE_VCPU call to match it. It didn't break anything yet because
today the APIC ID is assumed to be == the CPU index, but this won't be
true in the future.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
kvm-all.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kvm-all.c b/kvm-all.c
index f8e4328..2d1fe6a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -212,7 +212,7 @@ int kvm_init_vcpu(CPUArchState *env)
DPRINTF("kvm_init_vcpu\n");
- ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
+ ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpuid_apic_id);
if (ret < 0) {
DPRINTF("kvm_create_vcpu failed\n");
goto err;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH 4/7] i386: create apic_id_for_cpu() function
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
` (2 preceding siblings ...)
2012-07-10 20:22 ` [Qemu-devel] [QEMU PATCH 3/7] kvm: set vcpu_id to APIC ID instead of CPU index Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it Eduardo Habkost
` (4 subsequent siblings)
8 siblings, 0 replies; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
Currently we need a way to calculate the Initial APIC ID using only the
CPU index, as the CPU hotplug interface doesn't have a method to send
the correct APIC ID to the ACPI hotplug code at hotplug-time.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
target-i386/cpu.c | 2 +-
target-i386/cpu.h | 17 +++++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b257241..c54619a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1770,7 +1770,7 @@ static void x86_cpu_initfn(Object *obj)
x86_cpuid_get_tsc_freq,
x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
- env->cpuid_apic_id = env->cpu_index;
+ env->cpuid_apic_id = apic_id_for_cpu(env->cpu_index);
/* init various static tables used in TCG mode */
if (tcg_enabled() && !inited) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 6924c7f..e983c10 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -908,6 +908,23 @@ void cpu_clear_apic_feature(CPUX86State *env);
void host_cpuid(uint32_t function, uint32_t count,
uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
+
+/* Calculate initial APIC ID for a specific CPU index
+ *
+ * Currently we need to be able to calculate the APIC ID from the CPU index
+ * only, as the QEMU<->Seabios(ACPI) interface for CPU hotplug isn't able
+ * to let the ACPI CPU hotplug code to retrivew the APIC ID at hotplug
+ * time.
+ */
+static inline uint8_t apic_id_for_cpu(int cpu_index)
+{
+ /* right now APIC ID == CPU index. this will eventually change to use
+ * the CPU topology configuration properly
+ */
+ return cpu_index;
+}
+
+
/* helper.c */
int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
int is_write, int mmu_idx);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
` (3 preceding siblings ...)
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 4/7] i386: create apic_id_for_cpu() function Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-12 19:29 ` Blue Swirl
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions Eduardo Habkost
` (3 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/pc.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/hw/pc.c b/hw/pc.c
index 3b8e469..dc95fb8 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -71,6 +71,7 @@
#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
+#define FW_CFG_LAPIC_INFO (FW_CFG_ARCH_LOCAL + 5)
#define MSI_ADDR_BASE 0xfee00000
@@ -87,6 +88,18 @@ struct e820_table {
struct e820_entry entry[E820_NR_ENTRIES];
} QEMU_PACKED __attribute((__aligned__(4)));
+struct lapic_info_entry {
+ uint8_t apic_id;
+ uint8_t reserved1;
+ uint16_t reserved2;
+ uint32_t reserved3;
+} QEMU_PACKED;
+
+struct lapic_info_table {
+ uint64_t count;
+ struct lapic_info_entry entries[];
+} QEMU_PACKED;
+
static struct e820_table e820_table;
struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
@@ -652,6 +665,16 @@ static void *bochs_bios_init(void)
fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, (uint8_t *)numa_fw_cfg,
(1 + max_cpus + nb_numa_nodes) * 8);
+ size_t lapic_info_size = sizeof(struct lapic_info_table) + \
+ sizeof(struct lapic_info_entry) * max_cpus;
+ struct lapic_info_table *lapic_table = g_malloc0(lapic_info_size);;
+ lapic_table->count = max_cpus;
+ for (i = 0; i < max_cpus; i++) {
+ lapic_table->entries[i].apic_id = apic_id_for_cpu(i);
+ }
+ fw_cfg_add_bytes(fw_cfg, FW_CFG_LAPIC_INFO, (uint8_t *)lapic_table,
+ lapic_info_size);
+
return fw_cfg;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
` (4 preceding siblings ...)
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-12 19:37 ` Blue Swirl
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 7/7] generate APIC IDs according to CPU topology Eduardo Habkost
` (2 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
target-i386/topology.h | 138 ++++++++++++++++++++++++++++++++++++++++++++++++
tests/.gitignore | 1 +
tests/Makefile | 7 ++-
tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++
4 files changed, 253 insertions(+), 1 deletion(-)
create mode 100644 target-i386/topology.h
create mode 100644 tests/test-x86-cpuid.c
diff --git a/target-i386/topology.h b/target-i386/topology.h
new file mode 100644
index 0000000..de0f17a
--- /dev/null
+++ b/target-i386/topology.h
@@ -0,0 +1,138 @@
+/*
+ * x86 CPU topology data structures and functions
+ *
+ * Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef __QEMU_X86_TOPOLOGY_H__
+#define __QEMU_X86_TOPOLOGY_H__
+
+#include <stdint.h>
+#include <string.h>
+
+#include "bitops.h"
+
+/* Return the bit width needed for 'count' IDs
+ */
+static unsigned bits_for_count(unsigned count)
+{
+ g_assert(count >= 1);
+ if (count == 1) {
+ return 0;
+ }
+ return bitops_flsl(count - 1) + 1;
+}
+
+/* Bit width of the SMT_ID (thread ID) field on the APIC ID
+ */
+static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned nr_threads)
+{
+ return bits_for_count(nr_threads);
+}
+
+/* Bit width of the Core_ID field
+ */
+static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
+{
+ return bits_for_count(nr_cores);
+}
+
+/* Bit offset of the Core_ID field
+ */
+static inline unsigned apicid_core_offset(unsigned nr_cores,
+ unsigned nr_threads)
+{
+ return apicid_smt_width(nr_cores, nr_threads);
+}
+
+/* Bit offset of the Pkg_ID (socket ID) field
+ */
+static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
+{
+ return apicid_core_offset(nr_cores, nr_threads) + \
+ apicid_core_width(nr_cores, nr_threads);
+}
+
+/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
+ *
+ * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
+ */
+static inline uint8_t __make_apicid(unsigned nr_cores, unsigned nr_threads,
+ unsigned pkg_id, unsigned core_id,
+ unsigned smt_id)
+{
+ return (pkg_id << apicid_pkg_offset(nr_cores, nr_threads)) | \
+ (core_id << apicid_core_offset(nr_cores, nr_threads)) | \
+ smt_id;
+}
+
+/* Calculate thread/core/package IDs for a specific topology,
+ * based on (continguous) CPU index
+ */
+static inline void topo_ids_from_idx(unsigned nr_cores, unsigned nr_threads,
+ unsigned cpu_index,
+ unsigned *pkg_id, unsigned *core_id,
+ unsigned *smt_id)
+{
+ unsigned core_index = cpu_index / nr_threads;
+ *smt_id = cpu_index % nr_threads;
+ *core_id = core_index % nr_cores;
+ *pkg_id = core_index / nr_cores;
+}
+
+/* Get package ID from an APIC ID
+ */
+static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads,
+ uint8_t apic_id)
+{
+ return apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
+}
+
+/* Get core ID from an APIC ID
+ */
+static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads,
+ uint8_t apic_id)
+{
+ return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \
+ ((1 << apicid_core_width(nr_cores, nr_threads)) - 1);
+}
+
+/* Get SMT (thread) ID from an APIC ID
+ */
+static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads,
+ uint8_t apic_id)
+{
+ return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1);
+}
+
+/* Make APIC ID for the CPU 'cpu_index'
+ *
+ * 'cpu_index' is a sequential, contiguous ID for the CPU.
+ */
+static inline uint8_t topo_make_apicid(unsigned nr_cores, unsigned nr_threads,
+ unsigned cpu_index)
+{
+ unsigned pkg_id, core_id, smt_id;
+ topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
+ &pkg_id, &core_id, &smt_id);
+ return __make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
+}
+
+#endif /* __QMU_X86_TOPOLOGY_H__ */
diff --git a/tests/.gitignore b/tests/.gitignore
index f9041f3..38c94ef 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -10,4 +10,5 @@ test-qmp-commands.h
test-qmp-commands
test-qmp-input-strict
test-qmp-marshal.c
+test-x86-cpuid
*-test
diff --git a/tests/Makefile b/tests/Makefile
index b605e14..89bd890 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
check-unit-y += tests/test-coroutine$(EXESUF)
check-unit-y += tests/test-visitor-serialization$(EXESUF)
check-unit-y += tests/test-iov$(EXESUF)
+check-unit-y += tests/test-x86-cpuid$(EXESUF)
check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
@@ -33,7 +34,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
tests/test-coroutine.o tests/test-string-output-visitor.o \
tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
- tests/test-qmp-commands.o tests/test-visitor-serialization.o
+ tests/test-qmp-commands.o tests/test-visitor-serialization.o \
+ tests/test-x86-cpuid.o
test-qapi-obj-y = $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -41,6 +43,8 @@ test-qapi-obj-y += module.o
$(test-obj-y): QEMU_INCLUDES += -Itests
+tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
+
tests/check-qint$(EXESUF): tests/check-qint.o qint.o $(tools-obj-y)
tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o $(tools-obj-y)
tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o $(tools-obj-y)
@@ -49,6 +53,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
tests/test-iov$(EXESUF): tests/test-iov.o iov.o
+tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
tests/test-qapi-types.c tests/test-qapi-types.h :\
$(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
new file mode 100644
index 0000000..b27ed62
--- /dev/null
+++ b/tests/test-x86-cpuid.c
@@ -0,0 +1,108 @@
+/*
+ * Test code for x86 CPUID and Topology functions
+ *
+ * Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <glib.h>
+
+/*FIXME: this should be built inside the i386 target directory instead */
+#include "topology.h"
+
+static void test_topo_bits(void)
+{
+ /* simple tests for 1 thread per core, 1 core per socket */
+ g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
+ g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
+
+ g_assert_cmpuint(topo_make_apicid(1, 1, 0), ==, 0);
+ g_assert_cmpuint(topo_make_apicid(1, 1, 1), ==, 1);
+ g_assert_cmpuint(topo_make_apicid(1, 1, 2), ==, 2);
+ g_assert_cmpuint(topo_make_apicid(1, 1, 3), ==, 3);
+
+
+ /* Test field width calculation for multiple values
+ */
+ g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1);
+ g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2);
+ g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2);
+
+ g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4);
+ g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4);
+ g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4);
+ g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5);
+
+
+ g_assert_cmpuint(apicid_core_width(30, 2), ==, 5);
+ g_assert_cmpuint(apicid_core_width(31, 2), ==, 5);
+ g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
+ g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
+
+
+ /* build a weird topology and see if IDs are calculated correctly
+ */
+
+ /* This will use 2 bits for thread ID and 3 bits for core ID
+ */
+ g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2);
+ g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
+ g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
+
+ g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2);
+
+ g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2);
+
+ g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2);
+
+ g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1);
+ g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2);
+
+ g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5));
+ g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==,
+ (1<<5) | (1<<2) | 1);
+
+ g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==,
+ (3<<5) | (5<<2) | 2);
+
+
+ /* Check the APIC ID -> {pkg,core,thread} ID functions */
+ g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3);
+ g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5);
+ g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2);
+}
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+
+ g_test_add_func("/cpuid/topology/basic", test_topo_bits);
+
+ g_test_run();
+
+ return 0;
+}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH 7/7] generate APIC IDs according to CPU topology
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
` (5 preceding siblings ...)
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [Seabios RFC PATCH 1/1] get lapic IDs from fw_cfg Eduardo Habkost
2012-07-12 13:51 ` [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Igor Mammedov
8 siblings, 0 replies; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
NOTE: this is unfinished, and will break live-migration between older
and newer QEMU versions if applied as-is. We need to make the older
machine-types keep the existing behavior when generating APIC IDs.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
target-i386/cpu.h | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e983c10..860aeaa 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -21,6 +21,8 @@
#include "config.h"
#include "qemu-common.h"
+#include "topology.h"
+#include "cpus.h"
#ifdef TARGET_X86_64
#define TARGET_LONG_BITS 64
@@ -918,10 +920,7 @@ void host_cpuid(uint32_t function, uint32_t count,
*/
static inline uint8_t apic_id_for_cpu(int cpu_index)
{
- /* right now APIC ID == CPU index. this will eventually change to use
- * the CPU topology configuration properly
- */
- return cpu_index;
+ return topo_make_apicid(smp_cores, smp_threads, cpu_index);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [Seabios RFC PATCH 1/1] get lapic IDs from fw_cfg
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
` (6 preceding siblings ...)
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 7/7] generate APIC IDs according to CPU topology Eduardo Habkost
@ 2012-07-10 20:22 ` Eduardo Habkost
2012-07-12 13:51 ` [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Igor Mammedov
8 siblings, 0 replies; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-10 20:22 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, seabios, Gleb Natapov, Anthony Liguori
This changes:
- MADT table generation
- SRAT table generation
Still missing:
- The _MAT method on SSDT Processor entries still return the wrong APIC
ID (breaking CPU hotplug)
- The MPTable generation code doesn't take the new APIC IDs into
account
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
src/acpi.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++--------
src/paravirt.c | 12 +++++++++++
src/paravirt.h | 13 ++++++++++++
3 files changed, 80 insertions(+), 9 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c
index 8549304..f9eafad 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -311,21 +311,40 @@ build_madt(void)
+ sizeof(struct madt_intsrcovr) * 16
+ sizeof(struct madt_local_nmi));
- struct multiple_apic_table *madt = malloc_high(madt_size);
+ struct fw_cfg_lapic_info_entry *lapics = NULL;
+ struct multiple_apic_table *madt = NULL;
+ u64 lapic_count = qemu_cfg_get_lapic_count();
+
+ if (lapic_count) {
+ lapics = malloc_tmphigh(sizeof(*lapics) * lapic_count);
+ if (!lapics) {
+ warn_noalloc();
+ goto out;
+ }
+ }
+
+ qemu_cfg_get_lapic_info(lapics, lapic_count);
+
+ madt = malloc_high(madt_size);
if (!madt) {
warn_noalloc();
- return NULL;
+ goto out;
}
+
memset(madt, 0, madt_size);
madt->local_apic_address = cpu_to_le32(BUILD_APIC_ADDR);
madt->flags = cpu_to_le32(1);
struct madt_processor_apic *apic = (void*)&madt[1];
int i;
+ int last_apic_id = -1;
for (i=0; i<MaxCountCPUs; i++) {
apic->type = APIC_PROCESSOR;
apic->length = sizeof(*apic);
apic->processor_id = i;
- apic->local_apic_id = i;
+ if (i < lapic_count)
+ last_apic_id = apic->local_apic_id = lapics[i].apic_id;
+ else
+ apic->local_apic_id = ++last_apic_id;
if (i < CountCPUs)
apic->flags = cpu_to_le32(1);
else
@@ -371,6 +390,10 @@ build_madt(void)
local_nmi++;
build_header((void*)madt, APIC_SIGNATURE, (void*)local_nmi - (void*)madt, 1);
+
+out:
+ if (lapics)
+ free(lapics);
return madt;
}
@@ -616,20 +639,35 @@ acpi_build_srat_memory(struct srat_memory_affinity *numamem,
static void *
build_srat(void)
{
+ struct fw_cfg_lapic_info_entry *lapics = NULL;
+ u64 lapic_count = 0;
+ struct system_resource_affinity_table *srat = NULL;
+ u64 *numadata = NULL;
int nb_numa_nodes = qemu_cfg_get_numa_nodes();
if (nb_numa_nodes == 0)
- return NULL;
+ goto out;
- u64 *numadata = malloc_tmphigh(sizeof(u64) * (MaxCountCPUs + nb_numa_nodes));
+ numadata = malloc_tmphigh(sizeof(u64) * (MaxCountCPUs + nb_numa_nodes));
if (!numadata) {
warn_noalloc();
- return NULL;
+ goto out;
}
qemu_cfg_get_numa_data(numadata, MaxCountCPUs + nb_numa_nodes);
- struct system_resource_affinity_table *srat;
+ lapic_count = qemu_cfg_get_lapic_count();
+
+ if (lapic_count) {
+ lapics = malloc_tmphigh(sizeof(*lapics) * lapic_count);
+ if (!lapics) {
+ warn_noalloc();
+ goto out;
+ }
+ }
+
+ qemu_cfg_get_lapic_info(lapics, lapic_count);
+
int srat_size = sizeof(*srat) +
sizeof(struct srat_processor_affinity) * MaxCountCPUs +
sizeof(struct srat_memory_affinity) * (nb_numa_nodes + 2);
@@ -646,11 +684,15 @@ build_srat(void)
struct srat_processor_affinity *core = (void*)(srat + 1);
int i;
u64 curnode;
+ int last_apic_id = -1;
for (i = 0; i < MaxCountCPUs; ++i) {
core->type = SRAT_PROCESSOR;
core->length = sizeof(*core);
- core->local_apic_id = i;
+ if (i < lapic_count)
+ last_apic_id = core->local_apic_id = lapics[i].apic_id;
+ else
+ core->local_apic_id = ++last_apic_id;
curnode = *numadata++;
core->proximity_lo = curnode;
memset(core->proximity_hi, 0, 3);
@@ -704,7 +746,11 @@ build_srat(void)
build_header((void*)srat, SRAT_SIGNATURE, srat_size, 1);
- free(numadata);
+out:
+ if (lapics)
+ free(lapics);
+ if (numadata)
+ free(numadata);
return srat;
}
diff --git a/src/paravirt.c b/src/paravirt.c
index 2a98d53..674d512 100644
--- a/src/paravirt.c
+++ b/src/paravirt.c
@@ -293,6 +293,18 @@ void qemu_cfg_get_numa_data(u64 *data, int n)
qemu_cfg_read((u8*)(data + i), sizeof(u64));
}
+u64 qemu_cfg_get_lapic_count(void)
+{
+ u64 cnt;
+ qemu_cfg_read_entry(&cnt, QEMU_CFG_LAPIC_INFO, sizeof(cnt));
+ return cnt;
+}
+
+void qemu_cfg_get_lapic_info(struct fw_cfg_lapic_info_entry *entries, u64 n)
+{
+ qemu_cfg_read((u8*)entries, sizeof(struct fw_cfg_lapic_info_entry)*n);
+}
+
u16 qemu_cfg_get_max_cpus(void)
{
u16 cnt;
diff --git a/src/paravirt.h b/src/paravirt.h
index a284c41..5e3e500 100644
--- a/src/paravirt.h
+++ b/src/paravirt.h
@@ -40,6 +40,8 @@ static inline int kvm_para_available(void)
#define QEMU_CFG_SMBIOS_ENTRIES (QEMU_CFG_ARCH_LOCAL + 1)
#define QEMU_CFG_IRQ0_OVERRIDE (QEMU_CFG_ARCH_LOCAL + 2)
#define QEMU_CFG_E820_TABLE (QEMU_CFG_ARCH_LOCAL + 3)
+#define QEMU_CFG_LAPIC_INFO (QEMU_CFG_ARCH_LOCAL + 5)
+
extern int qemu_cfg_present;
@@ -57,6 +59,17 @@ int qemu_cfg_smbios_load_external(int type, char **p, unsigned *nr_structs,
int qemu_cfg_get_numa_nodes(void);
void qemu_cfg_get_numa_data(u64 *data, int n);
u16 qemu_cfg_get_max_cpus(void);
+
+struct fw_cfg_lapic_info_entry {
+ u8 apic_id;
+ u8 reserved1;
+ u16 reserved2;
+ u32 reserved3;
+} PACKED;
+
+u64 qemu_cfg_get_lapic_count(void);
+void qemu_cfg_get_lapic_info(struct fw_cfg_lapic_info_entry *entries, u64 n);
+
struct e820_reservation {
u64 address;
u64 length;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
` (7 preceding siblings ...)
2012-07-10 20:22 ` [Qemu-devel] [Seabios RFC PATCH 1/1] get lapic IDs from fw_cfg Eduardo Habkost
@ 2012-07-12 13:51 ` Igor Mammedov
2012-07-12 14:00 ` Gleb Natapov
8 siblings, 1 reply; 28+ messages in thread
From: Igor Mammedov @ 2012-07-12 13:51 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Anthony Liguori, seabios, qemu-devel, Gleb Natapov
On 07/10/2012 10:22 PM, Eduardo Habkost wrote:
> The hotplug case is a bit more complex: we need to either:
> - have a mechanism to let the ACPI SSDT code know what's the APIC ID of
> hotplugged CPUs; or
> - make Seabios run some code in the hotplugged CPU (I am assuming that this is
> simply not possible).
If SMM is supported by qemu/kvm than it will be possible to trigger SMI from
APCI method and SMI handler, that are supposed to run on all CPUs (including hot-plugged one)
, could fixup APIC ID of added CPU in some memory region used by ACPI.
--
-----
Igor
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug
2012-07-12 13:51 ` [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Igor Mammedov
@ 2012-07-12 14:00 ` Gleb Natapov
0 siblings, 0 replies; 28+ messages in thread
From: Gleb Natapov @ 2012-07-12 14:00 UTC (permalink / raw)
To: Igor Mammedov; +Cc: seabios, Eduardo Habkost, Anthony Liguori, qemu-devel
On Thu, Jul 12, 2012 at 03:51:48PM +0200, Igor Mammedov wrote:
> On 07/10/2012 10:22 PM, Eduardo Habkost wrote:
> >The hotplug case is a bit more complex: we need to either:
> > - have a mechanism to let the ACPI SSDT code know what's the APIC ID of
> > hotplugged CPUs; or
> > - make Seabios run some code in the hotplugged CPU (I am assuming that this is
> > simply not possible).
> If SMM is supported by qemu/kvm than it will be possible to trigger SMI from
> APCI method and SMI handler, that are supposed to run on all CPUs (including hot-plugged one)
> , could fixup APIC ID of added CPU in some memory region used by ACPI.
>
SMM is not supported (arguably is should not be supported on real HW
too), but even if it was default SMM address is 0x38000 IIRC and it is a
middle of a memory and can be used by a guest OS.
--
Gleb.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h
2012-07-10 20:22 ` [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h Eduardo Habkost
@ 2012-07-12 19:24 ` Blue Swirl
2012-07-13 18:07 ` Eduardo Habkost
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-12 19:24 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Maybe the bitops functions should be renamed instead, for example
prefixed by 'qemu_'. That may be safer if one day the kernel find
their way to system headers too.
> ---
> hw/apic.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index 60552df..d322fe3 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -50,7 +50,7 @@ static int ffs_bit(uint32_t value)
> return ctz32(value);
> }
>
> -static inline void set_bit(uint32_t *tab, int index)
> +static inline void apic_set_bit(uint32_t *tab, int index)
> {
> int i, mask;
> i = index >> 5;
> @@ -58,7 +58,7 @@ static inline void set_bit(uint32_t *tab, int index)
> tab[i] |= mask;
> }
>
> -static inline void reset_bit(uint32_t *tab, int index)
> +static inline void apic_reset_bit(uint32_t *tab, int index)
> {
> int i, mask;
> i = index >> 5;
> @@ -66,7 +66,7 @@ static inline void reset_bit(uint32_t *tab, int index)
> tab[i] &= ~mask;
> }
>
> -static inline int get_bit(uint32_t *tab, int index)
> +static inline int apic_get_bit(uint32_t *tab, int index)
> {
> int i, mask;
> i = index >> 5;
> @@ -183,7 +183,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
> case APIC_DM_FIXED:
> if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
> break;
> - reset_bit(s->irr, lvt & 0xff);
> + apic_reset_bit(s->irr, lvt & 0xff);
> /* fall through */
> case APIC_DM_EXTINT:
> cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
> @@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
>
> static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
> {
> - apic_report_irq_delivered(!get_bit(s->irr, vector_num));
> + apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
>
> - set_bit(s->irr, vector_num);
> + apic_set_bit(s->irr, vector_num);
> if (trigger_mode)
> - set_bit(s->tmr, vector_num);
> + apic_set_bit(s->tmr, vector_num);
> else
> - reset_bit(s->tmr, vector_num);
> + apic_reset_bit(s->tmr, vector_num);
> if (s->vapic_paddr) {
> apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
> /*
> @@ -405,8 +405,8 @@ static void apic_eoi(APICCommonState *s)
> isrv = get_highest_priority_int(s->isr);
> if (isrv < 0)
> return;
> - reset_bit(s->isr, isrv);
> - if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
> + apic_reset_bit(s->isr, isrv);
> + if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && apic_get_bit(s->tmr, isrv)) {
> ioapic_eoi_broadcast(isrv);
> }
> apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
> @@ -445,7 +445,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> int idx = apic_find_dest(dest);
> memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
> if (idx >= 0)
> - set_bit(deliver_bitmask, idx);
> + apic_set_bit(deliver_bitmask, idx);
> }
> } else {
> /* XXX: cluster mode */
> @@ -455,11 +455,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> if (apic_iter) {
> if (apic_iter->dest_mode == 0xf) {
> if (dest & apic_iter->log_dest)
> - set_bit(deliver_bitmask, i);
> + 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)) {
> - set_bit(deliver_bitmask, i);
> + apic_set_bit(deliver_bitmask, i);
> }
> }
> } else {
> @@ -502,14 +502,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
> break;
> case 1:
> memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
> - set_bit(deliver_bitmask, s->idx);
> + apic_set_bit(deliver_bitmask, s->idx);
> break;
> case 2:
> memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> break;
> case 3:
> memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> - reset_bit(deliver_bitmask, s->idx);
> + apic_reset_bit(deliver_bitmask, s->idx);
> break;
> }
>
> @@ -557,8 +557,8 @@ int apic_get_interrupt(DeviceState *d)
> apic_sync_vapic(s, SYNC_TO_VAPIC);
> return s->spurious_vec & 0xff;
> }
> - reset_bit(s->irr, intno);
> - set_bit(s->isr, intno);
> + apic_reset_bit(s->irr, intno);
> + apic_set_bit(s->isr, intno);
> apic_sync_vapic(s, SYNC_TO_VAPIC);
> apic_update_irq(s);
> return intno;
> --
> 1.7.10.4
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it Eduardo Habkost
@ 2012-07-12 19:29 ` Blue Swirl
2012-07-13 18:09 ` Eduardo Habkost
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-12 19:29 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/pc.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 3b8e469..dc95fb8 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -71,6 +71,7 @@
> #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
> +#define FW_CFG_LAPIC_INFO (FW_CFG_ARCH_LOCAL + 5)
>
> #define MSI_ADDR_BASE 0xfee00000
>
> @@ -87,6 +88,18 @@ struct e820_table {
> struct e820_entry entry[E820_NR_ENTRIES];
> } QEMU_PACKED __attribute((__aligned__(4)));
>
> +struct lapic_info_entry {
> + uint8_t apic_id;
> + uint8_t reserved1;
> + uint16_t reserved2;
> + uint32_t reserved3;
> +} QEMU_PACKED;
CODING_STYLE requires CamelCase, also below.
> +
> +struct lapic_info_table {
> + uint64_t count;
> + struct lapic_info_entry entries[];
> +} QEMU_PACKED;
> +
> static struct e820_table e820_table;
> struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>
> @@ -652,6 +665,16 @@ static void *bochs_bios_init(void)
> fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, (uint8_t *)numa_fw_cfg,
> (1 + max_cpus + nb_numa_nodes) * 8);
>
> + size_t lapic_info_size = sizeof(struct lapic_info_table) + \
> + sizeof(struct lapic_info_entry) * max_cpus;
> + struct lapic_info_table *lapic_table = g_malloc0(lapic_info_size);;
;--
> + lapic_table->count = max_cpus;
> + for (i = 0; i < max_cpus; i++) {
> + lapic_table->entries[i].apic_id = apic_id_for_cpu(i);
> + }
> + fw_cfg_add_bytes(fw_cfg, FW_CFG_LAPIC_INFO, (uint8_t *)lapic_table,
You are passing a structure with host endianness. Please convert the
fields with cpu_to_le32().
> + lapic_info_size);
> +
> return fw_cfg;
> }
>
> --
> 1.7.10.4
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions Eduardo Habkost
@ 2012-07-12 19:37 ` Blue Swirl
2012-07-13 18:51 ` Eduardo Habkost
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-12 19:37 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> target-i386/topology.h | 138 ++++++++++++++++++++++++++++++++++++++++++++++++
> tests/.gitignore | 1 +
> tests/Makefile | 7 ++-
> tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 253 insertions(+), 1 deletion(-)
> create mode 100644 target-i386/topology.h
> create mode 100644 tests/test-x86-cpuid.c
>
> diff --git a/target-i386/topology.h b/target-i386/topology.h
> new file mode 100644
> index 0000000..de0f17a
> --- /dev/null
> +++ b/target-i386/topology.h
> @@ -0,0 +1,138 @@
> +/*
> + * x86 CPU topology data structures and functions
> + *
> + * Copyright (c) 2012 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#ifndef __QEMU_X86_TOPOLOGY_H__
> +#define __QEMU_X86_TOPOLOGY_H__
Please remove the leading and trailing underscores. The name should
match the path, so it should be TARGET_I386_TOPOLOGY_H.
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "bitops.h"
> +
> +/* Return the bit width needed for 'count' IDs
> + */
> +static unsigned bits_for_count(unsigned count)
> +{
> + g_assert(count >= 1);
> + if (count == 1) {
> + return 0;
> + }
> + return bitops_flsl(count - 1) + 1;
> +}
> +
> +/* Bit width of the SMT_ID (thread ID) field on the APIC ID
> + */
> +static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned nr_threads)
> +{
> + return bits_for_count(nr_threads);
> +}
> +
> +/* Bit width of the Core_ID field
> + */
> +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
> +{
> + return bits_for_count(nr_cores);
> +}
> +
> +/* Bit offset of the Core_ID field
> + */
> +static inline unsigned apicid_core_offset(unsigned nr_cores,
> + unsigned nr_threads)
> +{
> + return apicid_smt_width(nr_cores, nr_threads);
The indentation seems to be off, please use checkpatch.pl to avoid these issues.
> +}
> +
> +/* Bit offset of the Pkg_ID (socket ID) field
> + */
> +static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
> +{
> + return apicid_core_offset(nr_cores, nr_threads) + \
> + apicid_core_width(nr_cores, nr_threads);
> +}
> +
> +/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
> + *
> + * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
> + */
> +static inline uint8_t __make_apicid(unsigned nr_cores, unsigned nr_threads,
Again, remove leading underscores.
> + unsigned pkg_id, unsigned core_id,
> + unsigned smt_id)
> +{
> + return (pkg_id << apicid_pkg_offset(nr_cores, nr_threads)) | \
> + (core_id << apicid_core_offset(nr_cores, nr_threads)) | \
> + smt_id;
> +}
> +
> +/* Calculate thread/core/package IDs for a specific topology,
> + * based on (continguous) CPU index
> + */
> +static inline void topo_ids_from_idx(unsigned nr_cores, unsigned nr_threads,
> + unsigned cpu_index,
> + unsigned *pkg_id, unsigned *core_id,
> + unsigned *smt_id)
> +{
> + unsigned core_index = cpu_index / nr_threads;
> + *smt_id = cpu_index % nr_threads;
> + *core_id = core_index % nr_cores;
> + *pkg_id = core_index / nr_cores;
> +}
> +
> +/* Get package ID from an APIC ID
> + */
> +static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads,
> + uint8_t apic_id)
> +{
> + return apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
> +}
> +
> +/* Get core ID from an APIC ID
> + */
> +static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads,
> + uint8_t apic_id)
> +{
> + return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \
> + ((1 << apicid_core_width(nr_cores, nr_threads)) - 1);
> +}
> +
> +/* Get SMT (thread) ID from an APIC ID
> + */
> +static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads,
> + uint8_t apic_id)
> +{
> + return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1);
> +}
> +
> +/* Make APIC ID for the CPU 'cpu_index'
> + *
> + * 'cpu_index' is a sequential, contiguous ID for the CPU.
> + */
> +static inline uint8_t topo_make_apicid(unsigned nr_cores, unsigned nr_threads,
> + unsigned cpu_index)
> +{
> + unsigned pkg_id, core_id, smt_id;
> + topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
> + &pkg_id, &core_id, &smt_id);
> + return __make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
> +}
> +
> +#endif /* __QMU_X86_TOPOLOGY_H__ */
> diff --git a/tests/.gitignore b/tests/.gitignore
> index f9041f3..38c94ef 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -10,4 +10,5 @@ test-qmp-commands.h
> test-qmp-commands
> test-qmp-input-strict
> test-qmp-marshal.c
> +test-x86-cpuid
> *-test
> diff --git a/tests/Makefile b/tests/Makefile
> index b605e14..89bd890 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
> check-unit-y += tests/test-coroutine$(EXESUF)
> check-unit-y += tests/test-visitor-serialization$(EXESUF)
> check-unit-y += tests/test-iov$(EXESUF)
> +check-unit-y += tests/test-x86-cpuid$(EXESUF)
This probably tries to build the cpuid test also for non-x86 targets
and break them all.
>
> check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>
> @@ -33,7 +34,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
> tests/test-coroutine.o tests/test-string-output-visitor.o \
> tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
> tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
> - tests/test-qmp-commands.o tests/test-visitor-serialization.o
> + tests/test-qmp-commands.o tests/test-visitor-serialization.o \
> + tests/test-x86-cpuid.o
>
> test-qapi-obj-y = $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
> test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
> @@ -41,6 +43,8 @@ test-qapi-obj-y += module.o
>
> $(test-obj-y): QEMU_INCLUDES += -Itests
>
> +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
> +
> tests/check-qint$(EXESUF): tests/check-qint.o qint.o $(tools-obj-y)
> tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o $(tools-obj-y)
> tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o $(tools-obj-y)
> @@ -49,6 +53,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
> tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
> tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
> tests/test-iov$(EXESUF): tests/test-iov.o iov.o
> +tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
>
> tests/test-qapi-types.c tests/test-qapi-types.h :\
> $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
> diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
> new file mode 100644
> index 0000000..b27ed62
> --- /dev/null
> +++ b/tests/test-x86-cpuid.c
> @@ -0,0 +1,108 @@
> +/*
> + * Test code for x86 CPUID and Topology functions
> + *
> + * Copyright (c) 2012 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include <glib.h>
> +
> +/*FIXME: this should be built inside the i386 target directory instead */
> +#include "topology.h"
> +
> +static void test_topo_bits(void)
> +{
> + /* simple tests for 1 thread per core, 1 core per socket */
> + g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
> + g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
> +
> + g_assert_cmpuint(topo_make_apicid(1, 1, 0), ==, 0);
> + g_assert_cmpuint(topo_make_apicid(1, 1, 1), ==, 1);
> + g_assert_cmpuint(topo_make_apicid(1, 1, 2), ==, 2);
> + g_assert_cmpuint(topo_make_apicid(1, 1, 3), ==, 3);
> +
> +
> + /* Test field width calculation for multiple values
> + */
> + g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1);
> + g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2);
> + g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2);
> +
> + g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4);
> + g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4);
> + g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4);
> + g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5);
> +
> +
> + g_assert_cmpuint(apicid_core_width(30, 2), ==, 5);
> + g_assert_cmpuint(apicid_core_width(31, 2), ==, 5);
> + g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
> + g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
> +
> +
> + /* build a weird topology and see if IDs are calculated correctly
> + */
> +
> + /* This will use 2 bits for thread ID and 3 bits for core ID
> + */
> + g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2);
> + g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
> + g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
> +
> + g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0);
> + g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1);
> + g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2);
> +
> + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0);
Spaces are needed around operators.
> + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1);
> + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2);
> +
> + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0);
> + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1);
> + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2);
> +
> + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0);
> + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1);
> + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2);
> +
> + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5));
> + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==,
> + (1<<5) | (1<<2) | 1);
> +
> + g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==,
> + (3<<5) | (5<<2) | 2);
> +
> +
> + /* Check the APIC ID -> {pkg,core,thread} ID functions */
> + g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3);
> + g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5);
> + g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + g_test_init(&argc, &argv, NULL);
> +
> + g_test_add_func("/cpuid/topology/basic", test_topo_bits);
> +
> + g_test_run();
> +
> + return 0;
> +}
> --
> 1.7.10.4
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h
2012-07-12 19:24 ` Blue Swirl
@ 2012-07-13 18:07 ` Eduardo Habkost
2012-07-14 9:09 ` Blue Swirl
0 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-13 18:07 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Thu, Jul 12, 2012 at 07:24:35PM +0000, Blue Swirl wrote:
> On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>
> Maybe the bitops functions should be renamed instead, for example
> prefixed by 'qemu_'. That may be safer if one day the kernel find
> their way to system headers too.
Well, if there's any risk the kernel functions will conflict with the
QEMU function names, that would be an additional reason to rename the
apic.c functions too, so they don't conflict with the kernel functions
either.
Personally, I would never sent a patch to rename the bitops.h functions,
as the current names work perfectly to me.
>
> > ---
> > hw/apic.c | 34 +++++++++++++++++-----------------
> > 1 file changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 60552df..d322fe3 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -50,7 +50,7 @@ static int ffs_bit(uint32_t value)
> > return ctz32(value);
> > }
> >
> > -static inline void set_bit(uint32_t *tab, int index)
> > +static inline void apic_set_bit(uint32_t *tab, int index)
> > {
> > int i, mask;
> > i = index >> 5;
> > @@ -58,7 +58,7 @@ static inline void set_bit(uint32_t *tab, int index)
> > tab[i] |= mask;
> > }
> >
> > -static inline void reset_bit(uint32_t *tab, int index)
> > +static inline void apic_reset_bit(uint32_t *tab, int index)
> > {
> > int i, mask;
> > i = index >> 5;
> > @@ -66,7 +66,7 @@ static inline void reset_bit(uint32_t *tab, int index)
> > tab[i] &= ~mask;
> > }
> >
> > -static inline int get_bit(uint32_t *tab, int index)
> > +static inline int apic_get_bit(uint32_t *tab, int index)
> > {
> > int i, mask;
> > i = index >> 5;
> > @@ -183,7 +183,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
> > case APIC_DM_FIXED:
> > if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
> > break;
> > - reset_bit(s->irr, lvt & 0xff);
> > + apic_reset_bit(s->irr, lvt & 0xff);
> > /* fall through */
> > case APIC_DM_EXTINT:
> > cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
> > @@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
> >
> > static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
> > {
> > - apic_report_irq_delivered(!get_bit(s->irr, vector_num));
> > + apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
> >
> > - set_bit(s->irr, vector_num);
> > + apic_set_bit(s->irr, vector_num);
> > if (trigger_mode)
> > - set_bit(s->tmr, vector_num);
> > + apic_set_bit(s->tmr, vector_num);
> > else
> > - reset_bit(s->tmr, vector_num);
> > + apic_reset_bit(s->tmr, vector_num);
> > if (s->vapic_paddr) {
> > apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
> > /*
> > @@ -405,8 +405,8 @@ static void apic_eoi(APICCommonState *s)
> > isrv = get_highest_priority_int(s->isr);
> > if (isrv < 0)
> > return;
> > - reset_bit(s->isr, isrv);
> > - if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
> > + apic_reset_bit(s->isr, isrv);
> > + if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && apic_get_bit(s->tmr, isrv)) {
> > ioapic_eoi_broadcast(isrv);
> > }
> > apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
> > @@ -445,7 +445,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> > int idx = apic_find_dest(dest);
> > memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
> > if (idx >= 0)
> > - set_bit(deliver_bitmask, idx);
> > + apic_set_bit(deliver_bitmask, idx);
> > }
> > } else {
> > /* XXX: cluster mode */
> > @@ -455,11 +455,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> > if (apic_iter) {
> > if (apic_iter->dest_mode == 0xf) {
> > if (dest & apic_iter->log_dest)
> > - set_bit(deliver_bitmask, i);
> > + 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)) {
> > - set_bit(deliver_bitmask, i);
> > + apic_set_bit(deliver_bitmask, i);
> > }
> > }
> > } else {
> > @@ -502,14 +502,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
> > break;
> > case 1:
> > memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
> > - set_bit(deliver_bitmask, s->idx);
> > + apic_set_bit(deliver_bitmask, s->idx);
> > break;
> > case 2:
> > memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> > break;
> > case 3:
> > memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> > - reset_bit(deliver_bitmask, s->idx);
> > + apic_reset_bit(deliver_bitmask, s->idx);
> > break;
> > }
> >
> > @@ -557,8 +557,8 @@ int apic_get_interrupt(DeviceState *d)
> > apic_sync_vapic(s, SYNC_TO_VAPIC);
> > return s->spurious_vec & 0xff;
> > }
> > - reset_bit(s->irr, intno);
> > - set_bit(s->isr, intno);
> > + apic_reset_bit(s->irr, intno);
> > + apic_set_bit(s->isr, intno);
> > apic_sync_vapic(s, SYNC_TO_VAPIC);
> > apic_update_irq(s);
> > return intno;
> > --
> > 1.7.10.4
> >
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it
2012-07-12 19:29 ` Blue Swirl
@ 2012-07-13 18:09 ` Eduardo Habkost
0 siblings, 0 replies; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-13 18:09 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Thu, Jul 12, 2012 at 07:29:02PM +0000, Blue Swirl wrote:
[...]
> CODING_STYLE requires CamelCase, also below.
>
[...]
> > + struct lapic_info_table *lapic_table = g_malloc0(lapic_info_size);;
>
> ;--
>
> > + lapic_table->count = max_cpus;
> > + for (i = 0; i < max_cpus; i++) {
> > + lapic_table->entries[i].apic_id = apic_id_for_cpu(i);
> > + }
> > + fw_cfg_add_bytes(fw_cfg, FW_CFG_LAPIC_INFO, (uint8_t *)lapic_table,
>
> You are passing a structure with host endianness. Please convert the
> fields with cpu_to_le32().
Thanks. I will fix the issues if I send a new version of this patch. But
I believe we'll find a way to make FW_CFG_LAPIC_INFO unnecessary.
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-12 19:37 ` Blue Swirl
@ 2012-07-13 18:51 ` Eduardo Habkost
2012-07-14 9:14 ` Blue Swirl
0 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-13 18:51 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Thu, Jul 12, 2012 at 07:37:26PM +0000, Blue Swirl wrote:
> On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > +#ifndef __QEMU_X86_TOPOLOGY_H__
> > +#define __QEMU_X86_TOPOLOGY_H__
>
> Please remove the leading and trailing underscores. The name should
> match the path, so it should be TARGET_I386_TOPOLOGY_H.
Done. Will be fixed in the next version.
>
> > +/* Bit offset of the Core_ID field
> > + */
> > +static inline unsigned apicid_core_offset(unsigned nr_cores,
> > + unsigned nr_threads)
> > +{
> > + return apicid_smt_width(nr_cores, nr_threads);
>
> The indentation seems to be off, please use checkpatch.pl to avoid these issues.
Fixed for the next version.
(BTW, checkpatch.pl didn't detect any issues on this patch)
>
> > +}
> > +
> > +/* Bit offset of the Pkg_ID (socket ID) field
> > + */
> > +static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
> > +{
> > + return apicid_core_offset(nr_cores, nr_threads) + \
> > + apicid_core_width(nr_cores, nr_threads);
> > +}
> > +
> > +/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
> > + *
> > + * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
> > + */
> > +static inline uint8_t __make_apicid(unsigned nr_cores, unsigned nr_threads,
>
> Again, remove leading underscores.
Fixed for the next version.
>
[...]
> > diff --git a/tests/Makefile b/tests/Makefile
> > index b605e14..89bd890 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
> > check-unit-y += tests/test-coroutine$(EXESUF)
> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
> > check-unit-y += tests/test-iov$(EXESUF)
> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>
> This probably tries to build the cpuid test also for non-x86 targets
> and break them all.
I don't think there's any concept of "targets" for the check-unit tests.
I had to do the following, to be able to make a test that uses the
target-i386 code:
> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
Any suggestions to avoid this hack would be welcome.
>
[...]
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0);
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1);
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2);
> > +
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0);
>
> Spaces are needed around operators.
>
Do you honestly believe that this:
g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0);
g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1);
g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2);
is more readable than this:
g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0);
g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1);
g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2);
?
(I don't).
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1);
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2);
> > +
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0);
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1);
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2);
> > +
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0);
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1);
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2);
> > +
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5));
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==,
> > + (1<<5) | (1<<2) | 1);
> > +
> > + g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==,
> > + (3<<5) | (5<<2) | 2);
> > +
> > +
> > + /* Check the APIC ID -> {pkg,core,thread} ID functions */
> > + g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3);
> > + g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5);
> > + g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + g_test_init(&argc, &argv, NULL);
> > +
> > + g_test_add_func("/cpuid/topology/basic", test_topo_bits);
> > +
> > + g_test_run();
> > +
> > + return 0;
> > +}
> > --
> > 1.7.10.4
> >
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h
2012-07-13 18:07 ` Eduardo Habkost
@ 2012-07-14 9:09 ` Blue Swirl
2012-07-15 9:19 ` Gleb Natapov
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-14 9:09 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Fri, Jul 13, 2012 at 6:07 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Jul 12, 2012 at 07:24:35PM +0000, Blue Swirl wrote:
>> On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>> Maybe the bitops functions should be renamed instead, for example
>> prefixed by 'qemu_'. That may be safer if one day the kernel find
>> their way to system headers too.
>
> Well, if there's any risk the kernel functions will conflict with the
> QEMU function names, that would be an additional reason to rename the
> apic.c functions too, so they don't conflict with the kernel functions
> either.
Yes, that could be the case too.
>
> Personally, I would never sent a patch to rename the bitops.h functions,
> as the current names work perfectly to me.
>
>>
>> > ---
>> > hw/apic.c | 34 +++++++++++++++++-----------------
>> > 1 file changed, 17 insertions(+), 17 deletions(-)
>> >
>> > diff --git a/hw/apic.c b/hw/apic.c
>> > index 60552df..d322fe3 100644
>> > --- a/hw/apic.c
>> > +++ b/hw/apic.c
>> > @@ -50,7 +50,7 @@ static int ffs_bit(uint32_t value)
>> > return ctz32(value);
>> > }
>> >
>> > -static inline void set_bit(uint32_t *tab, int index)
>> > +static inline void apic_set_bit(uint32_t *tab, int index)
>> > {
>> > int i, mask;
>> > i = index >> 5;
>> > @@ -58,7 +58,7 @@ static inline void set_bit(uint32_t *tab, int index)
>> > tab[i] |= mask;
>> > }
>> >
>> > -static inline void reset_bit(uint32_t *tab, int index)
>> > +static inline void apic_reset_bit(uint32_t *tab, int index)
>> > {
>> > int i, mask;
>> > i = index >> 5;
>> > @@ -66,7 +66,7 @@ static inline void reset_bit(uint32_t *tab, int index)
>> > tab[i] &= ~mask;
>> > }
>> >
>> > -static inline int get_bit(uint32_t *tab, int index)
>> > +static inline int apic_get_bit(uint32_t *tab, int index)
>> > {
>> > int i, mask;
>> > i = index >> 5;
>> > @@ -183,7 +183,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
>> > case APIC_DM_FIXED:
>> > if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
>> > break;
>> > - reset_bit(s->irr, lvt & 0xff);
>> > + apic_reset_bit(s->irr, lvt & 0xff);
>> > /* fall through */
>> > case APIC_DM_EXTINT:
>> > cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
>> > @@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
>> >
>> > static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
>> > {
>> > - apic_report_irq_delivered(!get_bit(s->irr, vector_num));
>> > + apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
>> >
>> > - set_bit(s->irr, vector_num);
>> > + apic_set_bit(s->irr, vector_num);
>> > if (trigger_mode)
>> > - set_bit(s->tmr, vector_num);
>> > + apic_set_bit(s->tmr, vector_num);
>> > else
>> > - reset_bit(s->tmr, vector_num);
>> > + apic_reset_bit(s->tmr, vector_num);
>> > if (s->vapic_paddr) {
>> > apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
>> > /*
>> > @@ -405,8 +405,8 @@ static void apic_eoi(APICCommonState *s)
>> > isrv = get_highest_priority_int(s->isr);
>> > if (isrv < 0)
>> > return;
>> > - reset_bit(s->isr, isrv);
>> > - if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
>> > + apic_reset_bit(s->isr, isrv);
>> > + if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && apic_get_bit(s->tmr, isrv)) {
>> > ioapic_eoi_broadcast(isrv);
>> > }
>> > apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
>> > @@ -445,7 +445,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
>> > int idx = apic_find_dest(dest);
>> > memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
>> > if (idx >= 0)
>> > - set_bit(deliver_bitmask, idx);
>> > + apic_set_bit(deliver_bitmask, idx);
>> > }
>> > } else {
>> > /* XXX: cluster mode */
>> > @@ -455,11 +455,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
>> > if (apic_iter) {
>> > if (apic_iter->dest_mode == 0xf) {
>> > if (dest & apic_iter->log_dest)
>> > - set_bit(deliver_bitmask, i);
>> > + 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)) {
>> > - set_bit(deliver_bitmask, i);
>> > + apic_set_bit(deliver_bitmask, i);
>> > }
>> > }
>> > } else {
>> > @@ -502,14 +502,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
>> > break;
>> > case 1:
>> > memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
>> > - set_bit(deliver_bitmask, s->idx);
>> > + apic_set_bit(deliver_bitmask, s->idx);
>> > break;
>> > case 2:
>> > memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
>> > break;
>> > case 3:
>> > memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
>> > - reset_bit(deliver_bitmask, s->idx);
>> > + apic_reset_bit(deliver_bitmask, s->idx);
>> > break;
>> > }
>> >
>> > @@ -557,8 +557,8 @@ int apic_get_interrupt(DeviceState *d)
>> > apic_sync_vapic(s, SYNC_TO_VAPIC);
>> > return s->spurious_vec & 0xff;
>> > }
>> > - reset_bit(s->irr, intno);
>> > - set_bit(s->isr, intno);
>> > + apic_reset_bit(s->irr, intno);
>> > + apic_set_bit(s->isr, intno);
>> > apic_sync_vapic(s, SYNC_TO_VAPIC);
>> > apic_update_irq(s);
>> > return intno;
>> > --
>> > 1.7.10.4
>> >
>> >
>>
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-13 18:51 ` Eduardo Habkost
@ 2012-07-14 9:14 ` Blue Swirl
2012-07-16 17:42 ` Eduardo Habkost
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-14 9:14 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Fri, Jul 13, 2012 at 6:51 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Jul 12, 2012 at 07:37:26PM +0000, Blue Swirl wrote:
>> On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
>> > +#ifndef __QEMU_X86_TOPOLOGY_H__
>> > +#define __QEMU_X86_TOPOLOGY_H__
>>
>> Please remove the leading and trailing underscores. The name should
>> match the path, so it should be TARGET_I386_TOPOLOGY_H.
>
> Done. Will be fixed in the next version.
>
>>
>> > +/* Bit offset of the Core_ID field
>> > + */
>> > +static inline unsigned apicid_core_offset(unsigned nr_cores,
>> > + unsigned nr_threads)
>> > +{
>> > + return apicid_smt_width(nr_cores, nr_threads);
>>
>> The indentation seems to be off, please use checkpatch.pl to avoid these issues.
>
> Fixed for the next version.
>
> (BTW, checkpatch.pl didn't detect any issues on this patch)
>
>>
>> > +}
>> > +
>> > +/* Bit offset of the Pkg_ID (socket ID) field
>> > + */
>> > +static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
>> > +{
>> > + return apicid_core_offset(nr_cores, nr_threads) + \
>> > + apicid_core_width(nr_cores, nr_threads);
>> > +}
>> > +
>> > +/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
>> > + *
>> > + * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
>> > + */
>> > +static inline uint8_t __make_apicid(unsigned nr_cores, unsigned nr_threads,
>>
>> Again, remove leading underscores.
>
> Fixed for the next version.
>
>>
> [...]
>> > diff --git a/tests/Makefile b/tests/Makefile
>> > index b605e14..89bd890 100644
>> > --- a/tests/Makefile
>> > +++ b/tests/Makefile
>> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>> > check-unit-y += tests/test-coroutine$(EXESUF)
>> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> > check-unit-y += tests/test-iov$(EXESUF)
>> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>>
>> This probably tries to build the cpuid test also for non-x86 targets
>> and break them all.
>
> I don't think there's any concept of "targets" for the check-unit tests.
How about:
check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
> I had to do the following, to be able to make a test that uses the
> target-i386 code:
>
>> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
>
> Any suggestions to avoid this hack would be welcome.
Maybe it would be simpler to adjust #include path in the file.
>
>
>>
> [...]
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0);
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1);
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2);
>> > +
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0);
>>
>> Spaces are needed around operators.
>>
>
>
> Do you honestly believe that this:
>
> g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0);
> g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1);
> g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2);
>
> is more readable than this:
>
> g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0);
> g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1);
> g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2);
>
> ?
Yes, I do. It's also the common style.
>
> (I don't).
>
>
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1);
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2);
>> > +
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0);
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1);
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2);
>> > +
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0);
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1);
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2);
>> > +
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5));
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==,
>> > + (1<<5) | (1<<2) | 1);
>> > +
>> > + g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==,
>> > + (3<<5) | (5<<2) | 2);
>> > +
>> > +
>> > + /* Check the APIC ID -> {pkg,core,thread} ID functions */
>> > + g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3);
>> > + g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5);
>> > + g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2);
>> > +}
>> > +
>> > +int main(int argc, char **argv)
>> > +{
>> > + g_test_init(&argc, &argv, NULL);
>> > +
>> > + g_test_add_func("/cpuid/topology/basic", test_topo_bits);
>> > +
>> > + g_test_run();
>> > +
>> > + return 0;
>> > +}
>> > --
>> > 1.7.10.4
>> >
>> >
>>
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h
2012-07-14 9:09 ` Blue Swirl
@ 2012-07-15 9:19 ` Gleb Natapov
0 siblings, 0 replies; 28+ messages in thread
From: Gleb Natapov @ 2012-07-15 9:19 UTC (permalink / raw)
To: Blue Swirl
Cc: Igor Mammedov, seabios, Eduardo Habkost, Anthony Liguori, qemu-devel
On Sat, Jul 14, 2012 at 09:09:23AM +0000, Blue Swirl wrote:
> On Fri, Jul 13, 2012 at 6:07 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, Jul 12, 2012 at 07:24:35PM +0000, Blue Swirl wrote:
> >> On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>
> >> Maybe the bitops functions should be renamed instead, for example
> >> prefixed by 'qemu_'. That may be safer if one day the kernel find
> >> their way to system headers too.
> >
> > Well, if there's any risk the kernel functions will conflict with the
> > QEMU function names, that would be an additional reason to rename the
> > apic.c functions too, so they don't conflict with the kernel functions
> > either.
>
> Yes, that could be the case too.
>
Than it would be Linux headers problem and will be fixed there.
> >
> > Personally, I would never sent a patch to rename the bitops.h functions,
> > as the current names work perfectly to me.
> >
> >>
> >> > ---
> >> > hw/apic.c | 34 +++++++++++++++++-----------------
> >> > 1 file changed, 17 insertions(+), 17 deletions(-)
> >> >
> >> > diff --git a/hw/apic.c b/hw/apic.c
> >> > index 60552df..d322fe3 100644
> >> > --- a/hw/apic.c
> >> > +++ b/hw/apic.c
> >> > @@ -50,7 +50,7 @@ static int ffs_bit(uint32_t value)
> >> > return ctz32(value);
> >> > }
> >> >
> >> > -static inline void set_bit(uint32_t *tab, int index)
> >> > +static inline void apic_set_bit(uint32_t *tab, int index)
> >> > {
> >> > int i, mask;
> >> > i = index >> 5;
> >> > @@ -58,7 +58,7 @@ static inline void set_bit(uint32_t *tab, int index)
> >> > tab[i] |= mask;
> >> > }
> >> >
> >> > -static inline void reset_bit(uint32_t *tab, int index)
> >> > +static inline void apic_reset_bit(uint32_t *tab, int index)
> >> > {
> >> > int i, mask;
> >> > i = index >> 5;
> >> > @@ -66,7 +66,7 @@ static inline void reset_bit(uint32_t *tab, int index)
> >> > tab[i] &= ~mask;
> >> > }
> >> >
> >> > -static inline int get_bit(uint32_t *tab, int index)
> >> > +static inline int apic_get_bit(uint32_t *tab, int index)
> >> > {
> >> > int i, mask;
> >> > i = index >> 5;
> >> > @@ -183,7 +183,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
> >> > case APIC_DM_FIXED:
> >> > if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
> >> > break;
> >> > - reset_bit(s->irr, lvt & 0xff);
> >> > + apic_reset_bit(s->irr, lvt & 0xff);
> >> > /* fall through */
> >> > case APIC_DM_EXTINT:
> >> > cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
> >> > @@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
> >> >
> >> > static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
> >> > {
> >> > - apic_report_irq_delivered(!get_bit(s->irr, vector_num));
> >> > + apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
> >> >
> >> > - set_bit(s->irr, vector_num);
> >> > + apic_set_bit(s->irr, vector_num);
> >> > if (trigger_mode)
> >> > - set_bit(s->tmr, vector_num);
> >> > + apic_set_bit(s->tmr, vector_num);
> >> > else
> >> > - reset_bit(s->tmr, vector_num);
> >> > + apic_reset_bit(s->tmr, vector_num);
> >> > if (s->vapic_paddr) {
> >> > apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
> >> > /*
> >> > @@ -405,8 +405,8 @@ static void apic_eoi(APICCommonState *s)
> >> > isrv = get_highest_priority_int(s->isr);
> >> > if (isrv < 0)
> >> > return;
> >> > - reset_bit(s->isr, isrv);
> >> > - if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
> >> > + apic_reset_bit(s->isr, isrv);
> >> > + if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && apic_get_bit(s->tmr, isrv)) {
> >> > ioapic_eoi_broadcast(isrv);
> >> > }
> >> > apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
> >> > @@ -445,7 +445,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> >> > int idx = apic_find_dest(dest);
> >> > memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
> >> > if (idx >= 0)
> >> > - set_bit(deliver_bitmask, idx);
> >> > + apic_set_bit(deliver_bitmask, idx);
> >> > }
> >> > } else {
> >> > /* XXX: cluster mode */
> >> > @@ -455,11 +455,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> >> > if (apic_iter) {
> >> > if (apic_iter->dest_mode == 0xf) {
> >> > if (dest & apic_iter->log_dest)
> >> > - set_bit(deliver_bitmask, i);
> >> > + 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)) {
> >> > - set_bit(deliver_bitmask, i);
> >> > + apic_set_bit(deliver_bitmask, i);
> >> > }
> >> > }
> >> > } else {
> >> > @@ -502,14 +502,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
> >> > break;
> >> > case 1:
> >> > memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
> >> > - set_bit(deliver_bitmask, s->idx);
> >> > + apic_set_bit(deliver_bitmask, s->idx);
> >> > break;
> >> > case 2:
> >> > memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> >> > break;
> >> > case 3:
> >> > memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> >> > - reset_bit(deliver_bitmask, s->idx);
> >> > + apic_reset_bit(deliver_bitmask, s->idx);
> >> > break;
> >> > }
> >> >
> >> > @@ -557,8 +557,8 @@ int apic_get_interrupt(DeviceState *d)
> >> > apic_sync_vapic(s, SYNC_TO_VAPIC);
> >> > return s->spurious_vec & 0xff;
> >> > }
> >> > - reset_bit(s->irr, intno);
> >> > - set_bit(s->isr, intno);
> >> > + apic_reset_bit(s->irr, intno);
> >> > + apic_set_bit(s->isr, intno);
> >> > apic_sync_vapic(s, SYNC_TO_VAPIC);
> >> > apic_update_irq(s);
> >> > return intno;
> >> > --
> >> > 1.7.10.4
> >> >
> >> >
> >>
> >
> > --
> > Eduardo
--
Gleb.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-14 9:14 ` Blue Swirl
@ 2012-07-16 17:42 ` Eduardo Habkost
2012-07-23 16:49 ` Blue Swirl
0 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-16 17:42 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
[...]
> >> > diff --git a/tests/Makefile b/tests/Makefile
> >> > index b605e14..89bd890 100644
> >> > --- a/tests/Makefile
> >> > +++ b/tests/Makefile
> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
> >> > check-unit-y += tests/test-coroutine$(EXESUF)
> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >> > check-unit-y += tests/test-iov$(EXESUF)
> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
> >>
> >> This probably tries to build the cpuid test also for non-x86 targets
> >> and break them all.
> >
> > I don't think there's any concept of "targets" for the check-unit tests.
>
> How about:
> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
test-x86-cpuid is not a qtest test case.
>
> > I had to do the following, to be able to make a test that uses the
> > target-i386 code:
> >
> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
> >
> > Any suggestions to avoid this hack would be welcome.
>
> Maybe it would be simpler to adjust #include path in the file.
Using the full path on the #include line would break in case
target-i386/topology.h include other files from the target-i386
directory.
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-16 17:42 ` Eduardo Habkost
@ 2012-07-23 16:49 ` Blue Swirl
2012-07-23 18:59 ` Eduardo Habkost
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-23 16:49 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
> [...]
>> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> > index b605e14..89bd890 100644
>> >> > --- a/tests/Makefile
>> >> > +++ b/tests/Makefile
>> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>> >> > check-unit-y += tests/test-coroutine$(EXESUF)
>> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> >> > check-unit-y += tests/test-iov$(EXESUF)
>> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >>
>> >> This probably tries to build the cpuid test also for non-x86 targets
>> >> and break them all.
>> >
>> > I don't think there's any concept of "targets" for the check-unit tests.
>>
>> How about:
>> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
>
> test-x86-cpuid is not a qtest test case.
Why not? I don't think it is a unit test either, judging from what the
other unit tests do.
>
>>
>> > I had to do the following, to be able to make a test that uses the
>> > target-i386 code:
>> >
>> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
>> >
>> > Any suggestions to avoid this hack would be welcome.
>>
>> Maybe it would be simpler to adjust #include path in the file.
>
> Using the full path on the #include line would break in case
> target-i386/topology.h include other files from the target-i386
> directory.
That's fragile. Maybe the target-xyz files should use the full path.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-23 16:49 ` Blue Swirl
@ 2012-07-23 18:59 ` Eduardo Habkost
2012-07-23 19:11 ` Blue Swirl
0 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-23 18:59 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote:
> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
> > [...]
> >> >> > diff --git a/tests/Makefile b/tests/Makefile
> >> >> > index b605e14..89bd890 100644
> >> >> > --- a/tests/Makefile
> >> >> > +++ b/tests/Makefile
> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
> >> >> > check-unit-y += tests/test-coroutine$(EXESUF)
> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >> >> > check-unit-y += tests/test-iov$(EXESUF)
> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
> >> >>
> >> >> This probably tries to build the cpuid test also for non-x86 targets
> >> >> and break them all.
> >> >
> >> > I don't think there's any concept of "targets" for the check-unit tests.
> >>
> >> How about:
> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
> >
> > test-x86-cpuid is not a qtest test case.
>
> Why not? I don't think it is a unit test either, judging from what the
> other unit tests do.
It is absolutely a unit test. I don't know why you don't think so. It
simply checks the results of the APIC ID calculation functions.
>
> >
> >>
> >> > I had to do the following, to be able to make a test that uses the
> >> > target-i386 code:
> >> >
> >> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
> >> >
> >> > Any suggestions to avoid this hack would be welcome.
> >>
> >> Maybe it would be simpler to adjust #include path in the file.
> >
> > Using the full path on the #include line would break in case
> > target-i386/topology.h include other files from the target-i386
> > directory.
>
> That's fragile. Maybe the target-xyz files should use the full path.
Yes, it would be fragile. That's why I used -Itarget-i386 (not a perfect
solution, but more likely to stay working and not break easily).
I don't know if I understand what you propose. You mean to change the
256 target-specific #include lines inside qemu?
$ git grep -f <(find target-* -name '*.h' | sed -e 's@target-[^/]*/@#include "@' | sort -u) target-* | wc -l
256
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-23 18:59 ` Eduardo Habkost
@ 2012-07-23 19:11 ` Blue Swirl
2012-07-23 19:28 ` Eduardo Habkost
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-23 19:11 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote:
>> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
>> > [...]
>> >> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> >> > index b605e14..89bd890 100644
>> >> >> > --- a/tests/Makefile
>> >> >> > +++ b/tests/Makefile
>> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>> >> >> > check-unit-y += tests/test-coroutine$(EXESUF)
>> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> >> >> > check-unit-y += tests/test-iov$(EXESUF)
>> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >> >>
>> >> >> This probably tries to build the cpuid test also for non-x86 targets
>> >> >> and break them all.
>> >> >
>> >> > I don't think there's any concept of "targets" for the check-unit tests.
>> >>
>> >> How about:
>> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
>> >
>> > test-x86-cpuid is not a qtest test case.
>>
>> Why not? I don't think it is a unit test either, judging from what the
>> other unit tests do.
>
> It is absolutely a unit test. I don't know why you don't think so. It
> simply checks the results of the APIC ID calculation functions.
Yes, but the other 'unit tests' (the names used here are very
confusing, btw) check supporting functions like coroutines, not
hardware. Hardware tests (qtest) need to bind to an architecture, in
this case x86.
>
>
>>
>> >
>> >>
>> >> > I had to do the following, to be able to make a test that uses the
>> >> > target-i386 code:
>> >> >
>> >> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
>> >> >
>> >> > Any suggestions to avoid this hack would be welcome.
>> >>
>> >> Maybe it would be simpler to adjust #include path in the file.
>> >
>> > Using the full path on the #include line would break in case
>> > target-i386/topology.h include other files from the target-i386
>> > directory.
>>
>> That's fragile. Maybe the target-xyz files should use the full path.
>
> Yes, it would be fragile. That's why I used -Itarget-i386 (not a perfect
> solution, but more likely to stay working and not break easily).
>
> I don't know if I understand what you propose. You mean to change the
> 256 target-specific #include lines inside qemu?
>
> $ git grep -f <(find target-* -name '*.h' | sed -e 's@target-[^/]*/@#include "@' | sort -u) target-* | wc -l
> 256
That does not look very attractive either. Scripting could help for
such mechanical changes. Maybe later.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-23 19:11 ` Blue Swirl
@ 2012-07-23 19:28 ` Eduardo Habkost
2012-07-23 19:44 ` Blue Swirl
0 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-23 19:28 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote:
> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote:
> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
> >> > [...]
> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile
> >> >> >> > index b605e14..89bd890 100644
> >> >> >> > --- a/tests/Makefile
> >> >> >> > +++ b/tests/Makefile
> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF)
> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >> >> >> > check-unit-y += tests/test-iov$(EXESUF)
> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
> >> >> >>
> >> >> >> This probably tries to build the cpuid test also for non-x86 targets
> >> >> >> and break them all.
> >> >> >
> >> >> > I don't think there's any concept of "targets" for the check-unit tests.
> >> >>
> >> >> How about:
> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
> >> >
> >> > test-x86-cpuid is not a qtest test case.
> >>
> >> Why not? I don't think it is a unit test either, judging from what the
> >> other unit tests do.
> >
> > It is absolutely a unit test. I don't know why you don't think so. It
> > simply checks the results of the APIC ID calculation functions.
>
> Yes, but the other 'unit tests' (the names used here are very
> confusing, btw) check supporting functions like coroutines, not
> hardware. Hardware tests (qtest) need to bind to an architecture, in
> this case x86.
test-x86-cpuid doesn't check hardware, either. It just checks the simple
functions that calculate APIC IDs (to make sure the math is correct).
Those functions aren't even used by any hardware emulation code until
later in the patch series (when the CPU initialization code gets changed
to use the new function).
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-23 19:28 ` Eduardo Habkost
@ 2012-07-23 19:44 ` Blue Swirl
2012-07-23 20:14 ` Eduardo Habkost
0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2012-07-23 19:44 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote:
>> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote:
>> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
>> >> > [...]
>> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> >> >> > index b605e14..89bd890 100644
>> >> >> >> > --- a/tests/Makefile
>> >> >> >> > +++ b/tests/Makefile
>> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF)
>> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> >> >> >> > check-unit-y += tests/test-iov$(EXESUF)
>> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >> >> >>
>> >> >> >> This probably tries to build the cpuid test also for non-x86 targets
>> >> >> >> and break them all.
>> >> >> >
>> >> >> > I don't think there's any concept of "targets" for the check-unit tests.
>> >> >>
>> >> >> How about:
>> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
>> >> >
>> >> > test-x86-cpuid is not a qtest test case.
>> >>
>> >> Why not? I don't think it is a unit test either, judging from what the
>> >> other unit tests do.
>> >
>> > It is absolutely a unit test. I don't know why you don't think so. It
>> > simply checks the results of the APIC ID calculation functions.
>>
>> Yes, but the other 'unit tests' (the names used here are very
>> confusing, btw) check supporting functions like coroutines, not
>> hardware. Hardware tests (qtest) need to bind to an architecture, in
>> this case x86.
>
> test-x86-cpuid doesn't check hardware, either. It just checks the simple
> functions that calculate APIC IDs (to make sure the math is correct).
> Those functions aren't even used by any hardware emulation code until
> later in the patch series (when the CPU initialization code gets changed
> to use the new function).
By that time the function is clearly x86 HW and the check is an x86
hardware check. QEMU as whole consists of simple functions that
calculate something.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-23 19:44 ` Blue Swirl
@ 2012-07-23 20:14 ` Eduardo Habkost
2012-07-24 19:17 ` Blue Swirl
0 siblings, 1 reply; 28+ messages in thread
From: Eduardo Habkost @ 2012-07-23 20:14 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Mon, Jul 23, 2012 at 07:44:44PM +0000, Blue Swirl wrote:
> On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote:
> >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote:
> >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
> >> >> > [...]
> >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile
> >> >> >> >> > index b605e14..89bd890 100644
> >> >> >> >> > --- a/tests/Makefile
> >> >> >> >> > +++ b/tests/Makefile
> >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
> >> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF)
> >> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >> >> >> >> > check-unit-y += tests/test-iov$(EXESUF)
> >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
> >> >> >> >>
> >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets
> >> >> >> >> and break them all.
> >> >> >> >
> >> >> >> > I don't think there's any concept of "targets" for the check-unit tests.
> >> >> >>
> >> >> >> How about:
> >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
> >> >> >
> >> >> > test-x86-cpuid is not a qtest test case.
> >> >>
> >> >> Why not? I don't think it is a unit test either, judging from what the
> >> >> other unit tests do.
> >> >
> >> > It is absolutely a unit test. I don't know why you don't think so. It
> >> > simply checks the results of the APIC ID calculation functions.
> >>
> >> Yes, but the other 'unit tests' (the names used here are very
> >> confusing, btw) check supporting functions like coroutines, not
> >> hardware. Hardware tests (qtest) need to bind to an architecture, in
> >> this case x86.
> >
> > test-x86-cpuid doesn't check hardware, either. It just checks the simple
> > functions that calculate APIC IDs (to make sure the math is correct).
> > Those functions aren't even used by any hardware emulation code until
> > later in the patch series (when the CPU initialization code gets changed
> > to use the new function).
>
> By that time the function is clearly x86 HW and the check is an x86
> hardware check. QEMU as whole consists of simple functions that
> calculate something.
It's useful onily for a specific architecture, yes, but it surely
doesn't make libqtest necessary.
That's the difference between the unit tests and qtest test cases: unit
tests simply test small parts of the code (that may or may not be
hardware-specific), and qtest tests hardware emulation after starting up
a whole qemu process. It's a different kind of testing, with different
sets of goals.
I suppose you are not arguing that no function inside target-* would be
ever allowed to have a unit test written for it. That would be a very
silly constraint for people writing tests.
--
Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions
2012-07-23 20:14 ` Eduardo Habkost
@ 2012-07-24 19:17 ` Blue Swirl
0 siblings, 0 replies; 28+ messages in thread
From: Blue Swirl @ 2012-07-24 19:17 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anthony Liguori, Igor Mammedov, seabios, qemu-devel, Gleb Natapov
On Mon, Jul 23, 2012 at 8:14 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Jul 23, 2012 at 07:44:44PM +0000, Blue Swirl wrote:
>> On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote:
>> >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote:
>> >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote:
>> >> >> > [...]
>> >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile
>> >> >> >> >> > index b605e14..89bd890 100644
>> >> >> >> >> > --- a/tests/Makefile
>> >> >> >> >> > +++ b/tests/Makefile
>> >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>> >> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF)
>> >> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> >> >> >> >> > check-unit-y += tests/test-iov$(EXESUF)
>> >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> >> >> >> >>
>> >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets
>> >> >> >> >> and break them all.
>> >> >> >> >
>> >> >> >> > I don't think there's any concept of "targets" for the check-unit tests.
>> >> >> >>
>> >> >> >> How about:
>> >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF)
>> >> >> >
>> >> >> > test-x86-cpuid is not a qtest test case.
>> >> >>
>> >> >> Why not? I don't think it is a unit test either, judging from what the
>> >> >> other unit tests do.
>> >> >
>> >> > It is absolutely a unit test. I don't know why you don't think so. It
>> >> > simply checks the results of the APIC ID calculation functions.
>> >>
>> >> Yes, but the other 'unit tests' (the names used here are very
>> >> confusing, btw) check supporting functions like coroutines, not
>> >> hardware. Hardware tests (qtest) need to bind to an architecture, in
>> >> this case x86.
>> >
>> > test-x86-cpuid doesn't check hardware, either. It just checks the simple
>> > functions that calculate APIC IDs (to make sure the math is correct).
>> > Those functions aren't even used by any hardware emulation code until
>> > later in the patch series (when the CPU initialization code gets changed
>> > to use the new function).
>>
>> By that time the function is clearly x86 HW and the check is an x86
>> hardware check. QEMU as whole consists of simple functions that
>> calculate something.
>
> It's useful onily for a specific architecture, yes, but it surely
> doesn't make libqtest necessary.
>
> That's the difference between the unit tests and qtest test cases: unit
> tests simply test small parts of the code (that may or may not be
> hardware-specific), and qtest tests hardware emulation after starting up
> a whole qemu process. It's a different kind of testing, with different
> sets of goals.
>
Thanks for the clarification. I agree now that this is not a qtest. I
think this is a new category of tests compared to those we have now:
supporting function unit tests and hardware tests at device I/O
boundary level.
> I suppose you are not arguing that no function inside target-* would be
> ever allowed to have a unit test written for it. That would be a very
> silly constraint for people writing tests.
Of course. What I really want is that if x86 targets are not built,
this test is skipped like qtests. This could be achieved with
something like:
check-unit-arch-i386-y = tests/test-x86-cpuid$(EXESUF)
check-unit-y += $(foreach TARGET,$(TARGETS), $(check-unit-arch-$(TARGET)-y))
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2012-07-24 19:17 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-10 20:22 [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [PATCH 1/7] cpus.h: include cpu-common.h Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h Eduardo Habkost
2012-07-12 19:24 ` Blue Swirl
2012-07-13 18:07 ` Eduardo Habkost
2012-07-14 9:09 ` Blue Swirl
2012-07-15 9:19 ` Gleb Natapov
2012-07-10 20:22 ` [Qemu-devel] [QEMU PATCH 3/7] kvm: set vcpu_id to APIC ID instead of CPU index Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 4/7] i386: create apic_id_for_cpu() function Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it Eduardo Habkost
2012-07-12 19:29 ` Blue Swirl
2012-07-13 18:09 ` Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions Eduardo Habkost
2012-07-12 19:37 ` Blue Swirl
2012-07-13 18:51 ` Eduardo Habkost
2012-07-14 9:14 ` Blue Swirl
2012-07-16 17:42 ` Eduardo Habkost
2012-07-23 16:49 ` Blue Swirl
2012-07-23 18:59 ` Eduardo Habkost
2012-07-23 19:11 ` Blue Swirl
2012-07-23 19:28 ` Eduardo Habkost
2012-07-23 19:44 ` Blue Swirl
2012-07-23 20:14 ` Eduardo Habkost
2012-07-24 19:17 ` Blue Swirl
2012-07-10 20:22 ` [Qemu-devel] [QEMU RFC PATCH 7/7] generate APIC IDs according to CPU topology Eduardo Habkost
2012-07-10 20:22 ` [Qemu-devel] [Seabios RFC PATCH 1/1] get lapic IDs from fw_cfg Eduardo Habkost
2012-07-12 13:51 ` [Qemu-devel] [RFC 0/7+1] QEMU APIC ID + topology bug + CPU hotplug Igor Mammedov
2012-07-12 14:00 ` Gleb Natapov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.