| Message ID | 1425169895-10783-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
Am 02.03.2015 um 14:37 schrieb Alexander Graf: > On 01.03.15 01:31, Andreas Färber wrote: >> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid >> double-registration of types: >> >> Registering `POWER5+-powerpc64-cpu' which already exists >> >> Taking the textual description of a CPU type as part of a new type name >> is plain wrong, and so is unconditionally registering a new type here. >> >> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > Doesn't this break p8 support? Maybe, but p5 support was in longer and this is definitely a regression and really really wrong. If you know a way to fix it without handing it back to the IBM guys for more thought, feel free to give it a shot. Andreas
On 02.03.15 14:42, Andreas Färber wrote: > Am 02.03.2015 um 14:37 schrieb Alexander Graf: >> On 01.03.15 01:31, Andreas Färber wrote: >>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid >>> double-registration of types: >>> >>> Registering `POWER5+-powerpc64-cpu' which already exists >>> >>> Taking the textual description of a CPU type as part of a new type name >>> is plain wrong, and so is unconditionally registering a new type here. >>> >>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >>> Cc: qemu-stable@nongnu.org >>> Signed-off-by: Andreas Färber <afaerber@suse.de> >> >> Doesn't this break p8 support? > > Maybe, but p5 support was in longer and this is definitely a regression > and really really wrong. If you know a way to fix it without handing it > back to the IBM guys for more thought, feel free to give it a shot. I honestly don't fully remember what this was about. Wasn't this our special KVM class that we use to create a compatible cpu type on the fly? Alexey, please take a look at it. Alex > > Andreas >
Am 02.03.2015 um 14:51 schrieb Alexander Graf: > On 02.03.15 14:42, Andreas Färber wrote: >> Am 02.03.2015 um 14:37 schrieb Alexander Graf: >>> On 01.03.15 01:31, Andreas Färber wrote: >>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid >>>> double-registration of types: >>>> >>>> Registering `POWER5+-powerpc64-cpu' which already exists >>>> >>>> Taking the textual description of a CPU type as part of a new type name >>>> is plain wrong, and so is unconditionally registering a new type here. >>>> >>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> Cc: qemu-stable@nongnu.org >>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>> >>> Doesn't this break p8 support? >> >> Maybe, but p5 support was in longer and this is definitely a regression >> and really really wrong. If you know a way to fix it without handing it >> back to the IBM guys for more thought, feel free to give it a shot. > > I honestly don't fully remember what this was about. Wasn't this our > special KVM class that we use to create a compatible cpu type on the fly? No, the class I create on the fly is a few lines above: pvr_pcc = ppc_cpu_class_by_pvr(host_pvr); if (pvr_pcc == NULL) { pvr_pcc = ppc_cpu_class_by_pvr_mask(host_pvr); } if (pvr_pcc == NULL) { return -1; } type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc)); type_register(&type_info); So, if no matching class is returned, we never reach the offending code. Here, a second type with the same parent was being created in the kvm_ppc_register_host_cpu_type() function that is supposed to create that host CPU type. Why? The host CPU type by definition should already have the right PVR taken from the host. kvmppc_host_cpu_class_init(): /* Now fix up the class with information we can query from the host */ pcc->pvr = mfpvr(); > Alexey, please take a look at it. Thanks, Andreas
On 03/03/2015 12:51 AM, Alexander Graf wrote: > > > On 02.03.15 14:42, Andreas Färber wrote: >> Am 02.03.2015 um 14:37 schrieb Alexander Graf: >>> On 01.03.15 01:31, Andreas Färber wrote: >>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid >>>> double-registration of types: >>>> >>>> Registering `POWER5+-powerpc64-cpu' which already exists >>>> >>>> Taking the textual description of a CPU type as part of a new type name >>>> is plain wrong, and so is unconditionally registering a new type here. >>>> >>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> Cc: qemu-stable@nongnu.org >>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>> >>> Doesn't this break p8 support? >> >> Maybe, but p5 support was in longer and this is definitely a regression >> and really really wrong. If you know a way to fix it without handing it >> back to the IBM guys for more thought, feel free to give it a shot. > > I honestly don't fully remember what this was about. Wasn't this our > special KVM class that we use to create a compatible cpu type on the fly? > > Alexey, please take a look at it. I sent a note yesterday :-/ Here it is again: With this revert, running qemu with HV KVM and -cpu POWER7 fails on real POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the first place. QEMU looks at classes first, and if not found - at aliases, so this worked. I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias for POWER5+_v2.1 (or POWER5+_0.0).
On 03.03.15 01:42, Alexey Kardashevskiy wrote: > On 03/03/2015 12:51 AM, Alexander Graf wrote: >> >> >> On 02.03.15 14:42, Andreas Färber wrote: >>> Am 02.03.2015 um 14:37 schrieb Alexander Graf: >>>> On 01.03.15 01:31, Andreas Färber wrote: >>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid >>>>> double-registration of types: >>>>> >>>>> Registering `POWER5+-powerpc64-cpu' which already exists >>>>> >>>>> Taking the textual description of a CPU type as part of a new type >>>>> name >>>>> is plain wrong, and so is unconditionally registering a new type here. >>>>> >>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >>>>> Cc: qemu-stable@nongnu.org >>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>> >>>> Doesn't this break p8 support? >>> >>> Maybe, but p5 support was in longer and this is definitely a regression >>> and really really wrong. If you know a way to fix it without handing it >>> back to the IBM guys for more thought, feel free to give it a shot. >> >> I honestly don't fully remember what this was about. Wasn't this our >> special KVM class that we use to create a compatible cpu type on the fly? >> >> Alexey, please take a look at it. > > > I sent a note yesterday :-/ Here it is again: > > With this revert, running qemu with HV KVM and -cpu POWER7 fails on real > POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of > POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the > first place. QEMU looks at classes first, and if not found - at aliases, > so this worked. > > I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias > for POWER5+_v2.1 (or POWER5+_0.0). Care to send a patch? Alex
On 03/04/2015 07:43 AM, Alexander Graf wrote: > > > On 03.03.15 01:42, Alexey Kardashevskiy wrote: >> On 03/03/2015 12:51 AM, Alexander Graf wrote: >>> >>> >>> On 02.03.15 14:42, Andreas Färber wrote: >>>> Am 02.03.2015 um 14:37 schrieb Alexander Graf: >>>>> On 01.03.15 01:31, Andreas Färber wrote: >>>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid >>>>>> double-registration of types: >>>>>> >>>>>> Registering `POWER5+-powerpc64-cpu' which already exists >>>>>> >>>>>> Taking the textual description of a CPU type as part of a new type >>>>>> name >>>>>> is plain wrong, and so is unconditionally registering a new type here. >>>>>> >>>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >>>>>> Cc: qemu-stable@nongnu.org >>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>> >>>>> Doesn't this break p8 support? >>>> >>>> Maybe, but p5 support was in longer and this is definitely a regression >>>> and really really wrong. If you know a way to fix it without handing it >>>> back to the IBM guys for more thought, feel free to give it a shot. >>> >>> I honestly don't fully remember what this was about. Wasn't this our >>> special KVM class that we use to create a compatible cpu type on the fly? >>> >>> Alexey, please take a look at it. >> >> >> I sent a note yesterday :-/ Here it is again: >> >> With this revert, running qemu with HV KVM and -cpu POWER7 fails on real >> POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of >> POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the >> first place. QEMU looks at classes first, and if not found - at aliases, >> so this worked. >> >> I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias >> for POWER5+_v2.1 (or POWER5+_0.0). > > Care to send a patch? I wonder if Andreas has a better solution to my initial problem - he obviously won't like the proposed patch :)
Am 03.03.2015 um 23:14 schrieb Alexey Kardashevskiy: > On 03/04/2015 07:43 AM, Alexander Graf wrote: >> On 03.03.15 01:42, Alexey Kardashevskiy wrote: >>> On 03/03/2015 12:51 AM, Alexander Graf wrote: >>>> On 02.03.15 14:42, Andreas Färber wrote: >>>>> Am 02.03.2015 um 14:37 schrieb Alexander Graf: >>>>>> On 01.03.15 01:31, Andreas Färber wrote: >>>>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to >>>>>>> avoid >>>>>>> double-registration of types: >>>>>>> >>>>>>> Registering `POWER5+-powerpc64-cpu' which already exists >>>>>>> >>>>>>> Taking the textual description of a CPU type as part of a new type >>>>>>> name >>>>>>> is plain wrong, and so is unconditionally registering a new type >>>>>>> here. >>>>>>> >>>>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >>>>>>> Cc: qemu-stable@nongnu.org >>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>>> >>>>>> Doesn't this break p8 support? >>>>> >>>>> Maybe, but p5 support was in longer and this is definitely a >>>>> regression >>>>> and really really wrong. If you know a way to fix it without >>>>> handing it >>>>> back to the IBM guys for more thought, feel free to give it a shot. >>>> >>>> I honestly don't fully remember what this was about. Wasn't this our >>>> special KVM class that we use to create a compatible cpu type on the >>>> fly? >>>> >>>> Alexey, please take a look at it. >>> >>> >>> I sent a note yesterday :-/ Here it is again: >>> >>> With this revert, running qemu with HV KVM and -cpu POWER7 fails on real >>> POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of >>> POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the >>> first place. QEMU looks at classes first, and if not found - at aliases, >>> so this worked. >>> >>> I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias >>> for POWER5+_v2.1 (or POWER5+_0.0). >> >> Care to send a patch? > > I wonder if Andreas has a better solution to my initial problem - he > obviously won't like the proposed patch :) Quite predictable, am I? ;) Could you please explain in detail what problem you are seeing on POWER8 without this patch? From your new patch it rather sounds as if this was totally unrelated to -cpu host and a new KVM-only feature, reinforcing my feeling that my function is the wrong place for your code. Also, as I pointed out, the description cannot safely be used as part of the type name, as it may contain prohibited characters, so this still needs fixing. And for sure, if registering new types is indeed needed here, then a check is needed for whether that type already exists and appropriate error handling. I just don't understand why that is needed at all with -cpu host taking the PVR as you say is needed here. If you can precisely tell me what it is that you need then I'd be happy to cook up a patch. Regards, Andreas
On 03/05/2015 01:55 AM, Andreas Färber wrote: > Am 03.03.2015 um 23:14 schrieb Alexey Kardashevskiy: >> On 03/04/2015 07:43 AM, Alexander Graf wrote: >>> On 03.03.15 01:42, Alexey Kardashevskiy wrote: >>>> On 03/03/2015 12:51 AM, Alexander Graf wrote: >>>>> On 02.03.15 14:42, Andreas Färber wrote: >>>>>> Am 02.03.2015 um 14:37 schrieb Alexander Graf: >>>>>>> On 01.03.15 01:31, Andreas Färber wrote: >>>>>>>> This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to >>>>>>>> avoid >>>>>>>> double-registration of types: >>>>>>>> >>>>>>>> Registering `POWER5+-powerpc64-cpu' which already exists >>>>>>>> >>>>>>>> Taking the textual description of a CPU type as part of a new type >>>>>>>> name >>>>>>>> is plain wrong, and so is unconditionally registering a new type >>>>>>>> here. >>>>>>>> >>>>>>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> >>>>>>>> Cc: qemu-stable@nongnu.org >>>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>>>> >>>>>>> Doesn't this break p8 support? >>>>>> >>>>>> Maybe, but p5 support was in longer and this is definitely a >>>>>> regression >>>>>> and really really wrong. If you know a way to fix it without >>>>>> handing it >>>>>> back to the IBM guys for more thought, feel free to give it a shot. >>>>> >>>>> I honestly don't fully remember what this was about. Wasn't this our >>>>> special KVM class that we use to create a compatible cpu type on the >>>>> fly? >>>>> >>>>> Alexey, please take a look at it. >>>> >>>> >>>> I sent a note yesterday :-/ Here it is again: >>>> >>>> With this revert, running qemu with HV KVM and -cpu POWER7 fails on real >>>> POWER7 machine as my machine has pvr 003f 0201 and POWER7 is an alias of >>>> POWER7_v2.3 (pvr 003f 0203); and this is what I tried to fix at the >>>> first place. QEMU looks at classes first, and if not found - at aliases, >>>> so this worked. >>>> >>>> I would rename "POWER5+" to "POWER5+_0.0" and make "POWER5+" an alias >>>> for POWER5+_v2.1 (or POWER5+_0.0). >>> >>> Care to send a patch? >> >> I wonder if Andreas has a better solution to my initial problem - he >> obviously won't like the proposed patch :) > > Quite predictable, am I? ;) > > Could you please explain in detail what problem you are seeing on POWER8 > without this patch? > > From your new patch it rather sounds as if this was totally unrelated to > -cpu host and a new KVM-only feature, reinforcing my feeling that my > function is the wrong place for your code. > > Also, as I pointed out, the description cannot safely be used as part of > the type name, as it may contain prohibited characters, so this still > needs fixing. So I can duplicate the CPU family name in PowerPCCPUClass. Which will always be the same as DeviceClass::desc. Well, it may be the right thing to do. > And for sure, if registering new types is indeed needed here, then a > check is needed for whether that type already exists and appropriate > error handling. I'll cook a patch for this. > I just don't understand why that is needed at all with > -cpu host taking the PVR as you say is needed here. > > If you can precisely tell me what it is that you need then I'd be happy > to cook up a patch. I thought I did... I need a way to run QEMU under HV KVM with a CPU name, just like this - -cpu POWER7 on _any_ real POWER7 CPU (v2.1, v2.3 or even POWER7+). Simply because all CPUs from the family behave the same. But HV KVM cannot virtualize PVR, it is a hardware limitation. So this is what my original patch fixed/bandaid'd. The original request for -cpu POWER7 vs. -cpu host came from libvirt for migration purposes - afair the issue was that the destination QEMU must not start if it is POWER8-host and the source is POWER7-host so trying -cpu POWER7 will fail on POWER8 (but -cpu host would work and this would be wrong); but migration between POWER7 2.1 and POWER7 2.3 should still work. And in general there is no good reason why -cpu POWER7 cannot work on any POWER7 CPU. I could try adding a dynamic alias for "POWER7" to "host" but at the moment aliases are static so new dynamic class seemed simpler.
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 1edf2b5..9d614ef 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -2160,18 +2160,6 @@ bool kvmppc_has_cap_fixup_hcalls(void) return cap_fixup_hcalls; } -static PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc) -{ - ObjectClass *oc = OBJECT_CLASS(pcc); - - while (oc && !object_class_is_abstract(oc)) { - oc = object_class_get_parent(oc); - } - assert(oc); - - return POWERPC_CPU_CLASS(oc); -} - static int kvm_ppc_register_host_cpu_type(void) { TypeInfo type_info = { @@ -2181,7 +2169,6 @@ static int kvm_ppc_register_host_cpu_type(void) }; uint32_t host_pvr = mfpvr(); PowerPCCPUClass *pvr_pcc; - DeviceClass *dc; pvr_pcc = ppc_cpu_class_by_pvr(host_pvr); if (pvr_pcc == NULL) { @@ -2192,14 +2179,6 @@ static int kvm_ppc_register_host_cpu_type(void) } type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc)); type_register(&type_info); - - /* Register generic family CPU class for a family */ - pvr_pcc = ppc_cpu_get_family_class(pvr_pcc); - dc = DEVICE_CLASS(pvr_pcc); - type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc)); - type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU, dc->desc); - type_register(&type_info); - return 0; }
This reverts commit 5b79b1cadd3e565b6d1a5ba59764bd47af58b271 to avoid double-registration of types: Registering `POWER5+-powerpc64-cpu' which already exists Taking the textual description of a CPU type as part of a new type name is plain wrong, and so is unconditionally registering a new type here. Cc: Alexey Kardashevskiy <aik@ozlabs.ru> Cc: qemu-stable@nongnu.org Signed-off-by: Andreas Färber <afaerber@suse.de> --- target-ppc/kvm.c | 21 --------------------- 1 file changed, 21 deletions(-)