[v2,21/24] KVM: x86: drop picdev_in_range()
diff mbox

Message ID 20170323141357.12144-22-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand March 23, 2017, 2:13 p.m. UTC
We already have the exact same checks a couple of lines below.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/kvm/i8259.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

Comments

Paolo Bonzini March 29, 2017, 8:58 a.m. UTC | #1
On 29/03/2017 02:28, Steve Rutherford wrote:
> Not sure this matters, but this changes behavior slightly. This reorders
> validation of the length and and validation of the operation.

This would change behavior if for example:

- KVM registered port 0x22

- the guest wrote 4 bytes to port 0x1f

Before, picdev_write would return -EOPNOTSUPP and KVM would invoke the
handler for port 0x22; now it would return 0.

Since the PIT ports are far from anything else registered by the kernel,
this should not matter.

Paolo

Patch
diff mbox

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index a871e08..dda4abd 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -451,46 +451,33 @@  static u32 elcr_ioport_read(void *opaque, u32 addr1)
 	return s->elcr;
 }
 
-static int picdev_in_range(gpa_t addr)
-{
-	switch (addr) {
-	case 0x20:
-	case 0x21:
-	case 0xa0:
-	case 0xa1:
-	case 0x4d0:
-	case 0x4d1:
-		return 1;
-	default:
-		return 0;
-	}
-}
-
 static int picdev_write(struct kvm_pic *s,
 			 gpa_t addr, int len, const void *val)
 {
 	unsigned char data = *(unsigned char *)val;
-	if (!picdev_in_range(addr))
-		return -EOPNOTSUPP;
 
 	if (len != 1) {
 		pr_pic_unimpl("non byte write\n");
 		return 0;
 	}
-	pic_lock(s);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
 	case 0xa0:
 	case 0xa1:
+		pic_lock(s);
 		pic_ioport_write(&s->pics[addr >> 7], addr, data);
+		pic_unlock(s);
 		break;
 	case 0x4d0:
 	case 0x4d1:
+		pic_lock(s);
 		elcr_ioport_write(&s->pics[addr & 1], addr, data);
+		pic_unlock(s);
 		break;
+	default:
+		return -EOPNOTSUPP;
 	}
-	pic_unlock(s);
 	return 0;
 }
 
@@ -498,29 +485,31 @@  static int picdev_read(struct kvm_pic *s,
 		       gpa_t addr, int len, void *val)
 {
 	unsigned char data = 0;
-	if (!picdev_in_range(addr))
-		return -EOPNOTSUPP;
 
 	if (len != 1) {
 		memset(val, 0, len);
 		pr_pic_unimpl("non byte read\n");
 		return 0;
 	}
-	pic_lock(s);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
 	case 0xa0:
 	case 0xa1:
+		pic_lock(s);
 		data = pic_ioport_read(&s->pics[addr >> 7], addr);
+		pic_unlock(s);
 		break;
 	case 0x4d0:
 	case 0x4d1:
+		pic_lock(s);
 		data = elcr_ioport_read(&s->pics[addr & 1], addr);
+		pic_unlock(s);
 		break;
+	default:
+		return -EOPNOTSUPP;
 	}
 	*(unsigned char *)val = data;
-	pic_unlock(s);
 	return 0;
 }