Re: [Qemu-devel] [PATCH v14 00/24] MTTCG Base enabling patches with ARM enablement

[Pranith Kumar]

On Sat, Feb 25, 2017 at 8:46 PM, Programmingkid
 wrote:
>
>
> With the i386 target, I did see host CPU usage go up to 120%. That was nice
> to see. Windows XP and Windows 2000 would not boot to the desktop for some
> reason.

For windows XP, you need the patch here:
http://patchwork.ozlabs.org/patch/731915/

Let me know if that patch helps.

Thanks,
--
Pranith

Re: [Qemu-devel] [PATCH v14 00/24] MTTCG Base enabling patches with ARM enablement

[Programmingkid]

On Feb 24, 2017, at 1:37 PM, Alex Bennée wrote:

> 
> G 3  writes:
> 
>> Hi I was wondering if your MTTCG patches have been tested with a
>> PowerPC guest yet.
> 
> You would have to talk to PPC guys about the current status of MTTCG for
> the PowerPC architecture. You can force it to run with -accel
> tcg,thread=multi but it will likely behave weirdly if the atomic/barrier
> updates haven't been made to the front end.
> 
>> Also do you have a repo someone could clone to test
>> out all your patches?
> 
> Sure:
> 
>  https://github.com/stsquad/qemu/tree/mttcg/base-patches-v14
> 
> Hopefully it will get merged in the next couple of days ;-)
> 
> --
> Alex Bennée


Ok I have tried out the PowerPC target with "-accel tcg,thread=multi".
What I saw while booting was this message:

kernel[0]: USBF:235.754 AppleUSBOHCI[0x1151000] Watchdog detected
dead controller (hcca #: 0, hc #: 836)


The maximum host cpu usgae for QEMU went up, but only by 2 percent. Before
qemu-system-ppc would use at most 101% of the host CPU. With the accel option, 
it went
up to 104%.

My Mac OS X guest was able to boot up and shutdown correctly with your patches.

With the i386 target, I did see host CPU usage go up to 120%. That was nice
to see. Windows XP and Windows 2000 would not boot to the desktop for some
reason.

Your patches can be a real game changer for emulation. Please let me know
if there is anything else I could do.

Re: [Qemu-devel] [PATCH 06/10] sm501: Fix device endianness

[BALATON Zoltan]

On Sat, 25 Feb 2017, Peter Maydell wrote:

On 24 February 2017 at 20:35, BALATON Zoltan  wrote:

On Fri, 24 Feb 2017, Peter Maydell wrote:

On 19 February 2017 at 16:35, BALATON Zoltan  wrote:


Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c  |  6 +++---
 hw/display/sm501_template.h | 31 ++-
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 9091bb5..3d32a3c 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -846,7 +846,7 @@ static const MemoryRegionOps sm501_system_config_ops
= {
 .min_access_size = 4,
 .max_access_size = 4,
 },
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };

 static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
@@ -1082,7 +1082,7 @@ static const MemoryRegionOps sm501_disp_ctrl_ops =
{
 .min_access_size = 4,
 .max_access_size = 4,
 },
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };

 static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
@@ -1170,7 +1170,7 @@ static const MemoryRegionOps sm501_2d_engine_ops =
{
 .min_access_size = 4,
 .max_access_size = 4,
 },
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };



Does this still all work for the sh4eb (big-endian) sysbus device case?



Not sure. Is there some image somewhere I can test with?


I don't know, I'm afraid; I don't know anything about sh4.
I'm just going by looking at the code changes and flagging
up things which are potentially problems.


 /* draw line functions for all console modes */
diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index 832ee61..5b516d6 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -64,10 +64,16 @@ static void glue(draw_line16_, PIXEL_NAME)(
 uint8_t r, g, b;

 do {
-rgb565 = lduw_p(s);
-r = ((rgb565 >> 11) & 0x1f) << 3;
-g = ((rgb565 >>  5) & 0x3f) << 2;
-b = ((rgb565 >>  0) & 0x1f) << 3;
+rgb565 = lduw_le_p(s);
+#if defined(TARGET_WORDS_BIGENDIAN)
+r = (rgb565 >> 8) & 0xf8;
+g = (rgb565 >> 3) & 0xfc;
+b = (rgb565 << 3) & 0xf8;
+#else
+b = (rgb565 >> 8) & 0xf8;
+g = (rgb565 >> 3) & 0xfc;
+r = (rgb565 << 3) & 0xf8;
+#endif
 *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
 s += 2;
 d += BPP;
@@ -80,15 +86,14 @@ static void glue(draw_line32_, PIXEL_NAME)(
 uint8_t r, g, b;

 do {
-ldub_p(s);
 #if defined(TARGET_WORDS_BIGENDIAN)
+r = s[0];
+g = s[1];
+b = s[2];
+#else
 r = s[1];
 g = s[2];
 b = s[3];
-#else
-b = s[0];
-g = s[1];
-r = s[2];
 #endif



This looks really suspicious. Previously we had
TARGET_WORDS_BIGENDIAN -> bytes are XRGB
otherwise bytes are BGRX

Now we have
TARGET_WORDS_BIGENDIAN -> bytes are RGBX
otherwise  -> bytes are XRGB

That doesn't seem very plausible at all.



I've tested it with sh4 and ppc guests running on x86_64 host and these work
now while previous code resulted in mixed up colors. Maybe the host
endianness could also be a factor and the previous code assumed big endian
host or the previous code was already broken and only worked with the
default 8 bit depth. I'm not completely sure I understand endian handling in
QEMU to know if this is correct besides on what I've tested but at least
little endian and big endian guests should work on little endian hosts now
with my patch. I can't test on big endian host.

I've used this image: https://people.debian.org/~aurel32/qemu/sh4/
with video=800x600-16 kernel parameter where changing -16 to different bit
depths reproduces the problem.


Again, I don't know sh, and I don't know this particular device.
I'm just pointing out that the code change here looks really
implausible. Maybe the data sheet says that this is what the
pixel format looks like; but it's very odd.


I don't know much about it either. I could not find docs about framebuffer 
endianness of this device but all clients I've seen treat it as little 
endian, both on little endian SH4 and big endian PPC. Maybe the previous 
code was already broken, because on a black & white text console (like the 
SH4 test image listed on qemu.org) the problem is not obvious, it still 
looks like white text on black with mixed up color components. It can only 
be seen when some graphics is displayed like the Tux logo on framebuffer 
in the above Debian image. If it ever worked it could have been easily 
broken by some qemu console changes. Or maybe it worked on big endian host 
only but I could not verify that. All I can say it works for me now on LE 
host with both LE and BE guests that I could test.

[Qemu-devel] [PATCH v2 08/14] sm501: Fix hardware cursor

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c  | 169 +---
 hw/display/sm501_template.h |  25 +++
 2 files changed, 107 insertions(+), 87 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 2694081..c4bdc50 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -553,6 +553,24 @@ static uint32_t get_local_mem_size_index(uint32_t size)
 return index;
 }
 
+static inline int get_width(SM501State *s, int crt)
+{
+int width = crt ? s->dc_crt_h_total : s->dc_panel_h_total;
+return (width & 0x0FFF) + 1;
+}
+
+static inline int get_height(SM501State *s, int crt)
+{
+int height = crt ? s->dc_crt_v_total : s->dc_panel_v_total;
+return (height & 0x0FFF) + 1;
+}
+
+static inline int get_bpp(SM501State *s, int crt)
+{
+int bpp = crt ? s->dc_crt_control : s->dc_panel_control;
+return (8 << (bpp & 3)) / 8;
+}
+
 /**
  * Check the availability of hardware cursor.
  * @param crt  0 for PANEL, 1 for CRT.
@@ -567,10 +585,10 @@ static inline int is_hwc_enabled(SM501State *state, int 
crt)
  * Get the address which holds cursor pattern data.
  * @param crt  0 for PANEL, 1 for CRT.
  */
-static inline uint32_t get_hwc_address(SM501State *state, int crt)
+static inline uint8_t *get_hwc_address(SM501State *state, int crt)
 {
 uint32_t addr = crt ? state->dc_crt_hwc_addr : state->dc_panel_hwc_addr;
-return (addr & 0x03F0)/* >> 4*/;
+return state->local_mem + (addr & 0x03F0);
 }
 
 /**
@@ -596,50 +614,48 @@ static inline uint32_t get_hwc_x(SM501State *state, int 
crt)
 }
 
 /**
- * Get the cursor position in x coordinate.
+ * Get the hardware cursor palette.
  * @param crt  0 for PANEL, 1 for CRT.
- * @param index  0, 1, 2 or 3 which specifies color of corsor dot.
+ * @param palette  pointer to a [3 * 3] array to store color values in
  */
-static inline uint16_t get_hwc_color(SM501State *state, int crt, int index)
+static inline void get_hwc_palette(SM501State *state, int crt, uint8_t 
*palette)
 {
-uint32_t color_reg = 0;
-uint16_t color_565 = 0;
-
-if (index == 0) {
-return 0;
-}
-
-switch (index) {
-case 1:
-case 2:
-color_reg = crt ? state->dc_crt_hwc_color_1_2
-: state->dc_panel_hwc_color_1_2;
-break;
-case 3:
-color_reg = crt ? state->dc_crt_hwc_color_3
-: state->dc_panel_hwc_color_3;
-break;
-default:
-printf("invalid hw cursor color.\n");
-abort();
-}
+int i;
+uint32_t color_reg;
+uint16_t rgb565;
+
+for (i = 0; i < 3; i++) {
+if (i + 1 == 3) {
+color_reg = crt ? state->dc_crt_hwc_color_3
+: state->dc_panel_hwc_color_3;
+} else {
+color_reg = crt ? state->dc_crt_hwc_color_1_2
+: state->dc_panel_hwc_color_1_2;
+}
 
-switch (index) {
-case 1:
-case 3:
-color_565 = (uint16_t)(color_reg & 0x);
-break;
-case 2:
-color_565 = (uint16_t)((color_reg >> 16) & 0x);
-break;
+if (i + 1 == 2) {
+rgb565 = (color_reg >> 16) & 0x;
+} else {
+rgb565 = color_reg & 0x;
+}
+palette[i * 3 + 0] = (rgb565 << 3) & 0xf8; /* red */
+palette[i * 3 + 1] = (rgb565 >> 3) & 0xfc; /* green */
+palette[i * 3 + 2] = (rgb565 >> 8) & 0xf8; /* blue */
 }
-return color_565;
 }
 
-static int within_hwc_y_range(SM501State *state, int y, int crt)
+static inline void hwc_invalidate(SM501State *s, int crt)
 {
-int hwc_y = get_hwc_y(state, crt);
-return (hwc_y <= y && y < hwc_y + SM501_HWC_HEIGHT);
+int w = get_width(s, crt);
+int h = get_height(s, crt);
+int bpp = get_bpp(s, crt);
+int start = get_hwc_y(s, crt);
+int end = MIN(h, start + SM501_HWC_HEIGHT) + 1;
+
+start *= w * bpp;
+end *= w * bpp;
+
+memory_region_set_dirty(>local_mem_region, start, end - start);
 }
 
 static void sm501_2d_operation(SM501State *s)
@@ -1020,10 +1036,18 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr 
addr,
 break;
 
 case SM501_DC_PANEL_HWC_ADDR:
-s->dc_panel_hwc_addr = value & 0x8FF0;
+value &= 0x8FF0;
+if (value != s->dc_panel_hwc_addr) {
+hwc_invalidate(s, 0);
+s->dc_panel_hwc_addr = value;
+}
 break;
 case SM501_DC_PANEL_HWC_LOC:
-s->dc_panel_hwc_location = value & 0x0FFF0FFF;
+value &= 0x0FFF0FFF;
+if (value != s->dc_panel_hwc_location) {
+hwc_invalidate(s, 0);
+s->dc_panel_hwc_location = value;
+}
 break;
 case SM501_DC_PANEL_HWC_COLOR_1_2:
 s->dc_panel_hwc_color_1_2 = value;
@@ -1055,10 +1079,18 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr 
addr,
 break;
 
 case 

[Qemu-devel] [PATCH v2 01/14] sm501: Fixed code style and a few typos in comments

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
Reviewed-by: Peter Maydell 
---
 hw/display/sm501.c  | 1132 ++-
 hw/display/sm501_template.h |   52 +-
 2 files changed, 594 insertions(+), 590 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 040a0b9..4f40dee 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -38,7 +38,7 @@
 /*
  * Status: 2010/05/07
  *   - Minimum implementation for Linux console : mmio regs and CRT layer.
- *   - 2D grapihcs acceleration partially supported : only fill rectangle.
+ *   - 2D graphics acceleration partially supported : only fill rectangle.
  *
  * TODO:
  *   - Panel support
@@ -49,13 +49,13 @@
  *   - Performance tuning
  */
 
-//#define DEBUG_SM501
-//#define DEBUG_BITBLT
+/*#define DEBUG_SM501*/
+/*#define DEBUG_BITBLT*/
 
 #ifdef DEBUG_SM501
 #define SM501_DPRINTF(fmt, ...) printf(fmt, ## __VA_ARGS__)
 #else
-#define SM501_DPRINTF(fmt, ...) do {} while(0)
+#define SM501_DPRINTF(fmt, ...) do {} while (0)
 #endif
 
 
@@ -65,379 +65,379 @@
 
 /* System Configuration area */
 /* System config base */
-#define SM501_SYS_CONFIG   (0x00)
+#define SM501_SYS_CONFIG(0x00)
 
 /* config 1 */
-#define SM501_SYSTEM_CONTROL   (0x00)
+#define SM501_SYSTEM_CONTROL(0x00)
 
-#define SM501_SYSCTRL_PANEL_TRISTATE   (1<<0)
-#define SM501_SYSCTRL_MEM_TRISTATE (1<<1)
-#define SM501_SYSCTRL_CRT_TRISTATE (1<<2)
+#define SM501_SYSCTRL_PANEL_TRISTATE(1 << 0)
+#define SM501_SYSCTRL_MEM_TRISTATE  (1 << 1)
+#define SM501_SYSCTRL_CRT_TRISTATE  (1 << 2)
 
-#define SM501_SYSCTRL_PCI_SLAVE_BURST_MASK (3<<4)
-#define SM501_SYSCTRL_PCI_SLAVE_BURST_1(0<<4)
-#define SM501_SYSCTRL_PCI_SLAVE_BURST_2(1<<4)
-#define SM501_SYSCTRL_PCI_SLAVE_BURST_4(2<<4)
-#define SM501_SYSCTRL_PCI_SLAVE_BURST_8(3<<4)
+#define SM501_SYSCTRL_PCI_SLAVE_BURST_MASK (3 << 4)
+#define SM501_SYSCTRL_PCI_SLAVE_BURST_1 (0 << 4)
+#define SM501_SYSCTRL_PCI_SLAVE_BURST_2 (1 << 4)
+#define SM501_SYSCTRL_PCI_SLAVE_BURST_4 (2 << 4)
+#define SM501_SYSCTRL_PCI_SLAVE_BURST_8 (3 << 4)
 
-#define SM501_SYSCTRL_PCI_CLOCK_RUN_EN (1<<6)
-#define SM501_SYSCTRL_PCI_RETRY_DISABLE(1<<7)
-#define SM501_SYSCTRL_PCI_SUBSYS_LOCK  (1<<11)
-#define SM501_SYSCTRL_PCI_BURST_READ_EN(1<<15)
+#define SM501_SYSCTRL_PCI_CLOCK_RUN_EN  (1 << 6)
+#define SM501_SYSCTRL_PCI_RETRY_DISABLE (1 << 7)
+#define SM501_SYSCTRL_PCI_SUBSYS_LOCK   (1 << 11)
+#define SM501_SYSCTRL_PCI_BURST_READ_EN (1 << 15)
 
 /* miscellaneous control */
 
-#define SM501_MISC_CONTROL (0x04)
+#define SM501_MISC_CONTROL  (0x04)
 
-#define SM501_MISC_BUS_SH  (0x0)
-#define SM501_MISC_BUS_PCI (0x1)
-#define SM501_MISC_BUS_XSCALE  (0x2)
-#define SM501_MISC_BUS_NEC (0x6)
-#define SM501_MISC_BUS_MASK(0x7)
+#define SM501_MISC_BUS_SH   (0x0)
+#define SM501_MISC_BUS_PCI  (0x1)
+#define SM501_MISC_BUS_XSCALE   (0x2)
+#define SM501_MISC_BUS_NEC  (0x6)
+#define SM501_MISC_BUS_MASK (0x7)
 
-#define SM501_MISC_VR_62MB (1<<3)
-#define SM501_MISC_CDR_RESET   (1<<7)
-#define SM501_MISC_USB_LB  (1<<8)
-#define SM501_MISC_USB_SLAVE   (1<<9)
-#define SM501_MISC_BL_1(1<<10)
-#define SM501_MISC_MC  (1<<11)
-#define SM501_MISC_DAC_POWER   (1<<12)
-#define SM501_MISC_IRQ_INVERT  (1<<16)
-#define SM501_MISC_SH  (1<<17)
+#define SM501_MISC_VR_62MB  (1 << 3)
+#define SM501_MISC_CDR_RESET(1 << 7)
+#define SM501_MISC_USB_LB   (1 << 8)
+#define SM501_MISC_USB_SLAVE(1 << 9)
+#define SM501_MISC_BL_1 (1 << 10)
+#define SM501_MISC_MC   (1 << 11)
+#define SM501_MISC_DAC_POWER(1 << 12)
+#define SM501_MISC_IRQ_INVERT   (1 << 16)
+#define SM501_MISC_SH   (1 << 17)
 
-#define SM501_MISC_HOLD_EMPTY  (0<<18)
-#define SM501_MISC_HOLD_8  (1<<18)
-#define SM501_MISC_HOLD_16 (2<<18)
-#define SM501_MISC_HOLD_24 (3<<18)
-#define SM501_MISC_HOLD_32 (4<<18)
-#define SM501_MISC_HOLD_MASK   (7<<18)
+#define SM501_MISC_HOLD_EMPTY   (0 << 18)
+#define SM501_MISC_HOLD_8   (1 << 18)
+#define SM501_MISC_HOLD_16  (2 << 18)
+#define SM501_MISC_HOLD_24  (3 << 18)
+#define SM501_MISC_HOLD_32  (4 << 18)
+#define SM501_MISC_HOLD_MASK(7 << 18)
 
-#define SM501_MISC_FREQ_12 (1<<24)
-#define SM501_MISC_PNL_24BIT   (1<<25)
-#define SM501_MISC_8051_LE (1<<26)
+#define SM501_MISC_FREQ_12  (1 << 24)
+#define SM501_MISC_PNL_24BIT(1 << 25)
+#define SM501_MISC_8051_LE  (1 << 

Re: [Qemu-devel] [PATCH 03/10] sm501: QOMify

[BALATON Zoltan]

On Sat, 25 Feb 2017, Peter Maydell wrote:

On 24 February 2017 at 20:23, BALATON Zoltan  wrote:

The SM501/SM502 is a multimedia chip that besides a display controller also
contains some other functions (see
http://cateee.net/lkddb/web-lkddb/MFD_SM501.html) and this is what is
emulated here as these are part of the chip itself.


Hmm; that's pretty ugly but I guess it's sort of like a container
device that needs to contain various things inside it.



-/* create qemu graphic console */
-s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
+static Property sm501_sysbus_properties[] = {
+DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
+DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0),
+DEFINE_PROP_PTR("chr-state", SM501SysBusState, chr_state),


Pointer properties and properties which tell the device what
address it's at are both signs that something's not quite
modelled right. There may be no better way to do things right
now, or then again there may be.


Maybe but I think that would not be part of this series but some other 
clean up separately. This series makes it a bit cleaner but does not aim 
to fix everything.



+DEFINE_PROP_END_OF_LIST(),
+};




+static void sm501_sysbus_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->realize = sm501_realize_sysbus;
+set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+dc->desc = "SM501 Multimedia Companion";
+dc->props = sm501_sysbus_properties;
+/* Note: pointer property "chr-state" may remain null, thus
+ * no need for dc->cannot_instantiate_with_device_add_yet = true;
+ */
}



You also need a VMState struct registered in dc->vmsd and a reset
function registered in dc->reset.



Even if no migration is supported? Is there a simple example I could look at
on what should go into these?


The idea is to support migration. There are examples of doing
VMState structures all over the tree. You just need a structure
that lists all the bits of your state data structure that
contain guest-modifiable state.

Reset is straightforward: it's just a function that resets
the state of the device as if the system had been hard
powercycled.


I've tried to add these but vmstate is only added to the PCI version 
because the OHCI device I've looked at also does the same, which is likely 
beacuse the sysbus version is only used on SH4 which does not support 
migration anyway. The PCI verison can be instantiated on machines that 
have migration support so it makes sense to do it there. I hope this is 
acceptable.

[Qemu-devel] [PATCH v2 09/14] sm501: Misc clean ups

[BALATON Zoltan]

- Rename a variable
- Move variable declarations out of loop to the beginning in draw_hwc_line

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c  | 10 +-
 hw/display/sm501_template.h | 10 --
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index c4bdc50..9c5ded3 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1312,7 +1312,7 @@ static void sm501_draw_crt(SM501State *s)
 uint32_t *palette = (uint32_t *)>dc_palette[SM501_DC_CRT_PALETTE -
SM501_DC_PANEL_PALETTE];
 uint8_t hwc_palette[3 * 3];
-int ds_depth_index = get_depth_index(surface);
+int dst_depth_index = get_depth_index(surface);
 draw_line_func *draw_line = NULL;
 draw_hwc_line_func *draw_hwc_line = NULL;
 int full_update = 0;
@@ -1324,13 +1324,13 @@ static void sm501_draw_crt(SM501State *s)
 /* choose draw_line function */
 switch (src_bpp) {
 case 1:
-draw_line = draw_line8_funcs[ds_depth_index];
+draw_line = draw_line8_funcs[dst_depth_index];
 break;
 case 2:
-draw_line = draw_line16_funcs[ds_depth_index];
+draw_line = draw_line16_funcs[dst_depth_index];
 break;
 case 4:
-draw_line = draw_line32_funcs[ds_depth_index];
+draw_line = draw_line32_funcs[dst_depth_index];
 break;
 default:
 printf("sm501 draw crt : invalid DC_CRT_CONTROL=%x.\n",
@@ -1342,7 +1342,7 @@ static void sm501_draw_crt(SM501State *s)
 /* set up to draw hardware cursor */
 if (is_hwc_enabled(s, 1)) {
 /* choose cursor draw line function */
-draw_hwc_line = draw_hwc_line_funcs[ds_depth_index];
+draw_hwc_line = draw_hwc_line_funcs[dst_depth_index];
 hwc_src = get_hwc_address(s, 1);
 c_x = get_hwc_x(s, 1);
 c_y = get_hwc_y(s, 1);
diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index 39f2b67..c57f91a 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -108,7 +108,7 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(uint8_t *d, 
const uint8_t *s,
  int width, const uint8_t *palette, int c_x, int c_y)
 {
 int i;
-uint8_t bitset = 0;
+uint8_t r, g, b, v, bitset = 0;
 
 /* get cursor position */
 assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
@@ -116,8 +116,6 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(uint8_t *d, 
const uint8_t *s,
 d += c_x * BPP;
 
 for (i = 0; i < SM501_HWC_WIDTH && c_x + i < width; i++) {
-uint8_t v;
-
 /* get pixel value */
 if (i % 4 == 0) {
 bitset = ldub_p(s);
@@ -129,9 +127,9 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(uint8_t *d, 
const uint8_t *s,
 /* write pixel */
 if (v) {
 v--;
-uint8_t r = palette[v * 3 + 0];
-uint8_t g = palette[v * 3 + 1];
-uint8_t b = palette[v * 3 + 2];
+r = palette[v * 3 + 0];
+g = palette[v * 3 + 1];
+b = palette[v * 3 + 2];
 *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
 }
 d += BPP;
-- 
2.7.4

[Qemu-devel] [PATCH v2 11/14] sm501: Add some more missing registers

[BALATON Zoltan]

Values are not used yet, only stored to allow clients to initialise
these without failing as long as no 2D engine function is called that
would use the written value.

Signed-off-by: BALATON Zoltan 
---

v2: Fixed mask of video_control register for a read only bit
Changed IRQ status register to write ignored as IRQ is not implemented

 hw/display/sm501.c | 49 -
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index caa7e5c..af5e4db 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -511,6 +511,8 @@ typedef struct SM501State {
 uint32_t dc_panel_hwc_color_1_2;
 uint32_t dc_panel_hwc_color_3;
 
+uint32_t dc_video_control;
+
 uint32_t dc_crt_control;
 uint32_t dc_crt_fb_addr;
 uint32_t dc_crt_fb_offset;
@@ -530,13 +532,20 @@ typedef struct SM501State {
 uint32_t twoD_control;
 uint32_t twoD_pitch;
 uint32_t twoD_foreground;
+uint32_t twoD_background;
 uint32_t twoD_stretch;
+uint32_t twoD_color_compare;
 uint32_t twoD_color_compare_mask;
 uint32_t twoD_mask;
+uint32_t twoD_clip_tl;
+uint32_t twoD_clip_br;
+uint32_t twoD_mono_pattern_low;
+uint32_t twoD_mono_pattern_high;
 uint32_t twoD_window_width;
 uint32_t twoD_source_base;
 uint32_t twoD_destination_base;
-
+uint32_t twoD_alpha;
+uint32_t twoD_wrap;
 } SM501State;
 
 static uint32_t get_local_mem_size_index(uint32_t size)
@@ -945,6 +954,10 @@ static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr 
addr,
 ret = s->dc_panel_v_sync;
 break;
 
+case SM501_DC_VIDEO_CONTROL:
+ret = s->dc_video_control;
+break;
+
 case SM501_DC_CRT_CONTROL:
 ret = s->dc_crt_control;
 break;
@@ -1060,6 +1073,10 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr 
addr,
 s->dc_panel_hwc_color_3 = value & 0x;
 break;
 
+case SM501_DC_VIDEO_CONTROL:
+s->dc_video_control = value & 0x00037FFF;
+break;
+
 case SM501_DC_CRT_CONTROL:
 s->dc_crt_control = value & 0x0003;
 break;
@@ -1135,6 +1152,9 @@ static uint64_t sm501_2d_engine_read(void *opaque, hwaddr 
addr,
 case SM501_2D_SOURCE_BASE:
 ret = s->twoD_source_base;
 break;
+case SM501_2D_STATUS:
+ret = 0; /* Should return interrupt status */
+break;
 default:
 printf("sm501 disp ctrl : not implemented register read."
" addr=%x\n", (int)addr);
@@ -1177,15 +1197,33 @@ static void sm501_2d_engine_write(void *opaque, hwaddr 
addr,
 case SM501_2D_FOREGROUND:
 s->twoD_foreground = value;
 break;
+case SM501_2D_BACKGROUND:
+s->twoD_background = value;
+break;
 case SM501_2D_STRETCH:
 s->twoD_stretch = value;
 break;
+case SM501_2D_COLOR_COMPARE:
+s->twoD_color_compare = value;
+break;
 case SM501_2D_COLOR_COMPARE_MASK:
 s->twoD_color_compare_mask = value;
 break;
 case SM501_2D_MASK:
 s->twoD_mask = value;
 break;
+case SM501_2D_CLIP_TL:
+s->twoD_clip_tl = value;
+break;
+case SM501_2D_CLIP_BR:
+s->twoD_clip_br = value;
+break;
+case SM501_2D_MONO_PATTERN_LOW:
+s->twoD_mono_pattern_low = value;
+break;
+case SM501_2D_MONO_PATTERN_HIGH:
+s->twoD_mono_pattern_high = value;
+break;
 case SM501_2D_WINDOW_WIDTH:
 s->twoD_window_width = value;
 break;
@@ -1195,6 +1233,15 @@ static void sm501_2d_engine_write(void *opaque, hwaddr 
addr,
 case SM501_2D_DESTINATION_BASE:
 s->twoD_destination_base = value;
 break;
+case SM501_2D_ALPHA:
+s->twoD_alpha = value;
+break;
+case SM501_2D_WRAP:
+s->twoD_wrap = value;
+break;
+case SM501_2D_STATUS:
+/* ignored, writing 0 should clear interrupt status */
+break;
 default:
 printf("sm501 2d engine : not implemented register write."
" addr=%x, val=%x\n", (int)addr, (unsigned)value);
-- 
2.7.4

[Qemu-devel] [PATCH v2 03/14] sm501: QOMify

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---

v2: Add memory regions to device state instead of allocating them

 hw/display/sm501.c   | 139 +++
 hw/sh4/r2d.c |  11 +++-
 include/hw/devices.h |   5 --
 3 files changed, 106 insertions(+), 49 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 4eb085c..22cac97 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -58,8 +58,8 @@
 #define SM501_DPRINTF(fmt, ...) do {} while (0)
 #endif
 
-
 #define MMIO_BASE_OFFSET 0x3e0
+#define MMIO_SIZE 0x20
 
 /* SM501 register definitions taken from "linux/include/linux/sm501-regs.h" */
 
@@ -464,6 +464,10 @@ typedef struct SM501State {
 uint32_t local_mem_size_index;
 uint8_t *local_mem;
 MemoryRegion local_mem_region;
+MemoryRegion mmio_region;
+MemoryRegion system_config_region;
+MemoryRegion disp_ctrl_region;
+MemoryRegion twoD_engine_region;
 uint32_t last_width;
 uint32_t last_height;
 
@@ -1396,67 +1400,118 @@ static const GraphicHwOps sm501_ops = {
 .gfx_update  = sm501_update_display,
 };
 
-void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
-uint32_t local_mem_bytes, qemu_irq irq, Chardev *chr)
+static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base,
+   uint32_t local_mem_bytes)
 {
-SM501State *s;
-DeviceState *dev;
-MemoryRegion *sm501_system_config = g_new(MemoryRegion, 1);
-MemoryRegion *sm501_disp_ctrl = g_new(MemoryRegion, 1);
-MemoryRegion *sm501_2d_engine = g_new(MemoryRegion, 1);
-
-/* allocate management data region */
-s = g_new0(SM501State, 1);
 s->base = base;
 s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
-SM501_DPRINTF("local mem size=%x. index=%d\n", get_local_mem_size(s),
+SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s),
   s->local_mem_size_index);
 s->system_control = 0x0010; /* 2D engine FIFO empty */
 s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
 s->dc_panel_control = 0x0001; /* FIFO level 3 */
 s->dc_crt_control = 0x0001;
 
-/* allocate local memory */
-memory_region_init_ram(>local_mem_region, NULL, "sm501.local",
+/* local memory */
+memory_region_init_ram(>local_mem_region, OBJECT(dev), "sm501.local",
local_mem_bytes, _fatal);
 vmstate_register_ram_global(>local_mem_region);
 memory_region_set_log(>local_mem_region, true, DIRTY_MEMORY_VGA);
 s->local_mem = memory_region_get_ram_ptr(>local_mem_region);
-memory_region_add_subregion(address_space_mem, base, >local_mem_region);
-
-/* map mmio */
-memory_region_init_io(sm501_system_config, NULL, _system_config_ops,
-  s, "sm501-system-config", 0x6c);
-memory_region_add_subregion(address_space_mem, base + MMIO_BASE_OFFSET,
-sm501_system_config);
-memory_region_init_io(sm501_disp_ctrl, NULL, _disp_ctrl_ops, s,
+
+/* mmio */
+memory_region_init(>mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
+memory_region_init_io(>system_config_region, OBJECT(dev),
+  _system_config_ops, s,
+  "sm501-system-config", 0x6c);
+memory_region_add_subregion(>mmio_region, SM501_SYS_CONFIG,
+>system_config_region);
+memory_region_init_io(>disp_ctrl_region, OBJECT(dev),
+  _disp_ctrl_ops, s,
   "sm501-disp-ctrl", 0x1000);
-memory_region_add_subregion(address_space_mem,
-base + MMIO_BASE_OFFSET + SM501_DC,
-sm501_disp_ctrl);
-memory_region_init_io(sm501_2d_engine, NULL, _2d_engine_ops, s,
+memory_region_add_subregion(>mmio_region, SM501_DC,
+>disp_ctrl_region);
+memory_region_init_io(>twoD_engine_region, OBJECT(dev),
+  _2d_engine_ops, s,
   "sm501-2d-engine", 0x54);
-memory_region_add_subregion(address_space_mem,
-base + MMIO_BASE_OFFSET + SM501_2D_ENGINE,
-sm501_2d_engine);
+memory_region_add_subregion(>mmio_region, SM501_2D_ENGINE,
+>twoD_engine_region);
+
+/* create qemu graphic console */
+s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
+}
+
+#define TYPE_SYSBUS_SM501 "sysbus-sm501"
+#define SYSBUS_SM501(obj) \
+OBJECT_CHECK(SM501SysBusState, (obj), TYPE_SYSBUS_SM501)
+
+typedef struct {
+/*< private >*/
+SysBusDevice parent_obj;
+/*< public >*/
+SM501State state;
+uint32_t vram_size;
+uint32_t base;
+void *chr_state;
+} SM501SysBusState;
+
+static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
+{
+

[Qemu-devel] [PATCH v2 05/14] sm501: Add emulation of chip connected via PCI

[BALATON Zoltan]

Only the display controller part is created automatically on PCI

Signed-off-by: BALATON Zoltan 
---

v2: Split off removing dependency on base address to separate patch

 hw/display/sm501.c | 52 
 1 file changed, 52 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 1cda127..d9219bd 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -31,6 +31,7 @@
 #include "ui/console.h"
 #include "hw/devices.h"
 #include "hw/sysbus.h"
+#include "hw/pci/pci.h"
 #include "qemu/range.h"
 #include "ui/pixel_ops.h"
 #include "exec/address-spaces.h"
@@ -1507,9 +1508,60 @@ static const TypeInfo sm501_sysbus_info = {
 .class_init= sm501_sysbus_class_init,
 };
 
+#define TYPE_PCI_SM501 "sm501"
+#define PCI_SM501(obj) OBJECT_CHECK(SM501PCIState, (obj), TYPE_PCI_SM501)
+
+typedef struct {
+/*< private >*/
+PCIDevice parent_obj;
+/*< public >*/
+SM501State state;
+uint32_t vram_size;
+} SM501PCIState;
+
+static void sm501_realize_pci(PCIDevice *dev, Error **errp)
+{
+SM501PCIState *s = PCI_SM501(dev);
+
+sm501_init(>state, DEVICE(dev), s->vram_size);
+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
+ >state.local_mem_region);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
+ >state.mmio_region);
+}
+
+static Property sm501_pci_properties[] = {
+DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size,
+   64 * 1024 * 1024),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void sm501_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+k->realize = sm501_realize_pci;
+k->vendor_id = 0x126f;
+k->device_id = 0x0501;
+k->class_id = PCI_CLASS_DISPLAY_OTHER;
+set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+dc->desc = "SM501 Display Controller";
+dc->props = sm501_pci_properties;
+dc->hotpluggable = false;
+}
+
+static const TypeInfo sm501_pci_info = {
+.name  = TYPE_PCI_SM501,
+.parent= TYPE_PCI_DEVICE,
+.instance_size = sizeof(SM501PCIState),
+.class_init= sm501_pci_class_init,
+};
+
 static void sm501_register_types(void)
 {
 type_register_static(_sysbus_info);
+type_register_static(_pci_info);
 }
 
 type_init(sm501_register_types)
-- 
2.7.4

[Qemu-devel] [PATCH v2 02/14] sm501: Use defines instead of constants where available

[BALATON Zoltan]

This also fixes the initial value of misc_control register to match the
comment which is likely what was intended but the DAC_POWER bit was set
instead. This value is unused so it does not really matter but is
fixed here for consistency.

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c  | 8 
 hw/display/sm501_template.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 4f40dee..4eb085c 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -555,7 +555,7 @@ static uint32_t get_local_mem_size_index(uint32_t size)
 static inline int is_hwc_enabled(SM501State *state, int crt)
 {
 uint32_t addr = crt ? state->dc_crt_hwc_addr : state->dc_panel_hwc_addr;
-return addr & 0x8000;
+return addr & SM501_HWC_EN;
 }
 
 /**
@@ -1411,9 +1411,9 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t 
base,
 s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
 SM501_DPRINTF("local mem size=%x. index=%d\n", get_local_mem_size(s),
   s->local_mem_size_index);
-s->system_control = 0x0010;
-s->misc_control = 0x1000; /* assumes SH, active=low */
-s->dc_panel_control = 0x0001;
+s->system_control = 0x0010; /* 2D engine FIFO empty */
+s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
+s->dc_panel_control = 0x0001; /* FIFO level 3 */
 s->dc_crt_control = 0x0001;
 
 /* allocate local memory */
diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index aeeac5d..16e500b 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -108,7 +108,7 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, 
int crt,
 /* get hardware cursor pattern */
 uint32_t cursor_addr = get_hwc_address(s, crt);
 assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
-cursor_addr += 64 * c_y / 4;  /* 4 pixels per byte */
+cursor_addr += SM501_HWC_WIDTH * c_y / 4;  /* 4 pixels per byte */
 cursor_addr += s->base;
 
 /* get cursor position */
-- 
2.7.4

[Qemu-devel] [PATCH v2 04/14] sm501: Get rid of base address in draw_hwc_line

[BALATON Zoltan]

Do not use the base address to access data in local memory. This is in
preparation to allow chip connected via PCI where base address depends
on where the BAR is mapped so it will be unknown.

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c  | 6 ++
 hw/display/sm501_template.h | 8 
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 22cac97..1cda127 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -460,7 +460,6 @@ typedef struct SM501State {
 QemuConsole *con;
 
 /* status & internal resources */
-hwaddr base;
 uint32_t local_mem_size_index;
 uint8_t *local_mem;
 MemoryRegion local_mem_region;
@@ -1400,10 +1399,9 @@ static const GraphicHwOps sm501_ops = {
 .gfx_update  = sm501_update_display,
 };
 
-static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base,
+static void sm501_init(SM501State *s, DeviceState *dev,
uint32_t local_mem_bytes)
 {
-s->base = base;
 s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
 SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s),
   s->local_mem_size_index);
@@ -1461,7 +1459,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error 
**errp)
 SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 DeviceState *usb_dev;
 
-sm501_init(>state, dev, s->base, s->vram_size);
+sm501_init(>state, dev, s->vram_size);
 sysbus_init_mmio(sbd, >state.local_mem_region);
 sysbus_init_mmio(sbd, >state.mmio_region);
 
diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index 16e500b..832ee61 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -103,13 +103,13 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State 
*s, int crt,
  uint8_t *palette, int c_y, uint8_t *d, int width)
 {
 int x, i;
-uint8_t bitset = 0;
+uint8_t *pixval, bitset = 0;
 
 /* get hardware cursor pattern */
 uint32_t cursor_addr = get_hwc_address(s, crt);
 assert(0 <= c_y && c_y < SM501_HWC_HEIGHT);
 cursor_addr += SM501_HWC_WIDTH * c_y / 4;  /* 4 pixels per byte */
-cursor_addr += s->base;
+pixval = s->local_mem + cursor_addr;
 
 /* get cursor position */
 x = get_hwc_x(s, crt);
@@ -120,8 +120,8 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, 
int crt,
 
 /* get pixel value */
 if (i % 4 == 0) {
-bitset = ldub_phys(_space_memory, cursor_addr);
-cursor_addr++;
+bitset = ldub_p(pixval);
+pixval++;
 }
 v = bitset & 3;
 bitset >>= 2;
-- 
2.7.4

[Qemu-devel] [PATCH v2 13/14] sm501: Add reset function and vmstate descriptor

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c | 110 ++---
 1 file changed, 105 insertions(+), 5 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 32223f6..b682a95 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -65,6 +65,7 @@
 
 #define MMIO_BASE_OFFSET 0x3e0
 #define MMIO_SIZE 0x20
+#define DC_PALETTE_ENTRIES (0x400 * 3)
 
 /* SM501 register definitions taken from "linux/include/linux/sm501-regs.h" */
 
@@ -491,7 +492,7 @@ typedef struct SM501State {
 uint32_t uart0_mcr;
 uint32_t uart0_scr;
 
-uint8_t dc_palette[0x400 * 3];
+uint8_t dc_palette[DC_PALETTE_ENTRIES];
 
 uint32_t dc_panel_control;
 uint32_t dc_panel_panning_control;
@@ -1537,16 +1538,32 @@ static const GraphicHwOps sm501_ops = {
 .gfx_update  = sm501_update_display,
 };
 
+static void sm501_reset(void *p)
+{
+SM501State *s = p;
+
+s->system_control = 0x0010; /* 2D engine FIFO empty */
+s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
+s->gpio_31_0_control = 0;
+s->gpio_63_32_control = 0;
+s->dram_control = 0;
+s->arbitration_control = 0x05146732;
+s->irq_mask = 0;
+s->misc_timing = 0;
+s->power_mode_control = 0;
+s->dc_panel_control = 0x0001; /* FIFO level 3 */
+s->dc_video_control = 0;
+s->dc_crt_control = 0x0001;
+s->twoD_control = 0;
+}
+
 static void sm501_init(SM501State *s, DeviceState *dev,
uint32_t local_mem_bytes)
 {
 s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
 SM501_DPRINTF("sm501 local mem size=%x. index=%d\n", get_local_mem_size(s),
   s->local_mem_size_index);
-s->system_control = 0x0010; /* 2D engine FIFO empty */
-s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
-s->dc_panel_control = 0x0001; /* FIFO level 3 */
-s->dc_crt_control = 0x0001;
+qemu_register_reset(sm501_reset, s);
 
 /* local memory */
 memory_region_init_ram(>local_mem_region, OBJECT(dev), "sm501.local",
@@ -1577,6 +1594,77 @@ static void sm501_init(SM501State *s, DeviceState *dev,
 s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
 }
 
+static const VMStateDescription vmstate_sm501_regs = {
+.name = "sm501-regs",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(system_control, SM501State),
+VMSTATE_UINT32(misc_control, SM501State),
+VMSTATE_UINT32(gpio_31_0_control, SM501State),
+VMSTATE_UINT32(gpio_63_32_control, SM501State),
+VMSTATE_UINT32(dram_control, SM501State),
+VMSTATE_UINT32(arbitration_control, SM501State),
+VMSTATE_UINT32(irq_mask, SM501State),
+VMSTATE_UINT32(misc_timing, SM501State),
+VMSTATE_UINT32(power_mode_control, SM501State),
+VMSTATE_UINT32(uart0_ier, SM501State),
+VMSTATE_UINT32(uart0_lcr, SM501State),
+VMSTATE_UINT32(uart0_mcr, SM501State),
+VMSTATE_UINT32(uart0_scr, SM501State),
+VMSTATE_UINT8_ARRAY(dc_palette, SM501State, DC_PALETTE_ENTRIES),
+VMSTATE_UINT32(dc_panel_control, SM501State),
+VMSTATE_UINT32(dc_panel_panning_control, SM501State),
+VMSTATE_UINT32(dc_panel_fb_addr, SM501State),
+VMSTATE_UINT32(dc_panel_fb_offset, SM501State),
+VMSTATE_UINT32(dc_panel_fb_width, SM501State),
+VMSTATE_UINT32(dc_panel_fb_height, SM501State),
+VMSTATE_UINT32(dc_panel_tl_location, SM501State),
+VMSTATE_UINT32(dc_panel_br_location, SM501State),
+VMSTATE_UINT32(dc_panel_h_total, SM501State),
+VMSTATE_UINT32(dc_panel_h_sync, SM501State),
+VMSTATE_UINT32(dc_panel_v_total, SM501State),
+VMSTATE_UINT32(dc_panel_v_sync, SM501State),
+VMSTATE_UINT32(dc_panel_hwc_addr, SM501State),
+VMSTATE_UINT32(dc_panel_hwc_location, SM501State),
+VMSTATE_UINT32(dc_panel_hwc_color_1_2, SM501State),
+VMSTATE_UINT32(dc_panel_hwc_color_3, SM501State),
+VMSTATE_UINT32(dc_video_control, SM501State),
+VMSTATE_UINT32(dc_crt_control, SM501State),
+VMSTATE_UINT32(dc_crt_fb_addr, SM501State),
+VMSTATE_UINT32(dc_crt_fb_offset, SM501State),
+VMSTATE_UINT32(dc_crt_h_total, SM501State),
+VMSTATE_UINT32(dc_crt_h_sync, SM501State),
+VMSTATE_UINT32(dc_crt_v_total, SM501State),
+VMSTATE_UINT32(dc_crt_v_sync, SM501State),
+VMSTATE_UINT32(dc_crt_hwc_addr, SM501State),
+VMSTATE_UINT32(dc_crt_hwc_location, SM501State),
+VMSTATE_UINT32(dc_crt_hwc_color_1_2, SM501State),
+VMSTATE_UINT32(dc_crt_hwc_color_3, SM501State),
+VMSTATE_UINT32(twoD_source, SM501State),
+VMSTATE_UINT32(twoD_destination, SM501State),
+VMSTATE_UINT32(twoD_dimension, SM501State),
+VMSTATE_UINT32(twoD_control, SM501State),
+

[Qemu-devel] [PATCH v2 14/14] ppc: Add SM501 device in config for ppc and ppcemb targets

[BALATON Zoltan]

This is not used by default on any emulated machine yet but it is
still useful to have it compiled so it can be added from the command
line for clients that can use it (e.g. MorphOS has no driver for any
other emulated video cards but can output via SM501)

Signed-off-by: BALATON Zoltan 
---
 default-configs/ppc-softmmu.mak| 1 +
 default-configs/ppcemb-softmmu.mak | 1 +
 2 files changed, 2 insertions(+)

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 09c1d45..1f1cd85 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -45,6 +45,7 @@ CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 CONFIG_PLATFORM_BUS=y
 CONFIG_ETSEC=y
 CONFIG_LIBDECNUMBER=y
+CONFIG_SM501=y
 # For PReP
 CONFIG_SERIAL_ISA=y
 CONFIG_MC146818RTC=y
diff --git a/default-configs/ppcemb-softmmu.mak 
b/default-configs/ppcemb-softmmu.mak
index 7f56004..94340de 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -15,3 +15,4 @@ CONFIG_I8259=y
 CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_LIBDECNUMBER=y
+CONFIG_SM501=y
-- 
2.7.4

[Qemu-devel] [PATCH v2 10/14] sm501: Add support for panel layer

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---

v2: Split off renaming a variable to separate clean up patch

 hw/display/sm501.c | 63 +++---
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 9c5ded3..caa7e5c 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -2,6 +2,7 @@
  * QEMU SM501 Device
  *
  * Copyright (c) 2008 Shin-ichiro KAWASAKI
+ * Copyright (c) 2016 BALATON Zoltan
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to 
deal
@@ -41,8 +42,11 @@
  *   - Minimum implementation for Linux console : mmio regs and CRT layer.
  *   - 2D graphics acceleration partially supported : only fill rectangle.
  *
- * TODO:
+ * Status: 2016/12/04
+ *   - Misc fixes: endianness, hardware cursor
  *   - Panel support
+ *
+ * TODO:
  *   - Touch panel support
  *   - USB support
  *   - UART support
@@ -1300,18 +1304,16 @@ static inline int get_depth_index(DisplaySurface 
*surface)
 }
 }
 
-static void sm501_draw_crt(SM501State *s)
+static void sm501_update_display(void *opaque)
 {
+SM501State *s = (SM501State *)opaque;
 DisplaySurface *surface = qemu_console_surface(s->con);
 int y, c_x, c_y;
-uint8_t *hwc_src, *src = s->local_mem;
-int width = get_width(s, 1);
-int height = get_height(s, 1);
-int src_bpp = get_bpp(s, 1);
+int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
+int width = get_width(s, crt);
+int height = get_height(s, crt);
+int src_bpp = get_bpp(s, crt);
 int dst_bpp = surface_bytes_per_pixel(surface);
-uint32_t *palette = (uint32_t *)>dc_palette[SM501_DC_CRT_PALETTE -
-   SM501_DC_PANEL_PALETTE];
-uint8_t hwc_palette[3 * 3];
 int dst_depth_index = get_depth_index(surface);
 draw_line_func *draw_line = NULL;
 draw_hwc_line_func *draw_hwc_line = NULL;
@@ -1319,7 +1321,19 @@ static void sm501_draw_crt(SM501State *s)
 int y_start = -1;
 ram_addr_t page_min = ~0l;
 ram_addr_t page_max = 0l;
-ram_addr_t offset = 0;
+ram_addr_t offset;
+uint32_t *palette;
+uint8_t hwc_palette[3 * 3];
+uint8_t *hwc_src;
+
+if (!((crt ? s->dc_crt_control : s->dc_panel_control)
+  & SM501_DC_CRT_CONTROL_ENABLE)) {
+return;
+}
+
+palette = (uint32_t *)(crt ? >dc_palette[SM501_DC_CRT_PALETTE -
+SM501_DC_PANEL_PALETTE]
+   : >dc_palette[0]);
 
 /* choose draw_line function */
 switch (src_bpp) {
@@ -1333,20 +1347,19 @@ static void sm501_draw_crt(SM501State *s)
 draw_line = draw_line32_funcs[dst_depth_index];
 break;
 default:
-printf("sm501 draw crt : invalid DC_CRT_CONTROL=%x.\n",
-   s->dc_crt_control);
+printf("sm501 update display : invalid control register value.\n");
 abort();
 break;
 }
 
 /* set up to draw hardware cursor */
-if (is_hwc_enabled(s, 1)) {
+if (is_hwc_enabled(s, crt)) {
 /* choose cursor draw line function */
 draw_hwc_line = draw_hwc_line_funcs[dst_depth_index];
-hwc_src = get_hwc_address(s, 1);
-c_x = get_hwc_x(s, 1);
-c_y = get_hwc_y(s, 1);
-get_hwc_palette(s, 1, hwc_palette);
+hwc_src = get_hwc_address(s, crt);
+c_x = get_hwc_x(s, crt);
+c_y = get_hwc_y(s, crt);
+get_hwc_palette(s, crt, hwc_palette);
 }
 
 /* adjust console size */
@@ -1360,7 +1373,7 @@ static void sm501_draw_crt(SM501State *s)
 
 /* draw each line according to conditions */
 memory_region_sync_dirty_bitmap(>local_mem_region);
-for (y = 0; y < height; y++) {
+for (y = 0, offset = 0; y < height; y++, offset += width * src_bpp) {
 int update, update_hwc;
 ram_addr_t page0 = offset;
 ram_addr_t page1 = offset + width * src_bpp - 1;
@@ -1378,7 +1391,7 @@ static void sm501_draw_crt(SM501State *s)
 d +=  y * width * dst_bpp;
 
 /* draw graphics layer */
-draw_line(d, src, width, palette);
+draw_line(d, s->local_mem + offset, width, palette);
 
 /* draw hardware cursor */
 if (update_hwc) {
@@ -1401,9 +1414,6 @@ static void sm501_draw_crt(SM501State *s)
 y_start = -1;
 }
 }
-
-src += width * src_bpp;
-offset += width * src_bpp;
 }
 
 /* complete flush to display */
@@ -1419,15 +1429,6 @@ static void sm501_draw_crt(SM501State *s)
 }
 }
 
-static void sm501_update_display(void *opaque)
-{
-SM501State *s = (SM501State *)opaque;
-
-if (s->dc_crt_control & SM501_DC_CRT_CONTROL_ENABLE) {
-sm501_draw_crt(s);
-}
-}
-
 static const GraphicHwOps sm501_ops = {
 .gfx_update  = 

[Qemu-devel] [PATCH v2 12/14] sm501: Implement reading 2D engine registers

[BALATON Zoltan]

Clients normally only write to these registers, nothing is known to
ever read them but they are documented as read/write so allow clients
to also read the values.

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c | 57 ++
 1 file changed, 57 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index af5e4db..32223f6 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1149,9 +1149,66 @@ static uint64_t sm501_2d_engine_read(void *opaque, 
hwaddr addr,
 SM501_DPRINTF("sm501 2d engine regs : read addr=%x\n", (int)addr);
 
 switch (addr) {
+case SM501_2D_SOURCE:
+ret = s->twoD_source;
+break;
+case SM501_2D_DESTINATION:
+ret = s->twoD_destination;
+break;
+case SM501_2D_DIMENSION:
+ret = s->twoD_dimension;
+break;
+case SM501_2D_CONTROL:
+ret = s->twoD_control;
+break;
+case SM501_2D_PITCH:
+ret = s->twoD_pitch;
+break;
+case SM501_2D_FOREGROUND:
+ret = s->twoD_foreground;
+break;
+case SM501_2D_BACKGROUND:
+ret = s->twoD_background;
+break;
+case SM501_2D_STRETCH:
+ret = s->twoD_stretch;
+break;
+case SM501_2D_COLOR_COMPARE:
+ret = s->twoD_color_compare;
+break;
+case SM501_2D_COLOR_COMPARE_MASK:
+ret = s->twoD_color_compare_mask;
+break;
+case SM501_2D_MASK:
+ret = s->twoD_mask;
+break;
+case SM501_2D_CLIP_TL:
+ret = s->twoD_clip_tl;
+break;
+case SM501_2D_CLIP_BR:
+ret = s->twoD_clip_br;
+break;
+case SM501_2D_MONO_PATTERN_LOW:
+ret = s->twoD_mono_pattern_low;
+break;
+case SM501_2D_MONO_PATTERN_HIGH:
+ret = s->twoD_mono_pattern_high;
+break;
+case SM501_2D_WINDOW_WIDTH:
+ret = s->twoD_window_width;
+break;
 case SM501_2D_SOURCE_BASE:
 ret = s->twoD_source_base;
 break;
+case SM501_2D_DESTINATION_BASE:
+ret = s->twoD_destination_base;
+break;
+case SM501_2D_ALPHA:
+ret = s->twoD_alpha;
+break;
+case SM501_2D_WRAP:
+ret = s->twoD_wrap;
+break;
 case SM501_2D_STATUS:
 ret = 0; /* Should return interrupt status */
 break;
-- 
2.7.4

[Qemu-devel] [PATCH v2 07/14] sm501: Fix device endianness

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---

v2: Split off small clean up to other patch

 hw/display/sm501.c  |  6 +++---
 hw/display/sm501_template.h | 23 ++-
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index b977094..2694081 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -849,7 +849,7 @@ static const MemoryRegionOps sm501_system_config_ops = {
 .min_access_size = 4,
 .max_access_size = 4,
 },
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
@@ -1085,7 +1085,7 @@ static const MemoryRegionOps sm501_disp_ctrl_ops = {
 .min_access_size = 4,
 .max_access_size = 4,
 },
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
@@ -1173,7 +1173,7 @@ static const MemoryRegionOps sm501_2d_engine_ops = {
 .min_access_size = 4,
 .max_access_size = 4,
 },
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 /* draw line functions for all console modes */
diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index 832ee61..6e56baf 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -64,10 +64,16 @@ static void glue(draw_line16_, PIXEL_NAME)(
 uint8_t r, g, b;
 
 do {
-rgb565 = lduw_p(s);
-r = ((rgb565 >> 11) & 0x1f) << 3;
-g = ((rgb565 >>  5) & 0x3f) << 2;
-b = ((rgb565 >>  0) & 0x1f) << 3;
+rgb565 = lduw_le_p(s);
+#if defined(TARGET_WORDS_BIGENDIAN)
+r = (rgb565 >> 8) & 0xf8;
+g = (rgb565 >> 3) & 0xfc;
+b = (rgb565 << 3) & 0xf8;
+#else
+b = (rgb565 >> 8) & 0xf8;
+g = (rgb565 >> 3) & 0xfc;
+r = (rgb565 << 3) & 0xf8;
+#endif
 *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
 s += 2;
 d += BPP;
@@ -80,15 +86,14 @@ static void glue(draw_line32_, PIXEL_NAME)(
 uint8_t r, g, b;
 
 do {
-ldub_p(s);
 #if defined(TARGET_WORDS_BIGENDIAN)
+r = s[0];
+g = s[1];
+b = s[2];
+#else
 r = s[1];
 g = s[2];
 b = s[3];
-#else
-b = s[0];
-g = s[1];
-r = s[2];
 #endif
 *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
 s += 4;
-- 
2.7.4

[Qemu-devel] [PATCH v2 00/14] Improvements for SM501 display controller emulation

[BALATON Zoltan]

Second try, addressing review comments.

BALATON Zoltan (14):
  sm501: Fixed code style and a few typos in comments
  sm501: Use defines instead of constants where available
  sm501: QOMify
  sm501: Get rid of base address in draw_hwc_line
  sm501: Add emulation of chip connected via PCI
  sm501: Add missing arbitration control register
  sm501: Fix device endianness
  sm501: Fix hardware cursor
  sm501: Misc clean ups
  sm501: Add support for panel layer
  sm501: Add some more missing registers
  sm501: Implement reading 2D engine registers
  sm501: Add reset function and vmstate descriptor
  ppc: Add SM501 device in config for ppc and ppcemb targets

 default-configs/ppc-softmmu.mak|1 +
 default-configs/ppcemb-softmmu.mak |1 +
 hw/display/sm501.c | 1718 ++--
 hw/display/sm501_template.h|   94 +-
 hw/sh4/r2d.c   |   11 +-
 include/hw/devices.h   |5 -
 6 files changed, 1089 insertions(+), 741 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 06/14] sm501: Add missing arbitration control register

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---
 hw/display/sm501.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index d9219bd..b977094 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -477,6 +477,7 @@ typedef struct SM501State {
 uint32_t gpio_31_0_control;
 uint32_t gpio_63_32_control;
 uint32_t dram_control;
+uint32_t arbitration_control;
 uint32_t irq_mask;
 uint32_t misc_timing;
 uint32_t power_mode_control;
@@ -760,6 +761,9 @@ static uint64_t sm501_system_config_read(void *opaque, 
hwaddr addr,
 case SM501_DRAM_CONTROL:
 ret = (s->dram_control & 0x07F107C0) | s->local_mem_size_index << 13;
 break;
+case SM501_ARBTRTN_CONTROL:
+ret = s->arbitration_control;
+break;
 case SM501_IRQ_MASK:
 ret = s->irq_mask;
 break;
@@ -812,6 +816,9 @@ static void sm501_system_config_write(void *opaque, hwaddr 
addr,
 /* TODO : check validity of size change */
 s->dram_control |=  value & 0x7FC3;
 break;
+case SM501_ARBTRTN_CONTROL:
+s->arbitration_control =  value & 0x3777;
+break;
 case SM501_IRQ_MASK:
 s->irq_mask = value;
 break;
-- 
2.7.4

[Qemu-devel] [PATCH v3] target-s390x: Implement stfl and stfle

[Michal Marek]

The implementation is partially cargo cult based, but it works for the
linux kernel use case.

Signed-off-by: Michal Marek 
---
v3:
 - Initialize the buffer in do_stfle()
v2:
 - STFLE is not a privileged instruction, go through the MMU to store the
   result
 - annotate the stfl helper with TCG_CALL_NO_RWG
 - Use a large enough buffer to hold the feature bitmap
 - Fix coding style of the stfle helper
---
 target/s390x/cpu_features.c |  6 --
 target/s390x/cpu_features.h |  2 +-
 target/s390x/helper.h   |  2 ++
 target/s390x/insn-data.def  |  2 ++
 target/s390x/misc_helper.c  | 36 
 target/s390x/translate.c| 17 +
 6 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 42fd9d792bc8..d77c560380c4 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -286,11 +286,11 @@ void s390_init_feat_bitmap(const S390FeatInit init, 
S390FeatBitmap bitmap)
 }
 }
 
-void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
+int s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
   uint8_t *data)
 {
 S390Feat feat;
-int bit_nr;
+int bit_nr, res = 0;
 
 if (type == S390_FEAT_TYPE_STFL && test_bit(S390_FEAT_ZARCH, features)) {
 /* z/Architecture is always active if around */
@@ -303,9 +303,11 @@ void s390_fill_feat_block(const S390FeatBitmap features, 
S390FeatType type,
 bit_nr = s390_features[feat].bit;
 /* big endian on uint8_t array */
 data[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
+res = MAX(res, bit_nr / 8 + 1);
 }
 feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
 }
+return res;
 }
 
 void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index d66912178680..e3c41be08060 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -56,7 +56,7 @@ typedef uint64_t S390FeatInit[S390_FEAT_MAX / 64 + 1];
 const S390FeatDef *s390_feat_def(S390Feat feat);
 S390Feat s390_feat_by_type_and_bit(S390FeatType type, int bit);
 void s390_init_feat_bitmap(const S390FeatInit init, S390FeatBitmap bitmap);
-void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
+int s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
   uint8_t *data);
 void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
   uint8_t *data);
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 9102071d0aa4..fa4a617cd1ab 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -95,6 +95,8 @@ DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)
 DEF_HELPER_FLAGS_2(spt, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
 DEF_HELPER_4(stsi, i32, env, i64, i64, i64)
+DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
+DEF_HELPER_3(stfle, i64, env, i64, i64)
 DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 075ff597c3de..be830a42ed8d 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -899,6 +899,8 @@
 C(0xb202, STIDP,   S, Z,   la2, 0, new, m1_64, stidp, 0)
 /* STORE CPU TIMER */
 C(0xb209, STPT,S, Z,   la2, 0, new, m1_64, stpt, 0)
+/* STORE FACILITY LIST EXTENDED */
+C(0xb2b0, STFLE,   S,  SFLE,   0, a2, 0, 0, stfle, 0)
 /* STORE FACILITY LIST */
 C(0xb2b1, STFL,S, Z,   0, 0, 0, 0, stfl, 0)
 /* STORE PREFIX */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index c9604ea9c728..b51454ea7861 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -500,6 +500,42 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
 return cc;
 }
 
+static int do_stfle(CPUS390XState *env, uint64_t addr, int len)
+{
+S390CPU *cpu = s390_env_get_cpu(env);
+/* 256 doublewords as per STFLE documentation */
+uint8_t data[256 * 8] = { 0 };
+int i, res;
+
+memset(data, 0, sizeof(data));
+res = s390_fill_feat_block(cpu->model->features, S390_FEAT_TYPE_STFL, 
data);
+for (i = 0; i < MIN(res, len); i++) {
+cpu_stb_data(env, addr + i, data[i]);
+}
+
+return res;
+}
+
+uint64_t HELPER(stfle)(CPUS390XState *env, uint64_t a0, uint64_t r0)
+{
+int need, len = r0 & 0xff;
+
+need = do_stfle(env, a0, len * 8);
+need = DIV_ROUND_UP(need, 8);
+if (need <= len) {
+env->cc_op = 0;
+} else {
+env->cc_op = 3;
+}
+
+return (r0 & ~0xffLL) | (need - 1);
+}
+
+void HELPER(stfl)(CPUS390XState *env)

Re: [Qemu-devel] [PATCH] target-s390x: Implement stfl and stfle

[Michal Marek]

Dne 25.2.2017 v 21:39 Michal Marek napsal(a):
> Dne 25.2.2017 v 01:05 Richard Henderson napsal(a):
>> On 02/25/2017 12:44 AM, Michal Marek wrote:
>>> +static int do_stfle(CPUS390XState *env, uint64_t addr, int len)
>>> +{
>>> +S390CPU *cpu = s390_env_get_cpu(env);
>>> +uint8_t data[64];
>>
>> S390FeatBitmap or S390FeatInit?  Or even a sizeof?
>> Hard coding 64 certainly doesn't seem right.
> 
> I will change it to something more sensible.

I changed it to 2k, which is the maximum that the STFLE instruction can
store. I did not use S390FeatBitmap or S390FeatInit, as the
qemu-internal feature bit assignments differ from the PoO facility bit
assignments.

Michal

[Qemu-devel] [PATCH v2] target-s390x: Implement stfl and stfle

[Michal Marek]

The implementation is partially cargo cult based, but it works for the
linux kernel use case.

Signed-off-by: Michal Marek 
---
v2:
 - STFLE is not a privileged instruction, go through the MMU to store the
   result
 - annotate the stfl helper with TCG_CALL_NO_RWG
 - Use a large enough buffer to hold the feature bitmap
 - Fix coding style of the stfle helper
---
 target/s390x/cpu_features.c |  6 --
 target/s390x/cpu_features.h |  2 +-
 target/s390x/helper.h   |  2 ++
 target/s390x/insn-data.def  |  2 ++
 target/s390x/misc_helper.c  | 35 +++
 target/s390x/translate.c| 17 +
 6 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 42fd9d792bc8..d77c560380c4 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -286,11 +286,11 @@ void s390_init_feat_bitmap(const S390FeatInit init, 
S390FeatBitmap bitmap)
 }
 }
 
-void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
+int s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
   uint8_t *data)
 {
 S390Feat feat;
-int bit_nr;
+int bit_nr, res = 0;
 
 if (type == S390_FEAT_TYPE_STFL && test_bit(S390_FEAT_ZARCH, features)) {
 /* z/Architecture is always active if around */
@@ -303,9 +303,11 @@ void s390_fill_feat_block(const S390FeatBitmap features, 
S390FeatType type,
 bit_nr = s390_features[feat].bit;
 /* big endian on uint8_t array */
 data[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
+res = MAX(res, bit_nr / 8 + 1);
 }
 feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
 }
+return res;
 }
 
 void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index d66912178680..e3c41be08060 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -56,7 +56,7 @@ typedef uint64_t S390FeatInit[S390_FEAT_MAX / 64 + 1];
 const S390FeatDef *s390_feat_def(S390Feat feat);
 S390Feat s390_feat_by_type_and_bit(S390FeatType type, int bit);
 void s390_init_feat_bitmap(const S390FeatInit init, S390FeatBitmap bitmap);
-void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
+int s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
   uint8_t *data);
 void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
   uint8_t *data);
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 9102071d0aa4..fa4a617cd1ab 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -95,6 +95,8 @@ DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)
 DEF_HELPER_FLAGS_2(spt, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
 DEF_HELPER_4(stsi, i32, env, i64, i64, i64)
+DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
+DEF_HELPER_3(stfle, i64, env, i64, i64)
 DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 075ff597c3de..be830a42ed8d 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -899,6 +899,8 @@
 C(0xb202, STIDP,   S, Z,   la2, 0, new, m1_64, stidp, 0)
 /* STORE CPU TIMER */
 C(0xb209, STPT,S, Z,   la2, 0, new, m1_64, stpt, 0)
+/* STORE FACILITY LIST EXTENDED */
+C(0xb2b0, STFLE,   S,  SFLE,   0, a2, 0, 0, stfle, 0)
 /* STORE FACILITY LIST */
 C(0xb2b1, STFL,S, Z,   0, 0, 0, 0, stfl, 0)
 /* STORE PREFIX */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index c9604ea9c728..3baee27d40ce 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -500,6 +500,41 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
 return cc;
 }
 
+static int do_stfle(CPUS390XState *env, uint64_t addr, int len)
+{
+S390CPU *cpu = s390_env_get_cpu(env);
+uint8_t data[256 * 8]; /* 256 doublewords as per STFLE documentation */
+int i, res;
+
+memset(data, 0, sizeof(data));
+res = s390_fill_feat_block(cpu->model->features, S390_FEAT_TYPE_STFL, 
data);
+for (i = 0; i < MIN(res, len); i++) {
+cpu_stb_data(env, addr + i, data[i]);
+}
+
+return res;
+}
+
+uint64_t HELPER(stfle)(CPUS390XState *env, uint64_t a0, uint64_t r0)
+{
+int need, len = r0 & 0xff;
+
+need = do_stfle(env, a0, len * 8);
+need = DIV_ROUND_UP(need, 8);
+if (need <= len) {
+env->cc_op = 0;
+} else {
+env->cc_op = 3;
+}
+
+return (r0 & ~0xffLL) | (need - 1);
+}
+
+void HELPER(stfl)(CPUS390XState *env)
+{
+do_stfle(env, 200, 4);
+}
+
 uint32_t 

[Qemu-devel] [PATCH v2] linux-user: Add signal handling support for x86_64

[Pranith Kumar]

Note that x86_64 has only _rt signal handlers. This implementation
attempts to share code with the x86_32 implementation.

CC: Laurent Vivier 
Signed-off-by: Allan Wirth 
Reviewed-by: Peter Maydell  
Signed-off-by: Pranith Kumar 
---
v2: Update with review comments from Laurent Vivier
---
 linux-user/signal.c  | 280 ++-
 target/i386/cpu.h|   2 +
 target/i386/fpu_helper.c |  12 ++
 3 files changed, 243 insertions(+), 51 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 8209539555..c8eb854b29 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -254,7 +254,7 @@ int do_sigprocmask(int how, const sigset_t *set, sigset_t 
*oldset)
 }
 
 #if !defined(TARGET_OPENRISC) && !defined(TARGET_UNICORE32) && \
-!defined(TARGET_X86_64) && !defined(TARGET_NIOS2)
+!defined(TARGET_NIOS2)
 /* Just set the guest's signal mask to the specified value; the
  * caller is assumed to have called block_signals() already.
  */
@@ -512,7 +512,7 @@ void signal_init(void)
 }
 }
 
-#if !(defined(TARGET_X86_64) || defined(TARGET_UNICORE32))
+#ifndef TARGET_UNICORE32
 /* Force a synchronously taken signal. The kernel force_sig() function
  * also forces the signal to "not blocked, not ignored", but for QEMU
  * that work is done in process_pending_signals().
@@ -819,9 +819,8 @@ int do_sigaction(int sig, const struct target_sigaction 
*act,
 return ret;
 }
 
-#if defined(TARGET_I386) && TARGET_ABI_BITS == 32
-
-/* from the Linux kernel */
+#if defined(TARGET_I386)
+/* from the Linux kernel - /arch/x86/include/uapi/asm/sigcontext.h */
 
 struct target_fpreg {
 uint16_t significand[4];
@@ -835,58 +834,120 @@ struct target_fpxreg {
 };
 
 struct target_xmmreg {
-abi_ulong element[4];
+uint32_t element[4];
 };
 
-struct target_fpstate {
+struct target_fpstate_32 {
 /* Regular FPU environment */
-abi_ulong cw;
-abi_ulong sw;
-abi_ulong tag;
-abi_ulong ipoff;
-abi_ulong cssel;
-abi_ulong dataoff;
-abi_ulong datasel;
-struct target_fpreg _st[8];
+uint32_t cw;
+uint32_t sw;
+uint32_t tag;
+uint32_t ipoff;
+uint32_t cssel;
+uint32_t dataoff;
+uint32_t datasel;
+struct target_fpreg st[8];
 uint16_t  status;
 uint16_t  magic;  /* 0x = regular FPU data only */
 
 /* FXSR FPU environment */
-abi_ulong _fxsr_env[6];   /* FXSR FPU env is ignored */
-abi_ulong mxcsr;
-abi_ulong reserved;
-struct target_fpxreg _fxsr_st[8]; /* FXSR FPU reg data is ignored */
-struct target_xmmreg _xmm[8];
-abi_ulong padding[56];
+uint32_t _fxsr_env[6];   /* FXSR FPU env is ignored */
+uint32_t mxcsr;
+uint32_t reserved;
+struct target_fpxreg fxsr_st[8]; /* FXSR FPU reg data is ignored */
+struct target_xmmreg xmm[8];
+uint32_t padding[56];
 };
 
-#define X86_FXSR_MAGIC 0x
+struct target_fpstate_64 {
+/* FXSAVE format */
+uint16_t cw;
+uint16_t sw;
+uint16_t twd;
+uint16_t fop;
+uint64_t rip;
+uint64_t rdp;
+uint32_t mxcsr;
+uint32_t mxcsr_mask;
+uint32_t st_space[32];
+uint32_t xmm_space[64];
+uint32_t reserved[24];
+};
 
-struct target_sigcontext {
+#ifndef TARGET_X86_64
+# define target_fpstate target_fpstate_32
+#else
+# define target_fpstate target_fpstate_64
+#endif
+
+struct target_sigcontext_32 {
 uint16_t gs, __gsh;
 uint16_t fs, __fsh;
 uint16_t es, __esh;
 uint16_t ds, __dsh;
-abi_ulong edi;
-abi_ulong esi;
-abi_ulong ebp;
-abi_ulong esp;
-abi_ulong ebx;
-abi_ulong edx;
-abi_ulong ecx;
-abi_ulong eax;
-abi_ulong trapno;
-abi_ulong err;
-abi_ulong eip;
+uint32_t edi;
+uint32_t esi;
+uint32_t ebp;
+uint32_t esp;
+uint32_t ebx;
+uint32_t edx;
+uint32_t ecx;
+uint32_t eax;
+uint32_t trapno;
+uint32_t err;
+uint32_t eip;
 uint16_t cs, __csh;
-abi_ulong eflags;
-abi_ulong esp_at_signal;
+uint32_t eflags;
+uint32_t esp_at_signal;
 uint16_t ss, __ssh;
-abi_ulong fpstate; /* pointer */
-abi_ulong oldmask;
-abi_ulong cr2;
+uint32_t fpstate; /* pointer */
+uint32_t oldmask;
+uint32_t cr2;
+};
+
+struct target_sigcontext_64 {
+uint64_t r8;
+uint64_t r9;
+uint64_t r10;
+uint64_t r11;
+uint64_t r12;
+uint64_t r13;
+uint64_t r14;
+uint64_t r15;
+
+uint64_t rdi;
+uint64_t rsi;
+uint64_t rbp;
+uint64_t rbx;
+uint64_t rdx;
+uint64_t rax;
+uint64_t rcx;
+uint64_t rsp;
+uint64_t rip;
+
+uint64_t eflags;
+
+uint16_t cs;
+uint16_t gs;
+uint16_t fs;
+uint16_t ss;
+
+uint64_t err;
+uint64_t trapno;
+uint64_t oldmask;
+uint64_t cr2;
+
+uint64_t fpstate; /* pointer */
+uint64_t padding[8];
 };
 
+#ifndef TARGET_X86_64
+# define 

[Qemu-devel] [PATCH] slirp: allow host port 0 for hostfwd

[Vincent Bernat]

The OS will allocate automatically a free port. This is useful if you
want to be sure to not get any port conflict. You still have to figure
out which port you got, for example with "lsof" (this could be exposed
in the monitor if needed).

Example of use:

 $ qemu-system-x86_64 -net user,hostfwd=127.0.0.1:0-:22 ...

Then, get your port with:

 $ lsof -np 1474 | grep LISTEN
 qemu-syst 31777 bernat 12u IPv4 [...] TCP 127.0.0.1:35145 (LISTEN)

Signed-off-by: Vincent Bernat 
---
 net/slirp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/slirp.c b/net/slirp.c
index f97ec233454e..11b2dd249a79 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -487,7 +487,7 @@ static int slirp_hostfwd(SlirpState *s, const char 
*redir_str,
 goto fail_syntax;
 }
 host_port = strtol(buf, , 0);
-if (*end != '\0' || host_port < 1 || host_port > 65535) {
+if (*end != '\0' || host_port < 0 || host_port > 65535) {
 goto fail_syntax;
 }
 
-- 
2.11.0

Re: [Qemu-devel] [PULL 00/17] KVM and cpu-exec patches for 2.9 soft freeze

[Peter Maydell]

On 24 February 2017 at 17:40, Paolo Bonzini  wrote:
> The following changes since commit a1cf5fac2b929ffa2abd1285401f2535ff8c6fea:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-block-2017-02-21' 
> into staging (2017-02-21 13:58:50 +)
>
> are available in the git repository at:
>
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to f9e640f9a1a7185591985e10a557f11fbe783b87:
>
>   qmp-events: fix GUEST_PANICKED description formatting (2017-02-24 16:19:44 
> +0100)
>
> 
> * kernel header update (requested by David and Vijay)
> * GuestPanicInformation fixups (Anton)
> * record/replay icount fixes (Pavel)
> * cpu-exec cleanup, unification of icount_decr with tcg_exit_req (me)
> * KVM_CAP_IMMEDIATE_EXIT support (me)
> * vmxcap update (me)

The patchew s390 host build failure looks like a genuine
problem, so I'm dropping this from my to-merge queue.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 17/21] qapi: Drop unused non-strict qobject input visitor

[Eric Blake]

On 02/24/2017 09:02 AM, Markus Armbruster wrote:
> Paolo Bonzini  writes:
> 
>> On 23/02/2017 22:45, Markus Armbruster wrote:
>>> The split between tests/test-qobject-input-visitor.c and
>>> tests/test-qobject-input-strict.c now makes less sense than ever.  The
>>> next commit will take care of that.
>>
>> I'm actually adding a use for non-strict visitors (and one that makes
>> sense IMHO, with comments, testcases and all that :)).  See
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg432069.html.
> 
> I replied.

Hmm. Right now, we implement strict checking by populating a hash table
in parallel to the QObject being visited, then checking if it gets
emptied.  I wonder if it would be any simpler to clone the QObject and
then remove keys, rather than tracking a separate hash table.  In fact,
you could then turn strict checking into an after-the-fact item:
creating the visitor passes in a QObject, then after the visit you check
whether the QObject has been emptied.  But that's not something that has
to be solved for 2.9.

> 
>> For what it's worth, however, I believe that even non-strict visits
>> should detect unvisited list tails.
> 
> Bit of an asymmetry.  Not sure it matters, because to not visit list
> tails, you have to put in some effort.
> 
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 00/24] MTTCG Base enabling patches with ARM enablement

[Peter Maydell]

On 24 February 2017 at 11:20, Alex Bennée  wrote:
> The following changes since commit 2d896b454a0e19ec4c1ddbb0e0b65b7e54fcedf3:
>
>   Revert "hw/mips: MIPS Boston board support" (2017-02-23 18:04:45 +)
>
> are available in the git repository at:
>
>   https://github.com/stsquad/qemu.git tags/pull-mttcg-240217-1
>
> for you to fetch changes up to ca759f9e387db87e1719911f019bc60c74be9ed8:
>
>   tcg: enable MTTCG by default for ARM on x86 hosts (2017-02-24 10:32:46 
> +)
>
> 
> This is the MTTCG pull-request as posted yesterday.
>
> 

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH] target-s390x: Implement stfl and stfle

[Michal Marek]

Dne 25.2.2017 v 01:05 Richard Henderson napsal(a):
> On 02/25/2017 12:44 AM, Michal Marek wrote:
>> +DEF_HELPER_1(stfl, void, env)
> 
> DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env)
> 
> since this touches no registers, and only writes to lomem which afaik
> cannot fault in kernel mode.

OK.


>> +static int do_stfle(CPUS390XState *env, uint64_t addr, int len)
>> +{
>> +S390CPU *cpu = s390_env_get_cpu(env);
>> +uint8_t data[64];
> 
> S390FeatBitmap or S390FeatInit?  Or even a sizeof?
> Hard coding 64 certainly doesn't seem right.

I will change it to something more sensible.


>> +memset(data, 0, sizeof(data));
>> +res = s390_fill_feat_block(cpu->model->features,
>> S390_FEAT_TYPE_STFL, data);
>> +cpu_physical_memory_write(addr, data, MIN(res, len));
> 
> No, not physical memory, you need to write to virtual memory, at least
> for STFLE.  Which, as you'll recall can be used from user-mode.

Oh, I did not realize that STFLE is not a privileged instruction.

Thanks for the review!

Michal

[Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Vladimir Sementsov-Ogievskiy]

After migration all drives are inactive and savevm will fail with

qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
   Assertion `!(bs->open_flags & 0x0800)' failed.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/snapshot.c   |  3 ++-
 migration/savevm.c | 11 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index bf5c2ca5e1..256d06ac9f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
 int bdrv_can_snapshot(BlockDriverState *bs)
 {
 BlockDriver *drv = bs->drv;
-if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
+(bs->open_flags & BDRV_O_INACTIVE)) {
 return 0;
 }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 5ecd264134..75e56d2d07 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2068,6 +2068,17 @@ int save_vmstate(Monitor *mon, const char *name)
 Error *local_err = NULL;
 AioContext *aio_context;
 
+if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
+runstate_check(RUN_STATE_POSTMIGRATE) ||
+runstate_check(RUN_STATE_PRELAUNCH))
+{
+bdrv_invalidate_cache_all(_err);
+if (local_err) {
+error_report_err(local_err);
+return -EINVAL;
+}
+}
+
 if (!bdrv_all_can_snapshot()) {
 monitor_printf(mon, "Device '%s' is writable but does not "
"support snapshots.\n", bdrv_get_device_name(bs));
-- 
2.11.1

[Qemu-devel] [PATCH 4/4] migration: fix use-after-free of to_dst_file

[Vladimir Sementsov-Ogievskiy]

hmp_savevm calls qemu_savevm_state(f), which sets to_dst_file=f in
global migration state. Then hmp_savevm closes f (g_free called).

Next access to to_dst_file in migration state (for example,
qmp_migrate_set_speed) will use it after it was freed.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 migration/savevm.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 75e56d2d07..fcb8fd8acd 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1276,6 +1276,11 @@ done:
 status = MIGRATION_STATUS_COMPLETED;
 }
 migrate_set_state(>state, MIGRATION_STATUS_SETUP, status);
+
+/* f is outer parameter, it should not stay in global migration state after
+ * this function finished */
+ms->to_dst_file = NULL;
+
 return ret;
 }
 
-- 
2.11.1

[Qemu-devel] [PATCH 1/4] iotests: add migration corner cases test

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/175 | 71 ++
 tests/qemu-iotests/175.out |  5 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 77 insertions(+)
 create mode 100644 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
new file mode 100644
index 00..ef86c70db5
--- /dev/null
+++ b/tests/qemu-iotests/175
@@ -0,0 +1,71 @@
+#!/usr/bin/env python
+#
+# Test migration corner-cases
+#
+# Copyright (C) Vladimir Sementsov-Ogievskiy 2017
+#
+# 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 .
+#
+
+import os
+import iotests
+import time
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+
+class TestMigrationCornerCases(iotests.QMPTestCase):
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, disk, '10M')
+self.vm = iotests.VM().add_drive(disk)
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+os.remove(disk)
+
+def test_migrate_reset_cont_write(self):
+result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
+self.assert_qmp(result, 'return', {})
+time.sleep(4)
+
+result = self.vm.qmp('human-monitor-command',
+ command_line='system_reset')
+self.assert_qmp(result, 'return', '')
+
+result = self.vm.qmp('cont')
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io drive0 "write 0 512"')
+self.assert_qmp(result, 'return', '')
+
+def test_migrate_savevm(self):
+result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
+self.assert_qmp(result, 'return', {})
+time.sleep(4)
+
+result = self.vm.qmp('human-monitor-command', command_line='savevm')
+self.assert_qmp(result, 'return', '')
+
+def test_savevm_set_speed_savevm(self):
+for i in range(10):
+result = self.vm.qmp('human-monitor-command', 
command_line='savevm')
+self.assert_qmp(result, 'return', '')
+
+result = self.vm.qmp('migrate_set_speed', 
value=9223372036853727232)
+self.assert_qmp(result, 'return', {})
+
+if __name__ == '__main__':
+iotests.main()
diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
new file mode 100644
index 00..8d7e996700
--- /dev/null
+++ b/tests/qemu-iotests/175.out
@@ -0,0 +1,5 @@
+...
+--
+Ran 3 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 985b9a6a36..1f4bf03185 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -167,3 +167,4 @@
 172 auto
 173 rw auto
 174 auto
+175 auto quick
-- 
2.11.1

[Qemu-devel] [PATCH 0/4] some migration bugs

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are some migration related bugs, two about INACTIVE bdses and one
use-after-free.

I'm absolutely not sure, that these bugs should be fixed like I'm fixing,
but problem definitely exists.

Reset in stopped state is strange case, may be such usage should be
restricted.
About INACTIVE - looks like it should be a separate run-state, not only
bdrv-flag.
Situation with migration state, which is global, but is set/reset/changed
in not controlled manner is not very good too..

Vladimir Sementsov-Ogievskiy (4):
  iotests: add migration corner cases test
  qmp-cont: invalidate on RUN_STATE_PRELAUNCH
  savevm: fix savevm after migration
  migration: fix use-after-free of to_dst_file

 block/snapshot.c   |  3 +-
 migration/savevm.c | 16 +++
 qmp.c  |  3 +-
 tests/qemu-iotests/175 | 71 ++
 tests/qemu-iotests/175.out |  5 
 tests/qemu-iotests/group   |  1 +
 6 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

-- 
2.11.1

[Qemu-devel] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Vladimir Sementsov-Ogievskiy]

We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
come to

qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
   Assertion `!(bs->open_flags & 0x0800)' failed.

on the first write after vm start.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qmp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qmp.c b/qmp.c
index dfaabac1a6..e61795d033 100644
--- a/qmp.c
+++ b/qmp.c
@@ -198,7 +198,8 @@ void qmp_cont(Error **errp)
 /* Continuing after completed migration. Images have been inactivated to
  * allow the destination to take control. Need to get control back now. */
 if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
-runstate_check(RUN_STATE_POSTMIGRATE))
+runstate_check(RUN_STATE_POSTMIGRATE) ||
+runstate_check(RUN_STATE_PRELAUNCH))
 {
 bdrv_invalidate_cache_all(_err);
 if (local_err) {
-- 
2.11.1

Re: [Qemu-devel] [PULL 00/13] s390x patches for 2.9

[Peter Maydell]

On 24 February 2017 at 09:22, Cornelia Huck  wrote:
> The following changes since commit 10f25e4844cb9b3f02fb032f88051dd5b65b4206:
>
>   Merge remote-tracking branch 'remotes/yongbok/tags/mips-20170222' into 
> staging (2017-02-23 09:59:40 +)
>
> are available in the git repository at:
>
>   git://github.com/cohuck/qemu tags/s390x-20170224
>
> for you to fetch changes up to 9f94f84ce7df633142953806cc4c102765cabc0e:
>
>   s390x/css: handle format-0 TIC CCW correctly (2017-02-24 10:15:18 +0100)
>
> 
> A selection of s390x patches:
> - cleanups, fixes and improvements
> - program check loop detection (useful with the corresponding kernel
>   patch)
> - wire up virtio-crypto for ccw
> - and finally support many virtqueues for virtio-ccw
>
> 

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH 12/17] migration: add postcopy migration of dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

16.02.2017 16:04, Fam Zheng wrote:

On Mon, 02/13 12:54, Vladimir Sementsov-Ogievskiy wrote:

Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
associated with root nodes and non-root named nodes are migrated.

If destination qemu is already containing a dirty bitmap with the same name
as a migrated bitmap (for the same node), than, if their granularities are


[...]


+
+#define CHUNK_SIZE (1 << 10)
+
+/* Flags occupy from one to four bytes. In all but one the 7-th (EXTRA_FLAGS)
+ * bit should be set. */
+#define DIRTY_BITMAP_MIG_FLAG_EOS   0x01
+#define DIRTY_BITMAP_MIG_FLAG_ZEROES0x02
+#define DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME   0x04
+#define DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME   0x08
+#define DIRTY_BITMAP_MIG_FLAG_START 0x10
+#define DIRTY_BITMAP_MIG_FLAG_COMPLETE  0x20
+#define DIRTY_BITMAP_MIG_FLAG_BITS  0x40
+
+#define DIRTY_BITMAP_MIG_EXTRA_FLAGS0x80
+#define DIRTY_BITMAP_MIG_FLAGS_SIZE_16  0x8000

This flag means two bytes, right? But your above comment says "7-th bit should
be set". This doesn't make sense. Should this be "0x80" too?


Hmm, good caught, you are right. Also, the comment should be fixed so 
that there are may be 1,2 or 4 bytes, and of course EXTRA_FLAGS bit may 
be only in first and second bytes (the code do so).





+#define DIRTY_BITMAP_MIG_FLAGS_SIZE_32  0x8080
+
+#define DEBUG_DIRTY_BITMAP_MIGRATION 0


[...]


+
+static void send_bitmap_bits(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
+ uint64_t start_sector, uint32_t nr_sectors)
+{
+/* align for buffer_is_zero() */
+uint64_t align = 4 * sizeof(long);
+uint64_t unaligned_size =
+bdrv_dirty_bitmap_serialization_size(dbms->bitmap,
+ start_sector, nr_sectors);
+uint64_t buf_size = (unaligned_size + align - 1) & ~(align - 1);
+uint8_t *buf = g_malloc0(buf_size);
+uint32_t flags = DIRTY_BITMAP_MIG_FLAG_BITS;
+
+bdrv_dirty_bitmap_serialize_part(dbms->bitmap, buf,
+ start_sector, nr_sectors);
While these bdrv_dirty_bitmap_* calls here seem fine, BdrvDirtyBitmap API is not
in general thread-safe, while this function is called without any lock. This
feels dangerous, as noted below, I'm most concerned about use-after-free.


This should be safe as it is a postcopy migration - source should be 
already inactive.





+
+if (buffer_is_zero(buf, buf_size)) {
+g_free(buf);
+buf = NULL;
+flags |= DIRTY_BITMAP_MIG_FLAG_ZEROES;
+}
+
+trace_send_bitmap_bits(flags, start_sector, nr_sectors, buf_size);
+
+send_bitmap_header(f, dbms, flags);
+
+qemu_put_be64(f, start_sector);
+qemu_put_be32(f, nr_sectors);
+
+/* if a block is zero we need to flush here since the network
+ * bandwidth is now a lot higher than the storage device bandwidth.
+ * thus if we queue zero blocks we slow down the migration. */
+if (flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
+qemu_fflush(f);
+} else {
+qemu_put_be64(f, buf_size);
+qemu_put_buffer(f, buf, buf_size);
+}
+
+g_free(buf);
+}
+
+


[...]


+
+static void dirty_bitmap_save_pending(QEMUFile *f, void *opaque,
+  uint64_t max_size,
+  uint64_t *res_precopy_only,
+  uint64_t *res_compatible,
+  uint64_t *res_postcopy_only)
+{
+DirtyBitmapMigBitmapState *dbms;
+uint64_t pending = 0;
+
+qemu_mutex_lock_iothread();

Why do you need the BQL here but not in bulk_phase()?


bulk_phase is in postcopy, source is inactive


--
Best regards,
Vladimir

Re: [Qemu-devel] [PULL 0/5] Docker testing and shippable patches

[Peter Maydell]

On 24 February 2017 at 06:32, Fam Zheng  wrote:
> The following changes since commit 10f25e4844cb9b3f02fb032f88051dd5b65b4206:
>
>   Merge remote-tracking branch 'remotes/yongbok/tags/mips-20170222' into 
> staging (2017-02-23 09:59:40 +)
>
> are available in the git repository at:
>
>   git://github.com/famz/qemu.git tags/for-upstream
>
> for you to fetch changes up to a8f159d45bcb78c00ea160bfc2b94512e4ed9910:
>
>   docker: Install python2 explicitly in docker image (2017-02-24 14:18:11 
> +0800)
>
> 
>
> Hi Peter,
>
> These are testing and build automation patches:
>
> - Shippable.com powered CI config
> - Docker cross build
> - Fixes and MAINTAINERS tweaks.

Applied, thanks.

-- PMM

[Qemu-devel] [PATCH v16 00/22] qcow2: persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi all!

There is a new update of qcow2-bitmap series - v16.

web: 
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=qcow2-bitmap-v16
git: https://src.openvz.org/scm/~vsementsov/qemu.git (tag qcow2-bitmap-v16)

v16:

mostly by Kevin's comments:
07: just moved here, as preparation for merging refcounts to 08
08: move "qcow2-bitmap: refcounts" staff to this patch, to not break qcow2-img 
check
drop r-b's
move necessary supporting static functions to this patch too (to not break 
compilation)
fprintf -> error_report
other small changes with error messages and comments
code style
for qcow2-bitmap.c:
  'include "exec/log.h"' was dropped
  s/1/(1U << 0) for BME_FLAG_IN_USE
  add BME_TABLE_ENTRY_FLAG_ALL_ONES to replace magic 1
  don't check 'cluster_size <= 0' in check_table_entry
old "[PATCH v15 08/25] block: introduce auto-loading bitmaps" was dropped
09: was "[PATCH v15 09/25] qcow2: add .bdrv_load_autoloading_dirty_bitmaps"
drop r-b's
some staff was moved to 08
update_header_sync - error on bdrv_flush fail
rename disk_sectors_in_bitmap_cluster to sectors_covered_by_bitmap_cluster
 and adjust comment.
 so, variable for storing this function result: s/dsc/sbc/
s/1/BME_TABLE_ENTRY_FLAG_ALL_ONES/
also, as Qcow2BitmapTable already introduced in 08,
   s/table_offset/table.offset/ and s/table_size/table.size, etc..
update_ext_header_and_dir_in_place: add comments, add additional
  update_header_sync, to reduce indeterminacy in case of error.
call qcow2_load_autoloading_dirty_bitmaps directly from qcow2_open
no .bdrv_load_autoloading_dirty_bitmaps
11: drop bdrv_store_persistent_dirty_bitmaps at all.
drop r-b's
13: rename patch, rewrite commit msg
drop r-b's
move bdrv_release_named_dirty_bitmaps in bdrv_close() after 
drv->bdrv_close()
Qcow2BitmapTable is already introduced, so no
  s/table_offset/table.offset/ and s/table_size/table.size, etc.. here
old 25 with check_constraints_on_bitmap() improvements merged here (Eric)
like in 09, s/dsc/sbc/ and 
s/disk_sectors_in_bitmap_cluster/sectors_covered_by_bitmap_cluster/
no .bdrv_store_persistent_dirty_bitmaps
call qcow2_store_persistent_dirty_bitmaps directly from qcow2_inactivate
15: corresponding part of 25 merged here. Add John's r-b, as 25 was also 
reviewed by John.
16: add John's r-b


v15:
13,14: add John's r-b
15: qcow2_can_store_new_dirty_bitmap:
  - check max dirty bitmaps and bitmap directory overhead
  - switch to error_prepend
rm Max's r-b
not add John's r-b
17-24: add John's r-b
25: changed because 15 changed,
not add John's r-b


v14:

07: use '|=' to update need_update_header
add John's r-b
add Max's r-b
09: remove unused bitmap_table_to_cpu()
left Max's r-b, hope it's ok
add John's r-b
10: remove extra new line
add John's r-b
11: add John's r-b
12: add John's r-b
13: small fixes by John's review:
   - remove weird g_free of NULL pointer from
   if (tb == NULL) {
   g_free(tb);
   return -EINVAL;
   }
   - remove extra comment "/* errp is already set */"
   - s/"Too large bitmap directory"/"Bitmap directory is too large"/
left Max's r-b, hope you don't mind 
22: add Max's r-b
23: add Max's r-b
24: add Max's r-b
25: new patch to improve error message on check_constraints_on_bitmap fail


v13: Just a fix for style checker.
13: line over 80
14: line over 80
22: s/if () \n{/if () {/

v12:
07: do not update header in qcow2_read_extensions, instead do it in the
end of qcow2_open, where it is updated also to clear unknown
autoclear features.
Wrong bdrv_is_root_node used instead of bdrv_is_read_only is fixed
automatically.
08: add Max's r-b
09: contextual: s/raw_bsd/raw-format/
qcow2-bitmap.c: copyright s/2016/2017/
fix compilation: move update_header_sync() to this patch
add Max's r-b
13: update_header_sync() is moved to 09
add Max's r-b
15: add Max's r-b
16: As qmp-commands.txt is removed from master, remove it here too.
Sentence "For now only Qcow2 disks support persistent bitmaps.
 Default is false." moved to qapi/block-core.json.
Hope that is OK, Max's r-b is not dropped.
17: qmp-commands.txt deleted, r-b is not dropped too.
18: s/is failed/has failed/, add Max's r-b
19: copyright: s/2016/2017/
21: drop "res->corruptions > 0 => ret = -EINVAL", add Max's r-b
22: actually, patch is replaced with a new one, however some parts are the same.
instead of changing bdrv_release_dirty_bitmap behaviour, create new
bdrv_remove_persistent_dirty_bitmap
23: improve comment, about not-exists is not an error
24: new patch

Old 24 (qcow2-bitmap: cache bitmap list in BDRVQcow2State) will be sent
separately, to be applied after these series.

Also: about bdrv_dirty_bitmap_set_persistance: I've decided not change its 
behaviour
for now and just call bdrv_remove_persistent_dirty_bitmap from

[Qemu-devel] [PATCH v16 15/22] qcow2: add .bdrv_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_can_store_new_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 block/qcow2-bitmap.c | 51 +++
 block/qcow2.c|  2 ++
 block/qcow2.h|  4 
 3 files changed, 57 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index e377215d5c..8c0c24c208 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1280,3 +1280,54 @@ fail:
 
 bitmap_list_free(bm_list);
 }
+
+bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  uint32_t granularity,
+  Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+bool found;
+Qcow2BitmapList *bm_list;
+
+if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
+goto fail;
+}
+
+if (s->nb_bitmaps == 0) {
+return true;
+}
+
+if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
+error_setg(errp,
+   "Maximum number of persistent bitmaps is already reached");
+goto fail;
+}
+
+if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) >
+QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
+{
+error_setg(errp, "No enough space in the bitmap directory");
+goto fail;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+goto fail;
+}
+
+found = find_bitmap_by_name(bm_list, name);
+bitmap_list_free(bm_list);
+if (found) {
+error_setg(errp, "Bitmap with the same name is already stored");
+goto fail;
+}
+
+return true;
+
+fail:
+error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
+  name, bdrv_get_device_or_node_name(bs));
+return false;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 201e7186f0..8f7937dc50 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3555,6 +3555,8 @@ BlockDriver bdrv_qcow2 = {
 
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+.bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index e2ef5698cd..c291858425 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -622,5 +622,9 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
   int64_t *refcount_table_size);
 void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  uint32_t granularity,
+  Error **errp);
 
 #endif
-- 
2.11.1

[Qemu-devel] [PATCH v16 09/22] qcow2: autoloading dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
are loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive.

Extra data in bitmaps is not supported for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 367 +++
 block/qcow2.c|   7 +
 block/qcow2.h|   2 +
 3 files changed, 376 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b8e472b3e8..73a6e87038 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -44,6 +44,8 @@
 
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffcU
+#define BME_FLAG_IN_USE (1U << 0)
+#define BME_FLAG_AUTO   (1U << 1)
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
@@ -85,6 +87,23 @@ typedef enum BitmapType {
 BT_DIRTY_TRACKING_BITMAP = 1
 } BitmapType;
 
+static inline bool can_write(BlockDriverState *bs)
+{
+return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+}
+
+static int update_header_sync(BlockDriverState *bs)
+{
+int ret;
+
+ret = qcow2_update_header(bs);
+if (ret < 0) {
+return ret;
+}
+
+return bdrv_flush(bs);
+}
+
 static int check_table_entry(uint64_t entry, int cluster_size)
 {
 uint64_t offset;
@@ -146,6 +165,120 @@ fail:
 return ret;
 }
 
+/* This function returns the number of disk sectors covered by a single qcow2
+ * cluster of bitmap data. */
+static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
+  const BdrvDirtyBitmap 
*bitmap)
+{
+uint32_t sector_granularity =
+bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+
+return (uint64_t)sector_granularity * (s->cluster_size << 3);
+}
+
+/* bitmap table entries must satisfy specification constraints */
+static int load_bitmap_data(BlockDriverState *bs,
+const uint64_t *bitmap_table,
+uint32_t bitmap_table_size,
+BdrvDirtyBitmap *bitmap)
+{
+int ret = 0;
+BDRVQcow2State *s = bs->opaque;
+uint64_t sector, sbc;
+uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+uint8_t *buf = NULL;
+uint64_t i, tab_size =
+size_to_clusters(s,
+bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+if (tab_size != bitmap_table_size || tab_size > BME_MAX_TABLE_SIZE) {
+return -EINVAL;
+}
+
+bdrv_clear_dirty_bitmap(bitmap, NULL);
+
+buf = g_malloc(s->cluster_size);
+sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
+for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
+uint64_t count = MIN(bm_size - sector, sbc);
+uint64_t entry = bitmap_table[i];
+uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+
+assert(check_table_entry(entry, s->cluster_size) == 0);
+
+if (offset == 0) {
+if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
+bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, count,
+   false);
+} else {
+/* No need to deserialize zeros because the dirty bitmap is
+ * already cleared */
+}
+} else {
+ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
+if (ret < 0) {
+goto finish;
+}
+bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, count,
+   false);
+}
+}
+ret = 0;
+
+bdrv_dirty_bitmap_deserialize_finish(bitmap);
+
+finish:
+g_free(buf);
+
+return ret;
+}
+
+static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
+Qcow2Bitmap *bm, Error **errp)
+{
+int ret;
+uint64_t *bitmap_table = NULL;
+uint32_t granularity;
+BdrvDirtyBitmap *bitmap = NULL;
+
+if (bm->flags & BME_FLAG_IN_USE) {
+error_setg(errp, "Bitmap '%s' is in use", bm->name);
+goto fail;
+}
+
+ret = bitmap_table_load(bs, >table, _table);
+if (ret < 0) {
+error_setg_errno(errp, -ret,
+ "Could not read bitmap_table table from image for "
+ "bitmap '%s'", bm->name);
+goto fail;
+}
+
+granularity = 1U << bm->granularity_bits;
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+if (bitmap == NULL) {
+goto fail;
+}
+
+ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
+ bm->name);
+goto fail;
+}
+
+g_free(bitmap_table);
+return bitmap;
+
+fail:
+g_free(bitmap_table);
+if 

[Qemu-devel] [PATCH v16 07/22] qcow2-refcount: rename inc_refcounts() and make it public

[Vladimir Sementsov-Ogievskiy]

This is needed for the following patch, which will introduce refcounts
checking for qcow2 bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/qcow2-refcount.c | 53 ++
 block/qcow2.h  |  4 
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cbfb3fe064..14a736d245 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1309,11 +1309,10 @@ static int realloc_refcount_array(BDRVQcow2State *s, 
void **array,
  *
  * Modifies the number of errors in res.
  */
-static int inc_refcounts(BlockDriverState *bs,
- BdrvCheckResult *res,
- void **refcount_table,
- int64_t *refcount_table_size,
- int64_t offset, int64_t size)
+int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
+ void **refcount_table,
+ int64_t *refcount_table_size,
+ int64_t offset, int64_t size)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t start, last, cluster_offset, k, refcount;
@@ -1406,8 +1405,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 nb_csectors = ((l2_entry >> s->csize_shift) &
s->csize_mask) + 1;
 l2_entry &= s->cluster_offset_mask;
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l2_entry & ~511, nb_csectors * 512);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   l2_entry & ~511, nb_csectors * 512);
 if (ret < 0) {
 goto fail;
 }
@@ -1445,8 +1445,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 }
 
 /* Mark cluster as used */
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   offset, s->cluster_size);
 if (ret < 0) {
 goto fail;
 }
