From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59479) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwUfa-0000En-S0 for qemu-devel@nongnu.org; Tue, 18 Oct 2016 09:43:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwUfX-0000lK-Pz for qemu-devel@nongnu.org; Tue, 18 Oct 2016 09:43:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43370) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwUfX-0000l2-IL for qemu-devel@nongnu.org; Tue, 18 Oct 2016 09:43:03 -0400 Date: Tue, 18 Oct 2016 15:42:59 +0200 From: Igor Mammedov Message-ID: <20161018154259.29f31e24@nial.brq.redhat.com> In-Reply-To: <20161018130547.GC22567@thinpad.lan.raisama.net> References: <1476352367-69400-1-git-send-email-imammedo@redhat.com> <1476352367-69400-2-git-send-email-imammedo@redhat.com> <20161018124702.GM3275@thinpad.lan.raisama.net> <20161018130547.GC22567@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 01/13] pc: acpi: x2APIC support for MADT table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, kraxel@redhat.com, liuxiaojian6@huawei.com, mst@redhat.com, rkrcmar@redhat.com, peterx@redhat.com, kevin@koconnor.net, pbonzini@redhat.com, lersek@redhat.com, chao.gao@intel.com On Tue, 18 Oct 2016 11:05:47 -0200 Eduardo Habkost wrote: > On Tue, Oct 18, 2016 at 10:47:02AM -0200, Eduardo Habkost wrote: > > On Thu, Oct 13, 2016 at 11:52:35AM +0200, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov > > > > Reviewed-by: Eduardo Habkost > > Reviewed-by withdrawn. See below: > > [...] > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index e999654..702d254 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -340,24 +340,38 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm, > > > void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, > > > CPUArchIdList *apic_ids, GArray *entry) > > > { > > > - int apic_id; > > > - AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic); > > > - > > > - apic_id = apic_ids->cpus[uid].arch_id; > > > - apic->type = ACPI_APIC_PROCESSOR; > > > - apic->length = sizeof(*apic); > > > - apic->processor_id = uid; > > > - apic->local_apic_id = apic_id; > > > - if (apic_ids->cpus[uid].cpu != NULL) { > > > - apic->flags = cpu_to_le32(1); > > > > Shouldn't length, processor_id, and local_apic_id be converted to > > little-endian too? > > Erm, those fields are all 8-bit. Nevermind. But that means the > new x2apic code below seems wrong: Thanks for noticing, it needs cpu_to_le32() at some places. I'll fix it and post v4 here. > > > > > > + uint32_t apic_id = apic_ids->cpus[uid].arch_id; > > > + > > > + /* ACPI spec says that LAPIC entry for non present > > > + * CPU may be omitted from MADT or it must be marked > > > + * as disabled. However omitting non present CPU from > > > + * MADT breaks hotplug on linux. So possible CPUs > > > + * should be put in MADT but kept disabled. > > > + */ > > > + if (apic_id < 255) { > > > + AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic); > > > + > > > + apic->type = ACPI_APIC_PROCESSOR; > > > + apic->length = sizeof(*apic); > > > + apic->processor_id = uid; > > > + apic->local_apic_id = apic_id; > > > + if (apic_ids->cpus[uid].cpu != NULL) { > > > + apic->flags = cpu_to_le32(1); > > > + } else { > > > + apic->flags = cpu_to_le32(0); > > > + } > > > } else { > > > - /* ACPI spec says that LAPIC entry for non present > > > - * CPU may be omitted from MADT or it must be marked > > > - * as disabled. However omitting non present CPU from > > > - * MADT breaks hotplug on linux. So possible CPUs > > > - * should be put in MADT but kept disabled. > > > - */ > > > - apic->flags = cpu_to_le32(0); > > > + AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic); > > > + > > > + apic->type = ACPI_APIC_LOCAL_X2APIC; > > > + apic->length = sizeof(*apic); > > > + apic->uid = uid; > > cpu_to_le32()? > > > > + apic->x2apic_id = apic_id; > > cpu_to_le32()? > > > > + if (apic_ids->cpus[uid].cpu != NULL) { > > > + apic->flags = cpu_to_le32(1); > > > + } else { > > > + apic->flags = cpu_to_le32(0); > > > + } > > > } > > > } > [...] >