Re: [PATCH 0/9] gnttab: further work from XSA-380 / -382 context

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 07/10/2022 14:49, Jan Beulich wrote:
> On 26.08.2021 12:06, Jan Beulich wrote:
>> The first four patches can be attributed to the former, the last four
>> patches to the latter. The middle patch had been submitted standalone
>> before, has a suitable Reviewed-by tag, but also has an objection by
>> Andrew pending, which unfortunately has lead to this patch now being
>> stuck. Short of Andrew being willing to settle the disagreement more
>> with Julien than with me (although I'm on Julien's side), I have no
>> idea what to do here.
>>
>> There's probably not much interrelation between the patches, so they
>> can perhaps go in about any order.
>>
>> 1: defer allocation of maptrack frames table
>> 2: drop a redundant expression from gnttab_release_mappings()
>> 3: fold recurring is_iomem_page()
>> 4: drop GNTMAP_can_fail
>> 5: defer allocation of status frame tracking array
> Just to make "official" what I've said in the course of the resource
> management discussion at the event in Cambridge: I'm withdrawing 1
> and 5, in the expectation that eager/lazy allocation of resources
> will become a property to be honored uniformly for a guest. With 2,
> 3, 4, and 6 already having gone in, it would still be nice to
> (finally) have feedback on ...

To these issues in particular, there was specific work done in 4.16 to
address the impasse in a way which got the savings in most cases, but
without increasing the risk of hitting known buggy paths in guest drivers.


For patch 5, the ABI max version is now known to domain_create(), so the
status array allocation can safely be skipped when gnttab v2 isn't
available.

This gets the saving Julien wants on ARM, and on x86 we ought to
encourage people to restricting to v1 where possible, so they get the
savings too.


For patch 1, I agree with the observation that 1024 maptrack frames is a
silly default to have.  Adjusting this appropriately will result in the
kind of savings wanted in the patch, without modifying the hypervisor
directly.

We should have a patch to xl (or is it libxl?) to make a better choice
than blindly picking 1024.

Having written this out, something does strike me as odd.  These limits
are specified in frames, and for the gnttab limit, this equates into a
known number of grants.  But struct maptrack is internal Xen, so the
toolstack selecting 1024 frames can't know how many grant maps that
actually equates to in Xen.  Right now, 1024 maptrack frames equates to
2^18 mappings (I think).


For traditional server-virt VMs, they can have 0 because the frontends
should never be mapping grants.  We actually already do this for the
xenstored stubdom.  The same is almost certainly true for qemu stubdoms.

For VMs in a bit of a more interesting configuration, e.g. with virtio,
then they need "some".

For device driver VMs, they do need a dom0-like mapping capabilities.

Either way, there is definitely room to improve the status quo without
impacting runtime safety.  If patches were to appear promptly, I think
there's probably wiggle room to consider them for 4.17 at this point.

~Andrew