On Wed, Aug 17, 2016 at 12:45:17AM +0100, André Przywara wrote: > On 16/08/16 18:30, Christoffer Dall wrote: > > On Tue, Aug 16, 2016 at 05:51:06PM +0100, Andre Przywara wrote: > >> When a guest wants to map a device-ID/event-ID combination that is > >> already mapped, we may end up in a situation where an LPI is never > >> "put", thus never being freed. > >> Since the GICv3 spec says that mapping an already mapped LPI is > >> UNPREDICTABLE, lets just bail out early in this situation to avoid > >> any potential leaks. > >> > >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> > >> --- > >> virt/kvm/arm/vgic/vgic-its.c | 27 +++++++++++++-------------- > >> 1 file changed, 13 insertions(+), 14 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > >> index 9533080..4660a7d 100644 > >> --- a/virt/kvm/arm/vgic/vgic-its.c > >> +++ b/virt/kvm/arm/vgic/vgic-its.c > >> @@ -731,7 +731,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, > >> u32 device_id = its_cmd_get_deviceid(its_cmd); > >> u32 event_id = its_cmd_get_id(its_cmd); > >> u32 coll_id = its_cmd_get_collection(its_cmd); > >> - struct its_itte *itte, *new_itte = NULL; > >> + struct its_itte *itte; > >> struct its_device *device; > >> struct its_collection *collection, *new_coll = NULL; > >> int lpi_nr; > >> @@ -749,6 +749,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, > >> lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser)) > >> return E_ITS_MAPTI_PHYSICALID_OOR; > >> > >> + /* If there is an existing mapping, behavior is UNPREDICTABLE. */ > >> + if (find_itte(its, device_id, event_id)) > >> + return 0; > >> + > > > > By the way, this made me think how these errors are handled, and unless > > I'm mistaken, the return value from vgic_its_handle_command() is simply > > discarded, so even when we return things like -ENOMEM, this is just > > ignored? Is this really the intention? > > Yes, at least at the moment. The spec does not specify how ITS errors > should be communicated (IMPLEMENTATION DEFINED), only that an error > condition itself can be signaled via an SError - for which atm we lack > any code to inject, if I am not mistaken. > Still I wanted to assign those error codes: IMHO it improves readability > and simplifies any later extension in that respect. It's fine to return error codes, but at the very least we should have a comment saying "We throw away all errors because we cannot handle them and this is always fine to do, because of X". > > For the Linux errors (like -ENOMEM): Due to the asynchronous nature of > the ITS command handling and also the guest triggering the commands, > there is really no better way to report those OoM conditions, for > instance, so I treated them the same as "proper" ITS errors. I feel like a -ENOMEM should be reported back to userspace so we can give up on our giant resource hogging VM instead of just grinding on. Isn't this all done as part of a MMIO write, so you can return the error from that thing? If you were running this in a separate thread, it would be an entirely more difficult matter. Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
