Re: [Xen-devel] [PATCH RFC v1 52/74] xen: mark xenstore/console pages as RAM and add them to dom_io

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 08, 2018 at 06:49:21AM -0700, Jan Beulich wrote:
> >>> On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> There being no description at all makes it rather harder to review this
> one. I assume that marking the pages as RAM is necessary to make
> sure a struct page_info is being created for them, which in turn is a
> prereq for sharing the pages.

Yes, this was sent before we had time to add proper logs to some
commits, sorry.

> > +void __init hypervisor_fixup_e820(struct e820map *e820)
> > +{
> > +    uint64_t pfn = 0;
> 
> I don't think initializers of this kind are necessary (there are several
> instances of this).

gcc complains with "variable maybe used uninitialized" if I don't add
this. I haven't looked at the root cause of this.

> > +    long rc;
> > +
> > +    if ( !xen_guest )
> > +        return;
> > +
> > +#define MARK_PARAM_RAM(p) ({                    \
> > +    rc = xen_hypercall_hvm_get_param(p, &pfn);  \
> > +    if ( rc )                                   \
> > +        panic("Unable to get " #p);             \
> 
> The text here is the same in all three instances - please make it
> distinguishable, so one doesn't have to start guessing.
> 
> > +void __init hypervisor_init_memory(void)
> > +{
> > +    uint64_t pfn = 0;
> > +    long rc;
> > +
> > +    if ( !xen_guest )
> > +        return;
> > +
> > +#define SHARE_PARAM(p) ({                                                  
> >  \
> > +    rc = xen_hypercall_hvm_get_param(p, &pfn);                             
> > \
> > +    if ( rc )                                                              
> >  \
> > +        panic("Unable to get " #p);                                        
> >  \
> > +    share_xen_page_with_guest(mfn_to_page(pfn), dom_io, 
> > XENSHARE_writable); \
> 
> Why dom_io rather than the client domain?

The client domain is not yet created at this point. This is exactly
the same that Xen does for the low 1MiB for example.

> The more that dom_io
> pages can only be mapped by privileged guests (and hence I
> assume you need another tweak somewhere this way).

I just use unshare_xen_page and share it again with the guest.

> > +const unsigned long *__init hypervisor_reserved_pages(unsigned int *size)
> > +{
> > +    static unsigned long __initdata reserved_pages[2];
> > +    uint64_t pfn = 0;
> > +    long rc;
> > +
> > +    if ( !xen_guest )
> > +        return NULL;
> > +
> > +    *size = 0;
> > +
> > +#define RESERVE_PARAM(p) ({                             \
> > +    rc = xen_hypercall_hvm_get_param(p, &pfn);          \
> > +    if ( rc )                                           \
> > +        panic("Unable to get " #p);                     \
> > +    reserved_pages[(*size)++] = pfn << PAGE_SHIFT;      \
> > +})
> > +    RESERVE_PARAM(HVM_PARAM_STORE_PFN);
> > +    if ( !pv_console )
> > +        RESERVE_PARAM(HVM_PARAM_CONSOLE_PFN);
> > +#undef RESERVE_PARAM
> > +
> > +    return reserved_pages;
> > +}
> 
> Afaict this happens much later than hypervisor_fixup_e820() -
> can't you latch the PFNs into a file scope array there, and merely
> return the information here, rather than re-invoking the
> hypercalls? This would save at least one instance of the wrapper
> macros.

Right, this seems better.

Thanks, Roger.

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