diff mbox series

[kvmtool,17/21] virtio: Switch trap handling to use MMIO handler

Message ID 20201210142908.169597-18-andre.przywara@arm.com (mailing list archive)
State New, archived
Headers show
Series Unify I/O port and MMIO trap handling | expand

Commit Message

Andre Przywara Dec. 10, 2020, 2:29 p.m. UTC 
With the planned retirement of the special ioport emulation code, we
need to provide an emulation function compatible with the MMIO prototype.

Adjust the existing MMIO callback routine to automatically determine
the region this trap came through, and call the existing I/O handlers.
Register the ioport region using the new registration function.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virtio/pci.c | 42 ++++++++++--------------------------------
 1 file changed, 10 insertions(+), 32 deletions(-)

Comments

Alexandru Elisei Feb. 16, 2021, 5:03 p.m. UTC | #1 
Hi Andre,

Nitpick below, otherwise looks good.

On 12/10/20 2:29 PM, Andre Przywara wrote:
> With the planned retirement of the special ioport emulation code, we
> need to provide an emulation function compatible with the MMIO prototype.
>
> Adjust the existing MMIO callback routine to automatically determine
> the region this trap came through, and call the existing I/O handlers.
> Register the ioport region using the new registration function.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  virtio/pci.c | 42 ++++++++++--------------------------------
>  1 file changed, 10 insertions(+), 32 deletions(-)
>
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 6eea6c68..49d3f4d5 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -178,15 +178,6 @@ static bool virtio_pci__data_in(struct kvm_cpu *vcpu, struct virtio_device *vdev
>  	return ret;
>  }
>  
> -static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
> -{
> -	struct virtio_device *vdev = ioport->priv;
> -	struct virtio_pci *vpci = vdev->virtio;
> -	unsigned long offset = port - virtio_pci__port_addr(vpci);
> -
> -	return virtio_pci__data_in(vcpu, vdev, offset, data, size);
> -}
> -
>  static void update_msix_map(struct virtio_pci *vpci,
>  			    struct msix_table *msix_entry, u32 vecnum)
>  {
> @@ -334,20 +325,6 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde
>  	return ret;
>  }
>  
> -static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
> -{
> -	struct virtio_device *vdev = ioport->priv;
> -	struct virtio_pci *vpci = vdev->virtio;
> -	unsigned long offset = port - virtio_pci__port_addr(vpci);
> -
> -	return virtio_pci__data_out(vcpu, vdev, offset, data, size);
> -}
> -
> -static struct ioport_operations virtio_pci__io_ops = {
> -	.io_in	= virtio_pci__io_in,
> -	.io_out	= virtio_pci__io_out,
> -};
> -
>  static void virtio_pci__msix_mmio_callback(struct kvm_cpu *vcpu,
>  					   u64 addr, u8 *data, u32 len,
>  					   u8 is_write, void *ptr)
> @@ -455,12 +432,15 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
>  {
>  	struct virtio_device *vdev = ptr;
>  	struct virtio_pci *vpci = vdev->virtio;
> -	u32 mmio_addr = virtio_pci__mmio_addr(vpci);
> +	u32 base_addr = virtio_pci__mmio_addr(vpci);
> +
> +	if (addr < base_addr || addr >= base_addr + PCI_IO_SIZE)
> +		base_addr = virtio_pci__port_addr(vpci);

There are only two BARs that use this callback, the ioport BAR (BAR0) and the
memory BAR (BAR1) (MSIX uses a different emulation callback). The condition above
says that if addr is not inside the region described by the memory BAR, then it's
an ioport BAR. How about checking explicitly that it is inside the ioport region
like this (compile tested only), which looks a bit more intuitive for me:

diff --git a/virtio/pci.c b/virtio/pci.c
index 49d3f4d524b2..4024bcd709cd 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -432,10 +432,15 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
 {
        struct virtio_device *vdev = ptr;
        struct virtio_pci *vpci = vdev->virtio;
-       u32 base_addr = virtio_pci__mmio_addr(vpci);
+       u32 mmio_addr = virtio_pci__mmio_addr(vpci);
+       u32 ioport_addr = virtio_pci__port_addr(vpci);
+       u32 base_addr;
 
-       if (addr < base_addr || addr >= base_addr + PCI_IO_SIZE)
-               base_addr = virtio_pci__port_addr(vpci);
+       if (addr >= ioport_addr &&
+           addr < ioport_addr + pci__bar_size(&vpci->pci_hdr, 0))
+               base_addr = ioport_addr;
+       else
+               base_addr = mmio_addr;
 
        if (!is_write)
                virtio_pci__data_in(vcpu, vdev, addr - base_addr, data, len);

Thanks,

Alex

>  
>  	if (!is_write)
> -		virtio_pci__data_in(vcpu, vdev, addr - mmio_addr, data, len);
> +		virtio_pci__data_in(vcpu, vdev, addr - base_addr, data, len);
>  	else
> -		virtio_pci__data_out(vcpu, vdev, addr - mmio_addr, data, len);
> +		virtio_pci__data_out(vcpu, vdev, addr - base_addr, data, len);
>  }
>  
>  static int virtio_pci__bar_activate(struct kvm *kvm,
> @@ -478,10 +458,8 @@ static int virtio_pci__bar_activate(struct kvm *kvm,
>  
>  	switch (bar_num) {
>  	case 0:
> -		r = ioport__register(kvm, bar_addr, &virtio_pci__io_ops,
> -				     bar_size, vdev);
> -		if (r > 0)
> -			r = 0;
> +		r = kvm__register_pio(kvm, bar_addr, bar_size,
> +				      virtio_pci__io_mmio_callback, vdev);
>  		break;
>  	case 1:
>  		r =  kvm__register_mmio(kvm, bar_addr, bar_size, false,
> @@ -510,7 +488,7 @@ static int virtio_pci__bar_deactivate(struct kvm *kvm,
>  
>  	switch (bar_num) {
>  	case 0:
> -		r = ioport__unregister(kvm, bar_addr);
> +		r = kvm__deregister_pio(kvm, bar_addr);
>  		break;
>  	case 1:
>  	case 2:
> @@ -625,7 +603,7 @@ int virtio_pci__exit(struct kvm *kvm, struct virtio_device *vdev)
>  	virtio_pci__reset(kvm, vdev);
>  	kvm__deregister_mmio(kvm, virtio_pci__mmio_addr(vpci));
>  	kvm__deregister_mmio(kvm, virtio_pci__msix_io_addr(vpci));
> -	ioport__unregister(kvm, virtio_pci__port_addr(vpci));
> +	kvm__deregister_pio(kvm, virtio_pci__port_addr(vpci));
>  
>  	return 0;
>  }
Andre Przywara Feb. 18, 2021, 4:13 p.m. UTC | #2 
On Tue, 16 Feb 2021 17:03:04 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> Hi Andre,
> 
> Nitpick below, otherwise looks good.
> 
> On 12/10/20 2:29 PM, Andre Przywara wrote:
> > With the planned retirement of the special ioport emulation code, we
> > need to provide an emulation function compatible with the MMIO prototype.
> >
> > Adjust the existing MMIO callback routine to automatically determine
> > the region this trap came through, and call the existing I/O handlers.
> > Register the ioport region using the new registration function.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  virtio/pci.c | 42 ++++++++++--------------------------------
> >  1 file changed, 10 insertions(+), 32 deletions(-)
> >
> > diff --git a/virtio/pci.c b/virtio/pci.c
> > index 6eea6c68..49d3f4d5 100644
> > --- a/virtio/pci.c
> > +++ b/virtio/pci.c
> > @@ -178,15 +178,6 @@ static bool virtio_pci__data_in(struct kvm_cpu *vcpu, struct virtio_device *vdev
> >  	return ret;
> >  }
> >  
> > -static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
> > -{
> > -	struct virtio_device *vdev = ioport->priv;
> > -	struct virtio_pci *vpci = vdev->virtio;
> > -	unsigned long offset = port - virtio_pci__port_addr(vpci);
> > -
> > -	return virtio_pci__data_in(vcpu, vdev, offset, data, size);
> > -}
> > -
> >  static void update_msix_map(struct virtio_pci *vpci,
> >  			    struct msix_table *msix_entry, u32 vecnum)
> >  {
> > @@ -334,20 +325,6 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde
> >  	return ret;
> >  }
> >  
> > -static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
> > -{
> > -	struct virtio_device *vdev = ioport->priv;
> > -	struct virtio_pci *vpci = vdev->virtio;
> > -	unsigned long offset = port - virtio_pci__port_addr(vpci);
> > -
> > -	return virtio_pci__data_out(vcpu, vdev, offset, data, size);
> > -}
> > -
> > -static struct ioport_operations virtio_pci__io_ops = {
> > -	.io_in	= virtio_pci__io_in,
> > -	.io_out	= virtio_pci__io_out,
> > -};
> > -
> >  static void virtio_pci__msix_mmio_callback(struct kvm_cpu *vcpu,
> >  					   u64 addr, u8 *data, u32 len,
> >  					   u8 is_write, void *ptr)
> > @@ -455,12 +432,15 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
> >  {
> >  	struct virtio_device *vdev = ptr;
> >  	struct virtio_pci *vpci = vdev->virtio;
> > -	u32 mmio_addr = virtio_pci__mmio_addr(vpci);
> > +	u32 base_addr = virtio_pci__mmio_addr(vpci);
> > +
> > +	if (addr < base_addr || addr >= base_addr + PCI_IO_SIZE)
> > +		base_addr = virtio_pci__port_addr(vpci);  
> 
> There are only two BARs that use this callback, the ioport BAR (BAR0) and the
> memory BAR (BAR1) (MSIX uses a different emulation callback). The condition above
> says that if addr is not inside the region described by the memory BAR, then it's
> an ioport BAR. How about checking explicitly that it is inside the ioport region
> like this (compile tested only), which looks a bit more intuitive for me:

Fair enough.

Cheers,
Andre

> 
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 49d3f4d524b2..4024bcd709cd 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -432,10 +432,15 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
>  {
>         struct virtio_device *vdev = ptr;
>         struct virtio_pci *vpci = vdev->virtio;
> -       u32 base_addr = virtio_pci__mmio_addr(vpci);
> +       u32 mmio_addr = virtio_pci__mmio_addr(vpci);
> +       u32 ioport_addr = virtio_pci__port_addr(vpci);
> +       u32 base_addr;
>  
> -       if (addr < base_addr || addr >= base_addr + PCI_IO_SIZE)
> -               base_addr = virtio_pci__port_addr(vpci);
> +       if (addr >= ioport_addr &&
> +           addr < ioport_addr + pci__bar_size(&vpci->pci_hdr, 0))
> +               base_addr = ioport_addr;
> +       else
> +               base_addr = mmio_addr;
>  
>         if (!is_write)
>                 virtio_pci__data_in(vcpu, vdev, addr - base_addr, data, len);
> 
> Thanks,
> 
> Alex
> 
> >  
> >  	if (!is_write)
> > -		virtio_pci__data_in(vcpu, vdev, addr - mmio_addr, data, len);
> > +		virtio_pci__data_in(vcpu, vdev, addr - base_addr, data, len);
> >  	else
> > -		virtio_pci__data_out(vcpu, vdev, addr - mmio_addr, data, len);
> > +		virtio_pci__data_out(vcpu, vdev, addr - base_addr, data, len);
> >  }
> >  
> >  static int virtio_pci__bar_activate(struct kvm *kvm,
> > @@ -478,10 +458,8 @@ static int virtio_pci__bar_activate(struct kvm *kvm,
> >  
> >  	switch (bar_num) {
> >  	case 0:
> > -		r = ioport__register(kvm, bar_addr, &virtio_pci__io_ops,
> > -				     bar_size, vdev);
> > -		if (r > 0)
> > -			r = 0;
> > +		r = kvm__register_pio(kvm, bar_addr, bar_size,
> > +				      virtio_pci__io_mmio_callback, vdev);
> >  		break;
> >  	case 1:
> >  		r =  kvm__register_mmio(kvm, bar_addr, bar_size, false,
> > @@ -510,7 +488,7 @@ static int virtio_pci__bar_deactivate(struct kvm *kvm,
> >  
> >  	switch (bar_num) {
> >  	case 0:
> > -		r = ioport__unregister(kvm, bar_addr);
> > +		r = kvm__deregister_pio(kvm, bar_addr);
> >  		break;
> >  	case 1:
> >  	case 2:
> > @@ -625,7 +603,7 @@ int virtio_pci__exit(struct kvm *kvm, struct virtio_device *vdev)
> >  	virtio_pci__reset(kvm, vdev);
> >  	kvm__deregister_mmio(kvm, virtio_pci__mmio_addr(vpci));
> >  	kvm__deregister_mmio(kvm, virtio_pci__msix_io_addr(vpci));
> > -	ioport__unregister(kvm, virtio_pci__port_addr(vpci));
> > +	kvm__deregister_pio(kvm, virtio_pci__port_addr(vpci));
> >  
> >  	return 0;
> >  }
diff mbox series

Patch

diff --git a/virtio/pci.c b/virtio/pci.c
index 6eea6c68..49d3f4d5 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -178,15 +178,6 @@  static bool virtio_pci__data_in(struct kvm_cpu *vcpu, struct virtio_device *vdev
 	return ret;
 }
 
-static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
-{
-	struct virtio_device *vdev = ioport->priv;
-	struct virtio_pci *vpci = vdev->virtio;
-	unsigned long offset = port - virtio_pci__port_addr(vpci);
-
-	return virtio_pci__data_in(vcpu, vdev, offset, data, size);
-}
-
 static void update_msix_map(struct virtio_pci *vpci,
 			    struct msix_table *msix_entry, u32 vecnum)
 {
@@ -334,20 +325,6 @@  static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde
 	return ret;
 }
 
-static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
-{
-	struct virtio_device *vdev = ioport->priv;
-	struct virtio_pci *vpci = vdev->virtio;
-	unsigned long offset = port - virtio_pci__port_addr(vpci);
-
-	return virtio_pci__data_out(vcpu, vdev, offset, data, size);
-}
-
-static struct ioport_operations virtio_pci__io_ops = {
-	.io_in	= virtio_pci__io_in,
-	.io_out	= virtio_pci__io_out,
-};
-
 static void virtio_pci__msix_mmio_callback(struct kvm_cpu *vcpu,
 					   u64 addr, u8 *data, u32 len,
 					   u8 is_write, void *ptr)
@@ -455,12 +432,15 @@  static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
 {
 	struct virtio_device *vdev = ptr;
 	struct virtio_pci *vpci = vdev->virtio;
-	u32 mmio_addr = virtio_pci__mmio_addr(vpci);
+	u32 base_addr = virtio_pci__mmio_addr(vpci);
+
+	if (addr < base_addr || addr >= base_addr + PCI_IO_SIZE)
+		base_addr = virtio_pci__port_addr(vpci);
 
 	if (!is_write)
-		virtio_pci__data_in(vcpu, vdev, addr - mmio_addr, data, len);
+		virtio_pci__data_in(vcpu, vdev, addr - base_addr, data, len);
 	else
-		virtio_pci__data_out(vcpu, vdev, addr - mmio_addr, data, len);
+		virtio_pci__data_out(vcpu, vdev, addr - base_addr, data, len);
 }
 
 static int virtio_pci__bar_activate(struct kvm *kvm,
@@ -478,10 +458,8 @@  static int virtio_pci__bar_activate(struct kvm *kvm,
 
 	switch (bar_num) {
 	case 0:
-		r = ioport__register(kvm, bar_addr, &virtio_pci__io_ops,
-				     bar_size, vdev);
-		if (r > 0)
-			r = 0;
+		r = kvm__register_pio(kvm, bar_addr, bar_size,
+				      virtio_pci__io_mmio_callback, vdev);
 		break;
 	case 1:
 		r =  kvm__register_mmio(kvm, bar_addr, bar_size, false,
@@ -510,7 +488,7 @@  static int virtio_pci__bar_deactivate(struct kvm *kvm,
 
 	switch (bar_num) {
 	case 0:
-		r = ioport__unregister(kvm, bar_addr);
+		r = kvm__deregister_pio(kvm, bar_addr);
 		break;
 	case 1:
 	case 2:
@@ -625,7 +603,7 @@  int virtio_pci__exit(struct kvm *kvm, struct virtio_device *vdev)
 	virtio_pci__reset(kvm, vdev);
 	kvm__deregister_mmio(kvm, virtio_pci__mmio_addr(vpci));
 	kvm__deregister_mmio(kvm, virtio_pci__msix_io_addr(vpci));
-	ioport__unregister(kvm, virtio_pci__port_addr(vpci));
+	kvm__deregister_pio(kvm, virtio_pci__port_addr(vpci));
 
 	return 0;
 }