Re: [RFC v2 2/4] KVM: x86: add support for ioregionfd signal handling

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4421 bytes --]

On Tue, Feb 09, 2021 at 02:21:22PM +0800, Jason Wang wrote:
> On 2021/1/29 上午2:32, Elena Afanasova wrote:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index ddb28f5ca252..a04516b531da 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5799,19 +5799,33 @@ static int vcpu_mmio_write(struct kvm_vcpu *vcpu, gpa_t addr, int len,
> >   {
> >   	int handled = 0;
> >   	int n;
> > +	int ret = 0;
> > +	bool is_apic;
> >   	do {
> >   		n = min(len, 8);
> > -		if (!(lapic_in_kernel(vcpu) &&
> > -		      !kvm_iodevice_write(vcpu, &vcpu->arch.apic->dev, addr, n, v))
> > -		    && kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, n, v))
> > -			break;
> > +		is_apic = lapic_in_kernel(vcpu) &&
> > +			  !kvm_iodevice_write(vcpu, &vcpu->arch.apic->dev,
> > +					      addr, n, v);
> 
> 
> A better name is needed since "is_apic" only covers the first condition.

The kvm_iodevice_write() call is specific to vcpu->arch.apic->dev (the
in-kernel APIC device). It's not a generic kvm_io_bus_write() call, so
"is_apic" seems correct to me.

> > @@ -6428,16 +6468,27 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
> >   			       unsigned short port, void *val,
> >   			       unsigned int count, bool in)
> >   {
> > +	int ret = 0;
> > +
> >   	vcpu->arch.pio.port = port;
> >   	vcpu->arch.pio.in = in;
> >   	vcpu->arch.pio.count  = count;
> >   	vcpu->arch.pio.size = size;
> > -	if (!kernel_pio(vcpu, vcpu->arch.pio_data)) {
> > +	ret = kernel_pio(vcpu, vcpu->arch.pio_data);
> > +	if (!ret) {
> 
> 
> Unnecessary changes.
[...]
> >   		vcpu->arch.pio.count = 0;
> >   		return 1;
> >   	}
> > +#ifdef CONFIG_KVM_IOREGION
> > +	if (ret == -EINTR) {
> > +		vcpu->run->exit_reason = KVM_EXIT_INTR;
> > +		++vcpu->stat.signal_exits;
> > +		return 0;
> > +	}
> > +#endif

ret is used here. The change above looks necessary to me.

> > diff --git a/include/uapi/linux/ioregion.h b/include/uapi/linux/ioregion.h
> > new file mode 100644
> > index 000000000000..7898c01f84a1
> > --- /dev/null
> > +++ b/include/uapi/linux/ioregion.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +#ifndef _UAPI_LINUX_IOREGION_H
> > +#define _UAPI_LINUX_IOREGION_H
> > +
> > +/* Wire protocol */
> > +struct ioregionfd_cmd {
> > +	__u32 info;
> > +	__u32 padding;
> > +	__u64 user_data;
> > +	__u64 offset;
> > +	__u64 data;
> > +};
> > +
> > +struct ioregionfd_resp {
> > +	__u64 data;
> > +	__u8 pad[24];
> > +};
> > +
> > +#define IOREGIONFD_CMD_READ    0
> > +#define IOREGIONFD_CMD_WRITE   1
> > +
> > +#define IOREGIONFD_SIZE_8BIT   0
> > +#define IOREGIONFD_SIZE_16BIT  1
> > +#define IOREGIONFD_SIZE_32BIT  2
> > +#define IOREGIONFD_SIZE_64BIT  3
> > +
> > +#define IOREGIONFD_SIZE_OFFSET 4
> > +#define IOREGIONFD_RESP_OFFSET 6
> > +#define IOREGIONFD_SIZE(x) ((x) << IOREGIONFD_SIZE_OFFSET)
> > +#define IOREGIONFD_RESP(x) ((x) << IOREGIONFD_RESP_OFFSET)
> 
> 
> Instead of using macros, why not explicitly define them in struct
> ioregionfd_cmd instead of using info?

Good idea, these macros are a little confusing. They produce the info
field value but when reading the code quickly one might think they parse
it instead.

I would go all the way and use a union type:

  struct ioregionfd_cmd {
      __u8 cmd;
      union {
          /* IOREGIONFD_CMD_READ */
          struct {
              __u8 size_exponent : 4;
	      __u8 padding[6];
	      __u64 user_data;
	      __u64 offset;
	  } read;

          /* IOREGIONFD_CMD_WRITE */
          struct {
              __u8 size_exponent : 4;
	      __u8 padding[6];
	      __u64 user_data;
	      __u64 offset;
	      __u64 data;
	  } write;

	  __u8 padding[31];
      };
  };

That way we're not restricted to putting data into a single set of
struct fields for all commands. New commands can easily be added with
totally different fields.

(I didn't check whether the syntax above results in the desired memory
layout, buit you can check with pahole or similar tools.)

(Also, I checked that C bit-fields can be used in Linux uapi headers.
Although their representation is implementation-defined according to the
C standard there is a well-defined representation in the System V ABI
that gcc, clang, and other compilers follow on Linux.)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]