Re: [PATCH v2 02/11] hw/boards: Introduce machine_class_valid_for_accelerator()

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 2/22/21 6:34 PM, Cornelia Huck wrote:
> On Fri, 19 Feb 2021 18:38:38 +0100
> Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> wrote:
> 
>> Introduce the valid_accelerators[] field to express the list
>> of valid accelators a machine can use, and add the
>> machine_class_valid_for_current_accelerator() and
>> machine_class_valid_for_accelerator() methods.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
>> ---
>>  include/hw/boards.h | 24 ++++++++++++++++++++++++
>>  hw/core/machine.c   | 26 ++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 68d3d10f6b0..4d08bc12093 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -36,6 +36,24 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>                                 const CpuInstanceProperties *props,
>>                                 Error **errp);
>>  
>> +/**
>> + * machine_class_valid_for_accelerator:
>> + * @mc: the machine class
>> + * @acc_name: accelerator name
>> + *
>> + * Returns %true if the accelerator is valid for the machine, %false
>> + * otherwise. See #MachineClass.valid_accelerators.
> 
> Naming confusion: is the machine class valid for the accelerator, or
> the accelerator valid for the machine class? Or either? :)

"the accelerator valid for the machine class".

Is this clearer?

"Returns %true if the current accelerator is valid for the
 selected machine, %false otherwise.

Or...

"Returns %true if the selected accelerator is valid for the
 current machine, %false otherwise.

How would look "either"?

The machine is already selected, and the accelerator too...

> 
>> + */
>> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char 
>> *acc_name);
>> +/**
>> + * machine_class_valid_for_current_accelerator:
>> + * @mc: the machine class
>> + *
>> + * Returns %true if the accelerator is valid for the current machine,
>> + * %false otherwise. See #MachineClass.valid_accelerators.
> 
> Same here: current accelerator vs. current machine.
> 
>> + */
>> +bool machine_class_valid_for_current_accelerator(MachineClass *mc);
>> +
>>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
>> *type);
>>  /*
>>   * Checks that backend isn't used, preps it for exclusive usage and
>> @@ -125,6 +143,11 @@ typedef struct {
>>   *    should instead use "unimplemented-device" for all memory ranges where
>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>   *    implement and a stub device is required.
>> + * @valid_accelerators:
>> + *    If this machine supports a specific set of virtualization 
>> accelerators,
>> + *    this contains a NULL-terminated list of the accelerators that can be
>> + *    used. If this field is not set, any accelerator is valid. The QTest
>> + *    accelerator is always valid.
>>   * @kvm_type:
>>   *    Return the type of KVM corresponding to the kvm-type string option or
>>   *    computed based on other criteria such as the host kernel capabilities
>> @@ -166,6 +189,7 @@ struct MachineClass {
>>      const char *alias;
>>      const char *desc;
>>      const char *deprecation_reason;
>> +    const char *const *valid_accelerators;
>>  
>>      void (*init)(MachineState *state);
>>      void (*reset)(MachineState *state);
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 970046f4388..c42d8e382b1 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -518,6 +518,32 @@ static void machine_set_nvdimm_persistence(Object *obj, 
>> const char *value,
>>      nvdimms_state->persistence_string = g_strdup(value);
>>  }
>>  
>> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char 
>> *acc_name)
>> +{
>> +    const char *const *name = mc->valid_accelerators;
>> +
>> +    if (!name) {
>> +        return true;
>> +    }
>> +    if (strcmp(acc_name, "qtest") == 0) {
>> +        return true;
>> +    }
>> +
>> +    for (unsigned i = 0; name[i]; i++) {
>> +        if (strcasecmp(acc_name, name[i]) == 0) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>> +bool machine_class_valid_for_current_accelerator(MachineClass *mc)
>> +{
>> +    AccelClass *ac = ACCEL_GET_CLASS(current_accel());
>> +
>> +    return machine_class_valid_for_accelerator(mc, ac->name);
>> +}
> 
> The implementation of the function tests for the current accelerator,
> so I think you need to tweak the description above?
> 
>> +
>>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
>> *type)
>>  {
>>      QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
>