Re: [Xen-devel] [RFC PATCH v3 11/18] xen/arm: ITS: Add GITS registers emulation

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 22/06/2015 14:01, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> Emulate GITS* registers and handle LPI configuration
> table update trap.
The LPI configuration table as nothing to do with the GITS registers. 
It's related to the GICR. Why didn't you implement it on the next patch?
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/vgic-v3-its.c    |  516 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/gic-its.h |   14 ++
>   2 files changed, 530 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 0671434..fa9dccc 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -63,6 +63,46 @@ static void dump_cmd(its_cmd_block *cmd)
>   }
>   #endif
>
> +void vgic_its_disable_lpis(struct vcpu *v, uint32_t vlpi)
Any function non-exported should be static. Also you are disabling only 
one lpi, so I would rename the function vgic_its_disable_lpi.
> +{
> +    struct pending_irq *p;
> +    unsigned long flags;
> +
> +    p = irq_to_pending(v, vlpi);
> +    clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +    gic_remove_from_queues(v, vlpi);
> +    if ( p->desc != NULL )
> +    {
> +        spin_lock_irqsave(&p->desc->lock, flags);
> +        p->desc->handler->disable(p->desc);
> +        spin_unlock_irqrestore(&p->desc->lock, flags);
> +    }
It's not possible to associate a unique pLPI to a vLPI. This function 
should only update the pending_state.
> +}
> +
> +void vgic_its_enable_lpis(struct vcpu *v, uint32_t vlpi, uint8_t priority)
Ditto for static & name.

> +{
> +    struct pending_irq *p;
> +    unsigned long flags;
> +
> +    /* Get plpi for the given vlpi */
> +    p = irq_to_pending(v, vlpi);
> +    p->priority = priority;
Why? There is already a callback to get the IRQ priority (see 
get_irq_priority). It will retrieve the correct priority when the IRQ is 
injected (and not when it's has been enabled).
In anycase, this is not safe to write in p->pending like that.

> +    set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +
> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
The vIRQ is protected by the CPU where it's routed not the one which 
enabled. You have to find the right vCPU.
> +
> +    if ( !list_empty(&p->inflight) &&
> +         !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> +        gic_raise_guest_irq(v, irq_to_virq(p->desc), p->priority);
> +
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +    if ( p->desc != NULL )
> +    {
> +        spin_lock_irqsave(&p->desc->lock, flags);
> +        p->desc->handler->enable(p->desc);
> +        spin_unlock_irqrestore(&p->desc->lock, flags);
> +    }
Ditto for the pLPI to vLPI mapping.

> +}
Missing newline

>   /* ITS device table helper functions */
>   int vits_vdevice_entry(struct domain *d, uint32_t dev_id,
>                          struct vdevice_table *entry, int set)
> @@ -649,6 +689,482 @@ err:
>       return 0;
>   }
>
> +static int vgic_v3_gits_lpi_mmio_read(struct vcpu *v, mmio_info_t *info)
The LPI configuration table is not ITS specific but GICv3. Although, I'm 
fine if you let this code here for the time being.
> +{
> +    uint32_t offset;
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +    uint8_t cfg;
> +
> +    offset = info->gpa -
> +             (v->domain->arch.vits->propbase & 0xfffffffff000UL);
You are using 0xffff... in multiple place. Please add a define and use it.

> +
> +    if ( offset < SZ_64K )
AFAICT, the LPI configuration table can be smaller and higher than 64K. 
Although, this check is not necessary because you have registered the 
handler on a valid MMIO range.
> +    {
> +        DPRINTK("vITS:d%dv%d LPI Table read offset 0x%x\n",
> +                v->domain->domain_id, v->vcpu_id, offset);
%pv rather than d%dv%d

> +        cfg = readb_relaxed(v->domain->arch.vits->prop_page + offset);
Why do you use readb? Those helpers have been created to read MMIO not 
Xen memory.
Although what about other access? a 64/32/16 bits access are valid and 
will return the wrong value.
> +        *r = cfg;
> +        return 1;
> +    }
> +    else
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d LPI Table read with wrong offset 
> 0x%x\n",
> +                v->domain->domain_id, v->vcpu_id, offset);
> +
> +
> +    return 0;
> +}
> +
> +static int vgic_v3_gits_lpi_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    uint32_t offset;
> +    uint32_t vid;
> +    uint8_t cfg;
> +    bool_t enable;
> +    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 -
> +             (v->domain->arch.vits->propbase & 0xfffffffff000UL);
> +
> +    vid = offset + NR_GIC_LPI;
> +    if ( offset < SZ_64K )
> +    {
> +        DPRINTK("vITS:d%dv%d LPI Table write offset 0x%x\n",
> +                v->domain->domain_id, v->vcpu_id, offset);
%pv rather than d%dv%d

> +        cfg = readb_relaxed(v->domain->arch.vits->prop_page + offset);
> +        enable = (cfg & *r) & 0x1;
Please use a define rather than 0x1. It would have been more clear that 
we are checking the enable bit.
> +
> +        if ( !enable )
> +             vgic_its_enable_lpis(v, vid,  (*r & 0xfc));
Please a define for the priority mask.

> +        else
> +             vgic_its_disable_lpis(v, vid);
> +
> +        /* Update virtual prop page */
> +        writeb_relaxed((*r & 0xff),
> +                        v->domain->arch.vits->prop_page + offset);
Same remark as readb_relaxed. You also need to handle all the other 
possible write access.
Finally, this function is racy. Nothing prevent 2 vCPUs to write in the 
LPI configuration table. This may result to incoherency between the 
state of the pending_irq and the Configuration Table.
You have to provide at least a locking for the Configuration Table.

> +
> +        return 1;
> +    }
> +    else
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d LPI Table invalid write @ 0x%x\n",
> +                v->domain->domain_id, v->vcpu_id, offset);
> +
> +    return 0;
> +}
> +
> +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 vgic_its_unmap_lpi_prop(struct vcpu *v)
Please define the prototype of this function in the header within the 
same file if you plan to export. If not, it should be static.
> +{
> +    paddr_t maddr;
> +    uint32_t lpi_size;
> +    int i;
> +
> +    maddr = v->domain->arch.vits->propbase & 0xfffffffff000UL;
> +    lpi_size = 1UL << ((v->domain->arch.vits->propbase & 0x1f) + 1);
Please use a define for the 0x1f.

> +
> +    DPRINTK("vITS:d%dv%d Unmap guest LPI conf table maddr 0x%lx lpi_size 
> 0x%x\n",
> +             v->domain->domain_id, v->vcpu_id, maddr, lpi_size);
> +
> +    if ( lpi_size < SZ_64K )
Why this restriction? The IDbits can encode up to 32 bits interrupt
identifier.

You have to check this value against GICD_TYPER.IDbits.

> +    {
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d LPI Prop page < 64K\n",
> +                v->domain->domain_id, v->vcpu_id);
> +        return 0;
> +    }
> +
> +    /* XXX: As per 4.8.9 each re-distributor shares a common LPI configuration 
> table
Coding style:
/*
 * ...
 */

Also XXX means TODO.

4.8.9 of what? Please provide a section for the public doc (i.e 
IHI0069A...) and not private.
> +     * So one set of mmio handlers to manage configuration table is enough
> +     */
> +    for ( i = 0; i < lpi_size / PAGE_SIZE; i++ )
> +        guest_physmap_remove_page(v->domain, paddr_to_pfn(maddr),
> +                                gmfn_to_mfn(v->domain, paddr_to_pfn(maddr)), 
> 0);
No validation at all on the address pass for the guest? gmfn_to_mfn can
return an invalid MFN and I'm not sure what would happen if the guest is
trying to pass other things than RAM.

> +
> +    /* Register mmio handlers for this region */
> +    register_mmio_handler(v->domain, &vgic_gits_lpi_mmio_handler,
> +                          maddr, lpi_size);
> +
> +    /* Allocate Virtual LPI Property table */
> +    v->domain->arch.vits->prop_page =
> +        alloc_xenheap_pages(get_order_from_bytes(lpi_size), 0);
I wasn't able to find a place where you free the pages allocated... 
Please ensure that any allocated memory is freed when the guest is 
destroyed.
Rather than allocating the page from Xen memory, I'm wondering if we can 
re-use the page unmapped.
If not, I would prefer to see this allocation at domain creation time 
rather than when the domain is setting PROPBASER. The main reason is 
with the current implementation, the guest could abuse of PROPBASER and 
exhaust the Xen memory. It would require to do some management here.
Also, what if the guest decide to change it's property table? This is 
valid and I think we will hit with the suspend/resume case. I'm fine if 
you don't want to handle it now, but please make it obvious that you 
don't handle it.
> +    if ( !v->domain->arch.vits->prop_page )
> +    {
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d Failed to allocate LPI Prop page\n",
> +                v->domain->domain_id, v->vcpu_id);
> +        return 0;
> +    }
> +
> +    memset(v->domain->arch.vits->prop_page, 0xa2, lpi_size);
Where does this value come from? It's up to the guest to decide what 
will be the priority of vLPI. Also, have you a doc reference to say that 
we should ignore any value in the initial set of pages?
> +
> +    return 1;
> +}
> +
> +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 = v->domain->arch.vits;
You can directly do struct vgic_its *vits = v->domain->arch.vits

