[Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Gleb Natapov]

Hello,

  This is one more try to push this patch series into qemu. This time
I don't use vmport interface. Instead I implemented simple communication
channel between  qemu and BIOS using IO port.  I decided not to  use
firmware interface since current users of the interface may depend on
exact structure layout, so it doesn't look like a good idea to add
arbitrary fields there. Also I don't want to copy the whole ROM into BIOS
just to access one field of it. IO channel is implemented by the first
patch of the series. Patches 2-5 implement UUID related stuff. Patch 6
passes host CPU frequency to fill in SM BIOS table.

Please comment.

---

Gleb Natapov (6):
      Pass cpu speed into SM BIOS.
      Add UUID to BIOS configuration info.
      Use libuuid if available.
      Add "info uuid" command to monitor.
      Add -uuid command line option.
      Use IO port for qemu<->guest BIOS communication.


 Makefile.target |    4 ++
 configure       |   13 +++++
 hw/pc.c         |  141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 monitor.c       |   13 +++++
 sysemu.h        |    2 +
 vl.c            |   42 ++++++++++++++++
 6 files changed, 215 insertions(+), 0 deletions(-)

-- 
        Gleb.

[Qemu-devel] [PATCH 1/6] Use IO port for qemu<->guest BIOS communication.

[Gleb Natapov]

Use PIO to get configuration info between qemu process and guest BIOS.

Signed-off-by: Gleb Natapov <gleb@qumranet.com>
---

 hw/pc.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 213ead8..ccbe4b3 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -44,6 +44,7 @@
 
 /* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables.  */
 #define ACPI_DATA_SIZE       0x10000
+#define BIOS_CFG_IOPORT 0x1234
 
 #define MAX_IDE_BUS 2
 
@@ -53,6 +54,24 @@ static PITState *pit;
 static IOAPICState *ioapic;
 static PCIDevice *i440fx_state;
 
+typedef struct _BIOSCfgEntry {
+    uint16_t len;
+    uint8_t *data;
+} BIOSCfgEntry;
+
+typedef enum _BIOSCfgEntryNum{
+    BIOS_CFG_SIGNATURE,
+    BIOS_CFG_MAX_ENTRY
+} BIOSCfgEntryNum;
+
+typedef struct _BIOSCfgState {
+    BIOSCfgEntry entries[BIOS_CFG_MAX_ENTRY];
+    BIOSCfgEntryNum entry;
+    uint16_t cur_offset;
+} BIOSCfgState;
+
+static BIOSCfgState bios_params;
+
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
 }
@@ -716,6 +735,33 @@ static void pc_init_ne2k_isa(NICInfo *nd, qemu_irq *pic)
     nb_ne2k++;
 }
 
+static uint32_t bios_cfg_read(void *opaque, uint32_t addr)
+{
+    BIOSCfgEntry *e = &bios_params.entries[bios_params.entry];
+
+    if (!e->data)
+        return 0;
+
+    return e->data[bios_params.cur_offset++ % e->len];
+}
+
+static void bios_cfg_write(void *opaque, uint32_t addr, uint32_t value)
+{
+    bios_params.entry = value % BIOS_CFG_MAX_ENTRY;
+    bios_params.cur_offset = 0;
+}
+
+static int bios_cfg_add_data(BIOSCfgEntryNum entry, uint8_t *data, uint16_t len)
+{
+    if(entry >=  BIOS_CFG_MAX_ENTRY)
+        return 0;
+
+    bios_params.entries[entry].data = data;
+    bios_params.entries[entry].len = len;
+
+    return 1;
+}
+
 /* PC hardware initialisation */
 static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
                      const char *boot_device, DisplayState *ds,
@@ -871,6 +917,11 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     cpu_register_physical_memory((uint32_t)(-bios_size),
                                  bios_size, bios_offset | IO_MEM_ROM);
 
+    register_ioport_read(BIOS_CFG_IOPORT, 1, 1, bios_cfg_read, NULL);
+    register_ioport_write(BIOS_CFG_IOPORT, 1, 1, bios_cfg_write, NULL);
+
+    bios_cfg_add_data(BIOS_CFG_SIGNATURE, "QEMU", 4);
+
     bochs_bios_init();
 
     if (linux_boot)

[Qemu-devel] [PATCH 2/6] Add -uuid command line option.

[Gleb Natapov]

Let user specify UUID of the virtual machine.

Signed-off-by: Gleb Natapov <gleb@qumranet.com>
---

 sysemu.h |    2 ++
 vl.c     |   29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/sysemu.h b/sysemu.h
index b12fae0..931ac3a 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -8,6 +8,8 @@ extern const char *bios_dir;
 
 extern int vm_running;
 extern const char *qemu_name;
+extern uint8_t qemu_uuid[];
+#define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running);
diff --git a/vl.c b/vl.c
index 7ca8420..e2b4042 100644
--- a/vl.c
+++ b/vl.c
@@ -253,6 +253,8 @@ static int64_t qemu_icount_bias;
 QEMUTimer *icount_rt_timer;
 QEMUTimer *icount_vm_timer;
 
+uint8_t qemu_uuid[16];
+
 #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
 
 /***********************************************************/
@@ -7697,6 +7699,7 @@ static void help(int exitcode)
            "-g WxH[xDEPTH]  Set the initial graphical resolution and depth\n"
 #endif
            "-name string    set the name of the guest\n"
+           "-uuid %%08x-%%04x-%%04x-%%04x-%%012x specify machine UUID\n"
            "\n"
            "Network options:\n"
            "-net nic[,vlan=n][,macaddr=addr][,model=type]\n"
@@ -7891,6 +7894,7 @@ enum {
     QEMU_OPTION_startdate,
     QEMU_OPTION_tb_size,
     QEMU_OPTION_icount,
+    QEMU_OPTION_uuid,
 };
 
 typedef struct QEMUOption {
@@ -7979,6 +7983,7 @@ const QEMUOption qemu_options[] = {
 #ifdef CONFIG_CURSES
     { "curses", 0, QEMU_OPTION_curses },
 #endif
+    { "uuid", HAS_ARG, QEMU_OPTION_uuid },
 
     /* temporary options */
     { "usb", 0, QEMU_OPTION_usb },
@@ -8189,6 +8194,23 @@ static BOOL WINAPI qemu_ctrl_handler(DWORD type)
 }
 #endif
 
+static int qemu_uuid_parse(const char *str, uint8_t *uuid)
+{
+    int ret;
+
+    if(strlen(str) != 36)
+        return -1;
+
+    ret = sscanf(str, UUID_FMT, &uuid[0], &uuid[1], &uuid[2], &uuid[3],
+            &uuid[4], &uuid[5], &uuid[6], &uuid[7], &uuid[8], &uuid[9],
+            &uuid[10], &uuid[11], &uuid[12], &uuid[13], &uuid[14], &uuid[15]);
+
+    if(ret != 16)
+        return -1;
+
+    return 0;
+}
+
 #define MAX_NET_CLIENTS 32
 
 #ifndef _WIN32
@@ -8767,6 +8789,13 @@ int main(int argc, char **argv)
             case QEMU_OPTION_show_cursor:
                 cursor_hide = 0;
                 break;
+            case QEMU_OPTION_uuid:
+                if(qemu_uuid_parse(optarg, qemu_uuid) < 0) {
+                    fprintf(stderr, "Fail to parse UUID string."
+                            " Wrong format.\n");
+                    exit(1);
+                }
+                break;
 	    case QEMU_OPTION_daemonize:
 		daemonize = 1;
 		break;

[Qemu-devel] [PATCH 3/6] Add "info uuid" command to monitor.

[Gleb Natapov]

Signed-off-by: Gleb Natapov <gleb@qumranet.com>
---

 monitor.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index e71f49e..1e477bb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -253,6 +253,17 @@ static void do_info_name(void)
         term_printf("%s\n", qemu_name);
 }
 
