Re: [Xen-devel] [RFC PATCH v3 08/18] xen/arm: vITS: Add virtual ITS driver
On Thu, 2015-07-02 at 19:03 +0530, Vijay Kilari wrote:
> Hi Julien,
>
> On Tue, Jun 23, 2015 at 10:09 PM, Julien Grall
> wrote:
> > Hi Vijay,
> >
> >>
> >> +struct vits_device {
> >> +uint32_t vdevid;
> >> +uint32_t pdevid;
> >> +struct its_device *its_dev;
> >> +struct rb_node node;
> >> +};
> >
> > We spoke about a specific structure in the design [2] but you introduced
> > a new one. Why?
>
> Section 6 of DraftG specifies to manage separate tree for device assignment.
> This helps to manage RB-tree per domain to hold list of devices
> assigned to this domain index with vdevid.
>
> This helps to check if device is assigned to this domain before processing
> any ITS command with that vdevid.
>
> >
> > Having everything in the its_device would help to catch a device
> > attached to 2 different domains...
>
> One option is to introduce a new variable inside its_device to know
> to which domain the device is currently assigned.
IIRC that's what I intended, e.g. two trees referencing the same
underlying data structure. Sorry that wasn't clear.
>
> >
> > Also, the field pdevid is not vits specific but its.
> pdevid can be removed as its_device structure already has it
>
> Regards
> Vijay
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v3 08/18] xen/arm: vITS: Add virtual ITS driver
Hi Julien,
On Tue, Jun 23, 2015 at 10:09 PM, Julien Grall wrote:
> Hi Vijay,
>
>>
>> +struct vits_device {
>> +uint32_t vdevid;
>> +uint32_t pdevid;
>> +struct its_device *its_dev;
>> +struct rb_node node;
>> +};
>
> We spoke about a specific structure in the design [2] but you introduced
> a new one. Why?
Section 6 of DraftG specifies to manage separate tree for device assignment.
This helps to manage RB-tree per domain to hold list of devices
assigned to this domain index with vdevid.
This helps to check if device is assigned to this domain before processing
any ITS command with that vdevid.
>
> Having everything in the its_device would help to catch a device
> attached to 2 different domains...
One option is to introduce a new variable inside its_device to know
to which domain the device is currently assigned.
>
> Also, the field pdevid is not vits specific but its.
pdevid can be removed as its_device structure already has it
Regards
Vijay
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v3 08/18] xen/arm: vITS: Add virtual ITS driver
On Mon, 2015-06-22 at 17:31 +0530, vijay.kil...@gmail.com wrote:
> +if ( !p2m_is_ram(p2mt) )
> +{
> +put_page(page);
> +dprintk(XENLOG_G_ERR, "vITS:d%dv%d with wrong attributes\n",
> +d->domain_id, current->vcpu_id);
"d%dv%d" should be done (everywhere) with the "%pv" format code passing
current as the argument. See docs/misc/printk-formats.txt.
> +return -EINVAL;
> +return -EINVAL;
> +}
> +
> +p = __map_domain_page(page);
> +/* Offset within the mapped page */
> +offset = dt_entry & (PAGE_SIZE - 1);
This should be "& ~PAGE_MASK" please.
> +
> +if ( set )
> +memcpy(p + offset, entry, sizeof(struct vdevice_table));
> +else
> +memcpy(entry, p + offset, sizeof(struct vdevice_table));
Personally I'd have done this with *entry = *(struct ..*)(p + offset)
and vice versa and let the compiler figure out how to achieve that.
> +/* dt_entry is validated when read */
The vits_get_vdevice_entry call is right above and in this
implementation I don't see any checks in that function, so I don't think
this comment is strictly true.
Since it is very important that information living in this guest
accessible memory is correctly validated before use I think these
comments need to be 100% trustworthy.
I think vits_get_vdevice_entry is indeed the right place for those
checks as the comment suggests.
If there are no actual checks needed (e.g. because everything is in
terms of IPA) then there should be a comment in that function explaining
exactly why nothing actually needs to be validated.
> +offset = event * sizeof(struct vitt);
> +if ( offset > dt_entry.vitt_size )
> +{
> +dprintk(XENLOG_G_ERR, "vITS:d%dv%d ITT out of range\n",
> +d->domain_id, current->vcpu_id);
> +return -EINVAL;
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v3 08/18] xen/arm: vITS: Add virtual ITS driver
Hi, On 22/06/15 13:01, vijay.kil...@gmail.com wrote: > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index f1a087e..da73cf5 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -115,6 +115,8 @@ struct arch_domain > #endif > } vgic; > > +struct vgic_its *vits; > + I forgot to mention that this should only be compiled when GICv3 is used (i.e only for ARM64). Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v3 08/18] xen/arm: vITS: Add virtual ITS driver
Hi Vijay,
On 22/06/15 13:01, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K
>
> This patch introduces virtual ITS driver with following
> functionality
> - Introduces helper functions to manage device table and
>ITT table in guest memory
> - Helper function to handle virtual ITS devices assigned
>to domain
>
> Signed-off-by: Vijaya Kumar K
> ---
> xen/arch/arm/Makefile |1 +
> xen/arch/arm/vgic-v3-its.c| 266
> +
> xen/include/asm-arm/domain.h |2 +
> xen/include/asm-arm/gic-its.h | 49
> 4 files changed, 318 insertions(+)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 1821ed2..8590846 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -33,6 +33,7 @@ obj-y += traps.o
> obj-y += vgic.o vgic-v2.o
> obj-$(CONFIG_ARM_64) += vgic-v3.o
> obj-$(CONFIG_ARM_64) += gic-v3-its.o
> +obj-$(CONFIG_ARM_64) += vgic-v3-its.o
I would prefer to the vgic-v3-its code not compiled until you finish to
implement it rather than having every function exported while it should
be static.
> obj-y += vtimer.o
> obj-y += vuart.o
> obj-y += hvm.o
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> new file mode 100644
> index 000..ea52a87
> --- /dev/null
> +++ b/xen/arch/arm/vgic-v3-its.c
[..]
> +/* GITS register definitions */
> +#define VITS_GITS_TYPER_HCC (0xffU << 24)
Where does this value comes from? The number of supported collection
should be the maximum number of VCPUs supported by the domain.
Note that the hardware can only hold 256 collections, all the other
requires provisioning. But it's fine for now, although a BUILD_BUG_ON in
the vGIC code would be nice.
> +#define VITS_GITS_TYPER_PTA_SHIFT (19)
> +#define VITS_GITS_DEV_BITS(0x14U << 13)
> +#define VITS_GITS_ID_BITS (0x13U << 8)
Where do those values come from? This needs at least to come from the
hardware GIC configuration if we plan to only support DOM0 for now.
> +#define VITS_GITS_ITT_SIZE(0x7U << 4)
The name is misleading, this is not the size of the ITT but the value to
store in the GITS_TYPER register.
> +#define VITS_GITS_DISTRIBUTED (0x1U << 3)
Why? This bit is implementation defined in the spec.
> +#define VITS_GITS_PLPIS (0x1U << 0)
For all these defines, why do you hardcode the shift rather using
GITS_TYPER_* you provide in patch #5? It would make the code more easier
to understand.
> +
> +/* GITS_PIDRn register values for ARM implementations */
> +#define GITS_PIDR0_VAL(0x94)
> +#define GITS_PIDR1_VAL(0xb4)
> +#define GITS_PIDR2_VAL(0x3b)
> +#define GITS_PIDR3_VAL(0x00)
> +#define GITS_PIDR4_VAL(0x04)
> +
> +// #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->raw_cmd[0], cmd->raw_cmd[1], cmd->raw_cmd[2],
> cmd->raw_cmd[3]);
> +}
> +#endif
> +
> +/* ITS device table helper functions */
> +int vits_vdevice_entry(struct domain *d, uint32_t dev_id,
> + struct vdevice_table *entry, int set)
Any functions not exported should be static. I won't repeat for all the
others within this file.
Also, please use bool_t for set.
> +{
> +uint64_t offset;
> +paddr_t dt_entry;
> +struct page_info *page;
> +p2m_type_t p2mt;
> +void *p;
> +
> +offset = dev_id * sizeof(struct vdevice_table);
> +if ( offset > d->arch.vits->dt_size )
If you gonna let the guest to write deci
> +{
> +dprintk(XENLOG_G_ERR,
> +"vITS:d%dv%d: Out of range offset 0x%lx id 0x%x size
> 0x%lx\n",
You can use %pv to print the domain/vcpuid (see an example in vgic-v3.c).
Also, all the value can be printed in decimal because we have to use
them for addition...
> +d->domain_id, current->vcpu_id, offset, dev_id,
Please don't mix d and current.
> +d->arch.vits->dt_size);
> +return -EINVAL;
> +}
> +
> +dt_entry = d->arch.vits->dt_ipa + offset;
> +
> +page = get_page_from_gfn(d, paddr_to_pfn(dt_entry), &p2mt, P2M_ALLOC);
> +if ( !page )
> +{
> +dprintk(XENLOG_G_ERR, "vITS:d%dv%d Failed to get VITT device
> table\n",
> +d->domain_id, current->vcpu_id);
> +return -EINVAL;
> +}
> +
> +if ( !p2m_is_ram(p2mt) )
> +{
> +put_page(page);
> +dprintk(XENLOG_G_ERR, "vITS:d%dv%d with wrong attributes\n",
> +d->domain_id, current->vcpu_id);
> +return -EINVAL;
> +return -EINVAL;
Duplicate "return -EINVAL".
> +}
> +
> +p = __map_domain_page(page);
> +/* Offset within the mapped page */
> +offs
