Skip to contentSkip to conversation

HVM performance improvements

PATCH In review Andrew Cooper 7 participants 25 messages Nov 26, 2012 – Aug 6, 2013

lore
Share & tools
lore

Andrew Cooper

author
#0 lore raw

PATCH 1/1 HVM performance improvements

This patch series contains the results of an investigation by XenServer to try and reduce dom0 load during a VDI bootstorm.

All patches reduce the number of polled IO traps to qemu that a booting HVM domain uses.

The patches are unmodified from the last time I posted them to the list, but are presented now outside of a feature freeze.

Signed-off-by Andrew Cooper <[email protected]>
author
#1 lore raw

PATCH 1/1 rombios/keyboard: Don't needlessly poll the status register

Repeated polling of the status register is not going to change its value, so don't needlessly take 8192 traps to Qemu when 1 will do.

Signed-off-by Andrew Cooper <[email protected]>
tools/firmware/rombios/rombios.c +2 -2
diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c
@@ -1805,12 +1805,12 @@ keyboard_init()
     while ( (inb(0x64) & 0x02) && (--max>0)) outb(0x80, 0x00);
 
     /* flush incoming keys */
-    max=0x2000;
+    max=2;
     while (--max > 0) {
         outb(0x80, 0x00);
         if (inb(0x64) & 0x01) {
             inb(0x60);
-            max = 0x2000;
+            max = 2;
             }
         }
#2 lore raw

Re: [PATCH 1 of 5] rombios/keyboard: Don't needlessly poll the status register

Quoted 1 line · Andrew Cooper

Repeated polling of the status register is not going to change its value,

but the passage of time while doing multiple polls might? If this is not the case then something needs to be said about why not.

Quoted 28 lines

so
don't needlessly take 8192 traps to Qemu when 1 will do.

Signed-off-by Andrew Cooper <[email protected]>

diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +2 -2
@@ -1805,12 +1805,12 @@ keyboard_init()
     while ( (inb(0x64) & 0x02) && (--max>0)) outb(0x80, 0x00);
 
     /* flush incoming keys */
-    max=0x2000;
+    max=2;
     while (--max > 0) {
         outb(0x80, 0x00);
         if (inb(0x64) & 0x01) {
             inb(0x60);
-            max = 0x2000;
+            max = 2;
             }
         }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#3 lore raw

Re: [PATCH 1 of 5] rombios/keyboard: Don't needlessly poll the status register

Quoted 4 lines · Ian Campbell
Quoted 1 line · Andrew Cooper

Repeated polling of the status register is not going to change its value,

but the passage of time while doing multiple polls might? If this is not the case then something needs to be said about why not.

In reality, we failed to ever get a keypress on the console to appear here, even when trying. If I remember correctly (it was a long time ago now), this code runs before the PCI bus is scanned, so is really early on during boot.

Quoted 30 lines
Quoted 29 lines

so
don't needlessly take 8192 traps to Qemu when 1 will do.

Signed-off-by Andrew Cooper <[email protected]>

diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +2 -2
@@ -1805,12 +1805,12 @@ keyboard_init()
     while ( (inb(0x64) & 0x02) && (--max>0)) outb(0x80, 0x00);
 
     /* flush incoming keys */
-    max=0x2000;
+    max=2;
     while (--max > 0) {
         outb(0x80, 0x00);
         if (inb(0x64) & 0x01) {
             inb(0x60);
-            max = 0x2000;
+            max = 2;
             }
         }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
#4 lore raw

Re: [PATCH 1 of 5] rombios/keyboard: Don't needlessly poll the status register

In reply to Andrew Cooper
Quoted 2 lines · Andrew Cooper

Repeated polling of the status register is not going to change its value, so don't needlessly take 8192 traps to Qemu when 1 will do.

AFAICS the purpose of this loop is to handle the case where someone's actually typing at the time. The 0x2000 is intended to make us wait long enough for the next keypress or autorepeat.

Reducing the loop to _2_ outb(0x80)s seems strange -- is there some case in qemu where another character will be presented after 2 delays but not immediately?

If not, can we dispense with 'max' altogether and just have

    while ( (inb(0x64) & 0x01) ) { inb(0x60); outb(0x80, 0x00); }
or even
    while ( (inb(0x64) & 0x01) ) inb(0x60);

?

Tim.

Quoted 25 lines
Signed-off-by Andrew Cooper <[email protected]>

diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +2 -2
@@ -1805,12 +1805,12 @@ keyboard_init()
     while ( (inb(0x64) & 0x02) && (--max>0)) outb(0x80, 0x00);
 
     /* flush incoming keys */
-    max=0x2000;
+    max=2;
     while (--max > 0) {
         outb(0x80, 0x00);
         if (inb(0x64) & 0x01) {
             inb(0x60);
-            max = 0x2000;
+            max = 2;
             }
         }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#5 lore raw

Re: [PATCH 1 of 5] rombios/keyboard: Don't needlessly poll the status register

Quoted 10 lines · Tim Deegan
Quoted 2 lines · Andrew Cooper

Repeated polling of the status register is not going to change its value, so don't needlessly take 8192 traps to Qemu when 1 will do.

AFAICS the purpose of this loop is to handle the case where someone's actually typing at the time. The 0x2000 is intended to make us wait long enough for the next keypress or autorepeat.

Reducing the loop to _2_ outb(0x80)s seems strange -- is there some case in qemu where another character will be presented after 2 delays but not immediately?

The reduction to 2 instead of 1 is because the while loop uses a pre-decrement on max.

The patch was chosen at the time to have minimal change to the code.

~Andrew

Quoted 34 lines

If not, can we dispense with 'max' altogether and just have

    while ( (inb(0x64) & 0x01) ) { inb(0x60); outb(0x80, 0x00); }
or even
    while ( (inb(0x64) & 0x01) ) inb(0x60);

?

Tim.

Quoted 25 lines
Signed-off-by Andrew Cooper <[email protected]>

diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +2 -2
@@ -1805,12 +1805,12 @@ keyboard_init()
     while ( (inb(0x64) & 0x02) && (--max>0)) outb(0x80, 0x00);
 
     /* flush incoming keys */
-    max=0x2000;
+    max=2;
     while (--max > 0) {
         outb(0x80, 0x00);
         if (inb(0x64) & 0x01) {
             inb(0x60);
-            max = 0x2000;
+            max = 2;
             }
         }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
author
#6 lore raw

PATCH 1/1 rombios/ata: Do not wait for BSY to be set

In reply to Andrew Cooper

I can't find any guarantee in the ATA specification that this will happen, and it certainly does not with Qemu. SeaBIOS has replaced it with a call to udelay(5) instead.

As rombios does not have an equivalent udelay(), replace the wait with a write to port 0x80 which is whitelisted by Xen for 'a small delay'.

This causes roughly 42k fewer traps to Qemu, which is very roughly 10% of the number of traps during boot of a Win7 guest.

Signed-off-by Andrew Cooper <[email protected]>
tools/firmware/rombios/rombios.c +2 -2
diff -r 1728fb789940 -r c223c044afbf tools/firmware/rombios/rombios.c
@@ -2914,8 +2914,8 @@ Bit16u device;
 // 8.2.1 (a) -- set SRST in DC
   outb(iobase2+ATA_CB_DC, ATA_CB_DC_HD15 | ATA_CB_DC_NIEN | ATA_CB_DC_SRST);
 
-// 8.2.1 (b) -- wait for BSY
-  await_ide(BSY, iobase1, 20);
+// 8.2.1 (b) -- wait
+  outb(0x80, 0x00);
 
 // 8.2.1 (f) -- clear SRST
   outb(iobase2+ATA_CB_DC, ATA_CB_DC_HD15 | ATA_CB_DC_NIEN);
#7 lore raw

Re: [PATCH 2 of 5] rombios/ata: Do not wait for BSY to be set

Quoted 6 lines · Andrew Cooper

I can't find any guarantee in the ATA specification that this will happen, and it certainly does not with Qemu. SeaBIOS has replaced it with a call to udelay(5) instead.

As rombios does not have an equivalent udelay(), replace the wait with a write to port 0x80 which is whitelisted by Xen for 'a small delay'.

Does this actually do anything useful? I'd guess that on qemu the extra outb has no effect and on real hardware that needed a delay here, one I/O cycle would not be enough.

Tim.

Quoted 24 lines

This causes roughly 42k fewer traps to Qemu, which is very roughly 10% of the number of traps during boot of a Win7 guest.

Signed-off-by Andrew Cooper <[email protected]>

diff -r 1728fb789940 -r c223c044afbf tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +2 -2
@@ -2914,8 +2914,8 @@ Bit16u device;
 // 8.2.1 (a) -- set SRST in DC
   outb(iobase2+ATA_CB_DC, ATA_CB_DC_HD15 | ATA_CB_DC_NIEN | ATA_CB_DC_SRST);
 
-// 8.2.1 (b) -- wait for BSY
-  await_ide(BSY, iobase1, 20);
+// 8.2.1 (b) -- wait
+  outb(0x80, 0x00);
 
 // 8.2.1 (f) -- clear SRST
   outb(iobase2+ATA_CB_DC, ATA_CB_DC_HD15 | ATA_CB_DC_NIEN);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#8 lore raw

Re: [PATCH 2 of 5] rombios/ata: Do not wait for BSY to be set

Quoted 10 lines · Tim Deegan
Quoted 6 lines · Andrew Cooper

I can't find any guarantee in the ATA specification that this will happen, and it certainly does not with Qemu. SeaBIOS has replaced it with a call to udelay(5) instead.

As rombios does not have an equivalent udelay(), replace the wait with a write to port 0x80 which is whitelisted by Xen for 'a small delay'.

Does this actually do anything useful? I'd guess that on qemu the extra outb has no effect and on real hardware that needed a delay here, one I/O cycle would not be enough.

I believe it's quite a significant delay also on real hardware - but real hardware isn't booting 100s of OSes in short succession [typically, at least], as it has to trickle through all the buses and stuff down to the southbridge. Which is quite a large number of cycles even in a modern system.

It is typically used to ensure that "some other bus cycle" goes in between two IO operations that affect each other, where the hardware is run by a significantly slower processor than the CPU - a 16MHz 16-bit processor or some such. It allows the hardware to "react" to the IO instructions, and change state. Yes, it's daft, and we can't fix that.

In QEMU, it's not going to matter, as the "hardware" is idealised and don't have the same timing restrictions of REAL hardware.

--
mats

Quoted 33 lines

Tim.

Quoted 20 lines

This causes roughly 42k fewer traps to Qemu, which is very roughly 10% of the number of traps during boot of a Win7 guest.

Signed-off-by Andrew Cooper <[email protected]>

diff -r 1728fb789940 -r c223c044afbf tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +2 -2
@@ -2914,8 +2914,8 @@ Bit16u device;
  // 8.2.1 (a) -- set SRST in DC
    outb(iobase2+ATA_CB_DC, ATA_CB_DC_HD15 | ATA_CB_DC_NIEN | ATA_CB_DC_SRST);
 
-// 8.2.1 (b) -- wait for BSY
-  await_ide(BSY, iobase1, 20);
+// 8.2.1 (b) -- wait
+  outb(0x80, 0x00);
 
  // 8.2.1 (f) -- clear SRST
    outb(iobase2+ATA_CB_DC, ATA_CB_DC_HD15 | ATA_CB_DC_NIEN);
Quoted 4 lines

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#9 lore raw

Re: [PATCH 2 of 5] rombios/ata: Do not wait for BSY to be set

In reply to Tim Deegan
Quoted 3 lines

Does this actually do anything useful? I'd guess that on qemu the extra outb has no effect and on real hardware that needed a delay here, one I/O cycle would not be enough.

It ought to be plenty - the time before the response is valid from an ATA command is very short but in the real hardware world most definitely present and important.

However you also need to think about the state of the device registers after the second outb. If you don't wait for BSY and for some reason it's delayed in the emulation or on a multiprocessor what occurs.

Might be safer to do

	outb(0x80,0x00)
	await_ide(...)

at which point the await_ide would run once in the usual case.

Alan

author
#10 lore raw

Re: [PATCH 2 of 5] rombios/ata: Do not wait for BSY to be set

Quoted 11 lines · Alan Cox
Quoted 3 lines

Does this actually do anything useful? I'd guess that on qemu the extra outb has no effect and on real hardware that needed a delay here, one I/O cycle would not be enough.

It ought to be plenty - the time before the response is valid from an ATA command is very short but in the real hardware world most definitely present and important.

However you also need to think about the state of the device registers after the second outb. If you don't wait for BSY and for some reason it's delayed in the emulation or on a multiprocessor what occurs.

Quoted 8 lines

Might be safer to do

	outb(0x80,0x00)
	await_ide(...)

at which point the await_ide would run once in the usual case.

Alan

No - this is what we are trying to avoid.

SEABios has explicitly removed the wait for the BSY bit to be set. We want to remove it because Qemu will not set the BSY bit, resulting in 42k needless polled IO traps while we wait for the timeout (which is, if I remember correctly, based on a loop counter rather than any notion of actual time)

We can certainly argue about the outb(0x80,0x00). When I was comparing ROMBios with SEABios, this outb was the closest I could easily get to a udelay(5) without implement udelay() in ROMBios. Xen will execute an outb(0x80, 0x00) on the real hardware if a guest executes it; It will never result in a trap to qemu.

Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
#11 lore raw

Re: [PATCH 2 of 5] rombios/ata: Do not wait for BSY to be set

O> SEABios has explicitly removed the wait for the BSY bit to be set. We

Quoted 1 line

want to remove it because Qemu will not set the BSY bit, resulting in

So why not fix the qemu emulation bug ?

Quoted 3 lines

42k needless polled IO traps while we wait for the timeout (which is, if I remember correctly, based on a loop counter rather than any notion of actual time)

Not good, although half caused by the fact that years on qemu still hasn't implemented predictors to avoid trapping further than the kernel layer 8)

Quoted 4 lines

We can certainly argue about the outb(0x80,0x00). When I was comparing ROMBios with SEABios, this outb was the closest I could easily get to a udelay(5) without implement udelay() in ROMBios. Xen will execute an outb(0x80, 0x00) on the real hardware if a guest executes it;

Don't do that then. 0x80 is not safe on a modern PC system (you'll note the Linux kernel changed behaviour here). It may be a debug port but we've seen some hardware do worse things if you touch it. It's also completely out on a limb with EFI based systems which don't necessarily have the legacy stuff present.

Alan

author
#12 lore raw

Re: [PATCH 2 of 5] rombios/ata: Do not wait for BSY to be set

Quoted 3 lines · Alan Cox

O> SEABios has explicitly removed the wait for the BSY bit to be set. We

Quoted 1 line

want to remove it because Qemu will not set the BSY bit, resulting in

So why not fix the qemu emulation bug ?

I do not have a copy of the ATA specification, and
http://code.coreboot.org/p/seabios/source/commit/580e33293244fee4556e56ecc67b8bd877f3c496/
certainly implies that waiting for BSY is not required

Quoted 12 lines
Quoted 4 lines

42k needless polled IO traps while we wait for the timeout (which is, if I remember correctly, based on a loop counter rather than any notion of actual time)

Not good, although half caused by the fact that years on qemu still hasn't implemented predictors to avoid trapping further than the kernel layer 8)

Quoted 4 lines

We can certainly argue about the outb(0x80,0x00). When I was comparing ROMBios with SEABios, this outb was the closest I could easily get to a udelay(5) without implement udelay() in ROMBios. Xen will execute an outb(0x80, 0x00) on the real hardware if a guest executes it;

Quoted 7 lines

Don't do that then. 0x80 is not safe on a modern PC system (you'll note the Linux kernel changed behaviour here). It may be a debug port but we've seen some hardware do worse things if you touch it. It's also completely out on a limb with EFI based systems which don't necessarily have the legacy stuff present.

