lkml.org 
[lkml]   [2017]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 03/10] KVM: arm/arm64: vgic-its: Improve error reporting on device table save
    On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote:
    > At the moment the device table save() returns -EINVAL if
    > vgic_its_check_id() fails to return the gpa of the entry
    > associated to the device/collection id. Let vgic_its_check_id()
    > return an int instead of a bool and return a more precised
    > error value:
    > - EINVAL in case the id is out of range
    > - EFAULT if the gpa is not provisionned or is not valid
    >

    This is just to ease debugging, yes?

    Seems a bit invasive to me for that purpose.

    > We also check first the GITS_BASER<n> Valid bit is set.
    >
    > This allows the userspace to discriminate failure reasons.
    >
    > Signed-off-by: Eric Auger <eric.auger@redhat.com>
    >
    > ---
    >
    > need to CC stable

    Is this solving a real issue that someone has? Otherwise I don't think
    it should be cc'ed to stable, nor am I sure it warrants merging as a
    fix.

    Thanks,
    -Christoffer

    > ---
    > virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++--------------
    > 1 file changed, 36 insertions(+), 17 deletions(-)
    >
    > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
    > index 76bed2d..c1f7972 100644
    > --- a/virt/kvm/arm/vgic/vgic-its.c
    > +++ b/virt/kvm/arm/vgic/vgic-its.c
    > @@ -688,14 +688,24 @@ 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.
    > + * vgic_its_check_id - 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 accessible).
    > * For this we have to read the respective first level entry.
    > + *
    > + * @its: its handle
    > + * @baser: GITS_BASER<n> register
    > + * @id: id of the device/collection
    > + * @eaddr: output gpa of the corresponding table entry
    > + *
    > + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if
    > + * the address cannot be computed or is not valid
    > */
    > -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
    > - gpa_t *eaddr)
    > +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
    > + gpa_t *eaddr)
    > {
    > int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
    > u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
    > @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
    > int index;
    > gfn_t gfn;
    >
    > + if (!(baser & GITS_BASER_VALID))
    > + return -EFAULT;
    > +
    > switch (type) {
    > case GITS_BASER_TYPE_DEVICE:
    > if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
    > - return false;
    > + return -EINVAL;
    > break;
    > case GITS_BASER_TYPE_COLLECTION:
    > /* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */
    > if (id >= BIT_ULL(16))
    > - return false;
    > + return -EINVAL;
    > break;
    > default:
    > - return false;
    > + return -EINVAL;
    > }
    >
    > if (!(baser & GITS_BASER_INDIRECT)) {
    > phys_addr_t addr;
    >
    > if (id >= (l1_tbl_size / esz))
    > - return false;
    > + return -EINVAL;
    >
    > addr = BASER_ADDRESS(baser) + id * esz;
    > gfn = addr >> PAGE_SHIFT;
    >
    > if (eaddr)
    > *eaddr = addr;
    > - return kvm_is_visible_gfn(its->dev->kvm, gfn);
    > + if (kvm_is_visible_gfn(its->dev->kvm, gfn))
    > + return 0;
    > + else
    > + return -EFAULT;
    > }
    >
    > /* calculate and check the index into the 1st level */
    > index = id / (SZ_64K / esz);
    > if (index >= (l1_tbl_size / sizeof(u64)))
    > - return false;
    > + return -EINVAL;
    >
    > /* Each 1st level entry is represented by a 64-bit value. */
    > if (kvm_read_guest(its->dev->kvm,
    > BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
    > &indirect_ptr, sizeof(indirect_ptr)))
    > - return false;
    > + return -EFAULT;
    >
    > indirect_ptr = le64_to_cpu(indirect_ptr);
    >
    > /* check the valid bit of the first level entry */
    > if (!(indirect_ptr & BIT_ULL(63)))
    > - return false;
    > + return -EFAULT;
    >
    > /*
    > * Mask the guest physical address and calculate the frame number.
    > @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
    >
    > if (eaddr)
    > *eaddr = indirect_ptr;
    > - return kvm_is_visible_gfn(its->dev->kvm, gfn);
    > + if (kvm_is_visible_gfn(its->dev->kvm, gfn))
    > + return 0;
    > + else
    > + return -EFAULT;
    > }
    >
    > static int vgic_its_alloc_collection(struct vgic_its *its,
    > @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
    > {
    > struct its_collection *collection;
    >
    > - if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
    > + if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
    > return E_ITS_MAPC_COLLECTION_OOR;
    >
    > collection = kzalloc(sizeof(*collection), GFP_KERNEL);
    > @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
    > gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
    > struct its_device *device;
    >
    > - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
    > + if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
    > return E_ITS_MAPD_DEVICE_OOR;
    >
    > if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
    > @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
    > int ret;
    > gpa_t eaddr;
    >
    > - if (!vgic_its_check_id(its, baser,
    > - dev->device_id, &eaddr))
    > - return -EINVAL;
    > + ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr);
    > + if (ret)
    > + return ret;
    >
    > ret = vgic_its_save_itt(its, dev);
    > if (ret)
    > --
    > 2.5.5
    >

    \
     
     \ /
      Last update: 2017-10-13 15:16    [W:3.684 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site