diff mbox series

[v2,kvmtool,17/30] hw/vesa: Don't ignore fatal errors

Message ID 20200123134805.1993-18-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series Add reassignable BARs and PCIE 1.1 support | expand

Commit Message

Alexandru Elisei Jan. 23, 2020, 1:47 p.m. UTC 
Failling an mmap call or creating a memslot means that device emulation
will not work, don't ignore it.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 hw/vesa.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Andre Przywara Jan. 30, 2020, 2:52 p.m. UTC | #1 
On Thu, 23 Jan 2020 13:47:52 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> Failling an mmap call or creating a memslot means that device emulation
> will not work, don't ignore it.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  hw/vesa.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vesa.c b/hw/vesa.c
> index b92cc990b730..a665736a76d7 100644
> --- a/hw/vesa.c
> +++ b/hw/vesa.c
> @@ -76,9 +76,11 @@ struct framebuffer *vesa__init(struct kvm *kvm)
>  
>  	mem = mmap(NULL, VESA_MEM_SIZE, PROT_RW, MAP_ANON_NORESERVE, -1, 0);
>  	if (mem == MAP_FAILED)
> -		ERR_PTR(-errno);
> +		return ERR_PTR(-errno);
>  
> -	kvm__register_dev_mem(kvm, VESA_MEM_ADDR, VESA_MEM_SIZE, mem);
> +	r = kvm__register_dev_mem(kvm, VESA_MEM_ADDR, VESA_MEM_SIZE, mem);
> +	if (r < 0)
> +		return ERR_PTR(r);

For the sake of correctness, we should munmap here, I think.
With that fixed:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

>  
>  	vesafb = (struct framebuffer) {
>  		.width			= VESA_WIDTH,
Alexandru Elisei March 6, 2020, 12:33 p.m. UTC | #2 
Hi,

On 1/30/20 2:52 PM, Andre Przywara wrote:
> On Thu, 23 Jan 2020 13:47:52 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
>> Failling an mmap call or creating a memslot means that device emulation
>> will not work, don't ignore it.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  hw/vesa.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/vesa.c b/hw/vesa.c
>> index b92cc990b730..a665736a76d7 100644
>> --- a/hw/vesa.c
>> +++ b/hw/vesa.c
>> @@ -76,9 +76,11 @@ struct framebuffer *vesa__init(struct kvm *kvm)
>>  
>>  	mem = mmap(NULL, VESA_MEM_SIZE, PROT_RW, MAP_ANON_NORESERVE, -1, 0);
>>  	if (mem == MAP_FAILED)
>> -		ERR_PTR(-errno);
>> +		return ERR_PTR(-errno);
>>  
>> -	kvm__register_dev_mem(kvm, VESA_MEM_ADDR, VESA_MEM_SIZE, mem);
>> +	r = kvm__register_dev_mem(kvm, VESA_MEM_ADDR, VESA_MEM_SIZE, mem);
>> +	if (r < 0)
>> +		return ERR_PTR(r);
> For the sake of correctness, we should munmap here, I think.
> With that fixed:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Actually, I think the correct cleanup order should be munmap(mem) ->
device__unregister(vesa_device) -> ioport__unregister(vesa_base_addr). I'll drop
your R-b.

Thanks,
Alex
>
> Cheers,
> Andre.
>
>>  
>>  	vesafb = (struct framebuffer) {
>>  		.width			= VESA_WIDTH,
diff mbox series

Patch

diff --git a/hw/vesa.c b/hw/vesa.c
index b92cc990b730..a665736a76d7 100644
--- a/hw/vesa.c
+++ b/hw/vesa.c
@@ -76,9 +76,11 @@  struct framebuffer *vesa__init(struct kvm *kvm)
 
 	mem = mmap(NULL, VESA_MEM_SIZE, PROT_RW, MAP_ANON_NORESERVE, -1, 0);
 	if (mem == MAP_FAILED)
-		ERR_PTR(-errno);
+		return ERR_PTR(-errno);
 
-	kvm__register_dev_mem(kvm, VESA_MEM_ADDR, VESA_MEM_SIZE, mem);
+	r = kvm__register_dev_mem(kvm, VESA_MEM_ADDR, VESA_MEM_SIZE, mem);
+	if (r < 0)
+		return ERR_PTR(r);
 
 	vesafb = (struct framebuffer) {
 		.width			= VESA_WIDTH,