KVM: IOAPIC: only access APIC registers one dword at a time

The IOAPIC spec says:

When accessing these registers, accesses must be done one dword at a time. For example, software should never access byte 2 from the Data register before accessing bytes 0 and 1. The hardware will not attempt to recover from a bad programming model in this case.

So, this patch removes other width access

Signed-off-by Xiao Guangrong <[email protected]>

---
 virt/kvm/ioapic.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

virt/kvm/ioapic.c +0 -0

index 1149c60..f96816f 100644

virt/kvm/ioapic.c static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len, · line 315

Quoted 9 lines

virt/kvm/ioapic.c +0 -6

 	switch (len) {
-	case 8:
-		*(u64 *) val = result;
-		break;
-	case 1:
-	case 2:
 	case 4:
-		memcpy(val, (char *)&result, len);

Here the parameter len is not used for reading data from ioapic register, before switch case the value of ioapic register has been read by ioapic_read_indirect().

virt/kvm/ioapic.c static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len, · line 314

Quoted 6 lines

virt/kvm/ioapic.c +2 -0

+		*(u32 *) val = result;
 		break;
+
 	default:
 		printk(KERN_WARNING "ioapic: wrong length %d\n", len);

And I think here is not good to print warning message. Maybe it is better to do such kind of checking at the first of this function before ioapic_read_indirect().

The IOAPIC spec says:

When accessing these registers, accesses must be done one dword at a time. For example, software should never access byte 2 from the Data register before accessing bytes 0 and 1. The hardware will not attempt to recover from a bad programming model in this case.

So, this patch removes other width access

Signed-off-by Xiao Guangrong <[email protected]>

---
 virt/kvm/ioapic.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)

virt/kvm/ioapic.c +0 -0

index 1149c60..6610d11 100644

Review

Quoted 5 lines

(quoted diff) +5 -0

+	if (len != 4) {
+		printk(KERN_WARNING "ioapic: wrong length %d\n", len);
+		return 0;
+	}
+

Just remove the printks please, as guests can flood hosts dmesg.

The IOAPIC spec says:

When accessing these registers, accesses must be done one dword at a time. For example, software should never access byte 2 from the Data register before accessing bytes 0 and 1. The hardware will not attempt to recover from a bad programming model in this case.

So, this patch removes other width access

Quoted 10 lines · Xiao Guangrong

The IOAPIC spec says:

When accessing these registers, accesses must be done one dword at a time.
For example, software should never access byte 2 from the Data register before
accessing bytes 0 and 1. The hardware will not attempt to recover from a bad
programming model in this case.

Quoted 3 lines

So, this patch removes other width access

The ioapic code also implements the ia64 iosapic. I'm guessing that does support 64-bit accesses. Please check the iosapic documentation.

There might be guests that use incorrect access despite the documentation; if real hardware supports it, it should work. So we need to start with just a warning, and allow the access. Later we can drop the invalid access.

Quoted 6 lines

virt/kvm/ioapic.c +0 -0

@@ -288,6 +288,11 @@ static int ioapic_mmio_read(struct kvm_io_device
 *this, gpa_t addr, int len,
      ioapic_debug("addr %lx\n", (unsigned long)addr);
      ASSERT(!(addr&  0xf));    /* check alignment */
 

virt/kvm/ioapic.c static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,

Quoted 6 lines

virt/kvm/ioapic.c +5 -0

+    if (len != 4) {
+        printk(KERN_WARNING "ioapic: wrong length %d\n", len);
+        return 0;
+    }
+
 

Guest triggered, so needs to be rate limited.

Quoted 4 lines

There might be guests that use incorrect access despite the documentation; if real hardware supports it, it should work. So we need to start with just a warning, and allow the access. Later we can drop the invalid access.

If the OS contravene the spec, i thinks it's the OS's bug, also, i have tested some versions windows/linux guests, it's no broken, can we directly drop the other wide access?

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Xiao Guangrong

The IOAPIC spec says:

When accessing these registers, accesses must be done one dword at a time.
For example, software should never access byte 2 from the Data register before
accessing bytes 0 and 1. The hardware will not attempt to recover from a bad
programming model in this case.

Quoted 8 lines

Quoted 4 lines

So, this patch removes other width access

The ioapic code also implements the ia64 iosapic. I'm guessing that does support 64-bit accesses. Please check the iosapic documentation.

The iosapic also using 32-bit to access registers:

All registers are accessed using 32-bit uncacheable loads and stores to a reserved memory location in system memory. This implies that to modify a field (e.g., a bit or a byte) in any register, the whole 32-bit register must be read, the field modified, and the 32 bits written back. Partial register access, or non-aligned register access, are implementation-defined by the I/O xAPIC and will not be compatible across different implementations. Also, registers that are described as 64 bits wide are accessed as multiple independent 32-bit registers.

[ From<< Intel® Itanium® Processor Family Interrupt Architecture Guide>>, P2-6 ]

Quoted 5 lines

There might be guests that use incorrect access despite the documentation; if real hardware supports it, it should work. So we need to start with just a warning, and allow the access. Later we can drop the invalid access.

If the OS contravene the spec, i thinks it's the OS's bug, also, i have tested some versions windows/linux guests, it's no broken, can we directly drop the other wide access?

Quoted 6 lines

Quoted 6 lines

There might be guests that use incorrect access despite the documentation; if real hardware supports it, it should work. So we need to start with just a warning, and allow the access. Later we can drop the invalid access.

Quoted 5 lines

If the OS contravene the spec, i thinks it's the OS's bug, also, i have tested some versions
windows/linux guests, it's no broken, can we directly drop the other wide access?

Well, there's the spec and there's real life, but in this case we can try and if we see a problem we'll re-add the other access length.

Quoted 5 lines

Well, there's the spec and there's real life, but in this case we can try and if we see a problem we'll re-add the other access length.

OK, i'll do it as your suggestion, thanks.