On 12/09/2024 1:10 pm, Jan Beulich wrote:
> On 12.09.2024 14:06, Andrew Cooper wrote:
>> stdvga_mem_accept() is called on almost all IO emulations, and the
>> overwhelming likely answer is to reject the ioreq. Simply rearranging the
>> expression yields an improvement:
>>
>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
>> Function old new delta
>> stdvga_mem_accept 109 52 -57
>>
>> which is best explained looking at the disassembly:
>>
>> Before: After:
>> f3 0f 1e fa endbr64 f3 0f 1e fa
>> endbr64
>> 0f b6 4e 1e movzbl 0x1e(%rsi),%ecx | 0f b6 46 1e
>> movzbl 0x1e(%rsi),%eax
>> 48 8b 16 mov (%rsi),%rdx | 31 d2
>> xor %edx,%edx
>> f6 c1 40 test $0x40,%cl | a8 30
>> test $0x30,%al
>> 75 38 jne <stdvga_mem_accept+0x48> | 75 23
>> jne <stdvga_mem_accept+0x31>
>> 31 c0 xor %eax,%eax <
>> 48 81 fa ff ff 09 00 cmp $0x9ffff,%rdx <
>> 76 26 jbe <stdvga_mem_accept+0x41> <
>> 8b 46 14 mov 0x14(%rsi),%eax <
>> 8b 7e 10 mov 0x10(%rsi),%edi <
>> 48 0f af c7 imul %rdi,%rax <
>> 48 8d 54 02 ff lea -0x1(%rdx,%rax,1),%rdx <
>> 31 c0 xor %eax,%eax <
>> 48 81 fa ff ff 0b 00 cmp $0xbffff,%rdx <
>> 77 0c ja <stdvga_mem_accept+0x41> <
>> 83 e1 30 and $0x30,%ecx <
>> 75 07 jne <stdvga_mem_accept+0x41> <
>> 83 7e 10 01 cmpl $0x1,0x10(%rsi) 83 7e 10 01
>> cmpl $0x1,0x10(%rsi)
>> 0f 94 c0 sete %al | 75 1d
>> jne <stdvga_mem_accept+0x31>
>> c3 ret | 48 8b 0e
>> mov (%rsi),%rcx
>> 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) | 48 81 f9 ff ff
>> 09 00 cmp $0x9ffff,%rcx
>> 8b 46 10 mov 0x10(%rsi),%eax | 76 11
>> jbe <stdvga_mem_accept+0x31>
>> 8b 7e 14 mov 0x14(%rsi),%edi | 8b 46 14
>> mov 0x14(%rsi),%eax
>> 49 89 d0 mov %rdx,%r8 | 48 8d 44 01 ff
>> lea -0x1(%rcx,%rax,1),%rax
>> 48 83 e8 01 sub $0x1,%rax | 48 3d ff ff 0b
>> 00 cmp $0xbffff,%rax
>> 48 8d 54 3a ff lea -0x1(%rdx,%rdi,1),%rdx | 0f 96 c2
>> setbe %dl
>> 48 0f af c7 imul %rdi,%rax | 89 d0
>> mov %edx,%eax
>> 49 29 c0 sub %rax,%r8 <
>> 31 c0 xor %eax,%eax <
>> 49 81 f8 ff ff 09 00 cmp $0x9ffff,%r8 <
>> 77 be ja <stdvga_mem_accept+0x2a> <
>> c3 ret c3
>> ret
>>
>> By moving the "p->count != 1" check ahead of the
>> ioreq_mmio_{first,last}_byte() calls, both multiplies disappear along with a
>> lot of surrounding logic.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <[email protected]>
> Reviewed-by: Jan Beulich <[email protected]>
Thanks.
>
>> --- a/xen/arch/x86/hvm/stdvga.c
>> +++ b/xen/arch/x86/hvm/stdvga.c
>> @@ -69,18 +69,14 @@ static int cf_check stdvga_mem_write(
>> static bool cf_check stdvga_mem_accept(
>> const struct hvm_io_handler *handler, const ioreq_t *p)
>> {
>> - if ( (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
>> + /*
>> + * Only accept single direct writes, as that's the only thing we can
>> + * accelerate using buffered ioreq handling.
>> + */
>> + if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 ||
>> + (ioreq_mmio_first_byte(p) < VGA_MEM_BASE) ||
>> (ioreq_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
> Arguably the function calls are then pointless (as generated code proves),
> but maybe keeping them for doc purposes is indeed worthwhile.
They're static inlines, but the code is more readable this way IMO.
One thing that did occur to me though. The compiler doesn't know that
p->df is only relevant for REP instructions, and the p->count == 1 case
is ambiguous.
I don't think we can do any better, considering that ioreq is a public type.
~Andrew
