Re: [PATCH v16 09/14] numa: Extend CLI to provide memory side cache information

[Tao Xu]

On 11/19/2019 7:47 PM, Igor Mammedov wrote:
> On Fri, 15 Nov 2019 15:53:47 +0800
> Tao Xu <address@hidden> wrote:
>
> > From: Liu Jingqi <address@hidden>
> >
> > 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).
> >
> > Reviewed-by: Daniel Black <address@hidden>
> > Signed-off-by: Liu Jingqi <address@hidden>
> > Signed-off-by: Tao Xu <address@hidden>
> > ---
> >
> > Changes in v16:
> >      - Add cross check with hmat_lb data (Igor)
> >      - Drop total_levels in struct HMAT_Cache_Info (Igor)
> >      - Correct the error table number (Igor)
> >
> > Changes in v15:
> >      - Change the QAPI version tag to 5.0 (Eric)
> >
> > Changes in v13:
> >      - Drop the total_levels option.
> >      - Use readable cache size (Igor)
> > ---
> >   hw/core/numa.c        | 73 ++++++++++++++++++++++++++++++++++++++++
> >   include/sysemu/numa.h | 26 +++++++++++++++
> >   qapi/machine.json     | 78 +++++++++++++++++++++++++++++++++++++++++--
> >   qemu-options.hx       | 16 +++++++--
> >   4 files changed, 189 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/core/numa.c b/hw/core/numa.c
> > index 47c7a96863..4d7af60bfd 100644
> > --- a/hw/core/numa.c
> > +++ b/hw/core/numa.c
> > @@ -359,6 +359,66 @@ void parse_numa_hmat_lb(NumaState *numa_state, 
> > NumaHmatLBOptions *node,
> >       g_array_append_val(hmat_lb->list, lb_data);
> >   }
> >   
> > +void parse_numa_hmat_cache(MachineState *ms, NumaHmatCacheOptions *node,
> > +                           Error **errp)
> > +{
> > +    int nb_numa_nodes = ms->numa_state->num_nodes;
> > +    NodeInfo *numa_info = ms->numa_state->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 (numa_info[node->node_id].lb_info_provided != (BIT(0) | BIT(1))) {
> > +        error_setg(errp, "The latency and bandwidth information of "
> > +                   "node-id=%" PRIu32 " should be provided before configure "
>
> s/configure//
>
> > +                   "memory side cache attributes",
> > +                   node->node_id);
> > +        return;
> > +    }
> > +
> > +    if (node->level >= HMAT_LB_LEVELS) {
> > +        error_setg(errp, "Invalid level=%" PRIu8 ", it should be less than or "
> > +                   "equal to %d", node->level, HMAT_LB_LEVELS - 1);
> > +        return;
> > +    }
> > +    assert(node->assoc < HMAT_CACHE_ASSOCIATIVITY__MAX);
> > +    assert(node->policy < HMAT_CACHE_WRITE_POLICY__MAX);
> > +    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] &&
> will this check work as expected if I define on CLI "L+1" and then after it "L"?

No, I will fix it.
> > +        (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->proximity = node->node_id;
> > +    hmat_cache->size = node->size;
> > +    hmat_cache->level = node->level;
> > +    hmat_cache->associativity = node->assoc;
> > +    hmat_cache->write_policy = node->policy;
> > +    hmat_cache->line_size = node->line;
> > +
> > +    ms->numa_state->hmat_cache[node->node_id][node->level] = hmat_cache;
> > +}
> > +
> >   void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
> >   {
> >       Error *err = NULL;
> > @@ -410,6 +470,19 @@ void set_numa_options(MachineState *ms, NumaOptions 
> > *object, Error **errp)
> >               goto end;
> >           }
> >           break;
> > +    case NUMA_OPTIONS_TYPE_HMAT_CACHE:
> > +        if (!ms->numa_state->hmat_enabled) {
> > +            error_setg(errp, "ACPI Heterogeneous Memory Attribute Table "
> > +                       "(HMAT) is disabled, enable it with -machine hmat=on "
> > +                       "before using any of hmat specific options");
> > +            return;
> > +        }
> > +
> > +        parse_numa_hmat_cache(ms, &object->u.hmat_cache, &err);
> > +        if (err) {
> > +            goto end;
> > +        }
> > +        break;
> >       default:
> >           abort();
> >       }
> > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > index 70f93c83d7..b415550678 100644
> > --- a/include/sysemu/numa.h
> > +++ b/include/sysemu/numa.h
> > @@ -76,6 +76,27 @@ struct HMAT_LB_Info {
> >   };
> >   typedef struct HMAT_LB_Info HMAT_LB_Info;
> >   
> > +struct HMAT_Cache_Info {
> > +    /* The memory proximity domain to which the memory belongs. */
> > +    uint32_t    proximity;
> > +
> > +    /* Size of memory side cache in bytes. */
> > +    uint64_t    size;
> > +
> > +    /* Cache level described in this structure. */
> > +    uint8_t     level;
> > +
> > +    /* Cache Associativity: None/Direct Mapped/Comple Cache Indexing */
> > +    uint8_t     associativity;
> > +
> > +    /* Write Policy: None/Write Back(WB)/Write Through(WT) */
> > +    uint8_t     write_policy;
> > +
> > +    /* Cache Line size in bytes. */
> > +    uint16_t    line_size;
> > +};
> > +typedef struct HMAT_Cache_Info HMAT_Cache_Info;
> Is there a reason why you use this structure instead of QAPI
> generated NumaHmatCacheOptions structure?

Thank you for your suggestion. I will use NumaHmatCacheOptions to 
replace HMAT_Cache_Info.