Re: [Xen-devel] [PATCH] xen: Improvements to domain_crash()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 8/30/18 6:31 PM, Andrew Cooper wrote:
> There original reason for this patch was to fix a livepatching problem;
> unnecesserily large livepatchs due to the use of __LINE__.
> 
> A second problem is one of debugability.  A number of domain_crash()
> invocations have no logging at all, and number of others only have logging
> when compiled with a debug hypervisor.
> 
> Change the interface to require the caller to pass a printk() message, which
> is emitted at guest error level.  This should ensure that every time a domain
> is crashed, an informative log message is also present.
> 
> Update all callers to either merge with a previous printk(), or invent an
> informative log message.  A few select callers are switched to the
> non-printing version, when they've already emitted a relevent state dump.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Brian Woods <brian.woods@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> CC: Dario Faggioli <dfaggioli@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> 
> It is unfortunate that this is one monolithic patch, but I can't find any
> reasonable way to split it up.
> ---
>  xen/arch/arm/mem_access.c               | 12 ++----
>  xen/arch/arm/traps.c                    |  6 +--
>  xen/arch/x86/cpu/mcheck/mcaction.c      |  2 +-
>  xen/arch/x86/domain.c                   | 13 ++----
>  xen/arch/x86/hvm/emulate.c              |  9 ++--
>  xen/arch/x86/hvm/hvm.c                  | 74 
> ++++++++++++++++-----------------
>  xen/arch/x86/hvm/intercept.c            | 25 +++++++----
>  xen/arch/x86/hvm/io.c                   |  3 +-
>  xen/arch/x86/hvm/ioreq.c                | 19 +++++----
>  xen/arch/x86/hvm/svm/svm.c              | 53 ++++++++++-------------
>  xen/arch/x86/hvm/viridian.c             |  2 +-
>  xen/arch/x86/hvm/vlapic.c               |  5 +--
>  xen/arch/x86/hvm/vmx/realmode.c         |  2 +-
>  xen/arch/x86/hvm/vmx/vmcs.c             |  2 +-
>  xen/arch/x86/hvm/vmx/vmx.c              | 55 ++++++++++--------------
>  xen/arch/x86/hvm/vmx/vvmx.c             |  4 +-
>  xen/arch/x86/hvm/vpt.c                  | 10 ++---
>  xen/arch/x86/mm.c                       |  9 ++--
>  xen/arch/x86/mm/hap/hap.c               |  7 +---
>  xen/arch/x86/mm/hap/nested_hap.c        |  9 ++--
>  xen/arch/x86/mm/mem_access.c            |  5 +--
>  xen/arch/x86/mm/p2m-pod.c               | 19 ++++-----
>  xen/arch/x86/mm/p2m.c                   | 35 ++++++----------
>  xen/arch/x86/mm/shadow/common.c         | 42 +++++++------------
>  xen/arch/x86/mm/shadow/multi.c          | 17 ++++----
>  xen/arch/x86/msi.c                      |  2 +-
>  xen/arch/x86/pv/iret.c                  | 30 ++++++-------
>  xen/arch/x86/traps.c                    |  8 ++--
>  xen/arch/x86/x86_emulate/x86_emulate.c  |  2 +-
>  xen/arch/x86/xstate.c                   | 14 +++----
>  xen/common/compat/grant_table.c         |  2 +-
>  xen/common/compat/memory.c              |  6 +--
>  xen/common/domain.c                     |  2 +-
>  xen/common/grant_table.c                | 17 +++-----
>  xen/common/memory.c                     |  2 +-
>  xen/common/page_alloc.c                 |  2 +-
>  xen/common/wait.c                       | 12 ++----
>  xen/drivers/passthrough/amd/iommu_map.c | 25 +++++------
>  xen/drivers/passthrough/iommu.c         |  8 ++--
>  xen/drivers/passthrough/pci.c           |  2 +-
>  xen/drivers/passthrough/vtd/iommu.c     |  2 +-
>  xen/include/xen/sched.h                 | 10 +++--
>  42 files changed, 253 insertions(+), 332 deletions(-)
> 
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index ba4ec78..be99fbd 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -293,12 +293,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, 
> const struct npfec npfec)
>      {
>          /* No listener */
>          if ( p2m->access_required )
> -        {
> -            gdprintk(XENLOG_INFO, "Memory access permissions failure, "
> -                                  "no vm_event listener VCPU %d, dom %d\n",
> -                                  v->vcpu_id, v->domain->domain_id);
> -            domain_crash(v->domain);
> -        }
> +            domain_crash(v->domain, "No vm_event listener\n");
>          else
>          {
>              /* n2rwx was already handled */
> @@ -337,8 +332,9 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const 
> struct npfec npfec)
>          req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
>          req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
>  
> -        if ( monitor_traps(v, (xma != XENMEM_access_n2rwx), req) < 0 )
> -            domain_crash(v->domain);
> +        rc = monitor_traps(v, (xma != XENMEM_access_n2rwx), req);
> +        if ( rc < 0 )
> +            domain_crash(v->domain, "monitor_traps() returned %d\n", rc);
It looks like that rc variable is unnecessary in p2m_mem_access_check().
The code before your patch only actually checks it once:

239     rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma);
240     if ( rc )
241         return true;

and then that could be condensed to:

if ( p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma) )
    return true;

But then it's not reasonable to ask for that change as part of this
patch, so:

Acked-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>


Thanks,
Razvan

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