Re: [Xen-devel] [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map

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

On 2014/9/2 16:34, Jan Beulich wrote:
> > > > On 26.08.14 at 13:02, <tiejun.chen@xxxxxxxxx> wrote:
> > libxc can expose how many reserved device memory entries
> > hvmloader should get. And '0' means that doesn't exist so
> > we can skip this check.
> I assume you introduce this without consumer to limit patch size. In
> such a case title _and_ description should be more meaningful as
> to what this really does and what it's intended use is.
This patch involves two files, one is xen file, 
xen/include/public/hvm/hvm_info_table.h, and a tools file, 
tools/libxc/xc_hvm_build_x86.c.
So I'm considering to split with two small patches like this:

#1 Introduce this new field in xen/include/public/hvm/hvm_info_table.h

xen/hvm_info_table: introduce a new field nr_device_reserved_memory_map

In hvm_info_table this field represents the number of all device memory 
maps. It will be convenient to expose such a information to VM.
#2 Construct this field in tools/libxc/xc_hvm_build_x86.c

tools/libxc: construct nr_device_reserved_memory_map

While building hvm info, libxc is responsible for constructing
this number after check_drm_overlap().

Is it reasonable to use different patches covering xen internal and 
tools, respectively?
Or just one is already fine?

> > @@ -370,6 +375,9 @@ static int setup_guest(xc_interface *xch,
> >       rc = check_rmrr_overlap(xch, mmio_start, mmio_start);
> I skipped several tools side patches, but here I see that somewhere
> you still left the term "RMRR" in the code, when you were asked
> before to use more abstract naming (and this of course not only
> extended to the public interface).
I will replace those info with 'device reserved memory maps' and s/RMRR/DRM.

> >       if ( rc < 0 )
> >           goto error_out;
> > +    /* Always return entries number. */
> > +    else
> The "else" here is bogus considering the "goto" above.
Remove this 'else'.

> > --- a/xen/include/public/hvm/hvm_info_table.h
> > +++ b/xen/include/public/hvm/hvm_info_table.h
> > @@ -67,6 +67,9 @@ struct hvm_info_table {
> >
> >       /* Bitmap of which CPUs are online at boot time. */
> >       uint8_t     vcpu_online[(HVM_MAX_VCPUS + 7)/8];
> > +
> > +    /* How many reserved device memory does this domain have? */
> > +    uint32_t    nr_reserved_device_memory_map;
> >   };
> This being defacto a private interface between tools and hvmloader
> I wonder if it wouldn't be better to put this before the (in the future)
> eventually growing vcpu_online[].
Any latest consideration? Could we push line above 'uint8_t 
vcpu_online[(HVM_MAX_VCPUS + 7)/8];'?
And I just think it may be reasonable and convenient to expose this info 
in hvm_info_table since this is a fixed value that we should record for 
all VMs.

Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel