[PATCH 0/2] x86: hvmloader improvements

[PATCH 0/2] x86: hvmloader improvements

[Jan Beulich]

1: limit CPUs exposed to guests
2: don't hard-code IO-APIC parameters

Signed-off-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5168 bytes --]

Various Linux versions allocate (partial) per-CPU data for all of them,
as there is no indication in MADT whether they're hotpluggable. That's
a little wasteful in terms of resource consumption especially for
- guests with not overly much memory assigned,
- 32-bit guests not having overly much address space available.
Therefore limit what we put into MADT to the "maxvcpus" value, and make
sure AML doesn't touch memory addresses corresponding to CPUs beyond
that value (we can't reasonably make the respective processor objects
disappear).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -48,6 +48,7 @@ struct acpi_info {
     uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
     uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */
     uint8_t  hpet_present:1;    /* 0[3] - System has HPET? */
+    uint16_t nr_cpus;           /* 2    - Number of CPUs */
     uint32_t pci_min, pci_len;  /* 4, 8 - PCI I/O hole boundaries */
     uint32_t madt_csum_addr;    /* 12   - Address of MADT checksum */
     uint32_t madt_lapic0_addr;  /* 16   - Address of first MADT LAPIC struct */
@@ -55,9 +56,6 @@ struct acpi_info {
     uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
 };
 
-/* Number of processor objects in the chosen DSDT. */
-static unsigned int nr_processor_objects;
-
 static void set_checksum(
     void *table, uint32_t checksum_offset, uint32_t length)
 {
@@ -89,7 +87,7 @@ static struct acpi_20_madt *construct_ma
     sz  = sizeof(struct acpi_20_madt);
     sz += sizeof(struct acpi_20_madt_intsrcovr) * 16;
     sz += sizeof(struct acpi_20_madt_ioapic);
-    sz += sizeof(struct acpi_20_madt_lapic) * nr_processor_objects;
+    sz += sizeof(struct acpi_20_madt_lapic) * hvm_info->nr_vcpus;
 
     madt = mem_alloc(sz, 16);
     if (!madt) return NULL;
@@ -142,8 +140,9 @@ static struct acpi_20_madt *construct_ma
     io_apic->ioapic_addr = IOAPIC_BASE_ADDRESS;
 
     lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
+    info->nr_cpus = hvm_info->nr_vcpus;
     info->madt_lapic0_addr = (uint32_t)lapic;
-    for ( i = 0; i < nr_processor_objects; i++ )
+    for ( i = 0; i < hvm_info->nr_vcpus; i++ )
     {
         memset(lapic, 0, sizeof(*lapic));
         lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
@@ -151,8 +150,7 @@ static struct acpi_20_madt *construct_ma
         /* Processor ID must match processor-object IDs in the DSDT. */
         lapic->acpi_processor_id = i;
         lapic->apic_id = LAPIC_ID(i);
-        lapic->flags = ((i < hvm_info->nr_vcpus) &&
-                        test_bit(i, hvm_info->vcpu_online)
+        lapic->flags = (test_bit(i, hvm_info->vcpu_online)
                         ? ACPI_LOCAL_APIC_ENABLED : 0);
         lapic++;
     }
@@ -534,14 +532,12 @@ void acpi_build_tables(struct acpi_confi
         dsdt = mem_alloc(config->dsdt_15cpu_len, 16);
         if (!dsdt) goto oom;
         memcpy(dsdt, config->dsdt_15cpu, config->dsdt_15cpu_len);
-        nr_processor_objects = 15;
     }
     else
     {
         dsdt = mem_alloc(config->dsdt_anycpu_len, 16);
         if (!dsdt) goto oom;
         memcpy(dsdt, config->dsdt_anycpu, config->dsdt_anycpu_len);
-        nr_processor_objects = HVM_MAX_VCPUS;
     }
 
     /*
--- a/tools/firmware/hvmloader/acpi/dsdt.asl
+++ b/tools/firmware/hvmloader/acpi/dsdt.asl
@@ -50,7 +50,8 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2,
            UAR2, 1,
            LTP1, 1,
            HPET, 1,
-           Offset(4),
+           Offset(2),
+           NCPU, 16,
            PMIN, 32,
            PLEN, 32,
            MSUA, 32, /* MADT checksum address */
--- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
+++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
@@ -150,6 +150,14 @@ int main(int argc, char **argv)
     indent(); printf("MSU, 8\n");
     pop_block();
 
+    /* Processor object helpers. */
+    push_block("Method", "PMAT, 2");
+    push_block("If", "LLess(Arg0, NCPU)");
+    stmt("Return", "ToBuffer(Arg1)");
+    pop_block();
+    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
+    pop_block();
+
     /* Define processor objects and control methods. */
     for ( cpu = 0; cpu < max_cpus; cpu++)
     {
@@ -171,17 +179,22 @@ int main(int argc, char **argv)
         pop_block();
 
         push_block("Method", "_MAT, 0");
-        stmt("Return", "ToBuffer(MAT)");
+        if ( cpu )
+            stmt("Return", "PMAT (%d, MAT)", cpu);
+        else
+            stmt("Return", "ToBuffer(MAT)");
         pop_block();
 
         push_block("Method", "_STA");
+        if ( cpu )
+            push_block("If", "LLess(%d, \\_SB.NCPU)", cpu);
         push_block("If", "FLG");
         stmt("Return", "0xF");
         pop_block();
-        push_block("Else", NULL);
+        if ( cpu )
+            pop_block();
         stmt("Return", "0x0");
         pop_block();
-        pop_block();
 
         push_block("Method", "_EJ0, 1, NotSerialized");
         stmt("Sleep", "0xC8");



[-- Attachment #2: hvmloader-limit-CPUs.patch --]
[-- Type: text/plain, Size: 5207 bytes --]

hvmloader: limit CPUs exposed to guests

Various Linux versions allocate (partial) per-CPU data for all of them,
as there is no indication in MADT whether they're hotpluggable. That's
a little wasteful in terms of resource consumption especially for
- guests with not overly much memory assigned,
- 32-bit guests not having overly much address space available.
Therefore limit what we put into MADT to the "maxvcpus" value, and make
sure AML doesn't touch memory addresses corresponding to CPUs beyond
that value (we can't reasonably make the respective processor objects
disappear).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -48,6 +48,7 @@ struct acpi_info {
     uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
     uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */
     uint8_t  hpet_present:1;    /* 0[3] - System has HPET? */
+    uint16_t nr_cpus;           /* 2    - Number of CPUs */
     uint32_t pci_min, pci_len;  /* 4, 8 - PCI I/O hole boundaries */
     uint32_t madt_csum_addr;    /* 12   - Address of MADT checksum */
     uint32_t madt_lapic0_addr;  /* 16   - Address of first MADT LAPIC struct */
@@ -55,9 +56,6 @@ struct acpi_info {
     uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
 };
 
-/* Number of processor objects in the chosen DSDT. */
-static unsigned int nr_processor_objects;
-
 static void set_checksum(
     void *table, uint32_t checksum_offset, uint32_t length)
 {
@@ -89,7 +87,7 @@ static struct acpi_20_madt *construct_ma
     sz  = sizeof(struct acpi_20_madt);
     sz += sizeof(struct acpi_20_madt_intsrcovr) * 16;
     sz += sizeof(struct acpi_20_madt_ioapic);
-    sz += sizeof(struct acpi_20_madt_lapic) * nr_processor_objects;
+    sz += sizeof(struct acpi_20_madt_lapic) * hvm_info->nr_vcpus;
 
     madt = mem_alloc(sz, 16);
     if (!madt) return NULL;
@@ -142,8 +140,9 @@ static struct acpi_20_madt *construct_ma
     io_apic->ioapic_addr = IOAPIC_BASE_ADDRESS;
 
     lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
+    info->nr_cpus = hvm_info->nr_vcpus;
     info->madt_lapic0_addr = (uint32_t)lapic;
-    for ( i = 0; i < nr_processor_objects; i++ )
+    for ( i = 0; i < hvm_info->nr_vcpus; i++ )
     {
         memset(lapic, 0, sizeof(*lapic));
         lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
@@ -151,8 +150,7 @@ static struct acpi_20_madt *construct_ma
         /* Processor ID must match processor-object IDs in the DSDT. */
         lapic->acpi_processor_id = i;
         lapic->apic_id = LAPIC_ID(i);
-        lapic->flags = ((i < hvm_info->nr_vcpus) &&
-                        test_bit(i, hvm_info->vcpu_online)
+        lapic->flags = (test_bit(i, hvm_info->vcpu_online)
                         ? ACPI_LOCAL_APIC_ENABLED : 0);
         lapic++;
     }
@@ -534,14 +532,12 @@ void acpi_build_tables(struct acpi_confi
         dsdt = mem_alloc(config->dsdt_15cpu_len, 16);
         if (!dsdt) goto oom;
         memcpy(dsdt, config->dsdt_15cpu, config->dsdt_15cpu_len);
-        nr_processor_objects = 15;
     }
     else
     {
         dsdt = mem_alloc(config->dsdt_anycpu_len, 16);
         if (!dsdt) goto oom;
         memcpy(dsdt, config->dsdt_anycpu, config->dsdt_anycpu_len);
-        nr_processor_objects = HVM_MAX_VCPUS;
     }
 
     /*
--- a/tools/firmware/hvmloader/acpi/dsdt.asl
+++ b/tools/firmware/hvmloader/acpi/dsdt.asl
@@ -50,7 +50,8 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2,
            UAR2, 1,
            LTP1, 1,
            HPET, 1,
-           Offset(4),
+           Offset(2),
+           NCPU, 16,
            PMIN, 32,
            PLEN, 32,
            MSUA, 32, /* MADT checksum address */
--- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
+++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
@@ -150,6 +150,14 @@ int main(int argc, char **argv)
     indent(); printf("MSU, 8\n");
     pop_block();
 
+    /* Processor object helpers. */
+    push_block("Method", "PMAT, 2");
+    push_block("If", "LLess(Arg0, NCPU)");
+    stmt("Return", "ToBuffer(Arg1)");
+    pop_block();
+    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
+    pop_block();
+
     /* Define processor objects and control methods. */
     for ( cpu = 0; cpu < max_cpus; cpu++)
     {
@@ -171,17 +179,22 @@ int main(int argc, char **argv)
         pop_block();
 
         push_block("Method", "_MAT, 0");
-        stmt("Return", "ToBuffer(MAT)");
+        if ( cpu )
+            stmt("Return", "PMAT (%d, MAT)", cpu);
+        else
+            stmt("Return", "ToBuffer(MAT)");
         pop_block();
 
         push_block("Method", "_STA");
+        if ( cpu )
+            push_block("If", "LLess(%d, \\_SB.NCPU)", cpu);
         push_block("If", "FLG");
         stmt("Return", "0xF");
         pop_block();
-        push_block("Else", NULL);
+        if ( cpu )
+            pop_block();
         stmt("Return", "0x0");
         pop_block();
-        pop_block();
 
         push_block("Method", "_EJ0, 1, NotSerialized");
         stmt("Sleep", "0xC8");

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Boris Ostrovsky]

On 06/16/2016 05:40 AM, Jan Beulich wrote:

--- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
+++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
@@ -150,6 +150,14 @@ int main(int argc, char **argv)
     indent(); printf("MSU, 8\n");
     pop_block();
 
+    /* Processor object helpers. */
+    push_block("Method", "PMAT, 2");
+    push_block("If", "LLess(Arg0, NCPU)");
+    stmt("Return", "ToBuffer(Arg1)");
+    pop_block();
+    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");


Could you explain what this is? (I suspect  this is related to MAT
object and I don't think I understand what it is).

-boris



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

Re: [PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Jan Beulich]

>>> On 16.06.16 at 17:09, <boris.ostrovsky@oracle.com> wrote:
> On 06/16/2016 05:40 AM, Jan Beulich wrote:
> 
> --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
> +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
> @@ -150,6 +150,14 @@ int main(int argc, char **argv)
>      indent(); printf("MSU, 8\n");
>      pop_block();
>  
> +    /* Processor object helpers. */
> +    push_block("Method", "PMAT, 2");
> +    push_block("If", "LLess(Arg0, NCPU)");
> +    stmt("Return", "ToBuffer(Arg1)");
> +    pop_block();
> +    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
> 
> 
> Could you explain what this is? (I suspect  this is related to MAT
> object and I don't think I understand what it is).

This is a helper routine for _MAT(), helping greatly to reduce
overall size of the DSDT. It checks whether the CPU is within the
range of available ones (online or offline), and if it isn't returns a
static buffer instead of data read from MADT (as it's the purpose
of this patch to remove these MADT entries for not present CPUs).

Jan


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

Re: [PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Boris Ostrovsky]

On 06/16/2016 11:25 AM, Jan Beulich wrote:
>>>> On 16.06.16 at 17:09, <boris.ostrovsky@oracle.com> wrote:
>> On 06/16/2016 05:40 AM, Jan Beulich wrote:
>>
>> --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
>> +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
>> @@ -150,6 +150,14 @@ int main(int argc, char **argv)
>>      indent(); printf("MSU, 8\n");
>>      pop_block();
>>  
>> +    /* Processor object helpers. */
>> +    push_block("Method", "PMAT, 2");
>> +    push_block("If", "LLess(Arg0, NCPU)");
>> +    stmt("Return", "ToBuffer(Arg1)");
>> +    pop_block();
>> +    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
>>
>>
>> Could you explain what this is? (I suspect  this is related to MAT
>> object and I don't think I understand what it is).
> This is a helper routine for _MAT(), helping greatly to reduce
> overall size of the DSDT. It checks whether the CPU is within the
> range of available ones (online or offline), and if it isn't returns a
> static buffer instead of data read from MADT (as it's the purpose
> of this patch to remove these MADT entries for not present CPUs).

I meant just the last line (I understand what the routine is --- I am
not clear where MAT format is defined).

-boris

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

Re: [PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Jan Beulich]

>>> On 16.06.16 at 17:37, <boris.ostrovsky@oracle.com> wrote:
> On 06/16/2016 11:25 AM, Jan Beulich wrote:
>>>>> On 16.06.16 at 17:09, <boris.ostrovsky@oracle.com> wrote:
>>> On 06/16/2016 05:40 AM, Jan Beulich wrote:
>>>
>>> --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
>>> +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
>>> @@ -150,6 +150,14 @@ int main(int argc, char **argv)
>>>      indent(); printf("MSU, 8\n");
>>>      pop_block();
>>>  
>>> +    /* Processor object helpers. */
>>> +    push_block("Method", "PMAT, 2");
>>> +    push_block("If", "LLess(Arg0, NCPU)");
>>> +    stmt("Return", "ToBuffer(Arg1)");
>>> +    pop_block();
>>> +    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
>>>
>>>
>>> Could you explain what this is? (I suspect  this is related to MAT
>>> object and I don't think I understand what it is).
>> This is a helper routine for _MAT(), helping greatly to reduce
>> overall size of the DSDT. It checks whether the CPU is within the
>> range of available ones (online or offline), and if it isn't returns a
>> static buffer instead of data read from MADT (as it's the purpose
>> of this patch to remove these MADT entries for not present CPUs).
> 
> I meant just the last line (I understand what the routine is --- I am
> not clear where MAT format is defined).

That's a MADT entry of type ACPI_MADT_TYPE_LOCAL_APIC. Basically
an equivalent of the ToBuffer(MAT) used previously (and now still used
for CPU0).

Jan


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

Re: [PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Boris Ostrovsky]

On 06/16/2016 12:04 PM, Jan Beulich wrote:
>>>> On 16.06.16 at 17:37, <boris.ostrovsky@oracle.com> wrote:
>> On 06/16/2016 11:25 AM, Jan Beulich wrote:
>>>>>> On 16.06.16 at 17:09, <boris.ostrovsky@oracle.com> wrote:
>>>> On 06/16/2016 05:40 AM, Jan Beulich wrote:
>>>>
>>>> --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
>>>> +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
>>>> @@ -150,6 +150,14 @@ int main(int argc, char **argv)
>>>>      indent(); printf("MSU, 8\n");
>>>>      pop_block();
>>>>  
>>>> +    /* Processor object helpers. */
>>>> +    push_block("Method", "PMAT, 2");
>>>> +    push_block("If", "LLess(Arg0, NCPU)");
>>>> +    stmt("Return", "ToBuffer(Arg1)");
>>>> +    pop_block();
>>>> +    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
>>>>
>>>>
>>>> Could you explain what this is? (I suspect  this is related to MAT
>>>> object and I don't think I understand what it is).
>>> This is a helper routine for _MAT(), helping greatly to reduce
>>> overall size of the DSDT. It checks whether the CPU is within the
>>> range of available ones (online or offline), and if it isn't returns a
>>> static buffer instead of data read from MADT (as it's the purpose
>>> of this patch to remove these MADT entries for not present CPUs).
>> I meant just the last line (I understand what the routine is --- I am
>> not clear where MAT format is defined).
> That's a MADT entry of type ACPI_MADT_TYPE_LOCAL_APIC. Basically
> an equivalent of the ToBuffer(MAT) used previously (and now still used
> for CPU0).

Ah, ok -- makes sense now.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


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

Re: [PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Andrew Cooper]

On 16/06/16 10:40, Jan Beulich wrote:
> Various Linux versions allocate (partial) per-CPU data for all of them,
> as there is no indication in MADT whether they're hotpluggable. That's
> a little wasteful in terms of resource consumption especially for
> - guests with not overly much memory assigned,
> - 32-bit guests not having overly much address space available.
> Therefore limit what we put into MADT to the "maxvcpus" value, and make
> sure AML doesn't touch memory addresses corresponding to CPUs beyond
> that value (we can't reasonably make the respective processor objects
> disappear).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I don't feel that I know AML well enough to review, but the C looks ok
and this seems like a sensible change.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

The IO-APIC address has variable bits determined by the PCI-to-ISA
bridge, and the IO-APIC version should be read from the IO-APIC. (Note
that there's still implicit rather than explicit agreement on the
IO-APIC base address between qemu and the hypervisor.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -138,7 +138,7 @@ static struct acpi_20_madt *construct_ma
     io_apic->type        = ACPI_IO_APIC;
     io_apic->length      = sizeof(*io_apic);
     io_apic->ioapic_id   = IOAPIC_ID;
-    io_apic->ioapic_addr = IOAPIC_BASE_ADDRESS;
+    io_apic->ioapic_addr = ioapic_base_address;
 
     lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
     info->nr_cpus = hvm_info->nr_vcpus;
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -42,9 +42,10 @@ extern struct bios_config ovmf_config;
 #define PAGE_SHIFT 12
 #define PAGE_SIZE  (1ul << PAGE_SHIFT)
 
-#define IOAPIC_BASE_ADDRESS 0xfec00000
+extern uint32_t ioapic_base_address;
+extern uint8_t ioapic_version;
+
 #define IOAPIC_ID           0x01
-#define IOAPIC_VERSION      0x11
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
 #define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -108,6 +108,9 @@ asm (
 
 unsigned long scratch_start = SCRATCH_PHYSICAL_ADDRESS;
 
+uint32_t ioapic_base_address = 0xfec00000;
+uint8_t ioapic_version;
+
 static void init_hypercalls(void)
 {
     uint32_t eax, ebx, ecx, edx;
@@ -185,6 +188,9 @@ static void init_vm86_tss(void)
 
 static void apic_setup(void)
 {
+    ioapic_base_address |= (pci_readb(PCI_ISA_DEVFN, 0x80) & 0x3f) << 10;
+    ioapic_version = ioapic_read(0x01) & 0xff;
+
     /* Set the IOAPIC ID to the static value used in the MP/ACPI tables. */
     ioapic_write(0x00, IOAPIC_ID);
 
--- a/tools/firmware/hvmloader/mp_tables.c
+++ b/tools/firmware/hvmloader/mp_tables.c
@@ -227,9 +227,9 @@ static void fill_mp_ioapic_entry(struct
 {
     mpie->type = ENTRY_TYPE_IOAPIC;
     mpie->ioapic_id = IOAPIC_ID;
-    mpie->ioapic_version = IOAPIC_VERSION;
+    mpie->ioapic_version = ioapic_version;
     mpie->ioapic_flags = 1; /* enabled */
-    mpie->ioapic_addr = IOAPIC_BASE_ADDRESS;
+    mpie->ioapic_addr = ioapic_base_address;
 }
 
 
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -490,14 +490,14 @@ void *scratch_alloc(uint32_t size, uint3
 
 uint32_t ioapic_read(uint32_t reg)
 {
-    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
-    return *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10);
+    *(volatile uint32_t *)(ioapic_base_address + 0x00) = reg;
+    return *(volatile uint32_t *)(ioapic_base_address + 0x10);
 }
 
 void ioapic_write(uint32_t reg, uint32_t val)
 {
-    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
-    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10) = val;
+    *(volatile uint32_t *)(ioapic_base_address + 0x00) = reg;
+    *(volatile uint32_t *)(ioapic_base_address + 0x10) = val;
 }
 
 uint32_t lapic_read(uint32_t reg)




[-- Attachment #2: hvmloader-IOAPIC-settings.patch --]
[-- Type: text/plain, Size: 3265 bytes --]

hvmloader: don't hard-code IO-APIC parameters

The IO-APIC address has variable bits determined by the PCI-to-ISA
bridge, and the IO-APIC version should be read from the IO-APIC. (Note
that there's still implicit rather than explicit agreement on the
IO-APIC base address between qemu and the hypervisor.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -138,7 +138,7 @@ static struct acpi_20_madt *construct_ma
     io_apic->type        = ACPI_IO_APIC;
     io_apic->length      = sizeof(*io_apic);
     io_apic->ioapic_id   = IOAPIC_ID;
-    io_apic->ioapic_addr = IOAPIC_BASE_ADDRESS;
+    io_apic->ioapic_addr = ioapic_base_address;
 
     lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
     info->nr_cpus = hvm_info->nr_vcpus;
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -42,9 +42,10 @@ extern struct bios_config ovmf_config;
 #define PAGE_SHIFT 12
 #define PAGE_SIZE  (1ul << PAGE_SHIFT)
 
-#define IOAPIC_BASE_ADDRESS 0xfec00000
+extern uint32_t ioapic_base_address;
+extern uint8_t ioapic_version;
+
 #define IOAPIC_ID           0x01
-#define IOAPIC_VERSION      0x11
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
 #define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -108,6 +108,9 @@ asm (
 
 unsigned long scratch_start = SCRATCH_PHYSICAL_ADDRESS;
 
+uint32_t ioapic_base_address = 0xfec00000;
+uint8_t ioapic_version;
+
 static void init_hypercalls(void)
 {
     uint32_t eax, ebx, ecx, edx;
@@ -185,6 +188,9 @@ static void init_vm86_tss(void)
 
 static void apic_setup(void)
 {
+    ioapic_base_address |= (pci_readb(PCI_ISA_DEVFN, 0x80) & 0x3f) << 10;
+    ioapic_version = ioapic_read(0x01) & 0xff;
+
     /* Set the IOAPIC ID to the static value used in the MP/ACPI tables. */
     ioapic_write(0x00, IOAPIC_ID);
 
--- a/tools/firmware/hvmloader/mp_tables.c
+++ b/tools/firmware/hvmloader/mp_tables.c
@@ -227,9 +227,9 @@ static void fill_mp_ioapic_entry(struct
 {
     mpie->type = ENTRY_TYPE_IOAPIC;
     mpie->ioapic_id = IOAPIC_ID;
-    mpie->ioapic_version = IOAPIC_VERSION;
+    mpie->ioapic_version = ioapic_version;
     mpie->ioapic_flags = 1; /* enabled */
-    mpie->ioapic_addr = IOAPIC_BASE_ADDRESS;
+    mpie->ioapic_addr = ioapic_base_address;
 }
 
 
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -490,14 +490,14 @@ void *scratch_alloc(uint32_t size, uint3
 
 uint32_t ioapic_read(uint32_t reg)
 {
-    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
-    return *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10);
+    *(volatile uint32_t *)(ioapic_base_address + 0x00) = reg;
+    return *(volatile uint32_t *)(ioapic_base_address + 0x10);
 }
 
 void ioapic_write(uint32_t reg, uint32_t val)
 {
-    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
-    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10) = val;
+    *(volatile uint32_t *)(ioapic_base_address + 0x00) = reg;
+    *(volatile uint32_t *)(ioapic_base_address + 0x10) = val;
 }
 
 uint32_t lapic_read(uint32_t reg)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Boris Ostrovsky]

On 06/16/2016 05:40 AM, Jan Beulich wrote:
> The IO-APIC address has variable bits determined by the PCI-to-ISA
> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
> that there's still implicit rather than explicit agreement on the
> IO-APIC base address between qemu and the hypervisor.)

It probably doesn't matter now for PVHv2 since we are not going to
initially emulate IOAPIC but when/if we do, how do we query for base
address and version? Should we just rely on the same implicit agreement?

-boris

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

Re: [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Jan Beulich]

>>> On 16.06.16 at 18:49, <boris.ostrovsky@oracle.com> wrote:
> On 06/16/2016 05:40 AM, Jan Beulich wrote:
>> The IO-APIC address has variable bits determined by the PCI-to-ISA
>> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
>> that there's still implicit rather than explicit agreement on the
>> IO-APIC base address between qemu and the hypervisor.)
> 
> It probably doesn't matter now for PVHv2 since we are not going to
> initially emulate IOAPIC but when/if we do, how do we query for base
> address and version? Should we just rely on the same implicit agreement?

If this refers to the sentence in parentheses, then this was really
meant to indicate a work item. I.e. qemu should negotiate with
Xen. For PVHv2 this would then probably mean for hvmloader to
query Xen (via new hypercall).

Jan


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

Re: [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Boris Ostrovsky]

On 06/17/2016 02:00 AM, Jan Beulich wrote:
>>>> On 16.06.16 at 18:49, <boris.ostrovsky@oracle.com> wrote:
>> On 06/16/2016 05:40 AM, Jan Beulich wrote:
>>> The IO-APIC address has variable bits determined by the PCI-to-ISA
>>> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
>>> that there's still implicit rather than explicit agreement on the
>>> IO-APIC base address between qemu and the hypervisor.)
>> It probably doesn't matter now for PVHv2 since we are not going to
>> initially emulate IOAPIC but when/if we do, how do we query for base
>> address and version? Should we just rely on the same implicit agreement?
> If this refers to the sentence in parentheses, then this was really
> meant to indicate a work item. I.e. qemu should negotiate with
> Xen. For PVHv2 this would then probably mean for hvmloader to
> query Xen (via new hypercall).

We are not using hvmloader for PVH so I am not sure how that would work.
You mean the toolstack?

-boris

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

Re: [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Jan Beulich]

>>> On 17.06.16 at 13:51, <boris.ostrovsky@oracle.com> wrote:
> On 06/17/2016 02:00 AM, Jan Beulich wrote:
>>>>> On 16.06.16 at 18:49, <boris.ostrovsky@oracle.com> wrote:
>>> On 06/16/2016 05:40 AM, Jan Beulich wrote:
>>>> The IO-APIC address has variable bits determined by the PCI-to-ISA
>>>> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
>>>> that there's still implicit rather than explicit agreement on the
>>>> IO-APIC base address between qemu and the hypervisor.)
>>> It probably doesn't matter now for PVHv2 since we are not going to
>>> initially emulate IOAPIC but when/if we do, how do we query for base
>>> address and version? Should we just rely on the same implicit agreement?
>> If this refers to the sentence in parentheses, then this was really
>> meant to indicate a work item. I.e. qemu should negotiate with
>> Xen. For PVHv2 this would then probably mean for hvmloader to
>> query Xen (via new hypercall).
> 
> We are not using hvmloader for PVH so I am not sure how that would work.
> You mean the toolstack?

Oh, right. Whoever puts the ACPI tables in place.

Jan


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

Re: [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 711 bytes --]

On 16/06/16 10:40, Jan Beulich wrote:
> The IO-APIC address has variable bits determined by the PCI-to-ISA
> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
> that there's still implicit rather than explicit agreement on the
> IO-APIC base address between qemu and the hypervisor.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

The status quo is not great, and I can see why you want to improve it.

However, I think that this is not the way to do that.  It ties HVMLoader
to the PIIX4 board in Qemu, and will break attempts to use Q35 or
something else.  (In Q35, the IO-APIC decode address comes from Chipset
Configuration Register, rather than ISA device config space).

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 1206 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Jan Beulich]

>>> On 17.06.16 at 12:05, <andrew.cooper3@citrix.com> wrote:
> On 16/06/16 10:40, Jan Beulich wrote:
>> The IO-APIC address has variable bits determined by the PCI-to-ISA
>> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
>> that there's still implicit rather than explicit agreement on the
>> IO-APIC base address between qemu and the hypervisor.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> The status quo is not great, and I can see why you want to improve it.
> 
> However, I think that this is not the way to do that.  It ties HVMLoader
> to the PIIX4 board in Qemu, and will break attempts to use Q35 or
> something else.  (In Q35, the IO-APIC decode address comes from Chipset
> Configuration Register, rather than ISA device config space).

hvmloader is already tied to PIIX4, and enabling Q35 will, among
other things, require hvmloader to become aware of that chipset.

Jan


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