[RFC PATCH V2 0/4] Extend resources to support more vcpus in single VM

[RFC PATCH V2 0/4] Extend resources to support more vcpus in single VM

[Lan Tianyu]

Change since v1:
	1) Increase hap page pool according vcpu number
	2) Use "Processor" syntax to define vcpus with APIC id < 255
in dsdt and use "Device" syntax for other vcpus in ACPI DSDT table.
	3) Use XAPIC structure for vcpus with APIC id < 255
in dsdt and use x2APIC structure for other vcpus in the ACPI MADT table.

This patchset is to extend some resources(i.e, event channel,
hap and so) to support more vcpus for single VM.


Chao Gao (1):
  xl/libacpi: extend lapic_id() to uint32_t

Lan Tianyu (3):
  xen/hap: Increase hap size for more vcpus support
  Tool/ACPI: DSDT extension to support more vcpus
  hvmload: Add x2apic entry support in the MADT build

 tools/firmware/hvmloader/util.c |  2 +-
 tools/libacpi/acpi2_0.h         | 10 +++++++
 tools/libacpi/build.c           | 59 +++++++++++++++++++++++++++++------------
 tools/libacpi/libacpi.h         |  2 +-
 tools/libacpi/mk_dsdt.c         | 18 ++++++++-----
 tools/libxl/libxl_x86_acpi.c    |  2 +-
 xen/arch/x86/mm/hap/hap.c       | 10 ++++++-
 7 files changed, 76 insertions(+), 27 deletions(-)

-- 
1.8.3.1


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

[RFC PATCH V2 1/4] xen/hap: Increase hap page pool size for more vcpus support

[Lan Tianyu]

