On Fri, Sep 30, 2016 at 4:55 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > > > On 20/09/2016 17:42, David Kiarie wrote: >> Hi all, >> >> This patchset adds basic AMD IOMMU emulation support to Qemu. >> >> Resent this with some changes suggested by Michael. > > > Hi David,
Hi Paolo, > > please fix the problems below, that were reported by Coverity. Thanks, will do ASAP. > > Paolo > > > > > > > ** CID 1363372: Uninitialized variables (UNINIT) > /hw/i386/amd_iommu.c: 147 in amdvi_generate_msi_interrupt() > ________________________________________________________________________________________________________ > 141 amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val); > 142 } > 143 > 144 static void amdvi_generate_msi_interrupt(AMDVIState *s) > 145 { > 146 MSIMessage msg; >>>> CID 1363372: Uninitialized variables (UNINIT) >>>> Declaring variable "attrs" without initializer. > 147 MemTxAttrs attrs; > 148 > 149 attrs.requester_id = pci_requester_id(&s->pci.dev); > 150 > 151 if (msi_enabled(&s->pci.dev)) { > 152 msg = msi_get_message(&s->pci.dev, 0); > > > > > > > ** CID 1363371: Code maintainability issues (SIZEOF_MISMATCH) > /hw/i386/amd_iommu.c: 337 in amdvi_update_iotlb() > ________________________________________________________________________________________________________ > 331 > 332 static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid, > 333 uint64_t gpa, IOMMUTLBEntry to_cache, > 334 uint16_t domid) > 335 { > 336 AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry)); >>>> CID 1363371: Code maintainability issues (SIZEOF_MISMATCH) >>>> Passing argument "8UL /* sizeof (key) */" to function "g_malloc" and >>>> then >>>> casting the return value to "uint64_t *" is suspicious. In this >>>> particular case >>>> "sizeof (uint64_t *)" happens to be equal to "sizeof (uint64_t)", but >>>> this is >>>> not a portable assumption. > 337 uint64_t *key = g_malloc(sizeof(key)); > 338 uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K; > 339 > 340 /* don't cache erroneous translations */ > 341 if (to_cache.perm != IOMMU_NONE) { > 342 trace_amdvi_cache_update(domid, PCI_BUS_NUM(devid), > PCI_SLOT(devid), > > > > > > ** CID 1363370: Resource leaks (RESOURCE_LEAK) > /hw/i386/amd_iommu.c: 356 in amdvi_update_iotlb() > ________________________________________________________________________________________________________ > 350 entry->perms = to_cache.perm; > 351 entry->translated_addr = to_cache.translated_addr; > 352 entry->page_mask = to_cache.addr_mask; > 353 *key = gfn | ((uint64_t)(devid) << AMDVI_DEVID_SHIFT); > 354 g_hash_table_replace(s->iotlb, key, entry); > 355 } >>>> CID 1363370: Resource leaks (RESOURCE_LEAK) >>>> Variable "entry" going out of scope leaks the storage it points to. > 356 } > 357 > 358 static void amdvi_completion_wait(AMDVIState *s, uint64_t *cmd) > 359 { > 360 /* pad the last 3 bits */ > 361 hwaddr addr = cpu_to_le64(extract64(cmd[0], 3, 49)) << 3; > > > > > > > ** CID 1363369: Resource leaks (RESOURCE_LEAK) > /hw/i386/amd_iommu.c: 356 in amdvi_update_iotlb() > ________________________________________________________________________________________________________ > 350 entry->perms = to_cache.perm; > 351 entry->translated_addr = to_cache.translated_addr; > 352 entry->page_mask = to_cache.addr_mask; > 353 *key = gfn | ((uint64_t)(devid) << AMDVI_DEVID_SHIFT); > 354 g_hash_table_replace(s->iotlb, key, entry); > 355 } >>>> CID 1363369: Resource leaks (RESOURCE_LEAK) >>>> Variable "key" going out of scope leaks the storage it points to. > 356 } > 357 > 358 static void amdvi_completion_wait(AMDVIState *s, uint64_t *cmd) > 359 { > 360 /* pad the last 3 bits */ > 361 hwaddr addr = cpu_to_le64(extract64(cmd[0], 3, 49)) << 3; > > > > > > > ** CID 1363364: (CHECKED_RETURN) > /hw/i386/amd_iommu.c: 1150 in amdvi_realize() > /hw/i386/amd_iommu.c: 1151 in amdvi_realize() > ________________________________________________________________________________________________________ > 1144 /* This device should take care of IOMMU PCI properties */ > 1145 x86_iommu->type = TYPE_AMD; > 1146 qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus); > 1147 object_property_set_bool(OBJECT(&s->pci), true, "realized", err); > 1148 s->capab_offset = pci_add_capability(&s->pci.dev, > AMDVI_CAPAB_ID_SEC, 0, > 1149 AMDVI_CAPAB_SIZE); >>>> CID 1363364: (CHECKED_RETURN) >>>> Calling "pci_add_capability" without checking return value. > 1150 pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, > AMDVI_CAPAB_REG_SIZE); >>>> CID 1363364: (CHECKED_RETURN) >>>> Calling "pci_add_capability" without checking return value. > 1151 pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, > AMDVI_CAPAB_REG_SIZE); > 1152 > 1153 /* set up MMIO */ > 1154 memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, > "amdvi-mmio", > 1155 AMDVI_MMIO_SIZE); > 1156 > > > > > > > > ** CID 1363363: Integer handling issues (BAD_SHIFT) > /hw/i386/amd_iommu.c: 188 in amdvi_setevent_bits() > ________________________________________________________________________________________________________ > 182 } > 183 > 184 static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int > start, > 185 int length) > 186 { > 187 int index = start / 64, bitpos = start % 64; >>>> CID 1363363: Integer handling issues (BAD_SHIFT) >>>> In expression "(1 << length) - 1 << bitpos", left shifting by more than >>>> 31 bits has undefined behavior. The shift amount, "bitpos", is as >>>> much as 63. > 188 uint64_t mask = ((1 << length) - 1) << bitpos; > 189 buffer[index] &= ~mask; > 190 buffer[index] |= (value << bitpos) & mask; > 191 } > 192 /* > 193 * AMDVi event structure