From 75f19f8c3006970632303b49043b075dc4fe922e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 16 Sep 2016 00:36:58 +0200 Subject: [PATCH 1/6] megasas: do not call pci_dma_unmap after having freed the frame once Commit 8cc4678 ("megasas: remove useless check for cmd->frame", 2016-07-17) was wrong because I trusted Coverity too much. It turns out that there _is_ a path through which cmd->frame can become NULL. After megasas_handle_frame's switch (md->frame->header.frame_cmd), megasas_init_firmware can be called. From there, megasas_reset_frames will call megasas_unmap_frame which resets cmd->frame = NULL. However, there is another bug to fix in there, because megasas_unmap_frame is called again after setting the command status. In this case QEMU should not do anything, instead it calls pci_dma_unmap again. Harmless, but better fix it. Signed-off-by: Paolo Bonzini --- hw/scsi/megasas.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 52a41239cf..ca62952941 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -461,9 +461,12 @@ static void megasas_unmap_frame(MegasasState *s, MegasasCmd *cmd) { PCIDevice *p = PCI_DEVICE(s); - pci_dma_unmap(p, cmd->frame, cmd->pa_size, 0, 0); + if (cmd->pa_size) { + pci_dma_unmap(p, cmd->frame, cmd->pa_size, 0, 0); + } cmd->frame = NULL; cmd->pa = 0; + cmd->pa_size = 0; clear_bit(cmd->index, s->frame_map); } From 9e55d58806feedca9341a63243304fbbeaac741d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 10 Nov 2016 16:27:51 +0100 Subject: [PATCH 2/6] megasas: clean up and fix request completion/cancellation megasas_command_cancel is a callback; it should report the abort in the frame, not try another abort! Compare for instance with mptsas_request_cancelled. So extract the common bits for request completion in a new function megasas_complete_command, call it from both the .complete and .cancel callbacks, and remove duplicate pieces from the DCMD path. Signed-off-by: Paolo Bonzini Message-Id: <20161110152751.4267-2-pbonzini@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Paolo Bonzini --- hw/scsi/megasas.c | 53 +++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index ca62952941..67fc1e7893 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -300,12 +300,6 @@ unmap: return iov_count - i; } -static void megasas_unmap_sgl(MegasasCmd *cmd) -{ - qemu_sglist_destroy(&cmd->qsg); - cmd->iov_offset = 0; -} - /* * passthrough sense and io sense are at the same offset */ @@ -580,6 +574,20 @@ static void megasas_complete_frame(MegasasState *s, uint64_t context) } } +static void megasas_complete_command(MegasasCmd *cmd) +{ + qemu_sglist_destroy(&cmd->qsg); + cmd->iov_size = 0; + cmd->iov_offset = 0; + + cmd->req->hba_private = NULL; + scsi_req_unref(cmd->req); + cmd->req = NULL; + + megasas_unmap_frame(cmd->state, cmd); + megasas_complete_frame(cmd->state, cmd->context); +} + static void megasas_reset_frames(MegasasState *s) { int i; @@ -596,9 +604,9 @@ static void megasas_reset_frames(MegasasState *s) static void megasas_abort_command(MegasasCmd *cmd) { - if (cmd->req) { + /* Never abort internal commands. */ + if (cmd->req != NULL) { scsi_req_cancel(cmd->req); - cmd->req = NULL; } } @@ -689,9 +697,6 @@ static void megasas_finish_dcmd(MegasasCmd *cmd, uint32_t iov_size) { trace_megasas_finish_dcmd(cmd->index, iov_size); - if (cmd->frame->header.sge_count) { - qemu_sglist_destroy(&cmd->qsg); - } if (iov_size > cmd->iov_size) { if (megasas_frame_is_ieee_sgl(cmd)) { cmd->frame->dcmd.sgl.sg_skinny->len = cpu_to_le32(iov_size); @@ -701,7 +706,6 @@ static void megasas_finish_dcmd(MegasasCmd *cmd, uint32_t iov_size) cmd->frame->dcmd.sgl.sg32->len = cpu_to_le32(iov_size); } } - cmd->iov_size = 0; } static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd) @@ -1589,7 +1593,6 @@ static int megasas_finish_internal_dcmd(MegasasCmd *cmd, int lun = req->lun; opcode = le32_to_cpu(cmd->frame->dcmd.opcode); - scsi_req_unref(req); trace_megasas_dcmd_internal_finish(cmd->index, opcode, lun); switch (opcode) { case MFI_DCMD_PD_GET_INFO: @@ -1860,7 +1863,11 @@ static void megasas_command_complete(SCSIRequest *req, uint32_t status, trace_megasas_command_complete(cmd->index, status, resid); - if (cmd->req != req) { + if (req->io_canceled) { + return; + } + + if (cmd->req == NULL) { /* * Internal command complete */ @@ -1879,25 +1886,21 @@ static void megasas_command_complete(SCSIRequest *req, uint32_t status, megasas_copy_sense(cmd); } - megasas_unmap_sgl(cmd); cmd->frame->header.scsi_status = req->status; - scsi_req_unref(cmd->req); - cmd->req = NULL; } cmd->frame->header.cmd_status = cmd_status; - megasas_unmap_frame(cmd->state, cmd); - megasas_complete_frame(cmd->state, cmd->context); + megasas_complete_command(cmd); } -static void megasas_command_cancel(SCSIRequest *req) +static void megasas_command_cancelled(SCSIRequest *req) { MegasasCmd *cmd = req->hba_private; - if (cmd) { - megasas_abort_command(cmd); - } else { - scsi_req_unref(req); + if (!cmd) { + return; } + cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED; + megasas_complete_command(cmd); } static int megasas_handle_abort(MegasasState *s, MegasasCmd *cmd) @@ -2316,7 +2319,7 @@ static const struct SCSIBusInfo megasas_scsi_info = { .transfer_data = megasas_xfer_complete, .get_sg_list = megasas_get_sg_list, .complete = megasas_command_complete, - .cancel = megasas_command_cancel, + .cancel = megasas_command_cancelled, }; static void megasas_scsi_realize(PCIDevice *dev, Error **errp) From 64e184e2608d3c93dda1bba8ae6dc2185b5228fb Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Fri, 25 Nov 2016 10:55:22 +0800 Subject: [PATCH 3/6] pci-assign: sync MSI/MSI-X cap and table with PCIDevice Since commit e1d4fb2d ("kvm-irqchip: x86: add msi route notify fn"), kvm_irqchip_add_msi_route() starts to use pci_get_msi_message() to fetch MSI info. This requires that we setup MSI related fields in PCIDevice. For most devices, that won't be a problem, as long as we are using general interfaces like msi_init()/msix_init(). However, for pci-assign devices, MSI/MSI-X is treated differently - PCI assign devices are maintaining its own MSI table and cap information in AssignedDevice struct. however that's not synced up with PCIDevice's fields. That will leads to pci_get_msi_message() failed to find correct MSI capability, even with an NULL msix_table. A quick fix is to sync up the two places: both the capability bits and table address for MSI/MSI-X. Reported-by: Changlimin Tested-by: Changlimin Cc: qemu-stable@nongnu.org Fixes: e1d4fb2d ("kvm-irqchip: x86: add msi route notify fn") Signed-off-by: Peter Xu Message-Id: <1480042522-16551-1-git-send-email-peterx@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Paolo Bonzini --- hw/i386/kvm/pci-assign.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c index 8238fbc630..87dcbdd51a 100644 --- a/hw/i386/kvm/pci-assign.c +++ b/hw/i386/kvm/pci-assign.c @@ -1251,6 +1251,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp) error_propagate(errp, local_err); return -ENOTSUP; } + dev->dev.cap_present |= QEMU_PCI_CAP_MSI; dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI; /* Only 32-bit/no-mask currently supported */ ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSI, pos, 10, @@ -1285,6 +1286,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp) error_propagate(errp, local_err); return -ENOTSUP; } + dev->dev.cap_present |= QEMU_PCI_CAP_MSIX; dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX; ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSIX, pos, 12, &local_err); @@ -1648,6 +1650,7 @@ static void assigned_dev_register_msix_mmio(AssignedDevice *dev, Error **errp) dev->msix_table = NULL; return; } + dev->dev.msix_table = (uint8_t *)dev->msix_table; assigned_dev_msix_reset(dev); @@ -1665,6 +1668,7 @@ static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev) error_report("error unmapping msix_table! %s", strerror(errno)); } dev->msix_table = NULL; + dev->dev.msix_table = NULL; } static const VMStateDescription vmstate_assigned_device = { From 04e27c6bb034e57e60739362a90bc11a4d6f3ad4 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Mon, 28 Nov 2016 13:32:00 +0000 Subject: [PATCH 4/6] migration/pcspk: Add a property to state if pcspk is migrated Allow us to turn migration of pcspk off for compatibility. Signed-off-by: Dr. David Alan Gilbert Message-Id: <20161128133201.16104-2-dgilbert@redhat.com> Signed-off-by: Paolo Bonzini --- hw/audio/pcspk.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c index 984534b2d1..798002277b 100644 --- a/hw/audio/pcspk.c +++ b/hw/audio/pcspk.c @@ -54,6 +54,7 @@ typedef struct { unsigned int play_pos; uint8_t data_on; uint8_t dummy_refresh_clock; + bool migrate; } PCSpkState; static const char *s_spk = "pcspk"; @@ -187,11 +188,19 @@ static void pcspk_realizefn(DeviceState *dev, Error **errp) pcspk_state = s; } +static bool migrate_needed(void *opaque) +{ + PCSpkState *s = opaque; + + return s->migrate; +} + static const VMStateDescription vmstate_spk = { .name = "pcspk", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, + .needed = migrate_needed, .fields = (VMStateField[]) { VMSTATE_UINT8(data_on, PCSpkState), VMSTATE_UINT8(dummy_refresh_clock, PCSpkState), @@ -201,6 +210,7 @@ static const VMStateDescription vmstate_spk = { static Property pcspk_properties[] = { DEFINE_PROP_UINT32("iobase", PCSpkState, iobase, -1), + DEFINE_PROP_BOOL("migrate", PCSpkState, migrate, true), DEFINE_PROP_END_OF_LIST(), }; From f9f885b78a6d9d1f0619d250f8d518313a146885 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Mon, 28 Nov 2016 13:32:01 +0000 Subject: [PATCH 5/6] migration/pcspk: Turn migration of pcspk off for 2.7 and older To keep backwards migration compatibility allow us to turn pcspk migration off. Signed-off-by: Dr. David Alan Gilbert Message-Id: <20161128133201.16104-3-dgilbert@redhat.com> Signed-off-by: Paolo Bonzini --- include/hw/i386/pc.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 67a1a9e786..4b74130559 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -395,6 +395,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); .driver = "Opteron_G3" "-" TYPE_X86_CPU,\ .property = "stepping",\ .value = "1",\ + },\ + {\ + .driver = "isa-pcspk",\ + .property = "migrate",\ + .value = "off",\ }, #define PC_COMPAT_2_6 \ From c96f0ee6a67ca6277366e78ce5d84d5c20dd596f Mon Sep 17 00:00:00 2001 From: Adrian Bunk Date: Sun, 27 Nov 2016 18:28:17 +0200 Subject: [PATCH 6/6] rules.mak: Use -r instead of -Wl, -r to fix building when PIE is default Building qemu fails in distributions where gcc enables PIE by default (e.g. Debian unstable) with: /usr/bin/ld: -r and -pie may not be used together Use -r instead of -Wl,-r to avoid gcc passing -pie to the linker when PIE is enabled and a relocatable object is passed. Signed-off-by: Adrian Bunk Message-Id: <20161127162817.15144-1-bunk@stusta.de> Cc: qemu-stable@nongnu.org Signed-off-by: Paolo Bonzini --- rules.mak | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules.mak b/rules.mak index 0333ae3c95..545ebd9c68 100644 --- a/rules.mak +++ b/rules.mak @@ -93,7 +93,7 @@ module-common.o: CFLAGS += $(DSO_OBJ_CFLAGS) $(if $(findstring /,$@),$(call quiet-command,cp $@ $(subst /,-,$@),"CP","$(subst /,-,$@)")) -LD_REL := $(CC) -nostdlib -Wl,-r $(LD_REL_FLAGS) +LD_REL := $(CC) -nostdlib -r $(LD_REL_FLAGS) %.mo: $(call quiet-command,$(LD_REL) -o $@ $^,"LD","$(TARGET_DIR)$@")