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

Re: [Xen-devel] [RFC PATCH] arm64: vgic-v3: Add ITS doorbell region in Dom0 stage-2



Hi Manish,

On 18/01/18 06:15, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
From: Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>

This patch introduces a function vgic_map_translation_space for mapping
ITS 64K translater space for doorbells in dom0 stage-2.
It is required as dom0 PCI devices behined SMMU  wont be able to write MSIs.

s/behined/behind/

also there a two spaces after SMMU. One should be enough.


This function is called from vgic_v3_its_init_domain only for hardware domain.

This is really confusing. You mix dom0 and hardware domain in the same commit. Please use only "hardware domain".


This patch is also required for testing
[RFC 00/11] acpi: arm: IORT Support for Xen [1]
+ [2]  [RFC v4 0/8] SMMUv3 driver
[1] https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00007.html
[2] https://lists.xen.org/archives/html/xen-devel/2017-12/msg01294.html

Anything after "This patch..." does not seem to be relevant in the commit. You might want to add that after --- so it get stripped after committing.


Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
---
  xen/arch/arm/vgic-v3-its.c | 26 ++++++++++++++++++++++++++
  1 file changed, 26 insertions(+)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 72a5c70656..a8f8079149 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1511,6 +1511,30 @@ unsigned int vgic_v3_its_count(const struct domain *d)
      return ret;
  }
+static int vgic_map_translation_space(struct domain *d, paddr_t guest_addr)

This function is quite confusing. What is guest_addr? It looks like it is the base address of the ITS but it is not very clear here.

+{
+    u64 addr, size;

This should be paddr_t.

+    int ret;

Newline here please.

+    addr = guest_addr + SZ_64K;
+    size = SZ_64K;
+
+    ret = map_mmio_regions(d,
+                           paddr_to_pfn(addr & PAGE_MASK),
+                           DIV_ROUND_UP(size, PAGE_SIZE),
+                           paddr_to_pfn(addr & PAGE_MASK));

You are assuming a 1:1 mapping in the domain. Nowhere you explain that assumption nor that it will not work for guest. At least you need an ASSERT(is_domain_direct_mapped(d)).

Furthermore, as we discussed yesterday I would expect a big fat comment that this need to be revisited when DomU support will be added as it may not be safe to map the Doorbell in page-tables used by the processor.

+
+    if ( ret )
+    {
+        printk(XENLOG_ERR "Unable to map to dom%d access to"
+               " 0x%"PRIx64" - 0x%"PRIx64"\n",
+               d->domain_id,
+               addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+    }
+
+    return ret;
+
+}
+
  /*
   * For a hardware domain, this will iterate over the host ITSes
   * and map one virtual ITS per host ITS at the same address.
@@ -1541,6 +1565,8 @@ int vgic_v3_its_init_domain(struct domain *d)
                  return ret;
              else
                  d->arch.vgic.has_its = true;
+
+            vgic_map_translation_space(d, hw_its->addr);
Your new function may return an error. Therefore you should test it.

          }
      }

Cheers,

--
Julien Grall

_______________________________________________
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®.