Re: [Xen-devel] [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 06.10.14 at 16:34, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >>> On 06.10.14 at 15:46, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >> >>> On 06.10.14 at 15:08, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >> >> > The other approach to do this, that I avoided because it is slower (2
> >> >> > spinlocks instead of 1), is to use the existing mapcount function.
> >> >> > mapcount is based on:
> >> >> > 
> >> >> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
> >> >> > 
> >> >> > do you think it is better?
> >> >> 
> >> >> No, that's worse: This is the Dom0 side of things, and the loop
> >> >> bound would still be controlled by that same command line option.
> >> >> 
> >> >> So between the two, the original solution would be better, and
> >> >> maybe enforcing a lower limit on DomU-s might actually be an
> >> >> option.
> >> > 
> >> > What about keeping track of the highest ref number actually used so far?
> >> > I could use that as upper limit instead of the theoretical max. It would
> >> > also make the execution faster.
> >> 
> >> But the loops are already using a value on the order of the high
> >> watermark, not the theoretical one. I.e. you're not alway looping
> >> for a long time, it is just possible for this to happen.
> > 
> > OK, I understand.
> > 
> > The default max value is 32 pages that lead to 16384 iterations in
> > grant_map_exists. Should I reduce it?
> > 
> > Given the simple operations in the loop, the function shouldn't actually
> > be so slow to be a security risk. It should take less that 0.1
> > milliseconds to complete.
> 
> I don't think there's problem with the default (i.e. no reason to reduce
> that value). But bumping the limit via command line option should still
> result in a secure system. Therefore I think the limit should be split to
> separate Dom0 and DomU ones (e.g. along the lines of the
> "extra_guest_irqs=" option), and the DomU limit should then get
> bounded more strictly. 

That makes sense.
Would you prefer having the changes as part of this series or separately?


> (And I wonder btw whether things work right
> even without your patch if passing a sufficiently big value to
> "gnttab_max_nr_frames=".)

at the very least mapcount would be badly affected

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