Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-06 Thread Julien Grall 

Hi Andre,

Another thing I missed.

On 04/06/2017 12:19 AM, Andre Przywara wrote:

+case VREG64(GITS_BASER0):   /* device table */
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+spin_lock(>its_lock);
+
+/*
+ * Changing base registers with the ITS enabled is UNPREDICTABLE,
+ * we choose to ignore it, but warn.
+ */
+if ( its->enabled )
+{
+spin_unlock(>its_lock);
+gdprintk(XENLOG_WARNING, "ITS: tried to change BASER with the ITS 
enabled.\n");
+
+return 1;
+}
+
+reg = its->baser_dev;
+vgic_reg64_update(, r, info);
+
+reg &= ~GITS_BASER_RO_MASK;
+reg |= (sizeof(uint64_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
+reg |= GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
+sanitize_its_base_reg();
+
+if ( reg & GITS_VALID_BIT )
+its->max_devices = its_baser_nr_entries(reg);


Should not you validate this against the maximum number of device 
supported by the ITS (i.e devid)?



+else
+its->max_devices = 0;
+
+its->baser_dev = reg;
+spin_unlock(>its_lock);
+return 1;
+case VREG64(GITS_BASER1):   /* collection table */
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+spin_lock(>its_lock);
+/*
+ * Changing base registers with the ITS enabled is UNPREDICTABLE,
+ * we choose to ignore it, but warn.
+ */
+if ( its->enabled )
+{
+spin_unlock(>its_lock);
+gdprintk(XENLOG_INFO, "ITS: tried to change BASER with the ITS 
enabled.\n");
+return 1;
+}
+
+reg = its->baser_coll;
+vgic_reg64_update(, r, info);
+reg &= ~GITS_BASER_RO_MASK;
+reg |= (sizeof(uint16_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
+reg |= GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
+sanitize_its_base_reg();
+
+if ( reg & GITS_VALID_BIT )
+its->max_collections = its_baser_nr_entries(reg);


Similar question for the collection. Although, I am not sure against what.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-06 Thread Julien Grall 

Hi Andre,

On 04/06/2017 11:22 PM, André Przywara wrote:

On 06/04/17 22:43, Julien Grall wrote:

On 04/06/2017 12:19 AM, Andre Przywara wrote:

+static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv)
+{
+struct virt_its *its = priv;
+uint64_t reg;
+
+switch ( info->gpa & 0x )
+{
+case VREG32(GITS_CTLR):
+if ( info->dabt.size != DABT_WORD ) goto bad_width;
+
+spin_lock(>vcmd_lock);
+spin_lock(>its_lock);
+if ( its->enabled )
+reg = GITS_CTLR_ENABLE;
+else
+reg = 0;
+
+if ( its->cwriter == its->creadr )


I think it would be better if you can read atomically cwriter and
creadr.


What do you mean by "atomically" here? For reading and comparing *both*
registers I have to hold a lock, isn't it?


This would avoid to take the vcmd_lock here, as this would
prevent a guest vCPU to access this register while you are processing
the command queue.


That's a noble goal, but I don't know if this is easily feasible.
I wonder if one can use spin_trylock(>vcmd_lock) here, and assume
not quiescent if that fails? My understanding is that an OS would poll
CTLR until it becomes quiescent (Linux does so), so it would try again,
returning here until the lock becomes actually free.
Would that work?
But looking at the Linux code again I see that this is only done once at
probe time (before the ITS is enabled), so at least from that side it's
not really worth optimizing.


As you may know, I don't buy the reason: "Linux is only doing it once".

In this case my worry is not about optimizing because a guest could call 
it often but the fact the you might end up in all the vCPUs but one 
reading GITS_CTLR and one handling the command queue. So formers will 
wait on the lock which will monopolize the pCPUs.


The command queue handling is not ideal (it can take a bit of time), but 
this is going to be worst. And I really don't want to see that.



+*r = vgic_reg64_extract(its->cwriter, info);
+spin_unlock(>vcmd_lock);
+break;
+case VREG64(GITS_CREADR):
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+spin_lock(>vcmd_lock);


Ditto.


Currently this does not work because it could read it when creadr
reaches the end of the buffer (before the code above resets it to 0).
But by rewriting that code using a local variable this should be doable.


Please do it.


+
+static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
+  register_t r, void *priv)
+{
+struct domain *d = v->domain;
+struct virt_its *its = priv;
+uint64_t reg;
+uint32_t reg32, ctlr;
+
+switch ( info->gpa & 0x )
+{
+case VREG32(GITS_CTLR):
+if ( info->dabt.size != DABT_WORD ) goto bad_width;
+
+spin_lock(>vcmd_lock);


I am not sure to understand why taking the vcmd_lock here.


To prevent a nasty guest from turning off the ITS while commands are
still handled.


Please document it.




+spin_lock(>its_lock);
+ctlr = its->enabled ? GITS_CTLR_ENABLE : 0;
+reg32 = ctlr;
+vgic_reg32_update(, r, info);
+
+if ( ctlr ^ reg32 )
+its->enabled = vgic_v3_verify_its_status(its,
+ reg32 &
GITS_CTLR_ENABLE);


Should not you print a warning to help a user debugging if you don't
enable ITS as expected?


Good idea.


Also, how do you disable it?


Not sure what you mean? vgic_v3_verify_its_status() returns false if the
ENABLE_BIT has been cleared, so its->enabled becomes false.
Or what do I miss?


I think I wrote the question and answered myself before sending the 
e-mail. Though I forgot to drop it.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-06 Thread Julien Grall 

Hi Andre,

On 04/06/2017 12:19 AM, Andre Przywara wrote:

+case VREG64(GITS_BASER0):   /* device table */
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+spin_lock(>its_lock);
+
+/*
+ * Changing base registers with the ITS enabled is UNPREDICTABLE,
+ * we choose to ignore it, but warn.
+ */
+if ( its->enabled )
+{
+spin_unlock(>its_lock);
+gdprintk(XENLOG_WARNING, "ITS: tried to change BASER with the ITS 
enabled.\n");
+
+return 1;
+}
+
+reg = its->baser_dev;
+vgic_reg64_update(, r, info);
+
+reg &= ~GITS_BASER_RO_MASK;


It took me a bit of time to understand how you ensure the guest will not 
hand over two-level table. But this is hidden in the GITS_BASER_RO_MASK 
as you always mask the indirect bit.


I would prefer if you make clear that two-level table are not currently 
supported with a comment.




+reg |= (sizeof(uint64_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
+reg |= GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
+sanitize_its_base_reg();
+
+if ( reg & GITS_VALID_BIT )
+its->max_devices = its_baser_nr_entries(reg);
+else
+its->max_devices = 0;
+
+its->baser_dev = reg;
+spin_unlock(>its_lock);
+return 1;
+case VREG64(GITS_BASER1):   /* collection table */
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+spin_lock(>its_lock);
+/*
+ * Changing base registers with the ITS enabled is UNPREDICTABLE,
+ * we choose to ignore it, but warn.
+ */
+if ( its->enabled )
+{
+spin_unlock(>its_lock);
+gdprintk(XENLOG_INFO, "ITS: tried to change BASER with the ITS 
enabled.\n");
+return 1;
+}
+
+reg = its->baser_coll;
+vgic_reg64_update(, r, info);
+reg &= ~GITS_BASER_RO_MASK;


Ditto.


+reg |= (sizeof(uint16_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
+reg |= GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
+sanitize_its_base_reg();
+
+if ( reg & GITS_VALID_BIT )
+its->max_collections = its_baser_nr_entries(reg);
+else
+its->max_collections = 0;
+its->baser_coll = reg;
+spin_unlock(>its_lock);
+return 1;
+case VRANGE64(GITS_BASER2, GITS_BASER7):
+goto write_ignore_64;
+default:
+gdprintk(XENLOG_G_WARNING, "ITS: unhandled ITS register 0x%lx\n",
+ info->gpa & 0x);
+return 0;
+}



Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-06 Thread André Przywara 
On 06/04/17 22:43, Julien Grall wrote:

Salut Julien,

> On 04/06/2017 12:19 AM, Andre Przywara wrote:
>> Emulate the memory mapped ITS registers and provide a stub to introduce
>> the ITS command handling framework (but without actually emulating any
>> commands at this time).
>>
>> Signed-off-by: Andre Przywara 
>> ---
>>  xen/arch/arm/vgic-v3-its.c| 416
>> ++
>>  xen/arch/arm/vgic-v3.c|   9 -
>>  xen/include/asm-arm/gic_v3_defs.h |   9 +
>>  xen/include/asm-arm/gic_v3_its.h  |   3 +
>>  4 files changed, 428 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 9dfda59..f6bf1ee 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -78,6 +78,422 @@ void vgic_v3_its_free_domain(struct domain *d)
>>  ASSERT(RB_EMPTY_ROOT(>arch.vgic.its_devices));
>>  }
>>
>> +/**
>> + * Functions that handle ITS commands *
>> + **/
>> +
>> +static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
>> +   unsigned int shift, unsigned int
>> size)
>> +{
>> +return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
>> +}
>> +
>> +#define its_cmd_get_command(cmd)its_cmd_mask_field(cmd, 0, 
>> 0,  8)
>> +#define its_cmd_get_deviceid(cmd)   its_cmd_mask_field(cmd, 0,
>> 32, 32)
>> +#define its_cmd_get_size(cmd)   its_cmd_mask_field(cmd, 1, 
>> 0,  5)
>> +#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1, 
>> 0, 32)
>> +#define its_cmd_get_physical_id(cmd)its_cmd_mask_field(cmd, 1,
>> 32, 32)
>> +#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2, 
>> 0, 16)
>> +#define its_cmd_get_target_addr(cmd)its_cmd_mask_field(cmd, 2,
>> 16, 32)
>> +#define its_cmd_get_validbit(cmd)   its_cmd_mask_field(cmd, 2,
>> 63,  1)
>> +#define its_cmd_get_ittaddr(cmd)(its_cmd_mask_field(cmd, 2,
>> 8, 44) << 8)
>> +
>> +#define ITS_CMD_BUFFER_SIZE(baser)  baser) & 0xff) + 1) << 12)
>> +
>> +/*
>> + * Requires the vcmd_lock to be held.
>> + * TODO: Investigate whether we can be smarter here and don't need to
>> hold
>> + * the lock all of the time.
>> + */
>> +static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
>> +{
>> +paddr_t addr = its->cbaser & GENMASK_ULL(51, 12);
>> +uint64_t command[4];
>> +
>> +ASSERT(spin_is_locked(>vcmd_lock));
>> +
>> +if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +return -1;
>> +
>> +while ( its->creadr != its->cwriter )
>> +{
>> +int ret;
>> +
>> +ret = vgic_access_guest_memory(d, addr + its->creadr,
>> +   command, sizeof(command), false);
>> +if ( ret )
>> +return ret;
>> +
>> +switch ( its_cmd_get_command(command) )
>> +{
>> +case GITS_CMD_SYNC:
>> +/* We handle ITS commands synchronously, so we ignore
>> SYNC. */
>> +break;
>> +default:
>> +gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
>> + its_cmd_get_command(command));
>> +break;
>> +}
>> +
>> +its->creadr += ITS_CMD_SIZE;
>> +if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +its->creadr = 0;
>> +
>> +if ( ret )
>> +gdprintk(XENLOG_WARNING,
>> + "ITS: ITS command error %d while handling
>> command %lu\n",
>> + ret, its_cmd_get_command(command));
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/*
>> + * ITS registers read access *
>> + */
>> +
>> +static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
>> + register_t *r, void *priv)
>> +{
>> +struct virt_its *its = priv;
>> +uint64_t reg;
>> +
>> +switch ( info->gpa & 0x )
>> +{
>> +case VREG32(GITS_CTLR):
>> +if ( info->dabt.size != DABT_WORD ) goto bad_width;
>> +
>> +spin_lock(>vcmd_lock);
>> +spin_lock(>its_lock);
>> +if ( its->enabled )
>> +reg = GITS_CTLR_ENABLE;
>> +else
>> +reg = 0;
>> +
>> +if ( its->cwriter == its->creadr )
> 
> I think it would be better if you can read atomically cwriter and
> creadr.

What do you mean by "atomically" here? For reading and comparing *both*
registers I have to hold a lock, isn't it?

> This would avoid to take the vcmd_lock here, as this would
> prevent a guest vCPU to access this register while you are processing
> the command queue.

That's a noble goal, but I don't know if this is easily feasible.
I wonder if one can use spin_trylock(>vcmd_lock) here, and assume
not quiescent if that fails? My understanding is that an OS would poll
CTLR until it

Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-06 Thread Julien Grall 

Hi Andre,

On 04/06/2017 12:19 AM, Andre Przywara wrote:

Emulate the memory mapped ITS registers and provide a stub to introduce
the ITS command handling framework (but without actually emulating any
commands at this time).

Signed-off-by: Andre Przywara 
---
 xen/arch/arm/vgic-v3-its.c| 416 ++
 xen/arch/arm/vgic-v3.c|   9 -
 xen/include/asm-arm/gic_v3_defs.h |   9 +
 xen/include/asm-arm/gic_v3_its.h  |   3 +
 4 files changed, 428 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 9dfda59..f6bf1ee 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -78,6 +78,422 @@ void vgic_v3_its_free_domain(struct domain *d)
 ASSERT(RB_EMPTY_ROOT(>arch.vgic.its_devices));
 }

+/**
+ * Functions that handle ITS commands *
+ **/
+
+static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
+   unsigned int shift, unsigned int size)
+{
+return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
+}
+
+#define its_cmd_get_command(cmd)its_cmd_mask_field(cmd, 0,  0,  8)
+#define its_cmd_get_deviceid(cmd)   its_cmd_mask_field(cmd, 0, 32, 32)
+#define its_cmd_get_size(cmd)   its_cmd_mask_field(cmd, 1,  0,  5)
+#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1,  0, 32)
+#define its_cmd_get_physical_id(cmd)its_cmd_mask_field(cmd, 1, 32, 32)
+#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2,  0, 16)
+#define its_cmd_get_target_addr(cmd)its_cmd_mask_field(cmd, 2, 16, 32)
+#define its_cmd_get_validbit(cmd)   its_cmd_mask_field(cmd, 2, 63,  1)
+#define its_cmd_get_ittaddr(cmd)(its_cmd_mask_field(cmd, 2, 8, 44) << 
8)
+
+#define ITS_CMD_BUFFER_SIZE(baser)  baser) & 0xff) + 1) << 12)
+
+/*
+ * Requires the vcmd_lock to be held.
+ * TODO: Investigate whether we can be smarter here and don't need to hold
+ * the lock all of the time.
+ */
+static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
+{
+paddr_t addr = its->cbaser & GENMASK_ULL(51, 12);
+uint64_t command[4];
+
+ASSERT(spin_is_locked(>vcmd_lock));
+
+if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
+return -1;
+
+while ( its->creadr != its->cwriter )
+{
+int ret;
+
+ret = vgic_access_guest_memory(d, addr + its->creadr,
+   command, sizeof(command), false);
+if ( ret )
+return ret;
+
+switch ( its_cmd_get_command(command) )
+{
+case GITS_CMD_SYNC:
+/* We handle ITS commands synchronously, so we ignore SYNC. */
+break;
+default:
+gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
+ its_cmd_get_command(command));
+break;
+}
+
+its->creadr += ITS_CMD_SIZE;
+if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
+its->creadr = 0;
+
+if ( ret )
+gdprintk(XENLOG_WARNING,
+ "ITS: ITS command error %d while handling command %lu\n",
+ ret, its_cmd_get_command(command));
+}
+
+return 0;
+}
+
+/*
+ * ITS registers read access *
+ */
+
+static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv)
+{
+struct virt_its *its = priv;
+uint64_t reg;
+
+switch ( info->gpa & 0x )
+{
+case VREG32(GITS_CTLR):
+if ( info->dabt.size != DABT_WORD ) goto bad_width;
+
+spin_lock(>vcmd_lock);
+spin_lock(>its_lock);
+if ( its->enabled )
+reg = GITS_CTLR_ENABLE;
+else
+reg = 0;
+
+if ( its->cwriter == its->creadr )


I think it would be better if you can read atomically cwriter and 
creadr. This would avoid to take the vcmd_lock here, as this would 
prevent a guest vCPU to access this register while you are processing 
the command queue.



+reg |= GITS_CTLR_QUIESCENT;
+spin_unlock(>its_lock);
+spin_unlock(>vcmd_lock);
+
+*r = vgic_reg32_extract(reg, info);
+break;
+case VREG32(GITS_IIDR):
+if ( info->dabt.size != DABT_WORD ) goto bad_width;
+*r = vgic_reg32_extract(GITS_IIDR_VALUE, info);
+break;
+case VREG64(GITS_TYPER):
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+reg = GITS_TYPER_PHYSICAL;
+reg |= (sizeof(struct vits_itte) - 1) << GITS_TYPER_ITT_SIZE_SHIFT;
+reg |= (its->intid_bits - 1) << GITS_TYPER_IDBITS_SHIFT;
+reg |= (its->devid_bits - 1) << GITS_TYPER_DEVIDS_SHIFT;
+*r = vgic_reg64_extract(reg, info);
+break;
+case

Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-06 Thread Andre Przywara 
Hi Stefano,

On 06/04/17 01:21, Stefano Stabellini wrote:
> On Thu, 6 Apr 2017, Andre Przywara wrote:
>> Emulate the memory mapped ITS registers and provide a stub to introduce
>> the ITS command handling framework (but without actually emulating any
>> commands at this time).
>>
>> Signed-off-by: Andre Przywara 
>> ---
>>  xen/arch/arm/vgic-v3-its.c| 416 
>> ++
>>  xen/arch/arm/vgic-v3.c|   9 -
>>  xen/include/asm-arm/gic_v3_defs.h |   9 +
>>  xen/include/asm-arm/gic_v3_its.h  |   3 +
>>  4 files changed, 428 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 9dfda59..f6bf1ee 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -78,6 +78,422 @@ void vgic_v3_its_free_domain(struct domain *d)
>>  ASSERT(RB_EMPTY_ROOT(>arch.vgic.its_devices));
>>  }
>>  
>> +/**
>> + * Functions that handle ITS commands *
>> + **/
>> +
>> +static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
>> +   unsigned int shift, unsigned int size)
>> +{
>> +return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
>> +}
>> +
>> +#define its_cmd_get_command(cmd)its_cmd_mask_field(cmd, 0,  0,  8)
>> +#define its_cmd_get_deviceid(cmd)   its_cmd_mask_field(cmd, 0, 32, 32)
>> +#define its_cmd_get_size(cmd)   its_cmd_mask_field(cmd, 1,  0,  5)
>> +#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1,  0, 32)
>> +#define its_cmd_get_physical_id(cmd)its_cmd_mask_field(cmd, 1, 32, 32)
>> +#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2,  0, 16)
>> +#define its_cmd_get_target_addr(cmd)its_cmd_mask_field(cmd, 2, 16, 32)
>> +#define its_cmd_get_validbit(cmd)   its_cmd_mask_field(cmd, 2, 63,  1)
>> +#define its_cmd_get_ittaddr(cmd)(its_cmd_mask_field(cmd, 2, 8, 44) 
>> << 8)
>> +
>> +#define ITS_CMD_BUFFER_SIZE(baser)  baser) & 0xff) + 1) << 12)
>> +
>> +/*
>> + * Requires the vcmd_lock to be held.
>> + * TODO: Investigate whether we can be smarter here and don't need to hold
>> + * the lock all of the time.
>> + */
>> +static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
>> +{
>> +paddr_t addr = its->cbaser & GENMASK_ULL(51, 12);
>> +uint64_t command[4];
>> +
>> +ASSERT(spin_is_locked(>vcmd_lock));
>> +
>> +if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +return -1;
>> +
>> +while ( its->creadr != its->cwriter )
>> +{
>> +int ret;
>> +
>> +ret = vgic_access_guest_memory(d, addr + its->creadr,
>> +   command, sizeof(command), false);
>> +if ( ret )
>> +return ret;
>> +
>> +switch ( its_cmd_get_command(command) )
>> +{
>> +case GITS_CMD_SYNC:
>> +/* We handle ITS commands synchronously, so we ignore SYNC. */
>> +break;
>> +default:
>> +gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
>> + its_cmd_get_command(command));
>> +break;
>> +}
>> +
>> +its->creadr += ITS_CMD_SIZE;
>> +if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +its->creadr = 0;
>> +
>> +if ( ret )
>> +gdprintk(XENLOG_WARNING,
>> + "ITS: ITS command error %d while handling command 
>> %lu\n",
>> + ret, its_cmd_get_command(command));
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/*
>> + * ITS registers read access *
>> + */
>> +
>> +static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
>> + register_t *r, void *priv)
>> +{
>> +struct virt_its *its = priv;
>> +uint64_t reg;
>> +
>> +switch ( info->gpa & 0x )
>> +{
>> +case VREG32(GITS_CTLR):
>> +if ( info->dabt.size != DABT_WORD ) goto bad_width;
>> +
>> +spin_lock(>vcmd_lock);
>> +spin_lock(>its_lock);
>> +if ( its->enabled )
>> +reg = GITS_CTLR_ENABLE;
>> +else
>> +reg = 0;
>> +
>> +if ( its->cwriter == its->creadr )
>> +reg |= GITS_CTLR_QUIESCENT;
>> +spin_unlock(>its_lock);
>> +spin_unlock(>vcmd_lock);
>> +
>> +*r = vgic_reg32_extract(reg, info);
>> +break;
>> +case VREG32(GITS_IIDR):
>> +if ( info->dabt.size != DABT_WORD ) goto bad_width;
>> +*r = vgic_reg32_extract(GITS_IIDR_VALUE, info);
>> +break;
>> +case VREG64(GITS_TYPER):
>> +if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
>> +
>> +reg = GITS_TYPER_PHYSICAL;
>> +reg |= (sizeof(struct vits_itte) - 1) << GITS_TYPER_ITT_SIZE_SHIFT;
>> +

Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-05 Thread Stefano Stabellini 
On Thu, 6 Apr 2017, Andre Przywara wrote:
> Emulate the memory mapped ITS registers and provide a stub to introduce
> the ITS command handling framework (but without actually emulating any
> commands at this time).
>
> Signed-off-by: Andre Przywara 
> ---
>  xen/arch/arm/vgic-v3-its.c| 416 
> ++
>  xen/arch/arm/vgic-v3.c|   9 -
>  xen/include/asm-arm/gic_v3_defs.h |   9 +
>  xen/include/asm-arm/gic_v3_its.h  |   3 +
>  4 files changed, 428 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 9dfda59..f6bf1ee 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -78,6 +78,422 @@ void vgic_v3_its_free_domain(struct domain *d)
>  ASSERT(RB_EMPTY_ROOT(>arch.vgic.its_devices));
>  }
>  
> +/**
> + * Functions that handle ITS commands *
> + **/
> +
> +static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
> +   unsigned int shift, unsigned int size)
> +{
> +return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
> +}
> +
> +#define its_cmd_get_command(cmd)its_cmd_mask_field(cmd, 0,  0,  8)
> +#define its_cmd_get_deviceid(cmd)   its_cmd_mask_field(cmd, 0, 32, 32)
> +#define its_cmd_get_size(cmd)   its_cmd_mask_field(cmd, 1,  0,  5)
> +#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1,  0, 32)
> +#define its_cmd_get_physical_id(cmd)its_cmd_mask_field(cmd, 1, 32, 32)
> +#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2,  0, 16)
> +#define its_cmd_get_target_addr(cmd)its_cmd_mask_field(cmd, 2, 16, 32)
> +#define its_cmd_get_validbit(cmd)   its_cmd_mask_field(cmd, 2, 63,  1)
> +#define its_cmd_get_ittaddr(cmd)(its_cmd_mask_field(cmd, 2, 8, 44) 
> << 8)
> +
> +#define ITS_CMD_BUFFER_SIZE(baser)  baser) & 0xff) + 1) << 12)
> +
> +/*
> + * Requires the vcmd_lock to be held.
> + * TODO: Investigate whether we can be smarter here and don't need to hold
> + * the lock all of the time.
> + */
> +static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
> +{
> +paddr_t addr = its->cbaser & GENMASK_ULL(51, 12);
> +uint64_t command[4];
> +
> +ASSERT(spin_is_locked(>vcmd_lock));
> +
> +if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
> +return -1;
> +
> +while ( its->creadr != its->cwriter )
> +{
> +int ret;
> +
> +ret = vgic_access_guest_memory(d, addr + its->creadr,
> +   command, sizeof(command), false);
> +if ( ret )
> +return ret;
> +
> +switch ( its_cmd_get_command(command) )
> +{
> +case GITS_CMD_SYNC:
> +/* We handle ITS commands synchronously, so we ignore SYNC. */
> +break;
> +default:
> +gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
> + its_cmd_get_command(command));
> +break;
> +}
> +
> +its->creadr += ITS_CMD_SIZE;
> +if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
> +its->creadr = 0;
> +
> +if ( ret )
> +gdprintk(XENLOG_WARNING,
> + "ITS: ITS command error %d while handling command 
> %lu\n",
> + ret, its_cmd_get_command(command));
> +}
> +
> +return 0;
> +}
> +
> +/*
> + * ITS registers read access *
> + */
> +
> +static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
> + register_t *r, void *priv)
> +{
> +struct virt_its *its = priv;
> +uint64_t reg;
> +
> +switch ( info->gpa & 0x )
> +{
> +case VREG32(GITS_CTLR):
> +if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +
> +spin_lock(>vcmd_lock);
> +spin_lock(>its_lock);
> +if ( its->enabled )
> +reg = GITS_CTLR_ENABLE;
> +else
> +reg = 0;
> +
> +if ( its->cwriter == its->creadr )
> +reg |= GITS_CTLR_QUIESCENT;
> +spin_unlock(>its_lock);
> +spin_unlock(>vcmd_lock);
> +
> +*r = vgic_reg32_extract(reg, info);
> +break;
> +case VREG32(GITS_IIDR):
> +if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +*r = vgic_reg32_extract(GITS_IIDR_VALUE, info);
> +break;
> +case VREG64(GITS_TYPER):
> +if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +reg = GITS_TYPER_PHYSICAL;
> +reg |= (sizeof(struct vits_itte) - 1) << GITS_TYPER_ITT_SIZE_SHIFT;
> +reg |= (its->intid_bits - 1) << GITS_TYPER_IDBITS_SHIFT;
> +reg |= (its->devid_bits - 1) << GITS_TYPER_DEVIDS_SHIFT;
> +*r = vgic_reg64_extract(reg, info);
> +break;

[Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation

2017-04-05 Thread Andre Przywara 
Emulate the memory mapped ITS registers and provide a stub to introduce
the ITS command handling framework (but without actually emulating any
commands at this time).

Signed-off-by: Andre Przywara 
---
 xen/arch/arm/vgic-v3-its.c| 416 ++
 xen/arch/arm/vgic-v3.c|   9 -
 xen/include/asm-arm/gic_v3_defs.h |   9 +
 xen/include/asm-arm/gic_v3_its.h  |   3 +
 4 files changed, 428 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 9dfda59..f6bf1ee 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -78,6 +78,422 @@ void vgic_v3_its_free_domain(struct domain *d)
 ASSERT(RB_EMPTY_ROOT(>arch.vgic.its_devices));
 }
 
+/**
+ * Functions that handle ITS commands *
+ **/
+
+static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
+   unsigned int shift, unsigned int size)
+{
+return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
+}
+
+#define its_cmd_get_command(cmd)its_cmd_mask_field(cmd, 0,  0,  8)
+#define its_cmd_get_deviceid(cmd)   its_cmd_mask_field(cmd, 0, 32, 32)
+#define its_cmd_get_size(cmd)   its_cmd_mask_field(cmd, 1,  0,  5)
+#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1,  0, 32)
+#define its_cmd_get_physical_id(cmd)its_cmd_mask_field(cmd, 1, 32, 32)
+#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2,  0, 16)
+#define its_cmd_get_target_addr(cmd)its_cmd_mask_field(cmd, 2, 16, 32)
+#define its_cmd_get_validbit(cmd)   its_cmd_mask_field(cmd, 2, 63,  1)
+#define its_cmd_get_ittaddr(cmd)(its_cmd_mask_field(cmd, 2, 8, 44) << 
8)
+
+#define ITS_CMD_BUFFER_SIZE(baser)  baser) & 0xff) + 1) << 12)
+
+/*
+ * Requires the vcmd_lock to be held.
+ * TODO: Investigate whether we can be smarter here and don't need to hold
+ * the lock all of the time.
+ */
+static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
+{
+paddr_t addr = its->cbaser & GENMASK_ULL(51, 12);
+uint64_t command[4];
+
+ASSERT(spin_is_locked(>vcmd_lock));
+
+if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
+return -1;
+
+while ( its->creadr != its->cwriter )
+{
+int ret;
+
+ret = vgic_access_guest_memory(d, addr + its->creadr,
+   command, sizeof(command), false);
+if ( ret )
+return ret;
+
+switch ( its_cmd_get_command(command) )
+{
+case GITS_CMD_SYNC:
+/* We handle ITS commands synchronously, so we ignore SYNC. */
+break;
+default:
+gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
+ its_cmd_get_command(command));
+break;
+}
+
+its->creadr += ITS_CMD_SIZE;
+if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
+its->creadr = 0;
+
+if ( ret )
+gdprintk(XENLOG_WARNING,
+ "ITS: ITS command error %d while handling command %lu\n",
+ ret, its_cmd_get_command(command));
+}
+
+return 0;
+}
+
+/*
+ * ITS registers read access *
+ */
+
+static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv)
+{
+struct virt_its *its = priv;
+uint64_t reg;
+
+switch ( info->gpa & 0x )
+{
+case VREG32(GITS_CTLR):
+if ( info->dabt.size != DABT_WORD ) goto bad_width;
+
+spin_lock(>vcmd_lock);
+spin_lock(>its_lock);
+if ( its->enabled )
+reg = GITS_CTLR_ENABLE;
+else
+reg = 0;
+
+if ( its->cwriter == its->creadr )
+reg |= GITS_CTLR_QUIESCENT;
+spin_unlock(>its_lock);
+spin_unlock(>vcmd_lock);
+
+*r = vgic_reg32_extract(reg, info);
+break;
+case VREG32(GITS_IIDR):
+if ( info->dabt.size != DABT_WORD ) goto bad_width;
+*r = vgic_reg32_extract(GITS_IIDR_VALUE, info);
+break;
+case VREG64(GITS_TYPER):
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+reg = GITS_TYPER_PHYSICAL;
+reg |= (sizeof(struct vits_itte) - 1) << GITS_TYPER_ITT_SIZE_SHIFT;
+reg |= (its->intid_bits - 1) << GITS_TYPER_IDBITS_SHIFT;
+reg |= (its->devid_bits - 1) << GITS_TYPER_DEVIDS_SHIFT;
+*r = vgic_reg64_extract(reg, info);
+break;
+case VREG64(GITS_CBASER):
+if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+spin_lock(>its_lock);
+*r = vgic_reg64_extract(its->cbaser, info);
+spin_unlock(>its_lock);
+break;
+case VREG64(GITS_CWRITER):
+if (