| Message ID | 1505278369-21605-3-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 13, 2017 at 12:52:48AM -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 | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c > index 2daf32c..09c1529 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,27 @@ 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); > > - stmt("Name", "_HID, \"ACPI0007\""); > +#ifdef CONFIG_X86 > + unsigned int apic_id = cpu * 2; As said earlier, I don't like have some many places where apic id is calculated. Please look into unifying those. Also, declaring a new variable here is wrong. > + > + if ( apic_id > 254 ) 255? An APIC ID of 255 should still be fine. > + { > + push_block("Device", CPU_NAME_FMT, cpu); > + } > + else > +#endif > + { > + if (cpu > 255) > + { > + fprintf(stderr, "Exceed the range of processor ID \n"); > + return -1; > + } I'm not sure whether ARM shoudln't just use Device processor objects directly. x86 has to use Processor because of compatibility reasons, but I guess that's not an issue for ARM. Thanks, Roger.
>>> On 19.09.17 at 15:29, <roger.pau@citrix.com> wrote: > On Wed, Sep 13, 2017 at 12:52:48AM -0400, Lan Tianyu wrote: >> + if ( apic_id > 254 ) > > 255? An APIC ID of 255 should still be fine. Wasn't it you who (validly) asked for the boundary to be 254, due to 0xff being the broadcast value? Jan
On Tue, Sep 19, 2017 at 07:44:21AM -0600, Jan Beulich wrote: > >>> On 19.09.17 at 15:29, <roger.pau@citrix.com> wrote: > > On Wed, Sep 13, 2017 at 12:52:48AM -0400, Lan Tianyu wrote: > >> + if ( apic_id > 254 ) > > > > 255? An APIC ID of 255 should still be fine. > > Wasn't it you who (validly) asked for the boundary to be 254, due > to 0xff being the broadcast value? But that's the ACPI ID, not the APIC ID. Roger.
>>> On 19.09.17 at 15:48, <roger.pau@citrix.com> wrote: > On Tue, Sep 19, 2017 at 07:44:21AM -0600, Jan Beulich wrote: >> >>> On 19.09.17 at 15:29, <roger.pau@citrix.com> wrote: >> > On Wed, Sep 13, 2017 at 12:52:48AM -0400, Lan Tianyu wrote: >> >> + if ( apic_id > 254 ) >> > >> > 255? An APIC ID of 255 should still be fine. >> >> Wasn't it you who (validly) asked for the boundary to be 254, due >> to 0xff being the broadcast value? > > But that's the ACPI ID, not the APIC ID. The code above says "apic_id" - is the variable mis-named? Or am I reading your reply the wrong way round, in which case the question would be why an ACPI ID could ever express something like "broadcast"? Jan
On Tue, Sep 19, 2017 at 07:55:32AM -0600, Jan Beulich wrote: > >>> On 19.09.17 at 15:48, <roger.pau@citrix.com> wrote: > > On Tue, Sep 19, 2017 at 07:44:21AM -0600, Jan Beulich wrote: > >> >>> On 19.09.17 at 15:29, <roger.pau@citrix.com> wrote: > >> > On Wed, Sep 13, 2017 at 12:52:48AM -0400, Lan Tianyu wrote: > >> >> + if ( apic_id > 254 ) > >> > > >> > 255? An APIC ID of 255 should still be fine. > >> > >> Wasn't it you who (validly) asked for the boundary to be 254, due > >> to 0xff being the broadcast value? > > > > But that's the ACPI ID, not the APIC ID. > > The code above says "apic_id" - is the variable mis-named? Or am > I reading your reply the wrong way round, in which case the question > would be why an ACPI ID could ever express something like > "broadcast"? Yes, sorry I got messed up. This is indeed fine, as a local APIC ID of 255 is the broadcast ID. But this also applies to the ACPI ID, since an ACPI ID of 255 is also the broadcast ID for local APIC entries in the MADT. For example a Local APIC NMI Structure with an ACPI ID of 255 applies to all local APICs. We need to be careful to not create local APIC entries with either APIC or ACPI ID equal to 255 (and to also not create Processor objects with ACPI ID of 255). Roger.
>>> On 19.09.17 at 16:13, <roger.pau@citrix.com> wrote: > On Tue, Sep 19, 2017 at 07:55:32AM -0600, Jan Beulich wrote: >> >>> On 19.09.17 at 15:48, <roger.pau@citrix.com> wrote: >> > On Tue, Sep 19, 2017 at 07:44:21AM -0600, Jan Beulich wrote: >> >> >>> On 19.09.17 at 15:29, <roger.pau@citrix.com> wrote: >> >> > On Wed, Sep 13, 2017 at 12:52:48AM -0400, Lan Tianyu wrote: >> >> >> + if ( apic_id > 254 ) >> >> > >> >> > 255? An APIC ID of 255 should still be fine. >> >> >> >> Wasn't it you who (validly) asked for the boundary to be 254, due >> >> to 0xff being the broadcast value? >> > >> > But that's the ACPI ID, not the APIC ID. >> >> The code above says "apic_id" - is the variable mis-named? Or am >> I reading your reply the wrong way round, in which case the question >> would be why an ACPI ID could ever express something like >> "broadcast"? > > Yes, sorry I got messed up. This is indeed fine, as a local APIC ID > of 255 is the broadcast ID. But this also applies to the ACPI ID, > since an ACPI ID of 255 is also the broadcast ID for local APIC > entries in the MADT. For example a Local APIC NMI Structure with an > ACPI ID of 255 applies to all local APICs. Indeed. > We need to be careful to not create local APIC entries with either > APIC or ACPI ID equal to 255 (and to also not create Processor objects > with ACPI ID of 255). Why? An ACPI or APIC ID is still fine as long as it does only occur in x2APIC contexts. Jan
On Tue, Sep 19, 2017 at 09:02:06AM -0600, Jan Beulich wrote: > >>> On 19.09.17 at 16:13, <roger.pau@citrix.com> wrote: > > We need to be careful to not create local APIC entries with either > > APIC or ACPI ID equal to 255 (and to also not create Processor objects > > with ACPI ID of 255). > > Why? An ACPI or APIC ID is still fine as long as it does only occur > in x2APIC contexts. That's what I was trying to reference to with "local APIC entries" and "Processor objects" as opposed to "x2APIC entries" and "Processor Devices", which are x2APIC contexts. AFAICT we are talking about the same. Roger.
diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c index 2daf32c..09c1529 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,27 @@ 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); - stmt("Name", "_HID, \"ACPI0007\""); +#ifdef CONFIG_X86 + unsigned int apic_id = cpu * 2; + + if ( apic_id > 254 ) + { + push_block("Device", CPU_NAME_FMT, cpu); + } + else +#endif + { + if (cpu > 255) + { + fprintf(stderr, "Exceed the range of processor ID \n"); + return -1; + } + 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 +287,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();
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 | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)