Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On 17/07/15 16:34, Ian Campbell wrote: > On Fri, 2015-07-17 at 16:15 +0100, Julien Grall wrote: > Also, you have to translate the col_id into to a VCPU ID. >>> >>> This is virtual collection id which itself is vcpu id. isn't it? >> >> No. A collection can move between CPU, so you will end up to have vCPUID >> != vCID. > > Specifically it is a TA (TargetAddress) and not a collection which is > equal to a VCPU ID (in our implementation, because we have chosen to > implement GITS_TYPER.PTA==0). > > A collection is mapped onto a TA by MAPC command. > > Thus some sort of record of which collection is mapped to which > TA(==vcpuid) is required. Correct, I shouldn't have take shortcut while writing my previous mail. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On Fri, 2015-07-17 at 16:15 +0100, Julien Grall wrote: > >> > >> Also, you have to translate the col_id into to a VCPU ID. > > > > This is virtual collection id which itself is vcpu id. isn't it? > > No. A collection can move between CPU, so you will end up to have vCPUID > != vCID. Specifically it is a TA (TargetAddress) and not a collection which is equal to a VCPU ID (in our implementation, because we have chosen to implement GITS_TYPER.PTA==0). A collection is mapped onto a TA by MAPC command. Thus some sort of record of which collection is mapped to which TA(==vcpuid) is required. > Your MAPC command is correctly storing the mapping in collections, so > please use it to retrieve the correct vCPU. Indeed. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On 17/07/15 15:12, Vijay Kilari wrote:
> Hi Julien,
Hi Vijay,
> On Wed, Jul 15, 2015 at 6:27 PM, Julien Grall wrote:
>> On 10/07/2015 09:42, vijay.kil...@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
>>> ---
> [..]
>>> +
>>> +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...
>
>vcol_id is read from ITS command but not from guest memory.
> So command values are validated here instead of doing at time time
> of LPI injection.
>
> If not done here, then we still allow malicious guest to run and during
> LPI injection if invalid col_id is found in ITT we just drop LPI.
You are storing the collection in the ITT. The ITT is part of the guest
memory.
The guest can modify at his convenience the region before we receive an
LPI and handle it. So we *must* check the validity of the collection
(and anything else in the ITT entry) everytime we receveive an LPI.
Therefore this check here is pointless.
>>
>> 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.
>
> Why do we need to restrict number of LPIs to domain?
Why do you speak about LPIs here? its_get_nr_events return the maximum
number of eventID and not intID. LPIs are validate against the later.
Although, to answer to your question, the reason are various, one of
them is to save Xen memory, everytime we are creating domain we have to
allocate a pending_irq structure per vLPIs.
It's pointless to allocate all of them if we know that only a few will
be used.
FWIW, we are doing the same for SPIs.
> [...]
>
>>> +
>>> +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?
>
> IIRC, INT command contains vlpi which does not have its_device because
> irq_desc
> is not reserved for this. Hence it is handled similar to event_ch int.
Why do you need an irq_desc in vgic_vcpu_inject_lpi? If you follow what
Ian suggested on patch #12 [1], you can directly call the function and
avoid to duplicate most of the code.
>
>>
>> Also, you have to translate the col_id into to a VCPU ID.
>
> This is virtual collection id which itself is vcpu id. isn't it?
No. A collection can move between CPU, so you will end up to have vCPUID
!= vCID.
Your MAPC command is correctly storing the mapping in collections, so
please use it to retrieve the correct vCPU.
Regards,
[1] http://lists.xen.org/archives/html/xen-devel/2015-07/msg02160.html
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
Hi Ian, On Mon, Jul 13, 2015 at 5:07 PM, Ian Campbell wrote: > On Mon, 2015-07-13 at 16:45 +0530, Vijay Kilari wrote: > >> BTW, I want to know if atomic_t supports 64-bit access?. > > I don't know off the top of my head. I'm sure it would be apparent in > the code. atomic_t is 32 bit. But CREADER is 64-bit register. But only bit[19:0] are valid. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
Hi Julien,
On Wed, Jul 15, 2015 at 6:27 PM, Julien Grall wrote:
> Hi Vijay,
>
> On 10/07/2015 09:42, vijay.kil...@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
>> ---
[..]
>> +
>> +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...
vcol_id is read from ITS command but not from guest memory.
So command values are validated here instead of doing at time time
of LPI injection.
If not done here, then we still allow malicious guest to run and during
LPI injection if invalid col_id is found in ITT we just drop LPI.
>
> 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.
Why do we need to restrict number of LPIs to domain?
[...]
>> +
>> +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?
IIRC, INT command contains vlpi which does not have its_device because irq_desc
is not reserved for this. Hence it is handled similar to event_ch int.
>
> Also, you have to translate the col_id into to a VCPU ID.
This is virtual collection id which itself is vcpu id. isn't it?
Regards
Vijay
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On Wed, 2015-07-15 at 15:02 +0200, Julien Grall wrote: > Hi Ian, > > On 10/07/2015 16:35, Ian Campbell wrote: > >> 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_COUNT4 > >> +#define BIT_48_12_MASK 0xf000UL > > > > I think you should use ~PAGE_MASK instead of defining this. > > This used to get the physical address in the GITS_CBASER register. This > field is not based on the Xen page granularity neither 4KB granularity. > It's just a field containing the bits [47:12] of the address. Then it should be GITS_CBASER_PA_MASK or something along those lines. Ian. > > Therefore it would be very strange to use PAGE_MASK and an hypothetical > 4K_MASK. Though I would prefer the latter. > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
Hi Ian, On 10/07/2015 16:35, Ian Campbell wrote: 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_COUNT4 +#define BIT_48_12_MASK 0xf000UL I think you should use ~PAGE_MASK instead of defining this. This used to get the physical address in the GITS_CBASER register. This field is not based on the Xen page granularity neither 4KB granularity. It's just a field containing the bits [47:12] of the address. Therefore it would be very strange to use PAGE_MASK and an hypothetical 4K_MASK. Though I would prefer the latter. -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
Hi Vijay,
On 10/07/2015 09:42, vijay.kil...@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
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On Mon, 2015-07-13 at 16:45 +0530, Vijay Kilari wrote: > BTW, I want to know if atomic_t supports 64-bit access?. I don't know off the top of my head. I'm sure it would be apparent in the code. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On Mon, Jul 13, 2015 at 2:52 PM, Ian Campbell wrote:
> On Sat, 2015-07-11 at 20:19 +0530, Vijay Kilari wrote:
>> >> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
>> >> +{
>> >> +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 );
>> >
>> > I can't find anywhere other than here where vits->cmd_write is touched.
>> > What am I missing?
>>
>>It is written by guest by GITS_CWRITER emulation in patch #9
>
> Ah, then please reverse the order so that the variable comes first and
> the target comes second.
>
> Also I think you need to find a better name that "cmd_write_save".
> Something which indicates the progress made perhaps? But why isn't this
> just cmd_read? Why the separate progress pointer?
I will check If I can use cmd_read.
BTW, I want to know if atomic_t supports 64-bit access?.
I have not made cmd_read as atomic_t.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On Sat, 2015-07-11 at 20:19 +0530, Vijay Kilari wrote:
> >> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
> >> +{
> >> +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 );
> >
> > I can't find anywhere other than here where vits->cmd_write is touched.
> > What am I missing?
>
>It is written by guest by GITS_CWRITER emulation in patch #9
Ah, then please reverse the order so that the variable comes first and
the target comes second.
Also I think you need to find a better name that "cmd_write_save".
Something which indicates the progress made perhaps? But why isn't this
just cmd_read? Why the separate progress pointer?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
>> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
>> +{
>> +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 );
>
> I can't find anywhere other than here where vits->cmd_write is touched.
> What am I missing?
It is written by guest by GITS_CWRITER emulation in patch #9
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
On Fri, 2015-07-10 at 13:12 +0530, vijay.kil...@gmail.com wrote:
[...]
> +static int vgic_its_process_int(struct vcpu *v, struct vgic_its *vits,
> +its_cmd_block *virt_cmd)
> +{
> [...]
> +
> +col_id = vitt_entry.vcollection;
> +if ( col_id < d->max_vcpus )
I think the condition here is backwards? And might be missing a + 1?
I think you've ended up open coding this max_vcpus+1 a lot. I think you
should encapsulate it into an valid_vcollection(d, col_id) helper and
use it throughout.
> +{
> +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);
> +
> +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,
Where you have uintXX_t types being printed please always use %"PRIxXX"
or %"PRIdXX" etc and not just %x or %lx etc. This is good practice even
in code which is only compiled for 64-bit.
Please also avoid uXX types in favour of uintXX_t (i.e. use the latter)
in code that hasn't come from elsewhere.
Both of these comments likely apply to all sort of bits of this series.
> +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;
> +
> +if ( virt_cmd->mapc.valid )
> +d->arch.vits->collections[vcol_id].target_address = vta;
> +else
> +d->arch.vits->collections[vcol_id].target_address = ~0UL;
You should use INVALID_PADDR here.
> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits)
> +{
> +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 );
I can't find anywhere other than here where vits->cmd_write is touched.
What am I missing?
> +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? */
> +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_COUNT4
> +#define BIT_48_12_MASK 0xf000UL
I think you should use ~PAGE_MASK instead of defining this.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support
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);
+}
+
/* 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)
+{
+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;
+
+entry.valid = true;
+entry.vcollection = vcol_id;
+entry.vlpi = vid;
+
+if ( cmd == GITS_CMD_MAPI )
+vits_set_vitt_entry(d, dev_id, vid, &entry);
+else
+{
+event = virt_cmd->mapvi.event;
+vits_set_vitt_entry(d, dev_id, event, &entry);
+}
+
+return 0;
+}
+
+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;
+
+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)
+{
+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;
+
+if ( vits_get_vitt_entry(d, dev_id, event, &entry) )
+return -E
