Re: [v8][PATCH 07/17] hvmloader/util: get reserved device memory maps

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

On 2014/12/3 4:01, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 01, 2014 at 05:24:25PM +0800, Tiejun Chen wrote:
>> We need to use reserved device memory maps with multiple times, so
>> provide just one common function should be friend.
>
> We need to call reserved device memory maps hypercall
> (XENMEM_reserved_device_memory_map) many times, hence provide one common function.

Rephrased and thanks.

>
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/util.c | 59 +++++++++++++++++++++++++++++++++++++++++
>>   tools/firmware/hvmloader/util.h |  2 ++
>>   2 files changed, 61 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
>> index 80d822f..dd81fb6 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -22,11 +22,14 @@
>>   #include "config.h"
>>   #include "hypercall.h"
>>   #include "ctype.h"
>> +#include "errno.h"
>>   #include <stdint.h>
>>   #include <xen/xen.h>
>>   #include <xen/memory.h>
>>   #include <xen/sched.h>
>>
>> +struct xen_reserved_device_memory *rdm_map;
>> +
>>   void wrmsr(uint32_t idx, uint64_t v)
>>   {
>>       asm volatile (
>> @@ -828,6 +831,62 @@ int hpet_exists(unsigned long hpet_base)
>>       return ((hpet_id >> 16) == 0x8086);
>>   }
>>
>> +static int
>> +get_reserved_device_memory_map(struct xen_reserved_device_memory entries[],
>> +                               uint32_t *max_entries)
>> +{
>> +    int rc;
>> +    struct xen_reserved_device_memory_map xrdmmap = {
>> +        .domid = DOMID_SELF,
>> +        .nr_entries = *max_entries
>> +    };
>> +
>> +    set_xen_guest_handle(xrdmmap.buffer, entries);
>> +
>> +    rc = hypercall_memory_op(XENMEM_reserved_device_memory_map, &xrdmmap);
>> +    *max_entries = xrdmmap.nr_entries;
>
> Don't you want to check rc first before altering 'max_entries' ?

-    *max_entries = xrdmmap.nr_entries;
+    if ( rc == -ENOBUFS )
+        *max_entries = xrdmmap.nr_entries;

>
>> +
>> +    return rc;
>> +}
>> +
>> +/*
>> + * Getting all reserved device memory map info in case of hvmloader.
>> + * We just return zero for any failed cases, and this means we
>> + * can't further handle any reserved device memory.
>
> That does not sound like the right error value. Why not a proper
> return value? At worst you can put 'nr_entries' as an parameter
> and return the error value.

Any caller to use hvm_get_reserved_device_memory_map() don't care that 
real return value, the caller can work just with a return value as 
unsigned int. '0' means we have nothing to do, '>0' should be good to 
handle.

>
>> + */
>> +unsigned int hvm_get_reserved_device_memory_map(void)
>> +{
>> +    static unsigned int nr_entries = 0;
>> +    int rc = get_reserved_device_memory_map(rdm_map, &nr_entries);
>> +
>> +    if ( rc == -ENOBUFS )
>> +    {
>> +        rdm_map = mem_alloc(nr_entries*sizeof(struct xen_reserved_device_memory),
>
> That '*' being squashed looks wrong. Just make it bigger and don't worry about
> the 80 line.

-        rdm_map = mem_alloc(nr_entries*sizeof(struct 
xen_reserved_device_memory),
+        rdm_map = mem_alloc(nr_entries * sizeof(struct 
xen_reserved_device_memory),


Thanks
Tiejun

>
>> +                            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 0;
>> +            }
>> +        }
>> +        else
>> +        {
>> +            printf("No space to get reserved dev memory maps!\n");
>> +            return 0;
>> +        }
>> +    }
>> +    else if ( rc )
>> +    {
>> +        printf("Could not get reserved dev memory info on domain");
>> +        return 0;
>> +    }
>> +
>> +    return nr_entries;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
>> index a70e4aa..e4f1851 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -241,6 +241,8 @@ int build_e820_table(struct e820entry *e820,
>>                        unsigned int bios_image_base);
>>   void dump_e820_table(struct e820entry *e820, unsigned int nr);
>>
>> +unsigned int hvm_get_reserved_device_memory_map(void);
>> +
>>   #ifndef NDEBUG
>>   void perform_tests(void);
>>   #else
>> --
>> 1.9.1
>>
>