Re: [Xen-devel] [PATCH v2 13/19] hvmloader: construct SRAT
On Wed, Dec 10, 2014 at 11:06:20AM +, Jan Beulich wrote: > >>> On 10.12.14 at 11:54, wrote: > > On Wed, Dec 10, 2014 at 08:20:38AM +, Jan Beulich wrote: > >> >>> On 09.12.14 at 19:06, wrote: > >> > On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote: > >> >> >>> On 01.12.14 at 16:33, wrote: > >> >> > +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. > >> > >> Except for that original calculation being wrong - that's what I would > >> mean such a check to verify. > >> > > > > Do you mean the code is wrong? I don't think so. > > No, the code looks right. It's just that two disconnected > calculations easily get out of sync when someone later changes > them. > I see. > > Even if I have > > > > +if ( ((unsigned long)memory) - ((unsigned long)p) != size ) > > +return NULL; > > + > > > > Hvmloader doesn't complain, i.e. I don't see error message printed out > > in the construct_secondary_tables. > > > > Anyway, if the above snippet is what you want, I can add it. > > I was actually more after a BUG_ON() / ASSERT() kind of thing. Ack. > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 13/19] hvmloader: construct SRAT
>>> On 10.12.14 at 11:54, wrote: > On Wed, Dec 10, 2014 at 08:20:38AM +, Jan Beulich wrote: >> >>> On 09.12.14 at 19:06, wrote: >> > On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote: >> >> >>> On 01.12.14 at 16:33, wrote: >> >> > +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. >> >> Except for that original calculation being wrong - that's what I would >> mean such a check to verify. >> > > Do you mean the code is wrong? I don't think so. No, the code looks right. It's just that two disconnected calculations easily get out of sync when someone later changes them. > Even if I have > > +if ( ((unsigned long)memory) - ((unsigned long)p) != size ) > +return NULL; > + > > Hvmloader doesn't complain, i.e. I don't see error message printed out > in the construct_secondary_tables. > > Anyway, if the above snippet is what you want, I can add it. I was actually more after a BUG_ON() / ASSERT() kind of thing. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 13/19] hvmloader: construct SRAT
On Wed, Dec 10, 2014 at 08:20:38AM +, Jan Beulich wrote: > >>> On 09.12.14 at 19:06, wrote: > > On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote: > >> >>> On 01.12.14 at 16:33, wrote: > >> > +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. > > Except for that original calculation being wrong - that's what I would > mean such a check to verify. > Do you mean the code is wrong? I don't think so. Even if I have +if ( ((unsigned long)memory) - ((unsigned long)p) != size ) +return NULL; + Hvmloader doesn't complain, i.e. I don't see error message printed out in the construct_secondary_tables. Anyway, if the above snippet is what you want, I can add it. Wei. > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 13/19] hvmloader: construct SRAT
>>> On 09.12.14 at 19:06, wrote: > On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote: >> >>> On 01.12.14 at 16:33, wrote: >> > +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. Except for that original calculation being wrong - that's what I would mean such a check to verify. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 13/19] hvmloader: construct SRAT
On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote:
> >>> On 01.12.14 at 16:33, 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@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 13/19] hvmloader: construct SRAT
>>> On 01.12.14 at 16:33, 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).
> +
> +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?
> +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?
> @@ -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.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 13/19] hvmloader: construct SRAT
Signed-off-by: Wei Liu
Cc: Jan Beulich
---
Changes in v2:
1. Remove explicit zero initializers.
2. Adapt to new vNUMA retrieval routine.
3. Move SRAT very late in secondary table build.
---
tools/firmware/hvmloader/acpi/acpi2_0.h | 53 +++
tools/firmware/hvmloader/acpi/build.c | 71 +++
2 files changed, 124 insertions(+)
diff --git a/tools/firmware/hvmloader/acpi/acpi2_0.h
b/tools/firmware/hvmloader/acpi/acpi2_0.h
index 7b22d80..6169213 100644
--- a/tools/firmware/hvmloader/acpi/acpi2_0.h
+++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
@@ -364,6 +364,57 @@ struct acpi_20_madt_intsrcovr {
};
/*
+ * System Resource Affinity Table header definition (SRAT)
+ */
+struct acpi_20_srat {
+struct acpi_header header;
+uint32_t table_revision;
+uint32_t reserved2[2];
+};
+
+#define ACPI_SRAT_TABLE_REVISION 1
+
+/*
+ * System Resource Affinity Table structure types.
+ */
+#define ACPI_PROCESSOR_AFFINITY 0x0
+#define ACPI_MEMORY_AFFINITY0x1
+struct acpi_20_srat_processor {
+uint8_t type;
+uint8_t length;
+uint8_t domain;
+uint8_t apic_id;
+uint32_t flags;
+uint8_t sapic_id;
+uint8_t domain_hi[3];
+uint32_t reserved;
+};
+
+/*
+ * Local APIC Affinity Flags. All other bits are reserved and must be 0.
+ */
+#define ACPI_LOCAL_APIC_AFFIN_ENABLED (1 << 0)
+
+struct acpi_20_srat_memory {
+uint8_t type;
+uint8_t length;
+uint32_t domain;
+uint16_t reserved;
+uint64_t base_address;
+uint64_t mem_length;
+uint32_t reserved2;
+uint32_t flags;
+uint64_t reserved3;
+};
+
+/*
+ * Memory Affinity Flags. All other bits are reserved and must be 0.
+ */
+#define ACPI_MEM_AFFIN_ENABLED (1 << 0)
+#define ACPI_MEM_AFFIN_HOTPLUGGABLE (1 << 1)
+#define ACPI_MEM_AFFIN_NONVOLATILE (1 << 2)
+
+/*
* Table Signatures.
*/
#define ACPI_2_0_RSDP_SIGNATURE ASCII64('R','S','D',' ','P','T','R',' ')
@@ -375,6 +426,7 @@ struct acpi_20_madt_intsrcovr {
#define ACPI_2_0_TCPA_SIGNATURE ASCII32('T','C','P','A')
#define ACPI_2_0_HPET_SIGNATURE ASCII32('H','P','E','T')
#define ACPI_2_0_WAET_SIGNATURE ASCII32('W','A','E','T')
+#define ACPI_2_0_SRAT_SIGNATURE ASCII32('S','R','A','T')
/*
* Table revision numbers.
@@ -388,6 +440,7 @@ struct acpi_20_madt_intsrcovr {
#define ACPI_2_0_HPET_REVISION 0x01
#define ACPI_2_0_WAET_REVISION 0x01
#define ACPI_1_0_FADT_REVISION 0x01
+#define ACPI_2_0_SRAT_REVISION 0x01
#pragma pack ()
diff --git a/tools/firmware/hvmloader/acpi/build.c
b/tools/firmware/hvmloader/acpi/build.c
index 1431296..917b2c9 100644
--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -23,6 +23,7 @@
#include "ssdt_pm.h"
#include "../config.h"
#include "../util.h"
+#include "../vnuma.h"
#include
#include
@@ -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;
+
+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;
+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;
+set_checksum(srat, offsetof(struct acpi_header, checksum), size);
+
+return srat;
+}
+
static int construct_passthrough_tabl
