[PATCH] x86: Check for machine state object class before typecasting it

[PATCH] x86: Check for machine state object class before typecasting it

[Michal Privoznik]

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(+)

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);
-- 
2.24.1

Re: [PATCH] x86: Check for machine state object class before typecasting it

[no-reply]

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

Re: [PATCH] x86: Check for machine state object class before typecasting it

[Philippe Mathieu-Daudé]

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);
> 

Re: [PATCH] x86: Check for machine state object class before typecasting it

[Michal Prívozník]

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

Re: [PATCH] x86: Check for machine state object class before typecasting it

[Philippe Mathieu-Daudé]

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
> 

Re: [PATCH] x86: Check for machine state object class before typecasting it

[Michal Prívozník]

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

Re: [PATCH] x86: Check for machine state object class before typecasting it

[Philippe Mathieu-Daudé]

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 :)

Re: [PATCH] x86: Check for machine state object class before typecasting it

[Paolo Bonzini]

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