| Message ID | 7cc91bab3191bfd7e071bdd3fdf7fe2a2991deb0.1577692822.git.mprivozn@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Check for machine state object class before typecasting it | expand |
Patchew URL: https://patchew.org/QEMU/7cc91bab3191bfd7e071bdd3fdf7fe2a2991deb0.1577692822.git.mprivozn@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/7cc91bab3191bfd7e071bdd3fdf7fe2a2991deb0.1577692822.git.mprivozn@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 12/30/19 9:00 AM, Michal Privoznik wrote: > In v4.2.0-246-ged9e923c3c the SMM property was moved from PC Typo v4.2.0-246-ged9e923c3c -> ed9e923c3c. > machine class to x86 machine class. Makes sense, but the change > was too aggressive: in target/i386/kvm.c:kvm_arch_init() it > altered check which sets SMRAM if given machine has SMM enabled. > The line that detects whether given machine object is class of > PC_MACHINE was removed from the check. This makes qemu try to > enable SMRAM for all machine types, which is not what we want. > Fixes: ed9e923c3c Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > target/i386/kvm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 0b511906e3..7ee3202634 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -2173,6 +2173,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s) > } > > if (kvm_check_extension(s, KVM_CAP_X86_SMM) && > + object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE) && > x86_machine_is_smm_enabled(X86_MACHINE(ms))) { > smram_machine_done.notify = register_smram_listener; > qemu_add_machine_init_done_notifier(&smram_machine_done); >
On 12/30/19 9:41 AM, Philippe Mathieu-Daudé wrote: > On 12/30/19 9:00 AM, Michal Privoznik wrote: >> In v4.2.0-246-ged9e923c3c the SMM property was moved from PC > > Typo v4.2.0-246-ged9e923c3c -> ed9e923c3c. This depends on how you format the hash :-) I've used 'git describe ed9e923c3c' because I find it more readable for us humans (at least we see what version the commit was introduced in). But I don't know what the praxis is in qemu. > >> machine class to x86 machine class. Makes sense, but the change >> was too aggressive: in target/i386/kvm.c:kvm_arch_init() it >> altered check which sets SMRAM if given machine has SMM enabled. >> The line that detects whether given machine object is class of >> PC_MACHINE was removed from the check. This makes qemu try to >> enable SMRAM for all machine types, which is not what we want. >> > > Fixes: ed9e923c3c > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks, Michal
On 12/30/19 10:35 AM, Michal Prívozník wrote: > On 12/30/19 9:41 AM, Philippe Mathieu-Daudé wrote: >> On 12/30/19 9:00 AM, Michal Privoznik wrote: >>> In v4.2.0-246-ged9e923c3c the SMM property was moved from PC >> >> Typo v4.2.0-246-ged9e923c3c -> ed9e923c3c. > > This depends on how you format the hash :-) > I've used 'git describe ed9e923c3c' because I find it more readable for > us humans (at least we see what version the commit was introduced in). > But I don't know what the praxis is in qemu. Hmm I never used it. Your explanation makes sense, but the tag confused me because I don't have it locally. However git (and gitk) seems clever enough to only use the useful part: $ git show randomcrap-ged9e923c3c commit ed9e923c3c9a2c50c4e82ba178b3fb1feba56867 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Thu Dec 12 17:28:01 2019 +0100 x86: move SMM property to X86MachineState FYI My output is different: $ git describe ed9e923c3c pull-target-arm-20191216-1-199-ged9e923c3c >>> machine class to x86 machine class. Makes sense, but the change >>> was too aggressive: in target/i386/kvm.c:kvm_arch_init() it >>> altered check which sets SMRAM if given machine has SMM enabled. >>> The line that detects whether given machine object is class of >>> PC_MACHINE was removed from the check. This makes qemu try to >>> enable SMRAM for all machine types, which is not what we want. >>> >> >> Fixes: ed9e923c3c >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Thanks, > Michal >
On 12/30/19 10:45 AM, Philippe Mathieu-Daudé wrote: > On 12/30/19 10:35 AM, Michal Prívozník wrote: >> On 12/30/19 9:41 AM, Philippe Mathieu-Daudé wrote: >>> On 12/30/19 9:00 AM, Michal Privoznik wrote: >>>> In v4.2.0-246-ged9e923c3c the SMM property was moved from PC >>> >>> Typo v4.2.0-246-ged9e923c3c -> ed9e923c3c. >> >> This depends on how you format the hash :-) >> I've used 'git describe ed9e923c3c' because I find it more readable for >> us humans (at least we see what version the commit was introduced in). >> But I don't know what the praxis is in qemu. > > Hmm I never used it. Your explanation makes sense, but the tag confused > me because I don't have it locally. However git (and gitk) seems clever > enough to only use the useful part: The v4.2.0 tag is in origin. I wonder how come you do not have it. > > $ git show randomcrap-ged9e923c3c > commit ed9e923c3c9a2c50c4e82ba178b3fb1feba56867 > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Thu Dec 12 17:28:01 2019 +0100 > > x86: move SMM property to X86MachineState > > FYI My output is different: > > $ git describe ed9e923c3c > pull-target-arm-20191216-1-199-ged9e923c3c You may want to use 'git describe --tags --match "v*" $commit' But again, feel free to change it to whatever you/committer wants. Michal
On 12/30/19 2:17 PM, Michal Prívozník wrote: > On 12/30/19 10:45 AM, Philippe Mathieu-Daudé wrote: >> On 12/30/19 10:35 AM, Michal Prívozník wrote: >>> On 12/30/19 9:41 AM, Philippe Mathieu-Daudé wrote: >>>> On 12/30/19 9:00 AM, Michal Privoznik wrote: >>>>> In v4.2.0-246-ged9e923c3c the SMM property was moved from PC >>>> >>>> Typo v4.2.0-246-ged9e923c3c -> ed9e923c3c. >>> >>> This depends on how you format the hash :-) >>> I've used 'git describe ed9e923c3c' because I find it more readable for >>> us humans (at least we see what version the commit was introduced in). >>> But I don't know what the praxis is in qemu. >> >> Hmm I never used it. Your explanation makes sense, but the tag confused >> me because I don't have it locally. However git (and gitk) seems clever >> enough to only use the useful part: > > The v4.2.0 tag is in origin. I wonder how come you do not have it. > >> >> $ git show randomcrap-ged9e923c3c >> commit ed9e923c3c9a2c50c4e82ba178b3fb1feba56867 >> Author: Paolo Bonzini <pbonzini@redhat.com> >> Date: Thu Dec 12 17:28:01 2019 +0100 >> >> x86: move SMM property to X86MachineState >> >> FYI My output is different: >> >> $ git describe ed9e923c3c >> pull-target-arm-20191216-1-199-ged9e923c3c > > You may want to use 'git describe --tags --match "v*" $commit' Oh yes I have it :> $ git describe --tags --match "v*" ed9e923c3c v4.2.0-246-ged9e923c3c The difference seems to be I have other tags between v4.2.0 and ed9e923c3c, and git-describe choose the closest (newer?). > > But again, feel free to change it to whatever you/committer wants. This is fine, no problem, today I learned something new :)
On 30/12/19 14:17, Michal Prívozník wrote: > On 12/30/19 10:45 AM, Philippe Mathieu-Daudé wrote: >> On 12/30/19 10:35 AM, Michal Prívozník wrote: >>> On 12/30/19 9:41 AM, Philippe Mathieu-Daudé wrote: >>>> On 12/30/19 9:00 AM, Michal Privoznik wrote: >>>>> In v4.2.0-246-ged9e923c3c the SMM property was moved from PC >>>> >>>> Typo v4.2.0-246-ged9e923c3c -> ed9e923c3c. >>> >>> This depends on how you format the hash :-) >>> I've used 'git describe ed9e923c3c' because I find it more readable for >>> us humans (at least we see what version the commit was introduced in). >>> But I don't know what the praxis is in qemu. >> >> Hmm I never used it. Your explanation makes sense, but the tag confused >> me because I don't have it locally. However git (and gitk) seems clever >> enough to only use the useful part: > > The v4.2.0 tag is in origin. I wonder how come you do not have it. > >> >> $ git show randomcrap-ged9e923c3c Cool, I didn't know this. I usually format it like ed9e923c3c ("x86: move SMM property to X86MachineState", 2019-12-17) I can see why most developers don't use "git describe", because at least those that also are maintainers will often have intermediate tags after a release. Anyway I've queued this patch, thanks. Paolo
diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 0b511906e3..7ee3202634 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -2173,6 +2173,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s) } if (kvm_check_extension(s, KVM_CAP_X86_SMM) && + object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE) && x86_machine_is_smm_enabled(X86_MACHINE(ms))) { smram_machine_done.notify = register_smram_listener; qemu_add_machine_init_done_notifier(&smram_machine_done);
In v4.2.0-246-ged9e923c3c the SMM property was moved from PC machine class to x86 machine class. Makes sense, but the change was too aggressive: in target/i386/kvm.c:kvm_arch_init() it altered check which sets SMRAM if given machine has SMM enabled. The line that detects whether given machine object is class of PC_MACHINE was removed from the check. This makes qemu try to enable SMRAM for all machine types, which is not what we want. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- target/i386/kvm.c | 1 + 1 file changed, 1 insertion(+)