Re: [Qemu-devel] [Qemu-stable] [PATCH 0/3] ARM: three easy patches for coverity-reported issues

[Michael Roth]

Quoting Paolo Bonzini (2014-02-18 07:53:33)
> Il 18/02/2014 13:44, Andreas Färber ha scritto:
> >> > There isn't really a standard criterion.  It's up to each maintainer to
> >> > be stricter or looser on what goes to stable.
> > The criteria is pretty simple: Was the breakage in the last release
> > already or was it introduced only intermittently.
> 
> You haven't defined breakage; what breakage deserves a change in a 
> stable release.  Some interpret it as regression, some as "any bug", 
> some as "any crash bug", and so on.

Personally I think it's fair to punt that determination to the stable
maintainers: if it's a bug that existed in a previous release, however minor,
and you or someone else cares enough to cc: qemu-stable about, it's a
reasonable *candidate* for consideration.

Factors like whether it breaks guest ABI, migration compatibility, is too risky
a backport, etc, should be considered, but if unsure it's fine to punt to stable
and let the filtering happen there. If it's rejected/problematic stable should
provide a response/reason and the discussion can go from there.

Perhaps this may need to be revisited in the future if traffic to qemu-stable
becomes unwieldly but I don't think we're there yet.

> 
> Paolo

Re: [Qemu-devel] [PATCH] hw/arm/musicpal: Remove nonexistent CDTP2, CDTP3 registers

[Jan Kiszka]

On 2014-02-18 16:28, Peter Maydell wrote:
> The ethernet device in the musicpal only has two tx queues,
> but we modelled it with four CTDP registers, presumably a
> cut and paste from the rx queue registers. Since the tx_queue[]
> array is only 2 entries long this allowed a guest to overrun
> this buffer. Remove the nonexistent registers.
> 
> Signed-off-by: Peter Maydell 

Acked-by: Jan Kiszka 

> ---
> There's no readily available documentation for this SoC,
> but I'm told the BSP for it indicates that there are
> indeed only two tx queues.
> 
>  hw/arm/musicpal.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 023e875..a8d0086 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -92,8 +92,6 @@
>  #define MP_ETH_CRDP30x4AC
>  #define MP_ETH_CTDP00x4E0
>  #define MP_ETH_CTDP10x4E4
> -#define MP_ETH_CTDP20x4E8
> -#define MP_ETH_CTDP30x4EC
>  
>  /* MII PHY access */
>  #define MP_ETH_SMIR_DATA0x
> @@ -308,7 +306,7 @@ static uint64_t mv88w8618_eth_read(void *opaque, hwaddr 
> offset,
>  case MP_ETH_CRDP0 ... MP_ETH_CRDP3:
>  return s->rx_queue[(offset - MP_ETH_CRDP0)/4];
>  
> -case MP_ETH_CTDP0 ... MP_ETH_CTDP3:
> +case MP_ETH_CTDP0 ... MP_ETH_CTDP1:
>  return s->tx_queue[(offset - MP_ETH_CTDP0)/4];
>  
>  default:
> @@ -362,7 +360,7 @@ static void mv88w8618_eth_write(void *opaque, hwaddr 
> offset,
>  s->cur_rx[(offset - MP_ETH_CRDP0)/4] = value;
>  break;
>  
> -case MP_ETH_CTDP0 ... MP_ETH_CTDP3:
> +case MP_ETH_CTDP0 ... MP_ETH_CTDP1:
>  s->tx_queue[(offset - MP_ETH_CTDP0)/4] = value;
>  break;
>  }
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] rdma: bug fixes

[Michael Roth]

Quoting mrhi...@linux.vnet.ibm.com (2014-02-17 20:34:06)
> From: "Michael R. Hines" 
> 
> 1. Fix small memory leak in parsing inet address from command line in 
> data_init()
> 2. Fix ibv_post_send() return value check and pass error code back up 
> correctly.
> 3. Fix rdma_destroy_qp() segfault after failure to connect to destination.
> 
> Reported-by: frank.yang...@gmail.com
> Reported-by: dgilb...@redhat.com
> Signed-off-by: Michael R. Hines 

Reviewed-by: Michael Roth 

> ---
>  migration-rdma.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/migration-rdma.c b/migration-rdma.c
> index f94f3b4..29351a6 100644
> --- a/migration-rdma.c
> +++ b/migration-rdma.c
> @@ -1589,13 +1589,11 @@ static int qemu_rdma_post_send_control(RDMAContext 
> *rdma, uint8_t *buf,
>  }
> 
> 
> -if (ibv_post_send(rdma->qp, &send_wr, &bad_wr)) {
> -return -1;
> -}
> +ret = ibv_post_send(rdma->qp, &send_wr, &bad_wr);
> 
> -if (ret < 0) {
> +if (ret > 0) {
>  fprintf(stderr, "Failed to use post IB SEND for control!\n");
> -return ret;
> +return -ret;
>  }
> 
>  ret = qemu_rdma_block_for_wrid(rdma, RDMA_WRID_SEND_CONTROL, NULL);
> @@ -2237,10 +2235,6 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>  }
>  }
> 
> -if (rdma->qp) {
> -rdma_destroy_qp(rdma->cm_id);
> -rdma->qp = NULL;
> -}
>  if (rdma->cq) {
>  ibv_destroy_cq(rdma->cq);
>  rdma->cq = NULL;
> @@ -2258,6 +2252,10 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>  rdma->listen_id = NULL;
>  }
>  if (rdma->cm_id) {
> +if (rdma->qp) {
> +rdma_destroy_qp(rdma->cm_id);
> +rdma->qp = NULL;
> +}
>  rdma_destroy_id(rdma->cm_id);
>  rdma->cm_id = NULL;
>  }
> @@ -2512,8 +2510,10 @@ static void *qemu_rdma_data_init(const char 
> *host_port, Error **errp)
>  } else {
>  ERROR(errp, "bad RDMA migration address '%s'", host_port);
>  g_free(rdma);
> -return NULL;
> +rdma = NULL;
>  }
> +
> +qapi_free_InetSocketAddress(addr);
>  }
> 
>  return rdma;
> -- 
> 1.8.1.2

[Qemu-devel] [PATCH 5/5] xen, gfx passthrough: add opregion mapping

[Yang Zhang]

From: Yang Zhang 

The OpRegion shouldn't be mapped 1:1 because the address in the host
can't be used in the guest directly.

This patch traps read and write access to the opregion of the Intel
GPU config space (offset 0xfc).

The original patch is from Jean Guyader 

Signed-off-by: Yang Zhang 
Cc: Jean Guyader 
---
 hw/xen/xen_pt.h |4 ++-
 hw/xen/xen_pt_config_init.c |   45 ++-
 hw/xen/xen_pt_graphics.c|   45 +++
 3 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 92e4d51..9f7fd4e 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -63,7 +63,7 @@ typedef int (*xen_pt_conf_byte_read)
 #define XEN_PT_BAR_UNMAPPED (-1)
 
 #define PCI_CAP_MAX 48
-
+#define PCI_INTEL_OPREGION 0xfc
 
 typedef enum {
 XEN_PT_GRP_TYPE_HARDWIRED = 0,  /* 0 Hardwired reg group */
@@ -307,5 +307,7 @@ int intel_pch_init(PCIBus *bus);
 void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr,
uint32_t val, int len);
 uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len);
+uint32_t igd_read_opregion(XenPCIPassthroughState *s);
+void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val);
 
 #endif /* !XEN_PT_H */
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 8ccc2e4..30135c1 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -575,6 +575,22 @@ static int 
xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
 return 0;
 }
 
+static int xen_pt_intel_opregion_read(XenPCIPassthroughState *s,
+  XenPTReg *cfg_entry,
+  uint32_t *value, uint32_t valid_mask)
+{
+*value = igd_read_opregion(s);
+return 0;
+}
+
+static int xen_pt_intel_opregion_write(XenPCIPassthroughState *s,
+   XenPTReg *cfg_entry, uint32_t *value,
+   uint32_t dev_value, uint32_t valid_mask)
+{
+igd_write_opregion(s, *value);
+return 0;
+}
+
 /* Header Type0 reg static information table */
 static XenPTRegInfo xen_pt_emu_reg_header0[] = {
 /* Vendor ID reg */
@@ -1438,6 +1454,20 @@ static XenPTRegInfo xen_pt_emu_reg_msix[] = {
 },
 };
 
+static XenPTRegInfo xen_pt_emu_reg_igd_opregion[] = {
+/* Intel IGFX OpRegion reg */
+{
+.offset = 0x0,
+.size   = 4,
+.init_val   = 0,
+.no_wb  = 1,
+.u.dw.read   = xen_pt_intel_opregion_read,
+.u.dw.write  = xen_pt_intel_opregion_write,
+},
+{
+.size = 0,
+},
+};
 
 /
  * Capabilities
@@ -1675,6 +1705,14 @@ static const XenPTRegGroupInfo xen_pt_emu_reg_grps[] = {
 .size_init   = xen_pt_msix_size_init,
 .emu_regs = xen_pt_emu_reg_msix,
 },
+/* Intel IGD Opregion group */
+{
+.grp_id  = PCI_INTEL_OPREGION,
+.grp_type= XEN_PT_GRP_TYPE_EMU,
+.grp_size= 0x4,
+.size_init   = xen_pt_reg_grp_size_init,
+.emu_regs= xen_pt_emu_reg_igd_opregion,
+},
 {
 .grp_size = 0,
 },
@@ -1804,7 +1842,8 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
 uint32_t reg_grp_offset = 0;
 XenPTRegGroup *reg_grp_entry = NULL;
 
-if (xen_pt_emu_reg_grps[i].grp_id != 0xFF) {
+if (xen_pt_emu_reg_grps[i].grp_id != 0xFF
+&& xen_pt_emu_reg_grps[i].grp_id != PCI_INTEL_OPREGION) {
 if (xen_pt_hide_dev_cap(&s->real_device,
 xen_pt_emu_reg_grps[i].grp_id)) {
 continue;
@@ -1817,6 +1856,10 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
 }
 }
 
+if (xen_pt_emu_reg_grps[i].grp_id == PCI_INTEL_OPREGION) {
+reg_grp_offset = PCI_INTEL_OPREGION;
+}
+
 reg_grp_entry = g_new0(XenPTRegGroup, 1);
 QLIST_INIT(®_grp_entry->reg_tbl_list);
 QLIST_INSERT_HEAD(&s->reg_grps, reg_grp_entry, entries);
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index 2a01406..bebfcfd 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -6,6 +6,7 @@
 #include "hw/xen/xen_backend.h"
 
 int igd_passthru;
+static int igd_guest_opregion;
 
 /*
  * register VGA resources for the domain with assigned gfx
@@ -360,3 +361,47 @@ err_out:
 return -1;
 }
 
+uint32_t igd_read_opregion(XenPCIPassthroughState *s)
+{
+uint32_t val = -1;
+
+if (igd_guest_opregion == 0) {
+return val;
+}
+
+val = igd_guest_opregion;
+
+XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val);
+return val;
+}
+
+void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val)
+{
+uint32_t host_opregion = 0;
+int ret;
+
+if (igd_guest_opregion) {
+XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring 
%x\n",
+ 

[Qemu-devel] [PATCH 4/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D

[Yang Zhang]

From: Yang Zhang 

Some registers of Intel IGD are mapped in host bridge, so it needs to
passthrough these registers of physical host bridge to guest because
emulated host bridge in guest doesn't have these mappings.

The original patch is from Weidong Han < weidong.han @ intel.com >

Signed-off-by: Yang Zhang 
Cc:Weidong Han 
---
 hw/pci-host/piix.c   |   15 ++
 hw/pci/pci.c |   13 +
 hw/xen/xen_pt.h  |5 ++
 hw/xen/xen_pt_graphics.c |  127 ++
 4 files changed, 160 insertions(+), 0 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index ffdc853..68cf756 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -34,6 +34,9 @@
 #include "sysemu/sysemu.h"
 #include "hw/i386/ioapic.h"
 #include "qapi/visitor.h"
+#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
+#include "hw/xen/xen_pt.h"
+#endif
 
 /*
  * I440FX chipset data sheet.
@@ -389,6 +392,18 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
 
 i440fx_update_memory_mappings(f);
 
+#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
+/*
+ * Some registers of Intel IGD are mapped in host bridge, so it needs to
+ * passthrough these registers of physical host bridge to guest because
+ * emulated host bridge in guest doesn't have these mappings.
+ */
+if (intel_pch_init(b) == 0) {
+d->config_read = igd_pci_read;
+d->config_write = igd_pci_write;
+}
+#endif
+
 return b;
 }
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e81816e..7016b71 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,9 @@
 #include "hw/pci/msix.h"
 #include "exec/address-spaces.h"
 #include "hw/hotplug.h"
+#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
+#include "hw/xen/xen_pt.h"
+#endif
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -805,6 +808,16 @@ static PCIDevice *do_pci_register_device(PCIDevice 
*pci_dev, PCIBus *bus,
 PCIConfigWriteFunc *config_write = pc->config_write;
 AddressSpace *dma_as;
 
+#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
+/*
+ * Some video bioses and gfx drivers will assume the bdf of IGD is 00:02.0.
+ * So user need to set it to 00:02.0 in Xen configure file explicitly,
+ * otherwise IGD will fail to work.
+ */
+if (gfx_passthru && devfn == 0x10)
+igd_passthru = 1;
+else
+#endif
 if (devfn < 0) {
 for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
 devfn += PCI_FUNC_MAX) {
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index c04bbfd..92e4d51 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -299,8 +299,13 @@ static inline bool 
xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
 }
 
 extern int gfx_passthru;
+extern int igd_passthru;
 int register_vga_regions(XenHostPCIDevice *dev);
 int unregister_vga_regions(XenHostPCIDevice *dev);
 int setup_vga_pt(XenHostPCIDevice *dev);
+int intel_pch_init(PCIBus *bus);
+void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr,
+   uint32_t val, int len);
+uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len);
 
 #endif /* !XEN_PT_H */
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index 54f16cf..2a01406 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -5,6 +5,8 @@
 #include "xen-host-pci-device.h"
 #include "hw/xen/xen_backend.h"
 
+int igd_passthru;
+
 /*
  * register VGA resources for the domain with assigned gfx
  */
@@ -233,3 +235,128 @@ static int create_pch_isa_bridge(PCIBus *bus, 
XenHostPCIDevice *hdev)
 XEN_PT_LOG(dev, "Intel PCH ISA bridge is created.\n");
 return 0;
 }
+
+int intel_pch_init(PCIBus *bus)
+{
+XenHostPCIDevice hdev;
+int r = 0;
+
+if (!gfx_passthru) {
+return -1;
+}
+
+r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0);
+if (r) {
+XEN_PT_ERR(NULL, "Fail to find intel PCH in host\n");
+goto err;
+}
+
+if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) {
+r = create_pch_isa_bridge(bus, &hdev);
+if (r) {
+XEN_PT_ERR(NULL, "Fail to create PCH ISA bridge.\n");
+goto err;
+}
+}
+
+xen_host_pci_device_put(&hdev);
+
+return  r;
+
+err:
+return r;
+}
+
+void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr,
+   uint32_t val, int len)
+{
+XenHostPCIDevice dev;
+int r;
+
+assert(pci_dev->devfn == 0x00);
+
+if (!igd_passthru) {
+goto write_default;
+}
+
+switch (config_addr) {
+case 0x58:  /* PAVPC Offset */
+break;
+default:
+goto write_default;
+}
+
+/* Host write */
+r = xen_host_pci_device_get(&dev, 0, 0, 0, 0);
+if (r) {
+XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
+abort();
+}
+
+r = xen_host_pci_set_block(&dev, config_addr, (uint8_t *)&val, len);
+if (r) {
+XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
+abort();
+}
+
+xen_

[Qemu-devel] [PATCH 3/5] xen, gfx passthrough: create intel isa bridge

[Yang Zhang]

From: Yang Zhang 

ISA bridge is needed since Intel gfx drive will probe it instead
of Dev31:Fun0 to make graphics device passthrough work easy for VMM, that
only need to expose ISA bridge to let driver know the real hardware underneath.

The original patch is from Allen Kay [allen.m@intel.com]

Signed-off-by: Yang Zhang 
Cc: Allen Kay 
---
 hw/xen/xen_pt_graphics.c |   71 ++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
index 9ad8a74..54f16cf 100644
--- a/hw/xen/xen_pt_graphics.c
+++ b/hw/xen/xen_pt_graphics.c
@@ -162,3 +162,74 @@ out:
 free(bios);
 return rc;
 }
+
+static uint32_t isa_bridge_read_config(PCIDevice *d, uint32_t addr, int len)
+{
+uint32_t v;
+
+v = pci_default_read_config(d, addr, len);
+
+return v;
+}
+
+static void isa_bridge_write_config(PCIDevice *d, uint32_t addr, uint32_t v,
+int len)
+{
+pci_default_write_config(d, addr, v, len);
+
+return;
+}
+
+static void isa_bridge_class_init(ObjectClass *klass, void *data)
+{
+PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+k->config_read = isa_bridge_read_config;
+k->config_write = isa_bridge_write_config;
+
+return;
+};
+
+typedef struct {
+PCIDevice dev;
+} ISABridgeState;
+
+static TypeInfo isa_bridge_info = {
+.name  = "inte-pch-isa-bridge",
+.parent= TYPE_PCI_DEVICE,
+.instance_size = sizeof(ISABridgeState),
+.class_init = isa_bridge_class_init,
+};
+
+static void xen_pt_graphics_register_types(void)
+{
+type_register_static(&isa_bridge_info);
+}
+
+type_init(xen_pt_graphics_register_types)
+
+static int create_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
+{
+struct PCIDevice *dev;
+
+char rid;
+
+dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "inte-pch-isa-bridge");
+if (!dev) {
+XEN_PT_LOG(dev, "fail to create PCH ISA bridge.\n");
+return -1;
+}
+
+qdev_init_nofail(&dev->qdev);
+
+pci_config_set_vendor_id(dev->config, hdev->vendor_id);
+pci_config_set_device_id(dev->config, hdev->device_id);
+
+xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1);
+
+pci_config_set_revision(dev->config, rid);
+pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_ISA);
+
+XEN_PT_LOG(dev, "Intel PCH ISA bridge is created.\n");
+return 0;
+}
-- 
1.7.1

[Qemu-devel] [PATCH 1/5] xen, gfx passthrough: basic graphics passthrough support

[Yang Zhang]

From: Yang Zhang 

basic gfx passthrough support:
- add a vga type for gfx passthrough
- retrieve VGA bios from host 0xC, then load it to guest 0xC
- register/unregister legacy VGA I/O ports and MMIOs for passthroughed gfx

The original patch is from Weidong Han 

Signed-off-by: Yang Zhang 
Cc: Weidong Han 
---
 configure|2 +-
 hw/xen/Makefile.objs |2 +-
 hw/xen/xen-host-pci-device.c |5 ++
 hw/xen/xen-host-pci-device.h |1 +
 hw/xen/xen_pt.c  |   10 +++
 hw/xen/xen_pt.h  |4 +
 hw/xen/xen_pt_graphics.c |  164 ++
 qemu-options.hx  |9 +++
 vl.c |8 ++
 9 files changed, 203 insertions(+), 2 deletions(-)
 create mode 100644 hw/xen/xen_pt_graphics.c

diff --git a/configure b/configure
index 4648117..19525ab 100755
--- a/configure
+++ b/configure
@@ -4608,7 +4608,7 @@ case "$target_name" in
 if test "$xen" = "yes" -a "$target_softmmu" = "yes" ; then
   echo "CONFIG_XEN=y" >> $config_target_mak
   if test "$xen_pci_passthrough" = yes; then
-echo "CONFIG_XEN_PCI_PASSTHROUGH=y" >> "$config_target_mak"
+echo "CONFIG_XEN_PCI_PASSTHROUGH=y" >> "$config_host_mak"
   fi
 fi
 ;;
diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
index ce640c6..350d337 100644
--- a/hw/xen/Makefile.objs
+++ b/hw/xen/Makefile.objs
@@ -3,4 +3,4 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o 
xen_devconfig.o
 
 obj-$(CONFIG_XEN_I386) += xen_platform.o xen_apic.o xen_pvdevice.o
 obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
-obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o
+obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o 
xen_pt_msi.o xen_pt_graphics.o
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 743b37b..a54b7de 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -376,6 +376,11 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t 
domain,
 goto error;
 }
 d->irq = v;
+rc = xen_host_pci_get_hex_value(d, "class", &v);
+if (rc) {
+goto error;
+}
+d->class_code = v;
 d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
 
 return 0;
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index c2486f0..f1e1c30 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -25,6 +25,7 @@ typedef struct XenHostPCIDevice {
 
 uint16_t vendor_id;
 uint16_t device_id;
+uint32_t class_code;
 int irq;
 
 XenHostPCIIORegion io_regions[PCI_NUM_REGIONS - 1];
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index be4220b..5a36902 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -450,6 +450,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState 
*s)
d->rom.size, d->rom.base_addr);
 }
 
+register_vga_regions(d);
 return 0;
 }
 
@@ -470,6 +471,8 @@ static void 
xen_pt_unregister_regions(XenPCIPassthroughState *s)
 if (d->rom.base_addr && d->rom.size) {
 memory_region_destroy(&s->rom);
 }
+
+unregister_vga_regions(d);
 }
 
 /* region mapping */
@@ -693,6 +696,13 @@ static int xen_pt_initfn(PCIDevice *d)
 /* Handle real device's MMIO/PIO BARs */
 xen_pt_register_regions(s);
 
+/* Setup VGA bios for passthroughed gfx */
+if (setup_vga_pt(&s->real_device) < 0) {
+XEN_PT_ERR(d, "Setup VGA BIOS of passthroughed gfx failed!\n");
+xen_host_pci_device_put(&s->real_device);
+return -1;
+}
+
 /* reinitialize each config register to be emulated */
 if (xen_pt_config_init(s)) {
 XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 942dc60..c04bbfd 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -298,5 +298,9 @@ static inline bool 
xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
 return s->msix && s->msix->bar_index == bar;
 }
 
+extern int gfx_passthru;
+int register_vga_regions(XenHostPCIDevice *dev);
+int unregister_vga_regions(XenHostPCIDevice *dev);
+int setup_vga_pt(XenHostPCIDevice *dev);
 
 #endif /* !XEN_PT_H */
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
new file mode 100644
index 000..9ad8a74
--- /dev/null
+++ b/hw/xen/xen_pt_graphics.c
@@ -0,0 +1,164 @@
+/*
+ * graphics passthrough
+ */
+#include "xen_pt.h"
+#include "xen-host-pci-device.h"
+#include "hw/xen/xen_backend.h"
+
+/*
+ * register VGA resources for the domain with assigned gfx
+ */
+int register_vga_regions(XenHostPCIDevice *dev)
+{
+int ret = 0;
+
+if (!gfx_passthru || ((dev->class_code >> 0x8) != 0x0300)) {
+return ret;
+}
+
+ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3B0,
+0x3B0, 0xA, DPCI_ADD_MAPPING);
+
+ret |= xc_domain_ioport_mapping(xen_xc, xen_domid, 0x3C0,
+0x3C0, 0x20,

[Qemu-devel] [PATCH 0/5] xen: add Intel IGD passthrough support

[Yang Zhang]

From: Yang Zhang 

The following patches are ported from Xen Qemu-traditional branch which are
adding Intel IGD passthrough supporting to Qemu upstream.

To pass through IGD to guest, user need to add following lines in Xen config
file:
gfx_passthru=1
pci=['00:02.0@2']

Besides, since Xen + Qemu upstream is requiring seabios, user also need to
recompile seabios with CONFIG_OPTIONROMS_DEPLOYED=y to allow IGD pass through
successfully:
1. change CONFIG_OPTIONROMS_DEPLOYED=y in file: 
xen/tools/firmware/seabios-config
2. recompile the tools

I have successfully boot Win 7 and RHEL6u4 guests with IGD assigned in Haswell
desktop with Latest Xen + Qemu upstream.

Yang Zhang (5):
  xen, gfx passthrough: basic graphics passthrough support
  xen, gfx passthrough: reserve 00:02.0 for INTEL IGD
  xen, gfx passthrough: create intel isa bridge
  xen, gfx passthrough: support Intel IGD passthrough with VT-D
  xen, gfx passthrough: add opregion mapping

 configure|2 +-
 hw/pci-host/piix.c   |   15 ++
 hw/pci/pci.c |   19 ++
 hw/xen/Makefile.objs |2 +-
 hw/xen/xen-host-pci-device.c |5 +
 hw/xen/xen-host-pci-device.h |1 +
 hw/xen/xen_pt.c  |   10 +
 hw/xen/xen_pt.h  |   13 ++-
 hw/xen/xen_pt_config_init.c  |   45 +-
 hw/xen/xen_pt_graphics.c |  407 ++
 qemu-options.hx  |9 +
 vl.c |8 +
 12 files changed, 532 insertions(+), 4 deletions(-)
 create mode 100644 hw/xen/xen_pt_graphics.c

[Qemu-devel] [PATCH 2/5] xen, gfx passthrough: reserve 00:02.0 for INTEL IGD

[Yang Zhang]

From: Yang Zhang 

Some VBIOSs and drivers assume the IGD BDF (bus:device:function) is
always 00:02.0, so this patch reserves 00:02.0 for assigned IGD in
guest.

The original patch is from Weidong Han 

Signed-off-by: Yang Zhang 
Cc: Weidong Han 
---
 hw/pci/pci.c |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4e0701d..e81816e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -808,6 +808,12 @@ static PCIDevice *do_pci_register_device(PCIDevice 
*pci_dev, PCIBus *bus,
 if (devfn < 0) {
 for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
 devfn += PCI_FUNC_MAX) {
+#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
+/* If gfx_passthru is in use, reserve 00:02.* for the IGD */
+if (gfx_passthru && devfn == 0x10) {
+continue;
+}
+#endif
 if (!bus->devices[devfn])
 goto found;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH] KVM: Use return value for error print

[Paolo Bonzini]

Il 27/01/2014 15:18, Alexander Graf ha scritto:

Commit 94ccff13 introduced a more verbose failure message and retry
operations on KVM VM creation. However, it ended up using a variable
for its failure message that hasn't been initialized yet.

Fix it to use the value it meant to set.

Signed-off-by: Alexander Graf 
---
 kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kvm-all.c b/kvm-all.c
index a3fb8de..3f78651 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1427,7 +1427,7 @@ int kvm_init(void)
 } while (ret == -EINTR);

 if (ret < 0) {
-fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -s->vmfd,
+fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret,
 strerror(-ret));

 #ifdef TARGET_S390X



Applied to uq/master, thanks.

Paolo

Re: [Qemu-devel] CentOS 5.x intermittently fails to boot on QEMU 1.7.0

[Paolo Bonzini]

Il 21/02/2014 05:34, Matt Lupfer ha scritto:


This doesn't appear to be a solution, because with the timer rewrite, QEMU
moves its periodic (1 ms) qemu_notify_event() call to break out of
the main event loop from a SIGALRM handler to the rearm of a QEMU timer.
Presumably QEMU is counting on these generic callbacks.


Thanks for the report, I'll try to reproduce.

Paolo

Re: [Qemu-devel] [PATCH] KVM: Use return value for error print

[Michael Roth]

Quoting Alexander Graf (2014-01-27 08:18:09)
> Commit 94ccff13 introduced a more verbose failure message and retry
> operations on KVM VM creation. However, it ended up using a variable
> for its failure message that hasn't been initialized yet.
> 
> Fix it to use the value it meant to set.
> 
> Signed-off-by: Alexander Graf 

ping for 1.7.1

> ---
>  kvm-all.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index a3fb8de..3f78651 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1427,7 +1427,7 @@ int kvm_init(void)
>  } while (ret == -EINTR);
> 
>  if (ret < 0) {
> -fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -s->vmfd,
> +fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -ret,
>  strerror(-ret));
> 
>  #ifdef TARGET_S390X
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH] pseries: Update SLOF firmware image to 20140204

[Alexey Kardashevskiy]

On 02/10/2014 05:52 PM, Alexey Kardashevskiy wrote:
> The changelog is:
>   > version: update to 20140204
>   > virtio-9p: disable unused structure
>   > Make "boot net:dhcp" boot from IPv4 only
>   > Fix virtio device shutdown
>   > Change shutdown method name for virtio-scsi
>   > Add support for 64bit LE ABI v1 and v2 support
>   > Change representation of string environment variable
>   > cas: return error when unknown node found
>   > version: update
>   > Reset obp-tftp arguments before parsing
>   > Enable seamless netboot on IPv6 network
>   > Fix shutdown for virtio devices
>   > Fix zero checksum in UDP header
>   > Handle router advertisement message properly
>   > [oex]hci_exit: Check before freeing/unmapping memory
>   > Work around missing sc 1 traps on pHyp
>   > fix print_version() to return where it came from
>   > usb-xhci: memory freeing and using returns as bool uniformly
>   > Output banner and initial display output in VNC window
>   > use VERSION file to generate FW version
>   > cas: remove warning
>   > Add support for loading little endian ELF binaries.
>   > Add bswap_{16,32,64}p
>   > dhcpv6 and other minor net-snk fixes
>   > Fix missing drop in virtio-fs setup-alias
>   > Find next available alias name
>   > SLOF does not exit if given 1KB disk
>   > boot: enable support for bootindex
>   > pci-properties: add properties to enable hotplug for spapr
>   > e1000: remember node handle
>   > Increase quiesce tokens array size
>   > virtio: timeout after 5sec
>   > Enable IPv6 support in dns
>   > usb-ohci: fix warnings
>   > Add ipv6 support in net-snk
>   > ipv4: fix frame overwriting following arp_send_request
>   > e1000: fix SLOF_dma_map_out arguments
>   > Maintain single global packet buffer for tftp
>   > Increase virtio-net receive queue size
>   > Increase veth receive queue size
>   > Fix dprintf macros at various points
>   > usb-ohci: rewrite done_head processing code
>   > boot: add net in default boot order
>   > block 0 address in the allocator
>   > scsi: make-media-alias fix
>   > usb-xhci: add xhci host controller support
>   > usb-xhci: add xhci support
>   > Avoid veth read/write calls with zero length buffer
>   > boot: include other aliases
>   > usb-core: disable xhci


Ping?



-- 
Alexey

Re: [Qemu-devel] [PATCH] target-i386: Fix CC_OP_CLR vs PF

[Michael Roth]

Quoting Richard Henderson (2014-01-10 14:39:56)
> Parity should be set for a zero result.
> 
> Signed-off-by: Richard Henderson 

ping for 1.7.1

> ---
>  target-i386/cc_helper.c | 2 +-
>  target-i386/translate.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/cc_helper.c b/target-i386/cc_helper.c
> index ee04092..05dd12b 100644
> --- a/target-i386/cc_helper.c
> +++ b/target-i386/cc_helper.c
> @@ -103,7 +103,7 @@ target_ulong helper_cc_compute_all(target_ulong dst, 
> target_ulong src1,
>  case CC_OP_EFLAGS:
>  return src1;
>  case CC_OP_CLR:
> -return CC_Z;
> +return CC_Z | CC_P;
> 
>  case CC_OP_MULB:
>  return compute_all_mulb(dst, src1);
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index b0f2279..34f35e7 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -748,7 +748,7 @@ static void gen_compute_eflags(DisasContext *s)
>  return;
>  }
>  if (s->cc_op == CC_OP_CLR) {
> -tcg_gen_movi_tl(cpu_cc_src, CC_Z);
> +tcg_gen_movi_tl(cpu_cc_src, CC_Z | CC_P);
>  set_cc_op(s, CC_OP_EFLAGS);
>  return;
>  }
> -- 
> 1.8.4.2

[Qemu-devel] [PATCH] virtio-net: calculate proper msix vectors on init

[Jason Wang]

Currently, the default msix vectors for virtio-net-pci is 3 which is
obvious not suitable for multiqueue guest, so we depends on the user
or management tools to pass a correct vectors parameter. In fact, we
can simplifying this by calculate the number of vectors on init.

Consider we have N queues, the number of vectors needed is 2*N + 2
(plus one config interrupt and control vq). We didn't check whether or
not host support control vq because it was added unconditionally by
qemu to avoid breaking legacy guests such as Minix.

Cc: Paolo Bonzini 
Cc: Michael S. Tsirkin 
Signed-off-by: Jason Wang 
---
 hw/virtio/virtio-pci.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7b91841..1dec491 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1416,7 +1416,8 @@ static const TypeInfo virtio_serial_pci_info = {
 static Property virtio_net_properties[] = {
 DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
 VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
-DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
+DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
+   DEV_NVECTORS_UNSPECIFIED),
 DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
 DEFINE_NIC_PROPERTIES(VirtIONetPCI, vdev.nic_conf),
 DEFINE_VIRTIO_NET_PROPERTIES(VirtIONetPCI, vdev.net_conf),
@@ -1428,6 +1429,13 @@ static int virtio_net_pci_init(VirtIOPCIProxy *vpci_dev)
 DeviceState *qdev = DEVICE(vpci_dev);
 VirtIONetPCI *dev = VIRTIO_NET_PCI(vpci_dev);
 DeviceState *vdev = DEVICE(&dev->vdev);
+VirtIONet *net = VIRTIO_NET(&dev->vdev);
+
+if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
+vpci_dev->nvectors = 2 * MAX(net->nic_conf.queues, 1) + 2;
+}
+
+fprintf(stderr, "vectors is %d\n", vpci_dev->nvectors);
 
 virtio_net_set_config_size(&dev->vdev, vpci_dev->host_features);
 virtio_net_set_netclient_name(&dev->vdev, qdev->id,
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Stefan Weil]

Am 20.02.2014 23:18, schrieb Peter Maydell:
> On 20 February 2014 21:18, Stefan Weil  wrote:
>> MinGW-w64's gcc has cpuid.h, so my 32 and 64 bit cross builds work
>> without problems. We can use that code for MinGW, too, but we could also
>> stop supporting MinGW (which has several other deficits).
> 
> We need the conditionals for MacOSX builds anyway, so we
> don't need to drop MinGW for this. (I compile with the 32 bit
> version rather than -w64 because I was able to get that to
> install on my Ubuntu box, whereas the -w64 seemed to have
> dependency issues/conflicts somehow. I figured the 32 bit
> version was good enough for detecting the typical "long is
> a funny size and we don't build" issues.)

One of my hosts runs Ubuntu precise. mingw-w64 works fine here and
includes both 32 bit and 64 compilers and libraries (the -w64 in its
name might be misleading). Maybe you will also need mingw-w64-tools, and
you can also add g++-mingw-w64 (which also includes two compilers).

Run configure with --cross-prefix=i686-w64-mingw32- or
--cross-prefix=x86_64-w64-mingw32- to build 32 or 64 bit executables.

Stefan

Re: [Qemu-devel] [PATCH] hw/arm/spitz: Avoid clash with Windows header symbol MOD_SHIFT

[Stefan Weil]

Am 20.02.2014 23:22, schrieb Peter Maydell:
> On 20 February 2014 21:26, Stefan Weil  wrote:
>> There was a suggestion to remove some dependencies on windows.h (which
>> causes the trouble here). I recently started doing this, and that
>> approach fixes the warning, too. Maybe I can send a patch next weekend.
> 
> That would probably be the nicest approach, yes. I won't apply
> this to the target-arm queue just yet.
> 
> I'm wondering if we might be able to get to the point of enabling
> -Werror for Windows and MacOSX builds. Do you think that would
> be a useful thing to do? (Obviously for git builds only, same as Linux.)

Yes, I think so. With MinGW-w64 this is quite realistic, even when I add
-Wextra (which I also use for my Linux builds). Some less important
warnings must be suppressed then, of course.

> (For MacOSX there are some deprecation warnings in the audio
> code I need to fix first but we otherwise compile pretty much
> warning free on 10.8. I'd be reliant on reports from other people
> for warnings on 10.9 and on earlier-than-10.8, though.)

10.9 also shows the audio deprecation warnings, but that's all as far as
I remember.

Stefan

Re: [Qemu-devel] Can KVM be enabled for Ubuntu Guest on Windows Host

[Marcus]

Sorry, for some reason I thought this was the CloudStack mailing list...

On Thu, Feb 20, 2014 at 10:31 PM, Marcus  wrote:
> Nested virtualization will work, if the hypervisor you are using
> supports passing the vmx flag in the guest.  As far as I'm aware,
> VMware fusion does, but qemu on windows probably doesn't.
>
> I once made a few code changes to allow KVM guests to work without the
> need for vmx (for devcloud), but the result was practically unusable
> as nested virtualization.
>
> On Thu, Feb 20, 2014 at 9:22 PM, Shehbaz Jaffer
>  wrote:
>> Hi,
>>
>> I wanted to know if I have a Windows Machine, and I run Ubuntu Guest on my
>> Windows machine using QEMU, will I be able to enable kvm inside my Ubuntu
>> VM? (Basically is nested virtualization support avaiable for Windows -
>> Ubuntu - Another Guest?)
>>
>> Thankyou
>> Shehbaz.

Re: [Qemu-devel] Can KVM be enabled for Ubuntu Guest on Windows Host

[Marcus]

Nested virtualization will work, if the hypervisor you are using
supports passing the vmx flag in the guest.  As far as I'm aware,
VMware fusion does, but qemu on windows probably doesn't.

I once made a few code changes to allow KVM guests to work without the
need for vmx (for devcloud), but the result was practically unusable
as nested virtualization.

On Thu, Feb 20, 2014 at 9:22 PM, Shehbaz Jaffer
 wrote:
> Hi,
>
> I wanted to know if I have a Windows Machine, and I run Ubuntu Guest on my
> Windows machine using QEMU, will I be able to enable kvm inside my Ubuntu
> VM? (Basically is nested virtualization support avaiable for Windows -
> Ubuntu - Another Guest?)
>
> Thankyou
> Shehbaz.

Re: [Qemu-devel] [RFC PATCH] file ram alloc: fail if cannot preallocate

[Alexey Kardashevskiy]

On 02/10/2014 05:32 PM, Alexey Kardashevskiy wrote:
> At the moment if the user asked for huge pages and there is no more huge
> pages, QEMU prints warning and falls back to the anonymous memory
> allocator which is quite easy not to notice. QEMU also does so even
> if the user specified -mem-prealloc and it seems wrong as the user
> specifically requested huge pages for the entire RAM but QEMU failed to do
> so and continued. On PPC64 this will produce a fragile guest as QEMU
> tells the guest via device-tree that it can use huge pages when it
> actually cannot.
> 
> This adds message+exit if RAM cannot be preallocated from huge pages.


Too bad? Should I increase my personal pinging timeout from 1 to 2 weeks to
avoid annoying the community? :) Thanks!



> Signed-off-by: Alexey Kardashevskiy 
> ---
>  exec.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 9ad0a4b..deb8279 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1041,6 +1041,10 @@ static void *file_ram_alloc(RAMBlock *block,
>  
>  area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>  if (area == MAP_FAILED) {
> +if (mem_prealloc) {
> +perror("file_ram_alloc: failed to preallocate RAM");
> +exit(1);
> +}
>  perror("file_ram_alloc: can't mmap RAM pages");
>  close(fd);
>  return (NULL);
> 


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH v2 01/12] mc: add documentation for micro-checkpointing

[Michael R. Hines]

On 02/21/2014 12:32 AM, Dr. David Alan Gilbert wrote:


I'm happy to use more memory to get FT, all I'm trying to do is see
if it's possible to put a lower bound than 2x on it while still maintaining
full FT, at the expense of performance in the case where it uses
a lot of memory.


The bottom line is: if you put a *hard* constraint on memory usage,
what will happen to the guest when that garbage collection you mentioned
shows up later and runs for several minutes? How about an hour?
Are we just going to block the guest from being allowed to start a
checkpoint until the memory usage goes down just for the sake of avoiding
the 2x memory usage?

Yes, or move to the next checkpoint sooner than the N milliseconds when
we see the buffer is getting full.


OK, I see there is definitely some common ground there: So to be
more specific, what we really need is two things: (I've learned that
the reviewers are very cautious about adding to much policy into
QEMU itself, but let's iron this out anyway:)

1. First, we need to throttle down the guest (QEMU can already do this
using the recently introduced "auto-converge" feature). This means
that the guest is still making forward progress, albeit slow progress.

2. Then we would need some kind of policy, or better yet, a trigger that
does something to the effect of "we're about to use a whole lot of
checkpoint memory soon - can we afford this much memory usage".
Such a trigger would be conditional on the current policy of the
administrator or management software: We would either have a QMP
command that with a boolean flag that says "Yes" or "No", it's
tolerable or not to use that much memory in the next checkpoint.

If the answer is "Yes", then nothing changes.
If the answer is "No", then we should either:
   a) throttle down the guest
   b) Adjust the checkpoint frequency
   c) Or pause it altogether while we migrate some other VMs off the
   host such that we can complete the next checkpoint in its 
entirety.


It's not clear to me how much of this (or any) of this control loop should
be in QEMU or in the management software, but I would definitely agree
that a minimum of at least the ability to detect the situation and remedy
the situation should be in QEMU. I'm not entirely convince that the
ability to *decide* to remedy the situation should be in QEMU, though.





If you block the guest from being checkpointed,
then what happens if there is a failure during that extended period?
We will have saved memory at the expense of availability.

If the active machine fails during this time then the secondary carries
on from it's last good snapshot in the knowledge that the active
never finished the new snapshot and so never uncorked it's previous packets.

If the secondary machine fails during this time then tha active drops
it's nascent snapshot and carries on.


Yes, that makes sense. Where would that policy go, though,
continuing the above concern?


However, what you have made me realise is that I don't have an answer
for the memory usage on the secondary; while the primary can pause
it's guest until the secondary ack's the checkpoint, the secondary has
to rely on the primary not to send it huge checkpoints.


Good question: There's a lot of work ideas out there in the academic
community to compress the secondary, or push the secondary to
a flash-based device, or de-duplicate the secondary. I'm sure any
of them would put a dent in the problem, but I'm not seeing a smoking
gun solution that would absolutely save all that memory completely.

(Personally, I don't believe in swap. I wouldn't even consider swap
or any kind of traditional disk-based remedy to be a viable solution).


The customer that is expecting 100% fault tolerance and the provider
who is supporting it need to have an understanding that fault tolerance
is not free and that constraining memory usage will adversely affect
the VM's ability to be protected.

Do I understand your expectations correctly? Is fault tolerance
something you're willing to sacrifice?

As above, no I'm willing to sacrifice performance but not fault tolerance.
(It is entirely possible that others would want the other trade off, i.e.
some minimum performance is worse than useless, so if we can't maintain
that performance then dropping FT leaves us in a more-working position).



Agreed - I think a "proactive" failover in this case would solve the 
problem.

If we observed that availability/fault tolerance was going to be at
risk soon (which is relatively easy to detect) - we could just *force*
a failover to the secondary host and restart the protection from
scratch.




Well, that's simple: If there is a failure of the source, the destination
will simply revert to the previous checkpoint using the same mode
of operation. The lost ACKs that you're curious about only
apply to the checkpoint that is in progress. Just because a
checkpoint is in progress does not mean that the previ

[Qemu-devel] CentOS 5.x intermittently fails to boot on QEMU 1.7.0

[Matt Lupfer]

Hello,

After upgrading to QEMU 1.7.0, CentOS 5.x guests often fail to boot
with the following kernel apic=debug output:

> ACPI: Core revision 20060707
> enabled ExtINT on CPU#0
> ENABLING IO-APIC IRQs
> ..TIMER: vector=0x31 apic1=0 pin1=2 apic2=-1 pin2=-1
> ..MP-BIOS bug: 8254 timer not connected to IO-APIC
> ...trying to set up timer (IRQ0) through the 8259A ...  failed.
> ...trying to set up timer as Virtual Wire IRQ... failed.
> ...trying to set up timer as ExtINT IRQ... failed .
> Kernel panic - not syncing: IO-APIC + timer doesn't work! Try using the 
'noapic' kernel parameter

This happens greater than 50% of the time in my configuration.

Adding the noapic or no_timer_check parameter causes the guest to boot
properly.  I'd like to find a way to restore the previous behavior, which
didn't require these guest kernel parameters.

Host is a fully updated Fedora 20, kernel 3.12.10-300.fc20.x86_64 with an Intel
Core i5-2500 CPU. Guest is a fully updated base install of CentOS 5.10, kernel
2.6.18-371.4.1.el5.x86_64 (installed with "noapic", but booted with default
parameters).

QEMU invocation:
./x86_64-softmmu/qemu-system-x86_64 -m 4096 -cpu host -enable-kvm -drive 
file=~/ddn-001.img,cache=off -serial telnet:0.0.0.0:,server,nowait

A git bisect points to this commit as the culprit:

b1bbfe7 aio / timers: On timer modification, qemu_notify or aio_notify

which was part of the Aug 2013 timer rewrite.  Reverting this hunk in
particular makes the issue go away:

> @@ -522,9 +531,7 @@ void qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
>  }
>  /* Interrupt execution to force deadline recalculation.  */
>  qemu_clock_warp(ts->timer_list->clock);
> -if (use_icount) {
> -timerlist_notify(ts->timer_list);
> -}
> +timerlist_notify(ts->timer_list);
>  }
>  }

(Note this was later refactored into timerlist_rearm() in 1.7.0, so I
mean that I modified timerlist_rearm() in 1.7.0 to read as that hunk
did before the b1bbfe7 commit.)

This doesn't appear to be a solution, because with the timer rewrite, QEMU
moves its periodic (1 ms) qemu_notify_event() call to break out of
the main event loop from a SIGALRM handler to the rearm of a QEMU timer.
Presumably QEMU is counting on these generic callbacks.

It appears that in QEMU 1.7.0, QEMU/KVM doesn't inject timer interrupts, or
alternatively the guest doesn't handle them, quickly enough to pass
the timer check in the guest kernel reliably.

I've found that if I suppress the first 20ms of calls to timerlist_notify()
in timerlist_rearm() by timers on the QEMU_CLOCK_VIRTUAL, the system is
able to boot successfully and remains stable.  Not calling
qemu_notify_event() on the first 20 ticks of QEMU_CLOCK_VIRTUAL seems to
alter the timings enough to produce a reliable result.  I tried this after
realizing that the guest kernel enables the HPET, which enables the QEMU
virtual clock, immediately before the guest timer check occurs.  I also
observed that the kernel boots fine with the "nohpet" parameter, and
I suspected that this could be a source of resource contention.

Finally, the QEMU options to disable KVM PIT IRQ reinjection and to disable
the kvm kernel irqchip altogether result in less frequent panics, but the
guest still panics within 100 boots.

Thanks for any assistance you can provide.

Matt

[Qemu-devel] Can KVM be enabled for Ubuntu Guest on Windows Host

[Shehbaz Jaffer]

Hi,

I wanted to know if I have a Windows Machine, and I run Ubuntu Guest on my
Windows machine using QEMU, will I be able to enable kvm inside my Ubuntu
VM? (Basically is nested virtualization support avaiable for Windows -
Ubuntu - Another Guest?)

Thankyou
Shehbaz.

Re: [Qemu-devel] [PATCH] spapr-vlan: flush queue whenever can_receive can go from false to true

[Alexey Kardashevskiy]

On 02/14/2014 12:27 PM, Alexey Kardashevskiy wrote:
> When the guests adds buffers to receive queue, the network device
> should flush its queue of pending packets. This is done with
> qemu_flush_queued_packets.
> 
> This adds a call to qemu_flush_queued_packets() which wakes up the main
> loop and let QEMU update the network device status which now is "can
> receive". The patch basically does the same thing as e8b4c68 does.


Ping, anyone?


> Suggested-by: Max Filippov 
> Signed-off-by: Alexey Kardashevskiy 
> ---
>  hw/net/spapr_llan.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index 72b0513..a7f00db 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -408,6 +408,8 @@ static target_ulong h_add_logical_lan_buffer(PowerPCCPU 
> *cpu,
>  
>  dev->rx_bufs++;
>  
> +qemu_flush_queued_packets(qemu_get_queue(dev->nic));
> +
>  DPRINTF("h_add_logical_lan_buffer():  Added buf  ptr=%d  rx_bufs=%d"
>  " bd=0x%016llx\n", dev->add_buf_ptr, dev->rx_bufs,
>  (unsigned long long)buf);
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v5 3/6] vl: allow customizing the class of /machine

[Alexey Kardashevskiy]

On 02/21/2014 12:50 AM, Alexey Kardashevskiy wrote:
> From: Paolo Bonzini 
> 
> This is a first step towards QOMifying /machine.
> 
> Signed-off-by: Paolo Bonzini 

I got interesting conversation about "sob" in my team so here it is:

Signed-off-by: Alexey Kardashevskiy 

Is that enough or I better repost the patch?
May be patchworks will pick it as it does for "RB" and other "by"'s.


> ---
>  include/hw/boards.h | 1 +
>  vl.c| 5 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index c2096e6..8640272 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -29,6 +29,7 @@ struct QEMUMachine {
>  const char *name;
>  const char *alias;
>  const char *desc;
> +const char *class_name;
>  QEMUMachineInitFunc *init;
>  QEMUMachineResetFunc *reset;
>  QEMUMachineHotAddCPUFunc *hot_add_cpu;
> diff --git a/vl.c b/vl.c
> index 01ab7e4..b300721 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4034,6 +4034,11 @@ int main(int argc, char **argv, char **envp)
>  qtest_init(qtest_chrdev, qtest_log);
>  }
>  
> +if (machine->class_name) {
> +Object *m = object_new(machine->class_name);
> +object_property_add_child(object_get_root(), "machine", m, NULL);
> +}
> +
>  machine_opts = qemu_get_machine_opts();
>  kernel_filename = qemu_opt_get(machine_opts, "kernel");
>  initrd_filename = qemu_opt_get(machine_opts, "initrd");
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v18 03/14] NUMA: Add numa_info structure to contain numa nodes info

[hu tao]

On Wed, Feb 19, 2014 at 5:26 PM, Igor Mammedov  wrote:

> On Wed, 19 Feb 2014 15:53:54 +0800
> Hu Tao  wrote:
>
> > From: Wanlong Gao 
> >
> > Add the numa_info structure to contain the numa nodes memory,
> > VCPUs information and the future added numa nodes host memory
> > policies.
> this is old version that breaks spar build which Wanlong already fixed.
>
> You can replace patches 1-5 with more recent ones posted recently:
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg216404.html


Thanks!

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

Alexander Graf  writes:
> On 02/18/2014 05:17 PM, Cornelia Huck wrote:
>> Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the
>> byteswap value) for legacy devices? The device implementation will be
>> aware of the virtio version anyway.
>
> Yeah, but I would hope we want to share as much code as possible here, 
> so that config accessors can be shared between legacy virtio and 1.0 
> virtio. And in that case we want to have a generic helper to read/write 
> pieces of that config space - which this patch introduces for us :).
>
>> (Btw., can some of those architectures supporting both le/be run with
>> mixed le/be cpus? That would be a mess for legacy devices.)
>
> Yes, they can. No, we don't care :).

There's per-cpu (what endian) and per-device (legacy or no).  To do this
Right, we'd need to consult both, but we don't always have that
information.  So this the minimal viable solution.

We'll need a per-device legacy flag eventually (there are other changes
than just endian).  But getting the wrappers in place is a good first
step.

Cheers,
Rusty.

Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Rusty Russell]

Greg Kurz  writes:
> On Tue, 18 Feb 2014 20:25:15 +0100
> Andreas Färber  wrote:
>> Am 18.02.2014 13:38, schrieb Greg Kurz:
>> > diff --git a/include/hw/virtio/virtio-access.h
>> > b/include/hw/virtio/virtio-access.h new file mode 100644
>> > index 000..2e22a47
>> > --- /dev/null
>> > +++ b/include/hw/virtio/virtio-access.h
>> > @@ -0,0 +1,132 @@
>> > +/*
>> > + * Virtio Accessor Support: In case your target can change endian.
>> > + *
>> > + * Copyright IBM, Corp. 2013
>> > + *
>> > + * Authors:
>> > + *  Rusty Russell   
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL, version 2.
>> > See
>> > + * the COPYING file in the top-level directory.
>> [snip]
>> 
>> I notice that this has been GPL-2.0 from Rusty's first series on. Is
>> there a reason not to make the new file GPL-2.0+?
>> 
>> Cf. http://wiki.qemu.org/Relicensing
>> 
>> Thanks,
>> Andreas
>> 
>
> Rusty ? It is your call. :)

I cut & paste that header.  I always prefer 2+ to 2 anyway.

Please fix:

 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation, either version 2 of the License, or
 * (at your option) any later version.

Thanks,
Rusty.

[Qemu-devel] [PATCH v2] XBZRLE: Fix qemu crash when resize the xbzrle cache

[Gonglei (Arei)]

Resizing the xbzrle cache during migration causes qemu-crash,
because the main-thread and migration-thread modify the xbzrle
cache size concurrently without lock-protection.

Signed-off-by: ChenLiang 
Signed-off-by: Gonglei 
---
Changes against the previous version:
 *Remove function cache_max_num_items and cache_page_size
 *Protect migration_end and ram_save_setup by XBZRLE.lock

 arch_init.c |   61 ++
 1 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 80574a0..d295237 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -164,26 +164,69 @@ static struct {
 uint8_t *encoded_buf;
 /* buffer for storing page content */
 uint8_t *current_buf;
-/* Cache for XBZRLE */
+/* Cache for XBZRLE, Protected by lock. */
 PageCache *cache;
+QemuMutex lock;
 } XBZRLE = {
 .encoded_buf = NULL,
 .current_buf = NULL,
 .cache = NULL,
+/* use the lock carefully */
+.lock = {PTHREAD_MUTEX_INITIALIZER},
 };
 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;
 
+static void XBZRLE_cache_lock(void)
+{
+qemu_mutex_lock(&XBZRLE.lock);
+}
+
+static void XBZRLE_cache_unlock(void)
+{
+qemu_mutex_unlock(&XBZRLE.lock);
+}
+
 int64_t xbzrle_cache_resize(int64_t new_size)
 {
+PageCache *new_cache, *old_cache, *cache;
+
 if (new_size < TARGET_PAGE_SIZE) {
 return -1;
 }
 
-if (XBZRLE.cache != NULL) {
-return cache_resize(XBZRLE.cache, new_size / TARGET_PAGE_SIZE) *
-TARGET_PAGE_SIZE;
+XBZRLE_cache_lock();
+/* check XBZRLE.cache again later */
+cache = XBZRLE.cache;
+XBZRLE_cache_unlock();
+
+if (cache != NULL) {
+if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
+goto ret;
+}
+new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
+TARGET_PAGE_SIZE);
+if (!new_cache) {
+DPRINTF("Error creating cache\n");
+return -1;
+}
+
+XBZRLE_cache_lock();
+/* the XBZRLE.cache may have be destroyed, check it again */
+if (XBZRLE.cache != NULL) {
+old_cache = XBZRLE.cache;
+XBZRLE.cache = new_cache;
+new_cache = NULL;
+}
+XBZRLE_cache_unlock();
+
+if (NULL == new_cache) {
+cache_fini(old_cache);
+} else {
+cache_fini(new_cache);
+}
 }
+ret:
 return pow2floor(new_size);
 }
 
@@ -522,6 +565,8 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
 ret = ram_control_save_page(f, block->offset,
offset, TARGET_PAGE_SIZE, &bytes_sent);
 
+XBZRLE_cache_lock();
+
 if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
 if (ret != RAM_SAVE_CONTROL_DELAYED) {
 if (bytes_sent > 0) {
@@ -553,6 +598,8 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
 acct_info.norm_pages++;
 }
 
+XBZRLE_cache_unlock();
+
 /* if page is unmodified, continue to the next */
 if (bytes_sent > 0) {
 last_sent_block = block;
@@ -620,6 +667,7 @@ static void migration_end(void)
 migration_bitmap = NULL;
 }
 
+XBZRLE_cache_lock();
 if (XBZRLE.cache) {
 cache_fini(XBZRLE.cache);
 g_free(XBZRLE.cache);
@@ -629,6 +677,7 @@ static void migration_end(void)
 XBZRLE.encoded_buf = NULL;
 XBZRLE.current_buf = NULL;
 }
+XBZRLE_cache_unlock();
 }
 
 static void ram_migration_cancel(void *opaque)
@@ -659,13 +708,16 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 dirty_rate_high_cnt = 0;
 
 if (migrate_use_xbzrle()) {
+XBZRLE_cache_lock();
 XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
   TARGET_PAGE_SIZE,
   TARGET_PAGE_SIZE);
 if (!XBZRLE.cache) {
+XBZRLE_cache_unlock();
 DPRINTF("Error creating cache\n");
 return -1;
 }
+XBZRLE_cache_unlock();
 
 /* We prefer not to abort if there is no memory */
 XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
@@ -681,7 +733,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 XBZRLE.encoded_buf = NULL;
 return -1;
 }
-
 acct_clear();
 }
 
-- 
1.6.0.2

Best regards,
-Gonglei

Re: [Qemu-devel] MSI interrupt support with vioscsi.c miniport driver

[Vadim Rozenfeld]

On Wed, 2014-02-19 at 15:25 -0800, Nicholas A. Bellinger wrote:
> On Wed, 2014-02-19 at 19:03 +1100, Vadim Rozenfeld wrote:
> > On Tue, 2014-02-18 at 13:00 -0800, Nicholas A. Bellinger wrote:
> > > On Mon, 2014-02-10 at 11:05 -0800, Nicholas A. Bellinger wrote:
> > > 
> > > 
> > > 
> > > > > > > Hi Yan,
> > > > > > > 
> > > > > > > So recently I've been doing some KVM guest performance comparisons
> > > > > > > between the scsi-mq prototype using virtio-scsi + vhost-scsi, and
> > > > > > > Windows Server 2012 with vioscsi.sys (virtio-win-0.1-74.iso) +
> > > > > > > vhost-scsi using PCIe flash backend devices.
> > > > > > > 
> > > > > > > I've noticed that small block random performance for the MSFT 
> > > > > > > guest is
> > > > > > > at around ~80K IOPs with multiple vioscsi LUNs + adapters, which 
> > > > > > > ends up
> > > > > > > being well below what the Linux guest with scsi-mq + virtio-scsi 
> > > > > > > is
> > > > > > > capable of (~500K).
> > > > > > > 
> > > > > > > After searching through the various vioscsi registry settings, it
> > > > > > > appears that MSIEnabled is being explicitly disabled 
> > > > > > > (0x), that
> > > > > > > is different from what vioscsi.inx is currently defining:
> > > > > > > 
> > > > > > > [pnpsafe_pci_addreg_msix]
> > > > > > > HKR, "Interrupt Management",, 0x0010
> > > > > > > HKR, "Interrupt Management\MessageSignaledInterruptProperties",, 
> > > > > > > 0x0010
> > > > > > > HKR, "Interrupt Management\MessageSignaledInterruptProperties", 
> > > > > > > MSISupported, 0x00010001, 0
> > > > > > > HKR, "Interrupt Management\MessageSignaledInterruptProperties", 
> > > > > > > MessageNumberLimit, 0x00010001, 4
> > > > > > > 
> > > > > > > Looking deeper at vioscsi.c code, I've noticed that 
> > > > > > > MSI_SUPPORTED=0 is
> > > > > > > explicitly disabled at build time in SOURCES + vioscsi.vcxproj, 
> > > > > > > as well
> > > > > > > as VioScsiFindAdapter() code always ends setting msix_enabled = 
> > > > > > > FALSE
> > > > > > > here, regardless of MSI_SUPPORTED:
> > > > > > > 
> > > > > > >  
> > > > > > > https://github.com/YanVugenfirer/kvm-guest-drivers-windows/blob/master/vioscsi/vioscsi.c#L340
> > > > > > > 
> > > > > > > Also looking at virtio_stor.c for the raw block driver, 
> > > > > > > MSI_SUPPORTED=1
> > > > > > > appears to be the default setting for the driver included in the 
> > > > > > > offical
> > > > > > > virtio-win iso builds, right..?
> > > > > > > 
> > > > > > > Sooo, I'd like to try enabling MSI_SUPPORTED=1 in a test 
> > > > > > > vioscsi.sys
> > > > > > > build of my own, but before going down the WDK development rabbit 
> > > > > > > whole,
> > > > > > > I'd like to better understand why you've explicitly disabled this 
> > > > > > > logic
> > > > > > > within vioscsi.c code to start..?
> > > > > > > 
> > > > > > > Is there anything that needs to be addressed / carried over from
> > > > > > > virtio_stor.c in order to get MSI_SUPPORTED=1 to work with 
> > > > > > > vioscsi.c
> > > > > > > miniport code..?
> > > > > 
> > > > > Hi Nicholas,
> > > > > 
> > > > > I was thinking about enabling MSI in RHEL 6.6 (build 74) but for some
> > > > > reasons decided to keep it disabled until adding mq support.
> > > > > 
> > > > > 
> > > > > You definitely should be able to turn on MSI_SUPPORTED, rebuild the
> > > > > driver, and switch MSISupported to 1 to make vioscsi driver working in
> > > > > MSI mode.
> > > > >
> > > > 
> > > > Thanks for the quick response.  We'll give MSI_SUPPORTED=1 a shot over
> > > > the next days with a test build on Server 2012 / Server 2008 R2 and see
> > > > how things go..
> > > > 
> > > 
> > > Just a quick update on progress.
> > > 
> > > I've been able to successfully build + load a unsigned vioscsi.sys
> > > driver on Server 2012 with WDK 8.0.
> > > 
> > > Running with MSI_SUPPORTED=1 against vhost-scsi results in a significant
> > > performance and efficiency gain, on the order of 100K to 225K IOPs for
> > > 4K block random I/O workload, depending on read/write mix.
> > > 
> > > Below is a simple patch to enable MSI operation by default.  Any chance
> > > to apply this separate from future mq efforts..?
> > 
> > Yes, we differently can enable MSI and rebuild vioscsi.
> > But then we need to re-spin WHQL testing for this particular
> > driver. This process requires a lot of resources, and I doubt that
> > it will be initiated soon, unless we have some significant amount of
> > bug-fixes.
> > 
> 
> Any idea on a rough time frame to expect an official WHQL build with MSI
> enabled..?
In June for sure :)
> Or, would it be possible to generate some -BETA builds that are at least
> signed and don't require extra hoops to jump through for testing..?

It's doable. 
I hope we can make a new build next week.
Best regards,
Vadim.

> 
> Thanks again,
> 
> --nab
> 
> 

Re: [Qemu-devel] [PATCH] configure: check that C++ compiler actually works

[Alexey Kardashevskiy]

On 02/21/2014 02:58 AM, Thomas Huth wrote:
> On Thu, 20 Feb 2014 15:10:16 +
> Peter Maydell  wrote:
> 
>> Check that the C++ compiler works with the C compiler; if it
>> does not, then don't pass CXX to the build process. This
>> fixes a regression where QEMU was no longer building if the
>> build environment didn't have a C++ compiler (introduced
>> in commit 3144f78b, which incorrectly assumed that rules.mak
>> would only see a non-empty $(CXX) if configure had actually
>> found a working C++ compiler).
>>
>> Signed-off-by: Peter Maydell 
>> Reported-by: Alexey Kardashevskiy 
>> Reported-by: Thomas Huth 
>> ---
>> Apologies for the breakage for people who were building in
>> setups with no C++ compiler -- I should have tested the
>> original change more thoroughly.
>>
>>  configure | 29 +
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 4648117..6829cbb 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1289,6 +1289,35 @@ else
>>  error_exit "\"$cc\" either does not exist or does not work"
>>  fi
>>
>> +# Check that the C++ compiler exists and works with the C compiler
>> +if has $cxx; then
>> +cat > $TMPC <> +int c_function(void);
>> +int main(void) { return c_function(); }
>> +EOF
>> +
>> +compile_object
>> +
>> +cat > $TMPC <> +extern "C" {
>> +   int c_function(void);
>> +}
>> +int c_function(void) { return 42; }
>> +EOF
>> +
>> +if (cc=$cxx do_cc $QEMU_CFLAGS -o $TMPE $TMPC $TMPO $LDFLAGS); then
>> +# C++ compiler $cxx works ok with C compiler $cc
>> +:
>> +else
>> +echo "C++ compiler $cxx does not work with C compiler $cc"
>> +echo "Disabling C++ specific optional code"
>> +cxx=
>> +fi
>> +else
>> +echo "No C++ compiler available; disabling C++ specific optional code"
>> +cxx=
>> +fi
>> +
>>  # Consult white-list to determine whether to enable werror
>>  # by default.  Only enable by default for git builds
>>  z_version=`cut -f3 -d. $source_path/VERSION`
> 
> Works fine for me now on my system w/o C++ compiler:
> Tested-by: Thomas Huth 


Works for me too, in cross environment for ppc64 on x86_64,
Tested-by: Alexey Kardashevskiy 

Thanks.


-- 
Alexey

[Qemu-devel] [PATCH target-arm v8 1/1] target-arm: Implements the ARM PMCCNTR register

[Alistair Francis]

This patch implements the ARM PMCCNTR register including
the disable and reset components of the PMCR register.

Signed-off-by: Alistair Francis 
---
V8: Only implement the register in system mode to make sure it doesn't
break compilation of linux-user targets.
V7: Fixed a bug that caused the cycle count scaling to be determined
by the PMCRDP register instead of PMCRD. Also stopped PMCRDP from
disabling the counter. Thanks to Peter Maydell
V6: Rebase to include Peter Maydell's 'Convert performance monitor
reginfo to accesfn' patch. Remove the raw_fn's as the read/write
functions already do what is required.
V5: Implement the actual write function to make sure that
migration works correctly. Also includes the raw_read/write as
the normal read/write functions depend on the pmcr register. So
they don't allow for the pmccntr register to be written first.
V4: Some bug fixes pointed out by Peter Crosthwaite. Including
increasing the accuracy of the timer.
V3: Fixed up incorrect reset, disable and enable handling that
was submitted in V2. The patch should now also handle changing
of the clock scaling.
V2: Incorporated the comments that Peter Maydell and Peter
Crosthwaite had. Now the implementation only requires one
CPU state

 target-arm/cpu.h|4 ++
 target-arm/helper.c |   89 ++-
 2 files changed, 92 insertions(+), 1 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 3c8a2db..14fd1ae 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -215,6 +215,10 @@ typedef struct CPUARMState {
 uint32_t c15_diagnostic; /* diagnostic register */
 uint32_t c15_power_diagnostic;
 uint32_t c15_power_control; /* power control */
+/* If the counter is enabled, this stores the last time the counter
+ * was reset. Otherwise it stores the counter value
+ */
+uint32_t c15_ccnt;
 } cp15;
 
 struct {
diff --git a/target-arm/helper.c b/target-arm/helper.c
index b547f04..923bf2c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -13,6 +13,13 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t 
address,
 target_ulong *page_size);
 #endif
 
+/* Definitions for the PMCCNTR and PMCR registers */
+#ifndef CONFIG_LINUX_USER
+#define PMCRD   0x8
+#define PMCRC   0x4
+#define PMCRE   0x1
+#endif
+
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
 {
 int nregs;
@@ -478,9 +485,42 @@ static CPAccessResult pmreg_access(CPUARMState *env, const 
ARMCPRegInfo *ri)
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
 {
+/* Don't computer the number of ticks in user mode */
+#ifndef CONFIG_LINUX_USER
+uint32_t temp_ticks;
+
+temp_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
+  get_ticks_per_sec() / 100;
+
+if (env->cp15.c9_pmcr & PMCRE) {
+/* If the counter is enabled */
+if (env->cp15.c9_pmcr & PMCRD) {
+/* Increment once every 64 processor clock cycles */
+env->cp15.c15_ccnt = (temp_ticks/64) - env->cp15.c15_ccnt;
+} else {
+env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+}
+}
+
+if (value & PMCRC) {
+/* The counter has been reset */
+env->cp15.c15_ccnt = 0;
+}
+#endif
+
 /* only the DP, X, D and E bits are writable */
 env->cp15.c9_pmcr &= ~0x39;
 env->cp15.c9_pmcr |= (value & 0x39);
+
+#ifndef CONFIG_LINUX_USER
+if (env->cp15.c9_pmcr & PMCRE) {
+if (env->cp15.c9_pmcr & PMCRD) {
+/* Increment once every 64 processor clock cycles */
+temp_ticks /= 64;
+}
+env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+}
+#endif
 }
 
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -536,6 +576,49 @@ static void vbar_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 env->cp15.c12_vbar = value & ~0x1Ful;
 }
 
+/* Only declare the pmccntr registers in system mode */
+#ifndef CONFIG_LINUX_USER
+static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+uint32_t total_ticks;
+
+if (!(env->cp15.c9_pmcr & PMCRE)) {
+/* Counter is disabled, do not change value */
+return env->cp15.c15_ccnt;
+}
+
+total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
+  get_ticks_per_sec() / 100;
+
+if (env->cp15.c9_pmcr & PMCRD) {
+/* Increment once every 64 processor clock cycles */
+total_ticks /= 64;
+}
+return total_ticks - env->cp15.c15_ccnt;
+}
+
+static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+uint64_t value)
+{
+uint32_t total_ticks;
+
+if (!(env->cp15.c9_pmcr & PMCRE)) {
+/* Counter is disabled, set the absolute value */
+env->cp15.c15_ccnt = value;
+return;
+}
+
+total_ticks = qemu

Re: [Qemu-devel] [PATCH target-arm v7 1/1] target-arm: Implements the ARM PMCCNTR register

[Alistair Francis]

On Fri, Feb 21, 2014 at 10:07 AM, Peter Maydell
 wrote:
> On 20 February 2014 23:58, Alistair Francis  
> wrote:
>> On Fri, Feb 21, 2014 at 1:44 AM, Peter Maydell  
>> wrote:
>>> This breaks compilation of the arm-linux-user target
>>> (the qemu_clock_get_* functions are only available in
>>> system mode). You probably want to do something similar
>>> to how we handle the generic timers, where we #ifdef
>>> everything out. (I don't think Linux gives user
>>> space access to the perf monitor registers.)
>
>> What do you think the functions should do when in user space? Should
>> they just return 0?
>
> Just don't define reginfo entries for them at all -- then we will
> UNDEF because the lookup will return no result. This is what
> the generic timer code does.
>
> thanks
> -- PMM
>

Ok, so if it is user mode, it just treats the pmccntr register exactly
as it does before the patch. I've implemented that and it seems to
work. I'll submit the patch now

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Brad Smith]

On 20/02/14 7:09 PM, Peter Maydell wrote:

On 20 February 2014 23:55, Brad Smith  wrote:

On 20/02/14 12:50 PM, Peter Maydell wrote:


Win32 doesn't have a cpuid.h, and MacOSX may have one but without
the __cpuid() function we use, which means that commit 9d2eec20
broke the build for those platforms. Fix this by tightening up
our configure cpuid.h check to test that the functions we need
are present, and adding some missing #ifdef guerds in
tcg/i386/tcg-target.c.



The build will also fail if not using fairly new GCC


Do you happen to know how new 'fairly new' is? My stock compile
is with gcc 4.6.something, which isn't a spring chicken any more,
and that worked OK.

(We're going to fix this anyway, so it's just for my curiosity.)


Ok, it wasn't as new as I had thought. Actually taking a look at the
various releases __cpuid was added with GCC 4.3. __cpuid_count was
added with GCC 4.4.


--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union

[Wenchao Xia]

于 2014/2/21 0:38, Markus Armbruster 写道:
> Wenchao Xia  writes:
> 
>> By default, any union will automatically generate a enum type as
>> "[UnionName]Kind" in C code, and it is duplicated when the discriminator
>> is specified as a pre-defined enum type in schema. After this patch,
>> the pre-defined enum type will be really used as the switch case
>> condition in generated C code, if discriminator is an enum field.
>>
>> Signed-off-by: Wenchao Xia 
>> ---
>>   docs/qapi-code-gen.txt |8 ++--
>>   scripts/qapi-types.py  |   20 
>>   scripts/qapi-visit.py  |   27 ---
>>   scripts/qapi.py|   13 -
>>   4 files changed, 54 insertions(+), 14 deletions(-)
>>
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index 0728f36..a2e7921 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -123,11 +123,15 @@ And it looks like this on the wire:
>>   
>>   Flat union types avoid the nesting on the wire. They are used whenever a
>>   specific field of the base type is declared as the discriminator ('type' is
>> -then no longer generated). The discriminator must always be a string field.
>> +then no longer generated). The discriminator can be a string field or a
>> +predefined enum field. If it is a string field, a hidden enum type will be
>> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time 
>> check
>> +will be done to verify the correctness. It is recommended to use an enum 
>> field.
>>   The above example can then be modified as follows:
>>   
>> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
>>{ 'type': 'BlockdevCommonOptions',
>> -   'data': { 'driver': 'str', 'readonly': 'bool' } }
>> +   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
>>{ 'union': 'BlockdevOptions',
>>  'base': 'BlockdevCommonOptions',
>>  'discriminator': 'driver',
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index 656a9a0..4098c60 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -201,14 +201,22 @@ def generate_union(expr):
>>   base = expr.get('base')
>>   discriminator = expr.get('discriminator')
>>   
>> +expr_elem = {'expr': expr}
>> +enum_define = discriminator_find_enum_define(expr_elem)
> 
> expr_elem has no fp, line.  What if discriminator_find_enum_define
> throws a QAPIExprError?
> 
  It shouldn't happen, since all error check happens in parse_schema().

> More of the same below.
> 
>> +if enum_define:
>> +discriminator_type_name = enum_define['enum_name']
>> +else:
>> +discriminator_type_name = '%sKind' % (name)
>> +
>>   ret = mcgen('''
>>   struct %(name)s
>>   {
>> -%(name)sKind kind;
>> +%(discriminator_type_name)s kind;
>>   union {
>>   void *data;
>>   ''',
>> -name=name)
>> +name=name,
>> +discriminator_type_name=discriminator_type_name)
>>   
>>   for key in typeinfo:
>>   ret += mcgen('''
>> @@ -389,8 +397,12 @@ for expr in exprs:
>>   fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>>   elif expr.has_key('union'):
>>   ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>> -ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
>> -fdef.write(generate_enum_lookup('%sKind' % expr['union'], 
>> expr['data'].keys()))
>> +expr_elem = {'expr': expr}
>> +enum_define = discriminator_find_enum_define(expr_elem)
>> +if not enum_define:
>> +ret += generate_enum('%sKind' % expr['union'], 
>> expr['data'].keys())
>> +fdef.write(generate_enum_lookup('%sKind' % expr['union'],
>> +expr['data'].keys()))
> 
> Generate the implicit enum only when we don't have an explicit enum
> discriminator.  Good.
> 
>>   if expr.get('discriminator') == {}:
>>   fdef.write(generate_anon_union_qtypes(expr))
>>   else:
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 87e6df7..08685a7 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -260,10 +260,17 @@ def generate_visit_union(expr):
>>   assert not base
>>   return generate_visit_anon_union(name, members)
>>   
>> -# There will always be a discriminator in the C switch code, by default 
>> it
>> -# is an enum type generated silently as "'%sKind' % (name)"
>> -ret = generate_visit_enum('%sKind' % name, members.keys())
>> -discriminator_type_name = '%sKind' % (name)
>> +expr_elem = {'expr': expr}
>> +enum_define = discriminator_find_enum_define(expr_elem)
>> +if enum_define:
>> +# Use the predefined enum type as discriminator
>> +ret = ""
>> +discriminator_type_name = enum_define['enum_name']
>> +else:
>> +# There will always be a discriminator in the C switch co

Re: [Qemu-devel] [PATCH V11 0/8] qcow2: rollback the modification on fail in snapshot creation

[Wenchao Xia]

Hello, is this patch OK to be merged?

Re: [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing

[Wenchao Xia]

于 2014/2/20 20:22, Markus Armbruster 写道:
> Wenchao Xia  writes:
> 
>> Before this patch, 'QAPISchemaError' scans whole input until 'pos'
>> to get error line number. After this patch, the scan is avoided since
>> line number is remembered in schema parsing. This patch also benefits
>> other error report functions, which would be introduced later.
> 
> Not sure avoiding the scan is worthwhile, but since you coded it
> already...  no objections.
> 
>>
>> Signed-off-by: Wenchao Xia 
>> ---
>>   scripts/qapi.py |   14 --
>>   1 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 3732fe1..c504eb4 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -39,12 +39,10 @@ class QAPISchemaError(Exception):
>>   def __init__(self, schema, msg):
>>   self.fp = schema.fp
>>   self.msg = msg
>> -self.line = self.col = 1
>> -for ch in schema.src[0:schema.pos]:
>> -if ch == '\n':
>> -self.line += 1
>> -self.col = 1
>> -elif ch == '\t':
>> +self.col = 1
>> +self.line = schema.line
>> +for ch in schema.src[schema.line_pos:schema.pos]:
>> +if ch == '\t':
>>   self.col = (self.col + 7) % 8 + 1
> 
> Column computation is wrong.  Should be something like
> 
> self.col = ((self.col + 7) & ~7) + 1
> 
> Not your fault, of course, and you don't have to fix it to get my R-by.
> If you want to fix it, separate patch, and please include suitable
> tests.
> 
  Thanks for your quick review, I'll respin it next week.

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Peter Maydell]

On 20 February 2014 23:55, Brad Smith  wrote:
> On 20/02/14 12:50 PM, Peter Maydell wrote:
>>
>> Win32 doesn't have a cpuid.h, and MacOSX may have one but without
>> the __cpuid() function we use, which means that commit 9d2eec20
>> broke the build for those platforms. Fix this by tightening up
>> our configure cpuid.h check to test that the functions we need
>> are present, and adding some missing #ifdef guerds in
>> tcg/i386/tcg-target.c.
>
>
> The build will also fail if not using fairly new GCC

Do you happen to know how new 'fairly new' is? My stock compile
is with gcc 4.6.something, which isn't a spring chicken any more,
and that worked OK.

(We're going to fix this anyway, so it's just for my curiosity.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v7 1/1] target-arm: Implements the ARM PMCCNTR register

[Peter Maydell]

On 20 February 2014 23:58, Alistair Francis  wrote:
> On Fri, Feb 21, 2014 at 1:44 AM, Peter Maydell  
> wrote:
>> This breaks compilation of the arm-linux-user target
>> (the qemu_clock_get_* functions are only available in
>> system mode). You probably want to do something similar
>> to how we handle the generic timers, where we #ifdef
>> everything out. (I don't think Linux gives user
>> space access to the perf monitor registers.)

> What do you think the functions should do when in user space? Should
> they just return 0?

Just don't define reginfo entries for them at all -- then we will
UNDEF because the lookup will return no result. This is what
the generic timer code does.

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v7 1/1] target-arm: Implements the ARM PMCCNTR register

[Alistair Francis]

On Fri, Feb 21, 2014 at 1:44 AM, Peter Maydell  wrote:
> On 19 February 2014 23:57, Alistair Francis  
> wrote:
>> This patch implements the ARM PMCCNTR register including
>> the disable and reset components of the PMCR register.
>>
>> Signed-off-by: Alistair Francis 
>
> This breaks compilation of the arm-linux-user target
> (the qemu_clock_get_* functions are only available in
> system mode). You probably want to do something similar
> to how we handle the generic timers, where we #ifdef
> everything out. (I don't think Linux gives user
> space access to the perf monitor registers.)
>
> Other than that I think it looks good now.
>
> thanks
> -- PMM
>

What do you think the functions should do when in user space? Should
they just return 0?

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Brad Smith]

On 20/02/14 12:50 PM, Peter Maydell wrote:

Win32 doesn't have a cpuid.h, and MacOSX may have one but without
the __cpuid() function we use, which means that commit 9d2eec20
broke the build for those platforms. Fix this by tightening up
our configure cpuid.h check to test that the functions we need
are present, and adding some missing #ifdef guerds in
tcg/i386/tcg-target.c.


The build will also fail if not using fairly new GCC or LLVM/Clang 3.4
or newer.

--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [Qemu-devel] [PATCH] qom/cpu: Remove cpu->exit_request from reset state

[Peter Maydell]

On 20 February 2014 23:26, Edgar E. Iglesias  wrote:
> On Thu, Feb 20, 2014 at 03:58:15PM +, Peter Maydell wrote:
>> However, doesn't this also apply to interrupt_request ?
>> If we have a pending asserted interrupt on the CPU
>> (ie the IRQ line into the chip is being held high)
>> this should result in an interrupt as soon as the
>> CPU reenables interrupts after reset, I would have
>> thought. Clearing cpu->interrupt_request here will
>> make us drop it on the floor.

> I'm not sure about interrupt_request, seems to be a bit of
> a mix. For example, I' not sure it's safe to keep all
> the CPU_INTERRUPT_TGT_INT_X bits alive across a reset?
> Agree with you about the interrupt hard line though.

Mm, could probably use an audit of the uses of interrupt_request.
I suspect that one doesn't matter so much in practice because
a guest dealing with a reset CPU is going to either (a) ensure
that any sources of interrupts are cleared before it unmasks
them or (b) have arranged that nobody sends it interrupts
during the reset in the first place. It would be nice to go through
and check we can avoid the reset of interrupt_request, but
we don't need to delay this patch for that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] qom/cpu: Remove cpu->exit_request from reset state

[Edgar E. Iglesias]

On Thu, Feb 20, 2014 at 03:58:15PM +, Peter Maydell wrote:
> On 16 February 2014 02:07, Edgar E. Iglesias  wrote:
> > On Sat, Feb 15, 2014 at 03:42:56PM +, Peter Maydell wrote:
> >> On 13 February 2014 05:07,   wrote:
> >> > From: "Edgar E. Iglesias" 
> >> >
> >> > cpu->exit_request is part of the execution environment and should
> >> > not be cleared when a CPU resets.
> >> >
> >> > Otherwise, we might deadlock QEMU if a CPU resets while there is
> >> > I/O going on.
> >> >
> >> > Signed-off-by: Edgar E. Iglesias 
> >> > ---
> >> >  qom/cpu.c | 1 -
> >> >  1 file changed, 1 deletion(-)
> >> >
> >> > diff --git a/qom/cpu.c b/qom/cpu.c
> >> > index 9d62479..40d82dd 100644
> >> > --- a/qom/cpu.c
> >> > +++ b/qom/cpu.c
> >> > @@ -195,7 +195,6 @@ static void cpu_common_reset(CPUState *cpu)
> >> >  log_cpu_state(cpu, cc->reset_dump_flags);
> >> >  }
> >> >
> >> > -cpu->exit_request = 0;
> >> >  cpu->interrupt_request = 0;
> >> >  cpu->current_tb = NULL;
> >> >  cpu->halted = 0;
> >>
> >> This looks kind of odd to me. What's the situation you see where
> >> this matters -- is the CPU resetting itself, or is some other device
> >> in another thread triggering the CPU reset? TCG or KVM?
> >
> > Seeing this in TCG. The CPU gets signaled by the IO thread while the
> > CPU is resetting itself. If the CPU looses the race, it clears its
> > exit_request leaving the IO thread waiting for the global lock
> > potentially forever.
> >
> > The CPU actually exits generated code but goes right back in because
> > there is no exit_request pending.
> 
> Yes, having looked at the code I agree with you, so:
> 
> Reviewed-by: Peter Maydell 
> 
> However, doesn't this also apply to interrupt_request ?
> If we have a pending asserted interrupt on the CPU
> (ie the IRQ line into the chip is being held high)
> this should result in an interrupt as soon as the
> CPU reenables interrupts after reset, I would have
> thought. Clearing cpu->interrupt_request here will
> make us drop it on the floor.

Hi,

I'm not sure about interrupt_request, seems to be a bit of
a mix. For example, I' not sure it's safe to keep all
the CPU_INTERRUPT_TGT_INT_X bits alive across a reset?
Agree with you about the interrupt hard line though.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

On 02/20/2014 02:47 PM, Peter Maydell wrote:
> On 20 February 2014 22:36, Mario Smarduch  wrote:
>> On 02/20/2014 02:10 PM, Peter Maydell wrote:
>>> On 20 February 2014 21:59, Mario Smarduch  wrote:
 On 02/20/2014 11:35 AM, Peter Maydell wrote:
> On 20 February 2014 19:09, Mario Smarduch  wrote:
>> host features since you don't know what virtio device will be plugged
>> in later.
>
> I think this function is the right place to set these properties,
> yes. What I'm saying is that I don't see why you're doing it
> this way rather than using the existing per-backend hook.
> Maybe there's a reason not to use that hook, but you don't say.
>

 Appears virtio-net beckend hooks are common to several transports,
 and would require virtio-mmio exception to set the host_features.
 If I'm missing something please recommend.
>>>
>>> Yes, they're supposed to be common across transports, because
>>> the backend isn't supposed to care about which transport in
>>> particular. If there's a condition where the backend needs to
>>> do something which only happens for one transport, maybe we
>>> need a new hook.
>>
>> So something like set_transport_features(...) in VirtiIODeviceClass,
>> and call it from the realize hook where you can access
>> the virtio-mmio transport class instance.
> 
> Not sure. You should probably also look at whether connecting
> a virtio-pci transport to a virtio-net backend gets these feature
> bits wrong (that's different from instantiating a single virtio-net-pci
> device). I suspect they both get this wrong and this isn't particularly
> virtio-mmio specific.
> 
> thanks
> -- PMM
> 

It appears separate pci transport/virtio-net backend have the same issue.
I'll look at this closer for a more generic solution.

Thanks,
  Mario

Re: [Qemu-devel] [RFH] Qemu main thread is blocked in g_poll in windows guest

[Andrey Korolyov]

On 10/15/2013 04:18 PM, Xiexiangyou wrote:
> Thanks for your reply :-)
> The QEMU version is 1.5.1,and the KVM version is 3.6
> 
> QEMU command:
> /usr/bin/qemu-kvm -name win2008_dc_5 -S -machine 
> pc-i440fx-1.5,accel=kvm,usb=off -m 2048 -realtime mlock=off -smp 
> 4,maxcpus=64,sockets=16,cores=4,threads=1 -uuid 
> 13e08e3e-cd23-4450-8bd3-60e7c220316d -no-user-config -nodefaults -chardev 
> socket,id=charmonitor,path=/var/lib/libvirt/qemu/win2008_dc_5.monitor,server,nowait
>  -mon chardev=charmonitor,id=monitor,mode=control -rtc 
> base=utc,clock=vm,driftfix=slew -no-hpet -no-shutdown -device 
> piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device 
> virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x4 -device 
> virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive 
> file=/dev/vmdisk/win2008_dc_5,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=none,aio=native
>  -device 
> scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1
>  -netdev tap,fd=28,id=hostnet0,vhost=on,vhostfd=29 -device 
> virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:16:49:23,bus=pci.0,addr=0x3
>  -chardev socket,id=charchannel0,path=/var/run/libvirt/qe
 m
u/win2008_dc_5.extend,server,nowait -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.1
 -chardev 
socket,id=charchannel1,path=/var/run/libvirt/qemu/win2008_dc_5.agent,server,nowait
 -device 
virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=org.qemu.guest_agent.0
 -device usb-tablet,id=input0 -vnc 0.0.0.0:4 -device 
cirrus-vga,id=video0,bus=pci.0,addr=0x2 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6
> 
> (gdb) bt
> #0  0x7f9ba661a423 in poll () from /lib64/libc.so.6
> #1  0x0059460f in os_host_main_loop_wait (timeout=4294967295) at 
> main-loop.c:226
> #2  0x005946a4 in main_loop_wait (nonblocking=0) at main-loop.c:464
> #3  0x00619309 in main_loop () at vl.c:2182
> #4  0x0061fb5e in main (argc=54, argv=0x7fff879830c8, 
> envp=0x7fff87983280) at vl.c:4611
> 
> Main thread's strace message:
> # strace -p 6386
> Process 6386 attached - interrupt to quit
> restart_syscall(<... resuming interrupted call ...>
> 
> cpu thread's strace message:
> # strace -p 6389
> Process 6389 attached - interrupt to quit
> rt_sigtimedwait([BUS USR1], 0x7f9ba36fbc00) = -1 EAGAIN (Resource temporarily 
> unavailable)
> rt_sigpending([])   = 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ioctl(17, 0xae80, 0)= 0
> ...
> 
> Thanks!
> --xie
> 
> -Original Message-
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo 
> Bonzini
> Sent: Tuesday, October 15, 2013 7:52 PM
> To: Xiexiangyou
> Cc: qemu-devel@nongnu.org; qemu-devel-requ...@nongnu.org; 
> k...@vger.kernel.org; Huangpeng (Peter); Luonengjun
> Subject: Re: [RFH] Qemu main thread is blocked in g_poll in windows guest
> 
> Il 15/10/2013 12:21, Xiexiangyou ha scritto:
>> Hi all:
>>
>> Windows2008 Guest run without pressure for long time. Sometimes, it
>> stop and looks like hanging. But when I connect to it with VNC, It
>> resume to run, but VM's time is delayed . When the vm is hanging, I
>> check the main thread of QEMU. I find that the thread is blocked in
>> g_poll function. it is waiting for a SIG, However, there is no SIG .
>>
>> I tried the clock with "hpet" and "no hpet", but came out the same
>> problem. Then I upgrade the glibc to newer, it didn't work too. I'm
>> confused. Is the reason that VM in sleep state and doesn't emit the
>> signal. I set the windows 's "power option", enable/disable the
>> "allow the wake timers", I didn't work.
>>
>> Is anybody have met the same problem before, or know the reason. Your
>> reply will be very helpful.
> 
> This post is missing a few pieces of information:
> 
> * What version of QEMU is this?
> 
> * What is the command line?
> 
> * How do you know g_poll is waiting for a signal and not for a file
> descriptor?
> 
> * What is the backtrace of the main thread?  What is the backtrace of
> the VCPU thread?
> 
> etc.
> 
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


Hello,

To revive this thread - I have exactly the same problem on freshly
migrated virtual ma

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 22:36, Mario Smarduch  wrote:
> On 02/20/2014 02:10 PM, Peter Maydell wrote:
>> On 20 February 2014 21:59, Mario Smarduch  wrote:
>>> On 02/20/2014 11:35 AM, Peter Maydell wrote:
 On 20 February 2014 19:09, Mario Smarduch  wrote:
> host features since you don't know what virtio device will be plugged
> in later.

 I think this function is the right place to set these properties,
 yes. What I'm saying is that I don't see why you're doing it
 this way rather than using the existing per-backend hook.
 Maybe there's a reason not to use that hook, but you don't say.

>>>
>>> Appears virtio-net beckend hooks are common to several transports,
>>> and would require virtio-mmio exception to set the host_features.
>>> If I'm missing something please recommend.
>>
>> Yes, they're supposed to be common across transports, because
>> the backend isn't supposed to care about which transport in
>> particular. If there's a condition where the backend needs to
>> do something which only happens for one transport, maybe we
>> need a new hook.
>
> So something like set_transport_features(...) in VirtiIODeviceClass,
> and call it from the realize hook where you can access
> the virtio-mmio transport class instance.

Not sure. You should probably also look at whether connecting
a virtio-pci transport to a virtio-net backend gets these feature
bits wrong (that's different from instantiating a single virtio-net-pci
device). I suspect they both get this wrong and this isn't particularly
virtio-mmio specific.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

On 02/20/2014 02:10 PM, Peter Maydell wrote:
> On 20 February 2014 21:59, Mario Smarduch  wrote:
>> On 02/20/2014 11:35 AM, Peter Maydell wrote:
>>> On 20 February 2014 19:09, Mario Smarduch  wrote:
 host features since you don't know what virtio device will be plugged
 in later.
>>>
>>> I think this function is the right place to set these properties,
>>> yes. What I'm saying is that I don't see why you're doing it
>>> this way rather than using the existing per-backend hook.
>>> Maybe there's a reason not to use that hook, but you don't say.
>>>
>>
>> Appears virtio-net beckend hooks are common to several transports,
>> and would require virtio-mmio exception to set the host_features.
>> If I'm missing something please recommend.
> 
> Yes, they're supposed to be common across transports, because
> the backend isn't supposed to care about which transport in
> particular. If there's a condition where the backend needs to
> do something which only happens for one transport, maybe we
> need a new hook.

So something like set_transport_features(...) in VirtiIODeviceClass,
and call it from the realize hook where you can access
the virtio-mmio transport class instance.

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] hw/arm/spitz: Avoid clash with Windows header symbol MOD_SHIFT

[Peter Maydell]

On 20 February 2014 21:26, Stefan Weil  wrote:
> There was a suggestion to remove some dependencies on windows.h (which
> causes the trouble here). I recently started doing this, and that
> approach fixes the warning, too. Maybe I can send a patch next weekend.

That would probably be the nicest approach, yes. I won't apply
this to the target-arm queue just yet.

I'm wondering if we might be able to get to the point of enabling
-Werror for Windows and MacOSX builds. Do you think that would
be a useful thing to do? (Obviously for git builds only, same as Linux.)

(For MacOSX there are some deprecation warnings in the audio
code I need to fix first but we otherwise compile pretty much
warning free on 10.8. I'd be reliant on reports from other people
for warnings on 10.9 and on earlier-than-10.8, though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Peter Maydell]

On 20 February 2014 21:18, Stefan Weil  wrote:
> MinGW-w64's gcc has cpuid.h, so my 32 and 64 bit cross builds work
> without problems. We can use that code for MinGW, too, but we could also
> stop supporting MinGW (which has several other deficits).

We need the conditionals for MacOSX builds anyway, so we
don't need to drop MinGW for this. (I compile with the 32 bit
version rather than -w64 because I was able to get that to
install on my Ubuntu box, whereas the -w64 seemed to have
dependency issues/conflicts somehow. I figured the 32 bit
version was good enough for detecting the typical "long is
a funny size and we don't build" issues.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-ppc/translate.c: Use ULL suffix for 64 bit constants

[Peter Maydell]

On 20 February 2014 21:33, Stefan Weil  wrote:
> (I personally prefer ULL instead of ull because it avoids constants
> ending with 'full', and if we ignore the disas code, ULL is also more
> common in QEMU)

I agree, as it happens, but I opted to stick with the existing case here.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 21:59, Mario Smarduch  wrote:
> On 02/20/2014 11:35 AM, Peter Maydell wrote:
>> On 20 February 2014 19:09, Mario Smarduch  wrote:
>>> host features since you don't know what virtio device will be plugged
>>> in later.
>>
>> I think this function is the right place to set these properties,
>> yes. What I'm saying is that I don't see why you're doing it
>> this way rather than using the existing per-backend hook.
>> Maybe there's a reason not to use that hook, but you don't say.
>>
>
> Appears virtio-net beckend hooks are common to several transports,
> and would require virtio-mmio exception to set the host_features.
> If I'm missing something please recommend.

Yes, they're supposed to be common across transports, because
the backend isn't supposed to care about which transport in
particular. If there's a condition where the backend needs to
do something which only happens for one transport, maybe we
need a new hook.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

On 02/20/2014 11:35 AM, Peter Maydell wrote:
> On 20 February 2014 19:09, Mario Smarduch  wrote:
>> host features since you don't know what virtio device will be plugged
>> in later.
> 
> I think this function is the right place to set these properties,
> yes. What I'm saying is that I don't see why you're doing it
> this way rather than using the existing per-backend hook.
> Maybe there's a reason not to use that hook, but you don't say.
> 

Appears virtio-net beckend hooks are common to several transports, 
and would require virtio-mmio exception to set the host_features. 
If I'm missing something please recommend.

>> It's virtio_net_properties[] can only set virtio-net
>> properites when it's instantiated. I think the proper way would
>> be to refactor virtio-mmio to similar structure PCI version uses then
>> one virtio_net_properties[] can be used selecting PCI or virtio-mmio at
>> compile time. But right now it's unclear to me how pci and virtio-mmio
>> versions intend to co-exist.

> 
> This is the result of a refactoring last year to allow virtio-mmio.
> Basically the underlying structure now is:
> 
>  [some virtio transport device] <- virtio bus -> [virtio backend]
> 
> where the transport could be mmio, PCI, CCW, etc, and the
> backend is net, blk, balloon, etc. We also provide devices
> which are "virtio PCI transport plus a specific backend"
> for (a) user convenience and (b) backwards compatibility, since
> the pre-refactoring structure put the transport and backend
> all in one lump. The all-in-one-lump arrangement is legacy:
> we shouldn't break it, but it's not the model for virtio-mmio
> to follow either.
> 
> The transport shouldn't know anything specific about the
> backend it's plugged into (or vice-versa), but it's fine
> for there to be hook functions so the transport and backend
> can call each other at the appropriate times.
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] trace: Fix build warnings for Win32 build

[Stefan Weil]

Am 20.02.2014 20:44, schrieb Peter Maydell:
> The Win32 build warns about trace/control-internal.h:
> 
> warning: 'trace_event_count' declared inline after being called
> 
> Fix this by simply reordering trace_event_id() and
> trace_event_count().
> 
> Signed-off-by: Peter Maydell 
> ---
>  trace/control-internal.h | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)


Reviewed-by: Stefan Weil 

This is not OS specific, but depends on the compiler. MinGW-w64 uses a
newer gcc which does not complain. Reordering the two function is
reasonable nevertheless.

Regards
Stefan

Re: [Qemu-devel] [PATCH] target-ppc/translate.c: Use ULL suffix for 64 bit constants

[Stefan Weil]

Am 20.02.2014 20:47, schrieb Peter Maydell:
> 64 bit constants need the "ULL" suffix, not just "UL", because
> on 32 bit platforms 'long' is not large enough and this will
> cause a compiler warning.
> 
> Signed-off-by: Peter Maydell 
> ---
> I think plain "UL" as a suffix is pretty much never right;
> it should either be "U" or "ULL".
> 
>  target-ppc/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index c5c1108..54013a2 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -7179,8 +7179,8 @@ static void gen_xxpermdi(DisasContext *ctx)
>  #define OP_NABS 2
>  #define OP_NEG 3
>  #define OP_CPSGN 4
> -#define SGN_MASK_DP  0x8000ul
> -#define SGN_MASK_SP 0x80008000ul
> +#define SGN_MASK_DP  0x8000ull
> +#define SGN_MASK_SP 0x80008000ull
>  
>  #define VSX_SCALAR_MOVE(name, op, sgn_mask)   \
>  static void glue(gen_, name)(DisasContext * ctx)  \
> 


Reviewed-by: Stefan Weil 

(I personally prefer ULL instead of ull because it avoids constants
ending with 'full', and if we ignore the disas code, ULL is also more
common in QEMU)

Re: [Qemu-devel] [PATCH] hw/arm/spitz: Avoid clash with Windows header symbol MOD_SHIFT

[Stefan Weil]

Am 20.02.2014 20:45, schrieb Peter Maydell:
> The Windows headers provided by MinGW define MOD_SHIFT. Avoid
> it by using SPITZ_MOD_* for our constants here.
> 
> Signed-off-by: Peter Maydell 
> ---
> The other approach would be just to #undef MOD_SHIFT, (and
> looking back through the archives I see Stefan posted a patch
> to do just that last year) but I think it's cleaner to do this.
> 
> I replaced a few of the /* apostrophe */ keysym names with
> the symbols just to keep us under the 80 column limit...
> ---
>  hw/arm/spitz.c | 108 
> +++--
>  1 file changed, 58 insertions(+), 50 deletions(-)


That old patch is still in my local queue :-)

There was a suggestion to remove some dependencies on windows.h (which
causes the trouble here). I recently started doing this, and that
approach fixes the warning, too. Maybe I can send a patch next weekend.

Regards
Stefan

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Stefan Weil]

Am 20.02.2014 18:50, schrieb Peter Maydell:
> Win32 doesn't have a cpuid.h, and MacOSX may have one but without
> the __cpuid() function we use, which means that commit 9d2eec20
> broke the build for those platforms. Fix this by tightening up
> our configure cpuid.h check to test that the functions we need
> are present, and adding some missing #ifdef guerds in
> tcg/i386/tcg-target.c.
> 
> Signed-off-by: Peter Maydell 
> ---
> Tested with Linux x86/64 gcc build, Linux x86/64 clang build,
> W32 cross-build and MacOSX 10.8 build. If somebody would like to
> review this I'll apply it directly to unbreak things.
> Apologies for not catching it before I pushed the tcg pullreq;
> I had forgotten to add the 'build on w32' command to my script.
> 


MinGW-w64's gcc has cpuid.h, so my 32 and 64 bit cross builds work
without problems. We can use that code for MinGW, too, but we could also
stop supporting MinGW (which has several other deficits).

Regards
Stefan

Re: [Qemu-devel] [PATCH v2] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Richard Henderson]

On 02/20/2014 01:42 PM, Peter Maydell wrote:
> Win32 doesn't have a cpuid.h, and MacOSX may have one but without
> the __cpuid() function we use, which means that commit 9d2eec20
> broke the build for those platforms. Fix this by tightening up
> our configure cpuid.h check to test that the functions we need
> are present, and adding some missing #ifdef guards in
> tcg/i386/tcg-target.c.
> 
> Signed-off-by: Peter Maydell 
> ---
> Changes v1->v2: fix typo in commit message, add __cpuid_count()
>  to the configure test
> 
>  configure | 13 -
>  tcg/i386/tcg-target.c |  4 +++-
>  2 files changed, 15 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 1/6] qemu-option: Introduce has_help_option()

[Jeff Cody]

On Thu, Feb 20, 2014 at 03:57:18PM +0100, Kevin Wolf wrote:
> This new function checks if any help option ('help' or '?') occurs
> anywhere in an option string, so that things like 'cluster_size=4k,help'
> are recognised.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Jeff Cody 

> ---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c| 24 
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 3ea871a..8d44167 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -79,6 +79,7 @@ void parse_option_size(const char *name, const char *value,
>  void free_option_parameters(QEMUOptionParameter *list);
>  void print_option_parameters(QEMUOptionParameter *list);
>  void print_option_help(QEMUOptionParameter *list);
> +bool has_help_option(const char *param);
>  
>  /* -- */
>  
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 668e5d9..ce1eba8 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -450,6 +450,30 @@ fail:
>  return NULL;
>  }
>  
> +bool has_help_option(const char *param)
> +{
> +size_t buflen = strlen(param) + 1;
> +char *buf = g_malloc0(buflen);
> +const char *p = param;
> +bool result = false;
> +
> +while (*p) {
> +p = get_opt_value(buf, buflen, p);
> +if (*p) {
> +p++;
> +}
> +
> +if (is_help_option(buf)) {
> +result = true;
> +goto out;
> +}
> +}
> +
> +out:
> +free(buf);
> +return result;
> +}
> +
>  /*
>   * Prints all options of a list that have a value to stdout
>   */
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH v2 5/6] qemu-img: Allow -o help with incomplete argument list

[Jeff Cody]

On Thu, Feb 20, 2014 at 03:57:22PM +0100, Kevin Wolf wrote:
> This patch allows using 'qemu-img $subcmd -o help' for the create,
> convert and amend subcommands, without specifying the previously
> required filename arguments.
> 
> Note that it's still allowed and meaningful to specify a filename: An
> invocation like 'qemu-img create -o help sheepdog:foo' will also display
> options that are provided by the Sheepdog driver.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-img.c | 58 +++---
>  1 file changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d6dc7ec..98ddbe7 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -250,16 +250,19 @@ static int print_block_option_help(const char 
> *filename, const char *fmt)
>  return 1;
>  }
>  
> -proto_drv = bdrv_find_protocol(filename, true);
> -if (!proto_drv) {
> -error_report("Unknown protocol '%s'", filename);
> -return 1;
> -}
> -
>  create_options = append_option_parameters(create_options,
>drv->create_options);
> -create_options = append_option_parameters(create_options,
> -  proto_drv->create_options);
> +
> +if (filename) {
> +proto_drv = bdrv_find_protocol(filename, true);
> +if (!proto_drv) {
> +error_report("Unknown protocol '%s'", filename);
> +return 1;
> +}
> +create_options = append_option_parameters(create_options,
> +  proto_drv->create_options);
> +}
> +
>  print_option_help(create_options);
>  free_option_parameters(create_options);
>  return 0;
> @@ -390,10 +393,16 @@ static int img_create(int argc, char **argv)
>  }
>  
>  /* Get the filename */
> +filename = (optind < argc) ? argv[optind] : NULL;
> +if (options && has_help_option(options)) {
> +g_free(options);
> +return print_block_option_help(filename, fmt);
> +}
> +

Same comment as patch #2 with regards to common cleanup code in
img_create().

Reviewed-by: Jeff Cody 

>  if (optind >= argc) {
>  help();
>  }
> -filename = argv[optind++];
> +optind++;
>  
>  /* Get image size, if specified */
>  if (optind < argc) {
> @@ -417,11 +426,6 @@ static int img_create(int argc, char **argv)
>  help();
>  }
>  
> -if (options && has_help_option(options)) {
> -g_free(options);
> -return print_block_option_help(filename, fmt);
> -}
> -
>  bdrv_img_create(filename, fmt, base_filename, base_fmt,
>  options, img_size, BDRV_O_FLAGS, &local_err, quiet);
>  if (error_is_set(&local_err)) {
> @@ -1260,17 +1264,18 @@ static int img_convert(int argc, char **argv)
>  }
>  
>  bs_n = argc - optind - 1;
> -if (bs_n < 1) {
> -help();
> -}
> -
> -out_filename = argv[argc - 1];
> +out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
>  
>  if (options && has_help_option(options)) {
>  ret = print_block_option_help(out_filename, out_fmt);
>  goto out;
>  }
>  
> +if (bs_n < 1) {
> +help();
> +}
> +
> +
>  if (bs_n > 1 && out_baseimg) {
>  error_report("-B makes no sense when concatenating multiple input "
>   "images");
> @@ -2675,15 +2680,21 @@ static int img_amend(int argc, char **argv)
>  }
>  }
>  
> -if (optind != argc - 1) {
> +if (!options) {
>  help();
>  }
>  
> -if (!options) {
> -help();
> +filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
> +if (fmt && has_help_option(options)) {
> +/* If a format is explicitly specified (and possibly no filename is
> + * given), print option help here */
> +ret = print_block_option_help(filename, fmt);
> +goto out;
>  }
>  
> -filename = argv[argc - 1];
> +if (optind != argc - 1) {
> +help();
> +}
>  
>  bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, 
> quiet);
>  if (!bs) {
> @@ -2695,6 +2706,7 @@ static int img_amend(int argc, char **argv)
>  fmt = bs->drv->format_name;
>  
>  if (has_help_option(options)) {
> +/* If the format was auto-detected, print option help here */
>  ret = print_block_option_help(filename, fmt);
>  goto out;
>  }
> -- 
> 1.8.1.4
> 
> 

[Qemu-devel] spapr_pci.c:spapr_pci_msi_init() creates memory region whose size is host-dependent

[Peter Maydell]

spapr_pci_msi_init() does this:

memory_region_init_io(&spapr->msiwindow, NULL, &spapr_msi_ops, spapr,
  "msi", getpagesize());

That means this device's memory region size will depend on
the host OS CPU and configuration, which seems like a bad idea,
especially if this machine is supposed to work with TCG.
It also means that on Win32 the compiler complains:

  CCppc64-softmmu/hw/ppc/spapr_pci.o
cc1: warnings being treated as errors
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c: In
function ‘spapr_pci_msi_init’:
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c:482:
warning: implicit declaration of function ‘getpagesize’
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c:482:
warning: nested extern declaration of ‘getpagesize’

since getpagesize() doesn't exist there.

Not sure which of the following is best:
 * use a fixed size for the memory region (eg "worst
   case page size for target CPU")
 * query the target CPU for its page size rather than the
   host OS/CPU
 * guard with suitable ifdefs if this code can't actually
   be used except with KVM
 * abstract out the "how do I find my page size on $OS?"
   check to an os-*.c file
 * something else

Any suggestions?

It would be nice to fix this because I think this is the last
Win32 compiler warning.

thanks
-- PMM

[Qemu-devel] [PATCH] target-ppc/translate.c: Use ULL suffix for 64 bit constants

[Peter Maydell]

64 bit constants need the "ULL" suffix, not just "UL", because
on 32 bit platforms 'long' is not large enough and this will
cause a compiler warning.

Signed-off-by: Peter Maydell 
---
I think plain "UL" as a suffix is pretty much never right;
it should either be "U" or "ULL".

 target-ppc/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index c5c1108..54013a2 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -7179,8 +7179,8 @@ static void gen_xxpermdi(DisasContext *ctx)
 #define OP_NABS 2
 #define OP_NEG 3
 #define OP_CPSGN 4
-#define SGN_MASK_DP  0x8000ul
-#define SGN_MASK_SP 0x80008000ul
+#define SGN_MASK_DP  0x8000ull
+#define SGN_MASK_SP 0x80008000ull
 
 #define VSX_SCALAR_MOVE(name, op, sgn_mask)   \
 static void glue(gen_, name)(DisasContext * ctx)  \
-- 
1.8.5

[Qemu-devel] [PATCH] hw/arm/spitz: Avoid clash with Windows header symbol MOD_SHIFT

[Peter Maydell]

The Windows headers provided by MinGW define MOD_SHIFT. Avoid
it by using SPITZ_MOD_* for our constants here.

Signed-off-by: Peter Maydell 
---
The other approach would be just to #undef MOD_SHIFT, (and
looking back through the archives I see Stefan posted a patch
to do just that last year) but I think it's cleaner to do this.

I replaced a few of the /* apostrophe */ keysym names with
the symbols just to keep us under the 80 column limit...
---
 hw/arm/spitz.c | 108 +++--
 1 file changed, 58 insertions(+), 50 deletions(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 2decff1..232a587 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -285,9 +285,9 @@ static void spitz_keyboard_keydown(SpitzKeyboardState *s, 
int keycode)
 spitz_keyboard_sense_update(s);
 }
 
-#define MOD_SHIFT   (1 << 7)
-#define MOD_CTRL(1 << 8)
-#define MOD_FN  (1 << 9)
+#define SPITZ_MOD_SHIFT   (1 << 7)
+#define SPITZ_MOD_CTRL(1 << 8)
+#define SPITZ_MOD_FN  (1 << 9)
 
 #define QUEUE_KEY(c)   s->fifo[(s->fifopos + s->fifolen ++) & 0xf] = c
 
@@ -324,21 +324,26 @@ static void spitz_keyboard_handler(void *opaque, int 
keycode)
 }
 
 code = s->pre_map[mapcode = ((s->modifiers & 3) ?
-(keycode | MOD_SHIFT) :
-(keycode & ~MOD_SHIFT))];
+(keycode | SPITZ_MOD_SHIFT) :
+(keycode & ~SPITZ_MOD_SHIFT))];
 
 if (code != mapcode) {
 #if 0
-if ((code & MOD_SHIFT) && !(s->modifiers & 1))
+if ((code & SPITZ_MOD_SHIFT) && !(s->modifiers & 1)) {
 QUEUE_KEY(0x2a | (keycode & 0x80));
-if ((code & MOD_CTRL ) && !(s->modifiers & 4))
+}
+if ((code & SPITZ_MOD_CTRL) && !(s->modifiers & 4)) {
 QUEUE_KEY(0x1d | (keycode & 0x80));
-if ((code & MOD_FN   ) && !(s->modifiers & 8))
+}
+if ((code & SPITZ_MOD_FN) && !(s->modifiers & 8)) {
 QUEUE_KEY(0x38 | (keycode & 0x80));
-if ((code & MOD_FN   ) && (s->modifiers & 1))
+}
+if ((code & SPITZ_MOD_FN) && (s->modifiers & 1)) {
 QUEUE_KEY(0x2a | (~keycode & 0x80));
-if ((code & MOD_FN   ) && (s->modifiers & 2))
+}
+if ((code & SPITZ_MOD_FN) && (s->modifiers & 2)) {
 QUEUE_KEY(0x36 | (~keycode & 0x80));
+}
 #else
 if (keycode & 0x80) {
 if ((s->imodifiers & 1   ) && !(s->modifiers & 1))
@@ -353,24 +358,27 @@ static void spitz_keyboard_handler(void *opaque, int 
keycode)
 QUEUE_KEY(0x36);
 s->imodifiers = 0;
 } else {
-if ((code & MOD_SHIFT) && !((s->modifiers | s->imodifiers) & 1)) {
+if ((code & SPITZ_MOD_SHIFT) &&
+!((s->modifiers | s->imodifiers) & 1)) {
 QUEUE_KEY(0x2a);
 s->imodifiers |= 1;
 }
-if ((code & MOD_CTRL ) && !((s->modifiers | s->imodifiers) & 4)) {
+if ((code & SPITZ_MOD_CTRL) &&
+!((s->modifiers | s->imodifiers) & 4)) {
 QUEUE_KEY(0x1d);
 s->imodifiers |= 4;
 }
-if ((code & MOD_FN   ) && !((s->modifiers | s->imodifiers) & 8)) {
+if ((code & SPITZ_MOD_FN) &&
+!((s->modifiers | s->imodifiers) & 8)) {
 QUEUE_KEY(0x38);
 s->imodifiers |= 8;
 }
-if ((code & MOD_FN   ) && (s->modifiers & 1) &&
+if ((code & SPITZ_MOD_FN) && (s->modifiers & 1) &&
 !(s->imodifiers & 0x10)) {
 QUEUE_KEY(0x2a | 0x80);
 s->imodifiers |= 0x10;
 }
-if ((code & MOD_FN   ) && (s->modifiers & 2) &&
+if ((code & SPITZ_MOD_FN) && (s->modifiers & 2) &&
 !(s->imodifiers & 0x20)) {
 QUEUE_KEY(0x36 | 0x80);
 s->imodifiers |= 0x20;
@@ -402,38 +410,38 @@ static void spitz_keyboard_pre_map(SpitzKeyboardState *s)
 int i;
 for (i = 0; i < 0x100; i ++)
 s->pre_map[i] = i;
-s->pre_map[0x02 | MOD_SHIFT] = 0x02 | MOD_SHIFT;   /* exclam */
-s->pre_map[0x28 | MOD_SHIFT] = 0x03 | MOD_SHIFT;   /* quotedbl */
-s->pre_map[0x04 | MOD_SHIFT] = 0x04 | MOD_SHIFT;   /* numbersign */
-s->pre_map[0x05 | MOD_SHIFT] = 0x05 | MOD_SHIFT;   /* dollar */
-s->pre_map[0x06 | MOD_SHIFT] = 0x06 | MOD_SHIFT;   /* percent */
-s->pre_map[0x08 | MOD_SHIFT] = 0x07 | MOD_SHIFT;   /* ampersand */
-s->pre_map[0x28] = 0x08 | MOD_SHIFT;   /* apostrophe */
-s->pre_map[0x0a | MOD_SHIFT] = 0x09 | MOD_SHIFT;   /* parenleft */
-s->pre_map[0x0b | MOD_SHIFT] = 0x0a | MOD_SHIFT;   /* parenright */
-s->pre_map[0x29 | MOD_SHIFT] = 0x0b | MOD_SHIFT;   /* asciitilde */
-s->pre_map[0x03 | MOD_SHIFT] = 0x0c | MOD_SHIFT;   /* at */
-s->pre_map[0xd3  

[Qemu-devel] [PATCH] trace: Fix build warnings for Win32 build

[Peter Maydell]

The Win32 build warns about trace/control-internal.h:

warning: 'trace_event_count' declared inline after being called

Fix this by simply reordering trace_event_id() and
trace_event_count().

Signed-off-by: Peter Maydell 
---
 trace/control-internal.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/trace/control-internal.h b/trace/control-internal.h
index cce2da4..b3f587e 100644
--- a/trace/control-internal.h
+++ b/trace/control-internal.h
@@ -16,15 +16,15 @@
 extern TraceEvent trace_events[];
 
 
-static inline TraceEvent *trace_event_id(TraceEventID id)
+static inline TraceEventID trace_event_count(void)
 {
-assert(id < trace_event_count());
-return &trace_events[id];
+return TRACE_EVENT_COUNT;
 }
 
-static inline TraceEventID trace_event_count(void)
+static inline TraceEvent *trace_event_id(TraceEventID id)
 {
-return TRACE_EVENT_COUNT;
+assert(id < trace_event_count());
+return &trace_events[id];
 }
 
 static inline bool trace_event_is_pattern(const char *str)
-- 
1.8.5

[Qemu-devel] [PATCH v2] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Peter Maydell]

Win32 doesn't have a cpuid.h, and MacOSX may have one but without
the __cpuid() function we use, which means that commit 9d2eec20
broke the build for those platforms. Fix this by tightening up
our configure cpuid.h check to test that the functions we need
are present, and adding some missing #ifdef guards in
tcg/i386/tcg-target.c.

Signed-off-by: Peter Maydell 
---
Changes v1->v2: fix typo in commit message, add __cpuid_count()
 to the configure test

 configure | 13 -
 tcg/i386/tcg-target.c |  4 +++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 4648117..79eb54c 100755
--- a/configure
+++ b/configure
@@ -3564,7 +3564,18 @@ cpuid_h=no
 cat > $TMPC << EOF
 #include 
 int main(void) {
-  return 0;
+unsigned a, b, c, d;
+int max = __get_cpuid_max(0, 0);
+
+if (max >= 1) {
+__cpuid(1, a, b, c, d);
+}
+
+if (max >= 7) {
+__cpuid_count(7, 0, a, b, c, d);
+}
+
+return 0;
 }
 EOF
 if compile_prog "" "" ; then
diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
index fef1717..f832282 100644
--- a/tcg/i386/tcg-target.c
+++ b/tcg/i386/tcg-target.c
@@ -115,7 +115,7 @@ static const int tcg_target_call_oarg_regs[] = {
is available.  */
 #if TCG_TARGET_REG_BITS == 64
 # define have_cmov 1
-#elif defined(CONFIG_CPUID_H)
+#elif defined(CONFIG_CPUID_H) && defined(bit_CMOV)
 static bool have_cmov;
 #else
 # define have_cmov 0
@@ -2295,6 +2295,7 @@ static void tcg_target_qemu_prologue(TCGContext *s)
 
 static void tcg_target_init(TCGContext *s)
 {
+#ifdef CONFIG_CPUID_H
 unsigned a, b, c, d;
 int max = __get_cpuid_max(0, 0);
 
@@ -2323,6 +2324,7 @@ static void tcg_target_init(TCGContext *s)
 have_bmi2 = (b & bit_BMI2) != 0;
 #endif
 }
+#endif
 
 if (TCG_TARGET_REG_BITS == 64) {
 tcg_regset_set32(tcg_target_available_regs[TCG_TYPE_I32], 0, 0x);
-- 
1.8.5

Re: [Qemu-devel] [PATCH v2 4/6] qemu-img amend: Support multiple -o options

[Jeff Cody]

On Thu, Feb 20, 2014 at 03:57:21PM +0100, Kevin Wolf wrote:
> Instead of ignoring all option values but the last one, multiple -o
> options now have the same meaning as having a single option with all
> settings in the order of their respective -o options.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Jeff Cody 

> ---
>  qemu-img.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index ba6e82d..d6dc7ec 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2658,7 +2658,13 @@ static int img_amend(int argc, char **argv)
>  help();
>  break;
>  case 'o':
> -options = optarg;
> +if (!options) {
> +options = g_strdup(optarg);
> +} else {
> +char *old_options = options;
> +options = g_strdup_printf("%s,%s", options, optarg);
> +g_free(old_options);
> +}
>  break;
>  case 'f':
>  fmt = optarg;
> @@ -2688,7 +2694,7 @@ static int img_amend(int argc, char **argv)
>  
>  fmt = bs->drv->format_name;
>  
> -if (is_help_option(options)) {
> +if (has_help_option(options)) {
>  ret = print_block_option_help(filename, fmt);
>  goto out;
>  }
> @@ -2715,6 +2721,8 @@ out:
>  }
>  free_option_parameters(create_options);
>  free_option_parameters(options_param);
> +g_free(options);
> +
>  if (ret) {
>  return 1;
>  }
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Richard Henderson]

On 02/20/2014 11:50 AM, Peter Maydell wrote:
> +unsigned a, b, c, d;
> +int max = __get_cpuid_max(0, 0);
> +
> +if (max >= 1) {
> +__cpuid(1, a, b, c, d);
> +}
> +return 0;
>  }

You might as well check for __cpuid_count too while you're at it:

  if (max >= 7)
__cpuid_count(7, 0, a, b, c, d);

Otherwise it looks good to me.

r~

Re: [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options

[Eric Blake]

On 02/20/2014 12:33 PM, Jeff Cody wrote:
> On Thu, Feb 20, 2014 at 03:57:20PM +0100, Kevin Wolf wrote:
>> Instead of ignoring all option values but the last one, multiple -o
>> options now have the same meaning as having a single option with all
>> settings in the order of their respective -o options.
>>

> There is a 'return -1' that is missed, that I think needs to be 'goto
> out;' for cleanup.  It is right after the call to
> bdrv_parse_cache_flags() in the img_convert() function:
> 
> ret = bdrv_parse_cache_flags(cache, &flags);
> if (ret < 0) {
> error_report("Invalid cache option: %s", cache);
> return -1;
> }
> 
> Looks like this has been this way for a while, though.  And this is an
> instance (the only instance) where img_convert returns -1 instead of 0
> or 1.  I'm not sure the implications if that changed, and became '1',
> so we'd probably want to preserve the return value.

No, exit status of 255 on an invalid cache option is a bug; fixing it to
return EXIT_FAILURE (1) is indeed correct.

[Hmm, this shows that I only reviewed the changed sections, rather than
also looking for all other 'return' statements - thanks Jeff for the
more thorough review]

> 
> Since this doesn't have anything to do with this patch (although
> leaking 'options' now, in addition to the other stuff leaked), I'll go
> ahead and give my reviewed-by, and we can fix this in another
> follow-up patch:

Agreed to that plan of attack (we've now racked up several good followups).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 19:09, Mario Smarduch  wrote:
> Questionable in this patch - is cutting through several layers to set
> proxy properties. They should be set from "device" instance init
> before it's realized. The problem though is that unlike PCI
> that sets proxy and virtio-net properties via its virtio_net_properites[],
> the virtio-mmio version instantiates the proxy in the machine model,
> so it doesn't appear to be good place to set virtio-net
> host features since you don't know what virtio device will be plugged
> in later.

I think this function is the right place to set these properties,
yes. What I'm saying is that I don't see why you're doing it
this way rather than using the existing per-backend hook.
Maybe there's a reason not to use that hook, but you don't say.

> It's virtio_net_properties[] can only set virtio-net
> properites when it's instantiated. I think the proper way would
> be to refactor virtio-mmio to similar structure PCI version uses then
> one virtio_net_properties[] can be used selecting PCI or virtio-mmio at
> compile time. But right now it's unclear to me how pci and virtio-mmio
> versions intend to co-exist.

This is the result of a refactoring last year to allow virtio-mmio.
Basically the underlying structure now is:

 [some virtio transport device] <- virtio bus -> [virtio backend]

where the transport could be mmio, PCI, CCW, etc, and the
backend is net, blk, balloon, etc. We also provide devices
which are "virtio PCI transport plus a specific backend"
for (a) user convenience and (b) backwards compatibility, since
the pre-refactoring structure put the transport and backend
all in one lump. The all-in-one-lump arrangement is legacy:
we shouldn't break it, but it's not the model for virtio-mmio
to follow either.

The transport shouldn't know anything specific about the
backend it's plugged into (or vice-versa), but it's fine
for there to be hook functions so the transport and backend
can call each other at the appropriate times.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/6] qemu-img convert: Support multiple -o options

[Jeff Cody]

On Thu, Feb 20, 2014 at 03:57:20PM +0100, Kevin Wolf wrote:
> Instead of ignoring all option values but the last one, multiple -o
> options now have the same meaning as having a single option with all
> settings in the order of their respective -o options.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-img.c | 29 -
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 770e4e6..ba6e82d 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1160,6 +1160,9 @@ static int img_convert(int argc, char **argv)
>  Error *local_err = NULL;
>  QemuOpts *sn_opts = NULL;
>  
> +/* Initialize before goto out */
> +qemu_progress_init(progress, 1.0);
> +
>  fmt = NULL;
>  out_fmt = "raw";
>  cache = "unsafe";
> @@ -1191,13 +1194,21 @@ static int img_convert(int argc, char **argv)
>  case 'e':
>  error_report("option -e is deprecated, please use \'-o "
>"encryption\' instead!");
> -return 1;
> +ret = -1;
> +goto out;
>  case '6':
>  error_report("option -6 is deprecated, please use \'-o "
>"compat6\' instead!");
> -return 1;
> +ret = -1;
> +goto out;
>  case 'o':
> -options = optarg;
> +if (!options) {
> +options = g_strdup(optarg);
> +} else {
> +char *old_options = options;
> +options = g_strdup_printf("%s,%s", options, optarg);
> +g_free(old_options);
> +}
>  break;
>  case 's':
>  snapshot_name = optarg;
> @@ -1208,7 +1219,8 @@ static int img_convert(int argc, char **argv)
>  if (!sn_opts) {
>  error_report("Failed in parsing snapshot param '%s'",
>   optarg);
> -return 1;
> +ret = -1;
> +goto out;
>  }
>  } else {
>  snapshot_name = optarg;
> @@ -1221,7 +1233,8 @@ static int img_convert(int argc, char **argv)
>  sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);
>  if (sval < 0 || *end) {
>  error_report("Invalid minimum zero buffer size for sparse 
> output specified");
> -return 1;
> +ret = -1;
> +goto out;
>  }

There is a 'return -1' that is missed, that I think needs to be 'goto
out;' for cleanup.  It is right after the call to
bdrv_parse_cache_flags() in the img_convert() function:

ret = bdrv_parse_cache_flags(cache, &flags);
if (ret < 0) {
error_report("Invalid cache option: %s", cache);
return -1;
}

Looks like this has been this way for a while, though.  And this is an
instance (the only instance) where img_convert returns -1 instead of 0
or 1.  I'm not sure the implications if that changed, and became '1',
so we'd probably want to preserve the return value.

Since this doesn't have anything to do with this patch (although
leaking 'options' now, in addition to the other stuff leaked), I'll go
ahead and give my reviewed-by, and we can fix this in another
follow-up patch:

Reviewed-by: Jeff Cody 

>  
>  min_sparse = sval / BDRV_SECTOR_SIZE;
> @@ -1253,10 +1266,7 @@ static int img_convert(int argc, char **argv)
>  
>  out_filename = argv[argc - 1];
>  
> -/* Initialize before goto out */
> -qemu_progress_init(progress, 1.0);
> -
> -if (options && is_help_option(options)) {
> +if (options && has_help_option(options)) {
>  ret = print_block_option_help(out_filename, out_fmt);
>  goto out;
>  }
> @@ -1649,6 +1659,7 @@ out:
>  free_option_parameters(create_options);
>  free_option_parameters(param);
>  qemu_vfree(buf);
> +g_free(options);
>  if (sn_opts) {
>  qemu_opts_del(sn_opts);
>  }
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH v2 2/6] qemu-img create: Support multiple -o options

[Jeff Cody]

On Thu, Feb 20, 2014 at 03:57:19PM +0100, Kevin Wolf wrote:
> If you specified multiple -o options for qemu-img create, it would
> silently ignore all but the last one. This patch fixes the problem.
> 
> Now multiple -o options has the same meaning as having a single option
> with all settings in the order of their respective -o options.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-img.c | 24 ++--
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b834d52..770e4e6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -369,13 +369,19 @@ static int img_create(int argc, char **argv)
>  case 'e':
>  error_report("option -e is deprecated, please use \'-o "
>"encryption\' instead!");
> -return 1;
> +goto fail;
>  case '6':
>  error_report("option -6 is deprecated, please use \'-o "
>"compat6\' instead!");
> -return 1;
> +goto fail;
>  case 'o':
> -options = optarg;
> +if (!options) {
> +options = g_strdup(optarg);
> +} else {
> +char *old_options = options;
> +options = g_strdup_printf("%s,%s", options, optarg);
> +g_free(old_options);
> +}
>  break;
>  case 'q':
>  quiet = true;
> @@ -403,7 +409,7 @@ static int img_create(int argc, char **argv)
>  error_report("kilobytes, megabytes, gigabytes, terabytes, "
>   "petabytes and exabytes.");
>  }
> -return 1;
> +goto fail;
>  }
>  img_size = (uint64_t)sval;
>  }
> @@ -411,7 +417,8 @@ static int img_create(int argc, char **argv)
>  help();
>  }
>  
> -if (options && is_help_option(options)) {
> +if (options && has_help_option(options)) {
> +g_free(options);
>  return print_block_option_help(filename, fmt);
>  }
>  
> @@ -420,10 +427,15 @@ static int img_create(int argc, char **argv)
>  if (error_is_set(&local_err)) {
>  error_report("%s: %s", filename, error_get_pretty(local_err));
>  error_free(local_err);
> -return 1;
> +goto fail;
>  }
>  
> +g_free(options);
>  return 0;
> +
> +fail:
> +g_free(options);
> +return 1;

One minor preference: there are 3 separate cleanup spots to free
'options'.  If a ret variable was used, the label 'fail:' could become
'out:', and those 3 cleanups could be consolidated to a single
g_free(options).  That would help any future committer not forget a
cleanup spot.

But, as it is correct as-is:

Reviewed-by: Jeff Cody 

>  }
>  
>  static void dump_json_image_check(ImageCheck *check, bool quiet)
> -- 
> 1.8.1.4
> 
> 

[Qemu-devel] [PATCH] tcg: Fix typo in comment (dependancies -> dependencies)

[Stefan Weil]

Signed-off-by: Stefan Weil 
---
 tcg/tcg.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 7ff1a8f..44ec42e 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -41,7 +41,7 @@
 #include "qemu/host-utils.h"
 #include "qemu/timer.h"
 
-/* Note: the long term plan is to reduce the dependancies on the QEMU
+/* Note: the long term plan is to reduce the dependencies on the QEMU
CPU definitions. Currently they are used for qemu_ld/st
instructions */
 #define NO_CPU_IO_DEFS
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

Peter thanks.

Questionable in this patch - is cutting through several layers to set
proxy properties. They should be set from "device" instance init
before it's realized. The problem though is that unlike PCI
that sets proxy and virtio-net properties via its virtio_net_properites[],
the virtio-mmio version instantiates the proxy in the machine model,
so it doesn't appear to be good place to set virtio-net
host features since you don't know what virtio device will be plugged
in later. It's virtio_net_properties[] can only set virtio-net
properites when it's instantiated. I think the proper way would
be to refactor virtio-mmio to similar structure PCI version uses then
one virtio_net_properties[] can be used selecting PCI or virtio-mmio at
compile time. But right now it's unclear to me how pci and virtio-mmio
versions intend to co-exist. I'm fairly new to qemu community.

- Mario

On 02/20/2014 10:30 AM, Peter Maydell wrote:
> On 20 February 2014 18:12, Mario Smarduch  wrote:
>>
>> Hello,
>>
>> any feedback on this patch, after a brief email exchange Anthony deferred to
>> Peter.
>>
>> Lack of improper host features handling lowers 1g & 10g performance
>> substantially on arm-kvm compared to xeon.
>>
>> We would like to have this fixed so we don't have to patch every new release
>> of qemu, especially virtio stuff.
> 
> I don't know enough about virtio to review, really, but
> I'll have a go:
> 
>>> On 13 February 2014 21:13, Mario Smarduch  wrote:
 virtio: set virtio-net/virtio-mmio host features

 Patch sets 'virtio-net/virtio-mmio' host features to enable network
 features based on peer capabilities. Currently host features turn
 of all features by default.

 Signed-off-by: Mario Smarduch 
 ---
  hw/virtio/virtio-mmio.c |   29 +
  1 file changed, 29 insertions(+)

 diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
 index 8829eb0..1d940b7 100644
 --- a/hw/virtio/virtio-mmio.c
 +++ b/hw/virtio/virtio-mmio.c
 @@ -23,6 +23,7 @@
  #include "hw/virtio/virtio.h"
  #include "qemu/host-utils.h"
  #include "hw/virtio/virtio-bus.h"
 +#include "hw/virtio/virtio-net.h"

  /* #define DEBUG_VIRTIO_MMIO */

 @@ -92,6 +93,12 @@ typedef struct {
  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
  VirtIOMMIOProxy *dev);

 +/* all possible virtio-net features supported */
 +static Property virtio_mmio_net_properties[] = {
 +DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
 +DEFINE_PROP_END_OF_LIST(),
 +};
 +
  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned 
 size)
  {
  VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
 @@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)

  /* virtio-mmio device */

 +/* Walk virtio-net possible supported features and set host_features, this
 + * should be done earlier when the object is instantiated but at that 
 point
 + * you don't know what type of device will be plugged in.
 + */
 +static void virtio_mmio_set_net_features(Property *prop, uint32_t 
 *features)
 +{
 +for (; prop && prop->name; prop++) {
 +if (prop->defval == true) {
 +*features |= (1 << prop->bitnr);
 +}  else  {
 +*features &= ~(1 << prop->bitnr);
 +}
 +}
 +}
 +
  /* This is called by virtio-bus just after the device is plugged. */
  static void virtio_mmio_device_plugged(DeviceState *opaque)
  {
  VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
 +VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
 +Object *obj = OBJECT(vdev);

 +/* set host features only for virtio-net */
 +if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
 +virtio_mmio_set_net_features(virtio_mmio_net_properties,
 +
 &proxy->host_features);
 +}
> 
> This looks weird. We already have a mechanism for saying
> "hey the thing we just plugged in gets to tell us about
> feature bits":
> 
  proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
  proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
  
 proxy->host_features);
> 
> ...this is making an indirect call to the backend device's
> get_features method, which for virtio-net is
> virtio_net_get_features(). Why should the transport
> need special case code for virtio-net backends?
> 
> thanks
> -- PMM
> 

[Qemu-devel] [PATCH] stubs: Optimize dependencies for gdbstub.c

[Stefan Weil]

It does not need qemu-common.h. Including exec/gdbstub.h fixes a warning
from static code analyzers and avoids mismatching declarations for
xml_builtin.

Signed-off-by: Stefan Weil 
---
 stubs/gdbstub.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/stubs/gdbstub.c b/stubs/gdbstub.c
index c1dbfe7..f6a4553 100644
--- a/stubs/gdbstub.c
+++ b/stubs/gdbstub.c
@@ -1,4 +1,6 @@
-#include "qemu-common.h"
+#include "stdbool.h"/* bool (in exec/gdbstub.h) */
+#include "stddef.h" /* NULL */
+#include "exec/gdbstub.h"   /* xml_builtin */
 
 const char *const xml_builtin[][2] = {
   { NULL, NULL }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing

[Jeff Cody]

On Thu, Feb 20, 2014 at 03:57:23PM +0100, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/082 | 187 +++
>  tests/qemu-iotests/082.out | 436 
> +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 624 insertions(+)
>  create mode 100755 tests/qemu-iotests/082
>  create mode 100644 tests/qemu-iotests/082.out
> 
> diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
> new file mode 100755
> index 000..10a4b1e
> --- /dev/null
> +++ b/tests/qemu-iotests/082
> @@ -0,0 +1,187 @@
> +#!/bin/bash
> +#
> +# Test qemu-img command line parsing
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +# creator
> +owner=kw...@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +
> +_cleanup()
> +{
> + _cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +function run_qemu_img()
> +{
> +echo
> +echo Testing: "$@" | _filter_testdir
> +$QEMU_IMG "$@" 2>&1 | _filter_testdir

$QEMU_IMG needs quoted: "$QEMU_IMG"

> +}
> +
> +size=128M
> +
> +echo
> +echo === create: Options specified more than once ===
> +
> +# Last -f should win
> +run_qemu_img create -f foo -f $IMGFMT $TEST_IMG $size
> +run_qemu_img info $TEST_IMG
> +
> +# Multiple -o should be merged
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on 
> $TEST_IMG $size

s/\$TEST_IMG/\$\"TEST_IMG\"/g

After those two changes, ./check -qcow2 082 runs fine in my spaced
directory test.


> +run_qemu_img info $TEST_IMG
> +
> +# If the same -o key is specified more than once, the last one wins
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o 
> cluster_size=8k $TEST_IMG $size
> +run_qemu_img info $TEST_IMG
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,cluster_size=8k $TEST_IMG 
> $size
> +run_qemu_img info $TEST_IMG
> +
> +echo
> +echo === create: help for -o ===
> +
> +# Adding the help option to a command without other -o options
> +run_qemu_img create -f $IMGFMT -o help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o \? $TEST_IMG $size
> +
> +# Adding the help option to the same -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,\? $TEST_IMG $size
> +
> +# Adding the help option to a separate -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? $TEST_IMG $size
> +
> +# Looks like a help option, but is part of the backing file name
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG 
> $size
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG $size
> +
> +# Leave out everything that isn't needed
> +run_qemu_img create -f $IMGFMT -o help
> +run_qemu_img create -o help
> +
> +echo
> +echo === convert: Options specified more than once ===
> +
> +# We need a valid source image
> +run_qemu_img create -f $IMGFMT $TEST_IMG $size
> +
> +# Last -f should win
> +run_qemu_img convert -f foo -f $IMGFMT $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +# Last -O should win
> +run_qemu_img convert -O foo -O $IMGFMT $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +# Multiple -o should be merged
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on 
> $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +# If the same -o key is specified more than once, the last one wins
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k -o lazy_refcounts=on -o 
> cluster_size=8k $TEST_IMG $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +run_qemu_img convert -O $IMGFMT -o cluster_size=4k,cluster_size=8k $TEST_IMG 
> $TEST_IMG.base
> +run_qemu_img info $TEST_IMG.base
> +
> +echo
> +echo === convert: help for -o ===
> +
> +# Adding the help option to a command without other -o options
> +run_qemu_img convert -O $IMGFMT -o help $TEST_IMG $TEST_IMG.base
> +run_qemu_img convert -O $IMGFMT -

Re: [Qemu-devel] [PATCH v2] virtio-net: add a field to indicate if vlan table is used

[Vlad Yasevich]

On 02/20/2014 11:38 AM, Amos Kong wrote:
> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't
> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated.
> 
> This patch added a new field to @RxFilterInfo to indicate if management
> uses the vlan table.
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html
> 
> Signed-off-by: Amos Kong 
> ---
> V2: don't make vlan-table optional, add a flag to indicate
> if vlan table is used by management
> ---
>  hw/net/virtio-net.c | 38 +-
>  qapi-schema.json|  3 +++
>  qmp-commands.hx |  2 ++
>  3 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3626608..f591f4e 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -222,13 +222,33 @@ static char *mac_strdup_printf(const uint8_t *mac)
>  mac[1], mac[2], mac[3], mac[4], mac[5]);
>  }
>  
> +static intList *get_vlan_table(VirtIONet *n)
> +{
> +intList *list, *entry;
> +int i, j;
> +
> +list = NULL;
> +for (i = 0; i < MAX_VLAN >> 5; i++) {
> +for (j = 0; n->vlans[i] && j < 0x1f; j++) {
> +if (n->vlans[i] & (1U << j)) {
> +entry = g_malloc0(sizeof(*entry));
> +entry->value = (i << 5) + j;
> +entry->next = list;
> +list = entry;
> +}
> +}
> +}
> +
> +return list;
> +}
> +
>  static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>  {
>  VirtIONet *n = qemu_get_nic_opaque(nc);
> +VirtIODevice *vdev = VIRTIO_DEVICE(n);
>  RxFilterInfo *info;
>  strList *str_list, *entry;
> -intList *int_list, *int_entry;
> -int i, j;
> +int i;
>  
>  info = g_malloc0(sizeof(*info));
>  info->name = g_strdup(nc->name);
> @@ -273,19 +293,11 @@ static RxFilterInfo 
> *virtio_net_query_rxfilter(NetClientState *nc)
>  str_list = entry;
>  }
>  info->multicast_table = str_list;
> +info->vlan_table = get_vlan_table(n);
>  
> -int_list = NULL;
> -for (i = 0; i < MAX_VLAN >> 5; i++) {
> -for (j = 0; n->vlans[i] && j < 0x1f; j++) {
> -if (n->vlans[i] & (1U << j)) {
> -int_entry = g_malloc0(sizeof(*int_entry));
> -int_entry->value = (i << 5) + j;
> -int_entry->next = int_list;
> -int_list = int_entry;
> -}
> -}
> +if ((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features) {
> +info->vlan = true;
>  }

So, in the case that vlan filtering is not supported in the guest
we get:
 "vlan": false,
 "vlan-table": [
 0,
 1,
 2,
 ...
 4095
]
since virtio_net now initializes the table to all 1s.
Seems a bit awkward.  We are providing a lot of data that
is simply going to be ignored.

> -info->vlan_table = int_list;
>  
>  /* enable event notification after query */
>  nc->rxfilter_notify_enabled = 1;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7cfb5e5..5b54e94 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4032,6 +4032,8 @@
>  #
>  # @unicast-overflow: unicast table is overflowed or not
>  #
> +# @vlan: whether management uses the vlan table
> +#

The above description seems a bit confusing to me.  The value
we are returning describes whether or not qemu is performing
vlan filtering.  I am not sure if it has any bearing on what
management may be doing.

I think the idea is that management, in the future, would look at
this value and make some decision about applying provided filter
to the current host configuration.

>  # @main-mac: the main macaddr string
>  #
>  # @vlan-table: a list of active vlan id
> @@ -4052,6 +4054,7 @@
>  'broadcast-allowed':  'bool',
>  'multicast-overflow': 'bool',
>  'unicast-overflow':   'bool',
> +'vlan':   'bool',

Not terribly descriptive.  May be call it vlan-filter?

Thanks
-vlad
>  'main-mac':   'str',
>  'vlan-table': ['int'],
>  'unicast-table':  ['str'],
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index cce6b81..b170c79 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3307,6 +3307,7 @@ Each array entry contains the following:
>  - "broadcast-allowed": allow to receive broadcast (json-bool)
>  - "multicast-overflow": multicast table is overflowed (json-bool)
>  - "unicast-overflow": unicast table is overflowed (json-bool)
> +- "vlan": management uses the vlan table (json-bool)
>  - "main-mac": main macaddr string (json-string)
>  - "vlan-table": a json-array of active vlan id
>  - "unicast-table": a json-array of unicast macaddr string
> @@ -3321,6 +3322,7 @@ Example:
>  "name": "vnet0",
>  "main-mac": "52:54:00:12:34:56",
>  "

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Peter Maydell]

On 20 February 2014 18:12, Mario Smarduch  wrote:
>
> Hello,
>
> any feedback on this patch, after a brief email exchange Anthony deferred to
> Peter.
>
> Lack of improper host features handling lowers 1g & 10g performance
> substantially on arm-kvm compared to xeon.
>
> We would like to have this fixed so we don't have to patch every new release
> of qemu, especially virtio stuff.

I don't know enough about virtio to review, really, but
I'll have a go:

>> On 13 February 2014 21:13, Mario Smarduch  wrote:
>>> virtio: set virtio-net/virtio-mmio host features
>>>
>>> Patch sets 'virtio-net/virtio-mmio' host features to enable network
>>> features based on peer capabilities. Currently host features turn
>>> of all features by default.
>>>
>>> Signed-off-by: Mario Smarduch 
>>> ---
>>>  hw/virtio/virtio-mmio.c |   29 +
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>>> index 8829eb0..1d940b7 100644
>>> --- a/hw/virtio/virtio-mmio.c
>>> +++ b/hw/virtio/virtio-mmio.c
>>> @@ -23,6 +23,7 @@
>>>  #include "hw/virtio/virtio.h"
>>>  #include "qemu/host-utils.h"
>>>  #include "hw/virtio/virtio-bus.h"
>>> +#include "hw/virtio/virtio-net.h"
>>>
>>>  /* #define DEBUG_VIRTIO_MMIO */
>>>
>>> @@ -92,6 +93,12 @@ typedef struct {
>>>  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
>>>  VirtIOMMIOProxy *dev);
>>>
>>> +/* all possible virtio-net features supported */
>>> +static Property virtio_mmio_net_properties[] = {
>>> +DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
>>> +DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned 
>>> size)
>>>  {
>>>  VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
>>> @@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)
>>>
>>>  /* virtio-mmio device */
>>>
>>> +/* Walk virtio-net possible supported features and set host_features, this
>>> + * should be done earlier when the object is instantiated but at that point
>>> + * you don't know what type of device will be plugged in.
>>> + */
>>> +static void virtio_mmio_set_net_features(Property *prop, uint32_t 
>>> *features)
>>> +{
>>> +for (; prop && prop->name; prop++) {
>>> +if (prop->defval == true) {
>>> +*features |= (1 << prop->bitnr);
>>> +}  else  {
>>> +*features &= ~(1 << prop->bitnr);
>>> +}
>>> +}
>>> +}
>>> +
>>>  /* This is called by virtio-bus just after the device is plugged. */
>>>  static void virtio_mmio_device_plugged(DeviceState *opaque)
>>>  {
>>>  VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>>> +VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>> +Object *obj = OBJECT(vdev);
>>>
>>> +/* set host features only for virtio-net */
>>> +if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
>>> +virtio_mmio_set_net_features(virtio_mmio_net_properties,
>>> +
>>> &proxy->host_features);
>>> +}

This looks weird. We already have a mechanism for saying
"hey the thing we just plugged in gets to tell us about
feature bits":

>>>  proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
>>>  proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
>>>  
>>> proxy->host_features);

...this is making an indirect call to the backend device's
get_features method, which for virtio-net is
virtio_net_get_features(). Why should the transport
need special case code for virtio-net backends?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] xen_disk: add discard support

[Olaf Hering]

On Thu, Feb 20, Stefano Stabellini wrote:

> On Thu, 6 Feb 2014, Olaf Hering wrote:
> > @@ -253,6 +254,8 @@ static int ioreq_parse(struct ioreq *ioreq)
> >  case BLKIF_OP_WRITE:
> >  ioreq->prot = PROT_READ; /* from memory */
> >  break;
> > +case BLKIF_OP_DISCARD:
> > +return 0;
> >  default:
> >  xen_be_printf(&blkdev->xendev, 0, "error: unknown operation 
> > (%d)\n",
> >ioreq->req.operation);
> 
> Unfortunately I didn't realize before that older Xen releases don't
> define BLKIF_OP_DISCARD, therefore this patch would cause QEMU build
> failures against Xen 4.1 for example.

Would that work for you?

Olaf

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 2d5a25b..4ca03ff 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -144,6 +144,9 @@ static inline int xen_xc_hvm_inject_msi(XenXC xen_xc, 
domid_t dom,
 {
 return -ENOSYS;
 }
+#ifndef BLKIF_OP_DISCARD
+#define BLKIF_OP_DISCARD 5
+#endif
 #else
 static inline int xen_xc_hvm_inject_msi(XenXC xen_xc, domid_t dom,
 uint64_t addr, uint32_t data)

Re: [Qemu-devel] [PATCH v2] xen_disk: add discard support

[Stefano Stabellini]

On Thu, 20 Feb 2014, Olaf Hering wrote:
> On Thu, Feb 20, Stefano Stabellini wrote:
> 
> > On Thu, 6 Feb 2014, Olaf Hering wrote:
> > > @@ -253,6 +254,8 @@ static int ioreq_parse(struct ioreq *ioreq)
> > >  case BLKIF_OP_WRITE:
> > >  ioreq->prot = PROT_READ; /* from memory */
> > >  break;
> > > +case BLKIF_OP_DISCARD:
> > > +return 0;
> > >  default:
> > >  xen_be_printf(&blkdev->xendev, 0, "error: unknown operation 
> > > (%d)\n",
> > >ioreq->req.operation);
> > 
> > Unfortunately I didn't realize before that older Xen releases don't
> > define BLKIF_OP_DISCARD, therefore this patch would cause QEMU build
> > failures against Xen 4.1 for example.
> 
> Why would that matter?
> Is new qemu seriously supposed to work with stale Xen releases?
> But I will have a look how to solve this.

It needs to build at least.

Re: [Qemu-devel] [PATCH v2] xen_disk: add discard support

[Olaf Hering]

On Thu, Feb 20, Stefano Stabellini wrote:

> On Thu, 6 Feb 2014, Olaf Hering wrote:
> > @@ -253,6 +254,8 @@ static int ioreq_parse(struct ioreq *ioreq)
> >  case BLKIF_OP_WRITE:
> >  ioreq->prot = PROT_READ; /* from memory */
> >  break;
> > +case BLKIF_OP_DISCARD:
> > +return 0;
> >  default:
> >  xen_be_printf(&blkdev->xendev, 0, "error: unknown operation 
> > (%d)\n",
> >ioreq->req.operation);
> 
> Unfortunately I didn't realize before that older Xen releases don't
> define BLKIF_OP_DISCARD, therefore this patch would cause QEMU build
> failures against Xen 4.1 for example.

Why would that matter?
Is new qemu seriously supposed to work with stale Xen releases?
But I will have a look how to solve this.

Olaf

[Qemu-devel] [PULL 2/2] xen_disk: fix io accounting

[Stefano Stabellini]

From: Olaf Hering 

bdrv_acct_done was called unconditional. But in case the ioreq has no
segments there is no matching bdrv_acct_start call. This could lead to
bogus accounting values.

Found by code inspection.

Signed-off-by: Olaf Hering 
Signed-off-by: Stefano Stabellini 
---
 hw/block/xen_disk.c |   13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 098f6c6..7f0f14a 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -483,7 +483,18 @@ static void qemu_aio_complete(void *opaque, int ret)
 ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
 ioreq_unmap(ioreq);
 ioreq_finish(ioreq);
-bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
+switch (ioreq->req.operation) {
+case BLKIF_OP_WRITE:
+case BLKIF_OP_FLUSH_DISKCACHE:
+if (!ioreq->req.nr_segments) {
+break;
+}
+case BLKIF_OP_READ:
+bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
+break;
+default:
+break;
+}
 qemu_bh_schedule(ioreq->blkdev->bh);
 }
 
-- 
1.7.10.4

[Qemu-devel] [PULL 1/2] Call pci_piix3_xen_ide_unplug from unplug_disks

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini 
Acked-by: Paolo Bonzini 
---
 hw/ide/piix.c |3 +--
 hw/xen/xen_platform.c |3 ++-
 include/hw/ide.h  |1 +
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 0eda301..40757eb 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
 return 0;
 }
 
-static int pci_piix3_xen_ide_unplug(DeviceState *dev)
+int pci_piix3_xen_ide_unplug(DeviceState *dev)
 {
 PCIIDEState *pci_ide;
 DriveInfo *di;
@@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass, 
void *data)
 k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
 k->class_id = PCI_CLASS_STORAGE_IDE;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-dc->unplug = pci_piix3_xen_ide_unplug;
 }
 
 static const TypeInfo piix3_ide_xen_info = {
diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
index 70875e4..1d9d0e9 100644
--- a/hw/xen/xen_platform.c
+++ b/hw/xen/xen_platform.c
@@ -27,6 +27,7 @@
 
 #include "hw/hw.h"
 #include "hw/i386/pc.h"
+#include "hw/ide.h"
 #include "hw/pci/pci.h"
 #include "hw/irq.h"
 #include "hw/xen/xen_common.h"
@@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *o)
 if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
 PCI_CLASS_STORAGE_IDE
 && strcmp(d->name, "xen-pci-passthrough") != 0) {
-qdev_unplug(DEVICE(d), NULL);
+pci_piix3_xen_ide_unplug(DEVICE(d));
 }
 }
 
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 507e6d3..bc8bd32 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
 PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
+int pci_piix3_xen_ide_unplug(DeviceState *dev);
 void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 
 /* ide-mmio.c */
-- 
1.7.10.4

[Qemu-devel] [PULL 0/2] xen-140220

[Stefano Stabellini]

The following changes since commit 2ca92bb993991d6dcb8f68751aca9fc2ec2b8867:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-usb-3' into staging 
(2014-02-20 15:25:05 +)

are available in the git repository at:


  git://xenbits.xen.org/people/sstabellini/qemu-dm.git xen-140220

for you to fetch changes up to 58da5b1e01a586eb5a52ba3eec342d6828269839:

  xen_disk: fix io accounting (2014-02-20 17:57:13 +)


Olaf Hering (1):
  xen_disk: fix io accounting

Stefano Stabellini (1):
  Call pci_piix3_xen_ide_unplug from unplug_disks

 hw/block/xen_disk.c   |   13 -
 hw/ide/piix.c |3 +--
 hw/xen/xen_platform.c |3 ++-
 include/hw/ide.h  |1 +
 4 files changed, 16 insertions(+), 4 deletions(-)

Re: [Qemu-devel] [PATCH] virtio: set virtio-net/virtio-mmio host features

[Mario Smarduch]

Hello,

any feedback on this patch, after a brief email exchange Anthony deferred to
Peter. 

Lack of improper host features handling lowers 1g & 10g performance 
substantially on arm-kvm compared to xeon. 

We would like to have this fixed so we don't have to patch every new release
of qemu, especially virtio stuff.

- Mario

On 02/14/2014 03:49 PM, Peter Maydell wrote:
> CCing the virtio maintainers.
> 
> thanks
> -- PMM
> 
> On 13 February 2014 21:13, Mario Smarduch  wrote:
>> virtio: set virtio-net/virtio-mmio host features
>>
>> Patch sets 'virtio-net/virtio-mmio' host features to enable network
>> features based on peer capabilities. Currently host features turn
>> of all features by default.
>>
>> Signed-off-by: Mario Smarduch 
>> ---
>>  hw/virtio/virtio-mmio.c |   29 +
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>> index 8829eb0..1d940b7 100644
>> --- a/hw/virtio/virtio-mmio.c
>> +++ b/hw/virtio/virtio-mmio.c
>> @@ -23,6 +23,7 @@
>>  #include "hw/virtio/virtio.h"
>>  #include "qemu/host-utils.h"
>>  #include "hw/virtio/virtio-bus.h"
>> +#include "hw/virtio/virtio-net.h"
>>
>>  /* #define DEBUG_VIRTIO_MMIO */
>>
>> @@ -92,6 +93,12 @@ typedef struct {
>>  static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
>>  VirtIOMMIOProxy *dev);
>>
>> +/* all possible virtio-net features supported */
>> +static Property virtio_mmio_net_properties[] = {
>> +DEFINE_VIRTIO_NET_FEATURES(VirtIOMMIOProxy, host_features),
>> +DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>>  {
>>  VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
>> @@ -347,11 +354,33 @@ static void virtio_mmio_reset(DeviceState *d)
>>
>>  /* virtio-mmio device */
>>
>> +/* Walk virtio-net possible supported features and set host_features, this
>> + * should be done earlier when the object is instantiated but at that point
>> + * you don't know what type of device will be plugged in.
>> + */
>> +static void virtio_mmio_set_net_features(Property *prop, uint32_t *features)
>> +{
>> +for (; prop && prop->name; prop++) {
>> +if (prop->defval == true) {
>> +*features |= (1 << prop->bitnr);
>> +}  else  {
>> +*features &= ~(1 << prop->bitnr);
>> +}
>> +}
>> +}
>> +
>>  /* This is called by virtio-bus just after the device is plugged. */
>>  static void virtio_mmio_device_plugged(DeviceState *opaque)
>>  {
>>  VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>> +VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>> +Object *obj = OBJECT(vdev);
>>
>> +/* set host features only for virtio-net */
>> +if (object_dynamic_cast(obj, TYPE_VIRTIO_NET)) {
>> +virtio_mmio_set_net_features(virtio_mmio_net_properties,
>> +
>> &proxy->host_features);
>> +}
>>  proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
>>  proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
>>  
>> proxy->host_features);
>> --
>> 1.7.9.5
>>

Re: [Qemu-devel] [PATCH v2, Ping] SMBIOS: Upgrade Type17 to v2.3, add Type2

[Laszlo Ersek]

On 02/20/14 16:38, Gabriel L. Somlo wrote:
> On Thu, Feb 20, 2014 at 04:27:27PM +0100, Gerd Hoffmann wrote:
>> On Mi, 2014-02-19 at 15:40 -0500, Gabriel L. Somlo wrote:
>>> So I gave up on that relatively quickly, as there's no easy and
>>> convenient way to "harvest" a binary of just one table type from
>>> a host that works the way I want it... :(
>>
>> "dmidecode --type 2" ?
> 
> That doesn't work with --dump-bin, to get me something I could just
> load with "-smbios file=".
> 
> On Wed, Feb 19, 2014 at 11:20:37PM +0100, Laszlo Ersek wrote:
>> If you want to export a new table whole-sale (SMBIOS_TABLE_ENTRY),
>> then
>> you don't have to modify SeaBIOS. However, in qemu:
>> - you either need to pass single-table blobs on the qemu command
>> line, or
>> - patch qemu so that it internally prepares the *complete* table for
>> the
>> boot firmware (meaning you must encode knowledge about formatted vs.
>> unformatted fields in qemu -- you must set up the unformatted area
>> and
>> the string indices yourself). (*)
>>
>> You have to encode the formatted vs. unformatted knowledge
>> *somewhere*.
>> You can push it around (qemu command line, qemu code, seabios code),
>> but
>> you have to encode it explicitly somewhere.
>>
>> (Writing the SMBIOS patches for OVMF (type0 and type1) took me three
>> days of *uninterrupted* misery ^W coding.)
>>
>> Preferably, (*) should be implemented, because then SeaBIOS and OVMF
>> can
>> both profit immediately. O:-)
> 
> At this point, the question is "cut'n'paste from SeaBIOS" vs. rewrite
> the table generation routines from scratch in QEMU? I remember ACPI
> was mostly cut'n'pasted into QEMU, so would that be an OK starting
> point for SMBIOS as well ? I looked at licenses, and the SeaBIOS
> smbios.c file is gplv3, whereas the current smbios.c in QEMU is some
> combination of gplv2 and gplv2+. Any thoughts on whether that's a
> problem, legally speaking ?

I have no clue about the technical feasibility. You'll probably only
know if you try it :)

Regarding licensing, IIRC Michael ported the ACPI code by:
- posting the qemu patch on both qemu-devel and the seabios list,
- CC'd all people who ever modified the seabios files being ported
  (this is easy with "git log")
- asked all CC'd people to respond with an Acked-by if they agreed to
  relicense their seabios contribs.

Laszlo

Re: [Qemu-devel] [PATCH v2] xen_disk: add discard support

[Stefano Stabellini]

On Thu, 6 Feb 2014, Olaf Hering wrote:
> @@ -253,6 +254,8 @@ static int ioreq_parse(struct ioreq *ioreq)
>  case BLKIF_OP_WRITE:
>  ioreq->prot = PROT_READ; /* from memory */
>  break;
> +case BLKIF_OP_DISCARD:
> +return 0;
>  default:
>  xen_be_printf(&blkdev->xendev, 0, "error: unknown operation (%d)\n",
>ioreq->req.operation);

Unfortunately I didn't realize before that older Xen releases don't
define BLKIF_OP_DISCARD, therefore this patch would cause QEMU build
failures against Xen 4.1 for example.
Give a look at include/hw/xen/xen_common.h to see how compatibility
with older Xen versions is usually achieved in QEMU.

Re: [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator

[Eric Blake]

On 02/19/2014 10:54 PM, Wenchao Xia wrote:
> After this patch, hidden enum type BlockdevOptionsKind will not
> be generated, and other API can use enum BlockdevDriver.
> 
> Signed-off-by: Wenchao Xia 
> ---
>  qapi-schema.json |   14 +-
>  1 files changed, 13 insertions(+), 1 deletions(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Markus Armbruster]

Paolo Bonzini  writes:

> Il 20/02/2014 17:02, Andreas Färber ha scritto:
>> Not unexpected, it's the older; the - convention was introduced possibly
>> with QOM around start of 2012. Or at least there it's been enforced, and

QMP preferred '-' from the start, but insufficient decoupling from
command line and HMP let in many '_', and the bad examples then got
copied around, as usual.

>> by my understanding of QOM and QMP visibility it then applies to devices
>> as well.
>>
>> Regarding QMP, I consider it smarter to do the _ -> - matching at
>> QemuOpts level than somewhere inside QOM.
>
> That's fine, because device_add uses QemuOpts internally.
>
>> For -cpu we have such compatibility code (although non-QemuOpts) in
>> target-i386/cpu.c, converting all underscores. Unfortunately that won't
>> work as long as there are underscores in old properties. Maybe you have
>> some cool patch idea?
>
> Well, no cool idea except "once conversion is done at the QemuOpts
> level, do a full sweep of s/_/-/".

Required anyway, to get the help texts consistent, and to get rid of the
bad examples.

> This should be done before we'll
> be able to create devices with object-add.  I think as long as we have
> a plan, consistency trumps design-by-committee convention.

Re: [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum

[Eric Blake]

On 02/20/2014 09:54 AM, Markus Armbruster wrote:
>> +# When c is upper and no "_" appears before, do more checks
>> +if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
> 
> c_fun_str[i - 1]... what if i == 0?

How? We already had '(i > 0) and' prior to the use of i-1.


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Peter Maydell]

On 20 February 2014 17:50, Peter Maydell  wrote:
> Win32 doesn't have a cpuid.h, and MacOSX may have one but without
> the __cpuid() function we use, which means that commit 9d2eec20
> broke the build for those platforms. Fix this by tightening up
> our configure cpuid.h check to test that the functions we need
> are present, and adding some missing #ifdef guerds in

"guards"; will fix locally.

> tcg/i386/tcg-target.c.

thanks
-- PMM

[Qemu-devel] [PATCH 1/4] target-ppc: Fix htab_mask calculation

[Greg Kurz]

From: Aneesh Kumar K.V 

Correctly update the htab_mask using the return value of
KVM_PPC_ALLOCATE_HTAB ioctl. Also we don't update sdr1
on GET_SREGS for HV. We check for external htab and if
found true, we don't need to update sdr1

Signed-off-by: Aneesh Kumar K.V 
[ fixed pte group offset computation in ppc_hash64_htab_lookup() that
  caused TCG to fail, Greg Kurz  ]
Signed-off-by: Greg Kurz 
---
 hw/ppc/spapr.c   |8 +++-
 hw/ppc/spapr_hcall.c |   19 +++
 target-ppc/cpu.h |1 +
 target-ppc/kvm.c |4 +++-
 target-ppc/machine.c |   11 +++
 target-ppc/misc_helper.c |4 +++-
 target-ppc/mmu-hash64.c  |4 ++--
 target-ppc/mmu_helper.c  |3 ++-
 8 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0989ed6..8ac4d8a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -749,7 +749,13 @@ static void spapr_cpu_reset(void *opaque)
 env->external_htab = (void *)1;
 }
 env->htab_base = -1;
-env->htab_mask = HTAB_SIZE(spapr) - 1;
+/*
+ * htab_mask is the mask used to normalize hash value to PTEG index.
+ * htab_shift is log2 of hash table size.
+ * We have 8 hpte per group, and each hpte is 16 bytes.
+ * ie have 128 bytes per hpte entry.
+ */
+env->htab_mask = (1ULL << ((spapr)->htab_shift - 7)) - 1;
 env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab |
 (spapr->htab_shift - 18);
 }
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 3ffcc65..d19e3fc 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -40,6 +40,17 @@ static target_ulong compute_tlbie_rb(target_ulong v, 
target_ulong r,
 return rb;
 }
 
+static inline bool valid_pte_index(CPUPPCState *env, target_ulong pte_index)
+{
+/*
+ * hash value/pteg group index is normalized by htab_mask
+ */
+if (((pte_index & ~7ULL) / HPTES_PER_GROUP) & ~env->htab_mask) {
+return false;
+}
+return true;
+}
+
 static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 target_ulong opcode, target_ulong *args)
 {
@@ -91,7 +102,7 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 
 pteh &= ~0x60ULL;
 
-if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
+if (!valid_pte_index(env, pte_index)) {
 return H_PARAMETER;
 }
 if (likely((flags & H_EXACT) == 0)) {
@@ -136,7 +147,7 @@ static RemoveResult remove_hpte(CPUPPCState *env, 
target_ulong ptex,
 hwaddr hpte;
 target_ulong v, r, rb;
 
-if ((ptex * HASH_PTE_SIZE_64) & ~env->htab_mask) {
+if (!valid_pte_index(env, ptex)) {
 return REMOVE_PARM;
 }
 
@@ -262,7 +273,7 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 hwaddr hpte;
 target_ulong v, r, rb;
 
-if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
+if (!valid_pte_index(env, pte_index)) {
 return H_PARAMETER;
 }
 
@@ -299,7 +310,7 @@ static target_ulong h_read(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 uint8_t *hpte;
 int i, ridx, n_entries = 1;
 
-if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) {
+if (!valid_pte_index(env, pte_index)) {
 return H_PARAMETER;
 }
 
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 7ccf4c6..44ade0c 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -966,6 +966,7 @@ struct CPUPPCState {
 #endif
 /* segment registers */
 hwaddr htab_base;
+/* mask used to normalize hash value to PTEG index */
 hwaddr htab_mask;
 target_ulong sr[32];
 /* externally stored hash table */
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 33d69d2..969ebdd 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1031,7 +1031,9 @@ int kvm_arch_get_registers(CPUState *cs)
 return ret;
 }
 
-ppc_store_sdr1(env, sregs.u.s.sdr1);
+if (!env->external_htab) {
+ppc_store_sdr1(env, sregs.u.s.sdr1);
+}
 
 /* Sync SLB */
 #ifdef TARGET_PPC64
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 12c174f..2d46cec 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -70,7 +70,9 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
version_id)
 qemu_get_betls(f, &env->pb[i]);
 for (i = 0; i < 1024; i++)
 qemu_get_betls(f, &env->spr[i]);
-ppc_store_sdr1(env, sdr1);
+if (!env->external_htab) {
+ppc_store_sdr1(env, sdr1);
+}
 qemu_get_be32s(f, &env->vscr);
 qemu_get_be64s(f, &env->spe_acc);
 qemu_get_be32s(f, &env->spe_fscr);
@@ -179,9 +181,10 @@ static int cpu_post_load(void *opaque, int version_id)
 env->IBAT[1][i+4] = env->spr[SPR_IBAT4U + 2*i + 1];
 }
 
-/* Restore htab_base and htab_mask variables */
-ppc_store_sdr1(env, env->spr[SPR_SDR1]);
-
+if (!env->external_htab) {
+/* Restore htab_base and htab_mask variables

[Qemu-devel] [PATCH 4/4] target-ppc: Update ppc_hash64_store_hpte to support updating in-kernel htab

[Greg Kurz]

From: Aneesh Kumar K.V 

This support updating htab managed by the hypervisor. Currently we don't have
any user for this feature. This actually bring the store_hpte interface
in-line with the load_hpte one. We may want to use this when we want to
emulate henter hcall in qemu for HV kvm.

Signed-off-by: Aneesh Kumar K.V 
[ folded fix for the "warn_unused_result" build break in
  kvmppc_hash64_write_pte(), Greg Kurz  ]
Signed-off-by: Greg Kurz 
---
 target-ppc/kvm.c|   36 
 target-ppc/kvm_ppc.h|   10 ++
 target-ppc/mmu-hash64.c |   20 
 target-ppc/mmu-hash64.h |   17 ++---
 4 files changed, 68 insertions(+), 15 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index e009919..b5fff70 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1992,3 +1992,39 @@ void kvmppc_hash64_free_pteg(uint64_t token)
 g_free(htab_buf);
 return;
 }
+
+void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
+ target_ulong pte0, target_ulong pte1)
+{
+int htab_fd;
+struct kvm_get_htab_fd ghf;
+struct kvm_get_htab_buf hpte_buf;
+
+ghf.flags = 0;
+ghf.start_index = 0; /* Ignored */
+htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
+if (htab_fd < 0) {
+goto error_out;
+}
+
+hpte_buf.header.n_valid = 1;
+hpte_buf.header.n_invalid = 0;
+hpte_buf.header.index = pte_index;
+hpte_buf.hpte[0] = pte0;
+hpte_buf.hpte[1] = pte1;
+/*
+ * Write the hpte entry.
+ * CAUTION: write() has the warn_unused_result attribute. Hence we
+ * need to check the return value, even though we do nothing.
+ */
+if (write(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
+goto out_close;
+}
+
+out_close:
+close(htab_fd);
+return;
+
+error_out:
+return;
+}
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 800e1ad..a65d345 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -47,6 +47,9 @@ int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t 
index,
 uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index);
 void kvmppc_hash64_free_pteg(uint64_t token);
 
+void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
+ target_ulong pte0, target_ulong pte1);
+
 #else
 
 static inline uint32_t kvmppc_get_tbfreq(void)
@@ -207,6 +210,13 @@ static inline void kvmppc_hash64_free_pteg(uint64_t token)
 abort();
 }
 
+static inline void kvmppc_hash64_write_pte(CPUPPCState *env,
+   target_ulong pte_index,
+   target_ulong pte0, target_ulong 
pte1)
+{
+abort();
+}
+
 #endif
 
 #ifndef CONFIG_KVM
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 8dd5d22..f2af4fb 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -603,3 +603,23 @@ hwaddr ppc_hash64_get_phys_page_debug(CPUPPCState *env, 
target_ulong addr)
 
 return ppc_hash64_pte_raddr(slb, pte, addr) & TARGET_PAGE_MASK;
 }
+
+void ppc_hash64_store_hpte(CPUPPCState *env,
+   target_ulong pte_index,
+   target_ulong pte0, target_ulong pte1)
+{
+CPUState *cs = ENV_GET_CPU(env);
+
+if (kvmppc_kern_htab) {
+return kvmppc_hash64_write_pte(env, pte_index, pte0, pte1);
+}
+
+pte_index *= HASH_PTE_SIZE_64;
+if (env->external_htab) {
+stq_p(env->external_htab + pte_index, pte0);
+stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64/2, pte1);
+} else {
+stq_phys(cs->as, env->htab_base + pte_index, pte0);
+stq_phys(cs->as, env->htab_base + pte_index + HASH_PTE_SIZE_64/2, 
pte1);
+}
+}
diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
index 49d866b..1746b3e 100644
--- a/target-ppc/mmu-hash64.h
+++ b/target-ppc/mmu-hash64.h
@@ -9,6 +9,8 @@ int ppc_store_slb (CPUPPCState *env, target_ulong rb, 
target_ulong rs);
 hwaddr ppc_hash64_get_phys_page_debug(CPUPPCState *env, target_ulong addr);
 int ppc_hash64_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw,
 int mmu_idx);
+void ppc_hash64_store_hpte(CPUPPCState *env, target_ulong index,
+   target_ulong pte0, target_ulong pte1);
 #endif
 
 /*
@@ -106,21 +108,6 @@ static inline target_ulong 
ppc_hash64_load_hpte1(CPUPPCState *env,
 }
 }
 
-static inline void ppc_hash64_store_hpte(CPUPPCState *env,
- target_ulong pte_index,
- target_ulong pte0, target_ulong pte1)
-{
-CPUState *cs = ENV_GET_CPU(env);
-pte_index *= HASH_PTE_SIZE_64;
-if (env->external_htab) {
-stq_p(env->external_htab + pte_index, pte0);
-stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64/2, pte1);
-} else {
-stq_phys(cs->as, e

[Qemu-devel] [PATCH 3/4] target-ppc: Change the hpte store API

[Greg Kurz]

From: Aneesh Kumar K.V 

For updating in kernel htab we need to provide both pte0 and pte1, hence update
the interface to take pte0 and pte1 together

Signed-off-by: Aneesh Kumar K.V 
[ ldq_phys() API change, Greg Kurz  ]
Signed-off-by: Greg Kurz 
---
 hw/ppc/spapr_hcall.c|   20 ++--
 target-ppc/mmu-hash64.c |3 ++-
 target-ppc/mmu-hash64.h |   24 
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 7493302..b0f2529 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -62,7 +62,6 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment 
*spapr,
 target_ulong page_shift = 12;
 target_ulong raddr;
 target_ulong index;
-hwaddr hpte;
 uint64_t token;
 
 /* only handle 4k and 16M pages for now */
@@ -108,7 +107,6 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 }
 
 index = 0;
-hpte = pte_index * HASH_PTE_SIZE_64;
 if (likely((flags & H_EXACT) == 0)) {
 pte_index &= ~7ULL;
 token = ppc_hash64_start_access(cpu, pte_index);
@@ -130,11 +128,9 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 }
 ppc_hash64_stop_access(token);
 }
-hpte += index * HASH_PTE_SIZE_64;
 
-ppc_hash64_store_hpte1(env, hpte, ptel);
-/* eieio();  FIXME: need some sort of barrier for smp? */
-ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
+ppc_hash64_store_hpte(env, pte_index + index,
+  pteh | HPTE64_V_HPTE_DIRTY, ptel);
 
 args[0] = pte_index + index;
 return H_SUCCESS;
@@ -152,7 +148,6 @@ static RemoveResult remove_hpte(CPUPPCState *env, 
target_ulong ptex,
 target_ulong flags,
 target_ulong *vp, target_ulong *rp)
 {
-hwaddr hpte;
 uint64_t token;
 target_ulong v, r, rb;
 
@@ -172,8 +167,7 @@ static RemoveResult remove_hpte(CPUPPCState *env, 
target_ulong ptex,
 }
 *vp = v;
 *rp = r;
-hpte = ptex * HASH_PTE_SIZE_64;
-ppc_hash64_store_hpte0(env, hpte, HPTE64_V_HPTE_DIRTY);
+ppc_hash64_store_hpte(env, ptex, HPTE64_V_HPTE_DIRTY, 0);
 rb = compute_tlbie_rb(v, r, ptex);
 ppc_tlb_invalidate_one(env, rb);
 return REMOVE_SUCCESS;
@@ -280,7 +274,6 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 target_ulong flags = args[0];
 target_ulong pte_index = args[1];
 target_ulong avpn = args[2];
-hwaddr hpte;
 uint64_t token;
 target_ulong v, r, rb;
 
@@ -304,12 +297,11 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 r |= (flags << 48) & HPTE64_R_KEY_HI;
 r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
 rb = compute_tlbie_rb(v, r, pte_index);
-hpte = pte_index * HASH_PTE_SIZE_64;
-ppc_hash64_store_hpte0(env, hpte, (v & ~HPTE64_V_VALID) | 
HPTE64_V_HPTE_DIRTY);
+ppc_hash64_store_hpte(env, pte_index,
+  (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
 ppc_tlb_invalidate_one(env, rb);
-ppc_hash64_store_hpte1(env, hpte, r);
 /* Don't need a memory barrier, due to qemu's global lock */
-ppc_hash64_store_hpte0(env, hpte, v | HPTE64_V_HPTE_DIRTY);
+ppc_hash64_store_hpte(env, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
 return H_SUCCESS;
 }
 
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 68a6f69..8dd5d22 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -566,7 +566,8 @@ int ppc_hash64_handle_mmu_fault(CPUPPCState *env, 
target_ulong eaddr,
 }
 
 if (new_pte1 != pte.pte1) {
-ppc_hash64_store_hpte1(env, pte_offset, new_pte1);
+ppc_hash64_store_hpte(env, pte_offset / HASH_PTE_SIZE_64,
+  pte.pte0, new_pte1);
 }
 
 /* 7. Determine the real address from the PTE */
diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
index e7cb96f..49d866b 100644
--- a/target-ppc/mmu-hash64.h
+++ b/target-ppc/mmu-hash64.h
@@ -106,26 +106,18 @@ static inline target_ulong 
ppc_hash64_load_hpte1(CPUPPCState *env,
 }
 }
 
-static inline void ppc_hash64_store_hpte0(CPUPPCState *env,
-  hwaddr pte_offset, target_ulong pte0)
+static inline void ppc_hash64_store_hpte(CPUPPCState *env,
+ target_ulong pte_index,
+ target_ulong pte0, target_ulong pte1)
 {
 CPUState *cs = ENV_GET_CPU(env);
+pte_index *= HASH_PTE_SIZE_64;
 if (env->external_htab) {
-stq_p(env->external_htab + pte_offset, pte0);
+stq_p(env->external_htab + pte_index, pte0);
+stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64/2, pte1);
 } else {
-stq_phys(cs->as, env->htab_base + pte_offset, pte0);
-}
-}
-
-static inline void ppc_hash64_store_hpte1(CPUPPCStat

[Qemu-devel] [PATCH 2/4] target-ppc: Fix page table lookup with kvm enabled

[Greg Kurz]

From: Aneesh Kumar K.V 

With kvm enabled, we store the hash page table information in the hypervisor.
Use ioctl to read the htab contents. Without this we get the below error when
trying to read the guest address

 (gdb) x/10 do_fork
 0xc0098660 :   Cannot access memory at address 
0xc0098660
 (gdb)

Signed-off-by: Aneesh Kumar K.V 
[ fixes for 32 bit build (casts!), ldq_phys() API change,
  Greg Kurz 
---
 hw/ppc/spapr.c  |1 +
 hw/ppc/spapr_hcall.c|   50 +++---
 target-ppc/kvm.c|   54 +
 target-ppc/kvm_ppc.h|   19 +++
 target-ppc/mmu-hash64.c |   78 +++
 target-ppc/mmu-hash64.h |   22 +
 6 files changed, 184 insertions(+), 40 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8ac4d8a..94cf520 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -686,6 +686,7 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
 if (shift > 0) {
 /* Kernel handles htab, we don't need to allocate one */
 spapr->htab_shift = shift;
+kvmppc_kern_htab = true;
 } else {
 if (!spapr->htab) {
 /* Allocate an htab if we don't yet have one */
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index d19e3fc..7493302 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -61,8 +61,9 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment 
*spapr,
 target_ulong ptel = args[3];
 target_ulong page_shift = 12;
 target_ulong raddr;
-target_ulong i;
+target_ulong index;
 hwaddr hpte;
+uint64_t token;
 
 /* only handle 4k and 16M pages for now */
 if (pteh & HPTE64_V_LARGE) {
@@ -105,30 +106,37 @@ static target_ulong h_enter(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 if (!valid_pte_index(env, pte_index)) {
 return H_PARAMETER;
 }
+
+index = 0;
+hpte = pte_index * HASH_PTE_SIZE_64;
 if (likely((flags & H_EXACT) == 0)) {
 pte_index &= ~7ULL;
-hpte = pte_index * HASH_PTE_SIZE_64;
-for (i = 0; ; ++i) {
-if (i == 8) {
+token = ppc_hash64_start_access(cpu, pte_index);
+do {
+if (index == 8) {
+ppc_hash64_stop_access(token);
 return H_PTEG_FULL;
 }
-if ((ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) == 0) {
+if ((ppc_hash64_load_hpte0(env, token, index) & HPTE64_V_VALID) == 
0) {
 break;
 }
-hpte += HASH_PTE_SIZE_64;
-}
+} while (index++);
+ppc_hash64_stop_access(token);
 } else {
-i = 0;
-hpte = pte_index * HASH_PTE_SIZE_64;
-if (ppc_hash64_load_hpte0(env, hpte) & HPTE64_V_VALID) {
+token = ppc_hash64_start_access(cpu, pte_index);
+if (ppc_hash64_load_hpte0(env, token, 0) & HPTE64_V_VALID) {
+ppc_hash64_stop_access(token);
 return H_PTEG_FULL;
 }
+ppc_hash64_stop_access(token);
 }
+hpte += index * HASH_PTE_SIZE_64;
+
 ppc_hash64_store_hpte1(env, hpte, ptel);
 /* eieio();  FIXME: need some sort of barrier for smp? */
 ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
 
-args[0] = pte_index + i;
+args[0] = pte_index + index;
 return H_SUCCESS;
 }
 
@@ -145,16 +153,17 @@ static RemoveResult remove_hpte(CPUPPCState *env, 
target_ulong ptex,
 target_ulong *vp, target_ulong *rp)
 {
 hwaddr hpte;
+uint64_t token;
 target_ulong v, r, rb;
 
 if (!valid_pte_index(env, ptex)) {
 return REMOVE_PARM;
 }
 
-hpte = ptex * HASH_PTE_SIZE_64;
-
-v = ppc_hash64_load_hpte0(env, hpte);
-r = ppc_hash64_load_hpte1(env, hpte);
+token = ppc_hash64_start_access(ppc_env_get_cpu(env), ptex);
+v = ppc_hash64_load_hpte0(env, token, 0);
+r = ppc_hash64_load_hpte1(env, token, 0);
+ppc_hash64_stop_access(token);
 
 if ((v & HPTE64_V_VALID) == 0 ||
 ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
@@ -163,6 +172,7 @@ static RemoveResult remove_hpte(CPUPPCState *env, 
target_ulong ptex,
 }
 *vp = v;
 *rp = r;
+hpte = ptex * HASH_PTE_SIZE_64;
 ppc_hash64_store_hpte0(env, hpte, HPTE64_V_HPTE_DIRTY);
 rb = compute_tlbie_rb(v, r, ptex);
 ppc_tlb_invalidate_one(env, rb);
@@ -271,16 +281,17 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
sPAPREnvironment *spapr,
 target_ulong pte_index = args[1];
 target_ulong avpn = args[2];
 hwaddr hpte;
+uint64_t token;
 target_ulong v, r, rb;
 
 if (!valid_pte_index(env, pte_index)) {
 return H_PARAMETER;
 }
 
-hpte = pte_index * HASH_PTE_SIZE_64;
-
-v = ppc_hash64_load_hpte0(env, hpte);
-r = ppc_hash64_load_hpte1(env, hpte);
+token = ppc_hash64_start_access(cpu, pte_index);
+v = ppc_hash64_load_hpte0(env, token, 0);
+r = 

[Qemu-devel] [PATCH 0/4] target-ppc: htab fixes (V2)

[Greg Kurz]

On Mon, 17 Feb 2014 14:22:14 +0100
Greg Kurz  wrote:
> Hi,
> 
> This is a new tentative for the patches 2/5 to 5/5 from the "target-ppc:
> Add support for dumping guest memory using qemu gdb server" patchset:
> 
> https://lists.nongnu.org/archive/html/qemu-ppc/2014-01/msg00380.html
> 
> All patches have been rebased on the current ppc-next head (72c798d7dccc).
> 
> To ensure proper bisectability, the following was verified for each
> individual patch:
> •- 32 and 64 bit build of ppc-softmmu and ppc64-softmmu (fedora 19 ppc64)
> •- 64 bit pseries guest with KVM on a POWER7 host (fedora 19 ppc64)
> •- 64 bit pseries guest with 64 bit TCG on a x86_64 host (fedora 19 ppc64)
> •- 64 bit pseries guest with 32 bit TCG on a x86_64 host (fedora 19 ppc64)
> •- 32 bit mac99 guest with 64 bit TCG on a x86_64 host (wheezy ppc)
> •- 32 bit mac99 guest with 32 bit TCG on a x86_64 host (wheezy ppc)
> 
> Alex,
> 
> This should address all the requirements you expressed in your last mail.
> Please tell me if something is missing.

Alex,

I have added the 32 bit build with KVM enabled to catch the remaining build
breaks:

target-ppc/kvm.c: In function 'kvmppc_hash64_read_pteg':
target-ppc/kvm.c:1977:12: error: cast from pointer to integer of different size
 [-Werror=pointer-to-int-cast]
target-ppc/kvm.c: In function 'kvmppc_hash64_free_pteg':
target-ppc/kvm.c:1990:82: error: cast to pointer from integer of different size
 [-Werror=int-to-pointer-cast]

I have folded the fix into patch 2/4.

I have also fixed the SoB mess, and rebased on the current ppc-next.

Hope this time, it is okay ! :)

Best Regards.

---

Aneesh Kumar K.V (4):
target-ppc: Fix htab_mask calculation
target-ppc: Fix page table lookup with kvm enabled
target-ppc: Change the hpte store API
target-ppc: Update ppc_hash64_store_hpte to support updating in-kernel htab


 hw/ppc/spapr.c   |9 
 hw/ppc/spapr_hcall.c |   81 ++---
 target-ppc/cpu.h |1 
 target-ppc/kvm.c |   94 ++-
 target-ppc/kvm_ppc.h |   29 +
 target-ppc/machine.c |   11 +++--
 target-ppc/misc_helper.c |4 +-
 target-ppc/mmu-hash64.c  |  101 +++---
 target-ppc/mmu-hash64.h  |   47 -
 target-ppc/mmu_helper.c  |3 +
 10 files changed, 294 insertions(+), 86 deletions(-)

-- 
Greg

Re: [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Check qemu-img command line parsing

[Eric Blake]

On 02/20/2014 07:57 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/082 | 187 +++
>  tests/qemu-iotests/082.out | 436 
> +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 624 insertions(+)
>  create mode 100755 tests/qemu-iotests/082
>  create mode 100644 tests/qemu-iotests/082.out
> 

> +
> +seq=`basename $0`

[rambling side note - I ought to submit a patch that updates all this
copy-and-pasted boilerplate to use $() instead of ``]


> +echo === create: Options specified more than once ===
> +
> +# Last -f should win
> +run_qemu_img create -f foo -f $IMGFMT $TEST_IMG $size
> +run_qemu_img info $TEST_IMG
> +
> +# Multiple -o should be merged
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o lazy_refcounts=on 
> $TEST_IMG $size
> +run_qemu_img info $TEST_IMG

If we later fix my corner case of trailing ',', don't forget to also add
a test for the new behavior :)

> +
> +# Adding the help option to the same -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k,\? $TEST_IMG $size

You should probably also test -o help,cluster_size=4k.

> +
> +# Adding the help option to a separate -o option
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o help $TEST_IMG $size
> +run_qemu_img create -f $IMGFMT -o cluster_size=4k -o \? $TEST_IMG $size
> +
> +# Looks like a help option, but is part of the backing file name
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,help $TEST_IMG 
> $size
> +run_qemu_img create -f $IMGFMT -o backing_file=$TEST_IMG,,\? $TEST_IMG $size

Or, per my corner case:

-o backing_file=$TESET_IMG, -o help

is currently perversely part of the file name.

Even with my suggestions for further tests and improvements, this is
strictly better than what we previously had, so:

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] tcg/i386: Fix build for systems without working cpuid.h (MacOSX, Win32)

[Peter Maydell]

Win32 doesn't have a cpuid.h, and MacOSX may have one but without
the __cpuid() function we use, which means that commit 9d2eec20
broke the build for those platforms. Fix this by tightening up
our configure cpuid.h check to test that the functions we need
are present, and adding some missing #ifdef guerds in
tcg/i386/tcg-target.c.

Signed-off-by: Peter Maydell 
---
Tested with Linux x86/64 gcc build, Linux x86/64 clang build,
W32 cross-build and MacOSX 10.8 build. If somebody would like to
review this I'll apply it directly to unbreak things.
Apologies for not catching it before I pushed the tcg pullreq;
I had forgotten to add the 'build on w32' command to my script.

 configure | 8 +++-
 tcg/i386/tcg-target.c | 4 +++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 4648117..a2af9db 100755
--- a/configure
+++ b/configure
@@ -3564,7 +3564,13 @@ cpuid_h=no
 cat > $TMPC << EOF
 #include 
 int main(void) {
-  return 0;
+unsigned a, b, c, d;
+int max = __get_cpuid_max(0, 0);
+
+if (max >= 1) {
+__cpuid(1, a, b, c, d);
+}
+return 0;
 }
 EOF
 if compile_prog "" "" ; then
diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
index fef1717..f832282 100644
--- a/tcg/i386/tcg-target.c
+++ b/tcg/i386/tcg-target.c
@@ -115,7 +115,7 @@ static const int tcg_target_call_oarg_regs[] = {
is available.  */
 #if TCG_TARGET_REG_BITS == 64
 # define have_cmov 1
-#elif defined(CONFIG_CPUID_H)
+#elif defined(CONFIG_CPUID_H) && defined(bit_CMOV)
 static bool have_cmov;
 #else
 # define have_cmov 0
@@ -2295,6 +2295,7 @@ static void tcg_target_qemu_prologue(TCGContext *s)
 
 static void tcg_target_init(TCGContext *s)
 {
+#ifdef CONFIG_CPUID_H
 unsigned a, b, c, d;
 int max = __get_cpuid_max(0, 0);
 
@@ -2323,6 +2324,7 @@ static void tcg_target_init(TCGContext *s)
 have_bmi2 = (b & bit_BMI2) != 0;
 #endif
 }
+#endif
 
 if (TCG_TARGET_REG_BITS == 64) {
 tcg_regset_set32(tcg_target_available_regs[TCG_TYPE_I32], 0, 0x);
-- 
1.8.5

Re: [Qemu-devel] [PATCH 4/5] tcg/i386: Use ANDN instruction

[Peter Maydell]

On 20 February 2014 16:43, Richard Henderson  wrote:
> Can you try this?

[summarising an irc discussion]

That fixes W32 but not MacOSX, because there clang has a
cpuid.h but it doesn't have __get_cpuid_max() or __cpuid().
I'm just testing a patch which enhances our configure test
to check for these and puts ifdefs in the right places.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 00/20] Improve bdrv_open error messages

[Paolo Bonzini]

Il 19/02/2014 15:06, Kevin Wolf ha scritto:

Thanks, applied to the block branch.


Conflicts with master... to help you rebasing I put my own version at 
block-open-errors on git://github.com/bonzini/qemu.git.


Paolo

[Qemu-devel] [PATCH] qemu-iotests: add more tests to the "quick" group

[Paolo Bonzini]

None of these needs QEMU_PROG, and they all take but a few seconds.
We need to point the launching script to qemu-nbd, though.

Signed-off-by: Paolo Bonzini 
---
 tests/qemu-iotests-quick.sh |  1 +
 tests/qemu-iotests/group| 34 +-
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests-quick.sh b/tests/qemu-iotests-quick.sh
index cf90de0..c449e8a 100755
--- a/tests/qemu-iotests-quick.sh
+++ b/tests/qemu-iotests-quick.sh
@@ -8,6 +8,7 @@ export QEMU_PROG="this_should_be_unused"
 
 export QEMU_IMG_PROG="$(pwd)/qemu-img"
 export QEMU_IO_PROG="$(pwd)/qemu-io"
+export QEMU_NBD_PROG="$(pwd)/qemu-nbd"
 
 cd $SRC_PATH/tests/qemu-iotests
 
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d8be74a..46b8a93 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -58,28 +58,28 @@
 049 rw auto
 050 rw auto backing quick
 051 rw auto
-052 rw auto backing
-053 rw auto
-054 rw auto
+052 rw auto backing quick
+053 rw auto quick
+054 rw auto quick
 055 rw auto
 056 rw auto backing
 057 rw auto
-058 rw auto
-059 rw auto
-060 rw auto
-061 rw auto
-062 rw auto
-063 rw auto
-064 rw auto
+058 rw auto quick
+059 rw auto quick
+060 rw auto quick
+061 rw auto quick
+062 rw auto quick
+063 rw auto quick
+064 rw auto quick
 065 rw auto
-066 rw auto
+066 rw auto quick
 067 rw auto
 068 rw auto
-069 rw auto
-070 rw auto
+069 rw auto quick
+070 rw auto quick
 071 rw auto
-072 rw auto
-073 rw auto
-074 rw auto
-077 rw auto
+072 rw auto quick
+073 rw auto quick
+074 rw auto quick
+077 rw auto quick
 079 rw auto
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 2/2 v2] vfio: blacklist loading of unstable roms

[Bandan Das]

Alex Williamson  writes:

> On Wed, 2014-02-19 at 15:20 -0500, Bandan Das wrote:
>> Certain cards such as the Broadcom BCM57810 have rom quirks
>> that exhibit unstable system behavior duing device assignment. In
>> the particular case of 57810, rom execution hangs and if a FLR
>> follows, the device becomes inoperable until a power cycle. This
>> is a simple change to blacklist loading of rom for such cards
>> unless the user specifies a romfile or rombar=1 on the cmd line
>> 
>> Signed-off-by: Bandan Das 
>> ---
>>  hw/misc/vfio.c | 58 
>> ++
>>  1 file changed, 58 insertions(+)
>> 
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 8db182f..58f348e 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -209,6 +209,16 @@ typedef struct VFIOGroup {
>>  QLIST_ENTRY(VFIOGroup) container_next;
>>  } VFIOGroup;
>>  
>> +typedef struct VFIORomBlacklistEntry {
>> +uint16_t vendor_id;
>> +uint16_t device_id;
>> +} VFIORomBlacklistEntry;
>> +
>> +static const VFIORomBlacklistEntry romblacklist[] = {
>> +/* Broadcom BCM 57810 */
>> +{ 0x14e4, 0x168e }
>> +};
>> +
>
> Ideally we'd want to be able to reference a bugzilla here so we have
> some reference in the code to track developments.  Also, can we capture
> the ROM version known to cause this problem so if somebody later says
> that it works we can have something to compare?  The PCI firmware spec
> defines the data structure.  Effectively you can dump the ROM from the
> device, run xxd on it and look for the "PCIR" string that defines the
> start of the PCI data structure.  The 2 bytes at offset 12h (where 0h is
> the 'P' in "PCIR") have a revision level field.
>
>>  #define MSIX_CAP_LENGTH 12
>>  
>>  static QLIST_HEAD(, VFIOContainer)
>> @@ -1197,6 +1207,26 @@ static const MemoryRegionOps vfio_rom_ops = {
>>  .endianness = DEVICE_LITTLE_ENDIAN,
>>  };
>>  
>> +static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
>> +{
>> +PCIDevice *pdev = &vdev->pdev;
>> +uint16_t vendor_id, device_id;
>> +int count = 0;
>> +
>> +vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID);
>> +device_id = pci_get_word(pdev->config + PCI_DEVICE_ID);
>> +
>> +while (count < ARRAY_SIZE(romblacklist)) {
>> +if (romblacklist[count].vendor_id == vendor_id &&
>> +romblacklist[count].device_id == device_id) {
>> +return true;
>> +}
>> +count++;
>> +}
>> +
>> +return false;
>> +}
>> +
>>  static void vfio_pci_size_rom(VFIODevice *vdev)
>>  {
>>  uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
>> @@ -1204,9 +1234,37 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
>>  char name[32];
>>  
>>  if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>> +/* Since pci handles romfile, just print a message and return */
>> +if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
>> +error_printf("Warning : Device at %04x:%02x:%02x.%x "
>> + "is known to cause system instability issues 
>> during "
>> + "option rom execution. "
>> + "Proceeding anyway since user specified romfile\n",
>> + vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> + vdev->host.function);
>> +}
>>  return;
>>  }
>>  
>> +if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) {
>> +if (vdev->pdev.rom_bar == 1) {
>> +error_printf("Warning : Device at %04x:%02x:%02x.%x "
>> + "is known to cause system instability issues 
>> during "
>> + "option rom execution. "
>> + "Proceeding anyway since user specified 
>> rombar=1\n",
>> + vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> + vdev->host.function);
>> +} else {
>> +error_printf("Warning : Rom loading for device at "
>> + "%04x:%02x:%02x.%x has been disabled due to "
>> + "system instability issues. "
>> + "Specify rombar=1 or romfile to force\n",
>> + vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> + vdev->host.function);
>> +return;
>> +}
>> +}
>> +
>
> What happens if this card, or some later user of this blacklisting, has
> SKUs that don't have a ROM?  Aren't we going to print this last warning
> regardless of whether we found anything to load?  Just shifting this
> whole block down below the next couple tests is probably sufficient.
> Thanks,

Thanks for catching this! I will shift this down for the next version..

> Alex
>
>>  /*
>>   * Use the same size ROM BAR as the physical device.  The contents
>>   * will get filled in later when the guest tries to read it.