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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> +    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 more that dom_io
pages can only be mapped by privileged guests (and hence I
assume you need another tweak somewhere this way).

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

> --- a/xen/drivers/char/xen_pv_console.c
> +++ b/xen/drivers/char/xen_pv_console.c
> @@ -35,6 +35,8 @@ static evtchn_port_t cons_evtchn;
>  static serial_rx_fn cons_rx_handler;
>  static DEFINE_SPINLOCK(tx_lock);
>  
> +bool pv_console;

__read_mostly?

Jan

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