* Re: [PATCH] KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections
@ 2019-12-13 10:53 ` Zenghui Yu
0 siblings, 0 replies; 14+ messages in thread
From: Zenghui Yu @ 2019-12-13 10:53 UTC (permalink / raw)
To: Eric Auger, eric.auger.pro, maz, linux-kernel, kvmarm
Hi Eric,
On 2019/12/13 17:42, Eric Auger wrote:
> Saving/restoring an unmapped collection is a valid scenario. For
> example this happens if a MAPTI command was sent, featuring an
> unmapped collection. At the moment the CTE fails to be restored.
> Only compare against the number of online vcpus if the rdist
> base is set.
Have you actually seen a problem and this patch fixed it? To be honest,
I'm surprised to find that we can map a LPI to an unmapped collection ;)
(and prevent it to be delivered to vcpu with an INT_UNMAPPED_INTERRUPT
error, until someone had actually mapped the collection).
After a quick glance of spec (MAPTI), just as you said, this is valid.
If Marc has no objection to this fix, please add
Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Thanks,
Zenghui
>
> Cc: stable@vger.kernel.org # v4.11+
> Fixes: ea1ad53e1e31a ("KVM: arm64: vgic-its: Collection table save/restore")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
> virt/kvm/arm/vgic/vgic-its.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 98c7360d9fb7..17920d1b350a 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2475,7 +2475,8 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
> coll_id = val & KVM_ITS_CTE_ICID_MASK;
>
> - if (target_addr >= atomic_read(&kvm->online_vcpus))
> + if (target_addr != COLLECTION_NOT_MAPPED &&
> + target_addr >= atomic_read(&kvm->online_vcpus))
> return -EINVAL;
>
> collection = find_collection(its, coll_id);
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections
2019-12-13 10:53 ` Zenghui Yu
@ 2019-12-13 11:06 ` Auger Eric
-1 siblings, 0 replies; 14+ messages in thread
From: Auger Eric @ 2019-12-13 11:06 UTC (permalink / raw)
To: Zenghui Yu, eric.auger.pro, maz, linux-kernel, kvmarm
Hi Zenghui,
On 12/13/19 11:53 AM, Zenghui Yu wrote:
> Hi Eric,
>
> On 2019/12/13 17:42, Eric Auger wrote:
>> Saving/restoring an unmapped collection is a valid scenario. For
>> example this happens if a MAPTI command was sent, featuring an
>> unmapped collection. At the moment the CTE fails to be restored.
>> Only compare against the number of online vcpus if the rdist
>> base is set.
>
> Have you actually seen a problem and this patch fixed it?
It is not with a linux guest but with kvm-unit-test.
To be honest,
> I'm surprised to find that we can map a LPI to an unmapped collection ;)
> (and prevent it to be delivered to vcpu with an INT_UNMAPPED_INTERRUPT
> error, until someone had actually mapped the collection).
> After a quick glance of spec (MAPTI), just as you said, this is valid.
>
> If Marc has no objection to this fix, please add
>
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Thank you for the review.
Eric
>
>
> Thanks,
> Zenghui
>
>>
>> Cc: stable@vger.kernel.org # v4.11+
>> Fixes: ea1ad53e1e31a ("KVM: arm64: vgic-its: Collection table
>> save/restore")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> virt/kvm/arm/vgic/vgic-its.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 98c7360d9fb7..17920d1b350a 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -2475,7 +2475,8 @@ static int vgic_its_restore_cte(struct vgic_its
>> *its, gpa_t gpa, int esz)
>> target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
>> coll_id = val & KVM_ITS_CTE_ICID_MASK;
>> - if (target_addr >= atomic_read(&kvm->online_vcpus))
>> + if (target_addr != COLLECTION_NOT_MAPPED &&
>> + target_addr >= atomic_read(&kvm->online_vcpus))
>> return -EINVAL;
>> collection = find_collection(its, coll_id);
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections
@ 2019-12-13 11:06 ` Auger Eric
0 siblings, 0 replies; 14+ messages in thread
From: Auger Eric @ 2019-12-13 11:06 UTC (permalink / raw)
To: Zenghui Yu, eric.auger.pro, maz, linux-kernel, kvmarm
Hi Zenghui,
On 12/13/19 11:53 AM, Zenghui Yu wrote:
> Hi Eric,
>
> On 2019/12/13 17:42, Eric Auger wrote:
>> Saving/restoring an unmapped collection is a valid scenario. For
>> example this happens if a MAPTI command was sent, featuring an
>> unmapped collection. At the moment the CTE fails to be restored.
>> Only compare against the number of online vcpus if the rdist
>> base is set.
>
> Have you actually seen a problem and this patch fixed it?
It is not with a linux guest but with kvm-unit-test.
To be honest,
> I'm surprised to find that we can map a LPI to an unmapped collection ;)
> (and prevent it to be delivered to vcpu with an INT_UNMAPPED_INTERRUPT
> error, until someone had actually mapped the collection).
> After a quick glance of spec (MAPTI), just as you said, this is valid.
>
> If Marc has no objection to this fix, please add
>
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Thank you for the review.
Eric
>
>
> Thanks,
> Zenghui
>
>>
>> Cc: stable@vger.kernel.org # v4.11+
>> Fixes: ea1ad53e1e31a ("KVM: arm64: vgic-its: Collection table
>> save/restore")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> virt/kvm/arm/vgic/vgic-its.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 98c7360d9fb7..17920d1b350a 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -2475,7 +2475,8 @@ static int vgic_its_restore_cte(struct vgic_its
>> *its, gpa_t gpa, int esz)
>> target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
>> coll_id = val & KVM_ITS_CTE_ICID_MASK;
>> - if (target_addr >= atomic_read(&kvm->online_vcpus))
>> + if (target_addr != COLLECTION_NOT_MAPPED &&
>> + target_addr >= atomic_read(&kvm->online_vcpus))
>> return -EINVAL;
>> collection = find_collection(its, coll_id);
>>
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections
2019-12-13 10:53 ` Zenghui Yu
@ 2019-12-13 11:28 ` Marc Zyngier
-1 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2019-12-13 11:28 UTC (permalink / raw)
To: Zenghui Yu; +Cc: Eric Auger, eric.auger.pro, linux-kernel, kvmarm
Hi Zenghui,
On 2019-12-13 10:53, Zenghui Yu wrote:
> Hi Eric,
>
> On 2019/12/13 17:42, Eric Auger wrote:
>> Saving/restoring an unmapped collection is a valid scenario. For
>> example this happens if a MAPTI command was sent, featuring an
>> unmapped collection. At the moment the CTE fails to be restored.
>> Only compare against the number of online vcpus if the rdist
>> base is set.
>
> Have you actually seen a problem and this patch fixed it? To be
> honest,
> I'm surprised to find that we can map a LPI to an unmapped collection
> ;)
> (and prevent it to be delivered to vcpu with an
> INT_UNMAPPED_INTERRUPT
> error, until someone had actually mapped the collection).
> After a quick glance of spec (MAPTI), just as you said, this is
> valid.
Yes, this is one of the (many) odd bits in the architecture. And there
is
a bizarre wording in the MAPC description when V=0:
"Behavior is unpredictable if there are interrupts that are mapped to
the
specified collection, with the restriction that further translation
requests
from that device are ignored."
It is really odd that:
- it is unpredictable to unmap the collection with mapped interrupts,
but mapping interrupts to an unmapped collection is fine
- the notion of "interrupts from that device" doesn't match any of the
MAPC parameters
Do you hate the GIC already? ;-)
> If Marc has no objection to this fix, please add
>
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Thanks for that, I've applied it to the patch and will push out
the update as soon as ra.kernel.org is reachable again.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections
@ 2019-12-13 11:28 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2019-12-13 11:28 UTC (permalink / raw)
To: Zenghui Yu; +Cc: kvmarm, linux-kernel, eric.auger.pro
Hi Zenghui,
On 2019-12-13 10:53, Zenghui Yu wrote:
> Hi Eric,
>
> On 2019/12/13 17:42, Eric Auger wrote:
>> Saving/restoring an unmapped collection is a valid scenario. For
>> example this happens if a MAPTI command was sent, featuring an
>> unmapped collection. At the moment the CTE fails to be restored.
>> Only compare against the number of online vcpus if the rdist
>> base is set.
>
> Have you actually seen a problem and this patch fixed it? To be
> honest,
> I'm surprised to find that we can map a LPI to an unmapped collection
> ;)
> (and prevent it to be delivered to vcpu with an
> INT_UNMAPPED_INTERRUPT
> error, until someone had actually mapped the collection).
> After a quick glance of spec (MAPTI), just as you said, this is
> valid.
Yes, this is one of the (many) odd bits in the architecture. And there
is
a bizarre wording in the MAPC description when V=0:
"Behavior is unpredictable if there are interrupts that are mapped to
the
specified collection, with the restriction that further translation
requests
from that device are ignored."
It is really odd that:
- it is unpredictable to unmap the collection with mapped interrupts,
but mapping interrupts to an unmapped collection is fine
- the notion of "interrupts from that device" doesn't match any of the
MAPC parameters
Do you hate the GIC already? ;-)
> If Marc has no objection to this fix, please add
>
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
Thanks for that, I've applied it to the patch and will push out
the update as soon as ra.kernel.org is reachable again.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections
2019-12-13 11:28 ` Marc Zyngier
@ 2019-12-13 14:22 ` Zenghui Yu
-1 siblings, 0 replies; 14+ messages in thread
From: Zenghui Yu @ 2019-12-13 14:22 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Eric Auger, eric.auger.pro, linux-kernel, kvmarm
Hi Marc,
On 2019/12/13 19:28, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2019-12-13 10:53, Zenghui Yu wrote:
>> Hi Eric,
>>
>> On 2019/12/13 17:42, Eric Auger wrote:
>>> Saving/restoring an unmapped collection is a valid scenario. For
>>> example this happens if a MAPTI command was sent, featuring an
>>> unmapped collection. At the moment the CTE fails to be restored.
>>> Only compare against the number of online vcpus if the rdist
>>> base is set.
>>
>> Have you actually seen a problem and this patch fixed it? To be honest,
>> I'm surprised to find that we can map a LPI to an unmapped collection ;)
>> (and prevent it to be delivered to vcpu with an INT_UNMAPPED_INTERRUPT
>> error, until someone had actually mapped the collection).
>> After a quick glance of spec (MAPTI), just as you said, this is valid.
>
> Yes, this is one of the (many) odd bits in the architecture. And there is
> a bizarre wording in the MAPC description when V=0:
>
> "Behavior is unpredictable if there are interrupts that are mapped to the
> specified collection, with the restriction that further translation
> requests
> from that device are ignored."
>
> It is really odd that:
>
> - it is unpredictable to unmap the collection with mapped interrupts,
> but mapping interrupts to an unmapped collection is fine
Yes, looks odd... Without Eric's patch, I won't even notice it.
I guess that unmapping the collection with mapped interrupts will make
it difficult for Hardware to manage those interrupts' internal states,
but only a guess.
> - the notion of "interrupts from that device" doesn't match any of the
> MAPC parameters
Looks like a writing mistake, a better statement *might be*
"further translation requests targeting that ICID are ignored"?
> Do you hate the GIC already? ;-)
Not yet! (I'd like to continue being messed with GIC and see when I
will hate it :)
>
>> If Marc has no objection to this fix, please add
>>
>> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
>
> Thanks for that, I've applied it to the patch and will push out
> the update as soon as ra.kernel.org is reachable again.
Thanks,
Zenghui
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections
@ 2019-12-13 14:22 ` Zenghui Yu
0 siblings, 0 replies; 14+ messages in thread
From: Zenghui Yu @ 2019-12-13 14:22 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvmarm, linux-kernel, eric.auger.pro
Hi Marc,
On 2019/12/13 19:28, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2019-12-13 10:53, Zenghui Yu wrote:
>> Hi Eric,
>>
>> On 2019/12/13 17:42, Eric Auger wrote:
>>> Saving/restoring an unmapped collection is a valid scenario. For
>>> example this happens if a MAPTI command was sent, featuring an
>>> unmapped collection. At the moment the CTE fails to be restored.
>>> Only compare against the number of online vcpus if the rdist
>>> base is set.
>>
>> Have you actually seen a problem and this patch fixed it? To be honest,
>> I'm surprised to find that we can map a LPI to an unmapped collection ;)
>> (and prevent it to be delivered to vcpu with an INT_UNMAPPED_INTERRUPT
>> error, until someone had actually mapped the collection).
>> After a quick glance of spec (MAPTI), just as you said, this is valid.
>
> Yes, this is one of the (many) odd bits in the architecture. And there is
> a bizarre wording in the MAPC description when V=0:
>
> "Behavior is unpredictable if there are interrupts that are mapped to the
> specified collection, with the restriction that further translation
> requests
> from that device are ignored."
>
> It is really odd that:
>
> - it is unpredictable to unmap the collection with mapped interrupts,
> but mapping interrupts to an unmapped collection is fine
Yes, looks odd... Without Eric's patch, I won't even notice it.
I guess that unmapping the collection with mapped interrupts will make
it difficult for Hardware to manage those interrupts' internal states,
but only a guess.
> - the notion of "interrupts from that device" doesn't match any of the
> MAPC parameters
Looks like a writing mistake, a better statement *might be*
"further translation requests targeting that ICID are ignored"?
> Do you hate the GIC already? ;-)
Not yet! (I'd like to continue being messed with GIC and see when I
will hate it :)
>
>> If Marc has no objection to this fix, please add
>>
>> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
>
> Thanks for that, I've applied it to the patch and will push out
> the update as soon as ra.kernel.org is reachable again.
Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread