|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 12/22] xen/arm: ITS: Add GICR register emulation
Hi Vijay,
On 27/07/15 12:11, vijay.kilari@xxxxxxxxx wrote:
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
[..]
> +static int gicv3_dist_supports_lpis(void)
> +{
> + return readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_LPIS_SUPPORTED;
> +}
> +
> static int __cpuinit gicv3_cpu_init(void)
> {
> int i;
> @@ -1274,6 +1279,11 @@ static int __init gicv3_init(void)
>
> spin_lock(&gicv3.lock);
>
> + if ( gicv3_dist_supports_lpis() )
> + gicv3_info.lpi_supported = 1;
> + else
> + gicv3_info.lpi_supported = 0;
> +
You will avoid 3 lines if you do:
gicv3_info.lpi_supported = !!gicv3_dist_supports_lpis();
> gicv3_dist_init();
> res = gicv3_cpu_init();
> gicv3_hyp_init();
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 1757193..af8a34b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -67,6 +67,11 @@ unsigned int gic_number_lines(void)
> return gic_hw_ops->info->nr_lines;
> }
>
> +bool_t gic_lpi_supported(void)
> +{
> + return gic_hw_ops->info->lpi_supported;
> +}
> +
> void gic_save_state(struct vcpu *v)
> {
> ASSERT(!local_irq_is_enabled());
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 1c7d9b6..4afb62b 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
Can you explain why the emulation of the LPI property table has to be
done in the vITS code?
Overall, there is nothing vITS specific in this code and all the
functions you've introduced within this file are called only by the
vgic-v3 code.
> @@ -28,6 +28,7 @@
> #include <asm/io.h>
> #include <asm/gic_v3_defs.h>
> #include <asm/gic.h>
> +#include <asm/gic-its.h>
> #include <asm/vgic.h>
> #include <asm/gic-its.h>
> #include <asm/atomic.h>
> @@ -76,6 +77,34 @@ static inline uint32_t vits_get_max_collections(struct
> domain *d)
> return (d->max_vcpus + 1);
> }
>
> +static void vits_disable_lpi(struct vcpu *v, uint32_t vlpi)
> +{
> + struct pending_irq *p;
> +
> + p = irq_to_pending(v, vlpi);
> + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> + gic_remove_from_queues(v, vlpi);
> +}
> +
> +static void vits_enable_lpi(struct vcpu *v, uint32_t vlpi, uint8_t priority)
> +{
> + struct pending_irq *p;
> + unsigned long flags;
> +
> + p = irq_to_pending(v, vlpi);
> +
> + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +
> + spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> + /*XXX: raise on right vcpu */
As said on the previous versions, I think there will be locking issue
given that pending_irq structure is protected by the vCPU where the IRQ
is locked.
If you think it's not the case please explain why but don't leave the
question unanswered.
> + if ( !list_empty(&p->inflight) &&
> + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> + gic_raise_guest_irq(v, vlpi, p->priority);
> +
> + spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +}
> +
> static int vits_access_guest_table(struct domain *d, paddr_t entry, void
> *addr,
> uint32_t size, bool_t set)
> {
> @@ -551,6 +580,145 @@ static inline uint32_t vits_get_word(uint32_t
> reg_offset, uint64_t val)
> return (u32)(val >> 32);
> }
>
> +static int vgic_v3_gits_lpi_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> + uint32_t offset;
> + struct vgic_its *vits = v->domain->arch.vgic.vits;
> + struct hsr_dabt dabt = info->dabt;
> + struct cpu_user_regs *regs = guest_cpu_user_regs();
> + register_t *r = select_user_reg(regs, dabt.reg);
> +
> + offset = info->gpa - (vits->propbase & MASK_4K);
> +
> + DPRINTK("%pv: vITS: LPI Table read offset 0x%"PRIx32"\n", v, offset);
> + spin_lock(&vits->prop_lock);
There is a potential deadlock with this lock here and the one in
vits_get_priority (see patch #16):
- The spin lock is taken in vgic_v3_gits_lpi_mmio_read
<==== LPI received on the same CPU
- vits_get_priority try to get the lock
=> Not possible because already locked
This could be avoid by disabling the IRQ when taking the spin lock and
reenable it when you release it.
> + if ( dabt.size == DABT_DOUBLE_WORD )
Please use a switch rather than if/else if/else if... and assuming the
last else the 8bit reading.
> + *r = *((u64*)vits->prop_page + offset);
> + else if (dabt.size == DABT_WORD )
> + *r = *((u32*)vits->prop_page + offset);
> + else if (dabt.size == DABT_HALF_WORD )
> + *r = *((u16*)vits->prop_page + offset);
The returned value won't be correct for 64, 32 and 16 bit read.
The cast is only applied to vits->prop_page, therefore the pointer will
be increment by offset * sizeof(uN).
You need to do the arithmetic on the pointer and then doing the cast.
> + else
> + *r = *((u8*)vits->prop_page + offset);
> + spin_unlock(&vits->prop_lock);
> +
> + return 1;
> +}
> +
> +static int vgic_v3_gits_lpi_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> + uint32_t offset, vid;
> + uint8_t cfg, *p, i, iter;
> + bool_t enable;
> + struct vgic_its *vits = v->domain->arch.vgic.vits;
> + struct hsr_dabt dabt = info->dabt;
> + struct cpu_user_regs *regs = guest_cpu_user_regs();
> + register_t *r = select_user_reg(regs, dabt.reg);
> +
> + offset = info->gpa - (vits->propbase & MASK_4K);
> +
> + DPRINTK("%pv: vITS: LPI Table write offset 0x%"PRIx32"\n", v, offset);
> +
> + if ( dabt.size == DABT_DOUBLE_WORD )
Please use a switch rather than if/else if/else if...
> + iter = 8;
> + else if ( dabt.size == DABT_WORD )
> + iter = 4;
> + else if ( dabt.size == DABT_HALF_WORD )
> + iter = 2;
> + else
> + iter = 1;
> +
> + spin_lock(&vits->prop_lock);
Same remark as the previous spin_lock.
> + p = ((u8*)vits->prop_page + offset);
> +
> + vid = offset + FIRST_GIC_LPI;
> + for ( i = 0 ; i < iter; i++ )
> + {
> + cfg = *p;
> + enable = (cfg & *r) & 0x1;
As said on v4, please use a define rather than 0x1. It would have been
more clear that we are checking the enable bit.
> +
> + if ( !enable )
> + vits_enable_lpi(v, vid, (*r & LPI_PRIORITY_MASK));
> + else
> + vits_disable_lpi(v, vid);
> +
> + /* Update virtual prop page */
> + *p = (*r & 0xff);
> + p++;
> + vid++;
> + }
> + spin_unlock(&vits->prop_lock);
> +
> + return 1;
> +}
> +
> +static const struct mmio_handler_ops vgic_gits_lpi_mmio_handler = {
> + .read_handler = vgic_v3_gits_lpi_mmio_read,
> + .write_handler = vgic_v3_gits_lpi_mmio_write,
> +};
> +
> +int vits_map_lpi_prop(struct domain *d)
> +{
> + struct vgic_its *vits = d->arch.vgic.vits;
> + paddr_t gaddr, addr;
> + unsigned long mfn;
> + uint32_t lpi_size, id_bits;
> + int i;
> +
> + gaddr = vits->propbase & MASK_4K;
The physical address is only bits [47:12]. The uppers are either RES0 or
used for the OuterCache.
> + id_bits = ((vits->propbase & GICR_PROPBASER_IDBITS_MASK) + 1);
> +
> + if ( id_bits > d->arch.vgic.id_bits )
> + id_bits = d->arch.vgic.id_bits;
As said on v4, you are allowing the possibility to have a smaller
property table than the effective number of LPIs.
An ASSERT in vits_get_priority (patch #16) doesn't ensure the validity
of the size of the property table provided by the guest. This will
surely crash Xen in debug mode, and who knows what will happen in
production mode.
> +
> + lpi_size = 1UL << id_bits;
> +
> + DPRINTK("vITS:Map LPI conf table @ 0x%"PRIx64" size 0x%"PRIx32"\n",
Missing space after ":"
s/PRIx64/PRIpaddr/
> + gaddr, lpi_size);
> +
> + vits->prop_size = lpi_size;
> + /* Allocate Virtual LPI Property table */
> + /* TODO: To re-use guest property table? */
> + vits->prop_page = alloc_xenheap_pages(get_order_from_bytes(lpi_size), 0);
> + if ( !vits->prop_page )
> + {
> + dprintk(XENLOG_G_ERR, "%pv: vITS: Fail to allocate LPI Prop page\n",
> current);
I think you should use d->domain_id and only print the domain id for
more consistency with the function rather than current.
> + return 0;
> + }
> +
> + addr = gaddr;
> + for ( i = 0; i < lpi_size / PAGE_SIZE; i++ )
> + {
> + vits_access_guest_table(d, addr,
> + (void *)(vits->prop_page + i * PAGE_SIZE),
> + PAGE_SIZE, 0);
> + addr += PAGE_SIZE;
As said on v4, it's good to copy the value from the guest memory to our
internal table. But there is no point to do it if you don't replicate
the value in the internal LPIs. I.e enable/disable the LPIs when
necessary....
Note that it may be a long process. So either you correctly handle the
copy or you don't do it at all. But avoid to do something in the middle,
because it will break the state in Xen.
> + }
> +
> + /*
> + * Each re-distributor shares a common LPI configuration table
> + * So one set of mmio handlers to manage configuration table is enough
> + */
> + addr = gaddr;
> + for ( i = 0; i < lpi_size / PAGE_SIZE; i++ )
> + {
> + mfn = gmfn_to_mfn(d, paddr_to_pfn(addr));
> + if ( unlikely(!mfn_valid(mfn)) )
> + {
> + dprintk(XENLOG_G_ERR, "%pv: vITS: Invalid propbaser address\n",
> current);
> + return 0;
> + }
> + guest_physmap_remove_page(d, paddr_to_pfn(addr), mfn, 0);
> + addr += PAGE_SIZE;
> + }
> +
> + /* Register mmio handlers for this region */
> + register_mmio_handler(d, &vgic_gits_lpi_mmio_handler,
> + gaddr, lpi_size);
> +
> + return 1;
> +}
> +
> static inline void vits_spin_lock(struct vgic_its *vits)
> {
> spin_lock(&vits->lock);
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 683e3cc..a466a8f 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -29,6 +29,8 @@
> #include <asm/current.h>
> #include <asm/mmio.h>
> #include <asm/gic_v3_defs.h>
> +#include <asm/gic.h>
> +#include <asm/gic-its.h>
> #include <asm/vgic.h>
>
> /* GICD_PIDRn register values for ARM implementations */
> @@ -109,29 +111,47 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu
> *v, mmio_info_t *info,
> struct hsr_dabt dabt = info->dabt;
> struct cpu_user_regs *regs = guest_cpu_user_regs();
> register_t *r = select_user_reg(regs, dabt.reg);
> - uint64_t aff;
> + uint64_t aff, val;
>
> switch ( gicr_reg )
> {
> case GICR_CTLR:
> - /* We have not implemented LPI's, read zero */
> - goto read_as_zero_32;
> + if ( dabt.size != DABT_WORD ) goto bad_width;
> + vgic_lock(v);
> + *r = v->domain->arch.vgic.gicr_ctlr;
> + vgic_unlock(v);
> + return 1;
> case GICR_IIDR:
> if ( dabt.size != DABT_WORD ) goto bad_width;
> *r = GICV3_GICR_IIDR_VAL;
> return 1;
> case GICR_TYPER:
> - if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> - /* TBD: Update processor id in [23:8] when ITS support is added */
> + case GICR_TYPER + 4:
Why did you introduce it only in v5? This change is not related to the
LPI but a correct implementation of access size on GICv3 register.
Overall, I think this should be done in a separate patch for all the
registers and not only this one. It would make the code change less
complicate to read.
Please fix the write version of TYPER which happen to be 64 bits only too.
> + if ( dabt.size != DABT_DOUBLE_WORD && dabt.size != DABT_WORD)
> + goto bad_width;
> aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
> MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
> MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
> MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
> - *r = aff;
> -
> + val = aff;
> + if ( gic_lpi_supported() )
The registers/values used only for LPIs should only be exposed for guest
using ITS. I.e only for DOM0.
Please introduce an helpers vgic_is_lpi_supported(d) which return true
only when the ITS is in-use.
In general seen direct call to the hardware in the vgic is a wrong
things. The vGIC could should be pretty much hardware agnostic.
> + {
> + /* Set Physical LPIs support */
> + val |= GICR_TYPER_PLPIS;
> + /* GITS_TYPER.PTA is 0. Provide vcpu number as target address */
Well this is not specific to the ITS neither LPIs. This could be used by
the guest is we ever happen to support GICv2 on GICv3. Therefore this
should be outside the if.
[..]
> @@ -245,10 +286,37 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu
> *v, mmio_info_t *info,
> /* LPI is not implemented */
> goto write_ignore_64;
> case GICR_PROPBASER:
> - /* LPI is not implemented */
> + if ( gic_lpi_supported() )
> + {
> + if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> + vgic_lock(v);
> + /* LPI configuration tables are shared across cpus. Should be
> same */
> + /*
> + * Allow updating on propbase only once with below check.
> + * TODO: Manage change in property table.
> + */
> + if ( v->domain->arch.vgic.vits->propbase != 0 )
> + {
> + printk(XENLOG_G_WARNING
> + "%pv: vGICR: Updating LPI propbase is not allowed\n",
> v);
Hmmmm, AFAICT this warning will be printed even in valid case. i.e for
every VCPUs but VCPU0 setting the propbaser.
This warning should only be printed when it's necessary to avoid
question from user about false positive.
> + vgic_unlock(v);
> + return 1;
> + }
> + v->domain->arch.vgic.vits->propbase = *r;
> + vgic_unlock(v);
> + return vits_map_lpi_prop(v->domain);
> + }
> goto write_ignore_64;
> case GICR_PENDBASER:
> - /* LPI is not implemented */
> + if ( gic_lpi_supported() )
> + {
> + /* Just hold pendbaser value for guest read */
> + if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> + vgic_lock(v);
> + v->domain->arch.vgic.vits->pendbase[v->vcpu_id] = *r;
If you store the value, you should at least implementing correctly for
the return (see 8.11.18 ARM IHI 0069A).
> + vgic_unlock(v);
> + return 1;
> + }
> goto write_ignore_64;
> case GICR_INVLPIR:
> /* LPI is not implemented */
> @@ -715,6 +783,14 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v,
> mmio_info_t *info)
> *r = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT |
> DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32));
>
> + if ( gic_lpi_supported() )
> + {
> + irq_bits = v->domain->arch.vgic.id_bits;
> + *r |= GICD_TYPE_LPIS;
> + }
> + else
> + irq_bits = get_count_order(vgic_num_irqs(v->domain));
Why do you do a specific case when the lpi is not supported? The id_bits
ought to be valid for both case.
> *r |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
>
> return 1;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 986a4d6..c4cf65e 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -93,6 +93,8 @@ struct arch_domain
> spinlock_t lock;
> int ctlr;
> int nr_spis; /* Number of SPIs */
> + int nr_lpis; /* Number of LPIs */
> + int id_bits;
Please add a comment explaing id_bits.
> unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
> struct vgic_irq_rank *shared_irqs;
> /*
> @@ -106,6 +108,7 @@ struct arch_domain
> #ifdef HAS_GICV3
> /* Virtual ITS */
> struct vgic_its *vits;
> + int gicr_ctlr;
> /* GIC V3 addressing */
> /* List of contiguous occupied by the redistributors */
> struct vgic_rdist_region {
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 0854cde..30316fd 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -116,6 +116,7 @@ extern unsigned int num_of_lpis;
>
> #define LPI_PROP_ENABLED (1 << 0)
> #define LPI_PROP_GROUP1 (1 << 1)
> +#define LPI_PRIORITY_MASK (0xfc)
>
> /*
> * Collection structure - just an ID, and a redistributor address to
> @@ -147,6 +148,16 @@ struct vgic_its
> paddr_t gits_base;
> /* GICR ctrl register */
> uint32_t ctrl;
> + /* LPI propbase */
> + paddr_t propbase;
> + /* percpu pendbase */
> + paddr_t pendbase[MAX_VIRT_CPUS];
Please create a field in arch_vcpu.vgic structure rather than having a
static array in arch_domain.
> + /* Virtual LPI property table */
> + void *prop_page;
> + /* Virtual LPI property size */
> + uint64_t prop_size;
Please use uint32_t here, the size is only encoding on 32 bits.
> + /* spinlock to protect lpi property table */
> + spinlock_t prop_lock;
> /* vITT device table ipa */
> paddr_t dt_ipa;
> /* vITT device table size */
> @@ -344,6 +355,7 @@ int vits_get_vitt_entry(struct domain *d, uint32_t devid,
> int vits_get_vdevice_entry(struct domain *d, uint32_t devid,
> struct vdevice_table *entry);
> void vits_setup_hw(struct gic_its_info *info);
> +int vits_map_lpi_prop(struct domain *d);
>
> #endif /* __ASM_ARM_GIC_ITS_H__ */
> /*
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 5f37f56..a9a5874 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -97,6 +97,7 @@
> #define GICD_TYPE_CPUS_SHIFT 5
> #define GICD_TYPE_CPUS 0x0e0
> #define GICD_TYPE_SEC 0x400
> +#define GICD_TYPE_LPIS (0x1UL << 17)
This field is GICv3 specific. Therefore this should go in gic_v3_defs.h.
>
> #define GICC_CTL_ENABLE 0x1
> #define GICC_CTL_EOI (0x1 << 9)
> @@ -283,6 +284,8 @@ extern void gic_dump_info(struct vcpu *v);
>
> /* Number of interrupt lines */
> extern unsigned int gic_number_lines(void);
> +/* LPI support info */
> +bool_t gic_lpi_supported(void);
I'm not convinced that this new function in the common GIC is not
necesarry. At least not within this patch. If you appear to need it in
another patch please introduce it there.
> /* IRQ translation function for the device tree */
> int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
> @@ -300,6 +303,8 @@ struct gic_info {
> unsigned int maintenance_irq;
> /* Pointer to the device tree node representing the interrupt controller
> */
> const struct dt_device_node *node;
> + /* LPIs are support information */
What do you mean?
> + bool_t lpi_supported;
> };
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |