|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 13/22] xen/arm: its: Add virtual ITS command support
Hello,
Second part of the review.
On 19/03/15 14:38, vijay.kilari@xxxxxxxxx wrote:
> +static int vgic_its_build_sync_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
> +{
> + uint64_t pta;
> +
> + its_encode_cmd(phys_cmd, GITS_CMD_SYNC);
> + pta = vgic_its_get_pta(v, vits, its_decode_target(virt_cmd));
The command SYNC is sent to all the ITS. But the target address may not
be the same depending on GITS_TYPER.PTA.
> +
> + return 0;
> +}
> +
> +static int vgic_its_build_mapvi_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
I'm not sure to understand why we have to implement mapvi. The spec says
"It is expected that in GICv3 systems this command will only be used by
hypervisor software".
So why Linux is using it?
Also, you handle both mapi and mapvi here. So I would rename the function.
> +{
> + struct domain *d = v->domain;
> + struct its_device *dev;
> + uint32_t pcol_id;
> + uint32_t pid;
> + struct irq_desc *desc;
> + uint32_t dev_id = its_decode_devid(virt_cmd);
> + uint32_t id = its_decode_event_id(virt_cmd);
> + uint8_t vcol_id = its_decode_collection(virt_cmd);
> + uint32_t vid = its_decode_phys_id(virt_cmd);
> + uint8_t cmd = its_decode_cmd(virt_cmd);
It looks like to me that its_cmd_block would benefit to be an union of
command. It would avoid defining so many variable at each command.
> +
> + DPRINTK("vITS: MAPVI: dev_id 0x%x vcol_id %d vid %d \n",
> + dev_id, vcol_id, vid);
> +
> + /* Search if device entry exists */
> + dev = vgic_its_check_device(v, dev_id);
> + if ( dev == NULL )
> + {
> + dprintk(XENLOG_ERR, "vITS: MAPVI: Fail to find device 0x%x\n",
> dev_id);
> + return 1;
> + }
> +
> + /* Check if Collection id exists */
> + if ( vgic_its_check_cid(v, vits, vcol_id, &pcol_id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: MAPVI: with wrong Collection %d\n",
> vcol_id);
> + return 1;
> + }
> + if ( vits_alloc_device_irq(dev, id, &pid, vid, vcol_id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: MAPVI: Failed to alloc irq\n");
> + return 1;
> + }
> +
> + /* Allocate irq desc for this pirq */
> + desc = irq_to_desc(pid);
> +
> + route_irq_to_guest(d, pid, "LPI");
route_irq_to_guest may fail.
> +
> + /* Assign device structure to desc data */
> + desc->arch.dev = dev;
> + desc->arch.virq = vid;
In the case of the command MAPI, the vid (i.e Physical ID field) is mark
as reserved.
From the spec, the LPI number will be the "ID" field.
> +
> + its_encode_cmd(phys_cmd, GITS_CMD_MAPVI);
> + its_encode_devid(phys_cmd, dev_id);
> +
> + if ( cmd == GITS_CMD_MAPI )
> + its_encode_event_id(phys_cmd, vid);
As said above, vid (i.e Physical ID field) is mark as reserved so you
can't use as an ID.
> + else
> + its_encode_event_id(phys_cmd, its_decode_event_id(virt_cmd));
> +
> + its_encode_phys_id(phys_cmd, pid);
> + its_encode_collection(phys_cmd, pcol_id);
> +
> + return 0;
> +}
> +
> +static int vgic_its_build_movi_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
> +{
> + uint32_t pcol_id;
> + struct its_device *dev;
> + uint32_t dev_id = its_decode_devid(virt_cmd);
> + uint8_t vcol_id = its_decode_collection(virt_cmd);
> + uint32_t id = its_decode_event_id(virt_cmd);
> +
> + DPRINTK("vITS: MOVI: dev_id 0x%x vcol_id %d\n", dev_id, vcol_id);
> + /* Search if device entry exists */
> + dev = vgic_its_check_device(v, dev_id);
> + if ( dev == NULL )
> + {
> + dprintk(XENLOG_ERR, "vITS: MOVI: Failed to find device 0x%x\n",
> dev_id);
> + return 1;
> + }
> +
> + /* Check if Collection id exists */
> + if ( vgic_its_check_cid(v, vits, vcol_id, &pcol_id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: MOVI: with wrong Collection %d\n",
> vcol_id);
> + return 1;
> + }
> +
> + if ( vgic_its_check_device_id(v, dev, id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: MOVI: Invalid ID %d\n", id);
> + return 1;
> + }
> +
> + its_encode_cmd(phys_cmd, GITS_CMD_MOVI);
> + its_encode_devid(phys_cmd, dev_id);
> + its_encode_event_id(phys_cmd, id);
> + its_encode_collection(phys_cmd, pcol_id);
> +
> + return 0;
> +}
> +
> +static int vgic_its_build_discard_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
> +{
> + struct its_device *dev;
> + uint32_t id = its_decode_event_id(virt_cmd);
> + uint32_t dev_id = its_decode_devid(virt_cmd);
> +
> + DPRINTK("vITS: DISCARD: dev_id 0x%x id %d\n", dev_id, id);
> + /* Search if device entry exists */
> + dev = vgic_its_check_device(v, dev_id);
> + if ( dev == NULL )
> + {
> + dprintk(XENLOG_ERR, "vITS: DISCARD: Failed to find device 0x%x\n",
> + dev_id);
> + return 1;
> + }
> +
> + if ( vgic_its_check_device_id(v, dev, id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: DISCARD: Invalid vID %d\n", id);
> + return 1;
> + }
> +
> + /* Check if PID is exists for this VID for this device and unmap it */
> + vgic_its_unmap_id(v, dev, id, 0);
> +
> + /* Fetch and encode cmd */
> + its_encode_cmd(phys_cmd, GITS_CMD_DISCARD);
> + its_encode_devid(phys_cmd, its_decode_devid(virt_cmd));
You can reuse id here.
> + its_encode_event_id(phys_cmd, its_decode_event_id(virt_cmd));
and reuse dev_id here.
> +
> + return 0;
> +}
> +
> +static int vgic_its_build_inv_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
> +{
> + struct its_device *dev;
> + uint32_t dev_id = its_decode_devid(virt_cmd);
> + uint32_t id = its_decode_event_id(virt_cmd);
> +
> + DPRINTK("vITS: INV: dev_id 0x%x id %d\n",dev_id, id);
> + /* Search if device entry exists */
> + dev = vgic_its_check_device(v, dev_id);
> + if ( dev == NULL )
> + {
> + dprintk(XENLOG_ERR, "vITS: INV: Failed to find device 0x%x\n",
> dev_id);
> + return 1;
> + }
> +
> + if ( vgic_its_check_device_id(v, dev, id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: INV: Invalid ID %d\n", id);
> + return 1;
> + }
> +
> + its_encode_cmd(phys_cmd, GITS_CMD_INV);
> + its_encode_devid(phys_cmd, dev_id);
> + its_encode_event_id(phys_cmd, id);
> +
> + return 0;
> +}
> +
> +static int vgic_its_build_clear_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
> +{
I suspect that this function should also clear the pending status of the
internal representation of the LPI.
> + struct its_device *dev;
> + uint32_t dev_id = its_decode_devid(virt_cmd);
> + uint32_t id = its_decode_event_id(virt_cmd);
> +
> + DPRINTK("vITS: CLEAR: dev_id 0x%x id %d\n", dev_id, id);
> + /* Search if device entry exists */
> + dev = vgic_its_check_device(v, dev_id);
> + if ( dev == NULL )
> + {
> + dprintk(XENLOG_ERR, "vITS: CLEAR: Fail to find device 0x%x\n",
> dev_id);
> + return 1;
> + }
> +
> + if ( vgic_its_check_device_id(v, dev, id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: CLEAR: Invalid ID %d\n", id);
> + return 1;
> + }
> +
> + its_encode_cmd(phys_cmd, GITS_CMD_INV);
> + its_encode_event_id(phys_cmd, id);
> +
> + return 0;
> +}
> +
> +static int vgic_its_build_invall_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
> +{
The collection may be shared between multiple domain. How this command
will impact another domain?
> + uint32_t pcol_id;
> + uint8_t vcol_id = its_decode_collection(virt_cmd);
> +
> + DPRINTK("vITS: INVALL: vCID %d\n", vcol_id);
> + /* Check if Collection id exists */
> + if ( vgic_its_check_cid(v, vits, vcol_id, &pcol_id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: INVALL: Wrong Collection %d\n", vcol_id);
> + return 1;
> + }
> +
> + its_encode_cmd(phys_cmd, GITS_CMD_INVALL);
> + its_encode_collection(phys_cmd, pcol_id);
> +
> + return 0;
> +}
> +
> +static int vgic_its_build_int_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd,
> + struct its_cmd_block *phys_cmd)
> +{
> + uint32_t dev_id = its_decode_devid(virt_cmd);
> + struct its_device *dev;
> + uint32_t id = its_decode_event_id(virt_cmd);
> +
> + DPRINTK("vITS: INT: Device 0x%x id %d\n", its_decode_devid(virt_cmd),
> id);
> + /* Search if device entry exists */
> + dev = vgic_its_check_device(v, dev_id);
> + if ( dev == NULL )
> + {
> + dprintk(XENLOG_ERR, "vITS: INT: Failed to find device 0x%x\n",
> dev_id);
> + return 1;
> + }
> +
> + if ( vgic_its_check_device_id(v, dev, id) )
> + {
> + dprintk(XENLOG_ERR, "vITS: INT: Invalid ID %d\n", id);
> + return 1;
> + }
> +
> + its_encode_cmd(phys_cmd, GITS_CMD_INT);
> + its_encode_devid(phys_cmd, its_decode_devid(virt_cmd));
> + its_encode_event_id(phys_cmd, its_decode_event_id(virt_cmd));
> +
> + return 0;
> +}
> +
> +static void vgic_its_free_device(struct its_device *dev)
> +{
> + xfree(dev);
> +}
> +
> +static int vgic_its_add_device(struct vcpu *v, struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd)
The function name is wrong. you are not only add a device but also
remove it.
Given the size of the function it would be good to split in two parts.
[..]
> +
> +static int vgic_its_process_mapc(struct vcpu *v, struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd)
> +{
> + uint32_t pcid = 0;
> + int idx;
uint32_t as nmap.
> + uint32_t nmap;
> + uint8_t vcol_id;
> + uint64_t vta = 0;
> +
> + nmap = vits->cid_map.nr_cid;
> + vcol_id = its_decode_collection(virt_cmd);
> + vta = its_decode_target(virt_cmd);
> +
> + for ( idx = 0; idx < nmap; idx++ )
> + {
> + if ( vcol_id == vits->cid_map.vcid[idx] )
> + break;
> + }
> + if ( idx == nmap )
> + vits->cid_map.vcid[idx] = vcol_id;
You array has a specific size, so you need to check that idx is not
greater that this size.
> +
> + if ( its_get_physical_cid(v->domain, &pcid, vta) )
> + BUG_ON(1);
No BUG_ON().
> + vits->cid_map.pcid[idx] = pcid;
> + vits->cid_map.vta[idx] = vta;
> + vits->cid_map.nr_cid++;
> + DPRINTK("vITS: MAPC: vCID %d vTA 0x%lx added @idx 0x%x \n",
> + vcol_id, vta, idx);
AFAIU, a collection = a pCPU. As the vCPU can move from a pCPU to
another. How do you plan to handle interrupt migration?
> +
> + return 0;
> +}
> +
> +static void vgic_its_update_read_ptr(struct vcpu *v, struct vgic_its *vits)
> +{
> + vits->cmd_read = vits->cmd_write;
> +}
> +
> +#ifdef DEBUG_ITS
> +char *cmd_str[] = {
> + [GITS_CMD_MOVI] = "MOVI",
> + [GITS_CMD_INT] = "INT",
> + [GITS_CMD_CLEAR] = "CLEAR",
> + [GITS_CMD_SYNC] = "SYNC",
> + [GITS_CMD_MAPD] = "MAPD",
> + [GITS_CMD_MAPC] = "MAPC",
> + [GITS_CMD_MAPVI] = "MAPVI",
> + [GITS_CMD_MAPI] = "MAPI",
> + [GITS_CMD_INV] = "INV",
> + [GITS_CMD_INVALL] = "INVALL",
> + [GITS_CMD_MOVALL] = "MOVALL",
> + [GITS_CMD_DISCARD] = "DISCARD",
> + };
> +#endif
> +
> +#define SEND_NONE 0x0
> +#define SEND_CMD 0x1
> +#define SEND_ALL 0x2
> +
> +static int vgic_its_parse_its_command(struct vcpu *v, struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd)
> +{
> + uint8_t cmd = its_decode_cmd(virt_cmd);
> + struct its_cmd_block phys_cmd;
> + int ret;
> + int send_flag = SEND_CMD;
> +
> +#ifdef DEBUG_ITS
> + DPRINTK("vITS: Received cmd %s (0x%x)\n", cmd_str[cmd], cmd);
> + DPRINTK("Dump Virt cmd: ");
> + dump_cmd(virt_cmd);
> +#endif
> +
> + memset(&phys_cmd, 0x0, sizeof(struct its_cmd_block));
> + switch ( cmd )
> + {
> + case GITS_CMD_MAPD:
> + /* create virtual device entry */
> + if ( vgic_its_add_device(v, vits, virt_cmd) )
> + return ENODEV;
> + ret = vgic_its_build_mapd_cmd(v, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_MAPC:
> + /* Physical ITS driver already mapped physical Collection */
> + send_flag = SEND_NONE;
> + ret = vgic_its_process_mapc(v, vits, virt_cmd);
> + break;
> + case GITS_CMD_MAPI:
> + /* MAPI is same as MAPVI */
This message is not true. MAPI and MAPVI doesn't have the same layout...
> + case GITS_CMD_MAPVI:
> + ret = vgic_its_build_mapvi_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_MOVI:
> + ret = vgic_its_build_movi_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_DISCARD:
> + ret = vgic_its_build_discard_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_INV:
> + ret = vgic_its_build_inv_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_INVALL:
> + /* XXX: SYNC is sent on all physical ITS */
I guess you meant to INVALL.
XXX means "FIXME", so what do you need to fix?
> + send_flag = SEND_ALL;
> + ret = vgic_its_build_invall_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_INT:
> + ret = vgic_its_build_int_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_CLEAR:
> + ret = vgic_its_build_clear_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + case GITS_CMD_SYNC:
> + /* XXX: SYNC is sent on all physical ITS */
Ditto for the "XXX".
> + send_flag = SEND_ALL;
> + ret = vgic_its_build_sync_cmd(v, vits, virt_cmd, &phys_cmd);
> + break;
> + /*TODO: GITS_CMD_MOVALL not implemented */
> + default:
> + dprintk(XENLOG_ERR, "vITS: Unhandled command cmd %d\n", cmd);
> + return 1;
> + }
> +
> +#ifdef DEBUG_ITS
> + DPRINTK("Dump Phys cmd: ");
> + dump_cmd(&phys_cmd);
> +#endif
> +
> + if ( ret )
> + {
> + dprintk(XENLOG_ERR, "vITS: Failed to handle cmd %d\n", cmd);
> + return 1;
> + }
> +
> + if ( send_flag )
> + {
> + /* XXX: Always send on physical ITS on which device is assingned */
Ditto XXX and s/assigngned/assigned/
> + if ( !gic_its_send_cmd(v,
> + its_get_phys_node(its_decode_devid(&phys_cmd)),
> + &phys_cmd, (send_flag & SEND_ALL)) )
> + {
> + dprintk(XENLOG_ERR, "vITS: Failed to push cmd %d\n", cmd);
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* Called with its lock held */
Please add an ASSERT to validate this assumption.
> +static int vgic_its_read_virt_cmd(struct vcpu *v,
> + struct vgic_its *vits,
> + struct its_cmd_block *virt_cmd)
> +{
> + struct page_info * page;
page_info *page;
> + void *p;
> + paddr_t paddr;
> + paddr_t maddr = vits->cmd_base & 0xfffffffff000UL;
Define a mask would make the code clearer.
Also I don't see any check on GITS_CBASER.Valid in this code and in the
register emulation.
If GITS_CBASER.Valid = 0, command should be ignore.
> + uint64_t offset;
> +
> + /* CMD Q can be more than 1 page. Map only page that is required */
> + maddr = ((vits->cmd_base & 0xfffffffff000UL) +
> + vits->cmd_write_save ) & PAGE_MASK;
> +
> + paddr = p2m_lookup(v->domain, maddr, NULL);
p2m_lookup can fail and return INVALID_PADDR.
> +
> + DPRINTK("vITS: Mapping CMD Q maddr 0x%lx paddr 0x%lx write_save 0x%lx
> \n",
> + maddr, paddr, vits->cmd_write_save);
> + page = get_page_from_paddr(v->domain, paddr, 0);
> + if ( page == NULL )
> + {
> + dprintk(XENLOG_ERR, "vITS: Failed to get command page\n");
> + return 1;
> + }
Althoug you may want to use get_page_from_gfn rather than p2m_lookup +
get_page_from_paddr.
> +
> + p = __map_domain_page(page);
__map_domain_page will map the page cacheable. What happen if the guest
decides to use a non-cacheable mapping? I think this will corrupt the
commands quickly.
I'm not against using __map_domain_page but we should at least detect
and print an error message if the caching attribute are different.
> +
> + /* Offset within the mapped 4K page to read */
> + offset = vits->cmd_write_save & 0xfff;
> +
> + memcpy(virt_cmd, p + offset, sizeof(struct its_cmd_block));
> +
> + /* No command queue is created by vits to check on Q full */
> + vits->cmd_write_save += 0x20;
0x20 is confusing. You should use sizeof (struct its_cmd_block) or add a
command to explain the 0x20.
Although I would add a BUILD_BUG_ON(sizeof (struct its_cmd_block) == 32);
> + if ( vits->cmd_write_save == vits->cmd_qsize )
> + {
> + DPRINTK("vITS: Reset write_save 0x%lx qsize 0x%lx \n",
> + vits->cmd_write_save,
> + vits->cmd_qsize);
> + vits->cmd_write_save = 0x0;
> + }
> +
> + unmap_domain_page(p);
> + put_page(page);
> +
> + return 0;
> +}
> +
> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
Missing a static
> +{
> + struct its_cmd_block virt_cmd;
> +
> + /* XXX: Currently we are processing one cmd at a time */
This comment seems wrong. You are handling multiple commands at the same
time.
> + ASSERT(spin_is_locked(&vits->lock));
> +
> + do {
> + if ( vgic_its_read_virt_cmd(v, vits, &virt_cmd) )
> + goto err;
> + if ( vgic_its_parse_its_command(v, vits, &virt_cmd) )
> + goto err;
From the Spec: if a command error occur, the command should be ignore.
If the command queue is not empty, we must continue processing commands.
> + } while ( vits->cmd_write != vits->cmd_write_save );
> +
> + vits->cmd_write_save = vits->cmd_write;
Why this line? At the end of the loop cmd_write_save is equals to
write_save.
> + DPRINTK("vITS: write_save 0x%lx write 0x%lx \n",
> + vits->cmd_write_save,
> + vits->cmd_write);
> + /* XXX: Currently we are processing one cmd at a time */
Ditto for the command.
> + vgic_its_update_read_ptr(v, vits);
> +
> + dsb(ishst);
Why the dsb?
> +
> + return 1;
> +err:
> + dprintk(XENLOG_ERR, "vITS: Failed to process guest cmd\n");
> + return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 9e0419e..bc7aee9 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -114,6 +114,15 @@ struct arch_domain
> #endif
> } vgic;
>
> + struct vgic_its *vits;
> + struct vgic_lpi_conf *lpi_conf;
This patch is handling command not lpi_conf.
It seems that you mix the skeleton of VGIC with the implementation of
the command queue...
It would make more sense to have a separate patch introducing the vGIC
skeleton.
> +
> + struct vits_devs {
> + spinlock_t lock;
> + /* ITS Device list */
> + struct list_head dev_list;
> + } vits_devs;
> +
> struct vuart {
> #define VUART_BUF_SIZE 128
> char *buf;
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index fa1e305..70ec913 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -22,6 +22,72 @@
> #ifndef __ASM_ARM_GIC_ITS_H__
> #define __ASM_ARM_GIC_ITS_H__
>
> +#include <asm/gic_v3_defs.h>
> +
> +struct its_node;
> +
> +/* Collection ID mapping */
> +struct cid_mapping
> +{
> + uint8_t nr_cid;
> + /* XXX: assume one collection id per vcpu. can set to MAX_VCPUS? */
MAX_VCPUS can be very high, so this has to be allocated dynamically
per-domain.
> + /* Virtual Collection id */
> + uint8_t vcid[32];
> + /* Physical Collection id */
> + uint8_t pcid[32];
> + /* Virtual target address of this collection id */
> + uint64_t vta[32];
It would have been easier to understand the matching with a structure
containing vcid, pcid, vta.
> +};
> +
> +/*
> + * Per domain virtual ITS structure.
> + * One per Physical ITS node available for the domain
> + */
> +
> +struct vgic_its
> +{
> + spinlock_t lock;
> + /* Emulation of BASER */
> + paddr_t baser[8];
> + /* Command queue base */
> + paddr_t cmd_base;
> + /* Command queue write pointer */
> + paddr_t cmd_write;
> + /* Command queue write saved pointer */
> + paddr_t cmd_write_save;
> + /* Command queue read pointer */
> + 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;
> + /* ITS physical node */
> + struct its_node *its;
> + /* GICR ctrl register */
> + uint32_t ctrl;
> + /* Virtual to Physical Collection id mapping */
> + struct cid_mapping cid_map;
> +};
Most of this code doesn't belong to the command queue implementation...
> +
> +struct vgic_lpi_conf
> +{
> + /* LPI propbase */
> + paddr_t propbase;
> + /* percpu pendbase */
> + paddr_t pendbase[MAX_VIRT_CPUS];
Please allocate it dynamically.
> + /* Virtual LPI property table */
> + void * prop_page;
> +};
> +
Ditto for this structure.
> +struct vid_map
> +{
Please add comment in this structure.
> + uint32_t vlpi;
> + uint32_t plpi;
> + uint32_t id;
"id" means lots of thing: devID, eventID, pID... Please rename this
field to something more meaningful.
> +};
> +
> /*
> * The ITS command block, which is what the ITS actually parses.
> */
> @@ -37,12 +103,21 @@ struct its_device {
> struct list_head entry;
> struct its_node *its;
> struct its_collection *collection;
> - void *itt;
Why did you introduce itt if you dropped it here?
> + /* Virtual ITS node */
> + struct vgic_its *vits;
> + paddr_t itt_addr;
> + unsigned long itt_size;
> unsigned long *lpi_map;
> u32 lpi_base;
> int nr_lpis;
> u32 nr_ites;
> u32 device_id;
> + /* Spinlock for vlpi allocation */
> + spinlock_t vlpi_lock;
> + /* vlpi bitmap */
> + unsigned long *vlpi_map;
> + /* vlpi <=> plpi mapping */
> + struct vid_map *vlpi_entries;
> };
Ditto about the patch splitting for this structure.
>
> static inline uint8_t its_decode_cmd(struct its_cmd_block *cmd)
> @@ -144,6 +219,15 @@ static inline void its_encode_collection(struct
> its_cmd_block *cmd, u16 col)
> cmd->raw_cmd[2] |= col;
> }
>
> +int its_get_physical_cid(struct domain *d, uint32_t *col_id, uint64_t ta);
> +int its_get_target(uint8_t pcid, uint64_t *pta);
> +int its_alloc_device_irq(struct its_device *dev, uint32_t *plpi);
> +int gic_its_send_cmd(struct vcpu *v, struct its_node *its,
> + struct its_cmd_block *phys_cmd, int send_all);
> +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);
> +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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |