Re: [Qemu-devel] [SeaBIOS] [RFC v2 0/3] Support multiple pci domains in pci_device

[Gerd Hoffmann]

On Tue, Aug 28, 2018 at 04:12:17AM +, Zihan Yang wrote:
> Gerd Hoffmann  于2018年8月27日周一 上午7:04写道：
> >
> >   Hi,
> >
> > > >   However, QEMU only binds port 0xcf8 and 0xcfc to
> > > > bus pcie.0. To avoid bus confliction, we should use other port pairs for
> > > > busses under new domains.
> > >
> > > I would skip support for IO based configuration and use only MMCONFIG
> > > for extra root buses.
> > >
> > > The question remains: how do we assign MMCONFIG space for
> > > each PCI domain.
> >
> > Allocation-wise it would be easiest to place them above 4G.  Right after
> > memory, or after etc/reserved-memory-end (if that fw_cfg file is
> > present), where the 64bit pci bars would have been placed.  Move the pci
> > bars up in address space to make room.
> >
> > Only problem is that seabios wouldn't be able to access mmconfig then.
> >
> > Placing them below 4G would work at least for a few pci domains.  q35
> > mmconfig bar is placed at 0xb000 -> 0xbfff, basically for
> > historical reasons.  Old qemu versions had 2.75G low memory on q35 (up
> > to 0xafff), and I think old machine types still have that for live
> > migration compatibility reasons.  Modern qemu uses 2G only, to make
> > gigabyte alignment work.
> >
> > 32bit pci bars are placed above 0xc000.  The address space from 2G
> > to 2.75G (0x800 -> 0xafff) is unused on new machine types.
> > Enough room for three additional mmconfig bars (full size), so four
> > pci domains total if you add the q35 one.
> 
> Maybe we can support 4 domains first before we come up
> with a better solution. But I'm not sure if four domains are
> enough for those who want too many devices?

There is the option to use ovmf instead.  Runs in long mode, so mapping
and using mmconfig above 4G isn't a problem for ovmf.  Given that
alternative exists I think it is reasonable to declare only 4 pci
domains being fully supported with seabios.

We can even configure mmconfig (above 4G) for the 5th and above, seabios
just would not enumerate the devices and map the pci bars for these
domains and leave it to the guest os instead.

cheers,
  Gerd

Re: [Qemu-devel] [SeaBIOS] [RFC v2 0/3] Support multiple pci domains in pci_device

[Marcel Apfelbaum]

Hi Gerd

On 08/28/2018 07:12 AM, Zihan Yang wrote:

Gerd Hoffmann  于2018年8月27日周一 上午7:04写道：

   Hi,


   However, QEMU only binds port 0xcf8 and 0xcfc to
bus pcie.0. To avoid bus confliction, we should use other port pairs for
busses under new domains.

I would skip support for IO based configuration and use only MMCONFIG
for extra root buses.

The question remains: how do we assign MMCONFIG space for
each PCI domain.


Thanks for your comments!


Allocation-wise it would be easiest to place them above 4G.  Right after
memory, or after etc/reserved-memory-end (if that fw_cfg file is
present), where the 64bit pci bars would have been placed.  Move the pci
bars up in address space to make room.

Only problem is that seabios wouldn't be able to access mmconfig then.

Placing them below 4G would work at least for a few pci domains.  q35
mmconfig bar is placed at 0xb000 -> 0xbfff, basically for
historical reasons.  Old qemu versions had 2.75G low memory on q35 (up
to 0xafff), and I think old machine types still have that for live
migration compatibility reasons.  Modern qemu uses 2G only, to make
gigabyte alignment work.

32bit pci bars are placed above 0xc000.  The address space from 2G
to 2.75G (0x800 -> 0xafff) is unused on new machine types.
Enough room for three additional mmconfig bars (full size), so four
pci domains total if you add the q35 one.

Maybe we can support 4 domains first before we come up
with a better solution. But I'm not sure if four domains are
enough for those who want too many devices?


(Adding Michael)

Since we will not use all 256 buses of an extra PCI domain,
I think this space will allow us to support more PCI domains.

How will the flow look like ?

1. QEMU passes to SeaBIOS information of how many extra
   PCI domains needs, and how many buses per domain.
   How it will pass this info? A vendor specific capability,
   some PCI registers or modifying extra-pci-roots fw_cfg file?

2. SeaBIOS assigns the address for each PCI Domain and
    returns the information to QEMU.
    How it will do that? Some pxb-pcie registers? Or do we model
    the MMCFG like a PCI BAR?

3. Once QEMU gets the MMCFG addresses, it can answer to
    mmio configuration cycles.

4. SeaBIOS queries all PCI domains devices, computes
   and assigns IO/MEM resources (for PCI domains > 0 it will
   use MMCFG to configure the PCI devices)

5. QEMU uses the IO/MEM information to create the CRS for each
    extra PCI host bridge.

6. SeaBIOS gets the ACPI tables from QEMU and passes them to the
   guest OS.

Thanks,
Marcel






cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/6] json: Nicer recovery from lexical errors

[Markus Armbruster]

Eric Blake  writes:

> On 08/27/2018 02:00 AM, Markus Armbruster wrote:
>> When the lexer chokes on an input character, it consumes the
>> character, emits a JSON error token, and enters its start state.  This
>> can lead to suboptimal error recovery.  For instance, input
>>
>>  0123 ,
>>
>> produces the tokens
>>
>>  JSON_ERROR01
>>  JSON_INTEGER  23
>>  JSON_COMMA,
>>
>> Make the lexer skip characters after a lexical error until a
>> structural character ('[', ']', '{', '}', ':', ','), an ASCII control
>> character, or '\xFE', or '\xFF'.
>>
>> Note that we must not skip ASCII control characters, '\xFE', '\xFF',
>> because those are documented to force the JSON parser into known-good
>> state, by docs/interop/qmp-spec.txt.
>>
>> The lexer now produces
>>
>>  JSON_ERROR01
>>  JSON_COMMA,
>
> So the lexer has now completely skipped the intermediate input, but
> the resulting error message need only point at the start of where
> input went wrong, and skipping to a sane point results in fewer error
> tokens to be reported.  Makes sense.

Exactly.

We could emit a recovery token to let json_message_process_token()
report what we skipped, but I don't think it's worth the trouble.

>> Update qmp-test for the nicer error recovery: QMP now report just one
>
> s/report/reports/

Will fix.

>> error for input %p instead of two.  Also drop the newline after %p; it
>> was needed to tease out the second error.
>
> That's because pre-patch, 'p' is one of the input characters that
> requires lookahead to determine if it forms a complete token (and the
> newline provides the transition needed to consume it); now post-patch,
> the 'p' is consumed as part of the junk after the error is first
> detected at the '%'.

Exactly.

> And to my earlier complaint about 0x1a resulting in JSON_ERROR then
> JSON_INTEGER then JSON_KEYWORD, that sequence is likewise now
> identified as a single JSON_ERROR at the 'x', with the rest of the
> attempted hex number (invalid in JSON) silently skipped.  Nice.

Correct.

>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>   qobject/json-lexer.c | 43 +--
>>   tests/qmp-test.c |  6 +-
>>   2 files changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
>> index 28582e17d9..39c7ce7adc 100644
>> --- a/qobject/json-lexer.c
>> +++ b/qobject/json-lexer.c
>> @@ -101,6 +101,7 @@
>> enum json_lexer_state {
>>   IN_ERROR = 0,   /* must really be 0, see json_lexer[] */
>> +IN_RECOVERY,
>>   IN_DQ_STRING_ESCAPE,
>>   IN_DQ_STRING,
>>   IN_SQ_STRING_ESCAPE,
>> @@ -130,6 +131,28 @@ QEMU_BUILD_BUG_ON(IN_START_INTERP != IN_START + 1);
>>   static const uint8_t json_lexer[][256] =  {
>>   /* Relies on default initialization to IN_ERROR! */
>>   +/* error recovery */
>> +[IN_RECOVERY] = {
>> +/*
>> + * Skip characters until a structural character, an ASCII
>> + * control character other than '\t', or impossible UTF-8
>> + * bytes '\xFE', '\xFF'.  Structural characters and line
>> + * endings are promising resynchronization points.  Clients
>> + * may use the others to force the JSON parser into known-good
>> + * state; see docs/interop/qmp-spec.txt.
>> + */
>> +[0 ... 0x1F] = IN_START | LOOKAHEAD,
>
> And here's where the LOOKAHEAD bit has to be separate, because you are
> now assigning semantics to the transition on '\0' that are distinct
> from either of the two roles previously enumerated as possible.

I could do

TERMINAL(IN_START)
[0x20 ... 0xFD] = IN_RECOVERY,
['\t'] = IN_RECOVERY,

but it would be awful :)

>> +[0x20 ... 0xFD] = IN_RECOVERY,
>> +[0xFE ... 0xFF] = IN_START | LOOKAHEAD,
>> +['\t'] = IN_RECOVERY,
>> +['['] = IN_START | LOOKAHEAD,
>> +[']'] = IN_START | LOOKAHEAD,
>> +['{'] = IN_START | LOOKAHEAD,
>> +['}'] = IN_START | LOOKAHEAD,
>> +[':'] = IN_START | LOOKAHEAD,
>> +[','] = IN_START | LOOKAHEAD,
>> +},
>
> It took me a couple of reads before I was satisfied that everything is
> initialized as desired (range assignments followed by more-specific
> re-assignment works, but isn't common), but this looks right.
>
> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH 5/6] json: Eliminate lexer state IN_ERROR

[Markus Armbruster]

Eric Blake  writes:

> On 08/27/2018 02:00 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>   qobject/json-lexer.c  | 9 +
>>   qobject/json-parser-int.h | 8 
>>   2 files changed, 9 insertions(+), 8 deletions(-)
>
>> -
>>   typedef enum json_token_type {
>> -JSON_MIN = 100,
>> -JSON_LCURLY = JSON_MIN,
>> +JSON_ERROR = 0, /* must be zero, see json_lexer[] */
>> +/* Gap for lexer states */
>> +JSON_LCURLY = 100,
>> +JSON_MIN = JSON_LCURLY,
>
> In an earlier version of this type of cleanup, you swapped the IN_ and
> JSON_ values and eliminated the gap, to make the overall table more
> compact (no storage wasted on any of the states in the gap between the
> two).
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg01178.html
>
> Is it still worth trying to minimize the gap between the two
> sequences, even if you now no longer swap them in order?

You caught me :)

Eliminating the gap actually enlarges the table.  I first got confused,
then measured the size change backwards to confirm my confused ideas.
When I looked at the patch again, I realized my mistake, and silently
dropped this part of the change.

Re: [Qemu-devel] [PATCH 6/6] json: Eliminate lexer state IN_WHITESPACE, pseudo-token JSON_SKIP

[Markus Armbruster]

Eric Blake  writes:

> On 08/27/2018 02:00 AM, Markus Armbruster wrote:
>> The lexer ignores whitespace like this:
>>
>>   on whitespace  on non-ws   spontaneously
>>  IN_START --> IN_WHITESPACE --> JSON_SKIP --> IN_START
>>  ^|
>>   \__/  on whitespace
>>
>> This accumulates a whitespace token in state IN_WHITESPACE, only to
>> throw it away on the transition via JSON_SKIP to the start state.
>> Wasteful.  Go from IN_START to IN_START on whitspace directly,
>
> s/whitspace/whitespace/

Will fix.

>> dropping the whitespace character.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>   qobject/json-lexer.c  | 22 +-
>>   qobject/json-parser-int.h |  1 -
>>   2 files changed, 5 insertions(+), 18 deletions(-)
>>
>> @@ -263,10 +253,10 @@ static const uint8_t json_lexer[][256] =  {
>>   [','] = JSON_COMMA,
>>   [':'] = JSON_COLON,
>>   ['a' ... 'z'] = IN_KEYWORD,
>> -[' '] = IN_WHITESPACE,
>> -['\t'] = IN_WHITESPACE,
>> -['\r'] = IN_WHITESPACE,
>> -['\n'] = IN_WHITESPACE,
>> +[' '] = IN_START,
>> +['\t'] = IN_START,
>> +['\r'] = IN_START,
>> +['\n'] = IN_START,
>>   },
>>   [IN_START_INTERP]['%'] = IN_INTERP,
>
> Don't you need to set [IN_START_INTERP][' '] to IN_START_INTERP,
> rather than IN_START?  Otherwise, the presence of skipped whitespace
> would change whether interpolation happens.  (At least, that's what
> you had in an earlier version of this patch).
>
>>   };
>> @@ -323,10 +313,8 @@ static void json_lexer_feed_char(JSONLexer *lexer, char 
>> ch, bool flush)
>>   json_message_process_token(lexer, lexer->token, new_state,
>>  lexer->x, lexer->y);
>>   /* fall through */
>> -case JSON_SKIP:
>> -g_string_truncate(lexer->token, 0);
>> -/* fall through */
>>   case IN_START:
>> +g_string_truncate(lexer->token, 0);
>>   new_state = lexer->start_state;
>
> Oh, I see. We are magically reverting to the correct start state if
> the requested transition reports IN_START, rather than blindly using
> IN_START.

Correct.  Less repetitive this way.

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH 2/6] json: Clean up how lexer consumes "end of input"

[Markus Armbruster]

Eric Blake  writes:

> On 08/27/2018 02:00 AM, Markus Armbruster wrote:
>> When the lexer isn't in its start state at the end of input, it's
>> working on a token.  To flush it out, it needs to transit to its start
>> state on "end of input" lookahead.
>>
>> There are two ways to the start state, depending on the current state:
>>
>> * If the lexer is in a TERMINAL(JSON_FOO) state, it can emit a
>>JSON_FOO token.
>>
>> * Else, it can go to IN_ERROR state, and emit a JSON_ERROR token.
>>
>> There are complications, however:
>>
>> * The transition to IN_ERROR state consumes the input character and
>>adds it to the JSON_ERROR token.  The latter is inappropriate for
>>the "end of input" character, so we suppress that.  See also recent
>>commit "json: Fix lexer to include the bad character in JSON_ERROR
>>token".
>
> Now commit a2ec6be7

I'll update the commit message.

>>
>> * The transition to a TERMINAL(JSON_FOO) state doesn't consume the
>>input character.  In that case, the lexer normally loops until it is
>>consumed.  We have to suppress that for the "end of input" input
>>character.  If we didn't, the lexer would consume it by entering
>>IN_ERROR state, emitting a bogus JSON_ERROR token.  We fixed that in
>>commit bd3924a33a6.
>>
>> However, simply breaking the loop this way assumes that the lexer
>> needs exactly one state transition to reach its start state.  That
>> assumption is correct now, but it's unclean, and I'll soon break it.
>> Clean up: instead of breaking the loop after one iteration, break it
>> after it reached the start state.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>   qobject/json-lexer.c | 17 +
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
>> index 4867839f66..ec3aec726f 100644
>> --- a/qobject/json-lexer.c
>> +++ b/qobject/json-lexer.c
>> @@ -261,7 +261,8 @@ void json_lexer_init(JSONLexer *lexer, bool 
>> enable_interpolation)
>> static void json_lexer_feed_char(JSONLexer *lexer, char ch, bool
>> flush)
>>   {
>> -int char_consumed, new_state;
>> +int new_state;
>> +bool char_consumed = false;
>
> Yay for the switch to bool.
>
> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH 1/6] json: Fix lexer for lookahead character beyond '\x7F'

[Markus Armbruster]

Eric Blake  writes:

> On 08/27/2018 02:00 AM, Markus Armbruster wrote:
>> The lexer fails to end a valid token when the lookahead character is
>> beyond '\x7F'.  For instance, input
>>
>>  true\xC2\xA2
>>
>> produces the tokens
>>
>>  JSON_ERROR true\xC2
>>  JSON_ERROR \xA2
>>
>> The first token should be
>>
>>  JSON_KEYWORD   true
>>
>> instead.
>
> As long as we still get a JSON_ERROR in the end.

We do: one for \xC2, and one for \xA2.  PATCH 4 will lose the second one.

>> The culprit is
>>
>>  #define TERMINAL(state) [0 ... 0x7F] = (state)
>>
>> It leaves [0x80..0xFF] zero, i.e. IN_ERROR.  Has always been broken.
>
> I wonder if that was done because it was assuming that valid input is
> only ASCII, and that any byte larger than 0x7f is invalid except
> within the context of a string.

Plausible thinko.

>  But whatever the reason for the
> original bug, your fix makes sense.
>
>> Fix it to initialize the complete array.
>
> Worth testsuite coverage?

Since lookahead bytes > 0x7F are always a parse error, all the bug can
do is swallow a TERMINAL() token right before a parse error.  The
TERMINAL() tokens are JSON_INTEGER, JSON_FLOAT, JSON_KEYWORD, JSON_SKIP,
JSON_INTERP.  Fairly harmless.  In particular, JSON objects get through
even when followed by a byte > 0x7F.

Of course, test coverage wouldn't hurt regardless.

>> Signed-off-by: Markus Armbruster 
>> ---
>>   qobject/json-lexer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>
> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [SeaBIOS] [RFC v2 0/3] Support multiple pci domains in pci_device

[Zihan Yang]

Gerd Hoffmann  于2018年8月27日周一 上午7:04写道：
>
>   Hi,
>
> > >   However, QEMU only binds port 0xcf8 and 0xcfc to
> > > bus pcie.0. To avoid bus confliction, we should use other port pairs for
> > > busses under new domains.
> >
> > I would skip support for IO based configuration and use only MMCONFIG
> > for extra root buses.
> >
> > The question remains: how do we assign MMCONFIG space for
> > each PCI domain.
>
> Allocation-wise it would be easiest to place them above 4G.  Right after
> memory, or after etc/reserved-memory-end (if that fw_cfg file is
> present), where the 64bit pci bars would have been placed.  Move the pci
> bars up in address space to make room.
>
> Only problem is that seabios wouldn't be able to access mmconfig then.
>
> Placing them below 4G would work at least for a few pci domains.  q35
> mmconfig bar is placed at 0xb000 -> 0xbfff, basically for
> historical reasons.  Old qemu versions had 2.75G low memory on q35 (up
> to 0xafff), and I think old machine types still have that for live
> migration compatibility reasons.  Modern qemu uses 2G only, to make
> gigabyte alignment work.
>
> 32bit pci bars are placed above 0xc000.  The address space from 2G
> to 2.75G (0x800 -> 0xafff) is unused on new machine types.
> Enough room for three additional mmconfig bars (full size), so four
> pci domains total if you add the q35 one.

Maybe we can support 4 domains first before we come up
with a better solution. But I'm not sure if four domains are
enough for those who want too many devices?

> cheers,
>   Gerd
>

Re: [Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Peter Xu]

On Tue, Aug 28, 2018 at 12:23:19AM +0200, Marc-André Lureau wrote:
> terminal3270 is uses the front-end side of the chardev. It shouldn't
> create sources from backend side context. Fwiw, send_timing_mark_cb
> calls qemu_chr_fe_write_all() which should be thread safe.
> 
> This partially reverts changes from commit
> 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.

I don't fully understand too on why "It shouldn't create sources from
backend side context".  Could you explain a bit?

If you don't want the backend gcontext to be exposed to the frontend,
IMHO the simplest thing is just to use the NULL gcontext in
qemu_chr_timeout_add_ms() which is a one-liner change. I don't see
much point on re-using the GSource tags since IMHO GSource pointer is
always superior to the old tag system.

> 
> CC: Peter Xu 
> Signed-off-by: Marc-André Lureau 
> ---
>  hw/char/terminal3270.c | 15 ++-
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index e9c45e55b1..35b079d5c4 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -31,7 +31,7 @@ typedef struct Terminal3270 {
>  uint8_t outv[OUTPUT_BUFFER_SIZE];
>  int in_len;
>  bool handshake_done;
> -GSource *timer_src;
> +guint timer_tag;
>  } Terminal3270;
>  
>  #define TYPE_TERMINAL_3270 "x-terminal3270"
> @@ -47,10 +47,9 @@ static int terminal_can_read(void *opaque)
>  
>  static void terminal_timer_cancel(Terminal3270 *t)
>  {
> -if (t->timer_src) {
> -g_source_destroy(t->timer_src);
> -g_source_unref(t->timer_src);
> -t->timer_src = NULL;
> +if (t->timer_tag) {
> +g_source_remove(t->timer_tag);
> +t->timer_tag = 0;
>  }
>  }
>  
> @@ -100,8 +99,7 @@ static void terminal_read(void *opaque, const uint8_t 
> *buf, int size)
>  assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
>  
>  terminal_timer_cancel(t);
> -t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
> -   send_timing_mark_cb, t);
> +t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
>  memcpy(>inv[t->in_len], buf, size);
>  t->in_len += size;
>  if (t->in_len < 2) {
> @@ -160,8 +158,7 @@ static void chr_event(void *opaque, int event)
>   * char-socket.c. Once qemu receives the terminal-type of the
>   * client, mark handshake done and trigger everything rolling again.
>   */
> -t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
> -   send_timing_mark_cb, t);
> +t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
>  break;
>  case CHR_EVENT_CLOSED:
>  sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
> -- 
> 2.18.0.547.g1d89318c48
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] editorconfig: set emacs mode

[Markus Armbruster]

Marc-André Lureau  writes:

> Some time ago, I proposed to use an (eval) in .dir-locals.el to set
> the mode for all json files and Makefile. Unfortunately, this isn't
> safe, and emacs will prompt the user, which isn't very friendly.
>
> Fortunately, editorconfig provides a special config key which does
> allow to set the emacs mode. Add a few missing entries and set the
> emacs mode.
>
> Update top comment to provide a short summary about the file and the
> IDE plugins while at it.
>
> Signed-off-by: Marc-André Lureau 

Acked-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event

[Peter Xu]

On Mon, Aug 27, 2018 at 02:30:12PM -0500, Eric Blake wrote:
> On 08/15/2018 08:37 AM, Peter Xu wrote:
> > Now it was not used any more.  Drop it, especially if we can do that
> > before we release QEMU 3.0.
> 
> Stale commit message, now that 3.0 has been released. But still doable,
> since the event could not be emitted without use of the experimental x-oob
> option.

Fixed:

Now it was not used any more, drop it.  We can still do that since
out-of-band is still experimental, and this event is only used when
out-of-band is enabled.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission

[Peter Xu]

On Mon, Aug 27, 2018 at 02:14:25PM +0200, Markus Armbruster wrote:

[...]

> Let's improve the commit message a bit.  Here's my try:
> 
> qapi: Drop qapi_event_send_FOO()'s Error ** argument
> 
> The generated qapi_event_send_FOO() take an Error ** argument.  They
> can't actually fail, because all they do with the argument is passing it
> to functions that can't fail: the QObject output visitor, and the
> @qmp_emit callback, which is either monitor_qapi_event_queue() or
> event_test_emit().
> 
> Drop the argument, and pass _abort to the QObject output visitor
> and @qmp_emit instead.

I'm stealing this into my local tree.

> 
> With something like that:
> Reviewed-by: Markus Armbruster 

Thanks for the analysis and r-b; stealing the r-b too.

-- 
Peter Xu

Re: [Qemu-devel] [virtio-dev] Re: [PATCH v25 0/2] virtio-crypto: virtio crypto device specification

[Gonglei (Arei)]

> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, August 24, 2018 8:54 PM
> 
> On Fri, Aug 24, 2018 at 12:07:44PM +, Gonglei (Arei) wrote:
> > Hi Michael,
> >
> > > -Original Message-
> > > From: virtio-...@lists.oasis-open.org
> [mailto:virtio-...@lists.oasis-open.org]
> > > On Behalf Of Michael S. Tsirkin
> > > Sent: Friday, August 24, 2018 7:23 PM
> > > To: longpeng 
> > > Cc: xin.z...@intel.com; Gonglei (Arei) ;
> > > pa...@linux.vnet.ibm.com; qemu-devel@nongnu.org;
> > > virtio-...@lists.oasis-open.org; coh...@redhat.com;
> stefa...@redhat.com;
> > > denglin...@chinamobile.com; Jani Kokkonen
> ;
> > > ola.liljed...@arm.com; varun.se...@freescale.com;
> > > brian.a.keat...@intel.com; liang.j...@intel.com; john.grif...@intel.com;
> > > ag...@suse.de; jasow...@redhat.com; vincent.jar...@6wind.com;
> > > Huangweidong (C) ; wangxin (U)
> > > ; Zhoujian (jay)
> 
> > > Subject: [virtio-dev] Re: [PATCH v25 0/2] virtio-crypto: virtio crypto 
> > > device
> > > specification
> > >
> > > Is there a github issue? If not pls create one.
> > >
> >
> > I just created one issue:
> >
> > https://github.com/oasis-tcs/virtio-spec/issues/19
> 
> All set to start voting whenever you request it.
> 

Hi Michael,

Since no comments currently, pls help to start a ballot for virtio crypto spec 
if you can. :)


Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh

[Peter Xu]

On Mon, Aug 27, 2018 at 01:29:29PM +0200, Markus Armbruster wrote:
> > @@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void 
> > *opaque)
> >  Monitor *mon = opaque;
> >  GMainContext *context;
> >  
> > -if (mon->use_io_thread) {
> > -/* Use @mon_iothread context */
> > -context = monitor_get_io_context();
> > -assert(context);
> > -} else {
> > -/* Use default main loop context */
> > -context = NULL;
> > -}
> > -
> > +assert(mon->use_io_thread);
> > +/* Use @mon_iothread context */
> 
> Mind if I drop this comment?

Yes, please.

> 
> > +context = monitor_get_io_context();
> > +assert(context);
> >  qemu_chr_fe_set_handlers(>chr, monitor_can_read, monitor_qmp_read,
> >   monitor_qmp_event, NULL, mon, context, true);
> >  monitor_list_append(mon);
> 
> R-by stands, of course.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v5 3/3] intel-iommu: replace more vtd_err_* traces

[Peter Xu]

On Mon, Aug 27, 2018 at 03:17:45PM +0200, Markus Armbruster wrote:
> Peter Xu  writes:
> 
> > Replace all the trace_vtd_err_*() hooks with the new error_report_once()
> > since they are similar to trace_vtd_err() - dumping the first error
> > would be mostly enough, then we have them on by default too.
> >
> > Signed-off-by: Peter Xu 
> > ---
> [...]
> 
> Let's use "%x" instead of "%" PRIx16 for simplicity, and add spaces
> around PRIx64 & friends.
> 
> Squashing in:

Thanks for touching up the places around in both patches.

-- 
Peter Xu

[Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary

[yuchenlin--- via Qemu-devel]

From: yuchenlin 

There is a rare case which the size of last compressed cluster
is larger than the cluster size, which will cause the file is
not aligned at the sector boundary.

Signed-off-by: yuchenlin 
---
 block/vmdk.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..a8ae7c65d2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,24 @@ static int coroutine_fn
 vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
 {
+if (bytes == 0) {
+/* align end of file to a sector boundary. */
+BDRVVmdkState *s = bs->opaque;
+int i, ret;
+int64_t length;
+
+for (i = 0; i < s->num_extents; i++) {
+length = bdrv_getlength(s->extents[i].file->bs);
+if (length < 0) {
+return length;
+}
+ret = bdrv_truncate(s->extents[i].file, length, PREALLOC_MODE_OFF, 
NULL);
+if (ret < 0) {
+return ret;
+}
+}
+return 0;
+}
 return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
 }
 
-- 
2.17.0

[Qemu-devel] [Bug 1755912] Re: qemu-system-x86_64 crashed with SIGABRT when using option -vga qxl

[Leonardo Müller]

Thank you for the effort to backport this bug fix, I have tested the
proposed version 2.11.1(Debian 1:2.11+dfsg-1ubuntu7.5) and the crash
when changing resolution is no longer happening. Unfortunately, after
some time (about 10 minutes, much longer than without this fix), the
guest freezes.

I reported that in https://bugs.launchpad.net/qemu/+bug/1787070

I'll change the tag to verification-done-bionic, as this particular
issue was corrected. The other issue still exists in QEMU upstream.

** Tags removed: verification-needed-bionic
** Tags added: verification-done-bionic

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

Title:
  qemu-system-x86_64 crashed with SIGABRT when using option -vga qxl

Status in QEMU:
  Fix Released
Status in qemu package in Ubuntu:
  Fix Released
Status in qemu source package in Bionic:
  Fix Committed

Bug description:
  [Impact]

   * There are conditions where the vga/qxl driver can crash the qemu 
 process.

   * It is like a very complex case of a non initialized var - without the 
 fix it might try to ask for updates without having a valid primary 
 surface.

   * Backport from upstream
  
https://git.qemu.org/?p=qemu.git;a=commit;h=5bd5c27c7d284d01477c5cc022ce22438c46bf9f
  to avoid the crash

  
  [Test Case]

   * Sometimes booting xubuntu was reported to be enough, at other times
  it was needed to change resolution a few times to trigger.

# get xubuntu iso (actually other UI Isos should do as well)
$ qemu-system-x86_64 -vga qxl -enable-kvm -cpu host -smp cores=2,threads=2 
-m 2048 -cdrom xubuntu-18.04-desktop-amd64.iso
# If it boots successfully, change resolution until it crashes.
$ while true ; do xrandr --output Virtual-0 --mode 640x480 ; sleep 1 ; 
xrandr --output Virtual-0 --mode 1280x720 ; sleep 1 ; xrandr --output Virtual-0 
--mode 1920x1080 ; sleep 1 ; done

   * Without the fix that will trigger the qemu crash

  [Regression Potential]

   * The change "just" adds QXL_MODE_UNDEFINED as one more trigger to leave 
 the rendering update. That sounds rather safe. But thinking hard on 
 potential updates I could think of theoretical setups that were  in 
 undefined mode all the time (unlikely or impossible I think) that now 
 would get no updates anymore. Well I really don't think this is an 
 issue, but since this section should be open thinking on "potential" 
 regressions that is what comes to my mind.

  [Other Info]
   
   * Thanks to Leonardo for most of the bisecting and discussion work!

  
  ---

  
  When using qemu-system-x86_64 with the option -vga qxl, it crashes. The 
easiest way to crash it is by trying to change the guest's resolution. However, 
the system may randomly crash too, not happening only when changing resolution. 
Here is the terminal output of one of these random crashes:

  

  $ qemu-system-x86_64 -hda /dev/sdb -m 2048 -enable-kvm -cpu host -vga qxl 
-nodefaults -netdev user,id=hostnet0 -device 
virtio-net-pci,id=net0,netdev=hostnet0
  WARNING: Image format was not specified for '/dev/sdb' and probing guessed 
raw.
   Automatically detecting the format is dangerous for raw images, 
write operations on block 0 will be restricted.
   Specify the 'raw' format explicitly to remove the restrictions.

  (process:21313): Spice-WARNING **: 16:01:45.759: display-
  channel.c:2431:display_channel_validate_surface: canvas address is
  0x7f8eb948ab18 for 0 (and is NULL)

  (process:21313): Spice-WARNING **: 16:01:45.759: display-
  channel.c:2432:display_channel_validate_surface: failed on 0

  (process:21313): Spice-CRITICAL **: 16:01:45.759: 
display-channel.c:2035:display_channel_update: condition 
`display_channel_validate_surface(display, surface_id)' failed
  Abortado (imagem do núcleo gravada)

  

  I was running QEMU as a normal user which is on the groups kvm and
  disk. Initially I supposed the problem was because I was running QEMU
  as root, but as a normal user this happens too.

  I have tested with guests with different Ubuntu version: 18.04, 17.10
  and 16.04. It is happening with them all.

  ProblemType: Crash
  DistroRelease: Ubuntu 18.04
  Package: qemu-system-x86 1:2.11+dfsg-1ubuntu4
  ProcVersionSignature: Ubuntu 4.15.0-10.11-generic 4.15.3
  Uname: Linux 4.15.0-10-generic x86_64
  ApportVersion: 2.20.8-0ubuntu10
  Architecture: amd64
  CurrentDesktop: XFCE
  Date: Wed Mar 14 17:13:52 2018
  ExecutablePath: /usr/bin/qemu-system-x86_64
  InstallationDate: Installed on 2017-06-13 (273 days ago)
  InstallationMedia: Xubuntu 17.04 "Zesty Zapus" - Release amd64 (20170412)
  KvmCmdLine: COMMAND STAT  EUID  RUID   PID  PPID %CPU COMMAND
  MachineType: LENOVO 80UG
  ProcCmdline: qemu-system-x86_64 -hda /dev/sdb -smp cpus=2 -m 512 -enable-kvm 
-cpu host -vga qxl -nodefaults -netdev user,id=hostnet0 -device 

Re: [Qemu-devel] [PATCH v2] vl.c deprecate incorrect CPUs topology

[Eduardo Habkost]

On Mon, Aug 27, 2018 at 03:53:26PM +0200, Igor Mammedov wrote:
> -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> so that total number of logical CPUs [sockets * cores * threads]
> would be equal to [maxcpus], however historically we didn't have
> such check in QEMU and it is possible to start VM with an invalid
> topology.
> Deprecate invalid options combination so we can make sure that
> the topology VM started with is always correct in the future.
> Users with an invalid sockets/cores/threads/maxcpus values should
> fix their CLI to make sure that
>[sockets * cores * threads] == [maxcpus]
> 
> Signed-off-by: Igor Mammedov 
> ---
> v2:
>   - spelling& fixes (Andrew Jones )
> ---
>  qemu-deprecated.texi | 11 +++
>  vl.c |  6 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 87212b6..b649b2e 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -159,6 +159,17 @@ The 'file' driver for drives is no longer appropriate 
> for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> +
> +CPU topology properties should describe whole machine topology including
> +possible CPUs. but historically it was possible to start QEMU with
> +an incorrect topology where
> +  sockets * cores * threads >= X && X < maxcpus
> +which could lead to incorrect topology enumeration by the guest.
> +Support for invalid topology will be removed, the user must ensure
> +topologies described with -smp includes all possible cpus, i.e.:
> +  sockets * cores * threads == maxcpus
> +
>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> diff --git a/vl.c b/vl.c
> index 5ba06ad..238f856 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1238,6 +1238,12 @@ static void smp_parse(QemuOpts *opts)
>  exit(1);
>  }
>  
> +if (sockets * cores * threads != max_cpus) {
> +warn_report("Invalid CPU topology deprecated: "
> +"sockets (%u) * cores (%u) * threads (%u) "
> +"!= maxcpus (%u)",
> +sockets, cores, threads, max_cpus);
> +}

We don't need to print the warning if
  sockets * cores * threads > max_cpus,
because it already triggers error_report()/exit() below.  I
suggest reordering the checks:

if (sockets * cores * threads > max_cpus) {
error_report(...);
exit(1);
}
if (sockets * cores * threads != max_cpus) {
warn_report(...)
}


>  if (sockets * cores * threads > max_cpus) {
>  error_report("cpu topology: "
>   "sockets (%u) * cores (%u) * threads (%u) > "
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] blkdebug: Add support for latency rules

[Marc Olson via Qemu-devel]

On 08/24/2018 09:11 AM, Eric Blake wrote:

On 08/24/2018 12:06 AM, Marc Olson via Qemu-devel wrote:

Allow rules to be created to inject latency into I/O operations.

Signed-off-by: Marc Olson 
---
  block/blkdebug.c    | 101 
++--

  docs/devel/blkdebug.txt |  30 ++
  2 files changed, 103 insertions(+), 28 deletions(-)


The commit message could usefully summarize some of the doc additions 
(so you can learn more about it without having to read the patch 
itself) - but the doc additions are appreciated.

I can be more verbose in the commit.


Missing: you should enhance an existing (or add a new) iotests usage 
of blkdebug to specify a latency operation. One possible test would be 
forcing a read to take longer than a write - then issue an aio read, a 
write immediately after, and verify that the write completes first and 
then the read (whether the read sees the old or the new data just 
written would then depend on whether the delay is acted on before or 
after blkdebug forwards the read on to the real block layer).



I'll add at least a simple test to the repo.



diff --git a/block/blkdebug.c b/block/blkdebug.c
index e216699..762e438 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -65,6 +65,7 @@ typedef struct BlkdebugSuspendedReq {
    enum {
  ACTION_INJECT_ERROR,
+    ACTION_DELAY,
  ACTION_SET_STATE,
  ACTION_SUSPEND,
  };


Does any of this need to be accessible via QMP?  (If QMP can't already 
fine-tune what blkdebug can do, I'm not proposing that you have to 
make it do so - but if QMP can specify error injections, it should 
also be able to specify delays).  Even if it doesn't need to be in 
QMP, should we specify this enum and related types in our qapi/*.json 
files?
It does appear that blkdebug is in the qapi/*.json files. I've added the 
appropriate types, but I'm less familiar with the QMP incantations, and 
seem to have misconfigured my tests as they result in an uninitialized 
mutex when trying to do aio*. Using the existing tests as a guide, 
there's no global BlockBackend created, and so hmp_qemu_io() creates a 
local backend that is cleaned up before the aio completes. Removing the 
blk_unref() clears it up, but of course leaks memory. I don't see an API 
that creates the appropriate QMP BlockBackend for file backed devices.





@@ -123,6 +127,33 @@ static QemuOptsList inject_error_opts = {
  },
  };
  +static QemuOptsList delay_opts = {


One reason for suggesting that this be done with QAPI types is that 
QemuOpts is already proving to be painfully hard to extend, where in 
general we are trying to move towards using QAPI types rather than yet 
more QemuOpts munging.


@@ -182,16 +214,21 @@ static int add_rule(void *opaque, QemuOpts 
*opts, Error **errp)

  .state  = qemu_opt_get_number(opts, "state", 0),
  };
  +    rule->once  = qemu_opt_get_bool(opts, "once", 0);
+    sector = qemu_opt_get_number(opts, "sector", -1);
+    rule->offset = sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;


I really wish we could spend time moving blkdebug to be byte-based 
instead of sector-based.
I'm not certain I see the benefit of moving to byte based. Storage 
devices are sector based. The biggest pain point with blkdebug is not 
that it's sector based, but that offsets are based on the backing file, 
and not as viewed by the guest. In order to properly use blkdebug with 
qcow2, for example, you have to do some gymnastics.



+++ b/docs/devel/blkdebug.txt
@@ -24,7 +24,7 @@ This way, all error paths can be tested to make 
sure they are correct.

  Rules
  -
  The blkdebug block driver takes a list of "rules" that tell the 
error injection

-engine when to fail an I/O request.
+engine when to fail (inject-error) or add latency to (delay) an I/O 
request.
    Each I/O request is evaluated against the rules.  If a rule 
matches the request

  then its "action" is executed.
@@ -33,17 +33,25 @@ Rules can be placed in a configuration file; the 
configuration file
  follows the same .ini-like format used by QEMU's -readconfig 
option, and

  each section of the file represents a rule.
  -The following configuration file defines a single rule:
+The following configuration file defines multiple rules:
      $ cat blkdebug.conf
    [inject-error]
    event = "read_aio"
    errno = "28"
  -This rule fails all aio read requests with ENOSPC (28).  Note that 
the errno

-value depends on the host.  On Linux, see
+  [delay]
+  event = "read_aio"
+  sector = "2048"
+  latency = "50"
+
+The error rule fails all aio read requests with ENOSPC (28). Note 
that the

+errno value depends on the host.  On Linux, see
  /usr/include/asm-generic/errno-base.h for errno values.
  +The delay rule adds 500 ms of latency to a read I/O request 
containing sector

+2048.


So in this example, you get a failure no matter what, but that one 
sector takes even longer to fail?
No, in the case of injection that overlaps, it 

Re: [Qemu-devel] [PATCH] pc: make sure that guest isn't able to unplug the first cpu

[David Gibson]

On Mon, Aug 27, 2018 at 04:02:39PM +0200, Greg Kurz wrote:
> On Mon, 27 Aug 2018 13:07:10 +0200
> Igor Mammedov  wrote:
> 
> > The first cpu unplug wasn't ever supported and corresponding
> > monitor/qmp commands refuse to unplug it. However guest is able
> > to issue eject request either using following command:
> >   # echo 1 >/sys/devices/system/cpu/cpu0/firmware_node/eject
> 
> I can't reproduce the issue with a pc guest and current master...
> 
> All I seem to get is an error in dmesg:
> 
> [   97.435446] processor cpu0: Offline failed.
> 
> > or directly writing to cpu hotplug registers, which makes
> > qemu crash with SIGSEGV following back trace:
> > 
> >kvm_flush_coalesced_mmio_buffer ()
> >while (ring->first != ring->last)
> >...
> >qemu_flush_coalesced_mmio_buffer
> >prepare_mmio_access
> >flatview_read_continue
> >flatview_read
> >address_space_read_full
> >address_space_rw
> >kvm_cpu_exec(cpu!0)
> >qemu_kvm_cpu_thread_fn
> > 
> > the reason for which is that ring == KVMState::coalesced_mmio_ring
> > happens to be a part of 1st CPU that was uplugged by guest.
> > 
> > Fix it by forbidding 1st cpu unplug from guest side and in addition
> > remove CPU0._EJ0 ACPI method to make clear that unplug of the first
> > CPU is not supported.
> > 
> > Signed-off-by: Igor Mammedov 
> > ---
> > CCing spapr and s390x folks in case targets need to prevent 1st CPU unplug 
> > as well
> > 
> 
> A spapr guest can _release_ the first cpu by doing something like:
> 
> # echo -n "/cpus/PowerPC,POWER8@0" > /sys/devices/system/cpu/release
> 
> But AFAIK, this doesn't unplug the cpu from a QEMU standpoint.

Unplugging CPU 0 with device_del should be ok too.

-- 
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-ppc] [PATCH] 40p: fix PCI interrupt routing

[David Gibson]

On Mon, Aug 27, 2018 at 07:12:29PM +0200, BALATON Zoltan wrote:
> On Mon, 27 Aug 2018, Mark Cave-Ayland wrote:
> > According to the PReP specification section 6.1.6 "System Interrupt
> > Assignments", all PCI interrupts are routed via IRQ 15.
> > 
> > With this patch applied it is now possible to boot the sandalfoot
> > zImage all the way through to a working userspace when using
> > OpenBIOS.
> > 
> > Signed-off-by: Mark Cave-Ayland 
> > ---
> > hw/ppc/prep.c | 9 +
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> > index 162b27a3b8..e82c1355d9 100644
> > --- a/hw/ppc/prep.c
> > +++ b/hw/ppc/prep.c
> > @@ -668,10 +668,11 @@ static void ibm_40p_init(MachineState *machine)
> > dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(11, 0), "i82378"));
> > qdev_connect_gpio_out(dev, 0,
> >   cpu->env.irq_inputs[PPC6xx_INPUT_INT]);
> > -sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(dev, 15));
> > -sysbus_connect_irq(pcihost, 1, qdev_get_gpio_in(dev, 13));
> > -sysbus_connect_irq(pcihost, 2, qdev_get_gpio_in(dev, 15));
> > -sysbus_connect_irq(pcihost, 3, qdev_get_gpio_in(dev, 13));
> > +/* According to PReP specification section 6.1.6 "System Interrupt
> > + * Assignments", all PCI interrupts are routed via IRQ 15 */
> > +for (i = 0; i < PCI_NUM_PINS; i++) {
> > +sysbus_connect_irq(pcihost, i, qdev_get_gpio_in(dev, 15));
> > +}
> 
> I'm not sure but this looks similar to what we had with sam460ex:
> 
> http://lists.nongnu.org/archive/html/qemu-ppc/2018-07/msg00359.html
> 
> I think you may not connect multiple interrupts to the same host irq line
> this way but you either need an OR gate or handle it within the mapping in
> the PCI host model (which is what we ended up with for the sam460ex).
> Peter's suggestion was to do whichever matches real hardware the most if you
> can find out that (as noted here also with more explanation that could be
> useful):
> 
> http://lists.nongnu.org/archive/html/qemu-ppc/2018-07/msg00360.html
> 
> But I could be mistaken in this case, haven't checked it in detail.

Yeah..  I was thinking this looked very familiar.  I believe you do
need to construct the OR explicitly, not just wire all the irqs to the
same line.

-- 
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] Guest crash when VNC connection is established with password (QEMU 2.5.1.1)

[John Y.]

(Sorry for repost,  I had asked in qemu-stable, but someone told me that I
should send to qemu-devel.)

After I updated my Centos to 7.5.1804, my virtual machine crashed when I
connected with vnc password.

*1. My problem：*

Here  what I tested:
(1).  started a guest with : qemu-system-x86_64 -m 2048 test.img -vnc
0.0.0.0:3 ,password -monitor stdio
(2). change password to 1234567.
(3). connected with VncViewer

Here was the output:

# qemu-system-x86_64 -m 2048 test.img -vnc 0.0.0.0:3,password -monitor stdio
> QEMU 2.5.1.1 monitor - type 'help' for more information
> (qemu) change vnc password 12345678
> (qemu) Segmentation fault
>

I  debugged with gdb and got:

> (qemu) change vnc password 12345678
> (qemu)
> Program received signal SIGSEGV, Segmentation fault.
> 0x7528cc80 in pthread_mutex_lock () from /lib64/libpthread.so.0
> (gdb) bt
> #0  0x7528cc80 in pthread_mutex_lock () from /lib64/libpthread.so.0
> #1  0x55a97f4a in qemu_mutex_lock (mutex=0x0) at
> util/qemu-thread-posix.c:73
> #2  0x55a5cce3 in qcrypto_gcrypt_mutex_lock (priv=0x76e90ca0)
> at crypto/init.c:97
> #3  0x76c217c5 in mutex_init () from /lib64/libgcrypt.so.11
> #4  0x76c21baa in _gcry_ath_mutex_lock () from
> /lib64/libgcrypt.so.11
> #5  0x76c5ad20 in lock_pool () from /lib64/libgcrypt.so.11
> #6  0x76c5be2e in _gcry_rngcsprng_fast_poll () from
> /lib64/libgcrypt.so.11
> #7  0x76c23d4d in _gcry_cipher_open () from /lib64/libgcrypt.so.11
> #8  0x55a60251 in qcrypto_cipher_new
> (alg=QCRYPTO_CIPHER_ALG_DES_RFB, mode=QCRYPTO_CIPHER_MODE_ECB,
> key=0x7fffdf30 "12345678\020", nkey=8, errp=0x7fffdf08) at
> ./crypto/cipher-gcrypt.c:97
> #9  0x559d8912 in protocol_client_auth_vnc (vs=0x565b5660,
> data=0x57a6c0a0 "\036\352\"s٘\373\345<ܯ\210L\b;$\220\300\246WUU",
> len=16) at ui/vnc.c:2551
> #10 0x559d60b5 in vnc_client_read (opaque=0x565b5660) at
> ui/vnc.c:1564
> #11 0x55a046a7 in aio_dispatch (ctx=0x56508790) at
> aio-posix.c:326
> #12 0x559f4166 in aio_ctx_dispatch (source=0x56508790,
> callback=0x0, user_data=0x0) at async.c:231
> #13 0x75f10969 in g_main_context_dispatch () from
> /lib64/libglib-2.0.so.0
> #14 0x55a026e7 in glib_pollfds_poll () at main-loop.c:211
> #15 0x55a027c4 in os_host_main_loop_wait (timeout=54752182) at
> main-loop.c:256
> #16 0x55a02874 in main_loop_wait (nonblocking=0) at main-loop.c:504
> #17 0x558251c1 in main_loop () at vl.c:1923
> #18 0x5582cbe5 in main (argc=8, argv=0x7fffe558,
> envp=0x7fffe5a0) at vl.c:4699
>


*2.  Other Infomation*
(1).  Version of qemu:
QEMU emulator version 2.5.1.1, Copyright (c) 2003-2008 Fabrice Bellard

(2).  kernel and os：
kernel : 4.16.3-1.el7.elrepo.x86_64
os: CentOS Linux release 7.5.1804 (Core)

(3).  I had try /usr/libexec/qemu-kvm(qemu-kvm-1.5.3-156.el7_5.5)  and qemu
2.11, both of them worked fine.

(4).  Has no problem witout vnc password.

(5). I update my host with yum update.

(6).  Everything work find before update.

*3.  My Question*
For some reasons I have to use qemu 2.5.11 and update Centos.
(1) What causes this problem and how can I solve it ?

Looking forward to your reply.

Regards,
John

Re: [Qemu-devel] [PATCH v3 8/9] qga: process_event() simplification

[Michael Roth]

Quoting Marc-André Lureau (2018-08-25 08:57:23)
> Simplify the code around qmp_dispatch():
> - rely on qmp_dispatch/check_obj() for message checking
> - have a single send_response() point
> - constify send_response() argument
> 
> It changes a couple of error messages:
> 
> * When @req isn't a dictionary, from
> Invalid JSON syntax
>   to
> QMP input must be a JSON object
> 
> * When @req lacks member "execute", from
> this feature or command is not currently supported
>   to
> QMP input lacks member 'execute'
> 
> CC: Michael Roth 
> Signed-off-by: Marc-André Lureau 
> ---
>  qga/main.c | 47 +--
>  1 file changed, 9 insertions(+), 38 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 6d70242d05..f0ec035996 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -544,15 +544,15 @@ fail:
>  #endif
>  }
> 
> -static int send_response(GAState *s, QDict *payload)
> +static int send_response(GAState *s, const QDict *rsp)
>  {
>  const char *buf;
>  QString *payload_qstr, *response_qstr;
>  GIOStatus status;
> 
> -g_assert(payload && s->channel);
> +g_assert(rsp && s->channel);
> 
> -payload_qstr = qobject_to_json(QOBJECT(payload));
> +payload_qstr = qobject_to_json(QOBJECT(rsp));
>  if (!payload_qstr) {
>  return -EINVAL;
>  }
> @@ -578,53 +578,24 @@ static int send_response(GAState *s, QDict *payload)
>  return 0;
>  }
> 
> -static void process_command(GAState *s, QDict *req)
> -{
> -QDict *rsp;
> -int ret;
> -
> -g_assert(req);
> -g_debug("processing command");
> -rsp = qmp_dispatch(_commands, QOBJECT(req), false);
> -if (rsp) {
> -ret = send_response(s, rsp);
> -if (ret < 0) {
> -g_warning("error sending response: %s", strerror(-ret));
> -}
> -qobject_unref(rsp);
> -}

We used to check here for the success-response=false scenario (e.g.
guest-shutdown qga command). Now we pass the result of qmp_dispatch()
directly to send_response(), which looks like that might cause an
assertion now. Do we need to add handling for this?

> -}
> -
>  /* handle requests/control events coming in over the channel */
>  static void process_event(void *opaque, QObject *obj, Error *err)
>  {
>  GAState *s = opaque;
> -QDict *req, *rsp;
> +QDict *rsp;
>  int ret;
> 
>  g_debug("process_event: called");
>  assert(!obj != !err);
>  if (err) {
> -goto err;
> -}
> -req = qobject_to(QDict, obj);
> -if (!req) {
> -error_setg(, "Input must be a JSON object");
> -goto err;
> -}
> -if (!qdict_haskey(req, "execute")) {
> -g_warning("unrecognized payload format");
> -error_setg(, QERR_UNSUPPORTED);
> -goto err;
> +rsp = qmp_error_response(err);
> +goto end;
>  }
> 
> -process_command(s, req);
> -qobject_unref(obj);
> -return;
> +g_debug("processing command");
> +rsp = qmp_dispatch(_commands, obj, false);
> 
> -err:
> -g_warning("failed to parse event: %s", error_get_pretty(err));
> -rsp = qmp_error_response(err);
> +end:
>  ret = send_response(s, rsp);
>  if (ret < 0) {
>  g_warning("error sending error response: %s", strerror(-ret));
> -- 
> 2.18.0.547.g1d89318c48
> 

[Qemu-devel] [PATCH v3] editorconfig: set emacs mode

[Marc-André Lureau]

Some time ago, I proposed to use an (eval) in .dir-locals.el to set
the mode for all json files and Makefile. Unfortunately, this isn't
safe, and emacs will prompt the user, which isn't very friendly.

Fortunately, editorconfig provides a special config key which does
allow to set the emacs mode. Add a few missing entries and set the
emacs mode.

Update top comment to provide a short summary about the file and the
IDE plugins while at it.

Signed-off-by: Marc-André Lureau 
---
 .editorconfig | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/.editorconfig b/.editorconfig
index b2022e391a..1582883393 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -1,4 +1,10 @@
-# http://editorconfig.org
+# EditorConfig is a file format and collection of text editor plugins
+# for maintaining consistent coding styles between different editors
+# and IDEs. Most popular editors support this either natively or via
+# plugin.
+#
+# Check https://editorconfig.org for details.
+
 root = true
 
 [*]
@@ -6,10 +12,23 @@ end_of_line = lf
 insert_final_newline = true
 charset = utf-8
 
+[*.mak]
+indent_style = tab
+indent_size = 8
+file_type_emacs = makefile
+
 [Makefile*]
 indent_style = tab
 indent_size = 8
+file_type_emacs = makefile
 
 [*.{c,h}]
 indent_style = space
 indent_size = 4
+
+[*.{vert,frag}]
+file_type_emacs = glsl
+
+[*.json]
+indent_style = space
+file_type_emacs = python
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 7/9] chardev: add a note about frontend sources and context switch

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 include/chardev/char-fe.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 4677a9e65a..25fe62e1cf 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -166,6 +166,9 @@ void qemu_chr_fe_printf(CharBackend *be, const char *fmt, 
...)
  * is active; return the #GSource's tag.  If it is disconnected,
  * or without associated Chardev, return 0.
  *
+ * Note that you are responsible to update the front-end sources if
+ * you are switching the main context with qemu_chr_fe_set_handlers().
+ *
  * Returns: the source tag
  */
 guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 9/9] char-pty: remove write_lock usage

[Marc-André Lureau]

The lock usage was described with its introduction in commit
9005b2a7589540a3733b3abdcfbccfe7746cd1a1. It was necessary because PTY
write() shares more state than GIOChannel with other
operations.

This made char-pty a bit different from other chardev, that only lock
around the write operation.  This was apparent in commit
7b3621f47a990c5099c6385728347f69a8d0e55c, which introduced an idle
source to avoid the lock.

By removing the PTY chardev state sharing on write() with previous
patch, we can remove the lock and the idle source.

Signed-off-by: Marc-André Lureau 
---
 chardev/char-pty.c | 50 +++---
 1 file changed, 3 insertions(+), 47 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index ebd7035c6d..b28bf97233 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -40,15 +40,12 @@ typedef struct {
 QIOChannel *ioc;
 int read_bytes;
 
-/* Protected by the Chardev chr_write_lock.  */
 int connected;
 GSource *timer_src;
-GSource *open_source;
 } PtyChardev;
 
 #define PTY_CHARDEV(obj) OBJECT_CHECK(PtyChardev, (obj), TYPE_CHARDEV_PTY)
 
-static void pty_chr_update_read_handler_locked(Chardev *chr);
 static void pty_chr_state(Chardev *chr, int connected);
 
 static void pty_chr_timer_cancel(PtyChardev *s)
@@ -60,32 +57,19 @@ static void pty_chr_timer_cancel(PtyChardev *s)
 }
 }
 
-static void pty_chr_open_src_cancel(PtyChardev *s)
-{
-if (s->open_source) {
-g_source_destroy(s->open_source);
-g_source_unref(s->open_source);
-s->open_source = NULL;
-}
-}
-
 static gboolean pty_chr_timer(gpointer opaque)
 {
 struct Chardev *chr = CHARDEV(opaque);
 PtyChardev *s = PTY_CHARDEV(opaque);
 
-qemu_mutex_lock(>chr_write_lock);
 pty_chr_timer_cancel(s);
-pty_chr_open_src_cancel(s);
 if (!s->connected) {
 /* Next poll ... */
-pty_chr_update_read_handler_locked(chr);
+qemu_chr_be_update_read_handlers(chr, chr->gcontext);
 }
-qemu_mutex_unlock(>chr_write_lock);
 return FALSE;
 }
 
-/* Called with chr_write_lock held.  */
 static void pty_chr_rearm_timer(Chardev *chr, int ms)
 {
 PtyChardev *s = PTY_CHARDEV(chr);
@@ -98,8 +82,7 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
 g_free(name);
 }
 
-/* Called with chr_write_lock held.  */
-static void pty_chr_update_read_handler_locked(Chardev *chr)
+static void pty_chr_update_read_handler(Chardev *chr)
 {
 PtyChardev *s = PTY_CHARDEV(chr);
 GPollFD pfd;
@@ -121,14 +104,6 @@ static void pty_chr_update_read_handler_locked(Chardev 
*chr)
 }
 }
 
-static void pty_chr_update_read_handler(Chardev *chr)
-{
-qemu_mutex_lock(>chr_write_lock);
-pty_chr_update_read_handler_locked(chr);
-qemu_mutex_unlock(>chr_write_lock);
-}
-
-/* Called with chr_write_lock held.  */
 static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
 PtyChardev *s = PTY_CHARDEV(chr);
@@ -183,23 +158,11 @@ static gboolean pty_chr_read(QIOChannel *chan, 
GIOCondition cond, void *opaque)
 return TRUE;
 }
 
-static gboolean qemu_chr_be_generic_open_func(gpointer opaque)
-{
-Chardev *chr = CHARDEV(opaque);
-PtyChardev *s = PTY_CHARDEV(opaque);
-
-s->open_source = NULL;
-qemu_chr_be_event(chr, CHR_EVENT_OPENED);
-return FALSE;
-}
-
-/* Called with chr_write_lock held.  */
 static void pty_chr_state(Chardev *chr, int connected)
 {
 PtyChardev *s = PTY_CHARDEV(chr);
 
 if (!connected) {
-pty_chr_open_src_cancel(s);
 remove_fd_in_watch(chr);
 s->connected = 0;
 /* (re-)connect poll interval for idle guests: once per second.
@@ -209,13 +172,8 @@ static void pty_chr_state(Chardev *chr, int connected)
 } else {
 pty_chr_timer_cancel(s);
 if (!s->connected) {
-g_assert(s->open_source == NULL);
-s->open_source = g_idle_source_new();
 s->connected = 1;
-g_source_set_callback(s->open_source,
-  qemu_chr_be_generic_open_func,
-  chr, NULL);
-g_source_attach(s->open_source, chr->gcontext);
+qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
 if (!chr->gsource) {
 chr->gsource = io_add_watch_poll(chr, s->ioc,
@@ -231,11 +189,9 @@ static void char_pty_finalize(Object *obj)
 Chardev *chr = CHARDEV(obj);
 PtyChardev *s = PTY_CHARDEV(obj);
 
-qemu_mutex_lock(>chr_write_lock);
 pty_chr_state(chr, 0);
 object_unref(OBJECT(s->ioc));
 pty_chr_timer_cancel(s);
-qemu_mutex_unlock(>chr_write_lock);
 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Marc-André Lureau]

terminal3270 is uses the front-end side of the chardev. It shouldn't
create sources from backend side context. Fwiw, send_timing_mark_cb
calls qemu_chr_fe_write_all() which should be thread safe.

This partially reverts changes from commit
2c716ba1506769c9be2caa02f0f6d6e7c00f4304.

CC: Peter Xu 
Signed-off-by: Marc-André Lureau 
---
 hw/char/terminal3270.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index e9c45e55b1..35b079d5c4 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -31,7 +31,7 @@ typedef struct Terminal3270 {
 uint8_t outv[OUTPUT_BUFFER_SIZE];
 int in_len;
 bool handshake_done;
-GSource *timer_src;
+guint timer_tag;
 } Terminal3270;
 
 #define TYPE_TERMINAL_3270 "x-terminal3270"
@@ -47,10 +47,9 @@ static int terminal_can_read(void *opaque)
 
 static void terminal_timer_cancel(Terminal3270 *t)
 {
-if (t->timer_src) {
-g_source_destroy(t->timer_src);
-g_source_unref(t->timer_src);
-t->timer_src = NULL;
+if (t->timer_tag) {
+g_source_remove(t->timer_tag);
+t->timer_tag = 0;
 }
 }
 
@@ -100,8 +99,7 @@ static void terminal_read(void *opaque, const uint8_t *buf, 
int size)
 assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
 
 terminal_timer_cancel(t);
-t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
-   send_timing_mark_cb, t);
+t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
 memcpy(>inv[t->in_len], buf, size);
 t->in_len += size;
 if (t->in_len < 2) {
@@ -160,8 +158,7 @@ static void chr_event(void *opaque, int event)
  * char-socket.c. Once qemu receives the terminal-type of the
  * client, mark handshake done and trigger everything rolling again.
  */
-t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
-   send_timing_mark_cb, t);
+t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
 break;
 case CHR_EVENT_CLOSED:
 sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 8/9] char-pty: remove check for connection on write

[Marc-André Lureau]

This doesn't give much compared to the 1 second timer: there is no
reliable / unracy way to have slave connect & master write. However,
we can simplify the code around chr_write() since the write lock is no
longer needed for various other char-pty callbacks (see following
patch).

Signed-off-by: Marc-André Lureau 
---
 chardev/char-pty.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 68fd4e20c3..ebd7035c6d 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -134,11 +134,7 @@ static int char_pty_chr_write(Chardev *chr, const uint8_t 
*buf, int len)
 PtyChardev *s = PTY_CHARDEV(chr);
 
 if (!s->connected) {
-/* guest sends data, check for (re-)connect */
-pty_chr_update_read_handler_locked(chr);
-if (!s->connected) {
-return len;
-}
+return len;
 }
 return io_channel_send(s->ioc, buf, len);
 }
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 5/9] char-fe: set_handlers() needs an associted chardev

[Marc-André Lureau]

It is futile to call qemu_chr_fe_set_handlers() without an associated
chardev, because the function is doing nothing in that case, not even
reporting an error, it would likely be a programming error. Let's not
handle that hypothetical case.

(fwiw, I introduced the check in commit
94a40fc56036b5058b0b194d9e372a22e65ce7be, that was a mistake imho)

Signed-off-by: Marc-André Lureau 
---
 include/chardev/char-fe.h | 2 --
 chardev/char-fe.c | 7 +--
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 21071f1fb1..4677a9e65a 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -82,8 +82,6 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
  *
  * Set the front end char handlers. The front end takes the focus if
  * any of the handler is non-NULL.
- *
- * Without associated Chardev, nothing is changed.
  */
 void qemu_chr_fe_set_handlers(CharBackend *b,
   IOCanReadHandler *fd_can_read,
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 6ed8bff46a..e3b1c54721 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -254,14 +254,9 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
   GMainContext *context,
   bool set_open)
 {
-Chardev *s;
+Chardev *s = b->chr;
 int fe_open;
 
-s = b->chr;
-if (!s) {
-return;
-}
-
 if (!opaque && !fd_can_read && !fd_read && !fd_event) {
 fe_open = 0;
 remove_fd_in_watch(s);
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 1/9] char.h: fix gtk-doc comment style

[Marc-André Lureau]

Use the gtk-doc function comment style, as documented in:
https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

Signed-off-by: Marc-André Lureau 
---
 include/chardev/char-fe.h | 81 ++-
 include/chardev/char.h| 61 +
 2 files changed, 63 insertions(+), 79 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index c67271f1ba..21071f1fb1 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -20,7 +20,7 @@ struct CharBackend {
 };
 
 /**
- * @qemu_chr_fe_init:
+ * qemu_chr_fe_init:
  *
  * Initializes a front end for the given CharBackend and
  * Chardev. Call qemu_chr_fe_deinit() to remove the association and
@@ -31,7 +31,7 @@ struct CharBackend {
 bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp);
 
 /**
- * @qemu_chr_fe_deinit:
+ * qemu_chr_fe_deinit:
  * @b: a CharBackend
  * @del: if true, delete the chardev backend
 *
@@ -42,9 +42,9 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error 
**errp);
 void qemu_chr_fe_deinit(CharBackend *b, bool del);
 
 /**
- * @qemu_chr_fe_get_driver:
+ * qemu_chr_fe_get_driver:
  *
- * Returns the driver associated with a CharBackend or NULL if no
+ * Returns: the driver associated with a CharBackend or NULL if no
  * associated Chardev.
  * Note: avoid this function as the driver should never be accessed directly,
  *   especially by the frontends that support chardevice hotswap.
@@ -53,21 +53,21 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del);
 Chardev *qemu_chr_fe_get_driver(CharBackend *be);
 
 /**
- * @qemu_chr_fe_backend_connected:
+ * qemu_chr_fe_backend_connected:
  *
- * Returns true if there is a chardevice associated with @be.
+ * Returns: true if there is a chardevice associated with @be.
  */
 bool qemu_chr_fe_backend_connected(CharBackend *be);
 
 /**
- * @qemu_chr_fe_backend_open:
+ * qemu_chr_fe_backend_open:
  *
- * Returns true if chardevice associated with @be is open.
+ * Returns: true if chardevice associated with @be is open.
  */
 bool qemu_chr_fe_backend_open(CharBackend *be);
 
 /**
- * @qemu_chr_fe_set_handlers:
+ * qemu_chr_fe_set_handlers:
  * @b: a CharBackend
  * @fd_can_read: callback to get the amount of data the frontend may
  *   receive
@@ -95,7 +95,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
   bool set_open);
 
 /**
- * @qemu_chr_fe_take_focus:
+ * qemu_chr_fe_take_focus:
  *
  * Take the focus (if the front end is muxed).
  *
@@ -104,14 +104,14 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
 void qemu_chr_fe_take_focus(CharBackend *b);
 
 /**
- * @qemu_chr_fe_accept_input:
+ * qemu_chr_fe_accept_input:
  *
  * Notify that the frontend is ready to receive data
  */
 void qemu_chr_fe_accept_input(CharBackend *be);
 
 /**
- * @qemu_chr_fe_disconnect:
+ * qemu_chr_fe_disconnect:
  *
  * Close a fd accepted by character backend.
  * Without associated Chardev, do nothing.
@@ -119,7 +119,7 @@ void qemu_chr_fe_accept_input(CharBackend *be);
 void qemu_chr_fe_disconnect(CharBackend *be);
 
 /**
- * @qemu_chr_fe_wait_connected:
+ * qemu_chr_fe_wait_connected:
  *
  * Wait for characted backend to be connected, return < 0 on error or
  * if no associated Chardev.
@@ -127,19 +127,18 @@ void qemu_chr_fe_disconnect(CharBackend *be);
 int qemu_chr_fe_wait_connected(CharBackend *be, Error **errp);
 
 /**
- * @qemu_chr_fe_set_echo:
+ * qemu_chr_fe_set_echo:
+ * @echo true to enable echo, false to disable echo
  *
  * Ask the backend to override its normal echo setting.  This only really
  * applies to the stdio backend and is used by the QMP server such that you
  * can see what you type if you try to type QMP commands.
  * Without associated Chardev, do nothing.
- *
- * @echo true to enable echo, false to disable echo
  */
 void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
 
 /**
- * @qemu_chr_fe_set_open:
+ * qemu_chr_fe_set_open:
  *
  * Set character frontend open status.  This is an indication that the
  * front end is ready (or not) to begin doing I/O.
@@ -148,83 +147,77 @@ void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
 void qemu_chr_fe_set_open(CharBackend *be, int fe_open);
 
 /**
- * @qemu_chr_fe_printf:
+ * qemu_chr_fe_printf:
+ * @fmt see #printf
  *
  * Write to a character backend using a printf style interface.  This
  * function is thread-safe. It does nothing without associated
  * Chardev.
- *
- * @fmt see #printf
  */
 void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
 GCC_FMT_ATTR(2, 3);
 
 /**
- * @qemu_chr_fe_add_watch:
+ * qemu_chr_fe_add_watch:
+ * @cond the condition to poll for
+ * @func the function to call when the condition happens
+ * @user_data the opaque pointer to pass to @func
  *
  * If the backend is connected, create and add a #GSource that fires
  * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP)
  * is active; return the #GSource's tag.  If it is 

[Qemu-devel] [PATCH 3/9] chardev: use a child source for qio input source

[Marc-André Lureau]

GLib child source were added with version 2.28. We can use them now
that we bumped our requirement to 2.40.

Signed-off-by: Marc-André Lureau 
---
 chardev/char-io.c | 48 +--
 1 file changed, 5 insertions(+), 43 deletions(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index f81052481a..8ced184160 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -33,7 +33,6 @@ typedef struct IOWatchPoll {
 IOCanReadHandler *fd_can_read;
 GSourceFunc fd_read;
 void *opaque;
-GMainContext *context;
 } IOWatchPoll;
 
 static IOWatchPoll *io_watch_poll_from_source(GSource *source)
@@ -55,47 +54,24 @@ static gboolean io_watch_poll_prepare(GSource *source,
 iwp->src = qio_channel_create_watch(
 iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
 g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
-g_source_attach(iwp->src, iwp->context);
-} else {
-g_source_destroy(iwp->src);
+g_source_add_child_source(source, iwp->src);
 g_source_unref(iwp->src);
+} else {
+g_source_remove_child_source(source, iwp->src);
 iwp->src = NULL;
 }
 return FALSE;
 }
 
-static gboolean io_watch_poll_check(GSource *source)
-{
-return FALSE;
-}
-
 static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
gpointer user_data)
 {
-abort();
-}
-
-static void io_watch_poll_finalize(GSource *source)
-{
-/* Due to a glib bug, removing the last reference to a source
- * inside a finalize callback causes recursive locking (and a
- * deadlock).  This is not a problem inside other callbacks,
- * including dispatch callbacks, so we call io_remove_watch_poll
- * to remove this source.  At this point, iwp->src must
- * be NULL, or we would leak it.
- *
- * This would be solved much more elegantly by child sources,
- * but we support older glib versions that do not have them.
- */
-IOWatchPoll *iwp = io_watch_poll_from_source(source);
-assert(iwp->src == NULL);
+return G_SOURCE_CONTINUE;
 }
 
 static GSourceFuncs io_watch_poll_funcs = {
 .prepare = io_watch_poll_prepare,
-.check = io_watch_poll_check,
 .dispatch = io_watch_poll_dispatch,
-.finalize = io_watch_poll_finalize,
 };
 
 GSource *io_add_watch_poll(Chardev *chr,
@@ -115,7 +91,6 @@ GSource *io_add_watch_poll(Chardev *chr,
 iwp->ioc = ioc;
 iwp->fd_read = (GSourceFunc) fd_read;
 iwp->src = NULL;
-iwp->context = context;
 
 name = g_strdup_printf("chardev-iowatch-%s", chr->label);
 g_source_set_name((GSource *)iwp, name);
@@ -126,23 +101,10 @@ GSource *io_add_watch_poll(Chardev *chr,
 return (GSource *)iwp;
 }
 
-static void io_remove_watch_poll(GSource *source)
-{
-IOWatchPoll *iwp;
-
-iwp = io_watch_poll_from_source(source);
-if (iwp->src) {
-g_source_destroy(iwp->src);
-g_source_unref(iwp->src);
-iwp->src = NULL;
-}
-g_source_destroy(>parent);
-}
-
 void remove_fd_in_watch(Chardev *chr)
 {
 if (chr->gsource) {
-io_remove_watch_poll(chr->gsource);
+g_source_destroy(chr->gsource);
 chr->gsource = NULL;
 }
 }
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 0/9] chardev: various cleanups and improvements

[Marc-André Lureau]

Hi,

Some chardev-related patches I have accumulated.
Please review, thanks!

Marc-André Lureau (9):
  char.h: fix gtk-doc comment style
  chardev: mark the calls that allow an implicit mux monitor
  chardev: use a child source for qio input source
  char: update the mux hanlders in class callback
  char-fe: set_handlers() needs an associted chardev
  terminal3270: do not use backend timer sources
  chardev: add a note about frontend sources and context switch
  char-pty: remove check for connection on write
  char-pty: remove write_lock usage

 include/chardev/char-fe.h  | 84 ++
 include/chardev/char-mux.h |  1 -
 include/chardev/char.h | 81 +++-
 chardev/char-fe.c  | 11 +
 chardev/char-io.c  | 48 +++---
 chardev/char-mux.c |  5 ++-
 chardev/char-pty.c | 56 ++---
 chardev/char.c | 32 +++
 gdbstub.c  |  6 ++-
 hw/char/terminal3270.c | 15 +++
 hw/char/xen_console.c  |  5 ++-
 net/slirp.c|  5 ++-
 vl.c   | 10 ++---
 13 files changed, 145 insertions(+), 214 deletions(-)

-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 2/9] chardev: mark the calls that allow an implicit mux monitor

[Marc-André Lureau]

This is mostly for readability of the code. Let's make it clear which
callers can create an implicit monitor when the chardev is muxed.

This will also enforce a safer behaviour, as we don't really support
creating monitor anywhere/anytime at the moment. Add an assert() to
make sure the programmer explicitely wanted that behaviour.

There are documented cases, such as: -serial/-parallel/-virtioconsole
and to less extent -debugcon.

Less obvious and questionable ones are -gdb, SLIRP -guestfwd and Xen
console. Add a FIXME note for those, but keep the support for now.

Other qemu_chr_new() callers either have a fixed parameter/filename
string or do not need it, such as -qtest:

* qtest.c: qtest_init()
  Afaik, only used by tests/libqtest.c, without mux. I don't think we
  support it outside of qemu testing: drop support for implicit mux
  monitor (qemu_chr_new() call: no implicit mux now).

* hw/
  All with literal @filename argument that doesn't enable mux monitor.

* tests/
  All with @filename argument that doesn't enable mux monitor.

On a related note, the list of monitor creation places:

- the chardev creators listed above: all from command line (except
  perhaps Xen console?)

- -gdb & hmp gdbserver will create a "GDB monitor command" chardev
  that is wired to an HMP monitor.

- -mon command line option

>From this short study, I would like to think that a monitor may only
be created in the main thread today, though I remain skeptical :)

Signed-off-by: Marc-André Lureau 
---
 include/chardev/char.h | 24 
 chardev/char.c | 32 +---
 gdbstub.c  |  6 +-
 hw/char/xen_console.c  |  5 -
 net/slirp.c|  5 -
 vl.c   | 10 +-
 6 files changed, 63 insertions(+), 19 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 3e4fe6dad0..268daaa90b 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -105,14 +105,27 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
  * @filename: the URI
  *
  * Create a new character backend from a URI.
+ * Do not implicitly initialize a monitor if the chardev is muxed.
  *
  * Returns: a new character backend
  */
 Chardev *qemu_chr_new(const char *label, const char *filename);
 
 /**
- * qemu_chr_change:
- * @opts: the new backend options
+ * qemu_chr_new_mux_mon:
+ * @label: the name of the backend
+ * @filename: the URI
+ *
+ * Create a new character backend from a URI.
+ * Implicitly initialize a monitor if the chardev is muxed.
+ *
+ * Returns: a new character backend
+ */
+Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
+
+/**
+* qemu_chr_change:
+* @opts: the new backend options
  *
  * Change an existing character backend
  */
@@ -129,6 +142,7 @@ void qemu_chr_cleanup(void);
  * qemu_chr_new_noreplay:
  * @label: the name of the backend
  * @filename: the URI
+ * @with_mux_mon: if chardev is muxed, initialize a monitor
  *
  * Create a new character backend from a URI.
  * Character device communications are not written
@@ -136,7 +150,8 @@ void qemu_chr_cleanup(void);
  *
  * Returns: a new character backend
  */
-Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
+Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
+   bool with_mux_mon);
 
 /**
  * qemu_chr_be_can_write:
@@ -194,7 +209,8 @@ bool qemu_chr_has_feature(Chardev *chr,
   ChardevFeature feature);
 void qemu_chr_set_feature(Chardev *chr,
   ChardevFeature feature);
-QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
+QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
+bool with_mux_mon);
 int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
 #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
 int qemu_chr_wait_connected(Chardev *chr, Error **errp);
diff --git a/chardev/char.c b/chardev/char.c
index 76d866e6fe..c1b89b6638 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -329,7 +329,8 @@ int qemu_chr_wait_connected(Chardev *chr, Error **errp)
 return 0;
 }
 
-QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
+QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
+bool with_mux_mon)
 {
 char host[65], port[33], width[8], height[8];
 int pos;
@@ -344,6 +345,10 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
char *filename)
 }
 
 if (strstart(filename, "mon:", )) {
+if (!with_mux_mon) {
+error_report("mon: isn't supported in this context");
+return NULL;
+}
 filename = p;
 qemu_opt_set(opts, "mux", "on", _abort);
 if (strcmp(filename, "stdio") == 0) {
@@ -683,7 +688,8 @@ out:
 return chr;
 }
 
-Chardev 

[Qemu-devel] [PATCH 4/9] char: update the mux hanlders in class callback

[Marc-André Lureau]

Instead of handling mux chardev in a special way in
qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
class callback instead.

Signed-off-by: Marc-André Lureau 
---
 include/chardev/char-mux.h | 1 -
 chardev/char-fe.c  | 4 
 chardev/char-mux.c | 5 +++--
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/chardev/char-mux.h b/include/chardev/char-mux.h
index 1e13187767..572cefd517 100644
--- a/include/chardev/char-mux.h
+++ b/include/chardev/char-mux.h
@@ -55,7 +55,6 @@ typedef struct MuxChardev {
 #define CHARDEV_IS_MUX(chr) \
 object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX)
 
-void mux_chr_set_handlers(Chardev *chr, GMainContext *context);
 void mux_set_focus(Chardev *chr, int focus);
 void mux_chr_send_all_event(Chardev *chr, int event);
 
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b1f228e8b5..6ed8bff46a 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -288,10 +288,6 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
 qemu_chr_be_event(s, CHR_EVENT_OPENED);
 }
 }
-
-if (CHARDEV_IS_MUX(s)) {
-mux_chr_set_handlers(s, context);
-}
 }
 
 void qemu_chr_fe_take_focus(CharBackend *b)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 6055e76293..9406eaf08d 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -278,7 +278,7 @@ static void char_mux_finalize(Object *obj)
 qemu_chr_fe_deinit(>chr, false);
 }
 
-void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
+static void mux_chr_update_read_handlers(Chardev *chr)
 {
 MuxChardev *d = MUX_CHARDEV(chr);
 
@@ -289,7 +289,7 @@ void mux_chr_set_handlers(Chardev *chr, GMainContext 
*context)
  mux_chr_event,
  NULL,
  chr,
- context, true);
+ chr->gcontext, true);
 }
 
 void mux_set_focus(Chardev *chr, int focus)
@@ -383,6 +383,7 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
 cc->chr_add_watch = mux_chr_add_watch;
 cc->chr_be_event = mux_chr_be_event;
 cc->chr_machine_done = open_muxes;
+cc->chr_update_read_handler = mux_chr_update_read_handlers;
 }
 
 static const TypeInfo char_mux_type_info = {
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor

[Marc-André Lureau]

Hi
On Mon, Aug 27, 2018 at 10:10 AM Markus Armbruster  wrote:
>
> Marc-André Lureau  writes:
>
> > Hi
> > On Fri, Aug 24, 2018 at 9:37 AM Markus Armbruster  wrote:
> >>
> >> Marc-André Lureau  writes:
> >>
> >> > This is mostly for readability of the code. Let's make it clear which
> >> > callers can create an implicit monitor when the chardev is muxed.
> >> >
> >> > This will also enforce a safer behaviour, as we don't really support
> >> > creating monitor anywhere/anytime at the moment.
> >> >
> >> > There are documented cases, such as: -serial/-parallel/-virtioconsole
> >> > and to less extent -debugcon.
> >> >
> >> > Less obvious and questionnable ones are -gdb and Xen console. Add a
> >> > FIXME note for those, but keep the support for now.
> >> >
> >> > Other qemu_chr_new() callers either have a fixed parameter/filename
> >> > string, or do preliminary checks on the string (such as slirp), or do
> >> > not need it, such as -qtest.
> >> >
> >> > On a related note, the list of monitor creation places:
> >> >
> >> > - the chardev creators listed above: all from command line (except
> >> >   perhaps Xen console?)
> >> >
> >> > - -gdb & hmp gdbserver will create a "GDB monitor command" chardev
> >> >   that is wired to an HMP monitor.
> >> >
> >> > - -mon command line option
> >>
> >> Aside: the question "what does it mean to connect a monitor to a chardev
> >> that has an implicit monitor" crosses my mind.  Next thought is "the
> >> day's too short to go there".
> >>
> >> > From this short study, I would like to think that a monitor may only
> >> > be created in the main thread today, though I remain skeptical :)
> >>
> >> Hah!
> >>
> >> > Signed-off-by: Marc-André Lureau 
> >> > ---
> >> >  include/chardev/char.h | 18 +-
> >> >  chardev/char.c | 21 +
> >> >  gdbstub.c  |  6 +-
> >> >  hw/char/xen_console.c  |  5 -
> >> >  vl.c   |  8 
> >> >  5 files changed, 47 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> >> > index 6f0576e214..be2b9b817e 100644
> >> > --- a/
> >> > +++ b/include/chardev/char.h
> >> > @@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> >> >   * @qemu_chr_new:
> >> >   *
> >> >   * Create a new character backend from a URI.
> >> > + * Do not implicitely initialize a monitor if the chardev is muxed.
> >> >   *
> >> >   * @label the name of the backend
> >> >   * @filename the URI
> >> > @@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> >> >   */
> >> >  Chardev *qemu_chr_new(const char *label, const char *filename);
> >> >
> >> > +/**
> >> > + * @qemu_chr_new_mux_mon:
> >> > + *
> >> > + * Create a new character backend from a URI.
> >> > + * Implicitely initialize a monitor if the chardev is muxed.
> >> > + *
> >> > + * @label the name of the backend
> >> > + * @filename the URI
> >>
> >> I'm no friend of GTK-Doc style, but where we use it, we should use it
> >> correctly: put a colon after @param.
> >
> > I copied existing comment... Should I fixed all others in the headers?
>
> Do we expect to actually *use* doc comments for anything?  We haven't in
> a decade or so, but if we still expect to all the same, then not fixing
> them up now merely delays the inevitable.
>
> Do we think adding the colons makes the comments easier to read?  For
> what it's worth, I do.
>
> Decision's up to you.

Let's improve it.

>
> >> > + *
> >> > + * Returns: a new character backend
> >> > + */
> >> > +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
> >> > +
> >> >  /**
> >> >   * @qemu_chr_change:
> >> >   *
> >> > @@ -138,10 +152,12 @@ void qemu_chr_cleanup(void);
> >> >   *
> >> >   * @label the name of the backend
> >> >   * @filename the URI
> >> > + * @with_mux_mon if chardev is muxed, initialize a monitor
> >> >   *
> >> >   * Returns: a new character backend
> >> >   */
> >> > -Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
> >> > +Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> >> > +   bool with_mux_mon);
> >> >
> >> >  /**
> >> >   * @qemu_chr_be_can_write:
> >> > diff --git a/chardev/char.c b/chardev/char.c
> >> > index 76d866e6fe..2c295ad676 100644
> >> > --- a/chardev/char.c
> >> > +++ b/chardev/char.c
> >> > @@ -683,7 +683,8 @@ out:
> >> >  return chr;
> >> >  }
> >> >
> >> > -Chardev *qemu_chr_new_noreplay(const char *label, const char *filename)
> >> > +Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> >> > +   bool with_mux_mon)
> >> >  {
> >> >  const char *p;
> >> >  Chardev *chr;
> >> > @@ -702,17 +703,19 @@ Chardev *qemu_chr_new_noreplay(const char *label, 
> >> > const char *filename)
> >> >  if (err) {
> >> >  error_report_err(err);
> >> >  }
> >> > -if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
> 

[Qemu-devel] [PATCH v2 2/2] qapi: Add comments to aid debugging generated introspection

[Eric Blake]

We consciously chose in commit 1a9a507b to hide QAPI type names
from the introspection output on the wire, but added a command
line option -u to unmask the type name when doing a debug build.
The unmask option still remains useful to some other forms of
automated analysis, so it will not be removed; however, when it
is not in use, the generated .c file can be hard to read.  At
the time when we first introduced masking, the generated file
consisted only of a monolithic C string, so there was no clean
way to inject any comments.

Later, in commit 7d0f982b, we switched the generation to output
a QLit object, in part to make it easier for future addition of
conditional compilation.  In fact, commit d626b6c1 took advantage
of this by passing a tuple instead of a bare object for encoding
the output of conditionals.  By extending that tuple, we can now
interject strategic comments.

For now, type name debug aid comments are only output once per
meta-type, rather than at all uses of the number used to encode
the type within the introspection data.  But this is still a lot
more convenient than having to regenerate the file with the
unmask operation temporarily turned on - merely search the
generated file for '"NNN" =' to learn the corresponding source
name and associated definition of type NNN.

The generated qapi-introspect.c changes only with the addition
of comments, such as:

| @@ -14755,6 +15240,7 @@
|  { "name", QLIT_QSTR("[485]"), },
|  {}
|  })),
| +/* "485" = QCryptoBlockInfoLUKSSlot */
|  QLIT_QDICT(((QLitDictEntry[]) {
|  { "members", QLIT_QLIST(((QLitObject[]) {
|  QLIT_QDICT(((QLitDictEntry[]) {

Signed-off-by: Eric Blake 

---
v2: rebase on conditional code additions, drop patch to remove -u,
update documentation to match
---
 docs/devel/qapi-code-gen.txt |  1 +
 scripts/qapi/introspect.py   | 27 +--
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index c2e11465f0f..9d5b1409e72 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1412,6 +1412,7 @@ Example:
 { "name", QLIT_QSTR("Event") },
 { }
 })),
+/* "0" = q_empty */
 QLIT_QDICT(((QLitDictEntry[]) {
 { "members", QLIT_QLIST(((QLitObject[]) {
 { }
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 43e81a06938..67d6106f77b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -19,12 +19,17 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
 return level * 4 * ' '

 if isinstance(obj, tuple):
-ifobj, ifcond = obj
-ret = gen_if(ifcond)
+ifobj, extra = obj
+ifcond = extra.get('if')
+comment = extra.get('comment')
+ret = ''
+if comment:
+ret += indent(level) + '/* %s */\n' % comment
+if ifcond:
+ret += gen_if(ifcond)
 ret += to_qlit(ifobj, level)
-endif = gen_endif(ifcond)
-if endif:
-ret += '\n' + endif
+if ifcond:
+ret += '\n' + gen_endif(ifcond)
 return ret

 ret = ''
@@ -137,11 +142,21 @@ const QLitObject %(c_name)s = %(c_string)s;
 return self._name(typ.name)

 def _gen_qlit(self, name, mtype, obj, ifcond):
+extra = {}
 if mtype not in ('command', 'event', 'builtin', 'array'):
+if not self._unmask:
+# Output a comment to make it easy to map masked names
+# back to the source when reading the generated output.
+extra['comment'] = '"%s" = %s' % (self._name(name), name)
 name = self._name(name)
 obj['name'] = name
 obj['meta-type'] = mtype
-self._qlits.append((obj, ifcond))
+if ifcond:
+extra['if'] = ifcond
+if extra:
+self._qlits.append((obj, extra))
+else:
+self._qlits.append(obj)

 def _gen_member(self, member):
 ret = {'name': member.name, 'type': self._use_type(member.type)}
-- 
2.17.1

[Qemu-devel] [PATCH v2 1/2] qapi: Minor introspect.py cleanups

[Eric Blake]

Commit 7d0f982b changed generated introspection output to no longer
produce long lines in the generated .c file, but failed to adjust
comments to match.  Add some clarity that the shorter length that
matters most is the overall QMP response on the wire.

Commit 25b1ef31 triggers a pep8 formatting nit.

Signed-off-by: Eric Blake 
---
 scripts/qapi/introspect.py | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 189a4edabab..43e81a06938 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -89,7 +89,6 @@ class 
QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
 for typ in self._used_types:
 typ.visit(self)
 # generate C
-# TODO can generate awfully long lines
 name = c_name(self._prefix, protect=False) + 'qmp_schema_qlit'
 self._genh.add(mcgen('''
 #include "qapi/qmp/qlit.h"
@@ -129,8 +128,8 @@ const QLitObject %(c_name)s = %(c_string)s;
 if typ not in self._used_types:
 self._used_types.append(typ)
 # Clients should examine commands and events, not types.  Hide
-# type names to reduce the temptation.  Also saves a few
-# characters.
+# type names as integers to reduce the temptation.  Also, it
+# saves a few characters on the wire.
 if isinstance(typ, QAPISchemaBuiltinType):
 return typ.name
 if isinstance(typ, QAPISchemaArrayType):
@@ -185,7 +184,7 @@ const QLitObject %(c_name)s = %(c_string)s;
 arg_type = arg_type or self._schema.the_empty_object_type
 ret_type = ret_type or self._schema.the_empty_object_type
 obj = {'arg-type': self._use_type(arg_type),
-   'ret-type': self._use_type(ret_type) }
+   'ret-type': self._use_type(ret_type)}
 if allow_oob:
 obj['allow-oob'] = allow_oob
 self._gen_qlit(name, 'command', obj, ifcond)
-- 
2.17.1

[Qemu-devel] [PATCH v2 0/2] qapi: easier debugging of introspection file

[Eric Blake]

When inspecting the generated qapi-introspect.c (for debugging,
or to see what QAPI changes are user visible vs. internal only),
the fact that we've intentionally masked names from the QMP client
makes it harder to tie back generated code back to the original
QAPI .json files.  We have a -u switch to qapi-gen for temporarily
bypassing the name masking, but that's a rather heavy-handed
tactic just for some temporary debugging.  Better is to just make
the generated file include strategic comments.

v1 was sent back in June, but was stalled due to 3.0 hard freeze.
Now that 3.1 is open, it's time to revisit this.

Since then:
- drop the original patch 2 (deleting --unmask proved controversial) [Markus]
- rebase patch on top of Marc-Andre's conditional work [Markus]
- update documentation to match code change [Eric]
- split out a preliminary cleanup patch [pep8]

Eric Blake (2):
  qapi: Minor introspect.py cleanups
  qapi: Add comments to aid debugging generated introspection

 docs/devel/qapi-code-gen.txt |  1 +
 scripts/qapi/introspect.py   | 34 --
 2 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.17.1

Re: [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event

[Eric Blake]

On 08/15/2018 08:37 AM, Peter Xu wrote:

Now it was not used any more.  Drop it, especially if we can do that
before we release QEMU 3.0.


Stale commit message, now that 3.0 has been released. But still doable, 
since the event could not be emitted without use of the experimental 
x-oob option.




Signed-off-by: Peter Xu 
---
  docs/interop/qmp-spec.txt |  5 +++--
  qapi/misc.json| 40 ---
  2 files changed, 3 insertions(+), 42 deletions(-)

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 4/4] net: Remove the deprecated -tftp, -bootp, -redir and -smb options

[Samuel Thibault]

Thomas Huth, le lun. 27 août 2018 16:54:12 +0200, a ecrit:
> These options likely do not work as expected as soon as the user
> tries to use more than one network interface at once. The parameters
> have been marked as deprecated since QEMU v2.6, so users had plenty
> of time to move their scripts to the new syntax. Time to remove the
> old parameters now.
> 
> Signed-off-by: Thomas Huth 

Reviewed-by: Samuel Thibault 

> ---
>  include/net/net.h|   3 --
>  include/net/slirp.h  |   4 --
>  net/slirp.c  | 132 
> +++
>  os-posix.c   |   8 
>  qemu-deprecated.texi |  34 -
>  qemu-options.hx  |  15 --
>  vl.c |  18 ---
>  7 files changed, 29 insertions(+), 185 deletions(-)
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index 1425960..7936d53 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -201,9 +201,6 @@ extern NICInfo nd_table[MAX_NICS];
>  extern const char *host_net_devices[];
>  
>  /* from net.c */
> -extern const char *legacy_tftp_prefix;
> -extern const char *legacy_bootp_filename;
> -
>  int net_client_parse(QemuOptsList *opts_list, const char *str);
>  int net_init_clients(Error **errp);
>  void net_check_clients(void);
> diff --git a/include/net/slirp.h b/include/net/slirp.h
> index 4d63d74..bad3e1e 100644
> --- a/include/net/slirp.h
> +++ b/include/net/slirp.h
> @@ -30,10 +30,6 @@
>  void hmp_hostfwd_add(Monitor *mon, const QDict *qdict);
>  void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict);
>  
> -int net_slirp_redir(const char *redir_str);
> -
> -int net_slirp_smb(const char *exported_dir);
> -
>  void hmp_info_usernet(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/net/slirp.c b/net/slirp.c
> index 1e14318..c18060f 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -67,13 +67,11 @@ static int get_str_sep(char *buf, int buf_size, const 
> char **pp, int sep)
>  /* slirp network adapter */
>  
>  #define SLIRP_CFG_HOSTFWD 1
> -#define SLIRP_CFG_LEGACY  2
>  
>  struct slirp_config_str {
>  struct slirp_config_str *next;
>  int flags;
>  char str[1024];
> -int legacy_format;
>  };
>  
>  typedef struct SlirpState {
> @@ -87,19 +85,13 @@ typedef struct SlirpState {
>  } SlirpState;
>  
>  static struct slirp_config_str *slirp_configs;
> -const char *legacy_tftp_prefix;
> -const char *legacy_bootp_filename;
>  static QTAILQ_HEAD(slirp_stacks, SlirpState) slirp_stacks =
>  QTAILQ_HEAD_INITIALIZER(slirp_stacks);
>  
> -static int slirp_hostfwd(SlirpState *s, const char *redir_str,
> - int legacy_format, Error **errp);
> -static int slirp_guestfwd(SlirpState *s, const char *config_str,
> -  int legacy_format, Error **errp);
> +static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp);
> +static int slirp_guestfwd(SlirpState *s, const char *config_str, Error 
> **errp);
>  
>  #ifndef _WIN32
> -static const char *legacy_smb_export;
> -
>  static int slirp_smb(SlirpState *s, const char *exported_dir,
>   struct in_addr vserver_addr, Error **errp);
>  static void slirp_smb_cleanup(SlirpState *s);
> @@ -196,13 +188,6 @@ static int net_slirp_init(NetClientState *peer, const 
> char *model,
>  return -1;
>  }
>  
> -if (!tftp_export) {
> -tftp_export = legacy_tftp_prefix;
> -}
> -if (!bootfile) {
> -bootfile = legacy_bootp_filename;
> -}
> -
>  if (vnetwork) {
>  if (get_str_sep(buf, sizeof(buf), , '/') < 0) {
>  if (!inet_aton(vnetwork, )) {
> @@ -382,21 +367,16 @@ static int net_slirp_init(NetClientState *peer, const 
> char *model,
>  
>  for (config = slirp_configs; config; config = config->next) {
>  if (config->flags & SLIRP_CFG_HOSTFWD) {
> -if (slirp_hostfwd(s, config->str,
> -  config->flags & SLIRP_CFG_LEGACY, errp) < 0) {
> +if (slirp_hostfwd(s, config->str, errp) < 0) {
>  goto error;
>  }
>  } else {
> -if (slirp_guestfwd(s, config->str,
> -   config->flags & SLIRP_CFG_LEGACY, errp) < 0) {
> +if (slirp_guestfwd(s, config->str, errp) < 0) {
>  goto error;
>  }
>  }
>  }
>  #ifndef _WIN32
> -if (!smb_export) {
> -smb_export = legacy_smb_export;
> -}
>  if (smb_export) {
>  if (slirp_smb(s, smb_export, smbsrv, errp) < 0) {
>  goto error;
> @@ -506,8 +486,7 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
>  monitor_printf(mon, "invalid format\n");
>  }
>  
> -static int slirp_hostfwd(SlirpState *s, const char *redir_str,
> - int legacy_format, Error **errp)
> +static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
>  {
>  struct in_addr host_addr = { .s_addr = INADDR_ANY };

Re: [Qemu-devel] [PATCH v2 8/9] jobs: remove ret argument to job_completed; privatize it

[John Snow]

On 08/27/2018 06:52 AM, Max Reitz wrote:
> On 2018-08-24 00:08, John Snow wrote:
>> Jobs are now expected to return their retcode on the stack, from the
>> .run callback, so we can remove that argument.
>>
>> job_cancel does not need to set -ECANCELED because job_completed will
>> update the return code itself if the job was canceled.
>>
>> While we're here, make job_completed static to job.c and remove it from
>> job.h; move the documentation of return code to the .run() callback and
>> to the job->ret property, accordingly.
>>
>> Signed-off-by: John Snow 
>> ---
>>  include/qemu/job.h | 24 +++-
>>  job.c  | 11 ++-
>>  trace-events   |  2 +-
>>  3 files changed, 18 insertions(+), 19 deletions(-)
> 
> Er, yeah.  Sorry for not being able to read.  Again.
> 
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index c67f6a647e..2990f28edc 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -124,7 +124,7 @@ typedef struct Job {
>>  /** Estimated progress_current value at the completion of the job */
>>  int64_t progress_total;
>>  
>> -/** ret code passed to job_completed. */
>> +/** Return code from @run callback; 0 on success and -errno on failure. 
>> */
> 
> Hm.  Not really, it's the general status of the whole job, isn't it?
> Besides being the return value from .run(), it's also set by .exit() (so
> it's presumably going to be the return value from .prepare() after part
> 2) and by job_update_rc() when the job has been cancelled.
> 

You're right. I was trying to emphasize where it gets set in the
normative case. I'll rephrase.

What I want to say is effectively: "This is the return code for the job,
which is what gets returned by the .run and/or .prepare callbacks, or
gets set to -ECANCELED if the job is canceled and the job itself
neglects to set a nonzero code."

>>  int ret;
>>  
>>  /** Error object for a failed job **/
>> @@ -168,7 +168,16 @@ struct JobDriver {
>>  /** Enum describing the operation */
>>  JobType job_type;
>>  
>> -/** Mandatory: Entrypoint for the Coroutine. */
>> +/**
>> + * Mandatory: Entrypoint for the Coroutine.
>> + *
>> + * This callback will be invoked when moving from CREATED to RUNNING.
>> + *
>> + * If this callback returns nonzero, the job transaction it is part of 
>> is
>> + * aborted. If it returns zero, the job moves into the WAITING state. 
>> If it
>> + * is the last job to complete in its transaction, all jobs in the
>> + * transaction move from WAITING to PENDING.
>> + */
> 
> Moving this description from job_completed() seems to imply we do want
> to call job_update_rc() right after invoking .run().
> 

Sure, I'll take a look at that.

>>  int coroutine_fn (*run)(Job *job, Error **errp);
>>  
>>  /**
>> @@ -492,17 +501,6 @@ void job_early_fail(Job *job);
>>  /** Moves the @job from RUNNING to READY */
>>  void job_transition_to_ready(Job *job);
>>  
>> -/**
>> - * @job: The job being completed.
>> - * @ret: The status code.
>> - *
>> - * Marks @job as completed. If @ret is non-zero, the job transaction it is 
>> part
>> - * of is aborted. If @ret is zero, the job moves into the WAITING state. If 
>> it
>> - * is the last job to complete in its transaction, all jobs in the 
>> transaction
>> - * move from WAITING to PENDING.
>> - */
>> -void job_completed(Job *job, int ret);
>> -
>>  /** Asynchronously complete the specified @job. */
>>  void job_complete(Job *job, Error **errp);
>>  
>> diff --git a/job.c b/job.c
>> index bc8dad4e71..213042b762 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -535,6 +535,8 @@ void job_drain(Job *job)
>>  }
>>  }
>>  
>> +static void job_completed(Job *job);
>> +
>>  static void job_exit(void *opaque)
>>  {
>>  Job *job = (Job *)opaque;
>> @@ -545,7 +547,7 @@ static void job_exit(void *opaque)
>>  job->driver->exit(job);
>>  aio_context_release(aio_context);
>>  }
>> -job_completed(job, job->ret);
>> +job_completed(job);
>>  }
>>  
>>  /**
>> @@ -883,13 +885,12 @@ static void job_completed_txn_success(Job *job)
>>  }
>>  }
>>  
>> -void job_completed(Job *job, int ret)
>> +static void job_completed(Job *job)
>>  {
>>  assert(job && job->txn && !job_is_completed(job));
>>  
>> -job->ret = ret;
>>  job_update_rc(job);
> 
> I think we want to remove the job_update_rc() from here.  It should be
> called after job->ret is updated, i.e. immediately after .run() and
> .exit() have been invoked.  (Or presumably .prepare() in part 2.)
> Oh, and in job_cancel() before it invokes job_completed()?
> 
> But then again, maybe it would be easiest to keep it here...  It just
> doesn't feel quite right to me.
> 
> Max
> 

It does feel slightly strange now. I'll see if I can find something that
feels better.

>> -trace_job_completed(job, ret, job->ret);
>> +trace_job_completed(job, job->ret);
>>  if (job->ret) {
>>  

[Qemu-devel] [PATCH v2 0/2] chardev: Add websocket support

[Julia Suvorova via Qemu-devel]

v2:
* Fixed initialization order [Daniel]
* Function arguments refactoring [Paolo]
* Added test [Stefan]
* Added meaningful error message [Stefan]
* Added "websock:" URI prefix support

Julia Suvorova (2):
  chardev: Add websocket support
  tests/test-char: Check websocket chardev functionality

 chardev/char-socket.c | 124 ++---
 chardev/char.c|   8 ++-
 qapi/char.json|   3 +
 tests/test-char.c | 125 ++
 4 files changed, 228 insertions(+), 32 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality

[Julia Suvorova via Qemu-devel]

Test order:
Creating server websocket chardev
Creating usual tcp chardev client
Sending handshake message from client
Receiving handshake reply
Sending ping frame with "hello" payload
Receiving pong reply
Sending binary data "world"
Checking the received data on server side
Checking of closing handshake

Signed-off-by: Julia Suvorova 
---
 tests/test-char.c | 125 ++
 1 file changed, 125 insertions(+)

diff --git a/tests/test-char.c b/tests/test-char.c
index 5905d31441..deba699b25 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -406,6 +406,130 @@ static void char_socket_fdpass_test(void)
 }
 
 
+static void websock_server_read(void *opaque, const uint8_t *buf, int size)
+{
+g_assert(size == 5);
+g_assert(strncmp((char *) buf, "world", size) == 0);
+quit = true;
+}
+
+
+static int websock_server_can_read(void *opaque)
+{
+return 10;
+}
+
+
+static bool websock_check_http_headers(char *buf, int size)
+{
+int i;
+const char *ans[] = { "HTTP/1.1 101 Switching Protocols\r\n",
+  "Server: QEMU VNC\r\n",
+  "Upgrade: websocket\r\n",
+  "Connection: Upgrade\r\n",
+  "Sec-WebSocket-Accept:",
+  "Sec-WebSocket-Protocol: binary\r\n" };
+
+for (i = 0; i < 6; i++) {
+if (g_strstr_len(buf, size, ans[i]) == NULL) {
+return false;
+}
+}
+
+return true;
+}
+
+
+static void websock_client_read(void *opaque, const uint8_t *buf, int size)
+{
+const uint8_t ping[] = { 0x89, 0x85,  /* Ping header */
+ 0x07, 0x77, 0x9e, 0xf9,  /* Masking key */
+ 0x6f, 0x12, 0xf2, 0x95, 0x68 /* "hello" */ };
+
+const uint8_t binary[] = { 0x82, 0x85,  /* Binary header */
+   0x74, 0x90, 0xb9, 0xdf,  /* Masking key */
+   0x03, 0xff, 0xcb, 0xb3, 0x10 /* "world" */ };
+Chardev *chr_client = opaque;
+
+if (websock_check_http_headers((char *) buf, size)) {
+qemu_chr_fe_write(chr_client->be, ping, sizeof(ping));
+} else if (buf[0] == 0x8a && buf[1] == 0x05) {
+g_assert(strncmp((char *) buf + 2, "hello", 5) == 0);
+qemu_chr_fe_write(chr_client->be, binary, sizeof(binary));
+} else {
+g_assert(buf[0] == 0x88 && buf[1] == 0x16);
+g_assert(strncmp((char *) buf + 4, "peer requested close", 10) == 0);
+quit = true;
+}
+}
+
+
+static int websock_client_can_read(void *opaque)
+{
+return 4096;
+}
+
+
+static void char_websock_test(void)
+{
+QObject *addr;
+QDict *qdict;
+const char *port;
+char *tmp;
+char *handshake_port;
+CharBackend be;
+CharBackend client_be;
+Chardev *chr_client;
+Chardev *chr = qemu_chr_new("server",
+"websock:127.0.0.1:0,server,nowait");
+const char handshake[] = "GET / HTTP/1.1\r\n"
+ "Upgrade: websocket\r\n"
+ "Connection: Upgrade\r\n"
+ "Host: localhost:%s\r\n"
+ "Origin: http://localhost:%s\r\n;
+ "Sec-WebSocket-Key: o9JHNiS3/0/0zYE1wa3yIw==\r\n"
+ "Sec-WebSocket-Version: 13\r\n"
+ "Sec-WebSocket-Protocol: binary\r\n\r\n";
+const uint8_t close[] = { 0x88, 0x82, /* Close header */
+  0xef, 0xaa, 0xc5, 0x97, /* Masking key */
+  0xec, 0x42  /* Status code */ };
+
+addr = object_property_get_qobject(OBJECT(chr), "addr", _abort);
+qdict = qobject_to(QDict, addr);
+port = qdict_get_str(qdict, "port");
+tmp = g_strdup_printf("tcp:127.0.0.1:%s", port);
+handshake_port = g_strdup_printf(handshake, port, port);
+qobject_unref(qdict);
+
+qemu_chr_fe_init(, chr, _abort);
+qemu_chr_fe_set_handlers(, websock_server_can_read, websock_server_read,
+ NULL, NULL, chr, NULL, true);
+
+chr_client = qemu_chr_new("client", tmp);
+qemu_chr_fe_init(_be, chr_client, _abort);
+qemu_chr_fe_set_handlers(_be, websock_client_can_read,
+ websock_client_read,
+ NULL, NULL, chr_client, NULL, true);
+g_free(tmp);
+
+qemu_chr_write_all(chr_client,
+   (uint8_t *) handshake_port,
+   strlen(handshake_port));
+g_free(handshake_port);
+main_loop();
+
+g_assert(object_property_get_bool(OBJECT(chr), "connected", _abort));
+g_assert(object_property_get_bool(OBJECT(chr_client),
+  "connected", _abort));
+
+qemu_chr_write_all(chr_client, close, sizeof(close));
+main_loop();
+
+

[Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support

[Julia Suvorova via Qemu-devel]

New option "websock" added to allow using websocket protocol for
chardev socket backend.
Example:
-chardev socket,websock,id=...

Signed-off-by: Julia Suvorova 
---
 chardev/char-socket.c | 124 +++---
 chardev/char.c|   8 ++-
 qapi/char.json|   3 +
 3 files changed, 103 insertions(+), 32 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index efbad6ee7c..1d3c14d85d 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -26,6 +26,7 @@
 #include "chardev/char.h"
 #include "io/channel-socket.h"
 #include "io/channel-tls.h"
+#include "io/channel-websock.h"
 #include "io/net-listener.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
@@ -69,6 +70,8 @@ typedef struct {
 GSource *telnet_source;
 TCPChardevTelnetInit *telnet_init;
 
+bool is_websock;
+
 GSource *reconnect_timer;
 int64_t reconnect_time;
 bool connect_err_reported;
@@ -385,30 +388,37 @@ static void tcp_chr_free_connection(Chardev *chr)
 s->connected = 0;
 }
 
-static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
-  bool is_listen, bool is_telnet)
+static const char *qemu_chr_socket_protocol(SocketChardev *s)
+{
+if (s->is_telnet) {
+return "telnet";
+}
+return s->is_websock ? "websock" : "tcp";
+}
+
+static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
 {
-switch (addr->type) {
+switch (s->addr->type) {
 case SOCKET_ADDRESS_TYPE_INET:
 return g_strdup_printf("%s%s:%s:%s%s", prefix,
-   is_telnet ? "telnet" : "tcp",
-   addr->u.inet.host,
-   addr->u.inet.port,
-   is_listen ? ",server" : "");
+   qemu_chr_socket_protocol(s),
+   s->addr->u.inet.host,
+   s->addr->u.inet.port,
+   s->is_listen ? ",server" : "");
 break;
 case SOCKET_ADDRESS_TYPE_UNIX:
 return g_strdup_printf("%sunix:%s%s", prefix,
-   addr->u.q_unix.path,
-   is_listen ? ",server" : "");
+   s->addr->u.q_unix.path,
+   s->is_listen ? ",server" : "");
 break;
 case SOCKET_ADDRESS_TYPE_FD:
-return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
-   is_listen ? ",server" : "");
+return g_strdup_printf("%sfd:%s%s", prefix, s->addr->u.fd.str,
+   s->is_listen ? ",server" : "");
 break;
 case SOCKET_ADDRESS_TYPE_VSOCK:
 return g_strdup_printf("%svsock:%s:%s", prefix,
-   addr->u.vsock.cid,
-   addr->u.vsock.port);
+   s->addr->u.vsock.cid,
+   s->addr->u.vsock.port);
 default:
 abort();
 }
@@ -419,8 +429,7 @@ static void update_disconnected_filename(SocketChardev *s)
 Chardev *chr = CHARDEV(s);
 
 g_free(chr->filename);
-chr->filename = SocketAddress_to_str("disconnected:", s->addr,
- s->is_listen, s->is_telnet);
+chr->filename = qemu_chr_socket_address(s, "disconnected:");
 }
 
 /* NB may be called even if tcp_chr_connect has not been
@@ -506,10 +515,12 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t 
*buf, int len)
 return size;
 }
 
-static char *sockaddr_to_str(struct sockaddr_storage *ss, socklen_t ss_len,
- struct sockaddr_storage *ps, socklen_t ps_len,
- bool is_listen, bool is_telnet)
+static char *qemu_chr_compute_filename(SocketChardev *s)
 {
+struct sockaddr_storage *ss = >sioc->localAddr;
+struct sockaddr_storage *ps = >sioc->remoteAddr;
+socklen_t ss_len = s->sioc->localAddrLen;
+socklen_t ps_len = s->sioc->remoteAddrLen;
 char shost[NI_MAXHOST], sserv[NI_MAXSERV];
 char phost[NI_MAXHOST], pserv[NI_MAXSERV];
 const char *left = "", *right = "";
@@ -519,7 +530,7 @@ static char *sockaddr_to_str(struct sockaddr_storage *ss, 
socklen_t ss_len,
 case AF_UNIX:
 return g_strdup_printf("unix:%s%s",
((struct sockaddr_un *)(ss))->sun_path,
-   is_listen ? ",server" : "");
+   s->is_listen ? ",server" : "");
 #endif
 case AF_INET6:
 left  = "[";
@@ -531,9 +542,9 @@ static char *sockaddr_to_str(struct sockaddr_storage *ss, 
socklen_t ss_len,
 getnameinfo((struct sockaddr *) ps, ps_len, phost, sizeof(phost),
 pserv, sizeof(pserv), NI_NUMERICHOST | NI_NUMERICSERV);
 return g_strdup_printf("%s:%s%s%s:%s%s <-> %s%s%s:%s",
-   is_telnet ? 

Re: [Qemu-devel] [PATCH v2 1/6] target/mips: Add MXU instructions S32I2M and S32M2I

[Aleksandar Markovic]

> From: Craig Janeczek 
> Sent: Monday, August 27, 2018 4:38 PM
> Subject: [PATCH v2 1/6] target/mips: Add MXU instructions S32I2M and S32M2I
> 
> This commit makes the MXU registers and the utility functions for
> reading/writing to them. This is required for full MXU instruction
> support.
> 
> Adds support for emulating the S32I2M and S32M2I MXU instructions.
> 
> Signed-off-by: Craig Janeczek 
> ---
>  v1
> - initial patch
>  v2
> - Fix checkpatch.pl errors
> - remove mips64 ifdef
> - changed bitfield usage to extract32
> - squashed register addition patch into this one
> 
>  target/mips/cpu.h   |  1 +
>  target/mips/translate.c | 71 +
>  2 files changed, 72 insertions(+)
> 

Hi, Craig,

So far it is going well. You respond well to the reviews. I encourage you to 
persevere, this is a nice addition to QEMU's MIPS functionality, we want it.

Just something about overall patch organization:

This (1/7) patch should be split/refactored in this way:

PATCH 1: Introduce MXU registers (16 fields and their names, plus 
initialization)

PATCH 2: Add MXU opcodes (if possible, add ALL MXU opcodes, there are 50 to 60 
ones, if I am not mistaken)

PATCH 3: The rest of this patch (so, S32I2M and S32M2I emulation)

... and further patches as you already organized.

However, there is more. We plan to significantly revamp translate.c in near 
future, and this series, even though it is consistent with the rest of code in 
the same file, would not fit in its present state well into the new 
organization of translate.c. I am asking you, therefore, to reorganize code in 
the following fashion:

- implementation of emulation of each instruction should be in a separate 
function (baring some exceptions); for this patch functions gen_mxu_s32i2m() 
and gen_mxu_s32m2i() should be created; the only arguments of these function 
should be "DisasContext *ctx" and "uint32_t opc";

- each such function should be preceded by a comment similar to this:

/*
 * S32M2I XRa, rb - Register move from XRF to GRF
 */

- gen_mxu() would disappear;

- decode_opc_special2_legacy() should call various gen_mxu_XXX() directly, in 
separate "case" sections for each instruction.

Please rearrange the code this way.

Thanks,
Aleksandar

> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 009202cf64..4b2948a2c8 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -170,6 +170,7 @@ struct TCState {
>  MSACSR_FS_MASK)
> 
>  float_status msa_fp_status;
> +target_ulong mxu_gpr[16];
>  };
> 
>  typedef struct CPUMIPSState CPUMIPSState;
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index bdd880bb77..ef819d67e0 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -364,6 +364,9 @@ enum {
>  OPC_CLO  = 0x21 | OPC_SPECIAL2,
>  OPC_DCLZ = 0x24 | OPC_SPECIAL2,
>  OPC_DCLO = 0x25 | OPC_SPECIAL2,
> +/* MXU */
> +OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
> +OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
>  /* Special */
>  OPC_SDBBP= 0x3F | OPC_SPECIAL2,
>  };
> @@ -1398,6 +1401,9 @@ static TCGv_i32 fpu_fcr0, fpu_fcr31;
>  static TCGv_i64 fpu_f64[32];
>  static TCGv_i64 msa_wr_d[64];
> 
> +/* MXU registers */
> +static TCGv mxu_gpr[16];
> +
>  #include "exec/gen-icount.h"
> 
>  #define gen_helper_0e0i(name, arg) do {   \
> @@ -1517,6 +1523,13 @@ static const char * const msaregnames[] = {
>  "w30.d0", "w30.d1", "w31.d0", "w31.d1",
>  };
> 
> +static const char * const mxuregnames[] = {
> +"XR1",  "XR2",  "XR3",  "XR4",  "XR5",
> +"XR6",  "XR7",  "XR8",  "XR9",  "XR10",
> +"XR11", "XR12", "XR13", "XR14", "XR15",
> +"XR16",
> +};
> +
>  #define LOG_DISAS(...)   
>  \
>  do { 
>  \
>  if (MIPS_DEBUG_DISAS) {  
>  \
> @@ -1550,6 +1563,23 @@ static inline void gen_store_gpr (TCGv t, int reg)
>  tcg_gen_mov_tl(cpu_gpr[reg], t);
>  }
> 
> +/* MXU General purpose registers moves. */
> +static inline void gen_load_mxu_gpr(TCGv t, int reg)
> +{
> +if (reg == 0) {
> +tcg_gen_movi_tl(t, 0);
> +} else {
> +tcg_gen_mov_tl(t, mxu_gpr[reg - 1]);
> +}
> +}
> +
> +static inline void gen_store_mxu_gpr(TCGv t, int reg)
> +{
> +if (reg != 0) {
> +tcg_gen_mov_tl(mxu_gpr[reg - 1], t);
> +}
> +}
> +
>  /* Moves to/from shadow registers. */
>  static inline void gen_load_srsgpr (int from, int to)
>  {
> @@ -3738,6 +3768,35 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
>  }
>  }
> 
> +/* MXU Instructions */
> +static void gen_mxu(DisasContext *ctx, uint32_t opc)
> +{
> +TCGv t0;
> +uint32_t xra, rb;
> +
> +t0 = tcg_temp_new();
> +
> +switch (opc) {
> +case OPC_MXU_S32I2M:
> +xra = extract32(ctx->opcode, 6, 

[Qemu-devel] Hints for a networkable console

[Vincenzo Romano]

I'd like to work on an idea that could be explained with the examples below.

-nographic \
-chardev socket,id=console,host=127.1.2.3,port=1234,server \
-display curses,chardev=console

and

-nographic \
-chardev socket,id=console,path=/tmp/asocket,server \
-display curses,chardev=console

This would allow for a "networkable" character-based console,
as opposed to an "image-oriented" one like VNC.

I have already given a look to some code (mainly in vl.c) and
I'd like to avoid reverse-engineering the code structure.
Where can I find some details about the code structure and maybe
a general overview?

--
Vincenzo Romano - NotOrAnd.IT
Information Technologies
--
NON QVIETIS MARIBVS NAVTA PERITVS

Re: [Qemu-devel] [PATCH 5/6] json: Eliminate lexer state IN_ERROR

[Eric Blake]

On 08/27/2018 02:00 AM, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 
---
  qobject/json-lexer.c  | 9 +
  qobject/json-parser-int.h | 8 
  2 files changed, 9 insertions(+), 8 deletions(-)



-
  typedef enum json_token_type {
-JSON_MIN = 100,
-JSON_LCURLY = JSON_MIN,
+JSON_ERROR = 0, /* must be zero, see json_lexer[] */
+/* Gap for lexer states */
+JSON_LCURLY = 100,
+JSON_MIN = JSON_LCURLY,


In an earlier version of this type of cleanup, you swapped the IN_ and 
JSON_ values and eliminated the gap, to make the overall table more 
compact (no storage wasted on any of the states in the gap between the two).


https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg01178.html

Is it still worth trying to minimize the gap between the two sequences, 
even if you now no longer swap them in order?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] json: Eliminate lexer state IN_WHITESPACE, pseudo-token JSON_SKIP

[Eric Blake]

On 08/27/2018 02:00 AM, Markus Armbruster wrote:

The lexer ignores whitespace like this:

  on whitespace  on non-ws   spontaneously
 IN_START --> IN_WHITESPACE --> JSON_SKIP --> IN_START
 ^|
  \__/  on whitespace

This accumulates a whitespace token in state IN_WHITESPACE, only to
throw it away on the transition via JSON_SKIP to the start state.
Wasteful.  Go from IN_START to IN_START on whitspace directly,


s/whitspace/whitespace/


dropping the whitespace character.

Signed-off-by: Markus Armbruster 
---
  qobject/json-lexer.c  | 22 +-
  qobject/json-parser-int.h |  1 -
  2 files changed, 5 insertions(+), 18 deletions(-)

@@ -263,10 +253,10 @@ static const uint8_t json_lexer[][256] =  {
  [','] = JSON_COMMA,
  [':'] = JSON_COLON,
  ['a' ... 'z'] = IN_KEYWORD,
-[' '] = IN_WHITESPACE,
-['\t'] = IN_WHITESPACE,
-['\r'] = IN_WHITESPACE,
-['\n'] = IN_WHITESPACE,
+[' '] = IN_START,
+['\t'] = IN_START,
+['\r'] = IN_START,
+['\n'] = IN_START,
  },
  [IN_START_INTERP]['%'] = IN_INTERP,


Don't you need to set [IN_START_INTERP][' '] to IN_START_INTERP, rather 
than IN_START?  Otherwise, the presence of skipped whitespace would 
change whether interpolation happens.  (At least, that's what you had in 
an earlier version of this patch).



  };
@@ -323,10 +313,8 @@ static void json_lexer_feed_char(JSONLexer *lexer, char 
ch, bool flush)
  json_message_process_token(lexer, lexer->token, new_state,
 lexer->x, lexer->y);
  /* fall through */
-case JSON_SKIP:
-g_string_truncate(lexer->token, 0);
-/* fall through */
  case IN_START:
+g_string_truncate(lexer->token, 0);
  new_state = lexer->start_state;


Oh, I see. We are magically reverting to the correct start state if the 
requested transition reports IN_START, rather than blindly using IN_START.


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 4/6] json: Nicer recovery from lexical errors

[Eric Blake]

On 08/27/2018 02:00 AM, Markus Armbruster wrote:

When the lexer chokes on an input character, it consumes the
character, emits a JSON error token, and enters its start state.  This
can lead to suboptimal error recovery.  For instance, input

 0123 ,

produces the tokens

 JSON_ERROR01
 JSON_INTEGER  23
 JSON_COMMA,

Make the lexer skip characters after a lexical error until a
structural character ('[', ']', '{', '}', ':', ','), an ASCII control
character, or '\xFE', or '\xFF'.

Note that we must not skip ASCII control characters, '\xFE', '\xFF',
because those are documented to force the JSON parser into known-good
state, by docs/interop/qmp-spec.txt.

The lexer now produces

 JSON_ERROR01
 JSON_COMMA,


So the lexer has now completely skipped the intermediate input, but the 
resulting error message need only point at the start of where input went 
wrong, and skipping to a sane point results in fewer error tokens to be 
reported.  Makes sense.




Update qmp-test for the nicer error recovery: QMP now report just one


s/report/reports/


error for input %p instead of two.  Also drop the newline after %p; it
was needed to tease out the second error.


That's because pre-patch, 'p' is one of the input characters that 
requires lookahead to determine if it forms a complete token (and the 
newline provides the transition needed to consume it); now post-patch, 
the 'p' is consumed as part of the junk after the error is first 
detected at the '%'.


And to my earlier complaint about 0x1a resulting in JSON_ERROR then 
JSON_INTEGER then JSON_KEYWORD, that sequence is likewise now identified 
as a single JSON_ERROR at the 'x', with the rest of the attempted hex 
number (invalid in JSON) silently skipped.  Nice.




Signed-off-by: Markus Armbruster 
---
  qobject/json-lexer.c | 43 +--
  tests/qmp-test.c |  6 +-
  2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 28582e17d9..39c7ce7adc 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -101,6 +101,7 @@
  
  enum json_lexer_state {

  IN_ERROR = 0,   /* must really be 0, see json_lexer[] */
+IN_RECOVERY,
  IN_DQ_STRING_ESCAPE,
  IN_DQ_STRING,
  IN_SQ_STRING_ESCAPE,
@@ -130,6 +131,28 @@ QEMU_BUILD_BUG_ON(IN_START_INTERP != IN_START + 1);
  static const uint8_t json_lexer[][256] =  {
  /* Relies on default initialization to IN_ERROR! */
  
+/* error recovery */

+[IN_RECOVERY] = {
+/*
+ * Skip characters until a structural character, an ASCII
+ * control character other than '\t', or impossible UTF-8
+ * bytes '\xFE', '\xFF'.  Structural characters and line
+ * endings are promising resynchronization points.  Clients
+ * may use the others to force the JSON parser into known-good
+ * state; see docs/interop/qmp-spec.txt.
+ */
+[0 ... 0x1F] = IN_START | LOOKAHEAD,


And here's where the LOOKAHEAD bit has to be separate, because you are 
now assigning semantics to the transition on '\0' that are distinct from 
either of the two roles previously enumerated as possible.



+[0x20 ... 0xFD] = IN_RECOVERY,
+[0xFE ... 0xFF] = IN_START | LOOKAHEAD,
+['\t'] = IN_RECOVERY,
+['['] = IN_START | LOOKAHEAD,
+[']'] = IN_START | LOOKAHEAD,
+['{'] = IN_START | LOOKAHEAD,
+['}'] = IN_START | LOOKAHEAD,
+[':'] = IN_START | LOOKAHEAD,
+[','] = IN_START | LOOKAHEAD,
+},


It took me a couple of reads before I was satisfied that everything is 
initialized as desired (range assignments followed by more-specific 
re-assignment works, but isn't common), but this looks right.


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [Qemu-ppc] [PATCH] 40p: fix PCI interrupt routing

[BALATON Zoltan]

On Mon, 27 Aug 2018, Mark Cave-Ayland wrote:

According to the PReP specification section 6.1.6 "System Interrupt
Assignments", all PCI interrupts are routed via IRQ 15.

With this patch applied it is now possible to boot the sandalfoot
zImage all the way through to a working userspace when using
OpenBIOS.

Signed-off-by: Mark Cave-Ayland 
---
hw/ppc/prep.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 162b27a3b8..e82c1355d9 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -668,10 +668,11 @@ static void ibm_40p_init(MachineState *machine)
dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(11, 0), "i82378"));
qdev_connect_gpio_out(dev, 0,
  cpu->env.irq_inputs[PPC6xx_INPUT_INT]);
-sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(dev, 15));
-sysbus_connect_irq(pcihost, 1, qdev_get_gpio_in(dev, 13));
-sysbus_connect_irq(pcihost, 2, qdev_get_gpio_in(dev, 15));
-sysbus_connect_irq(pcihost, 3, qdev_get_gpio_in(dev, 13));
+/* According to PReP specification section 6.1.6 "System Interrupt
+ * Assignments", all PCI interrupts are routed via IRQ 15 */
+for (i = 0; i < PCI_NUM_PINS; i++) {
+sysbus_connect_irq(pcihost, i, qdev_get_gpio_in(dev, 15));
+}


I'm not sure but this looks similar to what we had with sam460ex:

http://lists.nongnu.org/archive/html/qemu-ppc/2018-07/msg00359.html

I think you may not connect multiple interrupts to the same host irq line 
this way but you either need an OR gate or handle it within the mapping in 
the PCI host model (which is what we ended up with for the sam460ex). 
Peter's suggestion was to do whichever matches real hardware the most if 
you can find out that (as noted here also with more explanation that could 
be useful):


http://lists.nongnu.org/archive/html/qemu-ppc/2018-07/msg00360.html

But I could be mistaken in this case, haven't checked it in detail.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 5/6] json: Eliminate lexer state IN_ERROR

[Eric Blake]

On 08/27/2018 02:00 AM, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 
---
  qobject/json-lexer.c  | 9 +
  qobject/json-parser-int.h | 8 
  2 files changed, 9 insertions(+), 8 deletions(-)


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/6] json: Make lexer's "character consumed" logic less confusing

[Eric Blake]

On 08/27/2018 02:00 AM, Markus Armbruster wrote:

The lexer uses macro TERMINAL_NEEDED_LOOKAHEAD() to decide whether a
state transition consumes the input character.  It returns true when
the state transition is defined with the TERMINAL() macro.  To detect
that, it checks whether input '\0' would have resulted in the same
state transition, and the new state is not IN_ERROR.

Why does that even work?  For all states, the new state on input '\0'
is either IN_ERROR or defined with TERMINAL().  If the state
transition equals the one we'd get for input '\0', it goes to IN_ERROR
or to the argument of TERMINAL().  We never use TERMINAL(IN_ERROR),
because it makes no sense.  Thus, if it doesn't go to IN_ERROR, it
must be defined with TERMINAL().

Since this isn't quite confusing enough, we negate the result to get
@char_consumed, and ignore it when @flush is true.

Instead of deriving the lookahead bit from the state transition, make
it explicit.  This is easier to understand, and a bit more flexible,
too.

Signed-off-by: Markus Armbruster 
---
  qobject/json-lexer.c  | 27 ---
  qobject/json-parser-int.h |  1 +
  2 files changed, 17 insertions(+), 11 deletions(-)



Good description of the convoluted logic. And yes, I find the new 
version more legible, even if more lines of code.


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/6] json: Clean up how lexer consumes "end of input"

[Eric Blake]

On 08/27/2018 02:00 AM, Markus Armbruster wrote:

When the lexer isn't in its start state at the end of input, it's
working on a token.  To flush it out, it needs to transit to its start
state on "end of input" lookahead.

There are two ways to the start state, depending on the current state:

* If the lexer is in a TERMINAL(JSON_FOO) state, it can emit a
   JSON_FOO token.

* Else, it can go to IN_ERROR state, and emit a JSON_ERROR token.

There are complications, however:

* The transition to IN_ERROR state consumes the input character and
   adds it to the JSON_ERROR token.  The latter is inappropriate for
   the "end of input" character, so we suppress that.  See also recent
   commit "json: Fix lexer to include the bad character in JSON_ERROR
   token".


Now commit a2ec6be7



* The transition to a TERMINAL(JSON_FOO) state doesn't consume the
   input character.  In that case, the lexer normally loops until it is
   consumed.  We have to suppress that for the "end of input" input
   character.  If we didn't, the lexer would consume it by entering
   IN_ERROR state, emitting a bogus JSON_ERROR token.  We fixed that in
   commit bd3924a33a6.

However, simply breaking the loop this way assumes that the lexer
needs exactly one state transition to reach its start state.  That
assumption is correct now, but it's unclean, and I'll soon break it.
Clean up: instead of breaking the loop after one iteration, break it
after it reached the start state.

Signed-off-by: Markus Armbruster 
---
  qobject/json-lexer.c | 17 +
  1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 4867839f66..ec3aec726f 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -261,7 +261,8 @@ void json_lexer_init(JSONLexer *lexer, bool 
enable_interpolation)
  
  static void json_lexer_feed_char(JSONLexer *lexer, char ch, bool flush)

  {
-int char_consumed, new_state;
+int new_state;
+bool char_consumed = false;


Yay for the switch to bool.

Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PULL v2 0/3] Error reporting patches for 2018-08-27

[Peter Maydell]

On 27 August 2018 at 14:48, Markus Armbruster  wrote:
> The following changes since commit 235c82acca0491465e94be3cae2583b42d37c859:
>
>   Merge remote-tracking branch 'remotes/otubo/tags/pull-seccomp-20180823' 
> into staging (2018-08-25 13:08:57 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-error-2018-08-27-v2
>
> for you to fetch changes up to 4e4abd111a2af0179a4467368d695958844bf113:
>
>   intel-iommu: replace more vtd_err_* traces (2018-08-27 15:09:20 +0200)
>
> 
> Error reporting patches for 2018-08-27
>
> * Provide error_report_once(), along with first users
>
> 
> Peter Xu (3):
>   qemu-error: introduce {error|warn}_report_once
>   intel-iommu: start to use error_report_once
>   intel-iommu: replace more vtd_err_* traces

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH 1/6] json: Fix lexer for lookahead character beyond '\x7F'

[Eric Blake]

On 08/27/2018 02:00 AM, Markus Armbruster wrote:

The lexer fails to end a valid token when the lookahead character is
beyond '\x7F'.  For instance, input

 true\xC2\xA2

produces the tokens

 JSON_ERROR true\xC2
 JSON_ERROR \xA2

The first token should be

 JSON_KEYWORD   true

instead.


As long as we still get a JSON_ERROR in the end.



The culprit is

 #define TERMINAL(state) [0 ... 0x7F] = (state)

It leaves [0x80..0xFF] zero, i.e. IN_ERROR.  Has always been broken.


I wonder if that was done because it was assuming that valid input is 
only ASCII, and that any byte larger than 0x7f is invalid except within 
the context of a string.  But whatever the reason for the original bug, 
your fix makes sense.



Fix it to initialize the complete array.


Worth testsuite coverage?



Signed-off-by: Markus Armbruster 
---
  qobject/json-lexer.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Eric Blake 


diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index e1745a3d95..4867839f66 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -123,7 +123,7 @@ enum json_lexer_state {
  QEMU_BUILD_BUG_ON((int)JSON_MIN <= (int)IN_START_INTERP);
  QEMU_BUILD_BUG_ON(IN_START_INTERP != IN_START + 1);
  
-#define TERMINAL(state) [0 ... 0x7F] = (state)

+#define TERMINAL(state) [0 ... 0xFF] = (state)
  
  /* Return whether TERMINAL is a terminal state and the transition to it

 from OLD_STATE required lookahead.  This happens whenever the table



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 4/4] net: Remove the deprecated -tftp, -bootp, -redir and -smb options

[Thomas Huth]

On 2018-08-27 18:11, Peter Maydell wrote:
> On 27 August 2018 at 15:54, Thomas Huth  wrote:
>> These options likely do not work as expected as soon as the user
>> tries to use more than one network interface at once. The parameters
>> have been marked as deprecated since QEMU v2.6, so users had plenty
>> of time to move their scripts to the new syntax.
> 
> I still haven't changed my scripts, because the warning
> message if you use -redir still doesn't tell you what
> exactly the new syntax option equivalent is...

Oh, c'mon, I explicitly updated the documentation after you've
complained the last time:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0065e915192cdf83c270

Just RTFM ;-)

 Thomas

Re: [Qemu-devel] [PATCH 4/4] net: Remove the deprecated -tftp, -bootp, -redir and -smb options

[Peter Maydell]

On 27 August 2018 at 15:54, Thomas Huth  wrote:
> These options likely do not work as expected as soon as the user
> tries to use more than one network interface at once. The parameters
> have been marked as deprecated since QEMU v2.6, so users had plenty
> of time to move their scripts to the new syntax.

I still haven't changed my scripts, because the warning
message if you use -redir still doesn't tell you what
exactly the new syntax option equivalent is...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-block] [PATCH v0 0/2] Postponed actions

[John Snow]

On 08/27/2018 03:05 AM, Denis Plotnikov wrote:
> PING! PING!
> 

Sorry, Kevin and Stefan are both on PTO right now, I think. I can't
promise I have the time to look soon, but you at least deserve an answer
for the radio silence the last week.

--js

> On 14.08.2018 10:08, Denis Plotnikov wrote:
>>
>>
>> On 13.08.2018 19:30, Kevin Wolf wrote:
>>> Am 13.08.2018 um 10:32 hat Denis Plotnikov geschrieben:
 Ping ping!

 On 16.07.2018 21:59, John Snow wrote:
>
>
> On 07/16/2018 11:01 AM, Denis Plotnikov wrote:
>> Ping!
>>
>
> I never saw a reply to Stefan's question on July 2nd, did you reply
> off-list?
>
> --js
 Yes, I did. I talked to Stefan why the patch set appeared.
>>>
>>> The rest of us still don't know the answer. I had the same question.
>>>
>>> Kevin
>> Yes, that's my fault. I should have post it earlier.
>>
>> I reviewed the problem once again and come up with the following
>> explanation.
>> Indeed, if the global lock has been taken by the main thread the vCPU
>> threads won't be able to execute mmio ide.
>> But, if the main thread will release the lock then nothing will prevent
>> vCPU threads form execution what they want, e.g writing to the block
>> device.
>>
>> In case of running the mirroring it is possible. Let's take a look
>> at the following snippet of mirror_run. This is a part the mirroring
>> completion part.
>>
>>  bdrv_drained_begin(bs);
>>  cnt = bdrv_get_dirty_count(s->dirty_bitmap);
>>  >>  if (cnt > 0 || mirror_flush(s) < 0) {
>>  bdrv_drained_end(bs);
>>  continue;
>>  }
>>
>> (X)     assert(QLIST_EMPTY(>tracked_requests));
>>
>> mirror_flush here can yield the current coroutine so nothing more can
>> be executed.
>> We could end up with the situation when the main loop have to revolve
>> to poll for another timer/bh to process. While revolving it releases
>> the global lock. If the global lock is waited for by a vCPU (any
>> other) thread, the waiting thread will get the lock and make what it
>> intends.
>>
>> This is something that I can observe:
>>
>> mirror_flush yields coroutine, the main thread revolves and locks
>> because a vCPU was waiting for the lock. Now the vCPU thread owns the
>> lock and the main thread waits for the lock releasing.
>> The vCPU thread does cmd_write_dma and releases the lock. Then, the main
>> thread gets the lock and continues to run eventually proceeding with
>> the coroutine yeiled.
>> If the vCPU requests aren't completed by the moment we will assert at
>> (X). If the vCPU requests are completed we won't even notice that we
>> had some writes while in the drained section.
>>
>> Denis
>>>
>> On 29.06.2018 15:40, Denis Plotnikov wrote:
>>> There are cases when a request to a block driver state shouldn't
>>> have
>>> appeared producing dangerous race conditions.
>>> This misbehaviour is usually happens with storage devices emulated
>>> without eventfd for guest to host notifications like IDE.
>>>
>>> The issue arises when the context is in the "drained" section
>>> and doesn't expect the request to come, but request comes from the
>>> device not using iothread and which context is processed by the main
>>> loop.
>>>
>>> The main loop apart of the iothread event loop isn't blocked by the
>>> "drained" section.
>>> The request coming and processing while in "drained" section can
>>> spoil
>>> the
>>> block driver state consistency.
>>>
>>> This behavior can be observed in the following KVM-based case:
>>>
>>> 1. Setup a VM with an IDE disk.
>>> 2. Inside a VM start a disk writing load for the IDE device
>>>      e.g: dd if= of= bs=X count=Y oflag=direct
>>> 3. On the host create a mirroring block job for the IDE device
>>>      e.g: drive_mirror  
>>> 4. On the host finish the block job
>>>      e.g: block_job_complete 
>>>     Having done the 4th action, you could get an assert:
>>> assert(QLIST_EMPTY(>tracked_requests)) from mirror_run.
>>> On my setup, the assert is 1/3 reproducible.
>>>
>>> The patch series introduces the mechanism to postpone the requests
>>> until the BDS leaves "drained" section for the devices not using
>>> iothreads.
>>> Also, it modifies the asynchronous block backend infrastructure
>>> to use
>>> that mechanism to release the assert bug for IDE devices.
>>>
>>> Denis Plotnikov (2):
>>>      async: add infrastructure for postponed actions
>>>      block: postpone the coroutine executing if the BDS's is drained
>>>
>>>     block/block-backend.c | 58
>>> ++-
>>>     include/block/aio.h   | 63
>>> +++
>>>     util/async.c  | 33 +++
>>>     3 files changed, 142 insertions(+), 12 deletions(-)

Re: [Qemu-devel] [PATCH 1/7] jobs: change start callback to run callback

[John Snow]

On 08/25/2018 09:33 AM, Max Reitz wrote:
> On 2018-08-23 01:01, John Snow wrote:
>>
>>
>> On 08/22/2018 06:51 AM, Max Reitz wrote:
>>> On 2018-08-17 21:04, John Snow wrote:
 Presently we codify the entry point for a job as the "start" callback,
 but a more apt name would be "run" to clarify the idea that when this
 function returns we consider the job to have "finished," except for
 any cleanup which occurs in separate callbacks later.

 As part of this clarification, change the signature to include an error
 object and a return code. The error ptr is not yet used, and the return
 code while captured, will be overwritten by actions in the job_completed
 function.

 Signed-off-by: John Snow 
 ---
  block/backup.c|  7 ---
  block/commit.c|  7 ---
  block/create.c|  8 +---
  block/mirror.c| 10 ++
  block/stream.c|  7 ---
  include/qemu/job.h|  2 +-
  job.c |  6 +++---
  tests/test-bdrv-drain.c   |  7 ---
  tests/test-blockjob-txn.c | 16 
  tests/test-blockjob.c |  7 ---
  10 files changed, 43 insertions(+), 34 deletions(-)
>>>
>>> [...]
>>>
 diff --git a/job.c b/job.c
 index fa671b431a..898260b2b3 100644
 --- a/job.c
 +++ b/job.c
 @@ -544,16 +544,16 @@ static void coroutine_fn job_co_entry(void *opaque)
  {
  Job *job = opaque;
  
 -assert(job && job->driver && job->driver->start);
 +assert(job && job->driver && job->driver->run);
  job_pause_point(job);
 -job->driver->start(job);
 +job->ret = job->driver->run(job, NULL);
  }
>>>
>>> Hmmm, this breaks the iff relationship with job->error.  We might call
>>> job_update_rc() afterwards, but then job_completed() would need to free
>>> it if it overwrites it with the error description from a potential error
>>> object.
>>>
>>> Also, I suspect the following patches might fix the relationship anyway?
>>>  (But then an "XXX: This does not hold right now, but will be fixed in a
>>> future patch" in the documentation of Job.error might help.)
>>>
>>> Max
>>>
>>
>> Hmm... does it? ... I guess you mean that we are setting job->ret
>> earlier than we used to, which gives us a window where you can have ret
>> set, but error unset.
>>
>> This will get settled out by the end of the series anyway:
> 
> Oh no, it appears I accidentally removed yet another chunk from my reply
> to patch 2...
> 
>> - char *error gets replaced with Error *err,
> 
> Which is basically the same.  I noted in the deleted chunk that patch 2
> just removes the iff relationship from the describing comment, but, well...
> 
>> - I remove the error object from job_completed
>> - v2 will remove the ret argument, too.
> 
> The most important bit of the chunk I removed was that I was complaining
> about Job.ret still being there.  I don't really see the point of this
> patch here at this point.
> 
> Unfortunately I can't quite recall...
> 
> Having a central Error object doesn't really make sense.  Whenever the
> block job wants to return an error, it should probably do so as "return"
> values of methods (like .run()).  Same for ret, of course.
> 
> I understand that this is probably really only possible after v2 when
> you've made more use of abort/commit.  But still, I don't think this
> patch improves anything, so I would leave this clean-up until later when
> you can really do something.
> 
> I suppose the idea here is that you want to drop the errp parameter from
> job_completed(), because it is not going to be called by .exit().  But
> the obvious way around this would be to pass an errp to .exit() and then
> pass the result on to job_completed().
> 
> Max
> 

That might be a good idea, but I need to look at the pathway for
actually showing that error to the user, since we need to pass it down a
few times anyway. It's certainly simpler to just stash it in the object.

I'll take a look.

Re: [Qemu-devel] [PATCH 3/7] jobs: add exit shim

[John Snow]

On 08/25/2018 09:05 AM, Max Reitz wrote:
> On 2018-08-22 23:52, John Snow wrote:
>>
>>
>> On 08/22/2018 07:43 AM, Max Reitz wrote:
>>> On 2018-08-17 21:04, John Snow wrote:
 All jobs do the same thing when they leave their running loop:
 - Store the return code in a structure
 - wait to receive this structure in the main thread
 - signal job completion via job_completed

 Few jobs do anything beyond exactly this. Consolidate this exit
 logic for a net reduction in SLOC.

 More seriously, when we utilize job_defer_to_main_loop_bh to call
 a function that calls job_completed, job_finalize_single will run
 in a context where it has recursively taken the aio_context lock,
 which can cause hangs if it puts down a reference that causes a flush.

 You can observe this in practice by looking at mirror_exit's careful
 placement of job_completed and bdrv_unref calls.

 If we centralize job exiting, we can signal job completion from outside
 of the aio_context, which should allow for job cleanup code to run with
 only one lock, which makes cleanup callbacks less tricky to write.

 Signed-off-by: John Snow 
 ---
  include/qemu/job.h |  7 +++
  job.c  | 19 +++
  2 files changed, 26 insertions(+)
>>>
>>> Currently all jobs do this, the question of course is why.  The answer
>>> is because they are block jobs that need to do some graph manipulation
>>> in the main thread, right?
>>>
>>
>> Yep.
>>
>>> OK, that's reasonable enough, that sounds like even non-block jobs may
>>> need this (i.e. modify some global qemu state that you can only do in
>>> the main loop).  Interestingly, the create job only calls
>>> job_completed() of which it says nowhere that it needs to be executed in
>>> the main loop.
>>>
>>
>> Yeah, not all jobs will have anything meaningful to do in the main loop
>> context. This is one of them.
>>
>>> ...on second thought, do we really want to execute job_complete() in the
>>> main loop?  First of all, all of the transactional functions will run in
>>> the main loop.  Which makes sense, but it isn't noted anywhere.
>>> Secondly, we may end up calling JobDriver.user_resume(), which is
>>> probably not something we want to call in the main loop.
>>>
>>
>> I think we need to execute job_complete in the main loop, or otherwise
>> restructure the code that can run between job_completed and
>> job_finalize_single so that .prepare/.commit/.abort/.clean run in the
>> main thread, which is something we want to preserve.
> 
> Sure.
> 
>> It's simpler just to say that complete will run from the main thread,
>> like it does presently.
> 
> Yes, but we don't say that.
> 
>> Why would we not want to call user_resume from the main loop? That's
>> directly where it's called from, since it gets invoked directly from the
>> qmp thread.
> 
> Hmm!  True indeed.
> 
> The reason why we might not want to do it is because the job may not run
> in the main loop, so modifying the job (especially invoking a job
> method) may be dangerous without taking precautions.
> 
>>> OTOH, job_finish_sync() is something that has to be run in the main loop
>>> because it polls the main loop (and as far as my FUSE experiments have
>>> told me, polling a foreign AioContext doesn't work).
>>>
>>> So...  I suppose it would be nice if we had a real distinction which
>>> functions are run in which AioContext.  It seems like we indeed want to
>>> run job_completed() in the main loop, but what to do about the
>>> user_resume() call in job_cancel_async()?
>>>
>>
>> I don't think we need to do anything -- at least, these functions
>> *already* run from the main loop.
> 
> Yeah, but we don't mark that anywhere.  I really don't like that.  Jobs
> need to know which of their functions are run in which AioContext.
> 
>> mirror_exit et al get scheduled from job_defer_to_main_loop and call
>> job_completed there, so it's already always done from the main loop; I'm
>> just cutting out the part where the jobs have to manually schedule this.
> 
> I'm not saying what you're doing is wrong, I'm just saying tracking
> which things are running in which context is not easy because there are
> no comments on how it's supposed to be run.  (Apart from your new
> .exit() method which does say that it's run in the main loop.)
> 
> No, I don't find it obvious which functions are run in which context
> when first I have to think about in which context those functions are
> used (e.g. user_resume is usually the result of a QMP command, so it's
> run in the main loop; the transactional methods are part of completion,
> which is done in the main loop, so they are also called in the main
> loop; and so on).
> 
> But that's not part of this series.  It just occurred to me when
> tracking down which function belongs to which context when reviewing
> this patch.
> 
> Max
> 

Oh, I see. I can mark up the functions I/we expect to run in the main

Re: [Qemu-devel] [PULL 0/9] Disable tests when device is not compiled in

[Peter Maydell]

On 27 August 2018 at 11:04, Juan Quintela  wrote:
> The following changes since commit 235c82acca0491465e94be3cae2583b42d37c859:
>
>   Merge remote-tracking branch 'remotes/otubo/tags/pull-seccomp-20180823' 
> into staging (2018-08-25 13:08:57 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/check/20180827
>
> for you to fetch changes up to ee1f6c812b3240420dff07a3860060b7d4abfe09:
>
>   check: Move wdt_ib700 test to common (2018-08-27 11:27:07 +0200)
>
> 
> check/next for 20180827
>
> Hi
>
> This are the patches that are reviewed to disable tests when devices
> are not compiled in.  I will stop now until the changes to qtest are
> finished.
>
> Later, Juan.
>
Applied, thanks.

-- PMM

Re: [Qemu-devel] [libvirt] [PATCH 0/4] Remove more deprecated options

[Ján Tomko]

On Mon, Aug 27, 2018 at 05:08:45PM +0200, Peter Krempa wrote:

On Mon, Aug 27, 2018 at 16:54:08 +0200, Thomas Huth wrote:

These options are deprecated since at least two releases, and nobody
complained. Time to remove them now.

(I'm sending these patches as a series since Paolo asked me to send a PULL
request on my own for them ... but if one of the subsystems maintainers
prefers to take the patches through their own tree instead, just let me
know)

Thomas Huth (4):
  Remove the deprecated -balloon option
  Remove the deprecated -nodefconfig option
  Remove the deprecated options -startdate, -localtime and -rtc-td-hack
  net: Remove the deprecated -tftp, -bootp, -redir and -smb options


Libvirt does not use any of the options above thankfully so a weak-ACK
from the libvirt side.


And another:
ACK yeah!
from the libvirt side.

Jano


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH V4 4/4] target-i386: add i440fx 0xcf8 portascoalesced_pio

[peng.hao2]

>On Mon, Aug 27, 2018 at 04:25:00PM +0800, peng.h...@zte.com.cn wrote:
>> >> On 25 Aug 2018, at 15:19, Peng Hao  wrote:
>> >> 
>> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> >> index 0e60834..da73743 100644
>> >> --- a/hw/pci-host/piix.c
>> >> +++ b/hw/pci-host/piix.c
>> >> @@ -327,6 +327,10 @@ static void i440fx_pcihost_realize(DeviceState *dev, 
>> >> Error **errp)
>> >> 
>> >> sysbus_add_io(sbd, 0xcfc, >data_mem);
>> >> sysbus_init_ioports(sbd, 0xcfc, 4);
>> >> +
>> >> +/* register i440fx 0xcf8 port as coalesced pio */
>> >> +memory_region_set_flush_coalesced(>data_mem);
>> >> +memory_region_add_coalescing(>conf_mem, 0, 4);
>> >> }
>> >> 
>> >Is there a reason to not register this port as coalesced PIO also for Q35?
>> >In q35_host_realize()?
>> >If not, I would do that as an extra patch as part of this series.
>> Just as I mentioned in patch [0/4] , you can add pci->>host config port as 
>> coalesecd pio. I think  it works for q35 port 0xcf8.
>> >-Liran
>
>What's the performance improvement for q35?
q35 also has  the same pci-host config port 0xcf8  as  piix. I test the 
coalesced pio for 
q35 pci-host config port 0xcf8. It spent less VM-exit avg time from 3us to 
0.6us.

>-- 
>MST

Re: [Qemu-devel] [PATCH v2] vl.c deprecate incorrect CPUs topology

[Andrew Jones]

On Mon, Aug 27, 2018 at 03:53:26PM +0200, Igor Mammedov wrote:
> -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> so that total number of logical CPUs [sockets * cores * threads]
> would be equal to [maxcpus], however historically we didn't have
> such check in QEMU and it is possible to start VM with an invalid
> topology.
> Deprecate invalid options combination so we can make sure that
> the topology VM started with is always correct in the future.
> Users with an invalid sockets/cores/threads/maxcpus values should
> fix their CLI to make sure that
>[sockets * cores * threads] == [maxcpus]
> 
> Signed-off-by: Igor Mammedov 
> ---
> v2:
>   - spelling& fixes (Andrew Jones )
> ---
>  qemu-deprecated.texi | 11 +++
>  vl.c |  6 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 87212b6..b649b2e 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -159,6 +159,17 @@ The 'file' driver for drives is no longer appropriate 
> for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> +
> +CPU topology properties should describe whole machine topology including
> +possible CPUs. but historically it was possible to start QEMU with
^ missed this correction should be a ','

> +an incorrect topology where
> +  sockets * cores * threads >= X && X < maxcpus
> +which could lead to incorrect topology enumeration by the guest.

an incorrect

> +Support for invalid topology will be removed, the user must ensure

invalid topologies

> +topologies described with -smp includes all possible cpus, i.e.:

include

no ':' after 'i.e.'

> +  sockets * cores * threads == maxcpus
> +
>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> diff --git a/vl.c b/vl.c
> index 5ba06ad..238f856 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1238,6 +1238,12 @@ static void smp_parse(QemuOpts *opts)
>  exit(1);
>  }
>  
> +if (sockets * cores * threads != max_cpus) {
> +warn_report("Invalid CPU topology deprecated: "
> +"sockets (%u) * cores (%u) * threads (%u) "
> +"!= maxcpus (%u)",
> +sockets, cores, threads, max_cpus);
> +}
>  if (sockets * cores * threads > max_cpus) {
>  error_report("cpu topology: "
>   "sockets (%u) * cores (%u) * threads (%u) > "
> -- 
> 2.7.4
> 
>

Thanks,
drew 

Re: [Qemu-devel] [libvirt] [PATCH 0/4] Remove more deprecated options

[Peter Krempa]

On Mon, Aug 27, 2018 at 16:54:08 +0200, Thomas Huth wrote:
> These options are deprecated since at least two releases, and nobody
> complained. Time to remove them now.
> 
> (I'm sending these patches as a series since Paolo asked me to send a PULL
> request on my own for them ... but if one of the subsystems maintainers
> prefers to take the patches through their own tree instead, just let me
> know)
> 
> Thomas Huth (4):
>   Remove the deprecated -balloon option
>   Remove the deprecated -nodefconfig option
>   Remove the deprecated options -startdate, -localtime and -rtc-td-hack
>   net: Remove the deprecated -tftp, -bootp, -redir and -smb options

Libvirt does not use any of the options above thankfully so a weak-ACK
from the libvirt side.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 3/4] Remove the deprecated options -startdate, -localtime and -rtc-td-hack

[Thomas Huth]

Deprecated since two releases, nobody complained, thus it's time to
remove them now.

Signed-off-by: Thomas Huth 
---
 qemu-deprecated.texi | 13 -
 qemu-options.hx  |  7 -
 vl.c | 76 +++-
 3 files changed, 22 insertions(+), 74 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 19c8ae2..ca52e83 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -96,19 +96,6 @@ The @code{--no-frame} argument works with SDL 1.2 only. The 
other user
 interfaces never implemented this in the first place. So this will be
 removed together with SDL 1.2 support.
 
-@subsection -rtc-td-hack (since 2.12.0)
-
-The @code{-rtc-td-hack} option has been replaced by
-@code{-rtc driftfix=slew}.
-
-@subsection -localtime (since 2.12.0)
-
-The @code{-localtime} option has been replaced by @code{-rtc base=localtime}.
-
-@subsection -startdate (since 2.12.0)
-
-The @code{-startdate} option has been replaced by @code{-rtc base=@var{date}}.
-
 @subsection -virtioconsole (since 3.0.0)
 
 Option @option{-virtioconsole} has been replaced by
diff --git a/qemu-options.hx b/qemu-options.hx
index d9be20b..7ca539a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1711,9 +1711,6 @@ Windows 2000 is installed, you no longer need this option 
(this option
 slows down the IDE transfers).
 ETEXI
 
-HXCOMM Deprecated by -rtc
-DEF("rtc-td-hack", 0, QEMU_OPTION_rtc_td_hack, "", QEMU_ARCH_I386)
-
 DEF("no-fd-bootchk", 0, QEMU_OPTION_no_fd_bootchk,
 "-no-fd-bootchk  disable boot signature checking for floppy disks\n",
 QEMU_ARCH_I386)
@@ -3471,10 +3468,6 @@ ETEXI
 HXCOMM Silently ignored for compatibility
 DEF("clock", HAS_ARG, QEMU_OPTION_clock, "", QEMU_ARCH_ALL)
 
-HXCOMM Options deprecated by -rtc
-DEF("localtime", 0, QEMU_OPTION_localtime, "", QEMU_ARCH_ALL)
-DEF("startdate", HAS_ARG, QEMU_OPTION_startdate, "", QEMU_ARCH_ALL)
-
 DEF("rtc", HAS_ARG, QEMU_OPTION_rtc, \
 "-rtc [base=utc|localtime|date][,clock=host|rt|vm][,driftfix=none|slew]\n" 
\
 "set the RTC base and clock, enable drift fix for clock 
ticks (x86 only)\n",
diff --git a/vl.c b/vl.c
index 386c71d..cfeee0d 100644
--- a/vl.c
+++ b/vl.c
@@ -823,44 +823,33 @@ int qemu_timedate_diff(struct tm *tm)
 return seconds - qemu_time();
 }
 
-static void configure_rtc_date_offset(const char *startdate, int legacy)
+static void configure_rtc_date_offset(const char *startdate)
 {
 time_t rtc_start_date;
 struct tm tm;
 
-if (!strcmp(startdate, "now") && legacy) {
-rtc_date_offset = -1;
+if (sscanf(startdate, "%d-%d-%dT%d:%d:%d", _year, _mon,
+   _mday, _hour, _min, _sec) == 6) {
+/* OK */
+} else if (sscanf(startdate, "%d-%d-%d",
+  _year, _mon, _mday) == 3) {
+tm.tm_hour = 0;
+tm.tm_min = 0;
+tm.tm_sec = 0;
 } else {
-if (sscanf(startdate, "%d-%d-%dT%d:%d:%d",
-   _year,
-   _mon,
-   _mday,
-   _hour,
-   _min,
-   _sec) == 6) {
-/* OK */
-} else if (sscanf(startdate, "%d-%d-%d",
-  _year,
-  _mon,
-  _mday) == 3) {
-tm.tm_hour = 0;
-tm.tm_min = 0;
-tm.tm_sec = 0;
-} else {
-goto date_fail;
-}
-tm.tm_year -= 1900;
-tm.tm_mon--;
-rtc_start_date = mktimegm();
-if (rtc_start_date == -1) {
-date_fail:
-error_report("invalid date format");
-error_printf("valid formats: "
- "'2006-06-17T16:01:21' or '2006-06-17'\n");
-exit(1);
-}
-rtc_date_offset = qemu_time() - rtc_start_date;
+goto date_fail;
+}
+tm.tm_year -= 1900;
+tm.tm_mon--;
+rtc_start_date = mktimegm();
+if (rtc_start_date == -1) {
+date_fail:
+error_report("invalid date format");
+error_printf("valid formats: "
+ "'2006-06-17T16:01:21' or '2006-06-17'\n");
+exit(1);
 }
+rtc_date_offset = qemu_time() - rtc_start_date;
 }
 
 static void configure_rtc(QemuOpts *opts)
@@ -878,7 +867,7 @@ static void configure_rtc(QemuOpts *opts)
   "-rtc base=localtime");
 replay_add_blocker(blocker);
 } else {
-configure_rtc_date_offset(value, 0);
+configure_rtc_date_offset(value);
 }
 }
 value = qemu_opt_get(opts, "clock");
@@ -3269,11 +3258,6 @@ int main(int argc, char **argv, char **envp)
 case QEMU_OPTION_k:
 keyboard_layout = optarg;
 break;
-case QEMU_OPTION_localtime:
-rtc_utc = 0;
-warn_report("This option is deprecated, "
-"use '-rtc base=localtime' instead.");
- 

[Qemu-devel] [PATCH 1/4] Remove the deprecated -balloon option

[Thomas Huth]

The "-balloon" option has been replaced by "-device virtio-balloon".
It's been marked as deprecated since two releases, and nobody
complained, so let's remove it now.

Acked-by: Paolo Bonzini 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Cornelia Huck 
Reviewed-by: Michael S. Tsirkin 
Reviewed-by: David Hildenbrand 
Signed-off-by: Thomas Huth 
---
 docs/virtio-balloon-stats.txt |  6 +++---
 qemu-deprecated.texi  |  5 -
 qemu-options.hx   | 10 --
 vl.c  | 36 
 4 files changed, 3 insertions(+), 54 deletions(-)

diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
index 9985e1d..1732cc8 100644
--- a/docs/virtio-balloon-stats.txt
+++ b/docs/virtio-balloon-stats.txt
@@ -61,9 +61,9 @@ It's also important to note the following:
respond to the request the timer will never be re-armed, which has
the same effect as disabling polling
 
-Here are a few examples. QEMU is started with '-balloon virtio', which
-generates '/machine/peripheral-anon/device[1]' as the QOM path for the
-balloon device.
+Here are a few examples. QEMU is started with '-device virtio-balloon',
+which generates '/machine/peripheral-anon/device[1]' as the QOM path for
+the balloon device.
 
 Enable polling with 2 seconds interval:
 
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007..98f5062 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -86,11 +86,6 @@ enabled via the ``-machine usb=on'' argument.
 
 The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
 
-@subsection -balloon (since 2.12.0)
-
-The @option{--balloon virtio} argument has been superseded by
-@option{--device virtio-balloon}.
-
 @subsection -fsdev handle (since 2.12.0)
 
 The ``handle'' fsdev backend does not support symlinks and causes the 9p
diff --git a/qemu-options.hx b/qemu-options.hx
index 654ef48..ccf7155 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -454,16 +454,6 @@ modprobe i810_audio clocking=48000
 @end example
 ETEXI
 
-DEF("balloon", HAS_ARG, QEMU_OPTION_balloon,
-"-balloon virtio[,addr=str]\n"
-"enable virtio balloon device (deprecated)\n", 
QEMU_ARCH_ALL)
-STEXI
-@item -balloon virtio[,addr=@var{addr}]
-@findex -balloon
-Enable virtio balloon device, optionally with PCI address @var{addr}. This
-option is deprecated, use @option{-device virtio-balloon} instead.
-ETEXI
-
 DEF("device", HAS_ARG, QEMU_OPTION_device,
 "-device driver[,prop[=value][,...]]\n"
 "add device (based on driver)\n"
diff --git a/vl.c b/vl.c
index 5ba06ad..674c42f 100644
--- a/vl.c
+++ b/vl.c
@@ -2127,36 +2127,6 @@ static void parse_display(const char *p)
 }
 }
 
-static int balloon_parse(const char *arg)
-{
-QemuOpts *opts;
-
-warn_report("This option is deprecated. "
-"Use '--device virtio-balloon' to enable the balloon device.");
-
-if (strcmp(arg, "none") == 0) {
-return 0;
-}
-
-if (!strncmp(arg, "virtio", 6)) {
-if (arg[6] == ',') {
-/* have params -> parse them */
-opts = qemu_opts_parse_noisily(qemu_find_opts("device"), arg + 7,
-   false);
-if (!opts)
-return  -1;
-} else {
-/* create empty opts */
-opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
-_abort);
-}
-qemu_opt_set(opts, "driver", "virtio-balloon", _abort);
-return 0;
-}
-
-return -1;
-}
-
 char *qemu_find_file(int type, const char *name)
 {
 int i;
@@ -3660,12 +3630,6 @@ int main(int argc, char **argv, char **envp)
 case QEMU_OPTION_no_hpet:
 no_hpet = 1;
 break;
-case QEMU_OPTION_balloon:
-if (balloon_parse(optarg) < 0) {
-error_report("unknown -balloon argument %s", optarg);
-exit(1);
-}
-break;
 case QEMU_OPTION_no_reboot:
 no_reboot = 1;
 break;
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] net: Remove the deprecated -tftp, -bootp, -redir and -smb options

[Thomas Huth]

These options likely do not work as expected as soon as the user
tries to use more than one network interface at once. The parameters
have been marked as deprecated since QEMU v2.6, so users had plenty
of time to move their scripts to the new syntax. Time to remove the
old parameters now.

Signed-off-by: Thomas Huth 
---
 include/net/net.h|   3 --
 include/net/slirp.h  |   4 --
 net/slirp.c  | 132 +++
 os-posix.c   |   8 
 qemu-deprecated.texi |  34 -
 qemu-options.hx  |  15 --
 vl.c |  18 ---
 7 files changed, 29 insertions(+), 185 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 1425960..7936d53 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -201,9 +201,6 @@ extern NICInfo nd_table[MAX_NICS];
 extern const char *host_net_devices[];
 
 /* from net.c */
-extern const char *legacy_tftp_prefix;
-extern const char *legacy_bootp_filename;
-
 int net_client_parse(QemuOptsList *opts_list, const char *str);
 int net_init_clients(Error **errp);
 void net_check_clients(void);
diff --git a/include/net/slirp.h b/include/net/slirp.h
index 4d63d74..bad3e1e 100644
--- a/include/net/slirp.h
+++ b/include/net/slirp.h
@@ -30,10 +30,6 @@
 void hmp_hostfwd_add(Monitor *mon, const QDict *qdict);
 void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict);
 
-int net_slirp_redir(const char *redir_str);
-
-int net_slirp_smb(const char *exported_dir);
-
 void hmp_info_usernet(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/net/slirp.c b/net/slirp.c
index 1e14318..c18060f 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -67,13 +67,11 @@ static int get_str_sep(char *buf, int buf_size, const char 
**pp, int sep)
 /* slirp network adapter */
 
 #define SLIRP_CFG_HOSTFWD 1
-#define SLIRP_CFG_LEGACY  2
 
 struct slirp_config_str {
 struct slirp_config_str *next;
 int flags;
 char str[1024];
-int legacy_format;
 };
 
 typedef struct SlirpState {
@@ -87,19 +85,13 @@ typedef struct SlirpState {
 } SlirpState;
 
 static struct slirp_config_str *slirp_configs;
-const char *legacy_tftp_prefix;
-const char *legacy_bootp_filename;
 static QTAILQ_HEAD(slirp_stacks, SlirpState) slirp_stacks =
 QTAILQ_HEAD_INITIALIZER(slirp_stacks);
 
-static int slirp_hostfwd(SlirpState *s, const char *redir_str,
- int legacy_format, Error **errp);
-static int slirp_guestfwd(SlirpState *s, const char *config_str,
-  int legacy_format, Error **errp);
+static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp);
+static int slirp_guestfwd(SlirpState *s, const char *config_str, Error **errp);
 
 #ifndef _WIN32
-static const char *legacy_smb_export;
-
 static int slirp_smb(SlirpState *s, const char *exported_dir,
  struct in_addr vserver_addr, Error **errp);
 static void slirp_smb_cleanup(SlirpState *s);
@@ -196,13 +188,6 @@ static int net_slirp_init(NetClientState *peer, const char 
*model,
 return -1;
 }
 
-if (!tftp_export) {
-tftp_export = legacy_tftp_prefix;
-}
-if (!bootfile) {
-bootfile = legacy_bootp_filename;
-}
-
 if (vnetwork) {
 if (get_str_sep(buf, sizeof(buf), , '/') < 0) {
 if (!inet_aton(vnetwork, )) {
@@ -382,21 +367,16 @@ static int net_slirp_init(NetClientState *peer, const 
char *model,
 
 for (config = slirp_configs; config; config = config->next) {
 if (config->flags & SLIRP_CFG_HOSTFWD) {
-if (slirp_hostfwd(s, config->str,
-  config->flags & SLIRP_CFG_LEGACY, errp) < 0) {
+if (slirp_hostfwd(s, config->str, errp) < 0) {
 goto error;
 }
 } else {
-if (slirp_guestfwd(s, config->str,
-   config->flags & SLIRP_CFG_LEGACY, errp) < 0) {
+if (slirp_guestfwd(s, config->str, errp) < 0) {
 goto error;
 }
 }
 }
 #ifndef _WIN32
-if (!smb_export) {
-smb_export = legacy_smb_export;
-}
 if (smb_export) {
 if (slirp_smb(s, smb_export, smbsrv, errp) < 0) {
 goto error;
@@ -506,8 +486,7 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
 monitor_printf(mon, "invalid format\n");
 }
 
-static int slirp_hostfwd(SlirpState *s, const char *redir_str,
- int legacy_format, Error **errp)
+static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 {
 struct in_addr host_addr = { .s_addr = INADDR_ANY };
 struct in_addr guest_addr = { .s_addr = 0 };
@@ -532,18 +511,16 @@ static int slirp_hostfwd(SlirpState *s, const char 
*redir_str,
 goto fail_syntax;
 }
 
-if (!legacy_format) {
-if (get_str_sep(buf, sizeof(buf), , ':') < 0) {
-fail_reason = "Missing : separator";
-goto fail_syntax;
-}
-if (buf[0] 

[Qemu-devel] [PATCH 2/4] Remove the deprecated -nodefconfig option

[Thomas Huth]

It's the same as -no-user-config and marked as deprecated since three
releases already. Time to remove it now.

Signed-off-by: Thomas Huth 
---
 docs/interop/live-block-operations.rst | 4 ++--
 qemu-deprecated.texi   | 4 
 qemu-options.hx| 4 ++--
 vl.c   | 2 --
 4 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/docs/interop/live-block-operations.rst 
b/docs/interop/live-block-operations.rst
index 734252b..48afdc7 100644
--- a/docs/interop/live-block-operations.rst
+++ b/docs/interop/live-block-operations.rst
@@ -129,7 +129,7 @@ To show some example invocations of command-line, we will 
use the
 following invocation of QEMU, with a QMP server running over UNIX
 socket::
 
-$ ./x86_64-softmmu/qemu-system-x86_64 -display none -nodefconfig \
+$ ./x86_64-softmmu/qemu-system-x86_64 -display none -no-user-config \
 -M q35 -nodefaults -m 512 \
 -blockdev 
node-name=node-A,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./a.qcow2
 \
 -device virtio-blk,drive=node-A,id=virtio0 \
@@ -694,7 +694,7 @@ instance, with the following invocation.  (As noted 
earlier, for
 simplicity's sake, the destination QEMU is started on the same host, but
 it could be located elsewhere)::
 
-$ ./x86_64-softmmu/qemu-system-x86_64 -display none -nodefconfig \
+$ ./x86_64-softmmu/qemu-system-x86_64 -display none -no-user-config \
 -M q35 -nodefaults -m 512 \
 -blockdev 
node-name=node-TargetDisk,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./target-disk.qcow2
 \
 -device virtio-blk,drive=node-TargetDisk,id=virtio0 \
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 98f5062..19c8ae2 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -82,10 +82,6 @@ would automatically enable USB support on the machine type.
 If using the new syntax, USB support must be explicitly
 enabled via the ``-machine usb=on'' argument.
 
-@subsection -nodefconfig (since 2.11.0)
-
-The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
-
 @subsection -fsdev handle (since 2.12.0)
 
 The ``handle'' fsdev backend does not support symlinks and causes the 9p
diff --git a/qemu-options.hx b/qemu-options.hx
index ccf7155..d9be20b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3833,8 +3833,7 @@ Write device configuration to @var{file}. The @var{file} 
can be either filename
 command line and device configuration into file or dash @code{-}) character to 
print the
 output to stdout. This can be later used as input file for @code{-readconfig} 
option.
 ETEXI
-HXCOMM Deprecated, same as -no-user-config
-DEF("nodefconfig", 0, QEMU_OPTION_nodefconfig, "", QEMU_ARCH_ALL)
+
 DEF("no-user-config", 0, QEMU_OPTION_nouserconfig,
 "-no-user-config\n"
 "do not load default user-provided config files at 
startup\n",
@@ -3845,6 +3844,7 @@ STEXI
 The @code{-no-user-config} option makes QEMU not load any of the user-provided
 config files on @var{sysconfdir}.
 ETEXI
+
 DEF("trace", HAS_ARG, QEMU_OPTION_trace,
 "-trace [[enable=]][,events=][,file=]\n"
 "specify tracing options\n",
diff --git a/vl.c b/vl.c
index 674c42f..386c71d 100644
--- a/vl.c
+++ b/vl.c
@@ -2999,7 +2999,6 @@ int main(int argc, char **argv, char **envp)
 
 popt = lookup_opt(argc, argv, , );
 switch (popt->index) {
-case QEMU_OPTION_nodefconfig:
 case QEMU_OPTION_nouserconfig:
 userconfig = false;
 break;
@@ -3927,7 +3926,6 @@ int main(int argc, char **argv, char **envp)
 case QEMU_OPTION_enable_sync_profile:
 qsp_enable();
 break;
-case QEMU_OPTION_nodefconfig:
 case QEMU_OPTION_nouserconfig:
 /* Nothing to be parsed here. Especially, do not error out 
below. */
 break;
-- 
1.8.3.1

[Qemu-devel] [PATCH 0/4] Remove more deprecated options

[Thomas Huth]

These options are deprecated since at least two releases, and nobody
complained. Time to remove them now.

(I'm sending these patches as a series since Paolo asked me to send a PULL
request on my own for them ... but if one of the subsystems maintainers
prefers to take the patches through their own tree instead, just let me
know)

Thomas Huth (4):
  Remove the deprecated -balloon option
  Remove the deprecated -nodefconfig option
  Remove the deprecated options -startdate, -localtime and -rtc-td-hack
  net: Remove the deprecated -tftp, -bootp, -redir and -smb options

 docs/interop/live-block-operations.rst |   4 +-
 docs/virtio-balloon-stats.txt  |   6 +-
 include/net/net.h  |   3 -
 include/net/slirp.h|   4 -
 net/slirp.c| 132 -
 os-posix.c |   8 --
 qemu-deprecated.texi   |  56 --
 qemu-options.hx|  36 +
 vl.c   | 132 ++---
 9 files changed, 58 insertions(+), 323 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/6] target/mips: Add MXU instruction D16MAC

[Craig Janeczek via Qemu-devel]

Adds support for emulating the D16MAC instruction.

Signed-off-by: Craig Janeczek 
---
 v1
- initial patch
 v2
- changed bitfield usage to extract32
- used sextract_tl instructions instead of shift and ext

 target/mips/translate.c | 62 -
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 8a6b4f2899..7d37567652 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -366,6 +366,7 @@ enum {
 OPC_DCLO = 0x25 | OPC_SPECIAL2,
 /* MXU */
 OPC_MXU_D16MUL = 0x08 | OPC_SPECIAL2,
+OPC_MXU_D16MAC = 0x0A | OPC_SPECIAL2,
 OPC_MXU_S8LDD  = 0x22 | OPC_SPECIAL2,
 OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
 OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
@@ -3774,7 +3775,7 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
 static void gen_mxu(DisasContext *ctx, uint32_t opc)
 {
 TCGv t0, t1, t2, t3;
-uint32_t rb, xra, xrb, xrc, xrd, s8, sel, optn2, optn3;
+uint32_t rb, xra, xrb, xrc, xrd, s8, sel, optn2, optn3, aptn2;
 
 t0 = tcg_temp_new();
 t1 = tcg_temp_new();
@@ -3892,6 +3893,64 @@ static void gen_mxu(DisasContext *ctx, uint32_t opc)
 gen_store_mxu_gpr(t3, xra);
 gen_store_mxu_gpr(t2, xrd);
 break;
+
+case OPC_MXU_D16MAC:
+xra = extract32(ctx->opcode, 6, 4);
+xrb = extract32(ctx->opcode, 10, 4);
+xrc = extract32(ctx->opcode, 14, 4);
+xrd = extract32(ctx->opcode, 18, 4);
+optn2 = extract32(ctx->opcode, 22, 2);
+aptn2 = extract32(ctx->opcode, 24, 2);
+
+gen_load_mxu_gpr(t1, xrb);
+tcg_gen_sextract_tl(t0, t1, 0, 16);
+tcg_gen_sextract_tl(t1, t1, 16, 16);
+gen_load_mxu_gpr(t3, xrc);
+tcg_gen_sextract_tl(t2, t3, 0, 16);
+tcg_gen_sextract_tl(t3, t3, 16, 16);
+
+switch (optn2) {
+case 0: /* XRB.H*XRC.H == lop, XRB.L*XRC.L == rop */
+tcg_gen_mul_tl(t3, t1, t3);
+tcg_gen_mul_tl(t2, t0, t2);
+break;
+case 1: /* XRB.L*XRC.H == lop, XRB.L*XRC.L == rop */
+tcg_gen_mul_tl(t3, t0, t3);
+tcg_gen_mul_tl(t2, t0, t2);
+break;
+case 2: /* XRB.H*XRC.H == lop, XRB.H*XRC.L == rop */
+tcg_gen_mul_tl(t3, t1, t3);
+tcg_gen_mul_tl(t2, t1, t2);
+break;
+case 3: /* XRB.L*XRC.H == lop, XRB.H*XRC.L == rop */
+tcg_gen_mul_tl(t3, t0, t3);
+tcg_gen_mul_tl(t2, t1, t2);
+break;
+}
+gen_load_mxu_gpr(t0, xra);
+gen_load_mxu_gpr(t1, xrd);
+
+switch (aptn2) {
+case 0:
+tcg_gen_add_tl(t3, t0, t3);
+tcg_gen_add_tl(t2, t1, t2);
+break;
+case 1:
+tcg_gen_add_tl(t3, t0, t3);
+tcg_gen_sub_tl(t2, t1, t2);
+break;
+case 2:
+tcg_gen_sub_tl(t3, t0, t3);
+tcg_gen_add_tl(t2, t1, t2);
+break;
+case 3:
+tcg_gen_sub_tl(t3, t0, t3);
+tcg_gen_sub_tl(t2, t1, t2);
+break;
+}
+gen_store_mxu_gpr(t3, xra);
+gen_store_mxu_gpr(t2, xrd);
+break;
 }
 
 tcg_temp_free(t0);
@@ -17985,6 +18044,7 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 case OPC_MXU_S32M2I:
 case OPC_MXU_S8LDD:
 case OPC_MXU_D16MUL:
+case OPC_MXU_D16MAC:
 gen_mxu(ctx, op1);
 break;
 
-- 
2.18.0

[Qemu-devel] [PATCH v2 3/6] target/mips: Add MXU instruction D16MUL

[Craig Janeczek via Qemu-devel]

Adds support for emulating the D16MUL instruction.

Signed-off-by: Craig Janeczek 
---
 v1
- initial patch
 v2
- changed bitfield usage to extract32
- used sextract_tl instructions instead of shift and ext

 target/mips/translate.c | 51 +++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index f5725d8eda..8a6b4f2899 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -365,6 +365,7 @@ enum {
 OPC_DCLZ = 0x24 | OPC_SPECIAL2,
 OPC_DCLO = 0x25 | OPC_SPECIAL2,
 /* MXU */
+OPC_MXU_D16MUL = 0x08 | OPC_SPECIAL2,
 OPC_MXU_S8LDD  = 0x22 | OPC_SPECIAL2,
 OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
 OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
@@ -3772,11 +3773,13 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
 /* MXU Instructions */
 static void gen_mxu(DisasContext *ctx, uint32_t opc)
 {
-TCGv t0, t1;
-uint32_t xra, rb, s8, optn3;
+TCGv t0, t1, t2, t3;
+uint32_t rb, xra, xrb, xrc, xrd, s8, sel, optn2, optn3;
 
 t0 = tcg_temp_new();
 t1 = tcg_temp_new();
+t2 = tcg_temp_new();
+t3 = tcg_temp_new();
 
 switch (opc) {
 case OPC_MXU_S32I2M:
@@ -3848,10 +3851,53 @@ static void gen_mxu(DisasContext *ctx, uint32_t opc)
 }
 gen_store_mxu_gpr(t0, xra);
 break;
+
+case OPC_MXU_D16MUL:
+xra = extract32(ctx->opcode, 6, 4);
+xrb = extract32(ctx->opcode, 10, 4);
+xrc = extract32(ctx->opcode, 14, 4);
+xrd = extract32(ctx->opcode, 18, 4);
+optn2 = extract32(ctx->opcode, 22, 2);
+sel = extract32(ctx->opcode, 24, 2);
+
+if (sel == 1) {
+/* D16MULE is not supported */
+generate_exception_end(ctx, EXCP_RI);
+}
+gen_load_mxu_gpr(t1, xrb);
+tcg_gen_sextract_tl(t0, t1, 0, 16);
+tcg_gen_sextract_tl(t1, t1, 16, 16);
+gen_load_mxu_gpr(t3, xrc);
+tcg_gen_sextract_tl(t2, t3, 0, 16);
+tcg_gen_sextract_tl(t3, t3, 16, 16);
+
+switch (optn2) {
+case 0: /* XRB.H*XRC.H == lop, XRB.L*XRC.L == rop */
+tcg_gen_mul_tl(t3, t1, t3);
+tcg_gen_mul_tl(t2, t0, t2);
+break;
+case 1: /* XRB.L*XRC.H == lop, XRB.L*XRC.L == rop */
+tcg_gen_mul_tl(t3, t0, t3);
+tcg_gen_mul_tl(t2, t0, t2);
+break;
+case 2: /* XRB.H*XRC.H == lop, XRB.H*XRC.L == rop */
+tcg_gen_mul_tl(t3, t1, t3);
+tcg_gen_mul_tl(t2, t1, t2);
+break;
+case 3: /* XRB.L*XRC.H == lop, XRB.H*XRC.L == rop */
+tcg_gen_mul_tl(t3, t0, t3);
+tcg_gen_mul_tl(t2, t1, t2);
+break;
+}
+gen_store_mxu_gpr(t3, xra);
+gen_store_mxu_gpr(t2, xrd);
+break;
 }
 
 tcg_temp_free(t0);
 tcg_temp_free(t1);
+tcg_temp_free(t2);
+tcg_temp_free(t3);
 }
 
 /* Godson integer instructions */
@@ -17938,6 +17984,7 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 case OPC_MXU_S32I2M:
 case OPC_MXU_S32M2I:
 case OPC_MXU_S8LDD:
+case OPC_MXU_D16MUL:
 gen_mxu(ctx, op1);
 break;
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH v5 12/13] target/arm: Mark PMINTENSET accesses as possibly doing IO

[Aaron Lindsay]

On Jun 28 17:25, Peter Maydell wrote:
> Shouldn't PMINTENCLR and PMINTENCLR_EL1 also be ARM_CP_IO ?

Yes, that was an oversight - I've added a patch addressing this for v6.

-Aaron

[Qemu-devel] [PATCH v2 5/6] target/mips: Add MXU instructions Q8MUL and Q8MULSU

[Craig Janeczek via Qemu-devel]

Adds support for emulating the Q8MUL and Q8MULSU instructions.

Signed-off-by: Craig Janeczek 
---
 v1
- initial patch
 v2
- changed bitfield usage to extract32

 target/mips/translate.c | 70 -
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 7d37567652..e2def36b03 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -370,6 +370,7 @@ enum {
 OPC_MXU_S8LDD  = 0x22 | OPC_SPECIAL2,
 OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
 OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
+OPC_MXU_Q8MUL  = 0x38 | OPC_SPECIAL2,
 /* Special */
 OPC_SDBBP= 0x3F | OPC_SPECIAL2,
 };
@@ -3774,13 +3775,17 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
 /* MXU Instructions */
 static void gen_mxu(DisasContext *ctx, uint32_t opc)
 {
-TCGv t0, t1, t2, t3;
+TCGv t0, t1, t2, t3, t4, t5, t6, t7;
 uint32_t rb, xra, xrb, xrc, xrd, s8, sel, optn2, optn3, aptn2;
 
 t0 = tcg_temp_new();
 t1 = tcg_temp_new();
 t2 = tcg_temp_new();
 t3 = tcg_temp_new();
+t4 = tcg_temp_new();
+t5 = tcg_temp_new();
+t6 = tcg_temp_new();
+t7 = tcg_temp_new();
 
 switch (opc) {
 case OPC_MXU_S32I2M:
@@ -3951,12 +3956,74 @@ static void gen_mxu(DisasContext *ctx, uint32_t opc)
 gen_store_mxu_gpr(t3, xra);
 gen_store_mxu_gpr(t2, xrd);
 break;
+
+case OPC_MXU_Q8MUL:
+xra = extract32(ctx->opcode, 6, 4);
+xrb = extract32(ctx->opcode, 10, 4);
+xrc = extract32(ctx->opcode, 14, 4);
+xrd = extract32(ctx->opcode, 18, 4);
+sel = extract32(ctx->opcode, 22, 4);
+
+gen_load_mxu_gpr(t3, xrb);
+gen_load_mxu_gpr(t7, xrc);
+
+if (sel == 0x2) {
+/* Q8MULSU */
+tcg_gen_ext8s_tl(t0, t3);
+tcg_gen_shri_tl(t3, t3, 8);
+tcg_gen_ext8s_tl(t1, t3);
+tcg_gen_shri_tl(t3, t3, 8);
+tcg_gen_ext8s_tl(t2, t3);
+tcg_gen_shri_tl(t3, t3, 8);
+tcg_gen_ext8s_tl(t3, t3);
+} else {
+/* Q8MUL */
+tcg_gen_ext8u_tl(t0, t3);
+tcg_gen_shri_tl(t3, t3, 8);
+tcg_gen_ext8u_tl(t1, t3);
+tcg_gen_shri_tl(t3, t3, 8);
+tcg_gen_ext8u_tl(t2, t3);
+tcg_gen_shri_tl(t3, t3, 8);
+tcg_gen_ext8u_tl(t3, t3);
+}
+
+tcg_gen_ext8u_tl(t4, t7);
+tcg_gen_shri_tl(t7, t7, 8);
+tcg_gen_ext8u_tl(t5, t7);
+tcg_gen_shri_tl(t7, t7, 8);
+tcg_gen_ext8u_tl(t6, t7);
+tcg_gen_shri_tl(t7, t7, 8);
+tcg_gen_ext8u_tl(t7, t7);
+
+tcg_gen_mul_tl(t0, t0, t4);
+tcg_gen_mul_tl(t1, t1, t5);
+tcg_gen_mul_tl(t2, t2, t6);
+tcg_gen_mul_tl(t3, t3, t7);
+
+tcg_gen_andi_tl(t0, t0, 0x);
+tcg_gen_andi_tl(t1, t1, 0x);
+tcg_gen_andi_tl(t2, t2, 0x);
+tcg_gen_andi_tl(t3, t3, 0x);
+
+tcg_gen_shli_tl(t1, t1, 16);
+tcg_gen_shli_tl(t3, t3, 16);
+
+tcg_gen_or_tl(t0, t0, t1);
+tcg_gen_or_tl(t1, t2, t3);
+
+gen_store_mxu_gpr(t0, xrd);
+gen_store_mxu_gpr(t1, xra);
+break;
 }
 
 tcg_temp_free(t0);
 tcg_temp_free(t1);
 tcg_temp_free(t2);
 tcg_temp_free(t3);
+tcg_temp_free(t4);
+tcg_temp_free(t5);
+tcg_temp_free(t6);
+tcg_temp_free(t7);
 }
 
 /* Godson integer instructions */
@@ -18045,6 +18112,7 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 case OPC_MXU_S8LDD:
 case OPC_MXU_D16MUL:
 case OPC_MXU_D16MAC:
+case OPC_MXU_Q8MUL:
 gen_mxu(ctx, op1);
 break;
 
-- 
2.18.0

[Qemu-devel] [PATCH v2 6/6] target/mips: Add MXU instructions S32LDD and S32LDDR

[Craig Janeczek via Qemu-devel]

Adds support for emulating the S32LDD and S32LDDR MXU instructions.

Signed-off-by: Craig Janeczek 
---
 v1
- initial patch
 v2
- changed bitfield usage to extract32

 target/mips/translate.c | 43 -
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index e2def36b03..2eb7c02741 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -347,7 +347,8 @@ enum {
 OPC_MSUB = 0x04 | OPC_SPECIAL2,
 OPC_MSUBU= 0x05 | OPC_SPECIAL2,
 /* Loongson 2F */
-OPC_MULT_G_2F   = 0x10 | OPC_SPECIAL2,
+/* opcode 0x10 overlaps loongson and MXU command */
+OPC_MULT_G_2F_MXU_S32LDD   = 0x10 | OPC_SPECIAL2,
 OPC_DMULT_G_2F  = 0x11 | OPC_SPECIAL2,
 OPC_MULTU_G_2F  = 0x12 | OPC_SPECIAL2,
 OPC_DMULTU_G_2F = 0x13 | OPC_SPECIAL2,
@@ -3776,7 +3777,7 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
 static void gen_mxu(DisasContext *ctx, uint32_t opc)
 {
 TCGv t0, t1, t2, t3, t4, t5, t6, t7;
-uint32_t rb, xra, xrb, xrc, xrd, s8, sel, optn2, optn3, aptn2;
+uint32_t rb, xra, xrb, xrc, xrd, s8, s12, sel, optn2, optn3, aptn2;
 
 t0 = tcg_temp_new();
 t1 = tcg_temp_new();
@@ -3858,6 +3859,29 @@ static void gen_mxu(DisasContext *ctx, uint32_t opc)
 gen_store_mxu_gpr(t0, xra);
 break;
 
+case OPC_MULT_G_2F_MXU_S32LDD:
+xra = extract32(ctx->opcode, 6, 4);
+s12 = extract32(ctx->opcode, 10, 10);
+sel = extract32(ctx->opcode, 20, 1);
+rb = extract32(ctx->opcode, 21, 5);
+
+gen_load_gpr(t0, rb);
+
+tcg_gen_movi_tl(t1, s12);
+tcg_gen_shli_tl(t1, t1, 2);
+if (s12 & 0x200) {
+tcg_gen_ori_tl(t1, t1, 0xF000);
+}
+tcg_gen_add_tl(t1, t0, t1);
+tcg_gen_qemu_ld_tl(t1, t1, ctx->mem_idx, MO_SL);
+
+if (sel == 1) {
+/* S32LDDR */
+tcg_gen_bswap32_tl(t1, t1);
+}
+gen_store_mxu_gpr(t1, xra);
+break;
+
 case OPC_MXU_D16MUL:
 xra = extract32(ctx->opcode, 6, 4);
 xrb = extract32(ctx->opcode, 10, 4);
@@ -4039,7 +4063,7 @@ static void gen_loongson_integer(DisasContext *ctx, 
uint32_t opc,
 
 switch (opc) {
 case OPC_MULT_G_2E:
-case OPC_MULT_G_2F:
+case OPC_MULT_G_2F_MXU_S32LDD:
 case OPC_MULTU_G_2E:
 case OPC_MULTU_G_2F:
 #if defined(TARGET_MIPS64)
@@ -4062,7 +4086,7 @@ static void gen_loongson_integer(DisasContext *ctx, 
uint32_t opc,
 
 switch (opc) {
 case OPC_MULT_G_2E:
-case OPC_MULT_G_2F:
+case OPC_MULT_G_2F_MXU_S32LDD:
 tcg_gen_mul_tl(cpu_gpr[rd], t0, t1);
 tcg_gen_ext32s_tl(cpu_gpr[rd], cpu_gpr[rd]);
 break;
@@ -18099,7 +18123,6 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 break;
 case OPC_DIV_G_2F:
 case OPC_DIVU_G_2F:
-case OPC_MULT_G_2F:
 case OPC_MULTU_G_2F:
 case OPC_MOD_G_2F:
 case OPC_MODU_G_2F:
@@ -18116,6 +18139,16 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 gen_mxu(ctx, op1);
 break;
 
+case OPC_MULT_G_2F_MXU_S32LDD:
+/* There is an overlap of opcodes between Loongson2F and MXU */
+if (ctx->insn_flags & INSN_LOONGSON2F) {
+check_insn(ctx, INSN_LOONGSON2F);
+gen_loongson_integer(ctx, op1, rd, rs, rt);
+} else {
+gen_mxu(ctx, op1);
+}
+break;
+
 case OPC_CLO:
 case OPC_CLZ:
 check_insn(ctx, ISA_MIPS32);
-- 
2.18.0

[Qemu-devel] [PATCH v2 0/6] Add limited MXU instruction support

[Craig Janeczek via Qemu-devel]

This patch set begins to add MXU instruction support for mips
emulation.

Craig Janeczek (6):
  target/mips: Add MXU instructions S32I2M and S32M2I
  target/mips: Add MXU instruction S8LDD
  target/mips: Add MXU instruction D16MUL
  target/mips: Add MXU instruction D16MAC
  target/mips: Add MXU instructions Q8MUL and Q8MULSU
  target/mips: Add MXU instructions S32LDD and S32LDDR

 target/mips/cpu.h   |   1 +
 target/mips/translate.c | 345 +++-
 2 files changed, 342 insertions(+), 4 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH v2 1/6] target/mips: Add MXU instructions S32I2M and S32M2I

[Craig Janeczek via Qemu-devel]

This commit makes the MXU registers and the utility functions for
reading/writing to them. This is required for full MXU instruction
support.

Adds support for emulating the S32I2M and S32M2I MXU instructions.

Signed-off-by: Craig Janeczek 
---
 v1
- initial patch
 v2
- Fix checkpatch.pl errors
- remove mips64 ifdef
- changed bitfield usage to extract32
- squashed register addition patch into this one

 target/mips/cpu.h   |  1 +
 target/mips/translate.c | 71 +
 2 files changed, 72 insertions(+)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 009202cf64..4b2948a2c8 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -170,6 +170,7 @@ struct TCState {
 MSACSR_FS_MASK)
 
 float_status msa_fp_status;
+target_ulong mxu_gpr[16];
 };
 
 typedef struct CPUMIPSState CPUMIPSState;
diff --git a/target/mips/translate.c b/target/mips/translate.c
index bdd880bb77..ef819d67e0 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -364,6 +364,9 @@ enum {
 OPC_CLO  = 0x21 | OPC_SPECIAL2,
 OPC_DCLZ = 0x24 | OPC_SPECIAL2,
 OPC_DCLO = 0x25 | OPC_SPECIAL2,
+/* MXU */
+OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
+OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
 /* Special */
 OPC_SDBBP= 0x3F | OPC_SPECIAL2,
 };
@@ -1398,6 +1401,9 @@ static TCGv_i32 fpu_fcr0, fpu_fcr31;
 static TCGv_i64 fpu_f64[32];
 static TCGv_i64 msa_wr_d[64];
 
+/* MXU registers */
+static TCGv mxu_gpr[16];
+
 #include "exec/gen-icount.h"
 
 #define gen_helper_0e0i(name, arg) do {   \
@@ -1517,6 +1523,13 @@ static const char * const msaregnames[] = {
 "w30.d0", "w30.d1", "w31.d0", "w31.d1",
 };
 
+static const char * const mxuregnames[] = {
+"XR1",  "XR2",  "XR3",  "XR4",  "XR5",
+"XR6",  "XR7",  "XR8",  "XR9",  "XR10",
+"XR11", "XR12", "XR13", "XR14", "XR15",
+"XR16",
+};
+
 #define LOG_DISAS(...)\
 do {  \
 if (MIPS_DEBUG_DISAS) {   \
@@ -1550,6 +1563,23 @@ static inline void gen_store_gpr (TCGv t, int reg)
 tcg_gen_mov_tl(cpu_gpr[reg], t);
 }
 
+/* MXU General purpose registers moves. */
+static inline void gen_load_mxu_gpr(TCGv t, int reg)
+{
+if (reg == 0) {
+tcg_gen_movi_tl(t, 0);
+} else {
+tcg_gen_mov_tl(t, mxu_gpr[reg - 1]);
+}
+}
+
+static inline void gen_store_mxu_gpr(TCGv t, int reg)
+{
+if (reg != 0) {
+tcg_gen_mov_tl(mxu_gpr[reg - 1], t);
+}
+}
+
 /* Moves to/from shadow registers. */
 static inline void gen_load_srsgpr (int from, int to)
 {
@@ -3738,6 +3768,35 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
 }
 }
 
+/* MXU Instructions */
+static void gen_mxu(DisasContext *ctx, uint32_t opc)
+{
+TCGv t0;
+uint32_t xra, rb;
+
+t0 = tcg_temp_new();
+
+switch (opc) {
+case OPC_MXU_S32I2M:
+xra = extract32(ctx->opcode, 6, 5);
+rb = extract32(ctx->opcode, 16, 5);
+
+gen_load_gpr(t0, rb);
+gen_store_mxu_gpr(t0, xra);
+break;
+
+case OPC_MXU_S32M2I:
+xra = extract32(ctx->opcode, 6, 5);
+rb = extract32(ctx->opcode, 16, 5);
+
+gen_load_mxu_gpr(t0, xra);
+gen_store_gpr(t0, rb);
+break;
+}
+
+tcg_temp_free(t0);
+}
+
 /* Godson integer instructions */
 static void gen_loongson_integer(DisasContext *ctx, uint32_t opc,
  int rd, int rs, int rt)
@@ -17818,6 +17877,12 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 check_insn(ctx, INSN_LOONGSON2F);
 gen_loongson_integer(ctx, op1, rd, rs, rt);
 break;
+
+case OPC_MXU_S32I2M:
+case OPC_MXU_S32M2I:
+gen_mxu(ctx, op1);
+break;
+
 case OPC_CLO:
 case OPC_CLZ:
 check_insn(ctx, ISA_MIPS32);
@@ -20742,6 +20807,12 @@ void mips_tcg_init(void)
 fpu_fcr31 = tcg_global_mem_new_i32(cpu_env,
offsetof(CPUMIPSState, 
active_fpu.fcr31),
"fcr31");
+
+for (i = 0; i < 16; i++)
+mxu_gpr[i] = tcg_global_mem_new(cpu_env,
+offsetof(CPUMIPSState,
+ active_tc.mxu_gpr[i]),
+mxuregnames[i]);
 }
 
 #include "translate_init.inc.c"
-- 
2.18.0

Re: [Qemu-devel] [PATCH] pc: make sure that guest isn't able to unplug the first cpu

[Igor Mammedov]

On Mon, 27 Aug 2018 16:02:39 +0200
Greg Kurz  wrote:

> On Mon, 27 Aug 2018 13:07:10 +0200
> Igor Mammedov  wrote:
> 
> > The first cpu unplug wasn't ever supported and corresponding
> > monitor/qmp commands refuse to unplug it. However guest is able
> > to issue eject request either using following command:
> >   # echo 1 >/sys/devices/system/cpu/cpu0/firmware_node/eject  
> 
> I can't reproduce the issue with a pc guest and current master...
> 
> All I seem to get is an error in dmesg:
> 
> [   97.435446] processor cpu0: Offline failed.

To be able to unplug CPU0 one need to set CONFIG_BOOTPARAM_HOTPLUG_CPU0=y
(I've tested it with a RHLE7 kernel)
 
> > or directly writing to cpu hotplug registers, which makes
> > qemu crash with SIGSEGV following back trace:
> > 
> >kvm_flush_coalesced_mmio_buffer ()
> >while (ring->first != ring->last)
> >...
> >qemu_flush_coalesced_mmio_buffer
> >prepare_mmio_access
> >flatview_read_continue
> >flatview_read
> >address_space_read_full
> >address_space_rw
> >kvm_cpu_exec(cpu!0)
> >qemu_kvm_cpu_thread_fn
> > 
> > the reason for which is that ring == KVMState::coalesced_mmio_ring
> > happens to be a part of 1st CPU that was uplugged by guest.
> > 
> > Fix it by forbidding 1st cpu unplug from guest side and in addition
> > remove CPU0._EJ0 ACPI method to make clear that unplug of the first
> > CPU is not supported.
> > 
> > Signed-off-by: Igor Mammedov 
> > ---
> > CCing spapr and s390x folks in case targets need to prevent 1st CPU unplug 
> > as well
> >   
> 
> A spapr guest can _release_ the first cpu by doing something like:
> 
> # echo -n "/cpus/PowerPC,POWER8@0" > /sys/devices/system/cpu/release
> 
> But AFAIK, this doesn't unplug the cpu from a QEMU standpoint.
Good,
so only x86 fix should be fixed.


> > CC: m...@redhat.com
> > CC: pbonz...@redhat.com
> > CC: da...@gibson.dropbear.id.au
> > CC: gr...@kaod.org
> > CC: da...@redhat.com
> > CC: coh...@redhat.com
> > ---
> >  hw/acpi/cpu.c | 10 ++
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index 5ae595e..4bb8371 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -117,7 +117,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, 
> > uint64_t data,
> >  DeviceState *dev = NULL;
> >  HotplugHandler *hotplug_ctrl = NULL;
> >  
> > -if (!cdev->cpu) {
> > +if (!cdev->cpu || cdev->cpu == first_cpu) {
> >  trace_cpuhp_acpi_ejecting_invalid_cpu(cpu_st->selector);
> >  break;
> >  }
> > @@ -541,9 +541,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, 
> > CPUHotplugFeatures opts,
> >  aml_buffer(madt_buf->len, (uint8_t *)madt_buf->data)));
> >  g_array_free(madt_buf, true);
> >  
> > -method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> > -aml_append(method, aml_call1(CPU_EJECT_METHOD, uid));
> > -aml_append(dev, method);
> > +if (CPU(arch_ids->cpus[i].cpu) != first_cpu) {
> > +method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> > +aml_append(method, aml_call1(CPU_EJECT_METHOD, uid));
> > +aml_append(dev, method);
> > +}
> >  
> >  method = aml_method("_OST", 3, AML_SERIALIZED);
> >  aml_append(method,  
> 

Re: [Qemu-devel] [Qemu-ppc] [PULL 25/26] spapr_pci: factorize the use of SPAPR_MACHINE_GET_CLASS()

[Greg Kurz]

On Mon, 27 Aug 2018 11:03:39 +0200
Greg Kurz  wrote:

> On Mon, 27 Aug 2018 08:21:48 +0200
> Thomas Huth  wrote:
> 
> > On 2018-08-24 18:43, Cédric Le Goater wrote:  
> > > On 08/24/2018 05:38 PM, Greg Kurz wrote:
> > >> On Fri, 24 Aug 2018 17:30:12 +0200
> > >> Cédric Le Goater  wrote:
> > >>
> > >>> On 08/24/2018 05:09 PM, Peter Maydell wrote:
> >  On 21 August 2018 at 05:33, David Gibson  
> >  wrote:  
> > > From: Cédric Le Goater 
> > [...]  
> > >>> Is there a way to specify which device type can or can not be 
> > >>> plugged on a machine ? 
> > >>>
> > >>> I suppose we cannot use : 
> > >>>
> > >>> machine_class_allow_dynamic_sysbus_dev() 
> > >>>
> > >>> for cold plugged devices. Or can we ? That would be better.
> > >>>
> > >>
> > >> Hmm... not sure this would help. The root problem is that many
> > >> places in spapr_pci and spapr_cpu_core assume the machine is
> > >> sPAPR.
> > > 
> > > which is a perfectly legitimate assumption for a sPAPR only device,
> > > same for spapr_cpu_core. I would think. Shouldn't we enforce 
> > > the restriction at the machine level instead and not at the device
> > > level ? 
> > > 
> > > I thought that was the purpose of commit 0bd1909da606 ("machine: 
> > > Replace has_dynamic_sysbus with list of allowed devices"), to 
> > > make sure machines had a predefined list of user-creatable devices.
> > 
> > The "spapr-pci-host-bridge" is explicitly marked with
> > "dc->user_creatable = true" - so it is creatable everywhere. You could
> > try whether it is possible to make it only creatable via the white list
> > instead  
> 
> Hmm... how would you do that ?
> 

The white list is checked in machine_init_notify() which gets called way after
spapr_phb_realize()... we can't rely on this to check the machine and the PHB
are compatible. Maybe add a dedicated bus for the PHBs in the spapr machine ?

> > ... not sure whether that works though, since there is a class
> > hierarchy (TYPE_PCI_HOST_BRIDGE) in between?
> >   
> 
> Also, as said above, we have the very same problem with spapr_cpu_core,
> which is definitely not a sysbus device...
> 
> Cheers,
> 
> --
> Greg
> 
> >  Thomas  
> 
> 

[Qemu-devel] [Bug 1755912] Re: qemu-system-x86_64 crashed with SIGABRT when using option -vga qxl

[Łukasz Zemczak]

Hello Leonardo, or anyone else affected,

Accepted qemu into bionic-proposed. The package will build now and be
available at https://launchpad.net/ubuntu/+source/qemu/1:2.11+dfsg-
1ubuntu7.5 in a few hours, and then in the -proposed repository.

Please help us by testing this new package.  See
https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how
to enable and use -proposed.Your feedback will aid us getting this
update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug,
mentioning the version of the package you tested and change the tag from
verification-needed-bionic to verification-done-bionic. If it does not
fix the bug for you, please add a comment stating that, and change the
tag to verification-failed-bionic. In either case, without details of
your testing we will not be able to proceed.

Further information regarding the verification process can be found at
https://wiki.ubuntu.com/QATeam/PerformingSRUVerification .  Thank you in
advance!

** Changed in: qemu (Ubuntu Bionic)
   Status: Triaged => Fix Committed

** Tags added: verification-needed verification-needed-bionic

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

Title:
  qemu-system-x86_64 crashed with SIGABRT when using option -vga qxl

Status in QEMU:
  Fix Released
Status in qemu package in Ubuntu:
  Fix Released
Status in qemu source package in Bionic:
  Fix Committed

Bug description:
  [Impact]

   * There are conditions where the vga/qxl driver can crash the qemu 
 process.

   * It is like a very complex case of a non initialized var - without the 
 fix it might try to ask for updates without having a valid primary 
 surface.

   * Backport from upstream
  
https://git.qemu.org/?p=qemu.git;a=commit;h=5bd5c27c7d284d01477c5cc022ce22438c46bf9f
  to avoid the crash

  
  [Test Case]

   * Sometimes booting xubuntu was reported to be enough, at other times
  it was needed to change resolution a few times to trigger.

# get xubuntu iso (actually other UI Isos should do as well)
$ qemu-system-x86_64 -vga qxl -enable-kvm -cpu host -smp cores=2,threads=2 
-m 2048 -cdrom xubuntu-18.04-desktop-amd64.iso
# If it boots successfully, change resolution until it crashes.
$ while true ; do xrandr --output Virtual-0 --mode 640x480 ; sleep 1 ; 
xrandr --output Virtual-0 --mode 1280x720 ; sleep 1 ; xrandr --output Virtual-0 
--mode 1920x1080 ; sleep 1 ; done

   * Without the fix that will trigger the qemu crash

  [Regression Potential]

   * The change "just" adds QXL_MODE_UNDEFINED as one more trigger to leave 
 the rendering update. That sounds rather safe. But thinking hard on 
 potential updates I could think of theoretical setups that were  in 
 undefined mode all the time (unlikely or impossible I think) that now 
 would get no updates anymore. Well I really don't think this is an 
 issue, but since this section should be open thinking on "potential" 
 regressions that is what comes to my mind.

  [Other Info]
   
   * Thanks to Leonardo for most of the bisecting and discussion work!

  
  ---

  
  When using qemu-system-x86_64 with the option -vga qxl, it crashes. The 
easiest way to crash it is by trying to change the guest's resolution. However, 
the system may randomly crash too, not happening only when changing resolution. 
Here is the terminal output of one of these random crashes:

  

  $ qemu-system-x86_64 -hda /dev/sdb -m 2048 -enable-kvm -cpu host -vga qxl 
-nodefaults -netdev user,id=hostnet0 -device 
virtio-net-pci,id=net0,netdev=hostnet0
  WARNING: Image format was not specified for '/dev/sdb' and probing guessed 
raw.
   Automatically detecting the format is dangerous for raw images, 
write operations on block 0 will be restricted.
   Specify the 'raw' format explicitly to remove the restrictions.

  (process:21313): Spice-WARNING **: 16:01:45.759: display-
  channel.c:2431:display_channel_validate_surface: canvas address is
  0x7f8eb948ab18 for 0 (and is NULL)

  (process:21313): Spice-WARNING **: 16:01:45.759: display-
  channel.c:2432:display_channel_validate_surface: failed on 0

  (process:21313): Spice-CRITICAL **: 16:01:45.759: 
display-channel.c:2035:display_channel_update: condition 
`display_channel_validate_surface(display, surface_id)' failed
  Abortado (imagem do núcleo gravada)

  

  I was running QEMU as a normal user which is on the groups kvm and
  disk. Initially I supposed the problem was because I was running QEMU
  as root, but as a normal user this happens too.

  I have tested with guests with different Ubuntu version: 18.04, 17.10
  and 16.04. It is happening with them all.

  ProblemType: Crash
  DistroRelease: Ubuntu 18.04
  Package: qemu-system-x86 1:2.11+dfsg-1ubuntu4
  ProcVersionSignature: Ubuntu 4.15.0-10.11-generic 4.15.3
  Uname: Linux 

[Qemu-devel] [Bug 1780812] Re: Full-Screen Switch Does Nothing When Using SDL

[Thomas Huth]

Fix has been merged:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=6fb34ffcaae0823

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

** Changed in: qemu
 Assignee: (unassigned) => Thomas Huth (th-huth)

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

Title:
  Full-Screen Switch Does Nothing When Using SDL

Status in QEMU:
  Fix Committed

Bug description:
  When using SDL switches, e.g.

  -sdl -full-screen -display sdl

  ... you'd expect the display to start full-screen, as per the switch
  description, but it just starts in a window. Pressing the full-screen
  key combination (Ctrl+Alt+F) enters fullscreen mode as expected.

  Tested on QEmu 2.12.0 using qemu-system-x86_64. OS is Arch Linux.
  Guest is Windows 98 in this test, but happens universally so the guest
  shouldn't be relevant in this instance.

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

[Qemu-devel] [PATCH v2 2/6] target/mips: Add MXU instruction S8LDD

[Craig Janeczek via Qemu-devel]

Adds support for emulating the S8LDD MXU instruction.

Signed-off-by: Craig Janeczek 
---
 v1
- initial patch
 v2
- changed bitfield usage to extract32
- used deposit_tl instructions instead of shift and bitmask

 target/mips/translate.c | 62 +++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index ef819d67e0..f5725d8eda 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -365,6 +365,7 @@ enum {
 OPC_DCLZ = 0x24 | OPC_SPECIAL2,
 OPC_DCLO = 0x25 | OPC_SPECIAL2,
 /* MXU */
+OPC_MXU_S8LDD  = 0x22 | OPC_SPECIAL2,
 OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
 OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
 /* Special */
@@ -3771,10 +3772,11 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
 /* MXU Instructions */
 static void gen_mxu(DisasContext *ctx, uint32_t opc)
 {
-TCGv t0;
-uint32_t xra, rb;
+TCGv t0, t1;
+uint32_t xra, rb, s8, optn3;
 
 t0 = tcg_temp_new();
+t1 = tcg_temp_new();
 
 switch (opc) {
 case OPC_MXU_S32I2M:
@@ -3792,9 +3794,64 @@ static void gen_mxu(DisasContext *ctx, uint32_t opc)
 gen_load_mxu_gpr(t0, xra);
 gen_store_gpr(t0, rb);
 break;
+
+case OPC_MXU_S8LDD:
+xra = extract32(ctx->opcode, 6, 4);
+s8 = extract32(ctx->opcode, 10, 8);
+optn3 = extract32(ctx->opcode, 18, 3);
+rb = extract32(ctx->opcode, 21, 5);
+
+gen_load_gpr(t0, rb);
+tcg_gen_addi_tl(t0, t0, (int8_t)s8);
+switch (optn3) {
+case 0: /*XRa[7:0] = tmp8 */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_UB);
+gen_load_mxu_gpr(t0, xra);
+tcg_gen_deposit_tl(t0, t0, t1, 0, 8);
+break;
+case 1: /* XRa[15:8] = tmp8 */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_UB);
+gen_load_mxu_gpr(t0, xra);
+tcg_gen_deposit_tl(t0, t0, t1, 8, 8);
+break;
+case 2: /* XRa[23:16] = tmp8 */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_UB);
+gen_load_mxu_gpr(t0, xra);
+tcg_gen_deposit_tl(t0, t0, t1, 16, 8);
+break;
+case 3: /* XRa[31:24] = tmp8 */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_UB);
+gen_load_mxu_gpr(t0, xra);
+tcg_gen_deposit_tl(t0, t0, t1, 24, 8);
+break;
+case 4: /* XRa = {8'b0, tmp8, 8'b0, tmp8} */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_UB);
+tcg_gen_deposit_tl(t0, t1, t1, 16, 16);
+break;
+case 5: /* XRa = {tmp8, 8'b0, tmp8, 8'b0} */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_UB);
+tcg_gen_shli_tl(t1, t1, 8);
+tcg_gen_deposit_tl(t0, t1, t1, 16, 16);
+break;
+case 6: /* XRa = {{8{sign of tmp8}}, tmp8, {8{sign of tmp8}}, tmp8} */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_SB);
+tcg_gen_mov_tl(t0, t1);
+tcg_gen_andi_tl(t0, t0, 0xFF00);
+tcg_gen_shli_tl(t1, t1, 16);
+tcg_gen_or_tl(t0, t0, t1);
+break;
+case 7: /* XRa = {tmp8, tmp8, tmp8, tmp8} */
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_UB);
+tcg_gen_deposit_tl(t1, t1, t1, 8, 8);
+tcg_gen_deposit_tl(t0, t1, t1, 16, 16);
+break;
+}
+gen_store_mxu_gpr(t0, xra);
+break;
 }
 
 tcg_temp_free(t0);
