Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:
> Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> to get number of CPUs present at boot. However 1 byte is
> not enough to handle more than 255 CPUs.  So add a new
> fw_cfg file that would allow QEMU to tell it.
> For compat reasons add file only for machine types that
> support more than 255 CPUs.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
[...]
>  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
>                            Error **errp)
>  {
> @@ -1232,6 +1221,11 @@ static void 
> pc_build_feature_control_file(PCMachineState *pcms)
>      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, 
> sizeof(*val));
>  }
>  
> +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> +{
> +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);

If we have more than 255 CPUs, shouldn't we at least tell the old
BIOS that we have 255, instead of silently truncating bits?

> +}
> +
>  static
>  void pc_machine_done(Notifier *notifier, void *data)
>  {
> @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>      PCIBus *bus = pcms->bus;
>  
>      /* set the number of CPUs */
> -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
>  
>      if (bus) {
>          int extra_hosts = 0;
> @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
>  
>      acpi_setup();
>      if (pcms->fw_cfg) {
> +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> +
>          pc_build_smbios(pcms->fw_cfg);
>          pc_build_feature_control_file(pcms);
> +
> +        if (mc->max_cpus > 255) {
> +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", 
> &pcms->boot_cpus_le,
> +                            sizeof(pcms->boot_cpus_le));
> +        }
>      }
>  }
>  
> @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>          }
>      }
>  
> +    /* increment the number of CPUs */
> +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);

Is this really safe? What if the guest is in the middle of a
etc/boot-cpus read?

>      if (dev->hotplugged) {
> -        /* increment the number of CPUs */
> -        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> +        /* Update the number of CPUs in CMOS */
> +        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
>      }
>  
>      found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> @@ -1842,7 +1845,10 @@ static void pc_cpu_unplug_cb(HotplugHandler 
> *hotplug_dev,
>      found_cpu->cpu = NULL;
>      object_unparent(OBJECT(dev));
>  
> -    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
> +    /* decrement the number of CPUs */
> +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1);
> +    /* Update the number of CPUs in CMOS */
> +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
>   out:
>      error_propagate(errp, local_err);
>  }
> -- 
> 2.7.4
> 

-- 
Eduardo