[...]
> > diff --git a/drivers/iommu/hyperv/Kconfig
> > b/drivers/iommu/hyperv/Kconfig index 30f40d867036..9e658d5c9a77 100644
> > --- a/drivers/iommu/hyperv/Kconfig
> > +++ b/drivers/iommu/hyperv/Kconfig
> > @@ -8,3 +8,20 @@ config HYPERV_IOMMU
> > help
> > Stub IOMMU driver to handle IRQs to support Hyper-V Linux
> > guest and root partitions.
> > +
> > +if HYPERV_IOMMU
> > +config HYPERV_PVIOMMU
> > + bool "Microsoft Hypervisor para-virtualized IOMMU support"
> > + depends on X86 && HYPERV
> > + select IOMMU_API
> > + select GENERIC_PT
> > + select IOMMU_PT
> > + select IOMMU_PT_X86_64
> nit:
> If HYPERV_PVIOMMU is enabled on a (hypothetical) platform with
> GENERIC_ATOMIC64=y, the select would force-enable IOMMU_PT_X86_64 even
> though its depends on is unsatisfied — leading to a build failure.
>
> In practice this can't happen today because HYPERV_PVIOMMU already
> depends on X86 && HYPERV, and x86 never sets GENERIC_ATOMIC64. But
> adding the explicit guard is more defensive.
> i.e.
> depends on !GENERIC_ATOMIC64 # for cmpxchg64 in IOMMU_PT
>
Good point. Will add "depends on !GENERIC_ATOMIC64".
[...]
> > +
> > +/*
> > + * Look up the logical device ID for a vPCI device. Returns 0 on
> > success
> > + * with *logical_id filled in; -ENODEV if no entry registered for
> > this
> > + * device's vPCI bus.
> > + */
> > +static int hv_iommu_lookup_logical_dev_id(struct pci_dev *pdev, u64
> > *logical_id) +{
> > + struct hv_pci_busdata *bus;
> > + int domain = pci_domain_nr(pdev->bus);
> > + int ret = -ENODEV;
> > +
> > + spin_lock(&hv_iommu_pci_bus_lock);
> this is called under local_irq_save, should use raw_spinlock_t for RT
> kernel?
>
Yes, this is problematic on PREEMPT_RT. Michael also suggested hoisting
the lookup before the local_irq_save() section instead of using a raw
spinlock, which I think is a great idea - all 3 call sites (detach_dev,
attach_dev, get_logical_device_property) will be changed.
> > + list_for_each_entry(bus, &hv_iommu_pci_bus_list, list) {
> > + if (bus->pci_domain_nr == domain) {
> > + *logical_id =
> > (u64)bus->logical_dev_id_prefix |
> > + PCI_FUNC(pdev->devfn);
> > + ret = 0;
> > + break;
> > + }
> > + }
> > + spin_unlock(&hv_iommu_pci_bus_lock);
> > + return ret;
> > +}
> > +
[...]
> > +static void hv_iommu_release_device(struct device *dev)
> > +{
> > + struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > + if (pdev->ats_enabled)
> > + pci_disable_ats(pdev);
> > +
> > + dev_iommu_priv_set(dev, NULL);
> > + set_dma_ops(dev, NULL);
> I don't think this is necessary.
>
Oh, yes. Thanks!
> > +
> > + kfree(vdev);
> > +}
> > +
> > +static struct iommu_group *hv_iommu_device_group(struct device *dev)
> > +{
> > + if (dev_is_pci(dev))
> > + return pci_device_group(dev);
> > + else
> > + return generic_device_group(dev);
> non pci device already rejected during attach, maybe we should warn
> here?
> WARN_ON_ONCE(1);
> return generic_device_group(dev);
>
Good idea. Will add WARN_ON_ONCE(1).
> > +}
> > +
> > +static int hv_configure_device_domain(struct hv_iommu_domain
> > *hv_domain, u32 domain_type) +{
> > + u64 status;
> > + unsigned long flags;
> > + struct pt_iommu_x86_64_hw_info pt_info;
> > + struct hv_input_configure_device_domain *input;
> > +
> > + local_irq_save(flags);
> > +
> > + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> > + memset(input, 0, sizeof(*input));
> > + input->device_domain = hv_domain->device_domain;
> > + input->settings.flags.blocked = (domain_type ==
> > IOMMU_DOMAIN_BLOCKED);
> > + input->settings.flags.translation_enabled = (domain_type !=
> > IOMMU_DOMAIN_IDENTITY); +
> Should this be:
> input->settings.flags.translation_enabled =
> (domain_type & __IOMMU_DOMAIN_PAGING);
> Otherwise, blocked domain will have translation enabled. Maybe add some
> explanation of what HV expects.
>
I do agree this is not intuitive, but current hypervisor implementation
requires "blocked == 1" to be paired with "translation_enabled = 1",
otherwise it returns HV_STATUS_INVALID_PARAMETER. But I can add some
comment at least.
Thanks for the thorough review, Jacob!
B.R.
Yu
