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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@citrix.com>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>>   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@lists.xen.org
http://lists.xen.org/xen-devel