| Message ID | 1424183053-4310-14-git-send-email-mimu@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 17.02.15 15:24, Michael Mueller wrote: > This patch implements the functions: > > - s390_get_proceccor_props() > - s390_set_proceccor_props() > > They can be used to request or retrieve processor related information from an accelerator. > That information comprises the cpu identifier, the ICB value and the facility lists. > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> Hrm, I still seem to miss the point of this interface. What do you need it for? Alex
On Fri, 20 Feb 2015 15:03:30 +0100 Alexander Graf <agraf@suse.de> wrote: > > > > - s390_get_proceccor_props() > > - s390_set_proceccor_props() > > > > They can be used to request or retrieve processor related information from an accelerator. > > That information comprises the cpu identifier, the ICB value and the facility lists. > > > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > > Hrm, I still seem to miss the point of this interface. What do you need > it for? These functions make the internal s390 cpu model API independent from a specific accelerator: int s390_set_processor_props(S390ProcessorProps *prop) { if (kvm_enabled()) { return kvm_s390_set_processor_props(prop); } return -ENOSYS; } It's called by: s390_select_cpu_model(const char *model) which is itself called by: S390CPU *cpu_s390x_init(const char *cpu_model) { S390CPU *cpu; cpu = S390_CPU(object_new(s390_select_cpu_model(cpu_model))); object_property_set_bool(OBJECT(cpu), true, "realized", NULL); return cpu; } So above s390_set/get_processor_props() the code is accelerator independent. Michael
Am 20.02.2015 um 16:32 schrieb Michael Mueller: > On Fri, 20 Feb 2015 15:03:30 +0100 > Alexander Graf <agraf@suse.de> wrote: > >>> >>> - s390_get_proceccor_props() >>> - s390_set_proceccor_props() >>> >>> They can be used to request or retrieve processor related information from an accelerator. >>> That information comprises the cpu identifier, the ICB value and the facility lists. >>> >>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> >> >> Hrm, I still seem to miss the point of this interface. What do you need >> it for? > > These functions make the internal s390 cpu model API independent from a specific accelerator: > > int s390_set_processor_props(S390ProcessorProps *prop) > { > if (kvm_enabled()) { > return kvm_s390_set_processor_props(prop); > } > return -ENOSYS; > } > > It's called by: > > s390_select_cpu_model(const char *model) > > which is itself called by: > > S390CPU *cpu_s390x_init(const char *cpu_model) > { > S390CPU *cpu; > > cpu = S390_CPU(object_new(s390_select_cpu_model(cpu_model))); > > object_property_set_bool(OBJECT(cpu), true, "realized", NULL); > > return cpu; > } > > So above s390_set/get_processor_props() the code is accelerator independent. Can't you just implement the class-level name-to-ObjectClass callback that other CPUs have grown for the above use case? Also a general comment: cpu-model/ is not an existing directory nor one you add, so please use "target-s390x: Add foo to S390CPU" or so. Regards, Andreas
On Fri, 20 Feb 2015 16:41:49 +0100 Andreas Färber <afaerber@suse.de> wrote: > Can't you just implement the class-level name-to-ObjectClass callback > that other CPUs have grown for the above use case? If it fulfills the requirements sure. Please point me to an example, sounds that s390_select_cpu_model() is doing something similar to that, just that it hooks in the s390_set_processor_props() call. const char *s390_select_cpu_model(const char *model) { S390ProcessorProps proc; const char *typename; S390CPUClass *cc; /* return already selected cpu typename */ typename = s390_cpu_typename(); if (typename) { goto out; } /* return standard cpu typename when cpu models are unavailable */ typename = TYPE_S390_CPU; if (!s390_cpu_classes_initialized() || !model) { goto out; } cc = S390_CPU_CLASS(s390_cpu_class_by_name(model)); if (!cc) { goto out; } proc.cpuid = cpuid(cc->proc); proc.ibc = cc->proc->ibc; memcpy(proc.fac_list, cc->fac_list, S390_ARCH_FAC_LIST_SIZE_BYTE); if (s390_set_processor_props(&proc)) { goto out; } /* return requested cpu typename in success case */ typename = object_class_get_name((ObjectClass *) cc); out: selected_cpu_typename = typename; trace_select_cpu_model(model, typename); return typename; } > > Also a general comment: cpu-model/ is not an existing directory nor one > you add, so please use "target-s390x: Add foo to S390CPU" or so.
On Fri, 20 Feb 2015 16:41:49 +0100 Andreas Färber <afaerber@suse.de> wrote: > Also a general comment: cpu-model/ is not an existing directory nor one > you add, so please use "target-s390x: Add foo to S390CPU" or so. I will address this with v3, thanks a lot for the hint, I never saw this as directories though... Michael
Am 20.02.2015 um 17:04 schrieb Michael Mueller: > On Fri, 20 Feb 2015 16:41:49 +0100 > Andreas Färber <afaerber@suse.de> wrote: > >> Can't you just implement the class-level name-to-ObjectClass callback >> that other CPUs have grown for the above use case? > > If it fulfills the requirements sure. Please point me to an example, Take a look at include/qom/cpu.h CPUClass::class_by_name and git-grep the existing targets - most implement it already. It's a generic hook to be used from everywhere rather than a local function specific to the legacy init function. Apart from the error handling it should be straight-forward. > sounds that > s390_select_cpu_model() is doing something similar to that, just that it hooks in > the s390_set_processor_props() call. > > const char *s390_select_cpu_model(const char *model) > { > S390ProcessorProps proc; > const char *typename; > S390CPUClass *cc; > > /* return already selected cpu typename */ > typename = s390_cpu_typename(); > if (typename) { > goto out; > } > > /* return standard cpu typename when cpu models are unavailable */ > typename = TYPE_S390_CPU; > if (!s390_cpu_classes_initialized() || !model) { > goto out; > } > cc = S390_CPU_CLASS(s390_cpu_class_by_name(model)); > if (!cc) { > goto out; > } > proc.cpuid = cpuid(cc->proc); > proc.ibc = cc->proc->ibc; > memcpy(proc.fac_list, cc->fac_list, S390_ARCH_FAC_LIST_SIZE_BYTE); > if (s390_set_processor_props(&proc)) { > goto out; > } Sorry for my ignorance, but what is proc actually needed for? For initializing the class, there's .class_init (and cc->fac_list apparently is initialized here). If you need to pass info to KVM, you can do so in DeviceClass::realize when the vCPU actually goes "live". A string-to-string (or string-to-ObjectClass) translation function seems like a weird point in time to take action with global effect. Anyway, please implement the generic callback, then you can still call it from your own helper functions if needed. Regards, Andreas > > /* return requested cpu typename in success case */ > typename = object_class_get_name((ObjectClass *) cc); > out: > selected_cpu_typename = typename; > trace_select_cpu_model(model, typename); > return typename; > }
On Fri, 20 Feb 2015 17:28:14 +0100 Andreas Färber <afaerber@suse.de> wrote: Andreas, > Sorry for my ignorance, but what is proc actually needed for? For > initializing the class, there's .class_init (and cc->fac_list apparently > is initialized here). If you need to pass info to KVM, you can do so in yes, it is communication to the accelerator to prepare its local cpu model related data structures which are used to initialize a vcpu (e.g. the facility list beside others) > DeviceClass::realize when the vCPU actually goes "live". A I will look what "goes live" in detail means here, it should at least be before kvm_arch_vcpu_setup() gets triggered on accelerator side. > string-to-string (or string-to-ObjectClass) translation function seems > like a weird point in time to take action with global effect. > > Anyway, please implement the generic callback, then you can still call > it from your own helper functions if needed. Thanks a lot! Michael
On 20.02.15 16:32, Michael Mueller wrote: > On Fri, 20 Feb 2015 15:03:30 +0100 > Alexander Graf <agraf@suse.de> wrote: > >>> >>> - s390_get_proceccor_props() >>> - s390_set_proceccor_props() >>> >>> They can be used to request or retrieve processor related information from an accelerator. >>> That information comprises the cpu identifier, the ICB value and the facility lists. >>> >>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> >> >> Hrm, I still seem to miss the point of this interface. What do you need >> it for? > > These functions make the internal s390 cpu model API independent from a specific accelerator: > > int s390_set_processor_props(S390ProcessorProps *prop) > { > if (kvm_enabled()) { > return kvm_s390_set_processor_props(prop); > } > return -ENOSYS; > } > > It's called by: > > s390_select_cpu_model(const char *model) > > which is itself called by: > > S390CPU *cpu_s390x_init(const char *cpu_model) > { > S390CPU *cpu; > > cpu = S390_CPU(object_new(s390_select_cpu_model(cpu_model))); > > object_property_set_bool(OBJECT(cpu), true, "realized", NULL); > > return cpu; > } > > So above s390_set/get_processor_props() the code is accelerator independent. Any particular reason you can't do it like PPC? Alex
On Fri, 20 Feb 2015 18:00:19 +0100 Alexander Graf <agraf@suse.de> wrote: > > So above s390_set/get_processor_props() the code is accelerator independent. > > Any particular reason you can't do it like PPC? That seems to be a short question... and when I started one year ago, I oriented myself on the PPC version and I'm also willing to revisit it but I can't give you a quick answer different from no currently to that. There are no PVRs for s390x CPUs and thus I came up with "pseudo PVRs": /* * bits 0-7 : CMOS generation * bits 8-9 : reserved * bits 10-11 : machine class 0=unknown 1=EC 2=BC * bits 12-15 : GA * bits 16-31 : machine type * * note: bits are named according to s390 * architecture specific endienness */ enum { CPU_S390_2064_GA1 = 0x07112064, CPU_S390_2064_GA2 = 0x07122064, CPU_S390_2064_GA3 = 0x07132064, CPU_S390_2066_GA1 = 0x07212066, CPU_S390_2084_GA1 = 0x08112084, CPU_S390_2084_GA2 = 0x08122084, CPU_S390_2084_GA3 = 0x08132084, CPU_S390_2084_GA4 = 0x08142084, CPU_S390_2084_GA5 = 0x08152084, CPU_S390_2086_GA1 = 0x08212086, CPU_S390_2086_GA2 = 0x08222086, CPU_S390_2086_GA3 = 0x08232086, CPU_S390_2094_GA1 = 0x09112094, CPU_S390_2094_GA2 = 0x09122094, CPU_S390_2094_GA3 = 0x09132094, CPU_S390_2096_GA1 = 0x09212096, CPU_S390_2096_GA2 = 0x09222096, CPU_S390_2097_GA1 = 0x0a112097, CPU_S390_2097_GA2 = 0x0a122097, CPU_S390_2097_GA3 = 0x0a132097, CPU_S390_2098_GA1 = 0x0a212098, CPU_S390_2098_GA2 = 0x0a222098, CPU_S390_2817_GA1 = 0x0b112817, CPU_S390_2817_GA2 = 0x0b122817, CPU_S390_2818_GA1 = 0x0b212818, CPU_S390_2827_GA1 = 0x0c112827, CPU_S390_2827_GA2 = 0x0c122827, CPU_S390_2828_GA1 = 0x0c212828, CPU_S390_2964_GA1 = 0x0d112964, }; And initially I had a version that was limiting the accelerator to be able to implement just them with all their properties encapsulated in the a accelerator as well. After identifying the real processor related attributes defining the model, I changed the interface such that KVM or other accelerators give hints what it is able to support in dependency of the current code version and the hosting machine and let QEMU decide how to set these attributes (cpuid,ibc,fac_list). Thus I think the implementation is now quite open and easily adoptable also for TCG and possibly others as well. Eventually the integration and also some trigger points of my code are to adjust. So coming back to your question, the answer is still no for the whole item but eventually yes if you have limited it to the s390_set/get_processor_props() triggers. But I have to look into it first again. I will do that when I'm back on Tuesday morning. Thanks and have a nice WE Michael
diff --git a/target-s390x/cpu-models.c b/target-s390x/cpu-models.c index c63b4c2..9bc4d89 100644 --- a/target-s390x/cpu-models.c +++ b/target-s390x/cpu-models.c @@ -579,6 +579,42 @@ void s390_cpu_list_entry(gpointer data, gpointer user_data) } /** + * s390_get_processor_props - retrieves processor properties from + * from the currently used accelerator + * @prop: address to processor property structure to store the + * retrieved processor properties + * + * Returns: 0 in case of success + * -ENOSYS in case the current accelerator has no processor + * properties implemented + */ +int s390_get_processor_props(S390ProcessorProps *prop) +{ + if (kvm_enabled()) { + return kvm_s390_get_processor_props(prop); + } + return -ENOSYS; +} + +/** + * s390_set_processor_props - stores processor properties in the + * currently used accelerator + * @prop: address to processor property structure to store the + * retrieved processor properties + * + * Returns: 0 in case of success + * -ENOSYS in case the current accelerator has no processor + * properties implemented + */ +int s390_set_processor_props(S390ProcessorProps *prop) +{ + if (kvm_enabled()) { + return kvm_s390_set_processor_props(prop); + } + return -ENOSYS; +} + +/** * s390_cpu_classes_initialized - indicates if the all cpu classes and * their properties have been initialized * diff --git a/target-s390x/cpu-models.h b/target-s390x/cpu-models.h index d41fc37..bc478d8 100644 --- a/target-s390x/cpu-models.h +++ b/target-s390x/cpu-models.h @@ -108,6 +108,8 @@ int s390_setup_cpu_classes(AccelId accel, S390MachineProps *prop); gint s390_cpu_class_asc_order_compare(gconstpointer a, gconstpointer b); gint s390_cpu_class_desc_order_compare(gconstpointer a, gconstpointer b); void s390_cpu_list_entry(gpointer data, gpointer user_data); +int s390_get_processor_props(S390ProcessorProps *prop); +int s390_set_processor_props(S390ProcessorProps *prop); bool s390_cpu_classes_initialized(void); bool s390_test_facility(S390CPUClass *cc, unsigned long nr); bool s390_probe_mode(void);
This patch implements the functions: - s390_get_proceccor_props() - s390_set_proceccor_props() They can be used to request or retrieve processor related information from an accelerator. That information comprises the cpu identifier, the ICB value and the facility lists. Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> --- target-s390x/cpu-models.c | 36 ++++++++++++++++++++++++++++++++++++ target-s390x/cpu-models.h | 2 ++ 2 files changed, 38 insertions(+)