Re: [v7][RFC][PATCH 04/13] hvmloader/util: get reserved device memory maps

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/10/24 22:22, Jan Beulich wrote:
>>>> On 24.10.14 at 09:34, <tiejun.chen@intel.com> wrote:
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -828,6 +828,72 @@ int hpet_exists(unsigned long hpet_base)
>>       return ((hpet_id >> 16) == 0x8086);
>>   }
>>
>> +int get_reserved_device_memory_map(struct xen_mem_reserved_device_memory entries[],
>> +                                   uint32_t *max_entries)
>> +{
>> +    int rc;
>> +    struct xen_mem_reserved_device_memory_map memmap = {
>> +        .nr_entries = *max_entries
>> +    };
>> +
>> +    set_xen_guest_handle(memmap.buffer, entries);
>> +
>> +    rc = hypercall_memory_op(XENMEM_reserved_device_memory_map, &memmap);
>> +    if (rc == -ENOBUFS)
>
> Coding style.

     if ( rc == -ENOBUFS )

>
>> +        *max_entries = memmap.nr_entries;
>> +
>> +    return rc;
>> +}
>> +
>> +/* Getting all reserved device memory map info in case of hvmloader. */
>> +int hvm_get_reserved_device_memory_map(void)
>> +{
>> +    static uint32_t nr_entries = 0;
>> +    int rc = 0;
>> +
>> +    if ( !rdm_map )
>> +    {
>> +        /* Assume we have one entry if not enough we'll expand.*/
>> +        nr_entries = 1;
>> +        rdm_map = mem_alloc(nr_entries *
>> +                            sizeof(struct xen_mem_reserved_device_memory), 0);
>> +        if ( !rdm_map )
>> +        {
>> +            printf("No space to get reserved dev memory maps!\n");
>> +            return rc;
>> +        }
>> +
>> +        rc = get_reserved_device_memory_map(rdm_map, &nr_entries);
>> +        if ( rc == -ENOBUFS )
>> +        {
>> +            rdm_map = mem_alloc(nr_entries *
>> +                                sizeof(struct xen_mem_reserved_device_memory),
>> +                                0);
>> +            if ( rdm_map )
>> +            {
>> +                rc = get_reserved_device_memory_map(rdm_map, &nr_entries);
>> +                if ( rc )
>> +                {
>> +                    printf("Could not get reserved dev memory info on domain");
>> +                    return rc;
>> +                }
>> +            }
>> +            else
>> +            {
>> +                printf("No space to get reserved dev memory maps!\n");
>> +                return rc;
>> +            }
>> +        }
>> +        else if ( rc )
>> +        {
>> +            printf("Could not get reserved dev memory info on domain");
>> +            return rc;
>> +        }
>> +    }
>> +
>> +    return nr_entries;
>> +}
>
> I continue to think that adding these functions without user isn't
> really worthwhile in a separate patch.

These functions will be called in pci.c:pci_setup() and 
e820.c:build_e820_table(). If you really like one big patch I can squash 
these three patches as one.

>
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -241,6 +241,12 @@ int build_e820_table(struct e820entry *e820,
>>                        unsigned int bios_image_base);
>>   void dump_e820_table(struct e820entry *e820, unsigned int nr);
>>
>> +#include <xen/memory.h>
>> +#define ENOBUFS     105 /* No buffer space available */
>
> This is a joke I hope? The #include belongs at the top (albeit afaict
> you don't really need it here), and the #define is completely

If without this line, #include <xen/memory.h>,

In file included from build.c:25:0:
../util.h:246:70: error: array type has incomplete element type
  int get_reserved_device_memory_map(struct xen_reserved_device_memory 
entries[],
                                                                       ^
make[8]: *** [build.o] Error 1

> misplaced here. While I generally wouldn't recommend doing this, I
> think in the case here including the hypervisor header that defines
> them would be okay. Perhaps not via relative path, but via having

Seems we just need to include this,

#include <errno.h>

> the Makefile symlink the hypervisor header here.
>
>> +struct xen_mem_reserved_device_memory *rdm_map;
>> +int get_reserved_device_memory_map(struct xen_mem_reserved_device_memory entries[],
>> +                                   uint32_t *max_entries);
>> +int hvm_get_reserved_device_memory_map(void);
>>   #ifndef NDEBUG
>
> Blank line missing after your additions.
>

Added.

Thanks
Tiejun