Re: [Qemu-devel] [v19 0/4] AMD IOMMU

[David Kiarie]

On Fri, Sep 30, 2016 at 4:57 PM, David Kiarie <address@hidden> wrote:
> 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     }

This looks like a false positive.

>> 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