[PATCH v3 08/19] KVM: arm64: ITS: Interpret MAPD Size field and check related errors

Tue Mar 21 10:57:14 PDT 2017

Hi Eric,

On 21/03/17 17:40, Auger Eric wrote:
> Hi Andre,
> 
> On 17/03/2017 16:03, Andre Przywara wrote:
>> Hi,
>>
>> On 06/03/17 11:34, Eric Auger wrote:
>>> Up to now the MAPD's ITT size field has been ignored. It encodes
>>> the number of eventid bit minus 1. It should be used to check
>>> the eventid when a MAPTI command is issued on a device. Let's
>>> store the nb_eventid_bits in the its_device and do the check
>>> on MAPTI. Also make sure the ITT size field does not exceed the
>>> GITS_TYPER IDBITS field.
>>>
>>> Signed-off-by: Eric Auger <eric.auger at redhat.com>
>>>
>>> ---
>>> ---
>>>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>>>  virt/kvm/arm/vgic/vgic-its.c       | 14 +++++++++++++-
>>>  2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>>> index d81f10f..a7a5548 100644
>>> --- a/include/linux/irqchip/arm-gic-v3.h
>>> +++ b/include/linux/irqchip/arm-gic-v3.h
>>> @@ -338,9 +338,11 @@
>>>  #define E_ITS_INT_UNMAPPED_INTERRUPT		0x010307
>>>  #define E_ITS_CLEAR_UNMAPPED_INTERRUPT		0x010507
>>>  #define E_ITS_MAPD_DEVICE_OOR			0x010801
>>> +#define E_ITS_MAPD_ITTSIZE_OOR			0x010802
>>>  #define E_ITS_MAPC_PROCNUM_OOR			0x010902
>>>  #define E_ITS_MAPC_COLLECTION_OOR		0x010903
>>>  #define E_ITS_MAPTI_UNMAPPED_DEVICE		0x010a04
>>> +#define E_ITS_MAPTI_ID_OOR			0x010a05
>>>  #define E_ITS_MAPTI_PHYSICALID_OOR		0x010a06
>>>  #define E_ITS_INV_UNMAPPED_INTERRUPT		0x010c07
>>>  #define E_ITS_INVALL_UNMAPPED_COLLECTION	0x010d09
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>> index 6d84508..56bcd92 100644
>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -99,6 +99,7 @@ struct its_device {
>>>  
>>>  	/* the head for the list of ITTEs */
>>>  	struct list_head itt_head;
>>> +	u32 nb_eventid_bits;
>>>  	u32 device_id;
>>>  };
>>>  
>>> @@ -177,6 +178,7 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
>>>  #define GIC_LPI_OFFSET 8192
>>>  
>>>  #define VITS_ESZ 8
>>> +#define VITS_TYPER_IDBITS 0xF
>>
>> Alone for consistency reasons I'd use "16" here and subtract one upon
>> encoding this into the TYPER register.
> OK done
>>
>>>  
>>>  /*
>>>   * Finds and returns a collection in the ITS collection table.
>>> @@ -400,7 +402,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
>>>  	 * DevBits low - as least for the time being.
>>>  	 */
>>>  	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>>> -	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
>>> +	reg |= VITS_TYPER_IDBITS << GITS_TYPER_IDBITS_SHIFT;
>>>  	reg |= (VITS_ESZ - 1) << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
>>>  
>>>  	return extract_bytes(reg, addr & 7, len);
>>> @@ -560,6 +562,7 @@ static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
>>>  #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_size(cmd)		its_cmd_mask_field(cmd, 1, 0,  5)
>>
>> Can you please move this up to be in order with the other fields?
> done
>>
>>>  /*
>>>   * The DISCARD command frees an Interrupt Translation Table Entry (ITTE).
>>> @@ -745,6 +748,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>>>  	if (!device)
>>>  		return E_ITS_MAPTI_UNMAPPED_DEVICE;
>>>  
>>> +	if (event_id >= (2 << device->nb_eventid_bits))
>>
>> And I'd find it less confusing to hold the actual bit size in
>> nb_eventid_bits (and abstract from the particular encoding in TYPER).
>> That would avoid this slightly odd (2 << ...) here, for instance.
> OK I replaced nb_eventid_bits by nb_eventids which corresponds to the
> the max number of eventids for this device
> 
> More than odd, the shift is completely wrong. Now I use BIT_ULL() as you
> mentioned later on.
> 
> The only drawback is that when flushing the DTE I need to use ilog2 to
> encode the size field again.

Oh sorry, I am fine (and actually prefer) to encode the number of bits,
but the *actual* number of bits, not something like -1. This seems to be
a bit inconsistent throughout the series, I was suspecting that those "2
<< x" expressions were to take care of the -1/+1 encoding.
The problem with using the actual number (apart from the ilog2 issue) is
that you can't really cover exactly 32 bits worth of devices.

So sorry for the confusion, and hope you can easily revert your change ;-)

Cheers,
Andre.

>>
>>> +		return E_ITS_MAPTI_ID_OOR;
>>> +
>>>  	if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI)
>>>  		lpi_nr = its_cmd_get_physical_id(its_cmd);
>>>  	else
>>> @@ -825,11 +831,15 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>>  {
>>>  	u32 device_id = its_cmd_get_deviceid(its_cmd);
>>>  	bool valid = its_cmd_get_validbit(its_cmd);
>>> +	size_t size = its_cmd_get_size(its_cmd);
>>
>> If you add 1 here, you'd make clear that this is an encoding property.
> done
> 
> Thanks
> 
> Eric
> 
>>
>> Cheers,
>> Andre.
>>
>>>  	struct its_device *device;
>>>  
>>>  	if (!vgic_its_check_id(its, its->baser_device_table, device_id))
>>>  		return E_ITS_MAPD_DEVICE_OOR;
>>>  
>>> +	if (valid && size > VITS_TYPER_IDBITS)
>>> +		return E_ITS_MAPD_ITTSIZE_OOR;
>>> +
>>>  	device = find_its_device(its, device_id);
>>>  
>>>  	/*
>>> @@ -852,6 +862,8 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>>  		return -ENOMEM;
>>>  
>>>  	device->device_id = device_id;
>>> +	device->nb_eventid_bits = size + 1;
>>> +
>>>  	INIT_LIST_HEAD(&device->itt_head);
>>>  
>>>  	list_add_tail(&device->dev_list, &its->device_list);
>>>