Re: [Xen-devel] [PATCH v4 28/31] libxc/xen: introduce HVM_PARAM_CMDLINE_PFN

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/08/15 03:01, Roger Pau Monnà wrote:
> El 07/08/15 a les 19.30, Andrew Cooper ha escrit:
> > On 07/08/15 11:18, Roger Pau Monne wrote:
> > > This HVM parameter returns a PFN that contains the address of the memory
> > > page where the guest command line has been placed.
> > >
> > > Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > ---
> > >   tools/libxc/xc_dom_x86.c        | 17 +++++++++++++++++
> > >   xen/arch/x86/hvm/hvm.c          |  2 ++
> > >   xen/include/public/hvm/params.h |  5 ++++-
> > >   3 files changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> > > index 87bce6e..369745d 100644
> > > --- a/tools/libxc/xc_dom_x86.c
> > > +++ b/tools/libxc/xc_dom_x86.c
> > > @@ -562,6 +562,23 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> > >       xc_hvm_param_set(xch, domid, HVM_PARAM_SHARING_RING_PFN,
> > >                        special_pfn(SPECIALPAGE_SHARING));
> > >   
> > > +    if ( dom->cmdline )
> > > +    {
> > > +        xen_pfn_t cmdline_pfn = xc_dom_alloc_page(dom, "command line");
> > > +        char *cmdline = xc_map_foreign_range(xch, domid, PAGE_SIZE,
> > > +                                             PROT_READ | PROT_WRITE,
> > > +                                             cmdline_pfn);
> > > +        if ( cmdline == NULL ) {
> > Mix of coding styles.
> >
> > > +            DOMPRINTF("Unable to map command line page");
> > > +            goto error_out;
> > > +        }
> > > +
> > > +        strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
> > > +        cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
> > > +        munmap(cmdline, PAGE_SIZE);
> > > +        xc_hvm_param_set(xch, domid, HVM_PARAM_CMDLINE_PFN, cmdline_pfn);
> > I am still very much on the fence about the use of HVM parameters here,
> > especially as this is one-shot information which will be freed by the
> > guest boot process.
> >
> > How much of an inconvenience would it be to specify:
> >
> > %ebx is a phyaddr of a structure looking like:
> >
> > struct dmlike_start_info {
> >      uint32_t magic;
> >      uint32_t flags; /* so as not to shoot ourselves in the foot */
> >      uint32_t cmdline_paddr;
> >      uint32_t nr_modules;
> >      uint32_t modlist_paddr;
> > };
> >
> > And a
> >
> > struct dmlite_modlist_entry {
> >      uint64_t paddr;
> >      uint64_t size;
> > };
> >
> > This avoids enforcing page alignment on the two bits of information.  I
> > don't forsee the structures needing to change in the future, and in the
> > typical case can fit in a few hundred bytes rather than two pages.
> I agree, using two pages was overkill for this, specially taking into
> account that the command line cannot exceed 1024 chars.
This is a silly restriction, and should not be propagated.  Here, it 
should simply be specified as a NUL-terminated octet stream.  Linux in 
perfectly capable of dealing with a longer command line.
> The above structure looks fine, I would note that the command line is
> limited to 1024 characters, so that leaves us with a maximum of 190
> modules if we want to limit all this metainfo to one page only, which
> IMHO is more than enough.
Just because the information would typically be less than 1 page doesn't 
impose any form of 4K upper limit.
~Andrew

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