Re: [Xen-devel] [PATCH 3/4] x86: Add map_domain_pages_global

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.09.18 at 17:01, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> Create a single mapping for multiple domain pages.

At least as far as map_domain_pages_global() goes, you will want to justify
why what we have isn't enough.

> --- a/tools/libxc/xc_vm_event.c
> +++ b/tools/libxc/xc_vm_event.c
> @@ -74,7 +74,7 @@ static int xc_vm_event_domctl(int type, unsigned int *param)
>      {
>      case XEN_VM_EVENT_TYPE_PAGING:
>          *param = XEN_DOMCTL_VM_EVENT_OP_PAGING;
> -     break;
> +        break;
>  
>      case XEN_VM_EVENT_TYPE_MONITOR:
>          *param = XEN_DOMCTL_VM_EVENT_OP_MONITOR;

Clearly not something you want to do in an otherwise hypervisor-only
patch.

> +void *__map_domain_pages_global(const struct page_info *pg, unsigned int nr)

I we really think we need this function, no new name space violations
please (here: no undue leading underscores).

> +{
> +    mfn_t mfn[nr];

I think we'd like to avoid any (new) VLAs on the stack.

> +    int i;

unsigned int

> @@ -68,6 +71,12 @@ static inline void *__map_domain_page_global(const struct 
> page_info *pg)
>      return page_to_virt(pg);
>  }
>  
> +static inline void *__map_domain_pages_global(const struct page_info *pg,
> +                                              unsigned int nr)
> +{
> +    return __map_domain_page_global(pg);
> +}

Even if functionally this might be correct, you shouldn't do so, as you
drop nr without any explanation.

Overall, considering the other comment you've got on the last patch,
assuming contiguous physical pages here is likely the wrong route
anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel