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

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

[Gleb Natapov]

Resend the patch series after addressing most comments.

This patch series implements simple qemu<->bios communication channel
using IO port in patch 1. Patches 2-5 implement UUID support. Patch 6
implements CPU speed detection needed by SMBIOS.

---

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       |   21 ++++++++
 hw/pc.c         |  149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 monitor.c       |   11 ++++
 sysemu.h        |    2 +
 vl.c            |   42 ++++++++++++++++
 6 files changed, 229 insertions(+), 0 deletions(-)

-- 
        Gleb.

[Qemu-devel] [PATCH v2 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 |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 213ead8..8caa48f 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,26 @@ 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_ID,
+    BIOS_CFG_MAX_ENTRY
+} BIOSCfgEntryNum;
+
+typedef struct _BIOSCfgState {
+    BIOSCfgEntry entries[BIOS_CFG_MAX_ENTRY];
+    BIOSCfgEntryNum entry;
+    uint16_t cur_offset;
+} BIOSCfgState;
+
+static uint32_t bios_cfg_id = 1;
+static BIOSCfgState *bios_params;
+
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
 }
@@ -716,6 +737,36 @@ static void pc_init_ne2k_isa(NICInfo *nd, qemu_irq *pic)
     nb_ne2k++;
 }
 
+static uint32_t bios_cfg_read(void *opaque, uint32_t addr)
+{
+    BIOSCfgState *s = opaque;
+    BIOSCfgEntry *e = &s->entries[s->entry];
+
+    if (s->entry == BIOS_CFG_MAX_ENTRY || !e->data || s->cur_offset >= e->len)
+        return 0;
+
+    return e->data[s->cur_offset++];
+}
+
+static void bios_cfg_write(void *opaque, uint32_t addr, uint32_t value)
+{
+    BIOSCfgState *s = opaque;
+    s->entry = (value > BIOS_CFG_MAX_ENTRY) ? BIOS_CFG_MAX_ENTRY : value;
+    s->cur_offset = 0;
+}
+
+static int bios_cfg_add_data(BIOSCfgState *s, BIOSCfgEntryNum entry,
+        uint8_t *data, uint16_t len)
+{
+    if(entry >=  BIOS_CFG_MAX_ENTRY)
+        return 0;
+
+    s->entries[entry].data = data;
+    s->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 +922,14 @@ 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);
 
+    bios_params = qemu_mallocz(sizeof(BIOSCfgState));
+
+    register_ioport_read(BIOS_CFG_IOPORT, 1, 1, bios_cfg_read, bios_params);
+    register_ioport_write(BIOS_CFG_IOPORT, 1, 1, bios_cfg_write, bios_params);
+
+    bios_cfg_add_data(bios_params, BIOS_CFG_SIGNATURE, "QEMU", 4);
+    bios_cfg_add_data(bios_params, BIOS_CFG_ID, (uint8_t*)&bios_cfg_id, 4);
+
     bochs_bios_init();
 
     if (linux_boot)

[Qemu-devel] [PATCH v2 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 245177a..30fef2a 100644
--- a/vl.c
+++ b/vl.c
@@ -257,6 +257,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)
 
 /***********************************************************/
@@ -7710,6 +7712,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"
@@ -7904,6 +7907,7 @@ enum {
     QEMU_OPTION_startdate,
     QEMU_OPTION_tb_size,
     QEMU_OPTION_icount,
+    QEMU_OPTION_uuid,
 };
 
 typedef struct QEMUOption {
@@ -7992,6 +7996,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 },
@@ -8202,6 +8207,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
@@ -8780,6 +8802,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 v2 3/6] Add "info uuid" command to monitor.

[Gleb Natapov]

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

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

diff --git a/monitor.c b/monitor.c
index e71f49e..314c71e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -253,6 +253,15 @@ static void do_info_name(void)
         term_printf("%s\n", qemu_name);
 }
 
+static void do_info_uuid(void)
+{
+    term_printf(UUID_FMT "\n", 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]);
+}
+
 static void do_info_block(void)
 {
     bdrv_info();
@@ -1501,6 +1510,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 v2 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       |   21 +++++++++++++++++++++
 vl.c            |   13 +++++++++++++
 3 files changed, 38 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..fa46a64 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
@@ -780,6 +783,19 @@ EOF
 fi
 
 ##########################################
+# uuid library
+if test "$uuid" = "yes" ; then
+  uuid=no
+  cat > $TMPC << EOF
+#include <uuid/uuid.h>
+int main(void) { uuid_t u; return 0; }
+EOF
+  if $cc -o $TMPE $TMPC -luuid 2> /dev/null ; then
+    uuid=yes
+  fi
+fi
+
+##########################################
 # Sound support libraries probe
 
 audio_drv_probe()
@@ -961,6 +977,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 +1185,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 30fef2a..ea240d8 100644
--- a/vl.c
+++ b/vl.c
@@ -142,6 +142,11 @@ int inet_aton(const char *cp, struct in_addr *ia);
 
 #include "exec-all.h"
 
+#ifdef CONFIG_UUID
+#include <uuid/uuid.h>
+static int generate_uuid = 1;
+#endif
+
 #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
 #define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
 #ifdef __sun__
@@ -8808,6 +8813,9 @@ int main(int argc, char **argv)
                             " Wrong format.\n");
                     exit(1);
                 }
+#ifdef CONFIG_UUID
+                generate_uuid = 0;
+#endif
                 break;
 	    case QEMU_OPTION_daemonize:
 		daemonize = 1;
@@ -8908,6 +8916,11 @@ int main(int argc, char **argv)
            monitor_device = "stdio";
     }
 
+#if CONFIG_UUID
+    if (generate_uuid)
+        uuid_generate(qemu_uuid);
+#endif
+
 #ifndef _WIN32
     if (daemonize) {
 	pid_t pid;

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

[Gleb Natapov]

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

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

diff --git a/hw/pc.c b/hw/pc.c
index 8caa48f..15a86c5 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -62,6 +62,7 @@ typedef struct _BIOSCfgEntry {
 typedef enum _BIOSCfgEntryNum{
     BIOS_CFG_SIGNATURE,
     BIOS_CFG_ID,
+    BIOS_CFG_UUID,
     BIOS_CFG_MAX_ENTRY
 } BIOSCfgEntryNum;
 
@@ -929,6 +930,7 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 
     bios_cfg_add_data(bios_params, BIOS_CFG_SIGNATURE, "QEMU", 4);
     bios_cfg_add_data(bios_params, BIOS_CFG_ID, (uint8_t*)&bios_cfg_id, 4);
+    bios_cfg_add_data(bios_params, BIOS_CFG_UUID, qemu_uuid, 16);
 
     bochs_bios_init();
 

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

[Gleb Natapov]

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

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

diff --git a/hw/pc.c b/hw/pc.c
index 15a86c5..27e0939 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -63,6 +63,7 @@ typedef enum _BIOSCfgEntryNum{
     BIOS_CFG_SIGNATURE,
     BIOS_CFG_ID,
     BIOS_CFG_UUID,
+    BIOS_CFG_CPUSPEED,
     BIOS_CFG_MAX_ENTRY
 } BIOSCfgEntryNum;
 
@@ -75,6 +76,8 @@ typedef struct _BIOSCfgState {
 static uint32_t bios_cfg_id = 1;
 static BIOSCfgState *bios_params;
 
+static uint32_t cpu_speed;
+
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
 }
@@ -768,6 +771,89 @@ static int bios_cfg_add_data(BIOSCfgState *s, BIOSCfgEntryNum entry,
     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,
@@ -931,6 +1017,8 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     bios_cfg_add_data(bios_params, BIOS_CFG_SIGNATURE, "QEMU", 4);
     bios_cfg_add_data(bios_params, BIOS_CFG_ID, (uint8_t*)&bios_cfg_id, 4);
     bios_cfg_add_data(bios_params, BIOS_CFG_UUID, qemu_uuid, 16);
+    cpu_speed = (uint32_t)get_freq(1, 1);
+    bios_cfg_add_data(bios_params, BIOS_CFG_CPUSPEED, (uint8_t*)&cpu_speed, 4);
 
     bochs_bios_init();
 

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

[Andreas Färber]

Am 25.08.2008 um 11:58 schrieb 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>
> ---

I don't remember hearing an answer on why this is necessary. For which  
use case can't you just either use -uuid `uuidgen` or a hardcoded  
default value like the Slirp IPv4 subnet?

With your patch, not specifying a UUID would generate new random UUIDs  
even for a single disk image, which doesn't sound good (think of  
placing a Windows volume into a different PC on each boot) and is  
inconsistent with previous QEMU policies. We're not talking about a  
rare corner case here - it's a new option, so all legacy scripts and  
setups would be affected by this non-deterministic behavioral change.

In case you didn't find the thread I referenced earlier, here's a link:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg15809.html

Andreas

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 01:08:50PM +0200, Andreas Färber wrote:
>
> Am 25.08.2008 um 11:58 schrieb 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>
>> ---
>
> I don't remember hearing an answer on why this is necessary. For which  
> use case can't you just either use -uuid `uuidgen` or a hardcoded  
> default value like the Slirp IPv4 subnet?
It can be used for UUID generation when VM is started for the first time.
Management application can retrieve UUID using monitor and use it for
consequent runs. But the same result can be achieved in a different way
too. So no, I really don't have a strong use case for this feature, but
the patch set is organised in such way that it is possible to ignore
this particular patch.

--
			Gleb.

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

[Jamie Lokier]

Gleb Natapov wrote:
> > I don't remember hearing an answer on why this is necessary. For which  
> > use case can't you just either use -uuid `uuidgen` or a hardcoded  
> > default value like the Slirp IPv4 subnet?
> It can be used for UUID generation when VM is started for the first time.
> Management application can retrieve UUID using monitor and use it for
> consequent runs. But the same result can be achieved in a different way
> too. So no, I really don't have a strong use case for this feature, but
> the patch set is organised in such way that it is possible to ignore
> this particular patch.

Yeah, but Management Application is often not used.

QEMU's philosophy seems to be that it should be quite usable from the
command line, and leaving _off_ options should give sane defaults.

My immediate thought is that UUID would make sense at disk image
generation time - i.e. stored in qcow2 when qemu-img runs - until such
time as QEMU has a useful config/state file format.  Then it should go
in that file, just like all other VM systems.

-- Jamie

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

[Andreas Färber]

Am 25.08.2008 um 13:26 schrieb Gleb Natapov:

> On Mon, Aug 25, 2008 at 01:08:50PM +0200, Andreas Färber wrote:
>>
>> Am 25.08.2008 um 11:58 schrieb 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>
>>> ---
>>
>> I don't remember hearing an answer on why this is necessary. For  
>> which
>> use case can't you just either use -uuid `uuidgen` or a hardcoded
>> default value like the Slirp IPv4 subnet?
> It can be used for UUID generation when VM is started for the first  
> time.
> Management application can retrieve UUID using monitor and use it for
> consequent runs. But the same result can be achieved in a different  
> way
> too. So no, I really don't have a strong use case for this feature,  
> but
> the patch set is organised in such way that it is possible to ignore
> this particular patch.

Okay, then I'd ask to invert the logic so that the user has to  
explicitly ask for it, maybe -uuid random (setting generate_uuid = 0  
by default and =1 in that case). Then no accidental damage should be  
done and a Management Application can still use it.

Andreas

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 01:45:53PM +0200, Andreas Färber wrote:
>>> I don't remember hearing an answer on why this is necessary. For  
>>> which
>>> use case can't you just either use -uuid `uuidgen` or a hardcoded
>>> default value like the Slirp IPv4 subnet?
>> It can be used for UUID generation when VM is started for the first  
>> time.
>> Management application can retrieve UUID using monitor and use it for
>> consequent runs. But the same result can be achieved in a different  
>> way
>> too. So no, I really don't have a strong use case for this feature,  
>> but
>> the patch set is organised in such way that it is possible to ignore
>> this particular patch.
>
> Okay, then I'd ask to invert the logic so that the user has to  
> explicitly ask for it, maybe -uuid random (setting generate_uuid = 0 by 
> default and =1 in that case). Then no accidental damage should be done 
> and a Management Application can still use it.
>
Something like this?

---
    Use libuuid if available.
    
    If libuuid is available use it for UUID generation in case a user
    asks for it.
    
    Signed-off-by: Gleb Natapov <gleb@qumranet.com>

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..fa46a64 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
@@ -780,6 +783,19 @@ EOF
 fi
 
 ##########################################
+# uuid library
+if test "$uuid" = "yes" ; then
+  uuid=no
+  cat > $TMPC << EOF
+#include <uuid/uuid.h>
+int main(void) { uuid_t u; return 0; }
+EOF
+  if $cc -o $TMPE $TMPC -luuid 2> /dev/null ; then
+    uuid=yes
+  fi
+fi
+
+##########################################
 # Sound support libraries probe
 
 audio_drv_probe()
@@ -961,6 +977,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 +1185,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 30fef2a..655dd3c 100644
--- a/vl.c
+++ b/vl.c
@@ -142,6 +142,11 @@ int inet_aton(const char *cp, struct in_addr *ia);
 
 #include "exec-all.h"
 
+#ifdef CONFIG_UUID
+#include <uuid/uuid.h>
+static int generate_uuid;
+#endif
+
 #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
 #define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
 #ifdef __sun__
@@ -8803,6 +8808,12 @@ int main(int argc, char **argv)
                 cursor_hide = 0;
                 break;
             case QEMU_OPTION_uuid:
+#ifdef CONFIG_UUID
+                if (strcmp(optarg, "gen") == 0) {
+                    generate_uuid = 1;
+                    break;
+                }
+#endif
                 if(qemu_uuid_parse(optarg, qemu_uuid) < 0) {
                     fprintf(stderr, "Fail to parse UUID string."
                             " Wrong format.\n");
@@ -8908,6 +8919,11 @@ int main(int argc, char **argv)
            monitor_device = "stdio";
     }
 
+#if CONFIG_UUID
+    if (generate_uuid)
+        uuid_generate(qemu_uuid);
+#endif
+
 #ifndef _WIN32
     if (daemonize) {
 	pid_t pid;
--
			Gleb.

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

[Anthony Liguori]

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

This is the incorrect frequency for a QEMU guest.  I'm not sure why this 
is included.  What does passing the frequency give us?

Regards,

Anthony Liguori

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

[Anthony Liguori]

Gleb Natapov wrote:
> Use PIO to get configuration info between qemu process and guest BIOS.
>
> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
> ---
>
>  hw/pc.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 213ead8..8caa48f 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,26 @@ static PITState *pit;
>  static IOAPICState *ioapic;
>  static PCIDevice *i440fx_state;
>  
> +typedef struct _BIOSCfgEntry {
> +    uint16_t len;
> +    uint8_t *data;
> +} BIOSCfgEntry;
>   

So why aren't we using the ROM mechanism suggested by Blue Swirl?  
Unless we're switching other firmwares to use this mechanism, I think it 
would be better to have a single QEMU<->firmware interface that we could 
support for all architectures.

Regards,

Anthony Liguori

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

[Anthony Liguori]

Gleb Natapov wrote:
> Use PIO to get configuration info between qemu process and guest BIOS.
>
> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
> ---
>
>  hw/pc.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 213ead8..8caa48f 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
>   

I don't think this is a very safe IO port to use.  Avi suggested that we 
advertise the IO port that we use as reserved in the ACPI tables.

I would have chosen something in the low 400/500 range since Bochs is 
already using this range for debugging.

Regards,

Anthony Liguori

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 09:17:31AM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote:
>> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
>>   
>
> This is the incorrect frequency for a QEMU guest.  I'm not sure why this  
> is included.  What does passing the frequency give us?
>
This is included for two reasons. First one is to demonstrate that
proposed interface is extensible. Second is that Micrisoft SVVP test 
requires this info (and much more) to be set in SMBIOS tables. Using
real HW value seams to be better than just provide some default one.
Perhaps scale it a little bit before passing to SMBIOS?

--
			Gleb.

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 09:24:33AM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote:
>> Use PIO to get configuration info between qemu process and guest BIOS.
>>
>> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
>> ---
>>
>>  hw/pc.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 59 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 213ead8..8caa48f 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,26 @@ static PITState *pit;
>>  static IOAPICState *ioapic;
>>  static PCIDevice *i440fx_state;
>>  +typedef struct _BIOSCfgEntry {
>> +    uint16_t len;
>> +    uint8_t *data;
>> +} BIOSCfgEntry;
>>   
>
> So why aren't we using the ROM mechanism suggested by Blue Swirl?   
> Unless we're switching other firmwares to use this mechanism, I think it  
> would be better to have a single QEMU<->firmware interface that we could  
> support for all architectures.
>
There was a long discussion about this. Have you read it already? My reasoning is
that firmware structure mostly provides information that PC bios doesn't
need and don't provides info that PC bios needs. Nobody showed what is
the benefit of using firmware interface for communicating with PC bios
yet. Because firmware interface contains mostly unneeded info there is
no point in copying the whole structure into the bios, only specific
fields will be copied and to do that bios will have to know magic value
(offset of the field). So just instead of pretending we are using firmware
interface we can simply define magic values for each parameter we want
to pass from qemu to bios and use them instead of structure offsets.

Note that all current users of firmware ABI uses memory mapped access to
firmware structure and you proposed to use port IO for PC.

--
			Gleb.

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 09:25:55AM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote:
>> Use PIO to get configuration info between qemu process and guest BIOS.
>>
>> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
>> ---
>>
>>  hw/pc.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 59 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 213ead8..8caa48f 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
>>   
>
> I don't think this is a very safe IO port to use.  Avi suggested that we  
> advertise the IO port that we use as reserved in the ACPI tables.
>
The port number here is just place holder. Any idea of what port we
should use are welcome. ACPI changes should go into bochs tree and not
included in this series.
 
> I would have chosen something in the low 400/500 range since Bochs is  
> already using this range for debugging.
>
What about 410?

--
			Gleb.

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

[Blue Swirl]

On 8/25/08, Gleb Natapov <gleb@qumranet.com> wrote:
> On Mon, Aug 25, 2008 at 09:24:33AM -0500, Anthony Liguori wrote:
>  > Gleb Natapov wrote:
>  >> Use PIO to get configuration info between qemu process and guest BIOS.
>  >>
>  >> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
>  >> ---
>  >>
>  >>  hw/pc.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  >>  1 files changed, 59 insertions(+), 0 deletions(-)
>  >>
>  >> diff --git a/hw/pc.c b/hw/pc.c
>  >> index 213ead8..8caa48f 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,26 @@ static PITState *pit;
>  >>  static IOAPICState *ioapic;
>  >>  static PCIDevice *i440fx_state;
>  >>  +typedef struct _BIOSCfgEntry {
>  >> +    uint16_t len;
>  >> +    uint8_t *data;
>  >> +} BIOSCfgEntry;
>  >>
>  >
>  > So why aren't we using the ROM mechanism suggested by Blue Swirl?
>  > Unless we're switching other firmwares to use this mechanism, I think it
>  > would be better to have a single QEMU<->firmware interface that we could
>  > support for all architectures.
>  >
>
> There was a long discussion about this. Have you read it already? My reasoning is
>  that firmware structure mostly provides information that PC bios doesn't
>  need and don't provides info that PC bios needs. Nobody showed what is
>  the benefit of using firmware interface for communicating with PC bios
>  yet. Because firmware interface contains mostly unneeded info there is
>  no point in copying the whole structure into the bios, only specific
>  fields will be copied and to do that bios will have to know magic value
>  (offset of the field). So just instead of pretending we are using firmware
>  interface we can simply define magic values for each parameter we want
>  to pass from qemu to bios and use them instead of structure offsets.

This has the advantage that the sizes of the fields are not fixed by
the structure layout. On the other hand, same can be achieved with ROM
by using an index, which lists the offsets and sizes in the beginning
of the ROM for each magic parameter. If some parameter is not used
(for that architecture or because a newer version has suppressed it),
size and index can be zero.

A more important benefit is in my view that the whole protocol can be
extended, whereas a ROM will always be a ROM. But then taking a
security angle, that could be an advantage too. I don't know which is
more important.

>  Note that all current users of firmware ABI uses memory mapped access to
>  firmware structure and you proposed to use port IO for PC.

On other machines, the IO port would be replaced with MMIO if we want
to use the same mechanism.

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

[Jamie Lokier]

Gleb Natapov wrote:
> >> +#define BIOS_CFG_IOPORT 0x1234
> >>   
> >
> > I don't think this is a very safe IO port to use.  Avi suggested that we  
> > advertise the IO port that we use as reserved in the ACPI tables.
> >
> The port number here is just place holder. Any idea of what port we
> should use are welcome. ACPI changes should go into bochs tree and not
> included in this series.
>  
> > I would have chosen something in the low 400/500 range since Bochs is  
> > already using this range for debugging.
> >
> What about 410?

Whatever port is chosen, it should be one which common and rare
operating systems, recent and ancient, don't "probe" to look for
non-PNP ISA devices - unless the protocol requires a magic sequence to
be woken (and goes back to sleep following a CPU reset).

I quick Google suggests 0x410 is most commonly used by PCMCIA ethernet
cards.  I've no idea if they are probed using that port.

-- Jamie

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

[Anthony Liguori]

Blue Swirl wrote:
> On 8/25/08, Gleb Natapov <gleb@qumranet.com> wrote:
>>
>> There was a long discussion about this. Have you read it already?

Yes, but it wasn't clear to me that there was a consensus in that 
discussion.  Since the code isn't structured for reuse either, that 
raised a flag.  I think it would be pretty useful to have a standard 
mechanism that worked across architectures to query QEMU-specific 
information.

>>  My reasoning is
>>  that firmware structure mostly provides information that PC bios doesn't
>>  need and don't provides info that PC bios needs. Nobody showed what is
>>  the benefit of using firmware interface for communicating with PC bios
>>  yet. Because firmware interface contains mostly unneeded info there is
>>  no point in copying the whole structure into the bios, only specific
>>  fields will be copied and to do that bios will have to know magic value
>>  (offset of the field). So just instead of pretending we are using firmware
>>  interface we can simply define magic values for each parameter we want
>>  to pass from qemu to bios and use them instead of structure offsets.
>>     
>
> This has the advantage that the sizes of the fields are not fixed by
> the structure layout. On the other hand, same can be achieved with ROM
> by using an index, which lists the offsets and sizes in the beginning
> of the ROM for each magic parameter. If some parameter is not used
> (for that architecture or because a newer version has suppressed it),
> size and index can be zero.
>
> A more important benefit is in my view that the whole protocol can be
> extended, whereas a ROM will always be a ROM. But then taking a
> security angle, that could be an advantage too. I don't know which is
> more important.
>   

Blue Swirl: What do you think of switching sparc to use a structure more 
like this?  I do prefer a key-value mechanism verses a blob.  Even with 
pure MMIO, the same results could be achieved by treating the MMIO 
region as registers and using a selector.

Regards,

Anthony Liguori

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

[Blue Swirl]

On 8/25/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Blue Swirl wrote:
>
> > On 8/25/08, Gleb Natapov <gleb@qumranet.com> wrote:
> >
> > >
> > > There was a long discussion about this. Have you read it already?
> > >
> >
>
>  Yes, but it wasn't clear to me that there was a consensus in that
> discussion.  Since the code isn't structured for reuse either, that raised a
> flag.  I think it would be pretty useful to have a standard mechanism that
> worked across architectures to query QEMU-specific information.
>
>
> >
> > >  My reasoning is
> > >  that firmware structure mostly provides information that PC bios
> doesn't
> > >  need and don't provides info that PC bios needs. Nobody showed what is
> > >  the benefit of using firmware interface for communicating with PC bios
> > >  yet. Because firmware interface contains mostly unneeded info there is
> > >  no point in copying the whole structure into the bios, only specific
> > >  fields will be copied and to do that bios will have to know magic value
> > >  (offset of the field). So just instead of pretending we are using
> firmware
> > >  interface we can simply define magic values for each parameter we want
> > >  to pass from qemu to bios and use them instead of structure offsets.
> > >
> > >
> >
> > This has the advantage that the sizes of the fields are not fixed by
> > the structure layout. On the other hand, same can be achieved with ROM
> > by using an index, which lists the offsets and sizes in the beginning
> > of the ROM for each magic parameter. If some parameter is not used
> > (for that architecture or because a newer version has suppressed it),
> > size and index can be zero.
> >
> > A more important benefit is in my view that the whole protocol can be
> > extended, whereas a ROM will always be a ROM. But then taking a
> > security angle, that could be an advantage too. I don't know which is
> > more important.
> >
> >
>
>  Blue Swirl: What do you think of switching sparc to use a structure more
> like this?  I do prefer a key-value mechanism verses a blob.  Even with pure
> MMIO, the same results could be achieved by treating the MMIO region as
> registers and using a selector.

I could switch Sparc to something like this, if the goal is that it
will be used by all targets.

There should be a .h file which lists the keys and which can be
included from C and asm, like current firmware_abi.h. I'd define an
offset (0x8000?) for architecture-specific keys which need not be in
the same .h file.

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

[Sebastian Herbszt]

Jamie Lokier wrote:
>> What about 410?
> 
> Whatever port is chosen, it should be one which common and rare
> operating systems, recent and ancient, don't "probe" to look for
> non-PNP ISA devices - unless the protocol requires a magic sequence to
> be woken (and goes back to sleep following a CPU reset).
> 
> I quick Google suggests 0x410 is most commonly used by PCMCIA ethernet
> cards.  I've no idea if they are probed using that port.

Ralf Brown's "Ports List, part 2 of 3" says
"PORT 040A-043F - Intel 82378ZB embedded DMA controller".

- Sebastian

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

[Sebastian Herbszt]

Andreas Färber wrote:

> Okay, then I'd ask to invert the logic so that the user has to  explicitly ask for it, maybe -uuid random (setting generate_uuid = 
> 0  by default and =1 in that case). Then no accidental damage should be  done and a Management Application can still use it.

If qemu would have HAS_OPT_ARG beside HAS_ARG i think it
would be nice to use "-uuid UUID_HERE" to supply one and just
"-uuid" (without argument) to request a random one.

- Sebastian

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

[Sebastian Herbszt]

Gleb Natapov wrote:

<snip>
> +#define CPUFREQ_PREFIX "/sys/devices/system/cpu"
<snip>>
> + f = fopen("/proc/cpuinfo", "r");
<snip>

How stable are the values provided by those files?
AFAIK the data from /proc/cpuinfo does change if cpufreq is used.
Could this somehow "hurt" the VM, e.g. by SMBIOS table value
possibly changing on consecutive qemu starts?

- Sebastian

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

[Sebastian Herbszt]

Gleb Natapov wrote:

>> This is the incorrect frequency for a QEMU guest.  I'm not sure why this  
>> is included.  What does passing the frequency give us?
>>
> This is included for two reasons. First one is to demonstrate that
> proposed interface is extensible. Second is that Micrisoft SVVP test 
> requires this info (and much more) to be set in SMBIOS tables.

Can you elaborate a bit on the "and much more" part? Is this the stuff
described in "SMBIOS "Designed for Windows" Logo Requirements" at
http://www.microsoft.com/whdc/system/platform/firmware/SMBIOS.mspx ?

> Using real HW value seams to be better than just provide some default one.

What downside does using a default value have? On the upside this patch would
not be needed.

- Sebastian

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 09:26:56PM +0200, Sebastian Herbszt wrote:
> Gleb Natapov wrote:
>
>>> This is the incorrect frequency for a QEMU guest.  I'm not sure why 
>>> this  is included.  What does passing the frequency give us?
>>>
>> This is included for two reasons. First one is to demonstrate that
>> proposed interface is extensible. Second is that Micrisoft SVVP test  
>> requires this info (and much more) to be set in SMBIOS tables.
>
> Can you elaborate a bit on the "and much more" part? Is this the stuff
> described in "SMBIOS "Designed for Windows" Logo Requirements" at
> http://www.microsoft.com/whdc/system/platform/firmware/SMBIOS.mspx ?
>
I think yes. There is Microsoft test suit that test SMBIOS
implementation. Running it on bochs bios results in a couple of errors.
Some fields cannot be empty. Some bits should be set. Most things can be
fixed without qemu help, but for CPU speed we need to ask qemu.

>> Using real HW value seams to be better than just provide some default one.
>
> What downside does using a default value have? On the upside this patch would
> not be needed.
>
>
What value do you propose? Without knowing how Windows uses it we can't
say what are the downsides.

--
			Gleb.

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 09:15:51PM +0200, Sebastian Herbszt wrote:
> Gleb Natapov wrote:
>
> <snip>
>> +#define CPUFREQ_PREFIX "/sys/devices/system/cpu"
> <snip>>
>> + f = fopen("/proc/cpuinfo", "r");
> <snip>
>
> How stable are the values provided by those files?
> AFAIK the data from /proc/cpuinfo does change if cpufreq is used.
Looking at the code it is indeed how it works.

> Could this somehow "hurt" the VM, e.g. by SMBIOS table value
> possibly changing on consecutive qemu starts?
>
Don't know, but on one of my PCs SMBIOS value for CPU speed is greater
then /proc/cpuinfo value.

--
			Gleb.

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 08:53:05PM +0200, Sebastian Herbszt wrote:
> Jamie Lokier wrote:
>>> What about 410?
>>
>> Whatever port is chosen, it should be one which common and rare
>> operating systems, recent and ancient, don't "probe" to look for
>> non-PNP ISA devices - unless the protocol requires a magic sequence to
>> be woken (and goes back to sleep following a CPU reset).
>>
>> I quick Google suggests 0x410 is most commonly used by PCMCIA ethernet
>> cards.  I've no idea if they are probed using that port.
>
> Ralf Brown's "Ports List, part 2 of 3" says
> "PORT 040A-043F - Intel 82378ZB embedded DMA controller".
>
It lists ports 0x401/0x403 as part of EISA DMA Controller, but they are
used by qemu for BIOS debug. What about port 0x510? It is not listed
there (but on my desktop 0x500-0x53f are used by ISA bridge 82801IH).
Perhaps choose some high port (0x6666 for instance)?

--
			Gleb.

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

[Gleb Natapov]

On Mon, Aug 25, 2008 at 09:27:18PM +0300, Blue Swirl wrote:
> >  Blue Swirl: What do you think of switching sparc to use a structure more
> > like this?  I do prefer a key-value mechanism verses a blob.  Even with pure
> > MMIO, the same results could be achieved by treating the MMIO region as
> > registers and using a selector.
> 
> I could switch Sparc to something like this, if the goal is that it
> will be used by all targets.
> 
> There should be a .h file which lists the keys and which can be
> included from C and asm, like current firmware_abi.h. I'd define an
> offset (0x8000?) for architecture-specific keys which need not be in
> the same .h file.

Is the patch below what you mean? (not tested, but compiles)


---

    Use IO port for qemu<->guest BIOS communication.
    
    Use PIO to get configuration info between qemu process and guest BIOS.
    
    Signed-off-by: Gleb Natapov <gleb@qumranet.com>

diff --git a/hw/fw_channel.h b/hw/fw_channel.h
new file mode 100644
index 0000000..3d44af1
--- /dev/null
+++ b/hw/fw_channel.h
@@ -0,0 +1,68 @@
+#ifndef FW_CHANNEL_H
+#define FW_CHANNEL_H
+
+#define FW_CFG_SIGNATURE        0x00
+#define FW_CFG_ID               0x01
+#define FW_CFG_MAX_ENTRY        0x10
+
+#define FW_CFG_ARCH_LOCAL       0x8000
+
+#ifndef __ASSEMBLY__
+
+typedef struct _FWCfgEntry {
+    uint16_t len;
+    uint8_t *data;
+} FWCfgEntry;
+
+typedef struct _FWCfgState {
+    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
+    FWCfgEntry *cur_entry;
+    uint16_t cur_offset;
+} FWCfgState;
+
+static inline int firmware_cfg_add(FWCfgState *s, uint16_t key,
+        uint8_t *data, uint16_t len)
+{
+    int arch = key & FW_CFG_ARCH_LOCAL;
+
+    key &= (~FW_CFG_ARCH_LOCAL);
+
+    if (key >= FW_CFG_MAX_ENTRY)
+        return 0;
+
+    s->entries[arch][key].data = data;
+    s->entries[arch][key].len = len;
+
+    return 1;
+}
+
+static inline int firmware_cfg_select(FWCfgState *s, uint16_t key)
+{
+    int arch = key & FW_CFG_ARCH_LOCAL;
+
+    key &= (~FW_CFG_ARCH_LOCAL);
+
+    s->cur_offset = 0;
+    if (key >= FW_CFG_MAX_ENTRY) {
+        s->cur_entry = NULL;
+        return 0;
+    }
+
+    s->cur_entry = &s->entries[arch][key];
+
+    return 1;
+}
+
+static inline uint8_t firmware_cfg_read(FWCfgState *s)
+{
+    FWCfgEntry *e = s->cur_entry;
+
+    if (!e || !e->data || s->cur_offset >= e->len)
+        return 0;
+
+    return e->data[s->cur_offset++];
+}
+
+#endif /* __ASSEMBLY__ */
+
+#endif
diff --git a/hw/pc.c b/hw/pc.c
index 213ead8..db5089b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -32,6 +32,7 @@
 #include "smbus.h"
 #include "boards.h"
 #include "console.h"
+#include "hw/fw_channel.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -44,6 +45,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 0x510
 
 #define MAX_IDE_BUS 2
 
@@ -53,6 +55,9 @@ static PITState *pit;
 static IOAPICState *ioapic;
 static PCIDevice *i440fx_state;
 
+static uint32_t bios_cfg_id = 1;
+static FWCfgState *bios_params;
+
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
 }
@@ -367,6 +372,21 @@ static uint32_t ioport92_read(void *opaque, uint32_t addr)
     return ioport_get_a20() << 1;
 }
 
