Re: [PATCH for-4.10 3/5] tools/dombuilder: Switch to using gfn terminology for console and xenstore rings

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

On 06/10/17 10:57, Roger Pau Monné wrote:
> On Thu, Oct 05, 2017 at 06:23:41PM +0000, Andrew Cooper wrote:
>>  
>> -    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
>> -                  scratch_gpfn);
>> +    xc_dom_printf(xch, "%s: called, scratch gfn=0x%"PRI_xen_pfn, __FUNCTION__,
>> +                  scratch_gfn);
>>  
>>  
>>      rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
>> @@ -357,7 +357,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>>      }
>>  
>>      rc = xc_dom_gnttab_seed(xch, domid,
>> -                            console_gpfn, xenstore_gpfn,
>> +                            console_gfn, xenstore_gfn,
>>                              console_domid, xenstore_domid);
>>      if (rc != 0)
>>      {
>> @@ -385,12 +385,11 @@ int xc_dom_gnttab_init(struct xc_dom_image *dom)
>>  {
>>      if ( xc_dom_translated(dom) ) {
>>          return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
>> -                                      dom->console_pfn, dom->xenstore_pfn,
>> +                                      dom->console_gfn, dom->xenstore_gfn,
>>                                        dom->console_domid, dom->xenstore_domid);
>>      } else {
>>          return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
>> -                                  xc_dom_p2m(dom, dom->console_pfn),
>> -                                  xc_dom_p2m(dom, dom->xenstore_pfn),
>> +                                  dom->console_gfn, dom->xenstore_gfn,
>>                                    dom->console_domid, dom->xenstore_domid);
> return xc_dom_translated(dom) ? xc_dom_gnttab_hvm_seed : xc_dom_gnttab_seed
>                                 (dom->xch, dom->guest_domid, dom->console_gfn,
>                                  dom->xenstore_gfn, dom->console_domid,
>                                  dom->xenstore_domid);
>
> Not sure about the best indentation here. Or that could even be hidden
> inside of xc_dom_gnttab_seed, so that xc_dom_gnttab_hvm_seed can be
> removed.

These seed functions are also needed in isolation from the migration
code, so I can't make them local.  I considered what you suggest here,
but I'm not sure that it helps the readability.

>
>>      }
>>  }
>> diff --git a/tools/libxc/xc_dom_compat_linux.c b/tools/libxc/xc_dom_compat_linux.c
>> index c922c61..6d27ec2 100644
>> --- a/tools/libxc/xc_dom_compat_linux.c
>> +++ b/tools/libxc/xc_dom_compat_linux.c
>> @@ -78,8 +78,8 @@ int xc_linux_build(xc_interface *xch, uint32_t domid,
>>      if ( (rc = xc_dom_gnttab_init(dom)) != 0)
>>          goto out;
>>  
>> -    *console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>> -    *store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
>> +    *console_mfn = dom->console_gfn;
>> +    *store_mfn = dom->xenstore_gfn;
>>  
>>   out:
>>      xc_dom_release(dom);
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 0c80b59..aa0ced1 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -536,21 +536,23 @@ static int alloc_p2m_list_x86_64(struct xc_dom_image *dom)
>>  
>>  static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>>  {
>> +    xen_pfn_t pfn;
>> +
>>      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
>>      if ( dom->start_info_pfn == INVALID_PFN )
>>          return -1;
>>  
>> -    dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
>> -    if ( dom->xenstore_pfn == INVALID_PFN )
>> +    pfn = xc_dom_alloc_page(dom, "xenstore");
>> +    if ( pfn == INVALID_PFN )
>>          return -1;
>> -    xc_clear_domain_page(dom->xch, dom->guest_domid,
>> -                         xc_dom_p2m(dom, dom->xenstore_pfn));
>> +    dom->xenstore_gfn = xc_dom_p2m(dom, pfn);
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_gfn);
>>  
>> -    dom->console_pfn = xc_dom_alloc_page(dom, "console");
>> -    if ( dom->console_pfn == INVALID_PFN )
>> +    pfn = xc_dom_alloc_page(dom, "console");
>> +    if ( pfn == INVALID_PFN )
>>          return -1;
>> -    xc_clear_domain_page(dom->xch, dom->guest_domid,
>> -                         xc_dom_p2m(dom, dom->console_pfn));
>> +    dom->console_gfn = xc_dom_p2m(dom, pfn);
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_gfn);
>>  
>>      dom->alloc_bootstack = 1;
>>  
>> @@ -612,14 +614,19 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>                                 X86_HVM_NR_SPECIAL_PAGES) )
>>              goto error_out;
>>  
>> -    xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN,
>> -                     special_pfn(SPECIALPAGE_XENSTORE));
>> +    dom->xenstore_gfn = special_pfn(SPECIALPAGE_XENSTORE);
> A pre-patch to s/special_pfn/special_gfn/ would be nice :) for
> coherency.

Sorting out all terminology is a far larger problem than I have time for
atm.  For HVM guests, pfn == gfn, so I chose not to dive down that
rabbit hole right now.

>
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index ef834e6..0389a06 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -851,14 +851,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>      if (ret != 0)
>>          goto out;
>>  
>> -    if (xc_dom_translated(dom)) {
>> -        state->console_mfn = dom->console_pfn;
>> -        state->store_mfn = dom->xenstore_pfn;
>> -        state->vuart_gfn = dom->vuart_gfn;
> This chunk should go with patch 1, it's a PVHv1 leftover also.

Sadly, no its not :(

ARM uses libxl__build_pv(), not libxl__build_hvm()

~Andrew

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