date:20220728


On 7/28/22 09:55, Daniel Henrique Barboza wrote:

The following changes since commit 3e4abe2c92964aadd35344a635b0f32cb487fd5c:

   Merge tag 'pull-block-2022-07-27' of https://gitlab.com/vsementsov/qemu into 
staging (2022-07-27 20:10:15 -0700)

are available in the Git repository at:

   https://gitlab.com/danielhb/qemu.git pull-ppc-20220728

for you to fetch changes up to 0c9717ff35d2fe46fa9cb91566fe2afbed9f4f2a:

   target/ppc: Implement new wait variants (2022-07-28 13:30:41 -0300)


ppc patch queue for 2022-07-28:

Short queue with 2 Coverity fixes and one fix of the
'wait' insns that is causing hangs if the guest kernel uses
the most up to date wait opcode.

- target/ppc:
   - implement new wait variants to fix guest hang when using the new opcode
- ppc440_uc: initialize length passed to cpu_physical_memory_map()
- spapr_nvdimm: check if spapr_drc_index() returns NULL


Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/7.1 as 
appropriate.


r~





Daniel Henrique Barboza (1):
   hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c

Nicholas Piggin (1):
   target/ppc: Implement new wait variants

Peter Maydell (1):
   hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

  hw/ppc/ppc440_uc.c |  5 ++-
  hw/ppc/spapr_nvdimm.c  | 18 +++---
  target/ppc/internal.h  |  3 ++
  target/ppc/translate.c | 96 +-
  4 files changed, 109 insertions(+), 13 deletions(-)

Re: [RFC 0/3] Add Generic SPI GPIO model

2022-07-28 Thread Peter Delevoryas

On Thu, Jul 28, 2022 at 04:23:19PM -0700, Iris Chen wrote:
> Hey everyone,
> 
> I have been working on a project to add support for SPI-based TPMs in QEMU.
> Currently, most of our vboot platforms using a SPI-based TPM use the Linux 
> SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed 
> SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an 
> implementation 
> deficiency that prevents bi-directional operations. Thus, in order to connect 
> a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
> counterpart of the Linux SPI-GPIO driver).
> 
> As we use SPI-based TPMs on many of our BMCs for the secure-boot 
> implementation,  
> I have already tested this implementation locally with our Yosemite-v3.5 
> platform 
> and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR 
> (m25p80 
> for example) to the Yosemite-v3.5 SPI bus containing the TPM.
> 
> This patch is an RFC because I have several questions about design. Although 
> the
> model is working, I understand there are many areas where the design decision
> is not deal (ie. abstracting hard coded GPIO values). Below are some details 
> of the 
> patch and specific areas where I would appreciate feedback on how to make 
> this better:
>  
> hw/arm/aspeed.c: 
> I am not sure where the best place is to instantiate the spi_gpio besides the
> aspeed_machine_init. Could we add the ability to instantiate it on the 
> command line?

Yes, hypothetically I think it would be something like this:

-device spi-gpio,miso=aspeed.gpio.gpioX4,mosi=aspeed.gpio.gpioX5,id=foo
-device n25q00,bus=foo,drive=bar
-drive file=bar.mtd,format=raw,if=mtd,id=bar

Something like that? I haven't really looked into the details. I think
it requires work in several other places though.

> 
> m25p80_transfer8_ex in hw/block/m25p80.c: 
> Existing SPI model assumed that the output byte is fully formed, can be 
> passed to 
> the SPI device, and the input byte can be returned, all in one operation. 
> With 
> SPI-GPIO we don't have the input byte until all 8 bits of the output have 
> been 
> shifted out, so we have to prime the MISO with bogus values (0xFF).

Perhaps the Aspeed FMC model needs to support asynchronous transfers?
(Is the M25P80 model not asynchronous already?) I'm still skeptical that
the m25p80_transfer8_ex solution is appropriate.

> 
> MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime 
> value
> of the gpio for input bits to prevent bugs with caching the mosi value. It 
> was discovered 
> during testing that when using the mosi pin as the input pin, the mosi value 
> was not 
> being updated due to a kernel and aspeed_gpio model optimization. Thus, here 
> we are 
> reading the value directly from the gpio controller instead of waiting for 
> the push.
> 
> I realize there are Aspeed specifics in the spi_gpio model. To make this 
> extensible, 
> is it preferred to make this into a base class and have our Aspeed SPI GPIO 
> extend 
> this or we could set up params to pass in the constructor?

Actually, I would hope that there won't be any inheritance here. The
kernel driver doesn't have some kind of inheritance implementation, for
example.

> 
> Thanks for your review and any direction here would be helpful :) 
> 
> Iris Chen (3):
>   hw: m25p80: add prereading ability in transfer8
>   hw: spi_gpio: add spi gpio model
>   hw: aspeed: hook up the spi gpio model
> 
>  hw/arm/Kconfig|   1 +
>  hw/arm/aspeed.c   |   5 ++
>  hw/block/m25p80.c |  29 ++-
>  hw/ssi/Kconfig|   4 +
>  hw/ssi/meson.build|   1 +
>  hw/ssi/spi_gpio.c | 166 ++
>  hw/ssi/ssi.c  |   4 -
>  include/hw/ssi/spi_gpio.h |  38 +
>  include/hw/ssi/ssi.h  |   5 ++
>  9 files changed, 248 insertions(+), 5 deletions(-)
>  create mode 100644 hw/ssi/spi_gpio.c
>  create mode 100644 include/hw/ssi/spi_gpio.h
> 
> -- 
> 2.30.2
>

RE: [PATCH] configure: pass correct cflags to container-based cross compilers

2022-07-28 Thread Taylor Simpson



> -Original Message-
> From: Richard Henderson 
> Sent: Thursday, July 28, 2022 5:11 PM
> To: Paolo Bonzini ; qemu-devel@nongnu.org
> Cc: Taylor Simpson 
> Subject: Re: [PATCH] configure: pass correct cflags to container-based cross
> compilers
> 
> On 7/28/22 15:22, Paolo Bonzini wrote:
> > probe_target_compiler returns nonempty $target_cc for installed
> > toolchains and $container_cross_cc for container-based toolchains.  In
> > both cases however the flags (coming from
> > $cross_cc_cflags_${target_arch}) must be in $target_cflags.
> >
> > Therefore, do not clear them prior to returning from
> probe_target_compiler.
> >
> > Reported-by: Taylor Simpson 
> > Fixes: 92e288fcfb ("build: try both native and cross compilers",
> > 2022-07-08)
> > Signed-off-by: Paolo Bonzini 
> > ---
> >   configure | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/configure b/configure
> > index c4c02b8438..72ab03f11a 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2173,7 +2173,6 @@ probe_target_compiler() {
> >   build_static=
> >   target_cc=
> >   target_ccas=
> > -target_cflags=
> >   target_ar=
> >   target_as=
> >   target_ld=
> 
> Reviewed-by: Richard Henderson 
> 
> r~

Reviewed-by: Taylor Simpson

[python-qemu-qmp MR #13] Sphinx: improve version detection for SDists

2022-07-28 Thread GitLab Bot

Author: John Snow - https://gitlab.com/jsnow
Merge Request: 
https://gitlab.com/qemu-project/python-qemu-qmp/-/merge_requests/13
... from: jsnow/python-qemu-qmp:doc-version-fix
... into: qemu-project/python-qemu-qmp:main

When building docs from SDist files, we won't have git metadata
available. Use any tools we may have at our disposal to determine the
package version when possible in these cases.

Note: this patch avoids trying to blindly import "qemu.qmp" in just such
a case that the user's environment already has such a package installed:
we want to query specifically the version from the unpackaged source
files.

Fixes #27

---

This is an automated message. This bot will only relay the creation of new merge
requests and will not relay review comments, new revisions, or concluded merges.
Please follow the GitLab link to participate in review.

[RFC 2/3] hw: spi_gpio: add spi gpio model

2022-07-28 Thread Iris Chen

Signed-off-by: Iris Chen 
---
 hw/ssi/spi_gpio.c | 166 ++
 include/hw/ssi/spi_gpio.h |  38 +
 2 files changed, 204 insertions(+)
 create mode 100644 hw/ssi/spi_gpio.c
 create mode 100644 include/hw/ssi/spi_gpio.h

diff --git a/hw/ssi/spi_gpio.c b/hw/ssi/spi_gpio.c
new file mode 100644
index 00..1e99c55933
--- /dev/null
+++ b/hw/ssi/spi_gpio.c
@@ -0,0 +1,166 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
+ *
+ * This code is licensed under the GPL version 2 or later. See the COPYING
+ * file in the top-level directory.
+ */
+
+#include "hw/ssi/spi_gpio.h"
+#include "hw/irq.h"
+
+#define SPI_CPHA BIT(0) /* clock phase (1 = SPI_CLOCK_PHASE_SECOND) */
+#define SPI_CPOL BIT(1) /* clock polarity (1 = SPI_POLARITY_HIGH) */
+
+static void do_leading_edge(SpiGpioState *s)
+{
+if (!s->CPHA) {
+s->input_bits |= object_property_get_bool(OBJECT(s->controller_state),
+  "gpioX4", NULL);
+/*
+ * According to SPI protocol:
+ * CPHA=0 leading half clock cycle is sampling phase
+ * We technically should not drive out miso
+ * However, when the kernel bitbang driver is setting the clk pin,
+ * it will overwrite the miso value, so we are driving out miso in
+ * the sampling half clock cycle as well to workaround this issue
+ */
+s->miso = !!(s->output_bits & 0x80);
+object_property_set_bool(OBJECT(s->controller_state), "gpioX5", 
s->miso,
+ NULL);
+}
+}
+
+static void do_trailing_edge(SpiGpioState *s)
+{
+if (s->CPHA) {
+s->input_bits |= object_property_get_bool(OBJECT(s->controller_state),
+  "gpioX4", NULL);
+/*
+ * According to SPI protocol:
+ * CPHA=1 trailing half clock cycle is sampling phase
+ * We technically should not drive out miso
+ * However, when the kernel bitbang driver is setting the clk pin,
+ * it will overwrite the miso value, so we are driving out miso in
+ * the sampling half clock cycle as well to workaround this issue
+ */
+s->miso = !!(s->output_bits & 0x80);
+object_property_set_bool(OBJECT(s->controller_state), "gpioX5", 
s->miso,
+ NULL);
+}
+}
+
+static void cs_set_level(void *opaque, int n, int level)
+{
+SpiGpioState *s = SPI_GPIO(opaque);
+s->cs = !!level;
+
+/* relay the CS value to the CS output pin */
+qemu_set_irq(s->cs_output_pin, s->cs);
+
+s->miso = !!(s->output_bits & 0x80);
+object_property_set_bool(OBJECT(s->controller_state),
+ "gpioX5", s->miso, NULL);
+
+s->clk = !!(s->mode & SPI_CPOL);
+}
+
+static void clk_set_level(void *opaque, int n, int level)
+{
+SpiGpioState *s = SPI_GPIO(opaque);
+
+bool cur = !!level;
+
+/* CS# is high/not selected, do nothing */
+if (s->cs) {
+return;
+}
+
+/* When the lock has not changed, do nothing */
+if (s->clk == cur) {
+return;
+}
+
+s->clk = cur;
+
+/* Leading edge */
+if (s->clk != s->CIDLE) {
+do_leading_edge(s);
+}
+
+/* Trailing edge */
+if (s->clk == s->CIDLE) {
+do_trailing_edge(s);
+s->clk_counter++;
+
+/*
+ * Deliver the input to and
+ * get the next output byte
+ * from the SPI device
+ */
+if (s->clk_counter == 8) {
+s->output_bits = ssi_transfer(s->spi, s->input_bits);
+s->clk_counter = 0;
+s->input_bits = 0;
+ } else {
+s->input_bits <<= 1;
+s->output_bits <<= 1;
+ }
+}
+}
+
+static void spi_gpio_realize(DeviceState *dev, Error **errp)
+{
+SpiGpioState *s = SPI_GPIO(dev);
+
+s->spi = ssi_create_bus(dev, "spi");
+s->spi->preread = true;
+
+s->mode = 0;
+s->clk_counter = 0;
+
+s->cs = true;
+s->clk = true;
+
+/* Assuming the first output byte is 0 */
+s->output_bits = 0;
+s->CIDLE = !!(s->mode & SPI_CPOL);
+s->CPHA = !!(s->mode & SPI_CPHA);
+
+/* init the input GPIO lines */
+/* SPI_CS_in connects to the Aspeed GPIO */
+qdev_init_gpio_in_named(dev, cs_set_level, "SPI_CS_in", 1);
+qdev_init_gpio_in_named(dev, clk_set_level, "SPI_CLK", 1);
+
+/* init the output GPIO lines */
+/* SPI_CS_out connects to the SSI_GPIO_CS */
+qdev_init_gpio_out_named(dev, >cs_output_pin, "SPI_CS_out", 1);
+
+qdev_connect_gpio_out_named(s->controller_state, "sysbus-irq",
+AST_GPIO_IRQ_X0_NUM, qdev_get_gpio_in_named(
+DEVICE(s), "SPI_CS_in", 0));
+qdev_connect_gpio_out_named(s->controller_state, "sysbus-irq",
+AST_GPIO_IRQ_X3_NUM, qdev_get_gpio_in_named(
+

[RFC 1/3] hw: m25p80: add prereading ability in transfer8

2022-07-28 Thread Iris Chen

With SPI-GPIO we don't have the input bits until
all 8 bits of the output have been shifted out,
so we have to prime the MISO with bogus values (0xFF).

Signed-off-by: Iris Chen 
---
 hw/block/m25p80.c| 29 -
 hw/ssi/ssi.c |  4 
 include/hw/ssi/ssi.h |  5 +
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index a8d2519141..9b26bb96e5 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1462,7 +1462,7 @@ static int m25p80_cs(SSIPeripheral *ss, bool select)
 return 0;
 }
 
-static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx)
+static uint32_t m25p80_transfer8_ex(SSIPeripheral *ss, uint32_t tx)
 {
 Flash *s = M25P80(ss);
 uint32_t r = 0;
@@ -1548,6 +1548,33 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, 
uint32_t tx)
 return r;
 }
 
+/*
+ * Pre-reading logic for m25p80_transfer8:
+ * The existing SPI model assumes the output byte is fully formed,
+ * can be passed to the SPI device, and the input byte can be returned,
+ * all in one operation. With SPI-GPIO, we don't have the input byte
+ * until all 8 bits of the output have been shifted out, so we have
+ * to prime the MISO with bogus values (0xFF).
+ */
+static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx)
+{
+Flash *s = M25P80(ss);
+SSIBus *ssibus = (SSIBus *)qdev_get_parent_bus(DEVICE(s));
+
+uint8_t prev_state = s->state;
+uint32_t r = m25p80_transfer8_ex(ss, tx);
+uint8_t curr_state = s->state;
+
+if (ssibus->preread &&
+   /* cmd state has changed into DATA reading state */
+   (!(prev_state == STATE_READ || prev_state == STATE_READING_DATA) &&
+   (curr_state == STATE_READ || curr_state == STATE_READING_DATA))) {
+r = m25p80_transfer8_ex(ss, 0xFF);
+}
+
+return r;
+}
+
 static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int 
level)
 {
 Flash *s = M25P80(opaque);
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 003931fb50..21570fe25f 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -19,10 +19,6 @@
 #include "qapi/error.h"
 #include "qom/object.h"
 
-struct SSIBus {
-BusState parent_obj;
-};
-
 #define TYPE_SSI_BUS "SSI"
 OBJECT_DECLARE_SIMPLE_TYPE(SSIBus, SSI_BUS)
 
diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index f411858ab0..e54073d822 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -30,6 +30,11 @@ enum SSICSMode {
 SSI_CS_HIGH,
 };
 
+struct SSIBus {
+BusState parent_obj;
+bool preread;
+};
+
 /* Peripherals.  */
 struct SSIPeripheralClass {
 DeviceClass parent_class;
-- 
2.30.2

[RFC 0/3] Add Generic SPI GPIO model

2022-07-28 Thread Iris Chen

Hey everyone,

I have been working on a project to add support for SPI-based TPMs in QEMU.
Currently, most of our vboot platforms using a SPI-based TPM use the Linux 
SPI-GPIO driver to "bit-bang" the SPI protocol. This is because the Aspeed 
SPI controller (modeled in QEMU under hw/ssi/aspeed_smc.c) has an 
implementation 
deficiency that prevents bi-directional operations. Thus, in order to connect 
a TPM to this bus, my patch implements a QEMU SPI-GPIO driver (as the QEMU
counterpart of the Linux SPI-GPIO driver).

As we use SPI-based TPMs on many of our BMCs for the secure-boot 
implementation,  
I have already tested this implementation locally with our Yosemite-v3.5 
platform 
and Facebook-OpenBMC. This model was tested by connecting a generic SPI-NOR 
(m25p80 
for example) to the Yosemite-v3.5 SPI bus containing the TPM.

This patch is an RFC because I have several questions about design. Although the
model is working, I understand there are many areas where the design decision
is not deal (ie. abstracting hard coded GPIO values). Below are some details of 
the 
patch and specific areas where I would appreciate feedback on how to make this 
better:
 
hw/arm/aspeed.c: 
I am not sure where the best place is to instantiate the spi_gpio besides the
aspeed_machine_init. Could we add the ability to instantiate it on the command 
line?

m25p80_transfer8_ex in hw/block/m25p80.c: 
Existing SPI model assumed that the output byte is fully formed, can be passed 
to 
the SPI device, and the input byte can be returned, all in one operation. With 
SPI-GPIO we don't have the input byte until all 8 bits of the output have been 
shifted out, so we have to prime the MISO with bogus values (0xFF).

MOSI pin in spi_gpio: the mosi pin is not included and we poll the realtime 
value
of the gpio for input bits to prevent bugs with caching the mosi value. It was 
discovered 
during testing that when using the mosi pin as the input pin, the mosi value 
was not 
being updated due to a kernel and aspeed_gpio model optimization. Thus, here we 
are 
reading the value directly from the gpio controller instead of waiting for the 
push.

I realize there are Aspeed specifics in the spi_gpio model. To make this 
extensible, 
is it preferred to make this into a base class and have our Aspeed SPI GPIO 
extend 
this or we could set up params to pass in the constructor?

Thanks for your review and any direction here would be helpful :) 

Iris Chen (3):
  hw: m25p80: add prereading ability in transfer8
  hw: spi_gpio: add spi gpio model
  hw: aspeed: hook up the spi gpio model

 hw/arm/Kconfig|   1 +
 hw/arm/aspeed.c   |   5 ++
 hw/block/m25p80.c |  29 ++-
 hw/ssi/Kconfig|   4 +
 hw/ssi/meson.build|   1 +
 hw/ssi/spi_gpio.c | 166 ++
 hw/ssi/ssi.c  |   4 -
 include/hw/ssi/spi_gpio.h |  38 +
 include/hw/ssi/ssi.h  |   5 ++
 9 files changed, 248 insertions(+), 5 deletions(-)
 create mode 100644 hw/ssi/spi_gpio.c
 create mode 100644 include/hw/ssi/spi_gpio.h

-- 
2.30.2

Re: [PATCH] configure: pass correct cflags to container-based cross compilers


On 7/28/22 15:22, Paolo Bonzini wrote:

probe_target_compiler returns nonempty $target_cc for installed toolchains
and $container_cross_cc for container-based toolchains.  In both cases
however the flags (coming from $cross_cc_cflags_${target_arch}) must be
in $target_cflags.

Therefore, do not clear them prior to returning from probe_target_compiler.

Reported-by: Taylor Simpson 
Fixes: 92e288fcfb ("build: try both native and cross compilers", 2022-07-08)
Signed-off-by: Paolo Bonzini 
---
  configure | 1 -
  1 file changed, 1 deletion(-)

diff --git a/configure b/configure
index c4c02b8438..72ab03f11a 100755
--- a/configure
+++ b/configure
@@ -2173,7 +2173,6 @@ probe_target_compiler() {
  build_static=
  target_cc=
  target_ccas=
-target_cflags=
  target_ar=
  target_as=
  target_ld=


Reviewed-by: Richard Henderson 

r~

[PATCH] configure: pass correct cflags to container-based cross compilers

probe_target_compiler returns nonempty $target_cc for installed toolchains
and $container_cross_cc for container-based toolchains.  In both cases
however the flags (coming from $cross_cc_cflags_${target_arch}) must be
in $target_cflags.

Therefore, do not clear them prior to returning from probe_target_compiler.

Reported-by: Taylor Simpson 
Fixes: 92e288fcfb ("build: try both native and cross compilers", 2022-07-08)
Signed-off-by: Paolo Bonzini 
---
 configure | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure b/configure
index c4c02b8438..72ab03f11a 100755
--- a/configure
+++ b/configure
@@ -2173,7 +2173,6 @@ probe_target_compiler() {
 build_static=
 target_cc=
 target_ccas=
-target_cflags=
 target_ar=
 target_as=
 target_ld=
-- 
2.36.1

Re: [PATCH] Hexagon (tests/tcg/hexagon) add compiler options to EXTRA_CFLAGS


On 7/26/22 21:17, Taylor Simpson wrote:

The cross_cc_cflags_hexagon in configure are not getting passed to
the Hexagon cross compiler.  Set EXTRA_CFLAGS in
tests/tcg/hexagon/Makefile.target.

Suggested-by: Richard Henderson
Signed-off-by: Taylor Simpson
---


The bug applies to all targets, I am going to post a patch to fix it.

Paolo

Re: Question to mem-path support at QEMU for Xen

2022-07-28 Thread Stefano Stabellini

On Thu, 28 Jul 2022, Igor Mammedov wrote:
> On Thu, 28 Jul 2022 15:17:49 +0800
> Huang Rui  wrote:
> 
> > Hi Igor,
> > 
> > Appreciate you for the reply!
> > 
> > On Wed, Jul 27, 2022 at 04:19:30PM +0800, Igor Mammedov wrote:
> > > On Tue, 26 Jul 2022 15:27:07 +0800
> > > Huang Rui  wrote:
> > >   
> > > > Hi Anthony and other Qemu/Xen guys,
> > > > 
> > > > We are trying to enable venus on Xen virtualization platform. And we 
> > > > would
> > > > like to use the backend memory with memory-backend-memfd,id=mem1,size=4G
> > > > options on QEMU, however, the QEMU will tell us the "-mem-path" is not
> > > > supported with Xen. I verified the same function on KVM.
> > > > 
> > > > qemu-system-i386: -mem-path not supported with Xen
> > > > 
> > > > So may I know whether Xen has any limitation that support
> > > > memory-backend-memfd in QEMU or just implementation is not done yet?  
> > > 
> > > Currently Xen doesn't use guest RAM allocation the way the rest of
> > > accelerators do. (it has hacks in memory_region/ramblock API,
> > > that divert RAM allocation calls to Xen specific API)  
> > 
> > I am new for Xen and QEMU, we are working on GPU. We would like to have a
> > piece of backend memroy like video memory for VirtIO GPU to support guest
> > VM Mesa Vulkan (Venus). Do you mean we can the memory_region/ramblock APIs
> > to work around it?
> > 
> > > 
> > > The sane way would extend Xen to accept RAM regions (whatever they are
> > > ram or fd based) QEMU allocates instead of going its own way. This way
> > > it could reuse all memory backends that QEMU provides for the rest of
> > > the non-Xen world. (not to mention that we could drop non trivial
> > > Xen hacks so that guest RAM handling would be consistent with other
> > > accelerators)
> > >   
> > 
> > May I know what do you mean by "going its own way"? This sounds good, could
> > you please elaborate on how can we implement this? We would like to give a
> > try to address the problem on Xen. Would you mind to point somewhere that I
> > can learn and understand the RAM region. Very happy to see your
> > suggestions!
> 
> see for example see ram_block_add(), if Xen could be persuaded to use memory
> allocated by '!xen_enabled()' branch then it's likely file base backends
> would also become usable.
> 
> Whether it is possible for Xen or not I don't know,
> I guess CCed Xen folks will suggest something useful.

Hi Igor, Huang,

I think there is room for improvement in the way Xen support in QEMU is
implemented, especially because it predates support for other
accelerators in QEMU. I am happy to help come up with a good idea or two
on how to do it better.

At the same time it is not trivial. The way Xen works with QEMU is that
Xen is the one allocating memory for the VM. Keep in mind that in a Xen
setup, the VM where QEMU is running is just one of the many VMs in the
system (even if it is dom0) and doesn't have ownership over the entire
memory. It is also possible to run QEMU in a regular guest for security
benefits.

Given that, Xen is typically the one allocating memory for the VM and by
the time QEMU is started the main memory is already allocated.

That is the reason why normally ram_block_add() is implemented in a
special way for Xen using xen_ram_alloc. In most cases, there is no need
to allocate any memory at all.

However, it is also possible for QEMU to allocate memory for the VM. It
is done by QEMU issuing a hypercall to Xen. See for instance the call to
xc_domain_populate_physmap_exact. As you can see from the implementation
of xen_ram_alloc, it is already used for things that are not normal
memory (see the check mr == _memory). So if you want to allocate
memory for the VM, you could use xc_domain_populate_physmap_exact.

Another thing to note is that Xen can connect to multiple emulators at
the same time. Each emulator (each instance of QEMU, or other emulators)
uses a hypercall to connect to Xen and register a PCI device or memory
range that it is emulating. Each emulator is called "IOREQ server" in
Xen terminology because it is handling "IO emulation REQuests" from Xen.
The IOREQ interface is very simple. each IOREQ is defined a ioreq_t
struct, see: xen/include/public/hvm/ioreq.h:ioreq_t

So if you are looking to connect a secondary emulator to QEMU, a
different, more efficient, way to solve the problem on Xen would be to
connect the secondary emulator directly to Xen. Instead of:

  Xen -> QEMU -> secondary emulator

You would do:

  Xen ---> QEMU
   |
   +-> secondary emulator

The second approach is much faster in terms of latency because you are
going to skip a step in the notification chain. See how QEMU calls
xen_register_ioreq to register itself with Xen.

Re: [PATCH for-7.1?] kvm: don't use perror() without useful errno

Queued, thanks.

Paolo

Re: [PATCH] configure: Fix ppc container_cross_cc substitution

Queued, thanks.

Paolo

Re: [PATCH] ppc: Remove redundant macro MSR_BOOK3S_MASK.





On 7/28/22 17:11, Yonggang Luo wrote:

Signed-off-by: Yonggang Luo 
---
  target/ppc/excp_helper.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index cb752b184a..7550aafed6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2015,7 +2015,6 @@ void helper_rfi(CPUPPCState *env)
  do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xul);
  }
  
-#define MSR_BOOK3S_MASK


The tag was introduced by a2e71b28e832 ("ppc: Fix rfi/rfid/hrfi/... emulation").
Even back then it wasn't being used.


Reviewed-by: Daniel Henrique Barboza 


Laurent, I believe this is qemu-trivial material. Let me know if you want
me to get it via the ppc tree instead.


Daniel



  #if defined(TARGET_PPC64)
  void helper_rfid(CPUPPCState *env)
  {

Re: [PULL 0/3] ppc queue





On 7/28/22 17:18, Richard Henderson wrote:

On 7/28/22 09:55, Daniel Henrique Barboza wrote:

   https://gitlab.com/danielhb/qemu.git  pull-ppc-20220728


fatal: couldn't find remote ref pull-ppc-20220728


Did you forget to push the tag to gitlab?


I guess I mistyped the credentials when running make-pullreq.sh and the
tag wasn't pushed. Can you try again? It is pushed now:

https://gitlab.com/danielhb/qemu/-/commits/pull-ppc-20220728


Thanks,


Daniel




r~

Re: [PATCH] Hexagon (tests/tcg/hexagon) add compiler options to EXTRA_CFLAGS


On 7/26/22 12:17, Taylor Simpson wrote:

The cross_cc_cflags_hexagon in configure are not getting passed to
the Hexagon cross compiler.  Set EXTRA_CFLAGS in
tests/tcg/hexagon/Makefile.target.

Suggested-by: Richard Henderson 
Signed-off-by: Taylor Simpson 
---
  tests/tcg/hexagon/Makefile.target | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Richard Henderson 


r~



diff --git a/tests/tcg/hexagon/Makefile.target 
b/tests/tcg/hexagon/Makefile.target
index 23b9870534..627bf58fe6 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -20,6 +20,7 @@ EXTRA_RUNS =
  
  CFLAGS += -Wno-incompatible-pointer-types -Wno-undefined-internal

  CFLAGS += -fno-unroll-loops
+EXTRA_CFLAGS += -mv67 -O2
  
  HEX_SRC=$(SRC_PATH)/tests/tcg/hexagon

  VPATH += $(HEX_SRC)

Re: [PULL 0/3] ppc queue


On 7/28/22 09:55, Daniel Henrique Barboza wrote:

   https://gitlab.com/danielhb/qemu.git  pull-ppc-20220728


fatal: couldn't find remote ref pull-ppc-20220728


Did you forget to push the tag to gitlab?


r~

[PATCH] ppc: Remove redundant macro MSR_BOOK3S_MASK.

2022-07-28 Thread Yonggang Luo

Signed-off-by: Yonggang Luo 
---
 target/ppc/excp_helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index cb752b184a..7550aafed6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2015,7 +2015,6 @@ void helper_rfi(CPUPPCState *env)
 do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xul);
 }
 
-#define MSR_BOOK3S_MASK
 #if defined(TARGET_PPC64)
 void helper_rfid(CPUPPCState *env)
 {
-- 
2.36.1.windows.1

Re: [PATCH 1/1] block: add missed block_acct_setup with new block device init procedure

2022-07-28 Thread Denis V. Lunev


On 28.07.2022 16:42, Vladimir Sementsov-Ogievskiy wrote:

On 7/11/22 14:07, Denis V. Lunev wrote:

Commit 5f76a7aac156ca75680dad5df4a385fd0b58f6b1 is looking harmless from
the first glance, but it has changed things a lot. 'libvirt' uses it to
detect that it should follow new initialization way and this changes
things considerably. With this procedure followed, blockdev_init() is
not called anymore and thus block_acct_setup() helper is not called.


I'm not sure that 5f76a7aac156ca is really the corner stone.. But yes,
libvirt moved to "blockdev era", which means that we don't use old 
-drive,
instead block nodes are created by -blockdev / qmp: blockdev-add, and 
attached

to block devices by node-name.


git bisected, thus I am sure here


And if I understand correctly blockdev_init() is called only on -drive 
path.


I have some questions:

1. After this patch, don't we call block_acct_setup() twice on old 
path with -drive? That seems safe as block_acct_setup just assign 
fields of BlockAcctStats.. But that's doesn't look good.



hmmm

2. Do we really need these options? Could we instead just enable 
accounting invalid and failed ops unconditionally? I doubt that 
someone will learn that these new options appeared and will use them 
to disable the failed/invalid accounting again.



I can move assignment of these fields to true int
block_acct_init() and forget about "configurable"
items in new path. I do not think that somebody
ever has these options set.

The real question in this patch is that this initialization
was a precondition for old good "long IO" report
configuration, which should be "enableable".

But  we could move this option to "tracked request"
layer only and this will solve my puzzle. So, I'll move
"long IO report" to tracked request level only and will
create an option for it on bdrv_ level and will avoid
it on blk_ accounting.

What do you think?

Den







This means in particular that defaults for block accounting statistics
are changed and account_invalid/account_failed are actually initialized
as false instead of true originally.

This commit changes things to match original world. It adds
account_invalid/account_failed properties to BlockConf and setups them
accordingly.

Signed-off-by: Denis V. Lunev 
CC: Peter Krempa 
CC: Markus Armbruster 
CC: John Snow 
CC: Kevin Wolf 
CC: Hanna Reitz 
---
  hw/block/block.c   |  2 +
  include/hw/block/block.h   |  7 +++-
  tests/qemu-iotests/172.out | 76 ++
  3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 25f45df723..53b100cdc3 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -205,6 +205,8 @@ bool blkconf_apply_backend_options(BlockConf 
*conf, bool readonly,

 BlockdevOnError rerror;

  blk_set_enable_write_cache(blk, wce);
  blk_set_on_error(blk, rerror, werror);
  +    block_acct_setup(blk_get_stats(blk), conf->account_invalid,
+ conf->account_failed);
  return true;
  }
  diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 5902c0440a..ffd439fc83 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -31,6 +31,7 @@ typedef struct BlockConf {
  uint32_t lcyls, lheads, lsecs;
  OnOffAuto wce;
  bool share_rw;
+    bool account_invalid, account_failed;
  BlockdevOnError rerror;
  BlockdevOnError werror;
  } BlockConf;
@@ -61,7 +62,11 @@ static inline unsigned int 
get_physical_block_exp(BlockConf *conf)
 _conf.discard_granularity, 
-1),  \
  DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, 
_conf.wce,   \

ON_OFF_AUTO_AUTO),  \
-    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
+    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, 
false),    \
+    DEFINE_PROP_BOOL("account-invalid", 
_state, \
+ _conf.account_invalid, 
true),  \
+    DEFINE_PROP_BOOL("account-failed", 
_state,  \

+ _conf.account_failed, true)
    #define DEFINE_BLOCK_PROPERTIES(_state, 
_conf)  \
  DEFINE_PROP_DRIVE("drive", _state, 
_conf.blk),  \

diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index 9479b92185..a6c451e098 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -28,6 +28,8 @@ Formatting 'TEST_DIR/t.IMGFMT.3', fmt=IMGFMT 
size=737280

  discard_granularity = 4294967295 (4 GiB)
  write-cache = "auto"
  share-rw = false
+    account-invalid = true
+    account-failed = true
  drive-type = "288"
    @@ -55,6 +57,8 @@ Testing: -fda TEST_DIR/t.qcow2
  discard_granularity = 4294967295 (4 GiB)
  write-cache = "auto"
  share-rw = false
+

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

2022-07-28 Thread Ani Sinha




On Thu, 28 Jul 2022, Peter Maydell wrote:

> On Thu, 28 Jul 2022 at 12:50, Igor Mammedov  wrote:
> >
> > QEMU crashes trying to save VMSTATE when only MIPS target are compiled in
> >   $ qemu-system-mips -monitor stdio
> >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> >   Segmentation fault (core dumped)
> >
> > It happens due to PIIX4_PM trying to parse hotplug vmstate structures
> > which are valid only for x86 and not for MIPS (as it requires ACPI
> > tables support which is not existent for ithe later)
> >
> > Issue was probably exposed by trying to cleanup/compile out unused
> > ACPI bits from MIPS target (but forgetting about migration bits).
> >
> > Disable compiled out features using compat properties as the least
> > risky way to deal with issue.
> >
> > Signed-off-by: Igor Mammedov 
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/995


Reviewed-by: Ani Sinha 

