Re: [Qemu-devel] [QEMU-PPC] [PATCH v2] powerpc/spapr: Add host threads parameter to ibm, get_system_parameter

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190626051903.26829-1-sjitindarsi...@gmail.com/



Hi,

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

Message-id: 20190626051903.26829-1-sjitindarsi...@gmail.com
Type: series
Subject: [Qemu-devel] [QEMU-PPC] [PATCH v2] powerpc/spapr: Add host threads 
parameter to ibm, get_system_parameter

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
83350a8 powerpc/spapr: Add host threads parameter to ibm, get_system_parameter

=== OUTPUT BEGIN ===
ERROR: braces {} are necessary for all arms of this statement
#42: FILE: hw/ppc/spapr_rtas.c:243:
+if (!kvm_enabled())
[...]

ERROR: braces {} are necessary for all arms of this statement
#47: FILE: hw/ppc/spapr_rtas.c:248:
+if (!dir)
[...]

ERROR: braces {} are necessary for all arms of this statement
#72: FILE: hw/ppc/spapr_rtas.c:273:
+if (subcores)
[...]

total: 3 errors, 0 warnings, 71 lines checked

Commit 83350a8ff0c8 (powerpc/spapr: Add host threads parameter to ibm, 
get_system_parameter) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190626051903.26829-1-sjitindarsi...@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[Qemu-devel] [QEMU-PPC] [PATCH v2] powerpc/spapr: Add host threads parameter to ibm, get_system_parameter

[Suraj Jitindar Singh]

The ibm,get_system_parameter rtas call is used by the guest to retrieve
data relating to certain parameters of the system. The SPLPAR
characteristics option (token 20) is used to determin characteristics of
the environment in which the lpar will run.

It may be useful for a guest to know the number of physical host threads
present on the underlying system where it is being run. Add the
characteristic "HostThrs" to the SPLPAR Characteristics
ibm,get_system_parameter rtas call to expose this information to a
guest and provide an implementation which determines this information
based on the number of interrupt servers present in the device tree.

Signed-off-by: Suraj Jitindar Singh 

---

V1 -> V2:
- Take into account that the core may be operating in split core mode
  meaning a single core may be split into multiple subcores.
---
 hw/ppc/spapr_rtas.c | 59 +
 1 file changed, 59 insertions(+)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 5bc1a93271..9b15e7606b 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -229,6 +229,55 @@ static inline int sysparm_st(target_ulong addr, 
target_ulong len,
 return RTAS_OUT_SUCCESS;
 }
 
+#define CPUS_PATH   "/proc/device-tree/cpus/"
+#define SUBCORE_PATH"/sys/devices/system/cpu/subcores_per_core"
+
+static int rtas_get_num_host_threads(void)
+{
+int num_threads = -1;
+unsigned long len;
+const char *entry;
+char *buf;
+GDir *dir;
+
+if (!kvm_enabled())
+return 1;
+
+/* Read interrupt servers to determine number of threads per core */
+dir = g_dir_open(CPUS_PATH, 0, NULL);
+if (!dir)
+return -1;
+
+while ((entry = g_dir_read_name(dir))) {
+if (!strncmp(entry, "PowerPC,POWER", strlen("PowerPC,POWER"))) {
+char *path;
+
+path = g_strconcat(CPUS_PATH, entry, "/ibm,ppc-interrupt-server#s",
+   NULL);
+if (g_file_get_contents(path, , , NULL)) {
+num_threads = len / sizeof(int);
+g_free(buf);
+}
+
+g_free(path);
+break;
+}
+}
+
+g_dir_close(dir);
+
+/* Check if split core mode in use */
+if (g_file_get_contents(SUBCORE_PATH, , , NULL)) {
+int subcores = g_ascii_strtoll(buf, NULL, 10);
+
+if (subcores)
+num_threads /= subcores;
+g_free(buf);
+}
+
+return num_threads;
+}
+
 static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
   SpaprMachineState *spapr,
   uint32_t token, uint32_t nargs,
@@ -250,6 +299,16 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
   current_machine->ram_size / MiB,
   smp_cpus,
   max_cpus);
+int num_host_threads = rtas_get_num_host_threads();
+
+if (num_host_threads > 0) {
+char *hostthr_val, *old = param_val;
+
+hostthr_val = g_strdup_printf(",HostThrs=%d", num_host_threads);
+param_val = g_strconcat(param_val, hostthr_val, NULL);
+g_free(hostthr_val);
+g_free(old);
+}
 ret = sysparm_st(buffer, length, param_val, strlen(param_val) + 1);
 g_free(param_val);
 break;
-- 
2.13.6

Re: [Qemu-devel] [Qemu-ppc] [PATCH v10 6/6] ppc: spapr: Handle "ibm, nmi-register" and "ibm, nmi-interlock" RTAS calls

[Aravinda Prasad]

On Tuesday 25 June 2019 12:30 PM, Greg Kurz wrote:
> On Tue, 25 Jun 2019 11:46:06 +0530
> Aravinda Prasad  wrote:
> 
>> On Monday 24 June 2019 07:59 PM, Greg Kurz wrote:
>>> On Wed, 12 Jun 2019 14:51:38 +0530
>>> Aravinda Prasad  wrote:
>>>   
 This patch adds support in QEMU to handle "ibm,nmi-register"
 and "ibm,nmi-interlock" RTAS calls and sets the default
 value of SPAPR_CAP_FWNMI_MCE to SPAPR_CAP_ON for machine
 type 4.0.
  
>>>
>>> Next machine type is 4.1.  
>>
>> ok.
>>
>>>   
 The machine check notification address is saved when the
 OS issues "ibm,nmi-register" RTAS call.

 This patch also handles the case when multiple processors
 experience machine check at or about the same time by
 handling "ibm,nmi-interlock" call. In such cases, as per
 PAPR, subsequent processors serialize waiting for the first
 processor to issue the "ibm,nmi-interlock" call. The second
 processor that also received a machine check error waits
 till the first processor is done reading the error log.
 The first processor issues "ibm,nmi-interlock" call
 when the error log is consumed.

 Signed-off-by: Aravinda Prasad 
 ---
  hw/ppc/spapr.c |6 -
  hw/ppc/spapr_rtas.c|   63 
 
  include/hw/ppc/spapr.h |5 +++-
  3 files changed, 72 insertions(+), 2 deletions(-)

 diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
 index 3d6d139..213d493 100644
 --- a/hw/ppc/spapr.c
 +++ b/hw/ppc/spapr.c
 @@ -2946,6 +2946,9 @@ static void spapr_machine_init(MachineState *machine)
  /* Create the error string for live migration blocker */
  error_setg(>fwnmi_migration_blocker,
  "Live migration not supported during machine check 
 handling");
 +
 +/* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
 +spapr_fwnmi_register();  
>>>
>>> IIRC this was supposed to depend on SPAPR_CAP_FWNMI_MCE being ON.  
>>
>> Yes this is inside SPAPR_CAP_FWNMI_MCE check:
>>
>> if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_ON) {
>> /*
>>  * Ensure that the rtas image size is less than RTAS_ERROR_LOG_OFFSET
>>  * or else the rtas image will be overwritten with the rtas error log
>>  * when a machine check exception is encountered.
>>  */
>> g_assert(spapr->rtas_size < RTAS_ERROR_LOG_OFFSET);
>>
>> /* Resize rtas blob to accommodate error log */
>> spapr->rtas_size = RTAS_ERROR_LOG_MAX;
>>
>> /* Create the error string for live migration blocker */
>> error_setg(>fwnmi_migration_blocker,
>> "Live migration not supported during machine check handling");
>>
>> /* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
>> spapr_fwnmi_register();
>> }
>>
> 
> Oops my bad... sorry for the noise.
> 
>>
>>>   
  }
  
  spapr->rtas_blob = g_malloc(spapr->rtas_size);
 @@ -4408,7 +4411,7 @@ static void spapr_machine_class_init(ObjectClass 
 *oc, void *data)
  smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
  smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
  smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
 -smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
 +smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_ON;
  spapr_caps_add_properties(smc, _abort);
  smc->irq = _irq_dual;
  smc->dr_phb_enabled = true;
 @@ -4512,6 +4515,7 @@ static void 
 spapr_machine_3_1_class_options(MachineClass *mc)
  smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
  smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
  smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_OFF;
 +smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;  
>>>
>>> This should have been put into spapr_machine_4_0_class_options().  
>>
>> ok. I will change it.
>>
>>>
>>> But unless you manage to get this merged before soft-freeze (2019-07-02),
>>> I'm afraid this will be a 4.2 feature.  
>>
>> If there are no other comments, can this be merged to 4.1? I will send a
>> revised version with the above changes.
>>
> 
> This is David's call.

David, can you let me know if this can be merged to 4.1 with the above
minor changes?

> 
>> Regards,
>> Aravinda
>>
>>>   
  }
  
  DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
 diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
 index a015a80..e010cb2 100644
 --- a/hw/ppc/spapr_rtas.c
 +++ b/hw/ppc/spapr_rtas.c
 @@ -49,6 +49,7 @@
  #include "hw/ppc/fdt.h"
  #include "target/ppc/mmu-hash64.h"
  #include "target/ppc/mmu-book3s-v3.h"
 +#include "migration/blocker.h"
  
  static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState 
 *spapr,

Re: [Qemu-devel] [PATCH v2] deprecate -mem-path fallback to anonymous RAM

[Markus Armbruster]

Igor Mammedov  writes:

> Fallback might affect guest or worse whole host performance
> or functionality if backing file were used to share guest RAM
> with another process.
>
> Patch deprecates fallback so that we could remove it in future
> and ensure that QEMU will provide expected behavior and fail if
> it can't use user provided backing file.
>
> Signed-off-by: Igor Mammedov 
> ---
> v2:
>  * improve text language
> (Markus Armbruster )
>
>  numa.c   | 6 --
>  qemu-deprecated.texi | 9 +
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/numa.c b/numa.c
> index 91a29138a2..c15e53e92d 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -494,8 +494,10 @@ static void allocate_system_memory_nonnuma(MemoryRegion 
> *mr, Object *owner,
>  if (mem_prealloc) {
>  exit(1);
>  }
> -error_report("falling back to regular RAM allocation.");
> -
> +warn_report("falling back to regular RAM allocation");
> +error_printf("This is deprecated. Make sure that -mem-path "
> + " specified path has sufficient resources to 
> allocate"
> + " -m specified RAM amount or QEMU will fail to 
> start");

The "or QEMU will fail to start" is confusing, I'm afraid.  *This* QEMU
won't.  A future QEMU may.  Suggest to simply delete the confusing part.

>  /* Legacy behavior: if allocation failed, fall back to
>   * regular RAM allocation.
>   */
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 2fe9b72121..1b7f3b10dc 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -112,6 +112,15 @@ QEMU using implicit generic or board specific splitting 
> rule.
>  Use @option{memdev} with @var{memory-backend-ram} backend or @option{mem} (if
>  it's supported by used machine type) to define mapping explictly instead.
>  
> +@subsection -mem-path fallback to RAM (since 4.1)
> +Currently if guest RAM allocation from file pointed by @option{mem-path}
> +fails, QEMU falls back to allocating from RAM, which might result
> +in unpredictable behavior since the backing file specified by the user
> +is ignored. In the future, users will be responsible for making sure
> +the backing storage specified with @option{-mem-path} can actually provide
> +the guest RAM configured with @option{-m} and fail to start up if RAM 
> allocation
> +is unsuccessful.

Grammar nit: the subject of "fail to start up" is "users", oopsie.
Suggest ", and QEMU will fail to start up".

> +
>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)

With these tweaks:
Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [Qemu-block] [RFC] nvme: how to support multiple namespaces

[Markus Armbruster]

Klaus Birkelund  writes:

> On Mon, Jun 24, 2019 at 12:18:45PM +0200, Kevin Wolf wrote:
>> Am 24.06.2019 um 10:01 hat Klaus Birkelund geschrieben:
>> > On Thu, Jun 20, 2019 at 05:37:24PM +0200, Laszlo Ersek wrote:
>> > > On 06/17/19 10:12, Klaus Birkelund wrote:
>> > > > Hi all,
>> > > > 
>> > > > I'm thinking about how to support multiple namespaces in the NVMe
>> > > > device. My first idea was to add a "namespaces" property array to the
>> > > > device that references blockdevs, but as Laszlo writes below, this 
>> > > > might
>> > > > not be the best idea. It also makes it troublesome to add per-namespace
>> > > > parameters (which is something I will be required to do for other
>> > > > reasons). Some of you might remember my first attempt at this that
>> > > > included adding a new block driver (derived from raw) that could be
>> > > > given certain parameters that would then be stored in the image. But I
>> > > > understand that this is a no-go, and I can see why.
>> > > > 
>> > > > I guess the optimal way would be such that the parameters was something
>> > > > like:
>> > > > 
>> > > >-blockdev 
>> > > > raw,node-name=blk_ns1,file.driver=file,file.filename=blk_ns1.img
>> > > >-blockdev 
>> > > > raw,node-name=blk_ns2,file.driver=file,file.filename=blk_ns2.img
>> > > >-device nvme-ns,drive=blk_ns1,ns-specific-options 
>> > > > (nsfeat,mc,dlfeat)...
>> > > >-device nvme-ns,drive=blk_ns2,...
>> > > >-device nvme,...
>> > > > 
>> > > > My question is how to state the parent/child relationship between the
>> > > > nvme and nvme-ns devices. I've been looking at how ide and virtio does
>> > > > this, and maybe a "bus" is the right way to go?
>> > > 
>> > > I've added Markus to the address list, because of this question. No
>> > > other (new) comments from me on the thread starter at this time, just
>> > > keeping the full context.
>> > > 
>> > 
>> > Hi all,
>> > 
>> > I've succesfully implemented this by introducing a new 'nvme-ns' device
>> > model. The nvme device creates a bus named from the device id ('id'
>> > parameter) and the nvme-ns devices are then registered on this.
>> > 
>> > This results in an nvme device being creates like this (two namespaces
>> > example):
>> > 
>> >   -drive file=nvme0n1.img,if=none,id=disk1
>> >   -drive file=nvme0n2.img,if=none,id=disk2
>> >   -device nvme,serial=deadbeef,id=nvme0
>> >   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
>> >   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
>> > 
>> > How does that look as a way forward?
>> 
>> This looks very similar to what other devices do (one bus controller
>> that has multiple devices on its but), so I like it.
>> 
>> The thing that is special here is that -device nvme is already a block
>> device by itself that can take a drive property. So how does this play
>> together? Can I choose to either specify a drive directly for the nvme
>> device or nvme-ns devices, but when I do both, I will get an error? What
>> happens if I don't specify a drive for nvme, but also don't add nvme-ns
>> devices?
>> 
>
> Hi Kevin,
>
> Yes, the nvme device is already a block device. My current patch removes
> that property from the nvme device. I guess this breaks backward
> compatibiltiy. We could accept a drive for the nvme device only if no
> nvme-ns devices are configured and connected on the bus.

Sounds awful :)

> I'm not entirely sure on the spec, but my gut tells me that an nvme
> device without any namespaces is technically a valid device, although it
> is a bit useless.

So maybe the device actually consists of a controller part (no drive
property) and namespace parts (one drive property each).

If yes, then the existing nvme device model is flawed.  Suggest to
deprecate and start over.  This should be possible without duplicating
code.

The alternative is bad magic, like the one you sketched above.  We
usually come to regret such magic.

Whether the controller device should be a composite device containing
the namespace parts is a separate question.

> I will post my patch (as part of a larger series) and we can discuss it
> there.

Yes, please.

> Thanks for the feedback!
>
> Klaus

Re: [Qemu-devel] [Qemu-block] [RFC] nvme: how to support multiple namespaces

[Markus Armbruster]

Cc: QOM maintainers in case I'm talking nonsense about QOM.

Klaus Birkelund  writes:

> On Tue, Jun 25, 2019 at 07:51:29AM +0200, Markus Armbruster wrote:
>> Laszlo Ersek  writes:
>> 
>> > On 06/24/19 12:18, Kevin Wolf wrote:
>> >> Am 24.06.2019 um 10:01 hat Klaus Birkelund geschrieben:
>> >>> On Thu, Jun 20, 2019 at 05:37:24PM +0200, Laszlo Ersek wrote:
>>  On 06/17/19 10:12, Klaus Birkelund wrote:
>> > Hi all,
>> >
>> > I'm thinking about how to support multiple namespaces in the NVMe
>> > device. My first idea was to add a "namespaces" property array to the
>> > device that references blockdevs, but as Laszlo writes below, this 
>> > might
>> > not be the best idea. It also makes it troublesome to add per-namespace
>> > parameters (which is something I will be required to do for other
>> > reasons). Some of you might remember my first attempt at this that
>> > included adding a new block driver (derived from raw) that could be
>> > given certain parameters that would then be stored in the image. But I
>> > understand that this is a no-go, and I can see why.
>> >
>> > I guess the optimal way would be such that the parameters was something
>> > like:
>> >
>> >-blockdev 
>> > raw,node-name=blk_ns1,file.driver=file,file.filename=blk_ns1.img
>> >-blockdev 
>> > raw,node-name=blk_ns2,file.driver=file,file.filename=blk_ns2.img
>> >-device nvme-ns,drive=blk_ns1,ns-specific-options 
>> > (nsfeat,mc,dlfeat)...
>> >-device nvme-ns,drive=blk_ns2,...
>> >-device nvme,...
>> >
>> > My question is how to state the parent/child relationship between the
>> > nvme and nvme-ns devices. I've been looking at how ide and virtio does
>> > this, and maybe a "bus" is the right way to go?
>> 
>>  I've added Markus to the address list, because of this question. No
>>  other (new) comments from me on the thread starter at this time, just
>>  keeping the full context.
>> 
>> >>>
>> >>> Hi all,
>> >>>
>> >>> I've succesfully implemented this by introducing a new 'nvme-ns' device
>> >>> model. The nvme device creates a bus named from the device id ('id'
>> >>> parameter) and the nvme-ns devices are then registered on this.
>> >>>
>> >>> This results in an nvme device being creates like this (two namespaces
>> >>> example):
>> >>>
>> >>>   -drive file=nvme0n1.img,if=none,id=disk1
>> >>>   -drive file=nvme0n2.img,if=none,id=disk2
>> >>>   -device nvme,serial=deadbeef,id=nvme0
>> >>>   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
>> >>>   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
>> >>>
>> >>> How does that look as a way forward?
>> >> 
>> >> This looks very similar to what other devices do (one bus controller
>> >> that has multiple devices on its but), so I like it.
>> 
>> Devices can be wired together without a bus intermediary.  You
>> definitely want a bus when the physical connection you model has one.
>> If not, a bus may be useful anyway, say because it provides a convenient
>> way to encapsulate the connection model, or to support -device bus=...
>> 
>  
> I'm not sure how to wire it together without the bus abstraction? So
> I'll stick with the bus for now. It *is* extremely convenient!

As far as I can tell offhand, a common use of bus-less connections
between devices is wiring together composite devices.  Example:

static void designware_pcie_host_init(Object *obj)
{
DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(obj);
DesignwarePCIERoot *root = >root;

object_initialize_child(obj, "root",  root, sizeof(*root),
TYPE_DESIGNWARE_PCIE_ROOT, _abort, NULL);
qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
qdev_prop_set_bit(DEVICE(root), "multifunction", false);
}

This creates a TYPE_DESIGNWARE_PCIE_ROOT device "within" the
TYPE_DESIGNWARE_PCIE_HOST device.

Bus-less connections between separate devices (i.e. neither device is a
part of the other) are also possible.  But I'm failing at grep right
now.  Here's an example for connecting a device to a machine:

static void mch_realize(PCIDevice *d, Error **errp)
{
int i;
MCHPCIState *mch = MCH_PCI_DEVICE(d);

[...]
object_property_add_const_link(qdev_get_machine(), "smram",
   OBJECT(>smram), _abort);
[...]
}

Paolo, can you provide guidance on when to use a bus, and when not to?

[Qemu-devel] [PATCH] qemu-nbd: Permit TLS with Unix sockets

[Eric Blake]

Although you generally won't use encryption with a Unix socket (after
all, everything is local, so why waste the CPU power), there are
situations in testsuites where Unix sockets are much nicer than TCP
sockets.  Since nbdkit allows encryption over both types of sockets,
it makes sense for qemu-nbd to do likewise.

Signed-off-by: Eric Blake 
---
 qemu-nbd.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index a8cb39e51043..ddfb6815fb69 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -931,10 +931,6 @@ int main(int argc, char **argv)
 }

 if (tlscredsid) {
-if (sockpath) {
-error_report("TLS is only supported with IPv4/IPv6");
-exit(EXIT_FAILURE);
-}
 if (device) {
 error_report("TLS is not supported with a host device");
 exit(EXIT_FAILURE);
-- 
2.20.1

[Qemu-devel] [PATCH v4 4/5] virtio: Make sure we get correct state of device on handle_aio_output()

[elohimes]

From: Xie Yongji 

We should set the flags: "start_on_kick" and "started" after we call
the kick functions (handle_aio_output() and handle_output()).

Signed-off-by: Xie Yongji 
---
 hw/virtio/virtio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5fd25d98a9..e098fc8ef0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1571,10 +1571,10 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
 event_notifier_set(>host_notifier);
 } else if (vq->handle_output) {
 vq->handle_output(vdev, vq);
-}
 
-if (unlikely(vdev->start_on_kick)) {
-virtio_set_started(vdev, true);
+if (unlikely(vdev->start_on_kick)) {
+virtio_set_started(vdev, true);
+}
 }
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH v4 3/5] virtio: Set "start_on_kick" on virtio_set_features()

[elohimes]

From: Xie Yongji 

The guest feature is not set correctly on virtio_reset() and
virtio_init(). So we should not use it to set "start_on_kick" at that
point. This patch set "start_on_kick" on virtio_set_features() instead.

Fixes: badaf79cfdbd3 ("virtio: Introduce started flag to VirtioDevice")
Signed-off-by: Xie Yongji 
Reviewed-by: Greg Kurz 
---
 hw/virtio/virtio.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f7504d1395..5fd25d98a9 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1212,7 +1212,7 @@ void virtio_reset(void *opaque)
 k->reset(vdev);
 }
 
-vdev->start_on_kick = !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1);
+vdev->start_on_kick = false;
 vdev->started = false;
 vdev->broken = false;
 vdev->guest_features = 0;
@@ -2063,14 +2063,21 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t 
val)
 return -EINVAL;
 }
 ret = virtio_set_features_nocheck(vdev, val);
-if (!ret && virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
-/* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
-int i;
-for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-if (vdev->vq[i].vring.num != 0) {
-virtio_init_region_cache(vdev, i);
+if (!ret) {
+if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
+/* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
+int i;
+for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+if (vdev->vq[i].vring.num != 0) {
+virtio_init_region_cache(vdev, i);
+}
 }
 }
+
+if (!virtio_device_started(vdev, vdev->status) &&
+!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+vdev->start_on_kick = true;
+}
 }
 return ret;
 }
@@ -,6 +2229,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
 }
 }
 
+if (!virtio_device_started(vdev, vdev->status) &&
+!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+vdev->start_on_kick = true;
+}
+
 rcu_read_lock();
 for (i = 0; i < num; i++) {
 if (vdev->vq[i].vring.desc) {
@@ -2324,7 +2336,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
 g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
 }
 
-vdev->start_on_kick = !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1);
+vdev->start_on_kick = false;
 vdev->started = false;
 vdev->device_id = device_id;
 vdev->status = 0;
-- 
2.17.1

[Qemu-devel] [PATCH v4 5/5] virtio: Don't change "started" flag on virtio_vmstate_change()

[elohimes]

From: Xie Yongji 

We will call virtio_set_status() on virtio_vmstate_change().
The "started" flag should not be changed in this case. Otherwise,
we may get an incorrect value when we set "started" flag but
not set DRIVER_OK in source VM.

Signed-off-by: Xie Yongji 
---
 hw/virtio/virtio.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e098fc8ef0..18f9f4c372 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1163,7 +1163,10 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
 }
 }
 
-virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
+if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) !=
+(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
+virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
+}
 
 if (k->set_status) {
 k->set_status(vdev, val);
-- 
2.17.1

[Qemu-devel] [PATCH v4 1/5] virtio: add "use-started" property

[elohimes]

From: Xie Yongji 

In order to avoid migration issues, we introduce a "use-started"
property to the base virtio device to indicate whether use
"started" flag or not. This property will be true by default and
set to false when machine type <= 4.0.

Suggested-by: Greg Kurz 
Signed-off-by: Xie Yongji 
---
 hw/block/vhost-user-blk.c  |  4 ++--
 hw/core/machine.c  |  1 +
 hw/virtio/virtio.c | 18 +++---
 include/hw/virtio/virtio.h | 21 +
 4 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9cb61336a6..85bc4017e7 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -191,7 +191,7 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
 static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
 {
 VHostUserBlk *s = VHOST_USER_BLK(vdev);
-bool should_start = vdev->started;
+bool should_start = virtio_device_started(vdev, status);
 int ret;
 
 if (!vdev->vm_running) {
@@ -317,7 +317,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
 }
 
 /* restore vhost state */
-if (vdev->started) {
+if (virtio_device_started(vdev, vdev->status)) {
 ret = vhost_user_blk_start(vdev);
 if (ret < 0) {
 error_report("vhost-user-blk: vhost start failed: %s",
diff --git a/hw/core/machine.c b/hw/core/machine.c
index ea5a01aa49..ea84bd6788 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,6 +30,7 @@ GlobalProperty hw_compat_4_0[] = {
 { "bochs-display",  "edid", "false" },
 { "virtio-vga", "edid", "false" },
 { "virtio-gpu-pci", "edid", "false" },
+{ "virtio-device", "use-started", "false" },
 };
 const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e1e90fcfd6..c9a6ca04b8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1162,10 +1162,8 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
 }
 }
 }
-vdev->started = val & VIRTIO_CONFIG_S_DRIVER_OK;
-if (unlikely(vdev->start_on_kick && vdev->started)) {
-vdev->start_on_kick = false;
-}
+
+virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
 
 if (k->set_status) {
 k->set_status(vdev, val);
@@ -1536,8 +1534,7 @@ static bool virtio_queue_notify_aio_vq(VirtQueue *vq)
 ret = vq->handle_aio_output(vdev, vq);
 
 if (unlikely(vdev->start_on_kick)) {
-vdev->started = true;
-vdev->start_on_kick = false;
+virtio_set_started(vdev, true);
 }
 }
 
@@ -1557,8 +1554,7 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
 vq->handle_output(vdev, vq);
 
 if (unlikely(vdev->start_on_kick)) {
-vdev->started = true;
-vdev->start_on_kick = false;
+virtio_set_started(vdev, true);
 }
 }
 }
@@ -1579,8 +1575,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
 }
 
 if (unlikely(vdev->start_on_kick)) {
-vdev->started = true;
-vdev->start_on_kick = false;
+virtio_set_started(vdev, true);
 }
 }
 
@@ -2291,7 +2286,7 @@ static void virtio_vmstate_change(void *opaque, int 
running, RunState state)
 VirtIODevice *vdev = opaque;
 BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
 VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-bool backend_run = running && vdev->started;
+bool backend_run = running && virtio_device_started(vdev, vdev->status);
 vdev->vm_running = running;
 
 if (backend_run) {
@@ -2669,6 +2664,7 @@ static void virtio_device_instance_finalize(Object *obj)
 
 static Property virtio_properties[] = {
 DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
+DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 27c0efc3d0..15d5366939 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -105,6 +105,7 @@ struct VirtIODevice
 uint16_t device_id;
 bool vm_running;
 bool broken; /* device in invalid state, needs reset */
+bool use_started;
 bool started;
 bool start_on_kick; /* virtio 1.0 transitional devices support that */
 VMChangeStateEntry *vmstate;
@@ -351,4 +352,24 @@ static inline bool virtio_is_big_endian(VirtIODevice *vdev)
 /* Devices conforming to VIRTIO 1.0 or later are always LE. */
 return false;
 }
+
+static inline bool virtio_device_started(VirtIODevice *vdev, uint8_t status)
+{
+if (vdev->use_started) {
+return vdev->started;
+}
+
+return status & VIRTIO_CONFIG_S_DRIVER_OK;
+}
+
+static inline void virtio_set_started(VirtIODevice *vdev, bool started)
+{
+if (started) {
+vdev->start_on_kick = false;
+}
+
+if (vdev->use_started) {
+vdev->started = started;
+

[Qemu-devel] [PATCH v4 0/5] virtio: fix some issues of "started" and "start_on_kick" flag

[elohimes]

From: Xie Yongji 

We introduced two flags "started" and "start_on_kick" to indicate virtio
device's state before. But there still are some problems with them. So
we try to fixup them in this patchset.

The patch 1 introduces a "use-started" property to avoid a migration
issue under Greg Kurz's suggestion [1].

The patch 2 set "start_on_kick" flag for legacy devices.

The patch 3 fixes a regression bug that old guest is not able to boot with
vhost-user-blk device.

The patch 4,5 fix some problems with "started" and "start_on_kick" flag.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg06247.html

v4:
- Patch 1 is rebased on commit 8e8cbed09ad9d5 ("hw: Nuke hw_compat_4_0_1 and
  pc_compat_4_0_1")

v3:
- change the order of patches
- Also disable "use-started" property by hw_compat_4_0

v2:
- Recalculate "start_on_kick" flag after migration instead of migrating
  it
- Set "start_on_kick" flag for legacy devices
- Avoid setting "started" flag when "use_started" property is true
- Set "use_started" to false by hw_compat_4_0_1 instead of hw_compat_4_0

Xie Yongji (5):
  virtio: add "use-started" property
  virtio: Set "start_on_kick" for legacy devices
  virtio: Set "start_on_kick" on virtio_set_features()
  virtio: Make sure we get correct state of device on
handle_aio_output()
  virtio: Don't change "started" flag on virtio_vmstate_change()

 hw/block/vhost-user-blk.c  |  4 +--
 hw/core/machine.c  |  1 +
 hw/virtio/virtio.c | 53 ++
 include/hw/virtio/virtio.h | 23 -
 4 files changed, 56 insertions(+), 25 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v4 2/5] virtio: Set "start_on_kick" for legacy devices

[elohimes]

From: Xie Yongji 

Besides virtio 1.0 transitional devices, we should also
set "start_on_kick" flag for legacy devices (virtio 0.9).

Signed-off-by: Xie Yongji 
Reviewed-by: Greg Kurz 
---
 hw/virtio/virtio.c | 6 ++
 include/hw/virtio/virtio.h | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index c9a6ca04b8..f7504d1395 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1212,8 +1212,7 @@ void virtio_reset(void *opaque)
 k->reset(vdev);
 }
 
-vdev->start_on_kick = (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-  !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1));
+vdev->start_on_kick = !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1);
 vdev->started = false;
 vdev->broken = false;
 vdev->guest_features = 0;
@@ -2325,8 +2324,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
 g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
 }
 
-vdev->start_on_kick = (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-  !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1));
+vdev->start_on_kick = !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1);
 vdev->started = false;
 vdev->device_id = device_id;
 vdev->status = 0;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 15d5366939..b189788cb2 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -107,7 +107,7 @@ struct VirtIODevice
 bool broken; /* device in invalid state, needs reset */
 bool use_started;
 bool started;
-bool start_on_kick; /* virtio 1.0 transitional devices support that */
+bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
 VMChangeStateEntry *vmstate;
 char *bus_name;
 uint8_t device_endian;
-- 
2.17.1

Re: [Qemu-devel] [PATCH] virtio-pci: fix missing device properties

[Eduardo Habkost]

On Wed, Jun 26, 2019 at 01:23:33AM +0200, Marc-André Lureau wrote:
> Since commit a4ee4c8baa37154 ("virtio: Helper for registering virtio
> device types"), virtio-gpu-pci, virtio-vga, and virtio-crypto-pci lost
> some properties: "ioeventfd" and "vectors". This may cause various
> issues, such as failing migration or invalid properties.
> 
> Since those VirtioPCI devices do not have a base name, their class are
> initialized with virtio_pci_generic_base_class_init(). However, if the
> VirtioPCIDeviceTypeInfo provided a class_init which sets dc->props,
> the properties were overwritten by virtio_pci_generic_class_init().
> 
> Instead, introduce an intermediary base-type to register the generic
> properties.
> 
> Fixes: a4ee4c8baa37154f42b4dc6a13fee79268d15238
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Marc-André Lureau 
> ---
>  hw/virtio/virtio-pci.c | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index e6d5467e54..62c4977332 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1913,13 +1913,6 @@ static void virtio_pci_generic_class_init(ObjectClass 
> *klass, void *data)
>  dc->props = virtio_pci_generic_properties;
>  }
>  
> -/* Used when the generic type and the base type is the same */
> -static void virtio_pci_generic_base_class_init(ObjectClass *klass, void 
> *data)
> -{
> -virtio_pci_base_class_init(klass, data);
> -virtio_pci_generic_class_init(klass, NULL);
> -}
> -
>  static void virtio_pci_transitional_instance_init(Object *obj)
>  {
>  VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> @@ -1938,14 +1931,13 @@ static void 
> virtio_pci_non_transitional_instance_init(Object *obj)
>  
>  void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
>  {
> +char *base_name = NULL;
>  TypeInfo base_type_info = {
>  .name  = t->base_name,
>  .parent= t->parent ? t->parent : TYPE_VIRTIO_PCI,
>  .instance_size = t->instance_size,
>  .instance_init = t->instance_init,
>  .class_size= t->class_size,
> -.class_init= virtio_pci_base_class_init,
> -.class_data= (void *)t,
>  .abstract  = true,
>  };
>  TypeInfo generic_type_info = {
> @@ -1961,13 +1953,20 @@ void virtio_pci_types_register(const 
> VirtioPCIDeviceTypeInfo *t)
>  
>  if (!base_type_info.name) {
>  /* No base type -> register a single generic device type */
> -base_type_info.name = t->generic_name;
> -base_type_info.class_init = virtio_pci_generic_base_class_init;
> -base_type_info.interfaces = generic_type_info.interfaces;
> -base_type_info.abstract = false;
> -generic_type_info.name = NULL;
> +/* use intermediate %s-base-type to add generic device props */
> +base_name = g_strdup_printf("%s-base-type", t->generic_name);
> +base_type_info.name = base_name;
> +base_type_info.class_init = virtio_pci_generic_class_init;
> +
> +generic_type_info.parent = base_name;
> +generic_type_info.class_init = virtio_pci_base_class_init;
> +generic_type_info.class_data = (void *)t;

Why are you using virtio_pci_generic_class_init for the base
class, and virtio_pci_base_class_init for the subclass, but doing
exactly the opposite when t->base_name is set?

Isn't it simpler to just initialize base_type_info.name and leave
all the rest alone?  Patch below.

Signed-off-by: Eduardo Habkost 
---
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e6d5467e54..3ee50a0783 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1913,13 +1913,6 @@ static void virtio_pci_generic_class_init(ObjectClass 
*klass, void *data)
 dc->props = virtio_pci_generic_properties;
 }
 
-/* Used when the generic type and the base type is the same */
-static void virtio_pci_generic_base_class_init(ObjectClass *klass, void *data)
-{
-virtio_pci_base_class_init(klass, data);
-virtio_pci_generic_class_init(klass, NULL);
-}
-
 static void virtio_pci_transitional_instance_init(Object *obj)
 {
 VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
@@ -1938,8 +1931,11 @@ static void 
virtio_pci_non_transitional_instance_init(Object *obj)
 
 void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
 {
+char *base_name = t->base_name ?
+  NULL :
+  g_strdup_printf("%s-base-type", t->generic_name);
 TypeInfo base_type_info = {
-.name  = t->base_name,
+.name  = t->base_name ?: base_name,
 .parent= t->parent ? t->parent : TYPE_VIRTIO_PCI,
 .instance_size = t->instance_size,
 .instance_init = t->instance_init,
@@ -1959,21 +1955,8 @@ void virtio_pci_types_register(const 
VirtioPCIDeviceTypeInfo *t)
 },
 };
 
-if (!base_type_info.name) {
-/* No base type -> register a 

Re: [Qemu-devel] [PATCH] riscv: virt: Correct pci "bus-range" encoding

[Bin Meng]

Hi,

On Fri, Jun 7, 2019 at 2:46 AM Alistair Francis  wrote:
>
> On Thu, Jun 6, 2019 at 5:55 AM Bin Meng  wrote:
> >
> > On Thu, May 30, 2019 at 11:36 AM Bin Meng  wrote:
> > >
> > > Hi Alistair,
> > >
> > > On Thu, May 30, 2019 at 11:14 AM Alistair Francis  
> > > wrote:
> > > >
> > > > On Wed, May 29, 2019 at 1:52 AM Bin Meng  wrote:
> > > > >
> > > > > The largest pci bus number should be calculated from ECAM size,
> > > > > instead of its base address.
> > > > >
> > > > > Signed-off-by: Bin Meng 
> > > >
> > > > This seems ok, can you maybe explain what this fixes?
> > > >
> > >
> > > The logic is wrong, as the commit message said. With current wrong
> > > logic, the largest pci bus number encoded in "bus-ranges" property was
> > > wrongly set to 0x2ff in this case. Per pci spec, the bus number should
> > > not exceed 0xff.
> > >
> >
> > Ping?
>
> Reviewed-by: Alistair Francis 

Can this go in the 4.1 PR?

Regards,
Bin

Re: [Qemu-devel] [PATCH 1/2] riscv: sifive_u: Do not create hard-coded phandles in DT

[Bin Meng]

Hi,

On Sat, May 18, 2019 at 5:34 AM Alistair Francis
 wrote:
>
> On Fri, 2019-05-17 at 08:51 -0700, Bin Meng wrote:
> > At present the cpu, plic and ethclk nodes' phandles are hard-coded
> > to 1/2/3 in DT. If we configure more than 1 cpu for the machine,
> > all cpu nodes' phandles conflict with each other as they are all 1.
> > Fix it by removing the hardcode.
> >
> > Signed-off-by: Bin Meng 
>
> Reviewed-by: Alistair Francis 
>

Can this go in the 4.1 PR?

Regards,
Bin

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

Hi Palmer,

On Sun, Jun 23, 2019 at 10:40 PM Palmer Dabbelt  wrote:
>
> On Thu, 20 Jun 2019 22:40:24 PDT (-0700), bmeng...@gmail.com wrote:
> > Hi Palmer,
> >
> > On Fri, Jun 21, 2019 at 10:53 AM Palmer Dabbelt  wrote:
> >>
> >> On Wed, 19 Jun 2019 06:42:21 PDT (-0700), bmeng...@gmail.com wrote:
> >> > Hi Alistair,
> >> >
> >> > On Tue, Jun 18, 2019 at 1:15 AM Alistair Francis  
> >> > wrote:
> >> >>
> >> >> On Fri, Jun 14, 2019 at 8:30 AM Bin Meng  wrote:
> >> >> >
> >> >> > This adds a reset opcode for sifive_test device to trigger a system
> >> >> > reset for testing purpose.
> >> >> >
> >> >> > Signed-off-by: Bin Meng 
> >> >> > ---
> >> >> >
> >> >> >  hw/riscv/sifive_test.c | 4 
> >> >> >  include/hw/riscv/sifive_test.h | 3 ++-
> >> >> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> >> >> > index 24a04d7..cd86831 100644
> >> >> > --- a/hw/riscv/sifive_test.c
> >> >> > +++ b/hw/riscv/sifive_test.c
> >> >> > @@ -21,6 +21,7 @@
> >> >> >  #include "qemu/osdep.h"
> >> >> >  #include "hw/sysbus.h"
> >> >> >  #include "qemu/module.h"
> >> >> > +#include "sysemu/sysemu.h"
> >> >> >  #include "target/riscv/cpu.h"
> >> >> >  #include "hw/riscv/sifive_test.h"
> >> >> >
> >> >> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr 
> >> >> > addr,
> >> >> >  exit(code);
> >> >> >  case FINISHER_PASS:
> >> >> >  exit(0);
> >> >> > +case FINISHER_RESET:
> >> >> > +qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >> >> > +return;
> >> >> >  default:
> >> >> >  break;
> >> >> >  }
> >> >> > diff --git a/include/hw/riscv/sifive_test.h 
> >> >> > b/include/hw/riscv/sifive_test.h
> >> >> > index 71d4c9f..c186a31 100644
> >> >> > --- a/include/hw/riscv/sifive_test.h
> >> >> > +++ b/include/hw/riscv/sifive_test.h
> >> >> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
> >> >> >
> >> >> >  enum {
> >> >> >  FINISHER_FAIL = 0x,
> >> >> > -FINISHER_PASS = 0x
> >> >> > +FINISHER_PASS = 0x,
> >> >> > +FINISHER_RESET = 0x
> >> >>
> >> >> Do you mind sharing where you got this value from? I can't find
> >> >> details on this device in the SiFive manuals.
> >> >>
> >> >
> >> > I don't think this is a device that actually exists on SiFive's
> >> > chipset. It's hypothetical.
> >>
> >> The device actually does exist in the hardware, but that's just an
> >> implementation quirk.  Essentially what's going on here is that the RTL
> >> contains this device, which has a register and then a behavioral verilog 
> >> block
> >> that causes simulations to terminate.  This is how we exit from tests in 
> >> RTL
> >> simulation, and we've just gone ahead and implemented the same device in 
> >> QEMU
> >> in order to make it easy to have compatibility with those bare-metal tests.
> >> Due to how our design flow is set up we end up with exactly the same block 
> >> in
> >> the ASIC.  The register is still there, but the behavioral code to exit
> >> simulations doesn't do anything so it's essentially just a useless device.
> >> Since it's useless we don't bother writing it up in the ASIC 
> >> documentation, but
> >> it should be in the RTL documentation.
> >>
> >> I'm not opposed to extending the interface in the suggested fashion, but I
> >> wanted to check with the hardware team first to see if they're doing 
> >> anything
> >> with the other numbers.  I'm out of the office (and somewhat backed up on 
> >> code
> >> review) until July, so it might take a bit to dig through this.
> >
> > Thanks for the clarification. The main reason of adding this
> > functionality is to provide software a way of rebooting the whole
> > system. Please provide update after you consult SiFive hardware guys
> > :)
>
> Ya, it makes sense.  My only worry here is that we have some way of doing this
> already, in which case I'd just want to make sure it matches.

If the SiFive chipset already has the functionality to do the reset
via this test module, I agree let's align the number to what hardware
actually accepts.

Regards,
Bin

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [PATCH] powerpc/spapr: Add host threads parameter to ibm, get_system_parameter

[Suraj Jitindar Singh]

On Mon, 2019-06-24 at 10:37 +0200, Greg Kurz wrote:
> On Mon, 24 Jun 2019 11:39:21 +1000
> Suraj Jitindar Singh  wrote:
> 
> > The ibm,get_system_parameter rtas call is used by the guest to
> > retrieve
> > data relating to certain parameters of the system. The SPLPAR
> > characteristics option (token 20) is used to determin
> > characteristics of
> > the environment in which the lpar will run.
> > 
> > It may be useful for a guest to know the number of physical host
> > threads
> > present on the underlying system where it is being run. Add the
> > characteristic "HostThrs" to the SPLPAR Characteristics
> > ibm,get_system_parameter rtas call to expose this information to a
> > guest and provide an implementation which determines this
> > information
> > based on the number of interrupt servers present in the device
> > tree.
> > 
> 
> Shouldn't this also take split core into account, ie. divide the
> result by "/sys/devices/system/cpu/subcores_per_core" like the 
> ppc64_cpu command from powerpc-utils does ?

That makes sense, I'll modify the code to account for that.

Thanks,
Suraj

> 
> > Signed-off-by: Suraj Jitindar Singh 
> > ---
> >  hw/ppc/spapr_rtas.c | 44
> > 
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 5bc1a93271..a33d87794c 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -229,6 +229,40 @@ static inline int sysparm_st(target_ulong
> > addr, target_ulong len,
> >  return RTAS_OUT_SUCCESS;
> >  }
> >  
> > +static int rtas_get_num_host_threads(void)
> > +{
> > +const char *entry, *name = "/proc/device-tree/cpus/";
> > +int num_threads = -1;
> > +GDir *dir;
> > +
> > +if (!kvm_enabled())
> > +return 1;
> > +
> > +dir = g_dir_open(name, 0, NULL);
> > +if (!dir)
> > +return -1;
> > +
> > +while ((entry = g_dir_read_name(dir))) {
> > +if (!strncmp(entry, "PowerPC,POWER",
> > strlen("PowerPC,POWER"))) {
> > +unsigned long len;
> > +char *path, *buf;
> > +
> > +path = g_strconcat(name, entry, "/ibm,ppc-interrupt-
> > server#s",
> > +   NULL);
> > +if (g_file_get_contents(path, , , NULL)) {
> > +num_threads = len / sizeof(int);
> > +g_free(buf);
> > +}
> > +
> > +g_free(path);
> > +break;
> > +}
> > +}
> > +
> > +g_dir_close(dir);
> > +return num_threads;
> > +}
> > +
> >  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> >SpaprMachineState
> > *spapr,
> >uint32_t token, uint32_t
> > nargs,
> > @@ -250,6 +284,16 @@ static void
> > rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> >current_machine-
> > >ram_size / MiB,
> >smp_cpus,
> >max_cpus);
> > +int num_host_threads = rtas_get_num_host_threads();
> > +
> > +if (num_host_threads > 0) {
> > +char *hostthr_val, *old = param_val;
> > +
> > +hostthr_val = g_strdup_printf(",HostThrs=%d",
> > num_host_threads);
> > +param_val = g_strconcat(param_val, hostthr_val, NULL);
> > +g_free(hostthr_val);
> > +g_free(old);
> > +}
> >  ret = sysparm_st(buffer, length, param_val,
> > strlen(param_val) + 1);
> >  g_free(param_val);
> >  break;
> 
> 

Re: [Qemu-devel] [PATCH] ui: Correct icon install path

[Colin Xu]

On 2019-06-25 21:40, Eric Blake wrote:

On 6/25/19 5:42 AM, Daniel P. Berrangé wrote:

On Tue, Jun 25, 2019 at 11:21:42AM +0800, Colin Xu wrote:

The double slash in path will fail the installation on MINGW/MSYS.

Fixes: a8260d387638 (ui: install logo icons to $prefix/share/icons)

Signed-off-by: Colin Xu 
---
  Makefile | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

Hmmm I swear this exact fix has been posted before but I can't find
/ remember where and obviously it didnt get merged.

https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg04885.html


Aha, you are right Daniel. This has been posted before.
--
Best Regards,
Colin Xu

Re: [Qemu-devel] [PATCH v4 00/13] Add migration support for VFIO device

[Yan Zhao]

On Tue, Jun 25, 2019 at 03:00:24AM +0800, Dr. David Alan Gilbert wrote:
> * Kirti Wankhede (kwankh...@nvidia.com) wrote:
> > 
> > 
> > On 6/21/2019 2:16 PM, Yan Zhao wrote:
> > > On Fri, Jun 21, 2019 at 04:02:50PM +0800, Kirti Wankhede wrote:
> > >>
> > >>
> > >> On 6/21/2019 6:54 AM, Yan Zhao wrote:
> > >>> On Fri, Jun 21, 2019 at 08:25:18AM +0800, Yan Zhao wrote:
> >  On Thu, Jun 20, 2019 at 10:37:28PM +0800, Kirti Wankhede wrote:
> > > Add migration support for VFIO device
> > >
> > > This Patch set include patches as below:
> > > - Define KABI for VFIO device for migration support.
> > > - Added save and restore functions for PCI configuration space
> > > - Generic migration functionality for VFIO device.
> > >   * This patch set adds functionality only for PCI devices, but can be
> > > extended to other VFIO devices.
> > >   * Added all the basic functions required for pre-copy, 
> > > stop-and-copy and
> > > resume phases of migration.
> > >   * Added state change notifier and from that notifier function, VFIO
> > > device's state changed is conveyed to VFIO device driver.
> > >   * During save setup phase and resume/load setup phase, migration 
> > > region
> > > is queried and is used to read/write VFIO device data.
> > >   * .save_live_pending and .save_live_iterate are implemented to use 
> > > QEMU's
> > > functionality of iteration during pre-copy phase.
> > >   * In .save_live_complete_precopy, that is in stop-and-copy phase,
> > > iteration to read data from VFIO device driver is implemented 
> > > till pending
> > > bytes returned by driver are not zero.
> > >   * Added function to get dirty pages bitmap for the pages which are 
> > > used by
> > > driver.
> > > - Add vfio_listerner_log_sync to mark dirty pages.
> > > - Make VFIO PCI device migration capable. If migration region is not 
> > > provided by
> > >   driver, migration is blocked.
> > >
> > > Below is the flow of state change for live migration where states in 
> > > brackets
> > > represent VM state, migration state and VFIO device state as:
> > > (VM state, MIGRATION_STATUS, VFIO_DEVICE_STATE)
> > >
> > > Live migration save path:
> > > QEMU normal running state
> > > (RUNNING, _NONE, _RUNNING)
> > > |
> > > migrate_init spawns migration_thread.
> > > (RUNNING, _SETUP, _RUNNING|_SAVING)
> > > Migration thread then calls each device's .save_setup()
> > > |
> > > (RUNNING, _ACTIVE, _RUNNING|_SAVING)
> > > If device is active, get pending bytes by .save_live_pending()
> > > if pending bytes >= threshold_size,  call save_live_iterate()
> > > Data of VFIO device for pre-copy phase is copied.
> > > Iterate till pending bytes converge and are less than threshold
> > > |
> > > On migration completion, vCPUs stops and calls 
> > > .save_live_complete_precopy
> > > for each active device. VFIO device is then transitioned in
> > >  _SAVING state.
> > > (FINISH_MIGRATE, _DEVICE, _SAVING)
> > > For VFIO device, iterate in  .save_live_complete_precopy  until
> > > pending data is 0.
> > > (FINISH_MIGRATE, _DEVICE, _STOPPED)
> > 
> >  I suggest we also register to VMStateDescription, whose .pre_save
> >  handler would get called after .save_live_complete_precopy in pre-copy
> >  only case, and will called before .save_live_iterate in post-copy
> >  enabled case.
> >  In the .pre_save handler, we can save all device state which must be
> >  copied after device stop in source vm and before device start in 
> >  target vm.
> > 
> > >>> hi
> > >>> to better describe this idea:
> > >>>
> > >>> in pre-copy only case, the flow is
> > >>>
> > >>> start migration --> .save_live_iterate (several round) -> stop source vm
> > >>> --> .save_live_complete_precopy --> .pre_save  -->start target vm
> > >>> -->migration complete
> > >>>
> > >>>
> > >>> in post-copy enabled case, the flow is
> > >>>
> > >>> start migration --> .save_live_iterate (several round) --> start post 
> > >>> copy --> 
> > >>> stop source vm --> .pre_save --> start target vm --> .save_live_iterate 
> > >>> (several round) 
> > >>> -->migration complete
> > >>>
> > >>> Therefore, we should put saving of device state in .pre_save interface
> > >>> rather than in .save_live_complete_precopy. 
> > >>> The device state includes pci config data, page tables, register state, 
> > >>> etc.
> > >>>
> > >>> The .save_live_iterate and .save_live_complete_precopy should only deal
> > >>> with saving dirty memory.
> > >>>
> > >>
> > >> Vendor driver can decide when to save device state depending on the VFIO
> > >> device state set by user. Vendor 

Re: [Qemu-devel] [PATCH] ui: Correct icon install path

[Colin Xu]

On 2019-06-25 18:43, Philippe Mathieu-Daudé wrote:

Hi Colin,

On 6/25/19 5:21 AM, Colin Xu wrote:

The double slash in path will fail the installation on MINGW/MSYS.

Fixes: a8260d387638 (ui: install logo icons to $prefix/share/icons)

Signed-off-by: Colin Xu 
---
  Makefile | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index cfb18f152544..562205be290c 100644
--- a/Makefile
+++ b/Makefile
@@ -875,19 +875,19 @@ ifneq ($(DESCS),)
done
  endif
for s in $(ICON_SIZES); do \
-   mkdir -p "$(DESTDIR)/$(qemu_icondir)/hicolor/$${s}/apps"; \
+   mkdir -p "$(DESTDIR)$(qemu_icondir)/hicolor/$${s}/apps"; \
$(INSTALL_DATA) $(SRC_PATH)/ui/icons/qemu_$${s}.png \
-   
"$(DESTDIR)/$(qemu_icondir)/hicolor/$${s}/apps/qemu.png"; \
+   
"$(DESTDIR)$(qemu_icondir)/hicolor/$${s}/apps/qemu.png"; \
done; \
-   mkdir -p "$(DESTDIR)/$(qemu_icondir)/hicolor/32x32/apps"; \
+   mkdir -p "$(DESTDIR)$(qemu_icondir)/hicolor/32x32/apps"; \
$(INSTALL_DATA) $(SRC_PATH)/ui/icons/qemu_32x32.bmp \
-   "$(DESTDIR)/$(qemu_icondir)/hicolor/32x32/apps/qemu.bmp"; \
-   mkdir -p "$(DESTDIR)/$(qemu_icondir)/hicolor/scalable/apps"; \
+   "$(DESTDIR)$(qemu_icondir)/hicolor/32x32/apps/qemu.bmp"; \
+   mkdir -p "$(DESTDIR)$(qemu_icondir)/hicolor/scalable/apps"; \
$(INSTALL_DATA) $(SRC_PATH)/ui/icons/qemu.svg \
-   "$(DESTDIR)/$(qemu_icondir)/hicolor/scalable/apps/qemu.svg"
-   mkdir -p "$(DESTDIR)/$(qemu_desktopdir)"
+   "$(DESTDIR)$(qemu_icondir)/hicolor/scalable/apps/qemu.svg"
+   mkdir -p "$(DESTDIR)$(qemu_desktopdir)"
$(INSTALL_DATA) $(SRC_PATH)/ui/qemu.desktop \
-   "$(DESTDIR)/$(qemu_desktopdir)/qemu.desktop"
+   "$(DESTDIR)$(qemu_desktopdir)/qemu.desktop"
  ifdef CONFIG_GTK
$(MAKE) -C po $@
  endif


I'm not sure about this. Did you test it on a POSIX system?

Maybe we should escape an eventual trailing '/' in datadir and DESTDIR?


Hi Philippe,
Yes, tested. Actually the other DIR referened in the Makefile only use 
$(DESTDIR)$(bindir)
, $(DESTDIR)$(qemu_docdir), etc. So I guess the inconsistencie should be fixed.

Best Regards,
Colin Xu

Re: [Qemu-devel] [PATCH v4 10/13] vfio: Add function to get dirty page list

[Yan Zhao]

On Thu, Jun 20, 2019 at 10:37:38PM +0800, Kirti Wankhede wrote:
> Dirty page tracking (.log_sync) is part of RAM copying state, where
> vendor driver provides the bitmap of pages which are dirtied by vendor
> driver through migration region and as part of RAM copy, those pages
> gets copied to file stream.
> 
> To get dirty page bitmap:
> - write start address, page_size and pfn count.
> - read count of pfns copied.
> - Vendor driver should return 0 if driver doesn't have any page to
>   report dirty in given range.
> - Vendor driver should return -1 to mark all pages dirty for given range.
> - read data_offset, where vendor driver has written bitmap.
> - read bitmap from the region or mmaped part of the region. This copy is
>   iterated till page bitmap for all requested pfns are copied.
> 
> Signed-off-by: Kirti Wankhede 
> Reviewed-by: Neo Jia 
> ---
>  hw/vfio/migration.c   | 119 
> ++
>  include/hw/vfio/vfio-common.h |   2 +
>  2 files changed, 121 insertions(+)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index e4895f91761d..68775b5dec11 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -228,6 +228,125 @@ static int vfio_load_device_config_state(QEMUFile *f, 
> void *opaque)
>  return qemu_file_get_error(f);
>  }
>  
> +void vfio_get_dirty_page_list(VFIODevice *vbasedev,
> +  uint64_t start_pfn,
> +  uint64_t pfn_count,
> +  uint64_t page_size)
> +{
> +VFIOMigration *migration = vbasedev->migration;
> +VFIORegion *region = >region.buffer;
> +uint64_t count = 0;
> +int64_t copied_pfns = 0;
> +int ret;
> +
> +qemu_mutex_lock(>lock);
> +ret = pwrite(vbasedev->fd, _pfn, sizeof(start_pfn),
> + region->fd_offset + offsetof(struct 
> vfio_device_migration_info,
> +  start_pfn));
> +if (ret < 0) {
> +error_report("Failed to set dirty pages start address %d %s",
> +ret, strerror(errno));
> +goto dpl_unlock;
> +}
> +
> +ret = pwrite(vbasedev->fd, _size, sizeof(page_size),
> + region->fd_offset + offsetof(struct 
> vfio_device_migration_info,
> +  page_size));
> +if (ret < 0) {
> +error_report("Failed to set dirty page size %d %s",
> +ret, strerror(errno));
> +goto dpl_unlock;
> +}
> +
> +ret = pwrite(vbasedev->fd, _count, sizeof(pfn_count),
> + region->fd_offset + offsetof(struct 
> vfio_device_migration_info,
> +  total_pfns));
> +if (ret < 0) {
> +error_report("Failed to set dirty page total pfns %d %s",
> +ret, strerror(errno));
> +goto dpl_unlock;
> +}
> +
> +do {
> +uint64_t bitmap_size, data_offset = 0;
> +void *buf = NULL;
> +bool buffer_mmaped = false;
> +
> +/* Read copied dirty pfns */
> +ret = pread(vbasedev->fd, _pfns, sizeof(copied_pfns),
> +region->fd_offset + offsetof(struct 
> vfio_device_migration_info,
> + copied_pfns));
> +if (ret < 0) {
> +error_report("Failed to get dirty pages bitmap count %d %s",
> +ret, strerror(errno));
> +goto dpl_unlock;
> +}
> +
> +if (copied_pfns == 0) {
> +/*
> + * copied_pfns could be 0 if driver doesn't have any page to
> + * report dirty in given range
> + */
> +break;
this copied_pfn is the dirty page count in which range?
if it is got each iteration, why break here rather than continue ?
consider there's a big region with pfn_count, and it is now breaked into
several smaller subregions, and copied_pfns is 0 in the first subregion,
it doesn't mean copied_pfns are all 0 in the remaining subregions.

> +} else if (copied_pfns == -1) {
> +/* Mark all pages dirty for this range */
> +cpu_physical_memory_set_dirty_range(start_pfn * page_size,
> +pfn_count * page_size,
> +DIRTY_MEMORY_MIGRATION);
> +break;
> +}
> +
> +bitmap_size = (BITS_TO_LONGS(copied_pfns) + 1) * sizeof(unsigned 
> long);
> +
> +ret = pread(vbasedev->fd, _offset, sizeof(data_offset),
> +region->fd_offset + offsetof(struct 
> vfio_device_migration_info,
> + data_offset));
> +if (ret != sizeof(data_offset)) {
> +error_report("Failed to get migration buffer data offset %d",
> + ret);
> +goto dpl_unlock;
> +}
> +
> +if (region->mmaps) {
> +int i;
> +

[Qemu-devel] [PATCH] migration: current_migration is never NULL

[Wei Yang]

migration_object_init() create and assign current_migration, which means
it will never be null until migration_shutdown().

Signed-off-by: Wei Yang 
---
 migration/migration.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0fd2364961..43fd8297ef 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1667,10 +1667,6 @@ bool migration_is_idle(void)
 {
 MigrationState *s = current_migration;
 
-if (!s) {
-return true;
-}
-
 switch (s->state) {
 case MIGRATION_STATUS_NONE:
 case MIGRATION_STATUS_CANCELLED:
-- 
2.19.1

[Qemu-devel] [PATCH] virtio-pci: fix missing device properties

[Marc-André Lureau]

Since commit a4ee4c8baa37154 ("virtio: Helper for registering virtio
device types"), virtio-gpu-pci, virtio-vga, and virtio-crypto-pci lost
some properties: "ioeventfd" and "vectors". This may cause various
issues, such as failing migration or invalid properties.

Since those VirtioPCI devices do not have a base name, their class are
initialized with virtio_pci_generic_base_class_init(). However, if the
VirtioPCIDeviceTypeInfo provided a class_init which sets dc->props,
the properties were overwritten by virtio_pci_generic_class_init().

Instead, introduce an intermediary base-type to register the generic
properties.

Fixes: a4ee4c8baa37154f42b4dc6a13fee79268d15238
Cc: qemu-sta...@nongnu.org
Signed-off-by: Marc-André Lureau 
---
 hw/virtio/virtio-pci.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e6d5467e54..62c4977332 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1913,13 +1913,6 @@ static void virtio_pci_generic_class_init(ObjectClass 
*klass, void *data)
 dc->props = virtio_pci_generic_properties;
 }
 
-/* Used when the generic type and the base type is the same */
-static void virtio_pci_generic_base_class_init(ObjectClass *klass, void *data)
-{
-virtio_pci_base_class_init(klass, data);
-virtio_pci_generic_class_init(klass, NULL);
-}
-
 static void virtio_pci_transitional_instance_init(Object *obj)
 {
 VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
@@ -1938,14 +1931,13 @@ static void 
virtio_pci_non_transitional_instance_init(Object *obj)
 
 void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
 {
+char *base_name = NULL;
 TypeInfo base_type_info = {
 .name  = t->base_name,
 .parent= t->parent ? t->parent : TYPE_VIRTIO_PCI,
 .instance_size = t->instance_size,
 .instance_init = t->instance_init,
 .class_size= t->class_size,
-.class_init= virtio_pci_base_class_init,
-.class_data= (void *)t,
 .abstract  = true,
 };
 TypeInfo generic_type_info = {
@@ -1961,13 +1953,20 @@ void virtio_pci_types_register(const 
VirtioPCIDeviceTypeInfo *t)
 
 if (!base_type_info.name) {
 /* No base type -> register a single generic device type */
-base_type_info.name = t->generic_name;
-base_type_info.class_init = virtio_pci_generic_base_class_init;
-base_type_info.interfaces = generic_type_info.interfaces;
-base_type_info.abstract = false;
-generic_type_info.name = NULL;
+/* use intermediate %s-base-type to add generic device props */
+base_name = g_strdup_printf("%s-base-type", t->generic_name);
+base_type_info.name = base_name;
+base_type_info.class_init = virtio_pci_generic_class_init;
+
+generic_type_info.parent = base_name;
+generic_type_info.class_init = virtio_pci_base_class_init;
+generic_type_info.class_data = (void *)t;
+
 assert(!t->non_transitional_name);
 assert(!t->transitional_name);
+} else {
+base_type_info.class_init = virtio_pci_base_class_init;
+base_type_info.class_data = (void *)t;
 }
 
 type_register(_type_info);
@@ -2005,6 +2004,7 @@ void virtio_pci_types_register(const 
VirtioPCIDeviceTypeInfo *t)
 };
 type_register(_type_info);
 }
+g_free(base_name);
 }
 
 /* virtio-pci-bus */
-- 
2.22.0.rc2.384.g1a9a72ea1d

Re: [Qemu-devel] [PATCH] migration: Use RunState enum to save global state pre migrate

[Maxiwell S. Garcia]

On Tue, Jun 25, 2019 at 11:18:00AM +0100, Dr. David Alan Gilbert wrote:
> * Maxiwell S. Garcia (maxiw...@linux.ibm.com) wrote:
> > The GlobalState struct has two confusing fields:
> > - uint8_t runstate[100]
> > - RunState state
> > 
> > The first field saves the 'current_run_state' from vl.c file before
> > migrate. The second field is filled in the post load func using the
> > 'runstate' value. So, this commit renames the 'runstate' to
> > 'state_pre_migrate' and use the same type used by 'state' and
> > 'current_run_state' variables.
> > 
> > Signed-off-by: Maxiwell S. Garcia 
> 
> Hi,
>   Thanks for the patch.
> 
>   Unfortunately this wont work for a few different reasons:
> 
>   a) 'RunState' is an enum whose order and encoding is not specified - 
>  to keep migration compatibility the wire format must be stable.
>  The textual version is more stable.
> 
>   b) It's also too late to change it, because existing migration streams
>  send the textual Runstate; this change breaks migration
>  compatibility from/to existing qemu's.
> 

It makes sense. What do you think about adding a comment to explain it
(as suggest Philippe) and change the 'runstate' to 'state_stored' (or
'state_pre_migrate') to improve the legibility?

Thanks,


> Dave
> 
> > ---
> >  include/sysemu/sysemu.h  |  2 +-
> >  migration/global_state.c | 65 ++--
> >  vl.c | 11 ++-
> >  3 files changed, 12 insertions(+), 66 deletions(-)
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 61579ae71e..483b536c4f 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -23,7 +23,7 @@ bool runstate_check(RunState state);
> >  void runstate_set(RunState new_state);
> >  int runstate_is_running(void);
> >  bool runstate_needs_reset(void);
> > -bool runstate_store(char *str, size_t size);
> > +RunState runstate_get(void);
> >  typedef struct vm_change_state_entry VMChangeStateEntry;
> >  typedef void VMChangeStateHandler(void *opaque, int running, RunState 
> > state);
> >  
> > diff --git a/migration/global_state.c b/migration/global_state.c
> > index 2c8c447239..b49b99f3a1 100644
> > --- a/migration/global_state.c
> > +++ b/migration/global_state.c
> > @@ -20,8 +20,7 @@
> >  #include "trace.h"
> >  
> >  typedef struct {
> > -uint32_t size;
> > -uint8_t runstate[100];
> > +RunState state_pre_migrate;
> >  RunState state;
> >  bool received;
> >  } GlobalState;
> > @@ -30,21 +29,14 @@ static GlobalState global_state;
> >  
> >  int global_state_store(void)
> >  {
> > -if (!runstate_store((char *)global_state.runstate,
> > -sizeof(global_state.runstate))) {
> > -error_report("runstate name too big: %s", global_state.runstate);
> > -trace_migrate_state_too_big();
> > -return -EINVAL;
> > -}
> > +global_state.state_pre_migrate = runstate_get();
> > +
> >  return 0;
> >  }
> >  
> >  void global_state_store_running(void)
> >  {
> > -const char *state = RunState_str(RUN_STATE_RUNNING);
> > -assert(strlen(state) < sizeof(global_state.runstate));
> > -strncpy((char *)global_state.runstate,
> > -   state, sizeof(global_state.runstate));
> > +global_state.state_pre_migrate = RUN_STATE_RUNNING;
> >  }
> >  
> >  bool global_state_received(void)
> > @@ -60,7 +52,6 @@ RunState global_state_get_runstate(void)
> >  static bool global_state_needed(void *opaque)
> >  {
> >  GlobalState *s = opaque;
> > -char *runstate = (char *)s->runstate;
> >  
> >  /* If it is not optional, it is mandatory */
> >  
> > @@ -70,8 +61,8 @@ static bool global_state_needed(void *opaque)
> >  
> >  /* If state is running or paused, it is not needed */
> >  
> > -if (strcmp(runstate, "running") == 0 ||
> > -strcmp(runstate, "paused") == 0) {
> > +if (s->state_pre_migrate == RUN_STATE_RUNNING ||
> > +s->state_pre_migrate == RUN_STATE_PAUSED) {
> >  return false;
> >  }
> >  
> > @@ -82,45 +73,10 @@ static bool global_state_needed(void *opaque)
> >  static int global_state_post_load(void *opaque, int version_id)
> >  {
> >  GlobalState *s = opaque;
> > -Error *local_err = NULL;
> > -int r;
> > -char *runstate = (char *)s->runstate;
> > -
> >  s->received = true;
> > -trace_migrate_global_state_post_load(runstate);
> > -
> > -if (strnlen((char *)s->runstate,
> > -sizeof(s->runstate)) == sizeof(s->runstate)) {
> > -/*
> > - * This condition should never happen during migration, because
> > - * all runstate names are shorter than 100 bytes (the size of
> > - * s->runstate). However, a malicious stream could overflow
> > - * the qapi_enum_parse() call, so we force the last character
> > - * to a NUL byte.
> > - */
> > -s->runstate[sizeof(s->runstate) - 1] = '\0';
> > -}
> > -r = 

[Qemu-devel] [PATCH v2 3/5] iotests: Allow skipping test cases

[Max Reitz]

case_notrun() does not actually skip the current test case.  It just
adds a "notrun" note and then returns to the caller, who manually has to
skip the test.  Generally, skipping a test case is as simple as
returning from the current function, but not always: For example, this
model does not allow skipping tests already in the setUp() function.

Thus, add a QMPTestCase.case_skip() function that invokes case_notrun()
and then self.skipTest().  To make this work, we need to filter the
information on how many test cases were skipped from the unittest
output.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3ecef5bc90..1b0ac50153 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -741,6 +741,11 @@ class QMPTestCase(unittest.TestCase):
 return self.pause_wait(job_id)
 return result
 
+def case_skip(self, reason):
+'''Skip this test case'''
+case_notrun(reason)
+self.skipTest(reason)
+
 
 def notrun(reason):
 '''Skip this test suite'''
@@ -752,7 +757,10 @@ def notrun(reason):
 sys.exit(0)
 
 def case_notrun(reason):
-'''Skip this test case'''
+'''Mark this test case as not having been run, but do not actually
+skip it; that is left to the caller.  See QMPTestCase.case_skip()
+for a variant that actually skips the current test case.'''
+
 # Each test in qemu-iotests has a number ("seq")
 seq = os.path.basename(sys.argv[0])
 
@@ -879,4 +887,12 @@ def main(supported_fmts=[], supported_oses=['linux'], 
supported_cache_modes=[],
 unittest.main(testRunner=MyTestRunner)
 finally:
 if not debug:
-sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 
tests', output.getvalue()))
+out = output.getvalue()
+out = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', out)
+
+# Hide skipped tests from the reference output
+out = re.sub(r'OK \(skipped=\d+\)', 'OK', out)
+out_first_line, out_rest = out.split('\n', 1)
+out = out_first_line.replace('s', '.') + '\n' + out_rest
+
+sys.stderr.write(out)
-- 
2.21.0

[Qemu-devel] [PATCH v2 5/5] iotests: Test driver whitelisting in 136

[Max Reitz]

null-aio may not be whitelisted.  Skip all test cases that require it.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/136 | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index af7ffa4540..4ef95517a1 100755
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -28,9 +28,11 @@ op_latency = nsec_per_sec // 1000 # See qtest_latency_ns in 
accounting.c
 bad_sector = 8192
 bad_offset = bad_sector * 512
 blkdebug_file = os.path.join(iotests.test_dir, 'blkdebug.conf')
+supported_null_drivers = [ f for f in iotests.supported_formats()
+   if f.startswith('null-') ]
 
 class BlockDeviceStatsTestCase(iotests.QMPTestCase):
-test_img = "null-aio://"
+test_driver = "null-aio"
 total_rd_bytes = 0
 total_rd_ops = 0
 total_wr_bytes = 0
@@ -68,6 +70,10 @@ sector = "%d"
 file.close()
 
 def setUp(self):
+global supported_null_drivers
+if self.test_driver not in supported_null_drivers:
+self.case_skip('%s support missing' % self.test_driver)
+
 drive_args = []
 drive_args.append("stats-intervals.0=%d" % interval_length)
 drive_args.append("stats-account-invalid=%s" %
@@ -75,8 +81,8 @@ sector = "%d"
 drive_args.append("stats-account-failed=%s" %
   (self.account_failed and "on" or "off"))
 self.create_blkdebug_file()
-self.vm = iotests.VM().add_drive('blkdebug:%s:%s' %
- (blkdebug_file, self.test_img),
+self.vm = iotests.VM().add_drive('blkdebug:%s:%s://' %
+ (blkdebug_file, self.test_driver),
  ','.join(drive_args))
 self.vm.launch()
 # Set an initial value for the clock
@@ -336,7 +342,9 @@ class 
BlockDeviceStatsTestAccountBoth(BlockDeviceStatsTestCase):
 account_failed = True
 
 class BlockDeviceStatsTestCoroutine(BlockDeviceStatsTestCase):
-test_img = "null-co://"
+test_driver = "null-co"
 
 if __name__ == '__main__':
+if 'null-co' not in supported_null_drivers:
+iotests.notrun('null-co driver support missing')
 iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-devel] [PATCH v2 4/5] iotests: Test driver whitelisting in 093

[Max Reitz]

null-aio may not be whitelisted.  Skip all test cases that require it.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/093 | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index bd56c94708..806bdcaa24 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -22,9 +22,11 @@
 import iotests
 
 nsec_per_sec = 10
+supported_null_drivers = [ f for f in iotests.supported_formats()
+   if f.startswith('null-') ]
 
 class ThrottleTestCase(iotests.QMPTestCase):
-test_img = "null-aio://"
+test_driver = "null-aio"
 max_drives = 3
 
 def blockstats(self, device):
@@ -36,9 +38,13 @@ class ThrottleTestCase(iotests.QMPTestCase):
 raise Exception("Device not found for blockstats: %s" % device)
 
 def setUp(self):
+global supported_null_drivers
+if self.test_driver not in supported_null_drivers:
+self.case_skip('%s support missing' % self.test_driver)
+
 self.vm = iotests.VM()
 for i in range(0, self.max_drives):
-self.vm.add_drive(self.test_img)
+self.vm.add_drive(self.test_driver + "://")
 self.vm.launch()
 
 def tearDown(self):
@@ -264,7 +270,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
 self.assertEqual(self.blockstats('drive1')[0], 4096)
 
 class ThrottleTestCoroutine(ThrottleTestCase):
-test_img = "null-co://"
+test_driver = "null-co"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
 max_drives = 3
@@ -426,4 +432,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 
 if __name__ == '__main__':
+if 'null-co' not in supported_null_drivers:
+iotests.notrun('null-co driver support missing')
 iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-devel] [PATCH v2 2/5] iotests: Prefer null-co over null-aio

[Max Reitz]

We use null-co basically everywhere in the iotests.  Unless we want to
test null-aio specifically, we should use it instead (for consistency).

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/093 | 7 +++
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index d88fbc182e..bd56c94708 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -267,13 +267,12 @@ class ThrottleTestCoroutine(ThrottleTestCase):
 test_img = "null-co://"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
-test_img = "null-aio://"
 max_drives = 3
 
 def setUp(self):
 self.vm = iotests.VM()
 for i in range(0, self.max_drives):
-self.vm.add_drive(self.test_img, "throttling.iops-total=100")
+self.vm.add_drive("null-co://", "throttling.iops-total=100")
 self.vm.launch()
 
 def tearDown(self):
@@ -377,10 +376,10 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 def test_removable_media(self):
 # Add a couple of dummy nodes named cd0 and cd1
-result = self.vm.qmp("blockdev-add", driver="null-aio",
+result = self.vm.qmp("blockdev-add", driver="null-co",
  node_name="cd0")
 self.assert_qmp(result, 'return', {})
-result = self.vm.qmp("blockdev-add", driver="null-aio",
+result = self.vm.qmp("blockdev-add", driver="null-co",
  node_name="cd1")
 self.assert_qmp(result, 'return', {})
 
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bc1ceb9792..ae169778b0 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -598,7 +598,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 ##
 ## null ##
 ##
-opts = {'driver': 'null-aio', 'node-name': 'root', 'size': 1024}
+opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024}
 
 result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
 self.assert_qmp(result, 'return', {})
-- 
2.21.0

[Qemu-devel] [PATCH v2 1/5] iotests: Add -display none to the qemu options

[Max Reitz]

Without this argument, qemu will print an angry message about not being
able to connect to a display server if $DISPLAY is not set.  For me,
that breaks iotests.supported_formats() because it thus only sees
["Could", "not", "connect"] as the supported formats.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/check | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index f925606cc5..f2a0df8af2 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -581,13 +581,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
 
 case "$QEMU_PROG" in
 *qemu-system-arm|*qemu-system-aarch64)
-export QEMU_OPTIONS="-nodefaults -machine virt,accel=qtest"
+export QEMU_OPTIONS="-nodefaults -display none -machine 
virt,accel=qtest"
 ;;
 *qemu-system-tricore)
-export QEMU_OPTIONS="-nodefaults -machine 
tricore_testboard,accel=qtest"
+export QEMU_OPTIONS="-nodefaults -display none -machine 
tricore_testboard,accel=qtest"
 ;;
 *)
-export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
+export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
 ;;
 esac
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 0/5] iotests: Selfish patches

[Max Reitz]

Hi,

These are some rather selfish iotests patches.  The first patch helps me
personally because I tend to run the tests over SSH and forget to set
$DISPLAY.  That makes test 139 skip the tests annotated with
skip_if_unsupprted(), because iotests.py can no longer determine the
list of whitelisted formats.

Patches 2 through 5 are specifically for RHEL.  We have not whitelisted
null-aio, so it would be nice if tests didn’t require it.  Sorry, I
don’t have a better reason to give.
In all seriousness, null-co is used widely in many tests, it basically
is our standard null driver.  Tests should prefer it over null-aio, just
for consistency alone.  It is not completely unreasonable to treat
null-aio as optional.  I guess.


v2:
- Allow tests to use the unittest module’s skipTest() so it is possible
  to skip a test case in the setUp() method (new patch 3).  Then use
  this in patches 4 and 5 to skip all null-aio cases instead of falling
  back to null-co and thus running tests twice.

- Patch 1 needed to be rebased on
  4a715461c8eab628e79b1e6889d650455e043b88
  (“tests/qemu-iotests/check: Pick a default machine if necessary”)


git backport-diff against v1:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0008] [FC] 'iotests: Add -display none to the qemu options'
002/5:[] [--] 'iotests: Prefer null-co over null-aio'
003/5:[down] 'iotests: Allow skipping test cases'
004/5:[0007] [FC] 'iotests: Test driver whitelisting in 093'
005/5:[0007] [FC] 'iotests: Test driver whitelisting in 136'


Max Reitz (5):
  iotests: Add -display none to the qemu options
  iotests: Prefer null-co over null-aio
  iotests: Allow skipping test cases
  iotests: Test driver whitelisting in 093
  iotests: Test driver whitelisting in 136

 tests/qemu-iotests/093| 21 ++---
 tests/qemu-iotests/136| 16 
 tests/qemu-iotests/245|  2 +-
 tests/qemu-iotests/check  |  6 +++---
 tests/qemu-iotests/iotests.py | 20 ++--
 5 files changed, 48 insertions(+), 17 deletions(-)

-- 
2.21.0

Re: [Qemu-devel] [PULL 8/9] virtio-gpu: split virtio-gpu-pci & virtio-vga

[Marc-André Lureau]

Hi

On Mon, Jun 24, 2019 at 5:53 PM Laurent Vivier  wrote:
>
> On 29/05/2019 06:40, Gerd Hoffmann wrote:
> > From: Marc-André Lureau 
> >
> > Add base classes that are common to vhost-user-gpu-pci and
> > vhost-user-vga.
> >
> > Signed-off-by: Marc-André Lureau 
> > Message-id: 20190524130946.31736-9-marcandre.lur...@redhat.com
> > Signed-off-by: Gerd Hoffmann 
> > ---
> >  hw/display/virtio-vga.h |  32 +
> >  hw/display/virtio-gpu-pci.c |  52 +-
> >  hw/display/virtio-vga.c | 135 ++--
> >  MAINTAINERS |   2 +-
> >  4 files changed, 137 insertions(+), 84 deletions(-)
> >  create mode 100644 hw/display/virtio-vga.h
> >
>
> This patch breaks something in the migration (no need of an OS, tested during 
> SLOF sequence).
>
> Tested between v4.0.0 and master.
>
> v4.0.0: ppc64-softmmu/qemu-system-ppc64 -machine pseries-4.0 \
> -device virtio-gpu-pci \
> -serial mon:stdio -incoming tcp:0:
>
> master: ppc64-softmmu/qemu-system-ppc64 -machine pseries-4.0 \
> -device virtio-gpu-pci \
> -serial mon:stdio
>
>
> master: (qemu) migrate tcp:localhost:
>
> v4.0.0:
>
>   qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x34 read: 98 
> device: 84 cmask: ff wmask: 0 w1cmask:0
>   qemu-system-ppc64: Failed to load PCIDevice:config
>   qemu-system-ppc64: Failed to load virtio-gpu:virtio
>   qemu-system-ppc64: error while loading state for instance 0x0 of device 
> 'pci@8002000:02.0/virtio-gpu'
>   qemu-system-ppc64: load of migration failed: Invalid argument
>
> Is this something known?

Thanks for the report, I wasn't aware of it.

The problem actually started from:

commit a4ee4c8baa37154f42b4dc6a13fee79268d15238
Author: Eduardo Habkost 
Date:   Wed Dec 5 17:57:03 2018 -0200

virtio: Helper for registering virtio device types


Since that commit virtio_gpu_pci_properties got lost (overwritten in
virtio_pci_generic_class_init), and thus the PCI device changed (bar1
missing etc).

My patches actually restored it as a side effect of defining a new base class.

There are other devices affected with missing vectors and ioeventfd
(and sometime a few others props): virtio-crypto-pci, virtio-9p-pci,
vhost-user-blk-pci and more. They probably suffer similar migration
issues.

Given that this is a 4.0 regression, I think we should fix the
remaining devices, and backport fixes to 4.0-stable. I am looking at
it




--
Marc-André Lureau

Re: [Qemu-devel] [PATCH for 4.1 v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren

[Jonathan Behrens]

I just did some testing on a HiFive Unleashed board and can confirm what
you are saying. The low 5 bits of both mcounteren and scounteren are
writable (if you try to write 0x to them, they'll take on the value
0x1F) but even with the TM bit set in both mcounteren and scounteren the
rdtime instruction always generates an illegal instruction exception.

Reading through the relevant chapter of the spec, I still think that having
mcounteren.TM be writable but making rdtime unconditionally trap is
non-conformant. If other people feel strongly that rdtime should always
require trapping into firmware then the natural change would be to simply
hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in
that case so it could be left writable). My own (biased) personal feeling
is that this full implementation makes sense at least for the `virt`
machine type because it represents a clear case where deviating from
current hardware enables a performance boost, and would not break
compatibility with any current software: both OpenSBI and BBL try to enable
hardware handling of rdtime when the platform claims to support it.

On Tue, Jun 25, 2019 at 5:56 AM Palmer Dabbelt  wrote:

> On Mon, 24 Jun 2019 16:03:20 PDT (-0700), finte...@gmail.com wrote:
> > Apparently my previous message didn't make it out onto the list (sorry
> > about all these email glitches!). I've included the message again below.
> > Hopefully either a patch like this one or something simpler that just
> hard
> > codes mcounteren.TM to zero (so QEMU is at least conformant) can be
> merged
> > in time for 4.1.
> >
> > On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens 
> > wrote:
> >
> >> I'm not sure that is accurate. Based on the discussion here
> >>  the
> >> HiFive Unleashed actually does support reading the timer CSR from
> >> unprivileged modes (from that discussion it does so a little too well...
> >> but it should presumably be fixed in later iterations of the processor).
> >> And even if no real hardware supported this capability, it still might
> make
> >> sense to provide it in QEMU as an optimization.
>
> time and cycle are different registers: rdtime should trap, but rdcycle
> should
> work.  The hardware traps rdtime into machine mode and emulates it via a
> memory
> mapped register.  The bug in the FU540-C000 appears to be related to the
> counter enables, not the presence of the counters.
>
> >>
> >> On Fri, Jun 14, 2019 at 7:52 AM Palmer Dabbelt 
> wrote:
> >>
> >>> On Tue, 28 May 2019 11:30:20 PDT (-0700), jonat...@fintelia.io wrote:
> >>> > Currently mcounteren.TM acts as though it is hardwired to zero, even
> >>> though QEMU allows it to be set. This change resolves the issue by
> allowing
> >>> reads to the time and timeh control registers when running in a
> privileged
> >>> mode where such accesses are allowed.
> >>> >
> >>> > The frequency of the time register is stored in the time_freq field
> of
> >>> each hart so that it is accessible during CSR reads, but must be the
> same
> >>> across all harts. Each board can initialize it to a custom value,
> although
> >>> all currently use a 10 MHz frequency.
> >>> >
> >>> > Signed-off-by: Jonathan Behrens 
> >>> > ---
> >>> >  hw/riscv/riscv_hart.c   |  4 
> >>> >  hw/riscv/sifive_clint.c | 30 ++
> >>> >  hw/riscv/sifive_e.c |  2 ++
> >>> >  hw/riscv/sifive_u.c |  4 +++-
> >>> >  hw/riscv/spike.c|  6 +-
> >>> >  hw/riscv/virt.c |  4 +++-
> >>> >  include/hw/riscv/riscv_hart.h   |  1 +
> >>> >  include/hw/riscv/sifive_clint.h |  4 
> >>> >  include/hw/riscv/sifive_e.h |  4 
> >>> >  include/hw/riscv/sifive_u.h |  1 +
> >>> >  include/hw/riscv/spike.h|  1 +
> >>> >  include/hw/riscv/virt.h |  1 +
> >>> >  target/riscv/cpu.h  |  2 ++
> >>> >  target/riscv/csr.c  | 17 +++--
> >>> >  14 files changed, 60 insertions(+), 21 deletions(-)
> >>> >
> >>> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
> >>> > index e34a26a0ef..c39cd55330 100644
> >>> > --- a/hw/riscv/riscv_hart.c
> >>> > +++ b/hw/riscv/riscv_hart.c
> >>> > @@ -19,6 +19,7 @@
> >>> >   */
> >>> >
> >>> >  #include "qemu/osdep.h"
> >>> > +#include "qemu/timer.h"
> >>> >  #include "qapi/error.h"
> >>> >  #include "hw/sysbus.h"
> >>> >  #include "target/riscv/cpu.h"
> >>> > @@ -27,6 +28,8 @@
> >>> >  static Property riscv_harts_props[] = {
> >>> >  DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts,
> 1),
> >>> >  DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
> >>> > +DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState,
> >>> time_freq,
> >>> > +   NANOSECONDS_PER_SECOND),
> >>> >  DEFINE_PROP_END_OF_LIST(),
> >>> >  };
> >>> >
> >>> > @@ -49,6 +52,7 @@ static void 

Re: [Qemu-devel] [PATCH] target/i386: fix feature check in hyperv-stub.c

[Eduardo Habkost]

On Mon, Jun 24, 2019 at 03:37:24PM +0200, Paolo Bonzini wrote:
> On 24/06/19 15:22, Paolo Bonzini wrote:
> > On 24/06/19 14:38, Alex Bennée wrote:
> >> Commit 2d384d7c8 broken the build when built with:
> >>
> >>   configure --without-default-devices --disable-user
> >>
> >> The reason was the conversion of cpu->hyperv_synic to
> >> cpu->hyperv_synic_kvm_only although the rest of the patch introduces a
> >> feature checking mechanism. So I've fixed the KVM_EXIT_HYPERV_SYNIC in
> >> hyperv-stub to do the same feature check as in the real hyperv.c
> >>
> >> Signed-off-by: Alex Bennée 
> >> Cc: Vitaly Kuznetsov 
> >> Cc: Paolo Bonzini 
> >> Cc: Roman Kagan 
> >> ---
> >>  target/i386/hyperv-stub.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/target/i386/hyperv-stub.c b/target/i386/hyperv-stub.c
> >> index fe548cbae2..0028527e79 100644
> >> --- a/target/i386/hyperv-stub.c
> >> +++ b/target/i386/hyperv-stub.c
> >> @@ -15,7 +15,7 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct 
> >> kvm_hyperv_exit *exit)
> >>  {
> >>  switch (exit->type) {
> >>  case KVM_EXIT_HYPERV_SYNIC:
> >> -if (!cpu->hyperv_synic) {
> >> +if (!hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC)) {
> >>  return -1;
> >>  }
> >>  
> >>
> > 
> > Queued, thanks.
> 
> Alex will queue it instead, so
> 
> Acked-by: Paolo Bonzini 

I was planning to send a machine + x86 pull request today, and
I'll have to include to make sure builds won't fail.  I don't
think this should prevent the patch from being applied to other
trees, though.

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH v2] deprecate -mem-path fallback to anonymous RAM

[Daniel P . Berrangé]

On Tue, Jun 25, 2019 at 01:18:01PM -0500, Eric Blake wrote:
> On 6/25/19 11:16 AM, Igor Mammedov wrote:
> > Fallback might affect guest or worse whole host performance
> > or functionality if backing file were used to share guest RAM
> > with another process.
> > 
> > Patch deprecates fallback so that we could remove it in future
> > and ensure that QEMU will provide expected behavior and fail if
> > it can't use user provided backing file.
> > 
> > Signed-off-by: Igor Mammedov 
> > ---
> > v2:
> >  * improve text language
> > (Markus Armbruster )
> > 
> 
> Is this deprecation introspectible? Does it need to be?
> 
> Do we even need a deprecation period, or can we declare this a bug fix
> (it was a bug that we didn't fail outright on an impossible request) and
> do it immediately?

I think it is hard to call it a bug when we added explicit extra code to
make it work as it does today.

It is really a misguided feature.

> If it is not a bug fix, perhaps it could be made introspectible by
> having a new boolean parameter to opt in to the failure now, rather than
> 2 releases from now?

>From libvirt's POV I don't see a need for introspection. There's no
special action we need to take to deal with the new behaviour - it is
ultimately just providing the behaviour we kind of assumed it already
had.


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

Re: [Qemu-devel] [libvirt] [PATCH v2] deprecate -mem-path fallback to anonymous RAM

[Eric Blake]

On 6/25/19 11:16 AM, Igor Mammedov wrote:
> Fallback might affect guest or worse whole host performance
> or functionality if backing file were used to share guest RAM
> with another process.
> 
> Patch deprecates fallback so that we could remove it in future
> and ensure that QEMU will provide expected behavior and fail if
> it can't use user provided backing file.
> 
> Signed-off-by: Igor Mammedov 
> ---
> v2:
>  * improve text language
> (Markus Armbruster )
> 

Is this deprecation introspectible? Does it need to be?

Do we even need a deprecation period, or can we declare this a bug fix
(it was a bug that we didn't fail outright on an impossible request) and
do it immediately?

If it is not a bug fix, perhaps it could be made introspectible by
having a new boolean parameter to opt in to the failure now, rather than
2 releases from now?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 06/10] hw/mips/gt64xxx_pci: Convert debug printf()s to trace events

[Aleksandar Markovic]

On Jun 25, 2019 12:46 AM, "Philippe Mathieu-Daudé"  wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---

Reviewed-by: Aleksandar Markovic 

>  Makefile.objs |  1 +
>  hw/mips/gt64xxx_pci.c | 29 ++---
>  hw/mips/trace-events  |  4 
>  3 files changed, 15 insertions(+), 19 deletions(-)
>  create mode 100644 hw/mips/trace-events
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 658cfc9d9f..3b83621f32 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -163,6 +163,7 @@ trace-events-subdirs += hw/input
>  trace-events-subdirs += hw/intc
>  trace-events-subdirs += hw/isa
>  trace-events-subdirs += hw/mem
> +trace-events-subdirs += hw/mips
>  trace-events-subdirs += hw/misc
>  trace-events-subdirs += hw/misc/macio
>  trace-events-subdirs += hw/net
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index f44326f14f..815ef0711d 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -30,14 +30,7 @@
>  #include "hw/pci/pci_host.h"
>  #include "hw/i386/pc.h"
>  #include "exec/address-spaces.h"
> -
> -//#define DEBUG
> -
> -#ifdef DEBUG
> -#define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__,
##__VA_ARGS__)
> -#else
> -#define DPRINTF(fmt, ...)
> -#endif
> +#include "trace.h"
>
>  #define GT_REGS (0x1000 >> 2)
>
> @@ -294,9 +287,7 @@ static void gt64120_isd_mapping(GT64120State *s)
>  check_reserved_space(, );
>  length = 0x1000;
>  /* Map new address */
> -DPRINTF("ISD: "TARGET_FMT_plx"@"TARGET_FMT_plx
> -" -> "TARGET_FMT_plx"@"TARGET_FMT_plx"\n",
> -s->ISD_length, s->ISD_start, length, start);
> +trace_gt64120_isd_remap(s->ISD_length, s->ISD_start, length, start);
>  s->ISD_start = start;
>  s->ISD_length = length;
>  memory_region_add_subregion(get_system_memory(), s->ISD_start,
>ISD_mem);
> @@ -648,19 +639,19 @@ static void gt64120_writel(void *opaque, hwaddr
addr,
>  /* not really implemented */
>  s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffe));
>  s->regs[saddr] |= !!(s->regs[saddr] & 0xfffe);
> -DPRINTF("INTRCAUSE %" PRIx64 "\n", val);
> +trace_gt64120_write("INTRCAUSE", size << 1, val);
>  break;
>  case GT_INTRMASK:
>  s->regs[saddr] = val & 0x3c3e;
> -DPRINTF("INTRMASK %" PRIx64 "\n", val);
> +trace_gt64120_write("INTRMASK", size << 1, val);
>  break;
>  case GT_PCI0_ICMASK:
>  s->regs[saddr] = val & 0x03fe;
> -DPRINTF("ICMASK %" PRIx64 "\n", val);
> +trace_gt64120_write("ICMASK", size << 1, val);
>  break;
>  case GT_PCI0_SERR0MASK:
>  s->regs[saddr] = val & 0x003f;
> -DPRINTF("SERR0MASK %" PRIx64 "\n", val);
> +trace_gt64120_write("SERR0MASK", size << 1, val);
>  break;
>
>  /* Reserved when only PCI_0 is configured. */
> @@ -936,19 +927,19 @@ static uint64_t gt64120_readl(void *opaque,
>  /* Interrupts */
>  case GT_INTRCAUSE:
>  val = s->regs[saddr];
> -DPRINTF("INTRCAUSE %x\n", val);
> +trace_gt64120_read("INTRCAUSE", size << 1, val);
>  break;
>  case GT_INTRMASK:
>  val = s->regs[saddr];
> -DPRINTF("INTRMASK %x\n", val);
> +trace_gt64120_read("INTRMASK", size << 1, val);
>  break;
>  case GT_PCI0_ICMASK:
>  val = s->regs[saddr];
> -DPRINTF("ICMASK %x\n", val);
> +trace_gt64120_read("ICMASK", size << 1, val);
>  break;
>  case GT_PCI0_SERR0MASK:
>  val = s->regs[saddr];
> -DPRINTF("SERR0MASK %x\n", val);
> +trace_gt64120_read("SERR0MASK", size << 1, val);
>  break;
>
>  /* Reserved when only PCI_0 is configured. */
> diff --git a/hw/mips/trace-events b/hw/mips/trace-events
> new file mode 100644
> index 00..75d4c73f2e
> --- /dev/null
> +++ b/hw/mips/trace-events
> @@ -0,0 +1,4 @@
> +# gt64xxx.c
> +gt64120_read(const char *regname, int width, uint64_t value) "gt64120
read %s value:0x%0*" PRIx64
> +gt64120_write(const char *regname, int width, uint64_t value) "gt64120
write %s value:0x%0*" PRIx64
> +gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t
to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " ->
0x%08" PRIx64 "@0x%08" PRIx64
> --
> 2.19.1
>
>

Re: [Qemu-devel] [PATCH 05/10] hw/mips/gt64xxx_pci: Use qemu_log_mask() instead of debug printf()

[Aleksandar Markovic]

On Jun 25, 2019 12:42 AM, "Philippe Mathieu-Daudé"  wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---

Reviewed-by: Aleksandar Markovic 

>  hw/mips/gt64xxx_pci.c | 48 +--
>  1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 0b9fb02475..f44326f14f 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -23,6 +23,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "hw/hw.h"
>  #include "hw/mips/mips.h"
>  #include "hw/pci/pci.h"
> @@ -466,12 +467,20 @@ static void gt64120_writel(void *opaque, hwaddr
addr,
>  case GT_CPUERR_DATAHI:
>  case GT_CPUERR_PARITY:
>  /* Read-only registers, do nothing */
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "gt64120: Read-only register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>
>  /* CPU Sync Barrier */
>  case GT_PCI0SYNC:
>  case GT_PCI1SYNC:
>  /* Read-only registers, do nothing */
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "gt64120: Read-only register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>
>  /* SDRAM and Device Address Decode */
> @@ -510,7 +519,10 @@ static void gt64120_writel(void *opaque, hwaddr addr,
>  case GT_DEV_B3:
>  case GT_DEV_BOOT:
>  /* Not implemented */
> -DPRINTF ("Unimplemented device register offset 0x%x\n", saddr <<
2);
> +qemu_log_mask(LOG_UNIMP,
> +  "gt64120: Unimplemented device register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>
>  /* ECC */
> @@ -520,6 +532,10 @@ static void gt64120_writel(void *opaque, hwaddr addr,
>  case GT_ECC_CALC:
>  case GT_ECC_ERRADDR:
>  /* Read-only registers, do nothing */
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "gt64120: Read-only register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>
>  /* DMA Record */
> @@ -543,23 +559,20 @@ static void gt64120_writel(void *opaque, hwaddr
addr,
>  case GT_DMA1_CUR:
>  case GT_DMA2_CUR:
>  case GT_DMA3_CUR:
> -/* Not implemented */
> -DPRINTF ("Unimplemented DMA register offset 0x%x\n", saddr << 2);
> -break;
>
>  /* DMA Channel Control */
>  case GT_DMA0_CTRL:
>  case GT_DMA1_CTRL:
>  case GT_DMA2_CTRL:
>  case GT_DMA3_CTRL:
> -/* Not implemented */
> -DPRINTF ("Unimplemented DMA register offset 0x%x\n", saddr << 2);
> -break;
>
>  /* DMA Arbiter */
>  case GT_DMA_ARB:
>  /* Not implemented */
> -DPRINTF ("Unimplemented DMA register offset 0x%x\n", saddr << 2);
> +qemu_log_mask(LOG_UNIMP,
> +  "gt64120: Unimplemented DMA register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>
>  /* Timer/Counter */
> @@ -569,7 +582,10 @@ static void gt64120_writel(void *opaque, hwaddr addr,
>  case GT_TC3:
>  case GT_TC_CONTROL:
>  /* Not implemented */
> -DPRINTF ("Unimplemented timer register offset 0x%x\n", saddr <<
2);
> +qemu_log_mask(LOG_UNIMP,
> +  "gt64120: Unimplemented timer register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>
>  /* PCI Internal */
> @@ -610,6 +626,10 @@ static void gt64120_writel(void *opaque, hwaddr addr,
>  case GT_PCI1_CFGADDR:
>  case GT_PCI1_CFGDATA:
>  /* not implemented */
> +qemu_log_mask(LOG_UNIMP,
> +  "gt64120: Unimplemented timer register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>  case GT_PCI0_CFGADDR:
>  phb->config_reg = val & 0x80fc;
> @@ -666,7 +686,10 @@ static void gt64120_writel(void *opaque, hwaddr addr,
>  break;
>
>  default:
> -DPRINTF ("Bad register offset 0x%x\n", (int)addr);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "gt64120: Illegal register write "
> +  "reg:0x03%x size:%u value:0x%0*" PRIx64 "\n",
> +  saddr << 2, size, size << 1, val);
>  break;
>  }
>  }
> @@ -940,7 +963,10 @@ static uint64_t gt64120_readl(void *opaque,
>
>  default:
>  val = 

Re: [Qemu-devel] [PATCH 0/6] x86 CPU model versioning

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 05:13:57PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:02AM -0300, Eduardo Habkost wrote:
> > This series implements basic infrastructure for CPU model
> > versioning, as discussed before[1][2][3].  This will finally
> > allow us to update CPU models in ways that introduce new software
> > or hardware requirements.
> > 
> > My original plan was to use "query-cpu-model-expansion
> > mode=static" to resolve aliases, but I dropped that plan because
> > it would increase complexity for management software a lot.
> > static CPU models are documented as not being affected by the
> > machine type and accelerator at all, which would make the
> > versioned CPU models very inconvenient to use in the command
> > line.  e.g.: users would be forced to replace:
> > 
> >   -cpu Haswell
> > 
> > with:
> > 
> >   -cpu 
> > Haswell-4.1,+2apic,+monitor,+kvmclock,+kvm-nopiodelay,+kvm-asyncpf,+kvm-steal-time,+kvm-pv-eoi,+kvmclock-stable-bit,+x2apic,-acpi,-monitor,-svm
> > 
> > In the end, making the versioned CPU models static is not a
> > requirement at all: what we really need is to drop the
> > runnability guarantees from unversioned CPU model names, and
> > require management software to resolve the unversioned alias
> > before saving the VM configuration.
> > 
> > Guest ABI compatibility and live migration guarantees are going
> > to be kept: unversioned CPU models will still be usable with live
> > migration.  Only runnability guarantees when updating the machine
> > type will be dropped.  This means unversioned CPU models are
> > still reported as migration-safe in query-cpu-definitions.
> 
> I'm having a little bit of a hard time parsing the overall behaviour
> from the above description. Let me outline the examples so you can
> confirm if I've got it right.
> 
> Lets assume there is a VM configured using "Haswell"
> 
> Today a mgmt app would pass the CPU model name in to QEMU as is,
> and thus we get
> 
>   $QEMU -machine pc-i440fx-4.0 -cpu Haswell ...more args...
> 
> The semantics of "Haswell" here is going to vary according to
> what the machine type pc-i440fx-4.0 customizes wrt Haswell.
> 
> If the config later has the machine type changed to pc-i440fx-4.1
> the semantics of Haswell might change in an arbitrary way. It
> might even become unrunnable on the current hardware.

If we follow our deprecation policy, this shouldn't happen until
pc-*-4.3.  On pc-*-4.1 and pc-*-4.2, I expect "-cpu Haswell" to
stay runnable if it's already runnable with pc-*-4.0.  This will
give libvirt and other management software some time to adapt.

CPU model changes that don't affect runnability, though, are
already allowed and will still be allowed.


> 
> With the new versioned machine types, the VM config of "Haswell"
> would be resolved into some arbitrary versioned machine type
> "Haswell-NNN" and thus the mgmt app would launch
> 
>   $QEMU -machine pc-i440fx-4.0 -cpu Haswell-NNN ...more args...
> 
> The semantics of "Haswell-NNN" will never change no matter what
> the machine type does.

Exactly.

> 
> The end user has the option to explicitly give "Haswell-NNN" to
> the mgmt app if they know they want that particular variant, and
> in this case the mgmt app won't need to resolve anything (or at
> least the process of trying to resolve it won't change it).

Yes.  Management software should see that "Haswell-NNN" has no
"alias-of" field, so it doesn't need to be changed.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 07:08:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> > Base code for versioned CPU models.  This will register a "-4.1"
> > version of all existing CPU models, and make the unversioned CPU
> > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > types.
> 
> Currently we have some CPUs that I'd consider historical "mistakes"
> due to fact that versioning didn't previously exist.
> 
> eg
> 
>Haswell
>Haswell-noTSX
>Haswell-noTSX-IBRS
> 
> IIUC this patch adds
> 
>   Haswellalias-of Haswell-4.1
>   Haswell-noTSX  alias-of Haswell-noTSX-4.1
>   Haswell-noTSX-IBRS alias-of Haswell-noTSX-IBRS-4.1
> 
> I'm thinking we should instead be merging all these haswell variants
> 
> 
>   Haswellalias-of Haswell-4.1.1
>   Haswell-noTSX  alias-of Haswell-4.1.2
>   Haswell-noTSX-IBRS alias-of Haswell-4.1.3
> 
> Or if we used the simple counter versioning
> 
>   Haswellalias-of Haswell-1
>   Haswell-noTSX  alias-of Haswell-2
>   Haswell-noTSX-IBRS alias-of Haswell-3
> 
> Likewise for the other named CPUs with wierd variants.
> 
> This would effectively be "deprecating" the noTSX and IBRS variants
> in favour of using the versioning approach

Sounds good.  I will do it.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Daniel P . Berrangé]

On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> Base code for versioned CPU models.  This will register a "-4.1"
> version of all existing CPU models, and make the unversioned CPU
> models be an alias for the -4.1 versions on the pc-*-4.1 machine
> types.

Currently we have some CPUs that I'd consider historical "mistakes"
due to fact that versioning didn't previously exist.

eg

   Haswell
   Haswell-noTSX
   Haswell-noTSX-IBRS

IIUC this patch adds

  Haswellalias-of Haswell-4.1
  Haswell-noTSX  alias-of Haswell-noTSX-4.1
  Haswell-noTSX-IBRS alias-of Haswell-noTSX-IBRS-4.1

I'm thinking we should instead be merging all these haswell variants


  Haswellalias-of Haswell-4.1.1
  Haswell-noTSX  alias-of Haswell-4.1.2
  Haswell-noTSX-IBRS alias-of Haswell-4.1.3

Or if we used the simple counter versioning

  Haswellalias-of Haswell-1
  Haswell-noTSX  alias-of Haswell-2
  Haswell-noTSX-IBRS alias-of Haswell-3

Likewise for the other named CPUs with wierd variants.

This would effectively be "deprecating" the noTSX and IBRS variants
in favour of using the versioning approach

> 
> On older machine types, the unversioned CPU models will keep the
> old behavior.  This way, management software can use old machine
> types while resolving aliases if compatibility with older QEMU
> versions is required.
> 
> Using "-machine none", the unversioned CPU models will be aliases
> to the latest CPU model version.
> 
> Includes a test case to ensure that:
> old machine types won't report any alias to versioned CPU models;
> "pc-*-4.1" will return aliases to -4.1 CPU models;
> and "-machine none" will report aliases to some versioned CPU model.
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Cc: Paolo Bonzini 
> Cc: Richard Henderson 
> ---
>  include/hw/i386/pc.h   |   3 +
>  target/i386/cpu-qom.h  |  10 +-
>  target/i386/cpu.h  |  10 ++
>  hw/i386/pc.c   |   3 +
>  hw/i386/pc_piix.c  |   4 +
>  hw/i386/pc_q35.c   |   4 +
>  target/i386/cpu.c  | 159 +
>  tests/acceptance/x86_cpu_model_versions.py | 102 +
>  8 files changed, 263 insertions(+), 32 deletions(-)
>  create mode 100644 tests/acceptance/x86_cpu_model_versions.py
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index c54cc54a47..d2e2ed072f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -107,6 +107,9 @@ typedef struct PCMachineClass {
>  
>  /* Compat options: */
>  
> +/* Default CPU model version.  See x86_cpu_set_default_version(). */
> +const char *default_cpu_version;
> +
>  /* ACPI compat: */
>  bool has_acpi_build;
>  bool rsdp_in_ram;
> diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h
> index 22f95eb3a4..1a52f02a4c 100644
> --- a/target/i386/cpu-qom.h
> +++ b/target/i386/cpu-qom.h
> @@ -36,13 +36,7 @@
>  #define X86_CPU_GET_CLASS(obj) \
>  OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
>  
> -/**
> - * X86CPUDefinition:
> - *
> - * CPU model definition data that was not converted to QOM per-subclass
> - * property defaults yet.
> - */
> -typedef struct X86CPUDefinition X86CPUDefinition;
> +typedef struct X86CPUModel X86CPUModel;
>  
>  /**
>   * X86CPUClass:
> @@ -64,7 +58,7 @@ typedef struct X86CPUClass {
>  /* CPU definition, automatically loaded by instance_init if not NULL.
>   * Should be eventually replaced by subclass-specific property defaults.
>   */
> -X86CPUDefinition *cpu_def;
> +X86CPUModel *model;
>  
>  bool host_cpuid_required;
>  int ordering;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 25544fdaaa..800bee3c6a 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1925,6 +1925,16 @@ void apic_handle_tpr_access_report(DeviceState *d, 
> target_ulong ip,
>   */
>  void x86_cpu_change_kvm_default(const char *prop, const char *value);
>  
> +/*
> + * Set default CPU model version for all CPU models
> + *
> + * If set to NULL, the old unversioned CPU models will be used by default.
> + *
> + * If non-NULL, the unversioned CPU models will be aliases to the
> + * corresponding version.
> + */
> +void x86_cpu_set_default_version(const char *version);
> +
>  /* Return name of 32-bit register, from a R_* constant */
>  const char *get_register_name_32(unsigned int reg);
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e96360b47a..d2852a77f8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1561,6 +1561,9 @@ void pc_cpus_init(PCMachineState *pcms)
>  const CPUArchIdList *possible_cpus;
>  MachineState *ms = MACHINE(pcms);
>  MachineClass *mc = MACHINE_GET_CLASS(pcms);
> +PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
> +
> +x86_cpu_set_default_version(pcmc->default_cpu_version);
>  
>  /* Calculates the limit to CPU APIC ID values

Re: [Qemu-devel] [PATCH v2 1/4] add and link a statistic struct to TBs

[Alex Bennée]

Vanderson Martins do Rosario  writes:

> When the tb_flush removes a block and it's recreated, this shouldn't
> be creating a new block but using the one that is found by:
>
> lookup_result = g_list_find_custom(tb_ctx.tb_statistics, new_stats,
> statistics_cmp);
>
> So the tb_statisticics will be reused and we could just add this
> regen counter in the if statement: if (lookup_result)
>
> isn't that correct?

Yes, although as I said earlier you want to use a QHT hash table rather
than a g_list to avoid racing with multiple translations at once.

>
> Vanderson M. Rosario
>
>
> On Tue, Jun 25, 2019 at 1:28 PM Alex Bennée  wrote:
>
>>
>> Alex Bennée  writes:
>>
>> > vandersonmr  writes:
>> >
>> >> We want to store statistics for each TB even after flushes.
>> >> We do not want to modify or grow the TB struct.
>> >> So we create a new struct to contain this statistics and
>> >> link it to each TB while they are created.
>> >>
>> >> Signed-off-by: Vanderson M. do Rosario 
>> >> ---
>> >>  accel/tcg/translate-all.c | 40 +++
>> >>  include/exec/exec-all.h   | 21 
>> >>  include/exec/tb-context.h |  1 +
>> >>  include/qemu/log.h|  1 +
>> >>  4 files changed, 63 insertions(+)
>> >>
>> >> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> >> index 5d1e08b169..f7e99f90e0 100644
>> >> --- a/accel/tcg/translate-all.c
>> >> +++ b/accel/tcg/translate-all.c
>> >> @@ -1118,6 +1118,18 @@ static inline void code_gen_alloc(size_t tb_size)
>> >>  }
>> >>  }
>> >>
>> >> +static gint statistics_cmp(gconstpointer p1, gconstpointer p2)
>> >> +{
>> >> +const TBStatistics *a = (TBStatistics *) p1;
>> >> +const TBStatistics *b = (TBStatistics *) p2;
>> >> +
>> >> +return (a->pc == b->pc &&
>> >> +   a->cs_base == b->cs_base &&
>> >> +   a->flags == b->flags &&
>> >> +   a->page_addr[0] == b->page_addr[0] &&
>> >> +   a->page_addr[1] == b->page_addr[1]) ? 0 : 1;
>> >> +}
>> >> +
>> >
>> > There are a bunch of white space errors that need fixing up as you
>> > probably already know from patchew ;-)
>> >
>> >>  static bool tb_cmp(const void *ap, const void *bp)
>> >>  {
>> >>  const TranslationBlock *a = ap;
>> >> @@ -1586,6 +1598,29 @@ static inline void tb_page_add(PageDesc *p,
>> TranslationBlock *tb,
>> >>  #endif
>> >>  }
>> >>
>> >> +static void tb_insert_statistics_structure(TranslationBlock *tb) {
>> >> +TBStatistics *new_stats = g_new0(TBStatistics, 1);
>> >> +new_stats->pc = tb->pc;
>> >> +new_stats->cs_base = tb->cs_base;
>> >> +new_stats->flags = tb->flags;
>> >> +new_stats->page_addr[0] = tb->page_addr[0];
>> >> +new_stats->page_addr[1] = tb->page_addr[1];
>> >> +
>> >> +GList *lookup_result = g_list_find_custom(tb_ctx.tb_statistics,
>> new_stats, statistics_cmp);
>> >> +
>> >> +if (lookup_result) {
>> >> +/* If there is already a TBStatistic for this TB from a
>> previous flush
>> >> +* then just make the new TB point to the older TBStatistic
>> >> +*/
>> >> +free(new_stats);
>> >> +tb->tb_stats = lookup_result->data;
>> >> +} else {
>> >> +/* If not, then points to the new tb_statistics and add it
>> to the hash */
>> >> +tb->tb_stats = new_stats;
>> >> +tb_ctx.tb_statistics = g_list_prepend(tb_ctx.tb_statistics,
>> >> new_stats);
>> >
>> > This is going to race as nothing prevents two threads attempting to
>> > insert records at the same time.
>> >
>> >> +}
>> >> +}
>> >> +
>> >>  /* add a new TB and link it to the physical page tables. phys_page2 is
>> >>   * (-1) to indicate that only one page contains the TB.
>> >>   *
>> >> @@ -1636,6 +1671,11 @@ tb_link_page(TranslationBlock *tb,
>> tb_page_addr_t phys_pc,
>> >>  void *existing_tb = NULL;
>> >>  uint32_t h;
>> >>
>> >> +if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
>> >> +/* create and link to its TB a structure to store
>> statistics */
>> >> +tb_insert_statistics_structure(tb);
>> >> +}
>> >> +
>> >>  /* add in the hash table */
>> >>  h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags &
>> CF_HASH_MASK,
>> >>   tb->trace_vcpu_dstate);
>> >
>> > Perhaps we could just have a second qht array which allows for
>> > "unlocked" insertion of record. You could take advantage of the fact the
>> > hash would be the same:
>> >
>> > h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags &
>> CF_HASH_MASK,
>> >  tb->trace_vcpu_dstate);
>> > qht_insert(_ctx.htable, tb, h, _tb);
>> >
>> > /* remove TB from the page(s) if we couldn't insert it */
>> > if (unlikely(existing_tb)) {
>> > ...
>> > tb = existing_tb;
>> > } else if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
>> > TBStatistics *new_stats = 

Re: [Qemu-devel] [PATCH v4 0/7] tcg/ppc: Add vector opcodes

[Aleksandar Markovic]

On Jun 25, 2019 5:42 PM, "Mark Cave-Ayland" 
wrote:

>
> The problem is that in tcg/tcg-op.h we define "DEF(dup2_vec, 1, 2, 0,
IMPLVEC |
> IMPL(TCG_TARGET_REG_BITS == 32))" and in the last patchset dup2_vec isn't
introduced
> until towards the end. Unfortunately it's not a simple as bringing the
patch forward
> within the series to maintain bisectability because the current
implementation
> depends on VMRG which only appears in the patch just before it...
>

My strong impression is that VMRG,  VSPLT, VSLDOI, ... opcodes and basic
logic could have been defined very early in the series. (They all just
support other TCG vector operations. Their functionalty just helps achieve
other, exposed, backend functionalities.) That would reduce patch
dependencies and  allow “patch mobility” within the rest of the series.

However, I am not positive at all that would solve the problem at hand.

Aleksandar

Re: [Qemu-devel] [PATCH 3/6] qmp: Add "alias-of" field to query-cpu-definitions

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 05:15:33PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:05AM -0300, Eduardo Habkost wrote:
> > Management software will be expected to resolve CPU model name
> > aliases using the new field.
> > 
> > Signed-off-by: Eduardo Habkost 
> > ---
> > Cc: Eric Blake 
> > Cc: Markus Armbruster 
> > ---
> >  qapi/target.json | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/target.json b/qapi/target.json
> > index 1d4d54b600..0197c7962c 100644
> > --- a/qapi/target.json
> > +++ b/qapi/target.json
> > @@ -475,6 +475,12 @@
> >  #to introspect properties configurable using -cpu or -global.
> >  #(since 2.9)
> >  #
> > +# @alias-of: Name of CPU model this model is an alias for.  The target of 
> > the
> > +#CPU model alias may change depending on the machine type.
> > +#Management software is supposed to translate CPU model aliases
> > +#in the VM configuration, because aliases may stop being
> > +#migration-safe in the future (since 4.1)
> > +#
> >  # @unavailable-features is a list of QOM property names that
> >  # represent CPU model attributes that prevent the CPU from running.
> >  # If the QOM property is read-only, that means there's no known
> > @@ -498,7 +504,8 @@
> >  '*migration-safe': 'bool',
> >  'static': 'bool',
> >  '*unavailable-features': [ 'str' ],
> > -'typename': 'str' },
> > +'typename': 'str',
> > +'*alias-of' : 'str' },
> >'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || 
> > defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> 
> IIUC, this means that data for a "Haswell" CPU model will now report
> "alias-of": "Haswell-NNN"  (for some arbitrary NNN which may change
> at will in any release).

That's correct.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 05:26:03PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> > Base code for versioned CPU models.  This will register a "-4.1"
> > version of all existing CPU models, and make the unversioned CPU
> > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > types.
> > 
> > On older machine types, the unversioned CPU models will keep the
> > old behavior.  This way, management software can use old machine
> > types while resolving aliases if compatibility with older QEMU
> > versions is required.
> > 
> > Using "-machine none", the unversioned CPU models will be aliases
> > to the latest CPU model version.
> > 
> > Includes a test case to ensure that:
> > old machine types won't report any alias to versioned CPU models;
> > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > and "-machine none" will report aliases to some versioned CPU model.
> 
> I'm wondering about the of tieing CPU versions to the release version
> number and whether its a good idea or not ?
> 
> Could there be a reason for us to introduce 2 or more variants
> of a CPU in the same release & would that be a problem if we needed
> todo it ?

I don't see a problem, we can use 3-digit versions that won't be
enabled by any machine type by default.

> 
> Consider if we did not have a Broadwell CPU model defined yet
> and we were adding it at the same time as Spectre came out. We
> might have needed to add "Broadwell-NN" and "Broadwell-MM" one
> with "spec-ctrl" and one without, in order to ensure runability
> on hosts with & without the microcode upgrade.  "Broadwell" alias
> would resolve to either the NN or MM variant according to what
> the current host supported.
> 
> One way to cope with that would have been to add a 3rd digit
> after the version number. eg a Broadwell-4.1.1 and Broadwell-4.1.2

That's exactly what I did for Cascadelake-Server, see patch 6/6.

> 
> An alternative could consider using a plain counter for the CPU
> versions eg Broadwell-1, Broadwell-2, etc ?

This is possible too.  It would require a more complex mapping
between machine types and CPU model versions, though.  Maybe this
is worth the extra complexity because it would make the external
interfaces simpler.

> 
> 
> If we want to backport the newly added CPU model variants to
> exist branches, plain counters don't have the unsightly mismatch.
> eg we'd backport Broadwell-3 to QEMU 3.1, not Broadwell-4.1 to
> QEMU 3.1.  This isn't a functional problem, just something looking
> a bit odd.

I think I'm liking this approach.  If we're untying CPU models
from machine types, let's do it all the way.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 0/7] tcg/ppc: Add vector opcodes

[Mark Cave-Ayland]

On 25/06/2019 16:56, Richard Henderson wrote:

> On 6/25/19 5:37 PM, Mark Cave-Ayland wrote:
>> The problem is that in tcg/tcg-op.h we define "DEF(dup2_vec, 1, 2, 0, 
>> IMPLVEC |
>> IMPL(TCG_TARGET_REG_BITS == 32))" and in the last patchset dup2_vec isn't 
>> introduced
>> until towards the end. Unfortunately it's not a simple as bringing the patch 
>> forward
>> within the series to maintain bisectability because the current 
>> implementation
>> depends on VMRG which only appears in the patch just before it...
> 
> Ah, that would explain it.  I admit I haven't looked at v5 that closely.

It's actually the same in both patchsets: I'm still playing with v4 at the 
moment
since I have a few hacks in place to help me figure out what's going on.

>> Next to try and figure out what exactly is causing the fault. Just a quick 
>> question
>> out of curiosity: is your Power9 system BE or LE?
> 
> The Power9 is LE.
> 
> I do have access to a Power7 BE system, and that worked last time I checked.
> I'll try that again tomorrow to be sure.

And here's where we are blowing up according to -d in_asm,op_out_asm:

IN:
0x00f22ca0:  101ffc84  vor  v0, v31, v31

OP:
 ld_i32 tmp0,env,$0xfff8
 movi_i32 tmp1,$0x0
 brcond_i32 tmp0,tmp1,lt,$L0

  00f22ca0
 ld_vec v128,e8,tmp2,env,$0xd6b0
 st_vec v128,e8,tmp2,env,$0xd4c0
 movi_i32 nip,$0xf22ca4
 movi_i32 nip,$0xf22ca4
 movi_i32 tmp0,$0x10002
 call raise_exception,$0x2,$0,env,tmp0
 exit_tb $0x0
 set_label $L0
 exit_tb $0xa4e7f0c3

OUT: [size=96]
0xa4e7f120:  81dbfff8  lwz  r14, -8(r27)
0xa4e7f124:  2f8e  cmpwicr7, r14, 0
0xa4e7f128:  419c004c  blt  cr7, 0xa4e7f174
0xa4e7f12c:  3c40  lis  r2, 0
0xa4e7f130:  6042d6b0  ori  r2, r2, 0xd6b0
0xa4e7f134:  7c5b10ce  lvx  v2, r27, r2
0xa4e7f138:  3c40  lis  r2, 0
0xa4e7f13c:  6042d4c0  ori  r2, r2, 0xd4c0
0xa4e7f140:  7c5b11ce  stvx v2, r27, r2
0xa4e7f144:  3dc000f2  lis  r14, 0xf2
0xa4e7f148:  61ce2ca4  ori  r14, r14, 0x2ca4
0xa4e7f14c:  91db016c  stw  r14, 0x16c(r27)
0xa4e7f150:  7f63db78  mr   r3, r27
0xa4e7f154:  3c81  lis  r4, 1
0xa4e7f158:  60840002  ori  r4, r4, 2
0xa4e7f15c:  3c87  lis  r0, 0x87
0xa4e7f160:  6000b618  ori  r0, r0, 0xb618
0xa4e7f164:  7c0903a6  mtctrr0
0xa4e7f168:  4e800421  bctrl
0xa4e7f16c:  3860  li   r3, 0
0xa4e7f170:  4bfffef0  b0xa4e7f060
0xa4e7f174:  3c60a4e7  lis  r3, -0x5b19
0xa4e7f178:  6063f0c3  ori  r3, r3, 0xf0c3
0xa4e7f17c:  4bfffee4  b0xa4e7f060

Any ideas what might be going on here?


ATB,

Mark.

Re: [Qemu-devel] [PATCH v2 1/4] add and link a statistic struct to TBs

[Vanderson Martins do Rosario]

When the tb_flush removes a block and it's recreated, this shouldn't
be creating a new block but using the one that is found by:

lookup_result = g_list_find_custom(tb_ctx.tb_statistics, new_stats,
statistics_cmp);

So the tb_statisticics will be reused and we could just add this
regen counter in the if statement: if (lookup_result)

isn't that correct?

Vanderson M. Rosario


On Tue, Jun 25, 2019 at 1:28 PM Alex Bennée  wrote:

>
> Alex Bennée  writes:
>
> > vandersonmr  writes:
> >
> >> We want to store statistics for each TB even after flushes.
> >> We do not want to modify or grow the TB struct.
> >> So we create a new struct to contain this statistics and
> >> link it to each TB while they are created.
> >>
> >> Signed-off-by: Vanderson M. do Rosario 
> >> ---
> >>  accel/tcg/translate-all.c | 40 +++
> >>  include/exec/exec-all.h   | 21 
> >>  include/exec/tb-context.h |  1 +
> >>  include/qemu/log.h|  1 +
> >>  4 files changed, 63 insertions(+)
> >>
> >> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> >> index 5d1e08b169..f7e99f90e0 100644
> >> --- a/accel/tcg/translate-all.c
> >> +++ b/accel/tcg/translate-all.c
> >> @@ -1118,6 +1118,18 @@ static inline void code_gen_alloc(size_t tb_size)
> >>  }
> >>  }
> >>
> >> +static gint statistics_cmp(gconstpointer p1, gconstpointer p2)
> >> +{
> >> +const TBStatistics *a = (TBStatistics *) p1;
> >> +const TBStatistics *b = (TBStatistics *) p2;
> >> +
> >> +return (a->pc == b->pc &&
> >> +   a->cs_base == b->cs_base &&
> >> +   a->flags == b->flags &&
> >> +   a->page_addr[0] == b->page_addr[0] &&
> >> +   a->page_addr[1] == b->page_addr[1]) ? 0 : 1;
> >> +}
> >> +
> >
> > There are a bunch of white space errors that need fixing up as you
> > probably already know from patchew ;-)
> >
> >>  static bool tb_cmp(const void *ap, const void *bp)
> >>  {
> >>  const TranslationBlock *a = ap;
> >> @@ -1586,6 +1598,29 @@ static inline void tb_page_add(PageDesc *p,
> TranslationBlock *tb,
> >>  #endif
> >>  }
> >>
> >> +static void tb_insert_statistics_structure(TranslationBlock *tb) {
> >> +TBStatistics *new_stats = g_new0(TBStatistics, 1);
> >> +new_stats->pc = tb->pc;
> >> +new_stats->cs_base = tb->cs_base;
> >> +new_stats->flags = tb->flags;
> >> +new_stats->page_addr[0] = tb->page_addr[0];
> >> +new_stats->page_addr[1] = tb->page_addr[1];
> >> +
> >> +GList *lookup_result = g_list_find_custom(tb_ctx.tb_statistics,
> new_stats, statistics_cmp);
> >> +
> >> +if (lookup_result) {
> >> +/* If there is already a TBStatistic for this TB from a
> previous flush
> >> +* then just make the new TB point to the older TBStatistic
> >> +*/
> >> +free(new_stats);
> >> +tb->tb_stats = lookup_result->data;
> >> +} else {
> >> +/* If not, then points to the new tb_statistics and add it
> to the hash */
> >> +tb->tb_stats = new_stats;
> >> +tb_ctx.tb_statistics = g_list_prepend(tb_ctx.tb_statistics,
> >> new_stats);
> >
> > This is going to race as nothing prevents two threads attempting to
> > insert records at the same time.
> >
> >> +}
> >> +}
> >> +
> >>  /* add a new TB and link it to the physical page tables. phys_page2 is
> >>   * (-1) to indicate that only one page contains the TB.
> >>   *
> >> @@ -1636,6 +1671,11 @@ tb_link_page(TranslationBlock *tb,
> tb_page_addr_t phys_pc,
> >>  void *existing_tb = NULL;
> >>  uint32_t h;
> >>
> >> +if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
> >> +/* create and link to its TB a structure to store
> statistics */
> >> +tb_insert_statistics_structure(tb);
> >> +}
> >> +
> >>  /* add in the hash table */
> >>  h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags &
> CF_HASH_MASK,
> >>   tb->trace_vcpu_dstate);
> >
> > Perhaps we could just have a second qht array which allows for
> > "unlocked" insertion of record. You could take advantage of the fact the
> > hash would be the same:
> >
> > h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags &
> CF_HASH_MASK,
> >  tb->trace_vcpu_dstate);
> > qht_insert(_ctx.htable, tb, h, _tb);
> >
> > /* remove TB from the page(s) if we couldn't insert it */
> > if (unlikely(existing_tb)) {
> > ...
> > tb = existing_tb;
> > } else if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
> > TBStatistics *new_stats = g_new0(TBStatistics, 1);
> > bool ok;
> > new_stats->pc = tb->pc;
> > new_stats->cs_base = tb->cs_base;
> > new_stats->flags = tb->flags;
> > new_stats->page_addr[0] = tb->page_addr[0];
> > new_stats->page_addr[1] = tb->page_addr[1];
> > ok 

Re: [Qemu-devel] [PATCH v8 03/10] dp8393x: manage big endian bus

[Laurent Vivier]

Le 25/06/2019 à 17:57, Philippe Mathieu-Daudé a écrit :
> On 6/24/19 10:07 PM, Laurent Vivier wrote:
>> Hi,
>>
>> Jason, Can I have an Acked-by from you (as network devices maintainer)?
> 
> Hmm something seems odd here indeed...
> 
> What a stable model! This file has no logical modification since its
> introduction, a65f56eeba "Implement sonic netcard (MIPS Jazz)"
> 
> Here we had:
> 
> static void dp8393x_writeb(void *opaque, hwaddr addr, uint32_t val)
> {
> uint16_t old_val = dp8393x_readw(opaque, addr & ~0x1);
> 
> switch (addr & 3) {
> case 0:
> val = val | (old_val & 0xff00);
> break;
> case 1:
> val = (val << 8) | (old_val & 0x00ff);
> break;
> }
> dp8393x_writew(opaque, addr & ~0x1, val);
> }
> 
> So we had 16-bit endian shifting there.
> 
> And few lines below:
> 
> /* XXX: Check byte ordering */
> ...
> /* Calculate the ethernet checksum */
> #ifdef SONIC_CALCULATE_RXCRC
> checksum = cpu_to_le32(crc32(0, buf, rx_len));
> #else
> checksum = 0;
> #endif
> 
> After various housekeeping, we get:
> 
> 84689cbb97 "net/dp8393x: do not use old_mmio accesses"
> 
> The MIPS Jazz is known to run in both endianess, but I haven't checked
> if at that time both were available.
> 
> Have you tried this patch?
> 
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index bdb0b3b2c2..646e11206f 100644
> @@ -651,7 +651,7 @@ static const MemoryRegionOps dp8393x_ops = {
>  .write = dp8393x_write,
>  .impl.min_access_size = 2,
>  .impl.max_access_size = 2,
> -.endianness = DEVICE_NATIVE_ENDIAN,
> +.endianness = DEVICE_LITTLE_ENDIAN,
>  };
> ---
> 
> (but then mips64-softmmu Jazz would have networking broken).
> 

I doesn't help, the endianness is a MemoryRegion property (see
memory_region_wrong_endianness()) so it is used when the CPU writes to
the device MMIO, not when the device accesses the other memory.
In this case, it reads from system_memory. Perhaps we can create the
address_space with a system_memory in big endian mode?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v1 0/5] RISC-V: Add firmware loading support and default

[Alistair Francis]

On Mon, Jun 24, 2019 at 3:14 PM Alistair Francis
 wrote:
>
> This series consolidates the current RISC-V kernel loading
> impelementation while also adding support for the -bios option and more
> advanced kernel image types.
>
> After consolidating the kernel loading we can extend the boot loader to
> support a -bios option. We can also extend the kernel loading options to
> support not just ELF files but other standard formats.
>
> Finally we can include the OpenSBI firmware for QEMU users.
>
> To avoid breakages we have not changed the default behaviour of QEMU.
> The plan is to change the default though, which is why an entry to the
> qemu-deprecated.texi file has been added as well as a new warning.
>
> After this series QEMU 4.1 has three options:
>  1. ``-bios none`` - This is the current default behavior if no -bios option
>   is included. QEMU will not automatically load any firmware. It is up
>   to the user to load all the images they need.
>  2. ``-bios default`` - In a future QEMU release this will become the default
>   behaviour if no -bios option is specified. This option will load the
>   default OpenSBI firmware automatically. The firmware is included with
>   the QEMU release and no user interaction is required. All a user needs
>   to do is specify the kernel they want to boot with the -kernel option
>  3. ``-bios `` - Tells QEMU to load the specified file as the firmwrae.
>
> All users should transition to using a -bios option. We can start
> updating all documentation after the release of 4.1.
>
> At the end of this series and the transition period we are in the good
> place of no longer requiring users to build firmware to boot a kernel.
> Instead users can just run QEMU with the -kernel option and everything
> will work. They can also override the firmware with their own using
> the -bios option. Using "-bios none" will result in no firmware being
> loaded (as it is today).
>

@Palmer Dabbelt can this go in your 4.1 PR? It has been reviewed and tested.

Alistair

>
> Alistair Francis (5):
>   hw/riscv: Split out the boot functions
>   hw/riscv: Add support for loading a firmware
>   hw/riscv: Extend the kernel loading support
>   roms: Add OpenSBI version 0.3
>   hw/riscv: Load OpenSBI as the default firmware
>
>  .gitmodules  |   3 +
>  Makefile |   5 +-
>  hw/riscv/Makefile.objs   |   1 +
>  hw/riscv/boot.c  | 154 +++
>  hw/riscv/sifive_e.c  |  17 +-
>  hw/riscv/sifive_u.c  |  22 +--
>  hw/riscv/spike.c |  21 +--
>  hw/riscv/virt.c  |  60 ++--
>  include/hw/riscv/boot.h  |  32 
>  pc-bios/opensbi-riscv32-virt-fw_jump.bin | Bin 0 -> 28848 bytes
>  pc-bios/opensbi-riscv64-sifive_u-fw_jump.bin | Bin 0 -> 28904 bytes
>  pc-bios/opensbi-riscv64-virt-fw_jump.bin | Bin 0 -> 28904 bytes
>  qemu-deprecated.texi |  20 +++
>  roms/Makefile|  48 --
>  roms/opensbi |   1 +
>  15 files changed, 278 insertions(+), 106 deletions(-)
>  create mode 100644 hw/riscv/boot.c
>  create mode 100644 include/hw/riscv/boot.h
>  create mode 100644 pc-bios/opensbi-riscv32-virt-fw_jump.bin
>  create mode 100644 pc-bios/opensbi-riscv64-sifive_u-fw_jump.bin
>  create mode 100644 pc-bios/opensbi-riscv64-virt-fw_jump.bin
>  create mode 16 roms/opensbi
>
> --
> 2.22.0
>

Re: [Qemu-devel] [Qemu-block] [RFC] nvme: how to support multiple namespaces

[Klaus Birkelund]

On Mon, Jun 24, 2019 at 12:18:45PM +0200, Kevin Wolf wrote:
> Am 24.06.2019 um 10:01 hat Klaus Birkelund geschrieben:
> > On Thu, Jun 20, 2019 at 05:37:24PM +0200, Laszlo Ersek wrote:
> > > On 06/17/19 10:12, Klaus Birkelund wrote:
> > > > Hi all,
> > > > 
> > > > I'm thinking about how to support multiple namespaces in the NVMe
> > > > device. My first idea was to add a "namespaces" property array to the
> > > > device that references blockdevs, but as Laszlo writes below, this might
> > > > not be the best idea. It also makes it troublesome to add per-namespace
> > > > parameters (which is something I will be required to do for other
> > > > reasons). Some of you might remember my first attempt at this that
> > > > included adding a new block driver (derived from raw) that could be
> > > > given certain parameters that would then be stored in the image. But I
> > > > understand that this is a no-go, and I can see why.
> > > > 
> > > > I guess the optimal way would be such that the parameters was something
> > > > like:
> > > > 
> > > >-blockdev 
> > > > raw,node-name=blk_ns1,file.driver=file,file.filename=blk_ns1.img
> > > >-blockdev 
> > > > raw,node-name=blk_ns2,file.driver=file,file.filename=blk_ns2.img
> > > >-device nvme-ns,drive=blk_ns1,ns-specific-options 
> > > > (nsfeat,mc,dlfeat)...
> > > >-device nvme-ns,drive=blk_ns2,...
> > > >-device nvme,...
> > > > 
> > > > My question is how to state the parent/child relationship between the
> > > > nvme and nvme-ns devices. I've been looking at how ide and virtio does
> > > > this, and maybe a "bus" is the right way to go?
> > > 
> > > I've added Markus to the address list, because of this question. No
> > > other (new) comments from me on the thread starter at this time, just
> > > keeping the full context.
> > > 
> > 
> > Hi all,
> > 
> > I've succesfully implemented this by introducing a new 'nvme-ns' device
> > model. The nvme device creates a bus named from the device id ('id'
> > parameter) and the nvme-ns devices are then registered on this.
> > 
> > This results in an nvme device being creates like this (two namespaces
> > example):
> > 
> >   -drive file=nvme0n1.img,if=none,id=disk1
> >   -drive file=nvme0n2.img,if=none,id=disk2
> >   -device nvme,serial=deadbeef,id=nvme0
> >   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> >   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> > 
> > How does that look as a way forward?
> 
> This looks very similar to what other devices do (one bus controller
> that has multiple devices on its but), so I like it.
> 
> The thing that is special here is that -device nvme is already a block
> device by itself that can take a drive property. So how does this play
> together? Can I choose to either specify a drive directly for the nvme
> device or nvme-ns devices, but when I do both, I will get an error? What
> happens if I don't specify a drive for nvme, but also don't add nvme-ns
> devices?
> 

Hi Kevin,

Yes, the nvme device is already a block device. My current patch removes
that property from the nvme device. I guess this breaks backward
compatibiltiy. We could accept a drive for the nvme device only if no
nvme-ns devices are configured and connected on the bus.

I'm not entirely sure on the spec, but my gut tells me that an nvme
device without any namespaces is technically a valid device, although it
is a bit useless.

I will post my patch (as part of a larger series) and we can discuss it
there.

Thanks for the feedback!

Klaus

Re: [Qemu-devel] [Qemu-block] [RFC] nvme: how to support multiple namespaces

[Klaus Birkelund]

On Tue, Jun 25, 2019 at 07:51:29AM +0200, Markus Armbruster wrote:
> Laszlo Ersek  writes:
> 
> > On 06/24/19 12:18, Kevin Wolf wrote:
> >> Am 24.06.2019 um 10:01 hat Klaus Birkelund geschrieben:
> >>> On Thu, Jun 20, 2019 at 05:37:24PM +0200, Laszlo Ersek wrote:
>  On 06/17/19 10:12, Klaus Birkelund wrote:
> > Hi all,
> >
> > I'm thinking about how to support multiple namespaces in the NVMe
> > device. My first idea was to add a "namespaces" property array to the
> > device that references blockdevs, but as Laszlo writes below, this might
> > not be the best idea. It also makes it troublesome to add per-namespace
> > parameters (which is something I will be required to do for other
> > reasons). Some of you might remember my first attempt at this that
> > included adding a new block driver (derived from raw) that could be
> > given certain parameters that would then be stored in the image. But I
> > understand that this is a no-go, and I can see why.
> >
> > I guess the optimal way would be such that the parameters was something
> > like:
> >
> >-blockdev 
> > raw,node-name=blk_ns1,file.driver=file,file.filename=blk_ns1.img
> >-blockdev 
> > raw,node-name=blk_ns2,file.driver=file,file.filename=blk_ns2.img
> >-device nvme-ns,drive=blk_ns1,ns-specific-options 
> > (nsfeat,mc,dlfeat)...
> >-device nvme-ns,drive=blk_ns2,...
> >-device nvme,...
> >
> > My question is how to state the parent/child relationship between the
> > nvme and nvme-ns devices. I've been looking at how ide and virtio does
> > this, and maybe a "bus" is the right way to go?
> 
>  I've added Markus to the address list, because of this question. No
>  other (new) comments from me on the thread starter at this time, just
>  keeping the full context.
> 
> >>>
> >>> Hi all,
> >>>
> >>> I've succesfully implemented this by introducing a new 'nvme-ns' device
> >>> model. The nvme device creates a bus named from the device id ('id'
> >>> parameter) and the nvme-ns devices are then registered on this.
> >>>
> >>> This results in an nvme device being creates like this (two namespaces
> >>> example):
> >>>
> >>>   -drive file=nvme0n1.img,if=none,id=disk1
> >>>   -drive file=nvme0n2.img,if=none,id=disk2
> >>>   -device nvme,serial=deadbeef,id=nvme0
> >>>   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> >>>   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> >>>
> >>> How does that look as a way forward?
> >> 
> >> This looks very similar to what other devices do (one bus controller
> >> that has multiple devices on its but), so I like it.
> 
> Devices can be wired together without a bus intermediary.  You
> definitely want a bus when the physical connection you model has one.
> If not, a bus may be useful anyway, say because it provides a convenient
> way to encapsulate the connection model, or to support -device bus=...
> 
 
I'm not sure how to wire it together without the bus abstraction? So
I'll stick with the bus for now. It *is* extremely convenient!

Cheers,
Klaus

Re: [Qemu-devel] [PATCH v2] deprecate -mem-path fallback to anonymous RAM

[Daniel P . Berrangé]

On Tue, Jun 25, 2019 at 12:16:29PM -0400, Igor Mammedov wrote:
> Fallback might affect guest or worse whole host performance
> or functionality if backing file were used to share guest RAM
> with another process.
> 
> Patch deprecates fallback so that we could remove it in future
> and ensure that QEMU will provide expected behavior and fail if
> it can't use user provided backing file.
> 
> Signed-off-by: Igor Mammedov 
> ---
> v2:
>  * improve text language
> (Markus Armbruster )
> 
>  numa.c   | 6 --
>  qemu-deprecated.texi | 9 +
>  2 files changed, 13 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [Qemu-devel] [PATCH v4 04/10] block/pflash_cfi02: Implement intereleaved flash devices

[Stephen Checkoway]

> On Jun 25, 2019, at 04:32, Markus Armbruster  wrote:
> 
> Stephen Checkoway  writes:
> 
>>> On Jun 24, 2019, at 12:05, Philippe Mathieu-Daudé  wrote:
>>> 
 On 6/22/19 2:25 PM, Philippe Mathieu-Daudé wrote:
 Hi Stephen,
 
 This series haven't fall through the cracks, however it is taking me
 longer than expected to review it.
 
> On 4/26/19 6:26 PM, Stephen Checkoway wrote:
> It's common for multiple narrow flash chips to be hooked up in parallel
> to support wider buses. For example, four 8-bit wide flash chips (x8)
> may be combined in parallel to produce a 32-bit wide device. Similarly,
> two 16-bit wide chips (x16) may be combined.
> 
> This commit introduces `device-width` and `max-device-width` properties,
> similar to pflash_cfi01, with the following meanings:
> - `width`: The width of the logical, qemu device (same as before);
> - `device-width`: The width of an individual flash chip, defaulting to
> `width`; and
> - `max-device-width`: The maximum width of an individual flash chip,
> defaulting to `device-width`.
> 
> Nothing needs to change to support reading such interleaved devices but
> commands (e.g., erase and programming) must be sent to all devices at
> the same time or else the various chips will be in different states.
 
 After some thoughts on this, I'd rather we model how hardware manage
 interleaved devices: do it at the bus level, and instanciate N devices
 in an interleaved config.
 I believe that would drastically reduce this device complexity, and we
 would match the real internal state machine.
 Also this could be reused by other parallel devices used in a such config.
 
> For example, a 4-byte wide logical device can be composed of four x8/x16
> devices in x8 mode. That is, each device supports both x8 or x16 and
> they're being used in the byte, rather than word, mode. This
> configuration would have `width=4`, `device-width=1`, and
> `max-device-width=2`.
 
 
 I'm thinking of this draft:
 
 FlashDevice # x8
 MemoryRegionOps
   .valid.max_access_size = 1
 
 FlashDevice # x16
 MemoryRegionOps
   .valid.min_access_size = 2
   .valid.max_access_size = 2
 
 FlashDevice # x8/x16
 MemoryRegionOps
   .valid.min_access_size = 1
   .valid.max_access_size = 2
 
 We might use .impl.min_access_size = 2 and consider all NOR flash using
 16-bit words internally.
   .impl.max_access_size = 2 is implicit.
 
 So for you example we'd instanciate one:
 
 InterleaverDevice
 Property
   .bus_width = 4 # 4-byte wide logical device, `width=4`
   .device_width = 1 # `device-width=1`
 MemoryRegionOps
   .valid.max_access_size = .bus_width # 4, set at realize()
   .impl.max_access_size = .device_width # 1, set at realize()
 
 Then instanciate 4 pflash devices, and link them to the interleaver
 using object_property_set_link().
 
 typedef struct {
   SysBusDevice parent_obj;
   MemoryRegion iomem;
   char *name;
   /*
* On a 64-bit wide bus we can have at most
* 8 devices in 8-bit access mode.
*/
   MemoryRegion device[8];
   unsigned device_count;
   unsigned device_index_mask;
   /* Properties */
   unsigned bus_width;
   unsigned device_width;
 } InterleaverDeviceState;
 
 static Property interleaver_properties[] = {
   DEFINE_PROP_LINK("device[0]", InterleaverDeviceState,
device[0],
TYPE_MEMORY_REGION, MemoryRegion *),
   ...
   DEFINE_PROP_LINK("device[7]", InterleaverDeviceState,
device[7],
TYPE_MEMORY_REGION, MemoryRegion *),
   DEFINE_PROP_END_OF_LIST(),
 };
 
 Then previous to call InterleaverDevice.realize():
 
 In the board realize():
 
 
   for (i = 0; i < interleaved_devices; i++) {
   pflash[i] = create_pflash(...);
   ...
   }
 
   ild = ... create InterleaverDevice ...
   for (i = 0; i < interleaved_devices; i++) {
   char *propname = g_strdup_printf("device[%u]", i);
 
 
   object_property_set_link(OBJECT(>device[i]),
OBJECT(pflash[i]),
propname, );
   ...
   }
 
 Finally,
 
 static void interleaved_realize(DeviceState *dev, Error **errp)
> 
> I guess you mean interleaver_realize().
> 
 {
   InterleaverDeviceState *s = INTERLEAVER_DEVICE(opaque);
 
   s->device_count = s->bus_width / s->device_width;
   s->device_index_mask = ~(s->device_count - 1);
   ...
 }
 
 static void interleaved_write(void *opaque, hwaddr offset,
 uint64_t value, unsigned size)
> 
> 

[Qemu-devel] [PATCH v2] deprecate -mem-path fallback to anonymous RAM

[Igor Mammedov]

Fallback might affect guest or worse whole host performance
or functionality if backing file were used to share guest RAM
with another process.

Patch deprecates fallback so that we could remove it in future
and ensure that QEMU will provide expected behavior and fail if
it can't use user provided backing file.

Signed-off-by: Igor Mammedov 
---
v2:
 * improve text language
(Markus Armbruster )

 numa.c   | 6 --
 qemu-deprecated.texi | 9 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/numa.c b/numa.c
index 91a29138a2..c15e53e92d 100644
--- a/numa.c
+++ b/numa.c
@@ -494,8 +494,10 @@ static void allocate_system_memory_nonnuma(MemoryRegion 
*mr, Object *owner,
 if (mem_prealloc) {
 exit(1);
 }
-error_report("falling back to regular RAM allocation.");
-
+warn_report("falling back to regular RAM allocation");
+error_printf("This is deprecated. Make sure that -mem-path "
+ " specified path has sufficient resources to allocate"
+ " -m specified RAM amount or QEMU will fail to 
start");
 /* Legacy behavior: if allocation failed, fall back to
  * regular RAM allocation.
  */
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 2fe9b72121..1b7f3b10dc 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -112,6 +112,15 @@ QEMU using implicit generic or board specific splitting 
rule.
 Use @option{memdev} with @var{memory-backend-ram} backend or @option{mem} (if
 it's supported by used machine type) to define mapping explictly instead.
 
+@subsection -mem-path fallback to RAM (since 4.1)
+Currently if guest RAM allocation from file pointed by @option{mem-path}
+fails, QEMU falls back to allocating from RAM, which might result
+in unpredictable behavior since the backing file specified by the user
+is ignored. In the future, users will be responsible for making sure
+the backing storage specified with @option{-mem-path} can actually provide
+the guest RAM configured with @option{-m} and fail to start up if RAM 
allocation
+is unsuccessful.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
-- 
2.18.1

Re: [Qemu-devel] [PATCH v5 0/2] Guest Support for DIAGNOSE 0x318

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/1561475829-19202-1-git-send-email-wall...@linux.ibm.com/



Hi,

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

Subject: [Qemu-devel] [PATCH v5 0/2] Guest Support for DIAGNOSE 0x318
Type: series
Message-id: 1561475829-19202-1-git-send-email-wall...@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag] 
patchew/1561475829-19202-1-git-send-email-wall...@linux.ibm.com -> 
patchew/1561475829-19202-1-git-send-email-wall...@linux.ibm.com
Switched to a new branch 'test'
2cd6021 s390: diagnose 318 info reset and migration support
f01785b s390/kvm: header sync for diag318

=== OUTPUT BEGIN ===
1/2 Checking commit f01785b90310 (s390/kvm: header sync for diag318)
2/2 Checking commit 2cd6021b6d2a (s390: diagnose 318 info reset and migration 
support)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#37: 
new file mode 100644

ERROR: adding a line without newline at end of file
#165: FILE: hw/s390x/diag318.h:38:
+#endif /* HW_DIAG318_H */

ERROR: line over 90 characters
#285: FILE: target/s390x/cpu_features.c:131:
+FEAT_INIT("diag318", S390_FEAT_TYPE_SCLP_BYTE_134, 0, "Control program 
name and version codes"),

total: 2 errors, 1 warnings, 300 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1561475829-19202-1-git-send-email-wall...@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [Qemu-devel] [PATCH 3/6] qmp: Add "alias-of" field to query-cpu-definitions

[Daniel P . Berrangé]

On Tue, Jun 25, 2019 at 02:00:05AM -0300, Eduardo Habkost wrote:
> Management software will be expected to resolve CPU model name
> aliases using the new field.
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Cc: Eric Blake 
> Cc: Markus Armbruster 
> ---
>  qapi/target.json | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/target.json b/qapi/target.json
> index 1d4d54b600..0197c7962c 100644
> --- a/qapi/target.json
> +++ b/qapi/target.json
> @@ -475,6 +475,12 @@
>  #to introspect properties configurable using -cpu or -global.
>  #(since 2.9)
>  #
> +# @alias-of: Name of CPU model this model is an alias for.  The target of the
> +#CPU model alias may change depending on the machine type.
> +#Management software is supposed to translate CPU model aliases
> +#in the VM configuration, because aliases may stop being
> +#migration-safe in the future (since 4.1)
> +#
>  # @unavailable-features is a list of QOM property names that
>  # represent CPU model attributes that prevent the CPU from running.
>  # If the QOM property is read-only, that means there's no known
> @@ -498,7 +504,8 @@
>  '*migration-safe': 'bool',
>  'static': 'bool',
>  '*unavailable-features': [ 'str' ],
> -'typename': 'str' },
> +'typename': 'str',
> +'*alias-of' : 'str' },
>'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) 
> || defined(TARGET_S390X) || defined(TARGET_MIPS)' }

IIUC, this means that data for a "Haswell" CPU model will now report
"alias-of": "Haswell-NNN"  (for some arbitrary NNN which may change
at will in any release).


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

Re: [Qemu-devel] [PATCH v8 10/10] hw/m68k: define Macintosh Quadra 800

[Thomas Huth]

Am Thu, 20 Jun 2019 00:19:33 +0200
schrieb Laurent Vivier :

> If you want to test the machine, it doesn't yet boot a MacROM, but
> you can boot a linux kernel from the command line.

I gave the patch series a quick try, and was indeed able to boot the
Debian installer with the q800 machine, so:

Tested-by: Thomas Huth 

Re: [Qemu-devel] [PATCH 0/6] x86 CPU model versioning

[Daniel P . Berrangé]

On Tue, Jun 25, 2019 at 02:00:02AM -0300, Eduardo Habkost wrote:
> This series implements basic infrastructure for CPU model
> versioning, as discussed before[1][2][3].  This will finally
> allow us to update CPU models in ways that introduce new software
> or hardware requirements.
> 
> My original plan was to use "query-cpu-model-expansion
> mode=static" to resolve aliases, but I dropped that plan because
> it would increase complexity for management software a lot.
> static CPU models are documented as not being affected by the
> machine type and accelerator at all, which would make the
> versioned CPU models very inconvenient to use in the command
> line.  e.g.: users would be forced to replace:
> 
>   -cpu Haswell
> 
> with:
> 
>   -cpu 
> Haswell-4.1,+2apic,+monitor,+kvmclock,+kvm-nopiodelay,+kvm-asyncpf,+kvm-steal-time,+kvm-pv-eoi,+kvmclock-stable-bit,+x2apic,-acpi,-monitor,-svm
> 
> In the end, making the versioned CPU models static is not a
> requirement at all: what we really need is to drop the
> runnability guarantees from unversioned CPU model names, and
> require management software to resolve the unversioned alias
> before saving the VM configuration.
> 
> Guest ABI compatibility and live migration guarantees are going
> to be kept: unversioned CPU models will still be usable with live
> migration.  Only runnability guarantees when updating the machine
> type will be dropped.  This means unversioned CPU models are
> still reported as migration-safe in query-cpu-definitions.

I'm having a little bit of a hard time parsing the overall behaviour
from the above description. Let me outline the examples so you can
confirm if I've got it right.

Lets assume there is a VM configured using "Haswell"

Today a mgmt app would pass the CPU model name in to QEMU as is,
and thus we get

  $QEMU -machine pc-i440fx-4.0 -cpu Haswell ...more args...

The semantics of "Haswell" here is going to vary according to
what the machine type pc-i440fx-4.0 customizes wrt Haswell.

If the config later has the machine type changed to pc-i440fx-4.1
the semantics of Haswell might change in an arbitrary way. It
might even become unrunnable on the current hardware.

With the new versioned machine types, the VM config of "Haswell"
would be resolved into some arbitrary versioned machine type
"Haswell-NNN" and thus the mgmt app would launch

  $QEMU -machine pc-i440fx-4.0 -cpu Haswell-NNN ...more args...

The semantics of "Haswell-NNN" will never change no matter what
the machine type does.

The end user has the option to explicitly give "Haswell-NNN" to
the mgmt app if they know they want that particular variant, and
in this case the mgmt app won't need to resolve anything (or at
least the process of trying to resolve it won't change it).


> 
> The last patch in the series demonstrates how the new feature can
> be used to update a CPU model: it adds a Cascadelake-Server-4.1.1
> CPU model, including "arch-capabilities=on" and "stepping=5".
> Unfortunately we can't enable arch-capabilities in the -4.1
> version of Cascadelake-Server because it would break our existing
> runnability guarantees.
> 
> [1] https://www.mail-archive.com/libvir-list@redhat.com/msg167342.html
> [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg590034.html
> [3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg611244.html
> 
> ---
> Cc: Paolo Bonzini 
> Cc: Pavel Hrdina 
> Cc: Jiri Denemark 
> Cc: "Hu, Robert" 
> Cc: Tao Xu 
> Cc: Richard Henderson 
> 
> Eduardo Habkost (6):
>   i386: Add x-force-features option for testing
>   i386: Remove unused host_cpudef variable
>   qmp: Add "alias-of" field to query-cpu-definitions
>   i386: Infrastructure for versioned CPU models
>   docs: Deprecate CPU model runnability guarantees
>   i386: Add Cascadelake-Server-4.1.1 CPU model
> 
>  qapi/target.json   |   9 +-
>  include/hw/i386/pc.h   |   3 +
>  target/i386/cpu-qom.h  |  10 +-
>  target/i386/cpu.h  |  16 ++
>  hw/i386/pc.c   |   3 +
>  hw/i386/pc_piix.c  |   4 +
>  hw/i386/pc_q35.c   |   4 +
>  target/i386/cpu.c  | 188 +
>  qemu-deprecated.texi   |  19 +++
>  tests/acceptance/x86_cpu_model_versions.py | 173 +++
>  10 files changed, 388 insertions(+), 41 deletions(-)
>  create mode 100644 tests/acceptance/x86_cpu_model_versions.py
> 
> -- 
> 2.18.0.rc1.1.g3f1ff2140
> 

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

Re: [Qemu-devel] [PATCH v2 1/4] add and link a statistic struct to TBs

[Alex Bennée]

Alex Bennée  writes:

> vandersonmr  writes:
>
>> We want to store statistics for each TB even after flushes.
>> We do not want to modify or grow the TB struct.
>> So we create a new struct to contain this statistics and
>> link it to each TB while they are created.
>>
>> Signed-off-by: Vanderson M. do Rosario 
>> ---
>>  accel/tcg/translate-all.c | 40 +++
>>  include/exec/exec-all.h   | 21 
>>  include/exec/tb-context.h |  1 +
>>  include/qemu/log.h|  1 +
>>  4 files changed, 63 insertions(+)
>>
>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> index 5d1e08b169..f7e99f90e0 100644
>> --- a/accel/tcg/translate-all.c
>> +++ b/accel/tcg/translate-all.c
>> @@ -1118,6 +1118,18 @@ static inline void code_gen_alloc(size_t tb_size)
>>  }
>>  }
>>
>> +static gint statistics_cmp(gconstpointer p1, gconstpointer p2)
>> +{
>> +const TBStatistics *a = (TBStatistics *) p1;
>> +const TBStatistics *b = (TBStatistics *) p2;
>> +
>> +return (a->pc == b->pc &&
>> +   a->cs_base == b->cs_base &&
>> +   a->flags == b->flags &&
>> +   a->page_addr[0] == b->page_addr[0] &&
>> +   a->page_addr[1] == b->page_addr[1]) ? 0 : 1;
>> +}
>> +
>
> There are a bunch of white space errors that need fixing up as you
> probably already know from patchew ;-)
>
>>  static bool tb_cmp(const void *ap, const void *bp)
>>  {
>>  const TranslationBlock *a = ap;
>> @@ -1586,6 +1598,29 @@ static inline void tb_page_add(PageDesc *p, 
>> TranslationBlock *tb,
>>  #endif
>>  }
>>
>> +static void tb_insert_statistics_structure(TranslationBlock *tb) {
>> +TBStatistics *new_stats = g_new0(TBStatistics, 1);
>> +new_stats->pc = tb->pc;
>> +new_stats->cs_base = tb->cs_base;
>> +new_stats->flags = tb->flags;
>> +new_stats->page_addr[0] = tb->page_addr[0];
>> +new_stats->page_addr[1] = tb->page_addr[1];
>> +
>> +GList *lookup_result = g_list_find_custom(tb_ctx.tb_statistics, 
>> new_stats, statistics_cmp);
>> +
>> +if (lookup_result) {
>> +/* If there is already a TBStatistic for this TB from a 
>> previous flush
>> +* then just make the new TB point to the older TBStatistic
>> +*/
>> +free(new_stats);
>> +tb->tb_stats = lookup_result->data;
>> +} else {
>> +/* If not, then points to the new tb_statistics and add it to 
>> the hash */
>> +tb->tb_stats = new_stats;
>> +tb_ctx.tb_statistics = g_list_prepend(tb_ctx.tb_statistics,
>> new_stats);
>
> This is going to race as nothing prevents two threads attempting to
> insert records at the same time.
>
>> +}
>> +}
>> +
>>  /* add a new TB and link it to the physical page tables. phys_page2 is
>>   * (-1) to indicate that only one page contains the TB.
>>   *
>> @@ -1636,6 +1671,11 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t 
>> phys_pc,
>>  void *existing_tb = NULL;
>>  uint32_t h;
>>
>> +if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
>> +/* create and link to its TB a structure to store statistics */
>> +tb_insert_statistics_structure(tb);
>> +}
>> +
>>  /* add in the hash table */
>>  h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & 
>> CF_HASH_MASK,
>>   tb->trace_vcpu_dstate);
>
> Perhaps we could just have a second qht array which allows for
> "unlocked" insertion of record. You could take advantage of the fact the
> hash would be the same:
>
> h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & 
> CF_HASH_MASK,
>  tb->trace_vcpu_dstate);
> qht_insert(_ctx.htable, tb, h, _tb);
>
> /* remove TB from the page(s) if we couldn't insert it */
> if (unlikely(existing_tb)) {
> ...
> tb = existing_tb;
> } else if (qemu_loglevel_mask(CPU_LOG_HOT_TBS)) {
> TBStatistics *new_stats = g_new0(TBStatistics, 1);
> bool ok;
> new_stats->pc = tb->pc;
> new_stats->cs_base = tb->cs_base;
> new_stats->flags = tb->flags;
> new_stats->page_addr[0] = tb->page_addr[0];
> new_stats->page_addr[1] = tb->page_addr[1];
> ok = qht_insert(_ctx.tb_stats, new_stats, h, NULL);
> /*
>  * This should never fail as we succeeded in inserting the
>  * TB itself which means we haven't seen this combination yet.
>  */
> g_assert(ok);
> }
>
> It would also avoid the clunkiness of having to allocate and then
> freeing an unused structure.


