[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.