Re: [Xen-devel] [PATCH RFC v1 53/74] xen/pvshim: modify Dom0 builder in order to build a DomU

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 
> According to the PV ABI the initial virtual memory regions should
> contain the xenstore and console pages after the start_info. Fix this
> and add the pages to the p2m/m2p after the start_info page also.

I don't think "fix" is the right term here.

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -31,9 +31,8 @@
>  #define L3_PROT (BASE_PROT|_PAGE_DIRTY)
>  #define L4_PROT (BASE_PROT|_PAGE_DIRTY)
>  
> -static __init void dom0_update_physmap(struct domain *d, unsigned long pfn,
> -                                       unsigned long mfn,
> -                                       unsigned long vphysmap_s)
> +__init void dom0_update_physmap(struct domain *d, unsigned long pfn,

Please don't re-order type and annotation.

> @@ -443,9 +446,18 @@ int __init dom0_construct_pv(struct domain *d,
>      vstartinfo_start = round_pgup(vphysmap_end);
>      vstartinfo_end   = (vstartinfo_start +
>                          sizeof(struct start_info) +
> -                        sizeof(struct dom0_vga_console_info));
> +                        (pv_shim ? 0 : sizeof(struct 
> dom0_vga_console_info)));

Why not move this addition ...

> -    vpt_start        = round_pgup(vstartinfo_end);
> +    if ( pv_shim )
> +    {
> +        vxenstore_start  = round_pgup(vstartinfo_end);
> +        vxenstore_end    = vxenstore_start + PAGE_SIZE;
> +        vconsole_start   = vxenstore_end;
> +        vconsole_end     = vconsole_start + PAGE_SIZE;
> +        vpt_start        = vconsole_end;
> +    }
> +    else
> +        vpt_start        = round_pgup(vstartinfo_end);

... into this "else" block.

> @@ -538,6 +550,8 @@ int __init dom0_construct_pv(struct domain *d,
>             " Init. ramdisk: %p->%p\n"
>             " Phys-Mach map: %p->%p\n"
>             " Start info:    %p->%p\n"
> +           " Xenstore ring: %p->%p\n"
> +           " Console ring:  %p->%p\n"
>             " Page tables:   %p->%p\n"
>             " Boot stack:    %p->%p\n"
>             " TOTAL:         %p->%p\n",
> @@ -545,6 +559,8 @@ int __init dom0_construct_pv(struct domain *d,
>             _p(vinitrd_start), _p(vinitrd_end),
>             _p(vphysmap_start), _p(vphysmap_end),
>             _p(vstartinfo_start), _p(vstartinfo_end),
> +           _p(vxenstore_start), _p(vxenstore_end),
> +           _p(vconsole_start), _p(vconsole_end),

I'm not convinced the extra verbosity is helpful - the pages are at
fixed offsets from start_info, which already is being logged.

> @@ -830,15 +847,20 @@ int __init dom0_construct_pv(struct domain *d,
>          strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
>  
>  #ifdef CONFIG_VIDEO
> -    if ( fill_console_start_info((void *)(si + 1)) )
> +    if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
>      {
>          si->console.dom0.info_off  = sizeof(struct start_info);
>          si->console.dom0.info_size = sizeof(struct dom0_vga_console_info);
>      }
>  #endif
>  
> +    if ( pv_shim )
> +        pv_shim_setup_dom(d, l4start, v_start, vxenstore_start, 
> vconsole_start,
> +                          vphysmap_start, si);

With fill_console_start_info() being given a stub in the !CONFIG_VIDEO
case (ideally right in the earlier patch), this could become

    if ( pv_shim )
        pv_shim_setup_dom(d, l4start, v_start, vxenstore_start, vconsole_start,
                          vphysmap_start, si);
    else if ( fill_console_start_info((void *)(si + 1)) )
     {
         si->console.dom0.info_off  = sizeof(struct start_info);
         si->console.dom0.info_size = sizeof(struct dom0_vga_console_info);
     }

> +static void __init replace_va(struct domain *d, l4_pgentry_t *l4start,
> +                              unsigned long va, unsigned long mfn)

I tdoesn't look like you're replacing a VA here (and really: how could
you?), so how about "replace_va_mapping()"?

> +{
> +    struct page_info *page;
> +    l4_pgentry_t *pl4e;
> +    l3_pgentry_t *pl3e;
> +    l2_pgentry_t *pl2e;
> +    l1_pgentry_t *pl1e;
> +
> +    pl4e = l4start + l4_table_offset(va);
> +    pl3e = l4e_to_l3e(*pl4e);
> +    pl3e += l3_table_offset(va);
> +    pl2e = l3e_to_l2e(*pl3e);
> +    pl2e += l2_table_offset(va);
> +    pl1e = l2e_to_l1e(*pl2e);
> +    pl1e += l1_table_offset(va);
> +
> +    page = mfn_to_page(l1e_get_pfn(*pl1e));
> +    /* Free original page, will be replaced */
> +    put_page_and_type(page);
> +    free_domheap_pages(page, 0);

This looks bogus - free_domheap_pages() should be called by
the last put_page(), not directly. If that doesn't happen, I
would guess you need the usual PGC_allocated clearing logic
here.

> +void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
> +                              unsigned long va_start, unsigned long store_va,
> +                              unsigned long console_va, unsigned long 
> vphysmap,
> +                              start_info_t *si)
> +{
> +    uint64_t param = 0;
> +    long rc;
> +
> +#define SET_AND_MAP_PARAM(p, si, va) ({                                      
>   \
> +    rc = xen_hypercall_hvm_get_param(p, &param);                             
>   \
> +    if ( rc )                                                                
>   \
> +        panic("Unable to get " #p "\n");                                     
>   \
> +    (si) = param;                                                            
>   \
> +    if ( va )                                                                
>  \
> +    {                                                                        
>   \
> +        BUG_ON(unshare_xen_page_with_guest(mfn_to_page(param), dom_io));     
>   \
> +        share_xen_page_with_guest(mfn_to_page(param), d, XENSHARE_writable); 
>   \
> +        replace_va(d, l4start, va, param);                                   
>   \
> +        dom0_update_physmap(d, (va - va_start) >> PAGE_SHIFT, param, 
> vphysmap);\

PFN_DOWN()

> +    }                                                                        
>   \
> +})
> +    SET_AND_MAP_PARAM(HVM_PARAM_STORE_PFN, si->store_mfn, store_va);
> +    SET_AND_MAP_PARAM(HVM_PARAM_STORE_EVTCHN, si->store_evtchn, 0);
> +    if ( !pv_console )
> +    {
> +        SET_AND_MAP_PARAM(HVM_PARAM_CONSOLE_PFN, si->console.domU.mfn,
> +                          console_va);
> +        SET_AND_MAP_PARAM(HVM_PARAM_CONSOLE_EVTCHN, si->console.domU.evtchn, 
> 0);
> +    }
> +#undef SET_AND_MAP_PARAM

Here, even more than earlier on, it becomes rather desirable to move
the HVM_PARAM_ prefixes into the macro. But yes, I know at least
Andrew won't like it ...

Jan

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