Re: [Qemu-devel] [PATCH v4 09/11] numa: Extend the command-line to provide memory side cache information

[Igor Mammedov <imammedo@redhat.com>]

On Wed,  8 May 2019 14:17:24 +0800
Tao Xu <tao3.xu@intel.com> wrote:

> From: Liu Jingqi <jingqi.liu@intel.com>
> 
> Add -numa hmat-cache option to provide Memory Side Cache Information.
> These memory attributes help to build Memory Side Cache Information
> Structure(s) in ACPI Heterogeneous Memory Attribute Table (HMAT).
> 
> Signed-off-by: Liu Jingqi <jingqi.liu@intel.com>
> Signed-off-by: Tao Xu <tao3.xu@intel.com>
> ---
> 
> Changes in v4 -> v3:
>     - update the version tag from 4.0 to 4.1
> ---
>  numa.c         | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/misc.json | 72 ++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 145 insertions(+), 2 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index 1aecb7a2e9..4866736fc8 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -300,6 +300,75 @@ static void parse_numa_hmat_lb(MachineState *ms, NumaHmatLBOptions *node,
>      }
>  }
>  
> +static
> +void parse_numa_hmat_cache(MachineState *ms, NumaHmatCacheOptions *node,
> +                            Error **errp)
> +{
> +    int nb_numa_nodes = ms->numa_state->num_nodes;
> +    HMAT_Cache_Info *hmat_cache = NULL;
> +
> +    if (node->node_id >= nb_numa_nodes) {
> +        error_setg(errp, "Invalid node-id=%" PRIu32
> +                   ", it should be less than %d.",
> +                   node->node_id, nb_numa_nodes);
> +        return;
> +    }
> +    if (!ms->numa_state->nodes[node->node_id].is_target) {
> +        error_setg(errp, "Invalid node-id=%" PRIu32
> +                   ", it isn't a target proximity domain.",
> +                   node->node_id);
> +        return;
> +    }
> +
> +    if (node->total > MAX_HMAT_CACHE_LEVEL) {
> +        error_setg(errp, "Invalid total=%" PRIu8
> +                   ", it should be less than or equal to %d.",
> +                   node->total, MAX_HMAT_CACHE_LEVEL);
> +        return;
> +    }
> +    if (node->level > node->total) {
> +        error_setg(errp, "Invalid level=%" PRIu8
> +                   ", it should be less than or equal to"
> +                   " total=%" PRIu8 ".",
> +                   node->level, node->total);
> +        return;
> +    }
> +    if (ms->numa_state->hmat_cache[node->node_id][node->level]) {
> +        error_setg(errp, "Duplicate configuration of the side cache for "
> +                   "node-id=%" PRIu32 " and level=%" PRIu8 ".",
> +                   node->node_id, node->level);
> +        return;
> +    }
> +
> +    if ((node->level > 1) &&
> +        ms->numa_state->hmat_cache[node->node_id][node->level - 1] &&
> +        (node->size >=
> +            ms->numa_state->hmat_cache[node->node_id][node->level - 1]->size)) {
> +        error_setg(errp, "Invalid size=0x%" PRIx64
> +                   ", the size of level=%" PRIu8
> +                   " should be less than the size(0x%" PRIx64
> +                   ") of level=%" PRIu8 ".",
> +                   node->size, node->level,
> +                   ms->numa_state->hmat_cache[node->node_id]
> +                                             [node->level - 1]->size,
> +                   node->level - 1);
> +        return;
> +    }
> +
> +    hmat_cache = g_malloc0(sizeof(*hmat_cache));
> +
> +    hmat_cache->mem_proximity = node->node_id;
> +    hmat_cache->size = node->size;
> +    hmat_cache->total_levels = node->total;
> +    hmat_cache->level = node->level;
> +    hmat_cache->associativity = node->assoc;
> +    hmat_cache->write_policy = node->policy;
> +    hmat_cache->line_size = node->line;
> +    hmat_cache->num_smbios_handles = 0;
> +
> +    ms->numa_state->hmat_cache[node->node_id][node->level] = hmat_cache;
> +}
> +
>  static
>  void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
>  {
> @@ -344,6 +413,12 @@ void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
>              goto end;
>          }
>          break;
> +    case NUMA_OPTIONS_TYPE_HMAT_CACHE:
> +        parse_numa_hmat_cache(ms, &object->u.hmat_cache, &err);
> +        if (err) {
> +            goto end;
> +        }
> +        break;
>      default:
>          abort();
>      }
> diff --git a/qapi/misc.json b/qapi/misc.json
> index d7fce75702..2b7e34b469 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -2541,10 +2541,12 @@
>  #
>  # @hmat-lb: memory latency and bandwidth information (Since: 4.1)
>  #
> +# @hmat-cache: memory side cache information (Since: 4.1)
> +#
>  # Since: 2.1
>  ##
>  { 'enum': 'NumaOptionsType',
> -  'data': [ 'node', 'dist', 'cpu', 'hmat-lb' ] }
> +   'data': [ 'node', 'dist', 'cpu', 'hmat-lb', 'hmat-cache' ] }
stray whitespace in front???


