Hello Vijay,
On 19/03/2015 14:38, vijay.kil...@gmail.com wrote:
From: Vijaya Kumar K <vijaya.ku...@caviumnetworks.com>
Add support for emulating GITS_* registers
Signed-off-by: Vijaya Kumar K <vijaya.ku...@caviumnetworks.com>
---
v2: - Each Virtual ITS is attached to Physical ITS.
- Introduce helper function to lock and unlock
virtual ITS lock.
- Introduced helper to get virtual ITS structure pointer
based on emulation address.
---
xen/arch/arm/gic-v3-its.c | 8 +
xen/arch/arm/vgic-v3-its.c | 412 +++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/gic-its.h | 1 +
3 files changed, 421 insertions(+)
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index a9aab73..e382f8d 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -101,6 +101,8 @@ struct its_node {
};
uint32_t pta_type;
+/* Number of physical its nodes present */
+uint32_t nr_its = 0;
This variable is not exported so static.
Although, I'm not convinced this variable is useful. See my comments later.
#define ITS_ITT_ALIGN SZ_256
@@ -146,6 +148,11 @@ uint32_t its_get_pta_type(void)
return pta_type;
}
+uint32_t its_get_nr_its(void)
+{
+ return nr_its;
+}
+
struct its_node * its_get_phys_node(uint32_t dev_id)
{
struct its_node *its;
@@ -1170,6 +1177,7 @@ static int its_probe(struct dt_device_node *node)
}
spin_lock(&its_lock);
list_add(&its->entry, &its_nodes);
+ nr_its++;
spin_unlock(&its_lock);
return 0;
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 7530a88..4d8945f 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -869,6 +869,418 @@ err:
return 0;
}
+struct vgic_its *its_to_vits(struct vcpu *v, paddr_t phys_base)
This function is not exported so static.
Also, it looks like to me that the vcpu is not necessary. Please use
struct domain *d.
+{
+ struct vgic_its *vits = NULL;
+ int i;
+
+ /* Mask 64K offset */
+ phys_base = phys_base & ~(SZ_64K - 1);
+ if ( is_hardware_domain(v->domain) )
Why do you need to have a specific case for the hardware domain? All the
vITS code should be domain agnostic except the initialization function.
That would make the code a lot simpler.
+ {
+ for ( i = 0; i < its_get_nr_its(); i++ )
I would prefer if you introduce a new field in domain->arch to store the
number of ITS for the domain.
+ {
+ if ( v->domain->arch.vits[i].phys_base == phys_base )
+ {
+ vits = &v->domain->arch.vits[i];
+ break;
+ }
+ }
+ }
+ else
+ vits = &v->domain->arch.vits[0];
You should not assume that the guest as only one vITS.
+
+ return vits;
+}
+
+static inline void vits_spin_lock(struct vgic_its *vits)
+{
+ spin_lock(&vits->lock);
+}
+
+static inline void vits_spin_unlock(struct vgic_its *vits)
+{
+ spin_unlock(&vits->lock);
+}
+
+static int vgic_v3_gits_mmio_read(struct vcpu *v, mmio_info_t *info)
+{
+ struct vgic_its *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);
+ uint64_t val = 0;
+ uint32_t index, gits_reg;
+
+ vits = its_to_vits(v, info->gpa);
+ if ( vits == NULL ) BUG_ON(1);
BUG_ON(vits != NULL);
Although I would document this BUG_ON to explain that its_to_vits should
never fail because MMIOs registered always point to an ITS.
Though, an ASSERT maybe better here.
+
+ gits_reg = info->gpa - vits->phys_base;
+
+ if ( gits_reg >= SZ_64K )
+ {
+ gdprintk(XENLOG_G_WARNING, "vGITS: unknown gpa read address \
+ %"PRIpaddr"\n", info->gpa);
+ return 0;
+ }
This can never happen, you always register 64K range.
+
+ switch ( gits_reg )
+ {
+ case GITS_CTLR:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
Missing implementation for GITS_CTLR
+ return 1;
+ case GITS_IIDR:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
Missing implementation for GITS_IIDR
+ return 1;
+ case GITS_TYPER:
+ /* GITS_TYPER support word read */
+ vits_spin_lock(vits);
+ val = ((its_get_pta_type() << VITS_GITS_TYPER_PTA_SHIFT) |
As said on a previous patch, each ITS may have a different value in PTA.
I think it would make the command emulation simpler if we use an
hardcoded PTA (PTA = 0 i.e using linear processor numbers seems the
simpler).
+ VITS_GITS_TYPER_HCC | VITS_GITS_DEV_BITS |
I will comment the value here..
Where does the value of HCC and DEV_BITS come from? Both of them looks
wrong to me.
+ VITS_GITS_ID_BITS | VITS_GITS_ITT_SIZE |
Ditto for ID_BITS and ITT_SIZE.
Although, it looks like that ITT_SIZE should be the same as the
hardware. This is because you let the guest allocation the ITT.
I would also rename ITT_SIZE to ITT_ENTRY_SIZE.
+ VITS_GITS_DISTRIBUTED | VITS_GITS_PLPIS);
The bit 3 is marked as implementation defined. So why did you name it
DISTRIBUTED?
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ *r = val;
+ else if ( dabt.size == DABT_WORD )
+ *r = (u32)(val >> 32);
+ else
+ {
+ vits_spin_unlock(vits);
+ goto bad_width;
+ }
+ vits_spin_unlock(vits);
The vits_spin_unlock could be done before setting *r.
+ return 1;
+ case GITS_TYPER + 4:
I don't like the idea to duplicate the code for GITS_TYPER just for
reading the top word. Isn't possible to merge the 2 switch case?
Reading the spec again, it's not mandatory support support 32-bit access
on 64-bit registers.
Given that we don't support GICv3 for 32-bit guest, I would completely
drop the 32-bit access on 64-bit guest.
+ if (dabt.size != DABT_WORD ) goto bad_width;
if ( ...
+ vits_spin_lock(vits);
+ val = ((its_get_pta_type() << VITS_GITS_TYPER_PTA_SHIFT) |
+ VITS_GITS_TYPER_HCC | VITS_GITS_DEV_BITS |
+ VITS_GITS_ID_BITS | VITS_GITS_ITT_SIZE |
+ VITS_GITS_DISTRIBUTED | VITS_GITS_PLPIS);
+ *r = (u32)val;
+ vits_spin_unlock(vits);
+ return 1;
+ case 0x0010 ... 0x007c:
+ case 0xc000 ... 0xffcc:
+ /* Implementation defined -- read ignored */
+ dprintk(XENLOG_ERR,
+ "vGITS: read unknown 0x000c - 0x007c r%d offset %#08x\n",
+ dabt.reg, gits_reg);
Please don't use XENLOG_ERR in guest code. Also, this printk is not
useful and has been dropped in other emulation.
+ goto read_as_zero;
+ case GITS_CBASER:
+ vits_spin_lock(vits);
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ *r = vits->cmd_base && 0xc7ffffffffffffffUL;
Why the && and what does mean this constant?
+ else if ( dabt.size == DABT_WORD )
+ *r = (u32)vits->cmd_base;
+ else
+ {
+ vits_spin_unlock(vits);
+ goto bad_width;
+ }
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_CBASER + 4:
+ /* CBASER support word read */
+ if (dabt.size != DABT_WORD ) goto bad_width;
if ( ...
+ vits_spin_lock(vits);
+ *r = (u32)(vits->cmd_base >> 32);
+ vits_spin_unlock(vits);
+ return 1;
Same remark as GITS_TYPER for the word read support.
+ case GITS_CWRITER:
+ vits_spin_lock(vits);
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ *r = vits->cmd_write;
+ else if ( dabt.size == DABT_WORD )
+ *r = (u32)vits->cmd_write;
+ else
+ {
+ vits_spin_unlock(vits);
+ goto bad_width;
+ }
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_CWRITER + 4:
+ /* CWRITER support word read */
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ vits_spin_lock(vits);
+ *r = (u32)(vits->cmd_write >> 32);
+ vits_spin_unlock(vits);
+ return 1;
Ditt for the word-read
+ case GITS_CREADR:
+ vits_spin_lock(vits);
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ *r = vits->cmd_read;
+ else if ( dabt.size == DABT_WORD )
+ *r = (u32)vits->cmd_read;
+ else
+ {
+ vits_spin_unlock(vits);
+ goto bad_width;
+ }
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_CREADR + 4:
+ /* CREADR support word read */
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ vits_spin_lock(vits);
+ *r = (u32)(vits->cmd_read >> 32);
+ vits_spin_unlock(vits);
+ return 1;
Ditto
+ case 0x0098 ... 0x009c:
+ case 0x00a0 ... 0x00fc:
+ case 0x0140 ... 0xbffc:
+ /* Reserved -- read ignored */
+ dprintk(XENLOG_ERR,
+ "vGITS: read unknown 0x0098-9c or 0x00a0-fc r%d offset
%#08x\n",
+ dabt.reg, gits_reg);
No need of printk here.
+ goto read_as_zero;
+ case GITS_BASER ... GITS_BASERN:
The spec says that registers are RES0 if not implemented.
As you use at all baser outside the register emulation, I would
implement them RAZ/WI.
That would avoid a wrong write implementation.
+ vits_spin_lock(vits);
+ index = (gits_reg - GITS_BASER) / 8;
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ *r = vits->baser[index];
+ else if ( dabt.size == DABT_WORD )
+ {
+ if ( (gits_reg % 8) == 0 )
+ *r = (u32)vits->baser[index];
+ else
+ *r = (u32)(vits->baser[index] >> 32);
+ }
+ else
+ {
+ vits_spin_unlock(vits);
+ goto bad_width;
+ }
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_PIDR0:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ *r = GITS_PIDR0_VAL;
+ return 1;
+ case GITS_PIDR1:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ *r = GITS_PIDR1_VAL;
+ return 1;
+ case GITS_PIDR2:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ *r = GITS_PIDR2_VAL;
+ return 1;
+ case GITS_PIDR3:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ *r = GITS_PIDR3_VAL;
+ return 1;
+ case GITS_PIDR4:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ *r = GITS_PIDR4_VAL;
+ return 1;
+ case GITS_PIDR5 ... GITS_PIDR7:
+ goto read_as_zero;
Please check that we access is done via a word-access by introducing a
new label read_as_zero_32 (for instance see the vgic v2 emulation).
+ default:
+ dprintk(XENLOG_ERR, "vGITS: unhandled read r%d offset %#08x\n",
+ dabt.reg, gits_reg);
printk(XENLOG_G_ERR "%pv: ....", v,...)
Also it may be useful to printk which vITS is in use.
+ return 0;
+ }
+
+bad_width:
+ dprintk(XENLOG_ERR, "vGITS: bad read width %d r%d offset %#08x\n",
+ dabt.size, dabt.reg, gits_reg);
printk(XENLOG_G_ERR "%pv: ...", v,...)
Same remark for printing the vITS.
+ domain_crash_synchronous();
+ return 0;
+
+read_as_zero:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
How do you know that all RAZ access 32-bit access? See implementation
defined registers for instance.
I would prefer to introduce multiple label:
read_as_zero_32: /* RAZ 32-bit */
if ( dabt.size != DABT_WORD ) goto bad_width;
read_as_zero: /* Not check necessary */
*r = 0;
And use the correctly label for goto in the emulation. So the code would
be self-documented too.
+ *r = 0;
+ return 1;
+}
+
+static int vgic_v3_gits_mmio_write(struct vcpu *v, mmio_info_t *info)
+{
+ struct vgic_its *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);
+ int ret;
+ uint32_t index, gits_reg;
+ uint64_t val;
+
+ vits = its_to_vits(v, info->gpa);
+ if ( vits == NULL ) BUG_ON(1);
Same remark as the BUG_ON in vgic_v3_gits_mmio_read.
I'm wondering if it would be better to extend the read/write handler to
get an opaque pointer in parameter.
In this case, it would contain the vits and would avoid the its_to_vits
every time.
+ gits_reg = info->gpa - vits->phys_base;
+
+ if ( gits_reg >= SZ_64K )
+ {
+ gdprintk(XENLOG_G_WARNING, "vGIC-ITS: unknown gpa write address"
+ " %"PRIpaddr"\n", info->gpa);
+ return 0;
+ }
This check is not necessary.
+ switch ( gits_reg )
+ {
+ case GITS_CTLR:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ vits_spin_lock(vits);
+ vits->ctrl = *r;
Only bit[0] (Enabled) is writable.
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_IIDR:
+ /* R0 -- write ignored */
+ goto write_ignore;
goto write_ignore_32;
+ case GITS_TYPER:
+ case GITS_TYPER + 4:
+ /* R0 -- write ignored */
+ goto write_ignore;
Please explicitly check the access size. That would avoid to crash the
guest when TYPER is write with a 64-bit access.
+ case 0x0010 ... 0x007c:
+ case 0xc000 ... 0xffcc:
+ /* Implementation defined -- write ignored */
+ dprintk(XENLOG_ERR,
+ "vGITS: write to unknown 0x000c - 0x007c r%d offset %#08x\n",
+ dabt.reg, gits_reg);
Please drop the dprintk.
+ goto write_ignore;
+ case GITS_CBASER:
+ if ( dabt.size == DABT_BYTE ) goto bad_width;
Please do the check in the invert way. i.e
(dabt.size != DABT_WORD) && (dabt.size != DABT_DOUBLE_WORD)
Also GITS_CBASER is read-only when GITS_CTLR.Enable is Zero or
GITS_CTLR.Quiescent is zero.
+ vits_spin_lock(vits);
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ vits->cmd_base = *r;
+ else
+ {
+ val = vits->cmd_base & 0xffffffff00000000UL;
The mask is difficult to read. And not all the bits are writeable.
+ val = (*r) | val;
+ vits->cmd_base = val;
+ }
+ vits->cmd_qsize = SZ_4K * ((*r & 0xff) + 1);
Please use a define for the mask.
Also I would use cmd_qsize to know if the valid is set or not. I.e
cmd_qsize = 0 => command queue not valid.
You forgot to update GITS_CREADR (i.e setting to 0) when GITS_CBASER is
successfully written.
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_CBASER + 4:
32-bit support is not necessary and make the code more complex for nothing.
+ /* CBASER support word read */
+ if (dabt.size != DABT_WORD ) goto bad_width;
+ vits_spin_lock(vits);
+ val = vits->cmd_base & 0xffffffffUL;
+ val = ((*r & 0xffffffffUL) << 32 ) | val;
+ vits->cmd_base = val;
+ /* No Need to update cmd_qsize with higher word write */
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_CWRITER:
+ if ( dabt.size == DABT_BYTE ) goto bad_width;
+ vits_spin_lock(vits);
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ vits->cmd_write = *r;
Only Bits[19:5] are writable.
+ else
+ {
+ val = vits->cmd_write & 0xffffffff00000000UL;
+ val = (*r) | val;
+ vits->cmd_write = val;
+ }
No validation of the value written by the guest? Given your
implementation of the command processing, any invalid value will end up
to an infinite loop in the hypervisor. Whoops :).
+ ret = vgic_its_process_cmd(v, vits);
+ vits_spin_unlock(vits);
+ return ret;
+ case GITS_CWRITER + 4:
Same remark as GITS_CBASER for the 32-bit support.
+ if (dabt.size != DABT_WORD ) goto bad_width;
+ vits_spin_lock(vits);
+ val = vits->cmd_write & 0xffffffffUL;
+ val = ((*r & 0xffffffffUL) << 32) | val;
+ vits->cmd_write = val;
+ ret = vgic_its_process_cmd(v, vits);
+ vits_spin_unlock(vits);
+ return ret;
+ case GITS_CREADR:
+ /* R0 -- write ignored */
+ goto write_ignore;
+ case 0x0098 ... 0x009c:
+ case 0x00a0 ... 0x00fc:
+ case 0x0140 ... 0xbffc:
+ /* Reserved -- write ignored */
+ dprintk(XENLOG_ERR,
+ "vGITS: write to unknown 0x98-9c or 0xa0-fc r%d offset
%#08x\n",
+ dabt.reg, gits_reg);
Please drop the dprintk
+ goto write_ignore;
+ case GITS_BASER ... GITS_BASERN:
+ /* Nothing to do with this values. Just store and emulate */
As you don't use those values at all, write ignore would be better.
+ vits_spin_lock(vits);
+ index = (gits_reg - GITS_BASER) / 8;
+ if ( dabt.size == DABT_DOUBLE_WORD )
+ vits->baser[index] = *r;
+ else if ( dabt.size == DABT_WORD )
+ {
+ if ( (gits_reg % 8) == 0 )
+ {
+ val = vits->cmd_write & 0xffffffff00000000UL;
cmd_write seems to come out of nowhere...
+ val = (*r) | val;
+ vits->baser[index] = val;
+ }
+ else
+ {
+ val = vits->baser[index] & 0xffffffffUL;
+ val = ((*r & 0xffffffffUL) << 32) | val;
+ vits->baser[index] = val;
+ }
+ }
+ else
+ {
+ goto bad_width;
+ vits_spin_unlock(vits);
+ }
+ vits_spin_unlock(vits);
+ return 1;
+ case GITS_PIDR7 ... GITS_PIDR0:
+ /* R0 -- write ignored */
+ goto write_ignore;
+ default:
+ dprintk(XENLOG_ERR, "vGITS: unhandled write r%d offset %#08x\n",
+ dabt.reg, gits_reg);
printk(XENLOG_G_ERR "%pv: .....", v, ....);
+ Print which ITS is in use.
+ return 0;
+ }
+
+bad_width:
+ dprintk(XENLOG_ERR, "vGITS: bad write width %d r%d offset %#08x\n",
+ dabt.size, dabt.reg, gits_reg);
Ditto
+ domain_crash_synchronous();
+ return 0;
+
+write_ignore:
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+ *r = 0;
+ return 1;
Same remark as read_ignore.
+}
+
+static const struct mmio_handler_ops vgic_gits_mmio_handler = {
+ .read_handler = vgic_v3_gits_mmio_read,
+ .write_handler = vgic_v3_gits_mmio_write,
+};
+
+int vgic_its_domain_init(struct domain *d)
You forgot to add the prototype of this function in the header...
+{
This code is not really part of the ITS registers emulation...
Your patchs series splitting is really confusing.
+ uint32_t num_its;
+ int i;
+
+ num_its = its_get_nr_its();
+
+ d->arch.vits = xzalloc_array(struct vgic_its, num_its);
Hmm... why did you use the number of physical ITS rather than the number
of vITS used by the guest.
It would avoid to waste so much memory for every domain.
+ if ( d->arch.vits == NULL )
+ return -ENOMEM;
+
+ spin_lock_init(&d->arch.vits->lock);
+
+ spin_lock_init(&d->arch.vits_devs.lock);
+ INIT_LIST_HEAD(&d->arch.vits_devs.dev_list);
+
+ d->arch.lpi_conf = xzalloc(struct vgic_lpi_conf);
+ if ( d->arch.lpi_conf == NULL )
+ return -ENOMEM;
+
+ for ( i = 0; i < num_its; i++)
+ {
+ spin_lock_init(&d->arch.vits[i].lock);
+ register_mmio_handler(d, &vgic_gits_mmio_handler,
+ d->arch.vits[i].phys_base,
+ SZ_64K);
+ }
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
index 70ec913..82cfbdc 100644
--- a/xen/include/asm-arm/gic-its.h
+++ b/xen/include/asm-arm/gic-its.h
@@ -227,6 +227,7 @@ int gic_its_send_cmd(struct vcpu *v, struct its_node *its,
void its_lpi_free(unsigned long *bitmap, int base, int nr_ids);
unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids);
uint32_t its_get_pta_type(void);
+uint32_t its_get_nr_its(void);
struct its_node * its_get_phys_node(uint32_t dev_id);
#endif /* __ASM_ARM_GIC_ITS_H__ */
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel