Re: [PATCH v8 4/8] hw/acpi/aml-build: Add Processor hierarchy node structure

[wangyanan (Y)]

On 2021/10/20 18:04, Michael S. Tsirkin wrote:
> On Wed, Oct 20, 2021 at 05:51:46PM +0800, wangyanan (Y) wrote:
> > On 2021/10/20 15:43, Eric Auger wrote:
> > > Hi
> > >
> > > On 10/14/21 3:22 PM, Yanan Wang wrote:
> > > > Add a generic API to build Processor hierarchy node structure (Type 0),
> > > > which is strictly consistent with descriptions in ACPI 6.2: 5.2.29.1.
> > > >
> > > > This function will be used to build ACPI PPTT table for cpu topology.
> > > >
> > > > Co-developed-by: Ying Fang <fangying1@huawei.com>
> > > > Co-developed-by: Henglong Fan <fanhenglong@huawei.com>
> > > > Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> > > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >    hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
> > > >    include/hw/acpi/aml-build.h |  4 ++++
> > > >    2 files changed, 30 insertions(+)
> > > >
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > > index 76af0ebaf9..b7b9db6888 100644
> > > > --- a/hw/acpi/aml-build.c
> > > > +++ b/hw/acpi/aml-build.c
> > > > @@ -1964,6 +1964,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, 
> > > > MachineState *ms,
> > > >        acpi_table_end(linker, &table);
> > > >    }
> > > > +/* ACPI 6.2: 5.2.29.1 Processor hierarchy node structure (Type 0) */
> > > > +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> > > > +                                    uint32_t parent, uint32_t id,
> > > > +                                    uint32_t *priv_rsrc, uint32_t priv_num)
> > > > +{
> > > > +    int i;
> > > > +
> > > > +    build_append_byte(tbl, 0);                 /* Type 0 - processor */
> > > nit: I think Igor would generally prefer
> > > build_append_byte(tbl, 0 /* processor structure */);  /* Type */
> > > although it generates checkpatch warnings
> > Ok.
> I think it's ok as is. Right Igor?
Understood.

Thanks,
Yanan
.
> > > > +    build_append_byte(tbl, 20 + priv_num * 4); /* Length */
> > > > +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
> > > > +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
> > > > +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
> > > > +    build_append_int_noprefix(tbl, id, 4);     /* ACPI Processor ID */
> > > > +
> > > > +    /* Number of private resources */
> > > > +    build_append_int_noprefix(tbl, priv_num, 4);
> > > > +
> > > > +    /* Private resources[N] */
> > > > +    if (priv_num > 0) {
> > > > +        assert(priv_rsrc);
> > > > +        for (i = 0; i < priv_num; i++) {
> > > > +            build_append_int_noprefix(tbl, priv_rsrc[i], 4);
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > >    /* build rev1/rev3/rev5.1 FADT */
> > > >    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> > > >                    const char *oem_id, const char *oem_table_id)
> > > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > > index 3cf6f2c1b9..2c457c8f17 100644
> > > > --- a/include/hw/acpi/aml-build.h
> > > > +++ b/include/hw/acpi/aml-build.h
> > > > @@ -489,6 +489,10 @@ void build_srat_memory(GArray *table_data, uint64_t base,
> > > >    void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> > > >                    const char *oem_id, const char *oem_table_id);
> > > > +void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> > > > +                                    uint32_t parent, uint32_t id,
> > > > +                                    uint32_t *priv_rsrc, uint32_t priv_num);
> > > > +
> > > >    void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> > > >                    const char *oem_id, const char *oem_table_id);
> > > Besides
> > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > Thanks for your review!
> >
> > Yanan
> > .
> > > Eric
> > >
> > > .
> .