| Message ID | 20191225133014.1825-1-yuzenghui@huawei.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 821c10c2ae0bac5a8503cc7e961e7af90ea676eb |
| Headers | show |
| Series | KVM: arm/arm64: vgic-its: Check hopefully the last DISCARD command error | expand |
Hi Zenghui, On 12/25/19 2:30 PM, Zenghui Yu wrote: > DISCARD command error occurs if any of the following apply: > > - [ ... (those which we have already handled) ] nit: I would remove the above and simply say the discard is supposed to fail if the collection is not mapped to any target redistributor. If an ITE exists then the ite->collection is non NULL. What needs to be checked is its_is_collection_mapped(). By the way update_affinity_collection() also tests ite->collection. I think this is useless or do I miss something? Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > - The EventID for the device is mapped to a collection that > has not been mapped to an RDbase using MAPC. > > Let's take the unmapped collection case into account and report > a DISCARD command error if it really happens. > > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> > --- > virt/kvm/arm/vgic/vgic-its.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 17920d1b350a..d53d34a33e35 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -839,9 +839,8 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its, > u32 event_id = its_cmd_get_id(its_cmd); > struct its_ite *ite; > > - > ite = find_ite(its, device_id, event_id); > - if (ite && ite->collection) { > + if (ite && its_is_collection_mapped(ite->collection)) { > /* > * Though the spec talks about removing the pending state, we > * don't bother here since we clear the ITTE anyway and the >
Hi Eric, On 2020/1/10 16:37, Auger Eric wrote: > Hi Zenghui, > > On 12/25/19 2:30 PM, Zenghui Yu wrote: >> DISCARD command error occurs if any of the following apply: >> >> - [ ... (those which we have already handled) ] > nit: I would remove the above and simply say the discard is supposed to > fail if the collection is not mapped to any target redistributor. If an > ITE exists then the ite->collection is non NULL. I think this is not always true. Let's talk about the following scenario (a bit insane, though): 1. First map a LPI to an unmapped Collection, then ite->collection is non NULL and its target_addr is COLLECTION_NOT_MAPPED. 2. Then issue MAPC and unMAPC(V=0) commands on this Collection, the ite->collection will be NULL, see vgic_its_free_collection(). Discard the LPI mapping after "1" or "2", we will both encounter the unmapped collection command error. > What needs to be checked is its_is_collection_mapped(). > > By the way update_affinity_collection() also tests ite->collection. I > think this is useless or do I miss something? Yeah, I agree. We managed to invoke update_affinity_collection(,, coll), ensure that the 'coll' can _not_ be NULL. So '!ite->collection' is already a subcase of 'coll != ite->collection'. We can safely get rid of it. > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > Thanks for that. I'll change the commit message with your suggestion and add your R-b in v2. Thanks, Zenghui
Hi Zenghui, On 1/14/20 8:10 AM, Zenghui Yu wrote: > Hi Eric, > > On 2020/1/10 16:37, Auger Eric wrote: >> Hi Zenghui, >> >> On 12/25/19 2:30 PM, Zenghui Yu wrote: >>> DISCARD command error occurs if any of the following apply: >>> >>> - [ ... (those which we have already handled) ] >> nit: I would remove the above and simply say the discard is supposed to >> fail if the collection is not mapped to any target redistributor. If an >> ITE exists then the ite->collection is non NULL. > > I think this is not always true. Let's talk about the following scenario > (a bit insane, though): > > 1. First map a LPI to an unmapped Collection, then ite->collection is > non NULL and its target_addr is COLLECTION_NOT_MAPPED. > > 2. Then issue MAPC and unMAPC(V=0) commands on this Collection, the > ite->collection will be NULL, see vgic_its_free_collection(). You're right I missed that case. > > Discard the LPI mapping after "1" or "2", we will both encounter the > unmapped collection command error. > >> What needs to be checked is its_is_collection_mapped(). >> >> By the way update_affinity_collection() also tests ite->collection. I >> think this is useless or do I miss something? > > Yeah, I agree. We managed to invoke update_affinity_collection(,, coll), > ensure that the 'coll' can _not_ be NULL. > So '!ite->collection' is already a subcase of 'coll != ite->collection'. > We can safely get rid of it. OK. But that's not for the (wrong) reason I mentioned above. So it is a minor cleanup and you may just leave it as is and just focus on this fix. Thanks Eric > >> >> Reviewed-by: Eric Auger <eric.auger@redhat.com> >> > > Thanks for that. I'll change the commit message with your suggestion and > add your R-b in v2. > > > Thanks, > Zenghui > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 17920d1b350a..d53d34a33e35 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -839,9 +839,8 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its, u32 event_id = its_cmd_get_id(its_cmd); struct its_ite *ite; - ite = find_ite(its, device_id, event_id); - if (ite && ite->collection) { + if (ite && its_is_collection_mapped(ite->collection)) { /* * Though the spec talks about removing the pending state, we * don't bother here since we clear the ITTE anyway and the
DISCARD command error occurs if any of the following apply: - [ ... (those which we have already handled) ] - The EventID for the device is mapped to a collection that has not been mapped to an RDbase using MAPC. Let's take the unmapped collection case into account and report a DISCARD command error if it really happens. Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> --- virt/kvm/arm/vgic/vgic-its.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)