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

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

[Andrew Cooper]

[-- Attachment #1: rombios-keyboard.patch --]
[-- Type: text/x-patch, Size: 693 bytes --]

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 <andrew.cooper3@citrix.com>

diff -r e6266fc76d08 -r 2c92985dc53f tools/firmware/rombios/rombios.c
--- a/tools/firmware/rombios/rombios.c
+++ b/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;
             }
         }
 

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 0 of 5] HVM performance improvements

[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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c
--- a/tools/firmware/rombios/rombios.c
+++ b/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;
             }
         }

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

[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 <andrew.cooper3@citrix.com>

diff -r 1728fb789940 -r c223c044afbf tools/firmware/rombios/rombios.c
--- a/tools/firmware/rombios/rombios.c
+++ b/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);

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

[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 <andrew.cooper3@citrix.com>

diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c
--- a/tools/firmware/rombios/rombios.c
+++ b/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++;

[PATCH 4 of 5] rombios/ata Remove more needless traps from the int 0x13 path

[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 <andrew.cooper3@citrix.com>

diff -r a8645d836a0e -r 8dbcbde13914 tools/firmware/rombios/rombios.c
--- a/tools/firmware/rombios/rombios.c
+++ b/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

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

[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 <andrew.cooper3@citrix.com>

diff -r 8dbcbde13914 -r f997ed9b64ab tools/firmware/rombios/rombios.h
--- a/tools/firmware/rombios/rombios.h
+++ b/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

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

[Ian Campbell]

On Mon, 2012-11-26 at 16:41 +0000, Andrew Cooper wrote:
> 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.

>  so
> don't needlessly take 8192 traps to Qemu when 1 will do.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c
> --- a/tools/firmware/rombios/rombios.c
> +++ b/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;
>              }
>          }
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Ian Campbell]

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

Does it have any "irrelevant" side effects?

On Mon, 2012-11-26 at 16:41 +0000, Andrew Cooper wrote:
> 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 <andrew.cooper3@citrix.com>
> 
> diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c
> --- a/tools/firmware/rombios/rombios.c
> +++ b/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++;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Andrew Cooper]

On 26/11/12 16:48, Ian Campbell wrote:
> On Mon, 2012-11-26 at 16:41 +0000, Andrew Cooper wrote:
>> 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.

>
>>  so
>> don't needlessly take 8192 traps to Qemu when 1 will do.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c
>> --- a/tools/firmware/rombios/rombios.c
>> +++ b/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;
>>              }
>>          }
>>  
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

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

[Andrew Cooper]

On 26/11/12 16:51, Ian Campbell wrote:
> 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

>
> On Mon, 2012-11-26 at 16:41 +0000, Andrew Cooper wrote:
>> 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 <andrew.cooper3@citrix.com>
>>
>> diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c
>> --- a/tools/firmware/rombios/rombios.c
>> +++ b/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++;
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

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

[Ian Campbell]

On Mon, 2012-11-26 at 16:56 +0000, Andrew Cooper wrote:
> On 26/11/12 16:51, Ian Campbell wrote:
> > 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.

>   The codepath has no side effects
> we could determine.
> 
> ~Andrew
> 
> >
> > On Mon, 2012-11-26 at 16:41 +0000, Andrew Cooper wrote:
> >> 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 <andrew.cooper3@citrix.com>
> >>
> >> diff -r c223c044afbf -r a8645d836a0e tools/firmware/rombios/rombios.c
> >> --- a/tools/firmware/rombios/rombios.c
> >> +++ b/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++;
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> http://lists.xen.org/xen-devel
> >
> 

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

[Tim Deegan]

At 16:41 +0000 on 26 Nov (1353948092), Andrew Cooper wrote:
> 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.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c
> --- a/tools/firmware/rombios/rombios.c
> +++ b/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;
>              }
>          }
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Tim Deegan]

At 16:41 +0000 on 26 Nov (1353948093), Andrew Cooper wrote:
> 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.

