| Message ID | 0036b69f-0d56-9ac4-1afa-06640c9007de@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VT-d: XSA-321 follow-up | expand |
On Wed, Jul 15, 2020 at 12:03:57PM +0200, Jan Beulich wrote: > Instead of checking inside the hook whether any non-coherent IOMMUs are > present, simply install the hook only when this is the case. > > To prove that there are no other references to the now dynamically > updated ops structure (and hence that its updating happens early > enough), make it static and rename it at the same time. > > Note that this change implies that sync_cache() shouldn't be called > directly unless there are unusual circumstances, like is the case in > alloc_pgtable_maddr(), which gets invoked too early for iommu_ops to > be set already (and therefore we also need to be careful there to > avoid accessing vtd_ops later on, as it lives in .init). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I think this is slightly better than what we currently have: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> I would however prefer if we also added a check to assert that alloc_pgtable_maddr is never called before iommu_alloc. We could maybe poison the .sync_cache field, and then either set to NULL or to sync_cache in iommu_alloc? Maybe I'm just overly paranoid with this. Thanks.
On 16.07.2020 12:14, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 12:03:57PM +0200, Jan Beulich wrote: >> Instead of checking inside the hook whether any non-coherent IOMMUs are >> present, simply install the hook only when this is the case. >> >> To prove that there are no other references to the now dynamically >> updated ops structure (and hence that its updating happens early >> enough), make it static and rename it at the same time. >> >> Note that this change implies that sync_cache() shouldn't be called >> directly unless there are unusual circumstances, like is the case in >> alloc_pgtable_maddr(), which gets invoked too early for iommu_ops to >> be set already (and therefore we also need to be careful there to >> avoid accessing vtd_ops later on, as it lives in .init). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I think this is slightly better than what we currently have: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > I would however prefer if we also added a check to assert that > alloc_pgtable_maddr is never called before iommu_alloc. It would be quite odd for this to happen - what point would there be to allocate a table to hang off of an IOMMU when no IOMMU was found at all so far? Furthermore, such a restriction could either be viewed to not suffice afaict (as a subsequent iommu_alloc() may in principle fine a non- coherent IOMMU), or to be pointless (until a non-coherent IOMMU was found and allocated any table for, it doesn't really matter whether we sync cache). In the end, whether to sync cache in alloc_pgtable_maddr() doesn't really depend on any global property, but only on the property / properties of the IOMMU(s) the table is going to be made visible to. > We could maybe > poison the .sync_cache field, and then either set to NULL or to > sync_cache in iommu_alloc? Poisoning is at least latently problematic, due to alternative insn patching we use for indirect calls. There are two passes, where the 1st pass skips any instances where the target address is still NULL. Of course that code could be made honor the poison value as well, but to me this looks like going too far. Jan
On Thu, Jul 16, 2020 at 12:25:40PM +0200, Jan Beulich wrote: > On 16.07.2020 12:14, Roger Pau Monné wrote: > > On Wed, Jul 15, 2020 at 12:03:57PM +0200, Jan Beulich wrote: > >> Instead of checking inside the hook whether any non-coherent IOMMUs are > >> present, simply install the hook only when this is the case. > >> > >> To prove that there are no other references to the now dynamically > >> updated ops structure (and hence that its updating happens early > >> enough), make it static and rename it at the same time. > >> > >> Note that this change implies that sync_cache() shouldn't be called > >> directly unless there are unusual circumstances, like is the case in > >> alloc_pgtable_maddr(), which gets invoked too early for iommu_ops to > >> be set already (and therefore we also need to be careful there to > >> avoid accessing vtd_ops later on, as it lives in .init). > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > I think this is slightly better than what we currently have: > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > > I would however prefer if we also added a check to assert that > > alloc_pgtable_maddr is never called before iommu_alloc. > > It would be quite odd for this to happen - what point would > there be to allocate a table to hang off of an IOMMU when > no IOMMU was found at all so far? Furthermore, such a > restriction could either be viewed to not suffice afaict (as > a subsequent iommu_alloc() may in principle fine a non- > coherent IOMMU), or to be pointless (until a non-coherent > IOMMU was found and allocated any table for, it doesn't > really matter whether we sync cache). In the end, whether to > sync cache in alloc_pgtable_maddr() doesn't really depend on > any global property, but only on the property / properties > of the IOMMU(s) the table is going to be made visible to. Right, I think I was indeed overly paranoid. I was mostly worried about iommu_alloc calling alloc_pgtable_maddr before checking whether the IOOMMU is incoherent, but this is not likely to happen. Thanks.
> From: Jan Beulich <jbeulich@suse.com> > Sent: Wednesday, July 15, 2020 6:04 PM > > Instead of checking inside the hook whether any non-coherent IOMMUs are > present, simply install the hook only when this is the case. > > To prove that there are no other references to the now dynamically > updated ops structure (and hence that its updating happens early > enough), make it static and rename it at the same time. > > Note that this change implies that sync_cache() shouldn't be called > directly unless there are unusual circumstances, like is the case in > alloc_pgtable_maddr(), which gets invoked too early for iommu_ops to > be set already (and therefore we also need to be careful there to > avoid accessing vtd_ops later on, as it lives in .init). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > --- a/xen/drivers/passthrough/vtd/extern.h > +++ b/xen/drivers/passthrough/vtd/extern.h > @@ -28,7 +28,6 @@ > struct pci_ats_dev; > extern bool_t rwbf_quirk; > extern const struct iommu_init_ops intel_iommu_init_ops; > -extern const struct iommu_ops intel_iommu_ops; > > void print_iommu_regs(struct acpi_drhd_unit *drhd); > void print_vtd_entries(struct vtd_iommu *iommu, int bus, int devfn, u64 > gmfn); > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -59,6 +59,7 @@ bool __read_mostly iommu_snoop = true; > > int nr_iommus; > > +static struct iommu_ops vtd_ops; > static struct tasklet vtd_fault_tasklet; > > static int setup_hwdom_device(u8 devfn, struct pci_dev *); > @@ -146,16 +147,11 @@ static int context_get_domain_id(struct > return domid; > } > > -static int iommus_incoherent; > - > static void sync_cache(const void *addr, unsigned int size) > { > static unsigned long clflush_size = 0; > const void *end = addr + size; > > - if ( !iommus_incoherent ) > - return; > - > if ( clflush_size == 0 ) > clflush_size = get_cache_line_size(); > > @@ -217,7 +213,8 @@ uint64_t alloc_pgtable_maddr(unsigned lo > vaddr = __map_domain_page(cur_pg); > memset(vaddr, 0, PAGE_SIZE); > > - sync_cache(vaddr, PAGE_SIZE); > + if ( (iommu_ops.init ? &iommu_ops : &vtd_ops)->sync_cache ) > + sync_cache(vaddr, PAGE_SIZE); > unmap_domain_page(vaddr); > cur_pg++; > } > @@ -1227,7 +1224,7 @@ int __init iommu_alloc(struct acpi_drhd_ > iommu->nr_pt_levels = agaw_to_level(agaw); > > if ( !ecap_coherent(iommu->ecap) ) > - iommus_incoherent = 1; > + vtd_ops.sync_cache = sync_cache; > > /* allocate domain id bitmap */ > nr_dom = cap_ndoms(iommu->cap); > @@ -2737,7 +2734,7 @@ static int __init intel_iommu_quarantine > return level ? -ENOMEM : rc; > } > > -const struct iommu_ops __initconstrel intel_iommu_ops = { > +static struct iommu_ops __initdata vtd_ops = { > .init = intel_iommu_domain_init, > .hwdom_init = intel_iommu_hwdom_init, > .quarantine_init = intel_iommu_quarantine_init, > @@ -2768,11 +2765,10 @@ const struct iommu_ops __initconstrel in > .iotlb_flush_all = iommu_flush_iotlb_all, > .get_reserved_device_memory = > intel_iommu_get_reserved_device_memory, > .dump_p2m_table = vtd_dump_p2m_table, > - .sync_cache = sync_cache, > }; > > const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { > - .ops = &intel_iommu_ops, > + .ops = &vtd_ops, > .setup = vtd_setup, > .supports_x2apic = intel_iommu_supports_eim, > };
--- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -28,7 +28,6 @@ struct pci_ats_dev; extern bool_t rwbf_quirk; extern const struct iommu_init_ops intel_iommu_init_ops; -extern const struct iommu_ops intel_iommu_ops; void print_iommu_regs(struct acpi_drhd_unit *drhd); void print_vtd_entries(struct vtd_iommu *iommu, int bus, int devfn, u64 gmfn); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -59,6 +59,7 @@ bool __read_mostly iommu_snoop = true; int nr_iommus; +static struct iommu_ops vtd_ops; static struct tasklet vtd_fault_tasklet; static int setup_hwdom_device(u8 devfn, struct pci_dev *); @@ -146,16 +147,11 @@ static int context_get_domain_id(struct return domid; } -static int iommus_incoherent; - static void sync_cache(const void *addr, unsigned int size) { static unsigned long clflush_size = 0; const void *end = addr + size; - if ( !iommus_incoherent ) - return; - if ( clflush_size == 0 ) clflush_size = get_cache_line_size(); @@ -217,7 +213,8 @@ uint64_t alloc_pgtable_maddr(unsigned lo vaddr = __map_domain_page(cur_pg); memset(vaddr, 0, PAGE_SIZE); - sync_cache(vaddr, PAGE_SIZE); + if ( (iommu_ops.init ? &iommu_ops : &vtd_ops)->sync_cache ) + sync_cache(vaddr, PAGE_SIZE); unmap_domain_page(vaddr); cur_pg++; } @@ -1227,7 +1224,7 @@ int __init iommu_alloc(struct acpi_drhd_ iommu->nr_pt_levels = agaw_to_level(agaw); if ( !ecap_coherent(iommu->ecap) ) - iommus_incoherent = 1; + vtd_ops.sync_cache = sync_cache; /* allocate domain id bitmap */ nr_dom = cap_ndoms(iommu->cap); @@ -2737,7 +2734,7 @@ static int __init intel_iommu_quarantine return level ? -ENOMEM : rc; } -const struct iommu_ops __initconstrel intel_iommu_ops = { +static struct iommu_ops __initdata vtd_ops = { .init = intel_iommu_domain_init, .hwdom_init = intel_iommu_hwdom_init, .quarantine_init = intel_iommu_quarantine_init, @@ -2768,11 +2765,10 @@ const struct iommu_ops __initconstrel in .iotlb_flush_all = iommu_flush_iotlb_all, .get_reserved_device_memory = intel_iommu_get_reserved_device_memory, .dump_p2m_table = vtd_dump_p2m_table, - .sync_cache = sync_cache, }; const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { - .ops = &intel_iommu_ops, + .ops = &vtd_ops, .setup = vtd_setup, .supports_x2apic = intel_iommu_supports_eim, };
Instead of checking inside the hook whether any non-coherent IOMMUs are present, simply install the hook only when this is the case. To prove that there are no other references to the now dynamically updated ops structure (and hence that its updating happens early enough), make it static and rename it at the same time. Note that this change implies that sync_cache() shouldn't be called directly unless there are unusual circumstances, like is the case in alloc_pgtable_maddr(), which gets invoked too early for iommu_ops to be set already (and therefore we also need to be careful there to avoid accessing vtd_ops later on, as it lives in .init). Signed-off-by: Jan Beulich <jbeulich@suse.com>