| Message ID | 1492164934-988-14-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 14, 2017 at 3:45 PM, Eric Auger <eric.auger@redhat.com> 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@redhat.com> Tested-by: Prakash, Brahmajyosyula <Brahmajyosyula.Prakash@cavium.com>
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@redhat.com> > > --- > > 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? > if (!(baser & GITS_BASER_INDIRECT)) { > phys_addr_t addr; > > -- > 2.5.5 > Thanks, -Christoffer
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@redhat.com> >> >> --- >> >> 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 >
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; + if (!(baser & GITS_BASER_INDIRECT)) { phys_addr_t addr;
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@redhat.com> --- 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(-)