| Message ID | 20210219173847.2054123-3-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/accel: Exit gracefully when accelerator is invalid | expand |
On Fri, 19 Feb 2021 18:38:38 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> 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@redhat.com> > --- > 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? :) > + */ > +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));
On 2/22/21 6:34 PM, Cornelia Huck wrote: > On Fri, 19 Feb 2021 18:38:38 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> 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@redhat.com> >> --- >> 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)); >
On Mon, 22 Feb 2021 18:46:15 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 2/22/21 6:34 PM, Cornelia Huck wrote: > > On Fri, 19 Feb 2021 18:38:38 +0100 > > Philippe Mathieu-Daudé <philmd@redhat.com> 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@redhat.com> > >> --- > >> 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. Maybe that one, given how it ends up being called? Or "specified machine"? > > How would look "either"? > > The machine is already selected, and the accelerator too... Yes, so this is basically testing the (machine,accelerator) tuple, which is what I meant with 'either'. > > > > >> + */ > >> +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. So maybe "Returns %true if the current accelerator is valid for the specified machine class..." ? > > > >> + */ > >> +bool machine_class_valid_for_current_accelerator(MachineClass *mc);
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. + */ +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. + */ +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); +} + void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type) { QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
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@redhat.com> --- include/hw/boards.h | 24 ++++++++++++++++++++++++ hw/core/machine.c | 26 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)