+tcg_temp_free(t1);
 }
 
 /* Godson integer instructions */
@@ -17880,6 +17937,7 @@ static void decode_opc_special2_legacy(CPUMIPSState 
*env, DisasContext *ctx)
 
 case OPC_MXU_S32I2M:
 case OPC_MXU_S32M2I:
+case OPC_MXU_S8LDD:
 gen_mxu(ctx, op1);
 break;
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 13/13] block/backup: qapi documentation fixup

[Max Reitz]

On 2018-08-24 00:22, John Snow wrote:
> Fix documentation to match the other jobs amended for 3.1.
> 
> Signed-off-by: John Snow 
> ---
>  qapi/block-core.json | 18 ++
>  1 file changed, 10 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 12/13] qapi/block-stream: expose new job properties

[Max Reitz]

On 2018-08-24 00:22, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  blockdev.c   |  9 +
>  hmp.c|  5 +++--
>  qapi/block-core.json | 16 +++-
>  3 files changed, 27 insertions(+), 3 deletions(-)

[...]

> diff --git a/hmp.c b/hmp.c
> index 2aafb50e8e..e3c3ecd76f 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1865,8 +1865,9 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
>  int64_t speed = qdict_get_try_int(qdict, "speed", 0);
>  
>  qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
> - false, NULL, qdict_haskey(qdict, "speed"), speed,
> - true, BLOCKDEV_ON_ERROR_REPORT, );
> + false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> + BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,

