Re: [Xen-devel] [PATCH 12/19] hvmloader: construct SRAT

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Nov 24, 2014 at 10:08:42AM +0000, Jan Beulich wrote:
> >>> On 21.11.14 at 16:06, <wei.liu2@xxxxxxxxxx> wrote:
> > --- a/tools/firmware/hvmloader/acpi/build.c
> > +++ b/tools/firmware/hvmloader/acpi/build.c
> > @@ -203,6 +203,66 @@ static struct acpi_20_waet *construct_waet(void)
> >      return waet;
> >  }
> >  
> > +static struct acpi_20_srat *construct_srat(void)
> > +{
> > +    struct acpi_20_srat *srat;
> > +    struct acpi_20_srat_processor *processor;
> > +    struct acpi_20_srat_memory *memory;
> > +    unsigned int size;
> > +    void *p;
> > +    int i;
> > +    uint64_t mem;
> > +
> > +    size = sizeof(*srat) + sizeof(*processor) * hvm_info->nr_vcpus +
> > +        sizeof(*memory) * hvm_info->nr_vmemranges;
> > +
> > +    p = mem_alloc(size, 16);
> > +    if (!p) return NULL;
> > +
> > +    srat = p;
> > +    memset(srat, 0, sizeof(*srat));
> > +    srat->header.signature    = ACPI_2_0_SRAT_SIGNATURE;
> > +    srat->header.revision     = ACPI_2_0_SRAT_REVISION;
> > +    fixed_strcpy(srat->header.oem_id, ACPI_OEM_ID);
> > +    fixed_strcpy(srat->header.oem_table_id, ACPI_OEM_TABLE_ID);
> > +    srat->header.oem_revision = ACPI_OEM_REVISION;
> > +    srat->header.creator_id   = ACPI_CREATOR_ID;
> > +    srat->header.creator_revision = ACPI_CREATOR_REVISION;
> > +    srat->table_revision      = ACPI_SRAT_TABLE_REVISION;
> > +
> > +    processor = (struct acpi_20_srat_processor *)(srat + 1);
> > +    for ( i = 0; i < hvm_info->nr_vcpus; i++ )
> > +    {
> > +        memset(processor, 0, sizeof(*processor));
> > +        processor->type     = ACPI_PROCESSOR_AFFINITY;
> > +        processor->length   = sizeof(*processor);
> > +        processor->domain   = hvm_info->vcpu_to_vnode[i];
> > +        processor->apic_id  = LAPIC_ID(i);
> > +        processor->flags    = ACPI_LOCAL_APIC_AFFIN_ENABLED;
> > +        processor->sapic_id = 0;
> 
> Either you initialize all fields explicitly and drop the memset(), or
> you consistently avoid explicit zero initializers (as being redundant).
> 

Ack.

> > @@ -270,6 +331,13 @@ static int construct_secondary_tables(unsigned long 
> > *table_ptrs,
> >          table_ptrs[nr_tables++] = (unsigned long)madt;
> >      }
> >  
> > +    if ( hvm_info->nr_nodes > 0 )
> > +    {
> > +        srat = construct_srat();
> > +        if (!srat) return -1;
> 
> I don't think failure to set up secondary information (especially when
> it requires a variable size table and hence has [slightly] higher
> likelihood of table space allocation failing) should result in skipping
> other table setup.
> 

But MADT, HPET and WAET are treated like that. I want to be
consistent.

Wei.

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel