Re: [Qemu-devel] fw_cfg DMA security
Hi, > One complication I thought of was that it might be tricky to deal with > the implications of allowing this DMA to specify any old address to > fill with fw_cfg data. > > So, for example, since Red Hat is working on SMM. Would a DMA to SMRAM > be protected? > > I haven't watched the fw_cfg DMA discussion too closely, but has this > been thought about? Yes. That problem isn't new and it isn't specific to fw_cfg. You also don't want grant dma access to smram/tseg to your ide/sata/scsi controller or NIC. > One idea I had was that near the end of the firmware boot, the > firmware could trigger fw_cfg in QEMU to stop supporting DMA until a > reset. Should not be needed. We have address spaces in qemu, and the smram/tseg regions are explicitly excluded (when enabled) from dma-able memory. mark: when writing a fw_cfg_dma tests it is a good idea to add a testcase for this, so make sure this works as intended and to avoid security-sensitive regressions. cheers, Gerd
Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
On Fri, Oct 23, 2015 at 12:13:53PM +0800, Cao jin wrote:
> Hi Michael
>
> On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
> >On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
> >>Enable pcie device multifunction hot, just ensure the function 0
> >>added last, then driver will got the notification to scan all the
> >>function in the slot.
> >>
> >>Signed-off-by: Cao jin
> >>---
> >> hw/pci/pci.c | 29 +
> >> hw/pci/pci_host.c| 11 +--
> >> hw/pci/pcie.c| 18 +-
> >> include/hw/pci/pci.h | 1 +
> >> 4 files changed, 48 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>index b0bf540..c068248 100644
> >>--- a/hw/pci/pci.c
> >>+++ b/hw/pci/pci.c
> >>@@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> >>*pci_dev, PCIBus *bus,
> >> PCIConfigWriteFunc *config_write = pc->config_write;
> >> Error *local_err = NULL;
> >> AddressSpace *dma_as;
> >>+DeviceState *dev = DEVICE(pci_dev);
> >>
> >> if (devfn < 0) {
> >> for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >>@@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice
> >>*pci_dev, PCIBus *bus,
> >> PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> >> bus->devices[devfn]->name);
> >> return NULL;
> >>+} else if (dev->hotplugged &&
> >>+ ((!pc->is_express &&
> >>bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
> >>+(pc->is_express && bus->devices[0]))) {
> >
> >So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
>
> ok, will modify it and all the following comment.
>
> >
> >>+error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> >>+ " new func %s cannot be exposed to guest.",
> >>+ PCI_SLOT(devfn),
> >>+ bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
> >>+ name);
> >>+
> >>+ return NULL;
> >> }
> >>
> >> pci_dev->bus = bus;
> >>@@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >> pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >> }
> >>
> >>+/* ARI device function number range is 0-255, means has only 1 function0;
> >>+ * while PCI device has 1 function0 in each slot(up to 32 slot) */
> >>+bool pci_is_function_0(PCIDevice *pci_dev)
> >>+{
> >>+PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> >>+bool is_func0 = false;
> >>+
> >>+if (pc->is_express) {
> >
> >
> >This is not what the spec says. It says:
> >devices connected to an upstream port.
> >
> Yes, I am wrong here. I got confused about the ARI device definition in
> spec:"a device associated with an Upstream Port". Because as I understand,
> ARI device is a EP immediately below a ARI downstream port, just as Figure
> 6-13(Example System Topology with ARI Devices) shows in the spec, but where
> is upstream port in the definition come from, what the relationship between
> upstream port and a ARI device?
See Terms and Acronyms:
The Port on a
component that contains only Endpoint or Bridge Functions is an Upstream
Port.
> >
> >>+if (!pci_dev->devfn)
> >>+is_func0 = true;
> >
> >Just do return !pci_dev->devfn here.
> >
> >>+} else {
> >>+if(!PCI_FUNC(pci_dev->devfn))
> >>+is_func0 = true;
> >>+}
> >>+
> >>+return is_func0;
> >>+}
> >>+
> >> static const TypeInfo pci_device_type_info = {
> >> .name = TYPE_PCI_DEVICE,
> >> .parent = TYPE_DEVICE,
> >>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> >>index 3e26f92..40087c9 100644
> >>--- a/hw/pci/pci_host.c
> >>+++ b/hw/pci/pci_host.c
> >>@@ -20,6 +20,7 @@
> >>
> >> #include "hw/pci/pci.h"
> >> #include "hw/pci/pci_host.h"
> >>+#include "hw/pci/pci_bus.h"
> >> #include "trace.h"
> >>
> >> /* debug PCI */
> >>@@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t
> >>val, int len)
> >> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >>
> >>-if (!pci_dev) {
> >>+/* non-zero functions are only exposed when function 0 is present,
> >>+ * allowing direct removal of unexposed functions.
> >>+ */
> >>+if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >
> >Reuse pci_get_function_0 here too?
> >
> >> return;
> >> }
> >>
> >>@@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >> uint32_t val;
> >>
> >>-if (!pci_dev) {
> >>+/* non-zero functions are only exposed when function 0 is present,
> >>+ * allowing direct removal of unexposed functions.
> >>+ */
> >>+if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >
> >
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
Eduardo Habkost writes: > On Wed, Oct 21, 2015 at 12:54:23PM +0200, Markus Armbruster wrote: >> Valerio Aimale writes: > [...] >> > There's also a similar patch, floating around the internet, the uses >> > shared memory, instead of sockets, as inter-process communication >> > between libvmi and QEMU. I've never used that. >> >> By the time you built a working IPC mechanism on top of shared memory, >> you're often no better off than with AF_LOCAL sockets. >> >> Crazy idea: can we allocate guest memory in a way that support sharing >> it with another process? Eduardo, can -mem-path do such wild things? > > It can't today, but just because it creates a temporary file inside > mem-path and unlinks it immediately after opening a file descriptor. We > could make memory-backend-file also accept a full filename as argument, > or add a mechanism to let QEMU send the open file descriptor to a QMP > client. Valerio, would an command line option to share guest memory suffice, or does it have to be a monitor command? If the latter, why? Eduardo, I'm not sure writing to guest memory behind TCG's back will work. Do you know? Will it work with KVM?
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
Valerio Aimale writes: > On 10/22/15 5:50 AM, Markus Armbruster wrote: >> Valerio Aimale writes: >> >>> On 10/21/15 4:54 AM, Markus Armbruster wrote: [...] Can you give an example? >>> Yes. I was trying to dump the full extent of physical memory of a VM >>> that has 8GB memory space (ballooned). I simply did this: >>> >>> $ telnet localhost 1234 >>> Trying 127.0.0.1... >>> Connected to localhost. >>> Escape character is '^]'. >>> QEMU 2.4.0.1 monitor - type 'help' for more information >>> (qemu) pmemsave 0 8589934591 "/tmp/memsaved" >>> 'pmemsave' has failed: integer is for 32-bit values >>> >>> Maybe I misunderstood how pmemsave works. Maybe I should have used >>> dump-guest-memory >> This is am unnecessary limitation caused by 'size:i' instead of >> 'size:o'. Fixable. > I think I tried changing size:i to size:l, but, I was still receiving > the error. I should have a look. HMP is relatively low priority for me; feel free to remind me in a week or two.
[Qemu-devel] fw_cfg DMA security
Back when I was looking at fw_cfg support for -kernel in OVMF, I noted that it took a while to read the kernel. We improved the perf substantially by using a 'rep insb' instruction, which I think kvm special cases to minimize VM traps. Nevertheless, I thought that it would be good to implement a DMA interface to fw_cfg. It's great to see that Marc made that happen. One complication I thought of was that it might be tricky to deal with the implications of allowing this DMA to specify any old address to fill with fw_cfg data. So, for example, since Red Hat is working on SMM. Would a DMA to SMRAM be protected? I haven't watched the fw_cfg DMA discussion too closely, but has this been thought about? One idea I had was that near the end of the firmware boot, the firmware could trigger fw_cfg in QEMU to stop supporting DMA until a reset. -Jordan
Re: [Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members
Eric Blake writes: > On 10/21/2015 07:34 AM, Markus Armbruster wrote: > >> >> The least verbose naming convention for a conversion function I can >> think of right now is TBase(), where T is the name of a type with a >> base. Compare: >> >> foo((Parent *)child, blah) >> foo(ChildBase(child), blah) >> >> Tolerable? Worthwhile? > > 'TBase' won't work. We already have BlockdevOptionsBase as a subtype of > BlockdevOptions, and using 'TBase' would give us > 'BlockdevOptionsBase(options)' which is now ambiguous between the type > name and intended function call. I'll probably go with > qapi_TYPE_base(), and see how long that looks. I'd have no problem with renaming things that don't impact wire ABI. A quick grep suggests churn would be minimal.
Re: [Qemu-devel] [PATCH v9 15/17] tpm: Convert to new qapi union layout
Eric Blake writes: > On 10/22/2015 08:26 AM, Eric Blake wrote: > >>> PATCH 08-15 appear to be a purely mechanical switch to u. and from kind >>> to type, except for a qapi.py hunk that looks like it should be in PATCH >>> 07, and a comment update to tests/qapi-schema/union-clash-type.json. >>> Did I miss anything? >>> >>> Combined diffstat isn't so bad: >>> >>> 36 files changed, 393 insertions(+), 394 deletions(-) >> >> It already needs a rebase; some of Dan's work has caused more changes to >> ui/vnc.c and util/qemu-sockets.c. So hopefully I post v10 soon. >> >>> >>> I've seen worse tree-wide changes, some of them my own. I'd be tempted >>> to squash the complete switch together. But squashing is easy, so we >>> can keep it separate while we review, and decide when we're done. >> >> Sure, v10 will keep things separate, but squashing won't hurt too much. >> After all, v5 had it all as one patch. > > Just so I'm clear, if we wanted to squash, would it be just 8-15 (just > the mechanical changes, but keeping the front-end scaffolding hack and > backend cleanup, and keeping non-mechanical changes split off of 7 and 8 > as a separate patch), or the entire 7-16 (no hack at all, and nothing to > split off of 7 and 8)? Either way could work.
Re: [Qemu-devel] [PULL] vhost-user: fix up rhel6 build
Hello,
On Thu, Oct 22, 2015 at 9:37 PM, Michael S. Tsirkin wrote:
> Build on RHEL6 fails:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42875
>
> Apparently unnamed unions couldn't use C99 named field initializers.
> Let's just name the payload union field.
This fixes the issue I previously reported.
> Signed-off-by: Michael S. Tsirkin
Tested-by: Laurent Desnogues
Thanks,
Laurent
> ---
> hw/virtio/vhost-user.c | 48
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 78442ba..0aa8e0d 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -89,7 +89,7 @@ typedef struct VhostUserMsg {
> struct vhost_vring_state state;
> struct vhost_vring_addr addr;
> VhostUserMemory memory;
> -};
> +} payload;
> } QEMU_PACKED VhostUserMsg;
>
> static VhostUserMsg m __attribute__ ((unused));
> @@ -200,8 +200,8 @@ static int vhost_user_set_log_base(struct vhost_dev *dev,
> uint64_t base,
> VhostUserMsg msg = {
> .request = VHOST_USER_SET_LOG_BASE,
> .flags = VHOST_USER_VERSION,
> -.u64 = base,
> -.size = sizeof(m.u64),
> +.payload.u64 = base,
> +.size = sizeof(m.payload.u64),
> };
>
> if (shmfd && log->fd != -1) {
> @@ -247,17 +247,17 @@ static int vhost_user_set_mem_table(struct vhost_dev
> *dev,
> &ram_addr);
> fd = qemu_get_ram_fd(ram_addr);
> if (fd > 0) {
> -msg.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
> -msg.memory.regions[fd_num].memory_size = reg->memory_size;
> -msg.memory.regions[fd_num].guest_phys_addr =
> reg->guest_phys_addr;
> -msg.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
> +msg.payload.memory.regions[fd_num].userspace_addr =
> reg->userspace_addr;
> +msg.payload.memory.regions[fd_num].memory_size =
> reg->memory_size;
> +msg.payload.memory.regions[fd_num].guest_phys_addr =
> reg->guest_phys_addr;
> +msg.payload.memory.regions[fd_num].mmap_offset =
> reg->userspace_addr -
> (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
> assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> fds[fd_num++] = fd;
> }
> }
>
> -msg.memory.nregions = fd_num;
> +msg.payload.memory.nregions = fd_num;
>
> if (!fd_num) {
> error_report("Failed initializing vhost-user memory map, "
> @@ -265,8 +265,8 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
> return -1;
> }
>
> -msg.size = sizeof(m.memory.nregions);
> -msg.size += sizeof(m.memory.padding);
> +msg.size = sizeof(m.payload.memory.nregions);
> +msg.size += sizeof(m.payload.memory.padding);
> msg.size += fd_num * sizeof(VhostUserMemoryRegion);
>
> vhost_user_write(dev, &msg, fds, fd_num);
> @@ -280,7 +280,7 @@ static int vhost_user_set_vring_addr(struct vhost_dev
> *dev,
> VhostUserMsg msg = {
> .request = VHOST_USER_SET_VRING_ADDR,
> .flags = VHOST_USER_VERSION,
> -.addr = *addr,
> +.payload.addr = *addr,
> .size = sizeof(*addr),
> };
>
> @@ -303,7 +303,7 @@ static int vhost_set_vring(struct vhost_dev *dev,
> VhostUserMsg msg = {
> .request = request,
> .flags = VHOST_USER_VERSION,
> -.state = *ring,
> +.payload.state = *ring,
> .size = sizeof(*ring),
> };
>
> @@ -345,7 +345,7 @@ static int vhost_user_get_vring_base(struct vhost_dev
> *dev,
> VhostUserMsg msg = {
> .request = VHOST_USER_GET_VRING_BASE,
> .flags = VHOST_USER_VERSION,
> -.state = *ring,
> +.payload.state = *ring,
> .size = sizeof(*ring),
> };
>
> @@ -361,12 +361,12 @@ static int vhost_user_get_vring_base(struct vhost_dev
> *dev,
> return -1;
> }
>
> -if (msg.size != sizeof(m.state)) {
> +if (msg.size != sizeof(m.payload.state)) {
> error_report("Received bad msg size.");
> return -1;
> }
>
> -*ring = msg.state;
> +*ring = msg.payload.state;
>
> return 0;
> }
> @@ -380,14 +380,14 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
> VhostUserMsg msg = {
> .request = request,
> .flags = VHOST_USER_VERSION,
> -.u64 = file->index & VHOST_USER_VRING_IDX_MASK,
> -.size = sizeof(m.u64),
> +.payload.u64 = file->index & VHOST_USER_VRING_IDX_MASK,
> +.size = sizeof(m.payload.u64),
> };
>
> if (ioeventfd_enabled() && file->fd > 0) {
> fds[fd_num++] = file->fd;
> } else {
> -msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
> +msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
> }
>
> vhost_user_write(dev, &msg, fds, fd_num);
> @@ -412,8 +412,8 @@ static
[Qemu-devel] [PATCH v10 17/25] net: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for net-related code.
Signed-off-by: Eric Blake
---
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
net/dump.c | 4 ++--
net/hub.c| 4 ++--
net/l2tpv3.c | 4 ++--
net/net.c| 24
net/slirp.c | 4 ++--
net/socket.c | 4 ++--
net/tap-win32.c | 4 ++--
net/tap.c| 8
net/vde.c| 4 ++--
net/vhost-user.c | 4 ++--
10 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/net/dump.c b/net/dump.c
index 08259af..2812070 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -154,8 +154,8 @@ int net_init_dump(const NetClientOptions *opts, const char
*name,
char def_file[128];
const NetdevDumpOptions *dump;
-assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
-dump = opts->dump;
+assert(opts->type == NET_CLIENT_OPTIONS_KIND_DUMP);
+dump = opts->u.dump;
assert(peer);
diff --git a/net/hub.c b/net/hub.c
index 3047f12..9ae9f01 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -285,9 +285,9 @@ int net_init_hubport(const NetClientOptions *opts, const
char *name,
{
const NetdevHubPortOptions *hubport;
-assert(opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT);
+assert(opts->type == NET_CLIENT_OPTIONS_KIND_HUBPORT);
assert(!peer);
-hubport = opts->hubport;
+hubport = opts->u.hubport;
net_hub_add_port(hubport->hubid, name);
return 0;
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 4f9bcee..8e68e54 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -545,8 +545,8 @@ int net_init_l2tpv3(const NetClientOptions *opts,
s->queue_tail = 0;
s->header_mismatch = false;
-assert(opts->kind == NET_CLIENT_OPTIONS_KIND_L2TPV3);
-l2tpv3 = opts->l2tpv3;
+assert(opts->type == NET_CLIENT_OPTIONS_KIND_L2TPV3);
+l2tpv3 = opts->u.l2tpv3;
if (l2tpv3->has_ipv6 && l2tpv3->ipv6) {
s->ipv6 = l2tpv3->ipv6;
diff --git a/net/net.c b/net/net.c
index 3c68f3f..d5aeb31 100644
--- a/net/net.c
+++ b/net/net.c
@@ -882,8 +882,8 @@ static int net_init_nic(const NetClientOptions *opts, const
char *name,
NICInfo *nd;
const NetLegacyNicOptions *nic;
-assert(opts->kind == NET_CLIENT_OPTIONS_KIND_NIC);
-nic = opts->nic;
+assert(opts->type == NET_CLIENT_OPTIONS_KIND_NIC);
+nic = opts->u.nic;
idx = nic_get_free_idx();
if (idx == -1 || nb_nics >= MAX_NICS) {
@@ -984,9 +984,9 @@ static int net_client_init1(const void *object, int
is_netdev, Error **errp)
opts = netdev->opts;
name = netdev->id;
-if (opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP ||
-opts->kind == NET_CLIENT_OPTIONS_KIND_NIC ||
-!net_client_init_fun[opts->kind]) {
+if (opts->type == NET_CLIENT_OPTIONS_KIND_DUMP ||
+opts->type == NET_CLIENT_OPTIONS_KIND_NIC ||
+!net_client_init_fun[opts->type]) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a netdev backend type");
return -1;
@@ -997,16 +997,16 @@ static int net_client_init1(const void *object, int
is_netdev, Error **errp)
/* missing optional values have been initialized to "all bits zero" */
name = net->has_id ? net->id : net->name;
-if (opts->kind == NET_CLIENT_OPTIONS_KIND_NONE) {
+if (opts->type == NET_CLIENT_OPTIONS_KIND_NONE) {
return 0; /* nothing to do */
}
-if (opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
+if (opts->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a net type");
return -1;
}
-if (!net_client_init_fun[opts->kind]) {
+if (!net_client_init_fun[opts->type]) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a net backend type (maybe it is not compiled "
"into this binary)");
@@ -1014,17 +1014,17 @@ static int net_client_init1(const void *object, int
is_netdev, Error **errp)
}
/* Do not add to a vlan if it's a nic with a netdev= parameter. */
-if (opts->kind != NET_CLIENT_OPTIONS_KIND_NIC ||
-!opts->nic->has_netdev) {
+if (opts->type != NET_CLIENT_OPTIONS_KIND_NIC ||
+!opts->u.nic->has_netdev) {
peer = net_hub_add_port(net->has_vlan ? net->vlan : 0, NULL);
}
}
-if (net_client_init_fun[opts->kind](opts, name, peer, errp
[Qemu-devel] [PATCH v10 15/25] block: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for block-related code.
Signed-off-by: Eric Blake
---
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
block/nbd.c | 18 +-
block/qcow2.c | 10 --
block/vmdk.c | 6 +++---
blockdev.c| 47 ---
4 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index c2a87e9..cd6a587 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -206,24 +206,24 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict
*options, char **export,
saddr = g_new0(SocketAddress, 1);
if (qdict_haskey(options, "path")) {
-saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
-saddr->q_unix = g_new0(UnixSocketAddress, 1);
-saddr->q_unix->path = g_strdup(qdict_get_str(options, "path"));
+saddr->type = SOCKET_ADDRESS_KIND_UNIX;
+saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+saddr->u.q_unix->path = g_strdup(qdict_get_str(options, "path"));
qdict_del(options, "path");
} else {
-saddr->kind = SOCKET_ADDRESS_KIND_INET;
-saddr->inet = g_new0(InetSocketAddress, 1);
-saddr->inet->host = g_strdup(qdict_get_str(options, "host"));
+saddr->type = SOCKET_ADDRESS_KIND_INET;
+saddr->u.inet = g_new0(InetSocketAddress, 1);
+saddr->u.inet->host = g_strdup(qdict_get_str(options, "host"));
if (!qdict_get_try_str(options, "port")) {
-saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+saddr->u.inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
} else {
-saddr->inet->port = g_strdup(qdict_get_str(options, "port"));
+saddr->u.inet->port = g_strdup(qdict_get_str(options, "port"));
}
qdict_del(options, "host");
qdict_del(options, "port");
}
-s->client.is_unix = saddr->kind == SOCKET_ADDRESS_KIND_UNIX;
+s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
*export = g_strdup(qdict_get_try_str(options, "export"));
if (*export) {
diff --git a/block/qcow2.c b/block/qcow2.c
index bacc4f2..88f56c8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2738,18 +2738,16 @@ static ImageInfoSpecific
*qcow2_get_specific_info(BlockDriverState *bs)
ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
*spec_info = (ImageInfoSpecific){
-.kind = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
-{
-.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
-},
+.type = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
+.u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
};
if (s->qcow_version == 2) {
-*spec_info->qcow2 = (ImageInfoSpecificQCow2){
+*spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
.compat = g_strdup("0.10"),
.refcount_bits = s->refcount_bits,
};
} else if (s->qcow_version == 3) {
-*spec_info->qcow2 = (ImageInfoSpecificQCow2){
+*spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
.compat = g_strdup("1.1"),
.lazy_refcounts = s->compatible_features &
QCOW2_COMPAT_LAZY_REFCOUNTS,
diff --git a/block/vmdk.c b/block/vmdk.c
index 0effb7d..6f819e4 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2161,19 +2161,19 @@ static ImageInfoSpecific
*vmdk_get_specific_info(BlockDriverState *bs)
ImageInfoList **next;
*spec_info = (ImageInfoSpecific){
-.kind = IMAGE_INFO_SPECIFIC_KIND_VMDK,
+.type = IMAGE_INFO_SPECIFIC_KIND_VMDK,
{
.vmdk = g_new0(ImageInfoSpecificVmdk, 1),
},
};
-*spec_info->vmdk = (ImageInfoSpecificVmdk) {
+*spec_info->u.vmdk = (ImageInfoSpecificVmdk) {
.create_type = g_strdup(s->create_type),
.cid = s->cid,
.parent_cid = s->parent_cid,
};
-next = &spec_info->vmdk->extents;
+next = &spec_info->u.vmdk->extents;
for (i = 0; i < s->num_extents; i++) {
*next = g_new0(ImageInfoList, 1);
(*next)->value = vmdk_get_extent_info(&s->extents[i]);
diff --git a/blockdev.c b/blockdev.c
index 8141b6b..0bcd3f1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1050,13 +1050,14 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
}
}
-static void blockdev_do_action(int kind, void *data, Error **errp)
+static void blockdev_do_action(TransactionActionKind type, void *data,
+ Error **errp)
{
Tran
[Qemu-devel] [PATCH v10 25/25] qapi: Simplify gen_struct_field()
Rather than having all callers pass a name, type, and optional
flag, have them instead pass a QAPISchemaObjectTypeMember which
already has all that information.
No change to generated code.
Signed-off-by: Eric Blake
---
v10: no change
v9: rebase after kind/base cleanups, don't rely on member.c_name()
v8: new patch
---
scripts/qapi-types.py | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index d131430..f8fc272 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -36,18 +36,18 @@ struct %(c_name)s {
c_name=c_name(name), c_type=element_type.c_type())
-def gen_struct_field(name, typ, optional):
+def gen_struct_field(member):
ret = ''
-if optional:
+if member.optional:
ret += mcgen('''
bool has_%(c_name)s;
''',
- c_name=c_name(name))
+ c_name=c_name(member.name))
ret += mcgen('''
%(c_type)s %(c_name)s;
''',
- c_type=typ.c_type(), c_name=c_name(name))
+ c_type=member.type.c_type(), c_name=c_name(member.name))
return ret
@@ -65,7 +65,7 @@ def gen_struct_fields(members, base=None):
''')
for memb in members:
-ret += gen_struct_field(memb.name, memb.type, memb.optional)
+ret += gen_struct_field(memb)
return ret
@@ -148,9 +148,7 @@ struct %(c_name)s {
if base:
ret += gen_struct_fields([], base)
else:
-ret += gen_struct_field(variants.tag_member.name,
-variants.tag_member.type,
-False)
+ret += gen_struct_field(variants.tag_member)
# FIXME: What purpose does data serve, besides preventing a union that
# has a branch named 'data'? We use it in qapi-visit.py to decide
--
2.4.3
[Qemu-devel] [PATCH v10 20/25] memory: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for memory-related code.
Signed-off-by: Eric Blake
---
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
hmp.c| 6 +++---
hw/mem/pc-dimm.c | 6 +++---
numa.c | 8
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hmp.c b/hmp.c
index cc4946d..39d5815 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1958,12 +1958,12 @@ void hmp_info_memory_devices(Monitor *mon, const QDict
*qdict)
value = info->value;
if (value) {
-switch (value->kind) {
+switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
-di = value->dimm;
+di = value->u.dimm;
monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
- MemoryDeviceInfoKind_lookup[value->kind],
+ MemoryDeviceInfoKind_lookup[value->type],
di->id ? di->id : "");
monitor_printf(mon, " addr: 0x%" PRIx64 "\n", di->addr);
monitor_printf(mon, " slot: %" PRId64 "\n", di->slot);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 2bae994..fd84a90 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -180,7 +180,7 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
NULL);
di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
-info->dimm = di;
+info->u.dimm = di;
elem->value = info;
elem->next = NULL;
**prev = elem;
@@ -204,9 +204,9 @@ ram_addr_t get_current_ram_size(void)
MemoryDeviceInfo *value = info->value;
if (value) {
-switch (value->kind) {
+switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
-size += value->dimm->size;
+size += value->u.dimm->size;
break;
default:
break;
diff --git a/numa.c b/numa.c
index e9b18f5..fdfe294 100644
--- a/numa.c
+++ b/numa.c
@@ -226,9 +226,9 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error
**errp)
goto error;
}
-switch (object->kind) {
+switch (object->type) {
case NUMA_OPTIONS_KIND_NODE:
-numa_node_parse(object->node, opts, &err);
+numa_node_parse(object->u.node, opts, &err);
if (err) {
goto error;
}
@@ -487,9 +487,9 @@ static void numa_stat_memory_devices(uint64_t node_mem[])
MemoryDeviceInfo *value = info->value;
if (value) {
-switch (value->kind) {
+switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
-node_mem[value->dimm->node] += value->dimm->size;
+node_mem[value->u.dimm->node] += value->u.dimm->size;
break;
default:
break;
--
2.4.3
[Qemu-devel] [PATCH v10 24/25] qapi: Remove outdated tests related to QMP/branch collisions
Now that branches are in a separate C namespace, we can remove
the restrictions in the parser that claim a branch name would
collide with QMP, and delete the negative tests that are no
longer problematic. A separate patch can then add positive
tests to qapi-schema-test to test that any corner cases will
compile correctly.
Signed-off-by: Eric Blake
---
v10: new patch
---
scripts/qapi.py| 7 +--
tests/Makefile | 3 ---
tests/qapi-schema/flat-union-clash-branch.err | 0
tests/qapi-schema/flat-union-clash-branch.exit | 1 -
tests/qapi-schema/flat-union-clash-branch.json | 18 --
tests/qapi-schema/flat-union-clash-branch.out | 14 --
tests/qapi-schema/flat-union-clash-type.err| 1 -
tests/qapi-schema/flat-union-clash-type.exit | 1 -
tests/qapi-schema/flat-union-clash-type.json | 14 --
tests/qapi-schema/flat-union-clash-type.out| 0
tests/qapi-schema/union-clash-type.err | 1 -
tests/qapi-schema/union-clash-type.exit| 1 -
tests/qapi-schema/union-clash-type.json| 9 -
tests/qapi-schema/union-clash-type.out | 0
14 files changed, 1 insertion(+), 69 deletions(-)
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.err
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.exit
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.json
delete mode 100644 tests/qapi-schema/flat-union-clash-branch.out
delete mode 100644 tests/qapi-schema/flat-union-clash-type.err
delete mode 100644 tests/qapi-schema/flat-union-clash-type.exit
delete mode 100644 tests/qapi-schema/flat-union-clash-type.json
delete mode 100644 tests/qapi-schema/flat-union-clash-type.out
delete mode 100644 tests/qapi-schema/union-clash-type.err
delete mode 100644 tests/qapi-schema/union-clash-type.exit
delete mode 100644 tests/qapi-schema/union-clash-type.json
delete mode 100644 tests/qapi-schema/union-clash-type.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index b941ccf..3a5dbac 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -548,7 +548,7 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
-values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'}
+values = {'MAX': '(automatic)'}
# Two types of unions, determined by discriminator.
@@ -614,11 +614,6 @@ def check_union(expr, expr_info):
"Discriminator value '%s' is not found in "
"enum '%s'" %
(key, enum_define["enum_name"]))
-if discriminator in enum_define['enum_values']:
-raise QAPIExprError(expr_info,
-"Discriminator name '%s' collides with "
-"enum value in '%s'" %
-(discriminator, enum_define["enum_name"]))
# Otherwise, check for conflicts in the generated enum
else:
diff --git a/tests/Makefile b/tests/Makefile
index 8c239f8..70524df 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -277,9 +277,7 @@ qapi-schema += flat-union-bad-base.json
qapi-schema += flat-union-bad-discriminator.json
qapi-schema += flat-union-base-any.json
qapi-schema += flat-union-base-union.json
-qapi-schema += flat-union-clash-branch.json
qapi-schema += flat-union-clash-member.json
-qapi-schema += flat-union-clash-type.json
qapi-schema += flat-union-empty.json
qapi-schema += flat-union-inline.json
qapi-schema += flat-union-int-branch.json
@@ -339,7 +337,6 @@ qapi-schema += union-bad-branch.json
qapi-schema += union-base-no-discriminator.json
qapi-schema += union-clash-branches.json
qapi-schema += union-clash-data.json
-qapi-schema += union-clash-type.json
qapi-schema += union-empty.json
qapi-schema += union-invalid-base.json
qapi-schema += union-max.json
diff --git a/tests/qapi-schema/flat-union-clash-branch.err
b/tests/qapi-schema/flat-union-clash-branch.err
deleted file mode 100644
index e69de29..000
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit
b/tests/qapi-schema/flat-union-clash-branch.exit
deleted file mode 100644
index 573541a..000
--- a/tests/qapi-schema/flat-union-clash-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/flat-union-clash-branch.json
b/tests/qapi-schema/flat-union-clash-branch.json
deleted file mode 100644
index e593336..000
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ /dev/null
@@ -1,18 +0,0 @@
-# Flat union branch name collision
-# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
-# (one from the base member, the other from the branch name). We should
-# either reject the collision at parse time, or munge the generated branch
-# name to allow this to compile.
-{ 'enum': 'TestEnum',
- 'data': [ 'base', 'c-d' ] }
-{
[Qemu-devel] [PATCH v10 16/25] sockets: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for socket-related code.
Signed-off-by: Eric Blake
---
v10: rebase to latest, retitle from nbd to sockets
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
qemu-nbd.c | 16 +++---
ui/vnc.c| 44 ++---
util/qemu-sockets.c | 62 ++---
3 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 422a607..3afec76 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -362,17 +362,17 @@ static SocketAddress *nbd_build_socket_address(const char
*sockpath,
saddr = g_new0(SocketAddress, 1);
if (sockpath) {
-saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
-saddr->q_unix = g_new0(UnixSocketAddress, 1);
-saddr->q_unix->path = g_strdup(sockpath);
+saddr->type = SOCKET_ADDRESS_KIND_UNIX;
+saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+saddr->u.q_unix->path = g_strdup(sockpath);
} else {
-saddr->kind = SOCKET_ADDRESS_KIND_INET;
-saddr->inet = g_new0(InetSocketAddress, 1);
-saddr->inet->host = g_strdup(bindto);
+saddr->type = SOCKET_ADDRESS_KIND_INET;
+saddr->u.inet = g_new0(InetSocketAddress, 1);
+saddr->u.inet->host = g_strdup(bindto);
if (port) {
-saddr->inet->port = g_strdup(port);
+saddr->u.inet->port = g_strdup(port);
} else {
-saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+saddr->u.inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
}
}
diff --git a/ui/vnc.c b/ui/vnc.c
index cec2cee..7b37e3b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3524,9 +3524,9 @@ void vnc_display_open(const char *id, Error **errp)
}
if (strncmp(vnc, "unix:", 5) == 0) {
-saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
-saddr->q_unix = g_new0(UnixSocketAddress, 1);
-saddr->q_unix->path = g_strdup(vnc + 5);
+saddr->type = SOCKET_ADDRESS_KIND_UNIX;
+saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+saddr->u.q_unix->path = g_strdup(vnc + 5);
if (vs->ws_enabled) {
error_setg(errp, "UNIX sockets not supported with websock");
@@ -3534,12 +3534,12 @@ void vnc_display_open(const char *id, Error **errp)
}
} else {
unsigned long long baseport;
-saddr->kind = SOCKET_ADDRESS_KIND_INET;
-saddr->inet = g_new0(InetSocketAddress, 1);
+saddr->type = SOCKET_ADDRESS_KIND_INET;
+saddr->u.inet = g_new0(InetSocketAddress, 1);
if (vnc[0] == '[' && vnc[hlen - 1] == ']') {
-saddr->inet->host = g_strndup(vnc + 1, hlen - 2);
+saddr->u.inet->host = g_strndup(vnc + 1, hlen - 2);
} else {
-saddr->inet->host = g_strndup(vnc, hlen);
+saddr->u.inet->host = g_strndup(vnc, hlen);
}
if (parse_uint_full(h + 1, &baseport, 10) < 0) {
error_setg(errp, "can't convert to a number: %s", h + 1);
@@ -3550,28 +3550,28 @@ void vnc_display_open(const char *id, Error **errp)
error_setg(errp, "port %s out of range", h + 1);
goto fail;
}
-saddr->inet->port = g_strdup_printf(
+saddr->u.inet->port = g_strdup_printf(
"%d", (int)baseport + 5900);
if (to) {
-saddr->inet->has_to = true;
-saddr->inet->to = to;
+saddr->u.inet->has_to = true;
+saddr->u.inet->to = to;
}
-saddr->inet->ipv4 = saddr->inet->has_ipv4 = has_ipv4;
-saddr->inet->ipv6 = saddr->inet->has_ipv6 = has_ipv6;
+saddr->u.inet->ipv4 = saddr->u.inet->has_ipv4 = has_ipv4;
+saddr->u.inet->ipv6 = saddr->u.inet->has_ipv6 = has_ipv6;
if (vs->ws_enabled) {
-wsaddr->kind = SOCKET_ADDRESS_KIND_INET;
-wsaddr->inet = g_new0(InetSocketAddress, 1);
-wsaddr->inet->host = g_strdup(saddr->inet->host);
-wsaddr->inet->port = g_strdup(websocket);
+wsaddr->type = SOCKET_ADDRESS_KIND_INET;
+wsaddr->u.inet = g_new0(InetSocketAddress, 1);
+wsaddr->u.inet->host = g_strdup(saddr->u.inet->host);
+wsaddr->u.inet->port = g_strdup(websocket);
if (to) {
-
[Qemu-devel] [PATCH v10 21/25] tpm: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for TPM-related code.
Signed-off-by: Eric Blake
---
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
hmp.c | 6 +++---
tpm.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hmp.c b/hmp.c
index 39d5815..a15d00c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -841,11 +841,11 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
c, TpmModel_lookup[ti->model]);
monitor_printf(mon, " \\ %s: type=%s",
- ti->id, TpmTypeOptionsKind_lookup[ti->options->kind]);
+ ti->id, TpmTypeOptionsKind_lookup[ti->options->type]);
-switch (ti->options->kind) {
+switch (ti->options->type) {
case TPM_TYPE_OPTIONS_KIND_PASSTHROUGH:
-tpo = ti->options->passthrough;
+tpo = ti->options->u.passthrough;
monitor_printf(mon, "%s%s%s%s",
tpo->has_path ? ",path=" : "",
tpo->has_path ? tpo->path : "",
diff --git a/tpm.c b/tpm.c
index 4e9b109..f2c59d1 100644
--- a/tpm.c
+++ b/tpm.c
@@ -260,9 +260,9 @@ static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)
switch (drv->ops->type) {
case TPM_TYPE_PASSTHROUGH:
-res->options->kind = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;
+res->options->type = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;
tpo = g_new0(TPMPassthroughOptions, 1);
-res->options->passthrough = tpo;
+res->options->u.passthrough = tpo;
if (drv->path) {
tpo->path = g_strdup(drv->path);
tpo->has_path = true;
--
2.4.3
[Qemu-devel] [PATCH v10 18/25] char: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for character-related
code.
Signed-off-by: Eric Blake
---
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
qemu-char.c | 160 +++---
spice-qemu-char.c | 12 ++--
2 files changed, 86 insertions(+), 86 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 908e712..d842aa6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -97,18 +97,18 @@ static int SocketAddress_to_str(char *dest, int max_len,
const char *prefix, SocketAddress *addr,
bool is_listen, bool is_telnet)
{
-switch (addr->kind) {
+switch (addr->type) {
case SOCKET_ADDRESS_KIND_INET:
return snprintf(dest, max_len, "%s%s:%s:%s%s", prefix,
-is_telnet ? "telnet" : "tcp", addr->inet->host,
-addr->inet->port, is_listen ? ",server" : "");
+is_telnet ? "telnet" : "tcp", addr->u.inet->host,
+addr->u.inet->port, is_listen ? ",server" : "");
break;
case SOCKET_ADDRESS_KIND_UNIX:
return snprintf(dest, max_len, "%sunix:%s%s", prefix,
-addr->q_unix->path, is_listen ? ",server" : "");
+addr->u.q_unix->path, is_listen ? ",server" : "");
break;
case SOCKET_ADDRESS_KIND_FD:
-return snprintf(dest, max_len, "%sfd:%s%s", prefix, addr->fd->str,
+return snprintf(dest, max_len, "%sfd:%s%s", prefix, addr->u.fd->str,
is_listen ? ",server" : "");
break;
default:
@@ -661,7 +661,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id,
ChardevBackend *backend,
ChardevReturn *ret, Error **errp)
{
-ChardevMux *mux = backend->mux;
+ChardevMux *mux = backend->u.mux;
CharDriverState *chr, *drv;
MuxDriver *d;
@@ -1070,7 +1070,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
ChardevReturn *ret,
Error **errp)
{
-ChardevHostdev *opts = backend->pipe;
+ChardevHostdev *opts = backend->u.pipe;
int fd_in, fd_out;
char filename_in[CHR_MAX_FILENAME_SIZE];
char filename_out[CHR_MAX_FILENAME_SIZE];
@@ -1148,7 +1148,7 @@ static CharDriverState *qemu_chr_open_stdio(const char
*id,
ChardevReturn *ret,
Error **errp)
{
-ChardevStdio *opts = backend->stdio;
+ChardevStdio *opts = backend->u.stdio;
CharDriverState *chr;
struct sigaction act;
@@ -2147,7 +2147,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
ChardevReturn *ret,
Error **errp)
{
-ChardevHostdev *opts = backend->pipe;
+ChardevHostdev *opts = backend->u.pipe;
const char *filename = opts->device;
CharDriverState *chr;
WinCharState *s;
@@ -3202,7 +3202,7 @@ static CharDriverState *qemu_chr_open_ringbuf(const char
*id,
ChardevReturn *ret,
Error **errp)
{
-ChardevRingbuf *opts = backend->ringbuf;
+ChardevRingbuf *opts = backend->u.ringbuf;
CharDriverState *chr;
RingBufCharDriver *d;
@@ -3477,16 +3477,16 @@ static void qemu_chr_parse_file_out(QemuOpts *opts,
ChardevBackend *backend,
error_setg(errp, "chardev: file: no filename given");
return;
}
-backend->file = g_new0(ChardevFile, 1);
-backend->file->out = g_strdup(path);
+backend->u.file = g_new0(ChardevFile, 1);
+backend->u.file->out = g_strdup(path);
}
static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend,
Error **errp)
{
-backend->stdio = g_new0(ChardevStdio, 1);
-backend->stdio->has_signal = true;
-backend->stdio->signal = qemu_opt_get_bool(opts, "signal", true);
+backend->u.stdio = g_new0(ChardevStdio, 1);
+backend->u.stdio->has_signal = true;
+backend->u.stdio->signal = qemu_opt_get_bool(opts, "signal", true);
}
#ifdef HAVE_CHARDEV_SERIAL
@@ -3499,8 +3499,8 @@ static void qemu_chr_parse_serial(QemuOpts *opts,
ChardevBackend *backend,
error_setg(errp, "chardev: serial/tty: no device path given");
[Qemu-devel] [PATCH v10 10/25] qapi: Unbox base members
Rather than storing a base class as a pointer to a box, just
store the fields of that base class in the same order, so that
a child struct can be directly cast to its parent. This gives
less malloc overhead, less pointer dereferencing, and even less
generated code. Compare to the earlier commit 1e6c1616a "qapi:
Generate a nicer struct for flat unions" (although that patch
had fewer places to change, as less of qemu was directly using
qapi structs for flat unions). It also drops a hack that was
needed for type-safe casting to the base class of a struct.
Changes to the generated code look like this in qapi-types.h:
| struct SpiceChannel {
|-SpiceBasicInfo *base;
|+/* Members inherited from SpiceBasicInfo: */
|+char *host;
|+char *port;
|+NetworkAddressFamily family;
|+/* Own members: */
| int64_t connection_id;
as well as:
| static inline SpiceBasicInfo *qapi_SpiceChannel_base(const SpiceChannel *obj)
| {
|-return (SpiceBasicInfo *)obj->base;
|+return (SpiceBasicInfo *)obj;
| }
and this in qapi-visit.c:
| static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj,
Error **errp)
| {
| Error *err = NULL;
|
|-visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err);
|+visit_type_SpiceBasiInfo_fields(v, (SpiceBasicInfo **)obj, &err);
| if (err) {
plus the wholesale elimination of some now-unused
visit_type_implicit_FOO() functions.
Without boxing, the corner case of one empty struct having
another empty struct as its base type now requires inserting a
dummy member (previously, the pointer to the base would have
sufficed).
And now that we no longer consume a 'base' member in the generated
C struct, we can delete the former negative struct-base-clash-base
test.
Signed-off-by: Eric Blake
---
v10: split off some of the cleanups into earlier patches, improve
commit message, don't emit visit_type_FOO_fields out of order,
save positive tests in qapi-schema-tests for later
v9: (no v6-8): hoist from v5 34/46, rebase to master
---
hmp.c | 6 +++---
scripts/qapi-types.py | 20 ++--
scripts/qapi-visit.py | 13 -
tests/Makefile| 1 -
tests/qapi-schema/struct-base-clash-base.err | 0
tests/qapi-schema/struct-base-clash-base.exit | 1 -
tests/qapi-schema/struct-base-clash-base.json | 9 -
tests/qapi-schema/struct-base-clash-base.out | 5 -
tests/test-qmp-commands.c | 15 +--
tests/test-qmp-event.c| 8 ++--
tests/test-qmp-input-visitor.c| 4 ++--
tests/test-qmp-output-visitor.c | 13 +
tests/test-visitor-serialization.c| 14 ++
ui/spice-core.c | 9 +++--
ui/vnc.c | 12
15 files changed, 40 insertions(+), 90 deletions(-)
delete mode 100644 tests/qapi-schema/struct-base-clash-base.err
delete mode 100644 tests/qapi-schema/struct-base-clash-base.exit
delete mode 100644 tests/qapi-schema/struct-base-clash-base.json
delete mode 100644 tests/qapi-schema/struct-base-clash-base.out
diff --git a/hmp.c b/hmp.c
index 5048eee..88fd804 100644
--- a/hmp.c
+++ b/hmp.c
@@ -569,8 +569,8 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
for (client = info->clients; client; client = client->next) {
monitor_printf(mon, "Client:\n");
monitor_printf(mon, " address: %s:%s\n",
- client->value->base->host,
- client->value->base->service);
+ client->value->host,
+ client->value->service);
monitor_printf(mon, " x509_dname: %s\n",
client->value->x509_dname ?
client->value->x509_dname : "none");
@@ -638,7 +638,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
for (chan = info->channels; chan; chan = chan->next) {
monitor_printf(mon, "Channel:\n");
monitor_printf(mon, " address: %s:%s%s\n",
- chan->value->base->host, chan->value->base->port,
+ chan->value->host, chan->value->port,
chan->value->tls ? " [tls]" : "");
monitor_printf(mon, " session: %" PRId64 "\n",
chan->value->connection_id);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index ed4a11c..9c51d8f 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -76,16 +76,13 @@ struct %(c_name)s {
''',
c_name=c_name(name))
-if base:
-ret += gen_struct_field('base', base, False)
-
-ret += gen_struct_fields(members)
+ret += gen_struct_fields(members, base)
# Make sure that all structs have at lea
[Qemu-devel] [PATCH v10 19/25] input: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for input-related code.
Signed-off-by: Eric Blake
---
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
hmp.c | 8 ++---
hw/char/escc.c | 12 +++
hw/input/hid.c | 32 -
hw/input/ps2.c | 24 ++---
hw/input/virtio-input-hid.c | 27 ---
ui/console.c| 20 +--
ui/input-keymap.c | 20 +--
ui/input-legacy.c | 21 ++--
ui/input.c | 84 ++---
9 files changed, 125 insertions(+), 123 deletions(-)
diff --git a/hmp.c b/hmp.c
index 88fd804..cc4946d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1735,15 +1735,15 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
if (*endp != '\0') {
goto err_out;
}
-keylist->value->kind = KEY_VALUE_KIND_NUMBER;
-keylist->value->number = value;
+keylist->value->type = KEY_VALUE_KIND_NUMBER;
+keylist->value->u.number = value;
} else {
int idx = index_from_key(keyname_buf);
if (idx == Q_KEY_CODE_MAX) {
goto err_out;
}
-keylist->value->kind = KEY_VALUE_KIND_QCODE;
-keylist->value->qcode = idx;
+keylist->value->type = KEY_VALUE_KIND_QCODE;
+keylist->value->u.qcode = idx;
}
if (!separator) {
diff --git a/hw/char/escc.c b/hw/char/escc.c
index ba653ef..c144e95 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -842,13 +842,13 @@ static void sunkbd_handle_event(DeviceState *dev,
QemuConsole *src,
ChannelState *s = (ChannelState *)dev;
int qcode, keycode;
-assert(evt->kind == INPUT_EVENT_KIND_KEY);
-qcode = qemu_input_key_value_to_qcode(evt->key->key);
+assert(evt->type == INPUT_EVENT_KIND_KEY);
+qcode = qemu_input_key_value_to_qcode(evt->u.key->key);
trace_escc_sunkbd_event_in(qcode, QKeyCode_lookup[qcode],
- evt->key->down);
+ evt->u.key->down);
if (qcode == Q_KEY_CODE_CAPS_LOCK) {
-if (evt->key->down) {
+if (evt->u.key->down) {
s->caps_lock_mode ^= 1;
if (s->caps_lock_mode == 2) {
return; /* Drop second press */
@@ -862,7 +862,7 @@ static void sunkbd_handle_event(DeviceState *dev,
QemuConsole *src,
}
if (qcode == Q_KEY_CODE_NUM_LOCK) {
-if (evt->key->down) {
+if (evt->u.key->down) {
s->num_lock_mode ^= 1;
if (s->num_lock_mode == 2) {
return; /* Drop second press */
@@ -876,7 +876,7 @@ static void sunkbd_handle_event(DeviceState *dev,
QemuConsole *src,
}
keycode = qcode_to_keycode[qcode];
-if (!evt->key->down) {
+if (!evt->u.key->down) {
keycode |= 0x80;
}
trace_escc_sunkbd_event_out(keycode);
diff --git a/hw/input/hid.c b/hw/input/hid.c
index 21ebd9e..e39269f 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -119,33 +119,33 @@ static void hid_pointer_event(DeviceState *dev,
QemuConsole *src,
assert(hs->n < QUEUE_LENGTH);
e = &hs->ptr.queue[(hs->head + hs->n) & QUEUE_MASK];
-switch (evt->kind) {
+switch (evt->type) {
case INPUT_EVENT_KIND_REL:
-if (evt->rel->axis == INPUT_AXIS_X) {
-e->xdx += evt->rel->value;
-} else if (evt->rel->axis == INPUT_AXIS_Y) {
-e->ydy += evt->rel->value;
+if (evt->u.rel->axis == INPUT_AXIS_X) {
+e->xdx += evt->u.rel->value;
+} else if (evt->u.rel->axis == INPUT_AXIS_Y) {
+e->ydy += evt->u.rel->value;
}
break;
case INPUT_EVENT_KIND_ABS:
-if (evt->rel->axis == INPUT_AXIS_X) {
-e->xdx = evt->rel->value;
-} else if (evt->rel->axis == INPUT_AXIS_Y) {
-e->ydy = evt->rel->value;
+if (evt->u.rel->axis == INPUT_AXIS_X) {
+e->xdx = evt->u.rel->value;
+} else if (evt->u.rel->axis == INPUT_AXIS_Y) {
+e->ydy = evt->u.rel->value;
}
break;
case INPUT_EVENT_KIND_BTN:
-if (evt->btn->down) {
-e->buttons_state |= bmap[evt->btn->button];
-if (evt->btn->button == INPUT_BUTTON_WHEEL_UP) {
+if (evt->u.btn->down) {
+e->buttons_state |= bmap[evt->u.btn->button];
+if (evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
[Qemu-devel] [PATCH v10 14/25] tests: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for testsuite code.
Signed-off-by: Eric Blake
---
v10: split qapi.py change into earlier less-mechanical patch
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
tests/test-qmp-commands.c | 4 +--
tests/test-qmp-input-visitor.c | 78 -
tests/test-qmp-output-visitor.c | 42 +++---
3 files changed, 62 insertions(+), 62 deletions(-)
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index ea700d8..f23d8ea 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -62,8 +62,8 @@ __org_qemu_x_Union1
*qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
{
__org_qemu_x_Union1 *ret = g_new0(__org_qemu_x_Union1, 1);
-ret->kind = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
-ret->__org_qemu_x_branch = strdup("blah1");
+ret->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
+ret->u.__org_qemu_x_branch = strdup("blah1");
return ret;
}
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 514cfd0..617eada 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -359,7 +359,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData
*data,
g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1);
g_assert_cmpstr(tmp->string, ==, "str");
g_assert_cmpint(tmp->integer, ==, 41);
-g_assert_cmpint(tmp->value1->boolean, ==, true);
+g_assert_cmpint(tmp->u.value1->boolean, ==, true);
qapi_free_UserDefFlatUnion(tmp);
}
@@ -372,15 +372,15 @@ static void
test_visitor_in_alternate(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
-g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
-g_assert_cmpint(tmp->i, ==, 42);
+g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
+g_assert_cmpint(tmp->u.i, ==, 42);
qapi_free_UserDefAlternate(tmp);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "'string'");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
-g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S);
-g_assert_cmpstr(tmp->s, ==, "string");
+g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
+g_assert_cmpstr(tmp->u.s, ==, "string");
qapi_free_UserDefAlternate(tmp);
visitor_input_teardown(data, NULL);
@@ -419,8 +419,8 @@ static void
test_visitor_in_alternate_number(TestInputVisitorData *data,
* parse the same as ans */
v = visitor_input_test_init(data, "42");
visit_type_AltStrNum(v, &asn, NULL, &err);
-/* FIXME g_assert_cmpint(asn->kind, == ALT_STR_NUM_KIND_N); */
-/* FIXME g_assert_cmpfloat(asn->n, ==, 42); */
+/* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
+/* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
g_assert(err);
error_free(err);
err = NULL;
@@ -429,29 +429,29 @@ static void
test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_AltNumStr(v, &ans, NULL, &error_abort);
-g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
-g_assert_cmpfloat(ans->n, ==, 42);
+g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
+g_assert_cmpfloat(ans->u.n, ==, 42);
qapi_free_AltNumStr(ans);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltStrInt(v, &asi, NULL, &error_abort);
-g_assert_cmpint(asi->kind, ==, ALT_STR_INT_KIND_I);
-g_assert_cmpint(asi->i, ==, 42);
+g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I);
+g_assert_cmpint(asi->u.i, ==, 42);
qapi_free_AltStrInt(asi);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltIntNum(v, &ain, NULL, &error_abort);
-g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_I);
-g_assert_cmpint(ain->i, ==, 42);
+g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I);
+g_assert_cmpint(ain->u.i, ==, 42);
qapi_free_AltIntNum(ain);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltNumInt(v, &ani, NULL, &error_abort);
-g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_I);
-g_assert_cmpint(ani->i, ==, 42);
+g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I);
+g_assert_cmpint(ani->u.i, ==, 42);
qapi_free_AltNumInt(ani);
visitor_input_teardown(data, N
[Qemu-devel] [PATCH v10 07/25] qapi-visit: Split off visit_type_FOO_fields forward decl
We generate a static visit_type_FOO_fields() for every type
FOO. However, sometimes we need a forward declaration. Split
the code to generate the forward declaration out of
gen_visit_implicit_struct() into a new gen_visit_fields_decl(),
and also prepare for a forward declaration to be emitted
during gen_visit_struct(), so that a future patch can switch
from using visit_type_FOO_implicit() to the simpler
visit_type_FOO_fields() as part of unboxing the base class
of a struct.
No change to generated code.
Signed-off-by: Eric Blake
---
v10: new patch, split from 5/17
---
scripts/qapi-visit.py | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index e878018..7204ed0 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,7 +15,12 @@
from qapi import *
import re
+# visit_type_FOO_implicit() is emitted as needed; track if it has already
+# been output. No forward declaration is needed.
implicit_structs_seen = set()
+
+# visit_type_FOO_fields() is always emitted; track if a forward declaration
+# or implementation has already been output.
struct_fields_seen = set()
@@ -29,19 +34,24 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const
char *name, Error **
c_name=c_name(name), c_type=c_type)
+def gen_visit_fields_decl(typ):
+ret = ''
+if typ.name not in struct_fields_seen:
+ret += mcgen('''
+
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error
**errp);
+''',
+ c_type=typ.c_name())
+struct_fields_seen.add(typ.name)
+return ret
+
+
def gen_visit_implicit_struct(typ):
if typ in implicit_structs_seen:
return ''
implicit_structs_seen.add(typ)
-ret = ''
-if typ.name not in struct_fields_seen:
-# Need a forward declaration
-ret += mcgen('''
-
-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error
**errp);
-''',
- c_type=typ.c_name())
+ret = gen_visit_fields_decl(typ)
ret += mcgen('''
@@ -62,13 +72,12 @@ static void visit_type_implicit_%(c_type)s(Visitor *v,
%(c_type)s **obj, Error *
def gen_visit_struct_fields(name, base, members):
-struct_fields_seen.add(name)
-
ret = ''
if base:
ret += gen_visit_implicit_struct(base)
+struct_fields_seen.add(name)
ret += mcgen('''
static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error
**errp)
@@ -100,7 +109,11 @@ out:
def gen_visit_struct(name, base, members):
-ret = gen_visit_struct_fields(name, base, members)
+ret = ''
+if base:
+ret += gen_visit_fields_decl(base)
+
+ret += gen_visit_struct_fields(name, base, members)
# FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
# *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
--
2.4.3
[Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes
A previous patch (commit 1e6c1616) made it possible to
directly cast from a qapi type to its base type. A future
patch will do likewise for structs. However, it requires
the client code to use a C cast, which turns off compiler
type-safety checks if the client gets it wrong. So this
patch adds inline type-safe wrappers named qapi_FOO_base()
for any type FOO that has a base, which can be used to
upcast a qapi type to its base, then uses the new generated
functions in places where we were already casting.
Some of the ugliness of this patch will disappear once
structs are laid out in the same manner as unions; mark
it with TODO for now.
Note that C makes const-correct upcasts annoying because
it lacks overloads; these functions cast away const so that
they can accept user pointers whether const or not, and the
result in turn can be assigned to normal or const pointers.
Alternatively, this could have been done with macros, but
those tend to not have quite as much type safety.
This patch just adds upcasts. None of our code needed to
downcast from a base qapi class to a child. Also, in the
case of grandchildren (such as BlockdevOptionsQcow2), the
caller will need to call two functions to get to the inner
base (although it wouldn't be too hard to generate a
qapi_FOO_base_base() if desired). If a user changes qapi
to alter the base class hierarchy, such as going from
'A -> C' to 'A -> B -> C', it will change the type of
'qapi_C_base()', and the compiler will point out the places
that are affected by the new base.
One alternative was proposed, but was deemed too ugly to use
in practice: the generators could output redundant
information using anonymous types:
| struct Child {
| union {
| struct {
| Type1 parent_member1;
| Type2 parent_member2;
| };
| Parent base;
| };
| };
With that ugly proposal, for a given qapi type, obj->member
and obj->base.member would refer to the same storage; allowing
convenience in working with members without needing 'base.'
allowing typesafe upcast without needing a C cast by accessing
'&obj->base', and allowing downcasts from the parent back to
the child possible through container_of(obj, Child, base).
Signed-off-by: Eric Blake
---
v10: new patch
---
scripts/qapi-types.py | 20
ui/spice-core.c | 14 ++
ui/vnc.c | 9 ++---
3 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index a97cc10..ed4a11c 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -97,6 +97,23 @@ struct %(c_name)s {
return ret
+def gen_upcast(name, base, struct):
+# C makes const-correctness ugly. We have to cast away const to let
+# this function work for both const and non-const obj.
+# TODO Ugly difference between struct and flat union bases
+member = ''
+if struct:
+member = '->base'
+return mcgen('''
+
+static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
+{
+return (%(base)s *)obj%(member)s;
+}
+''',
+ c_name=c_name(name), base=base.c_name(), member=member)
+
+
def gen_alternate_qtypes_decl(name):
return mcgen('''
@@ -268,6 +285,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.decl += gen_union(name, base, variants)
else:
self.decl += gen_struct(name, base, members)
+if base:
+# TODO Drop last param once structs have sane layout
+self.decl += gen_upcast(name, base, not variants)
self._gen_type_cleanup(name)
def visit_alternate_type(self, name, info, variants):
diff --git a/ui/spice-core.c b/ui/spice-core.c
index bf4fd07..d43cfbe 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -218,9 +218,11 @@ static void channel_event(int event, SpiceChannelEventInfo
*info)
}
if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
-add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
+add_addr_info(qapi_SpiceChannel_base(client),
+ (struct sockaddr *)&info->paddr_ext,
info->plen_ext);
-add_addr_info(server->base, (struct sockaddr *)&info->laddr_ext,
+add_addr_info(qapi_SpiceServerInfo_base(server),
+ (struct sockaddr *)&info->laddr_ext,
info->llen_ext);
} else {
error_report("spice: %s, extended address is expected",
@@ -229,7 +231,9 @@ static void channel_event(int event, SpiceChannelEventInfo
*info)
switch (event) {
case SPICE_CHANNEL_EVENT_CONNECTED:
-qapi_event_send_spice_connected(server->base, client->base,
&error_abort);
+qapi_event_send_spice_connected(qapi_SpiceServerInfo_base(server),
+qapi_SpiceChannel_base(client),
+&error_abort);
break;
case SPICE_CHANNEL_EVENT_INITIALIZ
[Qemu-devel] [PATCH v10 22/25] qapi: Finish converting to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
This patch is the back end for a series that converts to a
saner qapi union layout. Now that all clients have been
converted to use 'type' and 'obj->u.value', we can drop the
temporary parallel support for 'kind' and 'obj->value'.
Given a simple union qapi type:
{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
this is the overall effect, when compared to the state before
this series of patches:
| struct Foo {
|-FooKind kind;
|-union { /* union tag is @kind */
|+FooKind type;
|+union { /* union tag is @type */
| void *data;
| int64_t a;
| bool b;
|-};
|+} u;
| };
Note, however, that we do not rename the generated enum, which
is still 'FooKind'. A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.
Signed-off-by: Eric Blake
---
v10: rebase to earlier changes, match commit wording
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
scripts/qapi-types.py | 26 +-
1 file changed, 5 insertions(+), 21 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 579532d..d131430 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -148,23 +148,10 @@ struct %(c_name)s {
if base:
ret += gen_struct_fields([], base)
else:
-# TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
-# want to use only 'type', but the conversion is large enough to
-# require staging over several commits.
-ret += mcgen('''
-union {
-%(c_type)s kind;
-%(c_type)s type;
-};
-''',
- c_type=c_name(variants.tag_member.type.name))
+ret += gen_struct_field(variants.tag_member.name,
+variants.tag_member.type,
+False)
-# TODO As a hack, we emit the union twice, once as an anonymous union
-# and once as a named union. Ultimately, we want to use only the
-# named union version (as it avoids conflicts between tag values as
-# branch names competing with non-variant QMP names), but the conversion
-# is large enough to require staging over several commits.
-tmp = ''
# FIXME: What purpose does data serve, besides preventing a union that
# has a branch named 'data'? We use it in qapi-visit.py to decide
# whether to bypass the switch statement if visiting the discriminator
@@ -173,7 +160,7 @@ struct %(c_name)s {
# should not be any data leaks even without a data pointer. Or, if
# 'data' is merely added to guarantee we don't have an empty union,
# shouldn't we enforce that at .json parse time?
-tmp += mcgen('''
+ret += mcgen('''
union { /* union tag is @%(c_name)s */
void *data;
''',
@@ -182,17 +169,14 @@ struct %(c_name)s {
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
typ = var.simple_union_type() or var.type
-tmp += mcgen('''
+ret += mcgen('''
%(c_type)s %(c_name)s;
''',
c_type=typ.c_type(),
c_name=c_name(var.name))
-ret += tmp
-ret += '' + '\n'.join(tmp.split('\n'))
ret += mcgen('''
} u;
-};
};
''')
--
2.4.3
[Qemu-devel] [PATCH v10 23/25] qapi: Reserve 'u' member name
Now that we have separated union tag values from colliding with
non-variant C names, by naming the union 'u', we should reserve
this name for our use. Note that we want to forbid 'u' even in
a struct with no variants, because it is possible for a future
qemu release to extend QMP in a backwards-compatible manner while
converting from a struct to a flat union. Fortunately, no
existing clients were using this member name. If we ever find
the need for QMP to have a member 'u', we could at that time
relax things, perhaps by having c_name() munge the QMP member to
'q_u'.
Note that we cannot forbid 'u' everywhere (by adding the
rejection code to check_name()), because the existing QKeyCode
enum already uses it. And it is still okay to use it as a
branch name within a union, although not worth adding a
positive test to qapi-schema-test for this corner case.
Signed-off-by: Eric Blake
---
v10: new patch, split from 1/17 and 3/17; by deferring this until
after the rename was complete, it reduces churn due to the new feature
---
scripts/qapi.py| 2 +-
tests/Makefile | 1 +
tests/qapi-schema/struct-member-u.err | 1 +
tests/qapi-schema/struct-member-u.exit | 1 +
tests/qapi-schema/struct-member-u.json | 7 +++
tests/qapi-schema/struct-member-u.out | 0
6 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/struct-member-u.err
create mode 100644 tests/qapi-schema/struct-member-u.exit
create mode 100644 tests/qapi-schema/struct-member-u.json
create mode 100644 tests/qapi-schema/struct-member-u.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8b9f5f7..b941ccf 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -490,7 +490,7 @@ def check_type(expr_info, source, value, allow_array=False,
for (key, arg) in value.items():
check_name(expr_info, "Member of %s" % source, key,
allow_optional=allow_optional)
-if c_name(key).startswith('has_'):
+if key == 'u' or c_name(key).startswith('has_'):
raise QAPIExprError(expr_info,
"Member of %s uses reserved name '%s'"
% (source, key))
diff --git a/tests/Makefile b/tests/Makefile
index 9194264..8c239f8 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -326,6 +326,7 @@ qapi-schema += struct-base-clash-deep.json
qapi-schema += struct-base-clash.json
qapi-schema += struct-data-invalid.json
qapi-schema += struct-member-invalid.json
+qapi-schema += struct-member-u.json
qapi-schema += struct-name-list.json
qapi-schema += trailing-comma-list.json
qapi-schema += trailing-comma-object.json
diff --git a/tests/qapi-schema/struct-member-u.err
b/tests/qapi-schema/struct-member-u.err
new file mode 100644
index 000..0dc354c
--- /dev/null
+++ b/tests/qapi-schema/struct-member-u.err
@@ -0,0 +1 @@
+tests/qapi-schema/struct-member-u.json:7: Member of 'data' for struct 'Oops'
uses reserved name 'u'
diff --git a/tests/qapi-schema/struct-member-u.exit
b/tests/qapi-schema/struct-member-u.exit
new file mode 100644
index 000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/struct-member-u.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/struct-member-u.json
b/tests/qapi-schema/struct-member-u.json
new file mode 100644
index 000..1eaf0f3
--- /dev/null
+++ b/tests/qapi-schema/struct-member-u.json
@@ -0,0 +1,7 @@
+# Potential C member name collision
+# We reject use of 'u' as a member name, to allow it for internal use in
+# putting union branch members in a separate namespace from QMP members.
+# This is true even for non-unions, because it is possible to convert a
+# struct to flat union while remaining backwards compatible in QMP.
+# TODO - we could munge the member name to 'q_u' to avoid the collision
+{ 'struct': 'Oops', 'data': { 'u': 'str' } }
diff --git a/tests/qapi-schema/struct-member-u.out
b/tests/qapi-schema/struct-member-u.out
new file mode 100644
index 000..e69de29
--
2.4.3
[Qemu-devel] [PATCH v10 04/25] qapi: Reserve '*List' type names for list types
Type names ending in 'List' can clash with qapi list types in
generated C. We don't currently use such names. It is easier to
outlaw them now than to worry about how to resolve such a clash
in the future. For precedence, see commit 4dc2e69, which did the
same for names ending in 'Kind' versus implicit enum types for
qapi unions.
Note that this only forbids explicit creation of new (non-array)
types or commands ending in 'List'; the parser already rejects
attempts to use ['intList'] as the type for an object member
(although 'intList' happens to be the C spelling for the qapi
type ['int'], that does not make it a recognized name in qapi).
Whether we later turn on support for multi-dimensional arrays
(via [['int']], not ['intList']) is a design decision best left
for another day.
Update the testsuite to match.
Signed-off-by: Eric Blake
---
v10: improve commit message, including a retitle (qapi list types
may be JSON arrays, but they are C linked lists, not C arrays)
v9: new patch
---
docs/qapi-code-gen.txt | 3 ++-
scripts/qapi.py | 10 --
tests/qapi-schema/struct-name-list.err | 1 +
tests/qapi-schema/struct-name-list.exit | 2 +-
tests/qapi-schema/struct-name-list.json | 6 +++---
tests/qapi-schema/struct-name-list.out | 3 ---
6 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 2afab20..c4264a8 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -106,7 +106,8 @@ Types, commands, and events share a common namespace.
Therefore,
generally speaking, type definitions should always use CamelCase for
user-defined type names, while built-in types are lowercase. Type
definitions should not end in 'Kind', as this namespace is used for
-creating implicit C enums for visiting union types. Command names,
+creating implicit C enums for visiting union types, or in 'List', as
+this namespace is used for creating array types. Command names,
and field names within a type, should be all lower case with words
separated by a hyphen. However, some existing older commands and
complex types use underscore; when extending such expressions,
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3af4c2c..d53b5c4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -390,10 +390,10 @@ def add_name(name, info, meta, implicit=False):
raise QAPIExprError(info,
"%s '%s' is already defined"
% (all_names[name], name))
-if not implicit and name.endswith('Kind'):
+if not implicit and (name.endswith('Kind') or name.endswith('List')):
raise QAPIExprError(info,
-"%s '%s' should not end in 'Kind'"
-% (meta, name))
+"%s '%s' should not end in '%s'"
+% (meta, name, name[-4:]))
all_names[name] = meta
@@ -1196,9 +1196,7 @@ class QAPISchema(object):
return name
def _make_array_type(self, element_type, info):
-# TODO fooList namespace is not reserved; user can create collisions,
-# or abuse our type system with ['fooList'] for 2D array
-name = element_type + 'List'
+name = element_type + 'List' # Use namespace reserved by add_name()
if not self.lookup_type(name):
self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
diff --git a/tests/qapi-schema/struct-name-list.err
b/tests/qapi-schema/struct-name-list.err
index e69de29..ee5b09b 100644
--- a/tests/qapi-schema/struct-name-list.err
+++ b/tests/qapi-schema/struct-name-list.err
@@ -0,0 +1 @@
+tests/qapi-schema/struct-name-list.json:5: struct 'FooList' should not end in
'List'
diff --git a/tests/qapi-schema/struct-name-list.exit
b/tests/qapi-schema/struct-name-list.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/struct-name-list.exit
+++ b/tests/qapi-schema/struct-name-list.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/struct-name-list.json
b/tests/qapi-schema/struct-name-list.json
index 8ad38e6..98d53bf 100644
--- a/tests/qapi-schema/struct-name-list.json
+++ b/tests/qapi-schema/struct-name-list.json
@@ -1,5 +1,5 @@
# Potential C name collision
-# FIXME - This parses and compiles on its own, but prevents the user from
-# creating a type named 'Foo' and using ['Foo'] for an array. We should
-# reject the use of any non-array type names ending in 'List'.
+# We reserve names ending in 'List' for use by array types.
+# TODO - we could choose array names to avoid collision with user types,
+# in order to let this compile
{ 'struct': 'FooList', 'data': { 's': 'str' } }
diff --git a/tests/qapi-schema/struct-name-list.out
b/tests/qapi-schema/struct-name-list.out
index 0406bfe..e69de29 100644
--- a/tests/qapi-schema/struct-name-list.out
+++ b/tests/qapi-schema/struct-name-list.out
@@ -1,3 +0,0 @@
-object :empty
-object FooList
-me
[Qemu-devel] [PATCH v10 00/25] qapi collision reduction (post-introspection subset B')
No pending prerequisites (applies to current master) Also available as a tag at this location: git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv10b and I plan to eventually forcefully update my branch with the rest of the v5 series, at: http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi v10 notes: Rebase to master, reduce churn in the testsuite (particularly for things that disappear once the named union conversion is complete), and rearrange various hunks either forward or backward. 17 patches turned into 25, because I split things. Also, several patches got retitled, which messes up git backport-diff reporting, but some survived unscathed: 001/25:[down] 'tests/qapi-schema: Test for reserved names, empty struct' 002/25:[down] 'qapi: More idiomatic string operations' 003/25:[down] 'qapi: More robust conditions for when labels are needed' 004/25:[down] 'qapi: Reserve '*List' type names for list types' 005/25:[down] 'qapi: Reserve 'q_*' and 'has_*' member names' 006/25:[down] 'vnc: Hoist allocation of VncBasicInfo to callers' 007/25:[down] 'qapi-visit: Split off visit_type_FOO_fields forward decl' 008/25:[down] 'qapi-types: Refactor base fields output' 009/25:[down] 'qapi: Prefer typesafe upcasts to qapi base classes' 010/25:[0079] [FC] 'qapi: Unbox base members' 011/25:[0005] [FC] 'qapi-visit: Remove redundant functions for flat union base' 012/25:[0032] [FC] 'qapi: Start converting to new qapi union layout' 013/25:[down] 'qapi-visit: Convert to new qapi union layout' 014/25:[0010] [FC] 'tests: Convert to new qapi union layout' 015/25:[] [--] 'block: Convert to new qapi union layout' 016/25:[down] 'sockets: Convert to new qapi union layout' 017/25:[] [--] 'net: Convert to new qapi union layout' 018/25:[] [--] 'char: Convert to new qapi union layout' 019/25:[] [--] 'input: Convert to new qapi union layout' 020/25:[] [--] 'memory: Convert to new qapi union layout' 021/25:[] [--] 'tpm: Convert to new qapi union layout' 022/25:[0008] [FC] 'qapi: Finish converting to new qapi union layout' 023/25:[down] 'qapi: Reserve 'u' member name' 024/25:[down] 'qapi: Remove outdated tests related to QMP/branch collisions' 025/25:[] [-C] 'qapi: Simplify gen_struct_field()' v9 notes: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03730.html Calling this subset B', because it is mostly new content, but based heavily on review comments on subset B. Also, I need to rebase pending subset C on top of this. A couple patches from v5 have been hoisted earlier into the series; while v8 11/18 (Simplify gen_struct_field()) makes an appearance at the end, and v8 10/18 (Move union tag quirks into subclass) is all but eliminated. Since the concepts of these patches has been on the list for a while, it should be safe to take even during soft freeze, even if the actual versions of these patches have lots of new content. The general goal of this batch of patches is to rework the C layout of qapi unions so that we get rid of the type/kind mismatch and isolate tag values from colliding with QMP names. The tail end of v8 (detecting collisions in check()) will have to wait a bit longer, but it should be a lot easier to reason about now that there are fewer collisions possible. Not worth a backport diff, since most of it is new or previously unreviewed. v8 notes: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02879.html Address review comments, including fixing a bug with tracking branch name collisions. Include a few more simple test cleanups (4-6), and add another patch (11) that will make converting from kind=>type easier in a later subset. More details in per-patch changelogs. v7 notes: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01387.html Address comments, including a couple of new commits that made later patches a bit cleaner. Backport diff gets a bit confused by a couple of patch titles changing. v6 notes: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg00562.html This is patches 11-16 of my v5 series; it has grown a bit with splitting some patches and adding some others. I suspect that 12/12 on this series will be discarded, but am including it because it was split from v5 content. Not much review comments other than on the original 11/46, but there is enough churn due to rebasing that it's now easier to review this version than plowing through v5. Subset C (and more?) will come later. In v5: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05410.html I _did_ rearrange patches to try and group related features: 1-2: Groundwork cleanups 3-5: Add more test cases 6-16: Front-end cleanups 17-18: Introspection output cleanups 19-20: 'alternate' type cleanups 21-29: qapi visitor cleanups 30-45: qapi-ify netdev_add 46: add qapi shorthand for flat unions Lots of fixes based on additional testing, and rebased to track other changes that happened in the meantime. The series is huge; I can split off smaller portions as requested. In v4
[Qemu-devel] [PATCH v10 13/25] qapi-visit: Convert to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for qapi-visit.py. Also
make a change in qapi.py to quit using the name 'kind', and
the corresponding change in the testsuite. However, many more
changes will be made to the testsuite later, including the
entire deletion of union-clash-type.json, after the completion
of the conversion eliminates collisions between union tag
branch names and non-variant names.
Signed-off-by: Eric Blake
---
v10: new patch, split from 7/17 an 8/17
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
scripts/qapi-visit.py | 24 +---
scripts/qapi.py| 2 +-
tests/qapi-schema/union-clash-type.err | 2 +-
3 files changed, 11 insertions(+), 17 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3c23c53..421fcb6 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -187,18 +187,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
if (err) {
goto out;
}
-visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name,
&err);
+visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name,
&err);
if (err) {
goto out_obj;
}
-switch ((*obj)->kind) {
+switch ((*obj)->type) {
''',
c_name=c_name(name))
for var in variants.variants:
ret += mcgen('''
case %(case)s:
-visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err);
+visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
break;
''',
case=c_enum_const(variants.tag_member.type.name,
@@ -259,22 +259,16 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
''',
c_type=variants.tag_member.type.c_name(),
- # TODO ugly special case for simple union
- # Use same tag name in C as on the wire to get rid of
- # it, then: c_name=c_name(variants.tag_member.name)
- c_name='kind',
+ c_name=c_name(variants.tag_member.name),
name=variants.tag_member.name)
ret += gen_err_check(label='out_obj')
ret += mcgen('''
-if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
+if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
goto out_obj;
}
switch ((*obj)->%(c_name)s) {
''',
- # TODO ugly special case for simple union
- # Use same tag name in C as on the wire to get rid of
- # it, then: c_name=c_name(variants.tag_member.name)
- c_name=c_name(variants.tag_name or 'kind'))
+ c_name=c_name(variants.tag_member.name))
for var in variants.variants:
# TODO ugly special case for simple union
@@ -286,13 +280,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
var.name))
if simple_union_type:
ret += mcgen('''
-visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
+visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, "data", &err);
''',
c_type=simple_union_type.c_name(),
c_name=c_name(var.name))
else:
ret += mcgen('''
-visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
+visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
@@ -308,7 +302,7 @@ out_obj:
error_propagate(errp, err);
err = NULL;
if (*obj) {
-visit_end_union(v, !!(*obj)->data, &err);
+visit_end_union(v, !!(*obj)->u.data, &err);
}
error_propagate(errp, err);
err = NULL;
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 04bcbf7..8b9f5f7 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -548,7 +548,7 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
-values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
+values = {'MAX': '(automatic)', 'TYPE': '(automatic tag)'}
# Two types of unions, determined by discriminator.
diff --git a/tests/qapi-schema/union-clash-type.err
b/tests/qapi-schema/union-clash-type.err
index a5dead1..
[Qemu-devel] [PATCH v10 11/25] qapi-visit: Remove redundant functions for flat union base
The code for visiting the base class of a child struct created
visit_type_Base_fields() which covers all fields of Base; while
the code for visiting the base class of a flat union created
visit_type_Union_fields() covering all fields of the base
except the discriminator. But since the base class includes
the discriminator of a flat union, we can just visit the entire
base, without needing a separate visit of the discriminator.
Not only is consistently visiting all fields easier to
understand, it lets us share code.
The generated code in qapi-visit.c loses several now-unused
visit_type_UNION_fields(), along with changes like:
|@@ -1654,11 +1557,7 @@ void visit_type_BlockdevOptions(Visitor
| if (!*obj) {
| goto out_obj;
| }
|-visit_type_BlockdevOptions_fields(v, obj, &err);
|-if (err) {
|-goto out_obj;
|-}
|-visit_type_BlockdevDriver(v, &(*obj)->driver, "driver", &err);
|+visit_type_BlockdevOptionsBase_fields(v, (BlockdevOptionsBase **)obj,
&err);
| if (err) {
| goto out_obj;
| }
Signed-off-by: Eric Blake
---
v10: split out regex to earlier commit, rebase to earlier changes,
improve commit message
v9: (no v6-8): hoist from v5 35/46; rebase to master; fix indentation
botch in gen_visit_union(); polish commit message
---
scripts/qapi-visit.py | 32 +++-
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index e30062b..3c23c53 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -226,8 +226,7 @@ def gen_visit_union(name, base, variants):
ret = ''
if base:
-members = [m for m in base.members if m != variants.tag_member]
-ret += gen_visit_struct_fields(name, None, members)
+ret += gen_visit_fields_decl(base)
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
@@ -252,31 +251,30 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
if base:
ret += mcgen('''
-visit_type_%(c_name)s_fields(v, obj, &err);
+visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
''',
- c_name=c_name(name))
-ret += gen_err_check(label='out_obj')
-
-tag_key = variants.tag_member.name
-if not variants.tag_name:
-# we pointlessly use a different key for simple unions
-tag_key = 'type'
-ret += mcgen('''
+ c_name=base.c_name())
+else:
+ret += mcgen('''
visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
-if (err) {
-goto out_obj;
-}
+''',
+ c_type=variants.tag_member.type.c_name(),
+ # TODO ugly special case for simple union
+ # Use same tag name in C as on the wire to get rid of
+ # it, then: c_name=c_name(variants.tag_member.name)
+ c_name='kind',
+ name=variants.tag_member.name)
+ret += gen_err_check(label='out_obj')
+ret += mcgen('''
if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
goto out_obj;
}
switch ((*obj)->%(c_name)s) {
''',
- c_type=variants.tag_member.type.c_name(),
# TODO ugly special case for simple union
# Use same tag name in C as on the wire to get rid of
# it, then: c_name=c_name(variants.tag_member.name)
- c_name=c_name(variants.tag_name or 'kind'),
- name=tag_key)
+ c_name=c_name(variants.tag_name or 'kind'))
for var in variants.variants:
# TODO ugly special case for simple union
--
2.4.3
[Qemu-devel] [PATCH v10 12/25] qapi: Start converting to new qapi union layout
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
This patch is the front end for a series that converts to a
saner qapi union layout. By the end of the series, we will no
longer have the type/kind mismatch, and all tag values will be
under a named union, which requires clients to access
'obj->u.value' instead of 'obj->value'. But since the
conversion touches a number of files, it is easiest if we
temporarily support BOTH layouts simultaneously.
Given a simple union qapi type:
{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
make the following changes in generated qapi-types.h:
| struct Foo {
|-FooKind kind;
|-union { /* union tag is @kind */
|+union {
|+FooKind kind;
|+FooKind type;
|+};
|+union { /* union tag is @type */
| void *data;
| int64_t a;
| bool b;
|+union { /* union tag is @type */
|+void *data;
|+int64_t a;
|+bool b;
|+} u;
| };
| };
Flat unions do not need the anonymous union for the tag member,
as we already fixed that to use the member name instead of 'kind'
back in commit 0f61af3e.
Later, when the conversions are complete, we will remove the
duplication hacks.
Note, however, that we do not rename the generated enum, which
is still 'FooKind'. A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.
Signed-off-by: Eric Blake
---
v10: rebase to earlier changes, improve commit message, defer
qapi-visit and testsuite 'u' changes to later
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
scripts/qapi-types.py | 26 +++---
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9c51d8f..579532d 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -148,11 +148,23 @@ struct %(c_name)s {
if base:
ret += gen_struct_fields([], base)
else:
+# TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
+# want to use only 'type', but the conversion is large enough to
+# require staging over several commits.
ret += mcgen('''
-%(c_type)s kind;
+union {
+%(c_type)s kind;
+%(c_type)s type;
+};
''',
c_type=c_name(variants.tag_member.type.name))
+# TODO As a hack, we emit the union twice, once as an anonymous union
+# and once as a named union. Ultimately, we want to use only the
+# named union version (as it avoids conflicts between tag values as
+# branch names competing with non-variant QMP names), but the conversion
+# is large enough to require staging over several commits.
+tmp = ''
# FIXME: What purpose does data serve, besides preventing a union that
# has a branch named 'data'? We use it in qapi-visit.py to decide
# whether to bypass the switch statement if visiting the discriminator
@@ -161,25 +173,25 @@ struct %(c_name)s {
# should not be any data leaks even without a data pointer. Or, if
# 'data' is merely added to guarantee we don't have an empty union,
# shouldn't we enforce that at .json parse time?
-ret += mcgen('''
+tmp += mcgen('''
union { /* union tag is @%(c_name)s */
void *data;
''',
- # TODO ugly special case for simple union
- # Use same tag name in C as on the wire to get rid of
- # it, then: c_name=c_name(variants.tag_member.name)
- c_name=c_name(variants.tag_name or 'kind'))
+ c_name=c_name(variants.tag_member.name))
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
typ = var.simple_union_type() or var.type
-ret += mcgen('''
+tmp += mcgen('''
%(c_type)s %(c_name)s;
''',
c_type=typ.c_type(),
c_name=c_name(var.name))
+ret += tmp
+ret += '' + '\n'.join(tmp.split('\n'))
ret += mcgen('''
+} u;
};
};
''')
--
2.4.3
[Qemu-devel] [PATCH v10 08/25] qapi-types: Refactor base fields output
Move code from gen_union() into gen_struct_fields() in order for
a later patch to share code when enumerating inherited fields
for struct types.
No change to generated code.
Signed-off-by: Eric Blake
---
v10: new patch, split off of 5/17
---
scripts/qapi-types.py | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4fe618e..a97cc10 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -51,9 +51,19 @@ def gen_struct_field(name, typ, optional):
return ret
-def gen_struct_fields(members):
+def gen_struct_fields(members, base=None):
ret = ''
+if base:
+ret += mcgen('''
+/* Members inherited from %(c_name)s: */
+''',
+ c_name=base.c_name())
+ret += gen_struct_fields(base.members)
+ret += mcgen('''
+/* Own members: */
+''')
+
for memb in members:
ret += gen_struct_field(memb.name, memb.type, memb.optional)
return ret
@@ -126,14 +136,7 @@ struct %(c_name)s {
''',
c_name=c_name(name))
if base:
-ret += mcgen('''
-/* Members inherited from %(c_name)s: */
-''',
- c_name=c_name(base.name))
-ret += gen_struct_fields(base.members)
-ret += mcgen('''
-/* Own members: */
-''')
+ret += gen_struct_fields([], base)
else:
ret += mcgen('''
%(c_type)s kind;
--
2.4.3
[Qemu-devel] [PATCH v10 06/25] vnc: Hoist allocation of VncBasicInfo to callers
A future qapi patch will rework generated structs with a base
class to be unboxed. In preparation for that, change the code
that allocates then populates an info struct to instead merely
populate the fields of an info field passed in as a parameter
(renaming vnc_basic_info_get* to vnc_init_basic_info*). Add
rudimentary Error handling at the lowest levels for cases
where the old code returned NULL; but rather than plumb Error
all the way through the stack, the callers drop the error and
return NULL as before.
Signed-off-by: Eric Blake
---
v10: improve commit message (including a retitle), rename
functions, tweak error message, don't change semantics of
vnc_server_info_get()
v9: (no v6-8): hoist from v5 33/46
---
ui/vnc.c | 57 ++---
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index faff054..502a10a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -156,10 +156,11 @@ char *vnc_socket_remote_addr(const char *format, int fd) {
return addr_to_string(format, &sa, salen);
}
-static VncBasicInfo *vnc_basic_info_get(struct sockaddr_storage *sa,
-socklen_t salen)
+static void vnc_init_basic_info(struct sockaddr_storage *sa,
+socklen_t salen,
+VncBasicInfo *info,
+Error **errp)
{
-VncBasicInfo *info;
char host[NI_MAXHOST];
char serv[NI_MAXSERV];
int err;
@@ -168,42 +169,44 @@ static VncBasicInfo *vnc_basic_info_get(struct
sockaddr_storage *sa,
host, sizeof(host),
serv, sizeof(serv),
NI_NUMERICHOST | NI_NUMERICSERV)) != 0) {
-VNC_DEBUG("Cannot resolve address %d: %s\n",
- err, gai_strerror(err));
-return NULL;
+error_setg(errp, "Cannot resolve address: %s",
+ gai_strerror(err));
+return;
}
-info = g_malloc0(sizeof(VncBasicInfo));
info->host = g_strdup(host);
info->service = g_strdup(serv);
info->family = inet_netfamily(sa->ss_family);
-return info;
}
-static VncBasicInfo *vnc_basic_info_get_from_server_addr(int fd)
+static void vnc_init_basic_info_from_server_addr(int fd, VncBasicInfo *info,
+ Error **errp)
{
struct sockaddr_storage sa;
socklen_t salen;
salen = sizeof(sa);
if (getsockname(fd, (struct sockaddr*)&sa, &salen) < 0) {
-return NULL;
+error_setg_errno(errp, errno, "getsockname failed");
+return;
}
-return vnc_basic_info_get(&sa, salen);
+vnc_init_basic_info(&sa, salen, info, errp);
}
-static VncBasicInfo *vnc_basic_info_get_from_remote_addr(int fd)
+static void vnc_init_basic_info_from_remote_addr(int fd, VncBasicInfo *info,
+ Error **errp)
{
struct sockaddr_storage sa;
socklen_t salen;
salen = sizeof(sa);
if (getpeername(fd, (struct sockaddr*)&sa, &salen) < 0) {
-return NULL;
+error_setg_errno(errp, errno, "getpeername failed");
+return;
}
-return vnc_basic_info_get(&sa, salen);
+vnc_init_basic_info(&sa, salen, info, errp);
}
static const char *vnc_auth_name(VncDisplay *vd) {
@@ -256,15 +259,18 @@ static const char *vnc_auth_name(VncDisplay *vd) {
static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
{
VncServerInfo *info;
-VncBasicInfo *bi = vnc_basic_info_get_from_server_addr(vd->lsock);
-if (!bi) {
-return NULL;
-}
+Error *err = NULL;
info = g_malloc(sizeof(*info));
-info->base = bi;
+info->base = g_malloc(sizeof(*info->base));
+vnc_init_basic_info_from_server_addr(vd->lsock, info->base, &err);
info->has_auth = true;
info->auth = g_strdup(vnc_auth_name(vd));
+if (err) {
+qapi_free_VncServerInfo(info);
+info = NULL;
+error_free(err);
+}
return info;
}
@@ -291,11 +297,16 @@ static void vnc_client_cache_auth(VncState *client)
static void vnc_client_cache_addr(VncState *client)
{
-VncBasicInfo *bi = vnc_basic_info_get_from_remote_addr(client->csock);
+Error *err = NULL;
-if (bi) {
-client->info = g_malloc0(sizeof(*client->info));
-client->info->base = bi;
+client->info = g_malloc0(sizeof(*client->info));
+client->info->base = g_malloc0(sizeof(*client->info->base));
+vnc_init_basic_info_from_remote_addr(client->csock, client->info->base,
+ &err);
+if (err) {
+qapi_free_VncClientInfo(client->info);
+client->info = NULL;
+error_free(err);
}
}
--
2.4.3
[Qemu-devel] [PATCH v10 05/25] qapi: Reserve 'q_*' and 'has_*' member names
c_name() produces names starting with 'q_' when protecting
a QMP member name that would fail to directly compile, but
in doing so can cause clashes with any QMP name already
beginning with 'q-' or 'q_'. Likewise, we create a C name
'has_' for any optional member, that can clash with any QMP
name beginning with 'has-' or 'has_'.
Technically, rather than blindly reserving the namespace,
we could try to complain about user names only when an actual
collision occurs, or even teach c_name() how to munge names
to avoid collisions. But it is not trivial, especially when
collisions can occur across multiple types (such as via
inheritance or flat unions). Besides, no existing .json
files are trying to use these names. So it's easier to just
outright forbid the potential for collision. We can always
relax things in the future if a real need arises for QMP to
express member names that have been forbidden here.
'has_' only has to be reserved for struct/union member names,
while 'q_' is reserved everywhere (matching the fact that
we only have optional members, but use c_name() everywhere).
Update and add tests to cover the new error messages.
Signed-off-by: Eric Blake
---
v10: retitle, improve comments, defer 'u' changes for later, use
c_name(name).startswith('has_') rather than open coding
v9: new patch
---
docs/qapi-code-gen.txt | 11 +++
scripts/qapi.py | 8 +++-
tests/qapi-schema/args-has-clash.err| 1 +
tests/qapi-schema/args-has-clash.exit | 2 +-
tests/qapi-schema/args-has-clash.json | 8
tests/qapi-schema/args-has-clash.out| 6 --
tests/qapi-schema/reserved-command.err | 1 +
tests/qapi-schema/reserved-command.exit | 2 +-
tests/qapi-schema/reserved-command.json | 7 +++
tests/qapi-schema/reserved-command.out | 5 -
tests/qapi-schema/reserved-member.err | 1 +
tests/qapi-schema/reserved-member.exit | 2 +-
tests/qapi-schema/reserved-member.json | 7 +++
tests/qapi-schema/reserved-member.out | 4
14 files changed, 30 insertions(+), 35 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index c4264a8..4d8c2fc 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -112,7 +112,9 @@ and field names within a type, should be all lower case
with words
separated by a hyphen. However, some existing older commands and
complex types use underscore; when extending such expressions,
consistency is preferred over blindly avoiding underscore. Event
-names should be ALL_CAPS with words separated by underscore.
+names should be ALL_CAPS with words separated by underscore. Field
+names cannot start with 'has-' or 'has_', as this is reserved for
+tracking optional fields.
Any name (command, event, type, field, or enum value) beginning with
"x-" is marked experimental, and may be withdrawn or changed
@@ -123,9 +125,10 @@ vendor), even if the rest of the name uses dash (example:
__com.redhat_drive-mirror). Other than downstream extensions (with
leading underscore and the use of dots), all names should begin with a
letter, and contain only ASCII letters, digits, dash, and underscore.
-It is okay to reuse names that match C keywords; the generator will
-rename a field named "default" in the QAPI to "q_default" in the
-generated C code.
+Names beginning with 'q_' are reserved for the generator: QMP names
+that resemble C keywords or other problematic strings will be munged
+in C to use this prefix. For example, a field named "default" in
+qapi becomes "q_default" in the generated C code.
In the rest of this document, usage lines are given for each
expression type, with literal strings written in lower case and
diff --git a/scripts/qapi.py b/scripts/qapi.py
index d53b5c4..04bcbf7 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -376,7 +376,9 @@ def check_name(expr_info, source, name,
allow_optional=False,
# code always prefixes it with the enum name
if enum_member:
membername = '_' + membername
-if not valid_name.match(membername):
+# Reserve the entire 'q_' namespace for c_name()
+if not valid_name.match(membername) or \
+ membername.replace('-', '_').startswith('q_'):
raise QAPIExprError(expr_info,
"%s uses invalid name '%s'" % (source, name))
@@ -488,6 +490,10 @@ def check_type(expr_info, source, value, allow_array=False,
for (key, arg) in value.items():
check_name(expr_info, "Member of %s" % source, key,
allow_optional=allow_optional)
+if c_name(key).startswith('has_'):
+raise QAPIExprError(expr_info,
+"Member of %s uses reserved name '%s'"
+% (source, key))
# Todo: allow dictionaries to represent default values of
# an optional argument.
check_type(expr_info, "Member '%s' of %s" % (key, source), arg,
diff --git a/tests/qapi-schema
[Qemu-devel] [PATCH v10 01/25] tests/qapi-schema: Test for reserved names, empty struct
We are failing to detect a collision between a QMP member and the implicit 'has_*' flag for another optional QMP member. The easiest fix would be for a future patch to reserve the entire "has[-_]" namespace for member names (the collision is also possible for branch names within flat unions, but only as long as branch names can collide with QMP names; since future patches are about to remove that, it is not worth testing here). A similar collision exists between a QMP member where c_name() munges what might otherwise be a reserved name to start with 'q_', and another member explicitly starts with "q[-_]". Again, the easiest task for a future patch will be reserving the entire namespace, but here for commands as well as members. Our current representation of qapi arrays is done by appending 'List' to the element name; but we are not preventing the creation of a non-array type with the same name. Finally, our testsuite does not have any compilation coverage of struct inheritance with empty qapi structs. Add tests to cover these issues. On the other hand, there is currently no technical reason to forbid type name patterns from member names, or member name patterns from types, since the two are not in the same namespace in C and won't collide (but it's not worth adding positive tests of these corner cases, especially while there is other churn pending in patches that rearrange which collisions actually happen). Signed-off-by: Eric Blake --- v10: retitle, split off 'u' collisions and positive tests for later, add 'q_' collisions and empty struct inheritance, improve commit message, rename args-name-has to args-has-clash v9: new patch --- tests/Makefile | 4 tests/qapi-schema/args-has-clash.err| 0 tests/qapi-schema/args-has-clash.exit | 1 + tests/qapi-schema/args-has-clash.json | 6 ++ tests/qapi-schema/args-has-clash.out| 6 ++ tests/qapi-schema/qapi-schema-test.json | 4 tests/qapi-schema/qapi-schema-test.out | 3 +++ tests/qapi-schema/reserved-command.err | 0 tests/qapi-schema/reserved-command.exit | 1 + tests/qapi-schema/reserved-command.json | 7 +++ tests/qapi-schema/reserved-command.out | 5 + tests/qapi-schema/reserved-member.err | 0 tests/qapi-schema/reserved-member.exit | 1 + tests/qapi-schema/reserved-member.json | 6 ++ tests/qapi-schema/reserved-member.out | 4 tests/qapi-schema/struct-name-list.err | 0 tests/qapi-schema/struct-name-list.exit | 1 + tests/qapi-schema/struct-name-list.json | 5 + tests/qapi-schema/struct-name-list.out | 3 +++ 19 files changed, 57 insertions(+) create mode 100644 tests/qapi-schema/args-has-clash.err create mode 100644 tests/qapi-schema/args-has-clash.exit create mode 100644 tests/qapi-schema/args-has-clash.json create mode 100644 tests/qapi-schema/args-has-clash.out create mode 100644 tests/qapi-schema/reserved-command.err create mode 100644 tests/qapi-schema/reserved-command.exit create mode 100644 tests/qapi-schema/reserved-command.json create mode 100644 tests/qapi-schema/reserved-command.out create mode 100644 tests/qapi-schema/reserved-member.err create mode 100644 tests/qapi-schema/reserved-member.exit create mode 100644 tests/qapi-schema/reserved-member.json create mode 100644 tests/qapi-schema/reserved-member.out create mode 100644 tests/qapi-schema/struct-name-list.err create mode 100644 tests/qapi-schema/struct-name-list.exit create mode 100644 tests/qapi-schema/struct-name-list.json create mode 100644 tests/qapi-schema/struct-name-list.out diff --git a/tests/Makefile b/tests/Makefile index 0531b30..cc6b24f 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -237,6 +237,7 @@ qapi-schema += args-alternate.json qapi-schema += args-any.json qapi-schema += args-array-empty.json qapi-schema += args-array-unknown.json +qapi-schema += args-has-clash.json qapi-schema += args-int.json qapi-schema += args-invalid.json qapi-schema += args-member-array-bad.json @@ -314,6 +315,8 @@ qapi-schema += redefined-builtin.json qapi-schema += redefined-command.json qapi-schema += redefined-event.json qapi-schema += redefined-type.json +qapi-schema += reserved-command.json +qapi-schema += reserved-member.json qapi-schema += returns-alternate.json qapi-schema += returns-array-bad.json qapi-schema += returns-dict.json @@ -324,6 +327,7 @@ qapi-schema += struct-base-clash-deep.json qapi-schema += struct-base-clash.json qapi-schema += struct-data-invalid.json qapi-schema += struct-member-invalid.json +qapi-schema += struct-name-list.json qapi-schema += trailing-comma-list.json qapi-schema += trailing-comma-object.json qapi-schema += type-bypass-bad-gen.json diff --git a/tests/qapi-schema/args-has-clash.err b/tests/qapi-schema/args-has-clash.err new file mode 100644 index 000..e69de29 diff --git a/tests/qapi-schema/args-has-clash.exit b/tests/qapi-schema/args-has-clash.exit new file mode 100644 index 000..573541a --- /dev/null +++ b/tests/
[Qemu-devel] [PATCH v10 03/25] qapi: More robust conditions for when labels are needed
We were using regular expressions to see if ret included
any earlier text that emitted a 'goto out;' line, to decide
whether we needed to output an 'out:' label. But this is
fragile, if the ret text can possibly combine more than one
generated function body, where the first function used a
goto but the second does not. Change the code to just check
for the known conditions which cause an error check to be
needed. Besides, it's slightly more efficient to use plain
checks than regular expression searching.
No change to generated code.
Signed-off-by: Eric Blake
---
v10: new patch, split in part from 6/17
---
scripts/qapi-commands.py | 2 +-
scripts/qapi-visit.py| 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 43a893b..d6a4bf4 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -175,7 +175,7 @@ def gen_marshal(name, arg_type, ret_type):
ret += gen_marshal_input_visit(arg_type)
ret += gen_call(name, arg_type, ret_type)
-if re.search('^ *goto out;', ret, re.MULTILINE):
+if (arg_type and arg_type.members) or ret_type:
ret += mcgen('''
out:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d0759d7..e878018 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -87,7 +87,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
%(c_name)s **obj, Error **e
ret += gen_visit_fields(members, prefix='(*obj)->')
-if re.search('^ *goto out;', ret, re.MULTILINE):
+if base or members:
ret += mcgen('''
out:
--
2.4.3
[Qemu-devel] [PATCH v10 02/25] qapi: More idiomatic string operations
Rather than slicing the end of a string, we can use python's
endswith(). And rather than creating a set of characters,
we can search for a character within a string.
Signed-off-by: Eric Blake
---
v10: new patch
---
scripts/qapi.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 9d53255..3af4c2c 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -172,7 +172,7 @@ class QAPISchemaParser(object):
if self.tok == '#':
self.cursor = self.src.find('\n', self.cursor)
-elif self.tok in ['{', '}', ':', ',', '[', ']']:
+elif self.tok in "{}:,[]":
return
elif self.tok == "'":
string = ''
@@ -390,7 +390,7 @@ def add_name(name, info, meta, implicit=False):
raise QAPIExprError(info,
"%s '%s' is already defined"
% (all_names[name], name))
-if not implicit and name[-4:] == 'Kind':
+if not implicit and name.endswith('Kind'):
raise QAPIExprError(info,
"%s '%s' should not end in 'Kind'"
% (meta, name))
@@ -910,7 +910,7 @@ class QAPISchemaEnumType(QAPISchemaType):
def is_implicit(self):
# See QAPISchema._make_implicit_enum_type()
-return self.name[-4:] == 'Kind'
+return self.name.endswith('Kind')
def c_type(self, is_param=False):
return c_name(self.name)
--
2.4.3
Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT
On 2015-10-22 12:46:56, Paolo Bonzini wrote:
>
> On 22/10/2015 20:04, Kevin O'Connor wrote:
> > On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote:
> >> On 21/10/2015 20:36, Jordan Justen wrote:
> >>> On 2015-10-20 11:14:00, Laszlo Ersek wrote:
> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the
> ich9_apm_ctrl_changed() ioport write callback function such that it would
> inject the SMI, in response to a write to the APM_CNT register, on the
> first CPU, invariably.
>
> Since this register is used by guest code to trigger an SMI
> synchronously,
> the interrupt should be injected on the VCPU that is performing the
> write.
> >>>
> >>> Why not send an SMI to *all* processors, like the real chipsets do?
> >>
> >> That's much less scalable, and more important I would have to check that
> >> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't
> >> relocate SMBASEs.
> >
> > SeaBIOS is only expecting its SMI handler to be called once in
> > response to a synchronous SMI. We can change SeaBIOS to fix that.
> >
> > SeaBIOS does relocate the smbase from 0x3 to 0xa during its
> > init phase (by creating a synchronous SMI on the BSP and then setting
> > the smbase register to 0xa in the smi handler).
>
> Right; however it would also have to relocate the SMBASE on the APs (in
> case they were halted with cli;hlt and not INITed). It's really not
> worth the hassle,
It's not worth the hassle to relocate the SMBASE of the APs?
So, basically, write to 0x3-0x38000, then send an SMI IPI to the
AP and now you have the AP running in SMI and it has extra privileges?
> it's not even documented in the chipset docs whether
> 0xb2 sends an SMI to all processors or only the running one.
My knowledge of recent chipsets is getting rusty, but in ICH ~ ICH6
the SMI# pin was wired to every processor. Any chipset based SMI would
cause all processors to enter SMM.
In more recent chipsets, I could speculate that maybe the SMI could be
delivered over a bus instead. But, I still think the chipset SMI's
would go to all processors. I did note from the ICH9 datasheet that
there is still a SMI# pin.
I also saw this under "Interrupt Message Format" of the ICH9
datasheet:
"The ICH9’s I/O APIC can only send interrupts due to interrupts which
do not include SMI, NMI or INIT. This means that in IA-32/Intel ® 64
based platforms, Front Side Bus interrupt message format delivery
modes 010 (SMI/ PMI), 100 (NMI), and 101 (INIT) as indicated in this
section, must not be used and is not supported. Only the hardware pin
connection is supported by ICH9."
This leads me to think that the ICH9 system designs continued to wire
the SMI# pin to all processors.
-Jordan
Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
Hi Michael
On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
Enable pcie device multifunction hot, just ensure the function 0
added last, then driver will got the notification to scan all the
function in the slot.
Signed-off-by: Cao jin
---
hw/pci/pci.c | 29 +
hw/pci/pci_host.c| 11 +--
hw/pci/pcie.c| 18 +-
include/hw/pci/pci.h | 1 +
4 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0bf540..c068248 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev, PCIBus *bus,
PCIConfigWriteFunc *config_write = pc->config_write;
Error *local_err = NULL;
AddressSpace *dma_as;
+DeviceState *dev = DEVICE(pci_dev);
if (devfn < 0) {
for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
@@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev, PCIBus *bus,
PCI_SLOT(devfn), PCI_FUNC(devfn), name,
bus->devices[devfn]->name);
return NULL;
+} else if (dev->hotplugged &&
+ ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn),
0)]) ||
+(pc->is_express && bus->devices[0]))) {
So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
ok, will modify it and all the following comment.
+error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
+ " new func %s cannot be exposed to guest.",
+ PCI_SLOT(devfn),
+ bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
+ name);
+
+ return NULL;
}
pci_dev->bus = bus;
@@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
}
+/* ARI device function number range is 0-255, means has only 1 function0;
+ * while PCI device has 1 function0 in each slot(up to 32 slot) */
+bool pci_is_function_0(PCIDevice *pci_dev)
+{
+PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
+bool is_func0 = false;
+
+if (pc->is_express) {
This is not what the spec says. It says:
devices connected to an upstream port.
Yes, I am wrong here. I got confused about the ARI device definition in
spec:"a device associated with an Upstream Port". Because as I
understand, ARI device is a EP immediately below a ARI downstream port,
just as Figure 6-13(Example System Topology with ARI Devices) shows in
the spec, but where is upstream port in the definition come from, what
the relationship between upstream port and a ARI device?
+if (!pci_dev->devfn)
+is_func0 = true;
Just do return !pci_dev->devfn here.
+} else {
+if(!PCI_FUNC(pci_dev->devfn))
+is_func0 = true;
+}
+
+return is_func0;
+}
+
static const TypeInfo pci_device_type_info = {
.name = TYPE_PCI_DEVICE,
.parent = TYPE_DEVICE,
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..40087c9 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -20,6 +20,7 @@
#include "hw/pci/pci.h"
#include "hw/pci/pci_host.h"
+#include "hw/pci/pci_bus.h"
#include "trace.h"
/* debug PCI */
@@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val,
int len)
PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
-if (!pci_dev) {
+/* non-zero functions are only exposed when function 0 is present,
+ * allowing direct removal of unexposed functions.
+ */
+if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
Reuse pci_get_function_0 here too?
return;
}
@@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
uint32_t val;
-if (!pci_dev) {
+/* non-zero functions are only exposed when function 0 is present,
+ * allowing direct removal of unexposed functions.
+ */
+if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
And here?
return ~0x0;
}
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index b1adeaf..d0a8006 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler
*hotplug_dev, DeviceState *dev,
return;
}
-/* TODO: multifunction hot-plug.
- * Right now, only a device of function = 0 is allowed to be
- * hot plugged/unplugged.
+/* To enable multifunction hot-plug, we just ensure the function
+ * 0 added last. Until function 0 added, we set the sltsta and
+ * inform OS via event notification.
Should be "when function 0 is added".
*/
-assert(PCI_FUNC
Re: [Qemu-devel] [RFC PATCH 0/3] Qemu/IXGBE: Add live migration support for SRIOV NIC
On 2015年10月22日 02:39, Alex Williamson wrote: > On Thu, 2015-10-22 at 00:52 +0800, Lan Tianyu wrote: >> This patchset is Qemu part for live migration support for SRIOV NIC. >> kernel part patch information is in the following link. >> http://marc.info/?l=kvm&m=144544635330193&w=2 >> >> >> Lan Tianyu (3): >> Qemu: Add pci-assign.h to share functions and struct definition with >> new file >> Qemu: Add post_load_state() to run after restoring CPU state >> Qemu: Introduce pci-sriov device type to support VF live migration >> >> hw/i386/kvm/Makefile.objs | 2 +- >> hw/i386/kvm/pci-assign.c| 113 +-- >> hw/i386/kvm/pci-assign.h| 109 +++ >> hw/i386/kvm/sriov.c | 213 >> >> include/migration/vmstate.h | 2 + >> migration/savevm.c | 15 >> 6 files changed, 344 insertions(+), 110 deletions(-) >> create mode 100644 hw/i386/kvm/pci-assign.h >> create mode 100644 hw/i386/kvm/sriov.c >> > Hi Lan, Hi Alex: Thanks a lot for your comments. It's very helpful. > > Seems like there are a couple immediate problems with this approach. > The first is that you're modifying legacy KVM device assignment, which > is deprecated upstream and not even enabled by some distros. VFIO is > the supported mechanism for doing PCI device assignment now and any > features like this need to be added there first. It's not only more > secure than legacy KVM device assignment, but it also doesn't limit this > to an x86-only solution. Surely you want to support 82599 VF migration > on other platforms as well. Yes, we will turn to VFIO and just uses legacy mode to show our idea as soon as possible. > > Using sysfs to interact with the PF is also problematic since that means > that libvirt needs to grant qemu access to these files, adding one more > layer to the stack. If we were to use VFIO, we could potentially enable > this through a save-state region on the device file descriptor and if > necessary, virtual interrupt channels for the device as well. This of > course implies that the kernel internal channels are made as general as > possible in order to support any PF driver. This sounds reasonable. > > That said, there are some nice features here. Using unused PCI config > bytes to communicate with the guest driver and enable guest-based page > dirtying is a nice hack. However, if we want to add this capability to > other devices, we're not always going to be able to use fixed addresses > 0xf0 and 0xf1. I would suggest that we probably want to create a > virtual capability in the config space of the VF, perhaps a Vendor > Specific capability. Obviously some devices won't have room for a full > capability in the standard config space, so we may need to optionally > expose it in extended config space. Those device would be limited to > only supporting migration in PCI-e configurations in the guest. Also, > plenty of devices make use of undefined PCI config space, so we may not > be able to simply add a capability to a region we think is unused, maybe > it needs to happen through reserved space in another capability or > perhaps defining a virtual BAR that unenlightened guest drivers would > ignore. The point is that we somehow need to standardize that so that > rather than implicitly know that it's at 0xf0/0xf1 on 82599 VFs. Yes, use "0xF0" and "0xF1" to show idea and it's need more effort to find the suitable place. Will research more. > > Also, I haven't looked at the kernel-side patches yet, but the saved > state received from and loaded into the PF driver needs to be versioned > and maybe we need some way to know whether versions are compatible. > Migration version information is difficult enough for QEMU, it's a > completely foreign concept in the kernel. Thanks, Good point. Will add it into next version. > > Alex > -- Best regards Tianyu Lan
[Qemu-devel] [PATCH v7 08/10] block: Add "drained begin/end" for transactional blockdev-backup
Similar to the previous patch, make sure that external events are not
dispatched during transaction operations.
Signed-off-by: Fam Zheng
Reviewed-by: Jeff Cody
Reviewed-by: Kevin Wolf
---
blockdev.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 0a7848b..52f44b2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1756,6 +1756,11 @@ static void blockdev_backup_prepare(BlkTransactionState
*common, Error **errp)
return;
}
+if (!blk_is_available(blk)) {
+error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device);
+return;
+}
+
target = blk_by_name(backup->target);
if (!target) {
error_setg(errp, "Device '%s' not found", backup->target);
@@ -1770,6 +1775,8 @@ static void blockdev_backup_prepare(BlkTransactionState
*common, Error **errp)
return;
}
aio_context_acquire(state->aio_context);
+state->bs = blk_bs(blk);
+bdrv_drained_begin(state->bs);
qmp_blockdev_backup(backup->device, backup->target,
backup->sync,
@@ -1782,7 +1789,6 @@ static void blockdev_backup_prepare(BlkTransactionState
*common, Error **errp)
return;
}
-state->bs = blk_bs(blk);
state->job = state->bs->job;
}
@@ -1802,6 +1808,7 @@ static void blockdev_backup_clean(BlkTransactionState
*common)
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
common);
if (state->aio_context) {
+bdrv_drained_end(state->bs);
aio_context_release(state->aio_context);
}
}
--
2.4.3
Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate
On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > scaling, guest programs will observe TSC increasing in the migrated rate
> > other than the host TSC rate.
> >
> > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > present, then the loading will be enabled and the migrated vcpu's TSC
> > rate will override the value specified by the cpu option
> > 'tsc-freq'. Otherwise, the loading will be disabled.
>
> Why do we need an option? Why can't we enable loading unconditionally?
>
If TSC scaling is not supported by KVM and CPU, unconditionally
enabling this loading will not take effect which would be different
from users' expectation. 'load-tsc-freq' is introduced to allow users
to enable the loading of migrated TSC frequency if they do know the
underlying KVM and CPU have TSC scaling support.
> >
> > The setting of vcpu's TSC rate in this patch duplicates the code in
> > kvm_arch_init_vcpu(), so we remove the latter one.
> >
> > Signed-off-by: Haozhong Zhang
> > ---
> > target-i386/cpu.c | 1 +
> > target-i386/cpu.h | 1 +
> > target-i386/kvm.c | 28 +++-
> > 3 files changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b6bb457..763ba4b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> > DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> > DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> > DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> > +DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> > DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> > DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> > DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index ba1a289..353f5fb 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> > int64_t tsc_khz;
> > int64_t tsc_khz_incoming;
> > bool save_tsc_khz;
> > +bool load_tsc_khz;
> > void *kvm_xsave_buf;
> >
> > uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 698524a..34616f5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -743,15 +743,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > return r;
> > }
> >
> > -r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > -if (r && env->tsc_khz) {
> > -r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > -if (r < 0) {
> > -fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > -return r;
> > -}
> > -}
> > -
> > if (kvm_has_xsave()) {
> > env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> > }
> > @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> > return 0;
> >
> > /*
> > + * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate
> > in the
> > + * migrated state will be used and the overrides the user-specified
> > vcpu's
> > + * TSC rate (if any).
> > + */
> > +if (runstate_check(RUN_STATE_INMIGRATE) &&
> > +env->load_tsc_khz && env->tsc_khz_incoming) {
> > +env->tsc_khz = env->tsc_khz_incoming;
> > +}
>
> Please don't make the results of the function depend on global QEMU
> runstate, as it makes it harder to reason about it, and easy to
> introduce subtle bugs if we change initialization order. Can't we just
> ensure tsc_khz gets set to the right value before the function is
> called, inside the code that loads migration data?
>
> > +
> > +r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > +if (r && env->tsc_khz) {
> > +r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > +if (r < 0) {
> > +fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > +return r;
> > +}
> > +}
>
> So, the final result here does not depend on the configuration, but also
> on host capabilities. That means nobody can possibly know if the
> tsc-freq option really works, until they enable it, run the VM, and
> check the results from inside the VM. Not a good idea.
>
> (This doesn't apply just to the new code, the existing code is already
> broken this way.)
>
> --
> Eduardo
[Qemu-devel] [PATCH v7 07/10] block: Add "drained begin/end" for transactional backup
This ensures the atomicity of the transaction by avoiding processing of
external requests such as those from ioeventfd.
Move the assignment to state->bs up right after bdrv_drained_begin, so
that we can use it in the clean callback. The abort callback will still
check bs->job and state->job, so it's OK.
Signed-off-by: Fam Zheng
Reviewed-by: Jeff Cody
Reviewed-by: Kevin Wolf
---
blockdev.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index e4a5eb4..0a7848b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1684,9 +1684,16 @@ static void drive_backup_prepare(BlkTransactionState
*common, Error **errp)
return;
}
+if (!blk_is_available(blk)) {
+error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device);
+return;
+}
+
/* AioContext is released in .clean() */
state->aio_context = blk_get_aio_context(blk);
aio_context_acquire(state->aio_context);
+bdrv_drained_begin(blk_bs(blk));
+state->bs = blk_bs(blk);
qmp_drive_backup(backup->device, backup->target,
backup->has_format, backup->format,
@@ -1702,7 +1709,6 @@ static void drive_backup_prepare(BlkTransactionState
*common, Error **errp)
return;
}
-state->bs = blk_bs(blk);
state->job = state->bs->job;
}
@@ -1722,6 +1728,7 @@ static void drive_backup_clean(BlkTransactionState
*common)
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
if (state->aio_context) {
+bdrv_drained_end(state->bs);
aio_context_release(state->aio_context);
}
}
--
2.4.3
[Qemu-devel] [PATCH v7 09/10] block: Add "drained begin/end" for internal snapshot
This ensures the atomicity of the transaction by avoiding processing of
external requests such as those from ioeventfd.
state->bs is assigned right after bdrv_drained_begin. Because it was
used as the flag for deletion or not in abort, now we need a separate
flag - InternalSnapshotState.created.
Signed-off-by: Fam Zheng
---
blockdev.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 52f44b2..18712d2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1370,6 +1370,7 @@ typedef struct InternalSnapshotState {
BlockDriverState *bs;
AioContext *aio_context;
QEMUSnapshotInfo sn;
+bool created;
} InternalSnapshotState;
static void internal_snapshot_prepare(BlkTransactionState *common,
@@ -1414,6 +1415,9 @@ static void internal_snapshot_prepare(BlkTransactionState
*common,
}
bs = blk_bs(blk);
+state->bs = bs;
+bdrv_drained_begin(bs);
+
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
return;
}
@@ -1465,7 +1469,7 @@ static void internal_snapshot_prepare(BlkTransactionState
*common,
}
/* 4. succeed, mark a snapshot is created */
-state->bs = bs;
+state->created = true;
}
static void internal_snapshot_abort(BlkTransactionState *common)
@@ -1476,7 +1480,7 @@ static void internal_snapshot_abort(BlkTransactionState
*common)
QEMUSnapshotInfo *sn = &state->sn;
Error *local_error = NULL;
-if (!bs) {
+if (!state->created) {
return;
}
@@ -1497,6 +1501,9 @@ static void internal_snapshot_clean(BlkTransactionState
*common)
common, common);
if (state->aio_context) {
+if (state->bs) {
+bdrv_drained_end(state->bs);
+}
aio_context_release(state->aio_context);
}
}
--
2.4.3
[Qemu-devel] [PATCH v7 06/10] block: Add "drained begin/end" for transactional external snapshot
This ensures the atomicity of the transaction by avoiding processing of
external requests such as those from ioeventfd.
Signed-off-by: Fam Zheng
Reviewed-by: Jeff Cody
Reviewed-by: Kevin Wolf
---
blockdev.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 35e5719..e4a5eb4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1569,6 +1569,7 @@ static void external_snapshot_prepare(BlkTransactionState
*common,
/* Acquire AioContext now so any threads operating on old_bs stop */
state->aio_context = bdrv_get_aio_context(state->old_bs);
aio_context_acquire(state->aio_context);
+bdrv_drained_begin(state->old_bs);
if (!bdrv_is_inserted(state->old_bs)) {
error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
@@ -1638,8 +1639,6 @@ static void external_snapshot_commit(BlkTransactionState
*common)
* don't want to abort all of them if one of them fails the reopen */
bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
NULL);
-
-aio_context_release(state->aio_context);
}
static void external_snapshot_abort(BlkTransactionState *common)
@@ -1649,7 +1648,14 @@ static void external_snapshot_abort(BlkTransactionState
*common)
if (state->new_bs) {
bdrv_unref(state->new_bs);
}
+}
+
+static void external_snapshot_clean(BlkTransactionState *common)
+{
+ExternalSnapshotState *state =
+ DO_UPCAST(ExternalSnapshotState, common, common);
if (state->aio_context) {
+bdrv_drained_end(state->old_bs);
aio_context_release(state->aio_context);
}
}
@@ -1809,6 +1815,7 @@ static const BdrvActionOps actions[] = {
.prepare = external_snapshot_prepare,
.commit = external_snapshot_commit,
.abort = external_snapshot_abort,
+.clean = external_snapshot_clean,
},
[TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
.instance_size = sizeof(DriveBackupState),
--
2.4.3
[Qemu-devel] [PATCH v7 05/10] block: Introduce "drained begin/end" API
The semantics is that after bdrv_drained_begin(bs), bs will not get new external
requests until the matching bdrv_drained_end(bs).
Signed-off-by: Fam Zheng
---
block/io.c| 17 +
include/block/block.h | 19 +++
include/block/block_int.h | 2 ++
3 files changed, 38 insertions(+)
diff --git a/block/io.c b/block/io.c
index 2fd7a1d..5ac6256 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2624,3 +2624,20 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
}
bdrv_start_throttled_reqs(bs);
}
+
+void bdrv_drained_begin(BlockDriverState *bs)
+{
+if (!bs->quiesce_counter++) {
+aio_disable_external(bdrv_get_aio_context(bs));
+}
+bdrv_drain(bs);
+}
+
+void bdrv_drained_end(BlockDriverState *bs)
+{
+assert(bs->quiesce_counter > 0);
+if (--bs->quiesce_counter > 0) {
+return;
+}
+aio_enable_external(bdrv_get_aio_context(bs));
+}
diff --git a/include/block/block.h b/include/block/block.h
index 28d903c..5d722a7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -610,4 +610,23 @@ void bdrv_io_plug(BlockDriverState *bs);
void bdrv_io_unplug(BlockDriverState *bs);
void bdrv_flush_io_queue(BlockDriverState *bs);
+/**
+ * bdrv_drained_begin:
+ *
+ * Begin a quiesced section for exclusive access to the BDS, by disabling
+ * external request sources including NBD server and device model. Note that
+ * this doesn't block timers or coroutines from submitting more requests, which
+ * means block_job_pause is still necessary.
+ *
+ * This function can be recursive.
+ */
+void bdrv_drained_begin(BlockDriverState *bs);
+
+/**
+ * bdrv_drained_end:
+ *
+ * End a quiescent section started by bdrv_drained_begin().
+ */
+void bdrv_drained_end(BlockDriverState *bs);
+
#endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e472a03..e317b14 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -448,6 +448,8 @@ struct BlockDriverState {
/* threshold limit for writes, in bytes. "High water mark". */
uint64_t write_threshold_offset;
NotifierWithReturn write_threshold_notifier;
+
+int quiesce_counter;
};
struct BlockBackendRootState {
--
2.4.3
Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
On 10/22/2015 10:05 PM, Leonid Bloch wrote:
> On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang wrote:
>>
>>
>> On 10/21/2015 05:13 PM, Leonid Bloch wrote:
>>> Hi Jason, thanks for the review!
>>>
>>> On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang wrote:
On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> These registers appear in Intel's specs, but were not implemented.
> These registers are now implemented trivially, i.e. they are initiated
> with zero values, and if they are RW, they can be written or read by the
> driver, or read only if they are R (essentially retaining their zero
> values). For these registers no other procedures are performed.
>
> The registers implemented here are:
>
> Transmit:
> RW: AIT
>
> Management:
> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
My version of DSM (Revision) said WUS is read only.
>>> This seems to be a typo in the specs. We also have the specs where it
>>> is said that WUS's read only, but exactly two lines below it - writing
>>> to it is mentioned. Additionally, in the specs for newer Intel's
>>> devices, where the offset and the functionality of WUS are exactly the
>>> same, it is written that WUS is RW.
>> Ok.
>>
> Diagnostic:
> RW: RDFHRDFTRDFHS RDFTS RDFPC PBM*
For those diagnostic register, isn't it better to warn the incomplete
emulation instead of trying to give all zero values silently? I suspect
this can make diagnostic software think the device is malfunction?
>>> That's a good point. What do you think about keeping the zero values,
>>> but printing out a warning (via DBGOUT) on each read/write attempt?
>> This is fine for me.
>>
> Statistic:
> RW: FCRUC TDFHTDFTTDFHS TDFTS TDFPC
TDFHTDFTTDFHS TDFTS TDFPC should be Diagnostic?
>>> Yes, they are. Thanks, I'll reword.
> R: RNBCTSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> LATECOL MCC COLCDC TNCRS SEC CEXTERR RLECXONRXC
> XONTXC XOFFRXC XOFFTXC
>
> Signed-off-by: Leonid Bloch
> Signed-off-by: Dmitry Fleytman
> ---
> hw/net/e1000.c | 52
> +---
> hw/net/e1000_regs.h | 6 ++
> 2 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 6d57663..6f754ac 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -168,7 +168,17 @@ enum {
> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> defreg(VET), defreg(RDTR),defreg(RADV),defreg(TADV),
> -defreg(ITR),
> +defreg(ITR), defreg(FCRUC), defreg(TDFH),defreg(TDFT),
> +defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> +defreg(RDFT),defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> +defreg(IPAV),defreg(WUC), defreg(WUS), defreg(AIT),
> +defreg(IP6AT), defreg(IP4AT), defreg(FFLT),defreg(FFMT),
> +defreg(FFVT),defreg(WUPM),defreg(PBM), defreg(SCC),
> +defreg(ECOL),defreg(MCC), defreg(LATECOL), defreg(COLC),
> +defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> +defreg(RLEC),defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> +defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> +defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> };
>
> static void
> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> }
>
> static uint32_t
> +mac_low11_read(E1000State *s, int index)
> +{
> +return s->mac_reg[index] & 0x7ff;
> +}
> +
> +static uint32_t
> +mac_low13_read(E1000State *s, int index)
> +{
> +return s->mac_reg[index] & 0x1fff;
> +}
> +
> +static uint32_t
> mac_icr_read(E1000State *s, int index)
> {
> uint32_t ret = s->mac_reg[ICR];
> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *,
> int) = {
> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> getreg(TDBAL),getreg(TDBAH),getreg(RDBAH),getreg(RDBAL),
> getreg(TDLEN),getreg(RDLEN),getreg(RDTR), getreg(RADV),
> -getreg(TADV), getreg(ITR),
> +getreg(TADV), getreg(ITR), getreg(FCRUC),getreg(IPAV),
> +getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
For AIT should we use low16_read() ?
>>> Contrary to registers where lowXX_read() is used, for AIT the specs
>>> say that the higher bits should be written with 0b, and not "read as
>>> 0b". That's my reasoning for that. What do you think?
>> I think it's better to test this be
[Qemu-devel] [PATCH v7 04/10] aio: introduce aio_{disable, enable}_external
Signed-off-by: Fam Zheng
---
aio-posix.c | 3 ++-
aio-win32.c | 3 ++-
include/block/aio.h | 38 ++
3 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/aio-posix.c b/aio-posix.c
index f0f9122..0467f23 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -261,7 +261,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
/* fill pollfds */
QLIST_FOREACH(node, &ctx->aio_handlers, node) {
-if (!node->deleted && node->pfd.events) {
+if (!node->deleted && node->pfd.events
+&& aio_node_check(ctx, node->is_external)) {
add_pollfd(node);
}
}
diff --git a/aio-win32.c b/aio-win32.c
index 3110d85..43c4c79 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -309,7 +309,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
/* fill fd sets */
count = 0;
QLIST_FOREACH(node, &ctx->aio_handlers, node) {
-if (!node->deleted && node->io_notify) {
+if (!node->deleted && node->io_notify
+&& aio_node_check(ctx, node->is_external)) {
events[count++] = event_notifier_get_handle(node->e);
}
}
diff --git a/include/block/aio.h b/include/block/aio.h
index 12f1141..bcc7d43 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -122,6 +122,8 @@ struct AioContext {
/* TimerLists for calling timers - one per clock type */
QEMUTimerListGroup tlg;
+
+int external_disable_cnt;
};
/**
@@ -375,4 +377,40 @@ static inline void aio_timer_init(AioContext *ctx,
*/
int64_t aio_compute_timeout(AioContext *ctx);
+/**
+ * aio_disable_external:
+ * @ctx: the aio context
+ *
+ * Disable the further processing of external clients.
+ */
+static inline void aio_disable_external(AioContext *ctx)
+{
+atomic_inc(&ctx->external_disable_cnt);
+}
+
+/**
+ * aio_enable_external:
+ * @ctx: the aio context
+ *
+ * Enable the processing of external clients.
+ */
+static inline void aio_enable_external(AioContext *ctx)
+{
+assert(ctx->external_disable_cnt > 0);
+atomic_dec(&ctx->external_disable_cnt);
+}
+
+/**
+ * aio_node_check:
+ * @ctx: the aio context
+ * @is_external: Whether or not the checked node is an external event source.
+ *
+ * Check if the node's is_external flag is okay to be polled by the ctx at this
+ * moment. True means green light.
+ */
+static inline bool aio_node_check(AioContext *ctx, bool is_external)
+{
+return !is_external || !atomic_read(&ctx->external_disable_cnt);
+}
+
#endif
--
2.4.3
[Qemu-devel] [PATCH v7 10/10] tests: Add test case for aio_disable_external
Signed-off-by: Fam Zheng
Reviewed-by: Kevin Wolf
---
tests/test-aio.c | 24
1 file changed, 24 insertions(+)
diff --git a/tests/test-aio.c b/tests/test-aio.c
index 03cd45d..1623803 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -374,6 +374,29 @@ static void test_flush_event_notifier(void)
event_notifier_cleanup(&data.e);
}
+static void test_aio_external_client(void)
+{
+int i, j;
+
+for (i = 1; i < 3; i++) {
+EventNotifierTestData data = { .n = 0, .active = 10, .auto_set = true
};
+event_notifier_init(&data.e, false);
+aio_set_event_notifier(ctx, &data.e, true, event_ready_cb);
+event_notifier_set(&data.e);
+for (j = 0; j < i; j++) {
+aio_disable_external(ctx);
+}
+for (j = 0; j < i; j++) {
+assert(!aio_poll(ctx, false));
+assert(event_notifier_test_and_clear(&data.e));
+event_notifier_set(&data.e);
+aio_enable_external(ctx);
+}
+assert(aio_poll(ctx, false));
+event_notifier_cleanup(&data.e);
+}
+}
+
static void test_wait_event_notifier_noflush(void)
{
EventNotifierTestData data = { .n = 0 };
@@ -832,6 +855,7 @@ int main(int argc, char **argv)
g_test_add_func("/aio/event/wait", test_wait_event_notifier);
g_test_add_func("/aio/event/wait/no-flush-cb",
test_wait_event_notifier_noflush);
g_test_add_func("/aio/event/flush", test_flush_event_notifier);
+g_test_add_func("/aio/external-client", test_aio_external_client);
g_test_add_func("/aio/timer/schedule", test_timer_schedule);
g_test_add_func("/aio-gsource/flush", test_source_flush);
--
2.4.3
[Qemu-devel] [PATCH v7 03/10] dataplane: Mark host notifiers' client type as "external"
They will be excluded by type in the nested event loops in block layer,
so that unwanted events won't be processed there.
Signed-off-by: Fam Zheng
Reviewed-by: Kevin Wolf
---
hw/block/dataplane/virtio-blk.c | 5 ++---
hw/scsi/virtio-scsi-dataplane.c | 18 --
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index f8716bc..c42ddeb 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -283,7 +283,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
/* Get this show started by hooking up our callbacks */
aio_context_acquire(s->ctx);
-aio_set_event_notifier(s->ctx, &s->host_notifier, false,
+aio_set_event_notifier(s->ctx, &s->host_notifier, true,
handle_notify);
aio_context_release(s->ctx);
return;
@@ -320,8 +320,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
aio_context_acquire(s->ctx);
/* Stop notifications for new requests from guest */
-aio_set_event_notifier(s->ctx, &s->host_notifier, false,
- NULL);
+aio_set_event_notifier(s->ctx, &s->host_notifier, true, NULL);
/* Drain and switch bs back to the QEMU main loop */
blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 21f51df..0d8d71e 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -60,8 +60,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
r = g_new(VirtIOSCSIVring, 1);
r->host_notifier = *virtio_queue_get_host_notifier(vq);
r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
-aio_set_event_notifier(s->ctx, &r->host_notifier, false,
- handler);
+aio_set_event_notifier(s->ctx, &r->host_notifier, true, handler);
r->parent = s;
@@ -72,8 +71,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
return r;
fail_vring:
-aio_set_event_notifier(s->ctx, &r->host_notifier, false,
- NULL);
+aio_set_event_notifier(s->ctx, &r->host_notifier, true, NULL);
k->set_host_notifier(qbus->parent, n, false);
g_free(r);
return NULL;
@@ -165,16 +163,16 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
if (s->ctrl_vring) {
aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
- false, NULL);
+ true, NULL);
}
if (s->event_vring) {
aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
- false, NULL);
+ true, NULL);
}
if (s->cmd_vrings) {
for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
- false, NULL);
+ true, NULL);
}
}
}
@@ -296,12 +294,12 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
aio_context_acquire(s->ctx);
aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
- false, NULL);
+ true, NULL);
aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
- false, NULL);
+ true, NULL);
for (i = 0; i < vs->conf.num_queues; i++) {
aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
- false, NULL);
+ true, NULL);
}
blk_drain_all(); /* ensure there are no in-flight requests */
--
2.4.3
[Qemu-devel] [PATCH v7 01/10] aio: Add "is_external" flag for event handlers
All callers pass in false, and the real external ones will switch to
true in coming patches.
Signed-off-by: Fam Zheng
Reviewed-by: Jeff Cody
Reviewed-by: Kevin Wolf
---
aio-posix.c | 6 -
aio-win32.c | 5
async.c | 3 ++-
block/curl.c| 14 +-
block/iscsi.c | 9 +++
block/linux-aio.c | 5 ++--
block/nbd-client.c | 10 ---
block/nfs.c | 17 +---
block/sheepdog.c| 38 ++-
block/ssh.c | 5 ++--
block/win32-aio.c | 5 ++--
hw/block/dataplane/virtio-blk.c | 6 +++--
hw/scsi/virtio-scsi-dataplane.c | 24 +++--
include/block/aio.h | 2 ++
iohandler.c | 3 ++-
nbd.c | 4 ++-
tests/test-aio.c| 58 +++--
17 files changed, 130 insertions(+), 84 deletions(-)
diff --git a/aio-posix.c b/aio-posix.c
index d477033..f0f9122 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -25,6 +25,7 @@ struct AioHandler
IOHandler *io_write;
int deleted;
void *opaque;
+bool is_external;
QLIST_ENTRY(AioHandler) node;
};
@@ -43,6 +44,7 @@ static AioHandler *find_aio_handler(AioContext *ctx, int fd)
void aio_set_fd_handler(AioContext *ctx,
int fd,
+bool is_external,
IOHandler *io_read,
IOHandler *io_write,
void *opaque)
@@ -82,6 +84,7 @@ void aio_set_fd_handler(AioContext *ctx,
node->io_read = io_read;
node->io_write = io_write;
node->opaque = opaque;
+node->is_external = is_external;
node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);
node->pfd.events |= (io_write ? G_IO_OUT | G_IO_ERR : 0);
@@ -92,10 +95,11 @@ void aio_set_fd_handler(AioContext *ctx,
void aio_set_event_notifier(AioContext *ctx,
EventNotifier *notifier,
+bool is_external,
EventNotifierHandler *io_read)
{
aio_set_fd_handler(ctx, event_notifier_get_fd(notifier),
- (IOHandler *)io_read, NULL, notifier);
+ is_external, (IOHandler *)io_read, NULL, notifier);
}
bool aio_prepare(AioContext *ctx)
diff --git a/aio-win32.c b/aio-win32.c
index 50a6867..3110d85 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -28,11 +28,13 @@ struct AioHandler {
GPollFD pfd;
int deleted;
void *opaque;
+bool is_external;
QLIST_ENTRY(AioHandler) node;
};
void aio_set_fd_handler(AioContext *ctx,
int fd,
+bool is_external,
IOHandler *io_read,
IOHandler *io_write,
void *opaque)
@@ -86,6 +88,7 @@ void aio_set_fd_handler(AioContext *ctx,
node->opaque = opaque;
node->io_read = io_read;
node->io_write = io_write;
+node->is_external = is_external;
event = event_notifier_get_handle(&ctx->notifier);
WSAEventSelect(node->pfd.fd, event,
@@ -98,6 +101,7 @@ void aio_set_fd_handler(AioContext *ctx,
void aio_set_event_notifier(AioContext *ctx,
EventNotifier *e,
+bool is_external,
EventNotifierHandler *io_notify)
{
AioHandler *node;
@@ -133,6 +137,7 @@ void aio_set_event_notifier(AioContext *ctx,
node->e = e;
node->pfd.fd = (uintptr_t)event_notifier_get_handle(e);
node->pfd.events = G_IO_IN;
+node->is_external = is_external;
QLIST_INSERT_HEAD(&ctx->aio_handlers, node, node);
g_source_add_poll(&ctx->source, &node->pfd);
diff --git a/async.c b/async.c
index efce14b..bdc64a3 100644
--- a/async.c
+++ b/async.c
@@ -247,7 +247,7 @@ aio_ctx_finalize(GSource *source)
}
qemu_mutex_unlock(&ctx->bh_lock);
-aio_set_event_notifier(ctx, &ctx->notifier, NULL);
+aio_set_event_notifier(ctx, &ctx->notifier, false, NULL);
event_notifier_cleanup(&ctx->notifier);
rfifolock_destroy(&ctx->lock);
qemu_mutex_destroy(&ctx->bh_lock);
@@ -329,6 +329,7 @@ AioContext *aio_context_new(Error **errp)
}
g_source_set_can_recurse(&ctx->source, true);
aio_set_event_notifier(ctx, &ctx->notifier,
+ false,
(EventNotifierHandler *)
event_notifier_dummy_cb);
ctx->thread_pool = NULL;
diff --git a/block/curl.c b/block/curl.c
index 032cc8a..8994182 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -154,18 +154,20 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int
action,
DPRINTF("CURL (AIO): Sock acti
[Qemu-devel] [PATCH v7 02/10] nbd: Mark fd handlers client type as "external"
So we could distinguish it from internal used fds, thus avoid handling
unwanted events in nested aio polls.
Signed-off-by: Fam Zheng
Reviewed-by: Jeff Cody
Reviewed-by: Kevin Wolf
---
nbd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/nbd.c b/nbd.c
index fbc66be..dab1ebb 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1446,7 +1446,7 @@ static void nbd_set_handlers(NBDClient *client)
{
if (client->exp && client->exp->ctx) {
aio_set_fd_handler(client->exp->ctx, client->sock,
- false,
+ true,
client->can_read ? nbd_read : NULL,
client->send_coroutine ? nbd_restart_write : NULL,
client);
@@ -1457,7 +1457,7 @@ static void nbd_unset_handlers(NBDClient *client)
{
if (client->exp && client->exp->ctx) {
aio_set_fd_handler(client->exp->ctx, client->sock,
- false, NULL, NULL, NULL);
+ true, NULL, NULL, NULL);
}
}
--
2.4.3
[Qemu-devel] [PATCH v7 00/10] block: Protect nested event loop with bdrv_drained_begin and bdrv_drained_end
v7: Exclude bdrv_drain and bdrv_qed_drain patches, they'll follow the
bdrv_drain fix for bdrv_aio_flush.
Fix internal snapshot clean.
v6: Add Kevin's rev-by in patches 1-3, 6-8, 10, 12.
Add Jeff's rev-by in patches 1, 2, 6-8, 10.
04: Fix spelling and wording in comments. [Jeff]
Add assert at decrement. [Jeff]
05: Fix bad rebase. [Jeff]
09: Let blk_is_available come first. [Jeff, Kevin]
11: Rewrite bdrv_qed_drain. [Jeff]
v5: Rebase onto Kevin's block tree.
v4: Rebase on to master so fix the "bdrv_move_feature_fields" issue.
v3: Call bdrv_drain unconditionally in bdrv_drained_begin.
Document the internal I/O implications between bdrv_drain_begin and end.
The nested aio_poll()'s in block layer has a bug that new r/w requests from
ioeventfds and nbd exports are processed, which might break the caller's
semantics (qmp_transaction) or even pointers (bdrv_reopen).
Fam Zheng (10):
aio: Add "is_external" flag for event handlers
nbd: Mark fd handlers client type as "external"
dataplane: Mark host notifiers' client type as "external"
aio: introduce aio_{disable,enable}_external
block: Introduce "drained begin/end" API
block: Add "drained begin/end" for transactional external snapshot
block: Add "drained begin/end" for transactional backup
block: Add "drained begin/end" for transactional blockdev-backup
block: Add "drained begin/end" for internal snapshot
tests: Add test case for aio_disable_external
aio-posix.c | 9 -
aio-win32.c | 8 +++-
async.c | 3 +-
block/curl.c| 14 ---
block/io.c | 17 +
block/iscsi.c | 9 ++---
block/linux-aio.c | 5 ++-
block/nbd-client.c | 10 +++--
block/nfs.c | 17 -
block/sheepdog.c| 38 ---
block/ssh.c | 5 ++-
block/win32-aio.c | 5 ++-
blockdev.c | 40 +---
hw/block/dataplane/virtio-blk.c | 5 ++-
hw/scsi/virtio-scsi-dataplane.c | 22 +++
include/block/aio.h | 40
include/block/block.h | 19 ++
include/block/block_int.h | 2 +
iohandler.c | 3 +-
nbd.c | 4 +-
tests/test-aio.c| 82 -
21 files changed, 265 insertions(+), 92 deletions(-)
--
2.4.3
Re: [Qemu-devel] [PATCH v6 11/12] qed: Implement .bdrv_drain
On Thu, 10/22 22:59, Paolo Bonzini wrote: > > > On 22/10/2015 12:53, Kevin Wolf wrote: > > Am 22.10.2015 um 08:32 hat Fam Zheng geschrieben: > >> The "need_check_timer" is used to clear the "NEED_CHECK" flag in the > >> image header after a grace period once metadata update has finished. In > >> compliance to the bdrv_drain semantics we should make sure it remains > >> deleted once .bdrv_drain is called. > >> > >> We cannot reuse qed_need_check_timer_cb because it calls bdrv_aio_flush > >> with a completion callback that starts more I/O, bdrv_drain cannot cope > >> with this. > > > > For the record: I discussed this with Fam on IRC and we came to the > > conclusion that we should instead fix bdrv_drain() to correctly drain > > in-flight flushes. > > > > In order to achieve this, Fam will send a series that adds a > > BdrvTrackedRequest to all remaining asynchronous operations. So far we > > have identified flush, discard and aio_ioctl. At the same time he will > > send a rebased version of this series that goes back to asynchronous > > flushing in bdrv_qed_drain(). > > This is a completely separate bug. Can you at least merge all patches > except this, so that everything except QED can do snapshots with > dataplane? Of course there will be a v7 at least for patch 9. > So I'll drop the two patches for QED in v7, and let them follow the bdrv_drain fixing series. Actually, I think the current being of the QED timer doesn't change guest visible data - if the callback would dequeue more allocating requests (qed_unplug_allocating_write_reqs), the loop in bdrv_drain will wait for them. So it's not a big problem for the sake of transaction after all. Fam
Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate
On Thu, Oct 22, 2015 at 04:11:37PM -0200, Eduardo Habkost wrote:
> On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> > Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> > scaling, guest programs will observe TSC increasing in the migrated rate
> > other than the host TSC rate.
> >
> > The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> > present, then the loading will be enabled and the migrated vcpu's TSC
> > rate will override the value specified by the cpu option
> > 'tsc-freq'. Otherwise, the loading will be disabled.
>
> Why do we need an option? Why can't we enable loading unconditionally?
>
> >
> > The setting of vcpu's TSC rate in this patch duplicates the code in
> > kvm_arch_init_vcpu(), so we remove the latter one.
> >
> > Signed-off-by: Haozhong Zhang
> > ---
> > target-i386/cpu.c | 1 +
> > target-i386/cpu.h | 1 +
> > target-i386/kvm.c | 28 +++-
> > 3 files changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b6bb457..763ba4b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> > DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> > DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> > DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> > +DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> > DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> > DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> > DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index ba1a289..353f5fb 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> > int64_t tsc_khz;
> > int64_t tsc_khz_incoming;
> > bool save_tsc_khz;
> > +bool load_tsc_khz;
> > void *kvm_xsave_buf;
> >
> > uint64_t mcg_cap;
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 698524a..34616f5 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -743,15 +743,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > return r;
> > }
> >
> > -r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > -if (r && env->tsc_khz) {
> > -r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > -if (r < 0) {
> > -fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > -return r;
> > -}
> > -}
> > -
> > if (kvm_has_xsave()) {
> > env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> > }
> > @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> > return 0;
> >
> > /*
> > + * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate
> > in the
> > + * migrated state will be used and the overrides the user-specified
> > vcpu's
> > + * TSC rate (if any).
> > + */
> > +if (runstate_check(RUN_STATE_INMIGRATE) &&
> > +env->load_tsc_khz && env->tsc_khz_incoming) {
> > +env->tsc_khz = env->tsc_khz_incoming;
> > +}
>
> Please don't make the results of the function depend on global QEMU
> runstate, as it makes it harder to reason about it, and easy to
> introduce subtle bugs if we change initialization order. Can't we just
> ensure tsc_khz gets set to the right value before the function is
> called, inside the code that loads migration data?
>
I can make kvm_setup_tsc_khz() called in
do_kvm_cpu_synchronize_post_init() and also make empty stubs for other
targets.
> > +
> > +r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> > +if (r && env->tsc_khz) {
> > +r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> > +if (r < 0) {
> > +fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> > +return r;
> > +}
> > +}
>
> So, the final result here does not depend on the configuration, but also
> on host capabilities. That means nobody can possibly know if the
> tsc-freq option really works, until they enable it, run the VM, and
> check the results from inside the VM. Not a good idea.
>
> (This doesn't apply just to the new code, the existing code is already
> broken this way.)
>
Really should abort QEMU here for both tsc-freq and migration.
> --
> Eduardo
Re: [Qemu-devel] [RFC PATCH 0/2] target-ppc migration fixes
On Sun, Sep 20, 2015 at 10:31:01PM +0200, Alexander Graf wrote: > > > On 14.09.15 21:30, Mark Cave-Ayland wrote: > > Whilst trying to fix migration of g3beige/mac99 images I came up with the > > following patchset. The first patch is really cosmetic, while the second > > patch > > alters the migration stream to include internal CPU IRQ state which appears > > to fix an issue where images randomly fail to resume after migration. > > > > As the second patch would need more work if deemed correct (the change in > > migration stream would require a bump in version number), it seemed worth > > putting this out for review in case this is actually the symptom of another > > bug. > > > > Signed-off-by: Mark Cave-Ayland > > David, when a non-RFC version of this patch comes around, could you > please review and if good apply it to the tree via your branch? > > Thanks a bunch! Mark, I haven't seen a revised version of this. Is that because it hasn't been posted, or just because I've missed it somehow? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] PPC VCPU ID packing via KVM_CAP_PPC_SMT
On Mon, Oct 19, 2015 at 02:34:47PM +1100, Sam Bobroff wrote: > Hi everyone, > > It's currently possible to configure QEMU and KVM such that (on a Power 7 or 8 > host) users are unable to create as many VCPUs as they might reasonably > expect. > I'll outline one fairly straight forward solution (below) and I would welcome > feedback: Does this seem a reasonable approach? Are there alternatives? > > The issue: > > The behaviour is caused by three things: > * QEMU limits the total number (count) of VCPUs based on the machine type > (hard > coded to 256 for pseries). > * See hw/ppc/spapr.c spapr_machine_class_init() > * KVM limits the highest VCPU ID to CONFIG_NR_CPUS (2048 for > pseries_defconfig). > * See arch/powerpc/configs/pseries_defconfig > * and arch/powerpc/include/asm/kvm_host.h > * If the host SMT mode is higher than the guest SMT mode when creating VCPUs, > QEMU must "pad out" the VCPU IDs to align the VCPUs with physical cores (KVM > doesn't know which SMT mode the guest wants). > * See target-ppc/translate_init.c ppc_cpu_realizefn(). > > In the most pathological case the guest is SMT 1 (smp_threads = 1) and the > host > SMT 8 (max_smt = 8), which causes the VCPU IDs to be spaced 8 apart (e.g. 0, > 8, > 24, ...). > > This doesn't produce any strange behaviour with default limits, but consider > the case where CONFIG_NR_CPUs is set to 1024 (with the same SMT modes as > above): as the 128th VCPU is created, it's VCPU ID will be 128 * 8 = 1024, > which will be rejected by KVM. This could be surprising because only 128 VCPUs > can be created when max_cpus = 256 and CONFIG_NR_CPUS = 1024. > > Proposal: > > One solution is to provide a way for QEMU to inform KVM of the guest's SMT > mode. This would allow KVM to place the VCPUs correctly within physical cores > without any VCPU ID padding. I think that's a good idea. In fact it's what we should have done in the first place. Controlling the guest SMT mode implicitly with the vcpu IDs was a case of too-clever-by-half on my part. > And one way to do that would be for KVM to allow QEMU to set the (currently > read-only) KVM_CAP_PPC_SMT capability to the required guest SMT mode. Sounds ok. > The simplest implementation would seem to be to add a new version of the > pseries machine and have it require that the kernel support setting > KVM_CAP_PPC_SMT, but would this be a reasonable restriction? It's.. not great. > Should we add a > property (where?) to allow the new machine version to run without the new > kernel feature? Could that property default to "on" or "on if supported by the > kernel" without it becoming too complicated or causing trouble during > migration? So, migration is the issue, yes. But.. I thought we already disconnected the KVM vcpu IDs from the qemu internal cpu IDs, which is what we need for migration. If that's so (check, please), then it should be sufficient to make sure that the KVM vcpu ID is included in the migration stream - it might be already. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] prep: do not use CPU_LOG_IOPORT, convert to tracepoints
On Fri, Oct 16, 2015 at 03:16:11PM +0200, Paolo Bonzini wrote: > These messages are disabled by default; a perfect usecase for tracepoints. > Convert them over. > > Signed-off-by: Paolo Bonzini Looks good to me. Applied to ppc-next. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v2 0/3] target-i386: save/restore vcpu's TSC rate during migration
On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote: > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote: > > This patchset enables QEMU to save/restore vcpu's TSC rate during the > > migration. When cooperating with KVM which supports TSC scaling, guest > > programs can observe a consistent guest TSC rate even though they are > > migrated among machines with different host TSC rates. > > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to > > control the migration of vcpu's TSC rate. > > The requirements and goals aren't clear to me. I see two possible use > cases, here: > > 1) Best effort to keep TSC frequency constant if possible (but not >aborting migration if not possible). This would be an interesting >default, but a bit unpredictable. > 2) Strictly ensuring TSC frequency stays constant on migration (and >aborting migration if not possible). This would be an useful feature, >but can't be enabled by default unless both hosts have the same TSC >frequency or support TSC scaling. > > Which one(s) you are trying to implement? > The former. I agree that it's unpredictable if setting vcpu's TSC frequency to the migrated value is enabled by default (but not in this patchset). The cpu option 'load-tsc-freq' is introduced to allow users to enable this behavior if they do know the underlying KVM and CPU support TSC scaling. In this way, I think the behavior is predictable as users do know what they are doing. > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if > the requirements and goals are not clear. > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host TSC frequency as vcpu's TSC frequency. If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the setting of TSC frequency will fail and abort either the VM creation (this is the case for cpu option 'tsc-freq') or the migration. > Once we know what exactly is the goal, we could enable the new mode with > a single option, instead of raw options to control migration stream > loading/saving. > Saving vcpu's TSC frequency does not depend on TSC scaling but the loading does. And that is why I introduce two cpu options to control them separately. Haozhong > > > * By default, the migration of vcpu's TSC rate is enabled only on > >pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq' > >is present, the vcpu's TSC rate will be migrated from older machine > >types as well. > > * Another cpu option 'load-tsc-freq' controls whether the migrated > >vcpu's TSC rate is used. By default, QEMU will not use the migrated > >TSC rate if this option is not present. Otherwise, QEMU will use > >the migrated TSC rate and override the TSC rate given by the cpu > >option 'tsc-freq'. > > > > Changes in v2: > > * Add a pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' to > >control the migration of vcpu's TSC rate. > > * Move all logic of setting TSC rate to target-i386. > > * Remove the duplicated TSC setup in kvm_arch_init_vcpu(). > > > > Haozhong Zhang (3): > > target-i386: add a subsection for migrating vcpu's TSC rate > > target-i386: calculate vcpu's TSC rate to be migrated > > target-i386: load the migrated vcpu's TSC rate > > > > include/hw/i386/pc.h | 5 + > > target-i386/cpu.c | 2 ++ > > target-i386/cpu.h | 3 +++ > > target-i386/kvm.c | 61 > > +++ > > target-i386/machine.c | 19 > > 5 files changed, 81 insertions(+), 9 deletions(-) > > > > -- > > 2.4.8 > > > > -- > Eduardo
Re: [Qemu-devel] [PATCH v9 05/17] qapi: Unbox base members
On 10/21/2015 07:34 AM, Markus Armbruster wrote: > > The least verbose naming convention for a conversion function I can > think of right now is TBase(), where T is the name of a type with a > base. Compare: > > foo((Parent *)child, blah) > foo(ChildBase(child), blah) > > Tolerable? Worthwhile? 'TBase' won't work. We already have BlockdevOptionsBase as a subtype of BlockdevOptions, and using 'TBase' would give us 'BlockdevOptionsBase(options)' which is now ambiguous between the type name and intended function call. I'll probably go with qapi_TYPE_base(), and see how long that looks. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] Question about odd snapshot behaviour
On Mon, Oct 19, 2015 at 02:50:17PM +0200, Kevin Wolf wrote:
> [ CC: qemu-block ]
>
> Am 19.10.2015 um 07:27 hat Sam Bobroff geschrieben:
> > Hi all,
> >
> > While working through the QEMU code that saves and loads snapshots, I've
> > noticed some confusing behaviour when using named VM snapshots that may
> > need to
> > be fixed somehow. This is what I've noticed:
> >
> > * If I create two empty qcow2 filesystems: fs1.qcow2 and fs2.qcow2:
> > * e.g. qemu-image create -f qcow2 fs1.qcow2 10G
> > * e.g. qemu-image create -f qcow2 fs2.qcow2 10G
> > * Then boot the guest with the above filesystems as hda and hdb (my guest's
> > rootfs
> > is an initramfs, not either of these):
> > * e.g. qemu-system-ppc64 ... -hda fs1.qcow2 -hdb fs2.qcow2
> > * In the QEMU monitor, create a blockdev snapshot on hd2 called "foo":
> > e.g. snapshot_blkdev_internal scsi0-hd1 foo
> > * Check fs2 on the host command line:
> > e.g. qemu-img snapshot -l fs2.qcow2
> > Snapshot list:
> > IDTAG VM SIZEDATE VM CLOCK
> > 1 foo 0 2015-10-19 15:56:29 00:00:20.969
> > * In the QEMU monitor, save the vm state as "bar":
> > e.g. savevm bar
> > * Check the snapshot on fs1:
> > e.g. info snapshots
> > IDTAG VM SIZEDATE VM CLOCK
> > 1 bar215M 2015-10-19 04:57:13 00:01:05.589
> > * Check fs2 again on the host command line:
> > e.g. qemu-img snapshot -l fs2.qcow2
> > Snapshot list:
> > IDTAG VM SIZEDATE VM CLOCK
> > 1 foo 0 2015-10-19 15:56:29 00:00:20.969
> > 2 bar 0 2015-10-19 15:57:13 00:01:05.589
> >
> > * Now the fun part: overwrite the bar snapshot on fs1 by ID:
> > * savevm 1
> > * Examine the results from the monitor:
> > * e.g. info snapshots
> > There is no suitable snapshot available
> > * Examine fs1 and fs2 from the command line:
> > * e.g. qemu-img snapshot -l fs1.qcow2
> > Snapshot list:
> > IDTAG VM SIZEDATE VM CLOCK
> > 1 bar215M 2015-10-19 16:00:45 00:04:36.065
> > * e.g. qemu-img snapshot -l fs2.qcow2
> > Snapshot list:
> > IDTAG VM SIZEDATE VM CLOCK
> > 2 bar 0 2015-10-19 15:57:13 00:01:05.589
> > 3 bar 0 2015-10-19 16:00:45 00:04:36.065
>
> I must admit that I wouldn't have been able to predict this result. :-)
>
> Generally, internal snapshots with multiple disks are somewhat tricky
> because we have individual names and IDs in every image file, and each
> file can individually be snapshotted, but need to create a consistent
> view from it.
>
> > So what seems odd is:
> >
> > * QEMU has lost the bar snapshot on fs1, although qemu-img still shows
> > it and it re-appears in QEMU if the guest is started with only
> > hd1.qcow attached.
>
> This one I think is a feature: A snapshot can only be loaded if it's
> present on all image files. Loading the snapshot only on some disks and
> leaving the other disks alone is likely to result in an inconsistent
> state.
How about expanding the output from "info snapshots" to include the invalid
ones but with enough information so that you can see that they're invalid, and
why that is?
> > * QEMU has deleted the snapshot named "foo" on hd2, even though we issued
> > "savevm 1" and the old snapshot (on hd1) with ID 1 was named "bar". This
> > surprised me.
>
> You mean we should check whether a snapshot identified by an ID has the
> same name on all image files before it's considered valid? That might be
> a reasonable constraint.
>
> What I would have expected is that the old snapshot with ID 1 indeed
> gets deleted, but a new one with ID 1 is created again. I'm not sure if
> I would have expected "foo" or "bar" as its name, I could argue either
> way.
A large part of the confusion comes from the way the search functions work:
they take a string but will match either the ID or name, and there's no way to
specify which. Could we change the HMP command so that by default it works only
by name, but can optionally take an ID (via an explicit flag or something)?
> > * QEMU has created a snapshot on hd2 with a duplicate name ("bar") and if we
> > issue a "loadvm bar" QEMU loads the snapshot with ID 1 on hd1 and the
> > first
> > snapshot with the name "bar" on hd2 which is the one with ID 2. That is
> > not
> > the snapshot that matches the vm state (which is the one with ID 3) and
> > presumably can lead to filesystem corruption on hd2.
>
> Yup, this doesn't seem very nice. I'm not sure whether we can forbid
> duplicate names, though, without breaking compatibility in an
> unacceptable way.
It seems li
[Qemu-devel] [PATCH] vfio/pci: Hide device PCIe capability on non-express buses for PCIe VMs
When we have a PCIe VM, such as Q35, guests start to care more about
valid configurations of devices relative to the VM view of the PCI
topology. Windows will error with a Code 10 for an assigned device if
a PCIe capability is found for a device on a conventional bus. We
also have the possibility of IOMMUs, like VT-d, where the where the
guest may be acutely aware of valid express capabilities on physical
hardware.
Some devices, like tg3 are adversely affected by this due to driver
dependencies on the PCIe capability. The only solution for such
devices is to attach them to an express capable bus in the VM.
Signed-off-by: Alex Williamson
---
hw/vfio/pci.c | 36 +++-
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8fadbcf..035007f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -28,6 +28,7 @@
#include "config.h"
#include "hw/pci/msi.h"
#include "hw/pci/msix.h"
+#include "hw/pci/pci_bridge.h"
#include "qemu/error-report.h"
#include "qemu/range.h"
#include "sysemu/kvm.h"
@@ -1524,13 +1525,38 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int
pos, uint8_t size)
}
if (!pci_bus_is_express(vdev->pdev.bus)) {
+PCIBus *bus = vdev->pdev.bus;
+PCIDevice *bridge;
+
/*
- * Use express capability as-is on PCI bus. It doesn't make much
- * sense to even expose, but some drivers (ex. tg3) depend on it
- * and guests don't seem to be particular about it. We'll need
- * to revist this or force express devices to express buses if we
- * ever expose an IOMMU to the guest.
+ * Traditionally PCI device assignment exposes the PCIe capability
+ * as-is on non-express buses. The reason being that some drivers
+ * simply assume that it's there, for example tg3. However when
+ * we're running on a native PCIe machine type, like Q35, we need
+ * to hide the PCIe capability. The reason for this is twofold;
+ * first Windows guests get a Code 10 error when the PCIe capability
+ * is exposed in this configuration. Therefore express devices won't
+ * work at all unless they're attached to express buses in the VM.
+ * Second, a native PCIe machine introduces the possibility of fine
+ * granularity IOMMUs supporting both translation and isolation.
+ * Guest code to discover the IOMMU visibility of a device, such as
+ * IOMMU grouping code on Linux, is very aware of device types and
+ * valid transitions between bus types. An express device on a non-
+ * express bus is not a valid combination on bare metal systems.
+ *
+ * Drivers that require a PCIe capability to make the device
+ * functional are simply going to need to have their devices placed
+ * on a PCIe bus in the VM.
*/
+while (!pci_bus_is_root(bus)) {
+bridge = pci_bridge_get_device(bus);
+bus = bridge->bus;
+}
+
+if (pci_bus_is_express(bus)) {
+return 0;
+}
+
} else if (pci_bus_is_root(vdev->pdev.bus)) {
/*
* On a Root Complex bus Endpoints become Root Complex Integrated
[Qemu-devel] [PATCH] pci: Adjust PCI config limit based on bus topology
A conventional PCI bus does not support config space accesses above
the standard 256 byte configuration space. PCIe-to-PCI bridges are
not permitted to forward transactions if the extended register address
field is non-zero and must handle it as an unsupported request (PCIe
bridge spec rev 1.0, 4.1.3, 4.1.4). Therefore, we should not support
extended config space if there is a conventional bus anywhere on the
path to a device.
Signed-off-by: Alex Williamson
---
hw/pci/pci_host.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..b6846ee 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -19,6 +19,7 @@
*/
#include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
#include "hw/pci/pci_host.h"
#include "trace.h"
@@ -48,9 +49,29 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus,
uint32_t addr)
return pci_find_device(bus, bus_num, devfn);
}
+static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit)
+{
+if (*limit > PCI_CONFIG_SPACE_SIZE) {
+if (!pci_bus_is_express(bus)) {
+*limit = PCI_CONFIG_SPACE_SIZE;
+return;
+}
+
+if (!pci_bus_is_root(bus)) {
+PCIDevice *bridge = pci_bridge_get_device(bus);
+pci_adjust_config_limit(bridge->bus, limit);
+}
+}
+}
+
void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
uint32_t limit, uint32_t val, uint32_t len)
{
+pci_adjust_config_limit(pci_dev->bus, &limit);
+if (limit <= addr) {
+return;
+}
+
assert(len <= 4);
trace_pci_cfg_write(pci_dev->name, PCI_SLOT(pci_dev->devfn),
PCI_FUNC(pci_dev->devfn), addr, val);
@@ -62,6 +83,11 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev,
uint32_t addr,
{
uint32_t ret;
+pci_adjust_config_limit(pci_dev->bus, &limit);
+if (limit <= addr) {
+return ~0x0;
+}
+
assert(len <= 4);
ret = pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
trace_pci_cfg_read(pci_dev->name, PCI_SLOT(pci_dev->devfn),
Re: [Qemu-devel] [PATCH v2 1/8] i.MX: Standardize i.MX serial debug.
On Wed, Oct 21, 2015 at 2:35 PM, Jean-Christophe Dubois
wrote:
> The goal is to have debug code always compiled during build.
>
> We standardize all debug output on the following format:
>
> [QOM_TYPE_NAME]reporting_function: debug message
>
> We also replace IPRINTF with qemu_log_mask(). The qemu_log_mask() output
> is following the same format as the above debug.
>
> Signed-off-by: Jean-Christophe Dubois
> ---
>
> Changes since v1:
> * use HWADDR_PRIx for address formating
> * standardize qemu_log_mask on same model.
>
> hw/char/imx_serial.c | 57
> ++--
> 1 file changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index f0c4c72..73e8eb3 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -22,25 +22,17 @@
> #include "sysemu/sysemu.h"
> #include "sysemu/char.h"
>
> -//#define DEBUG_SERIAL 1
> -#ifdef DEBUG_SERIAL
> -#define DPRINTF(fmt, args...) \
> -do { printf("%s: " fmt , TYPE_IMX_SERIAL, ##args); } while (0)
> -#else
> -#define DPRINTF(fmt, args...) do {} while (0)
> +#ifndef DEBUG_IMX_UART
> +#define DEBUG_IMX_UART 0
> #endif
>
> -/*
> - * Define to 1 for messages about attempts to
> - * access unimplemented registers or similar.
> - */
> -//#define DEBUG_IMPLEMENTATION 1
> -#ifdef DEBUG_IMPLEMENTATION
> -# define IPRINTF(fmt, args...) \
> -do { fprintf(stderr, "%s: " fmt, TYPE_IMX_SERIAL, ##args); } while (0)
> -#else
> -# define IPRINTF(fmt, args...) do {} while (0)
> -#endif
> +#define DPRINTF(fmt, args...) \
> + do { \
> + if (DEBUG_IMX_UART) { \
> + fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SERIAL, \
> + __func__, ##args); \
> + } \
> + } while (0)
>
> static const VMStateDescription vmstate_imx_serial = {
> .name = TYPE_IMX_SERIAL,
> @@ -113,10 +105,12 @@ static uint64_t imx_serial_read(void *opaque, hwaddr
> offset,
> unsigned size)
> {
> IMXSerialState *s = (IMXSerialState *)opaque;
> +uint32_t reg = offset >> 2;
Why the new variable? If anything, your change seems to have obsoleted
the need for a reg variable as you reduce the usage-count of (offset
>> 2) to just one (in the switch) where it vould stay inline as in
original code. Unless I am missing something?
With just that change (reverting)
Reviewed-by: Peter Crosthwaite
Regards,
Peter
> uint32_t c;
>
> -DPRINTF("read(offset=%x)\n", offset >> 2);
> -switch (offset >> 2) {
> +DPRINTF("read(offset=0x%" HWADDR_PRIx ")\n", offset);
> +
> +switch (reg) {
> case 0x0: /* URXD */
> c = s->readbuff;
> if (!(s->uts1 & UTS1_RXEMPTY)) {
> @@ -167,7 +161,8 @@ static uint64_t imx_serial_read(void *opaque, hwaddr
> offset,
> return 0x0; /* TODO */
>
> default:
> -IPRINTF("%s: bad offset: 0x%x\n", __func__, (int)offset);
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> + HWADDR_PRIx "\n", TYPE_IMX_SERIAL, __func__, offset);
> return 0;
> }
> }
> @@ -176,13 +171,13 @@ static void imx_serial_write(void *opaque, hwaddr
> offset,
> uint64_t value, unsigned size)
> {
> IMXSerialState *s = (IMXSerialState *)opaque;
> +uint32_t reg = offset >> 2;
> unsigned char ch;
>
> -DPRINTF("write(offset=%x, value = %x) to %s\n",
> -offset >> 2,
> -(unsigned int)value, s->chr ? s->chr->label : "NODEV");
> +DPRINTF("write(offset=0x%" HWADDR_PRIx ", value = 0x%x) to %s\n",
> +offset, (unsigned int)value, s->chr ? s->chr->label : "NODEV");
>
> -switch (offset >> 2) {
> +switch (reg) {
> case 0x10: /* UTXD */
> ch = value;
> if (s->ucr2 & UCR2_TXEN) {
> @@ -198,7 +193,9 @@ static void imx_serial_write(void *opaque, hwaddr offset,
>
> case 0x20: /* UCR1 */
> s->ucr1 = value & 0x;
> +
> DPRINTF("write(ucr1=%x)\n", (unsigned int)value);
> +
> imx_update(s);
> break;
>
> @@ -266,12 +263,14 @@ static void imx_serial_write(void *opaque, hwaddr
> offset,
>
> case 0x2d: /* UTS1 */
> case 0x23: /* UCR4 */
> -IPRINTF("Unimplemented Register %x written to\n", offset >> 2);
> + qemu_log_mask(LOG_UNIMP, "[%s]%s: Unimplemented reg %d\n",
> + TYPE_IMX_SERIAL, __func__, reg);
> /* TODO */
> break;
>
> default:
> -IPRINTF("%s: Bad offset 0x%x\n", __func__, (int)offset);
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> + HWADDR_PRIx "\n", TYPE_IMX_SERIAL, __func__, offset);
> }
> }
>
> @@ -284,7 +283,9 @@ static int imx_can_receive(void *opaque)
> static void imx_put_data(void *opaque, uint32_t value)
> {
> IMXSerialState *s = (IMXSerialState *)opaque;
> +
> D
Re: [Qemu-devel] [PATCH] target-tilegx: Implement prefetch instructions in pipe y2
On 10/23/15 01:53, Richard Henderson wrote: > On 10/20/2015 05:26 AM, Chen Gang wrote: >>> From 14fe2a651b3f5729f1d402dfcd6eb5f7da0f42b1 Mon Sep 17 00:00:00 2001 >> From: Chen Gang >> Date: Tue, 20 Oct 2015 23:19:02 +0800 >> Subject: [PATCH] target-tilegx: Implement prefetch instructions in pipe y2 >> >> Originally, tilegx qemu only implement prefetch instructions in pipe x1, >> did not implement them in pipe y2. >> >> Signed-off-by: Chen Gang > > Applied. > OK, thanks. And at present, I am trying to implement floating point insns within this month. Hope I can succeed. Welcome any members' additional ideas, suggestions, and completions. Thanks. -- Chen Gang (陈刚) Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On 10/22/15 3:47 PM, Eduardo Habkost wrote: On Thu, Oct 22, 2015 at 01:57:13PM -0600, Valerio Aimale wrote: On 10/22/15 1:12 PM, Eduardo Habkost wrote: On Wed, Oct 21, 2015 at 12:54:23PM +0200, Markus Armbruster wrote: Valerio Aimale writes: [...] There's also a similar patch, floating around the internet, the uses shared memory, instead of sockets, as inter-process communication between libvmi and QEMU. I've never used that. By the time you built a working IPC mechanism on top of shared memory, you're often no better off than with AF_LOCAL sockets. Crazy idea: can we allocate guest memory in a way that support sharing it with another process? Eduardo, can -mem-path do such wild things? It can't today, but just because it creates a temporary file inside mem-path and unlinks it immediately after opening a file descriptor. We could make memory-backend-file also accept a full filename as argument, or add a mechanism to let QEMU send the open file descriptor to a QMP client. Eduardo, would my "artisanal" idea of creating an mmap'ed image of the guest memory footprint work, augmented by Eric's suggestion of having the qmp client pass the filename? The code below doesn't make sense to me. Ok. What I am trying to do is to create a mmapped() memory area of the guest physical memory that can be shared between QEMU and an external process, such that the external process can read arbitrary locations of the qemu guest physical memory. In short, I'm using mmap MAP_SHARED to share the guest memory area with a process that is external to QEMU does it make better sense now?
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On Thu, Oct 22, 2015 at 01:57:13PM -0600, Valerio Aimale wrote:
> On 10/22/15 1:12 PM, Eduardo Habkost wrote:
> >On Wed, Oct 21, 2015 at 12:54:23PM +0200, Markus Armbruster wrote:
> >>Valerio Aimale writes:
> >[...]
> >>>There's also a similar patch, floating around the internet, the uses
> >>>shared memory, instead of sockets, as inter-process communication
> >>>between libvmi and QEMU. I've never used that.
> >>By the time you built a working IPC mechanism on top of shared memory,
> >>you're often no better off than with AF_LOCAL sockets.
> >>
> >>Crazy idea: can we allocate guest memory in a way that support sharing
> >>it with another process? Eduardo, can -mem-path do such wild things?
> >It can't today, but just because it creates a temporary file inside
> >mem-path and unlinks it immediately after opening a file descriptor. We
> >could make memory-backend-file also accept a full filename as argument,
> >or add a mechanism to let QEMU send the open file descriptor to a QMP
> >client.
> >
> Eduardo, would my "artisanal" idea of creating an mmap'ed image of the guest
> memory footprint work, augmented by Eric's suggestion of having the qmp
> client pass the filename?
The code below doesn't make sense to me.
>
> qmp_pmemmap( [...]) {
>
> char *template = "/tmp/QEM_mmap_XXX";
> int mmap_fd;
> uint8_t *local_memspace = malloc( (size_t) 8589934592 /* assuming VM
> with 8GB RAM */);
>
> cpu_physical_memory_rw( (hwaddr) 0, local_memspace , (hwaddr)
> 8589934592 /* assuming VM with 8GB RAM */, 0 /* no write for now will
> discuss write later */);
Are you suggesting copying all the VM RAM contents to a whole new area?
Why?
>
>mmap_fd = mkstemp("/tmp/QEUM_mmap_XXX");
>
>mmap((void *) local_memspace, (size_t) 8589934592, PROT_READ |
> PROT_WRITE, MAP_SHARED | MAP_ANON, mmap_fd, (off_t) 0);
I don't understand what you are trying to do here.
>
> /* etc */
>
> }
>
> pmemmap would return the following json
>
> {
> 'success' : 'true',
> 'map_filename' : '/tmp/QEM_mmap_1234567'
> }
>
>
--
Eduardo
Re: [Qemu-devel] exec: About DISAS_JUMP and DISAS_UPDATE
On 22 October 2015 at 19:28, Sergey Fedorov wrote: > Hi all, > > I am trying to understand what the difference should be between > DISAS_JUMP and DISAS_UPDATE. Actually, these macros have comments in > include/exec/exec-all.h which say that DISAS_JUMP should be used when > only PC was modified dynamically whereas DISAS_UPDATE should be used > when some other CPU state was (in addition to PC?) modified dynamically. > In fact, every target except ARM AArch64 does not distinguish between > them. As I can see ARM AArch64 seems to suppose that: (1) PC was not > modified when DISAS_UPDATE is used and should be updated with dc->pc > when finishing translation; (2) DISAS_JUMP can be used to indicate that > a new PC value was set and it should be preserved when finishing > translation. As Richard says, (a) the semantics for these values are really private to each translator (b) the general idea is how AArch64 uses them. I think the 32-bit ARM code does something a bit odd because it has to handle conditional execution (some things we might have otherwise done immediately in the decode function get postponed to the end of the loop). Mostly I haven't messed around too much with that bit of the code because it works and it's kind of complicated to understand. But the AArch64 stuff we wrote from scratch so it does things in the straightforward way. -- PMM
Re: [Qemu-devel] [PATCH v3 4/5] Enable fw_cfg DMA interface for ARM
On Sat, 19 Sep 2015, Laszlo Ersek wrote: > Got some good news: with those two fixups in place (register block > size corrected, and dma_enabled set via device property), I could > test the AAVMF / ArmVirtPkg / > patches. > > On my APM Mustang, downloading a decompressed kernel (14,475,776 > bytes), a decompressed initrd (18,177,264), and a cmdline (104 bytes :)), > in total 32,653,144 bytes, takes approx. 24 seconds with the 8-byte wide > MMIO data register. (Yeah, it's *really* slow.) > > Using the DMA interface, the same takes about 52 milliseconds, and > that still includes one progress message per 1 MB downloaded :) > > It's a factor of approx. 450. Not bad. Not bad. :) So I've been catching up (after a several-week-long day-job related detour :) with the latest developments in fw_cfg -- and the DMA stuff looks good, and makes for a very educational read! I was re-reading the documentation for fw_cfg_add_file_callback(), and noticed that non-dma read operations check for the presence of a callback (and call it if present) for *every* *single* *byte*, even on 64-bit MMIO reads. That's also what the documentation says (in docs/specs/fw_cfg.txt, being moved into fw_cfg.h as per http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05315.html). During DMA reads, however, the callback is only checked once before each chunk, effectively once per DMA read operation. Now, typical callbacks I found throughout the qemu source tend to return immediately except for the first time they're invoked, but I wonder if skipping over all those extra "do I have a callback, if so call it, mostly so it can return without doing anything" per-byte operations account in some significant part for the dramatically faster transfers? Not sure how I'd test for that -- besides my not having anything resembling a viable ARM setup, I'm not sure if limiting the callbacks to only be invoked if (s->cur_offset == 0) would make sense, just as a test ? Either way, I'll send out a v2 of my fw_cfg function-call doc patch to additionally say something like: * structure residing at key value FW_CFG_FILE_DIR, containing the * item name, * data size, and assigned selector key value. * Additionally, set a callback function (and argument) to be called * each - * time a byte is read by the guest from this particular item. + * time a byte is read by the guest from this particular item, or once per + * each DMA guest read operation. * NOTE: In addition to the opaque argument set here, the callback * function * takes the current data offset as an additional argument, allowing * it the * option of only acting upon specific offset values (e.g., 0, before * the Let me know what you all think... Thanks much, --Gabriel
Re: [Qemu-devel] [PATCH v6 11/12] qed: Implement .bdrv_drain
On 22/10/2015 12:53, Kevin Wolf wrote: > Am 22.10.2015 um 08:32 hat Fam Zheng geschrieben: >> The "need_check_timer" is used to clear the "NEED_CHECK" flag in the >> image header after a grace period once metadata update has finished. In >> compliance to the bdrv_drain semantics we should make sure it remains >> deleted once .bdrv_drain is called. >> >> We cannot reuse qed_need_check_timer_cb because it calls bdrv_aio_flush >> with a completion callback that starts more I/O, bdrv_drain cannot cope >> with this. > > For the record: I discussed this with Fam on IRC and we came to the > conclusion that we should instead fix bdrv_drain() to correctly drain > in-flight flushes. > > In order to achieve this, Fam will send a series that adds a > BdrvTrackedRequest to all remaining asynchronous operations. So far we > have identified flush, discard and aio_ioctl. At the same time he will > send a rebased version of this series that goes back to asynchronous > flushing in bdrv_qed_drain(). This is a completely separate bug. Can you at least merge all patches except this, so that everything except QED can do snapshots with dataplane? Of course there will be a v7 at least for patch 9. Paolo
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On 10/22/15 2:03 PM, Eric Blake wrote:
On 10/22/2015 01:57 PM, Valerio Aimale wrote:
pmemmap would return the following json
{
'success' : 'true',
'map_filename' : '/tmp/QEM_mmap_1234567'
}
In general, it is better if the client controls the filename, and not
qemu. This is because things like libvirt like to run qemu in a
highly-constrained environment, where the caller can pass in a file
descriptor that qemu cannot itself open(). So returning a filename is
pointless if the filename was already provided by the caller.
Eric, I absolutely and positively agree with you. I was just
brainstorming. Consider my pseudo-C code as the mailing list analog of
somebody scribbling on a white board and trying explain an idea.
I agree with you the file name should come from the qmp client.
[Qemu-devel] [PATCH v2] i386/acpi: add _HID to processor objects
This patch appends "ACPI0007" as the HID to each processor object.
Until commit 20843d processor objects used to have a _HID. According
to the ACPI spec this is not required but removing it breaks systems
which relied on the HID. As it does no harm it is safe to add _HID
to processor objects and restore the old behaviour.
Signed-off-by: Matthias Lange
---
hw/i386/acpi-build.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..314cd0b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1153,6 +1153,9 @@ build_ssdt(GArray *table_data, GArray *linker,
for (i = 0; i < acpi_cpus; i++) {
dev = aml_processor(i, 0, 0, "CP%.02X", i);
+/* for processor objects a _HID is not strictly required, however
it
+ * does no harm and preserves compatibility with other BIOSes */
+aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
method = aml_method("_MAT", 0);
aml_append(method, aml_return(aml_call1("CPMA", aml_int(i;
aml_append(dev, method);
--
1.9.1
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On 10/22/2015 01:57 PM, Valerio Aimale wrote:
>
> pmemmap would return the following json
>
> {
> 'success' : 'true',
> 'map_filename' : '/tmp/QEM_mmap_1234567'
> }
In general, it is better if the client controls the filename, and not
qemu. This is because things like libvirt like to run qemu in a
highly-constrained environment, where the caller can pass in a file
descriptor that qemu cannot itself open(). So returning a filename is
pointless if the filename was already provided by the caller.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On 10/22/15 1:12 PM, Eduardo Habkost wrote:
On Wed, Oct 21, 2015 at 12:54:23PM +0200, Markus Armbruster wrote:
Valerio Aimale writes:
[...]
There's also a similar patch, floating around the internet, the uses
shared memory, instead of sockets, as inter-process communication
between libvmi and QEMU. I've never used that.
By the time you built a working IPC mechanism on top of shared memory,
you're often no better off than with AF_LOCAL sockets.
Crazy idea: can we allocate guest memory in a way that support sharing
it with another process? Eduardo, can -mem-path do such wild things?
It can't today, but just because it creates a temporary file inside
mem-path and unlinks it immediately after opening a file descriptor. We
could make memory-backend-file also accept a full filename as argument,
or add a mechanism to let QEMU send the open file descriptor to a QMP
client.
Eduardo, would my "artisanal" idea of creating an mmap'ed image of the
guest memory footprint work, augmented by Eric's suggestion of having
the qmp client pass the filename?
qmp_pmemmap( [...]) {
char *template = "/tmp/QEM_mmap_XXX";
int mmap_fd;
uint8_t *local_memspace = malloc( (size_t) 8589934592 /* assuming
VM with 8GB RAM */);
cpu_physical_memory_rw( (hwaddr) 0, local_memspace , (hwaddr)
8589934592 /* assuming VM with 8GB RAM */, 0 /* no write for now will
discuss write later */);
mmap_fd = mkstemp("/tmp/QEUM_mmap_XXX");
mmap((void *) local_memspace, (size_t) 8589934592, PROT_READ |
PROT_WRITE, MAP_SHARED | MAP_ANON, mmap_fd, (off_t) 0);
/* etc */
}
pmemmap would return the following json
{
'success' : 'true',
'map_filename' : '/tmp/QEM_mmap_1234567'
}
Re: [Qemu-devel] exec: About DISAS_JUMP and DISAS_UPDATE
On 10/22/2015 08:28 AM, Sergey Fedorov wrote: Hi all, I am trying to understand what the difference should be between DISAS_JUMP and DISAS_UPDATE. Actually, these macros have comments in include/exec/exec-all.h which say that DISAS_JUMP should be used when only PC was modified dynamically whereas DISAS_UPDATE should be used when some other CPU state was (in addition to PC?) modified dynamically. In fact, every target except ARM AArch64 does not distinguish between them. As I can see ARM AArch64 seems to suppose that: (1) PC was not modified when DISAS_UPDATE is used and should be updated with dc->pc when finishing translation; (2) DISAS_JUMP can be used to indicate that a new PC value was set and it should be preserved when finishing translation. So I'm a bit confused... What the difference should be? Maybe something should be fixed/clarified to make the comments and the code consistent. It's a mistake that these are defined in exec/. They ought to be totally private to each translator. See e.g. ExitStatus in target-alpha/translate.c. But yes, what you see in aarch64 is approximately what is intended. r~
Re: [Qemu-devel] [PATCH] copy, dd: simplify and optimize NUL bytes detection
On 22/10/2015 19:39, Radim Krčmář wrote:
> 2015-10-22 18:14+0200, Paolo Bonzini:
>> On 22/10/2015 18:02, Eric Blake wrote:
>>> I see a bug in there:
>>
>> Of course. You shouldn't have told me what the bug was, I deserved
>> to look for it myself. :)
>
> It rather seems that you don't want spoilers, :)
>
> I see two bugs now.
Me too. :) But Rusty surely has some testcases in case he wants to
adopt some of the ideas here. O:-)
Paolo
>> bool memeqzero4_paolo(const void *data, size_t length)
>> {
>> const unsigned char *p = data;
>> unsigned long word;
>>
>> while (__builtin_expect(length & (sizeof(word) - 1), 0)) {
>> if (*p)
>> return false;
>> p++;
>> length--;
>> if (!length)
>> return true;
>> }
>>
>> /* We must always read one byte or word, even if everything is aligned!
>> * Otherwise, memcmp(data, data, length) is trivially true.
>> */
>> for (;;) {
>> memcpy(&word, p, sizeof(word));
>> if (word)
>> return false;
>> if (__builtin_expect(length & (16 - sizeof(word)), 0) == 0)
>> break;
>> p += sizeof(word);
>> length -= sizeof(word);
>> if (!length)
>> return true;
>> }
>>
>> /* Now we know that's zero, memcmp with self. */
>> return memcmp(data, p, length) == 0;
>> }
Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT
On 22/10/2015 20:04, Kevin O'Connor wrote:
> On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote:
>> On 21/10/2015 20:36, Jordan Justen wrote:
>>> On 2015-10-20 11:14:00, Laszlo Ersek wrote:
Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the
ich9_apm_ctrl_changed() ioport write callback function such that it would
inject the SMI, in response to a write to the APM_CNT register, on the
first CPU, invariably.
Since this register is used by guest code to trigger an SMI synchronously,
the interrupt should be injected on the VCPU that is performing the write.
>>>
>>> Why not send an SMI to *all* processors, like the real chipsets do?
>>
>> That's much less scalable, and more important I would have to check that
>> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't
>> relocate SMBASEs.
>
> SeaBIOS is only expecting its SMI handler to be called once in
> response to a synchronous SMI. We can change SeaBIOS to fix that.
>
> SeaBIOS does relocate the smbase from 0x3 to 0xa during its
> init phase (by creating a synchronous SMI on the BSP and then setting
> the smbase register to 0xa in the smi handler).
Right; however it would also have to relocate the SMBASE on the APs (in
case they were halted with cli;hlt and not INITed). It's really not
worth the hassle, it's not even documented in the chipset docs whether
0xb2 sends an SMI to all processors or only the running one.
Paolo
[Qemu-devel] [PULL] vhost-user: fix up rhel6 build
Build on RHEL6 fails:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42875
Apparently unnamed unions couldn't use C99 named field initializers.
Let's just name the payload union field.
Signed-off-by: Michael S. Tsirkin
---
hw/virtio/vhost-user.c | 48
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 78442ba..0aa8e0d 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -89,7 +89,7 @@ typedef struct VhostUserMsg {
struct vhost_vring_state state;
struct vhost_vring_addr addr;
VhostUserMemory memory;
-};
+} payload;
} QEMU_PACKED VhostUserMsg;
static VhostUserMsg m __attribute__ ((unused));
@@ -200,8 +200,8 @@ static int vhost_user_set_log_base(struct vhost_dev *dev,
uint64_t base,
VhostUserMsg msg = {
.request = VHOST_USER_SET_LOG_BASE,
.flags = VHOST_USER_VERSION,
-.u64 = base,
-.size = sizeof(m.u64),
+.payload.u64 = base,
+.size = sizeof(m.payload.u64),
};
if (shmfd && log->fd != -1) {
@@ -247,17 +247,17 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
&ram_addr);
fd = qemu_get_ram_fd(ram_addr);
if (fd > 0) {
-msg.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
-msg.memory.regions[fd_num].memory_size = reg->memory_size;
-msg.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
-msg.memory.regions[fd_num].mmap_offset = reg->userspace_addr -
+msg.payload.memory.regions[fd_num].userspace_addr =
reg->userspace_addr;
+msg.payload.memory.regions[fd_num].memory_size = reg->memory_size;
+msg.payload.memory.regions[fd_num].guest_phys_addr =
reg->guest_phys_addr;
+msg.payload.memory.regions[fd_num].mmap_offset =
reg->userspace_addr -
(uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
fds[fd_num++] = fd;
}
}
-msg.memory.nregions = fd_num;
+msg.payload.memory.nregions = fd_num;
if (!fd_num) {
error_report("Failed initializing vhost-user memory map, "
@@ -265,8 +265,8 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
return -1;
}
-msg.size = sizeof(m.memory.nregions);
-msg.size += sizeof(m.memory.padding);
+msg.size = sizeof(m.payload.memory.nregions);
+msg.size += sizeof(m.payload.memory.padding);
msg.size += fd_num * sizeof(VhostUserMemoryRegion);
vhost_user_write(dev, &msg, fds, fd_num);
@@ -280,7 +280,7 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
VhostUserMsg msg = {
.request = VHOST_USER_SET_VRING_ADDR,
.flags = VHOST_USER_VERSION,
-.addr = *addr,
+.payload.addr = *addr,
.size = sizeof(*addr),
};
@@ -303,7 +303,7 @@ static int vhost_set_vring(struct vhost_dev *dev,
VhostUserMsg msg = {
.request = request,
.flags = VHOST_USER_VERSION,
-.state = *ring,
+.payload.state = *ring,
.size = sizeof(*ring),
};
@@ -345,7 +345,7 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
VhostUserMsg msg = {
.request = VHOST_USER_GET_VRING_BASE,
.flags = VHOST_USER_VERSION,
-.state = *ring,
+.payload.state = *ring,
.size = sizeof(*ring),
};
@@ -361,12 +361,12 @@ static int vhost_user_get_vring_base(struct vhost_dev
*dev,
return -1;
}
-if (msg.size != sizeof(m.state)) {
+if (msg.size != sizeof(m.payload.state)) {
error_report("Received bad msg size.");
return -1;
}
-*ring = msg.state;
+*ring = msg.payload.state;
return 0;
}
@@ -380,14 +380,14 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
VhostUserMsg msg = {
.request = request,
.flags = VHOST_USER_VERSION,
-.u64 = file->index & VHOST_USER_VRING_IDX_MASK,
-.size = sizeof(m.u64),
+.payload.u64 = file->index & VHOST_USER_VRING_IDX_MASK,
+.size = sizeof(m.payload.u64),
};
if (ioeventfd_enabled() && file->fd > 0) {
fds[fd_num++] = file->fd;
} else {
-msg.u64 |= VHOST_USER_VRING_NOFD_MASK;
+msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
}
vhost_user_write(dev, &msg, fds, fd_num);
@@ -412,8 +412,8 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int
request, uint64_t u64)
VhostUserMsg msg = {
.request = request,
.flags = VHOST_USER_VERSION,
-.u64 = u64,
-.size = sizeof(m.u64),
+.payload.u64 = u64,
+.size = sizeof(m.payload.u64),
};
vhost_user_write(dev, &msg, NULL, 0);
@@ -456,12 +456,12 @@ static int vhost_user_get_u64(struct vhost_dev *dev,
[Qemu-devel] [PULL] vhost: build fix
The following changes since commit 3c23402d4032f69af44a87fdb8019ad3229a4f31: hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT (2015-10-22 14:39:09 +0300) are available in the git repository at: git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream for you to fetch changes up to 7f4a930e64b9e69cd340395a7e4f0494aef4fcdd: vhost-user: fix up rhel6 build (2015-10-22 22:34:59 +0300) vhost: build fix Fix build breakages when using older gcc. Signed-off-by: Michael S. Tsirkin Michael S. Tsirkin (1): vhost-user: fix up rhel6 build hw/virtio/vhost-user.c | 48 1 file changed, 24 insertions(+), 24 deletions(-)
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On Wed, Oct 21, 2015 at 12:54:23PM +0200, Markus Armbruster wrote: > Valerio Aimale writes: [...] > > There's also a similar patch, floating around the internet, the uses > > shared memory, instead of sockets, as inter-process communication > > between libvmi and QEMU. I've never used that. > > By the time you built a working IPC mechanism on top of shared memory, > you're often no better off than with AF_LOCAL sockets. > > Crazy idea: can we allocate guest memory in a way that support sharing > it with another process? Eduardo, can -mem-path do such wild things? It can't today, but just because it creates a temporary file inside mem-path and unlinks it immediately after opening a file descriptor. We could make memory-backend-file also accept a full filename as argument, or add a mechanism to let QEMU send the open file descriptor to a QMP client. -- Eduardo
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On 10/22/2015 12:43 PM, Valerio Aimale wrote:
>
> What if there was a qmp command, say 'pmemmap' then when invoked,
> performs the following:
>
> qmp_pmemmap( [...]) {
>
> char *template = "/tmp/QEM_mmap_XXX";
Why not let the caller pass in the file name, rather than opening it
ourselves? But the idea of coordinating a file that both caller and qemu
mmap to the same guest memory view might indeed have merit.
[by the way, it's okay to trim messages to the relevant portions, rather
than making readers scroll through lots of quoted content to find the meat]
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 0/3] target-i386: save/restore vcpu's TSC rate during migration
On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote: > This patchset enables QEMU to save/restore vcpu's TSC rate during the > migration. When cooperating with KVM which supports TSC scaling, guest > programs can observe a consistent guest TSC rate even though they are > migrated among machines with different host TSC rates. > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to > control the migration of vcpu's TSC rate. The requirements and goals aren't clear to me. I see two possible use cases, here: 1) Best effort to keep TSC frequency constant if possible (but not aborting migration if not possible). This would be an interesting default, but a bit unpredictable. 2) Strictly ensuring TSC frequency stays constant on migration (and aborting migration if not possible). This would be an useful feature, but can't be enabled by default unless both hosts have the same TSC frequency or support TSC scaling. Which one(s) you are trying to implement? In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or KVM_CAP_TSC_CONTROL is not available? We can't answer that question if the requirements and goals are not clear. Once we know what exactly is the goal, we could enable the new mode with a single option, instead of raw options to control migration stream loading/saving. > * By default, the migration of vcpu's TSC rate is enabled only on >pc-*-2.5 and newer machine types. If the cpu option 'save-tsc-freq' >is present, the vcpu's TSC rate will be migrated from older machine >types as well. > * Another cpu option 'load-tsc-freq' controls whether the migrated >vcpu's TSC rate is used. By default, QEMU will not use the migrated >TSC rate if this option is not present. Otherwise, QEMU will use >the migrated TSC rate and override the TSC rate given by the cpu >option 'tsc-freq'. > > Changes in v2: > * Add a pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' to >control the migration of vcpu's TSC rate. > * Move all logic of setting TSC rate to target-i386. > * Remove the duplicated TSC setup in kvm_arch_init_vcpu(). > > Haozhong Zhang (3): > target-i386: add a subsection for migrating vcpu's TSC rate > target-i386: calculate vcpu's TSC rate to be migrated > target-i386: load the migrated vcpu's TSC rate > > include/hw/i386/pc.h | 5 + > target-i386/cpu.c | 2 ++ > target-i386/cpu.h | 3 +++ > target-i386/kvm.c | 61 > +++ > target-i386/machine.c | 19 > 5 files changed, 81 insertions(+), 9 deletions(-) > > -- > 2.4.8 > -- Eduardo
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On 10/21/15 4:54 AM, Markus Armbruster wrote:
Valerio Aimale writes:
On 10/19/15 1:52 AM, Markus Armbruster wrote:
Valerio Aimale writes:
On 10/16/15 2:15 AM, Markus Armbruster wrote:
vale...@aimale.com writes:
All-
I've produced a patch for the current QEMU HEAD, for libvmi to
introspect QEMU/KVM VMs.
Libvmi has patches for the old qeum-kvm fork, inside its source tree:
https://github.com/libvmi/libvmi/tree/master/tools/qemu-kvm-patch
This patch adds a hmp and a qmp command, "pmemaccess". When the
commands is invoked with a string arguments (a filename), it will open
a UNIX socket and spawn a listening thread.
The client writes binary commands to the socket, in the form of a c
structure:
struct request {
uint8_t type; // 0 quit, 1 read, 2 write, ... rest reserved
uint64_t address; // address to read from OR write to
uint64_t length;// number of bytes to read OR write
};
The client receives as a response, either (length+1) bytes, if it is a
read operation, or 1 byte ifit is a write operation.
The last bytes of a read operation response indicates success (1
success, 0 failure). The single byte returned for a write operation
indicates same (1 success, 0 failure).
So, if you ask to read 1 MiB, and it fails, you get back 1 MiB of
garbage followed by the "it failed" byte?
Markus, that appear to be the case. However, I did not write the
communication protocol between libvmi and qemu. I'm assuming that the
person that wrote the protocol, did not want to bother with over
complicating things.
https://github.com/libvmi/libvmi/blob/master/libvmi/driver/kvm/kvm.c
I'm thinking he assumed reads would be small in size and the price of
reading garbage was less than the price of writing a more complicated
protocol. I can see his point, confronted with the same problem, I
might have done the same.
All right, the interface is designed for *small* memory blocks then.
Makes me wonder why he needs a separate binary protocol on a separate
socket. Small blocks could be done just fine in QMP.
The problem is speed. if one's analyzing the memory space of a running
process (physical and paged), libvmi will make a large number of small
and mid-sized reads. If one uses xp, or pmemsave, the overhead is
quite significant. xp has overhead due to encoding, and pmemsave has
overhead due to file open/write (server), file open/read/close/unlink
(client).
Others have gone through the problem before me. It appears that
pmemsave and xp are significantly slower than reading memory using a
socket via pmemaccess.
That they're slower isn't surprising, but I'd expect the cost of
encoding a small block to be insiginificant compared to the cost of the
network roundtrips.
As block size increases, the space overhead of encoding will eventually
bite. But for that usage, the binary protocol appears ill-suited,
unless the client can pretty reliably avoid read failure. I haven't
examined its failure modes, yet.
The following data is not mine, but it shows the time, in
milliseconds, required to resolve the content of a paged memory
address via socket (pmemaccess) , pmemsave and xp
http://cl.ly/image/322a3s0h1V05
Again, I did not produce those data points, they come from an old
libvmi thread.
90ms is a very long time. What exactly was measured?
I think it might be conceivable that there could be a QMP command that
returns the content of an arbitrarily size memory region as a base64
or a base85 json string. It would still have both time- (due to
encoding/decoding) and space- (base64 has 33% and ase85 would be 7%)
overhead, + json encoding/decoding overhead. It might still be the
case that socket would outperform such a command as well,
speed-vise. I don't think it would be any faster than xp.
A special-purpose binary protocol over a dedicated socket will always do
less than a QMP solution (ignoring foolishness like transmitting crap on
read error the client is then expected to throw away). The question is
whether the difference in work translates to a worthwhile difference in
performance.
The larger question is actually whether we have an existing interface
that can serve the libvmi's needs. We've discussed monitor commands
like xp, pmemsave, pmemread. There's another existing interface: the
GDB stub. Have you considered it?
There's also a similar patch, floating around the internet, the uses
shared memory, instead of sockets, as inter-process communication
between libvmi and QEMU. I've never used that.
By the time you built a working IPC mechanism on top of shared memory,
you're often no better off than with AF_LOCAL sockets.
Crazy idea: can we allocate guest memory in a way that support sharing
it with another process? Eduardo, can -mem-path do such wild things?
Markus, your suggestion led to a lightbulb going off in my head.
What if there was a qmp command, say 'pmemmap' then when invoked,
performs the following:
qmp_pmemmap( [...]) {
char *template = "/tmp/QEM_mmap_
[Qemu-devel] exec: About DISAS_JUMP and DISAS_UPDATE
Hi all, I am trying to understand what the difference should be between DISAS_JUMP and DISAS_UPDATE. Actually, these macros have comments in include/exec/exec-all.h which say that DISAS_JUMP should be used when only PC was modified dynamically whereas DISAS_UPDATE should be used when some other CPU state was (in addition to PC?) modified dynamically. In fact, every target except ARM AArch64 does not distinguish between them. As I can see ARM AArch64 seems to suppose that: (1) PC was not modified when DISAS_UPDATE is used and should be updated with dc->pc when finishing translation; (2) DISAS_JUMP can be used to indicate that a new PC value was set and it should be preserved when finishing translation. So I'm a bit confused... What the difference should be? Maybe something should be fixed/clarified to make the comments and the code consistent. Best regards, Sergey
Re: [Qemu-devel] QEMU patch to allow VM introspection via libvmi
On 10/22/15 5:50 AM, Markus Armbruster wrote:
Valerio Aimale writes:
On 10/21/15 4:54 AM, Markus Armbruster wrote:
Valerio Aimale writes:
On 10/19/15 1:52 AM, Markus Armbruster wrote:
Valerio Aimale writes:
On 10/16/15 2:15 AM, Markus Armbruster wrote:
vale...@aimale.com writes:
All-
I've produced a patch for the current QEMU HEAD, for libvmi to
introspect QEMU/KVM VMs.
Libvmi has patches for the old qeum-kvm fork, inside its source tree:
https://github.com/libvmi/libvmi/tree/master/tools/qemu-kvm-patch
This patch adds a hmp and a qmp command, "pmemaccess". When the
commands is invoked with a string arguments (a filename), it will open
a UNIX socket and spawn a listening thread.
The client writes binary commands to the socket, in the form of a c
structure:
struct request {
uint8_t type; // 0 quit, 1 read, 2 write, ... rest reserved
uint64_t address; // address to read from OR write to
uint64_t length;// number of bytes to read OR write
};
The client receives as a response, either (length+1) bytes, if it is a
read operation, or 1 byte ifit is a write operation.
The last bytes of a read operation response indicates success (1
success, 0 failure). The single byte returned for a write operation
indicates same (1 success, 0 failure).
So, if you ask to read 1 MiB, and it fails, you get back 1 MiB of
garbage followed by the "it failed" byte?
Markus, that appear to be the case. However, I did not write the
communication protocol between libvmi and qemu. I'm assuming that the
person that wrote the protocol, did not want to bother with over
complicating things.
https://github.com/libvmi/libvmi/blob/master/libvmi/driver/kvm/kvm.c
I'm thinking he assumed reads would be small in size and the price of
reading garbage was less than the price of writing a more complicated
protocol. I can see his point, confronted with the same problem, I
might have done the same.
All right, the interface is designed for *small* memory blocks then.
Makes me wonder why he needs a separate binary protocol on a separate
socket. Small blocks could be done just fine in QMP.
The problem is speed. if one's analyzing the memory space of a running
process (physical and paged), libvmi will make a large number of small
and mid-sized reads. If one uses xp, or pmemsave, the overhead is
quite significant. xp has overhead due to encoding, and pmemsave has
overhead due to file open/write (server), file open/read/close/unlink
(client).
Others have gone through the problem before me. It appears that
pmemsave and xp are significantly slower than reading memory using a
socket via pmemaccess.
That they're slower isn't surprising, but I'd expect the cost of
encoding a small block to be insiginificant compared to the cost of the
network roundtrips.
As block size increases, the space overhead of encoding will eventually
bite. But for that usage, the binary protocol appears ill-suited,
unless the client can pretty reliably avoid read failure. I haven't
examined its failure modes, yet.
The following data is not mine, but it shows the time, in
milliseconds, required to resolve the content of a paged memory
address via socket (pmemaccess) , pmemsave and xp
http://cl.ly/image/322a3s0h1V05
Again, I did not produce those data points, they come from an old
libvmi thread.
90ms is a very long time. What exactly was measured?
That is a fair question to ask. Unfortunately, I extracted that data
plot from an old thread in some libvmi mailing list. I do not have the
data and code that produced it. Sifting through the thread, I can see
the code
was never published. I will take it upon myself to produce code that
compares timing - in a fair fashion - of libvmi doing an atomic
operation and a larger-scale operation (like listing running
processes) via gdb, pmemaccess/socket, pmemsave, xp, and hopefully, a
version of xp that returns byte streams of memory regions base64 or
base85 encoded in json strings. I'll publish results and code.
However, given workload and life happening, it will be some time
before I complete that task.
No problem. I'd like to have your use case addressed, but there's no
need for haste.
Thanks, Markus. Appreciate your help.
[...]
Also, the pmemsave commands QAPI should be changed to be usable with
64bit VM's
in qapi-schema.json
from
---
{ 'command': 'pmemsave',
'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
---
to
---
{ 'command': 'pmemsave',
'data': {'val': 'int64', 'size': 'int64', 'filename': 'str'} }
---
In the QAPI schema, 'int' is actually an alias for 'int64'. Yes, that's
confusing.
I think it's confusing for the HMP parser too. If you have a VM with
8Gb of RAM and want to snapshot the whole physical memory, via HMP
over telnet this is what happens:
$ telnet localhost 1234
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
QEMU 2.4.0.1 monitor - type 'help' for more information
(qemu) help pmemsave
pmemsave addr
Re: [Qemu-devel] [PATCH v2 3/3] target-i386: load the migrated vcpu's TSC rate
On Tue, Oct 20, 2015 at 03:22:54PM +0800, Haozhong Zhang wrote:
> Set vcpu's TSC rate to the migrated value (if any). If KVM supports TSC
> scaling, guest programs will observe TSC increasing in the migrated rate
> other than the host TSC rate.
>
> The loading is controlled by a new cpu option 'load-tsc-freq'. If it is
> present, then the loading will be enabled and the migrated vcpu's TSC
> rate will override the value specified by the cpu option
> 'tsc-freq'. Otherwise, the loading will be disabled.
Why do we need an option? Why can't we enable loading unconditionally?
>
> The setting of vcpu's TSC rate in this patch duplicates the code in
> kvm_arch_init_vcpu(), so we remove the latter one.
>
> Signed-off-by: Haozhong Zhang
> ---
> target-i386/cpu.c | 1 +
> target-i386/cpu.h | 1 +
> target-i386/kvm.c | 28 +++-
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b6bb457..763ba4b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3144,6 +3144,7 @@ static Property x86_cpu_properties[] = {
> DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> DEFINE_PROP_BOOL("save-tsc-freq", X86CPU, env.save_tsc_khz, true),
> +DEFINE_PROP_BOOL("load-tsc-freq", X86CPU, env.load_tsc_khz, false),
> DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
> DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index ba1a289..353f5fb 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -968,6 +968,7 @@ typedef struct CPUX86State {
> int64_t tsc_khz;
> int64_t tsc_khz_incoming;
> bool save_tsc_khz;
> +bool load_tsc_khz;
> void *kvm_xsave_buf;
>
> uint64_t mcg_cap;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 698524a..34616f5 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -743,15 +743,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
> return r;
> }
>
> -r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> -if (r && env->tsc_khz) {
> -r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> -if (r < 0) {
> -fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> -return r;
> -}
> -}
> -
> if (kvm_has_xsave()) {
> env->kvm_xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave));
> }
> @@ -2223,6 +2214,25 @@ static int kvm_setup_tsc_khz(X86CPU *cpu, int level)
> return 0;
>
> /*
> + * If the cpu option 'load-tsc-freq' is present, the vcpu's TSC rate in
> the
> + * migrated state will be used and the overrides the user-specified
> vcpu's
> + * TSC rate (if any).
> + */
> +if (runstate_check(RUN_STATE_INMIGRATE) &&
> +env->load_tsc_khz && env->tsc_khz_incoming) {
> +env->tsc_khz = env->tsc_khz_incoming;
> +}
Please don't make the results of the function depend on global QEMU
runstate, as it makes it harder to reason about it, and easy to
introduce subtle bugs if we change initialization order. Can't we just
ensure tsc_khz gets set to the right value before the function is
called, inside the code that loads migration data?
> +
> +r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> +if (r && env->tsc_khz) {
> +r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> +if (r < 0) {
> +fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> +return r;
> +}
> +}
So, the final result here does not depend on the configuration, but also
on host capabilities. That means nobody can possibly know if the
tsc-freq option really works, until they enable it, run the VM, and
check the results from inside the VM. Not a good idea.
(This doesn't apply just to the new code, the existing code is already
broken this way.)
--
Eduardo
[Qemu-devel] [PULL v3 4/4] configure: avoid polluting global CFLAGS with tasn1 flags
The previous commit commit 9a2fd4347c40321f5cbb4ab4220e759fcbf87d03 Author: Daniel P. Berrange Date: Mon Apr 13 14:01:39 2015 +0100 crypto: add sanity checking of TLS x509 credentials defined new variables $TEST_LIBS and $TEST_CFLAGS and used them in tests/Makefile to augment $LIBS and $CFLAGS. Unfortunately this overlooks the fact that tests/Makefile is not executed via recursive-make, it is just pulled into the top level Makefile via an include statement. So rather than just augmenting the compiler/linker flags for tests it polluted the global flags. This is thought to be behind a reported failure when building the pixman module as a sub-module, since global $CFLAGS are passed down to configure in pixman. This change removes the $TEST_LIBS and $TEST_CFLAGS replacing them with $TASN1_LIBS and $TASN1_CFLAGS, setting only against specific objects/executables that need them. Signed-off-by: Daniel P. Berrange --- configure | 11 --- tests/Makefile | 10 -- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/configure b/configure index 94f38a3..2a129bb 100755 --- a/configure +++ b/configure @@ -419,9 +419,6 @@ if test "$debug_info" = "yes"; then LDFLAGS="-g $LDFLAGS" fi -test_cflags="" -test_libs="" - # make source path absolute source_path=`cd "$source_path"; pwd` @@ -2362,11 +2359,11 @@ fi # libtasn1 - only for the TLS creds/session test suite tasn1=yes +tasn1_cflags="" +tasn1_libs="" if $pkg_config --exists "libtasn1"; then tasn1_cflags=`$pkg_config --cflags libtasn1` tasn1_libs=`$pkg_config --libs libtasn1` -test_cflags="$test_cflags $tasn1_cflags" -test_libs="$test_libs $tasn1_libs" else tasn1=no fi @@ -5408,8 +5405,8 @@ echo "EXESUF=$EXESUF" >> $config_host_mak echo "DSOSUF=$DSOSUF" >> $config_host_mak echo "LDFLAGS_SHARED=$LDFLAGS_SHARED" >> $config_host_mak echo "LIBS_QGA+=$libs_qga" >> $config_host_mak -echo "TEST_LIBS=$test_libs" >> $config_host_mak -echo "TEST_CFLAGS=$test_cflags" >> $config_host_mak +echo "TASN1_LIBS=$tasn1_libs" >> $config_host_mak +echo "TASN1_CFLAGS=$tasn1_cflags" >> $config_host_mak echo "POD2MAN=$POD2MAN" >> $config_host_mak echo "TRANSLATE_OPT_CFLAGS=$TRANSLATE_OPT_CFLAGS" >> $config_host_mak if test "$gcov" = "yes" ; then diff --git a/tests/Makefile b/tests/Makefile index 0531b30..9341498 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -444,8 +444,16 @@ tests/test-mul64$(EXESUF): tests/test-mul64.o $(test-util-obj-y) tests/test-bitops$(EXESUF): tests/test-bitops.o $(test-util-obj-y) tests/test-crypto-hash$(EXESUF): tests/test-crypto-hash.o $(test-crypto-obj-y) tests/test-crypto-cipher$(EXESUF): tests/test-crypto-cipher.o $(test-crypto-obj-y) + +tests/crypto-tls-x509-helpers.o-cflags := $(TASN1_CFLAGS) +tests/crypto-tls-x509-helpers.o-libs := $(TASN1_LIBS) +tests/pkix_asn1_tab.o-cflags := $(TASN1_CFLAGS) + +tests/test-crypto-tlscredsx509.o-cflags := $(TASN1_CFLAGS) tests/test-crypto-tlscredsx509$(EXESUF): tests/test-crypto-tlscredsx509.o \ tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o $(test-crypto-obj-y) + +tests/test-crypto-tlssession.o-cflags := $(TASN1_CFLAGS) tests/test-crypto-tlssession$(EXESUF): tests/test-crypto-tlssession.o \ tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o $(test-crypto-obj-y) @@ -518,8 +526,6 @@ tests/test-netfilter$(EXESUF): tests/test-netfilter.o $(qtest-obj-y) ifeq ($(CONFIG_POSIX),y) LIBS += -lutil endif -LIBS += $(TEST_LIBS) -CFLAGS += $(TEST_CFLAGS) # QTest rules -- 2.4.3
[Qemu-devel] [PULL v3 3/4] crypto: add sanity checking of plaintext/ciphertext length
When encrypting/decrypting data, the plaintext/ciphertext
buffers are required to be a multiple of the cipher block
size. If this is not done, nettle will abort and gcrypt
will report an error. To get consistent behaviour add
explicit checks upfront for the buffer sizes.
Signed-off-by: Daniel P. Berrange
---
crypto/cipher-builtin.c| 15
crypto/cipher-gcrypt.c | 61 ++
crypto/cipher-nettle.c | 28 +++--
tests/test-crypto-cipher.c | 50 +
4 files changed, 130 insertions(+), 24 deletions(-)
diff --git a/crypto/cipher-builtin.c b/crypto/cipher-builtin.c
index 37e1a19..39e31a7 100644
--- a/crypto/cipher-builtin.c
+++ b/crypto/cipher-builtin.c
@@ -39,6 +39,7 @@ struct QCryptoCipherBuiltin {
QCryptoCipherBuiltinAES aes;
QCryptoCipherBuiltinDESRFB desrfb;
} state;
+size_t blocksize;
void (*free)(QCryptoCipher *cipher);
int (*setiv)(QCryptoCipher *cipher,
const uint8_t *iv, size_t niv,
@@ -181,6 +182,7 @@ static int qcrypto_cipher_init_aes(QCryptoCipher *cipher,
goto error;
}
+ctxt->blocksize = AES_BLOCK_SIZE;
ctxt->free = qcrypto_cipher_free_aes;
ctxt->setiv = qcrypto_cipher_setiv_aes;
ctxt->encrypt = qcrypto_cipher_encrypt_aes;
@@ -282,6 +284,7 @@ static int qcrypto_cipher_init_des_rfb(QCryptoCipher
*cipher,
memcpy(ctxt->state.desrfb.key, key, nkey);
ctxt->state.desrfb.nkey = nkey;
+ctxt->blocksize = 8;
ctxt->free = qcrypto_cipher_free_des_rfb;
ctxt->setiv = qcrypto_cipher_setiv_des_rfb;
ctxt->encrypt = qcrypto_cipher_encrypt_des_rfb;
@@ -370,6 +373,12 @@ int qcrypto_cipher_encrypt(QCryptoCipher *cipher,
{
QCryptoCipherBuiltin *ctxt = cipher->opaque;
+if (len % ctxt->blocksize) {
+error_setg(errp, "Length %zu must be a multiple of block size %zu",
+ len, ctxt->blocksize);
+return -1;
+}
+
return ctxt->encrypt(cipher, in, out, len, errp);
}
@@ -382,6 +391,12 @@ int qcrypto_cipher_decrypt(QCryptoCipher *cipher,
{
QCryptoCipherBuiltin *ctxt = cipher->opaque;
+if (len % ctxt->blocksize) {
+error_setg(errp, "Length %zu must be a multiple of block size %zu",
+ len, ctxt->blocksize);
+return -1;
+}
+
return ctxt->decrypt(cipher, in, out, len, errp);
}
diff --git a/crypto/cipher-gcrypt.c b/crypto/cipher-gcrypt.c
index 8cfc562..c4f8114 100644
--- a/crypto/cipher-gcrypt.c
+++ b/crypto/cipher-gcrypt.c
@@ -34,6 +34,11 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg)
}
}
+typedef struct QCryptoCipherGcrypt QCryptoCipherGcrypt;
+struct QCryptoCipherGcrypt {
+gcry_cipher_hd_t handle;
+size_t blocksize;
+};
QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
QCryptoCipherMode mode,
@@ -41,7 +46,7 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
Error **errp)
{
QCryptoCipher *cipher;
-gcry_cipher_hd_t handle;
+QCryptoCipherGcrypt *ctx;
gcry_error_t err;
int gcryalg, gcrymode;
@@ -87,7 +92,9 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
cipher->alg = alg;
cipher->mode = mode;
-err = gcry_cipher_open(&handle, gcryalg, gcrymode, 0);
+ctx = g_new0(QCryptoCipherGcrypt, 1);
+
+err = gcry_cipher_open(&ctx->handle, gcryalg, gcrymode, 0);
if (err != 0) {
error_setg(errp, "Cannot initialize cipher: %s",
gcry_strerror(err));
@@ -100,10 +107,12 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm
alg,
* bizarre RFB variant of DES :-)
*/
uint8_t *rfbkey = qcrypto_cipher_munge_des_rfb_key(key, nkey);
-err = gcry_cipher_setkey(handle, rfbkey, nkey);
+err = gcry_cipher_setkey(ctx->handle, rfbkey, nkey);
g_free(rfbkey);
+ctx->blocksize = 8;
} else {
-err = gcry_cipher_setkey(handle, key, nkey);
+err = gcry_cipher_setkey(ctx->handle, key, nkey);
+ctx->blocksize = 16;
}
if (err != 0) {
error_setg(errp, "Cannot set key: %s",
@@ -111,11 +120,12 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm
alg,
goto error;
}
-cipher->opaque = handle;
+cipher->opaque = ctx;
return cipher;
error:
-gcry_cipher_close(handle);
+gcry_cipher_close(ctx->handle);
+g_free(ctx);
g_free(cipher);
return NULL;
}
@@ -123,12 +133,13 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm
alg,
void qcrypto_cipher_free(QCryptoCipher *cipher)
{
-gcry_cipher_hd_t handle;
+QCryptoCipherGcrypt *ctx;
if (!cipher) {
return;
}
-handle = cipher->opaque;
-gcry_cipher_close(handle);
+ctx = cipher->opaque;
+gcry_cipher_close(ctx->handle);
+g_free(ctx);
g_free(cipher);
[Qemu-devel] [PULL v3 2/4] crypto: don't let builtin aes crash if no IV is provided
If no IV is provided, then use a default IV of all-zeros
instead of crashing. This gives parity with gcrypt and
nettle backends.
Signed-off-by: Daniel P. Berrange
---
crypto/cipher-builtin.c| 14 +-
tests/test-crypto-cipher.c | 30 ++
2 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/crypto/cipher-builtin.c b/crypto/cipher-builtin.c
index 30f4853..37e1a19 100644
--- a/crypto/cipher-builtin.c
+++ b/crypto/cipher-builtin.c
@@ -25,8 +25,7 @@ typedef struct QCryptoCipherBuiltinAES
QCryptoCipherBuiltinAES;
struct QCryptoCipherBuiltinAES {
AES_KEY encrypt_key;
AES_KEY decrypt_key;
-uint8_t *iv;
-size_t niv;
+uint8_t iv[AES_BLOCK_SIZE];
};
typedef struct QCryptoCipherBuiltinDESRFB QCryptoCipherBuiltinDESRFB;
struct QCryptoCipherBuiltinDESRFB {
@@ -61,7 +60,6 @@ static void qcrypto_cipher_free_aes(QCryptoCipher *cipher)
{
QCryptoCipherBuiltin *ctxt = cipher->opaque;
-g_free(ctxt->state.aes.iv);
g_free(ctxt);
cipher->opaque = NULL;
}
@@ -145,15 +143,13 @@ static int qcrypto_cipher_setiv_aes(QCryptoCipher *cipher,
Error **errp)
{
QCryptoCipherBuiltin *ctxt = cipher->opaque;
-if (niv != 16) {
-error_setg(errp, "IV must be 16 bytes not %zu", niv);
+if (niv != AES_BLOCK_SIZE) {
+error_setg(errp, "IV must be %d bytes not %zu",
+ AES_BLOCK_SIZE, niv);
return -1;
}
-g_free(ctxt->state.aes.iv);
-ctxt->state.aes.iv = g_new0(uint8_t, niv);
-memcpy(ctxt->state.aes.iv, iv, niv);
-ctxt->state.aes.niv = niv;
+memcpy(ctxt->state.aes.iv, iv, AES_BLOCK_SIZE);
return 0;
}
diff --git a/tests/test-crypto-cipher.c b/tests/test-crypto-cipher.c
index 9d38d26..1b60c34 100644
--- a/tests/test-crypto-cipher.c
+++ b/tests/test-crypto-cipher.c
@@ -287,6 +287,32 @@ static void test_cipher(const void *opaque)
qcrypto_cipher_free(cipher);
}
+
+static void test_cipher_null_iv(void)
+{
+QCryptoCipher *cipher;
+uint8_t key[32] = { 0 };
+uint8_t plaintext[32] = { 0 };
+uint8_t ciphertext[32] = { 0 };
+
+cipher = qcrypto_cipher_new(
+QCRYPTO_CIPHER_ALG_AES_256,
+QCRYPTO_CIPHER_MODE_CBC,
+key, sizeof(key),
+&error_abort);
+g_assert(cipher != NULL);
+
+/* Don't call qcrypto_cipher_setiv */
+
+qcrypto_cipher_encrypt(cipher,
+ plaintext,
+ ciphertext,
+ sizeof(plaintext),
+ &error_abort);
+
+qcrypto_cipher_free(cipher);
+}
+
int main(int argc, char **argv)
{
size_t i;
@@ -298,5 +324,9 @@ int main(int argc, char **argv)
for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
g_test_add_data_func(test_data[i].path, &test_data[i], test_cipher);
}
+
+g_test_add_func("/crypto/cipher/null-iv",
+test_cipher_null_iv);
+
return g_test_run();
}
--
2.4.3
[Qemu-devel] [PULL v3 0/4] Misc fixes for crypto code module
The following changes since commit ca3e40e233e87f7b29442311736a82da01c0df7b: Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2015-10-22 12:41:44 +0100) are available in the git repository at: https://github.com/berrange/qemu.git tags/qcrypto-fixes-pull-20151022-2 for you to fetch changes up to 90246037760a2a1d64da67782200b690de24cc49: configure: avoid polluting global CFLAGS with tasn1 flags (2015-10-22 19:03:08 +0100) Merge qcrypto-fixes 2015/10/22 Daniel P. Berrange (4): crypto: allow use of nettle/gcrypt to be selected explicitly crypto: don't let builtin aes crash if no IV is provided crypto: add sanity checking of plaintext/ciphertext length configure: avoid polluting global CFLAGS with tasn1 flags configure | 111 + crypto/cipher-builtin.c| 29 crypto/cipher-gcrypt.c | 61 ++--- crypto/cipher-nettle.c | 28 crypto/cipher.c| 8 ++-- crypto/init.c | 26 +-- tests/Makefile | 10 +++- tests/test-crypto-cipher.c | 80 8 files changed, 282 insertions(+), 71 deletions(-) -- 2.4.3
[Qemu-devel] [PULL v3 1/4] crypto: allow use of nettle/gcrypt to be selected explicitly
Currently the choice of whether to use nettle or gcrypt is
made based on what gnutls is linked to. There are times
when it is desirable to be able to force build against a
specific library. For example, if testing changes to QEMU's
crypto code all 3 possible backends need to be checked
regardless of what the local gnutls uses.
It is also desirable to be able to enable nettle/gcrypt
for cipher/hash algorithms, without enabling gnutls
for TLS support.
This gives two new configure flags, which allow the
following possibilities
Automatically determine nettle vs gcrypt from what
gnutls links to (recommended to minimize number of
crypto libraries linked to)
./configure
Automatically determine nettle vs gcrypt based on
which is installed
./configure --disable-gnutls
Force use of nettle
./configure --enable-nettle
Force use of gcrypt
./configure --enable-gcrypt
Force use of built-in AES & crippled-DES
./configure --disable-nettle --disable-gcrypt
Signed-off-by: Daniel P. Berrange
---
configure | 100 +---
crypto/cipher.c | 8 ++---
crypto/init.c | 26 +++
3 files changed, 105 insertions(+), 29 deletions(-)
diff --git a/configure b/configure
index 211bc6e..94f38a3 100755
--- a/configure
+++ b/configure
@@ -331,6 +331,8 @@ gtkabi=""
gtk_gl="no"
gnutls=""
gnutls_hash=""
+nettle=""
+gcrypt=""
vte=""
virglrenderer=""
tpm="yes"
@@ -1114,6 +1116,14 @@ for opt do
;;
--enable-gnutls) gnutls="yes"
;;
+ --disable-nettle) nettle="no"
+ ;;
+ --enable-nettle) nettle="yes"
+ ;;
+ --disable-gcrypt) gcrypt="no"
+ ;;
+ --enable-gcrypt) gcrypt="yes"
+ ;;
--enable-rdma) rdma="yes"
;;
--disable-rdma) rdma="no"
@@ -1324,6 +1334,8 @@ disabled with --disable-FEATURE, default is enabled if
available:
sparse sparse checker
gnutls GNUTLS cryptography support
+ nettle nettle cryptography support
+ gcrypt libgcrypt cryptography support
sdl SDL UI
--with-sdlabi select preferred SDL ABI 1.2 or 2.0
gtk gtk UI
@@ -2254,20 +2266,76 @@ else
gnutls_hash="no"
fi
-if test "$gnutls_gcrypt" != "no"; then
-if has "libgcrypt-config"; then
+
+# If user didn't give a --disable/enable-gcrypt flag,
+# then mark as disabled if user requested nettle
+# explicitly, or if gnutls links to nettle
+if test -z "$gcrypt"
+then
+if test "$nettle" = "yes" || test "$gnutls_nettle" = "yes"
+then
+gcrypt="no"
+fi
+fi
+
+# If user didn't give a --disable/enable-nettle flag,
+# then mark as disabled if user requested gcrypt
+# explicitly, or if gnutls links to gcrypt
+if test -z "$nettle"
+then
+if test "$gcrypt" = "yes" || test "$gnutls_gcrypt" = "yes"
+then
+nettle="no"
+fi
+fi
+
+has_libgcrypt_config() {
+if ! has "libgcrypt-config"
+then
+ return 1
+fi
+
+if test -n "$cross_prefix"
+then
+ host=`libgcrypt-config --host`
+ if test "$host-" != $cross_prefix
+ then
+ return 1
+ fi
+fi
+
+return 0
+}
+
+if test "$gcrypt" != "no"; then
+if has_libgcrypt_config; then
gcrypt_cflags=`libgcrypt-config --cflags`
gcrypt_libs=`libgcrypt-config --libs`
+# Debian has remove -lgpg-error from libgcrypt-config
+# as it "spreads unnecessary dependencies" which in
+# turn breaks static builds...
+if test "$static" = "yes"
+then
+gcrypt_libs="$gcrypt_libs -lgpg-error"
+fi
libs_softmmu="$gcrypt_libs $libs_softmmu"
libs_tools="$gcrypt_libs $libs_tools"
QEMU_CFLAGS="$QEMU_CFLAGS $gcrypt_cflags"
+gcrypt="yes"
+if test -z "$nettle"; then
+ nettle="no"
+fi
else
-feature_not_found "gcrypt" "Install gcrypt devel"
+if test "$gcrypt" = "yes"; then
+feature_not_found "gcrypt" "Install gcrypt devel"
+else
+gcrypt="no"
+fi
fi
fi
-if test "$gnutls_nettle" != "no"; then
+if test "$nettle" != "no"; then
if $pkg_config --exists "nettle"; then
nettle_cflags=`$pkg_config --cflags nettle`
nettle_libs=`$pkg_config --libs nettle`
@@ -2275,11 +2343,21 @@ if test "$gnutls_nettle" != "no"; then
libs_softmmu="$nettle_libs $libs_softmmu"
libs_tools="$nettle_libs $libs_tools"
QEMU_CFLAGS="$QEMU_CFLAGS $nettle_cflags"
+nettle="yes"
else
-feature_not_found "nettle" "Install nettle devel"
+if test "$nettle" = "yes"; then
+feature_not_found "nettle" "Install nettle devel"
+else
+nettle="no"
+fi
fi
fi
+if test "$gcrypt" = "yes" && test "$nettle" = "yes"
+then
+error_exit "Only one of gcrypt & nettle can be enabled"
+fi
+
##
# libtasn1 - only for the TLS creds/session test suite
@@ -4621,8 +4699,8 @@ echo "GTK suppor
Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT
On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote:
> On 21/10/2015 20:36, Jordan Justen wrote:
> > On 2015-10-20 11:14:00, Laszlo Ersek wrote:
> > > Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the
> > > ich9_apm_ctrl_changed() ioport write callback function such that it would
> > > inject the SMI, in response to a write to the APM_CNT register, on the
> > > first CPU, invariably.
> > >
> > > Since this register is used by guest code to trigger an SMI synchronously,
> > > the interrupt should be injected on the VCPU that is performing the write.
> >
> > Why not send an SMI to *all* processors, like the real chipsets do?
>
> That's much less scalable, and more important I would have to check that
> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't
> relocate SMBASEs.
SeaBIOS is only expecting its SMI handler to be called once in
response to a synchronous SMI. We can change SeaBIOS to fix that.
SeaBIOS does relocate the smbase from 0x3 to 0xa during its
init phase (by creating a synchronous SMI on the BSP and then setting
the smbase register to 0xa in the smi handler).
-Kevin
Re: [Qemu-devel] [PATCH] target-tilegx: Implement prefetch instructions in pipe y2
On 10/20/2015 05:26 AM, Chen Gang wrote: From 14fe2a651b3f5729f1d402dfcd6eb5f7da0f42b1 Mon Sep 17 00:00:00 2001 From: Chen Gang Date: Tue, 20 Oct 2015 23:19:02 +0800 Subject: [PATCH] target-tilegx: Implement prefetch instructions in pipe y2 Originally, tilegx qemu only implement prefetch instructions in pipe x1, did not implement them in pipe y2. Signed-off-by: Chen Gang Applied. r~
Re: [Qemu-devel] [PULL 00/10] collected tcg patches
On 22 October 2015 at 12:02, Peter Maydell wrote: > Hi. I'm going to hold off on processing this pull for a few days > in the hope that the gcc compile farm's ppc64be box is working > again... ...now applied, thanks. (The folks behind the gcc cfarm did a very fast job with getting the packages installed I requested, so thanks to them as well.) -- PMM
Re: [Qemu-devel] [PATCH] copy, dd: simplify and optimize NUL bytes detection
2015-10-22 18:14+0200, Paolo Bonzini:
> On 22/10/2015 18:02, Eric Blake wrote:
>> I see a bug in there:
>
> Of course. You shouldn't have told me what the bug was, I deserved
> to look for it myself. :)
It rather seems that you don't want spoilers, :)
I see two bugs now.
> bool memeqzero4_paolo(const void *data, size_t length)
> {
> const unsigned char *p = data;
> unsigned long word;
>
> while (__builtin_expect(length & (sizeof(word) - 1), 0)) {
> if (*p)
> return false;
> p++;
> length--;
> if (!length)
> return true;
> }
>
> /* We must always read one byte or word, even if everything is aligned!
> * Otherwise, memcmp(data, data, length) is trivially true.
> */
> for (;;) {
> memcpy(&word, p, sizeof(word));
> if (word)
> return false;
> if (__builtin_expect(length & (16 - sizeof(word)), 0) == 0)
> break;
> p += sizeof(word);
> length -= sizeof(word);
> if (!length)
> return true;
> }
>
> /* Now we know that's zero, memcmp with self. */
> return memcmp(data, p, length) == 0;
> }
[Qemu-devel] [PATCH] virtio-9p: add savem handlers
We don't support migration of mounted 9p shares. This is handled by a
migration blocker.
One would expect, however, to be able to migrate if the share is unmounted.
Unfortunately virtio-9p-device does not register savevm handlers at all !
Migration succeeds and leaves the guest with a dangling device...
This patch simply registers migration handlers for virtio-9p-device. Whether
migration is possible or not still depends on the migration blocker.
Signed-off-by: Greg Kurz
---
Michael, Aneesh,
This is the same patch minus the call to unregister_savevm() since we don't
have an unrealize handler.
I decided to simply drop all the other patches. Hot-unplug support is totally
missing and definitely needs more work. I'll try to come up with a solution
in its own series.
Cheers.
--
Greg
---
hw/9pfs/virtio-9p-device.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 93a407c45926..e3abcfaffb2a 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -43,6 +43,16 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t
*config)
g_free(cfg);
}
+static void virtio_9p_save(QEMUFile *f, void *opaque)
+{
+virtio_save(VIRTIO_DEVICE(opaque), f);
+}
+
+static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
+{
+return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
+
static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
{
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -130,6 +140,7 @@ static void virtio_9p_device_realize(DeviceState *dev,
Error **errp)
}
v9fs_path_free(&path);
+register_savevm(dev, "virtio-9p", -1, 1, virtio_9p_save, virtio_9p_load,
s);
return;
out:
g_free(s->ctx.fs_root);
Re: [Qemu-devel] [PATCH 00/40] Patch Round-up for stable 2.4.1, freeze on 2015-10-29
On 10/21/2015 02:05 PM, Cole Robinson wrote: > On 10/21/2015 01:51 PM, Michael Roth wrote: >> Hi everyone, >> >> The following new patches are queued for QEMU stable v2.4.1: >> >> https://github.com/mdroth/qemu/commits/stable-2.4-staging >> >> The release is planned for 2015-11-03: >> >> http://wiki.qemu.org/Planning/2.4 >> >> Please respond here or CC qemu-sta...@nongnu.org on any patches you >> think should be included in the release. >> > Another potential: commit 98cf48f60aa4999f5b2808569a193a401a390e6a Author: Paolo Bonzini Date: Wed Sep 16 17:38:44 2015 +0200 trace: remove malloc tracing Prevents qemu from dropping this stderr warning with latest glib: (process:23283): GLib-WARNING **: gmem.c:482: custom memory allocation vtable not supported Not sure if it meets the stable criteria, but the error is annoying and I'll be adding that patch to the fedora builds Thanks, Cole
Re: [Qemu-devel] [PULL 0/8] QOM CPUState patch queue 2015-10-22
On 22 October 2015 at 17:22, Andreas Färber wrote: > Hello Peter, > > This is my QOM CPU patch queue. Please pull. > > Remaining maintainers should've had more than enough time to object or ack > now. > > Regards, > Andreas > > Cc: Peter Maydell > > Cc: Peter Crosthwaite > > The following changes since commit ca3e40e233e87f7b29442311736a82da01c0df7b: > > Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging > (2015-10-22 12:41:44 +0100) > > are available in the git repository at: > > git://github.com/afaerber/qemu-cpu.git tags/qom-cpu-for-peter > > for you to fetch changes up to 0960be7cffa7b30189f2f0f76b1ac3c8115660f3: > > disas: QOMify alpha specific disas setup (2015-10-22 15:49:40 +0200) > > > QOM CPUState and X86CPU > > * Adoption of CPUClass::disas_set_info() hook > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v2 1/5] mirror: keep an extra reference to the target image during the job
On Thu 22 Oct 2015 05:13:55 PM CEST, Alberto Garcia wrote: > During the 'drive-mirror' operation the target image only has the > monitor reference, therefore there's nothing that prevents its > deletion using the 'x-blockdev-del' command before the block job has > finished. Answering myself: it's probably better to use op blockers for this. Berto
Re: [Qemu-devel] cpu modelling and hotplug
Hi, Am 22.10.2015 um 03:27 schrieb Zhu Guihua: > May I know whether the discussion is still ongoing? We did have some discussions at KVM Forum, you may want to check the video recording of my CPU hot-plug talk (end was cut off, I think). > I checked Andreas's git tree, there was no changes about the topology. I've pushed the latest rebase, including machine->cpu_model change, but no other changes yet, and I did not re-review it at all beyond conflict resolution and "make check" testing. The first few patches may or may not be ready for a respin... > Plz let me know the schedule about this. I am currently occupied with other (mostly downstream) bugs and do not have a clear schedule to offer. We can still get patches in during the Soft Freeze if there is agreement on some subset. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH QEMU-XEN v4 1/9] xen_console: correctly cleanup primary console on teardown.
On Wed, 21 Oct 2015, Ian Campbell wrote:
> All of the work in con_disconnect applies to the primary console case
> (when xendev->dev is NULL). Therefore remove the early check and bail
> and allow it to fall through. All of the existing code is correctly
> conditional already.
>
> The ->dev and ->gnttabdev handles are either both set or neither. For
> consistency with con_initialise() with to the former here too.
>
> With this con_initialise and con_disconnect now mirror each other.
>
> Fix up a hard tab in the function while editing.
>
> Signed-off-by: Ian Campbell
Reviewed-by: Stefano Stabellini
> v4: New patch based on feedback to "xen: Switch uses of
> xc_map_foreign_bulk to use libxenforeignmemory API."
> ---
> hw/char/xen_console.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index eb7f450..63ade33 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -265,9 +265,6 @@ static void con_disconnect(struct XenDevice *xendev)
> {
> struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
>
> -if (!xendev->dev) {
> -return;
> -}
> if (con->chr) {
> qemu_chr_add_handlers(con->chr, NULL, NULL, NULL, NULL);
> qemu_chr_fe_release(con->chr);
> @@ -275,12 +272,12 @@ static void con_disconnect(struct XenDevice *xendev)
> xen_be_unbind_evtchn(&con->xendev);
>
> if (con->sring) {
> -if (!xendev->gnttabdev) {
> +if (!xendev->dev) {
> munmap(con->sring, XC_PAGE_SIZE);
> } else {
> xc_gnttab_munmap(xendev->gnttabdev, con->sring, 1);
> }
> - con->sring = NULL;
> +con->sring = NULL;
> }
> }
>
> --
> 2.1.4
>
Re: [Qemu-devel] [PATCH v9 15/17] tpm: Convert to new qapi union layout
On 10/22/2015 08:26 AM, Eric Blake wrote: >> PATCH 08-15 appear to be a purely mechanical switch to u. and from kind >> to type, except for a qapi.py hunk that looks like it should be in PATCH >> 07, and a comment update to tests/qapi-schema/union-clash-type.json. >> Did I miss anything? >> >> Combined diffstat isn't so bad: >> >> 36 files changed, 393 insertions(+), 394 deletions(-) > > It already needs a rebase; some of Dan's work has caused more changes to > ui/vnc.c and util/qemu-sockets.c. So hopefully I post v10 soon. > >> >> I've seen worse tree-wide changes, some of them my own. I'd be tempted >> to squash the complete switch together. But squashing is easy, so we >> can keep it separate while we review, and decide when we're done. > > Sure, v10 will keep things separate, but squashing won't hurt too much. > After all, v5 had it all as one patch. Just so I'm clear, if we wanted to squash, would it be just 8-15 (just the mechanical changes, but keeping the front-end scaffolding hack and backend cleanup, and keeping non-mechanical changes split off of 7 and 8 as a separate patch), or the entire 7-16 (no hack at all, and nothing to split off of 7 and 8)? -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 9/9] kvm/x86: Hyper-V kvm exit
On 22/10/2015 18:10, Andrey Smetanin wrote: > A new vcpu exit is introduced to notify the userspace of the > changes in Hyper-V SynIC configuration triggered by guest writing to the > corresponding MSRs. > > Changes v3: > * added KVM_EXIT_HYPERV types and structs notes into docs Thanks. The changes look good. I look forward to the unit tests so I can merge it. Paolo
