Re: [PATCH 3/5] qom: Remove module_obj_name parameter from OBJECT_DECLARE* macros
On 9/16/20 8:25 PM, Eduardo Habkost wrote: > One of the goals of having less boilerplate on QOM declarations > is to avoid human error. Requiring an extra argument that is > never used is an opportunity for mistakes. > > Remove the unused argument from OBJECT_DECLARE_TYPE and > OBJECT_DECLARE_SIMPLE_TYPE. > > Coccinelle patch used to convert all users of the macros: > > @@ > declarer name OBJECT_DECLARE_TYPE; > identifier InstanceType, ClassType, lowercase, UPPERCASE; > @@ >OBJECT_DECLARE_TYPE(InstanceType, ClassType, > -lowercase, >UPPERCASE); > > @@ > declarer name OBJECT_DECLARE_SIMPLE_TYPE; > identifier InstanceType, lowercase, UPPERCASE; > @@ >OBJECT_DECLARE_SIMPLE_TYPE(InstanceType, > -lowercase, >UPPERCASE); > > Signed-off-by: Eduardo Habkost For the ppc part, Reviewed-by: Cédric Le Goater > --- > Cc: "Marc-André Lureau" > Cc: Gerd Hoffmann > Cc: "Michael S. Tsirkin" > Cc: "Daniel P. Berrangé" > Cc: Peter Maydell > Cc: Corey Minyard > Cc: "Cédric Le Goater" > Cc: David Gibson > Cc: Cornelia Huck > Cc: Thomas Huth > Cc: Halil Pasic > Cc: Christian Borntraeger > Cc: "Philippe Mathieu-Daudé" > Cc: Alistair Francis > Cc: David Hildenbrand > Cc: Laurent Vivier > Cc: Amit Shah > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Paul Durrant > Cc: Paolo Bonzini > Cc: Eduardo Habkost > Cc: Fam Zheng > Cc: "Gonglei (Arei)" > Cc: Igor Mammedov > Cc: Stefan Berger > Cc: Richard Henderson > Cc: Michael Rolnik > Cc: Sarah Harris > Cc: "Edgar E. Iglesias" > Cc: Michael Walle > Cc: Aleksandar Markovic > Cc: Aurelien Jarno > Cc: Jiaxun Yang > Cc: Aleksandar Rikalo > Cc: Anthony Green > Cc: Chris Wulff > Cc: Marek Vasut > Cc: Stafford Horne > Cc: Palmer Dabbelt > Cc: Sagar Karandikar > Cc: Bastian Koppelmann > Cc: Yoshinori Sato > Cc: Mark Cave-Ayland > Cc: Artyom Tarasenko > Cc: Guan Xuetao > Cc: Max Filippov > Cc: qemu-de...@nongnu.org > Cc: qemu-...@nongnu.org > Cc: qemu-...@nongnu.org > Cc: qemu-s3...@nongnu.org > Cc: qemu-bl...@nongnu.org > Cc: xen-devel@lists.xenproject.org > Cc: qemu-ri...@nongnu.org > --- > hw/audio/intel-hda.h| 2 +- > hw/display/virtio-vga.h | 2 +- > include/authz/base.h| 2 +- > include/authz/list.h| 2 +- > include/authz/listfile.h| 2 +- > include/authz/pamacct.h | 2 +- > include/authz/simple.h | 2 +- > include/crypto/secret_common.h | 2 +- > include/crypto/secret_keyring.h | 2 +- > include/hw/arm/armsse.h | 2 +- > include/hw/hyperv/vmbus.h | 2 +- > include/hw/i2c/i2c.h| 2 +- > include/hw/i2c/smbus_slave.h| 2 +- > include/hw/ipack/ipack.h| 2 +- > include/hw/ipmi/ipmi.h | 2 +- > include/hw/mem/pc-dimm.h| 2 +- > include/hw/ppc/pnv.h| 2 +- > include/hw/ppc/pnv_core.h | 2 +- > include/hw/ppc/pnv_homer.h | 2 +- > include/hw/ppc/pnv_occ.h| 2 +- > include/hw/ppc/pnv_psi.h| 2 +- > include/hw/ppc/pnv_xive.h | 2 +- > include/hw/ppc/spapr_cpu_core.h | 2 +- > include/hw/ppc/spapr_vio.h | 2 +- > include/hw/ppc/xics.h | 2 +- > include/hw/ppc/xive.h | 2 +- > include/hw/s390x/event-facility.h | 2 +- > include/hw/s390x/s390_flic.h| 2 +- > include/hw/s390x/sclp.h | 2 +- > include/hw/sd/sd.h | 2 +- > include/hw/ssi/ssi.h| 2 +- > include/hw/sysbus.h | 2 +- > include/hw/virtio/virtio-gpu.h | 2 +- > include/hw/virtio/virtio-input.h| 2 +- > include/hw/virtio/virtio-mem.h | 2 +- > include/hw/virtio/virtio-pmem.h | 2 +- > include/hw/virtio/virtio-serial.h | 2 +- > include/hw/xen/xen-bus.h| 2 +- > include/io/channel.h| 2 +- > include/io/dns-resolver.h | 2 +- > include/io/net-listener.h | 2 +- > include/qom/object.h| 6 ++ > include/scsi/pr-manager.h | 2 +- > include/sysemu/cryptodev.h | 2 +- > include/sysemu/hostmem.h| 2 +- > include/sysemu/rng.h| 2 +- > include/sysemu/tpm_backend.h| 2 +- > include/sysemu/vhost-user-backend.h | 2 +- > target/alpha/cpu-qom.h | 2 +- > target/arm/cpu-qom.h| 2 +- > target
Re: [PATCH 4/5] [automated] Use OBJECT_DECLARE_TYPE when possible
On 9/16/20 8:25 PM, Eduardo Habkost wrote: > This converts existing DECLARE_OBJ_CHECKERS usage to > OBJECT_DECLARE_TYPE when possible. > > $ ./scripts/codeconverter/converter.py -i \ >--pattern=AddObjectDeclareType $(git grep -l '' -- '*.[ch]') > > Signed-off-by: Eduardo Habkost For the aspeed part, Reviewed-by: Cédric Le Goater > --- > Cc: Peter Maydell > Cc: Andrzej Zaborowski > Cc: Alistair Francis > Cc: Kevin Wolf > Cc: Max Reitz > Cc: Mark Cave-Ayland > Cc: David Gibson > Cc: Richard Henderson > Cc: David Hildenbrand > Cc: Cornelia Huck > Cc: Thomas Huth > Cc: Halil Pasic > Cc: Christian Borntraeger > Cc: "Michael S. Tsirkin" > Cc: Paolo Bonzini > Cc: Fam Zheng > Cc: Dmitry Fleytman > Cc: Gerd Hoffmann > Cc: "Marc-André Lureau" > Cc: "Cédric Le Goater" > Cc: Andrew Jeffery > Cc: Joel Stanley > Cc: Andrew Baumann > Cc: "Philippe Mathieu-Daudé" > Cc: Eric Auger > Cc: Eduardo Habkost > Cc: Marcel Apfelbaum > Cc: Laurent Vivier > Cc: Sergio Lopez > Cc: John Snow > Cc: Xiao Guangrong > Cc: Peter Chubb > Cc: "Daniel P. Berrangé" > Cc: Beniamino Galvani > Cc: "Edgar E. Iglesias" > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Paul Durrant > Cc: Jason Wang > Cc: qemu-...@nongnu.org > Cc: qemu-de...@nongnu.org > Cc: qemu-bl...@nongnu.org > Cc: qemu-...@nongnu.org > Cc: qemu-s3...@nongnu.org > Cc: xen-devel@lists.xenproject.org > --- > hw/ppc/e500.h | 5 + > hw/s390x/ccw-device.h | 4 +--- > hw/s390x/virtio-ccw.h | 5 + > hw/usb/ccid.h | 5 + > hw/usb/hcd-dwc2.h | 3 +-- > hw/usb/hcd-ehci.h | 5 + > hw/virtio/virtio-pci.h| 5 + > include/chardev/char.h| 4 +--- > include/hw/arm/aspeed_soc.h | 5 + > include/hw/arm/bcm2836.h | 5 + > include/hw/arm/smmu-common.h | 5 + > include/hw/arm/smmuv3.h | 5 + > include/hw/arm/virt.h | 5 + > include/hw/boards.h | 3 +-- > include/hw/display/macfb.h| 5 + > include/hw/gpio/aspeed_gpio.h | 5 + > include/hw/i2c/aspeed_i2c.h | 5 + > include/hw/i386/ioapic_internal.h | 5 + > include/hw/i386/microvm.h | 5 + > include/hw/i386/pc.h | 4 +--- > include/hw/i386/x86-iommu.h | 5 + > include/hw/i386/x86.h | 5 + > include/hw/ide/internal.h | 4 +--- > include/hw/input/adb.h| 4 +--- > include/hw/isa/i8259_internal.h | 5 + > include/hw/isa/isa.h | 4 +--- > include/hw/mem/nvdimm.h | 5 + > include/hw/misc/aspeed_scu.h | 5 + > include/hw/misc/aspeed_sdmc.h | 5 + > include/hw/misc/imx_ccm.h | 5 + > include/hw/misc/mos6522.h | 5 + > include/hw/pci-host/pnv_phb4.h| 5 + > include/hw/pci/pci.h | 4 +--- > include/hw/pci/pci_host.h | 4 +--- > include/hw/pcmcia.h | 5 + > include/hw/ppc/spapr.h| 5 + > include/hw/qdev-core.h| 4 +--- > include/hw/rtc/allwinner-rtc.h| 5 + > include/hw/s390x/3270-ccw.h | 5 + > include/hw/s390x/s390-virtio-ccw.h| 5 + > include/hw/s390x/storage-attributes.h | 5 + > include/hw/s390x/storage-keys.h | 5 + > include/hw/s390x/tod.h| 5 + > include/hw/scsi/scsi.h| 4 +--- > include/hw/sd/allwinner-sdhost.h | 5 + > include/hw/sd/sd.h| 5 + > include/hw/ssi/aspeed_smc.h | 5 + > include/hw/ssi/xilinx_spips.h | 4 +--- > include/hw/timer/aspeed_timer.h | 5 + > include/hw/timer/i8254.h | 5 + > include/hw/usb.h | 4 +--- > include/hw/virtio/virtio.h| 4 +--- > include/hw/watchdog/wdt_aspeed.h | 5 + > include/hw/xen/xen-block.h| 4 +--- > include/hw/xen/xen-bus.h | 4 +--- > include/net/can_host.h| 5 + > include/net/filter.h | 4 +--- > include/ui/console.h | 4 +--- > hw/arm/mps2-tz.c | 5 + > hw/arm/mps2.c | 5 + > hw/arm/musca.c| 5 + > hw/arm/spitz.c| 5 + > hw/arm/vexpress
Re: [PATCH-for-5.1 1/3] target: Remove unnecessary CPU() cast
On 4/12/20 11:09 PM, Philippe Mathieu-Daudé wrote:
> The CPU() macro is defined as:
>
> #define CPU(obj) ((CPUState *)(obj))
>
> Remove an unnecessary CPU() cast.
>
> Patch created mechanically using spatch with this script:
>
> @@
> typedef CPUState;
> CPUState *s;
> @@
> - CPU(s)
> + s
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
> ---
> target/ppc/mmu_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 86c667b094..8972714775 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -1820,7 +1820,7 @@ static inline void do_invalidate_BAT(CPUPPCState *env,
> target_ulong BATu,
> if (((end - base) >> TARGET_PAGE_BITS) > 1024) {
> /* Flushing 1024 4K pages is slower than a complete flush */
> LOG_BATS("Flush all BATs\n");
> -tlb_flush(CPU(cs));
> +tlb_flush(cs);
> LOG_BATS("Flush done\n");
> return;
> }
>
Re: [PATCH-for-5.1 3/3] hw: Remove unnecessary DEVICE() cast
On 4/12/20 11:09 PM, Philippe Mathieu-Daudé wrote:
> The DEVICE() macro is defined as:
>
> #define DEVICE(obj) OBJECT_CHECK(DeviceState, (obj), TYPE_DEVICE)
>
> Remove unnecessary DEVICE() casts.
>
> Patch created mechanically using spatch with this script:
>
> @@
> typedef DeviceState;
> DeviceState *s;
> @@
> - DEVICE(s)
> + s
>
> Signed-off-by: Philippe Mathieu-Daudé
For ftgmac100,
Reviewed-by: Cédric Le Goater
> ---
> hw/display/artist.c | 2 +-
> hw/display/cg3.c| 2 +-
> hw/display/sm501.c | 2 +-
> hw/display/tcx.c| 4 ++--
> hw/display/vga-isa.c| 2 +-
> hw/i2c/imx_i2c.c| 2 +-
> hw/i2c/mpc_i2c.c| 2 +-
> hw/ide/piix.c | 2 +-
> hw/misc/macio/pmu.c | 2 +-
> hw/net/ftgmac100.c | 3 +--
> hw/net/imx_fec.c| 2 +-
> hw/nubus/nubus-device.c | 2 +-
> hw/pci-host/bonito.c| 2 +-
> hw/ppc/spapr.c | 2 +-
> hw/sh4/sh_pci.c | 2 +-
> hw/xen/xen-legacy-backend.c | 2 +-
> 16 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/hw/display/artist.c b/hw/display/artist.c
> index 753dbb9a77..7e2a4556bd 100644
> --- a/hw/display/artist.c
> +++ b/hw/display/artist.c
> @@ -1353,7 +1353,7 @@ static void artist_realizefn(DeviceState *dev, Error
> **errp)
> s->cursor_height = 32;
> s->cursor_width = 32;
>
> -s->con = graphic_console_init(DEVICE(dev), 0, &artist_ops, s);
> +s->con = graphic_console_init(dev, 0, &artist_ops, s);
> qemu_console_resize(s->con, s->width, s->height);
> }
>
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index a1ede10394..f7f1c199ce 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -321,7 +321,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>
> sysbus_init_irq(sbd, &s->irq);
>
> -s->con = graphic_console_init(DEVICE(dev), 0, &cg3_ops, s);
> +s->con = graphic_console_init(dev, 0, &cg3_ops, s);
> qemu_console_resize(s->con, s->width, s->height);
> }
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index de0ab9d977..2a564889bd 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1839,7 +1839,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
> &s->twoD_engine_region);
>
> /* create qemu graphic console */
> -s->con = graphic_console_init(DEVICE(dev), 0, &sm501_ops, s);
> +s->con = graphic_console_init(dev, 0, &sm501_ops, s);
> }
>
> static const VMStateDescription vmstate_sm501_state = {
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 76de16e8ea..1fb45b1aab 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -868,9 +868,9 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
> sysbus_init_irq(sbd, &s->irq);
>
> if (s->depth == 8) {
> -s->con = graphic_console_init(DEVICE(dev), 0, &tcx_ops, s);
> +s->con = graphic_console_init(dev, 0, &tcx_ops, s);
> } else {
> -s->con = graphic_console_init(DEVICE(dev), 0, &tcx24_ops, s);
> +s->con = graphic_console_init(dev, 0, &tcx24_ops, s);
> }
> s->thcmisc = 0;
>
> diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
> index 0633ed382c..3aaeeeca1e 100644
> --- a/hw/display/vga-isa.c
> +++ b/hw/display/vga-isa.c
> @@ -74,7 +74,7 @@ static void vga_isa_realizefn(DeviceState *dev, Error
> **errp)
> 0x000a,
> vga_io_memory, 1);
> memory_region_set_coalescing(vga_io_memory);
> -s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s);
> +s->con = graphic_console_init(dev, 0, s->hw_ops, s);
>
> memory_region_add_subregion(isa_address_space(isadev),
> VBE_DISPI_LFB_PHYSICAL_ADDRESS,
> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
> index 30b9aea247..2e02e1c4fa 100644
> --- a/hw/i2c/imx_i2c.c
> +++ b/hw/i2c/imx_i2c.c
> @@ -305,7 +305,7 @@ static void imx_i2c_realize(DeviceState *dev, Error
> **errp)
>IMX_I2C_MEM_SIZE);
> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> -s->bus = i2c_init_bus(DEVICE(dev), NULL);
> +s->bus = i2c_init_bus(dev, NULL);
> }
>
> static void imx_i2c_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc
Re: [PATCH v3 0/3] various: Remove unnecessary casts
On 5/18/20 3:17 PM, Markus Armbruster wrote: > Paolo Bonzini writes: > >> On 15/05/20 07:58, Markus Armbruster wrote: >>> Philippe Mathieu-Daudé writes: >>> Remove unnecessary casts using coccinelle scripts. The CPU()/OBJECT() patches don't introduce logical change, The DEVICE() one removes various OBJECT_CHECK() calls. >>> Queued, thanks! >>> >>> Managing expecations: I'm not a QOM maintainer, I don't want to become >>> one, and I don't normally queue QOM patches :) >>> >> >> I want to be again a QOM maintainer, but it's not the best time for me >> to be one. So thanks for picking up my slack. > > You're welcome :) Could you help me getting this patch merged ? :) http://patchwork.ozlabs.org/project/qemu-devel/patch/20200404153340.164861-1-...@kaod.org/ Thanks, C.
Re: [PATCH v2 1/8] hw/arm/aspeed: Correct DRAM container region size
On 6/1/20 4:29 PM, Philippe Mathieu-Daudé wrote: > memory_region_set_size() handle the 16 Exabytes limit by > special-casing the UINT64_MAX value. This is not a problem > for the 32-bit maximum, 4 GiB. > By using the UINT32_MAX value, the aspeed-ram-container > MemoryRegion ends up missing 1 byte: > > $ qemu-system-arm -M ast2600-evb -S -monitor stdio > (qemu) info mtree > > address-space: aspeed.fmc-ast2600-dma-dram > 8000-00017ffe (prio 0, i/o): aspeed-ram-container > 8000-bfff (prio 0, ram): ram > c000- (prio 0, i/o): max_ram > > Fix by using the correct value. We now have: > > address-space: aspeed.fmc-ast2600-dma-dram > 8000-00017fff (prio 0, i/o): aspeed-ram-container > 8000-bfff (prio 0, ram): ram > c000- (prio 0, i/o): max_ram > > Reviewed-by: Peter Maydell > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Thanks, C. > --- > hw/arm/aspeed.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 2c23297edf..62344ac6a3 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -262,7 +262,7 @@ static void aspeed_machine_init(MachineState *machine) > bmc = g_new0(AspeedBoardState, 1); > > memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container", > - UINT32_MAX); > + 4 * GiB); > memory_region_add_subregion(&bmc->ram_container, 0, machine->ram); > > object_initialize_child(OBJECT(machine), "soc", &bmc->soc, >
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
ice.c | 4 +-- softmmu/dma-helpers.c| 4 +-- softmmu/memory_mapping.c | 2 +- target/i386/cpu-sysemu.c | 2 +- target/i386/hax/hax-accel-ops.c | 4 +-- target/i386/nvmm/nvmm-accel-ops.c| 4 +-- target/i386/whpx/whpx-accel-ops.c| 4 +-- target/i386/whpx/whpx-all.c | 2 +- target/s390x/cpu-sysemu.c| 2 +- tests/unit/test-hbitmap.c| 2 +- tests/unit/test-qmp-cmds.c | 14 tests/unit/test-qobject-output-visitor.c | 2 +- tests/unit/test-vmstate.c| 42 ui/vnc-enc-tight.c | 2 +- util/envlist.c | 2 +- util/hbitmap.c | 2 +- util/main-loop.c | 2 +- util/qemu-timer.c| 2 +- util/vfio-helpers.c | 4 +-- 104 files changed, 197 insertions(+), 202 deletions(-) PPC part: Reviewed-by: Cédric Le Goater Thanks, C.
Re: [patch 05/22] genirq/msi: Fixup includes
On 11/27/21 02:18, Thomas Gleixner wrote: Remove the kobject.h include from msi.h as it's not required and add a sysfs.h include to the core code instead. Signed-off-by: Thomas Gleixner This patch breaks compile on powerpc : CC arch/powerpc/kernel/msi.o In file included from ../arch/powerpc/kernel/msi.c:7: ../include/linux/msi.h:410:65: error: ‘struct cpumask’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror] 410 | int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask, | ^~~ cc1: all warnings being treated as errors Below is fix you can merge in patch 5. Thanks, C. --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -2,6 +2,7 @@ #ifndef LINUX_MSI_H #define LINUX_MSI_H +#include #include #include --- include/linux/msi.h |1 - kernel/irq/msi.c|1 + 2 files changed, 1 insertion(+), 1 deletion(-) --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -2,7 +2,6 @@ #ifndef LINUX_MSI_H #define LINUX_MSI_H -#include #include #include --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include "internals.h"
Re: [patch 17/22] PCI/MSI: Split out !IRQDOMAIN code
On 11/27/21 02:19, Thomas Gleixner wrote:
Split out the non irqdomain code into its own file.
Signed-off-by: Thomas Gleixner
---
drivers/pci/msi/Makefile |5 ++--
drivers/pci/msi/legacy.c | 51
+++
drivers/pci/msi/msi.c| 46 --
3 files changed, 54 insertions(+), 48 deletions(-)
--- a/drivers/pci/msi/Makefile
+++ b/drivers/pci/msi/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
#
# Makefile for the PCI/MSI
-obj-$(CONFIG_PCI) += pcidev_msi.o
-obj-$(CONFIG_PCI_MSI) += msi.o
+obj-$(CONFIG_PCI) += pcidev_msi.o
+obj-$(CONFIG_PCI_MSI) += msi.o
+obj-$(CONFIG_PCI_MSI_ARCH_FALLBACKS) += legacy.o
--- /dev/null
+++ b/drivers/pci/msi/legacy.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Message Signaled Interrupt (MSI).
+ *
+ * Legacy architecture specific setup and teardown mechanism.
+ */
+#include "msi.h"
I am getting a :
../drivers/pci/msi/legacy.c:7:10: fatal error: msi.h: No such file or directory
7 | #include "msi.h"
which seems to be fixed later.
C.
+
+/* Arch hooks */
+int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
+{
+ return -EINVAL;
+}
+
+void __weak arch_teardown_msi_irq(unsigned int irq)
+{
+}
+
+int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
+{
+ struct msi_desc *desc;
+ int ret;
+
+ /*
+* If an architecture wants to support multiple MSI, it needs to
+* override arch_setup_msi_irqs()
+*/
+ if (type == PCI_CAP_ID_MSI && nvec > 1)
+ return 1;
+
+ for_each_pci_msi_entry(desc, dev) {
+ ret = arch_setup_msi_irq(dev, desc);
+ if (ret)
+ return ret < 0 ? ret : -ENOSPC;
+ }
+
+ return 0;
+}
+
+void __weak arch_teardown_msi_irqs(struct pci_dev *dev)
+{
+ struct msi_desc *desc;
+ int i;
+
+ for_each_pci_msi_entry(desc, dev) {
+ if (desc->irq) {
+ for (i = 0; i < entry->nvec_used; i++)
+ arch_teardown_msi_irq(desc->irq + i);
+ }
+ }
+}
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -50,52 +50,6 @@ static void pci_msi_teardown_msi_irqs(st
#define pci_msi_teardown_msi_irqs arch_teardown_msi_irqs
#endif
-#ifdef CONFIG_PCI_MSI_ARCH_FALLBACKS
-/* Arch hooks */
-int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
-{
- return -EINVAL;
-}
-
-void __weak arch_teardown_msi_irq(unsigned int irq)
-{
-}
-
-int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
-{
- struct msi_desc *entry;
- int ret;
-
- /*
-* If an architecture wants to support multiple MSI, it needs to
-* override arch_setup_msi_irqs()
-*/
- if (type == PCI_CAP_ID_MSI && nvec > 1)
- return 1;
-
- for_each_pci_msi_entry(entry, dev) {
- ret = arch_setup_msi_irq(dev, entry);
- if (ret < 0)
- return ret;
- if (ret > 0)
- return -ENOSPC;
- }
-
- return 0;
-}
-
-void __weak arch_teardown_msi_irqs(struct pci_dev *dev)
-{
- int i;
- struct msi_desc *entry;
-
- for_each_pci_msi_entry(entry, dev)
- if (entry->irq)
- for (i = 0; i < entry->nvec_used; i++)
- arch_teardown_msi_irq(entry->irq + i);
-}
-#endif /* CONFIG_PCI_MSI_ARCH_FALLBACKS */
-
/*
* PCI 2.3 does not specify mask bits for each MSI interrupt. Attempting to
* mask all MSI interrupts by clearing the MSI enable bit does not work
Re: [patch 17/22] PCI/MSI: Split out !IRQDOMAIN code
+void __weak arch_teardown_msi_irqs(struct pci_dev *dev)
+{
+ struct msi_desc *desc;
+ int i;
+
+ for_each_pci_msi_entry(desc, dev) {
+ if (desc->irq) {
+ for (i = 0; i < entry->nvec_used; i++)
I guess this is 'desc' ?
Thanks,
C.
+ arch_teardown_msi_irq(desc->irq + i);
+ }
+ }
+}
Re: [patch 00/22] genirq/msi, PCI/MSI: Spring cleaning - Part 1
On 11/27/21 02:18, Thomas Gleixner wrote: The [PCI] MSI code has gained quite some warts over time. A recent discussion unearthed a shortcoming: the lack of support for expanding PCI/MSI-X vectors after initialization of MSI-X. PCI/MSI-X has no requirement to setup all vectors when MSI-X is enabled in the device. The non-used vectors have just to be masked in the vector table. For PCI/MSI this is not possible because the number of vectors cannot be changed after initialization. The PCI/MSI code, but also the core MSI irq domain code are built around the assumption that all required vectors are installed at initialization time and freed when the device is shut down by the driver. Supporting dynamic expansion at least for MSI-X is important for VFIO so that the host side interrupts for passthrough devices can be installed on demand. This is the first part of a large (total 101 patches) series which refactors the [PCI]MSI infrastructure to make runtime expansion of MSI-X vectors possible. The last part (10 patches) provide this functionality. The first part is mostly a cleanup which consolidates code, moves the PCI MSI code into a separate directory and splits it up into several parts. No functional change intended except for patch 2/N which changes the behaviour of pci_get_vector()/affinity() to get rid of the assumption that the provided index is the "index" into the descriptor list instead of using it as the actual MSI[X] index as seen by the hardware. This would break users of sparse allocated MSI-X entries, but non of them use these functions. This series is based on 5.16-rc2 and also available via git: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v1-part-1 For the curious who can't wait for the next part to arrive the full series is available via: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v1-part-4 After fixing the compile failures, I didn't see any regressions on these platforms : PowerNV, pSeries under KVM and PowerVM, using POWER8/9 processors. Thanks, C. Thanks, tglx --- arch/powerpc/platforms/4xx/msi.c| 281 b/Documentation/driver-api/pci/pci.rst |2 b/arch/mips/pci/msi-octeon.c| 32 - b/arch/powerpc/platforms/4xx/Makefile |1 b/arch/powerpc/platforms/cell/axon_msi.c|2 b/arch/powerpc/platforms/powernv/pci-ioda.c |4 b/arch/powerpc/platforms/pseries/msi.c |6 b/arch/powerpc/sysdev/Kconfig |6 b/arch/s390/pci/pci_irq.c |4 b/arch/sparc/kernel/pci_msi.c |4 b/arch/x86/hyperv/irqdomain.c | 55 -- b/arch/x86/include/asm/x86_init.h |6 b/arch/x86/include/asm/xen/hypervisor.h |8 b/arch/x86/kernel/apic/msi.c|8 b/arch/x86/kernel/x86_init.c| 12 b/arch/x86/pci/xen.c| 19 b/drivers/irqchip/irq-gic-v2m.c |1 b/drivers/irqchip/irq-gic-v3-its-pci-msi.c |1 b/drivers/irqchip/irq-gic-v3-mbi.c |1 b/drivers/net/wireless/ath/ath11k/pci.c |2 b/drivers/pci/Makefile |3 b/drivers/pci/msi/Makefile |7 b/drivers/pci/msi/irqdomain.c | 267 +++ b/drivers/pci/msi/legacy.c | 79 +++ b/drivers/pci/msi/msi.c | 645 b/drivers/pci/msi/msi.h | 39 + b/drivers/pci/msi/pcidev_msi.c | 43 + b/drivers/pci/pci-sysfs.c |7 b/drivers/pci/xen-pcifront.c|2 b/include/linux/msi.h | 135 ++--- b/include/linux/pci.h |1 b/kernel/irq/msi.c | 41 + 32 files changed, 696 insertions(+), 1028 deletions(-)
Re: [patch 05/22] genirq/msi: Fixup includes
On 11/29/21 22:38, Thomas Gleixner wrote: Cedric, On Mon, Nov 29 2021 at 08:33, Cédric Le Goater wrote: On 11/27/21 02:18, Thomas Gleixner wrote: Remove the kobject.h include from msi.h as it's not required and add a sysfs.h include to the core code instead. Signed-off-by: Thomas Gleixner This patch breaks compile on powerpc : CC arch/powerpc/kernel/msi.o In file included from ../arch/powerpc/kernel/msi.c:7: ../include/linux/msi.h:410:65: error: ‘struct cpumask’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror] 410 | int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask, | ^~~ cc1: all warnings being treated as errors Below is fix you can merge in patch 5. thanks for having a look. I fixed up this and other fallout and pushed out an updated series (all 4 parts) to: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel msi pSeries fails to allocate MSIs starting with this patch : [PATCH 049/101] powerpc/pseries/msi: Let core code check for contiguous ... I will dig in later on. C.
Re: [patch 05/22] genirq/msi: Fixup includes
On 11/30/21 23:41, Thomas Gleixner wrote: On Tue, Nov 30 2021 at 23:10, Thomas Gleixner wrote: On Tue, Nov 30 2021 at 22:48, Cédric Le Goater wrote: On 11/29/21 22:38, Thomas Gleixner wrote: On Mon, Nov 29 2021 at 08:33, Cédric Le Goater wrote: thanks for having a look. I fixed up this and other fallout and pushed out an updated series (all 4 parts) to: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel msi pSeries fails to allocate MSIs starting with this patch : [PATCH 049/101] powerpc/pseries/msi: Let core code check for contiguous ... I will dig in later on. Let me stare at the core function.. It's not the core function. It's the patch above and I'm a moron. All good now. Ship it ! Thanks, C.
Re: [patch V2 01/23] powerpc/4xx: Remove MSI support which never worked
Hello Thomas,
On 12/6/21 23:27, Thomas Gleixner wrote:
This code is broken since day one. ppc4xx_setup_msi_irqs() has the
following gems:
1) The handling of the result of msi_bitmap_alloc_hwirqs() is completely
broken:
When the result is greater than or equal 0 (bitmap allocation
successful) then the loop terminates and the function returns 0
(success) despite not having installed an interrupt.
When the result is less than 0 (bitmap allocation fails), it prints an
error message and continues to "work" with that error code which would
eventually end up in the MSI message data.
2) On every invocation the file global pp4xx_msi::msi_virqs bitmap is
allocated thereby leaking the previous one.
IOW, this has never worked and for more than 10 years nobody cared. Remove
the gunk.
Fixes: 3fb7933850fa ("powerpc/4xx: Adding PCIe MSI support")
Shouldn't we remove all of it ? including the updates in the device trees
and the Kconfig changes under :
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/40x/Kconfig: select PPC4xx_MSI
Thanks,
C.
Fixes: 247540b03bfc ("powerpc/44x: Fix PCI MSI support for Maui APM821xx SoC and
Bluestone board")
Signed-off-by: Thomas Gleixner
Reviewed-by: Jason Gunthorpe
Cc: Michael Ellerman
Cc: Paul Mackerras
Cc: Benjamin Herrenschmidt
Cc: linuxppc-...@lists.ozlabs.org
---
arch/powerpc/platforms/4xx/Makefile |1
arch/powerpc/platforms/4xx/msi.c| 281
arch/powerpc/sysdev/Kconfig |6
3 files changed, 288 deletions(-)
--- a/arch/powerpc/platforms/4xx/Makefile
+++ b/arch/powerpc/platforms/4xx/Makefile
@@ -3,6 +3,5 @@ obj-y += uic.o machine_check.o
obj-$(CONFIG_4xx_SOC) += soc.o
obj-$(CONFIG_PCI) += pci.o
obj-$(CONFIG_PPC4xx_HSTA_MSI) += hsta_msi.o
-obj-$(CONFIG_PPC4xx_MSI) += msi.o
obj-$(CONFIG_PPC4xx_CPM) += cpm.o
obj-$(CONFIG_PPC4xx_GPIO) += gpio.o
--- a/arch/powerpc/platforms/4xx/msi.c
+++ /dev/null
@@ -1,281 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Adding PCI-E MSI support for PPC4XX SoCs.
- *
- * Copyright (c) 2010, Applied Micro Circuits Corporation
- * Authors:Tirumala R Marri
- * Feng Kan
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#define PEIH_TERMADH 0x00
-#define PEIH_TERMADL 0x08
-#define PEIH_MSIED 0x10
-#define PEIH_MSIMK 0x18
-#define PEIH_MSIASS0x20
-#define PEIH_FLUSH00x30
-#define PEIH_FLUSH10x38
-#define PEIH_CNTRST0x48
-
-static int msi_irqs;
-
-struct ppc4xx_msi {
- u32 msi_addr_lo;
- u32 msi_addr_hi;
- void __iomem *msi_regs;
- int *msi_virqs;
- struct msi_bitmap bitmap;
- struct device_node *msi_dev;
-};
-
-static struct ppc4xx_msi ppc4xx_msi;
-
-static int ppc4xx_msi_init_allocator(struct platform_device *dev,
- struct ppc4xx_msi *msi_data)
-{
- int err;
-
- err = msi_bitmap_alloc(&msi_data->bitmap, msi_irqs,
- dev->dev.of_node);
- if (err)
- return err;
-
- err = msi_bitmap_reserve_dt_hwirqs(&msi_data->bitmap);
- if (err < 0) {
- msi_bitmap_free(&msi_data->bitmap);
- return err;
- }
-
- return 0;
-}
-
-static int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
-{
- int int_no = -ENOMEM;
- unsigned int virq;
- struct msi_msg msg;
- struct msi_desc *entry;
- struct ppc4xx_msi *msi_data = &ppc4xx_msi;
-
- dev_dbg(&dev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
- __func__, nvec, type);
- if (type == PCI_CAP_ID_MSIX)
- pr_debug("ppc4xx msi: MSI-X untested, trying anyway.\n");
-
- msi_data->msi_virqs = kmalloc_array(msi_irqs, sizeof(int), GFP_KERNEL);
- if (!msi_data->msi_virqs)
- return -ENOMEM;
-
- for_each_pci_msi_entry(entry, dev) {
- int_no = msi_bitmap_alloc_hwirqs(&msi_data->bitmap, 1);
- if (int_no >= 0)
- break;
- if (int_no < 0) {
- pr_debug("%s: fail allocating msi interrupt\n",
- __func__);
- }
- virq = irq_of_parse_and_map(msi_data->msi_dev, int_no);
- if (!virq) {
- dev_err(&dev->dev, "%s: fail mapping irq\n", __func__);
- msi_bitmap_free_hwirqs(&msi_data->bitmap, int_no, 1);
- return -ENOSPC;
- }
- dev_dbg(&dev->dev, "%s: virq = %d\n", __func__, virq)
Re: [patch V2 18/36] genirq/msi: Add msi_device_data::properties
Hello Thomas,
On 12/6/21 23:39, Thomas Gleixner wrote:
Add a properties field which allows core code to store information for easy
retrieval in order to replace MSI descriptor fiddling.
Signed-off-by: Thomas Gleixner
---
V2: Add a setter function to prepare for future changes
---
include/linux/msi.h | 17 +
kernel/irq/msi.c| 24
2 files changed, 41 insertions(+)
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -4,6 +4,7 @@
#include
#include
+#include
#include
/* Dummy shadow structures if an architecture does not define them */
@@ -153,6 +154,22 @@ struct msi_device_data {
int msi_setup_device_data(struct device *dev);
+/* MSI device properties */
+#define MSI_PROP_PCI_MSI BIT(0)
+#define MSI_PROP_PCI_MSIX BIT(1)
+#define MSI_PROP_64BIT BIT(2)
+
+#ifdef CONFIG_GENERIC_MSI_IRQ
+bool msi_device_has_property(struct device *dev, unsigned long prop);
+void msi_device_set_properties(struct device *dev, unsigned long prop);
+#else
+static inline bool msi_device_has_property(struct device *dev, unsigned long
prop)
+{
+ return false;
+}
+static inline void msi_device_set_properties(struct device *dev, unsigned long
prop) { }
+#endif
+
/* Helpers to hide struct msi_desc implementation details */
#define msi_desc_to_dev(desc) ((desc)->dev)
#define dev_to_msi_list(dev) (&(dev)->msi_list)
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -60,6 +60,30 @@ void free_msi_entry(struct msi_desc *ent
kfree(entry);
}
+/**
+ * msi_device_set_properties - Set device specific MSI properties
+ * @dev: Pointer to the device which is queried
+ * @prop: Properties to set
+ */
+void msi_device_set_properties(struct device *dev, unsigned long prop)
+{
+ if (WARN_ON_ONCE(!dev->msi.data))
+ return ;
+ dev->msi.data->properties = 0;
It would work better if the prop variable was used instead of 0.
With that fixed,
Reviewed-by: Cédric Le Goater
Thanks,
C.
+}
+
+/**
+ * msi_device_has_property - Check whether a device has a specific MSI property
+ * @dev: Pointer to the device which is queried
+ * @prop: Property to check for
+ */
+bool msi_device_has_property(struct device *dev, unsigned long prop)
+{
+ if (!dev->msi.data)
+ return false;
+ return !!(dev->msi.data->properties & prop);
+}
+
void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
{
*msg = entry->msg;
Re: [patch V2 01/23] powerpc/4xx: Remove MSI support which never worked
On 12/7/21 12:36, Michael Ellerman wrote:
Cédric Le Goater writes:
Hello Thomas,
On 12/6/21 23:27, Thomas Gleixner wrote:
This code is broken since day one. ppc4xx_setup_msi_irqs() has the
following gems:
1) The handling of the result of msi_bitmap_alloc_hwirqs() is completely
broken:
When the result is greater than or equal 0 (bitmap allocation
successful) then the loop terminates and the function returns 0
(success) despite not having installed an interrupt.
When the result is less than 0 (bitmap allocation fails), it prints an
error message and continues to "work" with that error code which would
eventually end up in the MSI message data.
2) On every invocation the file global pp4xx_msi::msi_virqs bitmap is
allocated thereby leaking the previous one.
IOW, this has never worked and for more than 10 years nobody cared. Remove
the gunk.
Fixes: 3fb7933850fa ("powerpc/4xx: Adding PCIe MSI support")
Shouldn't we remove all of it ? including the updates in the device trees
and the Kconfig changes under :
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/44x/Kconfig: select PPC4xx_MSI
arch/powerpc/platforms/40x/Kconfig: select PPC4xx_MSI
This patch should drop those selects I guess. Can you send an
incremental diff for Thomas to squash in?
Sure.
Removing all the tendrils in various device tree files will probably
require some archaeology, and it should be perfectly safe to leave those
in the tree with the driver gone. So I think we can do that as a
subsequent patch, rather than in this series.
Here are the changes. Compiled tested with ppc40x and ppc44x defconfigs.
Thanks,
C.
diff --git a/arch/powerpc/boot/dts/bluestone.dts
b/arch/powerpc/boot/dts/bluestone.dts
index aa1ae94cd776..6971595319c1 100644
--- a/arch/powerpc/boot/dts/bluestone.dts
+++ b/arch/powerpc/boot/dts/bluestone.dts
@@ -366,30 +366,5 @@ PCIE0: pcie@d {
0x0 0x0 0x0 0x3 &UIC3 0xe 0x4 /* swizzled int C
*/
0x0 0x0 0x0 0x4 &UIC3 0xf 0x4 /* swizzled int D
*/>;
};
-
- MSI: ppc4xx-msi@C1000 {
- compatible = "amcc,ppc4xx-msi", "ppc4xx-msi";
- reg = < 0xC 0x1000 0x100
- 0xC 0x1000 0x100>;
- sdr-base = <0x36C>;
- msi-data = <0x4440>;
- msi-mask = <0xffe0>;
- interrupts =<0 1 2 3 4 5 6 7>;
- interrupt-parent = <&MSI>;
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- msi-available-ranges = <0x0 0x100>;
- interrupt-map = <
- 0 &UIC3 0x18 1
- 1 &UIC3 0x19 1
- 2 &UIC3 0x1A 1
- 3 &UIC3 0x1B 1
- 4 &UIC3 0x1C 1
- 5 &UIC3 0x1D 1
- 6 &UIC3 0x1E 1
- 7 &UIC3 0x1F 1
- >;
- };
};
};
diff --git a/arch/powerpc/boot/dts/canyonlands.dts
b/arch/powerpc/boot/dts/canyonlands.dts
index c5fbb08e0a6e..5db1bff6b23d 100644
--- a/arch/powerpc/boot/dts/canyonlands.dts
+++ b/arch/powerpc/boot/dts/canyonlands.dts
@@ -544,23 +544,5 @@ PCIE1: pcie@d2000 {
0x0 0x0 0x0 0x3 &UIC3 0x12 0x4 /* swizzled int
C */
0x0 0x0 0x0 0x4 &UIC3 0x13 0x4 /* swizzled int D
*/>;
};
-
- MSI: ppc4xx-msi@C1000 {
- compatible = "amcc,ppc4xx-msi", "ppc4xx-msi";
- reg = < 0xC 0x1000 0x100>;
- sdr-base = <0x36C>;
- msi-data = <0x>;
- msi-mask = <0x>;
- interrupt-count = <3>;
- interrupts = <0 1 2 3>;
- interrupt-parent = <&UIC3>;
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- interrupt-map = <0 &UIC3 0x18 1
- 1 &UIC3 0x19 1
- 2 &UIC3 0x1A 1
- 3 &UIC3 0x1B 1>;
Re: [patch V2 29/36] PCI/MSI: Simplify pci_irq_get_affinity()
Thomas,
On 12/6/21 23:39, Thomas Gleixner wrote:
Replace open coded MSI descriptor chasing and use the proper accessor
functions instead.
Signed-off-by: Thomas Gleixner
Reviewed-by: Greg Kroah-Hartman
Reviewed-by: Jason Gunthorpe
---
drivers/pci/msi/msi.c | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -1056,26 +1056,20 @@ EXPORT_SYMBOL(pci_irq_vector);
*/
const struct cpumask *pci_irq_get_affinity(struct pci_dev *dev, int nr)
{
- if (dev->msix_enabled) {
- struct msi_desc *entry;
+ int irq = pci_irq_vector(dev, nr);
+ struct msi_desc *desc;
- for_each_pci_msi_entry(entry, dev) {
- if (entry->msi_index == nr)
- return &entry->affinity->mask;
- }
- WARN_ON_ONCE(1);
+ if (WARN_ON_ONCE(irq <= 0))
return NULL;
- } else if (dev->msi_enabled) {
- struct msi_desc *entry = first_pci_msi_entry(dev);
- if (WARN_ON_ONCE(!entry || !entry->affinity ||
-nr >= entry->nvec_used))
- return NULL;
-
- return &entry->affinity[nr].mask;
- } else {
+ desc = irq_get_msi_desc(irq);
+ /* Non-MSI does not have the information handy */
+ if (!desc)
return cpu_possible_mask;
- }
+
+ if (WARN_ON_ONCE(!desc->affinity))
+ return NULL;
+ return &desc->affinity[nr].mask;
}
EXPORT_SYMBOL(pci_irq_get_affinity);
This is breaking nvme on pseries but it's probably one of the previous
patches. I haven't figured out what's wrong yet. Here is the oops FYI.
Thanks,
C.
[ 32.494536] [ cut here ]
[ 32.494562] WARNING: CPU: 26 PID: 658 at kernel/irq/chip.c:210
irq_startup+0x1c0/0x1e0
[ 32.494575] Modules linked in: ibmvscsi ibmveth scsi_transport_srp bnx2x ipr
libata xhci_pci xhci_hcd nvme xts vmx_crypto nvme_core mdio t10_pi libcrc32c
dm_mirror dm_region_hash dm_log dm_mod
[ 32.494601] CPU: 26 PID: 658 Comm: kworker/26:1H Not tainted 5.16.0-rc4-clg+
#54
[ 32.494607] Workqueue: kblockd blk_mq_timeout_work
[ 32.494615] NIP: c0206f00 LR: c0206df0 CTR: c0201570
[ 32.494619] REGS: c018050f3610 TRAP: 0700 Not tainted
(5.16.0-rc4-clg+)
[ 32.494624] MSR: 8282b033 CR:
44002288 XER:
[ 32.494636] CFAR: c0206e0c IRQMASK: 1
[ 32.494636] GPR00: c0206df0 c018050f38b0 c1ca2900
0800
[ 32.494636] GPR04: c1ce21b8 0800 0800
[ 32.494636] GPR08: 0200
fffd
[ 32.494636] GPR12: c01fff7c5880 c018f488
c0012faaba40
[ 32.494636] GPR16:
0001
[ 32.494636] GPR20: c018050f3c40 c076e110
c0013ac23678
[ 32.494636] GPR24: 007f 0100 0001
c01805b08000
[ 32.494636] GPR28: c00139b8cc18 0001 0001
c00139b8cc00
[ 32.494681] NIP [c0206f00] irq_startup+0x1c0/0x1e0
[ 32.494686] LR [c0206df0] irq_startup+0xb0/0x1e0
[ 32.494690] Call Trace:
[ 32.494692] [c018050f38b0] [c0206df0] irq_startup+0xb0/0x1e0
(unreliable)
[ 32.494699] [c018050f38f0] [c020155c] __enable_irq+0x9c/0xb0
[ 32.494705] [c018050f3950] [c02015d0] enable_irq+0x60/0xc0
[ 32.494710] [c018050f39d0] [c00814a54ae8]
nvme_poll_irqdisable+0x80/0xc0 [nvme]
[ 32.494719] [c018050f3a00] [c00814a55824] nvme_timeout+0x18c/0x420
[nvme]
[ 32.494726] [c018050f3ae0] [c076e1b8]
blk_mq_check_expired+0xa8/0x130
[ 32.494732] [c018050f3b10] [c07793e8] bt_iter+0xd8/0x120
[ 32.494737] [c018050f3b60] [c077a34c]
blk_mq_queue_tag_busy_iter+0x25c/0x3f0
[ 32.494742] [c018050f3c20] [c076ffa4]
blk_mq_timeout_work+0x84/0x1a0
[ 32.494747] [c018050f3c70] [c0182a78]
process_one_work+0x2a8/0x5a0
[ 32.494754] [c018050f3d10] [c0183468] worker_thread+0xa8/0x610
[ 32.494759] [c018050f3da0] [c018f634] kthread+0x1b4/0x1c0
[ 32.494764] [c018050f3e10] [c000cd64]
ret_from_kernel_thread+0x5c/0x64
[ 32.494769] Instruction dump:
[ 32.494773] 6000 0b03 38a0 7f84e378 7fc3f378 4bff9a55 6000
7fe3fb78
[ 32.494781] 4bfffd79 eb810020 7c7e1b78 4bfffe94 <0fe0> 6000 6000
6042
[ 32.494788] ---[ end trace 2a27b87f2b3e7a1f ]---
[ 32.494798] nvme nvme0: I/O 192 QID 128 timeout, aborting
[ 32.584562] nvme nvme0: Abort status: 0x0
[ 62.574526] nvme nvme0: I/O 200 QID 128 timeout, aborting
[ 62.574587] nvme0n1: p1
Re: [patch V2 01/23] powerpc/4xx: Remove MSI support which never worked
On 12/7/21 21:42, Thomas Gleixner wrote: Cedric, On Tue, Dec 07 2021 at 16:50, Cédric Le Goater wrote: On 12/7/21 12:36, Michael Ellerman wrote: This patch should drop those selects I guess. Can you send an incremental diff for Thomas to squash in? Sure. Removing all the tendrils in various device tree files will probably require some archaeology, and it should be perfectly safe to leave those in the tree with the driver gone. So I think we can do that as a subsequent patch, rather than in this series. Here are the changes. Compiled tested with ppc40x and ppc44x defconfigs. < Lots of patch skipped /> @@ -141,7 +138,6 @@ config REDWOOD select FORCE_PCI select PPC4xx_PCI_EXPRESS select PCI_MSI - select PPC4xx_MSI help This option enables support for the AMCC PPC460SX Redwood board. While that is incremental it certainly is worth a patch on it's own. Could you add a proper changelog and an SOB please? Here you are. https://github.com/legoater/linux/commit/75d2764b11fe8f6d8bf50d60a3feb599ce27b16d Thanks, C.
Re: [patch V3 28/35] PCI/MSI: Simplify pci_irq_get_affinity()
On 12/18/21 11:25, Thomas Gleixner wrote:
On Fri, Dec 17 2021 at 15:30, Nathan Chancellor wrote:
On Fri, Dec 10, 2021 at 11:19:26PM +0100, Thomas Gleixner wrote:
I just bisected a boot failure on my AMD test desktop to this patch as
commit f48235900182 ("PCI/MSI: Simplify pci_irq_get_affinity()") in
-next. It looks like there is a problem with the NVMe drive after this
change according to the logs. Given that the hard drive is not getting
mounted for journald to write logs to, I am not really sure how to get
them from the machine so I have at least taken a picture of what I see
on my screen; open to ideas on that front!
Bah. Fix below.
That's a fix for the issue I was seeing on pseries with NVMe.
Tested-by: Cédric Le Goater
Thanks,
C.
Thanks,
tglx
---
diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 71802410e2ab..9b4910befeda 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -1100,7 +1100,7 @@ EXPORT_SYMBOL(pci_irq_vector);
*/
const struct cpumask *pci_irq_get_affinity(struct pci_dev *dev, int nr)
{
- int irq = pci_irq_vector(dev, nr);
+ int idx, irq = pci_irq_vector(dev, nr);
struct msi_desc *desc;
if (WARN_ON_ONCE(irq <= 0))
@@ -1113,7 +1113,10 @@ const struct cpumask *pci_irq_get_affinity(struct
pci_dev *dev, int nr)
if (WARN_ON_ONCE(!desc->affinity))
return NULL;
- return &desc->affinity[nr].mask;
+
+ /* MSI has a mask array in the descriptor. */
+ idx = dev->msi_enabled ? nr : 0;
+ return &desc->affinity[idx].mask;
}
EXPORT_SYMBOL(pci_irq_get_affinity);
Re: [patch] genirq/msi: Populate sysfs entry only once
On 1/10/22 19:12, Thomas Gleixner wrote:
The MSI entries for multi-MSI are populated en bloc for the MSI descriptor,
but the current code invokes the population inside the per interrupt loop
which triggers a warning in the sysfs code and causes the interrupt
allocation to fail.
Move it outside of the loop so it works correctly for single and multi-MSI.
Fixes: bf5e758f02fc ("genirq/msi: Simplify sysfs handling")
Reported-by: Borislav Petkov
Signed-off-by: Thomas Gleixner
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
kernel/irq/msi.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -887,12 +887,11 @@ int __msi_domain_alloc_irqs(struct irq_d
ret = msi_init_virq(domain, virq + i, vflags);
if (ret)
return ret;
-
- if (info->flags & MSI_FLAG_DEV_SYSFS) {
- ret = msi_sysfs_populate_desc(dev, desc);
- if (ret)
- return ret;
- }
+ }
+ if (info->flags & MSI_FLAG_DEV_SYSFS) {
+ ret = msi_sysfs_populate_desc(dev, desc);
+ if (ret)
+ return ret;
}
allocated++;
}
[PATCH for-9.1 v5 11/14] memory: Add Error** argument to the global_dirty_log routines
Now that the log_global*() handlers take an Error** parameter and
return a bool, do the same for memory_global_dirty_log_start() and
memory_global_dirty_log_stop(). The error is reported in the callers
for now and it will be propagated in the call stack in the next
changes.
To be noted a functional change in ram_init_bitmaps(), if the dirty
pages logger fails to start, there is no need to synchronize the dirty
pages bitmaps. colo_incoming_start_dirty_log() could be modified in a
similar way.
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: David Hildenbrand
Cc: Hyman Huang
Signed-off-by: Cédric Le Goater
---
Changes in v5:
- Removed Yong Huang's R-b
- Made use of ram_bitmaps_destroy() in ram_init_bitmaps() to cleanup
allocated bitmaps
include/exec/memory.h | 5 -
hw/i386/xen/xen-hvm.c | 2 +-
migration/dirtyrate.c | 13 +++--
migration/ram.c | 23 +--
system/memory.c | 11 +--
5 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index
567bc4c9fdb53e8f63487f1400980275687d..c129ee6db7162504bd72d4cfc69b5affb2cd87e8
100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2570,8 +2570,11 @@ void memory_listener_unregister(MemoryListener
*listener);
* memory_global_dirty_log_start: begin dirty logging for all regions
*
* @flags: purpose of starting dirty log, migration or dirty rate
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void memory_global_dirty_log_start(unsigned int flags);
+bool memory_global_dirty_log_start(unsigned int flags, Error **errp);
/**
* memory_global_dirty_log_stop: end dirty logging for all regions
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index
f6e9a1bc86491783077b5cb5aafdb19ab294e392..006d219ad52d739cc406ad5f8082ca82c16c61cc
100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -669,7 +669,7 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t
length)
void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
{
if (enable) {
-memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
+memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
} else {
memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
}
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index
1d2e85746fb7b10eb7f149976970f9a92125af8a..d02d70b7b4b86a29d4d5540ded416543536d8f98
100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -90,9 +90,15 @@ static int64_t do_calculate_dirtyrate(DirtyPageRecord
dirty_pages,
void global_dirty_log_change(unsigned int flag, bool start)
{
+Error *local_err = NULL;
+bool ret;
+
bql_lock();
if (start) {
-memory_global_dirty_log_start(flag);
+ret = memory_global_dirty_log_start(flag, &local_err);
+if (!ret) {
+error_report_err(local_err);
+}
} else {
memory_global_dirty_log_stop(flag);
}
@@ -608,9 +614,12 @@ static void calculate_dirtyrate_dirty_bitmap(struct
DirtyRateConfig config)
{
int64_t start_time;
DirtyPageRecord dirty_pages;
+Error *local_err = NULL;
bql_lock();
-memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE);
+if (!memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE, &local_err)) {
+error_report_err(local_err);
+}
/*
* 1'round of log sync may return all 1 bits with
diff --git a/migration/ram.c b/migration/ram.c
index
f0bd71438a4f7212118593b51648b645737933d4..bade3e9281ae839578033524b800dcf3c6f486dc
100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2862,18 +2862,32 @@ static void
migration_bitmap_clear_discarded_pages(RAMState *rs)
static void ram_init_bitmaps(RAMState *rs)
{
+Error *local_err = NULL;
+bool ret = true;
+
qemu_mutex_lock_ramlist();
WITH_RCU_READ_LOCK_GUARD() {
ram_list_init_bitmaps();
/* We don't use dirty log with background snapshots */
if (!migrate_background_snapshot()) {
-memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
+ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION,
+&local_err);
+if (!ret) {
+error_report_err(local_err);
+goto out_unlock;
+}
migration_bitmap_sync_precopy(rs, false);
}
}
+out_unlock:
qemu_mutex_unlock_ramlist();
+if (!ret) {
+ram_bitmaps_destroy();
+return;
+}
+
/*
* After an eventual first bitmap sync, fixup the initial bitmap
* containing all 1s to exclude any discarded pages from migration.
@@ -3665,6 +3679,8 @@ int colo_init_ram_cache(void)
void colo_incoming_start_di
[PATCH for-9.1 v5 09/14] memory: Add Error** argument to .log_global_start() handler
Modify all .log_global_start() handlers to take an Error** parameter
and return a bool. Adapt memory_global_dirty_log_start() to interrupt
on the first error the loop on handlers. In such case, a rollback is
performed to stop dirty logging on all listeners where it was
previously enabled.
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: David Hildenbrand
Signed-off-by: Cédric Le Goater
---
Changes in v5:
- Removed memory_global_dirty_log_rollback
- Introduced memory_global_dirty_log_do_start() to call
.log_global_start() handlers and do the rollback in case of error.
- Kept modification of the global_dirty_tracking flag within
memory_global_dirty_log_start()
- Added an assert on error of a .log_global_start() handler in
listener_add_address_space()
include/exec/memory.h | 5 -
hw/i386/xen/xen-hvm.c | 3 ++-
hw/vfio/common.c | 4 +++-
hw/virtio/vhost.c | 3 ++-
system/memory.c | 37 +++--
5 files changed, 46 insertions(+), 6 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index
8626a355b310ed7b1a1db7978ba4b394032c2f15..567bc4c9fdb53e8f63487f1400980275687d
100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -998,8 +998,11 @@ struct MemoryListener {
* active at that time.
*
* @listener: The #MemoryListener.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
*/
-void (*log_global_start)(MemoryListener *listener);
+bool (*log_global_start)(MemoryListener *listener, Error **errp);
/**
* @log_global_stop:
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index
7745cb39631ea423aeb6e5d3eb7f7bcbe27ec6fa..f6e9a1bc86491783077b5cb5aafdb19ab294e392
100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -457,11 +457,12 @@ static void xen_log_sync(MemoryListener *listener,
MemoryRegionSection *section)
int128_get64(section->size));
}
-static void xen_log_global_start(MemoryListener *listener)
+static bool xen_log_global_start(MemoryListener *listener, Error **errp)
{
if (xen_enabled()) {
xen_in_migration = true;
}
+return true;
}
static void xen_log_global_stop(MemoryListener *listener)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
011ceaab89433de4496dffadc737286e053f321d..8f9cbdc0264044ce587877a7d19d14b28527291b
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1066,7 +1066,8 @@ out:
return ret;
}
-static void vfio_listener_log_global_start(MemoryListener *listener)
+static bool vfio_listener_log_global_start(MemoryListener *listener,
+ Error **errp)
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
@@ -1083,6 +1084,7 @@ static void vfio_listener_log_global_start(MemoryListener
*listener)
ret, strerror(-ret));
vfio_set_migration_error(ret);
}
+return !ret;
}
static void vfio_listener_log_global_stop(MemoryListener *listener)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index
2e4e040db87acf45166da86d268077f54511d82c..d405f03caf2fd3a5ea23bdc0392f4c6c072bc10b
100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1044,7 +1044,7 @@ check_dev_state:
return r;
}
-static void vhost_log_global_start(MemoryListener *listener)
+static bool vhost_log_global_start(MemoryListener *listener, Error **errp)
{
int r;
@@ -1052,6 +1052,7 @@ static void vhost_log_global_start(MemoryListener
*listener)
if (r < 0) {
abort();
}
+return true;
}
static void vhost_log_global_stop(MemoryListener *listener)
diff --git a/system/memory.c b/system/memory.c
index
a229a79988fce2aa3cb77e3a130db4c694e8cd49..ca4d91484fb3d06f4b902486fea49dba86dc141b
100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2914,9 +2914,33 @@ static unsigned int postponed_stop_flags;
static VMChangeStateEntry *vmstate_change;
static void memory_global_dirty_log_stop_postponed_run(void);
+static bool memory_global_dirty_log_do_start(Error **errp)
+{
+MemoryListener *listener;
+
+QTAILQ_FOREACH(listener, &memory_listeners, link) {
+if (listener->log_global_start) {
+if (!listener->log_global_start(listener, errp)) {
+goto err;
+}
+}
+}
+return true;
+
+err:
+while ((listener = QTAILQ_PREV(listener, link)) != NULL) {
+if (listener->log_global_stop) {
+listener->log_global_stop(listener);
+}
+}
+
+return false;
+}
+
void memory_global_dirty_log_start(unsigned int flags)
{
unsigned int old_flags;
+Error *local_err = NULL;
assert(flags && !(flags & (
Re: [PATCH for-9.1 v5 09/14] memory: Add Error** argument to .log_global_start() handler
On 3/20/24 15:42, Peter Xu wrote:
On Wed, Mar 20, 2024 at 07:49:05AM +0100, Cédric Le Goater wrote:
Modify all .log_global_start() handlers to take an Error** parameter
and return a bool. Adapt memory_global_dirty_log_start() to interrupt
on the first error the loop on handlers. In such case, a rollback is
performed to stop dirty logging on all listeners where it was
previously enabled.
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: David Hildenbrand
Signed-off-by: Cédric Le Goater
Reviewed-by: Peter Xu
Still one comment below:
@@ -3014,8 +3044,11 @@ static void listener_add_address_space(MemoryListener
*listener,
listener->begin(listener);
}
if (global_dirty_tracking) {
+/*
+ * Migration has already started. Assert on any error.
If you won't mind, I can change this to:
/*
* Currently only VFIO can fail log_global_start(), and it's not allowed
* to hotplug a VFIO device during migration, so this should never fail
* when invoked. If it can start to fail in the future, we need to be
* able to fail the whole listener_add_address_space() and its callers.
*/
Sure, or I will in a v6. Markus had a comment on 8/14.
Thanks,
C.
Re: [PATCH 1/6] system/cpus: rename qemu_mutex_lock_iothread() to qemu_bql_lock()
On 11/29/23 22:26, Stefan Hajnoczi wrote: The Big QEMU Lock (BQL) has many names and they are confusing. The actual QemuMutex variable is called qemu_global_mutex but it's commonly referred to as the BQL in discussions and some code comments. The locking APIs, however, are called qemu_mutex_lock_iothread() and qemu_mutex_unlock_iothread(). The "iothread" name is historic and comes from when the main thread was split into into KVM vcpu threads and the "iothread" (now called the main loop thread). I have contributed to the confusion myself by introducing a separate --object iothread, a separate concept unrelated to the BQL. The "iothread" name is no longer appropriate for the BQL. Rename the locking APIs to: - void qemu_bql_lock(void) - void qemu_bql_unlock(void) - bool qemu_bql_locked(void) There are more APIs with "iothread" in their names. Subsequent patches will rename them. There are also comments and documentation that will be updated in later patches. Signed-off-by: Stefan Hajnoczi Reviewed-by: Cédric Le Goater Thanks, C.
Re: [PATCH 2/6] qemu/main-loop: rename QEMU_IOTHREAD_LOCK_GUARD to QEMU_BQL_LOCK_GUARD
On 11/29/23 22:26, Stefan Hajnoczi wrote: The name "iothread" is overloaded. Use the term Big QEMU Lock (BQL) instead, it is already widely used and unambiguous. Signed-off-by: Stefan Hajnoczi Reviewed-by: Cédric Le Goater Thanks, C.
Re: [PATCH 3/6] qemu/main-loop: rename qemu_cond_wait_iothread() to qemu_cond_wait_bql()
On 11/29/23 22:26, Stefan Hajnoczi wrote: The name "iothread" is overloaded. Use the term Big QEMU Lock (BQL) instead, it is already widely used and unambiguous. Signed-off-by: Stefan Hajnoczi Reviewed-by: Cédric Le Goater Thanks, C.
Re: [PATCH 4/6] system/cpus: rename qemu_global_mutex to qemu_bql
On 11/29/23 22:26, Stefan Hajnoczi wrote: The APIs using qemu_global_mutex now follow the Big QEMU Lock (BQL) nomenclature. It's a little strange that the actual QemuMutex variable that embodies the BQL is called qemu_global_mutex instead of qemu_bql. Rename it for consistency. Signed-off-by: Stefan Hajnoczi Reviewed-by: Cédric Le Goater Thanks, C.
Re: [PATCH v3 4/5] Replace "iothread lock" with "BQL" in comments
On 1/2/24 16:35, Stefan Hajnoczi wrote: The term "iothread lock" is obsolete. The APIs use Big QEMU Lock (BQL) in their names. Update the code comments to use "BQL" instead of "iothread lock". Signed-off-by: Stefan Hajnoczi Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Thanks, C.
Re: [PATCH v3 5/5] Rename "QEMU global mutex" to "BQL" in comments and docs
On 1/2/24 16:35, Stefan Hajnoczi wrote:
The term "QEMU global mutex" is identical to the more widely used Big
QEMU Lock ("BQL"). Update the code comments and documentation to use
"BQL" instead of "QEMU global mutex".
Signed-off-by: Stefan Hajnoczi
Acked-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
Re: [PATCH v3 22/46] hw/arm/aspeed: use qemu_configure_nic_device()
On 1/8/24 21:26, David Woodhouse wrote:
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/aspeed.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cc59176563..bed5e4f40b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -356,7 +356,6 @@ static void aspeed_machine_init(MachineState *machine)
AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
AspeedSoCClass *sc;
int i;
-NICInfo *nd = &nd_table[0];
bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
@@ -371,10 +370,10 @@ static void aspeed_machine_init(MachineState *machine)
&error_fatal);
for (i = 0; i < sc->macs_num; i++) {
-if ((amc->macs_mask & (1 << i)) && nd->used) {
-qemu_check_nic_model(nd, TYPE_FTGMAC100);
-qdev_set_nic_properties(DEVICE(&bmc->soc->ftgmac100[i]), nd);
-nd++;
+if ((amc->macs_mask & (1 << i)) &&
+!qemu_configure_nic_device(DEVICE(&bmc->soc->ftgmac100[i]),
+ true, NULL)) {
+ break; /* No configs left; stop asking */
}
}
Acked-by: Cédric Le Goater
Thanks,
C.
Re: [PATCH v4 22/47] hw/arm/aspeed: use qemu_configure_nic_device()
On 1/26/24 18:24, David Woodhouse wrote:
From: David Woodhouse
Signed-off-by: David Woodhouse
Acked-by: Cédric Le Goater
and
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/arm/aspeed.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cc59176563..bed5e4f40b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -356,7 +356,6 @@ static void aspeed_machine_init(MachineState *machine)
AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
AspeedSoCClass *sc;
int i;
-NICInfo *nd = &nd_table[0];
bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
@@ -371,10 +370,10 @@ static void aspeed_machine_init(MachineState *machine)
&error_fatal);
for (i = 0; i < sc->macs_num; i++) {
-if ((amc->macs_mask & (1 << i)) && nd->used) {
-qemu_check_nic_model(nd, TYPE_FTGMAC100);
-qdev_set_nic_properties(DEVICE(&bmc->soc->ftgmac100[i]), nd);
-nd++;
+if ((amc->macs_mask & (1 << i)) &&
+!qemu_configure_nic_device(DEVICE(&bmc->soc->ftgmac100[i]),
+ true, NULL)) {
+break; /* No configs left; stop asking */
}
}
Re: [PATCH 3/3] net: checksum: Introduce fine control over checksum type
Hello !
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 782ff19..fbae1f1 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -573,7 +573,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t
> tx_ring,
> }
>
> if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
> -net_checksum_calculate(s->frame, frame_size);
> +/*
> + * TODO:
> + * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP
> checksum,
> + * however previous net_checksum_calculate() did not
> calculate
> + * IP checksum at all. Passing CSUM_ALL for now until someone
> + * who is familar with this MAC to figure out what should be
> + * properly added for TCP/UDP checksum offload.
> + */
> +net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
> }
> /* Last buffer in frame. */
> qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
You can test your changes using the HOWTO Joel provided here :
https://github.com/openbmc/qemu/wiki/Usage
Please also check the Linux driver :
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/faraday/ftgmac100.c#n685
That said, something like the change below should be more appropriate.
Thanks,
C.
+static int ftgmac100_convert_csum_flag(uint32_t flags)
+{
+int csum = 0;
+
+if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
+csum |= CSUM_IP;
+}
+if (flags & FTGMAC100_TXDES1_TCP_CHKSUM) {
+csum |= CSUM_TCP;
+}
+if (flags & FTGMAC100_TXDES1_UDP_CHKSUM) {
+csum |= CSUM_UDP;
+}
+return csum;
+}
+
static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
uint32_t tx_descriptor)
{
@@ -602,6 +618,7 @@ static void ftgmac100_do_tx(FTGMAC100Sta
ptr += len;
frame_size += len;
if (bd.des0 & FTGMAC100_TXDES0_LTS) {
+int csum = ftgmac100_convert_csum_flag(flags);
/* Check for VLAN */
if (flags & FTGMAC100_TXDES1_INS_VLANTAG &&
@@ -610,16 +627,8 @@ static void ftgmac100_do_tx(FTGMAC100Sta
FTGMAC100_TXDES1_VLANTAG_CI(flags));
}
-if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
-/*
- * TODO:
- * FTGMAC100_TXDES1_IP_CHKSUM seems to be only for IP checksum,
- * however previous net_checksum_calculate() did not calculate
- * IP checksum at all. Passing CSUM_ALL for now until someone
- * who is familar with this MAC to figure out what should be
- * properly added for TCP/UDP checksum offload.
- */
-net_checksum_calculate(s->frame, frame_size, CSUM_ALL);
+if (csum) {
+net_checksum_calculate(s->frame, frame_size, csum);
}
/* Last buffer in frame. */
qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
Re: [PATCH v2 3/3] net: checksum: Introduce fine control over checksum type
On 12/11/20 10:35 AM, Bin Meng wrote:
> From: Bin Meng
>
> At present net_checksum_calculate() blindly calculates all types of
> checksums (IP, TCP, UDP). Some NICs may have a per type setting in
> their BDs to control what checksum should be offloaded. To support
> such hardware behavior, introduce a 'csum_flag' parameter to the
> net_checksum_calculate() API to allow fine control over what type
> checksum is calculated.
>
> Existing users of this API are updated accordingly.
>
> Signed-off-by: Bin Meng
For the ftgmac100 part,
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
>
> Changes in v2:
> - update ftgmac100.c per Cédric Le Goater's suggestion
> - simplify fsl_etsec and imx_fec checksum logic
>
> include/net/checksum.h| 7 ++-
> hw/net/allwinner-sun8i-emac.c | 2 +-
> hw/net/cadence_gem.c | 2 +-
> hw/net/fsl_etsec/rings.c | 18 +-
> hw/net/ftgmac100.c| 13 -
> hw/net/imx_fec.c | 20
> hw/net/virtio-net.c | 2 +-
> hw/net/xen_nic.c | 2 +-
> net/checksum.c| 18 ++
> net/filter-rewriter.c | 4 ++--
> 10 files changed, 55 insertions(+), 33 deletions(-)
>
> diff --git a/include/net/checksum.h b/include/net/checksum.h
> index 05a0d27..7dec37e 100644
> --- a/include/net/checksum.h
> +++ b/include/net/checksum.h
> @@ -21,11 +21,16 @@
> #include "qemu/bswap.h"
> struct iovec;
>
> +#define CSUM_IP 0x01
> +#define CSUM_TCP0x02
> +#define CSUM_UDP0x04
> +#define CSUM_ALL(CSUM_IP | CSUM_TCP | CSUM_UDP)
> +
> uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
> uint16_t net_checksum_finish(uint32_t sum);
> uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
> uint8_t *addrs, uint8_t *buf);
> -void net_checksum_calculate(uint8_t *data, int length);
> +void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
>
> static inline uint32_t
> net_checksum_add(int len, uint8_t *buf)
> diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
> index 38d3285..0427689 100644
> --- a/hw/net/allwinner-sun8i-emac.c
> +++ b/hw/net/allwinner-sun8i-emac.c
> @@ -514,7 +514,7 @@ static void
> allwinner_sun8i_emac_transmit(AwSun8iEmacState *s)
> /* After the last descriptor, send the packet */
> if (desc.status2 & TX_DESC_STATUS2_LAST_DESC) {
> if (desc.status2 & TX_DESC_STATUS2_CHECKSUM_MASK) {
> -net_checksum_calculate(packet_buf, packet_bytes);
> +net_checksum_calculate(packet_buf, packet_bytes, CSUM_ALL);
> }
>
> qemu_send_packet(nc, packet_buf, packet_bytes);
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 7a53469..9a4474a 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1266,7 +1266,7 @@ static void gem_transmit(CadenceGEMState *s)
>
> /* Is checksum offload enabled? */
> if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
> -net_checksum_calculate(s->tx_packet, total_bytes);
> +net_checksum_calculate(s->tx_packet, total_bytes,
> CSUM_ALL);
> }
>
> /* Update MAC statistics */
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 628648a..121415a 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -183,13 +183,11 @@ static void process_tx_fcb(eTSEC *etsec)
> uint8_t *l3_header = etsec->tx_buffer + 8 + l3_header_offset;
> /* L4 header */
> uint8_t *l4_header = l3_header + l4_header_offset;
> +int csum = 0;
>
> /* if packet is IP4 and IP checksum is requested */
> if (flags & FCB_TX_IP && flags & FCB_TX_CIP) {
> -/* do IP4 checksum (TODO This function does TCP/UDP checksum
> - * but not sure if it also does IP4 checksum.) */
> -net_checksum_calculate(etsec->tx_buffer + 8,
> -etsec->tx_buffer_len - 8);
> +csum |= CSUM_IP;
> }
> /* TODO Check the correct usage of the PHCS field of the FCB in case the
> NPH
> * flag is on */
> @@ -201,9 +199,7 @@ static void process_tx_fcb(eTSEC *etsec)
> /* if checksum is requested */
> if (flags & FCB_TX_CTU) {
> /* do UDP checksum */
> -
> -net_checksum_calculate(etsec->tx_buffer + 8,
> -etsec->tx_buffer_len - 8);
>
Re: [PATCH 7/7] qobject atomics osdep: Make a few macros more hygienic
On 8/31/23 16:30, Eric Blake wrote:
On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
[This paragraph written last: Bear with my stream of consciousness
review below, where I end up duplicating some of the conslusions you
reached before the point where I saw where the patch was headed]
Variables declared in macros can shadow other variables. Much of the
time, this is harmless, e.g.:
#define _FDT(exp) \
do { \
int ret = (exp); \
if (ret < 0) { \
error_report("error creating device tree: %s: %s", \
#exp, fdt_strerror(ret)); \
exit(1); \
} \
} while (0)
Which is why I've seen some projects require a strict namespace
separation: if all macro parameters and any identifiers declared in
macros use either a leading or a trailing _ (I prefer a trailing one,
to avoid risking conflicts with libc reserved namespace; but leading
is usually okay), and all other identifiers avoid that namespace, then
you will never have shadowing by calling a macro from normal code.
I started fixing the _FDT() macro since it is quite noisy at compile.
Same for qemu_fdt_setprop_cells(). So are we ok with names like 'ret_'
and 'i_' ? I used a 'local_' prefix for now but I can change.
I also have a bunch of fixes for ppc.
C.
Harmless shadowing in h_client_architecture_support():
target_ulong ret;
[...]
ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
if (ret == H_SUCCESS) {
_FDT((fdt_pack(spapr->fdt_blob)));
[...]
}
return ret;
However, we can get in trouble when the shadowed variable is used in a
macro argument:
#define QOBJECT(obj) ({ \
typeof(obj) o = (obj); \
o ? container_of(&(o)->base, QObject, base) : NULL; \
})
QOBJECT(o) expands into
({
--->typeof(o) o = (o);
o ? container_of(&(o)->base, QObject, base) : NULL;
})
Unintended variable name capture at --->. We'd be saved by
-Winit-self. But I could certainly construct more elaborate death
traps that don't trigger it.
Indeed, not fully understanding the preprocessor makes for some
interesting death traps.
To reduce the risk of trapping ourselves, we use variable names in
macros that no sane person would use elsewhere. Here's our actual
definition of QOBJECT():
#define QOBJECT(obj) ({ \
typeof(obj) _obj = (obj); \
_obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
})
Yep, goes back to the policy I've seen of enforcing naming conventions
that ensure macros can't shadow normal code.
Works well enough until we nest macro calls. For instance, with
#define qobject_ref(obj) ({ \
typeof(obj) _obj = (obj); \
qobject_ref_impl(QOBJECT(_obj));\
_obj; \
})
the expression qobject_ref(obj) expands into
({
typeof(obj) _obj = (obj);
qobject_ref_impl(
({
--->typeof(_obj) _obj = (_obj);
_obj ? container_of(&(_obj)->base, QObject, base) : NULL;
}));
_obj;
})
Unintended variable name capture at --->.
Yep, you've just proven how macros calling macros requires care, as we
no longer have the namespace protections. If macro calls can only
form a DAG, it is possible to choose unique intermediate declarations
for every macro (although remembering to do that, and still keeping
the macro definition legible, may not be easy). But if you can have
macros that form any sort of nesting loop (and we WANT to allow things
like MIN(MIN(a, b), c) - which is such a loop), dynamic naming becomes
the only solution.
The only reliable way to prevent unintended variable name capture is
-Wshadow.
Yes, I would love to have that enabled eventually.
One blocker for enabling it is shadowing hiding in function-like
macros like
qdict_put(dict, "name", qobject_ref(...))
qdict_put() wraps its last argument in QOBJECT(), and the last
argument here contains another QOBJECT().
Use dark preprocessor sorcery to make the macros that give us this
problem use different variable names on every call.
Sounds foreboding; hopefully not many macros are affected...
Signed-off-by: Markus Armbruster
---
include/qapi/qmp/qobject.h | 8 +---
include/qemu/atomic.h | 11 ++-
include/qem
Re: [PATCH 7/7] qobject atomics osdep: Make a few macros more hygienic
On 9/1/23 16:50, Markus Armbruster wrote:
Cédric Le Goater writes:
On 8/31/23 16:30, Eric Blake wrote:
On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
[This paragraph written last: Bear with my stream of consciousness
review below, where I end up duplicating some of the conslusions you
reached before the point where I saw where the patch was headed]
Variables declared in macros can shadow other variables. Much of the
time, this is harmless, e.g.:
#define _FDT(exp) \
do { \
int ret = (exp); \
if (ret < 0) { \
error_report("error creating device tree: %s: %s", \
#exp, fdt_strerror(ret)); \
exit(1); \
} \
} while (0)
Which is why I've seen some projects require a strict namespace
separation: if all macro parameters and any identifiers declared in
macros use either a leading or a trailing _ (I prefer a trailing one,
to avoid risking conflicts with libc reserved namespace; but leading
is usually okay), and all other identifiers avoid that namespace, then
you will never have shadowing by calling a macro from normal code.
I started fixing the _FDT() macro since it is quite noisy at compile.
Same for qemu_fdt_setprop_cells(). So are we ok with names like 'ret_'
and 'i_' ? I used a 'local_' prefix for now but I can change.
I believe identifiers with a '_' suffix are just fine in macros. We
have quite a few of them already.
ok
I also have a bunch of fixes for ppc.
Appreciated!
count me on for the ppc files :
hw/ppc/pnv_psi.c
hw/ppc/spapr.c
hw/ppc/spapr_drc.c
hw/ppc/spapr_pci.c
include/hw/ppc/fdt.h
and surely some other files under target/ppc/
This one was taken care of by Phil:
include/sysemu/device_tree.h
C.
Re: [PATCH 41/71] hw/net: Constify all Property
On 12/13/24 20:07, Richard Henderson wrote:
Signed-off-by: Richard Henderson
---
hw/net/allwinner-sun8i-emac.c | 2 +-
hw/net/allwinner_emac.c| 2 +-
hw/net/cadence_gem.c | 2 +-
hw/net/can/xlnx-versal-canfd.c | 2 +-
hw/net/can/xlnx-zynqmp-can.c | 2 +-
hw/net/dp8393x.c | 2 +-
hw/net/e1000.c | 2 +-
hw/net/e1000e.c| 2 +-
hw/net/eepro100.c | 2 +-
hw/net/fsl_etsec/etsec.c | 2 +-
hw/net/ftgmac100.c | 4 ++--
hw/net/igb.c | 2 +-
hw/net/imx_fec.c | 2 +-
hw/net/lan9118.c | 2 +-
hw/net/lance.c | 2 +-
hw/net/lasi_i82596.c | 2 +-
hw/net/mcf_fec.c | 2 +-
hw/net/mipsnet.c | 2 +-
hw/net/msf2-emac.c | 2 +-
hw/net/mv88w8618_eth.c | 2 +-
hw/net/ne2000-isa.c| 2 +-
hw/net/ne2000-pci.c| 2 +-
hw/net/npcm7xx_emc.c | 2 +-
hw/net/npcm_gmac.c | 2 +-
hw/net/opencores_eth.c | 2 +-
hw/net/pcnet-pci.c | 2 +-
hw/net/rocker/rocker.c | 2 +-
hw/net/rtl8139.c | 2 +-
hw/net/smc91c111.c | 2 +-
hw/net/spapr_llan.c| 2 +-
hw/net/stellaris_enet.c| 2 +-
hw/net/sungem.c| 2 +-
hw/net/sunhme.c| 2 +-
hw/net/tulip.c | 2 +-
hw/net/virtio-net.c| 2 +-
hw/net/vmxnet3.c | 2 +-
hw/net/xen_nic.c | 2 +-
hw/net/xgmac.c | 2 +-
hw/net/xilinx_axienet.c| 2 +-
hw/net/xilinx_ethlite.c| 2 +-
40 files changed, 41 insertions(+), 41 deletions(-)
For the ftgmac100,
Reviewed-by: Cédric Le Goater
Thanks,
C.
diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c
index cdae74f503..3f03060bf5 100644
--- a/hw/net/allwinner-sun8i-emac.c
+++ b/hw/net/allwinner-sun8i-emac.c
@@ -829,7 +829,7 @@ static void allwinner_sun8i_emac_realize(DeviceState *dev,
Error **errp)
qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
}
-static Property allwinner_sun8i_emac_properties[] = {
+static const Property allwinner_sun8i_emac_properties[] = {
DEFINE_NIC_PROPERTIES(AwSun8iEmacState, conf),
DEFINE_PROP_UINT8("phy-addr", AwSun8iEmacState, mii_phy_addr, 0),
DEFINE_PROP_LINK("dma-memory", AwSun8iEmacState, dma_mr,
diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
index c104c2588e..39c10426cf 100644
--- a/hw/net/allwinner_emac.c
+++ b/hw/net/allwinner_emac.c
@@ -462,7 +462,7 @@ static void aw_emac_realize(DeviceState *dev, Error **errp)
fifo8_create(&s->tx_fifo[1], TX_FIFO_SIZE);
}
-static Property aw_emac_properties[] = {
+static const Property aw_emac_properties[] = {
DEFINE_NIC_PROPERTIES(AwEmacState, conf),
DEFINE_PROP_UINT8("phy-addr", AwEmacState, phy_addr, 0),
DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 526739887c..3fce01315f 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1784,7 +1784,7 @@ static const VMStateDescription vmstate_cadence_gem = {
}
};
-static Property gem_properties[] = {
+static const Property gem_properties[] = {
DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
GEM_MODID_VALUE),
diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index e148bd7b46..97fa46c4b3 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -2042,7 +2042,7 @@ static const VMStateDescription vmstate_canfd = {
}
};
-static Property canfd_core_properties[] = {
+static const Property canfd_core_properties[] = {
DEFINE_PROP_UINT8("rx-fifo0", XlnxVersalCANFDState, cfg.rx0_fifo, 0x40),
DEFINE_PROP_UINT8("rx-fifo1", XlnxVersalCANFDState, cfg.rx1_fifo, 0x40),
DEFINE_PROP_UINT8("tx-fifo", XlnxVersalCANFDState, cfg.tx_fifo, 0x20),
diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
index 58f1432bb3..61c104c18b 100644
--- a/hw/net/can/xlnx-zynqmp-can.c
+++ b/hw/net/can/xlnx-zynqmp-can.c
@@ -1169,7 +1169,7 @@ static const VMStateDescription vmstate_can = {
}
};
-static Property xlnx_zynqmp_can_properties[] = {
+static const Property xlnx_zynqmp_can_properties[] = {
DEFINE_PROP_UINT32("ext_clk_freq", XlnxZynqMPCANState, cfg.ext_clk_freq,
CAN_DEFAULT_CLOCK),
DEFINE_PROP_LINK("canbus", XlnxZynqMPCANState, canbus, TYPE_CAN_BUS,
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index c0977308ba..e3ca11991b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -931,7 +931,7 @@ static const VMStateDescription vmstate_dp8393x =
