On Fri, Apr 28, 2017 at 01:05:15PM +0200, Auger Eric wrote:
> Hi Christoffer,
>
> On 28/04/2017 12:44, Christoffer Dall wrote:
> > On Fri, Apr 14, 2017 at 12:15:29PM +0200, Eric Auger wrote:
> >> The save path copies the collection entries into guest RAM
> >> at the GPA specified in the BASER register. This obviously
> >> requires the BASER to be set. The last written element is a
> >> dummy collection table entry.
> >>
> >> We do not index by collection ID as the collection entry
> >> can fit into 8 bytes while containing the collection ID.
> >>
> >> On restore path we re-allocate the collection objects.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> >>
> >> ---
> >> v4 -> v5:
> >> - add macros for field encoding/decoding
> >> - use abi->cte_esz
> >> - rename flush into save
> >> - check the target_addr is valid
> >>
> >> v3 -> v4:
> >> - replaced u64 *ptr by gpa_t gpa
> >> - check the collection does not exist before allocating it
> >>
> >> v1 -> v2:
> >> - reword commit message and directly use 8 as entry size
> >> - no kvm parameter anymore
> >> - add helper for flush/restore cte
> >> - table size computed here
> >> - add le64/cpu conversions
> >> ---
> >> virt/kvm/arm/vgic/vgic-its.c | 109 ++++++++++++++++++++++++++++++++++++++++++-
> >> virt/kvm/arm/vgic/vgic.h | 9 ++++
> >> 2 files changed, 116 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index c22b35d..484e541 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -1785,13 +1785,97 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
> >> return -ENXIO;
> >> }
> >>
> >> +static int vgic_its_save_cte(struct vgic_its *its,
> >> + struct its_collection *collection,
> >> + gpa_t gpa, int esz)
> >> +{
> >> + u64 val;
> >> + int ret;
> >> +
> >> + val = (1ULL << KVM_ITS_CTE_VALID_SHIFT |
> >> + ((u64)collection->target_addr << KVM_ITS_CTE_RDBASE_SHIFT) |
> >> + collection->collection_id);
> >> + val = cpu_to_le64(val);
> >> + ret = kvm_write_guest(its->dev->kvm, gpa, &val, esz);
> >> + return ret;
> >> +}
> >> +
> >> +static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa,
> >> + int esz, bool *valid)
> >> +{
> >> + struct its_collection *collection;
> >> + struct kvm *kvm = its->dev->kvm;
> >> + u32 target_addr;
> >> + u32 coll_id;
> >> + u64 val;
> >> + int ret;
> >> +
> >> + *valid = false;
> >
> > I don't see why you need this.
> I initialized it here in case kvm_read_guest() fails
If the caller looks at the valid setting when it receives an error code,
that's really weird.
> >
> >> +
> >> + ret = kvm_read_guest(kvm, gpa, &val, esz);
> >
> > hmm, we better not have an esz larger than sizeof(u64) here then.
> Yes this could be part if the ABI ops but is that worth the effort now?
> I can add a comment somewhere to mention that trap.
I wasn't really sure what we should do here, but I think we should add
BUG_ON(esz > sizeof(val)) when going over this again.
Thanks,
-Christoffer