Alan

If port 0x80 is problematic, then this is a Xen problem, but a legacy guest should have no adverse effects. Perhaps we should "emulate" port 0x80 by deliberately re-scheduling another VCPU ?

Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
author
#13 lore raw

PATCH 1/1 rombios/ata: Reading this status register has no relevant side effects

In reply to Andrew Cooper

So taking two traps when one will do is pointless. This codepath is on the int 0x13 hot path, and removing it has about a 30% reduction in the number of traps to Qemu during Win7 boot.

Signed-off-by Andrew Cooper <[email protected]>
tools/firmware/rombios/rombios.c +0 -1
diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c
@@ -2540,7 +2540,6 @@ static int await_ide(when_done,base,time
   Bit32u time=0,last=0;
   Bit16u status;
   Bit8u result;
-  status = inb(base + ATA_CB_STAT); // for the times you're supposed to throw one away
   for(;;) {
     status = inb(base+ATA_CB_STAT);
     time++;
#14 lore raw

Re: [PATCH 3 of 5] rombios/ata: Reading this status register has no relevant side effects

No relevant side effects according to some spec or according to the current qemu implementation?

Does it have any "irrelevant" side effects?

Quoted 22 lines · Andrew Cooper

So taking two traps when one will do is pointless. This codepath is on the int 0x13 hot path, and removing it has about a 30% reduction in the number of traps to Qemu during Win7 boot.

Signed-off-by Andrew Cooper <[email protected]>

diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +0 -1
@@ -2540,7 +2540,6 @@ static int await_ide(when_done,base,time
   Bit32u time=0,last=0;
   Bit16u status;
   Bit8u result;
-  status = inb(base + ATA_CB_STAT); // for the times you're supposed to throw one away
   for(;;) {
     status = inb(base+ATA_CB_STAT);
     time++;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#15 lore raw

Re: [PATCH 3 of 5] rombios/ata: Reading this status register has no relevant side effects

Quoted 4 lines · Ian Campbell

No relevant side effects according to some spec or according to the current qemu implementation?

Does it have any "irrelevant" side effects?

I am not sure whether the spec gurarentees no side effects, but this is according to the qemu implementation. The codepath has no side effects we could determine.

~Andrew

Quoted 25 lines · Andrew Cooper
Quoted 22 lines

So taking two traps when one will do is pointless. This codepath is on the int 0x13 hot path, and removing it has about a 30% reduction in the number of traps to Qemu during Win7 boot.

Signed-off-by Andrew Cooper <[email protected]>

diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +0 -1
@@ -2540,7 +2540,6 @@ static int await_ide(when_done,base,time
   Bit32u time=0,last=0;
   Bit16u status;
   Bit8u result;
-  status = inb(base + ATA_CB_STAT); // for the times you're supposed to throw one away
   for(;;) {
     status = inb(base+ATA_CB_STAT);
     time++;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
#16 lore raw

Re: [PATCH 3 of 5] rombios/ata: Reading this status register has no relevant side effects

Quoted 8 lines · Andrew Cooper
Quoted 4 lines · Ian Campbell

No relevant side effects according to some spec or according to the current qemu implementation?

Does it have any "irrelevant" side effects?

I am not sure whether the spec gurarentees no side effects, but this is according to the qemu implementation.

This sort of thing is a critical part of the commit message, for the benefit of some future bug hunter if qemu ever changes this. Although to be honest this possibility makes me rather sceptical of this sort of change in the first place.

Quoted 31 lines

The codepath has no side effects
we could determine.

~Andrew

Quoted 25 lines · Andrew Cooper
Quoted 22 lines

So taking two traps when one will do is pointless. This codepath is on the int 0x13 hot path, and removing it has about a 30% reduction in the number of traps to Qemu during Win7 boot.

Signed-off-by Andrew Cooper <[email protected]>

diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c

tools/firmware/rombios/rombios.c +0 -1
@@ -2540,7 +2540,6 @@ static int await_ide(when_done,base,time
   Bit32u time=0,last=0;
   Bit16u status;
   Bit8u result;
-  status = inb(base + ATA_CB_STAT); // for the times you're supposed to throw one away
   for(;;) {
     status = inb(base+ATA_CB_STAT);
     time++;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

author
#17 lore raw

PATCH 1/1 rombios/ata Remove more needless traps from the int 0x13 path

In reply to Andrew Cooper

The return value from await_ide() is always ignored, and most calls to await_ide() immediately reread the status register.

Therefore, making await_ide() return the last value of the status register removes a further two traps on the int 0x13 path, with a similar reduction in the total number of traps during Win7 boot.

Signed-off-by Andrew Cooper <[email protected]>
tools/firmware/rombios/rombios.c +13 -19
diff -r a8645d836a0e -r 8dbcbde13914 tools/firmware/rombios/rombios.c
@@ -2531,14 +2531,14 @@ void ata_init( )
 
 #define IDE_TIMEOUT 32000u //32 seconds max for IDE ops
 
-int await_ide();
-static int await_ide(when_done,base,timeout)
+Bit8u await_ide();
+static Bit8u await_ide(when_done,base,timeout)
   Bit8u when_done;
   Bit16u base;
   Bit16u timeout;
 {
   Bit32u time=0,last=0;
-  Bit16u status;
+  Bit8u status;
   Bit8u result;
   for(;;) {
     status = inb(base+ATA_CB_STAT);

@@ -2556,7 +2556,7 @@ static int await_ide(when_done,base,time
     else if (when_done == TIMEOUT)
       result = 0;
 
-    if (result) return 0;
+    if (result) return status;
     if (time>>16 != last) // mod 2048 each 16 ms
     {
       last = time >>16;

@@ -2565,12 +2565,12 @@ static int await_ide(when_done,base,time
     if (status & ATA_CB_STAT_ERR)
     {
       BX_DEBUG_ATA("await_ide: ERROR (TIMEOUT,BSY,!BSY,!BSY_DRQ,!BSY_!DRQ,!BSY_RDY) %d time= %ld timeout= %d\n",when_done,time>>11, timeout);
-      return -1;
+      return status;
     }
     if ((timeout == 0) || ((time>>11) > timeout)) break;
   }
   BX_INFO("IDE time out\n");
-  return -1;
+  return status;
 }
 
 // ---------------------------------------------------------------------------

@@ -3016,8 +3016,7 @@ Bit32u lba_low, lba_high;
   outb(iobase1 + ATA_CB_DH, (slave ? ATA_CB_DH_DEV1 : ATA_CB_DH_DEV0) | (Bit8u) head );
   outb(iobase1 + ATA_CB_CMD, command);
 
-  await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
-  status = inb(iobase1 + ATA_CB_STAT);
+  status = await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
 
   if (status & ATA_CB_STAT_ERR) {
     BX_DEBUG_ATA("ata_cmd_data_in : read error\n");

@@ -3077,8 +3076,7 @@ ASM_END
     current++;
     write_word(ebda_seg, &EbdaData->ata.trsfsectors,current);
     count--;
-    await_ide(NOT_BSY, iobase1, IDE_TIMEOUT);
-    status = inb(iobase1 + ATA_CB_STAT);
+    status = await_ide(NOT_BSY, iobase1, IDE_TIMEOUT);
     if (count == 0) {
       if ( (status & (ATA_CB_STAT_BSY | ATA_CB_STAT_RDY | ATA_CB_STAT_DRQ | ATA_CB_STAT_ERR) )
           != ATA_CB_STAT_RDY ) {

@@ -3167,8 +3165,7 @@ Bit32u lba_low, lba_high;
   outb(iobase1 + ATA_CB_DH, (slave ? ATA_CB_DH_DEV1 : ATA_CB_DH_DEV0) | (Bit8u) head );
   outb(iobase1 + ATA_CB_CMD, command);
 
-  await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
-  status = inb(iobase1 + ATA_CB_STAT);
+  status = await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
 
   if (status & ATA_CB_STAT_ERR) {
     BX_DEBUG_ATA("ata_cmd_data_out : read error\n");

@@ -3316,8 +3313,7 @@ Bit32u length;
   outb(iobase1 + ATA_CB_CMD, ATA_CMD_PACKET);
 
   // Device should ok to receive command
-  await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
-  status = inb(iobase1 + ATA_CB_STAT);
+  status = await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
 
   if (status & ATA_CB_STAT_ERR) {
     BX_DEBUG_ATA("ata_cmd_packet : error, status is %02x\n",status);

@@ -3353,8 +3349,7 @@ ASM_START
 ASM_END
 
   if (inout == ATA_DATA_NO) {
-    await_ide(NOT_BSY, iobase1, IDE_TIMEOUT);
-    status = inb(iobase1 + ATA_CB_STAT);
+    status = await_ide(NOT_BSY, iobase1, IDE_TIMEOUT);
     }
   else {
         Bit16u loops = 0;

@@ -3363,13 +3358,12 @@ ASM_END
 
       if (loops == 0) {//first time through
         status = inb(iobase2 + ATA_CB_ASTAT);
-        await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
+        status = await_ide(NOT_BSY_DRQ, iobase1, IDE_TIMEOUT);
       }
       else
-        await_ide(NOT_BSY, iobase1, IDE_TIMEOUT);
+        status = await_ide(NOT_BSY, iobase1, IDE_TIMEOUT);
       loops++;
 
-      status = inb(iobase1 + ATA_CB_STAT);
       sc = inb(iobase1 + ATA_CB_SC);
 
       // Check if command completed
author
#18 lore raw

PATCH 1/1 rombios/debug: Reduce verbosity of rombios

In reply to Andrew Cooper

Default builds of Qemu appear not to log the Bochs debug port, so having rombios write to the port causes pointless traps.

Signed-off-by Andrew Cooper <[email protected]>
tools/firmware/rombios/rombios.h +2 -1
diff -r 8dbcbde13914 -r f997ed9b64ab tools/firmware/rombios/rombios.h
@@ -48,10 +48,11 @@
 // per-device basis. Debug info are sent only in debug mode
 #if DEBUG_ROMBIOS
 #  define BX_DEBUG(format, p...)  bios_printf(BIOS_PRINTF_INFO, format, ##p)
+#  define BX_INFO(format, p...)   bios_printf(BIOS_PRINTF_INFO, format, ##p)
 #else
 #  define BX_DEBUG(format, p...)
+#  define BX_INFO(format, p...)
 #endif
-#define BX_INFO(format, p...)   bios_printf(BIOS_PRINTF_INFO, format, ##p)
 #define BX_PANIC(format, p...)  bios_printf(BIOS_PRINTF_DEBHALT, format, ##p)
 
 #define ACPI_DATA_SIZE    0x00010000L
#19 lore raw

Re: [PATCH 5 of 5] rombios/debug: Reduce verbosity of rombios

Quoted 2 lines · Andrew Cooper

Default builds of Qemu appear not to log the Bochs debug port, so having rombios write to the port causes pointless traps.

This BIOS_PRINTF_INFO output is also sent to the Xen debug port (0xE9) by send() in rombios.c. I think the right change if the Bochs ports aren't useful is to set BX_VIRTUAL_PORTS to 0 in rombios.c.

Tim.

Quoted 23 lines
Signed-off-by Andrew Cooper <[email protected]>

diff -r 8dbcbde13914 -r f997ed9b64ab tools/firmware/rombios/rombios.h

tools/firmware/rombios/rombios.h +2 -1
@@ -48,10 +48,11 @@
 // per-device basis. Debug info are sent only in debug mode
 #if DEBUG_ROMBIOS
 #  define BX_DEBUG(format, p...)  bios_printf(BIOS_PRINTF_INFO, format, ##p)
+#  define BX_INFO(format, p...)   bios_printf(BIOS_PRINTF_INFO, format, ##p)
 #else
 #  define BX_DEBUG(format, p...)
+#  define BX_INFO(format, p...)
 #endif
-#define BX_INFO(format, p...)   bios_printf(BIOS_PRINTF_INFO, format, ##p)
 #define BX_PANIC(format, p...)  bios_printf(BIOS_PRINTF_DEBHALT, format, ##p)
 
 #define ACPI_DATA_SIZE    0x00010000L

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#20 lore raw
In reply to Andrew Cooper
Quoted 9 lines · Andrew Cooper

This patch series contains the results of an investigation by XenServer to try and reduce dom0 load during a VDI bootstorm.

All patches reduce the number of polled IO traps to qemu that a booting HVM domain uses.

The patches are unmodified from the last time I posted them to the list, but are presented now outside of a feature freeze.

It's feature freeze time again :)

Are these patches still necessary? for Xen 4.4?

-- Pasi

Quoted 2 lines
Signed-off-by Andrew Cooper <[email protected]>
#21 lore raw
Quoted 7 lines · Pasi Kärkkäinen
Quoted 6 lines · Andrew Cooper

This patch series contains the results of an investigation by XenServer to try and reduce dom0 load during a VDI bootstorm.

All patches reduce the number of polled IO traps to qemu that a booting HVM domain uses.

Quoted 8 lines
Quoted 3 lines

The patches are unmodified from the last time I posted them to the list, but are presented now outside of a feature freeze.

It's feature freeze time again :)

Are these patches still necessary? for Xen 4.4?

Xen 4.3 is now out of the door, so should these patches be considered for Xen 4.4 ?

Thanks,

-- Pasi

author
#22 lore raw
Quoted 13 lines · Pasi Kärkkäinen
Quoted 12 lines · Pasi Kärkkäinen
Quoted 9 lines · Andrew Cooper

This patch series contains the results of an investigation by XenServer to try and reduce dom0 load during a VDI bootstorm.

All patches reduce the number of polled IO traps to qemu that a booting HVM domain uses.

The patches are unmodified from the last time I posted them to the list, but are presented now outside of a feature freeze.

It's feature freeze time again :)

Quoted 8 lines
Quoted 2 lines

Are these patches still necessary? for Xen 4.4?

Xen 4.3 is now out of the door, so should these patches be considered for Xen 4.4 ?

Thanks,

-- Pasi

I would dearly love for these patches to finally go in. The patches likely apply as-are.

~Andrew

#23 lore raw
Quoted 18 lines · Andrew Cooper
Quoted 17 lines · Pasi Kärkkäinen
Quoted 7 lines · Pasi Kärkkäinen
Quoted 6 lines · Andrew Cooper

This patch series contains the results of an investigation by XenServer to try and reduce dom0 load during a VDI bootstorm.

All patches reduce the number of polled IO traps to qemu that a booting HVM domain uses.

Quoted 7 lines
Quoted 3 lines

The patches are unmodified from the last time I posted them to the list, but are presented now outside of a feature freeze.

It's feature freeze time again :)

Are these patches still necessary? for Xen 4.4?

Xen 4.3 is now out of the door, so should these patches be considered for Xen 4.4 ?

Quoted 7 lines
Quoted 4 lines

Thanks,

-- Pasi

I would dearly love for these patches to finally go in. The patches likely apply as-are.

Any chance you could re-post them?

-George

author
#24 lore raw
Quoted 21 lines · George Dunlap
Quoted 20 lines · Andrew Cooper
Quoted 19 lines · Pasi Kärkkäinen
Quoted 7 lines · Pasi Kärkkäinen
Quoted 6 lines · Andrew Cooper

This patch series contains the results of an investigation by XenServer to try and reduce dom0 load during a VDI bootstorm.

All patches reduce the number of polled IO traps to qemu that a booting HVM domain uses.

Quoted 7 lines
Quoted 3 lines

The patches are unmodified from the last time I posted them to the list, but are presented now outside of a feature freeze.

It's feature freeze time again :)

Are these patches still necessary? for Xen 4.4?

Xen 4.3 is now out of the door, so should these patches be considered for Xen 4.4 ?

Thanks,

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

-- Pasi

I would dearly love for these patches to finally go in. The patches likely apply as-are.

Any chance you could re-post them?

-George

Sure - coming up

~Andrew