| Message ID | CAKmqyKMKL_BGSbaetubaPv8gkmF9XFFdDwkXAPsUZdV1qkOyqA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > <alistair.francis@xilinx.com> wrote: > > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 20 December 2017 at 00:27, Alistair Francis > >> <alistair.francis@xilinx.com> wrote: > >>> There are numorous QEMU machines that only have a single or a handful of > >>> valid CPU options. To simplyfy the management of specificying which CPU > >>> is/isn't valid let's create a property that can be set in the machine > >>> init. We can then check to see if the user supplied CPU is in that list > >>> or not. > >>> > >>> I have added the valid_cpu_types for some ARM machines only at the > >>> moment. > >>> > >>> Here is what specifying the CPUs looks like now: > >>> > >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > >>> QEMU 2.10.50 monitor - type 'help' for more information > >>> (qemu) info cpus > >>> * CPU #0: thread_id=24175 > >>> (qemu) q > >>> > >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > >>> QEMU 2.10.50 monitor - type 'help' for more information > >>> (qemu) q > >>> > >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > >>> > >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > >> > >> Thanks for this; we really should be more strict about > >> forbidding "won't work" combinations than we have > >> been in the past. > >> > >> In the last of these cases, I think that when we > >> list the invalid CPU type and the valid types > >> we should use the same names we want the user to > >> use on the command line, without the "-arm-cpu" > >> suffixes. > > > > Hmm... That is a good point, it is confusing that they don't line up. Agreed. > > > > The problem is that we are just doing a simple > > object_class_dynamic_cast() in hw/core/machine.c which I think > > (untested) requires us to have the full name in the valid cpu array. [...] > > I think an earlier version of my previous series adding the support to > machine.c did string comparison, but it was decided to utilise objects > instead. One option is to make the array 2 wide and have the second > string be user friendly? Making the array 2-column will duplicate information that we can already find out using other methods, and it won't solve the problem if an entry has a parent class with multiple subclasses (the original reason I suggested object_class_dynamic_cast()). The main obstacle to fix this easily is that we do have a common ObjectClass *cpu_class_by_name(const char *cpu_model) function, but not a common method to get the model name from a CPUClass. Implementing this is possible, but probably better to do it after moving the existing arch-specific CPU model enumeration hooks to common code (currently we duplicate lots of CPU enumeration/lookup boilerplate code that we shouldn't have to). Listing only the human-friendly names in the array like in the original patch could be a reasonable temporary solution. It won't allow us to use a single entry for all subclasses of a given type by now (e.g. listing only TYPE_X86_CPU on PC), but at least we can address this issue without waiting for a refactor of the CPU model enumeration code.
On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: >> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis >> <alistair.francis@xilinx.com> wrote: >> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On 20 December 2017 at 00:27, Alistair Francis >> >> <alistair.francis@xilinx.com> wrote: >> >>> There are numorous QEMU machines that only have a single or a handful of >> >>> valid CPU options. To simplyfy the management of specificying which CPU >> >>> is/isn't valid let's create a property that can be set in the machine >> >>> init. We can then check to see if the user supplied CPU is in that list >> >>> or not. >> >>> >> >>> I have added the valid_cpu_types for some ARM machines only at the >> >>> moment. >> >>> >> >>> Here is what specifying the CPUs looks like now: >> >>> >> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S >> >>> QEMU 2.10.50 monitor - type 'help' for more information >> >>> (qemu) info cpus >> >>> * CPU #0: thread_id=24175 >> >>> (qemu) q >> >>> >> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S >> >>> QEMU 2.10.50 monitor - type 'help' for more information >> >>> (qemu) q >> >>> >> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S >> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' >> >>> >> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S >> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu >> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu >> >> >> >> Thanks for this; we really should be more strict about >> >> forbidding "won't work" combinations than we have >> >> been in the past. >> >> >> >> In the last of these cases, I think that when we >> >> list the invalid CPU type and the valid types >> >> we should use the same names we want the user to >> >> use on the command line, without the "-arm-cpu" >> >> suffixes. >> > >> > Hmm... That is a good point, it is confusing that they don't line up. > > Agreed. > >> > >> > The problem is that we are just doing a simple >> > object_class_dynamic_cast() in hw/core/machine.c which I think >> > (untested) requires us to have the full name in the valid cpu array. > [...] >> >> I think an earlier version of my previous series adding the support to >> machine.c did string comparison, but it was decided to utilise objects >> instead. One option is to make the array 2 wide and have the second >> string be user friendly? > > Making the array 2-column will duplicate information that we can > already find out using other methods, and it won't solve the > problem if an entry has a parent class with multiple subclasses > (the original reason I suggested object_class_dynamic_cast()). > > The main obstacle to fix this easily is that we do have a common > ObjectClass *cpu_class_by_name(const char *cpu_model) > function, but not a common method to get the model name from a > CPUClass. Implementing this is possible, but probably better to > do it after moving the existing arch-specific CPU model > enumeration hooks to common code (currently we duplicate lots of > CPU enumeration/lookup boilerplate code that we shouldn't have > to). > > Listing only the human-friendly names in the array like in the > original patch could be a reasonable temporary solution. It > won't allow us to use a single entry for all subclasses of a > given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > least we can address this issue without waiting for a refactor of > the CPU model enumeration code. Ok, so it sounds like I'll respin this series with an extra column in the array for human readable names. Then in the future we can work on removing that. Alistair > > -- > Eduardo >
On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis >>> <alistair.francis@xilinx.com> wrote: >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> >> On 20 December 2017 at 00:27, Alistair Francis >>> >> <alistair.francis@xilinx.com> wrote: >>> >>> There are numorous QEMU machines that only have a single or a handful of >>> >>> valid CPU options. To simplyfy the management of specificying which CPU >>> >>> is/isn't valid let's create a property that can be set in the machine >>> >>> init. We can then check to see if the user supplied CPU is in that list >>> >>> or not. >>> >>> >>> >>> I have added the valid_cpu_types for some ARM machines only at the >>> >>> moment. >>> >>> >>> >>> Here is what specifying the CPUs looks like now: >>> >>> >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S >>> >>> QEMU 2.10.50 monitor - type 'help' for more information >>> >>> (qemu) info cpus >>> >>> * CPU #0: thread_id=24175 >>> >>> (qemu) q >>> >>> >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S >>> >>> QEMU 2.10.50 monitor - type 'help' for more information >>> >>> (qemu) q >>> >>> >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' >>> >>> >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu >>> >> >>> >> Thanks for this; we really should be more strict about >>> >> forbidding "won't work" combinations than we have >>> >> been in the past. >>> >> >>> >> In the last of these cases, I think that when we >>> >> list the invalid CPU type and the valid types >>> >> we should use the same names we want the user to >>> >> use on the command line, without the "-arm-cpu" >>> >> suffixes. >>> > >>> > Hmm... That is a good point, it is confusing that they don't line up. >> >> Agreed. >> >>> > >>> > The problem is that we are just doing a simple >>> > object_class_dynamic_cast() in hw/core/machine.c which I think >>> > (untested) requires us to have the full name in the valid cpu array. >> [...] >>> >>> I think an earlier version of my previous series adding the support to >>> machine.c did string comparison, but it was decided to utilise objects >>> instead. One option is to make the array 2 wide and have the second >>> string be user friendly? >> >> Making the array 2-column will duplicate information that we can >> already find out using other methods, and it won't solve the >> problem if an entry has a parent class with multiple subclasses >> (the original reason I suggested object_class_dynamic_cast()). >> >> The main obstacle to fix this easily is that we do have a common >> ObjectClass *cpu_class_by_name(const char *cpu_model) >> function, but not a common method to get the model name from a >> CPUClass. Implementing this is possible, but probably better to >> do it after moving the existing arch-specific CPU model >> enumeration hooks to common code (currently we duplicate lots of >> CPU enumeration/lookup boilerplate code that we shouldn't have >> to). >> >> Listing only the human-friendly names in the array like in the >> original patch could be a reasonable temporary solution. It >> won't allow us to use a single entry for all subclasses of a >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at >> least we can address this issue without waiting for a refactor of >> the CPU model enumeration code. Ah, I just re-read this. Do you mean go back to the original RFC and just use strcmp() to compare the human readable cpu_model? Alistair > > Ok, so it sounds like I'll respin this series with an extra column in > the array for human readable names. Then in the future we can work on > removing that. > > Alistair > >> >> -- >> Eduardo >>
On Fri, Dec 22, 2017 at 11:47:00AM -0800, Alistair Francis wrote: > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis > <alistair.francis@xilinx.com> wrote: > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > >>> <alistair.francis@xilinx.com> wrote: > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > >>> >> On 20 December 2017 at 00:27, Alistair Francis > >>> >> <alistair.francis@xilinx.com> wrote: > >>> >>> There are numorous QEMU machines that only have a single or a handful of > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU > >>> >>> is/isn't valid let's create a property that can be set in the machine > >>> >>> init. We can then check to see if the user supplied CPU is in that list > >>> >>> or not. > >>> >>> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the > >>> >>> moment. > >>> >>> > >>> >>> Here is what specifying the CPUs looks like now: > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > >>> >>> (qemu) info cpus > >>> >>> * CPU #0: thread_id=24175 > >>> >>> (qemu) q > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > >>> >>> (qemu) q > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > >>> >> > >>> >> Thanks for this; we really should be more strict about > >>> >> forbidding "won't work" combinations than we have > >>> >> been in the past. > >>> >> > >>> >> In the last of these cases, I think that when we > >>> >> list the invalid CPU type and the valid types > >>> >> we should use the same names we want the user to > >>> >> use on the command line, without the "-arm-cpu" > >>> >> suffixes. > >>> > > >>> > Hmm... That is a good point, it is confusing that they don't line up. > >> > >> Agreed. > >> > >>> > > >>> > The problem is that we are just doing a simple > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think > >>> > (untested) requires us to have the full name in the valid cpu array. > >> [...] > >>> > >>> I think an earlier version of my previous series adding the support to > >>> machine.c did string comparison, but it was decided to utilise objects > >>> instead. One option is to make the array 2 wide and have the second > >>> string be user friendly? > >> > >> Making the array 2-column will duplicate information that we can > >> already find out using other methods, and it won't solve the > >> problem if an entry has a parent class with multiple subclasses > >> (the original reason I suggested object_class_dynamic_cast()). > >> > >> The main obstacle to fix this easily is that we do have a common > >> ObjectClass *cpu_class_by_name(const char *cpu_model) > >> function, but not a common method to get the model name from a > >> CPUClass. Implementing this is possible, but probably better to > >> do it after moving the existing arch-specific CPU model > >> enumeration hooks to common code (currently we duplicate lots of > >> CPU enumeration/lookup boilerplate code that we shouldn't have > >> to). > >> > >> Listing only the human-friendly names in the array like in the > >> original patch could be a reasonable temporary solution. It > >> won't allow us to use a single entry for all subclasses of a > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > >> least we can address this issue without waiting for a refactor of > >> the CPU model enumeration code. > > Ah, I just re-read this. Do you mean go back to the original RFC and > just use strcmp() to compare the human readable cpu_model? Yes, I think this would be simpler, considering that the current users don't need the object_class_dynamic_cast() stuff. Then whoever decide to make PC simply list TYPE_X86_CPU later (possibly me) will need to clean up the CPU model enumeration/lookup code and implement a cpu_model_from_type_name() helper.
On Fri, 22 Dec 2017 11:47:00 -0800 Alistair Francis <alistair.francis@xilinx.com> wrote: > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis > <alistair.francis@xilinx.com> wrote: > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > >>> <alistair.francis@xilinx.com> wrote: > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > >>> >> On 20 December 2017 at 00:27, Alistair Francis > >>> >> <alistair.francis@xilinx.com> wrote: > >>> >>> There are numorous QEMU machines that only have a single or a handful of > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU > >>> >>> is/isn't valid let's create a property that can be set in the machine > >>> >>> init. We can then check to see if the user supplied CPU is in that list > >>> >>> or not. > >>> >>> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the > >>> >>> moment. > >>> >>> > >>> >>> Here is what specifying the CPUs looks like now: > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > >>> >>> (qemu) info cpus > >>> >>> * CPU #0: thread_id=24175 > >>> >>> (qemu) q > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > >>> >>> (qemu) q > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > >>> >>> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > >>> >> > >>> >> Thanks for this; we really should be more strict about > >>> >> forbidding "won't work" combinations than we have > >>> >> been in the past. > >>> >> > >>> >> In the last of these cases, I think that when we > >>> >> list the invalid CPU type and the valid types > >>> >> we should use the same names we want the user to > >>> >> use on the command line, without the "-arm-cpu" > >>> >> suffixes. > >>> > > >>> > Hmm... That is a good point, it is confusing that they don't line up. > >> > >> Agreed. > >> > >>> > > >>> > The problem is that we are just doing a simple > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think > >>> > (untested) requires us to have the full name in the valid cpu array. > >> [...] > >>> > >>> I think an earlier version of my previous series adding the support to > >>> machine.c did string comparison, but it was decided to utilise objects > >>> instead. One option is to make the array 2 wide and have the second > >>> string be user friendly? > >> > >> Making the array 2-column will duplicate information that we can > >> already find out using other methods, and it won't solve the > >> problem if an entry has a parent class with multiple subclasses > >> (the original reason I suggested object_class_dynamic_cast()). > >> > >> The main obstacle to fix this easily is that we do have a common > >> ObjectClass *cpu_class_by_name(const char *cpu_model) > >> function, but not a common method to get the model name from a > >> CPUClass. Implementing this is possible, but probably better to > >> do it after moving the existing arch-specific CPU model > >> enumeration hooks to common code (currently we duplicate lots of > >> CPU enumeration/lookup boilerplate code that we shouldn't have > >> to). > >> > >> Listing only the human-friendly names in the array like in the > >> original patch could be a reasonable temporary solution. It > >> won't allow us to use a single entry for all subclasses of a > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > >> least we can address this issue without waiting for a refactor of > >> the CPU model enumeration code. > > Ah, I just re-read this. Do you mean go back to the original RFC and > just use strcmp() to compare the human readable cpu_model? It's sort of going backwards but I won't object to this as far as you won't use machine->cpu_model (which is in process of being removed) BTW: how hard is it, to add cpu_type2cpu_name function? > Alistair > > > > > Ok, so it sounds like I'll respin this series with an extra column in > > the array for human readable names. Then in the future we can work on > > removing that. > > > > Alistair > > > >> > >> -- > >> Eduardo > >>
On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote: > On Fri, 22 Dec 2017 11:47:00 -0800 > Alistair Francis <alistair.francis@xilinx.com> wrote: > > > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis > > <alistair.francis@xilinx.com> wrote: > > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > > >>> <alistair.francis@xilinx.com> wrote: > > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > >>> >> On 20 December 2017 at 00:27, Alistair Francis > > >>> >> <alistair.francis@xilinx.com> wrote: > > >>> >>> There are numorous QEMU machines that only have a single or a handful of > > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU > > >>> >>> is/isn't valid let's create a property that can be set in the machine > > >>> >>> init. We can then check to see if the user supplied CPU is in that list > > >>> >>> or not. > > >>> >>> > > >>> >>> I have added the valid_cpu_types for some ARM machines only at the > > >>> >>> moment. > > >>> >>> > > >>> >>> Here is what specifying the CPUs looks like now: > > >>> >>> > > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > >>> >>> (qemu) info cpus > > >>> >>> * CPU #0: thread_id=24175 > > >>> >>> (qemu) q > > >>> >>> > > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > >>> >>> (qemu) q > > >>> >>> > > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > > >>> >>> > > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > > >>> >> > > >>> >> Thanks for this; we really should be more strict about > > >>> >> forbidding "won't work" combinations than we have > > >>> >> been in the past. > > >>> >> > > >>> >> In the last of these cases, I think that when we > > >>> >> list the invalid CPU type and the valid types > > >>> >> we should use the same names we want the user to > > >>> >> use on the command line, without the "-arm-cpu" > > >>> >> suffixes. > > >>> > > > >>> > Hmm... That is a good point, it is confusing that they don't line up. > > >> > > >> Agreed. > > >> > > >>> > > > >>> > The problem is that we are just doing a simple > > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think > > >>> > (untested) requires us to have the full name in the valid cpu array. > > >> [...] > > >>> > > >>> I think an earlier version of my previous series adding the support to > > >>> machine.c did string comparison, but it was decided to utilise objects > > >>> instead. One option is to make the array 2 wide and have the second > > >>> string be user friendly? > > >> > > >> Making the array 2-column will duplicate information that we can > > >> already find out using other methods, and it won't solve the > > >> problem if an entry has a parent class with multiple subclasses > > >> (the original reason I suggested object_class_dynamic_cast()). > > >> > > >> The main obstacle to fix this easily is that we do have a common > > >> ObjectClass *cpu_class_by_name(const char *cpu_model) > > >> function, but not a common method to get the model name from a > > >> CPUClass. Implementing this is possible, but probably better to > > >> do it after moving the existing arch-specific CPU model > > >> enumeration hooks to common code (currently we duplicate lots of > > >> CPU enumeration/lookup boilerplate code that we shouldn't have > > >> to). > > >> > > >> Listing only the human-friendly names in the array like in the > > >> original patch could be a reasonable temporary solution. It > > >> won't allow us to use a single entry for all subclasses of a > > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > > >> least we can address this issue without waiting for a refactor of > > >> the CPU model enumeration code. > > > > Ah, I just re-read this. Do you mean go back to the original RFC and > > just use strcmp() to compare the human readable cpu_model? > It's sort of going backwards but I won't object to this as far as you > won't use machine->cpu_model (which is in process of being removed) > > > BTW: > how hard is it, to add cpu_type2cpu_name function? It shouldn't be hard, but I would like to avoid adding yet another arch-specific hook just for that. Probably we would need to clean up the existing CPU model enumeration/lookup code if we want to avoid increase duplication of code on arch-specific hooks.
On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote: >> On Fri, 22 Dec 2017 11:47:00 -0800 >> Alistair Francis <alistair.francis@xilinx.com> wrote: >> >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis >> > <alistair.francis@xilinx.com> wrote: >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: >> > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis >> > >>> <alistair.francis@xilinx.com> wrote: >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis >> > >>> >> <alistair.francis@xilinx.com> wrote: >> > >>> >>> There are numorous QEMU machines that only have a single or a handful of >> > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU >> > >>> >>> is/isn't valid let's create a property that can be set in the machine >> > >>> >>> init. We can then check to see if the user supplied CPU is in that list >> > >>> >>> or not. >> > >>> >>> >> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the >> > >>> >>> moment. >> > >>> >>> >> > >>> >>> Here is what specifying the CPUs looks like now: >> > >>> >>> >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information >> > >>> >>> (qemu) info cpus >> > >>> >>> * CPU #0: thread_id=24175 >> > >>> >>> (qemu) q >> > >>> >>> >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information >> > >>> >>> (qemu) q >> > >>> >>> >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S >> > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' >> > >>> >>> >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S >> > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu >> > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu >> > >>> >> >> > >>> >> Thanks for this; we really should be more strict about >> > >>> >> forbidding "won't work" combinations than we have >> > >>> >> been in the past. >> > >>> >> >> > >>> >> In the last of these cases, I think that when we >> > >>> >> list the invalid CPU type and the valid types >> > >>> >> we should use the same names we want the user to >> > >>> >> use on the command line, without the "-arm-cpu" >> > >>> >> suffixes. >> > >>> > >> > >>> > Hmm... That is a good point, it is confusing that they don't line up. >> > >> >> > >> Agreed. >> > >> >> > >>> > >> > >>> > The problem is that we are just doing a simple >> > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think >> > >>> > (untested) requires us to have the full name in the valid cpu array. >> > >> [...] >> > >>> >> > >>> I think an earlier version of my previous series adding the support to >> > >>> machine.c did string comparison, but it was decided to utilise objects >> > >>> instead. One option is to make the array 2 wide and have the second >> > >>> string be user friendly? >> > >> >> > >> Making the array 2-column will duplicate information that we can >> > >> already find out using other methods, and it won't solve the >> > >> problem if an entry has a parent class with multiple subclasses >> > >> (the original reason I suggested object_class_dynamic_cast()). >> > >> >> > >> The main obstacle to fix this easily is that we do have a common >> > >> ObjectClass *cpu_class_by_name(const char *cpu_model) >> > >> function, but not a common method to get the model name from a >> > >> CPUClass. Implementing this is possible, but probably better to >> > >> do it after moving the existing arch-specific CPU model >> > >> enumeration hooks to common code (currently we duplicate lots of >> > >> CPU enumeration/lookup boilerplate code that we shouldn't have >> > >> to). >> > >> >> > >> Listing only the human-friendly names in the array like in the >> > >> original patch could be a reasonable temporary solution. It >> > >> won't allow us to use a single entry for all subclasses of a >> > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at >> > >> least we can address this issue without waiting for a refactor of >> > >> the CPU model enumeration code. >> > >> > Ah, I just re-read this. Do you mean go back to the original RFC and >> > just use strcmp() to compare the human readable cpu_model? >> It's sort of going backwards but I won't object to this as far as you >> won't use machine->cpu_model (which is in process of being removed) Wait, machine->cpu_model is the human readable name. Without using that we can't use just human readable strings for the valid cpu types. Alistair >> >> >> BTW: >> how hard is it, to add cpu_type2cpu_name function? > > It shouldn't be hard, but I would like to avoid adding yet > another arch-specific hook just for that. Probably we would need > to clean up the existing CPU model enumeration/lookup code if we > want to avoid increase duplication of code on arch-specific > hooks. > > -- > Eduardo >
On Wed, Jan 10, 2018 at 01:30:29PM -0800, Alistair Francis wrote: > On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote: > >> On Fri, 22 Dec 2017 11:47:00 -0800 > >> Alistair Francis <alistair.francis@xilinx.com> wrote: > >> > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis > >> > <alistair.francis@xilinx.com> wrote: > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > >> > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > >> > >>> <alistair.francis@xilinx.com> wrote: > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis > >> > >>> >> <alistair.francis@xilinx.com> wrote: > >> > >>> >>> There are numorous QEMU machines that only have a single or a handful of > >> > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU > >> > >>> >>> is/isn't valid let's create a property that can be set in the machine > >> > >>> >>> init. We can then check to see if the user supplied CPU is in that list > >> > >>> >>> or not. > >> > >>> >>> > >> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the > >> > >>> >>> moment. > >> > >>> >>> > >> > >>> >>> Here is what specifying the CPUs looks like now: > >> > >>> >>> > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > >> > >>> >>> (qemu) info cpus > >> > >>> >>> * CPU #0: thread_id=24175 > >> > >>> >>> (qemu) q > >> > >>> >>> > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > >> > >>> >>> (qemu) q > >> > >>> >>> > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > >> > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > >> > >>> >>> > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > >> > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > >> > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > >> > >>> >> > >> > >>> >> Thanks for this; we really should be more strict about > >> > >>> >> forbidding "won't work" combinations than we have > >> > >>> >> been in the past. > >> > >>> >> > >> > >>> >> In the last of these cases, I think that when we > >> > >>> >> list the invalid CPU type and the valid types > >> > >>> >> we should use the same names we want the user to > >> > >>> >> use on the command line, without the "-arm-cpu" > >> > >>> >> suffixes. > >> > >>> > > >> > >>> > Hmm... That is a good point, it is confusing that they don't line up. > >> > >> > >> > >> Agreed. > >> > >> > >> > >>> > > >> > >>> > The problem is that we are just doing a simple > >> > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think > >> > >>> > (untested) requires us to have the full name in the valid cpu array. > >> > >> [...] > >> > >>> > >> > >>> I think an earlier version of my previous series adding the support to > >> > >>> machine.c did string comparison, but it was decided to utilise objects > >> > >>> instead. One option is to make the array 2 wide and have the second > >> > >>> string be user friendly? > >> > >> > >> > >> Making the array 2-column will duplicate information that we can > >> > >> already find out using other methods, and it won't solve the > >> > >> problem if an entry has a parent class with multiple subclasses > >> > >> (the original reason I suggested object_class_dynamic_cast()). > >> > >> > >> > >> The main obstacle to fix this easily is that we do have a common > >> > >> ObjectClass *cpu_class_by_name(const char *cpu_model) > >> > >> function, but not a common method to get the model name from a > >> > >> CPUClass. Implementing this is possible, but probably better to > >> > >> do it after moving the existing arch-specific CPU model > >> > >> enumeration hooks to common code (currently we duplicate lots of > >> > >> CPU enumeration/lookup boilerplate code that we shouldn't have > >> > >> to). > >> > >> > >> > >> Listing only the human-friendly names in the array like in the > >> > >> original patch could be a reasonable temporary solution. It > >> > >> won't allow us to use a single entry for all subclasses of a > >> > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > >> > >> least we can address this issue without waiting for a refactor of > >> > >> the CPU model enumeration code. > >> > > >> > Ah, I just re-read this. Do you mean go back to the original RFC and > >> > just use strcmp() to compare the human readable cpu_model? > >> It's sort of going backwards but I won't object to this as far as you > >> won't use machine->cpu_model (which is in process of being removed) > > Wait, machine->cpu_model is the human readable name. Without using > that we can't use just human readable strings for the valid cpu types. Well, if we want to deprecate machine->cpu_model we need to offer an alternative first, otherwise we can't prevent people from using it. Igor, do you see an (existing) alternative to machine->cpu_model that would allow us to avoid using it in machine_run_board_init()?
On Wed, 10 Jan 2018 19:48:00 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Jan 10, 2018 at 01:30:29PM -0800, Alistair Francis wrote: > > On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote: > > >> On Fri, 22 Dec 2017 11:47:00 -0800 > > >> Alistair Francis <alistair.francis@xilinx.com> wrote: > > >> > > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis > > >> > <alistair.francis@xilinx.com> wrote: > > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > >> > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > > >> > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > > >> > >>> <alistair.francis@xilinx.com> wrote: > > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis > > >> > >>> >> <alistair.francis@xilinx.com> wrote: > > >> > >>> >>> There are numorous QEMU machines that only have a single or a handful of > > >> > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU > > >> > >>> >>> is/isn't valid let's create a property that can be set in the machine > > >> > >>> >>> init. We can then check to see if the user supplied CPU is in that list > > >> > >>> >>> or not. > > >> > >>> >>> > > >> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the > > >> > >>> >>> moment. > > >> > >>> >>> > > >> > >>> >>> Here is what specifying the CPUs looks like now: > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > >> > >>> >>> (qemu) info cpus > > >> > >>> >>> * CPU #0: thread_id=24175 > > >> > >>> >>> (qemu) q > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > >> > >>> >>> (qemu) q > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > > >> > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > > >> > >>> >>> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > > >> > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > > >> > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > > >> > >>> >> > > >> > >>> >> Thanks for this; we really should be more strict about > > >> > >>> >> forbidding "won't work" combinations than we have > > >> > >>> >> been in the past. > > >> > >>> >> > > >> > >>> >> In the last of these cases, I think that when we > > >> > >>> >> list the invalid CPU type and the valid types > > >> > >>> >> we should use the same names we want the user to > > >> > >>> >> use on the command line, without the "-arm-cpu" > > >> > >>> >> suffixes. > > >> > >>> > > > >> > >>> > Hmm... That is a good point, it is confusing that they don't line up. > > >> > >> > > >> > >> Agreed. > > >> > >> > > >> > >>> > > > >> > >>> > The problem is that we are just doing a simple > > >> > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think > > >> > >>> > (untested) requires us to have the full name in the valid cpu array. > > >> > >> [...] > > >> > >>> > > >> > >>> I think an earlier version of my previous series adding the support to > > >> > >>> machine.c did string comparison, but it was decided to utilise objects > > >> > >>> instead. One option is to make the array 2 wide and have the second > > >> > >>> string be user friendly? > > >> > >> > > >> > >> Making the array 2-column will duplicate information that we can > > >> > >> already find out using other methods, and it won't solve the > > >> > >> problem if an entry has a parent class with multiple subclasses > > >> > >> (the original reason I suggested object_class_dynamic_cast()). > > >> > >> > > >> > >> The main obstacle to fix this easily is that we do have a common > > >> > >> ObjectClass *cpu_class_by_name(const char *cpu_model) > > >> > >> function, but not a common method to get the model name from a > > >> > >> CPUClass. Implementing this is possible, but probably better to > > >> > >> do it after moving the existing arch-specific CPU model > > >> > >> enumeration hooks to common code (currently we duplicate lots of > > >> > >> CPU enumeration/lookup boilerplate code that we shouldn't have > > >> > >> to). > > >> > >> > > >> > >> Listing only the human-friendly names in the array like in the > > >> > >> original patch could be a reasonable temporary solution. It > > >> > >> won't allow us to use a single entry for all subclasses of a > > >> > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > > >> > >> least we can address this issue without waiting for a refactor of > > >> > >> the CPU model enumeration code. > > >> > > > >> > Ah, I just re-read this. Do you mean go back to the original RFC and > > >> > just use strcmp() to compare the human readable cpu_model? > > >> It's sort of going backwards but I won't object to this as far as you > > >> won't use machine->cpu_model (which is in process of being removed) > > > > Wait, machine->cpu_model is the human readable name. Without using > > that we can't use just human readable strings for the valid cpu types. > > Well, if we want to deprecate machine->cpu_model we need to offer > an alternative first, otherwise we can't prevent people from > using it. > > Igor, do you see an (existing) alternative to machine->cpu_model > that would allow us to avoid using it in > machine_run_board_init()? In recently merged refactoring machine->cpu_model is being replaced by machine->cpu_type. So currently we don't need machine->cpu_model anywhere except machine('none'), and once I refactor that it could be dropped completely and after some work on *-user targets we can practically get rid of cpu_model notion completely (excluding of -cpu option parser). My dislike of idea is that it's adding back cpumodel strings in boards code again (which I've just got rid of). I hate to say that but it looks like we need more refactoring for this series to print cpumodels back to user. We already have FOO_cpu_list()/FOO_query_cpu_definitions() which already do cpu type => cpumodel conversion (and even have some code duplication within a target), I'd suggest generalizing that across targets and then using generic helper for printing back to user converted cpu types from mc->valid_cpu_types which this series introduces.
On Thu, Jan 11, 2018 at 11:25:08AM +0100, Igor Mammedov wrote: > On Wed, 10 Jan 2018 19:48:00 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Jan 10, 2018 at 01:30:29PM -0800, Alistair Francis wrote: > > > On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote: > > > >> On Fri, 22 Dec 2017 11:47:00 -0800 > > > >> Alistair Francis <alistair.francis@xilinx.com> wrote: > > > >> > > > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis > > > >> > <alistair.francis@xilinx.com> wrote: > > > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > >> > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: > > > >> > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis > > > >> > >>> <alistair.francis@xilinx.com> wrote: > > > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis > > > >> > >>> >> <alistair.francis@xilinx.com> wrote: > > > >> > >>> >>> There are numorous QEMU machines that only have a single or a handful of > > > >> > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU > > > >> > >>> >>> is/isn't valid let's create a property that can be set in the machine > > > >> > >>> >>> init. We can then check to see if the user supplied CPU is in that list > > > >> > >>> >>> or not. > > > >> > >>> >>> > > > >> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the > > > >> > >>> >>> moment. > > > >> > >>> >>> > > > >> > >>> >>> Here is what specifying the CPUs looks like now: > > > >> > >>> >>> > > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > > >> > >>> >>> (qemu) info cpus > > > >> > >>> >>> * CPU #0: thread_id=24175 > > > >> > >>> >>> (qemu) q > > > >> > >>> >>> > > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information > > > >> > >>> >>> (qemu) q > > > >> > >>> >>> > > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > > > >> > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' > > > >> > >>> >>> > > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > > > >> > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > > > >> > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > > > >> > >>> >> > > > >> > >>> >> Thanks for this; we really should be more strict about > > > >> > >>> >> forbidding "won't work" combinations than we have > > > >> > >>> >> been in the past. > > > >> > >>> >> > > > >> > >>> >> In the last of these cases, I think that when we > > > >> > >>> >> list the invalid CPU type and the valid types > > > >> > >>> >> we should use the same names we want the user to > > > >> > >>> >> use on the command line, without the "-arm-cpu" > > > >> > >>> >> suffixes. > > > >> > >>> > > > > >> > >>> > Hmm... That is a good point, it is confusing that they don't line up. > > > >> > >> > > > >> > >> Agreed. > > > >> > >> > > > >> > >>> > > > > >> > >>> > The problem is that we are just doing a simple > > > >> > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think > > > >> > >>> > (untested) requires us to have the full name in the valid cpu array. > > > >> > >> [...] > > > >> > >>> > > > >> > >>> I think an earlier version of my previous series adding the support to > > > >> > >>> machine.c did string comparison, but it was decided to utilise objects > > > >> > >>> instead. One option is to make the array 2 wide and have the second > > > >> > >>> string be user friendly? > > > >> > >> > > > >> > >> Making the array 2-column will duplicate information that we can > > > >> > >> already find out using other methods, and it won't solve the > > > >> > >> problem if an entry has a parent class with multiple subclasses > > > >> > >> (the original reason I suggested object_class_dynamic_cast()). > > > >> > >> > > > >> > >> The main obstacle to fix this easily is that we do have a common > > > >> > >> ObjectClass *cpu_class_by_name(const char *cpu_model) > > > >> > >> function, but not a common method to get the model name from a > > > >> > >> CPUClass. Implementing this is possible, but probably better to > > > >> > >> do it after moving the existing arch-specific CPU model > > > >> > >> enumeration hooks to common code (currently we duplicate lots of > > > >> > >> CPU enumeration/lookup boilerplate code that we shouldn't have > > > >> > >> to). > > > >> > >> > > > >> > >> Listing only the human-friendly names in the array like in the > > > >> > >> original patch could be a reasonable temporary solution. It > > > >> > >> won't allow us to use a single entry for all subclasses of a > > > >> > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at > > > >> > >> least we can address this issue without waiting for a refactor of > > > >> > >> the CPU model enumeration code. > > > >> > > > > >> > Ah, I just re-read this. Do you mean go back to the original RFC and > > > >> > just use strcmp() to compare the human readable cpu_model? > > > >> It's sort of going backwards but I won't object to this as far as you > > > >> won't use machine->cpu_model (which is in process of being removed) > > > > > > Wait, machine->cpu_model is the human readable name. Without using > > > that we can't use just human readable strings for the valid cpu types. > > > > Well, if we want to deprecate machine->cpu_model we need to offer > > an alternative first, otherwise we can't prevent people from > > using it. > > > > Igor, do you see an (existing) alternative to machine->cpu_model > > that would allow us to avoid using it in > > machine_run_board_init()? > In recently merged refactoring machine->cpu_model is being replaced > by machine->cpu_type. So currently we don't need machine->cpu_model > anywhere except machine('none'), and once I refactor that it could > be dropped completely and after some work on *-user targets we can > practically get rid of cpu_model notion completely > (excluding of -cpu option parser). > > My dislike of idea is that it's adding back cpumodel strings > in boards code again (which I've just got rid of). > > I hate to say that but it looks like we need more refactoring > for this series to print cpumodels back to user. > > We already have FOO_cpu_list()/FOO_query_cpu_definitions() > which already do cpu type => cpumodel conversion (and even > have some code duplication within a target), I'd suggest > generalizing that across targets and then using generic > helper for printing back to user converted cpu types from > mc->valid_cpu_types which this series introduces. I agree with the long term goal of making cpu type => cpu model conversion generic. But until we refactor the arch-specific code and implement that, we have 2 options: 1) Keep printing a confusing error message until we implement cpu type => cpu model conversion; 2) Keep the MachineState::cpu_model field until we implement cpu type => cpu model conversion. I don't see any reason to pick (1) instead of (2).
On Thu, Jan 11, 2018 at 4:58 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jan 11, 2018 at 11:25:08AM +0100, Igor Mammedov wrote: >> On Wed, 10 Jan 2018 19:48:00 -0200 >> Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> > On Wed, Jan 10, 2018 at 01:30:29PM -0800, Alistair Francis wrote: >> > > On Thu, Dec 28, 2017 at 6:59 AM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > > > On Thu, Dec 28, 2017 at 02:39:31PM +0100, Igor Mammedov wrote: >> > > >> On Fri, 22 Dec 2017 11:47:00 -0800 >> > > >> Alistair Francis <alistair.francis@xilinx.com> wrote: >> > > >> >> > > >> > On Fri, Dec 22, 2017 at 10:45 AM, Alistair Francis >> > > >> > <alistair.francis@xilinx.com> wrote: >> > > >> > > On Wed, Dec 20, 2017 at 2:06 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > > >> > >> On Tue, Dec 19, 2017 at 05:03:59PM -0800, Alistair Francis wrote: >> > > >> > >>> On Tue, Dec 19, 2017 at 4:55 PM, Alistair Francis >> > > >> > >>> <alistair.francis@xilinx.com> wrote: >> > > >> > >>> > On Tue, Dec 19, 2017 at 4:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> > > >> > >>> >> On 20 December 2017 at 00:27, Alistair Francis >> > > >> > >>> >> <alistair.francis@xilinx.com> wrote: >> > > >> > >>> >>> There are numorous QEMU machines that only have a single or a handful of >> > > >> > >>> >>> valid CPU options. To simplyfy the management of specificying which CPU >> > > >> > >>> >>> is/isn't valid let's create a property that can be set in the machine >> > > >> > >>> >>> init. We can then check to see if the user supplied CPU is in that list >> > > >> > >>> >>> or not. >> > > >> > >>> >>> >> > > >> > >>> >>> I have added the valid_cpu_types for some ARM machines only at the >> > > >> > >>> >>> moment. >> > > >> > >>> >>> >> > > >> > >>> >>> Here is what specifying the CPUs looks like now: >> > > >> > >>> >>> >> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S >> > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information >> > > >> > >>> >>> (qemu) info cpus >> > > >> > >>> >>> * CPU #0: thread_id=24175 >> > > >> > >>> >>> (qemu) q >> > > >> > >>> >>> >> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S >> > > >> > >>> >>> QEMU 2.10.50 monitor - type 'help' for more information >> > > >> > >>> >>> (qemu) q >> > > >> > >>> >>> >> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S >> > > >> > >>> >>> qemu-system-aarch64: unable to find CPU model 'cortex-m5' >> > > >> > >>> >>> >> > > >> > >>> >>> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S >> > > >> > >>> >>> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu >> > > >> > >>> >>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu >> > > >> > >>> >> >> > > >> > >>> >> Thanks for this; we really should be more strict about >> > > >> > >>> >> forbidding "won't work" combinations than we have >> > > >> > >>> >> been in the past. >> > > >> > >>> >> >> > > >> > >>> >> In the last of these cases, I think that when we >> > > >> > >>> >> list the invalid CPU type and the valid types >> > > >> > >>> >> we should use the same names we want the user to >> > > >> > >>> >> use on the command line, without the "-arm-cpu" >> > > >> > >>> >> suffixes. >> > > >> > >>> > >> > > >> > >>> > Hmm... That is a good point, it is confusing that they don't line up. >> > > >> > >> >> > > >> > >> Agreed. >> > > >> > >> >> > > >> > >>> > >> > > >> > >>> > The problem is that we are just doing a simple >> > > >> > >>> > object_class_dynamic_cast() in hw/core/machine.c which I think >> > > >> > >>> > (untested) requires us to have the full name in the valid cpu array. >> > > >> > >> [...] >> > > >> > >>> >> > > >> > >>> I think an earlier version of my previous series adding the support to >> > > >> > >>> machine.c did string comparison, but it was decided to utilise objects >> > > >> > >>> instead. One option is to make the array 2 wide and have the second >> > > >> > >>> string be user friendly? >> > > >> > >> >> > > >> > >> Making the array 2-column will duplicate information that we can >> > > >> > >> already find out using other methods, and it won't solve the >> > > >> > >> problem if an entry has a parent class with multiple subclasses >> > > >> > >> (the original reason I suggested object_class_dynamic_cast()). >> > > >> > >> >> > > >> > >> The main obstacle to fix this easily is that we do have a common >> > > >> > >> ObjectClass *cpu_class_by_name(const char *cpu_model) >> > > >> > >> function, but not a common method to get the model name from a >> > > >> > >> CPUClass. Implementing this is possible, but probably better to >> > > >> > >> do it after moving the existing arch-specific CPU model >> > > >> > >> enumeration hooks to common code (currently we duplicate lots of >> > > >> > >> CPU enumeration/lookup boilerplate code that we shouldn't have >> > > >> > >> to). >> > > >> > >> >> > > >> > >> Listing only the human-friendly names in the array like in the >> > > >> > >> original patch could be a reasonable temporary solution. It >> > > >> > >> won't allow us to use a single entry for all subclasses of a >> > > >> > >> given type by now (e.g. listing only TYPE_X86_CPU on PC), but at >> > > >> > >> least we can address this issue without waiting for a refactor of >> > > >> > >> the CPU model enumeration code. >> > > >> > >> > > >> > Ah, I just re-read this. Do you mean go back to the original RFC and >> > > >> > just use strcmp() to compare the human readable cpu_model? >> > > >> It's sort of going backwards but I won't object to this as far as you >> > > >> won't use machine->cpu_model (which is in process of being removed) >> > > >> > > Wait, machine->cpu_model is the human readable name. Without using >> > > that we can't use just human readable strings for the valid cpu types. >> > >> > Well, if we want to deprecate machine->cpu_model we need to offer >> > an alternative first, otherwise we can't prevent people from >> > using it. >> > >> > Igor, do you see an (existing) alternative to machine->cpu_model >> > that would allow us to avoid using it in >> > machine_run_board_init()? >> In recently merged refactoring machine->cpu_model is being replaced >> by machine->cpu_type. So currently we don't need machine->cpu_model >> anywhere except machine('none'), and once I refactor that it could >> be dropped completely and after some work on *-user targets we can >> practically get rid of cpu_model notion completely >> (excluding of -cpu option parser). >> >> My dislike of idea is that it's adding back cpumodel strings >> in boards code again (which I've just got rid of). >> >> I hate to say that but it looks like we need more refactoring >> for this series to print cpumodels back to user. >> >> We already have FOO_cpu_list()/FOO_query_cpu_definitions() >> which already do cpu type => cpumodel conversion (and even >> have some code duplication within a target), I'd suggest >> generalizing that across targets and then using generic >> helper for printing back to user converted cpu types from >> mc->valid_cpu_types which this series introduces. > > I agree with the long term goal of making cpu type => cpu model > conversion generic. But until we refactor the arch-specific code > and implement that, we have 2 options: > > 1) Keep printing a confusing error message until we implement cpu > type => cpu model conversion; > 2) Keep the MachineState::cpu_model field until we implement cpu > type => cpu model conversion. > > I don't see any reason to pick (1) instead of (2). Ok, I'm going over this again (sorry for the delay). I'm going to respin using machine->cpu_model. It shouldn't be hard to refactor in the future to machine->cpu_type, once that is or can be translated to a user friendly string. Alistair > > -- > Eduardo >
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c index 111a1d0aba..702e5e8fcf 100644 --- a/hw/arm/netduino2.c +++ b/hw/arm/netduino2.c @@ -42,8 +42,8 @@ static void netduino2_init(MachineState *machine) } static const char *netduino_valid_cpus[] = { - ARM_CPU_TYPE_NAME("cortex-m3"), - ARM_CPU_TYPE_NAME("cortex-m4"), + "cortex-m3", + "cortex-m4", NULL }; diff --git a/hw/core/machine.c b/hw/core/machine.c index c857f3f934..4b817ccc58 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -761,8 +761,8 @@ void machine_run_board_init(MachineState *machine) /* If the machine supports the valid_cpu_types check and the user * specified a CPU with -cpu check here that the user CPU is supported. */ - if (machine_class->valid_cpu_types && machine->cpu_type) { - ObjectClass *class = object_class_by_name(machine->cpu_type); + if (machine_class->valid_cpu_types && machine->cpu_model) { + ObjectClass *class = object_class_by_name(machine->cpu_model); int i; for (i = 0; machine_class->valid_cpu_types[i]; i++) { @@ -777,7 +777,7 @@ void machine_run_board_init(MachineState *machine) if (!machine_class->valid_cpu_types[i]) { /* The user specified CPU is not valid */ - error_report("Invalid CPU type: %s", machine->cpu_type); + error_report("Invalid CPU type: %s", machine->cpu_model); error_printf("The valid types are: %s", machine_class->valid_cpu_types[0]); for (i = 1; machine_class->valid_cpu_types[i]; i++) {