+static void do_info_uuid(void)
+{
+    char uuid_str[37];
+    sprintf(uuid_str, UUID_FMT, qemu_uuid[0], qemu_uuid[1], qemu_uuid[2],
+            qemu_uuid[3], qemu_uuid[4], qemu_uuid[5], qemu_uuid[6],
+            qemu_uuid[7], qemu_uuid[8], qemu_uuid[9], qemu_uuid[10],
+            qemu_uuid[11], qemu_uuid[12], qemu_uuid[13], qemu_uuid[14],
+            qemu_uuid[15]);
+    term_printf("%s\n", uuid_str);
+}
+
 static void do_info_block(void)
 {
     bdrv_info();
@@ -1501,6 +1512,8 @@ static term_cmd_t info_cmds[] = {
       "", "show the vnc server status"},
     { "name", "", do_info_name,
       "", "show the current VM name" },
+    { "uuid", "", do_info_uuid,
+      "", "show the current VM UUID" },
 #if defined(TARGET_PPC)
     { "cpustats", "", do_info_cpu_stats,
       "", "show CPU statistics", },

[Qemu-devel] [PATCH 4/6] Use libuuid if available.

[Gleb Natapov]

If libuuid is available use it for UUID generation in case a user does not
provide one.

Signed-off-by: Gleb Natapov <gleb@qumranet.com>
---

 Makefile.target |    4 ++++
 configure       |   13 +++++++++++++
 vl.c            |   13 +++++++++++++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 2464484..54defd9 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -518,6 +518,10 @@ CPPFLAGS += $(CONFIG_VNC_TLS_CFLAGS)
 LIBS += $(CONFIG_VNC_TLS_LIBS)
 endif
 
+ifdef CONFIG_UUID
+LIBS += -luuid
+endif
+
 # SCSI layer
 OBJS+= lsi53c895a.o esp.o
 
diff --git a/configure b/configure
index acb4a4a..2815ca5 100755
--- a/configure
+++ b/configure
@@ -110,6 +110,7 @@ curses="yes"
 aio="yes"
 nptl="yes"
 mixemu="no"
+uuid="yes"
 
 # OS specific
 targetos=`uname -s`
@@ -316,6 +317,8 @@ for opt do
   ;;
   --enable-uname-release=*) uname_release="$optarg"
   ;;
+  --disable-uuid) uuid="no"
+  ;;
   --sparc_cpu=*)
       sparc_cpu="$optarg"
       case $sparc_cpu in
@@ -779,6 +782,11 @@ EOF
     fi
 fi
 
+# uuid library
+if test "$uuid" = "yes" ; then
+  `pkg-config uuid 2> /dev/null` || uuid="no"
+fi
+
 ##########################################
 # Sound support libraries probe
 
@@ -961,6 +969,7 @@ echo "uname -r          $uname_release"
 echo "NPTL support      $nptl"
 echo "vde support       $vde"
 echo "AIO support       $aio"
+echo "UUID support      $uuid"
 
 if test $sdl_too_old = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -1168,6 +1177,10 @@ if test "$vnc_tls" = "yes" ; then
   echo "CONFIG_VNC_TLS_LIBS=$vnc_tls_libs" >> $config_mak
   echo "#define CONFIG_VNC_TLS 1" >> $config_h
 fi
+if test "$uuid" = "yes" ; then
+  echo "CONFIG_UUID=yes" >> $config_mak
+  echo "#define CONFIG_UUID 1" >> $config_h
+fi
 qemu_version=`head $source_path/VERSION`
 echo "VERSION=$qemu_version" >>$config_mak
 echo "#define QEMU_VERSION \"$qemu_version\"" >> $config_h
diff --git a/vl.c b/vl.c
index e2b4042..532a03e 100644
--- a/vl.c
+++ b/vl.c
@@ -138,6 +138,11 @@ int inet_aton(const char *cp, struct in_addr *ia);
 
 #include "exec-all.h"
 
+#ifdef CONFIG_UUID
+#include <uuid/uuid.h>
+static int uuid_dont_generate;
+#endif
+
 #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
 #define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
 #ifdef __sun__
@@ -8795,6 +8800,9 @@ int main(int argc, char **argv)
                             " Wrong format.\n");
                     exit(1);
                 }
+#ifdef CONFIG_UUID
+                uuid_dont_generate = 1;
+#endif
                 break;
 	    case QEMU_OPTION_daemonize:
 		daemonize = 1;
@@ -8895,6 +8903,11 @@ int main(int argc, char **argv)
            monitor_device = "stdio";
     }
 