> +
> +    gits_reg = info->gpa - vits->phys_base;
> +
> +    if ( gits_reg >= SZ_64K )
This can never happen, you always register a valid 64K range.

> +    {
> +        gdprintk(XENLOG_G_WARNING,
> +                 "vITS:d%dv%d unknown gpa read address %"PRIpaddr"\n",
> +                 v->domain->domain_id, v->vcpu_id, info->gpa);
> +        return 0;
> +    }
> +
> +    switch ( gits_reg )
> +    {
> +    case GITS_CTLR:
> +        if ( dabt.size != DABT_WORD )
> +            goto bad_width;
> +        *r = 0;
Missing implementation of GITS_CTLR.

> +        return 1;
> +    case GITS_IIDR:
> +        if ( dabt.size != DABT_WORD )
> +            goto bad_width;
> +        *r = 0;
Missing implementaiton of GITS_IIDR

> +        return 1;
> +    case GITS_TYPER:
> +        vits_spin_lock(vits);
Why the lock?

Please add a comment explaining the configuration of this field. It 
would avoid to scratch his head try to understand why PTA is not 
mentioned...
> +        val = (((v->domain->max_vcpus + 1) << GITS_TYPER_HCC_SHIFT ) |
I would prefer to see a macro to compute the number of collection.

Although, HCC is only able to encore 16-bit of collection ID (i.e 256 
collection => 255 VCPUs). Please make a note somewhere and add a 
BUILD_ON_BUG on anything else to ensure that it will never happen.
HCC is only able to encode 16-bit collection ID (i.e 255 collection).


> +                 VITS_GITS_DEV_BITS | VITS_GITS_ID_BITS              |
Please introduce new fields in vits to store the number of deviceID bits 
and eventID bits. Don't harcode them.
> +                 VITS_GITS_ITT_SIZE | VITS_GITS_DISTRIBUTED          |
The bit 3 is marked as implementation defined. Why do you set and name 
it DISTRIBUTED?
> +                 VITS_GITS_PLPIS);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            *r = val;
> +        else if ( dabt.size == DABT_WORD )
> +            *r = (u32)val;
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
> +        vits_spin_unlock(vits);
> +        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?
> +        if (dabt.size != DABT_WORD ) goto bad_width;
> +        vits_spin_lock(vits);
> +        val = (((v->domain->max_vcpus + 1) << GITS_TYPER_HCC_SHIFT ) |
> +                 VITS_GITS_DEV_BITS | VITS_GITS_ID_BITS              |
> +                 VITS_GITS_ITT_SIZE | VITS_GITS_DISTRIBUTED          |
> +                 VITS_GITS_PLPIS);
> +        *r = (u32)(val >> 32);
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case 0x0010 ... 0x007c:
> +    case 0xc000 ... 0xffcc:
> +        /* Implementation defined -- read ignored */
> +        dprintk(XENLOG_ERR,
> +                "vITS:d%dv%d read unknown 0x000c - 0x007c r%d offset %#08x\n",
> +                v->domain->domain_id, v->vcpu_id, 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:
> +        /* XXX: Only read support 32/64-bit access */
XXX means TODO which is not the case here.

> +        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);
The lock section could have been smaller and avoid to duplicat 
vits_spin_unlock by first reading the value then setting the register.
> +        return 1;
> +    case GITS_CBASER + 4:
Same remark as GITS_TYPER for the support for reading word.

> +        if (dabt.size != DABT_WORD )
if ( ... )

> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = (u32)(vits->cmd_base >> 32);
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CWRITER:
See all my comment on CBASER.

> +        /* XXX: Only read support 32/64-bit access */
> +        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:
> +        if ( dabt.size != DABT_WORD )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        *r = (u32)(vits->cmd_write >> 32);
> +        vits_spin_unlock(vits);
> +        return 1;
ditto for word-read

> +    case GITS_CREADR:
> +        /* XXX: Only read support 32/64-bit access */
> +        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:
> +        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,
> +                "vITS:d%dv%d read unknown 0x0098-9c or 0x00a0-fc r%d offset 
> %#08x\n",
> +                v->domain->domain_id, v->vcpu_id, dabt.reg, gits_reg);
No need to the message.