+static uint32_t bios_cfg_read(void *opaque, uint32_t addr)
+{
+    FWCfgState *s = opaque;
+
+    return firmware_cfg_read(s);
+}
+
+static void bios_cfg_write(void *opaque, uint32_t addr, uint32_t value)
+{
+    FWCfgState *s = opaque;
+
+    firmware_cfg_select(s, (uint16_t)value);
+}
+
+
 /***********************************************************/
 /* Bochs BIOS debug ports */
 
@@ -426,6 +446,14 @@ static void bochs_bios_init(void)
     register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
     register_ioport_write(0x500, 1, 1, bochs_bios_write, NULL);
     register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
+
+    bios_params = qemu_mallocz(sizeof(FWCfgState));
+
+    register_ioport_read(BIOS_CFG_IOPORT, 1, 1, bios_cfg_read, bios_params);
+    register_ioport_write(BIOS_CFG_IOPORT, 1, 1, bios_cfg_write, bios_params);
+
+    firmware_cfg_add(bios_params, FW_CFG_SIGNATURE, "QEMU", 4);
+    firmware_cfg_add(bios_params, FW_CFG_ID, (uint8_t*)&bios_cfg_id, 4);
 }
 
 /* Generate an initial boot sector which sets state and jump to
--
			Gleb.

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

[Blue Swirl]

On 8/26/08, Gleb Natapov <gleb@qumranet.com> wrote:
> On Mon, Aug 25, 2008 at 09:27:18PM +0300, Blue Swirl wrote:
>  > >  Blue Swirl: What do you think of switching sparc to use a structure more
>  > > like this?  I do prefer a key-value mechanism verses a blob.  Even with pure
>  > > MMIO, the same results could be achieved by treating the MMIO region as
>  > > registers and using a selector.
>  >
>  > I could switch Sparc to something like this, if the goal is that it
>  > will be used by all targets.
>  >
>  > There should be a .h file which lists the keys and which can be
>  > included from C and asm, like current firmware_abi.h. I'd define an
>  > offset (0x8000?) for architecture-specific keys which need not be in
>  > the same .h file.
>
>
> Is the patch below what you mean? (not tested, but compiles)

Yes, but I'd still put the code from the .h file and pc.c to a new .c
file, only the keys and function prototypes to .h.

> +    int arch = key & FW_CFG_ARCH_LOCAL;
>  +
>  +    key &= (~FW_CFG_ARCH_LOCAL);
>  +
>  +    if (key >= FW_CFG_MAX_ENTRY)
>  +        return 0;

I was actually not thinking to use the arch part like this, but in
your solution, arch part storage is handled same way as others. Great!

> +static FWCfgState *bios_params;

Now that the _read and _write functions use the opaque argument, this
is not needed once allocated.

>  +static uint32_t bios_cfg_read(void *opaque, uint32_t addr)
>  +{
>
> +    FWCfgState *s = opaque;
>  +
>  +    return firmware_cfg_read(s);
>
> +}
>  +
>  +static void bios_cfg_write(void *opaque, uint32_t addr, uint32_t value)
>  +{
>
> +    FWCfgState *s = opaque;
>  +
>  +    firmware_cfg_select(s, (uint16_t)value);
>  +}
>  +

I'd put these to the new .c file.

One problem with this IO method is that it is not SMP safe, because of
the cur_entry selection and reading will not be atomic. Think of boot
CPU waking up all other CPUs with broadcast interrupt and then these
other CPUs all want to know the boot parameters at the same time. The
same could happen with warm reset. Directly mapped ROM doesn't have
this problem, but multiplexed ROM (or Sparc64 nvram used now) is also
unsafe.

I could think of a couple of solutions:
- different IO port for each CPU (not possible on PC)
- per-CPU cur_entry, the device "knows" the CPU index (not very beautiful)
- lock mechanism: before access, the CPU must win lock and only then
it can access the data. But IO ports don't support atomic
instructions? How about some kind of MAC-like protocol? It gets pretty
complex.

Are there other solutions?

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

[Avi Kivity]

Blue Swirl wrote:
>
> One problem with this IO method is that it is not SMP safe, because of
> the cur_entry selection and reading will not be atomic. Think of boot
> CPU waking up all other CPUs with broadcast interrupt and then these
> other CPUs all want to know the boot parameters at the same time. The
> same could happen with warm reset. Directly mapped ROM doesn't have
> this problem, but multiplexed ROM (or Sparc64 nvram used now) is also
> unsafe.
>
> I could think of a couple of solutions:
> - different IO port for each CPU (not possible on PC)
> - per-CPU cur_entry, the device "knows" the CPU index (not very beautiful)
> - lock mechanism: before access, the CPU must win lock and only then
> it can access the data. But IO ports don't support atomic
> instructions? How about some kind of MAC-like protocol? It gets pretty
> complex.
>
> Are there other solutions?
>
>   

Put the lock in memory.  Or even simpler, have the boot cpu extract the
parameters and place them in memory, and then wake up other cpus.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Blue Swirl]

On 8/26/08, Avi Kivity <avi@qumranet.com> wrote:
> Blue Swirl wrote:
>  >
>  > One problem with this IO method is that it is not SMP safe, because of
>  > the cur_entry selection and reading will not be atomic. Think of boot
>  > CPU waking up all other CPUs with broadcast interrupt and then these
>  > other CPUs all want to know the boot parameters at the same time. The
>  > same could happen with warm reset. Directly mapped ROM doesn't have
>  > this problem, but multiplexed ROM (or Sparc64 nvram used now) is also
>  > unsafe.
>  >
>  > I could think of a couple of solutions:
>  > - different IO port for each CPU (not possible on PC)
>  > - per-CPU cur_entry, the device "knows" the CPU index (not very beautiful)
>  > - lock mechanism: before access, the CPU must win lock and only then
>  > it can access the data. But IO ports don't support atomic
>  > instructions? How about some kind of MAC-like protocol? It gets pretty
>  > complex.
>  >
>  > Are there other solutions?
>  >
>  >
>
>
> Put the lock in memory.  Or even simpler, have the boot cpu extract the
>  parameters and place them in memory, and then wake up other cpus.

Where in the memory? What about reset, when all CPUs are online? And
CPUs would need to know if they are the boot CPU or not without
disturbing our IO port.

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

[Avi Kivity]

Blue Swirl wrote:
>>
>> Put the lock in memory.  Or even simpler, have the boot cpu extract the
>>  parameters and place them in memory, and then wake up other cpus.
>>     
>
> Where in the memory?

Anywhere you like.  Of course that doesn't work if you need the 
information to get memory to work.

>  What about reset, when all CPUs are online? And
> CPUs would need to know if they are the boot CPU or not without
> disturbing our IO port.
>   

On x86, reset has once cpu running and the rest in a holding pattern.

If your arch has all cpus running on reset, and you can't determine 
which is cpu 0, you can construct a lock device using mmio which doesn't 
require any atomic ops.

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

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

[Gleb Natapov]

On Tue, Aug 26, 2008 at 07:46:20PM +0300, Blue Swirl wrote:
> > Is the patch below what you mean? (not tested, but compiles)
> 
> Yes, but I'd still put the code from the .h file and pc.c to a new .c
> file, only the keys and function prototypes to .h.
> 
Okey, here is updated one (compiled only)

---
    Use IO port for qemu<->guest BIOS communication.
    
    Use PIO to get configuration info between qemu process and guest BIOS.
    
    Signed-off-by: Gleb Natapov <gleb@qumranet.com>

diff --git a/Makefile.target b/Makefile.target
index 2464484..4d7a1ef 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -537,7 +537,7 @@ OBJS += e1000.o
 ifeq ($(TARGET_BASE_ARCH), i386)
 # Hardware support
 OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o
-OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
+OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o fw_channel.o
 OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
 OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
diff --git a/hw/fw_channel.c b/hw/fw_channel.c
new file mode 100644
index 0000000..581a83c
--- /dev/null
+++ b/hw/fw_channel.c
@@ -0,0 +1,54 @@
+#include "hw.h"
+#include <hw/fw_channel.h>
+
+int firmware_cfg_add(FWCfgState *s, uint16_t key, uint8_t *data, uint16_t len)
+{
+    int arch = !!(key & FW_CFG_ARCH_LOCAL);
+
+    key &= (~FW_CFG_ARCH_LOCAL);
+
+    if (key >= FW_CFG_MAX_ENTRY)
+        return 0;
+
+    s->entries[arch][key].data = data;
+    s->entries[arch][key].len = len;
+
+    return 1;
+}
+
+int firmware_cfg_select(FWCfgState *s, uint16_t key)
+{
+    int arch = !!(key & FW_CFG_ARCH_LOCAL);
+
+    key &= (~FW_CFG_ARCH_LOCAL);
+
+    s->cur_offset = 0;
+    if (key >= FW_CFG_MAX_ENTRY) {
+        s->cur_entry = NULL;
+        return 0;
+    }
+
+    s->cur_entry = &s->entries[arch][key];
+
+    return 1;
+}
+
+uint8_t firmware_cfg_read(FWCfgState *s)
+{
+    FWCfgEntry *e = s->cur_entry;
+
+    if (!e || !e->data || s->cur_offset >= e->len)
+        return 0;
+
+    return e->data[s->cur_offset++];
+}
+
+uint32_t firmware_io_cfg_read(void *opaque, uint32_t addr)
+{
+    return firmware_cfg_read(opaque);
+}
+
+void firmware_io_cfg_write(void *opaque, uint32_t addr, uint32_t value)
+{
+    firmware_cfg_select(opaque, (uint16_t)value);
+}
diff --git a/hw/fw_channel.h b/hw/fw_channel.h
new file mode 100644
index 0000000..841c1dd
--- /dev/null
+++ b/hw/fw_channel.h
@@ -0,0 +1,32 @@
+#ifndef FW_CHANNEL_H
+#define FW_CHANNEL_H
+
+#define FW_CFG_SIGNATURE        0x00
+#define FW_CFG_ID               0x01
+#define FW_CFG_MAX_ENTRY        0x10
+
+#define FW_CFG_ARCH_LOCAL       0x8000
+
+#ifndef __ASSEMBLY__
+
+typedef struct _FWCfgEntry {
+    uint16_t len;
+    uint8_t *data;
+} FWCfgEntry;
+
+typedef struct _FWCfgState {
+    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
+    FWCfgEntry *cur_entry;
+    uint16_t cur_offset;
+} FWCfgState;
+
+int firmware_cfg_add(FWCfgState *s, uint16_t key, uint8_t *data, uint16_t len);
+int firmware_cfg_select(FWCfgState *s, uint16_t key);
+uint8_t firmware_cfg_read(FWCfgState *s);
+
+uint32_t firmware_io_cfg_read(void *opaque, uint32_t addr);
+void firmware_io_cfg_write(void *opaque, uint32_t addr, uint32_t value);
+
+#endif /* __ASSEMBLY__ */
+
+#endif
diff --git a/hw/pc.c b/hw/pc.c
index 213ead8..f7deab5 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -32,6 +32,7 @@
 #include "smbus.h"
 #include "boards.h"
 #include "console.h"