+#if CONFIG_UUID
+    if (!uuid_dont_generate)
+        uuid_generate(qemu_uuid);
+#endif
+
 #ifndef _WIN32
     if (daemonize) {
 	pid_t pid;

[Qemu-devel] [PATCH 5/6] Add UUID to BIOS configuration info.

[Gleb Natapov]

---

 hw/pc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index ccbe4b3..03b1b58 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -61,6 +61,7 @@ typedef struct _BIOSCfgEntry {
 
 typedef enum _BIOSCfgEntryNum{
     BIOS_CFG_SIGNATURE,
+    BIOS_CFG_UUID,
     BIOS_CFG_MAX_ENTRY
 } BIOSCfgEntryNum;
 
@@ -921,6 +922,7 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     register_ioport_write(BIOS_CFG_IOPORT, 1, 1, bios_cfg_write, NULL);
 
     bios_cfg_add_data(BIOS_CFG_SIGNATURE, "QEMU", 4);
+    bios_cfg_add_data(BIOS_CFG_UUID, qemu_uuid, 16);
 
     bochs_bios_init();
 

[Qemu-devel] [PATCH 6/6] Pass cpu speed into SM BIOS.

[Gleb Natapov]

---

 hw/pc.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 88 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 03b1b58..a1dd3f4 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -62,6 +62,7 @@ typedef struct _BIOSCfgEntry {
 typedef enum _BIOSCfgEntryNum{
     BIOS_CFG_SIGNATURE,
     BIOS_CFG_UUID,
+    BIOS_CFG_CPUSPEED,
     BIOS_CFG_MAX_ENTRY
 } BIOSCfgEntryNum;
 
@@ -73,6 +74,8 @@ typedef struct _BIOSCfgState {
 
 static BIOSCfgState bios_params;
 
+static uint32_t cpu_speed;
+
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
 }
@@ -763,6 +766,89 @@ static int bios_cfg_add_data(BIOSCfgEntryNum entry, uint8_t *data, uint16_t len)
     return 1;
 }
 
+
+#ifdef __linux__
+/* get_freq () function is taken from conky source code */
+#define CPUFREQ_PREFIX "/sys/devices/system/cpu"
+#define CPUFREQ_POSTFIX "cpufreq/scaling_cur_freq"
+
+/* return system frequency in MHz (use divisor=1) or GHz (use divisor=1000) */
+static double get_freq(int divisor, unsigned int cpu)
+{
+	FILE *f;
+	char frequency[32];
+	char s[256];
+	double freq = 0;
+
+	if (divisor <= 0)
+		return 0;
+
+	snprintf(s, 256, "%s/cpu%d/%s", CPUFREQ_PREFIX, cpu - 1, CPUFREQ_POSTFIX);
+	f = fopen(s, "r");
+	if (f) {
+		/* if there's a cpufreq /sys node, read the current frequency from
+		 * this node and divide by 1000 to get Mhz. */
+		if (fgets(s, sizeof(s), f)) {
+			s[strlen(s) - 1] = '\0';
+			freq = strtod(s, NULL);
+		}
+		fclose(f);
+		return (freq / 1000) / divisor;
+	}
+
+	// open the CPU information file
+	f = fopen("/proc/cpuinfo", "r");
+	if (!f) {
+		perror("Failed to access '/proc/cpuinfo' at get_freq()");
+		return 0;
+	}
+
+	// read the file
+	while (fgets(s, sizeof(s), f) != NULL) {
+
+#if defined(__i386) || defined(__x86_64)
+		// and search for the cpu mhz
+		if (strncmp(s, "cpu MHz", 7) == 0 && cpu == 0) {
+#else
+#if defined(__alpha)
+		// different on alpha
+		if (strncmp(s, "cycle frequency [Hz]", 20) == 0 && cpu == 0) {
+#else
+		// this is different on ppc for some reason
+		if (strncmp(s, "clock", 5) == 0 && cpu == 0) {
+#endif // defined(__alpha)
+#endif // defined(__i386) || defined(__x86_64)
+
+			// copy just the number
+			strcpy(frequency, strchr(s, ':') + 2);
+#if defined(__alpha)
+			// strip " est.\n"
+			frequency[strlen(frequency) - 6] = '\0';
+			// kernel reports in Hz
+			freq = strtod(frequency, NULL) / 1000000;
+#else
+			// strip \n
+			frequency[strlen(frequency) - 1] = '\0';
+			freq = strtod(frequency, NULL);
+#endif
+			break;
+		}
+		if (strncmp(s, "processor", 9) == 0) {
+			cpu--;
+			continue;
+		}
+	}
+
+	fclose(f);
+	return freq / divisor;
+}
+#else
+static double get_freq(int divisor, unsigned int cpu)
+{
+    return 0;
+}
+#endif
+
 /* PC hardware initialisation */
 static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
                      const char *boot_device, DisplayState *ds,
@@ -923,6 +1009,8 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 
     bios_cfg_add_data(BIOS_CFG_SIGNATURE, "QEMU", 4);
     bios_cfg_add_data(BIOS_CFG_UUID, qemu_uuid, 16);
+    cpu_speed = (uint32_t)get_freq(1, 1);
+    bios_cfg_add_data(BIOS_CFG_CPUSPEED, (uint8_t*)&cpu_speed, 4);
 
     bochs_bios_init();
 

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> Hello,
>
>   This is one more try to push this patch series into qemu. This time
>  I don't use vmport interface. Instead I implemented simple communication
>  channel between  qemu and BIOS using IO port.  I decided not to  use
>  firmware interface since current users of the interface may depend on
>  exact structure layout, so it doesn't look like a good idea to add
>  arbitrary fields there. Also I don't want to copy the whole ROM into BIOS
>  just to access one field of it. IO channel is implemented by the first
>  patch of the series. Patches 2-5 implement UUID related stuff. Patch 6
>  passes host CPU frequency to fill in SM BIOS table.
>
>  Please comment.

I disagree about not using the firmware interface. The structure can
be extended and even new versions defined, all structure users are
open source. UUID is not architecture specific, so it should use the
main structure instead if the architecture specific substructure
(nvram_arch*). Adding UUID to unused fields will not break anything.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Gleb Natapov]

On Sun, Aug 24, 2008 at 03:01:22PM +0300, Blue Swirl wrote:
> On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> > Hello,
> >
> >   This is one more try to push this patch series into qemu. This time
> >  I don't use vmport interface. Instead I implemented simple communication
> >  channel between  qemu and BIOS using IO port.  I decided not to  use
> >  firmware interface since current users of the interface may depend on
> >  exact structure layout, so it doesn't look like a good idea to add
> >  arbitrary fields there. Also I don't want to copy the whole ROM into BIOS
> >  just to access one field of it. IO channel is implemented by the first
> >  patch of the series. Patches 2-5 implement UUID related stuff. Patch 6
> >  passes host CPU frequency to fill in SM BIOS table.
> >
> >  Please comment.
> 
> I disagree about not using the firmware interface. The structure can
> be extended and even new versions defined, all structure users are
> open source.
It is hard enough to change interface between qemu and bochs BIOS. How
complex it will be to simultaneously change interface for several boot
loaders.

>               UUID is not architecture specific, so it should use the
> main structure instead if the architecture specific substructure
> (nvram_arch*). Adding UUID to unused fields will not break anything.
Most info in ohwcfg_v3_t are not needed (or can be obtained by other
means) by PC BIOS, so there is no point in coping the whole structure
into BIOS. Of cause BIOS don't have to copy entire ohwcfg_v3_t, but
access only required fields by reading only specific offsets, but then the
interface will be exactly like the one I proposed with only difference
that instead of specifying magic value (like 1 for reading UUID in my
patch series), BIOS will have to specify magic offset (like 0xE0).

--
			Gleb.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> On Sun, Aug 24, 2008 at 03:01:22PM +0300, Blue Swirl wrote:
>  > On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
>  > > Hello,
>  > >
>  > >   This is one more try to push this patch series into qemu. This time
>  > >  I don't use vmport interface. Instead I implemented simple communication
>  > >  channel between  qemu and BIOS using IO port.  I decided not to  use
>  > >  firmware interface since current users of the interface may depend on
>  > >  exact structure layout, so it doesn't look like a good idea to add
>  > >  arbitrary fields there. Also I don't want to copy the whole ROM into BIOS
>  > >  just to access one field of it. IO channel is implemented by the first
>  > >  patch of the series. Patches 2-5 implement UUID related stuff. Patch 6
>  > >  passes host CPU frequency to fill in SM BIOS table.
>  > >
>  > >  Please comment.
>  >
>  > I disagree about not using the firmware interface. The structure can
>  > be extended and even new versions defined, all structure users are
>  > open source.
>
> It is hard enough to change interface between qemu and bochs BIOS. How
>  complex it will be to simultaneously change interface for several boot
>  loaders.

This is because no Qemu developer has commit rights to Bochs, and
Bochs releases too infrequently. With commit rights, synchronizing a
commit would not be too difficult. And this should be needed only if
the common interface changes incompatibly.

>  >               UUID is not architecture specific, so it should use the
>  > main structure instead if the architecture specific substructure
>  > (nvram_arch*). Adding UUID to unused fields will not break anything.
>
> Most info in ohwcfg_v3_t are not needed (or can be obtained by other
>  means) by PC BIOS, so there is no point in coping the whole structure
>  into BIOS. Of cause BIOS don't have to copy entire ohwcfg_v3_t, but

For example, Bochs seems to use i440fx registers to determine the
available physical memory. This could be changed to use the
configuration structure instead. It's a matter of taste, but I would
find this an improvement.

>  access only required fields by reading only specific offsets, but then the
>  interface will be exactly like the one I proposed with only difference
>  that instead of specifying magic value (like 1 for reading UUID in my
>  patch series), BIOS will have to specify magic offset (like 0xE0).

There is no need for a magic offset, ohwcfg_v3_t is designed to be
included even from asm.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Gleb Natapov]

On Sun, Aug 24, 2008 at 03:45:51PM +0300, Blue Swirl wrote:
> > It is hard enough to change interface between qemu and bochs BIOS. How
> >  complex it will be to simultaneously change interface for several boot
> >  loaders.
> 
> This is because no Qemu developer has commit rights to Bochs, and
> Bochs releases too infrequently. With commit rights, synchronizing a
> commit would not be too difficult. And this should be needed only if
> the common interface changes incompatibly.
> 
Is this situation going to change? Are Qemu developers has commit rights
to other boot loader?

BTW we are going to use qemu<->bios channel not only for UUID and cpu
frequency, but for number of other things. Adding ACPI tables provided
by user for instance (this is needed for Vista OEM installation).

> >  >               UUID is not architecture specific, so it should use the
> >  > main structure instead if the architecture specific substructure
> >  > (nvram_arch*). Adding UUID to unused fields will not break anything.
> >
> > Most info in ohwcfg_v3_t are not needed (or can be obtained by other
> >  means) by PC BIOS, so there is no point in coping the whole structure
> >  into BIOS. Of cause BIOS don't have to copy entire ohwcfg_v3_t, but
> 
> For example, Bochs seems to use i440fx registers to determine the
> available physical memory. This could be changed to use the
> configuration structure instead. It's a matter of taste, but I would
> find this an improvement.
If by Bochs you mean Bochs BIOS then this is not the case. There are
standard CMOS locations where amount of available memory is specified.
Qemu initialize those locations and BIOS reads them. I don't think this is
going to change since Bochs BIOS is used not only by Qemu. If only Bochs
will move to firmware interface too...

> 
> >  access only required fields by reading only specific offsets, but then the
> >  interface will be exactly like the one I proposed with only difference
> >  that instead of specifying magic value (like 1 for reading UUID in my
> >  patch series), BIOS will have to specify magic offset (like 0xE0).
> 
> There is no need for a magic offset, ohwcfg_v3_t is designed to be
> included even from asm.
> 
The thing is I don't want to copy the whole ROM into BIOS during boot.
As far as I see on other platforms ROM is memory mapped, so no copying
is required and this was you first proposal, but if we are going to use
port IO to access ROM then it will either have to be copied into main
memory and then accessed, or only required fields  will have to be
copied and that mean coping data from magic offsets.

--
			Gleb.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Avi Kivity]

Blue Swirl wrote:
> For example, Bochs seems to use i440fx registers to determine the
> available physical memory. This could be changed to use the
> configuration structure instead. It's a matter of taste, but I would
> find this an improvement.
>   

More hardwarelike would be to detect how much memory exists and have the 
bios program those registers, instead of reading those registers (if I 
understand you correctly).


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> On Sun, Aug 24, 2008 at 03:45:51PM +0300, Blue Swirl wrote:
>  > > It is hard enough to change interface between qemu and bochs BIOS. How
>  > >  complex it will be to simultaneously change interface for several boot
>  > >  loaders.
>  >
>  > This is because no Qemu developer has commit rights to Bochs, and
>  > Bochs releases too infrequently. With commit rights, synchronizing a
>  > commit would not be too difficult. And this should be needed only if
>  > the common interface changes incompatibly.
>  >
>
> Is this situation going to change? Are Qemu developers has commit rights
>  to other boot loader?

I can commit to OpenBIOS, if that's what you asked.

>  BTW we are going to use qemu<->bios channel not only for UUID and cpu
>  frequency, but for number of other things. Adding ACPI tables provided
>  by user for instance (this is needed for Vista OEM installation).

ACPI is architecture-specific, so this can easily go to arch area. And
ROM works here fine.

>  > >  >               UUID is not architecture specific, so it should use the
>  > >  > main structure instead if the architecture specific substructure
>  > >  > (nvram_arch*). Adding UUID to unused fields will not break anything.
>  > >
>  > > Most info in ohwcfg_v3_t are not needed (or can be obtained by other
>  > >  means) by PC BIOS, so there is no point in coping the whole structure
>  > >  into BIOS. Of cause BIOS don't have to copy entire ohwcfg_v3_t, but
>  >
>  > For example, Bochs seems to use i440fx registers to determine the
>  > available physical memory. This could be changed to use the
>  > configuration structure instead. It's a matter of taste, but I would
>  > find this an improvement.
>
> If by Bochs you mean Bochs BIOS then this is not the case. There are
>  standard CMOS locations where amount of available memory is specified.
>  Qemu initialize those locations and BIOS reads them. I don't think this is
>  going to change since Bochs BIOS is used not only by Qemu. If only Bochs
>  will move to firmware interface too...

If we can push UUID or ACPI patches, we can push just as easily
configuration ROM and related interface patches. Of course Bochs devs
could want other changes.

>  > >  access only required fields by reading only specific offsets, but then the
>  > >  interface will be exactly like the one I proposed with only difference
>  > >  that instead of specifying magic value (like 1 for reading UUID in my
>  > >  patch series), BIOS will have to specify magic offset (like 0xE0).
>  >
>  > There is no need for a magic offset, ohwcfg_v3_t is designed to be
>  > included even from asm.
>  >
>
> The thing is I don't want to copy the whole ROM into BIOS during boot.
>  As far as I see on other platforms ROM is memory mapped, so no copying
>  is required and this was you first proposal, but if we are going to use
>  port IO to access ROM then it will either have to be copied into main
>  memory and then accessed, or only required fields  will have to be
>  copied and that mean coping data from magic offsets.

I'd still prefer to use a memory mapped interface, but I was told that
this would not be easy to access from 16 bit code. It's equally
possible to make the ROM indexed even on Sparc32 or maybe even add a
hidden m48t59 to PC.

Maybe we mean different things with a magic offset. Bochs BIOS sources
can include firmware_abi.h unchanged, then use the #defined offsets to
access either a memory-mapped ROM or IO port version. To my mind this
does not mean using any magic offsets.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Blue Swirl]

