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

2014-12-10 Thread Jan Beulich 
 On 10.12.14 at 11:54, wei.l...@citrix.com wrote:
 On Wed, Dec 10, 2014 at 08:20:38AM +, Jan Beulich wrote:
  On 09.12.14 at 19:06, wei.l...@citrix.com wrote:
  On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote:
   On 01.12.14 at 16:33, wei.l...@citrix.com 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

2014-12-10 Thread Wei Liu 
On Wed, Dec 10, 2014 at 11:06:20AM +, Jan Beulich wrote:
  On 10.12.14 at 11:54, wei.l...@citrix.com wrote:
  On Wed, Dec 10, 2014 at 08:20:38AM +, Jan Beulich wrote:
   On 09.12.14 at 19:06, wei.l...@citrix.com wrote:
   On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote:
On 01.12.14 at 16:33, wei.l...@citrix.com 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

2014-12-09 Thread Jan Beulich 
 On 01.12.14 at 16:33, wei.l...@citrix.com 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

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

2014-12-09 Thread Wei Liu 
On Tue, Dec 09, 2014 at 04:53:40PM +, Jan Beulich wrote:
  On 01.12.14 at 16:33, wei.l...@citrix.com 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

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

2014-12-01 Thread Wei Liu 
Signed-off-by: Wei Liu wei.l...@citrix.com
Cc: Jan Beulich jbeul...@suse.com
---
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 xen/hvm/hvm_xs_strings.h
 #include xen/hvm/params.h
 
@@ -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;
+}
+