Re: [PATCH 06/18] lib{xc, xl}: Seed grant tables with xenstore and console grants

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 01/18/2012 06:05 AM, Ian Campbell wrote:
> On Thu, 2012-01-12 at 23:35 +0000, Daniel De Graaf wrote:
>> @@ -275,6 +276,169 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>>      return rc;
>>  }
>>
>> +static unsigned long xc_dom_gnttab_setup(xc_interface *xch, uint32_t domid)
>> +{
>> +    DECLARE_HYPERCALL;
>> +    gnttab_setup_table_t setup_table;
>> +    DECLARE_HYPERCALL_BUFFER(unsigned long, gmfnp);
>> +    int rc;
>> +       unsigned long gmfn;
> 
> There's a bunch of weird alignment issues in this patch. Presumably some
> hard tabs have snuck in.
> 
>> +
>> +    gmfnp = xc_hypercall_buffer_alloc(xch, gmfnp, sizeof(*gmfnp));
>> +       if (gmfnp == NULL)
>> +               return -1;
> 
> Alignment.
> 
>> +
>> +    setup_table.dom = domid;
>> +    setup_table.nr_frames = 1;
>> +    set_xen_guest_handle(setup_table.frame_list, gmfnp);
>> +    setup_table.status = 0;
>> +
>> +    hypercall.op = __HYPERVISOR_grant_table_op;
>> +    hypercall.arg[0] = GNTTABOP_setup_table;
>> +    hypercall.arg[1] = (unsigned long) &setup_table;
>> +    hypercall.arg[2] = 1;
>> +
>> +    rc = do_xen_hypercall(xch, &hypercall);
>> +       gmfn = *gmfnp;
> 
> Alignment. I'm going to stop pointing these out, I guess grep will find
> them...
> 
> [...]
>> +int xc_dom_gnttab_init(struct xc_dom_image *dom)
>> +{
>> +    unsigned long console_gmfn;
>> +    unsigned long xenstore_gmfn;
>> +    int autotranslated;
>> +
>> +    autotranslated = xc_dom_feature_translated(dom);
>> +    console_gmfn = autotranslated ?
>> +           dom->console_pfn : xc_dom_p2m_host(dom, dom->console_pfn);
>> +    xenstore_gmfn = autotranslated ?
>> +           dom->xenstore_pfn : xc_dom_p2m_host(dom, dom->xenstore_pfn);
>> +
>> +    return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
>> +                              console_gmfn, xenstore_gmfn,
>> +                              dom->console_domid, dom->xenstore_domid);
> 
> So on build we have can do the p2m lookup and hence use the PV version
> of gnttab_seed, whereas on restore we don't have a p2m and hence can't?
> 
> The internals of xc_domain_restore do have the p2m though, so in
> principal we could avoid the need for the hvm version and the
> reintroduction of the remove_from_physmap by making xc_domain_restore
> output the necessary mfns? I'm not sure it is worth it though.
> 

The reason for needing remove_from_physmap is to temporarily access the
grant table, which can be done differently in PV and is not related to
p2m translation.

> 
>> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
>> index 3fda6f8..8bee684 100644
>> --- a/tools/libxc/xc_domain_restore.c
>> +++ b/tools/libxc/xc_domain_restore.c
>> @@ -1259,7 +1259,8 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>>
>>  int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>>                        unsigned int store_evtchn, unsigned long *store_mfn,
>> -                      unsigned int console_evtchn, unsigned long *console_mfn,
>> +                      uint32_t store_domid, unsigned int console_evtchn,
>> +                      unsigned long *console_mfn, uint32_t console_domid,
>>                        unsigned int hvm, unsigned int pae, int superpages,
>>                        int no_incr_generationid,
>>                        unsigned long *vm_generationid_addr)
>> @@ -2018,6 +2019,14 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>>          memcpy(ctx->live_p2m, ctx->p2m, dinfo->p2m_size * sizeof(xen_pfn_t));
>>      munmap(ctx->live_p2m, P2M_FL_ENTRIES * PAGE_SIZE);
>>
>> +    rc = xc_dom_gnttab_seed(xch, dom, *console_mfn, *store_mfn,
>> +                            console_domid, store_domid);
>> +    if (rc != 0)
>> +    {
>> +        ERROR("error seeding grant table");
>> +        goto out;
>> +    }
>> +
>>      DPRINTF("Domain ready to be built.\n");
>>      rc = 0;
>>      goto out;
>> @@ -2076,6 +2085,14 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>>          goto out;
>>      }
>>
>> +    rc = xc_dom_gnttab_hvm_seed(xch, dom, *console_mfn, *store_mfn,
>> +                                console_domid, store_domid);
> 
> Oh, we don't even need to return them from xc_domain_restore since we
> could just translate from gfn to mfn right here (since we have a p2m in
> hand) and call xc_dom_gnttab_seed. Or am I missing something?

For HVM, we still have to use guest frame numbers because that's what Xen
expects to find in the grant table.

Or are you talking about the fact xc_domain_restore returns the mfns? That
isn't added by this patch; they are just used to populate the grant table.
 
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index 5e39595..04db22f 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
> [...]
>> @@ -305,6 +312,8 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
>>      xc_set_hvm_param(handle, domid, HVM_PARAM_NESTEDHVM, info->u.hvm.nested_hvm);
>>      xc_set_hvm_param(handle, domid, HVM_PARAM_STORE_EVTCHN, store_evtchn);
>>      xc_set_hvm_param(handle, domid, HVM_PARAM_CONSOLE_EVTCHN, console_evtchn);
>> +
>> +    xc_dom_gnttab_hvm_seed(handle, domid, *console_mfn, *store_mfn, console_domid, store_domid);
> 
> Didn't this already happen in xc_linux_build_internal which was called
> via xc_linux_build_mem?

No, xl doesn't call xc_linux_build_mem; that's only used by xm.

-- 
Daniel De Graaf
National Security Agency