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.coop...@citrix.com>
>> ---
>> CC: George Dunlap <george.dun...@eu.citrix.com>
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Tim Deegan <t...@xen.org>
>> CC: Wei Liu <wei.l...@citrix.com>
>> CC: Julien Grall <julien.gr...@arm.com>
>> CC: Jun Nakajima <jun.nakaj...@intel.com>
>> CC: Kevin Tian <kevin.t...@intel.com>
>> CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
>> CC: Brian Woods <brian.wo...@amd.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Juergen Gross <jgr...@suse.com>
>> CC: Dario Faggioli <dfaggi...@suse.com>
>> CC: Paul Durrant <paul.durr...@citrix.com>
>> CC: Razvan Cojocaru <rcojoc...@bitdefender.com>
>> CC: Tamas K Lengyel <ta...@tklengyel.com>
>>
>> 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 <rcojoc...@bitdefender.com>
Thanks.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
