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

[Beata Michalska]

Hi Andrew,

On Tue, 22 Oct 2019 at 14:43, Andrew Jones <address@hidden> 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
>