Re: [Xen-devel] [PATCH] xen pv-on-hvm: add pfn_is_ram helper for kdump

Re: [Xen-devel] [PATCH] xen pv-on-hvm: add pfn_is_ram helper for kdump

[Konrad Rzeszutek Wilk]

On Thu, Jul 12, 2012 at 07:20:39PM +0200, Olaf Hering wrote:
> Register pfn_is_ram helper speed up reading /proc/vmcore in the kdump
> kernel. See commit message of 997c136f518c ("fs/proc/vmcore.c: add hook
> to read_from_oldmem() to check for non-ram pages") for details.
> 
> It makes use of a new hvmop HVMOP_get_mem_type which was introduced in
> xen 4.2 (23298:26413986e6e0) and backported to 4.1.1.
> 
> The new function is currently only enabled for reading /proc/vmcore.
> Later it will be used also for the kexec kernel. Since that requires
> more changes in the generic kernel make it static for the time being.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  arch/x86/xen/mmu.c                 |   41 ++++++++++++++++++++++++++++++++++++
>  include/xen/interface/hvm/hvm_op.h |   20 ++++++++++++++++++
>  2 Dateien geändert, 61 Zeilen hinzugefügt(+)
> 
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index 3a73785..54de84d 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -47,6 +47,7 @@
>  #include <linux/gfp.h>
>  #include <linux/memblock.h>
>  #include <linux/seq_file.h>
> +#include <linux/crash_dump.h>
>  
>  #include <trace/events/xen.h>
>  
> @@ -2245,6 +2246,43 @@ void xen_destroy_contiguous_region(unsigned long vstart, unsigned int order)
>  EXPORT_SYMBOL_GPL(xen_destroy_contiguous_region);
>  
>  #ifdef CONFIG_XEN_PVHVM
> +#ifdef CONFIG_PROC_VMCORE
> +/*
> + * This function is used in two contexts:
> + * - the kdump kernel has to check whether a pfn of the crashed kernel
> + *   was a ballooned page. vmcore is using this function to decide
> + *   whether to access a pfn of the crashed kernel.
> + * - the kexec kernel has to check whether a pfn was ballooned by the
> + *   previous kernel. If the pfn is ballooned, handle it properly.
> + * Returns 0 if the pfn is not backed by a RAM page, the caller may
> + * handle the pfn special in this case.
> + */
> +static int xen_oldmem_pfn_is_ram(unsigned long pfn)
> +{
> +	struct xen_hvm_get_mem_type a = {
> +		.domid = DOMID_SELF,
> +		.pfn = pfn,
> +	};
> +	int ram;
> +
> +	if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a))
> +		return -ENXIO;
> +
> +	switch (a.mem_type) {
> +		case HVMMEM_mmio_dm:
> +			ram = 0;
> +			break;
> +		case HVMMEM_ram_rw:
> +		case HVMMEM_ram_ro:
> +		default:
> +			ram = 1;
> +			break;
> +	}
> +
> +	return ram;
> +}
> +#endif
> +
>  static void xen_hvm_exit_mmap(struct mm_struct *mm)
>  {
>  	struct xen_hvm_pagetable_dying a;
> @@ -2275,6 +2313,9 @@ void __init xen_hvm_init_mmu_ops(void)
>  {
>  	if (is_pagetable_dying_supported())
>  		pv_mmu_ops.exit_mmap = xen_hvm_exit_mmap;
> +#ifdef CONFIG_PROC_VMCORE
> +	register_oldmem_pfn_is_ram(&xen_oldmem_pfn_is_ram);
> +#endif
>  }
>  #endif
>  
> diff --git a/include/xen/interface/hvm/hvm_op.h b/include/xen/interface/hvm/hvm_op.h
> index a4827f4..0816ae4 100644
> --- a/include/xen/interface/hvm/hvm_op.h
> +++ b/include/xen/interface/hvm/hvm_op.h
> @@ -43,4 +43,24 @@ struct xen_hvm_pagetable_dying {
>  typedef struct xen_hvm_pagetable_dying xen_hvm_pagetable_dying_t;
>  DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_pagetable_dying_t);
>   
> +typedef enum {
> +    HVMMEM_ram_rw,             /* Normal read/write guest RAM */
> +    HVMMEM_ram_ro,             /* Read-only; writes are discarded */
> +    HVMMEM_mmio_dm,            /* Reads and write go to the device model */
> +} hvmmem_type_t;

Does this have to be a typdef?

> +
> +#define HVMOP_get_mem_type    15
> +/* Return hvmmem_type_t for the specified pfn. */
> +struct xen_hvm_get_mem_type {
> +    /* Domain to be queried. */
> +    domid_t domid;
> +    /* OUT variable. */
> +    uint16_t mem_type;
> +    uint16_t pad[2]; /* align next field on 8-byte boundary */
> +    /* IN variable. */
> +    uint64_t pfn;
> +};
> +typedef struct xen_hvm_get_mem_type xen_hvm_get_mem_type_t;

Please no typdefs. I can fix this up, but in the future pls don't add
more of them.


> +DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_get_mem_type_t);
> +
>  #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen pv-on-hvm: add pfn_is_ram helper for kdump

