Re: [PATCH v2 09/27] ARM: GICv3: introduce separate pending_irq structs for LPIs

From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Vijay Kilari <vijay.kilari@gmail.com>,
	Andre Przywara <andre.przywara@arm.com>,
	george.dunlap@citrix.com, jbeulich@suse.com,
	andrew.cooper3@citrix.com, xen-devel@lists.xenproject.org,
	nd@arm.com, Shanker Donthineni <shankerd@codeaurora.org>
Subject: Re: [PATCH v2 09/27] ARM: GICv3: introduce separate pending_irq structs for LPIs
Date: Mon, 27 Mar 2017 22:24:36 +0100	[thread overview]
Message-ID: <a76048c8-e8e5-ac0e-8387-a34c2a57c45b@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1703271102240.8001@sstabellini-ThinkPad-X260>

Hi Stefano,

On 27/03/2017 19:39, Stefano Stabellini wrote:
> On Mon, 27 Mar 2017, Julien Grall wrote:
>> For both guest could potentially flood us. It would take us a lot of time to
>> allocate/free memory for each vLPIs modified. Hence, why I didn't suggest it
>> and said: "One could argue that we could allocate on MAPTI to limit the
>> allocation...".
>
> Given that Xen wouldn't allocate the same pending_irq twice, at most Xen
> would allocate the same amount of memory and the same number of
> pending_irq that it would otherwise allocate if we did it at assignment
> time, right?
>
> Therefore, we are not concerned about memory utilization. We are
> concerned about the CPU time to do the allocation itself, right?
>
> However, the CPU time to run xmalloc/xfree should be small and in any
> case the scheduler has always the chance to deschedule the vcpu if it
> wants to. This is no different than issuing any of the hypercalls that
> require some work on the hypervisor side.
>
> I don't think we have reasons to be concerned. Any opinions?

Well, I have already said on the IRQ version but forgot to mention here. 
How do you report error if xmalloc has failed? This could happen if 
memory resource has been exhausted even for a well-behaved guest.

If you do the allocation on enable, this means on a memory trap, you 
would have to inject a data abort to the guest. If you do on the MAPTI 
command, it would be a SError with a IMPLEMENTATION DEFINED error code. 
In both case the guest will likely not able interpret it and crash. This 
could be a vector attack by exhausting the resource.

Furthermore, in the case of enable/disable vLPI solution, you can 
disable an LPI that are still inflight. So you would not be able to free 
here. Furthermore enable/disable can happen often if you want to mask 
the interrupt temporarily (all MSI are edge-interrupt).

Lastly, in the case of MAPTI it is not possible to preempt the command 
queue that can contain ~32K of commands. So we may have to do 32K of 
xmalloc/xfree. Even one MAPTI is fast, likely 32K will be slow and 
monopolize a pCPU for more than expected.

But can we focus on getting a sensible design which can be easily 
extended and without any flaw? This already is quite a difficult tasks 
with the ITS. Asking also for performance from the first draft is making 
much worse.

If you recall, a lot of ARM subsystems (P2M, vGIC...) has been built in 
multiple stage. Each stage improved the performance. It sounds quite 
unfair to me to require the same level of performance as the current 
vGIC just because the code was merged later one.

This is raising the bar to contribute on Xen on ARM very high and may 
discourage someone to push something upstream.

To be clear, I do care about performance. But I think this has to come 
in a second step when it is too complicate to address as a first step. 
The first step is to be able to use Xen on the hardware without any flaw 
and allowing us to extend the code easily.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel