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:32, Julien Grall wrote:
> Hi Andrew,
>>>>      #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.
>
> Dom0 is using the host memory layout that may differ between
> platforms. So there is not a region address that would fit everyone.
>
> This function is here to find at boot a suitable region in the layout
> where the OS can map the grant-table. The result will be written in
> the firmware table.
>
>>
>> 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).
>
> I think it should be fine for Dom0 as find_gnttab_region is called
> from construct_dom0 and d->grant_table.max_grant_frames would be set
> before via domain_create().
>
> Assuming d->grant_table.max_grant_frames can only be 0 before
> initialization, I would potentially add a
> BUG_ON(!d->grant_table.max_grant_frames) to make sure this always stay
> like that.

Actually, I remember now what the problem was.  d->grant_table is an
opaque type, so .max_grant_frames can't be accessed.

One of my indented bits of cleanup here is to remove the
gnttab_dom0_frames() function, because it has no business living in the
core grant_table.c

Would you be happy if I replaced gnttab_dom0_max() in asm-arm with
gnttab_dom0_frames() which accounts for the exiting min(), and means
that domain_build.c will be ultimately unchanged?

~Andrew

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