Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues

[Markus Armbruster]

Peter Xu  writes:

> On Thu, Jun 07, 2018 at 01:53:01PM +0200, Markus Armbruster wrote:
>> Peter Xu  writes:
>> 
>> > Previously we cleanup the queues when we got CLOSED event.  It was used
>> 
>> we clean up
>> 
>> > to make sure we won't leftover replies/events of a old client to a new
>> 
>> we won't send leftover replies/events of an old client
>> 
>> > client.  Now this patch postpones that until OPENED.
>> 
>> What if OPENED never comes?
>
> Then we clean that up until destruction of the monitor.  IMHO it's
> fine, but I'm not sure whether that's an overall acceptable approach.

See below.

>> > In most cases, it does not really matter much since either way will make
>> > sure that the new client won't receive unexpected old events/responses.
>> > However there can be a very rare race condition with the old way when we
>> > use QMP with stdio and pipelines (which is quite common in iotests).
>> > The problem is that, we can easily have something like this in scripts:
>> >
>> >   cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands
>> >
>> > In most cases, a QMP session will be based on a bidirectional
>> > channel (read/write to a TCP port, for example), so IN and OUT are
>> > sharing some fundamentally same thing.  However that's untrue for pipes
>> 
>> Suggest "are fundamentally the same thing".
>> 
>> > above - the IN is the "cat" program, while the "OUT" is directed to the
>> > "filter_commands" which is actually another process.
>> 
>> Regarding 'IN is the "cat" program': a pipe is not a program.  Suggest
>> 'IN is connected to "cat", while OUT is connected to "filter_commands",
>> which are separate processes.
>> 
>> > Now when we received the CLOSED event, we cleanup the queues (including
>> 
>> we clean up
>> 
>> > the QMP response queue).  That means, if we kill/stop the "cat" process
>> > faster than the filter_commands process, the filter_commands process is
>> > possible to miss some responses/events that should belong to it.
>> 
>> I'm not sure I understand the error scenario.  Can you explain it in
>> more concrete terms?  Like "cat terminates, QEMU reads EOF, QEMU does
>> this, QEMU does that, oops, it shouldn't have done that".
>
> One condition could be this (after "quit" command is executed and QEMU
> quits the main loop):
>
> 1. [main thread] QEMU queues one SHUTDOWN event into response queue
>
> 2. "cat" terminates (to distinguish it from the animal, I quote it).
>Logically it can terminate even earlier, but let's just assume it's
>here.
>
> 3. [monitor iothread] QEMU reads EOF from stdin, which connects to the
>"cat" process
>
> 4. [monitor iothread] QEMU calls the CLOSED event hook for the monitor,
>which will clean up the response queue of the monitor, then the
>SHUTDOWN event is dropped
>
> 5. [main thread] clean up the monitors in monitor_cleanup(), when
>trying to flush pending responses, it sees nothing.  SHUTDOWN is
>lost forever

Got it, thanks!

The actual problem is that we drop the output queue when we see EOF on
input.

Here's what I think we should do on such EOF:

1. Shut down input

   Stop reading input.  If input has its own file descriptor (as opposed
   to a read/write fd shared with output), close it.

2. Flush output

   Continue to write output until the queue becomes empty or we run into
   an error (such as EPIPE, telling us the output's consumer went away).
   Works exactly as before, except we proceed to step 3 when the queue
   becomes empty.

3. Shut down output

   Close the output file descriptor.

> Note that before the monitor iothread was introduced, step [4]/[5]
> could never happen since the main loop was the only place to detect
> the EOF event of stdin and run the CLOSED event hooks.

I can't quite see what ensured response queue is empty before main loop
runs the CLOSED event hook.

> Now things can
> happen in parallel in the iothread.

Perhaps that just made the bug more likely to bite.

> I can add these into commit message.
>
>> 
>> > In practise, I encountered a very strange SHUTDOWN event missing when
>> 
>> practice
>> 
>> May I suggest use of a spell checker?  ;)
>
> Sorry for that.  TODO added: "Find a spell checker for Emacs".

C-h i
M emacs RET
M spelling RET

>> > running test with iotest 087.  Without this patch, iotest 078 will have
>> 
>> 087 or 078?
>
> 087.  Err.  Even a spell checker won't help me here!

Need to leave something for reviewers to do ;)

>> > ~10% chance to miss the SHUTDOWN event and fail when with Out-Of-Band
>> > enabled:
>> >
>> > 087 8s ... - output mismatch (see 087.out.bad)
>
> I'll take all the rest comments.  Thanks for reviewing.

I'm not sure this patch is the proper solution.  Can we explore the one
I sketched?

Re: [Qemu-devel] [PATCH v10 5/7] monitor: remove event_clock_type

[Markus Armbruster]

Peter Xu  writes:

> On Fri, Jun 08, 2018 at 07:38:11AM +0200, Markus Armbruster wrote:
>
> [...]
>
>> > +/*
>> > + * This should never be called before configure_accelerator() since
>> > + * only until then could we know whether qtest was enabled or not.
>> 
>> Uh, we know it after then, not until then.  What about
>> 
>>/*
>> * Return the clock to use for recording an event's time.
>> * Beware: result is invalid before configure_accelerator().
>
> Oh, another Chinglish from me. :(

Useful code, occasional Chinglish, the humility to go with it, and a
sense of humor --- you're doing fine.

[...]

Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang]

On 06/08/2018 10:17 AM, Peter Xu wrote:

On Thu, Jun 07, 2018 at 07:59:22PM +0800, Wei Wang wrote:

Not necessarily _need_ to share it, I meant it can be shared using qemu
command line.
Live migration doesn't happen all the time, and that optimization doesn't
run that long, if users want to have other BHs run in this iothread context,
they can only create one iothread via the qemu cmd line.

IMO iothreads and aiocontexts are for event-driven model.


To me it's just a thread which polls for submitted callbacks to run. 
When migration reaches the place that needs to submit the optimization 
function, it calls start() to submit it. I'm not sure why there is a 
worry about what's inside the callback.



Busy loop
is not an event-driven model.  Here if we want a busy loop I'll create
a thread when start page hinting, then join the thread when done.


 The old (v4) implementation worked that way as you mentioned above, 
and Michael suggested to use iothread in the previous discussion. I'm 
fine with both actually. For the virtio part, we've had many 
discussions, I would take the choice I had with Michael before, unless 
there is an obvious advantage (e.g. proved better performance).





But I'll stop commenting on this.  Please prepare a more clear
interface for migration in your next post.  I'll read that.



Sure, thanks. The new version is coming soon.







I'm a bit curious on how much time will it use to do
one round of the free page hints (e.g., an idle guest with 8G mem, or
any configuration you tested)?  I suppose during that time the
iothread will be held steady with 100% cpu usage, am I right?

Compared to the time spent by the legacy migration to send free pages, that
small amount of CPU usage spent on filtering free pages could be neglected.
Grinding a chopper will not hold up the work of cutting firewood :)

Sorry I didn't express myself clearly.

My question was that, have you measured how long time it will take
from starting of the free page hints (when balloon state is set to
FREE_PAGE_REPORT_S_REQUESTED), until it completes (when QEMU receives
the VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID, then set the status to
FREE_PAGE_REPORT_S_STOP)?


I vaguely remember it's several ms (for around 7.5G free pages) long time
ago. What would be the concern behind that number you want to know?

Because roughly I know the time between two bitmap syncs.  Then I will
know how possible a free page hinting process won't stop until the
next bitmap sync happens.


We have a function, stop(), to stop the optimization before the next 
bitmap sync if the optimization is still running. But I never saw that 
case happens (the free page hinting finishes itself before that).


Best,
Wei

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Cornelia Huck]

On Thu,  7 Jun 2018 18:52:18 +0200
David Hildenbrand  wrote:

> Let's introduce and use local error variables in the hotplug handler
> functions.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/s390x/s390-virtio-ccw.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7ae5fb38dd..29ea50a177 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>   DeviceState *dev, Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -s390_cpu_plug(hotplug_dev, dev, errp);
> +s390_cpu_plug(hotplug_dev, dev, &local_err);
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error 
> **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -error_setg(errp, "CPU hot unplug not supported on this machine");
> -return;
> +error_setg(&local_err, "CPU hot unplug not supported on this 
> machine");
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,

Just seeing this patch by itself, it does not really make much sense.
Even if this is a split out clean-up series, I'd prefer this to go
together with a patch that actually adds something more to the
plug/unplug functions.

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Christian Borntraeger]

On 06/08/2018 09:25 AM, Cornelia Huck wrote:
> On Thu,  7 Jun 2018 18:52:18 +0200
> David Hildenbrand  wrote:
> 
>> Let's introduce and use local error variables in the hotplug handler
>> functions.
>>
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 11 ---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 7ae5fb38dd..29ea50a177 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>   DeviceState *dev, Error **errp)
>>  {
>> +Error *local_err = NULL;
>> +
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -s390_cpu_plug(hotplug_dev, dev, errp);
>> +s390_cpu_plug(hotplug_dev, dev, &local_err);
>>  }
>> +error_propagate(errp, local_err);
>>  }
>>  
>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>> DeviceState *dev, Error 
>> **errp)
>>  {
>> +Error *local_err = NULL;
>> +
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -error_setg(errp, "CPU hot unplug not supported on this machine");
>> -return;
>> +error_setg(&local_err, "CPU hot unplug not supported on this 
>> machine");
>>  }
>> +error_propagate(errp, local_err);
>>  }
>>  
>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
> 
> Just seeing this patch by itself, it does not really make much sense.
> Even if this is a split out clean-up series, I'd prefer this to go
> together with a patch that actually adds something more to the
> plug/unplug functions.

+1. It is hard to see the "why". Maybe a better patch description could help 
here?

Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang]

On 06/07/2018 02:32 PM, Peter Xu wrote:

Btw, the migration_state_notifiers is already there, but seems not really
used (I only tracked spice-core.c called
add_migration_state_change_notifier). I thought adding new migration states
can reuse all that we have.
What's your real concern about that? (not sure how defining new events would
make a difference)

Migration state is exposed via control path (QMP).  Adding new states
mean that the QMP clients will see more.  IMO that's not really
anything that a QMP client will need to know, instead we can keep it
internally.  That's a reason from compatibility pov.

Meanwhile, it's not really a state-thing at all for me.  It looks
really more like hook or event (start/stop of sync).

Thanks for sharing your concerns in detail, which are quite helpful for the
discussion. To reuse 99a0db9b, we can also add sub-states (or say events),
instead of new migration states.
For example, we can still define "enum RamSaveState" as above, which can be
an indication for the notifier queued on the 99a0db9b notider_list to decide
whether to call start or stop.
Does this solve your concern?

Frankly speaking I don't fully understand how you would add that
sub-state.  If you are confident with the idea, maybe you can post
your new version with the change, then I can read the code.


Reusing 99a0db9b functions well, but I find it is more clear to let ram 
save state have it's own notifier list..will show how that works in v8.


Best,
Wei

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:12 +0200
David Hildenbrand  wrote:

> The node property can always be queried and the value has already been
> verified in pc_dimm_realize().
> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/ppc/spapr.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2375cbee12..d038f3243e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  error_setg(errp, "Memory hotplug not supported for this 
> machine");
>  return;
>  }
> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> errp);
> -if (*errp) {

Good riddance :)

> -return;
> -}
> -if (node < 0 || node >= MAX_NODES) {
> -error_setg(errp, "Invaild node %d", node);
> -return;
> -}
> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> NULL);

Maybe pass &error_abort ?

>  
>  spapr_memory_plug(hotplug_dev, dev, node, errp);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[David Hildenbrand]

On 08.06.2018 09:27, Christian Borntraeger wrote:
> 
> 
> On 06/08/2018 09:25 AM, Cornelia Huck wrote:
>> On Thu,  7 Jun 2018 18:52:18 +0200
>> David Hildenbrand  wrote:
>>
>>> Let's introduce and use local error variables in the hotplug handler
>>> functions.
>>>
>>> Signed-off-by: David Hildenbrand 
>>> ---
>>>  hw/s390x/s390-virtio-ccw.c | 11 ---
>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 7ae5fb38dd..29ea50a177 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>>   DeviceState *dev, Error **errp)
>>>  {
>>> +Error *local_err = NULL;
>>> +
>>>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>> -s390_cpu_plug(hotplug_dev, dev, errp);
>>> +s390_cpu_plug(hotplug_dev, dev, &local_err);
>>>  }
>>> +error_propagate(errp, local_err);
>>>  }
>>>  
>>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>>> DeviceState *dev, Error 
>>> **errp)
>>>  {
>>> +Error *local_err = NULL;
>>> +
>>>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>> -error_setg(errp, "CPU hot unplug not supported on this machine");
>>> -return;
>>> +error_setg(&local_err, "CPU hot unplug not supported on this 
>>> machine");
>>>  }
>>> +error_propagate(errp, local_err);
>>>  }
>>>  
>>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
>>
>> Just seeing this patch by itself, it does not really make much sense.
>> Even if this is a split out clean-up series, I'd prefer this to go
>> together with a patch that actually adds something more to the
>> plug/unplug functions.
> 
> +1. It is hard to see the "why". Maybe a better patch description could help 
> here?
> 

When checking for an error (*errp) we should make sure that we don't
dereference the NULL pointer. I will be doing that in the future (memory
devices), but as you both don't seem to like this patch, I'll drop it
for now.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 09:34, Greg Kurz wrote:
> On Thu,  7 Jun 2018 18:52:12 +0200
> David Hildenbrand  wrote:
> 
>> The node property can always be queried and the value has already been
>> verified in pc_dimm_realize().
>>
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/ppc/spapr.c | 9 +
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 2375cbee12..d038f3243e 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler 
>> *hotplug_dev,
>>  error_setg(errp, "Memory hotplug not supported for this 
>> machine");
>>  return;
>>  }
>> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
>> errp);
>> -if (*errp) {
> 
> Good riddance :)
> 
>> -return;
>> -}
>> -if (node < 0 || node >= MAX_NODES) {
>> -error_setg(errp, "Invaild node %d", node);
>> -return;
>> -}
>> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
>> NULL);
> 
> Maybe pass &error_abort ?

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually
never fail. But I can use error_abort here, does not matter.

Thanks!

> 
>>  
>>  spapr_memory_plug(hotplug_dev, dev, node, errp);
>>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/4] pc-bios/s390-ccw/net: Update code for the latest changes in SLOF

[Christian Borntraeger]

On 06/07/2018 02:22 PM, Thomas Huth wrote:
> The ip_version information now has to be stored in the filename_ip_t
> structure, and there is now a common function called tftp_get_error_info()
> which can be used to get the error string for a TFTP error code.
> We can also get rid of some superfluous "(char *)" casts now.
> 
> Signed-off-by: Thomas Huth 
Acked-by: Christian Borntraeger 



> ---
>  pc-bios/s390-ccw/netboot.mak |  2 +-
>  pc-bios/s390-ccw/netmain.c   | 86 
> +---
>  2 files changed, 18 insertions(+), 70 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
> index 4f64128..a73be36 100644
> --- a/pc-bios/s390-ccw/netboot.mak
> +++ b/pc-bios/s390-ccw/netboot.mak
> @@ -34,7 +34,7 @@ STDLIB_OBJS = atoi.o atol.o strtoul.o strtol.o rand.o 
> malloc.o free.o
>  %.o : $(SLOF_DIR)/lib/libc/stdlib/%.c
>   $(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ 
> $<,"CC","$(TARGET_DIR)$@")
>  
> -STDIO_OBJS = sprintf.o vfprintf.o vsnprintf.o vsprintf.o fprintf.o \
> +STDIO_OBJS = sprintf.o snprintf.o vfprintf.o vsnprintf.o vsprintf.o 
> fprintf.o \
>printf.o putc.o puts.o putchar.o stdchnls.o fileno.o
>  %.o : $(SLOF_DIR)/lib/libc/stdio/%.c
>   $(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ 
> $<,"CC","$(TARGET_DIR)$@")
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index 6000241..d007fb7 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -47,7 +47,6 @@ IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
>  static char cfgbuf[2048];
>  
>  static SubChannelId net_schid = { .one = 1 };
> -static int ip_version = 4;
>  static uint64_t dest_timer;
>  
>  static uint64_t get_timer_ms(void)
> @@ -100,10 +99,10 @@ static int dhcp(struct filename_ip *fn_ip, int retries)
>  printf("\nGiving up after %d DHCP requests\n", retries);
>  return -1;
>  }
> -ip_version = 4;
> +fn_ip->ip_version = 4;
>  rc = dhcpv4(NULL, fn_ip);
>  if (rc == -1) {
> -ip_version = 6;
> +fn_ip->ip_version = 6;
>  set_ipv6_address(fn_ip->fd, 0);
>  rc = dhcpv6(NULL, fn_ip);
>  if (rc == 0) {
> @@ -137,8 +136,7 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, 
> int len)
>  tftp_err_t tftp_err;
>  int rc;
>  
> -rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err, 1, 1428,
> -  ip_version);
> +rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err);
>  
>  if (rc < 0) {
>  /* Make sure that error messages are put into a new line */
> @@ -149,61 +147,11 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, 
> int len)
>  printf("  TFTP: Received %s (%d KBytes)\n", fnip->filename, rc / 
> 1024);
>  } else if (rc > 0) {
>  printf("  TFTP: Received %s (%d Bytes)\n", fnip->filename, rc);
> -} else if (rc == -1) {
> -puts("unknown TFTP error");
> -} else if (rc == -2) {
> -printf("TFTP buffer of %d bytes is too small for %s\n",
> -len, fnip->filename);
> -} else if (rc == -3) {
> -printf("file not found: %s\n", fnip->filename);
> -} else if (rc == -4) {
> -puts("TFTP access violation");
> -} else if (rc == -5) {
> -puts("illegal TFTP operation");
> -} else if (rc == -6) {
> -puts("unknown TFTP transfer ID");
> -} else if (rc == -7) {
> -puts("no such TFTP user");
> -} else if (rc == -8) {
> -puts("TFTP blocksize negotiation failed");
> -} else if (rc == -9) {
> -puts("file exceeds maximum TFTP transfer size");
> -} else if (rc <= -10 && rc >= -15) {
> -const char *icmp_err_str;
> -switch (rc) {
> -case -ICMP_NET_UNREACHABLE - 10:
> -icmp_err_str = "net unreachable";
> -break;
> -case -ICMP_HOST_UNREACHABLE - 10:
> -icmp_err_str = "host unreachable";
> -break;
> -case -ICMP_PROTOCOL_UNREACHABLE - 10:
> -icmp_err_str = "protocol unreachable";
> -break;
> -case -ICMP_PORT_UNREACHABLE - 10:
> -icmp_err_str = "port unreachable";
> -break;
> -case -ICMP_FRAGMENTATION_NEEDED - 10:
> -icmp_err_str = "fragmentation needed and DF set";
> -break;
> -case -ICMP_SOURCE_ROUTE_FAILED - 10:
> -icmp_err_str = "source route failed";
> -break;
> -default:
> -icmp_err_str = " UNKNOWN";
> -break;
> -}
> -printf("ICMP ERROR \"%s\"\n", icmp_err_str);
> -} else if (rc == -40) {
> -printf("TFTP error occurred after %d bad packets received",
> -tftp_err.bad_tftp_packets);
> -} else if (rc == -41) {
> -printf("TFTP error occurred after missing %d responses",
> -tftp_err.no_packets

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Fri, 8 Jun 2018 09:42:48 +0200
David Hildenbrand  wrote:

> On 08.06.2018 09:34, Greg Kurz wrote:
> > On Thu,  7 Jun 2018 18:52:12 +0200
> > David Hildenbrand  wrote:
> >   
> >> The node property can always be queried and the value has already been
> >> verified in pc_dimm_realize().
> >>
> >> Signed-off-by: David Hildenbrand 
> >> ---
> >>  hw/ppc/spapr.c | 9 +
> >>  1 file changed, 1 insertion(+), 8 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 2375cbee12..d038f3243e 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3578,14 +3578,7 @@ static void 
> >> spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >>  error_setg(errp, "Memory hotplug not supported for this 
> >> machine");
> >>  return;
> >>  }
> >> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> >> errp);
> >> -if (*errp) {  
> > 
> > Good riddance :)
> >   
> >> -return;
> >> -}
> >> -if (node < 0 || node >= MAX_NODES) {
> >> -error_setg(errp, "Invaild node %d", node);
> >> -return;
> >> -}
> >> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> >> NULL);  
> > 
> > Maybe pass &error_abort ?  
> 
> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> 
> ("error ignored" vs. "error leads to an abort") - but this will actually
> never fail. But I can use error_abort here, does not matter.
> 

Heh, /me paranoid but this is David's call and he acked that already
so I guess it's okay.

Reviewed-by: Greg Kurz 

> Thanks!
> 
> >   
> >>  
> >>  spapr_memory_plug(hotplug_dev, dev, node, errp);
> >>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 09:46, Greg Kurz wrote:
> On Fri, 8 Jun 2018 09:42:48 +0200
> David Hildenbrand  wrote:
> 
>> On 08.06.2018 09:34, Greg Kurz wrote:
>>> On Thu,  7 Jun 2018 18:52:12 +0200
>>> David Hildenbrand  wrote:
>>>   
 The node property can always be queried and the value has already been
 verified in pc_dimm_realize().

 Signed-off-by: David Hildenbrand 
 ---
  hw/ppc/spapr.c | 9 +
  1 file changed, 1 insertion(+), 8 deletions(-)

 diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
 index 2375cbee12..d038f3243e 100644
 --- a/hw/ppc/spapr.c
 +++ b/hw/ppc/spapr.c
 @@ -3578,14 +3578,7 @@ static void 
 spapr_machine_device_plug(HotplugHandler *hotplug_dev,
  error_setg(errp, "Memory hotplug not supported for this 
 machine");
  return;
  }
 -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
 errp);
 -if (*errp) {  
>>>
>>> Good riddance :)
>>>   
 -return;
 -}
 -if (node < 0 || node >= MAX_NODES) {
 -error_setg(errp, "Invaild node %d", node);
 -return;
 -}
 +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
 NULL);  
>>>
>>> Maybe pass &error_abort ?  
>>
>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>>
>> ("error ignored" vs. "error leads to an abort") - but this will actually
>> never fail. But I can use error_abort here, does not matter.
>>
> 
> Heh, /me paranoid but this is David's call and he acked that already
> so I guess it's okay.

NULL makes it fit into a single line :)

Thanks!

> 
> Reviewed-by: Greg Kurz 
> 
>> Thanks!
>>
>>>   
  
  spapr_memory_plug(hotplug_dev, dev, node, errp);
  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {  
>>>   
>>
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Christian Borntraeger]

On 06/08/2018 09:40 AM, David Hildenbrand wrote:
> On 08.06.2018 09:27, Christian Borntraeger wrote:
>>
>>
>> On 06/08/2018 09:25 AM, Cornelia Huck wrote:
>>> On Thu,  7 Jun 2018 18:52:18 +0200
>>> David Hildenbrand  wrote:
>>>
 Let's introduce and use local error variables in the hotplug handler
 functions.

 Signed-off-by: David Hildenbrand 
 ---
  hw/s390x/s390-virtio-ccw.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)

 diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
 index 7ae5fb38dd..29ea50a177 100644
 --- a/hw/s390x/s390-virtio-ccw.c
 +++ b/hw/s390x/s390-virtio-ccw.c
 @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
   DeviceState *dev, Error **errp)
  {
 +Error *local_err = NULL;
 +
  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
 -s390_cpu_plug(hotplug_dev, dev, errp);
 +s390_cpu_plug(hotplug_dev, dev, &local_err);
  }
 +error_propagate(errp, local_err);
  }
  
  static void s390_machine_device_unplug_request(HotplugHandler 
 *hotplug_dev,
 DeviceState *dev, Error 
 **errp)
  {
 +Error *local_err = NULL;
 +
  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
 -error_setg(errp, "CPU hot unplug not supported on this machine");
 -return;
 +error_setg(&local_err, "CPU hot unplug not supported on this 
 machine");
  }
 +error_propagate(errp, local_err);
  }
  
  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
>>>
>>> Just seeing this patch by itself, it does not really make much sense.
>>> Even if this is a split out clean-up series, I'd prefer this to go
>>> together with a patch that actually adds something more to the
>>> plug/unplug functions.
>>
>> +1. It is hard to see the "why". Maybe a better patch description could help 
>> here?
>>
> 
> When checking for an error (*errp) we should make sure that we don't
> dereference the NULL pointer. I will be doing that in the future (memory
> devices), but as you both don't seem to like this patch, I'll drop it
> for now.

With such a patch description (making the handler safe against NULL errp) the 
patch
suddenly makes sense on its own. 

Re: [Qemu-devel] [Qemu-block] [RFC PATCH V4] qemu-img: make convert async

[Stefan Hajnoczi]

On Thu, Jun 07, 2018 at 01:19:29PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2018 13:10, Stefan Hajnoczi wrote:
> > On Fri, Jun 01, 2018 at 07:16:14PM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> > > 20.02.2017 17:59, Peter Lieven wrote:
> > > > Am 20.02.2017 um 15:50 schrieb Stefan Hajnoczi:
> > > > > On Fri, Feb 17, 2017 at 05:00:24PM +0100, Peter Lieven wrote:
> > > > > > this is something I have been thinking about for almost 2 years now.
> > > > > > we heavily have the following two use cases when using qemu-img
> > > > > > convert.
> > > > > > 
> > > > > > 
> > > [...]
> > > 
> > > > > Does this patch work with compressed images?  Especially the
> > > > > out-of-order write mode may be problematic with a compressed qcow2
> > > > > image.
> > > > It does, but you are right, out-of-order writes and compression should
> > > > be mutually exclusive.
> > > 
> > > Sorry for being late, but can you please explain to me, why?
> > There are image format-specific limitations on compressed writes.  For
> > some reason I thought they were append-only in qcow2, but I was wrong.
> > 
> > Stefan
> 
> And what are limitations for compressed writes in qcow2?

Writes must be cluster-aligned and the size must be 1 cluster (except
for the last cluster in an image).

qemu-img convert honors this, so it's not a problem.

> We can't write asynchronously? Why?

Async compressed writes are supported nowadays.

I think my original comment was wrong.  It should be fine to use
out-of-order compressed writes.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] Cortex M0 emulation tasks

[Stefan Hajnoczi]

On Thu, Jun 07, 2018 at 06:46:26AM -0400, Liviu Ionescu wrote:
> On 7 June 2018 at 13:36:30, Stefan Hajnoczi (stefa...@gmail.com) wrote:
> 
> > If you do want to upstream the code you linked, please let us know
> > the
> > details of how you want to do it and how long it might take. Maybe
> > there is a way to work together on it...
> 
> yes, I considered this, but I do not have a schedule yet, probably in Q4.
> 
> I first plan to upgrade from 2.8 and only later to re-evaluate the
> entire project, to bring it closer to the upstream version.
> 
> 
> the main change I envision is to untangle the Cortex-M code from the
> current ARM code. maybe now the status is better, but when I did my
> changes I had lots of problems.

Thanks for the info.  We'll CC you on Cortex M0 patches.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v1 0/8] pc/spapr/s390x: machine hotplug handler cleanups

[David Hildenbrand]

