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

[Suthikulpanit, Suravee]

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