>
> > ---
> > PS:
> > another approach could be setting defaults to disabled state and
> > enabling them using compat props on PC machines (which is more
> > code to deal with => more risky) or continue with PIIX4_PM
> > refactoring to split x86-shism out (which I'm not really
> > interested in due to risk of regressions for not much of
> > benefit)
> > ---
> >  hw/mips/malta.c | 9 +
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index 7a0ec513b0..0e932988e0 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = {
> >  .instance_init = mips_malta_instance_init,
> >  };
> >
> > +GlobalProperty malta_compat[] = {
> > +{ "PIIX4_PM", "memory-hotplug-support", "off" },
> > +{ "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" },
> > +{ "PIIX4_PM", "acpi-root-pci-hotplug", "off" },
> > +{ "PIIX4_PM", "x-not-migrate-acpi-index", "true" },
> > +};
>
> Is there an easy way to assert in hw/acpi/piix4.c that if
> CONFIG_ACPI_PCIHP was not set then the board has initialized
> all these properties to the don't-use-hotplug state ?
> That would be a guard against similar bugs (though I suppose
> we probably aren't likely to add new piix4 boards...)
>
> > +const size_t malta_compat_len = G_N_ELEMENTS(malta_compat);
> > +
> >  static void mips_malta_machine_init(MachineClass *mc)
> >  {
> >  mc->desc = "MIPS Malta Core LV";
> > @@ -1455,6 +1463,7 @@ static void mips_malta_machine_init(MachineClass *mc)
> >  mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
> >  #endif
> >  mc->default_ram_id = "mips_malta.ram";
> > +compat_props_add(mc->compat_props, malta_compat, malta_compat_len);
> >  }
> >
> >  DEFINE_MACHINE("malta", mips_malta_machine_init)
> > --
> > 2.31.1
>
> thanks
> -- PMM
>

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

2022-07-28 Thread Ani Sinha




On Thu, 28 Jul 2022, Peter Maydell wrote:

> On Thu, 28 Jul 2022 at 15:44, Dr. David Alan Gilbert
>  wrote:

> > Isn't the problem partially due to a 'stub' vmsd which isn't terminated?
>
> Yes, but setting these properties causes that vmsd
> (vmstate_acpi_pcihp_pci_status) to not be used:
>
>  * it is used only in VMSTATE_PCI_HOTPLUG()
>  * that macro is used only in hw/acpi/ich9.c (not relevant here) and
>hw/acpi/piix4.c
>  * in piix4.c it is invoked passing it the test functions
>vmstate_test_use_acpi_hotplug_bridge and
>vmstate_test_migrate_acpi_index
>  * setting the properties on the device as this patch does
>causes those test functions to return false, so the
>vmstate_acpi_pcihp_pci_status is never examined

I believe this happens in vmstate_save_state_v() in this condition
checking:

  while (field->name) {
if ((field->field_exists &&
 field->field_exists(opaque, version_id)) ||
(!field->field_exists &&
 field->version_id <= version_id)) {

[PATCH] configure: Fix ppc container_cross_cc substitution

When moving this code out of probe_target_compiler(), we failed to adjust
the variable in which the target is located, resulting in e.g.
powerpc64-linux-user-linux-gnu-gcc-10

Fixes: cd362defbbd ("tests/tcg: merge configure.sh back into main configure 
script")
Signed-off-by: Richard Henderson 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 2c19329d58..c4c02b8438 100755
--- a/configure
+++ b/configure
@@ -2028,7 +2028,7 @@ probe_target_compiler() {
 ;;
   ppc64|ppc64le)
 container_image=debian-powerpc-test-cross
-container_cross_prefix=powerpc${1#ppc}-linux-gnu-
+container_cross_prefix=powerpc${target_arch#ppc}-linux-gnu-
 container_cross_cc=${container_cross_prefix}gcc-10
 ;;
   riscv64)
-- 
2.34.1

Re: [PULL 0/2] riscv-to-apply queue


On 7/27/22 17:59, Alistair Francis wrote:

From: Alistair Francis 

The following changes since commit 7b17a1a841fc2336eba53afade9cadb14bd3dd9a:

   Update version for v7.1.0-rc0 release (2022-07-26 18:03:16 -0700)

are available in the Git repository at:

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

for you to fetch changes up to 54f218363052be210e77d2ada8c0c1e51b3ad6cd:

   hw/intc: sifive_plic: Fix multi-socket plic configuraiton (2022-07-28 
09:08:44 +1000)


Sixth RISC-V PR for QEMU 7.1

This is a PR to go in for RC1. It fixes a segfault that occurs
when using multiple sockets on the RISC-V virt board. It also
includes a small fix to allow both Zmmul and M extensions.

* Allow both Zmmul and M extension
* Fix multi-socket plic configuraiton


Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/7.1 as 
appropriate.


r~





Atish Patra (1):
   hw/intc: sifive_plic: Fix multi-socket plic configuraiton

Palmer Dabbelt (1):
   RISC-V: Allow both Zmmul and M

  hw/intc/sifive_plic.c | 4 ++--
  target/riscv/cpu.c| 5 -
  2 files changed, 2 insertions(+), 7 deletions(-)

[PATCH] hw/riscv: remove 'fdt' param from riscv_setup_rom_reset_vec()

The 'fdt' param is not being used in riscv_setup_rom_reset_vec().
Simplify the API by removing it. While we're at it, remove the redundant
'return' statement at the end of function.

Cc: Palmer Dabbelt 
Cc: Alistair Francis 
Cc: Bin Meng 
Cc: Vijai Kumar K 
Signed-off-by: Daniel Henrique Barboza 
---
 hw/riscv/boot.c| 4 +---
 hw/riscv/microchip_pfsoc.c | 2 +-
 hw/riscv/shakti_c.c| 3 +--
 hw/riscv/spike.c   | 2 +-
 hw/riscv/virt.c| 2 +-
 include/hw/riscv/boot.h| 2 +-
 6 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 06b4fc5ac3..1ae7596873 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -286,7 +286,7 @@ void riscv_setup_rom_reset_vec(MachineState *machine, 
RISCVHartArrayState *harts
hwaddr start_addr,
hwaddr rom_base, hwaddr rom_size,
uint64_t kernel_entry,
-   uint64_t fdt_load_addr, void *fdt)
+   uint64_t fdt_load_addr)
 {
 int i;
 uint32_t start_addr_hi32 = 0x;
@@ -326,8 +326,6 @@ void riscv_setup_rom_reset_vec(MachineState *machine, 
RISCVHartArrayState *harts
   rom_base, _space_memory);
 riscv_rom_copy_firmware_info(machine, rom_base, rom_size, 
sizeof(reset_vec),
  kernel_entry);
-
-return;
 }
 
 void riscv_setup_direct_kernel(hwaddr kernel_addr, hwaddr fdt_addr)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 10a5d0e501..7313153606 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -583,7 +583,7 @@ static void microchip_icicle_kit_machine_init(MachineState 
*machine)
 riscv_setup_rom_reset_vec(machine, >soc.u_cpus, firmware_load_addr,
   memmap[MICROCHIP_PFSOC_ENVM_DATA].base,
   memmap[MICROCHIP_PFSOC_ENVM_DATA].size,
-  kernel_entry, fdt_load_addr, machine->fdt);
+  kernel_entry, fdt_load_addr);
 }
 }
 
diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
index 90e2cf609f..e43cc9445c 100644
--- a/hw/riscv/shakti_c.c
+++ b/hw/riscv/shakti_c.c
@@ -66,8 +66,7 @@ static void shakti_c_machine_state_init(MachineState *mstate)
 riscv_setup_rom_reset_vec(mstate, >soc.cpus,
   shakti_c_memmap[SHAKTI_C_RAM].base,
   shakti_c_memmap[SHAKTI_C_ROM].base,
-  shakti_c_memmap[SHAKTI_C_ROM].size, 0, 0,
-  NULL);
+  shakti_c_memmap[SHAKTI_C_ROM].size, 0, 0);
 if (mstate->firmware) {
 riscv_load_firmware(mstate->firmware,
 shakti_c_memmap[SHAKTI_C_RAM].base,
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index e41b6aa9f0..5ba34543c8 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -308,7 +308,7 @@ static void spike_board_init(MachineState *machine)
 riscv_setup_rom_reset_vec(machine, >soc[0], memmap[SPIKE_DRAM].base,
   memmap[SPIKE_MROM].base,
   memmap[SPIKE_MROM].size, kernel_entry,
-  fdt_load_addr, s->fdt);
+  fdt_load_addr);
 
 /* initialize HTIF using symbols found in load_kernel */
 htif_mm_init(system_memory, mask_rom,
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index bc424dd2f5..2e9ed2628c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1299,7 +1299,7 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 riscv_setup_rom_reset_vec(machine, >soc[0], start_addr,
   virt_memmap[VIRT_MROM].base,
   virt_memmap[VIRT_MROM].size, kernel_entry,
-  fdt_load_addr, machine->fdt);
+  fdt_load_addr);
 
 /*
  * Only direct boot kernel is currently supported for KVM VM,
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index d2db29721a..a36f7618f5 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -51,7 +51,7 @@ void riscv_setup_rom_reset_vec(MachineState *machine, 
RISCVHartArrayState *harts
hwaddr saddr,
hwaddr rom_base, hwaddr rom_size,
uint64_t kernel_entry,
-   uint64_t fdt_load_addr, void *fdt);
+   uint64_t fdt_load_addr);
 void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
   hwaddr rom_size,
   uint32_t reset_vec_size,
-- 
2.36.1

Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

2022-07-28 Thread BALATON Zoltan


On Tue, 26 Jul 2022, Peter Maydell wrote:

This patchset is mainly trying to fix a problem that Coverity spotted
in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
is not correctly using the cpu_physical_memory_map() function.
While I was fixing that I noticed a second problem in this code,
where it doesn't have a fallback for when cpu_physical_memory_map()
says "I couldn't map that for you".

I've marked these patches as RFC, partly because I don't have any
guest that would exercise the code changes[*], and partly because
I don't have any documentation of the hardware to tell me how it
should behave, so patch 2 in particular has some FIXMEs. I also
notice that the code doesn't update any of the registers like the
count or source/base addresses when the DMA transfer happens, which
seems odd, but perhaps the real hardware does work like that.

I think we should probably take patch 1 (which is a fairly minimal
fix of the use-of-uninitialized-data problem), but patch 2 is a bit
more unfinished.

[*] The commit 3c409c1927efde2fc that added this code says it's used
by AmigaOS.)


AmigaOS still boots with these patches and I see no difference so

Tested-by: BALATON Zoltan 

(I did not check what parameters AmigaOS uses (could not find a simple 
trace option for that, may need to add some debug printfs to test that) so 
not sure if the added code was actually run or it still only uses the code 
path as before. Fixing the map length should show some effect but I don't 
see any.)


Regards,
BALATON Zoltan


thanks
-- PMM

Peter Maydell (2):
 hw/ppc/ppc440_uc: Initialize length passed to
   cpu_physical_memory_map()
 hw/ppc/ppc440_uc: Handle mapping failure in DMA engine

hw/ppc/ppc440_uc.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)

Re: [PATCH] linux-user: Do not treat madvise()'s advice as a bitmask

2022-07-28 Thread Laurent Vivier


Le 25/07/2022 à 15:41, Ilya Leoshkevich a écrit :

Advice is enum, not flags. Doing (advice & MADV_DONTNEED) also matches
e.g. MADV_MERGEABLE.

Signed-off-by: Ilya Leoshkevich 
---
  linux-user/mmap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 4e7a6be6ee..edceaca4a8 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -891,7 +891,7 @@ abi_long target_madvise(abi_ulong start, abi_ulong len_in, 
int advice)
   * anonymous mappings. In this case passthrough is safe, so do it.
   */
  mmap_lock();
-if ((advice & MADV_DONTNEED) &&
+if (advice == MADV_DONTNEED &&
  can_passthrough_madv_dontneed(start, end)) {
  ret = get_errno(madvise(g2h_untagged(start), len, MADV_DONTNEED));
  }


Fixes: 892a4f6a750a ("linux-user: Add partial support for MADV_DONTNEED")
Reviewed-by: Laurent Vivier

Re: [PATCH for-7.1] linux-user/flatload.c: Fix setting of image_info::end_code

2022-07-28 Thread Laurent Vivier


Le 28/07/2022 à 17:14, Peter Maydell a écrit :

The flatload loader sets the end_code field in the image_info struct
incorrectly, due to a typo.

This is a very long-standing bug (dating all the way back to when
the bFLT loader was added in 2006), but has gone unnoticed because
(a) most people don't use bFLT binaries
(b) we don't actually do anything with the end_code field, except
 print it in debugging traces and pass it to TCG plugins

Fix the typo.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1119
Signed-off-by: Peter Maydell 
---
  linux-user/flatload.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index e4c2f89a226..e99570ca182 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -808,7 +808,7 @@ int load_flt_binary(struct linux_binprm *bprm, struct 
image_info *info)
  
  /* Stash our initial stack pointer into the mm structure */

  info->start_code = libinfo[0].start_code;
-info->end_code = libinfo[0].start_code = libinfo[0].text_len;
+info->end_code = libinfo[0].start_code + libinfo[0].text_len;
  info->start_data = libinfo[0].start_data;
  info->end_data = libinfo[0].end_data;
  info->start_brk = libinfo[0].start_brk;


Applied to my linux-user-for-7.1 branch.

Thanks,
Laurent

Re: [PATCH for-7.1?] kvm: don't use perror() without useful errno


On 7/28/22 07:24, Cornelia Huck wrote:

perror() is designed to append the decoded errno value to a
string. This, however, only makes sense if we called something that
actually sets errno prior to that.

For the callers that check for split irqchip support that is not the
case, and we end up with confusing error messages that end in
"success". Use error_report() instead.

Signed-off-by: Cornelia Huck
---


Reviewed-by: Richard Henderson 


r~

Re: [PATCH for-7.1] linux-user/flatload.c: Fix setting of image_info::end_code


On 7/28/22 08:14, Peter Maydell wrote:

The flatload loader sets the end_code field in the image_info struct
incorrectly, due to a typo.

This is a very long-standing bug (dating all the way back to when
the bFLT loader was added in 2006), but has gone unnoticed because
(a) most people don't use bFLT binaries
(b) we don't actually do anything with the end_code field, except
 print it in debugging traces and pass it to TCG plugins

Fix the typo.

Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1119
Signed-off-by: Peter Maydell
---
  linux-user/flatload.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

2022-07-28 Thread Kevin Wolf

Am 28.07.2022 um 16:50 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf  wrote:
> >> >
> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> >> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> >> > > other interface types, too.
> >> > >
> >> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> >> > > EEPROM device.
> >> > >
> >> > > Do we want IF_OTHER?
> >> >
> >> > What would the semantics even be? Any block device that doesn't pick up
> >> > a different category may pick up IF_OTHER backends?
> >> >
> >> > It certainly feels like a strange interface to ask for "other" disk and
> >> > then getting as surprise what this other thing might be. It's
> >> > essentially the same as having an explicit '-device other', and I
> >> > suppose most people would find that strange.
> >> >
> >> > > If no, I guess we get to abuse IF_PFLASH some more.
> >> > >
> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> >> > > be worth the trouble, though.
> >> > >
> >> > > Thoughts?
> >> >
> >> > If the existing types aren't good enough (I don't have an opinion on
> >> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> >> > new one, not just "other".
> >> 
> >> I think the common thread is "this isn't what anybody actually thinks
> >> of as being a 'disk', but we would like to back it with a block device
> >> anyway". That can cover a fair range of possibilities...
> >
> > How confident are we that no board will ever have two devices of this
> > kind?
> >
> > As long as every board has at most one, if=other is a bad user interface
> > in terms of descriptiveness, but still more or less workable as long as
> > you know what it means for the specific board you use.
> >
> > But if you have more than one device, it becomes hard to predict which
> > device gets which backend - it depends on the initialisation order in
> > the code then,
> 
> Really?  Board code should use IF_OTHER devices just like it uses the
> other interface types, namely connecting each frontend device to a
> backend device with a well-known and fixed interface type and index (or
> bus and unit instead, where appropriate).
> 
> >and I'm pretty sure that this isn't something that should
> > have significance in external interfaces and therefore become a stable
> > API.
> 
> I agree that "implied by execution order" is a bad idea: commit
> 95fd260f0a "blockdev: Drop unused drive_get_next()".

Ah good, I was indeed thinking of something drive_get_next()-like.

In case the board works with explicit indices, the situation is not as
bad as I was afraid. It will certainly be workable (even if not obvious)
for any boards that have a fixed number of devices with block backends,
which should cover everything we're intending to cover here.

I still consider if=other a bad user interface because what it means is
completely opaque, but if that's okay for you in your board, who am I to
object.

(Of course, the real solution would be having a generic way to set qdev
properties for on-board devices. I'm not expecting that we're getting
this anytime soon, though.)

Kevin

[ANNOUNCE] QEMU 7.1.0-rc0 is now available

2022-07-28 Thread Michael Roth

Hello,

On behalf of the QEMU Team, I'd like to announce the availability of the
first release candidate for the QEMU 7.1 release. This release is meant
for testing purposes and should not be used in a production environment.

  http://download.qemu-project.org/qemu-7.1.0-rc0.tar.xz
  http://download.qemu-project.org/qemu-7.1.0-rc0.tar.xz.sig

You can help improve the quality of the QEMU 7.1 release by testing this
release and reporting bugs using our GitLab issue tracker:

  https://gitlab.com/qemu-project/qemu/-/issues

The release plan, as well a documented known issues for release
candidates, are available at:

  http://wiki.qemu.org/Planning/7.1

Please add entries to the ChangeLog for the 7.1 release below:

  http://wiki.qemu.org/ChangeLog/7.1

Thank you to everyone involved!

[PULL 2/3] hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

From: Peter Maydell 

In dcr_write_dma(), there is code that uses cpu_physical_memory_map()
to implement a DMA transfer.  That function takes a 'plen' argument,
which points to a hwaddr which is used for both input and output: the
caller must set it to the size of the range it wants to map, and on
return it is updated to the actual length mapped. The dcr_write_dma()
code fails to initialize rlen and wlen, so will end up mapping an
unpredictable amount of memory.

Initialize the length values correctly, and check that we managed to
map the entire range before using the fast-path memmove().

This was spotted by Coverity, which points out that we never
initialized the variables before using them.

Fixes: Coverity CID 1487137, 1487150
Signed-off-by: Peter Maydell 
Reviewed-by: Richard Henderson 
Message-Id: <20220726182341.1888115-2-peter.mayd...@linaro.org>
Signed-off-by: Daniel Henrique Barboza 
---
 hw/ppc/ppc440_uc.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index a1ecf6dd1c..11fdb88c22 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -904,14 +904,17 @@ static void dcr_write_dma(void *opaque, int dcrn, 
uint32_t val)
 int width, i, sidx, didx;
 uint8_t *rptr, *wptr;
 hwaddr rlen, wlen;
+hwaddr xferlen;
 
 sidx = didx = 0;
 width = 1 << ((val & DMA0_CR_PW) >> 25);
+xferlen = count * width;
+wlen = rlen = xferlen;
 rptr = cpu_physical_memory_map(dma->ch[chnl].sa, ,
false);
 wptr = cpu_physical_memory_map(dma->ch[chnl].da, ,
true);
-if (rptr && wptr) {
+if (rptr && rlen == xferlen && wptr && wlen == xferlen) {
 if (!(val & DMA0_CR_DEC) &&
 val & DMA0_CR_SAI && val & DMA0_CR_DAI) {
 /* optimise common case */
-- 
2.36.1

[PULL 3/3] target/ppc: Implement new wait variants

From: Nicholas Piggin 

ISA v2.06 adds new variations of wait, specified by the WC field. These
are not all compatible with the prior wait implementation, because they
add additional conditions that cause the processor to resume, which can
cause software to hang or run very slowly.

At this moment, with the current wait implementation and a pseries guest
using mainline kernel with new wait upcodes [1], QEMU hangs during boot if
more than one CPU is present:

 qemu-system-ppc64 -M pseries,x-vof=on -cpu POWER10 -smp 2 -nographic
-kernel zImage.pseries -no-reboot

QEMU will exit (as there's no filesystem) if the test "passes", or hang
during boot if it hits the bug.

ISA v3.0 changed the wait opcode and removed the new variants (retaining
the WC field but making non-zero values reserved).

ISA v3.1 added new WC values to the new wait opcode, and added a PL
field.

This patch implements the new wait encoding and supports WC variants
with no-op implementations, which provides basic correctness as
explained in comments.

[1] https://lore.kernel.org/all/20220720132132.903462-1-npig...@gmail.com/

Signed-off-by: Nicholas Piggin 
Reviewed-by: Víctor Colombo 
Tested-by: Joel Stanley 
Reviewed-by: Daniel Henrique Barboza 
Message-Id: <20220720133352.904263-1-npig...@gmail.com>
[danielhb: added information about the bug being fixed]
Signed-off-by: Daniel Henrique Barboza 
---
 target/ppc/internal.h  |  3 ++
 target/ppc/translate.c | 96 ++
 2 files changed, 91 insertions(+), 8 deletions(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 467f3046c8..337a362205 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -165,6 +165,9 @@ EXTRACT_HELPER_SPLIT_3(DX, 10, 6, 6, 5, 16, 1, 1, 0, 0)
 /* darn */
 EXTRACT_HELPER(L, 16, 2);
 #endif
+/* wait */
+EXTRACT_HELPER(WC, 21, 2);
+EXTRACT_HELPER(PL, 16, 2);
 
 /***Jump target decoding   ***/
 /* Immediate address */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 5a18ee577f..388337f81b 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4071,12 +4071,91 @@ static void gen_sync(DisasContext *ctx)
 /* wait */
 static void gen_wait(DisasContext *ctx)
 {
-TCGv_i32 t0 = tcg_const_i32(1);
-tcg_gen_st_i32(t0, cpu_env,
-   -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
-tcg_temp_free_i32(t0);
-/* Stop translation, as the CPU is supposed to sleep from now */
-gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
+uint32_t wc;
+
+if (ctx->insns_flags & PPC_WAIT) {
+/* v2.03-v2.07 define an older incompatible 'wait' encoding. */
+
+if (ctx->insns_flags2 & PPC2_PM_ISA206) {
+/* v2.06 introduced the WC field. WC > 0 may be treated as no-op. 
*/
+wc = WC(ctx->opcode);
+} else {
+wc = 0;
+}
+
+} else if (ctx->insns_flags2 & PPC2_ISA300) {
+/* v3.0 defines a new 'wait' encoding. */
+wc = WC(ctx->opcode);
+if (ctx->insns_flags2 & PPC2_ISA310) {
+uint32_t pl = PL(ctx->opcode);
+
+/* WC 1,2 may be treated as no-op. WC 3 is reserved. */
+if (wc == 3) {
+gen_invalid(ctx);
+return;
+}
+
+/* PL 1-3 are reserved. If WC=2 then the insn is treated as noop. 
*/
+if (pl > 0 && wc != 2) {
+gen_invalid(ctx);
+return;
+}
+
+} else { /* ISA300 */
+/* WC 1-3 are reserved */
+if (wc > 0) {
+gen_invalid(ctx);
+return;
+}
+}
+
+} else {
+warn_report("wait instruction decoded with wrong ISA flags.");
+gen_invalid(ctx);
+return;
+}
+
+/*
+ * wait without WC field or with WC=0 waits for an exception / interrupt
+ * to occur.
+ */
+if (wc == 0) {
+TCGv_i32 t0 = tcg_const_i32(1);
+tcg_gen_st_i32(t0, cpu_env,
+   -offsetof(PowerPCCPU, env) + offsetof(CPUState, 
halted));
+tcg_temp_free_i32(t0);
+/* Stop translation, as the CPU is supposed to sleep from now */
+gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
+}
+
+/*
+ * Other wait types must not just wait until an exception occurs because
+ * ignoring their other wake-up conditions could cause a hang.
+ *
+ * For v2.06 and 2.07, wc=1,2,3 are architected but may be implemented as
+ * no-ops.
+ *
+ * wc=1 and wc=3 explicitly allow the instruction to be treated as a no-op.
+ *
+ * wc=2 waits for an implementation-specific condition, such could be
+ * always true, so it can be implemented as a no-op.
+ *
+ * For v3.1, wc=1,2 are architected but may be implemented as no-ops.
+ *
+ * wc=1 (waitrsv) waits for an exception or a reservation to be lost.
+ *

[PULL 1/3] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c

spapr_nvdimm_flush_completion_cb() and flush_worker_cb() are using the
DRC object returned by spapr_drc_index() without checking it for NULL.
In this case we would be dereferencing a NULL pointer when doing
SPAPR_NVDIMM(drc->dev) and PC_DIMM(drc->dev).

This can happen if, during a scm_flush(), the DRC object is wrongly
freed/released (e.g. a bug in another part of the code).
spapr_drc_index() would then return NULL in the callbacks.

Fixes: Coverity CID 1487108, 1487178
Reviewed-by: Greg Kurz 
Message-Id: <20220409200856.283076-2-danielhb...@gmail.com>
Signed-off-by: Daniel Henrique Barboza 
---
 hw/ppc/spapr_nvdimm.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index c4c97da5de..04a64cada3 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -447,9 +447,15 @@ static int flush_worker_cb(void *opaque)
 {
 SpaprNVDIMMDeviceFlushState *state = opaque;
 SpaprDrc *drc = spapr_drc_by_index(state->drcidx);
-PCDIMMDevice *dimm = PC_DIMM(drc->dev);
-HostMemoryBackend *backend = MEMORY_BACKEND(dimm->hostmem);
-int backend_fd = memory_region_get_fd(>mr);
+PCDIMMDevice *dimm;
+HostMemoryBackend *backend;
+int backend_fd;
+
+g_assert(drc != NULL);
+
+dimm = PC_DIMM(drc->dev);
+backend = MEMORY_BACKEND(dimm->hostmem);
+backend_fd = memory_region_get_fd(>mr);
 
 if (object_property_get_bool(OBJECT(backend), "pmem", NULL)) {
 MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
@@ -475,7 +481,11 @@ static void spapr_nvdimm_flush_completion_cb(void *opaque, 
int hcall_ret)
 {
 SpaprNVDIMMDeviceFlushState *state = opaque;
 SpaprDrc *drc = spapr_drc_by_index(state->drcidx);
-SpaprNVDIMMDevice *s_nvdimm = SPAPR_NVDIMM(drc->dev);
+SpaprNVDIMMDevice *s_nvdimm;
+
+g_assert(drc != NULL);
+
+s_nvdimm = SPAPR_NVDIMM(drc->dev);
 
 state->hcall_ret = hcall_ret;
 QLIST_REMOVE(state, node);
-- 
2.36.1

[PULL 0/3] ppc queue

The following changes since commit 3e4abe2c92964aadd35344a635b0f32cb487fd5c:

  Merge tag 'pull-block-2022-07-27' of https://gitlab.com/vsementsov/qemu into 
staging (2022-07-27 20:10:15 -0700)

are available in the Git repository at:

  https://gitlab.com/danielhb/qemu.git pull-ppc-20220728

for you to fetch changes up to 0c9717ff35d2fe46fa9cb91566fe2afbed9f4f2a:

  target/ppc: Implement new wait variants (2022-07-28 13:30:41 -0300)


ppc patch queue for 2022-07-28:

Short queue with 2 Coverity fixes and one fix of the
'wait' insns that is causing hangs if the guest kernel uses
the most up to date wait opcode.

- target/ppc:
  - implement new wait variants to fix guest hang when using the new opcode
- ppc440_uc: initialize length passed to cpu_physical_memory_map()
- spapr_nvdimm: check if spapr_drc_index() returns NULL


Daniel Henrique Barboza (1):
  hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c

Nicholas Piggin (1):
  target/ppc: Implement new wait variants

Peter Maydell (1):
  hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map()

 hw/ppc/ppc440_uc.c |  5 ++-
 hw/ppc/spapr_nvdimm.c  | 18 +++---
 target/ppc/internal.h  |  3 ++
 target/ppc/translate.c | 96 +-
 4 files changed, 109 insertions(+), 13 deletions(-)

Re: [RFC patch 0/1] block: vhost-blk backend

2022-07-28 Thread Stefano Garzarella

On Thu, Jul 28, 2022 at 7:28 AM Andrey Zhadchenko 
 wrote:
> On 7/27/22 16:06, Stefano Garzarella wrote:
> > On Tue, Jul 26, 2022 at 04:15:48PM +0200, Denis V. Lunev wrote:
> >> On 26.07.2022 15:51, Michael S. Tsirkin wrote:
> >>> On Mon, Jul 25, 2022 at 11:55:26PM +0300, Andrey Zhadchenko wrote:
>  Although QEMU virtio-blk is quite fast, there is still some room for
>  improvements. Disk latency can be reduced if we handle virito-blk
>  requests
>  in host kernel so we avoid a lot of syscalls and context switches.
> 
>  The biggest disadvantage of this vhost-blk flavor is raw format.
>  Luckily Kirill Thai proposed device mapper driver for QCOW2 format
>  to attach
>  files as block devices:
>  https://www.spinics.net/lists/kernel/msg4292965.html
> >>> That one seems stalled. Do you plan to work on that too?
> >> We have too. The difference in numbers, as you seen below is quite too
> >> much. We have waited for this patch to be sent to keep pushing.
> >>
> >> It should be noted that may be talk on OSS this year could also push a
> >> bit.
> >
> > Cool, the results are similar of what I saw when I compared vhost-blk
> > and io_uring passthrough with NVMe (Slide 7 here: [1]).
> >
> > About QEMU block layer support, we recently started to work on libblkio
> > [2]. Stefan also sent an RFC [3] to implement the QEMU BlockDriver.
> > Currently it supports virtio-blk devices using vhost-vdpa and vhost-user.
> > We could add support for vhost (kernel) as well, though, we were
> > thinking of leveraging vDPA to implement in-kernel software device as well.
> >
> > That way we could reuse a lot of the code to support both hardware and
> > software accelerators.
> >
> > In the talk [1] I describe the idea a little bit, and a few months ago I
> > did a PoC (unsubmitted RFC) to see if it was feasible and the numbers
> > were in line with vhost-blk.
> >
> > Do you think we could join forces and just have an in-kernel vdpa-blk
> > software device?
>
> This seems worth trying. Why double the efforts to do the same. Yet I
> would like to play a bit with your vdpa-blk PoC beforehand.

Great :-)

> Can you send it to me with some instructions how to run it?

Yep, sure!

The PoC is available here: 
https://gitlab.com/sgarzarella/linux/-/tree/vdpa-sw-blk-poc

The tree was based on Linux v5.16, but I had some issues to rebuild with 
new gcc, so I rebased on v5.16.20 (not tested), configs needed:
CONFIG_VDPA_SW_BLOCK=m + CONFIG_VHOST_VDPA=m + dependencies.

It contains:
  - patches required for QEMU generic vhost-vdpa support
  - patches to support blk_mq_ops->poll() (to use io_uring iopoll) in
the guest virtio-blk driver (I used the same kernel on guest and
host)
  - some improvements for vringh (not completed, it could be a
bottleneck)
  - vdpa-sw and vdpa-sw-blk patches (and hacks)

It is based on the vDPA simulator framework already merged upstream. The 
idea is to generalize the simulator to share the code between both 
software devices and simulators. The code needs a lot of work, I was 
focusing just on a working virtio-blk device emulation, but more focus 
on the generic part should be done.
In the code there are a couple of defines to control polling.

About the vdpa-blk device, you need iproute2's vdpa tool available 
upstream:
  https://wiki.linuxfoundation.org/networking/iproute2

Once the device is instantiated (see instructions later), the backend 
(raw file or device) can be set through a device attribute (not robust, 
but it was a PoC): /sys/bus/vdpa/devices/$dev_name/backend_fd

I wrote a simple python script available here: 
https://github.com/stefano-garzarella/vm-build/blob/main/vm-tools/vdpa_set_backend_fd.py

For QEMU, we are working on libblkio to support both slow path (when 
QEMU block layer is needed) and fast path (vqs passed directly to the 
device). For now libblkio supports only slow path, so to test the fast 
path you can use Longpeng's patches (not yet merged upstream) with 
generic vhost-vdpa support: 
https://lore.kernel.org/qemu-devel/20220514041107.1980-1-longpe...@huawei.com/

Steps:
  # load vDPA block in-kernel sw device module
  modprobe vdpa_sw_blk

  # load nvme module with poll_queues set if you want to use iopoll
  modprobe nvme poll_queues=15

  # instantiate a new vdpa-blk device
  vdpa dev add mgmtdev vdpasw_blk name blk0

  # set backend (/dev/nvme0n1)
  vdpa_set_backend_fd.py -b /dev/nvme0n1 blk0

  # load vhost vDPA bus ...
  modprobe vhost_vdpa

  # ... and vhost-vdpa device will appear
  ls -l /dev/vhost-vdpa-0
  crw---. 1 root root 510, 0 Jul 28 17:06 /dev/vhost-vdpa-0

  # start QEMU patched with generic vhost-vdpa
  qemu-system-x86_64 ... \
  -device vhost-vdpa-device-pci,vhostdev=/dev/vhost-vdpa-0

I haven't tested it recently, so I'm not sure it all works, but in the 
next few days I'll try. For anything else, feel free to reach me here or 
on IRC (sgarzare on #qemu).

Thanks,
Stefano

Re: [RFC] hw/nvme: Use irqfd to send interrupts

2022-07-28 Thread Jinhao Fan

at 11:18 PM, Stefan Hajnoczi  wrote:

> I think that is incorrect. QEMU has guest notifier emulation for the
> non-KVM (and non-MSI-X PCI) cases. When there is no irqfd support
> available, QEMU sets up a regular eventfd and calls
> virtio_queue_guest_notifier_read() when it becomes readable.

Thanks Stefan. I finally understand why there is a `with_irqfd` parameter
for virtio_queue_set_guest_notifier_fd_handler.

But if `with_irqfd` is false, it seems OK to directly call virtio_irq(). Why
still bother using an eventfd? Is it for interrupt batching?

Re: [RFC] hw/nvme: Use irqfd to send interrupts

2022-07-28 Thread Stefan Hajnoczi

On Thu, Jul 28, 2022, 11:34 Jinhao Fan  wrote:

> at 11:18 PM, Stefan Hajnoczi  wrote:
>
> > I think that is incorrect. QEMU has guest notifier emulation for the
> > non-KVM (and non-MSI-X PCI) cases. When there is no irqfd support
> > available, QEMU sets up a regular eventfd and calls
> > virtio_queue_guest_notifier_read() when it becomes readable.
>
> Thanks Stefan. I finally understand why there is a `with_irqfd` parameter
> for virtio_queue_set_guest_notifier_fd_handler.
>
> But if `with_irqfd` is false, it seems OK to directly call virtio_irq().
> Why
> still bother using an eventfd? Is it for interrupt batching?
>

virtio_irq() is not thread safe so it cannot be called directly from the
IOThread. Bouncing through the eventfd ensures that the virtio_irq() call
happens in the QEMU main loop thread with the BQL held.

It may be cheaper to use a BH instead of an eventfd when irqfd is not
available, but this is a slow path anyway. We might as well reuse the
eventfd code that's already there.

Stefan

>

Re: [PATCH for-7.1 0/3] hw/nvme: misc ioeventfd fixes

2022-07-28 Thread Keith Busch

On Thu, Jul 28, 2022 at 10:25:19AM +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> A set of fixes/changes to the ioeventfd support.

Series looks good.

Reviewed-by: Keith Busch

Re: [PATCH for-7.1 0/3] hw/nvme: misc ioeventfd fixes

2022-07-28 Thread Jinhao Fan

at 4:25 PM, Klaus Jensen  wrote:

> From: Klaus Jensen 
> 
> A set of fixes/changes to the ioeventfd support.
> 
> Klaus Jensen (3):
>  hw/nvme: skip queue processing if notifier is cleared
>  hw/nvme: unregister the event notifier handler on the main loop
>  hw/nvme: do not enable ioeventfd by default
> 
> hw/nvme/ctrl.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
> 
> -- 
> 2.36.1

Looks good to me as well.

Reviewed-by: Jinhao Fan

Re: [PATCH v2 5/7] multifd: establishing connection between any non-default src and dest pair

2022-07-28 Thread Het Gala




On 26/07/22 4:14 pm, Daniel P. Berrangé wrote:

In $SUBJECT  s/multifd:/io:/ as this is not migration related.

On Thu, Jul 21, 2022 at 07:56:18PM +, Het Gala wrote:

i) Binding of the socket to source ip address and port on the non-default
interface has been implemented for multi-FD connection, which was not
necessary earlier because the binding was on the default interface itself.

ii) Created an end to end connection between all multi-FD source and
 destination pairs.

Suggested-by: Manish Mishra 
Signed-off-by: Het Gala 
---
  include/io/channel-socket.h| 44 
  include/qemu/sockets.h |  6 ++-
  io/channel-socket.c| 93 ++
  migration/socket.c |  4 +-
  tests/unit/test-util-sockets.c | 16 +++---
  util/qemu-sockets.c| 90 +++-
  6 files changed, 196 insertions(+), 57 deletions(-)

diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index 513c428fe4..8168866b06 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -211,6 +211,50 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
  GMainContext *context);
  
  
+/**

+ * qio_channel_socket_connect_all_sync:

This needs to be called qio_channel_socket_connect_full_sync to
match the naming conventions in use in IO code.
> Sorry Daniel, will definitely update this in the coming patchset for 
sure.

+ * @ioc: the socket channel object
+ * @dst_addr: the destination address to connect to
+ * @src_addr: the source address to be connected

'the optional source address to bind to'

> Sure, acknowledged.

+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Attempt to connect to the address @dst_addr with @src_addr.

   * Attempt to connect to the address @dst_addr. If @src_addr
   * is non-NULL, it will be bound to in order to control outbound
   * interface routing.



+ * This method will run in the foreground so the caller will not
+ * regain execution control until the connection is established or
+ * an error occurs.
+ */
+int qio_channel_socket_connect_all_sync(QIOChannelSocket *ioc,
+SocketAddress *dst_addr,
+SocketAddress *src_addr,
+Error **errp);

Vertical mis-alignment of parameters

> Acknowledged.

+
+/**
+ * qio_channel_socket_connect_all_async:

Needs to be qio_channel_socket_connect_full_async
> Acknowledged. Sorry for such nit errors. Will update them in next 
patchset

+ * @ioc: the socket channel object
+ * @dst_addr: the destination address to connect to

@src_addr needs to be placed here...

> Acknowledged.

+ * @callback: the function to invoke on completion
+ * @opaque: user data to pass to @callback
+ * @destroy: the function to free @opaque
+ * @context: the context to run the async task. If %NULL, the default
+ *   context will be used.
+ * @src_addr: the source address to be connected

...not here

and same note about the docs comment

> Acknowledged

+ *
+ * Attempt to connect to the address @dst_addr with the @src_addr.

Same note about the docs comment

> Acknowledged.



+ * This method will run in the background so the caller will regain
+ * execution control immediately. The function @callback
+ * will be invoked on completion or failure. The @dst_addr
+ * parameter will be copied, so may be freed as soon
+ * as this function returns without waiting for completion.
+ */
+void qio_channel_socket_connect_all_async(QIOChannelSocket *ioc,
+  SocketAddress *dst_addr,
+  QIOTaskFunc callback,
+  gpointer opaque,
+  GDestroyNotify destroy,
+  GMainContext *context,
+  SocketAddress *src_addr);
+
+
  /**
   * qio_channel_socket_get_local_address:
   * @ioc: the socket channel object





diff --git a/migration/socket.c b/migration/socket.c
index dab872a0fe..69fda774ba 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -57,8 +57,8 @@ int outgoing_param_total_multifds(void)
  void socket_send_channel_create(QIOTaskFunc f, void *data)
  {
  QIOChannelSocket *sioc = qio_channel_socket_new();
-qio_channel_socket_connect_async(sioc, outgoing_args.saddr,
- f, data, NULL, NULL);
+qio_channel_socket_connect_all_async(sioc, outgoing_args.saddr,
+ f, data, NULL, NULL, NULL);
  }

Don't change this call at all until the next patch which actually
needs to pass a non-NULL parameter for src.

> Sure, acknowledged.

  QIOChannel *socket_send_channel_create_sync(Error **errp)
diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
index

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

On Thu, 28 Jul 2022 at 15:44, Dr. David Alan Gilbert
 wrote:
>
> * Igor Mammedov (imamm...@redhat.com) wrote:
> > QEMU crashes trying to save VMSTATE when only MIPS target are compiled in
> >   $ qemu-system-mips -monitor stdio
> >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> >   Segmentation fault (core dumped)
> >
> > It happens due to PIIX4_PM trying to parse hotplug vmstate structures
> > which are valid only for x86 and not for MIPS (as it requires ACPI
> > tables support which is not existent for ithe later)
> >
> > Issue was probably exposed by trying to cleanup/compile out unused
> > ACPI bits from MIPS target (but forgetting about migration bits).
> >
> > Disable compiled out features using compat properties as the least
> > risky way to deal with issue.
>
> Isn't the problem partially due to a 'stub' vmsd which isn't terminated?

Yes, but setting these properties causes that vmsd
(vmstate_acpi_pcihp_pci_status) to not be used:

 * it is used only in VMSTATE_PCI_HOTPLUG()
 * that macro is used only in hw/acpi/ich9.c (not relevant here) and
   hw/acpi/piix4.c
 * in piix4.c it is invoked passing it the test functions
   vmstate_test_use_acpi_hotplug_bridge and
   vmstate_test_migrate_acpi_index
 * setting the properties on the device as this patch does
   causes those test functions to return false, so the
   vmstate_acpi_pcihp_pci_status is never examined

-- PMM

Re: [PATCH v3 0/2] migration-test: Allow test to run without uffd

On Thu, Jul 28, 2022 at 04:44:49PM +0200, Thomas Huth wrote:
> On 28/07/2022 15.35, Peter Xu wrote:
> > v2:
> > - Fix warning in patch 1 [Thomas]
> > - Collected R-b for Daniel
> > 
> > Compare to v1, this added a new patch as reported by Thomas to (hopefully)
> > allow auto-converge test to pass on some MacOS testbeds.
> > 
> > Please review, thanks.
> > 
> > Peter Xu (2):
> >migration-test: Use migrate_ensure_converge() for auto-converge
> >migration-test: Allow test to run without uffd
> > 
> >   tests/qtest/migration-test.c | 67 +++-
> >   1 file changed, 27 insertions(+), 40 deletions(-)
> 
> Seems to work now:
> 
> https://api.cirrus-ci.com/v1/task/4760264934424576/logs/build.log
> 
> Citing:
> 
> " 2/59 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test OK
> 218.87s   33 subtests passed"
> 
> Tested-by: Thomas Huth 

Thanks!

-- 
Peter Xu

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

On Thu, 28 Jul 2022 at 16:09, Dr. David Alan Gilbert
 wrote:
>
> * Igor Mammedov (imamm...@redhat.com) wrote:
> > On Thu, 28 Jul 2022 15:44:20 +0100
> > "Dr. David Alan Gilbert"  wrote:
> >
> > > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > > QEMU crashes trying to save VMSTATE when only MIPS target are compiled 
> > > > in
> > > >   $ qemu-system-mips -monitor stdio
> > > >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> > > >   Segmentation fault (core dumped)
> > > >
> > > > It happens due to PIIX4_PM trying to parse hotplug vmstate structures
> > > > which are valid only for x86 and not for MIPS (as it requires ACPI
> > > > tables support which is not existent for ithe later)
> > > >
> > > > Issue was probably exposed by trying to cleanup/compile out unused
> > > > ACPI bits from MIPS target (but forgetting about migration bits).
> > > >
> > > > Disable compiled out features using compat properties as the least
> > > > risky way to deal with issue.
> > >
> > > Isn't the problem partially due to a 'stub' vmsd which isn't terminated?
> >
> > Not sure what "'stub' vmsd" is, can you explain?
>
> In hw/acpi/acpi-pci-hotplug-stub.c there is :
> const VMStateDescription vmstate_acpi_pcihp_pci_status;
>
> this seg happens when the migration code walks into that - this should
> always get populated with some of the minimal fields, in particular the
> .name and .fields array terminated with VMSTATE_END_OF_LIST().

Either:
 (1) we should be sure the vmstate struct does not get used if the
 compile-time config has ended up with the stub
or
 (2) it needs to actually match the real vmstate struct, otherwise
 migration between a QEMU built with a config that just got the
 stub version and a QEMU built with a config that got the full
 version will break

This patch does the former. Segfaulting if we got something wrong
and tried to use the vmstate when we weren't expecting to is
arguably better than producing an incompatible migration stream.
(Better still would be if we caught this on machine startup rather
than only when savevm was invoked.)

thanks
-- PMM

[PATCH for-7.1] linux-user/flatload.c: Fix setting of image_info::end_code

The flatload loader sets the end_code field in the image_info struct
incorrectly, due to a typo.

This is a very long-standing bug (dating all the way back to when
the bFLT loader was added in 2006), but has gone unnoticed because
(a) most people don't use bFLT binaries
(b) we don't actually do anything with the end_code field, except
print it in debugging traces and pass it to TCG plugins

Fix the typo.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1119
Signed-off-by: Peter Maydell 
---
 linux-user/flatload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index e4c2f89a226..e99570ca182 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -808,7 +808,7 @@ int load_flt_binary(struct linux_binprm *bprm, struct 
image_info *info)
 
 /* Stash our initial stack pointer into the mm structure */
 info->start_code = libinfo[0].start_code;
-info->end_code = libinfo[0].start_code = libinfo[0].text_len;
+info->end_code = libinfo[0].start_code + libinfo[0].text_len;
 info->start_data = libinfo[0].start_data;
 info->end_data = libinfo[0].end_data;
 info->start_brk = libinfo[0].start_brk;
-- 
2.25.1

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

2022-07-28 Thread Dr. David Alan Gilbert

* Igor Mammedov (imamm...@redhat.com) wrote:
> On Thu, 28 Jul 2022 15:44:20 +0100
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > QEMU crashes trying to save VMSTATE when only MIPS target are compiled in
> > >   $ qemu-system-mips -monitor stdio
> > >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> > >   Segmentation fault (core dumped)
> > > 
> > > It happens due to PIIX4_PM trying to parse hotplug vmstate structures
> > > which are valid only for x86 and not for MIPS (as it requires ACPI
> > > tables support which is not existent for ithe later)
> > > 
> > > Issue was probably exposed by trying to cleanup/compile out unused
> > > ACPI bits from MIPS target (but forgetting about migration bits).
> > > 
> > > Disable compiled out features using compat properties as the least
> > > risky way to deal with issue.  
> > 
> > Isn't the problem partially due to a 'stub' vmsd which isn't terminated?
> 
> Not sure what "'stub' vmsd" is, can you explain?

In hw/acpi/acpi-pci-hotplug-stub.c there is :
const VMStateDescription vmstate_acpi_pcihp_pci_status;

this seg happens when the migration code walks into that - this should
always get populated with some of the minimal fields, in particular the
.name and .fields array terminated with VMSTATE_END_OF_LIST().

Dave

> > 
> > Dave
> > 
> > > Signed-off-by: Igor Mammedov 
> > > ---
> > > PS:
> > > another approach could be setting defaults to disabled state and
> > > enabling them using compat props on PC machines (which is more
> > > code to deal with => more risky) or continue with PIIX4_PM
> > > refactoring to split x86-shism out (which I'm not really
> > > interested in due to risk of regressions for not much of
> > > benefit)
> > > ---
> > >  hw/mips/malta.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > > index 7a0ec513b0..0e932988e0 100644
> > > --- a/hw/mips/malta.c
> > > +++ b/hw/mips/malta.c
> > > @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = {
> > >  .instance_init = mips_malta_instance_init,
> > >  };
> > >  
> > > +GlobalProperty malta_compat[] = {
> > > +{ "PIIX4_PM", "memory-hotplug-support", "off" },
> > > +{ "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" },
> > > +{ "PIIX4_PM", "acpi-root-pci-hotplug", "off" },
> > > +{ "PIIX4_PM", "x-not-migrate-acpi-index", "true" },
> > > +};
> > > +const size_t malta_compat_len = G_N_ELEMENTS(malta_compat);
> > > +
> > >  static void mips_malta_machine_init(MachineClass *mc)
> > >  {
> > >  mc->desc = "MIPS Malta Core LV";
> > > @@ -1455,6 +1463,7 @@ static void mips_malta_machine_init(MachineClass 
> > > *mc)
> > >  mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
> > >  #endif
> > >  mc->default_ram_id = "mips_malta.ram";
> > > +compat_props_add(mc->compat_props, malta_compat, malta_compat_len);
> > >  }
> > >  
> > >  DEFINE_MACHINE("malta", mips_malta_machine_init)
> > > -- 
> > > 2.31.1
> > >   
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v2 2/7] multifd: modifying 'migrate' qmp command to add multifd socket on particular src and dest pair

2022-07-28 Thread Het Gala




On 26/07/22 4:43 pm, Daniel P. Berrangé wrote:

On Thu, Jul 21, 2022 at 07:56:15PM +, Het Gala wrote:

i) Modified the format of the qemu monitor command : 'migrate' by adding a list,
each element in the list consisting of multifd connection parameters: source
uri, destination uri and of the number of multifd channels between each 
pair.

ii) Information of all multifd connection parameters' list and length of the
 list is stored in 'OutgoingMigrateParams' struct.

Suggested-by: Manish Mishra 
Signed-off-by: Het Gala 
---
  migration/migration.c | 52 +
  migration/socket.c| 60 ---
  migration/socket.h| 19 +-
  monitor/hmp-cmds.c|  1 +
  qapi/migration.json   | 47 +
  5 files changed, 160 insertions(+), 19 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 81185d4311..456247af8f 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1449,12 +1449,37 @@
  ##
  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
  
+##

+# @MigrateUriParameter:
+#
+# Information regarding which source interface is connected to which
+# destination interface and number of multifd channels over each interface.
+#
+# @source-uri: uri of the source VM. Default port number is 0.
+#
+# @destination-uri: uri of the destination VM
+#
+# @multifd-channels: number of parallel multifd channels used to migrate data
+#for specific source-uri and destination-uri. Default value
+#in this case is 2 (Since 7.1)
+#
+##
+{ 'struct' : 'MigrateUriParameter',
+  'data' : { 'source-uri' : 'str',
+ 'destination-uri' : 'str',
+ '*multifd-channels' : 'uint8'} }
+
  ##
  # @migrate:
  #
  # Migrates the current running guest to another Virtual Machine.
  #
  # @uri: the Uniform Resource Identifier of the destination VM
+#   for migration thread
+#
+# @multi-fd-uri-list: list of pair of source and destination VM Uniform
+# Resource Identifiers with number of multifd-channels
+# for each pair
  #
  # @blk: do block migration (full disk copy)
  #
@@ -1474,20 +1499,32 @@
  # 1. The 'query-migrate' command should be used to check migration's progress
  #and final result (this information is provided by the 'status' member)
  #
-# 2. All boolean arguments default to false
+# 2. The uri argument should have the Uniform Resource Identifier of default
+#destination VM. This connection will be bound to default network
  #
-# 3. The user Monitor's "detach" argument is invalid in QMP and should not
+# 3. All boolean arguments default to false
+#
+# 4. The user Monitor's "detach" argument is invalid in QMP and should not
  #be used
  #
  # Example:
  #
-# -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
+# -> { "execute": "migrate",
+#  "arguments": {
+#  "uri": "tcp:0:4446",
+#  "multi-fd-uri-list": [ { "source-uri": "tcp::6900",
+#   "destination-uri": "tcp:0:4480",
+#   "multifd-channels": 4},
+# { "source-uri": "tcp:10.0.0.0: ",
+#   "destination-uri": "tcp:11.0.0.0:7789",
+#   "multifd-channels": 5} ] } }
  # <- { "return": {} }
  #
  ##
  { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
-   '*detach': 'bool', '*resume': 'bool' } }
+  'data': {'uri': 'str', '*multi-fd-uri-list': ['MigrateUriParameter'],
+   '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
+   '*resume': 'bool' } }

Considering the existing migrate API from a QAPI design POV, I
think there are several significant flaws with it

The use of URIs is the big red flag. It is basically a data encoding
scheme within a data encoding scheme.  QEMU code should be able to
directly work with the results from QAPI, without having todo a
second level of parsing.

URIs made sense in the context of HMP or the QemuOpts CLI, but do not
make sense in QMP. We made a mistake in this respect when we first
introduced QMP and implemented 'migrate'.

If we going to extend the migrate API I think we should stop using URIs
for the new fields, and instead define a QAPI discriminated union for
the different data transport backends we offer.

  { 'enum': 'MigrateTransport',
'data': ['socket', 'exec'] }

  { 'union': 'MigrateAddress',
'base': { 'transport': 'MigrateTransport'},
'discriminator': 'transport',
'data': {
'socket': 'SocketAddress',
   'exec': ['str'],
}

NB, 'socket' should be able to cover all of  'tcp', 'unix', 'vsock'
and 'fd' already. I'm fuzzy on best way to represent RDMA.


IIUC, the desire of migration maintainers is that we can ultimately
have multifd as the preferred,

Re: [PATCH v2 3/7] multifd: adding multi-interface support for multifd on destination side

2022-07-28 Thread Het Gala




On 26/07/22 4:50 pm, Daniel P. Berrangé wrote:

On Thu, Jul 21, 2022 at 07:56:16PM +, Het Gala wrote:

i) Modified the format of qemu monitor command: 'migrate-incoming' by adding
a list, each element in the list to open socket listeners with a given
number of multifd channels.

ii) Qemu starts with -incoming flag defer and -multi-fd-incoming defer to
 allow the modified 'migrate-incoming' command to be used.

iii) Format for -multi-fd-incoming flag as a comma separated string has been
  added with each substring containing listener socket address and number
  of sockets to open.

Suggested-by: Manish Mishra 
Signed-off-by: Het Gala 



diff --git a/qemu-options.hx b/qemu-options.hx
index 79e00916a1..f0d2fd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4479,6 +4479,24 @@ SRST
  to issuing the migrate\_incoming to allow the migration to begin.
  ERST
  
+DEF("multi-fd-incoming", HAS_ARG, QEMU_OPTION_multi_fd_incoming, \

+"-multi-fd-incoming 
tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]\n" \
+"-multi-fd-incoming defer\n" \
+"wait for the URI to be specified via\n" \
+"multi_fd_migrate_incoming\n",
+QEMU_ARCH_ALL)
+SRST
+``-multi-fd-incoming 
tcp:[host]:port[:channel][,to=maxport][,ipv4=on|off][,ipv6=on|off]``
+Prepare for multi-fd incoming migration, with multi-fd listening sockets
+on that connection. Default number of multi-fd channels is 2.
+
+``-multi-fd-incoming defer``
+Wait for the URI to be specified via multi_fd_migrate\_incoming. The
+monitor can be used to change settings (such as migration parameters)
+prior to issuing the multi_fd_migrate\_incoming to allow the migration
+to begin.
+ERST

We should not be adding any new -multi-fd-incoming CLI parameter at all.
The CLI is so unsuitable for any complex configuration param and this
is a prime example.

If anything we should fully deprecate anything that is not '-incoming defer'
such that we become 100% QMP/QAPI based for incoming migration config.
> Sure Daniel. We will depricate this -multi-fd-incoming defer flag and 
only keep QMP/QAPI based migration config in the coming patchset series.

With regards,
Daniel

With Regards,
Het Gala

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

On Thu, 28 Jul 2022 at 15:50, Markus Armbruster  wrote:
> Kevin Wolf  writes:
> >
> > But if you have more than one device, it becomes hard to predict which
> > device gets which backend - it depends on the initialisation order in
> > the code then,
>
> Really?  Board code should use IF_OTHER devices just like it uses the
> other interface types, namely connecting each frontend device to a
> backend device with a well-known and fixed interface type and index (or
> bus and unit instead, where appropriate).

I think part of the problem is that unlike the typical disk
interface, where there is some idea of bus-and-unit-number or
index number that it makes sense to expose to users, these
"miscellaneous storage" devices don't have any particular index
concept -- in the real hardware there are just a random set of
devices that are connected in various places. So you're requiring
users to look up the documentation for "index 0 is this eeprom,
index 1 is that other eeprom, index 2 is ...".

thanks
-- PMM

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

2022-07-28 Thread Dr. David Alan Gilbert

* Igor Mammedov (imamm...@redhat.com) wrote:
> QEMU crashes trying to save VMSTATE when only MIPS target are compiled in
>   $ qemu-system-mips -monitor stdio
>   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
>   Segmentation fault (core dumped)
> 
> It happens due to PIIX4_PM trying to parse hotplug vmstate structures
> which are valid only for x86 and not for MIPS (as it requires ACPI
> tables support which is not existent for ithe later)
> 
> Issue was probably exposed by trying to cleanup/compile out unused
> ACPI bits from MIPS target (but forgetting about migration bits).
> 
> Disable compiled out features using compat properties as the least
> risky way to deal with issue.

Isn't the problem partially due to a 'stub' vmsd which isn't terminated?

Dave

> Signed-off-by: Igor Mammedov 
> ---
> PS:
> another approach could be setting defaults to disabled state and
> enabling them using compat props on PC machines (which is more
> code to deal with => more risky) or continue with PIIX4_PM
> refactoring to split x86-shism out (which I'm not really
> interested in due to risk of regressions for not much of
> benefit)
> ---
>  hw/mips/malta.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index 7a0ec513b0..0e932988e0 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = {
>  .instance_init = mips_malta_instance_init,
>  };
>  
> +GlobalProperty malta_compat[] = {
> +{ "PIIX4_PM", "memory-hotplug-support", "off" },
> +{ "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" },
> +{ "PIIX4_PM", "acpi-root-pci-hotplug", "off" },
> +{ "PIIX4_PM", "x-not-migrate-acpi-index", "true" },
> +};
> +const size_t malta_compat_len = G_N_ELEMENTS(malta_compat);
> +
>  static void mips_malta_machine_init(MachineClass *mc)
>  {
>  mc->desc = "MIPS Malta Core LV";
> @@ -1455,6 +1463,7 @@ static void mips_malta_machine_init(MachineClass *mc)
>  mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
>  #endif
>  mc->default_ram_id = "mips_malta.ram";
> +compat_props_add(mc->compat_props, malta_compat, malta_compat_len);
>  }
>  
>  DEFINE_MACHINE("malta", mips_malta_machine_init)
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

On Thu, 28 Jul 2022 15:44:20 +0100
"Dr. David Alan Gilbert"  wrote:

> * Igor Mammedov (imamm...@redhat.com) wrote:
> > QEMU crashes trying to save VMSTATE when only MIPS target are compiled in
> >   $ qemu-system-mips -monitor stdio
> >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> >   Segmentation fault (core dumped)
> > 
> > It happens due to PIIX4_PM trying to parse hotplug vmstate structures
> > which are valid only for x86 and not for MIPS (as it requires ACPI
> > tables support which is not existent for ithe later)
> > 
> > Issue was probably exposed by trying to cleanup/compile out unused
> > ACPI bits from MIPS target (but forgetting about migration bits).
> > 
> > Disable compiled out features using compat properties as the least
> > risky way to deal with issue.  
> 
> Isn't the problem partially due to a 'stub' vmsd which isn't terminated?

Not sure what "'stub' vmsd" is, can you explain?

> 
> Dave
> 
> > Signed-off-by: Igor Mammedov 
> > ---
> > PS:
> > another approach could be setting defaults to disabled state and
> > enabling them using compat props on PC machines (which is more
> > code to deal with => more risky) or continue with PIIX4_PM
> > refactoring to split x86-shism out (which I'm not really
> > interested in due to risk of regressions for not much of
> > benefit)
> > ---
> >  hw/mips/malta.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index 7a0ec513b0..0e932988e0 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = {
> >  .instance_init = mips_malta_instance_init,
> >  };
> >  
> > +GlobalProperty malta_compat[] = {
> > +{ "PIIX4_PM", "memory-hotplug-support", "off" },
> > +{ "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" },
> > +{ "PIIX4_PM", "acpi-root-pci-hotplug", "off" },
> > +{ "PIIX4_PM", "x-not-migrate-acpi-index", "true" },
> > +};
> > +const size_t malta_compat_len = G_N_ELEMENTS(malta_compat);
> > +
> >  static void mips_malta_machine_init(MachineClass *mc)
> >  {
> >  mc->desc = "MIPS Malta Core LV";
> > @@ -1455,6 +1463,7 @@ static void mips_malta_machine_init(MachineClass *mc)
> >  mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
> >  #endif
> >  mc->default_ram_id = "mips_malta.ram";
> > +compat_props_add(mc->compat_props, malta_compat, malta_compat_len);
> >  }
> >  
> >  DEFINE_MACHINE("malta", mips_malta_machine_init)
> > -- 
> > 2.31.1
> >

Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods

On Wed, 27 Jul 2022 13:22:34 +0800
Robert Hoo  wrote:

> On Thu, 2022-07-21 at 10:58 +0200, Igor Mammedov wrote:
> [...]
> Thanks Igor for review.
> > > > The patch it is too intrusive and my hunch is that it breaks
> > > > ABI and needs a bunch of compat knobs to work properly and
> > > > that I'd like to avoid unless there is not other way around
> > > > the problem.
> > > 
> > > Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> > > and the compat knobs refers to related functions' input/output
> > > params?  
> > 
> > ABI are structures that guest and QEMU pass through information
> > between each other. And knobs in this case would be compat
> > variable[s]
> > to keep old behavior in place for old machine types.  
> 
> My humble opinion:
> The changes of the compat variable(s) here don't break the ABI, the ABI
> between guest and host/qemu is the ACPI spec which we don't change and
> fully conform to it; actually we're implementing it.
> e.g. with these patches, old guest can boot up with no difference nor
> changes.

it's not about booting but about migration.
boot on old QEMU and then migrate to one with your patches,
then make guest use _DSM again. You will see that migrated
guest still uses _old_ ACPI tables/AML and ABI in new QEMU
_must_ be compatible with that.

As for the patch, it's too big, and looking at it I wasn't
able to convince myself that it's correct.

 
> >   
> > > My thoughts is that eventually, sooner or later, more ACPI methods
> > > will
> > > be implemented per request, although now we can play the trick of
> > > wrapper new methods over the pipe of old _DSM implementation.
> > > Though this changes a little on existing struct NvdimmDsmIn {}, it
> > > paves the way for the future; and actually the change is more an
> > > extension or generalization, not fundamentally changes the
> > > framework.
> > > 
> > > In short, my point is the change/generalization/extension will be
> > > inevitable, even if not present.  
> > 
> > Expanding ABI (interface between host) has 2 drawbacks
> >  * it exposes more attack surface of VMM to hostile guest
> >and rises chances that vulnerability would slip through
> >review/testing  
> 
> This patch doesn't increase attach surface, I think.
> 
> >  * migration wise, QEMU has to support any ABI for years
> >and not only latest an greatest interface but also old
> >ones to keep guest started on older QEMU working across
> >migration, so any ABI change should be considered very
> >carefully before being implemented otherwise it all
> >quickly snowballs in unsupportable mess of compat
> >variables smeared across host/guest.
> >Reducing exposed ABI and constant need to expand it
> >was a reason why we have moved ACPI code from firmware
> >into QEMU, so we could describe hardware without costs
> >associated with of maintaining ABI.  
> 
> Yeah, migration is the only broken thing. With this patch, guest ACPI
> table changes, live guest migrate between new and old qemus will have
> problem. But I think this is not the only example of such kind of
> problem. How about other similar cases?

Upstream policy for version-ed machine types (pc-*/q35-*/...),
forward migration _must_ work.
If you consider your device should e supported/usable downstream,
you also need take in account backward migration as well.


> In fact, the point of our contention is around this 
> https://www.qemu.org/docs/master/specs/acpi_nvdimm.html, whether or not
> change the implementation protocol by this patch. The protocol was for
> _DSM only. Unless we're not going to support any ACPI methods, it
> should be updated, and the _LS{I,R,W} are ACPI methods, we can play the
> trick in this special case, but definitely not next time.
> 
> I suggest to do it now, nevertheless, you maintainers make the final
> decision.

Not for this case (i.e. make patches minimal, touching only AML side
and reusing data that QEMU already provides via MMIO).

If ABI needs extending in future, that should be discussed separately
when there is actual need for it. 

> > 
> > There might be need to extend ABI eventually, but not in this case.
> >   
> > > > I was skeptical about this approach during v1 review and
> > > > now I'm pretty much sure it's over-engineered and we can
> > > > just repack data we receive from existing label _DSM functions
> > > > to provide _LS{I,R,W} like it was suggested in v1.
> > > > It will be much simpler and affect only AML side without
> > > > complicating ABI and without any compat cruft and will work
> > > > with ping-pong migration without any issues.
> > > 
> > > Ostensibly it may looks simpler, actually not, I think. The AML
> > > "common
> > > pipe" NCAL() is already complex, it packs all _DSMs and NFIT()
> > > function
> > > logics there, packing new stuff in/through it will be bug-prone.
> > > Though this time we can avert touching it, as the new ACPI methods
> > > deprecating old _DSM

Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

2022-07-28 Thread Markus Armbruster

Daniel P. Berrangé  writes:

> On Thu, Jul 28, 2022 at 02:40:22PM +0100, Peter Maydell wrote:
>> On Thu, 28 Jul 2022 at 14:30, Markus Armbruster  wrote:
>> > Peter Maydell  writes:
>> > I applaud the renaissance of roman-style inscriptions, but it's not just
>> > words without spaces, it's also capital letters only:
>> >
>> > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR
>> >
>> > Seriously, throw in some dashes or spaces.
>> 
>>   any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr
>
> On the basis that virtualization gives you turtles all the way down...
>
>  -device isa-applesmc,osk=

Oh, Egyptian rather than Roman.  Works for me!

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

2022-07-28 Thread Markus Armbruster

Kevin Wolf  writes:

> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
>> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf  wrote:
>> >
>> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
>> > > An OTP device isn't really a parallel flash, and neither are eFuses.
>> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
>> > > other interface types, too.
>> > >
>> > > This patch introduces IF_OTHER.  The patch after next uses it for an
>> > > EEPROM device.
>> > >
>> > > Do we want IF_OTHER?
>> >
>> > What would the semantics even be? Any block device that doesn't pick up
>> > a different category may pick up IF_OTHER backends?
>> >
>> > It certainly feels like a strange interface to ask for "other" disk and
>> > then getting as surprise what this other thing might be. It's
>> > essentially the same as having an explicit '-device other', and I
>> > suppose most people would find that strange.
>> >
>> > > If no, I guess we get to abuse IF_PFLASH some more.
>> > >
>> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
>> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
>> > > be worth the trouble, though.
>> > >
>> > > Thoughts?
>> >
>> > If the existing types aren't good enough (I don't have an opinion on
>> > whether IF_PFLASH is a good match), let's add a new one. But a specific
>> > new one, not just "other".
>> 
>> I think the common thread is "this isn't what anybody actually thinks
>> of as being a 'disk', but we would like to back it with a block device
>> anyway". That can cover a fair range of possibilities...
>
> How confident are we that no board will ever have two devices of this
> kind?
>
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.
>
> But if you have more than one device, it becomes hard to predict which
> device gets which backend - it depends on the initialisation order in
> the code then,

Really?  Board code should use IF_OTHER devices just like it uses the
other interface types, namely connecting each frontend device to a
backend device with a well-known and fixed interface type and index (or
bus and unit instead, where appropriate).

>and I'm pretty sure that this isn't something that should
> have significance in external interfaces and therefore become a stable
> API.

I agree that "implied by execution order" is a bad idea: commit
95fd260f0a "blockdev: Drop unused drive_get_next()".

Re: [PATCH v3 0/2] migration-test: Allow test to run without uffd

2022-07-28 Thread Thomas Huth


On 28/07/2022 15.35, Peter Xu wrote:

v2:
- Fix warning in patch 1 [Thomas]
- Collected R-b for Daniel

Compare to v1, this added a new patch as reported by Thomas to (hopefully)
allow auto-converge test to pass on some MacOS testbeds.

Please review, thanks.

Peter Xu (2):
   migration-test: Use migrate_ensure_converge() for auto-converge
   migration-test: Allow test to run without uffd

  tests/qtest/migration-test.c | 67 +++-
  1 file changed, 27 insertions(+), 40 deletions(-)


Seems to work now:

https://api.cirrus-ci.com/v1/task/4760264934424576/logs/build.log

Citing:

" 2/59 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test OK 
218.87s   33 subtests passed"


