| Message ID | 1493898284-29504-16-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@redhat.com> > > --- > > 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? I think in any case, we should have a comment here explaining that vcpu configuration data gets picked up later? Thanks, -Christoffer > if (IS_ERR(irq)) { > if (new_coll) > vgic_its_free_collection(its, coll_id); > @@ -823,15 +845,6 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, > } > ite->irq = irq; > > - update_affinity_ite(kvm, ite); > - > - /* > - * We "cache" the configuration table entries in out struct vgic_irq's. > - * However we only have those structs for mapped IRQs, so we read in > - * the respective config data from memory here upon mapping the LPI. > - */ > - update_lpi_config(kvm, ite->irq, NULL); > - > return 0; > } > > -- > 2.5.5 >
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@redhat.com> >> >> --- >> >> 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? Thanks, M.
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@redhat.com> >>> >>> --- >>> >>> 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. Thanks Eric > > Thanks, > > M. >
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@redhat.com> > >>> > >>> --- > >>> > >>> 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
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. + * 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); if (IS_ERR(irq)) { if (new_coll) vgic_its_free_collection(its, coll_id); @@ -823,15 +845,6 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, } ite->irq = irq; - update_affinity_ite(kvm, ite); - - /* - * We "cache" the configuration table entries in out struct vgic_irq's. - * However we only have those structs for mapped IRQs, so we read in - * the respective config data from memory here upon mapping the LPI. - */ - update_lpi_config(kvm, ite->irq, NULL); - return 0; }
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@redhat.com> --- v6: creation --- virt/kvm/arm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-)