Re: [Xen-devel] [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.18 at 12:11, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
>> @@ -48,6 +49,15 @@
>>   * 32 +----------------+
>>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>>   * 40 +----------------+
>> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + * 48 +----------------+
>> + *    | memmap_entries | Number of entries in the memory map table. Zero
>> + *    |                | if there is no memory map being provided. Only
> 
> Again (as I've mentioned in previous reviews) the way to signal a
> non-present memory map is to set memmap_paddr to 0, not memmap_entries
> to 0. This is already covered by the comment at the top of the header,
> which states:
> 
> NOTE: nothing will be loaded at physical address 0, so a 0 value in any
> of the address fields should be treated as not present.

I still think it is legitimate to direct consumers to look at the entry
count here.

>> @@ -86,6 +135,14 @@ struct hvm_start_info {
>>      uint64_t cmdline_paddr;     /* Physical address of the command line.    
>>  */
>>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data   
>>  */
>>                                  /* structure.                               
>>  */
>> +    uint64_t memmap_paddr;  /* Physical address of an array of           */
>> +                            /* hvm_memmap_table_entry. Only present in   */
>> +                            /* version 1 and newer of the structure      */
>> +    uint32_t memmap_entries;        /* Number of entries in the memmap 
>> table.    */
>> +                            /* Only present in version 1 and newer of    */
>> +                            /* the structure. Value will be zero if      */
>> +                            /* there is no memory map being provided.    */
>> +    uint32_t reserved;              /* Must be zero for Version 1.          
>>      */
> 
> As mentioned in previous reviews: no tabs please.
> 
>>  };
>>  
>>  struct hvm_modlist_entry {
>> @@ -95,4 +152,11 @@ struct hvm_modlist_entry {
>>      uint64_t reserved;
>>  };
>>  
>> +struct hvm_memmap_table_entry {
>> +    uint64_t addr;          /* Base address of the memory region         */
>> +    uint64_t size;          /* Size of the memory region in bytes        */
>> +    uint32_t type;          /* Mapping type                              */
>> +    uint32_t reserved;              /* Must be zero for Version 1.          
>>      */
>> +};
> 
> No tabs please.

Oh, I didn't even notice these - my ack is clearly dependent on them
gone.

Jan


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