| Message ID | 20250218111017.491719-14-aik@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,01/22] pci/doe: Define protocol types and make those public | expand |
On 2/4/25 03:11, Jason Gunthorpe wrote: > On Tue, Feb 18, 2025 at 10:10:00PM +1100, Alexey Kardashevskiy wrote: >> @@ -939,6 +939,7 @@ struct iommu_fault_alloc { >> enum iommu_viommu_type { >> IOMMU_VIOMMU_TYPE_DEFAULT = 0, >> IOMMU_VIOMMU_TYPE_ARM_SMMUV3 = 1, >> + IOMMU_VIOMMU_TYPE_TSM = 2, > > This should probably be some kind of AMD_TSM and the driver data blob > should carry any additional data needed to create the vIOMMU that is > visible to the guest. > >> @@ -2068,7 +2069,18 @@ static void set_dte_entry(struct amd_iommu *iommu, >> new.data[1] |= DTE_FLAG_IOTLB; >> >> old_domid = READ_ONCE(dte->data[1]) & DEV_DOMID_MASK; >> - new.data[1] |= domid; >> + >> + if (domain->aviommu) { > > AMD should be implementing viommu natively without CC as well, try to > structure things so it fits together better. This should only trigger > for the CC viommu type.. > >> + /* >> + * This runs when VFIO is bound to a device but TDI is not yet. >> + * Ideally TSM should change DTE only when TDI is bound. >> + */ >> + dev_info(dev_data->dev, "Skip DomainID=%x and set bit96\n", domid); >> + new.data[1] |= 1ULL << (96 - 64); >> + } else { >> + dev_info(dev_data->dev, "Not skip DomainID=%x and not set bit96\n", domid); >> + new.data[1] |= domid; >> + } >> >> /* >> * Restore cached persistent DTE bits, which can be set by information >> @@ -2549,12 +2561,15 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags, >> { >> struct amd_iommu *iommu = get_amd_iommu_from_dev(dev); >> const u32 supported_flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | >> + IOMMU_HWPT_ALLOC_PASID | >> + IOMMU_HWPT_ALLOC_NEST_PARENT; >> + const u32 supported_flags2 = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | >> IOMMU_HWPT_ALLOC_PASID; > > Just ignore NEST_PARENT? That seems wrong, it should force a V1 page > table?? Ahhh... This is because I still have troubles with what IOMMU_DOMAIN_NESTED means (and iommufd.rst does not help me). There is one device, one IOMMU table buuut 2 domains? Uh. > >> +static struct iommufd_viommu *amd_viommu_alloc(struct device *dev, >> + struct iommu_domain *parent, >> + struct iommufd_ctx *ictx, >> + unsigned int viommu_type) >> +{ >> + struct amd_viommu *aviommu; >> + struct protection_domain *domain = to_pdomain(parent); >> + >> + if (viommu_type != IOMMU_VIOMMU_TYPE_TSM) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + aviommu = iommufd_viommu_alloc(ictx, struct amd_viommu, core, &amd_viommu_ops); >> + if (IS_ERR(aviommu)) >> + return ERR_CAST(aviommu); >> + >> + aviommu->domain = domain; > > This is not OK, the parent domain of the viommu can be used with > multiple viommu objects, it can't just have a naked back reference > like this. > > You can get 1:1 domain objects linked to the viommu by creating the > 'S1' type domains, maybe that is what you want here. A special domain > type that is TSM that has a special DTE. Should not IOMMU_DOMAIN_NESTED be that "S1" domain? And what does "S1" mean here? Currently the domain in the hunk above is __IOMMU_DOMAIN_PAGING. > Though I'd really rather see the domain attach logic and DTE formation > in the AMD driver be fixed up before we made it more complex :\ > > It would be nice to see normal nesting and viommu support first too :\ It is in the works too. Thanks,
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h index b086fb632990..b5513bf05b27 100644 --- a/drivers/iommu/amd/amd_iommu_types.h +++ b/drivers/iommu/amd/amd_iommu_types.h @@ -593,6 +593,8 @@ struct protection_domain { struct mmu_notifier mn; /* mmu notifier for the SVA domain */ struct list_head dev_data_list; /* List of pdom_dev_data */ + + struct amd_viommu *aviommu; }; /* diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index 78747b24bd0f..b346fa11955c 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -939,6 +939,7 @@ struct iommu_fault_alloc { enum iommu_viommu_type { IOMMU_VIOMMU_TYPE_DEFAULT = 0, IOMMU_VIOMMU_TYPE_ARM_SMMUV3 = 1, + IOMMU_VIOMMU_TYPE_TSM = 2, }; /** diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index b48a72bd7b23..076c58d61d5e 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -30,6 +30,7 @@ #include <linux/percpu.h> #include <linux/io-pgtable.h> #include <linux/cc_platform.h> +#include <linux/iommufd.h> #include <asm/irq_remapping.h> #include <asm/io_apic.h> #include <asm/apic.h> @@ -2068,7 +2069,18 @@ static void set_dte_entry(struct amd_iommu *iommu, new.data[1] |= DTE_FLAG_IOTLB; old_domid = READ_ONCE(dte->data[1]) & DEV_DOMID_MASK; - new.data[1] |= domid; + + if (domain->aviommu) { + /* + * This runs when VFIO is bound to a device but TDI is not yet. + * Ideally TSM should change DTE only when TDI is bound. + */ + dev_info(dev_data->dev, "Skip DomainID=%x and set bit96\n", domid); + new.data[1] |= 1ULL << (96 - 64); + } else { + dev_info(dev_data->dev, "Not skip DomainID=%x and not set bit96\n", domid); + new.data[1] |= domid; + } /* * Restore cached persistent DTE bits, which can be set by information @@ -2549,12 +2561,15 @@ amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags, { struct amd_iommu *iommu = get_amd_iommu_from_dev(dev); const u32 supported_flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | + IOMMU_HWPT_ALLOC_PASID | + IOMMU_HWPT_ALLOC_NEST_PARENT; + const u32 supported_flags2 = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | IOMMU_HWPT_ALLOC_PASID; if ((flags & ~supported_flags) || user_data) return ERR_PTR(-EOPNOTSUPP); - switch (flags & supported_flags) { + switch (flags & supported_flags2) { case IOMMU_HWPT_ALLOC_DIRTY_TRACKING: /* Allocate domain with v1 page table for dirty tracking */ if (!amd_iommu_hd_support(iommu)) @@ -3015,6 +3030,46 @@ static int amd_iommu_dev_disable_feature(struct device *dev, return ret; } +struct amd_viommu { + struct iommufd_viommu core; + struct protection_domain *domain; +}; + +static void amd_viommu_destroy(struct iommufd_viommu *viommu) +{ + struct amd_viommu *aviommu = container_of(viommu, struct amd_viommu, core); + + if (!aviommu->domain) + return; + aviommu->domain->aviommu = NULL; +} + + +static const struct iommufd_viommu_ops amd_viommu_ops = { + .destroy = amd_viommu_destroy, +}; + +static struct iommufd_viommu *amd_viommu_alloc(struct device *dev, + struct iommu_domain *parent, + struct iommufd_ctx *ictx, + unsigned int viommu_type) +{ + struct amd_viommu *aviommu; + struct protection_domain *domain = to_pdomain(parent); + + if (viommu_type != IOMMU_VIOMMU_TYPE_TSM) + return ERR_PTR(-EOPNOTSUPP); + + aviommu = iommufd_viommu_alloc(ictx, struct amd_viommu, core, &amd_viommu_ops); + if (IS_ERR(aviommu)) + return ERR_CAST(aviommu); + + aviommu->domain = domain; + domain->aviommu = aviommu; + + return &aviommu->core; +} + const struct iommu_ops amd_iommu_ops = { .capable = amd_iommu_capable, .blocked_domain = &blocked_domain, @@ -3031,6 +3086,7 @@ const struct iommu_ops amd_iommu_ops = { .dev_enable_feat = amd_iommu_dev_enable_feature, .dev_disable_feat = amd_iommu_dev_disable_feature, .page_response = amd_iommu_page_response, + .viommu_alloc = amd_viommu_alloc, .default_domain_ops = &(const struct iommu_domain_ops) { .attach_dev = amd_iommu_attach_device, .map_pages = amd_iommu_map_pages,
The new AMD SEV-TIO feature allows DMA to/from encrypted memory and encrypted MMIO for use in CoCo VMs (SEV-SNP). The secure part of IOMMU is called sDTE ("Secure DTE") which resides in private memory and controlled by the PSP. sDTEs of passed through devices are in the TCB of CoCo VMs and inaccessible by the host OS. Implement vdevice in the AMD IOMMU host OS to represent the host instance of a secure IOMMU which is visible to the guest. This will be used for GHCB TIO GUEST REQUEST to manage secure sDTE and MMIO. Most parts of insecure DTE move to sDTE so DTEs need to be adjusted. At the moment this includes "domain_id" (moves to sDTE) and "IOTLB enable" (should stay in sync with sDTE). Signed-off-by: Alexey Kardashevskiy <aik@amd.com> --- drivers/iommu/amd/amd_iommu_types.h | 2 + include/uapi/linux/iommufd.h | 1 + drivers/iommu/amd/iommu.c | 60 +++++++++++++++++++- 3 files changed, 61 insertions(+), 2 deletions(-)