[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v19 0/4] AMD IOMMU
From: |
David Kiarie |
Subject: |
Re: [Qemu-devel] [v19 0/4] AMD IOMMU |
Date: |
Fri, 30 Sep 2016 16:57:46 +0300 |
On Fri, Sep 30, 2016 at 4:55 PM, Paolo Bonzini <address@hidden> 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