@@ -1498,8 +1499,8 @@ static int check_refcounts_l1(BlockDriverState *bs,
 l1_size2 = l1_size * sizeof(uint64_t);
 
 /* Mark L1 table as used */
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l1_table_offset, l1_size2);
+ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
refcount_table_size,
+   l1_table_offset, l1_size2);
 if (ret < 0) {
 goto fail;
 }
@@ -1528,8 +1529,9 @@ static int check_refcounts_l1(BlockDriverState *bs,
 if (l2_offset) {
 /* Mark L2 table as used */
 l2_offset &= L1E_OFFSET_MASK;
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l2_offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   l2_offset, s->cluster_size);
 if (ret < 0) {
 goto fail;
 }
@@ -1744,14 +1746,15 @@ static int check_refblocks(BlockDriverState *bs, 
BdrvCheckResult *res,
 }
 
 res->corruptions_fixed++;
-ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, nb_clusters,
+   offset, s->cluster_size);
 if (ret < 0) {
 return ret;
 }
 /* No need to check whether the refcount is now greater than 1:
  * This area was just allocated and zeroed, so it can only be
- * exactly 1 after inc_refcounts() */
+ * exactly 1 after qcow2_inc_refcounts_imrt() */
 continue;
 
 resize_fail:
@@ -1766,8 +1769,8 @@ resize_fail:
 }
 
 if (offset != 0) {
-ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
nb_clusters,
+   offset, s->cluster_size);
 if (ret < 0) {
 return ret;
 }

Re: [Qemu-devel] [PATCH 12/17] migration: add postcopy migration of dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

24.02.2017 16:26, Dr. David Alan Gilbert wrote:

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:

Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
associated with root nodes and non-root named nodes are migrated.

If destination qemu is already containing a dirty bitmap with the same name
as a migrated bitmap (for the same node), than, if their granularities are
the same the migration will be done, otherwise the error will be generated.

If destination qemu doesn't contain such bitmap it will be created.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/migration/block.h  |   1 +
  include/migration/migration.h  |   4 +
  migration/Makefile.objs|   2 +-
  migration/block-dirty-bitmap.c | 679 +
  migration/migration.c  |   3 +
  migration/savevm.c |   2 +
  migration/trace-events |  14 +
  vl.c   |   1 +
  8 files changed, 705 insertions(+), 1 deletion(-)
  create mode 100644 migration/block-dirty-bitmap.c

diff --git a/include/migration/block.h b/include/migration/block.h
index 41a1ac8..8333c43 100644
--- a/include/migration/block.h
+++ b/include/migration/block.h
@@ -14,6 +14,7 @@
  #ifndef MIGRATION_BLOCK_H
  #define MIGRATION_BLOCK_H
  
+void dirty_bitmap_mig_init(void);

  void blk_mig_init(void);
  int blk_mig_active(void);
  uint64_t blk_mig_bytes_transferred(void);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 46645f4..03a4993 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -371,4 +371,8 @@ int ram_save_queue_pages(MigrationState *ms, const char 
*rbname,
  PostcopyState postcopy_state_get(void);
  /* Set the state and return the old state */
  PostcopyState postcopy_state_set(PostcopyState new_state);
+
+void dirty_bitmap_mig_before_vm_start(void);
+void init_dirty_bitmap_incoming_migration(void);
+
  #endif
diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 480dd49..fa3bf6a 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -9,5 +9,5 @@ common-obj-y += qjson.o
  
  common-obj-$(CONFIG_RDMA) += rdma.o
  
-common-obj-y += block.o

+common-obj-y += block.o block-dirty-bitmap.o
  
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c

new file mode 100644
index 000..28e3732
--- /dev/null
+++ b/migration/block-dirty-bitmap.c
@@ -0,0 +1,679 @@
+/*
+ * Block dirty bitmap postcopy migration
+ *
+ * Copyright IBM, Corp. 2009
+ * Copyright (C) 2016 Parallels IP Holdings GmbH. All rights reserved.
+ *
+ * Authors:
+ *  Liran Schour   
+ *  Vladimir Sementsov-Ogievskiy 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ * This file is derived from migration/block.c, so it's author and IBM 
copyright
+ * are here, although content is quite different.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ *
+ ****
+ *
+ * Here postcopy migration of dirty bitmaps is realized. Only named dirty
+ * bitmaps, associated with root nodes and non-root named nodes are migrated.
+ *
+ * If destination qemu is already containing a dirty bitmap with the same name
+ * as a migrated bitmap (for the same node), then, if their granularities are
+ * the same the migration will be done, otherwise the error will be generated.
+ *
+ * If destination qemu doesn't contain such bitmap it will be created.
+ *
+ * format of migration:
+ *
+ * # Header (shared for different chunk types)
+ * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
+ * [ 1 byte: node name size ] \  flags & DEVICE_NAME
+ * [ n bytes: node name ] /
+ * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
+ * [ n bytes: bitmap name ] /
+ *
+ * # Start of bitmap migration (flags & START)
+ * header
+ * be64: granularity
+ * 1 byte: bitmap enabled flag
+ *
+ * # Complete of bitmap migration (flags & COMPLETE)
+ * header
+ *
+ * # Data chunk of bitmap migration
+ * header
+ * be64: start sector
+ * be32: number of sectors
+ * [ be64: buffer size  ] \ ! (flags & ZEROES)
+ * [ n bytes: buffer] /
+ *
+ * The last chunk in stream should contain flags & EOS. The chunk may skip
+ * device and/or bitmap names, assuming them to be the same with the previous
+ * chunk.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "block/block_int.h"
+#include "sysemu/block-backend.h"
+#include "qemu/main-loop.h"
+#include "qemu/error-report.h"
+#include "migration/block.h"
+#include "migration/migration.h"
+#include "qemu/hbitmap.h"
+#include "sysemu/sysemu.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "trace.h"
+#include 
+
+#define CHUNK_SIZE (1 << 10)
+
+/* Flags occupy from one to four bytes. In all 

[Qemu-devel] [PATCH v16 08/22] qcow2: add bitmaps extension

[Vladimir Sementsov-Ogievskiy]

Add bitmap extension as specified in docs/specs/qcow2.txt.
For now, just mirror extension header into Qcow2 state and check
constraints. Also, calculate refcounts for qcow2 bitmaps, to not break
qemu-img check.

For now, disable image resize if it has bitmaps. It will be fixed later.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/Makefile.objs|   2 +-
 block/qcow2-bitmap.c   | 439 +
 block/qcow2-refcount.c |   6 +
 block/qcow2.c  | 124 +-
 block/qcow2.h  |  27 +++
 5 files changed, 592 insertions(+), 6 deletions(-)
 create mode 100644 block/qcow2-bitmap.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index c6bd14e883..ff8d18511b 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,5 +1,5 @@
 block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o 
dmg.o
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o qcow2-bitmap.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 00..b8e472b3e8
--- /dev/null
+++ b/block/qcow2-bitmap.c
@@ -0,0 +1,439 @@
+/*
+ * Bitmaps for the QCOW version 2 format
+ *
+ * Copyright (c) 2014-2017 Vladimir Sementsov-Ogievskiy
+ *
+ * This file is derived from qcow2-snapshot.c, original copyright:
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+
+#include "block/block_int.h"
+#include "block/qcow2.h"
+
+/* NOTICE: BME here means Bitmaps Extension and used as a namespace for
+ * _internal_ constants. Please do not use this _internal_ abbreviation for
+ * other needs and/or outside of this file. */
+
+/* Bitmap directory entry constraints */
+#define BME_MAX_TABLE_SIZE 0x800
+#define BME_MAX_PHYS_SIZE 0x2000 /* restrict BdrvDirtyBitmap size in RAM */
+#define BME_MAX_GRANULARITY_BITS 31
+#define BME_MIN_GRANULARITY_BITS 9
+#define BME_MAX_NAME_SIZE 1023
+
+/* Bitmap directory entry flags */
+#define BME_RESERVED_FLAGS 0xfffcU
+
+/* bits [1, 8] U [56, 63] are reserved */
+#define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
+#define BME_TABLE_ENTRY_OFFSET_MASK 0x00fffe00ULL
+#define BME_TABLE_ENTRY_FLAG_ALL_ONES (1ULL << 0)
+
+typedef struct QEMU_PACKED Qcow2BitmapDirEntry {
+/* header is 8 byte aligned */
+uint64_t bitmap_table_offset;
+
+uint32_t bitmap_table_size;
+uint32_t flags;
+
+uint8_t type;
+uint8_t granularity_bits;
+uint16_t name_size;
+uint32_t extra_data_size;
+/* extra data follows  */
+/* name follows  */
+} Qcow2BitmapDirEntry;
+
+typedef struct Qcow2BitmapTable {
+uint64_t offset;
+uint32_t size; /* number of 64bit entries */
+QSIMPLEQ_ENTRY(Qcow2BitmapTable) entry;
+} Qcow2BitmapTable;
+
+typedef struct Qcow2Bitmap {
+Qcow2BitmapTable table;
+uint32_t flags;
+uint8_t granularity_bits;
+char *name;
+
+QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
+} Qcow2Bitmap;
+typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
+
+typedef enum BitmapType {
+BT_DIRTY_TRACKING_BITMAP = 1
+} BitmapType;
+
+static int check_table_entry(uint64_t entry, int cluster_size)
+{
+uint64_t offset;
+
+if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
+return -EINVAL;
+}
+
+offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+if (offset != 0) {
+/* if offset specified, bit 0 is reserved */
+if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
+return -EINVAL;
+}
+
+if (offset % cluster_size != 0) {
+return -EINVAL;
+}
+}

[Qemu-devel] [PATCH v16 18/22] qmp: add x-debug-block-dirty-bitmap-sha256

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c |  5 +
 blockdev.c   | 29 +
 include/block/dirty-bitmap.h |  2 ++
 include/qemu/hbitmap.h   |  8 
 qapi/block-core.json | 27 +++
 tests/Makefile.include   |  2 +-
 util/hbitmap.c   | 11 +++
 7 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9208844a7d..b684d8b00e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -588,3 +588,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState 
*bs,
 return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
 QLIST_NEXT(bitmap, list);
 }
+
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
+{
+return hbitmap_sha256(bitmap->bitmap, errp);
+}
diff --git a/blockdev.c b/blockdev.c
index e32ac69e4b..c41b791289 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2820,6 +2820,35 @@ void qmp_block_dirty_bitmap_clear(const char *node, 
const char *name,
 aio_context_release(aio_context);
 }
 
+BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
+  const char *name,
+  Error **errp)
+{
+AioContext *aio_context;
+BdrvDirtyBitmap *bitmap;
+BlockDriverState *bs;
+BlockDirtyBitmapSha256 *ret = NULL;
+char *sha256;
+
+bitmap = block_dirty_bitmap_lookup(node, name, , _context, errp);
+if (!bitmap || !bs) {
+return NULL;
+}
+
+sha256 = bdrv_dirty_bitmap_sha256(bitmap, errp);
+if (sha256 == NULL) {
+goto out;
+}
+
+ret = g_new(BlockDirtyBitmapSha256, 1);
+ret->sha256 = sha256;
+
+out:
+aio_context_release(aio_context);
+
+return ret;
+}
+
 void hmp_drive_del(Monitor *mon, const QDict *qdict)
 {
 const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index d71edc4d13..b022b34964 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -86,4 +86,6 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
 
 bool bdrv_has_persistent_bitmaps(BlockDriverState *bs);
 
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index b52304ac29..d3a74a21fc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -253,6 +253,14 @@ void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, 
uint64_t count,
 void hbitmap_deserialize_finish(HBitmap *hb);
 
 /**
+ * hbitmap_sha256:
+ * @bitmap: HBitmap to operate on.
+ *
+ * Returns SHA256 hash of the last level.
+ */
+char *hbitmap_sha256(const HBitmap *bitmap, Error **errp);
+
+/**
  * hbitmap_free:
  * @hb: HBitmap to operate on.
  *
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 09dcf4e6ae..d6f173448a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1642,6 +1642,33 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @BlockDirtyBitmapSha256:
+#
+# SHA256 hash of dirty bitmap data
+#
+# @sha256: ASCII representation of SHA256 bitmap hash
+#
+# Since: 2.9
+##
+  { 'struct': 'BlockDirtyBitmapSha256',
+'data': {'sha256': 'str'} }
+
+##
+# @x-debug-block-dirty-bitmap-sha256:
+#
+# Get bitmap SHA256
+#
+# Returns: BlockDirtyBitmapSha256 on success
+#  If @node is not a valid block device, DeviceNotFound
+#  If @name is not found or if hashing has failed, GenericError with an
+#  explanation
+#
+# Since: 2.9
+##
+  { 'command': 'x-debug-block-dirty-bitmap-sha256',
+'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
+
+##
 # @blockdev-mirror:
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/tests/Makefile.include b/tests/Makefile.include
index ad35a6494b..d0809fefb2 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -519,7 +519,7 @@ tests/test-blockjob$(EXESUF): tests/test-blockjob.o 
$(test-block-obj-y) $(test-u
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o 
$(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
-tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
+tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) 
$(test-crypto-obj-y)
 tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o 
page_cache.o $(test-util-obj-y)
 tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 

[Qemu-devel] [PATCH v16 01/22] specs/qcow2: fix bitmap granularity qemu-specific note

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 docs/specs/qcow2.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0e91..dda53dd2a3 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -472,8 +472,7 @@ Structure of a bitmap directory entry:
  17:granularity_bits
 Granularity bits. Valid values: 0 - 63.
 
-Note: Qemu currently doesn't support granularity_bits
-greater than 31.
+Note: Qemu currently supports only values 9 - 31.
 
 Granularity is calculated as
 granularity = 1 << granularity_bits
-- 
2.11.1

[Qemu-devel] [PATCH v16 13/22] qcow2: add persistent dirty bitmaps support

[Vladimir Sementsov-Ogievskiy]

Store persistent dirty bitmaps in qcow2 image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c  |   6 +-
 block/qcow2-bitmap.c | 473 +++
 block/qcow2.c|   9 +
 block/qcow2.h|   1 +
 4 files changed, 486 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index a0346c80c6..16cf522219 100644
--- a/block.c
+++ b/block.c
@@ -2322,9 +2322,6 @@ static void bdrv_close(BlockDriverState *bs)
 bdrv_flush(bs);
 bdrv_drain(bs); /* in case flush left pending I/O */
 
-bdrv_release_named_dirty_bitmaps(bs);
-assert(QLIST_EMPTY(>dirty_bitmaps));
-
 if (bs->drv) {
 BdrvChild *child, *next;
 
@@ -2363,6 +2360,9 @@ static void bdrv_close(BlockDriverState *bs)
 bs->full_open_options = NULL;
 }
 
+bdrv_release_named_dirty_bitmaps(bs);
+assert(QLIST_EMPTY(>dirty_bitmaps));
+
 QLIST_FOREACH_SAFE(ban, >aio_notifiers, list, ban_next) {
 g_free(ban);
 }
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ba72b7d2ac..e377215d5c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -27,6 +27,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 
 #include "block/block_int.h"
 #include "block/qcow2.h"
@@ -42,6 +43,10 @@
 #define BME_MIN_GRANULARITY_BITS 9
 #define BME_MAX_NAME_SIZE 1023
 
+#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
+#error In the code bitmap table physical size assumed to fit into int
+#endif
+
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffcU
 #define BME_FLAG_IN_USE (1U << 0)
@@ -72,6 +77,8 @@ typedef struct Qcow2BitmapTable {
 uint32_t size; /* number of 64bit entries */
 QSIMPLEQ_ENTRY(Qcow2BitmapTable) entry;
 } Qcow2BitmapTable;
+typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
+Qcow2BitmapTableList;
 
 typedef struct Qcow2Bitmap {
 Qcow2BitmapTable table;
@@ -79,6 +86,8 @@ typedef struct Qcow2Bitmap {
 uint8_t granularity_bits;
 char *name;
 
+BdrvDirtyBitmap *dirty_bitmap;
+
 QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
 } Qcow2Bitmap;
 typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
@@ -104,6 +113,15 @@ static int update_header_sync(BlockDriverState *bs)
 return bdrv_flush(bs);
 }
 
+static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size)
+{
+size_t i;
+
+for (i = 0; i < size; ++i) {
+cpu_to_be64s(_table[i]);
+}
+}
+
 static int check_table_entry(uint64_t entry, int cluster_size)
 {
 uint64_t offset;
@@ -127,6 +145,70 @@ static int check_table_entry(uint64_t entry, int 
cluster_size)
 return 0;
 }
 
+static int check_constraints_on_bitmap(BlockDriverState *bs,
+   const char *name,
+   uint32_t granularity,
+   Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+int granularity_bits = ctz32(granularity);
+int64_t len = bdrv_getlength(bs);
+
+assert(granularity > 0);
+assert((granularity & (granularity - 1)) == 0);
+
+if (len < 0) {
+error_setg_errno(errp, -len, "Failed to get size of '%s'",
+ bdrv_get_device_or_node_name(bs));
+return len;
+}
+
+if (granularity_bits > BME_MAX_GRANULARITY_BITS) {
+error_setg(errp, "Granularity exceeds maximum (%u bytes)",
+   1 << BME_MAX_GRANULARITY_BITS);
+return -EINVAL;
+}
+if (granularity_bits < BME_MIN_GRANULARITY_BITS) {
+error_setg(errp, "Granularity is under minimum (%u bytes)",
+   1 << BME_MIN_GRANULARITY_BITS);
+return -EINVAL;
+}
+
+if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
+(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
+   granularity_bits))
+{
+error_setg(errp, "Too much space will be occupied by the bitmap. "
+   "Use larger granularity");
+return -EINVAL;
+}
+
+if (strlen(name) > BME_MAX_NAME_SIZE) {
+error_setg(errp, "Name length exceeds maximum (%u characters)",
+   BME_MAX_NAME_SIZE);
+return -EINVAL;
+}
+
+return 0;
+}
+
+static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
+   uint32_t bitmap_table_size)
+{
+BDRVQcow2State *s = bs->opaque;
+int i;
+
+for (i = 0; i < bitmap_table_size; ++i) {
+uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
+if (!addr) {
+continue;
+}
+
+qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
+bitmap_table[i] = 0;
+}
+}
+
 static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable *tb,
  uint64_t **bitmap_table)
 {
@@ -165,6 +247,28 @@ fail:
 return ret;
 }
 
+static int 

[Qemu-devel] [PATCH v16 21/22] qcow2: add .bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_remove_persistent_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/qcow2-bitmap.c | 41 +
 block/qcow2.c|  1 +
 block/qcow2.h|  3 +++
 3 files changed, 45 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 8c0c24c208..834258e13c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1153,6 +1153,47 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList 
*bm_list,
 return NULL;
 }
 
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  Error **errp)
+{
+int ret;
+BDRVQcow2State *s = bs->opaque;
+Qcow2Bitmap *bm;
+Qcow2BitmapList *bm_list;
+
+if (s->nb_bitmaps == 0) {
+/* Absence of the bitmap is not an error: see explanation above
+ * bdrv_remove_persistent_dirty_bitmap() definition. */
+return;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+return;
+}
+
+bm = find_bitmap_by_name(bm_list, name);
+if (bm == NULL) {
+goto fail;
+}
+
+QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
+
+ret = update_ext_header_and_dir(bs, bm_list);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to update bitmap extension");
+goto fail;
+}
+
+free_bitmap_clusters(bs, >table);
+
+fail:
+bitmap_free(bm);
+bitmap_list_free(bm_list);
+}
+
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
diff --git a/block/qcow2.c b/block/qcow2.c
index 8f7937dc50..069d30939c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3557,6 +3557,7 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
 .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
+.bdrv_remove_persistent_dirty_bitmap = 
qcow2_remove_persistent_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index c291858425..cb70704c40 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -626,5 +626,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
   const char *name,
   uint32_t granularity,
   Error **errp);
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  Error **errp);
 
 #endif
-- 
2.11.1

[Qemu-devel] [PATCH v16 06/22] block/dirty-bitmap: add deserialize_ones func

[Vladimir Sementsov-Ogievskiy]

Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
all-ones.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c |  7 +++
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h   | 15 +++
 util/hbitmap.c   | 17 +
 4 files changed, 42 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 186941cfc3..90af37287f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -499,6 +499,13 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap 
*bitmap,
 hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish);
 }
 
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+uint64_t start, uint64_t count,
+bool finish)
+{
+hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish);
+}
+
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
 {
 hbitmap_deserialize_finish(bitmap->bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 7cbe623ba7..1e17729ac2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -70,6 +70,9 @@ void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap 
*bitmap,
 void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
   uint64_t start, uint64_t count,
   bool finish);
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+uint64_t start, uint64_t count,
+bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 6b04391266..b52304ac29 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -229,6 +229,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t 
start, uint64_t count,
 bool finish);
 
 /**
+ * hbitmap_deserialize_ones
+ * @hb: HBitmap to operate on.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ * @finish: Whether to call hbitmap_deserialize_finish automatically.
+ *
+ * Fills the bitmap with ones.
+ *
+ * If @finish is false, caller must call hbitmap_serialize_finish before using
+ * the bitmap.
+ */
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+  bool finish);
+
+/**
  * hbitmap_deserialize_finish
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 0b38817505..0c1591a594 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -551,6 +551,23 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t 
start, uint64_t count,
 }
 }
 
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+  bool finish)
+{
+uint64_t el_count;
+unsigned long *first;
+
+if (!count) {
+return;
+}
+serialization_chunk(hb, start, count, , _count);
+
+memset(first, 0xff, el_count * sizeof(unsigned long));
+if (finish) {
+hbitmap_deserialize_finish(hb);
+}
+}
+
 void hbitmap_deserialize_finish(HBitmap *bitmap)
 {
 int64_t i, size, prev_size;
-- 
2.11.1

[Qemu-devel] [PATCH v16 22/22] qmp: block-dirty-bitmap-remove: remove persistent

[Vladimir Sementsov-Ogievskiy]

Remove persistent bitmap from the storage on block-dirty-bitmap-remove.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index c41b791289..a365cdf3ed 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2767,6 +2767,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
 AioContext *aio_context;
 BlockDriverState *bs;
 BdrvDirtyBitmap *bitmap;
+Error *local_err = NULL;
 
 bitmap = block_dirty_bitmap_lookup(node, name, , _context, errp);
 if (!bitmap || !bs) {
@@ -2779,6 +2780,15 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
name);
 goto out;
 }
+
+if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+bdrv_remove_persistent_dirty_bitmap(bs, name, _err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto out;
+}
+}
+
 bdrv_dirty_bitmap_make_anon(bitmap);
 bdrv_release_dirty_bitmap(bs, bitmap);
 
-- 
2.11.1

[Qemu-devel] [PATCH v16 03/22] hbitmap: improve dirty iter

[Vladimir Sementsov-Ogievskiy]

Make dirty iter resistant to resetting bits in corresponding HBitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 include/qemu/hbitmap.h | 26 --
 util/hbitmap.c | 23 ++-
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 9239fe515e..6b04391266 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -256,10 +256,9 @@ void hbitmap_free(HBitmap *hb);
  * the lowest-numbered bit that is set in @hb, starting at @first.
  *
  * Concurrent setting of bits is acceptable, and will at worst cause the
- * iteration to miss some of those bits.  Resetting bits before the current
- * position of the iterator is also okay.  However, concurrent resetting of
- * bits can lead to unexpected behavior if the iterator has not yet reached
- * those bits.
+ * iteration to miss some of those bits.
+ *
+ * The concurrent resetting of bits is OK.
  */
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
 
@@ -298,24 +297,7 @@ void hbitmap_free_meta(HBitmap *hb);
  * Return the next bit that is set in @hbi's associated HBitmap,
  * or -1 if all remaining bits are zero.
  */
-static inline int64_t hbitmap_iter_next(HBitmapIter *hbi)
-{
-unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-int64_t item;
-
-if (cur == 0) {
-cur = hbitmap_iter_skip_words(hbi);
-if (cur == 0) {
-return -1;
-}
-}
-
-/* The next call will resume work from the next bit.  */
-hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
-item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
-
-return item << hbi->granularity;
-}
+int64_t hbitmap_iter_next(HBitmapIter *hbi);
 
 /**
  * hbitmap_iter_next_word:
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 35088e19c4..0b38817505 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 
 unsigned long cur;
 do {
-cur = hbi->cur[--i];
+i--;
 pos >>= BITS_PER_LEVEL;
+cur = hbi->cur[i] & hb->levels[i][pos];
 } while (cur == 0);
 
 /* Check for end of iteration.  We always use fewer than BITS_PER_LONG
@@ -139,6 +140,26 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 return cur;
 }
 
+int64_t hbitmap_iter_next(HBitmapIter *hbi)
+{
+unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1] &
+hbi->hb->levels[HBITMAP_LEVELS - 1][hbi->pos];
+int64_t item;
+
+if (cur == 0) {
+cur = hbitmap_iter_skip_words(hbi);
+if (cur == 0) {
+return -1;
+}
+}
+
+/* The next call will resume work from the next bit.  */
+hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
+item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
+
+return item << hbi->granularity;
+}
+
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
 {
 unsigned i, bit;
-- 
2.11.1

[Qemu-devel] [PATCH v16 20/22] block/dirty-bitmap: add bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Interface for removing persistent bitmap from its storage.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 18 ++
 include/block/block_int.h|  3 +++
 include/block/dirty-bitmap.h |  3 +++
 3 files changed, 24 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index b684d8b00e..05e0aaef90 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -327,12 +327,30 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 /**
  * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()).
  * There must not be any frozen bitmaps attached.
+ * This function does not remove persistent bitmaps from the storage.
  */
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
 {
 bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
 }
 
+/**
+ * Remove persistent dirty bitmap from the storage if it exists.
+ * Absence of bitmap is not an error, because we have the following scenario:
+ * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
+ * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
+ * not fail.
+ * This function doesn't release corresponding BdrvDirtyBitmap.
+ */
+void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp)
+{
+if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
+bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+}
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f943a7c24e..f7f7dd7717 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -324,6 +324,9 @@ struct BlockDriver {
 const char *name,
 uint32_t granularity,
 Error **errp);
+void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+const char *name,
+Error **errp);
 
 QLIST_ENTRY(BlockDriver) list;
 };
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index b022b34964..ce126109e0 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -25,6 +25,9 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
+void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
-- 
2.11.1

