thr3ads.net - Xen devel - [PATCH 0 of 5] HVM performance improvements [Nov 2012]

If this information is useful, please help other people find it:
Share via:

Andrew Cooper

2012-Nov-26 16:41 UTC

[PATCH 0 of 5] 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 <andrew.cooper3@citrix.com>

Andrew Cooper

2012-Nov-26 16:41 UTC

head link

[PATCH 1 of 5] 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 <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;
             }
         }

Andrew Cooper

2012-Nov-26 16:41 UTC

head link

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

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

Andrew Cooper

2012-Nov-26 16:41 UTC

head link

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

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++;

Andrew Cooper

2012-Nov-26 16:41 UTC

head link

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

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

Andrew Cooper

2012-Nov-26 16:41 UTC

head link

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

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

Ian Campbell

2012-Nov-26 16:48 UTC

head link

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

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

Ian Campbell

2012-Nov-26 16:51 UTC

head link

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?

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

2012-Nov-26 16:53 UTC

head link

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

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

Andrew Cooper

2012-Nov-26 16:56 UTC

head link

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

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

Ian Campbell

2012-Nov-26 17:01 UTC

head link

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

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

Tim Deegan

2012-Nov-27 16:33 UTC

head link

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

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

Tim Deegan

2012-Nov-27 16:46 UTC

head link

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

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

Mats Petersson

2012-Nov-27 17:02 UTC

head link

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

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

Tim Deegan

2012-Nov-27 17:07 UTC

head link

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

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

Alan Cox

2012-Nov-27 17:08 UTC

head link

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

> 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

Andrew Cooper

2012-Nov-27 17:08 UTC

head link

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

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

Andrew Cooper

2012-Nov-27 17:22 UTC

head link

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

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

Alan Cox

2012-Nov-27 17:49 UTC

head link

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

Andrew Cooper

2012-Nov-27 18:24 UTC

head link

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

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

Pasi Kärkkäinen

2013-May-09 14:07 UTC

head link

Re: [PATCH 0 of 5] HVM performance improvements

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

Pasi Kärkkäinen

2013-Aug-05 16:31 UTC

head link

Re: [PATCH 0 of 5] HVM performance improvements

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

Andrew Cooper

2013-Aug-05 17:10 UTC

head link

Re: [PATCH 0 of 5] HVM performance improvements

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

George Dunlap

2013-Aug-06 09:57 UTC

head link

Re: [PATCH 0 of 5] HVM performance improvements

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

Andrew Cooper

2013-Aug-06 09:59 UTC

head link

Re: [PATCH 0 of 5] HVM performance improvements

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

Xen devel - Nov 2012 - [PATCH 0 of 5] HVM performance improvements

[PATCH 0 of 5] HVM performance improvements

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Re: [PATCH 0 of 5] HVM performance improvements

Re: [PATCH 0 of 5] HVM performance improvements

Re: [PATCH 0 of 5] HVM performance improvements

Re: [PATCH 0 of 5] HVM performance improvements

Re: [PATCH 0 of 5] HVM performance improvements