> +        goto read_as_zero;
> +    case GITS_BASER ... GITS_BASERN:
> +        /* Supports only 64-bit access */
The XXX would have been useful here ;)

> +        if ( dabt.size != DABT_DOUBLE_WORD )
> +            goto bad_width;
> +        if ( (gits_reg % 8) != 0 )
What is used for?

> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        index = (gits_reg - GITS_BASER) / 8;
> +        *r = vits->baser[index];
> +        vits_spin_unlock(vits);
Based on the spec (8.19.1 in IHI0069A), any unimplemented register 
should be RES0.
As we only support BASER0, I would only implemented it with a check all 
the other would go to the read_as_zero.
> +        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_G_ERR, "vITS:d%dv%d unhandled read r%d offset %#08x\n",
> +                v->domain->domain_id, v->vcpu_id, dabt.reg, gits_reg);
> +        return 0;
> +    }
> +
> +bad_width:
> +    dprintk(XENLOG_G_ERR, "vITS:d%dv%d bad read width %d r%d offset %#08x\n",
> +           v->domain->domain_id, v->vcpu_id, dabt.size, dabt.reg, gits_reg);
printk(XENLOG_G_ERR "%pv: ....", v,...)

> +    domain_crash_synchronous();
> +    return 0;
> +
> +read_as_zero:
> +    if ( dabt.size != DABT_WORD )
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.
> +       goto bad_width;
> +    *r = 0;
> +    return 1;
> +}
> +
> +#define GITS_BASER_MASK  (~((0x7UL << GITS_BASER_TYPE_SHIFT) | \
> +                         (0xffUL << GITS_BASER_ENTRY_SIZE_SHIFT) | \
> +                         (0x3UL << GITS_BASER_SHAREABILITY_SHIFT)))
Please add a comment to explain what the mask is used for.

> +
> +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, sz, psz;
> +    uint64_t val;
> +
> +    vits = v->domain->arch.vits;
> +
> +    gits_reg = info->gpa - vits->phys_base;
> +
> +    if ( gits_reg >= SZ_64K )
> +    {
> +        gdprintk(XENLOG_G_WARNING,
> +                 "vITS:d%dv%d unknown gpa write address %"PRIpaddr"\n",
> +                 v->domain->domain_id, v->vcpu_id, 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;
> +        vits_spin_unlock(vits);
Only bit[0] (Enabled) is writable.

> +        return 1;
> +    case GITS_IIDR:
> +        /* R0 -- write ignored */
> +        goto write_ignore;
> +    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_G_ERR,
> +                "vITS:d%dv%d write to unknown 0x000c - 0x007c r%d offset 
> %#08x\n",
> +                v->domain->domain_id, v->vcpu_id, dabt.reg, gits_reg);
Please drop the dprintk.

> +        goto write_ignore;
> +    case GITS_CBASER:
GITS_CBASER is read-only when GITS_CTLR.Enable is Zero or 
GITS_CTLR.Quiescent is zero.
> +        if ( dabt.size != DABT_DOUBLE_WORD )
Missing a TODO for 32bit access support.

> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        vits->cmd_base = *r;
> +        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 (8.19.2 in IHI0069A).
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_CBASER + 4:
> +         /* XXX: Does not support word write */
> +        goto bad_width;
> +    case GITS_CWRITER:
> +        vits_spin_lock(vits);
> +        if ( dabt.size == DABT_DOUBLE_WORD )
> +            vits->cmd_write = *r;
Only Bits[19:5] are writable.

> +        else if ( dabt.size == DABT_WORD)
> +        {
> +            val = vits->cmd_write & 0xffffffff00000000UL;
> +            val = (*r) | val;
Only Bits[19:5] are writable.

> +            vits->cmd_write =  val;
> +        }
> +        else
> +        {
> +            vits_spin_unlock(vits);
> +            goto bad_width;
> +        }
This could be simplified to avoid 2 vits_spin_unlock.

Also, 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:
Only the Bits[19:5] of GITS_CWRITER are writable. So the emulation is 
pointless here.
> +        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_G_ERR,
> +                "vITS:d%dv%d write to unknown 0x98-9c or 0xa0-fc r%d offset 
> %#08x\n",
> +                v->domain->domain_id, v->vcpu_id, dabt.reg, gits_reg);
Please drop the printk

