Hi,
On 27/04/2017 18:48, Christoffer Dall wrote:
> On Fri, Apr 14, 2017 at 12:15:25PM +0200, Eric Auger wrote:
>> On MAPD we currently check the device id can be stored in the device table.
>> Let's first check it can be encoded within the range defined by TYPER
>> DEVBITS.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>>
>> v4 -> v5:
>> - use GIC_ENCODE_SZ macro
>>
>> v3 -> v4:
>> - VITS_TYPER_DEVBITS set to 16 for homogeneity
>> - use BIT_ULL
>> ---
>> virt/kvm/arm/vgic/vgic-its.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 757598d..de1ed6d 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -194,6 +194,7 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
>> #define GIC_LPI_OFFSET 8192
>>
>> #define VITS_TYPER_IDBITS 16
>> +#define VITS_TYPER_DEVBITS 16
>>
>> /*
>> * Finds and returns a collection in the ITS collection table.
>> @@ -394,7 +395,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
>> * To avoid memory waste in the guest, we keep the number of IDBits and
>> * DevBits low - as least for the time being.
>> */
>> - reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>> + reg |= GIC_ENCODE_SZ(VITS_TYPER_DEVBITS, 5) << GITS_TYPER_DEVBITS_SHIFT;
>> reg |= GIC_ENCODE_SZ(VITS_TYPER_IDBITS, 5) << GITS_TYPER_IDBITS_SHIFT;
>> reg |= GIC_ENCODE_SZ(abi->ite_esz, 4) << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
>>
>> @@ -639,10 +640,10 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>> * Check whether an ID can be stored into the corresponding guest table.
>> * For a direct table this is pretty easy, but gets a bit nasty for
>> * indirect tables. We check whether the resulting guest physical address
>> - * is actually valid (covered by a memslot and guest accessbible).
>> + * is actually valid (covered by a memslot and guest accessible).
>> * For this we have to read the respective first level entry.
>> */
>> -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
>> +static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id)
>> {
>> int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>> int index;
>> @@ -650,6 +651,9 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
>> gfn_t gfn;
>> int esz = GITS_BASER_ENTRY_SIZE(baser);
>>
>> + if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
>> + return false;
>> +
>
> Isn't vgic_its_check_id called with both a device id and a collection
> id? How can this then be a valid check?
Hum yes that's correct. In practice the test is correct for collection
ID too since our virtual implementation supports collections in memory
(GITS_TYPER.CIL ==0, spec 8.19.8) and a 16-bit collection ID is
supported. But this is by chance and this really deserves some proper
differentiation. Thank you for spotting that one too!
Thanks
Eric
>
>> if (!(baser & GITS_BASER_INDIRECT)) {
>> phys_addr_t addr;
>>
>> --
>> 2.5.5
>>
>
> Thanks,
> -Christoffer
>