[Qemu-devel] [PATCH v16 19/22] iotests: test qcow2 persistent dirty bitmap

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 tests/qemu-iotests/165 | 89 ++
 tests/qemu-iotests/165.out |  5 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 95 insertions(+)
 create mode 100755 tests/qemu-iotests/165
 create mode 100644 tests/qemu-iotests/165.out

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
new file mode 100755
index 00..d583b33733
--- /dev/null
+++ b/tests/qemu-iotests/165
@@ -0,0 +1,89 @@
+#!/usr/bin/env python
+#
+# Tests for persistent dirty bitmaps.
+#
+# Copyright: Vladimir Sementsov-Ogievskiy 2015-2017
+#
+# 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 .
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+disk_size = 0x4000 # 1G
+
+# regions for qemu_io: (start, count) in bytes
+regions1 = ((0,0x10),
+(0x20, 0x10))
+
+regions2 = ((0x1000, 0x2),
+(0x3fff, 0x1))
+
+class TestPersistentDirtyBitmap(iotests.QMPTestCase):
+
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+
+def tearDown(self):
+os.remove(disk)
+
+def mkVm(self):
+return iotests.VM().add_drive(disk)
+
+def getSha256(self):
+result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+return result['return']['sha256']
+
+def checkBitmap(self, sha256):
+result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+self.assert_qmp(result, 'return/sha256', sha256);
+
+def writeRegions(self, regions):
+for r in regions:
+self.vm.hmp_qemu_io('drive0',
+'write %d %d' % r)
+
+def qmpAddBitmap(self):
+self.vm.qmp('block-dirty-bitmap-add', node='drive0',
+name='bitmap0', persistent=True, autoload=True)
+
+def test_persistent(self):
+self.vm = self.mkVm()
+self.vm.launch()
+self.qmpAddBitmap()
+
+self.writeRegions(regions1)
+sha256 = self.getSha256()
+
+self.vm.shutdown()
+self.vm = self.mkVm()
+self.vm.launch()
+
+self.checkBitmap(sha256)
+self.writeRegions(regions2)
+sha256 = self.getSha256()
+
+self.vm.shutdown()
+self.vm.launch()
+
+self.checkBitmap(sha256)
+
+self.vm.shutdown()
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
new file mode 100644
index 00..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/165.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 866c1a032d..8bd7a84f5a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -162,6 +162,7 @@
 159 rw auto quick
 160 rw auto quick
 162 auto quick
+165 rw auto quick
 170 rw auto quick
 171 rw auto quick
 172 auto
-- 
2.11.1

[Qemu-devel] [PATCH v16 11/22] block: introduce persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
on bdrv_close, using format driver. Format driver should maintain bitmap
storing.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/dirty-bitmap.c | 26 ++
 block/qcow2-bitmap.c |  1 +
 include/block/dirty-bitmap.h |  6 ++
 3 files changed, 33 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index a9dfce8d00..d2fbf55964 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,7 @@ struct BdrvDirtyBitmap {
 int64_t size;   /* Size of the bitmap (Number of sectors) */
 bool disabled;  /* Bitmap is read-only */
 int active_iterators;   /* How many iterators are active */
+bool persistent;/* bitmap must be saved to owner disk image */
 bool autoload;  /* For persistent bitmaps: bitmap must be
autoloaded on image opening */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -72,6 +73,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
 bitmap->name = NULL;
+bitmap->persistent = false;
 bitmap->autoload = false;
 }
 
@@ -241,6 +243,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->name = NULL;
 successor->name = name;
 bitmap->successor = NULL;
+successor->persistent = bitmap->persistent;
+bitmap->persistent = false;
 successor->autoload = bitmap->autoload;
 bitmap->autoload = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
@@ -555,3 +559,25 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
*bitmap)
 {
 return bitmap->autoload;
 }
+
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool 
persistent)
+{
+bitmap->persistent = persistent;
+}
+
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
+{
+return bitmap->persistent;
+}
+
+bool bdrv_has_persistent_bitmaps(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bm;
+QLIST_FOREACH(bm, >dirty_bitmaps, list) {
+if (bm->persistent) {
+return true;
+}
+}
+
+return false;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6b1a2c9c67..ba72b7d2ac 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -780,6 +780,7 @@ void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState 
*bs, Error **errp)
 goto fail;
 }
 
+bdrv_dirty_bitmap_set_persistance(bitmap, true);
 bdrv_dirty_bitmap_set_autoload(bitmap, true);
 bm->flags |= BME_FLAG_IN_USE;
 created_dirty_bitmaps =
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 45a389a20a..8dbd16b040 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -77,4 +77,10 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap 
*bitmap);
 
 void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+bool persistent);
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+
+bool bdrv_has_persistent_bitmaps(BlockDriverState *bs);
+
 #endif
-- 
2.11.1

[Qemu-devel] [PATCH v16 04/22] tests: add hbitmap iter test

[Vladimir Sementsov-Ogievskiy]

Test that hbitmap iter is resistant to bitmap resetting.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 tests/test-hbitmap.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 23773d2051..1acb353889 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -909,6 +909,22 @@ static void hbitmap_test_add(const char *testpath,
hbitmap_test_teardown);
 }
 
+static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
+const void *unused)
+{
+HBitmapIter hbi;
+
+hbitmap_test_init(data, L1 * 2, 0);
+hbitmap_set(data->hb, 0, data->size);
+
+hbitmap_iter_init(, data->hb, BITS_PER_LONG - 1);
+
+hbitmap_iter_next();
+
+hbitmap_reset_all(data->hb);
+hbitmap_iter_next();
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
@@ -966,6 +982,9 @@ int main(int argc, char **argv)
  test_hbitmap_serialize_part);
 hbitmap_test_add("/hbitmap/serialize/zeroes",
  test_hbitmap_serialize_zeroes);
+
+hbitmap_test_add("/hbitmap/iter/iter_and_reset",
+ test_hbitmap_iter_and_reset);
 g_test_run();
 
 return 0;
-- 
2.11.1

[Qemu-devel] [PATCH v16 12/22] block/dirty-bitmap: add bdrv_dirty_bitmap_next()

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 7 +++
 include/block/dirty-bitmap.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d2fbf55964..9208844a7d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -581,3 +581,10 @@ bool bdrv_has_persistent_bitmaps(BlockDriverState *bs)
 
 return false;
 }
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+BdrvDirtyBitmap *bitmap)
+{
+return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
+QLIST_NEXT(bitmap, list);
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8dbd16b040..d71edc4d13 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -81,6 +81,9 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap 
*bitmap,
 bool persistent);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
 
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+BdrvDirtyBitmap *bitmap);
+
 bool bdrv_has_persistent_bitmaps(BlockDriverState *bs);
 
 #endif
-- 
2.11.1

[Qemu-devel] [PATCH v16 05/22] block: fix bdrv_dirty_bitmap_granularity signature

[Vladimir Sementsov-Ogievskiy]

Make getter signature const-correct. This allows other functions with
const dirty bitmap parameter use bdrv_dirty_bitmap_granularity().

Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Kevin Wolf 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/dirty-bitmap.c | 2 +-
 include/block/dirty-bitmap.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 519737c8d3..186941cfc3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ uint32_t 
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 return granularity;
 }
 
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
 return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9dea14ba03..7cbe623ba7 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -29,7 +29,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
 uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
-- 
2.11.1

[Qemu-devel] [PATCH v16 10/22] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

Mirror AUTO flag from Qcow2 bitmap in BdrvDirtyBitmap. This will be
needed in future, to save this flag back to Qcow2 for persistent
bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 15 +++
 block/qcow2-bitmap.c |  2 ++
 include/block/dirty-bitmap.h |  2 ++
 3 files changed, 19 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..a9dfce8d00 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
 int64_t size;   /* Size of the bitmap (Number of sectors) */
 bool disabled;  /* Bitmap is read-only */
 int active_iterators;   /* How many iterators are active */
+bool autoload;  /* For persistent bitmaps: bitmap must be
+   autoloaded on image opening */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -70,6 +72,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
 bitmap->name = NULL;
+bitmap->autoload = false;
 }
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
@@ -238,6 +241,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->name = NULL;
 successor->name = name;
 bitmap->successor = NULL;
+successor->autoload = bitmap->autoload;
+bitmap->autoload = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
 
 return successor;
@@ -540,3 +545,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
 return hbitmap_count(bitmap->meta);
 }
+
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
+{
+bitmap->autoload = autoload;
+}
+
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
+{
+return bitmap->autoload;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 73a6e87038..6b1a2c9c67 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -779,6 +779,8 @@ void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState 
*bs, Error **errp)
 if (bitmap == NULL) {
 goto fail;
 }
+
+bdrv_dirty_bitmap_set_autoload(bitmap, true);
 bm->flags |= BME_FLAG_IN_USE;
 created_dirty_bitmaps =
 g_slist_append(created_dirty_bitmaps, bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1e17729ac2..45a389a20a 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -75,4 +75,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap 
*bitmap,
 bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 #endif
-- 
2.11.1

[Qemu-devel] [PATCH v16 02/22] specs/qcow2: do not use wording 'bitmap header'

[Vladimir Sementsov-Ogievskiy]

A bitmap directory entry is sometimes called a 'bitmap header'. This
patch leaves only one name - 'bitmap directory entry'. The name 'bitmap
header' creates misunderstandings with 'qcow2 header' and 'qcow2 bitmap
header extension' (which is extension of qcow2 header)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
---
 docs/specs/qcow2.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index dda53dd2a3..8874e8c774 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -201,7 +201,7 @@ The fields of the bitmaps extension are:
 
   8 - 15:  bitmap_directory_size
Size of the bitmap directory in bytes. It is the cumulative
-   size of all (nb_bitmaps) bitmap headers.
+   size of all (nb_bitmaps) bitmap directory entries.
 
  16 - 23:  bitmap_directory_offset
Offset into the image file at which the bitmap directory
@@ -426,8 +426,7 @@ Each bitmap saved in the image is described in a bitmap 
directory entry. The
 bitmap directory is a contiguous area in the image file, whose starting offset
 and length are given by the header extension fields bitmap_directory_offset and
 bitmap_directory_size. The entries of the bitmap directory have variable
-length, depending on the lengths of the bitmap name and extra data. These
-entries are also called bitmap headers.
+length, depending on the lengths of the bitmap name and extra data.
 
 Structure of a bitmap directory entry:
 
-- 
2.11.1

[Qemu-devel] [PATCH v16 14/22] block: add bdrv_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

This will be needed to check some restrictions before making bitmap
persistent in qmp-block-dirty-bitmap-add (this functionality will be
added by future patch)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block.c   | 22 ++
 include/block/block.h |  3 +++
 include/block/block_int.h |  5 +
 3 files changed, 30 insertions(+)

diff --git a/block.c b/block.c
index 16cf522219..cf9919a5e0 100644
--- a/block.c
+++ b/block.c
@@ -4093,3 +4093,25 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
BdrvChild *child, Error **errp)
 
 parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