[Olaf Hering]

On Thu, Jul 12, Konrad Rzeszutek Wilk wrote:

> > +++ b/include/xen/interface/hvm/hvm_op.h
> > @@ -43,4 +43,24 @@ struct xen_hvm_pagetable_dying {
> >  typedef struct xen_hvm_pagetable_dying xen_hvm_pagetable_dying_t;
> >  DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_pagetable_dying_t);
> >   
> > +typedef enum {
> > +    HVMMEM_ram_rw,             /* Normal read/write guest RAM */
> > +    HVMMEM_ram_ro,             /* Read-only; writes are discarded */
> > +    HVMMEM_mmio_dm,            /* Reads and write go to the device model */
> > +} hvmmem_type_t;
> 
> Does this have to be a typdef?
> 
> > +
> > +#define HVMOP_get_mem_type    15
> > +/* Return hvmmem_type_t for the specified pfn. */
> > +struct xen_hvm_get_mem_type {
> > +    /* Domain to be queried. */
> > +    domid_t domid;
> > +    /* OUT variable. */
> > +    uint16_t mem_type;
> > +    uint16_t pad[2]; /* align next field on 8-byte boundary */
> > +    /* IN variable. */
> > +    uint64_t pfn;
> > +};
> > +typedef struct xen_hvm_get_mem_type xen_hvm_get_mem_type_t;
> 
> Please no typdefs. I can fix this up, but in the future pls don't add
> more of them.

Its just a forward port from what went into linux-2.6.18.


Olaf

Re: [Xen-devel] [PATCH] xen pv-on-hvm: add pfn_is_ram helper for kdump

[Konrad Rzeszutek Wilk]

On Thu, Jul 12, 2012 at 11:00:05AM +0200, Olaf Hering wrote:
> Register pfn_is_ram helper speed up reading /proc/vmcore in the kdump
> kernel. See commit message of 997c136f518c ("fs/proc/vmcore.c: add hook
> to read_from_oldmem() to check for non-ram pages") for details.

What about the 'unregister_oldmem_pfn_is_ram' call? Should this
be implemented here as well?

Comments below.

> 
> The new function is currently only enabled for reading /proc/vmcore.
> Later it will be used also for the kexec kernel. Since that requires
> more changes in the generic kernel make it static.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  arch/x86/xen/mmu.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index 3a73785..ac876c2 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -47,6 +47,7 @@
>  #include <linux/gfp.h>
>  #include <linux/memblock.h>
>  #include <linux/seq_file.h>
> +#include <linux/crash_dump.h>

Should this also have an #idef CONFIG_PROC_VMCORE?
Or is it going to compile OK without CONFIG_PROC_VMCORE defined?

>  
>  #include <trace/events/xen.h>
>  
> @@ -2245,6 +2246,41 @@ void xen_destroy_contiguous_region(unsigned long vstart, unsigned int order)
>  EXPORT_SYMBOL_GPL(xen_destroy_contiguous_region);
>  
>  #ifdef CONFIG_XEN_PVHVM
> +#ifdef CONFIG_PROC_VMCORE
> +/*
> + * This function is used in two contexts:
> + * - the kdump kernel has to check wether a pfn of the crashed kernel

s/wether/whether

> + *   was a ballooned page. vmcore is using this function to decide
> + *   wether to access a pfn of the crashed kernel.

s/wether/wheter

> + * - the kexec kernel has to check wether a pfn was ballooned by the

Ditto.

> + *   previous kernel. If the pfn is ballooned, handle it properly.

. and by handle it properly you mean return -ENXIO?

> + * Returns 0 if the pfn is not backed by a RAM page.
> + */
> +static int xen_oldmem_pfn_is_ram(unsigned long pfn)
> +{
> +	struct xen_hvm_get_mem_type a;

Can you make this have already initialized values, like:

	struct xen_hvm_get_mem_type a = {
		.domid = DOMID_SELF;
		.pfn = pfn
	};

Also is this hypercall new? Or has it been in existence for some time?

> +	int ram;
> +
> +	a.domid = DOMID_SELF;
> +	a.pfn = pfn;
> +	if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a))
> +		return -ENXIO;
> +
> +	switch (a.mem_type) {
> +		case HVMMEM_mmio_dm:
> +			ram = 0;
> +			break;
> +		case HVMMEM_ram_rw:
> +		case HVMMEM_ram_ro:
> +		default:
> +			ram = 1;
> +			break;
> +	}
> +
> +	return ram;
> +}
> +#endif
> +
>  static void xen_hvm_exit_mmap(struct mm_struct *mm)
>  {
>  	struct xen_hvm_pagetable_dying a;
> @@ -2275,6 +2311,9 @@ void __init xen_hvm_init_mmu_ops(void)
>  {
>  	if (is_pagetable_dying_supported())
>  		pv_mmu_ops.exit_mmap = xen_hvm_exit_mmap;
> +#ifdef CONFIG_PROC_VMCORE
> +	register_oldmem_pfn_is_ram(&xen_oldmem_pfn_is_ram);
> +#endif
>  }
>  #endif
>  
> -- 
> 1.7.3.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen pv-on-hvm: add pfn_is_ram helper for kdump

[Olaf Hering]

On Thu, Jul 12, Konrad Rzeszutek Wilk wrote:

> On Thu, Jul 12, 2012 at 11:00:05AM +0200, Olaf Hering wrote:
> > Register pfn_is_ram helper speed up reading /proc/vmcore in the kdump
> > kernel. See commit message of 997c136f518c ("fs/proc/vmcore.c: add hook
> > to read_from_oldmem() to check for non-ram pages") for details.
> 
> What about the 'unregister_oldmem_pfn_is_ram' call? Should this
> be implemented here as well?

There is no need to unregister because PVonHVM is not going away during
the lifetime of the guest.

> > +#include <linux/crash_dump.h>
> 
> Should this also have an #idef CONFIG_PROC_VMCORE?
> Or is it going to compile OK without CONFIG_PROC_VMCORE defined?

All includes are supposed to work without ifdefs in the code, which is
the case also for crash_dump.h

> > + *   previous kernel. If the pfn is ballooned, handle it properly.
> 
> . and by handle it properly you mean return -ENXIO?

Return code 0 means its a ballooned page and needs special care,
non-null means it has to be handled as a RAM page.

If the hypervisor is too old (4.1 and earlier) it just means that in
case of kdump it will just cause high load in dom0. 
In case of kexec it will mean that the new kernel will crash because it
can not know which pfn is actually there.

> > + * Returns 0 if the pfn is not backed by a RAM page.
> > + */
> > +static int xen_oldmem_pfn_is_ram(unsigned long pfn)
> > +{
> > +	struct xen_hvm_get_mem_type a;
> 
> Can you make this have already initialized values, like:
> 
> 	struct xen_hvm_get_mem_type a = {
> 		.domid = DOMID_SELF;
> 		.pfn = pfn
> 	};

Yes.

> Also is this hypercall new? Or has it been in existence for some time?

Its new in 4.2, and was backported to 4.1.1 as well.


Olaf

Re: [Xen-devel] [PATCH] xen pv-on-hvm: add pfn_is_ram helper for kdump

[Konrad Rzeszutek Wilk]

On Thu, Jul 12, 2012 at 04:22:34PM +0200, Olaf Hering wrote:
> On Thu, Jul 12, Konrad Rzeszutek Wilk wrote:
> 
> > On Thu, Jul 12, 2012 at 11:00:05AM +0200, Olaf Hering wrote:
> > > Register pfn_is_ram helper speed up reading /proc/vmcore in the kdump
> > > kernel. See commit message of 997c136f518c ("fs/proc/vmcore.c: add hook
> > > to read_from_oldmem() to check for non-ram pages") for details.
> > 
> > What about the 'unregister_oldmem_pfn_is_ram' call? Should this
> > be implemented here as well?
> 
> There is no need to unregister because PVonHVM is not going away during
> the lifetime of the guest.
> 
> > > +#include <linux/crash_dump.h>
> > 
> > Should this also have an #idef CONFIG_PROC_VMCORE?
> > Or is it going to compile OK without CONFIG_PROC_VMCORE defined?
> 
> All includes are supposed to work without ifdefs in the code, which is
> the case also for crash_dump.h

OK.
> 
> > > + *   previous kernel. If the pfn is ballooned, handle it properly.
> > 
> > . and by handle it properly you mean return -ENXIO?
> 
> Return code 0 means its a ballooned page and needs special care,
> non-null means it has to be handled as a RAM page.
> 
> If the hypervisor is too old (4.1 and earlier) it just means that in
> case of kdump it will just cause high load in dom0. 
> In case of kexec it will mean that the new kernel will crash because it
> can not know which pfn is actually there.
> 
> > > + * Returns 0 if the pfn is not backed by a RAM page.
> > > + */
> > > +static int xen_oldmem_pfn_is_ram(unsigned long pfn)
> > > +{
> > > +	struct xen_hvm_get_mem_type a;
> > 
> > Can you make this have already initialized values, like:
> > 
> > 	struct xen_hvm_get_mem_type a = {
> > 		.domid = DOMID_SELF;
> > 		.pfn = pfn
> > 	};
> 
> Yes.
> 
> > Also is this hypercall new? Or has it been in existence for some time?
> 
> Its new in 4.2, and was backported to 4.1.1 as well.

Can you include the c/s in git commit pls?