Re: [PATCH v5 07/11] hw/char: Initial commit of Ibex UART

[Alistair Francis]

On Wed, Jun 3, 2020 at 10:06 PM LIU Zhiwei  wrote:
>
>
>
> On 2020/6/4 12:35, Alistair Francis wrote:
> > On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei  wrote:
> >>
> >>
> >> On 2020/6/3 23:56, Alistair Francis wrote:
> >>> On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei  wrote:
>  On 2020/6/3 1:54, Alistair Francis wrote:
> > On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei  wrote:
> >> Hi Alistair,
> >>
> >> There are still some questions I don't understand.
> >>
> >> 1. Is the baud rate  or fifo a necessary feature to simulate?
> >> As you can see, qemu_chr_fe_write will send the byte as soon as 
> >> possible.
> >> When you want to transmit a byte through WDATA,  you can call
> >> qemu_chr_fe_write directly.
> > So qemu_chr_fe_write() will send the data straight away. This doesn't
> > match what teh hardware does though. So by modelling a FIFO and a
> > delay in sending we can better match the hardware.
>  I see many UARTs have similar features. Does the software really care 
>  about
>  these features? Usually I just want to print something to the terminal
>  through UART.
> >>> In this case Tock (which is the OS used for OpenTitan) does car about
> >>> these features as it relies on interrupts generated by the HW to
> >>> complete the serial send task. It also just makes the QEMU model more
> >>> accurate.
> >> Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
> >> At least,
> >> it can verify the ISP.
> > Exactly :)
> >
>  Most simulation in QEMU is for running software, not exactly the details
>  of hardware.
>  For example, we will not simulate the 16x oversamples in this UART.
> >>> Agreed. Lots of UARTs don't bother modelling the delay from the
> >>> hardware as generally it doesn't matter. In this case it does make a
> >>> difference for the software and it makes the QEMU model more accurate,
> >>> which is always a good thing.
> >>>
>  There is no error here. Personally I  think it is necessary to simulate
>  the FIFO and baud rate,
>  maybe for supporting some backends.
> >>> So baud rate doesn't need to be modelled as we aren't actually sending
> >>> UART data, just pretending and then printing it.
> >>>
>  Can someone give a reasonable answer for this question?
> >>> Which question?
> >> I see  the UART can work with many  different backends,  such as pty ,
> >> file, socket and so on.
> >> I wonder if this a backend, which has some requirements on the baud
> > The backend should be independent so it doesn't matter what baud rate
> > we choose here.
> >
> >> rate.  You can ignore it,
> >> as it doesn't matter.
> >> 2.  The baud rate calculation method is not strictly right.
> >> I think when a byte write to FIFO,  char_tx_time * 8 is the correct 
> >> time
> >> to send the byte instead of
> >> char_tx_time * 4.
> > Do you mind explaining why 8 is correct instead of 4?
>  Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
>  Translate a bit will take
>  char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
> >>> I see your point. I just used the 4 as that is what the Cadence one
> >>> does. I don't think it matters too much as it's just the delay for a
> >>> timer (that isn't used as an accurate timer).
> >> Got it. Just a way to send the bytes at sometime later.
> >> 3.  Why add a watch here?
> > This is based on the Cadence UART implementation in QEMU (which does
> > the same thing). This will trigger a callback when we can write more
> > data or when the backend has hung up.
>  Many other serials do the same thing, like virtio-console and serial. So
>  it may be a common
>  interface here. I will try to understand it(Not yet).
> >>> Yep, it's just a more complete model of that the HW does.
> >> I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
> >> function.
> >> The breakpoint did't match.
> >>
> >> I just wonder if there is a case really need the callback function.
> > AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
> > shouldn't be triggered.
> I tried with the UART in the virt board.  It also registers the watch
> callback on
> G_IO_HUP and G_IO_OUT.
>
> However I will through the question alone in another mail.

Ah, sorry I misunderstood what you meant.

I haven't looked at it, it's possible it's not enabled by Linux.

>
> After the two points addressed,
>
> Reviewed-by: LIU Zhiwei

Thanks!

Alistair

>
> Zhiwei
> >
> > Alistair
> >
> >> Zhiwei
> >>> Alistair
>
>
>

Re: [PATCH v5 07/11] hw/char: Initial commit of Ibex UART

[LIU Zhiwei]

On 2020/6/4 12:35, Alistair Francis wrote:

On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei  wrote:



On 2020/6/3 23:56, Alistair Francis wrote:

On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei  wrote:

On 2020/6/3 1:54, Alistair Francis wrote:

On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei  wrote:

Hi Alistair,

There are still some questions I don't understand.

1. Is the baud rate  or fifo a necessary feature to simulate?
As you can see, qemu_chr_fe_write will send the byte as soon as possible.
When you want to transmit a byte through WDATA,  you can call
qemu_chr_fe_write directly.

So qemu_chr_fe_write() will send the data straight away. This doesn't
match what teh hardware does though. So by modelling a FIFO and a
delay in sending we can better match the hardware.

I see many UARTs have similar features. Does the software really care about
these features? Usually I just want to print something to the terminal
through UART.

In this case Tock (which is the OS used for OpenTitan) does car about
these features as it relies on interrupts generated by the HW to
complete the serial send task. It also just makes the QEMU model more
accurate.

Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
At least,
it can verify the ISP.

Exactly :)


Most simulation in QEMU is for running software, not exactly the details
of hardware.
For example, we will not simulate the 16x oversamples in this UART.

Agreed. Lots of UARTs don't bother modelling the delay from the
hardware as generally it doesn't matter. In this case it does make a
difference for the software and it makes the QEMU model more accurate,
which is always a good thing.


There is no error here. Personally I  think it is necessary to simulate
the FIFO and baud rate,
maybe for supporting some backends.

So baud rate doesn't need to be modelled as we aren't actually sending
UART data, just pretending and then printing it.


Can someone give a reasonable answer for this question?

Which question?

I see  the UART can work with many  different backends,  such as pty ,
file, socket and so on.
I wonder if this a backend, which has some requirements on the baud

The backend should be independent so it doesn't matter what baud rate
we choose here.


rate.  You can ignore it,
as it doesn't matter.

2.  The baud rate calculation method is not strictly right.
I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
to send the byte instead of
char_tx_time * 4.

Do you mind explaining why 8 is correct instead of 4?

Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
Translate a bit will take
char_tx_time. So it will take char_tx_time * 8 to transmit a byte.

I see your point. I just used the 4 as that is what the Cadence one
does. I don't think it matters too much as it's just the delay for a
timer (that isn't used as an accurate timer).

Got it. Just a way to send the bytes at sometime later.

3.  Why add a watch here?

This is based on the Cadence UART implementation in QEMU (which does
the same thing). This will trigger a callback when we can write more
data or when the backend has hung up.

Many other serials do the same thing, like virtio-console and serial. So
it may be a common
interface here. I will try to understand it(Not yet).

Yep, it's just a more complete model of that the HW does.

I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
function.
The breakpoint did't match.

I just wonder if there is a case really need the callback function.

AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
shouldn't be triggered.
I tried with the UART in the virt board.  It also registers the watch 
callback on

G_IO_HUP and G_IO_OUT.

However I will throw the question alone in another thread.

After the two comments pointed at another email addressed,

Reviewed-by: LIU Zhiwei

Zhiwei


Alistair


Zhiwei

Alistair

Re: [PATCH v2 2/2] pci: ensure configuration access is within bounds

[P J P]

+-- On Thu, 4 Jun 2020, BALATON Zoltan wrote --+
| On Thu, 4 Jun 2020, P J P wrote:
| > +assert(address + len <= pci_config_size(d));
| 
| Does this allow guest now to crash QEMU?

Yes, possible. Such crash (assert failure) can be a regular bug, as reading 
PCI configuration is likely a privileged operation inside guest.

| If this is considered to be an error now to call this function with wrong 
| parameters did you check other callers?

No, I haven't checked all other cases.

| Would it be better to not crash just log invalid access and either fix up 
| parameters or return some garbage like 0?

* Earlier patch v1 did the same, returned 0.

* Assert(3) may help to fix current and future incorrect usage of the call.

@mst ...?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v2 2/2] pci: ensure configuration access is within bounds

[Gerd Hoffmann]

  Hi,

> > +assert(address + len <= pci_config_size(d));
> 
> Does this allow guest now to crash QEMU?

Looks like it does (didn't actually try though).

> I think it was suggested that assert should only be used for cases
> that can only arise from a programming error and not from values set
> by the guest.

Correct.  We do have guest-triggerable asserts in the code base.  They
are not the end of the world as the guest will only hurt itself.  But
in general we try to get rid of them instead of adding new ones ...

Often you can just ignore the illegal guest action (bonus points for
logging GUEST_ERROR as debugging aid).  Sometimes it is more difficult
to deal with it (in case the hardware is expected to throw an error irq
for example).

take care,
  Gerd

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Thomas Huth]

On 04/06/2020 05.45, Richard Henderson wrote:
> Clang 10 enables this by default with -Wtype-limit.
> 
> All of the instances flagged by this Werror so far have been
> cases in which we really do want the compiler to optimize away
> the test completely.  Disabling the warning will avoid having
> to add ifdefs to work around this.
> 
> Signed-off-by: Richard Henderson 
> ---
>  configure | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/configure b/configure
> index f087d2bcd1..693f01327f 100755
> --- a/configure
> +++ b/configure
> @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body 
> -Wnested-externs $gcc_flags"
>  gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
>  gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
>  gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags"
> +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare"
> +
>  # Note that we do not add -Werror to gcc_flags here, because that would
>  # enable it for all configure tests. If a configure test failed due
>  # to -Werror this would just silently disable some features,

Acked-by: Thomas Huth 

Re: [PATCH v5 07/11] hw/char: Initial commit of Ibex UART

[LIU Zhiwei]

On 2020/6/4 12:35, Alistair Francis wrote:

On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei  wrote:



On 2020/6/3 23:56, Alistair Francis wrote:

On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei  wrote:

On 2020/6/3 1:54, Alistair Francis wrote:

On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei  wrote:

Hi Alistair,

There are still some questions I don't understand.

1. Is the baud rate  or fifo a necessary feature to simulate?
As you can see, qemu_chr_fe_write will send the byte as soon as possible.
When you want to transmit a byte through WDATA,  you can call
qemu_chr_fe_write directly.

So qemu_chr_fe_write() will send the data straight away. This doesn't
match what teh hardware does though. So by modelling a FIFO and a
delay in sending we can better match the hardware.

I see many UARTs have similar features. Does the software really care about
these features? Usually I just want to print something to the terminal
through UART.

In this case Tock (which is the OS used for OpenTitan) does car about
these features as it relies on interrupts generated by the HW to
complete the serial send task. It also just makes the QEMU model more
accurate.

Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
At least,
it can verify the ISP.

Exactly :)


Most simulation in QEMU is for running software, not exactly the details
of hardware.
For example, we will not simulate the 16x oversamples in this UART.

Agreed. Lots of UARTs don't bother modelling the delay from the
hardware as generally it doesn't matter. In this case it does make a
difference for the software and it makes the QEMU model more accurate,
which is always a good thing.


There is no error here. Personally I  think it is necessary to simulate
the FIFO and baud rate,
maybe for supporting some backends.

So baud rate doesn't need to be modelled as we aren't actually sending
UART data, just pretending and then printing it.


Can someone give a reasonable answer for this question?

Which question?

I see  the UART can work with many  different backends,  such as pty ,
file, socket and so on.
I wonder if this a backend, which has some requirements on the baud

The backend should be independent so it doesn't matter what baud rate
we choose here.


rate.  You can ignore it,
as it doesn't matter.

2.  The baud rate calculation method is not strictly right.
I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
to send the byte instead of
char_tx_time * 4.

Do you mind explaining why 8 is correct instead of 4?

Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
Translate a bit will take
char_tx_time. So it will take char_tx_time * 8 to transmit a byte.

I see your point. I just used the 4 as that is what the Cadence one
does. I don't think it matters too much as it's just the delay for a
timer (that isn't used as an accurate timer).

Got it. Just a way to send the bytes at sometime later.

3.  Why add a watch here?

This is based on the Cadence UART implementation in QEMU (which does
the same thing). This will trigger a callback when we can write more
data or when the backend has hung up.

Many other serials do the same thing, like virtio-console and serial. So
it may be a common
interface here. I will try to understand it(Not yet).

Yep, it's just a more complete model of that the HW does.

I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
function.
The breakpoint did't match.

I just wonder if there is a case really need the callback function.

AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
shouldn't be triggered.
I tried with the UART in the virt board.  It also registers the watch 
callback on

G_IO_HUP and G_IO_OUT.

However I will through the question alone in another mail.

After the two points addressed,

Reviewed-by: LIU Zhiwei

Zhiwei


Alistair


Zhiwei

Alistair

Re: [PATCH v5 07/11] hw/char: Initial commit of Ibex UART

[Alistair Francis]

On Wed, Jun 3, 2020 at 6:59 PM LIU Zhiwei  wrote:
>
>
>
> On 2020/6/3 23:56, Alistair Francis wrote:
> > On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei  wrote:
> >> On 2020/6/3 1:54, Alistair Francis wrote:
> >>> On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei  wrote:
>  Hi Alistair,
> 
>  There are still some questions I don't understand.
> 
>  1. Is the baud rate  or fifo a necessary feature to simulate?
>  As you can see, qemu_chr_fe_write will send the byte as soon as possible.
>  When you want to transmit a byte through WDATA,  you can call
>  qemu_chr_fe_write directly.
> >>> So qemu_chr_fe_write() will send the data straight away. This doesn't
> >>> match what teh hardware does though. So by modelling a FIFO and a
> >>> delay in sending we can better match the hardware.
> >> I see many UARTs have similar features. Does the software really care about
> >> these features? Usually I just want to print something to the terminal
> >> through UART.
> > In this case Tock (which is the OS used for OpenTitan) does car about
> > these features as it relies on interrupts generated by the HW to
> > complete the serial send task. It also just makes the QEMU model more
> > accurate.
>
> Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO.
> At least,
> it can verify the ISP.

Exactly :)

> >> Most simulation in QEMU is for running software, not exactly the details
> >> of hardware.
> >> For example, we will not simulate the 16x oversamples in this UART.
> > Agreed. Lots of UARTs don't bother modelling the delay from the
> > hardware as generally it doesn't matter. In this case it does make a
> > difference for the software and it makes the QEMU model more accurate,
> > which is always a good thing.
> >
> >> There is no error here. Personally I  think it is necessary to simulate
> >> the FIFO and baud rate,
> >> maybe for supporting some backends.
> > So baud rate doesn't need to be modelled as we aren't actually sending
> > UART data, just pretending and then printing it.
> >
> >> Can someone give a reasonable answer for this question?
> > Which question?
> I see  the UART can work with many  different backends,  such as pty ,
> file, socket and so on.
> I wonder if this a backend, which has some requirements on the baud

The backend should be independent so it doesn't matter what baud rate
we choose here.

> rate.  You can ignore it,
> as it doesn't matter.
> >
>  2.  The baud rate calculation method is not strictly right.
>  I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
>  to send the byte instead of
>  char_tx_time * 4.
> >>> Do you mind explaining why 8 is correct instead of 4?
> >> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
> >> Translate a bit will take
> >> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.
> > I see your point. I just used the 4 as that is what the Cadence one
> > does. I don't think it matters too much as it's just the delay for a
> > timer (that isn't used as an accurate timer).
> Got it. Just a way to send the bytes at sometime later.
>  3.  Why add a watch here?
> >>> This is based on the Cadence UART implementation in QEMU (which does
> >>> the same thing). This will trigger a callback when we can write more
> >>> data or when the backend has hung up.
> >> Many other serials do the same thing, like virtio-console and serial. So
> >> it may be a common
> >> interface here. I will try to understand it(Not yet).
> > Yep, it's just a more complete model of that the HW does.
> I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback
> function.
> The breakpoint did't match.
>
> I just wonder if there is a case really need the callback function.

AFAIK Linux doesn't support the Ibex UART (or Ibex at all) so it
shouldn't be triggered.

Alistair

>
> Zhiwei
> >
> > Alistair
>

Re: [PATCH v4 00/10] vhost-user: Lift Max Ram Slots Limitation

[Raphael Norwitz]

ping

On Thu, May 21, 2020 at 1:00 AM Raphael Norwitz
 wrote:
>
> In QEMU today, a VM with a vhost-user device can hot add memory a
> maximum of 8 times. See these threads, among others:
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01046.html
> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01236.html
>
> [2] https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04656.html
>
> This series introduces a new protocol feature
> VHOST_USER_PROTOCOL_F_CONFIGURE_SLOTS which, when enabled, lifts the
> restriction on the maximum number RAM slots imposed by vhost-user.
>
> Without vhost-user, a Qemu VM can support 256 ram slots (for ACPI targets),
> or potentially more (the KVM max is 512). With each region, a file descriptor
> must be sent over the socket. If that many regions are sent in a single 
> message
> there could be upwards of 256 file descriptors being opened in the backend 
> process
> at once. Opening that many fds could easily push the process past the open fd 
> limit,
> especially considering one backend process could have multiple vhost threads,
> exposing different devices to different Qemu instances. Therefore to safely 
> lift the
> limit, transmitting regions should be split up over multiple messages.
>
> In addition, the VHOST_USER_SET_MEM_TABLE message was not reused because
> as the number of regions grows, the message becomes very large. In practice, 
> such
> large messages caused problems (truncated messages) and in the past it seems
> the community has opted for smaller fixed size messages where possible. 
> VRINGs,
> for example, are sent to the backend individually instead of in one massive
> message.
>
> The implementation details are explained in more detail in the commit
> messages, but at a high level the new protocol feature works as follows:
> - If the VHOST_USER_PROTCOL_F_CONFIGURE_MEM_SLOTS feature is enabled,
>   QEMU will send multiple VHOST_USER_ADD_MEM_REG and
>   VHOST_USER_REM_MEM_REG messages to map and unmap individual memory
>  regions instead of one large VHOST_USER_SET_MEM_TABLE message containing
>   all memory regions.
> - The vhost-user struct maintains a ’shadow state’ of memory regions
>   already sent to the guest. Each time vhost_user_set_mem_table is called,
>   the shadow state is compared with the new device state. A
>   VHOST_USER_REM_MEM_REG will be sent for each region in the shadow state
>   not in the device state. Then, a VHOST_USER_ADD_MEM_REG will be sent
>   for each region in the device state but not the shadow state. After
>   these messages have been sent, the shadow state will be updated to
>   reflect the new device state.
>
> The series consists of 10 changes:
> 1. Add helper to populate vhost-user message regions:
> This change adds a helper to populate a VhostUserMemoryRegion from a
> struct vhost_memory_region, which needs to be done in multiple places in
> in this series.
>
> 2. Add vhost-user helper to get MemoryRegion data
> This changes adds a helper to get a pointer to a MemoryRegion struct, 
> along
> with it's offset address and associated file descriptor. This helper is 
> used to
> simplify other vhost-user code paths and will be needed elsewhere in this
> series.
>
> 3. Add VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
> This change adds the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
> protocol feature. At this point, if negotiated, the feature only allows 
> the
> backend to limit the number of max ram slots to a number less than
> VHOST_MEMORY_MAX_NREGIONS = 8.
>
> 4. Transmit vhost-user memory regions individually
> With this change, if the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
> protocol feature is enabled, Qemu will send regions to the backend using
> individual VHOST_USER_ADD_MEM_REG and VHOST_USER_REM_MEM_REG
> messages.
> The max number of ram slots supported is still limited to 8.
>
> 5. Lift max memory slots imposed by vhost-user
> With this change, if the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
> protocol feature is enabled, the backend can support a configurable 
> number of
> ram slots up to the maximum allowed by the target platform.
>
> 6. Refactor out libvhost-user fault generation logic
> This cleanup moves some logic from vu_set_mem_table_exec_postcopy() to a
> separate helper, which will be needed elsewhere.
>
> 7. Support ram slot configuration in libvhost-user
>This change adds support for processing VHOST_USER_GET_MAX_MEMSLOTS
> messages in libvhost-user.
> The VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS protocol is not yet
> enabled in libvhost-user, so at this point this change is non-functional.
>
> 8. Support adding individual regions in libvhost-user
> This change adds libvhost-user support for mapping in new memory regions
> when receiving VHOST_USER_ADD_MEM_REG messages.
> The VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS protocol is not yet
> enabled in 

[PATCH] configure: Disable -Wtautological-type-limit-compare

[Richard Henderson]

Clang 10 enables this by default with -Wtype-limit.

All of the instances flagged by this Werror so far have been
cases in which we really do want the compiler to optimize away
the test completely.  Disabling the warning will avoid having
to add ifdefs to work around this.

Signed-off-by: Richard Henderson 
---
 configure | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configure b/configure
index f087d2bcd1..693f01327f 100755
--- a/configure
+++ b/configure
@@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body 
-Wnested-externs $gcc_flags"
 gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
 gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags"
 gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags"
+gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare"
+
 # Note that we do not add -Werror to gcc_flags here, because that would
 # enable it for all configure tests. If a configure test failed due
 # to -Werror this would just silently disable some features,
-- 
2.26.2

Re: [PATCH v4] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[Richard Henderson]

On 6/3/20 6:50 PM, agrecascino...@gmail.com wrote:
>  static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int 
> c)
>  {
> +tcg_debug_assert((c < 64) && (c >= 0));
>  tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
>  }
>  
> @@ -2610,21 +2614,33 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, 
> const TCGArg *args,
>  
>  case INDEX_op_shl_i32:
>  if (const_args[2]) {
> -tcg_out_shli32(s, args[0], args[1], args[2]);
> +/*
> + * Limit shift immediate to prevent illegal instruction
> + * from bitmask corruption
> + */
> +tcg_out_shli32(s, args[0], args[1], args[2] & 31);

Why are you duplicating these?

I suggested masking, and now you are, but you're also retaining the assert.
What's the point?  Just mask, IMO.

>  case INDEX_op_sar_i64:
>  if (const_args[2]) {
> -int sh = SH(args[2] & 0x1f) | (((args[2] >> 5) & 1) << 1);
> +/*
> + * Same for SRADI, except there's no function
> + * to call into.
> + */
> +int sh = SH(((args[2] & 63) & 0x1f)
> +| args[2] & 63) >> 5) & 1) << 1));

As for this, there's zero point.  We are already masking.  You can see it right
there.


r~

Re: [PATCH v3] migration/xbzrle: add encoding rate

[Richard Henderson]

On 6/3/20 7:58 PM, Wei Wang wrote:
> It is possible that encoded_size==0, but unencoded_size !=0. For example,
> a page is written with the same data that it already has.

That really contains 0 bytes?
Not even the ones that say "same data"?

You certainly have a magical compression algorithm there.
Or bad accounting.

> The encoding_rate is expected to reflect if the page is xbzrle encoding 
> friendly.
> The larger, the more friendly, so 0 might not be a good representation here.
> 
> Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue?

~0ull is no different than UINT64_MAX -- indeed, they are *exactly* the same
value -- and is not an exactly representible floating-point value.

If unencoded_size != 0, and (somehow) encoded_size == 0, then

  unencoded_size / encoded_size = Inf

which is indeed the limit of x -> 0, n / x.

Which is *also* printable by %0.2f.

I still contend that the middle if should be removed, and you should print out
whatever's left.  Either NaN or Inf is instructive.  Certainly nothing in the
middle cares about the actual value.


r~

Re: [PATCH v3] migration/xbzrle: add encoding rate

[Wei Wang]

On 06/04/2020 03:28 AM, Richard Henderson wrote:

On Wed, 29 Apr 2020 at 18:54, Wei Wang  wrote:

+if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
+xbzrle_counters.encoding_rate = 0;
+} else if (!encoded_size) {
+xbzrle_counters.encoding_rate = UINT64_MAX;
+} else {
+xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
+}

With clang 10, this produces

   CC  aarch64-softmmu/migration/ram.o
/home/rth/qemu/qemu/migration/ram.c:919:45: error: implicit conversion
from 'unsigned long' to 'double' changes value from
18446744073709551615 to 18446744073709551616
[-Werror,-Wimplicit-int-float-conversion]
 xbzrle_counters.encoding_rate = UINT64_MAX;
   ~ ^~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
# define UINT64_MAX (__UINT64_C(18446744073709551615))
  ^~~~
/usr/include/stdint.h:107:25: note: expanded from macro '__UINT64_C'
#  define __UINT64_C(c) c ## UL
 ^~~
:36:1: note: expanded from here
18446744073709551615UL
^~
1 error generated.

UINT64_MAX apprears both arbitrary and wrong.

The only way I can imagine encoded_size == 0 is unencoded_size == 0,
so 0 seems like the correct answer.  Moreover, it really seems like
the first test sufficiently covers that possibility.


It is possible that encoded_size==0, but unencoded_size !=0. For example,
a page is written with the same data that it already has.




In addition, the only user of this value is


+monitor_printf(mon, "xbzrle encoding rate: %0.2f\n",
+   info->xbzrle_cache->encoding_rate);

which would be quite happy to print NaN even if the 0/0 computation
were to run.  Though as I say above, I don't think that's reachable.


The encoding_rate is expected to reflect if the page is xbzrle encoding 
friendly.

The larger, the more friendly, so 0 might not be a good representation here.

Maybe, we could change UINT64_MAX above to "~0ULL" to avoid the issue?

Best,
Wei

Re: [PATCH v5 07/11] hw/char: Initial commit of Ibex UART

[LIU Zhiwei]

On 2020/6/3 23:56, Alistair Francis wrote:

On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei  wrote:

On 2020/6/3 1:54, Alistair Francis wrote:

On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei  wrote:

Hi Alistair,

There are still some questions I don't understand.

1. Is the baud rate  or fifo a necessary feature to simulate?
As you can see, qemu_chr_fe_write will send the byte as soon as possible.
When you want to transmit a byte through WDATA,  you can call
qemu_chr_fe_write directly.

So qemu_chr_fe_write() will send the data straight away. This doesn't
match what teh hardware does though. So by modelling a FIFO and a
delay in sending we can better match the hardware.

I see many UARTs have similar features. Does the software really care about
these features? Usually I just want to print something to the terminal
through UART.

In this case Tock (which is the OS used for OpenTitan) does car about
these features as it relies on interrupts generated by the HW to
complete the serial send task. It also just makes the QEMU model more
accurate.


Fair enough. I see the "tx_watermark" interrupt, which needs the FIFO. 
At least,

it can verify the ISP.

Most simulation in QEMU is for running software, not exactly the details
of hardware.
For example, we will not simulate the 16x oversamples in this UART.

Agreed. Lots of UARTs don't bother modelling the delay from the
hardware as generally it doesn't matter. In this case it does make a
difference for the software and it makes the QEMU model more accurate,
which is always a good thing.


There is no error here. Personally I  think it is necessary to simulate
the FIFO and baud rate,
maybe for supporting some backends.

So baud rate doesn't need to be modelled as we aren't actually sending
UART data, just pretending and then printing it.


Can someone give a reasonable answer for this question?

Which question?
I see  the UART can work with many  different backends,  such as pty , 
file, socket and so on.
I wonder if this a backend, which has some requirements on the baud 
rate.  You can ignore it,

as it doesn't matter.



2.  The baud rate calculation method is not strictly right.
I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
to send the byte instead of
char_tx_time * 4.

Do you mind explaining why 8 is correct instead of 4?

Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
Translate a bit will take
char_tx_time. So it will take char_tx_time * 8 to transmit a byte.