+
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
+{
+BlockDriver *drv = bs->drv;
+
+if (!drv) {
+error_setg_errno(errp, ENOMEDIUM,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+if (!drv->bdrv_can_store_new_dirty_bitmap) {
+error_setg_errno(errp, ENOTSUP,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 4e81f2069b..93718ab245 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -551,4 +551,7 @@ void bdrv_add_child(BlockDriverState *parent, 
BlockDriverState *child,
 Error **errp);
 void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
 
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 2d92d7edfe..f943a7c24e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -320,6 +320,11 @@ struct BlockDriver {
 void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
Error **errp);
 
+bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
+const char *name,
+uint32_t granularity,
+Error **errp);
+
 QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.11.1

[Qemu-devel] [PATCH v16 17/22] qmp: add autoload parameter to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Optional. Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 18 --
 qapi/block-core.json |  6 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 40605faccc..e32ac69e4b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1968,6 +1968,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
+   action->has_autoload, action->autoload,
_err);
 
 if (!local_err) {
@@ -2698,6 +2699,7 @@ out:
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
 bool has_persistent, bool persistent,
+bool has_autoload, bool autoload,
 Error **errp)
 {
 AioContext *aio_context;
@@ -2731,6 +2733,15 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 if (!has_persistent) {
 persistent = false;
 }
+if (!has_autoload) {
+autoload = false;
+}
+
+if (has_autoload && !persistent) {
+error_setg(errp, "Autoload flag must be used only for persistent "
+ "bitmaps");
+goto out;
+}
 
 if (persistent &&
 !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
@@ -2739,10 +2750,13 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 }
 
 bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
-if (bitmap != NULL) {
-bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+if (bitmap == NULL) {
+goto out;
 }
 
+bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
+
  out:
 aio_context_release(aio_context);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 535df20212..09dcf4e6ae 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1564,11 +1564,15 @@
 #  Qcow2 disks support persistent bitmaps. Default is false.
 #  (Since 2.9)
 #
+# @autoload: #optional the bitmap will be automatically loaded when the image
+#it is stored in is opened. This flag may only be specified for
+#persistent bitmaps. Default is false. (Since 2.9)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-'*persistent': 'bool' } }
+'*persistent': 'bool', '*autoload': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add:
-- 
2.11.1

[Qemu-devel] [PATCH v16 16/22] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 18 +-
 qapi/block-core.json |  8 +++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 245e1e1d17..40605faccc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1967,6 +1967,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 /* AIO context taken and released within qmp_block_dirty_bitmap_add */
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
+   action->has_persistent, action->persistent,
_err);
 
 if (!local_err) {
@@ -2696,10 +2697,12 @@ out:
 
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
+bool has_persistent, bool persistent,
 Error **errp)
 {
 AioContext *aio_context;
 BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
 
 if (!name || name[0] == '\0') {
 error_setg(errp, "Bitmap name cannot be empty");
@@ -2725,7 +2728,20 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 granularity = bdrv_get_default_bitmap_granularity(bs);
 }
 
-bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+if (!has_persistent) {
+persistent = false;
+}
+
+if (persistent &&
+!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+{
+goto out;
+}
+
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+if (bitmap != NULL) {
+bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+}
 
  out:
 aio_context_release(aio_context);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 932f5bb3b4..535df20212 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1559,10 +1559,16 @@
 # @granularity: #optional the bitmap granularity, default is 64k for
 #   block-dirty-bitmap-add
 #
+# @persistent: #optional the bitmap is persistent, i.e. it will be saved to the
+#  corresponding block device image file on its close. For now only
+#  Qcow2 disks support persistent bitmaps. Default is false.
+#  (Since 2.9)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
-  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
+'*persistent': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add:
-- 
2.11.1

Re: [Qemu-devel] [PULL 00/24] option cutils: Fix and clean up number conversions

[Peter Maydell]

On 23 February 2017 at 19:53, Markus Armbruster  wrote:
> QemuOpts has its own code to convert strings to numbers, and being
> QemuOpts, it gets it wrong.  util/cutils is less wrong.  Fix it up
> some, and reuse it for QemuOpts.
>
> The following changes since commit 10f25e4844cb9b3f02fb032f88051dd5b65b4206:
>
>   Merge remote-tracking branch 'remotes/yongbok/tags/mips-20170222' into 
> staging (2017-02-23 09:59:40 +)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-util-2017-02-23
>
> for you to fetch changes up to 75cdcd1553e74b5edc58aed23e3b2da8dabb1876:
>
>   option: Fix checking of sizes for overflow and trailing crap (2017-02-23 
> 20:35:36 +0100)
>
> 
> option cutils: Fix and clean up number conversions
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH 09/10] sm501: Add some more missing registers

[Peter Maydell]

On 24 February 2017 at 21:49, BALATON Zoltan  wrote:
> On Fri, 24 Feb 2017, BALATON Zoltan wrote:
>>
>> On Fri, 24 Feb 2017, Peter Maydell wrote:
>>>
>>> On 19 February 2017 at 16:35, BALATON Zoltan  wrote:

 Write only to allow clients to initialise these without failing

 Signed-off-by: BALATON Zoltan 
>>>
>>>
>>> What's the point in write-only register values?

>>> If the registers are writes-ignored, there's no need to store
>>> the data written into the state struct; if the registers are
>>> reads-as-written then implement them that way.
>
>
> Still not sure what do you mean by read-as-written because I think that's
> exactly what is done here, value stored and read back as is, except for
> video_control where there are reserved bits that are always 0.

There are several ways we can make a register behave:

"Read only" -> the guest can only read, it cannot write
"Writes ignored" -> the guest can write but it has no effect
"Reads as written" -> the guest can write, and we store the
data so that when the guest reads it gets back the
value it wrote, but it doesn't have any effect on device behaviour
"Reads as zero" -> the guest can read but it always gets zero
"Write only" -> the guest can write values and we store them
but don't let the guest read them back. This is pointless.

For cases where we don't actually implement some bit of hardware
behaviour yet, it can be a reasonable choice to do either
"read-as-zero/writes-ignored", or "reads as written",
depending on what the guest is expecting. (writes-ignored
is the easiest behaviour to implement, but sometimes guests
won't work if you do that.)

If you're implementing "reads as written" then the commit
message shouldn't say "write only"... But the patch seems
to only add code to the register-write function, not to
the register-read function. That means we're storing
values the guest writes but never doing anything with them.
Either throw away the values immediately ("writes ignored")
or let the guest read them back ("reads as written").

(Optionally, we can also log the access via LOG_UNIMP
to let the user know the guest is trying to use some
bit of the device that doesn't work yet.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 16/21] test-qobject-input-visitor: Use strict visitor

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> The qobject input visitor comes in a strict and a non-strict variant.
> This test is the non-strict variant's last user.

Well, depending on Paolo's proposed addition.

>  Turns out it relies
> on non-strict only in test_visitor_in_null(), and just out of
> laziness.  We don't actually test the non-strict behavior.
> 
> Clean up test_visitor_in_null(), and switch to the strict variant.
> The next commit will drop the non-strict variant.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  tests/test-qobject-input-visitor.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

I'm in favor of unifying the two tests into one and reducing the
duplication (this and the next patch), whether or not we decide that
Paolo's patch means we don't want to eliminate non-strict mode.

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 08/10] sm501: Add support for panel layer

[Peter Maydell]

On 24 February 2017 at 20:38, BALATON Zoltan  wrote:
> On Fri, 24 Feb 2017, Peter Maydell wrote:
>> Please don't change variable names in the middle of a patch that's
>> adding new functionality, it makes the patch harder to review.
>
>
> Where should I do it then? Again another patch?

Yes. Either make it its own patch, or drop the change altogether.
Anything that makes the core "this is making a bug fix or
adding new functionality" patch bigger by adding unnecessary
code change to it makes that patch harder to review.
(Conversely a patch that's just "change this variable name"
is trivially easy to review.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 15/21] qom: Make object_property_set_qobject()'s input visitor strict

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> Commit 240f64b made all qobject input visitors created outside tests
> strict, except for the one in object_property_set_qobject().  That one
> was left behind only because Eric couldn't spare the time to figure
> out whether making it strict would break anything, with a TODO
> comment.  Time to resolve it.

Yay.

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 06/10] sm501: Fix device endianness

[Peter Maydell]

On 24 February 2017 at 20:35, BALATON Zoltan  wrote:
> On Fri, 24 Feb 2017, Peter Maydell wrote:
>>
>> On 19 February 2017 at 16:35, BALATON Zoltan  wrote:
>>>
>>> Signed-off-by: BALATON Zoltan 
>>> ---
>>>  hw/display/sm501.c  |  6 +++---
>>>  hw/display/sm501_template.h | 31 ++-
>>>  2 files changed, 21 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index 9091bb5..3d32a3c 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -846,7 +846,7 @@ static const MemoryRegionOps sm501_system_config_ops
>>> = {
>>>  .min_access_size = 4,
>>>  .max_access_size = 4,
>>>  },
>>> -.endianness = DEVICE_NATIVE_ENDIAN,
>>> +.endianness = DEVICE_LITTLE_ENDIAN,
>>>  };
>>>
>>>  static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
>>> @@ -1082,7 +1082,7 @@ static const MemoryRegionOps sm501_disp_ctrl_ops =
>>> {
>>>  .min_access_size = 4,
>>>  .max_access_size = 4,
>>>  },
>>> -.endianness = DEVICE_NATIVE_ENDIAN,
>>> +.endianness = DEVICE_LITTLE_ENDIAN,
>>>  };
>>>
>>>  static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
>>> @@ -1170,7 +1170,7 @@ static const MemoryRegionOps sm501_2d_engine_ops =
>>> {
>>>  .min_access_size = 4,
>>>  .max_access_size = 4,
>>>  },
>>> -.endianness = DEVICE_NATIVE_ENDIAN,
>>> +.endianness = DEVICE_LITTLE_ENDIAN,
>>>  };
>>
>>
>> Does this still all work for the sh4eb (big-endian) sysbus device case?
>
>
> Not sure. Is there some image somewhere I can test with?

I don't know, I'm afraid; I don't know anything about sh4.
I'm just going by looking at the code changes and flagging
up things which are potentially problems.

>>>  /* draw line functions for all console modes */
>>> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
>>> index 832ee61..5b516d6 100644
>>> --- a/hw/display/sm501_template.h
>>> +++ b/hw/display/sm501_template.h
>>> @@ -64,10 +64,16 @@ static void glue(draw_line16_, PIXEL_NAME)(
>>>  uint8_t r, g, b;
>>>
>>>  do {
>>> -rgb565 = lduw_p(s);
>>> -r = ((rgb565 >> 11) & 0x1f) << 3;
>>> -g = ((rgb565 >>  5) & 0x3f) << 2;
>>> -b = ((rgb565 >>  0) & 0x1f) << 3;
>>> +rgb565 = lduw_le_p(s);
>>> +#if defined(TARGET_WORDS_BIGENDIAN)
>>> +r = (rgb565 >> 8) & 0xf8;
>>> +g = (rgb565 >> 3) & 0xfc;
>>> +b = (rgb565 << 3) & 0xf8;
>>> +#else
>>> +b = (rgb565 >> 8) & 0xf8;
>>> +g = (rgb565 >> 3) & 0xfc;
>>> +r = (rgb565 << 3) & 0xf8;
>>> +#endif
>>>  *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>>>  s += 2;
>>>  d += BPP;
>>> @@ -80,15 +86,14 @@ static void glue(draw_line32_, PIXEL_NAME)(
>>>  uint8_t r, g, b;
>>>
>>>  do {
>>> -ldub_p(s);
>>>  #if defined(TARGET_WORDS_BIGENDIAN)
>>> +r = s[0];
>>> +g = s[1];
>>> +b = s[2];
>>> +#else
>>>  r = s[1];
>>>  g = s[2];
>>>  b = s[3];
>>> -#else
>>> -b = s[0];
>>> -g = s[1];
>>> -r = s[2];
>>>  #endif
>>
>>
>> This looks really suspicious. Previously we had
>> TARGET_WORDS_BIGENDIAN -> bytes are XRGB
>> otherwise bytes are BGRX
>>
>> Now we have
>> TARGET_WORDS_BIGENDIAN -> bytes are RGBX
>> otherwise  -> bytes are XRGB
>>
>> That doesn't seem very plausible at all.
>
>
> I've tested it with sh4 and ppc guests running on x86_64 host and these work
> now while previous code resulted in mixed up colors. Maybe the host
> endianness could also be a factor and the previous code assumed big endian
> host or the previous code was already broken and only worked with the
> default 8 bit depth. I'm not completely sure I understand endian handling in
> QEMU to know if this is correct besides on what I've tested but at least
> little endian and big endian guests should work on little endian hosts now
> with my patch. I can't test on big endian host.
>
> I've used this image: https://people.debian.org/~aurel32/qemu/sh4/
> with video=800x600-16 kernel parameter where changing -16 to different bit
> depths reproduces the problem.

Again, I don't know sh, and I don't know this particular device.
I'm just pointing out that the code change here looks really
implausible. Maybe the data sheet says that this is what the
pixel format looks like; but it's very odd.

>>>  *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>>>  s += 4;
>>
>>
>>
>>> @@ -103,7 +108,7 @@ static void glue(draw_hwc_line_,
>>> PIXEL_NAME)(SM501State *s, int crt,
>>>   uint8_t *palette, int c_y, uint8_t *d, int width)
>>>  {
>>>  int x, i;
>>> -uint8_t *pixval, bitset = 0;
>>> +uint8_t *pixval, r, g, b, bitset = 0;
>>>
>>>  /* get hardware cursor pattern */
>>>  uint32_t cursor_addr = get_hwc_address(s, 

Re: [Qemu-devel] [PATCH 04/10] sm501: Add emulation of chip connected via PCI

[Peter Maydell]

On 24 February 2017 at 20:25, BALATON Zoltan  wrote:
> On Fri, 24 Feb 2017, Peter Maydell wrote:
>> Dropping the requirement for a base addr is not really the same
>> change as adding the PCI device.
>
>
> This is needed for PCI device because the base is not accessible before
> something maps the BARs of the device so I had to use something else that is
> known. So this change is part of making it a PCI device but I could make
> this a separate patch if you think that's better. Should I split it off or
> can leave it here?

I would split it off.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/10] sm501: QOMify

[Peter Maydell]

On 24 February 2017 at 20:23, BALATON Zoltan  wrote:
> On Fri, 24 Feb 2017, Peter Maydell wrote:
>>
>> On 19 February 2017 at 16:35, BALATON Zoltan  wrote:
>>>
>>> Signed-off-by: BALATON Zoltan 
>>> ---
>>>  hw/display/sm501.c   | 133
>>> +++
>>>  hw/sh4/r2d.c |  11 -
>>>  include/hw/devices.h |   5 --
>>>  3 files changed, 101 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index 4eb085c..b592022 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -58,8 +58,8 @@
>>>  #define SM501_DPRINTF(fmt, ...) do {} while (0)
>>>  #endif
>>>
>>> -
>>>  #define MMIO_BASE_OFFSET 0x3e0
>>> +#define MMIO_SIZE 0x20
>>>
>>>  /* SM501 register definitions taken from
>>> "linux/include/linux/sm501-regs.h" */
>>>
>>> @@ -464,6 +464,7 @@ typedef struct SM501State {
>>>  uint32_t local_mem_size_index;
>>>  uint8_t *local_mem;
>>>  MemoryRegion local_mem_region;
>>> +MemoryRegion mmio_region;
>>>  uint32_t last_width;
>>>  uint32_t last_height;
>>>
>>> @@ -1396,20 +1397,14 @@ static const GraphicHwOps sm501_ops = {
>>>  .gfx_update  = sm501_update_display,
>>>  };
>>>
>>> -void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
>>> -uint32_t local_mem_bytes, qemu_irq irq, Chardev *chr)
>>> +static void sm501_init(SM501State *s, DeviceState *dev, uint32_t base,
>>> +   uint32_t local_mem_bytes)
>>>  {
>>> -SM501State *s;
>>> -DeviceState *dev;
>>> -MemoryRegion *sm501_system_config = g_new(MemoryRegion, 1);
>>> -MemoryRegion *sm501_disp_ctrl = g_new(MemoryRegion, 1);
>>> -MemoryRegion *sm501_2d_engine = g_new(MemoryRegion, 1);
>>> -
>>> -/* allocate management data region */
>>> -s = g_new0(SM501State, 1);
>>> +MemoryRegion *mr;
>>> +
>>>  s->base = base;
>>>  s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
>>> -SM501_DPRINTF("local mem size=%x. index=%d\n",
>>> get_local_mem_size(s),
>>> +SM501_DPRINTF("sm501 local mem size=%x. index=%d\n",
>>> get_local_mem_size(s),
>>>s->local_mem_size_index);
>>>  s->system_control = 0x0010; /* 2D engine FIFO empty */
>>>  s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low
>>> */
>>> @@ -1417,46 +1412,102 @@ void sm501_init(MemoryRegion *address_space_mem,
>>> uint32_t base,
>>>  s->dc_crt_control = 0x0001;
>>>
>>>  /* allocate local memory */
>>> -memory_region_init_ram(>local_mem_region, NULL, "sm501.local",
>>> +memory_region_init_ram(>local_mem_region, OBJECT(dev),
>>> "sm501.local",
>>> local_mem_bytes, _fatal);
>>>  vmstate_register_ram_global(>local_mem_region);
>>>  memory_region_set_log(>local_mem_region, true, DIRTY_MEMORY_VGA);
>>>  s->local_mem = memory_region_get_ram_ptr(>local_mem_region);
>>> -memory_region_add_subregion(address_space_mem, base,
>>> >local_mem_region);
>>> -
>>> -/* map mmio */
>>> -memory_region_init_io(sm501_system_config, NULL,
>>> _system_config_ops,
>>> -  s, "sm501-system-config", 0x6c);
>>> -memory_region_add_subregion(address_space_mem, base +
>>> MMIO_BASE_OFFSET,
>>> -sm501_system_config);
>>> -memory_region_init_io(sm501_disp_ctrl, NULL, _disp_ctrl_ops,
>>> s,
>>> +
>>> +/* allocate mmio */
>>> +memory_region_init(>mmio_region, OBJECT(dev), "sm501.mmio",
>>> MMIO_SIZE);
>>> +mr = g_new(MemoryRegion, 1);
>>
>>
>> There's no need to dynamically allocate any of these memory regions;
>> just make them be MemoryRegion fields inside SM501State.
>>
>>> +memory_region_init_io(mr, OBJECT(dev), _system_config_ops, s,
>>> +  "sm501-system-config", 0x6c);
>>> +memory_region_add_subregion(>mmio_region, SM501_SYS_CONFIG, mr);
>>> +mr = g_new(MemoryRegion, 1);
>>> +memory_region_init_io(mr, OBJECT(dev), _disp_ctrl_ops, s,
>>>"sm501-disp-ctrl", 0x1000);
>>> -memory_region_add_subregion(address_space_mem,
>>> -base + MMIO_BASE_OFFSET + SM501_DC,
>>> -sm501_disp_ctrl);
>>> -memory_region_init_io(sm501_2d_engine, NULL, _2d_engine_ops,
>>> s,
>>> +memory_region_add_subregion(>mmio_region, SM501_DC, mr);
>>> +mr = g_new(MemoryRegion, 1);
>>> +memory_region_init_io(mr, OBJECT(dev), _2d_engine_ops, s,
>>>"sm501-2d-engine", 0x54);
>>> -memory_region_add_subregion(address_space_mem,
>>> -base + MMIO_BASE_OFFSET +
>>> SM501_2D_ENGINE,
>>> -sm501_2d_engine);
>>> +memory_region_add_subregion(>mmio_region, SM501_2D_ENGINE, mr);
>>> +
>>> +/* create qemu graphic console */
>>> +s->con = 

Re: [Qemu-devel] [PATCH 14/21] qapi: Make string input and opts visitor require non-null input

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> The string input visitor tries to cope with null input.  Null input
> isn't used anywhere, and isn't covered by tests.  Unsurprisingly, it
> doesn't fully work: start_list() crashes because it passes the input
> via parse_str() to strtoll() unchecked.
> 
> Make string_input_visitor_new() assert its argument isn't null, and
> drop the code trying to deal with null input.
> 
> The opts visitor crashes when you try to actually visit something with
> null input.  Make opts_visitor_new() assert its argument isn't null,
> mostly for clarity.
> 
> qobject_input_visitor_new() already asserts its argument isn't null.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/opts-visitor.c |  1 +
>  qapi/string-input-visitor.c | 54 
> ++---
>  2 files changed, 18 insertions(+), 37 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 13/21] qapi: Drop string input visitor method optional()

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> visit_optional() is to be called only between visit_start_struct() and
> visit_end_struct().  Visitors that don't support struct visits,
> i.e. don't implement start_struct(), end_struct(), have no use for it.
> Clarify documentation.
> 
> The string input visitor doesn't support struct visits.  Its
> parse_optional() is therefore useless.  Drop it.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/visitor-impl.h |  4 ++--
>  qapi/string-input-visitor.c | 13 -
>  2 files changed, 2 insertions(+), 15 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 v15 21/25] qcow2-bitmap: refcounts

[Vladimir Sementsov-Ogievskiy]

16.02.2017 17:27, Kevin Wolf wrote:

Am 15.02.2017 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben:

Calculate refcounts for qcow2 bitmaps. It is needed for qcow2's qemu-img
check implementation.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 

Maybe this should come earlier in the series so that qemu-img check
doesn't corrupt them. Basically it's needed as soon as we add the
autoclear flag as supported.


so, it should be merged into 07



Anyway, the actual point I was going to make is that it would be good to
have a qemu-iotests case which actually invokes qemu-img check on an
image with dirty bitmaps. In general, the test case side of the series
seems to be rather weak so far. Just keep in mind that I won't feel bad
for breaking anything that isn't tested by a test case. So if you like
to keep persistent bitmaps working in the long run, you'd better write
tests and give me no chance to break anything without them failing.

Kevin



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 12/21] qapi: Improve qobject input visitor error reporting

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> Error messages refer to nodes of the QObject being visited by name.
> Trouble is the names are sometimes less than helpful:
> 

> Improve error messages by referring to nodes by path instead, as
> follows:
> 
> * The path of the root QObject is whatever @name argument got passed
>   to the visitor, except NULL gets mapped to "".
> 
> * The path of a root QDict's member is the member key.
> 
> * The path of a root QList's member is "[%zu]", where %zu is the list
>   index, starting at zero.
> 
> * The path of a non-root QDict's member is the path of the QDict
>   concatenated with "." and the member key.
> 
> * The path of a non-root QList's member is the path of the QList
>   concatenated with "[%zu]", where %zu is the list index.
> 

> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/visitor.h   |   6 ---
>  qapi/qobject-input-visitor.c | 121 
> +++
>  2 files changed, 87 insertions(+), 40 deletions(-)
> 

> +typedef struct StackObject {
> +const char *name;/* Name of @obj in its parent, if any */
> +QObject *obj;/* QDict or QList being visited */
>  void *qapi; /* sanity check that caller uses same pointer */
>  
> -GHashTable *h;   /* If obj is dict: unvisited keys */
> -const QListEntry *entry; /* If obj is list: unvisited tail */
> +GHashTable *h;  /* If @obj is QDict: unvisited keys */
> +const QListEntry *entry;/* If @obj is QList: unvisited tail */
> +unsigned index; /* If @obj is QList: list index of @entry */

Could perhaps make the QDict vs. QList fields shared through a union if
we were tight on storage space or cache line pressure. I don't think
that's the case, though, so no need to change it.

> +static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> +{
> +StackObject *so;
> +char buf[32];
> +
> +if (qiv->errname) {
> +g_string_truncate(qiv->errname, 0);
> +} else {
> +qiv->errname = g_string_new("");
> +}
> +
> +QSLIST_FOREACH(so , >stack, node) {
> +if (qobject_type(so->obj) == QTYPE_QDICT) {
> +g_string_prepend(qiv->errname, name);
> +g_string_prepend_c(qiv->errname, '.');
> +} else {
> +snprintf(buf, sizeof(buf), "[%u]", so->index);
> +g_string_prepend(qiv->errname, buf);
> +}
> +name = so->name;
> +}

Building the string from back to front requires quite a bit of memory
reshuffling, as well as the temporary buffer for integer formatting. Is
there any way to build it from front to back? But this code is only
triggered on error paths, so I don't care if it is slow.  I'm fine with
what you have.

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 11/21] qapi: Make QObject input visitor set *list reliably

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> qobject_input_start_struct() sets *list, except when it fails because
> qobject_input_get_object() fails, i.e. the input object doesn't exist.
> 
> All the other input visitor start_struct(), start_list(),
> start_alternate() always set *obj / *list.
> 
> Change qobject_input_start_struct() to match.

I thought I'd caught all those. Thanks for the cleanup.

> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/qobject-input-visitor.c | 14 +-
>  1 file changed, 5 insertions(+), 9 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 54/54] block: Add Error parameter to bdrv_append()

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Aborting on error in bdrv_append() isn't correct. This patch fixes it
> and lets the callers handle failures.
> 
> Test case 085 needs a reference output update. This is caused by the
> reversed order of bdrv_set_backing_hd() and change_parent_backing_link()
> in bdrv_append(): When the backing file of the new node is set, the
> parent nodes are still pointing to the old top, so the backing blocker
> is now initialised with the node name rather than the BlockBackend name.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c| 23 +--
>  block/mirror.c |  9 -
>  blockdev.c | 15 +--
>  include/block/block.h  |  3 ++-
>  tests/qemu-iotests/085.out |  2 +-
>  5 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b3f03a4..33edbda 100644
> --- a/block.c
> +++ b/block.c
> @@ -2060,6 +2060,7 @@ static BlockDriverState 
> *bdrv_append_temp_snapshot(BlockDriverState *bs,
>  int64_t total_size;
>  QemuOpts *opts = NULL;
>  BlockDriverState *bs_snapshot;
> +Error *local_err = NULL;
>  int ret;
>  
>  /* if snapshot, we create a temporary backing file and open it
> @@ -2109,7 +2110,12 @@ static BlockDriverState 
> *bdrv_append_temp_snapshot(BlockDriverState *bs,
>   * call bdrv_unref() on it), so in order to be able to return one, we 
> have
>   * to increase bs_snapshot's refcount here */
>  bdrv_ref(bs_snapshot);
> -bdrv_append(bs_snapshot, bs);
> +bdrv_append(bs_snapshot, bs, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +goto out;
> +}
>  
>  g_free(tmp_filename);
>  return bs_snapshot;
> @@ -2900,20 +2906,25 @@ static void 
> change_parent_backing_link(BlockDriverState *from,
>   * parents of bs_top after bdrv_append() returns. If the caller needs to 
> keep a
>   * reference of its own, it must call bdrv_ref().
>   */
> -void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> + Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  assert(!atomic_read(_top->in_flight));
>  assert(!atomic_read(_new->in_flight));
>  
> -bdrv_ref(bs_top);
> +bdrv_set_backing_hd(bs_new, bs_top, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto out;
> +}
>  
>  change_parent_backing_link(bs_top, bs_new);
> -/* FIXME Error handling */
> -bdrv_set_backing_hd(bs_new, bs_top, _abort);
> -bdrv_unref(bs_top);
>  
>  /* bs_new is now referenced by its new parents, we don't need the
>   * additional reference any more. */
> +out:
>  bdrv_unref(bs_new);
>  }
>  
> diff --git a/block/mirror.c b/block/mirror.c
> index abbd847..4e35d85 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1066,6 +1066,7 @@ static void mirror_start_job(const char *job_id, 
> BlockDriverState *bs,
>  BlockDriverState *mirror_top_bs;
>  bool target_graph_mod;
>  bool target_is_backing;
> +Error *local_err = NULL;
>  int ret;
>  
>  if (granularity == 0) {
> @@ -1096,9 +1097,15 @@ static void mirror_start_job(const char *job_id, 
> BlockDriverState *bs,
>   * it alive until block_job_create() even if bs has no parent. */
>  bdrv_ref(mirror_top_bs);
>  bdrv_drained_begin(bs);
> -bdrv_append(mirror_top_bs, bs);
> +bdrv_append(mirror_top_bs, bs, _err);
>  bdrv_drained_end(bs);
>  
> +if (local_err) {
> +bdrv_unref(mirror_top_bs);
> +error_propagate(errp, local_err);
> +return;
> +}
> +
>  /* Make sure that the source is not resized while the job is running */
>  s = block_job_create(job_id, driver, mirror_top_bs,
>   BLK_PERM_CONSISTENT_READ,
> diff --git a/blockdev.c b/blockdev.c
> index 63b1ac4..580fa66 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1764,6 +1764,16 @@ static void external_snapshot_prepare(BlkActionState 
> *common,
>  
>  if (!state->new_bs->drv->supports_backing) {
>  error_setg(errp, "The snapshot does not support backing images");
> +return;
> +}
> +
> +/* This removes our old bs and adds the new bs. This is an operation that
> + * can fail, so we need to do it in .prepare; undoing it for abort is
> + * always possible. */
> +bdrv_append(state->new_bs, state->old_bs, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
>  }

After bdrv_append(), the state->new_bs reference is no longer valid,
necessarily; especially if the operation failed.

>  }
>  
> @@ -1774,8 +1784,6 @@ static void external_snapshot_commit(BlkActionState 
> *common)
>  
>  bdrv_set_aio_context(state->new_bs, state->aio_context);
>  
> -/* This removes our old bs and adds 

Re: [Qemu-devel] [PATCH 10/21] qapi: Clean up after commit 3d344c2

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> Drop unused QIV_STACK_SIZE and unused qobject_input_start_struct()
> parameter errp.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/qobject-input-visitor.c | 14 +++---
>  1 file changed, 3 insertions(+), 11 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 09/21] qapi: Improve a QObject input visitor error message

[Eric Blake]

On 02/23/2017 03:45 PM, Markus Armbruster wrote:
> The QObject input visitor has three error message formats:
> 
> * Parameter '%s' is missing
> * "Invalid parameter type for '%s', expected: %s"
> * "QMP input object member '%s' is unexpected"
> 
> The '%s' are member names (or "null", but I'll fix that later).
> 
> The last error message calls the thing "QMP input object member"
> instead of "parameter".  Misleading when the visitor is used on
> QObjects that don't come from QMP.  Change it to "Parameter '%s' is
> unexpected".
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/qobject-input-visitor.c | 3 +--
>  tests/test-qga.c | 2 +-
>  2 files changed, 2 insertions(+), 3 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 53/54] block: Add Error parameter to bdrv_set_backing_hd()

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Not all callers of bdrv_set_backing_hd() know for sure that attaching
> the backing file will be allowed by the permission system. Return the
> error from the function rather than aborting.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 27 ---
>  block/commit.c| 14 +++---
>  block/mirror.c| 16 +++-
>  block/stream.c|  9 -
>  block/vvfat.c |  2 +-
>  include/block/block.h |  3 ++-
>  6 files changed, 53 insertions(+), 18 deletions(-)

It's a bit unfortunate that bdrv_set_backing_hd() is not atomic, that
is, that the node will no longer have a backing file if the command
fails. Oh well.

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 52/54] block: Assertions for resize permission

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> This adds an assertion that ensures that the necessary resize permission
> has been granted before bdrv_truncate() is called.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 51/54] block: Assertions for write permissions

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> This adds assertions that ensure that the necessary write permissions
> have been granted before someone attempts to write to a node.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 63e4400..4c79745 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -925,9 +925,11 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, 
> uint64_t offset,
>  return drv->bdrv_co_pwritev_compressed(bs, offset, bytes, qiov);
>  }
>  
> -static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
> +static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>  int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
>  {
> +BlockDriverState *bs = child->bs;
> +
>  /* Perform I/O through a temporary buffer so that users who scribble over
>   * their read buffer while the operation is in progress do not end up
>   * modifying the image file.  This is critical for zero-copy guest I/O

As I said, I think this hunk needs to be in the previous patch.

Max

> @@ -943,6 +945,8 @@ static int coroutine_fn 
> bdrv_co_do_copy_on_readv(BlockDriverState *bs,
>  size_t skip_bytes;
>  int ret;
>  
> +assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
> +
>  /* Cover entire cluster so no additional backing file I/O is required 
> when
>   * allocating cluster in the image file.
>   */
> @@ -1051,7 +1055,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
> *child,
>  }
>  
>  if (!ret || pnum != nb_sectors) {
> -ret = bdrv_co_do_copy_on_readv(bs, offset, bytes, qiov);
> +ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
>  goto out;
>  }
>  }
> @@ -1334,6 +1338,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
> *child,
>  assert(!waited || !req->serialising);
>  assert(req->overlap_offset <= offset);
>  assert(offset + bytes <= req->overlap_offset + req->overlap_bytes);
> +assert(child->perm & BLK_PERM_WRITE);
>  
>  ret = notifier_with_return_list_notify(>before_write_notifiers, req);
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 50/54] block: Pass BdrvChild to bdrv_aligned_preadv/pwritev

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> This is where we want to check the permissions, so we need to have the
> BdrvChild around where they are stored.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c | 31 +--
>  1 file changed, 17 insertions(+), 14 deletions(-)

I was about to inquire about bdrv_co_do_pwrite_zeroes() which is called
by bdrv_co_do_copy_on_readv(), but then I saw that you make the latter
take a BdrvChild in the next patch. OK, but I think that change needs to
be here.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 49/54] tests: Remove FIXME comments

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Not requesting any permissions is actually correct for these test cases
> because no actual I/O or other operation covered by the permission
> system is performed.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/test-blockjob.c | 2 +-
>  tests/test-throttle.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 48/54] nbd/server: Use real permissions for NBD exports

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> NBD can't cope with device size changes, so resize must be forbidden,
> but otherwise we can tolerate anything. Depending on whether the export
> is writable or not, we only require consistent reads and writes.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  nbd/server.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 47/54] migration/block: Use real permissions

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Request BLK_PERM_CONSISTENT_READ for the source of block migration, and
> handle potential permission errors as good as we can in this place
> (which is not very good, but it matches the other failure cases).
> 
> Signed-off-by: Kevin Wolf 
> ---
>  migration/block.c | 21 -
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index d259936..958d0fc 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -379,7 +379,7 @@ static void unset_dirty_tracking(void)
>  }
>  }
>  
> -static void init_blk_migration(QEMUFile *f)
> +static int init_blk_migration(QEMUFile *f)
>  {
>  BlockDriverState *bs;
>  BlkMigDevState *bmds;
> @@ -390,6 +390,8 @@ static void init_blk_migration(QEMUFile *f)
>  BlkMigDevState *bmds;
>  BlockDriverState *bs;
>  } *bmds_bs;
> +Error *local_err = NULL;
> +int ret;
>  
>  block_mig_state.submitted = 0;
>  block_mig_state.read_done = 0;
> @@ -411,12 +413,12 @@ static void init_blk_migration(QEMUFile *f)
>  
>  sectors = bdrv_nb_sectors(bs);
>  if (sectors <= 0) {
> +ret = sectors;
>  goto out;
>  }
>  
>  bmds = g_new0(BlkMigDevState, 1);
> -/* FIXME Use real permissions */
> -bmds->blk = blk_new(0, BLK_PERM_ALL);
> +bmds->blk = blk_new(BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL);
>  bmds->blk_name = g_strdup(bdrv_get_device_name(bs));
>  bmds->bulk_completed = 0;
>  bmds->total_sectors = sectors;
> @@ -446,7 +448,11 @@ static void init_blk_migration(QEMUFile *f)
>  BlockDriverState *bs = bmds_bs[i].bs;
>  
>  if (bmds) {
> -blk_insert_bs(bmds->blk, bs, _abort);
> +ret = blk_insert_bs(bmds->blk, bs, _err);
> +if (ret < 0) {
> +error_report_err(local_err);
> +goto out;
> +}
>  
>  alloc_aio_bitmap(bmds);
>  error_setg(>blocker, "block device is in use by 
> migration");
> @@ -454,8 +460,10 @@ static void init_blk_migration(QEMUFile *f)
>  }
>  }
>  
> +ret = 0;
>  out:
>  g_free(bmds_bs);
> +return ret;
>  }
>  
>  /* Called with no lock taken.  */
> @@ -706,7 +714,10 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>  block_mig_state.submitted, block_mig_state.transferred);
>  
>  qemu_mutex_lock_iothread();
> -init_blk_migration(f);
> +ret = init_blk_migration(f);
> +if (ret < 0) {

What about qemu_mutex_unlock_iothread()?

Max

> +return ret;
> +}
>  
>  /* start track dirty blocks */
>  ret = set_dirty_tracking();
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 46/54] hmp: Request permissions in qemu-io

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> The HMP command 'qemu-io' is a bit tricky because it wants to work on
> the original BlockBackend, but additional permissions could be required.
> The details are explained in a comment in the code, but in summary, just
> request whatever permissions the current qemu-io command needs.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/block-backend.c  |  6 ++
>  hmp.c  | 26 +-
>  include/qemu-io.h  |  1 +
>  include/sysemu/block-backend.h |  1 +
>  qemu-io-cmds.c | 28 
>  5 files changed, 61 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 45/54] commit: Add filter-node-name to block-commit

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Management tools need to be able to know about every node in the graph
> and need a way to address them. This new option to block-commit allows
> the client to set a node-name for the automatically inserted filter
> driver, and at the same time serves as a witness that this version of
> qemu does automatically insert a filter driver.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/commit.c|  6 +++---
>  block/mirror.c|  3 ++-
>  block/replication.c   |  2 +-
>  blockdev.c| 10 +++---
>  include/block/block_int.h | 13 ++---
>  qapi/block-core.json  |  8 +++-
>  qemu-img.c|  4 ++--
>  7 files changed, 32 insertions(+), 14 deletions(-)

[...]

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 563b30c..a57c0bf 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -780,13 +780,16 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>   * @on_error: The action to take upon error.
>   * @backing_file_str: String to use as the backing file in @top's overlay
> + * @filter_node_name: The node name that should be assigned to the filter
> + * driver that the commit job inserts into the graph above @top. NULL means
> + * that a node name should be autogenerated.
>   * @errp: Error object.
>   *
>   */
>  void commit_start(const char *job_id, BlockDriverState *bs,
>BlockDriverState *base, BlockDriverState *top, int64_t 
> speed,
>BlockdevOnError on_error, const char *backing_file_str,
> -  Error **errp);
> +  const char *filter_node_name, Error **errp);
>  /**
>   * commit_active_start:
>   * @job_id: The id of the newly-created job, or %NULL to use the
> @@ -797,6 +800,9 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>   *  See @BlockJobCreateFlags
>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>   * @on_error: The action to take upon error.
> + * @filter_node_name: The node name that should be assigned to the filter
> + * driver that the commit job inserts into the graph above @bs. NULL means 
> that

Maybe s/commit/mirror/, but just maybe.

> + * a node name should be autogenerated.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
>   * @errp: Error object.
> @@ -806,8 +812,9 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>  void commit_active_start(const char *job_id, BlockDriverState *bs,
>   BlockDriverState *base, int creation_flags,
>   int64_t speed, BlockdevOnError on_error,
> - BlockCompletionFunc *cb,
> - void *opaque, Error **errp, bool auto_complete);
> + const char *filter_node_name,
> + BlockCompletionFunc *cb, void *opaque, Error **errp,
> + bool auto_complete);
>  /*
>   * mirror_start:
>   * @job_id: The id of the newly-created job, or %NULL to use the
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 893fa34..36d38b9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1304,6 +1304,11 @@
>  #
>  # @speed:  #optional the maximum speed, in bytes per second
>  #
> +# @filter-node-name: #optional the node name that should be assigned to the
> +#filter driver that the commit job inserts into the graph
> +#above @device. If this option is not given, a node name 
> is

Above @top, actually.

> +#autogenerated. (Since: 2.9)
> +#

I'm much less enthusiastic about this than about the previous patch.
This is because this node appears to me just to be some kind of a hack
to silence op blockers and it doesn't seem obvious where to go with this.

For mirror, it's obvious. In the future, the mirror filter will get the
target as a second child and then it will actually perform all of the
mirror work and indeed be something useful.

For commit, no such immediate plans exist, as far as I'm aware. Well,
for active commit it's just the same, of course. Maybe we can model
non-active commit in the same way: Place it above @top, keep everything
writable and then add @base as a second child. Then it would actually
make sense and deserve a user-specifiable node name.

OK, now that I actually see a way to make use of the new node, this
patch suddenly looks better to me.

So with s/@device/@top/:

Reviewed-by: Max Reitz 

>  # Returns: Nothing on success
>  #  If commit or stream is already active on this device, DeviceInUse
>  #  If @device does not exist, DeviceNotFound
> @@ -1323,7 +1328,8 @@
>  ##
>  { 'command': 'block-commit',
>'data': { '*job-id': 'str', 

Re: [Qemu-devel] [PATCH 44/54] mirror: Add filter-node-name to blockdev-mirror

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Management tools need to be able to know about every node in the graph
> and need a way to address them. This new option to blockdev-mirror
> allows the client to set a node-name for the automatically inserted
> filter driver, and at the same time serves as a witness that this
> version of qemu does automatically insert a filter driver.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/mirror.c| 14 --
>  blockdev.c| 12 +++-
>  include/block/block_int.h |  5 -
>  qapi/block-core.json  |  8 +++-
>  4 files changed, 30 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz 

I'm not so happy about "...serves as a witness..." because that doesn't
make it less of an incompatible change.

However, since (as you know) I had plans to introduce such a filter node
myself anyway, I'll gladly let you take all the git-blame if something
breaks.

(And it should be easy for me to add the mirror target as another child
to the filter node.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 43/54] stream: Use real permissions in streaming block job

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> The correct permissions are relatively obvious here (and explained in
> code comments). For intermediate streaming, we need to reopen the top
> node read-write before creating the job now because the permissions
> system catches attempts to get the BLK_PERM_WRITE_UNCHANGED permission
> on a read-only node.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/stream.c | 37 +
>  1 file changed, 25 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 42/54] mirror: Use real permissions in mirror/active commit block job

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> The mirror block job is mainly used for two different scenarios:
> Mirroring to an otherwise unused, independent target node, or for active
> commit where the target node is part of the backing chain of the source.
> 
> Similarly to the commit block job patch, we need to insert a new filter
> node to keep the permissions correct during active commit.
> 
> Note that one change this implies is that job->blk points to
> mirror_top_bs as its root now, and mirror_top_bs (rather than the actual
> source node) contains the bs->job pointer. This requires qemu-img commit
> to get the job by name now rather than just taking bs->job.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/mirror.c | 161 
> +
>  qemu-img.c |   6 +-
>  tests/qemu-iotests/141 |   2 +-
>  tests/qemu-iotests/141.out |   4 +-
>  4 files changed, 143 insertions(+), 30 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 767b7e7..252107d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -38,7 +38,10 @@ typedef struct MirrorBlockJob {
>  BlockJob common;
>  RateLimit limit;
>  BlockBackend *target;
> +BlockDriverState *mirror_top_bs;
> +BlockDriverState *source;
>  BlockDriverState *base;
> +
>  /* The name of the graph node to replace */
>  char *replaces;
>  /* The BDS to replace */
> @@ -325,7 +328,7 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
>  
>  static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>  {
> -BlockDriverState *source = blk_bs(s->common.blk);
> +BlockDriverState *source = s->source;
>  int64_t sector_num, first_chunk;
>  uint64_t delay_ns = 0;
>  /* At least the first dirty chunk is mirrored in one iteration. */
> @@ -495,12 +498,14 @@ static void mirror_exit(BlockJob *job, void *opaque)
>  MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>  MirrorExitData *data = opaque;
>  AioContext *replace_aio_context = NULL;
> -BlockDriverState *src = blk_bs(s->common.blk);
> +BlockDriverState *src = s->source;
>  BlockDriverState *target_bs = blk_bs(s->target);
> +BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>  
>  /* Make sure that the source BDS doesn't go away before we called
>   * block_job_completed(). */
>  bdrv_ref(src);
> +bdrv_ref(mirror_top_bs);
>  
>  if (s->to_replace) {
>  replace_aio_context = bdrv_get_aio_context(s->to_replace);
> @@ -522,13 +527,6 @@ static void mirror_exit(BlockJob *job, void *opaque)
>  bdrv_drained_begin(target_bs);
>  bdrv_replace_in_backing_chain(to_replace, target_bs);
>  bdrv_drained_end(target_bs);
> -
> -/* We just changed the BDS the job BB refers to, so switch the BB 
> back
> - * so the cleanup does the right thing. We don't need any permissions
> - * any more now. */
> -blk_remove_bs(job->blk);
> -blk_set_perm(job->blk, 0, BLK_PERM_ALL, _abort);
> -blk_insert_bs(job->blk, src, _abort);
>  }
>  if (s->to_replace) {
>  bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
> @@ -541,9 +539,23 @@ static void mirror_exit(BlockJob *job, void *opaque)
>  g_free(s->replaces);
>  blk_unref(s->target);
>  s->target = NULL;
> +
> +/* Remove the mirror filter driver from the graph */
> +bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
> +
> +/* We just changed the BDS the job BB refers to (with either or both of 
> the
> + * bdrv_replace_in_backing_chain() calls), so switch the BB back so the
> + * cleanup does the right thing. We don't need any permissions any more
> + * now. */
> +blk_remove_bs(job->blk);
> +blk_set_perm(job->blk, 0, BLK_PERM_ALL, _abort);
> +blk_insert_bs(job->blk, mirror_top_bs, _abort);
> +
>  block_job_completed(>common, data->ret);
> +
>  g_free(data);
>  bdrv_drained_end(src);
> +bdrv_unref(mirror_top_bs);
>  bdrv_unref(src);
>  }
>  
> @@ -563,7 +575,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob 
> *s)
>  {
>  int64_t sector_num, end;
>  BlockDriverState *base = s->base;
> -BlockDriverState *bs = blk_bs(s->common.blk);
> +BlockDriverState *bs = s->source;
>  BlockDriverState *target_bs = blk_bs(s->target);
>  int ret, n;
>  
> @@ -642,7 +654,7 @@ static void coroutine_fn mirror_run(void *opaque)
>  {
>  MirrorBlockJob *s = opaque;
>  MirrorExitData *data;
> -BlockDriverState *bs = blk_bs(s->common.blk);
> +BlockDriverState *bs = s->source;
>  BlockDriverState *target_bs = blk_bs(s->target);
>  bool need_drain = true;
>  int64_t length;
> @@ -876,7 +888,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
>  MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>  BlockDriverState *src, *target;

Re: [Qemu-devel] [PATCH 41/54] block: Allow backing file links in change_parent_backing_link()

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Now that the backing file child role implements .attach/.detach
> callbacks, nothing prevents us from modifying the graph even if that
> involves changing backing file links.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 40/54] block: BdrvChildRole.attach/detach() callbacks

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Backing files are somewhat special compared to other kinds of children
> because they are attached and detached using bdrv_set_backing_hd()
> rather than the normal set of functions, which does a few more things
> like setting backing blockers, toggling the BDRV_O_NO_BACKING flag,
> setting parent_bs->backing_file, etc.
> 
> These special features are a reason why change_parent_backing_link()
> can't handle backing files yet. With abstracting the additional features
> into .attach/.detach callbacks, we get a step closer to a function that
> can actually deal with this.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 94 
> +--
>  include/block/block_int.h |  3 ++
>  2 files changed, 62 insertions(+), 35 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 39/54] block: Fix pending requests check in bdrv_append()

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> bdrv_append() cares about isolation of the node that it modifies, but
> not about activity in some subtree below it. Instead of using the
> recursive bdrv_requests_pending(), directly check bs->in_flight, which
> considers only the node in question.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 38/54] backup: Use real permissions in backup block job

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> The backup block job doesn't have very complicated requirements: It
> needs to read from the source and write to the target, but it's fine
> with either side being changed. The only restriction is that we can't
> resize the image because the job uses a cached value.
> 
> qemu-iotests 055 needs to be changed because it used a target which was
> already attached to a virtio-blk device. The permission system correctly
> forbids this (virtio-blk can't accept another writer with its default
> share-rw=off).
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/backup.c | 15 ++-
>  tests/qemu-iotests/055 | 11 +++
>  2 files changed, 17 insertions(+), 9 deletions(-)

Not sure how much sense it makes to allow other writers on the target
node, but it's not as if the backup block job itself would care:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 37/54] commit: Use real permissions for HMP 'commit'

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> This is a little simpler than the commit block job because it's
> synchronous and only commits into the immediate backing file, but
> otherwise doing more or less the same.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/commit.c | 31 ++-
>  1 file changed, 26 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 18/54] block: Default .bdrv_child_perm() for format drivers

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Almost all format drivers have the same characteristics as far as
> permissions are concerned: They have one or more children for storing
> their own data and, more importantly, metadata (can be written to and
> grow even without external write requests, must be protected against
> other writers and present consistent data) and optionally a backing file
> (this is just data, so like for a filter, it only depends on what the
> parent nodes need).
> 
> This provides a default implementation that can be shared by most of
> our format drivers.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 42 ++
>  include/block/block_int.h |  8 
>  2 files changed, 50 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 523cbd3..f2e7178 100644
> --- a/block.c
> +++ b/block.c
> @@ -1554,6 +1554,48 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
> (c->shared_perm & DEFAULT_PERM_UNCHANGED);
>  }
>  
> +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> +   const BdrvChildRole *role,
> +   uint64_t perm, uint64_t shared,
> +   uint64_t *nperm, uint64_t *nshared)
> +{
> +bool backing = (role == _backing);
> +assert(role == _backing || role == _file);
> +
> +if (!backing) {
> +/* Apart from the modifications below, the same permissions are
> + * forwarded and left alone as for filters */
> +bdrv_filter_default_perms(bs, c, role, perm, shared, , );
> +
> +/* Format drivers may touch metadata even if the guest doesn't write 
> */
> +if (!bdrv_is_read_only(bs)) {
> +perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> +}
> +
> +/* bs->file always needs to be consistent because of the metadata. We
> + * can never allow other users to resize or write to it. */
> +perm |= BLK_PERM_CONSISTENT_READ;
> +shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> +} else {
> +/* We want consistent read from backing files if the parent needs it.
> + * No other operations are performed on backing files. */
> +perm &= BLK_PERM_CONSISTENT_READ;
> +
> +/* If the parent can deal with changing data, we're okay with a
> + * writable and resizable backing file. */
> +if (shared & BLK_PERM_WRITE) {
> +shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;

Wouldn't this break CONSISTENT_READ?

Max

> +} else {
> +shared = 0;
> +}
> +
> +shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
> +  BLK_PERM_WRITE_UNCHANGED;
> +}
> +
> +*nperm = perm;
> +*nshared = shared;
> +}
>  
>  static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>  {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 17f4c2d..eb0598e 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -880,6 +880,14 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
> uint64_t perm, uint64_t shared,
> uint64_t *nperm, uint64_t *nshared);
>  
> +/* Default implementation for BlockDriver.bdrv_child_perm() that can be used 
> by
> + * (non-raw) image formats: Like above for bs->backing, but for bs->file it
> + * requires WRITE | RESIZE for read-write images, always requires
> + * CONSISTENT_READ and doesn't share WRITE. */
> +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> +   const BdrvChildRole *role,
> +   uint64_t perm, uint64_t shared,
> +   uint64_t *nperm, uint64_t *nshared);
>  
>  const char *bdrv_get_parent_name(const BlockDriverState *bs);
>  void blk_dev_change_media_cb(BlockBackend *blk, bool load);
> 




signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [Bug 1580459] Re: Windows (10?) guest freezes entire host on shutdown if using PCI passthrough

[yanman]

Hi guys, not sure if I'm on the right track here but I think I'm
experiencing the same issue. My install might be a bit of a mess
combining bits from the VFIO Tips site and Ubuntu guides on GPU
passthrough, but I *did* have it all working for a few hours at a
stretch before I got this lock up.

The trouble with this is that after the host lockup, the Windows VM
seems to corrupt the EFI config or something like that as I can never
get it to boot again properly, even though the main partition seems fine
when tested in a bootable WinPE distro.

I'd be happy to supply versions and configs to help if it's related
however.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1580459

Title:
  Windows (10?) guest freezes entire host on shutdown if using PCI
  passthrough

Status in libvirt:
  New
Status in QEMU:
  New
Status in Arch Linux:
  New
Status in Debian:
  New
Status in Fedora:
  New

Bug description:
  Problem: after leaving a Windows VM that uses PCI passthrough (as we
  do for gaming graphics cards, sound cards, and in my case, a USB card)
  running for some amount of time between 1 and 2 hours (it's not
  consistent with exactly how long), and for any amount of time longer
  than that, shutting down that guest will, right as it finishes
  shutting down, freeze the host computer, making it require a hard
  reboot. Unbinding (or in the other user's case, unbinding and THEN
  binding) any PCI device in sysfs, even one that has nothing to do with
  the VM, also has the same effect as shutting down the VM (if the VM
  has been running long enough). So, it's probably an issue related to
  unbinding and binding PCI devices.

  There's a lot of info on this problem over at 
https://bbs.archlinux.org/viewtopic.php?id=206050
  Here's a better-organized list of main details:
  -at least 2 confirmed victims of this bug; 2 (including me) have provided 
lots of info in the link
  -I'm on Arch Linux and the other one is on Gentoo (distro-nonspecific)
  -issue affects my Windows 10 guest and others' Windows guests, but not my 
Arch Linux guest (the others don't have non-Windows guests to test)
  -I'm using libvirt but the other user is not, so it's not an issue with 
libvirt
  -It seems to be version non-specific, too. I first noticed it at, or when 
testing versions still had the issue at (whichever version is lower), Linux 4.1 
and qemu 2.4.0. It still persists in all releases of both since, including the 
newest ones.
  -I can't track down exactly what package downgrade can fix it, as downgrading 
further than Linux 4.1 and qemu 2.4.0 requires Herculean and system-destroying 
changes such as downgrading ncurses, meaning I don't know whether it's a bug in 
QEMU, the Linux kernel, or some weird seemingly unrelated thing.
  -According to the other user, "graphics intensive gameplay (GTA V) can cause 
the crash to happen sooner," as soon as "15 minutes"
  -Also, "bringing up a second passthrough VM with separate hardware will cause 
the same crash," and "bringing up another VM before the two-hour mark will not 
result in a crash," further cementing that it's triggered by the un/binding of 
PCI devices.
  -This is NOT related to the very similar bug that can be worked around by not 
passing through the HDMI device or sound card. Even when we removed all traces 
of any sort of sound card from the VM, it still had the same behavior.

To manage notifications about this bug go to:
https://bugs.launchpad.net/libvirt/+bug/1580459/+subscriptions

[Qemu-devel] [PATCH 2/2] linux-user: fix do_rt_sigreturn on m68k linux userspace emulation

[Laurent Vivier]

From: Michael Karcher 

do_rt_sigreturn uses an uninitialised local variable instead of fetching
the old signal mask directly from the signal frame when restoring the mask,
so the signal mask is undefined after do_rt_sigreturn. As the signal
frame data is in target-endian order, target_to_host_sigset instead of
target_to_host_sigset_internal is required.

do_sigreturn is correct in using target_to_host_sigset_internal, because
get_user already did the endianness conversion.

Signed-off-by: Michael Karcher 
Signed-off-by: Laurent Vivier 
---
 linux-user/signal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index df452ba..8382561 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5670,14 +5670,13 @@ long do_rt_sigreturn(CPUM68KState *env)
 {
 struct target_rt_sigframe *frame;
 abi_ulong frame_addr = env->aregs[7] - 4;
-target_sigset_t target_set;
 sigset_t set;
 
 trace_user_do_rt_sigreturn(env, frame_addr);
 if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
 goto badframe;
 
-target_to_host_sigset_internal(, _set);
+target_to_host_sigset(, >uc.tuc_sigmask);
 set_sigmask();
 
 /* restore registers */
-- 
2.9.3

[Qemu-devel] [PATCH 1/2] linux-user: correctly manage SR in ucontext

[Laurent Vivier]

Use cpu_m68k_get_ccr()/cpu_m68k_set_ccr() to setup and restore correctly
the value of SR in the ucontext structure

Signed-off-by: Laurent Vivier 
---
 linux-user/signal.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5064de0..df452ba 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5495,6 +5495,7 @@ static inline int target_rt_setup_ucontext(struct 
target_ucontext *uc,
CPUM68KState *env)
 {
 target_greg_t *gregs = uc->tuc_mcontext.gregs;
+uint32_t sr = cpu_m68k_get_ccr(env);
 
 __put_user(TARGET_MCONTEXT_VERSION, >tuc_mcontext.version);
 __put_user(env->dregs[0], [0]);
@@ -5514,7 +5515,7 @@ static inline int target_rt_setup_ucontext(struct 
target_ucontext *uc,
 __put_user(env->aregs[6], [14]);
 __put_user(env->aregs[7], [15]);
 __put_user(env->pc, [16]);
-__put_user(env->sr, [17]);
+__put_user(sr, [17]);
 
 return 0;
 }
@@ -5548,7 +5549,7 @@ static inline int target_rt_restore_ucontext(CPUM68KState 
*env,
 __get_user(env->aregs[7], [15]);
 __get_user(env->pc, [16]);
 __get_user(temp, [17]);
-env->sr = (env->sr & 0xff00) | (temp & 0xff);
+cpu_m68k_set_ccr(env, temp);
 
 return 0;
 
-- 
2.9.3

[Qemu-devel] [PATCH 0/2] linux-user: m68k update

[Laurent Vivier]

A couple of m68k patches I have in my stack for
a while and needed to have a working linux-user qemu.

The first one is needed to be able to use RISU with m68k.

Laurent Vivier (1):
  linux-user: correctly manage SR in ucontext

Michael Karcher (1):
  linux-user: fix do_rt_sigreturn on m68k linux userspace emulation

 linux-user/signal.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.9.3

Re: [Qemu-devel] [PATCH] spapr/pci: populate PCI DT in reverse order

[Greg Kurz]

On Sat, 25 Feb 2017 20:39:18 +1100
Alexey Kardashevskiy  wrote:

> On 22/02/17 21:56, Greg Kurz wrote:
> > From: Greg Kurz 
> > 
> > Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", 
> > QEMU
> > populates the PCI device tree in the opposite order compared to SLOF.
> > 
> > Before 1d2d974244c6:
> > 
> > Populating /pci@8002000
> >  00  (D) : 1af4 1000virtio [ net ]
> >  00 0800 (D) : 1af4 1001virtio [ block ]
> >  00 1000 (D) : 1af4 1009virtio [ network ]
> > Populating /pci@8002000/unknown-legacy-device@2
> > 
> > 7e5294b8 :  /pci@8002000
> > 7e52b998 :  |-- ethernet@0
> > 7e52c0c8 :  |-- scsi@1
> > 7e52c7e8 :  +-- unknown-legacy-device@2 ok
> > 
> > Since 1d2d974244c6:
> > 
> > Populating /pci@8002000
> >  00 1000 (D) : 1af4 1009virtio [ network ]
> > Populating /pci@8002000/unknown-legacy-device@2
> >  00 0800 (D) : 1af4 1001virtio [ block ]
> >  00  (D) : 1af4 1000virtio [ net ]
> > 
> > 7e5e8118 :  /pci@8002000
> > 7e5ea6a0 :  |-- unknown-legacy-device@2
> > 7e5eadb8 :  |-- scsi@1
> > 7e5eb4d8 :  +-- ethernet@0 ok
> > 
> > This behaviour change is not actually a bug since no assumptions should be
> > made on DT ordering. But it has no real justification either, other than
> > being the consequence of the way fdt_add_subnode() inserts new elements
> > to the front of the FDT rather than adding them to the tail.
> > 
> > This patch reverts to the historical SLOF ordering by walking PCI devices
> > in reverse order. This reconciles pseries with x86 machine types behavior.
> > It is expected to make things easier when porting existing applications to
> > power.
> > 
> > Signed-off-by: Greg Kurz 
> > Tested-by: Thomas Huth 
> > Reviewed-by: Nikunj A Dadhania 
> > (slight update to the changelog)
> > Signed-off-by: Greg Kurz 
> > ---
> >  hw/pci/pci.c |   28 
> >  hw/ppc/spapr_pci.c   |   12 ++--
> >  include/hw/pci/pci.h |4 
> >  3 files changed, 38 insertions(+), 6 deletions(-)
> > 
> > David,
> > 
> > This patch was posted and already discussed during 2.5 development:
> > 
> > http://patchwork.ozlabs.org/patch/549925/
> > 
> > The "consensus" at the time was that guests should not rely on device
> > ordering (i.e. use persistent naming instead).
> > 
> > I got recently contacted by OpenStack people who had several complaints
> > about the reverse ordering of PCI devices in pseries: different behavior
> > between ppc64 and x86, lots of time spent in debugging when porting
> > applications from x86 to ppc64 before realizing that it is caused by the
> > reverse ordering, necessity to carry hacky workarounds...  
> 
> 
> x86 does not have a device tree, and PCI id (bus:slot:fn) is the same
> regardless the scanning order, i.e. "lspci" will show the same picture with
> either order.
> 
> How could OpenStack tell the difference and require workaround for what
> precisely?
> 
> I am definitely missing the point here...
> 

NICs get probed in reverse order and are assigned different names compared
to the same setup on x86 (i.e. eth0 becomes eth1). They end up using wrong
network settings.

The same happens when using Nova libvirt driver which puts each disk on its
own PCI slot (vda becomes vdb).

This is usually avoided with persistent naming but as mentioned below, this
would require a lot of extra work, *just* because ppc64 guests don't do like
everybody else.

> 
> > 
> > One strong argument against handling this properly with persistent naming
> > is that it requires systemd/udev. This option is considered as painful
> > with CirrOS, which aims at remaining as minimal as possible and is widely
> > used in the OpenStack ecosystem.
> > 
> > Would you re-consider your position and apply this patch ?
> > 
> > Cheers.
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index a563555e7da7..273f1e46025a 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1530,6 +1530,34 @@ static const pci_class_desc pci_class_descriptions[] 
> > =
> >  { 0, NULL}
> >  };
> >  
> > +static void pci_for_each_device_under_bus_reverse(PCIBus *bus,
> > +  void (*fn)(PCIBus *b,
> > + PCIDevice *d,
> > + void *opaque),
> > +  void *opaque)
> > +{
> > +PCIDevice *d;
> > +int devfn;
> > +
> > +for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> > +d = bus->devices[ARRAY_SIZE(bus->devices) - 1 - devfn];
> > +if (d) {
> > +fn(bus, d, opaque);
> > +}
> > +

Re: [Qemu-devel] [PATCH] spapr/pci: populate PCI DT in reverse order

[Alexey Kardashevskiy]

On 22/02/17 21:56, Greg Kurz wrote:
> From: Greg Kurz 
> 
> Since commit 1d2d974244c6 "spapr_pci: enumerate and add PCI device tree", QEMU
> populates the PCI device tree in the opposite order compared to SLOF.
> 
> Before 1d2d974244c6:
> 
> Populating /pci@8002000
>  00  (D) : 1af4 1000virtio [ net ]
>  00 0800 (D) : 1af4 1001virtio [ block ]
>  00 1000 (D) : 1af4 1009virtio [ network ]
> Populating /pci@8002000/unknown-legacy-device@2
> 
> 7e5294b8 :  /pci@8002000
> 7e52b998 :  |-- ethernet@0
> 7e52c0c8 :  |-- scsi@1
> 7e52c7e8 :  +-- unknown-legacy-device@2 ok
> 
> Since 1d2d974244c6:
> 
> Populating /pci@8002000
>  00 1000 (D) : 1af4 1009virtio [ network ]
> Populating /pci@8002000/unknown-legacy-device@2
>  00 0800 (D) : 1af4 1001virtio [ block ]
>  00  (D) : 1af4 1000virtio [ net ]
> 
> 7e5e8118 :  /pci@8002000
> 7e5ea6a0 :  |-- unknown-legacy-device@2
> 7e5eadb8 :  |-- scsi@1
> 7e5eb4d8 :  +-- ethernet@0 ok
> 
> This behaviour change is not actually a bug since no assumptions should be
> made on DT ordering. But it has no real justification either, other than
> being the consequence of the way fdt_add_subnode() inserts new elements
> to the front of the FDT rather than adding them to the tail.
> 
> This patch reverts to the historical SLOF ordering by walking PCI devices
> in reverse order. This reconciles pseries with x86 machine types behavior.
> It is expected to make things easier when porting existing applications to
> power.
> 
> Signed-off-by: Greg Kurz 
> Tested-by: Thomas Huth 
> Reviewed-by: Nikunj A Dadhania 
> (slight update to the changelog)
> Signed-off-by: Greg Kurz 
> ---
>  hw/pci/pci.c |   28 
>  hw/ppc/spapr_pci.c   |   12 ++--
>  include/hw/pci/pci.h |4 
>  3 files changed, 38 insertions(+), 6 deletions(-)
> 
> David,
> 
> This patch was posted and already discussed during 2.5 development:
> 
> http://patchwork.ozlabs.org/patch/549925/
> 
> The "consensus" at the time was that guests should not rely on device
> ordering (i.e. use persistent naming instead).
> 
> I got recently contacted by OpenStack people who had several complaints
> about the reverse ordering of PCI devices in pseries: different behavior
> between ppc64 and x86, lots of time spent in debugging when porting
> applications from x86 to ppc64 before realizing that it is caused by the
> reverse ordering, necessity to carry hacky workarounds...


x86 does not have a device tree, and PCI id (bus:slot:fn) is the same
regardless the scanning order, i.e. "lspci" will show the same picture with
either order.

How could OpenStack tell the difference and require workaround for what
precisely?

I am definitely missing the point here...


> 
> One strong argument against handling this properly with persistent naming
> is that it requires systemd/udev. This option is considered as painful
> with CirrOS, which aims at remaining as minimal as possible and is widely
> used in the OpenStack ecosystem.
> 
> Would you re-consider your position and apply this patch ?
> 
> Cheers.
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a563555e7da7..273f1e46025a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1530,6 +1530,34 @@ static const pci_class_desc pci_class_descriptions[] =
>  { 0, NULL}
>  };
>  
> +static void pci_for_each_device_under_bus_reverse(PCIBus *bus,
> +  void (*fn)(PCIBus *b,
> + PCIDevice *d,
> + void *opaque),
> +  void *opaque)
> +{
> +PCIDevice *d;
> +int devfn;
> +
> +for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> +d = bus->devices[ARRAY_SIZE(bus->devices) - 1 - devfn];
> +if (d) {
> +fn(bus, d, opaque);
> +}
> +}
> +}
> +
> +void pci_for_each_device_reverse(PCIBus *bus, int bus_num,
> + void (*fn)(PCIBus *b, PCIDevice *d, void *opaque),
> + void *opaque)
> +{
> +bus = pci_find_bus_nr(bus, bus_num);
> +
> +if (bus) {
> +pci_for_each_device_under_bus_reverse(bus, fn, opaque);
> +}
> +}
> +
>  static void pci_for_each_device_under_bus(PCIBus *bus,
>void (*fn)(PCIBus *b, PCIDevice *d,
>   void *opaque),
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index fd6fc1d95344..2a20c2a140fc 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1782,9 +1782,9 @@ static void