* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
@ 2020-03-06 2:17 linmiaohe
2020-03-06 5:32 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: linmiaohe @ 2020-03-06 2:17 UTC (permalink / raw)
To: Paolo Bonzini, vkuznets
Cc: rkrcmar, sean.j.christopherson, jmattson, joro, tglx, mingo, bp,
hpa, kvm, linux-kernel, x86
Paolo Bonzini <pbonzini@redhat.com> wrote:
>On 05/03/20 03:35, linmiaohe wrote:
>> (X86_EFLAGS_IOPL | X86_EFLAGS_VM) indicates the eflag bits that can
>> not be owned by realmode guest, i.e. ~RMODE_GUEST_OWNED_EFLAGS_BITS.
>
>... but ~RMODE_GUEST_OWNED_EFLAGS_BITS is the bits that are owned by the host; they could be 0 or 1 and that's why the code was using X86_EFLAGS_IOPL | X86_EFLAGS_VM.
>
>I understand where ~RMODE_GUEST_OWNED_EFLAGS_BITS is better than X86_EFLAGS_IOPL | X86_EFLAGS_VM, but I cannot think of a way to express it that is the best of both worlds.
>
Define a macro RMODE_HOST_OWNED_EFLAGS_BITS for (X86_EFLAGS_IOPL | X86_EFLAGS_VM) as suggested by Vitaly seems a good way to fix this ? Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
2020-03-06 2:17 [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly linmiaohe
@ 2020-03-06 5:32 ` Paolo Bonzini
2020-03-06 9:44 ` Vitaly Kuznetsov
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2020-03-06 5:32 UTC (permalink / raw)
To: linmiaohe, vkuznets
Cc: rkrcmar, sean.j.christopherson, jmattson, joro, tglx, mingo, bp,
hpa, kvm, linux-kernel, x86
On 06/03/20 03:17, linmiaohe wrote:
> Define a macro RMODE_HOST_OWNED_EFLAGS_BITS for (X86_EFLAGS_IOPL |
> X86_EFLAGS_VM) as suggested by Vitaly seems a good way to fix this ?
> Thanks.
No, what if a host-owned flag was zero? I'd just leave it as is.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
2020-03-06 5:32 ` Paolo Bonzini
@ 2020-03-06 9:44 ` Vitaly Kuznetsov
2020-03-06 10:00 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Vitaly Kuznetsov @ 2020-03-06 9:44 UTC (permalink / raw)
To: Paolo Bonzini, linmiaohe
Cc: rkrcmar, sean.j.christopherson, jmattson, joro, tglx, mingo, bp,
hpa, kvm, linux-kernel, x86
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 06/03/20 03:17, linmiaohe wrote:
>> Define a macro RMODE_HOST_OWNED_EFLAGS_BITS for (X86_EFLAGS_IOPL |
>> X86_EFLAGS_VM) as suggested by Vitaly seems a good way to fix this ?
>> Thanks.
>
> No, what if a host-owned flag was zero? I'd just leave it as is.
>
I'm not saying my suggestion was a good idea but honestly I'm failing to
wrap my head around this. The suggested 'RMODE_HOST_OWNED_EFLAGS_BITS'
would just be a define for (X86_EFLAGS_IOPL | X86_EFLAGS_VM) so
technically the patch would just be nop, no?
--
Vitaly
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
2020-03-06 9:44 ` Vitaly Kuznetsov
@ 2020-03-06 10:00 ` Paolo Bonzini
2020-03-06 10:15 ` Vitaly Kuznetsov
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2020-03-06 10:00 UTC (permalink / raw)
To: Vitaly Kuznetsov, linmiaohe
Cc: rkrcmar, sean.j.christopherson, jmattson, joro, tglx, mingo, bp,
hpa, kvm, linux-kernel, x86
On 06/03/20 10:44, Vitaly Kuznetsov wrote:
>>> Define a macro RMODE_HOST_OWNED_EFLAGS_BITS for (X86_EFLAGS_IOPL |
>>> X86_EFLAGS_VM) as suggested by Vitaly seems a good way to fix this ?
>>> Thanks.
>> No, what if a host-owned flag was zero? I'd just leave it as is.
>>
> I'm not saying my suggestion was a good idea but honestly I'm failing to
> wrap my head around this. The suggested 'RMODE_HOST_OWNED_EFLAGS_BITS'
> would just be a define for (X86_EFLAGS_IOPL | X86_EFLAGS_VM) so
> technically the patch would just be nop, no?
It would not be a nop for the reader.
Something called RMODE_{GUEST,HOST}_OWNED_EFLAGS_BITS is a mask. It
tells you nothing about whether those bugs are 0 or 1. It's just by
chance that all three host-owned EFLAGS bits are 1 while in real mode.
It wouldn't be the case if, for example, we ran the guest using vm86
mode extensions (i.e. setting CR4.VME=1). Then VIF would be host-owned,
but it wouldn't necessarily be 1.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
2020-03-06 10:00 ` Paolo Bonzini
@ 2020-03-06 10:15 ` Vitaly Kuznetsov
0 siblings, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2020-03-06 10:15 UTC (permalink / raw)
To: Paolo Bonzini, linmiaohe
Cc: rkrcmar, sean.j.christopherson, jmattson, joro, tglx, mingo, bp,
hpa, kvm, linux-kernel, x86
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 06/03/20 10:44, Vitaly Kuznetsov wrote:
>>>> Define a macro RMODE_HOST_OWNED_EFLAGS_BITS for (X86_EFLAGS_IOPL |
>>>> X86_EFLAGS_VM) as suggested by Vitaly seems a good way to fix this ?
>>>> Thanks.
>>> No, what if a host-owned flag was zero? I'd just leave it as is.
>>>
>> I'm not saying my suggestion was a good idea but honestly I'm failing to
>> wrap my head around this. The suggested 'RMODE_HOST_OWNED_EFLAGS_BITS'
>> would just be a define for (X86_EFLAGS_IOPL | X86_EFLAGS_VM) so
>> technically the patch would just be nop, no?
>
> It would not be a nop for the reader.
>
> Something called RMODE_{GUEST,HOST}_OWNED_EFLAGS_BITS is a mask. It
> tells you nothing about whether those bugs are 0 or 1. It's just by
> chance that all three host-owned EFLAGS bits are 1 while in real mode.
> It wouldn't be the case if, for example, we ran the guest using vm86
> mode extensions (i.e. setting CR4.VME=1). Then VIF would be host-owned,
> but it wouldn't necessarily be 1.
Got it, it's the name which is causing the confusion, we're using mask
as something different. Make sense, let's keep the code as-is then.
--
Vitaly
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
@ 2020-03-06 2:11 linmiaohe
0 siblings, 0 replies; 9+ messages in thread
From: linmiaohe @ 2020-03-06 2:11 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
jmattson, joro, tglx, mingo, bp, hpa
Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>linmiaohe <linmiaohe@huawei.com> writes:
>
>> From: Miaohe Lin <linmiaohe@huawei.com>
>>
>>
>> vmcs_writel(GUEST_RFLAGS, flags);
>> vmcs_writel(GUEST_CR4, vmcs_readl(GUEST_CR4) | X86_CR4_VME);
>
>Double negations are evil, let's define a macro for 'X86_EFLAGS_IOPL | X86_EFLAGS_VM' instead (completely untested):
You catch the evil guys again. :) But ~RMODE_GUEST_OWNED_EFLAGS_BITS is used by many other func, we should fix them
together. Would try your version, many thanks!
>
>
>- flags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
>+ flags |= RMODE_HOST_OWNED_EFLAGS_BITS;
>
> vmcs_writel(GUEST_RFLAGS, flags);
> vmcs_writel(GUEST_CR4, vmcs_readl(GUEST_CR4) | X86_CR4_VME);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
2020-03-05 2:35 linmiaohe
2020-03-05 10:08 ` Vitaly Kuznetsov
@ 2020-03-05 11:49 ` Paolo Bonzini
1 sibling, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2020-03-05 11:49 UTC (permalink / raw)
To: linmiaohe, rkrcmar, sean.j.christopherson, vkuznets, jmattson,
joro, tglx, mingo, bp, hpa
Cc: kvm, linux-kernel, x86
On 05/03/20 03:35, linmiaohe wrote:
> (X86_EFLAGS_IOPL | X86_EFLAGS_VM) indicates the eflag bits that can not be
> owned by realmode guest, i.e. ~RMODE_GUEST_OWNED_EFLAGS_BITS.
... but ~RMODE_GUEST_OWNED_EFLAGS_BITS is the bits that are owned by the
host; they could be 0 or 1 and that's why the code was using
X86_EFLAGS_IOPL | X86_EFLAGS_VM.
I understand where ~RMODE_GUEST_OWNED_EFLAGS_BITS is better than
X86_EFLAGS_IOPL | X86_EFLAGS_VM, but I cannot think of a way to express
it that is the best of both worlds.
Paolo
Use wrapper
> macro directly to make it clear and also improve readability.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> arch/x86/kvm/vmx/vmx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 743b81642ce2..9571f8dea016 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1466,7 +1466,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
> vmx->rflags = rflags;
> if (vmx->rmode.vm86_active) {
> vmx->rmode.save_rflags = rflags;
> - rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
> + rflags |= ~RMODE_GUEST_OWNED_EFLAGS_BITS;
> }
> vmcs_writel(GUEST_RFLAGS, rflags);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
2020-03-05 2:35 linmiaohe
@ 2020-03-05 10:08 ` Vitaly Kuznetsov
2020-03-05 11:49 ` Paolo Bonzini
1 sibling, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2020-03-05 10:08 UTC (permalink / raw)
To: linmiaohe
Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
jmattson, joro, tglx, mingo, bp, hpa
linmiaohe <linmiaohe@huawei.com> writes:
> From: Miaohe Lin <linmiaohe@huawei.com>
>
> (X86_EFLAGS_IOPL | X86_EFLAGS_VM) indicates the eflag bits that can not be
> owned by realmode guest, i.e. ~RMODE_GUEST_OWNED_EFLAGS_BITS. Use wrapper
> macro directly to make it clear and also improve readability.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> arch/x86/kvm/vmx/vmx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 743b81642ce2..9571f8dea016 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1466,7 +1466,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
> vmx->rflags = rflags;
> if (vmx->rmode.vm86_active) {
> vmx->rmode.save_rflags = rflags;
> - rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
> + rflags |= ~RMODE_GUEST_OWNED_EFLAGS_BITS;
> }
> vmcs_writel(GUEST_RFLAGS, rflags);
>
> @@ -2797,7 +2797,7 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
> flags = vmcs_readl(GUEST_RFLAGS);
> vmx->rmode.save_rflags = flags;
>
> - flags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
> + flags |= ~RMODE_GUEST_OWNED_EFLAGS_BITS;
>
> vmcs_writel(GUEST_RFLAGS, flags);
> vmcs_writel(GUEST_CR4, vmcs_readl(GUEST_CR4) | X86_CR4_VME);
Double negations are evil, let's define a macro for 'X86_EFLAGS_IOPL |
X86_EFLAGS_VM' instead (completely untested):
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4ee19fb35cde..d838f93bd6d2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -139,7 +139,8 @@ module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
#define KVM_PMODE_VM_CR4_ALWAYS_ON (X86_CR4_PAE | X86_CR4_VMXE)
#define KVM_RMODE_VM_CR4_ALWAYS_ON (X86_CR4_VME | X86_CR4_PAE | X86_CR4_VMXE)
-#define RMODE_GUEST_OWNED_EFLAGS_BITS (~(X86_EFLAGS_IOPL | X86_EFLAGS_VM))
+#define RMODE_HOST_OWNED_EFLAGS_BITS (X86_EFLAGS_IOPL | X86_EFLAGS_VM)
+#define RMODE_GUEST_OWNED_EFLAGS_BITS (~RMODE_HOST_OWNED_EFLAGS_BITS)
#define MSR_IA32_RTIT_STATUS_MASK (~(RTIT_STATUS_FILTEREN | \
RTIT_STATUS_CONTEXTEN | RTIT_STATUS_TRIGGEREN | \
@@ -1468,7 +1469,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
vmx->rflags = rflags;
if (vmx->rmode.vm86_active) {
vmx->rmode.save_rflags = rflags;
- rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
+ rflags |= RMODE_HOST_OWNED_EFLAGS_BITS;
}
vmcs_writel(GUEST_RFLAGS, rflags);
@@ -2794,7 +2795,7 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
flags = vmcs_readl(GUEST_RFLAGS);
vmx->rmode.save_rflags = flags;
- flags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
+ flags |= RMODE_HOST_OWNED_EFLAGS_BITS;
vmcs_writel(GUEST_RFLAGS, flags);
vmcs_writel(GUEST_CR4, vmcs_readl(GUEST_CR4) | X86_CR4_VME);
--
Vitaly
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly
@ 2020-03-05 2:35 linmiaohe
2020-03-05 10:08 ` Vitaly Kuznetsov
2020-03-05 11:49 ` Paolo Bonzini
0 siblings, 2 replies; 9+ messages in thread
From: linmiaohe @ 2020-03-05 2:35 UTC (permalink / raw)
To: pbonzini, rkrcmar, sean.j.christopherson, vkuznets, jmattson,
joro, tglx, mingo, bp, hpa
Cc: linmiaohe, kvm, linux-kernel, x86
From: Miaohe Lin <linmiaohe@huawei.com>
(X86_EFLAGS_IOPL | X86_EFLAGS_VM) indicates the eflag bits that can not be
owned by realmode guest, i.e. ~RMODE_GUEST_OWNED_EFLAGS_BITS. Use wrapper
macro directly to make it clear and also improve readability.
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
arch/x86/kvm/vmx/vmx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 743b81642ce2..9571f8dea016 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1466,7 +1466,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
vmx->rflags = rflags;
if (vmx->rmode.vm86_active) {
vmx->rmode.save_rflags = rflags;
- rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
+ rflags |= ~RMODE_GUEST_OWNED_EFLAGS_BITS;
}
vmcs_writel(GUEST_RFLAGS, rflags);
@@ -2797,7 +2797,7 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
flags = vmcs_readl(GUEST_RFLAGS);
vmx->rmode.save_rflags = flags;
- flags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
+ flags |= ~RMODE_GUEST_OWNED_EFLAGS_BITS;
vmcs_writel(GUEST_RFLAGS, flags);
vmcs_writel(GUEST_CR4, vmcs_readl(GUEST_CR4) | X86_CR4_VME);
--
2.19.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-06 10:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-06 2:17 [PATCH] KVM: VMX: Use wrapper macro ~RMODE_GUEST_OWNED_EFLAGS_BITS directly linmiaohe
2020-03-06 5:32 ` Paolo Bonzini
2020-03-06 9:44 ` Vitaly Kuznetsov
2020-03-06 10:00 ` Paolo Bonzini
2020-03-06 10:15 ` Vitaly Kuznetsov
-- strict thread matches above, loose matches on Subject: below --
2020-03-06 2:11 linmiaohe
2020-03-05 2:35 linmiaohe
2020-03-05 10:08 ` Vitaly Kuznetsov
2020-03-05 11:49 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).