Actually thinking on this we still have to handle it. We may have
tb_flushed away a translation and just be regenerating the same block.
As TBStatistics should transcend tb_flush events we need to re-use it's
structure.

It would be worth counting the regens as well so 

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Daniel P . Berrangé]

On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> Base code for versioned CPU models.  This will register a "-4.1"
> version of all existing CPU models, and make the unversioned CPU
> models be an alias for the -4.1 versions on the pc-*-4.1 machine
> types.
> 
> On older machine types, the unversioned CPU models will keep the
> old behavior.  This way, management software can use old machine
> types while resolving aliases if compatibility with older QEMU
> versions is required.
> 
> Using "-machine none", the unversioned CPU models will be aliases
> to the latest CPU model version.
> 
> Includes a test case to ensure that:
> old machine types won't report any alias to versioned CPU models;
> "pc-*-4.1" will return aliases to -4.1 CPU models;
> and "-machine none" will report aliases to some versioned CPU model.

I'm wondering about the of tieing CPU versions to the release version
number and whether its a good idea or not ?

Could there be a reason for us to introduce 2 or more variants
of a CPU in the same release & would that be a problem if we needed
todo it ?

Consider if we did not have a Broadwell CPU model defined yet
and we were adding it at the same time as Spectre came out. We
might have needed to add "Broadwell-NN" and "Broadwell-MM" one
with "spec-ctrl" and one without, in order to ensure runability
on hosts with & without the microcode upgrade.  "Broadwell" alias
would resolve to either the NN or MM variant according to what
the current host supported.

One way to cope with that would have been to add a 3rd digit
after the version number. eg a Broadwell-4.1.1 and Broadwell-4.1.2

An alternative could consider using a plain counter for the CPU
versions eg Broadwell-1, Broadwell-2, etc ?


If we want to backport the newly added CPU model variants to
exist branches, plain counters don't have the unsightly mismatch.
eg we'd backport Broadwell-3 to QEMU 3.1, not Broadwell-4.1 to
QEMU 3.1.  This isn't a functional problem, just something looking
a bit odd.


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

Re: [Qemu-devel] [PATCH v2] block/rbd: add preallocation support

[Max Reitz]

On 06.05.19 14:23, Stefano Garzarella wrote:
> This patch adds the support of preallocation (off/full) for the RBD
> block driver.
> If available, we use rbd_writesame() to quickly fill the image when
> full preallocation is required.
> 
> Signed-off-by: Stefano Garzarella 
> ---
> v2:
> - Use 4 KiB buffer for rbd_writesame() [Jason]
> - Use "rbd_concurrent_management_ops" and "stripe_unit" to limit
>   concurrent ops to the backing cluster [Jason]
> ---
>  block/rbd.c  | 188 +++
>  qapi/block-core.json |   4 +-
>  2 files changed, 175 insertions(+), 17 deletions(-)

This patch conflicts a bit with the rbd file growth patch, so that would
need to be resolved in a v3.

But still, that doesn’t prevent me from reviewing it as it is:

> diff --git a/block/rbd.c b/block/rbd.c
> index 0c549c9935..bc54395e1c 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c

[...]

> @@ -331,6 +333,147 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)

[...]

> +static int qemu_rbd_do_truncate(rados_t cluster, rbd_image_t image,
> +int64_t offset, PreallocMode prealloc,
> +Error **errp)
> +{

[...]

> +#ifdef LIBRBD_SUPPORTS_WRITESAME
> +uint64_t stripe_unit, writesame_max_size;
> +int max_concurrent_ops;
> +
> +max_concurrent_ops = qemu_rbd_get_max_concurrent_ops(cluster);
> +ret = rbd_get_stripe_unit(image, _unit);
> +if (ret < 0) {
> +error_setg_errno(errp, -ret, "Failed to get stripe unit");
> +goto out;
> +}
> +
> +/*
> + * We limit the rbd_writesame() size to avoid to spawn more then

s/then/than/

> + * 'rbd_concurrent_management_ops' concurrent operations.
> + */
> +writesame_max_size = MIN(stripe_unit * max_concurrent_ops, INT_MAX);
> +#endif /* LIBRBD_SUPPORTS_WRITESAME */
> +
> +buf = g_malloc(buf_size);
> +/*
> + * Some versions of rbd_writesame() discards writes of buffers with
> + * all zeroes. In order to avoid this behaviour, we set the first 
> byte
> + * to one.
> + */
> +buf[0] = 1;

But I guess you’ll need to rewrite it as zero later, or the
“bdrv_rbd.bdrv_has_zero_init = bdrv_has_zero_init_1” is no longer quite
true.

[...]

> @@ -449,6 +603,16 @@ static int coroutine_fn qemu_rbd_co_create_opts(const 
> char *filename,
> BLOCK_OPT_CLUSTER_SIZE, 
> 0);
>  rbd_opts->has_cluster_size = (rbd_opts->cluster_size != 0);
>  
> +prealloc = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> +rbd_opts->preallocation = qapi_enum_parse(_lookup, prealloc,
> +  PREALLOC_MODE_OFF, _err);

You also need to set rbd_opts->has_preallocation to true.

> +g_free(prealloc);
> +if (local_err) {
> +ret = -EINVAL;
> +error_propagate(errp, local_err);
> +goto exit;
> +}
> +
>  options = qdict_new();
>  qemu_rbd_parse_filename(filename, options, _err);
>  if (local_err) {

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..db25a4065b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4277,13 +4277,15 @@
>  #   point to a snapshot.
>  # @size Size of the virtual disk in bytes
>  # @cluster-size RBD object size
> +# @preallocationPreallocation mode (allowed values: off, full)

There should be a "(Since: 4.1)" note here.

Max

>  #
>  # Since: 2.12
>  ##
>  { 'struct': 'BlockdevCreateOptionsRbd',
>'data': { 'location': 'BlockdevOptionsRbd',
>  'size': 'size',
> -'*cluster-size' :   'size' } }
> +'*cluster-size' :   'size',
> +'*preallocation':   'PreallocMode' } }
>  
>  ##
>  # @BlockdevVmdkSubformat:
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 03:56:51PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > On Tue, Jun 25, 2019 at 03:32:16PM +0100, Dr. David Alan Gilbert wrote:
> > > * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > > > On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > > > > > Base code for versioned CPU models.  This will register a "-4.1"
> > > > > > version of all existing CPU models, and make the unversioned CPU
> > > > > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > > > > types.
> > > > > > 
> > > > > > On older machine types, the unversioned CPU models will keep the
> > > > > > old behavior.  This way, management software can use old machine
> > > > > > types while resolving aliases if compatibility with older QEMU
> > > > > > versions is required.
> > > > > > 
> > > > > > Using "-machine none", the unversioned CPU models will be aliases
> > > > > > to the latest CPU model version.
> > > > > > 
> > > > > > Includes a test case to ensure that:
> > > > > > old machine types won't report any alias to versioned CPU models;
> > > > > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > > > > and "-machine none" will report aliases to some versioned CPU model.
> > > > > > 
> > > > > > Signed-off-by: Eduardo Habkost 
> > > > > 
> > > > > What happens when we add the next new CPU model?  So say in 4.2 we add
> > > > > a new CPU, does that default to being newcpu-4.2 ?
> > > > 
> > > > We can choose between providing old versions of the CPU model
> > > > retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> > > > only "NewModel-4.2".
> > > > 
> > > > The question is: if we provide only "NewModel-4.2", what should
> > > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> > > 
> > > Perhaps the existing CPUs and the first instance of a new CPU
> > > we should use something non-numeric, e.g. 'orig' rather than 4.1;
> > > we only go numeric when we cause a divergence.
> > 
> > What would be the advantage of a non-numeric version identifier?
> > I believe it would be more confusing to have (e.g.)
> > ["NewModel-orig", "NewModel-4.3"] in QEMU 4.3 instead of
> > ["NewModel-4.2", "NewModel-4.3"].
> 
> To my mind it answers your question:
> > > > The question is: if we provide only "NewModel-4.2", what should
> > > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> 
> NewModel-orig doesn't look weird in pc-i440fx-4.1

It doesn't look weird in the surface, but it doesn't change the
fact that "NewModel-orig" is the version of NewModel that was
shipped in in QEMU 4.2.  Why would we want to hide that fact?

What's the advantage of:

  "-machine pc-i440fx-4.1 -cpu NewModel" will use "NewModel-orig",
  which is the version of NewModel added in QEMU 4.2.

in relation to:

  "-machine pc-i440fx-4.1 -cpu NewModel" will use "NewModel-4.2",
  which is the version of NewModel added in QEMU 4.2.


-- 
Eduardo

Re: [Qemu-devel] [Bug 1833668] Re: linux-user: Unable to run ARM binaries on Aarch64

[Philippe Mathieu-Daudé]

On 6/25/19 5:27 PM, Laurent Vivier wrote:
> Le 25/06/2019 à 16:43, Richard Henderson a écrit :
>> Of course.  There's a separate qemu-arm executable for that.
> 
> On some other architectures (like ppc/ppc64) the idea is the 64bit
> version supports also all 32bit versions CPUs.
> 
> I think it's why this bug has been opened.

At any rate the error message could be more explicit, to avoid confusion.

Re: [Qemu-devel] [PATCH v4 0/7] tcg/ppc: Add vector opcodes

[Richard Henderson]

On 6/25/19 5:37 PM, Mark Cave-Ayland wrote:
> The problem is that in tcg/tcg-op.h we define "DEF(dup2_vec, 1, 2, 0, IMPLVEC 
> |
> IMPL(TCG_TARGET_REG_BITS == 32))" and in the last patchset dup2_vec isn't 
> introduced
> until towards the end. Unfortunately it's not a simple as bringing the patch 
> forward
> within the series to maintain bisectability because the current implementation
> depends on VMRG which only appears in the patch just before it...

Ah, that would explain it.  I admit I haven't looked at v5 that closely.

> Next to try and figure out what exactly is causing the fault. Just a quick 
> question
> out of curiosity: is your Power9 system BE or LE?

The Power9 is LE.

I do have access to a Power7 BE system, and that worked last time I checked.
I'll try that again tomorrow to be sure.


r~

Re: [Qemu-devel] [PATCH v3 0/4] Clean ups in net/net.c

[Stefano Garzarella]

Ping.

Thanks,
Stefano

On Fri, May 17, 2019 at 3:51 PM Stefano Garzarella  wrote:
>
> This series contains some clean ups in net/net.c
>
> The patch 1 solves an assertion failure when ipv6-prefixlen is not a number,
>
> Following the Markus' advice, I modified the parsing of IPv6 prefix
> (patch 2) and IPv4 host:port (patch 3). Then I removed the get_str_sep()
> function (patch 4) because it is no longer used.
>
> v3:
>  - Patch 2:
>- fix indentation [Markus]
>- move substrings at the function level, and call g_strfreev(substrings)
>  at the end of the function [Markus]
>  - add Markus' R-b
>
> v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg615866.html
> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg614561.html
>
> Stefano Garzarella (4):
>   net: fix assertion failure when ipv6-prefixlen is not a number
>   net: avoid using variable length array in net_client_init()
>   net: use g_strsplit() for parsing host address and port
>   net: remove unused get_str_sep() function
>
>  net/net.c | 99 +++
>  1 file changed, 49 insertions(+), 50 deletions(-)
>
> --
> 2.20.1
>
>

Re: [Qemu-devel] [PATCH v8 03/10] dp8393x: manage big endian bus

[Philippe Mathieu-Daudé]

On 6/24/19 10:07 PM, Laurent Vivier wrote:
> Hi,
> 
> Jason, Can I have an Acked-by from you (as network devices maintainer)?

Hmm something seems odd here indeed...

What a stable model! This file has no logical modification since its
introduction, a65f56eeba "Implement sonic netcard (MIPS Jazz)"

Here we had:

static void dp8393x_writeb(void *opaque, hwaddr addr, uint32_t val)
{
uint16_t old_val = dp8393x_readw(opaque, addr & ~0x1);

switch (addr & 3) {
case 0:
val = val | (old_val & 0xff00);
break;
case 1:
val = (val << 8) | (old_val & 0x00ff);
break;
}
dp8393x_writew(opaque, addr & ~0x1, val);
}

So we had 16-bit endian shifting there.

And few lines below:

/* XXX: Check byte ordering */
...
/* Calculate the ethernet checksum */
#ifdef SONIC_CALCULATE_RXCRC
checksum = cpu_to_le32(crc32(0, buf, rx_len));
#else
checksum = 0;
#endif

After various housekeeping, we get:

84689cbb97 "net/dp8393x: do not use old_mmio accesses"

The MIPS Jazz is known to run in both endianess, but I haven't checked
if at that time both were available.

Have you tried this patch?

-- >8 --
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index bdb0b3b2c2..646e11206f 100644
@@ -651,7 +651,7 @@ static const MemoryRegionOps dp8393x_ops = {
 .write = dp8393x_write,
 .impl.min_access_size = 2,
 .impl.max_access_size = 2,
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 };
---

(but then mips64-softmmu Jazz would have networking broken).

Regards,

Phil.

> Le 20/06/2019 à 00:19, Laurent Vivier a écrit :
>> This is needed by Quadra 800, this card can run on little-endian
>> or big-endian bus.
>>
>> Signed-off-by: Laurent Vivier 
>> Tested-by: Hervé Poussineau 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Reviewed-by: Hervé Poussineau 
>> ---
>>  hw/net/dp8393x.c | 88 +++-
>>  1 file changed, 57 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index bdb0b3b2c2..b014c015c6 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -150,6 +150,7 @@ typedef struct dp8393xState {
>>  
>>  /* Hardware */
>>  uint8_t it_shift;
>> +bool big_endian;
>>  qemu_irq irq;
>>  #ifdef DEBUG_SONIC
>>  int irq_level;
>> @@ -220,6 +221,29 @@ static uint32_t dp8393x_wt(dp8393xState *s)
>>  return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
>>  }
>>  
>> +static uint16_t dp8393x_get(dp8393xState *s, int width, uint16_t *base,
>> +int offset)
>> +{
>> +uint16_t val;
>> +
>> +if (s->big_endian) {
>> +val = be16_to_cpu(base[offset * width + width - 1]);
>> +} else {
>> +val = le16_to_cpu(base[offset * width]);
>> +}
>> +return val;
>> +}
>> +
>> +static void dp8393x_put(dp8393xState *s, int width, uint16_t *base, int 
>> offset,
>> +uint16_t val)
>> +{
>> +if (s->big_endian) {
>> +base[offset * width + width - 1] = cpu_to_be16(val);
>> +} else {
>> +base[offset * width] = cpu_to_le16(val);
>> +}
>> +}
>> +
>>  static void dp8393x_update_irq(dp8393xState *s)
>>  {
>>  int level = (s->regs[SONIC_IMR] & s->regs[SONIC_ISR]) ? 1 : 0;
>> @@ -251,12 +275,12 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>>  /* Fill current entry */
>>  address_space_rw(>as, dp8393x_cdp(s),
>>  MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
>> -s->cam[index][0] = data[1 * width] & 0xff;
>> -s->cam[index][1] = data[1 * width] >> 8;
>> -s->cam[index][2] = data[2 * width] & 0xff;
>> -s->cam[index][3] = data[2 * width] >> 8;
>> -s->cam[index][4] = data[3 * width] & 0xff;
>> -s->cam[index][5] = data[3 * width] >> 8;
>> +s->cam[index][0] = dp8393x_get(s, width, data, 1) & 0xff;
>> +s->cam[index][1] = dp8393x_get(s, width, data, 1) >> 8;
>> +s->cam[index][2] = dp8393x_get(s, width, data, 2) & 0xff;
>> +s->cam[index][3] = dp8393x_get(s, width, data, 2) >> 8;
>> +s->cam[index][4] = dp8393x_get(s, width, data, 3) & 0xff;
>> +s->cam[index][5] = dp8393x_get(s, width, data, 3) >> 8;
>>  DPRINTF("load cam[%d] with %02x%02x%02x%02x%02x%02x\n", index,
>>  s->cam[index][0], s->cam[index][1], s->cam[index][2],
>>  s->cam[index][3], s->cam[index][4], s->cam[index][5]);
>> @@ -269,7 +293,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>>  /* Read CAM enable */
>>  address_space_rw(>as, dp8393x_cdp(s),
>>  MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
>> -s->regs[SONIC_CE] = data[0 * width];
>> +s->regs[SONIC_CE] = dp8393x_get(s, width, data, 0);
>>  DPRINTF("load cam done. cam enable mask 0x%04x\n", s->regs[SONIC_CE]);
>>  
>>  /* Done */
>> @@ -290,10 +314,10 @@ static void dp8393x_do_read_rra(dp8393xState *s)
>>  

Re: [Qemu-devel] [PATCH v4 0/7] tcg/ppc: Add vector opcodes

[Mark Cave-Ayland]

On 25/06/2019 07:56, Richard Henderson wrote:

>>> One more hint: if I try a build of d8dcbb57e9 along with my 
>>> tcg_can_emit_vec_op()
>>> hack and pass --enable-debug-tcg to configure then I get an assert on 
>>> startup:
>>>
>>> qemu-system-ppc: /home/mca/src/qemu/tcg/tcg.c:2207: process_op_defs: 
>>> Assertion `tdefs
>>> != ((void *)0)' failed.
>>> Aborted
>>>
>>
>> Mark, Richard, Howard, David,
>>
>> I just sent v5 of the series, that is (in the sense of net-result of
>> code changes) equivalent to v4, but the patch 1/7 from v4 is now split
>> into ten smaller patches. This was done mainly to enable Mark to
>> perhaps try v5 and bisect, in order to at least somewhat narrow down
>> the culprit. Most likely it will be patch 5 from v5, that is still
>> sizeable, but even if this is the case, we can eliminate other smaller
>> things from consideration.
> 
> Thanks for the help on that.
> 
> I don't believe your split is actually bisectable -- there's a minimum amount
> that is required to enable vector opcodes at all.  Patch 5 is the first that
> enables tcg_out_{mov,ld,st}, so while patches beforehand may compile, they
> certainly will not run.
> 
> I can retain your split, but for real bisectability we need to move the enable
> of TCG_TARGET_HAS_v128 from patch 2 to patch 5.
> 
> Given that all this works for me on a Power9 host, I expect that there's a
> simple fix for Mark's G5 host.  Given the above assertion, a missing opcode
> definition, perhaps for -m32 vs -m64?

Right, I'm starting to dig into this a bit more now. First of all, I've figured 
out
what is triggering the above assertion:

"qemu-system-ppc: /home/mca/src/qemu/tcg/tcg.c:2207: process_op_defs: Assertion
`tdefs != ((void *)0)' failed."

The problem is that in tcg/tcg-op.h we define "DEF(dup2_vec, 1, 2, 0, IMPLVEC |
IMPL(TCG_TARGET_REG_BITS == 32))" and in the last patchset dup2_vec isn't 
introduced
until towards the end. Unfortunately it's not a simple as bringing the patch 
forward
within the series to maintain bisectability because the current implementation
depends on VMRG which only appears in the patch just before it...

Next to try and figure out what exactly is causing the fault. Just a quick 
question
out of curiosity: is your Power9 system BE or LE?


ATB,

Mark.

Re: [Qemu-devel] [PATCH] atomic failures on qemu-system-riscv64

[Richard Henderson]

On 6/24/19 8:08 PM, Joel Sing wrote:
> From 8ef31a2ce8ef1cbeee92995a0b2994f480e9bb6d Mon Sep 17 00:00:00 2001
> From: Joel Sing 
> Date: Tue, 25 Jun 2019 02:44:24 +1000
> Subject: [PATCH] Clear load reservations on qemu riscv target
> 
> This prevents a load reservation from being placed in one context/process,
> then being used in another, resulting in an SC succeeding incorrectly and
> breaking atomics.
> 
> Signed-off-by: Joel Sing 

Reviewed-by: Richard Henderson 

> +/* Clear the load reservation - otherwise a reservation placed in one

Excepting this line, which will fail checkpatch.pl.
Needs to be formatted as

/*
 * Clear the load...


r~

Re: [Qemu-devel] [PATCH v3] block/rbd: increase dynamically the image size

[Max Reitz]

On 25.06.19 17:28, Stefano Garzarella wrote:
> On Tue, Jun 25, 2019 at 04:57:53PM +0200, Max Reitz wrote:
>> On 25.06.19 16:47, Stefano Garzarella wrote:
>>> On Tue, Jun 25, 2019 at 04:02:04PM +0200, Max Reitz wrote:
 On 09.05.19 16:59, Stefano Garzarella wrote:
> RBD APIs don't allow us to write more than the size set with
> rbd_create() or rbd_resize().
> In order to support growing images (eg. qcow2), we resize the
> image before write operations that exceed the current size.
>
> Signed-off-by: Stefano Garzarella 
> ---
> v3:
>   - add 'image_size' field in the BDRVRBDState to keep track of the
> current size of the RBD image [Jason, Kevin]
> ---
>  block/rbd.c | 42 +++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 0c549c9935..b0355a2ce0 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c

 [...]

> @@ -833,6 +842,22 @@ static void qemu_rbd_close(BlockDriverState *bs)
>  rados_shutdown(s->cluster);
>  }
>  
> +/* Resize the RBD image and update the 'image_size' with the current 
> size */
> +static int qemu_rbd_resize(BlockDriverState *bs, uint64_t size)
> +{
> +BDRVRBDState *s = bs->opaque;
> +int r;
> +
> +r = rbd_resize(s->image, size);
> +if (r < 0) {
> +return r;
> +}
> +
> +s->image_size = size;

 I think this should update bs->total_sectors, too.  In fact, I’m
 wondering why you don’t just use bs->total_sectors (or bdrv_getlength(),
 which returns bs->total_sectors * 512) instead of adding this new field?

>>>
>>> Hi Max,
>>> thanks for taking a look!
>>>
>>> I used bs->total_sectors in the v2, but Jason pointed out a possible
>>> issue with this, so I proposed to add a variable in the BDRVRBDState to
>>> track the latest resize and Kevin acked [1].
>>>
>>> IIUC what Kevin said on his comment, the 'bs->total_sectors' should be
>>> updated by bdrv_co_write_req_finish(), for this reason I didn't update
>>> it.
>>>
>>> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg615195.html
>>
>> Ah, right!  Yeah, sure, now that I think about it, the block layer must
>> have general code for successful writes beyond EOF...  (Read: Now that
>> I’m pointed towards it...)
> 
> Yeah, do you mean for example to call .bdrv_co_truncate() (or a new
> callback implemented only in the driver that need to be resized like
> rbd) in the bdrv_driver_pwritev() if we will write beyond EOF?

No, I just mean that in theory all drivers should support resizing by
writing beyond the EOF (in practice, it only matters for protocol
drivers).  The general block layer code needs to recognize this and
handle it as if the BDS was explicitly resized with bdrv_co_truncate().

And it does do that, specifically in bdrv_co_write_req_finish(), as you
wrote.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 1/1] hw/pvrdma: Add live migration support

[Yuval Shaia]

On Fri, Jun 21, 2019 at 08:15:41PM +0530, Sukrit Bhatnagar wrote:
> Define and register SaveVMHandlers pvrdma_save and
> pvrdma_load for saving and loading the device state,
> which currently includes only the dma, command slot
> and response slot addresses.
> 
> Remap the DSR, command slot and response slot upon
> loading the addresses in the pvrdma_load function.
> 
> Cc: Marcel Apfelbaum 
> Cc: Yuval Shaia 
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  hw/rdma/vmw/pvrdma_main.c | 56 +++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index adcf79cd63..cd8573173c 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/sysemu.h"
>  #include "monitor/monitor.h"
>  #include "hw/rdma/rdma.h"
> +#include "migration/register.h"
>  
>  #include "../rdma_rm.h"
>  #include "../rdma_backend.h"
> @@ -592,9 +593,62 @@ static void pvrdma_shutdown_notifier(Notifier *n, void 
> *opaque)
>  pvrdma_fini(pci_dev);
>  }
>  
> +static void pvrdma_save(QEMUFile *f, void *opaque)
> +{
> +PVRDMADev *dev = PVRDMA_DEV(opaque);
> +
> +qemu_put_be64(f, dev->dsr_info.dma);
> +qemu_put_be64(f, dev->dsr_info.dsr->cmd_slot_dma);
> +qemu_put_be64(f, dev->dsr_info.dsr->resp_slot_dma);
> +}
> +
> +static int pvrdma_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +PVRDMADev *dev = PVRDMA_DEV(opaque);
> +PCIDevice *pci_dev = PCI_DEVICE(dev);
> +
> +// Remap DSR
> +dev->dsr_info.dma = qemu_get_be64(f);
> +dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
> +sizeof(struct 
> pvrdma_device_shared_region));
> +if (!dev->dsr_info.dsr) {
> +rdma_error_report("Failed to map to DSR");
> +return -1;
> +}
> +qemu_log("pvrdma_load: successfully remapped to DSR\n");
> +
> +// Remap cmd slot
> +dev->dsr_info.dsr->cmd_slot_dma = qemu_get_be64(f);
> +dev->dsr_info.req = rdma_pci_dma_map(pci_dev, 
> dev->dsr_info.dsr->cmd_slot_dma,
> + sizeof(union pvrdma_cmd_req));
> +if (!dev->dsr_info.req) {

So this is where you hit the error, right?
All looks good to me, i wonder why the first pci_dma_map works fine but
second fails where the global BounceBuffer is marked as used.

Anyone?

> +rdma_error_report("Failed to map to command slot address");
> +return -1;
> +}
> +qemu_log("pvrdma_load: successfully remapped to cmd slot\n");
> +
> +// Remap rsp slot

btw, this is RFC so we are fine but try to avoid such way of comments.

> +dev->dsr_info.dsr->resp_slot_dma = qemu_get_be64(f);
> +dev->dsr_info.rsp = rdma_pci_dma_map(pci_dev, 
> dev->dsr_info.dsr->resp_slot_dma,
> + sizeof(union pvrdma_cmd_resp));
> +if (!dev->dsr_info.rsp) {
> +rdma_error_report("Failed to map to response slot address");
> +return -1;
> +}
> +qemu_log("pvrdma_load: successfully remapped to rsp slot\n");
> +
> +return 0;
> +}
> +
> +static SaveVMHandlers savevm_pvrdma = {
> +.save_state = pvrdma_save,
> +.load_state = pvrdma_load,
> +};
> +
>  static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>  {
>  int rc = 0;
> +DeviceState *s = DEVICE(pdev);
>  PVRDMADev *dev = PVRDMA_DEV(pdev);
>  Object *memdev_root;
>  bool ram_shared = false;
> @@ -666,6 +720,8 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>  dev->shutdown_notifier.notify = pvrdma_shutdown_notifier;
>  qemu_register_shutdown_notifier(>shutdown_notifier);
>  
> +register_savevm_live(s, "pvrdma", -1, 1, _pvrdma, dev);
> +
>  out:
>  if (rc) {
>  pvrdma_fini(pdev);
> -- 
> 2.21.0
> 
> 

Re: [Qemu-devel] [RFC 1/1] hw/pvrdma: Add live migration support

[Yuval Shaia]

On Fri, Jun 21, 2019 at 08:15:41PM +0530, Sukrit Bhatnagar wrote:
> Define and register SaveVMHandlers pvrdma_save and
> pvrdma_load for saving and loading the device state,
> which currently includes only the dma, command slot
> and response slot addresses.
> 
> Remap the DSR, command slot and response slot upon
> loading the addresses in the pvrdma_load function.
> 
> Cc: Marcel Apfelbaum 
> Cc: Yuval Shaia 
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  hw/rdma/vmw/pvrdma_main.c | 56 +++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index adcf79cd63..cd8573173c 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/sysemu.h"
>  #include "monitor/monitor.h"
>  #include "hw/rdma/rdma.h"
> +#include "migration/register.h"
>  
>  #include "../rdma_rm.h"
>  #include "../rdma_backend.h"
> @@ -592,9 +593,62 @@ static void pvrdma_shutdown_notifier(Notifier *n, void 
> *opaque)
>  pvrdma_fini(pci_dev);
>  }
>  
> +static void pvrdma_save(QEMUFile *f, void *opaque)
> +{
> +PVRDMADev *dev = PVRDMA_DEV(opaque);
> +
> +qemu_put_be64(f, dev->dsr_info.dma);
> +qemu_put_be64(f, dev->dsr_info.dsr->cmd_slot_dma);
> +qemu_put_be64(f, dev->dsr_info.dsr->resp_slot_dma);
> +}
> +
> +static int pvrdma_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +PVRDMADev *dev = PVRDMA_DEV(opaque);
> +PCIDevice *pci_dev = PCI_DEVICE(dev);
> +
> +// Remap DSR
> +dev->dsr_info.dma = qemu_get_be64(f);
> +dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
> +sizeof(struct 
> pvrdma_device_shared_region));
> +if (!dev->dsr_info.dsr) {
> +rdma_error_report("Failed to map to DSR");
> +return -1;
> +}
> +qemu_log("pvrdma_load: successfully remapped to DSR\n");
> +
> +// Remap cmd slot
> +dev->dsr_info.dsr->cmd_slot_dma = qemu_get_be64(f);
> +dev->dsr_info.req = rdma_pci_dma_map(pci_dev, 
> dev->dsr_info.dsr->cmd_slot_dma,
> + sizeof(union pvrdma_cmd_req));
> +if (!dev->dsr_info.req) {
> +rdma_error_report("Failed to map to command slot address");
> +return -1;
> +}
> +qemu_log("pvrdma_load: successfully remapped to cmd slot\n");
> +
> +// Remap rsp slot
> +dev->dsr_info.dsr->resp_slot_dma = qemu_get_be64(f);
> +dev->dsr_info.rsp = rdma_pci_dma_map(pci_dev, 
> dev->dsr_info.dsr->resp_slot_dma,
> + sizeof(union pvrdma_cmd_resp));
> +if (!dev->dsr_info.rsp) {
> +rdma_error_report("Failed to map to response slot address");
> +return -1;
> +}
> +qemu_log("pvrdma_load: successfully remapped to rsp slot\n");
> +
> +return 0;
> +}
> +
> +static SaveVMHandlers savevm_pvrdma = {
> +.save_state = pvrdma_save,
> +.load_state = pvrdma_load,
> +};
> +
>  static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>  {
>  int rc = 0;
> +DeviceState *s = DEVICE(pdev);
>  PVRDMADev *dev = PVRDMA_DEV(pdev);
>  Object *memdev_root;
>  bool ram_shared = false;
> @@ -666,6 +720,8 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>  dev->shutdown_notifier.notify = pvrdma_shutdown_notifier;
>  qemu_register_shutdown_notifier(>shutdown_notifier);
>  
> +register_savevm_live(s, "pvrdma", -1, 1, _pvrdma, dev);
> +

Don't forget to unregister_savevm on fini()

>  out:
>  if (rc) {
>  pvrdma_fini(pdev);
> -- 
> 2.21.0
> 
> 

Re: [Qemu-devel] [PATCH] atomic failures on qemu-system-riscv64

[Richard Henderson]

On 6/24/19 8:08 PM, Joel Sing wrote:
> Regarding the alignment for reservations, the
> specification does require this, although I do not recall seeing any 
> enforcement
> of this by qemu itself.

Ah, I see it now.  Enforcement begins here:

static bool trans_lr_w(DisasContext *ctx, arg_lr_w *a)
{
REQUIRE_EXT(ctx, RVA);
return gen_lr(ctx, a, (MO_ALIGN | MO_TESL));
   

This will force softmmu (but notably not linux-user; a design limitation) to
generate an alignment fault for an unaligned address.


r~

Re: [Qemu-devel] [PATCH v3] block/rbd: increase dynamically the image size

[Stefano Garzarella]

On Tue, Jun 25, 2019 at 04:57:53PM +0200, Max Reitz wrote:
> On 25.06.19 16:47, Stefano Garzarella wrote:
> > On Tue, Jun 25, 2019 at 04:02:04PM +0200, Max Reitz wrote:
> >> On 09.05.19 16:59, Stefano Garzarella wrote:
> >>> RBD APIs don't allow us to write more than the size set with
> >>> rbd_create() or rbd_resize().
> >>> In order to support growing images (eg. qcow2), we resize the
> >>> image before write operations that exceed the current size.
> >>>
> >>> Signed-off-by: Stefano Garzarella 
> >>> ---
> >>> v3:
> >>>   - add 'image_size' field in the BDRVRBDState to keep track of the
> >>> current size of the RBD image [Jason, Kevin]
> >>> ---
> >>>  block/rbd.c | 42 +++---
> >>>  1 file changed, 39 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/block/rbd.c b/block/rbd.c
> >>> index 0c549c9935..b0355a2ce0 100644
> >>> --- a/block/rbd.c
> >>> +++ b/block/rbd.c
> >>
> >> [...]
> >>
> >>> @@ -833,6 +842,22 @@ static void qemu_rbd_close(BlockDriverState *bs)
> >>>  rados_shutdown(s->cluster);
> >>>  }
> >>>  
> >>> +/* Resize the RBD image and update the 'image_size' with the current 
> >>> size */
> >>> +static int qemu_rbd_resize(BlockDriverState *bs, uint64_t size)
> >>> +{
> >>> +BDRVRBDState *s = bs->opaque;
> >>> +int r;
> >>> +
> >>> +r = rbd_resize(s->image, size);
> >>> +if (r < 0) {
> >>> +return r;
> >>> +}
> >>> +
> >>> +s->image_size = size;
> >>
> >> I think this should update bs->total_sectors, too.  In fact, I’m
> >> wondering why you don’t just use bs->total_sectors (or bdrv_getlength(),
> >> which returns bs->total_sectors * 512) instead of adding this new field?
> >>
> > 
> > Hi Max,
> > thanks for taking a look!
> > 
> > I used bs->total_sectors in the v2, but Jason pointed out a possible
> > issue with this, so I proposed to add a variable in the BDRVRBDState to
> > track the latest resize and Kevin acked [1].
> > 
> > IIUC what Kevin said on his comment, the 'bs->total_sectors' should be
> > updated by bdrv_co_write_req_finish(), for this reason I didn't update
> > it.
> > 
> > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg615195.html
> 
> Ah, right!  Yeah, sure, now that I think about it, the block layer must
> have general code for successful writes beyond EOF...  (Read: Now that
> I’m pointed towards it...)

Yeah, do you mean for example to call .bdrv_co_truncate() (or a new
callback implemented only in the driver that need to be resized like
rbd) in the bdrv_driver_pwritev() if we will write beyond EOF?

> 
> OK then; thanks for the patch, applied to my block branch:

Thanks to take this!
Stefano

Re: [Qemu-devel] [PATCH] RISC-V: Add support for the Zicsr extension

[Alistair Francis]

On Tue, Jun 25, 2019 at 3:09 AM Palmer Dabbelt  wrote:
>
> The various CSR instructions have been split out of the base ISA as part
> of the ratification process.  This patch adds a Zicsr argument, which
> disables all the CSR instructions.
>
> Signed-off-by: Palmer Dabbelt 
> ---
>  target/riscv/cpu.c | 1 +
>  target/riscv/cpu.h | 1 +
>  target/riscv/csr.c | 5 +
>  3 files changed, 7 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index bbad39a337b3..915b9e77df33 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -442,6 +442,7 @@ static Property riscv_cpu_properties[] = {
>  DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
>  DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
>  DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> +DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
>  DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
>  DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
>  DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index ba551cd3082c..0adb307f3298 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -224,6 +224,7 @@ typedef struct RISCVCPU {
>  bool ext_u;
>  bool ext_counters;
>  bool ext_ifencei;
> +bool ext_icsr;
>
>  char *priv_spec;
>  char *user_spec;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index de67741f3648..ff988917b995 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -793,6 +793,7 @@ int riscv_csrrw(CPURISCVState *env, int csrno, 
> target_ulong *ret_value,
>  {
>  int ret;
>  target_ulong old_value;
> +RISCVCPU *cpu = env_archcpu(env);
>
>  /* check privileges and return -1 if check fails */
>  #if !defined(CONFIG_USER_ONLY)
> @@ -803,6 +804,10 @@ int riscv_csrrw(CPURISCVState *env, int csrno, 
> target_ulong *ret_value,
>  }
>  #endif
>
> +/* ensure the CSR extension is enabled. */
> +if (!cpu->cfg.ext_icsr)
> +return -1;

QEMU style include curly braces around a single line if. Plus I think
it makes it less error prone. Can you add braces?

After that:

Reviewed-by: Alistair Francis 

Alistair

> +
>  /* check predicate */
>  if (!csr_ops[csrno].predicate || csr_ops[csrno].predicate(env, csrno) < 
> 0) {
>  return -1;
> --
> 2.21.0
>
>

Re: [Qemu-devel] [PATCH v4 01/13] vfio: KABI for migration interface

[Kirti Wankhede]

On 6/25/2019 12:31 AM, Alex Williamson wrote:
> On Tue, 25 Jun 2019 00:22:16 +0530
> Kirti Wankhede  wrote:
> 
>> On 6/24/2019 8:55 PM, Alex Williamson wrote:
>>> On Mon, 24 Jun 2019 20:30:08 +0530
>>> Kirti Wankhede  wrote:
>>>   
 On 6/22/2019 3:31 AM, Alex Williamson wrote:  
> On Sat, 22 Jun 2019 02:00:08 +0530
> Kirti Wankhede  wrote:
>> On 6/22/2019 1:30 AM, Alex Williamson wrote:
>>> On Sat, 22 Jun 2019 01:05:48 +0530
>>> Kirti Wankhede  wrote:
>>>   
 On 6/21/2019 8:33 PM, Alex Williamson wrote:  
> On Fri, 21 Jun 2019 11:22:15 +0530
> Kirti Wankhede  wrote:
> 
>> On 6/20/2019 10:48 PM, Alex Williamson wrote:
>>> On Thu, 20 Jun 2019 20:07:29 +0530
>>> Kirti Wankhede  wrote:
>>>   
 - Defined MIGRATION region type and sub-type.
 - Used 3 bits to define VFIO device states.
 Bit 0 => _RUNNING
 Bit 1 => _SAVING
 Bit 2 => _RESUMING
 Combination of these bits defines VFIO device's state during 
 migration
 _STOPPED => All bits 0 indicates VFIO device stopped.
 _RUNNING => Normal VFIO device running state.
 _SAVING | _RUNNING => vCPUs are running, VFIO device is 
 running but start
   saving state of device i.e. pre-copy 
 state
 _SAVING  => vCPUs are stoppped, VFIO device should be stopped, 
 and
   save device state,i.e. stop-n-copy state
 _RESUMING => VFIO device resuming state.
 _SAVING | _RESUMING => Invalid state if _SAVING and _RESUMING 
 bits are set
 - Defined vfio_device_migration_info structure which will be 
 placed at 0th
   offset of migration region to get/set VFIO device related 
 information.
   Defined members of structure and usage on read/write access:
 * device_state: (read/write)
 To convey VFIO device state to be transitioned to. Only 3 
 bits are used
 as of now.
 * pending bytes: (read only)
 To get pending bytes yet to be migrated for VFIO device.
 * data_offset: (read only)
 To get data offset in migration from where data exist 
 during _SAVING
 and from where data should be written by user space 
 application during
  _RESUMING state
 * data_size: (read/write)
 To get and set size of data copied in migration region 
 during _SAVING
 and _RESUMING state.
 * start_pfn, page_size, total_pfns: (write only)
 To get bitmap of dirty pages from vendor driver from given
 start address for total_pfns.
 * copied_pfns: (read only)
 To get number of pfns bitmap copied in migration region.
 Vendor driver should copy the bitmap with bits set only for
 pages to be marked dirty in migration region. Vendor driver
 should return 0 if there are 0 pages dirty in requested
 range. Vendor driver should return -1 to mark all pages in 
 the section
 as dirty

 Migration region looks like:
  --
 |vfio_device_migration_info|data section  |
 |  | ///  |
  --
  ^  ^  ^
  offset 0-trapped partdata_offset data_size

 Data section is always followed by vfio_device_migration_info
 structure in the region, so data_offset will always be none-0.
 Offset from where data is copied is decided by kernel driver, data
 section can be trapped or mapped depending on how kernel driver
 defines data section. If mmapped, then data_offset should be page
 aligned, where as initial section which contain
 vfio_device_migration_info structure might not end at offset which
 is page aligned.

 Signed-off-by: Kirti Wankhede 
 Reviewed-by: Neo Jia 
 ---
  linux-headers/linux/vfio.h | 71 
 ++
  1 file changed, 71 insertions(+)

 diff 

Re: [Qemu-devel] [Bug 1833668] Re: linux-user: Unable to run ARM binaries on Aarch64

[Laurent Vivier]

Le 25/06/2019 à 16:43, Richard Henderson a écrit :
> Of course.  There's a separate qemu-arm executable for that.

On some other architectures (like ppc/ppc64) the idea is the 64bit
version supports also all 32bit versions CPUs.

I think it's why this bug has been opened.

[Qemu-devel] [PATCH v5 1/2] s390/kvm: header sync for diag318

[Collin Walling]

Signed-off-by: Collin Walling 
---
 linux-headers/asm-s390/kvm.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 03ab596..4a857bb 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
 #define KVM_S390_VM_CRYPTO 2
 #define KVM_S390_VM_CPU_MODEL  3
 #define KVM_S390_VM_MIGRATION  4
+#define KVM_S390_VM_MISC   5
 
 /* kvm attributes for mem_ctrl */
 #define KVM_S390_VM_MEM_ENABLE_CMMA0
@@ -171,6 +172,9 @@ struct kvm_s390_vm_cpu_subfunc {
 #define KVM_S390_VM_MIGRATION_START1
 #define KVM_S390_VM_MIGRATION_STATUS   2
 
+/* kvm attributes for KVM_S390_VM_MISC */
+#define KVM_S390_VM_MISC_DIAG318   0
+
 /* for KVM_GET_REGS and KVM_SET_REGS */
 struct kvm_regs {
/* general purpose regs for s390 */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v7 0/3] block/stream: get rid of the base

[Andrey Shinkevich]

On 25/06/2019 18:03, Max Reitz wrote:
> On 25.06.19 16:40, Andrey Shinkevich wrote:
>>
>>
>> On 19/06/2019 22:29, Max Reitz wrote:
>>> On 29.05.19 19:56, Andrey Shinkevich wrote:
 This series introduces a bottom intermediate node that eliminates the
 dependency on the base that may change while stream job is running.
 It happens when stream/commit parallel jobs are running on the same
 backing chain. The base node of the stream job may be a top node of
 the parallel commit job and can change before the stream job is
 completed. We avoid that dependency by introducing the bottom node.

 v7: [resend by Andrey]
 01: assert(intermediate) was inserted before the call to
 bdrv_is_allocated() in the intermediate node loop of the
 bdrv_is_allocated_above() as suggested by Max.
 02: The change of the intermediate node loop in the stream_start() was
 rolled back to its original design and the reassignment of the base
 node pointer was added as Vladimir and Max suggested. The relevant
 comment was amended.

 v6: [resend by Vladimir]
 01: improve comment in block/io.c, suggested by Alberto

 v5: [resend by Vladimir]
 01: use comment wording in block/io.c suggested by Alberto

 v4:
 trace_stream_start reverted to the base.
 bdrv_is_allocated_above_inclusive() deleted and the new parameter
 'bool include_base' was added to the bdrv_is_allocated_above().

 Andrey Shinkevich (3):
 block: include base when checking image chain for block allocation
 block/stream: refactor stream_run: drop goto
 block/stream: introduce a bottom node

block/commit.c |  2 +-
block/io.c | 21 +--
block/mirror.c |  2 +-
block/replication.c|  2 +-
block/stream.c | 56 
 --
include/block/block.h  |  3 ++-
tests/qemu-iotests/245 |  4 ++--
7 files changed, 49 insertions(+), 41 deletions(-)
>>>
>>> Reviewed-by: Max Reitz 
>>>
>>> Just needs some simple changes to patch 1 to rebase it on 863cc78f1b3
>>> and c8bb23cbdbe.
>>>
>>> Max
>>>
>>
>> Please let us know who would take this series for pull request?
> 
> Nothing, I was just waiting for my current pull request (lingering from
> last week due to a build failure because of one of the patches) to be
> processed.
> 
> I suppose I won’t wait longer and just take this series now and deal
> with the fallout later if there’s something else in my last week’s pull
> request that needs fixing...
> 
> So: Thanks, applied to my block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 
> Max
> 

Thanks a lot, Max.

Andrey
-- 
With the best regards,
Andrey Shinkevich

[Qemu-devel] [PATCH v5 0/2] Guest Support for DIAGNOSE 0x318

[Collin Walling]

Changelog:

v5
- split off header updates in kvm_s390x.h to a separate patch
- implemented CPU model feature for this insn made available with
zEC12-full and later models
- s/cpc/diag318_info in order to make the relevant data more clear
- reduced S390_MAX_CPUS from 248 to 247

The DIAGNOSE 0x318 instruction is a privileged instruction that is executed by 
the
linux kernel once and only once during setup. This requires interception by KVM 
to 
handle the instruction call safely. The instruction assists with determining 
the 
environment the VM is running in -- this is better described in the KVM patches.

The analogous KVM patches still under review can be found here:
https://marc.info/?l=linux-s390=156147521528818=2

Guest support for the diag 318 instruction is accomplished by implementing a 
device 
class, a cpu model feature, and adjusting the Read Info struct. The Read Info 
struct
adjustment coincidentally reduces the maximum number of VCPUs we can have by 
one.

A device class is used for this instruction in order to streamline the 
migration and 
reset of the DIAG 318 related data.

A CPU model feature is added for this instruction, appropriately named diag318.

The instruction is determined by a Read Info byte 134 bit 0. This is a new byte 
that
expands into the space of the Read Info SCCB that is also used to contain CPU 
entry
data. Due to this expansion, we lose space for one CPU entry and we must reduce 
the
maximum possible CPUs from 248 to 247. Hopefully this drawback does not affect 
many 
VMs.

Collin Walling (2):
  s390/kvm: header sync for diag318
  s390: diagnose 318 info reset and migration support

 hw/s390x/Makefile.objs  |  1 +
 hw/s390x/diag318.c  | 80 +
 hw/s390x/diag318.h  | 38 
 hw/s390x/s390-virtio-ccw.c  | 17 +
 hw/s390x/sclp.c |  3 ++
 include/hw/s390x/sclp.h |  2 ++
 linux-headers/asm-s390/kvm.h|  4 +++
 target/s390x/cpu.h  |  8 -
 target/s390x/cpu_features.c |  3 ++
 target/s390x/cpu_features.h |  1 +
 target/s390x/cpu_features_def.h |  3 ++
 target/s390x/gen-features.c |  1 +
 target/s390x/kvm-stub.c | 10 ++
 target/s390x/kvm.c  | 29 +++
 target/s390x/kvm_s390x.h|  2 ++
 15 files changed, 201 insertions(+), 1 deletion(-)
 create mode 100644 hw/s390x/diag318.c
 create mode 100644 hw/s390x/diag318.h

-- 
2.7.4

Re: [Qemu-devel] [PATCH v7 0/3] block/stream: get rid of the base

[Max Reitz]

On 25.06.19 16:40, Andrey Shinkevich wrote:
> 
> 
> On 19/06/2019 22:29, Max Reitz wrote:
>> On 29.05.19 19:56, Andrey Shinkevich wrote:
>>> This series introduces a bottom intermediate node that eliminates the
>>> dependency on the base that may change while stream job is running.
>>> It happens when stream/commit parallel jobs are running on the same
>>> backing chain. The base node of the stream job may be a top node of
>>> the parallel commit job and can change before the stream job is
>>> completed. We avoid that dependency by introducing the bottom node.
>>>
>>> v7: [resend by Andrey]
>>>01: assert(intermediate) was inserted before the call to
>>>bdrv_is_allocated() in the intermediate node loop of the
>>>bdrv_is_allocated_above() as suggested by Max.
>>>02: The change of the intermediate node loop in the stream_start() was
>>>rolled back to its original design and the reassignment of the base
>>>node pointer was added as Vladimir and Max suggested. The relevant
>>>comment was amended.
>>>
>>> v6: [resend by Vladimir]
>>>01: improve comment in block/io.c, suggested by Alberto
>>>
>>> v5: [resend by Vladimir]
>>>01: use comment wording in block/io.c suggested by Alberto
>>>
>>> v4:
>>> trace_stream_start reverted to the base.
>>> bdrv_is_allocated_above_inclusive() deleted and the new parameter
>>> 'bool include_base' was added to the bdrv_is_allocated_above().
>>>
>>> Andrey Shinkevich (3):
>>>block: include base when checking image chain for block allocation
>>>block/stream: refactor stream_run: drop goto
>>>block/stream: introduce a bottom node
>>>
>>>   block/commit.c |  2 +-
>>>   block/io.c | 21 +--
>>>   block/mirror.c |  2 +-
>>>   block/replication.c|  2 +-
>>>   block/stream.c | 56 
>>> --
>>>   include/block/block.h  |  3 ++-
>>>   tests/qemu-iotests/245 |  4 ++--
>>>   7 files changed, 49 insertions(+), 41 deletions(-)
>>
>> Reviewed-by: Max Reitz 
>>
>> Just needs some simple changes to patch 1 to rebase it on 863cc78f1b3
>> and c8bb23cbdbe.
>>
>> Max
>>
> 
> Please let us know who would take this series for pull request?

Nothing, I was just waiting for my current pull request (lingering from
last week due to a build failure because of one of the patches) to be
processed.

I suppose I won’t wait longer and just take this series now and deal
with the fallout later if there’s something else in my last week’s pull
request that needs fixing...

So: Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v5 2/2] s390: diagnose 318 info reset and migration support

[Collin Walling]

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
be intercepted by SIE and handled via KVM. Let's introduce some
functions to communicate between QEMU and KVM via ioctls. These
will be used to get/set the diag318 information.

The availability of this instruction is determined by byte 134, bit 0
of the Read Info block. This coincidentally expands into the space used
for CPU entries, which means VMs running with the diag318 capability
will have a reduced maximum CPU count. Let's reduce the maximum CPU
count from 248 to 247.

In order to simplify the migration and system reset requirements of
the diag318 data, let's introduce it as a device class and include
a VMStateDescription.

Diag318 is set to 0 during modified clear and load normal resets.

Signed-off-by: Collin Walling 
---
 hw/s390x/Makefile.objs  |  1 +
 hw/s390x/diag318.c  | 80 +
 hw/s390x/diag318.h  | 38 
 hw/s390x/s390-virtio-ccw.c  | 17 +
 hw/s390x/sclp.c |  3 ++
 include/hw/s390x/sclp.h |  2 ++
 target/s390x/cpu.h  |  8 -
 target/s390x/cpu_features.c |  3 ++
 target/s390x/cpu_features.h |  1 +
 target/s390x/cpu_features_def.h |  3 ++
 target/s390x/gen-features.c |  1 +
 target/s390x/kvm-stub.c | 10 ++
 target/s390x/kvm.c  | 29 +++
 target/s390x/kvm_s390x.h|  2 ++
 14 files changed, 197 insertions(+), 1 deletion(-)
 create mode 100644 hw/s390x/diag318.c
 create mode 100644 hw/s390x/diag318.h

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index e02ed80..93621dc 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -34,3 +34,4 @@ obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
 obj-y += s390-ccw.o
 obj-y += ap-device.o
 obj-y += ap-bridge.o
+obj-y += diag318.o
diff --git a/hw/s390x/diag318.c b/hw/s390x/diag318.c
new file mode 100644
index 000..0eb80fe
--- /dev/null
+++ b/hw/s390x/diag318.c
@@ -0,0 +1,80 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019
+ *
+ * Authors:
+ *  Collin Walling 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version. See the COPYING file in the top-level directory.
+ */
+
+#include "hw/s390x/diag318.h"
+#include "qapi/error.h"
+#include "kvm_s390x.h"
+#include "sysemu/kvm.h"
+
+static int diag318_post_load(void *opaque, int version_id)
+{
+DIAG318State *d = opaque;
+
+kvm_s390_set_diag318_info(d->info);
+return 0;
+}
+
+static int diag318_pre_save(void *opaque)
+{
+DIAG318State *d = opaque;
+
+kvm_s390_get_diag318_info(>info);
+return 0;
+}
+
+static bool diag318_needed(void *opaque)
+{
+return s390_has_feat(S390_FEAT_DIAG318);
+}
+
+const VMStateDescription vmstate_diag318 = {
+.name = "vmstate_diag318",
+.post_load = diag318_post_load,
+.pre_save = diag318_pre_save,
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = diag318_needed,
+.fields = (VMStateField[]) {
+VMSTATE_UINT64(info, DIAG318State),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static void s390_diag318_reset(DeviceState *dev)
+{
+kvm_s390_set_diag318_info(0);
+}
+
+static void s390_diag318_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->reset = s390_diag318_reset;
+dc->vmsd = _diag318;
+dc->hotpluggable = false;
+/* Reason: Set automatically during IPL */
+dc->user_creatable = false;
+}
+
+static const TypeInfo s390_diag318_info = {
+.class_init = s390_diag318_class_init,
+.parent = TYPE_DEVICE,
+.name = TYPE_S390_DIAG318,
+.instance_size = sizeof(DIAG318State),
+};
+
+static void s390_diag318_register_types(void)
+{
+type_register_static(_diag318_info);
+}
+
+type_init(s390_diag318_register_types)
diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
new file mode 100644
index 000..d588bdd
--- /dev/null
+++ b/hw/s390x/diag318.h
@@ -0,0 +1,38 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019
+ *
+ * Authors:
+ *  Collin Walling 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version. See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_DIAG318_H
+#define HW_DIAG318_H
+
+#include "qemu/osdep.h"
+#include "hw/qdev.h"
+
+#define TYPE_S390_DIAG318 "diag318"
+#define DIAG318(obj) \
+OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
+
+typedef struct DIAG318State {
+/*< private >*/
+DeviceState parent_obj;
+
+/*< public >*/
+uint64_t info;
+} DIAG318State;
+
+typedef struct DIAG318Class {
+/*< private >*/
+DeviceClass parent_class;
+
+/*< public >*/
+} DIAG318Class;
+
+#endif /* HW_DIAG318_H */
\ No newline at end of file
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 

Re: [Qemu-devel] [PATCH RFC] checkpatch: do not warn for multiline parenthesized returned value

[Richard Henderson]

On 6/21/19 1:28 PM, Paolo Bonzini wrote:
> While indeed we do not want to have
> 
> return (a);
> 
> it is less clear that this applies to
> 
> return (a &&
> b);
> 
> Some editors indent more nicely if you have parentheses, and some people's
> eyes may appreciate that as well.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Thanks, I do this semi-regularly.

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3] block/rbd: increase dynamically the image size

[Max Reitz]

On 25.06.19 16:47, Stefano Garzarella wrote:
> On Tue, Jun 25, 2019 at 04:02:04PM +0200, Max Reitz wrote:
>> On 09.05.19 16:59, Stefano Garzarella wrote:
>>> RBD APIs don't allow us to write more than the size set with
>>> rbd_create() or rbd_resize().
>>> In order to support growing images (eg. qcow2), we resize the
>>> image before write operations that exceed the current size.
>>>
>>> Signed-off-by: Stefano Garzarella 
>>> ---
>>> v3:
>>>   - add 'image_size' field in the BDRVRBDState to keep track of the
>>> current size of the RBD image [Jason, Kevin]
>>> ---
>>>  block/rbd.c | 42 +++---
>>>  1 file changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/rbd.c b/block/rbd.c
>>> index 0c549c9935..b0355a2ce0 100644
>>> --- a/block/rbd.c
>>> +++ b/block/rbd.c
>>
>> [...]
>>
>>> @@ -833,6 +842,22 @@ static void qemu_rbd_close(BlockDriverState *bs)
>>>  rados_shutdown(s->cluster);
>>>  }
>>>  
>>> +/* Resize the RBD image and update the 'image_size' with the current size 
>>> */
>>> +static int qemu_rbd_resize(BlockDriverState *bs, uint64_t size)
>>> +{
>>> +BDRVRBDState *s = bs->opaque;
>>> +int r;
>>> +
>>> +r = rbd_resize(s->image, size);
>>> +if (r < 0) {
>>> +return r;
>>> +}
>>> +
>>> +s->image_size = size;
>>
>> I think this should update bs->total_sectors, too.  In fact, I’m
>> wondering why you don’t just use bs->total_sectors (or bdrv_getlength(),
>> which returns bs->total_sectors * 512) instead of adding this new field?
>>
> 
> Hi Max,
> thanks for taking a look!
> 
> I used bs->total_sectors in the v2, but Jason pointed out a possible
> issue with this, so I proposed to add a variable in the BDRVRBDState to
> track the latest resize and Kevin acked [1].
> 
> IIUC what Kevin said on his comment, the 'bs->total_sectors' should be
> updated by bdrv_co_write_req_finish(), for this reason I didn't update
> it.
> 
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg615195.html

Ah, right!  Yeah, sure, now that I think about it, the block layer must
have general code for successful writes beyond EOF...  (Read: Now that
I’m pointed towards it...)

OK then; thanks for the patch, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 03:32:16PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > > * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > > > Base code for versioned CPU models.  This will register a "-4.1"
> > > > version of all existing CPU models, and make the unversioned CPU
> > > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > > types.
> > > > 
> > > > On older machine types, the unversioned CPU models will keep the
> > > > old behavior.  This way, management software can use old machine
> > > > types while resolving aliases if compatibility with older QEMU
> > > > versions is required.
> > > > 
> > > > Using "-machine none", the unversioned CPU models will be aliases
> > > > to the latest CPU model version.
> > > > 
> > > > Includes a test case to ensure that:
> > > > old machine types won't report any alias to versioned CPU models;
> > > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > > and "-machine none" will report aliases to some versioned CPU model.
> > > > 
> > > > Signed-off-by: Eduardo Habkost 
> > > 
> > > What happens when we add the next new CPU model?  So say in 4.2 we add
> > > a new CPU, does that default to being newcpu-4.2 ?
> > 
> > We can choose between providing old versions of the CPU model
> > retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> > only "NewModel-4.2".
> > 
> > The question is: if we provide only "NewModel-4.2", what should
> > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> 
> Perhaps the existing CPUs and the first instance of a new CPU
> we should use something non-numeric, e.g. 'orig' rather than 4.1;
> we only go numeric when we cause a divergence.

What would be the advantage of a non-numeric version identifier?
I believe it would be more confusing to have (e.g.)
["NewModel-orig", "NewModel-4.3"] in QEMU 4.3 instead of
["NewModel-4.2", "NewModel-4.3"].

However, you have another interesting point: should we introduce
-4.2 versions of all CPU models in QEMU 4.2, or only for the ones
that actually changed?  I think I prefer consistency, even if it
means making the list of CPU models larger.

What do others think?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabk...@redhat.com) wrote:
> On Tue, Jun 25, 2019 at 03:32:16PM +0100, Dr. David Alan Gilbert wrote:
> > * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > > On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > > > * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > > > > Base code for versioned CPU models.  This will register a "-4.1"
> > > > > version of all existing CPU models, and make the unversioned CPU
> > > > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > > > types.
> > > > > 
> > > > > On older machine types, the unversioned CPU models will keep the
> > > > > old behavior.  This way, management software can use old machine
> > > > > types while resolving aliases if compatibility with older QEMU
> > > > > versions is required.
> > > > > 
> > > > > Using "-machine none", the unversioned CPU models will be aliases
> > > > > to the latest CPU model version.
> > > > > 
> > > > > Includes a test case to ensure that:
> > > > > old machine types won't report any alias to versioned CPU models;
> > > > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > > > and "-machine none" will report aliases to some versioned CPU model.
> > > > > 
> > > > > Signed-off-by: Eduardo Habkost 
> > > > 
> > > > What happens when we add the next new CPU model?  So say in 4.2 we add
> > > > a new CPU, does that default to being newcpu-4.2 ?
> > > 
> > > We can choose between providing old versions of the CPU model
> > > retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> > > only "NewModel-4.2".
> > > 
> > > The question is: if we provide only "NewModel-4.2", what should
> > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> > 
> > Perhaps the existing CPUs and the first instance of a new CPU
> > we should use something non-numeric, e.g. 'orig' rather than 4.1;
> > we only go numeric when we cause a divergence.
> 
> What would be the advantage of a non-numeric version identifier?
> I believe it would be more confusing to have (e.g.)
> ["NewModel-orig", "NewModel-4.3"] in QEMU 4.3 instead of
> ["NewModel-4.2", "NewModel-4.3"].

To my mind it answers your question:
> > > The question is: if we provide only "NewModel-4.2", what should
> > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?

NewModel-orig doesn't look weird in pc-i440fx-4.1

Dave


> However, you have another interesting point: should we introduce
> -4.2 versions of all CPU models in QEMU 4.2, or only for the ones
> that actually changed?  I think I prefer consistency, even if it
> means making the list of CPU models larger.
> 
> What do others think?
> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [Bug 1833668] Re: linux-user: Unable to run ARM binaries on Aarch64

[Richard Henderson]

Of course.  There's a separate qemu-arm executable for that.

** Changed in: qemu
   Status: New => Invalid

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

Title:
  linux-user: Unable to run ARM binaries on Aarch64

Status in QEMU:
  Invalid

Bug description:
  Download a ARM package from https://packages.debian.org/sid/busybox-
  static

  Here tested with: busybox-static_1.30.1-4_armel.deb

  $ file busybox.armel
  busybox.armel: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), 
statically linked, for GNU/Linux 3.2.0, 
BuildID[sha1]=12cf572e016bafa240e113b57b3641e94b837f37, stripped

  $ qemu-aarch64 --version
  qemu-aarch64 version 2.11.1(Debian 1:2.11+dfsg-1ubuntu7.14)

  $ qemu-aarch64 busybox.armel
  busybox.armel: Invalid ELF image for this architecture

  $ qemu-aarch64 -cpu cortex-a7 busybox.armel
  unable to find CPU model 'cortex-a7'

  Also reproduced with commit 33d609990621dea6c7d056c86f707b8811320ac1,
  while the aarch64_cpus[] array contains Aarch64 CPUs, the arm_cpus[] array is 
empty:

  $ gdb -q aarch64-linux-user/qemu-aarch64
  (gdb) p aarch64_cpus
  $1 = {{name = 0x1fe4e8 "cortex-a57", initfn = 0x109bc0 , 
class_init = 0x0}, {name = 0x1fe508 "cortex-a53", initfn = 0x109a10 
, class_init = 0x0}, {name = 0x1fe518 "cortex-a72", 
  initfn = 0x109868 , class_init = 0x0}, {name = 
0x218020 "max", initfn = 0x109d70 , class_init = 0x0}, 
{name = 0x0, initfn = 0x0, class_init = 0x0}}
  (gdb) p arm_cpus
  $2 = {{name = 0x0, initfn = 0x0, class_init = 0x0}}

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

Re: [Qemu-devel] [PATCH 1/2] dma/rc4030: Fix off-by-one error in specified memory region size

[Philippe Mathieu-Daudé]

On 6/25/19 4:27 PM, Aleksandar Markovic wrote:
> From: Aleksandar Markovic 
> 
> The size is one byte less than it should be:
> 
> address-space: rc4030-dma
>   -fffe (prio 0, i/o): rc4030.dma
> 
> rc4030 is used in MIPS Jazz board context.

Ah thanks :) I was planing to send this once at home tonight.

> Signed-off-by: Aleksandar Markovic 

Reviewed-by: Philippe Mathieu-Daudé 

  (qemu) info mtree
  ...
  address-space: rc4030-dma
- (prio 0, i/o): rc4030.dma

  address-space: dp8393x
- (prio 0, i/o): rc4030.dma

Tested-by: Philippe Mathieu-Daudé 

> ---
>  hw/dma/rc4030.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
> index 6ccafec..88ff271 100644
> --- a/hw/dma/rc4030.c
> +++ b/hw/dma/rc4030.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "hw/hw.h"
>  #include "hw/mips/mips.h"
>  #include "hw/sysbus.h"
> @@ -678,7 +679,7 @@ static void rc4030_realize(DeviceState *dev, Error **errp)
>  
>  memory_region_init_iommu(>dma_mr, sizeof(s->dma_mr),
>   TYPE_RC4030_IOMMU_MEMORY_REGION,
> - o, "rc4030.dma", UINT32_MAX);
> + o, "rc4030.dma", 4 * GiB);
>  address_space_init(>dma_as, MEMORY_REGION(>dma_mr), "rc4030-dma");
>  }
>  
> 

Re: [Qemu-devel] [PATCH v3] block/rbd: increase dynamically the image size

[Stefano Garzarella]

On Tue, Jun 25, 2019 at 04:02:04PM +0200, Max Reitz wrote:
> On 09.05.19 16:59, Stefano Garzarella wrote:
> > RBD APIs don't allow us to write more than the size set with
> > rbd_create() or rbd_resize().
> > In order to support growing images (eg. qcow2), we resize the
> > image before write operations that exceed the current size.
> > 
> > Signed-off-by: Stefano Garzarella 
> > ---
> > v3:
> >   - add 'image_size' field in the BDRVRBDState to keep track of the
> > current size of the RBD image [Jason, Kevin]
> > ---
> >  block/rbd.c | 42 +++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index 0c549c9935..b0355a2ce0 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> 
> [...]
> 
> > @@ -833,6 +842,22 @@ static void qemu_rbd_close(BlockDriverState *bs)
> >  rados_shutdown(s->cluster);
> >  }
> >  
> > +/* Resize the RBD image and update the 'image_size' with the current size 
> > */
> > +static int qemu_rbd_resize(BlockDriverState *bs, uint64_t size)
> > +{
> > +BDRVRBDState *s = bs->opaque;
> > +int r;
> > +
> > +r = rbd_resize(s->image, size);
> > +if (r < 0) {
> > +return r;
> > +}
> > +
> > +s->image_size = size;
> 
> I think this should update bs->total_sectors, too.  In fact, I’m
> wondering why you don’t just use bs->total_sectors (or bdrv_getlength(),
> which returns bs->total_sectors * 512) instead of adding this new field?
> 

Hi Max,
thanks for taking a look!

I used bs->total_sectors in the v2, but Jason pointed out a possible
issue with this, so I proposed to add a variable in the BDRVRBDState to
track the latest resize and Kevin acked [1].

IIUC what Kevin said on his comment, the 'bs->total_sectors' should be
updated by bdrv_co_write_req_finish(), for this reason I didn't update
it.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg615195.html

Thanks,
Stefano

Re: [Qemu-devel] [PATCH 2/2] dma/rc4030: Minor code style cleanup

[Aleksandar Rikalo]

> From: Aleksandar Markovic 
> Sent: Tuesday, June 25, 2019 4:27 PM
> To: qemu-devel@nongnu.org
> Cc: Aleksandar Markovic; Aleksandar Rikalo; hpous...@reactos.org; 
> f4...@amsat.org
> Subject: [PATCH 2/2] dma/rc4030: Minor code style cleanup
>
> From: Aleksandar Markovic 
>
> Fix some simple checkpatch.pl warnings in rc4030.c.
>
> Signed-off-by: Aleksandar Markovic 
> ---
>  hw/dma/rc4030.c | 18 +++---
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
> index 88ff271..155af9b 100644
> --- a/hw/dma/rc4030.c
> +++ b/hw/dma/rc4030.c
> @@ -58,8 +58,8 @@ typedef struct dma_pagetable_entry {
>
>  #define TYPE_RC4030_IOMMU_MEMORY_REGION "rc4030-iommu-memory-region"
>
> -typedef struct rc4030State
> -{
> +typedef struct rc4030State {
> +
>  SysBusDevice parent;
>
>  uint32_t config; /* 0x: RC4030 config register */
> @@ -152,8 +152,9 @@ static uint64_t rc4030_read(void *opaque, hwaddr addr, 
> unsigned int size)
>  case 0x0058:
>  val = s->cache_bmask;
>  /* HACK */
> -if (s->cache_bmask == (uint32_t)-1)
> +if (s->cache_bmask == (uint32_t)-1) {
>  s->cache_bmask = 0;
> +}
>  break;
>  /* Remote Speed Registers */
>  case 0x0070:
> @@ -538,8 +539,9 @@ static void rc4030_reset(DeviceState *dev)
>
>  s->memory_refresh_rate = 0x18186;
>  s->nvram_protect = 7;
> -for (i = 0; i < 15; i++)
> +for (i = 0; i < 15; i++) {
>  s->rem_speed[i] = 7;
> +}
>  s->imr_jazz = 0x10; /* XXX: required by firmware, but why? */
>  s->isr_jazz = 0;
>
> @@ -551,7 +553,7 @@ static void rc4030_reset(DeviceState *dev)
>
>  static int rc4030_post_load(void *opaque, int version_id)
>  {
> -rc4030State* s = opaque;
> +rc4030State *s = opaque;
>
>  set_next_tick(s);
>  update_jazz_irq(s);
> @@ -591,7 +593,8 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t 
> *buf, int len, int is_wri
>  hwaddr dma_addr;
>  int dev_to_mem;
>
> -s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR 
> | DMA_FLAG_ADDR_INTR);
> +s->dma_regs[n][DMA_REG_ENABLE] &=
> +   ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
>
>  /* Check DMA channel consistency */
>  dev_to_mem = (s->dma_regs[n][DMA_REG_ENABLE] & DMA_FLAG_MEM_TO_DEV) ? 0 
> : 1;
> @@ -603,8 +606,9 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t 
> *buf, int len, int is_wri
>  }
>
>  /* Get start address and len */
> -if (len > s->dma_regs[n][DMA_REG_COUNT])
> +if (len > s->dma_regs[n][DMA_REG_COUNT]) {
>  len = s->dma_regs[n][DMA_REG_COUNT];
> +}
>  dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
>
>  /* Read/write data at right place */
> --
> 2.7.4

Reviewed-by: Aleksandar Rikalo 

Re: [Qemu-devel] [PATCH v7 0/3] block/stream: get rid of the base

[Andrey Shinkevich]

On 19/06/2019 22:29, Max Reitz wrote:
> On 29.05.19 19:56, Andrey Shinkevich wrote:
>> This series introduces a bottom intermediate node that eliminates the
>> dependency on the base that may change while stream job is running.
>> It happens when stream/commit parallel jobs are running on the same
>> backing chain. The base node of the stream job may be a top node of
>> the parallel commit job and can change before the stream job is
>> completed. We avoid that dependency by introducing the bottom node.
>>
>> v7: [resend by Andrey]
>>01: assert(intermediate) was inserted before the call to
>>bdrv_is_allocated() in the intermediate node loop of the
>>bdrv_is_allocated_above() as suggested by Max.
>>02: The change of the intermediate node loop in the stream_start() was
>>rolled back to its original design and the reassignment of the base
>>node pointer was added as Vladimir and Max suggested. The relevant
>>comment was amended.
>>
>> v6: [resend by Vladimir]
>>01: improve comment in block/io.c, suggested by Alberto
>>
>> v5: [resend by Vladimir]
>>01: use comment wording in block/io.c suggested by Alberto
>>
>> v4:
>> trace_stream_start reverted to the base.
>> bdrv_is_allocated_above_inclusive() deleted and the new parameter
>> 'bool include_base' was added to the bdrv_is_allocated_above().
>>
>> Andrey Shinkevich (3):
>>block: include base when checking image chain for block allocation
>>block/stream: refactor stream_run: drop goto
>>block/stream: introduce a bottom node
>>
>>   block/commit.c |  2 +-
>>   block/io.c | 21 +--
>>   block/mirror.c |  2 +-
>>   block/replication.c|  2 +-
>>   block/stream.c | 56 
>> --
>>   include/block/block.h  |  3 ++-
>>   tests/qemu-iotests/245 |  4 ++--
>>   7 files changed, 49 insertions(+), 41 deletions(-)
> 
> Reviewed-by: Max Reitz 
> 
> Just needs some simple changes to patch 1 to rebase it on 863cc78f1b3
> and c8bb23cbdbe.
> 
> Max
> 

Please let us know who would take this series for pull request?

Andrey
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH 2/2] dma/rc4030: Minor code style cleanup

[Philippe Mathieu-Daudé]

On 6/25/19 4:27 PM, Aleksandar Markovic wrote:
> From: Aleksandar Markovic 
> 
> Fix some simple checkpatch.pl warnings in rc4030.c.
> 
> Signed-off-by: Aleksandar Markovic 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/dma/rc4030.c | 18 +++---
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
> index 88ff271..155af9b 100644
> --- a/hw/dma/rc4030.c
> +++ b/hw/dma/rc4030.c
> @@ -58,8 +58,8 @@ typedef struct dma_pagetable_entry {
>  
>  #define TYPE_RC4030_IOMMU_MEMORY_REGION "rc4030-iommu-memory-region"
>  
> -typedef struct rc4030State
> -{
> +typedef struct rc4030State {
> +
>  SysBusDevice parent;
>  
>  uint32_t config; /* 0x: RC4030 config register */
> @@ -152,8 +152,9 @@ static uint64_t rc4030_read(void *opaque, hwaddr addr, 
> unsigned int size)
>  case 0x0058:
>  val = s->cache_bmask;
>  /* HACK */
> -if (s->cache_bmask == (uint32_t)-1)
> +if (s->cache_bmask == (uint32_t)-1) {
>  s->cache_bmask = 0;
> +}
>  break;
>  /* Remote Speed Registers */
>  case 0x0070:
> @@ -538,8 +539,9 @@ static void rc4030_reset(DeviceState *dev)
>  
>  s->memory_refresh_rate = 0x18186;
>  s->nvram_protect = 7;
> -for (i = 0; i < 15; i++)
> +for (i = 0; i < 15; i++) {
>  s->rem_speed[i] = 7;
> +}
>  s->imr_jazz = 0x10; /* XXX: required by firmware, but why? */
>  s->isr_jazz = 0;
>  
> @@ -551,7 +553,7 @@ static void rc4030_reset(DeviceState *dev)
>  
>  static int rc4030_post_load(void *opaque, int version_id)
>  {
> -rc4030State* s = opaque;
> +rc4030State *s = opaque;
>  
>  set_next_tick(s);
>  update_jazz_irq(s);
> @@ -591,7 +593,8 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t 
> *buf, int len, int is_wri
>  hwaddr dma_addr;
>  int dev_to_mem;
>  
> -s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR 
> | DMA_FLAG_ADDR_INTR);
> +s->dma_regs[n][DMA_REG_ENABLE] &=
> +   ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
>  
>  /* Check DMA channel consistency */
>  dev_to_mem = (s->dma_regs[n][DMA_REG_ENABLE] & DMA_FLAG_MEM_TO_DEV) ? 0 
> : 1;
> @@ -603,8 +606,9 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t 
> *buf, int len, int is_wri
>  }
>  
>  /* Get start address and len */
> -if (len > s->dma_regs[n][DMA_REG_COUNT])
> +if (len > s->dma_regs[n][DMA_REG_COUNT]) {
>  len = s->dma_regs[n][DMA_REG_COUNT];
> +}
>  dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
>  
>  /* Read/write data at right place */
> 

Re: [Qemu-devel] [PATCH v1 1/1] tcg/riscv: Fix RISC-VH host build failure

[Richard Henderson]

On 6/20/19 4:04 PM, Alistair Francis wrote:
> Commit 269bd5d8 "cpu: Move the softmmu tlb to CPUNegativeOffsetState'
> broke the RISC-V host build as there are two variables that are used but
> not defined.
> 
> This patch renames the undefined variables mask_off and table_off to the
> existing (but unused) mask_ofs and table_ofs variables.
> 
> Signed-off-by: Alistair Francis 

Oops.  Thanks, queued.


r~

Re: [Qemu-devel] [PATCH v5 0/6] target/mips: Improve MSA TCG tests

[Aleksandar Rikalo]

> From: Aleksandar Markovic 
> Sent: Tuesday, June 25, 2019 2:59 PM
> To: qemu-devel@nongnu.org
> Cc: Aleksandar Markovic; Aleksandar Rikalo
> Subject: [PATCH v5 0/6] target/mips: Improve MSA TCG tests
>
> From: Aleksandar Markovic 
>
> This series contains various improvements and additions of MSA ASE
> TCG tests.
>
> v4->v5:
>
>   - added patch on MIPS32R6 support
>   - amended other patches
>
> v3->v4:
>
>   - patches 1 and 2 from v3 got reviewed and integrated into upstream,
> so they are now removed in v4
>   - added tests for MSA int multiply instructions
>   - added support for MSA big-endian target testings
>   - amend4ed remainign patches
>   - rebased to the latest code
>
> v2->v3:
>
>   - added some tests from move group
>   - added some tests from int dot product group
>   - completed tests from bit move group
>
> v1->v2:
>
>   - added some tests from bit move group
>   - improved and updated commit messages
>
>
>
> Aleksandar Markovic (6):
>   tests/tcg: target/mips: Add tests for MSA bit move instructions
>   tests/tcg: target/mips: Add tests for MSA move instructions
>   tests/tcg: target/mips: Amend tests for MSA int dot product
> instructions
>   tests/tcg: target/mips: Amend tests for MSA int multiply instructions
>   tests/tcg: target/mips: Add support for MSA big-endian target testings
>   tests/tcg: target/mips: Add support for MSA MIPS32R6 testings
>
>  tests/tcg/mips/include/wrappers_msa.h  |  96 +++-
>  .../mips/user/ase/msa/bit-move/test_msa_binsl_b.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_binsl_d.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_binsl_h.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_binsl_w.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_binsr_b.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_binsr_d.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_binsr_h.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_binsr_w.c  | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_bmnz_v.c   | 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_bmz_v.c| 214 +++
>  .../mips/user/ase/msa/bit-move/test_msa_bsel_v.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpadd_s_d.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpadd_s_h.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpadd_s_w.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpadd_u_d.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpadd_u_h.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpadd_u_w.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpsub_s_d.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpsub_s_h.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpsub_s_w.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpsub_u_d.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpsub_u_h.c   | 214 +++
>  .../ase/msa/int-dot-product/test_msa_dpsub_u_w.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_maddv_b.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_maddv_d.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_maddv_h.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_maddv_w.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_msubv_b.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_msubv_d.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_msubv_h.c   | 214 +++
>  .../user/ase/msa/int-multiply/test_msa_msubv_w.c   | 214 +++
>  tests/tcg/mips/user/ase/msa/move/test_msa_move_v.c | 149 +
>  .../mips/user/ase/msa/test_msa_compile_32r6eb.sh   | 627 
> +
>  .../mips/user/ase/msa/test_msa_compile_32r6el.sh   | 627 
> +
>  .../mips/user/ase/msa/test_msa_compile_64r6eb.sh   | 627 
> +
>  ...t_msa_compile.sh => test_msa_compile_64r6el.sh} | 561 ++
>  tests/tcg/mips/user/ase/msa/test_msa_run.sh| 326 ---
>  tests/tcg/mips/user/ase/msa/test_msa_run_32r6eb.sh | 363 
>  tests/tcg/mips/user/ase/msa/test_msa_run_32r6el.sh | 363 
>  tests/tcg/mips/user/ase/msa/test_msa_run_64r6eb.sh | 363 
>  tests/tcg/mips/user/ase/msa/test_msa_run_64r6el.sh | 363 
>  42 files changed, 10523 insertions(+), 576 deletions(-)
>  create mode 100644 tests/tcg/mips/user/ase/msa/bit-move/test_msa_binsl_b.c
>  create mode 100644 tests/tcg/mips/user/ase/msa/bit-move/test_msa_binsl_d.c
>  create mode 100644 tests/tcg/mips/user/ase/msa/bit-move/test_msa_binsl_h.c
>  create mode 100644 tests/tcg/mips/user/ase/msa/bit-move/test_msa_binsl_w.c
>  create mode 100644 tests/tcg/mips/user/ase/msa/bit-move/test_msa_binsr_b.c
>  create mode 100644 tests/tcg/mips/user/ase/msa/bit-move/test_msa_binsr_d.c
>  create mode 100644 tests/tcg/mips/user/ase/msa/bit-move/test_msa_binsr_h.c
>  create mode 100644 

Re: [Qemu-devel] [PATCH 1/2] dma/rc4030: Fix off-by-one error in specified memory region size

[Aleksandar Rikalo]

> From: Aleksandar Markovic 
> Sent: Tuesday, June 25, 2019 4:27 PM
> To: qemu-devel@nongnu.org
> Cc: Aleksandar Markovic; Aleksandar Rikalo; hpous...@reactos.org; 
> f4...@amsat.org
> Subject: [PATCH 1/2] dma/rc4030: Fix off-by-one error in specified memory 
> region size
>
> From: Aleksandar Markovic 
>
> The size is one byte less than it should be:
>
> address-space: rc4030-dma
>   -fffe (prio 0, i/o): rc4030.dma
>
> rc4030 is used in MIPS Jazz board context.
>
> Signed-off-by: Aleksandar Markovic 
> ---
>  hw/dma/rc4030.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
> index 6ccafec..88ff271 100644
> --- a/hw/dma/rc4030.c
> +++ b/hw/dma/rc4030.c
> @@ -23,6 +23,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "hw/hw.h"
>  #include "hw/mips/mips.h"
>  #include "hw/sysbus.h"
> @@ -678,7 +679,7 @@ static void rc4030_realize(DeviceState *dev, Error **errp)
>
>  memory_region_init_iommu(>dma_mr, sizeof(s->dma_mr),
>   TYPE_RC4030_IOMMU_MEMORY_REGION,
> - o, "rc4030.dma", UINT32_MAX);
> + o, "rc4030.dma", 4 * GiB);
>  address_space_init(>dma_as, MEMORY_REGION(>dma_mr), "rc4030-dma");
>  }
>
> --
> 2.7.4

Reviewed-by: Aleksandar Rikalo 

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabk...@redhat.com) wrote:
> On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > > Base code for versioned CPU models.  This will register a "-4.1"
> > > version of all existing CPU models, and make the unversioned CPU
> > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > types.
> > > 
> > > On older machine types, the unversioned CPU models will keep the
> > > old behavior.  This way, management software can use old machine
> > > types while resolving aliases if compatibility with older QEMU
> > > versions is required.
> > > 
> > > Using "-machine none", the unversioned CPU models will be aliases
> > > to the latest CPU model version.
> > > 
> > > Includes a test case to ensure that:
> > > old machine types won't report any alias to versioned CPU models;
> > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > and "-machine none" will report aliases to some versioned CPU model.
> > > 
> > > Signed-off-by: Eduardo Habkost 
> > 
> > What happens when we add the next new CPU model?  So say in 4.2 we add
> > a new CPU, does that default to being newcpu-4.2 ?
> 
> We can choose between providing old versions of the CPU model
> retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> only "NewModel-4.2".
> 
> The question is: if we provide only "NewModel-4.2", what should
> be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?

Perhaps the existing CPUs and the first instance of a new CPU
we should use something non-numeric, e.g. 'orig' rather than 4.1;
we only go numeric when we cause a divergence.

Dave

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

[Qemu-devel] [PATCH 0/2] target/mips: jazz: rc4030: Minor cleanups

[Aleksandar Markovic]

From: Aleksandar Markovic 

This series contains several cleanup items inspired by recent
Philippe's cleanups for Malta devices.

Aleksandar Markovic (2):
  dma/rc4030: Fix off-by-one error in specified memory region size
  dma/rc4030: Minor code style cleanup

 hw/dma/rc4030.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 2/2] dma/rc4030: Minor code style cleanup

[Aleksandar Markovic]

From: Aleksandar Markovic 

Fix some simple checkpatch.pl warnings in rc4030.c.

Signed-off-by: Aleksandar Markovic 
---
 hw/dma/rc4030.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index 88ff271..155af9b 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -58,8 +58,8 @@ typedef struct dma_pagetable_entry {
 
 #define TYPE_RC4030_IOMMU_MEMORY_REGION "rc4030-iommu-memory-region"
 
-typedef struct rc4030State
-{
+typedef struct rc4030State {
+
 SysBusDevice parent;
 
 uint32_t config; /* 0x: RC4030 config register */
@@ -152,8 +152,9 @@ static uint64_t rc4030_read(void *opaque, hwaddr addr, 
unsigned int size)
 case 0x0058:
 val = s->cache_bmask;
 /* HACK */
-if (s->cache_bmask == (uint32_t)-1)
+if (s->cache_bmask == (uint32_t)-1) {
 s->cache_bmask = 0;
+}
 break;
 /* Remote Speed Registers */
 case 0x0070:
@@ -538,8 +539,9 @@ static void rc4030_reset(DeviceState *dev)
 
 s->memory_refresh_rate = 0x18186;
 s->nvram_protect = 7;
-for (i = 0; i < 15; i++)
+for (i = 0; i < 15; i++) {
 s->rem_speed[i] = 7;
+}
 s->imr_jazz = 0x10; /* XXX: required by firmware, but why? */
 s->isr_jazz = 0;
 
@@ -551,7 +553,7 @@ static void rc4030_reset(DeviceState *dev)
 
 static int rc4030_post_load(void *opaque, int version_id)
 {
-rc4030State* s = opaque;
+rc4030State *s = opaque;
 
 set_next_tick(s);
 update_jazz_irq(s);
@@ -591,7 +593,8 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t 
*buf, int len, int is_wri
 hwaddr dma_addr;
 int dev_to_mem;
 
-s->dma_regs[n][DMA_REG_ENABLE] &= ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR | 
DMA_FLAG_ADDR_INTR);
+s->dma_regs[n][DMA_REG_ENABLE] &=
+   ~(DMA_FLAG_TC_INTR | DMA_FLAG_MEM_INTR | DMA_FLAG_ADDR_INTR);
 
 /* Check DMA channel consistency */
 dev_to_mem = (s->dma_regs[n][DMA_REG_ENABLE] & DMA_FLAG_MEM_TO_DEV) ? 0 : 
1;
@@ -603,8 +606,9 @@ static void rc4030_do_dma(void *opaque, int n, uint8_t 
*buf, int len, int is_wri
 }
 
 /* Get start address and len */
-if (len > s->dma_regs[n][DMA_REG_COUNT])
+if (len > s->dma_regs[n][DMA_REG_COUNT]) {
 len = s->dma_regs[n][DMA_REG_COUNT];
+}
 dma_addr = s->dma_regs[n][DMA_REG_ADDRESS];
 
 /* Read/write data at right place */
-- 
2.7.4

[Qemu-devel] [PATCH 1/2] dma/rc4030: Fix off-by-one error in specified memory region size

[Aleksandar Markovic]

From: Aleksandar Markovic 

The size is one byte less than it should be:

address-space: rc4030-dma
  -fffe (prio 0, i/o): rc4030.dma

rc4030 is used in MIPS Jazz board context.

Signed-off-by: Aleksandar Markovic 
---
 hw/dma/rc4030.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index 6ccafec..88ff271 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "hw/hw.h"
 #include "hw/mips/mips.h"
 #include "hw/sysbus.h"
@@ -678,7 +679,7 @@ static void rc4030_realize(DeviceState *dev, Error **errp)
 
 memory_region_init_iommu(>dma_mr, sizeof(s->dma_mr),
  TYPE_RC4030_IOMMU_MEMORY_REGION,
- o, "rc4030.dma", UINT32_MAX);
+ o, "rc4030.dma", 4 * GiB);
 address_space_init(>dma_as, MEMORY_REGION(>dma_mr), "rc4030-dma");
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3] block/rbd: increase dynamically the image size

[Max Reitz]

On 09.05.19 16:59, Stefano Garzarella wrote:
> RBD APIs don't allow us to write more than the size set with
> rbd_create() or rbd_resize().
> In order to support growing images (eg. qcow2), we resize the
> image before write operations that exceed the current size.
> 
> Signed-off-by: Stefano Garzarella 
> ---
> v3:
>   - add 'image_size' field in the BDRVRBDState to keep track of the
> current size of the RBD image [Jason, Kevin]
> ---
>  block/rbd.c | 42 +++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 0c549c9935..b0355a2ce0 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c

[...]

> @@ -833,6 +842,22 @@ static void qemu_rbd_close(BlockDriverState *bs)
>  rados_shutdown(s->cluster);
>  }
>  
> +/* Resize the RBD image and update the 'image_size' with the current size */
> +static int qemu_rbd_resize(BlockDriverState *bs, uint64_t size)
> +{
> +BDRVRBDState *s = bs->opaque;
> +int r;
> +
> +r = rbd_resize(s->image, size);
> +if (r < 0) {
> +return r;
> +}
> +
> +s->image_size = size;

I think this should update bs->total_sectors, too.  In fact, I’m
wondering why you don’t just use bs->total_sectors (or bdrv_getlength(),
which returns bs->total_sectors * 512) instead of adding this new field?

Max

> +
> +return 0;
> +}
> +
>  static const AIOCBInfo rbd_aiocb_info = {
>  .aiocb_size = sizeof(RBDAIOCB),
>  };



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabk...@redhat.com) wrote:
> > Base code for versioned CPU models.  This will register a "-4.1"
> > version of all existing CPU models, and make the unversioned CPU
> > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > types.
> > 
> > On older machine types, the unversioned CPU models will keep the
> > old behavior.  This way, management software can use old machine
> > types while resolving aliases if compatibility with older QEMU
> > versions is required.
> > 
> > Using "-machine none", the unversioned CPU models will be aliases
> > to the latest CPU model version.
> > 
> > Includes a test case to ensure that:
> > old machine types won't report any alias to versioned CPU models;
> > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > and "-machine none" will report aliases to some versioned CPU model.
> > 
> > Signed-off-by: Eduardo Habkost 
> 
> What happens when we add the next new CPU model?  So say in 4.2 we add
> a new CPU, does that default to being newcpu-4.2 ?

We can choose between providing old versions of the CPU model
retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
only "NewModel-4.2".

The question is: if we provide only "NewModel-4.2", what should
be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?

-- 
Eduardo