Does this remind me more of Dilbert's RNG or of Wheatley brute-forcing
passwords in Portal 2?

Reviewed-by: Max Reitz 

> + );
>  
>  hmp_handle_error(mon, );
>  }



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] tests: vm: auto_install OpenBSD

[Brad Smith]

On 8/27/2018 5:18 AM, Fam Zheng wrote:


On Fri, 08/24 10:36, Brad Smith wrote:

I very much appreciate the effort to bump up to 6.3 as I was going
to suggest doing that at some point. But bumping up to 6.3 at the
moment will fail with the configure script. We've switched from GCC 4.2
to Clang. The TLS check will fail with Clang's emulated TLS. We've
had a local patch for awhile to fix the test but I don't think it is
appropriate to upstream as is.

Index: configure
--- configure.orig
+++ configure
@@ -1876,7 +1876,7 @@ static __thread int tls_var;
  int main(void) { return tls_var; }
  EOF
-if ! compile_prog "-Werror" "" ; then
+if ! compile_prog "-Werror" "-pthread" ; then
  error_exit "Your compiler does not support the __thread specifier for " \
"Thread-Local Storage (TLS). Please upgrade to a version that does."
  fi

Are you suggesting we use 6.2 for now?


Until the configure script has been fixed you will have to stick with 
6.1 for now.