>  ##
>  # @NumaOptions:
> @@ -2560,7 +2562,8 @@
>      'node': 'NumaNodeOptions',
>      'dist': 'NumaDistOptions',
>      'cpu': 'NumaCpuOptions',
> -    'hmat-lb': 'NumaHmatLBOptions' }}
> +    'hmat-lb': 'NumaHmatLBOptions',
> +    'hmat-cache': 'NumaHmatCacheOptions' }}
>  
>  ##
>  # @NumaNodeOptions:
> @@ -2710,6 +2713,71 @@
>     '*latency': 'uint16',
>     '*bandwidth': 'uint16' }}
>  
> +##
> +# @HmatCacheAssociativity:
> +#
> +# Cache associativity in the Memory Side Cache
> +# Information Structure of HMAT
> +#
> +# @none: None
> +#
> +# @direct: Direct Mapped
> +#
> +# @complex: Complex Cache Indexing (implementation specific)
it would be good to add reference to spec, like we do for ACPI API functions.
So that reader would know where to look for values and their meaning.

PS:
It applies to all fields that come from spec (in this and previous patches that add QAPI structures)

> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'HmatCacheAssociativity',
> +  'data': [ 'none', 'direct', 'complex' ] }
> +
> +##
> +# @HmatCacheWritePolicy:
> +#
> +# Cache write policy in the Memory Side Cache
> +# Information Structure of HMAT
> +#
> +# @none: None
> +#
> +# @write-back: Write Back (WB)
> +#
> +# @write-through: Write Through (WT)
> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'HmatCacheWritePolicy',
> +  'data': [ 'none', 'write-back', 'write-through' ] }
> +
> +##
> +# @NumaHmatCacheOptions:
> +#
> +# Set the memory side cache information for a given memory domain.
> +#
> +# @node-id: the memory proximity domain to which the memory belongs.
> +#
> +# @size: the size of memory side cache in bytes.
> +#
> +# @total: the total cache levels for this memory proximity domain.
> +#
> +# @level: the cache level described in this structure.
> +#
> +# @assoc: the cache associativity, none/direct-mapped/complex(complex cache indexing).
> +
> +# @policy: the write policy, none/write-back/write-through.
> +#
> +# @line: the cache Line size in bytes.
> +#
> +# Since: 4.1
> +##
> +{ 'struct': 'NumaHmatCacheOptions',
> +  'data': {
> +   'node-id': 'uint32',
> +   'size': 'size',
> +   'total': 'uint8',
> +   'level': 'uint8',
> +   'assoc': 'HmatCacheAssociativity',
> +   'policy': 'HmatCacheWritePolicy',
> +   'line': 'uint16' }}
> +
>  ##
>  # @HostMemPolicy:
>  #