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

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

On Thu, Oct 05, 2017 at 06:23:41PM +0000, Andrew Cooper wrote:
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index ce3c22e..a84a95e 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -257,24 +257,24 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, domid_t domid)
>  }
>  
>  int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
> -                       xen_pfn_t console_gmfn,
> -                       xen_pfn_t xenstore_gmfn,
> +                       xen_pfn_t console_gfn,
> +                       xen_pfn_t xenstore_gfn,
>                         domid_t console_domid,
>                         domid_t xenstore_domid)
>  {
>  
> -    xen_pfn_t gnttab_gmfn;
> +    xen_pfn_t gnttab_gfn;
>      grant_entry_v1_t *gnttab;
>  
> -    gnttab_gmfn = xc_dom_gnttab_setup(xch, domid);
> -    if ( gnttab_gmfn == -1 )
> +    gnttab_gfn = xc_dom_gnttab_setup(xch, domid);
> +    if ( gnttab_gfn == -1 )
>          return -1;
>  
>      gnttab = xc_map_foreign_range(xch,
>                                    domid,
>                                    PAGE_SIZE,
>                                    PROT_READ|PROT_WRITE,
> -                                  gnttab_gmfn);
> +                                  gnttab_gfn);
>      if ( gnttab == NULL )
>      {
>          xc_dom_panic(xch, XC_INTERNAL_ERROR,
> @@ -284,17 +284,17 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
>          return -1;
>      }
>  
> -    if ( domid != console_domid  && console_gmfn != -1)
> +    if ( domid != console_domid  && console_gfn != -1 )
                                  ^ extra space
>      {
>          gnttab[GNTTAB_RESERVED_CONSOLE].flags = GTF_permit_access;
>          gnttab[GNTTAB_RESERVED_CONSOLE].domid = console_domid;
> -        gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gmfn;
> +        gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gfn;
>      }
> -    if ( domid != xenstore_domid && xenstore_gmfn != -1)
> +    if ( domid != xenstore_domid && xenstore_gfn != -1 )
>      {
>          gnttab[GNTTAB_RESERVED_XENSTORE].flags = GTF_permit_access;
>          gnttab[GNTTAB_RESERVED_XENSTORE].domid = xenstore_domid;
> -        gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gmfn;
> +        gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gfn;
>      }
>  
>      if ( munmap(gnttab, PAGE_SIZE) == -1 )
> @@ -308,19 +308,19 @@ int xc_dom_gnttab_seed(xc_interface *xch, domid_t domid,
>  
>      /* Guest shouldn't really touch its grant table until it has
>       * enabled its caches. But lets be nice. */
> -    xc_domain_cacheflush(xch, domid, gnttab_gmfn, 1);
> +    xc_domain_cacheflush(xch, domid, gnttab_gfn, 1);
>  
>      return 0;
>  }
>  
>  int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
> -                           xen_pfn_t console_gpfn,
> -                           xen_pfn_t xenstore_gpfn,
> +                           xen_pfn_t console_gfn,
> +                           xen_pfn_t xenstore_gfn,
>                             domid_t console_domid,
>                             domid_t xenstore_domid)
>  {
>      int rc;
> -    xen_pfn_t scratch_gpfn;
> +    xen_pfn_t scratch_gfn;
>      struct xen_add_to_physmap xatp = {
>          .domid = domid,
>          .space = XENMAPSPACE_grant_table,
> @@ -330,7 +330,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>          .domid = domid,
>      };
>  
> -    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
> +    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gfn);
>      if ( rc < 0 )
>      {
>          xc_dom_panic(xch, XC_INTERNAL_ERROR,
> @@ -339,11 +339,11 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, domid_t domid,
>                       __FUNCTION__, errno);
>          return -1;
>      }
> -    xatp.gpfn = scratch_gpfn;
> -    xrfp.gpfn = scratch_gpfn;
> +    xatp.gpfn = scratch_gfn;
> +    xrfp.gpfn = scratch_gfn;

xatp.gpfn = xrfp.gpfn = scratch_gfn;

Maybe, not important IMHO.

>  
> -    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.

>      }
>  }
> 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.

> 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.

Thanks, Roger.

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