Re: [v4][PATCH 2/9] xen:x86: define a new hypercall to get RMRR mappings

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

On 26/08/14 04:12, Chen, Tiejun wrote:
> On 2014/8/25 20:07, Andrew Cooper wrote:
>> On 25/08/14 12:21, Chen, Tiejun wrote:
>>> On 2014/8/22 18:53, Andrew Cooper wrote:
>>>> On 22/08/14 11:09, Tiejun Chen wrote:
>>>>> We need this new hypercall to get RMRR mapping for VM.
>>>>>
>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>> ---
>>>>>    xen/arch/x86/mm.c           | 71
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>    xen/include/public/memory.h | 37 ++++++++++++++++++++++-
>>>>>    2 files changed, 107 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>>>> index d23cb3f..e0d6650 100644
>>>>> --- a/xen/arch/x86/mm.c
>>>>> +++ b/xen/arch/x86/mm.c
>>>>> @@ -123,6 +123,7 @@
>>>>>    #include <asm/setup.h>
>>>>>    #include <asm/fixmap.h>
>>>>>    #include <asm/pci.h>
>>>>> +#include <asm/acpi.h>
>>>>>
>>>>>    /* Mapping of the fixmap space needed early. */
>>>>>    l1_pgentry_t __attribute__ ((__section__ (".bss.page_aligned")))
>>>>> @@ -4842,6 +4843,76 @@ long arch_memory_op(unsigned long cmd,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>            return rc;
>>>>>        }
>>>>>
>>>>> +    case XENMEM_reserved_device_memory_map:
>>>>> +    {
>>>>> +        struct xen_reserved_device_memory_map map;
>>>>> +        XEN_GUEST_HANDLE(xen_reserved_device_memory_t) buffer;
>>>>> +        XEN_GUEST_HANDLE_PARAM(xen_reserved_device_memory_t)
>>>>> buffer_param;
>>>>> +        unsigned int i = 0;
>>>>> +        static unsigned int nr_entries = 0;
>>>>> +        static struct xen_reserved_device_memory *rmrr_map;
>>>>
>>>> Absolutely not.  This hypercall can easy be run concurrently.
>>>>
>>>>> +        struct acpi_rmrr_unit *rmrr;
>>>>> +
>>>>> +        if ( copy_from_guest(&map, arg, 1) )
>>>>> +            return -EFAULT;
>>>>> +
>>>>> +        if ( !nr_entries )
>>>>> +            /* Currently we just need to cover RMRR. */
>>>>> +            list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>>>>> +                nr_entries++;
>>>>
>>>> Maintain a global count as entries are added/removed from this
>>>> list.  It
>>>> is a waste of time recounting this list for each hypercall.
>>>
>>> Are you saying push this 'nr_entries' as a global count somewhere? I
>>> guess I can set this when we first construct acpi_rmrr_units in ACPI
>>> stuff.
>>
>> Not named "nr_entries", but yes.  It is constant after boot.
>>
>>>
>>>>
>>>>> +
>>>>> +        if ( !nr_entries )
>>>>> +                return -ENOENT;
>>>>> +        else
>>>>> +        {
>>>>> +            if ( rmrr_map == NULL )
>>>>> +            {
>>>>> +                rmrr_map =
>>>>> xmalloc_array(xen_reserved_device_memory_t,
>>>>> +                                         nr_entries);
>>>>
>>>> You can do all of this without any memory allocation.
>>>
>
> What is your way without any memory allocation? Do you mean I should
> predefine a static array here? But how to predetermine the size?
>
> Or you mean I should do something with one rmrr_map, like this,
>
>     struct xen_mem_reserved_device_memory rmrr_map;
>
>     list_for_each_entry( rmrr, &acpi_rmrr_units, list )
>     {
>         rmrr_map.start_pfn = ...;
>         rmrr_map.nr_pages = ...;
>         if ( copy_to_guest_offset(buffer, i, &rmrr_map, 1)
> )            return -EFAULT;
>         i++;
>     }

Yes - that is along the right lines.

~Andrew