Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion

[Andrew Jones]

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