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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 01/18/2018 10:22 PM, Julien Grall wrote:

Hi Manish,

Hi Julien,

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.

ok

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".

I stressed this here as this function is only called for hardware domain, if you see line 1550

if ( is_hardware_domain(d) ) is checked.

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.

ok

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.

ok I will change the name.

+{
+    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.

I think you didnt read the commit log properly, this function is *only* called for hardware domain. So there is no question of DomU.
Request you to please see the code again.

int vgic_v3_its_init_domain(struct domain *d)
{
 ...
    if ( is_hardware_domain(d) )
    {
        struct host_its *hw_its;

        list_for_each_entry(hw_its, &host_its_list, entry)
        {
 	   ...
            vgic_map_translation_space(d, hw_its->addr);
        }
    }
...
}

+
+    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.

yes, I thought we will focus more is this is the right place to add this test code.

          }
      }
 

Cheers,

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