| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b911d367-d000-1237-62fe-230c52e2842c@linux.intel.com>
Date: Wed, 28 Jun 2023 13:13:09 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Zeng Guang <guang.zeng@...el.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
H Peter Anvin <hpa@...or.com>, kvm@...r.kernel.org,
x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/6] KVM: x86: Consolidate flags for __linearize()
On 6/28/2023 1:40 AM, Sean Christopherson wrote:
> On Thu, Jun 01, 2023, Zeng Guang wrote:
>> From: Binbin Wu <binbin.wu@...ux.intel.com>
>>
>> Define a 32-bit parameter and consolidate the two bools into it.
> Please write changelogs so that they make sense without the context of the shortlog.
> In isolation, the above provides zero context. And there's no need to provide a
> play-by-play description of the change, e.g. this can be:
>
> Consolidate __linearize()'s @write and @fetch into a set of flags so that
> additional flags can be added without needing more/new boolean parameters,
> e.g. to precisely identify the access type for LASS.
>
>> __linearize() has two bool parameters write and fetch. And new flag
>> will be needed to support new feature (e.g. LAM needs a flag to skip
> s/LAM/LASS
>
>> address untag under some conditions).
> Looks like this was copy+pasted LAM. AIUI, there is no untagging in LASS. Please,
> please take the time to proofread what you're posting. To you it's a minor typo,
> to others, incorrect statements like this can create a lot of confusion.
>
>> No functional change intended.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@...ux.intel.com>
>> Signed-off-by: Zeng Guang <guang.zeng@...el.com>
>> ---
>> arch/x86/kvm/emulate.c | 19 +++++++++++++------
>> arch/x86/kvm/kvm_emulate.h | 4 ++++
>> 2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 936a397a08cd..9508836e8a35 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -687,8 +687,8 @@ static unsigned insn_alignment(struct x86_emulate_ctxt *ctxt, unsigned size)
>> static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
>> struct segmented_address addr,
>> unsigned *max_size, unsigned size,
>> - bool write, bool fetch,
>> - enum x86emul_mode mode, ulong *linear)
>> + u32 flags, enum x86emul_mode mode,
> "unsigned int", not "u32". They're obviously the same effective thing, but using
> "unsigned int" captures that the number of bits doesn't truly matter, e.g. isn't
> reflected in hardware or anything. This could just as easily be a u16, but there's
> obviously no reason to squeeze this into a u16.
OK. Thanks.
Except for the "u32" / "usigned int" thing, I have updated the patch in
KVM LAM enabling patch series v9:
https://lore.kernel.org/kvm/20230606091842.13123-2-binbin.wu@linux.intel.com/
It has dropped the unnecessary local variables you mentioned below and
improved the changelog a bit to be more informative.
Do you have any comment on this version?
>
>> + ulong *linear)
>> {
>> struct desc_struct desc;
>> bool usable;
>> @@ -696,6 +696,8 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
>> u32 lim;
>> u16 sel;
>> u8 va_bits;
>> + bool fetch = !!(flags & X86EMUL_F_FETCH);
>> + bool write = !!(flags & X86EMUL_F_WRITE);
>>
>> la = seg_base(ctxt, addr.seg) + addr.ea;
>> *max_size = 0;
>> @@ -757,7 +759,11 @@ static int linearize(struct x86_emulate_ctxt *ctxt,
>> ulong *linear)
>> {
>> unsigned max_size;
>> - return __linearize(ctxt, addr, &max_size, size, write, false,
>> + u32 flags = 0;
>> +
>> + if (write)
>> + flags |= X86EMUL_F_WRITE;
>> + return __linearize(ctxt, addr, &max_size, size, flags,
>> ctxt->mode, linear);
> I'm tempted to have this be:
>
> return __linearize(ctxt, addr, &max_size, size,
> write ? X86EMUL_F_WRITE : 0, ctxt->mode, linear);
>
> Mostly so that it's obvious "flags" is constant. The alterntive would e
>
> const unsigned int flags = write ? X86EMUL_F_WRITE : 0;
>
> But my preference is probably to omit "flags" entirely.
>
>> }
>>
>> @@ -768,10 +774,11 @@ static inline int assign_eip(struct x86_emulate_ctxt *ctxt, ulong dst)
>> unsigned max_size;
>> struct segmented_address addr = { .seg = VCPU_SREG_CS,
>> .ea = dst };
>> + u32 flags = X86EMUL_F_FETCH;
>>
>> if (ctxt->op_bytes != sizeof(unsigned long))
>> addr.ea = dst & ((1UL << (ctxt->op_bytes << 3)) - 1);
>> - rc = __linearize(ctxt, addr, &max_size, 1, false, true, ctxt->mode, &linear);
>> + rc = __linearize(ctxt, addr, &max_size, 1, flags, ctxt->mode, &linear);
> Meh, just pass X86EMUL_F_FETCH directly, i.e. drop the local "flags".
>
>> if (rc == X86EMUL_CONTINUE)
>> ctxt->_eip = addr.ea;
>> return rc;
>> @@ -896,6 +903,7 @@ static int __do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size)
>> int cur_size = ctxt->fetch.end - ctxt->fetch.data;
>> struct segmented_address addr = { .seg = VCPU_SREG_CS,
>> .ea = ctxt->eip + cur_size };
>> + u32 flags = X86EMUL_F_FETCH;
>>
>> /*
>> * We do not know exactly how many bytes will be needed, and
>> @@ -907,8 +915,7 @@ static int __do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size)
>> * boundary check itself. Instead, we use max_size to check
>> * against op_size.
>> */
>> - rc = __linearize(ctxt, addr, &max_size, 0, false, true, ctxt->mode,
>> - &linear);
>> + rc = __linearize(ctxt, addr, &max_size, 0, flags, ctxt->mode, &linear);
> And here.
>
>> if (unlikely(rc != X86EMUL_CONTINUE))
>> return rc;
>>
>> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
>> index ab65f3a47dfd..5b9ec610b2cb 100644
>> --- a/arch/x86/kvm/kvm_emulate.h
>> +++ b/arch/x86/kvm/kvm_emulate.h
>> @@ -88,6 +88,10 @@ struct x86_instruction_info {
>> #define X86EMUL_IO_NEEDED 5 /* IO is needed to complete emulation */
>> #define X86EMUL_INTERCEPTED 6 /* Intercepted by nested VMCB/VMCS */
>>
>> +/* x86-specific emulation flags */
>> +#define X86EMUL_F_FETCH BIT(0)
>> +#define X86EMUL_F_WRITE BIT(1)
>> +
>> struct x86_emulate_ops {
>> void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
>> /*
>> --
>> 2.27.0
>>
Powered by blists - more mailing lists