Re: [Qemu-devel] [PATCH v9 6/6] tpm: add ACPI memory clear interface

[Stefan Berger]

On 08/14/2018 06:02 AM, Marc-André Lureau wrote:



--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -121,6 +121,8 @@ layout:
   +--+++---+
   | next_step|   0x1  |  0x159 | Operation to execute after reboot by  |
   |  ||| firmware. Used by firmware.   |
+ +--+++---+
+ | movv |   0x1  |  0x200 | Memory overwrite variable |
   +--+++---+

why 0x200 and not 0x15a ?


I thought it would be better to leave some room for PPI, but that
probably doesn't help much. Let's move it to 0x15a.


Btw why TPM_PPI_ADDR_SIZE is 0x400 and excess bytes aren't documented anywhere?

I guess for the same reason, Stefan wanted to have some room for
future changes. Is that a problem?

Stefan, can you confirm?

Yes, this was the primary reason.

   Stefan

[Qemu-devel] [PULL v2 2/3] intel-iommu: start to use error_report_once

[Markus Armbruster]

From: Peter Xu 

Replace existing trace_vtd_err() with error_report_once() then stderr
will capture something if any of the error happens, meanwhile we don't
suffer from any DDOS.  Then remove the trace point.  Since at it,
provide more information where proper (now we can pass parameters into
the report function).