On 8/24/08, Avi Kivity <avi@qumranet.com> wrote:
> Blue Swirl wrote:
>
> > For example, Bochs seems to use i440fx registers to determine the
> > available physical memory. This could be changed to use the
> > configuration structure instead. It's a matter of taste, but I would
> > find this an improvement.
> >
> >
>
>  More hardwarelike would be to detect how much memory exists and have the
> bios program those registers, instead of reading those registers (if I
> understand you correctly).

True, but realistic probing for memory could take some time if there
is a huge amount of it. Moreover, this level of precision would not
gain anything in Qemu use, it would be unnecessary. If Bochs BIOS was
aiming to be used as a real, physical PC BIOS in the future, then
these kind of improvements might be useful.

A long time ago on Sparc32, I put the memory size to one of CPU
registers. That was quick and easy for both sides but dirty.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Avi Kivity]

Blue Swirl wrote:
> True, but realistic probing for memory could take some time if there
> is a huge amount of it.

No reason for that.  A binary search with a cap of 1TB and a granularity 
of 1MB requires only 20 probes.

>  Moreover, this level of precision would not
> gain anything in Qemu use, it would be unnecessary. If Bochs BIOS was
> aiming to be used as a real, physical PC BIOS in the future, then
> these kind of improvements might be useful.
>
>   