I see your point. I just used the 4 as that is what the Cadence one
does. I don't think it matters too much as it's just the delay for a
timer (that isn't used as an accurate timer).

Got it. Just a way to send the bytes at sometime later.

3.  Why add a watch here?

This is based on the Cadence UART implementation in QEMU (which does
the same thing). This will trigger a callback when we can write more
data or when the backend has hung up.

Many other serials do the same thing, like virtio-console and serial. So
it may be a common
interface here. I will try to understand it(Not yet).

Yep, it's just a more complete model of that the HW does.
I try to boot a RISC-V Linux, and set a breakpoint  to a watch callback 
function.

The breakpoint did't match.

I just wonder if there is a case really need the callback function.

Zhiwei


Alistair

Re: [PATCH 00/13] i386: hvf: Remove HVFX86EmulatorState

[Cameron Esfahani]

Reviewed-by: Cameron Esfahani  

Cameron Esfahani
di...@apple.com

"The cake is a lie."

Common wisdom



> On May 28, 2020, at 12:37 PM, Roman Bolshakov  wrote:
> 
> Hi,
> 
> This is a cleanup series for HVF accel.
> 
> HVF is using two emulator states CPUX86State and HVFX86EmulatorState
> simultaneously. HVFX86EmulatorState is used for instruction emulation.
> CPUX86State is used in all other places. Sometimes the states are in
> sync, sometimes they're not. It complicates reasoning about emulator
> behaviour given that there's a third state - VMCS.
> 
> The series tries to leverage CPUX86State for instruction decoding and
> removes HVFX86EmulatorState. I had to add two new hvf-specific fields to
> CPUX86State: lazy_flags and mmio_buf. It's likely that cc_op, cc_dst,
> etc could be reused for lazy_flags but it'd require major rework of flag
> processing during instruction emulation. Hopefully that'll happen too in
> the future.
> 
> I tried to include sysemu/hvf.h into target/i386/cpu.h to add definition
> of hvf lazy flags but couldn't do that at first it because it introduced
> circular dependency between existing sysemu/hvf.h and cpu.h. The first
> three patches untangle and prune sysemu/hvf.h to the bare minimum to
> allow inclusion of sysemu/hvf.h into target/i386/cpu.h.
> 
> This might conflict with [1], but merge/rebase should be trivial.
> 
> 1. https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg07449.html
> 
> Thanks,
> Roman
> 
> Roman Bolshakov (13):
>  i386: hvf: Move HVFState definition into hvf
>  i386: hvf: Drop useless declarations in sysemu
>  i386: hvf: Clean stray includes in sysemu
>  i386: hvf: Drop unused variable
>  i386: hvf: Use ins_len to advance IP
>  i386: hvf: Use IP from CPUX86State
>  i386: hvf: Drop fetch_rip from HVFX86EmulatorState
>  i386: hvf: Drop rflags from HVFX86EmulatorState
>  i386: hvf: Drop copy of RFLAGS defines
>  i386: hvf: Drop regs in HVFX86EmulatorState
>  i386: hvf: Move lazy_flags into CPUX86State
>  i386: hvf: Move mmio_buf into CPUX86State
>  i386: hvf: Drop HVFX86EmulatorState
> 
> include/qemu/typedefs.h  |   1 -
> include/sysemu/hvf.h |  73 ++---
> target/i386/cpu.h|   4 +-
> target/i386/hvf/hvf-i386.h   |  35 ++
> target/i386/hvf/hvf.c|  30 -
> target/i386/hvf/x86.c|   2 +-
> target/i386/hvf/x86.h|  89 ++---
> target/i386/hvf/x86_decode.c |  25 ---
> target/i386/hvf/x86_emu.c| 122 +--
> target/i386/hvf/x86_flags.c  |  81 ---
> target/i386/hvf/x86_task.c   |  10 +--
> target/i386/hvf/x86hvf.c |   6 +-
> 12 files changed, 186 insertions(+), 292 deletions(-)
> 
> -- 
> 2.26.1
> 
> 

[PATCH v4] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[agrecascino123]

From: "Catherine A. Frederick" 

Signed-off-by: Catherine A. Frederick 
---
This should finally do it, sorry for the style issues on v3.

 tcg/ppc/tcg-target.inc.c | 41 ++--
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 7da67086c6..5ed4e4c011 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg 
dst, TCGReg src)
 
 static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 32) && (c >= 0));
 tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
 }
 
 static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 64) && (c >= 0));
 tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
 }
 
 static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 32) && (c >= 0));
 tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
 }
 
 static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 64) && (c >= 0));
 tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
 }
 
