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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Mar 16, 2018 at 05:48:09AM -0600, 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.

Right, I don't think there should be any mention about how
memmap_paddr should be accessed, the current comment is enough IMHO,
and adding the "Zero if there is no memory map being provided" and
"... so guests that understand version 1 of the structure must check
that memmap_entries is non-zero before trying to read the memory map."
is just redundant and should be removed.

Roger.

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