| Message ID | 20210616204328.2611406-20-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386/sev: Housekeeping (OVMF + SEV-disabled binaries) | expand |
On Wed, 16 Jun 2021 22:43:24 +0200 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > pc_madt_cpu_entry() is specific to QEMU 'PC' machines, > move it to acpi-pc.c. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/i386/acpi-common.c | 40 ---------------------------------------- > hw/i386/acpi-pc.c | 39 +++++++++++++++++++++++++++++++++++++++ it's used not only by PC machines but also microvm, which didn't use acpi-build.c (aka acpi-pc.c) it only links fine by virtue that PC machines are object files are also included. > 2 files changed, 39 insertions(+), 40 deletions(-) > > diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c > index 77afebd9e1f..5ae1853b6f2 100644 > --- a/hw/i386/acpi-common.c > +++ b/hw/i386/acpi-common.c > @@ -23,49 +23,10 @@ > #include "qemu/osdep.h" > #include "hw/acpi/acpi.h" > #include "hw/acpi/aml-build.h" > -#include "hw/i386/pc.h" > #include "target/i386/cpu.h" > #include "acpi-build.h" > #include "acpi-common.h" > > -void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, > - const CPUArchIdList *apic_ids, GArray *entry) > -{ > - 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 { > - AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic); > - > - apic->type = ACPI_APIC_LOCAL_X2APIC; > - apic->length = sizeof(*apic); > - apic->uid = cpu_to_le32(uid); > - apic->x2apic_id = cpu_to_le32(apic_id); > - if (apic_ids->cpus[uid].cpu != NULL) { > - apic->flags = cpu_to_le32(1); > - } else { > - apic->flags = cpu_to_le32(0); > - } > - } > -} > - > void acpi_build_madt(GArray *table_data, BIOSLinker *linker, > X86MachineState *x86ms, AcpiDeviceIf *adev, > const char *oem_id, const char *oem_table_id) > @@ -155,4 +116,3 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker, > (void *)(table_data->data + madt_start), "APIC", > table_data->len - madt_start, 1, oem_id, oem_table_id); > } > - > diff --git a/hw/i386/acpi-pc.c b/hw/i386/acpi-pc.c > index 796ffc6f5c4..a3cd60d81e6 100644 > --- a/hw/i386/acpi-pc.c > +++ b/hw/i386/acpi-pc.c > @@ -2707,3 +2707,42 @@ void acpi_setup(void) > */ > acpi_build_tables_cleanup(&tables, false); > } > + > +void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, > + const CPUArchIdList *apic_ids, GArray *entry) > +{ > + 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 { > + AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic); > + > + apic->type = ACPI_APIC_LOCAL_X2APIC; > + apic->length = sizeof(*apic); > + apic->uid = cpu_to_le32(uid); > + apic->x2apic_id = cpu_to_le32(apic_id); > + if (apic_ids->cpus[uid].cpu != NULL) { > + apic->flags = cpu_to_le32(1); > + } else { > + apic->flags = cpu_to_le32(0); > + } > + } > +}
On 6/18/21 1:37 PM, Igor Mammedov wrote: > On Wed, 16 Jun 2021 22:43:24 +0200 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> pc_madt_cpu_entry() is specific to QEMU 'PC' machines, >> move it to acpi-pc.c. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/i386/acpi-common.c | 40 ---------------------------------------- >> hw/i386/acpi-pc.c | 39 +++++++++++++++++++++++++++++++++++++++ > > it's used not only by PC machines but also microvm, > which didn't use acpi-build.c (aka acpi-pc.c) > it only links fine by virtue that PC machines > are object files are also included. Is that something new? I can't see this in mainstream, the microvm machine builds fine without this code.
On Sat, Jun 19, 2021 at 10:45:17AM +0200, Philippe Mathieu-Daudé wrote: > On 6/18/21 1:37 PM, Igor Mammedov wrote: > > On Wed, 16 Jun 2021 22:43:24 +0200 > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > >> pc_madt_cpu_entry() is specific to QEMU 'PC' machines, > >> move it to acpi-pc.c. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> hw/i386/acpi-common.c | 40 ---------------------------------------- > >> hw/i386/acpi-pc.c | 39 +++++++++++++++++++++++++++++++++++++++ > > > > it's used not only by PC machines but also microvm, > > which didn't use acpi-build.c (aka acpi-pc.c) > > it only links fine by virtue that PC machines > > are object files are also included. > > Is that something new? I can't see this in mainstream, > the microvm machine builds fine without this code. I think Igor means this: hw/i386/generic_event_device_x86.c: adevc->madt_cpu = pc_madt_cpu_entry; and hw/i386/microvm.c: DeviceState *dev = qdev_new(TYPE_ACPI_GED_X86); Admittedly given it's not limited to pc the function name is wrong ...
On 6/19/21 11:32 PM, Michael S. Tsirkin wrote: > On Sat, Jun 19, 2021 at 10:45:17AM +0200, Philippe Mathieu-Daudé wrote: >> On 6/18/21 1:37 PM, Igor Mammedov wrote: >>> On Wed, 16 Jun 2021 22:43:24 +0200 >>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>> >>>> pc_madt_cpu_entry() is specific to QEMU 'PC' machines, >>>> move it to acpi-pc.c. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> hw/i386/acpi-common.c | 40 ---------------------------------------- >>>> hw/i386/acpi-pc.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> >>> it's used not only by PC machines but also microvm, >>> which didn't use acpi-build.c (aka acpi-pc.c) >>> it only links fine by virtue that PC machines >>> are object files are also included. >> >> Is that something new? I can't see this in mainstream, >> the microvm machine builds fine without this code. > > I think Igor means this: > > hw/i386/generic_event_device_x86.c: adevc->madt_cpu = pc_madt_cpu_entry; > > and > > hw/i386/microvm.c: DeviceState *dev = qdev_new(TYPE_ACPI_GED_X86); Oh I missed that, indeed I didn't runtime test. Thanks Michael. > Admittedly given it's not limited to pc the function name is wrong ... >
diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c index 77afebd9e1f..5ae1853b6f2 100644 --- a/hw/i386/acpi-common.c +++ b/hw/i386/acpi-common.c @@ -23,49 +23,10 @@ #include "qemu/osdep.h" #include "hw/acpi/acpi.h" #include "hw/acpi/aml-build.h" -#include "hw/i386/pc.h" #include "target/i386/cpu.h" #include "acpi-build.h" #include "acpi-common.h" -void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, - const CPUArchIdList *apic_ids, GArray *entry) -{ - 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 { - AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic); - - apic->type = ACPI_APIC_LOCAL_X2APIC; - apic->length = sizeof(*apic); - apic->uid = cpu_to_le32(uid); - apic->x2apic_id = cpu_to_le32(apic_id); - if (apic_ids->cpus[uid].cpu != NULL) { - apic->flags = cpu_to_le32(1); - } else { - apic->flags = cpu_to_le32(0); - } - } -} - void acpi_build_madt(GArray *table_data, BIOSLinker *linker, X86MachineState *x86ms, AcpiDeviceIf *adev, const char *oem_id, const char *oem_table_id) @@ -155,4 +116,3 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker, (void *)(table_data->data + madt_start), "APIC", table_data->len - madt_start, 1, oem_id, oem_table_id); } - diff --git a/hw/i386/acpi-pc.c b/hw/i386/acpi-pc.c index 796ffc6f5c4..a3cd60d81e6 100644 --- a/hw/i386/acpi-pc.c +++ b/hw/i386/acpi-pc.c @@ -2707,3 +2707,42 @@ void acpi_setup(void) */ acpi_build_tables_cleanup(&tables, false); } + +void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid, + const CPUArchIdList *apic_ids, GArray *entry) +{ + 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 { + AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic); + + apic->type = ACPI_APIC_LOCAL_X2APIC; + apic->length = sizeof(*apic); + apic->uid = cpu_to_le32(uid); + apic->x2apic_id = cpu_to_le32(apic_id); + if (apic_ids->cpus[uid].cpu != NULL) { + apic->flags = cpu_to_le32(1); + } else { + apic->flags = cpu_to_le32(0); + } + } +}
pc_madt_cpu_entry() is specific to QEMU 'PC' machines, move it to acpi-pc.c. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/i386/acpi-common.c | 40 ---------------------------------------- hw/i386/acpi-pc.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 40 deletions(-)