Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
Hi Andrew,
On Tue, 22 Oct 2019 at 14:43, Andrew Jones wrote:
>
> On Mon, Oct 21, 2019 at 04:07:14PM +0100, Beata Michalska wrote:
> > Indeed, the patch got bit messed-up. Apologies for that as well.
> > I have been testing manually but I did try the test you have provided
> > and yes it fails - there is a slight problem with the case when qdict_in
> > is empty,but this can be easily solved still keeping the single loop.
> > Otherwise I have seen you have posted a new patchest so I guess we are
> > dropping the idea of refactoring ?
>
> Well, without a patch that applies, I couldn't really evaluate your
> proposal. And, TBH, I'd rather not hold this series up on a refactoring
> that doesn't provide measurable performance improvements, especially when
> it's not in a performance critical path. Indeed, I'd like to get this
> series merged as soon as possible, which is why I posted v6 with your
> visit_free() fix already.
>
> >
> > One more question: in case of querying a property which is not supported
> > by given cpu model - we are returning properties that are actually valid
> > (the test case for cortex-a15 and aarch64 prop).
> > Shouldn't we return an error there? I honestly must admit I do not know
> > what is the expected behaviour for the qmp query in such cases.
>
> We do generate an error for that case:
>
> (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"}
> {"return": {"model": {"name": "cortex-a15", "props": {"pmu": true
>
> (QEMU) query-cpu-model-expansion type=full
> model={"name":"cortex-a15","props":{"aarch64":false}}
> {"error": {"class": "GenericError", "desc": "Property '.aarch64' not found"}}
>
>
> If you have any more comments on the series, please send them right away.
> I'd like Peter to be able to merge this soon, and I understand that he's
> waiting on your review.
>
I think we can proceed with the v6 as it is.
Thanks a lot.
BR
Beata
> Thanks,
> drew
>
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Mon, Oct 21, 2019 at 04:07:14PM +0100, Beata Michalska wrote:
> Indeed, the patch got bit messed-up. Apologies for that as well.
> I have been testing manually but I did try the test you have provided
> and yes it fails - there is a slight problem with the case when qdict_in
> is empty,but this can be easily solved still keeping the single loop.
> Otherwise I have seen you have posted a new patchest so I guess we are
> dropping the idea of refactoring ?
Well, without a patch that applies, I couldn't really evaluate your
proposal. And, TBH, I'd rather not hold this series up on a refactoring
that doesn't provide measurable performance improvements, especially when
it's not in a performance critical path. Indeed, I'd like to get this
series merged as soon as possible, which is why I posted v6 with your
visit_free() fix already.
>
> One more question: in case of querying a property which is not supported
> by given cpu model - we are returning properties that are actually valid
> (the test case for cortex-a15 and aarch64 prop).
> Shouldn't we return an error there? I honestly must admit I do not know
> what is the expected behaviour for the qmp query in such cases.
We do generate an error for that case:
(QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"}
{"return": {"model": {"name": "cortex-a15", "props": {"pmu": true
(QEMU) query-cpu-model-expansion type=full
model={"name":"cortex-a15","props":{"aarch64":false}}
{"error": {"class": "GenericError", "desc": "Property '.aarch64' not found"}}
If you have any more comments on the series, please send them right away.
I'd like Peter to be able to merge this soon, and I understand that he's
waiting on your review.
Thanks,
drew
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Wed, 16 Oct 2019 at 17:16, Andrew Jones wrote:
>
> On Wed, Oct 16, 2019 at 04:16:57PM +0100, Beata Michalska wrote:
> > On Wed, 16 Oct 2019 at 14:50, Andrew Jones wrote:
> > >
> > > On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote:
> > > > On Tue, 15 Oct 2019 at 12:56, Beata Michalska
> > > > wrote:
> > > > >
> > > > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones wrote:
> > > > > >
> > > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones
> > > > > > > wrote:
> > > > > > > > +
> > > > > > > > +obj = object_new(object_class_get_name(oc));
> > > > > > > > +
> > > > > > > > +if (qdict_in) {
> > > > > > > > +Visitor *visitor;
> > > > > > > > +Error *err = NULL;
> > > > > > > > +
> > > > > > > > +visitor = qobject_input_visitor_new(model->props);
> > > > > > > > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > > > > > +if (err) {
> > > > > > > > +object_unref(obj);
> > > > > > >
> > > > > > > Shouldn't we free the 'visitor' here as well ?
> > > > > >
> > > > > > Yes. Good catch. So we also need to fix
> > > > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> > > > > > construction (the construction from which I derived this)
> > > > > >
> > > > > > >
> > > > > > > > +error_propagate(errp, err);
> > > > > > > > +return NULL;
> > > > > > > > +}
> > > > > > > > +
> > > > > >
> > > > > > What about the rest of the patch? With that fixed for v6 can I
> > > > > > add your r-b?
> > > > > >
> > > > >
> > > > > I still got this feeling that we could optimize that a bit - which I'm
> > > > > currently on, so hopefully I'll be able to add more comments soon if
> > > > > that proves to be the case.
> > > > >
> > > > > BR
> > > > > Beata
> > > >
> > > > I think there are few options that might be considered though the gain
> > > > is not huge .. but it's always smth:
> > > >
> > > > > +CpuModelExpansionInfo
> > > > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> > > > > + CpuModelInfo
> > > > > *model,
> > > > > + Error **errp)
> > > > > +{
> > > > > +CpuModelExpansionInfo *expansion_info;
> > > > > +const QDict *qdict_in = NULL;
> > > > > +QDict *qdict_out;
> > > > > +ObjectClass *oc;
> > > > > +Object *obj;
> > > > > +const char *name;
> > > > > +int i;
> > > > > +
> > > > > +if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> > > > > +error_setg(errp, "The requested expansion type is not
> > > > > supported");
> > > > > +return NULL;
> > > > > +}
> > > > > +
> > > > > +if (!kvm_enabled() && !strcmp(model->name, "host")) {
> > > > > +error_setg(errp, "The CPU type '%s' requires KVM",
> > > > > model->name);
> > > > > +return NULL;
> > > > > +}
> > > > > +
> > > > > +oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> > > > > +if (!oc) {
> > > > > +error_setg(errp, "The CPU type '%s' is not a recognized ARM
> > > > > CPU type",
> > > > > + model->name);
> > > > > +return NULL;
> > > > > +}
> > > > > +
> > > > > +if (kvm_enabled()) {
> > > > > +const char *cpu_type = current_machine->cpu_type;
> > > > > +int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> > > > > +bool supported = false;
> > > > > +
> > > > > +if (!strcmp(model->name, "host") || !strcmp(model->name,
> > > > > "max")) {
> > > > > +/* These are kvmarm's recommended cpu types */
> > > > > +supported = true;
> > > > > +} else if (strlen(model->name) == len &&
> > > > > + !strncmp(model->name, cpu_type, len)) {
> > > > > +/* KVM is enabled and we're using this type, so it
> > > > > works. */
> > > > > +supported = true;
> > > > > +}
> > > > > +if (!supported) {
> > > > > +error_setg(errp, "We cannot guarantee the CPU type '%s'
> > > > > works "
> > > > > + "with KVM on this host", model->name);
> > > > > +return NULL;
> > > > > +}
> > > > > +}
> > > > > +
> > > >
> > > > The above section can be slightly reduced and rearranged - preferably
> > > > moved to a separate function
> > > > -> get_cpu_model (...) ?
> > > >
> > > > * You can check the 'host' model first and then validate the
> > > > accelerator ->
> > > > if ( !strcmp(model->name, "host")
> > > > if (!kvm_enabled())
> > > > log_error & leave
> > > >else
> > > > goto cpu_class_by_name /*cpu_class_by_name moved after the
> > > > final model check @see below */
> > > >
> > > > * the kvm_enabled section can be than slightly improved (dropping the
> > > > second compare against 'host')
> > > >
> > > > if (k
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Wed, Oct 16, 2019 at 04:16:57PM +0100, Beata Michalska wrote:
> On Wed, 16 Oct 2019 at 14:50, Andrew Jones wrote:
> >
> > On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote:
> > > On Tue, 15 Oct 2019 at 12:56, Beata Michalska
> > > wrote:
> > > >
> > > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones wrote:
> > > > >
> > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones
> > > > > > wrote:
> > > > > > > +
> > > > > > > +obj = object_new(object_class_get_name(oc));
> > > > > > > +
> > > > > > > +if (qdict_in) {
> > > > > > > +Visitor *visitor;
> > > > > > > +Error *err = NULL;
> > > > > > > +
> > > > > > > +visitor = qobject_input_visitor_new(model->props);
> > > > > > > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > > > > +if (err) {
> > > > > > > +object_unref(obj);
> > > > > >
> > > > > > Shouldn't we free the 'visitor' here as well ?
> > > > >
> > > > > Yes. Good catch. So we also need to fix
> > > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> > > > > construction (the construction from which I derived this)
> > > > >
> > > > > >
> > > > > > > +error_propagate(errp, err);
> > > > > > > +return NULL;
> > > > > > > +}
> > > > > > > +
> > > > >
> > > > > What about the rest of the patch? With that fixed for v6 can I
> > > > > add your r-b?
> > > > >
> > > >
> > > > I still got this feeling that we could optimize that a bit - which I'm
> > > > currently on, so hopefully I'll be able to add more comments soon if
> > > > that proves to be the case.
> > > >
> > > > BR
> > > > Beata
> > >
> > > I think there are few options that might be considered though the gain
> > > is not huge .. but it's always smth:
> > >
> > > > +CpuModelExpansionInfo
> > > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> > > > + CpuModelInfo
> > > > *model,
> > > > + Error **errp)
> > > > +{
> > > > +CpuModelExpansionInfo *expansion_info;
> > > > +const QDict *qdict_in = NULL;
> > > > +QDict *qdict_out;
> > > > +ObjectClass *oc;
> > > > +Object *obj;
> > > > +const char *name;
> > > > +int i;
> > > > +
> > > > +if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> > > > +error_setg(errp, "The requested expansion type is not
> > > > supported");
> > > > +return NULL;
> > > > +}
> > > > +
> > > > +if (!kvm_enabled() && !strcmp(model->name, "host")) {
> > > > +error_setg(errp, "The CPU type '%s' requires KVM",
> > > > model->name);
> > > > +return NULL;
> > > > +}
> > > > +
> > > > +oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> > > > +if (!oc) {
> > > > +error_setg(errp, "The CPU type '%s' is not a recognized ARM
> > > > CPU type",
> > > > + model->name);
> > > > +return NULL;
> > > > +}
> > > > +
> > > > +if (kvm_enabled()) {
> > > > +const char *cpu_type = current_machine->cpu_type;
> > > > +int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> > > > +bool supported = false;
> > > > +
> > > > +if (!strcmp(model->name, "host") || !strcmp(model->name,
> > > > "max")) {
> > > > +/* These are kvmarm's recommended cpu types */
> > > > +supported = true;
> > > > +} else if (strlen(model->name) == len &&
> > > > + !strncmp(model->name, cpu_type, len)) {
> > > > +/* KVM is enabled and we're using this type, so it works.
> > > > */
> > > > +supported = true;
> > > > +}
> > > > +if (!supported) {
> > > > +error_setg(errp, "We cannot guarantee the CPU type '%s'
> > > > works "
> > > > + "with KVM on this host", model->name);
> > > > +return NULL;
> > > > +}
> > > > +}
> > > > +
> > >
> > > The above section can be slightly reduced and rearranged - preferably
> > > moved to a separate function
> > > -> get_cpu_model (...) ?
> > >
> > > * You can check the 'host' model first and then validate the accelerator
> > > ->
> > > if ( !strcmp(model->name, "host")
> > > if (!kvm_enabled())
> > > log_error & leave
> > >else
> > > goto cpu_class_by_name /*cpu_class_by_name moved after the
> > > final model check @see below */
> > >
> > > * the kvm_enabled section can be than slightly improved (dropping the
> > > second compare against 'host')
> > >
> > > if (kvm_enabled() && strcmp(model->name, "max") {
> > >/*Validate the current_machine->cpu_type against the
> > > model->name and report error case mismatch
> > > /* otherwise just fall through */
> > > }
> > > * cpu_class_by_name moved here ...
> > > > +if (model->props) {
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Wed, 16 Oct 2019 at 14:50, Andrew Jones wrote:
>
> On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote:
> > On Tue, 15 Oct 2019 at 12:56, Beata Michalska
> > wrote:
> > >
> > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones wrote:
> > > >
> > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones wrote:
> > > > > > +
> > > > > > +obj = object_new(object_class_get_name(oc));
> > > > > > +
> > > > > > +if (qdict_in) {
> > > > > > +Visitor *visitor;
> > > > > > +Error *err = NULL;
> > > > > > +
> > > > > > +visitor = qobject_input_visitor_new(model->props);
> > > > > > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > > > +if (err) {
> > > > > > +object_unref(obj);
> > > > >
> > > > > Shouldn't we free the 'visitor' here as well ?
> > > >
> > > > Yes. Good catch. So we also need to fix
> > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> > > > construction (the construction from which I derived this)
> > > >
> > > > >
> > > > > > +error_propagate(errp, err);
> > > > > > +return NULL;
> > > > > > +}
> > > > > > +
> > > >
> > > > What about the rest of the patch? With that fixed for v6 can I
> > > > add your r-b?
> > > >
> > >
> > > I still got this feeling that we could optimize that a bit - which I'm
> > > currently on, so hopefully I'll be able to add more comments soon if
> > > that proves to be the case.
> > >
> > > BR
> > > Beata
> >
> > I think there are few options that might be considered though the gain
> > is not huge .. but it's always smth:
> >
> > > +CpuModelExpansionInfo
> > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> > > + CpuModelInfo *model,
> > > + Error **errp)
> > > +{
> > > +CpuModelExpansionInfo *expansion_info;
> > > +const QDict *qdict_in = NULL;
> > > +QDict *qdict_out;
> > > +ObjectClass *oc;
> > > +Object *obj;
> > > +const char *name;
> > > +int i;
> > > +
> > > +if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> > > +error_setg(errp, "The requested expansion type is not
> > > supported");
> > > +return NULL;
> > > +}
> > > +
> > > +if (!kvm_enabled() && !strcmp(model->name, "host")) {
> > > +error_setg(errp, "The CPU type '%s' requires KVM", model->name);
> > > +return NULL;
> > > +}
> > > +
> > > +oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> > > +if (!oc) {
> > > +error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU
> > > type",
> > > + model->name);
> > > +return NULL;
> > > +}
> > > +
> > > +if (kvm_enabled()) {
> > > +const char *cpu_type = current_machine->cpu_type;
> > > +int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> > > +bool supported = false;
> > > +
> > > +if (!strcmp(model->name, "host") || !strcmp(model->name, "max"))
> > > {
> > > +/* These are kvmarm's recommended cpu types */
> > > +supported = true;
> > > +} else if (strlen(model->name) == len &&
> > > + !strncmp(model->name, cpu_type, len)) {
> > > +/* KVM is enabled and we're using this type, so it works. */
> > > +supported = true;
> > > +}
> > > +if (!supported) {
> > > +error_setg(errp, "We cannot guarantee the CPU type '%s'
> > > works "
> > > + "with KVM on this host", model->name);
> > > +return NULL;
> > > +}
> > > +}
> > > +
> >
> > The above section can be slightly reduced and rearranged - preferably
> > moved to a separate function
> > -> get_cpu_model (...) ?
> >
> > * You can check the 'host' model first and then validate the accelerator ->
> > if ( !strcmp(model->name, "host")
> > if (!kvm_enabled())
> > log_error & leave
> >else
> > goto cpu_class_by_name /*cpu_class_by_name moved after the
> > final model check @see below */
> >
> > * the kvm_enabled section can be than slightly improved (dropping the
> > second compare against 'host')
> >
> > if (kvm_enabled() && strcmp(model->name, "max") {
> >/*Validate the current_machine->cpu_type against the
> > model->name and report error case mismatch
> > /* otherwise just fall through */
> > }
> > * cpu_class_by_name moved here ...
> > > +if (model->props) {
> > MInor: the CPUModelInfo seems to have dedicated field for that
> > verification -> has_props
> >
> > > +qdict_in = qobject_to(QDict, model->props);
> > > +if (!qdict_in) {
> > > +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props",
> > > "dict");
> > > +return NULL;
> > > +}
> > > +}
> > > +
> > > +
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote:
> On Tue, 15 Oct 2019 at 12:56, Beata Michalska
> wrote:
> >
> > On Tue, 15 Oct 2019 at 11:56, Andrew Jones wrote:
> > >
> > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones wrote:
> > > > > +
> > > > > +obj = object_new(object_class_get_name(oc));
> > > > > +
> > > > > +if (qdict_in) {
> > > > > +Visitor *visitor;
> > > > > +Error *err = NULL;
> > > > > +
> > > > > +visitor = qobject_input_visitor_new(model->props);
> > > > > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > > +if (err) {
> > > > > +object_unref(obj);
> > > >
> > > > Shouldn't we free the 'visitor' here as well ?
> > >
> > > Yes. Good catch. So we also need to fix
> > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> > > construction (the construction from which I derived this)
> > >
> > > >
> > > > > +error_propagate(errp, err);
> > > > > +return NULL;
> > > > > +}
> > > > > +
> > >
> > > What about the rest of the patch? With that fixed for v6 can I
> > > add your r-b?
> > >
> >
> > I still got this feeling that we could optimize that a bit - which I'm
> > currently on, so hopefully I'll be able to add more comments soon if
> > that proves to be the case.
> >
> > BR
> > Beata
>
> I think there are few options that might be considered though the gain
> is not huge .. but it's always smth:
>
> > +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType
> > type,
> > + CpuModelInfo *model,
> > + Error **errp)
> > +{
> > +CpuModelExpansionInfo *expansion_info;
> > +const QDict *qdict_in = NULL;
> > +QDict *qdict_out;
> > +ObjectClass *oc;
> > +Object *obj;
> > +const char *name;
> > +int i;
> > +
> > +if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> > +error_setg(errp, "The requested expansion type is not supported");
> > +return NULL;
> > +}
> > +
> > +if (!kvm_enabled() && !strcmp(model->name, "host")) {
> > +error_setg(errp, "The CPU type '%s' requires KVM", model->name);
> > +return NULL;
> > +}
> > +
> > +oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> > +if (!oc) {
> > +error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU
> > type",
> > + model->name);
> > +return NULL;
> > +}
> > +
> > +if (kvm_enabled()) {
> > +const char *cpu_type = current_machine->cpu_type;
> > +int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> > +bool supported = false;
> > +
> > +if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) {
> > +/* These are kvmarm's recommended cpu types */
> > +supported = true;
> > +} else if (strlen(model->name) == len &&
> > + !strncmp(model->name, cpu_type, len)) {
> > +/* KVM is enabled and we're using this type, so it works. */
> > +supported = true;
> > +}
> > +if (!supported) {
> > +error_setg(errp, "We cannot guarantee the CPU type '%s' works "
> > + "with KVM on this host", model->name);
> > +return NULL;
> > +}
> > +}
> > +
>
> The above section can be slightly reduced and rearranged - preferably
> moved to a separate function
> -> get_cpu_model (...) ?
>
> * You can check the 'host' model first and then validate the accelerator ->
> if ( !strcmp(model->name, "host")
> if (!kvm_enabled())
> log_error & leave
>else
> goto cpu_class_by_name /*cpu_class_by_name moved after the
> final model check @see below */
>
> * the kvm_enabled section can be than slightly improved (dropping the
> second compare against 'host')
>
> if (kvm_enabled() && strcmp(model->name, "max") {
>/*Validate the current_machine->cpu_type against the
> model->name and report error case mismatch
> /* otherwise just fall through */
> }
> * cpu_class_by_name moved here ...
> > +if (model->props) {
> MInor: the CPUModelInfo seems to have dedicated field for that
> verification -> has_props
>
> > +qdict_in = qobject_to(QDict, model->props);
> > +if (!qdict_in) {
> > +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> > +return NULL;
> > +}
> > +}
> > +
> > +obj = object_new(object_class_get_name(oc));
> > +
> > +if (qdict_in) {
> > +Visitor *visitor;
> > +Error *err = NULL;
> > +
> > +visitor = qobject_input_visitor_new(model->props);
> > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > +if (err) {
> > +object_unref(obj);
> >
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Tue, 15 Oct 2019 at 12:56, Beata Michalska
wrote:
>
> On Tue, 15 Oct 2019 at 11:56, Andrew Jones wrote:
> >
> > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones wrote:
> > > > +
> > > > +obj = object_new(object_class_get_name(oc));
> > > > +
> > > > +if (qdict_in) {
> > > > +Visitor *visitor;
> > > > +Error *err = NULL;
> > > > +
> > > > +visitor = qobject_input_visitor_new(model->props);
> > > > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > +if (err) {
> > > > +object_unref(obj);
> > >
> > > Shouldn't we free the 'visitor' here as well ?
> >
> > Yes. Good catch. So we also need to fix
> > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> > construction (the construction from which I derived this)
> >
> > >
> > > > +error_propagate(errp, err);
> > > > +return NULL;
> > > > +}
> > > > +
> >
> > What about the rest of the patch? With that fixed for v6 can I
> > add your r-b?
> >
>
> I still got this feeling that we could optimize that a bit - which I'm
> currently on, so hopefully I'll be able to add more comments soon if
> that proves to be the case.
>
> BR
> Beata
I think there are few options that might be considered though the gain
is not huge .. but it's always smth:
> +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType
> type,
> + CpuModelInfo *model,
> + Error **errp)
> +{
> +CpuModelExpansionInfo *expansion_info;
> +const QDict *qdict_in = NULL;
> +QDict *qdict_out;
> +ObjectClass *oc;
> +Object *obj;
> +const char *name;
> +int i;
> +
> +if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> +error_setg(errp, "The requested expansion type is not supported");
> +return NULL;
> +}
> +
> +if (!kvm_enabled() && !strcmp(model->name, "host")) {
> +error_setg(errp, "The CPU type '%s' requires KVM", model->name);
> +return NULL;
> +}
> +
> +oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> +if (!oc) {
> +error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU
> type",
> + model->name);
> +return NULL;
> +}
> +
> +if (kvm_enabled()) {
> +const char *cpu_type = current_machine->cpu_type;
> +int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> +bool supported = false;
> +
> +if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) {
> +/* These are kvmarm's recommended cpu types */
> +supported = true;
> +} else if (strlen(model->name) == len &&
> + !strncmp(model->name, cpu_type, len)) {
> +/* KVM is enabled and we're using this type, so it works. */
> +supported = true;
> +}
> +if (!supported) {
> +error_setg(errp, "We cannot guarantee the CPU type '%s' works "
> + "with KVM on this host", model->name);
> +return NULL;
> +}
> +}
> +
The above section can be slightly reduced and rearranged - preferably
moved to a separate function
-> get_cpu_model (...) ?
* You can check the 'host' model first and then validate the accelerator ->
if ( !strcmp(model->name, "host")
if (!kvm_enabled())
log_error & leave
else
goto cpu_class_by_name /*cpu_class_by_name moved after the
final model check @see below */
* the kvm_enabled section can be than slightly improved (dropping the
second compare against 'host')
if (kvm_enabled() && strcmp(model->name, "max") {
/*Validate the current_machine->cpu_type against the
model->name and report error case mismatch
/* otherwise just fall through */
}
* cpu_class_by_name moved here ...
> +if (model->props) {
MInor: the CPUModelInfo seems to have dedicated field for that
verification -> has_props
> +qdict_in = qobject_to(QDict, model->props);
> +if (!qdict_in) {
> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> +return NULL;
> +}
> +}
> +
> +obj = object_new(object_class_get_name(oc));
> +
> +if (qdict_in) {
> +Visitor *visitor;
> +Error *err = NULL;
> +
> +visitor = qobject_input_visitor_new(model->props);
> +visit_start_struct(visitor, NULL, NULL, 0, &err);
> +if (err) {
> +object_unref(obj);
> +error_propagate(errp, err);
> +return NULL;
> +}
> +
> +i = 0;
> +while ((name = cpu_model_advertised_features[i++]) != NULL) {
> +if (qdict_get(qdict_in, name)) {
> +object_property_set(obj, visitor, name, &err);
> +if (err) {
> +
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Tue, 15 Oct 2019 at 11:56, Andrew Jones wrote:
>
> On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > On Tue, 1 Oct 2019 at 14:04, Andrew Jones wrote:
> > > +
> > > +obj = object_new(object_class_get_name(oc));
> > > +
> > > +if (qdict_in) {
> > > +Visitor *visitor;
> > > +Error *err = NULL;
> > > +
> > > +visitor = qobject_input_visitor_new(model->props);
> > > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > +if (err) {
> > > +object_unref(obj);
> >
> > Shouldn't we free the 'visitor' here as well ?
>
> Yes. Good catch. So we also need to fix
> target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> construction (the construction from which I derived this)
>
> >
> > > +error_propagate(errp, err);
> > > +return NULL;
> > > +}
> > > +
>
> What about the rest of the patch? With that fixed for v6 can I
> add your r-b?
>
I still got this feeling that we could optimize that a bit - which I'm
currently on, so hopefully I'll be able to add more comments soon if
that proves to be the case.
BR
Beata
> Thanks,
> drew
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> On Tue, 1 Oct 2019 at 14:04, Andrew Jones wrote:
> > +
> > +obj = object_new(object_class_get_name(oc));
> > +
> > +if (qdict_in) {
> > +Visitor *visitor;
> > +Error *err = NULL;
> > +
> > +visitor = qobject_input_visitor_new(model->props);
> > +visit_start_struct(visitor, NULL, NULL, 0, &err);
> > +if (err) {
> > +object_unref(obj);
>
> Shouldn't we free the 'visitor' here as well ?
Yes. Good catch. So we also need to fix
target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
construction (the construction from which I derived this)
>
> > +error_propagate(errp, err);
> > +return NULL;
> > +}
> > +
What about the rest of the patch? With that fixed for v6 can I
add your r-b?
Thanks,
drew
Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
On Tue, 1 Oct 2019 at 14:04, Andrew Jones wrote:
>
> Add support for the query-cpu-model-expansion QMP command to Arm. We
> do this selectively, only exposing CPU properties which represent
> optional CPU features which the user may want to enable/disable.
> Additionally we restrict the list of queryable cpu models to 'max',
> 'host', or the current type when KVM is in use. And, finally, we only
> implement expansion type 'full', as Arm does not yet have a "base"
> CPU type. More details and example queries are described in a new
> document (docs/arm-cpu-features.rst).
>
> Note, certainly more features may be added to the list of advertised
> features, e.g. 'vfp' and 'neon'. The only requirement is that we can
> detect invalid configurations and emit failures at QMP query time.
> For 'vfp' and 'neon' this will require some refactoring to share a
> validation function between the QMP query and the CPU realize
> functions.
>
> Signed-off-by: Andrew Jones
> Reviewed-by: Richard Henderson
> Reviewed-by: Eric Auger
> ---
> docs/arm-cpu-features.rst | 137 +++
> qapi/machine-target.json | 6 +-
> target/arm/monitor.c | 145 ++
> 3 files changed, 285 insertions(+), 3 deletions(-)
> create mode 100644 docs/arm-cpu-features.rst
>
> diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
> new file mode 100644
> index ..c79dcffb5556
> --- /dev/null
> +++ b/docs/arm-cpu-features.rst
> @@ -0,0 +1,137 @@
> +
> +ARM CPU Features
> +
> +
> +Examples of probing and using ARM CPU features
> +
> +Introduction
> +
> +
> +CPU features are optional features that a CPU of supporting type may
> +choose to implement or not. In QEMU, optional CPU features have
> +corresponding boolean CPU proprieties that, when enabled, indicate
> +that the feature is implemented, and, conversely, when disabled,
> +indicate that it is not implemented. An example of an ARM CPU feature
> +is the Performance Monitoring Unit (PMU). CPU types such as the
> +Cortex-A15 and the Cortex-A57, which respectively implement ARM
> +architecture reference manuals ARMv7-A and ARMv8-A, may both optionally
> +implement PMUs. For example, if a user wants to use a Cortex-A15 without
> +a PMU, then the `-cpu` parameter should contain `pmu=off` on the QEMU
> +command line, i.e. `-cpu cortex-a15,pmu=off`.
> +
> +As not all CPU types support all optional CPU features, then whether or
> +not a CPU property exists depends on the CPU type. For example, CPUs
> +that implement the ARMv8-A architecture reference manual may optionally
> +support the AArch32 CPU feature, which may be enabled by disabling the
> +`aarch64` CPU property. A CPU type such as the Cortex-A15, which does
> +not implement ARMv8-A, will not have the `aarch64` CPU property.
> +
> +QEMU's support may be limited for some CPU features, only partially
> +supporting the feature or only supporting the feature under certain
> +configurations. For example, the `aarch64` CPU feature, which, when
> +disabled, enables the optional AArch32 CPU feature, is only supported
> +when using the KVM accelerator and when running on a host CPU type that
> +supports the feature.
> +
> +CPU Feature Probing
> +===
> +
> +Determining which CPU features are available and functional for a given
> +CPU type is possible with the `query-cpu-model-expansion` QMP command.
> +Below are some examples where `scripts/qmp/qmp-shell` (see the top comment
> +block in the script for usage) is used to issue the QMP commands.
> +
> +(1) Determine which CPU features are available for the `max` CPU type
> +(Note, we started QEMU with qemu-system-aarch64, so `max` is
> + implementing the ARMv8-A reference manual in this case)::
> +
> + (QEMU) query-cpu-model-expansion type=full model={"name":"max"}
> + { "return": {
> +"model": { "name": "max", "props": {
> +"pmu": true, "aarch64": true
> +
> +
> +We see that the `max` CPU type has the `pmu` and `aarch64` CPU features.
> +We also see that the CPU features are enabled, as they are all `true`.
> +
> +(2) Let's try to disable the PMU::
> +
> + (QEMU) query-cpu-model-expansion type=full
> model={"name":"max","props":{"pmu":false}}
> + { "return": {
> +"model": { "name": "max", "props": {
> +"pmu": false, "aarch64": true
> +
> +
> +We see it worked, as `pmu` is now `false`.
> +
> +(3) Let's try to disable `aarch64`, which enables the AArch32 CPU feature::
> +
> + (QEMU) query-cpu-model-expansion type=full
> model={"name":"max","props":{"aarch64":false}}
> + {"error": {
> + "class": "GenericError", "desc":
> + "'aarch64' feature cannot be disabled unless KVM is enabled and
> 32-bit EL1 is supported"
> + }}
> +
> +It looks like this feature is limited to a configuration we do not
> +currently have.
> +
> +(4) Let's try probing CPU f
[PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion
Add support for the query-cpu-model-expansion QMP command to Arm. We
do this selectively, only exposing CPU properties which represent
optional CPU features which the user may want to enable/disable.
Additionally we restrict the list of queryable cpu models to 'max',
'host', or the current type when KVM is in use. And, finally, we only
implement expansion type 'full', as Arm does not yet have a "base"
CPU type. More details and example queries are described in a new
document (docs/arm-cpu-features.rst).
Note, certainly more features may be added to the list of advertised
features, e.g. 'vfp' and 'neon'. The only requirement is that we can
detect invalid configurations and emit failures at QMP query time.
For 'vfp' and 'neon' this will require some refactoring to share a
validation function between the QMP query and the CPU realize
functions.
Signed-off-by: Andrew Jones
Reviewed-by: Richard Henderson
Reviewed-by: Eric Auger
---
docs/arm-cpu-features.rst | 137 +++
qapi/machine-target.json | 6 +-
target/arm/monitor.c | 145 ++
3 files changed, 285 insertions(+), 3 deletions(-)
create mode 100644 docs/arm-cpu-features.rst
diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
new file mode 100644
index ..c79dcffb5556
--- /dev/null
+++ b/docs/arm-cpu-features.rst
@@ -0,0 +1,137 @@
+
+ARM CPU Features
+
+
+Examples of probing and using ARM CPU features
+
+Introduction
+
+
+CPU features are optional features that a CPU of supporting type may
+choose to implement or not. In QEMU, optional CPU features have
+corresponding boolean CPU proprieties that, when enabled, indicate
+that the feature is implemented, and, conversely, when disabled,
+indicate that it is not implemented. An example of an ARM CPU feature
+is the Performance Monitoring Unit (PMU). CPU types such as the
+Cortex-A15 and the Cortex-A57, which respectively implement ARM
+architecture reference manuals ARMv7-A and ARMv8-A, may both optionally
+implement PMUs. For example, if a user wants to use a Cortex-A15 without
+a PMU, then the `-cpu` parameter should contain `pmu=off` on the QEMU
+command line, i.e. `-cpu cortex-a15,pmu=off`.
+
+As not all CPU types support all optional CPU features, then whether or
+not a CPU property exists depends on the CPU type. For example, CPUs
+that implement the ARMv8-A architecture reference manual may optionally
+support the AArch32 CPU feature, which may be enabled by disabling the
+`aarch64` CPU property. A CPU type such as the Cortex-A15, which does
+not implement ARMv8-A, will not have the `aarch64` CPU property.
+
+QEMU's support may be limited for some CPU features, only partially
+supporting the feature or only supporting the feature under certain
+configurations. For example, the `aarch64` CPU feature, which, when
+disabled, enables the optional AArch32 CPU feature, is only supported
+when using the KVM accelerator and when running on a host CPU type that
+supports the feature.
+
+CPU Feature Probing
+===
+
+Determining which CPU features are available and functional for a given
+CPU type is possible with the `query-cpu-model-expansion` QMP command.
+Below are some examples where `scripts/qmp/qmp-shell` (see the top comment
+block in the script for usage) is used to issue the QMP commands.
+
+(1) Determine which CPU features are available for the `max` CPU type
+(Note, we started QEMU with qemu-system-aarch64, so `max` is
+ implementing the ARMv8-A reference manual in this case)::
+
+ (QEMU) query-cpu-model-expansion type=full model={"name":"max"}
+ { "return": {
+"model": { "name": "max", "props": {
+"pmu": true, "aarch64": true
+
+
+We see that the `max` CPU type has the `pmu` and `aarch64` CPU features.
+We also see that the CPU features are enabled, as they are all `true`.
+
+(2) Let's try to disable the PMU::
+
+ (QEMU) query-cpu-model-expansion type=full
model={"name":"max","props":{"pmu":false}}
+ { "return": {
+"model": { "name": "max", "props": {
+"pmu": false, "aarch64": true
+
+
+We see it worked, as `pmu` is now `false`.
+
+(3) Let's try to disable `aarch64`, which enables the AArch32 CPU feature::
+
+ (QEMU) query-cpu-model-expansion type=full
model={"name":"max","props":{"aarch64":false}}
+ {"error": {
+ "class": "GenericError", "desc":
+ "'aarch64' feature cannot be disabled unless KVM is enabled and 32-bit
EL1 is supported"
+ }}
+
+It looks like this feature is limited to a configuration we do not
+currently have.
+
+(4) Let's try probing CPU features for the Cortex-A15 CPU type::
+
+ (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"}
+ {"return": {"model": {"name": "cortex-a15", "props": {"pmu": true
+
+Only the `pmu` CPU feature is available.
+
+A note about CPU feature dependen