@@ -2610,21 +2614,33 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, 
const TCGArg *args,
 
 case INDEX_op_shl_i32:
 if (const_args[2]) {
-tcg_out_shli32(s, args[0], args[1], args[2]);
+/*
+ * Limit shift immediate to prevent illegal instruction
+ * from bitmask corruption
+ */
+tcg_out_shli32(s, args[0], args[1], args[2] & 31);
 } else {
 tcg_out32(s, SLW | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_shr_i32:
 if (const_args[2]) {
-tcg_out_shri32(s, args[0], args[1], args[2]);
+/*
+ * Both use RLWINM, which has a 5 bit field for the
+ * shift mask.
+ */
+tcg_out_shri32(s, args[0], args[1], args[2] & 31);
 } else {
 tcg_out32(s, SRW | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_sar_i32:
 if (const_args[2]) {
-tcg_out32(s, SRAWI | RS(args[1]) | RA(args[0]) | SH(args[2]));
+/*
+ * SRAWI has a 5 bit sized field for the shift mask
+ * as well.
+ */
+tcg_out32(s, SRAWI | RS(args[1]) | RA(args[0]) | SH(args[2] & 31));
 } else {
 tcg_out32(s, SRAW | SAB(args[1], args[0], args[2]));
 }
@@ -2696,21 +2712,34 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, 
const TCGArg *args,
 
 case INDEX_op_shl_i64:
 if (const_args[2]) {
-tcg_out_shli64(s, args[0], args[1], args[2]);
+/*
+ * Limit shift immediate to prevent illegal instruction from
+ * from bitmask corruption
+ */
+tcg_out_shli64(s, args[0], args[1], args[2] & 63);
 } else {
 tcg_out32(s, SLD | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_shr_i64:
 if (const_args[2]) {
-tcg_out_shri64(s, args[0], args[1], args[2]);
+/*
+ * Same applies here, as both RLDICL, and RLDICR have a
+ * 6 bit large mask for the shift value
+ */
+tcg_out_shri64(s, args[0], args[1], args[2] & 63);
 } else {
 tcg_out32(s, SRD | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_sar_i64:
 if (const_args[2]) {
-int sh = SH(args[2] & 0x1f) | (((args[2] >> 5) & 1) << 1);
+/*
+ * Same for SRADI, except there's no function
+ * to call into.
+ */
+int sh = SH(((args[2] & 63) & 0x1f)
+| args[2] & 63) >> 5) & 1) << 1));
 tcg_out32(s, SRADI | RA(args[0]) | RS(args[1]) | sh);
 } else {
 tcg_out32(s, SRAD | SAB(args[1], args[0], args[2]));
-- 
2.26.2

[PATCH] [PATCH v6] linux-user: syscall: ioctls: support DRM_IOCTL_VERSION

[chengang]

From: Chen Gang 

Another DRM_IOCTL_* commands will be done later.

Signed-off-by: Chen Gang 
---
 configure  | 10 
 linux-user/ioctls.h|  5 ++
 linux-user/syscall.c   | 95 ++
 linux-user/syscall_defs.h  | 15 ++
 linux-user/syscall_types.h | 11 +
 5 files changed, 136 insertions(+)

diff --git a/configure b/configure
index f087d2bcd1..389dbb1d09 100755
--- a/configure
+++ b/configure
@@ -3136,6 +3136,13 @@ if ! check_include "ifaddrs.h" ; then
   have_ifaddrs_h=no
 fi
 
+#
+# libdrm check
+have_drm_h=no
+if check_include "libdrm/drm.h" ; then
+have_drm_h=yes
+fi
+
 ##
 # VTE probe
 
@@ -7127,6 +7134,9 @@ fi
 if test "$have_ifaddrs_h" = "yes" ; then
 echo "HAVE_IFADDRS_H=y" >> $config_host_mak
 fi
+if test "$have_drm_h" = "yes" ; then
+  echo "HAVE_DRM_H=y" >> $config_host_mak
+fi
 if test "$have_broken_size_max" = "yes" ; then
 echo "HAVE_BROKEN_SIZE_MAX=y" >> $config_host_mak
 fi
diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index 0defa1d8c1..f2e2fa9c87 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -574,6 +574,11 @@
   IOCTL_SPECIAL(SIOCDELRT, IOC_W, do_ioctl_rt,
 MK_PTR(MK_STRUCT(STRUCT_rtentry)))
 
+#ifdef HAVE_DRM_H
+  IOCTL_SPECIAL(DRM_IOCTL_VERSION, IOC_RW, do_ioctl_drm,
+MK_PTR(MK_STRUCT(STRUCT_drm_version)))
+#endif
+
 #ifdef TARGET_TIOCSTART
   IOCTL_IGNORE(TIOCSTART)
   IOCTL_IGNORE(TIOCSTOP)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 7f6700c54e..1744e7acc7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -112,6 +112,9 @@
 #include 
 #include 
 #include 
+#ifdef HAVE_DRM_H
+#include 
+#endif
 #include "linux_loop.h"
 #include "uname.h"
 
@@ -5276,6 +5279,98 @@ static abi_long do_ioctl_tiocgptpeer(const IOCTLEntry 
*ie, uint8_t *buf_temp,
 }
 #endif
 
+#ifdef HAVE_DRM_H
+
+static void unlock_drm_version(struct drm_version *host_ver)
+{
+unlock_user(host_ver->name, 0UL, 0);
+unlock_user(host_ver->date, 0UL, 0);
+unlock_user(host_ver->desc, 0UL, 0);
+}
+
+static inline abi_long target_to_host_drmversion(struct drm_version *host_ver,
+  struct target_drm_version 
*target_ver)
+{
+memset(host_ver, 0, sizeof(*host_ver));
+
+__get_user(host_ver->name_len, _ver->name_len);
+if (host_ver->name_len) {
+host_ver->name = lock_user(VERIFY_WRITE, target_ver->name,
+   target_ver->name_len, 0);
+if (!host_ver->name) {
+goto err;
+}
+}
+
+__get_user(host_ver->date_len, _ver->date_len);
+if (host_ver->date_len) {
+host_ver->date = lock_user(VERIFY_WRITE, target_ver->date,
+   target_ver->date_len, 0);
+if (!host_ver->date) {
+goto err;
+}
+}
+
+__get_user(host_ver->desc_len, _ver->desc_len);
+if (host_ver->desc_len) {
+host_ver->desc = lock_user(VERIFY_WRITE, target_ver->desc,
+   target_ver->desc_len, 0);
+if (!host_ver->desc) {
+goto err;
+}
+}
+
+return 0;
+err:
+unlock_drm_version(host_ver);
+return -EFAULT;
+}
+
+static inline abi_long host_to_target_drmversion(
+  struct target_drm_version 
*target_ver,
+  struct drm_version *host_ver)
+{
+__put_user(host_ver->version_major, _ver->version_major);
+__put_user(host_ver->version_minor, _ver->version_minor);
+__put_user(host_ver->version_patchlevel, _ver->version_patchlevel);
+__put_user(host_ver->name_len, _ver->name_len);
+__put_user(host_ver->date_len, _ver->date_len);
+__put_user(host_ver->desc_len, _ver->desc_len);
+unlock_user(host_ver->name, target_ver->name, host_ver->name_len);
+unlock_user(host_ver->date, target_ver->date, host_ver->date_len);
+unlock_user(host_ver->desc, target_ver->desc, host_ver->desc_len);
+return 0;
+}
+
+static abi_long do_ioctl_drm(const IOCTLEntry *ie, uint8_t *buf_temp,
+ int fd, int cmd, abi_long arg)
+{
+struct drm_version *ver;
+struct target_drm_version *target_ver;
+abi_long ret;
+
+switch (ie->host_cmd) {
+case DRM_IOCTL_VERSION:
+if (!lock_user_struct(VERIFY_WRITE, target_ver, arg, 0)) {
+return -TARGET_EFAULT;
+}
+ver = (struct drm_version *)buf_temp;
+ret = target_to_host_drmversion(ver, target_ver);
+if (!is_error(ret)) {
+ret = get_errno(safe_ioctl(fd, ie->host_cmd, ver));
+if (!is_error(ret)) {
+ret = host_to_target_drmversion(target_ver, ver);
+}
+unlock_drm_version(ver);
+}
+unlock_user_struct(target_ver, arg, 0);
+return ret;
+}
+return 

Re: [PATCH] configure: Don't warn about lack of PIE on macOS

[Cameron Esfahani]

Reviewed-by: Cameron Esfahani  

Cameron Esfahani
di...@apple.com

"It is the spirit and not the form of law that keeps justice alive."

Earl Warren



> On Jun 1, 2020, at 5:42 AM, Roman Bolshakov  wrote:
> 
> ld64 is making PIE executables for 10.7 and above by default, as
> documented in ld(1).
> 
> Signed-off-by: Roman Bolshakov 
> ---
> configure | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/configure b/configure
> index af2ba83f0e..6dddbca4b2 100755
> --- a/configure
> +++ b/configure
> @@ -2137,6 +2137,8 @@ elif compile_prog "-Werror -fPIE -DPIE" "-pie"; then
>   QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
>   QEMU_LDFLAGS="-pie $QEMU_LDFLAGS"
>   pie="yes"
> +elif test "$darwin" = "yes"; then
> +  pie="yes"
> elif test "$pie" = "yes"; then
>   error_exit "PIE not available due to missing toolchain support"
> else
> -- 
> 2.26.1
> 
> 

Re: [PATCH v5] linux-user: syscall: ioctls: support DRM_IOCTL_VERSION

[Chen Gang]

OK, thanks. I'll send patch v6. :)

On 2020/6/3 下午8:03, Laurent Vivier wrote:
> Le 03/06/2020 à 13:05, Chen Gang a écrit :
>> On 2020/6/3 下午5:49, Laurent Vivier wrote:
>>> Le 03/06/2020 à 03:08, cheng...@emindsoft.com.cn a écrit :
 +#ifdef HAVE_DRM_H
 +
 +static void unlock_drm_version(struct drm_version *host_ver)
 +{
 +if (host_ver->name) {
 +unlock_user(host_ver->name, 0UL, 0);
>>>
>>> unlock_user() allows to have a NULL host pointer parameter, so you don't
>>> need to check. But you must provide the target pointer, with the length.
>>> The same below.
>>>
>>
>> As far as I know, the unlock_user is defined in
>> include/exec/softmmu-semi.h, which only checks the len before calling
>> cpu_memory_rw_debug, and only calls free() for the host pointer.
>>
>> So we have to be sure that the host pointer must be valid. When we pass
>> 0 length to unlock_user, we want it to free host pointer only.
> 
> No, it is defined in our case in linux-user/qemu.h, and associated
> comment is:
> 
> /* Unlock an area of guest memory.  The first LEN bytes must be
>flushed back to guest memory. host_ptr = NULL is explicitly
>allowed and does nothing. */
> 
>>
 +if (host_ver->desc_len) {
 +host_ver->desc = lock_user(VERIFY_WRITE, target_ver->desc,
 +   target_ver->desc_len, 0);
 +if (!host_ver->desc) {
 +goto err;
 +}
 +}
 +
 +unlock_user_struct(target_ver, target_addr, 0);
 +return 0;
 +err:
 +unlock_drm_version(host_ver);
 +unlock_user_struct(target_ver, target_addr, 0);
 +return -ENOMEM;
>>>
>>> In fact it should be -TARGET_EFAULT: it has failed because of access rights.
>>>
>>
>> As far as I know, the lock_user is defined in
>> include/exec/softmmu-semi.h. If the parameter 'copy' is 0 (in our case),
>> lock_user will only malloc a host pointer and return it.
> 
> No, in linux-user/qemu.h:
> 
> /* Lock an area of guest memory into the host.  If copy is true then the
>host area will have the same contents as the guest.  */
> 
>> In our case, I guess the only failure from malloc() is "no memory".
> 
> See use-cases in syscall.c, they all fail with -TARGET_EFAULT.

Re: [PATCH for-5.1 V4 1/4] hw/mips: Implement the kvm_type() hook in MachineClass

[Huacai Chen]

Hi, Alexandar,

On Wed, Jun 3, 2020 at 10:34 PM Aleksandar Markovic
 wrote:
>
>
>
> уто, 2. јун 2020. у 04:38 Huacai Chen  је написао/ла:
>>
>> MIPS has two types of KVM: TE & VZ, and TE is the default type. Now we
>> can't create a VZ guest in QEMU because it lacks the kvm_type() hook in
>> MachineClass. Besides, libvirt uses a null-machine to detect the kvm
>> capability, so by default it will return "KVM not supported" on a VZ
>> platform. Thus, null-machine also need the kvm_type() hook.
>>
>> Reviewed-by: Aleksandar Markovic 
>> Signed-off-by: Huacai Chen 
>> Co-developed-by: Jiaxun Yang 
>> ---
>
>
>
> Hi, Huacai,
>
> For MIPS parts of QEMU, we prefer the following licence preamble:
>
>  *  This program is free software: you can redistribute it and/or modify
>  *  it under the terms of the GNU General Public License as published by
>  *  the Free Software Foundation, either version 2 of the License, or
>  *  (at your option) any later version.
>  *
>  *  This program is distributed in the hope that it will be useful,
>  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>  *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  *  GNU General Public License for more details.
>  *
>  *  You should have received a copy of the GNU General Public License
>  *  along with this program.  If not, see .
>
> Do you agree with such preamble in hw/mips/common.c?
Yes, I agree.

>
> (of course you name and email will stay intact)
>
> Regards,
> Aleksandar
>
>>  hw/core/Makefile.objs  |  2 +-
>>  hw/core/null-machine.c |  4 
>>  hw/mips/Makefile.objs  |  2 +-
>>  hw/mips/common.c   | 42 ++
>>  include/hw/mips/mips.h |  3 +++
>>  5 files changed, 51 insertions(+), 2 deletions(-)
>>  create mode 100644 hw/mips/common.c
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index 1d540ed..b5672f4 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -17,11 +17,11 @@ common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>>  common-obj-$(CONFIG_SOFTMMU) += sysbus.o
>>  common-obj-$(CONFIG_SOFTMMU) += machine.o
>> -common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>>  common-obj-$(CONFIG_SOFTMMU) += machine-hmp-cmds.o
>>  common-obj-$(CONFIG_SOFTMMU) += numa.o
>>  common-obj-$(CONFIG_SOFTMMU) += clock-vmstate.o
>> +obj-$(CONFIG_SOFTMMU) += null-machine.o
>>  obj-$(CONFIG_SOFTMMU) += machine-qmp-cmds.o
>>
>>  common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>> index cb47d9d..94a36f9 100644
>> --- a/hw/core/null-machine.c
>> +++ b/hw/core/null-machine.c
>> @@ -17,6 +17,7 @@
>>  #include "sysemu/sysemu.h"
>>  #include "exec/address-spaces.h"
>>  #include "hw/core/cpu.h"
>> +#include "hw/mips/mips.h"
>>
>>  static void machine_none_init(MachineState *mch)
>>  {
>> @@ -50,6 +51,9 @@ static void machine_none_machine_init(MachineClass *mc)
>>  mc->max_cpus = 1;
>>  mc->default_ram_size = 0;
>>  mc->default_ram_id = "ram";
>> +#ifdef TARGET_MIPS
>> +mc->kvm_type = mips_kvm_type;
>> +#endif
>>  }
>>
>>  DEFINE_MACHINE("none", machine_none_machine_init)
>> diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs
>> index 739e2b7..3b3e6ea 100644
>> --- a/hw/mips/Makefile.objs
>> +++ b/hw/mips/Makefile.objs
>> @@ -1,4 +1,4 @@
>> -obj-y += addr.o mips_int.o
>> +obj-y += addr.o common.o mips_int.o
>>  obj-$(CONFIG_R4K) += r4k.o
>>  obj-$(CONFIG_MALTA) += gt64xxx_pci.o malta.o
>>  obj-$(CONFIG_MIPSSIM) += mipssim.o
>> diff --git a/hw/mips/common.c b/hw/mips/common.c
>> new file mode 100644
>> index 000..4d8e141
>> --- /dev/null
>> +++ b/hw/mips/common.c
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Common MIPS routines
>> + *
>> + * Copyright (c) 2020 Huacai Chen (che...@lemote.com)
>> + * This code is licensed under the GNU GPL v2.
>> + */
>> +
>> +#include 
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "hw/boards.h"
>> +#include "hw/mips/mips.h"
>> +#include "sysemu/kvm_int.h"
>> +
>> +#ifndef CONFIG_KVM
>> +
>> +int mips_kvm_type(MachineState *machine, const char *vm_type)
>> +{
>> +return 0;
>> +}
>> +
>> +#else
>> +
>> +int mips_kvm_type(MachineState *machine, const char *vm_type)
>> +{
>> +int r;
>> +KVMState *s = KVM_STATE(machine->accelerator);
>> +
>> +r = kvm_check_extension(s, KVM_CAP_MIPS_VZ);
>> +if (r > 0) {
>> +return KVM_VM_MIPS_VZ;
>> +}
>> +
>> +r = kvm_check_extension(s, KVM_CAP_MIPS_TE);
>> +if (r > 0) {
>> +return KVM_VM_MIPS_TE;
>> +}
>> +
>> +return -1;
>> +}
>> +
>> +#endif
>> diff --git a/include/hw/mips/mips.h b/include/hw/mips/mips.h
>> index 0af4c3d..2ac0580 100644
>> --- a/include/hw/mips/mips.h
>> +++ b/include/hw/mips/mips.h
>> @@ -20,4 +20,7 @@ void rc4030_dma_write(void *dma, uint8_t *buf, int len);
>>
>>  

RE: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD

[Babu Moger]

Started looking at this. Let me know if you have any ideas. Will respond
with more details later this week.

> -Original Message-
> From: Eduardo Habkost 
> Sent: Tuesday, June 2, 2020 12:52 PM
> To: Moger, Babu 
> Cc: m...@redhat.com; marcel.apfelb...@gmail.com; pbonz...@redhat.com;
> r...@twiddle.net; mtosa...@redhat.com; qemu-devel@nongnu.org;
> k...@vger.kernel.org; k...@tripleback.net; ge...@hostfission.com; Dr. David
> Alan Gilbert 
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> 
> On Fri, Jun 08, 2018 at 06:56:17PM -0400, Babu Moger wrote:
> > Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> > match the underlying hardware. Please refer to the Processor Programming
> > Reference (PPR) for AMD Family 17h Model for more details.
> >
> > Signed-off-by: Babu Moger 
> [...]
> > +case 0x801E:
> > +assert(cpu->core_id <= 255);
> 
> It is possible to trigger this assert using:
> 
> $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> intel-iommu,intremap=on,eim=on -smp
> 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> qemu64,xlevel=0x801e -device qemu64-x86_64-cpu,apic-id=257
> qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258)
> exceeds the recommended cpus supported by KVM (240)
> qemu-system-x86_64:
> /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> Assertion `cpu->core_id <= 255' failed.
> Aborted (core dumped)
> 
> See bug report and discussion at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> redhat.com%2Fshow_bug.cgi%3Fid%3D1834200data=02%7C01%7Cbabu.
> moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408sdata=ib
> iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3Dreserved=0
> 
> Also, it looks like encode_topo_cpuid801e() assumes core_id
> has only 3 bits, so the existing assert() is not even sufficient.
> We need to decide what to do if the user requests nr_cores > 8.
> 
> Probably omitting CPUID[0x801E] if the VCPU topology is
> incompatible with encode_topo_cpuid801e() (and printing a
> warning) is the safest thing to do right now.
> 
> 
> 
> > +encode_topo_cpuid801e(cs, cpu,
> > +  eax, ebx, ecx, edx);
> > +break;
> >  case 0xC000:
> >  *eax = env->cpuid_xlevel2;
> >  *ebx = 0;
> > --
> > 1.8.3.1
> >
> 
> --
> Eduardo

Re: [PATCH v5 03/15] acpi: rtc: use a single crs range

[Cameron Esfahani]

Reviewed-by: Cameron Esfahani 

Cameron Esfahani
di...@apple.com

"Americans are very skilled at creating a custom meaning from something that's 
mass-produced."

Ann Powers


> On May 7, 2020, at 6:16 AM, Gerd Hoffmann  wrote:
> 
> Use a single io range for _CRS instead of two,
> following what real hardware does.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
> hw/rtc/mc146818rtc.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 2104e0aa3b14..ab0cc59973b3 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -1013,12 +1013,14 @@ static void rtc_build_aml(ISADevice *isadev, Aml 
> *scope)
> Aml *dev;
> Aml *crs;
> 
> +/*
> + * Reserving 8 io ports here, following what physical hardware
> + * does, even though qemu only responds to the first two ports.
> + */
> crs = aml_resource_template();
> aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
> -   0x10, 0x02));
> +   0x01, 0x08));
> aml_append(crs, aml_irq_no_flags(RTC_ISA_IRQ));
> -aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE + 2, RTC_ISA_BASE + 2,
> -   0x02, 0x06));
> 
> dev = aml_device("RTC");
> aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
> -- 
> 2.18.4
> 
> 

Re: [PATCH 04/11] sysemu/hvf: Only declare hvf_allowed when HVF is available

[Cameron Esfahani]

Commit message typo tcg_allowed -> hvf_allowed.

If fixed:
Reviewed-by: Cameron Esfahani 

Cameron Esfahani
di...@apple.com

"You only live once, and the way I live, once is enough"

Frank Sinatra



> On May 9, 2020, at 6:09 AM, Philippe Mathieu-Daudé  wrote:
> 
> When HVF is not available, the tcg_allowed variable does not exist.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> include/sysemu/hvf.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
> index d211e808e9..fe95743124 100644
> --- a/include/sysemu/hvf.h
> +++ b/include/sysemu/hvf.h
> @@ -18,7 +18,6 @@
> #include "exec/memory.h"
> #include "sysemu/accel.h"
> 
> -extern bool hvf_allowed;
> #ifdef CONFIG_HVF
> #include 
> #include 
> @@ -26,11 +25,12 @@ extern bool hvf_allowed;
> #include "target/i386/cpu.h"
> uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
>  int reg);
> +extern bool hvf_allowed;
> #define hvf_enabled() (hvf_allowed)
> -#else
> +#else /* !CONFIG_HVF */
> #define hvf_enabled() 0
> #define hvf_get_supported_cpuid(func, idx, reg) 0
> -#endif
> +#endif /* !CONFIG_HVF */
> 
> /* hvf_slot flags */
> #define HVF_SLOT_LOG (1 << 0)
> -- 
> 2.21.3
> 
> 

Re: [PATCH v3] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[Catherine A. Frederick / mptcultist]

Oh dear, I did it to myself again.

On Wed, Jun 3, 2020 at 7:13 PM  wrote:
>
> From: "Catherine A. Frederick" 
>
> Signed-off-by: Catherine A. Frederick 
> ---
>  tcg/ppc/tcg-target.inc.c | 28 ++--
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
> index 7da67086c6..3cab56fe91 100644
> --- a/tcg/ppc/tcg-target.inc.c
> +++ b/tcg/ppc/tcg-target.inc.c
> @@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg 
> dst, TCGReg src)
>
>  static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int 
> c)
>  {
> +tcg_debug_assert((c < 32) && (c >= 0));
>  tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
>  }
>
>  static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int 
> c)
>  {
> +tcg_debug_assert((c < 64) && (c >= 0));
>  tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
>  }
>
>  static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int 
> c)
>  {
> +tcg_debug_assert((c < 32) && (c >= 0));
>  tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
>  }
>
>  static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int 
> c)
>  {
> +tcg_debug_assert((c < 64) && (c >= 0));
>  tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
>  }
>
> @@ -2610,21 +2614,27 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, 
> const TCGArg *args,
>
>  case INDEX_op_shl_i32:
>  if (const_args[2]) {
> -tcg_out_shli32(s, args[0], args[1], args[2]);
> +/* Limit shift immediate to prevent illegal instruction
> +   from bitmask corruption */
> +tcg_out_shli32(s, args[0], args[1], args[2] & 31);
>  } else {
>  tcg_out32(s, SLW | SAB(args[1], args[0], args[2]));
>  }
>  break;
>  case INDEX_op_shr_i32:
>  if (const_args[2]) {
> -tcg_out_shri32(s, args[0], args[1], args[2]);
> +/* Both use RLWINM, which has a 5 bit field for the
> +   shift mask. */
> +tcg_out_shri32(s, args[0], args[1], args[2] & 31);
>  } else {
>  tcg_out32(s, SRW | SAB(args[1], args[0], args[2]));
>  }
>  break;
>  case INDEX_op_sar_i32:
>  if (const_args[2]) {
> -tcg_out32(s, SRAWI | RS(args[1]) | RA(args[0]) | SH(args[2]));
> +/* SRAWI has a 5 bit sized field for the shift mask
> +   as well. */
> +tcg_out32(s, SRAWI | RS(args[1]) | RA(args[0]) | SH(args[2] & 
> 31));
>  } else {
>  tcg_out32(s, SRAW | SAB(args[1], args[0], args[2]));
>  }
> @@ -2696,21 +2706,27 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, 
> const TCGArg *args,
>
>  case INDEX_op_shl_i64:
>  if (const_args[2]) {
> -tcg_out_shli64(s, args[0], args[1], args[2]);
> +/* Limit shift immediate to prevent illegal instruction from
> +   from bitmask corruption */
> +tcg_out_shli64(s, args[0], args[1], args[2] & 63);
>  } else {
>  tcg_out32(s, SLD | SAB(args[1], args[0], args[2]));
>  }
>  break;
>  case INDEX_op_shr_i64:
>  if (const_args[2]) {
> -tcg_out_shri64(s, args[0], args[1], args[2]);
> +/* Same applies here, as both RLDICL, and RLDICR have a
> +   6 bit large mask for the shift value */
> +tcg_out_shri64(s, args[0], args[1], args[2] & 63);
>  } else {
>  tcg_out32(s, SRD | SAB(args[1], args[0], args[2]));
>  }
>  break;
>  case INDEX_op_sar_i64:
>  if (const_args[2]) {
> -int sh = SH(args[2] & 0x1f) | (((args[2] >> 5) & 1) << 1);
> +/* Same for SRADI, except there's no function
> +   to call into. */
> +int sh = SH(((args[2] & 63) & 0x1f) | args[2] & 63) >> 5) & 
> 1) << 1));
>  tcg_out32(s, SRADI | RA(args[0]) | RS(args[1]) | sh);
>  } else {
>  tcg_out32(s, SRAD | SAB(args[1], args[0], args[2]));
> --
> 2.26.2
>

[PATCH v3] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[agrecascino123]

From: "Catherine A. Frederick" 

Signed-off-by: Catherine A. Frederick 
---
 tcg/ppc/tcg-target.inc.c | 28 ++--
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 7da67086c6..3cab56fe91 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg 
dst, TCGReg src)
 
 static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 32) && (c >= 0));
 tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
 }
 
 static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 64) && (c >= 0));
 tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
 }
 
 static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 32) && (c >= 0));
 tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
 }
 
 static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+tcg_debug_assert((c < 64) && (c >= 0));
 tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
 }
 
@@ -2610,21 +2614,27 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, 
const TCGArg *args,
 
 case INDEX_op_shl_i32:
 if (const_args[2]) {
-tcg_out_shli32(s, args[0], args[1], args[2]);
+/* Limit shift immediate to prevent illegal instruction
+   from bitmask corruption */
+tcg_out_shli32(s, args[0], args[1], args[2] & 31);
 } else {
 tcg_out32(s, SLW | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_shr_i32:
 if (const_args[2]) {
-tcg_out_shri32(s, args[0], args[1], args[2]);
+/* Both use RLWINM, which has a 5 bit field for the
+   shift mask. */
+tcg_out_shri32(s, args[0], args[1], args[2] & 31);
 } else {
 tcg_out32(s, SRW | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_sar_i32:
 if (const_args[2]) {
-tcg_out32(s, SRAWI | RS(args[1]) | RA(args[0]) | SH(args[2]));
+/* SRAWI has a 5 bit sized field for the shift mask
+   as well. */
+tcg_out32(s, SRAWI | RS(args[1]) | RA(args[0]) | SH(args[2] & 31));
 } else {
 tcg_out32(s, SRAW | SAB(args[1], args[0], args[2]));
 }
@@ -2696,21 +2706,27 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, 
const TCGArg *args,
 
 case INDEX_op_shl_i64:
 if (const_args[2]) {
-tcg_out_shli64(s, args[0], args[1], args[2]);
+/* Limit shift immediate to prevent illegal instruction from
+   from bitmask corruption */
+tcg_out_shli64(s, args[0], args[1], args[2] & 63);
 } else {
 tcg_out32(s, SLD | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_shr_i64:
 if (const_args[2]) {
-tcg_out_shri64(s, args[0], args[1], args[2]);
+/* Same applies here, as both RLDICL, and RLDICR have a
+   6 bit large mask for the shift value */
+tcg_out_shri64(s, args[0], args[1], args[2] & 63);
 } else {
 tcg_out32(s, SRD | SAB(args[1], args[0], args[2]));
 }
 break;
 case INDEX_op_sar_i64:
 if (const_args[2]) {
-int sh = SH(args[2] & 0x1f) | (((args[2] >> 5) & 1) << 1);
+/* Same for SRADI, except there's no function
+   to call into. */
+int sh = SH(((args[2] & 63) & 0x1f) | args[2] & 63) >> 5) & 1) 
<< 1));
 tcg_out32(s, SRADI | RA(args[0]) | RS(args[1]) | sh);
 } else {
 tcg_out32(s, SRAD | SAB(args[1], args[0], args[2]));
-- 
2.26.2

Re: [PATCH] tests: fix a memory in test_socket_unix_abstract_good

[xiaoqiang zhao]

在 2020/6/4 上午12:14, Li Qiang 写道:

After build qemu with '-fsanitize=address' extra-cflags,
'make check' show following leak:

=
==44580==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2500 byte(s) in 1 object(s) allocated from:
 #0 0x7f1b5a8b8d28 in __interceptor_calloc 
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28)
 #1 0x7f1b5a514b10 in g_malloc0 
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10)
 #2 0xd79ea4e4c0ad31c3  ()

SUMMARY: AddressSanitizer: 2500 byte(s) leaked in 1 allocation(s).

Call 'g_rand_free' in the end of function to avoid this.

Fixes: 4d3a329af59("tests/util-sockets: add abstract unix socket cases")
Signed-off-by: Li Qiang 
---
  tests/test-util-sockets.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 2ca1e99f17..ca6671f9bf 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -312,6 +312,7 @@ static void test_socket_unix_abstract_good(void)
  g_thread_join(serv);
  
  g_free(abstract_sock_name);

+g_rand_free(r);
  }
  #endif
  


Reviewed-by:  xiaoqiang zhao 

[PATCH 2/2] lockable: do not rely on optimization for null lockables

[Joe Slater]

If we use QLNULL for null lockables, we can always
use referencing unknown_lock_type as a link time
error indicator.

Signed-off-by: Joe Slater 
---
 include/qemu/lockable.h | 19 ++-
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index 7f7aebb..7fc5750 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -24,21 +24,14 @@ struct QemuLockable {
 QemuLockUnlockFunc *unlock;
 };
 
-#define QLNULL ((QemuLockable *)0)
-
-
-/* This function gives an error if an invalid, non-NULL pointer type is passed
- * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code 
elimination
- * from the compiler, and give the errors already at link time.
+/*
+ *  If unknown_lock_type() is referenced, it means we have tried to passed 
something
+ *  not recognized as lockable to the macros below.  Use QLNULL to 
intentionally pass
+ *  a null lockable.
  */
-#if defined(__OPTIMIZE__) && !defined(__SANITIZE_ADDRESS__)
+#define QLNULL ((QemuLockable *)0)
 void unknown_lock_type(void *);
-#else
-static inline void unknown_lock_type(void *unused)
-{
-abort();
-}
-#endif
+
 
 static inline __attribute__((__always_inline__)) QemuLockable *
 qemu_make_lockable(void *x, QemuLockable *lockable)
-- 
2.7.4

[PATCH 0/2] Use QLNULL for null lockable

[Joe Slater]

We currently will fail to build for optimizations like -Og because they do not
eliminate dead code.  We do not need such clean up if we use QLNULL.  There is 
no
need to produce a QemuLockable that will be thrown away.

Only testing:

$ ../configure
$ make -j16 CFLAGS="$CFLAGS"  # which I set to use -Og, then -O2

Joe Slater (2):
  lockable:  use QLNULL for a null lockable
  lockable: do not rely on optimization for null lockables

 block/block-backend.c  |  4 ++--
 block/block-copy.c |  2 +-
 block/mirror.c |  5 +++--
 fsdev/qemu-fsdev-throttle.c|  6 +++---
 hw/9pfs/9p.c   |  2 +-
 include/qemu/lockable.h| 16 ++--
 util/qemu-co-shared-resource.c |  2 +-
 7 files changed, 17 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH 1/2] lockable: use QLNULL for a null lockable

[Joe Slater]

Allows us to build with -Og and optimizations that
do not clean up dead-code.

Signed-off-by: Joe Slater 

to be squished

Signed-off-by: Joe Slater 
---
 block/block-backend.c  | 4 ++--
 block/block-copy.c | 2 +-
 block/mirror.c | 5 +++--
 fsdev/qemu-fsdev-throttle.c| 6 +++---
 hw/9pfs/9p.c   | 2 +-
 include/qemu/lockable.h| 3 +++
 util/qemu-co-shared-resource.c | 2 +-
 7 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25..92128e8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1174,7 +1174,7 @@ static void coroutine_fn 
blk_wait_while_drained(BlockBackend *blk)
 
 if (blk->quiesce_counter && !blk->disable_request_queuing) {
 blk_dec_in_flight(blk);
-qemu_co_queue_wait(>queued_requests, NULL);
+qemu_co_queue_wait(>queued_requests, QLNULL);
 blk_inc_in_flight(blk);
 }
 }
@@ -2367,7 +2367,7 @@ static void blk_root_drained_end(BdrvChild *child, int 
*drained_end_counter)
 if (blk->dev_ops && blk->dev_ops->drained_end) {
 blk->dev_ops->drained_end(blk->dev_opaque);
 }
-while (qemu_co_enter_next(>queued_requests, NULL)) {
+while (qemu_co_enter_next(>queued_requests, QLNULL)) {
 /* Resume all queued requests */
 }
 }
diff --git a/block/block-copy.c b/block/block-copy.c
index bb8d056..8de0b54 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -120,7 +120,7 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState 
*s, int64_t offset,
 return false;
 }
 
-qemu_co_queue_wait(>wait_queue, NULL);
+qemu_co_queue_wait(>wait_queue, QLNULL);
 
 return true;
 }
diff --git a/block/mirror.c b/block/mirror.c
index e8e8844..76c082d2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -28,6 +28,7 @@
 #define MAX_IO_BYTES (1 << 20) /* 1 Mb */
 #define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
 
+
 /* The mirroring buffer is a list of granularity-sized chunks.
  * Free chunks are organized in a list.
  */
@@ -157,7 +158,7 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp 
*self,
 if (ranges_overlap(self_start_chunk, self_nb_chunks,
op_start_chunk, op_nb_chunks))
 {
-qemu_co_queue_wait(>waiting_requests, NULL);
+qemu_co_queue_wait(>waiting_requests, QLNULL);
 break;
 }
 }
@@ -297,7 +298,7 @@ mirror_wait_for_any_operation(MirrorBlockJob *s, bool 
active)
 if (!op->is_pseudo_op && op->is_in_flight &&
 op->is_active_write == active)
 {
-qemu_co_queue_wait(>waiting_requests, NULL);
+qemu_co_queue_wait(>waiting_requests, QLNULL);
 return;
 }
 }
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 5c83a1c..78d256d 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -22,13 +22,13 @@
 static void fsdev_throttle_read_timer_cb(void *opaque)
 {
 FsThrottle *fst = opaque;
-qemu_co_enter_next(>throttled_reqs[false], NULL);
+qemu_co_enter_next(>throttled_reqs[false], QLNULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
 FsThrottle *fst = opaque;
-qemu_co_enter_next(>throttled_reqs[true], NULL);
+qemu_co_enter_next(>throttled_reqs[true], QLNULL);
 }
 
 int fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
@@ -100,7 +100,7 @@ void coroutine_fn fsdev_co_throttle_request(FsThrottle 
*fst, bool is_write,
 if (throttle_enabled(>cfg)) {
 if (throttle_schedule_timer(>ts, >tt, is_write) ||
 !qemu_co_queue_empty(>throttled_reqs[is_write])) {
-qemu_co_queue_wait(>throttled_reqs[is_write], NULL);
+qemu_co_queue_wait(>throttled_reqs[is_write], QLNULL);
 }
 
 throttle_account(>ts, is_write, iov_size(iov, iovcnt));
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 45a788f..35976e2 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2888,7 +2888,7 @@ static void coroutine_fn v9fs_flush(void *opaque)
 /*
  * Wait for pdu to complete.
  */
-qemu_co_queue_wait(_pdu->complete, NULL);
+qemu_co_queue_wait(_pdu->complete, QLNULL);
 if (!qemu_co_queue_next(_pdu->complete)) {
 cancel_pdu->cancelled = 0;
 pdu_free(cancel_pdu);
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index b620023..7f7aebb 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -24,6 +24,9 @@ struct QemuLockable {
 QemuLockUnlockFunc *unlock;
 };
 
+#define QLNULL ((QemuLockable *)0)
+
+
 /* This function gives an error if an invalid, non-NULL pointer type is passed
  * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code 
elimination
  * from the compiler, and give the errors already 

Re: [PATCH v2 2/2] pci: ensure configuration access is within bounds

[BALATON Zoltan]

On Thu, 4 Jun 2020, P J P wrote:

From: Prasad J Pandit 

While reading PCI configuration bytes, a guest may send an
address towards the end of the configuration space. It may lead
to an OOB access issue. Assert that 'address + len' is within
PCI configuration space.

Suggested-by: Philippe Mathieu-Daudé 
Signed-off-by: Prasad J Pandit 
---
hw/pci/pci.c | 2 ++
1 file changed, 2 insertions(+)

Update v2: assert PCI configuration access is within bounds
 -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00711.html

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 70c66965f5..173bec4fd5 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1381,6 +1381,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
{
uint32_t val = 0;

+assert(address + len <= pci_config_size(d));


Does this allow guest now to crash QEMU? I think it was suggested that 
assert should only be used for cases that can only arise from a 
programming error and not from values set by the guest. If this is 
considered to be an error now to call this function with wrong parameters 
did you check other callers? I've found a few such as:


hw/scsi/esp-pci.c
hw/watchdog/wdt_i6300esb.c
hw/ide/cmd646.c
hw/vfio/pci.c

and maybe others. Would it be better to not crash just log invalid access 
and either fix up parameters or return some garbage like 0?


Regards,
BALATON Zoltan


+
if (pci_is_express_downstream_port(d) &&
ranges_overlap(address, len, d->exp.exp_cap + PCI_EXP_LNKSTA, 2)) {
pcie_sync_bridge_lnk(d);

RE: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding functions

[Babu Moger]

> -Original Message-
> From: Eduardo Habkost 
> Sent: Wednesday, June 3, 2020 10:46 AM
> To: Moger, Babu 
> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> functions
> 
> On Wed, Jun 03, 2020 at 10:38:46AM -0500, Babu Moger wrote:
> >
> >
> > On 6/3/20 10:31 AM, Eduardo Habkost wrote:
> > > On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote:
> > >>
> > >>
> > >>> -Original Message-
> > >>> From: Eduardo Habkost 
> > >>> Sent: Tuesday, June 2, 2020 6:01 PM
> > >>> To: Moger, Babu 
> > >>> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com;
> r...@twiddle.net;
> > >>> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> > >>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> > >>> functions
> > >>>
> > >>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote:
> > 
> > 
> > > -Original Message-
> > > From: Eduardo Habkost 
> > > Sent: Tuesday, June 2, 2020 12:19 PM
> > > To: Moger, Babu 
> > > Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com;
> r...@twiddle.net;
> > > m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology
> decoding
> > > functions
> > >
> > > Hi,
> > >
> > > It looks like this series breaks -device and CPU hotplug:
> > >
> > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote:
> > >> These functions add support for building EPYC mode topology given
> the
> > >>> smp
> > >> details like numa nodes, cores, threads and sockets.
> > >>
> > >> The new apic id decoding is mostly similar to current apic id 
> > >> decoding
> > >> except that it adds a new field node_id when numa configured.
> Removes
> > >>> all
> > >> the hardcoded values. Subsequent patches will use these functions to
> build
> > >> the topology.
> > >>
> > >> Following functions are added.
> > >> apicid_llc_width_epyc
> > >> apicid_llc_offset_epyc
> > >> apicid_pkg_offset_epyc
> > >> apicid_from_topo_ids_epyc
> > >> x86_topo_ids_from_idx_epyc
> > >> x86_topo_ids_from_apicid_epyc
> > >> x86_apicid_from_cpu_idx_epyc
> > >>
> > >> The topology details are available in Processor Programming
> Reference
> > >>> (PPR)
> > >> for AMD Family 17h Model 01h, Revision B1 Processors. The revision
> > >>> guides
> > > are
> > >> available from the bugzilla Link below.
> > >> Link:
> > >
> > >>>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > >
> > >>>
> kernel.org%2Fshow_bug.cgi%3Fid%3D206537data=02%7C01%7Cbabu.m
> > >
> > >>>
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > >
> > >>>
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739sdata=wE0
> > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3Dreserved=0
> > >>
> > >> Signed-off-by: Babu Moger 
> > >> Acked-by: Igor Mammedov 
> > >> Acked-by: Michael S. Tsirkin 
> > >> ---
> > > [...]
> > >>  typedef struct X86CPUTopoIDs {
> > >>  unsigned pkg_id;
> > >> +unsigned node_id;
> > >
> > > You have added a new field here.
> > >
> > >>  unsigned die_id;
> > >>  unsigned core_id;
> > >>  unsigned smt_id;
> > > [...]
> > >> +static inline apic_id_t
> > >> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
> > >> +  const X86CPUTopoIDs *topo_ids)
> > >> +{
> > >> +return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) 
> > >> |
> > >> +   (topo_ids->node_id << 
> > >> apicid_node_offset_epyc(topo_info)) |
> > >
> > > You are using the new field here.
> > >
> > >> +   (topo_ids->die_id  << apicid_die_offset(topo_info)) |
> > >> +   (topo_ids->core_id << apicid_core_offset(topo_info)) |
> > >> +   topo_ids->smt_id;
> > >> +}
> > >
> > > But you are not initializing node_id in one caller of
> apicid_from_topo_ids():
> > >
> > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > > DeviceState *dev, Error **errp)
> > > {
> > > [...]
> > > X86CPUTopoIDs topo_ids;
> > > [...]
> > > if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > > [...]
> > > topo_ids.pkg_id = cpu->socket_id;
> > > topo_ids.die_id = cpu->die_id;
> > > topo_ids.core_id = cpu->core_id;
> > > topo_ids.smt_id = cpu->thread_id;
> > > cpu->apic_id = x86ms->apicid_from_topo_ids(_info,
> _ids);
> > > }
> > > [...]
> > > }
> > >
> > > Result: -device is broken when using -cpu EPYC:
> > >
> > >   

Re: [PATCH v2 1/2] ait-vga: check address before reading configuration bytes

[BALATON Zoltan]

On Thu, 4 Jun 2020, P J P wrote:

From: Prasad J Pandit 

While reading PCI configuration bytes, a guest may send an
address towards the end of the configuration space. It may lead
to an OOB access issue. Add check to ensure 'address + size' is
within PCI configuration space.

Reported-by: Ren Ding 
Reported-by: Hanqing Zhao 
Reported-by: Yi Ren 
Signed-off-by: Prasad J Pandit 
---
hw/display/ati.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)

Update v2: add check to avoid OOB PCI configuration space access
 -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00711.html

diff --git a/hw/display/ati.c b/hw/display/ati.c
index bda4a2d816..6671959e5d 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -384,7 +384,10 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
unsigned int size)
val = s->regs.crtc_pitch;
break;
case 0xf00 ... 0xfff:
-val = pci_default_read_config(>dev, addr - 0xf00, size);
+addr = addr - 0xf00;


I'd write this as addr -= 0xf00 but modifying addr here breaks the trace 
print out at end of function so better drop this line and do


if (addr + size <= 0xfff) {

instead. Or is that really (addr + size - 1 <= 0xfff) considering that 
reading the last byte with addr = 0xfff size = 1 should probably be valid.


Regards,
BALATON Zoltan


+if (addr + size <= 0xff) {
+val = pci_default_read_config(>dev, addr, size);
+}
break;
case CUR_OFFSET:
val = s->regs.cur_offset;

Re: [PATCH v2] ati-vga: check mm_index before recursive call

[BALATON Zoltan]

On Thu, 4 Jun 2020, P J P wrote:

From: Prasad J Pandit 

While accessing VGA registers via ati_mm_read/write routines,
a guest may set 's->regs.mm_index' such that it leads to infinite
recursion. Check mm_index value to avoid it.

Reported-by: Ren Ding 
Reported-by: Hanqing Zhao 
Reported-by: Yi Ren 


Suggested-by: BALATON Zoltan 

therefore I think this should work but someone else should give 
Reviewed-by to cross check this.


Regards,
BALATON Zoltan


Signed-off-by: Prasad J Pandit 
---
hw/display/ati.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Update v2: add check before recursive call
 -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00781.html

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 065f197678..bda4a2d816 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -285,7 +285,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
unsigned int size)
if (idx <= s->vga.vram_size - size) {
val = ldn_le_p(s->vga.vram_ptr + idx, size);
}
-} else {
+} else if (s->regs.mm_index > MM_DATA + 3) {
val = ati_mm_read(s, s->regs.mm_index + addr - MM_DATA, size);
}
break;
@@ -520,7 +520,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
if (idx <= s->vga.vram_size - size) {
stn_le_p(s->vga.vram_ptr + idx, size, data);
}
-} else {
+} else if (s->regs.mm_index > MM_DATA + 3) {
ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
}
break;

[PATCH v2 1/2] ait-vga: check address before reading configuration bytes

[P J P]

From: Prasad J Pandit 

While reading PCI configuration bytes, a guest may send an
address towards the end of the configuration space. It may lead
to an OOB access issue. Add check to ensure 'address + size' is
within PCI configuration space.

Reported-by: Ren Ding 
Reported-by: Hanqing Zhao 
Reported-by: Yi Ren 
Signed-off-by: Prasad J Pandit 
---
 hw/display/ati.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

Update v2: add check to avoid OOB PCI configuration space access
  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00711.html

diff --git a/hw/display/ati.c b/hw/display/ati.c
index bda4a2d816..6671959e5d 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -384,7 +384,10 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
unsigned int size)
 val = s->regs.crtc_pitch;
 break;
 case 0xf00 ... 0xfff:
-val = pci_default_read_config(>dev, addr - 0xf00, size);
+addr = addr - 0xf00;
+if (addr + size <= 0xff) {
+val = pci_default_read_config(>dev, addr, size);
+}
 break;
 case CUR_OFFSET:
 val = s->regs.cur_offset;
-- 
2.26.2

[PATCH v2 2/2] pci: ensure configuration access is within bounds

[P J P]

From: Prasad J Pandit 

While reading PCI configuration bytes, a guest may send an
address towards the end of the configuration space. It may lead
to an OOB access issue. Assert that 'address + len' is within
PCI configuration space.

Suggested-by: Philippe Mathieu-Daudé 
Signed-off-by: Prasad J Pandit 
---
 hw/pci/pci.c | 2 ++
 1 file changed, 2 insertions(+)

Update v2: assert PCI configuration access is within bounds
  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00711.html

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 70c66965f5..173bec4fd5 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1381,6 +1381,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
 {
 uint32_t val = 0;
 
+assert(address + len <= pci_config_size(d));
+
 if (pci_is_express_downstream_port(d) &&
 ranges_overlap(address, len, d->exp.exp_cap + PCI_EXP_LNKSTA, 2)) {
 pcie_sync_bridge_lnk(d);
-- 
2.26.2

[PATCH v2 0/2] Ensure PCI configuration access is within bounds

[P J P]

From: Prasad J Pandit 

Hello,

This patch series fixes

1. While reading PCI configuration bytes, a guest may send an address towards
   the end of the configuration space. It may lead to an OOB access issue.
   Add check to ensure 'addr + size' is within bounds.

2. Assert that PCI configuration access is within bounds.


Thank you.
--
Prasad J Pandit (2):
  ait-vga: check address before reading configuration bytes
  pci: ensure configuration access is within bounds

 hw/display/ati.c | 5 -
 hw/pci/pci.c | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.26.2

Re: [PATCH v3] migration/xbzrle: add encoding rate

[Richard Henderson]

On Wed, 29 Apr 2020 at 18:54, Wei Wang  wrote:
> +if (xbzrle_counters.pages == rs->xbzrle_pages_prev) {
> +xbzrle_counters.encoding_rate = 0;
> +} else if (!encoded_size) {
> +xbzrle_counters.encoding_rate = UINT64_MAX;
> +} else {
> +xbzrle_counters.encoding_rate = unencoded_size / encoded_size;
> +}

With clang 10, this produces

  CC  aarch64-softmmu/migration/ram.o
/home/rth/qemu/qemu/migration/ram.c:919:45: error: implicit conversion
from 'unsigned long' to 'double' changes value from
18446744073709551615 to 18446744073709551616
[-Werror,-Wimplicit-int-float-conversion]
xbzrle_counters.encoding_rate = UINT64_MAX;
  ~ ^~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
# define UINT64_MAX (__UINT64_C(18446744073709551615))
 ^~~~
/usr/include/stdint.h:107:25: note: expanded from macro '__UINT64_C'
#  define __UINT64_C(c) c ## UL
^~~
:36:1: note: expanded from here
18446744073709551615UL
^~
1 error generated.

UINT64_MAX apprears both arbitrary and wrong.

The only way I can imagine encoded_size == 0 is unencoded_size == 0,
so 0 seems like the correct answer.  Moreover, it really seems like
the first test sufficiently covers that possibility.

In addition, the only user of this value is

> +monitor_printf(mon, "xbzrle encoding rate: %0.2f\n",
> +   info->xbzrle_cache->encoding_rate);

which would be quite happy to print NaN even if the 0/0 computation
were to run.  Though as I say above, I don't think that's reachable.


r~

Re: [PATCH v3] osdep: Make MIN/MAX evaluate arguments only once

[Eric Blake]

On 6/3/20 11:32 AM, Richard Henderson wrote:


I'd prefer we generate a compile-time error than a runtime trap (or nothing,
depending on compiler flags controlling __builtin_unreachable).


What we have DOES produce a compile-time error.  If either expression to
MIN_CONST() is not actually const, the fact that __builtin_unreachable()
returns void causes a compilation failure because a value is expected.


Ah!  Well, that's good and certainly sufficient for my needs.

I do now wonder if it wouldn't be clearer to use "(void)0"
instead of __builtin_unreachable, and add a note to the comment just above.


Yes, I just tested; using "((void)0)" in place of 
"__builtin_unreachable()" has the same effect (no change to valid use, 
and still a compiler error on misuse). gcc:


/home/eblake/qemu/qemu-img.c: In function ‘is_allocated_sectors’:
/home/eblake/qemu/qemu-img.c:1225:15: error: void value not ignored as 
it ought to be

 1225 | i = MIN_CONST(i, n);
  |   ^

clang:

/home/eblake/qemu/qemu-img.c:1225:15: error: assigning to 'int' from 
incompatible type 'void'

i = MIN_CONST(i, n);
  ^ ~~~


Of course, a comment explaining the intent can't hurt either.  I'll wait 
to see if this gathers any other comments before spinning a v4 with that 
change.


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

Re: [PATCH v2] ati-vga: check mm_index before recursive call

[Philippe Mathieu-Daudé]

On 6/3/20 8:55 PM, P J P wrote:
> From: Prasad J Pandit 
> 
> While accessing VGA registers via ati_mm_read/write routines,
> a guest may set 's->regs.mm_index' such that it leads to infinite
> recursion. Check mm_index value to avoid it.
> 
> Reported-by: Ren Ding 
> Reported-by: Hanqing Zhao 
> Reported-by: Yi Ren 
> Signed-off-by: Prasad J Pandit 
> ---
>  hw/display/ati.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Update v2: add check before recursive call
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00781.html
> 
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 065f197678..bda4a2d816 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -285,7 +285,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
> unsigned int size)
>  if (idx <= s->vga.vram_size - size) {
>  val = ldn_le_p(s->vga.vram_ptr + idx, size);
>  }
> -} else {
> +} else if (s->regs.mm_index > MM_DATA + 3) {
>  val = ati_mm_read(s, s->regs.mm_index + addr - MM_DATA, size);

We usually log unexpected guest accesses with:

   } else {
   qemu_log_mask(LOG_GUEST_ERROR, ...

>  }
>  break;
> @@ -520,7 +520,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>  if (idx <= s->vga.vram_size - size) {
>  stn_le_p(s->vga.vram_ptr + idx, size, data);
>  }
> -} else {
> +} else if (s->regs.mm_index > MM_DATA + 3) {
>  ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);

Ditto:

   } else {
   qemu_log_mask(LOG_GUEST_ERROR, ...

>  }
>  break;
> 

Re: [PATCH] ati-vga: increment mm_index in ati_mm_read/write

[P J P]

+-- On Wed, 3 Jun 2020, BALATON Zoltan wrote --+
| or even > MM_DATA + 3 may be best as that only refers to defines used in 
| that case. So maybe
| 
| +  } else if (s->regs.mm_index > MM_DATA + 3) {
| > ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
| >  }
| >
| > and do the same in ati_mm_read() as well.

Sent patch v2. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[PATCH v2] ati-vga: check mm_index before recursive call

[P J P]

From: Prasad J Pandit 

While accessing VGA registers via ati_mm_read/write routines,
a guest may set 's->regs.mm_index' such that it leads to infinite
recursion. Check mm_index value to avoid it.

Reported-by: Ren Ding 
Reported-by: Hanqing Zhao 
Reported-by: Yi Ren 
Signed-off-by: Prasad J Pandit 
---
 hw/display/ati.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Update v2: add check before recursive call
  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00781.html

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 065f197678..bda4a2d816 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -285,7 +285,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
unsigned int size)
 if (idx <= s->vga.vram_size - size) {
 val = ldn_le_p(s->vga.vram_ptr + idx, size);
 }
-} else {
+} else if (s->regs.mm_index > MM_DATA + 3) {
 val = ati_mm_read(s, s->regs.mm_index + addr - MM_DATA, size);
 }
 break;
@@ -520,7 +520,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
 if (idx <= s->vga.vram_size - size) {
 stn_le_p(s->vga.vram_ptr + idx, size, data);
 }
-} else {
+} else if (s->regs.mm_index > MM_DATA + 3) {
 ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
 }
 break;
-- 
2.26.2

Re: [PATCH v3 14/20] numa: Handle virtio-mem in NUMA stats

[Pankaj Gupta]

> Account the memory to the configured nid.
>
> Cc: Eduardo Habkost 
> Cc: Marcel Apfelbaum 
> Cc: "Michael S. Tsirkin" 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/core/numa.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 316bc50d75..06960918e7 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -812,6 +812,7 @@ static void numa_stat_memory_devices(NumaNodeMem 
> node_mem[])
>  MemoryDeviceInfoList *info;
>  PCDIMMDeviceInfo *pcdimm_info;
>  VirtioPMEMDeviceInfo *vpi;
> +VirtioMEMDeviceInfo *vmi;
>
>  for (info = info_list; info; info = info->next) {
>  MemoryDeviceInfo *value = info->value;
> @@ -832,6 +833,11 @@ static void numa_stat_memory_devices(NumaNodeMem 
> node_mem[])
>  node_mem[0].node_mem += vpi->size;
>  node_mem[0].node_plugged_mem += vpi->size;
>  break;
> +case MEMORY_DEVICE_INFO_KIND_VIRTIO_MEM:
> +vmi = value->u.virtio_mem.data;
> +node_mem[vmi->node].node_mem += vmi->size;
> +node_mem[vmi->node].node_plugged_mem += vmi->size;
> +break;
>  default:
>  g_assert_not_reached();
>  }

Reviewed-by: Pankaj Gupta 

[PATCH] hw/sd/sdcard: Restrict Class 6 commands to SCSD cards

[Philippe Mathieu-Daudé]

Only SCSD cards support Class 6 (Block Oriented Write Protection)
commands.

  "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"

  4.3.14 Command Functional Difference in Card Capacity Types

  * Write Protected Group

  SDHC and SDXC do not support write-protected groups. Issuing
  CMD28, CMD29 and CMD30 generates the ILLEGAL_COMMAND error.

Signed-off-by: Philippe Mathieu-Daudé 
---
This patch doesn't fix CVE-2020-13253, but greatly reduce
QEMU exposure to it.
https://bugs.launchpad.net/qemu/+bug/1880822
---
 hw/sd/sd.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 3c06a0ac6d..da39590f58 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -905,6 +905,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
SDRequest req)
 sd->multi_blk_cnt = 0;
 }
 
+if (sd_cmd_class[req.cmd] == 6 && FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) 
{
+/* Only Standard Capacity cards support class 6 commands */
+return sd_illegal;
+}
+
 switch (req.cmd) {
 /* Basic commands (Class 0 and Class 1) */
 case 0:/* CMD0:   GO_IDLE_STATE */
-- 
2.21.3

Re: [RFC PATCH 2/2] linux-user/mmap: Fix Clang 'type-limit-compare' warning

[Thomas Huth]

On 03/06/2020 20.01, Richard Henderson wrote:
> On 6/3/20 9:06 AM, Eric Blake wrote:
>> Instead of using #if, the following suffices to shut up clang:
>>
>> diff --git c/linux-user/mmap.c w/linux-user/mmap.c
>> index e37803379747..8d9ba201625d 100644
>> --- c/linux-user/mmap.c
>> +++ w/linux-user/mmap.c
>> @@ -715,7 +715,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong 
>> old_size,
>>  host_addr = MAP_FAILED;
>>  }
>>  /* Check if address fits target address space */
>> -    if ((unsigned long)host_addr + new_size > (abi_ulong)-1) {
>> +    if ((unsigned long)host_addr > (abi_ulong)-1 - new_size) {
>>  /* Revert mremap() changes */
>>  host_addr = mremap(g2h(old_addr), new_size, old_size, flags);
>>  errno = ENOMEM;
>>
>>
>> That is, it is no longer a tautological type compare if you commute the
>> operations so that neither side is a compile-time constant.
> 
> To some extent the tautological compare is a hint to the compiler that the
> comparison may be optimized away.  If sizeof(abi_ulong) >= sizeof(unsigned
> long), then the host *cannot* produce an out-of-range target address.
> 
> We could add the sizeof test to the if, to preserve the optimization, but that
> by itself doesn't prevent the clang warning.
> 
> Which is why I have repeatedly suggested that we disable this warning 
> globally.

I guess most people (like me) don't have a strong opinion about this. So
maybe simply suggest a patch to disable the warning? I don't think that
anybody will object.

 Thomas

Re: [RFC PATCH 2/2] linux-user/mmap: Fix Clang 'type-limit-compare' warning

[Richard Henderson]

On 6/3/20 9:06 AM, Eric Blake wrote:
> Instead of using #if, the following suffices to shut up clang:
> 
> diff --git c/linux-user/mmap.c w/linux-user/mmap.c
> index e37803379747..8d9ba201625d 100644
> --- c/linux-user/mmap.c
> +++ w/linux-user/mmap.c
> @@ -715,7 +715,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong 
> old_size,
>  host_addr = MAP_FAILED;
>  }
>  /* Check if address fits target address space */
> -    if ((unsigned long)host_addr + new_size > (abi_ulong)-1) {
> +    if ((unsigned long)host_addr > (abi_ulong)-1 - new_size) {
>  /* Revert mremap() changes */
>  host_addr = mremap(g2h(old_addr), new_size, old_size, flags);
>  errno = ENOMEM;
> 
> 
> That is, it is no longer a tautological type compare if you commute the
> operations so that neither side is a compile-time constant.

To some extent the tautological compare is a hint to the compiler that the
comparison may be optimized away.  If sizeof(abi_ulong) >= sizeof(unsigned
long), then the host *cannot* produce an out-of-range target address.

We could add the sizeof test to the if, to preserve the optimization, but that
by itself doesn't prevent the clang warning.

Which is why I have repeatedly suggested that we disable this warning globally.
 Because every single instance so far has been of this form: where we are
expecting the compiler to fold away the block of code protected by the
tautological comparison.

I will also note that there is an existing off-by-one problem wrt the final
page: with a 12-bit page,

  0xf000 + 0x1000 > 0x

The proper test I think is

  (uintptr_t)host_addr + new_size - 1 > (abi_ulong)-1

If we're going to use your suggestion above, this becomes

  (uintptr_t)host_addr > (abi_ulong)-new_size


r~

Re: [PATCH v3] xen: fix build without pci passthrough

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200603160442.3151170-1-anthony.per...@citrix.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  io/channel-socket.o
In file included from /tmp/qemu-test/src/hw/xen/xen_pt.h:4,
 from /tmp/qemu-test/src/stubs/xen-pt.c:9:
/tmp/qemu-test/src/include/hw/xen/xen_common.h:13:10: fatal error: xenctrl.h: 
No such file or directory
 #include 
  ^~~
compilation terminated.
make: *** [/tmp/qemu-test/src/rules.mak:69: stubs/xen-pt.o] Error 1
make: *** Waiting for unfinished jobs
  CC  io/channel-tls.o
  CC  io/channel-watch.o
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=cd03ecea4c4345298864529c8bd56cc2', '-u', 
'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-fmy3mf1z/src/docker-src.2020-06-03-13.42.19.22059:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=cd03ecea4c4345298864529c8bd56cc2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-fmy3mf1z/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real2m17.803s
user0m9.104s


The full log is available at
http://patchew.org/logs/20200603160442.3151170-1-anthony.per...@citrix.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Alex Bennée]

Robert Foley  writes:

> On Wed, 3 Jun 2020 at 07:43, Alex Bennée  wrote:
>>
>>
>> Robert Foley  writes:
>>
> 
>> >
>> > When testing out the options, I noticed that
>> > if we supply arguments of "read", and "write", then we will only get
>> > the last one set, "write", since rw gets overwritten.
>> > One option would be to error out if more than one of these read/write
>> > args is supplied.
>>
>> Yeah the option parsing is a little clunky although given the way you
>> pass them from the QEMU command line perhaps not too worth finessing.
>> The default is rw so you make a conscious decision to only care about one
>> or the other.
>>
>> All you can really do is fail to initialise the plugin. Hopefully the
>> output should be enough clue.
>>
>> >
>> > Reviewed-by: Robert Foley 
>> > Tested-by: Robert Foley 
>>
>> Thanks.
>>
>> Out of interest what did you measure? Are there any useful use cases you can
>> think of?
>
> We did some testing where we booted an aarch64 VM and an i386 VM a few times
> with differentcore counts (up to 64), and viewed the counters.  We
> also did a test where
> we inserted another device (a virtfs mount), booted up and checked
> that there was another
> device listed (for virtio-9p).
>
> There are a few useful use cases we are thinking of, in general for debug/perf
>  testing of PCI devices/drivers.
> For example, debug and performance test of a case where we use a queue pair,
> (maybe for something like DPDK/SPDK), this plugin would be interesting for
> checking that the quantity and locations of accesses are expected.

So one thing that has come up in the VIRT-366 discussion is the
potential efficiencies of the various kick models for MMIO based
hypervisors. Each interaction with a trapped region of memory triggers a
vmexit and one thing I wanted to understand for example was the
difference between "normal" IRQs and MSIs.

-- 
Alex Bennée

Re: [PATCH] tests: fix a memory in test_socket_unix_abstract_good

[Philippe Mathieu-Daudé]

On 6/3/20 6:14 PM, Li Qiang wrote:
> After build qemu with '-fsanitize=address' extra-cflags,
> 'make check' show following leak:
> 
> =
> ==44580==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 2500 byte(s) in 1 object(s) allocated from:
> #0 0x7f1b5a8b8d28 in __interceptor_calloc 
> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28)
> #1 0x7f1b5a514b10 in g_malloc0 
> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10)
> #2 0xd79ea4e4c0ad31c3  ()
> 
> SUMMARY: AddressSanitizer: 2500 byte(s) leaked in 1 allocation(s).
> 
> Call 'g_rand_free' in the end of function to avoid this.

GLib doc doesn't seem complete (they don't mention explicitly the
generator returned from g_rand_new has to be released with g_rand_free).

Reviewed-by: Philippe Mathieu-Daudé 

> 
> Fixes: 4d3a329af59("tests/util-sockets: add abstract unix socket cases")
> Signed-off-by: Li Qiang 
> ---
>  tests/test-util-sockets.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
> index 2ca1e99f17..ca6671f9bf 100644
> --- a/tests/test-util-sockets.c
> +++ b/tests/test-util-sockets.c
> @@ -312,6 +312,7 @@ static void test_socket_unix_abstract_good(void)
>  g_thread_join(serv);
>  
>  g_free(abstract_sock_name);
> +g_rand_free(r);
>  }
>  #endif
>  
> 

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Alex Bennée]

Peter Maydell  writes:

> On Tue, 2 Jun 2020 at 16:54, Alex Bennée  wrote:
>>
>> This is a plugin intended to help with profiling access to various
>> bits of system hardware. It only really makes sense for system
>> emulation.
>>
>> It takes advantage of the recently exposed helper API that allows us
>> to see the device name (memory region name) associated with a device.
>
> This feels like we've let the plugin API get slightly more
> access to QEMU's internals than is ideal. Whether an area
> of memory happens to be an IO memory region or a memory-backed
> one (or whether a device is implemented with one region or
> three, or what names we happened to assign them) is kind of
> a QEMU internal implementation detail.

I'm not so sure it's that much of an implementation detail.

The distinction is between plain RAM and everything else. The details of
the everything else is opaque but the name we pass is public information
(you can get it from "info mtree -o") and you can certainly infer useful
stuff from it. For example the virtio-pci-notify areas are regions of
access that will trap on a real hypervisor so allow us to measure how
many vmexits some software might cause.

At the moment I do make up names for regions that get re-generated due
to "reasons" (I never quite understood what the region code was doing
under the hood). Maybe we should only export names of devices the user
has explicitly tagged with -device foo,id=bar?

What should we do about the offset? Most devices export several regions
and there is no reason why those regions should all be together in the
memory map. Does just exposing a physical address make sense here?

-- 
Alex Bennée

Re: another tst-arm-mte bug: qemu-system segfaults

[Szabolcs Nagy]

The 06/03/2020 09:21, Richard Henderson wrote:
> On 6/3/20 6:50 AM, Szabolcs Nagy wrote:
> > thanks my tests now get further but later i run into
> > the previous assert failure:
> > 
> > target/arm/mte_helper.c:97:allocation_tag_mem: assertion failed: (tag_size 
> > <= in_page)
> > 
> > i might be able to reduce it to a small reproducer
> > this time. i assume that will help.
> 
> Dang, I had hoped that the one fix would cover both -- it's definitely in the
> same area.  Yes, a small reproducer will help, but I will also try again with
> your larger reproducer.

reproducer .c and static exe attached.

the referenced __memcmp_aarch64 is again
from the arm optimized-routines repo.
#include 
#include 
#include 
#include 
#include 

int __memcmp_aarch64 (const void *, const void *, size_t);

#define PR_SET_TAGGED_ADDR_CTRL 55
#define PR_TAGGED_ADDR_ENABLE (1UL << 0)
#define PR_MTE_TCF_SHIFT 1
#define PR_MTE_TCF_SYNC (1UL << PR_MTE_TCF_SHIFT)
#define PR_MTE_TAG_SHIFT 3
#define PROT_MTE 0x20
#define MTE_GRANULE_SIZE 16

void *
alignup_mte (void *p)
{
  return (void *) (((uintptr_t) p + MTE_GRANULE_SIZE - 1)
		   & ~(MTE_GRANULE_SIZE - 1));
}

void *
aligndown_mte (void *p)
{
  return (void *) ((uintptr_t) p & ~(MTE_GRANULE_SIZE - 1));
}

void
tag_buffer_helper (void *p, int len)
{
  char *ptr = p;
  char *end = alignup_mte (ptr + len);
  ptr = aligndown_mte (p);
  for (; ptr < end; ptr += MTE_GRANULE_SIZE)
{
  __arm_mte_set_tag (ptr);
}
}

void *
tag_buffer (void *p, int len)
{
  p = __arm_mte_increment_tag (p, 1);
  tag_buffer_helper (p, len);
  return p;
}

int main (void)
{
  int r = prctl (PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC | (0xfffe << PR_MTE_TAG_SHIFT), 0, 0, 0);
  if (r < 0) return -1;
  char *src1 = mmap (NULL, 4096, PROT_READ | PROT_WRITE | PROT_MTE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
  char *src2 = mmap (NULL, 4096, PROT_READ | PROT_WRITE | PROT_MTE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
  if (src1 == MAP_FAILED) return -1;
  if (src2 == MAP_FAILED) return -1;
  char *s1 = src1;
  char *s2 = src2 + 15;
  for (int i = 0; i < 250; i++)
src1[i] = src2[i] = '?';
  for (int i = 0; i < 200; i++)
s1[i] = s2[i] = 'a' + i % 23;
  s1 = tag_buffer (s1, 200);
  s2 = tag_buffer (s2, 200);
  __memcmp_aarch64(s1, s2, 200);
  return 0;
}


bug2
Description: Binary data

Re: [PATCH v6 4/5] 9pfs: T_readdir latency optimization

[Christian Schoenebeck]

On Sonntag, 19. April 2020 17:06:17 CEST Christian Schoenebeck wrote:
> Make top half really top half and bottom half really bottom half:
> 
> Each T_readdir request handling is hopping between threads (main
> I/O thread and background I/O driver threads) several times for
> every individual directory entry, which sums up to huge latencies
> for handling just a single T_readdir request.
> 
> Instead of doing that, collect now all required directory entries
> (including all potentially required stat buffers for each entry) in
> one rush on a background I/O thread from fs driver by calling the
> previously added function v9fs_co_readdir_many() instead of
> v9fs_co_readdir(), then assemble the entire resulting network
> response message for the readdir request on main I/O thread. The
> fs driver is still aborting the directory entry retrieval loop
> (on the background I/O thread inside of v9fs_co_readdir_many())
> as soon as it would exceed the client's requested maximum R_readdir
> response size. So this will not introduce a performance penalty on
> another end.
> 
> Signed-off-by: Christian Schoenebeck 
> ---
>  hw/9pfs/9p.c | 122 +++
>  1 file changed, 55 insertions(+), 67 deletions(-)

Ping. Anybody?

I would like to roll out this patch set soon and this is the only patch in 
this series missing a review yet.

Re: [PATCH v3] xen: fix build without pci passthrough

[Paolo Bonzini]

On 03/06/20 18:04, Anthony PERARD wrote:
> From: Roger Pau Monne 
> 
> Xen PCI passthrough support may not be available and thus the global
> variable "has_igd_gfx_passthru" might be compiled out. Common code
> should not access it in that case.
> 
> Unfortunately, we can't use CONFIG_XEN_PCI_PASSTHROUGH directly in
> xen-common.c so this patch instead move access to the
> has_igd_gfx_passthru variable via function and those functions are
> also implemented as stubs. The stubs will be used when QEMU is built
> without passthrough support.
> 
> Now, when one will want to enable igd-passthru via the -machine
> property, they will get an error message if QEMU is built without
> passthrough support.
> 
> Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator 
> property')
> Reported-by: Roger Pau Monné 
> Signed-off-by: Anthony PERARD 
> ---
> 
> Notes:
> Changes in v3:
> - reworked to use stubs instead of #ifdef CONFIG_XEN_PCI_PASSTHROUGH
>   CONFIG_XEN_PCI_PASSTHROUGH isn't available in xen-common.c
> 
>   moving CONFIG_XEN_PCI_PASSTHROUGH to be in config_host_mak isn't
>   really possible, or at least I didn't managed to make that work.
> 
>  hw/i386/pc_piix.c   |  2 +-
>  hw/xen/xen-common.c |  4 ++--
>  hw/xen/xen_pt.c | 12 +++-
>  hw/xen/xen_pt.h |  6 --
>  stubs/Makefile.objs |  1 +
>  stubs/xen-pt.c  | 22 ++
>  6 files changed, 41 insertions(+), 6 deletions(-)
>  create mode 100644 stubs/xen-pt.c
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f66e1d73ce0b..347fb8c6c807 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -375,7 +375,7 @@ static void pc_init_isa(MachineState *machine)
>  #ifdef CONFIG_XEN
>  static void pc_xen_hvm_init_pci(MachineState *machine)
>  {
> -const char *pci_type = has_igd_gfx_passthru ?
> +const char *pci_type = xen_igd_gfx_pt_enabled() ?
>  TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : 
> TYPE_I440FX_PCI_DEVICE;
>  
>  pc_init1(machine,
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 70564cc952d5..dd2c22cc4c0b 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -129,12 +129,12 @@ static void xen_change_state_handler(void *opaque, int 
> running,
>  
>  static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>  {
> -return has_igd_gfx_passthru;
> +return xen_igd_gfx_pt_enabled();
>  }
>  
>  static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>  {
> -has_igd_gfx_passthru = value;
> +xen_igd_gfx_pt_set(value, errp);
>  }
>  
>  static void xen_setup_post(MachineState *ms, AccelState *accel)
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 81d5ad8da7f0..ab84443d5ec8 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -65,7 +65,17 @@
>  #include "qemu/range.h"
>  #include "exec/address-spaces.h"
>  
> -bool has_igd_gfx_passthru;
> +static bool has_igd_gfx_passthru;
> +
> +bool xen_igd_gfx_pt_enabled(void)
> +{
> +return has_igd_gfx_passthru;
> +}
> +
> +void xen_igd_gfx_pt_set(bool value, Error **errp)
> +{
> +has_igd_gfx_passthru = value;
> +}
>  
>  #define XEN_PT_NR_IRQS (256)
>  static uint8_t xen_pt_mapped_machine_irq[XEN_PT_NR_IRQS] = {0};
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 179775db7b22..6e9cec95f3b7 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -5,6 +5,9 @@
>  #include "hw/pci/pci.h"
>  #include "xen-host-pci-device.h"
>  
> +bool xen_igd_gfx_pt_enabled(void);
> +void xen_igd_gfx_pt_set(bool value, Error **errp);
> +
>  void xen_pt_log(const PCIDevice *d, const char *f, ...) GCC_FMT_ATTR(2, 3);
>  
>  #define XEN_PT_ERR(d, _f, _a...) xen_pt_log(d, "%s: Error: "_f, __func__, 
> ##_a)
> @@ -322,10 +325,9 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice 
> *dev,
>  unsigned int domain,
>  unsigned int bus, unsigned int 
> slot,
>  unsigned int function);
> -extern bool has_igd_gfx_passthru;
>  static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
>  {
> -return (has_igd_gfx_passthru
> +return (xen_igd_gfx_pt_enabled()
>  && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
>  }
>  int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 6a9e3135e8f9..564527e00997 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -40,6 +40,7 @@ stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += vmgenid.o
>  stub-obj-y += xen-common.o
>  stub-obj-y += xen-hvm.o
> +stub-obj-y += xen-pt.o
>  stub-obj-y += pci-host-piix.o
>  stub-obj-y += ram-block.o
>  stub-obj-y += ramfb.o
> diff --git a/stubs/xen-pt.c b/stubs/xen-pt.c
> new file mode 100644
> index ..2d8cac8d54b9
> --- /dev/null
> +++ b/stubs/xen-pt.c
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 

Re: [PATCH v3] xen: fix build without pci passthrough

[Roger Pau Monné]

On Wed, Jun 03, 2020 at 05:04:42PM +0100, Anthony PERARD wrote:
> From: Roger Pau Monne 
> 
> Xen PCI passthrough support may not be available and thus the global
> variable "has_igd_gfx_passthru" might be compiled out. Common code
> should not access it in that case.
> 
> Unfortunately, we can't use CONFIG_XEN_PCI_PASSTHROUGH directly in
> xen-common.c so this patch instead move access to the
> has_igd_gfx_passthru variable via function and those functions are
> also implemented as stubs. The stubs will be used when QEMU is built
> without passthrough support.
> 
> Now, when one will want to enable igd-passthru via the -machine
> property, they will get an error message if QEMU is built without
> passthrough support.
> 
> Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator 
> property')
> Reported-by: Roger Pau Monné 
> Signed-off-by: Anthony PERARD 

Tested-by: Roger Pau Monné 

Fixes the build for me on FreeBSD without pci-passthrough.

Thanks!

Re: [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext

[Robert Foley]

On Tue, 2 Jun 2020 at 15:22, Alex Bennée  wrote:
>
>
> Robert Foley  writes:
>
> > From: Lingfeng Yang 

> >
> > +# Thread sanitizer is, for now, much noisier than the other sanitizers;
> > +# keep it separate until that is not the case.
>
> I think we also need to stop both being enabled at once. On my clang-9
> setup I get:
>
>   make: *** [qapi/qobject-output-visitor.o] Error 1
>   clang: error: invalid argument '-fsanitize=address' not allowed with 
> '-fsanitize=thread'
>   clang: error: invalid argument '-fsanitize=address' not allowed with 
> '-fsanitize=thread'
>   clang: errorclang: : errorinvalid argument '-fsanitize=address' not allowed 
> with '-fsanitize=thread':
>   invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
>   clang: error: invalid argument '-fsanitize=address' not allowed with 
> '-fsanitize=thread'

Good point.  I see a similar error if I use --enable-sanitizers and
--enable-tsan.
Will look to add an error check for this.  Wondering if there are any
other interactions
we need to avoid?  Will poke around a bit here.



>
> Are we missing any LDFLAGS? On Ubuntu 18.04 with clang-9 I'm seeing:
>
> LINKqemu-ga
>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o):
>  warning: common of `__interception::real_setjmp' overridden by definition
>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o):
>  warning: defined here

Looks like these warnings come from use of --warn-common, so I think we need to
exclude this when using TSan if we want to silence these warnings from
the TSan libraries.

Thanks & Regards,
-Rob

>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o):
>  warning: common of `__interception::real__setjmp' overridden by definition
>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o):
>  warning: defined here
>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o):
>  warning: common of `__interception::real_sigsetjmp' overridden by definition
>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o):
>  warning: defined here
>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o):
>  warning: common of `__interception::real___sigsetjmp' overridden by 
> definition
>   
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o):
>  warning: defined here
>   
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:41:
>  multiple definition of `__tsan_mutex_linker_init'
>   
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:41:
>  first defined here
>   
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:50:
>  multiple definition of `__tsan_mutex_not_static'
>   
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:50:
>  first defined here
>   
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:55:
>  multiple definition of `__tsan_mutex_read_lock'
>   
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:55:
>  first defined here
>   
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:45:
>  multiple definition of `__tsan_mutex_read_reentrant'
>   
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:45:
>  first defined here
>   
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:64:
>  multiple definition of `__tsan_mutex_recursive_lock'
>   
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:64:
>  first defined here
>   
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:68:
>  multiple definition of `__tsan_mutex_recursive_unlock'
>
> 
>
> --
> Alex Bennée

Re: [PATCH] hw/isa/apm: Convert debug printf()s to trace events

[Richard Henderson]

On 5/24/20 9:48 AM, Philippe Mathieu-Daudé wrote:
> Convert APM_DPRINTF() to trace events and remove ifdef'ry.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/isa/apm.c| 15 +--
>  hw/isa/trace-events |  4 
>  2 files changed, 9 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3] osdep: Make MIN/MAX evaluate arguments only once

[Richard Henderson]

On 6/2/20 7:29 PM, Eric Blake wrote:
> Because:
> 
> #if MIN(...)
> 
> now fails to compile (you can't have { in a preprocessor expression), and:
> 
> #if MIN_CONST(...)
> 
> fails to compile (__builtin_constant_p() is not a preprocessor macro, so it
> warns that it is being treated as 0).  The only fix is to move the MIN() out 
> of
> the #if and into the #define.

Ah, right.  Thanks.

>> Is it possible to use qemu_build_not_reached?
> 
> Possibly.
> 
> /me goes and recompiles; touching osdep.h recompiles the world...
> 
> No, it blows up hard, because qemu_build_not_reached() is not embeddable in an
> expression:

Ah, right, because without -O, qemu_build_not_reached expands to
g_assert_not_reached and not to a symbol marked with QEMU_ERROR.

>> I'd prefer we generate a compile-time error than a runtime trap (or nothing,
>> depending on compiler flags controlling __builtin_unreachable).
> 
> What we have DOES produce a compile-time error.  If either expression to
> MIN_CONST() is not actually const, the fact that __builtin_unreachable()
> returns void causes a compilation failure because a value is expected.

Ah!  Well, that's good and certainly sufficient for my needs.

I do now wonder if it wouldn't be clearer to use "(void)0"
instead of __builtin_unreachable, and add a note to the comment just above.


r~

[PATCH] tests: fix a memory in test_socket_unix_abstract_good

[Li Qiang]

After build qemu with '-fsanitize=address' extra-cflags,
'make check' show following leak:

=
==44580==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2500 byte(s) in 1 object(s) allocated from:
#0 0x7f1b5a8b8d28 in __interceptor_calloc 
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28)
#1 0x7f1b5a514b10 in g_malloc0 
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10)
#2 0xd79ea4e4c0ad31c3  ()

SUMMARY: AddressSanitizer: 2500 byte(s) leaked in 1 allocation(s).

Call 'g_rand_free' in the end of function to avoid this.

Fixes: 4d3a329af59("tests/util-sockets: add abstract unix socket cases")
Signed-off-by: Li Qiang 
---
 tests/test-util-sockets.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 2ca1e99f17..ca6671f9bf 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -312,6 +312,7 @@ static void test_socket_unix_abstract_good(void)
 g_thread_join(serv);
 
 g_free(abstract_sock_name);
+g_rand_free(r);
 }
 #endif
 
-- 
2.17.1

[PULL 06/15] target/riscv: Remove the deprecated CPUs

[Alistair Francis]

Signed-off-by: Alistair Francis 
Reviewed-by: Bin Meng 
---
 docs/system/deprecated.rst  | 33 ++---
 target/riscv/cpu.h  |  7 ---
 target/riscv/cpu.c  | 28 
 tests/qtest/machine-none-test.c |  4 ++--
 4 files changed, 20 insertions(+), 52 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 50927bad74..bb14de9848 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -314,21 +314,6 @@ should be used instead of the 1.09.1 version.
 System emulator CPUS
 
 
-RISC-V ISA CPUs (since 4.1)
-'''
-
-The RISC-V cpus with the ISA version in the CPU name have been depcreated. The
-four CPUs are: ``rv32gcsu-v1.9.1``, ``rv32gcsu-v1.10.0``, ``rv64gcsu-v1.9.1`` 
and
-``rv64gcsu-v1.10.0``. Instead the version can be specified via the CPU 
``priv_spec``
-option when using the ``rv32`` or ``rv64`` CPUs.
-
-RISC-V ISA CPUs (since 4.1)
-'''
-
-The RISC-V no MMU cpus have been depcreated. The two CPUs: ``rv32imacu-nommu`` 
and
-``rv64imacu-nommu`` should no longer be used. Instead the MMU status can be 
specified
-via the CPU ``mmu`` option when using the ``rv32`` or ``rv64`` CPUs.
-
 ``compat`` property of server class POWER CPUs (since 5.0)
 ''
 
@@ -486,6 +471,24 @@ The ``hub_id`` parameter of ``hostfwd_add`` / 
``hostfwd_remove`` (removed in 5.0
 The ``[hub_id name]`` parameter tuple of the 'hostfwd_add' and
 'hostfwd_remove' HMP commands has been replaced by ``netdev_id``.
 
+System emulator CPUS
+
+
+RISC-V ISA Specific CPUs (removed in 5.1)
+'
+
+The RISC-V cpus with the ISA version in the CPU name have been removed. The
+four CPUs are: ``rv32gcsu-v1.9.1``, ``rv32gcsu-v1.10.0``, ``rv64gcsu-v1.9.1`` 
and
+``rv64gcsu-v1.10.0``. Instead the version can be specified via the CPU 
``priv_spec``
+option when using the ``rv32`` or ``rv64`` CPUs.
+
+RISC-V no MMU CPUs (removed in 5.1)
+'''
+
+The RISC-V no MMU cpus have been removed. The two CPUs: ``rv32imacu-nommu`` and
+``rv64imacu-nommu`` can no longer be used. Instead the MMU status can be 
specified
+via the CPU ``mmu`` option when using the ``rv32`` or ``rv64`` CPUs.
+
 System emulator machines
 
 
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index d0e7f5b9c5..76b98d7a33 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -40,13 +40,6 @@
 #define TYPE_RISCV_CPU_SIFIVE_E51   RISCV_CPU_TYPE_NAME("sifive-e51")
 #define TYPE_RISCV_CPU_SIFIVE_U34   RISCV_CPU_TYPE_NAME("sifive-u34")
 #define TYPE_RISCV_CPU_SIFIVE_U54   RISCV_CPU_TYPE_NAME("sifive-u54")
-/* Deprecated */
-#define TYPE_RISCV_CPU_RV32IMACU_NOMMU  RISCV_CPU_TYPE_NAME("rv32imacu-nommu")
-#define TYPE_RISCV_CPU_RV32GCSU_V1_09_1 RISCV_CPU_TYPE_NAME("rv32gcsu-v1.9.1")
-#define TYPE_RISCV_CPU_RV32GCSU_V1_10_0 RISCV_CPU_TYPE_NAME("rv32gcsu-v1.10.0")
-#define TYPE_RISCV_CPU_RV64IMACU_NOMMU  RISCV_CPU_TYPE_NAME("rv64imacu-nommu")
-#define TYPE_RISCV_CPU_RV64GCSU_V1_09_1 RISCV_CPU_TYPE_NAME("rv64gcsu-v1.9.1")
-#define TYPE_RISCV_CPU_RV64GCSU_V1_10_0 RISCV_CPU_TYPE_NAME("rv64gcsu-v1.10.0")
 
 #define RV32 ((target_ulong)1 << (TARGET_LONG_BITS - 2))
 #define RV64 ((target_ulong)2 << (TARGET_LONG_BITS - 2))
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 059d71f2c7..112f2e3a2f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -135,16 +135,6 @@ static void riscv_base32_cpu_init(Object *obj)
 set_misa(env, 0);
 }
 
-static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
-{
-CPURISCVState *env = _CPU(obj)->env;
-set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-set_priv_version(env, PRIV_VERSION_1_09_1);
-set_resetvec(env, DEFAULT_RSTVEC);
-set_feature(env, RISCV_FEATURE_MMU);
-set_feature(env, RISCV_FEATURE_PMP);
-}
-
 static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
 {
 CPURISCVState *env = _CPU(obj)->env;
@@ -182,16 +172,6 @@ static void riscv_base64_cpu_init(Object *obj)
 set_misa(env, 0);
 }
 
-static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
-{
-CPURISCVState *env = _CPU(obj)->env;
-set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-set_priv_version(env, PRIV_VERSION_1_09_1);
-set_resetvec(env, DEFAULT_RSTVEC);
-set_feature(env, RISCV_FEATURE_MMU);
-set_feature(env, RISCV_FEATURE_PMP);
-}
-
 static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
 {
 CPURISCVState *env = _CPU(obj)->env;
@@ -621,18 +601,10 @@ static const TypeInfo riscv_cpu_type_infos[] = {
 DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,   rv32imacu_nommu_cpu_init),
 DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E34,   rv32imafcu_nommu_cpu_init),
 DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,   rv32gcsu_priv1_10_0_cpu_init),
-

[PULL 08/15] docs: deprecated: Update the -bios documentation

[Alistair Francis]

Update the -bios deprecation documentation to describe the new
behaviour.

Signed-off-by: Alistair Francis 
Reviewed-by: Bin Meng 
---
 docs/system/deprecated.rst | 28 +---
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index d177609cbc..544ece0a45 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -138,25 +138,23 @@ the backing storage specified with ``-mem-path`` can 
actually provide
 the guest RAM configured with ``-m`` and QEMU will fail to start up if
 RAM allocation is unsuccessful.
 
-RISC-V ``-bios`` (since 4.1)
+RISC-V ``-bios`` (since 5.1)
 
 
 QEMU 4.1 introduced support for the -bios option in QEMU for RISC-V for the
-RISC-V virt machine and sifive_u machine.
-
-QEMU 4.1 has no changes to the default behaviour to avoid breakages. This
-default will change in a future QEMU release, so please prepare now. All users
-of the virt or sifive_u machine must change their command line usage.
-
-QEMU 4.1 has three options, please migrate to one of these three:
- 1. ``-bios none`` - This is the current default behavior if no -bios option
-  is included. QEMU will not automatically load any firmware. It is up
+RISC-V virt machine and sifive_u machine. QEMU 4.1 had no changes to the
+default behaviour to avoid breakages.
+
+QEMU 5.1 changes the default behaviour from ``-bios none`` to ``-bios 
default``.
+
+QEMU 5.1 has three options:
+ 1. ``-bios default`` - This is the current default behavior if no -bios option
+  is included. This option will load the default OpenSBI firmware 
automatically.
+  The firmware is included with the QEMU release and no user interaction is
+  required. All a user needs to do is specify the kernel they want to boot
+  with the -kernel option
+ 2. ``-bios none`` - QEMU will not automatically load any firmware. It is up
   to the user to load all the images they need.
- 2. ``-bios default`` - In a future QEMU release this will become the default
-  behaviour if no -bios option is specified. This option will load the
-  default OpenSBI firmware automatically. The firmware is included with
-  the QEMU release and no user interaction is required. All a user needs
-  to do is specify the kernel they want to boot with the -kernel option
  3. ``-bios `` - Tells QEMU to load the specified file as the firmwrae.
 
 ``-tb-size`` option (since 5.0)
-- 
2.26.2

[PULL 04/15] hw/riscv: virt: Remove the riscv_ prefix of the machine* functions

[Alistair Francis]

From: Bin Meng 

Remove the riscv_ prefix of the machine* functions.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Alistair Francis 
Message-id: 1590072147-13035-2-git-send-email-bmeng...@gmail.com
Message-Id: <1590072147-13035-2-git-send-email-bmeng...@gmail.com>
Signed-off-by: Alistair Francis 
---
 hw/riscv/virt.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 7ce28895bc..4e4c494a70 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -471,7 +471,7 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion 
*sys_mem,
 return dev;
 }
 
-static void riscv_virt_board_init(MachineState *machine)
+static void virt_machine_init(MachineState *machine)
 {
 const struct MemmapEntry *memmap = virt_memmap;
 RISCVVirtState *s = RISCV_VIRT_MACHINE(machine);
@@ -632,32 +632,32 @@ static void riscv_virt_board_init(MachineState *machine)
 g_free(plic_hart_config);
 }
 
-static void riscv_virt_machine_instance_init(Object *obj)
+static void virt_machine_instance_init(Object *obj)
 {
 }
 
-static void riscv_virt_machine_class_init(ObjectClass *oc, void *data)
+static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 
 mc->desc = "RISC-V VirtIO board";
-mc->init = riscv_virt_board_init;
+mc->init = virt_machine_init;
 mc->max_cpus = 8;
 mc->default_cpu_type = VIRT_CPU;
 mc->pci_allow_0_address = true;
 }
 
-static const TypeInfo riscv_virt_machine_typeinfo = {
+static const TypeInfo virt_machine_typeinfo = {
 .name   = MACHINE_TYPE_NAME("virt"),
 .parent = TYPE_MACHINE,
-.class_init = riscv_virt_machine_class_init,
-.instance_init = riscv_virt_machine_instance_init,
+.class_init = virt_machine_class_init,
+.instance_init = virt_machine_instance_init,
 .instance_size = sizeof(RISCVVirtState),
 };
 
-static void riscv_virt_machine_init_register_types(void)
+static void virt_machine_init_register_types(void)
 {
-type_register_static(_virt_machine_typeinfo);
+type_register_static(_machine_typeinfo);
 }
 
-type_init(riscv_virt_machine_init_register_types)
+type_init(virt_machine_init_register_types)
-- 
2.26.2

[PULL 01/15] riscv: Suppress the error report for QEMU testing with riscv_find_firmware()

[Alistair Francis]

From: Bin Meng 

We only ship plain binary bios images in the QEMU source. With Spike
machine that uses ELF images as the default bios, running QEMU test
will complain hence let's suppress the error report for QEMU testing.

Signed-off-by: Bin Meng 
Reviewed-by: Anup Patel 
Message-Id: <1588348254-7241-6-git-send-email-bmeng...@gmail.com>
Signed-off-by: Alistair Francis 
---
 hw/riscv/boot.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 726300a171..da5817d438 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -88,9 +88,17 @@ char *riscv_find_firmware(const char *firmware_filename)
 
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
 if (filename == NULL) {
-error_report("Unable to load the RISC-V firmware \"%s\"",
- firmware_filename);
-exit(1);
+if (!qtest_enabled()) {
+/*
+ * We only ship plain binary bios images in the QEMU source.
+ * With Spike machine that uses ELF images as the default bios,
+ * running QEMU test will complain hence let's suppress the error
+ * report for QEMU testing.
+ */
+error_report("Unable to load the RISC-V firmware \"%s\"",
+ firmware_filename);
+exit(1);
+}
 }
 
 return filename;
-- 
2.26.2

Re: [PATCH v5 0/4] introduction of migration_version attribute for VFIO live migration

[Alex Williamson]

On Wed, 3 Jun 2020 01:24:43 -0400
Yan Zhao  wrote:

> On Tue, Jun 02, 2020 at 09:55:28PM -0600, Alex Williamson wrote:
> > On Tue, 2 Jun 2020 23:19:48 -0400
> > Yan Zhao  wrote:
> >   
> > > On Tue, Jun 02, 2020 at 04:55:27PM -0600, Alex Williamson wrote:  
> > > > On Wed, 29 Apr 2020 20:39:50 -0400
> > > > Yan Zhao  wrote:
> > > > 
> > > > > On Wed, Apr 29, 2020 at 05:48:44PM +0800, Dr. David Alan Gilbert 
> > > > > wrote:
> > > > > 
> > > > > > > > > > > > > > > > > > An mdev type is meant to define a software 
> > > > > > > > > > > > > > > > > > compatible interface, so in
> > > > > > > > > > > > > > > > > > the case of mdev->mdev migration, doesn't 
> > > > > > > > > > > > > > > > > > migrating to a different type
> > > > > > > > > > > > > > > > > > fail the most basic of compatibility tests 
> > > > > > > > > > > > > > > > > > that we expect userspace to
> > > > > > > > > > > > > > > > > > perform?  IOW, if two mdev types are 
> > > > > > > > > > > > > > > > > > migration compatible, it seems a
> > > > > > > > > > > > > > > > > > prerequisite to that is that they provide 
> > > > > > > > > > > > > > > > > > the same software interface,
> > > > > > > > > > > > > > > > > > which means they should be the same mdev 
> > > > > > > > > > > > > > > > > > type.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > In the hybrid cases of mdev->phys or 
> > > > > > > > > > > > > > > > > > phys->mdev, how does a  
> > > > > > > > > > > > > > > > > management  
> > > > > > > > > > > > > > > > > > tool begin to even guess what might be 
> > > > > > > > > > > > > > > > > > compatible?  Are we expecting
> > > > > > > > > > > > > > > > > > libvirt to probe ever device with this 
> > > > > > > > > > > > > > > > > > attribute in the system?  Is
> > > > > > > > > > > > > > > > > > there going to be a new class hierarchy 
> > > > > > > > > > > > > > > > > > created to enumerate all
> > > > > > > > > > > > > > > > > > possible migrate-able devices?
> > > > > > > > > > > > > > > > > >  
> > > > > > > > > > > > > > > > > yes, management tool needs to guess and test 
> > > > > > > > > > > > > > > > > migration compatible
> > > > > > > > > > > > > > > > > between two devices. But I think it's not the 
> > > > > > > > > > > > > > > > > problem only for
> > > > > > > > > > > > > > > > > mdev->phys or phys->mdev. even for 
> > > > > > > > > > > > > > > > > mdev->mdev, management tool needs
> > > > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > > > first assume that the two mdevs have the same 
> > > > > > > > > > > > > > > > > type of parent devices
> > > > > > > > > > > > > > > > > (e.g.their pciids are equal). otherwise, it's 
> > > > > > > > > > > > > > > > > still enumerating
> > > > > > > > > > > > > > > > > possibilities.
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > on the other hand, for two mdevs,
> > > > > > > > > > > > > > > > > mdev1 from pdev1, its mdev_type is 1/2 of 
> > > > > > > > > > > > > > > > > pdev1;
> > > > > > > > > > > > > > > > > mdev2 from pdev2, its mdev_type is 1/4 of 
> > > > > > > > > > > > > > > > > pdev2;
> > > > > > > > > > > > > > > > > if pdev2 is exactly 2 times of pdev1, why not 
> > > > > > > > > > > > > > > > > allow migration between
> > > > > > > > > > > > > > > > > mdev1 <-> mdev2.  
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > How could the manage tool figure out that 1/2 
> > > > > > > > > > > > > > > > of pdev1 is equivalent 
> > > > > > > > > > > > > > > > to 1/4 of pdev2? If we really want to allow 
> > > > > > > > > > > > > > > > such thing happen, the best
> > > > > > > > > > > > > > > > choice is to report the same mdev type on both 
> > > > > > > > > > > > > > > > pdev1 and pdev2.  
> > > > > > > > > > > > > > > I think that's exactly the value of this 
> > > > > > > > > > > > > > > migration_version interface.
> > > > > > > > > > > > > > > the management tool can take advantage of this 
> > > > > > > > > > > > > > > interface to know if two
> > > > > > > > > > > > > > > devices are migration compatible, no matter they 
> > > > > > > > > > > > > > > are mdevs, non-mdevs,
> > > > > > > > > > > > > > > or mix.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > as I know, (please correct me if not right), 
> > > > > > > > > > > > > > > current libvirt still
> > > > > > > > > > > > > > > requires manually generating mdev devices, and it 
> > > > > > > > > > > > > > > just duplicates src vm
> > > > > > > > > > > > > > > configuration to the target vm.
> > > > > > > > > > > > > > > for libvirt, currently it's always phys->phys and 
> > > > > > > > > > > > > > > mdev->mdev (and of the
> > > > > > > > > > > > > > > same mdev type).
> > > > > > > > > > > > > > > But it does not justify that hybrid cases should 
> > > > > > > > > > > > > > > not be allowed. otherwise,
> > > > > > > > > > > > > > > why do we need to introduce this 
> > > > > > > > > > > > > > > migration_version 

[PULL 03/15] hw/riscv: sifive_u: Remove the riscv_ prefix of the soc* functions

[Alistair Francis]

From: Bin Meng 

To keep consistency with the machine* functions, remove the riscv_
prefix of the soc* functions.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Alistair Francis 
Message-id: 1590072147-13035-1-git-send-email-bmeng...@gmail.com
Message-Id: <1590072147-13035-1-git-send-email-bmeng...@gmail.com>
Signed-off-by: Alistair Francis 
---
 hw/riscv/sifive_u.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 4299bdf480..f9fef2be91 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -481,7 +481,7 @@ static void sifive_u_machine_init_register_types(void)
 
 type_init(sifive_u_machine_init_register_types)
 
-static void riscv_sifive_u_soc_init(Object *obj)
+static void sifive_u_soc_instance_init(Object *obj)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
 SiFiveUSoCState *s = RISCV_U_SOC(obj);
@@ -520,7 +520,7 @@ static void riscv_sifive_u_soc_init(Object *obj)
   TYPE_CADENCE_GEM);
 }
 
-static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
+static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
 SiFiveUSoCState *s = RISCV_U_SOC(dev);
@@ -635,32 +635,32 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, 
Error **errp)
 memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
 }
 
-static Property riscv_sifive_u_soc_props[] = {
+static Property sifive_u_soc_props[] = {
 DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
 DEFINE_PROP_END_OF_LIST()
 };
 
-static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
+static void sifive_u_soc_class_init(ObjectClass *oc, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(oc);
 
-device_class_set_props(dc, riscv_sifive_u_soc_props);
-dc->realize = riscv_sifive_u_soc_realize;
+device_class_set_props(dc, sifive_u_soc_props);
+dc->realize = sifive_u_soc_realize;
 /* Reason: Uses serial_hds in realize function, thus can't be used twice */
 dc->user_creatable = false;
 }
 
-static const TypeInfo riscv_sifive_u_soc_type_info = {
+static const TypeInfo sifive_u_soc_type_info = {
 .name = TYPE_RISCV_U_SOC,
 .parent = TYPE_DEVICE,
 .instance_size = sizeof(SiFiveUSoCState),
-.instance_init = riscv_sifive_u_soc_init,
-.class_init = riscv_sifive_u_soc_class_init,
+.instance_init = sifive_u_soc_instance_init,
+.class_init = sifive_u_soc_class_init,
 };
 
-static void riscv_sifive_u_soc_register_types(void)
+static void sifive_u_soc_register_types(void)
 {
-type_register_static(_sifive_u_soc_type_info);
+type_register_static(_u_soc_type_info);
 }
 
-type_init(riscv_sifive_u_soc_register_types)
+type_init(sifive_u_soc_register_types)
-- 
2.26.2

[PULL 09/15] riscv: sifive_e: Manually define the machine

[Alistair Francis]

Signed-off-by: Alistair Francis 
Reviewed-by: Palmer Dabbelt 
---
 include/hw/riscv/sifive_e.h |  4 
 hw/riscv/sifive_e.c | 41 +++--
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
index 25ce7aa9d5..414992119e 100644
--- a/include/hw/riscv/sifive_e.h
+++ b/include/hw/riscv/sifive_e.h
@@ -47,6 +47,10 @@ typedef struct SiFiveEState {
 SiFiveESoCState soc;
 } SiFiveEState;
 
+#define TYPE_RISCV_E_MACHINE MACHINE_TYPE_NAME("sifive_e")
+#define RISCV_E_MACHINE(obj) \
+OBJECT_CHECK(SiFiveEState, (obj), TYPE_RISCV_E_MACHINE)
+
 enum {
 SIFIVE_E_DEBUG,
 SIFIVE_E_MROM,
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index b53109521e..472a98970b 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -79,7 +79,7 @@ static void riscv_sifive_e_init(MachineState *machine)
 {
 const struct MemmapEntry *memmap = sifive_e_memmap;
 
-SiFiveEState *s = g_new0(SiFiveEState, 1);
+SiFiveEState *s = RISCV_E_MACHINE(machine);
 MemoryRegion *sys_mem = get_system_memory();
 MemoryRegion *main_mem = g_new(MemoryRegion, 1);
 int i;
@@ -115,6 +115,35 @@ static void riscv_sifive_e_init(MachineState *machine)
 }
 }
 
+static void sifive_e_machine_instance_init(Object *obj)
+{
+}
+
+static void sifive_e_machine_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+
+mc->desc = "RISC-V Board compatible with SiFive E SDK";
+mc->init = riscv_sifive_e_init;
+mc->max_cpus = 1;
+mc->default_cpu_type = SIFIVE_E_CPU;
+}
+
+static const TypeInfo sifive_e_machine_typeinfo = {
+.name   = MACHINE_TYPE_NAME("sifive_e"),
+.parent = TYPE_MACHINE,
+.class_init = sifive_e_machine_class_init,
+.instance_init = sifive_e_machine_instance_init,
+.instance_size = sizeof(SiFiveEState),
+};
+
+static void sifive_e_machine_init_register_types(void)
+{
+type_register_static(_e_machine_typeinfo);
+}
+
+type_init(sifive_e_machine_init_register_types)
+
 static void riscv_sifive_e_soc_init(Object *obj)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
@@ -214,16 +243,6 @@ static void riscv_sifive_e_soc_realize(DeviceState *dev, 
Error **errp)
 >xip_mem);
 }
 
-static void riscv_sifive_e_machine_init(MachineClass *mc)
-{
-mc->desc = "RISC-V Board compatible with SiFive E SDK";
-mc->init = riscv_sifive_e_init;
-mc->max_cpus = 1;
-mc->default_cpu_type = SIFIVE_E_CPU;
-}
-
-DEFINE_MACHINE("sifive_e", riscv_sifive_e_machine_init)
-
 static void riscv_sifive_e_soc_class_init(ObjectClass *oc, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(oc);
-- 
2.26.2

Re: another tst-arm-mte bug: qemu-system segfaults

[Richard Henderson]

On 6/3/20 6:50 AM, Szabolcs Nagy wrote:
> thanks my tests now get further but later i run into
> the previous assert failure:
> 
> target/arm/mte_helper.c:97:allocation_tag_mem: assertion failed: (tag_size <= 
> in_page)
> 
> i might be able to reduce it to a small reproducer
> this time. i assume that will help.

Dang, I had hoped that the one fix would cover both -- it's definitely in the
same area.  Yes, a small reproducer will help, but I will also try again with
your larger reproducer.


r~

[PULL 02/15] riscv: Change the default behavior if no -bios option is specified

[Alistair Francis]

From: Bin Meng 

Per QEMU deprecated doc, QEMU 4.1 introduced support for the -bios
option in QEMU for RISC-V for the virt machine and sifive_u machine.
The default behavior has been that QEMU does not automatically load
any firmware if no -bios option is included.

Now 2 releases passed, it's time to change the default behavior to
load the default OpenSBI firmware automatically. The firmware is
included with the QEMU release and no user interaction is required.
All a user needs to do is specify the kernel they want to boot with
the -kernel option.

Signed-off-by: Bin Meng 
Reviewed-by: Alistair Francis 
Message-id: 1588335545-649-1-git-send-email-bmeng...@gmail.com
Message-Id: <1588335545-649-1-git-send-email-bmeng...@gmail.com>
Signed-off-by: Alistair Francis 
---
 hw/riscv/boot.c | 31 ---
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index da5817d438..adb421b91b 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -41,34 +41,11 @@ void riscv_find_and_load_firmware(MachineState *machine,
 {
 char *firmware_filename = NULL;
 
-if (!machine->firmware) {
+if ((!machine->firmware) || (!strcmp(machine->firmware, "default"))) {
 /*
- * The user didn't specify -bios.
- * At the moment we default to loading nothing when this hapens.
- * In the future this defaul will change to loading the prebuilt
- * OpenSBI firmware. Let's warn the user and then continue.
-*/
-if (!qtest_enabled()) {
-warn_report("No -bios option specified. Not loading a firmware.");
-warn_report("This default will change in a future QEMU release. " \
-"Please use the -bios option to avoid breakages when "\
-"this happens.");
-warn_report("See QEMU's deprecation documentation for details.");
-}
-return;
-}
-
-if (!strcmp(machine->firmware, "default")) {
-/*
- * The user has specified "-bios default". That means we are going to
- * load the OpenSBI binary included in the QEMU source.
- *
- * We can't load the binary by default as it will break existing users
- * as users are already loading their own firmware.
- *
- * Let's try to get everyone to specify the -bios option at all times,
- * so then in the future we can make "-bios default" the default option
- * if no -bios option is set without breaking anything.
+ * The user didn't specify -bios, or has specified "-bios default".
+ * That means we are going to load the OpenSBI binary included in
+ * the QEMU source.
  */
 firmware_filename = riscv_find_firmware(default_machine_firmware);
 } else if (strcmp(machine->firmware, "none")) {
-- 
2.26.2

Re: [PATCH 2/5] linux-user: Add strace support for printing argument of syscalls used for extend attributes

[Laurent Vivier]

Le 02/06/2020 à 13:53, Filip Bozuta a écrit :
> From: Filip Bozuta 
> 
> This patch implements strace argument printing functionality for following 
> syscalls:
> 
> *getxattr, lgetxattr, fgetxattr - retrieve an extended attribute value
> 
> ssize_t getxattr(const char *path, const char *name, void *value, 
> size_t size)
> ssize_t lgetxattr(const char *path, const char *name, void *value, 
> size_t size)
> ssize_t fgetxattr(int fd, const char *name, void *value, size_t size)
> man page: https://www.man7.org/linux/man-pages/man2/getxattr.2.html
> 
> *listxattr, llistxattr, flistxattr - list extended attribute names
> 
> ssize_t listxattr(const char *path, char *list, size_t size)
> ssize_t llistxattr(const char *path, char *list, size_t size)
> ssize_t flistxattr(int fd, char *list, size_t size)
> man page: https://www.man7.org/linux/man-pages/man2/listxattr.2.html
> 
> Implementation notes:
> 
> All of the syscalls have strings as argument types and thus a separate
> printing function was stated in file "strace.list" for every one of them.
> All of these printing functions were defined in "strace.c" using existing
> printing functions for appropriate argument types:
>"print_strig()" - for (const char*) type
>"print_pointer()" - for (char*) and (void *) type
>"print_raw_param()" for (int) and (size_t) type
> Syscalls "getxattr()" and "lgetxattr()" have the same number and type of
> arguments and thus their print functions ("print_getxattr", 
> "print_lgetxattr")
> share a same definition. The same statement applies to syscalls 
> "listxattr()"
> and "llistxattr()".
> 
> Signed-off-by: Filip Bozuta 
> ---
>  linux-user/strace.c| 60 ++
>  linux-user/strace.list | 12 -
>  2 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index c578876d22..5cf419989c 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -1629,6 +1629,66 @@ print_fcntl(const struct syscallname *name,
>  #define print_fcntl64   print_fcntl
>  #endif
>  
> +#ifdef TARGET_NR_fgetxattr
> +static void
> +print_fgetxattr(const struct syscallname *name,
> +abi_long arg0, abi_long arg1, abi_long arg2,
> +abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +print_syscall_prologue(name);
> +print_raw_param("%d", arg0, 0);
> +print_string(arg1, 0);
> +print_pointer(arg2, 0);
> +print_raw_param("%u", arg3, 1);

size_t is generally "unsigned long", so you should use TARGET_FMT_lu

> +print_syscall_epilogue(name);
> +}
> +#endif
> +
> +#ifdef TARGET_NR_flistxattr
> +static void
> +print_flistxattr(const struct syscallname *name,
> +abi_long arg0, abi_long arg1, abi_long arg2,
> +abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +print_syscall_prologue(name);
> +print_raw_param("%d", arg0, 0);
> +print_pointer(arg1, 0);
> +print_raw_param("%u", arg2, 1);

TARGET_FMT_lu

> +print_syscall_epilogue(name);
> +}
> +#endif
> +
> +#if defined(TARGET_NR_getxattr) || defined(TARGET_NR_lgetxattr)
> +static void
> +print_getxattr(const struct syscallname *name,
> +abi_long arg0, abi_long arg1, abi_long arg2,
> +abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +print_syscall_prologue(name);
> +print_string(arg0, 0);
> +print_string(arg1, 0);
> +print_pointer(arg2, 0);
> +print_raw_param("%u", arg3, 1);

TARGET_FMT_lu

> +print_syscall_epilogue(name);
> +}
> +#define print_lgetxattr print_getxattr
> +#endif
> +
> +#if defined(TARGET_NR_listxattr) || defined(TARGET_NR_llistxattr)
> +static void
> +print_listxattr(const struct syscallname *name,
> +abi_long arg0, abi_long arg1, abi_long arg2,
> +abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +print_syscall_prologue(name);
> +print_string(arg0, 0);
> +print_pointer(arg1, 0);
> +print_raw_param("%u", arg2, 1);

TARGET_FMT_lu

> +print_syscall_epilogue(name);
> +}
> +#define print_llistxattr print_listxattr
> +#endif
> +
>  #ifdef TARGET_NR_futimesat
>  static void
>  print_futimesat(const struct syscallname *name,
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index fb9799e7e6..8d51c54bca 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -218,13 +218,13 @@
>  { TARGET_NR_fdatasync, "fdatasync" , "%s(%d)", NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_fgetxattr
> -{ TARGET_NR_fgetxattr, "fgetxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_fgetxattr, "fgetxattr" , NULL, print_fgetxattr, NULL },
>  #endif
>  #ifdef TARGET_NR_finit_module
>  { TARGET_NR_finit_module, "finit_module" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_flistxattr
> -{ TARGET_NR_flistxattr, "flistxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_flistxattr, "flistxattr" , NULL, print_flistxattr, NULL },
>  #endif
>  #ifdef 

[PULL 05/15] hw/riscv: spike: Remove deprecated ISA specific machines

[Alistair Francis]

The ISA specific Spike machines have been deprecated in QEMU since 4.1,
let's finally remove them.

Signed-off-by: Alistair Francis 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Bin Meng 
Reviewed-by: Thomas Huth 
---
 docs/system/deprecated.rst |  17 +--
 include/hw/riscv/spike.h   |   6 +-
 hw/riscv/spike.c   | 217 -
 3 files changed, 12 insertions(+), 228 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index f0061f94aa..50927bad74 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -379,13 +379,6 @@ This machine has been renamed ``fuloong2e``.
 These machine types are very old and likely can not be used for live migration
 from old QEMU versions anymore. A newer machine type should be used instead.
 
-``spike_v1.9.1`` and ``spike_v1.10`` (since 4.1)
-
-
-The version specific Spike machines have been deprecated in favour of the
-generic ``spike`` machine. If you need to specify an older version of the 
RISC-V
-spec you can use the ``-cpu rv64gcsu,priv_spec=v1.9.1`` command line argument.
-
 Device options
 --
 
@@ -493,6 +486,16 @@ The ``hub_id`` parameter of ``hostfwd_add`` / 
``hostfwd_remove`` (removed in 5.0
 The ``[hub_id name]`` parameter tuple of the 'hostfwd_add' and
 'hostfwd_remove' HMP commands has been replaced by ``netdev_id``.
 
+System emulator machines
+
+
+``spike_v1.9.1`` and ``spike_v1.10`` (removed in 5.1)
+'
+
+The version specific Spike machines have been removed in favour of the
+generic ``spike`` machine. If you need to specify an older version of the 
RISC-V
+spec you can use the ``-cpu rv64gcsu,priv_spec=v1.10.0`` command line argument.
+
 Related binaries
 
 
diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
index dc770421bc..1cd72b85d6 100644
--- a/include/hw/riscv/spike.h
+++ b/include/hw/riscv/spike.h
@@ -39,11 +39,9 @@ enum {
 };
 
 #if defined(TARGET_RISCV32)
-#define SPIKE_V1_09_1_CPU TYPE_RISCV_CPU_RV32GCSU_V1_09_1
-#define SPIKE_V1_10_0_CPU TYPE_RISCV_CPU_RV32GCSU_V1_10_0
+#define SPIKE_V1_10_0_CPU TYPE_RISCV_CPU_BASE32
 #elif defined(TARGET_RISCV64)
-#define SPIKE_V1_09_1_CPU TYPE_RISCV_CPU_RV64GCSU_V1_09_1
-#define SPIKE_V1_10_0_CPU TYPE_RISCV_CPU_RV64GCSU_V1_10_0
+#define SPIKE_V1_10_0_CPU TYPE_RISCV_CPU_BASE64
 #endif
 
 #endif
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index d0c4843712..7bbbdb5036 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -257,221 +257,6 @@ static void spike_board_init(MachineState *machine)
 false);
 }
 
-static void spike_v1_10_0_board_init(MachineState *machine)
-{
-const struct MemmapEntry *memmap = spike_memmap;
-
-SpikeState *s = g_new0(SpikeState, 1);
-MemoryRegion *system_memory = get_system_memory();
-MemoryRegion *main_mem = g_new(MemoryRegion, 1);
-MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
-int i;
-unsigned int smp_cpus = machine->smp.cpus;
-
-if (!qtest_enabled()) {
-info_report("The Spike v1.10.0 machine has been deprecated. "
-"Please use the generic spike machine and specify the ISA "
-"versions using -cpu.");
-}
-
-/* Initialize SOC */
-object_initialize_child(OBJECT(machine), "soc", >soc, sizeof(s->soc),
-TYPE_RISCV_HART_ARRAY, _abort, NULL);
-object_property_set_str(OBJECT(>soc), SPIKE_V1_10_0_CPU, "cpu-type",
-_abort);
-object_property_set_int(OBJECT(>soc), smp_cpus, "num-harts",
-_abort);
-object_property_set_bool(OBJECT(>soc), true, "realized",
-_abort);
-
-/* register system main memory (actual RAM) */
-memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
-   machine->ram_size, _fatal);
-memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
-main_mem);
-
-/* create device tree */
-create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
-
-/* boot rom */
-memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
-   memmap[SPIKE_MROM].size, _fatal);
-memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
-mask_rom);
-
-if (machine->kernel_filename) {
-riscv_load_kernel(machine->kernel_filename, htif_symbol_callback);
-}
-
-/* reset vector */
-uint32_t reset_vec[8] = {
-0x0297,  /* 1:  auipc  t0, %pcrel_hi(dtb) */
-0x02028593,  /* addi   a1, t0, %pcrel_lo(1b) */
-0xf1402573,  /* csrr   a0, mhartid  */
-#if defined(TARGET_RISCV32)
-0x0182a283,  /* lw t0, 24(t0) */
-#elif defined(TARGET_RISCV64)
-0x0182b283, 

[PULL 07/15] target/riscv: Drop support for ISA spec version 1.09.1

[Alistair Francis]

The RISC-V ISA spec version 1.09.1 has been deprecated in QEMU since
4.1. It's not commonly used so let's remove support for it.

Signed-off-by: Alistair Francis 
Reviewed-by: Bin Meng 
---
 docs/system/deprecated.rst|  20 +--
 target/riscv/cpu.h|   1 -
 target/riscv/cpu.c|   2 -
 target/riscv/cpu_helper.c |  82 ---
 target/riscv/csr.c| 138 --
 .../riscv/insn_trans/trans_privileged.inc.c   |  18 +--
 target/riscv/monitor.c|   5 -
 target/riscv/op_helper.c  |  17 +--
 8 files changed, 73 insertions(+), 210 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index bb14de9848..d177609cbc 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -301,16 +301,6 @@ The ``acl_show``, ``acl_reset``, ``acl_policy``, 
``acl_add``, and
 ``acl_remove`` commands are deprecated with no replacement. Authorization
 for VNC should be performed using the pluggable QAuthZ objects.
 
-Guest Emulator ISAs

-
-RISC-V ISA privledge specification version 1.09.1 (since 4.1)
-'
-
-The RISC-V ISA privledge specification version 1.09.1 has been deprecated.
-QEMU supports both the newer version 1.10.0 and the ratified version 1.11.0, 
these
-should be used instead of the 1.09.1 version.
-
 System emulator CPUS
 
 
@@ -471,6 +461,16 @@ The ``hub_id`` parameter of ``hostfwd_add`` / 
``hostfwd_remove`` (removed in 5.0
 The ``[hub_id name]`` parameter tuple of the 'hostfwd_add' and
 'hostfwd_remove' HMP commands has been replaced by ``netdev_id``.
 
+Guest Emulator ISAs
+---
+
+RISC-V ISA privledge specification version 1.09.1 (removed in 5.1)
+''
+
+The RISC-V ISA privledge specification version 1.09.1 has been removed.
+QEMU supports both the newer version 1.10.0 and the ratified version 1.11.0, 
these
+should be used instead of the 1.09.1 version.
+
 System emulator CPUS
 
 
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 76b98d7a33..c022539012 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -73,7 +73,6 @@ enum {
 RISCV_FEATURE_MISA
 };
 
-#define PRIV_VERSION_1_09_1 0x00010901
 #define PRIV_VERSION_1_10_0 0x00011000
 #define PRIV_VERSION_1_11_0 0x00011100
 
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 112f2e3a2f..eeb91f8513 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -368,8 +368,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 priv_version = PRIV_VERSION_1_11_0;
 } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
 priv_version = PRIV_VERSION_1_10_0;
-} else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.9.1")) {
-priv_version = PRIV_VERSION_1_09_1;
 } else {
 error_setg(errp,
"Unsupported privilege spec version '%s'",
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index bc80aa87cf..62fe1ecc8f 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -364,57 +364,36 @@ static int get_physical_address(CPURISCVState *env, 
hwaddr *physical,
 mxr = get_field(env->vsstatus, MSTATUS_MXR);
 }
 
-if (env->priv_ver >= PRIV_VERSION_1_10_0) {
-if (first_stage == true) {
-if (use_background) {
-base = (hwaddr)get_field(env->vsatp, SATP_PPN) << PGSHIFT;
-vm = get_field(env->vsatp, SATP_MODE);
-} else {
-base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
-vm = get_field(env->satp, SATP_MODE);
-}
-widened = 0;
+if (first_stage == true) {
+if (use_background) {
+base = (hwaddr)get_field(env->vsatp, SATP_PPN) << PGSHIFT;
+vm = get_field(env->vsatp, SATP_MODE);
 } else {
-base = (hwaddr)get_field(env->hgatp, HGATP_PPN) << PGSHIFT;
-vm = get_field(env->hgatp, HGATP_MODE);
-widened = 2;
-}
-sum = get_field(env->mstatus, MSTATUS_SUM);
-switch (vm) {
-case VM_1_10_SV32:
-  levels = 2; ptidxbits = 10; ptesize = 4; break;
-case VM_1_10_SV39:
-  levels = 3; ptidxbits = 9; ptesize = 8; break;
-case VM_1_10_SV48:
-  levels = 4; ptidxbits = 9; ptesize = 8; break;
-case VM_1_10_SV57:
-  levels = 5; ptidxbits = 9; ptesize = 8; break;
-case VM_1_10_MBARE:
-*physical = addr;
-*prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
-return TRANSLATE_SUCCESS;
-default:
-  g_assert_not_reached();
+base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
+vm 

[PULL 00/15] riscv-to-apply queue

[Alistair Francis]

The following changes since commit 5cc7a54c2e91d82cb6a52e4921325c511fd90712:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20200602' into 
staging (2020-06-02 18:16:38 +0100)

are available in the Git repository at:

  g...@github.com:alistair23/qemu.git tags/pull-riscv-to-apply-20200603

for you to fetch changes up to fe0fe4735e798578097758781166cc221319b93d:

  riscv: Initial commit of OpenTitan machine (2020-06-03 09:11:51 -0700)


This is a collection of RISC-V patches for 5.1.

This incldues removing deprecated features and part of the OpenTitan
support series.


Alistair Francis (11):
  hw/riscv: spike: Remove deprecated ISA specific machines
  target/riscv: Remove the deprecated CPUs
  target/riscv: Drop support for ISA spec version 1.09.1
  docs: deprecated: Update the -bios documentation
  riscv: sifive_e: Manually define the machine
  riscv/boot: Add a missing header include
  target/riscv: Don't overwrite the reset vector
  target/riscv: Disable the MMU correctly
  target/riscv: Don't set PMP feature in the cpu init
  target/riscv: Add the lowRISC Ibex CPU
  riscv: Initial commit of OpenTitan machine

Bin Meng (4):
  riscv: Suppress the error report for QEMU testing with 
riscv_find_firmware()
  riscv: Change the default behavior if no -bios option is specified
  hw/riscv: sifive_u: Remove the riscv_ prefix of the soc* functions
  hw/riscv: virt: Remove the riscv_ prefix of the machine* functions

 docs/system/deprecated.rst |  98 +--
 default-configs/riscv32-softmmu.mak|   1 +
 default-configs/riscv64-softmmu.mak|  11 +-
 include/hw/riscv/boot.h|   1 +
 include/hw/riscv/opentitan.h   |  68 
 include/hw/riscv/sifive_e.h|   4 +
 include/hw/riscv/spike.h   |   6 +-
 target/riscv/cpu.h |   9 +-
 hw/riscv/boot.c|  45 ++---
 hw/riscv/opentitan.c   | 184 +
 hw/riscv/sifive_e.c|  41 +++--
 hw/riscv/sifive_u.c|  24 +--
 hw/riscv/spike.c   | 217 -
 hw/riscv/virt.c|  20 +--
 target/riscv/cpu.c |  45 ++---
 target/riscv/cpu_helper.c  |  82 --
 target/riscv/csr.c | 138 +++-
 target/riscv/insn_trans/trans_privileged.inc.c |  18 +-
 target/riscv/monitor.c |   5 -
 target/riscv/op_helper.c   |  17 +-
 tests/qtest/machine-none-test.c|   4 +-
 MAINTAINERS|   9 +
 hw/riscv/Kconfig   |   5 +
 hw/riscv/Makefile.objs |   1 +
 24 files changed, 480 insertions(+), 573 deletions(-)
 create mode 100644 include/hw/riscv/opentitan.h
 create mode 100644 hw/riscv/opentitan.c

RE: [PATCH v3] xen: fix build without pci passthrough

[Paul Durrant]

> -Original Message-
> From: Anthony PERARD 
> Sent: 03 June 2020 17:05
> To: qemu-devel@nongnu.org
> Cc: Anthony PERARD ; Roger Pau Monne 
> ; Michael S.
> Tsirkin ; Marcel Apfelbaum ; 
> Paolo Bonzini
> ; Richard Henderson ; Eduardo Habkost 
> ;
> Stefano Stabellini ; Paul Durrant ; xen-
> de...@lists.xenproject.org
> Subject: [PATCH v3] xen: fix build without pci passthrough
> 
> From: Roger Pau Monne 
> 
> Xen PCI passthrough support may not be available and thus the global
> variable "has_igd_gfx_passthru" might be compiled out. Common code
> should not access it in that case.
> 
> Unfortunately, we can't use CONFIG_XEN_PCI_PASSTHROUGH directly in
> xen-common.c so this patch instead move access to the
> has_igd_gfx_passthru variable via function and those functions are
> also implemented as stubs. The stubs will be used when QEMU is built
> without passthrough support.
> 
> Now, when one will want to enable igd-passthru via the -machine
> property, they will get an error message if QEMU is built without
> passthrough support.
> 
> Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator 
> property')
> Reported-by: Roger Pau Monné 
> Signed-off-by: Anthony PERARD 

Acked-by: Paul Durrant 

> ---
> 
> Notes:
> Changes in v3:
> - reworked to use stubs instead of #ifdef CONFIG_XEN_PCI_PASSTHROUGH
>   CONFIG_XEN_PCI_PASSTHROUGH isn't available in xen-common.c
> 
>   moving CONFIG_XEN_PCI_PASSTHROUGH to be in config_host_mak isn't
>   really possible, or at least I didn't managed to make that work.
> 
>  hw/i386/pc_piix.c   |  2 +-
>  hw/xen/xen-common.c |  4 ++--
>  hw/xen/xen_pt.c | 12 +++-
>  hw/xen/xen_pt.h |  6 --
>  stubs/Makefile.objs |  1 +
>  stubs/xen-pt.c  | 22 ++
>  6 files changed, 41 insertions(+), 6 deletions(-)
>  create mode 100644 stubs/xen-pt.c
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f66e1d73ce0b..347fb8c6c807 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -375,7 +375,7 @@ static void pc_init_isa(MachineState *machine)
>  #ifdef CONFIG_XEN
>  static void pc_xen_hvm_init_pci(MachineState *machine)
>  {
> -const char *pci_type = has_igd_gfx_passthru ?
> +const char *pci_type = xen_igd_gfx_pt_enabled() ?
>  TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : 
> TYPE_I440FX_PCI_DEVICE;
> 
>  pc_init1(machine,
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 70564cc952d5..dd2c22cc4c0b 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -129,12 +129,12 @@ static void xen_change_state_handler(void *opaque, int 
> running,
> 
>  static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>  {
> -return has_igd_gfx_passthru;
> +return xen_igd_gfx_pt_enabled();
>  }
> 
>  static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>  {
> -has_igd_gfx_passthru = value;
> +xen_igd_gfx_pt_set(value, errp);
>  }
> 
>  static void xen_setup_post(MachineState *ms, AccelState *accel)
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 81d5ad8da7f0..ab84443d5ec8 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -65,7 +65,17 @@
>  #include "qemu/range.h"
>  #include "exec/address-spaces.h"
> 
> -bool has_igd_gfx_passthru;
> +static bool has_igd_gfx_passthru;
> +
> +bool xen_igd_gfx_pt_enabled(void)
> +{
> +return has_igd_gfx_passthru;
> +}
> +
> +void xen_igd_gfx_pt_set(bool value, Error **errp)
> +{
> +has_igd_gfx_passthru = value;
> +}
> 
>  #define XEN_PT_NR_IRQS (256)
>  static uint8_t xen_pt_mapped_machine_irq[XEN_PT_NR_IRQS] = {0};
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 179775db7b22..6e9cec95f3b7 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -5,6 +5,9 @@
>  #include "hw/pci/pci.h"
>  #include "xen-host-pci-device.h"
> 
> +bool xen_igd_gfx_pt_enabled(void);
> +void xen_igd_gfx_pt_set(bool value, Error **errp);
> +
>  void xen_pt_log(const PCIDevice *d, const char *f, ...) GCC_FMT_ATTR(2, 3);
> 
>  #define XEN_PT_ERR(d, _f, _a...) xen_pt_log(d, "%s: Error: "_f, __func__, 
> ##_a)
> @@ -322,10 +325,9 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice 
> *dev,
>  unsigned int domain,
>  unsigned int bus, unsigned int 
> slot,
>  unsigned int function);
> -extern bool has_igd_gfx_passthru;
>  static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
>  {
> -return (has_igd_gfx_passthru
> +return (xen_igd_gfx_pt_enabled()
>  && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
>  }
>  int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 6a9e3135e8f9..564527e00997 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -40,6 +40,7 @@ stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += 

Re: [PATCH 3/3] target/unicore32: Prefer qemu_semihosting_log_out() over curses

[Richard Henderson]

On 6/3/20 5:37 AM, Philippe Mathieu-Daudé wrote:
> Use the common API for semihosting logging.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  default-configs/unicore32-softmmu.mak |  1 +
>  target/unicore32/helper.c | 57 +++
>  2 files changed, 6 insertions(+), 52 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH 2/3] target/unicore32: Replace DPRINTF() by qemu_log_mask(GUEST_ERROR)

[Richard Henderson]

On 6/3/20 5:37 AM, Philippe Mathieu-Daudé wrote:
> Replace disabled DPRINTF() by qemu_log_mask(GUEST_ERROR).
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/unicore32/helper.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH 1/3] target/unicore32: Remove unused headers

[Richard Henderson]

On 6/3/20 5:37 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/unicore32/helper.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary

[Richard Henderson]

On 6/3/20 4:24 AM, Alex Bennée wrote:
> There is no particular reason why you can't have a watchpoint in TCG
> that covers a large chunk of the address space. We could be clever
> about it but these cases are pretty rare and we can assume the user
> will expect a little performance degradation.
> 
> NB: In my testing gdb will silently squash a watchpoint like:
> 
>   watch (char[0x7f]) *0x0
> 
> to a 4 byte watchpoint. Practically it will limit the maximum size
> based on max-value-size. However given enough of a tweak the sky is
> the limit.
> 
> Reported-by: Alexander Bulekov 
> Signed-off-by: Alex Bennée 
> 
> ---
> v2
>   - use cleaner in_page = -(addr | TARGET_PAGE_MASK) logic per rth
> ---
>  exec.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 

Queued to tcg-next.


r~

Re: [PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[Richard Henderson]

On 6/2/20 11:43 PM, Philippe Mathieu-Daudé wrote:
> 
> Hi Catherine,
> 
> On 6/3/20 7:23 AM, agrecascino...@gmail.com wrote:
>> From: "Catherine A. Frederick" 
>>
>> Signed-off-by: "Catherine A. Frederick" 
>> ---
>>  tcg/ppc/tcg-target.inc.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
>> index ee1f9227c1..a5450a5e67 100644
>> --- a/tcg/ppc/tcg-target.inc.c
>> +++ b/tcg/ppc/tcg-target.inc.c
>> @@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, 
>> TCGReg dst, TCGReg src)
>>  
>>  static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, 
>> int c)
>>  {
>> +c = ((unsigned)c > 32) ? 32 : c;
>>  tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
>>  }
>>  
>>  static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, 
>> int c)
>>  {
>> +c = ((unsigned)c > 64) ? 64 : c;
>>  tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
>>  }
>>  
>>  static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, 
>> int c)
>>  {
>> +c = ((unsigned)c > 32) ? 32 : c;
>>  tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
>>  }
>>  
>>  static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, 
>> int c)
>>  {
>> +c = ((unsigned)c > 64) ? 64 : c;
>>  tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
>>  }
> 
> I agree there is a bug, but I am not sure we should silently cap the
> value this way. I'd rather see the caller provide a value in range, and
> maybe the callee use 'tcg_debug_assert(c <= RANGE);' to catch future new
> caller added missing the range check.

We have done this before: see 1fd95946657.

In tcg/README, we note that out-of-range shifts produce undefined results, but
should not trap with illegal instruction.

I would like to know more about where these out-of-range shifts are being
generated, but I do know that there are innocent ways by which this can happen.

For instance, one way in which we can translate a guest in which out-of-range
shifts produce zero is

  x = (shift < 32 ? y << shift : 0)

using INDEX_op_movcond_i32 for the ?: operator.  Which means that
we use the original (out-of-range) shift and subsequently discard the undefined
result.

Catherine, I think it would be more appropriate to mask C rather than bound it
to another out-of-range value: c &= 31 or c &= 64, with a comment about
avoiding an illegal instruction, just as in the tcg/sparc patch I reference 
above.


r~

Re: [PATCH] util/oslib-posix : qemu_init_exec_dir implementation for MacOS

[Justin Hibbits]

On Wed, 3 Jun 2020 16:36:27 +0200
Philippe Mathieu-Daudé  wrote:

> On 6/3/20 4:09 PM, Justin Hibbits wrote:
> > On Wed, 3 Jun 2020 08:08:42 +0200
> > Philippe Mathieu-Daudé  wrote:
> >   
> >> Cc'ing more developers.
> >>
> >> On 5/26/20 10:40 PM, David CARLIER wrote:  
> >>> From b24a6702beb2a4e2a9c1c03b69c6d1dd07d4cf08 Mon Sep 17 00:00:00
> >>> 2001 From: David Carlier 
> >>> Date: Tue, 26 May 2020 21:35:27 +0100
> >>> Subject: [PATCH] util/oslib: current process full path resolution
> >>> on MacOS
> >>>
> >>> Using existing libproc to fill the path.
> >>>
> >>> Signed-off-by: David Carlier 
> >>> ---
> >>>  util/oslib-posix.c | 13 +
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> >>> index 062236a1ab..96f0405ee6 100644
> >>> --- a/util/oslib-posix.c
> >>> +++ b/util/oslib-posix.c
> >>> @@ -55,6 +55,10 @@
> >>>  #include 
> >>>  #endif
> >>>
> >>> +#ifdef __APPLE__
> >>> +#include 
> >>> +#endif
> >>> +
> >>>  #include "qemu/mmap-alloc.h"
> >>>
> >>>  #ifdef CONFIG_DEBUG_STACK_USAGE
> >>> @@ -366,6 +370,15 @@ void qemu_init_exec_dir(const char *argv0)
> >>>  p = buf;
> >>>  }
> >>>  }
> >>> +#elif defined(__APPLE__)
> >>> +{
> >>> +uint32_t len;
> >>> +len = proc_pidpath(getpid(), buf, sizeof(buf) - 1);
> >>> +if (len > 0) {
> >>> +buf[len] = 0;
> >>> +p = buf;
> >>> +}
> >>> +}
> >>>  #endif
> >>>  /* If we don't have any way of figuring out the actual
> >>> executable location then try argv[0].  */
> >>> 
> >>  
> > 
> > Apologies, I don't have context for this.  Why was I CC'd on this?  
> 
> I did after finding this patch of yours:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg639033.html

Ah, okay, thanks.  I haven't contributed much to qemu, so was curious.

- Justin

Re: [RFC PATCH 2/2] linux-user/mmap: Fix Clang 'type-limit-compare' warning

[Eric Blake]

On 5/3/20 6:32 AM, Philippe Mathieu-Daudé wrote:

When building with Clang 10 on Fedora 32, we get:

 CC  linux-user/mmap.o
   linux-user/mmap.c:720:49: error: result of comparison 'unsigned long' > 
18446744073709551615 is always false [-Werror,-Wtautological-type-limit-compare]
   if ((unsigned long)host_addr + new_size > (abi_ulong)-1) {
   ~~~ ^ ~

Fix by restricting the check for when target sizeof(abi_ulong) is
smaller than target sizeof(unsigned long).

Signed-off-by: Philippe Mathieu-Daudé 
---
  linux-user/mmap.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e378033797..b14652d894 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -714,6 +714,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong 
old_size,
  errno = ENOMEM;
  host_addr = MAP_FAILED;
  }
+#if TARGET_ABI_BITS < TARGET_LONG_BITS
  /* Check if address fits target address space */
  if ((unsigned long)host_addr + new_size > (abi_ulong)-1) {


Instead of using #if, the following suffices to shut up clang:

diff --git c/linux-user/mmap.c w/linux-user/mmap.c
index e37803379747..8d9ba201625d 100644
--- c/linux-user/mmap.c
+++ w/linux-user/mmap.c
@@ -715,7 +715,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong 
old_size,

 host_addr = MAP_FAILED;
 }
 /* Check if address fits target address space */
-if ((unsigned long)host_addr + new_size > (abi_ulong)-1) {
+if ((unsigned long)host_addr > (abi_ulong)-1 - new_size) {
 /* Revert mremap() changes */
 host_addr = mremap(g2h(old_addr), new_size, old_size, flags);
 errno = ENOMEM;


That is, it is no longer a tautological type compare if you commute the 
operations so that neither side is a compile-time constant.


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

Re: [PATCH v5 07/11] hw/char: Initial commit of Ibex UART

[Alistair Francis]

On Wed, Jun 3, 2020 at 3:33 AM LIU Zhiwei  wrote:
>
> On 2020/6/3 1:54, Alistair Francis wrote:
> > On Tue, Jun 2, 2020 at 5:28 AM LIU Zhiwei  wrote:
> >> Hi Alistair,
> >>
> >> There are still some questions I don't understand.
> >>
> >> 1. Is the baud rate  or fifo a necessary feature to simulate?
> >> As you can see, qemu_chr_fe_write will send the byte as soon as possible.
> >> When you want to transmit a byte through WDATA,  you can call
> >> qemu_chr_fe_write directly.
> > So qemu_chr_fe_write() will send the data straight away. This doesn't
> > match what teh hardware does though. So by modelling a FIFO and a
> > delay in sending we can better match the hardware.
> I see many UARTs have similar features. Does the software really care about
> these features? Usually I just want to print something to the terminal
> through UART.

In this case Tock (which is the OS used for OpenTitan) does car about
these features as it relies on interrupts generated by the HW to
complete the serial send task. It also just makes the QEMU model more
accurate.

> Most simulation in QEMU is for running software, not exactly the details
> of hardware.
> For example, we will not simulate the 16x oversamples in this UART.

Agreed. Lots of UARTs don't bother modelling the delay from the
hardware as generally it doesn't matter. In this case it does make a
difference for the software and it makes the QEMU model more accurate,
which is always a good thing.

>
> There is no error here. Personally I  think it is necessary to simulate
> the FIFO and baud rate,
> maybe for supporting some backends.

So baud rate doesn't need to be modelled as we aren't actually sending
UART data, just pretending and then printing it.

>
> Can someone give a reasonable answer for this question?

Which question?

> >> 2.  The baud rate calculation method is not strictly right.
> >> I think when a byte write to FIFO,  char_tx_time * 8 is the correct time
> >> to send the byte instead of
> >> char_tx_time * 4.
> > Do you mind explaining why 8 is correct instead of 4?
> Usually write a byte to WDATA will trigger a uart_write_tx_fifo.
> Translate a bit will take
> char_tx_time. So it will take char_tx_time * 8 to transmit a byte.

I see your point. I just used the 4 as that is what the Cadence one
does. I don't think it matters too much as it's just the delay for a
timer (that isn't used as an accurate timer).

> >> 3.  Why add a watch here?
> > This is based on the Cadence UART implementation in QEMU (which does
> > the same thing). This will trigger a callback when we can write more
> > data or when the backend has hung up.
> Many other serials do the same thing, like virtio-console and serial. So
> it may be a common
> interface here. I will try to understand it(Not yet).

Yep, it's just a more complete model of that the HW does.

Alistair

[PATCH v3] xen: fix build without pci passthrough

[Anthony PERARD]

From: Roger Pau Monne 

Xen PCI passthrough support may not be available and thus the global
variable "has_igd_gfx_passthru" might be compiled out. Common code
should not access it in that case.

Unfortunately, we can't use CONFIG_XEN_PCI_PASSTHROUGH directly in
xen-common.c so this patch instead move access to the
has_igd_gfx_passthru variable via function and those functions are
also implemented as stubs. The stubs will be used when QEMU is built
without passthrough support.

Now, when one will want to enable igd-passthru via the -machine
property, they will get an error message if QEMU is built without
passthrough support.

Fixes: 46472d82322d0 ('xen: convert "-machine igd-passthru" to an accelerator 
property')
Reported-by: Roger Pau Monné 
Signed-off-by: Anthony PERARD 
---

Notes:
Changes in v3:
- reworked to use stubs instead of #ifdef CONFIG_XEN_PCI_PASSTHROUGH
  CONFIG_XEN_PCI_PASSTHROUGH isn't available in xen-common.c

  moving CONFIG_XEN_PCI_PASSTHROUGH to be in config_host_mak isn't
  really possible, or at least I didn't managed to make that work.

 hw/i386/pc_piix.c   |  2 +-
 hw/xen/xen-common.c |  4 ++--
 hw/xen/xen_pt.c | 12 +++-
 hw/xen/xen_pt.h |  6 --
 stubs/Makefile.objs |  1 +
 stubs/xen-pt.c  | 22 ++
 6 files changed, 41 insertions(+), 6 deletions(-)
 create mode 100644 stubs/xen-pt.c

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f66e1d73ce0b..347fb8c6c807 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -375,7 +375,7 @@ static void pc_init_isa(MachineState *machine)
 #ifdef CONFIG_XEN
 static void pc_xen_hvm_init_pci(MachineState *machine)
 {
-const char *pci_type = has_igd_gfx_passthru ?
+const char *pci_type = xen_igd_gfx_pt_enabled() ?
 TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE : 
TYPE_I440FX_PCI_DEVICE;
 
 pc_init1(machine,
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 70564cc952d5..dd2c22cc4c0b 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -129,12 +129,12 @@ static void xen_change_state_handler(void *opaque, int 
running,
 
 static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
 {
-return has_igd_gfx_passthru;
+return xen_igd_gfx_pt_enabled();
 }
 
 static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
 {
-has_igd_gfx_passthru = value;
+xen_igd_gfx_pt_set(value, errp);
 }
 
 static void xen_setup_post(MachineState *ms, AccelState *accel)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 81d5ad8da7f0..ab84443d5ec8 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -65,7 +65,17 @@
 #include "qemu/range.h"
 #include "exec/address-spaces.h"
 
-bool has_igd_gfx_passthru;
+static bool has_igd_gfx_passthru;
+
+bool xen_igd_gfx_pt_enabled(void)
+{
+return has_igd_gfx_passthru;
+}
+
+void xen_igd_gfx_pt_set(bool value, Error **errp)
+{
+has_igd_gfx_passthru = value;
+}
 
 #define XEN_PT_NR_IRQS (256)
 static uint8_t xen_pt_mapped_machine_irq[XEN_PT_NR_IRQS] = {0};
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 179775db7b22..6e9cec95f3b7 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -5,6 +5,9 @@
 #include "hw/pci/pci.h"
 #include "xen-host-pci-device.h"
 
+bool xen_igd_gfx_pt_enabled(void);
+void xen_igd_gfx_pt_set(bool value, Error **errp);
+
 void xen_pt_log(const PCIDevice *d, const char *f, ...) GCC_FMT_ATTR(2, 3);
 
 #define XEN_PT_ERR(d, _f, _a...) xen_pt_log(d, "%s: Error: "_f, __func__, ##_a)
@@ -322,10 +325,9 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice *dev,
 unsigned int domain,
 unsigned int bus, unsigned int 
slot,
 unsigned int function);
-extern bool has_igd_gfx_passthru;
 static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
 {
-return (has_igd_gfx_passthru
+return (xen_igd_gfx_pt_enabled()
 && ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
 }
 int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 6a9e3135e8f9..564527e00997 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -40,6 +40,7 @@ stub-obj-y += target-get-monitor-def.o
 stub-obj-y += vmgenid.o
 stub-obj-y += xen-common.o
 stub-obj-y += xen-hvm.o
+stub-obj-y += xen-pt.o
 stub-obj-y += pci-host-piix.o
 stub-obj-y += ram-block.o
 stub-obj-y += ramfb.o
diff --git a/stubs/xen-pt.c b/stubs/xen-pt.c
new file mode 100644
index ..2d8cac8d54b9
--- /dev/null
+++ b/stubs/xen-pt.c
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2020   Citrix Systems UK Ltd.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/xen/xen_pt.h"
+#include "qapi/error.h"
+
+bool xen_igd_gfx_pt_enabled(void)
+{
+return false;
+}
+
+void 

Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding functions

[Babu Moger]

On 6/3/20 10:45 AM, Eduardo Habkost wrote:
> On Wed, Jun 03, 2020 at 10:38:46AM -0500, Babu Moger wrote:
>>
>>
>> On 6/3/20 10:31 AM, Eduardo Habkost wrote:
>>> On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote:


> -Original Message-
> From: Eduardo Habkost 
> Sent: Tuesday, June 2, 2020 6:01 PM
> To: Moger, Babu 
> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> functions
>
> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote:
>>
>>
>>> -Original Message-
>>> From: Eduardo Habkost 
>>> Sent: Tuesday, June 2, 2020 12:19 PM
>>> To: Moger, Babu 
>>> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
>>> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
>>> functions
>>>
>>> Hi,
>>>
>>> It looks like this series breaks -device and CPU hotplug:
>>>
>>> On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote:
 These functions add support for building EPYC mode topology given the
> smp
 details like numa nodes, cores, threads and sockets.

 The new apic id decoding is mostly similar to current apic id decoding
 except that it adds a new field node_id when numa configured. Removes
> all
 the hardcoded values. Subsequent patches will use these functions to 
 build
 the topology.

 Following functions are added.
 apicid_llc_width_epyc
 apicid_llc_offset_epyc
 apicid_pkg_offset_epyc
 apicid_from_topo_ids_epyc
 x86_topo_ids_from_idx_epyc
 x86_topo_ids_from_apicid_epyc
 x86_apicid_from_cpu_idx_epyc

 The topology details are available in Processor Programming Reference
> (PPR)
 for AMD Family 17h Model 01h, Revision B1 Processors. The revision
> guides
>>> are
 available from the bugzilla Link below.
 Link:
>>>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
>>>
> kernel.org%2Fshow_bug.cgi%3Fid%3D206537data=02%7C01%7Cbabu.m
>>>
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
>>>
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739sdata=wE0
>>> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3Dreserved=0

 Signed-off-by: Babu Moger 
 Acked-by: Igor Mammedov 
 Acked-by: Michael S. Tsirkin 
 ---
>>> [...]
  typedef struct X86CPUTopoIDs {
  unsigned pkg_id;
 +unsigned node_id;
>>>
>>> You have added a new field here.
>>>
  unsigned die_id;
  unsigned core_id;
  unsigned smt_id;
>>> [...]
 +static inline apic_id_t
 +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
 +  const X86CPUTopoIDs *topo_ids)
 +{
 +return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
 +   (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
>>>
>>> You are using the new field here.
>>>
 +   (topo_ids->die_id  << apicid_die_offset(topo_info)) |
 +   (topo_ids->core_id << apicid_core_offset(topo_info)) |
 +   topo_ids->smt_id;
 +}
>>>
>>> But you are not initializing node_id in one caller of 
>>> apicid_from_topo_ids():
>>>
>>> static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>> DeviceState *dev, Error **errp)
>>> {
>>> [...]
>>> X86CPUTopoIDs topo_ids;
>>> [...]
>>> if (cpu->apic_id == UNASSIGNED_APIC_ID) {
>>> [...]
>>> topo_ids.pkg_id = cpu->socket_id;
>>> topo_ids.die_id = cpu->die_id;
>>> topo_ids.core_id = cpu->core_id;
>>> topo_ids.smt_id = cpu->thread_id;
>>> cpu->apic_id = x86ms->apicid_from_topo_ids(_info, 
>>> _ids);
>>> }
>>> [...]
>>> }
>>>
>>> Result: -device is broken when using -cpu EPYC:
>>>
>>>   $ qemu-system-x86_64 -machine q35,accel=kvm -smp
>>> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64-
>>> cpu,core-id=0,socket-id=1,thread-id=0
>
> [1]
>
>>>   qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-
> id=1,thread-
>>> id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC 
>>> ID
> 21855,
>>> valid index range 0:1
>>>
>>> This happens because APIC ID is calculated using uninitialized
>>> memory.
>> This patch should initialize the 

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Peter Maydell]

On Tue, 2 Jun 2020 at 16:54, Alex Bennée  wrote:
>
> This is a plugin intended to help with profiling access to various
> bits of system hardware. It only really makes sense for system
> emulation.
>
> It takes advantage of the recently exposed helper API that allows us
> to see the device name (memory region name) associated with a device.

This feels like we've let the plugin API get slightly more
access to QEMU's internals than is ideal. Whether an area
of memory happens to be an IO memory region or a memory-backed
one (or whether a device is implemented with one region or
three, or what names we happened to assign them) is kind of
a QEMU internal implementation detail.

thanks
-- PMM

Re: [PATCH v1 09/12] tests/docker: Added docker build support for TSan.

[Robert Foley]

On Tue, 2 Jun 2020 at 16:21, Alex Bennée  wrote:
>
>
> Robert Foley  writes:

> >
> >  configure_qemu()
> >  {
> > +if test -z "$TSAN"; then
> > +requires clang tsan
> > +echo "Including TSan Support"
> > +tsan_log_dir="/tmp/qemu-test/build/tsan"
> > +mkdir -p $tsan_log_dir > /dev/null || true
> > +EXTRA_CONFIGURE_OPTS="${EXTRA_CONFIGURE_OPTS} --enable-tsan \
> > + --cc=clang-10 --cxx=clang++-10 \
> > + --disable-werror --extra-cflags=-O0"
> > +# detect deadlocks is false currently simply because
> > +# TSan crashes immediately with deadlock detecter enabled.
> > +# We have maxed out the history size to get the best chance of 
> > finding
> > +# warnings during testing.
> > +# Note, to get tsan to fail on warning, use exitcode=66 below.
> > +
> > tsan_opts="suppressions=/tmp/qemu-test/src/tests/tsan/suppressions.tsan\
> > +   detect_deadlocks=false history_size=7\
> > +   halt_on_error=0 exitcode=0 verbose=5\
> > +   log_path=$tsan_log_dir/tsan_warnings.txt"
> > +export TSAN_OPTIONS="$tsan_opts"
> > +fi
>
> ...I think it would be better to put this in it's own test (test-tsan?)
>

Makes sense, we will put this TSan code in its own separate test.
Sure, test-tsan seems like a good name for this.

Thanks & Regards,
-Rob

> --
> Alex Bennée

Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding functions

[Eduardo Habkost]

On Wed, Jun 03, 2020 at 10:38:46AM -0500, Babu Moger wrote:
> 
> 
> On 6/3/20 10:31 AM, Eduardo Habkost wrote:
> > On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: Eduardo Habkost 
> >>> Sent: Tuesday, June 2, 2020 6:01 PM
> >>> To: Moger, Babu 
> >>> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> >>> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> >>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> >>> functions
> >>>
> >>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote:
> 
> 
> > -Original Message-
> > From: Eduardo Habkost 
> > Sent: Tuesday, June 2, 2020 12:19 PM
> > To: Moger, Babu 
> > Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> > m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> > functions
> >
> > Hi,
> >
> > It looks like this series breaks -device and CPU hotplug:
> >
> > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote:
> >> These functions add support for building EPYC mode topology given the
> >>> smp
> >> details like numa nodes, cores, threads and sockets.
> >>
> >> The new apic id decoding is mostly similar to current apic id decoding
> >> except that it adds a new field node_id when numa configured. Removes
> >>> all
> >> the hardcoded values. Subsequent patches will use these functions to 
> >> build
> >> the topology.
> >>
> >> Following functions are added.
> >> apicid_llc_width_epyc
> >> apicid_llc_offset_epyc
> >> apicid_pkg_offset_epyc
> >> apicid_from_topo_ids_epyc
> >> x86_topo_ids_from_idx_epyc
> >> x86_topo_ids_from_apicid_epyc
> >> x86_apicid_from_cpu_idx_epyc
> >>
> >> The topology details are available in Processor Programming Reference
> >>> (PPR)
> >> for AMD Family 17h Model 01h, Revision B1 Processors. The revision
> >>> guides
> > are
> >> available from the bugzilla Link below.
> >> Link:
> >
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> >
> >>> kernel.org%2Fshow_bug.cgi%3Fid%3D206537data=02%7C01%7Cbabu.m
> >
> >>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> >
> >>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739sdata=wE0
> > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3Dreserved=0
> >>
> >> Signed-off-by: Babu Moger 
> >> Acked-by: Igor Mammedov 
> >> Acked-by: Michael S. Tsirkin 
> >> ---
> > [...]
> >>  typedef struct X86CPUTopoIDs {
> >>  unsigned pkg_id;
> >> +unsigned node_id;
> >
> > You have added a new field here.
> >
> >>  unsigned die_id;
> >>  unsigned core_id;
> >>  unsigned smt_id;
> > [...]
> >> +static inline apic_id_t
> >> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
> >> +  const X86CPUTopoIDs *topo_ids)
> >> +{
> >> +return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
> >> +   (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
> >
> > You are using the new field here.
> >
> >> +   (topo_ids->die_id  << apicid_die_offset(topo_info)) |
> >> +   (topo_ids->core_id << apicid_core_offset(topo_info)) |
> >> +   topo_ids->smt_id;
> >> +}
> >
> > But you are not initializing node_id in one caller of 
> > apicid_from_topo_ids():
> >
> > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > [...]
> > X86CPUTopoIDs topo_ids;
> > [...]
> > if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > [...]
> > topo_ids.pkg_id = cpu->socket_id;
> > topo_ids.die_id = cpu->die_id;
> > topo_ids.core_id = cpu->core_id;
> > topo_ids.smt_id = cpu->thread_id;
> > cpu->apic_id = x86ms->apicid_from_topo_ids(_info, 
> > _ids);
> > }
> > [...]
> > }
> >
> > Result: -device is broken when using -cpu EPYC:
> >
> >   $ qemu-system-x86_64 -machine q35,accel=kvm -smp
> > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64-
> > cpu,core-id=0,socket-id=1,thread-id=0
> >>>
> >>> [1]
> >>>
> >   qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-
> >>> id=1,thread-
> > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC 
> > ID
> >>> 21855,
> > valid index range 0:1
> >
> > This happens because APIC ID is calculated using uninitialized
> > memory.
>  This patch should initialize the node_id. But I am not sure how to
>  reproduce the 

Re: [PATCH] ati-vga: increment mm_index in ati_mm_read/write

[BALATON Zoltan]

On Wed, 3 Jun 2020, BALATON Zoltan wrote:

On Wed, 3 Jun 2020, P J P wrote:

+-- On Wed, 3 Jun 2020, Gerd Hoffmann wrote --+
| Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
| before calling ati_mm_read/ati_mm_write?

 if (s->regs.mm_index & BIT(31)) {
...
 } else {
ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
 }

Exit condition for recursion is to set (mm_index & BIT(31)), so recursion
would continue even with non-zero values I think.


MM_INDEX and MM_DATA allows access to registers (or memory if BIT(31) is set) 
via only these two registers. This is called indexed access in docs. So it 
does not really make sense to allow access to these registers as well thus we 
can avoid infinite recursion. It's not meant to recurse until BIT(31) is set. 
I think you can do:


 if (s->regs.mm_index & BIT(31)) {
...
-  } else {
+  } else if (s->regs.mm_index >= BIOS_0_SCRATCH) {


Actually this should be

+  } else if (s->regs.mm_index >= CLOCK_CNTL_INDEX) {

or even > MM_DATA + 3 may be best as that only refers to defines used in 
that case. So maybe


+  } else if (s->regs.mm_index > MM_DATA + 3) {


ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
 }

and do the same in ati_mm_read() as well.

Regards,
BALATON Zoltan

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Robert Foley]

On Wed, 3 Jun 2020 at 07:43, Alex Bennée  wrote:
>
>
> Robert Foley  writes:
>

> >
> > When testing out the options, I noticed that
> > if we supply arguments of "read", and "write", then we will only get
> > the last one set, "write", since rw gets overwritten.
> > One option would be to error out if more than one of these read/write
> > args is supplied.
>
> Yeah the option parsing is a little clunky although given the way you
> pass them from the QEMU command line perhaps not too worth finessing.
> The default is rw so you make a conscious decision to only care about one
> or the other.
>
> All you can really do is fail to initialise the plugin. Hopefully the
> output should be enough clue.
>
> >
> > Reviewed-by: Robert Foley 
> > Tested-by: Robert Foley 
>
> Thanks.
>
> Out of interest what did you measure? Are there any useful use cases you can
> think of?

We did some testing where we booted an aarch64 VM and an i386 VM a few times
with differentcore counts (up to 64), and viewed the counters.  We
also did a test where
we inserted another device (a virtfs mount), booted up and checked
that there was another
device listed (for virtio-9p).

There are a few useful use cases we are thinking of, in general for debug/perf
 testing of PCI devices/drivers.
For example, debug and performance test of a case where we use a queue pair,
(maybe for something like DPDK/SPDK), this plugin would be interesting for
checking that the quantity and locations of accesses are expected.

Thanks & Regards,
-Rob
>
> >
> >> +detail = true;
> >> +} else {
> >> +fprintf(stderr, "option parsing failed: %s\n", opt);
> >> +return -1;
> >> +}
> >> +}
> >> +
> >> +plugin_init();
> >> +
> >> +qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> >> +qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> >> +return 0;
> >> +}
> >> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
> >> index b3250e2504c..d87b8d40699 100644
> >> --- a/tests/plugin/Makefile
> >> +++ b/tests/plugin/Makefile
> >> @@ -14,6 +14,7 @@ NAMES += hotblocks
> >>  NAMES += howvec
> >>  NAMES += hotpages
> >>  NAMES += lockstep
> >> +NAMES += hwprofile
> >>
> >>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
> >>
> >> --
> >> 2.20.1
> >>
>
>
> --
> Alex Bennée

Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding functions

[Babu Moger]

On 6/3/20 10:31 AM, Eduardo Habkost wrote:
> On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote:
>>
>>
>>> -Original Message-
>>> From: Eduardo Habkost 
>>> Sent: Tuesday, June 2, 2020 6:01 PM
>>> To: Moger, Babu 
>>> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
>>> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
>>> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
>>> functions
>>>
>>> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote:


> -Original Message-
> From: Eduardo Habkost 
> Sent: Tuesday, June 2, 2020 12:19 PM
> To: Moger, Babu 
> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> functions
>
> Hi,
>
> It looks like this series breaks -device and CPU hotplug:
>
> On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote:
>> These functions add support for building EPYC mode topology given the
>>> smp
>> details like numa nodes, cores, threads and sockets.
>>
>> The new apic id decoding is mostly similar to current apic id decoding
>> except that it adds a new field node_id when numa configured. Removes
>>> all
>> the hardcoded values. Subsequent patches will use these functions to 
>> build
>> the topology.
>>
>> Following functions are added.
>> apicid_llc_width_epyc
>> apicid_llc_offset_epyc
>> apicid_pkg_offset_epyc
>> apicid_from_topo_ids_epyc
>> x86_topo_ids_from_idx_epyc
>> x86_topo_ids_from_apicid_epyc
>> x86_apicid_from_cpu_idx_epyc
>>
>> The topology details are available in Processor Programming Reference
>>> (PPR)
>> for AMD Family 17h Model 01h, Revision B1 Processors. The revision
>>> guides
> are
>> available from the bugzilla Link below.
>> Link:
>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
>
>>> kernel.org%2Fshow_bug.cgi%3Fid%3D206537data=02%7C01%7Cbabu.m
>
>>> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
>
>>> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739sdata=wE0
> ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3Dreserved=0
>>
>> Signed-off-by: Babu Moger 
>> Acked-by: Igor Mammedov 
>> Acked-by: Michael S. Tsirkin 
>> ---
> [...]
>>  typedef struct X86CPUTopoIDs {
>>  unsigned pkg_id;
>> +unsigned node_id;
>
> You have added a new field here.
>
>>  unsigned die_id;
>>  unsigned core_id;
>>  unsigned smt_id;
> [...]
>> +static inline apic_id_t
>> +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
>> +  const X86CPUTopoIDs *topo_ids)
>> +{
>> +return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
>> +   (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
>
> You are using the new field here.
>
>> +   (topo_ids->die_id  << apicid_die_offset(topo_info)) |
>> +   (topo_ids->core_id << apicid_core_offset(topo_info)) |
>> +   topo_ids->smt_id;
>> +}
>
> But you are not initializing node_id in one caller of 
> apicid_from_topo_ids():
>
> static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> [...]
> X86CPUTopoIDs topo_ids;
> [...]
> if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> [...]
> topo_ids.pkg_id = cpu->socket_id;
> topo_ids.die_id = cpu->die_id;
> topo_ids.core_id = cpu->core_id;
> topo_ids.smt_id = cpu->thread_id;
> cpu->apic_id = x86ms->apicid_from_topo_ids(_info, _ids);
> }
> [...]
> }
>
> Result: -device is broken when using -cpu EPYC:
>
>   $ qemu-system-x86_64 -machine q35,accel=kvm -smp
> 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64-
> cpu,core-id=0,socket-id=1,thread-id=0
>>>
>>> [1]
>>>
>   qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-
>>> id=1,thread-
> id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID
>>> 21855,
> valid index range 0:1
>
> This happens because APIC ID is calculated using uninitialized
> memory.
 This patch should initialize the node_id. But I am not sure how to
 reproduce the bug. Can you please send me the full command line to
 reproduce the problem. Also test different options.
>>>
>>> The full command line is above[1].
>>
>> I just picked up the latest tree from
>> git clone 
>> 

Re: [PATCH] ati-vga: increment mm_index in ati_mm_read/write

[BALATON Zoltan]

On Wed, 3 Jun 2020, P J P wrote:

+-- On Wed, 3 Jun 2020, Gerd Hoffmann wrote --+
| Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
| before calling ati_mm_read/ati_mm_write?

 if (s->regs.mm_index & BIT(31)) {
...
 } else {
ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
 }

Exit condition for recursion is to set (mm_index & BIT(31)), so recursion
would continue even with non-zero values I think.


MM_INDEX and MM_DATA allows access to registers (or memory if BIT(31) is 
set) via only these two registers. This is called indexed access in docs. 
So it does not really make sense to allow access to these registers as 
well thus we can avoid infinite recursion. It's not meant to recurse until 
BIT(31) is set. I think you can do:


  if (s->regs.mm_index & BIT(31)) {
 ...
-  } else {
+  } else if (s->regs.mm_index >= BIOS_0_SCRATCH) {
 ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
  }

and do the same in ati_mm_read() as well.

Regards,
BALATON Zoltan

Re: [PATCH v3] osdep: Make MIN/MAX evaluate arguments only once

[Eric Blake]

On 6/2/20 9:29 PM, Eric Blake wrote:


+++ b/accel/tcg/translate-all.c
@@ -2565,9 +2565,9 @@ int page_check_range(target_ulong start, 
target_ulong len, int flags)

  /* This function should never be called with addresses outside the
 guest address space.  If this assert fires, it probably 
indicates

 a missing call to h2g_valid.  */
-#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
-    assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
-#endif
+    if (TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS) {
+    assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
+    }


IIRC the ifdef is required for clang warnings vs the shift.
Have you tested that?


I have not yet tested with clang.  We'll see if the CLI bots get to that 
before I do...  But if clang isn't happy, I may have to introduce yet a 
third macro, MIN_PP, safe for use in preprocessor statements.


I've now run a clang build over the entire tree (using clang 10.0.0 from 
Fedora 32, which required other pending patches mentioned on the list to 
work around unrelated warnings), the entire tree built without issue. 
So at least one version of clang compiled my rewrite of this hunk just 
fine, although it does not rule out what older versions might do.


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

Re: [PATCH] ati-vga: increment mm_index in ati_mm_read/write

[Gerd Hoffmann]

On Wed, Jun 03, 2020 at 08:05:50PM +0530, P J P wrote:
> +-- On Wed, 3 Jun 2020, Gerd Hoffmann wrote --+
> | Hmm, why modify mm_index?  Shouldn't we just check it is non-zero
> | before calling ati_mm_read/ati_mm_write?
> 
>   if (s->regs.mm_index & BIT(31)) {
>  ...
>   } else {

} else if (s->regs.mm_index > 3) {

>  ati_mm_write(s, s->regs.mm_index + addr - MM_DATA, data, size);
>   }
> 
> Exit condition for recursion is to set (mm_index & BIT(31)), so recursion 
> would continue even with non-zero values I think.

It's wrapped into a case switch for all the registers.  The code quoted
above is only entered for "addr >= MM_DATA && addr <= MM_DATA+3", so the
check should stop recursion.  A less strict "s->regs.mm_index > 0" may
recurse a few times but will not recurse endless either.

cheers,
  Gerd

Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding functions

[Eduardo Habkost]

On Wed, Jun 03, 2020 at 10:22:10AM -0500, Babu Moger wrote:
> 
> 
> > -Original Message-
> > From: Eduardo Habkost 
> > Sent: Tuesday, June 2, 2020 6:01 PM
> > To: Moger, Babu 
> > Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> > m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> > functions
> > 
> > On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Eduardo Habkost 
> > > > Sent: Tuesday, June 2, 2020 12:19 PM
> > > > To: Moger, Babu 
> > > > Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> > > > m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> > > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> > > > functions
> > > >
> > > > Hi,
> > > >
> > > > It looks like this series breaks -device and CPU hotplug:
> > > >
> > > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote:
> > > > > These functions add support for building EPYC mode topology given the
> > smp
> > > > > details like numa nodes, cores, threads and sockets.
> > > > >
> > > > > The new apic id decoding is mostly similar to current apic id decoding
> > > > > except that it adds a new field node_id when numa configured. Removes
> > all
> > > > > the hardcoded values. Subsequent patches will use these functions to 
> > > > > build
> > > > > the topology.
> > > > >
> > > > > Following functions are added.
> > > > > apicid_llc_width_epyc
> > > > > apicid_llc_offset_epyc
> > > > > apicid_pkg_offset_epyc
> > > > > apicid_from_topo_ids_epyc
> > > > > x86_topo_ids_from_idx_epyc
> > > > > x86_topo_ids_from_apicid_epyc
> > > > > x86_apicid_from_cpu_idx_epyc
> > > > >
> > > > > The topology details are available in Processor Programming Reference
> > (PPR)
> > > > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision
> > guides
> > > > are
> > > > > available from the bugzilla Link below.
> > > > > Link:
> > > >
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > > >
> > kernel.org%2Fshow_bug.cgi%3Fid%3D206537data=02%7C01%7Cbabu.m
> > > >
> > oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > > >
> > 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739sdata=wE0
> > > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3Dreserved=0
> > > > >
> > > > > Signed-off-by: Babu Moger 
> > > > > Acked-by: Igor Mammedov 
> > > > > Acked-by: Michael S. Tsirkin 
> > > > > ---
> > > > [...]
> > > > >  typedef struct X86CPUTopoIDs {
> > > > >  unsigned pkg_id;
> > > > > +unsigned node_id;
> > > >
> > > > You have added a new field here.
> > > >
> > > > >  unsigned die_id;
> > > > >  unsigned core_id;
> > > > >  unsigned smt_id;
> > > > [...]
> > > > > +static inline apic_id_t
> > > > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
> > > > > +  const X86CPUTopoIDs *topo_ids)
> > > > > +{
> > > > > +return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
> > > > > +   (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) 
> > > > > |
> > > >
> > > > You are using the new field here.
> > > >
> > > > > +   (topo_ids->die_id  << apicid_die_offset(topo_info)) |
> > > > > +   (topo_ids->core_id << apicid_core_offset(topo_info)) |
> > > > > +   topo_ids->smt_id;
> > > > > +}
> > > >
> > > > But you are not initializing node_id in one caller of 
> > > > apicid_from_topo_ids():
> > > >
> > > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > > > DeviceState *dev, Error **errp)
> > > > {
> > > > [...]
> > > > X86CPUTopoIDs topo_ids;
> > > > [...]
> > > > if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > > > [...]
> > > > topo_ids.pkg_id = cpu->socket_id;
> > > > topo_ids.die_id = cpu->die_id;
> > > > topo_ids.core_id = cpu->core_id;
> > > > topo_ids.smt_id = cpu->thread_id;
> > > > cpu->apic_id = x86ms->apicid_from_topo_ids(_info, 
> > > > _ids);
> > > > }
> > > > [...]
> > > > }
> > > >
> > > > Result: -device is broken when using -cpu EPYC:
> > > >
> > > >   $ qemu-system-x86_64 -machine q35,accel=kvm -smp
> > > > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64-
> > > > cpu,core-id=0,socket-id=1,thread-id=0
> > 
> > [1]
> > 
> > > >   qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-
> > id=1,thread-
> > > > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC 
> > > > ID
> > 21855,
> > > > valid index range 0:1
> > > >
> > > > This happens because APIC ID is calculated using uninitialized
> > > > memory.
> > > This patch should initialize the node_id. But I am not sure how to
> > > reproduce the bug. Can you please send me the full command line to
> > > reproduce the problem. Also test different options.

Re: [PATCH v3 0/2] fuzz: Skip QTest serialization

[Alexander Bulekov]

On 200603 1646, Thomas Huth wrote:
> On 02/06/2020 15.40, Alexander Bulekov wrote:
> > Thank you Darren.
> > 
> > On 200602 1428, Darren Kenny wrote:
> >>
> >> Hi Alex,
> >>
> >> In general the series looks good, so:
> >>
> >> Reviewed-by: Darren Kenny 
> >>
> >> But not sure how to handle the patchew output though, not sure if it is
> >> really a concern or not, since do/while won't work that context.
> >>
> > 
> > Yes - I was not really sure how to deal with those failures, so I sent
> > the patch anyway. Maybe someone else knows a workaround.
> > -Alex
> 
> Don't worry, the checkpatch script is known to generate false
> warnings/errors in some cases. If you've got such a case, simply state
> it as a reply to the mail from patchew, and then the message from
> patchew can be simply ignored.
> 
> By the way, I'm finally back to the state where I can pick up qtest
> patches again (btw2, thanks for Stefan for picking up all the fuzzer
> patches in the past months). So question: With your two patches here,
> the patch from Philippe is not required anymore, right?

Hi Thomas,
Yes - I think this series enables the same improvements as Philippe's
patch.
Thank you
-Alex

>  Thanks,
>   Thomas 

> 
> >> On Friday, 2020-05-29 at 18:14:48 -04, Alexander Bulekov wrote:
> >>> In the same vein as Philippe's patch:
> >>>
> >>> https://patchew.org/QEMU/20200528165303.1877-1-f4...@amsat.org/
> >>>
> >>> This uses linker trickery to wrap calls to libqtest functions and
> >>> directly call the corresponding read/write functions, rather than
> >>> relying on the ASCII-serialized QTest protocol.
> >>>
> >>> v2: applies properly
> >>>
> >>> v3: add missing qtest_wrappers.c file and fix formatting in fuzz.c
> >>>
> >>> Alexander Bulekov (2):
> >>>   fuzz: skip QTest serialization
> >>>   fuzz: Add support for logging QTest commands
> >>>
> >>>  tests/qtest/fuzz/Makefile.include |  21 +++
> >>>  tests/qtest/fuzz/fuzz.c   |  20 ++-
> >>>  tests/qtest/fuzz/fuzz.h   |   3 +
> >>>  tests/qtest/fuzz/qtest_wrappers.c | 252 ++
> >>>  4 files changed, 295 insertions(+), 1 deletion(-)
> >>>  create mode 100644 tests/qtest/fuzz/qtest_wrappers.c
> >>>
> >>> -- 
> >>> 2.26.2
> > 
> 

RE: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding functions

[Babu Moger]

> -Original Message-
> From: Eduardo Habkost 
> Sent: Tuesday, June 2, 2020 6:01 PM
> To: Moger, Babu 
> Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> functions
> 
> On Tue, Jun 02, 2020 at 04:59:19PM -0500, Babu Moger wrote:
> >
> >
> > > -Original Message-
> > > From: Eduardo Habkost 
> > > Sent: Tuesday, June 2, 2020 12:19 PM
> > > To: Moger, Babu 
> > > Cc: marcel.apfelb...@gmail.com; pbonz...@redhat.com; r...@twiddle.net;
> > > m...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org
> > > Subject: Re: [PATCH v7 07/13] hw/386: Add EPYC mode topology decoding
> > > functions
> > >
> > > Hi,
> > >
> > > It looks like this series breaks -device and CPU hotplug:
> > >
> > > On Wed, Mar 11, 2020 at 05:53:34PM -0500, Babu Moger wrote:
> > > > These functions add support for building EPYC mode topology given the
> smp
> > > > details like numa nodes, cores, threads and sockets.
> > > >
> > > > The new apic id decoding is mostly similar to current apic id decoding
> > > > except that it adds a new field node_id when numa configured. Removes
> all
> > > > the hardcoded values. Subsequent patches will use these functions to 
> > > > build
> > > > the topology.
> > > >
> > > > Following functions are added.
> > > > apicid_llc_width_epyc
> > > > apicid_llc_offset_epyc
> > > > apicid_pkg_offset_epyc
> > > > apicid_from_topo_ids_epyc
> > > > x86_topo_ids_from_idx_epyc
> > > > x86_topo_ids_from_apicid_epyc
> > > > x86_apicid_from_cpu_idx_epyc
> > > >
> > > > The topology details are available in Processor Programming Reference
> (PPR)
> > > > for AMD Family 17h Model 01h, Revision B1 Processors. The revision
> guides
> > > are
> > > > available from the bugzilla Link below.
> > > > Link:
> > >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > >
> kernel.org%2Fshow_bug.cgi%3Fid%3D206537data=02%7C01%7Cbabu.m
> > >
> oger%40amd.com%7C3487f40d37df4d59097d08d807190248%7C3dd8961fe488
> > >
> 4e608e11a82d994e183d%7C0%7C0%7C637267151289763739sdata=wE0
> > > ukXIVh0l5eNQWsv6VDE9UZEVJmisofaW192gcZAI%3Dreserved=0
> > > >
> > > > Signed-off-by: Babu Moger 
> > > > Acked-by: Igor Mammedov 
> > > > Acked-by: Michael S. Tsirkin 
> > > > ---
> > > [...]
> > > >  typedef struct X86CPUTopoIDs {
> > > >  unsigned pkg_id;
> > > > +unsigned node_id;
> > >
> > > You have added a new field here.
> > >
> > > >  unsigned die_id;
> > > >  unsigned core_id;
> > > >  unsigned smt_id;
> > > [...]
> > > > +static inline apic_id_t
> > > > +x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
> > > > +  const X86CPUTopoIDs *topo_ids)
> > > > +{
> > > > +return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
> > > > +   (topo_ids->node_id << apicid_node_offset_epyc(topo_info)) |
> > >
> > > You are using the new field here.
> > >
> > > > +   (topo_ids->die_id  << apicid_die_offset(topo_info)) |
> > > > +   (topo_ids->core_id << apicid_core_offset(topo_info)) |
> > > > +   topo_ids->smt_id;
> > > > +}
> > >
> > > But you are not initializing node_id in one caller of 
> > > apicid_from_topo_ids():
> > >
> > > static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > > DeviceState *dev, Error **errp)
> > > {
> > > [...]
> > > X86CPUTopoIDs topo_ids;
> > > [...]
> > > if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > > [...]
> > > topo_ids.pkg_id = cpu->socket_id;
> > > topo_ids.die_id = cpu->die_id;
> > > topo_ids.core_id = cpu->core_id;
> > > topo_ids.smt_id = cpu->thread_id;
> > > cpu->apic_id = x86ms->apicid_from_topo_ids(_info, _ids);
> > > }
> > > [...]
> > > }
> > >
> > > Result: -device is broken when using -cpu EPYC:
> > >
> > >   $ qemu-system-x86_64 -machine q35,accel=kvm -smp
> > > 1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device EPYC-x86_64-
> > > cpu,core-id=0,socket-id=1,thread-id=0
> 
> [1]
> 
> > >   qemu-system-x86_64: -device EPYC-x86_64-cpu,core-id=0,socket-
> id=1,thread-
> > > id=0: Invalid CPU [socket: 21855, die: 0, core: 0, thread: 0] with APIC ID
> 21855,
> > > valid index range 0:1
> > >
> > > This happens because APIC ID is calculated using uninitialized
> > > memory.
> > This patch should initialize the node_id. But I am not sure how to
> > reproduce the bug. Can you please send me the full command line to
> > reproduce the problem. Also test different options.
> 
> The full command line is above[1].

I just picked up the latest tree from
git clone https://git.qemu.org/git/qemu.git qemu
Not seeing the problem.

./x86_64-softmmu/qemu-system-x86_64 -machine q35,accel=kvm -smp
1,maxcpus=2,cores=1,threads=1,sockets=2 -cpu EPYC -device
EPYC-x86_64-cpu,core-id=0,socket-id=1,thread-id=0
VNC server running on 

Re: [PATCH] qom-hmp-cmds: fix a memleak in hmp_qom_get

[Li Qiang]

Pan Nengyuan  于2020年6月3日周三 下午2:17写道：

> 'obj' forgot to free at the end of hmp_qom_get(). Fix that.
>
> The leak stack:
> Direct leak of 40 byte(s) in 1 object(s) allocated from:
> #0 0x7f4e3a779ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8)
> #1 0x7f4e398f91d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5)
> #2 0x55c9fd9a3999 in qstring_from_substr
> /build/qemu/src/qobject/qstring.c:45
> #3 0x55c9fd894bd3 in qobject_output_type_str
> /build/qemu/src/qapi/qobject-output-visitor.c:175
> #4 0x55c9fd894bd3 in qobject_output_type_str
> /build/qemu/src/qapi/qobject-output-visitor.c:168
> #5 0x55c9fd88b34d in visit_type_str
> /build/qemu/src/qapi/qapi-visit-core.c:308
> #6 0x55c9fd59aa6b in property_get_str /build/qemu/src/qom/object.c:2064
> #7 0x55c9fd5adb8a in object_property_get_qobject
> /build/qemu/src/qom/qom-qobject.c:38
> #8 0x55c9fd4a029d in hmp_qom_get /build/qemu/src/qom/qom-hmp-cmds.c:66
>
> Fixes: 89cf4fe34f4
> Reported-by: Euler Robot 
> Signed-off-by: Pan Nengyuan 
>


This can be tested by compile qemu with '-fsanitize=address' cflags and:
make check

Reviewed-by: Li Qiang 
Tested-by: Li Qiang 

Li Qiang


> ---
>  qom/qom-hmp-cmds.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index f704b6949a..3d2a23292d 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -71,6 +71,7 @@ void hmp_qom_get(Monitor *mon, const QDict *qdict)
>  qobject_unref(str);
>  }
>
> +qobject_unref(obj);
>  hmp_handle_error(mon, err);
>  }
>
> --
> 2.18.2
>
>
>

Re: [PATCH v3 00/20] virtio-mem: Paravirtualized memory hot(un)plug

[Eric Blake]

On 6/3/20 9:48 AM, David Hildenbrand wrote:

This is the very basic, initial version of virtio-mem. More info on
virtio-mem in general can be found in the Linux kernel driver v2 posting
[1] and in patch #10. The latest Linux driver v4 can be found at [2].

This series is based on [3]:
 "[PATCH v1] pc: Support coldplugging of virtio-pmem-pci devices on all
  buses"



Let's spell that in a form patchew can recognize:

Based-on: <20200525084511.51379-1-da...@redhat.com>

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

RE: [Question] Regarding containers "unattached/peripheral/anonymous" - their relation with hot(un)plug of devices

[Salil Mehta]

Hi Igor,
My sincere Apologies, I just realized that I missed to reply this mail.
I was distracted to something else in  the month of the February and
had only resumed working on hotplug in march. But will still reply to
this mail. Also, I have incorporated most of the below points as in the
vcpu hotplug patches as per your suggestions.


> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei@nongnu.org]
> On Behalf Of Igor Mammedov
> Sent: Monday, January 27, 2020 3:04 PM
> To: Salil Mehta 
> 
> On Fri, 24 Jan 2020 18:44:16 +
> Salil Mehta  wrote:
> 
> > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > Sent: Friday, January 24, 2020 4:07 PM
> > >
> > > On Fri, 24 Jan 2020 15:02:10 +
> > > Salil Mehta  wrote:
> > >
> > > > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > > > Sent: Friday, January 24, 2020 1:54 PM
> > > > > To: Salil Mehta 
> > > > >
> > > > > On Fri, 24 Jan 2020 11:20:15 +
> > > > > Salil Mehta  wrote:
> > > > >
> > > > > > Hello,
> > > > > > I am working on vCPU Hotplug feature for ARM64 and I am in mid of 
> > > > > > understanding
> > > > > > some aspect of device_add/device_del interface of the QEMU.
> > > > > >
> > > > > > Observations:
> > > > > > 1. Any object initialised by qmp_device_add() gets into 
> > > > > > /machine/unattached
> > > > > > container. I traced the flow to code leg inside  
> > > > > > device_set_realized()
> > > > > > 2. I could see the reverse qmp_device_del() expects the device to 
> > > > > > be in
> > > > > > /machine/peripheral container.
> > > > > > 3. I could see any object initially added to unattached container 
> > > > > > did not had
> > > > > > their parents until object_add_property_child() was called further 
> > > > > > in the leg.
> > > > > > which effectively meant a new property was created and property 
> > > > > > table
> > > > > > populated and child was parented.
> > > > > > 4. Generally, container  /machine/peripheral was being used wherever
> > > > > > DEVICE(dev)->id was present and non-null.
> > > > > >
> > > > > > Question:
> > > > > > 1. Wanted to confirm my understanding about the use of having 
> > > > > > separate
> > > > > > containers like unattached, peripheral and anonymous.
> > > > >
> > > > > > 2. At init time all the vcpus goes under *unattached* container. 
> > > > > > Now,
> > > > > > qmp_device_del() cannot be used to unplug them. I am wondering
> > > > >
> > > > > device is put into 'unattached' in case it wasn't assigned a parent.
> > > > > Usually it happens when board creates device directly.
> > > >
> > > >
> > > > Sure, but if we decide that certain number(N) of vcpus are hotplugabble
> > > > and certain subset of N (say 'n' < 'N') should be allowed to be present
> > > > or cold-plugged at the init time then I wonder which of the following
> > > > is correct approach:
> > > >
> > > > 1. Bring all of N vcpus at boot time under "peripheral" container
> > > >OR
> > > > 2. Just bring subset 'n' of 'N' under "peripheral" container and rest
> > > > under "unattached" container? And later as and when rest of the
> > > > vcpus are hotplugged they should be transferred from "unattached"
> > > > container to "peripheral" container?
> > >
> > > issue with that is that to put device into "peripheral" container,
> > > 'the user' must provide 'id'. (currently QEMU isn't able to do it on its 
> > > own
> > > [1])
> > >
> > > But it doesn't mean that cold-plugged CPUs can't be unpluged.
> > > What current users could do is start QEMU like this (simplified version):
> > >
> > >  $QEMU -smp 1,maxcpus=N -device foo-cpu-type,id=CPU00 -device
> > > foo-cpu-type,id=CPU01 ...
> > >
> > > i.e. 1st CPU is not manageable due to lack if 'id' and is created by 
> > > board code,
> > > the rest have 'id' and could be managed.
> >
> >
> > I understand, that we can somehow assign ids from the QMP interface but
> > above will not push vcpus into "peripheral" container. They will appear
> > in "unattached" container but with specified names and as-far-as I can
> > see in the code 'device_del' can only delete objects/devices from the
> > 'peripheral' container?
> 
> qemu-system-x86_64 -monitor stdio \
> -smp 1,maxcpus=3 \
> -device qemu64-x86_64-cpu,id=foo,socket-id=1,core-id=0,thread-id=0 \
> -device qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> 
> (qemu) info hotpluggable-cpus
> Hotpluggable CPUs:
>   type: "qemu64-x86_64-cpu"
>   vcpus_count: "1"
>   qom_path: "/machine/peripheral-anon/device[0]"
>   ^^^
>   CPUInstance Properties:
> socket-id: "2"
> core-id: "0"
> thread-id: "0"
>   type: "qemu64-x86_64-cpu"
>   vcpus_count: "1"
>   qom_path: "/machine/peripheral/foo"
>   ^^
> 
> in gist, if device is created with any variant of device_add,
> it goes to "peripheral[-anon]" including cold-plugged one.


I am not sure why you said "including cold-plugged one"? I hope by

Re: [PATCH 1/1] util/oslib: Returns real thread identifier on FreeBSD and NetBSD

[Li-Wen Hsu]

Sorry that I am not familiar with this part so I asked others to help
review (the FreeBSD related code).  It's good and please append:

Reviewed-by: Edward Tomasz Napierala 

Thanks!

On Wed, Jun 3, 2020 at 2:14 PM David CARLIER  wrote:
>
> Sorry it landed in the spam.
>
> It does make things more accurate, thus a bit more than cosmetic, as
> stated in the commit message, thr_self/_lwp_self represents the
> current thread id in multi thread context.
>
> For OpenBSD it is syscall(SYS_getthrid) I believe
> https://man.openbsd.org/getthrid.2
>
> On Wed, 3 Jun 2020 at 06:12, Philippe Mathieu-Daudé  wrote:
> >
> > ping?
> >
> > On 5/26/20 9:29 AM, David CARLIER wrote:
> > > From 792fbcd9114f43bd80fd1ef5b25cd9935a536f9f Mon Sep 17 00:00:00 2001
> > > From: David Carlier 
> > > Date: Tue, 26 May 2020 08:25:26 +0100
> > > Subject: [PATCH] util/oslib: Returns the real thread identifier on 
> > > FreeBSD and
> > >  NetBSD
> > >
> > > getpid is good enough in a mono thread context, however
> > >  thr_self/_lwp_self reflects the real current thread identifier
> > >  from a given process.
> > > ---
> > >  util/oslib-posix.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > > index 062236a1ab..916f1be224 100644
> > > --- a/util/oslib-posix.c
> > > +++ b/util/oslib-posix.c
> > > @@ -48,11 +48,13 @@
> > >  #ifdef __FreeBSD__
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #endif
> > >
> > >  #ifdef __NetBSD__
> > >  #include 
> > > +#include 
> > >  #endif
> > >
> > >  #include "qemu/mmap-alloc.h"
> > > @@ -84,6 +86,13 @@ int qemu_get_thread_id(void)
> > >  {
> > >  #if defined(__linux__)
> > >  return syscall(SYS_gettid);
> > > +#elif defined(__FreeBSD__)
> > > +/* thread id is up to INT_MAX */
> > > +long tid;
> > > +thr_self();
> > > +return (int)tid;
> > > +#elif defined(__NetBSD__)
> > > +return _lwp_self();
> > >  #else
> > >  return getpid();
> > >  #endif
> > >
> >

Re: [PATCH 3/4] block: Add block accounting code for GET LBA STATUS

[Claudio Fontana]

On 6/2/20 9:42 AM, Lin Ma wrote:
> Signed-off-by: Lin Ma 
> ---
>  include/block/accounting.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 878b4c3581..645014fb0b 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -38,6 +38,7 @@ enum BlockAcctType {
>  BLOCK_ACCT_WRITE,
>  BLOCK_ACCT_FLUSH,
>  BLOCK_ACCT_UNMAP,
> +BLOCK_ACCT_GET_LBA_STATUS,
>  BLOCK_MAX_IOTYPE,
>  };
>  
> 

This should probably be combined with the change that makes use of 
BLOCK_ACCT_GET_LBA_STATUS to constitute a single, meaningful change.

Ciao,

Claudio

[PATCH v2 2/2] hw: arm: Set vendor property for IMX SDHCI emulations

[Guenter Roeck]

Set vendor property to IMX to enable IMX specific functionality
in sdhci code.

Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Guenter Roeck 
---
v2: Added missing error checks
Added Philippe's Tested-by: tag

 hw/arm/fsl-imx25.c  | 6 ++
 hw/arm/fsl-imx6.c   | 6 ++
 hw/arm/fsl-imx6ul.c | 2 ++
 hw/arm/fsl-imx7.c   | 2 ++
 4 files changed, 16 insertions(+)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index cdaa79c26b..a853ffcc00 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -274,6 +274,12 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
**errp)
  );
 object_property_set_uint(OBJECT(>esdhc[i]), 
IMX25_ESDHC_CAPABILITIES,
  "capareg", );
+object_property_set_uint(OBJECT(>esdhc[i]), SDHCI_VENDOR_IMX,
+ "vendor", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>esdhc[i]), true, "realized", );
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index f58c85aa8c..29677cfd59 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -350,6 +350,12 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
**errp)
  );
 object_property_set_uint(OBJECT(>esdhc[i]), IMX6_ESDHC_CAPABILITIES,
  "capareg", );
+object_property_set_uint(OBJECT(>esdhc[i]), SDHCI_VENDOR_IMX,
+ "vendor", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>esdhc[i]), true, "realized", );
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index 3ecb212da6..ce1462927c 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -505,6 +505,8 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
**errp)
 FSL_IMX6UL_USDHC2_IRQ,
 };
 
+object_property_set_uint(OBJECT(>usdhc[i]), SDHCI_VENDOR_IMX,
+"vendor", _abort);
 object_property_set_bool(OBJECT(>usdhc[i]), true, "realized",
  _abort);
 
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 89c3b64c06..dbf16b2814 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -416,6 +416,8 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
 FSL_IMX7_USDHC3_IRQ,
 };
 
+object_property_set_uint(OBJECT(>usdhc[i]), SDHCI_VENDOR_IMX,
+ "vendor", _abort);
 object_property_set_bool(OBJECT(>usdhc[i]), true, "realized",
  _abort);
 
-- 
2.17.1

Re: [PATCH 4/4] scsi-disk: Add support for the GET LBA STATUS 16 command

[Claudio Fontana]

On 6/2/20 9:42 AM, Lin Ma wrote:
> Signed-off-by: Lin Ma 
> ---
>  hw/scsi/scsi-disk.c  | 92 
>  include/scsi/constants.h |  1 +
>  2 files changed, 93 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 387503e11b..2d2c6b4b82 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1866,6 +1866,91 @@ static void scsi_disk_emulate_write_data(SCSIRequest 
> *req)
>  }
>  }
>  
> +typedef struct GetLbaStatusCBData {
> +uint32_t num_blocks;
> +uint32_t is_deallocated;
> +SCSIDiskReq *r;
> +} GetLbaStatusCBData;
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret);
> +
> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int 
> ret)
> +{
> +SCSIDiskReq *r = data->r;
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +assert(r->req.aiocb == NULL);
> +
> +block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
> + s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
> +
> +r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
> +  r->req.cmd.lba * s->qdev.blocksize,
> +  s->qdev.blocksize,
> +  scsi_get_lba_status_complete, 
> data);
> +return;

this return statement does not add anything of value, I think you can remove it.

> +}
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret)
> +{
> +GetLbaStatusCBData *data = opaque;
> +SCSIDiskReq *r = data->r;
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +assert(r->req.aiocb != NULL);
> +r->req.aiocb = NULL;
> +
> +aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> +if (scsi_disk_req_check_error(r, ret, true)) {
> +g_free(data);
> +goto done;
> +}
> +
> +block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
> +scsi_req_unref(>req);
> +g_free(data);
> +
> +done:
> +aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> +}
> +
> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t 
> *outbuf)
> +{
> +SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> +GetLbaStatusCBData *data;
> +uint32_t *num_blocks;
> +uint32_t *is_deallocated;
> +
> +data = g_new0(GetLbaStatusCBData, 1);
> +data->r = r;
> +num_blocks = &(data->num_blocks);
> +is_deallocated = &(data->is_deallocated);
> +
> +scsi_req_ref(>req);
> +scsi_get_lba_status_complete_noio(data, 0);
> +
> +/* 8 + 16 is the length in bytes of response header and
> + * one LBA status descriptor
> + */

this should probably look like:

/*
 * 8 + 16 ...
 * one LBA ...
 */

> +memset(outbuf, 0, 8 + 16);
> +outbuf[3] = 20;
> +outbuf[8] = (req->cmd.lba >> 56) & 0xff;
> +outbuf[9] = (req->cmd.lba >> 48) & 0xff;
> +outbuf[10] = (req->cmd.lba >> 40) & 0xff;
> +outbuf[11] = (req->cmd.lba >> 32) & 0xff;
> +outbuf[12] = (req->cmd.lba >> 24) & 0xff;
> +outbuf[13] = (req->cmd.lba >> 16) & 0xff;
> +outbuf[14] = (req->cmd.lba >> 8) & 0xff;
> +outbuf[15] = req->cmd.lba & 0xff;
> +outbuf[16] = (*num_blocks >> 24) & 0xff;
> +outbuf[17] = (*num_blocks >> 16) & 0xff;
> +outbuf[18] = (*num_blocks >> 8) & 0xff;
> +outbuf[19] = *num_blocks & 0xff;
> +outbuf[20] = *is_deallocated ? 1 : 0;
> +
> +return;

(see above)

> +}
> +
>  static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>  {
>  SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> @@ -2076,6 +2161,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
> *req, uint8_t *buf)
>  
>  /* Protection, exponent and lowest lba field left blank. */
>  break;
> +} else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
> +if (req->cmd.lba > s->qdev.max_lba) {
> +goto illegal_lba;
> +}
> +scsi_disk_emulate_get_lba_status(req, outbuf);
> +r->iov.iov_len = req->cmd.xfer;
> +return r->iov.iov_len;
>  }
>  trace_scsi_disk_emulate_command_SAI_unsupported();
>  goto illegal_request;
> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
> index 874176019e..1b6417898a 100644
> --- a/include/scsi/constants.h
> +++ b/include/scsi/constants.h
> @@ -154,6 +154,7 @@
>   * SERVICE ACTION IN subcodes
>   */
>  #define SAI_READ_CAPACITY_16  0x10
> +#define SAI_GET_LBA_STATUS  0x12
>  
>  /*
>   * READ POSITION service action codes
> 

[PATCH v3 18/20] virtio-mem: Migration sanity checks

[David Hildenbrand]

We want to make sure that certain properties don't change during
migration, especially to catch user errors in a nice way. Let's migrate
a temporary structure and validate that the properties didn't change.

Cc: "Michael S. Tsirkin" 
Cc: "Dr. David Alan Gilbert" 
Signed-off-by: David Hildenbrand 
---
 hw/virtio/virtio-mem.c | 69 ++
 1 file changed, 69 insertions(+)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 455d957e17..158215613c 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -519,12 +519,81 @@ static int virtio_mem_post_load(void *opaque, int 
version_id)
 return virtio_mem_restore_unplugged(VIRTIO_MEM(opaque));
 }
 
+typedef struct VirtIOMEMMigSanityChecks {
+VirtIOMEM *parent;
+uint64_t addr;
+uint64_t region_size;
+uint32_t block_size;
+uint32_t node;
+} VirtIOMEMMigSanityChecks;
+
+static int virtio_mem_mig_sanity_checks_pre_save(void *opaque)
+{
+VirtIOMEMMigSanityChecks *tmp = opaque;
+VirtIOMEM *vmem = tmp->parent;
+
+tmp->addr = vmem->addr;
+tmp->region_size = memory_region_size(>memdev->mr);
+tmp->block_size = vmem->block_size;
+tmp->node = vmem->node;
+return 0;
+}
+
+static int virtio_mem_mig_sanity_checks_post_load(void *opaque, int version_id)
+{
+VirtIOMEMMigSanityChecks *tmp = opaque;
+VirtIOMEM *vmem = tmp->parent;
+const uint64_t new_region_size = memory_region_size(>memdev->mr);
+
+if (tmp->addr != vmem->addr) {
+error_report("Property '%s' changed from 0x%" PRIx64 " to 0x%" PRIx64,
+ VIRTIO_MEM_ADDR_PROP, tmp->addr, vmem->addr);
+return -EINVAL;
+}
+/*
+ * Note: Preparation for resizeable memory regions. The maximum size
+ * of the memory region must not change during migration.
+ */
+if (tmp->region_size != new_region_size) {
+error_report("region size changed from 0x%" PRIx64 " to 0x%" PRIx64,
+ tmp->region_size, new_region_size);
+return -EINVAL;
+}
+if (tmp->block_size != vmem->block_size) {
+error_report("Property '%s' changed from %0x" PRIx32 " to %0x" PRIx32,
+ VIRTIO_MEM_BLOCK_SIZE_PROP, tmp->block_size,
+ vmem->block_size);
+return -EINVAL;
+}
+if (tmp->node != vmem->node) {
+error_report("Property '%s' changed from %" PRIu32 " to %" PRIu32,
+ VIRTIO_MEM_NODE_PROP, tmp->node, vmem->node);
+return -EINVAL;
+}
+return 0;
+}
+
+static const VMStateDescription vmstate_virtio_mem_sanity_checks = {
+.name = "virtio-mem-device/sanity-checks",
+.pre_save = virtio_mem_mig_sanity_checks_pre_save,
+.post_load = virtio_mem_mig_sanity_checks_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_UINT64(addr, VirtIOMEMMigSanityChecks),
+VMSTATE_UINT64(region_size, VirtIOMEMMigSanityChecks),
+VMSTATE_UINT32(block_size, VirtIOMEMMigSanityChecks),
+VMSTATE_UINT32(node, VirtIOMEMMigSanityChecks),
+VMSTATE_END_OF_LIST(),
+},
+};
+
 static const VMStateDescription vmstate_virtio_mem_device = {
 .name = "virtio-mem-device",
 .minimum_version_id = 1,
 .version_id = 1,
 .post_load = virtio_mem_post_load,
 .fields = (VMStateField[]) {
+VMSTATE_WITH_TMP(VirtIOMEM, VirtIOMEMMigSanityChecks,
+ vmstate_virtio_mem_sanity_checks),
 VMSTATE_UINT64(usable_region_size, VirtIOMEM),
 VMSTATE_UINT64(size, VirtIOMEM),
 VMSTATE_UINT64(requested_size, VirtIOMEM),
-- 
2.25.4