From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35079) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gL8JS-0003HU-8i for qemu-devel@nongnu.org; Fri, 09 Nov 2018 10:03:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gL8JR-0001dE-4n for qemu-devel@nongnu.org; Fri, 09 Nov 2018 10:03:10 -0500 Received: from mail-ot1-x343.google.com ([2607:f8b0:4864:20::343]:46540) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gL8JD-0000sP-6h for qemu-devel@nongnu.org; Fri, 09 Nov 2018 10:03:08 -0500 Received: by mail-ot1-x343.google.com with SMTP id w25so1103076otm.13 for ; Fri, 09 Nov 2018 07:02:55 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <20181107155405.24013-1-minyard@acm.org> <20181107155405.24013-4-minyard@acm.org> <25d95354-e536-dff6-0936-7e8d951108a5@mvista.com> From: Peter Maydell Date: Fri, 9 Nov 2018 15:02:32 +0000 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: Re: [Qemu-devel] [PATCH 3/3] i2c: Add vmstate handling to the smbus eeprom List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Minyard Cc: Corey Minyard , QEMU Developers , Paolo Bonzini , "Dr . David Alan Gilbert" , "Michael S . Tsirkin" On 9 November 2018 at 14:56, Corey Minyard wrote: > On 11/8/18 12:03 PM, Peter Maydell wrote: >> >> On 8 November 2018 at 17:58, Corey Minyard wrote: >>> >>> On 11/8/18 8:08 AM, Peter Maydell wrote: >>>> >>>> This doesn't do anything for migration of the actual data contents. >>>> The current users of this device (hw/arm/aspeed.c and the >>>> smbus_eeprom_init() function) doesn't do anything >>>> to migrate the contents. For that matter, "user of the device >>>> passes a pointer to a random lump of memory via a device property" >>>> is a bit funky as an interface. The device should allocate its >>>> own memory and migrate it, and the user should just pass the >>>> initial required contents (default being "zero-initialize", >>>> which is what everybody except the mips_fulong2e, mips_malta >>>> and sam460ex want). >>> >>> I debated on this, and it depends on what the eeprom is used for. If >>> it's a DRAM eeprom, it shouldn't need to be transferred. >> >> It's guest-visible data; the guest can write it and read it back. >> So it needs to be migrated. Otherwise behaviour after migration >> will not be the same as if the guest had never migrated. > > > > I looked at adding it, but I ran into an issue. The value is a > > DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data) > > and that means the data has to be void *, but to transfer it it must be a > uint8_t *. > The pointer property seems to be a cheap hack, I'm not sure what it will > take > to fix it. The data provided by the caller is only the initialization data. So I think the device should create its own memory to copy that init data into, and migrate that ram, not wherever the initialization data lives. (Currently this "copy the data into our own ram" happens in the smbus_eeprom_init() wrapper, but we should do it in the device realize function instead.) Also there seem to be unresolved questions about what happens on reset -- should the EEPROM revert back to the initial contents? We don't do that at the moment, but this breaks the usual QEMU pattern that reset is as if you'd just completely restarted QEMU. thanks -- PMM