Re: [PATCH] KVM: arm64: ITS: avoid re-mapping LPIs

[Christoffer Dall <christoffer.dall@linaro.org>]

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@arm.com>
> >> ---
> >>  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