Agree, it isn't necessary.

> A long time ago on Sparc32, I put the memory size to one of CPU
> registers. That was quick and easy for both sides but dirty.
>   

For x86 it seems like an obvious candidate for vmport (and I don't 
understand the object to using vmport for uuid).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Gleb Natapov]

[-- Attachment #1: Type: text/plain, Size: 3781 bytes --]

On Sun, Aug 24, 2008 at 04:35:00PM +0300, Blue Swirl wrote:
> >  > >  >               UUID is not architecture specific, so it should use the
> >  > >  > main structure instead if the architecture specific substructure
> >  > >  > (nvram_arch*). Adding UUID to unused fields will not break anything.
> >  > >
> >  > > Most info in ohwcfg_v3_t are not needed (or can be obtained by other
> >  > >  means) by PC BIOS, so there is no point in coping the whole structure
> >  > >  into BIOS. Of cause BIOS don't have to copy entire ohwcfg_v3_t, but
> >  >
> >  > For example, Bochs seems to use i440fx registers to determine the
> >  > available physical memory. This could be changed to use the
> >  > configuration structure instead. It's a matter of taste, but I would
> >  > find this an improvement.
> >
> > If by Bochs you mean Bochs BIOS then this is not the case. There are
> >  standard CMOS locations where amount of available memory is specified.
> >  Qemu initialize those locations and BIOS reads them. I don't think this is
> >  going to change since Bochs BIOS is used not only by Qemu. If only Bochs
> >  will move to firmware interface too...
> 
> If we can push UUID or ACPI patches, we can push just as easily
> configuration ROM and related interface patches. Of course Bochs devs
> could want other changes.
I don't what to speak on behalf of bochs developers, but I am not sure
about "as easily" part. Why change something that works for both qemu and
bochs to qemu specific way and guard it with ifdefs.

> 
> >  > >  access only required fields by reading only specific offsets, but then the
> >  > >  interface will be exactly like the one I proposed with only difference
> >  > >  that instead of specifying magic value (like 1 for reading UUID in my
> >  > >  patch series), BIOS will have to specify magic offset (like 0xE0).
> >  >
> >  > There is no need for a magic offset, ohwcfg_v3_t is designed to be
> >  > included even from asm.
> >  >
> >
> > The thing is I don't want to copy the whole ROM into BIOS during boot.
> >  As far as I see on other platforms ROM is memory mapped, so no copying
> >  is required and this was you first proposal, but if we are going to use
> >  port IO to access ROM then it will either have to be copied into main
> >  memory and then accessed, or only required fields  will have to be
> >  copied and that mean coping data from magic offsets.
> 
> I'd still prefer to use a memory mapped interface, but I was told that
> this would not be easy to access from 16 bit code. It's equally
> possible to make the ROM indexed even on Sparc32 or maybe even add a
> hidden m48t59 to PC.
> 
> Maybe we mean different things with a magic offset. Bochs BIOS sources
> can include firmware_abi.h unchanged, then use the #defined offsets to
> access either a memory-mapped ROM or IO port version. To my mind this
> does not mean using any magic offsets.

By magic offset I mean 0xE0 in the code below that will be added to BIOS to
read UUID in case we will use firmware interface:

     outl(QEMU_CFG_PORT, 0xE0 /* why not 1 ? */);
     for(i=0;i<16;i++)
       uuid[i] = intb(QEMU_CFG_PORT);

To read CPU frequency we should do:
     outl(QEMU_CFG_PORT, some other offset /* why not 2 */);

To read acpi table:
     outl(QEMU_CFG_PORT, one more offset /* why not 3 */)

To read one more parameter
     outl(QEMU_CFG_PORT, this time the offset depends on the size of previously read acpi table)


The patch that make bochs BIOS to use my proposed interface is attached
just to clarify the interface.

So let me ask you a question. What's actually the advantage of using firmware
ABI? It surely don't make our job easier as we have to extend it to get
information we need and we don't need any information it provides.

--
			Gleb.

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

commit ae12d85485bd7bf0434f13619f49274ad9c18d93
Author: Gleb Natapov <gleb@qumranet.com>
Date:   Sun Aug 24 10:59:25 2008 +0300

    Use new io port interface for qemu<->BIOS communication.

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 2dc1d25..bdbd0fd 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -443,25 +443,49 @@ void wrmsr_smp(uint32_t index, uint64_t val)
     p->ecx = 0;
 }
 
+#ifdef BX_QEMU
+#define QEMU_CFG_PORT 0x1234
+enum {
+    QEMU_CFG_SIGNATURE,
+    QEMU_CFG_UUID,
+    QEMU_CFG_CPUSPEED,
+    QEMU_CFG_MAX_ENTRY
+};
+
+int qemu_cfg_port;
+
+void qemu_cfg_select(int f)
+{
+    outb(QEMU_CFG_PORT, f);
+}
+
+int qemu_cfg_port_probe()
+{
+    char *sig = "QEMU";
+    int i;
+
+    qemu_cfg_select(QEMU_CFG_SIGNATURE);
+
+    for (i = 0; i < 4; i++)
+        if (inb(QEMU_CFG_PORT) != sig[i])
+            return 0;
+
+    return 1;
+}
+
+void qemu_cfg_read(uint8_t *buf, int len)
+{
+    while (len--)
+        *(buf++) = inb(QEMU_CFG_PORT);
+}
+#endif
+
 void uuid_probe(void)
 {
 #ifdef BX_QEMU
-    uint32_t eax, ebx, ecx, edx;
-
-    // check if backdoor port exists
-    asm volatile ("outl %%eax, %%dx"
-        : "=a" (eax), "=b" (ebx), "=c" (ecx), "=d" (edx)
-        : "a" (0x564d5868), "c" (0xa), "d" (0x5658));
-    if (ebx == 0x564d5868) {
-        uint32_t *uuid_ptr = (uint32_t *)bios_uuid;
-        // get uuid
-        asm volatile ("outl %%eax, %%dx"
-            : "=a" (eax), "=b" (ebx), "=c" (ecx), "=d" (edx)
-            : "a" (0x564d5868), "c" (0x13), "d" (0x5658));
-        uuid_ptr[0] = eax;
-        uuid_ptr[1] = ebx;
-        uuid_ptr[2] = ecx;
-        uuid_ptr[3] = edx;
+    if(qemu_cfg_port) {
+        qemu_cfg_select(QEMU_CFG_UUID);
+        qemu_cfg_read(bios_uuid, 16);
     } else
 #endif
     {
@@ -1830,6 +1854,24 @@ smbios_type_3_init(void *start)
     return start+2;
 }
 
+static int qery_cpu_freq(void)
+{
+#ifdef BX_QEMU
+    int cpu_speed;
+
+    if(!qemu_cfg_port)
+        return 0;
+
+    qemu_cfg_select(QEMU_CFG_CPUSPEED);
+    qemu_cfg_read((uint8_t*)&cpu_speed, 4);
+
+    return cpu_speed;
+#else
+    return 0;
+#endif
+}
+
+
 /* Type 4 -- Processor Information */
 static void *
 smbios_type_4_init(void *start, unsigned int cpu_number)
@@ -1852,8 +1894,8 @@ smbios_type_4_init(void *start, unsigned int cpu_number)
     p->voltage = 0;
     p->external_clock = 0;
 
-    p->max_speed = 0; /* unknown */
-    p->current_speed = 0; /* unknown */
+    p->max_speed = 0x0FA0; /* 4000 MHZ */
+    p->current_speed = qery_cpu_freq(); /* unknown */
 
     p->status = 0x41; /* socket populated, CPU enabled */
     p->processor_upgrade = 0x01; /* other */
@@ -2058,6 +2100,10 @@ void rombios32_init(void)
 
     init_smp_msrs();
 
+#ifdef BX_QEMU
+    qemu_cfg_port = qemu_cfg_port_probe();
+#endif
+
     ram_probe();
 
     cpu_probe();

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Blue Swirl]

On 8/24/08, Avi Kivity <avi@qumranet.com> wrote:
> Blue Swirl wrote:
>
> > True, but realistic probing for memory could take some time if there
> > is a huge amount of it.
> >
>
>  No reason for that.  A binary search with a cap of 1TB and a granularity of
> 1MB requires only 20 probes.

But on real machines there could be gaps between the memory banks,
memory banks may be of different sizes and then there are aliasing
effects. I doubt you can manage with 20 probes then.

> >  Moreover, this level of precision would not
> > gain anything in Qemu use, it would be unnecessary. If Bochs BIOS was
> > aiming to be used as a real, physical PC BIOS in the future, then
> > these kind of improvements might be useful.
> >
> >
> >
>
>  Agree, it isn't necessary.
>
>
> > A long time ago on Sparc32, I put the memory size to one of CPU
> > registers. That was quick and easy for both sides but dirty.
> >
> >
>
>  For x86 it seems like an obvious candidate for vmport (and I don't
> understand the object to using vmport for uuid).

UUID can be used in all machines, so it would be nice if the interface
was same. I don't see much benefit in vmport compared to ROM solution.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> On Sun, Aug 24, 2008 at 04:35:00PM +0300, Blue Swirl wrote:
>  > >  > >  >               UUID is not architecture specific, so it should use the
>  > >  > >  > main structure instead if the architecture specific substructure
>  > >  > >  > (nvram_arch*). Adding UUID to unused fields will not break anything.
>  > >  > >
>  > >  > > Most info in ohwcfg_v3_t are not needed (or can be obtained by other
>  > >  > >  means) by PC BIOS, so there is no point in coping the whole structure
>  > >  > >  into BIOS. Of cause BIOS don't have to copy entire ohwcfg_v3_t, but
>  > >  >
>  > >  > For example, Bochs seems to use i440fx registers to determine the
>  > >  > available physical memory. This could be changed to use the
>  > >  > configuration structure instead. It's a matter of taste, but I would
>  > >  > find this an improvement.
>  > >
>  > > If by Bochs you mean Bochs BIOS then this is not the case. There are
>  > >  standard CMOS locations where amount of available memory is specified.
>  > >  Qemu initialize those locations and BIOS reads them. I don't think this is
>  > >  going to change since Bochs BIOS is used not only by Qemu. If only Bochs
>  > >  will move to firmware interface too...
>  >
>  > If we can push UUID or ACPI patches, we can push just as easily
>  > configuration ROM and related interface patches. Of course Bochs devs
>  > could want other changes.
>
> I don't what to speak on behalf of bochs developers, but I am not sure
>  about "as easily" part. Why change something that works for both qemu and
>  bochs to qemu specific way and guard it with ifdefs.
>
>
>  >
>  > >  > >  access only required fields by reading only specific offsets, but then the
>  > >  > >  interface will be exactly like the one I proposed with only difference
>  > >  > >  that instead of specifying magic value (like 1 for reading UUID in my
>  > >  > >  patch series), BIOS will have to specify magic offset (like 0xE0).
>  > >  >
>  > >  > There is no need for a magic offset, ohwcfg_v3_t is designed to be
>  > >  > included even from asm.
>  > >  >
>  > >
>  > > The thing is I don't want to copy the whole ROM into BIOS during boot.
>  > >  As far as I see on other platforms ROM is memory mapped, so no copying
>  > >  is required and this was you first proposal, but if we are going to use
>  > >  port IO to access ROM then it will either have to be copied into main
>  > >  memory and then accessed, or only required fields  will have to be
>  > >  copied and that mean coping data from magic offsets.
>  >
>  > I'd still prefer to use a memory mapped interface, but I was told that
>  > this would not be easy to access from 16 bit code. It's equally
>  > possible to make the ROM indexed even on Sparc32 or maybe even add a
>  > hidden m48t59 to PC.
>  >
>  > Maybe we mean different things with a magic offset. Bochs BIOS sources
>  > can include firmware_abi.h unchanged, then use the #defined offsets to
>  > access either a memory-mapped ROM or IO port version. To my mind this
>  > does not mean using any magic offsets.
>
>
> By magic offset I mean 0xE0 in the code below that will be added to BIOS to
>  read UUID in case we will use firmware interface:

#include "firmware_abi.h"

>      outl(QEMU_CFG_PORT, 0xE0 /* why not 1 ? */);
s/0xE0/OHW_UUID/g

>      for(i=0;i<16;i++)
>        uuid[i] = intb(QEMU_CFG_PORT);
>
>  To read CPU frequency we should do:
>      outl(QEMU_CFG_PORT, some other offset /* why not 2 */);
>
>  To read acpi table:
>      outl(QEMU_CFG_PORT, one more offset /* why not 3 */)
>
>  To read one more parameter
>      outl(QEMU_CFG_PORT, this time the offset depends on the size of previously read acpi table)
>
>
>  The patch that make bochs BIOS to use my proposed interface is attached
>  just to clarify the interface.
>
>  So let me ask you a question. What's actually the advantage of using firmware
>  ABI? It surely don't make our job easier as we have to extend it to get
>  information we need and we don't need any information it provides.

There is a signature, checksum, version ID. You already have signature
and not having a version ID will probably hurt later. I think there
was some discussion about SMP detection being unreliable, so getting
nb_cpus this way would be an improvement.

Of course you could add these and all the fields in the ohwcfg_v3_t
structure to the vmport protocol and even add Sparc-specific fields as
well. Then the device could be used in all machines so that it would
not be PC-specific.

I think the question is, is interactive IO port (or MMIO) protocol
better than configuration ROM? I'm not so sure anymore. Protocols can
be extended more easily. ROM is more realistic and it feels more
secure.

Re: [Qemu-devel] [PATCH 1/6] Use IO port for qemu<->guest BIOS communication.

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> Use PIO to get configuration info between qemu process and guest BIOS.

Could you make this a separate device, so that it could be used in
other machines? There is nothing PC-specific.

>  +static uint32_t bios_cfg_read(void *opaque, uint32_t addr)
>  +{
>  +    BIOSCfgEntry *e = &bios_params.entries[bios_params.entry];

You should use the opaque parameter and cast that to BIOSCfgState.

>  +    if (!e->data)
>  +        return 0;
>  +
>  +    return e->data[bios_params.cur_offset++ % e->len];

Instead of using modular arithmetic, zero should be returned for invalid values.

>  +static void bios_cfg_write(void *opaque, uint32_t addr, uint32_t value)
>  +{
>  +    bios_params.entry = value % BIOS_CFG_MAX_ENTRY;

Same here, its important for downward compatibility.

>  +    bios_cfg_add_data(BIOS_CFG_SIGNATURE, "QEMU", 4);

I'd add:
+    bios_cfg_add_data(BIOS_CFG_ID, 1, 4);

Re: [Qemu-devel] [PATCH 3/6] Add "info uuid" command to monitor.

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
>  ---
>
>   monitor.c |   13 +++++++++++++
>   1 files changed, 13 insertions(+), 0 deletions(-)
>
>  diff --git a/monitor.c b/monitor.c
>  index e71f49e..1e477bb 100644
>  --- a/monitor.c
>  +++ b/monitor.c
>  @@ -253,6 +253,17 @@ static void do_info_name(void)
>          term_printf("%s\n", qemu_name);
>   }
>
>  +static void do_info_uuid(void)
>  +{
>  +    char uuid_str[37];
>  +    sprintf(uuid_str, UUID_FMT, qemu_uuid[0], qemu_uuid[1], qemu_uuid[2],
>  +            qemu_uuid[3], qemu_uuid[4], qemu_uuid[5], qemu_uuid[6],
>  +            qemu_uuid[7], qemu_uuid[8], qemu_uuid[9], qemu_uuid[10],
>  +            qemu_uuid[11], qemu_uuid[12], qemu_uuid[13], qemu_uuid[14],
>  +            qemu_uuid[15]);
>  +    term_printf("%s\n", uuid_str);

Please use snprintf. Or why not directly term_printf(UUID_FMT "\n",
qemu_uuid[0] etc.?

Re: [Qemu-devel] [PATCH 4/6] Use libuuid if available.

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
>  +# uuid library
>  +if test "$uuid" = "yes" ; then
>  +  `pkg-config uuid 2> /dev/null` || uuid="no"
>  +fi

Please implement the probe like ncurses one.

>  +static int uuid_dont_generate;

Positive logic would be clearer: generate_uuid = 1;

>  +#ifdef CONFIG_UUID
>  +                uuid_dont_generate = 1;

generate_uuid = 0;

>  +    if (!uuid_dont_generate)
>  +        uuid_generate(qemu_uuid);

if (generate_uuid)

Re: [Qemu-devel] [PATCH 6/6] Pass cpu speed into SM BIOS.

[Blue Swirl]

On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
>  +static uint32_t cpu_speed;

32 bits won't be enough when we get >4GHz CPUs.

+static double get_freq(int divisor, unsigned int cpu)

I'd use uint64_t and throw away the divisor and double math.

On Sparc64, the frequency line in /proc/cpuinfo looks like:
Cpu0ClkTck      : 000000000ee6b280

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Kevin O'Connor]

On Sun, Aug 24, 2008 at 04:55:44PM +0300, Avi Kivity wrote:
> Blue Swirl wrote:
>> True, but realistic probing for memory could take some time if there
>> is a huge amount of it.
>
> No reason for that.  A binary search with a cap of 1TB and a granularity  
> of 1MB requires only 20 probes.

Real x86 machines detect the available memory chips via the smbus and
then program the memory controller.  The initialization code must do
this work without accessing any memory.  The coreboot project
(coreboot.org) has more info if you're interested.

-Kevin

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Kevin O'Connor]

On Sun, Aug 24, 2008 at 04:35:00PM +0300, Blue Swirl wrote:
> I'd still prefer to use a memory mapped interface, but I was told that
> this would not be easy to access from 16 bit code. It's equally
> possible to make the ROM indexed even on Sparc32 or maybe even add a
> hidden m48t59 to PC.

The bochs bios code that produces the acpi and smbios tables runs in
32bit mode.  So, I'm not sure why 16bit mode would matter.

As an aside, I recently ported the bochs bios to gcc - I call the
result SeaBIOS.  (Binaries are available at
http://linuxtogo.org/~kevin/SeaBIOS/ and source via "git clone
git://git.linuxtogo.org/home/kevin/seabios.git".)

In addition to supporting bochs and qemu, the code also works on real
hardware via coreboot (see http://coreboot.org/ ).  This conversation
is interesting, because on real hardware seabios can't get memory size
and other parameters from cmos or from "magic" io ports.  Instead, it
has to locate and use tables that coreboot sets up.

This, by no means, should limit how qemu implements a feature, but
it's interesting to see that there is overlap with other projects.

-Kevin

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Avi Kivity]

Kevin O'Connor wrote:
> On Sun, Aug 24, 2008 at 04:55:44PM +0300, Avi Kivity wrote:
>   
>> Blue Swirl wrote:
>>     
>>> True, but realistic probing for memory could take some time if there
>>> is a huge amount of it.
>>>       
>> No reason for that.  A binary search with a cap of 1TB and a granularity  
>> of 1MB requires only 20 probes.
>>     
>
> Real x86 machines detect the available memory chips via the smbus and
> then program the memory controller.  The initialization code must do
> this work without accessing any memory.  The coreboot project
> (coreboot.org) has more info if you're interested.
>   

Yeah, last time I did memory detection was before this existed.  So I 
guess we are better off with a qemu-specific hack.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Avi Kivity]

Blue Swirl wrote:
> On 8/24/08, Avi Kivity <avi@qumranet.com> wrote:
>   
>> Blue Swirl wrote:
>>
>>     
>>> True, but realistic probing for memory could take some time if there
>>> is a huge amount of it.
>>>
>>>       
>>  No reason for that.  A binary search with a cap of 1TB and a granularity of
>> 1MB requires only 20 probes.
>>     
>
> But on real machines there could be gaps between the memory banks,
> memory banks may be of different sizes and then there are aliasing
> effects. I doubt you can manage with 20 probes then.
>
>   

If we limit ourselves to qemu, this doesn't happen.  But I withdraw my 
suggestion as it seems real machines don't work this way.

>>> A long time ago on Sparc32, I put the memory size to one of CPU
>>> registers. That was quick and easy for both sides but dirty.
>>>
>>>
>>>       
>>  For x86 it seems like an obvious candidate for vmport (and I don't
>> understand the object to using vmport for uuid).
>>     
>
> UUID can be used in all machines, so it would be nice if the interface
> was same. I don't see much benefit in vmport compared to ROM solution.
>
>   

Maybe UUID is generic but the vast majority of things that need to 
communicate to qemu will be arch specific.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 6/6] Pass cpu speed into SM BIOS.

[Gleb Natapov]

On Sun, Aug 24, 2008 at 09:14:23PM +0300, Blue Swirl wrote:
> On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> >  +static uint32_t cpu_speed;
> 
> 32 bits won't be enough when we get >4GHz CPUs.
> 
The value stored there is in MHz. Actually even 32 bits is to
much since SMBIOS specification uses 16 bit for CPU speed.

> +static double get_freq(int divisor, unsigned int cpu)
> 
> I'd use uint64_t and throw away the divisor and double math.
> 
I've copied this function from another project and didn't want
to change it much in case we will want to use newer version someday.

> On Sparc64, the frequency line in /proc/cpuinfo looks like:
> Cpu0ClkTck      : 000000000ee6b280
> 
Can somebody with this HW add support for it. I don't want to do changes
which I can't test.

--
			Gleb.

Re: [Qemu-devel] [PATCH 4/6] Use libuuid if available.

[Gleb Natapov]

On Sun, Aug 24, 2008 at 08:50:42PM +0300, Blue Swirl wrote:
> On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> >  +# uuid library
> >  +if test "$uuid" = "yes" ; then
> >  +  `pkg-config uuid 2> /dev/null` || uuid="no"
> >  +fi
> 
> Please implement the probe like ncurses one.
> 
> >  +static int uuid_dont_generate;
> 
> Positive logic would be clearer: generate_uuid = 1;
> 
> >  +#ifdef CONFIG_UUID
> >  +                uuid_dont_generate = 1;
> 
> generate_uuid = 0;
> 
> >  +    if (!uuid_dont_generate)
> >  +        uuid_generate(qemu_uuid);
> 
> if (generate_uuid)
> 
Done.

--
			Gleb.

Re: [Qemu-devel] [PATCH 3/6] Add "info uuid" command to monitor.

[Gleb Natapov]

On Sun, Aug 24, 2008 at 08:43:41PM +0300, Blue Swirl wrote:
> On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> > Signed-off-by: Gleb Natapov <gleb@qumranet.com>
> >  ---
> >
> >   monitor.c |   13 +++++++++++++
> >   1 files changed, 13 insertions(+), 0 deletions(-)
> >
> >  diff --git a/monitor.c b/monitor.c
> >  index e71f49e..1e477bb 100644
> >  --- a/monitor.c
> >  +++ b/monitor.c
> >  @@ -253,6 +253,17 @@ static void do_info_name(void)
> >          term_printf("%s\n", qemu_name);
> >   }
> >
> >  +static void do_info_uuid(void)
> >  +{
> >  +    char uuid_str[37];
> >  +    sprintf(uuid_str, UUID_FMT, qemu_uuid[0], qemu_uuid[1], qemu_uuid[2],
> >  +            qemu_uuid[3], qemu_uuid[4], qemu_uuid[5], qemu_uuid[6],
> >  +            qemu_uuid[7], qemu_uuid[8], qemu_uuid[9], qemu_uuid[10],
> >  +            qemu_uuid[11], qemu_uuid[12], qemu_uuid[13], qemu_uuid[14],
> >  +            qemu_uuid[15]);
> >  +    term_printf("%s\n", uuid_str);
> 
> Please use snprintf. Or why not directly term_printf(UUID_FMT "\n",
> qemu_uuid[0] etc.?
Don't know. I've removed redundant sprintf. Thanks.

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/6] Use IO port for qemu<->guest BIOS communication.

[Gleb Natapov]

On Sun, Aug 24, 2008 at 08:39:44PM +0300, Blue Swirl wrote:
> On 8/24/08, Gleb Natapov <gleb@qumranet.com> wrote:
> > Use PIO to get configuration info between qemu process and guest BIOS.
> 
> Could you make this a separate device, so that it could be used in
> other machines? There is nothing PC-specific.
The code is less then 50 lines. Is it worth to move it to a separate
file (two of them .c and .h) before there is a real need?

> 
> >  +static uint32_t bios_cfg_read(void *opaque, uint32_t addr)
> >  +{
> >  +    BIOSCfgEntry *e = &bios_params.entries[bios_params.entry];
> 
> You should use the opaque parameter and cast that to BIOSCfgState.
> 
> >  +    if (!e->data)
> >  +        return 0;
> >  +
> >  +    return e->data[bios_params.cur_offset++ % e->len];
> 
> Instead of using modular arithmetic, zero should be returned for invalid values.
> 
> >  +static void bios_cfg_write(void *opaque, uint32_t addr, uint32_t value)
> >  +{
> >  +    bios_params.entry = value % BIOS_CFG_MAX_ENTRY;
> 
> Same here, its important for downward compatibility.
> 
> >  +    bios_cfg_add_data(BIOS_CFG_SIGNATURE, "QEMU", 4);
> 
> I'd add:
> +    bios_cfg_add_data(BIOS_CFG_ID, 1, 4);
> 
Done.

--
			Gleb.

Re: [Qemu-devel] [PATCH 0/6] Add UUID command-line option

[Anthony Liguori]

Avi Kivity wrote:
> Blue Swirl wrote:
>> On 8/24/08, Avi Kivity <avi@qumranet.com> wrote:
>>   
>>
>> UUID can be used in all machines, so it would be nice if the interface
>> was same. I don't see much benefit in vmport compared to ROM solution.
>>
>>   
>
> Maybe UUID is generic but the vast majority of things that need to 
> communicate to qemu will be arch specific.

Because it's not an interface we control.  VMware automatically upgrades 
their tool chain and doesn't support backwards compatibility in their 
tools.  They're completely free to change the semantics of the interface.

This could really cause confusion within guests.  It's one thing to 
implement vmport based ops when there is existing guest software that we 
can get working but I think it's not a great idea to write new guest 
software that's QEMU specific using an interface that we have no control 
over.

Regards,

Anthony Liguori