[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 06/27] ARM: GICv3 ITS: introduce device mapping
On 04/03/2017 09:08 PM, Andre Przywara wrote:
Hi,
Hi Andre,
On 22/03/17 17:29, Julien Grall wrote:
+int gicv3_its_map_guest_device(struct domain *d,
+ paddr_t host_doorbell, uint32_t
host_devid,
+ paddr_t guest_doorbell, uint32_t
guest_devid,
+ uint32_t nr_events, bool valid)
+{
+ void *itt_addr = NULL;
+ struct host_its *hw_its;
+ struct its_devices *dev = NULL, *temp;
+ struct rb_node **new = &d->arch.vgic.its_devices.rb_node, *parent
= NULL;
+ int ret = -ENOENT;
+
+ hw_its = gicv3_its_find_by_doorbell(host_doorbell);
+ if ( !hw_its )
+ return ret;
+
+ /* check for already existing mappings */
+ spin_lock(&d->arch.vgic.its_devices_lock);
+ while ( *new )
+ {
+ temp = rb_entry(*new, struct its_devices, rbnode);
+
+ parent = *new;
+ if ( !compare_its_guest_devices(temp, guest_doorbell,
guest_devid) )
+ {
+ if ( !valid )
+ rb_erase(&temp->rbnode, &d->arch.vgic.its_devices);
+
+ spin_unlock(&d->arch.vgic.its_devices_lock);
+
+ if ( valid )
Again, a printk(XENLOG_GUEST...) here would be useful to know which host
DeviceID was associated to the guest DeviceID.
I added a gprintk(XENLOG_DEBUG, ), which I think is more appropriate (as
it may spam the console when some stupid guest is running). Let me know
if you want to have a different loglevel.
I don't think this is more appropriate. gprintk will print the domain ID
of the current domain, whilst this function will be called by the
toolstack in the future.
Furthemore, if you look at the implementation of gprintk you will notice
that it is basically turning into printk(XENLOG_GUEST...) and adding
information of the current vCPU.
What matters for ratelimiting is XENLOG_GUEST.
+ return -EBUSY;
+
+ return remove_mapped_guest_device(temp);
Just above you removed the device from the RB-tree but this function may
fail and never free the memory. This means that memory will be leaked
leading to a potential denial of service.
So I fixed this case in v4, though there is still a tiny chance of a
memleak: if the MAPD(V=0) command fails. We can't free the ITT table
then, really, because it still belongs to the ITS. I don't think we can
do much about it, though.
This is a leak and even tiny is quite worrying. How do you plan to
address this in the future? What is the best thing to do?
I free the other allocations of the memory now, anyway.
+ }
+
+ if ( compare_its_guest_devices(temp, guest_doorbell,
guest_devid) > 0 )
+ new = &((*new)->rb_left);
+ else
+ new = &((*new)->rb_right);
+ }
+
+ if ( !valid )
+ goto out_unlock;
+
+ ret = -ENOMEM;
+
+ /* An Interrupt Translation Table needs to be 256-byte aligned. */
+ itt_addr = _xzalloc(nr_events * hw_its->itte_size, 256);
+ if ( !itt_addr )
+ goto out_unlock;
+
+ dev = xzalloc(struct its_devices);
+ if ( !dev )
+ goto out_unlock;
+
+ ret = its_send_cmd_mapd(hw_its, host_devid, fls(nr_events - 1) - 1,
I don't understand why nr_events - 1. Can you explain?
Xen lacks an ilog2, so "fls" is the closest I could find. "fls" has this
slightly weird semantic (from the Linux source):
"Note fls(0) = 0, fls(1) = 1, fls(0x80000000) = 32."
I think this translates into: "How many bits do I need to express this
number?". For our case the highest event number we need to encode is
"nr_events - 1", hence the subtraction.
So is it worth to introduce a:
static inline int ilog2_64(uint64_t n) ...
in xen/include/asm-arm/bitops.h to document this?
This might make easier to read the code.
[...]
+/* Removing any connections a domain had to any ITS in the system. */
+void gicv3_its_unmap_all_devices(struct domain *d)
+{
+ struct rb_node *victim;
+ struct its_devices *dev;
+
+ /*
+ * This is an easily readable, yet inefficient implementation.
+ * It uses the provided iteration wrapper and erases each node,
which
+ * possibly triggers rebalancing.
+ * This seems overkill since we are going to abolish the whole
tree, but
+ * avoids an open-coded re-implementation of the traversal
functions with
+ * some recursive function calls.
+ * Performance does not matter here, since we are destroying a
domain.
Again, this is slightly untrue. Performance matter when destroying a
domain as Xen cannot be preempted. So if it takes too long, you will
have an impact on the overall system.
I reworded this sentence in v3, since you apparently misunderstood me.
By inefficient I meant sub-optimal, but this is not a _critical_ path,
so we don't care too much. The execution time is clearly bounded by the
number of devices. We simply shouldn't allow gazillion of devices on a
DomU if we care about those things.
This is a very naive way of thinking how domain destruction is working
on Xen. Domain destruction is likely to get time, you have to release
all the resources (memory, devices...). So we usually split in smaller
pieces to be able to handle interrupts or allowing preemption.
However, I think it would be fair to assume that all device will be
deassigned before the ITS is destroyed. So I would just drop this
function. Note that we have the same assumption in the SMMU driver.
Well, why not keep it as a safeguard, then? If a domain gets destroyed,
aren't we supposed to clean up?
See above. We use ASSERT(....) in the SMMU code to check this assumption.
printk("GICv3: Found ITS @0x%lx\n", addr);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index d61479d..1fadb00 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1450,6 +1450,9 @@ static int vgic_v3_domain_init(struct domain *d)
d->arch.vgic.nr_regions = rdist_count;
d->arch.vgic.rdist_regions = rdist_regions;
+ spin_lock_init(&d->arch.vgic.its_devices_lock);
+ d->arch.vgic.its_devices = RB_ROOT;
Again, the placement of those 2 lines are likely wrong. This should
belong to the vITS and not the vgic-v3.
Well, it's a "domain which has an ITS" thing, as this is not per ITS. So
far we only have an per-ITS init function, as we don't iterate over the
host ITSes there anymore.
So I refrained from adding a separate function to initialize these
simple and generic data structures. Please let me know if you insist on
some ITS-per-domain init function.
Where is the limit to refrain from creating function then? What matter
is logic...
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
