[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


On 18/01/18 17:25, Manish Jaggi wrote:

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.


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".
I stressed this here as this function is only called for hardware domain, if you see line 1550

So why are you using the word Dom0 everywhere else? I keep asking you to use the term "Hardware Domain".

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

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.

I read properly the commit log... and my point still stands. This function is name vgic_map_translation_space. How a developer would guess that it can't be used for other domain than the hardware domain and direct mapped? More that you pass a domain pointer...

   * 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;
                  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.

This is not difficult to check error return in a first draft. Please stop sending patch that are full of coding style error or half done (without even mentioning it).

This is rather annoying and a way to miss that afterwards.


Julien Grall

Xen-devel mailing list



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