[Qemu-devel] Behavior of QMP "query-block"
Hello all, Summary I am using XEN hypervisor to run a HVM with a QEMU backed disk. After I start the HVM I use QMP "query-block" command to see the devices of the VM. Initially the command returns the disk that I set as part of the configuration. After a few seconds the a DEVICE_DELETE event occurs (for device nic0) and after that the "query-block" command returns an empty result. QEMU Version: 4.6.5 Arch: x86-64 QMP : 2.2.1 Note: 1. I was not performing any activity on the VM when the event occurs. 2. After the event, the QEMU process is still alive and the VM runs normally. 3. This behavior is consistently reproduced. Any help on how can I debug this issue would be greatly appreciated . Thanks, Bruno DETAILED LOGS I am using XEN hypervisor to run a HVM with QEMU backed disk. When I start the HVM I see the following QEMU process started: root 2199 1 0 18:57 ?00:00:02 /usr/local/lib/xen/bin/qemu-system-i386 -xen-domid 3 -chardev socket,id=libxl-cmd,path=/var/ run/xen/qmp-libxl-3,server,nowait -no-shutdown -mon chardev=libxl-cmd,mode=control -chardev socket,id=libxenstat-cmd,path= /var/run/xen/qmp-libxenstat-3,server,nowait -mon chardev=libxenstat-cmd,mode=control -nodefaults -no-user-config -name debianL2 -vnc :0,to=99 -display none -serial pty -device cirrus-vga,vgamem_mb=8 -boot order=d -device rtl8139,id=nic0,netdev=net0,mac=00:16:3e:1b:d0:7e -netdev type=tap,id=net0,ifname=vif3.0-emu,script=no,downscript=no -machine xenfv -m 1016 -drive file=/home/balvisio/debian- disk.img,if=ide,index=0,media=disk,format=raw,cache=writeback -drive if=ide,index=2,readonly=on,media=cdrom,id=ide-5632,file=/ home/balvisio/debian-live-8.7.1-amd64-gnome-desktop.iso,format=raw After launching the VM, I connected to the QMP socket: # rlwrap -C qmp socat STDIO UNIX:/var/run/xen/qmp-libxl-2 {"QMP": {"version": {"qemu": {"micro": 1, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}} {"execute":"qmp_capabilities"} {"return": {}} I issue the "query-block" command and I get: {"execute":"query-block"} {"return": [{"io-status": "ok", "device": "ide0-hd0", "locked": false, "removable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 10737418240, "filename": "/home/balvisio/debian-disk.img", "format": "raw", "actual-size": 4940075008, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "raw", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "/home/balvisio/debian-disk.img", "encryption_key_missing": false}, "type": "unknown"}]} After a few seconds, the following event happens asynchronoausly: {"timestamp": {"seconds": 1495590969, "microseconds": 898981}, "event": "DEVICE_DELETED", "data": {"device": "nic0", "path": "/machine/peripheral/nic0"}} After that event, I perform a "query-block" command again but now the block device is gone: {"execute":"query-block"} {"return": []} Xen Config File Used kernel="/usr/local/lib/xen/boot/hvmloader" builder='hvm' memory=1024 vcpus=1 name="debianL2" vfb = ['type=vnc'] vif= ['bridge=xenbr0'] boot='b' disk=['file:/home/balvisio/debian-disk.img,xvda,w'] acpi=1 device_model_version='qemu-xen' serial='pty' vnc=1 vnclisten="" vncpasswd=""
[Qemu-devel] [PULL 16/18] hw/ppc: migrating the DRC state of hotplugged devices
From: Daniel Henrique BarbozaIn pseries, a firmware abstraction called Dynamic Reconfiguration Connector (DRC) is used to assign a particular dynamic resource to the guest and provide an interface to manage configuration/removal of the resource associated with it. In other words, DRC is the 'plugged state' of a device. Before this patch, DRC wasn't being migrated. This causes post-migration problems due to DRC state mismatch between source and target. The DRC state of a device X in the source might change, while in the target the DRC state of X is still fresh. When migrating the guest, X will not have the same hotplugged state as it did in the source. This means that we can't hot unplug X in the target after migration is completed because its DRC state is not consistent. https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1677552 is one bug that is caused by this DRC state mismatch between source and target. To migrate the DRC state, we defined the VMStateDescription struct for spapr_drc to enable the transmission of spapr_drc state in migration. Not all the elements in the DRC state are migrated - only those that can be modified by guest actions or device add/remove operations: - 'isolation_state', 'allocation_state' and 'indicator_state' are involved in the DR state transition diagram from PAPR+ 2.7, 13.4; - 'configured', 'signalled', 'awaiting_release' and 'awaiting_allocation' are needed in attaching and detaching devices; - 'indicator_state' provides users with hardware state information. These are the DRC elements that are migrated. In this patch the DRC state is migrated for PCI, LMB and CPU connector types. At this moment there is no support to migrate DRC for the PHB (PCI Host Bridge) type. In the 'realize' function the DRC is registered using vmstate_register, similar to what hw/ppc/spapr_iommu.c does in 'spapr_tce_table_realize'. This approach works because DRCs are bus-less and do not sit on a BusClass that implements bc->get_dev_path, so as a fallback the VMSD gets identified via "spapr_drc"/get_index(drc). Signed-off-by: Daniel Henrique Barboza Signed-off-by: David Gibson --- hw/ppc/spapr_drc.c | 56 ++ 1 file changed, 56 insertions(+) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 2851e16..cc2400b 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -519,6 +519,60 @@ static void reset(DeviceState *d) } } +static bool spapr_drc_needed(void *opaque) +{ +sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque; +sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); +bool rc = false; +sPAPRDREntitySense value; +drck->entity_sense(drc, ); + +/* If no dev is plugged in there is no need to migrate the DRC state */ +if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) { +return false; +} + +/* + * If there is dev plugged in, we need to migrate the DRC state when + * it is different from cold-plugged state + */ +switch (drc->type) { +case SPAPR_DR_CONNECTOR_TYPE_PCI: +rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) && + (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) && + drc->configured && drc->signalled && !drc->awaiting_release); +break; +case SPAPR_DR_CONNECTOR_TYPE_CPU: +case SPAPR_DR_CONNECTOR_TYPE_LMB: +rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) && + (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) && + drc->configured && drc->signalled && !drc->awaiting_release); +break; +case SPAPR_DR_CONNECTOR_TYPE_PHB: +case SPAPR_DR_CONNECTOR_TYPE_VIO: +default: +g_assert(false); +} +return rc; +} + +static const VMStateDescription vmstate_spapr_drc = { +.name = "spapr_drc", +.version_id = 1, +.minimum_version_id = 1, +.needed = spapr_drc_needed, +.fields = (VMStateField []) { +VMSTATE_UINT32(isolation_state, sPAPRDRConnector), +VMSTATE_UINT32(allocation_state, sPAPRDRConnector), +VMSTATE_UINT32(indicator_state, sPAPRDRConnector), +VMSTATE_BOOL(configured, sPAPRDRConnector), +VMSTATE_BOOL(awaiting_release, sPAPRDRConnector), +VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector), +VMSTATE_BOOL(signalled, sPAPRDRConnector), +VMSTATE_END_OF_LIST() +} +}; + static void realize(DeviceState *d, Error **errp) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); @@ -547,6 +601,8 @@ static void realize(DeviceState *d, Error **errp) object_unref(OBJECT(drc)); } g_free(child_name); +vmstate_register(DEVICE(drc), drck->get_index(drc), _spapr_drc, + drc); trace_spapr_drc_realize_complete(drck->get_index(drc)); } -- 2.9.4
[Qemu-devel] [PULL 17/18] hw/ppc/spapr.c: recover pending LMB unplug info in spapr_lmb_release
From: Daniel Henrique BarbozaWhen a LMB hot unplug starts, the current DRC LMB status is stored at spapr->pending_dimm_unplugs QTAILQ. This queue isn't migrated, thus if a migration occurs in the middle of a LMB unplug the spapr_lmb_release callback will lost track of the LMB unplug progress. This patch implements a new recover function spapr_recover_pending_dimm_state that is used inside spapr_lmb_release to recover this DRC LMB release status that is lost during the migration. Signed-off-by: Daniel Henrique Barboza [dwg: Minor stylistic changes, simplify error handling] Signed-off-by: David Gibson --- hw/ppc/spapr.c | 43 ++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 14399f4..ab3aab1 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2655,6 +2655,40 @@ static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr, g_free(dimm_state); } +static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, +PCDIMMDevice *dimm) +{ +sPAPRDRConnector *drc; +PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); +MemoryRegion *mr = ddc->get_memory_region(dimm); +uint64_t size = memory_region_size(mr); +uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; +uint32_t avail_lmbs = 0; +uint64_t addr_start, addr; +int i; +sPAPRDIMMState *ds; + +addr_start = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, + _abort); + +addr = addr_start; +for (i = 0; i < nr_lmbs; i++) { +drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, + addr / SPAPR_MEMORY_BLOCK_SIZE); +g_assert(drc); +if (drc->indicator_state != SPAPR_DR_INDICATOR_STATE_INACTIVE) { +avail_lmbs++; +} +addr += SPAPR_MEMORY_BLOCK_SIZE; +} + +ds = g_malloc0(sizeof(sPAPRDIMMState)); +ds->nr_lmbs = avail_lmbs; +ds->dimm = dimm; +spapr_pending_dimm_unplugs_add(ms, ds); +return ds; +} + /* Callback to be called during DRC release. */ void spapr_lmb_release(DeviceState *dev) { @@ -2662,7 +2696,14 @@ void spapr_lmb_release(DeviceState *dev) sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl); sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); -if (--ds->nr_lmbs) { +/* This information will get lost if a migration occurs + * during the unplug process. In this case recover it. */ +if (ds == NULL) { +ds = spapr_recover_pending_dimm_state(spapr, PC_DIMM(dev)); +if (ds->nr_lmbs) { +return; +} +} else if (--ds->nr_lmbs) { return; } -- 2.9.4
[Qemu-devel] [PULL 14/18] hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState
The LMB DRC release callback, spapr_lmb_release(), uses an opaque parameter, a sPAPRDIMMState struct that stores the current LMBs that are allocated to a DIMM (nr_lmbs). After each call to this callback, the nr_lmbs is decremented by one and, when it reaches zero, the callback proceeds with the qdev calls to hot unplug the LMB. Using drc->detach_cb_opaque is problematic because it can't be migrated in the future DRC migration work. This patch makes the following changes to eliminate the usage of this opaque callback inside spapr_lmb_release: - sPAPRDIMMState was moved from spapr.c and added to spapr.h. A new attribute called 'addr' was added to it. This is used as an unique identifier to associate a sPAPRDIMMState to a PCDIMM element. - sPAPRMachineState now hosts a new QTAILQ called 'pending_dimm_unplugs'. This queue of sPAPRDIMMState elements will store the DIMM state of DIMMs that are currently going under an unplug process. - spapr_lmb_release() will now retrieve the nr_lmbs value by getting the correspondent sPAPRDIMMState. A helper function called spapr_dimm_get_address was created to fetch the address of a PCDIMM device inside spapr_lmb_release. When nr_lmbs reaches zero and the callback proceeds with the qdev hot unplug calls, the sPAPRDIMMState struct is removed from spapr->pending_dimm_unplugs. After these changes, the opaque argument for spapr_lmb_release is now unused and is passed as NULL inside spapr_del_lmbs. This and the other opaque arguments can now be safely removed from the code. As an additional cleanup made by this patch, the spapr_del_lmbs function was merged with spapr_memory_unplug_request. The former was being called only by the latter and both were small enough to fit one single function. Signed-off-by: Daniel Henrique Barboza[dwg: Minor stylistic cleanups] Signed-off-by: David Gibson --- hw/ppc/spapr.c | 105 +++-- include/hw/ppc/spapr.h | 6 +++ 2 files changed, 73 insertions(+), 38 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 3760d37..3a79dab 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2059,6 +2059,7 @@ static void ppc_spapr_init(MachineState *machine) msi_nonbroken = true; QLIST_INIT(>phbs); +QTAILQ_INIT(>pending_dimm_unplugs); /* Allocate RMA if necessary */ rma_alloc_size = kvmppc_alloc_rma(); @@ -2621,58 +2622,58 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, } } -typedef struct sPAPRDIMMState { +struct sPAPRDIMMState { +PCDIMMDevice *dimm; uint32_t nr_lmbs; -} sPAPRDIMMState; +QTAILQ_ENTRY(sPAPRDIMMState) next; +}; + +static sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *s, + PCDIMMDevice *dimm) +{ +sPAPRDIMMState *dimm_state = NULL; + +QTAILQ_FOREACH(dimm_state, >pending_dimm_unplugs, next) { +if (dimm_state->dimm == dimm) { +break; +} +} +return dimm_state; +} + +static void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr, + sPAPRDIMMState *dimm_state) +{ +g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->dimm)); +QTAILQ_INSERT_HEAD(>pending_dimm_unplugs, dimm_state, next); +} + +static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr, + sPAPRDIMMState *dimm_state) +{ +QTAILQ_REMOVE(>pending_dimm_unplugs, dimm_state, next); +g_free(dimm_state); +} static void spapr_lmb_release(DeviceState *dev, void *opaque) { -sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; -HotplugHandler *hotplug_ctrl; +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev); +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl); +sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); if (--ds->nr_lmbs) { return; } -g_free(ds); +spapr_pending_dimm_unplugs_remove(spapr, ds); /* * Now that all the LMBs have been removed by the guest, call the * pc-dimm unplug handler to cleanup up the pc-dimm device. */ -hotplug_ctrl = qdev_get_hotplug_handler(dev); hotplug_handler_unplug(hotplug_ctrl, dev, _abort); } -static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, - Error **errp) -{ -sPAPRDRConnector *drc; -sPAPRDRConnectorClass *drck; -uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; -int i; -sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState)); -uint64_t addr = addr_start; - -ds->nr_lmbs = nr_lmbs; -for (i = 0; i < nr_lmbs; i++) { -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, -addr / SPAPR_MEMORY_BLOCK_SIZE); -g_assert(drc); - -drck =
[Qemu-devel] [PULL 13/18] spapr: add pre_plug function for memory
From: Laurent VivierThis allows to manage errors before the memory has started to be hotplugged. We already have the function for the CPU cores. Signed-off-by: Laurent Vivier Reviewed-by: Greg Kurz [dwg: Fixed a couple of style nits] Signed-off-by: David Gibson --- hw/ppc/spapr.c | 41 ++--- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index bcb0e18..3760d37 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2578,20 +2578,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, uint64_t align = memory_region_get_alignment(mr); uint64_t size = memory_region_size(mr); uint64_t addr; -char *mem_dev; - -if (size % SPAPR_MEMORY_BLOCK_SIZE) { -error_setg(_err, "Hotplugged memory size must be a multiple of " - "%lld MB", SPAPR_MEMORY_BLOCK_SIZE/M_BYTE); -goto out; -} - -mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); -if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { -error_setg(_err, "Memory backend has bad page size. " - "Use 'memory-backend-file' with correct mem-path."); -goto out; -} pc_dimm_memory_plug(dev, >hotplug_memory, mr, align, _err); if (local_err) { @@ -2612,6 +2598,29 @@ out: error_propagate(errp, local_err); } +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, + Error **errp) +{ +PCDIMMDevice *dimm = PC_DIMM(dev); +PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); +MemoryRegion *mr = ddc->get_memory_region(dimm); +uint64_t size = memory_region_size(mr); +char *mem_dev; + +if (size % SPAPR_MEMORY_BLOCK_SIZE) { +error_setg(errp, "Hotplugged memory size must be a multiple of " + "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); +return; +} + +mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL); +if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) { +error_setg(errp, "Memory backend has bad page size. " + "Use 'memory-backend-file' with correct mem-path."); +return; +} +} + typedef struct sPAPRDIMMState { uint32_t nr_lmbs; } sPAPRDIMMState; @@ -3006,7 +3015,9 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { -if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { +if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { +spapr_memory_pre_plug(hotplug_dev, dev, errp); +} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { spapr_core_pre_plug(hotplug_dev, dev, errp); } } -- 2.9.4
[Qemu-devel] [PULL 18/18] xics: add unrealize handler
From: Greg KurzNow that ICPState objects get finalized on CPU unplug, we should unregister reset handlers as well to avoid a QEMU crash at machine reset time. Signed-off-by: Greg Kurz Signed-off-by: David Gibson --- hw/intc/xics.c | 5 + hw/intc/xics_kvm.c | 6 ++ 2 files changed, 11 insertions(+) diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 292fffe..ea35167 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -357,6 +357,10 @@ static void icp_realize(DeviceState *dev, Error **errp) qemu_register_reset(icp_reset, dev); } +static void icp_unrealize(DeviceState *dev, Error **errp) +{ +qemu_unregister_reset(icp_reset, dev); +} static void icp_class_init(ObjectClass *klass, void *data) { @@ -364,6 +368,7 @@ static void icp_class_init(ObjectClass *klass, void *data) dc->vmsd = _icp_server; dc->realize = icp_realize; +dc->unrealize = icp_unrealize; } static const TypeInfo icp_info = { diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c index dd7f298..14b8f6f 100644 --- a/hw/intc/xics_kvm.c +++ b/hw/intc/xics_kvm.c @@ -164,12 +164,18 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp) qemu_register_reset(icp_kvm_reset, dev); } +static void icp_kvm_unrealize(DeviceState *dev, Error **errp) +{ +qemu_unregister_reset(icp_kvm_reset, dev); +} + static void icp_kvm_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); ICPStateClass *icpc = ICP_CLASS(klass); dc->realize = icp_kvm_realize; +dc->unrealize = icp_kvm_unrealize; icpc->pre_save = icp_get_kvm_state; icpc->post_load = icp_set_kvm_state; icpc->cpu_setup = icp_kvm_cpu_setup; -- 2.9.4
[Qemu-devel] [PULL 15/18] hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque
From: Daniel Henrique BarbozaThe pointer drc->detach_cb is being used as a way of informing the detach() function inside spapr_drc.c which cb to execute. This information can also be retrieved simply by checking drc->type and choosing the right callback based on it. In this context, detach_cb is redundant information that must be managed. After the previous spapr_lmb_release change, no detach_cb_opaques are being used by any of the three callbacks functions. This is yet another information that is now unused and, on top of that, can't be migrated either. This patch makes the following changes: - removal of detach_cb_opaque. the 'opaque' argument was removed from the callbacks and from the detach() function of sPAPRConnectorClass. The attribute detach_cb_opaque of sPAPRConnector was removed. - removal of detach_cb from the detach() call. The function pointer detach_cb of sPAPRConnector was removed. detach() now uses a switch(drc->type) to execute the apropriate callback. To achieve this, spapr_core_release, spapr_lmb_release and spapr_phb_remove_pci_device_cb callbacks were made public to be visible inside detach(). Signed-off-by: Daniel Henrique Barboza Signed-off-by: David Gibson --- hw/ppc/spapr.c | 10 ++ hw/ppc/spapr_drc.c | 36 hw/ppc/spapr_pci.c | 5 +++-- include/hw/pci-host/spapr.h | 3 +++ include/hw/ppc/spapr.h | 4 include/hw/ppc/spapr_drc.h | 8 +--- 6 files changed, 37 insertions(+), 29 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 3a79dab..14399f4 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2655,7 +2655,8 @@ static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr, g_free(dimm_state); } -static void spapr_lmb_release(DeviceState *dev, void *opaque) +/* Callback to be called during DRC release. */ +void spapr_lmb_release(DeviceState *dev) { HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev); sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl); @@ -2720,7 +2721,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, g_assert(drc); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); -drck->detach(drc, dev, spapr_lmb_release, NULL, errp); +drck->detach(drc, dev, errp); addr += SPAPR_MEMORY_BLOCK_SIZE; } @@ -2767,7 +2768,8 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, object_unparent(OBJECT(dev)); } -static void spapr_core_release(DeviceState *dev, void *opaque) +/* Callback to be called during DRC release. */ +void spapr_core_release(DeviceState *dev) { HotplugHandler *hotplug_ctrl; @@ -2800,7 +2802,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, g_assert(drc); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); -drck->detach(drc, dev, spapr_core_release, NULL, _err); +drck->detach(drc, dev, _err); if (local_err) { error_propagate(errp, local_err); return; diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 9fa5545..2851e16 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -20,6 +20,7 @@ #include "qapi/visitor.h" #include "qemu/error-report.h" #include "hw/ppc/spapr.h" /* for RTAS return codes */ +#include "hw/pci-host/spapr.h" /* spapr_phb_remove_pci_device_cb callback */ #include "trace.h" #define DRC_CONTAINER_PATH "/dr-connector" @@ -99,8 +100,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, if (drc->awaiting_release) { if (drc->configured) { trace_spapr_drc_set_isolation_state_finalizing(get_index(drc)); -drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, - drc->detach_cb_opaque, NULL); +drck->detach(drc, DEVICE(drc->dev), NULL); } else { trace_spapr_drc_set_isolation_state_deferring(get_index(drc)); } @@ -153,8 +153,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc, if (drc->awaiting_release && drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { trace_spapr_drc_set_allocation_state_finalizing(get_index(drc)); -drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, - drc->detach_cb_opaque, NULL); +drck->detach(drc, DEVICE(drc->dev), NULL); } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) { drc->awaiting_allocation = false; } @@ -404,15 +403,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt, NULL, 0, NULL); } -static void detach(sPAPRDRConnector *drc, DeviceState *d, - spapr_drc_detach_cb *detach_cb, - void *detach_cb_opaque,
[Qemu-devel] [PULL 09/18] spapr_cpu_core: drop reference on ICP object during CPU realization
From: Greg KurzWhen a piece of code allocates an object, it implicitely gets a reference on it. If it then makes that object a child property of another object, it should drop its own reference at some point otherwise the child object can never be finalized. The current code hence leaks one ICP object per CPU when hot-removing a core. Failing to add a newly allocated ICP object to the CPU is a bug. While here, let's ensure QEMU aborts if this ever happens. Signed-off-by: Greg Kurz Signed-off-by: David Gibson --- hw/ppc/spapr_cpu_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index 1df1404..ff7058e 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -143,7 +143,8 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp) Object *obj; obj = object_new(spapr->icp_type); -object_property_add_child(OBJECT(cpu), "icp", obj, NULL); +object_property_add_child(OBJECT(cpu), "icp", obj, _abort); +object_unref(obj); object_property_add_const_link(obj, "xics", OBJECT(spapr), _abort); object_property_set_bool(obj, true, "realized", _err); if (local_err) { -- 2.9.4
[Qemu-devel] [PULL 10/18] spapr: fix error reporting in xics_system_init()
From: Greg KurzIf the user explicitely asked for kernel-irqchip support and "xics-kvm" initialization fails, we shouldn't fallback to emulated "xics" as we do now. It is also awkward to print an error message when we have an errp pointer argument. Let's use the errp argument to report the error and let the caller decide. This simplifies the code as we don't need a local Error * here. Signed-off-by: Greg Kurz Signed-off-by: David Gibson --- hw/ppc/spapr.c | 13 ++--- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index c912eaa..c92d269 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -128,18 +128,14 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) sPAPRMachineState *spapr = SPAPR_MACHINE(machine); if (kvm_enabled()) { -Error *err = NULL; - if (machine_kernel_irqchip_allowed(machine) && !xics_kvm_init(spapr, errp)) { spapr->icp_type = TYPE_KVM_ICP; -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, ); +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp); } if (machine_kernel_irqchip_required(machine) && !spapr->ics) { -error_reportf_err(err, - "kernel_irqchip requested but unavailable: "); -} else { -error_free(err); +error_prepend(errp, "kernel_irqchip requested but unavailable: "); +return; } } @@ -147,6 +143,9 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) xics_spapr_init(spapr); spapr->icp_type = TYPE_ICP; spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp); +if (!spapr->ics) { +return; +} } } -- 2.9.4
[Qemu-devel] [PULL 12/18] pseries: Restore support for total vcpus not a multiple of threads-per-core for old machine types
As of pseries-2.7 and later, we require the total number of guest vcpus to be a multiple of the threads-per-core. pseries-2.6 and earlier machine types, however, are supposed to allow this for the sake of migration from old qemu versions which allowed this. Unfortunately, 8149e29 "pseries: Enforce homogeneous threads-per-core" broke this by not considering the old machine type case. This fixes it by only applying the check when the machine type supports hotpluggable cpus. By not-entirely-coincidence, that corresponds to the same time when we started enforcing total threads being a multiple of threads-per-core. Fixes: 8149e2992f7811355cc34721b79d69d1a3a667dd Signed-off-by: David GibsonReviewed-by: Laurent Vivier Reviewed-by: Greg Kurz Tested-by: Greg Kurz --- hw/ppc/spapr.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index c92d269..bcb0e18 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2863,7 +2863,13 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, goto out; } -if (cc->nr_threads != smp_threads) { +/* + * In general we should have homogeneous threads-per-core, but old + * (pre hotplug support) machine types allow the last core to have + * reduced threads as a compatibility hack for when we allowed + * total vcpus not a multiple of threads-per-core. + */ +if (mc->has_hotpluggable_cpus && (cc->nr_threads != smp_threads)) { error_setg(errp, "invalid nr-threads %d, must be %d", cc->nr_threads, smp_threads); return; -- 2.9.4
[Qemu-devel] [PULL 11/18] pseries: Split CAS PVR negotiation out into a separate function
Guests of the qemu machine type go through a feature negotiation process known as "client architecture support" (CAS) during early boot. This does a number of things, one of which is finding a CPU compatibility mode which can be supported by both guest and host. In fact the CPU negotiation is probably the single most complex part of the CAS process, so this splits it out into a helper function. We've recently made some mistakes in maintaining backward compatibility for old machine types here. Splitting this out will also make it easier to fix this. This also adds a possibly useful error message if the negotiation fails (i.e. if there isn't a CPU mode that's suitable for both guest and host). Signed-off-by: David GibsonReviewed-by: Laurent Vivier Reviewed-by: Greg Kurz --- hw/ppc/spapr_hcall.c | 49 - 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 2daace4..77d2d66 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1044,19 +1044,13 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu, } } -static target_ulong h_client_architecture_support(PowerPCCPU *cpu, - sPAPRMachineState *spapr, - target_ulong opcode, - target_ulong *args) +static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr, + Error **errp) { -target_ulong list = ppc64_phys_to_real(args[0]); -target_ulong ov_table; bool explicit_match = false; /* Matched the CPU's real PVR */ uint32_t max_compat = cpu->max_compat; uint32_t best_compat = 0; int i; -sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates; -bool guest_radix; /* * We scan the supplied table of PVRs looking for two things @@ -1066,9 +1060,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, for (i = 0; i < 512; ++i) { uint32_t pvr, pvr_mask; -pvr_mask = ldl_be_phys(_space_memory, list); -pvr = ldl_be_phys(_space_memory, list + 4); -list += 8; +pvr_mask = ldl_be_phys(_space_memory, *addr); +pvr = ldl_be_phys(_space_memory, *addr + 4); +*addr += 8; if (~pvr_mask & pvr) { break; /* Terminator record */ @@ -1087,17 +1081,38 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, /* We couldn't find a suitable compatibility mode, and either * the guest doesn't support "raw" mode for this CPU, or raw * mode is disabled because a maximum compat mode is set */ -return H_HARDWARE; +error_setg(errp, "Couldn't negotiate a suitable PVR during CAS"); +return 0; } /* Parsing finished */ trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat); -/* Update CPUs */ -if (cpu->compat_pvr != best_compat) { -Error *local_err = NULL; +return best_compat; +} -ppc_set_compat_all(best_compat, _err); +static target_ulong h_client_architecture_support(PowerPCCPU *cpu, + sPAPRMachineState *spapr, + target_ulong opcode, + target_ulong *args) +{ +/* Working address in data buffer */ +target_ulong addr = ppc64_phys_to_real(args[0]); +target_ulong ov_table; +uint32_t cas_pvr; +sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates; +bool guest_radix; +Error *local_err = NULL; + +cas_pvr = cas_check_pvr(cpu, , _err); +if (local_err) { +error_report_err(local_err); +return H_HARDWARE; +} + +/* Update CPUs */ +if (cpu->compat_pvr != cas_pvr) { +ppc_set_compat_all(cas_pvr, _err); if (local_err) { error_report_err(local_err); return H_HARDWARE; @@ -1105,7 +1120,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, } /* For the future use: here @ov_table points to the first option vector */ -ov_table = list; +ov_table = addr; ov1_guest = spapr_ovec_parse_vector(ov_table, 1); ov5_guest = spapr_ovec_parse_vector(ov_table, 5); -- 2.9.4
[Qemu-devel] [PULL 07/18] spapr: ensure core_slot isn't NULL in spapr_core_unplug()
From: Greg KurzIf we go that far on the path of hot-removing a core and we find out that the core-id is invalid, then we have a serious bug. Let's make it explicit with an assert() instead of dereferencing a NULL pointer. This fixes Coverity issue CID 1375404. Signed-off-by: Greg Kurz Reviewed-by: Igor Mammedov Signed-off-by: David Gibson --- hw/ppc/spapr.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 35dceb0..c912eaa 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2725,6 +2725,7 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, CPUCore *cc = CPU_CORE(dev); CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL); +assert(core_slot); core_slot->cpu = NULL; object_unparent(OBJECT(dev)); } -- 2.9.4
[Qemu-devel] [PULL 06/18] xics_kvm: cache already enabled vCPU ids
From: Greg KurzSince commit a45863bda90d ("xics_kvm: Don't enable KVM_CAP_IRQ_XICS if already enabled"), we were able to re-hotplug a vCPU that had been hot- unplugged ealier, thanks to a boolean flag in ICPState that we set when enabling KVM_CAP_IRQ_XICS. This could work because the lifecycle of all ICPState objects was the same as the machine. Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under sPAPRCPUCore") broke this assumption and now we always pass a freshly allocated ICPState object (ie, with the flag unset) to icp_kvm_cpu_setup(). This cause re-hotplug to fail with: Unable to connect CPU8 to kernel XICS: Device or resource busy Let's fix this by caching all the vCPU ids for which KVM_CAP_IRQ_XICS was enabled. This also drops the now useless boolean flag from ICPState. Reported-by: Laurent Vivier Signed-off-by: Greg Kurz Tested-by: Laurent Vivier Reviewed-by: Laurent Vivier Reviewed-by: Cédric Le Goater Signed-off-by: David Gibson --- hw/intc/xics_kvm.c| 27 --- include/hw/ppc/xics.h | 1 - 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c index dd93531..dd7f298 100644 --- a/hw/intc/xics_kvm.c +++ b/hw/intc/xics_kvm.c @@ -42,6 +42,14 @@ static int kernel_xics_fd = -1; +typedef struct KVMEnabledICP { +unsigned long vcpu_id; +QLIST_ENTRY(KVMEnabledICP) node; +} KVMEnabledICP; + +static QLIST_HEAD(, KVMEnabledICP) +kvm_enabled_icps = QLIST_HEAD_INITIALIZER(_enabled_icps); + /* * ICP-KVM */ @@ -121,6 +129,8 @@ static void icp_kvm_reset(void *dev) static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) { CPUState *cs = CPU(cpu); +KVMEnabledICP *enabled_icp; +unsigned long vcpu_id = kvm_arch_vcpu_id(cs); int ret; if (kernel_xics_fd == -1) { @@ -132,18 +142,21 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) * which was hot-removed earlier we don't have to renable * KVM_CAP_IRQ_XICS capability again. */ -if (icp->cap_irq_xics_enabled) { -return; +QLIST_FOREACH(enabled_icp, _enabled_icps, node) { +if (enabled_icp->vcpu_id == vcpu_id) { +return; +} } -ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, - kvm_arch_vcpu_id(cs)); +ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id); if (ret < 0) { -error_report("Unable to connect CPU%ld to kernel XICS: %s", - kvm_arch_vcpu_id(cs), strerror(errno)); +error_report("Unable to connect CPU%ld to kernel XICS: %s", vcpu_id, + strerror(errno)); exit(1); } -icp->cap_irq_xics_enabled = true; +enabled_icp = g_malloc(sizeof(*enabled_icp)); +enabled_icp->vcpu_id = vcpu_id; +QLIST_INSERT_HEAD(_enabled_icps, enabled_icp, node); } static void icp_kvm_realize(DeviceState *dev, Error **errp) diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index d6cb51f..a3073f9 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -81,7 +81,6 @@ struct ICPState { uint8_t pending_priority; uint8_t mfrr; qemu_irq output; -bool cap_irq_xics_enabled; XICSFabric *xics; }; -- 2.9.4
[Qemu-devel] [PULL 08/18] hw/ppc/spapr_events.c: removing 'exception' from sPAPREventLogEntry
From: Daniel Henrique BarbozaCurrenty we do not have any RTAS event that is reported by the event-scan interface. The existing events, RTAS_LOG_TYPE_EPOW and RTAS_LOG_TYPE_HOTPLUG, are being reported by the check-exception interface and, as such, marked as 'exception=true'. Commit 79853e18d9, 'spapr_events: event-scan RTAS interface', added the event_scan interface because the guest kernel requires it to initialize other required interfaces. It is acting since then as a stub because no events that would be reported by it were added since then. However, the existence of the 'exception' boolean adds an unnecessary load in the future migration of the pending_events, sPAPREventLogEntry QTAILQ that hosts the pending RTAS events. To make the code cleaner and ease the future migration changes, this patch makes the following changes: - remove the 'exception' boolean that filter these events. There is nothing to filter since all events are reported by check-exception; - functions rtas_event_log_queue, rtas_event_log_dequeue and rtas_event_log_contains don't receive the 'exception' boolean as parameter; - event_scan function was simplified. It was calling 'rtas_event_log_dequeue(mask, false)' that was always returning 'NULL' because we have no events that are created with exception=false, thus in the end it would execute a jump to 'out_no_events' all the time. The function now assumes that this will always be the case and all the remaining logic were deleted. In the future, when or if we add new RTAS events that should be reported with the event_scan interface, we can refer to the changes made in this patch to add the event_scan logic back. Signed-off-by: Daniel Henrique Barboza Signed-off-by: David Gibson --- hw/ppc/spapr_events.c | 52 +++--- include/hw/ppc/spapr.h | 1 - 2 files changed, 7 insertions(+), 46 deletions(-) diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index f0b28d8..73e2a18 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -342,20 +342,18 @@ static int rtas_event_log_to_irq(sPAPRMachineState *spapr, int log_type) return source->irq; } -static void rtas_event_log_queue(int log_type, void *data, bool exception) +static void rtas_event_log_queue(int log_type, void *data) { sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1); g_assert(data); entry->log_type = log_type; -entry->exception = exception; entry->data = data; QTAILQ_INSERT_TAIL(>pending_events, entry, next); } -static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t event_mask, - bool exception) +static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t event_mask) { sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); sPAPREventLogEntry *entry = NULL; @@ -364,10 +362,6 @@ static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t event_mask, const sPAPREventSource *source = rtas_event_log_to_source(spapr, entry->log_type); -if (entry->exception != exception) { -continue; -} - if (source->mask & event_mask) { break; } @@ -380,7 +374,7 @@ static sPAPREventLogEntry *rtas_event_log_dequeue(uint32_t event_mask, return entry; } -static bool rtas_event_log_contains(uint32_t event_mask, bool exception) +static bool rtas_event_log_contains(uint32_t event_mask) { sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); sPAPREventLogEntry *entry = NULL; @@ -389,10 +383,6 @@ static bool rtas_event_log_contains(uint32_t event_mask, bool exception) const sPAPREventSource *source = rtas_event_log_to_source(spapr, entry->log_type); -if (entry->exception != exception) { -continue; -} - if (source->mask & event_mask) { return true; } @@ -479,7 +469,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque) epow->event_modifier = RTAS_LOG_V6_EPOW_MODIFIER_NORMAL; epow->extended_modifier = RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION_SPECIFIC; -rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true); +rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow); qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr), rtas_event_log_to_irq(spapr, @@ -572,7 +562,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action, cpu_to_be32(drc_id->count_indexed.index); } -rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true); +rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp); qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr), rtas_event_log_to_irq(spapr, @@ -667,7 +657,7 @@ static void
[Qemu-devel] [PULL 00/18] ppc-for-2.10 queue 20170525
The following changes since commit 9964e96dccf7f7c936ee854a795415d19b60: Merge remote-tracking branch 'jasowang/tags/net-pull-request' into staging (2017-05-23 15:01:31 +0100) are available in the git repository at: git://github.com/dgibson/qemu.git tags/ppc-for-2.10-20170525 for you to fetch changes up to 62f94fc94f98095173146e753a1f03d7c2cc7ba3: xics: add unrealize handler (2017-05-25 11:31:33 +1000) ppc patch queue 2017-05-25 Assorted accumulated patches. These are nearly all bugfixes at one level or another - some for longstanding problems, others for some regressions caused by more recent cleanups. This includes preliminary patches towards fixing migration for Radix Page Table guests under POWER9 and also fixing some migration regressions due to the re-organization of the interrupt controller code. Not all the pieces are there yet, so those still won't quite work, but the preliminary changes make sense on their own. Bharata B Rao (1): spapr: Consolidate HPT freeing code into a routine Daniel Henrique Barboza (4): hw/ppc/spapr_events.c: removing 'exception' from sPAPREventLogEntry hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque hw/ppc: migrating the DRC state of hotplugged devices hw/ppc/spapr.c: recover pending LMB unplug info in spapr_lmb_release David Gibson (3): pseries: Split CAS PVR negotiation out into a separate function pseries: Restore support for total vcpus not a multiple of threads-per-core for old machine types hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState Greg Kurz (8): ppc/xics: simplify prototype of xics_spapr_init() spapr: sanitize error handling in spapr_ics_create() spapr-cpu-core: release ICP object when realization fails xics_kvm: cache already enabled vCPU ids spapr: ensure core_slot isn't NULL in spapr_core_unplug() spapr_cpu_core: drop reference on ICP object during CPU realization spapr: fix error reporting in xics_system_init() xics: add unrealize handler Laurent Vivier (1): spapr: add pre_plug function for memory Nikunj A Dadhania (1): target/ppc: reset reservation in do_rfi() hw/intc/xics.c | 5 + hw/intc/xics_kvm.c | 33 -- hw/intc/xics_spapr.c| 3 +- hw/ppc/spapr.c | 239 +++- hw/ppc/spapr_cpu_core.c | 19 ++-- hw/ppc/spapr_drc.c | 92 ++--- hw/ppc/spapr_events.c | 52 ++ hw/ppc/spapr_hcall.c| 54 ++ hw/ppc/spapr_pci.c | 5 +- include/hw/pci-host/spapr.h | 3 + include/hw/ppc/spapr.h | 12 ++- include/hw/ppc/spapr_drc.h | 8 +- include/hw/ppc/xics.h | 3 +- target/ppc/excp_helper.c| 3 + 14 files changed, 349 insertions(+), 182 deletions(-)
[Qemu-devel] [PULL 05/18] spapr: Consolidate HPT freeing code into a routine
From: Bharata B RaoConsolidate the code that frees HPT into a separate routine spapr_free_hpt() as the same chunk of code is called from two places. Signed-off-by: Bharata B Rao Signed-off-by: David Gibson --- hw/ppc/spapr.c | 13 + hw/ppc/spapr_hcall.c | 5 + include/hw/ppc/spapr.h | 1 + 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index a9471b9..35dceb0 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1227,16 +1227,21 @@ static int spapr_hpt_shift_for_ramsize(uint64_t ramsize) return shift; } +void spapr_free_hpt(sPAPRMachineState *spapr) +{ +g_free(spapr->htab); +spapr->htab = NULL; +spapr->htab_shift = 0; +close_htab_fd(spapr); +} + static void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift, Error **errp) { long rc; /* Clean up any HPT info from a previous boot */ -g_free(spapr->htab); -spapr->htab = NULL; -spapr->htab_shift = 0; -close_htab_fd(spapr); +spapr_free_hpt(spapr); rc = kvmppc_reset_htab(shift); if (rc < 0) { diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 0d608d6..2daace4 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -913,10 +913,7 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState *spapr, /* We assume RADIX, so this catches all the "Do Nothing" cases */ } else if (!(patbe_old & PATBE1_GR)) { /* HASH->RADIX : Free HPT */ -g_free(spapr->htab); -spapr->htab = NULL; -spapr->htab_shift = 0; -close_htab_fd(spapr); +spapr_free_hpt(spapr); } else if (!(patbe_new & PATBE1_GR)) { /* RADIX->HASH || NOTHING->HASH : Allocate HPT */ spapr_setup_hpt_and_vrma(spapr); diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 5802f88..93c4cfc 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -610,6 +610,7 @@ int spapr_h_cas_compose_response(sPAPRMachineState *sm, sPAPROptionVector *ov5_updates); void close_htab_fd(sPAPRMachineState *spapr); void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr); +void spapr_free_hpt(sPAPRMachineState *spapr); sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn); void spapr_tce_table_enable(sPAPRTCETable *tcet, uint32_t page_shift, uint64_t bus_offset, -- 2.9.4
[Qemu-devel] [PULL 04/18] spapr-cpu-core: release ICP object when realization fails
From: Greg KurzWhile here we introduce a single error path to avoid code duplication. Signed-off-by: Greg Kurz Signed-off-by: David Gibson --- hw/ppc/spapr_cpu_core.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index a17ea07..1df1404 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -147,25 +147,25 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp) object_property_add_const_link(obj, "xics", OBJECT(spapr), _abort); object_property_set_bool(obj, true, "realized", _err); if (local_err) { -error_propagate(errp, local_err); -return; +goto error; } object_property_set_bool(child, true, "realized", _err); if (local_err) { -object_unparent(obj); -error_propagate(errp, local_err); -return; +goto error; } spapr_cpu_init(spapr, cpu, _err); if (local_err) { -object_unparent(obj); -error_propagate(errp, local_err); -return; +goto error; } xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj)); +return; + +error: +object_unparent(obj); +error_propagate(errp, local_err); } static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) -- 2.9.4
[Qemu-devel] [PULL 01/18] target/ppc: reset reservation in do_rfi()
From: Nikunj A DadhaniaFor transitioning back to userspace after the interrupt. Suggested-by: Richard Henderson Signed-off-by: Nikunj A Dadhania Signed-off-by: David Gibson --- target/ppc/excp_helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index a6bcb47..9cb2123 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -995,6 +995,9 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr) */ cs->interrupt_request |= CPU_INTERRUPT_EXITTB; +/* Reset the reservation */ +env->reserve_addr = -1; + /* Context synchronizing: check if TCG TLB needs flush */ check_tlb_flush(env, false); } -- 2.9.4
[Qemu-devel] [PULL 03/18] spapr: sanitize error handling in spapr_ics_create()
From: Greg KurzThe spapr_ics_create() function handles errors in a rather convoluted way, with two local Error * variables. Moreover, failing to parent the ICS object to the machine should be considered as a bug but it is currently ignored. This patch addresses both issues. Signed-off-by: Greg Kurz Signed-off-by: David Gibson --- hw/ppc/spapr.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 18709ad..a9471b9 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -101,21 +101,26 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr, const char *type_ics, int nr_irqs, Error **errp) { -Error *err = NULL, *local_err = NULL; +Error *local_err = NULL; Object *obj; obj = object_new(type_ics); -object_property_add_child(OBJECT(spapr), "ics", obj, NULL); +object_property_add_child(OBJECT(spapr), "ics", obj, _abort); object_property_add_const_link(obj, "xics", OBJECT(spapr), _abort); -object_property_set_int(obj, nr_irqs, "nr-irqs", ); +object_property_set_int(obj, nr_irqs, "nr-irqs", _err); +if (local_err) { +goto error; +} object_property_set_bool(obj, true, "realized", _err); -error_propagate(, local_err); -if (err) { -error_propagate(errp, err); -return NULL; +if (local_err) { +goto error; } return ICS_SIMPLE(obj); + +error: +error_propagate(errp, local_err); +return NULL; } static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) -- 2.9.4
[Qemu-devel] [PULL 02/18] ppc/xics: simplify prototype of xics_spapr_init()
From: Greg KurzThis function only does hypercall and RTAS-call registration, and thus never returns an error. This patch adapt the prototype to reflect that. Signed-off-by: Greg Kurz Reviewed-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: David Gibson --- hw/intc/xics_spapr.c | 3 +-- hw/ppc/spapr.c| 2 +- include/hw/ppc/xics.h | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c index f05308b8..d98ea8b 100644 --- a/hw/intc/xics_spapr.c +++ b/hw/intc/xics_spapr.c @@ -229,7 +229,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr, rtas_st(rets, 0, RTAS_OUT_SUCCESS); } -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp) +void xics_spapr_init(sPAPRMachineState *spapr) { /* Registration of global state belongs into realize */ spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive); @@ -243,7 +243,6 @@ int xics_spapr_init(sPAPRMachineState *spapr, Error **errp) spapr_register_hypercall(H_XIRR_X, h_xirr_x); spapr_register_hypercall(H_EOI, h_eoi); spapr_register_hypercall(H_IPOLL, h_ipoll); -return 0; } #define ICS_IRQ_FREE(ics, srcno) \ diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0980d73..18709ad 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -139,7 +139,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) } if (!spapr->ics) { -xics_spapr_init(spapr, errp); +xics_spapr_init(spapr); spapr->icp_type = TYPE_ICP; spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp); } diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index 05e6acb..d6cb51f 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -206,6 +206,6 @@ void icp_resend(ICPState *ss); typedef struct sPAPRMachineState sPAPRMachineState; int xics_kvm_init(sPAPRMachineState *spapr, Error **errp); -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp); +void xics_spapr_init(sPAPRMachineState *spapr); #endif /* XICS_H */ -- 2.9.4
[Qemu-devel] [PATCH] qtest: add rtc periodic timer test
From: Xiao GuangrongIt tests the accuracy of rtc periodic timer which is recently improved & fixed by: mc146818rtc: precisely count the clock for periodic timer (commit id has not been decided yet) Note: as qemu needs a precise timer to drive its rtc timer callbacks, that means clock=vm is not suitable for us as it's driven by icount for qtest, so that we use clock=host instead, it is why we put the periodic timer test separately without mixing with rtc-test Signed-off-by: Xiao Guangrong --- hw/timer/mc146818rtc.c | 15 ++- include/hw/timer/mc146818rtc.h | 19 tests/Makefile.include | 2 + tests/rtc-periodic-test.c | 100 + tests/rtc-test.c | 14 +- tests/rtc-test.h | 17 +++ 6 files changed, 142 insertions(+), 25 deletions(-) create mode 100644 tests/rtc-periodic-test.c create mode 100644 tests/rtc-test.h diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c index 6d0a610..0aba66c 100644 --- a/hw/timer/mc146818rtc.c +++ b/hw/timer/mc146818rtc.c @@ -120,7 +120,7 @@ static void rtc_coalesced_timer_update(RTCState *s) /* divide each RTC interval to 2 - 8 smaller intervals */ int c = MIN(s->irq_coalesced, 7) + 1; int64_t next_clock = qemu_clock_get_ns(rtc_clock) + -muldiv64(s->period / c, NANOSECONDS_PER_SECOND, RTC_CLOCK_RATE); +periodic_clock_to_ns(s->period / c); timer_mod(s->coalesced_timer, next_clock); } } @@ -178,16 +178,8 @@ static uint32_t rtc_periodic_clock_ticks(RTCState *s) } period_code = s->cmos_data[RTC_REG_A] & 0x0f; -if (!period_code) { -return 0; -} - -if (period_code <= 2) { -period_code += 7; -} -/* period in 32 Khz cycles */ -return 1 << (period_code - 1); +return periodic_period_to_clock(period_code); } /* @@ -266,8 +258,7 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) assert(lost_clock >= 0 && lost_clock <= period); next_irq_clock = cur_clock + period - lost_clock; -s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND, - RTC_CLOCK_RATE) + 1; +s->next_periodic_time = periodic_clock_to_ns(next_irq_clock) + 1; timer_mod(s->periodic_timer, s->next_periodic_time); } else { s->irq_coalesced = 0; diff --git a/include/hw/timer/mc146818rtc.h b/include/hw/timer/mc146818rtc.h index 7c8e64b..f23e734 100644 --- a/include/hw/timer/mc146818rtc.h +++ b/include/hw/timer/mc146818rtc.h @@ -10,4 +10,23 @@ ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq); void rtc_set_memory(ISADevice *dev, int addr, int val); int rtc_get_memory(ISADevice *dev, int addr); +static inline uint32_t periodic_period_to_clock(int period_code) +{ +if (!period_code) { +return 0; + } + +if (period_code <= 2) { +period_code += 7; +} +/* period in 32 Khz cycles */ + return 1 << (period_code - 1); +} + +#define RTC_CLOCK_RATE32768 + +static inline int64_t periodic_clock_to_ns(int64_t clocks) +{ +return muldiv64(clocks, NANOSECONDS_PER_SECOND, RTC_CLOCK_RATE); +} #endif /* MC146818RTC_H */ diff --git a/tests/Makefile.include b/tests/Makefile.include index 31931c0..6958d91 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -218,6 +218,7 @@ check-qtest-i386-y += tests/bios-tables-test$(EXESUF) check-qtest-i386-y += tests/boot-serial-test$(EXESUF) check-qtest-i386-y += tests/pxe-test$(EXESUF) check-qtest-i386-y += tests/rtc-test$(EXESUF) +check-qtest-i386-y += tests/rtc-periodic-test$(EXESUF) check-qtest-i386-y += tests/ipmi-kcs-test$(EXESUF) check-qtest-i386-y += tests/ipmi-bt-test$(EXESUF) check-qtest-i386-y += tests/i440fx-test$(EXESUF) @@ -677,6 +678,7 @@ libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virt tests/qmp-test$(EXESUF): tests/qmp-test.o tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o tests/rtc-test$(EXESUF): tests/rtc-test.o +tests/rtc-periodic-test$(EXESUF): tests/rtc-periodic-test.o tests/m48t59-test$(EXESUF): tests/m48t59-test.o tests/endianness-test$(EXESUF): tests/endianness-test.o tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y) diff --git a/tests/rtc-periodic-test.c b/tests/rtc-periodic-test.c new file mode 100644 index 000..8f3e4b5 --- /dev/null +++ b/tests/rtc-periodic-test.c @@ -0,0 +1,100 @@ +/* + * QTest testcase for the periodic timer of MC146818 real-time clock + * + * Copyright Tencent Corp. 2017 + * + * Authors: + * Xiao Guangrong + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "qemu/timer.h" +
Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/4] spapr: disable hotplugging without OS
On Wed, May 24, 2017 at 11:28:57AM +0200, Greg Kurz wrote: > On Wed, 24 May 2017 15:07:54 +1000 > David Gibsonwrote: > > > On Tue, May 23, 2017 at 01:18:11PM +0200, Laurent Vivier wrote: > > > If the OS is not started, QEMU sends an event to the OS > > > that is lost and cannot be recovered. An unplug is not > > > able to restore QEMU in a coherent state. > > > So, while the OS is not started, disable CPU and memory hotplug. > > > We use option vector 6 to know if the OS is started > > > > > > Signed-off-by: Laurent Vivier > > > > Urgh.. I'm not terribly confident that this is really correct. As > > discussed on the previous patch, you're essentially using OV6 as a > > flag that CAS is complete. > > > > But while it undoubtedly makes the race window much smaller, I don't > > see that there's any guarantee the guest OS will really be able to > > handle hotplug events immediately after CAS. > > > > In particular if the CAS process completes partially but then needs to > > trigger a reboot, I think that would end up setting the ov6 variable, > > but the OS would definitely not be in a state to accept events. > > > > We never have any guarantee that the OS will process an event that > we've sent actually (think of a kernel crash just after a successful > CAS negotiation for example, or any failure with the various guest > components involved in the process of hotplug). > > > Mike, I really think we need some input from someone familiar with how > > these hotplug events are supposed to work. What do we need to do to > > handle lost or stale events, such as those delivered when an OS is not > > booted. > > > > AFAIK, in the PowerVM world, the HMC exposes a user configurable timeout. > > https://www.ibm.com/support/knowledgecenter/POWER8/p8hat/p8hat_dlparprocpoweraddp6.htm > > I'm not sure we can do anything better than being able to "cancel" a previous > hotplug attempt if it takes too long, but I'm not necessarily the expert > you're > looking for :) Right, but at the moment we *don't* have a way to cancel a previous hotplug attempt. Trying to remove again ends up with things in a tangle. > > > > --- > > > hw/ppc/spapr.c | 22 +++--- > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index eceb4cc..2e9320d 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -2625,6 +2625,7 @@ out: > > > static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, > > > DeviceState *dev, > > > Error **errp) > > > { > > > +sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > > > PCDIMMDevice *dimm = PC_DIMM(dev); > > > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > > > MemoryRegion *mr = ddc->get_memory_region(dimm); > > > @@ -2645,6 +2646,13 @@ static void spapr_memory_pre_plug(HotplugHandler > > > *hotplug_dev, DeviceState *dev, > > > goto out; > > > } > > > > > > +if (dev->hotplugged) { > > > +if (!ms->os_name) { > > > +error_setg(_err, "Memory hotplug not supported without > > > OS"); > > > +goto out; > > > +} > > > +} > > > + > > > out: > > > error_propagate(errp, local_err); > > > } > > > @@ -2874,6 +2882,7 @@ static void spapr_core_pre_plug(HotplugHandler > > > *hotplug_dev, DeviceState *dev, > > > Error **errp) > > > { > > > MachineState *machine = MACHINE(OBJECT(hotplug_dev)); > > > +sPAPRMachineState *ms = SPAPR_MACHINE(machine); > > > MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev); > > > Error *local_err = NULL; > > > CPUCore *cc = CPU_CORE(dev); > > > @@ -2884,9 +2893,16 @@ static void spapr_core_pre_plug(HotplugHandler > > > *hotplug_dev, DeviceState *dev, > > > int node_id; > > > int index; > > > > > > -if (dev->hotplugged && !mc->has_hotpluggable_cpus) { > > > -error_setg(_err, "CPU hotplug not supported for this > > > machine"); > > > -goto out; > > > +if (dev->hotplugged) { > > > +if (!mc->has_hotpluggable_cpus) { > > > +error_setg(_err, > > > + "CPU hotplug not supported for this machine"); > > > +goto out; > > > +} > > > +if (!ms->os_name) { > > > +error_setg(_err, "CPU hotplug not supported without > > > OS"); > > > +goto out; > > > +} > > > } > > > > > > if (strcmp(base_core_type, type)) { > > > -- 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 3/4] spapr: disable hotplugging without OS
On Wed, May 24, 2017 at 12:14:02PM +0200, Igor Mammedov wrote: > On Wed, 24 May 2017 11:28:57 +0200 > Greg Kurzwrote: > > > On Wed, 24 May 2017 15:07:54 +1000 > > David Gibson wrote: > > > > > On Tue, May 23, 2017 at 01:18:11PM +0200, Laurent Vivier wrote: > > > > If the OS is not started, QEMU sends an event to the OS > > > > that is lost and cannot be recovered. An unplug is not > > > > able to restore QEMU in a coherent state. > > > > So, while the OS is not started, disable CPU and memory hotplug. > > > > We use option vector 6 to know if the OS is started > > > > > > > > Signed-off-by: Laurent Vivier > > > > > > Urgh.. I'm not terribly confident that this is really correct. As > > > discussed on the previous patch, you're essentially using OV6 as a > > > flag that CAS is complete. > > > > > > But while it undoubtedly makes the race window much smaller, I don't > > > see that there's any guarantee the guest OS will really be able to > > > handle hotplug events immediately after CAS. > > > > > > In particular if the CAS process completes partially but then needs to > > > trigger a reboot, I think that would end up setting the ov6 variable, > > > but the OS would definitely not be in a state to accept events. > wouldn't guest on reboot pick up updated fdt and online hotplugged > before crash cpu along with initial cpus? Ah.. yes, I guess so. Are we already resetting DRC and pending event state at reset? > > > We never have any guarantee that the OS will process an event that > > we've sent actually (think of a kernel crash just after a successful > > CAS negotiation for example, or any failure with the various guest > > components involved in the process of hotplug). > > > > > Mike, I really think we need some input from someone familiar with how > > > these hotplug events are supposed to work. What do we need to do to > > > handle lost or stale events, such as those delivered when an OS is not > > > booted. > > > > > > > AFAIK, in the PowerVM world, the HMC exposes a user configurable timeout. > > > > https://www.ibm.com/support/knowledgecenter/POWER8/p8hat/p8hat_dlparprocpoweraddp6.htm > > > > I'm not sure we can do anything better than being able to "cancel" a > > previous > > hotplug attempt if it takes too long, but I'm not necessarily the expert > > you're > > looking for :) > From x86/ACPI world: > - if hotplug happens early at boot before guest OS is running >hotplug notification (SCI interrupt) stays pending and once guest >is up it will/should handle it and online CPU Yeah.. I'm not sure how this will play out on pseries, I suspect the problem is here. > - if guest crashed and is rebooted it will pickup updated apci tables (fdt > equivalent) >with all present cpus (including hotplugged one before crash) and online >hotplugged cpu along with coldplugged ones I think that should work ok for us, as long as we're properly resetting in-flight hotplug state at a reset. > - if guest looses SCI somehow, it's considered guest issue and such cpu >stays unpluggable until guest picks it somehow (reboot, manually running > cpus scan >method from ACPI or another cpu hotplug event) and explicitly ejects it. > > Taking in account that CPUs don't support surprise removal and requires > guest cooperation it's fine to leave CPU plugged in until guest ejects it. > That's what I'd expect to happen on baremetal, > you hotplug CPU, hardware notifies OS about it and that's all, > cpu won't suddenly pop out if OS isn't able to online it. > > More over that hotplugged cpu might be executing some code or one of > already present cpus might be executing initialization routines to online > it (think of host overcommit and arbitrary delays) so it is not really safe > to remove hotplugged but not onlined cpu without OS consent > (i.e. explicit eject by OS/firmware). I think the lost event handling should > be > fixed on guest side and not in QEMU. I agree in principle, but it's not yet clear what needs to be done. I'm guessing the problem is amounting to lost events, but I'm not certain. The question is does the mechanism we're using to present the events have a means to safely not lose them. Are they being presented and lost during SLOF; is there some way we can prevent that. -- 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 3/4] spapr: disable hotplugging without OS
On Wed, May 24, 2017 at 12:40:37PM -0500, Michael Roth wrote: > Quoting Laurent Vivier (2017-05-24 11:02:30) > > On 24/05/2017 17:54, Greg Kurz wrote: > > > On Wed, 24 May 2017 12:14:02 +0200 > > > Igor Mammedovwrote: > > > > > >> On Wed, 24 May 2017 11:28:57 +0200 > > >> Greg Kurz wrote: > > >> > > >>> On Wed, 24 May 2017 15:07:54 +1000 > > >>> David Gibson wrote: > > >>> > > On Tue, May 23, 2017 at 01:18:11PM +0200, Laurent Vivier wrote: > > > If the OS is not started, QEMU sends an event to the OS > > > that is lost and cannot be recovered. An unplug is not > > > able to restore QEMU in a coherent state. > > > So, while the OS is not started, disable CPU and memory hotplug. > > > We use option vector 6 to know if the OS is started > > > > > > Signed-off-by: Laurent Vivier > > > > Urgh.. I'm not terribly confident that this is really correct. As > > discussed on the previous patch, you're essentially using OV6 as a > > flag that CAS is complete. > > > > But while it undoubtedly makes the race window much smaller, I don't > > see that there's any guarantee the guest OS will really be able to > > handle hotplug events immediately after CAS. > > > > In particular if the CAS process completes partially but then needs to > > trigger a reboot, I think that would end up setting the ov6 variable, > > but the OS would definitely not be in a state to accept events. > > >> wouldn't guest on reboot pick up updated fdt and online hotplugged > > >> before crash cpu along with initial cpus? > > >> > > > > > > Yes and that's what actually happens with cpus. > > > > > > But catching up with the background for this series, I have the > > > impression that the issue isn't the fact we loose an event if the OS > > > isn't started (which is not true), but more something wrong happening > > > when hotplugging+unplugging memory as described in this commit: > > > > > > commit fe6824d12642b005c69123ecf8631f9b13553f8b > > > Author: Laurent Vivier > > > Date: Tue Mar 28 14:09:34 2017 +0200 > > > > > > spapr: fix memory hot-unplugging > > > > > > > Yes, this commit try to fix that, but it's not possible. Some objects > > remain in memory: you can see with "info cpus" or "info memory-devices" > > that they are not really removed, and this prevents to hotplug them > > again, and moreover in the case of the memory hot-unplug we can rerun > > the device_del and crash qemu (as before the fix). > > > > Moreover all stuff normally cleared in detach() are not, and we can't do > > it later in set_allocation_state() because some are in use by the > > kernel, and this is the last call from the kernel. > > Focusing on the hotplug/add case, it's a bit odd that the guest would be > using the memory even though the hotplug event is clearly still sitting > in the queue. > > I think part of the issue is us not having a clear enough distinction in > the code between what constitutes the need for "boot-time" handling vs. > "hotplug" handling. > > We have this hook in spapr_add_lmbs: > > if (!dev->hotplugged) { > /* guests expect coldplugged LMBs to be pre-allocated */ > drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE); > drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED); > } > > Whereas the default allocation/isolation state for LMBs in spapr_drc.c is > UNUSABLE/ISOLATED, which is what covers the dev->hotplugged == true case. > > I need to spend some time testing to confirm, but trying to walk through the > various scenarios looking at the code: > > case 1) > > If the hotplug occurs before reset (not sure how likely this is), the event > will get dropped by reset handler, and the DRC stuff will be left in > UNUSABLE/ISOLATED. I think it's more appropriate to treat this as "boot-time" > and set it to USABLE/UNISOLATED like the !dev->hotplugged case. Right. It looks like we might need to go through all DRCs and sanitize their state at reset time. Essentially whatever their state before the reset, they should appear as cold-plugged after the reset, I think. > case 2) > > If the hotplug it occurs after reset, but before CAS, > spapr_populate_drconf_memory will be called to populate the DT with all active > LMBs. AFAICT, for hotplugged LMBs it marks everything where > memory_region_preset(get_system_memory(), addr) == true as > SPAPR_LMB_FLAGS_ASSIGNED. Since the region is mapped regardless of whether the > guest has acknowledged the hotplug, I think this would end up presenting the > LMB as having been present at boot-time. However, they will still be in the > UNUSABLE/ISOLATED state because dev->hotplugged == true. > > I would think that the delayed hotplug event would move them to the > appropriate > state later, allowing the unplug to
Re: [Qemu-devel] [PATCH 4/4] migration: use dirty_rate_high_cnt more aggressively
On Wed, May 24, 2017 at 05:10:03PM +0100, Felipe Franciosi wrote: > The commit message from 070afca25 suggests that dirty_rate_high_cnt > should be used more aggressively to start throttling after two > iterations instead of four. The code, however, only changes the auto > convergence behaviour to throttle after three iterations. This makes the > behaviour more aggressive by kicking off throttling after two iterations > as originally intended. For this one, I don't think fixing the code to match the commit message is that important. Instead, for me this patch looks more like something "changed iteration loops from 3 to 2". So the point is, what would be the best practical number for it. And when we change an existing value, we should have some reason, since it'll change behavior of existing user (though I'm not sure whether this one will affect much). I believe with higher dirty_rate_high_cnt, we have more smooth throttling, but it'll be slower in responding; While if lower or even remove it, we'll get very fast throttling response speed but I guess it may be more possible to report a false positive? IMHO here 3 is okay since after all we are solving the problem of unconverged migration, so as long as we can converge, I think it'll be fine. Thanks, > > Signed-off-by: Felipe Franciosi> --- > migration/ram.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 1a3d9e6..26e03a5 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -708,7 +708,7 @@ static void migration_bitmap_sync(RAMState *rs) > > if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE > > (bytes_xfer_now - rs->bytes_xfer_prev) / 2) && > -(rs->dirty_rate_high_cnt++ >= 2)) { > +(++rs->dirty_rate_high_cnt >= 2)) { > trace_migration_throttle(); > rs->dirty_rate_high_cnt = 0; > mig_throttle_guest_down(); > -- > 1.9.5 > -- Peter Xu
Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
On Wed, 24 May 2017 08:01:47 -0400 (EDT) Marcel Apfelbaumwrote: > - Original Message - > > From: "Markus Armbruster" > > To: "Philippe Mathieu-Daudé" > > Cc: qemu-devel@nongnu.org, "Mao Zhongyi" , > > j...@resnulli.us, jasow...@redhat.com, "Michael > > S. Tsirkin" , "Marcel Apfelbaum" > > Sent: Wednesday, May 24, 2017 8:35:04 AM > > Subject: Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error > > handling > > > > Philippe Mathieu-Daudé writes: > > > > > Hi Markus, > > > > > > On 05/23/2017 06:27 AM, Markus Armbruster wrote: > > > [...] > > >> There's one more cleanup opportunity: > > >> > > > [...] > > >>> if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, > > >>> size)) { > > >>> return NULL; > > >>> } > > >> > > >> None of the pci_dma_read() calls outside rocker check the return value. > > >> Just as well, because it always returns 0. Please clean this up in a > > >> separate followup patch. > > > > > > It may be the correct way to do it but this sounds like we are missing > > > something somewhere... pci_dma_read() calls pci_dma_rw() which always > > > returns 0. Why not let it returns void? It is inlined and never used > > > by address. Else we should document why returning 0 is correct, and > > > what is the reason to not use a void prototype. > > > > > > pci_dma_rw() calls dma_memory_rw() which does return a boolean value, > > > false on success (MEMTX_OK) and true on error > > > (MEMTX_ERROR/DECODE_ERROR) > > > > PCI question. Michael, Marcel? > > > > Hi Markus, > > Looking at the git history, pci_dma_rw used to call cpu_physical_memory_rw > which, at that time (commit ec17457), returned void. Since the interface > dictated > to return int, 0 is returned as "always OK". > > The callers to pci_dma_read did not bother to check it for obvious reasons > (even if they should). > > In the meantime the memory API has changed to allow returning errors, but > since the callers of > pci_dma_rw don't check the return value, why bother to update the PCI DMA? > > History aside (and my speculations above), it seems the right move is to > update > the return value and check it by callers, but honestly I don't have any idea > if the emulated devices expect pci dma to fail. > Adding Paolo and David for more insights. It seems to me that PCI DMA transfers ought to be able to fail, and devices ought to be able to handle that (to a limited extent). After all, what will happen if you try to DMA to PCI addresses that simply aren't mapped. Or which are in the domain of a vIOMMU which wither hasn't mapped those addreses, or has them mapped read-only (meaning host-to-device only in this context). -- David Gibson Principal Software Engineer, Virtualization, Red Hat pgpQEQtzVxx2q.pgp Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] xics: add unrealize handler
On Wed, May 24, 2017 at 07:40:43PM +0200, Greg Kurz wrote: > Now that ICPState objects get finalized on CPU unplug, we should unregister > reset handlers as well to avoid a QEMU crash at machine reset time. > > Signed-off-by: Greg KurzApplied to ppc-for-2.10. > --- > hw/intc/xics.c |5 + > hw/intc/xics_kvm.c |6 ++ > 2 files changed, 11 insertions(+) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 292fffecd376..ea3516794af7 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -357,6 +357,10 @@ static void icp_realize(DeviceState *dev, Error **errp) > qemu_register_reset(icp_reset, dev); > } > > +static void icp_unrealize(DeviceState *dev, Error **errp) > +{ > +qemu_unregister_reset(icp_reset, dev); > +} > > static void icp_class_init(ObjectClass *klass, void *data) > { > @@ -364,6 +368,7 @@ static void icp_class_init(ObjectClass *klass, void *data) > > dc->vmsd = _icp_server; > dc->realize = icp_realize; > +dc->unrealize = icp_unrealize; > } > > static const TypeInfo icp_info = { > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index dd7f29846235..14b8f6f6e478 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -164,12 +164,18 @@ static void icp_kvm_realize(DeviceState *dev, Error > **errp) > qemu_register_reset(icp_kvm_reset, dev); > } > > +static void icp_kvm_unrealize(DeviceState *dev, Error **errp) > +{ > +qemu_unregister_reset(icp_kvm_reset, dev); > +} > + > static void icp_kvm_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > ICPStateClass *icpc = ICP_CLASS(klass); > > dc->realize = icp_kvm_realize; > +dc->unrealize = icp_kvm_unrealize; > icpc->pre_save = icp_get_kvm_state; > icpc->post_load = icp_set_kvm_state; > icpc->cpu_setup = icp_kvm_cpu_setup; > -- 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] migration: remove register_savevm()
On Wed, May 24, 2017 at 02:10:48PM +0200, Laurent Vivier wrote: > We can replace the four remaining calls of register_savevm() by > calls to register_savevm_live(). So we can remove the function and > as we don't allocate anymore the ops pointer with g_new0() > we don't have to free it then. > > Signed-off-by: Laurent VivierReviewed-by: David Gibson > --- > hw/net/vmxnet3.c| 8 ++-- > hw/s390x/s390-skeys.c | 9 +++-- > hw/s390x/s390-virtio-ccw.c | 8 ++-- > include/migration/vmstate.h | 8 > migration/savevm.c | 16 > slirp/slirp.c | 8 ++-- > 6 files changed, 25 insertions(+), 32 deletions(-) > > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index 8b1fab2..4df3110 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -2262,6 +2262,11 @@ static const MemoryRegionOps b1_ops = { > }, > }; > > +static SaveVMHandlers savevm_vmxnet3_msix = { > +.save_state = vmxnet3_msix_save, > +.load_state = vmxnet3_msix_load, > +}; > + > static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) > { > uint64_t dsn_payload; > @@ -2331,8 +2336,7 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, > Error **errp) >vmxnet3_device_serial_num(s)); > } > > -register_savevm(dev, "vmxnet3-msix", -1, 1, > -vmxnet3_msix_save, vmxnet3_msix_load, s); > +register_savevm_live(dev, "vmxnet3-msix", -1, 1, _vmxnet3_msix, > s); > } > > static void vmxnet3_instance_init(Object *obj) > diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c > index e2d4e1a..a3dc088 100644 > --- a/hw/s390x/s390-skeys.c > +++ b/hw/s390x/s390-skeys.c > @@ -363,6 +363,11 @@ static inline bool > s390_skeys_get_migration_enabled(Object *obj, Error **errp) > return ss->migration_enabled; > } > > +static SaveVMHandlers savevm_s390_storage_keys = { > +.save_state = s390_storage_keys_save, > +.load_state = s390_storage_keys_load, > +}; > + > static inline void s390_skeys_set_migration_enabled(Object *obj, bool value, > Error **errp) > { > @@ -376,8 +381,8 @@ static inline void > s390_skeys_set_migration_enabled(Object *obj, bool value, > ss->migration_enabled = value; > > if (ss->migration_enabled) { > -register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save, > -s390_storage_keys_load, ss); > +register_savevm_live(NULL, TYPE_S390_SKEYS, 0, 1, > + _s390_storage_keys, ss); > } else { > unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss); > } > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index fdd4384..697a2d6 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -104,6 +104,11 @@ void s390_memory_init(ram_addr_t mem_size) > s390_skeys_init(); > } > > +static SaveVMHandlers savevm_gtod = { > +.save_state = gtod_save, > +.load_state = gtod_load, > +}; > + > static void ccw_init(MachineState *machine) > { > int ret; > @@ -146,8 +151,7 @@ static void ccw_init(MachineState *machine) > s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw"); > > /* Register savevm handler for guest TOD clock */ > -register_savevm(NULL, "todclock", 0, 1, > -gtod_save, gtod_load, kvm_state); > +register_savevm_live(NULL, "todclock", 0, 1, _gtod, kvm_state); > } > > static void s390_cpu_plug(HotplugHandler *hotplug_dev, > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index f97411d..21aa6ca 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -59,14 +59,6 @@ typedef struct SaveVMHandlers { > LoadStateHandler *load_state; > } SaveVMHandlers; > > -int register_savevm(DeviceState *dev, > -const char *idstr, > -int instance_id, > -int version_id, > -SaveStateHandler *save_state, > -LoadStateHandler *load_state, > -void *opaque); > - > int register_savevm_live(DeviceState *dev, > const char *idstr, > int instance_id, > diff --git a/migration/savevm.c b/migration/savevm.c > index d971e5e..32badfc 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -617,21 +617,6 @@ int register_savevm_live(DeviceState *dev, > return 0; > } > > -int register_savevm(DeviceState *dev, > -const char *idstr, > -int instance_id, > -int version_id, > -SaveStateHandler *save_state, > -LoadStateHandler *load_state, > -void *opaque) > -{ > -SaveVMHandlers *ops = g_new0(SaveVMHandlers, 1); > -
Re: [Qemu-devel] [PATCH v2 1/5] qemu-io: Don't die on second open
On Wed, 05/24 15:28, Eric Blake wrote: > Most callback commands in qemu-io return 0 to keep the interpreter > loop running, or 1 to quit immediately. However, open_f() just > passed through the return value of openfile(), which has different > semantics of returning 0 if a file was opened, or 1 on any failure. > > As a result of mixing the return semantics, we are forcing the > qemu-io interpreter to exit early on any failures, which is rather > annoying when some of the failures are obviously trying to give > the user a hint of how to proceed (if we didn't then kill qemu-io > out from under the user's feet): > > $ qemu-io > qemu-io> open foo > qemu-io> open foo > file open already, try 'help close' > $ echo $? > 0 > > Meanwhile, we WANT openfile() to report failures, as it is the > way that 'qemu-io -c "$something" no_such_file' knows to exit > early rather than attempting $something. So the solution is to > fix open_f() to always return 0 (when we are in interactive mode, > even failure to open should not end the session), and save the > return value of openfile() for command line use in main(). > > This has been awkward since at least as far back as commit > e3aff4f, in 2009. > > Signed-off-by: Eric Blake> > --- > v2: fix open_f(), not openfile() > --- > qemu-io.c | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/qemu-io.c b/qemu-io.c > index 34fa8a1..b3febc2 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -230,13 +230,14 @@ static int open_f(BlockBackend *blk, int argc, char > **argv) > qemu_opts_reset(_opts); > > if (optind == argc - 1) { > -return openfile(argv[optind], flags, writethrough, force_share, > opts); > +openfile(argv[optind], flags, writethrough, force_share, opts); > } else if (optind == argc) { > -return openfile(NULL, flags, writethrough, force_share, opts); > +openfile(NULL, flags, writethrough, force_share, opts); > } else { > QDECREF(opts); > -return qemuio_command_usage(_cmd); > +qemuio_command_usage(_cmd); > } > +return 0; > } > > static int quit_f(BlockBackend *blk, int argc, char **argv) > -- > 2.9.4 > > This is better, thanks! Reviewed-by: Fam Zheng
Re: [Qemu-devel] [PATCH 1/4] migration: keep bytes_xfer_prev init'd to zero
On Wed, May 24, 2017 at 05:10:00PM +0100, Felipe Franciosi wrote: > The first time migration_bitmap_sync() is called, bytes_xfer_prev is set > to ram_state.bytes_transferred which is, at this point, zero. The next > time migration_bitmap_sync() is called, an iteration has happened and > bytes_xfer_prev is set to 'x' bytes. Most likely, more than one second > has passed, so the auto converge logic will be triggered and > bytes_xfer_now will also be set to 'x' bytes. > > This condition is currently masked by dirty_rate_high_cnt, which will > wait for a few iterations before throttling. It would otherwise always > assume zero bytes have been copied and therefore throttle the guest > (possibly) prematurely. > > Given bytes_xfer_prev is only used by the auto convergence logic, it > makes sense to only set its value after a check has been made against > bytes_xfer_now. > > Signed-off-by: Felipe FranciosiYes, with patch 3, I think it should be accurate. Reviewed-by: Peter Xu > --- > migration/ram.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index c07a9c0..36bf720 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -673,10 +673,6 @@ static void migration_bitmap_sync(RAMState *rs) > > rs->bitmap_sync_count++; > > -if (!rs->bytes_xfer_prev) { > -rs->bytes_xfer_prev = ram_bytes_transferred(); > -} > - > if (!rs->time_last_bitmap_sync) { > rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > } > -- > 1.9.5 > -- Peter Xu
Re: [Qemu-devel] [PATCH 3/4] migration: set bytes_xfer_* outside of autoconverge logic
On Wed, May 24, 2017 at 05:10:02PM +0100, Felipe Franciosi wrote: > The bytes_xfer_now/prev counters are only used by the auto convergence > logic. However, they are used alongside the dirty_pages_rate counter, > which is calculated (and required) outside of this logic. The problem > with this approach is that if the auto convergence capability is changed > while a migration is ongoing, the relationship of the counters will be > broken. > > This moves the management of bytes_xfer_now/prev counters outside of the > auto convergence logic to address this issue. > > Signed-off-by: Felipe FranciosiReviewed-by: Peter Xu > --- > migration/ram.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 495ecbe..1a3d9e6 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -697,6 +697,7 @@ static void migration_bitmap_sync(RAMState *rs) > /* calculate period counters */ > rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000 > / (end_time - rs->time_last_bitmap_sync); > +bytes_xfer_now = ram_bytes_transferred(); > > if (migrate_auto_converge()) { > /* The following detection logic can be refined later. For now: > @@ -704,7 +705,6 @@ static void migration_bitmap_sync(RAMState *rs) > amount of bytes that just got transferred since the last time > we > were in this routine. If that happens twice, start or increase > throttling */ > -bytes_xfer_now = ram_bytes_transferred(); > > if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE > > (bytes_xfer_now - rs->bytes_xfer_prev) / 2) && > @@ -713,7 +713,6 @@ static void migration_bitmap_sync(RAMState *rs) > rs->dirty_rate_high_cnt = 0; > mig_throttle_guest_down(); > } > -rs->bytes_xfer_prev = bytes_xfer_now; > } > > if (migrate_use_xbzrle()) { > @@ -730,6 +729,7 @@ static void migration_bitmap_sync(RAMState *rs) > /* reset period counters */ > rs->time_last_bitmap_sync = end_time; > rs->num_dirty_pages_period = 0; > +rs->bytes_xfer_prev = bytes_xfer_now; > } > if (migrate_use_events()) { > qapi_event_send_migration_pass(rs->bitmap_sync_count, NULL); > -- > 1.9.5 > -- Peter Xu
Re: [Qemu-devel] [PATCH 2/4] migration: set dirty_pages_rate before autoconverge logic
On Wed, May 24, 2017 at 05:10:01PM +0100, Felipe Franciosi wrote: > Currently, a "period" in the RAM migration logic is at least a second > long and accounts for what happened since the last period (or the > beginning of the migration). The dirty_pages_rate counter is calculated > at the end this logic. > > If the auto convergence capability is enabled from the start of the > migration, it won't be able to use this counter the first time around. > This calculates dirty_pages_rate as soon as a period is deemed over, > which allows for it to be used immediately. > > Signed-off-by: Felipe FranciosiYou fixed the indents as well, but imho it's okay. Reviewed-by: Peter Xu > --- > migration/ram.c | 17 ++--- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 36bf720..495ecbe 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -694,6 +694,10 @@ static void migration_bitmap_sync(RAMState *rs) > > /* more than 1 second = 1000 millisecons */ > if (end_time > rs->time_last_bitmap_sync + 1000) { > +/* calculate period counters */ > +rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000 > +/ (end_time - rs->time_last_bitmap_sync); > + > if (migrate_auto_converge()) { > /* The following detection logic can be refined later. For now: > Check to see if the dirtied bytes is 50% more than the approx. > @@ -702,15 +706,14 @@ static void migration_bitmap_sync(RAMState *rs) > throttling */ > bytes_xfer_now = ram_bytes_transferred(); > > -if (rs->dirty_pages_rate && > - (rs->num_dirty_pages_period * TARGET_PAGE_SIZE > > +if ((rs->num_dirty_pages_period * TARGET_PAGE_SIZE > > (bytes_xfer_now - rs->bytes_xfer_prev) / 2) && > - (rs->dirty_rate_high_cnt++ >= 2)) { > +(rs->dirty_rate_high_cnt++ >= 2)) { > trace_migration_throttle(); > rs->dirty_rate_high_cnt = 0; > mig_throttle_guest_down(); > - } > - rs->bytes_xfer_prev = bytes_xfer_now; > +} > +rs->bytes_xfer_prev = bytes_xfer_now; > } > > if (migrate_use_xbzrle()) { > @@ -723,8 +726,8 @@ static void migration_bitmap_sync(RAMState *rs) > rs->iterations_prev = rs->iterations; > rs->xbzrle_cache_miss_prev = rs->xbzrle_cache_miss; > } > -rs->dirty_pages_rate = rs->num_dirty_pages_period * 1000 > -/ (end_time - rs->time_last_bitmap_sync); > + > +/* reset period counters */ > rs->time_last_bitmap_sync = end_time; > rs->num_dirty_pages_period = 0; > } > -- > 1.9.5 > -- Peter Xu
Re: [Qemu-devel] [PATCH v2 0/4] 9pfs: local: fix metadata of mapped-file security mode
On 05/24/2017 10:54 AM, Greg Kurz wrote: > On Wed, 24 May 2017 00:59:29 +0200 > Leo Gaspardwrote: > >> On 05/23/2017 04:32 PM, Greg Kurz wrote: >>> v2: - posted patch for CVE-2017-7493 separately >>> - other changes available in each patch changelog >>> >>> Leo, >>> >>> If you find time to test this series, I'll gladly add your Tested-by: to >>> it before merging. >> >> Just tested with a base of 2.9.0 with patches [1] [2] (from my >> distribution), [3] (required to apply cleanly) and this patchset. >> >> Things appear to work as expected, and .virtfs_metadata{,_root} appear >> to be neither readable nor writable by any user. >> > > Shall I add your Tested-by: to the patch then ? Hmm, I can't find the definition of Tested-by: on [1], but if it means "tested it by hand without maybe trying all possible edge cases" then I guess you can add it :) >> That said, one thing still bothering me with the fix in [3] is that it >> still "leaks" the host's uid/gid to the guest when a corresponding file >> in .virtfs_metadata is not present (while I'd have expected it to appear >> as root:root in the guest), but that's a separate issue, and I guess >> retro-compatibility prevents any fixing it. >> > > Heh, I had a tentative patch to create root:root credentials and 0700 mode > bits by default... but this could indeed break some setups, so I decided > not to post it. Hmm, maybe adding an option to the security_model=mapped-* that allows to run default to root:root 0700 would allow to keep retrocompat, and in a few versions swap the default value of the parameter? As the 'dscript=no' of -netdev type=tap appears to have disappeared -- and broke my scripts -- in the switch to 1.9.0, I guess such a change would be allowed by the retrocompatibility policy of qemu? Anyway, it's fun to see you had thought of that too! Cheers, Leo [1] http://wiki.qemu.org/Contribute/SubmitAPatch signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] migration: keep bytes_xfer_prev init'd to zero
On Wed, May 24, 2017 at 01:02:25PM +, Felipe Franciosi wrote: > > > On 23 May 2017, at 05:27, Peter Xuwrote: > > > > On Fri, May 19, 2017 at 10:59:02PM +0100, Felipe Franciosi wrote: > >> The first time migration_bitmap_sync() is called, bytes_xfer_prev is set > >> to ram_state.bytes_transferred which is, at this point, zero. The next > >> time migration_bitmap_sync() is called, an iteration has happened and > >> bytes_xfer_prev is set to 'x' bytes. Most likely, more than one second > >> has passed, so the auto converge logic will be triggered and > >> bytes_xfer_now will also be set to 'x' bytes. > >> > >> This condition is currently masked by dirty_rate_high_cnt, which will > >> wait for a few iterations before throttling. It would otherwise always > >> assume zero bytes have been copied and therefore throttle the guest > >> (possibly) prematurely. > >> > >> Given bytes_xfer_prev is only used by the auto convergence logic, it > >> makes sense to only set its value after a check has been made against > >> bytes_xfer_now. > >> > >> Signed-off-by: Felipe Franciosi > >> ~ > >> --- > >> migration/ram.c | 4 > >> 1 file changed, 4 deletions(-) > >> > >> diff --git a/migration/ram.c b/migration/ram.c > >> index f59fdd4..793af39 100644 > >> --- a/migration/ram.c > >> +++ b/migration/ram.c > >> @@ -670,10 +670,6 @@ static void migration_bitmap_sync(RAMState *rs) > >> > >> rs->bitmap_sync_count++; > >> > >> -if (!rs->bytes_xfer_prev) { > >> -rs->bytes_xfer_prev = ram_bytes_transferred(); > >> -} > >> - > >> if (!rs->time_last_bitmap_sync) { > >> rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > >> } > >> -- > >> 1.9.5 > >> > >> > > > > I feel like this patch wants to correctly initialize bytes_xfer_prev, > > however I still see problem. E.g., when user specify auto-convergence > > during migration, and in the first iteration we'll always have a very > > small bytes_xfer_prev (with this patch, it'll be zero) with a very big > > bytes_xfer_now (which is the ram_bytes_transferred() value). > > Interesting point. Worth noting, that's no different than what happens today > anyway (bytes_xfer_prev would be initialised to the first non-zero > bytes_transferred). I therefore don't think it should stop or slow this > patch's acceptance. > > > If so, do > > you think squash below change together with current one would be more > > accurate? > > As a matter of fact I had a different idea (below) to fix what you are > describing. I was still experimenting with this code, so haven't sent a patch > yet. But I'm going to send a series soon for comments. Basically, I think > some other changes are required to make sure these numbers are correct: I agree on most points. > > 1) dirty_rate_high_cnt++ >= 2 should be ++dirty_rate_high_cnt >= 2 > - The original commit msg from 070afca25 (Jason J. Herne) says that > convergence should be triggered after two passes. Current code does it after > three passes (and four passes the first time around; see number 2 below). > - I personally feel this counter should go away altogether. If the migration > is not converging and the VM is going to be throttled, there's no point > stressing the network any further; just start throttling straight away. > > 2) dirty_pages_rate should be updated before the autoconverge logic. > - Right now, we delay throttling by a further iteration, as dirty_pages_rate > is set after the first pass through the autoconverge logic (it is zero the > first time around). > - The "if (rs->dirty_pages_rate &&..." part of the conditional can then be > removed, as it won't ever be zero. For this one: why dirty_pages_rate cannot be zero? But I agree with you that it can be removed since even if it's zero, then the next check would fail as well (rs->num_dirty_pages_period * TARGET_PAGE_SIZE > (bytes_xfer_now - rs->bytes_xfer_prev) / 2). > > 3) bytes_xfer counters should be updated alongside dirty_pages counters (for > the same period). > - This fixes the issue you described, as bytes_xfer_* will correspond to the > period. > > I'll send the series shortly. Thoughts in the meantime? I'll reply to that patchset then. Thanks. -- Peter Xu
Re: [Qemu-devel] [PATCH] target/i386: use multiple CPU AddressSpaces
On 05/19/2017 02:16 AM, Paolo Bonzini wrote: This speeds up SMM switches. Later on it may remove the need to take the BQL, and it may also allow to reuse code between TCG and KVM. Signed-off-by: Paolo Bonzini--- target/i386/cpu.c| 15 +- target/i386/cpu.h| 11 +- target/i386/helper.c | 54 target/i386/machine.c| 4 target/i386/smm_helper.c | 18 5 files changed, 47 insertions(+), 55 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page
On Wed, May 24, 2017 at 03:16:23PM +0300, Alexey Perevalov wrote: > On 05/24/2017 03:01 PM, Peter Xu wrote: > >On Wed, May 24, 2017 at 10:56:37AM +0300, Alexey wrote: > >>On Wed, May 24, 2017 at 02:57:36PM +0800, Peter Xu wrote: > >>>On Tue, May 23, 2017 at 02:31:08PM +0300, Alexey Perevalov wrote: > This patch adds ability to track down already copied > pages, it's necessary for calculation vCPU block time in > postcopy migration feature and maybe for restore after > postcopy migration failure. > > Functions which work with RAMBlock are placed into ram.c, > due to TARGET_WORDS_BIGENDIAN is poisoned int postcopy-ram.c - > hardware independed code. > > Signed-off-by: Alexey Perevalov> --- > include/migration/migration.h | 16 +++ > migration/postcopy-ram.c | 22 --- > migration/ram.c | 63 > +++ > 3 files changed, 97 insertions(+), 4 deletions(-) > > diff --git a/include/migration/migration.h b/include/migration/migration.h > index 449cb07..4e05c83 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -101,6 +101,20 @@ struct MigrationIncomingState { > LoadStateEntry_Head loadvm_handlers; > /* > + * bitmap indicates whether page copied, > + * based on ramblock offset > + * now it is using only for blocktime calculation in > + * postcopy migration, so livetime of this entry: > + * since user requested blocktime calculation, > + * till the end of postcopy migration > + * as an example it could represend following memory map > + * ___ > + * |4k pages | hugepages | 4k pages > >>>Can we really do this? > >>>The problem is AFAIU migration stream is sending pages only in target > >>>page size, even for huge pages... so even for huge pages we should > >>>still need per TARGET_PAGE_SIZE bitmap? > >>sending maybe, but copying to userfault fd is doing in hugepage size > >Yes you are right. :) > > > >>in case of hugetlbfs memory backend. > >>>(Please refer to ram_state_init() on init of RAMBlock.bmap) > >>I thought to use bitmap per ramblock, but I decided to not do it, > >>because of following reasons: > >>1. There is only 4k ramblocks, for such ramblock it's not > >>optimal > >Could you explain what do you mean by "there is only 4k ramblocks"? > sorry, could be ramblocks with 4k size, > as example, your's qemu hmp info ramblock shows > Block NamePSize Offset Used Total > /objects/mem2 MiB 0x 0x2000 > 0x2000 > vga.vram4 KiB 0x2006 0x0100 > 0x0100 > /rom@etc/acpi/tables4 KiB 0x2113 0x0002 > 0x0020 > pc.bios4 KiB 0x2000 0x0004 > 0x0004 > :00:03.0/e1000.rom4 KiB 0x2107 0x0004 > 0x0004 > :00:04.0/e1000.rom4 KiB 0x210b 0x0004 > 0x0004 > :00:05.0/e1000.rom4 KiB 0x210f 0x0004 > 0x0004 > pc.rom4 KiB 0x2004 0x0002 > 0x0002 > :00:02.0/vga.rom4 KiB 0x2106 0x0001 > 0x0001 >/rom@etc/table-loader4 KiB 0x2133 0x1000 > 0x1000 > /rom@etc/acpi/rsdp4 KiB 0x21331000 0x1000 > 0x1000 > > /rom@etc/table-loader and /rom@etc/acpi/rsdp has only one 4K page. I see. Thanks for explaining this. A 4k sized ramblock means the bitmap would be only one unsigned long width (via bitmap_new(1)). I still don't see why it's not good... :-) -- Peter Xu
Re: [Qemu-devel] [PATCH 5/5] target/sh4: fix RTE instruction delay slot
On 05/16/2017 03:47 PM, Aurelien Jarno wrote: The ReTurn from Exception (RTE) instruction loads the system register (SR) with the saved system register (SSR). It has a delay slot, and behaves specially according to the SH4 manual: The SR value accessed by the instruction in the RTE delay slot is the value restored from SSR by the RTE instruction. The SR and MD values defined prior to RTE execution are used to fetch the instruction in the RTE delay slot. The instruction in the delay slot being often a NOP, it doesn't cause any issue most of the time except in some rare cases where the NOP is being splitted in a different TB (for example when the TCG op buffer is full). In that case the NOP is fetched with the user permissions and causes an instruction TLB protection violation exception. This patches fixes that by introducing a new delay slot flag for the RTE instruction. Given it's a privileged instruction, the RTE delay slot instruction is always fetched in privileged mode. It is therefore enough to to check for this flag in cpu_mmu_index. Signed-off-by: Aurelien Jarno--- target/sh4/cpu.h | 13 ++--- target/sh4/translate.c | 8 ++-- 2 files changed, 16 insertions(+), 5 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 2/5] target/sh4: fix reset when using a kernel and an initrd
On 05/16/2017 03:47 PM, Aurelien Jarno wrote: When a masked exception happens, the SH4 CPU generates a non-masked reset exception, which then jumps to the reset vector at address 0xA000. While this is emulated correctly in QEMU, this does not work when using a kernel and initrd as this address then contain an illegal instruction (and there is no guarantee the kernel and initrd haven't been overwritten). Therefore call qemu_system_reset_request to reload the kernel and initrd and load the program counter to the kernel entry point. Signed-off-by: Aurelien Jarno--- target/sh4/helper.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 4/5] target/sh4: ignore interrupts in a delay slot
On 05/16/2017 03:47 PM, Aurelien Jarno wrote: Delay slots are indivisible, therefore avoid scheduling an interrupt in the delay slot. However exceptions are possible. Signed-off-by: Aurelien Jarno--- target/sh4/helper.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 3/5] target/sh4: introduce DELAY_SLOT_MASK
On 05/16/2017 03:47 PM, Aurelien Jarno wrote: This will make easier the introduction of a new flag in the next patches. Signed-off-by: Aurelien Jarno--- target/sh4/cpu.h | 3 ++- target/sh4/helper.c| 4 ++-- target/sh4/translate.c | 17 - 3 files changed, 12 insertions(+), 12 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 1/5] target/sh4: log unauthorized accesses using qemu_log_mask
On 05/16/2017 03:47 PM, Aurelien Jarno wrote: qemu_log_mask() is preferred over fprintf() for logging errors. Signed-off-by: Aurelien Jarno--- target/sh4/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine
On Tue, May 23, 2017 at 02:12:43PM +0300, Diana Craciun wrote: > The NXP DPAA2 is a hardware architecture designed for high-speeed network > packet processing. The DPAA2 hardware components are managed by a hardware > component called the Management Complex (or MC) which provides an > object-base abstraction for software drivers to use the DPAA2 hardware. > For more details you can see: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-mc/README.txt?h=v4.10 > > The interrupts generated by the DPAA2 hardware components are MSIs. We will > add > support for direct assigning these DPAA2 components/objects to a virtual > machine. However, this will add the need to expand the MSI usage in QEMU. > > Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the > GIC ITS is using a device ID for interrupt translation. Currently, for > PCI, the requester ID is used as device ID. This will not work when > we add another entity that needs also a device ID which is supposed to > be unique across the system. > > My proposal is to add a static allocation in the virt machine. I considered > that this allocation is specific to each machine/platform. Currently only > virt machine has it, but other implementations may use the same mechanism > as well. > So, I used a static allocation with this formula: > > DeviceID = zero_extend( RequesterID[15:0] ) + 0x1 * Constant > > This formula was taken from SBSA spec (Appendix I: DeviceID generation and > ITS groups). In case of QEMU the constant will be different for each entity. > In this way a unique DeviceID will be generated and the device ID will be > derived from a requesterID (in case of PCI) or other means in case of other > entities. > > The implementation is generic as there might be in the future other non-pci > devices > that are using MSIs or IOMMU. Any architecture can use it, though currently > only the ARM architecture is using the function that retrieves the stream ID. > I > did not change all the replacements of the pci_requester_id (with > pci_stream_id) > in the code (although if the constant is 0, the stream_id is equal with > requester_id). > The other architectures (e.g. intel iommu code) assume that the ID is the > requester ID. > > Tested on NXP LS2080 platform. > > History: I am confused. I get it that non-PCI things want something else in their requester ID, but why require it for PCI devices? How about using Constant == 0 for PCI? This way you do not need to touch PCI at all as DeviceID == RequesterID ... > v1->v2 > -- > - the stream ID was added as a field in the pci device structure in order > not to traverse the PCI hierarchy each time a MSI is sent. > > > Diana Craciun (2): > Increased the size of requester_id field from MemTxAttrs > Add a unique ID in the virt machine to be used as device ID > > hw/arm/virt.c | 26 ++ > hw/i386/amd_iommu.c| 2 +- > hw/i386/intel_iommu.c | 2 +- > hw/intc/arm_gicv3_its_common.c | 2 +- > hw/intc/arm_gicv3_its_kvm.c| 2 +- > hw/pci-host/gpex.c | 6 ++ > hw/pci/msi.c | 2 +- > hw/pci/pci.c | 25 + > include/exec/memattrs.h| 4 ++-- > include/hw/arm/virt.h | 1 + > include/hw/intc/arm_gicv3_its_common.h | 2 +- > include/hw/pci-host/gpex.h | 2 ++ > include/hw/pci/pci.h | 8 > kvm-all.c | 4 ++-- > 14 files changed, 78 insertions(+), 10 deletions(-) > > -- > 2.5.5
[Qemu-devel] [PATCH 3/4] target/s390x: Implement EXECUTE via new TranslationBlock
Previously, helper_ex would construct the insn and then implement the insn via direct calls other helpers. This was sufficient to boot Linux but that is all. It is easy enough to go the whole nine yards by stashing state for EXECUTE within the cpu, and then rely on a new TB to be created that properly and completely interprets the insn. Signed-off-by: Richard Henderson--- target/s390x/cpu.h| 4 +- target/s390x/helper.c | 5 ++ target/s390x/machine.c| 19 target/s390x/mem_helper.c | 118 +- target/s390x/translate.c | 80 ++- 5 files changed, 85 insertions(+), 141 deletions(-) diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 4f38ba0..79235cf 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -103,6 +103,8 @@ typedef struct CPUS390XState { uint64_t cc_dst; uint64_t cc_vr; +uint64_t ex_value; + uint64_t __excp_addr; uint64_t psa; @@ -391,7 +393,7 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc, target_ulong *cs_base, uint32_t *flags) { *pc = env->psw.addr; -*cs_base = 0; +*cs_base = env->ex_value; *flags = ((env->psw.mask >> 32) & ~FLAG_MASK_CC) | ((env->psw.mask & PSW_MASK_32) ? FLAG_MASK_32 : 0); } diff --git a/target/s390x/helper.c b/target/s390x/helper.c index 9978490..f01811f 100644 --- a/target/s390x/helper.c +++ b/target/s390x/helper.c @@ -642,6 +642,11 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request) S390CPU *cpu = S390_CPU(cs); CPUS390XState *env = >env; +if (env->ex_value) { +/* Execution of the target insn is indivisible from + the parent EXECUTE insn. */ +return false; +} if (env->psw.mask & PSW_MASK_EXT) { s390_cpu_do_interrupt(cs); return true; diff --git a/target/s390x/machine.c b/target/s390x/machine.c index 8503fa1..8f908bb 100644 --- a/target/s390x/machine.c +++ b/target/s390x/machine.c @@ -34,6 +34,7 @@ static int cpu_post_load(void *opaque, int version_id) return 0; } + static void cpu_pre_save(void *opaque) { S390CPU *cpu = opaque; @@ -156,6 +157,23 @@ const VMStateDescription vmstate_riccb = { } }; +static bool exval_needed(void *opaque) +{ +S390CPU *cpu = opaque; +return cpu->env.ex_value != 0; +} + +const VMStateDescription vmstate_exval = { +.name = "cpu/exval", +.version_id = 1, +.minimum_version_id = 1, +.needed = exval_needed, +.fields = (VMStateField[]) { +VMSTATE_UINT64(env.ex_value, S390CPU), +VMSTATE_END_OF_LIST() +} +}; + const VMStateDescription vmstate_s390_cpu = { .name = "cpu", .post_load = cpu_post_load, @@ -188,6 +206,7 @@ const VMStateDescription vmstate_s390_cpu = { _fpu, _vregs, _riccb, +_exval, NULL }, }; diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index d57d5b1..3a77edc 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -435,37 +435,6 @@ uint64_t HELPER(mvst)(CPUS390XState *env, uint64_t c, uint64_t d, uint64_t s) return d + len; } -static uint32_t helper_icm(CPUS390XState *env, uint32_t r1, uint64_t address, - uint32_t mask) -{ -int pos = 24; /* top of the lower half of r1 */ -uint64_t rmask = 0xff00ULL; -uint8_t val = 0; -int ccd = 0; -uint32_t cc = 0; - -while (mask) { -if (mask & 8) { -env->regs[r1] &= ~rmask; -val = cpu_ldub_data(env, address); -if ((val & 0x80) && !ccd) { -cc = 1; -} -ccd = 1; -if (val && cc == 0) { -cc = 2; -} -env->regs[r1] |= (uint64_t)val << pos; -address++; -} -mask = (mask << 1) & 0xf; -pos -= 8; -rmask >>= 8; -} - -return cc; -} - /* load access registers r1 to r3 from memory at a2 */ void HELPER(lam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { @@ -1222,19 +1191,17 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) } #endif -/* execute instruction - this instruction executes an insn modified with the contents of r1 - it does not change the executed instruction in memory - it does not change the program counter - in other words: tricky... - currently implemented by interpreting the cases it is most commonly used. +/* Execute instruction. This instruction executes an insn modified with + the contents of r1. It does not change the executed instruction in memory; + it does not change the program counter. + + Perform this by recording the modified instruction in env->ex_value. + This will be noticed by cpu_get_tb_cpu_state and thus tb translation. */ void
[Qemu-devel] [PATCH 4/4] target/s390x: Re-implement a few EXECUTE target insns directly
While the previous patch is required for proper conformance, the vast majority of target insns are MVC and XC for implementing memmove and memset respectively. The next most common are CLC, TR, and SVC. Implementing these (and a few others for which we already have an implementation) directly is faster than going through full translation to a TB. Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 66 --- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 3a77edc..e35571e 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -200,31 +200,30 @@ uint32_t HELPER(oc)(CPUS390XState *env, uint32_t l, uint64_t dest, } /* memmove */ -static void do_helper_mvc(CPUS390XState *env, uint32_t l, uint64_t dest, - uint64_t src, uintptr_t ra) +static uint32_t do_helper_mvc(CPUS390XState *env, uint32_t l, uint64_t dest, + uint64_t src, uintptr_t ra) { uint32_t i; HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n", __func__, l, dest, src); +/* mvc and memmove do not behave the same when areas overlap! */ /* mvc with source pointing to the byte after the destination is the same as memset with the first source byte */ if (dest == src + 1) { fast_memset(env, dest, cpu_ldub_data_ra(env, src, ra), l + 1, ra); -return; -} - -/* mvc and memmove do not behave the same when areas overlap! */ -if (dest < src || src + l < dest) { +} else if (dest < src || src + l < dest) { fast_memmove(env, dest, src, l + 1, ra); -return; +} else { +/* slow version with byte accesses which always work */ +for (i = 0; i <= l; i++) { +uint8_t x = cpu_ldub_data_ra(env, src + i, ra); +cpu_stb_data_ra(env, dest + i, x, ra); +} } -/* slow version with byte accesses which always work */ -for (i = 0; i <= l; i++) { -cpu_stb_data_ra(env, dest + i, cpu_ldub_data_ra(env, src + i, ra), ra); -} +return env->cc_op; } void HELPER(mvc)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src) @@ -692,8 +691,8 @@ void HELPER(unpk)(CPUS390XState *env, uint32_t len, uint64_t dest, } } -static void do_helper_tr(CPUS390XState *env, uint32_t len, uint64_t array, - uint64_t trans, uintptr_t ra) +static uint32_t do_helper_tr(CPUS390XState *env, uint32_t len, uint64_t array, + uint64_t trans, uintptr_t ra) { uint32_t i; @@ -702,12 +701,14 @@ static void do_helper_tr(CPUS390XState *env, uint32_t len, uint64_t array, uint8_t new_byte = cpu_ldub_data_ra(env, trans + byte, ra); cpu_stb_data_ra(env, array + i, new_byte, ra); } + +return env->cc_op; } void HELPER(tr)(CPUS390XState *env, uint32_t len, uint64_t array, uint64_t trans) { -return do_helper_tr(env, len, array, trans, GETPC()); +do_helper_tr(env, len, array, trans, GETPC()); } uint64_t HELPER(tre)(CPUS390XState *env, uint64_t array, @@ -1221,6 +1222,41 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr) g_assert_not_reached(); } +/* The very most common cases can be sped up by avoiding a new TB. */ +if ((opc & 0xf0) == 0xd0) { +typedef uint32_t (*dx_helper)(CPUS390XState *, uint32_t, uint64_t, + uint64_t, uintptr_t); +static const dx_helper dx[16] = { +[0x2] = do_helper_mvc, +[0x4] = do_helper_nc, +[0x5] = do_helper_clc, +[0x6] = do_helper_oc, +[0x7] = do_helper_xc, +[0xc] = do_helper_tr, +[0xd] = do_helper_trt, +}; +dx_helper helper = dx[opc & 0xf]; + +if (helper) { +uint32_t l = extract64(insn, 48, 8); +uint32_t b1 = extract64(insn, 44, 4); +uint32_t d1 = extract64(insn, 32, 12); +uint32_t b2 = extract64(insn, 28, 4); +uint32_t d2 = extract64(insn, 16, 12); +uint64_t a1 = get_address(env, 0, b1, d1); +uint64_t a2 = get_address(env, 0, b2, d2); + +env->cc_op = helper(env, l, a1, a2, 0); +env->psw.addr += ilen; +return; +} +} else if (opc == 0x0a) { +env->int_svc_code = extract64(insn, 48, 8); +env->int_svc_ilen = ilen; +helper_exception(env, EXCP_SVC); +g_assert_not_reached(); +} + /* Record the insn we want to execute as well as the ilen to use during the execution of the target insn. This will also ensure that ex_value is non-zero, which flags that we are in a state -- 2.9.4
[Qemu-devel] [PATCH 2/4] target/s390x: End the TB after EXECUTE
This split will be required for implementing EXECUTE properly. Do this now as a separate step to aid comparison of before and after TB listings. Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 54 --- target/s390x/translate.c | 6 +- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 4b96c27..d57d5b1 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1234,6 +1234,7 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr) S390CPU *cpu = s390_env_get_cpu(env); uint64_t insn = cpu_lduw_code(env, addr); uint8_t opc = insn >> 8; +uint32_t cc; /* Or in the contents of R1[56:63]. */ insn |= r1 & 0xff; @@ -1263,42 +1264,46 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr) b2 = extract64(insn, 28, 4); d1 = extract64(insn, 32, 12); d2 = extract64(insn, 16, 12); + +cc = env->cc_op; switch (opc & 0xf) { case 0x2: do_helper_mvc(env, l, get_address(env, 0, b1, d1), get_address(env, 0, b2, d2), 0); -return; +break; case 0x4: -env->cc_op = do_helper_nc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -return; +cc = do_helper_nc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +break; case 0x5: -env->cc_op = do_helper_clc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -return; +cc = do_helper_clc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +break; case 0x6: -env->cc_op = do_helper_oc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -return; +cc = do_helper_oc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +break; case 0x7: -env->cc_op = do_helper_xc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -return; +cc = do_helper_xc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +break; case 0xc: do_helper_tr(env, l, get_address(env, 0, b1, d1), get_address(env, 0, b2, d2), 0); -return; +break; case 0xd: -env->cc_op = do_helper_trt(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -return; +cc = do_helper_trt(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +break; +default: +goto abort; } } else if (opc == 0x0a) { /* supervisor call */ env->int_svc_code = extract64(insn, 48, 8); env->int_svc_ilen = ilen; helper_exception(env, EXCP_SVC); -return; +g_assert_not_reached(); } else if (opc == 0xbf) { uint32_t r1, r3, b2, d2; @@ -1306,10 +1311,15 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr) r3 = extract64(insn, 48, 4); b2 = extract64(insn, 44, 4); d2 = extract64(insn, 32, 12); -env->cc_op = helper_icm(env, r1, get_address(env, 0, b2, d2), r3); -return; +cc = helper_icm(env, r1, get_address(env, 0, b2, d2), r3); +} else { + abort: +cpu_abort(CPU(cpu), + "EXECUTE on instruction prefix 0x%x not implemented\n", + opc); +g_assert_not_reached(); } -cpu_abort(CPU(cpu), "EXECUTE on instruction prefix 0x%x not implemented\n", - opc); +env->cc_op = cc; +env->psw.addr += ilen; } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 5b8333f..70212c8 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1163,6 +1163,8 @@ typedef enum { the PC (for whatever reason), so there's no need to do it again on exiting the TB. */ EXIT_PC_UPDATED, +/* We have updated the PC and CC values. */ +EXIT_PC_CC_UPDATED, /* We are exiting the TB, but have neither emitted a goto_tb, nor updated the PC for the next instruction to be executed. */ EXIT_PC_STALE, @@ -2216,7 +2218,7 @@ static ExitStatus op_ex(DisasContext *s, DisasOps *o) tcg_temp_free_i64(v1); } -return NO_EXIT; +return
[Qemu-devel] [PATCH 1/4] target/s390x: Save current ilen during translation
Use this saved value instead of recomputing from next_pc difference. Signed-off-by: Richard Henderson--- target/s390x/translate.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 4bd16d9..5b8333f 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -58,6 +58,7 @@ struct DisasContext { const DisasInsn *insn; DisasFields *fields; uint64_t pc, next_pc; +uint32_t ilen; enum cc_op cc_op; bool singlestep_enabled; }; @@ -349,7 +350,7 @@ static void gen_program_exception(DisasContext *s, int code) tcg_gen_st_i32(tmp, cpu_env, offsetof(CPUS390XState, int_pgm_code)); tcg_temp_free_i32(tmp); -tmp = tcg_const_i32(s->next_pc - s->pc); +tmp = tcg_const_i32(s->ilen); tcg_gen_st_i32(tmp, cpu_env, offsetof(CPUS390XState, int_pgm_ilen)); tcg_temp_free_i32(tmp); @@ -2207,7 +2208,7 @@ static ExitStatus op_ex(DisasContext *s, DisasOps *o) v1 = regs[r1]; } -ilen = tcg_const_i32(s->next_pc - s->pc); +ilen = tcg_const_i32(s->ilen); gen_helper_ex(cpu_env, ilen, v1, o->in2); tcg_temp_free_i32(ilen); @@ -4052,7 +4053,7 @@ static ExitStatus op_svc(DisasContext *s, DisasOps *o) tcg_gen_st_i32(t, cpu_env, offsetof(CPUS390XState, int_svc_code)); tcg_temp_free_i32(t); -t = tcg_const_i32(s->next_pc - s->pc); +t = tcg_const_i32(s->ilen); tcg_gen_st_i32(t, cpu_env, offsetof(CPUS390XState, int_svc_ilen)); tcg_temp_free_i32(t); @@ -5191,6 +5192,7 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s, op = (insn >> 8) & 0xff; ilen = get_ilen(op); s->next_pc = s->pc + ilen; +s->ilen = ilen; switch (ilen) { case 2: -- 2.9.4
[Qemu-devel] [PATCH 0/4] target/s390x Implement EXECUTE via TranslationBlock
This is the rewrite of EX that I posted last week, fixed with Aurelien's help, and adjusted to be applied on top of my v2 unwind patch set. It also splits the patch into more pieces to make it easier to debug, and keeps the direct implementation of the most common target insns. Which are in fact so common I don't see any other usage while booting the debian installer. r~ Richard Henderson (4): target/s390x: Save current ilen during translation target/s390x: End the TB after EXECUTE target/s390x: Implement EXECUTE via new TranslationBlock target/s390x: Re-implement a few EXECUTE target insns directly target/s390x/cpu.h| 4 +- target/s390x/helper.c | 5 ++ target/s390x/machine.c| 19 ++ target/s390x/mem_helper.c | 156 -- target/s390x/translate.c | 92 --- 5 files changed, 136 insertions(+), 140 deletions(-) -- 2.9.4
Re: [Qemu-devel] [PATCH] pc: ACPI BIOS: use highest NUMA node for hotplug mem hole SRAT entry
On Wed, May 24, 2017 at 11:16:14AM +0200, Ladi Prosek wrote: > On Wed, May 24, 2017 at 11:07 AM, Laszlo Ersekwrote: > > On 05/24/17 10:09, Ladi Prosek wrote: > >> For reasons unknown, Windows won't online all memory, both at command > >> line and hot-plugged later, unless the hotplug mem hole SRAT entry > >> specifies a node greater or equal to the ones where memory is added. > > > > s/greater or equal to/greater *than* or equal to/ > > > > Thanks, > > Laszlo > > (always finding the important issues! ;) ) > > Haha, thanks! Igor, please let me know if you'd like me to send a > corrected version. Yes and pls include Igor's note to maintainer after --- so I don't forget. > >> > >> Using the highest node on the machine makes recent versions of Windows > >> happy. > >> > >> With this example command line: > >> ... \ > >> -m 1024,slots=4,maxmem=32G \ > >> -numa node,nodeid=0 \ > >> -numa node,nodeid=1 \ > >> -numa node,nodeid=2 \ > >> -numa node,nodeid=3 \ > >> -object memory-backend-ram,size=1G,id=mem-mem1 \ > >> -device pc-dimm,id=dimm-mem1,memdev=mem-mem1,node=1 > >> > >> Windows reports a total of 1G of RAM without this commit and the expected > >> 2G with this commit. > >> > >> Signed-off-by: Ladi Prosek > >> --- > >> hw/i386/acpi-build.c | 7 +-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >> index afcadac..9653583 100644 > >> --- a/hw/i386/acpi-build.c > >> +++ b/hw/i386/acpi-build.c > >> @@ -2404,14 +2404,17 @@ build_srat(GArray *table_data, BIOSLinker *linker, > >> MachineState *machine) > >> } > >> > >> /* > >> - * Entry is required for Windows to enable memory hotplug in OS. > >> + * Entry is required for Windows to enable memory hotplug in OS > >> + * and for Linux to enable SWIOTLB even if booted with less than > >> + * 4G of RAM. Windows works better if the entry sets proximity > >> + * to the highest NUMA node in the machine. > >> * Memory devices may override proximity set by this entry, > >> * providing _PXM method if necessary. > >> */ > >> if (hotplugabble_address_space_size) { > >> numamem = acpi_data_push(table_data, sizeof *numamem); > >> build_srat_memory(numamem, pcms->hotplug_memory.base, > >> - hotplugabble_address_space_size, 0, > >> + hotplugabble_address_space_size, > >> pcms->numa_nodes - 1, > >>MEM_AFFINITY_HOTPLUGGABLE | > >> MEM_AFFINITY_ENABLED); > >> } > >> > >> > >
Re: [Qemu-devel] [PATCH v3] qemu-ga: add guest-get-osrelease command
On 05/24/2017 04:51 PM, Tomáš Golembiovský wrote: > So what about the following, would that be acceptable? > > > ## > # @GuestOSRelease: > # > # @content: > # POSIX systems the @kernel_version, @kernel_release and > # @machine_hardware correspond to the values release, version and > # machine returned by uname(2). On Windows, they correspond to the > # version number, build number and architecture. You'll have to actually document each field, not just a catch-all @content. You can list per-OS on which field is likely to be present or absent. > # > # On Linux-based system where os-release info is available either > # from /etc/os-release or from /usr/lib/os-release, the fields @id, > # @name, @pretty_name, @version, @version_codename, @variant, > # correspond to the fields of the same name defined in > os-release(5). > # On Windows, the data is generated based on the available > # inforamtion. s/inforamtion/information/ > # > # Since: 2.10 > ## > { 'struct': 'GuestOSRelease', > 'data': { > 'kernel_release': 'str', Please name this 'kernel-release'; new interfaces should use '-' rather than '_'. > 'kernel_version': 'str', Etc. > 'machine_hardware': 'str' > 'id': '*str', If a field doesn't make sense for all guests, then it should be marked optional (for example, a Linux guest that does not have /etc/os-release). Marking a field optional is done as '*id':'str' (you got it backwards). > 'name': '*str', > 'pretty_name': '*str', > 'version': '*str', > 'version_codename': '*str', > 'variant': '*str', > } } > > It's probably a reasonable start at the interface, though. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] vhost-user: pass message as a pointer to process_message_reply()
On Wed, May 24, 2017 at 11:05:20AM +0200, Maxime Coquelin wrote: > process_message_reply() was recently updated to get full message > content instead of only its request field. > > There is no need to copy all the struct content into the stack, > so just pass its pointer as const. > > Cc: Zhiyong Yang> Fixes: 60cd11024f41 ("hw/virtio: fix vhost user fails to startup when MQ") > Reviewed-by: Jens Freimann > Reviewed-by: Zhiyong Yang > Signed-off-by: Maxime Coquelin Why "Fixes"? It's not a bugfix, is it? Passing a pointer is slightly cleaner but it's not a big deal IMHO. I'll apply but would like to get clarification on this tag. > --- > V2: > - Make msg pointer const (Marc-Andre) > - Apply R-b's > > hw/virtio/vhost-user.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index b87a176..dde094a 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -162,11 +162,11 @@ fail: > } > > static int process_message_reply(struct vhost_dev *dev, > - VhostUserMsg msg) > + const VhostUserMsg *msg) > { > VhostUserMsg msg_reply; > > -if ((msg.flags & VHOST_USER_NEED_REPLY_MASK) == 0) { > +if ((msg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) { > return 0; > } > > @@ -174,10 +174,10 @@ static int process_message_reply(struct vhost_dev *dev, > return -1; > } > > -if (msg_reply.request != msg.request) { > +if (msg_reply.request != msg->request) { > error_report("Received unexpected msg type." > "Expected %d received %d", > - msg.request, msg_reply.request); > + msg->request, msg_reply.request); > return -1; > } > > @@ -324,7 +324,7 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev, > } > > if (reply_supported) { > -return process_message_reply(dev, msg); > +return process_message_reply(dev, ); > } > > return 0; > @@ -716,7 +716,7 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, > uint16_t mtu) > > /* If reply_ack supported, slave has to ack specified MTU is valid */ > if (reply_supported) { > -return process_message_reply(dev, msg); > +return process_message_reply(dev, ); > } > > return 0; > -- > 2.9.4
Re: [Qemu-devel] [PATCH v3] qemu-ga: add guest-get-osrelease command
On Wed, 24 May 2017 23:51:55 +0200 Tomáš Golembiovskýwrote: > On Wed, 12 Apr 2017 15:05:02 -0500 > Michael Roth wrote: > > > On 04/03/2017 10:17 AM, Marc-André Lureau wrote: > > > Hi > > > > > > On Fri, Mar 31, 2017 at 3:41 PM Eric Blake wrote: > > > > > >> On 03/31/2017 05:19 AM, Vinzenz 'evilissimo' Feenstra wrote: > > >>> From: Vinzenz Feenstra > > >>> > > >>> Add a new 'guest-get-osrelease' command to report OS information in > > >>> the > > >>> os-release format. As documented here: > > >>> https://www.freedesktop.org/software/systemd/man/os-release.html > > >>> > > >>> The win32 implementation generates the information. > > >>> On POSIX systems the /etc/os-release or /usr/lib/os-release files > > >>> content is returned when available and gets extended with the > > >>> fields: > > >>> - QGA_UNAME_RELEASE which is the content of `uname -r` > > >>> - QGA_UNAME_VERSION which is the content of `uname -v` > > >>> - QGA_UNAME_MACHINE which is the content of `uname -m` > > >>> > > >>> Here an example for a Fedora 25 VM: > > >>> > > >>> virsh # qemu-agent-command F25 '{ "execute": "guest-get-osrelease" > > >>> }' > > >>> {"return":{"content":"NAME=Fedora\nVERSION=\"25 (Server Edition)\"\n > > >>> ID=fedora\nVERSION_ID=25\nPRETTY_NAME=\"Fedora 25 (Server > > >>> Edition)\"\n > > >>> ANSI_COLOR=\"0;34\"\nCPE_NAME=\"cpe:/o:fedoraproject:fedora:25\"\n > > >>> HOME_URL=\"https://fedoraproject.org/\"\n > > >>> BUG_REPORT_URL=\"https://bugzilla.redhat.com/\"\n > > >>> REDHAT_BUGZILLA_PRODUCT=\"Fedora\"\n > > >>> REDHAT_BUGZILLA_PRODUCT_VERSION=25\nREDHAT_SUPPORT_PRODUCT=\"Fedora\"\n > > >>> REDHAT_SUPPORT_PRODUCT_VERSION=25\n > > >>> PRIVACY_POLICY_URL=https://fedoraproject.org/wiki/Legal:PrivacyPolicy\n > > >>> VARIANT=\"Server Edition\"\nVARIANT_ID=server\n\n > > >>> QGA_UNAME_RELEASE=\"4.8.6-300.fc25.x86_64\"\n > > >>> QGA_UNAME_VERSION=\"#1 SMP Tue Nov 1 12:36:38 UTC 2016\"\n > > >>> QGA_UNAME_MACHINE=\"x86_64\"\n"}} > > >> > > >> Uggh. This is a step backwards. Now you are requiring the end user > > >> to > > >> parse a raw string, instead of giving them the information already > > >> broken out as a JSON dictionary. > > >> > > > > > > yes otoh, it uses an existing standard to retrieve various guest os > > > release > > > informations, which existing tool may know how to handle. > > > > > > (I feel partially guilty about it since I suggested it, mainly in a > > > discussion over irc and Vinzenz adopted it) > > > > > > The format is fairly straightforward to parse, but perhaps it should > > > be > > > sent as a JSON dict instead? However, that would mean that the list of > > > keys > > > is limited by what QGA protocol defines, and the agent would have to > > > parse > > > the file himself. And we would have to duplicate the documentation > > > etc.. > > > > > > I would rely on the XDG format instead, given its simplicity, > > > extensibility > > > and documentation that fits the job nicely imho. > > > > I like the idea of using an existing standard, but if they really want > > to get at a raw dump of /etc/os-release to use with existing tools then > > I think guest-file-open/read are the more appropriate interfaces. > > > > Knowing that they *can* get at information like that for a particular > > guest, or do things like execute 'uname -m' via guest-exec, is where I > > think an interface like this has it's place. > > > > So I think a more curated/limited set of identifiers is sufficient, and > > still flexible enough to enable to more os-specific use-cases. > > > > But I also don't like the idea of re-defining what terms like > > "version_id", "variant", "varient_id", etc mean, so I think it's still > > a good idea to use the os-release-documented fields as the basis for > > the fields we decide to return in our dictionary, and note that > > explicitly in the schema documentation. > > > > So what about the following, would that be acceptable? > > > ## > # @GuestOSRelease: > # > # @content: > # POSIX systems the @kernel_version, @kernel_release and > # @machine_hardware correspond to the values release, version and > # machine returned by uname(2). On Windows, they correspond to the > # version number, build number and architecture. > # > # On Linux-based system where os-release info is available either > # from /etc/os-release or from /usr/lib/os-release, the fields @id, > # @name, @pretty_name, @version, @version_codename, @variant, > # correspond to the fields of the same name defined in > os-release(5). > # On Windows, the data is generated based on the available > # inforamtion. > # > # Since: 2.10 > ## > { 'struct': 'GuestOSRelease', > 'data': { > 'kernel_release': 'str', > 'kernel_version': 'str', > 'machine_hardware': 'str' > 'id': '*str', >
Re: [Qemu-devel] [PATCH v3] qemu-ga: add guest-get-osrelease command
On Wed, 12 Apr 2017 15:05:02 -0500 Michael Rothwrote: > On 04/03/2017 10:17 AM, Marc-André Lureau wrote: > > Hi > > > > On Fri, Mar 31, 2017 at 3:41 PM Eric Blake wrote: > > > >> On 03/31/2017 05:19 AM, Vinzenz 'evilissimo' Feenstra wrote: > >>> From: Vinzenz Feenstra > >>> > >>> Add a new 'guest-get-osrelease' command to report OS information in > >>> the > >>> os-release format. As documented here: > >>> https://www.freedesktop.org/software/systemd/man/os-release.html > >>> > >>> The win32 implementation generates the information. > >>> On POSIX systems the /etc/os-release or /usr/lib/os-release files > >>> content is returned when available and gets extended with the > >>> fields: > >>> - QGA_UNAME_RELEASE which is the content of `uname -r` > >>> - QGA_UNAME_VERSION which is the content of `uname -v` > >>> - QGA_UNAME_MACHINE which is the content of `uname -m` > >>> > >>> Here an example for a Fedora 25 VM: > >>> > >>> virsh # qemu-agent-command F25 '{ "execute": "guest-get-osrelease" > >>> }' > >>> {"return":{"content":"NAME=Fedora\nVERSION=\"25 (Server Edition)\"\n > >>> ID=fedora\nVERSION_ID=25\nPRETTY_NAME=\"Fedora 25 (Server > >>> Edition)\"\n > >>> ANSI_COLOR=\"0;34\"\nCPE_NAME=\"cpe:/o:fedoraproject:fedora:25\"\n > >>> HOME_URL=\"https://fedoraproject.org/\"\n > >>> BUG_REPORT_URL=\"https://bugzilla.redhat.com/\"\n > >>> REDHAT_BUGZILLA_PRODUCT=\"Fedora\"\n > >>> REDHAT_BUGZILLA_PRODUCT_VERSION=25\nREDHAT_SUPPORT_PRODUCT=\"Fedora\"\n > >>> REDHAT_SUPPORT_PRODUCT_VERSION=25\n > >>> PRIVACY_POLICY_URL=https://fedoraproject.org/wiki/Legal:PrivacyPolicy\n > >>> VARIANT=\"Server Edition\"\nVARIANT_ID=server\n\n > >>> QGA_UNAME_RELEASE=\"4.8.6-300.fc25.x86_64\"\n > >>> QGA_UNAME_VERSION=\"#1 SMP Tue Nov 1 12:36:38 UTC 2016\"\n > >>> QGA_UNAME_MACHINE=\"x86_64\"\n"}} > >> > >> Uggh. This is a step backwards. Now you are requiring the end user > >> to > >> parse a raw string, instead of giving them the information already > >> broken out as a JSON dictionary. > >> > > > > yes otoh, it uses an existing standard to retrieve various guest os > > release > > informations, which existing tool may know how to handle. > > > > (I feel partially guilty about it since I suggested it, mainly in a > > discussion over irc and Vinzenz adopted it) > > > > The format is fairly straightforward to parse, but perhaps it should > > be > > sent as a JSON dict instead? However, that would mean that the list of > > keys > > is limited by what QGA protocol defines, and the agent would have to > > parse > > the file himself. And we would have to duplicate the documentation > > etc.. > > > > I would rely on the XDG format instead, given its simplicity, > > extensibility > > and documentation that fits the job nicely imho. > > I like the idea of using an existing standard, but if they really want > to get at a raw dump of /etc/os-release to use with existing tools then > I think guest-file-open/read are the more appropriate interfaces. > > Knowing that they *can* get at information like that for a particular > guest, or do things like execute 'uname -m' via guest-exec, is where I > think an interface like this has it's place. > > So I think a more curated/limited set of identifiers is sufficient, and > still flexible enough to enable to more os-specific use-cases. > > But I also don't like the idea of re-defining what terms like > "version_id", "variant", "varient_id", etc mean, so I think it's still > a good idea to use the os-release-documented fields as the basis for > the fields we decide to return in our dictionary, and note that > explicitly in the schema documentation. > So what about the following, would that be acceptable? ## # @GuestOSRelease: # # @content: # POSIX systems the @kernel_version, @kernel_release and # @machine_hardware correspond to the values release, version and # machine returned by uname(2). On Windows, they correspond to the # version number, build number and architecture. # # On Linux-based system where os-release info is available either # from /etc/os-release or from /usr/lib/os-release, the fields @id, # @name, @pretty_name, @version, @version_codename, @variant, # correspond to the fields of the same name defined in os-release(5). # On Windows, the data is generated based on the available # inforamtion. # # Since: 2.10 ## { 'struct': 'GuestOSRelease', 'data': { 'kernel_release': 'str', 'kernel_version': 'str', 'machine_hardware': 'str' 'id': '*str', 'name': '*str', 'pretty_name': '*str', 'version': '*str', 'version_codename': '*str', 'variant': '*str', } } Tomas -- Tomáš Golembiovský
Re: [Qemu-devel] [PATCH 02/31] target/s390x: Implement EXECUTE via new TranslationBlock
On 05/24/2017 10:54 AM, Aurelien Jarno wrote: It seems the problem arise if an interrupt happens when the TB containing the EXECUTE instruction is being executed. In that case at the end of the TB, the interruption code is translated with the ex_value set, which means with the wrong PC, wrong permissions and wrong return address. This is the same kind of issue I identified on SH4 recently: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03880.html The same king of solution also works, that is disabling the interrupts when the ex_value is set: diff --git a/target/s390x/helper.c b/target/s390x/helper.c index 6f81b1a16c..a33abdef16 100644 --- a/target/s390x/helper.c +++ b/target/s390x/helper.c @@ -655,6 +657,10 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request) S390CPU *cpu = S390_CPU(cs); CPUS390XState *env = >env; +if (env->ex_value) { +return false; +} + if (env->psw.mask & PSW_MASK_EXT) { s390_cpu_do_interrupt(cs); return true; Thanks for the research. I've incorporated this into my patch set. r~
Re: [Qemu-devel] [PATCH] block/gluster: glfs_lseek() workaround
On Wed, May 24, 2017 at 04:50:03PM -0400, Jeff Cody wrote: > On Wed, May 24, 2017 at 11:02:02AM +0200, Niels de Vos wrote: > > On Tue, May 23, 2017 at 01:27:50PM -0400, Jeff Cody wrote: > > > On current released versions of glusterfs, glfs_lseek() will sometimes > > > return invalid values for SEEK_DATA or SEEK_HOLE. For SEEK_DATA and > > > SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in > > > the case of error: > > > > > > LSEEK(2): > > > > > > off_t lseek(int fd, off_t offset, int whence); > > > > > > [...] > > > > > > SEEK_HOLE > > > Adjust the file offset to the next hole in the file greater > > > than or equal to offset. If offset points into the middle > > > of > > > a hole, then the file offset is set to offset. If there is > > > no > > > hole past offset, then the file offset is adjusted to the > > > end > > > of the file (i.e., there is an implicit hole at the end of > > > any file). > > > > > > [...] > > > > > > RETURN VALUE > > > Upon successful completion, lseek() returns the > > > resulting > > > offset location as measured in bytes from the beginning of > > > the > > > file. On error, the value (off_t) -1 is returned and errno > > > is > > > set to indicate the error > > > > > > However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a > > > value less than the passed offset, yet greater than zero. > > > > > > For instance, here are example values observed from this call: > > > > > > offs = glfs_lseek(s->fd, start, SEEK_HOLE); > > > if (offs < 0) { > > > return -errno; /* D1 and (H3 or H4) */ > > > } > > > > > > start == 7608336384 > > > offs == 7607877632 > > > > > > This causes QEMU to abort on the assert test. When this value is > > > returned, errno is also 0. > > > > > > This is a reported and known bug to glusterfs: > > > https://bugzilla.redhat.com/show_bug.cgi?id=1425293 > > > > > > Although this is being fixed in gluster, we still should work around it > > > in QEMU, given that multiple released versions of gluster behave this > > > way. > > > > Versions of GlusterFS 3.8.0 - 3.8.8 are affected, 3.8.9 has the fix > > already and was released in February this year. We encourage users to > > update to recent versions, and provide a stable (bugfix only) update > > each month. > > I am able to reproduce this bug on a server running glusterfs 3.11.0rc0. Is > that expected? No, that really is not expected. The backport that was reported to fix it for a 3.8.4 version is definitely part of 3.11 already. Could you pass me some details about your Gluster environment and volume, either by email or in a bug? I'll try to reproduce and debug it from the Gluster side. There is a holiday tomorrow (I'm in The Netherlands), and I'm travelling the whole weekend. I might not be able to look into this before next week. Sorry about that! Thanks, Niels > > > > > The Red Hat Gluster Storage product that is often used in combination > > with QEMU (+oVirt) does unfortunately not have an update where this is > > fixed. > > > > Using an unfixed Gluster storage environment can cause QEMU to segfault. > > Although fixes exist for Gluster, not all users will have them > > installed. Preventing the segfault in QEMU due to a broken storage > > environment makes sense to me. > > > > > This patch treats the return case of (offs < start) the same as if an > > > error value other than ENXIO is returned; we will assume we learned > > > nothing, and there are no holes in the file. > > > > > > Signed-off-by: Jeff Cody> > > --- > > > block/gluster.c | 18 -- > > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/block/gluster.c b/block/gluster.c > > > index 7c76cd0..c147909e 100644 > > > --- a/block/gluster.c > > > +++ b/block/gluster.c > > > @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, > > > off_t start, > > > if (offs < 0) { > > > return -errno; /* D3 or D4 */ > > > } > > > -assert(offs >= start); > > > + > > > +if (offs < start) { > > > +/* This is not a valid return by lseek(). We are safe to just > > > return > > > + * -EIO in this case, and we'll treat it like D4. Unfortunately > > > some > > > + * versions of libgfapi will return offs < start, so an assert > > > here > > > + * will unneccesarily abort QEMU. */ > > > > This is not really correct, the problem is not in libgfapi, but in the > > protocol translator on the server-side. The version of libgfapi does not > > matter here, it is the version on the server. But that might be too much > > detail for the comment. > > > > > +return -EIO; > > > +} > > > > > > if (offs > start) { > > > /* D2: in hole, next data at
Re: [Qemu-devel] [PATCH] block/gluster: glfs_lseek() workaround
On Wed, May 24, 2017 at 11:02:02AM +0200, Niels de Vos wrote: > On Tue, May 23, 2017 at 01:27:50PM -0400, Jeff Cody wrote: > > On current released versions of glusterfs, glfs_lseek() will sometimes > > return invalid values for SEEK_DATA or SEEK_HOLE. For SEEK_DATA and > > SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in > > the case of error: > > > > LSEEK(2): > > > > off_t lseek(int fd, off_t offset, int whence); > > > > [...] > > > > SEEK_HOLE > > Adjust the file offset to the next hole in the file greater > > than or equal to offset. If offset points into the middle of > > a hole, then the file offset is set to offset. If there is no > > hole past offset, then the file offset is adjusted to the end > > of the file (i.e., there is an implicit hole at the end of > > any file). > > > > [...] > > > > RETURN VALUE > > Upon successful completion, lseek() returns the resulting > > offset location as measured in bytes from the beginning of the > > file. On error, the value (off_t) -1 is returned and errno is > > set to indicate the error > > > > However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a > > value less than the passed offset, yet greater than zero. > > > > For instance, here are example values observed from this call: > > > > offs = glfs_lseek(s->fd, start, SEEK_HOLE); > > if (offs < 0) { > > return -errno; /* D1 and (H3 or H4) */ > > } > > > > start == 7608336384 > > offs == 7607877632 > > > > This causes QEMU to abort on the assert test. When this value is > > returned, errno is also 0. > > > > This is a reported and known bug to glusterfs: > > https://bugzilla.redhat.com/show_bug.cgi?id=1425293 > > > > Although this is being fixed in gluster, we still should work around it > > in QEMU, given that multiple released versions of gluster behave this > > way. > > Versions of GlusterFS 3.8.0 - 3.8.8 are affected, 3.8.9 has the fix > already and was released in February this year. We encourage users to > update to recent versions, and provide a stable (bugfix only) update > each month. I am able to reproduce this bug on a server running glusterfs 3.11.0rc0. Is that expected? > > The Red Hat Gluster Storage product that is often used in combination > with QEMU (+oVirt) does unfortunately not have an update where this is > fixed. > > Using an unfixed Gluster storage environment can cause QEMU to segfault. > Although fixes exist for Gluster, not all users will have them > installed. Preventing the segfault in QEMU due to a broken storage > environment makes sense to me. > > > This patch treats the return case of (offs < start) the same as if an > > error value other than ENXIO is returned; we will assume we learned > > nothing, and there are no holes in the file. > > > > Signed-off-by: Jeff Cody> > --- > > block/gluster.c | 18 -- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/block/gluster.c b/block/gluster.c > > index 7c76cd0..c147909e 100644 > > --- a/block/gluster.c > > +++ b/block/gluster.c > > @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, > > off_t start, > > if (offs < 0) { > > return -errno; /* D3 or D4 */ > > } > > -assert(offs >= start); > > + > > +if (offs < start) { > > +/* This is not a valid return by lseek(). We are safe to just > > return > > + * -EIO in this case, and we'll treat it like D4. Unfortunately > > some > > + * versions of libgfapi will return offs < start, so an assert > > here > > + * will unneccesarily abort QEMU. */ > > This is not really correct, the problem is not in libgfapi, but in the > protocol translator on the server-side. The version of libgfapi does not > matter here, it is the version on the server. But that might be too much > detail for the comment. > > > +return -EIO; > > +} > > > > if (offs > start) { > > /* D2: in hole, next data at offs */ > > @@ -1307,7 +1314,14 @@ static int find_allocation(BlockDriverState *bs, > > off_t start, > > if (offs < 0) { > > return -errno; /* D1 and (H3 or H4) */ > > } > > -assert(offs >= start); > > + > > +if (offs < start) { > > +/* This is not a valid return by lseek(). We are safe to just > > return > > + * -EIO in this case, and we'll treat it like H4. Unfortunately > > some > > + * versions of libgfapi will return offs < start, so an assert > > here > > + * will unneccesarily abort QEMU. */ > > +return -EIO; > > +} > > > > if (offs > start) { > > /* > > -- > > 2.9.3 > > > > You might want to explain the problem a little different in the commit > message. It is fine too if
[Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()
Not all callers care about which BDS owns the mapping for a given range of the file. This patch merely simplifies the callers by consolidating the logic in the common call point, while guaranteeing a non-NULL file to all the driver callbacks, for no semantic change. However, this will also set the stage for a future cleanup: when a caller does not care about which BDS owns an offset, it would be nice to allow the driver to optimize things to not have to return BDRV_BLOCK_OFFSET_VALID in the first place. In the case of fragmented allocation (for example, it's fairly easy to create a qcow2 image where consecutive guest addresses are not at consecutive host addresses), the current contract requires bdrv_get_block_status() to clamp *pnum to the limit where host addresses are no longer consecutive, but allowing a NULL file means that *pnum could be set to the full length of known-allocated data. Signed-off-by: Eric Blake--- v2: new patch --- block/io.c | 15 +-- block/mirror.c | 3 +-- block/qcow2.c | 4 +--- qemu-img.c | 10 -- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/block/io.c b/block/io.c index 8e6c3fe..eea74cb 100644 --- a/block/io.c +++ b/block/io.c @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) { int64_t target_sectors, ret, nb_sectors, sector_num = 0; BlockDriverState *bs = child->bs; -BlockDriverState *file; int n; target_sectors = bdrv_nb_sectors(bs); @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) if (nb_sectors <= 0) { return 0; } -ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , ); +ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , NULL); if (ret < 0) { error_report("error getting block status at sector %" PRId64 ": %s", sector_num, strerror(-ret)); @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData { * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes * beyond the end of the disk image it will be clamped. * - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file' - * points to the BDS which the sector range is allocated in. + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and + * 'file' is non-NULL, then '*file' points to the BDS which the sector range + * is allocated in. */ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t sector_num, @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t total_sectors; int64_t n; int64_t ret, ret2; +BlockDriverState *tmpfile; +if (!file) { +file = +} *file = NULL; total_sectors = bdrv_nb_sectors(bs); if (total_sectors < 0) { @@ -1916,9 +1920,8 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { -BlockDriverState *file; int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, -); +NULL); if (ret < 0) { return ret; } diff --git a/block/mirror.c b/block/mirror.c index e86f8f8..4563ba7 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -392,7 +392,6 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) while (nb_chunks > 0 && sector_num < end) { int64_t ret; int io_sectors, io_sectors_acct; -BlockDriverState *file; enum MirrorMethod { MIRROR_METHOD_COPY, MIRROR_METHOD_ZERO, @@ -402,7 +401,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) assert(!(sector_num % sectors_per_chunk)); ret = bdrv_get_block_status_above(source, NULL, sector_num, nb_chunks * sectors_per_chunk, - _sectors, ); + _sectors, NULL); if (ret < 0) { io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors); } else if (ret & BDRV_BLOCK_DATA) { diff --git a/block/qcow2.c b/block/qcow2.c index b3ba5da..8e9e29b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2448,7 +2448,6 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, uint32_t count) { int nr; -BlockDriverState *file; int64_t res; if (start + count > bs->total_sectors) { @@ -2458,8 +2457,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, if (!count) { return true; } -res = bdrv_get_block_status_above(bs, NULL, start, count, -
[Qemu-devel] [PATCH v2 2/5] block: Guarantee that *file is set on bdrv_get_block_status()
We document that *file is valid if the return is not an error and includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract when a driver (such as blkdebug) lacks a callback. Broken in commit 67a0fd2 (v2.6), when we added the file parameter. Enhance qemu-iotest 177 to cover this, using a sequence that would print garbage or even SEGV, because it was dererefencing through uninitialized memory. [The resulting test output shows that we have less-than-ideal block status from the blkdebug driver, but that's a separate fix coming up soon.] Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is enough to fix the crash, but we can go one step further: always setting *file, even on error, means that a broken caller that blindly dereferences file without checking for error is now more likely to get a reliable SEGV instead of randomly acting on garbage, making it easier to diagnose such buggy callers. Adding an assertion that file is set where expected doesn't hurt either. CC: qemu-sta...@nongnu.org Signed-off-by: Eric Blake--- v2: drop redundant assignment --- block/io.c | 5 +++-- tests/qemu-iotests/177 | 3 +++ tests/qemu-iotests/177.out | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/block/io.c b/block/io.c index fdd7485..8e6c3fe 100644 --- a/block/io.c +++ b/block/io.c @@ -1749,6 +1749,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t n; int64_t ret, ret2; +*file = NULL; total_sectors = bdrv_nb_sectors(bs); if (total_sectors < 0) { return total_sectors; @@ -1769,11 +1770,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED; if (bs->drv->protocol_name) { ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE); +*file = bs; } return ret; } -*file = NULL; bdrv_inc_in_flight(bs); ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum, file); @@ -1783,7 +1784,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, } if (ret & BDRV_BLOCK_RAW) { -assert(ret & BDRV_BLOCK_OFFSET_VALID); +assert(ret & BDRV_BLOCK_OFFSET_VALID && *file); ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS, *pnum, pnum, file); goto out; diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177 index 2005c17..f8ed8fb 100755 --- a/tests/qemu-iotests/177 +++ b/tests/qemu-iotests/177 @@ -43,6 +43,7 @@ _supported_proto file CLUSTER_SIZE=1M size=128M options=driver=blkdebug,image.driver=qcow2 +nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG echo echo "== setting up files ==" @@ -106,6 +107,8 @@ function verify_io() } verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io +$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \ +| _filter_qemu_img_map _check_test_img diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out index e887542..b754ed4 100644 --- a/tests/qemu-iotests/177.out +++ b/tests/qemu-iotests/177.out @@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 23068672/23068672 bytes at offset 49056 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0 0x800 blkdebug::TEST_DIR/t.IMGFMT No errors were found on the image. *** done -- 2.9.4
[Qemu-devel] [PATCH v2 5/5] blkdebug: Support .bdrv_co_get_block_status
Without a passthrough status of BDRV_BLOCK_RAW, anything wrapped by blkdebug appears 100% allocated as data. Better is treating it the same as the underlying file being wrapped. Update iotest 177 for the new expected output. Signed-off-by: Eric Blake--- v2: tweak commit message --- block/blkdebug.c | 11 +++ tests/qemu-iotests/177.out | 5 - 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/block/blkdebug.c b/block/blkdebug.c index a5196e8..1ad8d65 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -642,6 +642,16 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs, return bdrv_co_pdiscard(bs->file->bs, offset, count); } +static int64_t coroutine_fn blkdebug_co_get_block_status( +BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, +BlockDriverState **file) +{ +*pnum = nb_sectors; +*file = bs->file->bs; +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | +(sector_num << BDRV_SECTOR_BITS); +} + static void blkdebug_close(BlockDriverState *bs) { BDRVBlkdebugState *s = bs->opaque; @@ -912,6 +922,7 @@ static BlockDriver bdrv_blkdebug = { .bdrv_co_flush_to_disk = blkdebug_co_flush, .bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes, .bdrv_co_pdiscard = blkdebug_co_pdiscard, +.bdrv_co_get_block_status = blkdebug_co_get_block_status, .bdrv_debug_event = blkdebug_debug_event, .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out index b754ed4..43a7778 100644 --- a/tests/qemu-iotests/177.out +++ b/tests/qemu-iotests/177.out @@ -46,6 +46,9 @@ read 30408704/30408704 bytes at offset 80740352 read 23068672/23068672 bytes at offset 49056 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File -0 0x800 blkdebug::TEST_DIR/t.IMGFMT +0 0x80TEST_DIR/t.IMGFMT +0x900x240 TEST_DIR/t.IMGFMT +0x3c0 0x110 TEST_DIR/t.IMGFMT +0x6a0 0x160 TEST_DIR/t.IMGFMT No errors were found on the image. *** done -- 2.9.4
[Qemu-devel] [PATCH v2 4/5] block: Simplify use of BDRV_BLOCK_RAW
The lone caller that cares about a return of BDRV_BLOCK_RAW (namely, io.c:bdrv_co_get_block_status) completely replaces the return value, so there is no point in passing BDRV_BLOCK_DATA. Signed-off-by: Eric Blake--- v2: fix subject, tweak commit message --- block/commit.c | 2 +- block/mirror.c | 2 +- block/raw-format.c | 2 +- block/vpc.c| 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/block/commit.c b/block/commit.c index 76a0d98..cf662ba 100644 --- a/block/commit.c +++ b/block/commit.c @@ -239,7 +239,7 @@ static int64_t coroutine_fn bdrv_commit_top_get_block_status( { *pnum = nb_sectors; *file = bs->backing->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/mirror.c b/block/mirror.c index 4563ba7..432d58d 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1046,7 +1046,7 @@ static int64_t coroutine_fn bdrv_mirror_top_get_block_status( { *pnum = nb_sectors; *file = bs->backing->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/raw-format.c b/block/raw-format.c index 36e6503..1136eba 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -259,7 +259,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, *pnum = nb_sectors; *file = bs->file->bs; sector_num += s->offset / BDRV_SECTOR_SIZE; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/vpc.c b/block/vpc.c index ecfee77..048504b 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -701,7 +701,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, if (be32_to_cpu(footer->type) == VHD_FIXED) { *pnum = nb_sectors; *file = bs->file->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } -- 2.9.4
[Qemu-devel] [PATCH v2 0/5] more blkdebug tweaks
I found a crasher and some odd behavior while rebasing my bdrv_get_block_status series, so I figured I'd get these things fixed first. This is based on top of Max's block branch. Since v1: - patch 1: patch open_f instead of openfile [Fam] - patch 2: drop redundant assignment - patch 3: new - patch 4, 5: tweak commit message (no diff from v1, since I forgot to tag that one) Available as a tag at: git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v2 Eric Blake (5): qemu-io: Don't die on second open block: Guarantee that *file is set on bdrv_get_block_status() block: Allow NULL file for bdrv_get_block_status() block: Simplify use of BDRV_BLOCK_RAW blkdebug: Support .bdrv_co_get_block_status block/blkdebug.c | 11 +++ block/commit.c | 2 +- block/io.c | 20 block/mirror.c | 5 ++--- block/qcow2.c | 4 +--- block/raw-format.c | 2 +- block/vpc.c| 2 +- qemu-img.c | 10 -- qemu-io.c | 7 --- tests/qemu-iotests/177 | 3 +++ tests/qemu-iotests/177.out | 5 + 11 files changed, 45 insertions(+), 26 deletions(-) -- 2.9.4
[Qemu-devel] [PATCH v2 1/5] qemu-io: Don't die on second open
Most callback commands in qemu-io return 0 to keep the interpreter loop running, or 1 to quit immediately. However, open_f() just passed through the return value of openfile(), which has different semantics of returning 0 if a file was opened, or 1 on any failure. As a result of mixing the return semantics, we are forcing the qemu-io interpreter to exit early on any failures, which is rather annoying when some of the failures are obviously trying to give the user a hint of how to proceed (if we didn't then kill qemu-io out from under the user's feet): $ qemu-io qemu-io> open foo qemu-io> open foo file open already, try 'help close' $ echo $? 0 Meanwhile, we WANT openfile() to report failures, as it is the way that 'qemu-io -c "$something" no_such_file' knows to exit early rather than attempting $something. So the solution is to fix open_f() to always return 0 (when we are in interactive mode, even failure to open should not end the session), and save the return value of openfile() for command line use in main(). This has been awkward since at least as far back as commit e3aff4f, in 2009. Signed-off-by: Eric Blake--- v2: fix open_f(), not openfile() --- qemu-io.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/qemu-io.c b/qemu-io.c index 34fa8a1..b3febc2 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -230,13 +230,14 @@ static int open_f(BlockBackend *blk, int argc, char **argv) qemu_opts_reset(_opts); if (optind == argc - 1) { -return openfile(argv[optind], flags, writethrough, force_share, opts); +openfile(argv[optind], flags, writethrough, force_share, opts); } else if (optind == argc) { -return openfile(NULL, flags, writethrough, force_share, opts); +openfile(NULL, flags, writethrough, force_share, opts); } else { QDECREF(opts); -return qemuio_command_usage(_cmd); +qemuio_command_usage(_cmd); } +return 0; } static int quit_f(BlockBackend *blk, int argc, char **argv) -- 2.9.4
[Qemu-devel] [PATCH v2 32/33] target/s390x: Use atomic operations for COMPARE SWAP PURGE
Also provide the cross-cpu tlb flushing required by the PoO. Signed-off-by: Richard Henderson--- target/s390x/helper.h | 2 +- target/s390x/insn-data.def | 2 +- target/s390x/mem_helper.c | 32 target/s390x/translate.c | 42 ++ 4 files changed, 48 insertions(+), 30 deletions(-) diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 3819409..cc451c7 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -107,13 +107,13 @@ DEF_HELPER_FLAGS_2(tprot, TCG_CALL_NO_RWG, i32, i64, i64) DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) -DEF_HELPER_3(csp, i32, env, i32, i64) DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) DEF_HELPER_4(sigp, i32, env, i64, i32, i64) DEF_HELPER_FLAGS_2(sacf, TCG_CALL_NO_WG, void, env, i64) DEF_HELPER_FLAGS_3(ipte, TCG_CALL_NO_RWG, void, env, i64, i64) DEF_HELPER_FLAGS_1(ptlb, TCG_CALL_NO_RWG, void, env) +DEF_HELPER_FLAGS_1(purge, TCG_CALL_NO_RWG, void, env) DEF_HELPER_2(lra, i64, env, i64) DEF_HELPER_FLAGS_2(lura, TCG_CALL_NO_WG, i64, env, i64) DEF_HELPER_FLAGS_2(lurag, TCG_CALL_NO_WG, i64, env, i64) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index 3c3541c..4c91f30 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -837,7 +837,7 @@ #ifndef CONFIG_USER_ONLY /* COMPARE AND SWAP AND PURGE */ -C(0xb250, CSP, RRE, Z, 0, ra2, 0, 0, csp, 0) +D(0xb250, CSP, RRE, Z, r1_32u, ra2, r1_P, 0, csp, 0, MO_TEUL) /* DIAGNOSE (KVM hypercall) */ C(0x8300, DIAG,RSI, Z, 0, 0, 0, 0, diag, 0) /* INSERT STORAGE KEY EXTENDED */ diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index fa03129..4b96c27 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1056,30 +1056,6 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2) return re >> 1; } -/* compare and swap and purge */ -uint32_t HELPER(csp)(CPUS390XState *env, uint32_t r1, uint64_t r2) -{ -S390CPU *cpu = s390_env_get_cpu(env); -uint32_t cc; -uint32_t o1 = env->regs[r1]; -uint64_t a2 = r2 & ~3ULL; -uint32_t o2 = cpu_ldl_data(env, a2); - -if (o1 == o2) { -cpu_stl_data(env, a2, env->regs[(r1 + 1) & 15]); -if (r2 & 0x3) { -/* flush TLB / ALB */ -tlb_flush(CPU(cpu)); -} -cc = 0; -} else { -env->regs[r1] = (env->regs[r1] & 0xULL) | o2; -cc = 1; -} - -return cc; -} - uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) { uintptr_t ra = GETPC(); @@ -1161,6 +1137,14 @@ void HELPER(ptlb)(CPUS390XState *env) tlb_flush(CPU(cpu)); } +/* flush global tlb */ +void HELPER(purge)(CPUS390XState *env) +{ +S390CPU *cpu = s390_env_get_cpu(env); + +tlb_flush_all_cpus_synced(CPU(cpu)); +} + /* load using real address */ uint64_t HELPER(lura)(CPUS390XState *env, uint64_t addr) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index b7b4843..a3fb324 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2001,11 +2001,45 @@ static ExitStatus op_cdsg(DisasContext *s, DisasOps *o) #ifndef CONFIG_USER_ONLY static ExitStatus op_csp(DisasContext *s, DisasOps *o) { -TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); +TCGMemOp mop = s->insn->data; +TCGv_i64 addr, old, cc; +TCGLabel *lab = gen_new_label(); + +/* Note that in1 = R1 (zero-extended expected value), + out = R1 (original reg), out2 = R1+1 (new value). */ + check_privileged(s); -gen_helper_csp(cc_op, cpu_env, r1, o->in2); -tcg_temp_free_i32(r1); -set_cc_static(s); +addr = tcg_temp_new_i64(); +old = tcg_temp_new_i64(); +tcg_gen_andi_i64(addr, o->in2, -1ULL << (mop & MO_SIZE)); +tcg_gen_atomic_cmpxchg_i64(old, addr, o->in1, o->out2, + get_mem_index(s), mop | MO_ALIGN); +tcg_temp_free_i64(addr); + +/* Are the memory and expected values (un)equal? */ +cc = tcg_temp_new_i64(); +tcg_gen_setcond_i64(TCG_COND_NE, cc, o->in1, old); +tcg_gen_extrl_i64_i32(cc_op, cc); + +/* Write back the output now, so that it happens before the + following branch, so that we don't need local temps. */ +if ((mop & MO_SIZE) == MO_32) { +tcg_gen_deposit_i64(o->out, o->out, old, 0, 32); +} else { +tcg_gen_mov_i64(o->out, old); +} +tcg_temp_free_i64(old); + +/* If the comparison was equal, and the LSB of R2 was set, + then we need to flush the TLB (for all cpus). */ +tcg_gen_xori_i64(cc, cc, 1); +tcg_gen_and_i64(cc, cc, o->in2); +tcg_gen_brcondi_i64(TCG_COND_EQ, cc, 0, lab); +tcg_temp_free_i64(cc); + +
[Qemu-devel] [PATCH v2 30/33] target/s390x: Fix some helper_ex problems
(1) The OR of the low bits or R1 into INSN were not being done consistently; it was forgotten along all but the SVC path. (2) The setting of ILEN was wrong on SVC path for EXRL. (3) The data load for ICM read too much. Fix these by consolidating data load at the beginning, using get_ilen to control the number of bytes loaded, and ORing in the byte from R1. Use extract64 from the full aligned insn to extract arguments. Pass in ILEN rather than RET as the more natural way to give the required data along the SVC path. Modify ENV->CC_OP directly rather than include it in the functional interface. Signed-off-by: Richard Henderson--- target/s390x/helper.h | 2 +- target/s390x/mem_helper.c | 135 +- target/s390x/translate.c | 8 +-- 3 files changed, 78 insertions(+), 67 deletions(-) diff --git a/target/s390x/helper.h b/target/s390x/helper.h index ea35834..3819409 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -14,7 +14,7 @@ DEF_HELPER_4(srst, i64, env, i64, i64, i64) DEF_HELPER_4(clst, i64, env, i64, i64, i64) DEF_HELPER_FLAGS_4(mvpg, TCG_CALL_NO_WG, i32, env, i64, i64, i64) DEF_HELPER_4(mvst, i64, env, i64, i64, i64) -DEF_HELPER_5(ex, i32, env, i32, i64, i64, i64) +DEF_HELPER_4(ex, void, env, i32, i64, i64) DEF_HELPER_FLAGS_4(stam, TCG_CALL_NO_WG, void, env, i32, i64, i32) DEF_HELPER_FLAGS_4(lam, TCG_CALL_NO_WG, void, env, i32, i64, i32) DEF_HELPER_4(mvcle, i32, env, i32, i64, i32) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index a73d486..fa03129 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1245,76 +1245,87 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) in other words: tricky... currently implemented by interpreting the cases it is most commonly used. */ -uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, -uint64_t addr, uint64_t ret) +void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr) { S390CPU *cpu = s390_env_get_cpu(env); -uint16_t insn = cpu_lduw_code(env, addr); - -HELPER_LOG("%s: v1 0x%lx addr 0x%lx insn 0x%x\n", __func__, v1, addr, - insn); -if ((insn & 0xf0ff) == 0xd000) { -uint32_t l, insn2, b1, b2, d1, d2; - -l = v1 & 0xff; -insn2 = cpu_ldl_code(env, addr + 2); -b1 = (insn2 >> 28) & 0xf; -b2 = (insn2 >> 12) & 0xf; -d1 = (insn2 >> 16) & 0xfff; -d2 = insn2 & 0xfff; -switch (insn & 0xf00) { -case 0x200: +uint64_t insn = cpu_lduw_code(env, addr); +uint8_t opc = insn >> 8; + +/* Or in the contents of R1[56:63]. */ +insn |= r1 & 0xff; + +/* Load the rest of the instruction. */ +insn <<= 48; +switch (get_ilen(opc)) { +case 2: +break; +case 4: +insn |= (uint64_t)cpu_lduw_code(env, addr + 2) << 32; +break; +case 6: +insn |= (uint64_t)(uint32_t)cpu_ldl_code(env, addr + 2) << 16; +break; +default: +g_assert_not_reached(); +} + +HELPER_LOG("%s: addr 0x%lx insn 0x%" PRIx64 "\n", __func__, addr, insn); + +if ((opc & 0xf0) == 0xd0) { +uint32_t l, b1, b2, d1, d2; + +l = extract64(insn, 48, 8); +b1 = extract64(insn, 44, 4); +b2 = extract64(insn, 28, 4); +d1 = extract64(insn, 32, 12); +d2 = extract64(insn, 16, 12); +switch (opc & 0xf) { +case 0x2: do_helper_mvc(env, l, get_address(env, 0, b1, d1), get_address(env, 0, b2, d2), 0); -break; -case 0x400: -cc = do_helper_nc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -break; -case 0x500: -cc = do_helper_clc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -break; -case 0x600: -cc = do_helper_oc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -break; -case 0x700: -cc = do_helper_xc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2), 0); -break; -case 0xc00: +return; +case 0x4: +env->cc_op = do_helper_nc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +return; +case 0x5: +env->cc_op = do_helper_clc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +return; +case 0x6: +env->cc_op = do_helper_oc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +return; +case 0x7: +env->cc_op =
[Qemu-devel] [PATCH v2 29/33] target/s390x: Use unwind data for helper_mvcs/mvcp
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 8 ++-- target/s390x/translate.c | 2 -- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 17d8257..a73d486 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1082,6 +1082,7 @@ uint32_t HELPER(csp)(CPUS390XState *env, uint32_t r1, uint64_t r2) uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) { +uintptr_t ra = GETPC(); int cc = 0, i; HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n", @@ -1095,7 +1096,8 @@ uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) /* XXX replace w/ memcpy */ for (i = 0; i < l; i++) { -cpu_stb_secondary(env, a1 + i, cpu_ldub_primary(env, a2 + i)); +uint8_t x = cpu_ldub_primary_ra(env, a2 + i, ra); +cpu_stb_secondary_ra(env, a1 + i, x, ra); } return cc; @@ -1103,6 +1105,7 @@ uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) { +uintptr_t ra = GETPC(); int cc = 0, i; HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n", @@ -1116,7 +1119,8 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) /* XXX replace w/ memcpy */ for (i = 0; i < l; i++) { -cpu_stb_primary(env, a1 + i, cpu_ldub_secondary(env, a2 + i)); +uint8_t x = cpu_ldub_secondary_ra(env, a2 + i, ra); +cpu_stb_primary_ra(env, a1 + i, x, ra); } return cc; diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 141be22..422bbf1 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2889,7 +2889,6 @@ static ExitStatus op_mvcp(DisasContext *s, DisasOps *o) { int r1 = get_field(s->fields, l1); check_privileged(s); -potential_page_fault(s); gen_helper_mvcp(cc_op, cpu_env, regs[r1], o->addr1, o->in2); set_cc_static(s); return NO_EXIT; @@ -2899,7 +2898,6 @@ static ExitStatus op_mvcs(DisasContext *s, DisasOps *o) { int r1 = get_field(s->fields, l1); check_privileged(s); -potential_page_fault(s); gen_helper_mvcs(cc_op, cpu_env, regs[r1], o->addr1, o->in2); set_cc_static(s); return NO_EXIT; -- 2.9.4
[Qemu-devel] [PATCH v2 28/33] target/s390x: Use unwind data for helper_lra
Fix saving exception_index around mmu_translate; eliminate a dead store. Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 6 +++--- target/s390x/translate.c | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index a8c85c9..17d8257 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1208,17 +1208,17 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) { CPUState *cs = CPU(s390_env_get_cpu(env)); uint32_t cc = 0; -int old_exc = cs->exception_index; uint64_t asc = env->psw.mask & PSW_MASK_ASC; uint64_t ret; -int flags; +int old_exc, flags; /* XXX incomplete - has more corner cases */ if (!(env->psw.mask & PSW_MASK_64) && (addr >> 32)) { +cpu_restore_state(cs, GETPC()); program_interrupt(env, PGM_SPECIAL_OP, 2); } -cs->exception_index = old_exc; +old_exc = cs->exception_index; if (mmu_translate(env, addr, 0, asc, , , true)) { cc = 3; } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 7b9c111..141be22 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2560,7 +2560,6 @@ static ExitStatus op_lctlg(DisasContext *s, DisasOps *o) static ExitStatus op_lra(DisasContext *s, DisasOps *o) { check_privileged(s); -potential_page_fault(s); gen_helper_lra(o->out, cpu_env, o->in2); set_cc_static(s); return NO_EXIT; -- 2.9.4
[Qemu-devel] [PATCH v2 27/33] target/s390x: Use unwind data for helper_tprot
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 1 - target/s390x/translate.c | 1 - 2 files changed, 2 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 7df2e53..a8c85c9 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -970,7 +970,6 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr) uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2) { /* XXX implement */ - return 0; } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index cd017ce..7b9c111 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4049,7 +4049,6 @@ static ExitStatus op_testblock(DisasContext *s, DisasOps *o) static ExitStatus op_tprot(DisasContext *s, DisasOps *o) { -potential_page_fault(s); gen_helper_tprot(cc_op, o->addr1, o->in2); set_cc_static(s); return NO_EXIT; -- 2.9.4
[Qemu-devel] [PATCH v2 33/33] target/s390x: Implement CSPG
Signed-off-by: Richard Henderson--- target/s390x/insn-data.def | 1 + target/s390x/translate.c | 1 + 2 files changed, 2 insertions(+) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index 4c91f30..f818437 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -838,6 +838,7 @@ #ifndef CONFIG_USER_ONLY /* COMPARE AND SWAP AND PURGE */ D(0xb250, CSP, RRE, Z, r1_32u, ra2, r1_P, 0, csp, 0, MO_TEUL) +D(0xb98a, CSPG,RRE, DAT_ENH, r1_o, ra2, r1_P, 0, csp, 0, MO_TEQ) /* DIAGNOSE (KVM hypercall) */ C(0x8300, DIAG,RSI, Z, 0, 0, 0, 0, diag, 0) /* INSERT STORAGE KEY EXTENDED */ diff --git a/target/s390x/translate.c b/target/s390x/translate.c index a3fb324..4bd16d9 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1195,6 +1195,7 @@ typedef enum DisasFacility { FAC_SFLE, /* store facility list extended */ FAC_ILA,/* interlocked access facility 1 */ FAC_LPP,/* load-program-parameter */ +FAC_DAT_ENH,/* DAT-enhancement */ } DisasFacility; struct DisasInsn { -- 2.9.4
[Qemu-devel] [PATCH v2 31/33] target/s390x: Fix EXECUTE with R1==0
The PoO specifies that when R1==0, no ORing into the insn loaded from storage takes place. Load a zero for this case. Signed-off-by: Richard Henderson--- target/s390x/insn-data.def | 4 ++-- target/s390x/translate.c | 14 +- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index cac0f51..3c3541c 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -327,9 +327,9 @@ C(0xeb57, XIY, SIY, LD, m1_8u, i2_8u, new, m1_8, xor, nz64) /* EXECUTE */ -C(0x4400, EX, RX_a, Z, r1_o, a2, 0, 0, ex, 0) +C(0x4400, EX, RX_a, Z, 0, a2, 0, 0, ex, 0) /* EXECUTE RELATIVE LONG */ -C(0xc600, EXRL,RIL_b, EE, r1_o, ri2, 0, 0, ex, 0) +C(0xc600, EXRL,RIL_b, EE, 0, ri2, 0, 0, ex, 0) /* EXTRACT ACCESS */ C(0xb24f, EAR, RRE, Z, 0, 0, new, r1_32, ear, 0) diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 921a842..b7b4843 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2159,15 +2159,27 @@ static ExitStatus op_ex(DisasContext *s, DisasOps *o) MVC inside of memcpy, which needs a helper call anyway. So perhaps this doesn't bear thinking about any further. */ +int r1 = get_field(s->fields, r1); TCGv_i32 ilen; +TCGv_i64 v1; update_psw_addr(s); gen_op_calc_cc(s); +if (r1 == 0) { +v1 = tcg_const_i64(0); +} else { +v1 = regs[r1]; +} + ilen = tcg_const_i32(s->next_pc - s->pc); -gen_helper_ex(cpu_env, ilen, o->in1, o->in2); +gen_helper_ex(cpu_env, ilen, v1, o->in2); tcg_temp_free_i32(ilen); +if (r1 == 0) { +tcg_temp_free_i64(v1); +} + return NO_EXIT; } -- 2.9.4
[Qemu-devel] [PATCH v2 23/33] target/s390x: Use unwind data for helper_lctlg
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 8 target/s390x/translate.c | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index ff12777..68e3817 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -848,20 +848,20 @@ void HELPER(cdsg)(CPUS390XState *env, uint64_t addr, #if !defined(CONFIG_USER_ONLY) void HELPER(lctlg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { +uintptr_t ra = GETPC(); S390CPU *cpu = s390_env_get_cpu(env); bool PERchanged = false; -int i; uint64_t src = a2; -uint64_t val; +uint32_t i; for (i = r1;; i = (i + 1) % 16) { -val = cpu_ldq_data(env, src); +uint64_t val = cpu_ldq_data_ra(env, src, ra); if (env->cregs[i] != val && i >= 9 && i <= 11) { PERchanged = true; } env->cregs[i] = val; HELPER_LOG("load ctl %d from 0x%" PRIx64 " == 0x%" PRIx64 "\n", - i, src, env->cregs[i]); + i, src, val); src += sizeof(uint64_t); if (i == r3) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index fed9f17..65ae573 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2552,7 +2552,6 @@ static ExitStatus op_lctlg(DisasContext *s, DisasOps *o) TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); check_privileged(s); -potential_page_fault(s); gen_helper_lctlg(cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); -- 2.9.4
[Qemu-devel] [PATCH v2 18/33] target/s390x: Use unwind data for helper_cksm
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 11 ++- target/s390x/translate.c | 1 - 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index f5a3044..d4ee364 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -633,6 +633,7 @@ uint32_t HELPER(clcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint64_t HELPER(cksm)(CPUS390XState *env, uint64_t r1, uint64_t src, uint64_t src_len) { +uintptr_t ra = GETPC(); uint64_t max_len, len; uint64_t cksm = (uint32_t)r1; @@ -642,21 +643,21 @@ uint64_t HELPER(cksm)(CPUS390XState *env, uint64_t r1, /* Process full words as available. */ for (len = 0; len + 4 <= max_len; len += 4, src += 4) { -cksm += (uint32_t)cpu_ldl_data(env, src); +cksm += (uint32_t)cpu_ldl_data_ra(env, src, ra); } switch (max_len - len) { case 1: -cksm += cpu_ldub_data(env, src) << 24; +cksm += cpu_ldub_data_ra(env, src, ra) << 24; len += 1; break; case 2: -cksm += cpu_lduw_data(env, src) << 16; +cksm += cpu_lduw_data_ra(env, src, ra) << 16; len += 2; break; case 3: -cksm += cpu_lduw_data(env, src) << 16; -cksm += cpu_ldub_data(env, src + 2) << 8; +cksm += cpu_lduw_data_ra(env, src, ra) << 16; +cksm += cpu_ldub_data_ra(env, src + 2, ra) << 8; len += 3; break; } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 9270067..76910bc 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1866,7 +1866,6 @@ static ExitStatus op_cksm(DisasContext *s, DisasOps *o) int r2 = get_field(s->fields, r2); TCGv_i64 len = tcg_temp_new_i64(); -potential_page_fault(s); gen_helper_cksm(len, cpu_env, o->in1, o->in2, regs[r2 + 1]); set_cc_static(s); return_low128(o->out); -- 2.9.4
[Qemu-devel] [PATCH v2 15/33] target/s390x: Use unwind data for helper_mvcl
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 7 --- target/s390x/translate.c | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 2acc984..49cfc9b 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -501,6 +501,7 @@ void HELPER(stam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) /* move long */ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2) { +uintptr_t ra = GETPC(); uint64_t destlen = env->regs[r1 + 1] & 0xff; uint64_t dest = get_address_31fix(env, r1); uint64_t srclen = env->regs[r2 + 1] & 0xff; @@ -522,12 +523,12 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2) } for (; destlen && srclen; src++, dest++, destlen--, srclen--) { -v = cpu_ldub_data(env, src); -cpu_stb_data(env, dest, v); +v = cpu_ldub_data_ra(env, src, ra); +cpu_stb_data_ra(env, dest, v, ra); } for (; destlen; dest++, destlen--) { -cpu_stb_data(env, dest, pad); +cpu_stb_data_ra(env, dest, pad, ra); } env->regs[r1 + 1] = destlen; diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 7af2a0b..fb2d6ff 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2871,7 +2871,6 @@ static ExitStatus op_mvcl(DisasContext *s, DisasOps *o) { TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r2 = tcg_const_i32(get_field(s->fields, r2)); -potential_page_fault(s); gen_helper_mvcl(cc_op, cpu_env, r1, r2); tcg_temp_free_i32(r1); tcg_temp_free_i32(r2); -- 2.9.4
[Qemu-devel] [PATCH v2 25/33] target/s390x: Use unwind data for helper_stctl
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 10 ++ target/s390x/translate.c | 2 -- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 614cdb2..b64c04e 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -907,11 +907,12 @@ void HELPER(lctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) void HELPER(stctg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { -int i; +uintptr_t ra = GETPC(); uint64_t dest = a2; +uint32_t i; for (i = r1;; i = (i + 1) % 16) { -cpu_stq_data(env, dest, env->cregs[i]); +cpu_stq_data_ra(env, dest, env->cregs[i], ra); dest += sizeof(uint64_t); if (i == r3) { @@ -922,11 +923,12 @@ void HELPER(stctg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { -int i; +uintptr_t ra = GETPC(); uint64_t dest = a2; +uint32_t i; for (i = r1;; i = (i + 1) % 16) { -cpu_stl_data(env, dest, env->cregs[i]); +cpu_stl_data_ra(env, dest, env->cregs[i], ra); dest += sizeof(uint32_t); if (i == r3) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 26f6b37..669da89 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3612,7 +3612,6 @@ static ExitStatus op_stctg(DisasContext *s, DisasOps *o) TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); check_privileged(s); -potential_page_fault(s); gen_helper_stctg(cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); @@ -3624,7 +3623,6 @@ static ExitStatus op_stctl(DisasContext *s, DisasOps *o) TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); check_privileged(s); -potential_page_fault(s); gen_helper_stctl(cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); -- 2.9.4
[Qemu-devel] [PATCH v2 24/33] target/s390x: Use unwind data for helper_lctl
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 9 + target/s390x/translate.c | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 68e3817..614cdb2 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -878,18 +878,19 @@ void HELPER(lctlg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) void HELPER(lctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { +uintptr_t ra = GETPC(); S390CPU *cpu = s390_env_get_cpu(env); bool PERchanged = false; -int i; uint64_t src = a2; -uint32_t val; +uint32_t i; for (i = r1;; i = (i + 1) % 16) { -val = cpu_ldl_data(env, src); +uint32_t val = cpu_ldl_data_ra(env, src, ra); if ((uint32_t)env->cregs[i] != val && i >= 9 && i <= 11) { PERchanged = true; } -env->cregs[i] = (env->cregs[i] & 0xULL) | val; +env->cregs[i] = deposit64(env->cregs[i], 0, 32, val); +HELPER_LOG("load ctl %d from 0x%" PRIx64 " == 0x%x\n", i, src, val); src += sizeof(uint32_t); if (i == r3) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 65ae573..26f6b37 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2540,7 +2540,6 @@ static ExitStatus op_lctl(DisasContext *s, DisasOps *o) TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); check_privileged(s); -potential_page_fault(s); gen_helper_lctl(cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); -- 2.9.4
[Qemu-devel] [PATCH v2 17/33] target/s390x: Use unwind data for helper_clcle
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 6 +++--- target/s390x/translate.c | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 8a095ad..f5a3044 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -595,12 +595,12 @@ uint32_t HELPER(mvcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t HELPER(clcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { +uintptr_t ra = GETPC(); uint64_t destlen = env->regs[r1 + 1]; uint64_t dest = get_address_31fix(env, r1); uint64_t srclen = env->regs[r3 + 1]; uint64_t src = get_address_31fix(env, r3); uint8_t pad = a2 & 0xff; -uint8_t v1 = 0, v2 = 0; uint32_t cc = 0; if (!(destlen || srclen)) { @@ -612,8 +612,8 @@ uint32_t HELPER(clcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, } for (; destlen || srclen; src++, dest++, destlen--, srclen--) { -v1 = srclen ? cpu_ldub_data(env, src) : pad; -v2 = destlen ? cpu_ldub_data(env, dest) : pad; +uint8_t v1 = srclen ? cpu_ldub_data_ra(env, src, ra) : pad; +uint8_t v2 = destlen ? cpu_ldub_data_ra(env, dest, ra) : pad; if (v1 != v2) { cc = (v1 < v2) ? 1 : 2; break; diff --git a/target/s390x/translate.c b/target/s390x/translate.c index b42acae..9270067 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1915,7 +1915,6 @@ static ExitStatus op_clcle(DisasContext *s, DisasOps *o) { TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); -potential_page_fault(s); gen_helper_clcle(cc_op, cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); -- 2.9.4
[Qemu-devel] [PATCH v2 14/33] target/s390x: Use unwind data for helper_stam
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 3 ++- target/s390x/translate.c | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 88e817a..2acc984 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -485,10 +485,11 @@ void HELPER(lam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) /* store access registers r1 to r3 in memory at a2 */ void HELPER(stam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { +uintptr_t ra = GETPC(); int i; for (i = r1;; i = (i + 1) % 16) { -cpu_stl_data(env, a2, env->aregs[i]); +cpu_stl_data_ra(env, a2, env->aregs[i], ra); a2 += 4; if (i == r3) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index dca2096..7af2a0b 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3862,7 +3862,6 @@ static ExitStatus op_stam(DisasContext *s, DisasOps *o) { TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); -potential_page_fault(s); gen_helper_stam(cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); -- 2.9.4
[Qemu-devel] [PATCH v2 26/33] target/s390x: Use unwind data for helper_testblock
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 3 +++ target/s390x/translate.c | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index b64c04e..7df2e53 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -939,6 +939,7 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr) { +uintptr_t ra = GETPC(); CPUState *cs = CPU(s390_env_get_cpu(env)); uint64_t abs_addr; int i; @@ -947,12 +948,14 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr) abs_addr = mmu_real2abs(env, real_addr) & TARGET_PAGE_MASK; if (!address_space_access_valid(_space_memory, abs_addr, TARGET_PAGE_SIZE, true)) { +cpu_restore_state(cs, ra); program_interrupt(env, PGM_ADDRESSING, 4); return 1; } /* Check low-address protection */ if ((env->cregs[0] & CR0_LOWPROT) && real_addr < 0x2000) { +cpu_restore_state(cs, ra); program_interrupt(env, PGM_PROTECTION, 4); return 1; } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 669da89..cd017ce 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4042,7 +4042,6 @@ static ExitStatus op_tcxb(DisasContext *s, DisasOps *o) static ExitStatus op_testblock(DisasContext *s, DisasOps *o) { check_privileged(s); -potential_page_fault(s); gen_helper_testblock(cc_op, cpu_env, o->in2); set_cc_static(s); return NO_EXIT; -- 2.9.4
[Qemu-devel] [PATCH v2 10/33] target/s390x: Use unwind data for helper_clst
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 5 +++-- target/s390x/translate.c | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 33d83e5..af2801e 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -362,6 +362,7 @@ uint64_t HELPER(srst)(CPUS390XState *env, uint64_t r0, uint64_t end, /* unsigned string compare (c is string terminator) */ uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c, uint64_t s1, uint64_t s2) { +uintptr_t ra = GETPC(); uint32_t len; c = c & 0xff; @@ -371,8 +372,8 @@ uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c, uint64_t s1, uint64_t s2) /* Lest we fail to service interrupts in a timely manner, limit the amount of work we're willing to do. For now, let's cap at 8k. */ for (len = 0; len < 0x2000; ++len) { -uint8_t v1 = cpu_ldub_data(env, s1 + len); -uint8_t v2 = cpu_ldub_data(env, s2 + len); +uint8_t v1 = cpu_ldub_data_ra(env, s1 + len, ra); +uint8_t v2 = cpu_ldub_data_ra(env, s2 + len, ra); if (v1 == v2) { if (v1 == c) { /* Equal. CC=0, and don't advance the registers. */ diff --git a/target/s390x/translate.c b/target/s390x/translate.c index cd33c51..a24e288 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1937,7 +1937,6 @@ static ExitStatus op_clm(DisasContext *s, DisasOps *o) static ExitStatus op_clst(DisasContext *s, DisasOps *o) { -potential_page_fault(s); gen_helper_clst(o->in1, cpu_env, regs[0], o->in1, o->in2); set_cc_static(s); return_low128(o->in2); -- 2.9.4
[Qemu-devel] [PATCH v2 21/33] target/s390x: Use unwind data for helper_tre
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 15 --- target/s390x/translate.c | 1 - 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index b37a963..fd6dbf7 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -744,9 +744,11 @@ void HELPER(tr)(CPUS390XState *env, uint32_t len, uint64_t array, uint64_t HELPER(tre)(CPUS390XState *env, uint64_t array, uint64_t len, uint64_t trans) { +uintptr_t ra = GETPC(); uint8_t end = env->regs[0] & 0xff; uint64_t l = len; uint64_t i; +uint32_t cc = 0; if (!(env->psw.mask & PSW_MASK_64)) { array &= 0x7fff; @@ -757,25 +759,24 @@ uint64_t HELPER(tre)(CPUS390XState *env, uint64_t array, amount of work we're willing to do. For now, let's cap at 8k. */ if (l > 0x2000) { l = 0x2000; -env->cc_op = 3; -} else { -env->cc_op = 0; +cc = 3; } for (i = 0; i < l; i++) { uint8_t byte, new_byte; -byte = cpu_ldub_data(env, array + i); +byte = cpu_ldub_data_ra(env, array + i, ra); if (byte == end) { -env->cc_op = 1; +cc = 1; break; } -new_byte = cpu_ldub_data(env, trans + byte); -cpu_stb_data(env, array + i, new_byte); +new_byte = cpu_ldub_data_ra(env, trans + byte, ra); +cpu_stb_data_ra(env, array + i, new_byte, ra); } +env->cc_op = cc; env->retxl = len - i; return array + i; } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 1842cc3..447ba07 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4073,7 +4073,6 @@ static ExitStatus op_tr(DisasContext *s, DisasOps *o) static ExitStatus op_tre(DisasContext *s, DisasOps *o) { -potential_page_fault(s); gen_helper_tre(o->out, cpu_env, o->out, o->out2, o->in2); return_low128(o->out2); set_cc_static(s); -- 2.9.4
[Qemu-devel] [PATCH v2 16/33] target/s390x: Use unwind data for helper_mvcle
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 7 --- target/s390x/translate.c | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 49cfc9b..8a095ad 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -544,6 +544,7 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2) uint32_t HELPER(mvcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { +uintptr_t ra = GETPC(); uint64_t destlen = env->regs[r1 + 1]; uint64_t dest = env->regs[r1]; uint64_t srclen = env->regs[r3 + 1]; @@ -572,12 +573,12 @@ uint32_t HELPER(mvcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, } for (; destlen && srclen; src++, dest++, destlen--, srclen--) { -v = cpu_ldub_data(env, src); -cpu_stb_data(env, dest, v); +v = cpu_ldub_data_ra(env, src, ra); +cpu_stb_data_ra(env, dest, v, ra); } for (; destlen; dest++, destlen--) { -cpu_stb_data(env, dest, pad); +cpu_stb_data_ra(env, dest, pad, ra); } env->regs[r1 + 1] = destlen; diff --git a/target/s390x/translate.c b/target/s390x/translate.c index fb2d6ff..b42acae 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2882,7 +2882,6 @@ static ExitStatus op_mvcle(DisasContext *s, DisasOps *o) { TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); -potential_page_fault(s); gen_helper_mvcle(cc_op, cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); -- 2.9.4
[Qemu-devel] [PATCH v2 22/33] target/s390x: Use unwind data for helper_trt
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 28 target/s390x/translate.c | 1 - 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index fd6dbf7..ff12777 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -781,25 +781,29 @@ uint64_t HELPER(tre)(CPUS390XState *env, uint64_t array, return array + i; } -uint32_t HELPER(trt)(CPUS390XState *env, uint32_t len, uint64_t array, - uint64_t trans) +static uint32_t do_helper_trt(CPUS390XState *env, uint32_t len, uint64_t array, + uint64_t trans, uintptr_t ra) { -uint32_t cc = 0; -int i; +uint32_t i; for (i = 0; i <= len; i++) { -uint8_t byte = cpu_ldub_data(env, array + i); -uint8_t sbyte = cpu_ldub_data(env, trans + byte); +uint8_t byte = cpu_ldub_data_ra(env, array + i, ra); +uint8_t sbyte = cpu_ldub_data_ra(env, trans + byte, ra); if (sbyte != 0) { env->regs[1] = array + i; -env->regs[2] = (env->regs[2] & ~0xff) | sbyte; -cc = (i == len) ? 2 : 1; -break; +env->regs[2] = deposit64(env->regs[2], 0, 8, sbyte); +return (i == len) ? 2 : 1; } } -return cc; +return 0; +} + +uint32_t HELPER(trt)(CPUS390XState *env, uint32_t len, uint64_t array, + uint64_t trans) +{ +return do_helper_trt(env, len, array, trans, GETPC()); } void HELPER(cdsg)(CPUS390XState *env, uint64_t addr, @@ -1275,8 +1279,8 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, get_address(env, 0, b2, d2), 0); return cc; case 0xd00: -cc = helper_trt(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); +cc = do_helper_trt(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); break; default: goto abort; diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 447ba07..fed9f17 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4082,7 +4082,6 @@ static ExitStatus op_tre(DisasContext *s, DisasOps *o) static ExitStatus op_trt(DisasContext *s, DisasOps *o) { TCGv_i32 l = tcg_const_i32(get_field(s->fields, l1)); -potential_page_fault(s); gen_helper_trt(cc_op, cpu_env, l, o->addr1, o->in2); tcg_temp_free_i32(l); set_cc_static(s); -- 2.9.4
[Qemu-devel] [PATCH v2 20/33] target/s390x: Use unwind data for helper_tr
Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 25 +++-- target/s390x/translate.c | 1 - 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 0701e10..b37a963 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -723,19 +723,24 @@ void HELPER(unpk)(CPUS390XState *env, uint32_t len, uint64_t dest, } } -void HELPER(tr)(CPUS390XState *env, uint32_t len, uint64_t array, -uint64_t trans) +static void do_helper_tr(CPUS390XState *env, uint32_t len, uint64_t array, + uint64_t trans, uintptr_t ra) { -int i; +uint32_t i; for (i = 0; i <= len; i++) { -uint8_t byte = cpu_ldub_data(env, array + i); -uint8_t new_byte = cpu_ldub_data(env, trans + byte); - -cpu_stb_data(env, array + i, new_byte); +uint8_t byte = cpu_ldub_data_ra(env, array + i, ra); +uint8_t new_byte = cpu_ldub_data_ra(env, trans + byte, ra); +cpu_stb_data_ra(env, array + i, new_byte, ra); } } +void HELPER(tr)(CPUS390XState *env, uint32_t len, uint64_t array, +uint64_t trans) +{ +return do_helper_tr(env, len, array, trans, GETPC()); +} + uint64_t HELPER(tre)(CPUS390XState *env, uint64_t array, uint64_t len, uint64_t trans) { @@ -1265,9 +1270,9 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, get_address(env, 0, b2, d2), 0); break; case 0xc00: -helper_tr(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2)); -break; +do_helper_tr(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); +return cc; case 0xd00: cc = helper_trt(env, l, get_address(env, 0, b1, d1), get_address(env, 0, b2, d2)); diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 4978f19..1842cc3 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4065,7 +4065,6 @@ static ExitStatus op_tprot(DisasContext *s, DisasOps *o) static ExitStatus op_tr(DisasContext *s, DisasOps *o) { TCGv_i32 l = tcg_const_i32(get_field(s->fields, l1)); -potential_page_fault(s); gen_helper_tr(cpu_env, l, o->addr1, o->in2); tcg_temp_free_i32(l); set_cc_static(s); -- 2.9.4
[Qemu-devel] [PATCH v2 06/33] target/s390x: Use unwind data for helper_mvc
Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 30 ++ target/s390x/translate.c | 1 - 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index b71437a..78a9ac1 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -91,7 +91,7 @@ static void fast_memset(CPUS390XState *env, uint64_t dest, uint8_t byte, } static void fast_memmove(CPUS390XState *env, uint64_t dest, uint64_t src, - uint32_t l) + uint32_t l, uintptr_t ra) { int mmu_idx = cpu_mmu_index(env, false); @@ -110,7 +110,7 @@ static void fast_memmove(CPUS390XState *env, uint64_t dest, uint64_t src, /* We failed to get access to one or both whole pages. The next read or write access will likely fill the QEMU TLB for the next iteration. */ -cpu_stb_data(env, dest, cpu_ldub_data(env, src)); +cpu_stb_data_ra(env, dest, cpu_ldub_data_ra(env, src, ra), ra); src++; dest++; l--; @@ -200,32 +200,38 @@ uint32_t HELPER(oc)(CPUS390XState *env, uint32_t l, uint64_t dest, } /* memmove */ -void HELPER(mvc)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src) +static void do_helper_mvc(CPUS390XState *env, uint32_t l, uint64_t dest, + uint64_t src, uintptr_t ra) { -int i = 0; +uint32_t i; HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n", __func__, l, dest, src); /* mvc with source pointing to the byte after the destination is the same as memset with the first source byte */ -if (dest == (src + 1)) { -fast_memset(env, dest, cpu_ldub_data(env, src), l + 1, 0); +if (dest == src + 1) { +fast_memset(env, dest, cpu_ldub_data_ra(env, src, ra), l + 1, ra); return; } /* mvc and memmove do not behave the same when areas overlap! */ -if ((dest < src) || (src + l < dest)) { -fast_memmove(env, dest, src, l + 1); +if (dest < src || src + l < dest) { +fast_memmove(env, dest, src, l + 1, ra); return; } /* slow version with byte accesses which always work */ for (i = 0; i <= l; i++) { -cpu_stb_data(env, dest + i, cpu_ldub_data(env, src + i)); +cpu_stb_data_ra(env, dest + i, cpu_ldub_data_ra(env, src + i, ra), ra); } } +void HELPER(mvc)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src) +{ +do_helper_mvc(env, l, dest, src, GETPC()); +} + /* compare unsigned byte arrays */ uint32_t HELPER(clc)(CPUS390XState *env, uint32_t l, uint64_t s1, uint64_t s2) { @@ -388,7 +394,7 @@ void HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) { /* XXX missing r0 handling */ env->cc_op = 0; -fast_memmove(env, r1, r2, TARGET_PAGE_SIZE); +fast_memmove(env, r1, r2, TARGET_PAGE_SIZE, 0); } /* string copy (c is string terminator) */ @@ -1223,8 +1229,8 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, d2 = insn2 & 0xfff; switch (insn & 0xf00) { case 0x200: -helper_mvc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2)); +do_helper_mvc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); break; case 0x400: cc = do_helper_nc(env, l, get_address(env, 0, b1, d1), diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 40a4099..729924a 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2866,7 +2866,6 @@ static ExitStatus op_movx(DisasContext *s, DisasOps *o) static ExitStatus op_mvc(DisasContext *s, DisasOps *o) { TCGv_i32 l = tcg_const_i32(get_field(s->fields, l1)); -potential_page_fault(s); gen_helper_mvc(cpu_env, l, o->addr1, o->in2); tcg_temp_free_i32(l); return NO_EXIT; -- 2.9.4
[Qemu-devel] [PATCH v2 13/33] target/s390x: Use unwind data for helper_lam
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 3 ++- target/s390x/translate.c | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index e4bfc6e..88e817a 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -469,10 +469,11 @@ static uint32_t helper_icm(CPUS390XState *env, uint32_t r1, uint64_t address, /* load access registers r1 to r3 from memory at a2 */ void HELPER(lam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { +uintptr_t ra = GETPC(); int i; for (i = r1;; i = (i + 1) % 16) { -env->aregs[i] = cpu_ldl_data(env, a2); +env->aregs[i] = cpu_ldl_data_ra(env, a2, ra); a2 += 4; if (i == r3) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 86e5567..dca2096 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2621,7 +2621,6 @@ static ExitStatus op_lam(DisasContext *s, DisasOps *o) { TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); TCGv_i32 r3 = tcg_const_i32(get_field(s->fields, r3)); -potential_page_fault(s); gen_helper_lam(cpu_env, r1, o->in2, r3); tcg_temp_free_i32(r1); tcg_temp_free_i32(r3); -- 2.9.4
[Qemu-devel] [PATCH v2 19/33] target/s390x: Use unwind data for helper_unpk
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 9 + target/s390x/translate.c | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index d4ee364..0701e10 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -679,6 +679,7 @@ uint64_t HELPER(cksm)(CPUS390XState *env, uint64_t r1, void HELPER(unpk)(CPUS390XState *env, uint32_t len, uint64_t dest, uint64_t src) { +uintptr_t ra = GETPC(); int len_dest = len >> 4; int len_src = len & 0xf; uint8_t b; @@ -688,8 +689,8 @@ void HELPER(unpk)(CPUS390XState *env, uint32_t len, uint64_t dest, src += len_src; /* last byte is special, it only flips the nibbles */ -b = cpu_ldub_data(env, src); -cpu_stb_data(env, dest, (b << 4) | (b >> 4)); +b = cpu_ldub_data_ra(env, src, ra); +cpu_stb_data_ra(env, dest, (b << 4) | (b >> 4), ra); src--; len_src--; @@ -699,7 +700,7 @@ void HELPER(unpk)(CPUS390XState *env, uint32_t len, uint64_t dest, uint8_t cur_byte = 0; if (len_src > 0) { -cur_byte = cpu_ldub_data(env, src); +cur_byte = cpu_ldub_data_ra(env, src, ra); } len_dest--; @@ -718,7 +719,7 @@ void HELPER(unpk)(CPUS390XState *env, uint32_t len, uint64_t dest, /* zone bits */ cur_byte |= 0xf0; -cpu_stb_data(env, dest, cur_byte); +cpu_stb_data_ra(env, dest, cur_byte, ra); } } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 76910bc..4978f19 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4094,7 +4094,6 @@ static ExitStatus op_trt(DisasContext *s, DisasOps *o) static ExitStatus op_unpk(DisasContext *s, DisasOps *o) { TCGv_i32 l = tcg_const_i32(get_field(s->fields, l1)); -potential_page_fault(s); gen_helper_unpk(cpu_env, l, o->addr1, o->in2); tcg_temp_free_i32(l); return NO_EXIT; -- 2.9.4
[Qemu-devel] [PATCH v2 12/33] target/s390x: Use unwind data for helper_mvst
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 5 +++-- target/s390x/translate.c | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 1c36a47..e4bfc6e 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -409,6 +409,7 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) /* string copy (c is string terminator) */ uint64_t HELPER(mvst)(CPUS390XState *env, uint64_t c, uint64_t d, uint64_t s) { +uintptr_t ra = GETPC(); uint32_t len; c = c & 0xff; @@ -418,8 +419,8 @@ uint64_t HELPER(mvst)(CPUS390XState *env, uint64_t c, uint64_t d, uint64_t s) /* Lest we fail to service interrupts in a timely manner, limit the amount of work we're willing to do. For now, let's cap at 8k. */ for (len = 0; len < 0x2000; ++len) { -uint8_t v = cpu_ldub_data(env, s + len); -cpu_stb_data(env, d + len, v); +uint8_t v = cpu_ldub_data_ra(env, s + len, ra); +cpu_stb_data_ra(env, d + len, v, ra); if (v == c) { /* Complete. Set CC=1 and advance R1. */ env->cc_op = 1; diff --git a/target/s390x/translate.c b/target/s390x/translate.c index f55f10a..86e5567 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2923,7 +2923,6 @@ static ExitStatus op_mvpg(DisasContext *s, DisasOps *o) static ExitStatus op_mvst(DisasContext *s, DisasOps *o) { -potential_page_fault(s); gen_helper_mvst(o->in1, cpu_env, regs[0], o->in1, o->in2); set_cc_static(s); return_low128(o->in2); -- 2.9.4
[Qemu-devel] [PATCH v2 05/33] target/s390x: Use unwind data for helper_xc
Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 44 target/s390x/translate.c | 1 - 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index b4b50d1..b71437a 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -57,7 +57,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, #endif /* Reduce the length so that addr + len doesn't cross a page boundary. */ -static inline uint64_t adj_len_to_page(uint64_t len, uint64_t addr) +static inline uint32_t adj_len_to_page(uint32_t len, uint64_t addr) { #ifndef CONFIG_USER_ONLY if ((addr & ~TARGET_PAGE_MASK) + len - 1 >= TARGET_PAGE_SIZE) { @@ -68,7 +68,7 @@ static inline uint64_t adj_len_to_page(uint64_t len, uint64_t addr) } static void fast_memset(CPUS390XState *env, uint64_t dest, uint8_t byte, -uint32_t l) +uint32_t l, uintptr_t ra) { int mmu_idx = cpu_mmu_index(env, false); @@ -76,14 +76,14 @@ static void fast_memset(CPUS390XState *env, uint64_t dest, uint8_t byte, void *p = tlb_vaddr_to_host(env, dest, MMU_DATA_STORE, mmu_idx); if (p) { /* Access to the whole page in write mode granted. */ -int l_adj = adj_len_to_page(l, dest); +uint32_t l_adj = adj_len_to_page(l, dest); memset(p, byte, l_adj); dest += l_adj; l -= l_adj; } else { /* We failed to get access to the whole page. The next write access will likely fill the QEMU TLB for the next iteration. */ -cpu_stb_data(env, dest, byte); +cpu_stb_data_ra(env, dest, byte, ra); dest++; l--; } @@ -100,7 +100,7 @@ static void fast_memmove(CPUS390XState *env, uint64_t dest, uint64_t src, void *dest_p = tlb_vaddr_to_host(env, dest, MMU_DATA_STORE, mmu_idx); if (src_p && dest_p) { /* Access to both whole pages granted. */ -int l_adj = adj_len_to_page(l, src); +uint32_t l_adj = adj_len_to_page(l, src); l_adj = adj_len_to_page(l_adj, dest); memmove(dest_p, src_p, l_adj); src += l_adj; @@ -144,30 +144,34 @@ uint32_t HELPER(nc)(CPUS390XState *env, uint32_t l, uint64_t dest, } /* xor on array */ -uint32_t HELPER(xc)(CPUS390XState *env, uint32_t l, uint64_t dest, -uint64_t src) +static uint32_t do_helper_xc(CPUS390XState *env, uint32_t l, uint64_t dest, + uint64_t src, uintptr_t ra) { -int i; -unsigned char x; -uint32_t cc = 0; +uint32_t i; +uint8_t c = 0; HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n", __func__, l, dest, src); /* xor with itself is the same as memset(0) */ if (src == dest) { -fast_memset(env, dest, 0, l + 1); +fast_memset(env, dest, 0, l + 1, ra); return 0; } for (i = 0; i <= l; i++) { -x = cpu_ldub_data(env, dest + i) ^ cpu_ldub_data(env, src + i); -if (x) { -cc = 1; -} -cpu_stb_data(env, dest + i, x); +uint8_t x = cpu_ldub_data_ra(env, src + i, ra); +x ^= cpu_ldub_data_ra(env, dest + i, ra); +c |= x; +cpu_stb_data_ra(env, dest + i, x, ra); } -return cc; +return c != 0; +} + +uint32_t HELPER(xc)(CPUS390XState *env, uint32_t l, uint64_t dest, +uint64_t src) +{ +return do_helper_xc(env, l, dest, src, GETPC()); } /* or on array */ @@ -206,7 +210,7 @@ void HELPER(mvc)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src) /* mvc with source pointing to the byte after the destination is the same as memset with the first source byte */ if (dest == (src + 1)) { -fast_memset(env, dest, cpu_ldub_data(env, src), l + 1); +fast_memset(env, dest, cpu_ldub_data(env, src), l + 1, 0); return; } @@ -1235,8 +1239,8 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, get_address(env, 0, b2, d2), 0); break; case 0x700: -cc = helper_xc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2)); +cc = do_helper_xc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); break; case 0xc00: helper_tr(env, l, get_address(env, 0, b1, d1), diff --git a/target/s390x/translate.c b/target/s390x/translate.c index db86b70..40a4099 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4160,7 +4160,6 @@ static ExitStatus op_xc(DisasContext *s, DisasOps *o) /* But in general we'll defer to a helper. */ o->in2 = get_address(s, 0, b2,
[Qemu-devel] [PATCH v2 03/33] target/s390x: Use unwind data for helper_nc
Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 31 ++- target/s390x/translate.c | 1 - 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 90b62fa..7d6133b 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -119,23 +119,28 @@ static void fast_memmove(CPUS390XState *env, uint64_t dest, uint64_t src, } /* and on array */ -uint32_t HELPER(nc)(CPUS390XState *env, uint32_t l, uint64_t dest, -uint64_t src) +static uint32_t do_helper_nc(CPUS390XState *env, uint32_t l, uint64_t dest, + uint64_t src, uintptr_t ra) { -int i; -unsigned char x; -uint32_t cc = 0; +uint32_t i; +uint8_t c = 0; HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n", __func__, l, dest, src); + for (i = 0; i <= l; i++) { -x = cpu_ldub_data(env, dest + i) & cpu_ldub_data(env, src + i); -if (x) { -cc = 1; -} -cpu_stb_data(env, dest + i, x); +uint8_t x = cpu_ldub_data_ra(env, src + i, ra); +x &= cpu_ldub_data_ra(env, dest + i, ra); +c |= x; +cpu_stb_data_ra(env, dest + i, x, ra); } -return cc; +return c != 0; +} + +uint32_t HELPER(nc)(CPUS390XState *env, uint32_t l, uint64_t dest, +uint64_t src) +{ +return do_helper_nc(env, l, dest, src, GETPC()); } /* xor on array */ @@ -1213,8 +1218,8 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, get_address(env, 0, b2, d2)); break; case 0x400: -cc = helper_nc(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); +cc = do_helper_nc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); break; case 0x500: cc = helper_clc(env, l, get_address(env, 0, b1, d1), diff --git a/target/s390x/translate.c b/target/s390x/translate.c index d6736e4..7e4cc6c 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3043,7 +3043,6 @@ static ExitStatus op_nabsf128(DisasContext *s, DisasOps *o) static ExitStatus op_nc(DisasContext *s, DisasOps *o) { TCGv_i32 l = tcg_const_i32(get_field(s->fields, l1)); -potential_page_fault(s); gen_helper_nc(cc_op, cpu_env, l, o->addr1, o->in2); tcg_temp_free_i32(l); set_cc_static(s); -- 2.9.4
[Qemu-devel] [PATCH v2 11/33] target/s390x: Use unwind data for helper_mvpg
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/helper.h | 2 +- target/s390x/mem_helper.c | 9 + target/s390x/translate.c | 3 +-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 1fae191..ea35834 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -12,7 +12,7 @@ DEF_HELPER_FLAGS_3(divs64, TCG_CALL_NO_WG, s64, env, s64, s64) DEF_HELPER_FLAGS_4(divu64, TCG_CALL_NO_WG, i64, env, i64, i64, i64) DEF_HELPER_4(srst, i64, env, i64, i64, i64) DEF_HELPER_4(clst, i64, env, i64, i64, i64) -DEF_HELPER_4(mvpg, void, env, i64, i64, i64) +DEF_HELPER_FLAGS_4(mvpg, TCG_CALL_NO_WG, i32, env, i64, i64, i64) DEF_HELPER_4(mvst, i64, env, i64, i64, i64) DEF_HELPER_5(ex, i32, env, i32, i64, i64, i64) DEF_HELPER_FLAGS_4(stam, TCG_CALL_NO_WG, void, env, i32, i64, i32) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index af2801e..1c36a47 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -398,11 +398,12 @@ uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c, uint64_t s1, uint64_t s2) } /* move page */ -void HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) +uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) { -/* XXX missing r0 handling */ -env->cc_op = 0; -fast_memmove(env, r1, r2, TARGET_PAGE_SIZE, 0); +/* ??? missing r0 handling, which includes access keys, but more + importantly optional suppression of the exception! */ +fast_memmove(env, r1, r2, TARGET_PAGE_SIZE, GETPC()); +return 0; /* data moved */ } /* string copy (c is string terminator) */ diff --git a/target/s390x/translate.c b/target/s390x/translate.c index a24e288..f55f10a 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2916,8 +2916,7 @@ static ExitStatus op_mvcs(DisasContext *s, DisasOps *o) static ExitStatus op_mvpg(DisasContext *s, DisasOps *o) { -potential_page_fault(s); -gen_helper_mvpg(cpu_env, regs[0], o->in1, o->in2); +gen_helper_mvpg(cc_op, cpu_env, regs[0], o->in1, o->in2); set_cc_static(s); return NO_EXIT; } -- 2.9.4
[Qemu-devel] [PATCH v2 01/33] target/s390x: Use cpu_loop_exit_restore for tlb_fill
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 0c6a0d9..e3325a4 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -41,15 +41,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) { -int ret; - -ret = s390_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx); +int ret = s390_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx); if (unlikely(ret != 0)) { -if (likely(retaddr)) { -/* now we have a real cpu fault */ -cpu_restore_state(cs, retaddr); -} -cpu_loop_exit(cs); +cpu_loop_exit_restore(cs, retaddr); } } -- 2.9.4
[Qemu-devel] [PATCH v2 09/33] target/s390x: Use unwind data for helper_srst
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 3 ++- target/s390x/translate.c | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 3e75cae..33d83e5 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -327,6 +327,7 @@ static inline uint64_t get_address_31fix(CPUS390XState *env, int reg) uint64_t HELPER(srst)(CPUS390XState *env, uint64_t r0, uint64_t end, uint64_t str) { +uintptr_t ra = GETPC(); uint32_t len; uint8_t v, c = r0; @@ -344,7 +345,7 @@ uint64_t HELPER(srst)(CPUS390XState *env, uint64_t r0, uint64_t end, env->cc_op = 2; return end; } -v = cpu_ldub_data(env, str + len); +v = cpu_ldub_data_ra(env, str + len, ra); if (v == c) { /* Character found. Set R1 to the location; R2 is unmodified. */ env->cc_op = 1; diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 34ccc22..cd33c51 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3972,7 +3972,6 @@ static ExitStatus op_stmh(DisasContext *s, DisasOps *o) static ExitStatus op_srst(DisasContext *s, DisasOps *o) { -potential_page_fault(s); gen_helper_srst(o->in1, cpu_env, regs[0], o->in1, o->in2); set_cc_static(s); return_low128(o->in2); -- 2.9.4
[Qemu-devel] [PATCH v2 04/33] target/s390x: Use unwind data for helper_oc
Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 31 ++- target/s390x/translate.c | 1 - 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 7d6133b..b4b50d1 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -171,23 +171,28 @@ uint32_t HELPER(xc)(CPUS390XState *env, uint32_t l, uint64_t dest, } /* or on array */ -uint32_t HELPER(oc)(CPUS390XState *env, uint32_t l, uint64_t dest, -uint64_t src) +static uint32_t do_helper_oc(CPUS390XState *env, uint32_t l, uint64_t dest, + uint64_t src, uintptr_t ra) { -int i; -unsigned char x; -uint32_t cc = 0; +uint32_t i; +uint8_t c = 0; HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n", __func__, l, dest, src); + for (i = 0; i <= l; i++) { -x = cpu_ldub_data(env, dest + i) | cpu_ldub_data(env, src + i); -if (x) { -cc = 1; -} -cpu_stb_data(env, dest + i, x); +uint8_t x = cpu_ldub_data_ra(env, src + i, ra); +x |= cpu_ldub_data_ra(env, dest + i, ra); +c |= x; +cpu_stb_data_ra(env, dest + i, x, ra); } -return cc; +return c != 0; +} + +uint32_t HELPER(oc)(CPUS390XState *env, uint32_t l, uint64_t dest, +uint64_t src) +{ +return do_helper_oc(env, l, dest, src, GETPC()); } /* memmove */ @@ -1226,8 +1231,8 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, get_address(env, 0, b2, d2)); break; case 0x600: -cc = helper_oc(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); +cc = do_helper_oc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); break; case 0x700: cc = helper_xc(env, l, get_address(env, 0, b1, d1), diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 7e4cc6c..db86b70 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3077,7 +3077,6 @@ static ExitStatus op_negf128(DisasContext *s, DisasOps *o) static ExitStatus op_oc(DisasContext *s, DisasOps *o) { TCGv_i32 l = tcg_const_i32(get_field(s->fields, l1)); -potential_page_fault(s); gen_helper_oc(cc_op, cpu_env, l, o->addr1, o->in2); tcg_temp_free_i32(l); set_cc_static(s); -- 2.9.4
[Qemu-devel] [PATCH v2 08/33] target/s390x: Use unwind data for helper_clm
Reviewed-by: Thomas HuthReviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- target/s390x/mem_helper.c | 11 ++- target/s390x/translate.c | 1 - 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 50689bb..3e75cae 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -268,16 +268,16 @@ uint32_t HELPER(clc)(CPUS390XState *env, uint32_t l, uint64_t s1, uint64_t s2) uint32_t HELPER(clm)(CPUS390XState *env, uint32_t r1, uint32_t mask, uint64_t addr) { -uint8_t r, d; -uint32_t cc; +uintptr_t ra = GETPC(); +uint32_t cc = 0; HELPER_LOG("%s: r1 0x%x mask 0x%x addr 0x%" PRIx64 "\n", __func__, r1, mask, addr); -cc = 0; + while (mask) { if (mask & 8) { -d = cpu_ldub_data(env, addr); -r = (r1 & 0xff00UL) >> 24; +uint8_t d = cpu_ldub_data_ra(env, addr, ra); +uint8_t r = extract32(r1, 24, 8); HELPER_LOG("mask 0x%x %02x/%02x (0x%" PRIx64 ") ", mask, r, d, addr); if (r < d) { @@ -292,6 +292,7 @@ uint32_t HELPER(clm)(CPUS390XState *env, uint32_t r1, uint32_t mask, mask = (mask << 1) & 0xf; r1 <<= 8; } + HELPER_LOG("\n"); return cc; } diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 0f9148a..34ccc22 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1928,7 +1928,6 @@ static ExitStatus op_clm(DisasContext *s, DisasOps *o) TCGv_i32 m3 = tcg_const_i32(get_field(s->fields, m3)); TCGv_i32 t1 = tcg_temp_new_i32(); tcg_gen_extrl_i64_i32(t1, o->in1); -potential_page_fault(s); gen_helper_clm(cc_op, cpu_env, t1, m3, o->in2); set_cc_static(s); tcg_temp_free_i32(t1); -- 2.9.4
[Qemu-devel] [PATCH v2 07/33] target/s390x: Use unwind data for helper_clc
Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 29 + target/s390x/translate.c | 1 - 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 78a9ac1..50689bb 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -233,32 +233,37 @@ void HELPER(mvc)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src) } /* compare unsigned byte arrays */ -uint32_t HELPER(clc)(CPUS390XState *env, uint32_t l, uint64_t s1, uint64_t s2) +static uint32_t do_helper_clc(CPUS390XState *env, uint32_t l, uint64_t s1, + uint64_t s2, uintptr_t ra) { -int i; -unsigned char x, y; -uint32_t cc; +uint32_t i; +uint32_t cc = 0; HELPER_LOG("%s l %d s1 %" PRIx64 " s2 %" PRIx64 "\n", __func__, l, s1, s2); + for (i = 0; i <= l; i++) { -x = cpu_ldub_data(env, s1 + i); -y = cpu_ldub_data(env, s2 + i); +uint8_t x = cpu_ldub_data_ra(env, s1 + i, ra); +uint8_t y = cpu_ldub_data_ra(env, s2 + i, ra); HELPER_LOG("%02x (%c)/%02x (%c) ", x, x, y, y); if (x < y) { cc = 1; -goto done; +break; } else if (x > y) { cc = 2; -goto done; +break; } } -cc = 0; - done: + HELPER_LOG("\n"); return cc; } +uint32_t HELPER(clc)(CPUS390XState *env, uint32_t l, uint64_t s1, uint64_t s2) +{ +return do_helper_clc(env, l, s1, s2, GETPC()); +} + /* compare logical under mask */ uint32_t HELPER(clm)(CPUS390XState *env, uint32_t r1, uint32_t mask, uint64_t addr) @@ -1237,8 +1242,8 @@ uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, get_address(env, 0, b2, d2), 0); break; case 0x500: -cc = helper_clc(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); +cc = do_helper_clc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2), 0); break; case 0x600: cc = do_helper_oc(env, l, get_address(env, 0, b1, d1), diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 729924a..0f9148a 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1901,7 +1901,6 @@ static ExitStatus op_clc(DisasContext *s, DisasOps *o) tcg_gen_qemu_ld64(cc_dst, o->in2, get_mem_index(s)); break; default: -potential_page_fault(s); vl = tcg_const_i32(l); gen_helper_clc(cc_op, cpu_env, vl, o->addr1, o->in2); tcg_temp_free_i32(vl); -- 2.9.4
[Qemu-devel] [PATCH v2 00/33] target/s390x unwind patches
Changes from v1: * Drop major implementation change to EXECUTE; I'll pick that up later. * But there are some implementation errors for EXECUTE, which are not exhibited in real-life code. Mostly because those edge cases are not really useful. * Incorporate feedback from Aurelien. r~ Richard Henderson (33): target/s390x: Use cpu_loop_exit_restore for tlb_fill target/s390x: Move helper_ex to end of file target/s390x: Use unwind data for helper_nc target/s390x: Use unwind data for helper_oc target/s390x: Use unwind data for helper_xc target/s390x: Use unwind data for helper_mvc target/s390x: Use unwind data for helper_clc target/s390x: Use unwind data for helper_clm target/s390x: Use unwind data for helper_srst target/s390x: Use unwind data for helper_clst target/s390x: Use unwind data for helper_mvpg target/s390x: Use unwind data for helper_mvst target/s390x: Use unwind data for helper_lam target/s390x: Use unwind data for helper_stam target/s390x: Use unwind data for helper_mvcl target/s390x: Use unwind data for helper_mvcle target/s390x: Use unwind data for helper_clcle target/s390x: Use unwind data for helper_cksm target/s390x: Use unwind data for helper_unpk target/s390x: Use unwind data for helper_tr target/s390x: Use unwind data for helper_tre target/s390x: Use unwind data for helper_trt target/s390x: Use unwind data for helper_lctlg target/s390x: Use unwind data for helper_lctl target/s390x: Use unwind data for helper_stctl target/s390x: Use unwind data for helper_testblock target/s390x: Use unwind data for helper_tprot target/s390x: Use unwind data for helper_lra target/s390x: Use unwind data for helper_mvcs/mvcp target/s390x: Fix some helper_ex problems target/s390x: Fix EXECUTE with R1==0 target/s390x: Use atomic operations for COMPARE SWAP PURGE target/s390x: Implement CSPG target/s390x/helper.h | 6 +- target/s390x/insn-data.def | 7 +- target/s390x/mem_helper.c | 537 - target/s390x/translate.c | 94 4 files changed, 354 insertions(+), 290 deletions(-) -- 2.9.4
[Qemu-devel] [PATCH v2 02/33] target/s390x: Move helper_ex to end of file
This will avoid needing forward declarations in following patches. Signed-off-by: Richard Henderson--- target/s390x/mem_helper.c | 161 +++--- 1 file changed, 81 insertions(+), 80 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index e3325a4..90b62fa 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -436,86 +436,6 @@ static uint32_t helper_icm(CPUS390XState *env, uint32_t r1, uint64_t address, return cc; } -/* execute instruction - this instruction executes an insn modified with the contents of r1 - it does not change the executed instruction in memory - it does not change the program counter - in other words: tricky... - currently implemented by interpreting the cases it is most commonly used in -*/ -uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, -uint64_t addr, uint64_t ret) -{ -S390CPU *cpu = s390_env_get_cpu(env); -uint16_t insn = cpu_lduw_code(env, addr); - -HELPER_LOG("%s: v1 0x%lx addr 0x%lx insn 0x%x\n", __func__, v1, addr, - insn); -if ((insn & 0xf0ff) == 0xd000) { -uint32_t l, insn2, b1, b2, d1, d2; - -l = v1 & 0xff; -insn2 = cpu_ldl_code(env, addr + 2); -b1 = (insn2 >> 28) & 0xf; -b2 = (insn2 >> 12) & 0xf; -d1 = (insn2 >> 16) & 0xfff; -d2 = insn2 & 0xfff; -switch (insn & 0xf00) { -case 0x200: -helper_mvc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2)); -break; -case 0x400: -cc = helper_nc(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); -break; -case 0x500: -cc = helper_clc(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); -break; -case 0x600: -cc = helper_oc(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); -break; -case 0x700: -cc = helper_xc(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2)); -break; -case 0xc00: -helper_tr(env, l, get_address(env, 0, b1, d1), - get_address(env, 0, b2, d2)); -break; -case 0xd00: -cc = helper_trt(env, l, get_address(env, 0, b1, d1), -get_address(env, 0, b2, d2)); -break; -default: -goto abort; -} -} else if ((insn & 0xff00) == 0x0a00) { -/* supervisor call */ -HELPER_LOG("%s: svc %ld via execute\n", __func__, (insn | v1) & 0xff); -env->psw.addr = ret - 4; -env->int_svc_code = (insn | v1) & 0xff; -env->int_svc_ilen = 4; -helper_exception(env, EXCP_SVC); -} else if ((insn & 0xff00) == 0xbf00) { -uint32_t insn2, r1, r3, b2, d2; - -insn2 = cpu_ldl_code(env, addr + 2); -r1 = (insn2 >> 20) & 0xf; -r3 = (insn2 >> 16) & 0xf; -b2 = (insn2 >> 12) & 0xf; -d2 = insn2 & 0xfff; -cc = helper_icm(env, r1, get_address(env, 0, b2, d2), r3); -} else { -abort: -cpu_abort(CPU(cpu), "EXECUTE on instruction prefix 0x%x not implemented\n", - insn); -} -return cc; -} - /* load access registers r1 to r3 from memory at a2 */ void HELPER(lam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) { @@ -1262,3 +1182,84 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) return ret; } #endif + +/* execute instruction + this instruction executes an insn modified with the contents of r1 + it does not change the executed instruction in memory + it does not change the program counter + in other words: tricky... + currently implemented by interpreting the cases it is most commonly used. +*/ +uint32_t HELPER(ex)(CPUS390XState *env, uint32_t cc, uint64_t v1, +uint64_t addr, uint64_t ret) +{ +S390CPU *cpu = s390_env_get_cpu(env); +uint16_t insn = cpu_lduw_code(env, addr); + +HELPER_LOG("%s: v1 0x%lx addr 0x%lx insn 0x%x\n", __func__, v1, addr, + insn); +if ((insn & 0xf0ff) == 0xd000) { +uint32_t l, insn2, b1, b2, d1, d2; + +l = v1 & 0xff; +insn2 = cpu_ldl_code(env, addr + 2); +b1 = (insn2 >> 28) & 0xf; +b2 = (insn2 >> 12) & 0xf; +d1 = (insn2 >> 16) & 0xfff; +d2 = insn2 & 0xfff; +switch (insn & 0xf00) { +case 0x200: +helper_mvc(env, l, get_address(env, 0, b1, d1), + get_address(env, 0, b2, d2)); +break; +case 0x400: +cc = helper_nc(env, l, get_address(env, 0, b1, d1), +get_address(env, 0, b2, d2)); +break; +
Re: [Qemu-devel] [PATCH 7/7] qcow2: Merge the writing of the COW regions with the guest data
On Wed 24 May 2017 06:43:31 PM CEST, Anton Nefedov wrote: >> +if (m->data_qiov) { >> +qemu_iovec_reset(); >> +qemu_iovec_add(, start_buffer, start->nb_bytes); >> +qemu_iovec_concat(, m->data_qiov, 0, data_bytes); >> +qemu_iovec_add(, end_buffer, end->nb_bytes); > > Can it be a problem if (m->data_qiov->niov == IOV_MAX)? > We had to add merge-iovecs code for the case (maybe there's better > solution?) You're right, good catch! I'll add a check for that. To be honest I don't think that's likely to happen in practice, so if it does we can simply fall back to the old behavior (separate writes). > Also, will this work if allocation is split into several l2metas? > e.g. one has cow_start.nb_bytes and another has cow_end.nb_bytes The guest request will be merged with the first l2meta that has to copy at least one of the two regions. It doesn't matter if the other one has nb_bytes == 0. Berto
Re: [Qemu-devel] [PATCH 00/12] self-announce updates
Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH 00/12] self-announce updates Message-id: 1495649128-10529-1-git-send-email-vyase...@redhat.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 3f42430 tests: Add a test for qemu self announcments dbfc50a tests/test-hmp: Add announce parameter tests 913d3ab hmp: Add hmp_announce_self a668d43 hmp: add announce paraters info/set 9e8f35b announce_timer: Add ability to reset an existing 0c23820 migration: Allow for a limited number of announce timers 1c59722 qmp: Expose qemu_announce_self as a qmp command 6662e88 virtio-net: Allow qemu_announce_self to trigger virtio announcements 220cde3 net: Add a network device specific self-announcement ability 10b866e migration: Switch to using announcement timer b7e78e9 migration: Introduce announcement timer df9e8b3 migration: Introduce announce parameters === OUTPUT BEGIN === Checking PATCH 1/12: migration: Introduce announce parameters... ERROR: braces {} are necessary for all arms of this statement #133: FILE: migration/savevm.c:174: +if (!qemu_validate_announce_parameters(params, errp)) [...] ERROR: trailing whitespace #175: FILE: qapi-schema.json:597: +# $ total: 2 errors, 0 warnings, 207 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 2/12: migration: Introduce announcement timer... Checking PATCH 3/12: migration: Switch to using announcement timer... Checking PATCH 4/12: net: Add a network device specific self-announcement ability... Checking PATCH 5/12: virtio-net: Allow qemu_announce_self to trigger virtio announcements... ERROR: braces {} are necessary for all arms of this statement #49: FILE: hw/net/virtio-net.c:139: +if (n->announce_timer->round) [...] total: 1 errors, 0 warnings, 55 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 6/12: qmp: Expose qemu_announce_self as a qmp command... ERROR: braces {} are necessary for all arms of this statement #29: FILE: migration/savevm.c:277: +if (has_params) [...] total: 1 errors, 0 warnings, 45 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 7/12: migration: Allow for a limited number of announce timers... Checking PATCH 8/12: announce_timer: Add ability to reset an existing... ERROR: space prohibited before that close parenthesis ')' #50: FILE: migration/savevm.c:239: +if (--timer->round ) { total: 1 errors, 0 warnings, 81 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 9/12: hmp: add announce paraters info/set... Checking PATCH 10/12: hmp: Add hmp_announce_self... Checking PATCH 11/12: tests/test-hmp: Add announce parameter tests... Checking PATCH 12/12: tests: Add a test for qemu self announcments... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH 00/12] self-announce updates
Hi, This series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Subject: [Qemu-devel] [PATCH 00/12] self-announce updates Message-id: 1495649128-10529-1-git-send-email-vyase...@redhat.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-quick@centos6 time make docker-test-mingw@fedora time make docker-test-build@min-glib === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 3f42430 tests: Add a test for qemu self announcments dbfc50a tests/test-hmp: Add announce parameter tests 913d3ab hmp: Add hmp_announce_self a668d43 hmp: add announce paraters info/set 9e8f35b announce_timer: Add ability to reset an existing 0c23820 migration: Allow for a limited number of announce timers 1c59722 qmp: Expose qemu_announce_self as a qmp command 6662e88 virtio-net: Allow qemu_announce_self to trigger virtio announcements 220cde3 net: Add a network device specific self-announcement ability 10b866e migration: Switch to using announcement timer b7e78e9 migration: Introduce announcement timer df9e8b3 migration: Introduce announce parameters === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-l1r65ell/src/dtc'... Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d' BUILD centos6 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-l1r65ell/src' ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPYRUNNER RUN test-quick in qemu:centos6 Packages installed: SDL-devel-1.2.14-7.el6_7.1.x86_64 ccache-3.1.6-2.el6.x86_64 epel-release-6-8.noarch gcc-4.4.7-17.el6.x86_64 git-1.7.1-4.el6_7.1.x86_64 glib2-devel-2.28.8-5.el6.x86_64 libfdt-devel-1.4.0-1.el6.x86_64 make-3.81-23.el6.x86_64 package g++ is not installed pixman-devel-0.32.8-1.el6.x86_64 tar-1.23-15.el6_8.x86_64 zlib-devel-1.2.3-29.el6.x86_64 Environment variables: PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel glib2-devel SDL-devel pixman-devel epel-release HOSTNAME=a36404b38874 TERM=xterm MAKEFLAGS= -j8 HISTSIZE=1000 J=8 USER=root CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 MAIL=/var/spool/mail/root PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ LANG=en_US.UTF-8 TARGET_LIST= HISTCONTROL=ignoredups SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test LOGNAME=root LESSOPEN=||/usr/bin/lesspipe.sh %s FEATURES= dtc DEBUG= G_BROKEN_FILENAMES=1 CCACHE_HASHDIR= _=/usr/bin/env Configure options: --enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/var/tmp/qemu-build/install No C++ compiler available; disabling C++ specific optional code Install prefix/var/tmp/qemu-build/install BIOS directory/var/tmp/qemu-build/install/share/qemu binary directory /var/tmp/qemu-build/install/bin library directory /var/tmp/qemu-build/install/lib module directory /var/tmp/qemu-build/install/lib/qemu libexec directory /var/tmp/qemu-build/install/libexec include directory /var/tmp/qemu-build/install/include config directory /var/tmp/qemu-build/install/etc local state directory /var/tmp/qemu-build/install/var Manual directory /var/tmp/qemu-build/install/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /tmp/qemu-test/src C compilercc Host C compiler cc C++ compiler Objective-C compiler cc ARFLAGS rv CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g QEMU_CFLAGS -I/usr/include/pixman-1 -I$(SRC_PATH)/dtc/libfdt -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g make make install install pythonpython -B smbd /usr/sbin/smbd module supportno host CPU x86_64 host big endian no target list x86_64-softmmu aarch64-softmmu tcg debug enabled no gprof enabled no sparse enabledno strip binariesyes profiler no static build no pixmansystem SDL support yes (1.2.14) GTK support no GTK GL supportno VTE support no TLS priority NORMAL GNUTLS supportno GNUTLS rndno libgcrypt no libgcrypt kdf no nettleno nettle kdfno libtasn1 no
[Qemu-devel] [PATCH] [SLIRP] Fix total IP header length in forwarded TCP packets
When forwarding TCP packets, the internal tcpiphdr struct length was wrongly used inside the IP header. This commit changes the behaviour to what is used by tcp_output.c, using the correct full IP header + payload length. Signed-off-by: Sjors Gielen--- slirp/tcp_subr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c index ed16e1807f..3d6193657c 100644 --- a/slirp/tcp_subr.c +++ b/slirp/tcp_subr.c @@ -204,7 +204,7 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m, m->m_len -= sizeof(struct tcpiphdr) - sizeof(struct tcphdr) - sizeof(struct ip); ip = mtod(m, struct ip *); -ip->ip_len = tlen; +ip->ip_len = m->m_len; ip->ip_dst = tcpiph_save.ti_dst; ip->ip_src = tcpiph_save.ti_src; ip->ip_p = tcpiph_save.ti_pr; @@ -224,7 +224,7 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m, m->m_len -= sizeof(struct tcpiphdr) - sizeof(struct tcphdr) - sizeof(struct ip6); ip6 = mtod(m, struct ip6 *); -ip6->ip_pl = tlen; +ip6->ip_pl = m->m_len; ip6->ip_dst = tcpiph_save.ti_dst6; ip6->ip_src = tcpiph_save.ti_src6; ip6->ip_nh = tcpiph_save.ti_nh6; -- 2.13.0
Re: [Qemu-devel] [PATCH 00/12] self-announce updates
Hi, This series failed build test on s390x host. Please find the details below. Type: series Subject: [Qemu-devel] [PATCH 00/12] self-announce updates Message-id: 1495649128-10529-1-git-send-email-vyase...@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch set -e echo "=== ENV ===" env echo "=== PACKAGES ===" rpm -qa echo "=== TEST BEGIN ===" CC=$HOME/bin/cc INSTALL=$PWD/install BUILD=$PWD/build echo -n "Using CC: " realpath $CC mkdir -p $BUILD $INSTALL SRC=$PWD cd $BUILD $SRC/configure --cc=$CC --prefix=$INSTALL make -j4 # XXX: we need reliable clean up # make check -j4 V=1 make install === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1495649128-10529-1-git-send-email-vyase...@redhat.com -> patchew/1495649128-10529-1-git-send-email-vyase...@redhat.com Switched to a new branch 'test' 3f42430 tests: Add a test for qemu self announcments dbfc50a tests/test-hmp: Add announce parameter tests 913d3ab hmp: Add hmp_announce_self a668d43 hmp: add announce paraters info/set 9e8f35b announce_timer: Add ability to reset an existing 0c23820 migration: Allow for a limited number of announce timers 1c59722 qmp: Expose qemu_announce_self as a qmp command 6662e88 virtio-net: Allow qemu_announce_self to trigger virtio announcements 220cde3 net: Add a network device specific self-announcement ability 10b866e migration: Switch to using announcement timer b7e78e9 migration: Introduce announcement timer df9e8b3 migration: Introduce announce parameters === OUTPUT BEGIN === === ENV === XDG_SESSION_ID=65511 SHELL=/bin/sh USER=fam PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug PATH=/usr/bin:/bin PWD=/var/tmp/patchew-tester-tmp-zy6kocel/src LANG=en_US.UTF-8 HOME=/home/fam SHLVL=2 LOGNAME=fam DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus XDG_RUNTIME_DIR=/run/user/1012 _=/usr/bin/env === PACKAGES === gpg-pubkey-873529b8-54e386ff xz-libs-5.2.2-2.fc24.s390x libxshmfence-1.2-3.fc24.s390x giflib-4.1.6-15.fc24.s390x trousers-lib-0.3.13-6.fc24.s390x ncurses-base-6.0-6.20160709.fc25.noarch gmp-6.1.1-1.fc25.s390x libidn-1.33-1.fc25.s390x slang-2.3.0-7.fc25.s390x libsemanage-2.5-8.fc25.s390x pkgconfig-0.29.1-1.fc25.s390x alsa-lib-1.1.1-2.fc25.s390x yum-metadata-parser-1.1.4-17.fc25.s390x python3-slip-dbus-0.6.4-4.fc25.noarch python2-cssselect-0.9.2-1.fc25.noarch python-fedora-0.8.0-2.fc25.noarch createrepo_c-libs-0.10.0-6.fc25.s390x initscripts-9.69-1.fc25.s390x wget-1.18-2.fc25.s390x dhcp-client-4.3.5-1.fc25.s390x parted-3.2-21.fc25.s390x flex-2.6.0-3.fc25.s390x colord-libs-1.3.4-1.fc25.s390x python-osbs-client-0.33-3.fc25.noarch perl-Pod-Simple-3.35-1.fc25.noarch python2-simplejson-3.10.0-1.fc25.s390x brltty-5.4-2.fc25.s390x librados2-10.2.4-2.fc25.s390x tcp_wrappers-7.6-83.fc25.s390x libcephfs_jni1-10.2.4-2.fc25.s390x nettle-devel-3.3-1.fc25.s390x bzip2-devel-1.0.6-21.fc25.s390x libuuid-2.28.2-2.fc25.s390x pango-1.40.4-1.fc25.s390x python3-dnf-1.1.10-6.fc25.noarch cryptsetup-libs-1.7.4-1.fc25.s390x texlive-kpathsea-doc-svn41139-33.fc25.1.noarch netpbm-10.77.00-3.fc25.s390x openssh-7.4p1-4.fc25.s390x texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x texlive-graphics-svn41015-33.fc25.1.noarch texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch texlive-mfware-svn40768-33.fc25.1.noarch texlive-texlive-scripts-svn41433-33.fc25.1.noarch texlive-euro-svn22191.1.1-33.fc25.1.noarch texlive-etex-svn37057.0-33.fc25.1.noarch texlive-iftex-svn29654.0.2-33.fc25.1.noarch texlive-palatino-svn31835.0-33.fc25.1.noarch texlive-texlive-docindex-svn41430-33.fc25.1.noarch texlive-xunicode-svn30466.0.981-33.fc25.1.noarch texlive-koma-script-svn41508-33.fc25.1.noarch texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch texlive-jknapltx-svn19440.0-33.fc25.1.noarch netpbm-progs-10.77.00-3.fc25.s390x texinfo-6.1-4.fc25.s390x openssl-devel-1.0.2k-1.fc25.s390x python2-sssdconfig-1.15.2-1.fc25.noarch gdk-pixbuf2-2.36.6-1.fc25.s390x mesa-libEGL-13.0.4-3.fc25.s390x pcre-cpp-8.40-6.fc25.s390x pcre-utf16-8.40-6.fc25.s390x glusterfs-extra-xlators-3.10.1-1.fc25.s390x mesa-libGL-devel-13.0.4-3.fc25.s390x nss-devel-3.29.3-1.1.fc25.s390x libaio-0.3.110-6.fc24.s390x libfontenc-1.1.3-3.fc24.s390x lzo-2.08-8.fc24.s390x isl-0.14-5.fc24.s390x libXau-1.0.8-6.fc24.s390x linux-atm-libs-2.5.1-14.fc24.s390x libXext-1.3.3-4.fc24.s390x libXxf86vm-1.1.4-3.fc24.s390x bison-3.0.4-4.fc24.s390x perl-srpm-macros-1-20.fc25.noarch gawk-4.1.3-8.fc25.s390x libwayland-client-1.12.0-1.fc25.s390x perl-Exporter-5.72-366.fc25.noarch perl-version-0.99.17-1.fc25.s390x fftw-libs-double-3.3.5-3.fc25.s390x libssh2-1.8.0-1.fc25.s390x ModemManager-glib-1.6.4-1.fc25.s390x newt-python3-0.52.19-2.fc25.s390x python-munch-2.0.4-3.fc25.noarch python-bugzilla-1.2.2-4.fc25.noarch