This patch is to increase hap page pool size to support more vcpus in single VM.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 xen/arch/x86/mm/hap/hap.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index cdc77a9..96a7ed0 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -464,6 +464,7 @@ void hap_domain_init(struct domain *d)
 int hap_enable(struct domain *d, u32 mode)
 {
     unsigned int old_pages;
+    unsigned int pages;
     unsigned int i;
     int rv = 0;
 
@@ -473,7 +474,14 @@ int hap_enable(struct domain *d, u32 mode)
     if ( old_pages == 0 )
     {
         paging_lock(d);
-        rv = hap_set_allocation(d, 256, NULL);
+
+        /* Increase hap page pool with max vcpu number. */
+        if ( d->max_vcpus > 128 )
+            pages = 256;
+        else
+            pages = 512;
+
+        rv = hap_set_allocation(d, pages, NULL);
         if ( rv != 0 )
         {
             hap_set_allocation(d, 0, NULL);
-- 
1.8.3.1


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

Re: [RFC PATCH V2 1/4] xen/hap: Increase hap page pool size for more vcpus support

[Andrew Cooper]

On 31/08/17 06:01, Lan Tianyu wrote:
> This patch is to increase hap page pool size to support more vcpus in single VM.
>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  xen/arch/x86/mm/hap/hap.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index cdc77a9..96a7ed0 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -464,6 +464,7 @@ void hap_domain_init(struct domain *d)
>  int hap_enable(struct domain *d, u32 mode)
>  {
>      unsigned int old_pages;
> +    unsigned int pages;
>      unsigned int i;
>      int rv = 0;
>  
> @@ -473,7 +474,14 @@ int hap_enable(struct domain *d, u32 mode)
>      if ( old_pages == 0 )
>      {
>          paging_lock(d);
> -        rv = hap_set_allocation(d, 256, NULL);
> +
> +        /* Increase hap page pool with max vcpu number. */
> +        if ( d->max_vcpus > 128 )
> +            pages = 256;
> +        else
> +            pages = 512;
> +
> +        rv = hap_set_allocation(d, pages, NULL);

What effect is this intended to have?  hap_enable() is always called
when d->max_vcpus is 0.

d->max_vcpus isn't chosen until a subsequent hypercall.  (This is one of
many unexpected surprised from multi-vcpu support having been hacked on
the side of existing Xen support, rather than being built in to the
createdomain hypercall).

~Andrew

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

Re: [RFC PATCH V2 1/4] xen/hap: Increase hap page pool size for more vcpus support

[Lan Tianyu]

On 2017年08月31日 21:56, Andrew Cooper wrote:
> On 31/08/17 06:01, Lan Tianyu wrote:
>> This patch is to increase hap page pool size to support more vcpus in single VM.
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  xen/arch/x86/mm/hap/hap.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
>> index cdc77a9..96a7ed0 100644
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -464,6 +464,7 @@ void hap_domain_init(struct domain *d)
>>  int hap_enable(struct domain *d, u32 mode)
>>  {
>>      unsigned int old_pages;
>> +    unsigned int pages;
>>      unsigned int i;
>>      int rv = 0;
>>  
>> @@ -473,7 +474,14 @@ int hap_enable(struct domain *d, u32 mode)
>>      if ( old_pages == 0 )
>>      {
>>          paging_lock(d);
>> -        rv = hap_set_allocation(d, 256, NULL);
>> +
>> +        /* Increase hap page pool with max vcpu number. */
>> +        if ( d->max_vcpus > 128 )
>> +            pages = 256;
>> +        else
>> +            pages = 512;
>> +
>> +        rv = hap_set_allocation(d, pages, NULL);
> 
> What effect is this intended to have?  hap_enable() is always called
> when d->max_vcpus is 0.

Sorry. I didn't notice that max_vcpus wasn't set at that point.I hope to
allocate hap pages according vcpu number. This means we don't know how
many vcpu will be used when allocate hap pages during creating domain,
right? If that, we have to increase page number unconditionally.

> 
> d->max_vcpus isn't chosen until a subsequent hypercall.  (This is one of
> many unexpected surprised from multi-vcpu support having been hacked on
> the side of existing Xen support, rather than being built in to the
> createdomain hypercall).
> 
> ~Andrew
> 


-- 
Best regards
Tianyu Lan

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

Re: [RFC PATCH V2 1/4] xen/hap: Increase hap page pool size for more vcpus support

[Jan Beulich]

>>> On 01.09.17 at 10:19, <tianyu.lan@intel.com> wrote:
> On 2017年08月31日 21:56, Andrew Cooper wrote:
>> On 31/08/17 06:01, Lan Tianyu wrote:
>>> This patch is to increase hap page pool size to support more vcpus in single 
> VM.
>>>
>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>> ---
>>>  xen/arch/x86/mm/hap/hap.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
>>> index cdc77a9..96a7ed0 100644
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -464,6 +464,7 @@ void hap_domain_init(struct domain *d)
>>>  int hap_enable(struct domain *d, u32 mode)
>>>  {
>>>      unsigned int old_pages;
>>> +    unsigned int pages;
>>>      unsigned int i;
>>>      int rv = 0;
>>>  
>>> @@ -473,7 +474,14 @@ int hap_enable(struct domain *d, u32 mode)
>>>      if ( old_pages == 0 )
>>>      {
>>>          paging_lock(d);
>>> -        rv = hap_set_allocation(d, 256, NULL);
>>> +
>>> +        /* Increase hap page pool with max vcpu number. */
>>> +        if ( d->max_vcpus > 128 )
>>> +            pages = 256;
>>> +        else
>>> +            pages = 512;
>>> +
>>> +        rv = hap_set_allocation(d, pages, NULL);
>> 
>> What effect is this intended to have?  hap_enable() is always called
>> when d->max_vcpus is 0.
> 
> Sorry. I didn't notice that max_vcpus wasn't set at that point.I hope to
> allocate hap pages according vcpu number. This means we don't know how
> many vcpu will be used when allocate hap pages during creating domain,
> right? If that, we have to increase page number unconditionally.

But that you were already told isn't really acceptable. Did you
consider calling hap_set_allocation() another time once vCPU
count was set?

Jan

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

Re: [RFC PATCH V2 1/4] xen/hap: Increase hap page pool size for more vcpus support

[Lan Tianyu]

On 2017年09月01日 16:34, Jan Beulich wrote:
>>>> On 01.09.17 at 10:19, <tianyu.lan@intel.com> wrote:
>> On 2017年08月31日 21:56, Andrew Cooper wrote:
>>> On 31/08/17 06:01, Lan Tianyu wrote:
>>>> This patch is to increase hap page pool size to support more vcpus in single 
>> VM.
>>>>
>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>>> ---
>>>>  xen/arch/x86/mm/hap/hap.c | 10 +++++++++-
>>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
>>>> index cdc77a9..96a7ed0 100644
>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>> @@ -464,6 +464,7 @@ void hap_domain_init(struct domain *d)
>>>>  int hap_enable(struct domain *d, u32 mode)
>>>>  {
>>>>      unsigned int old_pages;
>>>> +    unsigned int pages;
>>>>      unsigned int i;
>>>>      int rv = 0;
>>>>  
>>>> @@ -473,7 +474,14 @@ int hap_enable(struct domain *d, u32 mode)
>>>>      if ( old_pages == 0 )
>>>>      {
>>>>          paging_lock(d);
>>>> -        rv = hap_set_allocation(d, 256, NULL);
>>>> +
>>>> +        /* Increase hap page pool with max vcpu number. */
>>>> +        if ( d->max_vcpus > 128 )
>>>> +            pages = 256;
>>>> +        else
>>>> +            pages = 512;
>>>> +
>>>> +        rv = hap_set_allocation(d, pages, NULL);
>>>
>>> What effect is this intended to have?  hap_enable() is always called
>>> when d->max_vcpus is 0.
>>
>> Sorry. I didn't notice that max_vcpus wasn't set at that point.I hope to
>> allocate hap pages according vcpu number. This means we don't know how
>> many vcpu will be used when allocate hap pages during creating domain,
>> right? If that, we have to increase page number unconditionally.
> 
> But that you were already told isn't really acceptable. Did you
> consider calling hap_set_allocation() another time once vCPU
> count was set?
> 

That sounds feasible. I will try it. Thanks.


-- 
Best regards
Tianyu Lan

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

[RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Lan Tianyu]

This patch is to change DSDT table for processor object to support >128 vcpus
accroding to ACPI spec 8.4 Declaring Processors

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index 2daf32c..6c4c325 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -24,6 +24,8 @@
 #include <xen/arch-arm.h>
 #endif
 
+#define CPU_NAME_FMT      "P%.03X"
+
 static unsigned int indent_level;
 static bool debug = false;
 
@@ -196,10 +198,14 @@ int main(int argc, char **argv)
     /* Define processor objects and control methods. */
     for ( cpu = 0; cpu < max_cpus; cpu++)
     {
-        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
+        unsigned int apic_id = cpu * 2;
 
-        stmt("Name", "_HID, \"ACPI0007\"");
+        if ( apic_id > 255 )
+            push_block("Device", CPU_NAME_FMT, cpu);
+        else
+            push_block("Processor", CPU_NAME_FMT", %d, 0x0000b010, 0x06", cpu, cpu);
 
+        stmt("Name", "_HID, \"ACPI0007\"");
         stmt("Name", "_UID, %d", cpu);
 #ifdef CONFIG_ARM_64
         pop_block();
@@ -268,15 +274,15 @@ int main(int argc, char **argv)
         /* Extract current CPU's status: 0=offline; 1=online. */
         stmt("And", "Local1, 1, Local2");
         /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
-        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
+        push_block("If", "LNotEqual(Local2, \\_SB."CPU_NAME_FMT ".FLG)", cpu);
         /* ...If not, update it and the MADT checksum, and notify OSPM. */
-        stmt("Store", "Local2, \\_SB.PR%02X.FLG", cpu);
+        stmt("Store", "Local2, \\_SB."CPU_NAME_FMT".FLG", cpu);
         push_block("If", "LEqual(Local2, 1)");
-        stmt("Notify", "PR%02X, 1", cpu); /* Notify: Device Check */
+        stmt("Notify", CPU_NAME_FMT", 1", cpu); /* Notify: Device Check */
         stmt("Subtract", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
         pop_block();
         push_block("Else", NULL);
-        stmt("Notify", "PR%02X, 3", cpu); /* Notify: Eject Request */
+        stmt("Notify", CPU_NAME_FMT", 3", cpu); /* Notify: Eject Request */
         stmt("Add", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
         pop_block();
         pop_block();
-- 
1.8.3.1


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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Roger Pau Monné]

On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
> This patch is to change DSDT table for processor object to support >128 vcpus
> accroding to ACPI spec 8.4 Declaring Processors
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> index 2daf32c..6c4c325 100644
> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -24,6 +24,8 @@
>  #include <xen/arch-arm.h>
>  #endif
>  
> +#define CPU_NAME_FMT      "P%.03X"
> +
>  static unsigned int indent_level;
>  static bool debug = false;
>  
> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
>      /* Define processor objects and control methods. */
>      for ( cpu = 0; cpu < max_cpus; cpu++)
>      {
> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
> +        unsigned int apic_id = cpu * 2;

This is fragile, ideally there should be a single point where the APIC
ID is calculated. Although there are already two places where the APIC
ID is calculated, in hvmloader and libxl.

And I'm not sure how to use any of those here in order to avoid
introducing a third one.

>  
> -        stmt("Name", "_HID, \"ACPI0007\"");
> +        if ( apic_id > 255 )

We need to be careful with this. This is not a problem ATM because the
ACPI ID is the CPU ID, but care should be taken to not create a
Processor object with ACPI ID 255, because that's the broadcast ACPI
ID...

> +            push_block("Device", CPU_NAME_FMT, cpu);
> +        else

... IMHO an assert(cpu < 255); should be added here.

> +            push_block("Processor", CPU_NAME_FMT", %d, 0x0000b010, 0x06", cpu, cpu);
                                                   ^ space (here and below)

Please leave a space between the string literals and the defines, it
makes it easier to read. And this line needs to be split.

>  
> +        stmt("Name", "_HID, \"ACPI0007\"");
>          stmt("Name", "_UID, %d", cpu);
>  #ifdef CONFIG_ARM_64
>          pop_block();
> @@ -268,15 +274,15 @@ int main(int argc, char **argv)
>          /* Extract current CPU's status: 0=offline; 1=online. */
>          stmt("And", "Local1, 1, Local2");
>          /* Check if status is up-to-date in the relevant MADT LAPIC entry... */
> -        push_block("If", "LNotEqual(Local2, \\_SB.PR%02X.FLG)", cpu);
> +        push_block("If", "LNotEqual(Local2, \\_SB."CPU_NAME_FMT ".FLG)", cpu);
>          /* ...If not, update it and the MADT checksum, and notify OSPM. */
> -        stmt("Store", "Local2, \\_SB.PR%02X.FLG", cpu);
> +        stmt("Store", "Local2, \\_SB."CPU_NAME_FMT".FLG", cpu);
>          push_block("If", "LEqual(Local2, 1)");
> -        stmt("Notify", "PR%02X, 1", cpu); /* Notify: Device Check */
> +        stmt("Notify", CPU_NAME_FMT", 1", cpu); /* Notify: Device Check */
>          stmt("Subtract", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
>          pop_block();
>          push_block("Else", NULL);
> -        stmt("Notify", "PR%02X, 3", cpu); /* Notify: Eject Request */
> +        stmt("Notify", CPU_NAME_FMT", 3", cpu); /* Notify: Eject Request */
>          stmt("Add", "\\_SB.MSU, 1, \\_SB.MSU"); /* Adjust MADT csum */
>          pop_block();
>          pop_block();
> -- 
> 1.8.3.1
> 

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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Lan Tianyu]

On 2017年08月31日 23:38, Roger Pau Monné wrote:
> On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
>> This patch is to change DSDT table for processor object to support >128 vcpus
>> accroding to ACPI spec 8.4 Declaring Processors
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
>> index 2daf32c..6c4c325 100644
>> --- a/tools/libacpi/mk_dsdt.c
>> +++ b/tools/libacpi/mk_dsdt.c
>> @@ -24,6 +24,8 @@
>>  #include <xen/arch-arm.h>
>>  #endif
>>  
>> +#define CPU_NAME_FMT      "P%.03X"
>> +
>>  static unsigned int indent_level;
>>  static bool debug = false;
>>  
>> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
>>      /* Define processor objects and control methods. */
>>      for ( cpu = 0; cpu < max_cpus; cpu++)
>>      {
>> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
>> +        unsigned int apic_id = cpu * 2;
> 
> This is fragile, ideally there should be a single point where the APIC
> ID is calculated. Although there are already two places where the APIC
> ID is calculated, in hvmloader and libxl.
> 
> And I'm not sure how to use any of those here in order to avoid
> introducing a third one.

The mk_dsdt is independent tool to build dsdt table. It wasn't linked
with libxl and hvmloader. We can't reuse old function to do that.

But I think we may introduce a new LAPIC_ID(vcpu) in the arch head
file(i.e, #include <xen/arch-x86/xen.h>) and replace old ones.

> 
>>  
>> -        stmt("Name", "_HID, \"ACPI0007\"");
>> +        if ( apic_id > 255 )
> 
> We need to be careful with this. This is not a problem ATM because the
> ACPI ID is the CPU ID, but care should be taken to not create a
> Processor object with ACPI ID 255, because that's the broadcast ACPI
> ID...

Yes.

> 
>> +            push_block("Device", CPU_NAME_FMT, cpu);
>> +        else
> 
> ... IMHO an assert(cpu < 255); should be added here.

OK.

> 
>> +            push_block("Processor", CPU_NAME_FMT", %d, 0x0000b010, 0x06", cpu, cpu);
>                                                    ^ space (here and below)
> 
> Please leave a space between the string literals and the defines, it
> makes it easier to read. And this line needs to be split.
> 

OK. Will update.


-- 
Best regards
Tianyu Lan

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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Roger Pau Monné]

On Fri, Sep 01, 2017 at 10:54:02AM +0800, Lan Tianyu wrote:
> On 2017年08月31日 23:38, Roger Pau Monné wrote:
> > On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
> >> This patch is to change DSDT table for processor object to support >128 vcpus
> >> accroding to ACPI spec 8.4 Declaring Processors
> >>
> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >> ---
> >>  tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
> >>  1 file changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> >> index 2daf32c..6c4c325 100644
> >> --- a/tools/libacpi/mk_dsdt.c
> >> +++ b/tools/libacpi/mk_dsdt.c
> >> @@ -24,6 +24,8 @@
> >>  #include <xen/arch-arm.h>
> >>  #endif
> >>  
> >> +#define CPU_NAME_FMT      "P%.03X"
> >> +
> >>  static unsigned int indent_level;
> >>  static bool debug = false;
> >>  
> >> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
> >>      /* Define processor objects and control methods. */
> >>      for ( cpu = 0; cpu < max_cpus; cpu++)
> >>      {
> >> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
> >> +        unsigned int apic_id = cpu * 2;
> > 
> > This is fragile, ideally there should be a single point where the APIC
> > ID is calculated. Although there are already two places where the APIC
> > ID is calculated, in hvmloader and libxl.
> > 
> > And I'm not sure how to use any of those here in order to avoid
> > introducing a third one.
> 
> The mk_dsdt is independent tool to build dsdt table. It wasn't linked
> with libxl and hvmloader. We can't reuse old function to do that.
> 
> But I think we may introduce a new LAPIC_ID(vcpu) in the arch head
> file(i.e, #include <xen/arch-x86/xen.h>) and replace old ones.

There's already a LAPIC_ID macro in hvmloader headers which should be
placed somewhere suitable. What about removing the lapic_id hook from
acpi_config and placing the LAPIC_ID macro in the libacpi.h header?

I'm not sure why lapic_id needs to be a hook in any case, both it's
callers use the same exact formula (cpu_id * 2).

Thanks, Roger.

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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Jan Beulich]

>>> On 01.09.17 at 11:41, <roger.pau@citrix.com> wrote:
> I'm not sure why lapic_id needs to be a hook in any case, both it's
> callers use the same exact formula (cpu_id * 2).

You're aware that this is meant to be improved rather sooner than
later. At that point I'm then not sure a suitable LAPIC_ID() macro
could be created, as the potential users of this may not have
available all necessary inputs. I'd strongly suggest finding a way
to dynamically insert the APIC IDs (as found while booting) into
the ACPI tables. That's pretty likely how real BIOSes have to do
it too.

Jan


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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Roger Pau Monné]

On Fri, Sep 01, 2017 at 04:10:26AM -0600, Jan Beulich wrote:
> >>> On 01.09.17 at 11:41, <roger.pau@citrix.com> wrote:
> > I'm not sure why lapic_id needs to be a hook in any case, both it's
> > callers use the same exact formula (cpu_id * 2).
> 
> You're aware that this is meant to be improved rather sooner than
> later. At that point I'm then not sure a suitable LAPIC_ID() macro
> could be created, as the potential users of this may not have
> available all necessary inputs. I'd strongly suggest finding a way
> to dynamically insert the APIC IDs (as found while booting) into
> the ACPI tables. That's pretty likely how real BIOSes have to do
> it too.

There's a further problem here, which is that the DSDT is created at
tools compilation time, there's no way to know the APIC IDs at that
point in order to decide whether a CPU gets a Processor or a Device
object.

Leaving the DSDT issue aside, there's also the problem that the MADT
for PVH guests is build by the toolstack, not the firmware (because
there's none), so it's impossible to do it like real BIOSes.

So AFAICT we either remove the hook and code the logic to get the ACPI
ID inside libacpi, or we introduce yet another open coded one in order
to generate the DSDT from mk_dsdt.c

Roger.

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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Lan Tianyu]

On 2017年09月01日 17:41, Roger Pau Monné wrote:
> On Fri, Sep 01, 2017 at 10:54:02AM +0800, Lan Tianyu wrote:
>> On 2017年08月31日 23:38, Roger Pau Monné wrote:
>>> On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
>>>> This patch is to change DSDT table for processor object to support >128 vcpus
>>>> accroding to ACPI spec 8.4 Declaring Processors
>>>>
>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>>> ---
>>>>  tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
>>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
>>>> index 2daf32c..6c4c325 100644
>>>> --- a/tools/libacpi/mk_dsdt.c
>>>> +++ b/tools/libacpi/mk_dsdt.c
>>>> @@ -24,6 +24,8 @@
>>>>  #include <xen/arch-arm.h>
>>>>  #endif
>>>>  
>>>> +#define CPU_NAME_FMT      "P%.03X"
>>>> +
>>>>  static unsigned int indent_level;
>>>>  static bool debug = false;
>>>>  
>>>> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
>>>>      /* Define processor objects and control methods. */
>>>>      for ( cpu = 0; cpu < max_cpus; cpu++)
>>>>      {
>>>> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
>>>> +        unsigned int apic_id = cpu * 2;
>>>
>>> This is fragile, ideally there should be a single point where the APIC
>>> ID is calculated. Although there are already two places where the APIC
>>> ID is calculated, in hvmloader and libxl.
>>>
>>> And I'm not sure how to use any of those here in order to avoid
>>> introducing a third one.
>>
>> The mk_dsdt is independent tool to build dsdt table. It wasn't linked
>> with libxl and hvmloader. We can't reuse old function to do that.
>>
>> But I think we may introduce a new LAPIC_ID(vcpu) in the arch head
>> file(i.e, #include <xen/arch-x86/xen.h>) and replace old ones.
> 
> There's already a LAPIC_ID macro in hvmloader headers which should be
> placed somewhere suitable.

Yes, this is what I mentioned.

> What about removing the lapic_id hook from
> acpi_config and placing the LAPIC_ID macro in the libacpi.h header?

I think this should be ARCH specific. I am not sure whether ARM follows
rule of "apic_id = vcpu_id *2".

Julien, could you give some inputs? Thanks.



> 
> I'm not sure why lapic_id needs to be a hook in any case, both it's
> callers use the same exact formula (cpu_id * 2).
> 
> Thanks, Roger.
> 


-- 
Best regards
Tianyu Lan

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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Roger Pau Monné]

On Mon, Sep 04, 2017 at 11:07:14AM +0800, Lan Tianyu wrote:
> On 2017年09月01日 17:41, Roger Pau Monné wrote:
> > On Fri, Sep 01, 2017 at 10:54:02AM +0800, Lan Tianyu wrote:
> >> On 2017年08月31日 23:38, Roger Pau Monné wrote:
> >>> On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
> >>>> This patch is to change DSDT table for processor object to support >128 vcpus
> >>>> accroding to ACPI spec 8.4 Declaring Processors
> >>>>
> >>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >>>> ---
> >>>>  tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
> >>>>  1 file changed, 12 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> >>>> index 2daf32c..6c4c325 100644
> >>>> --- a/tools/libacpi/mk_dsdt.c
> >>>> +++ b/tools/libacpi/mk_dsdt.c
> >>>> @@ -24,6 +24,8 @@
> >>>>  #include <xen/arch-arm.h>
> >>>>  #endif
> >>>>  
> >>>> +#define CPU_NAME_FMT      "P%.03X"
> >>>> +
> >>>>  static unsigned int indent_level;
> >>>>  static bool debug = false;
> >>>>  
> >>>> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
> >>>>      /* Define processor objects and control methods. */
> >>>>      for ( cpu = 0; cpu < max_cpus; cpu++)
> >>>>      {
> >>>> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
> >>>> +        unsigned int apic_id = cpu * 2;
> >>>
> >>> This is fragile, ideally there should be a single point where the APIC
> >>> ID is calculated. Although there are already two places where the APIC
> >>> ID is calculated, in hvmloader and libxl.
> >>>
> >>> And I'm not sure how to use any of those here in order to avoid
> >>> introducing a third one.
> >>
> >> The mk_dsdt is independent tool to build dsdt table. It wasn't linked
> >> with libxl and hvmloader. We can't reuse old function to do that.
> >>
> >> But I think we may introduce a new LAPIC_ID(vcpu) in the arch head
> >> file(i.e, #include <xen/arch-x86/xen.h>) and replace old ones.
> > 
> > There's already a LAPIC_ID macro in hvmloader headers which should be
> > placed somewhere suitable.
> 
> Yes, this is what I mentioned.

Jan has expressed some concerns with removing the hook, see:

<59A94E320200007800176754@prv-mh.provo.novell.com>

> > What about removing the lapic_id hook from
> > acpi_config and placing the LAPIC_ID macro in the libacpi.h header?
> 
> I think this should be ARCH specific. I am not sure whether ARM follows
> rule of "apic_id = vcpu_id *2".
> 
> Julien, could you give some inputs? Thanks.

AFAIK ARM doesn't have a local APIC, so there are no xAPIC/x2APIC
entries in the ARM MADT.

Roger.

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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Lan Tianyu]

On 2017年09月04日 17:05, Roger Pau Monné wrote:
> On Mon, Sep 04, 2017 at 11:07:14AM +0800, Lan Tianyu wrote:
>> On 2017年09月01日 17:41, Roger Pau Monné wrote:
>>> On Fri, Sep 01, 2017 at 10:54:02AM +0800, Lan Tianyu wrote:
>>>> On 2017年08月31日 23:38, Roger Pau Monné wrote:
>>>>> On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
>>>>>> This patch is to change DSDT table for processor object to support >128 vcpus
>>>>>> accroding to ACPI spec 8.4 Declaring Processors
>>>>>>
>>>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>>>>> ---
>>>>>>  tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
>>>>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
>>>>>> index 2daf32c..6c4c325 100644
>>>>>> --- a/tools/libacpi/mk_dsdt.c
>>>>>> +++ b/tools/libacpi/mk_dsdt.c
>>>>>> @@ -24,6 +24,8 @@
>>>>>>  #include <xen/arch-arm.h>
>>>>>>  #endif
>>>>>>  
>>>>>> +#define CPU_NAME_FMT      "P%.03X"
>>>>>> +
>>>>>>  static unsigned int indent_level;
>>>>>>  static bool debug = false;
>>>>>>  
>>>>>> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
>>>>>>      /* Define processor objects and control methods. */
>>>>>>      for ( cpu = 0; cpu < max_cpus; cpu++)
>>>>>>      {
>>>>>> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
>>>>>> +        unsigned int apic_id = cpu * 2;
>>>>>
>>>>> This is fragile, ideally there should be a single point where the APIC
>>>>> ID is calculated. Although there are already two places where the APIC
>>>>> ID is calculated, in hvmloader and libxl.
>>>>>
>>>>> And I'm not sure how to use any of those here in order to avoid
>>>>> introducing a third one.
>>>>
>>>> The mk_dsdt is independent tool to build dsdt table. It wasn't linked
>>>> with libxl and hvmloader. We can't reuse old function to do that.
>>>>
>>>> But I think we may introduce a new LAPIC_ID(vcpu) in the arch head
>>>> file(i.e, #include <xen/arch-x86/xen.h>) and replace old ones.
>>>
>>> There's already a LAPIC_ID macro in hvmloader headers which should be
>>> placed somewhere suitable.
>>
>> Yes, this is what I mentioned.
> 
> Jan has expressed some concerns with removing the hook, see:
> 
> <59A94E320200007800176754@prv-mh.provo.novell.com>

So we still need to introduce LAPIC_ID() here, right?

> 
>>> What about removing the lapic_id hook from
>>> acpi_config and placing the LAPIC_ID macro in the libacpi.h header?
>>
>> I think this should be ARCH specific. I am not sure whether ARM follows
>> rule of "apic_id = vcpu_id *2".
>>
>> Julien, could you give some inputs? Thanks.
> 
> AFAIK ARM doesn't have a local APIC, so there are no xAPIC/x2APIC
> entries in the ARM MADT.
> 




-- 
Best regards
Tianyu Lan

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

Re: [RFC PATCH V2 2/4] Tool/ACPI: DSDT extension to support more vcpus

[Julien Grall]

Hi,

Sorry for the late reply.

On 04/09/17 10:05, Roger Pau Monné wrote:
> On Mon, Sep 04, 2017 at 11:07:14AM +0800, Lan Tianyu wrote:
>> On 2017年09月01日 17:41, Roger Pau Monné wrote:
>>> On Fri, Sep 01, 2017 at 10:54:02AM +0800, Lan Tianyu wrote:
>>>> On 2017年08月31日 23:38, Roger Pau Monné wrote:
>>>>> On Thu, Aug 31, 2017 at 01:01:47AM -0400, Lan Tianyu wrote:
>>>>>> This patch is to change DSDT table for processor object to support >128 vcpus
>>>>>> accroding to ACPI spec 8.4 Declaring Processors
>>>>>>
>>>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>>>>> ---
>>>>>>   tools/libacpi/mk_dsdt.c | 18 ++++++++++++------
>>>>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
>>>>>> index 2daf32c..6c4c325 100644
>>>>>> --- a/tools/libacpi/mk_dsdt.c
>>>>>> +++ b/tools/libacpi/mk_dsdt.c
>>>>>> @@ -24,6 +24,8 @@
>>>>>>   #include <xen/arch-arm.h>
>>>>>>   #endif
>>>>>>   
>>>>>> +#define CPU_NAME_FMT      "P%.03X"
>>>>>> +
>>>>>>   static unsigned int indent_level;
>>>>>>   static bool debug = false;
>>>>>>   
>>>>>> @@ -196,10 +198,14 @@ int main(int argc, char **argv)
>>>>>>       /* Define processor objects and control methods. */
>>>>>>       for ( cpu = 0; cpu < max_cpus; cpu++)
>>>>>>       {
>>>>>> -        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu);
>>>>>> +        unsigned int apic_id = cpu * 2;
>>>>>
>>>>> This is fragile, ideally there should be a single point where the APIC
>>>>> ID is calculated. Although there are already two places where the APIC
>>>>> ID is calculated, in hvmloader and libxl.
>>>>>
>>>>> And I'm not sure how to use any of those here in order to avoid
>>>>> introducing a third one.
>>>>
>>>> The mk_dsdt is independent tool to build dsdt table. It wasn't linked
>>>> with libxl and hvmloader. We can't reuse old function to do that.
>>>>
>>>> But I think we may introduce a new LAPIC_ID(vcpu) in the arch head
>>>> file(i.e, #include <xen/arch-x86/xen.h>) and replace old ones.
>>>
>>> There's already a LAPIC_ID macro in hvmloader headers which should be
>>> placed somewhere suitable.
>>
>> Yes, this is what I mentioned.
> 
> Jan has expressed some concerns with removing the hook, see:
> 
> <59A94E320200007800176754@prv-mh.provo.novell.com>
> 
>>> What about removing the lapic_id hook from
>>> acpi_config and placing the LAPIC_ID macro in the libacpi.h header?
>>
>> I think this should be ARCH specific. I am not sure whether ARM follows
>> rule of "apic_id = vcpu_id *2".
>>
>> Julien, could you give some inputs? Thanks.
> 
> AFAIK ARM doesn't have a local APIC, so there are no xAPIC/x2APIC
> entries in the ARM MADT.

That's correct. ARM does not use xAPIC/x2APIC tables.

Cheers,

-- 
Julien Grall

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

[RFC PATCH V2 3/4] hvmload: Add x2apic entry support in the MADT build

[Lan Tianyu]

This patch is to add x2apic entry support for ACPI MADT table
according to ACPI spec 5.2.12.12 Processor Local x2APIC Structure

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 tools/libacpi/acpi2_0.h | 10 +++++++++
 tools/libacpi/build.c   | 59 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
index 758a823..caa3682 100644
--- a/tools/libacpi/acpi2_0.h
+++ b/tools/libacpi/acpi2_0.h
@@ -322,6 +322,7 @@ struct acpi_20_waet {
 #define ACPI_IO_SAPIC                       0x06
 #define ACPI_PROCESSOR_LOCAL_SAPIC          0x07
 #define ACPI_PLATFORM_INTERRUPT_SOURCES     0x08
+#define ACPI_PROCESSOR_LOCAL_X2APIC         0x09
 
 /*
  * APIC Structure Definitions.
@@ -338,6 +339,15 @@ struct acpi_20_madt_lapic {
     uint32_t flags;
 };
 
+struct acpi_20_madt_x2apic {
+    uint8_t  type;
+    uint8_t  length;
+    uint16_t reserved;          /* reserved - must be zero */
+    uint32_t apic_id;           /* Processor x2APIC ID  */
+    uint32_t flags;
+    uint32_t acpi_processor_id; /* ACPI processor UID */
+};
+
 /*
  * Local APIC Flags.  All other bits are reserved and must be 0.
  */
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index c7cc784..0c95850 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -82,9 +82,9 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
     struct acpi_20_madt           *madt;
     struct acpi_20_madt_intsrcovr *intsrcovr;
     struct acpi_20_madt_ioapic    *io_apic;
-    struct acpi_20_madt_lapic     *lapic;
     const struct hvm_info_table   *hvminfo = config->hvminfo;
     int i, sz;
+    void *end;
 
     if ( config->lapic_id == NULL )
         return NULL;
@@ -92,7 +92,12 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
     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) * hvminfo->nr_vcpus;
+
+    if (hvminfo->nr_vcpus < 128)
+        sz += sizeof(struct acpi_20_madt_lapic) * hvminfo->nr_vcpus;
+    else
+        sz += sizeof(struct acpi_20_madt_lapic) * 128 +
+              sizeof(struct acpi_20_madt_x2apic) * (hvminfo->nr_vcpus - 128);
 
     madt = ctxt->mem_ops.alloc(ctxt, sz, 16);
     if (!madt) return NULL;
@@ -109,7 +114,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
     madt->flags      = ACPI_PCAT_COMPAT;
 
     if ( config->table_flags & ACPI_HAS_IOAPIC )
-    {     
+    {
         intsrcovr = (struct acpi_20_madt_intsrcovr *)(madt + 1);
         for ( i = 0; i < 16; i++ )
         {
@@ -146,27 +151,47 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
         io_apic->ioapic_id   = config->ioapic_id;
         io_apic->ioapic_addr = config->ioapic_base_address;
 
-        lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
+        end = (struct acpi_20_madt_lapic *)(io_apic + 1);
     }
     else
-        lapic = (struct acpi_20_madt_lapic *)(madt + 1);
+        end = (struct acpi_20_madt_lapic *)(madt + 1);
+
+    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, end);
 
-    info->nr_cpus = hvminfo->nr_vcpus;
-    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, lapic);
     for ( i = 0; i < hvminfo->nr_vcpus; i++ )
     {
-        memset(lapic, 0, sizeof(*lapic));
-        lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
-        lapic->length  = sizeof(*lapic);
-        /* Processor ID must match processor-object IDs in the DSDT. */
-        lapic->acpi_processor_id = i;
-        lapic->apic_id = config->lapic_id(i);
-        lapic->flags = (test_bit(i, hvminfo->vcpu_online)
-                        ? ACPI_LOCAL_APIC_ENABLED : 0);
-        lapic++;
+        unsigned int apic_id = config->lapic_id(i);
+
+        if ( apic_id < 255 ) {
+            struct acpi_20_madt_lapic *lapic = end;
+
+            memset(lapic, 0, sizeof(*lapic));
+            lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
+            lapic->length  = sizeof(*lapic);
+            /* Processor ID must match processor-object IDs in the DSDT. */
+            lapic->acpi_processor_id = i;
+            lapic->apic_id = config->lapic_id(i);
+            lapic->flags = ((i < hvminfo->nr_vcpus) &&
+                            test_bit(i, hvminfo->vcpu_online)
+                            ? ACPI_LOCAL_APIC_ENABLED : 0);
+            end = ++lapic;
+        } else {
+            struct acpi_20_madt_x2apic *lapic = end;
+
+            memset(lapic, 0, sizeof(*lapic));
+            lapic->type    = ACPI_PROCESSOR_LOCAL_X2APIC;
+            lapic->length  = sizeof(*lapic);
+            /* Processor ID must match processor-object IDs in the DSDT. */
+            lapic->acpi_processor_id = i;
+            lapic->apic_id = config->lapic_id(i);
+            lapic->flags =  test_bit(i, hvminfo->vcpu_online)
+                            ? ACPI_LOCAL_APIC_ENABLED : 0;
+            end = ++lapic;
+        }
     }
 
-    madt->header.length = (unsigned char *)lapic - (unsigned char *)madt;
+    info->nr_cpus = hvminfo->nr_vcpus;
+    madt->header.length = (unsigned char *)end - (unsigned char *)madt;
     set_checksum(madt, offsetof(struct acpi_header, checksum),
                  madt->header.length);
     info->madt_csum_addr =
-- 
1.8.3.1


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

Re: [RFC PATCH V2 3/4] hvmload: Add x2apic entry support in the MADT build

[Roger Pau Monné]

On Thu, Aug 31, 2017 at 01:01:48AM -0400, Lan Tianyu wrote:
> This patch is to add x2apic entry support for ACPI MADT table
> according to ACPI spec 5.2.12.12 Processor Local x2APIC Structure
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  tools/libacpi/acpi2_0.h | 10 +++++++++
>  tools/libacpi/build.c   | 59 +++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> index 758a823..caa3682 100644
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -322,6 +322,7 @@ struct acpi_20_waet {
>  #define ACPI_IO_SAPIC                       0x06
>  #define ACPI_PROCESSOR_LOCAL_SAPIC          0x07
>  #define ACPI_PLATFORM_INTERRUPT_SOURCES     0x08
> +#define ACPI_PROCESSOR_LOCAL_X2APIC         0x09
>  
>  /*
>   * APIC Structure Definitions.
> @@ -338,6 +339,15 @@ struct acpi_20_madt_lapic {
>      uint32_t flags;
>  };
>  
> +struct acpi_20_madt_x2apic {
> +    uint8_t  type;
> +    uint8_t  length;
> +    uint16_t reserved;          /* reserved - must be zero */
> +    uint32_t apic_id;           /* Processor x2APIC ID  */
> +    uint32_t flags;
> +    uint32_t acpi_processor_id; /* ACPI processor UID */
> +};
> +
>  /*
>   * Local APIC Flags.  All other bits are reserved and must be 0.
>   */
> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index c7cc784..0c95850 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -82,9 +82,9 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>      struct acpi_20_madt           *madt;
>      struct acpi_20_madt_intsrcovr *intsrcovr;
>      struct acpi_20_madt_ioapic    *io_apic;
> -    struct acpi_20_madt_lapic     *lapic;
>      const struct hvm_info_table   *hvminfo = config->hvminfo;
>      int i, sz;
> +    void *end;
>  
>      if ( config->lapic_id == NULL )
>          return NULL;
> @@ -92,7 +92,12 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>      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) * hvminfo->nr_vcpus;
> +
> +    if (hvminfo->nr_vcpus < 128)

This should be done based on APIC ID.

> +        sz += sizeof(struct acpi_20_madt_lapic) * hvminfo->nr_vcpus;
> +    else
> +        sz += sizeof(struct acpi_20_madt_lapic) * 128 +
> +              sizeof(struct acpi_20_madt_x2apic) * (hvminfo->nr_vcpus - 128);
>  
>      madt = ctxt->mem_ops.alloc(ctxt, sz, 16);
>      if (!madt) return NULL;
> @@ -109,7 +114,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>      madt->flags      = ACPI_PCAT_COMPAT;
>  
>      if ( config->table_flags & ACPI_HAS_IOAPIC )
> -    {     
> +    {

Spurious cleanup?

>          intsrcovr = (struct acpi_20_madt_intsrcovr *)(madt + 1);
>          for ( i = 0; i < 16; i++ )
>          {
> @@ -146,27 +151,47 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>          io_apic->ioapic_id   = config->ioapic_id;
>          io_apic->ioapic_addr = config->ioapic_base_address;
>  
> -        lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
> +        end = (struct acpi_20_madt_lapic *)(io_apic + 1);
>      }
>      else
> -        lapic = (struct acpi_20_madt_lapic *)(madt + 1);
> +        end = (struct acpi_20_madt_lapic *)(madt + 1);
> +
> +    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, end);
>  
> -    info->nr_cpus = hvminfo->nr_vcpus;

Why are you moving this? AFAICT the value of nr_vpcus is not changed,
so you might as well leave it as-is.

> -    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, lapic);
>      for ( i = 0; i < hvminfo->nr_vcpus; i++ )
>      {
> -        memset(lapic, 0, sizeof(*lapic));
> -        lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> -        lapic->length  = sizeof(*lapic);
> -        /* Processor ID must match processor-object IDs in the DSDT. */
> -        lapic->acpi_processor_id = i;
> -        lapic->apic_id = config->lapic_id(i);
> -        lapic->flags = (test_bit(i, hvminfo->vcpu_online)
> -                        ? ACPI_LOCAL_APIC_ENABLED : 0);
> -        lapic++;
> +        unsigned int apic_id = config->lapic_id(i);
> +
> +        if ( apic_id < 255 ) {
> +            struct acpi_20_madt_lapic *lapic = end;
> +
> +            memset(lapic, 0, sizeof(*lapic));
> +            lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> +            lapic->length  = sizeof(*lapic);
> +            /* Processor ID must match processor-object IDs in the DSDT. */
> +            lapic->acpi_processor_id = i;
> +            lapic->apic_id = config->lapic_id(i);
> +            lapic->flags = ((i < hvminfo->nr_vcpus) &&

I don't understand why you have added this.

Roger.

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

Re: [RFC PATCH V2 3/4] hvmload: Add x2apic entry support in the MADT build

[Lan Tianyu]

On 2017年09月01日 17:57, Roger Pau Monné wrote:
> On Thu, Aug 31, 2017 at 01:01:48AM -0400, Lan Tianyu wrote:
>> This patch is to add x2apic entry support for ACPI MADT table
>> according to ACPI spec 5.2.12.12 Processor Local x2APIC Structure
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  tools/libacpi/acpi2_0.h | 10 +++++++++
>>  tools/libacpi/build.c   | 59 +++++++++++++++++++++++++++++++++++--------------
>>  2 files changed, 52 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
>> index 758a823..caa3682 100644
>> --- a/tools/libacpi/acpi2_0.h
>> +++ b/tools/libacpi/acpi2_0.h
>> @@ -322,6 +322,7 @@ struct acpi_20_waet {
>>  #define ACPI_IO_SAPIC                       0x06
>>  #define ACPI_PROCESSOR_LOCAL_SAPIC          0x07
>>  #define ACPI_PLATFORM_INTERRUPT_SOURCES     0x08
>> +#define ACPI_PROCESSOR_LOCAL_X2APIC         0x09
>>  
>>  /*
>>   * APIC Structure Definitions.
>> @@ -338,6 +339,15 @@ struct acpi_20_madt_lapic {
>>      uint32_t flags;
>>  };
>>  
>> +struct acpi_20_madt_x2apic {
>> +    uint8_t  type;
>> +    uint8_t  length;
>> +    uint16_t reserved;          /* reserved - must be zero */
>> +    uint32_t apic_id;           /* Processor x2APIC ID  */
>> +    uint32_t flags;
>> +    uint32_t acpi_processor_id; /* ACPI processor UID */
>> +};
>> +
>>  /*
>>   * Local APIC Flags.  All other bits are reserved and must be 0.
>>   */
>> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
>> index c7cc784..0c95850 100644
>> --- a/tools/libacpi/build.c
>> +++ b/tools/libacpi/build.c
>> @@ -82,9 +82,9 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>>      struct acpi_20_madt           *madt;
>>      struct acpi_20_madt_intsrcovr *intsrcovr;
>>      struct acpi_20_madt_ioapic    *io_apic;
>> -    struct acpi_20_madt_lapic     *lapic;
>>      const struct hvm_info_table   *hvminfo = config->hvminfo;
>>      int i, sz;
>> +    void *end;
>>  
>>      if ( config->lapic_id == NULL )
>>          return NULL;
>> @@ -92,7 +92,12 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>>      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) * hvminfo->nr_vcpus;
>> +
>> +    if (hvminfo->nr_vcpus < 128)
> 
> This should be done based on APIC ID.

There will be a problem how to get max apic id. Should we use the max
vcpu index to get max APIC id?

> 
>> +        sz += sizeof(struct acpi_20_madt_lapic) * hvminfo->nr_vcpus;
>> +    else
>> +        sz += sizeof(struct acpi_20_madt_lapic) * 128 +
>> +              sizeof(struct acpi_20_madt_x2apic) * (hvminfo->nr_vcpus - 128);
>>  
>>      madt = ctxt->mem_ops.alloc(ctxt, sz, 16);
>>      if (!madt) return NULL;
>> @@ -109,7 +114,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>>      madt->flags      = ACPI_PCAT_COMPAT;
>>  
>>      if ( config->table_flags & ACPI_HAS_IOAPIC )
>> -    {     
>> +    {
> 
> Spurious cleanup?
> 
>>          intsrcovr = (struct acpi_20_madt_intsrcovr *)(madt + 1);
>>          for ( i = 0; i < 16; i++ )
>>          {
>> @@ -146,27 +151,47 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>>          io_apic->ioapic_id   = config->ioapic_id;
>>          io_apic->ioapic_addr = config->ioapic_base_address;
>>  
>> -        lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
>> +        end = (struct acpi_20_madt_lapic *)(io_apic + 1);
>>      }
>>      else
>> -        lapic = (struct acpi_20_madt_lapic *)(madt + 1);
>> +        end = (struct acpi_20_madt_lapic *)(madt + 1);
>> +
>> +    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, end);
>>  
>> -    info->nr_cpus = hvminfo->nr_vcpus;
> 
> Why are you moving this? AFAICT the value of nr_vpcus is not changed,
> so you might as well leave it as-is.

OK.

> 
>> -    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, lapic);
>>      for ( i = 0; i < hvminfo->nr_vcpus; i++ )
>>      {
>> -        memset(lapic, 0, sizeof(*lapic));
>> -        lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
>> -        lapic->length  = sizeof(*lapic);
>> -        /* Processor ID must match processor-object IDs in the DSDT. */
>> -        lapic->acpi_processor_id = i;
>> -        lapic->apic_id = config->lapic_id(i);
>> -        lapic->flags = (test_bit(i, hvminfo->vcpu_online)
>> -                        ? ACPI_LOCAL_APIC_ENABLED : 0);
>> -        lapic++;
>> +        unsigned int apic_id = config->lapic_id(i);
>> +
>> +        if ( apic_id < 255 ) {
>> +            struct acpi_20_madt_lapic *lapic = end;
>> +
>> +            memset(lapic, 0, sizeof(*lapic));
>> +            lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
>> +            lapic->length  = sizeof(*lapic);
>> +            /* Processor ID must match processor-object IDs in the DSDT. */
>> +            lapic->acpi_processor_id = i;
>> +            lapic->apic_id = config->lapic_id(i);
>> +            lapic->flags = ((i < hvminfo->nr_vcpus) &&
> 
> I don't understand why you have added this.

If apic_id < 255, still use xapic structure and use x2apic structure for
others. I think this is following your comment on last version.


> 
> Roger.
> 


-- 

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

Re: [RFC PATCH V2 3/4] hvmload: Add x2apic entry support in the MADT build

[Roger Pau Monné]

On Mon, Sep 04, 2017 at 06:59:24PM +0800, Lan Tianyu wrote:
> On 2017年09月01日 17:57, Roger Pau Monné wrote:
> > On Thu, Aug 31, 2017 at 01:01:48AM -0400, Lan Tianyu wrote:
> >> @@ -92,7 +92,12 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
> >>      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) * hvminfo->nr_vcpus;
> >> +
> >> +    if (hvminfo->nr_vcpus < 128)
> > 
> > This should be done based on APIC ID.
> 
> There will be a problem how to get max apic id. Should we use the max
> vcpu index to get max APIC id?

IMHO, this should become a loop that iterates over each vCPU getting
it's APIC ID, and add either a lapic or x2apic struct size to sz.

> >> -    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, lapic);
> >>      for ( i = 0; i < hvminfo->nr_vcpus; i++ )
> >>      {
> >> -        memset(lapic, 0, sizeof(*lapic));
> >> -        lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> >> -        lapic->length  = sizeof(*lapic);
> >> -        /* Processor ID must match processor-object IDs in the DSDT. */
> >> -        lapic->acpi_processor_id = i;
> >> -        lapic->apic_id = config->lapic_id(i);
> >> -        lapic->flags = (test_bit(i, hvminfo->vcpu_online)
> >> -                        ? ACPI_LOCAL_APIC_ENABLED : 0);
> >> -        lapic++;
> >> +        unsigned int apic_id = config->lapic_id(i);
> >> +
> >> +        if ( apic_id < 255 ) {
> >> +            struct acpi_20_madt_lapic *lapic = end;
> >> +
> >> +            memset(lapic, 0, sizeof(*lapic));
> >> +            lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> >> +            lapic->length  = sizeof(*lapic);
> >> +            /* Processor ID must match processor-object IDs in the DSDT. */
> >> +            lapic->acpi_processor_id = i;
> >> +            lapic->apic_id = config->lapic_id(i);
> >> +            lapic->flags = ((i < hvminfo->nr_vcpus) &&
> > 
> > I don't understand why you have added this.
> 
> If apic_id < 255, still use xapic structure and use x2apic structure for
> others. I think this is following your comment on last version.

I'm complaining about the (i < hvminfo->nr_vcpus) check, from the loop
above i is bounded to hvminfo->nr_vpcu, so this check is useless
AFAICT.

Roger.

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

Re: [RFC PATCH V2 3/4] hvmload: Add x2apic entry support in the MADT build

[Jan Beulich]

>>> On 04.09.17 at 13:12, <roger.pau@citrix.com> wrote:
> On Mon, Sep 04, 2017 at 06:59:24PM +0800, Lan Tianyu wrote:
>> On 2017年09月01日 17:57, Roger Pau Monné wrote:
>> > On Thu, Aug 31, 2017 at 01:01:48AM -0400, Lan Tianyu wrote:
>> >> @@ -92,7 +92,12 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>> >>      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) * hvminfo->nr_vcpus;
>> >> +
>> >> +    if (hvminfo->nr_vcpus < 128)
>> > 
>> > This should be done based on APIC ID.
>> 
>> There will be a problem how to get max apic id. Should we use the max
>> vcpu index to get max APIC id?
> 
> IMHO, this should become a loop that iterates over each vCPU getting
> it's APIC ID, and add either a lapic or x2apic struct size to sz.

Indeed.

Jan

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

[RFC PATCH V2 4/4] xl/libacpi: extend lapic_id() to uint32_t

[Lan Tianyu]

From: Chao Gao <chao.gao@intel.com>

This patch is to extend lapic_id() to support more vcpus.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 tools/firmware/hvmloader/util.c | 2 +-
 tools/libacpi/libacpi.h         | 2 +-
 tools/libxl/libxl_x86_acpi.c    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index db5f240..814ac2e 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -883,7 +883,7 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
     /* ACPI builder currently doesn't free memory so this is just a stub */
 }
 
-static uint8_t acpi_lapic_id(unsigned cpu)
+static uint32_t acpi_lapic_id(unsigned cpu)
 {
     return LAPIC_ID(cpu);
 }
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index 74778a5..0b04cbc 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -93,7 +93,7 @@ struct acpi_config {
     unsigned long rsdp;
 
     /* x86-specific parameters */
-    uint8_t (*lapic_id)(unsigned cpu);
+    uint32_t (*lapic_id)(unsigned cpu);
     uint32_t lapic_base_address;
     uint32_t ioapic_base_address;
     uint16_t pci_isa_irq_mask;
diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
index 1fa97ff..8fe084d 100644
--- a/tools/libxl/libxl_x86_acpi.c
+++ b/tools/libxl/libxl_x86_acpi.c
@@ -85,7 +85,7 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
 {
 }
 
-static uint8_t acpi_lapic_id(unsigned cpu)
+static uint32_t acpi_lapic_id(unsigned cpu)
 {
     return cpu * 2;
 }
-- 
1.8.3.1


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

Re: [RFC PATCH V2 4/4] xl/libacpi: extend lapic_id() to uint32_t

[Andrew Cooper]

On 31/08/17 06:01, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
>
> This patch is to extend lapic_id() to support more vcpus.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

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

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

Re: [RFC PATCH V2 4/4] xl/libacpi: extend lapic_id() to uint32_t

[Wei Liu]

On Thu, Aug 31, 2017 at 01:01:49AM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> This patch is to extend lapic_id() to support more vcpus.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Please remember to pick up my ack next time around -- I believed I
already acked this in the previous version.

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

Re: [RFC PATCH V2 4/4] xl/libacpi: extend lapic_id() to uint32_t

[Wei Liu]

On Fri, Sep 01, 2017 at 04:41:48PM +0100, Wei Liu wrote:
> On Thu, Aug 31, 2017 at 01:01:49AM -0400, Lan Tianyu wrote:
> > From: Chao Gao <chao.gao@intel.com>
> > 
> > This patch is to extend lapic_id() to support more vcpus.
> > 
> > Signed-off-by: Chao Gao <chao.gao@intel.com>
> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> Please remember to pick up my ack next time around -- I believed I
> already acked this in the previous version.

No, my memory messed up, I didn't actually give my ack. Sorry for the
noise.

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