From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support Date: Wed, 15 Jul 2015 14:57:09 +0200 Message-ID: <55A658A5.4000605@citrix.com> References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com> <1436514172-3263-8-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436514172-3263-8-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, Vijaya Kumar K , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 10/07/2015 09:42, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > Add Virtual ITS command processing support to Virtual ITS driver > > Signed-off-by: Vijaya Kumar K > --- > v4: - Use helper function to read from command queue > - Add MOVALL > - Removed check for entry in device in domain RB-tree > --- > xen/arch/arm/gic-v3-its.c | 7 + > xen/arch/arm/vgic-v3-its.c | 391 +++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/gic-its.h | 19 ++ > xen/include/asm-arm/gic.h | 1 + > 4 files changed, 418 insertions(+) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index b98d396..9161053 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -91,6 +91,7 @@ static LIST_HEAD(its_nodes); > static DEFINE_SPINLOCK(its_lock); > static struct rdist_prop *gic_rdists; > static struct rb_root rb_its_dev; > +static struct gic_its_info its_data; > > #define gic_data_rdist() (per_cpu(rdist, smp_processor_id())) > > @@ -102,6 +103,11 @@ void dump_cmd(its_cmd_block *cmd) > } > #endif > > +u32 its_get_nr_events(void) > +{ > + return (1 << its_data.id_bits); > +} > + Please give a look to the new vgic infrastructure in order to avoid introduced helper to pass data to the vgic. See for instance vgic_v3_setup_hw. > /* RB-tree helpers for its_device */ > struct its_device *its_find_device(u32 devid) > { > @@ -940,6 +946,7 @@ static int its_probe(struct dt_device_node *node) > its->phys_size = its_size; > typer = readl_relaxed(its_base + GITS_TYPER); > its->ite_size = ((typer >> 4) & 0xf) + 1; > + its_data.id_bits = GITS_TYPER_IDBITS(typer); > > its->cmd_base = xzalloc_bytes(ITS_CMD_QUEUE_SZ); > if ( !its->cmd_base ) > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index c63f478..af2bacd 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -31,6 +31,22 @@ > #include > #include > > +#define DEBUG_ITS > + > +#ifdef DEBUG_ITS > +# define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args) > +#else > +# define DPRINTK(fmt, args...) do {} while ( 0 ) > +#endif > + > +#ifdef DEBUG_ITS > +static void dump_cmd(its_cmd_block *cmd) > +{ > + printk("CMD[0] = 0x%lx CMD[1] = 0x%lx CMD[2] = 0x%lx CMD[3] = 0x%lx\n", > + cmd->bits[0], cmd->bits[1], cmd->bits[2], cmd->bits[3]); > +} > +#endif > + > static int vits_entry(struct domain *d, paddr_t entry, void *addr, > uint32_t size, bool_t set) > { > @@ -202,6 +218,381 @@ void vits_remove_device(struct rb_root *root, struct vits_device *dev) > rb_erase(&dev->node, root); > } > > +static int vgic_its_process_sync(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ While the "XXX" wasn't valid, the comment "ignored" was still valid... > + DPRINTK("%pv: vITS: SYNC: ta 0x%x \n", v, virt_cmd->sync.ta); > + > + return 0; > +} > + > +static int vgic_its_process_mapvi(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ > + struct vitt entry; > + struct domain *d = v->domain; > + uint8_t vcol_id, cmd; > + uint32_t vid, dev_id, event; > + > + vcol_id = virt_cmd->mapvi.col; > + vid = virt_cmd->mapvi.phy_id; > + cmd = virt_cmd->mapvi.cmd; > + dev_id = virt_cmd->mapvi.devid; > + > + DPRINTK("%pv: vITS: MAPVI: dev_id 0x%x vcol_id %d vid %d \n", > + v, dev_id, vcol_id, vid); > + > + if ( vcol_id > (d->max_vcpus + 1) || vid > its_get_nr_events() ) > + return -EINVAL; As said on v3, checking the validity is pointless as a malicious guest can rewrite the ITT. We only need to check it when the LPI is effectively injected. If you think this is necessary please explain why... Furthermore, its_get_nr_events is for the hardware and not the virtual ITS. I would prefer to see a field in the vits structure which contains the number of event ID bits for a given domain. [...] > +static int vgic_its_process_movi(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ > + struct vitt entry; > + struct domain *d = v->domain; > + uint32_t dev_id, event; > + uint8_t vcol_id; > + > + vcol_id = virt_cmd->movi.col; > + event = virt_cmd->movi.event; > + dev_id = virt_cmd->movi.devid; > + > + DPRINTK("%pv vITS: MOVI: dev_id 0x%x vcol_id %d event %d\n", > + v, dev_id, vcol_id, event); > + > + if ( vcol_id > (d->max_vcpus + 1) || event > its_get_nr_events() ) > + return -EINVAL; My comment on the check in the previous function is valid here too. > + > + if ( vits_get_vitt_entry(d, dev_id, event, &entry) ) > + return -EINVAL; > + > + entry.vcollection = vcol_id; > + > + if ( vits_set_vitt_entry(d, dev_id, event, &entry) ) > + return -EINVAL; > + > + return 0; > +} > + > +static int vgic_its_process_movall(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ /* Ignored */ DPRINTK("%pv vITS: MOVALL: ....",...); > + return 0; > +} > + > +static int vgic_its_process_discard(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ > + struct vitt entry; > + struct domain *d = v->domain; > + uint32_t event, dev_id; > + > + event = virt_cmd->discard.event; > + dev_id = virt_cmd->discard.devid; > + > + DPRINTK("%pv vITS: DISCARD: dev_id 0x%x id %d\n", > + v, virt_cmd->discard.devid, event); > + > + if ( event > its_get_nr_events() ) > + return -EINVAL; Ditto for the check. > + > + if ( vits_get_vitt_entry(d, dev_id, event, &entry) ) > + return -EINVAL; > + > + entry.valid = false; > + > + if ( vits_set_vitt_entry(d, dev_id, event, &entry) ) > + return -EINVAL; > + > + return 0; > +} > + > +static int vgic_its_process_inv(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ Please add /* Ignored */ > + DPRINTK("%pv vITS: INV: dev_id 0x%x id %d\n", > + v, virt_cmd->inv.devid, virt_cmd->inv.event); > + > + return 0; > +} > + > +static int vgic_its_process_clear(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ /* Ignored */ > + DPRINTK("%pv: vITS: CLEAR: dev_id 0x%x id %d\n", > + v, virt_cmd->clear.devid, virt_cmd->clear.event); > + > + return 0; > +} > + > +static int vgic_its_process_invall(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ /* Ignored */ > + DPRINTK("%pv: vITS: INVALL: vCID %d\n", v, virt_cmd->invall.col); > + > + return 0; > +} > + > +static int vgic_its_process_int(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ > + struct vitt vitt_entry; > + struct domain *d = v->domain; > + uint32_t event, dev_id, col_id; > + > + event = virt_cmd->int_cmd.cmd; > + dev_id = virt_cmd->int_cmd.devid; > + > + DPRINTK("%pv: vITS: INT: Device 0x%x id %d\n", v, dev_id, event); > + if ( event > its_get_nr_events() ) > + return -EINVAL; Ditto for the check. > + > + if ( vits_get_vitt_entry(d, dev_id, event, &vitt_entry) ) > + return -EINVAL; > + > + if ( !vitt_entry.valid ) > + { > + dprintk(XENLOG_G_ERR, > + "%pv: vITS: INT CMD invalid event %d for dev 0x%x\n", > + v, event, dev_id); > + return -EINVAL; > + } > + > + col_id = vitt_entry.vcollection; > + if ( col_id < d->max_vcpus ) > + { > + dprintk(XENLOG_G_ERR, > + "%pv: vITS: INT CMD invalid col_id %d for dev 0x%x\n", > + v, col_id, dev_id); > + return -EINVAL; > + } > + > + vgic_vcpu_inject_irq(d->vcpu[col_id], vitt_entry.vlpi); As said on v3, the design document [1] suggested to implement the INT command using vgic_vcpu_inject_lpi. Is there any issue to do it? Also, you have to translate the col_id into to a VCPU ID. > + > + return 0; > +} > + > +static int vgic_its_add_device(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ > + struct domain *d = v->domain; > + struct vdevice_table dt_entry; > + uint32_t dev_id = virt_cmd->mapd.devid; > + > + DPRINTK("%pv: vITS: Add device dev_id 0x%x vitt_ipa = 0x%lx size %d\n", > + v, dev_id, (u64)virt_cmd->mapd.itt << 8, > + virt_cmd->mapd.size); > + > + if ( virt_cmd->mapd.valid ) > + { > + /* itt field is 40 bit. extract 48 bit address by shifting */ > + dt_entry.vitt_ipa = virt_cmd->mapd.itt << 8; Could you introduce a define for the 8? It would be more clear than open-coding the value twice in the same function. > + dt_entry.vitt_size = (1 << (virt_cmd->mapd.size + 1)) * > + sizeof(struct vitt); > + } > + else > + { > + dt_entry.vitt_ipa = INVALID_PADDR; > + dt_entry.vitt_size = 0; > + } > + > + if ( vits_set_vdevice_entry(d, dev_id, &dt_entry) ) > + return -EINVAL; > + > + return 0; > +} > + > +static int vgic_its_process_mapc(struct vcpu *v, struct vgic_its *vits, > + its_cmd_block *virt_cmd) > +{ > + struct domain *d = v->domain; > + uint8_t vcol_id; > + uint64_t vta = 0; > + > + vcol_id = virt_cmd->mapc.col; > + vta = virt_cmd->mapc.ta; > + > + DPRINTK("%pv: vITS: MAPC: vCID %d vTA 0x%lx valid %d \n", > + v, vcol_id, vta, virt_cmd->mapc.valid); > + > + if ( vcol_id > (d->max_vcpus + 1) || vta > v->domain->max_vcpus ) > + return -EINVAL; The target address doesn't have to be valid when the collection is unmapped. [...] > +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits) Missing static, the function will never be called outside of this file. > +{ > + its_cmd_block virt_cmd; > + > + 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; > + vgic_its_update_read_ptr(v, vits); > + } while ( vits->cmd_write != vits->cmd_write_save ); > + > + DPRINTK("%pv: vITS: write_save 0x%lx write 0x%lx\n", > + v, vits->cmd_write_save, > + vits->cmd_write); > + > + return 1; > +err: > + dprintk(XENLOG_G_ERR, "%pv: vITS: Failed to process guest cmd\n", v); > + /*XXX: Be nice to guest though we cannot process command? */ /* ... */ It looks like to me we want to crash the guest using domain_crash_synchronous. > + return 0; > +} > + > /* > * Local variables: > * mode: C [..] > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 44c2317..fdd96c8 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -24,6 +24,7 @@ > #define NR_GIC_LPI 4096 > #define MAX_LPI (FIRST_GIC_LPI + NR_GIC_LPI) > #define MAX_RDIST_COUNT 4 > +#define BIT_48_12_MASK 0xfffffffff000UL This shouldn't be part of gic.h but gic-its.h Regards, -- Julien Grall