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

[Juergen Gross <jgross@xxxxxxxx>]

On 16/03/18 12:48, Jan Beulich wrote:
>>>> On 16.03.18 at 12:37, <roger.pau@xxxxxxxxxx> wrote:
>> On Fri, Mar 16, 2018 at 05:29:27AM -0600, Jan Beulich wrote:
>>>>>> 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.
>>
>> We have another similar field tuple already, modlist_paddr and
>> nr_modules and in that case (according to the current comments) the
>> proper way to check if there are modules is to check modlist_paddr !=
>> 0 and then get the count from nr_modules.
> 
> Well, that's the way it is now, as it's out in the wild already.
> 
>> Using this access strategy we avoid adding more comments about
>> checking nr_modules != 0 before trying to access modlist_paddr.
> 
> I don't follow this argumentation: There is an obvious implication
> that only nr_modules entries are valid to access at modlist_paddr.
> If nr_modules is zero, no entry is valid to access. Nothing to be
> said explicitly in the comment.

The 0 address is important for cases without a count field (e.g. the
rsdp_paddr field).

In case where a count is supplied I'd say a count value of 0 should be
used to indicate no entry is present. Setting the address to zero in
this case, too, should be allowed, of course.

I'd regard an address of zero and count > 0 as invalid.


Juergen

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