[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v5 28/30] ARM: vITS: create and initialize virtual ITSes for Dom0
Hi Andre,
On 06/04/17 00:19, Andre Przywara wrote:
For each hardware ITS create and initialize a virtual ITS for Dom0.
We use the same memory mapped address to keep the doorbell working.
This introduces a function to initialize a virtual ITS.
We maintain a list of virtual ITSes, at the moment for the only
purpose of later being able to free them again.
We advertise 24 bits worth of LPIs on the guest side, using the full
32 bits seems to trigger a Linux bug (to be investigated).
No, we don't emulate a specific number just because an OS is buggy
without a minimum of investigation.
But as I said, the vITS emulation for DOM0 should follow the host vITS
in term of number. If the hardware expose N bits, then it should be N
bits for the vITS.
If there is a Linux bug with 32 bits on Xen, either this bug is present
on the hardware too or it is something else.
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
xen/arch/arm/vgic-v3-its.c | 46 ++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/vgic-v3.c | 17 +++++++++++++++
xen/include/asm-arm/domain.h | 2 ++
xen/include/asm-arm/gic_v3_its.h | 12 +++++++++++
4 files changed, 77 insertions(+)
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 9684b3a..4e66cad 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -42,6 +42,7 @@
*/
struct virt_its {
struct domain *d;
+ struct list_head vits_list;
paddr_t doorbell_address;
unsigned int devid_bits;
unsigned int intid_bits;
@@ -72,12 +73,20 @@ struct vits_itte
void vgic_v3_its_init_domain(struct domain *d)
{
+ INIT_LIST_HEAD(&d->arch.vgic.vits_list);
spin_lock_init(&d->arch.vgic.its_devices_lock);
d->arch.vgic.its_devices = RB_ROOT;
}
void vgic_v3_its_free_domain(struct domain *d)
{
+ struct virt_its *pos, *temp;
+
+ list_for_each_entry_safe( pos, temp, &d->arch.vgic.vits_list, vits_list )
+ {
+ list_del(&pos->vits_list);
+ xfree(pos);
+ }
NIT: newline here.
ASSERT(RB_EMPTY_ROOT(&d->arch.vgic.its_devices));
}
@@ -1157,6 +1166,43 @@ static const struct mmio_handler_ops
vgic_its_mmio_handler = {
.write = vgic_v3_its_mmio_write,
};
+int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
+ unsigned int devid_bits, unsigned int intid_bits)
+{
+ struct virt_its *its;
+ uint64_t base_attr;
+
+ its = xzalloc(struct virt_its);
+ if ( !its )
+ return -ENOMEM;
+
+ base_attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
+ base_attr |= GIC_BASER_CACHE_SameAsInner <<
GITS_BASER_OUTER_CACHEABILITY_SHIFT;
+ base_attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
+
+ its->cbaser = base_attr;
+ base_attr |= 0ULL << GITS_BASER_PAGE_SIZE_SHIFT; /* 4K pages */
+ its->baser_dev = GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
+ its->baser_dev |= (sizeof(uint64_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
Why uint64_t? I know this is the size of the ITT, but my concern is you
spread this size of a bit everywhere in the code which will make very
difficult to update it.
If you have introduced a struct it would have been easier to update the
size layout.
So please find a way to minimize the number of uint64_t over the code.
+ its->baser_dev |= base_attr;
+ its->baser_coll = GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
+ its->baser_coll |= (sizeof(uint16_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
Same remark here.
+ its->baser_coll |= base_attr;
+ its->d = d;
+ its->doorbell_address = guest_addr + ITS_DOORBELL_OFFSET;
+ its->devid_bits = devid_bits;
+ its->intid_bits = intid_bits;
+ spin_lock_init(&its->vcmd_lock);
+ spin_lock_init(&its->its_lock);
I was expecting to some mapping for the doorbell in the IOMMU page
table. What is the plan to support SMMU and GICv3 ITS at the same time?
+
+ register_mmio_handler(d, &vgic_its_mmio_handler, guest_addr, SZ_64K, its);
You likely need to update mmio_count (see vgic_v3_init) when using ITS
so we can provide enough space the MMIO handler array to register all
the ITS.
+
+ /* Register the virtual ITSes to be able to clean them up later. */
+ list_add_tail(&its->vits_list, &d->arch.vgic.vits_list);
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index e6a33d0..3b01247 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -30,6 +30,7 @@
#include <asm/current.h>
#include <asm/mmio.h>
#include <asm/gic_v3_defs.h>
+#include <asm/gic_v3_its.h>
#include <asm/vgic.h>
#include <asm/vgic-emul.h>
#include <asm/vreg.h>
@@ -1569,6 +1570,7 @@ static int vgic_v3_domain_init(struct domain *d)
*/
if ( is_hardware_domain(d) )
{
+ struct host_its *hw_its;
unsigned int first_cpu = 0;
d->arch.vgic.dbase = vgic_v3_hw.dbase;
@@ -1594,6 +1596,21 @@ static int vgic_v3_domain_init(struct domain *d)
first_cpu += size / d->arch.vgic.rdist_stride;
}
+ d->arch.vgic.nr_regions = vgic_v3_hw.nr_rdist_regions;
Again, why this change? It is the 5 times I am saying this... It is
starting to be very annoying.
+
+ list_for_each_entry(hw_its, &host_its_list, entry)
This should go in vgic-v3-its.c (see discussion on patch #3).
+ {
+ /*
+ * For each host ITS create a virtual ITS using the same
+ * base and thus doorbell address.
+ * Use the same number of device ID bits as the host, and
+ * allow 24 bits for the interrupt ID.
See my comment above.
+ */
+ vgic_v3_its_init_virtual(d, hw_its->addr, hw_its->devid_bits, 24);
+
+ d->arch.vgic.has_its = true;
+ }
+
}
else
{
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index f993292..cbbfb99 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -115,6 +115,8 @@ struct arch_domain
spinlock_t its_devices_lock; /* Protects the its_devices tree */
struct radix_tree_root pend_lpi_tree; /* Stores struct pending_irq's */
rwlock_t pend_lpi_tree_lock; /* Protects the pend_lpi_tree */
+ struct list_head vits_list; /* List of virtual ITSes */
+ bool has_its;
#endif
} vgic;
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index daae143..1b8e47c 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -161,6 +161,10 @@ int gicv3_its_setup_collection(unsigned int cpu);
void vgic_v3_its_init_domain(struct domain *d);
void vgic_v3_its_free_domain(struct domain *d);
+/* Create and register a virtual ITS at the given guest address. */
+int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
+ unsigned int devid_bits, unsigned int intid_bits);
+
/*
* Map a device on the host by allocating an ITT on the host (ITS).
* "nr_event" specifies how many events (interrupts) this device will need.
@@ -237,6 +241,14 @@ static inline void vgic_v3_its_free_domain(struct domain
*d)
{
}
+static inline int vgic_v3_its_init_virtual(struct domain *d,
+ paddr_t guest_addr,
+ unsigned int devid_bits,
+ unsigned int intid_bits)
+{
+ return 0;
+}
+
#endif /* CONFIG_HAS_ITS */
#endif
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
