Re: [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb()

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

+ Suravee

On 2/17/25 02:26, Philippe Mathieu-Daudé wrote:
> On 31/7/24 19:00, Peter Maydell wrote:
>> In amdvi_update_iotlb() we will only put a new entry in the hash
>> table if to_cache.perm is not IOMMU_NONE.  However we allocate the
>> memory for the new AMDVIIOTLBEntry and for the hash table key
>> regardless.  This means that in the IOMMU_NONE case we will leak the
>> memory we alloacted.
>>
>> Move the allocations into the if() to the point where we know we're
>> going to add the item to the hash table.
>>
>> Cc: qemu-stable@nongnu.org
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2452
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Tested with 'make check' and 'make check-avocado' only, but the
>> bug and fix seem straightforward...
> 
> Oh now I remembered:
> https://lore.kernel.org/qemu-devel/CACGkMEtjmpX8G9HYZ0r3n5ErhAENKhQ81f4ocfCYrh=XoF=5hw@mail.gmail.com/

Re: AMD vIOMMU status, this recent series:

https://lore.kernel.org/all/20240927172913.121477-1-santosh.shukla@amd.com/

added support for using it with VFIO and the guest in passthrough mode.

I'm currently working on adding support for DMA remapping, and have a basic prototype that boots a guest with a VFIO assigned device and iommu.passthrough=0, which exercises the change in this patch.

I am using the fixes and optimizations that have gone into the VTD emulation code as a template to bring the AMDVi DMA remap up to a functional state. I'll send an RFC soon once I have fixed a remaining issue with the prototype.

Thank you,
Alejandro

> 
>> ---
>>   hw/i386/amd_iommu.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 6d4fde72f9b..87643d28917 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -357,12 +357,12 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
>>                                  uint64_t gpa, IOMMUTLBEntry to_cache,
>>                                  uint16_t domid)
>>   {
>> -    AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
>> -    uint64_t *key = g_new(uint64_t, 1);
>> -    uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
>> -
>>       /* don't cache erroneous translations */
>>       if (to_cache.perm != IOMMU_NONE) {
>> +        AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
>> +        uint64_t *key = g_new(uint64_t, 1);
>> +        uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
>> +
>>           trace_amdvi_cache_update(domid, PCI_BUS_NUM(devid), PCI_SLOT(devid),
>>                   PCI_FUNC(devid), gpa, to_cache.translated_addr);
> 
>