> 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 <andrew.cooper3@citrix.com>
> 
> diff -r 1728fb789940 -r c223c044afbf tools/firmware/rombios/rombios.c
> --- a/tools/firmware/rombios/rombios.c
> +++ b/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);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Mats Petersson]

On 27/11/12 16:46, Tim Deegan wrote:
> At 16:41 +0000 on 26 Nov (1353948093), Andrew Cooper wrote:
>> 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
>
> Tim.
>
>> 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 <andrew.cooper3@citrix.com>
>>
>> diff -r 1728fb789940 -r c223c044afbf tools/firmware/rombios/rombios.c
>> --- a/tools/firmware/rombios/rombios.c
>> +++ b/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);
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

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

[Tim Deegan]

At 16:41 +0000 on 26 Nov (1353948096), Andrew Cooper wrote:
> 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.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 8dbcbde13914 -r f997ed9b64ab tools/firmware/rombios/rombios.h
> --- a/tools/firmware/rombios/rombios.h
> +++ b/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
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Alan Cox]

> 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

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

[Andrew Cooper]

On 27/11/12 16:33, Tim Deegan wrote:
> At 16:41 +0000 on 26 Nov (1353948092), Andrew Cooper wrote:
>> 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

>
> 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.
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 0049de3827bc -r 1728fb789940 tools/firmware/rombios/rombios.c
>> --- a/tools/firmware/rombios/rombios.c
>> +++ b/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;
>>              }
>>          }
>>  
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

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

[Andrew Cooper]

On 27/11/12 17:08, Alan Cox wrote:
>> 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

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.

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

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

[Alan Cox]

O> 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

So why not fix the qemu emulation bug ?

> 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)

> 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

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

[Andrew Cooper]

On 27/11/12 17:49, Alan Cox wrote:
> O> 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
> 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

>
>> 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)
>
>> 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

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 ?

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH 0 of 5] HVM performance improvements

[Pasi Kärkkäinen]

On Mon, Nov 26, 2012 at 04:41:31PM +0000, Andrew Cooper wrote:
> 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


> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 

Re: [PATCH 0 of 5] HVM performance improvements

[Pasi Kärkkäinen]

On Thu, May 09, 2013 at 05:07:36PM +0300, Pasi Kärkkäinen wrote:
> On Mon, Nov 26, 2012 at 04:41:31PM +0000, Andrew Cooper wrote:
> > 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? 
> 

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

Thanks,

-- Pasi

Re: [PATCH 0 of 5] HVM performance improvements

[Andrew Cooper]

On 05/08/13 17:31, Pasi Kärkkäinen wrote:
> On Thu, May 09, 2013 at 05:07:36PM +0300, Pasi Kärkkäinen wrote:
>> On Mon, Nov 26, 2012 at 04:41:31PM +0000, Andrew Cooper wrote:
>>> 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? 
>>
> 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

Re: [PATCH 0 of 5] HVM performance improvements

[George Dunlap]

On Mon, Aug 5, 2013 at 6:10 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 05/08/13 17:31, Pasi Kärkkäinen wrote:
>> On Thu, May 09, 2013 at 05:07:36PM +0300, Pasi Kärkkäinen wrote:
>>> On Mon, Nov 26, 2012 at 04:41:31PM +0000, Andrew Cooper wrote:
>>>> 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?
>>>
>> 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.

Any chance you could re-post them?

 -George

Re: [PATCH 0 of 5] HVM performance improvements

[Andrew Cooper]

On 06/08/13 10:57, George Dunlap wrote:
> On Mon, Aug 5, 2013 at 6:10 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 05/08/13 17:31, Pasi Kärkkäinen wrote:
>>> On Thu, May 09, 2013 at 05:07:36PM +0300, Pasi Kärkkäinen wrote:
>>>> On Mon, Nov 26, 2012 at 04:41:31PM +0000, Andrew Cooper wrote:
>>>>> 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?
>>>>
>>> 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.
> Any chance you could re-post them?
>
>  -George

Sure - coming up

~Andrew