Re: [PATCH v16 00/23] i386 cleanup PART 2
On 3/15/21 10:44 AM, Claudio Fontana wrote:
> Hi Philippe,
>
> On 3/14/21 1:00 AM, Philippe Mathieu-Daudé wrote:
>> Hi Claudio,
>>
>> I am looking at this code:
>>
>> $ git grep tcg_ softmmu/physmem.c
>> softmmu/physmem.c:153:static void
>> tcg_log_global_after_sync(MemoryListener *listener);
>> softmmu/physmem.c:154:static void tcg_commit(MemoryListener *listener);
>> softmmu/physmem.c:161: * @tcg_as_listener: listener for tracking changes
>> to the AddressSpace
>> softmmu/physmem.c:167:MemoryListener tcg_as_listener;
>> softmmu/physmem.c:590:static void tcg_iommu_unmap_notify(IOMMUNotifier
>> *n, IOMMUTLBEntry *iotlb)
>> softmmu/physmem.c:606:static void tcg_register_iommu_notifier(CPUState *cpu,
>> softmmu/physmem.c:640:tcg_iommu_unmap_notify,
>> softmmu/physmem.c:654:void tcg_iommu_free_notifier_list(CPUState *cpu)
>> softmmu/physmem.c:668:void tcg_iommu_init_notifier_list(CPUState *cpu)
>> softmmu/physmem.c:698:tcg_register_iommu_notifier(cpu, iommu_mr,
>> iommu_idx);
>> softmmu/physmem.c:761:if (tcg_enabled()) {
>> softmmu/physmem.c:762:
>> newas->tcg_as_listener.log_global_after_sync = tcg_log_global_after_sync;
>> softmmu/physmem.c:763:newas->tcg_as_listener.commit = tcg_commit;
>> softmmu/physmem.c:764:
>> memory_listener_register(>tcg_as_listener, as);
>> softmmu/physmem.c:891:assert(tcg_enabled());
>> softmmu/physmem.c:904:if (cc->tcg_ops->adjust_watchpoint_address) {
>> softmmu/physmem.c:906:addr =
>> cc->tcg_ops->adjust_watchpoint_address(cpu, addr, len);
>> softmmu/physmem.c:927:if (wp->flags & BP_CPU &&
>> cc->tcg_ops->debug_check_watchpoint &&
>> softmmu/physmem.c:928:
>> !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
>> softmmu/physmem.c:1004:assert(tcg_enabled());
>> softmmu/physmem.c:1059:if (dirty && tcg_enabled()) {
>> softmmu/physmem.c:1107:if (tcg_enabled()) {
>> softmmu/physmem.c:2605:static void
>> tcg_log_global_after_sync(MemoryListener *listener)
>> softmmu/physmem.c:2634:cpuas = container_of(listener,
>> CPUAddressSpace, tcg_as_listener);
>> softmmu/physmem.c:2639:static void tcg_commit(MemoryListener *listener)
>> softmmu/physmem.c:2644:assert(tcg_enabled());
>> softmmu/physmem.c:2647:cpuas = container_of(listener,
>> CPUAddressSpace, tcg_as_listener);
>> softmmu/physmem.c:2700:assert(tcg_enabled());
>> softmmu/physmem.c:3000:if (tcg_enabled()) {
>>
>> which reminded me the starter generic part of your effort
>> (already merged).
>>
>> Do you have plans for this code?
>>
>> Similarly:
>>
>> $ git grep kvm_ softmmu/physmem.c
>> softmmu/physmem.c:752:assert(asidx == 0 || !kvm_enabled());
>> softmmu/physmem.c:1295:if (kvm_enabled())
>> softmmu/physmem.c:1296:kvm_flush_coalesced_mmio_buffer();
>> softmmu/physmem.c:1566:if (kvm_enabled()) {
>> softmmu/physmem.c:2046:if (kvm_enabled() && !kvm_has_sync_mmu()) {
>>
>> Thanks,
>>
>> Phil.
>>
>
> Hi Phil,
>
> indeed it is a juicy target for splitting things between TCG-only and non-TCG
> code,
> specifically as we discovered that we don't need any of the watchpoint stuff
> outside of TCG.
>
> I think I am tied up in the ARM code for a while,
> so if you are asking because you want to start there, just go ahead,
> I'll try to review, otherwise I'll get back to it (and to i386) later on.
No plan yet. I looked back at what I did + your patches to get
the big picture again, and had a look at my notes and wip patches.
Since you are making big changes/progress, I now prefer to check
upfront to avoid duplicated effort.
Regards,
Phil.
Re: [PATCH v16 00/23] i386 cleanup PART 2
Hi Philippe,
On 3/14/21 1:00 AM, Philippe Mathieu-Daudé wrote:
> Hi Claudio,
>
> On 2/4/21 5:39 PM, Claudio Fontana wrote:
>> v15 -> v16:
>>
>> * cpu: Move synchronize_from_tb() to tcg_ops:
>> - adjusted comments (Alex)
>>
>> * cpu: tcg_ops: move to tcg-cpu-ops.h, keep a pointer in CPUClass:
>> - remove forward decl. of AccelCPUClass, should be in a later patch. (Alex)
>> - simplified comment about tcg_ops in struct CPUClass (Alex)
>> - remove obsolete comment about ARM blocking TCGCPUOps from being const.
>> (Alex)
>>
>> * accel: replace struct CpusAccel with AccelOpsClass:
>> - reworded commit message to be clearer about the objective (Alex)
>>
>> * accel: introduce AccelCPUClass extending CPUClass
>> - reworded commit message to be clearer about the objective (Alex)
>>
>> * hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn:
>> - dropped this patch (Alex, Philippe)
>>
>> will try again later, also in the context of:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg686480.html
>>
>> * accel: introduce new accessor functions
>> - squashed comments in previous patch introducing accel-cpu.h. (Philippe)
>>
>> * accel-cpu: make cpu_realizefn return a bool
>> - split in two patches, separating the change to the phys_bits check
>> (Philippe)
>
> I am looking at this code:
>
> $ git grep tcg_ softmmu/physmem.c
> softmmu/physmem.c:153:static void
> tcg_log_global_after_sync(MemoryListener *listener);
> softmmu/physmem.c:154:static void tcg_commit(MemoryListener *listener);
> softmmu/physmem.c:161: * @tcg_as_listener: listener for tracking changes
> to the AddressSpace
> softmmu/physmem.c:167:MemoryListener tcg_as_listener;
> softmmu/physmem.c:590:static void tcg_iommu_unmap_notify(IOMMUNotifier
> *n, IOMMUTLBEntry *iotlb)
> softmmu/physmem.c:606:static void tcg_register_iommu_notifier(CPUState *cpu,
> softmmu/physmem.c:640:tcg_iommu_unmap_notify,
> softmmu/physmem.c:654:void tcg_iommu_free_notifier_list(CPUState *cpu)
> softmmu/physmem.c:668:void tcg_iommu_init_notifier_list(CPUState *cpu)
> softmmu/physmem.c:698:tcg_register_iommu_notifier(cpu, iommu_mr,
> iommu_idx);
> softmmu/physmem.c:761:if (tcg_enabled()) {
> softmmu/physmem.c:762:
> newas->tcg_as_listener.log_global_after_sync = tcg_log_global_after_sync;
> softmmu/physmem.c:763:newas->tcg_as_listener.commit = tcg_commit;
> softmmu/physmem.c:764:
> memory_listener_register(>tcg_as_listener, as);
> softmmu/physmem.c:891:assert(tcg_enabled());
> softmmu/physmem.c:904:if (cc->tcg_ops->adjust_watchpoint_address) {
> softmmu/physmem.c:906:addr =
> cc->tcg_ops->adjust_watchpoint_address(cpu, addr, len);
> softmmu/physmem.c:927:if (wp->flags & BP_CPU &&
> cc->tcg_ops->debug_check_watchpoint &&
> softmmu/physmem.c:928:
> !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
> softmmu/physmem.c:1004:assert(tcg_enabled());
> softmmu/physmem.c:1059:if (dirty && tcg_enabled()) {
> softmmu/physmem.c:1107:if (tcg_enabled()) {
> softmmu/physmem.c:2605:static void
> tcg_log_global_after_sync(MemoryListener *listener)
> softmmu/physmem.c:2634:cpuas = container_of(listener,
> CPUAddressSpace, tcg_as_listener);
> softmmu/physmem.c:2639:static void tcg_commit(MemoryListener *listener)
> softmmu/physmem.c:2644:assert(tcg_enabled());
> softmmu/physmem.c:2647:cpuas = container_of(listener,
> CPUAddressSpace, tcg_as_listener);
> softmmu/physmem.c:2700:assert(tcg_enabled());
> softmmu/physmem.c:3000:if (tcg_enabled()) {
>
> which reminded me the starter generic part of your effort
> (already merged).
>
> Do you have plans for this code?
>
> Similarly:
>
> $ git grep kvm_ softmmu/physmem.c
> softmmu/physmem.c:752:assert(asidx == 0 || !kvm_enabled());
> softmmu/physmem.c:1295:if (kvm_enabled())
> softmmu/physmem.c:1296:kvm_flush_coalesced_mmio_buffer();
> softmmu/physmem.c:1566:if (kvm_enabled()) {
> softmmu/physmem.c:2046:if (kvm_enabled() && !kvm_has_sync_mmu()) {
>
> Thanks,
>
> Phil.
>
Hi Phil,
indeed it is a juicy target for splitting things between TCG-only and non-TCG
code,
specifically as we discovered that we don't need any of the watchpoint stuff
outside of TCG.
I think I am tied up in the ARM code for a while,
so if you are asking because you want to start there, just go ahead,
I'll try to review, otherwise I'll get back to it (and to i386) later on.
Ciao,
Claudio
Re: [PATCH v16 00/23] i386 cleanup PART 2
Hi Claudio,
On 2/4/21 5:39 PM, Claudio Fontana wrote:
> v15 -> v16:
>
> * cpu: Move synchronize_from_tb() to tcg_ops:
> - adjusted comments (Alex)
>
> * cpu: tcg_ops: move to tcg-cpu-ops.h, keep a pointer in CPUClass:
> - remove forward decl. of AccelCPUClass, should be in a later patch. (Alex)
> - simplified comment about tcg_ops in struct CPUClass (Alex)
> - remove obsolete comment about ARM blocking TCGCPUOps from being const.
> (Alex)
>
> * accel: replace struct CpusAccel with AccelOpsClass:
> - reworded commit message to be clearer about the objective (Alex)
>
> * accel: introduce AccelCPUClass extending CPUClass
> - reworded commit message to be clearer about the objective (Alex)
>
> * hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn:
> - dropped this patch (Alex, Philippe)
>
> will try again later, also in the context of:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg686480.html
>
> * accel: introduce new accessor functions
> - squashed comments in previous patch introducing accel-cpu.h. (Philippe)
>
> * accel-cpu: make cpu_realizefn return a bool
> - split in two patches, separating the change to the phys_bits check
> (Philippe)
I am looking at this code:
$ git grep tcg_ softmmu/physmem.c
softmmu/physmem.c:153:static void
tcg_log_global_after_sync(MemoryListener *listener);
softmmu/physmem.c:154:static void tcg_commit(MemoryListener *listener);
softmmu/physmem.c:161: * @tcg_as_listener: listener for tracking changes
to the AddressSpace
softmmu/physmem.c:167:MemoryListener tcg_as_listener;
softmmu/physmem.c:590:static void tcg_iommu_unmap_notify(IOMMUNotifier
*n, IOMMUTLBEntry *iotlb)
softmmu/physmem.c:606:static void tcg_register_iommu_notifier(CPUState *cpu,
softmmu/physmem.c:640:tcg_iommu_unmap_notify,
softmmu/physmem.c:654:void tcg_iommu_free_notifier_list(CPUState *cpu)
softmmu/physmem.c:668:void tcg_iommu_init_notifier_list(CPUState *cpu)
softmmu/physmem.c:698:tcg_register_iommu_notifier(cpu, iommu_mr,
iommu_idx);
softmmu/physmem.c:761:if (tcg_enabled()) {
softmmu/physmem.c:762:
newas->tcg_as_listener.log_global_after_sync = tcg_log_global_after_sync;
softmmu/physmem.c:763:newas->tcg_as_listener.commit = tcg_commit;
softmmu/physmem.c:764:
memory_listener_register(>tcg_as_listener, as);
softmmu/physmem.c:891:assert(tcg_enabled());
softmmu/physmem.c:904:if (cc->tcg_ops->adjust_watchpoint_address) {
softmmu/physmem.c:906:addr =
cc->tcg_ops->adjust_watchpoint_address(cpu, addr, len);
softmmu/physmem.c:927:if (wp->flags & BP_CPU &&
cc->tcg_ops->debug_check_watchpoint &&
softmmu/physmem.c:928:
!cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
softmmu/physmem.c:1004:assert(tcg_enabled());
softmmu/physmem.c:1059:if (dirty && tcg_enabled()) {
softmmu/physmem.c:1107:if (tcg_enabled()) {
softmmu/physmem.c:2605:static void
tcg_log_global_after_sync(MemoryListener *listener)
softmmu/physmem.c:2634:cpuas = container_of(listener,
CPUAddressSpace, tcg_as_listener);
softmmu/physmem.c:2639:static void tcg_commit(MemoryListener *listener)
softmmu/physmem.c:2644:assert(tcg_enabled());
softmmu/physmem.c:2647:cpuas = container_of(listener,
CPUAddressSpace, tcg_as_listener);
softmmu/physmem.c:2700:assert(tcg_enabled());
softmmu/physmem.c:3000:if (tcg_enabled()) {
which reminded me the starter generic part of your effort
(already merged).
Do you have plans for this code?
Similarly:
$ git grep kvm_ softmmu/physmem.c
softmmu/physmem.c:752:assert(asidx == 0 || !kvm_enabled());
softmmu/physmem.c:1295:if (kvm_enabled())
softmmu/physmem.c:1296:kvm_flush_coalesced_mmio_buffer();
softmmu/physmem.c:1566:if (kvm_enabled()) {
softmmu/physmem.c:2046:if (kvm_enabled() && !kvm_has_sync_mmu()) {
Thanks,
Phil.
Re: [PATCH v16 00/23] i386 cleanup PART 2
On 2/4/21 6:39 AM, Claudio Fontana wrote: > Claudio Fontana (18): > target/riscv: remove CONFIG_TCG, as it is always TCG > accel/tcg: split TCG-only code from cpu_exec_realizefn > target/arm: do not use cc->do_interrupt for KVM directly > cpu: move cc->do_interrupt to tcg_ops > cpu: move cc->transaction_failed to tcg_ops > cpu: move do_unaligned_access to tcg_ops > physmem: make watchpoint checking code TCG-only > cpu: move adjust_watchpoint_address to tcg_ops > cpu: move debug_check_watchpoint to tcg_ops > cpu: tcg_ops: move to tcg-cpu-ops.h, keep a pointer in CPUClass > accel: extend AccelState and AccelClass to user-mode > accel: replace struct CpusAccel with AccelOpsClass > accel: introduce AccelCPUClass extending CPUClass > i386: split cpu accelerators from cpu.c, using AccelCPUClass > cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn > accel: introduce new accessor functions > target/i386: fix host_cpu_adjust_phys_bits error handling > accel-cpu: make cpu_realizefn return a bool > > Eduardo Habkost (5): > cpu: Introduce TCGCpuOperations struct > cpu: Move synchronize_from_tb() to tcg_ops > cpu: Move cpu_exec_* to tcg_ops > cpu: Move tlb_fill to tcg_ops > cpu: Move debug_excp_handler to tcg_ops Queuing patches 1-18 to tcg-next. r~
[PATCH v16 00/23] i386 cleanup PART 2
v15 -> v16: * cpu: Move synchronize_from_tb() to tcg_ops: - adjusted comments (Alex) * cpu: tcg_ops: move to tcg-cpu-ops.h, keep a pointer in CPUClass: - remove forward decl. of AccelCPUClass, should be in a later patch. (Alex) - simplified comment about tcg_ops in struct CPUClass (Alex) - remove obsolete comment about ARM blocking TCGCPUOps from being const. (Alex) * accel: replace struct CpusAccel with AccelOpsClass: - reworded commit message to be clearer about the objective (Alex) * accel: introduce AccelCPUClass extending CPUClass - reworded commit message to be clearer about the objective (Alex) * hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn: - dropped this patch (Alex, Philippe) will try again later, also in the context of: https://www.mail-archive.com/qemu-devel@nongnu.org/msg686480.html * accel: introduce new accessor functions - squashed comments in previous patch introducing accel-cpu.h. (Philippe) * accel-cpu: make cpu_realizefn return a bool - split in two patches, separating the change to the phys_bits check (Philippe) v14 -> v15: * change the TcgCpuOperations so that all fields of the struct are defined unconditionally, as per original patch by Eduardo, before moving them to a separate struct referenced by a pointer (Richard, Eduardo). * changed (c) year to 2021 * added a patch to make accel_cpu->cpu_realizefn return bool, and adapt host_cpu, kvm_cpu, hvf_cpu and tcg_cpu in i386 accordingly. Ultimately, consistently moving to a pattern of realize functions returning bool will require a rework of all devices. v13 -> v14: rebased on latest master. v12 -> v13: rebased on latest master. v11 -> v12: reordered patches and improved tcg_ops * reordered all TcgCpuOperations stuff so it is at the beginning * added patches for ARM-specific tcg ops debug_check_watchpoint and adjust_watchpoint_address * added a patch that puts a forward declared pointer in the struct, so as to reduce the change of misuse between common_ss and specific_ss code, and tidy up as a consequence all targets, by defining dedicated structs. v10 -> v11: split off PART 2, no further changes to PART 2 other than the split. v9 -> v10: minor tweaks and fixes * in "i386: split cpu accelerators from cpu.c", use kvm/kvm-cpu.c, hvf/hvf-cpu.c, tcg/tcg-cpu.c. Easier to understand compared to editing multiple cpu.c files, and matches the header files if needed (kvm-cpu.h). * in "accel: replace struct CpusAccel with AccelOpsClass", make it a bit more consistent, by naming the files defining the AccelOpsClass types "...-accel-ops.c" instead of the old naming "...-cpus.c". * in "cpu: move cc->transaction_failed to tcg_ops", protect with CONFIG_TCG the use of tcg_ops for hw/misc/jazz.c, #include "exec/memattrs.h" (Philippe, Eduardo) * in "cpu: Move synchronize_from_tb() to tcg_ops", #include "hw/core/cpu.h" (Philippe, Eduardo) do not remove the comment about struct TcgCpuOperations (Philippe) * in "accel/tcg: split TCG-only code from cpu_exec_realizefn", invert tcg_target_initialized set order (Alex) * in "i386: move TCG cpu class initialization out of helper.c", extract helper-tcg.h, tcg-cpu.c, and tcg-cpu.h directly into tcg/, avoiding the extra move later to tcg/ (Alex) v8 -> v9: move additional methods to CPUClass->tcg_ops do_unaligned_access, transaction_failed and do_interrupt. do_interrupt is a bit tricky, as the same code is reused (albeit not usually directly) for KVM under certain odd conditions. Change arm, as the only user of do_interrupt callback for KVM, to instead call the target function directly arm_do_interrupt. v7 -> v8: add missing CONFIG_TCGs, fix bugs * add the prerequisite patches for "3 tcg" at the beginning of the series for convenience (already reviewed, queued by RH). * add CONFIG_TCG to TCGCpuOperations and tcg_ops variable use * reduce the scope of the realizefn refactoring, do not introduce a separate cpu_accel_realize, and instead use the existing cpu_exec_realizefn, there is not enough benefit to introduce a new function. * fix bugs in user mode due to attempt to move the tcg_region_init() early, so it could be done just once in tcg_init() for both softmmu and user mode. Unfortunately it needs to remain deferred for user mode, as it needs to be done after prologue init and after the GUEST_BASE has been set. v6 -> v7: integrate TCGCpuOperations, refactored cpu_exec_realizefn * integrate TCGCpuOperations (Eduardo) Taken some refactoring from Eduardo for Tcg-only operations on CPUClass. * refactored cpu_exec_realizefn The other main change is a refactoring of cpu_exec_realizefn, directly linked to the effort of making many cpu_exec operations TCG-only (Eduardo series above): cpu_exec_realizefn is actually a TCG-only thing, with the exception of a couple things that can be done in base cpu code. This changes all targets realizefn, so I guess I have to Cc: the Multiverse? (Universe was
