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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/08/18 17:01, Razvan Cojocaru wrote:
> 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:

Hmm - the ARM code assigned to it 3 times after that point.  Smells to
me like there should be some more error checking :)

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

~Andrew

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