[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" prop

From: David Hildenbrand
Subject: Re: [Qemu-ppc] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" property during pre_plug
Date: Mon, 2 Jul 2018 12:39:43 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

On 02.07.2018 12:31, Igor Mammedov wrote:
> On Mon,  2 Jul 2018 11:37:55 +0200
> David Hildenbrand <address@hidden> wrote:
>> We can assign and verify the slot before realizing and trying to plug.
> s/slot/"addr"/
>> reading/writing the address property should never fail for DIMMs, so let's
>> reduce error handling a bit by using &error_abort. Getting access to the
>> memory region now might however fail. So forward errors from
>> get_memory_region() properly.
>> As all memory devices should use the alignment of the underlying memory
>> region for guest physical address asignment, do detection of the
>> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
>> alignment for compatibility handling.
>> Signed-off-by: David Hildenbrand <address@hidden>
> with commit message fixup,
> Reviewed-by: Igor Mammedov <address@hidden>
> ---
> For future reference, I don't really like 2 things about patch
>  1: mixes both error handling and functional changes (should be separate 
> patches)
>  2: that property setter may crash QEMU at preplug stage
>     where it should gracefully error out (so put proper error check
>     as follow up patch or respin this one maybe taking in account #1.)

Thanks for the review.

1. could have been factored out into a separate patch.

Regarding 2, I won't respin as long there is nothing else to take care
of. As long as we are in the pc-dimm world and we have static
properties, I don't see any reason to add error handling that cannot
happen. It will be different once we factor out more stuff into memory
device code. I assume you have a different opinion about that, but I
consider these nits.



David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]