Re: [PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

From: Christoffer Dall
Date: Tue Oct 17 2017 - 17:40:44 EST

Next message: Wolfram Sang: "Re: [PATCH v6] eeprom: at24: Add OF device ID table"
Previous message: Andi Kleen: "[PATCH] Support resetting WARN*_ONCE"
In reply to: Eric Auger: "[PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore"
Next in thread: Auger Eric: "Re: [PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Oct 17, 2017 at 09:09:59AM +0200, Eric Auger wrote:
> AT the moment if ITT only contains invalid entries,
> vgic_its_restore_itt returns 1 and this is considered as
> an an error in vgic_its_restore_dte.
>
> Also in case the device table only contains invalid entries,
> the table restore fails and this is not correct.
>
> This patch fully revisits the errror handling while fixing those
> 2 bugs.
>
> - entry_fn_t now takes a valid output paraleter

parameter

> - scan_its_table() now returns <= 0 values and output 2 booleans,
outputs
> valid and last.
> - vgic_its_restore_itt() now returns <= 0 values.
> - vgic_its_restore_device_tables() also returns <= 0 values.
>
> With that patch we are able to properly handle the case where
> all data are invalid but we still are able to detect the case
> where a next entry was referenced by some valid entry and
> never found.
>
> Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
> Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> Reported-by: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
>
> ---
>
> need to CC stable
>
> v3 -> v4:
> - set *valid at beginning of handle_l1_dte
>
> v2 -> v3:
> - add comments
> - added valid parameter
> - vgic_its_restore_itt don't return +1 anymore
> - reword the commit message
>
> v1 -> v2:
> - if (ret > 0) ret = 0
> ---
> virt/kvm/arm/vgic/vgic-its.c | 95 ++++++++++++++++++++++++++++++++------------
> 1 file changed, 70 insertions(+), 25 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index f51c1e1..eea14a1 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1772,16 +1772,20 @@ static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>
> /**
> * entry_fn_t - Callback called on a table entry restore path
> + *
> * @its: its handle
> * @id: id of the entry
> * @entry: pointer to the entry
> * @opaque: pointer to an opaque data
> + * @valid: indicates whether valid data is associated to this entry
> + * (the entry itself in case of linear table or entries in the next level,
> + * in case of hierachical tables)
> *
> * Return: < 0 on error, 0 if last element was identified, id offset to next
> * element otherwise
> */
> typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
> - void *opaque);
> + void *opaque, bool *valid);
>
> /**
> * scan_its_table - Scan a contiguous table in guest RAM and applies a function
> @@ -1794,29 +1798,34 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
> * @start_id: the ID of the first entry in the table
> * (non zero for 2d level tables)
> * @fn: function to apply on each entry
> + * @opaque: opaque data passed to @fn
> + * @valid: indicates whether the table contains any valid data
> + * @last: returns whether the last valid entry was decoded
> *
> - * Return: < 0 on error, 0 if last element was identified, 1 otherwise
> - * (the last element may not be found on second level tables)
> + * Return: < 0 on error, 0 on success
> */
> static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
> - int start_id, entry_fn_t fn, void *opaque)
> + int start_id, entry_fn_t fn, void *opaque,
> + bool *valid, bool *last)
> {
> void *entry = kzalloc(esz, GFP_KERNEL);
> struct kvm *kvm = its->dev->kvm;
> unsigned long len = size;
> int id = start_id;
> gpa_t gpa = base;
> + int next_offset = 0;
> int ret;
>
> while (len > 0) {
> - int next_offset;
> size_t byte_offset;
> + bool entry_valid;
>
> ret = kvm_read_guest(kvm, gpa, entry, esz);
> if (ret)
> goto out;
>
> - next_offset = fn(its, id, entry, opaque);
> + next_offset = fn(its, id, entry, opaque, &entry_valid);
> + *valid |= entry_valid;
> if (next_offset <= 0) {
> ret = next_offset;
> goto out;
> @@ -1827,9 +1836,15 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
> gpa += byte_offset;
> len -= byte_offset;
> }
> - ret = 1;
> -
> + /*
> + * the table lookup was completed without identifying the
> + * last valid entry (ie. next_offset > 0).
> + */

but you never set last to false? If you require the caller to set the
variable to false, that should be documented, but it's weird.

> + ret = 0;
> out:
> + if (!next_offset)
> + *last = true;
> +

so if we scan the entire table to the end we won't set last? Isn't that
a bit strange?

Also, if we can get id of the valid out parameter and instead handle
that within this function, I don't think you'll need the 'last' return
value in vgic_its_restore_device_tables, and you could make this:

if (!next_offset && last)
*last = true;
else if (last)
*last = false;

> kfree(entry);
> return ret;
> }
> @@ -1854,12 +1869,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>
> /**
> * vgic_its_restore_ite - restore an interrupt translation entry
> + *
> * @event_id: id used for indexing
> * @ptr: pointer to the ITE entry
> * @opaque: pointer to the its_device
> + * @valid: indicates whether the ite is valid
> */
> static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
> - void *ptr, void *opaque)
> + void *ptr, void *opaque, bool *valid)
> {
> struct its_device *dev = (struct its_device *)opaque;
> struct its_collection *collection;
> @@ -1879,7 +1896,9 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
> coll_id = val & KVM_ITS_ITE_ICID_MASK;
> lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
>
> - if (!lpi_id)
> + *valid = !!lpi_id;
> +
> + if (!*valid)
> return 1; /* invalid entry, no choice but to scan next entry */
>
> if (lpi_id < VGIC_MIN_LPI)
> @@ -1940,6 +1959,14 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
> return 0;
> }
>
> +/**
> + * vgic_its_restore_itt - restore the ITT of a device
> + *
> + * @its: its handle
> + * @dev: device handle
> + *
> + * Return 0 on success, < 0 on error
> + */
> static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
> {
> const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> @@ -1947,9 +1974,15 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
> int ret;
> int ite_esz = abi->ite_esz;
> size_t max_size = BIT_ULL(dev->num_eventid_bits) * ite_esz;
> + bool valid = false, last = false;
>
> ret = scan_its_table(its, base, max_size, ite_esz, 0,
> - vgic_its_restore_ite, dev);
> + vgic_its_restore_ite, dev, &valid, &last);
> +
> + if (!ret && valid && !last) {
> + /* a next element was referenced but not found */
> + return -EINVAL;

So this is if we ever found a valid entry, but somehow it didn't lead us
to the last entry, right? Can't you handle that within the
scan_its_table?

As I understand it, scan_its_table is in one of two modes, either it's
linearly scanning, or it found a valid entry, and it's jumping from one
entry to the next, given the offsets. If it's in the second mode, and
finds an invalid entry, it should return an error.

I think you can also get rid of the '*valid = false; return 1;' thing,
which looks a bit strange.

> + }
>
> return ret;
> }
> @@ -1985,29 +2018,29 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
> * @id: device id the DTE corresponds to
> * @ptr: kernel VA where the 8 byte DTE is located
> * @opaque: unused
> + * @valid: indicates whether the dte is valid
> *
> * Return: < 0 on error, 0 if the dte is the last one, id offset to the
> * next dte otherwise
> */
> static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> - void *ptr, void *opaque)
> + void *ptr, void *opaque, bool *valid)
> {
> struct its_device *dev;
> gpa_t itt_addr;
> u8 num_eventid_bits;
> u64 entry = *(u64 *)ptr;
> - bool valid;
> u32 offset;
> int ret;
>
> entry = le64_to_cpu(entry);
>
> - valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
> + *valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
> num_eventid_bits = (entry & KVM_ITS_DTE_SIZE_MASK) + 1;
> itt_addr = ((entry & KVM_ITS_DTE_ITTADDR_MASK)
> >> KVM_ITS_DTE_ITTADDR_SHIFT) << 8;
>
> - if (!valid)
> + if (!*valid)
> return 1;
>
> /* dte entry is valid */
> @@ -2082,13 +2115,14 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
> * @id: index of the entry in the L1 table
> * @addr: kernel VA
> * @opaque: unused
> + * @valid: indicates whether any dte entry was found
> *
> * L1 table entries are scanned by steps of 1 entry
> * Return < 0 if error, 0 if last dte was found when scanning the L2
> * table, +1 otherwise (meaning next L1 entry must be scanned)
> */
> static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
> - void *opaque)
> + void *opaque, bool *valid)
> {
> const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> int l2_start_id = id * (SZ_64K / abi->dte_esz);
> @@ -2096,21 +2130,29 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
> int dte_esz = abi->dte_esz;
> gpa_t gpa;
> int ret;
> + bool last;
>
> entry = le64_to_cpu(entry);
>
> - if (!(entry & KVM_ITS_L1E_VALID_MASK))
> + *valid = entry & KVM_ITS_L1E_VALID_MASK;
> +
> + if (!*valid)
> return 1;
>
> gpa = entry & KVM_ITS_L1E_ADDR_MASK;
>
> ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
> - l2_start_id, vgic_its_restore_dte, NULL);
> + l2_start_id, vgic_its_restore_dte, NULL,
> + valid, &last);
>
> - if (ret <= 0)
> - return ret;
> + /*
> + * if the last dte has not been found in this L2 table, we
> + * need to scan the next L1 entry
> + */
> + if (!ret && !last)
> + return 1;
>
> - return 1;
> + return ret;
> }
>
> /**
> @@ -2124,6 +2166,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
> int l1_esz, ret;
> int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
> gpa_t l1_gpa;
> + bool valid = false, last = false;
>
> if (!(baser & GITS_BASER_VALID))
> return 0;
> @@ -2133,15 +2176,17 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
> if (baser & GITS_BASER_INDIRECT) {
> l1_esz = GITS_LVL1_ENTRY_SIZE;
> ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
> - handle_l1_dte, NULL);
> + handle_l1_dte, NULL, &valid, &last);
> } else {
> l1_esz = abi->dte_esz;
> ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
> - vgic_its_restore_dte, NULL);
> + vgic_its_restore_dte, NULL, &valid, &last);
> }
>
> - if (ret > 0)
> - ret = -EINVAL;
> + if (!ret && valid && !last) {
> + /* a next element was referenced but not found */
> + return -EINVAL;
> + }

Same comment as above.

>
> return ret;
> }
> --
> 2.5.5
>
Thanks,
-Christoffer

Next message: Wolfram Sang: "Re: [PATCH v6] eeprom: at24: Add OF device ID table"
Previous message: Andi Kleen: "[PATCH] Support resetting WARN*_ONCE"
In reply to: Eric Auger: "[PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore"
Next in thread: Auger Eric: "Re: [PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]