Re: [PATCH 06/24] iommu/amd: Map vIOMMU VF and VF Control MMIO BARs

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

On 8/10/2026 5:00 PM, Vasant Hegde wrote:
> Hi Suravee,
> 
>> .....
>>
>> diff --git a/drivers/iommu/amd/viommu.c b/drivers/iommu/amd/viommu.c
>> index f4b5f96d4785..014ae16bf58b 100644
>> --- a/drivers/iommu/amd/viommu.c
>> +++ b/drivers/iommu/amd/viommu.c
>> @@ -7,9 +7,15 @@
>>   #define dev_fmt(fmt)    pr_fmt(fmt)
>>   
>>   #include <linux/iommu.h>
>> +#include <linux/amd-iommu.h>
>> +
>> +#include <linux/fs.h>
>> +#include <linux/cdev.h>
>> +#include <linux/ioctl.h>
>>   #include <linux/iommufd.h>
>>   #include <linux/amd-iommu.h>
>>   #include <uapi/linux/iommufd.h>
>> +#include <linux/mem_encrypt.h>
>>   
>>   #include <asm/iommu.h>
>>   #include <asm/set_memory.h>
>> @@ -18,12 +24,130 @@
>>   #include "amd_iommu.h"
>>   #include "amd_iommu_types.h"
>>   #include "amd_viommu.h"
>> +#include "../iommu-pages.h"
>> +
>> +LIST_HEAD(viommu_devid_map);
>> +
>> +static int viommu_init_pci_vsc(struct amd_iommu *iommu)
> 
> May be viommu_find_pci_vsc as its not initializing, instead find the offset.

I'll remove this

> Also if we pass vsc_offset to viommu_vf_vfcntl_init() -OR- call this function
> from viommu_vf_vfcntl_init() then we can skip tracking `vsc_offset` in amd_iommu
> structure.

Ok. I'll refactor to consolidate viommu_init_pci_vsc() and 
viommu_vf_vfcntl_init() and remove vsc_offset.

>> +{
>> +	iommu->vsc_offset = pci_find_capability(iommu->dev, PCI_CAP_ID_VNDR);
>> +	if (!iommu->vsc_offset)
>> +		return -ENODEV;
>> +
>> +	DUMP_printk("device:%s, vsc offset:%04x\n",
>> +		    pci_name(iommu->dev), iommu->vsc_offset);
>> +	return 0;
>> +}
>> +
>> +static void amd_viommu_gid_ida_init(struct amd_iommu *iommu)
>> +{
>> +	ida_init(&iommu->gid_ida);
>> +	iommu->gid_ida_inited = true;
> 
> Once we remove `gid_ida_inited` may be we can call `ida_init` inside
> amd_viommu_init() itself.

Ok.

>> +}
>> +
>> +static void amd_viommu_gid_ida_fini(struct amd_iommu *iommu)
>> +{
>> +	if (!iommu->gid_ida_inited)
>> +		return;
>> +
>> +	ida_destroy(&iommu->gid_ida);
>> +	iommu->gid_ida_inited = false;
>> +}
>> +
>> +static void __init amd_viommu_vf_vfcntl_unmap(struct amd_iommu *iommu)
>> +{
>> +	if (iommu->vfctrl_base) {
>> +		iounmap(iommu->vfctrl_base);
>> +		iommu->vfctrl_base = NULL;
>> +	}
>> +	if (iommu->vf_cntl_phys)
>> +		release_mem_region(iommu->vf_cntl_phys, VIOMMU_VF_CNTL_MMIO_MAP_SIZE);
>> +
>> +	if (iommu->vf_base) {
>> +		iounmap(iommu->vf_base);
>> +		iommu->vf_base = NULL;
>> +	}
>> +	if (iommu->vf_base_phys)
>> +		release_mem_region(iommu->vf_base_phys, VIOMMU_VF_MMIO_MAP_SIZE);
>> +}
>> +
>> +void __init amd_viommu_uninit(struct amd_iommu *iommu)
>> +{
>> +	amd_viommu_gid_ida_fini(iommu);
>> +	amd_viommu_vf_vfcntl_unmap(iommu);
>> +}
>> +
>> +static int __init viommu_vf_vfcntl_init(struct amd_iommu *iommu)
>> +{
>> +	u32 lo, hi;
>> +	u64 vf_phys, vf_cntl_phys;
>> +
>> +	/* Setting up VF and VF_CNTL MMIOs */
>> +	pci_read_config_dword(iommu->dev, iommu->vsc_offset + MMIO_VSC_VF_BAR_LO_OFFSET, &lo);
>> +	pci_read_config_dword(iommu->dev, iommu->vsc_offset + MMIO_VSC_VF_BAR_HI_OFFSET, &hi);
>> +	vf_phys = hi;
>> +	vf_phys = (vf_phys << 32) | lo;
>> +	if (!(vf_phys & 1)) {
>> +		pr_err(FW_BUG "vf_phys disabled\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pci_read_config_dword(iommu->dev, iommu->vsc_offset + MMIO_VSC_VF_CNTL_BAR_LO_OFFSET, &lo);
>> +	pci_read_config_dword(iommu->dev, iommu->vsc_offset + MMIO_VSC_VF_CNTL_BAR_HI_OFFSET, &hi);
>> +	vf_cntl_phys = hi;
>> +	vf_cntl_phys = (vf_cntl_phys << 32) | lo;
>> +	if (!(vf_cntl_phys & 1)) {
>> +		pr_err(FW_BUG "vf_cntl_phys disabled\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!vf_phys || !vf_cntl_phys) {
>> +		pr_err(FW_BUG "AMD-Vi: Unassigned VF resources.\n");
>> +		return -ENOMEM;
>> +	}
> 
> Redundant check as previous check confirms both vf_phys and vf_cntl_phys is
> enabled. If we want to check non-zero address then we have to remove bit zero
> and add check

Good point. I'll remove this for now.

>> +
>> +	/* Mapping 256MB of VF and 4MB of VF_CNTL BARs */
>> +	vf_phys &= ~1ULL;
>> +	iommu->vf_base = iommu_map_mmio_space(vf_phys, VIOMMU_VF_MMIO_MAP_SIZE);
>> +	if (!iommu->vf_base) {
>> +		pr_err("Can't reserve vf_base\n");
>> +		return -ENOMEM;
>> +	}
>> +	iommu->vf_base_phys = vf_phys;
>> +
>> +	vf_cntl_phys &= ~1ULL;
>> +	iommu->vfctrl_base = iommu_map_mmio_space(vf_cntl_phys, VIOMMU_VF_CNTL_MMIO_MAP_SIZE);
>> +	if (!iommu->vfctrl_base) {
>> +		pr_err("Can't reserve vfctrl_base\n");
>> +		goto err_out;
>> +	}
>> +	iommu->vf_cntl_phys = vf_cntl_phys;
>> +
>> +	pr_debug("%s: IOMMU device:%s, vf_base:%#llx, vfctrl_base:%#llx\n",
> 
> better DUMP_printk ?

Actually, I prefer pr_debug().

>> +		 __func__, pci_name(iommu->dev), vf_phys, vf_cntl_phys);
>> +	return 0;
>> +err_out:
>> +	amd_viommu_uninit(iommu);
> 
> Its odd. We shouldn't call amd_viommu_uninit from here. May be call
> amd_viommu_vf_vfcntl_unmap() ?

I'm fixing this in V5.

Thanks,
Suravee