Re: [PATCH 0/9] Replace remaining target_ulong in system-mode accel
[Trimming Cc list] 22.09.2023 13:45, Anton Johansson: On 21/09/23, Michael Tokarev wrote: Anton Johansson (9): accel/kvm: Widen pc/saved_insn for kvm_sw_breakpoint accel/hvf: Widen pc/saved_insn for hvf_sw_breakpoint target: Use vaddr for kvm_arch_[insert|remove]_hw_breakpoint target: Use vaddr for hvf_arch_[insert|remove]_hw_breakpoint Replace target_ulong with abi_ptr in cpu_[st|ld]*() include/exec: typedef abi_ptr to vaddr in softmmu include/exec: Widen tlb_hit/tlb_hit_page() accel/tcg: Widen address arg. in tlb_compare_set() accel/tcg: Update run_on_cpu_data static assert Pinging a relatively old patchset, - which fixes from here needs to go to stable-8.1? ... The rest of the patches can be delayed without issue. Now I'm confused. What do you mean "delayed"? Should the rest be picked up for 8.1.2 or 8.1.3 or maybe 8.1.4? The whole series has been accepted/applied to master, I asked which changes should be picked up for stable-8.1, - there's no delay involved, it is either picked up or not, either needed in stable or not. Yes, "Widen tlb_hit/tlb_hit_page()" fixes a known bug and I picked up that one, - unfortunately it missed 8.1.1 release. The question is about the other changes in this patch set, - do they fix other similar, not yet discovered, bugs in other places, or not fixing anything? Or should we delay picking them up until a bug is reported? :) Thank you for the changes and the reply! /mjt
[PATCH] hw/i386: changes towards enabling -Wshadow=local
Code changes that addresses all compiler complaints coming from enabling
-Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
other local variables or parameters. These makes the code confusing and/or adds
bugs that are difficult to catch.
CC: Markus Armbruster
CC: Philippe Mathieu-Daude
CC: m...@redhat.com
Message-Id: <87r0mqlf9x@pond.sub.org>
Signed-off-by: Ani Sinha
---
hw/i386/acpi-microvm.c | 12 ++--
hw/i386/intel_iommu.c | 8
hw/i386/pc.c | 1 -
hw/i386/x86.c | 2 --
4 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index a075360d85..6e4f8061eb 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -78,18 +78,18 @@ static void acpi_dsdt_add_virtio(Aml *scope,
hwaddr base = VIRTIO_MMIO_BASE + index * 512;
hwaddr size = 512;
-Aml *dev = aml_device("VR%02u", (unsigned)index);
-aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
-aml_append(dev, aml_name_decl("_UID", aml_int(index)));
-aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
+Aml *adev = aml_device("VR%02u", (unsigned)index);
+aml_append(adev, aml_name_decl("_HID", aml_string("LNRO0005")));
+aml_append(adev, aml_name_decl("_UID", aml_int(index)));
+aml_append(adev, aml_name_decl("_CCA", aml_int(1)));
Aml *crs = aml_resource_template();
aml_append(crs, aml_memory32_fixed(base, size, AML_READ_WRITE));
aml_append(crs,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, , 1));
-aml_append(dev, aml_name_decl("_CRS", crs));
-aml_append(scope, dev);
+aml_append(adev, aml_name_decl("_CRS", crs));
+aml_append(scope, adev);
}
}
}
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c0ce896668..c1fb69170f 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3770,9 +3770,9 @@ static void vtd_address_space_unmap(VTDAddressSpace *as,
IOMMUNotifier *n)
while (remain >= VTD_PAGE_SIZE) {
IOMMUTLBEvent event;
uint64_t mask = dma_aligned_pow2_mask(start, end, s->aw_bits);
-uint64_t size = mask + 1;
+uint64_t sz = mask + 1;
-assert(size);
+assert(sz);
event.type = IOMMU_NOTIFIER_UNMAP;
event.entry.iova = start;
@@ -3784,8 +3784,8 @@ static void vtd_address_space_unmap(VTDAddressSpace *as,
IOMMUNotifier *n)
memory_region_notify_iommu_one(n, );
-start += size;
-remain -= size;
+start += sz;
+remain -= sz;
}
assert(!remain);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3db0743f31..e7a233e886 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
if (machine->device_memory) {
uint64_t *val = g_malloc(sizeof(*val));
-PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
uint64_t res_mem_end = machine->device_memory->base;
if (!pcmc->broken_reserved_end) {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index f034df8bf6..b3d054889b 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, );
if (!cpu_slot) {
-MachineState *ms = MACHINE(x86ms);
-
x86_topo_ids_from_apicid(cpu->apic_id, _info, _ids);
error_setg(errp,
"Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
--
2.39.1
[PATCH v2] cxl/vendor: update niagara to only build on linux, add KConfig options
Niagara uses which presently limits its compatibility to
linux hosts. Change build to only build it on linux.
Add Kconfig file for skhynix directory, and make niagara depend on
CXL_MEM_DEVICE and LINUX. Add an explicit flag for niagara.
Signed-off-by: Gregory Price
---
hw/cxl/Kconfig| 2 ++
hw/cxl/vendor/Kconfig | 1 +
hw/cxl/vendor/skhynix/Kconfig | 4
hw/cxl/vendor/skhynix/meson.build | 2 +-
4 files changed, 8 insertions(+), 1 deletion(-)
create mode 100644 hw/cxl/vendor/Kconfig
create mode 100644 hw/cxl/vendor/skhynix/Kconfig
diff --git a/hw/cxl/Kconfig b/hw/cxl/Kconfig
index dd6c54b54d..88022008c7 100644
--- a/hw/cxl/Kconfig
+++ b/hw/cxl/Kconfig
@@ -1,3 +1,5 @@
+source vendor/Kconfig
+
config CXL
bool
default y if PCI_EXPRESS
diff --git a/hw/cxl/vendor/Kconfig b/hw/cxl/vendor/Kconfig
new file mode 100644
index 00..aa23bb051b
--- /dev/null
+++ b/hw/cxl/vendor/Kconfig
@@ -0,0 +1 @@
+source skhynix/Kconfig
diff --git a/hw/cxl/vendor/skhynix/Kconfig b/hw/cxl/vendor/skhynix/Kconfig
new file mode 100644
index 00..20942cffc2
--- /dev/null
+++ b/hw/cxl/vendor/skhynix/Kconfig
@@ -0,0 +1,4 @@
+config CXL_SKHYNIX_NIAGARA
+bool
+depends on CXL_MEM_DEVICE && LINUX
+default y if CXL_VENDOR
diff --git a/hw/cxl/vendor/skhynix/meson.build
b/hw/cxl/vendor/skhynix/meson.build
index 4e57db65f1..e3cb00e848 100644
--- a/hw/cxl/vendor/skhynix/meson.build
+++ b/hw/cxl/vendor/skhynix/meson.build
@@ -1 +1 @@
-system_ss.add(when: 'CONFIG_CXL_VENDOR', if_true: files('skhynix_niagara.c',))
+system_ss.add(when: 'CONFIG_CXL_SKHYNIX_NIAGARA', if_true:
files('skhynix_niagara.c',))
--
2.39.1
Re: [PATCH 6/9] vt82c686: Support machine-default audiodev with fallback
On Fri, 22 Sep 2023, Paolo Bonzini wrote:
On Fri, Sep 22, 2023 at 2:17 PM BALATON Zoltan wrote:
mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
mc->default_ram_size = 512 * MiB;
mc->default_ram_id = "ppc4xx.sdram";
+
+machine_add_audiodev_property(mc);
This hunk has nothing to do with vt82686 so probably should be in
previoius patch. Also sam460ex embedded sound part is not emulated and can
only use PCI sound cards. What this line is needed for?
No, this line shouldn't be there.
If every machine
now needs this call, can it be added in some generic machine init func in
hw/core/machine.c instead?
It is not needed by every machine, only by every machine that has
embedded sound.
I'm not sure about this whole series. Looks like it gets rid of 71 lines
but adding 158 and makes the users' life harder by requireing them to
specify drivers they may not even know about. What's the point? Is there
still a default to use the normally used audiodev for the platform or
people will now get errors and no sound unless they change their command
lines?
I think you're right, I should have sent this series without the last
two patches.
The first seven add more functionality, because they let you use
-audiodev for configuration of embedded boards. This is why they add
some lines of code.
The last two patches instead are the ones that make -audio or
-audiodev mandatory. They should be separate and they may not be a
good idea without something like "-audio default". And if no "-audio
default" is added, there is more code that can go (for example the
--audio-drv-list option to configure and CONFIG_AUDIO_DRIVERS).
I still don't see the point, because it already works without these
changes. With current master one can specify -audiodev for -M paegasos2
and it gives a warning but does the right thing and sets the audiodev for
via-ac97. I think the warning can be avoided by using -global to set the
via-ac97 audiodev property but since it picks up the -audiodev there's no
need to. Apart from the warning this is convenient for the user, what's
proposed in this series seems less so. What is the issue this series tries
to solve?
Regards,
BALATON Zoltan
Re: [PATCH 1/4] multifd: wait for channels_ready before sending sync
On Fri, Sep 22, 2023 at 01:06:53PM -0300, Fabiano Rosas wrote:
> Elena Ufimtseva writes:
>
> > In multifd_send_sync_main we need to wait for channels_ready
> > before submitting sync packet as the threads may still be sending
> > their previous pages.
> > There is also no need to check for channels_ready in the loop
> > before the wait for sem_sync, next iteration of sending pages
> > or another sync will start with waiting for channels_ready
> > semaphore.
> > Changes to commit 90b3cec351996dd8ef4eb847ad38607812c5e7f5
> > ("multifd: Fix the number of channels ready")
> >
> > Signed-off-by: Elena Ufimtseva
> > ---
> > migration/multifd.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 0f6b203877..e61e458151 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -595,6 +595,7 @@ int multifd_send_sync_main(QEMUFile *f)
> > }
> > }
> >
> > +qemu_sem_wait(_send_state->channels_ready);
> > /*
> > * When using zero-copy, it's necessary to flush the pages before any
> > of
> > * the pages can be sent again, so we'll make sure the new version of
> > the
> > @@ -630,7 +631,6 @@ int multifd_send_sync_main(QEMUFile *f)
> > for (i = 0; i < migrate_multifd_channels(); i++) {
> > MultiFDSendParams *p = _send_state->params[i];
> >
> > -qemu_sem_wait(_send_state->channels_ready);
> > trace_multifd_send_sync_main_wait(p->id);
> > qemu_sem_wait(>sem_sync);
>
> Please take a look at the series I just sent. Basically, I think we
> should wait on 'sem' for the number of existing channels and not just
> once per sync. Otherwise I think we'd hit the same issue this patch is
> trying to fix when we loop into the n+1 channels. I think the
> assert(!p->pending_job) in patch 3 helps prove that's more appropriate.
Thank you!
These patches make sense to me.
Agree on redundant sem_sync. Lets see what others think.
I will run some tests as well with your patches and spend
more time looking at [2/3] patch.
Elena U.
>
> Let me know what you think.
>
> Thanks
Re: [RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
On Fri, Sep 22, 2023 at 11:53:17AM -0300, Fabiano Rosas wrote:
> Commit d2026ee117 ("multifd: Fix the number of channels ready") moved
> the "post" of channels_ready to the start of the multifd_send_thread()
> loop and added a missing "wait" at multifd_send_sync_main(). While it
> does work, the placement of the wait goes against what the rest of the
> code does.
>
> The sequence at multifd_send_thread() is:
>
> qemu_sem_post(_send_state->channels_ready);
> qemu_sem_wait(>sem);
>
> if (flags & MULTIFD_FLAG_SYNC) {
> qemu_sem_post(>sem_sync);
> }
>
> Which means that the sending thread makes itself available
> (channels_ready) and waits for more work (sem). So the sequence in the
> migration thread should be to check if any channel is available
> (channels_ready), give it some work and set it off (sem):
>
> qemu_sem_wait(_send_state->channels_ready);
>
> qemu_sem_post(>sem);
> if (flags & MULTIFD_FLAG_SYNC) {
> qemu_sem_wait(>sem_sync);
> }
>
> The reason there's no deadlock today is that the migration thread
> enqueues the SYNC packet right before the wait on channels_ready and
> we end up taking advantage of the out-of-order post to sem:
>
> ...
> qemu_sem_post(>sem);
> }
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDSendParams *p = _send_state->params[i];
>
> qemu_sem_wait(_send_state->channels_ready);
> trace_multifd_send_sync_main_wait(p->id);
> qemu_sem_wait(>sem_sync);
> ...
>
> Move the channels_ready wait before the sem post to keep the sequence
> consistent. Also fix the error path to post to channels_ready and
> sem_sync in the correct order.
>
Thank you Fabiano,
Your solution is more complete. I also had in mind getting rid of
sem_sync.
With your second patch, this one could be merged with it?
> Signed-off-by: Fabiano Rosas
> ---
> migration/multifd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index a7c7a947e3..d626740f2f 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -618,6 +618,7 @@ int multifd_send_sync_main(QEMUFile *f)
>
> trace_multifd_send_sync_main_signal(p->id);
>
> +qemu_sem_wait(_send_state->channels_ready);
> qemu_mutex_lock(>mutex);
>
> if (p->quit) {
> @@ -635,7 +636,6 @@ int multifd_send_sync_main(QEMUFile *f)
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDSendParams *p = _send_state->params[i];
>
> -qemu_sem_wait(_send_state->channels_ready);
> trace_multifd_send_sync_main_wait(p->id);
> qemu_sem_wait(>sem_sync);
>
> @@ -763,8 +763,8 @@ out:
> * who pay attention to me.
> */
> if (ret != 0) {
> -qemu_sem_post(>sem_sync);
> qemu_sem_post(_send_state->channels_ready);
> +qemu_sem_post(>sem_sync);
Can this thread in this error case be woken up again between
these two qemu_sem_posts?
I see in other places p->quit is set to true before it.
Or maybe it should one more patch to make these consistent
as well.
Elena U.
> }
>
> qemu_mutex_lock(>mutex);
> --
> 2.35.3
>
Re: [PATCH] hw/timer/npcm7xx_timer: Prevent timer from counting down past zero
No. This patch does not address that issue and is not related. I was able
to reproduce it about 2/1000 iterations with and without this patch. I
will look into that issue separately.
-Chris
On Fri, Sep 22, 2023 at 11:24 AM Hao Wu wrote:
> Is this related to this error?
>
> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04903.html
>
> On Fri, Sep 22, 2023 at 11:14 AM Chris Rauer wrote:
>
>> The counter register is only 24-bits and counts down. If the timer is
>> running but the qtimer to reset it hasn't fired off yet, there is a chance
>> the regster read can return an invalid result.
>>
>> Signed-off-by: Chris Rauer
>> ---
>> hw/timer/npcm7xx_timer.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/timer/npcm7xx_timer.c b/hw/timer/npcm7xx_timer.c
>> index 32f5e021f8..a8bd93aeb2 100644
>> --- a/hw/timer/npcm7xx_timer.c
>> +++ b/hw/timer/npcm7xx_timer.c
>> @@ -138,6 +138,9 @@ static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer
>> *t, uint32_t count)
>> /* Convert a time interval in nanoseconds to a timer cycle count. */
>> static uint32_t npcm7xx_timer_ns_to_count(NPCM7xxTimer *t, int64_t ns)
>> {
>> +if (ns < 0) {
>> +return 0;
>> +}
>> return clock_ns_to_ticks(t->ctrl->clock, ns) /
>> npcm7xx_tcsr_prescaler(t->tcsr);
>> }
>> --
>> 2.42.0.515.g380fc7ccd1-goog
>>
>>
Re: [PATCH] intel_iommu: Fix shadow local variables on "size"
On Fri, Sep 22, 2023 at 12:04:10PM -0400, Peter Xu wrote:
> This patch fixes the warning of shadowed local variable:
>
> ../hw/i386/intel_iommu.c: In function ‘vtd_address_space_unmap’:
> ../hw/i386/intel_iommu.c:3773:18: warning: declaration of ‘size’ shadows a
> previous local [-Wshadow=compatible-local]
> 3773 | uint64_t size = mask + 1;
> | ^~~~
> ../hw/i386/intel_iommu.c:3747:12: note: shadowed declaration is here
> 3747 | hwaddr size, remain;
> |^~~~
>
> Cc: Jason Wang
> Cc: Eric Auger
> Cc: Michael S. Tsirkin
> Cc: Markus Armbruster
> Signed-off-by: Peter Xu
Reviewed-by: Michael S. Tsirkin
> ---
> hw/i386/intel_iommu.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c9961ef752..ae30c2b469 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
> PCIBus *bus,
> /* Unmap the whole range in the notifier's scope. */
> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> {
> -hwaddr size, remain;
> +hwaddr total, remain;
> hwaddr start = n->start;
> hwaddr end = n->end;
> IntelIOMMUState *s = as->iommu_state;
> @@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> }
>
> assert(start <= end);
> -size = remain = end - start + 1;
> +total = remain = end - start + 1;
>
> while (remain >= VTD_PAGE_SIZE) {
> IOMMUTLBEvent event;
> @@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
> VTD_PCI_SLOT(as->devfn),
> VTD_PCI_FUNC(as->devfn),
> - n->start, size);
> + n->start, total);
>
> map.iova = n->start;
> -map.size = size - 1; /* Inclusive */
> +map.size = total - 1; /* Inclusive */
> iova_tree_remove(as->iova_tree, map);
> }
>
> --
> 2.41.0
Re: [PATCH 2/4] hw/pci-bridge/cxl_upstream: Fix bandwidth entry base unit for SSLBIS
On 9/22/23 13:08, Michael Tokarev wrote:
> 04.09.2023 16:28, Jonathan Cameron:
>> From: Dave Jiang
>>
>> According to ACPI spec 6.5 5.2.28.4 System Locality Latency and Bandwidth
>> Information Structure, if the "Entry Base Unit" is 1024 for BW and the
>> matrix entry has the value of 100, the BW is 100 GB/s. So the
>> entry_base_unit should be changed from 1000 to 1024 given the comment notes
>> it's 16GB/s for .latency_bandwidth.
>>
>> Fixes: 882877fc359d ("hw/pci-bridge/cxl-upstream: Add a CDAT table access
>> DOE")
>> Signed-off-by: Dave Jiang
>> Signed-off-by: Jonathan Cameron
>> ---
>> hw/pci-bridge/cxl_upstream.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
>> index 9159f48a8c..2b9cf0cc97 100644
>> --- a/hw/pci-bridge/cxl_upstream.c
>> +++ b/hw/pci-bridge/cxl_upstream.c
>> @@ -262,7 +262,7 @@ static int build_cdat_table(CDATSubHeader ***cdat_table,
>> void *priv)
>> .length = sslbis_size,
>> },
>> .data_type = HMATLB_DATA_TYPE_ACCESS_BANDWIDTH,
>> - .entry_base_unit = 1000,
>> + .entry_base_unit = 1024,
>> },
>> };
>
> BTW, is this one stable-worthly? How it's been found, - due to some real
> issue or just by code review?
Code review. I was doing CXL CDAT parsing. So I guess I'm the first user. It's
small enough that it won't make much of a difference for the resulting computed
data. Mostly just correctness fixing.
>
> Thanks,
>
> /mjt
>
>
Re: [PATCH v2 0/4] Support dynamic MSI-X allocation
On Mon, 18 Sep 2023 05:45:03 -0400 Jing Liu wrote: > Changes since v1: > - v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg982842.html > - Revise Qemu to QEMU. (Cédric) > - Add g_free when failure of getting MSI-X irq info. (Cédric) > - Apply Cédric's Reviewed-by. (Cédric) > - Use g_autofree to automatically release. (Cédric) > - Remove the failure message in vfio_enable_msix_no_vec(). (Cédric) > > Changes since RFC v1: > - RFC v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg978637.html > - Revise the comments. (Alex) > - Report error of getting irq info and remove the trace of failure > case. (Alex, Cédric) > - Only store dynamic allocation flag as a bool type and test > accordingly. (Alex) > - Move dynamic allocation detection to vfio_msix_early_setup(). (Alex) > - Change the condition logic in vfio_msix_vector_do_use() that moving > the defer_kvm_irq_routing test out and create a common place to update > nr_vectors. (Alex) > - Consolidate the way of MSI-X enabling during device initialization and > interrupt restoring that uses fd = -1 trick. Create a function doing > that. (Alex) > > Before kernel v6.5, dynamic allocation of MSI-X interrupts was not > supported. QEMU therefore when allocating a new interrupt, should first > release all previously allocated interrupts (including disable of MSI-X) > and re-allocate all interrupts that includes the new one. > > The kernel series [1] adds the support of dynamic MSI-X allocation to > vfio-pci and uses the existing flag VFIO_IRQ_INFO_NORESIZE to guide user > space, that when dynamic MSI-X is supported the flag is cleared. > > This series makes the behavior for VFIO PCI devices when dynamic MSI-X > allocation is supported. When guest unmasks an interrupt, QEMU can > directly allocate an interrupt on host for this and has nothing to do > with the previously allocated ones. Therefore, host only allocates > interrupts for those unmasked (enabled) interrupts inside guest when > dynamic MSI-X allocation is supported by device. > > When guests enable MSI-X with all of the vectors masked, QEMU need match > the state to enable MSI-X with no vector enabled. During migration > restore, QEMU also need enable MSI-X first in dynamic allocation mode, > to avoid the guest unused vectors being allocated on host. To > consolidate them, we use vector 0 with an invalid fd to get MSI-X > enabled and create a common function for this. This is cleaner than > setting userspace triggering and immediately release. > > Any feedback is appreciated. > > Jing > > [1] https://lwn.net/Articles/931679/ > > Jing Liu (4): > vfio/pci: detect the support of dynamic MSI-X allocation > vfio/pci: enable vector on dynamic MSI-X allocation > vfio/pci: use an invalid fd to enable MSI-X > vfio/pci: enable MSI-X in interrupt restoring on dynamic allocation > > hw/vfio/pci.c| 121 +-- > hw/vfio/pci.h| 1 + > hw/vfio/trace-events | 2 +- > 3 files changed, 96 insertions(+), 28 deletions(-) > Some minor comments on 2/ but otherwise: Reviewed-by: Alex Williamson
[PATCH] qemu-nbd: changes towards enabling -Wshadow=local
Address all compiler complaints from -Wshadow in qemu-nbd. Several
instances of 'int ret' became shadows when commit 4fbec260 added 'ret'
at a higher scope in main. More interesting was the 'void *ret'
capturing the result of a pthread; where we were conceptually doing
'(void*)(intptr_t)EXIT_FAILURE != NULL' which just feels wrong (even
though it happens to compile correctly), so it was worth a better
cleanup.
Signed-off-by: Eric Blake
---
I'm happy to let Markus collect this with the growing pile on
shadow-next, instead of going through my NBD tree.
qemu-nbd.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 30eeb6f3c75..9bc410c6c56 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -937,7 +937,6 @@ int main(int argc, char **argv)
g_autoptr(GError) err = NULL;
int stderr_fd[2];
pid_t pid;
-int ret;
if (!g_unix_open_pipe(stderr_fd, FD_CLOEXEC, )) {
error_report("Error setting up communication pipe: %s",
@@ -1170,7 +1169,6 @@ int main(int argc, char **argv)
if (opts.device) {
#if HAVE_NBD_DEVICE
-int ret;
ret = pthread_create(_thread, NULL, nbd_client_thread, );
if (ret != 0) {
error_report("Failed to create client thread: %s", strerror(ret));
@@ -1217,9 +1215,10 @@ int main(int argc, char **argv)
qemu_opts_del(sn_opts);
if (opts.device) {
-void *ret;
-pthread_join(client_thread, );
-exit(ret != NULL);
+void *result;
+pthread_join(client_thread, );
+ret = (intptr_t)result;
+exit(ret);
} else {
exit(EXIT_SUCCESS);
}
--
2.41.0
Re: [PATCH v2 2/4] vfio/pci: enable vector on dynamic MSI-X allocation
On Mon, 18 Sep 2023 05:45:05 -0400
Jing Liu wrote:
> The vector_use callback is used to enable vector that is unmasked in
> guest. The kernel used to only support static MSI-X allocation. When
> allocating a new interrupt using "static MSI-X allocation" kernels,
> QEMU first disables all previously allocated vectors and then
> re-allocates all including the new one. The nr_vectors of VFIOPCIDevice
> indicates that all vectors from 0 to nr_vectors are allocated (and may
> be enabled), which is used to to loop all the possibly used vectors
^^ ^^
s/to to/to/
> When, e.g., disabling MSI-X interrupts.
>
> Extend the vector_use function to support dynamic MSI-X allocation when
> host supports the capability. QEMU therefore can individually allocate
> and enable a new interrupt without affecting others or causing interrupts
> lost during runtime.
>
> Utilize nr_vectors to calculate the upper bound of enabled vectors in
> dynamic MSI-X allocation mode since looping all msix_entries_nr is not
> efficient and unnecessary.
>
> Signed-off-by: Jing Liu
> Tested-by: Reinette Chatre
> ---
> Changes since v1:
> - Revise Qemu to QEMU.
>
> Changes since RFC v1:
> - Test vdev->msix->noresize to identify the allocation mode. (Alex)
> - Move defer_kvm_irq_routing test out and update nr_vectors in a
> common place before vfio_enable_vectors(). (Alex)
> - Revise the comments. (Alex)
> ---
> hw/vfio/pci.c | 44 +++-
> 1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 60654ca28ab8..84987e46fd7a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -470,6 +470,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> VFIOMSIVector *vector;
> int ret;
> +int old_nr_vecs = vdev->nr_vectors;
Minor suggestion, it reads slightly better below if this were something
like:
bool resizing = !!(vdev->nr_vectors < nr + 1);
Then use the bool in place of the nr+1 tests below. Thanks,
Alex
>
> trace_vfio_msix_vector_do_use(vdev->vbasedev.name, nr);
>
> @@ -512,33 +513,42 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> }
>
> /*
> - * We don't want to have the host allocate all possible MSI vectors
> - * for a device if they're not in use, so we shutdown and incrementally
> - * increase them as needed.
> + * When dynamic allocation is not supported, we don't want to have the
> + * host allocate all possible MSI vectors for a device if they're not
> + * in use, so we shutdown and incrementally increase them as needed.
> + * nr_vectors represents the total number of vectors allocated.
> + *
> + * When dynamic allocation is supported, let the host only allocate
> + * and enable a vector when it is in use in guest. nr_vectors represents
> + * the upper bound of vectors being enabled (but not all of the ranges
> + * is allocated or enabled).
> */
> if (vdev->nr_vectors < nr + 1) {
> vdev->nr_vectors = nr + 1;
> -if (!vdev->defer_kvm_irq_routing) {
> +}
> +
> +if (!vdev->defer_kvm_irq_routing) {
> +if (vdev->msix->noresize && (old_nr_vecs < nr + 1)) {
> vfio_disable_irqindex(>vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
> ret = vfio_enable_vectors(vdev, true);
> if (ret) {
> error_report("vfio: failed to enable vectors, %d", ret);
> }
> -}
> -} else {
> -Error *err = NULL;
> -int32_t fd;
> -
> -if (vector->virq >= 0) {
> -fd = event_notifier_get_fd(>kvm_interrupt);
> } else {
> -fd = event_notifier_get_fd(>interrupt);
> -}
> +Error *err = NULL;
> +int32_t fd;
>
> -if (vfio_set_irq_signaling(>vbasedev,
> - VFIO_PCI_MSIX_IRQ_INDEX, nr,
> - VFIO_IRQ_SET_ACTION_TRIGGER, fd, ))
> {
> -error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +if (vector->virq >= 0) {
> +fd = event_notifier_get_fd(>kvm_interrupt);
> +} else {
> +fd = event_notifier_get_fd(>interrupt);
> +}
> +
> +if (vfio_set_irq_signaling(>vbasedev,
> + VFIO_PCI_MSIX_IRQ_INDEX, nr,
> + VFIO_IRQ_SET_ACTION_TRIGGER, fd,
> )) {
> +error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +}
> }
> }
>
Re: [Stable-8.1.1 11/34] softmmu: Assert data in bounds in iotlb_to_section
20.09.2023 12:23, Alex Bennée: .. I wonder if I should keep 0d58c6606 for 8.1.1 (the deadline is tomorrow).. Unfortunately 0d58c is not the full fix, it papered over one crack but revealed others. It might be leading to a false sense of security. So I would argue: - keep the assert - better to fail early than to fail later in a hard to understand way - toss a coin for the 0d58c66 fix, if we include it we may end up reverting later once we have the "complete" fix but at least its slightly better for x86 while definitely breaking MIPS Heh. I've read this email just now, way after 8.1.1 has been tagged and the announcement sent. I haven't included 0d58c66 for now, without tossing coins - just to be on-par with 8.1.0, or else it is confusing at best (which stable releases brings with new issues). This whole thing is definitely worth a 8.1.2 once the fix is in. Meanwhile I pushed qemu with 0d58c66 and the "always require can_do_io" patchset to debian, - this one fixed all regressions so far. https://salsa.debian.org/qemu-team/qemu/-/tree/debian/1%258.1.0+ds-6/debian/patches/always-can-do-io-1866 https://gitlab.com/mjt0k/qemu/-/commits/staging-8.1-always-require-can_do_io/ Thank you for the thoughts, much apprecated! /mjt
Re: [PATCH 2/4] hw/pci-bridge/cxl_upstream: Fix bandwidth entry base unit for SSLBIS
04.09.2023 16:28, Jonathan Cameron:
From: Dave Jiang
According to ACPI spec 6.5 5.2.28.4 System Locality Latency and Bandwidth
Information Structure, if the "Entry Base Unit" is 1024 for BW and the
matrix entry has the value of 100, the BW is 100 GB/s. So the
entry_base_unit should be changed from 1000 to 1024 given the comment notes
it's 16GB/s for .latency_bandwidth.
Fixes: 882877fc359d ("hw/pci-bridge/cxl-upstream: Add a CDAT table access DOE")
Signed-off-by: Dave Jiang
Signed-off-by: Jonathan Cameron
---
hw/pci-bridge/cxl_upstream.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index 9159f48a8c..2b9cf0cc97 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -262,7 +262,7 @@ static int build_cdat_table(CDATSubHeader ***cdat_table,
void *priv)
.length = sslbis_size,
},
.data_type = HMATLB_DATA_TYPE_ACCESS_BANDWIDTH,
-.entry_base_unit = 1000,
+.entry_base_unit = 1024,
},
};
BTW, is this one stable-worthly? How it's been found, - due to some real
issue or just by code review?
Thanks,
/mjt
Re: [PATCH v4 20/21] i386: Use CPUCacheInfo.share_level to encode CPUID[0x8000001D].EAX[bits 25:14]
On 9/14/2023 2:21 AM, Zhao Liu wrote:
From: Zhao Liu
CPUID[0x801D].EAX[bits 25:14] NumSharingCache: number of logical
processors sharing cache.
The number of logical processors sharing this cache is
NumSharingCache + 1.
After cache models have topology information, we can use
CPUCacheInfo.share_level to decide which topology level to be encoded
into CPUID[0x801D].EAX[bits 25:14].
Signed-off-by: Zhao Liu
Reviewed-by: Babu Moger
---
Changes since v3:
* Explain what "CPUID[0x801D].EAX[bits 25:14]" means in the commit
message. (Babu)
Changes since v1:
* Use cache->share_level as the parameter in
max_processor_ids_for_cache().
---
target/i386/cpu.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index bc28c59df089..3bed823dc3b7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -482,20 +482,12 @@ static void encode_cache_cpuid801d(CPUCacheInfo
*cache,
uint32_t *eax, uint32_t *ebx,
uint32_t *ecx, uint32_t *edx)
{
-uint32_t num_sharing_cache;
assert(cache->size == cache->line_size * cache->associativity *
cache->partitions * cache->sets);
*eax = CACHE_TYPE(cache->type) | CACHE_LEVEL(cache->level) |
(cache->self_init ? CACHE_SELF_INIT_LEVEL : 0);
-
-/* L3 is shared among multiple cores */
-if (cache->level == 3) {
-num_sharing_cache = 1 << apicid_die_offset(topo_info);
-} else {
-num_sharing_cache = 1 << apicid_core_offset(topo_info);
-}
-*eax |= (num_sharing_cache - 1) << 14;
+*eax |= max_processor_ids_for_cache(topo_info, cache->share_level) << 14;
assert(cache->line_size > 0);
assert(cache->partitions > 0);
Re: [PATCH v4 19/21] i386: Use offsets get NumSharingCache for CPUID[0x8000001D].EAX[bits 25:14]
On 9/14/2023 2:21 AM, Zhao Liu wrote:
From: Zhao Liu
The commit 8f4202fb1080 ("i386: Populate AMD Processor Cache Information
for cpuid 0x801D") adds the cache topology for AMD CPU by encoding
the number of sharing threads directly.
From AMD's APM, NumSharingCache (CPUID[0x801D].EAX[bits 25:14])
means [1]:
The number of logical processors sharing this cache is the value of
this field incremented by 1. To determine which logical processors are
sharing a cache, determine a Share Id for each processor as follows:
ShareId = LocalApicId >> log2(NumSharingCache+1)
Logical processors with the same ShareId then share a cache. If
NumSharingCache+1 is not a power of two, round it up to the next power
of two.
From the description above, the calculation of this field should be same
as CPUID[4].EAX[bits 25:14] for Intel CPUs. So also use the offsets of
APIC ID to calculate this field.
[1]: APM, vol.3, appendix.E.4.15 Function 8000_001Dh--Cache Topology
Information
Signed-off-by: Zhao Liu
Reviewed-by: Babu Moger
---
Changes since v3:
* Rewrite the subject. (Babu)
* Delete the original "comment/help" expression, as this behavior is
confirmed for AMD CPUs. (Babu)
* Rename "num_apic_ids" (v3) to "num_sharing_cache" to match spec
definition. (Babu)
Changes since v1:
* Rename "l3_threads" to "num_apic_ids" in
encode_cache_cpuid801d(). (Yanan)
* Add the description of the original commit and add Cc.
---
target/i386/cpu.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5d066107d6ce..bc28c59df089 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -482,7 +482,7 @@ static void encode_cache_cpuid801d(CPUCacheInfo *cache,
uint32_t *eax, uint32_t *ebx,
uint32_t *ecx, uint32_t *edx)
{
-uint32_t l3_threads;
+uint32_t num_sharing_cache;
assert(cache->size == cache->line_size * cache->associativity *
cache->partitions * cache->sets);
@@ -491,13 +491,11 @@ static void encode_cache_cpuid801d(CPUCacheInfo *cache,
/* L3 is shared among multiple cores */
if (cache->level == 3) {
-l3_threads = topo_info->modules_per_die *
- topo_info->cores_per_module *
- topo_info->threads_per_core;
-*eax |= (l3_threads - 1) << 14;
+num_sharing_cache = 1 << apicid_die_offset(topo_info);
} else {
-*eax |= ((topo_info->threads_per_core - 1) << 14);
+num_sharing_cache = 1 << apicid_core_offset(topo_info);
}
+*eax |= (num_sharing_cache - 1) << 14;
assert(cache->line_size > 0);
assert(cache->partitions > 0);
Re: [PATCH 0/4] aspeed: Clean up local variable shadowing
On 9/22/23 20:20, Philippe Mathieu-Daudé wrote: On 22/9/23 17:59, Cédric Le Goater wrote: Hello, Here are cleanups for local variable shadowing warnings in aspeed models. Joel, Andrew, Could you please double check patch 4 ? Could Markus' MAKE_IDENTFIER() help there? ah ! you typed too fast and I also read too fast, as : MARKUS_IDENTIFIER() and I liked it :) but what is MAKE_IDENTIFIER ? really, please explain. Thanks, C.
Re: Help wanted for enabling -Wshadow=local
On Fri, Sep 22, 2023 at 11:49 AM Peter Maydell
wrote:
> On Fri, 22 Sept 2023 at 18:43, Daniel Henrique Barboza
> wrote:
> > Can you publish your branch with the current -Wshadow=local patches in
> > gitlab/github? I'm hitting (and fixing) a lot of errors that aren't
> listed
> > here, meaning they're either fixed already in your local branch or needs
> to
> > be fixed.
>
> Markus sent an email with the git branch, but it doesn't seem to have
> reached the list, perhaps because it also included a 10,000 line
> build log and probably hit the length limit... Anyway, to quote
> him from that email (which I got because of a direct CC):
>
> > Pushed to https://repo.or.cz/qemu/armbru.git branch shadow-next. I'll
> > keep collecting shadow patches there, and I'll rebase as needed.
>
I have 3 changes for bsd-user. Two are trivial, hardly worth commenting on.
The third one, though, makes me ask the question: When should we pass in
cpu_env to functions and when should we use the global value?
I have a lot of changes that look like:
-static inline abi_long do_freebsd_thr_exit(CPUArchState *cpu_env,
+static inline abi_long do_freebsd_thr_exit(CPUArchState *env,
abi_ulong tid_addr)
{
-CPUState *cpu = env_cpu(cpu_env);
+CPUState *cpu = env_cpu(env);
TaskState *ts;
...
env>
Should I just drop the arg, or do the arg rename? Or "Gee, Warner, that
really depends since it's context sensitive" in which case I'll just post a
review to the list.
Warner
Re: [PATCH 4/4] multifd: reset next_packet_len after sending pages
Elena Ufimtseva writes: > Sometimes multifd sends just sync packet with no pages > (normal_num is 0). In this case the old value is being > preserved and being accounted for while only packet_len > is being transferred. > Reset it to 0 after sending and accounting for. > Usually you'd finish your commit message here with your Signed off tag and the comments below would be included after the following --- sign. That way we can merge this patch without bring the unrelated discussion along. > TODO: Fix the same packet ids in the stream. > with this patch, there is still an issue with the duplicated > packets ids being sent (with different number of pages/flags). > See in below multifd_send trace (before this change): > multifd_send 394.774 pid=55477 id=0x1 packet_num=0x6f0 normal=0x57 flags=0x1 > next_packet_size=0x57000 > multifd_send 181.244 pid=55477 id=0x1 packet_num=0x6f0 normal=0x0 flags=0x0 > next_packet_size=0x57000 > > With this commit there are still duplicated packets, but since no pages > are being sent with sync flag set, next_packet_size is 0: > multifd_send 27.814 pid=18602 id=0x1 packet_num=0x574 normal=0x7b flags=0x1 > next_packet_size=0x7b000 > multifd_send 136054.792 pid=18602 id=0x1 packet_num=0x574 normal=0x0 > flags=0x0 next_packet_size=0x0 > If there is a suggestion how to fix this properly, I will be > glad to use it. What is going on here? Is it that we're incrementing the multifd_send_state->packet_num under different locks? Because p->mutex is per-channel? You could try turning that into an atomic operation instead.
Re: [PATCH 4/4] aspeed/timer: Clean up local variable shadowing
On 22/9/23 17:59, Cédric Le Goater wrote:
commit 8137355e850f ("aspeed/timer: Fix behaviour running Linux")
introduced a MAX() expression to calculate the next timer deadline :
return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
The second MAX() is not necessary since the compared values are an
unsigned and 0. Simply remove it and fix warning :
../hw/timer/aspeed_timer.c: In function ‘calculate_next’:
../include/qemu/osdep.h:396:31: warning: declaration of ‘_a’ shadows a
previous local [-Wshadow=compatible-local]
396 | typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
| ^~
../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
170 | next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
|^~~
../hw/timer/aspeed_timer.c:170:16: note: in expansion of macro ‘MAX’
170 | next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
|^~~
/home/legoater/work/qemu/qemu-aspeed.git/include/qemu/osdep.h:396:31: note:
shadowed declaration is here
396 | typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
| ^~
../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
170 | next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
|^~~
Cc: Joel Stanley
Cc: Andrew Jeffery
Signed-off-by: Cédric Le Goater
---
hw/timer/aspeed_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 4/4] multifd: reset next_packet_len after sending pages
Elena Ufimtseva writes: > Sometimes multifd sends just sync packet with no pages > (normal_num is 0). In this case the old value is being > preserved and being accounted for while only packet_len > is being transferred. > Reset it to 0 after sending and accounting for. > > TODO: Fix the same packet ids in the stream. > with this patch, there is still an issue with the duplicated > packets ids being sent (with different number of pages/flags). > See in below multifd_send trace (before this change): > multifd_send 394.774 pid=55477 id=0x1 packet_num=0x6f0 normal=0x57 flags=0x1 > next_packet_size=0x57000 > multifd_send 181.244 pid=55477 id=0x1 packet_num=0x6f0 normal=0x0 flags=0x0 > next_packet_size=0x57000 > > With this commit there are still duplicated packets, but since no pages > are being sent with sync flag set, next_packet_size is 0: > multifd_send 27.814 pid=18602 id=0x1 packet_num=0x574 normal=0x7b flags=0x1 > next_packet_size=0x7b000 > multifd_send 136054.792 pid=18602 id=0x1 packet_num=0x574 normal=0x0 > flags=0x0 next_packet_size=0x0 > If there is a suggestion how to fix this properly, I will be > glad to use it. > > Signed-off-by: Elena Ufimtseva > --- > migration/multifd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/migration/multifd.c b/migration/multifd.c > index 3281397b18..8b4e26051b 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -730,6 +730,7 @@ static void *multifd_send_thread(void *opaque) > p->next_packet_size + p->packet_len); > stat64_add(_stats.transferred, > p->next_packet_size + p->packet_len); > +p->next_packet_size = 0; > qemu_mutex_lock(>mutex); > p->pending_job--; > qemu_mutex_unlock(>mutex); Reviewed-by: Fabiano Rosas
Re: [PATCH] hw/timer/npcm7xx_timer: Prevent timer from counting down past zero
Is this related to this error?
https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg04903.html
On Fri, Sep 22, 2023 at 11:14 AM Chris Rauer wrote:
> The counter register is only 24-bits and counts down. If the timer is
> running but the qtimer to reset it hasn't fired off yet, there is a chance
> the regster read can return an invalid result.
>
> Signed-off-by: Chris Rauer
> ---
> hw/timer/npcm7xx_timer.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/timer/npcm7xx_timer.c b/hw/timer/npcm7xx_timer.c
> index 32f5e021f8..a8bd93aeb2 100644
> --- a/hw/timer/npcm7xx_timer.c
> +++ b/hw/timer/npcm7xx_timer.c
> @@ -138,6 +138,9 @@ static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer
> *t, uint32_t count)
> /* Convert a time interval in nanoseconds to a timer cycle count. */
> static uint32_t npcm7xx_timer_ns_to_count(NPCM7xxTimer *t, int64_t ns)
> {
> +if (ns < 0) {
> +return 0;
> +}
> return clock_ns_to_ticks(t->ctrl->clock, ns) /
> npcm7xx_tcsr_prescaler(t->tcsr);
> }
> --
> 2.42.0.515.g380fc7ccd1-goog
>
>
Re: [PATCH 2/4] aspeed: Clean up local variable shadowing
On 22/9/23 17:59, Cédric Le Goater wrote: Remove superfluous local 'irq' variables and use the one define at the top of the routine. This fixes warnings in aspeed_soc_ast2600_realize() such as : ../hw/arm/aspeed_ast2600.c: In function ‘aspeed_soc_ast2600_realize’: ../hw/arm/aspeed_ast2600.c:420:18: warning: declaration of ‘irq’ shadows a previous local [-Wshadow=compatible-local] 420 | qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i); | ^~~ ../hw/arm/aspeed_ast2600.c:312:14: note: shadowed declaration is here 312 | qemu_irq irq; | ^~~ Signed-off-by: Cédric Le Goater --- hw/arm/aspeed_ast2600.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 1/4] aspeed/i2c: Clean up local variable shadowing
On 22/9/23 17:59, Cédric Le Goater wrote: Remove superfluous local 'data' variable and use the one define at the top of the routine. This fixes : ../hw/i2c/aspeed_i2c.c: In function ‘aspeed_i2c_bus_recv’: ../hw/i2c/aspeed_i2c.c:315:17: warning: declaration of ‘data’ shadows a previous local [-Wshadow=compatible-local] 315 | uint8_t data; | ^~~~ ../hw/i2c/aspeed_i2c.c:288:13: note: shadowed declaration is here 288 | uint8_t data; | ^~~~ Signed-off-by: Cédric Le Goater --- hw/i2c/aspeed_i2c.c | 1 - 1 file changed, 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 0/4] aspeed: Clean up local variable shadowing
On 22/9/23 17:59, Cédric Le Goater wrote: Hello, Here are cleanups for local variable shadowing warnings in aspeed models. Joel, Andrew, Could you please double check patch 4 ? Could Markus' MAKE_IDENTFIER() help there? Thanks, C. Cédric Le Goater (4): aspeed/i2c: Clean up local variable shadowing aspeed: Clean up local variable shadowing aspeed/i3c: Rename variable shadowing a local aspeed/timer: Clean up local variable shadowing hw/arm/aspeed_ast2600.c | 10 +- hw/i2c/aspeed_i2c.c | 1 - hw/misc/aspeed_i3c.c| 6 +++--- hw/timer/aspeed_timer.c | 2 +- 4 files changed, 9 insertions(+), 10 deletions(-)
Re: [PATCH 1/2] crypto: remove shadowed 'ret' variable
On 22/9/23 18:06, Daniel P. Berrangé wrote: Both instances of 'ret' are used to store a gnutls API return code. Signed-off-by: Daniel P. Berrangé --- crypto/tls-cipher-suites.c | 1 - 1 file changed, 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH] intel_iommu: Fix shadow local variables on "size"
On 22/9/23 18:04, Peter Xu wrote: This patch fixes the warning of shadowed local variable: ../hw/i386/intel_iommu.c: In function ‘vtd_address_space_unmap’: ../hw/i386/intel_iommu.c:3773:18: warning: declaration of ‘size’ shadows a previous local [-Wshadow=compatible-local] 3773 | uint64_t size = mask + 1; | ^~~~ ../hw/i386/intel_iommu.c:3747:12: note: shadowed declaration is here 3747 | hwaddr size, remain; |^~~~ Cc: Jason Wang Cc: Eric Auger Cc: Michael S. Tsirkin Cc: Markus Armbruster Signed-off-by: Peter Xu --- hw/i386/intel_iommu.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH] tests/qtest/m48t59-test: Silence compiler warning with -Wshadow
On 22/9/23 18:37, Thomas Huth wrote: When compiling this file with -Wshadow=local , we get: ../tests/qtest/m48t59-test.c: In function ‘bcd_check_time’: ../tests/qtest/m48t59-test.c:195:17: warning: declaration of ‘s’ shadows a previous local [-Wshadow=local] 195 | long t, s; | ^ ../tests/qtest/m48t59-test.c:158:17: note: shadowed declaration is here 158 | QTestState *s = m48t59_qtest_start(); | ^ Rename the QTestState variable to "qts" which is the common naming for such a variable in other tests. Reported-by: Markus Armbruster Signed-off-by: Thomas Huth --- tests/qtest/m48t59-test.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH] hw/timer/npcm7xx_timer: Prevent timer from counting down past zero
The counter register is only 24-bits and counts down. If the timer is
running but the qtimer to reset it hasn't fired off yet, there is a chance
the regster read can return an invalid result.
Signed-off-by: Chris Rauer
---
hw/timer/npcm7xx_timer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/timer/npcm7xx_timer.c b/hw/timer/npcm7xx_timer.c
index 32f5e021f8..a8bd93aeb2 100644
--- a/hw/timer/npcm7xx_timer.c
+++ b/hw/timer/npcm7xx_timer.c
@@ -138,6 +138,9 @@ static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer *t,
uint32_t count)
/* Convert a time interval in nanoseconds to a timer cycle count. */
static uint32_t npcm7xx_timer_ns_to_count(NPCM7xxTimer *t, int64_t ns)
{
+if (ns < 0) {
+return 0;
+}
return clock_ns_to_ticks(t->ctrl->clock, ns) /
npcm7xx_tcsr_prescaler(t->tcsr);
}
--
2.42.0.515.g380fc7ccd1-goog
Re: [PATCH 3/4] multifd: fix counters in multifd_send_thread
Elena Ufimtseva writes: > Previous commit cbec7eb76879d419e7dbf531ee2506ec0722e825 > "migration/multifd: Compute transferred bytes correctly" > removed accounting for packet_len in non-rdma > case, but the next_packet_size only accounts for pages, not for > the header packet (normal_pages * PAGE_SIZE) that is being sent > as iov[0]. The packet_len part should be added to account for > the size of MultiFDPacket and the array of the offsets. > > Signed-off-by: Elena Ufimtseva I don't really understand the purpose of next_packet_size, but the accounting and explanation seem correct. Reviewed-by: Fabiano Rosas
Re: Help wanted for enabling -Wshadow=local
On Fri, 22 Sept 2023 at 18:43, Daniel Henrique Barboza wrote: > Can you publish your branch with the current -Wshadow=local patches in > gitlab/github? I'm hitting (and fixing) a lot of errors that aren't listed > here, meaning they're either fixed already in your local branch or needs to > be fixed. Markus sent an email with the git branch, but it doesn't seem to have reached the list, perhaps because it also included a 10,000 line build log and probably hit the length limit... Anyway, to quote him from that email (which I got because of a direct CC): > Pushed to https://repo.or.cz/qemu/armbru.git branch shadow-next. I'll > keep collecting shadow patches there, and I'll rebase as needed. thanks -- PMM
Re: [PATCH] tests/qtest/m48t59-test: Silence compiler warning with -Wshadow
On Fri, Sep 22, 2023 at 06:37:42PM +0200, Thomas Huth wrote: > When compiling this file with -Wshadow=local , we get: > > ../tests/qtest/m48t59-test.c: In function ‘bcd_check_time’: > ../tests/qtest/m48t59-test.c:195:17: warning: declaration of ‘s’ > shadows a previous local [-Wshadow=local] > 195 | long t, s; > | ^ > ../tests/qtest/m48t59-test.c:158:17: note: shadowed declaration is here > 158 | QTestState *s = m48t59_qtest_start(); > | ^ > > Rename the QTestState variable to "qts" which is the common > naming for such a variable in other tests. > > Reported-by: Markus Armbruster > Signed-off-by: Thomas Huth > --- > tests/qtest/m48t59-test.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 2/4] migration: check for rate_limit_max for RATE_LIMIT_DISABLED
Elena Ufimtseva writes: > In migration rate limiting atomic operations are used > to read the rate limit variables and transferred bytes and > they are expensive. Check first if rate_limit_max is equal > to RATE_LIMIT_DISABLED and return false immediately if so. > > Signed-off-by: Elena Ufimtseva > --- > migration/migration-stats.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/migration/migration-stats.c b/migration/migration-stats.c > index 095d6d75bb..abc31483d5 100644 > --- a/migration/migration-stats.c > +++ b/migration/migration-stats.c > @@ -24,14 +24,14 @@ bool migration_rate_exceeded(QEMUFile *f) > return true; > } > > -uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start); > -uint64_t rate_limit_current = migration_transferred_bytes(f); There's a qemu_fflush() hiding inside migration_transferred_bytes(). It currently always flushes if we haven't detected an error in the file. After this patch we will stop flushing at this point if ratelimiting is disabled. You might want to add that information to the commit message to make it easier to track if this ends up causing a regression. Reviewed-by: Fabiano Rosas
Re: Help wanted for enabling -Wshadow=local
> On 22-Sep-2023, at 3:07 PM, Markus Armbruster wrote: > > X86 Machines > > PC > M: Michael S. Tsirkin > M: Marcel Apfelbaum >hw/i386/acpi-build.c(*3*) >hw/i386/acpi-microvm.c(*2*) >hw/i386/intel_iommu.c(*3*) >hw/i386/pc.c(*2*) >hw/i386/x86.c(*2*) Since I already took care of api-build, I will take care of these next unless someone is already looking at it.
Re: [PATCH v6] hw/i386/pc: improve physical address space bound check for 32-bit x86 systems
On 22/9/23 18:04, Ani Sinha wrote:
32-bit x86 systems do not have a reserved memory for hole64. On those 32-bit
systems without PSE36 or PAE CPU features, hotplugging memory devices are not
supported by QEMU as QEMU always places hotplugged memory above 4 GiB boundary
which is beyond the physical address space of the processor. Linux guests also
does not support memory hotplug on those systems. Please see Linux
kernel commit b59d02ed08690 ("mm/memory_hotplug: disable the functionality
for 32b") for more details.
Therefore, the maximum limit of the guest physical address in the absence of
additional memory devices effectively coincides with the end of
"above 4G memory space" region for 32-bit x86 without PAE/PSE36. When users
configure additional memory devices, after properly accounting for the
additional device memory region to find the maximum value of the guest
physical address, the address will be outside the range of the processor's
physical address space.
This change adds improvements to take above into consideration.
For example, previously this was allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
With this change now it is no longer allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
qemu-system-x86_64: Address space limit 0x < 0x2bfff phys-bits too
low (32)
However, the following are allowed since on both cases physical address
space of the processor is 36 bits:
$ ./qemu-system-x86_64 -cpu pentium2 -m size=10G
$ ./qemu-system-x86_64 -cpu pentium,pse36=on -m size=10G
For 32-bit, without PAE/PSE36, hotplugging additional memory is no longer
allowed.
$ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
$ ./qemu-system-i386 -machine q35 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
A new compatibility flag is introduced to make sure pc_max_used_gpa() keeps
returning the old value for machines 8.1 and older.
Therefore, the above is still allowed for older machine types in order to
support
compatibility. Hence, the following still works:
$ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2
$ ./qemu-system-i386 -machine pc-q35-8.1 -m size=1G,maxmem=3G,slots=2
Further, following is also allowed as with PSE36, the processor has 36-bit
address space:
$ ./qemu-system-i386 -cpu 486,pse36=on -m size=1G,maxmem=3G,slots=2
After calling CPUID with EAX=0x8001, all AMD64 compliant processors
have the longmode-capable-bit turned on in the extended feature flags (bit 29)
in EDX. The absence of CPUID longmode can be used to differentiate between
32-bit and 64-bit processors and is the recommended approach. QEMU takes this
approach elsewhere (for example, please see x86_cpu_realizefn()), With
this change, pc_max_used_gpa() also uses the same method to detect 32-bit
processors.
Unit tests are modified to not run 32-bit x86 tests that use memory hotplug.
Suggested-by: David Hildenbrand
Signed-off-by: Ani Sinha
Reviewed-by: David Hildenbrand
---
hw/i386/pc.c | 32 +---
hw/i386/pc_piix.c | 4
hw/i386/pc_q35.c | 2 ++
include/hw/i386/pc.h | 6 ++
tests/qtest/bios-tables-test.c | 26 ++
tests/qtest/numa-test.c| 7 ++-
6 files changed, 65 insertions(+), 12 deletions(-)
changelog:
v6: more code messaging. incorporated another of phil's suggestions.
Thank you Ani, appreciated!
v5: addressed phil's suggestions in code reorg to make it cleaner.
v4: address comments from v3. Fix a bug where compat knob was absent
from q35 machines. Commit message adjustment.
v3: still accounting for additional memory device region above 4G.
unit tests fixed (not running for 32-bit where mem hotplug is used).
v2: removed memory hotplug region from max_gpa. added compat knobs.
Re: [PATCH] meson.build: Make keyutils independent from keyring
On 07/09/2023 21.57, Michael Tokarev wrote:
24.08.2023 12:42, Thomas Huth wrote:
Commit 0db0fbb5cf ("Add conditional dependency for libkeyutils")
tried to provide a possibility for the user to disable keyutils
if not required by makeing it depend on the keyring feature. This
looked reasonable at a first glance (the unit test in tests/unit/
needs both), but the condition in meson.build fails if the feature
is meant to be detected automatically, and there is also another
spot in backends/meson.build where keyutils is used independently
from keyring. So let's remove the dependency on keyring again and
introduce a proper meson build option instead.
Cc: qemu-sta...@nongnu.org
Fixes: 0db0fbb5cf ("Add conditional dependency for libkeyutils")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1842
Signed-off-by: Thomas Huth
Ping? Has this been forgotten?
I'll add it to my next pull request.
Thomas
Re: [PATCH v2 2/2] tests/tcg: Add -fno-stack-protector
On 31/07/2023 11.10, Akihiko Odaki wrote: A build of GCC 13.2 will have stack protector enabled by default if it was configured with --enable-default-ssp option. For such a compiler, it is necessary to explicitly disable stack protector when linking without standard libraries. Signed-off-by: Akihiko Odaki --- tests/tcg/mips/hello-mips.c | 4 ++-- tests/tcg/Makefile.target | 2 +- tests/tcg/aarch64/Makefile.target | 2 +- tests/tcg/arm/Makefile.target | 2 +- tests/tcg/cris/Makefile.target| 2 +- tests/tcg/hexagon/Makefile.target | 2 +- tests/tcg/i386/Makefile.target| 2 +- tests/tcg/minilib/Makefile.target | 2 +- tests/tcg/mips/Makefile.target| 2 +- 9 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/tcg/mips/hello-mips.c b/tests/tcg/mips/hello-mips.c index 4e1cf501af..0ba5f1bf23 100644 --- a/tests/tcg/mips/hello-mips.c +++ b/tests/tcg/mips/hello-mips.c @@ -5,8 +5,8 @@ * http://www.linux-mips.org/wiki/MIPSABIHistory * http://www.linux.com/howtos/Assembly-HOWTO/mips.shtml * -* mipsel-linux-gcc -nostdlib -mno-abicalls -fno-PIC -mabi=32 \ -* -O2 -static -o hello-mips hello-mips.c +* mipsel-linux-gcc -nostdlib -mno-abicalls -fno-PIC -fno-stack-protector \ + -mabi=32 -O2 -static -o hello-mips hello-mips.c You've lost the "*" at the beginning of the comment line here. But apart from that nit, the patch looks sane to me. Cc:-ing Alex Bennée ... could pick this patch up? Thomas * */ #define __NR_SYSCALL_BASE 4000 diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target index 3d7837d3b8..c43020d990 100644 --- a/tests/tcg/Makefile.target +++ b/tests/tcg/Makefile.target @@ -123,7 +123,7 @@ else # For softmmu targets we include a different Makefile fragment as the # build options for bare programs are usually pretty different. They # are expected to provide their own build recipes. -EXTRA_CFLAGS += -ffreestanding +EXTRA_CFLAGS += -ffreestanding -fno-stack-protector -include $(SRC_PATH)/tests/tcg/minilib/Makefile.target -include $(SRC_PATH)/tests/tcg/multiarch/system/Makefile.softmmu-target -include $(SRC_PATH)/tests/tcg/$(TARGET_NAME)/Makefile.softmmu-target diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target index 617f821613..55f8609897 100644 --- a/tests/tcg/aarch64/Makefile.target +++ b/tests/tcg/aarch64/Makefile.target @@ -49,7 +49,7 @@ endif # bti-1 tests the elf notes, so we require special compiler support. ifneq ($(CROSS_CC_HAS_ARMV8_BTI),) AARCH64_TESTS += bti-1 bti-3 -bti-1 bti-3: CFLAGS += -mbranch-protection=standard +bti-1 bti-3: CFLAGS += -fno-stack-protector -mbranch-protection=standard bti-1 bti-3: LDFLAGS += -nostdlib endif # bti-2 tests PROT_BTI, so no special compiler support required. diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target index 0038cef02c..3473f4619e 100644 --- a/tests/tcg/arm/Makefile.target +++ b/tests/tcg/arm/Makefile.target @@ -12,7 +12,7 @@ float_madds: CFLAGS+=-mfpu=neon-vfpv4 # Basic Hello World ARM_TESTS = hello-arm -hello-arm: CFLAGS+=-marm -ffreestanding +hello-arm: CFLAGS+=-marm -ffreestanding -fno-stack-protector hello-arm: LDFLAGS+=-nostdlib # IWMXT floating point extensions diff --git a/tests/tcg/cris/Makefile.target b/tests/tcg/cris/Makefile.target index 43587d2769..713e2a5b6c 100644 --- a/tests/tcg/cris/Makefile.target +++ b/tests/tcg/cris/Makefile.target @@ -30,7 +30,7 @@ AS= $(CC) -x assembler-with-cpp LD = $(CC) # we rely on GCC inline:ing the stuff we tell it to in many places here. -CFLAGS = -Winline -Wall -g -O2 -static +CFLAGS = -Winline -Wall -g -O2 -static -fno-stack-protector NOSTDFLAGS = -nostartfiles -nostdlib ASFLAGS += -mcpu=v10 -g -Wa,-I,$(SRC_PATH)/tests/tcg/cris/bare CRT_FILES = crt.o sys.o diff --git a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target index 87ed2c90b9..f839b2c0d5 100644 --- a/tests/tcg/hexagon/Makefile.target +++ b/tests/tcg/hexagon/Makefile.target @@ -19,7 +19,7 @@ EXTRA_RUNS = CFLAGS += -Wno-incompatible-pointer-types -Wno-undefined-internal -CFLAGS += -fno-unroll-loops +CFLAGS += -fno-unroll-loops -fno-stack-protector HEX_SRC=$(SRC_PATH)/tests/tcg/hexagon VPATH += $(HEX_SRC) diff --git a/tests/tcg/i386/Makefile.target b/tests/tcg/i386/Makefile.target index fdf757c6ce..3dec7c6c42 100644 --- a/tests/tcg/i386/Makefile.target +++ b/tests/tcg/i386/Makefile.target @@ -35,7 +35,7 @@ run-test-aes: QEMU_OPTS += -cpu max # # hello-i386 is a barebones app # -hello-i386: CFLAGS+=-ffreestanding +hello-i386: CFLAGS+=-ffreestanding -fno-stack-protector hello-i386: LDFLAGS+=-nostdlib # test-386 includes a couple of additional objects that need to be diff --git a/tests/tcg/minilib/Makefile.target b/tests/tcg/minilib/Makefile.target index c821d2806a..af0bf54be9 100644 --- a/tests/tcg/minilib/Makefile.target +++ b/tests/tcg/minilib/Makefile.target
[PATCH] tests/qtest/m48t59-test: Silence compiler warning with -Wshadow
When compiling this file with -Wshadow=local , we get: ../tests/qtest/m48t59-test.c: In function ‘bcd_check_time’: ../tests/qtest/m48t59-test.c:195:17: warning: declaration of ‘s’ shadows a previous local [-Wshadow=local] 195 | long t, s; | ^ ../tests/qtest/m48t59-test.c:158:17: note: shadowed declaration is here 158 | QTestState *s = m48t59_qtest_start(); | ^ Rename the QTestState variable to "qts" which is the common naming for such a variable in other tests. Reported-by: Markus Armbruster Signed-off-by: Thomas Huth --- tests/qtest/m48t59-test.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/qtest/m48t59-test.c b/tests/qtest/m48t59-test.c index 843d2ced8e..9487faff1a 100644 --- a/tests/qtest/m48t59-test.c +++ b/tests/qtest/m48t59-test.c @@ -155,7 +155,7 @@ static void bcd_check_time(void) struct tm *datep; time_t ts; const int wiggle = 2; -QTestState *s = m48t59_qtest_start(); +QTestState *qts = m48t59_qtest_start(); /* * This check assumes a few things. First, we cannot guarantee that we get @@ -173,10 +173,10 @@ static void bcd_check_time(void) ts = time(NULL); gmtime_r(, ); -cmos_get_date_time(s, [0]); -cmos_get_date_time(s, [1]); -cmos_get_date_time(s, [2]); -cmos_get_date_time(s, [3]); +cmos_get_date_time(qts, [0]); +cmos_get_date_time(qts, [1]); +cmos_get_date_time(qts, [2]); +cmos_get_date_time(qts, [3]); ts = time(NULL); gmtime_r(, ); @@ -207,7 +207,7 @@ static void bcd_check_time(void) g_assert_cmpint(ABS(t - s), <=, wiggle); } -qtest_quit(s); +qtest_quit(qts); } /* success if no crash or abort */ -- 2.41.0
Re: [PATCH v4 2/3] i386: Explicitly ignore unsupported BUS_MCEERR_AO MCE on AMD guest
On 9/22/23 16:30, Yazen Ghannam wrote: On 9/22/23 4:36 AM, William Roche wrote: On 9/21/23 19:41, Yazen Ghannam wrote: [...] Also, during page migration, does the data flow through the CPU core? Sorry for the basic question. I haven't done a lot with virtualization. Yes, in most cases (with the exception of RDMA) the data flow through the CPU cores because the migration verifies if the area to transfer has some empty pages. If the CPU moves the memory, then the data will pass through the core/L1 caches, correct? If so, then this will result in a MCE/poison consumption/AR event in that core. That's the entire point of this other patch I was referring to: "Qemu crashes on VM migration after an handled memory error" an example of a direct link: https://www.mail-archive.com/qemu-devel@nongnu.org/msg990803.html The idea is to skip the pages we know are poisoned -- so we have a chance to complete the migration without getting AR events :) So it seems to me that migration will always cause an AR event, and the gap you describe will not occur. Does this make sense? Sorry if I misunderstood. In general, the hardware is designed to detect and mark poison, and to not let poison escape a system undetected. In the strictest case, the hardware will perform a system reset if poison is leaving the system. In a more graceful case, the hardware will continue to pass the poison marker with the data, so the destination hardware will receive it. In both cases, the goal is to avoid silent data corruption, and to do so in the hardware, i.e. without relying on firmware or software management. The hardware designers are very keen on this point. For the moment virtualization needs *several* enhancements just to deal with memory errors -- what we are currently trying to fix is a good example of that ! BTW, the RDMA case will need further discussion. I *think* this would fall under the "strictest" case. And likely, CPU-based migration will also. But I think we can test this and find out. :) The test has been done, and showed that the RDMA migration is failing when poison exists. But we are discussing aspects that are probably too far from our main topic here. Please note that current AMD systems use an internal poison marker on memory. This cannot be cleared through normal memory operations. The only exception, I think, is to use the CLZERO instruction. This will completely wipe a cacheline including metadata like poison, etc. So the hardware should not (by design) loose track of poisoned data. This would be better, but virtualization migration currently looses track of this. Which is not a problem for VMs where the kernel took note of the poison and keeps track of it. Because this kernel will handle the poison locations it knows about, signaling when these poisoned locations are touched. Can you please elaborate on this? I would expect the host kernel to do all the physical, including poison, memory management. Yes, the host kernel does that, and the VM kernel too for its own address space. Or do you mean in the nested poison case like this? 1) The host detects an "AO/deferred" error. The host Kernel is notified by the hardware of an SRAO/deferred error 2) The host can try to recover the memory, if clean, etc. From my understanding, this is an uncorrectable error, standard case Kernel can't "clean" the error, but keeps track of it and tries to signal the user of the impacted memory page every-time it's needed. 3) Otherwise, the host passes the error info, with "AO/deferred" severity to the guest. Yes, in the case of a guest VM impacted, qemu asked to be informed of AO events, so that the host kernel should signal it to qemu. Qemu than relays the information (creating a virtual MCE event) that the VM Kernel receives and deals with. 4) The guest, in nested fashion, can try to recover the memory, if clean, etc. Or signal its own processes with the AO SIGBUS. Here again there is no recovery: The VM kernel does the same thing as the host kernel: memory management, possible signals, etc... An enhancement will be to take the MCA error information collected during the interrupt and extract useful data. For example, we'll need to translate the reported address to a system physical address that can be mapped to a page. This would be great, as it would mean that a kernel running in a VM can get notified too. Yes, I agree. Once we have the page, then we can decide how we want to signal the process(es). We could get a deferred/AO error in the host, and signal the guest with an AR. So the guest handling could be the same in both cases. > Would this be okay? Or is it important that the guest can distinguish between the A0/AR cases? SIGBUS/BUS_MCEERR_AO and BUS_MCEERR_AR are not interchangeable, it is important to distinguish them. AO is an asynchronous signal that is only generated when the process asked for it -- indicating that an error has been detected in its address space
Re: [PATCH] target/arm: Fix CNTPCT_EL0 trapping from EL0 when HCR_EL2.E2H is 0
On 22.09.23 16:21, Michal Orzel wrote:
Hello Michal
> On an attempt to access CNTPCT_EL0 from EL0 using a guest running on top
> of Xen, a trap from EL2 was observed which is something not reproducible
> on HW (also, Xen does not trap accesses to physical counter).
>
> This is because gt_counter_access() checks for an incorrect bit (1
> instead of 0) of CNTHCTL_EL2 if HCR_EL2.E2H is 0 and access is made to
> physical counter. Refer ARM ARM DDI 0487J.a, D19.12.2:
> When HCR_EL2.E2H is 0:
> - EL1PCTEN, bit [0]: refers to physical counter
> - EL1PCEN, bit [1]: refers to physical timer registers
>
> Fix it by checking for the right bit (i.e. 0) and update the comment
> referring to incorrect bit name.
>
> Fixes: 5bc8437136fb ("target/arm: Update timer access for VHE")
> Signed-off-by: Michal Orzel
You can add my:
[with Zephyr running as Xen guest and accessing CNTPCT_EL0 from EL0 ]
Tested-by: Oleksandr Tyshchenko
> ---
> This is now in conformance to ARM ARM CNTPCT_EL0 pseudocode:
> if PSTATE.EL == EL0 then
> ...
> elif EL2Enabled() && HCR_EL2.E2H == '0' && CNTHCTL_EL2.EL1PCTEN == '0'
> then
> AArch64.SystemAccessTrap(EL2, 0x18);
> ---
> target/arm/helper.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3b22596eabf3..3a2d77b3f81e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2483,9 +2483,9 @@ static CPAccessResult gt_counter_access(CPUARMState
> *env, int timeridx,
> return CP_ACCESS_TRAP_EL2;
> }
> } else {
> -/* If HCR_EL2. == 0: check CNTHCTL_EL2.EL1PCEN. */
> +/* If HCR_EL2. == 0: check CNTHCTL_EL2.EL1PCTEN. */
> if (has_el2 && timeridx == GTIMER_PHYS &&
> -!extract32(env->cp15.cnthctl_el2, 1, 1)) {
> +!extract32(env->cp15.cnthctl_el2, 0, 1)) {
> return CP_ACCESS_TRAP_EL2;
> }
> }
Re: [PATCH 1/2] migration: Fix rdma migration failed
On Fri, Sep 22, 2023 at 12:59:37PM -0300, Fabiano Rosas wrote:
> Peter Xu writes:
>
> > On Wed, Sep 20, 2023 at 05:04:11PM +0800, Li Zhijian wrote:
> >> From: Li Zhijian
> >>
> >> Destination will fail with:
> >> qemu-system-x86_64: rdma: Too many requests in this message
> >> (3638950032).Bailing.
> >>
> >> migrate with RDMA is different from tcp. RDMA has its own control
> >> message, and all traffic between RDMA_CONTROL_REGISTER_REQUEST and
> >> RDMA_CONTROL_REGISTER_FINISHED should not be disturbed.
> >>
> >> find_dirty_block() will be called during RDMA_CONTROL_REGISTER_REQUEST
> >> and RDMA_CONTROL_REGISTER_FINISHED, it will send a extra traffic to
> >> destination and cause migration to fail.
> >>
> >> Since there's no existing subroutine to indicate whether it's migrated
> >> by RDMA or not, and RDMA is not compatible with multifd, we use
> >> migrate_multifd() here.
> >>
> >> Fixes: 294e5a4034 ("multifd: Only flush once each full round of memory")
> >> Signed-off-by: Li Zhijian
> >> ---
> >> migration/ram.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 9040d66e61..89ae28e21a 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -1399,7 +1399,8 @@ static int find_dirty_block(RAMState *rs,
> >> PageSearchStatus *pss)
> >> pss->page = 0;
> >> pss->block = QLIST_NEXT_RCU(pss->block, next);
> >> if (!pss->block) {
> >> -if (!migrate_multifd_flush_after_each_section()) {
> >> +if (migrate_multifd() &&
> >> +!migrate_multifd_flush_after_each_section()) {
> >> QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
> >> int ret = multifd_send_sync_main(f);
> >> if (ret < 0) {
> >> --
> >> 2.31.1
> >>
> >
> > Maybe better to put that check at the entry of
> > migrate_multifd_flush_after_each_section()?
> >
> > I also hope that some day there's no multifd function called in generic
> > migration code paths..
>
> I wonder what happened with that MigrationOps idea. We added the
> ram_save_target_page pointer and nothing else. It seems like it could be
> something in the direction of allowing different parts of the migration
> code to provide different behavior without having to put these explicit
> checks all over the place.
Yeah..
https://lore.kernel.org/qemu-devel/20230130080956.3047-12-quint...@redhat.com/
Juan should know better.
Personally I think it'll be good we only introduce hook when there's a 2nd
user already. I assume Juan merged it planning that'll land soon but it
didn't.
--
Peter Xu
Re: [PATCH v4 00/21] Support smp.clusters for x86 in QEMU
Tested the series on AMD system. Created a VM and ran some basic commands.
Everything looks good.
Tested-by: Babu Moger
On 9/14/2023 2:21 AM, Zhao Liu wrote:
From: Zhao Liu
Hi list,
(CC k...@vger.kernel.org for better browsing.)
This is the our v4 patch series, rebased on the master branch at the
commit 9ef497755afc2 ("Merge tag 'pull-vfio-20230911' of
https://github.com/legoater/qemu into staging").
Comparing with v3 [1], v4 mainly refactors the CPUID[0x1F] encoding and
exposes module level in CPUID[0x1F] with these new patches:
* [PATCH v4 08/21] i386: Split topology types of CPUID[0x1F] from the
definitions of CPUID[0xB]
* [PATCH v4 09/21] i386: Decouple CPUID[0x1F] subleaf with specific
topology level
* [PATCH v4 12/21] i386: Expose module level in CPUID[0x1F]
v4 also fixes compile warnings and fixes cache topology uninitialization
bugs for some AMD CPUs.
Welcome your comments!
# Introduction
This series add the cluster support for x86 PC machine, which allows
x86 can use smp.clusters to configure the module level CPU topology
of x86.
And since the compatibility issue (see section: ## Why not share L2
cache in cluster directly), this series also introduce a new command
to adjust the topology of the x86 L2 cache.
Welcome your comments!
# Background
The "clusters" parameter in "smp" is introduced by ARM [2], but x86
hasn't supported it.
At present, x86 defaults L2 cache is shared in one core, but this is
not enough. There're some platforms that multiple cores share the
same L2 cache, e.g., Alder Lake-P shares L2 cache for one module of
Atom cores [3], that is, every four Atom cores shares one L2 cache.
Therefore, we need the new CPU topology level (cluster/module).
Another reason is for hybrid architecture. cluster support not only
provides another level of topology definition in x86, but would also
provide required code change for future our hybrid topology support.
# Overview
## Introduction of module level for x86
"cluster" in smp is the CPU topology level which is between "core" and
die.
For x86, the "cluster" in smp is corresponding to the module level [4],
which is above the core level. So use the "module" other than "cluster"
in x86 code.
And please note that x86 already has a cpu topology level also named
"cluster" [4], this level is at the upper level of the package. Here,
the cluster in x86 cpu topology is completely different from the
"clusters" as the smp parameter. After the module level is introduced,
the cluster as the smp parameter will actually refer to the module level
of x86.
## Why not share L2 cache in cluster directly
Though "clusters" was introduced to help define L2 cache topology
[2], using cluster to define x86's L2 cache topology will cause the
compatibility problem:
Currently, x86 defaults that the L2 cache is shared in one core, which
actually implies a default setting "cores per L2 cache is 1" and
therefore implicitly defaults to having as many L2 caches as cores.
For example (i386 PC machine):
-smp 16,sockets=2,dies=2,cores=2,threads=2,maxcpus=16 (*)
Considering the topology of the L2 cache, this (*) implicitly means "1
core per L2 cache" and "2 L2 caches per die".
If we use cluster to configure L2 cache topology with the new default
setting "clusters per L2 cache is 1", the above semantics will change
to "2 cores per cluster" and "1 cluster per L2 cache", that is, "2
cores per L2 cache".
So the same command (*) will cause changes in the L2 cache topology,
further affecting the performance of the virtual machine.
Therefore, x86 should only treat cluster as a cpu topology level and
avoid using it to change L2 cache by default for compatibility.
## module level in CPUID
Linux kernel (from v6.4, with commit edc0a2b595765 ("x86/topology: Fix
erroneous smp_num_siblings on Intel Hybrid platforms") is able to
handle platforms with Module level enumerated via CPUID.1F.
Expose the module level in CPUID[0x1F] (for Intel CPUs) if the machine
has more than 1 modules since v3.
We can configure CPUID.04H.02H (L2 cache topology) with module level by
a new command:
"-cpu,x-l2-cache-topo=cluster"
More information about this command, please see the section: "## New
property: x-l2-cache-topo".
## New cache topology info in CPUCacheInfo
Currently, by default, the cache topology is encoded as:
1. i/d cache is shared in one core.
2. L2 cache is shared in one core.
3. L3 cache is shared in one die.
This default general setting has caused a misunderstanding, that is, the
cache topology is completely equated with a specific cpu topology, such
as the connection between L2 cache and core level, and the connection
between L3 cache and die level.
In fact, the settings of these topologies depend on the specific
platform and are not static. For example, on Alder Lake-P, every
four Atom cores share the same L2 cache [2].
Thus, in this patch set, we explicitly define the corresponding cache
topology for different cpu models and this has two
Re: [PATCH 1/4] multifd: wait for channels_ready before sending sync
Elena Ufimtseva writes:
> In multifd_send_sync_main we need to wait for channels_ready
> before submitting sync packet as the threads may still be sending
> their previous pages.
> There is also no need to check for channels_ready in the loop
> before the wait for sem_sync, next iteration of sending pages
> or another sync will start with waiting for channels_ready
> semaphore.
> Changes to commit 90b3cec351996dd8ef4eb847ad38607812c5e7f5
> ("multifd: Fix the number of channels ready")
>
> Signed-off-by: Elena Ufimtseva
> ---
> migration/multifd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 0f6b203877..e61e458151 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -595,6 +595,7 @@ int multifd_send_sync_main(QEMUFile *f)
> }
> }
>
> +qemu_sem_wait(_send_state->channels_ready);
> /*
> * When using zero-copy, it's necessary to flush the pages before any of
> * the pages can be sent again, so we'll make sure the new version of the
> @@ -630,7 +631,6 @@ int multifd_send_sync_main(QEMUFile *f)
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDSendParams *p = _send_state->params[i];
>
> -qemu_sem_wait(_send_state->channels_ready);
> trace_multifd_send_sync_main_wait(p->id);
> qemu_sem_wait(>sem_sync);
Please take a look at the series I just sent. Basically, I think we
should wait on 'sem' for the number of existing channels and not just
once per sync. Otherwise I think we'd hit the same issue this patch is
trying to fix when we loop into the n+1 channels. I think the
assert(!p->pending_job) in patch 3 helps prove that's more appropriate.
Let me know what you think.
Thanks
[PATCH 2/2] seccomp: avoid shadowing of 'action' variable
This is confusing as one 'action' variable is used for storing
a SCMP_ enum value, while the other 'action' variable is used
for storing a SECCOMP_ enum value.
Signed-off-by: Daniel P. Berrangé
---
softmmu/qemu-seccomp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c
index d66a2a1226..4d7439e7f7 100644
--- a/softmmu/qemu-seccomp.c
+++ b/softmmu/qemu-seccomp.c
@@ -283,9 +283,9 @@ static uint32_t qemu_seccomp_update_action(uint32_t action)
if (action == SCMP_ACT_TRAP) {
static int kill_process = -1;
if (kill_process == -1) {
-uint32_t action = SECCOMP_RET_KILL_PROCESS;
+uint32_t testaction = SECCOMP_RET_KILL_PROCESS;
-if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, ) == 0) {
+if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, ) == 0) {
kill_process = 1;
} else {
kill_process = 0;
--
2.41.0
[PATCH 1/2] crypto: remove shadowed 'ret' variable
Both instances of 'ret' are used to store a gnutls API return code.
Signed-off-by: Daniel P. Berrangé
---
crypto/tls-cipher-suites.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/crypto/tls-cipher-suites.c b/crypto/tls-cipher-suites.c
index 5e4f597464..d0df4badc0 100644
--- a/crypto/tls-cipher-suites.c
+++ b/crypto/tls-cipher-suites.c
@@ -52,7 +52,6 @@ GByteArray
*qcrypto_tls_cipher_suites_get_data(QCryptoTLSCipherSuites *obj,
byte_array = g_byte_array_new();
for (i = 0;; i++) {
-int ret;
unsigned idx;
const char *name;
IANA_TLS_CIPHER cipher;
--
2.41.0
[PATCH 0/2] remove some variable shadowing
Daniel P. Berrangé (2): crypto: remove shadowed 'ret' variable seccomp: avoid shadowing of 'action' variable crypto/tls-cipher-suites.c | 1 - softmmu/qemu-seccomp.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) -- 2.41.0
Re: [PATCH v4 01/21] i386: Fix comment style in topology.h
On 9/14/2023 2:21 AM, Zhao Liu wrote:
From: Zhao Liu
For function comments in this file, keep the comment style consistent
with other files in the directory.
Signed-off-by: Zhao Liu
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Yanan Wang
Reviewed-by: Xiaoyao Li
Acked-by: Michael S. Tsirkin
Reviewed-by: Babu Moger
Thanks
Babu
---
Changes since v3:
* Optimized the description in commit message: Change "with other
places" to "with other files in the directory". (Babu)
---
include/hw/i386/topology.h | 33 +
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 81573f6cfde0..5a19679f618b 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -24,7 +24,8 @@
#ifndef HW_I386_TOPOLOGY_H
#define HW_I386_TOPOLOGY_H
-/* This file implements the APIC-ID-based CPU topology enumeration logic,
+/*
+ * This file implements the APIC-ID-based CPU topology enumeration logic,
* documented at the following document:
* Intel® 64 Architecture Processor Topology Enumeration
*
http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
@@ -41,7 +42,8 @@
#include "qemu/bitops.h"
-/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
+/*
+ * APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
*/
typedef uint32_t apic_id_t;
@@ -58,8 +60,7 @@ typedef struct X86CPUTopoInfo {
unsigned threads_per_core;
} X86CPUTopoInfo;
-/* Return the bit width needed for 'count' IDs
- */
+/* Return the bit width needed for 'count' IDs */
static unsigned apicid_bitwidth_for_count(unsigned count)
{
g_assert(count >= 1);
@@ -67,15 +68,13 @@ static unsigned apicid_bitwidth_for_count(unsigned count)
return count ? 32 - clz32(count) : 0;
}
-/* Bit width of the SMT_ID (thread ID) field on the APIC ID
- */
+/* Bit width of the SMT_ID (thread ID) field on the APIC ID */
static inline unsigned apicid_smt_width(X86CPUTopoInfo *topo_info)
{
return apicid_bitwidth_for_count(topo_info->threads_per_core);
}
-/* Bit width of the Core_ID field
- */
+/* Bit width of the Core_ID field */
static inline unsigned apicid_core_width(X86CPUTopoInfo *topo_info)
{
return apicid_bitwidth_for_count(topo_info->cores_per_die);
@@ -87,8 +86,7 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo
*topo_info)
return apicid_bitwidth_for_count(topo_info->dies_per_pkg);
}
-/* Bit offset of the Core_ID field
- */
+/* Bit offset of the Core_ID field */
static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info)
{
return apicid_smt_width(topo_info);
@@ -100,14 +98,14 @@ static inline unsigned apicid_die_offset(X86CPUTopoInfo
*topo_info)
return apicid_core_offset(topo_info) + apicid_core_width(topo_info);
}
-/* Bit offset of the Pkg_ID (socket ID) field
- */
+/* Bit offset of the Pkg_ID (socket ID) field */
static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
{
return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
}
-/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
+/*
+ * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
*
* The caller must make sure core_id < nr_cores and smt_id < nr_threads.
*/
@@ -120,7 +118,8 @@ static inline apic_id_t
x86_apicid_from_topo_ids(X86CPUTopoInfo *topo_info,
topo_ids->smt_id;
}
-/* Calculate thread/core/package IDs for a specific topology,
+/*
+ * Calculate thread/core/package IDs for a specific topology,
* based on (contiguous) CPU index
*/
static inline void x86_topo_ids_from_idx(X86CPUTopoInfo *topo_info,
@@ -137,7 +136,8 @@ static inline void x86_topo_ids_from_idx(X86CPUTopoInfo
*topo_info,
topo_ids->smt_id = cpu_index % nr_threads;
}
-/* Calculate thread/core/package IDs for a specific topology,
+/*
+ * Calculate thread/core/package IDs for a specific topology,
* based on APIC ID
*/
static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
@@ -155,7 +155,8 @@ static inline void x86_topo_ids_from_apicid(apic_id_t
apicid,
topo_ids->pkg_id = apicid >> apicid_pkg_offset(topo_info);
}
-/* Make APIC ID for the CPU 'cpu_index'
+/*
+ * Make APIC ID for the CPU 'cpu_index'
*
* 'cpu_index' is a sequential, contiguous ID for the CPU.
*/
Re: [PATCH] hw/acpi: changes towards enabling -Wshadow=local
On Fri, Sep 22, 2023 at 06:12:02PM +0530, Ani Sinha wrote:
> Code changes in acpi that addresses all compiler complaints coming from
> enabling
> -Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
> other local variables or parameters. These makes the code confusing and/or
> adds
> bugs that are difficult to catch.
>
> The code is tested to build with and without the flag turned on.
>
> CC: Markus Armbruster
> CC: Philippe Mathieu-Daude
> CC: m...@redhat.com
> CC: imamm...@redhat.com
> Message-Id: <87r0mqlf9x@pond.sub.org>
> Signed-off-by: Ani Sinha
Reviewed-by: Michael S. Tsirkin
> ---
> hw/acpi/cpu_hotplug.c | 25 +
> hw/i386/acpi-build.c | 24
> hw/smbios/smbios.c| 37 +++--
> 3 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index ff14c3f410..634bbecb31 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -265,26 +265,27 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx,
> MachineState *machine,
>
> /* build Processor object for each processor */
> for (i = 0; i < apic_ids->len; i++) {
> -int apic_id = apic_ids->cpus[i].arch_id;
> +int cpu_apic_id = apic_ids->cpus[i].arch_id;
>
> -assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
> +assert(cpu_apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
>
> -dev = aml_processor(i, 0, 0, "CP%.02X", apic_id);
> +dev = aml_processor(i, 0, 0, "CP%.02X", cpu_apic_id);
>
> method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
> aml_append(method,
> -aml_return(aml_call2(CPU_MAT_METHOD, aml_int(apic_id),
> aml_int(i))
> +aml_return(aml_call2(CPU_MAT_METHOD,
> + aml_int(cpu_apic_id), aml_int(i))
> ));
> aml_append(dev, method);
>
> method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> aml_append(method,
> -aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(apic_id;
> +aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(cpu_apic_id;
> aml_append(dev, method);
>
> method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> aml_append(method,
> -aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(apic_id),
> +aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(cpu_apic_id),
> aml_arg(0)))
> );
> aml_append(dev, method);
> @@ -298,11 +299,11 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx,
> MachineState *machine,
> /* Arg0 = APIC ID */
> method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
> for (i = 0; i < apic_ids->len; i++) {
> -int apic_id = apic_ids->cpus[i].arch_id;
> +int cpu_apic_id = apic_ids->cpus[i].arch_id;
>
> -if_ctx = aml_if(aml_equal(aml_arg(0), aml_int(apic_id)));
> +if_ctx = aml_if(aml_equal(aml_arg(0), aml_int(cpu_apic_id)));
> aml_append(if_ctx,
> -aml_notify(aml_name("CP%.02X", apic_id), aml_arg(1))
> +aml_notify(aml_name("CP%.02X", cpu_apic_id), aml_arg(1))
> );
> aml_append(method, if_ctx);
> }
> @@ -319,13 +320,13 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx,
> MachineState *machine,
> aml_varpackage(x86ms->apic_id_limit);
>
> for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
> -int apic_id = apic_ids->cpus[i].arch_id;
> +int cpu_apic_id = apic_ids->cpus[i].arch_id;
>
> -for (; apic_idx < apic_id; apic_idx++) {
> +for (; apic_idx < cpu_apic_id; apic_idx++) {
> aml_append(pkg, aml_int(0));
> }
> aml_append(pkg, aml_int(apic_ids->cpus[i].cpu ? 1 : 0));
> -apic_idx = apic_id + 1;
> +apic_idx = cpu_apic_id + 1;
> }
> aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
> aml_append(ctx, sb_scope);
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4d2d40bab5..95199c8900 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1585,12 +1585,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
> aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
> if (pci_bus_is_cxl(bus)) {
> -struct Aml *pkg = aml_package(2);
> +struct Aml *aml_pkg = aml_package(2);
>
> aml_append(dev, aml_name_decl("_HID",
> aml_string("ACPI0016")));
> -aml_append(pkg, aml_eisaid("PNP0A08"));
> -aml_append(pkg, aml_eisaid("PNP0A03"));
> -aml_append(dev, aml_name_decl("_CID", pkg));
> +aml_append(aml_pkg, aml_eisaid("PNP0A08"));
> +aml_append(aml_pkg, aml_eisaid("PNP0A03"));
> +
[PATCH v6] hw/i386/pc: improve physical address space bound check for 32-bit x86 systems
32-bit x86 systems do not have a reserved memory for hole64. On those 32-bit
systems without PSE36 or PAE CPU features, hotplugging memory devices are not
supported by QEMU as QEMU always places hotplugged memory above 4 GiB boundary
which is beyond the physical address space of the processor. Linux guests also
does not support memory hotplug on those systems. Please see Linux
kernel commit b59d02ed08690 ("mm/memory_hotplug: disable the functionality
for 32b") for more details.
Therefore, the maximum limit of the guest physical address in the absence of
additional memory devices effectively coincides with the end of
"above 4G memory space" region for 32-bit x86 without PAE/PSE36. When users
configure additional memory devices, after properly accounting for the
additional device memory region to find the maximum value of the guest
physical address, the address will be outside the range of the processor's
physical address space.
This change adds improvements to take above into consideration.
For example, previously this was allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
With this change now it is no longer allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
qemu-system-x86_64: Address space limit 0x < 0x2bfff phys-bits too
low (32)
However, the following are allowed since on both cases physical address
space of the processor is 36 bits:
$ ./qemu-system-x86_64 -cpu pentium2 -m size=10G
$ ./qemu-system-x86_64 -cpu pentium,pse36=on -m size=10G
For 32-bit, without PAE/PSE36, hotplugging additional memory is no longer
allowed.
$ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
$ ./qemu-system-i386 -machine q35 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
A new compatibility flag is introduced to make sure pc_max_used_gpa() keeps
returning the old value for machines 8.1 and older.
Therefore, the above is still allowed for older machine types in order to
support
compatibility. Hence, the following still works:
$ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2
$ ./qemu-system-i386 -machine pc-q35-8.1 -m size=1G,maxmem=3G,slots=2
Further, following is also allowed as with PSE36, the processor has 36-bit
address space:
$ ./qemu-system-i386 -cpu 486,pse36=on -m size=1G,maxmem=3G,slots=2
After calling CPUID with EAX=0x8001, all AMD64 compliant processors
have the longmode-capable-bit turned on in the extended feature flags (bit 29)
in EDX. The absence of CPUID longmode can be used to differentiate between
32-bit and 64-bit processors and is the recommended approach. QEMU takes this
approach elsewhere (for example, please see x86_cpu_realizefn()), With
this change, pc_max_used_gpa() also uses the same method to detect 32-bit
processors.
Unit tests are modified to not run 32-bit x86 tests that use memory hotplug.
Suggested-by: David Hildenbrand
Signed-off-by: Ani Sinha
Reviewed-by: David Hildenbrand
---
hw/i386/pc.c | 32 +---
hw/i386/pc_piix.c | 4
hw/i386/pc_q35.c | 2 ++
include/hw/i386/pc.h | 6 ++
tests/qtest/bios-tables-test.c | 26 ++
tests/qtest/numa-test.c| 7 ++-
6 files changed, 65 insertions(+), 12 deletions(-)
changelog:
v6: more code messaging. incorporated another of phil's suggestions.
v5: addressed phil's suggestions in code reorg to make it cleaner.
v4: address comments from v3. Fix a bug where compat knob was absent
from q35 machines. Commit message adjustment.
v3: still accounting for additional memory device region above 4G.
unit tests fixed (not running for 32-bit where mem hotplug is used).
v2: removed memory hotplug region from max_gpa. added compat knobs.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3db0743f31..a532d42cf4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -907,13 +907,39 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
{
X86CPU *cpu = X86_CPU(first_cpu);
+PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+MachineState *ms = MACHINE(pcms);
-/* 32-bit systems don't have hole64 thus return max CPU address */
-if (cpu->phys_bits <= 32) {
+if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
+/* 64-bit systems */
+return pc_pci_hole64_start() + pci_hole64_size - 1;
+}
+
+/* 32-bit systems */
+if (pcmc->broken_32bit_mem_addr_check) {
+/* old value for compatibility reasons */
return ((hwaddr)1 << cpu->phys_bits) - 1;
}
-return pc_pci_hole64_start() + pci_hole64_size - 1;
+/*
+ * 32-bit systems don't have hole64 but they might have a region for
+ * memory devices. Even if additional hotplugged memory devices might
+ * not be
Re: [PATCH 1/2] migration: Fix rdma migration failed
Peter Xu writes:
> On Wed, Sep 20, 2023 at 05:04:11PM +0800, Li Zhijian wrote:
>> From: Li Zhijian
>>
>> Destination will fail with:
>> qemu-system-x86_64: rdma: Too many requests in this message
>> (3638950032).Bailing.
>>
>> migrate with RDMA is different from tcp. RDMA has its own control
>> message, and all traffic between RDMA_CONTROL_REGISTER_REQUEST and
>> RDMA_CONTROL_REGISTER_FINISHED should not be disturbed.
>>
>> find_dirty_block() will be called during RDMA_CONTROL_REGISTER_REQUEST
>> and RDMA_CONTROL_REGISTER_FINISHED, it will send a extra traffic to
>> destination and cause migration to fail.
>>
>> Since there's no existing subroutine to indicate whether it's migrated
>> by RDMA or not, and RDMA is not compatible with multifd, we use
>> migrate_multifd() here.
>>
>> Fixes: 294e5a4034 ("multifd: Only flush once each full round of memory")
>> Signed-off-by: Li Zhijian
>> ---
>> migration/ram.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 9040d66e61..89ae28e21a 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1399,7 +1399,8 @@ static int find_dirty_block(RAMState *rs,
>> PageSearchStatus *pss)
>> pss->page = 0;
>> pss->block = QLIST_NEXT_RCU(pss->block, next);
>> if (!pss->block) {
>> -if (!migrate_multifd_flush_after_each_section()) {
>> +if (migrate_multifd() &&
>> +!migrate_multifd_flush_after_each_section()) {
>> QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
>> int ret = multifd_send_sync_main(f);
>> if (ret < 0) {
>> --
>> 2.31.1
>>
>
> Maybe better to put that check at the entry of
> migrate_multifd_flush_after_each_section()?
>
> I also hope that some day there's no multifd function called in generic
> migration code paths..
I wonder what happened with that MigrationOps idea. We added the
ram_save_target_page pointer and nothing else. It seems like it could be
something in the direction of allowing different parts of the migration
code to provide different behavior without having to put these explicit
checks all over the place.
And although we're removing the QEMUFile hooks, those also looked like
they could help mitigate these cross-layer interactions. (I'm NOT
advocating bringing them back).
[PATCH] intel_iommu: Fix shadow local variables on "size"
This patch fixes the warning of shadowed local variable:
../hw/i386/intel_iommu.c: In function ‘vtd_address_space_unmap’:
../hw/i386/intel_iommu.c:3773:18: warning: declaration of ‘size’ shadows a
previous local [-Wshadow=compatible-local]
3773 | uint64_t size = mask + 1;
| ^~~~
../hw/i386/intel_iommu.c:3747:12: note: shadowed declaration is here
3747 | hwaddr size, remain;
|^~~~
Cc: Jason Wang
Cc: Eric Auger
Cc: Michael S. Tsirkin
Cc: Markus Armbruster
Signed-off-by: Peter Xu
---
hw/i386/intel_iommu.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c9961ef752..ae30c2b469 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
PCIBus *bus,
/* Unmap the whole range in the notifier's scope. */
static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
{
-hwaddr size, remain;
+hwaddr total, remain;
hwaddr start = n->start;
hwaddr end = n->end;
IntelIOMMUState *s = as->iommu_state;
@@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace *as,
IOMMUNotifier *n)
}
assert(start <= end);
-size = remain = end - start + 1;
+total = remain = end - start + 1;
while (remain >= VTD_PAGE_SIZE) {
IOMMUTLBEvent event;
@@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
*as, IOMMUNotifier *n)
trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
VTD_PCI_SLOT(as->devfn),
VTD_PCI_FUNC(as->devfn),
- n->start, size);
+ n->start, total);
map.iova = n->start;
-map.size = size - 1; /* Inclusive */
+map.size = total - 1; /* Inclusive */
iova_tree_remove(as->iova_tree, map);
}
--
2.41.0
[PATCH 3/4] aspeed/i3c: Rename variable shadowing a local
to fix warning :
../hw/i3c/aspeed_i3c.c: In function ‘aspeed_i3c_realize’:
../hw/i3c/aspeed_i3c.c:1959:17: warning: declaration of ‘dev’ shadows a
parameter [-Wshadow=local]
1959 | Object *dev = OBJECT(>devices[i]);
| ^~~
../hw/i3c/aspeed_i3c.c:1942:45: note: shadowed declaration is here
1942 | static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
|~^~~
Signed-off-by: Cédric Le Goater
---
hw/misc/aspeed_i3c.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/misc/aspeed_i3c.c b/hw/misc/aspeed_i3c.c
index f54f5da522b3..d1ff61767167 100644
--- a/hw/misc/aspeed_i3c.c
+++ b/hw/misc/aspeed_i3c.c
@@ -296,13 +296,13 @@ static void aspeed_i3c_realize(DeviceState *dev, Error
**errp)
memory_region_add_subregion(>iomem_container, 0x0, >iomem);
for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) {
-Object *dev = OBJECT(>devices[i]);
+Object *i3c_dev = OBJECT(>devices[i]);
-if (!object_property_set_uint(dev, "device-id", i, errp)) {
+if (!object_property_set_uint(i3c_dev, "device-id", i, errp)) {
return;
}
-if (!sysbus_realize(SYS_BUS_DEVICE(dev), errp)) {
+if (!sysbus_realize(SYS_BUS_DEVICE(i3c_dev), errp)) {
return;
}
--
2.41.0
[PATCH 1/4] aspeed/i2c: Clean up local variable shadowing
Remove superfluous local 'data' variable and use the one define at the
top of the routine. This fixes :
../hw/i2c/aspeed_i2c.c: In function ‘aspeed_i2c_bus_recv’:
../hw/i2c/aspeed_i2c.c:315:17: warning: declaration of ‘data’ shadows a
previous local [-Wshadow=compatible-local]
315 | uint8_t data;
| ^~~~
../hw/i2c/aspeed_i2c.c:288:13: note: shadowed declaration is here
288 | uint8_t data;
| ^~~~
Signed-off-by: Cédric Le Goater
---
hw/i2c/aspeed_i2c.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 7275d40749a9..1037c22b2f79 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -312,7 +312,6 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
SHARED_ARRAY_FIELD_DP32(bus->regs, reg_pool_ctrl, RX_COUNT, i & 0xff);
SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, RX_BUFF_EN, 0);
} else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
-uint8_t data;
/* In new mode, clear how many bytes we RXed */
if (aspeed_i2c_is_new_mode(bus->controller)) {
ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, RX_LEN, 0);
--
2.41.0
[PATCH 0/4] aspeed: Clean up local variable shadowing
Hello, Here are cleanups for local variable shadowing warnings in aspeed models. Joel, Andrew, Could you please double check patch 4 ? Thanks, C. Cédric Le Goater (4): aspeed/i2c: Clean up local variable shadowing aspeed: Clean up local variable shadowing aspeed/i3c: Rename variable shadowing a local aspeed/timer: Clean up local variable shadowing hw/arm/aspeed_ast2600.c | 10 +- hw/i2c/aspeed_i2c.c | 1 - hw/misc/aspeed_i3c.c| 6 +++--- hw/timer/aspeed_timer.c | 2 +- 4 files changed, 9 insertions(+), 10 deletions(-) -- 2.41.0
[PATCH 2/4] aspeed: Clean up local variable shadowing
Remove superfluous local 'irq' variables and use the one define at the
top of the routine. This fixes warnings in aspeed_soc_ast2600_realize()
such as :
../hw/arm/aspeed_ast2600.c: In function ‘aspeed_soc_ast2600_realize’:
../hw/arm/aspeed_ast2600.c:420:18: warning: declaration of ‘irq’ shadows a
previous local [-Wshadow=compatible-local]
420 | qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
| ^~~
../hw/arm/aspeed_ast2600.c:312:14: note: shadowed declaration is here
312 | qemu_irq irq;
| ^~~
Signed-off-by: Cédric Le Goater
---
hw/arm/aspeed_ast2600.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index a8b3a8065a11..e122e1c32d42 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -388,7 +388,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
aspeed_mmio_map(s, SYS_BUS_DEVICE(>timerctrl), 0,
sc->memmap[ASPEED_DEV_TIMER1]);
for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
-qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
+irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
sysbus_connect_irq(SYS_BUS_DEVICE(>timerctrl), i, irq);
}
@@ -413,8 +413,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
}
aspeed_mmio_map(s, SYS_BUS_DEVICE(>i2c), 0, sc->memmap[ASPEED_DEV_I2C]);
for (i = 0; i < ASPEED_I2C_GET_CLASS(>i2c)->num_busses; i++) {
-qemu_irq irq = qdev_get_gpio_in(DEVICE(>a7mpcore),
-sc->irqmap[ASPEED_DEV_I2C] + i);
+irq = qdev_get_gpio_in(DEVICE(>a7mpcore),
+ sc->irqmap[ASPEED_DEV_I2C] + i);
/* The AST2600 I2C controller has one IRQ per bus. */
sysbus_connect_irq(SYS_BUS_DEVICE(>i2c.busses[i]), 0, irq);
}
@@ -611,8 +611,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
}
aspeed_mmio_map(s, SYS_BUS_DEVICE(>i3c), 0, sc->memmap[ASPEED_DEV_I3C]);
for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
-qemu_irq irq = qdev_get_gpio_in(DEVICE(>a7mpcore),
-sc->irqmap[ASPEED_DEV_I3C] + i);
+irq = qdev_get_gpio_in(DEVICE(>a7mpcore),
+ sc->irqmap[ASPEED_DEV_I3C] + i);
/* The AST2600 I3C controller has one IRQ per bus. */
sysbus_connect_irq(SYS_BUS_DEVICE(>i3c.devices[i]), 0, irq);
}
--
2.41.0
[PATCH 4/4] aspeed/timer: Clean up local variable shadowing
commit 8137355e850f ("aspeed/timer: Fix behaviour running Linux")
introduced a MAX() expression to calculate the next timer deadline :
return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
The second MAX() is not necessary since the compared values are an
unsigned and 0. Simply remove it and fix warning :
../hw/timer/aspeed_timer.c: In function ‘calculate_next’:
../include/qemu/osdep.h:396:31: warning: declaration of ‘_a’ shadows a
previous local [-Wshadow=compatible-local]
396 | typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
| ^~
../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
170 | next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
|^~~
../hw/timer/aspeed_timer.c:170:16: note: in expansion of macro ‘MAX’
170 | next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
|^~~
/home/legoater/work/qemu/qemu-aspeed.git/include/qemu/osdep.h:396:31: note:
shadowed declaration is here
396 | typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
| ^~
../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
170 | next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
|^~~
Cc: Joel Stanley
Cc: Andrew Jeffery
Signed-off-by: Cédric Le Goater
---
hw/timer/aspeed_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 9c20b3d6ad8a..72161f07bbee 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -167,7 +167,7 @@ static uint64_t calculate_next(struct AspeedTimer *t)
qemu_set_irq(t->irq, t->level);
}
-next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
+next = MAX(calculate_match(t, 0), calculate_match(t, 1));
t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
return calculate_time(t, next);
--
2.41.0
Re: [PATCH 2/2] migration/rdma: zore out head.repeat to make the error more clear
On Wed, Sep 20, 2023 at 05:04:12PM +0800, Li Zhijian wrote: > From: Li Zhijian > > Previously, we got a confusion error that complains > the RDMAControlHeader.repeat: > qemu-system-x86_64: rdma: Too many requests in this message > (3638950032).Bailing. > > Actually, it's caused by an unexpected RDMAControlHeader.type. > After this patch, error will become: > qemu-system-x86_64: Unknown control message QEMU FILE > > Signed-off-by: Li Zhijian Reviewed-by: Peter Xu -- Peter Xu
[PULL 5/9] hw/audio: Simplify hda audio init
From: Martin Kletzander
No return values are used anywhere, so switch the functions to be void
and add support for error reporting using errp for use in next patches.
Signed-off-by: Martin Kletzander
Reviewed-by: Daniel P. Berrangé
Message-ID:
Signed-off-by: Paolo Bonzini
---
hw/audio/hda-codec.c | 32 ++--
hw/audio/intel-hda.c | 4 +---
hw/audio/intel-hda.h | 2 +-
3 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index c51d8ba6177..a26048cf15e 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -675,7 +675,9 @@ static void hda_audio_stream(HDACodecDevice *hda, uint32_t
stnr, bool running, b
}
}
-static int hda_audio_init(HDACodecDevice *hda, const struct desc_codec *desc)
+static void hda_audio_init(HDACodecDevice *hda,
+ const struct desc_codec *desc,
+ Error **errp)
{
HDAAudioState *a = HDA_AUDIO(hda);
HDAAudioStream *st;
@@ -718,7 +720,6 @@ static int hda_audio_init(HDACodecDevice *hda, const struct
desc_codec *desc)
break;
}
}
-return 0;
}
static void hda_audio_exit(HDACodecDevice *hda)
@@ -848,37 +849,40 @@ static Property hda_audio_properties[] = {
DEFINE_PROP_END_OF_LIST(),
};
-static int hda_audio_init_output(HDACodecDevice *hda)
+static void hda_audio_init_output(HDACodecDevice *hda, Error **errp)
{
HDAAudioState *a = HDA_AUDIO(hda);
+const struct desc_codec *desc = _nomixemu;
if (!a->mixer) {
-return hda_audio_init(hda, _nomixemu);
-} else {
-return hda_audio_init(hda, _mixemu);
+desc = _mixemu;
}
+
+hda_audio_init(hda, desc, errp);
}
-static int hda_audio_init_duplex(HDACodecDevice *hda)
+static void hda_audio_init_duplex(HDACodecDevice *hda, Error **errp)
{
HDAAudioState *a = HDA_AUDIO(hda);
+const struct desc_codec *desc = _nomixemu;
if (!a->mixer) {
-return hda_audio_init(hda, _nomixemu);
-} else {
-return hda_audio_init(hda, _mixemu);
+desc = _mixemu;
}
+
+hda_audio_init(hda, desc, errp);
}
-static int hda_audio_init_micro(HDACodecDevice *hda)
+static void hda_audio_init_micro(HDACodecDevice *hda, Error **errp)
{
HDAAudioState *a = HDA_AUDIO(hda);
+const struct desc_codec *desc = _nomixemu;
if (!a->mixer) {
-return hda_audio_init(hda, _nomixemu);
-} else {
-return hda_audio_init(hda, _mixemu);
+desc = _mixemu;
}
+
+hda_audio_init(hda, desc, errp);
}
static void hda_audio_base_class_init(ObjectClass *klass, void *data)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index b9ed231fe84..78ff9f9a680 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -71,9 +71,7 @@ static void hda_codec_dev_realize(DeviceState *qdev, Error
**errp)
return;
}
bus->next_cad = dev->cad + 1;
-if (cdc->init(dev) != 0) {
-error_setg(errp, "HDA audio init failed");
-}
+cdc->init(dev, errp);
}
static void hda_codec_dev_unrealize(DeviceState *qdev)
diff --git a/hw/audio/intel-hda.h b/hw/audio/intel-hda.h
index f78c1833e34..8d710eee5d6 100644
--- a/hw/audio/intel-hda.h
+++ b/hw/audio/intel-hda.h
@@ -31,7 +31,7 @@ struct HDACodecBus {
struct HDACodecDeviceClass {
DeviceClass parent_class;
-int (*init)(HDACodecDevice *dev);
+void (*init)(HDACodecDevice *dev, Error **errp);
void (*exit)(HDACodecDevice *dev);
void (*command)(HDACodecDevice *dev, uint32_t nid, uint32_t data);
void (*stream)(HDACodecDevice *dev, uint32_t stnr, bool running, bool
output);
--
2.41.0
[PULL 4/9] hw/input/tsc210x: Extract common init code into new function
From: Martin Kletzander
This deduplicates several lines and will make future changes more
concise.
Signed-off-by: Martin Kletzander
Reviewed-by: Daniel P. Berrangé
Message-ID:
<1d75877cf4cc2a38f87633ff16f9fea3e1bb0c03.1650874791.git.mklet...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/input/tsc210x.c | 68 --
1 file changed, 24 insertions(+), 44 deletions(-)
diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
index 7eae5989f76..f568759e05a 100644
--- a/hw/input/tsc210x.c
+++ b/hw/input/tsc210x.c
@@ -30,6 +30,7 @@
#include "hw/input/tsc2xxx.h"
#include "hw/irq.h"
#include "migration/vmstate.h"
+#include "qapi/error.h"
#define TSC_DATA_REGISTERS_PAGE0x0
#define TSC_CONTROL_REGISTERS_PAGE 0x1
@@ -1069,20 +1070,10 @@ static const VMStateDescription vmstate_tsc2301 = {
.fields = vmstatefields_tsc210x,
};
-uWireSlave *tsc2102_init(qemu_irq pint)
+static void tsc210x_init(TSC210xState *s,
+ const char *name,
+ const VMStateDescription *vmsd)
{
-TSC210xState *s;
-
-s = g_new0(TSC210xState, 1);
-s->x = 160;
-s->y = 160;
-s->pressure = 0;
-s->precision = s->nextprecision = 0;
-s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, tsc210x_timer_tick, s);
-s->pint = pint;
-s->model = 0x2102;
-s->name = "tsc2102";
-
s->tr[0] = 0;
s->tr[1] = 1;
s->tr[2] = 1;
@@ -1104,13 +1095,29 @@ uWireSlave *tsc2102_init(qemu_irq pint)
tsc210x_reset(s);
-qemu_add_mouse_event_handler(tsc210x_touchscreen_event, s, 1,
-"QEMU TSC2102-driven Touchscreen");
+qemu_add_mouse_event_handler(tsc210x_touchscreen_event, s, 1, name);
AUD_register_card(s->name, >card);
qemu_register_reset((void *) tsc210x_reset, s);
-vmstate_register(NULL, 0, _tsc2102, s);
+vmstate_register(NULL, 0, vmsd, s);
+}
+
+uWireSlave *tsc2102_init(qemu_irq pint)
+{
+TSC210xState *s;
+
+s = g_new0(TSC210xState, 1);
+s->x = 160;
+s->y = 160;
+s->pressure = 0;
+s->precision = s->nextprecision = 0;
+s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, tsc210x_timer_tick, s);
+s->pint = pint;
+s->model = 0x2102;
+s->name = "tsc2102";
+
+tsc210x_init(s, "QEMU TSC2102-driven Touchscreen", _tsc2102);
return >chip;
}
@@ -1131,34 +1138,7 @@ uWireSlave *tsc2301_init(qemu_irq penirq, qemu_irq
kbirq, qemu_irq dav)
s->model = 0x2301;
s->name = "tsc2301";
-s->tr[0] = 0;
-s->tr[1] = 1;
-s->tr[2] = 1;
-s->tr[3] = 0;
-s->tr[4] = 1;
-s->tr[5] = 0;
-s->tr[6] = 1;
-s->tr[7] = 0;
-
-s->chip.opaque = s;
-s->chip.send = (void *) tsc210x_write;
-s->chip.receive = (void *) tsc210x_read;
-
-s->codec.opaque = s;
-s->codec.tx_swallow = (void *) tsc210x_i2s_swallow;
-s->codec.set_rate = (void *) tsc210x_i2s_set_rate;
-s->codec.in.fifo = s->in_fifo;
-s->codec.out.fifo = s->out_fifo;
-
-tsc210x_reset(s);
-
-qemu_add_mouse_event_handler(tsc210x_touchscreen_event, s, 1,
-"QEMU TSC2301-driven Touchscreen");
-
-AUD_register_card(s->name, >card);
-
-qemu_register_reset((void *) tsc210x_reset, s);
-vmstate_register(NULL, 0, _tsc2301, s);
+tsc210x_init(s, "QEMU TSC2301-driven Touchscreen", _tsc2301);
return >chip;
}
--
2.41.0
[PULL 6/9] hw/audio/lm4549: Add errp error reporting to init function
From: Martin Kletzander
This will be used in future commit.
Signed-off-by: Martin Kletzander
Reviewed-by: Daniel P. Berrangé
Message-ID:
Signed-off-by: Paolo Bonzini
---
hw/audio/lm4549.c | 3 ++-
hw/audio/lm4549.h | 3 ++-
hw/audio/pl041.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/audio/lm4549.c b/hw/audio/lm4549.c
index 32b1481b561..418041bc9c6 100644
--- a/hw/audio/lm4549.c
+++ b/hw/audio/lm4549.c
@@ -276,7 +276,8 @@ static int lm4549_post_load(void *opaque, int version_id)
return 0;
}
-void lm4549_init(lm4549_state *s, lm4549_callback data_req_cb, void* opaque)
+void lm4549_init(lm4549_state *s, lm4549_callback data_req_cb, void* opaque,
+ Error **errp)
{
struct audsettings as;
diff --git a/hw/audio/lm4549.h b/hw/audio/lm4549.h
index aba9bb5b077..61c3ab12dd3 100644
--- a/hw/audio/lm4549.h
+++ b/hw/audio/lm4549.h
@@ -36,7 +36,8 @@ typedef struct {
extern const VMStateDescription vmstate_lm4549_state;
-void lm4549_init(lm4549_state *s, lm4549_callback data_req, void *opaque);
+void lm4549_init(lm4549_state *s, lm4549_callback data_req, void *opaque,
+ Error **errp);
uint32_t lm4549_read(lm4549_state *s, hwaddr offset);
void lm4549_write(lm4549_state *s, hwaddr offset, uint32_t value);
uint32_t lm4549_write_samples(lm4549_state *s, uint32_t left, uint32_t right);
diff --git a/hw/audio/pl041.c b/hw/audio/pl041.c
index 03acd4fe344..868dffbfd32 100644
--- a/hw/audio/pl041.c
+++ b/hw/audio/pl041.c
@@ -564,7 +564,7 @@ static void pl041_realize(DeviceState *dev, Error **errp)
}
/* Init the codec */
-lm4549_init(>codec, _request_data, (void *)s);
+lm4549_init(>codec, _request_data, (void *)s, errp);
}
static const VMStateDescription vmstate_pl041_regfile = {
--
2.41.0
[PULL 9/9] vl: recognize audiodev groups in configuration files
This is necessary for the q35 configuration tests to pass,
once audiodev becomes mandatory.
Signed-off-by: Paolo Bonzini
---
docs/config/q35-emulated.cfg | 4
docs/config/q35-virtio-graphical.cfg | 4
softmmu/vl.c | 10 ++
3 files changed, 18 insertions(+)
diff --git a/docs/config/q35-emulated.cfg b/docs/config/q35-emulated.cfg
index c8806e6d362..b4bd7e858a9 100644
--- a/docs/config/q35-emulated.cfg
+++ b/docs/config/q35-emulated.cfg
@@ -288,3 +288,7 @@
driver = "hda-duplex"
bus = "ich9-hda-audio.0"
cad = "0"
+ audiodev = "audiodev0"
+
+[audiodev "audiodev0"]
+ driver = "none" # CHANGE ME
diff --git a/docs/config/q35-virtio-graphical.cfg
b/docs/config/q35-virtio-graphical.cfg
index 148b5d2c5e4..820860aefe0 100644
--- a/docs/config/q35-virtio-graphical.cfg
+++ b/docs/config/q35-virtio-graphical.cfg
@@ -248,3 +248,7 @@
driver = "hda-duplex"
bus = "sound.0"
cad = "0"
+ audiodev = "audiodev0"
+
+[audiodev "audiodev0"]
+ driver = "none" # CHANGE ME
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3db4fd26808..db04f98c36f 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2125,6 +2125,7 @@ static int global_init_func(void *opaque, QemuOpts *opts,
Error **errp)
static bool is_qemuopts_group(const char *group)
{
if (g_str_equal(group, "object") ||
+g_str_equal(group, "audiodev") ||
g_str_equal(group, "machine") ||
g_str_equal(group, "smp-opts") ||
g_str_equal(group, "boot-opts")) {
@@ -2140,6 +2141,15 @@ static void qemu_record_config_group(const char *group,
QDict *dict,
Visitor *v = qobject_input_visitor_new_keyval(QOBJECT(dict));
object_option_add_visitor(v);
visit_free(v);
+
+} else if (g_str_equal(group, "audiodev")) {
+Audiodev *dev = NULL;
+Visitor *v = qobject_input_visitor_new_keyval(QOBJECT(dict));
+if (visit_type_Audiodev(v, NULL, , errp)) {
+audio_define(dev);
+}
+visit_free(v);
+
} else if (g_str_equal(group, "machine")) {
/*
* Cannot merge string-valued and type-safe dictionaries, so JSON
--
2.41.0
[PULL 8/9] tests/qtest: Specify audiodev= and -audiodev
From: Martin Kletzander
This will enable removing deprecated default audiodev support.
I did not figure out how to make the audiodev represented as an
interface node, so this is a workaround. I am not sure what would be
the proper way.
Signed-off-by: Martin Kletzander
Reviewed-by: Daniel P. Berrangé
Message-ID:
<6e7f2808dd40679a415812767b88f2a411fc137f.1650874791.git.mklet...@redhat.com>
Signed-off-by: Paolo Bonzini
---
tests/qtest/es1370-test.c | 3 ++-
tests/qtest/fuzz/generic_fuzz_configs.h | 6 --
tests/qtest/intel-hda-test.c| 15 ++-
3 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/es1370-test.c b/tests/qtest/es1370-test.c
index 97ab65c4357..8387e74193b 100644
--- a/tests/qtest/es1370-test.c
+++ b/tests/qtest/es1370-test.c
@@ -46,7 +46,8 @@ static void *es1370_create(void *pci_bus, QGuestAllocator
*alloc, void *addr)
static void es1370_register_nodes(void)
{
QOSGraphEdgeOptions opts = {
-.extra_device_opts = "addr=04.0",
+.extra_device_opts = "addr=04.0,audiodev=audio0",
+.before_cmd_line = "-audiodev driver=none,id=audio0",
};
add_qpci_address(, &(QPCIAddress) { .devfn = QPCI_DEVFN(4, 0) });
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 50689da6539..4d7c8ca4ece 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -106,8 +106,10 @@ const generic_fuzz_config predefined_configs[] = {
},{
.name = "intel-hda",
.args = "-machine q35 -nodefaults -device intel-hda,id=hda0 "
-"-device hda-output,bus=hda0.0 -device hda-micro,bus=hda0.0 "
-"-device hda-duplex,bus=hda0.0",
+"-audiodev driver=none,id=audio0",
+"-device hda-output,bus=hda0.0,audiodev=audio0 "
+"-device hda-micro,bus=hda0.0,audiodev=audio0 "
+"-device hda-duplex,bus=hda0.0,audiodev=audio0",
.objects = "intel-hda",
},{
.name = "ide-hd",
diff --git a/tests/qtest/intel-hda-test.c b/tests/qtest/intel-hda-test.c
index d4a8db6fd60..663bb6c4854 100644
--- a/tests/qtest/intel-hda-test.c
+++ b/tests/qtest/intel-hda-test.c
@@ -11,20 +11,24 @@
#include "libqtest-single.h"
#define HDA_ID "hda0"
-#define CODEC_DEVICES " -device hda-output,bus=" HDA_ID ".0" \
- " -device hda-micro,bus=" HDA_ID ".0" \
- " -device hda-duplex,bus=" HDA_ID ".0"
+#define AUDIODEV " -audiodev driver=none,id=audio0 "
+#define AUDIODEV_REF "audiodev=audio0"
+#define CODEC_DEVICES " -device hda-output,bus=" HDA_ID ".0," AUDIODEV_REF \
+ " -device hda-micro,bus=" HDA_ID ".0," AUDIODEV_REF \
+ " -device hda-duplex,bus=" HDA_ID ".0," AUDIODEV_REF
/* Tests only initialization so far. TODO: Replace with functional tests */
static void ich6_test(void)
{
-qtest_start("-machine pc -device intel-hda,id=" HDA_ID CODEC_DEVICES);
+qtest_start(AUDIODEV "-machine pc -device intel-hda,id=" HDA_ID
CODEC_DEVICES);
qtest_end();
}
static void ich9_test(void)
{
-qtest_start("-machine q35 -device ich9-intel-hda,bus=pcie.0,addr=1b.0,id="
+qtest_start("-machine q35"
+AUDIODEV
+"-device ich9-intel-hda,bus=pcie.0,addr=1b.0,id="
HDA_ID CODEC_DEVICES);
qtest_end();
}
@@ -39,6 +43,7 @@ static void test_issue542_ich6(void)
QTestState *s;
s = qtest_init("-nographic -nodefaults -M pc-q35-6.2 "
+ AUDIODEV
"-device intel-hda,id=" HDA_ID CODEC_DEVICES);
qtest_outl(s, 0xcf8, 0x8804);
--
2.41.0
[PULL 7/9] hw/display/xlnx_dp.c: Add audiodev property
From: Martin Kletzander
There was no way to set this and we need that for it to be able to properly
initialise.
Signed-off-by: Martin Kletzander
Message-ID:
<16963256573fcbfa7720aa2fd000ba74a4055222.1650874791.git.mklet...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/display/xlnx_dp.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 43c7dd8e9cd..341e91e886f 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1385,6 +1385,11 @@ static void xlnx_dp_reset(DeviceState *dev)
xlnx_dp_update_irq(s);
}
+static Property xlnx_dp_device_properties[] = {
+DEFINE_AUDIO_PROPERTIES(XlnxDPState, aud_card),
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void xlnx_dp_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
@@ -1392,6 +1397,7 @@ static void xlnx_dp_class_init(ObjectClass *oc, void
*data)
dc->realize = xlnx_dp_realize;
dc->vmsd = _dp;
dc->reset = xlnx_dp_reset;
+device_class_set_props(dc, xlnx_dp_device_properties);
}
static const TypeInfo xlnx_dp_info = {
--
2.41.0
[PULL 3/9] qemu/timer: Add host ticks function for RISC-V
From: LIU Zhiwei
Signed-off-by: LIU Zhiwei
Message-ID: <20230911063223.742-1-zhiwei_...@linux.alibaba.com>
Signed-off-by: Paolo Bonzini
---
include/qemu/timer.h | 22 ++
1 file changed, 22 insertions(+)
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 9a91cb1248a..9a366e551fb 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -979,6 +979,28 @@ static inline int64_t cpu_get_host_ticks(void)
return cur - ofs;
}
+#elif defined(__riscv) && __riscv_xlen == 32
+static inline int64_t cpu_get_host_ticks(void)
+{
+uint32_t lo, hi, tmph;
+do {
+asm volatile("RDTIMEH %0\n\t"
+ "RDTIME %1\n\t"
+ "RDTIMEH %2"
+ : "=r"(hi), "=r"(lo), "=r"(tmph));
+} while (unlikely(tmph != hi));
+return lo | (uint64_t)hi << 32;
+}
+
+#elif defined(__riscv) && __riscv_xlen > 32
+static inline int64_t cpu_get_host_ticks(void)
+{
+int64_t val;
+
+asm volatile("RDTIME %0" : "=r"(val));
+return val;
+}
+
#else
/* The host CPU doesn't have an easily accessible cycle counter.
Just return a monotonically increasing value. This will be
--
2.41.0
[PULL 1/9] target/i386: enumerate bit 56 of MSR_IA32_VMX_BASIC
On parts that enumerate IA32_VMX_BASIC MSR bit as 1, any exception vector
can be delivered with or without an error code if the other consistency
checks are satisfied.
Signed-off-by: Paolo Bonzini
---
scripts/kvm/vmxcap | 1 +
target/i386/cpu.c | 1 +
target/i386/cpu.h | 1 +
3 files changed, 3 insertions(+)
diff --git a/scripts/kvm/vmxcap b/scripts/kvm/vmxcap
index ce27f5e635a..3fb4d5b3425 100755
--- a/scripts/kvm/vmxcap
+++ b/scripts/kvm/vmxcap
@@ -115,6 +115,7 @@ controls = [
(50, 53): 'VMCS memory type',
54: 'INS/OUTS instruction information',
55: 'IA32_VMX_TRUE_*_CTLS support',
+56: 'Skip checks on event error code',
},
msr = MSR_IA32_VMX_BASIC,
),
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b2a20365e10..d48607b4e1e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1340,6 +1340,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
.feat_names = {
[54] = "vmx-ins-outs",
[55] = "vmx-true-ctls",
+[56] = "vmx-any-errcode",
},
.msr = {
.index = MSR_IA32_VMX_BASIC,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index fbb05eace57..d1ffadd78be 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1039,6 +1039,7 @@ uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
#define MSR_VMX_BASIC_DUAL_MONITOR (1ULL << 49)
#define MSR_VMX_BASIC_INS_OUTS (1ULL << 54)
#define MSR_VMX_BASIC_TRUE_CTLS (1ULL << 55)
+#define MSR_VMX_BASIC_ANY_ERRCODE(1ULL << 56)
#define MSR_VMX_MISC_PREEMPTION_TIMER_SHIFT_MASK 0x1Full
#define MSR_VMX_MISC_STORE_LMA (1ULL << 5)
--
2.41.0
[PULL 2/9] target/i386: Export GDS_NO bit to guests
From: Pawan Gupta
Gather Data Sampling (GDS) is a side-channel attack using Gather
instructions. Some Intel processors will set ARCH_CAP_GDS_NO bit in
MSR IA32_ARCH_CAPABILITIES to report that they are not vulnerable to
GDS.
Make this bit available to guests.
Closes:
https://lore.kernel.org/qemu-devel/camgffemg6tnq0n3+4ojagxc8j0oevy60khzekxcbs3lok9v...@mail.gmail.com/
Reported-by: Jack Wang
Signed-off-by: Pawan Gupta
Tested-by: Jack Wang
Tested-by: Daniel Sneddon
Message-ID:
Signed-off-by: Paolo Bonzini
---
target/i386/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d48607b4e1e..f9e51a9d87e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1155,7 +1155,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
NULL, "sbdr-ssdp-no", "fbsdp-no", "psdp-no",
NULL, "fb-clear", NULL, NULL,
NULL, NULL, NULL, NULL,
-"pbrsb-no", NULL, NULL, NULL,
+"pbrsb-no", NULL, "gds-no", NULL,
NULL, NULL, NULL, NULL,
},
.msr = {
--
2.41.0
[PULL v2 0/9] i386, audio changes for 2023-09-22
The following changes since commit 005ad32358f12fe9313a4a01918a55e60d4f39e5: Merge tag 'pull-tpm-2023-09-12-3' of https://github.com/stefanberger/qemu-tpm into staging (2023-09-13 13:41:57 -0400) are available in the Git repository at: https://gitlab.com/bonzini/qemu.git tags/for-upstream for you to fetch changes up to adf7f6b72fb6d10e00e93d04dfa33ce8c5e384c8: vl: recognize audiodev groups in configuration files (2023-09-22 17:35:11 +0200) Zoltan suggested keeping the default audio backends, so I am sending a reduced version of the pull request. * add host ticks function for RISC-V * target/i386: Export GDS_NO bit * target/i386: add support for bit 56 of MSR_IA32_VMX_BASIC * first part of audiodev cleanups LIU Zhiwei (1): qemu/timer: Add host ticks function for RISC-V Martin Kletzander (5): hw/input/tsc210x: Extract common init code into new function hw/audio: Simplify hda audio init hw/audio/lm4549: Add errp error reporting to init function hw/display/xlnx_dp.c: Add audiodev property tests/qtest: Specify audiodev= and -audiodev Paolo Bonzini (2): target/i386: enumerate bit 56 of MSR_IA32_VMX_BASIC vl: recognize audiodev groups in configuration files Pawan Gupta (1): target/i386: Export GDS_NO bit to guests docs/config/q35-emulated.cfg| 4 ++ docs/config/q35-virtio-graphical.cfg| 4 ++ hw/audio/hda-codec.c| 32 +--- hw/audio/intel-hda.c| 4 +- hw/audio/intel-hda.h| 2 +- hw/audio/lm4549.c | 3 +- hw/audio/lm4549.h | 3 +- hw/audio/pl041.c| 2 +- hw/display/xlnx_dp.c| 6 +++ hw/input/tsc210x.c | 68 - include/qemu/timer.h| 22 +++ scripts/kvm/vmxcap | 1 + softmmu/vl.c| 10 + target/i386/cpu.c | 3 +- target/i386/cpu.h | 1 + tests/qtest/es1370-test.c | 3 +- tests/qtest/fuzz/generic_fuzz_configs.h | 6 ++- tests/qtest/intel-hda-test.c| 15 +--- 18 files changed, 115 insertions(+), 74 deletions(-) -- 2.41.0
Re: [PATCH 1/2] migration: Fix rdma migration failed
On Wed, Sep 20, 2023 at 05:04:11PM +0800, Li Zhijian wrote:
> From: Li Zhijian
>
> Destination will fail with:
> qemu-system-x86_64: rdma: Too many requests in this message
> (3638950032).Bailing.
>
> migrate with RDMA is different from tcp. RDMA has its own control
> message, and all traffic between RDMA_CONTROL_REGISTER_REQUEST and
> RDMA_CONTROL_REGISTER_FINISHED should not be disturbed.
>
> find_dirty_block() will be called during RDMA_CONTROL_REGISTER_REQUEST
> and RDMA_CONTROL_REGISTER_FINISHED, it will send a extra traffic to
> destination and cause migration to fail.
>
> Since there's no existing subroutine to indicate whether it's migrated
> by RDMA or not, and RDMA is not compatible with multifd, we use
> migrate_multifd() here.
>
> Fixes: 294e5a4034 ("multifd: Only flush once each full round of memory")
> Signed-off-by: Li Zhijian
> ---
> migration/ram.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 9040d66e61..89ae28e21a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1399,7 +1399,8 @@ static int find_dirty_block(RAMState *rs,
> PageSearchStatus *pss)
> pss->page = 0;
> pss->block = QLIST_NEXT_RCU(pss->block, next);
> if (!pss->block) {
> -if (!migrate_multifd_flush_after_each_section()) {
> +if (migrate_multifd() &&
> +!migrate_multifd_flush_after_each_section()) {
> QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
> int ret = multifd_send_sync_main(f);
> if (ret < 0) {
> --
> 2.31.1
>
Maybe better to put that check at the entry of
migrate_multifd_flush_after_each_section()?
I also hope that some day there's no multifd function called in generic
migration code paths..
--
Peter Xu
Re: [PATCH 2/4] hw/misc/arm_sysctl.c: Avoid shadowing local variable
On 22/9/23 17:29, Peter Maydell wrote: Avoid shadowing a local variable in arm_sysctl_write(): ../../hw/misc/arm_sysctl.c: In function ‘arm_sysctl_write’: ../../hw/misc/arm_sysctl.c:537:26: warning: declaration of ‘val’ shadows a parameter [-Wshadow=local] 537 | uint32_t val; | ^~~ ../../hw/misc/arm_sysctl.c:388:39: note: shadowed declaration is here 388 | uint64_t val, unsigned size) | ~^~~ Signed-off-by: Peter Maydell --- hw/misc/arm_sysctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 3/4] hw/arm/smmuv3.c: Avoid shadowing variable
On 22/9/23 17:29, Peter Maydell wrote:
Avoid shadowing a variable in smmuv3_notify_iova():
../../hw/arm/smmuv3.c: In function ‘smmuv3_notify_iova’:
../../hw/arm/smmuv3.c:1043:23: warning: declaration of ‘event’ shadows a
previous local [-Wshadow=local]
1043 | SMMUEventInfo event = {.inval_ste_allowed = true};
| ^
../../hw/arm/smmuv3.c:1038:19: note: shadowed declaration is here
1038 | IOMMUTLBEvent event;
| ^
Signed-off-by: Peter Maydell
---
hw/arm/smmuv3.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 1/4] hw/intc/arm_gicv3_its: Avoid shadowing variable in do_process_its_cmd()
On 22/9/23 17:29, Peter Maydell wrote:
Avoid shadowing a local variable in do_process_its_cmd():
../../hw/intc/arm_gicv3_its.c:548:17: warning: declaration of ‘ite’ shadows a
previous local [-Wshadow=compatible-local]
548 | ITEntry ite = {};
| ^~~
../../hw/intc/arm_gicv3_its.c:518:13: note: shadowed declaration is here
518 | ITEntry ite;
| ^~~
Signed-off-by: Peter Maydell
---
hw/intc/arm_gicv3_its.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [ANNOUNCE] QEMU 8.1.1 Stable released
22.09.2023 18:18, Michael Tokarev wrote: This update contains a pile of fixes for various architectures/subsystems, fixing a number of various bugs. Unfortunately some known bugs remain unfixed still, and hopefully will be addressed in subsequent 8.1.x releases. In particular, the following issues (which exists in 8.1.0 release too) remain unfixed in this release: https://gitlab.com/qemu-project/qemu/-/issues/1826 https://gitlab.com/qemu-project/qemu/-/issues/1834 https://gitlab.com/qemu-project/qemu/-/issues/1846 And here are another 2 incarnations of the same theme: https://gitlab.com/qemu-project/qemu/-/issues/1864 These are *not* fixed in 8.1.1. /mjt
Re: [PATCH v5] hw/i386/pc: improve physical address space bound check for 32-bit x86 systems
On 22/9/23 17:01, Ani Sinha wrote:
32-bit x86 systems do not have a reserved memory for hole64. On those 32-bit
systems without PSE36 or PAE CPU features, hotplugging memory devices are not
supported by QEMU as QEMU always places hotplugged memory above 4 GiB boundary
which is beyond the physical address space of the processor. Linux guests also
does not support memory hotplug on those systems. Please see Linux
kernel commit b59d02ed08690 ("mm/memory_hotplug: disable the functionality
for 32b") for more details.
Therefore, the maximum limit of the guest physical address in the absence of
additional memory devices effectively coincides with the end of
"above 4G memory space" region for 32-bit x86 without PAE/PSE36. When users
configure additional memory devices, after properly accounting for the
additional device memory region to find the maximum value of the guest
physical address, the address will be outside the range of the processor's
physical address space.
This change adds improvements to take above into consideration.
For example, previously this was allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
With this change now it is no longer allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
qemu-system-x86_64: Address space limit 0x < 0x2bfff phys-bits too
low (32)
However, the following are allowed since on both cases physical address
space of the processor is 36 bits:
$ ./qemu-system-x86_64 -cpu pentium2 -m size=10G
$ ./qemu-system-x86_64 -cpu pentium,pse36=on -m size=10G
For 32-bit, without PAE/PSE36, hotplugging additional memory is no longer
allowed.
$ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
$ ./qemu-system-i386 -machine q35 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
A new compatibility flag is introduced to make sure pc_max_used_gpa() keeps
returning the old value for machines 8.1 and older.
Therefore, the above is still allowed for older machine types in order to
support
compatibility. Hence, the following still works:
$ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2
$ ./qemu-system-i386 -machine pc-q35-8.1 -m size=1G,maxmem=3G,slots=2
Further, following is also allowed as with PSE36, the processor has 36-bit
address space:
$ ./qemu-system-i386 -cpu 486,pse36=on -m size=1G,maxmem=3G,slots=2
After calling CPUID with EAX=0x8001, all AMD64 compliant processors
have the longmode-capable-bit turned on in the extended feature flags (bit 29)
in EDX. The absence of CPUID longmode can be used to differentiate between
32-bit and 64-bit processors and is the recommended approach. QEMU takes this
approach elsewhere (for example, please see x86_cpu_realizefn()), With
this change, pc_max_used_gpa() also uses the same method to detect 32-bit
processors.
Unit tests are modified to not run 32-bit x86 tests that use memory hotplug.
Suggested-by: David Hildenbrand
Signed-off-by: Ani Sinha
Reviewed-by: David Hildenbrand
---
hw/i386/pc.c | 30 +++---
hw/i386/pc_piix.c | 4
hw/i386/pc_q35.c | 2 ++
include/hw/i386/pc.h | 6 ++
tests/qtest/bios-tables-test.c | 26 ++
tests/qtest/numa-test.c| 7 ++-
6 files changed, 63 insertions(+), 12 deletions(-)
changelog:
v5: addressed phil's suggestions in code reorg to make it cleaner.
v4: address comments from v3. Fix a bug where compat knob was absent
from q35 machines. Commit message adjustment.
v3: still accounting for additional memory device region above 4G.
unit tests fixed (not running for 32-bit where mem hotplug is used).
v2: removed memory hotplug region from max_gpa. added compat knobs.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3db0743f31..f8d1cd1362 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -907,13 +907,37 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
{
X86CPU *cpu = X86_CPU(first_cpu);
+PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+MachineState *ms = MACHINE(pcms);
+uint64_t devmem_start = 0;
+ram_addr_t devmem_size = 0;
-/* 32-bit systems don't have hole64 thus return max CPU address */
-if (cpu->phys_bits <= 32) {
+if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
+/* 64-bit systems */
+return pc_pci_hole64_start() + pci_hole64_size - 1;
+}
+
+/*
+ * 32-bit systems don't have hole64 but they might have a region for
+ * memory devices. Even if additional hotplugged memory devices might
+ * not be usable by most guest OSes, we need to still consider them for
+ * calculating the highest possible GPA so that we can properly report
+ * if someone configures them on a CPU that cannot
[PATCH v2] qapi: provide a friendly string representation of QAPI classes
If printing a QAPI schema object for debugging we get the classname and a hex value for the instance: With this change we instead get the classname and the human friendly name of the QAPI type instance: Signed-off-by: Daniel P. Berrangé --- v1 was two & half years ago: https://mail.gnu.org/archive/html/qemu-devel/2021-03/msg01645.html scripts/qapi/schema.py | 6 ++ 1 file changed, 6 insertions(+) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 231ebf61ba..20ffacbdf0 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -73,6 +73,12 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None): self.features = features or [] self._checked = False +def __repr__(self): +if self.name is not None: +return "<%s:%s>" % (type(self).__name__, self.name) +else: +return "<%s>" % type(self).__name__ + def c_name(self): return c_name(self.name) -- 2.41.0
[PATCH 3/4] hw/arm/smmuv3.c: Avoid shadowing variable
Avoid shadowing a variable in smmuv3_notify_iova():
../../hw/arm/smmuv3.c: In function ‘smmuv3_notify_iova’:
../../hw/arm/smmuv3.c:1043:23: warning: declaration of ‘event’ shadows a
previous local [-Wshadow=local]
1043 | SMMUEventInfo event = {.inval_ste_allowed = true};
| ^
../../hw/arm/smmuv3.c:1038:19: note: shadowed declaration is here
1038 | IOMMUTLBEvent event;
| ^
Signed-off-by: Peter Maydell
---
hw/arm/smmuv3.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 1e9be8e89af..6f2b2bd45f9 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1040,8 +1040,8 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
SMMUv3State *s = sdev->smmu;
if (!tg) {
-SMMUEventInfo event = {.inval_ste_allowed = true};
-SMMUTransCfg *cfg = smmuv3_get_config(sdev, );
+SMMUEventInfo eventinfo = {.inval_ste_allowed = true};
+SMMUTransCfg *cfg = smmuv3_get_config(sdev, );
SMMUTransTableInfo *tt;
if (!cfg) {
--
2.34.1
[PATCH 1/4] hw/intc/arm_gicv3_its: Avoid shadowing variable in do_process_its_cmd()
Avoid shadowing a local variable in do_process_its_cmd():
../../hw/intc/arm_gicv3_its.c:548:17: warning: declaration of ‘ite’ shadows a
previous local [-Wshadow=compatible-local]
548 | ITEntry ite = {};
| ^~~
../../hw/intc/arm_gicv3_its.c:518:13: note: shadowed declaration is here
518 | ITEntry ite;
| ^~~
Signed-off-by: Peter Maydell
---
hw/intc/arm_gicv3_its.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 5f552b4d37f..52e9aca9c65 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -545,10 +545,10 @@ static ItsCmdResult do_process_its_cmd(GICv3ITSState *s,
uint32_t devid,
}
if (cmdres == CMD_CONTINUE_OK && cmd == DISCARD) {
-ITEntry ite = {};
+ITEntry i = {};
/* remove mapping from interrupt translation table */
-ite.valid = false;
-return update_ite(s, eventid, , ) ? CMD_CONTINUE_OK :
CMD_STALL;
+i.valid = false;
+return update_ite(s, eventid, , ) ? CMD_CONTINUE_OK : CMD_STALL;
}
return CMD_CONTINUE_OK;
}
--
2.34.1
[PATCH 4/4] hw/arm/smmuv3-internal.h: Don't use locals in statement macros
The STE_CTXPTR() and STE_S2TTB() macros both extract two halves
of an address from fields in the STE and combine them into a
single value to return. The current code for this uses a GCC
statement expression. There are two problems with this:
(1) The type chosen for the variable in the statement expr
is 'unsigned long', which might not be 64 bits
(2) the name chosen for the variable causes -Wshadow warnings
because it's the same as a variable in use at the callsite:
In file included from ../../hw/arm/smmuv3.c:34:
../../hw/arm/smmuv3.c: In function ‘smmu_get_cd’:
../../hw/arm/smmuv3-internal.h:538:23: warning: declaration of ‘addr’ shadows a
previous local [-Wshadow=compatible-local]
538 | unsigned long addr; \
| ^~~~
../../hw/arm/smmuv3.c:339:23: note: in expansion of macro ‘STE_CTXPTR’
339 | dma_addr_t addr = STE_CTXPTR(ste);
| ^~
../../hw/arm/smmuv3.c:339:16: note: shadowed declaration is here
339 | dma_addr_t addr = STE_CTXPTR(ste);
|^~~~
Sidestep both of these problems by just using a single
expression rather than a statement expr.
For CMD_ADDR, we got the type of the variable right but still
run into -Wshadow problems:
In file included from ../../hw/arm/smmuv3.c:34:
../../hw/arm/smmuv3.c: In function ‘smmuv3_range_inval’:
../../hw/arm/smmuv3-internal.h:334:22: warning: declaration of ‘addr’ shadows a
previous local [-Wshadow=compatible-local]
334 | uint64_t addr = high << 32 | (low << 12); \
| ^~~~
../../hw/arm/smmuv3.c:1104:28: note: in expansion of macro ‘CMD_ADDR’
1104 | dma_addr_t end, addr = CMD_ADDR(cmd);
|^~~~
../../hw/arm/smmuv3.c:1104:21: note: shadowed declaration is here
1104 | dma_addr_t end, addr = CMD_ADDR(cmd);
| ^~~~
so convert it too.
CD_TTB has neither problem, but it is the only other macro in
the file that uses this pattern, so we convert it also for
consistency's sake.
We use extract64() rather than extract32() to avoid having
to explicitly cast the result to uint64_t.
Signed-off-by: Peter Maydell
---
hw/arm/smmuv3-internal.h | 41 +---
1 file changed, 13 insertions(+), 28 deletions(-)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 6d1c1edab7b..648c2e37a27 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -328,12 +328,9 @@ enum { /* Command completion notification */
#define CMD_TTL(x) extract32((x)->word[2], 8 , 2)
#define CMD_TG(x) extract32((x)->word[2], 10, 2)
#define CMD_STE_RANGE(x)extract32((x)->word[2], 0 , 5)
-#define CMD_ADDR(x) ({\
-uint64_t high = (uint64_t)(x)->word[3]; \
-uint64_t low = extract32((x)->word[2], 12, 20);\
-uint64_t addr = high << 32 | (low << 12); \
-addr; \
-})
+#define CMD_ADDR(x) \
+(((uint64_t)((x)->word[3]) << 32) | \
+ ((extract64((x)->word[2], 12, 20)) << 12))
#define SMMU_FEATURE_2LVL_STE (1 << 0)
@@ -533,21 +530,13 @@ typedef struct CD {
#define STE_S2S(x) extract32((x)->word[5], 25, 1)
#define STE_S2R(x) extract32((x)->word[5], 26, 1)
-#define STE_CTXPTR(x) \
-({ \
-unsigned long addr; \
-addr = (uint64_t)extract32((x)->word[1], 0, 16) << 32; \
-addr |= (uint64_t)((x)->word[0] & 0xffc0); \
-addr; \
-})
+#define STE_CTXPTR(x) \
+((extract64((x)->word[1], 0, 16) << 32) | \
+ ((x)->word[0] & 0xffc0))
-#define STE_S2TTB(x)\
-({ \
-unsigned long addr; \
-addr = (uint64_t)extract32((x)->word[7], 0, 16) << 32; \
-addr |= (uint64_t)((x)->word[6] & 0xfff0); \
-addr; \
-})
+#define STE_S2TTB(x)\
+((extract64((x)->word[7], 0, 16) << 32) | \
+ ((x)->word[6] & 0xfff0))
static inline int oas2bits(int oas_field)
{
@@ -585,14 +574,10 @@ static inline int pa_range(STE *ste)
#define CD_VALID(x) extract32((x)->word[0], 31, 1)
#define CD_ASID(x)extract32((x)->word[1], 16, 16)
-#define CD_TTB(x, sel) \
-({ \
-uint64_t hi, lo;
[PATCH 2/4] hw/misc/arm_sysctl.c: Avoid shadowing local variable
Avoid shadowing a local variable in arm_sysctl_write():
../../hw/misc/arm_sysctl.c: In function ‘arm_sysctl_write’:
../../hw/misc/arm_sysctl.c:537:26: warning: declaration of ‘val’ shadows a
parameter [-Wshadow=local]
537 | uint32_t val;
| ^~~
../../hw/misc/arm_sysctl.c:388:39: note: shadowed declaration is here
388 | uint64_t val, unsigned size)
| ~^~~
Signed-off-by: Peter Maydell
---
hw/misc/arm_sysctl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/misc/arm_sysctl.c b/hw/misc/arm_sysctl.c
index 42d46938543..3e4f4b05244 100644
--- a/hw/misc/arm_sysctl.c
+++ b/hw/misc/arm_sysctl.c
@@ -534,12 +534,12 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
s->sys_cfgstat |= 2;/* error */
}
} else {
-uint32_t val;
+uint32_t data;
if (!vexpress_cfgctrl_read(s, dcc, function, site, position,
- device, )) {
+ device, )) {
s->sys_cfgstat |= 2;/* error */
} else {
-s->sys_cfgdata = val;
+s->sys_cfgdata = data;
}
}
}
--
2.34.1
[PATCH 0/4] arm: fix some -Wshadow warnings
These patches fix some -Wshadow warnings in arm related code. -- PMM Peter Maydell (4): hw/intc/arm_gicv3_its: Avoid shadowing variable in do_process_its_cmd() hw/misc/arm_sysctl.c: Avoid shadowing local variable hw/arm/smmuv3.c: Avoid shadowing variable hw/arm/smmuv3-internal.h: Don't use locals in statement macros hw/arm/smmuv3-internal.h | 41 +--- hw/arm/smmuv3.c | 4 ++-- hw/intc/arm_gicv3_its.c | 6 +++--- hw/misc/arm_sysctl.c | 6 +++--- 4 files changed, 21 insertions(+), 36 deletions(-) -- 2.34.1
Re: [PATCH v5 2/5] softmmu: Support concurrent bounce buffers
On Wed, Sep 20, 2023 at 01:06:19AM -0700, Mattias Nissler wrote: > When DMA memory can't be directly accessed, as is the case when > running the device model in a separate process without shareable DMA > file descriptors, bounce buffering is used. > > It is not uncommon for device models to request mapping of several DMA > regions at the same time. Examples include: > * net devices, e.g. when transmitting a packet that is split across >several TX descriptors (observed with igb) > * USB host controllers, when handling a packet with multiple data TRBs >(observed with xhci) > > Previously, qemu only provided a single bounce buffer per AddressSpace > and would fail DMA map requests while the buffer was already in use. In > turn, this would cause DMA failures that ultimately manifest as hardware > errors from the guest perspective. > > This change allocates DMA bounce buffers dynamically instead of > supporting only a single buffer. Thus, multiple DMA mappings work > correctly also when RAM can't be mmap()-ed. > > The total bounce buffer allocation size is limited individually for each > AddressSpace. The default limit is 4096 bytes, matching the previous > maximum buffer size. A new x-max-bounce-buffer-size parameter is > provided to configure the limit for PCI devices. > > Signed-off-by: Mattias Nissler Acked-by: Peter Xu -- Peter Xu
Re: [PATCH 00/52] migration/rdma: Error handling fixes
On Thu, Sep 21, 2023 at 08:27:24AM +, Zhijian Li (Fujitsu) wrote: > I'm worried that I may not have enough time, ability, or environment to > review/test > the RDMA patches. but for this patch set, i will take a look later. That'll be helpful, thanks! So it seems maybe at least we should have an entry for rdma migration to reflect the state of the code there. AFAIU we don't strictly need a maintainer for the entries; an empty entry should suffice, which I can prepare a patch for it: RDMA Migration S: Odd Fixes F: migration/rdma* Zhijian, if you still want to get emails when someone changes the code, maybe you still wanted be listed as a reviewer (even if not a maintainer)? So that you don't necessarily need to review every time, but just in case you still want to get notified whenever someone changes it, that means one line added onto above: R: Li Zhijian Do you prefer me to add that R: for you when I send the maintainer file update? I'm curious whether Fujitsu is using this code in production, if so it'll be great if you can be supported as, perhaps part of, your job to maintain the rdma code. But maybe that's not the case. In all cases, thanks a lot for the helps already. -- Peter Xu
[PATCH] audio: don't abort on f32 audio format in wav backend
Print a debug message as is done for other unsupported audio formats
to give the user the chance to understand their mistake.
Signed-off-by: Daniel P. Berrangé
---
audio/wavaudio.c | 4
1 file changed, 4 insertions(+)
diff --git a/audio/wavaudio.c b/audio/wavaudio.c
index 6445a2cb90..e70e5ee0c3 100644
--- a/audio/wavaudio.c
+++ b/audio/wavaudio.c
@@ -97,6 +97,10 @@ static int wav_init_out(HWVoiceOut *hw, struct audsettings
*as,
dolog ("WAVE files can not handle 32bit formats\n");
return -1;
+case AUDIO_FORMAT_F32:
+dolog("WAVE files can not handle float formats\n");
+return -1;
+
default:
abort();
}
--
2.41.0
[ANNOUNCE] QEMU 8.1.1 Stable released
Hi everyone, The QEMU v8.1.1 stable release is now available. You can grab the tarball from our download page here: https://www.qemu.org/download/#source v8.1.1 is now tagged in the official qemu.git repository, and the stable-8.1 branch has been updated accordingly: https://gitlab.com/qemu-project/qemu/-/commits/stable-8.1?ref_type=heads This update contains a pile of fixes for various architectures/subsystems, fixing a number of various bugs. Unfortunately some known bugs remain unfixed still, and hopefully will be addressed in subsequent 8.1.x releases. In particular, the following issues (which exists in 8.1.0 release too) remain unfixed in this release: https://gitlab.com/qemu-project/qemu/-/issues/1826 https://gitlab.com/qemu-project/qemu/-/issues/1834 https://gitlab.com/qemu-project/qemu/-/issues/1846 The fix for these are available (which is commit 0d58c66068 "softmmu: Use async_run_on_cpu in tcg_commit", thanks to Richard Henderson), but it uncovers problems in other areas (eg https://gitlab.com/qemu-project/qemu/-/issues/1866 with at least two different issues) for which there's no fix in qemu master branch yet, so I had to delay inclusion of this one to stable-8.1. Thank you everyone who has been involved and helped with the stable series! Changelog: 6bb4a8a47a: Update version for 8.1.1 release (Michael Tokarev) 045fa84784 8e32ddff69: tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR (Marc-André Lureau) 56270e5d3d fb0a8b0e23: meson: Fix targetos match for illumos and Solaris. (Jonathan Perkin) 60da8301fe 297ec01f0b: s390x/ap: fix missing subsystem reset registration (Janosch Frank) 8b479229ff 48a35e12fa: ui: fix crash when there are no active_console (Marc-André Lureau) d4919bbcc2 04562ee88e: virtio-gpu/win32: set the destroy function on load (Marc-André Lureau) cae7dc1452 a7c272df82: target/riscv: Allocate itrigger timers only once (Akihiko Odaki) 7385e00665 4e3adce124: target/riscv/pmp.c: respect mseccfg.RLB for pmpaddrX changes (Leon Schuermann) 1d4fb5815c 3a2fc23563: target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0 (Daniel Henrique Barboza) b822207513 9ff3140631: hw/riscv: virt: Fix riscv,pmu DT node path (Conor Dooley) 2947da750e ae7d4d625c: linux-user/riscv: Use abi type for target_ucontext (LIU Zhiwei) 60a7f5c8fe 9382a9eafc: hw/intc: Make rtc variable names consistent (Jason Chien) 566dac7127 e0922b73ba: hw/intc: Fix upper/lower mtime write calculation (Jason Chien) 8ae20123b6 eda633a534: target/riscv: Fix zfa fleq.d and fltq.d (LIU Zhiwei) 6c24b6000b 4cc9f284d5: target/riscv: Fix page_check_range use in fault-only-first (LIU Zhiwei) 987e90cfd2 50f9464962: target/riscv/cpu.c: add zmmul isa string (Daniel Henrique Barboza) b9f83298b9 058096f1c5: hw/char/riscv_htif: Fix the console syscall on big endian hosts (Thomas Huth) 3d6251f416 c255946e3d: hw/char/riscv_htif: Fix printing of console characters on big endian hosts (Thomas Huth) 9832a670b3 682814e2a3: arm64: Restore trapless ptimer access (Colton Lewis) df33ce9b6d 92e2e6a867: virtio: Drop out of coroutine context in virtio_load() (Kevin Wolf) 989f72 95bef686e4: qxl: don't assert() if device isn't yet initialized (Marc-André Lureau) 93d4107937 90a0778421: hw/net/vmxnet3: Fix guest-triggerable assert() (Thomas Huth) 6356785daa b21a6e31a1: docs tests: Fix use of migrate_set_parameter (Markus Armbruster) 01bf87c8e3 bcd8e24308: qemu-options.hx: Rephrase the descriptions of the -hd* and -cdrom options (Thomas Huth) 25ec23ab3f 961faf3ddb: hw/i2c/aspeed: Fix TXBUF transmission start position error (Hang Yu) 9dc6f05cc8 97b8aa5ae9: hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode (Hang Yu) d5361580ac 9f89423537: hw/ide/ahci: fix broken SError handling (Niklas Cassel) e8f5ca57e4 7e85cb0db4: hw/ide/ahci: fix ahci_write_fis_sdb() (Niklas Cassel) 4448c345bc 1a16ce64fd: hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set (Niklas Cassel) 4fbd5a5202 d73b84d0b6: hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared (Niklas Cassel) 16cc9594d2 e2a5d9b3d9: hw/ide/ahci: simplify and document PxCI handling (Niklas Cassel) 1efefd13ca 2967dc8209: hw/ide/ahci: write D2H FIS when processing NCQ command (Niklas Cassel) c2e0495e3c c3461c6264: hw/ide/core: set ERR_STAT in unsupported command completion (Niklas Cassel) f64f1f8704 718209358f: target/ppc: Fix LQ, STQ register-pair order for big-endian (Nicholas Piggin) 9f54fef2c0 af03aeb631: target/ppc: Flush inputs to zero with NJ in ppc_store_vscr (Richard Henderson) 5358980d33 6ec65b69ba: hw/ppc/e500: fix broken snapshot replay (Maksim Kostin) 6864f05cb1 7b8589d7ce: ppc/vof: Fix missed fields in VOF cleanup (Nicholas Piggin) 0175121c6c cb6ccdc9ca: ui/dbus: Properly dispose touch/mouse dbus objects (Bilal Elmoussaoui) e975434d62 c1f27a0c6a: target/i386: raise FERR interrupt with iothread locked (Paolo Bonzini) e5e77f256f aec338d63b: linux-user: Adjust brk for load_bias (Richard
[ANNOUNCE] QEMU 8.0.5 Stable released
Hi everyone, The QEMU v8.0.5 stable release is now available. You can grab the tarball from our download page here: https://www.qemu.org/download/#source v8.0.5 is now tagged in the official qemu.git repository, and the stable-8.0 branch has been updated accordingly: https://gitlab.com/qemu-project/qemu/-/commits/stable-8.0?ref_type=heads This update contains usual pile of general fixes for various architectures and subsystems, and brings up a backport of re-entrancy fixes which landed in 8.1, together with all follow-ups and refinements to date: https://gitlab.com/qemu-project/qemu/-/issues/556 This should be last 8.0.x stable release, unless there's good compelling reason or big interest to continue 8.0.x branch maintenance. Thank you everyone who has been involved and helped with the stable series! Changelog: 6bbce8b464: Update version for 8.0.5 release (Michael Tokarev) fcf58d6f20 8e32ddff69: tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR (Marc-André Lureau) 0ef930a29f 297ec01f0b: s390x/ap: fix missing subsystem reset registration (Janosch Frank) 6c575436cd 48a35e12fa: ui: fix crash when there are no active_console (Marc-André Lureau) 36540b367e 4c46fe2ed4: hw/tpm: TIS on sysbus: Remove unsupport ppi command line option (Stefan Berger) 70c97e75d7 4e3adce124: target/riscv/pmp.c: respect mseccfg.RLB for pmpaddrX changes (Leon Schuermann) 1805e05db3 3a2fc23563: target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0 (Daniel Henrique Barboza) e94ea3c6db 9ff3140631: hw/riscv: virt: Fix riscv,pmu DT node path (Conor Dooley) 9bac2bcf10 ae7d4d625c: linux-user/riscv: Use abi type for target_ucontext (LIU Zhiwei) c00a9ec061 9382a9eafc: hw/intc: Make rtc variable names consistent (Jason Chien) fd1a0c89c6 e0922b73ba: hw/intc: Fix upper/lower mtime write calculation (Jason Chien) a57e4cc6fe 058096f1c5: hw/char/riscv_htif: Fix the console syscall on big endian hosts (Thomas Huth) aeb931d82b 24be3369ad: include/exec: Provide the tswap() functions for target independent code, too (Thomas Huth) 3af03de983 c255946e3d: hw/char/riscv_htif: Fix printing of console characters on big endian hosts (Thomas Huth) 53a4e7ef42 682814e2a3: arm64: Restore trapless ptimer access (Colton Lewis) 41af7a9bc4 92e2e6a867: virtio: Drop out of coroutine context in virtio_load() (Kevin Wolf) 5929f53091 95bef686e4: qxl: don't assert() if device isn't yet initialized (Marc-André Lureau) c68b844d33 90a0778421: hw/net/vmxnet3: Fix guest-triggerable assert() (Thomas Huth) 42edb4723a b21a6e31a1: docs tests: Fix use of migrate_set_parameter (Markus Armbruster) 45b61f730d bcd8e24308: qemu-options.hx: Rephrase the descriptions of the -hd* and -cdrom options (Thomas Huth) c2ec46c694 961faf3ddb: hw/i2c/aspeed: Fix TXBUF transmission start position error (Hang Yu) 4a398e64ba 97b8aa5ae9: hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode (Hang Yu) 4f6c553717 9f89423537: hw/ide/ahci: fix broken SError handling (Niklas Cassel) 9c7e2253eb 7e85cb0db4: hw/ide/ahci: fix ahci_write_fis_sdb() (Niklas Cassel) f7cca09987 1a16ce64fd: hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set (Niklas Cassel) 2eaf7775fc d73b84d0b6: hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared (Niklas Cassel) 7bcd32128b e2a5d9b3d9: hw/ide/ahci: simplify and document PxCI handling (Niklas Cassel) 362a4d8658 2967dc8209: hw/ide/ahci: write D2H FIS when processing NCQ command (Niklas Cassel) 67894ec9fd c3461c6264: hw/ide/core: set ERR_STAT in unsupported command completion (Niklas Cassel) 956b96f9e2 af03aeb631: target/ppc: Flush inputs to zero with NJ in ppc_store_vscr (Richard Henderson) ea25506b5d 7b8589d7ce: ppc/vof: Fix missed fields in VOF cleanup (Nicholas Piggin) fcb49ea23c 6ec65b69ba: hw/ppc/e500: fix broken snapshot replay (Maksim Kostin) e8bb4dc55a f187609f27: block-migration: Ensure we don't crash during migration cleanup (Fabiano Rosas) 3c934310ff 09a3fffae0: docs/about/license: Update LICENSE URL (Philippe Mathieu-Daudé) d4c0ac705d cd1e4db736: target/arm: Fix 64-bit SSRA (Richard Henderson) 09640031ed 4b3520fd93: target/arm: Fix SME ST1Q (Richard Henderson) f5cb21416e 1ab445af8c: accel/kvm: Specify default IPA size for arm64 (Akihiko Odaki) aa152711db 5e0d65909c: kvm: Introduce kvm_arch_get_default_type hook (Akihiko Odaki) f2f8e74ff4 d194362910: include/hw/virtio/virtio-gpu: Fix virtio-gpu with blob on big endian hosts (Thomas Huth) 96fd3b8508 6a2ea61518: target/s390x: Check reserved bits of VFMIN/VFMAX's M5 (Ilya Leoshkevich) 62ac9cbb6f 6db3518ba4: target/s390x: Fix VSTL with a large length (Ilya Leoshkevich) 14f78932e0 23e87d419f: target/s390x: Use a 16-bit immediate in VREP (Ilya Leoshkevich) 179a37924d 791b2b6a93: target/s390x: Fix the "ignored match" case in VSTRS (Ilya Leoshkevich) b4b3aac5b5 3b83079015: hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers (Bernhard Beschow) af0c16fae9 6ee960823d: Fixed incorrect LLONG alignment for
Re: [PATCH for-8.1] hw/rdma/vmw/pvrdma_cmd: Use correct struct in query_port()
Reviewed-by: Thomas Huth
Maybe this could go via qemu-trivial?
On 12/09/2023 16.08, Peter Maydell wrote:
Ping^2 for review/pickup by the rdma folks, please?
Is anybody still using this subsystem? ... if not, then it's maybe time to
set this on the deprecation list? ... just my 0.02 €.
Thomas
On Tue, 29 Aug 2023 at 16:49, Peter Maydell wrote:
On Tue, 25 Jul 2023 at 12:36, Peter Maydell wrote:
In query_port() we pass the address of a local pvrdma_port_attr
struct to the rdma_query_backend_port() function. Unfortunately,
rdma_backend_query_port() wants a pointer to a struct ibv_port_attr,
and the two are not the same length.
Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes
long, and ibv_port_attr is 52 bytes, because it has a few extra
fields at the end.
Fortunately, all we do with the attrs struct after the call is to
read a few specific fields out of it which are all at the same
offsets in both structs, so we can simply make the local variable the
correct type. This also lets us drop the cast (which should have
been a bit of a warning flag that we were doing something wrong
here).
Cc: qemu-sta...@nongnu.org
Signed-off-by: Peter Maydell
---
I don't know anything about the rdma code so this fix is based
purely on looking at the code, and is untested beyond just
make check/make check-avocado.
---
hw/rdma/vmw/pvrdma_cmd.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index c6ed0259821..d31c1875938 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union
pvrdma_cmd_req *req,
{
struct pvrdma_cmd_query_port *cmd = >query_port;
struct pvrdma_cmd_query_port_resp *resp = >query_port_resp;
-struct pvrdma_port_attr attrs = {};
+struct ibv_port_attr attrs = {};
if (cmd->port_num > MAX_PORTS) {
return -EINVAL;
}
-if (rdma_backend_query_port(>backend_dev,
-(struct ibv_port_attr *))) {
+if (rdma_backend_query_port(>backend_dev, )) {
return -ENOMEM;
}
Ping for review/testing by the rdma folks, please ?
Whose tree should this patch go through?
Re: [PATCH 11/11] qdev: Rework array properties based on list visitor
Kevin Wolf writes:
> Until now, array properties are actually implemented with a hack that
> uses multiple properties on the QOM level: a static "foo-len" property
> and after it is set, dynamically created "foo[i]" properties.
>
> In external interfaces (-device on the command line and device_add in
> QMP), this interface was broken by commit f3558b1b ('qdev: Base object
> creation on QDict rather than QemuOpts') because QDicts are unordered
> and therefore it could happen that QEMU tried to set the indexed
> properties before setting the length, which fails and effectively makes
> array properties inaccessible. In particular, this affects the 'ports'
> property of the 'rocker' device.
>
> This patch reworks the external interface so that instead of using a
> separate top-level property for the length and for each element, we use
> a single true array property that accepts a list value. In the external
> interfaces, this is naturally expressed as a JSON list and makes array
> properties accessible again.
>
> Creating an array property on the command line without using JSON format
> is currently not possible. This could be fixed by switching from
> QemuOpts to a keyval parser, which however requires consideration of the
> compatibility implications.
>
> All internal users of devices with array properties go through
> qdev_prop_set_array() at this point, so updating it takes care of all of
> them.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
> Signed-off-by: Kevin Wolf
> ---
> include/hw/qdev-properties.h | 23 ++--
> hw/core/qdev-properties-system.c | 2 +-
> hw/core/qdev-properties.c| 204 +++
> 3 files changed, 133 insertions(+), 96 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 7fa2fdb7c9..9370b36b72 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -61,7 +61,7 @@ extern const PropertyInfo qdev_prop_size;
> extern const PropertyInfo qdev_prop_string;
> extern const PropertyInfo qdev_prop_on_off_auto;
> extern const PropertyInfo qdev_prop_size32;
> -extern const PropertyInfo qdev_prop_arraylen;
> +extern const PropertyInfo qdev_prop_array;
> extern const PropertyInfo qdev_prop_link;
>
> #define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) { \
> @@ -115,8 +115,6 @@ extern const PropertyInfo qdev_prop_link;
> .bitmask= (_bitmask), \
> .set_default = false)
>
> -#define PROP_ARRAY_LEN_PREFIX "len-"
> -
> /**
> * DEFINE_PROP_ARRAY:
> * @_name: name of the array
> @@ -127,24 +125,21 @@ extern const PropertyInfo qdev_prop_link;
> * @_arrayprop: PropertyInfo defining what property the array elements have
> * @_arraytype: C type of the array elements
> *
> - * Define device properties for a variable-length array _name. A
> - * static property "len-arrayname" is defined. When the device creator
> - * sets this property to the desired length of array, further dynamic
> - * properties "arrayname[0]", "arrayname[1]", ... are defined so the
> - * device creator can set the array element values. Setting the
> - * "len-arrayname" property more than once is an error.
> + * Define device properties for a variable-length array _name. The array is
Please wrap comments around column 70. More of the same below, not
noted again.
> + * represented as a list in the visitor interface.
> + *
> + * @_arraytype is required to be movable with memcpy().
> *
> - * When the array length is set, the @_field member of the device
> + * When the array property is set, the @_field member of the device
> * struct is set to the array length, and @_arrayfield is set to point
> - * to (zero-initialised) memory allocated for the array. For a zero
> - * length array, @_field will be set to 0 and @_arrayfield to NULL.
> + * to the memory allocated for the array.
Worth mentioning that the @field member must be uint32_t?
> + *
> * It is the responsibility of the device deinit code to free the
> * @_arrayfield memory.
> */
> #define DEFINE_PROP_ARRAY(_name, _state, _field, \
>_arrayfield, _arrayprop, _arraytype) \
> -DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name), \
> -_state, _field, qdev_prop_arraylen, uint32_t, \
> +DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t, \
> .set_default = true, \
> .defval.u = 0, \
> .arrayinfo = &(_arrayprop),\
The backslashes are no longer aligned.
> diff --git a/hw/core/qdev-properties-system.c
> b/hw/core/qdev-properties-system.c
> index 6d5d43eda2..f557ee886e 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -450,7 +450,7 @@ static void
[PATCH v5] hw/i386/pc: improve physical address space bound check for 32-bit x86 systems
32-bit x86 systems do not have a reserved memory for hole64. On those 32-bit
systems without PSE36 or PAE CPU features, hotplugging memory devices are not
supported by QEMU as QEMU always places hotplugged memory above 4 GiB boundary
which is beyond the physical address space of the processor. Linux guests also
does not support memory hotplug on those systems. Please see Linux
kernel commit b59d02ed08690 ("mm/memory_hotplug: disable the functionality
for 32b") for more details.
Therefore, the maximum limit of the guest physical address in the absence of
additional memory devices effectively coincides with the end of
"above 4G memory space" region for 32-bit x86 without PAE/PSE36. When users
configure additional memory devices, after properly accounting for the
additional device memory region to find the maximum value of the guest
physical address, the address will be outside the range of the processor's
physical address space.
This change adds improvements to take above into consideration.
For example, previously this was allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
With this change now it is no longer allowed:
$ ./qemu-system-x86_64 -cpu pentium -m size=10G
qemu-system-x86_64: Address space limit 0x < 0x2bfff phys-bits too
low (32)
However, the following are allowed since on both cases physical address
space of the processor is 36 bits:
$ ./qemu-system-x86_64 -cpu pentium2 -m size=10G
$ ./qemu-system-x86_64 -cpu pentium,pse36=on -m size=10G
For 32-bit, without PAE/PSE36, hotplugging additional memory is no longer
allowed.
$ ./qemu-system-i386 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
$ ./qemu-system-i386 -machine q35 -m size=1G,maxmem=3G,slots=2
qemu-system-i386: Address space limit 0x < 0x1 phys-bits too
low (32)
A new compatibility flag is introduced to make sure pc_max_used_gpa() keeps
returning the old value for machines 8.1 and older.
Therefore, the above is still allowed for older machine types in order to
support
compatibility. Hence, the following still works:
$ ./qemu-system-i386 -machine pc-i440fx-8.1 -m size=1G,maxmem=3G,slots=2
$ ./qemu-system-i386 -machine pc-q35-8.1 -m size=1G,maxmem=3G,slots=2
Further, following is also allowed as with PSE36, the processor has 36-bit
address space:
$ ./qemu-system-i386 -cpu 486,pse36=on -m size=1G,maxmem=3G,slots=2
After calling CPUID with EAX=0x8001, all AMD64 compliant processors
have the longmode-capable-bit turned on in the extended feature flags (bit 29)
in EDX. The absence of CPUID longmode can be used to differentiate between
32-bit and 64-bit processors and is the recommended approach. QEMU takes this
approach elsewhere (for example, please see x86_cpu_realizefn()), With
this change, pc_max_used_gpa() also uses the same method to detect 32-bit
processors.
Unit tests are modified to not run 32-bit x86 tests that use memory hotplug.
Suggested-by: David Hildenbrand
Signed-off-by: Ani Sinha
Reviewed-by: David Hildenbrand
---
hw/i386/pc.c | 30 +++---
hw/i386/pc_piix.c | 4
hw/i386/pc_q35.c | 2 ++
include/hw/i386/pc.h | 6 ++
tests/qtest/bios-tables-test.c | 26 ++
tests/qtest/numa-test.c| 7 ++-
6 files changed, 63 insertions(+), 12 deletions(-)
changelog:
v5: addressed phil's suggestions in code reorg to make it cleaner.
v4: address comments from v3. Fix a bug where compat knob was absent
from q35 machines. Commit message adjustment.
v3: still accounting for additional memory device region above 4G.
unit tests fixed (not running for 32-bit where mem hotplug is used).
v2: removed memory hotplug region from max_gpa. added compat knobs.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3db0743f31..f8d1cd1362 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -907,13 +907,37 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
static hwaddr pc_max_used_gpa(PCMachineState *pcms, uint64_t pci_hole64_size)
{
X86CPU *cpu = X86_CPU(first_cpu);
+PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+MachineState *ms = MACHINE(pcms);
+uint64_t devmem_start = 0;
+ram_addr_t devmem_size = 0;
-/* 32-bit systems don't have hole64 thus return max CPU address */
-if (cpu->phys_bits <= 32) {
+if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
+/* 64-bit systems */
+return pc_pci_hole64_start() + pci_hole64_size - 1;
+}
+
+/*
+ * 32-bit systems don't have hole64 but they might have a region for
+ * memory devices. Even if additional hotplugged memory devices might
+ * not be usable by most guest OSes, we need to still consider them for
+ * calculating the highest possible GPA so that we can properly report
+ * if someone configures them on a CPU that cannot possibly address them.
+ */
+if
[RFC PATCH 2/3] migration/multifd: Decouple control flow from the SYNC packet
We currently have the sem_sync semaphore that is used:
1) on the sending side, to know when the multifd_send_thread has
finished sending the MULTIFD_FLAG_SYNC packet;
This is unnecessary. Multifd sends packets (not pages) one by one
and completion is already bound by both the channels_ready and sem
semaphores. The SYNC packet has nothing special that would require
it to have a separate semaphore on the sending side.
2) on the receiving side, to know when the multifd_recv_thread has
finished receiving the MULTIFD_FLAG_SYNC packet;
This is unnecessary because the multifd_recv_state->sem_sync
semaphore already does the same thing. We care that the SYNC arrived
from the source, knowing that the SYNC has been received by the recv
thread doesn't add anything.
3) on both sending and receiving sides, to wait for the multifd threads
to finish before cleaning up;
This happens because multifd_send_sync_main() blocks
ram_save_complete() from finishing until the semaphore is
posted. This is surprising and not documented.
Clarify the above situation by renaming 'sem_sync' to 'sem_done' and
making the #3 usage the main one. Stop tracking the SYNC packet on
source (#1) and leave multifd_recv_state->sem_sync untouched on the
destination (#2).
Due to the 'channels_ready' and 'sem' semaphores, we always send
packets in lockstep with switching MultiFDSendParams, so
p->pending_job is always either 1 or 0. The thread has no knowledge of
whether it will have more to send once it posts to
channels_ready. Send it on an extra loop so it sees no pending_job and
releases the semaphore.
Signed-off-by: Fabiano Rosas
---
migration/multifd.c| 89 --
migration/multifd.h| 8 ++--
migration/trace-events | 2 +-
3 files changed, 73 insertions(+), 26 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index d626740f2f..3d4a631915 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -541,7 +541,7 @@ void multifd_save_cleanup(void)
p->c = NULL;
qemu_mutex_destroy(>mutex);
qemu_sem_destroy(>sem);
-qemu_sem_destroy(>sem_sync);
+qemu_sem_destroy(>sem_done);
g_free(p->name);
p->name = NULL;
multifd_pages_clear(p->pages);
@@ -592,7 +592,7 @@ int multifd_send_sync_main(QEMUFile *f)
if (!migrate_multifd()) {
return 0;
-}
+
if (multifd_send_state->pages->num) {
if (multifd_send_pages(f) < 0) {
error_report("%s: multifd_send_pages fail", __func__);
@@ -600,6 +600,12 @@ int multifd_send_sync_main(QEMUFile *f)
}
}
+/* wait for all channels to be idle */
+for (i = 0; i < migrate_multifd_channels(); i++) {
+trace_multifd_send_sync_main_wait(p->id);
+qemu_sem_wait(_send_state->channels_ready);
+}
+
/*
* When using zero-copy, it's necessary to flush the pages before any of
* the pages can be sent again, so we'll make sure the new version of the
@@ -610,9 +616,46 @@ int multifd_send_sync_main(QEMUFile *f)
* to be less frequent, e.g. only after we finished one whole scanning of
* all the dirty bitmaps.
*/
-
flush_zero_copy = migrate_zero_copy_send();
+for (i = 0; i < migrate_multifd_channels(); i++) {
+MultiFDSendParams *p = _send_state->params[i];
+
+qemu_mutex_lock(>mutex);
+assert(!p->pending_job);
+qemu_mutex_unlock(>mutex);
+
+qemu_sem_post(>sem);
+qemu_sem_wait(>sem_done);
+
+if (flush_zero_copy && p->c && (multifd_zero_copy_flush(p->c) < 0)) {
+return -1;
+}
+}
+
+/*
+ * All channels went idle and have no more jobs. Unless we send
+ * them more work, we're good to allow any cleanup code to run at
+ * this point.
+ */
+
+return 0;
+}
+
+int multifd_send_sync_main(QEMUFile *f)
+{
+int i, ret;
+
+if (!migrate_multifd()) {
+return 0;
+}
+if (multifd_send_state->pages->num) {
+if (multifd_send_pages(f) < 0) {
+error_report("%s: multifd_send_pages fail", __func__);
+return -1;
+}
+}
+
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDSendParams *p = _send_state->params[i];
@@ -633,11 +676,21 @@ int multifd_send_sync_main(QEMUFile *f)
qemu_mutex_unlock(>mutex);
qemu_sem_post(>sem);
}
+
+for (i = 0; i < migrate_multifd_channels(); i++) {
+trace_multifd_send_wait(migrate_multifd_channels() - i);
+qemu_sem_wait(_send_state->channels_ready);
+}
+
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDSendParams *p = _send_state->params[i];
-trace_multifd_send_sync_main_wait(p->id);
-qemu_sem_wait(>sem_sync);
+qemu_mutex_lock(>mutex);
+assert(!p->pending_job);
+qemu_mutex_unlock(>mutex);
+
+qemu_sem_post(>sem);
+
[RFC PATCH 3/3] migration/multifd: Extract sem_done waiting into a function
This helps document the intent of the loop via the function name and
we can reuse this in the future.
Signed-off-by: Fabiano Rosas
---
migration/multifd.c | 38 +-
1 file changed, 5 insertions(+), 33 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 3d4a631915..159225530d 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -585,24 +585,14 @@ static int multifd_zero_copy_flush(QIOChannel *c)
return ret;
}
-int multifd_send_sync_main(QEMUFile *f)
+static int multifd_send_wait(void)
{
-int i;
bool flush_zero_copy;
-
-if (!migrate_multifd()) {
-return 0;
-
-if (multifd_send_state->pages->num) {
-if (multifd_send_pages(f) < 0) {
-error_report("%s: multifd_send_pages fail", __func__);
-return -1;
-}
-}
+int i;
/* wait for all channels to be idle */
for (i = 0; i < migrate_multifd_channels(); i++) {
-trace_multifd_send_sync_main_wait(p->id);
+trace_multifd_send_wait(migrate_multifd_channels() - i);
qemu_sem_wait(_send_state->channels_ready);
}
@@ -677,28 +667,10 @@ int multifd_send_sync_main(QEMUFile *f)
qemu_sem_post(>sem);
}
-for (i = 0; i < migrate_multifd_channels(); i++) {
-trace_multifd_send_wait(migrate_multifd_channels() - i);
-qemu_sem_wait(_send_state->channels_ready);
-}
-
-for (i = 0; i < migrate_multifd_channels(); i++) {
-MultiFDSendParams *p = _send_state->params[i];
-
-qemu_mutex_lock(>mutex);
-assert(!p->pending_job);
-qemu_mutex_unlock(>mutex);
-
-qemu_sem_post(>sem);
-qemu_sem_wait(>sem_done);
-
-if (flush_zero_copy && p->c && (multifd_zero_copy_flush(p->c) < 0)) {
-return -1;
-}
-}
+ret = multifd_send_wait();
trace_multifd_send_sync_main(multifd_send_state->packet_num);
-return 0;
+return ret;
}
static void *multifd_send_thread(void *opaque)
--
2.35.3
[RFC PATCH 0/3] migration/multifd: SYNC packet changes
I'm putting this RFC out early so we can discuss the issues around the SYNC packet of the multifd protocol. There's a related series posted by Elena Ufimtseva: https://lore.kernel.org/r/20230922065625.21848-1-elena.ufimts...@oracle.com My interest in this (aside from correctness) is that when doing the (upcoming) fixed-ram[1] migration, I would like to have multifd ignore the concept of packets altogether, since the file: migration is not synchronous. The main problem I hit is that multifd (ab)uses the knowledge that a sync packet is sent after a batch of pages and relies (perhaps inadvertently) on the last post to sem_sync to finish the migration. Which means that without the sync, the main thread just rushes and does cleanup while packets are still in flight. I have add another patch to this series that introduces a multifd-nopackets option (placeholder name), but it's probably too early to discuss that so I'm leaving it out. 1- https://lore.kernel.org/r/20230330180336.2791-1-faro...@suse.de Fabiano Rosas (3): migration/multifd: Move channels_ready semaphore migration/multifd: Decouple control flow from the SYNC packet migration/multifd: Extract sem_done waiting into a function migration/multifd.c| 97 +- migration/multifd.h| 8 ++-- migration/trace-events | 2 +- 3 files changed, 63 insertions(+), 44 deletions(-) -- 2.35.3
[RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
Commit d2026ee117 ("multifd: Fix the number of channels ready") moved
the "post" of channels_ready to the start of the multifd_send_thread()
loop and added a missing "wait" at multifd_send_sync_main(). While it
does work, the placement of the wait goes against what the rest of the
code does.
The sequence at multifd_send_thread() is:
qemu_sem_post(_send_state->channels_ready);
qemu_sem_wait(>sem);
if (flags & MULTIFD_FLAG_SYNC) {
qemu_sem_post(>sem_sync);
}
Which means that the sending thread makes itself available
(channels_ready) and waits for more work (sem). So the sequence in the
migration thread should be to check if any channel is available
(channels_ready), give it some work and set it off (sem):
qemu_sem_wait(_send_state->channels_ready);
qemu_sem_post(>sem);
if (flags & MULTIFD_FLAG_SYNC) {
qemu_sem_wait(>sem_sync);
}
The reason there's no deadlock today is that the migration thread
enqueues the SYNC packet right before the wait on channels_ready and
we end up taking advantage of the out-of-order post to sem:
...
qemu_sem_post(>sem);
}
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDSendParams *p = _send_state->params[i];
qemu_sem_wait(_send_state->channels_ready);
trace_multifd_send_sync_main_wait(p->id);
qemu_sem_wait(>sem_sync);
...
Move the channels_ready wait before the sem post to keep the sequence
consistent. Also fix the error path to post to channels_ready and
sem_sync in the correct order.
Signed-off-by: Fabiano Rosas
---
migration/multifd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index a7c7a947e3..d626740f2f 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -618,6 +618,7 @@ int multifd_send_sync_main(QEMUFile *f)
trace_multifd_send_sync_main_signal(p->id);
+qemu_sem_wait(_send_state->channels_ready);
qemu_mutex_lock(>mutex);
if (p->quit) {
@@ -635,7 +636,6 @@ int multifd_send_sync_main(QEMUFile *f)
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDSendParams *p = _send_state->params[i];
-qemu_sem_wait(_send_state->channels_ready);
trace_multifd_send_sync_main_wait(p->id);
qemu_sem_wait(>sem_sync);
@@ -763,8 +763,8 @@ out:
* who pay attention to me.
*/
if (ret != 0) {
-qemu_sem_post(>sem_sync);
qemu_sem_post(_send_state->channels_ready);
+qemu_sem_post(>sem_sync);
}
qemu_mutex_lock(>mutex);
--
2.35.3
[ANNOUNCE] QEMU 7.2.6 Stable released
Hi everyone, The QEMU v7.2.6 stable release is now available. You can grab the tarball from our download page here: https://www.qemu.org/download/#source v7.2.6 is now tagged in the official qemu.git repository, and the stable-7.2 branch has been updated accordingly: https://gitlab.com/qemu-project/qemu/-/commits/stable-7.2?ref_type=heads This update contains usual pile of general fixes for various architectures and subsystems, and brings up a backport of re-entrancy fixes which landed in 8.1, together with all follow-ups and refinements to date: https://gitlab.com/qemu-project/qemu/-/issues/556 I plan to maintain 7.2.x branch for a while still, to see how it goes, so hopefully there will be some more regular 7.2.x stable releases. Thank you everyone who has been involved and helped with the stable series! Changelog: 7be98a0583 Update version for 7.2.6 release feb0d5a932 tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR 2021c2d539 s390x/ap: fix missing subsystem reset registration d0f8b52fc1 ui: fix crash when there are no active_console 9eda3b6874 hw/tpm: TIS on sysbus: Remove unsupport ppi command line option b5fad36452 target/riscv/pmp.c: respect mseccfg.RLB for pmpaddrX changes 7601c960b6 hw/riscv: virt: Fix riscv,pmu DT node path f44ffdcef9 linux-user/riscv: Use abi type for target_ucontext d4a3464109 hw/intc: Make rtc variable names consistent 6097d3cbba hw/intc: Fix upper/lower mtime write calculation ec0afe3c0b hw/char/riscv_htif: Fix printing of console characters on big endian hosts c04c0123bd arm64: Restore trapless ptimer access 217ab2b86c virtio: Drop out of coroutine context in virtio_load() 3c99de0aa7 qxl: don't assert() if device isn't yet initialized ebac7b1bef hw/net/vmxnet3: Fix guest-triggerable assert() 721c4c8692 docs tests: Fix use of migrate_set_parameter af144c17b5 qemu-options.hx: Rephrase the descriptions of the -hd* and -cdrom options a5d911beb6 hw/i2c/aspeed: Fix TXBUF transmission start position error 2ed40ec1e0 hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode ccac65fbd1 hw/ide/ahci: fix broken SError handling 2aa37f5fa5 hw/ide/ahci: fix ahci_write_fis_sdb() 74d9ef9d0b hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set 458a5f95de hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared 1e5ad6b06b hw/ide/ahci: simplify and document PxCI handling 131bf5d20d hw/ide/ahci: write D2H FIS when processing NCQ command f86c6ff824 hw/ide/core: set ERR_STAT in unsupported command completion d4d975bb11 target/ppc: Flush inputs to zero with NJ in ppc_store_vscr 86b40ee537 ppc/vof: Fix missed fields in VOF cleanup 13f9872a10 hw/ppc/e500: fix broken snapshot replay 8b1eac90bb block-migration: Ensure we don't crash during migration cleanup 8f364b5b86 docs/about/license: Update LICENSE URL a451382580 target/arm: Fix 64-bit SSRA 7400b82afb target/arm: Fix SME ST1Q 0b133e1435 accel/kvm: Specify default IPA size for arm64 bfe41c8f65 kvm: Introduce kvm_arch_get_default_type hook 204ff2b8bb include/hw/virtio/virtio-gpu: Fix virtio-gpu with blob on big endian hosts e7ecf6a45f target/s390x: Check reserved bits of VFMIN/VFMAX's M5 7760cfd9c8 target/s390x: Fix VSTL with a large length 7c4ce14b41 target/s390x: Use a 16-bit immediate in VREP 08a4e6da12 target/s390x: Fix the "ignored match" case in VSTRS 0434ea16fe Fixed incorrect LLONG alignment for openrisc and cris e29b1ef53c include/exec/user: Set ABI_LLONG_ALIGNMENT to 4 for nios2 14e2c1c4ce include/exec/user: Set ABI_LLONG_ALIGNMENT to 4 for microblaze a568abcf17 linux-user/elfload: Set V in ELF_HWCAP for RISC-V 04535fb7b5 hw/nvme: fix CRC64 for guard tag 408179de49 dump: kdump-zlib data pages not dumped with pvtime/aarch64 5de2b51ebb hw/smbios: Fix core count in type4 9238e669ed hw/smbios: Fix thread count in type4 056bada5d2 hw/smbios: Fix smbios_smp_sockets caculation 9bb8c4fb6b machine: Add helpers to get cores/threads per socket bf202262e5 pnv_lpc: disable reentrancy detection for lpc-hc c34e604bf6 loongarch: mark loongarch_ipi_iocsr re-entrnacy safe 79873ecad0 apic: disable reentrancy detection for apic-msi 1247481530 raven: disable reentrancy detection for iomem 65ad790287 bcm2835_property: disable reentrancy detection for iomem f5072ff503 lsi53c895a: disable reentrancy detection for MMIO region, too c2cf7829a5 lsi53c895a: disable reentrancy detection for script RAM ae96dce3b7 hw: replace most qemu_bh_new calls with qemu_bh_new_guarded bb3522b4f8 checkpatch: add qemu_bh_new/aio_bh_new checks f88ebe0c7d async: avoid use-after-free on re-entrancy guard 61dacb401b async: Add an optional reentrancy guard to the BH API c40ca2301c memory: prevent dma-reentracy issues 590e71e536 python: drop pipenv b8d1fc55b5 gitlab-ci: check-dco.py: switch from master to stable-7.2 branch
Re: [PATCH 1/2] configure: support passthrough of -Dxxx args to meson
On Fri, Sep 22, 2023 at 04:36:00PM +0200, Thomas Huth wrote: > On 22/09/2023 16.00, Peter Maydell wrote: > > On Fri, 22 Sept 2023 at 14:56, Daniel P. Berrangé > > wrote: > > > > > > This can be useful for setting some meson global options, such as the > > > optimization level or debug state, which don't have an analogous > > > option explicitly defined in QEMU's configure wrapper script. > > > > > > Signed-off-by: Daniel P. Berrangé > > > > The commit message says it's adding support for a new feature... > > > > > --- > > > configure | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/configure b/configure > > > index e08127045d..cbd7e03e9f 100755 > > > --- a/configure > > > +++ b/configure > > > @@ -931,6 +931,8 @@ cat << EOF > > > bsd-userall BSD usermode emulation targets > > > pie Position Independent Executables > > > > > > + -Dmesonoptname=val passthrough option to meson unmodified > > > + > > > NOTE: The object files are built at the place where configure is > > > launched > > > EOF > > > exit 0 > > > > ...but the patch is only updating the --help text. Is there > > a missing piece of code here ? > > The patch has already been merged, see commit ff136d2a99253483f ... and IIRC > I slightly modified it when picking it up (according to the patch > description), so this here is likely a left-over of a rebase. Daniel, I > think you can drop this patch here. Opps, yes, a mistake updating an old branch. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Stable-8.1.1 11/34] softmmu: Assert data in bounds in iotlb_to_section
20.09.2023 18:04, Alex Bennée wrote: .. Sorry my previous reply was eaten by my MUA. That happens.. especially when MUA becomes hungry :) The main purpose of the asserts is to catch corruption to the Memory Regions early so we don't see weird failures later on (c.f. the 3 separate bugs for crashes in slightly different places). IOW is we are crashing on the asserts in this patch but it's booting without it we are just getting lucky. Thank you very much for this summary. Yeah, this confirms my thought, but I wanted to be sure. I left it in. /mjt
Re: [PATCH 1/2] configure: support passthrough of -Dxxx args to meson
On 22/09/2023 16.00, Peter Maydell wrote: On Fri, 22 Sept 2023 at 14:56, Daniel P. Berrangé wrote: This can be useful for setting some meson global options, such as the optimization level or debug state, which don't have an analogous option explicitly defined in QEMU's configure wrapper script. Signed-off-by: Daniel P. Berrangé The commit message says it's adding support for a new feature... --- configure | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configure b/configure index e08127045d..cbd7e03e9f 100755 --- a/configure +++ b/configure @@ -931,6 +931,8 @@ cat << EOF bsd-userall BSD usermode emulation targets pie Position Independent Executables + -Dmesonoptname=val passthrough option to meson unmodified + NOTE: The object files are built at the place where configure is launched EOF exit 0 ...but the patch is only updating the --help text. Is there a missing piece of code here ? The patch has already been merged, see commit ff136d2a99253483f ... and IIRC I slightly modified it when picking it up (according to the patch description), so this here is likely a left-over of a rebase. Daniel, I think you can drop this patch here. Thomas
Re: [PATCH] vfio/pci: rename vfio_put_device to vfio_pci_put_device
On 9/22/23 04:52, Zhenzhong Duan wrote:
vfio_put_device() is a VFIO PCI specific function, rename it with
'vfio_pci' prefix to avoid confusing.
No functional change.
There is more to be done but it can wait after the big code reshuffle.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Applied to vfio-next.
Thanks,
C.
---
hw/vfio/pci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3b2ca3c24ca2..b2d5010b9f0e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2826,7 +2826,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev,
Error **errp)
}
}
-static void vfio_put_device(VFIOPCIDevice *vdev)
+static void vfio_pci_put_device(VFIOPCIDevice *vdev)
{
g_free(vdev->vbasedev.name);
g_free(vdev->msix);
@@ -3317,7 +3317,7 @@ static void vfio_instance_finalize(Object *obj)
*
* g_free(vdev->igd_opregion);
*/
-vfio_put_device(vdev);
+vfio_pci_put_device(vdev);
vfio_put_group(group);
}
Re: [PATCH v4 2/3] i386: Explicitly ignore unsupported BUS_MCEERR_AO MCE on AMD guest
On 9/22/23 4:36 AM, William Roche wrote: > On 9/21/23 19:41, Yazen Ghannam wrote: >> On 9/20/23 7:13 AM, Joao Martins wrote: >>> On 18/09/2023 23:00, William Roche wrote: [...] So it looks like the mechanism works fine... unless the VM has migrated between the SRAO error and the first time it really touches the poisoned page to get an SRAR error ! In this case, its new address space (created on the migration destination) will have a zero-page where we had a poisoned page, and the AMD VM Kernel (that never dealt with the SRAO) doesn't know about the poisoned page and will access the page finding only zeros... We have a memory corruption ! >> >> I don't understand this. Why would the page be zero? Even so, why would >> that affect poison? > > The migration of a VM moves the memory content from a source platform to > a destination. This is mainly the qemu processes reading the data and > replicating it on the destination. The source qemu where a memory page > is poisoned is(will be[*]) able to skip the poisoned pages it knows > about to indicate to the destination machine to populate the associated > page(s) with zeros as there is no "poison destination page" mechanism in > place for this migration transfer. > >> >> Also, during page migration, does the data flow through the CPU core? >> Sorry for the basic question. I haven't done a lot with virtualization. > > Yes, in most cases (with the exception of RDMA) the data flow through > the CPU cores because the migration verifies if the area to transfer has > some empty pages. > If the CPU moves the memory, then the data will pass through the core/L1 caches, correct? If so, then this will result in a MCE/poison consumption/AR event in that core. So it seems to me that migration will always cause an AR event, and the gap you describe will not occur. Does this make sense? Sorry if I misunderstood. In general, the hardware is designed to detect and mark poison, and to not let poison escape a system undetected. In the strictest case, the hardware will perform a system reset if poison is leaving the system. In a more graceful case, the hardware will continue to pass the poison marker with the data, so the destination hardware will receive it. In both cases, the goal is to avoid silent data corruption, and to do so in the hardware, i.e. without relying on firmware or software management. The hardware designers are very keen on this point. BTW, the RDMA case will need further discussion. I *think* this would fall under the "strictest" case. And likely, CPU-based migration will also. But I think we can test this and find out. :) >> >> Please note that current AMD systems use an internal poison marker on >> memory. This cannot be cleared through normal memory operations. The >> only exception, I think, is to use the CLZERO instruction. This will >> completely wipe a cacheline including metadata like poison, etc. >> >> So the hardware should not (by design) loose track of poisoned data. > > This would be better, but virtualization migration currently looses > track of this. > Which is not a problem for VMs where the kernel took note of the poison > and keeps track of it. Because this kernel will handle the poison > locations it knows about, signaling when these poisoned locations are > touched. > Can you please elaborate on this? I would expect the host kernel to do all the physical, including poison, memory management. Or do you mean in the nested poison case like this? 1) The host detects an "AO/deferred" error. 2) The host can try to recover the memory, if clean, etc. 3) Otherwise, the host passes the error info, with "AO/deferred" severity to the guest. 4) The guest, in nested fashion, can try to recover the memory, if clean, etc. Or signal its own processes with the AO SIGBUS. >> It is a very rare window, but in order to fix it the most reasonable course of action would be to make the AMD emulation deal with SRAO errors, instead of ignoring them. Do you agree with my analysis ? >>> >>> Under the case that SRAO aren't handled well in the kernel today[*] for >>> AMD, we >>> could always add a migration blocker when we hit AO sigbus, in case ignoring >>> is our only option. But this would be less than ideal to propagating the >>> SRAO into the guest. >>> >>> [*] Meaning knowing that handling the SRAO would generate a crash in the >>> guest >>> >>> Perhaps as an improvement, perhaps allow qemu to choose to propagate should >>> this >>> limitation be lifted via a new -action value and allow it to >>> ignore/propagate or >>> not e.g. >>> >>> -action mce=none # default on Intel to propagate all MCE events to the >>> guest >>> -action mce=ignore-optional # Ignore SRAO > > Yes we may need to create something like that, but missing SRAO has > technical consequences too. > >>> >>> I suppose the second is also useful for ARM64 considering they currently >>> ignore >>> SRAO events
Re: [PATCH 01/11] qdev: Add qdev_prop_set_array()
Philippe Mathieu-Daudé writes:
> On 8/9/23 16:36, Kevin Wolf wrote:
>> Instead of exposing the ugly hack of how we represent arrays in qdev (a
>> static "foo-len" property and after it is set, dynamically created
>> "foo[i]" properties) to boards, add an interface that allows setting the
>> whole array at once.
>> Once all internal users of devices with array properties have been
>> converted to use this function, we can change the implementation to move
>> away from this hack.
>> Signed-off-by: Kevin Wolf
>> ---
>> include/hw/qdev-properties.h | 3 +++
>> hw/core/qdev-properties.c| 21 +
>> 2 files changed, 24 insertions(+)
>
>
>> +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>> +{
>> +const QListEntry *entry;
>> +g_autofree char *prop_len = g_strdup_printf("len-%s", name);
>> +uint32_t i = 0;
>
> "unsigned"? Anyway,
Yes, or even plain int. It all gets replaced in the last patch, though.
> Reviewed-by: Philippe Mathieu-Daudé
>
>> +
>> +object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
>> +_abort);
>> +
>> +QLIST_FOREACH_ENTRY(values, entry) {
>> +g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
>> +object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
>> +_abort);
>> +i++;
>> +}
>> +
>> +qobject_unref(values);
>> +}
Re: [PATCH 0/4] multifd: various fixes
Elena Ufimtseva writes: > Hello > > While working and testing various live migration scenarios, > a few issues were found. > > This is my first patches in live migration and I will > appreciate the suggestions from the community if these > patches could be done differently. > > [PATCH 1/4] multifd: wait for channels_ready before sending sync > I am not certain about this change since it seems that > the sync flag could be the part of the packets with pages that are > being sent out currently. > But the traces show this is not always the case: > multifd_send 230.873 pid=55477 id=0x0 packet_num=0x6f4 normal=0x40 flags=0x1 > next_packet_size=0x4 > multifd_send 14.718 pid=55477 id=0x1 packet_num=0x6f5 normal=0x0 flags=0x1 > next_packet_size=0x8 > If the sync packet is indeed can be a standalone one, then waiting for > channels_ready before seem to be appropriate, but waisting iteration on > sync only packet. I haven't looked at this code for a while, so there's some context switching to be made, but you're definitely on the right track here. I actually have an unsent patch doing almost the same as your patch 1/4. I'll comment more there. About the sync being standalone, I would expect that to always be the case since we're incrementing packet_num at that point. > [PATCH 4/4] is also relevant to 1/4, but fixes the over-accounting in > case of sync only packet. > > > Thank you in advance and looking forward for your feedback. > > Elena > > Elena Ufimtseva (4): > multifd: wait for channels_ready before sending sync > migration: check for rate_limit_max for RATE_LIMIT_DISABLED > multifd: fix counters in multifd_send_thread > multifd: reset next_packet_len after sending pages > > migration/migration-stats.c | 8 > migration/multifd.c | 11 ++- > 2 files changed, 10 insertions(+), 9 deletions(-)
[PATCH 3/3] i386: hvf: Updates API usage to use modern vCPU run function
macOS 10.15 introduced the more efficient hv_vcpu_run_until() function
to supersede hv_vcpu_run(). According to the documentation, there is no
longer any reason to use the latter on modern host OS versions.
Observed behaviour of the newer function is that as documented, it exits
much less frequently - and most of the original function’s exits seem to
have been effectively pointless.
Another reason to use the new function is that it is a prerequisite for
using newer features such as in-kernel APIC support. (Not covered by
this patch.)
This change implements the upgrade by selecting one of three code paths
at compile time: two static code paths for the new and old functions
respectively, when building for targets where the new function is either
not available, or where the built executable won’t run on older
platforms lacking the new function anyway. The third code path selects
dynamically based on runtime detected availability of the weakly-linked
symbol.
Signed-off-by: Phil Dennis-Jordan
---
target/i386/hvf/hvf.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 55bd7d2af8..e4c785c686 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -410,6 +410,27 @@ static void hvf_cpu_x86_cpuid(CPUX86State *env, uint32_t
index, uint32_t count,
}
}
+static hv_return_t hvf_vcpu_run(hv_vcpuid_t vcpu_id)
+{
+/*
+ * hv_vcpu_run_until is available and recommended from macOS 10.15+.
+ * Test for availability at runtime and fall back to hv_vcpu_run() only
+ * where necessary.
+ */
+#ifndef MAC_OS_X_VERSION_10_15
+return hv_vcpu_run(vcpu_id);
+#elif MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_15
+return hv_vcpu_run_until(vcpu_id, HV_DEADLINE_FOREVER);
+#else /* MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_15 */
+/* 10.15 SDK or newer, but could be < 10.15 at runtime */
+if (__builtin_available(macOS 10.15, *)) {
+return hv_vcpu_run(vcpu_id);
+} else {
+return hv_vcpu_run_until(vcpu_id, HV_DEADLINE_FOREVER);
+}
+#endif
+}
+
int hvf_vcpu_exec(CPUState *cpu)
{
X86CPU *x86_cpu = X86_CPU(cpu);
@@ -438,7 +459,7 @@ int hvf_vcpu_exec(CPUState *cpu)
return EXCP_HLT;
}
-hv_return_t r = hv_vcpu_run(cpu->accel->fd);
+hv_return_t r = hvf_vcpu_run(cpu->accel->fd);
assert_hvf_ok(r);
/* handle VMEXIT */
--
2.36.1