On 07.06.2018 18:52, David Hildenbrand wrote:
> I'll be messing with machine hotplug handlers of pc/spapr/s390x in the
> context of
> [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
> 
> So this is a spin-off of the cleanup patches produced so far.
> 
> David Hildenbrand (8):
>   pc: local error handling in hotplug handler functions
>   spapr: no need to verify the node
>   spapr: move all DIMM checks into spapr_memory_plug
>   spapr: local error handling in hotplug handler functions
>   spapr: introduce machine unplug handler
>   spapr: handle pc-dimm unplug via hotplug handler chain
>   spapr: handle cpu core unplug via hotplug handler chain
>   s390x: local error handling in hotplug handler functions
> 
>  hw/i386/pc.c   | 32 +-
>  hw/ppc/spapr.c | 89 +-
>  hw/s390x/s390-virtio-ccw.c | 11 +++--
>  3 files changed, 89 insertions(+), 43 deletions(-)
> 

I might be dropping the "local error handling" patches (as Conny and
Christian properly asked for the reason). Although they are not wrong,
they are only useful if performing checks on error pointers (*errp) or
when relying on the error to indicate the "return value" of a function.

And as I might be handling plugging of memory devices in subfunctions
(to keep the handler short as requested by Igor), this might not be
needed on this level.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [RFC v3] qapi: command category to stimulate high-level machine devices

[Stefan Hajnoczi]

On Thu, Jun 07, 2018 at 11:33:07AM +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 07, 2018 at 11:24:55AM +0100, Stefan Hajnoczi wrote:
> > On Mon, Jun 04, 2018 at 12:12:21PM +0200, Gerd Hoffmann wrote:
> > > On Mon, Jun 04, 2018 at 10:29:40AM +0100, Peter Maydell wrote:
> > > > On 4 June 2018 at 10:20, Stefan Hajnoczi  wrote:
> > > > > Many of these inputs/outputs can be tied to an external UI.  A degree 
> > > > > of
> > > > > timing precision is required so that the UI is responsive, although
> > > > > cycle-accurate timing is not what I'd expect from QMP.
> > > > 
> > > > Would we also be able to tie them to an internal UI, ie
> > > > something that appears as another view in the GTK/etc
> > > > UI frontends we have?
> > > 
> > > Should be doable too.  Basically a display device, which isn't a *real*
> > > display but the UI.  Could show a rendering of the board, simliar to how
> > > web emulation environments are doing it.  LED status could be rendered
> > > directly to the board.  A virtual mouse could map mouse clicks to button
> > > presses.
> > > 
> > > Doing more complex input that way (say a slider for the temperature
> > > sensor) isn't going to work very well though ...
> > > 
> > > Sensor input in general is pretty much unsupported in qemu.
> > 
> > For the micro:bit we've been thinking of a WebSocket monitor interface.
> > This way a web UI can work with both local and remote QEMU instances.
> > 
> > For security reasons, the WebSocket cannot be the regular QMP monitor.
> 
> FWIW, add ability to use websockets protocol over chardevs is fairly
> easy. We already have a QIOChannelWebsock for the VNC server, so it
> is just a little work to wire it into the chardev.

Cool, good to know.

> If the -monitor / -qmp arg took a filename containing a whitelist of
> allowed monitor commands, you could indeed use the regular QMP monitor
> instead of writing something new.

Yes, this is exactly what we need.

> > A slimmed down monitor is required with a subset of QMP commands and
> > events.  For example, users must not be able to migrate to an exec:
> > destination so we need to ban that command on the UI monitor :-).
> 
> FWIW, you could  use the "-sandbox spawn=off,elevateprivileges=off"
> arg to prevent ability of QEMU to fork/exec/setuid. Even if the
> monitor still allows it, it thus get blocked, albeit by immediately
> terminating the process.

True, but that's just one example of many.  Another one is "pmemsave",
which writes to the host file system.

I think a whitelist is the way to go.  It will allow us to secure the
monitor and expose it to untrusted UIs.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] Some question about savem/qcow2 incremental snapshot

[Pankaj Gupta]

Hi Junyan,

AFAICU you are trying to utilize qcow2 capabilities to do incremental
snapshot. As I understand NVDIMM device (being it real or emulated), its
contents are always be backed up in backing device.  

Now, the question comes to take a snapshot at some point in time. You are 
trying to achieve this with qcow2 format (not checked code yet), I have below 
queries:

- Are you implementing this feature for both actual DAX device pass-through 
  as well as emulated DAX?
- Are you using additional qcow2 disk for storing/taking snapshots? How we are 
  planning to use this feature?

Reason I asked this question is if we concentrate on integrating qcow2
with DAX, we will have a full fledged solution for most of the use-cases. 

Thanks,
Pankaj 

> 
> Dear all:
> 
> I just switched from graphic/media field to virtualization at the end of the
> last year,
> so I am sorry that though I have already try my best but I still feel a
> little dizzy
> about your previous discussion about NVDimm via block layer:)
> In today's qemu, we use the SaveVMHandlers functions to handle both snapshot
> and migration.
> So for nvdimm kind memory, its migration and snapshot use the same way as the
> ram(savevm_ram_handlers). But the difference is the size of nvdimm may be
> huge, and the load
> and store speed is slower. According to my usage, when I use 256G nvdimm as
> memory backend,
> it may take more than 5 minutes to complete one snapshot saving, and after
> saving the qcow2
> image is bigger than 50G. For migration, this may not be a problem because we
> do not need
> extra disk space and the guest is not paused when in migration process. But
> for snapshot,
> we need to pause the VM and the user experience is bad, and we got concerns
> about that.
> I posted this question in Jan this year but failed to get enough reply. Then
> I sent a RFC patch
> set in Mar, basic idea is using the dependency snapshot and dirty log trace
> in kernel to
> optimize this.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg04530.html
> 
> I use the simple way to handle this,
> 1. Separate the nvdimm region from ram when do snapshot.
> 2. If the first time, we dump all the nvdimm data the same as ram, and enable
> dirty log trace
> for nvdimm kind region.
> 3. If not the first time, we find the previous snapshot point and add
> reference to its clusters
> which is used to store nvdimm data. And this time, we just save dirty page
> bitmap and dirty pages.
> Because the previous nvdimm data clusters is ref added, we do not need to
> worry about its deleting.
> 
> I encounter a lot of problems:
> 1. Migration and snapshot logic is mixed and need to separate them for
> nvdimm.
> 2. Cluster has its alignment. When do snapshot, we just save data to disk
> continuous. Because we
> need to add ref to cluster, we really need to consider the alignment. I just
> use a little trick way
> to padding some data to alignment now, and I think it is not a good way.
> 3. Dirty log trace may have some performance problem.
> 
> In theory, this manner can be used to handle all kind of huge memory
> snapshot, we need to find the
> balance between guest performance(Because of dirty log trace) and snapshot
> saving time.
> 
> Thanks
> Junyan
> 
> 
> -Original Message-
> From: Stefan Hajnoczi [mailto:stefa...@redhat.com]
> Sent: Thursday, May 31, 2018 6:49 PM
> To: Kevin Wolf 
> Cc: Max Reitz ; He, Junyan ; Pankaj
> Gupta ; qemu-devel@nongnu.org; qemu block
> 
> Subject: Re: [Qemu-block] [Qemu-devel] Some question about savem/qcow2
> incremental snapshot
> 
> On Wed, May 30, 2018 at 06:07:19PM +0200, Kevin Wolf wrote:
> > Am 30.05.2018 um 16:44 hat Stefan Hajnoczi geschrieben:
> > > On Mon, May 14, 2018 at 02:48:47PM +0100, Stefan Hajnoczi wrote:
> > > > On Fri, May 11, 2018 at 07:25:31PM +0200, Kevin Wolf wrote:
> > > > > Am 10.05.2018 um 10:26 hat Stefan Hajnoczi geschrieben:
> > > > > > On Wed, May 09, 2018 at 07:54:31PM +0200, Max Reitz wrote:
> > > > > > > On 2018-05-09 12:16, Stefan Hajnoczi wrote:
> > > > > > > > On Tue, May 08, 2018 at 05:03:09PM +0200, Kevin Wolf wrote:
> > > > > > > >> Am 08.05.2018 um 16:41 hat Eric Blake geschrieben:
> > > > > > > >>> On 12/25/2017 01:33 AM, He Junyan wrote:
> > > > > > > >> I think it makes sense to invest some effort into such
> > > > > > > >> interfaces, but be prepared for a long journey.
> > > > > > > > 
> > > > > > > > I like the suggestion but it needs to be followed up with
> > > > > > > > a concrete design that is feasible and fair for Junyan and
> > > > > > > > others to implement.
> > > > > > > > Otherwise the "long journey" is really just a way of
> > > > > > > > rejecting this feature.
> > > 
> > > The discussion on NVDIMM via the block layer has runs its course.
> > > It would be a big project and I don't think it's fair to ask Junyan
> > > to implement it.
> > > 
> > > My understanding is this patch series doesn't modify the qcow2
> > > on-disk file format.  Rather, it just uses existin

Re: [Qemu-devel] [RFC v3] qapi: command category to stimulate high-level machine devices

[Stefan Hajnoczi]

On Thu, Jun 07, 2018 at 02:58:53PM +0200, Gerd Hoffmann wrote:
> > Gerd: What is your preference?  Do you want board-specific fake displays
> > inside the QEMU process as the long-term direction for UIs?
> 
> Well, it isn't the one or the other, we could actually do both.  For the
> deployments you are looking for the websocket approach proably works
> best.  Being able to do at least simple stuff (say led display and
> a+b+reset buttons) with a fake display, without requiring setting up
> something separate for the UI can be quite useful too.
> 
> Your call.

Okay.  There is an existing micro:bit web UI that we're interested in
either reusing or at least closely following as a model
(https://groklearning.com/learn/hoc-virtual-pet/intro/2/), so WebSockets
makes sense for this.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/8] pc: local error handling in hotplug handler functions

[Thomas Huth]

On 07.06.2018 18:52, David Hildenbrand wrote:
> Let's introduce and use local error variables in the hotplug handler
> functions.

Why? You don't check local_err in the functions, so I fail to see why
this is needed? If you need this in a later patch, I think this should
simply be part of that later patch instead.

 Thomas


> Signed-off-by: David Hildenbrand 
> ---
>  hw/i386/pc.c | 32 ++--
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3befe6721..8075c6af15 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2006,45 +2006,57 @@ static void pc_cpu_pre_plug(HotplugHandler 
> *hotplug_dev,
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>DeviceState *dev, Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -pc_cpu_pre_plug(hotplug_dev, dev, errp);
> +pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>DeviceState *dev, Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -pc_dimm_plug(hotplug_dev, dev, errp);
> +pc_dimm_plug(hotplug_dev, dev, &local_err);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -pc_cpu_plug(hotplug_dev, dev, errp);
> +pc_cpu_plug(hotplug_dev, dev, &local_err);
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>  DeviceState *dev, Error 
> **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -pc_dimm_unplug_request(hotplug_dev, dev, errp);
> +pc_dimm_unplug_request(hotplug_dev, dev, &local_err);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
> +pc_cpu_unplug_request_cb(hotplug_dev, dev, &local_err);
>  } else {
> -error_setg(errp, "acpi: device unplug request for not supported 
> device"
> -   " type: %s", object_get_typename(OBJECT(dev)));
> +error_setg(&local_err, "acpi: device unplug request for not 
> supported"
> +   " device type: %s", object_get_typename(OBJECT(dev)));
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>  DeviceState *dev, Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -pc_dimm_unplug(hotplug_dev, dev, errp);
> +pc_dimm_unplug(hotplug_dev, dev, &local_err);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -pc_cpu_unplug_cb(hotplug_dev, dev, errp);
> +pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
>  } else {
> -error_setg(errp, "acpi: device unplug for not supported device"
> +error_setg(&local_err, "acpi: device unplug for not supported device"
> " type: %s", object_get_typename(OBJECT(dev)));
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
> 

Re: [Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:13 +0200
David Hildenbrand  wrote:

> Let's clean the hotplug handler up by moving everything into
> spapr_memory_plug().
> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/ppc/spapr.c | 23 ++-
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d038f3243e..a12be24ca9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t 
> addr_start, uint64_t size,
>  }
>  
>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> -  uint32_t node, Error **errp)
> +  Error **errp)
>  {
>  Error *local_err = NULL;
>  sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>  PCDIMMDevice *dimm = PC_DIMM(dev);
>  PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>  MemoryRegion *mr;
>  uint64_t align, size, addr;
> +uint32_t node;
> +
> +if (!smc->dr_lmb_enabled) {
> +error_setg(&local_err, "Memory hotplug not supported for this 
> machine");
> +goto out;
> +}

Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?

> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>  
>  mr = ddc->get_memory_region(dimm, &local_err);
>  if (local_err) {
> @@ -3568,19 +3576,8 @@ out:
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>DeviceState *dev, Error **errp)
>  {
> -MachineState *ms = MACHINE(hotplug_dev);
> -sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> -
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -int node;
> -
> -if (!smc->dr_lmb_enabled) {
> -error_setg(errp, "Memory hotplug not supported for this 
> machine");
> -return;
> -}
> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> NULL);
> -
> -spapr_memory_plug(hotplug_dev, dev, node, errp);
> +spapr_memory_plug(hotplug_dev, dev, errp);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>  spapr_core_plug(hotplug_dev, dev, errp);
>  }

Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues

[Stefan Hajnoczi]

On Fri, Jun 08, 2018 at 12:42:35PM +0800, Peter Xu wrote:
> On Thu, Jun 07, 2018 at 01:53:01PM +0200, Markus Armbruster wrote:
> > Peter Xu  writes:
> > 
> > > Previously we cleanup the queues when we got CLOSED event.  It was used
> > 
> > we clean up
> > 
> > > to make sure we won't leftover replies/events of a old client to a new
> > 
> > we won't send leftover replies/events of an old client
> > 
> > > client.  Now this patch postpones that until OPENED.
> > 
> > What if OPENED never comes?
> 
> Then we clean that up until destruction of the monitor.  IMHO it's
> fine, but I'm not sure whether that's an overall acceptable approach.

I think this patch fixes the problem at the wrong level.  Marc-André's
fix seemed like a cleaner solution.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[David Hildenbrand]

On 08.06.2018 10:05, Greg Kurz wrote:
> On Thu,  7 Jun 2018 18:52:13 +0200
> David Hildenbrand  wrote:
> 
>> Let's clean the hotplug handler up by moving everything into
>> spapr_memory_plug().
>>
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/ppc/spapr.c | 23 ++-
>>  1 file changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index d038f3243e..a12be24ca9 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, 
>> uint64_t addr_start, uint64_t size,
>>  }
>>  
>>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> -  uint32_t node, Error **errp)
>> +  Error **errp)
>>  {
>>  Error *local_err = NULL;
>>  sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>>  PCDIMMDevice *dimm = PC_DIMM(dev);
>>  PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>>  MemoryRegion *mr;
>>  uint64_t align, size, addr;
>> +uint32_t node;
>> +
>> +if (!smc->dr_lmb_enabled) {
>> +error_setg(&local_err, "Memory hotplug not supported for this 
>> machine");
>> +goto out;
>> +}
> 
> Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() 
> ?

Think you're right (and as spapr_memory_pre_plug() already exists, it's
easy), other opinions? Thanks.

> 
>> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>>  
>>  mr = ddc->get_memory_region(dimm, &local_err);
>>  if (local_err) {
>> @@ -3568,19 +3576,8 @@ out:
>>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>>DeviceState *dev, Error **errp)
>>  {
>> -MachineState *ms = MACHINE(hotplug_dev);
>> -sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>> -
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -int node;
>> -
>> -if (!smc->dr_lmb_enabled) {
>> -error_setg(errp, "Memory hotplug not supported for this 
>> machine");
>> -return;
>> -}
>> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
>> NULL);
>> -
>> -spapr_memory_plug(hotplug_dev, dev, node, errp);
>> +spapr_memory_plug(hotplug_dev, dev, errp);
>>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>>  spapr_core_plug(hotplug_dev, dev, errp);
>>  }
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/8] pc: local error handling in hotplug handler functions

[David Hildenbrand]

On 08.06.2018 10:04, Thomas Huth wrote:
> On 07.06.2018 18:52, David Hildenbrand wrote:
>> Let's introduce and use local error variables in the hotplug handler
>> functions.
> 
> Why? You don't check local_err in the functions, so I fail to see why
> this is needed? If you need this in a later patch, I think this should
> simply be part of that later patch instead.

See the mail I sent a couple of minutes ago as reply to the cover
letter. Thanks.

> 
>  Thomas
> 
> 
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/i386/pc.c | 32 ++--
>>  1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f3befe6721..8075c6af15 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2006,45 +2006,57 @@ static void pc_cpu_pre_plug(HotplugHandler 
>> *hotplug_dev,
>>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>DeviceState *dev, Error **errp)
>>  {
>> +Error *local_err = NULL;
>> +
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -pc_cpu_pre_plug(hotplug_dev, dev, errp);
>> +pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
>>  }
>> +error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>DeviceState *dev, Error **errp)
>>  {
>> +Error *local_err = NULL;
>> +
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -pc_dimm_plug(hotplug_dev, dev, errp);
>> +pc_dimm_plug(hotplug_dev, dev, &local_err);
>>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -pc_cpu_plug(hotplug_dev, dev, errp);
>> +pc_cpu_plug(hotplug_dev, dev, &local_err);
>>  }
>> +error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>  DeviceState *dev, Error 
>> **errp)
>>  {
>> +Error *local_err = NULL;
>> +
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -pc_dimm_unplug_request(hotplug_dev, dev, errp);
>> +pc_dimm_unplug_request(hotplug_dev, dev, &local_err);
>>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
>> +pc_cpu_unplug_request_cb(hotplug_dev, dev, &local_err);
>>  } else {
>> -error_setg(errp, "acpi: device unplug request for not supported 
>> device"
>> -   " type: %s", object_get_typename(OBJECT(dev)));
>> +error_setg(&local_err, "acpi: device unplug request for not 
>> supported"
>> +   " device type: %s", object_get_typename(OBJECT(dev)));
>>  }
>> +error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>>  DeviceState *dev, Error **errp)
>>  {
>> +Error *local_err = NULL;
>> +
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -pc_dimm_unplug(hotplug_dev, dev, errp);
>> +pc_dimm_unplug(hotplug_dev, dev, &local_err);
>>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -pc_cpu_unplug_cb(hotplug_dev, dev, errp);
>> +pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
>>  } else {
>> -error_setg(errp, "acpi: device unplug for not supported device"
>> +error_setg(&local_err, "acpi: device unplug for not supported 
>> device"
>> " type: %s", object_get_typename(OBJECT(dev)));
>>  }
>> +error_propagate(errp, local_err);
>>  }
>>  
>>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Thomas Huth]

On 08.06.2018 09:48, David Hildenbrand wrote:
> On 08.06.2018 09:46, Greg Kurz wrote:
>> On Fri, 8 Jun 2018 09:42:48 +0200
>> David Hildenbrand  wrote:
>>
>>> On 08.06.2018 09:34, Greg Kurz wrote:
 On Thu,  7 Jun 2018 18:52:12 +0200
 David Hildenbrand  wrote:
   
> The node property can always be queried and the value has already been
> verified in pc_dimm_realize().
>
> Signed-off-by: David Hildenbrand 
> ---
>  hw/ppc/spapr.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2375cbee12..d038f3243e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3578,14 +3578,7 @@ static void 
> spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>  error_setg(errp, "Memory hotplug not supported for this 
> machine");
>  return;
>  }
> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> errp);
> -if (*errp) {  

 Good riddance :)
   
> -return;
> -}
> -if (node < 0 || node >= MAX_NODES) {
> -error_setg(errp, "Invaild node %d", node);
> -return;
> -}

Maybe turn that into an assert() instead? ... just for the paranoids ;-)

> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> NULL);  

 Maybe pass &error_abort ?  
>>>
>>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>>>
>>> ("error ignored" vs. "error leads to an abort") - but this will actually
>>> never fail. But I can use error_abort here, does not matter.
>>>
>>
>> Heh, /me paranoid but this is David's call and he acked that already
>> so I guess it's okay.
> 
> NULL makes it fit into a single line :)

+1 for error_abort, even if it takes another line.

 Thomas

Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues

[Markus Armbruster]

Stefan Hajnoczi  writes:

> On Fri, Jun 08, 2018 at 12:42:35PM +0800, Peter Xu wrote:
>> On Thu, Jun 07, 2018 at 01:53:01PM +0200, Markus Armbruster wrote:
>> > Peter Xu  writes:
>> > 
>> > > Previously we cleanup the queues when we got CLOSED event.  It was used
>> > 
>> > we clean up
>> > 
>> > > to make sure we won't leftover replies/events of a old client to a new
>> > 
>> > we won't send leftover replies/events of an old client
>> > 
>> > > client.  Now this patch postpones that until OPENED.
>> > 
>> > What if OPENED never comes?
>> 
>> Then we clean that up until destruction of the monitor.  IMHO it's
>> fine, but I'm not sure whether that's an overall acceptable approach.
>
> I think this patch fixes the problem at the wrong level.  Marc-André's
> fix seemed like a cleaner solution.

Is it the right solution?

I proposed another one:

[...]
>> > Here's what I think we should do on such EOF:
>> > 
>> > 1. Shut down input
>> > 
>> >Stop reading input.  If input has its own file descriptor (as opposed
>> >to a read/write fd shared with output), close it.
>> > 
>> > 2. Flush output
>> > 
>> >Continue to write output until the queue becomes empty or we run into
>> >an error (such as EPIPE, telling us the output's consumer went away).
>> >Works exactly as before, except we proceed to step 3 when the queue

exactly as before EOF

>> >becomes empty.
>> > 
>> > 3. Shut down output
>> > 
>> >Close the output file descriptor.

Re: [Qemu-devel] [PATCH 4/6] hmp: Add info commands for preconfig

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  writes:
> 
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> "Dr. David Alan Gilbert"  writes:
> >> 
> >> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> >> Peter Xu  writes:
> >> >> 
> >> >> > On Tue, Jun 05, 2018 at 01:26:34PM +0100, Dr. David Alan Gilbert 
> >> >> > (git) wrote:
> >> >> >> From: "Dr. David Alan Gilbert" 
> >> >> >> 
> >> >> >> Allow a bunch of the info commands to be used in preconfig.
> >> >> >> Could probably add most of them.
> >> >> >
> >> >> > I guess some of them may not work yet during preconfig.  E.g.:
> >> >> >
> >> >> > $ ./x86_64-softmmu/qemu-system-x86_64 -preconfig -monitor stdio
> >> >> > QEMU 2.12.50 monitor - type 'help' for more information
> >> >> > (qemu) info mtree
> >> >> > address-space: memory
> >> >> >   - (prio 0, i/o): system
> >> >> >
> >> >> > address-space: I/O
> >> >> >   - (prio 0, i/o): io
> >> >> >
> >> >> > But it's fine to enable that I guess.
> >> >> >
> >> >> > (Which "info" command would you want to use during preconfig?)
> >> >> >
> >> >> >> 
> >> >> >> Signed-off-by: Dr. David Alan Gilbert 
> >> >> >
> >> >> > Reviewed-by: Peter Xu 
> >> >> 
> >> >> The reason for having -preconfig is us despairing of making -S do the
> >> >> right thing.  We'd have to *understand* the tangled mess that is our
> >> >> startup, and rearrange it so QMP becomes available early enough for
> >> >> configuring NUMA (and other things), yet late enough for everything to
> >> >> work.
> >> >> 
> >> >> -preconfig is a cheap hack to avoid this headache, by bypassing almost
> >> >> all of "everything".
> >> >> 
> >> >> Now you bring back some of "everything".  Dangerous.  You better show it
> >> >> actually works.  Until you do:
> >> >> 
> >> >> NAK
> >> >
> >> > Well I did test each command in here to make sure it didn't
> >> > crash/produce complete junk; but here's the output with the v2 of this
> >> > patch that Igor R-b:
> >> [...]
> >> 
> >> For the sake of the argument, let's assume these commands all work in
> >> preconfig state.  Are their QMP equivalents all available in preconfig
> >> state?
> >
> > That I don't know; I was happy to fix my list to the ones
> > Igor recommended.  If you object to some particular entries I'll
> > be happy to change them.
> 
> HMP must not provide more functionality than QMP.  Specifically, we may
> provide "info FOO" only when we also provide query-FOO.
>
> There are exceptions to this rule.  I don't think they apply here.  I'm
> prepared to discuss them, of course.

No, that's strictly not true;  HMP can provide anything that helps
a human debug stuff.  The requirement is that if a tool needs it then it
must be provided in QMP.

> I wish there was a way to automate "provide command in HMP when its
> buddy is available in QMP", but since the buddies are only connected by
> code, that seems infeasible.
> 
> Without such automation, the two sets of available commands need to be
> kept consistent manually.  The larger they are, the more of a bother.
> 
> Bother is fine when it provides commensurate value to users.  Options in
> increasing order of value provided:
> 
> (1) HMP becomes ready only after we exit preconfig state (what I
> proposed in Message-ID: <87603cxob9@dusky.pond.sub.org>.
>
> (2) HMP provides help, quit, exit-preconfig.
> 
> (3) HMP provides (a subset of) the commands QMP provides.
> 
> I figure the maintenance cost of (1) and (2) will be negligible, but (3)
> could be a drag.  Are you sure it's worthwhile?


I'm not prepared to restrict to (2), and I'm not prepared to restrict
HMP to a subset of QMP;  As I said previously, if there's a command that
you think is incorrect/broken that I've enabled then I'm happy to
remove it.

Dave

--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] storing machine data in qcow images?

[Dr. David Alan Gilbert]

* Laszlo Ersek (ler...@redhat.com) wrote:
> On 06/07/18 12:54, Andrea Bolognani wrote:
> > On Thu, 2018-06-07 at 11:36 +0100, Daniel P. Berrangé wrote:
> >> On Thu, Jun 07, 2018 at 11:32:18AM +0100, Richard W.M. Jones wrote:
> >>> Another problem which Laszlo mentioned is the varstore isn't portable
> >>> between UEFI implementations, or if the UEFI is compiled with
> >>> different options.  You can even imagine shipping multiple
> >>> varstores(!) which argues for a tar-like format.
> >>
> >> Could we perhaps imagine shipping the actual UEFI bios, rather
> >> than only the varstore.  The bios blob runs in guest context,
> >> so there shouldn't be able security concerns from hosting
> >> vendors with running user provided bios. Mostly its a matter
> >> of confidence that the interface between bios & qemu is stable
> >> which feels easier than assuming varstore vs different bios is
> >> portable.
> > 
> > That sounds sensible, and further reinforces the idea that we
> > need way more than a single string baked into the qcow2 file.
> > 
> 
> Sorry for arriving late (thanks Rich for the Fwd).
> 
> The contents of the non-volatile UEFI variables should be considered
> part of (permanent) guest state, such as disk contents. Therefore I'd
> argue for bundling the varstore file with the disk image(s).
> 
> In turn, the best way to ensure comaptibility between varstore and
> firmware binary is to just bundle the firmware binary as well. It's
> generally not large (x86) or if it is, it compresses extremely well
> (aarch64). For extra politeness, image providers can bundle a text file
> with their firmware build options (like a kernel config), possibly even
> a JSON document conforming to the new firmware schema (qemu commit
> 3a0adfc9bfcf), but that's not a hard requirement I guess.
> 
> If such a VM is to be migrated between hosts, I'd expect the host admin
> to take care of installing the fw binary on all eligible hosts.

There's no way they can do that if they're just importing VMs from
templates that include the image; who is going to keep track of which
BIOSs are needed where?

Dave

> Regarding compat between QEMU and firmware binary, I see three cases:
> 
> (1) Static requirements presented by the firmware for the QEMU
> configuration. (Such as -D SMM_REQUIRE.) With the domain configuration
> captured one way or another alongside the disk image anyway, this should
> not be a problem.
> 
> (2) New firmware launched on old QEMU. The firmware generally detects or
> negotiates features with QEMU, so this should be safe.
> 
> (Discounting firmware regressions, of course -- for example, search
>  for
> the string "I messed up".)
> 
> (3) Old firmware launched on new QEMU. This scenario has given us a lot
> more grief than (2), but I think for the appliance distribution use
> case, it can be folded into case (1) above -- specify the machine type
> too in the domain config, and that should be compatible with the old
> firmware.
> 
> (The handling of (3) is not uniform between upstream QEMU and various
> downstreams. For example, consider
> . This was a latent bug in
> OVMF that got exposed by a new QEMU (due to a valid QEMU change), even
> when using old machine types. The upstream solution was to fix edk2 and
> stick with QEMU as-was (although the agreement around that hadn't been
> universal). Conversely, one downstream solution was to restrict the
> otherwise valid QEMU change to new machine types
> .)
> 
> 
> All in all I agree with Daniel's proposal; it seems to be the most
> robust one.
> 
> And, I too recall that, under AMD SEV, users will be supposed to, or
> allowed to, provide their own firmware binaries.
> 
> Thanks!
> Laszlo
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Gibson]

