Hi,
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, mja...@caviumnetworks.com wrote:
From: Manish Jaggi <mja...@caviumnetworks.com>
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
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.
ok
Signed-off-by: Manish Jaggi <manish.ja...@cavium.com>
---
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;
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.
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.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
