Re: [PATCH v4 25/31] xen: allow HVM guests to use XENMEM_memory_map

["Roger Pau Monné" <roger.pau@citrix.com>]

El 07/08/15 a les 17.55, Wei Liu ha escrit:
> On Fri, Aug 07, 2015 at 05:44:22PM +0200, Roger Pau Monné wrote:
>> El 07/08/15 a les 14.22, Wei Liu ha escrit:
>>> On Fri, Aug 07, 2015 at 12:18:02PM +0200, Roger Pau Monne wrote:
>>>> Enable this hypercall for HVM guests in order to fetch the e820 memory
>>>> map in the absence of an emulated BIOS. The memory map is populated and
>>>> notified to Xen in arch_setup_meminit_hvm.
>>>>
>>>> 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 | 29 ++++++++++++++++++++++++++++-
>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>>>> index b587b12..87bce6e 100644
>>>> --- a/tools/libxc/xc_dom_x86.c
>>>> +++ b/tools/libxc/xc_dom_x86.c
>>>> @@ -1205,6 +1205,7 @@ static int check_mmio_hole(uint64_t start, uint64_t memsize,
>>>>          return 1;
>>>>  }
>>>>  
>>>> +#define MAX_E820_ENTRIES    128
>>>>  static int meminit_hvm(struct xc_dom_image *dom)
>>>>  {
>>>>      unsigned long i, vmemid, nr_pages = dom->total_pages;
>>>> @@ -1225,6 +1226,8 @@ static int meminit_hvm(struct xc_dom_image *dom)
>>>>      unsigned int nr_vmemranges, nr_vnodes;
>>>>      xc_interface *xch = dom->xch;
>>>>      uint32_t domid = dom->guest_domid;
>>>> +    struct e820entry entries[MAX_E820_ENTRIES];
>>>> +    int e820_index = 0;
>>>>  
>>>>      if ( nr_pages > target_pages )
>>>>          memflags |= XENMEMF_populate_on_demand;
>>>> @@ -1275,6 +1278,13 @@ static int meminit_hvm(struct xc_dom_image *dom)
>>>>          vnode_to_pnode = dom->vnode_to_pnode;
>>>>      }
>>>>  
>>>> +    /* Add one additional memory range to account for the VGA hole */
>>>> +    if ( (nr_vmemranges + (dom->emulation ? 1 : 0)) > MAX_E820_ENTRIES )
>>>> +    {
>>>> +        DOMPRINTF("Too many memory ranges");
>>>> +        goto error_out;
>>>> +    }
>>>> +
>>>>      total_pages = 0;
>>>>      p2m_size = 0;
>>>>      for ( i = 0; i < nr_vmemranges; i++ )
>>>> @@ -1363,9 +1373,13 @@ static int meminit_hvm(struct xc_dom_image *dom)
>>>>       * Under 2MB mode, we allocate pages in batches of no more than 8MB to 
>>>>       * ensure that we can be preempted and hence dom0 remains responsive.
>>>>       */
>>>> -    if ( dom->emulation )
>>>> +    if ( dom->emulation ) {
>>>>          rc = xc_domain_populate_physmap_exact(
>>>>              xch, domid, 0xa0, 0, memflags, &dom->p2m_host[0x00]);
>>>> +        entries[e820_index].addr = 0;
>>>> +        entries[e820_index].size = 0xa0 << PAGE_SHIFT;
>>>> +        entries[e820_index++].type = E820_RAM;
>>>> +    }
>>>>  
>>>>      stat_normal_pages = 0;
>>>>      for ( vmemid = 0; vmemid < nr_vmemranges; vmemid++ )
>>>> @@ -1392,6 +1406,12 @@ static int meminit_hvm(struct xc_dom_image *dom)
>>>>          else
>>>>              cur_pages = vmemranges[vmemid].start >> PAGE_SHIFT;
>>>>  
>>>> +        /* Build an e820 map. */
>>>> +        entries[e820_index].addr = cur_pages << PAGE_SHIFT;
>>>> +        entries[e820_index].size = vmemranges[vmemid].end -
>>>> +                                   entries[e820_index].addr;
>>>> +        entries[e820_index++].type = E820_RAM;
>>>> +
>>>>          while ( (rc == 0) && (end_pages > cur_pages) )
>>>>          {
>>>>              /* Clip count to maximum 1GB extent. */
>>>> @@ -1509,6 +1529,13 @@ static int meminit_hvm(struct xc_dom_image *dom)
>>>>      DPRINTF("  2MB PAGES: 0x%016lx\n", stat_2mb_pages);
>>>>      DPRINTF("  1GB PAGES: 0x%016lx\n", stat_1gb_pages);
>>>>  
>>>> +    rc = xc_domain_set_memory_map(xch, domid, entries, e820_index);
>>>> +    if ( rc != 0 )
>>>> +    {
>>>> +        DOMPRINTF("unable to set memory map.");
>>>> +        goto error_out;
>>>> +    }
>>>> +
>>>
>>> I think in RDM series there is already a memory map generated in libxl.
>>>
>>> You might want to move this to libxl, which is supposed to be the
>>> arbitrator of what a guest should look like.
>>>
>>> What do you think?
>>
>> I would like to do that, the only problem I see is that libxl doesn't
>> have any notion of the "special pages", so it doesn't know the size of
>> the hole it has to made. I guess one solution would be moving the
>> defines at the top of xc_dom_x86.c to a public libxc header so libxl can
>> pick up the size and position of the hole.
> 
> But the code above has no notion of special pages either. Can it not
> just use the E820 map generated in libxl? I think at the time you wrote
> this patch libxl doesn't generate E820 for HVM guest, but it does now,
> so maybe you can drop this patch?

Yes, by the time I did that libxl was not building any memory map for
HVM guests. This was a preparatory patch so HVMlite guests could fetch a
memory map using the hypercall interface.

I agree that now that libxl builds a memory map this is no longer
needed, I will find a way to build a correct memory map for HVMlite
guests from libxl.

> BTW the title is not very accurate because this patch doesn't involve
> allowing / disallowing hvm to use that hypercall.

>From our talk above I guess this is going to be dropped/replaced from
the series anyway, so no need to think of a better subject :).

Roger.