Signed-off-by: Peter Xu 
Message-Id: <20180815095328.32414-3-pet...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
[Two format strings fixed, whitespace tidied up]
Signed-off-by: Markus Armbruster 
---
 hw/i386/intel_iommu.c | 65 ---
 hw/i386/trace-events  |  1 -
 2 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 0a8cd4e9cc..9c0f525408 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -311,14 +311,14 @@ static void vtd_generate_fault_event(IntelIOMMUState *s, 
uint32_t pre_fsts)
 {
 if (pre_fsts & VTD_FSTS_PPF || pre_fsts & VTD_FSTS_PFO ||
 pre_fsts & VTD_FSTS_IQE) {
-trace_vtd_err("There are previous interrupt conditions "
-  "to be serviced by software, fault event "
-  "is not generated.");
+error_report_once("There are previous interrupt conditions "
+  "to be serviced by software, fault event "
+  "is not generated");
 return;
 }
 vtd_set_clear_mask_long(s, DMAR_FECTL_REG, 0, VTD_FECTL_IP);
 if (vtd_get_long_raw(s, DMAR_FECTL_REG) & VTD_FECTL_IM) {
-trace_vtd_err("Interrupt Mask set, irq is not generated.");
+error_report_once("Interrupt Mask set, irq is not generated");
 } else {
 vtd_generate_interrupt(s, DMAR_FEADDR_REG, DMAR_FEDATA_REG);
 vtd_set_clear_mask_long(s, DMAR_FECTL_REG, VTD_FECTL_IP, 0);
@@ -426,20 +426,20 @@ static void vtd_report_dmar_fault(IntelIOMMUState *s, 
uint16_t source_id,
 trace_vtd_dmar_fault(source_id, fault, addr, is_write);
 
 if (fsts_reg & VTD_FSTS_PFO) {
-trace_vtd_err("New fault is not recorded due to "
-  "Primary Fault Overflow.");
+error_report_once("New fault is not recorded due to "
+  "Primary Fault Overflow");
 return;
 }
 
 if (vtd_try_collapse_fault(s, source_id)) {
-trace_vtd_err("New fault is not recorded due to "
-  "compression of faults.");
+error_report_once("New fault is not recorded due to "
+  "compression of faults");
 return;
 }
 
 if (vtd_is_frcd_set(s, s->next_frcd_reg)) {
-trace_vtd_err("Next Fault Recording Reg is used, "
-  "new fault is not recorded, set PFO field.");
+error_report_once("Next Fault Recording Reg is used, "
+  "new fault is not recorded, set PFO field");
 vtd_set_clear_mask_long(s, DMAR_FSTS_REG, 0, VTD_FSTS_PFO);
 return;
 }
@@ -447,8 +447,8 @@ static void vtd_report_dmar_fault(IntelIOMMUState *s, 
uint16_t source_id,
 vtd_record_frcd(s, s->next_frcd_reg, source_id, addr, fault, is_write);
 
 if (fsts_reg & VTD_FSTS_PPF) {
-trace_vtd_err("There are pending faults already, "
-  "fault event is not generated.");
+error_report_once("There are pending faults already, "
+  "fault event is not generated");
 vtd_set_frcd_and_update_ppf(s, s->next_frcd_reg);
 s->next_frcd_reg++;
 if (s->next_frcd_reg == DMAR_FRCD_REG_NR) {
@@ -1056,8 +1056,10 @@ static int 
vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
  * we just skip the sync for this time.  After all we even
  * don't have the root table pointer!
  */
-trace_vtd_err("Detected invalid context entry when "
-  "trying to sync shadow page table");
+error_report_once("%s: invalid context entry for bus 0x%x"
+  " devfn 0x%x",
+  __func__, pci_bus_num(vtd_as->bus),
+  vtd_as->devfn);
 return 0;
 }
 }
@@ -1514,7 +1516,8 @@ static uint64_t 
vtd_context_cache_invalidate(IntelIOMMUState *s, uint64_t val)
 break;
 
 default:
-trace_vtd_err("Context cache invalidate type error.");
+error_report_once("%s: invalid context: 0x%" PRIx64,
+  __func__, val);
 caig = 0;
 }
 return caig;
@@ -1634,7 +1637,8 @@ static uint64_t vtd_iotlb_flush(IntelIOMMUState *s, 
uint64_t val)
 am = VTD_IVA_AM(addr);
 addr = VTD_IVA_ADDR(addr);
 if (am > VTD_MAMV) {
-trace_vtd_err("IOTLB PSI flush: address mask overflow.");
+error_report_once("%s: address mask overflow: 0x%" PRIx64,
+  __func__, vtd_get_quad_raw(s, DMAR_IVA_REG));
 iaig 

[Qemu-devel] [PULL v2 1/3] qemu-error: introduce {error|warn}_report_once

[Markus Armbruster]

From: Peter Xu 

There are many error_report()s that can be used in frequently called
functions, especially on IO paths.  That can be unideal in that
malicious guest can try to trigger the error tons of time which might
use up the log space on the host (e.g., libvirt can capture the stderr
of QEMU and put it persistently onto disk).  In VT-d emulation code, we
have trace_vtd_error() tracer.  AFAIU all those places can be replaced
by something like error_report() but trace points are mostly used to
avoid the DDOS attack that mentioned above.  However using trace points
mean that errors are not dumped if trace not enabled.

It's not a big deal in most modern server managements since we have
things like logrotate to maintain the logs and make sure the quota is
expected.  However it'll still be nice that we just provide another way
to restrict message generations.  In most cases, this kind of
error_report()s will only provide valid information on the first message
sent, and all the rest of similar messages will be mostly talking about
the same thing.  This patch introduces *_report_once() helpers to allow
a message to be dumped only once during one QEMU process's life cycle.
It will make sure: (1) it's on by deffault, so we can even get something
without turning the trace on and reproducing, and (2) it won't be
affected by DDOS attack.

To implement it, I stole the printk_once() macro from Linux.

CC: Eric Blake 
CC: Markus Armbruster 
Signed-off-by: Peter Xu 
Message-Id: <20180815095328.32414-2-pet...@redhat.com>
Reviewed-by: Markus Armbruster 
[Whitespace adjusted, comments improved]
Signed-off-by: Markus Armbruster 
---
 include/qemu/error-report.h | 32 
 1 file changed, 32 insertions(+)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e1c8ae1a52..72fab2b031 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -44,6 +44,38 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
+/*
+ * Similar to error_report(), except it prints the message just once.
+ * Return true when it prints, false otherwise.
+ */
+#define error_report_once(fmt, ...) \
+({  \
+static bool print_once_;\
+bool ret_print_once_ = !print_once_;\
+\
+if (!print_once_) { \
+print_once_ = true; \
+error_report(fmt, ##__VA_ARGS__);   \
+}   \
+unlikely(ret_print_once_);  \
+})
+
+/*
+ * Similar to warn_report(), except it prints the message just once.
+ * Return true when it prints, false otherwise.
+ */
+#define warn_report_once(fmt, ...)  \
+({  \
+static bool print_once_;\
+bool ret_print_once_ = !print_once_;\
+\
+if (!print_once_) { \
+print_once_ = true; \
+warn_report(fmt, ##__VA_ARGS__);\
+}   \
+unlikely(ret_print_once_);  \
+})
+
 const char *error_get_progname(void);
 extern bool enable_timestamp_msg;
 
-- 
2.17.1

[Qemu-devel] [PULL v2 3/3] intel-iommu: replace more vtd_err_* traces

[Markus Armbruster]

From: Peter Xu 

Replace all the trace_vtd_err_*() hooks with the new error_report_once()
since they are similar to trace_vtd_err() - dumping the first error
would be mostly enough, then we have them on by default too.

Signed-off-by: Peter Xu 
Message-Id: <20180815095328.32414-4-pet...@redhat.com>
[Use "%x" instead of "%" PRIx16 to print uint16_t, whitespace tidied up]
Signed-off-by: Markus Armbruster 
---
 hw/i386/intel_iommu.c | 64 +++
 hw/i386/trace-events  | 12 
 2 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9c0f525408..3dfada19a6 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -705,7 +705,8 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t 
iova, bool is_write,
 uint64_t access_right_check;
 
 if (!vtd_iova_range_check(iova, ce, aw_bits)) {
-trace_vtd_err_dmar_iova_overflow(iova);
+error_report_once("%s: detected IOVA overflow (iova=0x%" PRIx64 ")",
+  __func__, iova);
 return -VTD_FR_ADDR_BEYOND_MGAW;
 }
 
@@ -717,7 +718,8 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t 
iova, bool is_write,
 slpte = vtd_get_slpte(addr, offset);
 
 if (slpte == (uint64_t)-1) {
-trace_vtd_err_dmar_slpte_read_error(iova, level);
+error_report_once("%s: detected read error on DMAR slpte "
+  "(iova=0x%" PRIx64 ")", __func__, iova);
 if (level == vtd_ce_get_level(ce)) {
 /* Invalid programming of context-entry */
 return -VTD_FR_CONTEXT_ENTRY_INV;
@@ -728,11 +730,17 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, 
uint64_t iova, bool is_write,
 *reads = (*reads) && (slpte & VTD_SL_R);
 *writes = (*writes) && (slpte & VTD_SL_W);
 if (!(slpte & access_right_check)) {
-trace_vtd_err_dmar_slpte_perm_error(iova, level, slpte, is_write);
+error_report_once("%s: detected slpte permission error "
+  "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
+  "slpte=0x%" PRIx64 ", write=%d)", __func__,
+  iova, level, slpte, is_write);
 return is_write ? -VTD_FR_WRITE : -VTD_FR_READ;
 }
 if (vtd_slpte_nonzero_rsvd(slpte, level)) {
-trace_vtd_err_dmar_slpte_resv_error(iova, level, slpte);
+error_report_once("%s: detected splte reserve non-zero "
+  "iova=0x%" PRIx64 ", level=0x%" PRIx32
+  "slpte=0x%" PRIx64 ")", __func__, iova,
+  level, slpte);
 return -VTD_FR_PAGING_ENTRY_RSVD;
 }
 
@@ -1697,7 +1705,10 @@ static void vtd_handle_gcmd_qie(IntelIOMMUState *s, bool 
en)
 /* Ok - report back to driver */
 vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_QIES, 0);
 } else {
-trace_vtd_err_qi_disable(s->iq_head, s->iq_tail, 
s->iq_last_desc_type);
+error_report_once("%s: detected improper state when disable QI "
+  "(head=0x%x, tail=0x%x, last_type=%d)",
+  __func__,
+  s->iq_head, s->iq_tail, s->iq_last_desc_type);
 }
 }
 }
@@ -2094,7 +2105,9 @@ static void vtd_fetch_inv_desc(IntelIOMMUState *s)
 
 if (s->iq_tail >= s->iq_size) {
 /* Detects an invalid Tail pointer */
-trace_vtd_err_qi_tail(s->iq_tail, s->iq_size);
+error_report_once("%s: detected invalid QI tail "
+  "(tail=0x%x, size=0x%x)",
+  __func__, s->iq_tail, s->iq_size);
 vtd_handle_inv_queue_error(s);
 return;
 }
@@ -2507,10 +2520,12 @@ static IOMMUTLBEntry 
vtd_iommu_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
  iotlb.iova, iotlb.translated_addr,
  iotlb.addr_mask);
 } else {
-trace_vtd_err_dmar_translate(pci_bus_num(vtd_as->bus),
- VTD_PCI_SLOT(vtd_as->devfn),
- VTD_PCI_FUNC(vtd_as->devfn),
- iotlb.iova);
+error_report_once("%s: detected translation failure "
+  "(dev=%02x:%02x:%02x, iova=0x%" PRIx64 ")",
+  __func__, pci_bus_num(vtd_as->bus),
+  VTD_PCI_SLOT(vtd_as->devfn),
+  VTD_PCI_FUNC(vtd_as->devfn),
+  iotlb.iova);
 }
 
 return iotlb;
@@ -2626,15 +2641,19 @@ static int vtd_irte_get(IntelIOMMUState *iommu, 
uint16_t index,
   le64_to_cpu(entry->data[0]));
 
 if (!entry->irte.present) {
-trace_vtd_err_irte(index, le64_to_cpu(entry->data[1]),
-   

Re: [Qemu-devel] [PATCH v2 09/13] jobs: remove .exit callback

[Max Reitz]

On 2018-08-24 00:22, John Snow wrote:
> Now that all of the jobs use the component finalization callbacks,
> there's no use for the heavy-hammer .exit callback anymore.
> 
> job_exit becomes a glorified type shim so that we can call
> job_completed from aio_bh_schedule_oneshot.
> 
> Move these three functions down into job.c to eliminate a
> forward reference.
> 
> Signed-off-by: John Snow 
> ---
>  include/qemu/job.h| 11 ---
>  job.c | 77 
> +--
>  tests/test-blockjob-txn.c |  4 +--
>  3 files changed, 36 insertions(+), 56 deletions(-)

[...]

> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index ef29f35e44..86606f92b3 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -24,7 +24,7 @@ typedef struct {
>  int *result;
>  } TestBlockJob;
>  
> -static void test_block_job_exit(Job *job)
> +static void test_block_job_clean(Job *job)
>  {
>  BlockJob *bjob = container_of(job, BlockJob, job);
>  BlockDriverState *bs = blk_bs(bjob->blk);
> @@ -73,7 +73,7 @@ static const BlockJobDriver test_block_job_driver = {
>  .user_resume   = block_job_user_resume,
>  .drain = block_job_drain,
>  .run   = test_block_job_run,
> -.exit  = test_block_job_exit,
> +.clean = test_block_job_clean,
>  },
>  };

Not sure whether this change warrants its own patch, but it probably
should be noted in the commit message.

With that done (or with this change split off into its own patch):

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] pc: make sure that guest isn't able to unplug the first cpu

[Greg Kurz]

On Mon, 27 Aug 2018 13:07:10 +0200
Igor Mammedov  wrote:

> The first cpu unplug wasn't ever supported and corresponding
> monitor/qmp commands refuse to unplug it. However guest is able
> to issue eject request either using following command:
>   # echo 1 >/sys/devices/system/cpu/cpu0/firmware_node/eject

I can't reproduce the issue with a pc guest and current master...

All I seem to get is an error in dmesg:

[   97.435446] processor cpu0: Offline failed.

> or directly writing to cpu hotplug registers, which makes
> qemu crash with SIGSEGV following back trace:
> 
>kvm_flush_coalesced_mmio_buffer ()
>while (ring->first != ring->last)
>...
>qemu_flush_coalesced_mmio_buffer
>prepare_mmio_access
>flatview_read_continue
>flatview_read
>address_space_read_full
>address_space_rw
>kvm_cpu_exec(cpu!0)
>qemu_kvm_cpu_thread_fn
> 
> the reason for which is that ring == KVMState::coalesced_mmio_ring
> happens to be a part of 1st CPU that was uplugged by guest.
> 
> Fix it by forbidding 1st cpu unplug from guest side and in addition
> remove CPU0._EJ0 ACPI method to make clear that unplug of the first
> CPU is not supported.
> 
> Signed-off-by: Igor Mammedov 
> ---
> CCing spapr and s390x folks in case targets need to prevent 1st CPU unplug as 
> well
> 

A spapr guest can _release_ the first cpu by doing something like:

# echo -n "/cpus/PowerPC,POWER8@0" > /sys/devices/system/cpu/release

But AFAIK, this doesn't unplug the cpu from a QEMU standpoint.

> CC: m...@redhat.com
> CC: pbonz...@redhat.com
> CC: da...@gibson.dropbear.id.au
> CC: gr...@kaod.org
> CC: da...@redhat.com
> CC: coh...@redhat.com
> ---
>  hw/acpi/cpu.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 5ae595e..4bb8371 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -117,7 +117,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, 
> uint64_t data,
>  DeviceState *dev = NULL;
>  HotplugHandler *hotplug_ctrl = NULL;
>  
> -if (!cdev->cpu) {
> +if (!cdev->cpu || cdev->cpu == first_cpu) {
>  trace_cpuhp_acpi_ejecting_invalid_cpu(cpu_st->selector);
>  break;
>  }
> @@ -541,9 +541,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, 
> CPUHotplugFeatures opts,
>  aml_buffer(madt_buf->len, (uint8_t *)madt_buf->data)));
>  g_array_free(madt_buf, true);
>  
> -method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> -aml_append(method, aml_call1(CPU_EJECT_METHOD, uid));
> -aml_append(dev, method);
> +if (CPU(arch_ids->cpus[i].cpu) != first_cpu) {
> +method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> +aml_append(method, aml_call1(CPU_EJECT_METHOD, uid));
> +aml_append(dev, method);
> +}
>  
>  method = aml_method("_OST", 3, AML_SERIALIZED);
>  aml_append(method,

[Qemu-devel] [PULL v2 0/3] Error reporting patches for 2018-08-27

[Markus Armbruster]

The following changes since commit 235c82acca0491465e94be3cae2583b42d37c859:

  Merge remote-tracking branch 'remotes/otubo/tags/pull-seccomp-20180823' into 
staging (2018-08-25 13:08:57 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-error-2018-08-27-v2

for you to fetch changes up to 4e4abd111a2af0179a4467368d695958844bf113:

  intel-iommu: replace more vtd_err_* traces (2018-08-27 15:09:20 +0200)


Error reporting patches for 2018-08-27

* Provide error_report_once(), along with first users


Peter Xu (3):
  qemu-error: introduce {error|warn}_report_once
  intel-iommu: start to use error_report_once
  intel-iommu: replace more vtd_err_* traces

 hw/i386/intel_iommu.c   | 129 
 hw/i386/trace-events|  13 -
 include/qemu/error-report.h |  32 +++
 3 files changed, 115 insertions(+), 59 deletions(-)

-- 
2.17.1

Re: [Qemu-devel] [PATCH] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

On Mon, 27 Aug 2018 14:13:43 +0200
Andrew Jones  wrote:

> On Mon, Aug 27, 2018 at 01:56:08PM +0200, Igor Mammedov wrote:
> > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > so that total number of logical CPUs [sockets * cores * threads]
> > would be equal to [maxcpus], however historically we didn't have
> > such check in QEMU and it is possible to start VM with an invalid
> > topology.
> > Deprecate invalid options combination so we can make sure that
> > the topology VM started with is always correct in the future.
> > Users with an invalid sockets/cores/threads/maxcpus values should
> > fix their CLI to make sure that
> >[sockets * cores * threads] == [maxcpus]
> > 
> > Signed-off-by: Igor Mammedov 
> > ---
> >  qemu-deprecated.texi | 11 +++
> >  vl.c |  6 ++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 87212b6..d5d9ce6 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -159,6 +159,17 @@ The 'file' driver for drives is no longer appropriate 
> > for character or host
> >  devices and will only accept regular files (S_IFREG). The correct driver
> >  for these file types is 'host_cdrom' or 'host_device' as appropriate.
> >  
> > +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)  
> 
> sockets, cores, threads
> 
> > +
> > +CPU topology properties should describe whole machine topology including
> > +possible CPUs. but historically it was possible to start QEMU with  
> 
> /./,/
> 
> > +an incorrect topology where
> > +  socket * core * thread >= X && < maxcpus  
> 
> sockets * cores * threads
> 
> && X < maxcpus
> 
> > +which could lead to incorrect topology enumeration by the guest.
> > +Support for invalid topology will be removed, end user must ensure  
> topologies
> /end user/the user/
> 
> > +that topology described with -smp includes all possible cpus, i.e.:  
> /that/the
> 
> > +  socket * core * thread == maxcpus  
> 
> sockets * cores * threads
> 
> > +
> >  @section QEMU Machine Protocol (QMP) commands
> >  
> >  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> > diff --git a/vl.c b/vl.c
> > index 5ba06ad..bc53828 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1238,6 +1238,12 @@ static void smp_parse(QemuOpts *opts)
> >  exit(1);
> >  }
> >  
> > +if (sockets * cores * threads != max_cpus) {
> > +warn_report("Invalid CPUs topology deprecated. "  
> 
> /CPUs/CPU/
> 
> Not sure we want a period after deprecated. Would ':' be more appropriate?
> 
> > +"sockets (%u) * cores (%u) * threads (%u) "
> > +"!= maxcpus (%u)",
> > +sockets, cores, threads, max_cpus);
> > +}
> >  if (sockets * cores * threads > max_cpus) {
> >  error_report("cpu topology: "
> >   "sockets (%u) * cores (%u) * threads (%u) > "
> > -- 
> > 2.7.4
> >  
> 
> Thanks,
> drew 
> 

Thanks for corrections, v2 is on the way

Re: [Qemu-devel] [PATCH] input-linux: toggle for lock keys

[Ryan El Kochta via Qemu-devel]

I personally use the Scroll Lock key. On Linux, Scroll Lock (along with all the 
other lock keys) is easy to disable with an xmodmap command. On Windows (or, 
I'd assume, other guests), it requires third party software, which is why it's 
likely easier to just filter them out from QEMU.
 Original Message 
On Aug 27, 2018, 6:16 AM, kra...@redhat.com wrote:

> On Thu, Aug 23, 2018 at 01:23:20AM +, Ryan El Kochta wrote:
>> This patch introduces three new options on the input-linux commandline:
>>
>> (a) ignore_caps_lock=[on|off]
>> (b) ignore_num_lock=[on|off]
>> (c) ignore_scroll_lock=[on|off]
>>
>> If enabled, the key will be disabled and not forwarded to the guest.
>> There are two main reasons for this:
>>
>> (a) Without keyboard LEDs, it can be difficult to tell whether it is
>> enabled or not
>> (b) Preparation for another patch which will allow changing the keys
>> used to toggle the input device's grab. If you set the key to
>> KEY_SCROLLLOCK for example, it can be frustrating disabling scroll lock
>> in the guest, as it will require pressing the key more than twice.
>
> Hmm. You have the same issue on the host, right?
>
> For the guest we can easily filter out the key events.
> For the host we can't. This is the reason I picked a
> key combination which has no side effects to switch
> between host and guest. And for the same reason I don't
> think it is useful to allow the user to specify arbitrary
> key combinations.
>
> Which combination do you want use instead of leftshift + rightshift?
>
> cheers,
> Gerd

[Qemu-devel] [PATCH v2] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

-smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
so that total number of logical CPUs [sockets * cores * threads]
would be equal to [maxcpus], however historically we didn't have
such check in QEMU and it is possible to start VM with an invalid
topology.
Deprecate invalid options combination so we can make sure that
the topology VM started with is always correct in the future.
Users with an invalid sockets/cores/threads/maxcpus values should
fix their CLI to make sure that
   [sockets * cores * threads] == [maxcpus]

Signed-off-by: Igor Mammedov 
---
v2:
  - spelling& fixes (Andrew Jones )
---
 qemu-deprecated.texi | 11 +++
 vl.c |  6 ++
 2 files changed, 17 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 87212b6..b649b2e 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -159,6 +159,17 @@ The 'file' driver for drives is no longer appropriate for 
character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
+@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
+
+CPU topology properties should describe whole machine topology including
+possible CPUs. but historically it was possible to start QEMU with
+an incorrect topology where
+  sockets * cores * threads >= X && X < maxcpus
+which could lead to incorrect topology enumeration by the guest.
+Support for invalid topology will be removed, the user must ensure
+topologies described with -smp includes all possible cpus, i.e.:
+  sockets * cores * threads == maxcpus
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
diff --git a/vl.c b/vl.c
index 5ba06ad..238f856 100644
--- a/vl.c
+++ b/vl.c
@@ -1238,6 +1238,12 @@ static void smp_parse(QemuOpts *opts)
 exit(1);
 }
 
+if (sockets * cores * threads != max_cpus) {
+warn_report("Invalid CPU topology deprecated: "
+"sockets (%u) * cores (%u) * threads (%u) "
+"!= maxcpus (%u)",
+sockets, cores, threads, max_cpus);
+}
 if (sockets * cores * threads > max_cpus) {
 error_report("cpu topology: "
  "sockets (%u) * cores (%u) * threads (%u) > "
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 10/13] qapi/block-commit: expose new job properties

[Max Reitz]

On 2018-08-24 00:22, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  blockdev.c   |  8 
>  qapi/block-core.json | 16 +++-
>  2 files changed, 23 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 00/12] Ui 20180827 v4 patches

[Peter Maydell]

On 27 August 2018 at 09:53, Gerd Hoffmann  wrote:
> The following changes since commit 5ccac548faf041ff5229a8e8342e3be14a34c8af:
>
>   Merge remote-tracking branch 'remotes/cody/tags/block-pull-request' into 
> staging (2018-08-23 17:35:48 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/ui-20180827-v4-pull-request
>
> for you to fetch changes up to b1d380372f31672da9318431e84e79bccd8ef3bf:
>
>   util: promote qemu_egl_rendernode_open() to libqemuutil (2018-08-27 
> 10:51:44 +0200)
>
> 
> ui: misc fixes which piled up during 3.0 release freeze
>
> 
Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v2 11/13] qapi/block-mirror: expose new job properties

[Max Reitz]

On 2018-08-24 00:22, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  blockdev.c   | 14 ++
>  qapi/block-core.json | 30 --
>  2 files changed, 42 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] 40p: fix PCI interrupt routing

[Mark Cave-Ayland]

On 27/08/18 12:05, Mark Cave-Ayland wrote:

> According to the PReP specification section 6.1.6 "System Interrupt
> Assignments", all PCI interrupts are routed via IRQ 15.
> 
> With this patch applied it is now possible to boot the sandalfoot
> zImage all the way through to a working userspace when using
> OpenBIOS.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/ppc/prep.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 162b27a3b8..e82c1355d9 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -668,10 +668,11 @@ static void ibm_40p_init(MachineState *machine)
>  dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(11, 0), "i82378"));
>  qdev_connect_gpio_out(dev, 0,
>cpu->env.irq_inputs[PPC6xx_INPUT_INT]);
> -sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(dev, 15));
> -sysbus_connect_irq(pcihost, 1, qdev_get_gpio_in(dev, 13));
> -sysbus_connect_irq(pcihost, 2, qdev_get_gpio_in(dev, 15));
> -sysbus_connect_irq(pcihost, 3, qdev_get_gpio_in(dev, 13));
> +/* According to PReP specification section 6.1.6 "System Interrupt
> + * Assignments", all PCI interrupts are routed via IRQ 15 */
> +for (i = 0; i < PCI_NUM_PINS; i++) {
> +sysbus_connect_irq(pcihost, i, qdev_get_gpio_in(dev, 15));
> +}
>  isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>  
>  /* Memory controller */

Actually it seems that this is just working around the fact that a real
40p machine has certain hard-coded interrupts which only get used when
the residual data sets the machine name to "IBM PPS Model 6015".

In fact if you apply the following diff to OpenBIOS:

diff --git a/arch/ppc/qemu/context.c b/arch/ppc/qemu/context.c
index 06e0122..5815895 100644
--- a/arch/ppc/qemu/context.c
+++ b/arch/ppc/qemu/context.c
@@ -111,7 +111,7 @@ static void *
 residual_build(uint32_t memsize, uint32_t load_base, uint32_t load_size)
 {
 residual_t *res;
-const unsigned char model[] = "Qemu\0PPC\0";
+const unsigned char model[] = "IBM PPS Model 6015\0";
 int i;

 res = malloc(sizeof(residual_t));

then QEMU/OpenBIOS can boot a working sandalfoot zImage but only by
coincidence: it just so happens that the PCI IRQ for device 12 maps to
PCI IRQ 2 using the standard algorithm, which in the original code above
maps to IRQ 13 which is reserved on some PReP machines for the SCSI
controller.

So I think there are 2 issues here:

1) The PReP interrupt controller needs to be remodelled to allow PCI
IRQs to be mapped in this way

This is of course possible, but would require implementing logic that
wouldn't be compatible with the existing -M prep machine. But should
this wait until the old -M prep machine has been removed?

2) Setting the machine name as above breaks NetBSD boot

As per this bug report from Artyom:
http://mail-index.netbsd.org/port-prep/2017/04/02/msg000109.html. I'm
inclined to think that the solution here is to get NetBSD to fix their code.

Hervé, what do you think is the best solution for now?


ATB,

Mark.

Re: [Qemu-devel] [PATCH v2 08/13] tests/test-blockjob: remove exit callback

[Max Reitz]

On 2018-08-24 00:22, John Snow wrote:
> We remove the exit callback and the completed boolean along with it.
> We can simulate it just fine by waiting for the job to defer to the
> main loop, and then giving it one final kick to get the main loop
> portion to run.
> 
> Signed-off-by: John Snow 
> ---
>  tests/test-blockjob.c | 16 ++--
>  1 file changed, 6 insertions(+), 10 deletions(-)

Hm, well, sure, but I'd think deferred_to_main_loop is an internal
attribute.  Considering this is just a test, it doesn't matter, though, so:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature