Re: [PATCH v2 2/7] qapi/qom: Define cache enumeration and properties

[Zhao Liu <zhao1.liu@intel.com>]

Hi Jonathan,

Thanks for your review and feedback!

[snip]

> > Note, define cache topology based on CPU topology level with two
> > reasons:
> > 
> >  1. In practice, a cache will always be bound to the CPU container
> >     (either private in the CPU container or shared among multiple
> >     containers), and CPU container is often expressed in terms of CPU
> >     topology level.
> >  2. The x86's cache-related CPUIDs encode cache topology based on APIC
> >     ID's CPU topology layout. And the ACPI PPTT table that ARM/RISCV
> >     relies on also requires CPU containers to help indicate the private
> 
> Really trivial but CPU Containers are a different ACPI concept.
> For PPTT they are referred to as Processor Groups. Wonderfully they
> 'might match a Processor Container in the namespace' which rather implies
> they might not.  In QEMU they always will because the next bit of the
> spec matters. "In that case this entry will match the value of the _UID
> method of the associated processor container. Where there is a match it must
> be represented."
> 
> So having said all that, CPU container is probably fine as a description.

Thanks for the explanation!

> >     shared hierarchy of the cache. Therefore, for SMP systems, it is
> >     natural to use the CPU topology hierarchy directly in QEMU to define
> >     the cache topology.
> > 
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
>
> Seems fine but my gut would be to combine this and next patch so we can
> see how it is used (assuming no one asked for it to be separate!)

No problem. I intended to make it easier to review the QAPI part, but
these two patches were simple enough that I was happy to combine them.

> Version numbers need an update I guess.

Ah, yes!

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

> > +##
> > +# @SmpCachePropertiesWrapper:
> > +#
> > +# List wrapper of SmpCacheProperties.
> > +#
> > +# @caches: the list of SmpCacheProperties.
> > +#
> > +# Since 9.1
> 
> Needs updating to 9.2 I guess.

Yes, I think so, too.

Thanks,
Zhao

> > +##
> > +{ 'struct': 'SmpCachePropertiesWrapper',
> > +  'data': { 'caches': ['SmpCacheProperties'] } }
>