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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Dec 09, 2014 at 04:53:40PM +0000, Jan Beulich wrote:
> >>> On 01.12.14 at 16:33, <wei.liu2@xxxxxxxxxx> wrote:
> > @@ -203,6 +204,65 @@ 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) * nr_vmemranges;
> > +
> > +    p = mem_alloc(size, 16);
> > +    if (!p) return NULL;
> 
> Coding style (despite you likely copied it from elsewhere).
> 

Fixed.

> > +
> > +    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   = vcpu_to_vnode[i];
> > +        processor->apic_id  = LAPIC_ID(i);
> > +        processor->flags    = ACPI_LOCAL_APIC_AFFIN_ENABLED;
> > +        processor++;
> > +    }
> > +
> > +    memory = (struct acpi_20_srat_memory *)processor;
> > +    for ( i = 0; i < nr_vmemranges; i++ )
> > +    {
> > +        mem = vmemrange[i].end - vmemrange[i].start;
> 
> Do you really need this helper variable?
> 

Removed.

> > +        memset(memory, 0, sizeof(*memory));
> > +        memory->type          = ACPI_MEMORY_AFFINITY;
> > +        memory->length        = sizeof(*memory);
> > +        memory->domain        = vmemrange[i].nid;
> > +        memory->flags         = ACPI_MEM_AFFIN_ENABLED;
> > +        memory->base_address  = vmemrange[i].start;
> > +        memory->mem_length    = mem;
> > +        memory++;
> > +    }
> > +
> > +    srat->header.length = size;
> 
> Mind checking size == memory - p here?
> 

Why? There doesn't seem to be anything that would cause memory -p !=
size in between during runtime.

> > @@ -346,6 +407,16 @@ static int construct_secondary_tables(unsigned long 
> > *table_ptrs,
> >          }
> >      }
> >  
> > +    /* SRAT */
> > +    if ( nr_vnodes > 0 )
> > +    {
> > +        srat = construct_srat();
> > +        if (srat)
> 
> Coding style again.
> 

Fixed.

Wei.

> Jan

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