linux-kernel - Re: [PATCH v2 3/5] KVM: VMX: Add error handling to VMREAD helper

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <abcf50fc-0037-446f-36a3-1bd00091ce4f@redhat.com>
Date:   Mon, 29 Jul 2019 11:20:16 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Radim Krčmář <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, Peter Zijlstra <peterz@...radead.org>,
        "H. Peter Anvin" <hpa@...or.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] KVM: VMX: Add error handling to VMREAD helper

On 28/07/19 21:36, Josh Poimboeuf wrote:
> On Fri, Jul 19, 2019 at 01:41:08PM -0700, Sean Christopherson wrote:
>> @@ -68,8 +67,22 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
>>  {
>>  	unsigned long value;
>>  
>> -	asm volatile (__ex_clear("vmread %1, %0", "%k0")
>> -		      : "=r"(value) : "r"(field));
>> +	asm volatile("1: vmread %2, %1\n\t"
>> +		     ".byte 0x3e\n\t" /* branch taken hint */
>> +		     "ja 3f\n\t"
>> +		     "mov %2, %%" _ASM_ARG1 "\n\t"
>> +		     "xor %%" _ASM_ARG2 ", %%" _ASM_ARG2 "\n\t"
>> +		     "2: call vmread_error\n\t"
>> +		     "xor %k1, %k1\n\t"
>> +		     "3:\n\t"
>> +
>> +		     ".pushsection .fixup, \"ax\"\n\t"
>> +		     "4: mov %2, %%" _ASM_ARG1 "\n\t"
>> +		     "mov $1, %%" _ASM_ARG2 "\n\t"
>> +		     "jmp 2b\n\t"
>> +		     ".popsection\n\t"
>> +		     _ASM_EXTABLE(1b, 4b)
>> +		     : ASM_CALL_CONSTRAINT, "=r"(value) : "r"(field) : "cc");
> 
> Was there a reason you didn't do the asm goto thing here like you did
> for the previous patch?  That seemed cleaner, and needs less asm.  

It's because asm goto doesn't support outputs.

Paolo

> I think the branch hints aren't needed -- they're ignored on modern
> processors.  Ditto for the previous patch.
> 
> Also please use named asm operands whereever you can, like "%[field]"
> instead of "%2".  It helps a lot with readability.
>