| Message ID | c5d2eaf3-77f6-f87e-6898-c4c475f607c1@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD IOMMU: further improvements | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 19 September 2019 14:25 > To: xen-devel@lists.xenproject.org > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > Subject: [Xen-devel] [PATCH v6 8/8] AMD/IOMMU: pre-fill all DTEs right after table allocation > > Make sure we don't leave any DTEs unexpected requests through which > would be passed through untranslated. Set V and IV right away (with > all other fields left as zero), relying on the V and/or IV bits > getting cleared only by amd_iommu_set_root_page_table() and > amd_iommu_set_intremap_table() under special pass-through circumstances. > Switch back to initial settings in amd_iommu_disable_domain_device(). > > Take the liberty and also make the latter function static, constifying > its first parameter at the same time, at this occasion. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- > v6: New. > > --- > xen/drivers/passthrough/amd/iommu_init.c | 22 +++++++++++++++++++--- > xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++++++++++++++---- > 2 files changed, 35 insertions(+), 7 deletions(-) > > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -1255,12 +1255,28 @@ static int __init amd_iommu_setup_device > > if ( !dt ) > { > + unsigned int size = dt_alloc_size(); > + > /* allocate 'device table' on a 4K boundary */ > dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) = > - allocate_buffer(dt_alloc_size(), "Device Table"); > + allocate_buffer(size, "Device Table"); > + if ( !dt ) > + return -ENOMEM; > + > + /* > + * Prefill every DTE such that all kinds of requests will get aborted. > + * Besides the two bits set to true below this builds upon > + * IOMMU_DEV_TABLE_SYS_MGT_DMA_ABORTED, > + * IOMMU_DEV_TABLE_IO_CONTROL_ABORTED, as well as > + * IOMMU_DEV_TABLE_INT_CONTROL_ABORTED all being zero, and us also > + * wanting at least TV, GV, I, and EX set to false. > + */ > + for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf ) > + { > + dt[bdf].v = true; > + dt[bdf].iv = true; > + } > } > - if ( !dt ) > - return -ENOMEM; > > /* Add device table entries */ > for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -267,9 +267,9 @@ static void __hwdom_init amd_iommu_hwdom > setup_hwdom_pci_devices(d, amd_iommu_add_device); > } > > -void amd_iommu_disable_domain_device(struct domain *domain, > - struct amd_iommu *iommu, > - u8 devfn, struct pci_dev *pdev) > +static void amd_iommu_disable_domain_device(const struct domain *domain, > + struct amd_iommu *iommu, > + uint8_t devfn, struct pci_dev *pdev) > { > struct amd_iommu_dte *table, *dte; > unsigned long flags; > @@ -284,9 +284,21 @@ void amd_iommu_disable_domain_device(str > spin_lock_irqsave(&iommu->lock, flags); > if ( dte->tv || dte->v ) > { > + /* See the comment in amd_iommu_setup_device_table(). */ > + dte->int_ctl = IOMMU_DEV_TABLE_INT_CONTROL_ABORTED; > + smp_wmb(); > + dte->iv = true; > dte->tv = false; > - dte->v = false; > + dte->gv = false; > dte->i = false; > + dte->ex = false; > + dte->sa = false; > + dte->se = false; > + dte->sd = false; > + dte->sys_mgt = IOMMU_DEV_TABLE_SYS_MGT_DMA_ABORTED; > + dte->ioctl = IOMMU_DEV_TABLE_IO_CONTROL_ABORTED; > + smp_wmb(); > + dte->v = true; > > amd_iommu_flush_device(iommu, req_id); > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On 19/09/2019 14:25, Jan Beulich wrote: > Make sure we don't leave any DTEs unexpected requests through which > would be passed through untranslated. Set V and IV right away (with > all other fields left as zero), relying on the V and/or IV bits > getting cleared only by amd_iommu_set_root_page_table() and > amd_iommu_set_intremap_table() under special pass-through circumstances. > Switch back to initial settings in amd_iommu_disable_domain_device(). > > Take the liberty and also make the latter function static, constifying > its first parameter at the same time, at this occasion. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v6: New. > > --- > xen/drivers/passthrough/amd/iommu_init.c | 22 +++++++++++++++++++--- > xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++++++++++++++---- > 2 files changed, 35 insertions(+), 7 deletions(-) > > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -1255,12 +1255,28 @@ static int __init amd_iommu_setup_device > > if ( !dt ) > { > + unsigned int size = dt_alloc_size(); > + > /* allocate 'device table' on a 4K boundary */ > dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) = > - allocate_buffer(dt_alloc_size(), "Device Table"); > + allocate_buffer(size, "Device Table"); > + if ( !dt ) > + return -ENOMEM; > + > + /* > + * Prefill every DTE such that all kinds of requests will get aborted. > + * Besides the two bits set to true below this builds upon > + * IOMMU_DEV_TABLE_SYS_MGT_DMA_ABORTED, > + * IOMMU_DEV_TABLE_IO_CONTROL_ABORTED, as well as > + * IOMMU_DEV_TABLE_INT_CONTROL_ABORTED all being zero, and us also > + * wanting at least TV, GV, I, and EX set to false. > + */ > + for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf ) > + { > + dt[bdf].v = true; > + dt[bdf].iv = true; > + } The DT-BAR is generally 2MB in size. It is inconvenient that we first zero it, then walk it a second time setting two bits. I'm not sure that allocate_buffer() needs to zero memory for any of its callers. The command ring writes a full entry at once, and the IOMMU writes full event/page logs at once. Dropping the memset() and changing this to be a loop of structure assignments would be rather more efficient. ~Andrew
On 25.09.2019 16:59, Andrew Cooper wrote: > On 19/09/2019 14:25, Jan Beulich wrote: >> Make sure we don't leave any DTEs unexpected requests through which >> would be passed through untranslated. Set V and IV right away (with >> all other fields left as zero), relying on the V and/or IV bits >> getting cleared only by amd_iommu_set_root_page_table() and >> amd_iommu_set_intremap_table() under special pass-through circumstances. >> Switch back to initial settings in amd_iommu_disable_domain_device(). >> >> Take the liberty and also make the latter function static, constifying >> its first parameter at the same time, at this occasion. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v6: New. >> >> --- >> xen/drivers/passthrough/amd/iommu_init.c | 22 +++++++++++++++++++--- >> xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++++++++++++++---- >> 2 files changed, 35 insertions(+), 7 deletions(-) >> >> --- a/xen/drivers/passthrough/amd/iommu_init.c >> +++ b/xen/drivers/passthrough/amd/iommu_init.c >> @@ -1255,12 +1255,28 @@ static int __init amd_iommu_setup_device >> >> if ( !dt ) >> { >> + unsigned int size = dt_alloc_size(); >> + >> /* allocate 'device table' on a 4K boundary */ >> dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) = >> - allocate_buffer(dt_alloc_size(), "Device Table"); >> + allocate_buffer(size, "Device Table"); >> + if ( !dt ) >> + return -ENOMEM; >> + >> + /* >> + * Prefill every DTE such that all kinds of requests will get aborted. >> + * Besides the two bits set to true below this builds upon >> + * IOMMU_DEV_TABLE_SYS_MGT_DMA_ABORTED, >> + * IOMMU_DEV_TABLE_IO_CONTROL_ABORTED, as well as >> + * IOMMU_DEV_TABLE_INT_CONTROL_ABORTED all being zero, and us also >> + * wanting at least TV, GV, I, and EX set to false. >> + */ >> + for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf ) >> + { >> + dt[bdf].v = true; >> + dt[bdf].iv = true; >> + } > > The DT-BAR is generally 2MB in size. It is inconvenient that we first > zero it, then walk it a second time setting two bits. > > I'm not sure that allocate_buffer() needs to zero memory for any of its > callers. The command ring writes a full entry at once, and the IOMMU > writes full event/page logs at once. But in the latter case we need the zeroing to work around errata 732 and 733; see parse_{event,ppr}_log_entry(). > Dropping the memset() and changing this to be a loop of structure > assignments would be rather more efficient. I'll add a boolean parameter to allocate_buffer(). Jan
--- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1255,12 +1255,28 @@ static int __init amd_iommu_setup_device if ( !dt ) { + unsigned int size = dt_alloc_size(); + /* allocate 'device table' on a 4K boundary */ dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) = - allocate_buffer(dt_alloc_size(), "Device Table"); + allocate_buffer(size, "Device Table"); + if ( !dt ) + return -ENOMEM; + + /* + * Prefill every DTE such that all kinds of requests will get aborted. + * Besides the two bits set to true below this builds upon + * IOMMU_DEV_TABLE_SYS_MGT_DMA_ABORTED, + * IOMMU_DEV_TABLE_IO_CONTROL_ABORTED, as well as + * IOMMU_DEV_TABLE_INT_CONTROL_ABORTED all being zero, and us also + * wanting at least TV, GV, I, and EX set to false. + */ + for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf ) + { + dt[bdf].v = true; + dt[bdf].iv = true; + } } - if ( !dt ) - return -ENOMEM; /* Add device table entries */ for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -267,9 +267,9 @@ static void __hwdom_init amd_iommu_hwdom setup_hwdom_pci_devices(d, amd_iommu_add_device); } -void amd_iommu_disable_domain_device(struct domain *domain, - struct amd_iommu *iommu, - u8 devfn, struct pci_dev *pdev) +static void amd_iommu_disable_domain_device(const struct domain *domain, + struct amd_iommu *iommu, + uint8_t devfn, struct pci_dev *pdev) { struct amd_iommu_dte *table, *dte; unsigned long flags; @@ -284,9 +284,21 @@ void amd_iommu_disable_domain_device(str spin_lock_irqsave(&iommu->lock, flags); if ( dte->tv || dte->v ) { + /* See the comment in amd_iommu_setup_device_table(). */ + dte->int_ctl = IOMMU_DEV_TABLE_INT_CONTROL_ABORTED; + smp_wmb(); + dte->iv = true; dte->tv = false; - dte->v = false; + dte->gv = false; dte->i = false; + dte->ex = false; + dte->sa = false; + dte->se = false; + dte->sd = false; + dte->sys_mgt = IOMMU_DEV_TABLE_SYS_MGT_DMA_ABORTED; + dte->ioctl = IOMMU_DEV_TABLE_IO_CONTROL_ABORTED; + smp_wmb(); + dte->v = true; amd_iommu_flush_device(iommu, req_id);
Make sure we don't leave any DTEs unexpected requests through which would be passed through untranslated. Set V and IV right away (with all other fields left as zero), relying on the V and/or IV bits getting cleared only by amd_iommu_set_root_page_table() and amd_iommu_set_intremap_table() under special pass-through circumstances. Switch back to initial settings in amd_iommu_disable_domain_device(). Take the liberty and also make the latter function static, constifying its first parameter at the same time, at this occasion. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v6: New. --- xen/drivers/passthrough/amd/iommu_init.c | 22 +++++++++++++++++++--- xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-)