diff mbox series

[v2,19/23] hw/i386: Move pc_madt_cpu_entry() to acpi-pc.c

Message ID 20210616204328.2611406-20-philmd@redhat.com
State New
Headers show
Series target/i386/sev: Housekeeping (OVMF + SEV-disabled binaries) | expand

Commit Message

Philippe Mathieu-Daudé June 16, 2021, 8:43 p.m. UTC 
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(-)

Comments

Igor Mammedov June 18, 2021, 11:37 a.m. UTC | #1 
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);
> +        }
> +    }
> +}
Philippe Mathieu-Daudé June 19, 2021, 8:45 a.m. UTC | #2 
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.
Michael S. Tsirkin June 19, 2021, 9:32 p.m. UTC | #3 
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 ...
Philippe Mathieu-Daudé June 21, 2021, 8:41 a.m. UTC | #4 
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 mbox series

Patch

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);
+        }
+    }
+}