Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 02/11/2012 03:12 PM, Andreas Färber wrote: Yes and no. They can have any target-specific pointer they want, just as before. But no global first_cpu / cpu_single_env pointer - that's replaced by CPU pointers, through which members of derived classes can be accessed (which did not work for CPUState due to CPU_COMMON members being at target-specific offset in the middle). Hmm, now I'm not even sure what I want that Andreas referred to. :) I definitely would like CPUState pointers to be changed into link properties, but that's not related to what Jan is doing here with cpu_single_env. Each LAPIC refers to a CPU, and that would become a link property indeed. But here we're using cpu_single_env to find out which LAPIC is being read. It's the other direction. Relying on thread-local cpu_single_env means that you restrict LAPIC memory reads to run in VCPU thread context, and this makes sense anyway. The only case of MMIO running in iothread context is Xen, but Xen always keeps the LAPIC in the hypervisor. Also, I think that having a view of CPUs in QOM is laudable, but I don't understand why that means you need to remove first_cpu / cpu_single_env. Finally, CPU_COMMON members may be referenced from TCG-generated code, how do you plan to move them and still keep the TLBs at small offsets within CPUState? Perhaps we need a drawing of the situation before and after the QOMization of CPUs. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 11.02.2012 15:24, schrieb Jan Kiszka: > On 2012-02-11 15:12, Andreas Färber wrote: >> Am 11.02.2012 15:02, schrieb Jan Kiszka: >>> On 2012-02-11 14:59, Andreas Färber wrote: Am 11.02.2012 14:35, schrieb Jan Kiszka: > On 2012-02-11 14:21, Andreas Färber wrote: >> CPU base class v3: >> http://patchwork.ozlabs.org/patch/139284/ (v4 coming up) >> >> That doesn't prevent target-specific devices. Although >> Paolo does want that to change wrt properties. > This patch doesn't explain yet what shall happen to > cpu_single_env and CPUState accesses from target-specific > devices. True. We can't change them before all targets are converted. So far I have 3/14 and still some review comments to work in. Another patch in that series uses a macro s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while we convert targets. Depending on our taste, cpu_single_env might become cpu_single_cpu, single_cpu or cpu_single. > Do you plan accessors for registers? No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU, etc. and their CPU*State. It would be possible to have static inline accessors but so far I've seen no need. >> >>> Then the devices need to have access to a CPUState pointer, >>> just as so far. >> >> Yes and no. They can have any target-specific pointer they want, >> just as before. But no global first_cpu / cpu_single_env pointer >> - that's > > If you want to drop first_cpu, you need to provide a for_each_cpu > iterating service instead. And cpu_single_env can only be obsoleted > if I/O access handlers can otherwise query the triggering CPU. I already answered that above. Please join #qemu if you further want to discuss that, for this thread seems to lead nowhere. Andreas > >> replaced by CPU pointers, through which members of derived >> classes can be accessed (which did not work for CPUState due to >> CPU_COMMON members being at target-specific offset in the >> middle). >> >> There's nothing wrong with your patch per se, just that it may >> need to get refactored some time soonish. We need to be aware of >> it so that we don't create merge conflicts for Anthony. > > There can't be logical merge conflicts as the no fundamentally new > requirements are introduced with this series. And we have no code > proposal seen yet to address them already for the existing use > cases. > > Jan > - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.18 (GNU/Linux) iQIcBAEBAgAGBQJPNoALAAoJEPou0S0+fgE/OHAQALK7X6v9nA0A4tZ8umD4Ak8p DkyHX9N0pkv8Jc9y06WWLCzsgCQJFxPKp0n0mpWHhG96mWryez+Cd8x00W9wJJWx A15beRV80jwpDWlkNMtnQ+T9kvVamUsL3a090Bgcb662EkCpfR5UtjDlrYBM7X7f C/ejV31NYnFIXM5F2TcsURrXZ7GRXNeSRsoXrt2WoCBhFkf+DBek8ejEsYcFS6q0 lrqoggHTVKRZuGbBIJ9yS3/L/pf6gWDOv1ZyUAHfAUeWt2rD3NxNFHHFLbrl3d47 k5Yev4acZOTe6ozgvK3qWcrvA2t42BmKTCA7FqLKg2057szll277wKHf0K2xqlvs oYTbSk4t9IWI4StBFevgVDM0kaXg6OAGKiDDP8PRrBI3YzJajLL6zkDVitA5Hp0N LPryOYwhI+KtO3Too7R919UDZIoZ+pg2Mm+L1/1IuneB8Ar1MeiPwU0zXLYGiDVx ZrMzjhhbYJn+PPC8FxI9gnaPkLVkZzSfcXkpA1RXLZdpkjdmt4rwA0KfFNB000DU fag3rAGTdcvT8O58K2FXYAWe8VFqA979VHWxsTOLrVX3rL9Xbi63SUfvz/joMryO mZMYsJ/NGHd5IVYdWP0kBdxuXRtFUaqHnp7PFnwj0IXtnV13csgB2nN+HN5wJULs A855i5ibqUahcKGej48W =jxwX -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 2012-02-11 15:12, Andreas Färber wrote: > Am 11.02.2012 15:02, schrieb Jan Kiszka: >> On 2012-02-11 14:59, Andreas Färber wrote: >>> Am 11.02.2012 14:35, schrieb Jan Kiszka: On 2012-02-11 14:21, Andreas Färber wrote: > CPU base class v3: http://patchwork.ozlabs.org/patch/139284/ > (v4 coming up) > > That doesn't prevent target-specific devices. Although Paolo > does want that to change wrt properties. >>> This patch doesn't explain yet what shall happen to cpu_single_env and CPUState accesses from target-specific devices. >>> >>> True. We can't change them before all targets are converted. So >>> far I have 3/14 and still some review comments to work in. >>> >>> Another patch in that series uses a macro >>> s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while >>> we convert targets. >>> >>> Depending on our taste, cpu_single_env might become >>> cpu_single_cpu, single_cpu or cpu_single. >>> Do you plan accessors for registers? >>> >>> No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU, >>> etc. and their CPU*State. It would be possible to have static >>> inline accessors but so far I've seen no need. > >> Then the devices need to have access to a CPUState pointer, just as >> so far. > > Yes and no. They can have any target-specific pointer they want, just > as before. But no global first_cpu / cpu_single_env pointer - that's If you want to drop first_cpu, you need to provide a for_each_cpu iterating service instead. And cpu_single_env can only be obsoleted if I/O access handlers can otherwise query the triggering CPU. > replaced by CPU pointers, through which members of derived classes can > be accessed (which did not work for CPUState due to CPU_COMMON members > being at target-specific offset in the middle). > > There's nothing wrong with your patch per se, just that it may need to > get refactored some time soonish. We need to be aware of it so that we > don't create merge conflicts for Anthony. There can't be logical merge conflicts as the no fundamentally new requirements are introduced with this series. And we have no code proposal seen yet to address them already for the existing use cases. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On Sat, Feb 11, 2012 at 14:18, Jan Kiszka wrote: > On 2012-02-11 15:11, Blue Swirl wrote: >> On Sat, Feb 11, 2012 at 14:00, Jan Kiszka wrote: >>> On 2012-02-11 14:54, Blue Swirl wrote: On Sat, Feb 11, 2012 at 12:43, Jan Kiszka wrote: > On 2012-02-11 12:49, Andreas Färber wrote: >> Am 11.02.2012 12:25, schrieb Blue Swirl: >>> I think using cpu_single_env is an indication of a problem, like poor >>> code, layering violation or poor API (vmport). What is your use case? >> >> I couldn't spot any in this series. Jan, note that any new use of env or >> cpu_single_env will need to be redone when we convert to QOM CPU. > > cpu_single_env should have nothing to do with QOM. > > The ABIs of vmport and the KVM VAPI require a reference to the calling > VCPU, and that's why you find tons of them in patch 5. Yes, this seems to be another case of a badly designed ABI. I guess there is no way to change that anymore, just like vmport? >>> >>> Believe me, I grumbled over it more than once while porting it from >>> qemu-kvm. The point is that some (Windows) VMs out there are running >>> already with this option ROM loaded and working this unfortunate ABI. >> >> Maybe in time those could be deprecated and a ROM using a sane ABI >> introduced instead. After some grace time the old ABI could be finally >> removed. > > At some point. But now we have this interface and no other even thought > out. Given that we want to provide a migration path from qemu-kvm to > upstream rather sooner than later, I think there is no way around this > model for a certain, not too short period. Yes, that is inevitable. >> Some of the cpu_single_env accesses in patch 5 could be avoided when APIC is moved closer to CPU. VAPIC should be also close to APIC so it should be able to access the CPU directly. In some other cases the current state could be passed around instead once it is known. >>> >>> Some callbacks are I/O-port originated, ie. not associated with the >>> per-CPU MMIO area or some MSR. So we would have to pass down the causing >>> CPU to every I/O handler - not sure if that is desired... >> >> I meant things like vapic_enable_tpr_reporting(), current CPUState >> could be passed via vapic_prepare() easily. > > Oh, there is in fact dead code in vapic_enable_tpr_reporting. It just > iterates over all VCPUs, no need to know the current one. Ok. > Jan > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 2012-02-11 15:11, Blue Swirl wrote: > On Sat, Feb 11, 2012 at 14:00, Jan Kiszka wrote: >> On 2012-02-11 14:54, Blue Swirl wrote: >>> On Sat, Feb 11, 2012 at 12:43, Jan Kiszka wrote: On 2012-02-11 12:49, Andreas Färber wrote: > Am 11.02.2012 12:25, schrieb Blue Swirl: >> I think using cpu_single_env is an indication of a problem, like poor >> code, layering violation or poor API (vmport). What is your use case? > > I couldn't spot any in this series. Jan, note that any new use of env or > cpu_single_env will need to be redone when we convert to QOM CPU. cpu_single_env should have nothing to do with QOM. The ABIs of vmport and the KVM VAPI require a reference to the calling VCPU, and that's why you find tons of them in patch 5. >>> >>> Yes, this seems to be another case of a badly designed ABI. I guess >>> there is no way to change that anymore, just like vmport? >> >> Believe me, I grumbled over it more than once while porting it from >> qemu-kvm. The point is that some (Windows) VMs out there are running >> already with this option ROM loaded and working this unfortunate ABI. > > Maybe in time those could be deprecated and a ROM using a sane ABI > introduced instead. After some grace time the old ABI could be finally > removed. At some point. But now we have this interface and no other even thought out. Given that we want to provide a migration path from qemu-kvm to upstream rather sooner than later, I think there is no way around this model for a certain, not too short period. > >>> >>> Some of the cpu_single_env accesses in patch 5 could be avoided when >>> APIC is moved closer to CPU. VAPIC should be also close to APIC so it >>> should be able to access the CPU directly. In some other cases the >>> current state could be passed around instead once it is known. >> >> Some callbacks are I/O-port originated, ie. not associated with the >> per-CPU MMIO area or some MSR. So we would have to pass down the causing >> CPU to every I/O handler - not sure if that is desired... > > I meant things like vapic_enable_tpr_reporting(), current CPUState > could be passed via vapic_prepare() easily. Oh, there is in fact dead code in vapic_enable_tpr_reporting. It just iterates over all VCPUs, no need to know the current one. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 11.02.2012 15:02, schrieb Jan Kiszka: > On 2012-02-11 14:59, Andreas Färber wrote: >> Am 11.02.2012 14:35, schrieb Jan Kiszka: >>> On 2012-02-11 14:21, Andreas Färber wrote: CPU base class v3: http://patchwork.ozlabs.org/patch/139284/ (v4 coming up) That doesn't prevent target-specific devices. Although Paolo does want that to change wrt properties. >> >>> This patch doesn't explain yet what shall happen to >>> cpu_single_env and CPUState accesses from target-specific >>> devices. >> >> True. We can't change them before all targets are converted. So >> far I have 3/14 and still some review comments to work in. >> >> Another patch in that series uses a macro >> s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while >> we convert targets. >> >> Depending on our taste, cpu_single_env might become >> cpu_single_cpu, single_cpu or cpu_single. >> >>> Do you plan accessors for registers? >> >> No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU, >> etc. and their CPU*State. It would be possible to have static >> inline accessors but so far I've seen no need. > > Then the devices need to have access to a CPUState pointer, just as > so far. Yes and no. They can have any target-specific pointer they want, just as before. But no global first_cpu / cpu_single_env pointer - that's replaced by CPU pointers, through which members of derived classes can be accessed (which did not work for CPUState due to CPU_COMMON members being at target-specific offset in the middle). There's nothing wrong with your patch per se, just that it may need to get refactored some time soonish. We need to be aware of it so that we don't create merge conflicts for Anthony. Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.18 (GNU/Linux) iQIcBAEBAgAGBQJPNndNAAoJEPou0S0+fgE/+EoQAJFau/xsn2CYcuKPEsJmAMRk yhOPBT6EJ2Q+34h31uLr3iftxQO9JpnLfEhB7ekTs36i0GklUsCQKgn4rg6vmPKj tLvUk/hF4zuqJzUJOwxnYxYjuzdEGHuEbkCYgclUtNnywHCo3GLXhqP0izSds9mF MhmqD45GblecjUpH7zdM/WTvulQm824hbDFPTCQaH8IQsw0QxT1Y4B71gpQtFJvJ pVk2+qfc488ClhOhPISC5IiQFPnR7DVju82FuDgn6JFq/db9o3KXqIRlQg7pqkPc h4K+Nz/rhzWpR6jtbTKqJV3yWBV9vxs6YDMSICZnGBabTlHh+tKoabg25Aj5zbcM 6Dmw10uFybi+jKlygKiSSxExParaRC9B3EFCk4dUhMC28B+qFSEkRA62Qpjndxwg HCmzg2kSQpufyrWNdWj8W+mNygU/0rm8xcB7fX1vhSOmdu3DNTPIH7P4C9hOfC1g hdIo0DpSd4AFfEIjZ0Loq0XOWKO9V05pOlcVsGmnCmGmfPXFPHWCFq3LGPz9Bj/7 rK1YtReDMXFOhq+QsOuRDuz1pCpPEfT4YhiXRuPsLlIaSszjFx3i6WAxBmh/tTtA oxoGZQPUI3SRZYZPN5W+J5HqRyNkB16ffsrbcHVTmCrUm33yT+7a6S/vPE9NlZpm zy92ShUp7JDvFjtnyOLK =uTCH -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On Sat, Feb 11, 2012 at 14:00, Jan Kiszka wrote: > On 2012-02-11 14:54, Blue Swirl wrote: >> On Sat, Feb 11, 2012 at 12:43, Jan Kiszka wrote: >>> On 2012-02-11 12:49, Andreas Färber wrote: Am 11.02.2012 12:25, schrieb Blue Swirl: > I think using cpu_single_env is an indication of a problem, like poor > code, layering violation or poor API (vmport). What is your use case? I couldn't spot any in this series. Jan, note that any new use of env or cpu_single_env will need to be redone when we convert to QOM CPU. >>> >>> cpu_single_env should have nothing to do with QOM. >>> >>> The ABIs of vmport and the KVM VAPI require a reference to the calling >>> VCPU, and that's why you find tons of them in patch 5. >> >> Yes, this seems to be another case of a badly designed ABI. I guess >> there is no way to change that anymore, just like vmport? > > Believe me, I grumbled over it more than once while porting it from > qemu-kvm. The point is that some (Windows) VMs out there are running > already with this option ROM loaded and working this unfortunate ABI. Maybe in time those could be deprecated and a ROM using a sane ABI introduced instead. After some grace time the old ABI could be finally removed. >> >> Some of the cpu_single_env accesses in patch 5 could be avoided when >> APIC is moved closer to CPU. VAPIC should be also close to APIC so it >> should be able to access the CPU directly. In some other cases the >> current state could be passed around instead once it is known. > > Some callbacks are I/O-port originated, ie. not associated with the > per-CPU MMIO area or some MSR. So we would have to pass down the causing > CPU to every I/O handler - not sure if that is desired... I meant things like vapic_enable_tpr_reporting(), current CPUState could be passed via vapic_prepare() easily. > Jan > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 2012-02-11 14:59, Andreas Färber wrote: > Am 11.02.2012 14:35, schrieb Jan Kiszka: >> On 2012-02-11 14:21, Andreas Färber wrote: >>> CPU base class v3: http://patchwork.ozlabs.org/patch/139284/ (v4 >>> coming up) >>> >>> That doesn't prevent target-specific devices. Although Paolo does >>> want that to change wrt properties. > >> This patch doesn't explain yet what shall happen to cpu_single_env >> and CPUState accesses from target-specific devices. > > True. We can't change them before all targets are converted. So far I > have 3/14 and still some review comments to work in. > > Another patch in that series uses a macro > s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while we > convert targets. > > Depending on our taste, cpu_single_env might become cpu_single_cpu, > single_cpu or cpu_single. > >> Do you plan accessors for registers? > > No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU, etc. > and their CPU*State. It would be possible to have static inline > accessors but so far I've seen no need. Then the devices need to have access to a CPUState pointer, just as so far. > >> And a service to return the CPU object associated with the >> execution context? > > What do you mean by execution context? TLS? The modelling of the state > is pretty orthogonal to how/where we store it. I mean "Where come this I/O access from?" and "am I running over some VCPU thread?". This questions need to be answered in target-specific device models and some parts of cpus.c (the latter is one motivation for this patch). Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 2012-02-11 14:54, Blue Swirl wrote: > On Sat, Feb 11, 2012 at 12:43, Jan Kiszka wrote: >> On 2012-02-11 12:49, Andreas Färber wrote: >>> Am 11.02.2012 12:25, schrieb Blue Swirl: I think using cpu_single_env is an indication of a problem, like poor code, layering violation or poor API (vmport). What is your use case? >>> >>> I couldn't spot any in this series. Jan, note that any new use of env or >>> cpu_single_env will need to be redone when we convert to QOM CPU. >> >> cpu_single_env should have nothing to do with QOM. >> >> The ABIs of vmport and the KVM VAPI require a reference to the calling >> VCPU, and that's why you find tons of them in patch 5. > > Yes, this seems to be another case of a badly designed ABI. I guess > there is no way to change that anymore, just like vmport? Believe me, I grumbled over it more than once while porting it from qemu-kvm. The point is that some (Windows) VMs out there are running already with this option ROM loaded and working this unfortunate ABI. > > Some of the cpu_single_env accesses in patch 5 could be avoided when > APIC is moved closer to CPU. VAPIC should be also close to APIC so it > should be able to access the CPU directly. In some other cases the > current state could be passed around instead once it is known. Some callbacks are I/O-port originated, ie. not associated with the per-CPU MMIO area or some MSR. So we would have to pass down the causing CPU to every I/O handler - not sure if that is desired... Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 11.02.2012 14:35, schrieb Jan Kiszka: > On 2012-02-11 14:21, Andreas Färber wrote: >> CPU base class v3: http://patchwork.ozlabs.org/patch/139284/ (v4 >> coming up) >> >> That doesn't prevent target-specific devices. Although Paolo does >> want that to change wrt properties. > > This patch doesn't explain yet what shall happen to cpu_single_env > and CPUState accesses from target-specific devices. True. We can't change them before all targets are converted. So far I have 3/14 and still some review comments to work in. Another patch in that series uses a macro s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while we convert targets. Depending on our taste, cpu_single_env might become cpu_single_cpu, single_cpu or cpu_single. > Do you plan accessors for registers? No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU, etc. and their CPU*State. It would be possible to have static inline accessors but so far I've seen no need. > And a service to return the CPU object associated with the > execution context? What do you mean by execution context? TLS? The modelling of the state is pretty orthogonal to how/where we store it. HTE, Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.18 (GNU/Linux) iQIcBAEBAgAGBQJPNnQvAAoJEPou0S0+fgE/yYAP/j4IUTrrP1invYoVLOiea7wn 9yJ3TgssSUPnQyQRUmkXFvx1hj0Z6umlyxTK+dc1DQF8jJtoouo3CS0D+tyIZEhd GHN9J0qtgiDzvl1c+3b092VTw47gtv+rXjGUcyKSiTqyGl3OCdbIgt21HK8cT6CN U7n2pFGBeZiX8GEYiZmhAglyJ45jGpWmVulGYiqzOlBYPLaYi0CQ25NVUalBpq4I MIEqdW/W8lx0+h5Onl+qUo2btHNQCnG1oPH/BmdVf7Pe1G99VynOXwFVNXeJ2gR4 Lb7wnzKFqdybktkNLtkAIuC0gFj+Ph9+wfVw/QGsCBc0r6gotE8O9uDTyxs1ro+2 in6Am/A+o4M02sOf9uhaYx0l3uryOyXifiIAVzj4Y8s0QhyeDfPa6f8p4iQKh+gE m/bvbDTb5hU9nW68IuiFXK9dfQMmU2ub5Gx7UAHuyOgEzV0gOPAf4nugYux3owIw kYJy6sWFUMH/+l94nKAI0FanmL6JSOmA8hfaLXCXOfvfX9CfJEN+KEotj7Ma0Tcz +YFAlGkwZYmnJmvFakxFlecRUYY/lpwlIusqRJsw1KP40pHuT8GZUDzv6Wn1UD2R /6xeL7007iUmD+mafc+3xFbKMXS+kyF6+syM3xh/7r1SRyAIZQeJnZvLm8pZXS0T tsKlb6nYaV+NLwD/rKy0 =09lZ -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On Sat, Feb 11, 2012 at 12:43, Jan Kiszka wrote: > On 2012-02-11 12:49, Andreas Färber wrote: >> Am 11.02.2012 12:25, schrieb Blue Swirl: >>> I think using cpu_single_env is an indication of a problem, like poor >>> code, layering violation or poor API (vmport). What is your use case? >> >> I couldn't spot any in this series. Jan, note that any new use of env or >> cpu_single_env will need to be redone when we convert to QOM CPU. > > cpu_single_env should have nothing to do with QOM. > > The ABIs of vmport and the KVM VAPI require a reference to the calling > VCPU, and that's why you find tons of them in patch 5. Yes, this seems to be another case of a badly designed ABI. I guess there is no way to change that anymore, just like vmport? Some of the cpu_single_env accesses in patch 5 could be avoided when APIC is moved closer to CPU. VAPIC should be also close to APIC so it should be able to access the CPU directly. In some other cases the current state could be passed around instead once it is known. > > Jan > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 2012-02-11 14:21, Andreas Färber wrote: > Am 11.02.2012 14:07, schrieb Jan Kiszka: >> On 2012-02-11 14:06, Andreas Färber wrote: >>> Am 11.02.2012 13:43, schrieb Jan Kiszka: On 2012-02-11 12:49, Andreas Färber wrote: > Am 11.02.2012 12:25, schrieb Blue Swirl: >> I think using cpu_single_env is an indication of a >> problem, like poor code, layering violation or poor API >> (vmport). What is your use case? > > I couldn't spot any in this series. Jan, note that any new > use of env or cpu_single_env will need to be redone when we > convert to QOM CPU. >>> cpu_single_env should have nothing to do with QOM. >>> >>> It does, cf. my patch series: Current CPU*State is being embedded >>> in the QOM object and most future code outside TCG will use a > > Let me stress this: > >>> CPU rather than CPUState pointer. > >>> The reason is that CPUState is totally target-specific and does >>> not belong in common code. > >> So are the devices that depend on a current CPU pointer. You will >> have to provide something equivalent. > > CPU base class v3: > http://patchwork.ozlabs.org/patch/139284/ (v4 coming up) > > That doesn't prevent target-specific devices. Although Paolo does want > that to change wrt properties. This patch doesn't explain yet what shall happen to cpu_single_env and CPUState accesses from target-specific devices. Do you plan accessors for registers? And a service to return the CPU object associated with the execution context? Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 11.02.2012 14:07, schrieb Jan Kiszka: > On 2012-02-11 14:06, Andreas Färber wrote: >> Am 11.02.2012 13:43, schrieb Jan Kiszka: >>> On 2012-02-11 12:49, Andreas Färber wrote: Am 11.02.2012 12:25, schrieb Blue Swirl: > I think using cpu_single_env is an indication of a > problem, like poor code, layering violation or poor API > (vmport). What is your use case? I couldn't spot any in this series. Jan, note that any new use of env or cpu_single_env will need to be redone when we convert to QOM CPU. >> >>> cpu_single_env should have nothing to do with QOM. >> >> It does, cf. my patch series: Current CPU*State is being embedded >> in the QOM object and most future code outside TCG will use a Let me stress this: >> CPU rather than CPUState pointer. >> The reason is that CPUState is totally target-specific and does >> not belong in common code. > > So are the devices that depend on a current CPU pointer. You will > have to provide something equivalent. CPU base class v3: http://patchwork.ozlabs.org/patch/139284/ (v4 coming up) That doesn't prevent target-specific devices. Although Paolo does want that to change wrt properties. Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.18 (GNU/Linux) iQIcBAEBAgAGBQJPNmtzAAoJEPou0S0+fgE/SRQP+gLK/FvwIOXZqvSn+i+ooxin jXOvH3oBtfiIQp5+59KGlOd7dSjILFwoPtH3U5tGDpI5HHLFpQQOsuppsiBwVOC9 9QUgqFt9d/xodvPJ0gv5ShghoEmCZNdFwNnBYeqB69mEDm5sZwYlvWgXaOgRti2+ 0lhGFVISetImmQbiy5l7ubMONwcGUCVuT7pjiZ+S/Cew7wvGW5O7fpo3P8b4Xw4E P7qX6y785Sm4Wn8iEangFOUqer5ALAS0fL2xHo5NYUUZ8jgn2xwDIT8TP9t8Pkei 5U0kWm+mNyvJ4VLxsN449LNGDV+c3AMyzPodRmV2KJBYISDRIFYlar/SkJGiBkvo cNKdJLrkm4KIEt6eomyhYgSHJi5nUeoT60lAaZkHIDNonKoFw8swhf85wSi7sQmq 38nIY+F5YAHZ3TQCfTfxTDHy2Wbc6G7bn792FWKOxCVLWtD2Bp3iQv8J3MlYEhMJ fnJv+/nKUQuPlti4LNwrhJyRLPUNrc6PKgzC8He4dupLMASFPuSMh4mKRlWj41+/ SYKvXz42elSqv2Z798eA8VNCbs7e+0EH67BJQLIL3QEuD4vY/Yfeulr3CGsfkLEL m+UIAAntzloSkZvxuKmI5MP5XrjHTAbWuab+Gh9kYVyEsWZj4TAvn1hobKtU7sv9 lMd32AbfCskRN8jAw8So =0Rvo -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 2012-02-11 14:06, Andreas Färber wrote: > Am 11.02.2012 13:43, schrieb Jan Kiszka: >> On 2012-02-11 12:49, Andreas Färber wrote: >>> Am 11.02.2012 12:25, schrieb Blue Swirl: I think using cpu_single_env is an indication of a problem, like poor code, layering violation or poor API (vmport). What is your use case? >>> >>> I couldn't spot any in this series. Jan, note that any new use of >>> env or cpu_single_env will need to be redone when we convert to >>> QOM CPU. > >> cpu_single_env should have nothing to do with QOM. > > It does, cf. my patch series: Current CPU*State is being embedded in > the QOM object and most future code outside TCG will use a CPU rather > than CPUState pointer. The reason is that CPUState is totally > target-specific and does not belong in common code. So are the devices that depend on a current CPU pointer. You will have to provide something equivalent. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 11.02.2012 13:43, schrieb Jan Kiszka: > On 2012-02-11 12:49, Andreas Färber wrote: >> Am 11.02.2012 12:25, schrieb Blue Swirl: >>> I think using cpu_single_env is an indication of a problem, >>> like poor code, layering violation or poor API (vmport). What >>> is your use case? >> >> I couldn't spot any in this series. Jan, note that any new use of >> env or cpu_single_env will need to be redone when we convert to >> QOM CPU. > > cpu_single_env should have nothing to do with QOM. It does, cf. my patch series: Current CPU*State is being embedded in the QOM object and most future code outside TCG will use a CPU rather than CPUState pointer. The reason is that CPUState is totally target-specific and does not belong in common code. Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.18 (GNU/Linux) iQIcBAEBAgAGBQJPNme8AAoJEPou0S0+fgE/bPcP/RRc85K6aJZEqRyw/lvN8+FB 2FtwOqCm6zTBiEfBOfs816YzBDl75F5BVRbNapMLi1Yp4y/BFwQF1lbpu7INF90R ZvY5BjjW8+xjBbGN0BhmkbjKdXZS1spjYNXDjIcUTvfj/GXW8Aamfj4IQVTpd+0D l1s6A/X4BgGoxEqLtnHi8mZojafFFW6Dy0tX7BOmAPwBJle+IK91huO/cmL3Ou3v 0X1Rl4UJlq7j5AxFZlbBkkMrB9vozMPZi983SpAyhieQTVqTB+XuRobwZZVWww0m ff2cBPBckFSF+i5L7eWvL+HfCD2aeYgwTCmfxtxOxjThwvM7gkyz59gQznUmb3yZ 0SLi9aj0dYQkuidoLxORZaAG20pqfvGCMezJQ6p45jhGmq7W3RzMqJX5Hh7GN0bY J+Yp1W/Svop9XS1MumERufO6E1+2TNpbtwGDizKV52DpT2dTtwQZJ9UjHUvLz52c avM5DvuuYLGDIyMteURoAh1eo27kfHFZs9vI6HFK3uXrmihgGihtzlVvFxf887kR LWt/QO8K/VzuktRKj9NutiMqJOUxIzddikxpkEU/80FOtMedy1Ne1cVpMWOTqXRh U0iayaZ3FKK+NfSYgHjSGCHubJG3JwV/Hawu01nWuUR1aGOsbQuxm1sNcQ+VV1zJ iGvD5Fdn+9+o+UTJSkiQ =zss6 -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On 2012-02-11 12:49, Andreas Färber wrote: > Am 11.02.2012 12:25, schrieb Blue Swirl: >> I think using cpu_single_env is an indication of a problem, like poor >> code, layering violation or poor API (vmport). What is your use case? > > I couldn't spot any in this series. Jan, note that any new use of env or > cpu_single_env will need to be redone when we convert to QOM CPU. cpu_single_env should have nothing to do with QOM. The ABIs of vmport and the KVM VAPI require a reference to the calling VCPU, and that's why you find tons of them in patch 5. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
Am 11.02.2012 12:25, schrieb Blue Swirl: > I think using cpu_single_env is an indication of a problem, like poor > code, layering violation or poor API (vmport). What is your use case? I couldn't spot any in this series. Jan, note that any new use of env or cpu_single_env will need to be redone when we convert to QOM CPU. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once
On Fri, Feb 10, 2012 at 18:31, Jan Kiszka wrote:
> As we have thread-local cpu_single_env now and KVM uses exactly one
> thread per VCPU, we can drop the cpu_single_env updates from the loop
> and initialize this variable only once during setup.
I don't think this is correct. Maybe you missed the part that sets
cpu_single_env to NULL, which I think is to annoy broken code that
assumes that some CPU state is always globally available. This is not
true for monitor context.
> Signed-off-by: Jan Kiszka
> ---
> cpus.c | 1 +
> kvm-all.c | 5 -
> 2 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index f45a438..d0c8340 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -714,6 +714,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
> qemu_mutex_lock(&qemu_global_mutex);
> qemu_thread_get_self(env->thread);
> env->thread_id = qemu_get_thread_id();
> + cpu_single_env = env;
>
> r = kvm_init_vcpu(env);
> if (r < 0) {
> diff --git a/kvm-all.c b/kvm-all.c
> index c4babda..e2cbc03 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1118,8 +1118,6 @@ int kvm_cpu_exec(CPUState *env)
> return EXCP_HLT;
> }
>
> - cpu_single_env = env;
> -
> do {
> if (env->kvm_vcpu_dirty) {
> kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
> @@ -1136,13 +1134,11 @@ int kvm_cpu_exec(CPUState *env)
> */
> qemu_cpu_kick_self();
> }
> - cpu_single_env = NULL;
> qemu_mutex_unlock_iothread();
>
> run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>
> qemu_mutex_lock_iothread();
> - cpu_single_env = env;
> kvm_arch_post_run(env, run);
>
> kvm_flush_coalesced_mmio_buffer();
> @@ -1206,7 +1202,6 @@ int kvm_cpu_exec(CPUState *env)
> }
>
> env->exit_request = 0;
> - cpu_single_env = NULL;
> return ret;
> }
>
> --
> 1.7.3.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