On Fri, Jun 08, 2018 at 09:46:57AM +0200, Greg Kurz wrote:
> On Fri, 8 Jun 2018 09:42:48 +0200
> David Hildenbrand  wrote:
> 
> > On 08.06.2018 09:34, Greg Kurz wrote:
> > > On Thu,  7 Jun 2018 18:52:12 +0200
> > > David Hildenbrand  wrote:
> > >   
> > >> The node property can always be queried and the value has already been
> > >> verified in pc_dimm_realize().
> > >>
> > >> Signed-off-by: David Hildenbrand 
> > >> ---
> > >>  hw/ppc/spapr.c | 9 +
> > >>  1 file changed, 1 insertion(+), 8 deletions(-)
> > >>
> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > >> index 2375cbee12..d038f3243e 100644
> > >> --- a/hw/ppc/spapr.c
> > >> +++ b/hw/ppc/spapr.c
> > >> @@ -3578,14 +3578,7 @@ static void 
> > >> spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > >>  error_setg(errp, "Memory hotplug not supported for this 
> > >> machine");
> > >>  return;
> > >>  }
> > >> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> > >> errp);
> > >> -if (*errp) {  
> > > 
> > > Good riddance :)
> > >   
> > >> -return;
> > >> -}
> > >> -if (node < 0 || node >= MAX_NODES) {
> > >> -error_setg(errp, "Invaild node %d", node);
> > >> -return;
> > >> -}
> > >> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> > >> NULL);  
> > > 
> > > Maybe pass &error_abort ?  
> > 
> > I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> > 
> > ("error ignored" vs. "error leads to an abort") - but this will actually
> > never fail. But I can use error_abort here, does not matter.
> > 
> 
> Heh, /me paranoid but this is David's call and he acked that already
> so I guess it's okay.

Actually, I missed this - error_abort is preferable.  That's general
the right choice for things that shouldn't ever fail.  This way if
they *do* fail we get a clear error immediately.

> Reviewed-by: Greg Kurz 


-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 10:20, David Gibson wrote:
> On Fri, Jun 08, 2018 at 09:46:57AM +0200, Greg Kurz wrote:
>> On Fri, 8 Jun 2018 09:42:48 +0200
>> David Hildenbrand  wrote:
>>
>>> On 08.06.2018 09:34, Greg Kurz wrote:
 On Thu,  7 Jun 2018 18:52:12 +0200
 David Hildenbrand  wrote:
   
> The node property can always be queried and the value has already been
> verified in pc_dimm_realize().
>
> Signed-off-by: David Hildenbrand 
> ---
>  hw/ppc/spapr.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2375cbee12..d038f3243e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3578,14 +3578,7 @@ static void 
> spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>  error_setg(errp, "Memory hotplug not supported for this 
> machine");
>  return;
>  }
> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> errp);
> -if (*errp) {  

 Good riddance :)
   
> -return;
> -}
> -if (node < 0 || node >= MAX_NODES) {
> -error_setg(errp, "Invaild node %d", node);
> -return;
> -}
> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> NULL);  

 Maybe pass &error_abort ?  
>>>
>>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>>>
>>> ("error ignored" vs. "error leads to an abort") - but this will actually
>>> never fail. But I can use error_abort here, does not matter.
>>>
>>
>> Heh, /me paranoid but this is David's call and he acked that already
>> so I guess it's okay.
> 
> Actually, I missed this - error_abort is preferable.  That's general
> the right choice for things that shouldn't ever fail.  This way if
> they *do* fail we get a clear error immediately.

error_abort it is :)

> 
>> Reviewed-by: Greg Kurz 
> 
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 1/2] memory-device: turn alignment assert into check

[Igor Mammedov]

On Thu,  7 Jun 2018 17:47:04 +0200
David Hildenbrand  wrote:

> The start of the address space indicates which maximum alignment is
> supported by our machine (e.g. ppc, x86 1GB). This is helpful to
> catch fragmenting guest physical memory in strange fashions.
> 
> Right now we can crash QEMU by e.g. (there might be easier examples)
> 
> qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 \
>  -object 
> memory-backend-file,id=mem0,size=8192M,mem-path=/dev/zero,align=8192M \
>  -device pc-dimm,id=dimm1,memdev=mem0
> 
> Signed-off-by: David Hildenbrand 
Reviewed-by: Igor Mammedov 

