[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
Tue, 3 Jul 2018 10:22:14 +0200
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0
On 07/02/2018 02:47 PM, David Hildenbrand wrote:
> On 02.07.2018 14:44, Igor Mammedov wrote:
>> On Mon, 2 Jul 2018 12:39:43 +0200
>> David Hildenbrand <address@hidden> wrote:
>>> 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.
>> it's not really nits. it happens to work now but static properties are
>> just current impl. detail which can easily change without amending
>> *plug handlers.
>> It's cleaner/safer to handle errors properly and keeping device model
>> separate from machine helpers as much as possible from maintainer pov,
>> even at expense of extra boiler plate.
>> I expect you to re-introduce proper error checking when you
>> factor out memory_device_pre_plug() part /it's condition for my ack
>> on this patch/.
> Yes, already contained.
Reviewed-by: Eric Auger <address@hidden>
Re: [Qemu-devel] [PATCH v3 4/4] pc-dimm: assign and verify the "addr" property during pre_plug, David Gibson, 2018/07/02
- [Qemu-devel] [PATCH v3 3/4] pc: drop memory region alignment check for 0, (continued)