|
[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
Hi,
On 22/03/17 17:29, Julien Grall wrote:
> Hi Andre,
>
> On 16/03/17 11:20, Andre Przywara wrote:
>> The ITS uses device IDs to map LPIs to a device. Dom0 will later use
>> those IDs, which we directly pass on to the host.
>> For this we have to map each device that Dom0 may request to a host
>> ITS device with the same identifier.
>> Allocate the respective memory and enter each device into an rbtree to
>> later be able to iterate over it or to easily teardown guests.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> xen/arch/arm/gic-v3-its.c | 207
>> +++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/vgic-v3.c | 3 +
>> xen/include/asm-arm/domain.h | 3 +
>> xen/include/asm-arm/gic_v3_its.h | 18 ++++
>> 4 files changed, 231 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 5c11b0d..60b15b5 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -21,6 +21,8 @@
>> #include <xen/lib.h>
>> #include <xen/delay.h>
>> #include <xen/mm.h>
>> +#include <xen/rbtree.h>
>> +#include <xen/sched.h>
>> #include <xen/sizes.h>
>> #include <asm/gic.h>
>> #include <asm/gic_v3_defs.h>
>> @@ -32,6 +34,17 @@
>>
>> LIST_HEAD(host_its_list);
>>
>> +struct its_devices {
>> + struct rb_node rbnode;
>> + struct host_its *hw_its;
>> + void *itt_addr;
>> + paddr_t guest_doorbell;
>
> I think it would be worth to explain in the commit message why you need
> the guest_doorbell in the struct its_devices and how you plan to use it.
In v4 I now also elaborated on the reason in a comment (before the
struct), which I deem more useful than something in the commit message.
>> + uint32_t host_devid;
>> + uint32_t guest_devid;
>> + uint32_t eventids;
>> + uint32_t *host_lpis;
>> +};
>> +
>> bool gicv3_its_host_has_its(void)
>> {
>> return !list_empty(&host_its_list);
>> @@ -149,6 +162,24 @@ static int its_send_cmd_mapc(struct host_its
>> *its, uint32_t collection_id,
>> return its_send_command(its, cmd);
>> }
>>
>> +static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
>> + uint8_t size_bits, paddr_t itt_addr,
>> bool valid)
>> +{
>> + uint64_t cmd[4];
>> +
>> + if ( valid )
>> + {
>> + ASSERT(size_bits < 32);
>
> It would be better if you do the check against the real number in
> hardware (i.e GITS_TYPER.ID_bits).
Added in v4.
>
>> + ASSERT(!(itt_addr & ~GENMASK(51, 8)));
>> + }
>> + cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
>> + cmd[1] = valid ? size_bits : 0x00;
>
> This is really confusing. The check was not on the previous version. So
> why do you need that?
Admittedly I was taken away be some intention to check this here
properly. But since itt_addr and size are only valid with V=1, I removed
this in v3.
> Also, it would have been better to hide the "size - 1" in the helper
> avoiding to really on the caller to do the right thing.
I tend to agree, but then we have the awkward case where an unmap passes
0 in size, which then gets decremented by one. But you are right that
it's still saner this way, so I pass 1 now in the unmap call and do the
"-1" encoding in here.
>> + cmd[2] = valid ? (itt_addr | GITS_VALID_BIT) : 0x00;
>
> Ditto about "valid? ...".
Removed in v3.
> [...]
>
>> +static struct host_its *gicv3_its_find_by_doorbell(paddr_t
>> doorbell_address)
>> +{
>> + struct host_its *hw_its;
>> +
>> + list_for_each_entry(hw_its, &host_its_list, entry)
>> + {
>> + if ( hw_its->addr + ITS_DOORBELL_OFFSET == doorbell_address )
>
> Why not storing the ITS address rather than the doorbell to avoid this
> check?
Because the doorbell address is a nice architectural property of MSIs in
general. And we need this check anyway, it's just the addition of the
doorbell offset that is different.
> [...]
>
>> +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.
>> + 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.
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?
>
> [...]
>
>> +/* 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.
> 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?
>> + */
>> +restart:
>> + spin_lock(&d->arch.vgic.its_devices_lock);
>> + if ( (victim = rb_first(&d->arch.vgic.its_devices)) )
>> + {
>> + dev = rb_entry(victim, struct its_devices, rbnode);
>> + rb_erase(victim, &d->arch.vgic.its_devices);
>> +
>> + spin_unlock(&d->arch.vgic.its_devices_lock);
>> +
>> + remove_mapped_guest_device(dev);
>> +
>> + goto restart;
>> + }
>> +
>> + spin_unlock(&d->arch.vgic.its_devices_lock);
>> +}
>> +
>> /* Scan the DT for any ITS nodes and create a list of host ITSes out
>> of it. */
>> void gicv3_its_dt_init(const struct dt_device_node *node)
>> {
>> @@ -455,6 +661,7 @@ void gicv3_its_dt_init(const struct dt_device_node
>> *node)
>> its_data->addr = addr;
>> its_data->size = size;
>> its_data->dt_node = its;
>> + spin_lock_init(&its_data->cmd_lock);
>
> This should be in patch #5.
Done in v4.
>>
>> 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.
Cheers,
Andre.
> I think it would make sense to get a patch that introduces a skeleton
> for the vITS before this patch and start plumbing through.
>
>> +
>> /*
>> * Domain 0 gets the hardware address.
>> * Guests get the virtual platform layout.
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |