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 10:00:54AM -0700, Maran Wilson wrote:
> On 3/16/2018 4:11 AM, Roger Pau Monné wrote:
> > On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
> > >    *  8 +----------------+
> > >    *    | flags          | SIF_xxx flags.
> > >    * 12 +----------------+
> > > @@ -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'm not really following the argument for why checking for PA != 0 is a
> better approach. The way I see it, we have to check #entries anyway so the
> consumer side code is more efficient if we just check #entries and assume
> the PA is valid when #entries != 0 (seems like a reasonable demand for the
> producer side). If we define it to be "PA of zero means no valid entries",
> then the consumer side code has to make the additional check of PA != 0 in
> addition to making sure there are greater than 0 entries. It's not a huge
> deal, but then again, I'm not seeing a huge win by going the other way
> either.
> 
> The fact that a previous comment already exists regarding PA of 0 is
> *slightly* awkward (redundant) depending on how you look at it. But it's not
> blatantly contradictory (in most cases, I'm sure PA and #entries will both
> be zero when no memory map is being provided) and it probably serves a
> purpose for the rsdp_paddr field.
> 
> So after carefully reading everyone's input on this thread, my preference as
> the person coding this up is to stick with the #entries check and leave the
> other existing comments in place to cover the existing fields and code that
> is already out there.
> 
> But if it's really a deal breaker for someone or if broad consensus is that
> it is better to just make the change so that PA of 0 is the definitive way
> to check for the presence of a memory map, then I will go ahead and make the
> change.

OK, I just wanted to have consistency with the nr_modules and
modlist_paddr field tuple. The consensus seem to be that spelling out
the "Zero if there is no memory map..." condition is fine, so I'm not
going to argue over it anymore.

Roger.

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