> +        goto write_ignore;
> +    case GITS_BASER0:
> +        if ( dabt.size != DABT_DOUBLE_WORD )
/* XXX: Support 32 bit access */

> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        vits->baser[0] = vits->baser[0] | (GITS_BASER_MASK & *r);
I would prefer to see *r & GITS_BASER_MASK which is more logical to read.

Although this doesn't do what you want. This will not erase bit set to 1 
in baser.
> +        vits->dt_ipa = vits->baser[0] & 0xfffffffff000UL;
> +        psz = (vits->baser[0] >> GITS_BASER_PAGE_SIZE_SHIFT) &
> +               GITS_BASER_PAGE_SIZE_MASK_VAL;
> +        if ( psz == GITS_BASER_PAGE_SIZE_4K_VAL )
> +            sz = 4;
> +        else if ( psz == GITS_BASER_PAGE_SIZE_16K_VAL )
> +            sz = 16;
> +        else
> +            sz = 64;
> +
> +        vits->dt_size = (vits->baser[0] & GITS_BASER_PAGES_MASK_VAL)
> +                        * sz * SZ_1K;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_BASER1 ... GITS_BASERN:
> +        /* Nothing to do with this values. Just store and emulate */
From the spec these register should be RES0. There is nothing to do here.

> +        if ( dabt.size != DABT_DOUBLE_WORD )
> +            goto bad_width;
> +        if ( (gits_reg % 8) != 0 )
> +            goto bad_width;
> +        vits_spin_lock(vits);
> +        index = (gits_reg - GITS_BASER) / 8;
> +        vits->baser[index] = *r;
> +        vits_spin_unlock(vits);
> +        return 1;
> +    case GITS_PIDR7 ... GITS_PIDR0:
> +        /* R0 -- write ignored */
> +        goto write_ignore;
> +   default:
> +        dprintk(XENLOG_G_ERR, "vITS:d%dv%d unhandled write r%d offset %#08x\n",
> +                v->domain->domain_id, v->vcpu_id, dabt.reg, gits_reg);
printk(XENLOG_G_ERR "VITS %pv: .....", v, ....);

> +        return 0;
> +    }
> +
> +bad_width:
> +    dprintk(XENLOG_G_ERR, "vITS:d%dv%d bad write width %d r%d offset %#08x\n",
> +            v->domain->domain_id, v->vcpu_id, dabt.size, dabt.reg, gits_reg);
Ditto

> +    domain_crash_synchronous();
> +    return 0;
> +
> +write_ignore:
Same remark as read_ignore.

> +    if ( dabt.size != DABT_WORD ) goto bad_width;
> +    *r = 0;
> +    return 1;
> +}
> +
> +
> +static const struct mmio_handler_ops vgic_gits_mmio_handler = {
> +    .read_handler  = vgic_v3_gits_mmio_read,
> +    .write_handler = vgic_v3_gits_mmio_write,
> +};
> +
Any patch should be compilable on standalone to avoid breaking 
bisection. I believe this is not the case here.
As suggested earlier, I would prefer the Makefile change to include this 
file when everything is done. This would avoid you s//static/ changes...
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 8f898a6..3271477 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -38,6 +38,8 @@ struct its_collection {
>   struct vgic_its
>   {
>      spinlock_t lock;
> +   /* Emulation of BASER */
> +   paddr_t baser[8];
I don't think this is useful.

>      /* Command queue base */
>      paddr_t cmd_base;
>      /* Command queue write pointer */
> @@ -48,8 +50,20 @@ struct vgic_its
>      paddr_t cmd_read;
>      /* Command queue size */
>      unsigned long cmd_qsize;
> +   /* ITS mmio physical base */
> +   paddr_t phys_base;
> +   /* ITS mmio physical size */
> +   unsigned long phys_size;
The names are misleading, I though it was referring to the hardware address.

I would prefer if you rename into gits_base and gits_size.

>      /* ITS physical node */
>      struct its_node *its;
> +   /* GICR ctrl register */
> +   uint32_t ctrl;
> +   /* LPI propbase */
> +   paddr_t propbase;
> +   /* percpu pendbase */
> +   paddr_t pendbase[MAX_VIRT_CPUS];
> +   /* Virtual LPI property table */
> +   void * prop_page;
Coding style: void *prop_page

>      /* vITT device table ipa */
>      paddr_t dt_ipa;
>      /* vITT device table size */
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel