From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36484) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZsgC-0005uO-Jp for qemu-devel@nongnu.org; Tue, 07 May 2013 20:56:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZsgB-0007DX-1u for qemu-devel@nongnu.org; Tue, 07 May 2013 20:56:24 -0400 Received: from mout.web.de ([212.227.15.3]:62806) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZsgA-0007DR-P2 for qemu-devel@nongnu.org; Tue, 07 May 2013 20:56:22 -0400 Message-ID: <5189A2AC.1060808@web.de> Date: Wed, 08 May 2013 02:56:12 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1367946947-17109-1-git-send-email-jordan.l.justen@intel.com> <1367946947-17109-4-git-send-email-jordan.l.justen@intel.com> <5189657B.4070109@redhat.com> <51897F59.8090303@redhat.com> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="----enig2NXQHVKXPTFJTRGCTMJSA" Subject: Re: [Qemu-devel] [PATCH v4 3/6] kvm: support using KVM_MEM_READONLY flag for readonly regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jordan Justen Cc: Paolo Bonzini , qemu-devel , Jordan Justen This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2NXQHVKXPTFJTRGCTMJSA Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2013-05-08 01:37, Jordan Justen wrote: > On Tue, May 7, 2013 at 3:25 PM, Paolo Bonzini wro= te: >> Il 08/05/2013 00:01, Jordan Justen ha scritto: >>> On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini w= rote: >>>> Il 07/05/2013 19:15, Jordan Justen ha scritto: >>>>> A slot that uses KVM_MEM_READONLY can be read from and code >>>>> can execute from the region, but writes will trap. >>>>> >>>>> For regions that are readonly and also not writeable, we >>>>> force the slot to be removed so reads or writes to the region >>>>> will trap. (A memory region in this state is not executable >>>>> within kvm.) >>>>> >>>>> Signed-off-by: Jordan Justen >>>>> Reviewed-by: Xiao Guangrong >>>>> --- >>>>> kvm-all.c | 36 +++++++++++++++++++++++++++--------- >>>>> 1 file changed, 27 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/kvm-all.c b/kvm-all.c >>>>> index 1686adc..fffd2f4 100644 >>>>> --- a/kvm-all.c >>>>> +++ b/kvm-all.c >>>>> @@ -201,12 +201,18 @@ static int kvm_set_user_memory_region(KVMStat= e *s, KVMSlot *slot) >>>>> >>>>> mem.slot =3D slot->slot; >>>>> mem.guest_phys_addr =3D slot->start_addr; >>>>> - mem.memory_size =3D slot->memory_size; >>>>> mem.userspace_addr =3D (unsigned long)slot->ram; >>>>> mem.flags =3D slot->flags; >>>>> if (s->migration_log) { >>>>> mem.flags |=3D KVM_MEM_LOG_DIRTY_PAGES; >>>>> } >>>>> + if (mem.flags & KVM_MEM_READONLY && mem.memory_size !=3D 0) { >>>>> + /* Set the slot size to 0 before setting the slot to the d= esired >>>>> + * value. This is needed based on KVM commit 75d61fbc. */ >>>>> + mem.memory_size =3D 0; >>>>> + kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem); >>>>> + } >>>>> + mem.memory_size =3D slot->memory_size; >>>>> return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem); >>>>> } >>>>> >>>>> @@ -268,9 +274,14 @@ err: >>>>> * dirty pages logging control >>>>> */ >>>>> >>>>> -static int kvm_mem_flags(KVMState *s, bool log_dirty) >>>>> +static int kvm_mem_flags(KVMState *s, bool log_dirty, bool readonl= y) >>>>> { >>>>> - return log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0; >>>>> + int flags =3D 0; >>>>> + flags =3D log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0; >>>>> + if (readonly && kvm_readonly_mem_allowed) { >>>>> + flags |=3D KVM_MEM_READONLY; >>>>> + } >>>>> + return flags; >>>>> } >>>>> >>>>> static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_= dirty) >>>>> @@ -281,7 +292,7 @@ static int kvm_slot_dirty_pages_log_change(KVMS= lot *mem, bool log_dirty) >>>>> >>>>> old_flags =3D mem->flags; >>>>> >>>>> - flags =3D (mem->flags & ~mask) | kvm_mem_flags(s, log_dirty); >>>>> + flags =3D (mem->flags & ~mask) | kvm_mem_flags(s, log_dirty, f= alse); >>>>> mem->flags =3D flags; >>>>> >>>>> /* If nothing changed effectively, no need to issue ioctl */ >>>>> @@ -638,7 +649,14 @@ static void kvm_set_phys_mem(MemoryRegionSecti= on *section, bool add) >>>>> } >>>>> >>>>> if (!memory_region_is_ram(mr)) { >>>>> - return; >>>>> + if (!mr->readonly || !kvm_readonly_mem_allowed) { >>>>> + return; >>>>> + } else if (!mr->readable && add) { >>>>> + /* If the memory range is not readable, then we actual= ly want >>>>> + * to remove the kvm memory slot so all accesses will = trap. */ >>>>> + assert(mr->readonly && kvm_readonly_mem_allowed); >>>>> + add =3D false; >>>>> + } >>>> >>>> mr->readable really means "read from ROM, write to device", hence it= >>> >>> "read from ROM, write to device" confuses me. >>> >>> Here is how I currently to interpret things: >>> !mr->readable: trap on read >>> mr->readonly: trap on write >> >> mtree_print_mr tells us how it really goes: >> >> mr->readable ? 'R' : '-', >> !mr->readonly && !(mr->rom_device && mr->readable) ? 'W' >> : '-', >> >> So perhaps >> >> bool readable =3D mr->readable; >> bool writeable =3D !mr->readonly && !memory_region_is_romd(mr)= ; >> assert(!(writeable && !readable)); >> if (writeable || !kvm_readonly_mem_allowed) { >> return; >> } >> if (!readable) { >> /* If the memory range is not readable, then we actually w= ant >> * to remove the kvm memory slot so all accesses will trap= =2E */ >> add =3D false; >> } >> >> This should still make patch 4 unnecessary. >=20 > Patch 4 sets readonly for the pflash device. Basically saying, it > always should trap on writes. This is wrong. The semantic of readonly is that writes shall be ignored by the emulation. But a flash device is _never_ readonly - otherwise you couldn't send the magic write-enable commands to them. >=20 > memory_region_is_romd seems to have more to do with the readability of > the range. Also wrong. A ROMD device is always readable (hence my patch to rename "readable" to "romd_mode" - it is misleading). What is changed via "readable" is who handles the read access, RAM or a device model. >=20 > Patch 4 would seem to make more sense if written as > memory_region_set_write_trapping(mr, true). Patch 4 is not correct. And "trapping" is, as Peter mentioned, a misleading term. You are talking about trapping /wrt KVM. But that's an internal thing, nothing a memory region user has to care about. What you need for proper KVM mapping: - region is readonly -> KVM read-only memslot on backing RAM, writes shall be ignored (in user space) - memory_region_is_romd() -> KVM read-only memslot, writes go to the MMIO handler of the region IOW, a read-only KVM slot is required if readonly || is_romd. That should be all. Jan ------enig2NXQHVKXPTFJTRGCTMJSA Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlGJorMACgkQitSsb3rl5xQ+bQCfYEuFGP+UCJ/xywawola4Lh6B tacAnRFK2+tzgccqSSrPcsVIE4Moq6yK =Bfj1 -----END PGP SIGNATURE----- ------enig2NXQHVKXPTFJTRGCTMJSA--