On Fri, May 05, 2017 at 04:50:01PM +0200, Auger Eric wrote:
> Hi,
>
> On 05/05/2017 14:50, Marc Zyngier wrote:
> > On 05/05/17 10:57, Christoffer Dall wrote:
> >> On Thu, May 04, 2017 at 01:44:35PM +0200, Eric Auger wrote:
> >>> When creating the lpi we now ask the redistributor what is the state
> >>> of the LPI (priority, enabled, pending).
> >>>
> >>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> >>>
> >>> ---
> >>>
> >>> v6: creation
> >>> ---
> >>> virt/kvm/arm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++-----------
> >>> 1 file changed, 24 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >>> index f43ea30c..3ad615a 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-its.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >>> @@ -38,6 +38,8 @@
> >>>
> >>> static int vgic_its_set_abi(struct vgic_its *its, int rev);
> >>> static const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its);
> >>> +static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
> >>> + struct kvm_vcpu *filter_vcpu);
> >>>
> >>> /*
> >>> * Creates a new (reference to a) struct vgic_irq for a given LPI.
> >>> @@ -46,10 +48,12 @@ static const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its);
> >>> * If this is a "new" LPI, we allocate and initialize a new struct vgic_irq.
> >>> * This function returns a pointer to the _unlocked_ structure.
> >>> */
> >>> -static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
> >>> +static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
> >>> + struct kvm_vcpu *vcpu)
> >>> {
> >>> struct vgic_dist *dist = &kvm->arch.vgic;
> >>> struct vgic_irq *irq = vgic_get_irq(kvm, NULL, intid), *oldirq;
> >>> + int ret;
> >>>
> >>> /* In this case there is no put, since we keep the reference. */
> >>> if (irq)
> >>> @@ -66,6 +70,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
> >>> irq->config = VGIC_CONFIG_EDGE;
> >>> kref_init(&irq->refcount);
> >>> irq->intid = intid;
> >>> + irq->target_vcpu = vcpu;
> >>>
> >>> spin_lock(&dist->lpi_list_lock);
> >>>
> >>> @@ -97,6 +102,19 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
> >>> out_unlock:
> >>> spin_unlock(&dist->lpi_list_lock);
> >>>
> >>> + /*
> >>> + * We "cache" the configuration table entries in out struct vgic_irq's.
> >>
> >> s/out/our/ ?
> >>
> >>> + * However we only have those structs for mapped IRQs, so we read in
> >>> + * the respective config data from memory here upon mapping the LPI.
> >>> + */
> >>> + ret = update_lpi_config(kvm, irq, NULL);
> >>> + if (ret)
> >>> + return ERR_PTR(ret);
> >>> +
> >>> + ret = vgic_v3_lpi_sync_pending_status(kvm, irq);
> >>> + if (ret)
> >>> + return ERR_PTR(ret);
> >>> +
> >>> return irq;
> >>> }
> >>>
> >>> @@ -769,6 +787,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >>> u32 event_id = its_cmd_get_id(its_cmd);
> >>> u32 coll_id = its_cmd_get_collection(its_cmd);
> >>> struct its_ite *ite;
> >>> + struct kvm_vcpu *vcpu = NULL;
> >>> struct its_device *device;
> >>> struct its_collection *collection, *new_coll = NULL;
> >>> int lpi_nr;
> >>> @@ -814,7 +833,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >>> ite->collection = collection;
> >>> ite->lpi = lpi_nr;
> >>>
> >>> - irq = vgic_add_lpi(kvm, lpi_nr);
> >>> + if (its_is_collection_mapped(collection))
> >>> + vcpu = kvm_get_vcpu(kvm, collection->target_addr);
> >>> +
> >>> + irq = vgic_add_lpi(kvm, lpi_nr, vcpu);
> >>
> >> So, if we don't have the collection and therefore end up with irq->vcpu
> >> == NULL, how do we ever read the pending bit from memory as the affinity
> >> may later change?
> >>
> >> Is this a problem with our idea of only looking at the pending bit on
> >> vgic_add_lpi, or does it just mean that we should sample the pending bit
> >> whenever we move LPIs around to VCPUs and if the bit is set, then also
> >> set the pennding_latch (if not already set), or what should happen here?
> >
> > It means that we would need to sample that bit on MOVI and maybe MOVALL
> > as well, but this feels a bit odd. How did that bit land there the first
> > place?
>
> Without talking about save/restore, before this series the pending table
> was sync'ed on RDIST LPI enable only and that's all. This is kept.
>
> Now talking about save/restore, if we restore an LPI whose collection is
> not attached to any RDIST, we can't sync at that time. The problem
> exists if we check the pending bit on vgic_add_lpi or later, ie. at the
> end of the ITS table restore process (as I did before). I don't see in
> the spec we are supposed to read the pending table on MAPTI or MAPC.
>
Good point. If we don't have a collection for a translation that maps
to an LPI then it must mean the mapping either never existed or was
removed, and I don't see why we should have preserved any pending state
after that.
Thanks,
-Christoffer
