Re: [PATCH v1 2/8] x86/xen: simplify xen_oldmem_pfn_is_ram()
On 29.09.21 16:22, Boris Ostrovsky wrote: On 9/29/21 5:03 AM, David Hildenbrand wrote: On 29.09.21 10:45, David Hildenbrand wrote: Can we go one step further and do @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) struct xen_hvm_get_mem_type a = { .domid = DOMID_SELF, .pfn = pfn, + .mem_type = HVMMEM_ram_rw, }; - int ram; - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, )) - 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; + HYPERVISOR_hvm_op(HVMOP_get_mem_type, ); + return a.mem_type != HVMMEM_mmio_dm; I was actually thinking of asking you to add another patch with pr_warn_once() here (and print error code as well). This call failing is indication of something going quite wrong and it would be good to know about this. Will include a patch in v2, thanks! -- Thanks, David / dhildenb ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 2/8] x86/xen: simplify xen_oldmem_pfn_is_ram()
On 9/29/21 5:03 AM, David Hildenbrand wrote: > On 29.09.21 10:45, David Hildenbrand wrote: >>> >> Can we go one step further and do >> >> >> @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) >> struct xen_hvm_get_mem_type a = { >> .domid = DOMID_SELF, >> .pfn = pfn, >> + .mem_type = HVMMEM_ram_rw, >> }; >> - int ram; >> - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, )) >> - 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; >> + HYPERVISOR_hvm_op(HVMOP_get_mem_type, ); >> + return a.mem_type != HVMMEM_mmio_dm; I was actually thinking of asking you to add another patch with pr_warn_once() here (and print error code as well). This call failing is indication of something going quite wrong and it would be good to know about this. >> } >> #endif >> >> >> Assuming that if HYPERVISOR_hvm_op() fails that >> .mem_type is not set to HVMMEM_mmio_dm. I don't think we can assume that argument described as OUT in the ABI will not be clobbered in case of error >> > > Okay we can't, due to "__must_check" ... so this is a good thing ;-) -boris ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 2/8] x86/xen: simplify xen_oldmem_pfn_is_ram()
On 29.09.21 10:45, David Hildenbrand wrote: How about return a.mem_type != HVMMEM_mmio_dm; Ha, how could I have missed that :) Result should be promoted to int and this has added benefit of not requiring changes in patch 4. Can we go one step further and do @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) struct xen_hvm_get_mem_type a = { .domid = DOMID_SELF, .pfn = pfn, + .mem_type = HVMMEM_ram_rw, }; - int ram; - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, )) - 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; + HYPERVISOR_hvm_op(HVMOP_get_mem_type, ); + return a.mem_type != HVMMEM_mmio_dm; } #endif Assuming that if HYPERVISOR_hvm_op() fails that .mem_type is not set to HVMMEM_mmio_dm. Okay we can't, due to "__must_check" ... -- Thanks, David / dhildenb ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 2/8] x86/xen: simplify xen_oldmem_pfn_is_ram()
How about return a.mem_type != HVMMEM_mmio_dm; Ha, how could I have missed that :) Result should be promoted to int and this has added benefit of not requiring changes in patch 4. Can we go one step further and do @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) struct xen_hvm_get_mem_type a = { .domid = DOMID_SELF, .pfn = pfn, + .mem_type = HVMMEM_ram_rw, }; - int ram; - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, )) - 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; + HYPERVISOR_hvm_op(HVMOP_get_mem_type, ); + return a.mem_type != HVMMEM_mmio_dm; } #endif Assuming that if HYPERVISOR_hvm_op() fails that .mem_type is not set to HVMMEM_mmio_dm. -- Thanks, David / dhildenb ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 2/8] x86/xen: simplify xen_oldmem_pfn_is_ram()
On 9/28/21 2:22 PM, David Hildenbrand wrote: > Let's simplify return handling. > > Signed-off-by: David Hildenbrand > --- > arch/x86/xen/mmu_hvm.c | 11 ++- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/xen/mmu_hvm.c b/arch/x86/xen/mmu_hvm.c > index b242d1f4b426..eb61622df75b 100644 > --- a/arch/x86/xen/mmu_hvm.c > +++ b/arch/x86/xen/mmu_hvm.c > @@ -21,23 +21,16 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) > .domid = DOMID_SELF, > .pfn = pfn, > }; > - int ram; > > if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, )) > return -ENXIO; > > switch (a.mem_type) { > case HVMMEM_mmio_dm: > - ram = 0; > - break; > - case HVMMEM_ram_rw: > - case HVMMEM_ram_ro: > + return 0; > default: > - ram = 1; > - break; > + return 1; > } > - > - return ram; > } > #endif > How about return a.mem_type != HVMMEM_mmio_dm; Result should be promoted to int and this has added benefit of not requiring changes in patch 4. -boris ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec