[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate...



On 30.07.2019 15:44, Paul Durrant wrote:
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1531,8 +1531,7 @@ int p2m_init(struct domain *d)
       * shared with the CPU, Xen has to make sure that the PT changes have
       * reached the memory
       */
-    p2m->clean_pte = iommu_enabled &&
-        !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
+    p2m->clean_pte = !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);

I can't tell if the original code was meant to be this way, but I'm
afraid your transformation is not correct: The prior construct,
expanding iommu_has_feature(), was

iommu_enabled && !(iommu_enabled && test_bit(feature, dom_iommu(d)->features))

which transforms through

iommu_enabled && (!iommu_enabled || !test_bit(feature, dom_iommu(d)->features))

to

(iommu_enabled && !iommu_enabled) || (iommu_enabled && !test_bit(feature, 
dom_iommu(d)->features))

and hence

iommu_enabled && !test_bit(feature, dom_iommu(d)->features)

whereas the new code simply is

!(iommu_enabled && test_bit(feature, dom_iommu(d)->features))

i.e.

!iommu_enabled || !test_bit(feature, dom_iommu(d)->features)

@@ -766,7 +766,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
          new_entry.sp = !!i;
          new_entry.sa_p2mt = p2mt;
          new_entry.access = p2ma;
-        new_entry.snp = (iommu_enabled && iommu_snoop);
+        new_entry.snp = is_iommu_enabled(p2m->domain) && iommu_snoop;

Please use d here.

Seeing that this is the last change in x86/mm/, did you overlook
the use in p2m_pt_set_entry()? Or is this meant to go on top of
Alexandru's "x86/mm: Clean IOMMU flags from p2m-pt code" (which
should then be noted in a post-commit-message remark)?

@@ -556,7 +556,7 @@ int iommu_do_domctl(
  {
      int ret = -ENODEV;
- if ( !iommu_enabled )
+    if ( !is_iommu_enabled(d) )
          return -ENOSYS;

ENOSYS was wrong here from the beginning, but it certainly gets
worse with this no longer being a system wide property. Please
change to EOPNOTSUPP or some other suitable one.

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -864,7 +864,7 @@ static int pci_clean_dpci_irqs(struct domain *d)

Above here there's another use in pci_enable_acs(), which should
imo act on pdev->domain.

There's another use in flask_iommu_resource_use_perm(). All
callers of the function hold a struct domain * in their hands,
which I think they should pass into this function such that the
conditional can be replaced.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.