Tested-by: Thomas Huth

Re: [PATCH 1/1] block: add missed block_acct_setup with new block device init procedure

2022-07-28 Thread Vladimir Sementsov-Ogievskiy


On 7/11/22 14:07, Denis V. Lunev wrote:

Commit 5f76a7aac156ca75680dad5df4a385fd0b58f6b1 is looking harmless from
the first glance, but it has changed things a lot. 'libvirt' uses it to
detect that it should follow new initialization way and this changes
things considerably. With this procedure followed, blockdev_init() is
not called anymore and thus block_acct_setup() helper is not called.


I'm not sure that 5f76a7aac156ca is really the corner stone.. But yes,
libvirt moved to "blockdev era", which means that we don't use old -drive,
instead block nodes are created by -blockdev / qmp: blockdev-add, and attached
to block devices by node-name.

And if I understand correctly blockdev_init() is called only on -drive path.

I have some questions:

1. After this patch, don't we call block_acct_setup() twice on old path with 
-drive? That seems safe as block_acct_setup just assign fields of 
BlockAcctStats.. But that's doesn't look good.

2. Do we really need these options? Could we instead just enable accounting 
invalid and failed ops unconditionally? I doubt that someone will learn that 
these new options appeared and will use them to disable the failed/invalid 
accounting again.




This means in particular that defaults for block accounting statistics
are changed and account_invalid/account_failed are actually initialized
as false instead of true originally.

This commit changes things to match original world. It adds
account_invalid/account_failed properties to BlockConf and setups them
accordingly.

Signed-off-by: Denis V. Lunev 
CC: Peter Krempa 
CC: Markus Armbruster 
CC: John Snow 
CC: Kevin Wolf 
CC: Hanna Reitz 
---
  hw/block/block.c   |  2 +
  include/hw/block/block.h   |  7 +++-
  tests/qemu-iotests/172.out | 76 ++
  3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 25f45df723..53b100cdc3 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -205,6 +205,8 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool 
readonly,

 BlockdevOnError rerror;

  blk_set_enable_write_cache(blk, wce);
  blk_set_on_error(blk, rerror, werror);
  
+block_acct_setup(blk_get_stats(blk), conf->account_invalid,

+ conf->account_failed);
  return true;
  }
  
diff --git a/include/hw/block/block.h b/include/hw/block/block.h

index 5902c0440a..ffd439fc83 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -31,6 +31,7 @@ typedef struct BlockConf {
  uint32_t lcyls, lheads, lsecs;
  OnOffAuto wce;
  bool share_rw;
+bool account_invalid, account_failed;
  BlockdevOnError rerror;
  BlockdevOnError werror;
  } BlockConf;
@@ -61,7 +62,11 @@ static inline unsigned int get_physical_block_exp(BlockConf 
*conf)
 _conf.discard_granularity, -1),  \
  DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce,   \
  ON_OFF_AUTO_AUTO),  \
-DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
+DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false),\
+DEFINE_PROP_BOOL("account-invalid", _state, \
+ _conf.account_invalid, true),  \
+DEFINE_PROP_BOOL("account-failed", _state,  \
+ _conf.account_failed, true)
  
  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)  \

  DEFINE_PROP_DRIVE("drive", _state, _conf.blk),  \
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index 9479b92185..a6c451e098 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -28,6 +28,8 @@ Formatting 'TEST_DIR/t.IMGFMT.3', fmt=IMGFMT size=737280
  discard_granularity = 4294967295 (4 GiB)
  write-cache = "auto"
  share-rw = false
+account-invalid = true
+account-failed = true
  drive-type = "288"
  
  
@@ -55,6 +57,8 @@ Testing: -fda TEST_DIR/t.qcow2

  discard_granularity = 4294967295 (4 GiB)
  write-cache = "auto"
  share-rw = false
+account-invalid = true
+account-failed = true
  drive-type = "144"
  floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
  Attached to:  /machine/unattached/device[N]
@@ -92,6 +96,8 @@ Testing: -fdb TEST_DIR/t.qcow2
  discard_granularity = 4294967295 (4 GiB)
  write-cache = "auto"
  share-rw = false
+account-invalid = true
+account-failed = true
  drive-type = "144"
dev: floppy, id ""
  unit = 0 (0x0)
@@ -104,6 +110,8 @@ Testing: -fdb TEST_DIR/t.qcow2
  discard_granularity = 4294967295 (4

[PATCH for-7.1?] kvm: don't use perror() without useful errno

2022-07-28 Thread Cornelia Huck

perror() is designed to append the decoded errno value to a
string. This, however, only makes sense if we called something that
actually sets errno prior to that.

For the callers that check for split irqchip support that is not the
case, and we end up with confusing error messages that end in
"success". Use error_report() instead.

Signed-off-by: Cornelia Huck 
---

Not sure if that is still 7.1 material; on the one hand, it's a small
fix; on the other hand, it has been like that forever...

I've kept the Arm-specific message in place, although it might be redundant.

---
 accel/kvm/kvm-all.c | 2 +-
 target/arm/kvm.c| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 99aede73b7cb..6955c0b23a22 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2265,7 +2265,7 @@ static void kvm_irqchip_create(KVMState *s)
 ret = kvm_arch_irqchip_create(s);
 if (ret == 0) {
 if (s->kernel_irqchip_split == ON_OFF_AUTO_ON) {
-perror("Split IRQ chip mode not supported.");
+error_report("Split IRQ chip mode not supported.");
 exit(1);
 } else {
 ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 4339e1cd6e08..e5c1bd50d29b 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -959,7 +959,7 @@ void kvm_arch_init_irq_routing(KVMState *s)
 int kvm_arch_irqchip_create(KVMState *s)
 {
 if (kvm_kernel_irqchip_split()) {
-perror("-machine kernel_irqchip=split is not supported on ARM.");
+error_report("-machine kernel_irqchip=split is not supported on ARM.");
 exit(1);
 }
 
-- 
2.35.3

Re: [PATCH 3/3] dump: support cancel dump process

2022-07-28 Thread Kevin Wolf

Am 28.07.2022 um 14:37 hat Marc-André Lureau geschrieben:
> Hi
> 
> On Wed, Jul 27, 2022 at 6:02 PM Hogan Wang via 
> wrote:
> 
> > Break saving pages or dump iterate when dump job in cancel state,
> > make sure dump process exits as soon as possible.
> >
> > Signed-off-by: Hogan Wang 
> >
> 
> Overall, the series looks good to me. Please send it with a top cover
> letter, so it can be processed by patchew too.
> 
> I am not familiar with the job infrastructure, it would be nice if Kevin
> could check the usage or give some advice.

There is one big point I see with this series, which is that it could be
considered an incompatible change to 'dump-guest-memory'. Clients are
expected to manage the job now. Though in practice with the default
settings, maybe it actually just results in clients receiving additional
QMP events. (Technically, it is still incompatible because the command
will now fail if you have another job called 'memory-guest-dump' - no
good reason to have that, but it's a scenario that worked and breaks
after this series.)

Markus, do you have an opinion on whether job creation must be
explicitly enabled with a new option or if we can just switch existing
callers?

The implementation of a very basic job looks mostly okay to me, though
of course it doesn't support a few things like pausing the job and
proper progress monitoring. But these things are optional, so it's not a
blocker.

The one thing I would strongly recommend to include is an auto-dismiss
option like every other job has. It is required so that management tools
can query the final job status before it goes away. Most of the
information is in QMP events, too, but events can be lost. auto-finalize
isn't required for this job because it doesn't actually do anything in
the finalize phase.

I'm not sure how safe the whole thing is when it runs in the background
and you can send additional QMP commands while it's running, but that is
preexisting with detach=true.

Kevin

Re: Question to mem-path support at QEMU for Xen

On Thu, 28 Jul 2022 15:17:49 +0800
Huang Rui  wrote:

> Hi Igor,
> 
> Appreciate you for the reply!
> 
> On Wed, Jul 27, 2022 at 04:19:30PM +0800, Igor Mammedov wrote:
> > On Tue, 26 Jul 2022 15:27:07 +0800
> > Huang Rui  wrote:
> >   
> > > Hi Anthony and other Qemu/Xen guys,
> > > 
> > > We are trying to enable venus on Xen virtualization platform. And we would
> > > like to use the backend memory with memory-backend-memfd,id=mem1,size=4G
> > > options on QEMU, however, the QEMU will tell us the "-mem-path" is not
> > > supported with Xen. I verified the same function on KVM.
> > > 
> > > qemu-system-i386: -mem-path not supported with Xen
> > > 
> > > So may I know whether Xen has any limitation that support
> > > memory-backend-memfd in QEMU or just implementation is not done yet?  
> > 
> > Currently Xen doesn't use guest RAM allocation the way the rest of
> > accelerators do. (it has hacks in memory_region/ramblock API,
> > that divert RAM allocation calls to Xen specific API)  
> 
> I am new for Xen and QEMU, we are working on GPU. We would like to have a
> piece of backend memroy like video memory for VirtIO GPU to support guest
> VM Mesa Vulkan (Venus). Do you mean we can the memory_region/ramblock APIs
> to work around it?
> 
> > 
> > The sane way would extend Xen to accept RAM regions (whatever they are
> > ram or fd based) QEMU allocates instead of going its own way. This way
> > it could reuse all memory backends that QEMU provides for the rest of
> > the non-Xen world. (not to mention that we could drop non trivial
> > Xen hacks so that guest RAM handling would be consistent with other
> > accelerators)
> >   
> 
> May I know what do you mean by "going its own way"? This sounds good, could
> you please elaborate on how can we implement this? We would like to give a
> try to address the problem on Xen. Would you mind to point somewhere that I
> can learn and understand the RAM region. Very happy to see your
> suggestions!

see for example see ram_block_add(), if Xen could be persuaded to use memory
allocated by '!xen_enabled()' branch then it's likely file base backends
would also become usable.

Whether it is possible for Xen or not I don't know,
I guess CCed Xen folks will suggest something useful.
 
> Thanks & Best Regards,
> Ray
>

Re: [PATCH 5/6] hw/loongarch: Add acpi ged support

On Tue, 12 Jul 2022 16:32:05 +0800
Xiaojuan Yang  wrote:

> Loongarch virt machine uses general hardware reduces acpi method, rather
> than LS7A acpi device. Now only power management function is used in
> acpi ged device, memory hotplug will be added later. Also acpi tables
> such as RSDP/RSDT/FADT etc.
> 
> The acpi table has submited to acpi spec, and will release soon.
> 
> Signed-off-by: Xiaojuan Yang 
> ---
>  hw/loongarch/Kconfig|   2 +
>  hw/loongarch/acpi-build.c   | 609 
>  hw/loongarch/loongson3.c|  78 -
>  hw/loongarch/meson.build|   1 +
>  include/hw/loongarch/virt.h |  13 +
>  include/hw/pci-host/ls7a.h  |   4 +
>  6 files changed, 704 insertions(+), 3 deletions(-)
>  create mode 100644 hw/loongarch/acpi-build.c
> 
> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
> index 610552e522..a99aa387c3 100644
> --- a/hw/loongarch/Kconfig
> +++ b/hw/loongarch/Kconfig
> @@ -15,3 +15,5 @@ config LOONGARCH_VIRT
>  select LOONGARCH_EXTIOI
>  select LS7A_RTC
>  select SMBIOS
> +select ACPI_PCI
> +select ACPI_HW_REDUCED
> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
> new file mode 100644
> index 00..b95b83b079
> --- /dev/null
> +++ b/hw/loongarch/acpi-build.c
[...]


Most of the following code copied from x86 which is needlessly
complicated for loongarch wich doesn't have all that legacy to care about,
see ARM's variant virt_acpi_setup() for a cleaner example and
drop not needed parts.

> +static void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> +{
> +LoongArchMachineState *lams = LOONGARCH_MACHINE(machine);
> +GArray *table_offsets;
> +AcpiFadtData fadt_data;
> +unsigned facs, rsdt, fadt, dsdt;
> +uint8_t *u;
> +size_t aml_len = 0;
> +GArray *tables_blob = tables->table_data;
> +
> +init_common_fadt_data(_data);
> +
> +table_offsets = g_array_new(false, true, sizeof(uint32_t));
> +ACPI_BUILD_DPRINTF("init ACPI tables\n");
> +
> +bios_linker_loader_alloc(tables->linker,
> + ACPI_BUILD_TABLE_FILE, tables_blob,
> + 64, false);
> +
> +/*
> + * FACS is pointed to by FADT.
> + * We place it first since it's the only table that has alignment
> + * requirements.
> + */
> +facs = tables_blob->len;
> +build_facs(tables_blob);
> +
> +/* DSDT is pointed to by FADT */
> +dsdt = tables_blob->len;
> +build_dsdt(tables_blob, tables->linker, machine);
> +
> +/*
> + * Count the size of the DSDT, we will need it for
> + * legacy sizing of ACPI tables.
> + */
> +aml_len += tables_blob->len - dsdt;
> +
> +/* ACPI tables pointed to by RSDT */
> +fadt = tables_blob->len;
> +acpi_add_table(table_offsets, tables_blob);
> +fadt_data.facs_tbl_offset = 
> +fadt_data.dsdt_tbl_offset = 
> +fadt_data.xdsdt_tbl_offset = 
> +build_fadt(tables_blob, tables->linker, _data,
> +   lams->oem_id, lams->oem_table_id);
> +aml_len += tables_blob->len - fadt;
> +
> +acpi_add_table(table_offsets, tables_blob);
> +build_madt(tables_blob, tables->linker, lams);
> +
> +acpi_add_table(table_offsets, tables_blob);
> +build_srat(tables_blob, tables->linker, machine);
> +
> +acpi_add_table(table_offsets, tables_blob);
> +{
> +AcpiMcfgInfo mcfg = {
> +   .base = cpu_to_le64(LS_PCIECFG_BASE),
> +   .size = cpu_to_le64(LS_PCIECFG_SIZE),
> +};
> +build_mcfg(tables_blob, tables->linker, , lams->oem_id,
> +   lams->oem_table_id);
> +}
> +
> +/* Add tables supplied by user (if any) */
> +for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
> +unsigned len = acpi_table_len(u);
> +
> +acpi_add_table(table_offsets, tables_blob);
> +g_array_append_vals(tables_blob, u, len);
> +}
> +
> +/* RSDT is pointed to by RSDP */
> +rsdt = tables_blob->len;
> +build_rsdt(tables_blob, tables->linker, table_offsets,
> +   lams->oem_id, lams->oem_table_id);
> +
> +/* RSDP is in FSEG memory, so allocate it separately */
> +{
> +AcpiRsdpData rsdp_data = {
> +.revision = 0,
> +.oem_id = lams->oem_id,
> +.xsdt_tbl_offset = NULL,
> +.rsdt_tbl_offset = ,
> +};
> +build_rsdp(tables->rsdp, tables->linker, _data);
> +}
> +
> +/*
> + * The align size is 128, warn if 64k is not enough therefore
> + * the align size could be resized.
> + */
> +if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
> +warn_report("ACPI table size %u exceeds %d bytes,"
> +" migration may not work",
> +tables_blob->len, ACPI_BUILD_TABLE_SIZE / 2);
> +error_printf("Try removing CPUs, NUMA nodes, memory slots"
> + " or PCI bridges.");
> +}
> +
> +

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

2022-07-28 Thread Cédric Le Goater

On 7/28/22 15:29, Kevin Wolf wrote:

Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:

On Wed, 27 Jul 2022 at 20:03, Kevin Wolf wrote:

Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:

An OTP device isn't really a parallel flash, and neither are eFuses.
More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
other interface types, too.

This patch introduces IF_OTHER. The patch after next uses it for an
EEPROM device.

Do we want IF_OTHER?

What would the semantics even be? Any block device that doesn't pick up
a different category may pick up IF_OTHER backends?

It certainly feels like a strange interface to ask for "other" disk and
then getting as surprise what this other thing might be. It's
essentially the same as having an explicit '-device other', and I
suppose most people would find that strange.

If no, I guess we get to abuse IF_PFLASH some more.

If yes, I guess we should use IF_PFLASH only for actual parallel flash
memory going forward. Cleaning up existing abuse of IF_PFLASH may not
be worth the trouble, though.

Thoughts?

If the existing types aren't good enough (I don't have an opinion on
whether IF_PFLASH is a good match), let's add a new one. But a specific
new one, not just "other".

I think the common thread is "this isn't what anybody actually thinks
of as being a 'disk', but we would like to back it with a block device
anyway". That can cover a fair range of possibilities...

How confident are we that no board will ever have two devices of this
kind?

The BMC machines have a lot of eeproms.

As long as every board has at most one, if=other is a bad user interface
in terms of descriptiveness, but still more or less workable as long as
you know what it means for the specific board you use.

But if you have more than one device, it becomes hard to predict which
device gets which backend - it depends on the initialisation order in
the code then, and I'm pretty sure that this isn't something that should
have significance in external interfaces and therefore become a stable
API.

Can't we use the drive index ?

There has been various attempts to solve this problem for the Aspeed
machines also. See below. May be we should introduce and IF_EEPROM for
the purpose.

Thanks,

[PATCH v2] aspeed: qcom: add block backed FRU devices
https://www.mail-archive.com/qemu-devel@nongnu.org/msg900485.html

[PATCH] aspeed: Enable backend file for eeprom
http://patchwork.ozlabs.org/project/qemu-devel/patch/20220728061228.152704-1-wangzhiqian...@inspur.com/

[PATCH v2 1/4] hw/virtio: incorporate backend features in features

There are some extra bits used over a vhost-user connection which are
hidden from the device itself. We need to set them here to ensure we
enable things like the protocol extensions.

Currently net/vhost-user.c has it's own inscrutable way of persisting
this data but it really should live in the core vhost_user code.

Signed-off-by: Alex Bennée 
Message-Id: <20220726192150.2435175-7-alex.ben...@linaro.org>
---
 hw/virtio/vhost-user.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 75b8df21a4..1936a44e82 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1460,7 +1460,14 @@ static int vhost_user_set_features(struct vhost_dev *dev,
  */
 bool log_enabled = features & (0x1ULL << VHOST_F_LOG_ALL);
 
-return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
+/*
+ * We need to include any extra backend only feature bits that
+ * might be needed by our device. Currently this includes the
+ * VHOST_USER_F_PROTOCOL_FEATURES bit for enabling protocol
+ * features.
+ */
+return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES,
+  features | dev->backend_features,
   log_enabled);
 }
 
-- 
2.30.2

[PATCH v2 2/4] hw/virtio: gracefully handle unset vhost_dev vdev

I've noticed asserts firing because we query the status of vdev after
a vhost connection is closed down. Rather than faulting on the NULL
indirect just quietly reply false.

Signed-off-by: Alex Bennée 
Message-Id: <20220726192150.2435175-8-alex.ben...@linaro.org>
---
 hw/virtio/vhost.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0827d631c0..f758f177bb 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -306,7 +306,7 @@ static inline void vhost_dev_log_resize(struct vhost_dev 
*dev, uint64_t size)
 dev->log_size = size;
 }
 