+#include "hw/fw_channel.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -44,6 +45,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 0x510
 
 #define MAX_IDE_BUS 2
 
@@ -53,6 +55,8 @@ static PITState *pit;
 static IOAPICState *ioapic;
 static PCIDevice *i440fx_state;
 
+static uint32_t bios_cfg_id = 1;
+
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
 }
@@ -416,6 +420,8 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
 
 static void bochs_bios_init(void)
 {
+    FWCfgState *bios_params;
+
     register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
     register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
     register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL);
@@ -426,6 +432,16 @@ static void bochs_bios_init(void)
     register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
     register_ioport_write(0x500, 1, 1, bochs_bios_write, NULL);
     register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
+
+    bios_params = qemu_mallocz(sizeof(FWCfgState));
+
+    register_ioport_read(BIOS_CFG_IOPORT, 1, 1, firmware_io_cfg_read,
+            bios_params);
+    register_ioport_write(BIOS_CFG_IOPORT, 1, 1, firmware_io_cfg_write,
+            bios_params);
+
+    firmware_cfg_add(bios_params, FW_CFG_SIGNATURE, "QEMU", 4);
+    firmware_cfg_add(bios_params, FW_CFG_ID, (uint8_t*)&bios_cfg_id, 4);
 }
 
 /* Generate an initial boot sector which sets state and jump to
--
			Gleb.

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

[Blue Swirl]

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

On 8/27/08, Gleb Natapov <gleb@qumranet.com> wrote:
>
>  On Tue, Aug 26, 2008 at 07:46:20PM +0300, Blue Swirl wrote:
>  > > Is the patch below what you mean? (not tested, but compiles)
>  >
>  > Yes, but I'd still put the code from the .h file and pc.c to a new .c
>  > file, only the keys and function prototypes to .h.
>  >
>
> Okey, here is updated one (compiled only)

I added some missing pieces like static/const keywords, device
save/load and reset, MMIO and Sparc32/64 support. I also renamed some
names for consistency.

[-- Attachment #2: new_fw_abi.diff --]
[-- Type: plain/text, Size: 11766 bytes --]

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

[Sebastian Herbszt]

Gleb Natapov wrote:
>>>> This is the incorrect frequency for a QEMU guest.  I'm not sure why 
>>>> this  is included.  What does passing the frequency give us?
>>>>
>>> This is included for two reasons. First one is to demonstrate that
>>> proposed interface is extensible. Second is that Micrisoft SVVP test  
>>> requires this info (and much more) to be set in SMBIOS tables.
>>
>> Can you elaborate a bit on the "and much more" part? Is this the stuff
>> described in "SMBIOS "Designed for Windows" Logo Requirements" at
>> http://www.microsoft.com/whdc/system/platform/firmware/SMBIOS.mspx ?
>>
> I think yes. There is Microsoft test suit that test SMBIOS
> implementation. Running it on bochs bios results in a couple of errors.

Could you maybe post those errors on the bochs mailing list?

> Some fields cannot be empty. Some bits should be set. Most things can be
> fixed without qemu help, but for CPU speed we need to ask qemu.

What about computing the frequency in rombios?

>>> Using real HW value seams to be better than just provide some default one.
>>
>> What downside does using a default value have? On the upside this patch would
>> not be needed.
>>
>>
> What value do you propose? Without knowing how Windows uses it we can't
> say what are the downsides.

- Sebastian

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

[Gleb Natapov]

On Thu, Aug 28, 2008 at 01:42:59AM +0200, Sebastian Herbszt wrote:
> Gleb Natapov wrote:
>>>>> This is the incorrect frequency for a QEMU guest.  I'm not sure 
>>>>> why this  is included.  What does passing the frequency give us?
>>>>>
>>>> This is included for two reasons. First one is to demonstrate that
>>>> proposed interface is extensible. Second is that Micrisoft SVVP 
>>>> test  requires this info (and much more) to be set in SMBIOS 
>>>> tables.
>>>
>>> Can you elaborate a bit on the "and much more" part? Is this the stuff
>>> described in "SMBIOS "Designed for Windows" Logo Requirements" at
>>> http://www.microsoft.com/whdc/system/platform/firmware/SMBIOS.mspx ?
>>>
>> I think yes. There is Microsoft test suit that test SMBIOS
>> implementation. Running it on bochs bios results in a couple of errors.
>
> Could you maybe post those errors on the bochs mailing list?
>
I'll have to rerun test. All I have now is fixes :)

>> Some fields cannot be empty. Some bits should be set. Most things can be
>> fixed without qemu help, but for CPU speed we need to ask qemu.
>
> What about computing the frequency in rombios?
>
There is a problem with this approach. What if the host is under heavy
load when guest is started? Linux computes frequency on a startup and
have the same problem. It was discussed recently on the KVM list.

--
			Gleb.

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

[Gleb Natapov]

On Wed, Aug 27, 2008 at 08:10:49PM +0300, Blue Swirl wrote:
> On 8/27/08, Gleb Natapov <gleb@qumranet.com> wrote:
> >
> >  On Tue, Aug 26, 2008 at 07:46:20PM +0300, Blue Swirl wrote:
> >  > > Is the patch below what you mean? (not tested, but compiles)
> >  >
> >  > Yes, but I'd still put the code from the .h file and pc.c to a new .c
> >  > file, only the keys and function prototypes to .h.
> >  >
> >
> > Okey, here is updated one (compiled only)
> 
> I added some missing pieces like static/const keywords, device
> save/load and reset, MMIO and Sparc32/64 support. I also renamed some
> names for consistency.
Thanks! I'll rebase my other patches on this and will test it.

--
			Gleb.

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

[Anthony Liguori]

Gleb Natapov wrote:
> On Wed, Aug 27, 2008 at 08:10:49PM +0300, Blue Swirl wrote:
>   
>> On 8/27/08, Gleb Natapov <gleb@qumranet.com> wrote:
>>     
>>>  On Tue, Aug 26, 2008 at 07:46:20PM +0300, Blue Swirl wrote:
>>>  > > Is the patch below what you mean? (not tested, but compiles)
>>>  >
>>>  > Yes, but I'd still put the code from the .h file and pc.c to a new .c
>>>  > file, only the keys and function prototypes to .h.
>>>  >
>>>
>>> Okey, here is updated one (compiled only)
>>>       
>> I added some missing pieces like static/const keywords, device
>> save/load and reset, MMIO and Sparc32/64 support. I also renamed some
>> names for consistency.
>>     
> Thanks! I'll rebase my other patches on this and will test it.
>   

I think this is missing save/restore support.  What happens if you do a 
save, move to a different machine, then do a restore, and reboot?  The 
guest will see a different value IIUC.

Also, instead of returning 0 on non-linux systems, why not just return 
some fixed value?  It's no more "wrong" than returning the host clock rate.

Regards,

Anthony Liguori

> --
> 			Gleb.
>
>
>   

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

[Blue Swirl]

On 9/7/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Gleb Natapov wrote:
>
> > On Wed, Aug 27, 2008 at 08:10:49PM +0300, Blue Swirl wrote:
> >
> >
> > > On 8/27/08, Gleb Natapov <gleb@qumranet.com> wrote:
> > >
> > >
> > > >  On Tue, Aug 26, 2008 at 07:46:20PM +0300, Blue Swirl wrote:
> > > >  > > Is the patch below what you mean? (not tested, but compiles)
> > > >  >
> > > >  > Yes, but I'd still put the code from the .h file and pc.c to a new
> .c
> > > >  > file, only the keys and function prototypes to .h.
> > > >  >
> > > >
> > > > Okey, here is updated one (compiled only)
> > > >
> > > >
> > > I added some missing pieces like static/const keywords, device
> > > save/load and reset, MMIO and Sparc32/64 support. I also renamed some
> > > names for consistency.
> > >
> > >
> > Thanks! I'll rebase my other patches on this and will test it.
> >
> >
>
>  I think this is missing save/restore support.  What happens if you do a
> save, move to a different machine, then do a restore, and reboot?  The guest
> will see a different value IIUC.
>
>  Also, instead of returning 0 on non-linux systems, why not just return some
> fixed value?  It's no more "wrong" than returning the host clock rate.

You commented the wrong patch, but anyway: the clock rate will be
saved by configuration device. Loading will be done after hardware has
been initializers have been called, so the saved value will be used.

The rates may be incorrect anyway. If x86 CPU definitions one day
include i386, giving i386 a modern frequency in GHz range can surprise
some guests.

I think Fabrice didn't like to use floating point arithmetics inside
the emulator, but I can't find a reference.

Otherwise, a mechanism to present the CPU frequency would be useful,
maybe even to other targets.

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

[Gleb Natapov]

I suppose you are referring to "cpu speed" patch. 

> I think this is missing save/restore support.  What happens if you do a  
> save, move to a different machine, then do a restore, and reboot?  The  
> guest will see a different value IIUC.
>
I don't think that getting a different value in SMBIOS tables after reboot
is problematic. It may change on real HW too I think. Because of power
saving on a laptop for instance.

> Also, instead of returning 0 on non-linux systems, why not just return  
> some fixed value?  It's no more "wrong" than returning the host clock 
> rate.
>
I am OK with that. What value do you propose?

I'll resent a whole series. Let's move discussion there. And if there is
no more comments about other parts of the series can we apply it finally?

--
			Gleb.