Re: [PATCH v2 06/12] xen/gnttab: Pass max_{grant, maptrack}_frames into grant_table_create()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/08/18 14:04, Julien Grall wrote:
> Hi Andrew,
>
> On 08/13/2018 11:01 AM, Andrew Cooper wrote:
>> ... rather than setting the limits up after domain_create() has
>> completed.
>>
>> This removes all the gnttab infrastructure for calculating the number
>> of dom0
>> grant frames, opting instead to require the dom0 construction code to
>> pass a
>> sane value in via the configuration.
>>
>> In practice, this now means that there is never a partially
>> constructed grant
>> table for a reference-able domain.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> v2:
>>   * Split/rearrange to avoid the post-domain-create error path.
>> ---
>>   xen/arch/arm/domain_build.c       |  3 ++-
>>   xen/arch/arm/setup.c              | 12 ++++++++++++
>>   xen/arch/x86/setup.c              |  3 +++
>>   xen/common/domain.c               |  3 ++-
>>   xen/common/grant_table.c          | 16 +++-------------
>>   xen/include/asm-arm/grant_table.h | 12 ------------
>>   xen/include/asm-x86/grant_table.h |  5 -----
>>   xen/include/xen/grant_table.h     |  6 ++----
>>   8 files changed, 24 insertions(+), 36 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 1351572..737e0f3 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2079,7 +2079,8 @@ static void __init find_gnttab_region(struct
>> domain *d,
>>        * enough space for a large grant table
>>        */
>>       kinfo->gnttab_start = __pa(_stext);
>> -    kinfo->gnttab_size = gnttab_dom0_frames() << PAGE_SHIFT;
>> +    kinfo->gnttab_size = min_t(unsigned int, opt_max_grant_frames,
>> +                               PFN_DOWN(_etext - _stext)) <<
>> PAGE_SHIFT;
>
>
> I agree with Jan's comment on v1 that there is a risk someone will
> update the size here but ...
>
>
>>     #ifdef CONFIG_ARM_32
>>       /*
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 45f3841..3d3b30c 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -20,6 +20,7 @@
>>   #include <xen/compile.h>
>>   #include <xen/device_tree.h>
>>   #include <xen/domain_page.h>
>> +#include <xen/grant_table.h>
>>   #include <xen/types.h>
>>   #include <xen/string.h>
>>   #include <xen/serial.h>
>> @@ -693,6 +694,17 @@ void __init start_xen(unsigned long
>> boot_phys_offset,
>>       struct domain *dom0;
>>       struct xen_domctl_createdomain dom0_cfg = {
>>           .max_evtchn_port = -1,
>> +
>> +        /*
>> +         * The region used by Xen on the memory will never be mapped
>> in DOM0
>> +         * memory layout. Therefore it can be used for the grant table.
>> +         *
>> +         * Only use the text section as it's always present and will
>> contain
>> +         * enough space for a large grant table
>> +         */
>> +        .max_grant_frames = min_t(unsigned int, opt_max_grant_frames,
>> +                                  PFN_DOWN(_etext - _stext)),
>
> ... not here. So I would prefer if we either keep an helper to find
> the size of pass that size around to domain_build. Do we store the
> size in the domain information?

I have to admit that I'm somewhat perplexed by ARM's
find_gnttab_region(), and I'm not sure why it exists.

The value is available from d->grant_table.max_grant_frames but ISTR
finding that the order of construction meant that it wasn't available
when needed (although this was all from code inspection, so I could very
easily be wrong).

~Andrew

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