[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" pr
Re: [Qemu-devel] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" property during pre_plug
Mon, 2 Jul 2018 12:39:43 +0200
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.
>> 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
> 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
Re: [Qemu-devel] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" property during pre_plug, David Gibson, 2018/07/02