On Wed, Jun 08, 2022, Paolo Bonzini wrote:
> static void complete_emulator_pio_in(struct kvm_vcpu *vcpu, void *val)
> @@ -7482,16 +7486,11 @@ static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
> * shenanigans as KVM doesn't support modifying the rep count,
> * and the emulator ensures @count doesn't overflow the buffer.
> */
> + complete_emulator_pio_in(vcpu, val);
> + return 1;
> } else {
> - int r = __emulator_pio_in(vcpu, size, port, count);
> - if (!r)
> - return r;
> -
> - /* Results already available, fall through. */
> + return __emulator_pio_in(vcpu, size, port, val, count);
Any objections to not using an "else"? I.e.
if (vcpu->arch.pio.count) {
/*
* Complete a previous iteration that required userspace I/O.
* Note, @count isn't guaranteed to match pio.count as userspace
* can modify ECX before rerunning the vCPU. Ignore any such
* shenanigans as KVM doesn't support modifying the rep count,
* and the emulator ensures @count doesn't overflow the buffer.
*/
complete_emulator_pio_in(vcpu, val);
return 1;
}
return __emulator_pio_in(vcpu, size, port, val, count);
> }
> -
> - complete_emulator_pio_in(vcpu, val);
> - return 1;
> }
>
> static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> @@ -7506,14 +7505,8 @@ static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
> unsigned short port, const void *val,
> unsigned int count)
> {
> - int ret;
> -
> trace_kvm_pio(KVM_PIO_OUT, port, size, count, val);
> - ret = emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
> - if (ret)
> - vcpu->arch.pio.count = 0;
> -
> - return ret;
> + return emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
> }
>
> static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
> @@ -13064,20 +13057,20 @@ static int kvm_sev_es_outs(struct kvm_vcpu *vcpu, unsigned int size,
> static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
> unsigned int port);
>
> -static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
> +static void advance_sev_es_emulated_ins(struct kvm_vcpu *vcpu, unsigned count, int size)
> {
> - unsigned count = vcpu->arch.pio.count;
> - complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
> vcpu->arch.sev_pio_count -= count;
> - vcpu->arch.sev_pio_data += count * vcpu->arch.pio.size;
> + vcpu->arch.sev_pio_data += count * size;
> }
>
> static int complete_sev_es_emulated_ins(struct kvm_vcpu *vcpu)
> {
> + unsigned count = vcpu->arch.pio.count;
Opportunistically use an "unsigned int" if you spin another version?
> int size = vcpu->arch.pio.size;
> int port = vcpu->arch.pio.port;
>
> - advance_sev_es_emulated_ins(vcpu);
> + complete_emulator_pio_in(vcpu, vcpu->arch.sev_pio_data);
> + advance_sev_es_emulated_ins(vcpu, count, size);
Eww. The dependency between vcpu->arch.pio.count and complete_emulator_pio_in()
is nasty. Can you add a comment above count to reduce the likelihood of someone
using vcpu->arch.pio.count directly here instead of making a snapshot?
/*
* Snapshot the count before completing userspace I/O, which will
* consume the userspace data and thus clear vcpu->arch.pio.count.
*/
unsigned int count = vcpu->arch.pio.count;
> if (vcpu->arch.sev_pio_count)
> return kvm_sev_es_ins(vcpu, size, port);
> return 1;
> @@ -13089,11 +13082,11 @@ static int kvm_sev_es_ins(struct kvm_vcpu *vcpu, unsigned int size,
> for (;;) {
> unsigned int count =
> min_t(unsigned int, PAGE_SIZE / size, vcpu->arch.sev_pio_count);
> - if (!__emulator_pio_in(vcpu, size, port, count))
> + if (!__emulator_pio_in(vcpu, size, port, vcpu->arch.sev_pio_data, count))
> break;
>
> /* Emulation done by the kernel. */
> - advance_sev_es_emulated_ins(vcpu);
> + advance_sev_es_emulated_ins(vcpu, count, size);
> if (!vcpu->arch.sev_pio_count)
> return 1;
> }
> --
> 2.31.1
>
>