-static int vhost_dev_has_iommu(struct vhost_dev *dev)
+static bool vhost_dev_has_iommu(struct vhost_dev *dev)
 {
 VirtIODevice *vdev = dev->vdev;
 
@@ -316,8 +316,12 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
  * does not have IOMMU, there's no need to enable this feature
  * which may cause unnecessary IOTLB miss/update transactions.
  */
-return virtio_bus_device_iommu_enabled(vdev) &&
-   virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+if (vdev) {
+return virtio_bus_device_iommu_enabled(vdev) &&
+virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+} else {
+return false;
+}
 }
 
 static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
-- 
2.30.2

[PATCH v2 4/4] hw/virtio: fix vhost_user_read tracepoint

As reads happen in the callback we were never seeing them. We only
really care about the header so move the tracepoint to when the header
is complete.

Fixes: 6ca6d8ee9d (hw/virtio: add vhost_user_[read|write] trace points)
Signed-off-by: Alex Bennée 
Acked-by: Jason Wang 
Message-Id: <20220726192150.2435175-10-alex.ben...@linaro.org>
---
 hw/virtio/vhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1936a44e82..c0b50deaf2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -295,6 +295,8 @@ static int vhost_user_read_header(struct vhost_dev *dev, 
VhostUserMsg *msg)
 return -EPROTO;
 }
 
+trace_vhost_user_read(msg->hdr.request, msg->hdr.flags);
+
 return 0;
 }
 
@@ -544,8 +546,6 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, 
uint64_t base,
 }
 }
 
-trace_vhost_user_read(msg.hdr.request, msg.hdr.flags);
-
 return 0;
 }
 
-- 
2.30.2

[PATCH for 7.1 v2 0/4] virtio fixes (split from virtio-gpio series)

Hi,

This is just a split out series based on:

   Subject: [PATCH v3 for 7.2 00/21] virtio-gpio and various virtio cleanups
   Date: Tue, 26 Jul 2022 20:21:29 +0100
   Message-Id: <20220726192150.2435175-1-alex.ben...@linaro.org>

with the patches identified by mst:

  Right. Still I think some are fixes we should merge now.
  I am thinking patches 5, 7,8,9 ? 6 if it makes backporting
  much easier. WDYT? If you agree pls separate bugfixes in
  series I can apply. Thanks!

but I've dropped the first patch as that was actually a backend and
I've added one of Jason's acked-bys.

Alex Bennée (4):
  hw/virtio: incorporate backend features in features
  hw/virtio: gracefully handle unset vhost_dev vdev
  hw/virtio: handle un-configured shutdown in virtio-pci
  hw/virtio: fix vhost_user_read tracepoint

 hw/virtio/vhost-user.c | 13 ++---
 hw/virtio/vhost.c  | 10 +++---
 hw/virtio/virtio-pci.c |  9 +++--
 3 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.30.2

Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

2022-07-28 Thread Daniel P . Berrangé

On Thu, Jul 28, 2022 at 02:40:22PM +0100, Peter Maydell wrote:
> On Thu, 28 Jul 2022 at 14:30, Markus Armbruster  wrote:
> > Peter Maydell  writes:
> > I applaud the renaissance of roman-style inscriptions, but it's not just
> > words without spaces, it's also capital letters only:
> >
> > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR
> >
> > Seriously, throw in some dashes or spaces.
> 
>   any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr

On the basis that virtualization gives you turtles all the way down...

 -device isa-applesmc,osk=

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

Re: virtio: why no full reset on virtio_set_status 0 ?

2022-07-28 Thread Michael S. Tsirkin

On Thu, Jul 28, 2022 at 11:09:15AM +0200, Claudio Fontana wrote:
> On 7/28/22 09:43, Claudio Fontana wrote:
> > On 7/28/22 03:27, Jason Wang wrote:
> >> On Wed, Jul 27, 2022 at 11:32 PM Michael S. Tsirkin  
> >> wrote:
> >>>
> >>> On Wed, Jul 27, 2022 at 12:51:31PM +0200, Claudio Fontana wrote:
>  Hi Michael and all,
> 
>  I have started researching a qemu / ovs / dpdk bug:
> 
>  https://inbox.dpdk.org/dev/322122fb-619d-96f6-5c3e-9eabdbf38...@redhat.com/T/
> 
>  that seems to be affecting multiple parties in the telco space,
> 
>  and during this process I noticed that qemu/hw/virtio/virtio.c does not 
>  do a full virtio reset
>  in virtio_set_status, when receiving a status value of 0.
> 
>  It seems it has always been this way, so I am clearly missing / 
>  forgetting something basic,
> 
>  I checked the virtio spec at https://docs.oasis-open.org/
> 
>  and from:
> 
>  "
>  4.1.4.3 Common configuration structure layout
> 
>  device_status
>  The driver writes the device status here (see 2.1). Writing 0 into this 
>  field resets the device.
> 
>  "
> 
>  and
> 
>  "
>  2.4.1 Device Requirements: Device Reset
>  A device MUST reinitialize device status to 0 after receiving a reset.
>  "
> 
>  I would conclude that in virtio.c::virtio_set_status we should 
>  unconditionally do a full virtio_reset.
> 
>  Instead, we have just the check:
> 
>  if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) !=
>  (val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>  virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
>  }
> 
>  which just sets the started field,
> 
>  and then we have the call to the virtio device class set_status 
>  (virtio_net...),
>  but the VirtioDevice is not fully reset, as per the virtio_reset() call 
>  we are missing:
> 
>  "
>  vdev->start_on_kick = false;
>  vdev->started = false;
>  vdev->broken = false;
>  vdev->guest_features = 0;
>  vdev->queue_sel = 0;
>  vdev->status = 0;
>  vdev->disabled = false;
>  qatomic_set(>isr, 0);
>  vdev->config_vector = VIRTIO_NO_VECTOR;
>  virtio_notify_vector(vdev, vdev->config_vector);
> 
>  for(i = 0; i < VIRTIO_QUEUE_MAX; i++) {
>  ... initialize vdev->vq[i] ...
>  }
>  "
> 
>  Doing a full reset seems to fix the problem for me, so I can send 
>  tentative patches if necessary,
>  but what am I missing here?
> 
>  Thanks,
> 
>  Claudio
> 
>  --
>  Claudio Fontana
>  Engineering Manager Virtualization, SUSE Labs Core
> 
>  SUSE Software Solutions Italy Srl
> >>>
> >>>
> >>> So for example for pci:
> >>>
> >>> case VIRTIO_PCI_STATUS:
> >>>
> >>>
> >>> 
> >>>
> >>> if (vdev->status == 0) {
> >>> virtio_pci_reset(DEVICE(proxy));
> >>> }
> >>>
> >>> which I suspect is a bug because:
> >>>
> >>> static void virtio_pci_reset(DeviceState *qdev)
> >>> {
> >>> VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> >>> VirtioBusState *bus = VIRTIO_BUS(>bus);
> >>> PCIDevice *dev = PCI_DEVICE(qdev);
> >>> int i;
> >>>
> >>> virtio_bus_reset(bus);
> >>
> >> Note that we do virtio_reset() here.
> > 
> > 
> > Yes, thank you, I completely overlooked it, I noticed this in Michael's 
> > response as well.
> > 
> > However we end up with multiple calls to k->set_status, one from the 
> > virtio_set_status call,
> > and one from the virtio_bus_reset(), which is probably something we don't 
> > want.
> > 
> > All in all it is not clear what the meaning of virtio_set_status is 
> > supposed to be I think,
> > and I wonder what the assumptions are among all the callers.
> > If it is supposed to be an implementation of the virtio standard field as 
> > described, I think we should do the reset right then and there,
> > but maybe the true meaning of the function is another one I couldn't 
> > understand, since _some_ of the cases are processes there.
> > 
> > And there is a question about ordering:
> > 
> > in virtio_pci we end up calling virtio_set_status(0), which gets us 
> > k->set_status(vdev, 0), which lands in virtio_net_set_status(0) and 
> > virtio_net_vhost_status,
> > which causes a vhost_net_stop().
> > 
> > Should we instead land in virtio_net_reset() first, by doing a virtio reset 
> > earlier when detecting a 0 value from the driver?
> > 
> > in the scenario I am looking at (with vhost-user, ovs/dpdk, and a guest 
> > testpmd application),
> > the guest application goes away without any chance to signal (kill -9), 
> > then gets immediately restarted and does a write of 0 to status, while qemu 
> > and ovs still hold the state for the device.
> > 
> > As QEMU lands in vhost_net_stop(), it seems to cause a chain of events

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

On Thu, 28 Jul 2022 at 14:30, Kevin Wolf  wrote:
>
> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> > On Wed, 27 Jul 2022 at 20:03, Kevin Wolf  wrote:
> > > If the existing types aren't good enough (I don't have an opinion on
> > > whether IF_PFLASH is a good match), let's add a new one. But a specific
> > > new one, not just "other".
> >
> > I think the common thread is "this isn't what anybody actually thinks
> > of as being a 'disk', but we would like to back it with a block device
> > anyway". That can cover a fair range of possibilities...
>
> How confident are we that no board will ever have two devices of this
> kind?
>
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.

Extremely non-confident, but that holds for all these things,
unless we add new IF_* for every possible new thing:
 IF_PFLASH
 IF_EEPROM
 IF_EFUSE
 IF_BBRAM
 ...
?

thanks
-- PMM

[PATCH v2 3/4] hw/virtio: handle un-configured shutdown in virtio-pci

The assert() protecting against leakage is a little aggressive and
causes needless crashes if a device is shutdown without having been
configured. In this case no descriptors are lost because none have
been assigned.

Signed-off-by: Alex Bennée 
Message-Id: <20220726192150.2435175-9-alex.ben...@linaro.org>
---
 hw/virtio/virtio-pci.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 45327f0b31..5ce61f9b45 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -996,9 +996,14 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, 
int nvqs, bool assign)
 
 nvqs = MIN(nvqs, VIRTIO_QUEUE_MAX);
 
-/* When deassigning, pass a consistent nvqs value
- * to avoid leaking notifiers.
+/*
+ * When deassigning, pass a consistent nvqs value to avoid leaking
+ * notifiers. But first check we've actually been configured, exit
+ * early if we haven't.
  */
+if (!assign && !proxy->nvqs_with_notifiers) {
+return 0;
+}
 assert(assign || nvqs == proxy->nvqs_with_notifiers);
 
 proxy->nvqs_with_notifiers = nvqs;
-- 
2.30.2

[PATCH for-7.1] tests: acpi: silence applesmc warning about invalid key

OSK value is irrelevant for ACPI test case.
Supply fake OSK explicitly to prevent QEMU complaining about
invalid key when it fallbacks to default_osk.

Suggested-by: Daniel P. Berrangé 
Signed-off-by: Igor Mammedov 
---
 tests/qtest/bios-tables-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 359916c228..7c5f736b51 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1632,7 +1632,9 @@ static void test_acpi_q35_applesmc(void)
 .variant = ".applesmc",
 };
 
-test_acpi_one("-device isa-applesmc", );
+/* supply fake 64-byte OSK to silence missing key warning */
+test_acpi_one("-device isa-applesmc,osk=any64characterfakeoskisenough"
+  "topreventinvalidkeywarningsonstderr", );
 free_test_data();
 }
 
-- 
2.31.1

Re: [PATCH for-7.2] hw: Add compat machines for 7.2

2022-07-28 Thread Michael S. Tsirkin

On Wed, Jul 27, 2022 at 02:17:55PM +0200, Cornelia Huck wrote:
> Add 7.2 machine types for arm/i440fx/m68k/q35/s390x/spapr.
> 
> Signed-off-by: Cornelia Huck 

Reviewed-by: Michael S. Tsirkin 

whoever needs this first, feel free to merge.

> ---
>  hw/arm/virt.c  |  9 -
>  hw/core/machine.c  |  3 +++
>  hw/i386/pc.c   |  3 +++
>  hw/i386/pc_piix.c  | 14 +-
>  hw/i386/pc_q35.c   | 13 -
>  hw/m68k/virt.c |  9 -
>  hw/ppc/spapr.c | 15 +--
>  hw/s390x/s390-virtio-ccw.c | 14 +-
>  include/hw/boards.h|  3 +++
>  include/hw/i386/pc.h   |  3 +++
>  10 files changed, 79 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9633f822f361..1a6480fd2a76 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -3094,10 +3094,17 @@ static void machvirt_machine_init(void)
>  }
>  type_init(machvirt_machine_init);
>  
> +static void virt_machine_7_2_options(MachineClass *mc)
> +{
> +}
> +DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
> +
>  static void virt_machine_7_1_options(MachineClass *mc)
>  {
> +virt_machine_7_2_options(mc);
> +compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
>  }
> -DEFINE_VIRT_MACHINE_AS_LATEST(7, 1)
> +DEFINE_VIRT_MACHINE(7, 1)
>  
>  static void virt_machine_7_0_options(MachineClass *mc)
>  {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a673302ccec5..aa520e74a8c8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -40,6 +40,9 @@
>  #include "hw/virtio/virtio-pci.h"
>  #include "qom/object_interfaces.h"
>  
> +GlobalProperty hw_compat_7_1[] = {};
> +const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> +
>  GlobalProperty hw_compat_7_0[] = {
>  { "arm-gicv3-common", "force-8-bit-prio", "on" },
>  { "nvme-ns", "eui64-default", "on"},
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 774cb2bf0748..31724c42ac90 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -107,6 +107,9 @@
>  { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
>  { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
>  
> +GlobalProperty pc_compat_7_1[] = {};
> +const size_t pc_compat_7_1_len = G_N_ELEMENTS(pc_compat_7_1);
> +
>  GlobalProperty pc_compat_7_0[] = {};
>  const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0);
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a234989ac363..34fa0021c7be 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -424,7 +424,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
>  machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
>  }
>  
> -static void pc_i440fx_7_1_machine_options(MachineClass *m)
> +static void pc_i440fx_7_2_machine_options(MachineClass *m)
>  {
>  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>  pc_i440fx_machine_options(m);
> @@ -433,6 +433,18 @@ static void pc_i440fx_7_1_machine_options(MachineClass 
> *m)
>  pcmc->default_cpu_version = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
> +  pc_i440fx_7_2_machine_options);
> +
> +static void pc_i440fx_7_1_machine_options(MachineClass *m)
> +{
> +pc_i440fx_7_2_machine_options(m);
> +m->alias = NULL;
> +m->is_default = false;
> +compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
> +compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
> +}
> +
>  DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
>pc_i440fx_7_1_machine_options);
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index f96cbd04e284..38634cd11413 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -362,7 +362,7 @@ static void pc_q35_machine_options(MachineClass *m)
>  m->max_cpus = 288;
>  }
>  
> -static void pc_q35_7_1_machine_options(MachineClass *m)
> +static void pc_q35_7_2_machine_options(MachineClass *m)
>  {
>  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>  pc_q35_machine_options(m);
> @@ -370,6 +370,17 @@ static void pc_q35_7_1_machine_options(MachineClass *m)
>  pcmc->default_cpu_version = 1;
>  }
>  
> +DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
> +   pc_q35_7_2_machine_options);
> +
> +static void pc_q35_7_1_machine_options(MachineClass *m)
> +{
> +pc_q35_7_2_machine_options(m);
> +m->alias = NULL;
> +compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
> +compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
> +}
> +
>  DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
> pc_q35_7_1_machine_options);
>  
> diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
> index 0aa383fa6bda..3122c8ef2c38 100644
> --- a/hw/m68k/virt.c
> +++ b/hw/m68k/virt.c
> @@ -322,10 +322,17 @@ type_init(virt_machine_register_types)
>  } \
>  type_init(machvirt_machine_##major##_##minor##_init);

Re: [PATCH v4 11/17] dump/dump: Add section string table support

Hi

On Tue, Jul 26, 2022 at 6:26 PM Janosch Frank  wrote:

> On 7/26/22 15:12, Marc-André Lureau wrote:
> > On Tue, Jul 26, 2022 at 4:55 PM Janosch Frank 
> wrote:
> >
> >> On 7/26/22 13:25, Marc-André Lureau wrote:
> >>> Hi
> >>>
> >>> On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank 
> >> wrote:
> 
>  As sections don't have a type like the notes do we need another way to
>  determine their contents. The string table allows us to assign each
>  section an identification string which architectures can then use to
>  tag their sections with.
> 
>  There will be no string table if the architecture doesn't add custom
>  sections which are introduced in a following patch.
> 
>  Signed-off-by: Janosch Frank 
> [...]
> >> [..]
> s->length = length;
>  +/* First index is 0, it's the special null name */
>  +s->string_table_buf = g_array_new(FALSE, TRUE, 1);
>  +/*
>  + * Allocate the null name, due to the clearing option set to true
>  + * it will be 0.
>  + */
>  +g_array_set_size(s->string_table_buf, 1);
> >>>
> >>> I wonder if GByteArray wouldn't be more appropriate, even if it
> >>> doesn't have the clearing option. If it's just for one byte, ...
> >>
> >> I don't really care but I need a decision on it to change it :)
> >>
> >
> > I haven't tried, but I think it would be a better fit.
>
> Looking at this a second time there's an issue you should consider:
>
> GByteArray uses guint8 while the GArray uses gchars which are apparently
> compatible with normal C chars.
>
> I.e. I need to cast all strings to (const guint8 *) when appending them
> to the GByteArray.
>

Agh, boring.. well, we also have include/qemu/buffer.h that could be
considered perhaps

-- 
Marc-André Lureau

[PATCH v3 1/2] migration-test: Use migrate_ensure_converge() for auto-converge

Thomas reported that auto-converge test will timeout on MacOS CI gatings.
Use the migrate_ensure_converge() helper too in the auto-converge as when
Daniel reworked the other test cases.

Since both max_bandwidth / downtime_limit will not be used for converge
calculations, make it simple by removing the remaining check, then we can
completely remove both variables altogether, since migrate_ensure_converge
is used the remaining check won't make much sense anyway.

Suggested-by: Daniel P. Berrange 
Reported-by: Thomas Huth 
Reviewed-by: Daniel P. Berrange 
Signed-off-by: Peter Xu 
---
 tests/qtest/migration-test.c | 19 ++-
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 71595a74fd..c1e002087d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1768,7 +1768,7 @@ static void test_migrate_auto_converge(void)
 g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
 MigrateStart args = {};
 QTestState *from, *to;
-int64_t remaining, percentage;
+int64_t percentage;
 
 /*
  * We want the test to be stable and as fast as possible.
@@ -1776,14 +1776,6 @@ static void test_migrate_auto_converge(void)
  * so we need to decrease a bandwidth.
  */
 const int64_t init_pct = 5, inc_pct = 50, max_pct = 95;
-const int64_t max_bandwidth = 4; /* ~400Mb/s */
-const int64_t downtime_limit = 250; /* 250ms */
-/*
- * We migrate through unix-socket (> 500Mb/s).
- * Thus, expected migration speed ~= bandwidth limit (< 500Mb/s).
- * So, we can predict expected_threshold
- */
-const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000;
 
 if (test_migrate_start(, , uri, )) {
 return;
@@ -1818,8 +1810,7 @@ static void test_migrate_auto_converge(void)
 /* The first percentage of throttling should be equal to init_pct */
 g_assert_cmpint(percentage, ==, init_pct);
 /* Now, when we tested that throttling works, let it converge */
-migrate_set_parameter_int(from, "downtime-limit", downtime_limit);
-migrate_set_parameter_int(from, "max-bandwidth", max_bandwidth);
+migrate_ensure_converge(from);
 
 /*
  * Wait for pre-switchover status to check last throttle percentage
@@ -1830,11 +1821,6 @@ static void test_migrate_auto_converge(void)
 /* The final percentage of throttling shouldn't be greater than max_pct */
 percentage = read_migrate_property_int(from, "cpu-throttle-percentage");
 g_assert_cmpint(percentage, <=, max_pct);
-
-remaining = read_ram_property_int(from, "remaining");
-g_assert_cmpint(remaining, <,
-(expected_threshold + expected_threshold / 100));
-
 migrate_continue(from, "pre-switchover");
 
 qtest_qmp_eventwait(to, "RESUME");
@@ -1842,7 +1828,6 @@ static void test_migrate_auto_converge(void)
 wait_for_serial("dest_serial");
 wait_for_migration_complete(from);
 
-
 test_migrate_end(from, to, true);
 }
 
-- 
2.32.0

[PATCH] vga: fix incorrect line height in 640x200x2 mode

When in CGA modes, QEMU wants to ignore the maximum scan field (bits 0..4) of
the maximum scan length register in the CRTC.  It is not clear why this is
needed---for example, Bochs ignores bit 7 instead.  The issue is that the
CGA modes are not detected correctly, and in particular mode 6 results in
multi_scan==3 according to how SeaBIOS programs it.  The right way to check
for CGA graphics modes is to check whether bit 13 of the address is special
cased by the CRT controller to achieve line interleaving, i.e. whether bit 0
of the CRTC mode control register is clear.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1020
Reported-by: Korneliusz Osmenda 
Signed-off-by: Paolo Bonzini 
---
 hw/display/vga.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 5dca2d1528..50ecb1ad02 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1514,9 +1514,10 @@ static void vga_draw_graphic(VGACommonState *s, int 
full_update)
 force_shadow = true;
 }
 
+/* bits 5-6: 0 = 16-color mode, 1 = 4-color mode, 2 = 256-color mode.  */
 shift_control = (s->gr[VGA_GFX_MODE] >> 5) & 3;
 double_scan = (s->cr[VGA_CRTC_MAX_SCAN] >> 7);
-if (shift_control != 1) {
+if (s->cr[VGA_CRTC_MODE] & 1) {
 multi_scan = (((s->cr[VGA_CRTC_MAX_SCAN] & 0x1f) + 1) << double_scan)
 - 1;
 } else {
-- 
2.36.1

Re: [PATCH for-7.1] tests: acpi: silence applesmc warning about invalid key

2022-07-28 Thread Daniel P . Berrangé

On Thu, Jul 28, 2022 at 09:37:13AM -0400, Igor Mammedov wrote:
> OSK value is irrelevant for ACPI test case.
> Supply fake OSK explicitly to prevent QEMU complaining about
> invalid key when it fallbacks to default_osk.
> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Igor Mammedov 
> ---
>  tests/qtest/bios-tables-test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 359916c228..7c5f736b51 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1632,7 +1632,9 @@ static void test_acpi_q35_applesmc(void)
>  .variant = ".applesmc",
>  };
>  
> -test_acpi_one("-device isa-applesmc", );
> +/* supply fake 64-byte OSK to silence missing key warning */
> +test_acpi_one("-device isa-applesmc,osk=any64characterfakeoskisenough"
> +  "topreventinvalidkeywarningsonstderr", );
>  free_test_data();
>  }

All 64 chars present and correct.

Reviewed-by: Daniel P. Berrangé 


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

[PATCH v3 0/2] migration-test: Allow test to run without uffd

v2:
- Fix warning in patch 1 [Thomas]
- Collected R-b for Daniel

Compare to v1, this added a new patch as reported by Thomas to (hopefully)
allow auto-converge test to pass on some MacOS testbeds.

Please review, thanks.

Peter Xu (2):
  migration-test: Use migrate_ensure_converge() for auto-converge
  migration-test: Allow test to run without uffd

 tests/qtest/migration-test.c | 67 +++-
 1 file changed, 27 insertions(+), 40 deletions(-)

-- 
2.32.0

Re: [PATCH 4/5] util/qemu-sockets: Enable unix socket support on Windows

2022-07-28 Thread Bin Meng

On Thu, Jul 28, 2022 at 9:11 PM Marc-André Lureau
 wrote:
>
> Hi
>
> On Wed, Jul 27, 2022 at 2:05 PM Bin Meng  wrote:
>>
>> On Wed, Jul 27, 2022 at 4:53 PM Konstantin Kostiuk  
>> wrote:
>> >
>> >
>> >
>> >
>> >
>> > On Wed, Jul 27, 2022 at 10:47 AM Bin Meng  wrote:
>> >>
>> >> From: Bin Meng 
>> >>
>> >> Support for the unix socket has existed both in BSD and Linux for the
>> >> longest time, but not on Windows. Since Windows 10 build 17063 [1],
>> >> the native support for the unix socket has came to Windows. Starting
>> >> this build, two Win32 processes can use the AF_UNIX address family
>> >> over Winsock API to communicate with each other.
>> >>
>> >> Introduce a new build time config option CONFIG_AF_UNIX when the build
>> >> host has such a capability, and a run-time check afunix_available() for
>> >> Windows host in the QEMU sockets util codes.
>> >>
>> >> [1] https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
>> >>
>> >> Signed-off-by: Xuzhou Cheng 
>> >> Signed-off-by: Bin Meng 
>> >> ---
>> >>
>> >>  meson.build |  6 ++
>> >>  util/qemu-sockets.c | 48 ++---
>> >>  2 files changed, 47 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/meson.build b/meson.build
>> >> index 75aaca8462..73e5de5957 100644
>> >> --- a/meson.build
>> >> +++ b/meson.build
>> >> @@ -2327,6 +2327,12 @@ have_afalg = get_option('crypto_afalg') \
>> >>'''), error_message: 'AF_ALG requested but could not be 
>> >> detected').allowed()
>> >>  config_host_data.set('CONFIG_AF_ALG', have_afalg)
>> >>
>> >> +if targetos != 'windows'
>> >> +  config_host_data.set('CONFIG_AF_UNIX', true)
>> >> +else
>> >> +  config_host_data.set('CONFIG_AF_UNIX', cc.has_header('afunix.h'))
>> >> +endif
>> >> +
>> >>  config_host_data.set('CONFIG_AF_VSOCK', cc.has_header_symbol(
>> >>'linux/vm_sockets.h', 'AF_VSOCK',
>> >>prefix: '#include ',
>> >> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> >> index 0e2298278f..d85f3ea3ee 100644
>> >> --- a/util/qemu-sockets.c
>> >> +++ b/util/qemu-sockets.c
>> >> @@ -17,6 +17,15 @@
>> >>   */
>> >>  #include "qemu/osdep.h"
>> >>
>> >> +#if defined(CONFIG_WIN32) && defined(CONFIG_AF_UNIX)
>> >> +# include 
>> >> +/*
>> >> + * AF_UNIX support is available since Windows 10 build 17063
>> >> + * See 
>> >> https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
>> >> + */
>> >> +# define WIN_BUILD_AF_UNIX 17063
>> >> +#endif /* CONFIG_WIN32 && CONFIG_AF_UNIX */
>> >> +
>> >>  #ifdef CONFIG_AF_VSOCK
>> >>  #include 
>> >>  #endif /* CONFIG_AF_VSOCK */
>> >> @@ -880,7 +889,7 @@ static int vsock_parse(VsockSocketAddress *addr, 
>> >> const char *str,
>> >>  }
>> >>  #endif /* CONFIG_AF_VSOCK */
>> >>
>> >> -#ifndef _WIN32
>> >> +#ifdef CONFIG_AF_UNIX
>> >>
>> >>  static bool saddr_is_abstract(UnixSocketAddress *saddr)
>> >>  {
>> >> @@ -900,6 +909,17 @@ static bool saddr_is_tight(UnixSocketAddress *saddr)
>> >>  #endif
>> >>  }
>> >>
>> >> +#ifdef CONFIG_WIN32
>> >> +static bool afunix_available(void)
>> >> +{
>> >> +OSVERSIONINFOEXW os_version = { 0 };
>> >> +
>> >> +os_get_win_version(_version);
>> >> +
>> >> +return os_version.dwBuildNumber >= WIN_BUILD_AF_UNIX;
>> >
>> >
>> > I think this is a bad variant to check feature support by checking
>> > Windows build. From my point, you should try to create an AF_UNIX
>> > socket and if it fails then fall back to the old behavior.
>> >
>>
>> The caller intends to create an AF_UNIX socket, and if Windows does
>> not have the capability, it fails, and we return -1 to the caller.
>> I am not sure what old behavior we should fall back to.
>>
>
> I agree with Konstantin, we shouldn't check the Windows version, but assume 
> the socket creation can work, and just report a regular error if not.
>
> (you can drop some of the preliminary patch then)
>

Sure, will do in v3.

Regards,
Bin

Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

On Thu, 28 Jul 2022 15:29:58 +0200
Markus Armbruster  wrote:

> Peter Maydell  writes:
> 
> > On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé  
> > wrote:  
> >>
> >> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:  
> >> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé  
> >> > wrote:  
> >> > >
> >> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:  
> >> > > > QEMU probably can't carry OSK key[1] for legal reasons so it
> >> > > > can't supply the valid default key. However when tests are run
> >> > > > applesmc will pollute test log with distracting warning,
> >> > > > silence that warning so it won't distract maintainers/CI.  
> >> > >
> >> > > What test is causing this problem ?  
> >> >
> >> > bios-tables-test -- see here for the relevant bit of the log:
> >> >
> >> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
> >> >   
> >>
> >> The right fix is for bios-tables-tests to pass an explicit 'osk' value
> >> then. As its a test it doesn't have to be a genuine OSK, jsut any old
> >> 64-byte string
> >>
> >> diff --git a/tests/qtest/bios-tables-test.c 
> >> b/tests/qtest/bios-tables-test.c
> >> index 359916c228..f6b5adf200 100644
> >> --- a/tests/qtest/bios-tables-test.c
> >> +++ b/tests/qtest/bios-tables-test.c
> >> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
> >>  .variant = ".applesmc",
> >>  };
> >>
> >> -test_acpi_one("-device isa-applesmc", );
> >> +test_acpi_one("-device 
> >> isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
> >>  );
> >>  free_test_data();  
> >
> > We should either have a comment saying that this has to be exactly
> > 64 characters and it doesn't matter what they are; or we could use
> >  any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr
> >
> > :-)  
> 
> I applaud the renaissance of roman-style inscriptions, but it's not just
> words without spaces, it's also capital letters only:
> 
> ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR
> 
> Seriously, throw in some dashes or spaces.

too late, but added a comment above hieroglyphs style string

>

Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

On Thu, 28 Jul 2022 at 14:30, Markus Armbruster  wrote:
> Peter Maydell  writes:
> I applaud the renaissance of roman-style inscriptions, but it's not just
> words without spaces, it's also capital letters only:
>
> ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR
>
> Seriously, throw in some dashes or spaces.

  any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr

-- PMM

qtest/libqos: How to find the free PCI slots in a qtest instance?

2022-07-28 Thread Frenkel, Sheindy

Hi,
I'm trying to find out how I can get the free PCI slots in a qtest code.
I want to assign a PCI device to a qtest-mode VM. If I assign this device to an 
unavailable address in the qemu process, I get this assertion: PCI: slot x 
function y not available for z.
I'm just wondering if I can avoid this error by checking the free PCI slot list 
with the qtest API. Is it possible?

Thanks,
Sheindy

-
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3 2/2] migration-test: Allow test to run without uffd

We used to stop running all tests if uffd is not detected.  However
logically that's only needed for postcopy not the rest of tests.

Keep running the rest when still possible.

Reviewed-by: Daniel P. Berrange 
Signed-off-by: Peter Xu 
---
 tests/qtest/migration-test.c | 48 +++-
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index c1e002087d..10ab7a708c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2424,14 +2424,11 @@ int main(int argc, char **argv)
 {
 char template[] = "/tmp/migration-test-XX";
 const bool has_kvm = qtest_has_accel("kvm");
+const bool has_uffd = ufd_version_check();
 int ret;
 
 g_test_init(, , NULL);
 
-if (!ufd_version_check()) {
-return g_test_run();
-}
-
 /*
  * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
  * is touchy due to race conditions on dirty bits (especially on PPC for
@@ -2460,13 +2457,15 @@ int main(int argc, char **argv)
 
 module_call_init(MODULE_INIT_QOM);
 
-qtest_add_func("/migration/postcopy/unix", test_postcopy);
-qtest_add_func("/migration/postcopy/plain", test_postcopy);
-qtest_add_func("/migration/postcopy/recovery/plain",
-   test_postcopy_recovery);
-qtest_add_func("/migration/postcopy/preempt/plain", test_postcopy_preempt);
-qtest_add_func("/migration/postcopy/preempt/recovery/plain",
-test_postcopy_preempt_recovery);
+if (has_uffd) {
+qtest_add_func("/migration/postcopy/unix", test_postcopy);
+qtest_add_func("/migration/postcopy/plain", test_postcopy);
+qtest_add_func("/migration/postcopy/recovery/plain",
+   test_postcopy_recovery);
+qtest_add_func("/migration/postcopy/preempt/plain", 
test_postcopy_preempt);
+qtest_add_func("/migration/postcopy/preempt/recovery/plain",
+   test_postcopy_preempt_recovery);
+}
 
 qtest_add_func("/migration/bad_dest", test_baddest);
 qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
@@ -2474,18 +2473,21 @@ int main(int argc, char **argv)
 #ifdef CONFIG_GNUTLS
 qtest_add_func("/migration/precopy/unix/tls/psk",
test_precopy_unix_tls_psk);
-/*
- * NOTE: psk test is enough for postcopy, as other types of TLS
- * channels are tested under precopy.  Here what we want to test is the
- * general postcopy path that has TLS channel enabled.
- */
-qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
-qtest_add_func("/migration/postcopy/recovery/tls/psk",
-   test_postcopy_recovery_tls_psk);
-qtest_add_func("/migration/postcopy/preempt/tls/psk",
-   test_postcopy_preempt_tls_psk);
-qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
-   test_postcopy_preempt_all);
+
+if (has_uffd) {
+/*
+ * NOTE: psk test is enough for postcopy, as other types of TLS
+ * channels are tested under precopy.  Here what we want to test is the
+ * general postcopy path that has TLS channel enabled.
+ */
+qtest_add_func("/migration/postcopy/tls/psk", test_postcopy_tls_psk);
+qtest_add_func("/migration/postcopy/recovery/tls/psk",
+   test_postcopy_recovery_tls_psk);
+qtest_add_func("/migration/postcopy/preempt/tls/psk",
+   test_postcopy_preempt_tls_psk);
+qtest_add_func("/migration/postcopy/preempt/recovery/tls/psk",
+   test_postcopy_preempt_all);
+}
 #ifdef CONFIG_TASN1
 qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
test_precopy_unix_tls_x509_default_host);
-- 
2.32.0

Re: [PATCH v2 1/2] migration-test: Use migrate_ensure_converge() for auto-converge

On Thu, Jul 28, 2022 at 03:04:27PM +0200, Thomas Huth wrote:
> On 22/07/2022 16.56, Peter Xu wrote:
> > Thomas reported that auto-converge test will timeout on MacOS CI gatings.
> > Use the migrate_ensure_converge() helper too in the auto-converge as when
> > Daniel reworked the other test cases.
> > 
> > Since both max_bandwidth / downtime_limit will not be used for converge
> > calculations, make it simple by removing the remaining check, then we can
> > completely remove both variables altogether, since migrate_ensure_converge
> > is used the remaining check won't make much sense anyway.
> > 
> > Suggested-by: Daniel P. Berrange 
> > Reported-by: Thomas Huth 
> > Signed-off-by: Peter Xu 
> > ---
> >   tests/qtest/migration-test.c | 17 +
> >   1 file changed, 1 insertion(+), 16 deletions(-)
> > 
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 71595a74fd..dd50aa600c 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -1776,14 +1776,6 @@ static void test_migrate_auto_converge(void)
> >* so we need to decrease a bandwidth.
> >*/
> >   const int64_t init_pct = 5, inc_pct = 50, max_pct = 95;
> > -const int64_t max_bandwidth = 4; /* ~400Mb/s */
> > -const int64_t downtime_limit = 250; /* 250ms */
> > -/*
> > - * We migrate through unix-socket (> 500Mb/s).
> > - * Thus, expected migration speed ~= bandwidth limit (< 500Mb/s).
> > - * So, we can predict expected_threshold
> > - */
> > -const int64_t expected_threshold = max_bandwidth * downtime_limit / 
> > 1000;
> >   if (test_migrate_start(, , uri, )) {
> >   return;
> > @@ -1818,8 +1810,7 @@ static void test_migrate_auto_converge(void)
> >   /* The first percentage of throttling should be equal to init_pct */
> >   g_assert_cmpint(percentage, ==, init_pct);
> >   /* Now, when we tested that throttling works, let it converge */
> > -migrate_set_parameter_int(from, "downtime-limit", downtime_limit);
> > -migrate_set_parameter_int(from, "max-bandwidth", max_bandwidth);
> > +migrate_ensure_converge(from);
> >   /*
> >* Wait for pre-switchover status to check last throttle percentage
> > @@ -1830,11 +1821,6 @@ static void test_migrate_auto_converge(void)
> >   /* The final percentage of throttling shouldn't be greater than 
> > max_pct */
> >   percentage = read_migrate_property_int(from, 
> > "cpu-throttle-percentage");
> >   g_assert_cmpint(percentage, <=, max_pct);
> > -
> > -remaining = read_ram_property_int(from, "remaining");
> > -g_assert_cmpint(remaining, <,
> > -(expected_threshold + expected_threshold / 100));
> > -
> 
> I'm getting this on FreeBSD:
> 
> ../tests/qtest/migration-test.c:1771:13: error: unused variable 'remaining'
> [-Werror,-Wunused-variable]
> int64_t remaining, percentage;
> ^
> 1 error generated.
> 
> Did you try this with -Werror ?

Thanks for catching that, I'll start to do so and repost.

-- 
Peter Xu

Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

2022-07-28 Thread Kevin Wolf

Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf  wrote:
> >
> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> > > An OTP device isn't really a parallel flash, and neither are eFuses.
> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
> > > other interface types, too.
> > >
> > > This patch introduces IF_OTHER.  The patch after next uses it for an
> > > EEPROM device.
> > >
> > > Do we want IF_OTHER?
> >
> > What would the semantics even be? Any block device that doesn't pick up
> > a different category may pick up IF_OTHER backends?
> >
> > It certainly feels like a strange interface to ask for "other" disk and
> > then getting as surprise what this other thing might be. It's
> > essentially the same as having an explicit '-device other', and I
> > suppose most people would find that strange.
> >
> > > If no, I guess we get to abuse IF_PFLASH some more.
> > >
> > > If yes, I guess we should use IF_PFLASH only for actual parallel flash
> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH may not
> > > be worth the trouble, though.
> > >
> > > Thoughts?
> >
> > If the existing types aren't good enough (I don't have an opinion on
> > whether IF_PFLASH is a good match), let's add a new one. But a specific
> > new one, not just "other".
> 
> I think the common thread is "this isn't what anybody actually thinks
> of as being a 'disk', but we would like to back it with a block device
> anyway". That can cover a fair range of possibilities...

How confident are we that no board will ever have two devices of this
kind?

As long as every board has at most one, if=other is a bad user interface
in terms of descriptiveness, but still more or less workable as long as
you know what it means for the specific board you use.

But if you have more than one device, it becomes hard to predict which
device gets which backend - it depends on the initialisation order in
the code then, and I'm pretty sure that this isn't something that should
have significance in external interfaces and therefore become a stable
API.

Kevin

Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

2022-07-28 Thread Markus Armbruster

Peter Maydell  writes:

> On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé  wrote:
>>
>> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
>> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé  
>> > wrote:
>> > >
>> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
>> > > > QEMU probably can't carry OSK key[1] for legal reasons so it
>> > > > can't supply the valid default key. However when tests are run
>> > > > applesmc will pollute test log with distracting warning,
>> > > > silence that warning so it won't distract maintainers/CI.
>> > >
>> > > What test is causing this problem ?
>> >
>> > bios-tables-test -- see here for the relevant bit of the log:
>> >
>> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
>>
>> The right fix is for bios-tables-tests to pass an explicit 'osk' value
>> then. As its a test it doesn't have to be a genuine OSK, jsut any old
>> 64-byte string
>>
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>> index 359916c228..f6b5adf200 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
>>  .variant = ".applesmc",
>>  };
>>
>> -test_acpi_one("-device isa-applesmc", );
>> +test_acpi_one("-device 
>> isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
>>  );
>>  free_test_data();
>
> We should either have a comment saying that this has to be exactly
> 64 characters and it doesn't matter what they are; or we could use
>  any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr
>
> :-)

I applaud the renaissance of roman-style inscriptions, but it's not just
words without spaces, it's also capital letters only:

ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR

Seriously, throw in some dashes or spaces.

Re: [PATCH v3] target/ppc: Implement new wait variants





On 7/28/22 02:29, Joel Stanley wrote:

On Wed, 27 Jul 2022 at 13:49, Daniel Henrique Barboza
 wrote:




On 7/20/22 10:33, Nicholas Piggin wrote:

ISA v2.06 adds new variations of wait, specified by the WC field. These
are not all compatible with the prior wait implementation, because they
add additional conditions that cause the processor to resume, which can
cause software to hang or run very slowly.


So I suppose this is not a new feature, but a bug fix to remediate these hangs
cause by the incompatibility of the WC field  with other ISA versions. Is
that right?


That's the case. Nick has some kernel patches that make Linux use the
new opcode:

  https://lore.kernel.org/all/20220720132132.903462-1-npig...@gmail.com/

With these applied the kernel hangs during boot if more than one CPU
is present. I was able to reproduce with ppc64le_defconfig and this
command line:

  qemu-system-ppc64 -M pseries,x-vof=on -cpu POWER10 -smp 2 -nographic
-kernel zImage.pseries -no-reboot

Qemu will exit (as there's no filesystem) if the test "passes", or
hang during boot if it hits the bug.


Thanks for the explanation. I'll handle it as a bug fix then.


Also, Nick, down below:




There's a kernel here with the patches applied in case someone else
wants to test:

https://ozlabs.org/~joel/zImage.pseries-v5.19-rc8-wait-v3

Tested-by: Joel Stanley 

Because of the hang it would be best if we merged the patch as a fix
sooner rather than later.

Cheers,

Joel


I'm explicitly asking for it because if it's a bug fix it's ok to pick it
during the freeze. Especially here, given that what you're doing is mostly
adding no-ops for conditions that we're not covering.



ISA v3.0 changed the wait opcode and removed the new variants (retaining
the WC field but making non-zero values reserved).

ISA v3.1 added new WC values to the new wait opcode, and added a PL
field.

This implements the new wait encoding and supports WC variants with
no-op implementations, which provides basic correctness as explained in
comments.

Signed-off-by: Nicholas Piggin 
---
v3:
- Add EXTRACT_HELPERs
- Reserved fields should be ignored, not trap.
- v3.1 defines special case of reserved PL values being treated as
a no-op when WC=2.
- Change code organization to (hopefully) be easier to follow each
ISA / variation.
- Tested old wait variant with Linux e6500 boot and verify that
gen_wait is called and takes the expected path.

Thanks,
Nick

   target/ppc/internal.h  |  3 ++
   target/ppc/translate.c | 96 ++
   2 files changed, 91 insertions(+), 8 deletions(-)

diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 2add128cd1..57c0a42a6b 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -168,6 +168,9 @@ EXTRACT_HELPER_SPLIT_3(DX, 10, 6, 6, 5, 16, 1, 1, 0, 0)
   /* darn */
   EXTRACT_HELPER(L, 16, 2);
   #endif
+/* wait */
+EXTRACT_HELPER(WC, 21, 2);
+EXTRACT_HELPER(PL, 16, 2);

   /***Jump target decoding   
***/
   /* Immediate address */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 1d6daa4608..e0a835ac90 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4066,12 +4066,91 @@ static void gen_sync(DisasContext *ctx)
   /* wait */
   static void gen_wait(DisasContext *ctx)
   {
-TCGv_i32 t0 = tcg_const_i32(1);
-tcg_gen_st_i32(t0, cpu_env,
-   -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
-tcg_temp_free_i32(t0);
-/* Stop translation, as the CPU is supposed to sleep from now */
-gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
+uint32_t wc;
+
+if (ctx->insns_flags & PPC_WAIT) {
+/* v2.03-v2.07 define an older incompatible 'wait' encoding. */
+
+if (ctx->insns_flags2 & PPC2_PM_ISA206) {
+/* v2.06 introduced the WC field. WC > 0 may be treated as no-op. 
*/
+wc = WC(ctx->opcode);
+} else {
+wc = 0;
+}
+
+} else if (ctx->insns_flags2 & PPC2_ISA300) {
+/* v3.0 defines a new 'wait' encoding. */
+wc = WC(ctx->opcode);



The ISA seems to indicate that WC=3 is always reserved in both ISA300 and
ISA310. I believe you can check for WC=3 and gen_invalid() return right
off the bat at this point.



I had a change of heart about this comment. It's better to have each ISA version
handle their conditions in separate, even if they overlap, to make it easier to
extend the function later on if required.


This means that the patch LGTM, so


Reviewed-by: Daniel Henrique Barboza 






Thanks,


Daniel




+if (ctx->insns_flags2 & PPC2_ISA310) {
+uint32_t pl = PL(ctx->opcode);
+
+/* WC 1,2 may be treated as no-op. WC 3 is reserved. */
+if (wc == 3) {
+gen_invalid(ctx);
+return;
+}
+
+/* PL 1-3 are reserved. If WC=2 then the insn is treated as noop. 
*/
+if

Re: [PATCH] aspeed: Enable backend file for eeprom

2022-07-28 Thread Cédric Le Goater


On 7/28/22 09:20, John Wang wrote:

Cédric Le Goater  于2022年7月28日周四 14:28写道：


Hello John,

On 7/28/22 08:12, John Wang wrote:

tested on a fp5280g2:

$QEMU_BIN -machine fp5280g2-bmc \
 -nographic \
 -drive file="${IMAGE_PATH}",format=raw,if=mtd \
 -drive file="eeprom.bin",format=raw,if=pflash,index=1 \
 ${NIC}

root@fp5280g2:/sys/bus/i2c/devices/1-0050# hexdump eeprom -C
  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  ||
*


0240  2c 87 a3 a4 1d d3 11 b2  02 d2 c2 9d 44 60 cf 3e  |,...D`.>|
0250  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  ||

It's same as the "eeprom.bin"

Signed-off-by: John Wang 
Change-Id: I5c44785a028144b24aa0b22643266d83addc5eab
---
   hw/arm/aspeed.c | 16 
   1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 4193a3d23d..80aa687372 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -431,12 +431,20 @@ static void aspeed_machine_init(MachineState *machine)
   arm_load_kernel(ARM_CPU(first_cpu), machine, _board_binfo);
   }

-static void at24c_eeprom_init(I2CBus *bus, uint8_t addr, uint32_t rsize)
+static void at24c_eeprom_init(I2CBus *bus, uint8_t addr, uint32_t rsize,
+  int index)
   {
   I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
   DeviceState *dev = DEVICE(i2c_dev);

+DriveInfo *dinfo = drive_get_by_index(IF_PFLASH, index);


I don't think IF_PFLASH is the appropriate type.


thanks



Jae proposed a similar patch with IF_NONE which should fit your need :


https://lore.kernel.org/all/20220718175214.2087644-1-quic_jaeh...@quicinc.com/

Could you please give it a try ?


I tested on a fp5280g2-bmc, It's ok.  I would abandon my patch :）



It's available on my branch :

https://github.com/legoater/qemu/commits/aspeed-7.1


I checked it, and will use this tree to module a new machine. :)


Or simply grab the patch if you only need one.

I don't think this is the correct approach for mainline. See this thread :

  
https://lore.kernel.org/all/CAFEAcA8sNjLsknea5Nt-tANEniFF2FYmjiV0xz=pr+vfwkx...@mail.gmail.com/t/

C.





Thanks,

C.



+BlockBackend *blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
+
+if (blk) {
+qdev_prop_set_drive(DEVICE(dev), "drive", blk);
+}
   qdev_prop_set_uint32(dev, "rom-size", rsize);
+
   i2c_slave_realize_and_unref(i2c_dev, bus, _abort);
   }

@@ -685,7 +693,7 @@ static void fp5280g2_bmc_i2c_init(AspeedMachineState *bmc)
   I2CSlave *i2c_mux;

   /* The at24c256 */
-at24c_eeprom_init(aspeed_i2c_get_bus(>i2c, 1), 0x50, 32768);
+at24c_eeprom_init(aspeed_i2c_get_bus(>i2c, 1), 0x50, 32768, 1);

   /* The fp5280g2 expects a TMP112 but a TMP105 is compatible */
   i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 2), TYPE_TMP105,
@@ -918,13 +926,13 @@ static void bletchley_bmc_i2c_init(AspeedMachineState 
*bmc)
   }

   /* Bus 6 */
-at24c_eeprom_init(i2c[6], 0x56, 65536);
+at24c_eeprom_init(i2c[6], 0x56, 65536, 1);
   /* Missing model: nxp,pcf85263 @ 0x51 , but ds1338 works enough */
   i2c_slave_create_simple(i2c[6], "ds1338", 0x51);


   /* Bus 7 */
-at24c_eeprom_init(i2c[7], 0x54, 65536);
+at24c_eeprom_init(i2c[7], 0x54, 65536, 2);

   /* Bus 9 */
   i2c_slave_create_simple(i2c[9], TYPE_TMP421, 0x4f);

Re: [PATCH for-7.2] hw: Add compat machines for 7.2





On 7/27/22 09:17, Cornelia Huck wrote:

Add 7.2 machine types for arm/i440fx/m68k/q35/s390x/spapr.

Signed-off-by: Cornelia Huck 
---


Looking good for pseries.


Reviewed-by: Daniel Henrique Barboza 



  hw/arm/virt.c  |  9 -
  hw/core/machine.c  |  3 +++
  hw/i386/pc.c   |  3 +++
  hw/i386/pc_piix.c  | 14 +-
  hw/i386/pc_q35.c   | 13 -
  hw/m68k/virt.c |  9 -
  hw/ppc/spapr.c | 15 +--
  hw/s390x/s390-virtio-ccw.c | 14 +-
  include/hw/boards.h|  3 +++
  include/hw/i386/pc.h   |  3 +++
  10 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9633f822f361..1a6480fd2a76 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -3094,10 +3094,17 @@ static void machvirt_machine_init(void)
  }
  type_init(machvirt_machine_init);
  
+static void virt_machine_7_2_options(MachineClass *mc)

+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
+
  static void virt_machine_7_1_options(MachineClass *mc)
  {
+virt_machine_7_2_options(mc);
+compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
  }
-DEFINE_VIRT_MACHINE_AS_LATEST(7, 1)
+DEFINE_VIRT_MACHINE(7, 1)
  
  static void virt_machine_7_0_options(MachineClass *mc)

  {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index a673302ccec5..aa520e74a8c8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,6 +40,9 @@
  #include "hw/virtio/virtio-pci.h"
  #include "qom/object_interfaces.h"
  
+GlobalProperty hw_compat_7_1[] = {};

+const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
+
  GlobalProperty hw_compat_7_0[] = {
  { "arm-gicv3-common", "force-8-bit-prio", "on" },
  { "nvme-ns", "eui64-default", "on"},
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 774cb2bf0748..31724c42ac90 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -107,6 +107,9 @@
  { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
  { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
  
+GlobalProperty pc_compat_7_1[] = {};

+const size_t pc_compat_7_1_len = G_N_ELEMENTS(pc_compat_7_1);
+
  GlobalProperty pc_compat_7_0[] = {};
  const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0);
  
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c

index a234989ac363..34fa0021c7be 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -424,7 +424,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
  machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
  }
  
-static void pc_i440fx_7_1_machine_options(MachineClass *m)

+static void pc_i440fx_7_2_machine_options(MachineClass *m)
  {
  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
  pc_i440fx_machine_options(m);
@@ -433,6 +433,18 @@ static void pc_i440fx_7_1_machine_options(MachineClass *m)
  pcmc->default_cpu_version = 1;
  }
  
+DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,

+  pc_i440fx_7_2_machine_options);
+
+static void pc_i440fx_7_1_machine_options(MachineClass *m)
+{
+pc_i440fx_7_2_machine_options(m);
+m->alias = NULL;
+m->is_default = false;
+compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
+compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
+}
+
  DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
pc_i440fx_7_1_machine_options);
  
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c

index f96cbd04e284..38634cd11413 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -362,7 +362,7 @@ static void pc_q35_machine_options(MachineClass *m)
  m->max_cpus = 288;
  }
  
-static void pc_q35_7_1_machine_options(MachineClass *m)

+static void pc_q35_7_2_machine_options(MachineClass *m)
  {
  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
  pc_q35_machine_options(m);
@@ -370,6 +370,17 @@ static void pc_q35_7_1_machine_options(MachineClass *m)
  pcmc->default_cpu_version = 1;
  }
  
+DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,

+   pc_q35_7_2_machine_options);
+
+static void pc_q35_7_1_machine_options(MachineClass *m)
+{
+pc_q35_7_2_machine_options(m);
+m->alias = NULL;
+compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
+compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
+}
+
  DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
 pc_q35_7_1_machine_options);
  
diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c

index 0aa383fa6bda..3122c8ef2c38 100644
--- a/hw/m68k/virt.c
+++ b/hw/m68k/virt.c
@@ -322,10 +322,17 @@ type_init(virt_machine_register_types)
  } \
  type_init(machvirt_machine_##major##_##minor##_init);
  
+static void virt_machine_7_2_options(MachineClass *mc)

+{
+}
+DEFINE_VIRT_MACHINE(7, 2, true)
+
  static void virt_machine_7_1_options(MachineClass *mc)
  {
+virt_machine_7_2_options(mc);
+compat_props_add(mc->compat_props,

Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM

On Thu, 28 Jul 2022 13:29:07 +0100
Peter Maydell  wrote:

> On Thu, 28 Jul 2022 at 12:50, Igor Mammedov  wrote:
> >
> > QEMU crashes trying to save VMSTATE when only MIPS target are compiled in
> >   $ qemu-system-mips -monitor stdio
> >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> >   Segmentation fault (core dumped)
> >
> > It happens due to PIIX4_PM trying to parse hotplug vmstate structures
> > which are valid only for x86 and not for MIPS (as it requires ACPI
> > tables support which is not existent for ithe later)
> >
> > Issue was probably exposed by trying to cleanup/compile out unused
> > ACPI bits from MIPS target (but forgetting about migration bits).
> >
> > Disable compiled out features using compat properties as the least
> > risky way to deal with issue.
> >
> > Signed-off-by: Igor Mammedov   
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/995
> 
> > ---
> > PS:
> > another approach could be setting defaults to disabled state and
> > enabling them using compat props on PC machines (which is more
> > code to deal with => more risky) or continue with PIIX4_PM
> > refactoring to split x86-shism out (which I'm not really
> > interested in due to risk of regressions for not much of
> > benefit)
> > ---
> >  hw/mips/malta.c | 9 +
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index 7a0ec513b0..0e932988e0 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = {
> >  .instance_init = mips_malta_instance_init,
> >  };
> >
> > +GlobalProperty malta_compat[] = {
> > +{ "PIIX4_PM", "memory-hotplug-support", "off" },
> > +{ "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" },
> > +{ "PIIX4_PM", "acpi-root-pci-hotplug", "off" },
> > +{ "PIIX4_PM", "x-not-migrate-acpi-index", "true" },
> > +};  
> 
> Is there an easy way to assert in hw/acpi/piix4.c that if
> CONFIG_ACPI_PCIHP was not set then the board has initialized
> all these properties to the don't-use-hotplug state ?
> That would be a guard against similar bugs (though I suppose
> we probably aren't likely to add new piix4 boards...)

unfortunately new features still creep in 'pc' machine
ex: "acpi-root-pci-hotplug"), and I don't see an easy
way to compile that nor enforce that in the future.

Far from easy would be split piix4_pm on base/enhanced
classes so we wouldn't need x86 specific hacks in 'base'
variant (assuming 'enhanced' could maintain the current
VMSTATE to keep cross-version migration working).

> > +const size_t malta_compat_len = G_N_ELEMENTS(malta_compat);
> > +
> >  static void mips_malta_machine_init(MachineClass *mc)
> >  {
> >  mc->desc = "MIPS Malta Core LV";
> > @@ -1455,6 +1463,7 @@ static void mips_malta_machine_init(MachineClass *mc)
> >  mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
> >  #endif
> >  mc->default_ram_id = "mips_malta.ram";
> > +compat_props_add(mc->compat_props, malta_compat, malta_compat_len);
> >  }
> >
> >  DEFINE_MACHINE("malta", mips_malta_machine_init)
> > --
> > 2.31.1  
> 
> thanks
> -- PMM
>

Re: [PATCH v3 00/12] powernv: introduce pnv-phb base/proxy devices





On 7/27/22 14:28, Frederic Barrat wrote:



On 24/06/2022 10:49, Daniel Henrique Barboza wrote:

Hi,

This is the version 3 of the pnv-phb proxy device which has the
following main differences from v2:

- it's rebased on top of "[PATCH v3 0/8] pnv-phb related cleanups"
- it doesn't have any patches related to user-created devices

There is no user visible change made here yet. We're making device
changes that are effective using default settings.

Changes from v2:
- all related changes made with the rebase on top of "[PATCH v3 0/8]
pnv-phb related cleanups"
- the following user devices patches were removed:
   - ppc/pnv: user created pnv-phb for powernv8
   - ppc/pnv: user created pnv-phb for powernv9
   - ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs
   - ppc/pnv: user creatable pnv-phb for powernv10
- v2 link: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg06254.html



This series look pretty good to me! I only have a couple of minor comments, 
which I don't think are worth a resend.

   Fred



Thanks for the review! Patches queued in gitlab.com/danielhb/qemu/tree/ppc-7.2 .


Daniel






Daniel Henrique Barboza (12):
   ppc/pnv: add PHB3 bus init helper
   ppc/pnv: add PnvPHB base/proxy device
   ppc/pnv: turn PnvPHB3 into a PnvPHB backend
   ppc/pnv: add PHB4 bus init helper
   ppc/pnv: turn PnvPHB4 into a PnvPHB backend
   ppc/pnv: add pnv-phb-root-port device
   ppc/pnv: remove pnv-phb3-root-port
   ppc/pnv: remove pnv-phb4-root-port
   ppc/pnv: remove root port name from pnv_phb_attach_root_port()
   ppc/pnv: remove pecc->rp_model
   ppc/pnv: remove PnvPHB4.version
   ppc/pnv: move attach_root_port helper to pnv-phb.c

  hw/pci-host/meson.build    |   3 +-
  hw/pci-host/pnv_phb.c  | 244 +
  hw/pci-host/pnv_phb.h  |  55 
  hw/pci-host/pnv_phb3.c | 106 --
  hw/pci-host/pnv_phb4.c | 144 ---
  hw/pci-host/pnv_phb4_pec.c |   5 +-
  hw/ppc/pnv.c   |  68 -
  include/hw/pci-host/pnv_phb3.h |  12 +-
  include/hw/pci-host/pnv_phb4.h |  18 +--
  include/hw/ppc/pnv.h   |   5 +-
  10 files changed, 401 insertions(+), 259 deletions(-)
  create mode 100644 hw/pci-host/pnv_phb.c
  create mode 100644 hw/pci-host/pnv_phb.h

Re: [PATCH 4/5] util/qemu-sockets: Enable unix socket support on Windows

Hi

On Wed, Jul 27, 2022 at 2:05 PM Bin Meng  wrote:

> On Wed, Jul 27, 2022 at 4:53 PM Konstantin Kostiuk 
> wrote:
> >
> >
> >
> >
> >
> > On Wed, Jul 27, 2022 at 10:47 AM Bin Meng  wrote:
> >>
> >> From: Bin Meng 
> >>
> >> Support for the unix socket has existed both in BSD and Linux for the
> >> longest time, but not on Windows. Since Windows 10 build 17063 [1],
> >> the native support for the unix socket has came to Windows. Starting
> >> this build, two Win32 processes can use the AF_UNIX address family
> >> over Winsock API to communicate with each other.
> >>
> >> Introduce a new build time config option CONFIG_AF_UNIX when the build
> >> host has such a capability, and a run-time check afunix_available() for
> >> Windows host in the QEMU sockets util codes.
> >>
> >> [1]
> https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
> >>
> >> Signed-off-by: Xuzhou Cheng 
> >> Signed-off-by: Bin Meng 
> >> ---
> >>
> >>  meson.build |  6 ++
> >>  util/qemu-sockets.c | 48 ++---
> >>  2 files changed, 47 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/meson.build b/meson.build
> >> index 75aaca8462..73e5de5957 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -2327,6 +2327,12 @@ have_afalg = get_option('crypto_afalg') \
> >>'''), error_message: 'AF_ALG requested but could not be
> detected').allowed()
> >>  config_host_data.set('CONFIG_AF_ALG', have_afalg)
> >>
> >> +if targetos != 'windows'
> >> +  config_host_data.set('CONFIG_AF_UNIX', true)
> >> +else
> >> +  config_host_data.set('CONFIG_AF_UNIX', cc.has_header('afunix.h'))
> >> +endif
> >> +
> >>  config_host_data.set('CONFIG_AF_VSOCK', cc.has_header_symbol(
> >>'linux/vm_sockets.h', 'AF_VSOCK',
> >>prefix: '#include ',
> >> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> >> index 0e2298278f..d85f3ea3ee 100644
> >> --- a/util/qemu-sockets.c
> >> +++ b/util/qemu-sockets.c
> >> @@ -17,6 +17,15 @@
> >>   */
> >>  #include "qemu/osdep.h"
> >>
> >> +#if defined(CONFIG_WIN32) && defined(CONFIG_AF_UNIX)
> >> +# include 
> >> +/*
> >> + * AF_UNIX support is available since Windows 10 build 17063
> >> + * See
> https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
> >> + */
> >> +# define WIN_BUILD_AF_UNIX 17063
> >> +#endif /* CONFIG_WIN32 && CONFIG_AF_UNIX */
> >> +
> >>  #ifdef CONFIG_AF_VSOCK
> >>  #include 
> >>  #endif /* CONFIG_AF_VSOCK */
> >> @@ -880,7 +889,7 @@ static int vsock_parse(VsockSocketAddress *addr,
> const char *str,
> >>  }
> >>  #endif /* CONFIG_AF_VSOCK */
> >>
> >> -#ifndef _WIN32
> >> +#ifdef CONFIG_AF_UNIX
> >>
> >>  static bool saddr_is_abstract(UnixSocketAddress *saddr)
> >>  {
> >> @@ -900,6 +909,17 @@ static bool saddr_is_tight(UnixSocketAddress
> *saddr)
> >>  #endif
> >>  }
> >>
> >> +#ifdef CONFIG_WIN32
> >> +static bool afunix_available(void)
> >> +{
> >> +OSVERSIONINFOEXW os_version = { 0 };
> >> +
> >> +os_get_win_version(_version);
> >> +
> >> +return os_version.dwBuildNumber >= WIN_BUILD_AF_UNIX;
> >
> >
> > I think this is a bad variant to check feature support by checking
> > Windows build. From my point, you should try to create an AF_UNIX
> > socket and if it fails then fall back to the old behavior.
> >
>
> The caller intends to create an AF_UNIX socket, and if Windows does
> not have the capability, it fails, and we return -1 to the caller.
> I am not sure what old behavior we should fall back to.
>
>
I agree with Konstantin, we shouldn't check the Windows version, but assume
the socket creation can work, and just report a regular error if not.

(you can drop some of the preliminary patch then)

-- 
Marc-André Lureau

Re: [PATCH v3 05/12] ppc/pnv: turn PnvPHB4 into a PnvPHB backend





On 7/27/22 14:41, Frederic Barrat wrote:



On 24/06/2022 10:49, Daniel Henrique Barboza wrote:


diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 1df91971b8..b7273f386e 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -672,7 +672,14 @@ static void pnv_chip_power8_pic_print_info(PnvChip *chip, 
Monitor *mon)
  static int pnv_chip_power9_pic_print_info_child(Object *child, void *opaque)
  {
  Monitor *mon = opaque;
-    PnvPHB4 *phb4 = (PnvPHB4 *) object_dynamic_cast(child, TYPE_PNV_PHB4);
+    PnvPHB *phb =  (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB);
+    PnvPHB4 *phb4;
+
+    if (!phb) {
+    return 0;
+    }
+
+    phb4 = (PnvPHB4 *)phb->backend;
  if (phb4) {
  pnv_phb4_pic_print_info(phb4, mon);



The full code in pnv_chip_power9_pic_print_info_child() looks like this:

     PnvPHB *phb =  (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB);
     PnvPHB4 *phb4;

     if (!phb) {
     return 0;
     }

     phb4 = (PnvPHB4 *)phb->backend;

     if (phb4) {
     pnv_phb4_pic_print_info(phb4, mon);
     }

Which is correct. However, if I want to nitpick, phb->backend is defined when 
the PnvPHB object is realized, so I don't think we can get here with the pointer 
being null, so we could remove the second if statement for readability. The reason 
I mention it is that we don't take that much care in the 
pnv_chip_power8_pic_print_info() function just above, so it looks a bit odd.


Good point. I changed it to look like this:


static int pnv_chip_power9_pic_print_info_child(Object *child, void *opaque)
{
Monitor *mon = opaque;
PnvPHB *phb =  (PnvPHB *) object_dynamic_cast(child, TYPE_PNV_PHB);

if (!phb) {
return 0;
}

pnv_phb4_pic_print_info(PNV_PHB4(phb->backend), mon);

return 0;
}

phb->backend being either NULL or not a PHB4 object is serious enough to assert
out, so the PNV_PHB4() macro seems justified.


Thanks,


Daniel



In any case:
Reviewed-by: Frederic Barrat 

   Fred





@@ -2122,8 +2129,14 @@ static void pnv_machine_power9_class_init(ObjectClass 
*oc, void *data)
  PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
  static const char compat[] = "qemu,powernv9\0ibm,powernv";
+    static GlobalProperty phb_compat[] = {
+    { TYPE_PNV_PHB, "version", "4" },
+    };
+
  mc->desc = "IBM PowerNV (Non-Virtualized) POWER9";
  mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
+    compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat));
+
  xfc->match_nvt = pnv_match_nvt;
  mc->alias = "powernv";
@@ -2140,8 +2153,13 @@ static void pnv_machine_power10_class_init(ObjectClass 
*oc, void *data)
  XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc);
  static const char compat[] = "qemu,powernv10\0ibm,powernv";
+    static GlobalProperty phb_compat[] = {
+    { TYPE_PNV_PHB, "version", "5" },
+    };
+
  mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
  mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v2.0");
+    compat_props_add(mc->compat_props, phb_compat, G_N_ELEMENTS(phb_compat));
  pmc->compat = compat;
  pmc->compat_size = sizeof(compat);
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 90843ac3a9..f22253358f 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -18,6 +18,7 @@
  typedef struct PnvPhb4PecState PnvPhb4PecState;
  typedef struct PnvPhb4PecStack PnvPhb4PecStack;
  typedef struct PnvPHB4 PnvPHB4;
+typedef struct PnvPHB PnvPHB;
  typedef struct PnvChip PnvChip;
  /*
@@ -78,7 +79,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB4, PNV_PHB4)
  #define PCI_MMIO_TOTAL_SIZE    (0x1ull << 60)
  struct PnvPHB4 {
-    PCIExpressHost parent_obj;
+    DeviceState parent;
+
+    PnvPHB *phb_base;
  uint32_t chip_id;
  uint32_t phb_id;

Re: [PATCH v2 1/2] migration-test: Use migrate_ensure_converge() for auto-converge

2022-07-28 Thread Thomas Huth


On 22/07/2022 16.56, Peter Xu wrote:

Thomas reported that auto-converge test will timeout on MacOS CI gatings.
Use the migrate_ensure_converge() helper too in the auto-converge as when
Daniel reworked the other test cases.

Since both max_bandwidth / downtime_limit will not be used for converge
calculations, make it simple by removing the remaining check, then we can
completely remove both variables altogether, since migrate_ensure_converge
is used the remaining check won't make much sense anyway.

Suggested-by: Daniel P. Berrange 
Reported-by: Thomas Huth 
Signed-off-by: Peter Xu 
---
  tests/qtest/migration-test.c | 17 +
  1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 71595a74fd..dd50aa600c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1776,14 +1776,6 @@ static void test_migrate_auto_converge(void)
   * so we need to decrease a bandwidth.
   */
  const int64_t init_pct = 5, inc_pct = 50, max_pct = 95;
-const int64_t max_bandwidth = 4; /* ~400Mb/s */
-const int64_t downtime_limit = 250; /* 250ms */
-/*
- * We migrate through unix-socket (> 500Mb/s).
- * Thus, expected migration speed ~= bandwidth limit (< 500Mb/s).
- * So, we can predict expected_threshold
- */
-const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000;
  
  if (test_migrate_start(, , uri, )) {

  return;
@@ -1818,8 +1810,7 @@ static void test_migrate_auto_converge(void)
  /* The first percentage of throttling should be equal to init_pct */
  g_assert_cmpint(percentage, ==, init_pct);
  /* Now, when we tested that throttling works, let it converge */
-migrate_set_parameter_int(from, "downtime-limit", downtime_limit);
-migrate_set_parameter_int(from, "max-bandwidth", max_bandwidth);
+migrate_ensure_converge(from);
  
  /*

   * Wait for pre-switchover status to check last throttle percentage
@@ -1830,11 +1821,6 @@ static void test_migrate_auto_converge(void)
  /* The final percentage of throttling shouldn't be greater than max_pct */
  percentage = read_migrate_property_int(from, "cpu-throttle-percentage");
  g_assert_cmpint(percentage, <=, max_pct);
-
-remaining = read_ram_property_int(from, "remaining");
-g_assert_cmpint(remaining, <,
-(expected_threshold + expected_threshold / 100));
-


I'm getting this on FreeBSD:

../tests/qtest/migration-test.c:1771:13: error: unused variable 'remaining' 
[-Werror,-Wunused-variable]

int64_t remaining, percentage;
^
1 error generated.

Did you try this with -Werror ?

 Thomas

Re: [PATCH v2 5/6] chardev/char-socket: Update AF_UNIX for Windows

Hi

On Wed, Jul 27, 2022 at 5:28 PM Bin Meng  wrote:
>
> From: Bin Meng 
>
> Now that AF_UNIX has come to Windows, update the existing logic in
> qemu_chr_compute_filename() and qmp_chardev_open_socket() for Windows.
>
> Signed-off-by: Bin Meng 

lgtm,

Reviewed-by: Marc-André Lureau 

> ---
>
> Changes in v2:
> - drop #include  as it is now already included in osdep.h
>
>  chardev/char-socket.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index dc4e218eeb..14a56b7b13 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -557,7 +557,7 @@ static char *qemu_chr_compute_filename(SocketChardev *s)
>  const char *left = "", *right = "";
>
>  switch (ss->ss_family) {
> -#ifndef _WIN32
> +#ifdef CONFIG_AF_UNIX
>  case AF_UNIX:
>  return g_strdup_printf("unix:%s%s",
> ((struct sockaddr_un *)(ss))->sun_path,
> @@ -1372,10 +1372,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>  }
>
>  qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
> +#ifndef _WIN32
>  /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
>  if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) {
>  qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>  }
> +#endif
>
>  /*
>   * In the chardev-change special-case, we shouldn't register a new yank
> --
> 2.34.1
>

Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

2022-07-28 Thread Daniel P . Berrangé

On Thu, Jul 28, 2022 at 02:00:37PM +0200, Igor Mammedov wrote:
> On Thu, 28 Jul 2022 11:23:00 +0100
> Daniel P. Berrangé  wrote:
> 
> > On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote:
> > > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé  
> > > wrote:  
> > > >
> > > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:  
> > > > > QEMU probably can't carry OSK key[1] for legal reasons so it
> > > > > can't supply the valid default key. However when tests are run
> > > > > applesmc will pollute test log with distracting warning,
> > > > > silence that warning so it won't distract maintainers/CI.  
> > > >
> > > > What test is causing this problem ?  
> > > 
> > > bios-tables-test -- see here for the relevant bit of the log:
> > > 
> > > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=tkcx...@mail.gmail.com/
> > >   
> > 
> > The right fix is for bios-tables-tests to pass an explicit 'osk' value
> > then. As its a test it doesn't have to be a genuine OSK, jsut any old
> > 64-byte string
> > 
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index 359916c228..f6b5adf200 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void)
> >  .variant = ".applesmc",
> >  };
> >  
> > -test_acpi_one("-device isa-applesmc", );
> > +test_acpi_one("-device 
> > isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr",
> >  );
> >  free_test_data();
> >  }
> 
> that will work, care tho send a formal patch or should I take over?

Could you spin something up, as I'm not in a position to do formal
testing today.

> However we still have bogus default_osk, yes it will cause warning which
> typically nobody will see and end user will still end up with upset guest.
> Right thing would be to require osk explicitly and drop default completely.
> Users who actually run MacOS guest must be providing OSK explicitly already
> so they won't be affected and anyone else using default is broken anyways
> (whether QEMU started directly or through mgmt layer)

There are other patches onlist to make QEMU extract an osk from the
host hardware, which is ok because IIUC macOS allows you to run macOS
as a VM, provided the host is Apple hardware. 


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

Re: [PATCH v2 3/6] qga/commands-win32: Use os_get_win_version()