> ---
>  hw/mem/memory-device.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 3e04f3954e..6de4f70bb4 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -116,9 +116,15 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>  address_space_start = ms->device_memory->base;
>  address_space_end = address_space_start +
>  memory_region_size(&ms->device_memory->mr);
> -g_assert(QEMU_ALIGN_UP(address_space_start, align) == 
> address_space_start);
>  g_assert(address_space_end >= address_space_start);
>  
> +/* address_space_start indicates the maximum alignment we expect */
> +if (QEMU_ALIGN_UP(address_space_start, align) != address_space_start) {
> +error_setg(errp, "the alignment (0%" PRIx64 ") is not supported",
> +   align);
> +return 0;
> +}
> +
>  memory_device_check_addable(ms, size, errp);
>  if (*errp) {
>  return 0;

Re: [Qemu-devel] [PATCH v1 2/2] exec: check that alignment is a power of two

[Igor Mammedov]

On Thu,  7 Jun 2018 17:47:05 +0200
David Hildenbrand  wrote:

> Right now we can crash QEMU using e.g.
> 
> qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 \
>  -object 
> memory-backend-file,id=mem0,size=12288,mem-path=/dev/zero,align=12288 \
>  -device pc-dimm,id=dimm1,memdev=mem0
> 
> qemu-system-x86_64: util/mmap-alloc.c:115:
>  qemu_ram_mmap: Assertion `is_power_of_2(align)' failed
> 
> Fix this by adding a proper check.
> 
> Signed-off-by: David Hildenbrand 
Reviewed-by: Igor Mammedov 

> ---
>  exec.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index f6645ede0c..f54c83ac61 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1681,6 +1681,10 @@ static void *file_ram_alloc(RAMBlock *block,
> " must be multiples of page size 0x%zx",
> block->mr->align, block->page_size);
>  return NULL;
> +} else if (block->mr->align && !is_power_of_2(block->mr->align)) {
> +error_setg(errp, "alignment 0x%" PRIx64
> +   " must be a power of two", block->mr->align);
> +return NULL;
>  }
>  block->mr->align = MAX(block->page_size, block->mr->align);
>  #if defined(__s390x__)

[Qemu-devel] [PATCH v8 3/6] migration: API to clear bits of guest free pages from the dirty bitmap

[Wei Wang]

This patch adds an API to clear bits corresponding to guest free pages
from the dirty bitmap. Spilt the free page block if it crosses the QEMU
RAMBlock boundary.

Signed-off-by: Wei Wang 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
CC: Michael S. Tsirkin 
CC: Peter Xu 
---
 include/migration/misc.h |  2 ++
 migration/migration.c|  2 +-
 migration/migration.h|  1 +
 migration/ram.c  | 48 
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 4ebf24c..113320e 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -14,11 +14,13 @@
 #ifndef MIGRATION_MISC_H
 #define MIGRATION_MISC_H
 
+#include "exec/cpu-common.h"
 #include "qemu/notify.h"
 
 /* migration/ram.c */
 
 void ram_mig_init(void);
+void qemu_guest_free_page_hint(void *addr, size_t len);
 
 /* migration/block.c */
 
diff --git a/migration/migration.c b/migration/migration.c
index 05aec2c..220ff48 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -647,7 +647,7 @@ MigrationParameters *qmp_query_migrate_parameters(Error 
**errp)
  * Return true if we're already in the middle of a migration
  * (i.e. any of the active or setup states)
  */
-static bool migration_is_setup_or_active(int state)
+bool migration_is_setup_or_active(int state)
 {
 switch (state) {
 case MIGRATION_STATUS_ACTIVE:
diff --git a/migration/migration.h b/migration/migration.h
index 8f0c821..5a74740 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -230,6 +230,7 @@ void migrate_fd_error(MigrationState *s, const Error 
*error);
 void migrate_fd_connect(MigrationState *s, Error *error_in);
 
 void migrate_init(MigrationState *s);
+bool migration_is_setup_or_active(int state);
 bool migration_is_blocked(Error **errp);
 /* True if outgoing migration has entered postcopy phase */
 bool migration_in_postcopy(void);
diff --git a/migration/ram.c b/migration/ram.c
index 2eabbe9..237f11e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2530,6 +2530,54 @@ static void ram_state_resume_prepare(RAMState *rs, 
QEMUFile *out)
 }
 
 /*
+ * This function clears bits of the free pages reported by the caller from the
+ * migration dirty bitmap. @addr is the host address corresponding to the
+ * start of the continuous guest free pages, and @len is the total bytes of
+ * those pages.
+ */
+void qemu_guest_free_page_hint(void *addr, size_t len)
+{
+RAMBlock *block;
+ram_addr_t offset;
+size_t used_len, start, npages;
+MigrationState *s = migrate_get_current();
+
+/* This function is currently expected to be used during live migration */
+if (!migration_is_setup_or_active(s->state)) {
+return;
+}
+
+for (; len > 0; len -= used_len) {
+block = qemu_ram_block_from_host(addr, false, &offset);
+assert(block);
+
+/*
+ * This handles the case that the RAMBlock is resized after the free
+ * page hint is reported.
+ */
+if (unlikely(offset > block->used_length)) {
+return;
+}
+
+if (len <= block->used_length - offset) {
+used_len = len;
+} else {
+used_len = block->used_length - offset;
+addr += used_len;
+}
+
+start = offset >> TARGET_PAGE_BITS;
+npages = used_len >> TARGET_PAGE_BITS;
+
+qemu_mutex_lock(&ram_state->bitmap_mutex);
+ram_state->migration_dirty_pages -=
+  bitmap_count_one_with_offset(block->bmap, start, npages);
+bitmap_clear(block->bmap, start, npages);
+qemu_mutex_unlock(&ram_state->bitmap_mutex);
+}
+}
+
+/*
  * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
  * long-running RCU critical section.  When rcu-reclaims in the code
  * start to become numerous it will be necessary to reduce the
-- 
1.8.3.1

[Qemu-devel] [PATCH v8 0/6] virtio-balloon: free page hint reporting support

[Wei Wang]

This is the deivce part implementation to add a new feature,
VIRTIO_BALLOON_F_FREE_PAGE_HINT to the virtio-balloon device. The device
receives the guest free page hints from the driver and clears the
corresponding bits in the dirty bitmap, so that those free pages are
not transferred by the migration thread to the destination.

- Test Environment
Host: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz
Guest: 8G RAM, 4 vCPU
Migration setup: migrate_set_speed 100G, migrate_set_downtime 2 second

- Test Results
- Idle Guest Live Migration Time (results are averaged over 10 runs):
- Optimization v.s. Legacy = 271ms vs 1769ms --> ~86% reduction
- Guest with Linux Compilation Workload (make bzImage -j4):
- Live Migration Time (average)
  Optimization v.s. Legacy = 1265ms v.s. 2634ms --> ~51% reduction
- Linux Compilation Time
  Optimization v.s. Legacy = 4min56s v.s. 5min3s
  --> no obvious difference

- Source Code
- QEMU:  https://github.com/wei-w-wang/qemu-free-page-lm.git
- Linux: https://github.com/wei-w-wang/linux-free-page-lm.git

ChangeLog:
v7->v8:
1) migration:
- qemu_guest_free_page_hint: 
- check if this function is called during migration;
- assert when the block of a given hint is not found;
- add a ram save state notifier list;
- move migrate_postcopy to an outside header for other
  subsystems (e.g. notifier callbacks) to use;
 2) virtio-balloon:
- start/stop the optimization via a notifier, and reduce the
  related global balloon interfaces;
- virtio_balloon_poll_free_page_hints: move the while loop into
  a function;
- put page poison value into a separate vmsd subsection.
v6->v7:
  virtio-balloon/virtio_balloo_poll_free_page_hints:
  - add virtio_notify() at the end to notify the driver that
the optimization is done, which indicates that the entries
have all been put back to the vq and ready to detach them.
v5->v6:
  virtio-balloon: use iothread to get free page hint
v4->v5:
1) migration:
- bitmap_clear_dirty: update the dirty bitmap and dirty page
  count under the bitmap mutex as what other functions are doing;
- qemu_guest_free_page_hint:
- add comments for this function;
- check the !block case;
- check "offset > block->used_length" before proceed;
- assign used_len inside the for{} body;
- update the dirty bitmap and dirty page counter under the
  bitmap mutex;
- ram_state_reset:
- rs->free_page_support: && with use "migrate_postcopy"
  instead of migration_in_postcopy;
- clear the ram_bulk_stage flag if free_page_support is true;
2) balloon:
 - add the usage documentation of balloon_free_page_start and
   balloon_free_page_stop in code;
 - the optimization thread is named "balloon_fpo" to meet the
   requirement of "less than 14 characters";
 - virtio_balloon_poll_free_page_hints:
 - run on condition when runstate_is_running() is true;
 - add a qemu spin lock to synchronize accesses to the free
   page reporting related fields shared among the migration
   thread and the optimization thread;
  - virtio_balloon_free_page_start: just return if
runstate_is_running is false;
  - virtio_balloon_free_page_stop: access to the free page
reporting related fields under a qemu spin lock;
  - virtio_balloon_device_unrealize/reset: call
virtio_balloon_free_page_stop is the free page hint feature is
used;
  - virtio_balloon_set_status: call irtio_balloon_free_page_stop
in case the guest is stopped by qmp when the optimization is
running;
v3->v4:
1) bitmap: add a new API to count 1s starting from an offset of a
   bitmap
2) migration:
- qemu_guest_free_page_hint: calculate
  ram_state->migration_dirty_pages by counting how many bits of
  free pages are truely cleared. If some of the bits were
  already 0, they shouldn't be deducted by
  ram_state->migration_dirty_pages. This wasn't needed for
  previous versions since we optimized bulk stage only,
  where all bits are guaranteed to be set. It's needed now
  because we extened the usage of this optimizaton to all stages
  except the last stop© stage. From 2nd stage onward, there
  are possibilities that some bits of free pages are already 0.
 3) virtio-balloon:
 - virtio_balloon_free_page_report_status: introduce a new status,
   FREE_PAGE_REPORT_S_EXIT. This status indicates that the
   optimization thread has exited. FREE_PAGE_REPORT_S_STOP means
   the reporting is stopped, but the op

[Qemu-devel] [PATCH v8 1/6] bitmap: bitmap_count_one_with_offset

[Wei Wang]

Count the number of 1s in a bitmap starting from an offset.

Signed-off-by: Wei Wang 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
CC: Michael S. Tsirkin 
Reviewed-by: Dr. David Alan Gilbert 
---
 include/qemu/bitmap.h | 13 +
 1 file changed, 13 insertions(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 509eedd..e3f31f1 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -228,6 +228,19 @@ static inline long bitmap_count_one(const unsigned long 
*bitmap, long nbits)
 }
 }
 
+static inline long bitmap_count_one_with_offset(const unsigned long *bitmap,
+long offset, long nbits)
+{
+long aligned_offset = QEMU_ALIGN_DOWN(offset, BITS_PER_LONG);
+long redundant_bits = offset - aligned_offset;
+long bits_to_count = nbits + redundant_bits;
+const unsigned long *bitmap_start = bitmap +
+aligned_offset / BITS_PER_LONG;
+
+return bitmap_count_one(bitmap_start, bits_to_count) -
+   bitmap_count_one(bitmap_start, redundant_bits);
+}
+
 void bitmap_set(unsigned long *map, long i, long len);
 void bitmap_set_atomic(unsigned long *map, long i, long len);
 void bitmap_clear(unsigned long *map, long start, long nr);
-- 
1.8.3.1

[Qemu-devel] [PATCH v8 5/6] migration: move migrate_postcopy() to include/migration/misc.h

[Wei Wang]

The ram save state notifier callback, for example the free page
optimization offerred by virtio-balloon, may need to check if
postcopy is in use, so move migrate_postcopy() to the outside header.

Signed-off-by: Wei Wang 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
CC: Michael S. Tsirkin 
CC: Peter Xu 
---
 include/migration/misc.h | 1 +
 migration/migration.h| 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index b970d7d..911aaf3 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -109,6 +109,7 @@ bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
 void migration_global_dump(Monitor *mon);
+bool migrate_postcopy(void);
 
 /* migration/block-dirty-bitmap.c */
 void dirty_bitmap_mig_init(void);
diff --git a/migration/migration.h b/migration/migration.h
index 5a74740..fee5af8 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -236,8 +236,6 @@ bool migration_is_blocked(Error **errp);
 bool migration_in_postcopy(void);
 MigrationState *migrate_get_current(void);
 
-bool migrate_postcopy(void);
-
 bool migrate_release_ram(void);
 bool migrate_postcopy_ram(void);
 bool migrate_zero_blocks(void);
-- 
1.8.3.1

[Qemu-devel] [PATCH v8 2/6] migration: use bitmap_mutex in migration_bitmap_clear_dirty

[Wei Wang]

The bitmap mutex is used to synchronize threads to update the dirty
bitmap and the migration_dirty_pages counter. For example, the free
page optimization clears bits of free pages from the bitmap in an
iothread context. This patch makes migration_bitmap_clear_dirty update
the bitmap and counter under the mutex.

Signed-off-by: Wei Wang 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
CC: Michael S. Tsirkin 
CC: Peter Xu 
---
 migration/ram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index c53e836..2eabbe9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1093,11 +1093,14 @@ static inline bool 
migration_bitmap_clear_dirty(RAMState *rs,
 {
 bool ret;
 
+qemu_mutex_lock(&rs->bitmap_mutex);
 ret = test_and_clear_bit(page, rb->bmap);
 
 if (ret) {
 rs->migration_dirty_pages--;
 }
+qemu_mutex_unlock(&rs->bitmap_mutex);
+
 return ret;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Igor Mammedov]

On Fri, 8 Jun 2018 10:07:31 +0200
Thomas Huth  wrote:

> On 08.06.2018 09:48, David Hildenbrand wrote:
> > On 08.06.2018 09:46, Greg Kurz wrote:  
> >> On Fri, 8 Jun 2018 09:42:48 +0200
> >> David Hildenbrand  wrote:
> >>  
> >>> On 08.06.2018 09:34, Greg Kurz wrote:  
>  On Thu,  7 Jun 2018 18:52:12 +0200
>  David Hildenbrand  wrote:
>  
> > The node property can always be queried and the value has already been
> > verified in pc_dimm_realize().
> >
> > Signed-off-by: David Hildenbrand 
> > ---
> >  hw/ppc/spapr.c | 9 +
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2375cbee12..d038f3243e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3578,14 +3578,7 @@ static void 
> > spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >  error_setg(errp, "Memory hotplug not supported for this 
> > machine");
> >  return;
> >  }
> > -node = object_property_get_uint(OBJECT(dev), 
> > PC_DIMM_NODE_PROP, errp);
> > -if (*errp) {
> 
>  Good riddance :)
>  
> > -return;
> > -}
> > -if (node < 0 || node >= MAX_NODES) {
> > -error_setg(errp, "Invaild node %d", node);
> > -return;
> > -}  
> 
> Maybe turn that into an assert() instead? ... just for the paranoids ;-)
> 
> > +node = object_property_get_uint(OBJECT(dev), 
> > PC_DIMM_NODE_PROP, NULL);
> 
>  Maybe pass &error_abort ?
> >>>
> >>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> >>>
> >>> ("error ignored" vs. "error leads to an abort") - but this will actually
> >>> never fail. But I can use error_abort here, does not matter.
> >>>  
> >>
> >> Heh, /me paranoid but this is David's call and he acked that already
> >> so I guess it's okay.  
> > 
> > NULL makes it fit into a single line :)  
> 
> +1 for error_abort, even if it takes another line.
+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored
and introduce undefined behavior.

> 
>  Thomas

[Qemu-devel] [PATCH v8 4/6] migration/ram.c: add ram save state notifiers

[Wei Wang]

This patch adds a ram save state notifier list, and expose RAMState for
the notifer callbacks to use.

Signed-off-by: Wei Wang 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
CC: Michael S. Tsirkin 
CC: Peter Xu 
---
 include/migration/misc.h | 52 +++
 migration/ram.c  | 64 +---
 2 files changed, 75 insertions(+), 41 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 113320e..b970d7d 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -16,9 +16,61 @@
 
 #include "exec/cpu-common.h"
 #include "qemu/notify.h"
+#include "qemu/thread.h"
 
 /* migration/ram.c */
 
+typedef enum RamSaveState {
+RAM_SAVE_ERR = 0,
+RAM_SAVE_RESET = 1,
+RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
+RAM_SAVE_AFTER_SYNC_BITMAP = 3,
+RAM_SAVE_MAX = 4,
+} RamSaveState;
+
+/* State of RAM for migration */
+typedef struct RAMState {
+/* QEMUFile used for this migration */
+QEMUFile *f;
+/* Last block that we have visited searching for dirty pages */
+RAMBlock *last_seen_block;
+/* Last block from where we have sent data */
+RAMBlock *last_sent_block;
+/* Last dirty target page we have sent */
+ram_addr_t last_page;
+/* last ram version we have seen */
+uint32_t last_version;
+/* We are in the first round */
+bool ram_bulk_stage;
+/* How many times we have dirty too many pages */
+int dirty_rate_high_cnt;
+/* ram save states used for notifiers */
+int ram_save_state;
+/* these variables are used for bitmap sync */
+/* last time we did a full bitmap_sync */
+int64_t time_last_bitmap_sync;
+/* bytes transferred at start_time */
+uint64_t bytes_xfer_prev;
+/* number of dirty pages since start_time */
+uint64_t num_dirty_pages_period;
+/* xbzrle misses since the beginning of the period */
+uint64_t xbzrle_cache_miss_prev;
+/* number of iterations at the beginning of period */
+uint64_t iterations_prev;
+/* Iterations since start */
+uint64_t iterations;
+/* number of dirty bits in the bitmap */
+uint64_t migration_dirty_pages;
+/* protects modification of the bitmap */
+QemuMutex bitmap_mutex;
+/* The RAMBlock used in the last src_page_requests */
+RAMBlock *last_req_rb;
+/* Queue of outstanding page requests from the destination */
+QemuMutex src_page_req_mutex;
+QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
+} RAMState;
+
+void add_ram_save_state_change_notifier(Notifier *notify);
 void ram_mig_init(void);
 void qemu_guest_free_page_hint(void *addr, size_t len);
 
diff --git a/migration/ram.c b/migration/ram.c
index 237f11e..d45b5a4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -56,6 +56,9 @@
 #include "qemu/uuid.h"
 #include "savevm.h"
 
+static NotifierList ram_save_state_notifiers =
+NOTIFIER_LIST_INITIALIZER(ram_save_state_notifiers);
+
 /***/
 /* ram save/restore */
 
@@ -267,49 +270,18 @@ struct RAMSrcPageRequest {
 QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
 };
 
-/* State of RAM for migration */
-struct RAMState {
-/* QEMUFile used for this migration */
-QEMUFile *f;
-/* Last block that we have visited searching for dirty pages */
-RAMBlock *last_seen_block;
-/* Last block from where we have sent data */
-RAMBlock *last_sent_block;
-/* Last dirty target page we have sent */
-ram_addr_t last_page;
-/* last ram version we have seen */
-uint32_t last_version;
-/* We are in the first round */
-bool ram_bulk_stage;
-/* How many times we have dirty too many pages */
-int dirty_rate_high_cnt;
-/* these variables are used for bitmap sync */
-/* last time we did a full bitmap_sync */
-int64_t time_last_bitmap_sync;
-/* bytes transferred at start_time */
-uint64_t bytes_xfer_prev;
-/* number of dirty pages since start_time */
-uint64_t num_dirty_pages_period;
-/* xbzrle misses since the beginning of the period */
-uint64_t xbzrle_cache_miss_prev;
-/* number of iterations at the beginning of period */
-uint64_t iterations_prev;
-/* Iterations since start */
-uint64_t iterations;
-/* number of dirty bits in the bitmap */
-uint64_t migration_dirty_pages;
-/* protects modification of the bitmap */
-QemuMutex bitmap_mutex;
-/* The RAMBlock used in the last src_page_requests */
-RAMBlock *last_req_rb;
-/* Queue of outstanding page requests from the destination */
-QemuMutex src_page_req_mutex;
-QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
-};
-typedef struct RAMState RAMState;
-
 static RAMState *ram_state;
 
+void add_ram_save_state_change_notifier(Notifier *notify)
+{
+notifier_list_add(&ram_save_state_notifiers, notify);
+}
+
+static void notify_ram_save_state_change_notifier(void)

Re: [Qemu-devel] [PATCH v1 4/8] spapr: local error handling in hotplug handler functions

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:14 +0200
David Hildenbrand  wrote:

> Let's introduce and use local error variables in the hotplug handler
> functions.
> 
> Signed-off-by: David Hildenbrand 
> ---

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr.c | 29 -
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a12be24ca9..4447cb197f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3576,11 +3576,14 @@ out:
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>DeviceState *dev, Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -spapr_memory_plug(hotplug_dev, dev, errp);
> +spapr_memory_plug(hotplug_dev, dev, &local_err);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -spapr_core_plug(hotplug_dev, dev, errp);
> +spapr_core_plug(hotplug_dev, dev, &local_err);
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> @@ -3588,10 +3591,11 @@ static void 
> spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>  {
>  sPAPRMachineState *sms = SPAPR_MACHINE(OBJECT(hotplug_dev));
>  MachineClass *mc = MACHINE_GET_CLASS(sms);
> +Error *local_err = NULL;
>  
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>  if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> -spapr_memory_unplug_request(hotplug_dev, dev, errp);
> +spapr_memory_unplug_request(hotplug_dev, dev, &local_err);
>  } else {
>  /* NOTE: this means there is a window after guest reset, prior to
>   * CAS negotiation, where unplug requests will fail due to the
> @@ -3599,25 +3603,32 @@ static void 
> spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>   * the case with PCI unplug, where the events will be queued and
>   * eventually handled by the guest after boot
>   */
> -error_setg(errp, "Memory hot unplug not supported for this 
> guest");
> +error_setg(&local_err,
> +   "Memory hot unplug not supported for this guest");
>  }
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>  if (!mc->has_hotpluggable_cpus) {
> -error_setg(errp, "CPU hot unplug not supported on this machine");
> -return;
> +error_setg(&local_err,
> +   "CPU hot unplug not supported on this machine");
> +goto out;
>  }
> -spapr_core_unplug_request(hotplug_dev, dev, errp);
> +spapr_core_unplug_request(hotplug_dev, dev, &local_err);
>  }
> +out:
> +error_propagate(errp, local_err);
>  }
>  
>  static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>DeviceState *dev, Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -spapr_memory_pre_plug(hotplug_dev, dev, errp);
> +spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -spapr_core_pre_plug(hotplug_dev, dev, errp);
> +spapr_core_pre_plug(hotplug_dev, dev, &local_err);
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

[Qemu-devel] [PATCH v8 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang]

The new feature enables the virtio-balloon device to receive hints of
guest free pages from the free page vq.

A notifier is registered to the migration ram save state notifier list.
The notifier calls free_page_start after the migration thread syncs the
dirty bitmap, so that the free page hinting optimization starts to clear
bits of free pages from the bitmap. It calls the free_page_stop
before the migration thread syncs the bitmap, which is the end of the
current round of ram save. The free_page_stop is also called to stop the
optimization in the case there is an error happened in the process of
ram save.

Note: balloon will report pages which were free at the time of this call.
As the reporting happens asynchronously, dirty bit logging must be
enabled before this free_page_start call is made. Guest reporting must be
disabled before the migration dirty bitmap is synchronized.

TODO:
- If free pages are poisoned by guest, the hints are dropped currently.
  We will support clearing bits of poisoned pages from the bitmap in the
  future.

Signed-off-by: Wei Wang 
CC: Michael S. Tsirkin 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
CC: Peter Xu 
---
 hw/virtio/virtio-balloon.c  | 260 
 include/hw/virtio/virtio-balloon.h  |  28 ++-
 include/standard-headers/linux/virtio_balloon.h |   7 +
 3 files changed, 294 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1f7a87f..a7bb971 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -28,6 +28,7 @@
 #include "qapi/visitor.h"
 #include "trace.h"
 #include "qemu/error-report.h"
+#include "migration/misc.h"
 
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
@@ -310,6 +311,166 @@ out:
 }
 }
 
+static void get_free_page_hints(VirtIOBalloon *dev)
+{
+VirtQueueElement *elem;
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VirtQueue *vq = dev->free_page_vq;
+uint32_t id;
+size_t size;
+
+while (dev->block_iothread) {
+qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
+}
+
+/*
+ * If the migration thread actively stops the reporting, exit
+ * immediately.
+ */
+if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
+return;
+}
+
+elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+if (!elem) {
+return;
+}
+
+if (elem->out_num) {
+size = iov_to_buf(elem->out_sg, elem->out_num, 0, &id, sizeof(id));
+virtqueue_push(vq, elem, size);
+g_free(elem);
+
+virtio_tswap32s(vdev, &id);
+if (unlikely(size != sizeof(id))) {
+virtio_error(vdev, "received an incorrect cmd id");
+return;
+}
+if (id == dev->free_page_report_cmd_id) {
+dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+} else {
+/*
+ * Stop the optimization only when it has started. This
+ * avoids a stale stop sign for the previous command.
+ */
+if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+return;
+}
+}
+}
+
+if (elem->in_num) {
+/* TODO: send the poison value to the destination */
+if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START &&
+!dev->poison_val) {
+qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
+  elem->in_sg[0].iov_len);
+}
+virtqueue_push(vq, elem, 0);
+g_free(elem);
+}
+}
+
+static void virtio_balloon_poll_free_page_hints(void *opaque)
+{
+VirtIOBalloon *dev = opaque;
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VirtQueue *vq = dev->free_page_vq;
+
+while (dev->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+qemu_mutex_lock(&dev->free_page_lock);
+get_free_page_hints(dev);
+qemu_mutex_unlock(&dev->free_page_lock);
+}
+virtio_notify(vdev, vq);
+}
+
+static bool virtio_balloon_free_page_support(void *opaque)
+{
+VirtIOBalloon *s = opaque;
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+}
+
+static bool virtio_balloon_page_poison_support(void *opaque)
+{
+VirtIOBalloon *s = opaque;
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+}
+
+static void virtio_balloon_free_page_start(void *opaque)
+{
+VirtIOBalloon *s = opaque;
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+/* For the stop and copy phase, we don't need to start the optimization */
+if (!vdev->vm_running) {
+return;
+}
+
+if (s->free_page_report_cmd_id == UINT_MAX) {
+s->free_page_report_cmd_id =
+   VIRTIO_BALLOON_FREE_

Re: [Qemu-devel] storing machine data in qcow images?

[Daniel P . Berrangé]

On Fri, Jun 08, 2018 at 09:21:30AM +0100, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (ler...@redhat.com) wrote:
> > On 06/07/18 12:54, Andrea Bolognani wrote:
> > > On Thu, 2018-06-07 at 11:36 +0100, Daniel P. Berrangé wrote:
> > >> On Thu, Jun 07, 2018 at 11:32:18AM +0100, Richard W.M. Jones wrote:
> > >>> Another problem which Laszlo mentioned is the varstore isn't portable
> > >>> between UEFI implementations, or if the UEFI is compiled with
> > >>> different options.  You can even imagine shipping multiple
> > >>> varstores(!) which argues for a tar-like format.
> > >>
> > >> Could we perhaps imagine shipping the actual UEFI bios, rather
> > >> than only the varstore.  The bios blob runs in guest context,
> > >> so there shouldn't be able security concerns from hosting
> > >> vendors with running user provided bios. Mostly its a matter
> > >> of confidence that the interface between bios & qemu is stable
> > >> which feels easier than assuming varstore vs different bios is
> > >> portable.
> > > 
> > > That sounds sensible, and further reinforces the idea that we
> > > need way more than a single string baked into the qcow2 file.
> > > 
> > 
> > Sorry for arriving late (thanks Rich for the Fwd).
> > 
> > The contents of the non-volatile UEFI variables should be considered
> > part of (permanent) guest state, such as disk contents. Therefore I'd
> > argue for bundling the varstore file with the disk image(s).
> > 
> > In turn, the best way to ensure comaptibility between varstore and
> > firmware binary is to just bundle the firmware binary as well. It's
> > generally not large (x86) or if it is, it compresses extremely well
> > (aarch64). For extra politeness, image providers can bundle a text file
> > with their firmware build options (like a kernel config), possibly even
> > a JSON document conforming to the new firmware schema (qemu commit
> > 3a0adfc9bfcf), but that's not a hard requirement I guess.
> > 
> > If such a VM is to be migrated between hosts, I'd expect the host admin
> > to take care of installing the fw binary on all eligible hosts.
> 
> There's no way they can do that if they're just importing VMs from
> templates that include the image; who is going to keep track of which
> BIOSs are needed where?

It isn't that unusual a requirement. When Openstack deploys a VM, it
has the user provided image as a base file, and then creates  qcow2
overlay.  If the VM is cold migrated (ie not running) to another
host, OpenStack has to make sure the same base file gets copied across
to the new host so that the overlay still works. Copying the BIOS file
and vars state across at the same time is no more difficult than what
its already doing.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH] typedefs: add QJSON

[Greg Kurz]

Since commit 83ee768d6247b, we now have two places that define the
QJSON type:

$ git grep 'typedef struct QJSON QJSON'
include/migration/vmstate.h:typedef struct QJSON QJSON;
migration/qjson.h:typedef struct QJSON QJSON;

This breaks docker-test-build@centos6:

In file included from /tmp/qemu-test/src/migration/savevm.c:59:
/tmp/qemu-test/src/migration/qjson.h:16: error: redefinition of typedef
 'QJSON'
/tmp/qemu-test/src/include/migration/vmstate.h:30: note: previous
 declaration of 'QJSON' was here
make: *** [migration/savevm.o] Error 1

This happens because CentOS 6 has an old GCC 4.4.7. Even if redefining
a typedef with the same type is permitted since GCC 4.6, unless -pedantic
is passed, we don't really need to do that on purpose. Let's have a
single definition in  instead.

Signed-off-by: Greg Kurz 
---
 include/migration/vmstate.h |2 --
 include/qemu/typedefs.h |1 +
 migration/qjson.h   |2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 3747110f951a..42b946ce902c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -27,8 +27,6 @@
 #ifndef QEMU_VMSTATE_H
 #define QEMU_VMSTATE_H
 
-typedef struct QJSON QJSON;
-
 typedef struct VMStateInfo VMStateInfo;
 typedef struct VMStateDescription VMStateDescription;
 typedef struct VMStateField VMStateField;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 325c72de3313..3ec0e13a967f 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -97,6 +97,7 @@ typedef struct QEMUTimer QEMUTimer;
 typedef struct QEMUTimerListGroup QEMUTimerListGroup;
 typedef struct QBool QBool;
 typedef struct QDict QDict;
+typedef struct QJSON QJSON;
 typedef struct QList QList;
 typedef struct QNull QNull;
 typedef struct QNum QNum;
diff --git a/migration/qjson.h b/migration/qjson.h
index 2978b5f37121..41664f2d71b5 100644
--- a/migration/qjson.h
+++ b/migration/qjson.h
@@ -13,8 +13,6 @@
 #ifndef QEMU_QJSON_H
 #define QEMU_QJSON_H
 
-typedef struct QJSON QJSON;
-
 QJSON *qjson_new(void);
 void qjson_destroy(QJSON *json);
 void json_prop_str(QJSON *json, const char *name, const char *str);

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 10:39, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 10:07:31 +0200
> Thomas Huth  wrote:
> 
>> On 08.06.2018 09:48, David Hildenbrand wrote:
>>> On 08.06.2018 09:46, Greg Kurz wrote:  
 On Fri, 8 Jun 2018 09:42:48 +0200
 David Hildenbrand  wrote:
  
> On 08.06.2018 09:34, Greg Kurz wrote:  
>> On Thu,  7 Jun 2018 18:52:12 +0200
>> David Hildenbrand  wrote:
>> 
>>> The node property can always be queried and the value has already been
>>> verified in pc_dimm_realize().
>>>
>>> Signed-off-by: David Hildenbrand 
>>> ---
>>>  hw/ppc/spapr.c | 9 +
>>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 2375cbee12..d038f3243e 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -3578,14 +3578,7 @@ static void 
>>> spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>>>  error_setg(errp, "Memory hotplug not supported for this 
>>> machine");
>>>  return;
>>>  }
>>> -node = object_property_get_uint(OBJECT(dev), 
>>> PC_DIMM_NODE_PROP, errp);
>>> -if (*errp) {
>>
>> Good riddance :)
>> 
>>> -return;
>>> -}
>>> -if (node < 0 || node >= MAX_NODES) {
>>> -error_setg(errp, "Invaild node %d", node);
>>> -return;
>>> -}  
>>
>> Maybe turn that into an assert() instead? ... just for the paranoids ;-)
>>
>>> +node = object_property_get_uint(OBJECT(dev), 
>>> PC_DIMM_NODE_PROP, NULL);
>>
>> Maybe pass &error_abort ?
>
> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>
> ("error ignored" vs. "error leads to an abort") - but this will actually
> never fail. But I can use error_abort here, does not matter.
>  

 Heh, /me paranoid but this is David's call and he acked that already
 so I guess it's okay.  
>>>
>>> NULL makes it fit into a single line :)  
>>
>> +1 for error_abort, even if it takes another line.
> +1 for error_abort
> call shouldn't fail, but if does it won't be silently ignored
> and introduce undefined behavior.

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

> 
>>
>>  Thomas
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[Igor Mammedov]

On Fri, 8 Jun 2018 10:07:59 +0200
David Hildenbrand  wrote:

> On 08.06.2018 10:05, Greg Kurz wrote:
> > On Thu,  7 Jun 2018 18:52:13 +0200
> > David Hildenbrand  wrote:
> >   
> >> Let's clean the hotplug handler up by moving everything into
> >> spapr_memory_plug().
> >>
> >> Signed-off-by: David Hildenbrand 
> >> ---
> >>  hw/ppc/spapr.c | 23 ++-
> >>  1 file changed, 10 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index d038f3243e..a12be24ca9 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, 
> >> uint64_t addr_start, uint64_t size,
> >>  }
> >>  
> >>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState 
> >> *dev,
> >> -  uint32_t node, Error **errp)
> >> +  Error **errp)
> >>  {
> >>  Error *local_err = NULL;
> >>  sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> >> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> >>  PCDIMMDevice *dimm = PC_DIMM(dev);
> >>  PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >>  MemoryRegion *mr;
> >>  uint64_t align, size, addr;
> >> +uint32_t node;
> >> +
> >> +if (!smc->dr_lmb_enabled) {
> >> +error_setg(&local_err, "Memory hotplug not supported for this 
> >> machine");
> >> +goto out;
> >> +}  
> > 
> > Wouldn't it be more appropriate to move this check to 
> > spapr_memory_pre_plug() ?  
> 
> Think you're right (and as spapr_memory_pre_plug() already exists, it's
> easy), other opinions? Thanks.
I also think that it should go to preplug

> 
> >   
> >> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
> >>  
> >>  mr = ddc->get_memory_region(dimm, &local_err);
> >>  if (local_err) {
> >> @@ -3568,19 +3576,8 @@ out:
> >>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >>DeviceState *dev, Error **errp)
> >>  {
> >> -MachineState *ms = MACHINE(hotplug_dev);
> >> -sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> >> -
> >>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >> -int node;
> >> -
> >> -if (!smc->dr_lmb_enabled) {
> >> -error_setg(errp, "Memory hotplug not supported for this 
> >> machine");
> >> -return;
> >> -}
> >> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, 
> >> NULL);
> >> -
> >> -spapr_memory_plug(hotplug_dev, dev, node, errp);
> >> +spapr_memory_plug(hotplug_dev, dev, errp);
> >>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> >>  spapr_core_plug(hotplug_dev, dev, errp);
> >>  }  
> >   
> 
> 

Re: [Qemu-devel] [RFC v2 04/12] Add vhost-user-backend

[Daniel P . Berrangé]

On Fri, Jun 08, 2018 at 12:34:15AM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jun 4, 2018 at 11:36 AM, Daniel P. Berrangé  
> wrote:
> > On Fri, Jun 01, 2018 at 06:27:41PM +0200, Marc-André Lureau wrote:
> >> Create a vhost-user-backend object that holds a connection to a
> >> vhost-user backend and can be referenced from virtio devices that
> >> support it. See later patches for input & gpu usage.
> >>
> >> A chardev can be specified to communicate with the vhost-user backend,
> >> ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
> >> vhost-user-backend,id=vuid,chardev=char0.
> >>
> >> Alternatively, an executable with its arguments may be given as 'cmd'
> >> property, ex: -object
> >> vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
> >> executable is then spawn and, by convention, the vhost-user socket is
> >> passed as fd=3. It may be considered a security breach to allow
> >> creating processes that may execute arbitrary executables, so this may
> >> be restricted to some known executables (via signature etc) or
> >> directory.
> >
> > Passing a binary and args as a string blob.
> >
> >> +static int
> >> +vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error 
> >> **errp)
> >> +{
> >> +int devnull = open("/dev/null", O_RDWR);
> >> +pid_t pid;
> >> +
> >> +assert(!b->child);
> >> +
> >> +if (!b->cmd) {
> >> +error_setg_errno(errp, errno, "Missing cmd property");
> >> +return -1;
> >> +}
> >> +if (devnull < 0) {
> >> +error_setg_errno(errp, errno, "Unable to open /dev/null");
> >> +return -1;
> >> +}
> >> +
> >> +pid = qemu_fork(errp);
> >> +if (pid < 0) {
> >> +close(devnull);
> >> +return -1;
> >> +}
> >> +
> >> +if (pid == 0) { /* child */
> >> +int fd, maxfd = sysconf(_SC_OPEN_MAX);
> >> +
> >> +dup2(devnull, STDIN_FILENO);
> >> +dup2(devnull, STDOUT_FILENO);
> >> +dup2(vhostfd, 3);
> >> +
> >> +signal(SIGINT, SIG_IGN);
> >
> > Why ignore SIGINT ?  Surely we want this extra process to be killed
> > someone ctrl-c's the parent QEMU.
> 
> leftover, removed
> 
> >
> >> +
> >> +for (fd = 4; fd < maxfd; fd++) {
> >> +close(fd);
> >> +}
> >> +
> >> +execlp("/bin/sh", "sh", "-c", b->cmd, NULL);
> >
> > ...which is then interpreted by the shell is a recipe for security
> > flaws. There needs to be a way to pass the command + arguments
> > to QEMU as an argv[] we can directly exec without involving the
> > shell.
> >
> 
> For now, I use g_shell_parse_argv(). Do you have a better idea?

Accept individual args at the cli level is far preferrable - we don't
want anything to be parsing shell strings:

 
vhost-user-backend,id=vui,binary=/sbin/vhost-user-input,arg=/dev/input,arg=foo,arg=bar



Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v1 5/8] spapr: introduce machine unplug handler

[Igor Mammedov]

On Thu,  7 Jun 2018 18:52:15 +0200
David Hildenbrand  wrote:

> We'll be handling unplug of e.g. CPUs and PCDIMMs  via the general
> hotplug handler soon, so let's add that handler function.
> 
> Signed-off-by: David Hildenbrand 
Reviewed-by: Igor Mammedov 

> ---
>  hw/ppc/spapr.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4447cb197f..bcb72d9fa7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  error_propagate(errp, local_err);
>  }
>  
> +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> +DeviceState *dev, Error **errp)
> +{
> +}
> +
>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>  DeviceState *dev, Error 
> **errp)
>  {
> @@ -3988,6 +3993,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>  mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
>  mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
>  hc->unplug_request = spapr_machine_device_unplug_request;
> +hc->unplug = spapr_machine_device_unplug;
>  
>  smc->dr_lmb_enabled = true;
>  mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

Re: [Qemu-devel] storing machine data in qcow images?

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Fri, Jun 08, 2018 at 09:21:30AM +0100, Dr. David Alan Gilbert wrote:
> > * Laszlo Ersek (ler...@redhat.com) wrote:
> > > On 06/07/18 12:54, Andrea Bolognani wrote:
> > > > On Thu, 2018-06-07 at 11:36 +0100, Daniel P. Berrangé wrote:
> > > >> On Thu, Jun 07, 2018 at 11:32:18AM +0100, Richard W.M. Jones wrote:
> > > >>> Another problem which Laszlo mentioned is the varstore isn't portable
> > > >>> between UEFI implementations, or if the UEFI is compiled with
> > > >>> different options.  You can even imagine shipping multiple
> > > >>> varstores(!) which argues for a tar-like format.
> > > >>
> > > >> Could we perhaps imagine shipping the actual UEFI bios, rather
> > > >> than only the varstore.  The bios blob runs in guest context,
> > > >> so there shouldn't be able security concerns from hosting
> > > >> vendors with running user provided bios. Mostly its a matter
> > > >> of confidence that the interface between bios & qemu is stable
> > > >> which feels easier than assuming varstore vs different bios is
> > > >> portable.
> > > > 
> > > > That sounds sensible, and further reinforces the idea that we
> > > > need way more than a single string baked into the qcow2 file.
> > > > 
> > > 
> > > Sorry for arriving late (thanks Rich for the Fwd).
> > > 
> > > The contents of the non-volatile UEFI variables should be considered
> > > part of (permanent) guest state, such as disk contents. Therefore I'd
> > > argue for bundling the varstore file with the disk image(s).
> > > 
> > > In turn, the best way to ensure comaptibility between varstore and
> > > firmware binary is to just bundle the firmware binary as well. It's
> > > generally not large (x86) or if it is, it compresses extremely well
> > > (aarch64). For extra politeness, image providers can bundle a text file
> > > with their firmware build options (like a kernel config), possibly even
> > > a JSON document conforming to the new firmware schema (qemu commit
> > > 3a0adfc9bfcf), but that's not a hard requirement I guess.
> > > 
> > > If such a VM is to be migrated between hosts, I'd expect the host admin
> > > to take care of installing the fw binary on all eligible hosts.
> > 
> > There's no way they can do that if they're just importing VMs from
> > templates that include the image; who is going to keep track of which
> > BIOSs are needed where?
> 
> It isn't that unusual a requirement. When Openstack deploys a VM, it
> has the user provided image as a base file, and then creates  qcow2
> overlay.  If the VM is cold migrated (ie not running) to another
> host, OpenStack has to make sure the same base file gets copied across
> to the new host so that the overlay still works. Copying the BIOS file
> and vars state across at the same time is no more difficult than what
> its already doing.

I'm kind of OK with management layers doing it; but Laszlo was
suggesting it was an admins problem;  if we can make it something
manageable by higher levels that's OK.
(Although I'm still concerned that making images with a UEFI image in
that's portable is still not going to work).

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 5/8] spapr: introduce machine unplug handler

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:15 +0200
David Hildenbrand  wrote:

> We'll be handling unplug of e.g. CPUs and PCDIMMs  via the general
> hotplug handler soon, so let's add that handler function.
> 
> Signed-off-by: David Hildenbrand 
> ---

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4447cb197f..bcb72d9fa7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  error_propagate(errp, local_err);
>  }
>  
> +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> +DeviceState *dev, Error **errp)
> +{
> +}
> +
>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>  DeviceState *dev, Error 
> **errp)
>  {
> @@ -3988,6 +3993,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>  mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
>  mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
>  hc->unplug_request = spapr_machine_device_unplug_request;
> +hc->unplug = spapr_machine_device_unplug;
>  
>  smc->dr_lmb_enabled = true;
>  mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers

[David Gibson]

On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan 

It's not clear to me why this is preferable to having the registers
embedded in the state structure.  The latter is pretty standard
practice for qemu.

> ---
>  hw/i2c/ppc4xx_i2c.c | 75 
> +
>  include/hw/i2c/ppc4xx_i2c.h | 19 ++--
>  2 files changed, 43 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index d1936db..a68b5f7 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -3,7 +3,7 @@
>   *
>   * Copyright (c) 2007 Jocelyn Mayer
>   * Copyright (c) 2012 François Revol
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2018 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
> @@ -46,9 +46,26 @@
>  
>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>  
> +typedef struct {
> +uint8_t mdata;
> +uint8_t lmadr;
> +uint8_t hmadr;
> +uint8_t cntl;
> +uint8_t mdcntl;
> +uint8_t sts;
> +uint8_t extsts;
> +uint8_t lsadr;
> +uint8_t hsadr;
> +uint8_t clkdiv;
> +uint8_t intrmsk;
> +uint8_t xfrcnt;
> +uint8_t xtcntlss;
> +uint8_t directcntl;
> +} PPC4xxI2CRegs;
> +
>  static void ppc4xx_i2c_reset(DeviceState *s)
>  {
> -PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> +PPC4xxI2CRegs *i2c = PPC4xx_I2C(s)->regs;
>  
>  /* FIXME: Should also reset bus?
>   *if (s->address != ADDR_RESET) {
> @@ -63,7 +80,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>  i2c->mdcntl = 0;
>  i2c->sts = 0;
>  i2c->extsts = 0x8f;
> -i2c->sdata = 0;
>  i2c->lsadr = 0;
>  i2c->hsadr = 0;
>  i2c->clkdiv = 0;
> @@ -71,7 +87,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>  i2c->xfrcnt = 0;
>  i2c->xtcntlss = 0;
>  i2c->directcntl = 0xf;
> -i2c->intr = 0;
>  }
>  
>  static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
> @@ -81,13 +96,14 @@ static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState 
> *i2c)
>  
>  static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int 
> size)
>  {
> -PPC4xxI2CState *i2c = PPC4xx_I2C(opaque);
> +PPC4xxI2CState *s = PPC4xx_I2C(opaque);
> +PPC4xxI2CRegs *i2c = s->regs;
>  uint64_t ret;
>  
>  switch (addr) {
>  case 0:
>  ret = i2c->mdata;
> -if (ppc4xx_i2c_is_master(i2c)) {
> +if (ppc4xx_i2c_is_master(s)) {
>  ret = 0xff;
>  
>  if (!(i2c->sts & IIC_STS_MDBS)) {
> @@ -98,7 +114,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr 
> addr, unsigned int size)
>  int pending = (i2c->cntl >> 4) & 3;
>  
>  /* get the next byte */
> -int byte = i2c_recv(i2c->bus);
> +int byte = i2c_recv(s->bus);
>  
>  if (byte < 0) {
>  qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
> @@ -113,13 +129,13 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr 
> addr, unsigned int size)
>  
>  if (!pending) {
>  i2c->sts &= ~IIC_STS_MDBS;
> -/*i2c_end_transfer(i2c->bus);*/
> +/*i2c_end_transfer(s->bus);*/
>  /*} else if (i2c->cntl & (IIC_CNTL_RPST | IIC_CNTL_CHT)) {*/
>  } else if (pending) {
>  /* current smbus implementation doesn't like
> multibyte xfer repeated start */
> -i2c_end_transfer(i2c->bus);
> -if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
> +i2c_end_transfer(s->bus);
> +if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
>  /* if non zero is returned, the adress is not valid 
> */
>  i2c->sts &= ~IIC_STS_PT;
>  i2c->sts |= IIC_STS_ERR;
> @@ -139,9 +155,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr 
> addr, unsigned int size)
>TYPE_PPC4xx_I2C, __func__);
>  }
>  break;
> -case 2:
> -ret = i2c->sdata;
> -break;
>  case 4:
>  ret = i2c->lmadr;
>  break;
> @@ -181,9 +194,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr 
> addr, unsigned int size)
>  case 16:
>  ret = i2c->directcntl;
>  break;
> -case 17:
> -ret = i2c->intr;
> -break;
>  default:
>  if (addr < PPC4xx_I2C_MEM_SIZE) {
>  qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> @@ -201,14 +211,15 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr 
> addr, unsigned int size)
>  static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>unsigned int size)
>  

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[Igor Mammedov]

On Thu,  7 Jun 2018 18:52:16 +0200
David Hildenbrand  wrote:

> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> unplug memory devices (which a pc-dimm is) later.
Perhaps something like following would be better:

Factor out memory unplug into separate function from spapr_lmb_release().
Then use generic hotplug_handler_unplug() to trigger memory unplug,
which would call spapr_machine_device_unplug() -> spapr_memory_unplug()
in the end .
This way unplug operation is not buried in lmb internals and located
in the same place like in other targets, following similar
logic/call chain across targets.


> Signed-off-by: David Hildenbrand 
> ---
>  hw/ppc/spapr.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bcb72d9fa7..0a8a3455d6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3298,7 +3298,8 @@ static sPAPRDIMMState 
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>  /* Callback to be called during DRC release. */
>  void spapr_lmb_release(DeviceState *dev)
>  {
> -sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
>  sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
> PC_DIMM(dev));
>  
>  /* This information will get lost if a migration occurs
> @@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
>  
>  /*
>   * Now that all the LMBs have been removed by the guest, call the
> - * pc-dimm unplug handler to cleanup up the pc-dimm device.
> + * unplug handler chain. This can never fail.
>   */
> -pc_dimm_memory_unplug(dev, MACHINE(spapr));
> +hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> +}
> +
> +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState 
> *dev)
> +{
> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> +sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
> PC_DIMM(dev));
> +
> +pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
>  object_unparent(OBJECT(dev));
>  spapr_pending_dimm_unplugs_remove(spapr, ds);
>  }
> @@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>  DeviceState *dev, Error **errp)
>  {
> +if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +spapr_memory_unplug(hotplug_dev, dev);
> +}
>  }
>  
>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging

[David Gibson]

On Wed, Jun 06, 2018 at 12:56:32PM -0300, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > Make it more readable by converting register indexes to decimal
> > (avoids lot of superfluous 0x0) and distinguish errors caused by
> > accessing non-existent vs. unimplemented registers.
> > No functional change.
> > 
> > Signed-off-by: BALATON Zoltan 
> > ---
> >  hw/i2c/ppc4xx_i2c.c | 94 
> > +
> >  1 file changed, 51 insertions(+), 43 deletions(-)
> > 
> > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > index ab64d19..d1936db 100644
> > --- a/hw/i2c/ppc4xx_i2c.c
> > +++ b/hw/i2c/ppc4xx_i2c.c
> > @@ -31,7 +31,7 @@
> >  #include "hw/hw.h"
> >  #include "hw/i2c/ppc4xx_i2c.h"
> >  
> > -#define PPC4xx_I2C_MEM_SIZE 0x12
> > +#define PPC4xx_I2C_MEM_SIZE 18
> 
> This looks weird, it seems all memory range sizes are in hex in other
> QEMU devices.

[snip]
> > @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr 
> > addr, uint64_t value,
> >  }
> >  }
> >  break;
> > -case 0x07:
> > -i2c->mdcntl = value & 0xDF;
> > +case 7:
> > +i2c->mdcntl = value & 0xdf;
> >  break;
> > -case 0x08:
> > -i2c->sts &= ~(value & 0x0A);
> > +case 8:
> > +i2c->sts &= ~(value & 0xa);
> 
> 'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
> Matter of taste...

I tend to prefer the forms you suggest, Philippe, but not by enough to
delay this otherwise good cleanup.  Especially since Balaton is taking
on this long neglected area of the code.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v1 7/8] spapr: handle cpu core unplug via hotplug handler chain

[Igor Mammedov]

On Thu,  7 Jun 2018 18:52:17 +0200
David Hildenbrand  wrote:

> Let's handle it via hotplug_handler_unplug() to make plug/unplug code
> look symmetrical.
ditto wrt commit message as 6/8

> 
> Acked-by: Igor Mammedov 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/ppc/spapr.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0a8a3455d6..994deea8cf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3415,7 +3415,15 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState 
> *cs, int *fdt_offset,
>  /* Callback to be called during DRC release. */
>  void spapr_core_release(DeviceState *dev)
>  {
> -MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +
> +/* Call the unplug handler chain. This can never fail. */
> +hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> +}
> +
> +static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> +MachineState *ms = MACHINE(hotplug_dev);
>  sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>  CPUCore *cc = CPU_CORE(dev);
>  CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> @@ -3600,6 +3608,8 @@ static void spapr_machine_device_unplug(HotplugHandler 
> *hotplug_dev,
>  {
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>  spapr_memory_unplug(hotplug_dev, dev);
> +} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +spapr_core_unplug(hotplug_dev, dev);
>  }
>  }
>  

Re: [Qemu-devel] [PATCH v1 7/8] spapr: handle cpu core unplug via hotplug handler chain

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:17 +0200
David Hildenbrand  wrote:

> Let's handle it via hotplug_handler_unplug() to make plug/unplug code
> look symmetrical.
> 
> Acked-by: Igor Mammedov 
> Signed-off-by: David Hildenbrand 
> ---

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0a8a3455d6..994deea8cf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3415,7 +3415,15 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState 
> *cs, int *fdt_offset,
>  /* Callback to be called during DRC release. */
>  void spapr_core_release(DeviceState *dev)
>  {
> -MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +
> +/* Call the unplug handler chain. This can never fail. */
> +hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> +}
> +
> +static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> +MachineState *ms = MACHINE(hotplug_dev);
>  sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>  CPUCore *cc = CPU_CORE(dev);
>  CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> @@ -3600,6 +3608,8 @@ static void spapr_machine_device_unplug(HotplugHandler 
> *hotplug_dev,
>  {
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>  spapr_memory_unplug(hotplug_dev, dev);
> +} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +spapr_core_unplug(hotplug_dev, dev);
>  }
>  }
>  

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[David Hildenbrand]

On 08.06.2018 10:56, Igor Mammedov wrote:
> On Thu,  7 Jun 2018 18:52:16 +0200
> David Hildenbrand  wrote:
> 
>> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
>> unplug memory devices (which a pc-dimm is) later.
> Perhaps something like following would be better:
> 
> Factor out memory unplug into separate function from spapr_lmb_release().
> Then use generic hotplug_handler_unplug() to trigger memory unplug,
> which would call spapr_machine_device_unplug() -> spapr_memory_unplug()
> in the end .
> This way unplug operation is not buried in lmb internals and located
> in the same place like in other targets, following similar
> logic/call chain across targets.

Can this be an addon patch? Sounds like factoring out more and moving more.

> 
> 
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/ppc/spapr.c | 18 +++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index bcb72d9fa7..0a8a3455d6 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3298,7 +3298,8 @@ static sPAPRDIMMState 
>> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>>  /* Callback to be called during DRC release. */
>>  void spapr_lmb_release(DeviceState *dev)
>>  {
>> -sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
>> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
>> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
>>  sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
>> PC_DIMM(dev));
>>  
>>  /* This information will get lost if a migration occurs
>> @@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
>>  
>>  /*
>>   * Now that all the LMBs have been removed by the guest, call the
>> - * pc-dimm unplug handler to cleanup up the pc-dimm device.
>> + * unplug handler chain. This can never fail.
>>   */
>> -pc_dimm_memory_unplug(dev, MACHINE(spapr));
>> +hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
>> +}
>> +
>> +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState 
>> *dev)
>> +{
>> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
>> +sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
>> PC_DIMM(dev));
>> +
>> +pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
>>  object_unparent(OBJECT(dev));
>>  spapr_pending_dimm_unplugs_remove(spapr, ds);
>>  }
>> @@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler 
>> *hotplug_dev,
>>  static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>>  DeviceState *dev, Error **errp)
>>  {
>> +if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> +spapr_memory_unplug(hotplug_dev, dev);
>> +}
>>  }
>>  
>>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Igor Mammedov]

On Fri, 8 Jun 2018 09:40:04 +0200
David Hildenbrand  wrote:

> On 08.06.2018 09:27, Christian Borntraeger wrote:
> > 
> > 
> > On 06/08/2018 09:25 AM, Cornelia Huck wrote:  
> >> On Thu,  7 Jun 2018 18:52:18 +0200
> >> David Hildenbrand  wrote:
> >>  
> >>> Let's introduce and use local error variables in the hotplug handler
> >>> functions.
> >>>
> >>> Signed-off-by: David Hildenbrand 
> >>> ---
> >>>  hw/s390x/s390-virtio-ccw.c | 11 ---
> >>>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >>> index 7ae5fb38dd..29ea50a177 100644
> >>> --- a/hw/s390x/s390-virtio-ccw.c
> >>> +++ b/hw/s390x/s390-virtio-ccw.c
> >>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
> >>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> >>>   DeviceState *dev, Error **errp)
> >>>  {
> >>> +Error *local_err = NULL;
> >>> +
> >>>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >>> -s390_cpu_plug(hotplug_dev, dev, errp);
> >>> +s390_cpu_plug(hotplug_dev, dev, &local_err);
> >>>  }
> >>> +error_propagate(errp, local_err);
> >>>  }
> >>>  
> >>>  static void s390_machine_device_unplug_request(HotplugHandler 
> >>> *hotplug_dev,
> >>> DeviceState *dev, Error 
> >>> **errp)
> >>>  {
> >>> +Error *local_err = NULL;
> >>> +
> >>>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >>> -error_setg(errp, "CPU hot unplug not supported on this machine");
> >>> -return;
> >>> +error_setg(&local_err, "CPU hot unplug not supported on this 
> >>> machine");
> >>>  }
> >>> +error_propagate(errp, local_err);
> >>>  }
> >>>  
> >>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,  
> >>
> >> Just seeing this patch by itself, it does not really make much sense.
> >> Even if this is a split out clean-up series, I'd prefer this to go
> >> together with a patch that actually adds something more to the
> >> plug/unplug functions.  
> > 
> > +1. It is hard to see the "why". Maybe a better patch description could 
> > help here?
> >   
> 
> When checking for an error (*errp) we should make sure that we don't
> dereference the NULL pointer. I will be doing that in the future (memory
> devices), but as you both don't seem to like this patch, I'll drop it
> for now.
hotplug handlers aren't called with NULL errp, so it not really necessary.
To be on the safe side we can ensure that errp is not NULL doing something
like:

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986..dc9e4bf 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -52,6 +52,7 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
 {
 HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
 
+g_assert(errp);
 if (hdc->unplug) {
 hdc->unplug(plug_handler, plugged_dev, errp);
 }

and do it for all similar wrappers in this file

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:16 +0200
David Hildenbrand  wrote:

> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> unplug memory devices (which a pc-dimm is) later.
> 
> Signed-off-by: David Hildenbrand 
> ---

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bcb72d9fa7..0a8a3455d6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3298,7 +3298,8 @@ static sPAPRDIMMState 
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>  /* Callback to be called during DRC release. */
>  void spapr_lmb_release(DeviceState *dev)
>  {
> -sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
>  sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
> PC_DIMM(dev));
>  
>  /* This information will get lost if a migration occurs
> @@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
>  
>  /*
>   * Now that all the LMBs have been removed by the guest, call the
> - * pc-dimm unplug handler to cleanup up the pc-dimm device.
> + * unplug handler chain. This can never fail.
>   */
> -pc_dimm_memory_unplug(dev, MACHINE(spapr));
> +hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> +}
> +
> +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState 
> *dev)
> +{
> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> +sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
> PC_DIMM(dev));
> +
> +pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
>  object_unparent(OBJECT(dev));
>  spapr_pending_dimm_unplugs_remove(spapr, ds);
>  }
> @@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler 
> *hotplug_dev,
>  static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
>  DeviceState *dev, Error **errp)
>  {
> +if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +spapr_memory_unplug(hotplug_dev, dev);
> +}
>  }
>  
>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

Re: [Qemu-devel] [PATCH] typedefs: add QJSON

[Dr. David Alan Gilbert]

* Greg Kurz (gr...@kaod.org) wrote:
> Since commit 83ee768d6247b, we now have two places that define the
> QJSON type:
> 
> $ git grep 'typedef struct QJSON QJSON'
> include/migration/vmstate.h:typedef struct QJSON QJSON;
> migration/qjson.h:typedef struct QJSON QJSON;
> 
> This breaks docker-test-build@centos6:
> 
> In file included from /tmp/qemu-test/src/migration/savevm.c:59:
> /tmp/qemu-test/src/migration/qjson.h:16: error: redefinition of typedef
>  'QJSON'
> /tmp/qemu-test/src/include/migration/vmstate.h:30: note: previous
>  declaration of 'QJSON' was here
> make: *** [migration/savevm.o] Error 1
> 
> This happens because CentOS 6 has an old GCC 4.4.7. Even if redefining
> a typedef with the same type is permitted since GCC 4.6, unless -pedantic
> is passed, we don't really need to do that on purpose. Let's have a
> single definition in  instead.
> 
> Signed-off-by: Greg Kurz 

Yeh that looks right to me;


Reviewed-by: Dr. David Alan Gilbert 

> ---
>  include/migration/vmstate.h |2 --
>  include/qemu/typedefs.h |1 +
>  migration/qjson.h   |2 --
>  3 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 3747110f951a..42b946ce902c 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -27,8 +27,6 @@
>  #ifndef QEMU_VMSTATE_H
>  #define QEMU_VMSTATE_H
>  
> -typedef struct QJSON QJSON;
> -
>  typedef struct VMStateInfo VMStateInfo;
>  typedef struct VMStateDescription VMStateDescription;
>  typedef struct VMStateField VMStateField;
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 325c72de3313..3ec0e13a967f 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -97,6 +97,7 @@ typedef struct QEMUTimer QEMUTimer;
>  typedef struct QEMUTimerListGroup QEMUTimerListGroup;
>  typedef struct QBool QBool;
>  typedef struct QDict QDict;
> +typedef struct QJSON QJSON;
>  typedef struct QList QList;
>  typedef struct QNull QNull;
>  typedef struct QNum QNum;
> diff --git a/migration/qjson.h b/migration/qjson.h
> index 2978b5f37121..41664f2d71b5 100644
> --- a/migration/qjson.h
> +++ b/migration/qjson.h
> @@ -13,8 +13,6 @@
>  #ifndef QEMU_QJSON_H
>  #define QEMU_QJSON_H
>  
> -typedef struct QJSON QJSON;
> -
>  QJSON *qjson_new(void);
>  void qjson_destroy(QJSON *json);
>  void json_prop_str(QJSON *json, const char *name, const char *str);
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Igor Mammedov]

On Fri, 8 Jun 2018 10:41:36 +0200
David Hildenbrand  wrote:

> On 08.06.2018 10:39, Igor Mammedov wrote:
> > On Fri, 8 Jun 2018 10:07:31 +0200
> > Thomas Huth  wrote:
> >   
> >> On 08.06.2018 09:48, David Hildenbrand wrote:  
> >>> On 08.06.2018 09:46, Greg Kurz wrote:
>  On Fri, 8 Jun 2018 09:42:48 +0200
>  David Hildenbrand  wrote:
> 
> > On 08.06.2018 09:34, Greg Kurz wrote:
> >> On Thu,  7 Jun 2018 18:52:12 +0200
> >> David Hildenbrand  wrote:
> >>   
> >>> The node property can always be queried and the value has already been
> >>> verified in pc_dimm_realize().
> >>>
> >>> Signed-off-by: David Hildenbrand 
> >>> ---
> >>>  hw/ppc/spapr.c | 9 +
> >>>  1 file changed, 1 insertion(+), 8 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index 2375cbee12..d038f3243e 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -3578,14 +3578,7 @@ static void 
> >>> spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >>>  error_setg(errp, "Memory hotplug not supported for this 
> >>> machine");
> >>>  return;
> >>>  }
> >>> -node = object_property_get_uint(OBJECT(dev), 
> >>> PC_DIMM_NODE_PROP, errp);
> >>> -if (*errp) {  
> >>
> >> Good riddance :)
> >>   
> >>> -return;
> >>> -}
> >>> -if (node < 0 || node >= MAX_NODES) {
> >>> -error_setg(errp, "Invaild node %d", node);
> >>> -return;
> >>> -}
> >>
> >> Maybe turn that into an assert() instead? ... just for the paranoids ;-)
> >>  
> >>> +node = object_property_get_uint(OBJECT(dev), 
> >>> PC_DIMM_NODE_PROP, NULL);  
> >>
> >> Maybe pass &error_abort ?  
> >
> > I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> >
> > ("error ignored" vs. "error leads to an abort") - but this will actually
> > never fail. But I can use error_abort here, does not matter.
> >
> 
>  Heh, /me paranoid but this is David's call and he acked that already
>  so I guess it's okay.
> >>>
> >>> NULL makes it fit into a single line :)
> >>
> >> +1 for error_abort, even if it takes another line.  
> > +1 for error_abort
> > call shouldn't fail, but if does it won't be silently ignored
> > and introduce undefined behavior.  
> 
> Maybe we should fix the others that pass in NULL.
> 
> (no, not me :D - I'm already busy with your requested pre_plug handling)
Add it to wiki page for bite sized tasks?

> 
> >   
> >>
> >>  Thomas  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging

[BALATON Zoltan]

On Fri, 8 Jun 2018, David Gibson wrote:

On Wed, Jun 06, 2018 at 12:56:32PM -0300, Philippe Mathieu-Daudé wrote:

On 06/06/2018 10:31 AM, BALATON Zoltan wrote:

Make it more readable by converting register indexes to decimal
(avoids lot of superfluous 0x0) and distinguish errors caused by
accessing non-existent vs. unimplemented registers.
No functional change.

Signed-off-by: BALATON Zoltan 
---
 hw/i2c/ppc4xx_i2c.c | 94 +
 1 file changed, 51 insertions(+), 43 deletions(-)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index ab64d19..d1936db 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -31,7 +31,7 @@
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"

-#define PPC4xx_I2C_MEM_SIZE 0x12
+#define PPC4xx_I2C_MEM_SIZE 18


This looks weird, it seems all memory range sizes are in hex in other
QEMU devices.


[snip]

@@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, 
uint64_t value,
 }
 }
 break;
-case 0x07:
-i2c->mdcntl = value & 0xDF;
+case 7:
+i2c->mdcntl = value & 0xdf;
 break;
-case 0x08:
-i2c->sts &= ~(value & 0x0A);
+case 8:
+i2c->sts &= ~(value & 0xa);


'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
Matter of taste...


I tend to prefer the forms you suggest, Philippe, but not by enough to
delay this otherwise good cleanup.  Especially since Balaton is taking
on this long neglected area of the code.


I prefer code that does not have unneeded chars so it's easier to read, 
that's why I've removed all 0x0 from this file which made it more 
comprehensible. But I'll add the 0 back to this place in next respin as 
you both seem to prefer that and since other bit masks are two digit too 
it's also consistent that way.

Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues

[Peter Xu]

On Fri, Jun 08, 2018 at 10:18:25AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi  writes:
> 
> > On Fri, Jun 08, 2018 at 12:42:35PM +0800, Peter Xu wrote:
> >> On Thu, Jun 07, 2018 at 01:53:01PM +0200, Markus Armbruster wrote:
> >> > Peter Xu  writes:
> >> > 
> >> > > Previously we cleanup the queues when we got CLOSED event.  It was used
> >> > 
> >> > we clean up
> >> > 
> >> > > to make sure we won't leftover replies/events of a old client to a new
> >> > 
> >> > we won't send leftover replies/events of an old client
> >> > 
> >> > > client.  Now this patch postpones that until OPENED.
> >> > 
> >> > What if OPENED never comes?
> >> 
> >> Then we clean that up until destruction of the monitor.  IMHO it's
> >> fine, but I'm not sure whether that's an overall acceptable approach.
> >
> > I think this patch fixes the problem at the wrong level.  Marc-André's
> > fix seemed like a cleaner solution.

Actually I like Marc-Andre's solution too.

However I left a comment there, I'm not sure whether that's workable.
My feeling is that currently our chardev can't really work very nicely
when the chardev backend is composed by two different channels, say,
when IN and OUT are different fds underneath.

Btw, I just noticed that fd_chr_add_watch() will only add a watch to
the ioc_out object, I'm not sure why (see that we have both ioc_in and
ioc_out for fd chardev):

static GSource *fd_chr_add_watch(Chardev *chr, GIOCondition cond)
{
FDChardev *s = FD_CHARDEV(chr);
return qio_channel_create_watch(s->ioc_out, cond);
}

(this is not related to current problem at all, it's a pure question;
 feel free to ignore)

> 
> Is it the right solution?
> 
> I proposed another one:
> 
> [...]
> >> > Here's what I think we should do on such EOF:
> >> > 
> >> > 1. Shut down input
> >> > 
> >> >Stop reading input.  If input has its own file descriptor (as opposed
> >> >to a read/write fd shared with output), close it.
> >> > 
> >> > 2. Flush output
> >> > 
> >> >Continue to write output until the queue becomes empty or we run into
> >> >an error (such as EPIPE, telling us the output's consumer went away).
> >> >Works exactly as before, except we proceed to step 3 when the queue
> 
> exactly as before EOF

Yeah this seems reasonable.

I did a quick test with below change and it fixes the problem too:

diff --git a/monitor.c b/monitor.c
index 5eb4630215..b76cab5022 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4366,6 +4366,8 @@ static void monitor_qmp_event(void *opaque, int event)
 mon_refcount++;
 break;
 case CHR_EVENT_CLOSED:
+/* Force flush all events */
+monitor_qmp_bh_responder(NULL);
 monitor_qmp_cleanup_queues(mon);
 json_message_parser_destroy(&mon->qmp.parser);
 json_message_parser_init(&mon->qmp.parser, handle_qmp_command);

(Though maybe I can do it a bit "prettier" when I post a formal
 patch... for example, only flush the response queue for that specific
 monitor)

Frankly speaking I think this might be an ideal fix as well.  For
example what if we are executing the dispatcher of a command when we
received the CLOSED event?  If so, the dispatcher will put the
response onto the response queue after the CLOSED event, and ideally
we'd better also deliver that to the filter_output process.

> 
> >> >becomes empty.
> >> > 
> >> > 3. Shut down output
> >> > 
> >> >Close the output file descriptor.

For this one I don't know whether it'll be necessary.  That's managed
by chardev backends now, I think it now won't be closed until QEMU
quits (for our bash pipelne scenario).

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] typedefs: add QJSON

[Greg Kurz]

On Fri, 8 Jun 2018 10:04:39 +0100
"Dr. David Alan Gilbert"  wrote:

> * Greg Kurz (gr...@kaod.org) wrote:
> > Since commit 83ee768d6247b, we now have two places that define the
> > QJSON type:
> > 
> > $ git grep 'typedef struct QJSON QJSON'
> > include/migration/vmstate.h:typedef struct QJSON QJSON;
> > migration/qjson.h:typedef struct QJSON QJSON;
> > 
> > This breaks docker-test-build@centos6:
> > 
> > In file included from /tmp/qemu-test/src/migration/savevm.c:59:
> > /tmp/qemu-test/src/migration/qjson.h:16: error: redefinition of typedef
> >  'QJSON'
> > /tmp/qemu-test/src/include/migration/vmstate.h:30: note: previous
> >  declaration of 'QJSON' was here
> > make: *** [migration/savevm.o] Error 1
> > 
> > This happens because CentOS 6 has an old GCC 4.4.7. Even if redefining
> > a typedef with the same type is permitted since GCC 4.6, unless -pedantic
> > is passed, we don't really need to do that on purpose. Let's have a
> > single definition in  instead.
> > 
> > Signed-off-by: Greg Kurz   
> 
> Yeh that looks right to me;
> 
> 
> Reviewed-by: Dr. David Alan Gilbert 
> 

Hi Dave,

Whose tree should this go through ?

$ git show | ./scripts/get_maintainer.pl 
Juan Quintela  (maintainer:Migration)
"Dr. David Alan Gilbert"  (maintainer:Migration)
qemu-devel@nongnu.org (open list:All patches CC here)

Cheers,

--
Greg

> > ---
> >  include/migration/vmstate.h |2 --
> >  include/qemu/typedefs.h |1 +
> >  migration/qjson.h   |2 --
> >  3 files changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 3747110f951a..42b946ce902c 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -27,8 +27,6 @@
> >  #ifndef QEMU_VMSTATE_H
> >  #define QEMU_VMSTATE_H
> >  
> > -typedef struct QJSON QJSON;
> > -
> >  typedef struct VMStateInfo VMStateInfo;
> >  typedef struct VMStateDescription VMStateDescription;
> >  typedef struct VMStateField VMStateField;
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index 325c72de3313..3ec0e13a967f 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -97,6 +97,7 @@ typedef struct QEMUTimer QEMUTimer;
> >  typedef struct QEMUTimerListGroup QEMUTimerListGroup;
> >  typedef struct QBool QBool;
> >  typedef struct QDict QDict;
> > +typedef struct QJSON QJSON;
> >  typedef struct QList QList;
> >  typedef struct QNull QNull;
> >  typedef struct QNum QNum;
> > diff --git a/migration/qjson.h b/migration/qjson.h
> > index 2978b5f37121..41664f2d71b5 100644
> > --- a/migration/qjson.h
> > +++ b/migration/qjson.h
> > @@ -13,8 +13,6 @@
> >  #ifndef QEMU_QJSON_H
> >  #define QEMU_QJSON_H
> >  
> > -typedef struct QJSON QJSON;
> > -
> >  QJSON *qjson_new(void);
> >  void qjson_destroy(QJSON *json);
> >  void json_prop_str(QJSON *json, const char *name, const char *str);
> >   
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers

[BALATON Zoltan]

On Fri, 8 Jun 2018, David Gibson wrote:

On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:

Signed-off-by: BALATON Zoltan 


It's not clear to me why this is preferable to having the registers
embedded in the state structure.  The latter is pretty standard
practice for qemu.


Maybe it will be clearer after the next patch in the series. I needed a 
place to store the bitbang_i2c_interface for the directcntl way of 
accessing the i2c bus but I can't include bitbang_i2c.h from the public 
header because it's a local header. So I needed a local extension to the 
state struct. Once I have that then it's a good place to also store 
private registers which are now defined in the same file so I don't have 
to look up them in a different place. This seemed clearer to me and easier 
to work with. Maybe the spliting of the rewrite did not make this clear.


One thing I'm not sure about though:


---
 hw/i2c/ppc4xx_i2c.c | 75 +
 include/hw/i2c/ppc4xx_i2c.h | 19 ++--
 2 files changed, 43 insertions(+), 51 deletions(-)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index d1936db..a68b5f7 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c

[...]

@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
 static void ppc4xx_i2c_init(Object *o)
 {
 PPC4xxI2CState *s = PPC4xx_I2C(o);
+PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));

+s->regs = r;
 memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
   TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
 sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);


I allocate memory here but I'm not sure if it should be g_free'd somewhere 
and if so where? I was not able to detangle QOM object hierarchies and 
there seems to be no good docs available or I haven't found them. (PCI 
devices seem to have unrealize methods but this did not work for I2C 
objects.)

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[David Hildenbrand]

On 08.06.2018 11:03, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 09:40:04 +0200
> David Hildenbrand  wrote:
> 
>> On 08.06.2018 09:27, Christian Borntraeger wrote:
>>>
>>>
>>> On 06/08/2018 09:25 AM, Cornelia Huck wrote:  
 On Thu,  7 Jun 2018 18:52:18 +0200
 David Hildenbrand  wrote:
  
> Let's introduce and use local error variables in the hotplug handler
> functions.
>
> Signed-off-by: David Hildenbrand 
> ---
>  hw/s390x/s390-virtio-ccw.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7ae5fb38dd..29ea50a177 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>   DeviceState *dev, Error **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -s390_cpu_plug(hotplug_dev, dev, errp);
> +s390_cpu_plug(hotplug_dev, dev, &local_err);
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static void s390_machine_device_unplug_request(HotplugHandler 
> *hotplug_dev,
> DeviceState *dev, Error 
> **errp)
>  {
> +Error *local_err = NULL;
> +
>  if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -error_setg(errp, "CPU hot unplug not supported on this machine");
> -return;
> +error_setg(&local_err, "CPU hot unplug not supported on this 
> machine");
>  }
> +error_propagate(errp, local_err);
>  }
>  
>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,  

 Just seeing this patch by itself, it does not really make much sense.
 Even if this is a split out clean-up series, I'd prefer this to go
 together with a patch that actually adds something more to the
 plug/unplug functions.  
>>>
>>> +1. It is hard to see the "why". Maybe a better patch description could 
>>> help here?
>>>   
>>
>> When checking for an error (*errp) we should make sure that we don't
>> dereference the NULL pointer. I will be doing that in the future (memory
>> devices), but as you both don't seem to like this patch, I'll drop it
>> for now.
> hotplug handlers aren't called with NULL errp, so it not really necessary.
> To be on the safe side we can ensure that errp is not NULL doing something
> like:

They are, but not on s390x :) But we should never life with such
assumptions - calling code may change. Passing NULL results right now
not in a crash, but the "return" value in case of a failure cannot be
indicated.

Maybe we should even change all users of NULL for errp to use &error_abort.

For now I dropped these "local_err" patches and kept only the spapr
cleanups.

> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986..dc9e4bf 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -52,6 +52,7 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
>  {
>  HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
>  
> +g_assert(errp);
>  if (hdc->unplug) {
>  hdc->unplug(plug_handler, plugged_dev, errp);
>  }
> 
> and do it for all similar wrappers in this file
> 
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] typedefs: add QJSON

[Dr. David Alan Gilbert]

* Greg Kurz (gr...@kaod.org) wrote:
> On Fri, 8 Jun 2018 10:04:39 +0100
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Greg Kurz (gr...@kaod.org) wrote:
> > > Since commit 83ee768d6247b, we now have two places that define the
> > > QJSON type:
> > > 
> > > $ git grep 'typedef struct QJSON QJSON'
> > > include/migration/vmstate.h:typedef struct QJSON QJSON;
> > > migration/qjson.h:typedef struct QJSON QJSON;
> > > 
> > > This breaks docker-test-build@centos6:
> > > 
> > > In file included from /tmp/qemu-test/src/migration/savevm.c:59:
> > > /tmp/qemu-test/src/migration/qjson.h:16: error: redefinition of typedef
> > >  'QJSON'
> > > /tmp/qemu-test/src/include/migration/vmstate.h:30: note: previous
> > >  declaration of 'QJSON' was here
> > > make: *** [migration/savevm.o] Error 1
> > > 
> > > This happens because CentOS 6 has an old GCC 4.4.7. Even if redefining
> > > a typedef with the same type is permitted since GCC 4.6, unless -pedantic
> > > is passed, we don't really need to do that on purpose. Let's have a
> > > single definition in  instead.
> > > 
> > > Signed-off-by: Greg Kurz   
> > 
> > Yeh that looks right to me;
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert 
> > 
> 
> Hi Dave,
> 
> Whose tree should this go through ?

I'm happy to take it through migration.

Dave

> $ git show | ./scripts/get_maintainer.pl 
> Juan Quintela  (maintainer:Migration)
> "Dr. David Alan Gilbert"  (maintainer:Migration)
> qemu-devel@nongnu.org (open list:All patches CC here)
> 
> Cheers,
> 
> --
> Greg
> 
> > > ---
> > >  include/migration/vmstate.h |2 --
> > >  include/qemu/typedefs.h |1 +
> > >  migration/qjson.h   |2 --
> > >  3 files changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > index 3747110f951a..42b946ce902c 100644
> > > --- a/include/migration/vmstate.h
> > > +++ b/include/migration/vmstate.h
> > > @@ -27,8 +27,6 @@
> > >  #ifndef QEMU_VMSTATE_H
> > >  #define QEMU_VMSTATE_H
> > >  
> > > -typedef struct QJSON QJSON;
> > > -
> > >  typedef struct VMStateInfo VMStateInfo;
> > >  typedef struct VMStateDescription VMStateDescription;
> > >  typedef struct VMStateField VMStateField;
> > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > > index 325c72de3313..3ec0e13a967f 100644
> > > --- a/include/qemu/typedefs.h
> > > +++ b/include/qemu/typedefs.h
> > > @@ -97,6 +97,7 @@ typedef struct QEMUTimer QEMUTimer;
> > >  typedef struct QEMUTimerListGroup QEMUTimerListGroup;
> > >  typedef struct QBool QBool;
> > >  typedef struct QDict QDict;
> > > +typedef struct QJSON QJSON;
> > >  typedef struct QList QList;
> > >  typedef struct QNull QNull;
> > >  typedef struct QNum QNum;
> > > diff --git a/migration/qjson.h b/migration/qjson.h
> > > index 2978b5f37121..41664f2d71b5 100644
> > > --- a/migration/qjson.h
> > > +++ b/migration/qjson.h
> > > @@ -13,8 +13,6 @@
> > >  #ifndef QEMU_QJSON_H
> > >  #define QEMU_QJSON_H
> > >  
> > > -typedef struct QJSON QJSON;
> > > -
> > >  QJSON *qjson_new(void);
> > >  void qjson_destroy(QJSON *json);
> > >  void json_prop_str(QJSON *json, const char *name, const char *str);
> > >   
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] target/ppc: Allow PIR read in privileged mode

[David Gibson]

On Wed, Jun 06, 2018 at 11:19:22AM +0200, Greg Kurz wrote:
> On Wed, 6 Jun 2018 10:53:17 +1000
> David Gibson  wrote:
> 
> > On Tue, Jun 05, 2018 at 06:46:12PM +0200, Greg Kurz wrote:
> > > On Mon, 4 Jun 2018 10:53:22 +1000
> > > David Gibson  wrote:
> > >   
> > > > On Mon, May 07, 2018 at 01:52:42PM -0300, luporl wrote:  
> > > > > According to PowerISA, the PIR register should be readable in 
> > > > > privileged
> > > > > mode also, not only in hypervisor privileged mode.
> > > > > 
> > > > > PowerISA 3.0 - 4.3.3 Processor Identification Register
> > > > > 
> > > > > "Read access to the PIR is privileged; write access is not
> > > > > provided."
> > > > 
> > > > Yes... but a little further down it says "The PIR is a hypervisor
> > > > resource".  Looking at the older 2.07 ISA, it says that
> > > > guest-supervisor mode reads to the PIR should be redirected to the
> > > > GPIR register, which this change won't accomplish.
> > > >   
> > > 
> > > Hmmm, there are two definitions for the PIR, one in Book III-S (4.3.3)
> > > and one in Book III-E (5.3.3). It looks like you're referring to the
> > > latter...
> > > 
> > > [Category:Embedded.Hypervisor]
> > > Read accesses to the PIR in guest supervisor state are
> > > mapped to the GPIR.
> > > 
> > > The Book III-S definition doesn't mention the GPIR.  
> > 
> > Oops, sorry.  Yes the GPIR stuff is only for BookE.  The statement
> > about the PIR being a hypervisor resource is definitely in the BookS
> > section, however (both 2.07 and 3.0).
> > 
> 
> Yes it is, but IIUC, this means that the guest cannot modify it, eg,
> do mtspr. Section 4.4.4 in Book III-S has a list of SPRs that seem to
> indicate that mfspr doesn't require hypervisor state with the PIR.

Ah, yes, I was looking for a summary that covered that, but hadn't
found it yet.

The patch doesn't actually apply clean to the current tree any more,
due to a rename.  So can you repost, and I'll apply.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

 +1 for error_abort, even if it takes another line.  
>>> +1 for error_abort
>>> call shouldn't fail, but if does it won't be silently ignored
>>> and introduce undefined behavior.  
>>
>> Maybe we should fix the others that pass in NULL.
>>
>> (no, not me :D - I'm already busy with your requested pre_plug handling)
> Add it to wiki page for bite sized tasks?

Done.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues

[Peter Xu]

On Fri, Jun 08, 2018 at 05:11:54PM +0800, Peter Xu wrote:

[...]

> Frankly speaking I think this might be an ideal fix as well.  For
> example what if we are executing the dispatcher of a command when we
> received the CLOSED event?  If so, the dispatcher will put the
> response onto the response queue after the CLOSED event, and ideally
> we'd better also deliver that to the filter_output process.

Please ignore this paragraph.  Actually if that happens, we'll queue
the response onto the response queue as usual, then as long as the
output channel is not closed it'll still be delivered to the
filter_output process.

So I think I agree with Markus's solution, we just flush the response
queue when we get CLOSED (but we don't close the output fd; IMHO
that's chardev backend's job).  Would that work?

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PULL 0/7] 9p patches 2018-06-07

[Peter Maydell]

On 7 June 2018 at 16:21, Greg Kurz  wrote:
> The following changes since commit 5d328d7d2f1fd4fb160bcfb6e4eb838720274438:
>
>   Merge remote-tracking branch 'remotes/awilliam/tags/vfio-update-20180605.0' 
> into staging (2018-06-07 08:59:28 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/gkurz/qemu.git tags/for-upstream
>
> for you to fetch changes up to aca6897fba149a2a650dcdf5a5e1ae828371f4aa:
>
>   9p: xattr: Properly translate xattrcreate flags (2018-06-07 12:17:22 +0200)
>
> 
> Mostly bug fixes and code sanitization motivated by the upcoming
> support for Darwin hosts. Thanks to Keno Fischer.
>
> 
> Keno Fischer (7):
>   9p: proxy: Fix size passed to `connect`
>   9p: local: Properly set errp in fstatfs error path
>   9p: Move a couple xattr functions to 9p-util
>   9p: xattr: Fix crashes due to free of uninitialized value
>   9p: local: Avoid warning if FS_IOC_GETVERSION is not defined
>   9p: Properly check/translate flags in unlinkat
>   9p: xattr: Properly translate xattrcreate flags

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH 4/6] hmp: Add info commands for preconfig

[Igor Mammedov]

On Fri, 8 Jun 2018 09:18:46 +0100
"Dr. David Alan Gilbert"  wrote:

> * Markus Armbruster (arm...@redhat.com) wrote:
> > "Dr. David Alan Gilbert"  writes:
> >   
> > > * Markus Armbruster (arm...@redhat.com) wrote:  
> > >> "Dr. David Alan Gilbert"  writes:
> > >>   
> > >> > * Markus Armbruster (arm...@redhat.com) wrote:  
> > >> >> Peter Xu  writes:
> > >> >>   
> > >> >> > On Tue, Jun 05, 2018 at 01:26:34PM +0100, Dr. David Alan Gilbert 
> > >> >> > (git) wrote:  
> > >> >> >> From: "Dr. David Alan Gilbert" 
> > >> >> >> 
> > >> >> >> Allow a bunch of the info commands to be used in preconfig.
> > >> >> >> Could probably add most of them.  
> > >> >> >
> > >> >> > I guess some of them may not work yet during preconfig.  E.g.:
> > >> >> >
> > >> >> > $ ./x86_64-softmmu/qemu-system-x86_64 -preconfig -monitor stdio
> > >> >> > QEMU 2.12.50 monitor - type 'help' for more information
> > >> >> > (qemu) info mtree
> > >> >> > address-space: memory
> > >> >> >   - (prio 0, i/o): system
> > >> >> >
> > >> >> > address-space: I/O
> > >> >> >   - (prio 0, i/o): io
> > >> >> >
> > >> >> > But it's fine to enable that I guess.
> > >> >> >
> > >> >> > (Which "info" command would you want to use during preconfig?)
> > >> >> >  
> > >> >> >> 
> > >> >> >> Signed-off-by: Dr. David Alan Gilbert   
> > >> >> >
> > >> >> > Reviewed-by: Peter Xu   
> > >> >> 
> > >> >> The reason for having -preconfig is us despairing of making -S do the
> > >> >> right thing.  We'd have to *understand* the tangled mess that is our
> > >> >> startup, and rearrange it so QMP becomes available early enough for
> > >> >> configuring NUMA (and other things), yet late enough for everything to
> > >> >> work.
> > >> >> 
> > >> >> -preconfig is a cheap hack to avoid this headache, by bypassing almost
> > >> >> all of "everything".
> > >> >> 
> > >> >> Now you bring back some of "everything".  Dangerous.  You better show 
> > >> >> it
> > >> >> actually works.  Until you do:
> > >> >> 
> > >> >> NAK  
> > >> >
> > >> > Well I did test each command in here to make sure it didn't
> > >> > crash/produce complete junk; but here's the output with the v2 of this
> > >> > patch that Igor R-b:  
> > >> [...]
> > >> 
> > >> For the sake of the argument, let's assume these commands all work in
> > >> preconfig state.  Are their QMP equivalents all available in preconfig
> > >> state?  
> > >
> > > That I don't know; I was happy to fix my list to the ones
> > > Igor recommended.  If you object to some particular entries I'll
> > > be happy to change them.  
> > 
> > HMP must not provide more functionality than QMP.  Specifically, we may
> > provide "info FOO" only when we also provide query-FOO.
> >
> > There are exceptions to this rule.  I don't think they apply here.  I'm
> > prepared to discuss them, of course.  
> 
> No, that's strictly not true;  HMP can provide anything that helps
> a human debug stuff.  The requirement is that if a tool needs it then it
> must be provided in QMP.
> 
> > I wish there was a way to automate "provide command in HMP when its
> > buddy is available in QMP", but since the buddies are only connected by
> > code, that seems infeasible.
> > 
> > Without such automation, the two sets of available commands need to be
> > kept consistent manually.  The larger they are, the more of a bother.
> > 
> > Bother is fine when it provides commensurate value to users.  Options in
> > increasing order of value provided:
> > 
> > (1) HMP becomes ready only after we exit preconfig state (what I
> > proposed in Message-ID: <87603cxob9@dusky.pond.sub.org>.
> >
> > (2) HMP provides help, quit, exit-preconfig.
> > 
> > (3) HMP provides (a subset of) the commands QMP provides.
> > 
> > I figure the maintenance cost of (1) and (2) will be negligible, but (3)
> > could be a drag.  Are you sure it's worthwhile?  
>
> 
> I'm not prepared to restrict to (2), and I'm not prepared to restrict
> HMP to a subset of QMP;  As I said previously, if there's a command that
> you think is incorrect/broken that I've enabled then I'm happy to
> remove it.
I'd prefer #3 if we going to expose HMP at all or #1.

#3 would allow testing via HMP for those who can't use qmp-shell.
we can trim HMP list to allowed-preconfig commands or audit respective
QMP variants (they should work without changes) and flag them with
allowed-preconfig. 

> 
> Dave
> 
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[Igor Mammedov]

On Fri, 8 Jun 2018 11:02:23 +0200
David Hildenbrand  wrote:

> On 08.06.2018 10:56, Igor Mammedov wrote:
> > On Thu,  7 Jun 2018 18:52:16 +0200
> > David Hildenbrand  wrote:
> >   
> >> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> >> unplug memory devices (which a pc-dimm is) later.  
> > Perhaps something like following would be better:
> > 
> > Factor out memory unplug into separate function from spapr_lmb_release().
> > Then use generic hotplug_handler_unplug() to trigger memory unplug,
> > which would call spapr_machine_device_unplug() -> spapr_memory_unplug()
> > in the end .
> > This way unplug operation is not buried in lmb internals and located
> > in the same place like in other targets, following similar
> > logic/call chain across targets.  
> 
> Can this be an addon patch? Sounds like factoring out more and moving more.
I've suggested ^^^ it as this patch description instead of the current one
that doesn't really makes the sense on it's own.

> >> Signed-off-by: David Hildenbrand 
> >> ---
> >>  hw/ppc/spapr.c | 18 +++---
> >>  1 file changed, 15 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index bcb72d9fa7..0a8a3455d6 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3298,7 +3298,8 @@ static sPAPRDIMMState 
> >> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> >>  /* Callback to be called during DRC release. */
> >>  void spapr_lmb_release(DeviceState *dev)
> >>  {
> >> -sPAPRMachineState *spapr = 
> >> SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> >> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> >> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
> >>  sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
> >> PC_DIMM(dev));
> >>  
> >>  /* This information will get lost if a migration occurs
> >> @@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
> >>  
> >>  /*
> >>   * Now that all the LMBs have been removed by the guest, call the
> >> - * pc-dimm unplug handler to cleanup up the pc-dimm device.
> >> + * unplug handler chain. This can never fail.
> >>   */
> >> -pc_dimm_memory_unplug(dev, MACHINE(spapr));
> >> +hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> >> +}
> >> +
> >> +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState 
> >> *dev)
> >> +{
> >> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> >> +sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, 
> >> PC_DIMM(dev));
> >> +
> >> +pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
> >>  object_unparent(OBJECT(dev));
> >>  spapr_pending_dimm_unplugs_remove(spapr, ds);
> >>  }
> >> @@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler 
> >> *hotplug_dev,
> >>  static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> >>  DeviceState *dev, Error **errp)
> >>  {
> >> +if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >> +spapr_memory_unplug(hotplug_dev, dev);
> >> +}
> >>  }
> >>  
> >>  static void spapr_machine_device_unplug_request(HotplugHandler 
> >> *hotplug_dev,  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[David Hildenbrand]

On 08.06.2018 11:35, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 11:02:23 +0200
> David Hildenbrand  wrote:
> 
>> On 08.06.2018 10:56, Igor Mammedov wrote:
>>> On Thu,  7 Jun 2018 18:52:16 +0200
>>> David Hildenbrand  wrote:
>>>   
 Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
 unplug memory devices (which a pc-dimm is) later.  
>>> Perhaps something like following would be better:
>>>
>>> Factor out memory unplug into separate function from spapr_lmb_release().
>>> Then use generic hotplug_handler_unplug() to trigger memory unplug,
>>> which would call spapr_machine_device_unplug() -> spapr_memory_unplug()
>>> in the end .
>>> This way unplug operation is not buried in lmb internals and located
>>> in the same place like in other targets, following similar
>>> logic/call chain across targets.  
>>
>> Can this be an addon patch? Sounds like factoring out more and moving more.
> I've suggested ^^^ it as this patch description instead of the current one
> that doesn't really makes the sense on it's own.

Okay, I was definitely misreading your comment :) Will change.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2] Show values and description when using "qom-list"

[Dr. David Alan Gilbert]

* Andreas Färber (afaer...@suse.de) wrote:
> Am 01.06.2018 um 17:39 schrieb Ricardo Perez Blanco:
> > For debugging purposes it is very useful to:
> >  - See the description of the field. This information is already filled
> >in but not shown in "qom-list" command.
> 
> No objection on this part.
> 
> >  - Display value of the field.
> 
> That is by definition the qom-get operation, not qom-list. Just like the
> ls command does not show file contents, there's cat etc. for that. For
> debugging purposes we had a qom-tree (?) command that would combine
> both.

I'm not too bothered about distinguishing between the two commands;
but it would be nice - one reason I'm not too bothered is because we've
failed to get a qom-get in multiple years of trying.


>   There might be unmerged patches on qemu-devel related to display
> of certain data types.

Which ones?

Dave

> Regards,
> Andreas
> 
> > 
> > Signed-off-by: Ricardo Perez Blanco 
> > ---
> >  hmp.c  | 13 +++--
> >  qapi/misc.json |  6 --
> >  qmp.c  |  7 +++
> >  qom/object.c   |  8 +++-
> >  4 files changed, 25 insertions(+), 9 deletions(-)
> [snip]
> 
> -- 
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2] cpu hot-del: leak fix by free the relevant members

[liujunjie]

THese leaks are found by ASAN with CPU hot-add and hot-del actions,
such as:
==14127==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 4096 byte(s) in 1 object(s) allocated from:
#0 0x7fc321cb6ec0 in posix_memalign 
(/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7ec0)
#1 0xf756b9 in qemu_try_memalign util/oslib-posix.c:110
#2 0xf7575b in qemu_memalign util/oslib-posix.c:126
#3 0x7769cb in kvm_arch_init_vcpu /root/qemu/target/i386/kvm.c:1103
#4 0x4cc4e6 in qemu_kvm_cpu_thread_fn /root/qemu/cpus.c:1201
#5 0x7fc31cb28dc4 in start_thread (/usr/lib64/libpthread.so.0+0x7dc4)

Direct leak of 4096 byte(s) in 1 object(s) allocated from:
#0 0x7fc321cb6560 in calloc (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560)
#1 0x7fc31cf944d6 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x4f4d6)
#2 0x4cc4e6 in qemu_kvm_cpu_thread_fn /root/qemu/cpus.c:1201
#3 0x7fc31cb28dc4 in start_thread (/usr/lib64/libpthread.so.0+0x7dc4)

Direct leak of 184 byte(s) in 1 object(s) allocated from:
#0 0x7fc321cb6560 in calloc (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560)
#1 0x7fc31cf944d6 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x4f4d6)
#2 0x4cda83 in qemu_init_vcpu /root/qemu/cpus.c:1993
#3 0x6aedbd in x86_cpu_realizefn /root/qemu/target/i386/cpu.c:4739
#4 0x8edfc8 in device_set_realized hw/core/qdev.c:826
#5 0xcff60c in property_set_bool qom/object.c:1928
#6 0xd09bd2 in object_property_set_qobject qom/qom-qobject.c:27
#7 0xd048e3 in object_property_set_bool qom/object.c:1188
#8 0x79ad83 in qdev_device_add /root/qemu/qdev-monitor.c:627
#9 0x79b50c in qmp_device_add /root/qemu/qdev-monitor.c:807
#10 0xf4ca5d in do_qmp_dispatch qapi/qmp-dispatch.c:119
#11 0xf4ce3c in qmp_dispatch qapi/qmp-dispatch.c:168
#12 0x4e2a5a in monitor_qmp_dispatch_one /root/qemu/monitor.c:4088
#13 0x4e2baf in monitor_qmp_bh_dispatcher /root/qemu/monitor.c:4146
#14 0xf67ad1 in aio_bh_poll util/async.c:118
#15 0xf724a3 in aio_dispatch util/aio-posix.c:436
#16 0xf67270 in aio_ctx_dispatch util/async.c:261
#17 0x7fc31cf8e999 in g_main_context_dispatch 
(/usr/lib64/libglib-2.0.so.0+0x4)

Direct leak of 64 byte(s) in 2 object(s) allocated from:
#0 0x7fc321cb6560 in calloc (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560)
#1 0x7fc31cf944d6 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x4f4d6)
#2 0x4cda1f in qemu_init_vcpu /root/qemu/cpus.c:1997
#3 0x6aedbd in x86_cpu_realizefn /root/qemu/target/i386/cpu.c:4739
#4 0x8edfc8 in device_set_realized hw/core/qdev.c:826
#5 0xcff60c in property_set_bool qom/object.c:1928
#6 0xd09bd2 in object_property_set_qobject qom/qom-qobject.c:27
#7 0xd048e3 in object_property_set_bool qom/object.c:1188
#8 0x79ad83 in qdev_device_add /root/qemu/qdev-monitor.c:627
#9 0x79b50c in qmp_device_add /root/qemu/qdev-monitor.c:807
#10 0xf4ca5d in do_qmp_dispatch qapi/qmp-dispatch.c:119
#11 0xf4ce3c in qmp_dispatch qapi/qmp-dispatch.c:168
#12 0x4e2a5a in monitor_qmp_dispatch_one /root/qemu/monitor.c:4088
#13 0x4e2baf in monitor_qmp_bh_dispatcher /root/qemu/monitor.c:4146
#14 0xf67ad1 in aio_bh_poll util/async.c:118
#15 0xf724a3 in aio_dispatch util/aio-posix.c:436
#16 0xf67270 in aio_ctx_dispatch util/async.c:261
#17 0x7fc31cf8e999 in g_main_context_dispatch 
(/usr/lib64/libglib-2.0.so.0+0x4)

SUMMARY: AddressSanitizer: 8440 byte(s) leaked in 5 allocation(s).

The relevant members in CPU Structure are freed to fix leak. Meanwhile, 
although the
VMChangeStateEntry added in kvm_arch_init_vcpu does not be reportd by ASAN 
since it still
in vm_change_state_head, it's not longer used after hot-del, so free it, too.

Signed-off-by: liujunjie 
Signed-off-by: linzhecheng 
---
 accel/kvm/kvm-all.c  |  3 +++
 cpus.c   |  6 ++
 include/sysemu/kvm.h |  1 +
 target/i386/cpu.h|  2 ++
 target/i386/kvm.c| 19 ++-
 5 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ffee68e..a0491dc 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -305,6 +305,9 @@ int kvm_destroy_vcpu(CPUState *cpu)
 vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
 vcpu->kvm_fd = cpu->kvm_fd;
 QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+#ifdef TARGET_I386
+kvm_arch_destroy_vcpu(cpu);
+#endif
 err:
 return ret;
 }
diff --git a/cpus.c b/cpus.c
index d1f1629..cbe28d6 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1842,6 +1842,12 @@ void cpu_remove_sync(CPUState *cpu)
 qemu_mutex_unlock_iothread();
 qemu_thread_join(cpu->thread);
 qemu_mutex_lock_iothread();
+g_free(cpu->thread);
+cpu->thread = NULL;
+g_free(cpu->halt_cond);
+cpu->halt_cond = NULL;
+g_free(cpu->cpu_ases);
+cpu->cpu_ases = NULL;
 }
 
 /* For temporary buffers for forming a name */
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 0b64b8e..eb277f4 100644
--- a/include/sysemu/kvm.h

[Qemu-devel] [PATCH v4] target/ppc: Allow PIR read in privileged mode

[Greg Kurz]

From: luporl 

According to PowerISA, the PIR register should be readable in privileged
mode also, not only in hypervisor privileged mode.

PowerISA 3.0 - 4.3.3 Processor Identification Register

"Read access to the PIR is privileged; write access is not provided."

Figure 18 in section 4.4.4 explicitly confirms that mfspr PIR is privileged
and doesn't require hypervisor state.

Cc: David Gibson 
Cc: Alexander Graf 
Cc: qemu-...@nongnu.org
Signed-off-by: Leandro Lupori 
Reviewed-by: Jose Ricardo Ziviani 
Reviewed-by: Greg Kurz 
Signed-off-by: Greg Kurz 
---
Changes in v2:
- added my Signed-off-by, maintainers CC and Jose's Reviewed-by tags

Changes in v3:
- added subsystem name, version tag and summary of changes
- added the section of PowerISA that describes PIR access privileges

Changes in v4 (Greg):
- rebased against ppc-for-3.0 (ie, file is now target/ppc/translate_init.inc.c)
- added some more context from PowerISA
---
 target/ppc/translate_init.inc.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 1a89017ddea8..bb9296f5a3da 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -7819,7 +7819,7 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
 /* Processor identification */
 spr_register_hv(env, SPR_PIR, "PIR",
  SPR_NOACCESS, SPR_NOACCESS,
- SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_generic, SPR_NOACCESS,
  &spr_read_generic, NULL,
  0x);
 spr_register_hv(env, SPR_HID0, "HID0",

Re: [Qemu-devel] [qemu-s390x] [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers

[David Hildenbrand]

On 01.06.2018 14:13, Igor Mammedov wrote:
> On Fri, 25 May 2018 14:43:39 +0200
> David Hildenbrand  wrote:
> 
>> On 17.05.2018 10:15, David Hildenbrand wrote:
>>> We can have devices that need certain other resources that are e.g.
>>> system resources managed by the machine. We need a clean way to assign
>>> these resources (without violating layers as brought up by Igor).
>>>
>>> One example is virtio-mem/virtio-pmem. Both device types need to be
>>> assigned some region in guest physical address space. This device memory
>>> belongs to the machine and is managed by it. However, virito devices are
>>> hotplugged using the hotplug handler their proxy device implements. So we
>>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
>>> hotplug handler for virtio-ccw. But definetly not the machine.
>>>
>>> Now, we can route other devices through the machine hotplug handler, to
>>> properly assign/unassign resources - like a portion in guest physical
>>> address space.
> 
> To sum up review:
> Some comments apply to several patches even though I commented only once.
> 
> I'd suggest to restructure and split series into several:
>   * unplug cleanups 08/14 & co
>   * generic _preplug refactoring so we won't have to go back to that question 
> again

Looking into the details, I don't think this is possible and should be
done for address asignment. It can be done for the node. Here is the
reason why:

In pre_plug(), we acccess a device before it has been realized. This is
okay, as long as we are using "direct properties" like the "node", that
won't be touched during realize.

However, for address detection we need access to the memory region. This
is a "derived property", as it is based on the "memdev" property.

While we can easily verify the validity of "memdev" in the pc_dimm
pre_plug handler (instead of doing that in pc_dimm_realize()), we cannot
access the memory region itself before the device has been realized
using get_memory_region() in all cases.

While this works for PC_DIMM (pc_dimm_get_memory_region() is only based
on dimm->hostmem, which we can verify as stated), we cannot do the same
thing for NVDIMM, because nvdimm_get_memory_region() relies on "realize"
already being called and setting up "nvdimm->nvdimm_mr".

In short: we should call device functions only after realize(),
therefore address assignment is not possible during pre_plug.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH] block/qcow2-bitmap: fix free_bitmap_clusters

[Vladimir Sementsov-Ogievskiy]

This assert may fail, because bitmap_table is not initialized. Just
drop it, as it's obvious, that bitmap_table_load sets bitmap_table
parameter only when returning zero.

Reported-by: Pavel Butsykin 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 60d5290f10..69485aa1de 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -254,7 +254,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, 
Qcow2BitmapTable *tb)
 
 ret = bitmap_table_load(bs, tb, &bitmap_table);
 if (ret < 0) {
-assert(bitmap_table == NULL);
 return ret;
 }
 
-- 
2.11.1

Re: [Qemu-devel] [PULL 0/5] slirp updates

[Peter Maydell]

On 8 June 2018 at 07:13, Samuel Thibault  wrote:
> The following changes since commit 9be4af13305f24d2dabf94bb53e6b65c76d08bb2:
>
>   Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into 
> staging (2018-06-01 14:58:53 +0100)
>
> are available in the Git repository at:
>
>   https://people.debian.org/~sthibault/qemu.git tags/samuel-thibault
>
> for you to fetch changes up to c22098c74a09164797fae6511c5eaf68f32c4dd8:
>
>   slirp: reformat m_inc routine (2018-06-08 09:08:30 +0300)
>
> 
> slirp updates
>
> Prasad J Pandit (2):
>   slirp: Fix buffer overflow on packet reassembling
>
> Samuel Thibault (3):
>   slirp: Add Samuel Thibault's staging tree for slirp
>   slirp: fix domainname version availability
>


Applied, thanks.

-- PMM

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Fri, 8 Jun 2018 11:24:51 +0200
David Hildenbrand  wrote:

>  +1 for error_abort, even if it takes another line.
> >>> +1 for error_abort
> >>> call shouldn't fail, but if does it won't be silently ignored
> >>> and introduce undefined behavior.
> >>
> >> Maybe we should fix the others that pass in NULL.
> >>
> >> (no, not me :D - I'm already busy with your requested pre_plug handling)  
> > Add it to wiki page for bite sized tasks?  
> 
> Done.
> 
> 

FWIW, I've also added a line to check and possibly fix places where we do
'if (*errp)', which would cause QEMU to crash if the caller passes NULL.

$ git grep 'if (\*errp)'
hmp.c:if (*errp) {
hw/ipmi/isa_ipmi_bt.c:if (*errp)
hw/ipmi/isa_ipmi_kcs.c:if (*errp)
hw/mem/memory-device.c:if (*errp) {
hw/mem/memory-device.c:if (*errp) {
hw/ppc/spapr.c:if (*errp) {
hw/s390x/event-facility.c:if (*errp) {
include/qapi/error.h: * if (*errp) { // WRONG!
qga/commands-posix.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
target/s390x/cpu_models.c:if (*errp) {
tests/test-crypto-tlscredsx509.c:if (*errp) {
tests/test-io-channel-tls.c:if (*errp) {

Re: [Qemu-devel] [PATCH 00/11] misc: Add trailing '\n' to qemu_log() calls

[Peter Maydell]

On 6 June 2018 at 20:43, John Snow  wrote:
> so error_setg must be used WITHOUT \n and logging must happen with \n?
>
> If we're sure that's the way we want to have things laid out, we really
> ought to augment checkpatch to catch this -- because there's 0% chance
> that we'll keep it straight on our own otherwise.

It's a historical side effect of them being basically separately
designed APIs. error_setg() takes the "give it the entire error
message" approach. logging is printf-style and gives the caller
flexibility to compose more complicated lines of logging with
multiple calls. Consistency between the two might be nice but
there are an awful lot of qemu_log calls in the codebase to check
and change to the other convention...

thanks
-- PMM

[Qemu-devel] [PATCH v3 6/6] bochs-display: enable vgabios

[Gerd Hoffmann]

---
 hw/display/bochs-display.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 1187d77576..12d8a66c6c 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -337,6 +337,7 @@ static void bochs_display_class_init(ObjectClass *klass, 
void *data)
 k->device_id = PCI_DEVICE_ID_QEMU_VGA;
 
 k->realize   = bochs_display_realize;
+k->romfile   = "vgabios-bochs-display.bin";
 k->exit  = bochs_display_exit;
 dc->vmsd = &vmstate_bochs_display;
 dc->props= bochs_display_properties;
-- 
2.9.3

[Qemu-devel] [PATCH v3 2/6] hw/display: add standalone ramfb device

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 include/hw/display/ramfb.h|  3 +++
 hw/arm/sysbus-fdt.c   |  7 +
 hw/arm/virt.c |  2 ++
 hw/display/ramfb-standalone.c | 62 +++
 hw/i386/pc_piix.c |  2 ++
 hw/i386/pc_q35.c  |  2 ++
 hw/display/Makefile.objs  |  1 +
 7 files changed, 79 insertions(+)
 create mode 100644 hw/display/ramfb-standalone.c

diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index a3d4c79942..b33a2c467b 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -6,4 +6,7 @@ typedef struct RAMFBState RAMFBState;
 void ramfb_display_update(QemuConsole *con, RAMFBState *s);
 RAMFBState *ramfb_setup(Error **errp);
 
+/* ramfb-standalone.c */
+#define TYPE_RAMFB_DEVICE "ramfb"
+
 #endif /* RAMFB_H */
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index e4c492ea44..277ed872e7 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -36,6 +36,7 @@
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
 #include "hw/vfio/vfio-amd-xgbe.h"
+#include "hw/display/ramfb.h"
 #include "hw/arm/fdt.h"
 
 /*
@@ -406,12 +407,18 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, 
void *opaque)
 
 #endif /* CONFIG_LINUX */
 
+static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+return 0;
+}
+
 /* list of supported dynamic sysbus devices */
 static const NodeCreationPair add_fdt_node_functions[] = {
 #ifdef CONFIG_LINUX
 {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
 {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
 #endif
+{TYPE_RAMFB_DEVICE, no_fdt_node},
 {"", NULL}, /* last element */
 };
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f0a4fa004c..98b99cf236 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -36,6 +36,7 @@
 #include "hw/arm/virt.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
 #include "hw/vfio/vfio-amd-xgbe.h"
+#include "hw/display/ramfb.h"
 #include "hw/devices.h"
 #include "net/net.h"
 #include "sysemu/device_tree.h"
@@ -1659,6 +1660,7 @@ static void virt_machine_class_init(ObjectClass *oc, void 
*data)
 mc->max_cpus = 255;
 machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
 machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
+machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
 mc->block_default_type = IF_VIRTIO;
 mc->no_cdrom = 1;
 mc->pci_allow_0_address = true;
diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
new file mode 100644
index 00..05d2b9839e
--- /dev/null
+++ b/hw/display/ramfb-standalone.c
@@ -0,0 +1,62 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/loader.h"
+#include "hw/isa/isa.h"
+#include "hw/display/ramfb.h"
+#include "ui/console.h"
+#include "sysemu/sysemu.h"
+
+#define RAMFB(obj) OBJECT_CHECK(RAMFBState, (obj), TYPE_RAMFB_DEVICE)
+
+typedef struct RAMFBState {
+SysBusDevice parent_obj;
+QemuConsole *con;
+RAMFBState *state;
+} RAMFBState;
+
+static void display_update_wrapper(void *dev)
+{
+RAMFBState *ramfb = RAMFB(dev);
+
+if (0 /* native driver active */) {
+/* non-standalone device would run native display update here */;
+} else {
+ramfb_display_update(ramfb->con, ramfb->state);
+}
+}
+
+static const GraphicHwOps wrapper_ops = {
+.gfx_update = display_update_wrapper,
+};
+
+static void ramfb_realizefn(DeviceState *dev, Error **errp)
+{
+RAMFBState *ramfb = RAMFB(dev);
+
+ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
+ramfb->state = ramfb_setup(errp);
+}
+
+static void ramfb_class_initfn(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+dc->realize = ramfb_realizefn;
+dc->desc = "ram framebuffer standalone device";
+dc->user_creatable = true;
+}
+
+static const TypeInfo ramfb_info = {
+.name  = TYPE_RAMFB_DEVICE,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(RAMFBState),
+.class_init= ramfb_class_initfn,
+};
+
+static void ramfb_register_types(void)
+{
+type_register_static(&ramfb_info);
+}
+
+type_init(ramfb_register_types)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3d81136065..5dc4220d02 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -28,6 +28,7 @@
 #include "hw/loader.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
+#include "hw/display/ramfb.h"
 #include "hw/smbios/smbios.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_ids.h"
@@ -423,6 +424,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
 m->desc = "Standard PC (i440FX + PIIX, 1996)";
 m->default_machine_opts = "firmware=bios-256k.bin";
 m->default_display = "std";
+machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
 }
 
 static void pc_i440fx_3_0_machine_options(MachineClass *m)
diff --git

[Qemu-devel] [PATCH v3 4/6] hw/vfio/display: add ramfb support

[Gerd Hoffmann]

So we have a boot display when using a vgpu as primary display.

Use vfio-pci-ramfb instead of vfio-pci to enable it.

Signed-off-by: Gerd Hoffmann 
---
 include/hw/vfio/vfio-common.h |  2 ++
 hw/vfio/display.c | 10 ++
 hw/vfio/pci.c | 15 +++
 3 files changed, 27 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index a9036929b2..a58d7e7e77 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -26,6 +26,7 @@
 #include "qemu/queue.h"
 #include "qemu/notify.h"
 #include "ui/console.h"
+#include "hw/display/ramfb.h"
 #ifdef CONFIG_LINUX
 #include 
 #endif
@@ -143,6 +144,7 @@ typedef struct VFIODMABuf {
 
 typedef struct VFIODisplay {
 QemuConsole *con;
+RAMFBState *ramfb;
 struct {
 VFIORegion buffer;
 DisplaySurface *surface;
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 59c0e5d1d7..409d5a2e3a 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -124,6 +124,9 @@ static void vfio_display_dmabuf_update(void *opaque)
 
 primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
 if (primary == NULL) {
+if (dpy->ramfb) {
+ramfb_display_update(dpy->con, dpy->ramfb);
+}
 return;
 }
 
@@ -181,6 +184,8 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, 
Error **errp)
 vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
   &vfio_display_dmabuf_ops,
   vdev);
+if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0)
+vdev->dpy->ramfb = ramfb_setup(errp);
 return 0;
 }
 
@@ -228,6 +233,9 @@ static void vfio_display_region_update(void *opaque)
 return;
 }
 if (!plane.drm_format || !plane.size) {
+if (dpy->ramfb) {
+ramfb_display_update(dpy->con, dpy->ramfb);
+}
 return;
 }
 format = qemu_drm_format_to_pixman(plane.drm_format);
@@ -300,6 +308,8 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, 
Error **errp)
 vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
   &vfio_display_region_ops,
   vdev);
+if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0)
+vdev->dpy->ramfb = ramfb_setup(errp);
 return 0;
 }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 18c493b49e..6a2b42a595 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3234,9 +3234,24 @@ static const TypeInfo vfio_pci_dev_info = {
 },
 };
 
+static void vfio_pci_ramfb_dev_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->hotpluggable = false;
+}
+
+static const TypeInfo vfio_pci_ramfb_dev_info = {
+.name = "vfio-pci-ramfb",
+.parent = "vfio-pci",
+.instance_size = sizeof(VFIOPCIDevice),
+.class_init = vfio_pci_ramfb_dev_class_init,
+};
+
 static void register_vfio_pci_dev_type(void)
 {
 type_register_static(&vfio_pci_dev_info);
+type_register_static(&vfio_pci_ramfb_dev_info);
 }
 
 type_init(register_vfio_pci_dev_type)
-- 
2.9.3

[Qemu-devel] [PATCH v3 5/6] ramfb: enable vgabios

[Gerd Hoffmann]

---
 hw/display/ramfb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index 258783fe3b..477316a14d 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -88,6 +88,7 @@ RAMFBState *ramfb_setup(Error **errp)
 
 s = g_new0(RAMFBState, 1);
 
+rom_add_vga("vgabios-ramfb.bin");
 fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
  NULL, ramfb_fw_cfg_write, s,
  &s->cfg, sizeof(s->cfg), false);
-- 
2.9.3

[Qemu-devel] [PATCH v3 3/6] hw/display: add virtio-ramfb device

[Gerd Hoffmann]

Like virtio-vga, but using ramfb instead of legacy vga.

Note: Not clear yet whenever this will be ever merged upstream.
  No clear benefit, given that edk2 has a virtio-gpu driver
  which doesn't depend on the vga compatibility mode.

  Will keeping it in the devel branch for now, for testing
  purposes and as playground.

Signed-off-by: Gerd Hoffmann 
---
 hw/display/virtio-ramfb.c | 149 ++
 hw/display/Makefile.objs  |   2 +-
 2 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 hw/display/virtio-ramfb.c

diff --git a/hw/display/virtio-ramfb.c b/hw/display/virtio-ramfb.c
new file mode 100644
index 00..e8ffe66047
--- /dev/null
+++ b/hw/display/virtio-ramfb.c
@@ -0,0 +1,149 @@
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "hw/pci/pci.h"
+#include "ui/console.h"
+#include "hw/virtio/virtio-pci.h"
+#include "hw/display/ramfb.h"
+#include "qapi/error.h"
+
+/*
+ * virtio-ramfb: This extends VirtioPCIProxy.
+ */
+#define TYPE_VIRTIO_RAMFB "virtio-ramfb"
+#define VIRTIO_RAMFB(obj) \
+OBJECT_CHECK(VirtIORAMFB, (obj), TYPE_VIRTIO_RAMFB)
+
+typedef struct VirtIORAMFB {
+VirtIOPCIProxy parent_obj;
+VirtIOGPU  vdev;
+RAMFBState *ramfb;
+} VirtIORAMFB;
+
+static void virtio_ramfb_invalidate_display(void *opaque)
+{
+VirtIORAMFB *vramfb = opaque;
+
+if (vramfb->vdev.enable) {
+virtio_gpu_ops.invalidate(&vramfb->vdev);
+}
+}
+
+static void virtio_ramfb_update_display(void *opaque)
+{
+VirtIORAMFB *vramfb = opaque;
+VirtIOGPU *g = &vramfb->vdev;
+
+if (vramfb->vdev.enable) {
+virtio_gpu_ops.gfx_update(&vramfb->vdev);
+} else {
+ramfb_display_update(g->scanout[0].con, vramfb->ramfb);
+}
+}
+
+static int virtio_ramfb_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
+{
+VirtIORAMFB *vramfb = opaque;
+
+if (virtio_gpu_ops.ui_info) {
+return virtio_gpu_ops.ui_info(&vramfb->vdev, idx, info);
+}
+return -1;
+}
+
+static void virtio_ramfb_gl_block(void *opaque, bool block)
+{
+VirtIORAMFB *vramfb = opaque;
+
+if (virtio_gpu_ops.gl_block) {
+virtio_gpu_ops.gl_block(&vramfb->vdev, block);
+}
+}
+
+static const GraphicHwOps virtio_ramfb_ops = {
+.invalidate = virtio_ramfb_invalidate_display,
+.gfx_update = virtio_ramfb_update_display,
+.ui_info = virtio_ramfb_ui_info,
+.gl_block = virtio_ramfb_gl_block,
+};
+
+static const VMStateDescription vmstate_virtio_ramfb = {
+.name = "virtio-ramfb",
+.version_id = 2,
+.minimum_version_id = 2,
+.fields = (VMStateField[]) {
+/* no pci stuff here, saving the virtio device will handle that */
+/* FIXME */
+VMSTATE_END_OF_LIST()
+}
+};
+
+/* RAMFB device wrapper around PCI device around virtio GPU */
+static void virtio_ramfb_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIORAMFB *vramfb = VIRTIO_RAMFB(vpci_dev);
+VirtIOGPU *g = &vramfb->vdev;
+Error *err = NULL;
+int i;
+
+/* init virtio bits */
+qdev_set_parent_bus(DEVICE(g), BUS(&vpci_dev->bus));
+virtio_pci_force_virtio_1(vpci_dev);
+object_property_set_bool(OBJECT(g), true, "realized", &err);
+if (err) {
+error_propagate(errp, err);
+return;
+}
+
+/* init ramfb */
+vramfb->ramfb = ramfb_setup(errp);
+graphic_console_set_hwops(g->scanout[0].con, &virtio_ramfb_ops, vramfb);
+
+for (i = 0; i < g->conf.max_outputs; i++) {
+object_property_set_link(OBJECT(g->scanout[i].con),
+ OBJECT(vpci_dev),
+ "device", errp);
+}
+}
+
+static Property virtio_ramfb_properties[] = {
+DEFINE_VIRTIO_GPU_PCI_PROPERTIES(VirtIOPCIProxy),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_ramfb_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+
+set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+dc->props = virtio_ramfb_properties;
+dc->vmsd = &vmstate_virtio_ramfb;
+dc->hotpluggable = false;
+
+k->realize = virtio_ramfb_realize;
+pcidev_k->class_id = PCI_CLASS_DISPLAY_OTHER;
+}
+
+static void virtio_ramfb_inst_initfn(Object *obj)
+{
+VirtIORAMFB *dev = VIRTIO_RAMFB(obj);
+
+virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+TYPE_VIRTIO_GPU);
+}
+
+static TypeInfo virtio_ramfb_info = {
+.name  = TYPE_VIRTIO_RAMFB,
+.parent= TYPE_VIRTIO_PCI,
+.instance_size = sizeof(struct VirtIORAMFB),
+.instance_init = virtio_ramfb_inst_initfn,
+.class_init= virtio_ramfb_class_init,
+};
+
+static void virtio_ramfb_register_types(void)
+{
+type_register_static(&virtio_ramfb_info);
+}
+
+type_init(virtio_ramfb_register_types)
diff --git a/hw/display/Makefile.objs b/hw/displ

[Qemu-devel] [PATCH v3 1/6] hw/display: add ramfb, a simple boot framebuffer living in guest ram

[Gerd Hoffmann]

The boot framebuffer is expected to be configured by the firmware, so it
uses fw_cfg as interface.  Initialization goes as follows:

  (1) Check whenever etc/ramfb is present.
  (2) Allocate framebuffer from RAM.
  (3) Fill struct RAMFBCfg, write it to etc/ramfb.

Done.  You can write stuff to the framebuffer now, and it should appear
automagically on the screen.

Note that this isn't very efficient because it does a full display
update on each refresh.  No dirty tracking.  Dirty tracking would have
to be active for the whole ram slot, so that wouldn't be very efficient
either.  For a boot display which is active for a short time only this
isn't a big deal.  As permanent guest display something better should be
used (if possible).

This is the ramfb core code.  Some windup is needed for display devices
which want have a ramfb boot display.

Signed-off-by: Gerd Hoffmann 
---
 include/hw/display/ramfb.h |  9 +
 hw/display/ramfb.c | 95 ++
 hw/display/Makefile.objs   |  2 +
 3 files changed, 106 insertions(+)
 create mode 100644 include/hw/display/ramfb.h
 create mode 100644 hw/display/ramfb.c

diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
new file mode 100644
index 00..a3d4c79942
--- /dev/null
+++ b/include/hw/display/ramfb.h
@@ -0,0 +1,9 @@
+#ifndef RAMFB_H
+#define RAMFB_H
+
+/* ramfb.c */
+typedef struct RAMFBState RAMFBState;
+void ramfb_display_update(QemuConsole *con, RAMFBState *s);
+RAMFBState *ramfb_setup(Error **errp);
+
+#endif /* RAMFB_H */
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
new file mode 100644
index 00..258783fe3b
--- /dev/null
+++ b/hw/display/ramfb.c
@@ -0,0 +1,95 @@
+/*
+ * early boot framebuffer in guest ram
+ * configured using fw_cfg
+ *
+ * Copyright Red Hat, Inc. 2017
+ *
+ * Author:
+ * Gerd Hoffmann 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/loader.h"
+#include "hw/display/ramfb.h"
+#include "ui/console.h"
+#include "sysemu/sysemu.h"
+
+struct RAMFBCfg {
+uint64_t addr;
+uint32_t fourcc;
+uint32_t flags;
+uint32_t width;
+uint32_t height;
+uint32_t stride;
+};
+
+struct RAMFBState {
+DisplaySurface *ds;
+uint32_t width, height;
+struct RAMFBCfg cfg;
+};
+
+static void ramfb_fw_cfg_write(void *dev, off_t offset, size_t len)
+{
+RAMFBState *s = dev;
+void *framebuffer;
+uint32_t stride, fourcc, format;
+hwaddr addr, length;
+
+s->width  = be32_to_cpu(s->cfg.width);
+s->height = be32_to_cpu(s->cfg.height);
+stride= be32_to_cpu(s->cfg.stride);
+fourcc= be32_to_cpu(s->cfg.fourcc);
+addr  = be64_to_cpu(s->cfg.addr);
+length= stride * s->height;
+format= qemu_drm_format_to_pixman(fourcc);
+
+fprintf(stderr, "%s: %dx%d @ 0x%" PRIx64 "\n", __func__,
+s->width, s->height, addr);
+framebuffer = address_space_map(&address_space_memory,
+addr, &length, false,
+MEMTXATTRS_UNSPECIFIED);
+if (!framebuffer || length < stride * s->height) {
+s->width = 0;
+s->height = 0;
+return;
+}
+s->ds = qemu_create_displaysurface_from(s->width, s->height,
+format, stride, framebuffer);
+}
+
+void ramfb_display_update(QemuConsole *con, RAMFBState *s)
+{
+if (!s->width || !s->height) {
+return;
+}
+
+if (s->ds) {
+dpy_gfx_replace_surface(con, s->ds);
+s->ds = NULL;
+}
+
+/* simple full screen update */
+dpy_gfx_update_full(con);
+}
+
+RAMFBState *ramfb_setup(Error **errp)
+{
+FWCfgState *fw_cfg = fw_cfg_find();
+RAMFBState *s;
+
+if (!fw_cfg || !fw_cfg->dma_enabled) {
+error_setg(errp, "ramfb device requires fw_cfg with DMA");
+return NULL;
+}
+
+s = g_new0(RAMFBState, 1);
+
+fw_cfg_add_file_callback(fw_cfg, "etc/ramfb",
+ NULL, ramfb_fw_cfg_write, s,
+ &s->cfg, sizeof(s->cfg), false);
+return s;
+}
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index b5d97ab26d..0af04985d2 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -1,3 +1,5 @@
+common-obj-y += ramfb.o
+
 common-obj-$(CONFIG_ADS7846) += ads7846.o
 common-obj-$(CONFIG_VGA_CIRRUS) += cirrus_vga.o
 common-obj-$(CONFIG_G364FB) += g364fb.o
-- 
2.9.3

[Qemu-devel] [PATCH v3 0/6] ramfb: simple boot framebuffer, no legacy vga

[Gerd Hoffmann]

  Hi,

Ok folks, here is a experimental patch series for a legacy free boot
framebuffer.  If you want play with it I recommend getting the bits from

https://www.kraxel.org/cgit/qemu/log/?h=sirius/ramfb

because they come with an updated seabios and a new vgabios rom and an
experimental OVMF build.

Functional overview
---

The boot framebuffer is expected to be configured by the firmware, so it
uses fw_cfg as interface.  Initialization goes as follows:

  (1) Check whenever etc/ramfb is present.
  (2) Allocate framebuffer from RAM.
  (3) Fill struct RAMFBCfg, write it to etc/ramfb.

Done.  You can write stuff to the framebuffer now, and it should appear
automagically on the screen.

Note that this isn't very efficient because it does a full display
update on each refresh.  No dirty tracking.  Dirty tracking would have
to be active for the whole ram slot, so that wouldn't be very efficient
either.

Firmware support -- seabios
---

seavgabios is able to emulate vga text mode on top of a framebuffer, for
coreboot native graphics initialialization.  Which works fine for
everything which writes text using the vgabios interface (basically
everyhing which works with sgabios).

So I hacked that up to work with ramfb (and, while being at it,
bochs-display too).  Right now it's proof-of-concept code with fwcfg
support being cut+paste, so it'll need cleanups before merging.
Look here:

https://www.kraxe.org/cgit/seabios/log/?h=bochs

Firmware support -- edk2


There is a EFI driver too.  Code is here:

https://github.com/kraxel/edk2/commits/ramfb

Firmware blob is in pc-bios/OVMF-ramfb.fd, to be used with -bios.

So, how to play?


There is ramfb.  Standalone device.

There is virtio-ramfb.  Simliar to virtio-vga, but using ramfb instead of
adding vga compatibility.  Shows how you can wire up ramfb support to
some display device.  Unlike virtio-vga it should work fine on arm.  Use
"qemu -vga none -device virtio-ramfb" for this one.  Not clear whenever
this will be actually be merged, given that edk2 has a native virtio-gpu
driver.

There is virtio-pci-ramfb, which provides boot display support to vgpu
devices.

What works?
---

Both windows (x86 tested) and linux (x86 + arm tested) UEFI guests
handle the ramfb GOP just fine.

BIOS boot loaders for linux all use vgabios calls for text mode, so they
show up just fine.  Also ipxe, seabios itself of course.  So you can
boot up your linux guest.  vesafb works too.

Windows in BIOS mode doesn't use text mode and works fine too.

What doesn't work?
--

vgacon (direct vga hardware access).  Linux boots just fine
nevertheless, the only effect is that you don't see any boot messages
until the drm driver loads.

Known issues


Handover from ramfb-backed efifb to the native linux driver is tricky.
Usually efifb gets kicked out when the native driver loads because of
overlapping ressources.  With efifb being in RAM instead of using a GPU
PCI bar this doesn't happen though, so you'll end up with two
framebuffer devices.

In case vgaarb classifies the GPU as primary display device fbcon will
switch all VTs over to the framebuffer device of the real GPU, so there
isn't a noticable difference.  Otherwise you'll end up with a
non-visible fbcon, because it continues to run on ramfb whereas qemu
switched over to the GPU because the native linux driver initialized the
display.

xorg/wayland will show up on the GPU in any case because they prefer drm
over fbdev, so they wouldn't run on efifb.

enjoy,
  Gerd

Gerd Hoffmann (6):
  hw/display: add ramfb, a simple boot framebuffer living in guest ram
  hw/display: add standalone ramfb device
  hw/display: add virtio-ramfb device
  hw/vfio/display: add ramfb support
  ramfb: enable vgabios
  bochs-display: enable vgabios

 include/hw/display/ramfb.h|  12 
 include/hw/vfio/vfio-common.h |   2 +
 hw/arm/sysbus-fdt.c   |   7 ++
 hw/arm/virt.c |   2 +
 hw/display/bochs-display.c|   1 +
 hw/display/ramfb-standalone.c |  62 ++
 hw/display/ramfb.c|  96 +++
 hw/display/virtio-ramfb.c | 149 ++
 hw/i386/pc_piix.c |   2 +
 hw/i386/pc_q35.c  |   2 +
 hw/vfio/display.c |  10 +++
 hw/vfio/pci.c |  15 +
 hw/display/Makefile.objs  |   5 +-
 13 files changed, 364 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/display/ramfb.h
 create mode 100644 hw/display/ramfb-standalone.c
 create mode 100644 hw/display/ramfb.c
 create mode 100644 hw/display/virtio-ramfb.c

-- 
2.9.3

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 12:52, Greg Kurz wrote:
> On Fri, 8 Jun 2018 11:24:51 +0200
> David Hildenbrand  wrote:
> 
>> +1 for error_abort, even if it takes another line.
> +1 for error_abort
> call shouldn't fail, but if does it won't be silently ignored
> and introduce undefined behavior.

 Maybe we should fix the others that pass in NULL.

 (no, not me :D - I'm already busy with your requested pre_plug handling)  
>>> Add it to wiki page for bite sized tasks?  
>>
>> Done.
>>
>>
> 
> FWIW, I've also added a line to check and possibly fix places where we do
> 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.
> 
> $ git grep 'if (\*errp)'
> hmp.c:if (*errp) {
> hw/ipmi/isa_ipmi_bt.c:if (*errp)
> hw/ipmi/isa_ipmi_kcs.c:if (*errp)
> hw/mem/memory-device.c:if (*errp) {
> hw/mem/memory-device.c:if (*errp) {
> hw/ppc/spapr.c:if (*errp) {
> hw/s390x/event-facility.c:if (*errp) {
> include/qapi/error.h: * if (*errp) { // WRONG!
> qga/commands-posix.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> target/s390x/cpu_models.c:if (*errp) {
> tests/test-crypto-tlscredsx509.c:if (*errp) {
> tests/test-io-channel-tls.c:if (*errp) {
> 

I think the more important part is actually looking out for people that
use NULL instead of error_abort. This way we won't silently ignore errors.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Cornelia Huck]

On Fri, 8 Jun 2018 13:28:01 +0200
David Hildenbrand  wrote:

> On 08.06.2018 12:52, Greg Kurz wrote:
> > On Fri, 8 Jun 2018 11:24:51 +0200
> > David Hildenbrand  wrote:
> >   
> >> +1 for error_abort, even if it takes another line.  
> > +1 for error_abort
> > call shouldn't fail, but if does it won't be silently ignored
> > and introduce undefined behavior.  
> 
>  Maybe we should fix the others that pass in NULL.
> 
>  (no, not me :D - I'm already busy with your requested pre_plug handling) 
> 
> >>> Add it to wiki page for bite sized tasks?
> >>
> >> Done.
> >>
> >>  
> > 
> > FWIW, I've also added a line to check and possibly fix places where we do
> > 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.
> > 
> > $ git grep 'if (\*errp)'
> > hmp.c:if (*errp) {
> > hw/ipmi/isa_ipmi_bt.c:if (*errp)
> > hw/ipmi/isa_ipmi_kcs.c:if (*errp)
> > hw/mem/memory-device.c:if (*errp) {
> > hw/mem/memory-device.c:if (*errp) {
> > hw/ppc/spapr.c:if (*errp) {
> > hw/s390x/event-facility.c:if (*errp) {
> > include/qapi/error.h: * if (*errp) { // WRONG!
> > qga/commands-posix.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > tests/test-crypto-tlscredsx509.c:if (*errp) {
> > tests/test-io-channel-tls.c:if (*errp) {
> >   
> 
> I think the more important part is actually looking out for people that
> use NULL instead of error_abort. This way we won't silently ignore errors.

I think we can assume that the callers here all pass in !NULL. Would
probably make sense to change these anyway because (a) better safe than
sorry, and (b) make sure new code does not copy it.

Re: [Qemu-devel] [PATCH v4] target/ppc: Allow PIR read in privileged mode

[David Gibson]

On Fri, Jun 08, 2018 at 11:46:55AM +0200, Greg Kurz wrote:
> From: luporl 
> 
> According to PowerISA, the PIR register should be readable in privileged
> mode also, not only in hypervisor privileged mode.
> 
> PowerISA 3.0 - 4.3.3 Processor Identification Register
> 
> "Read access to the PIR is privileged; write access is not provided."
> 
> Figure 18 in section 4.4.4 explicitly confirms that mfspr PIR is privileged
> and doesn't require hypervisor state.

Applied, thanks.

> 
> Cc: David Gibson 
> Cc: Alexander Graf 
> Cc: qemu-...@nongnu.org
> Signed-off-by: Leandro Lupori 
> Reviewed-by: Jose Ricardo Ziviani 
> Reviewed-by: Greg Kurz 
> Signed-off-by: Greg Kurz 
> ---
> Changes in v2:
> - added my Signed-off-by, maintainers CC and Jose's Reviewed-by tags
> 
> Changes in v3:
> - added subsystem name, version tag and summary of changes
> - added the section of PowerISA that describes PIR access privileges
> 
> Changes in v4 (Greg):
> - rebased against ppc-for-3.0 (ie, file is now 
> target/ppc/translate_init.inc.c)
> - added some more context from PowerISA
> ---
>  target/ppc/translate_init.inc.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 1a89017ddea8..bb9296f5a3da 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -7819,7 +7819,7 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
>  /* Processor identification */
>  spr_register_hv(env, SPR_PIR, "PIR",
>   SPR_NOACCESS, SPR_NOACCESS,
> - SPR_NOACCESS, SPR_NOACCESS,
> + &spr_read_generic, SPR_NOACCESS,
>   &spr_read_generic, NULL,
>   0x);
>  spr_register_hv(env, SPR_HID0, "HID0",
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[Qemu-devel] [Bug 1467240] Re: Regression - bridged networking broken for Mac OS X guest

[Thomas Huth]

Patch had been included here:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=5df6a1855b62dc6535

** Changed in: qemu
   Status: New => Fix Released

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

Title:
  Regression - bridged networking broken for Mac OS X guest

Status in QEMU:
  Fix Released

Bug description:
  Using the instructions at
  http://www.contrib.andrew.cmu.edu/~somlo/OSXKVM/ for running Mac OS X
  Snow Leopard under QEMU, bridged networking is broken when using QEMU
  git. The result is that Mac OS X is unable to obtain an IP address
  using DHCP. It works in the latest stable release - QEMU 2.3.0.

  Replace "-netdev user,id=hub0port0" with "-netdev
  bridge,br=br0,id=hub0port0" when testing bridged networking.

  Bisecting the git repository shows the following bad commit:
  commit a90a7425cf592a3afeff3eaf32f543b83050ee5c
  Author: Fam Zheng 
  Date:   Thu Jun 4 14:45:17 2015 +0800

  tap: Drop tap_can_send

  This callback is called by main loop before polling s->fd, if it returns
  false, the fd will not be polled in this iteration.

  This is redundant with checks inside read callback. After this patch,
  the data will be sent to peer when it arrives. If the device can't
  receive, it will be queued to incoming_queue, and when the device status
  changes, this queue will be flushed.

  Signed-off-by: Fam Zheng 
  Message-id: 1433400324-7358-7-git-send-email-f...@redhat.com
  Signed-off-by: Stefan Hajnoczi 

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

Re: [Qemu-devel] [PATCH v3 0/6] ramfb: simple boot framebuffer, no legacy vga

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180608112001.14729-1-kra...@redhat.com
Subject: [Qemu-devel] [PATCH v3 0/6] ramfb: simple boot framebuffer, no legacy 
vga

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   a674da0ab7..bac5ba3dc5  master -> master
 * [new tag]   patchew/20180608112001.14729-1-kra...@redhat.com -> 
patchew/20180608112001.14729-1-kra...@redhat.com
Switched to a new branch 'test'
b3b768bdcf bochs-display: enable vgabios
be08ed14be ramfb: enable vgabios
f2f2069625 hw/vfio/display: add ramfb support
717edb2f4d hw/display: add virtio-ramfb device
cfb87c9b3b hw/display: add standalone ramfb device
add39ce518 hw/display: add ramfb, a simple boot framebuffer living in guest ram

=== OUTPUT BEGIN ===
Checking PATCH 1/6: hw/display: add ramfb, a simple boot framebuffer living in 
guest ram...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#41: 
new file mode 100644

total: 0 errors, 1 warnings, 109 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/6: hw/display: add standalone ramfb device...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#71: 
new file mode 100644

total: 0 errors, 1 warnings, 141 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/6: hw/display: add virtio-ramfb device...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#32: 
new file mode 100644

total: 0 errors, 1 warnings, 157 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/6: hw/vfio/display: add ramfb support...
ERROR: braces {} are necessary for all arms of this statement
#31: FILE: hw/vfio/display.c:187:
+if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0)
[...]

ERROR: braces {} are necessary for all arms of this statement
#50: FILE: hw/vfio/display.c:311:
+if (strcmp(object_get_typename(OBJECT(vdev)), "vfio-pci-ramfb") == 0)
[...]

total: 2 errors, 0 warnings, 72 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/6: ramfb: enable vgabios...
Checking PATCH 6/6: bochs-display: enable vgabios...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Fri, 8 Jun 2018 13:28:01 +0200
David Hildenbrand  wrote:

> On 08.06.2018 12:52, Greg Kurz wrote:
> > On Fri, 8 Jun 2018 11:24:51 +0200
> > David Hildenbrand  wrote:
> >   
> >> +1 for error_abort, even if it takes another line.  
> > +1 for error_abort
> > call shouldn't fail, but if does it won't be silently ignored
> > and introduce undefined behavior.  
> 
>  Maybe we should fix the others that pass in NULL.
> 
>  (no, not me :D - I'm already busy with your requested pre_plug handling) 
> 
> >>> Add it to wiki page for bite sized tasks?
> >>
> >> Done.
> >>
> >>  
> > 
> > FWIW, I've also added a line to check and possibly fix places where we do
> > 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.
> > 
> > $ git grep 'if (\*errp)'
> > hmp.c:if (*errp) {
> > hw/ipmi/isa_ipmi_bt.c:if (*errp)
> > hw/ipmi/isa_ipmi_kcs.c:if (*errp)
> > hw/mem/memory-device.c:if (*errp) {
> > hw/mem/memory-device.c:if (*errp) {
> > hw/ppc/spapr.c:if (*errp) {
> > hw/s390x/event-facility.c:if (*errp) {
> > include/qapi/error.h: * if (*errp) { // WRONG!
> > qga/commands-posix.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > target/s390x/cpu_models.c:if (*errp) {
> > tests/test-crypto-tlscredsx509.c:if (*errp) {
> > tests/test-io-channel-tls.c:if (*errp) {
> >   
> 
> I think the more important part is actually looking out for people that
> use NULL instead of error_abort. This way we won't silently ignore errors.
> 

I agree that we should probably pass &error_abort in many places,
but passing NULL isn't bad per se. It means any failure in the
callee is unimportant enough that we can simply ignore it.

The error framework provides this possibility and so we should
never dereference errp.

Re: [Qemu-devel] [PATCH 00/11] misc: Add trailing '\n' to qemu_log() calls

[Peter Maydell]

On 6 June 2018 at 16:21, Philippe Mathieu-Daudé  wrote:
> Nothing very exciting here.
> I sometimes miss to notice some trace events, running with -d unimp,trace...
> then using 'grep ^...'. This is only due to a missing '\n' :)
>
> Philippe Mathieu-Daudé (11):
>   hw/sd/milkymist-memcard: Add trailing '\n' to qemu_log() call
>   hw/digic: Add trailing '\n' to qemu_log() calls
>   xilinx-dp: Add trailing '\n' to qemu_log() call
>   ppc/pnv: Add trailing '\n' to qemu_log() calls
>   hw/core/register: Add trailing '\n' to qemu_log() call
>   hw/mips/boston: Add trailing '\n' to qemu_log() calls
>   stellaris: Add trailing '\n' to qemu_log() calls
>   target/arm: Add trailing '\n' to qemu_log() calls
>   target/m68k: Add trailing '\n' to qemu_log() call
>   RISC-V: Add trailing '\n' to qemu_log() calls
>   RFC target/xtensa: Add trailing '\n' to qemu_log() calls
>
>  hw/arm/stellaris.c| 11 ++-
>  hw/char/digic-uart.c  |  4 ++--
>  hw/core/register.c|  2 +-
>  hw/display/xlnx_dp.c  |  4 +++-
>  hw/mips/boston.c  |  8 
>  hw/ppc/pnv_core.c |  4 ++--
>  hw/sd/milkymist-memcard.c |  2 +-
>  hw/timer/digic-timer.c|  4 ++--
>  target/arm/helper.c   |  4 ++--
>  target/m68k/translate.c   |  2 +-
>  target/riscv/op_helper.c  |  6 --
>  target/xtensa/translate.c |  6 +++---
>  12 files changed, 31 insertions(+), 26 deletions(-)


Applied to target-arm.next, since about half of these are arm
and the rest are trivial and have got ack/review by the relevant
maintainers.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/3] glib: update the min required version

[Peter Maydell]

On 6 June 2018 at 18:32, Daniel P. Berrangé  wrote:
> The previous patch to bump glib to 2.42 hit problems with Peter's build
> environment for testing merge:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg02557.html
>
> This posting drops back to 2.40, which allows Ubuntu 14.04 from GLibC
> compile farm to be supported.
>
> It does NOT try to go back to 2.34, because it is hoped that the mxe.cc
> Debian packages will be suitable for Peter to test Windows
> cross-compile. Alternatively the docker environments provided in tree
> can be used for mingw build testing on any host able to run docker.
>
> I also dropped some more GLIB_CHECK_VERSION checks that are redundant
> given the new min version.

Note that updating to MXE is still on my todo list; I'll let
you know when I get to it...

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v2 7/7] hw/sd/ssi-sd: Force cards connected in SPI mode to use Spec v1.10

[Peter Maydell]

On 7 June 2018 at 19:06, Philippe Mathieu-Daudé  wrote:
> Due to physical restriction in SPI mode the maximum transfer
> speed is limited. All the extensions added after Spec v3 are
> simply not supported in SPI mode:

You say here that SPI mode doesn't support extensions added
"after spec v3"...

>   7.1 Introduction
>
> The SPI mode consists of a secondary communication protocol
> that is offered by Flash-based SD Memory Cards. This mode is
> a subset of the SD Memory Card protocol, designed to communicate
> with a SPI channel, commonly found in Motorola's (and lately a
> few other vendors') microcontrollers. The interface is selected
> during the first reset command after power up (CMD0) and cannot
> be changed once the part is powered on.
> The SPI standard defines the physical link only, and not the
> complete data transfer protocol. The SD Memory Card SPI
> implementation uses a subset of the SD Memory Card protocol and
> command set. The advantage of the SPI mode is the capability of
> using an off-the-shelf host, hence reducing the design-in effort
> to minimum. The disadvantage is the loss of performance of the
> SPI mode versus SD mode (e.g. Single data line and hardware CS
> signal per card).
> The commands and functions in SD mode defined after the Version
> 2.00 are not supported in SPI mode.

...but here quote the spec which says that commands defined after
version 2.00 are not supported in SPI mode...

and then the patch itself enforces spec 1.10, rather than 2.
So I'm confused.

> The card may respond to the
> commands and functions even if the card is in SPI mode but host
> should not use them in SPI mode.
>
> Some firmwares use the CMD8 in SPI mode to poll which Spec version
> the SD card supports.
>
>   7.2.1 Mode Selection and Initialization (SPI mode)
>
> The SD Card is powered up in the SD mode. It will enter SPI mode
> if the CS signal is asserted (negative) during the reception of
> the reset command (CMD0). If the card recognizes that the SD mode
> is required it will not respond to the command and remain in the
> SD mode. If SPI mode is required, the card will switch to SPI and
> respond with the SPI mode R1 response.
> The only way to return to the SD mode is by entering the power
> cycle. In SPI mode, the SD Card protocol state machine in SD mode
> is not observed. All the SD Card commands supported in SPI mode
> are always available. [...]
> If the card indicates an illegal command, the card is legacy and
> does not support CMD8. If the card supports CMD8 and can operate
> on the supplied voltage, the response echoes back the supply
> voltage and the check pattern that were set in the command
> argument.
>
> The NuttX RTOS use it too:
>
> /* Check for SDHC Version 2.x.  CMD 8 is reserved on SD version 1.0 and
>  * MMC.
>  */
> finfo("Send CMD8\n");
> result = mmcsd_sendcmd(slot, &g_cmd8, 0x1aa);
> if (result == MMCSD_SPIR1_IDLESTATE)
>   ...
> /* Check for SDC version 1.x or MMC */
> else
>   ...
>
> See 
> https://bitbucket.org/nuttx/nuttx/src/nuttx-7.25/drivers/mmcsd/mmcsd_spi.c?mmcsd_spi.c-1645#mmcsd_spi.c-1645

This seems reasonable guest behaviour, and it's what the spec says
to do. Why do we need to enforce 1.10 to get things to work?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/7] sdcard: cleanup the SD_SPEC version

[Peter Maydell]

On 7 June 2018 at 19:06, Philippe Mathieu-Daudé  wrote:
> Since v2:
> - heavy documentation improvements
> - squashed previous 3+4 "add spec_version property default to v2" for 
> atomicity
> - SSI/SD patch downgraded to RFC
> - added Alistair R-b tags

> Philippe Mathieu-Daudé (7):
>   sdcard: Update the Configuration Register (SCR) to Spec Version 1.10
>   sdcard: Allow commands valid in SPI mode
>   sdcard: Add a 'spec_version' property, default to Spec v2.00
>   sdcard: Disable SEND_IF_COND (CMD8) for Spec v1
>   sdcard: Reflect when the Spec v3 is supported in the Config Register (SCR)
>   sdcard: Disable CMD19/CMD23 for Spec v2
>   hw/sd/ssi-sd: Force cards connected in SPI mode to use Spec v1.10

Hi; I've applied patches 1-6 to target-arm.next. 7 I had a
question about.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 2/2] vfio-ccw: support for halt/clear subchannel

[Cornelia Huck]

On Thu, 7 Jun 2018 18:37:16 +0200
Pierre Morel  wrote:

> On 07/06/2018 11:54, Cornelia Huck wrote:

> > --
> > |   scsw(g)  |  ssch
> > --   |
> >   |   guest
> > --
> >   |qemu
> > --   v
> > |   scsw(q)  | emulate
> > --   |
> >   |
> > --   v
> > |   scsw(r)  | pwrite()
> > --   |
> >   |
> > --
> >   |vfio
> >   v
> >  ssch
> >   |
> > --
> >   |hardware
> > --   v
> > |   scsw(h)  | actually do something
> > --
> >
> > The guest issues a ssch (which gets intercepted; it won't get control
> > back until ssch finishes with a cc set.) scsw(g) won't change, unless
> > the guest does a stsch for the subchannel on another vcpu, in which
> > case it will get whatever information qemu holds in scsw(q) at that
> > point in time.
> >
> > When qemu starts to emulate the guest's ssch, it will set the start
> > function bit in the fctl field of scsw(q). It then copies scsw(q) to
> > scsw(r) in the vfio region.
> >
> > The vfio code will then proceed to call ssch on the real subchannel.
> > This is the first time we get really asynchronous, as the ssch will
> > return with cc set and the start function will be performed at some
> > point in time. If we would do a stsch on the real subchannel, we would
> > see that scsw(h) now has the start function bit set.
> >
> > Currently, we won't return back up the chain until we get an interrupt
> > from the hardware, at which time we update the scsw(r) from the irb.  
> 
> I do not find where the thread waits for interrupt inside the
> write->fsm_io_request->fsm_io_helper->ssch chain.
> 
> I must have miss it ten times. Can you point it to me?

I'm not surprised you did not find it, because it isn't there :)

I've let myself be confused by the /* Start channel program and wait
for I/O interrupt. */ comment -- it is wrong and should be removed (but
we did have that waiting initially). So we already have the async
situation :)

> 
> > This will propagate into the scsw(q). At the time we finish handling
> > the guest's ssch and return control to it, we're all done and if the
> > guest does a stsch to update its scsw(g), it will get the current
> > scsw(q) which will already contain the scsw from the interrupt's irb
> > (indicating that the start function is already finished).
> >
> > Now let's imagine we have a future implementation that handles actually
> > performing the start on the hardware asynchronously, i.e. it returns
> > control to the guest without the interrupt having been posted (let's
> > say that it is a longer-running I/O request). If the guest now did a
> > stsch to update scsw(g), it would get the current state of scsw(q),
> > which would be "start function set, but not done yet".
> >
> > If the guest now does a hsch, it would trap in the same way as the ssch
> > before. When qemu gets control, it adds the halt bit in scsw(q) (which
> > is in accordance with the architecture).  
> 
> This I agree.
> The scsw(q) is part of the QEMU device.
> 
> >   My proposal is to do the same
> > copying to scsw(r) again, which would mean we get a request with both
> > the halt and the start bit set. The vfio code now needs to do a hsch
> > (instead of a ssch). The real channel subsystem should figure this out,
> > as we can't reliably check whether the start function has concluded
> > already (there's always a race window).  
> 
> This I do not agree scsw(r) is part of the driver.
> The interface here is not a device interface anymore but a driver 
> interface.
> SCSW is a status, it is at its place in QEMU device interface with the 
> guest
> but here pwrite() sends a command.

Hm, I rather consider that "we write a status, and the backend figures
out what to do based on that status".

> 
> 
> Since we do not do a STSCH on each command, scsw(q), should be
> updated by QEMU depending on syscall return value.
> This is not done currently, only the success path is right
> because it is set before the call.
> 
> If I read right and the IRQ is asynchronous, the scsw(q) is not right,
> because not updated, after the interrupt is received from the guest.

Yes, we're probably missing some updates.

Re: [Qemu-devel] [PATCH v4 04/14] pc: prepare for multi stage hotplug handlers

[Igor Mammedov]

On Thu, 7 Jun 2018 16:00:54 +0200
David Hildenbrand  wrote:

> On 07.06.2018 15:44, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 13:27:01 +0200
> > David Hildenbrand  wrote:
> >   
> >> On 31.05.2018 16:13, Igor Mammedov wrote:  
> >>> On Wed, 30 May 2018 16:13:32 +0200
> >>> David Hildenbrand  wrote:
> >>> 
>  On 30.05.2018 15:08, Igor Mammedov wrote:
> > On Thu, 17 May 2018 10:15:17 +0200
> > David Hildenbrand  wrote:
> >   
> >> For multi stage hotplug handlers, we'll have to do some error handling
> >> in some hotplug functions, so let's use a local error variable (except
> >> for unplug requests).  
> > I'd split out introducing local error into separate patch
> > so patch would do a single thing.
> >>
> >> I can do that if it makes review easier.
> >>  
> >   
> >> Also, add code to pass control to the final stage hotplug handler at 
> >> the
> >> parent bus.  
> > But I don't agree with generic
> >  "} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
> > forwarding, it's done by 3/14 for generic case and in case of
> > special device that needs bus handler called from machine one,
> > I'd suggest to do forwarding explicitly for that device only
> > like we do with acpi_dev.  
> 
>  I decided to do it that way because it is generic and results in nicer
>  recovery handling (e.g. in case pc_dimm plug fails, we can simply
>  rollback all (for now MemoryDevice) previous plug operations).
> >>> rollback should be managed by the caller of pc_dimm plug
> >>> directly, so it's not relevant here.
> >>> 
>  IMHO, the resulting code is easier to read.
> 
>  From this handling it is clear that
>  "if we reach the hotplug handler, and it is not some special device
>  plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug
>  handler if any exists"
> >>> I strongly disagree with that it's easier to deal with.
> >>> You are basically duplicating already generalized code
> >>> from qdev_get_hotplug_handler() back into boards.
> >>>
> >>> If a device doesn't have to be handled by machine handler,
> >>> than qdev_get_hotplug_handler() must return its bus handler
> >>> if any directly. So branch in question that your are copying
> >>> is a dead one, pls drop it.
> >>
> >> We forward selected (pc_get_hotpug_handler()) devices to the
> >> right hotplug handler. Nothing wrong about that. I don't agree
> >> with "basically duplicating already generalized code" wrong.
> >> We have to forward at some place. Your idea simply places that
> >> code at some other place.
> >>
> >>
> >> But I think we have to get the general idea sorted out first.
> >>
> >> What you have in mind (e.g. plug):
> >>
> >> if (TYPE_MEMORY_DEVICE) {
> >>memory_device_plug();
> >>if (local_err) {
> >>goto out;
> >>}
> >>
> >>if (TYPE_PC_DIMM) {
> >>pc_dimm_plug(hotplug_dev, dev, &local_err);
> >>} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >>hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, 
> >> &local_err);
> >>}
> >>if (local_err) {
> >>memory_device_unplug()
> >>goto out;
> >>}
> >> } else if (TYPE_CPU)
> >> ...
> >>
> >>
> >> What I have in mind (and implemented in this series):
> >>
> >>
> >> if (TYPE_MEMORY_DEVICE) {
> >>memory_device_plug();
> >> }
> >> /* possibly other interfaces */
> >> if (local_err) {
> >>error_handling();
> >>return;
> >> }
> >>
> >> if (TYPE_PC_DIMM) {
> >>pc_dimm_plug(hotplug_dev, dev, &local_err);
> >> } ...
> >> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >>hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
> >> }  
> > I don't like this implicit wiring, where reader of this part of code
> > has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs
> > request forwarding.
> > I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code,
> > something like this:
> > 
> > if (TYPE_PC_DIMM) {
> > pc_dimm_plug()
> > /* do here additional concrete machine specific things */
> > } else if (TYPE_VIRTIO_MEM) {
> > virtio_mem_plug() <- do forwarding in there
> > /* and do here additional concrete machine specific things */
> > } else if (TYPE_CPU) {
> > cpu_plug()
> > /* do here additional concrete machine specific things */
> > }  
> 
> That will result in a lot of duplicate code - for every machine we
> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) -
> virtio-mem and virtio-pmem could most probably share the code.
maybe or maybe not, depending on if pmem endups as memory device or
separate controller. And even with duplication, machine code would
be easy to follow just down one explicit call chain.

> 
> >   
> >> if (local_err) {
> >>if (object_dynamic_cast(OBJECT(dev),

[Qemu-devel] [PATCH v2 00/10] python: futurize --stage1 (Python 3 compatibility)

[Eduardo Habkost]

v2 note (v1 was RFC):

  Except for a trivial change in patch 02/10, this series is the
  same as the RFC I sent previously.  I plan to merge it and
  include it in a pull request soon.

>From the futurize[1] documentation:

> This applies fixes that modernize Python 2 code without
> changing the effect of the code.  With luck, this will not
> introduce any bugs into the code, or will at least be trivial
> to fix. The changes are those that bring the Python code
> up-to-date without breaking Py2 compatibility.  The resulting
> code will be modern Python 2.6-compatible code plus __future__
> imports from the following set:
>
> from __future__ import absolute_import
> from __future__ import division
> from __future__ import print_function
>
[...]
> The goal for this stage is to create most of the diff for the
> entire porting process, but without introducing any bugs.  It
> should be uncontroversial and safe to apply to every Python 2
> package.  The subsequent patches introducing Python 3
> compatibility should then be shorter and easier to review.

This series run all the fixers from futurize --stage1 on all
Python code in the tree.  To make review and testing easier, I
have run the fixers separately instead of doing all changes in a
single patch.

[1] http://python-future.org/automatic_conversion.html

Eduardo Habkost (10):
  python: futurize -f libfuturize.fixes.fix_print_with_import
  python: futurize -f libfuturize.fixes.fix_absolute_import
  python: futurize -f libfuturize.fixes.fix_next_call
  python: futurize -f lib2to3.fixes.fix_has_key
  python: futurize -f lib2to3.fixes.fix_standarderror
  python: futurize -f lib2to3.fixes.fix_reduce
  python: futurize -f lib2to3.fixes.fix_tuple_params
  python: futurize -f lib2to3.fixes.fix_renames
  python: futurize -f lib2to3.fixes.fix_except
  python: futurize -f lib2to3.fixes.fix_numliterals

 scripts/analyse-9p-simpletrace.py| 89 
 scripts/analyse-locks-simpletrace.py |  3 +-
 scripts/analyze-migration.py | 11 +--
 scripts/dump-guest-memory.py |  1 +
 scripts/ordereddict.py   |  4 +-
 scripts/replay-dump.py   | 21 +++---
 scripts/signrom.py   |  1 +
 scripts/simpletrace.py   |  5 +-
 scripts/vmstate-static-checker.py| 89 
 scripts/device-crash-test|  3 +-
 scripts/kvm/kvm_flightrecorder   | 21 +++---
 scripts/kvm/vmxcap   |  1 +
 scripts/qmp/qemu-ga-client   | 10 +--
 scripts/qmp/qmp  | 24 ---
 scripts/qmp/qmp-shell| 40 ++-
 scripts/qmp/qom-fuse | 11 +--
 scripts/qmp/qom-get  | 12 ++--
 scripts/qmp/qom-list | 16 +++--
 scripts/qmp/qom-set  | 10 +--
 scripts/qmp/qom-tree | 16 +++--
 tests/docker/docker.py   | 11 +--
 tests/docker/travis.py   | 15 ++--
 tests/guest-debug/test-gdbstub.py|  1 +
 tests/image-fuzzer/qcow2/__init__.py |  3 +-
 tests/image-fuzzer/qcow2/fuzz.py |  1 +
 tests/image-fuzzer/qcow2/layout.py   |  3 +-
 tests/image-fuzzer/runner.py | 42 +--
 tests/migration/guestperf/engine.py  | 29 
 tests/migration/guestperf/plot.py| 17 ++---
 tests/migration/guestperf/shell.py   | 19 ++---
 tests/qemu-iotests/093   |  2 +-
 tests/qemu-iotests/096   |  4 +-
 tests/qemu-iotests/118   | 24 +++
 tests/qemu-iotests/136   |  2 +-
 tests/qemu-iotests/149   |  3 +-
 tests/qemu-iotests/165   |  3 +-
 tests/qemu-iotests/iotests.py|  5 +-
 tests/qemu-iotests/nbd-fault-injector.py |  7 +-
 tests/qemu-iotests/qcow2.py  | 39 ++-
 tests/qemu-iotests/qed.py| 17 ++---
 tests/vm/basevm.py   |  3 +-
 41 files changed, 337 insertions(+), 301 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140