Re: elf2dmp: Fix memory leak on main() error paths
Le 11/09/2020 à 06:18, Thomas Huth a écrit : > On 26/08/2020 12.15, AlexChen wrote: >> From: AlexChen >> >> The 'kdgb' is allocating memory in get_kdbg(), but it is not freed >> in both fill_header() and fill_context() failed branches, fix it. >> >> Signed-off-by: AlexChen >> --- >> contrib/elf2dmp/main.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c >> index 9a2dbc2902..ac746e49e0 100644 >> --- a/contrib/elf2dmp/main.c >> +++ b/contrib/elf2dmp/main.c >> @@ -568,12 +568,12 @@ int main(int argc, char *argv[]) >> if (fill_header(&header, &ps, &vs, KdDebuggerDataBlock, kdbg, >> KdVersionBlock, qemu_elf.state_nr)) { >> err = 1; >> -goto out_pdb; >> +goto out_kdbg; >> } >> >> if (fill_context(kdbg, &vs, &qemu_elf)) { >> err = 1; >> -goto out_pdb; >> +goto out_kdbg; >> } >> >> if (write_dump(&ps, &header, argv[2])) { >> > > I think this could go via qemu-trivial (now on CC:). > > Reviewed-by: Thomas Huth > > Applied to my trivial-patches branch. Thanks, Laurent
Re: [PATCH 00/10] edk2: adopt the edk2-stable202008 release
On 09/10/20 21:31, Philippe Mathieu-Daudé wrote: > On 9/8/20 9:29 AM, Laszlo Ersek wrote: >> Ref:https://bugs.launchpad.net/qemu/+bug/1852196 >> Repo: https://github.com/lersek/qemu.git >> Branch: edk2stable202008_lp_1852196 >> >> This series consumes the following upstream edk2 releases: >> >> https://github.com/tianocore/edk2/releases/tag/edk2-stable201908 >> https://github.com/tianocore/edk2/releases/tag/edk2-stable201911 >> https://github.com/tianocore/edk2/releases/tag/edk2-stable202002 >> https://github.com/tianocore/edk2/releases/tag/edk2-stable202005 >> https://github.com/tianocore/edk2/releases/tag/edk2-stable202008 >> >> Worth mentioning (in random order): >> >> - various CVE fixes (see shortlog) >> - OpenSSL-1.1.1g >> - UEFI HTTPS Boot for ARM/AARCH64 >> - TPM2 for ARM/AARCH64 >> - VCPU hotplug with SMI >> - support for Linux v5.7+ initrd and mixed mode loading >> - Fusion-MPT SCSI driver in OVMF >> - VMware PVSCSI driver in OVMF >> - PXEv4 / PXEv6 boot possible to disable on the QEMU command line >> - SEV-ES support >> >> The IA32 and X64 binaries are now smaller -- the reason is that I buit >> them with DevToolSet 9 (gcc-9) on RHEL7, and so this is the first time >> they've undergone LTO (with the GCC5 edk2 toolchain settings). >> >> Cc: "Michael S. Tsirkin" >> Cc: Igor Mammedov >> Cc: Philippe Mathieu-Daudé >> >> Thanks, >> Laszlo >> >> Laszlo Ersek (10): >> Makefile: remove obsolete edk2 exception from "clean" rule >> roms/efirom, tests/uefi-test-tools: update edk2's own submodules first >> roms/Makefile.edk2: prepare for replacing TPM2*_ENABLE macros >> tests: acpi: tolerate "virt/SSDT.memhp" mismatch temporarily >> roms/edk2: update submodule from edk2-stable201905 to >> edk2-stable202008 >> roms/Makefile.edk2: complete replacing TPM2*_ENABLE macros >> roms/Makefile.edk2: enable new ARM/AARCH64 flags up to >> edk2-stable202008 >> pc-bios: refresh edk2 build artifacts for edk2-stable202008 >> pc-bios: update the README file with edk2-stable202008 information >> tests: acpi: update "virt/SSDT.memhp" for edk2-stable202008 >> >> Makefile | 1 - >> pc-bios/README | 4 +-- >> pc-bios/edk2-aarch64-code.fd.bz2 | Bin 1178070 -> 1507722 bytes >> pc-bios/edk2-arm-code.fd.bz2 | Bin 1172752 -> 1503187 bytes >> pc-bios/edk2-i386-code.fd.bz2 | Bin 1736199 -> 1646741 bytes >> pc-bios/edk2-i386-secure-code.fd.bz2 | Bin 1943949 -> 1860546 bytes >> pc-bios/edk2-x86_64-code.fd.bz2| Bin 1717094 -> 1680164 bytes >> pc-bios/edk2-x86_64-secure-code.fd.bz2 | Bin 1958037 -> 1912112 bytes >> roms/Makefile | 1 + >> roms/Makefile.edk2 | 26 >> roms/edk2 | 2 +- >> tests/data/acpi/virt/SSDT.memhp| Bin 736 -> 736 bytes >> tests/uefi-test-tools/Makefile | 1 + >> 13 files changed, 21 insertions(+), 14 deletions(-) > > Series: > Tested-by: Philippe Mathieu-Daudé > > And applied to the edk2-next tree. > > Thanks, > > Phil. > Thank you Phil! Laszlo
PATCH: Increase System Firmware Max Size
Hi all, (this is my first Qemu patch submission, please let me know if my formatting/content needs to be fixed). We have a need for increased firmware size, currently we are building Qemu with the following change to test our Uefi Firmware and it works well for us. Hope that this change can be made to open source. Thank you. --- Increase allowed system firmware size to 16M per comment suggesting up to 18M is permissible. Signed-off-by: Erich McMillan diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -46,7 +46,7 @@ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in * size. */ -#define FLASH_SIZE_LIMIT (8 * MiB) +#define FLASH_SIZE_LIMIT (16 * MiB) #define FLASH_SECTOR_SIZE 4096 ---
Re: [PATCH 2/6] hw/core/stream: Rename StreamSlave as StreamSink
On 10/09/20 09:01, Philippe Mathieu-Daudé wrote: > In order to use inclusive terminology, rename 'slave stream' > as 'sink stream'. > > Signed-off-by: Philippe Mathieu-Daudé >From Edgar Iglesias: Regarding streams, our stream module can be used to model a stream channel such as AXI stream but also other similar stream protocols. We actually don't use the AXI stream terminology [in hw/core/stream.c]. E.g, we use buf instead of DATA, EOP (end-of-packet) instead of LAST and have a flow-control mechanism that doesn't refer to valid/ready. IMO, since we're not matching specific protocol names, it would be fine to switch to generic terms like Source and Sink. Therefore, Acked-by: Paolo Bonzini Paolo > --- > include/hw/ssi/xilinx_spips.h | 2 +- > include/hw/stream.h | 46 +-- > hw/core/stream.c | 20 +++ > hw/dma/xilinx_axidma.c| 32 > hw/net/xilinx_axienet.c | 20 +++ > hw/ssi/xilinx_spips.c | 2 +- > 6 files changed, 61 insertions(+), 61 deletions(-) > > diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h > index 6a39b55a7bd..fde8a3ebda6 100644 > --- a/include/hw/ssi/xilinx_spips.h > +++ b/include/hw/ssi/xilinx_spips.h > @@ -97,7 +97,7 @@ typedef struct { > typedef struct { > XilinxQSPIPS parent_obj; > > -StreamSlave *dma; > +StreamSink *dma; > int gqspi_irqline; > > uint32_t regs[XLNX_ZYNQMP_SPIPS_R_MAX]; > diff --git a/include/hw/stream.h b/include/hw/stream.h > index ed09e83683d..8ca161991ca 100644 > --- a/include/hw/stream.h > +++ b/include/hw/stream.h > @@ -3,52 +3,52 @@ > > #include "qom/object.h" > > -/* stream slave. Used until qdev provides a generic way. */ > -#define TYPE_STREAM_SLAVE "stream-slave" > +/* stream sink. Used until qdev provides a generic way. */ > +#define TYPE_STREAM_SINK "stream-slave" > > -#define STREAM_SLAVE_CLASS(klass) \ > - OBJECT_CLASS_CHECK(StreamSlaveClass, (klass), TYPE_STREAM_SLAVE) > -#define STREAM_SLAVE_GET_CLASS(obj) \ > -OBJECT_GET_CLASS(StreamSlaveClass, (obj), TYPE_STREAM_SLAVE) > -#define STREAM_SLAVE(obj) \ > - INTERFACE_CHECK(StreamSlave, (obj), TYPE_STREAM_SLAVE) > +#define STREAM_SINK_CLASS(klass) \ > + OBJECT_CLASS_CHECK(StreamSinkClass, (klass), TYPE_STREAM_SINK) > +#define STREAM_SINK_GET_CLASS(obj) \ > +OBJECT_GET_CLASS(StreamSinkClass, (obj), TYPE_STREAM_SINK) > +#define STREAM_SINK(obj) \ > + INTERFACE_CHECK(StreamSink, (obj), TYPE_STREAM_SINK) > > -typedef struct StreamSlave StreamSlave; > +typedef struct StreamSink StreamSink; > > typedef void (*StreamCanPushNotifyFn)(void *opaque); > > -typedef struct StreamSlaveClass { > +typedef struct StreamSinkClass { > InterfaceClass parent; > /** > - * can push - determine if a stream slave is capable of accepting at > least > + * can push - determine if a stream sink is capable of accepting at least > * one byte of data. Returns false if cannot accept. If not implemented, > the > - * slave is assumed to always be capable of receiving. > - * @notify: Optional callback that the slave will call when the slave is > + * sink is assumed to always be capable of receiving. > + * @notify: Optional callback that the sink will call when the sink is > * capable of receiving again. Only called if false is returned. > * @notify_opaque: opaque data to pass to notify call. > */ > -bool (*can_push)(StreamSlave *obj, StreamCanPushNotifyFn notify, > +bool (*can_push)(StreamSink *obj, StreamCanPushNotifyFn notify, > void *notify_opaque); > /** > - * push - push data to a Stream slave. The number of bytes pushed is > - * returned. If the slave short returns, the master must wait before > trying > - * again, the slave may continue to just return 0 waiting for the vm > time to > + * push - push data to a Stream sink. The number of bytes pushed is > + * returned. If the sink short returns, the master must wait before > trying > + * again, the sink may continue to just return 0 waiting for the vm time > to > * advance. The can_push() function can be used to trap the point in time > - * where the slave is ready to receive again, otherwise polling on a QEMU > + * where the sink is ready to receive again, otherwise polling on a QEMU > * timer will work. > - * @obj: Stream slave to push to > + * @obj: Stream sink to push to > * @buf: Data to write > * @len: Maximum number of bytes to write > * @eop: End of packet flag > */ > -size_t (*push)(StreamSlave *obj, unsigned char *buf, size_t len, bool > eop); > -} StreamSlaveClass; > +size_t (*push)(StreamSink *obj, unsigned char *buf, size_t len, bool > eop); > +} StreamSinkClass; > > size_t > -stream_push(StreamSlave *sink, uint8_t *buf, size_t len, bool eop); > +stream_push(StreamSi
Re: [PATCH 3/6] hw/dma/xilinx_axidma: Rename StreamSlave as StreamSink
On 10/09/20 09:01, Philippe Mathieu-Daudé wrote: > In order to use inclusive terminology, rename 'slave stream' > as 'sink stream'. > > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/dma/xilinx_axidma.c | 26 +- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c > index cf12a852ea1..19e14a2997e 100644 > --- a/hw/dma/xilinx_axidma.c > +++ b/hw/dma/xilinx_axidma.c > @@ -46,11 +46,11 @@ > OBJECT_CHECK(XilinxAXIDMA, (obj), TYPE_XILINX_AXI_DMA) > > #define XILINX_AXI_DMA_DATA_STREAM(obj) \ > - OBJECT_CHECK(XilinxAXIDMAStreamSlave, (obj),\ > + OBJECT_CHECK(XilinxAXIDMAStreamSink, (obj),\ > TYPE_XILINX_AXI_DMA_DATA_STREAM) > > #define XILINX_AXI_DMA_CONTROL_STREAM(obj) \ > - OBJECT_CHECK(XilinxAXIDMAStreamSlave, (obj),\ > + OBJECT_CHECK(XilinxAXIDMAStreamSink, (obj),\ > TYPE_XILINX_AXI_DMA_CONTROL_STREAM) > > #define R_DMACR (0x00 / 4) > @@ -63,7 +63,7 @@ > #define CONTROL_PAYLOAD_SIZE (CONTROL_PAYLOAD_WORDS * (sizeof(uint32_t))) > > typedef struct XilinxAXIDMA XilinxAXIDMA; > -typedef struct XilinxAXIDMAStreamSlave XilinxAXIDMAStreamSlave; > +typedef struct XilinxAXIDMAStreamSink XilinxAXIDMAStreamSink; > > enum { > DMACR_RUNSTOP = 1, > @@ -118,7 +118,7 @@ struct Stream { > unsigned char txbuf[16 * 1024]; > }; > > -struct XilinxAXIDMAStreamSlave { > +struct XilinxAXIDMAStreamSink { > Object parent; > > struct XilinxAXIDMA *dma; > @@ -133,8 +133,8 @@ struct XilinxAXIDMA { > uint32_t freqhz; > StreamSink *tx_data_dev; > StreamSink *tx_control_dev; > -XilinxAXIDMAStreamSlave rx_data_dev; > -XilinxAXIDMAStreamSlave rx_control_dev; > +XilinxAXIDMAStreamSink rx_data_dev; > +XilinxAXIDMAStreamSink rx_control_dev; > > struct Stream streams[2]; > > @@ -390,7 +390,7 @@ static size_t > xilinx_axidma_control_stream_push(StreamSink *obj, unsigned char *buf, >size_t len, bool eop) > { > -XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(obj); > +XilinxAXIDMAStreamSink *cs = XILINX_AXI_DMA_CONTROL_STREAM(obj); > struct Stream *s = &cs->dma->streams[1]; > > if (len != CONTROL_PAYLOAD_SIZE) { > @@ -407,7 +407,7 @@ xilinx_axidma_data_stream_can_push(StreamSink *obj, > StreamCanPushNotifyFn notify, > void *notify_opaque) > { > -XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > +XilinxAXIDMAStreamSink *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > struct Stream *s = &ds->dma->streams[1]; > > if (!stream_running(s) || stream_idle(s)) { > @@ -423,7 +423,7 @@ static size_t > xilinx_axidma_data_stream_push(StreamSink *obj, unsigned char *buf, size_t > len, > bool eop) > { > -XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > +XilinxAXIDMAStreamSink *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > struct Stream *s = &ds->dma->streams[1]; > size_t ret; > > @@ -534,8 +534,8 @@ static const MemoryRegionOps axidma_ops = { > static void xilinx_axidma_realize(DeviceState *dev, Error **errp) > { > XilinxAXIDMA *s = XILINX_AXI_DMA(dev); > -XilinxAXIDMAStreamSlave *ds = > XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev); > -XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM( > +XilinxAXIDMAStreamSink *ds = XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev); > +XilinxAXIDMAStreamSink *cs = XILINX_AXI_DMA_CONTROL_STREAM( > > &s->rx_control_dev); > int i; > > @@ -634,7 +634,7 @@ static const TypeInfo axidma_info = { > static const TypeInfo xilinx_axidma_data_stream_info = { > .name = TYPE_XILINX_AXI_DMA_DATA_STREAM, > .parent= TYPE_OBJECT, > -.instance_size = sizeof(struct XilinxAXIDMAStreamSlave), > +.instance_size = sizeof(struct XilinxAXIDMAStreamSink), > .class_init= xilinx_axidma_stream_class_init, > .class_data= &xilinx_axidma_data_stream_class, > .interfaces = (InterfaceInfo[]) { > @@ -646,7 +646,7 @@ static const TypeInfo xilinx_axidma_data_stream_info = { > static const TypeInfo xilinx_axidma_control_stream_info = { > .name = TYPE_XILINX_AXI_DMA_CONTROL_STREAM, > .parent= TYPE_OBJECT, > -.instance_size = sizeof(struct XilinxAXIDMAStreamSlave), > +.instance_size = sizeof(struct XilinxAXIDMAStreamSink), > .class_init= xilinx_axidma_stream_class_init, > .class_data= &xilinx_axidma_control_stream_class, > .interfaces = (InterfaceInfo[]) { > Acked-by: Paolo Bonzini
Re: [PATCH 4/6] hw/net/xilinx_axienet: Rename StreamSlave as StreamSink
On 10/09/20 09:01, Philippe Mathieu-Daudé wrote: > In order to use inclusive terminology, rename 'slave stream' > as 'sink stream'. > > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/net/xilinx_axienet.c | 24 > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c > index 0c4ac727207..4e48535f373 100644 > --- a/hw/net/xilinx_axienet.c > +++ b/hw/net/xilinx_axienet.c > @@ -46,11 +46,11 @@ > OBJECT_CHECK(XilinxAXIEnet, (obj), TYPE_XILINX_AXI_ENET) > > #define XILINX_AXI_ENET_DATA_STREAM(obj) \ > - OBJECT_CHECK(XilinxAXIEnetStreamSlave, (obj),\ > + OBJECT_CHECK(XilinxAXIEnetStreamSink, (obj),\ > TYPE_XILINX_AXI_ENET_DATA_STREAM) > > #define XILINX_AXI_ENET_CONTROL_STREAM(obj) \ > - OBJECT_CHECK(XilinxAXIEnetStreamSlave, (obj),\ > + OBJECT_CHECK(XilinxAXIEnetStreamSink, (obj),\ > TYPE_XILINX_AXI_ENET_CONTROL_STREAM) > > /* Advertisement control register. */ > @@ -310,10 +310,10 @@ struct TEMAC { > void *parent; > }; > > -typedef struct XilinxAXIEnetStreamSlave XilinxAXIEnetStreamSlave; > +typedef struct XilinxAXIEnetStreamSink XilinxAXIEnetStreamSink; > typedef struct XilinxAXIEnet XilinxAXIEnet; > > -struct XilinxAXIEnetStreamSlave { > +struct XilinxAXIEnetStreamSink { > Object parent; > > struct XilinxAXIEnet *enet; > @@ -325,8 +325,8 @@ struct XilinxAXIEnet { > qemu_irq irq; > StreamSink *tx_data_dev; > StreamSink *tx_control_dev; > -XilinxAXIEnetStreamSlave rx_data_dev; > -XilinxAXIEnetStreamSlave rx_control_dev; > +XilinxAXIEnetStreamSink rx_data_dev; > +XilinxAXIEnetStreamSink rx_control_dev; > NICState *nic; > NICConf conf; > > @@ -859,7 +859,7 @@ xilinx_axienet_control_stream_push(StreamSink *obj, > uint8_t *buf, size_t len, > bool eop) > { > int i; > -XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM(obj); > +XilinxAXIEnetStreamSink *cs = XILINX_AXI_ENET_CONTROL_STREAM(obj); > XilinxAXIEnet *s = cs->enet; > > assert(eop); > @@ -880,7 +880,7 @@ static size_t > xilinx_axienet_data_stream_push(StreamSink *obj, uint8_t *buf, size_t size, > bool eop) > { > -XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(obj); > +XilinxAXIEnetStreamSink *ds = XILINX_AXI_ENET_DATA_STREAM(obj); > XilinxAXIEnet *s = ds->enet; > > /* TX enable ? */ > @@ -954,8 +954,8 @@ static NetClientInfo net_xilinx_enet_info = { > static void xilinx_enet_realize(DeviceState *dev, Error **errp) > { > XilinxAXIEnet *s = XILINX_AXI_ENET(dev); > -XilinxAXIEnetStreamSlave *ds = > XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev); > -XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM( > +XilinxAXIEnetStreamSink *ds = > XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev); > +XilinxAXIEnetStreamSink *cs = XILINX_AXI_ENET_CONTROL_STREAM( > > &s->rx_control_dev); > > object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet", > @@ -1046,7 +1046,7 @@ static const TypeInfo xilinx_enet_info = { > static const TypeInfo xilinx_enet_data_stream_info = { > .name = TYPE_XILINX_AXI_ENET_DATA_STREAM, > .parent= TYPE_OBJECT, > -.instance_size = sizeof(struct XilinxAXIEnetStreamSlave), > +.instance_size = sizeof(struct XilinxAXIEnetStreamSink), > .class_init= xilinx_enet_data_stream_class_init, > .interfaces = (InterfaceInfo[]) { > { TYPE_STREAM_SINK }, > @@ -1057,7 +1057,7 @@ static const TypeInfo xilinx_enet_data_stream_info = { > static const TypeInfo xilinx_enet_control_stream_info = { > .name = TYPE_XILINX_AXI_ENET_CONTROL_STREAM, > .parent= TYPE_OBJECT, > -.instance_size = sizeof(struct XilinxAXIEnetStreamSlave), > +.instance_size = sizeof(struct XilinxAXIEnetStreamSink), > .class_init= xilinx_enet_control_stream_class_init, > .interfaces = (InterfaceInfo[]) { > { TYPE_STREAM_SINK }, > Acked-by: Paolo Bonzini
Re: [PATCH 4/6] hw/net/xilinx_axienet: Rename StreamSlave as StreamSink
On Fri, Sep 11, 2020 at 09:28:38AM +0200, Paolo Bonzini wrote: > On 10/09/20 09:01, Philippe Mathieu-Daudé wrote: > > In order to use inclusive terminology, rename 'slave stream' > > as 'sink stream'. > > > > Signed-off-by: Philippe Mathieu-Daudé > > --- > > hw/net/xilinx_axienet.c | 24 > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c > > index 0c4ac727207..4e48535f373 100644 > > --- a/hw/net/xilinx_axienet.c > > +++ b/hw/net/xilinx_axienet.c > > @@ -46,11 +46,11 @@ > > OBJECT_CHECK(XilinxAXIEnet, (obj), TYPE_XILINX_AXI_ENET) > > > > #define XILINX_AXI_ENET_DATA_STREAM(obj) \ > > - OBJECT_CHECK(XilinxAXIEnetStreamSlave, (obj),\ > > + OBJECT_CHECK(XilinxAXIEnetStreamSink, (obj),\ > > TYPE_XILINX_AXI_ENET_DATA_STREAM) > > > > #define XILINX_AXI_ENET_CONTROL_STREAM(obj) \ > > - OBJECT_CHECK(XilinxAXIEnetStreamSlave, (obj),\ > > + OBJECT_CHECK(XilinxAXIEnetStreamSink, (obj),\ > > TYPE_XILINX_AXI_ENET_CONTROL_STREAM) > > > > /* Advertisement control register. */ > > @@ -310,10 +310,10 @@ struct TEMAC { > > void *parent; > > }; > > > > -typedef struct XilinxAXIEnetStreamSlave XilinxAXIEnetStreamSlave; > > +typedef struct XilinxAXIEnetStreamSink XilinxAXIEnetStreamSink; > > typedef struct XilinxAXIEnet XilinxAXIEnet; > > > > -struct XilinxAXIEnetStreamSlave { > > +struct XilinxAXIEnetStreamSink { > > Object parent; > > > > struct XilinxAXIEnet *enet; > > @@ -325,8 +325,8 @@ struct XilinxAXIEnet { > > qemu_irq irq; > > StreamSink *tx_data_dev; > > StreamSink *tx_control_dev; > > -XilinxAXIEnetStreamSlave rx_data_dev; > > -XilinxAXIEnetStreamSlave rx_control_dev; > > +XilinxAXIEnetStreamSink rx_data_dev; > > +XilinxAXIEnetStreamSink rx_control_dev; > > NICState *nic; > > NICConf conf; > > > > @@ -859,7 +859,7 @@ xilinx_axienet_control_stream_push(StreamSink *obj, > > uint8_t *buf, size_t len, > > bool eop) > > { > > int i; > > -XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM(obj); > > +XilinxAXIEnetStreamSink *cs = XILINX_AXI_ENET_CONTROL_STREAM(obj); > > XilinxAXIEnet *s = cs->enet; > > > > assert(eop); > > @@ -880,7 +880,7 @@ static size_t > > xilinx_axienet_data_stream_push(StreamSink *obj, uint8_t *buf, size_t size, > > bool eop) > > { > > -XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(obj); > > +XilinxAXIEnetStreamSink *ds = XILINX_AXI_ENET_DATA_STREAM(obj); > > XilinxAXIEnet *s = ds->enet; > > > > /* TX enable ? */ > > @@ -954,8 +954,8 @@ static NetClientInfo net_xilinx_enet_info = { > > static void xilinx_enet_realize(DeviceState *dev, Error **errp) > > { > > XilinxAXIEnet *s = XILINX_AXI_ENET(dev); > > -XilinxAXIEnetStreamSlave *ds = > > XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev); > > -XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM( > > +XilinxAXIEnetStreamSink *ds = > > XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev); > > +XilinxAXIEnetStreamSink *cs = XILINX_AXI_ENET_CONTROL_STREAM( > > > > &s->rx_control_dev); > > > > object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet", > > @@ -1046,7 +1046,7 @@ static const TypeInfo xilinx_enet_info = { > > static const TypeInfo xilinx_enet_data_stream_info = { > > .name = TYPE_XILINX_AXI_ENET_DATA_STREAM, > > .parent= TYPE_OBJECT, > > -.instance_size = sizeof(struct XilinxAXIEnetStreamSlave), > > +.instance_size = sizeof(struct XilinxAXIEnetStreamSink), > > .class_init= xilinx_enet_data_stream_class_init, > > .interfaces = (InterfaceInfo[]) { > > { TYPE_STREAM_SINK }, > > @@ -1057,7 +1057,7 @@ static const TypeInfo xilinx_enet_data_stream_info = { > > static const TypeInfo xilinx_enet_control_stream_info = { > > .name = TYPE_XILINX_AXI_ENET_CONTROL_STREAM, > > .parent= TYPE_OBJECT, > > -.instance_size = sizeof(struct XilinxAXIEnetStreamSlave), > > +.instance_size = sizeof(struct XilinxAXIEnetStreamSink), > > .class_init= xilinx_enet_control_stream_class_init, > > .interfaces = (InterfaceInfo[]) { > > { TYPE_STREAM_SINK }, > > > > Acked-by: Paolo Bonzini Reviewed-by: Edgar E. Iglesias
Re: [PATCH 2/6] hw/core/stream: Rename StreamSlave as StreamSink
On Fri, Sep 11, 2020 at 09:28:16AM +0200, Paolo Bonzini wrote: > On 10/09/20 09:01, Philippe Mathieu-Daudé wrote: > > In order to use inclusive terminology, rename 'slave stream' > > as 'sink stream'. > > > > Signed-off-by: Philippe Mathieu-Daudé > > From Edgar Iglesias: > > Regarding streams, our stream module can be used to model a stream > channel such as AXI stream but also other similar stream protocols. We > actually don't use the AXI stream terminology [in hw/core/stream.c]. > E.g, we use buf instead of DATA, EOP (end-of-packet) instead of LAST and > have a flow-control mechanism that doesn't refer to valid/ready. IMO, > since we're not matching specific protocol names, it would be fine to > switch to generic terms like Source and Sink. > > Therefore, > > Acked-by: Paolo Bonzini > Thanks Paolo, Yes, looks good to me! Reviewed-by: Edgar E. Iglesias > > > --- > > include/hw/ssi/xilinx_spips.h | 2 +- > > include/hw/stream.h | 46 +-- > > hw/core/stream.c | 20 +++ > > hw/dma/xilinx_axidma.c| 32 > > hw/net/xilinx_axienet.c | 20 +++ > > hw/ssi/xilinx_spips.c | 2 +- > > 6 files changed, 61 insertions(+), 61 deletions(-) > > > > diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h > > index 6a39b55a7bd..fde8a3ebda6 100644 > > --- a/include/hw/ssi/xilinx_spips.h > > +++ b/include/hw/ssi/xilinx_spips.h > > @@ -97,7 +97,7 @@ typedef struct { > > typedef struct { > > XilinxQSPIPS parent_obj; > > > > -StreamSlave *dma; > > +StreamSink *dma; > > int gqspi_irqline; > > > > uint32_t regs[XLNX_ZYNQMP_SPIPS_R_MAX]; > > diff --git a/include/hw/stream.h b/include/hw/stream.h > > index ed09e83683d..8ca161991ca 100644 > > --- a/include/hw/stream.h > > +++ b/include/hw/stream.h > > @@ -3,52 +3,52 @@ > > > > #include "qom/object.h" > > > > -/* stream slave. Used until qdev provides a generic way. */ > > -#define TYPE_STREAM_SLAVE "stream-slave" > > +/* stream sink. Used until qdev provides a generic way. */ > > +#define TYPE_STREAM_SINK "stream-slave" > > > > -#define STREAM_SLAVE_CLASS(klass) \ > > - OBJECT_CLASS_CHECK(StreamSlaveClass, (klass), TYPE_STREAM_SLAVE) > > -#define STREAM_SLAVE_GET_CLASS(obj) \ > > -OBJECT_GET_CLASS(StreamSlaveClass, (obj), TYPE_STREAM_SLAVE) > > -#define STREAM_SLAVE(obj) \ > > - INTERFACE_CHECK(StreamSlave, (obj), TYPE_STREAM_SLAVE) > > +#define STREAM_SINK_CLASS(klass) \ > > + OBJECT_CLASS_CHECK(StreamSinkClass, (klass), TYPE_STREAM_SINK) > > +#define STREAM_SINK_GET_CLASS(obj) \ > > +OBJECT_GET_CLASS(StreamSinkClass, (obj), TYPE_STREAM_SINK) > > +#define STREAM_SINK(obj) \ > > + INTERFACE_CHECK(StreamSink, (obj), TYPE_STREAM_SINK) > > > > -typedef struct StreamSlave StreamSlave; > > +typedef struct StreamSink StreamSink; > > > > typedef void (*StreamCanPushNotifyFn)(void *opaque); > > > > -typedef struct StreamSlaveClass { > > +typedef struct StreamSinkClass { > > InterfaceClass parent; > > /** > > - * can push - determine if a stream slave is capable of accepting at > > least > > + * can push - determine if a stream sink is capable of accepting at > > least > > * one byte of data. Returns false if cannot accept. If not > > implemented, the > > - * slave is assumed to always be capable of receiving. > > - * @notify: Optional callback that the slave will call when the slave > > is > > + * sink is assumed to always be capable of receiving. > > + * @notify: Optional callback that the sink will call when the sink is > > * capable of receiving again. Only called if false is returned. > > * @notify_opaque: opaque data to pass to notify call. > > */ > > -bool (*can_push)(StreamSlave *obj, StreamCanPushNotifyFn notify, > > +bool (*can_push)(StreamSink *obj, StreamCanPushNotifyFn notify, > > void *notify_opaque); > > /** > > - * push - push data to a Stream slave. The number of bytes pushed is > > - * returned. If the slave short returns, the master must wait before > > trying > > - * again, the slave may continue to just return 0 waiting for the vm > > time to > > + * push - push data to a Stream sink. The number of bytes pushed is > > + * returned. If the sink short returns, the master must wait before > > trying > > + * again, the sink may continue to just return 0 waiting for the vm > > time to > > * advance. The can_push() function can be used to trap the point in > > time > > - * where the slave is ready to receive again, otherwise polling on a > > QEMU > > + * where the sink is ready to receive again, otherwise polling on a > > QEMU > > * timer will work. > > - * @obj: Stream slave to push to > > + * @obj: Stream sink to push to > > * @buf: Data to write > > * @len: Maximum number of bytes
Re: [PATCH 3/6] hw/dma/xilinx_axidma: Rename StreamSlave as StreamSink
On Fri, Sep 11, 2020 at 09:28:34AM +0200, Paolo Bonzini wrote: > On 10/09/20 09:01, Philippe Mathieu-Daudé wrote: > > In order to use inclusive terminology, rename 'slave stream' > > as 'sink stream'. > > > > Signed-off-by: Philippe Mathieu-Daudé > > --- > > hw/dma/xilinx_axidma.c | 26 +- > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c > > index cf12a852ea1..19e14a2997e 100644 > > --- a/hw/dma/xilinx_axidma.c > > +++ b/hw/dma/xilinx_axidma.c > > @@ -46,11 +46,11 @@ > > OBJECT_CHECK(XilinxAXIDMA, (obj), TYPE_XILINX_AXI_DMA) > > > > #define XILINX_AXI_DMA_DATA_STREAM(obj) \ > > - OBJECT_CHECK(XilinxAXIDMAStreamSlave, (obj),\ > > + OBJECT_CHECK(XilinxAXIDMAStreamSink, (obj),\ > > TYPE_XILINX_AXI_DMA_DATA_STREAM) > > > > #define XILINX_AXI_DMA_CONTROL_STREAM(obj) \ > > - OBJECT_CHECK(XilinxAXIDMAStreamSlave, (obj),\ > > + OBJECT_CHECK(XilinxAXIDMAStreamSink, (obj),\ > > TYPE_XILINX_AXI_DMA_CONTROL_STREAM) > > > > #define R_DMACR (0x00 / 4) > > @@ -63,7 +63,7 @@ > > #define CONTROL_PAYLOAD_SIZE (CONTROL_PAYLOAD_WORDS * (sizeof(uint32_t))) > > > > typedef struct XilinxAXIDMA XilinxAXIDMA; > > -typedef struct XilinxAXIDMAStreamSlave XilinxAXIDMAStreamSlave; > > +typedef struct XilinxAXIDMAStreamSink XilinxAXIDMAStreamSink; > > > > enum { > > DMACR_RUNSTOP = 1, > > @@ -118,7 +118,7 @@ struct Stream { > > unsigned char txbuf[16 * 1024]; > > }; > > > > -struct XilinxAXIDMAStreamSlave { > > +struct XilinxAXIDMAStreamSink { > > Object parent; > > > > struct XilinxAXIDMA *dma; > > @@ -133,8 +133,8 @@ struct XilinxAXIDMA { > > uint32_t freqhz; > > StreamSink *tx_data_dev; > > StreamSink *tx_control_dev; > > -XilinxAXIDMAStreamSlave rx_data_dev; > > -XilinxAXIDMAStreamSlave rx_control_dev; > > +XilinxAXIDMAStreamSink rx_data_dev; > > +XilinxAXIDMAStreamSink rx_control_dev; > > > > struct Stream streams[2]; > > > > @@ -390,7 +390,7 @@ static size_t > > xilinx_axidma_control_stream_push(StreamSink *obj, unsigned char *buf, > >size_t len, bool eop) > > { > > -XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(obj); > > +XilinxAXIDMAStreamSink *cs = XILINX_AXI_DMA_CONTROL_STREAM(obj); > > struct Stream *s = &cs->dma->streams[1]; > > > > if (len != CONTROL_PAYLOAD_SIZE) { > > @@ -407,7 +407,7 @@ xilinx_axidma_data_stream_can_push(StreamSink *obj, > > StreamCanPushNotifyFn notify, > > void *notify_opaque) > > { > > -XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > > +XilinxAXIDMAStreamSink *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > > struct Stream *s = &ds->dma->streams[1]; > > > > if (!stream_running(s) || stream_idle(s)) { > > @@ -423,7 +423,7 @@ static size_t > > xilinx_axidma_data_stream_push(StreamSink *obj, unsigned char *buf, size_t > > len, > > bool eop) > > { > > -XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > > +XilinxAXIDMAStreamSink *ds = XILINX_AXI_DMA_DATA_STREAM(obj); > > struct Stream *s = &ds->dma->streams[1]; > > size_t ret; > > > > @@ -534,8 +534,8 @@ static const MemoryRegionOps axidma_ops = { > > static void xilinx_axidma_realize(DeviceState *dev, Error **errp) > > { > > XilinxAXIDMA *s = XILINX_AXI_DMA(dev); > > -XilinxAXIDMAStreamSlave *ds = > > XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev); > > -XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM( > > +XilinxAXIDMAStreamSink *ds = > > XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev); > > +XilinxAXIDMAStreamSink *cs = XILINX_AXI_DMA_CONTROL_STREAM( > > > > &s->rx_control_dev); > > int i; > > > > @@ -634,7 +634,7 @@ static const TypeInfo axidma_info = { > > static const TypeInfo xilinx_axidma_data_stream_info = { > > .name = TYPE_XILINX_AXI_DMA_DATA_STREAM, > > .parent= TYPE_OBJECT, > > -.instance_size = sizeof(struct XilinxAXIDMAStreamSlave), > > +.instance_size = sizeof(struct XilinxAXIDMAStreamSink), > > .class_init= xilinx_axidma_stream_class_init, > > .class_data= &xilinx_axidma_data_stream_class, > > .interfaces = (InterfaceInfo[]) { > > @@ -646,7 +646,7 @@ static const TypeInfo xilinx_axidma_data_stream_info = { > > static const TypeInfo xilinx_axidma_control_stream_info = { > > .name = TYPE_XILINX_AXI_DMA_CONTROL_STREAM, > > .parent= TYPE_OBJECT, > > -.instance_size = sizeof(struct XilinxAXIDMAStreamSlave), > > +.instance_size = sizeof(struct XilinxAXIDMAStreamSink), > > .class_init= xilinx_axidma_stream_class_init, > > .class_data= &xilinx_axidma_control_stream_class, > >
Re: [PATCH v5 1/7] usb/hcd-xhci: Make dma read/writes hooks pci free
On Thu, Sep 10, 2020 at 12:01:03PM +0530, Sai Pavan Boddu wrote: > This patch starts making the hcd-xhci.c pci free, as part of this > restructuring dma read/writes are handled without passing pci object. > > Signed-off-by: Sai Pavan Boddu Reviewed-by: Edgar E. Iglesias > --- > hw/usb/hcd-xhci.c | 24 +++- > hw/usb/hcd-xhci.h | 1 + > 2 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index 46a2186..254cf1e 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -495,7 +495,7 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, > dma_addr_t addr, > > assert((len % sizeof(uint32_t)) == 0); > > -pci_dma_read(PCI_DEVICE(xhci), addr, buf, len); > +dma_memory_read(xhci->as, addr, buf, len); > > for (i = 0; i < (len / sizeof(uint32_t)); i++) { > buf[i] = le32_to_cpu(buf[i]); > @@ -515,7 +515,7 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, > dma_addr_t addr, > for (i = 0; i < n; i++) { > tmp[i] = cpu_to_le32(buf[i]); > } > -pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len); > +dma_memory_write(xhci->as, addr, tmp, len); > } > > static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport) > @@ -644,7 +644,6 @@ static void xhci_die(XHCIState *xhci) > > static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v) > { > -PCIDevice *pci_dev = PCI_DEVICE(xhci); > XHCIInterrupter *intr = &xhci->intr[v]; > XHCITRB ev_trb; > dma_addr_t addr; > @@ -663,7 +662,7 @@ static void xhci_write_event(XHCIState *xhci, XHCIEvent > *event, int v) > ev_trb.status, ev_trb.control); > > addr = intr->er_start + TRB_SIZE*intr->er_ep_idx; > -pci_dma_write(pci_dev, addr, &ev_trb, TRB_SIZE); > +dma_memory_write(xhci->as, addr, &ev_trb, TRB_SIZE); > > intr->er_ep_idx++; > if (intr->er_ep_idx >= intr->er_size) { > @@ -720,12 +719,11 @@ static void xhci_ring_init(XHCIState *xhci, XHCIRing > *ring, > static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb, > dma_addr_t *addr) > { > -PCIDevice *pci_dev = PCI_DEVICE(xhci); > uint32_t link_cnt = 0; > > while (1) { > TRBType type; > -pci_dma_read(pci_dev, ring->dequeue, trb, TRB_SIZE); > +dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE); > trb->addr = ring->dequeue; > trb->ccs = ring->ccs; > le64_to_cpus(&trb->parameter); > @@ -762,7 +760,6 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing > *ring, XHCITRB *trb, > > static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring) > { > -PCIDevice *pci_dev = PCI_DEVICE(xhci); > XHCITRB trb; > int length = 0; > dma_addr_t dequeue = ring->dequeue; > @@ -773,7 +770,7 @@ static int xhci_ring_chain_length(XHCIState *xhci, const > XHCIRing *ring) > > while (1) { > TRBType type; > -pci_dma_read(pci_dev, dequeue, &trb, TRB_SIZE); > +dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE); > le64_to_cpus(&trb.parameter); > le32_to_cpus(&trb.status); > le32_to_cpus(&trb.control); > @@ -828,7 +825,7 @@ static void xhci_er_reset(XHCIState *xhci, int v) > xhci_die(xhci); > return; > } > -pci_dma_read(PCI_DEVICE(xhci), erstba, &seg, sizeof(seg)); > +dma_memory_read(xhci->as, erstba, &seg, sizeof(seg)); > le32_to_cpus(&seg.addr_low); > le32_to_cpus(&seg.addr_high); > le32_to_cpus(&seg.size); > @@ -1440,7 +1437,7 @@ static int xhci_xfer_create_sgl(XHCITransfer *xfer, int > in_xfer) > int i; > > xfer->int_req = false; > -pci_dma_sglist_init(&xfer->sgl, PCI_DEVICE(xhci), xfer->trb_count); > +qemu_sglist_init(&xfer->sgl, DEVICE(xhci), xfer->trb_count, xhci->as); > for (i = 0; i < xfer->trb_count; i++) { > XHCITRB *trb = &xfer->trbs[i]; > dma_addr_t addr; > @@ -2104,7 +2101,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, > unsigned int slotid, > assert(slotid >= 1 && slotid <= xhci->numslots); > > dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high); > -poctx = ldq_le_pci_dma(PCI_DEVICE(xhci), dcbaap + 8 * slotid); > +poctx = ldq_le_dma(xhci->as, dcbaap + 8 * slotid); > ictx = xhci_mask64(pictx); > octx = xhci_mask64(poctx); > > @@ -2442,7 +2439,7 @@ static TRBCCode xhci_get_port_bandwidth(XHCIState > *xhci, uint64_t pctx) > /* TODO: actually implement real values here */ > bw_ctx[0] = 0; > memset(&bw_ctx[1], 80, xhci->numports); /* 80% */ > -pci_dma_write(PCI_DEVICE(xhci), ctx, bw_ctx, sizeof(bw_ctx)); > +dma_memory_write(xhci->as, ctx, bw_ctx, sizeof(bw_ctx)); > > return CC_SUCCESS; > } > @@ -3434,6 +3431,7 @@ static void usb_xhci_realize(struct PCIDevice *dev, > Error **errp) > } > > usb_x
Re: PATCH: Increase System Firmware Max Size
+Markus, Dave, Phil On 09/11/20 03:45, McMillan, Erich wrote: > Hi all, > > (this is my first Qemu patch submission, please let me know if my > formatting/content needs to be fixed). > We have a need for increased firmware size, currently we are building Qemu > with the following change to test our Uefi Firmware and it works well for us. > Hope that this change can be made to open source. Thank you. > --- > Increase allowed system firmware size to 16M per comment suggesting up to 18M > is permissible. > > Signed-off-by: Erich McMillan > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index > b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca > 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -46,7 +46,7 @@ > * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in > * size. > */ > -#define FLASH_SIZE_LIMIT (8 * MiB) > +#define FLASH_SIZE_LIMIT (16 * MiB) > > #define FLASH_SECTOR_SIZE 4096 > --- > > (1) This is not a trivial change, so qemu-trivial: please ignore. (2) The comment has not been updated. (3) I'm almost certain that *if* we want to change this, it needs to be turned into a machine type (or some device model) property, for migration compatibility. (4) I feel we need much more justification for this change than just "our firmware is larger than upstream OVMF". In the upstream 4MB unified OVMF build, there's *plenty* of free room. Of course that's not to say that we're willing to *squander* that space -- whenever we include anything new in the upstream OVMF platform(s), there must be a very good reason for it --, just that, given the upstream OVMF status, the proposed pflash increase in QEMU is staggering. Considering upstream OVMF and QEMU, it should take *years* to even get close to filling the 4MB unified flash image of OVMF (hint: link-time optimization, LZMA compression), let alone to exhaust the twice as large (8MB) QEMU allowance. Unless you are committed to open source your guest firmware too (maybe as part of upstream edk2, maybe elsewhere), I seriously doubt we should accommodate this use case in *upstream* QEMU. It complicates things (minimally with regard to migration), and currently I don't see the benefit to the upstream community. Clearly, for building your firmware image, you must have minimally modified the DSC and FDF files in OVMF too, or created an entirely separate firmware platform -- DSC and FDF both. If you are using your downstream OVMF build as a testbed for your proprietary physical platform firmware, that's generally a use case that we're mildly interested in not breaking in upstream OvmfPkg. But in order to make me care for real (as an OvmfPkg co-maintainer), you'd need to upstream your OVMF platform to edk2 (we already have Xen and -- recently added -- bhyve firmware platforms under OvmfPkg, not just QEMU). You'd also have to offer long-term reviewership and testing for that platform in edk2 (like the Xen and bhyve stake-holders do). Then we could consider additional complexity in QEMU for booting that firmware platform. Thanks, Laszlo
[Bug 1895053] Re: Cannot nspawn raspbian 10 [FAILED] Failed to start Journal Service.
mhm thats somehow unfortunate since I dont know in what direction I would have to go to solve the issue :/ I tried 245.7 but without success. Do you maybe have an idea on how to investigate further? Is this some kind of distribution problem? What is the actual problem anyway? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1895053 Title: Cannot nspawn raspbian 10 [FAILED] Failed to start Journal Service. Status in QEMU: New Bug description: Hi, I'm using nspawn and asked the question @systemd-devel. They redirected me to you, guessing that nspawn calls a syscall or ioctl qemu isnt aware of and can't implement properly? They were like: "Sorry, that's not my department." ^^ Maybe you can reproduce the issue or help me investigating whats wrong or put the ball right back into their court? :D Testscript: wget https://downloads.raspberrypi.org/raspios_lite_armhf_latest -o r.zip unzip r.zip LOOP=$(losetup --show -Pf *raspios-buster-armhf-lite.img) mount ${LOOP}p2 /mnt mount ${LOOP}p1 /mnt/boot systemd-nspawn --bind /usr/bin/qemu-arm-static --boot --directory=/mnt -- systemd.log_level=debug Output: see attachment System: uname -a Linux MArch 5.8.7-arch1-1 #1 SMP PREEMPT Sat, 05 Sep 2020 12:31:32 + x86_64 GNU/Linux qemu-arm-static --version qemu-arm version 5.1.0 systemd-nspawn --version systemd 246 (246.4-1-arch) +PAM +AUDIT -SELINUX -IMA -APPARMOR +SMACK -SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 +ZSTD +SECCOMP +BLKID +ELFUTILS +KMOD +IDN2 -IDN +PCRE2 default-hierarchy=hybrid To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1895053/+subscriptions
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Ari Sundholm writes: > Hi Vladimir! > > Thank you for working on this. My comments below. > > On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote: >> It's simple to avoid error propagation in blk_log_writes_open(), we >> just need to refactor blk_log_writes_find_cur_log_sector() a bit. >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> >> --- >> block/blklogwrites.c | 23 +++ >> 1 file changed, 11 insertions(+), 12 deletions(-) >> diff --git a/block/blklogwrites.c b/block/blklogwrites.c >> index 7ef046cee9..c7da507b2d 100644 >> --- a/block/blklogwrites.c >> +++ b/block/blklogwrites.c >> @@ -96,10 +96,10 @@ static inline bool >> blk_log_writes_sector_size_valid(uint32_t sector_size) >> sector_size < (1ull << 24); >> } >> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild >> *log, >> - uint32_t sector_size, >> - uint64_t nr_entries, >> - Error **errp) >> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log, > > I'd rather not change the return type for reasons detailed below. > >> + uint32_t sector_size, >> + uint64_t nr_entries, >> + Error **errp) >> { >> uint64_t cur_sector = 1; >> uint64_t cur_idx = 0; >> @@ -112,13 +112,13 @@ static uint64_t >> blk_log_writes_find_cur_log_sector(BdrvChild *log, >> if (read_ret < 0) { >> error_setg_errno(errp, -read_ret, >>"Failed to read log entry %"PRIu64, cur_idx); >> -return (uint64_t)-1ull; >> +return read_ret; > > This is OK, provided the change in return type signedness is necessary > in the first place. > >> } >> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) { >> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry >> %"PRIu64, >> le64_to_cpu(cur_entry.flags), cur_idx); >> -return (uint64_t)-1ull; >> +return -EINVAL; > > This is OK, provided the return type signedness change is necessary, > although we already do have errp to indicate any errors. > >> } >> /* Account for the sector of the entry itself */ >> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, >> QDict *options, int flags, >> { >> BDRVBlkLogWritesState *s = bs->opaque; >> QemuOpts *opts; >> -Error *local_err = NULL; >> int ret; >> uint64_t log_sector_size; >> bool log_append; >> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, >> QDict *options, int flags, >> s->nr_entries = 0; >> if (blk_log_writes_sector_size_valid(log_sector_size)) { >> -s->cur_log_sector = >> +int64_t cur_log_sector = >> blk_log_writes_find_cur_log_sector(s->log_file, >> log_sector_size, >> -le64_to_cpu(log_sb.nr_entries), >> &local_err); >> -if (local_err) { >> -ret = -EINVAL; >> -error_propagate(errp, local_err); >> +le64_to_cpu(log_sb.nr_entries), errp); >> +if (cur_log_sector < 0) { >> +ret = cur_log_sector; > > This changes the semantics slightly. Changing the return type to int64 > may theoretically cause valid return values to be interpreted as > errors. Since we already do have a dedicated out pointer for the error > value (errp), why not use it? Error.h's big comment: * Error reporting system loosely patterned after Glib's GError. * * = Rules = [...] * - Whenever practical, also return a value that indicates success / * failure. This can make the error checking more concise, and can * avoid useless error object creation and destruction. Note that * we still have many functions returning void. We recommend * • bool-valued functions return true on success / false on failure, * • pointer-valued functions return non-null / null pointer, and * • integer-valued functions return non-negative / negative. blk_log_writes_find_cur_log_sector() does return such an error value before the patch: (uint64_t)-1. The caller does not use it to check for errors. It uses @err instead. Awkward, has to error_propagate(). Avoiding error_propagate() reduces the error handling boileplate. It also improves behavior when @errp is &error_abort: we get the abort right where the error happens instead of where we propagate it. Furthermore, caller has to make an error code (-EINVAL), because returning (uint64_t)-1 throws it away. Yes, a detailed error is stored into @err, but you can't cleanly extract the error code. Using a signed integer for returning "non-negat
Re: [PULL 06/10] configure: don't enable ppc64abi32-linux-user by default
Peter Maydell writes: > On Thu, 10 Sep 2020 at 14:15, Alex Bennée wrote: >> >> The user can still enable this explicitly but they will get a warning >> at the end of configure for their troubles. This also drops any builds >> of ppc64abi32 from our CI tests. >> >> Signed-off-by: Alex Bennée >> Reviewed-by: Philippe Mathieu-Daudé >> Message-Id: <20200909112742.25730-7-alex.ben...@linaro.org> > > I know this is in a pullreq at this point, but I just got round > to looking at it, and it has some odd logic in it so I figured > I'd give my review comments anyway. > >> +if test -z "$target_list_exclude" -a -z "$target_list"; then >> +# if the user doesn't specify anything lets skip deprecating stuff >> +target_list_exclude=ppc64abi32-linux-user > > Doesn't this have the slightly curious logic > that if we have more than one deprecated target then > configure with --target-list-exclude=something-else > will actually build more targets than configure without that > exclude option (because it will exclude the something-else > but stop excluding the deprecated targets)? I would have > expected the deprecated targets to be not compiled unless > the user explicitly enabled them. I think that would be > something more like > >deprecated_targets_list=ppc64abi32-linux-user >if test -z "$target_list"; then >target_list_exclude="$target_list_exclude,$deprecated_targets_list" >fi > > I suppose we would ideally like an "enable all including > the deprecated stuff", and that gets messy because there's > no way to do it except listing everything explicitly I think... Yeah - we could make it smoother although I think the only real users of --target-list-exclude are the CI systems and they all explicitly exclude ppc64abi32 when they do it. Do you want me to re-spin with those changes? > >> +fi >> + >> +exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g') >> +for config in $mak_wilds; do >> +target="$(basename "$config" .mak)" >> +exclude="no" >> +for excl in $exclude_list; do >> +if test "$excl" = "$target"; then >> +exclude="yes" >> +break; >> fi >> done >> -fi >> +if test "$exclude" = "no"; then >> +default_target_list="${default_target_list} $target" >> +fi >> +done >> >> # Enumerate public trace backends for --help output >> trace_backend_list=$(echo $(grep -le '^PUBLIC = True$' >> "$source_path"/scripts/tracetool/backend/*.py | sed -e >> 's/^.*\/\(.*\)\.py$/\1/')) >> @@ -7557,7 +7558,7 @@ TARGET_SYSTBL="" >> case "$target_name" in >>i386) >> mttcg="yes" >> - gdb_xml_files="i386-32bit.xml" >> +gdb_xml_files="i386-32bit.xml" >> TARGET_SYSTBL_ABI=i386 >> TARGET_SYSTBL=syscall_32.tbl >>;; > > (unrelated change ;-)) > >> @@ -7667,6 +7668,7 @@ case "$target_name" in >> TARGET_SYSTBL_ABI=common,nospu,32 >> echo "TARGET_ABI32=y" >> $config_target_mak >> gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml >> power-spe.xml power-vsx.xml" >> +deprecated_features="ppc64abi32 ${deprecated_features}" > > Maybe prefer > add_to deprecated_features ppc64abi32 > > ? > >>;; >>riscv32) >> TARGET_BASE_ARCH=riscv > > If we just made the deprecation warning be printed by > generic logic whenever a deprecated target ended up in > the enabled list then it would be easier to add other deprecated > targets to the list, as you wouldn't thae also have to find some > other part of configure like this to set a deprecated_features variable. > > (I'll send a patch to add lm32-softmmu,tilegx-linux-user,unicore32-softmmu > to the deprecated-target list later once we have the mechanism in place.) > >> @@ -8011,6 +8013,12 @@ fi >> touch ninjatool.stamp >> fi >> >> +if test -n "${deprecated_features}"; then >> +echo "Warning, deprecated features enabled." >> +echo "Please see docs/system/deprecated.rst" >> +echo " features: ${deprecated_features}" >> +fi >> + >> # Save the configure command line for later reuse. >> cat> #!/bin/sh >> -- > > thanks > -- PMM -- Alex Bennée
Re: [PATCH v5 7/7] Versal: Connect DWC3 controller with virt-versal
On Thu, Sep 10, 2020 at 12:01:09PM +0530, Sai Pavan Boddu wrote: > From: Vikram Garhwal > > Connect dwc3 controller and usb2-reg module to virt-versal. > Configure it as dual port host controller. > > Signed-off-by: Vikram Garhwal > Signed-off-by: Sai Pavan Boddu > --- > hw/arm/xlnx-versal-virt.c| 59 > > hw/arm/xlnx-versal.c | 38 > include/hw/arm/xlnx-versal.h | 14 +++ > 3 files changed, 111 insertions(+) > > diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c > index 4b3152e..398693d 100644 > --- a/hw/arm/xlnx-versal-virt.c > +++ b/hw/arm/xlnx-versal-virt.c > @@ -39,6 +39,8 @@ typedef struct VersalVirt { > uint32_t ethernet_phy[2]; > uint32_t clk_125Mhz; > uint32_t clk_25Mhz; > +uint32_t usb; > +uint32_t dwc; > } phandle; > struct arm_boot_info binfo; > > @@ -66,6 +68,8 @@ static void fdt_create(VersalVirt *s) > s->phandle.clk_25Mhz = qemu_fdt_alloc_phandle(s->fdt); > s->phandle.clk_125Mhz = qemu_fdt_alloc_phandle(s->fdt); > > +s->phandle.usb = qemu_fdt_alloc_phandle(s->fdt); > +s->phandle.dwc = qemu_fdt_alloc_phandle(s->fdt); > /* Create /chosen node for load_dtb. */ > qemu_fdt_add_subnode(s->fdt, "/chosen"); > > @@ -148,6 +152,60 @@ static void fdt_add_timer_nodes(VersalVirt *s) > compat, sizeof(compat)); > } > > +static void fdt_add_usb_xhci_nodes(VersalVirt *s) > +{ > +const char clocknames[] = "bus_clk\0ref_clk"; > +char *usb2name = g_strdup_printf("/usb@ff9d"); This string should be generated using the MM_USB2_REGS macro. > +const char dwcCompat[] = "xlnx,versal-dwc3"; You can use compat[] as we do in other places here. > +qemu_fdt_add_subnode(s->fdt, usb2name); > +qemu_fdt_setprop(s->fdt, usb2name, "compatible", > + dwcCompat, sizeof(dwcCompat)); > +qemu_fdt_setprop_sized_cells(s->fdt, usb2name, "reg", > + 2, MM_USB2_REGS, 2, 0x100); 0x100 as size looks small, you've added a macro for it, why not use it? > +qemu_fdt_setprop(s->fdt, usb2name, "clock-names", > + clocknames, sizeof(clocknames)); > +qemu_fdt_setprop_cells(s->fdt, usb2name, "clocks", > + s->phandle.clk_25Mhz, s->phandle.clk_125Mhz); > +qemu_fdt_setprop(s->fdt, usb2name, "ranges", NULL, 0); > +qemu_fdt_setprop_cell(s->fdt, usb2name, "#address-cells", 2); > +qemu_fdt_setprop_cell(s->fdt, usb2name, "#size-cells", 2); > +qemu_fdt_setprop_cell(s->fdt, usb2name, "phandle", s->phandle.usb); > +g_free(usb2name); > + > +{ > +uint64_t addr = MM_USB_XHCI_0; > +unsigned int irq = VERSAL_USB0_IRQ_0; You're only using irq once? why not just use VERSAL_USB0_IRQ_0 directly? > +const char compat[] = "snps,dwc3"; > +const char intName[] = "dwc_usb3"; I'd prefer interrupt_names[] or irq_names[]. > +uint32_t frameLen = 0x20; Can't you just directly use 0x20 when setting the prop? > + > +char *name = g_strdup_printf("/usb@ff9d/dwc3@%" PRIx64, addr); We shouldn't hard-code ff9d here. It also looks weird/wrong to have dwc3 as a subnode of usb like that. > +qemu_fdt_add_subnode(s->fdt, name); > +qemu_fdt_setprop(s->fdt, name, "compatible", > + compat, sizeof(compat)); > +qemu_fdt_setprop_sized_cells(s->fdt, name, "reg", > + 2, addr, 2, MM_USB_XHCI_SIZE_0); > +qemu_fdt_setprop(s->fdt, name, "interrupt-names", > + intName, sizeof(intName)); > +qemu_fdt_setprop_cells(s->fdt, name, "interrupts", > + GIC_FDT_IRQ_TYPE_SPI, irq, > + GIC_FDT_IRQ_FLAGS_LEVEL_HI); > +qemu_fdt_setprop_cell(s->fdt, name, > + "snps,quirk-frame-length-adjustment", > + frameLen); > +qemu_fdt_setprop_cells(s->fdt, name, "#stream-id-cells", 1); > +qemu_fdt_setprop_string(s->fdt, name, "dr_mode", "host"); > +qemu_fdt_setprop_string(s->fdt, name, "phy-names", "usb3-phy"); > +qemu_fdt_setprop(s->fdt, name, "snps,dis_u2_susphy_quirk", NULL, 0); > +qemu_fdt_setprop(s->fdt, name, "snps,dis_u3_susphy_quirk", NULL, 0); > +qemu_fdt_setprop(s->fdt, name, "snps,refclk_fladj", NULL, 0); > +qemu_fdt_setprop(s->fdt, name, "snps,mask_phy_reset", NULL, 0); > +qemu_fdt_setprop(s->fdt, name, "snps,usb3_lpm_capable", NULL, 0); > +qemu_fdt_setprop_cell(s->fdt, name, "phandle", s->phandle.dwc); > +qemu_fdt_setprop_string(s->fdt, name, "maximum-speed", "high-speed"); > +g_free(name); > +} > +} > static void fdt_add_uart_nodes(VersalVirt *s) > { > uint64_t addrs[] = { MM_UART1, MM_UART0 }; > @
Re: [PATCH 00/18] chardev: QOM cleanups
Hi On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost wrote: > Some chardev QOM cleanup patches had to be dropped from my queue > due to build erros introduced by code movements across ifdef > boundaries at char-parallel.c. This series redo the changes from > those patches, but the macro renames are now a little different: > > In this version I have decided to rename the type checking macros > from *_CHARDEV to CHARDEV_* instead of renaming tye > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the > identifiers actually match the QOM type name strings > ("chardev-*"). > Sounds reasonable to me, but it loses the matching with the structure/object type name though. - MuxChardev *d = MUX_CHARDEV(s); + MuxChardev *d = CHARDEV_MUX(s); I have a preference for the first. Unless we rename all the chardev types too... Imho, the QOM type name is mostly an internal detail, the C type name is dominant in the code. > Eduardo Habkost (18): > chardev: Move PARALLEL_CHARDEV macro to common code > chardev: Move ParallelChardev typedef to common code > chardev: Use DECLARE_INSTANCE_CHECKER macro for PARALLEL_CHARDEV > chardev: Rename MOUSE_CHARDEV to CHARDEV_MSMOUSE > chardev: Rename BAUM_CHARDEV to CHARDEV_BRAILLE > chardev: Rename FD_CHARDEV to CHARDEV_FD > chardev: Rename MUX_CHARDEV to CHARDEV_MUX > chardev: Rename PARALLEL_CHARDEV to CHARDEV_PARALLEL > chardev: Rename PTY_CHARDEV to CHARDEV_PTY > chardev: Rename RINGBUF_CHARDEV to CHARDEV_RINGBUF > chardev: Rename SOCKET_CHARDEV to CHARDEV_SOCKET > chardev: Rename SPICE_CHARDEV to CHARDEV_SPICE > chardev: Rename TESTDEV_CHARDEV to CHARDEV_TESTDEV > chardev: Rename UDP_CHARDEV to CHARDEV_UDP > chardev: Rename VC_CHARDEV to CHARDEV_VC > chardev: Rename WCTABLET_CHARDEV to CHARDEV_WCTABLET > chardev: Rename WIN_CHARDEV to CHARDEV_WIN > chardev: Rename WIN_STDIO_CHARDEV to CHARDEV_WIN_STDIO > > chardev/chardev-internal.h | 2 +- > include/chardev/char-fd.h | 2 +- > include/chardev/char-win.h | 2 +- > include/chardev/spice.h| 2 +- > chardev/baum.c | 14 > chardev/char-fd.c | 14 > chardev/char-fe.c | 4 +-- > chardev/char-mux.c | 22 ++-- > chardev/char-parallel.c| 28 > chardev/char-pipe.c| 2 +- > chardev/char-pty.c | 22 ++-- > chardev/char-ringbuf.c | 12 +++ > chardev/char-serial.c | 2 +- > chardev/char-socket.c | 68 +++--- > chardev/char-udp.c | 14 > chardev/char-win-stdio.c | 14 > chardev/char-win.c | 14 > chardev/char.c | 2 +- > chardev/msmouse.c | 12 +++ > chardev/spice.c| 16 - > chardev/testdev.c | 4 +-- > chardev/wctablet.c | 12 +++ > ui/console.c | 10 +++--- > ui/gtk.c | 8 ++--- > ui/spice-app.c | 2 +- > 25 files changed, 151 insertions(+), 153 deletions(-) > > -- > 2.26.2 > > > > -- Marc-André Lureau
Re: [PATCH v5 5/7] misc: Add versal-usb2-regs module
On Thu, Sep 10, 2020 at 12:01:07PM +0530, Sai Pavan Boddu wrote: > This module emulates control registers of versal usb2 controller, this is > added > just to make guest happy. In general this module would control the phy-reset > signal from usb controller, data coherency of the transactions, signals > the host system errors received from controller. > > Signed-off-by: Sai Pavan Boddu > Signed-off-by: Vikram Garhwal > --- > hw/misc/meson.build | 1 + > hw/misc/xlnx-versal-usb2-regs.c | 226 > > include/hw/misc/xlnx-versal-usb2-regs.h | 43 ++ > 3 files changed, 270 insertions(+) > create mode 100644 hw/misc/xlnx-versal-usb2-regs.c > create mode 100644 include/hw/misc/xlnx-versal-usb2-regs.h > > diff --git a/hw/misc/meson.build b/hw/misc/meson.build > index e1576b8..2d231d4 100644 > --- a/hw/misc/meson.build > +++ b/hw/misc/meson.build > @@ -99,3 +99,4 @@ specific_ss.add(when: 'CONFIG_MIPS_CPS', if_true: > files('mips_cmgcr.c', 'mips_cp > specific_ss.add(when: 'CONFIG_MIPS_ITU', if_true: files('mips_itu.c')) > > specific_ss.add(when: 'CONFIG_SBSA_REF', if_true: files('sbsa_ec.c')) > +specific_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: > files('xlnx-versal-usb2-regs.c')) > diff --git a/hw/misc/xlnx-versal-usb2-regs.c b/hw/misc/xlnx-versal-usb2-regs.c > new file mode 100644 > index 000..420cadd > --- /dev/null > +++ b/hw/misc/xlnx-versal-usb2-regs.c > @@ -0,0 +1,226 @@ > +/* > + * QEMU model of the XlnxUsb2Regs Register control block/Status for USB2.0 IP > + * > + * This module should control phy_reset, permanent device plugs, frame length > + * time adjust & setting of coherency paths. None of which are emulated in > + * present model. > + * > + * Copyright (c) 2020 Xilinx Inc. Vikram Garhwal > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > deal > + * in the Software without restriction, including without limitation the > rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +#include "qemu/osdep.h" > +#include "hw/sysbus.h" > +#include "hw/irq.h" > +#include "hw/register.h" > +#include "qemu/bitops.h" > +#include "qemu/log.h" > +#include "qom/object.h" > +#include "migration/vmstate.h" > +#include "hw/misc/xlnx-versal-usb2-regs.h" > + > +#ifndef XILINX_USB2_REGS_ERR_DEBUG > +#define XILINX_USB2_REGS_ERR_DEBUG 0 > +#endif > + > +#define TYPE_XILINX_USB2_REGS "xlnx.usb2_regs" I think we need to add versal to the name. Also, USB2 Regs is not a very good name (I know that's what it's internally called but). Perhaps xlnx.versal-usb2-ctrl-regs? And XlnxVersalUsb2CtrlRegs > + > +#define XILINX_USB2_REGS(obj) \ > + OBJECT_CHECK(XlnxUsb2Regs, (obj), TYPE_XILINX_USB2_REGS) > + > +REG32(BUS_FILTER, 0x30) > +FIELD(BUS_FILTER, BYPASS, 0, 4) > +REG32(PORT, 0x34) > +FIELD(PORT, HOST_SMI_BAR_WR, 4, 1) > +FIELD(PORT, HOST_SMI_PCI_CMD_REG_WR, 3, 1) > +FIELD(PORT, HOST_MSI_ENABLE, 2, 1) > +FIELD(PORT, PWR_CTRL_PRSNT, 1, 1) > +FIELD(PORT, HUB_PERM_ATTACH, 0, 1) > +REG32(JITTER_ADJUST, 0x38) > +FIELD(JITTER_ADJUST, FLADJ, 0, 6) > +REG32(BIGENDIAN, 0x40) > +FIELD(BIGENDIAN, ENDIAN_GS, 0, 1) > +REG32(COHERENCY, 0x44) > +FIELD(COHERENCY, USB_COHERENCY, 0, 1) > +REG32(XHC_BME, 0x48) > +FIELD(XHC_BME, XHC_BME, 0, 1) > +REG32(REG_CTRL, 0x60) > +FIELD(REG_CTRL, SLVERR_ENABLE, 0, 1) > +REG32(IR_STATUS, 0x64) > +FIELD(IR_STATUS, HOST_SYS_ERR, 1, 1) > +FIELD(IR_STATUS, ADDR_DEC_ERR, 0, 1) > +REG32(IR_MASK, 0x68) > +FIELD(IR_MASK, HOST_SYS_ERR, 1, 1) > +FIELD(IR_MASK, ADDR_DEC_ERR, 0, 1) > +REG32(IR_ENABLE, 0x6c) > +FIELD(IR_ENABLE, HOST_SYS_ERR, 1, 1) > +FIELD(IR_ENABLE, ADDR_DEC_ERR, 0, 1) > +REG32(IR_DISABLE, 0x70) > +FIELD(IR_DISABLE, HOST_SYS_ERR, 1, 1) > +FIELD(IR_DISABLE, ADDR_DEC_ERR, 0, 1) > +REG32(USB3, 0x78) > + > +static void ir_update_irq(XlnxUsb2Regs *s) > +{ > +bool pending = s->regs[R_IR_STATUS] & ~s->regs[R_IR_MASK]; > +qemu_set_irq(s->irq_ir, pending); > +} > + > +static void ir_status_postw(Re
Re: [PATCH 00/18] chardev: QOM cleanups
On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote: > Hi > > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost > wrote: > > > Some chardev QOM cleanup patches had to be dropped from my queue > > due to build erros introduced by code movements across ifdef > > boundaries at char-parallel.c. This series redo the changes from > > those patches, but the macro renames are now a little different: > > > > In this version I have decided to rename the type checking macros > > from *_CHARDEV to CHARDEV_* instead of renaming tye > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the > > identifiers actually match the QOM type name strings > > ("chardev-*"). > > > > Sounds reasonable to me, but it loses the matching with the > structure/object type name though. > > - MuxChardev *d = MUX_CHARDEV(s); > + MuxChardev *d = CHARDEV_MUX(s); > > I have a preference for the first. Unless we rename all the chardev types > too... I tend to think the structs should be renamed too - I've always considerd them to be backwards. > Imho, the QOM type name is mostly an internal detail, the C type name is > dominant in the code. Err it is the reverse. The QOM type name is exposed public API via QOM commands, while the C struct names are a internal private detail. 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 00/18] chardev: QOM cleanups
Hi On Fri, Sep 11, 2020 at 12:10 PM Daniel P. Berrangé wrote: > On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost > > wrote: > > > > > Some chardev QOM cleanup patches had to be dropped from my queue > > > due to build erros introduced by code movements across ifdef > > > boundaries at char-parallel.c. This series redo the changes from > > > those patches, but the macro renames are now a little different: > > > > > > In this version I have decided to rename the type checking macros > > > from *_CHARDEV to CHARDEV_* instead of renaming tye > > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the > > > identifiers actually match the QOM type name strings > > > ("chardev-*"). > > > > > > > Sounds reasonable to me, but it loses the matching with the > > structure/object type name though. > > > > - MuxChardev *d = MUX_CHARDEV(s); > > + MuxChardev *d = CHARDEV_MUX(s); > > > > I have a preference for the first. Unless we rename all the chardev types > > too... > > I tend to think the structs should be renamed too - I've always considerd > them to be backwards. > > > Imho, the QOM type name is mostly an internal detail, the C type name is > > dominant in the code. > > Err it is the reverse. The QOM type name is exposed public API via QOM > commands, while the C struct names are a internal private detail. > > Yes, but without the chardev- prefix (unless you try object-add which I don't think will work with chardev) -- Marc-André Lureau
Re: [PATCH] spapr: Handle HPT allocation failure in nested guest
On Fri, 11 Sep 2020 01:31:23 -0300 Fabiano Rosas wrote: > The nested KVM code does not yet support HPT guests. Calling the > KVM_CAP_PPC_ALLOC_HTAB ioctl currently leads to KVM setting the guest > as HPT and erroneously executing code in L1 that should only run in > hypervisor mode, leading to an exception in the L1 vcpu thread when it > enters the nested guest. > > This can be reproduced with -machine max-cpu-compat=power8 in the L2 > guest command line. > > The KVM code has since been modified to fail the ioctl when running in Well, this isn't technically true for now. The KVM patch hasn't been merged yet, but I guess it's okay to merge the QEMU patch anyway since it shouldn't break older KVMs. > a nested environment so QEMU needs to be able to handle that. This > patch provides an error message informing the user about the lack of > support for HPT in nested guests. > > Signed-off-by: Fabiano Rosas > --- > hw/ppc/spapr.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 9bce1892b5..ea2c755310 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1483,6 +1483,12 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, > int shift, > spapr_free_hpt(spapr); > > rc = kvmppc_reset_htab(shift); > + > +if (rc == -EOPNOTSUPP) { As noted on the kvm-ppc list, from a POSIX standpoint it seems that ENOTSUP would be more appropriate... but since linux only knows about EOPNOTSUPP and it has an unrelated and poorly named ENOTSUPP, I guess it is okay to keep EOPNOTSUPP for the sake of consistency. Reviewed-by: Greg Kurz > +error_setg(errp, "HPT not supported in nested guests"); > +return; > +} > + > if (rc < 0) { > /* kernel-side HPT needed, but couldn't allocate one */ > error_setg_errno(errp, errno,
Re: [PATCH] qemu-img: Support bitmap --merge into backing image
On 09.09.20 14:33, Eric Blake wrote: > If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a > bitmap from top into base, qemu-img was failing with: > > qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to > get shared "write" lock > Is another process using the image [base.qcow2]? > > The easiest fix is to not open the entire backing chain of the source > image, so that we aren't worrying about competing BDS visiting the > backing image as both a read-only backing of the source and the > writeable destination. > > Fixes: http://bugzilla.redhat.com/1877209 > Reported-by: Eyal Shenitzky > Signed-off-by: Eric Blake > --- > qemu-img.c | 3 +- > tests/qemu-iotests/291 | 12 > tests/qemu-iotests/291.out | 56 ++ > 3 files changed, 70 insertions(+), 1 deletion(-) > > diff --git a/qemu-img.c b/qemu-img.c > index eb2fc1f86243..b15098a2f9b3 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -4755,7 +4755,8 @@ static int img_bitmap(int argc, char **argv) > } > bs = blk_bs(blk); > if (src_filename) { > -src = img_open(false, src_filename, src_fmt, 0, false, false, false); > +src = img_open(false, src_filename, src_fmt, BDRV_O_NO_BACKING, > + false, false, false); Why not do the same for the destination BB? > if (!src) { > goto out; > } > diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291 > index 1e0bb76959bb..4f837b205655 100755 > --- a/tests/qemu-iotests/291 > +++ b/tests/qemu-iotests/291 [...] > @@ -107,6 +116,9 @@ $QEMU_IMG map --output=json --image-opts \ > nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG" > $QEMU_IMG map --output=json --image-opts \ > "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map > +nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG" Why not look into $TEST_IMG.base to see specifically whether the bitmap is there? (I also am quite surprised that it’s even possible to export bitmaps from backing nodes, but, well.) Max signature.asc Description: OpenPGP digital signature
Re: [PATCH 00/18] chardev: QOM cleanups
On Fri, Sep 11, 2020 at 12:19:08PM +0400, Marc-André Lureau wrote: > Hi > > On Fri, Sep 11, 2020 at 12:10 PM Daniel P. Berrangé > wrote: > > > On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote: > > > Hi > > > > > > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost > > > wrote: > > > > > > > Some chardev QOM cleanup patches had to be dropped from my queue > > > > due to build erros introduced by code movements across ifdef > > > > boundaries at char-parallel.c. This series redo the changes from > > > > those patches, but the macro renames are now a little different: > > > > > > > > In this version I have decided to rename the type checking macros > > > > from *_CHARDEV to CHARDEV_* instead of renaming tye > > > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the > > > > identifiers actually match the QOM type name strings > > > > ("chardev-*"). > > > > > > > > > > Sounds reasonable to me, but it loses the matching with the > > > structure/object type name though. > > > > > > - MuxChardev *d = MUX_CHARDEV(s); > > > + MuxChardev *d = CHARDEV_MUX(s); > > > > > > I have a preference for the first. Unless we rename all the chardev types > > > too... > > > > I tend to think the structs should be renamed too - I've always considerd > > them to be backwards. > > > > > Imho, the QOM type name is mostly an internal detail, the C type name is > > > dominant in the code. > > > > Err it is the reverse. The QOM type name is exposed public API via QOM > > commands, while the C struct names are a internal private detail. > > > > Yes, but without the chardev- prefix (unless you try object-add which I > don't think will work with chardev) Sure, that's just the way it had to be wired into the -chardev arg syntax, but from the POV of exposed QOM type information the canonical type name has the full chardev- prefix. 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 08/14] blockjob: return status from block_job_set_speed()
On Wed, 9 Sep 2020 21:59:24 +0300 Vladimir Sementsov-Ogievskiy wrote: > Better to return status together with setting errp. It allows to avoid > error propagation in the caller. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- Reviewed-by: Greg Kurz > include/block/blockjob.h | 2 +- > blockjob.c | 18 -- > 2 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index 35faa3aa26..d200f33c10 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -139,7 +139,7 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState > *bs); > * Set a rate-limiting parameter for the job; the actual meaning may > * vary depending on the job type. > */ > -void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); > +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); > > /** > * block_job_query: > diff --git a/blockjob.c b/blockjob.c > index 470facfd47..afddf7a1fb 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -254,28 +254,30 @@ static bool job_timer_pending(Job *job) > return timer_pending(&job->sleep_timer); > } > > -void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) > +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) > { > int64_t old_speed = job->speed; > > -if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp)) { > -return; > +if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { > +return false; > } > if (speed < 0) { > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed", > "a non-negative value"); > -return; > +return false; > } > > ratelimit_set_speed(&job->limit, speed, BLOCK_JOB_SLICE_TIME); > > job->speed = speed; > if (speed && speed <= old_speed) { > -return; > +return true; > } > > /* kick only if a timer is pending */ > job_enter_cond(&job->job, job_timer_pending); > + > +return true; > } > > int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) > @@ -448,12 +450,8 @@ void *block_job_create(const char *job_id, const > BlockJobDriver *driver, > > /* Only set speed when necessary to avoid NotSupported error */ > if (speed != 0) { > -Error *local_err = NULL; > - > -block_job_set_speed(job, speed, &local_err); > -if (local_err) { > +if (!block_job_set_speed(job, speed, errp)) { > job_early_fail(&job->job); > -error_propagate(errp, local_err); > return NULL; > } > }
Re: PATCH: Increase System Firmware Max Size
* Laszlo Ersek (ler...@redhat.com) wrote: > +Markus, Dave, Phil > > On 09/11/20 03:45, McMillan, Erich wrote: > > Hi all, > > > > (this is my first Qemu patch submission, please let me know if my > > formatting/content needs to be fixed). > > We have a need for increased firmware size, currently we are building Qemu > > with the following change to test our Uefi Firmware and it works well for > > us. Hope that this change can be made to open source. Thank you. > > --- > > Increase allowed system firmware size to 16M per comment suggesting up to > > 18M is permissible. > > > > Signed-off-by: Erich McMillan > > > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > > index > > b8d8ef59eb17c6ace8194fc69c3d27809becfbc0..f6f7cd744d0690cee0355fbd19ffdcdb71ea75ca > > 100644 > > --- a/hw/i386/pc_sysfw.c > > +++ b/hw/i386/pc_sysfw.c > > @@ -46,7 +46,7 @@ > > * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB > > in > > * size. > > */ > > -#define FLASH_SIZE_LIMIT (8 * MiB) > > +#define FLASH_SIZE_LIMIT (16 * MiB) > > > > #define FLASH_SECTOR_SIZE 4096 > > --- > > > > > > (1) This is not a trivial change, so qemu-trivial: please ignore. > > (2) The comment has not been updated. > > (3) I'm almost certain that *if* we want to change this, it needs to be > turned into a machine type (or some device model) property, for > migration compatibility. I'm missing what this constant exists for - why is the check there at all Is there something else that lives below this address that we have to protect? My reading of the code is that increasing that constant doesn't change the guests view at all, as long as the guest was given the same flash files - so if the guests view doesn't change, then why would we tie it to the machine type? > (4) I feel we need much more justification for this change than just > "our firmware is larger than upstream OVMF". > > In the upstream 4MB unified OVMF build, there's *plenty* of free room. > Of course that's not to say that we're willing to *squander* that space > -- whenever we include anything new in the upstream OVMF platform(s), > there must be a very good reason for it --, just that, given the > upstream OVMF status, the proposed pflash increase in QEMU is staggering. > > Considering upstream OVMF and QEMU, it should take *years* to even get > close to filling the 4MB unified flash image of OVMF (hint: link-time > optimization, LZMA compression), let alone to exhaust the twice as large > (8MB) QEMU allowance. > > Unless you are committed to open source your guest firmware too (maybe > as part of upstream edk2, maybe elsewhere), I seriously doubt we should > accommodate this use case in *upstream* QEMU. It complicates things > (minimally with regard to migration), and currently I don't see the > benefit to the upstream community. > > Clearly, for building your firmware image, you must have minimally > modified the DSC and FDF files in OVMF too, or created an entirely > separate firmware platform -- DSC and FDF both. > > If you are using your downstream OVMF build as a testbed for your > proprietary physical platform firmware, that's generally a use case that > we're mildly interested in not breaking in upstream OvmfPkg. But in > order to make me care for real (as an OvmfPkg co-maintainer), you'd need > to upstream your OVMF platform to edk2 (we already have Xen and -- > recently added -- bhyve firmware platforms under OvmfPkg, not just > QEMU). You'd also have to offer long-term reviewership and testing for > that platform in edk2 (like the Xen and bhyve stake-holders do). Then we > could consider additional complexity in QEMU for booting that firmware > platform. Our UEFI firmware is pretty sparse; it doesn't have any pretty graphics or snazzy stuff, or have to survive configuring lots of hardware; also I'm aware of other companies who are looking at putting big blobs of stuff in firmware for open uses; so I don't see a problem with changing this limit from the QEMU side of things. Dave > > Thanks, > Laszlo -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Bug 1895080] Re: pgb_reserved_va: Assertion `addr == test' failed
Have you got a static version of the test binary (or a mini rootfs with the libraries it needs)? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1895080 Title: pgb_reserved_va: Assertion `addr == test' failed Status in QEMU: New Bug description: This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- user. Firstly, compile fails: Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ undeclared here (not in a function) FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), I have to add below include to linux-user/strace.c diff --git a/linux-user/strace.c b/linux-user/strace.c index 11fea14fba..22e51d4a8a 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include Then trying qemu-riscv32 with a simple ELF, I get: linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. strace shows that: mmap(0x1000, 4294963200, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. ) = 103 The source code is in the function pgb_reserved_va (linux- user/elfload.c). I think mmap cannot guarantee that the returned pointer (test) equals to the parameter of addr. So is this a bug to assert (addr == test)? Attached configure script and test ELF file. Thanks. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions
[PATCH v5 0/7] vhost-user-blk: fix the migration issue and enhance qtests
v4 -> v5: - vhost: check queue state in the vhost_dev_set_log routine tests/qtest/vhost-user-test: prepare the tests for adding new dev class tests/qtest/vhost-user-test: add support for the vhost-user-blk device tests/qtest/vhost-user-test: add migrate_reconnect test Reviewed-by: Raphael Norwitz - Update qtest, by merging vhost-user-blk "if" case with the virtio-blk case. v3 -> v4: - vhost: recheck dev state in the vhost_migration_log routine Reviewed-by: Raphael Norwitz - vhost: check queue state in the vhost_dev_set_log routine Use "continue" instead of "break" to handle non-initialized virtqueue case. v2 -> v3: - update commit message for the "vhost: recheck dev state in the vhost_migration_log routine" commit - rename "started" field of the VhostUserBlk structure to "started_vu", so there will be no confustion with the VHOST started field - update vhost-user-test.c to always initialize nq local variable (spotted by patchew) v1 -> v2: - add comments to connected/started fields in the header file - move the "s->started" logic from the vhost_user_blk_disconnect routine to the vhost_user_blk_stop routine Reference e-mail threads: - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01509.html - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05241.html If vhost-user daemon is used as a backend for the vhost device, then we should consider a possibility of disconnect at any moment. There was a general question here: should we consider it as an error or okay state for the vhost-user devices during migration process? I think the disconnect event for the vhost-user devices should not break the migration process, because: - the device will be in the stopped state, so it will not be changed during migration - if reconnect will be made the migration log will be reinitialized as part of reconnect/init process: #0 vhost_log_global_start (listener=0x563989cf7be0) at hw/virtio/vhost.c:920 #1 0x56398603d8bc in listener_add_address_space (listener=0x563989cf7be0, as=0x563986ea4340 ) at softmmu/memory.c:2664 #2 0x56398603dd30 in memory_listener_register (listener=0x563989cf7be0, as=0x563986ea4340 ) at softmmu/memory.c:2740 #3 0x563985fd6956 in vhost_dev_init (hdev=0x563989cf7bd8, opaque=0x563989cf7e30, backend_type=VHOST_BACKEND_TYPE_USER, busyloop_timeout=0) at hw/virtio/vhost.c:1385 #4 0x563985f7d0b8 in vhost_user_blk_connect (dev=0x563989cf7990) at hw/block/vhost-user-blk.c:315 #5 0x563985f7d3f6 in vhost_user_blk_event (opaque=0x563989cf7990, event=CHR_EVENT_OPENED) at hw/block/vhost-user-blk.c:379 The first patch in the patchset fixes this issue by setting vhost device to the stopped state in the disconnect handler and check it the vhost_migration_log() routine before returning from the function. qtest framework was updated to test vhost-user-blk functionality. The vhost-user-blk/vhost-user-blk-tests/migrate_reconnect test was added to reproduce the original issue found. Dima Stepanov (7): vhost: recheck dev state in the vhost_migration_log routine vhost: check queue state in the vhost_dev_set_log routine tests/qtest/vhost-user-test: prepare the tests for adding new dev class tests/qtest/libqos/virtio-blk: add support for vhost-user-blk tests/qtest/vhost-user-test: add support for the vhost-user-blk device tests/qtest/vhost-user-test: add migrate_reconnect test tests/qtest/vhost-user-test: enable the reconnect tests hw/block/vhost-user-blk.c | 19 ++- hw/virtio/vhost.c | 39 - include/hw/virtio/vhost-user-blk.h | 10 ++ tests/qtest/libqos/virtio-blk.c| 14 +- tests/qtest/vhost-user-test.c | 290 +++-- 5 files changed, 322 insertions(+), 50 deletions(-) -- 2.7.4
[PATCH v5 6/7] tests/qtest/vhost-user-test: add migrate_reconnect test
Add new migrate_reconnect test for the vhost-user-blk device. Perform a disconnect after sending response for the VHOST_USER_SET_LOG_BASE command. Signed-off-by: Dima Stepanov Reviewed-by: Raphael Norwitz --- tests/qtest/vhost-user-test.c | 25 + 1 file changed, 25 insertions(+) diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c index a8af613..4b715d3 100644 --- a/tests/qtest/vhost-user-test.c +++ b/tests/qtest/vhost-user-test.c @@ -146,6 +146,7 @@ static VhostUserMsg m __attribute__ ((unused)); enum { TEST_FLAGS_OK, TEST_FLAGS_DISCONNECT, +TEST_FLAGS_MIGRATE_DISCONNECT, TEST_FLAGS_BAD, TEST_FLAGS_END, }; @@ -436,6 +437,15 @@ static void chr_read(void *opaque, const uint8_t *buf, int size) qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE); g_cond_broadcast(&s->data_cond); +/* + * Perform disconnect after sending a response. In this + * case the next write command on the QEMU side (for now + * it is SET_FEATURES will return -1, because of disconnect. + */ +if (s->test_flags == TEST_FLAGS_MIGRATE_DISCONNECT) { +qemu_chr_fe_disconnect(chr); +s->test_flags = TEST_FLAGS_BAD; +} break; case VHOST_USER_SET_VRING_BASE: @@ -737,6 +747,17 @@ static void *vhost_user_test_setup_memfd(GString *cmd_line, void *arg) return server; } +static void *vhost_user_test_setup_migrate_reconnect(GString *cmd_line, +void *arg) +{ +TestServer *server; + +server = vhost_user_test_setup_memfd(cmd_line, arg); +server->test_flags = TEST_FLAGS_MIGRATE_DISCONNECT; + +return server; +} + static void test_read_guest_mem(void *obj, void *arg, QGuestAllocator *alloc) { TestServer *server = arg; @@ -1150,5 +1171,9 @@ static void register_vhost_user_test(void) opts.before = vhost_user_test_setup_memfd; qos_add_test("migrate", "vhost-user-blk", test_migrate, &opts); + +opts.before = vhost_user_test_setup_migrate_reconnect; +qos_add_test("migrate_reconnect", "vhost-user-blk", + test_migrate, &opts); } libqos_init(register_vhost_user_test); -- 2.7.4
[PATCH v5 2/7] vhost: check queue state in the vhost_dev_set_log routine
If the vhost-user-blk daemon provides only one virtqueue, but device was added with several queues, then QEMU will send more VHOST-USER command than expected by daemon side. The vhost_virtqueue_start() routine handles such case by checking the return value from the virtio_queue_get_desc_addr() function call. Add the same check to the vhost_dev_set_log() routine. Signed-off-by: Dima Stepanov Reviewed-by: Raphael Norwitz --- hw/virtio/vhost.c | 12 1 file changed, 12 insertions(+) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index ffef7ab..a08b7d8 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev, static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) { int r, i, idx; +hwaddr addr; + r = vhost_dev_set_features(dev, enable_log); if (r < 0) { goto err_features; } for (i = 0; i < dev->nvqs; ++i) { idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i); +addr = virtio_queue_get_desc_addr(dev->vdev, idx); +if (!addr) { +/* + * The queue might not be ready for start. If this + * is the case there is no reason to continue the process. + * The similar logic is used by the vhost_virtqueue_start() + * routine. + */ +continue; +} r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, enable_log); if (r < 0) { -- 2.7.4
[PATCH v5 3/7] tests/qtest/vhost-user-test: prepare the tests for adding new dev class
For now only vhost-user-net device is supported by the test. Other vhost-user devices are not tested. As a first step make source code refactoring so new devices can reuse the same test routines. To make this provide a new vhost_user_ops structure with the methods to initialize device, its command line or make a proper vhost-user responses. Signed-off-by: Dima Stepanov Reviewed-by: Raphael Norwitz --- tests/qtest/vhost-user-test.c | 105 ++ 1 file changed, 76 insertions(+), 29 deletions(-) diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c index 9ee0f1e..3df5322 100644 --- a/tests/qtest/vhost-user-test.c +++ b/tests/qtest/vhost-user-test.c @@ -135,6 +135,10 @@ enum { TEST_FLAGS_END, }; +enum { +VHOST_USER_NET, +}; + typedef struct TestServer { gchar *socket_path; gchar *mig_path; @@ -154,10 +158,25 @@ typedef struct TestServer { bool test_fail; int test_flags; int queues; +struct vhost_user_ops *vu_ops; } TestServer; +struct vhost_user_ops { +/* Device types. */ +int type; +void (*append_opts)(TestServer *s, GString *cmd_line, +const char *chr_opts); + +/* VHOST-USER commands. */ +void (*set_features)(TestServer *s, CharBackend *chr, +VhostUserMsg *msg); +void (*get_protocol_features)(TestServer *s, +CharBackend *chr, VhostUserMsg *msg); +}; + static const char *init_hugepagefs(void); -static TestServer *test_server_new(const gchar *name); +static TestServer *test_server_new(const gchar *name, +struct vhost_user_ops *ops); static void test_server_free(TestServer *server); static void test_server_listen(TestServer *server); @@ -167,7 +186,7 @@ enum test_memfd { TEST_MEMFD_NO, }; -static void append_vhost_opts(TestServer *s, GString *cmd_line, +static void append_vhost_net_opts(TestServer *s, GString *cmd_line, const char *chr_opts) { g_string_append_printf(cmd_line, QEMU_CMD_CHR QEMU_CMD_NETDEV, @@ -332,25 +351,15 @@ static void chr_read(void *opaque, const uint8_t *buf, int size) break; case VHOST_USER_SET_FEATURES: -g_assert_cmpint(msg.payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES), -!=, 0ULL); -if (s->test_flags == TEST_FLAGS_DISCONNECT) { -qemu_chr_fe_disconnect(chr); -s->test_flags = TEST_FLAGS_BAD; +if (s->vu_ops->set_features) { +s->vu_ops->set_features(s, chr, &msg); } break; case VHOST_USER_GET_PROTOCOL_FEATURES: -/* send back features to qemu */ -msg.flags |= VHOST_USER_REPLY_MASK; -msg.size = sizeof(m.payload.u64); -msg.payload.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD; -msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_CROSS_ENDIAN; -if (s->queues > 1) { -msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_MQ; +if (s->vu_ops->get_protocol_features) { +s->vu_ops->get_protocol_features(s, chr, &msg); } -p = (uint8_t *) &msg; -qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size); break; case VHOST_USER_GET_VRING_BASE: @@ -467,7 +476,8 @@ static const char *init_hugepagefs(void) #endif } -static TestServer *test_server_new(const gchar *name) +static TestServer *test_server_new(const gchar *name, +struct vhost_user_ops *ops) { TestServer *server = g_new0(TestServer, 1); char template[] = "/tmp/vhost-test-XX"; @@ -495,6 +505,7 @@ static TestServer *test_server_new(const gchar *name) server->log_fd = -1; server->queues = 1; +server->vu_ops = ops; return server; } @@ -669,11 +680,11 @@ static void vhost_user_test_cleanup(void *s) static void *vhost_user_test_setup(GString *cmd_line, void *arg) { -TestServer *server = test_server_new("vhost-user-test"); +TestServer *server = test_server_new("vhost-user-test", arg); test_server_listen(server); append_mem_opts(server, cmd_line, 256, TEST_MEMFD_AUTO); -append_vhost_opts(server, cmd_line, ""); +server->vu_ops->append_opts(server, cmd_line, ""); g_test_queue_destroy(vhost_user_test_cleanup, server); @@ -682,11 +693,11 @@ static void *vhost_user_test_setup(GString *cmd_line, void *arg) static void *vhost_user_test_setup_memfd(GString *cmd_line, void *arg) { -TestServer *server = test_server_new("vhost-user-test"); +TestServer *server = test_server_new("vhost-user-test", arg); test_server_listen(server); append_mem_opts(server, cmd_line, 256, TEST_MEMFD_YES); -append_vhost_opts(server, cmd_line, ""); +server->vu_ops->append_opts(server, cmd_line, ""); g_test_queue_destroy(vhost_user_test_cleanup, server); @@ -720,7 +731,7 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) return; } -dest = test_server_new("
Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
On Wed, 9 Sep 2020 21:59:25 +0300 Vladimir Sementsov-Ogievskiy wrote: > Don't use error propagation in qcow2_get_specific_info(). For this > refactor qcow2_get_bitmap_info_list, its current interface rather ... interface is rather > weird. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- Reviewed-by: Greg Kurz > block/qcow2.h| 4 ++-- > block/qcow2-bitmap.c | 27 +-- > block/qcow2.c| 10 +++--- > 3 files changed, 18 insertions(+), 23 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 065ec3df0b..ac6a2d3e2a 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -967,8 +967,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, > BdrvCheckResult *res, >void **refcount_table, >int64_t *refcount_table_size); > bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); > -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, > -Error **errp); > +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > +Qcow2BitmapInfoList **info_list, Error > **errp); > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 8c34b2aef7..9b14c0791f 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1090,30 +1090,29 @@ static Qcow2BitmapInfoFlagsList > *get_bitmap_info_flags(uint32_t flags) > /* > * qcow2_get_bitmap_info_list() > * Returns a list of QCOW2 bitmap details. > - * In case of no bitmaps, the function returns NULL and > - * the @errp parameter is not set. > - * When bitmap information can not be obtained, the function returns > - * NULL and the @errp parameter is set. > + * On success return true with bm_list set (probably to NULL, if no bitmaps), > + * on failure return false with errp set. > */ > -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, > -Error **errp) > +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > +Qcow2BitmapInfoList **info_list, Error > **errp) > { > BDRVQcow2State *s = bs->opaque; > Qcow2BitmapList *bm_list; > Qcow2Bitmap *bm; > -Qcow2BitmapInfoList *list = NULL; > -Qcow2BitmapInfoList **plist = &list; > > if (s->nb_bitmaps == 0) { > -return NULL; > +*info_list = NULL; > +return true; > } > > bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > s->bitmap_directory_size, errp); > -if (bm_list == NULL) { > -return NULL; > +if (!bm_list) { > +return false; > } > > +*info_list = NULL; > + > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1); > Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1); > @@ -1121,13 +1120,13 @@ Qcow2BitmapInfoList > *qcow2_get_bitmap_info_list(BlockDriverState *bs, > info->name = g_strdup(bm->name); > info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS); > obj->value = info; > -*plist = obj; > -plist = &obj->next; > +*info_list = obj; > +info_list = &obj->next; > } > > bitmap_list_free(bm_list); > > -return list; > +return true; > } > > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) > diff --git a/block/qcow2.c b/block/qcow2.c > index 10175fa399..eb7c82120c 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -5056,12 +5056,10 @@ static ImageInfoSpecific > *qcow2_get_specific_info(BlockDriverState *bs, > BDRVQcow2State *s = bs->opaque; > ImageInfoSpecific *spec_info; > QCryptoBlockInfo *encrypt_info = NULL; > -Error *local_err = NULL; > > if (s->crypto != NULL) { > -encrypt_info = qcrypto_block_get_info(s->crypto, &local_err); > -if (local_err) { > -error_propagate(errp, local_err); > +encrypt_info = qcrypto_block_get_info(s->crypto, errp); > +if (!encrypt_info) { > return NULL; > } > } > @@ -5078,9 +5076,7 @@ static ImageInfoSpecific > *qcow2_get_specific_info(BlockDriverState *bs, > }; > } else if (s->qcow_version == 3) { > Qcow2BitmapInfoList *bitmaps; > -bitmaps = qcow2_get_bitmap_info_list(bs, &local_err); > -if (local_err) { > -error_propagate(errp, local_err); > +if (!qcow2_get_bitmap_info_list(bs, &bitmaps, errp)) { > qapi_free_ImageInfoSpecific(spec_info); > qapi_free_QCryptoBlockInfo(encrypt_info); > return NULL;
[PATCH v5 1/7] vhost: recheck dev state in the vhost_migration_log routine
vhost-user devices can get a disconnect in the middle of the VHOST-USER handshake on the migration start. If disconnect event happened right before sending next VHOST-USER command, then the vhost_dev_set_log() call in the vhost_migration_log() function will return error. This error will lead to the assert() and close the QEMU migration source process. For the vhost-user devices the disconnect event should not break the migration process, because: - the device will be in the stopped state, so it will not be changed during migration - if reconnect will be made the migration log will be reinitialized as part of reconnect/init process: #0 vhost_log_global_start (listener=0x563989cf7be0) at hw/virtio/vhost.c:920 #1 0x56398603d8bc in listener_add_address_space (listener=0x563989cf7be0, as=0x563986ea4340 ) at softmmu/memory.c:2664 #2 0x56398603dd30 in memory_listener_register (listener=0x563989cf7be0, as=0x563986ea4340 ) at softmmu/memory.c:2740 #3 0x563985fd6956 in vhost_dev_init (hdev=0x563989cf7bd8, opaque=0x563989cf7e30, backend_type=VHOST_BACKEND_TYPE_USER, busyloop_timeout=0) at hw/virtio/vhost.c:1385 #4 0x563985f7d0b8 in vhost_user_blk_connect (dev=0x563989cf7990) at hw/block/vhost-user-blk.c:315 #5 0x563985f7d3f6 in vhost_user_blk_event (opaque=0x563989cf7990, event=CHR_EVENT_OPENED) at hw/block/vhost-user-blk.c:379 Update the vhost-user-blk device with the internal started_vu field which will be used for initialization (vhost_user_blk_start) and clean up (vhost_user_blk_stop). This additional flag in the VhostUserBlk structure will be used to track whether the device really needs to be stopped and cleaned up on a vhost-user level. The disconnect event will set the overall VHOST device (not vhost-user) to the stopped state, so it can be used by the general vhost_migration_log routine. Such approach could be propogated to the other vhost-user devices, but better idea is just to make the same connect/disconnect code for all the vhost-user devices. This migration issue was slightly discussed earlier: - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01509.html - https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05241.html Signed-off-by: Dima Stepanov Reviewed-by: Raphael Norwitz --- hw/block/vhost-user-blk.c | 19 --- hw/virtio/vhost.c | 27 --- include/hw/virtio/vhost-user-blk.h | 10 ++ 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 39aec42..a076b1e 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -150,6 +150,7 @@ static int vhost_user_blk_start(VirtIODevice *vdev) error_report("Error starting vhost: %d", -ret); goto err_guest_notifiers; } +s->started_vu = true; /* guest_notifier_mask/pending not used yet, so just unmask * everything here. virtio-pci will do the right thing by @@ -175,6 +176,11 @@ static void vhost_user_blk_stop(VirtIODevice *vdev) VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); int ret; +if (!s->started_vu) { +return; +} +s->started_vu = false; + if (!k->set_guest_notifiers) { return; } @@ -341,9 +347,7 @@ static void vhost_user_blk_disconnect(DeviceState *dev) } s->connected = false; -if (s->dev.started) { -vhost_user_blk_stop(vdev); -} +vhost_user_blk_stop(vdev); vhost_dev_cleanup(&s->dev); } @@ -399,6 +403,15 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) NULL, NULL, false); aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque); } + +/* + * Move vhost device to the stopped state. The vhost-user device + * will be clean up and disconnected in BH. This can be useful in + * the vhost migration code. If disconnect was caught there is an + * option for the general vhost code to get the dev state without + * knowing its type (in this case vhost-user). + */ +s->dev.started = false; break; case CHR_EVENT_BREAK: case CHR_EVENT_MUX_IN: diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 1a1384e..ffef7ab 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -861,21 +861,42 @@ static int vhost_migration_log(MemoryListener *listener, bool enable) dev->log_enabled = enable; return 0; } + +r = 0; if (!enable) { r = vhost_dev_set_log(dev, false); if (r < 0) { -return r; +goto check_dev_state; } vhost_log_put(dev, false); } else { vhost_dev_log_resize(dev, vhost_get_log_size(dev)); r = vhost_dev_set_log(dev, true); if (r < 0) { -return r; +
[PATCH v5 4/7] tests/qtest/libqos/virtio-blk: add support for vhost-user-blk
Add support for the vhost-user-blk-pci device. This node can be used by the vhost-user-blk tests. Tests for the vhost-user-blk device are added in the following patches. Signed-off-by: Dima Stepanov --- tests/qtest/libqos/virtio-blk.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/qtest/libqos/virtio-blk.c b/tests/qtest/libqos/virtio-blk.c index 5da0259..c0fd9d2 100644 --- a/tests/qtest/libqos/virtio-blk.c +++ b/tests/qtest/libqos/virtio-blk.c @@ -30,7 +30,8 @@ static void *qvirtio_blk_get_driver(QVirtioBlk *v_blk, const char *interface) { -if (!g_strcmp0(interface, "virtio-blk")) { +if (!g_strcmp0(interface, "virtio-blk") || +!g_strcmp0(interface, "vhost-user-blk")) { return v_blk; } if (!g_strcmp0(interface, "virtio")) { @@ -120,6 +121,17 @@ static void virtio_blk_register_nodes(void) qos_node_produces("virtio-blk-pci", "virtio-blk"); g_free(arg); + +/* vhost-user-blk-pci */ +arg = g_strdup_printf("id=drv0,chardev=chdev0,addr=%x.%x", +PCI_SLOT, PCI_FN); +opts.extra_device_opts = arg; +add_qpci_address(&opts, &addr); +qos_node_create_driver("vhost-user-blk-pci", virtio_blk_pci_create); +qos_node_consumes("vhost-user-blk-pci", "pci-bus", &opts); +qos_node_produces("vhost-user-blk-pci", "vhost-user-blk"); + +g_free(arg); } libqos_init(virtio_blk_register_nodes); -- 2.7.4
[PATCH v5 5/7] tests/qtest/vhost-user-test: add support for the vhost-user-blk device
Add vhost_user_ops structure for the vhost-user-blk device class. Add the test_reconnect and test_migrate tests for this device. Signed-off-by: Dima Stepanov Reviewed-by: Raphael Norwitz --- tests/qtest/vhost-user-test.c | 139 +- 1 file changed, 137 insertions(+), 2 deletions(-) diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c index 3df5322..a8af613 100644 --- a/tests/qtest/vhost-user-test.c +++ b/tests/qtest/vhost-user-test.c @@ -24,6 +24,7 @@ #include "libqos/libqos.h" #include "libqos/pci-pc.h" #include "libqos/virtio-pci.h" +#include "libqos/virtio-blk.h" #include "libqos/malloc-pc.h" #include "hw/virtio/virtio-net.h" @@ -31,6 +32,7 @@ #include "standard-headers/linux/vhost_types.h" #include "standard-headers/linux/virtio_ids.h" #include "standard-headers/linux/virtio_net.h" +#include "standard-headers/linux/virtio_blk.h" #ifdef CONFIG_LINUX #include @@ -43,6 +45,7 @@ " -numa node,memdev=mem" #define QEMU_CMD_CHR" -chardev socket,id=%s,path=%s%s" #define QEMU_CMD_NETDEV " -netdev vhost-user,id=hs0,chardev=%s,vhostforce" +#define QEMU_CMD_BLKCHR " -chardev socket,id=chdev0,path=%s%s" #define HUGETLBFS_MAGIC 0x958458f6 @@ -55,6 +58,7 @@ #define VHOST_USER_PROTOCOL_F_MQ 0 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN 6 +#define VHOST_USER_PROTOCOL_F_CONFIG 9 #define VHOST_LOG_PAGE 0x1000 @@ -78,6 +82,8 @@ typedef enum VhostUserRequest { VHOST_USER_SET_PROTOCOL_FEATURES = 16, VHOST_USER_GET_QUEUE_NUM = 17, VHOST_USER_SET_VRING_ENABLE = 18, +VHOST_USER_GET_CONFIG = 24, +VHOST_USER_SET_CONFIG = 25, VHOST_USER_MAX } VhostUserRequest; @@ -99,6 +105,14 @@ typedef struct VhostUserLog { uint64_t mmap_offset; } VhostUserLog; +#define VHOST_USER_MAX_CONFIG_SIZE 256 +typedef struct VhostUserConfig { +uint32_t offset; +uint32_t size; +uint32_t flags; +uint8_t region[VHOST_USER_MAX_CONFIG_SIZE]; +} VhostUserConfig; + typedef struct VhostUserMsg { VhostUserRequest request; @@ -114,6 +128,7 @@ typedef struct VhostUserMsg { struct vhost_vring_addr addr; VhostUserMemory memory; VhostUserLog log; +VhostUserConfig config; } payload; } QEMU_PACKED VhostUserMsg; @@ -137,6 +152,7 @@ enum { enum { VHOST_USER_NET, +VHOST_USER_BLK, }; typedef struct TestServer { @@ -166,12 +182,15 @@ struct vhost_user_ops { int type; void (*append_opts)(TestServer *s, GString *cmd_line, const char *chr_opts); +void (*driver_init)(void *obj, QGuestAllocator *alloc); /* VHOST-USER commands. */ void (*set_features)(TestServer *s, CharBackend *chr, VhostUserMsg *msg); void (*get_protocol_features)(TestServer *s, CharBackend *chr, VhostUserMsg *msg); +void (*get_config)(TestServer *s, CharBackend *chr, +VhostUserMsg *msg); }; static const char *init_hugepagefs(void); @@ -194,6 +213,14 @@ static void append_vhost_net_opts(TestServer *s, GString *cmd_line, chr_opts, s->chr_name); } +static void append_vhost_blk_opts(TestServer *s, GString *cmd_line, + const char *chr_opts) +{ +g_string_append_printf(cmd_line, QEMU_CMD_BLKCHR, + s->socket_path, + chr_opts); +} + static void append_mem_opts(TestServer *server, GString *cmd_line, int size, enum test_memfd memfd) { @@ -425,6 +452,12 @@ static void chr_read(void *opaque, const uint8_t *buf, int size) qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size); break; +case VHOST_USER_GET_CONFIG: +if (s->vu_ops->get_config) { +s->vu_ops->get_config(s, chr, &msg); +} +break; + default: break; } @@ -727,6 +760,9 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) guint8 *log; guint64 size; +if (s->vu_ops->driver_init) { +s->vu_ops->driver_init(obj, alloc); +} if (!wait_for_fds(s)) { return; } @@ -796,6 +832,24 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc) g_string_free(dest_cmdline, true); } +static void vu_blk_driver_init(void *obj, QGuestAllocator *alloc) +{ +QVirtioBlk *blk_if; +QVirtioDevice *dev; +QVirtQueue *vq; +uint64_t features; + +blk_if = obj; +dev = blk_if->vdev; +features = qvirtio_get_features(dev); +qvirtio_set_features(dev, features); + +vq = qvirtqueue_setup(dev, alloc, 0); +g_assert(vq); + +qvirtio_set_driver_ok(dev); +} + static void wait_for_rings_started(TestServer *s, size_t count) { gint64 end_time; @@ -857,12 +911,21 @@ static void test_reconnect(void *obj, void *arg, QGuestAllocator *alloc) { TestServer *s = arg;
Re: [PATCH v6 3/4] util/vfio-helpers: Introduce qemu_vfio_pci_init_msix_irqs()
On Thu, Sep 10, 2020 at 05:29:25PM +0200, Philippe Mathieu-Daudé wrote: > Hi Stefan, Alex. > > On 9/10/20 12:44 PM, Stefan Hajnoczi wrote: > > On Wed, Sep 09, 2020 at 04:23:53PM +0200, Philippe Mathieu-Daudé wrote: > >> +/** > >> + * Initialize device MSIX IRQs and register event notifiers. > >> + * @irq_count: pointer to number of MSIX IRQs to initialize > >> + * @notifier: Array of @irq_count notifiers (each corresponding to a MSIX > >> IRQ) > >> + > >> + * If the number of IRQs requested exceeds the available on the device, > >> + * store the number of available IRQs in @irq_count and return -EOVERFLOW. > >> + */ > >> +int qemu_vfio_pci_init_msix_irqs(QEMUVFIOState *s, EventNotifier > >> *notifier, > >> + unsigned *irq_count, Error **errp) > >> +{ > >> +int r; > >> +size_t irq_set_size; > >> +struct vfio_irq_set *irq_set; > >> +struct vfio_irq_info irq_info = { > >> +.argsz = sizeof(irq_info), > >> +.index = VFIO_PCI_MSIX_IRQ_INDEX > >> +}; > >> + > >> +if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) { > >> +error_setg_errno(errp, errno, "Failed to get device interrupt > >> info"); > >> +return -errno; > >> +} > >> +if (irq_info.count < *irq_count) { > >> +error_setg(errp, "Not enough device interrupts available"); > >> +*irq_count = irq_info.count; > >> +return -EOVERFLOW; > >> +} > >> +if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) { > >> +error_setg(errp, "Device interrupt doesn't support eventfd"); > >> +return -EINVAL; > >> +} > >> + > >> +irq_set_size = sizeof(*irq_set) + *irq_count * sizeof(int32_t); > >> +irq_set = g_malloc0(irq_set_size); > >> + > >> +/* Get to a known IRQ state */ > >> +*irq_set = (struct vfio_irq_set) { > >> +.argsz = irq_set_size, > >> +.flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER, > >> +.index = irq_info.index, > >> +.start = 0, > >> +.count = *irq_count, > >> +}; > >> + > >> +for (unsigned i = 0; i < *irq_count; i++) { > >> +((int32_t *)&irq_set->data)[i] = > >> event_notifier_get_fd(¬ifier[i]); > >> +} > >> +r = ioctl(s->device, VFIO_DEVICE_SET_IRQS, irq_set); > >> +g_free(irq_set); > >> +if (r <= 0) { > >> +error_setg_errno(errp, errno, "Failed to setup device > >> interrupts"); > >> +return -errno; > >> +} else if (r < *irq_count) { > >> +error_setg(errp, "Not enough device interrupts available"); > >> +*irq_count = r; > >> +return -EOVERFLOW; > >> +} > > > > EOVERFLOW can occur in two cases: VFIO_DEVICE_GET_IRQ_INFO and > > VFIO_DEVICE_SET_IRQS. > > Yes. > > > > > If it happens in the second case the notifier[] array has been > > registered successfully. > > No, I don't think so: > > vfio_pci_set_msi_trigger() register the notifier only if > vfio_msi_enable() succeeded (returned 0). If vfio_msi_enable() > failed it returns the number of vectors available but do > not register the notifiers. Good, thanks! Stefan signature.asc Description: PGP signature
[PATCH 3/3] numa: remove fixup numa_state->num_nodes to MAX_NODES
current code permits only nodeids in [0..MAX_NODES) range due to nodeid check in parse_numa_node() if (nodenr >= MAX_NODES) { error_setg(errp, "Max number of NUMA nodes reached: %" so subj fixup is not reachable, drop it. Signed-off-by: Igor Mammedov --- hw/core/numa.c | 4 1 file changed, 4 deletions(-) diff --git a/hw/core/numa.c b/hw/core/numa.c index 706c1e84c6..7d5d413001 100644 --- a/hw/core/numa.c +++ b/hw/core/numa.c @@ -677,10 +677,6 @@ void numa_complete_configuration(MachineState *ms) if (ms->numa_state->num_nodes > 0) { uint64_t numa_total; -if (ms->numa_state->num_nodes > MAX_NODES) { -ms->numa_state->num_nodes = MAX_NODES; -} - numa_total = 0; for (i = 0; i < ms->numa_state->num_nodes; i++) { numa_total += numa_info[i].node_mem; -- 2.27.0
[PATCH 2/3] doc: Cleanup "'-mem-path' fallback to RAM" deprecation text
it was actually removed in 5.0, commit 68a86dc15c (numa: remove deprecated -mem-path fallback to anonymous RAM) clean up forgotten remnants in docs. Signed-off-by: Igor Mammedov --- docs/system/deprecated.rst | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 6f9441005a..f252c92901 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -104,17 +104,6 @@ error in the future. The ``-realtime mlock=on|off`` argument has been replaced by the ``-overcommit mem-lock=on|off`` argument. -``-mem-path`` fallback to RAM (since 4.1) -' - -Currently if guest RAM allocation from file pointed by ``mem-path`` -fails, QEMU falls back to allocating from RAM, which might result -in unpredictable behavior since the backing file specified by the user -is ignored. In the future, users will be responsible for making sure -the backing storage specified with ``-mem-path`` can actually provide -the guest RAM configured with ``-m`` and QEMU will fail to start up if -RAM allocation is unsuccessful. - RISC-V ``-bios`` (since 5.1) @@ -624,6 +613,16 @@ New machine versions (since 5.1) will not accept the option but it will still work with old machine types. User can check the QAPI schema to see if the legacy option is supported by looking at MachineInfo::numa-mem-supported property. +``-mem-path`` fallback to RAM (remove 5.0) +'' + +If guest RAM allocation from file pointed by ``mem-path`` failed, +QEMU was falling back to allocating from RAM, which might have resulted +in unpredictable behavior since the backing file specified by the user +as ignored. Currently, users are responsible for making sure the backing storage +specified with ``-mem-path`` can actually provide the guest RAM configured with +``-m`` and QEMU fails to start up if RAM allocation is unsuccessful. + Block devices - -- 2.27.0
[PATCH v5 7/7] tests/qtest/vhost-user-test: enable the reconnect tests
For now a QTEST_VHOST_USER_FIXME environment variable is used to separate reconnect tests for the vhost-user-net device. Looks like the reconnect functionality is pretty stable, so this separation is deprecated. Remove it and enable these tests for the default run. Signed-off-by: Dima Stepanov --- tests/qtest/vhost-user-test.c | 25 +++-- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c index 4b715d3..4b96312 100644 --- a/tests/qtest/vhost-user-test.c +++ b/tests/qtest/vhost-user-test.c @@ -1140,20 +1140,17 @@ static void register_vhost_user_test(void) "virtio-net", test_migrate, &opts); -/* keeps failing on build-system since Aug 15 2017 */ -if (getenv("QTEST_VHOST_USER_FIXME")) { -opts.before = vhost_user_test_setup_reconnect; -qos_add_test("vhost-user/reconnect", "virtio-net", - test_reconnect, &opts); - -opts.before = vhost_user_test_setup_connect_fail; -qos_add_test("vhost-user/connect-fail", "virtio-net", - test_vhost_user_started, &opts); - -opts.before = vhost_user_test_setup_flags_mismatch; -qos_add_test("vhost-user/flags-mismatch", "virtio-net", - test_vhost_user_started, &opts); -} +opts.before = vhost_user_test_setup_reconnect; +qos_add_test("vhost-user/reconnect", "virtio-net", + test_reconnect, &opts); + +opts.before = vhost_user_test_setup_connect_fail; +qos_add_test("vhost-user/connect-fail", "virtio-net", + test_vhost_user_started, &opts); + +opts.before = vhost_user_test_setup_flags_mismatch; +qos_add_test("vhost-user/flags-mismatch", "virtio-net", + test_vhost_user_started, &opts); opts.before = vhost_user_test_setup_multiqueue; opts.edge.extra_device_opts = "mq=on"; -- 2.7.4
[PATCH 0/3] numa: cleanups for 5.2
Remove deprecated default RAM splitting beween numa nodes that was deprecated since 4.1, and a couple of minor numa clean ups. Igor Mammedov (3): numa: drop support for '-numa node' (without memory specified) doc: Cleanup "'-mem-path' fallback to RAM" deprecation text numa: remove fixup numa_state->num_nodes to MAX_NODES include/hw/boards.h| 2 -- include/sysemu/numa.h | 4 --- docs/system/deprecated.rst | 44 +++- hw/core/machine.c | 1 - hw/core/numa.c | 59 -- hw/i386/pc_piix.c | 1 - hw/i386/pc_q35.c | 1 - hw/ppc/spapr.c | 1 - 8 files changed, 24 insertions(+), 89 deletions(-) -- 2.27.0
[PATCH 1/3] numa: drop support for '-numa node' (without memory specified)
it was deprecated since 4.1 commit 4bb4a2732e (numa: deprecate implict memory distribution between nodes) Users of existing VMs, wishing to preserve the same RAM distribution, should configure it explicitly using ``-numa node,memdev`` options. Current RAM distribution can be retrieved using HMP command `info numa` and if separate memory devices (pc|nv-dimm) are present use `info memory-device` and subtract device memory from output of `info numa`. Signed-off-by: Igor Mammedov --- include/hw/boards.h| 2 -- include/sysemu/numa.h | 4 --- docs/system/deprecated.rst | 23 +--- hw/core/machine.c | 1 - hw/core/numa.c | 55 -- hw/i386/pc_piix.c | 1 - hw/i386/pc_q35.c | 1 - hw/ppc/spapr.c | 1 - 8 files changed, 14 insertions(+), 74 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index bc5b82ad20..15fc1a2bac 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -208,8 +208,6 @@ struct MachineClass { strList *allowed_dynamic_sysbus_devices; bool auto_enable_numa_with_memhp; bool auto_enable_numa_with_memdev; -void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, - int nb_nodes, ram_addr_t size); bool ignore_boot_device_suffixes; bool smbus_no_migration_support; bool nvdimm_supported; diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h index ad58ee88f7..4173ef2afa 100644 --- a/include/sysemu/numa.h +++ b/include/sysemu/numa.h @@ -106,10 +106,6 @@ void parse_numa_hmat_cache(MachineState *ms, NumaHmatCacheOptions *node, void numa_complete_configuration(MachineState *ms); void query_numa_node_mem(NumaNodeMem node_mem[], MachineState *ms); extern QemuOptsList qemu_numa_opts; -void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes, - int nb_nodes, ram_addr_t size); -void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes, - int nb_nodes, ram_addr_t size); void numa_cpu_pre_plug(const struct CPUArchId *slot, DeviceState *dev, Error **errp); bool numa_uses_legacy_mem(void); diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 851dbdeb8a..6f9441005a 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -104,15 +104,6 @@ error in the future. The ``-realtime mlock=on|off`` argument has been replaced by the ``-overcommit mem-lock=on|off`` argument. -``-numa`` node (without memory specified) (since 4.1) -' - -Splitting RAM by default between NUMA nodes has the same issues as ``mem`` -parameter described above with the difference that the role of the user plays -QEMU using implicit generic or board specific splitting rule. -Use ``memdev`` with *memory-backend-ram* backend or ``mem`` (if -it's supported by used machine type) to define mapping explictly instead. - ``-mem-path`` fallback to RAM (since 4.1) ' @@ -602,6 +593,20 @@ error when ``-u`` is not used. Command line options +``-numa`` node (without memory specified) (removed 5.2) +''' + +Splitting RAM by default between NUMA nodes had the same issues as ``mem`` +parameter with the difference that the role of the user plays QEMU using +implicit generic or board specific splitting rule. +Use ``memdev`` with *memory-backend-ram* backend or ``mem`` (if +it's supported by used machine type) to define mapping explictly instead. +Users of existing VMs, wishing to preserve the same RAM distribution, should +configure it explicitly using ``-numa node,memdev`` options. Current RAM +distribution can be retrieved using HMP command ``info numa`` and if separate +memory devices (pc|nv-dimm) are present use ``info memory-device`` and subtract +device memory from output of ``info numa``. + ``-numa node,mem=``\ *size* (removed in 5.1) diff --git a/hw/core/machine.c b/hw/core/machine.c index ea26d61237..f70d388e86 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -797,7 +797,6 @@ static void machine_class_init(ObjectClass *oc, void *data) * On Linux, each node's border has to be 8MB aligned */ mc->numa_mem_align_shift = 23; -mc->numa_auto_assign_ram = numa_default_auto_assign_ram; object_class_property_add_str(oc, "kernel", machine_get_kernel, machine_set_kernel); diff --git a/hw/core/numa.c b/hw/core/numa.c index f9593ec716..706c1e84c6 100644 --- a/hw/core/numa.c +++ b/hw/core/numa.c @@ -611,42 +611,6 @@ static void complete_init_numa_distance(MachineState *ms) } } -void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes, - int nb_nodes, ram_addr_t size) -{ -int i; -uint64_
Re: [PATCH 0/2] Fix error handling in preallocate_co()
On 08.09.20 16:08, Alberto Garcia wrote: > This is a follow-up to "Fix removal of list members from > BDRVQcow2State.cluster_allocs": > >https://lists.gnu.org/archive/html/qemu-block/2020-09/msg00247.html > > However the patches themselves are independent and can be applied > separately. > > Regards, > > Berto > > Alberto Garcia (2): > qcow2: Handle QCowL2Meta on error in preallocate_co() > qcow2: Make qcow2_free_any_clusters() free only one cluster > > block/qcow2.h | 4 ++-- > block/qcow2-cluster.c | 6 +++--- > block/qcow2-refcount.c | 8 > block/qcow2.c | 40 +--- > 4 files changed, 26 insertions(+), 32 deletions(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block signature.asc Description: OpenPGP digital signature
Re: [PULL 02/10] usb-host: restrict workaround to new libusb versions
On Thu, 10 Sep 2020 14:14:56 +0100 Alex Bennée wrote: > From: Gerd Hoffmann > > Fixes build failures with old kernels (USBDEVFS_GET_SPEED missing), > on the assumtion that distros with old kernels also have old libusb. > > Reported-by: Alex Bennée > Signed-off-by: Gerd Hoffmann > Signed-off-by: Alex Bennée > Reviewed-by: Philippe Mathieu-Daudé > Message-Id: <20200902081445.3291-1-kra...@redhat.com> > Message-Id: <20200909112742.25730-3-alex.ben...@linaro.org> Can we get this merged to fix travis-ci builds, please. > > diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c > index 08604f787fd..c5d38cb09c0 100644 > --- a/hw/usb/host-libusb.c > +++ b/hw/usb/host-libusb.c > @@ -942,7 +942,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device > *dev, int hostfd) > usb_host_ep_update(s); > > libusb_speed = libusb_get_device_speed(dev); > -#ifdef CONFIG_LINUX > +#if LIBUSB_API_VERSION >= 0x01000107 && defined(CONFIG_LINUX) > if (hostfd && libusb_speed == 0) { > /* > * Workaround libusb bug: libusb_get_device_speed() does not
Re: [PATCH] qcow2: Return the original error code in qcow2_co_pwrite_zeroes()
On 09.09.20 14:37, Alberto Garcia wrote: > This function checks the current status of a (sub)cluster in order to > see if an unaligned 'write zeroes' request can be done efficiently by > simply updating the L2 metadata and without having to write actual > zeroes to disk. > > If the situation does not allow using the fast path then the function > returns -ENOTSUP and the caller falls back to writing zeroes. > > If can happen however that the aforementioned check returns an actual > error code so in this case we should pass it to the caller. > > Signed-off-by: Alberto Garcia > --- > block/qcow2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 1/2] qcow2: Report BDRV_BLOCK_ZERO more accurately in bdrv_co_block_status()
10.09.2020 20:46, Alberto Garcia wrote: If a BlockDriverState supports backing files but has none then any unallocated area reads back as zeroes. bdrv_co_block_status() is only reporting this is if want_zero is true, but this is an inexpensive test and there is no reason not to do it in all cases. Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Alberto Garcia Reviewed-by: Vladimir Sementsov-Ogievskiy --- block/io.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/io.c b/block/io.c index ad3a51ed53..1b0ae29610 100644 --- a/block/io.c +++ b/block/io.c @@ -2408,16 +2408,16 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) { ret |= BDRV_BLOCK_ALLOCATED; -} else if (want_zero && bs->drv->supports_backing) { -if (bs->backing) { +} else if (bs->drv->supports_backing) { +if (!bs->backing) { +ret |= BDRV_BLOCK_ZERO; +} else if (want_zero) { BlockDriverState *bs2 = bs->backing->bs; int64_t size2 = bdrv_getlength(bs2); if (size2 >= 0 && offset >= size2) { ret |= BDRV_BLOCK_ZERO; } -} else { -ret |= BDRV_BLOCK_ZERO; } } -- Best regards, Vladimir
[Bug 1895053] Re: Cannot nspawn raspbian 10 [FAILED] Failed to start Journal Service.
We can try to see what is failing by enabling qemu strace. To do that, I don't know a better solution than using a wrapper. Once /mnt is mounted, copy qemu-arm-static inside with a new name: sudo cp /usr/bin/qemu-arm-static /mnt/usr/bin/qemu-arm-org Then create the wrapper: cat > qemu-wrapper.c < #include #include static const char *baseargv[] = { "-strace", }; int main(int argc, char **argv, char **envp) { char *newargv[argc + sizeof(baseargv) / sizeof(char *) + 1]; int current = 0; newargv[current] = argv[0]; current++; memcpy(&newargv[current], baseargv, sizeof(baseargv)); current += sizeof(baseargv) / sizeof(char *); memcpy(&newargv[current], &argv[1], sizeof(*argv) * (argc - 1)); current += argc - 1; newargv[current] = NULL; return execve("/usr/bin/qemu-arm-org", newargv, envp); } EOF cc --static -o qemu-wrapper qemu-wrapper.c sudo cp qemu-wrapper /mnt/usr/bin/qemu-arm-static And then: systemd-nspawn --boot --directory=/mnt -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1895053 Title: Cannot nspawn raspbian 10 [FAILED] Failed to start Journal Service. Status in QEMU: New Bug description: Hi, I'm using nspawn and asked the question @systemd-devel. They redirected me to you, guessing that nspawn calls a syscall or ioctl qemu isnt aware of and can't implement properly? They were like: "Sorry, that's not my department." ^^ Maybe you can reproduce the issue or help me investigating whats wrong or put the ball right back into their court? :D Testscript: wget https://downloads.raspberrypi.org/raspios_lite_armhf_latest -o r.zip unzip r.zip LOOP=$(losetup --show -Pf *raspios-buster-armhf-lite.img) mount ${LOOP}p2 /mnt mount ${LOOP}p1 /mnt/boot systemd-nspawn --bind /usr/bin/qemu-arm-static --boot --directory=/mnt -- systemd.log_level=debug Output: see attachment System: uname -a Linux MArch 5.8.7-arch1-1 #1 SMP PREEMPT Sat, 05 Sep 2020 12:31:32 + x86_64 GNU/Linux qemu-arm-static --version qemu-arm version 5.1.0 systemd-nspawn --version systemd 246 (246.4-1-arch) +PAM +AUDIT -SELINUX -IMA -APPARMOR +SMACK -SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 +ZSTD +SECCOMP +BLKID +ELFUTILS +KMOD +IDN2 -IDN +PCRE2 default-hierarchy=hybrid To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1895053/+subscriptions
Re: [PATCH 0/2] block: remove stale runtime_opts
On 06.08.20 23:13, John Snow wrote: > > > John Snow (2): > block/rbd: remove runtime_opts > block/qcow: remove runtime opts > > block/qcow.c | 9 - > block/rbd.c | 42 -- > 2 files changed, 51 deletions(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 2/2] qcow2: Skip copy-on-write when allocating a zero cluster
10.09.2020 20:46, Alberto Garcia wrote: Since commit c8bb23cbdbe32f5c326365e0a82e1b0e68cdcd8a when a write request results in a new allocation QEMU first tries to see if the rest of the cluster outside the written area contains only zeroes. In that case, instead of doing a normal copy-on-write operation and writing explicit zero buffers to disk, the code zeroes the whole cluster efficiently using pwrite_zeroes() with BDRV_REQ_NO_FALLBACK. This improves performance very significantly but it only happens when we are writing to an area that was completely unallocated before. Zero clusters (QCOW2_CLUSTER_ZERO_*) are treated like normal clusters and are therefore slower to allocate. This happens because the code uses bdrv_is_allocated_above() rather bdrv_block_status_above(). The former is not as accurate for this purpose but it is faster. However in the case of qcow2 the underlying call does already report zero clusters just fine so there is no reason why we cannot use that information. After testing 4KB writes on an image that only contains zero clusters this patch results in almost five times more IOPS. Signed-off-by: Alberto Garcia --- include/block/block.h | 2 ++ block/io.c| 27 +++ block/qcow2.c | 35 +++ 3 files changed, 48 insertions(+), 16 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index 6e36154061..71f5678de7 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -496,6 +496,8 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, bool include_base, int64_t offset, int64_t bytes, int64_t *pnum); +int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset, + int64_t bytes); bool bdrv_is_read_only(BlockDriverState *bs); int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, diff --git a/block/io.c b/block/io.c index 1b0ae29610..5950ad87be 100644 --- a/block/io.c +++ b/block/io.c @@ -2557,6 +2557,33 @@ int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes, offset, bytes, pnum, map, file); } +/* + * Check @bs (and its backing chain) to see if the range defined + * by @offset and @bytes is known to read as zeroes. + * Return 1 if that is the case, 0 otherwise and -errno on error. + * This test is meant to be fast rather than accurate so returning 0 + * does not guarantee non-zero data. + */ +int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset, + int64_t bytes) +{ +int ret; +int64_t pnum = bytes; + +if (!bytes) { +return 1; +} + +ret = bdrv_common_block_status_above(bs, NULL, false, offset, + bytes, &pnum, NULL, NULL); + +if (ret < 0) { +return ret; +} + +return (pnum == bytes) && (ret & BDRV_BLOCK_ZERO); +} + int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, int64_t *pnum) { diff --git a/block/qcow2.c b/block/qcow2.c index da56b1a4df..68ab6562e3 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2391,26 +2391,26 @@ static bool merge_cow(uint64_t offset, unsigned bytes, return false; } -static bool is_unallocated(BlockDriverState *bs, int64_t offset, int64_t bytes) -{ -int64_t nr; -return !bytes || -(!bdrv_is_allocated_above(bs, NULL, false, offset, bytes, &nr) && - nr == bytes); -} - -static bool is_zero_cow(BlockDriverState *bs, QCowL2Meta *m) +/* + * Return 1 if the COW regions read as zeroes, 0 if not, < 0 on error. + * Note that returning 0 does not guarantee non-zero data. + */ +static int is_zero_cow(BlockDriverState *bs, QCowL2Meta *m) { /* * This check is designed for optimization shortcut so it must be * efficient. - * Instead of is_zero(), use is_unallocated() as it is faster (but not - * as accurate and can result in false negatives). + * Instead of is_zero(), use bdrv_co_is_zero_fast() as it is + * faster (but not as accurate and can result in false negatives). */ -return is_unallocated(bs, m->offset + m->cow_start.offset, - m->cow_start.nb_bytes) && - is_unallocated(bs, m->offset + m->cow_end.offset, - m->cow_end.nb_bytes); +int ret = bdrv_co_is_zero_fast(bs, m->offset + m->cow_start.offset, + m->cow_start.nb_bytes); +if (ret <= 0) { +return ret; +} + +return bdrv_co_is_zero_fast(bs, m->offset + m->cow_end.offset, +m->cow_end.nb_bytes); } static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta) @@
Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
s/startus/status On Wed, 9 Sep 2020 21:59:27 +0300 Vladimir Sementsov-Ogievskiy wrote: > It's better to return status together with setting errp. It makes > possible to avoid error propagation. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2.h| 2 +- > block/qcow2-bitmap.c | 13 ++--- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index e7e662533b..49824be5c6 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > Qcow2BitmapInfoList **info_list, Error > **errp); > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); > -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >bool release_stored, Error **errp); > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); > bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index f58923fce3..5eeff1cb1c 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1524,9 +1524,10 @@ out: > * readonly to begin with, and whether we opened directly or reopened to that > * state shouldn't matter for the state we get afterward. > */ > -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >bool release_stored, Error **errp) > { > +ERRP_GUARD(); Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes an error_prepend(errp, ...) not visible in the patch context. Anyway, Reviewed-by: Greg Kurz > BdrvDirtyBitmap *bitmap; > BDRVQcow2State *s = bs->opaque; > uint32_t new_nb_bitmaps = s->nb_bitmaps; > @@ -1546,7 +1547,7 @@ void > qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > s->bitmap_directory_size, errp); > if (bm_list == NULL) { > -return; > +return false; > } > } > > @@ -1661,7 +1662,7 @@ success: > } > > bitmap_list_free(bm_list); > -return; > +return true; > > fail: > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > @@ -1679,16 +1680,14 @@ fail: > } > > bitmap_list_free(bm_list); > +return false; > } > > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) > { > BdrvDirtyBitmap *bitmap; > -Error *local_err = NULL; > > -qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err); > -if (local_err != NULL) { > -error_propagate(errp, local_err); > +if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) { > return -EINVAL; > } >
Re: [PATCH 12/14] block/qcow2: read_cache_sizes: return status value
On Wed, 9 Sep 2020 21:59:28 +0300 Vladimir Sementsov-Ogievskiy wrote: > It's better to return status together with setting errp. It allows to > reduce error propagation. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- Reviewed-by: Greg Kurz > block/qcow2.c | 19 +-- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index c2cd9434cc..31dd28d19e 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -869,7 +869,7 @@ static void qcow2_attach_aio_context(BlockDriverState *bs, > cache_clean_timer_init(bs, new_context); > } > > -static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, > +static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, > uint64_t *l2_cache_size, > uint64_t *l2_cache_entry_size, > uint64_t *refcount_cache_size, Error **errp) > @@ -907,16 +907,16 @@ static void read_cache_sizes(BlockDriverState *bs, > QemuOpts *opts, > error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " > QCOW2_OPT_L2_CACHE_SIZE > " and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be > set " > "at the same time"); > -return; > +return false; > } else if (l2_cache_size_set && > (l2_cache_max_setting > combined_cache_size)) { > error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed " > QCOW2_OPT_CACHE_SIZE); > -return; > +return false; > } else if (*refcount_cache_size > combined_cache_size) { > error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed " > QCOW2_OPT_CACHE_SIZE); > -return; > +return false; > } > > if (l2_cache_size_set) { > @@ -955,8 +955,10 @@ static void read_cache_sizes(BlockDriverState *bs, > QemuOpts *opts, > error_setg(errp, "L2 cache entry size must be a power of two " > "between %d and the cluster size (%d)", > 1 << MIN_CLUSTER_BITS, s->cluster_size); > -return; > +return false; > } > + > +return true; > } > > typedef struct Qcow2ReopenState { > @@ -983,7 +985,6 @@ static int qcow2_update_options_prepare(BlockDriverState > *bs, > int i; > const char *encryptfmt; > QDict *encryptopts = NULL; > -Error *local_err = NULL; > int ret; > > qdict_extract_subqdict(options, &encryptopts, "encrypt."); > @@ -996,10 +997,8 @@ static int qcow2_update_options_prepare(BlockDriverState > *bs, > } > > /* get L2 table/refcount block cache size from command line options */ > -read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size, > - &refcount_cache_size, &local_err); > -if (local_err) { > -error_propagate(errp, local_err); > +if (!read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size, > + &refcount_cache_size, errp)) { > ret = -EINVAL; > goto fail; > }
Re: [Bug 1895080] Re: pgb_reserved_va: Assertion `addr == test' failed
Have you got a static version of the test binary (or a mini rootfs with > the libraries it needs)? > If the problem occurs, it does not reach the stage of the dependent libraries of the test ELF. Anyway, I've attached the static test binary as hello.static.elf. Thanks > -- > You received this bug notification because you are subscribed to the bug > report. > https://bugs.launchpad.net/bugs/1895080 > > Title: > pgb_reserved_va: Assertion `addr == test' failed > > Status in QEMU: > New > > Bug description: > This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head > (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- > user. > > Firstly, compile fails: > Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o > ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ > undeclared here (not in a function) >FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), > > I have to add below include to linux-user/strace.c > diff --git a/linux-user/strace.c b/linux-user/strace.c > index 11fea14fba..22e51d4a8a 100644 > --- a/linux-user/strace.c > +++ b/linux-user/strace.c > @@ -7,6 +7,7 @@ >#include >#include >#include > +#include >#include >#include >#include > > Then trying qemu-riscv32 with a simple ELF, I get: > linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' > failed. > > strace shows that: > mmap(0x1000, 4294963200, PROT_NONE, > MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 > write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: > ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == > test' failed. > ) = 103 > > The source code is in the function pgb_reserved_va (linux- > user/elfload.c). I think mmap cannot guarantee that the returned > pointer (test) equals to the parameter of addr. So is this a bug to > assert (addr == test)? > > Attached configure script and test ELF file. > > Thanks. > > To manage notifications about this bug go to: > https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions > ** Attachment added: "hello.static.elf" https://bugs.launchpad.net/bugs/1895080/+attachment/5409715/+files/hello.static.elf -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1895080 Title: pgb_reserved_va: Assertion `addr == test' failed Status in QEMU: New Bug description: This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- user. Firstly, compile fails: Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ undeclared here (not in a function) FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), I have to add below include to linux-user/strace.c diff --git a/linux-user/strace.c b/linux-user/strace.c index 11fea14fba..22e51d4a8a 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include Then trying qemu-riscv32 with a simple ELF, I get: linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. strace shows that: mmap(0x1000, 4294963200, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. ) = 103 The source code is in the function pgb_reserved_va (linux- user/elfload.c). I think mmap cannot guarantee that the returned pointer (test) equals to the parameter of addr. So is this a bug to assert (addr == test)? Attached configure script and test ELF file. Thanks. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions
Re: [Bug 1895080] [NEW] pgb_reserved_va: Assertion `addr == test' failed
Laurent Vivier writes: > Le 10/09/2020 à 07:21, Launchpad Bug Tracker a écrit : >> This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head >> (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- >> user. > > I tried to build qemu-5.1 on CentOS-7.5.1 but as python 3.5 is not > available, I gave up. > >> Firstly, compile fails: >> Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o >> ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ >> undeclared here (not in a function) >> FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), >> >> I have to add below include to linux-user/strace.c >> diff --git a/linux-user/strace.c b/linux-user/strace.c >> index 11fea14fba..22e51d4a8a 100644 >> --- a/linux-user/strace.c >> +++ b/linux-user/strace.c >> @@ -7,6 +7,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include > > In fact, fallocate(2) says fcntl.h must be included. > And qemu/osdep.h includes it. > So you should not have this problem. > >> >> Then trying qemu-riscv32 with a simple ELF, I get: >> linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. >> >> strace shows that: >> mmap(0x1000, 4294963200, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, >> -1, 0) = 0x1 >> write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: >> ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == >> test' failed. >> ) = 103 >> >> The source code is in the function pgb_reserved_va (linux- >> user/elfload.c). I think mmap cannot guarantee that the returned pointer >> (test) equals to the parameter of addr. So is this a bug to assert (addr >> == test)? > > I think Alex Bennée knows better this code than I do, so cc'ing him. It should be able to do so because the earlier code (pgb_static) checks for a hole the size of reserved_va in the host memory map. This should be fairly easy for 32 bit guests given the amount of spare address space you have on a 64 bit system. I'm assuming CentOS 7.5 actually has a definition for MAP_FIXED_NOREPLACE which should ensure we get what we asked for - otherwise we are in the position of hoping the kernel honours what we asked for. > > Thnaks > Laurent -- Alex Bennée -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1895080 Title: pgb_reserved_va: Assertion `addr == test' failed Status in QEMU: New Bug description: This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- user. Firstly, compile fails: Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ undeclared here (not in a function) FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), I have to add below include to linux-user/strace.c diff --git a/linux-user/strace.c b/linux-user/strace.c index 11fea14fba..22e51d4a8a 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include Then trying qemu-riscv32 with a simple ELF, I get: linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. strace shows that: mmap(0x1000, 4294963200, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. ) = 103 The source code is in the function pgb_reserved_va (linux- user/elfload.c). I think mmap cannot guarantee that the returned pointer (test) equals to the parameter of addr. So is this a bug to assert (addr == test)? Attached configure script and test ELF file. Thanks. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions
Re: [Bug 1895080] [NEW] pgb_reserved_va: Assertion `addr == test' failed
Alex Bennée writes: > Laurent Vivier writes: > >>> Then trying qemu-riscv32 with a simple ELF, I get: >>> linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. >>> >>> strace shows that: >>> mmap(0x1000, 4294963200, PROT_NONE, >>> MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 >>> write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: >>> ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == >>> test' failed. >>> ) = 103 >>> >>> The source code is in the function pgb_reserved_va (linux- >>> user/elfload.c). I think mmap cannot guarantee that the returned pointer >>> (test) equals to the parameter of addr. So is this a bug to assert (addr >>> == test)? >> > I'm assuming CentOS 7.5 actually has a definition for > MAP_FIXED_NOREPLACE which should ensure we get what we asked for - > otherwise we are in the position of hoping the kernel honours what we > asked for. Doh re-reading I see it's not set in the strace output. Maybe we should promote the assert case to the failure leg so we have: if (addr == MAP_FAILED || addr != test) { error_report(...) } so we at least fail with a user friendly error rather than an abort? -- Alex Bennée -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1895080 Title: pgb_reserved_va: Assertion `addr == test' failed Status in QEMU: New Bug description: This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- user. Firstly, compile fails: Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ undeclared here (not in a function) FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), I have to add below include to linux-user/strace.c diff --git a/linux-user/strace.c b/linux-user/strace.c index 11fea14fba..22e51d4a8a 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include Then trying qemu-riscv32 with a simple ELF, I get: linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. strace shows that: mmap(0x1000, 4294963200, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. ) = 103 The source code is in the function pgb_reserved_va (linux- user/elfload.c). I think mmap cannot guarantee that the returned pointer (test) equals to the parameter of addr. So is this a bug to assert (addr == test)? Attached configure script and test ELF file. Thanks. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions
Re: [PATCH v3 2/2] qcow2: Skip copy-on-write when allocating a zero cluster
On Fri 11 Sep 2020 11:34:37 AM CEST, Vladimir Sementsov-Ogievskiy wrote: >> -if (!is_zero_cow(bs, m)) { >> +ret = is_zero_cow(bs, m); >> +if (ret < 0) { >> +return ret; > > It's a common practice to treat block-status errors as "unknown" > status and not error-out immediately: > > - really, it's not critical, we can continue assuming non-zero > - if there are real problems with IO, we'll most probably fail on > real read or write operation, and report its status, which seems > better for user than block-status error But what's the problem exactly, does this complicate the code too much? :-? > So, I'd keep existing logic in handle_alloc_space(). And, if you agree > and resend, probably good to split this patch into two, one for > block.h/io.c and one for qcow2.c (still, I'm OK with it as one patch). Sure, I can split the patch if I have to resend it. Berto
Re: [PATCH] trace/simple: Enable tracing on startup only if the user specifies a trace option
On Sun, Aug 16, 2020 at 12:46:10PM -0500, dubo...@gmail.com wrote: > From: Josh DuBois > > Tracing can be enabled at the command line or via the > monitor. Command-line trace options are recorded during > trace_opt_parse(), but tracing is not enabled until the various > front-ends later call trace_init_file(). If the user passes a trace > option on the command-line, remember that and enable tracing during > trace_init_file(). Otherwise, trace_init_file() should record the > trace file specified by the frontend and avoid enabling traces > until the user requests them via the monitor. > > This fixes 1b7157be3a8c4300fc8044d40f4b2e64a152a1b4 and also > db25d56c014aa1a96319c663e0a60346a223b31e, by allowing the user > to enable traces on the command line and also avoiding > unwanted trace- files when the user has not asked for them. > > Fixes: 1b7157be3a8c4300fc8044d40f4b2e64a152a1b4 > Signed-off-by: Josh DuBois > --- > trace/control.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) Thanks, applied to my tracing-next tree: https://github.com/stefanha/qemu/commits/tracing-next Stefan signature.asc Description: PGP signature
Re: device compatibility interface for live migration with assigned devices
On Fri, 11 Sep 2020 08:56:00 +0800 Yan Zhao wrote: > On Thu, Sep 10, 2020 at 12:02:44PM -0600, Alex Williamson wrote: > > On Thu, 10 Sep 2020 13:50:11 +0100 > > Sean Mooney wrote: > > > > > On Thu, 2020-09-10 at 14:38 +0200, Cornelia Huck wrote: > > > > On Wed, 9 Sep 2020 10:13:09 +0800 > > > > Yan Zhao wrote: > > > > > > > > > > > still, I'd like to put it more explicitly to make ensure it's not > > > > > > > missed: > > > > > > > the reason we want to specify compatible_type as a trait and check > > > > > > > whether target compatible_type is the superset of source > > > > > > > compatible_type is for the consideration of backward > > > > > > > compatibility. > > > > > > > e.g. > > > > > > > an old generation device may have a mdev type xxx-v4-yyy, while a > > > > > > > newer > > > > > > > generation device may be of mdev type xxx-v5-yyy. > > > > > > > with the compatible_type traits, the old generation device is > > > > > > > still > > > > > > > able to be regarded as compatible to newer generation device even > > > > > > > their > > > > > > > mdev types are not equal. > > > > > > > > > > > > If you want to support migration from v4 to v5, can't the > > > > > > (presumably > > > > > > newer) driver that supports v5 simply register the v4 type as well, > > > > > > so > > > > > > that the mdev can be created as v4? (Just like QEMU versioned > > > > > > machine > > > > > > types work.) > > > > > > > > > > yes, it should work in some conditions. > > > > > but it may not be that good in some cases when v5 and v4 in the name > > > > > string > > > > > of mdev type identify hardware generation (e.g. v4 for gen8, and v5 > > > > > for > > > > > gen9) > > > > > > > > > > e.g. > > > > > (1). when src mdev type is v4 and target mdev type is v5 as > > > > > software does not support it initially, and v4 and v5 identify > > > > > hardware > > > > > differences. > > > > > > > > My first hunch here is: Don't introduce types that may be compatible > > > > later. Either make them compatible, or make them distinct by design, > > > > and possibly add a different, compatible type later. > > > > > > > > > then after software upgrade, v5 is now compatible to v4, should the > > > > > software now downgrade mdev type from v5 to v4? > > > > > not sure if moving hardware generation info into a separate attribute > > > > > from mdev type name is better. e.g. remove v4, v5 in mdev type, while > > > > > use > > > > > compatible_pci_ids to identify compatibility. > > > > > > > > If the generations are compatible, don't mention it in the mdev type. > > > > If they aren't, use distinct types, so that management software doesn't > > > > have to guess. At least that would be my naive approach here. [*] > > > yep that is what i would prefer to see too. > > > > > > > > > > > > > > (2) name string of mdev type is composed by "driver_name + type_name". > > > > > in some devices, e.g. qat, different generations of devices are > > > > > binding to > > > > > drivers of different names, e.g. "qat-v4", "qat-v5". > > > > > then though type_name is equal, mdev type is not equal. e.g. > > > > > "qat-v4-type1", "qat-v5-type1". > > > > > > > > I guess that shows a shortcoming of that "driver_name + type_name" > > > > approach? Or maybe I'm just confused. > > > yes i really dont like haveing the version in the mdev-type name > > > i would stongly perfger just qat-type-1 wehere qat is just there as a way > > > of namespacing. > > > although symmetric-cryto, asymmetric-cryto and compression woudl be a > > > better name then type-1, type-2, type-3 if > > > that is what they would end up mapping too. e.g. qat-compression or > > > qat-aes is a much better name then type-1 > > > higher layers of software are unlikely to parse the mdev names but as a > > > human looking at them its much eaiser to > > > understand if the names are meaningful. the qat prefix i think is > > > important however to make sure that your mdev-types > > > dont colide with other vendeors mdev types. so i woudl encurage all > > > vendors to prefix there mdev types with etiher the > > > device name or the vendor. > > > > +1 to all this, the mdev type is meant to indicate a software > > compatible interface, if different hardware versions can be software > > compatible, then don't make the job of finding a compatible device > > harder. The full type is a combination of the vendor driver name plus > > the vendor provided type name specifically in order to provide a type > > namespace per vendor driver. That's done at the mdev core level. > > Thanks, > > hi Alex, > got it. so do you suggest that vendors use consistent driver name over > generations of devices? > for qat, they create different modules for each generation. This > practice is not good if they want to support migration between devices > of different generations, right? Even if they create different modules, I'd assume that they have
RE: device compatibility interface for live migration with assigned devices
> From: Cornelia Huck > Sent: Friday, September 11, 2020 6:08 PM > > On Fri, 11 Sep 2020 08:56:00 +0800 > Yan Zhao wrote: > > > On Thu, Sep 10, 2020 at 12:02:44PM -0600, Alex Williamson wrote: > > > On Thu, 10 Sep 2020 13:50:11 +0100 > > > Sean Mooney wrote: > > > > > > > On Thu, 2020-09-10 at 14:38 +0200, Cornelia Huck wrote: > > > > > On Wed, 9 Sep 2020 10:13:09 +0800 > > > > > Yan Zhao wrote: > > > > > > > > > > > > > still, I'd like to put it more explicitly to make ensure it's > > > > > > > > not > missed: > > > > > > > > the reason we want to specify compatible_type as a trait and > check > > > > > > > > whether target compatible_type is the superset of source > > > > > > > > compatible_type is for the consideration of backward > compatibility. > > > > > > > > e.g. > > > > > > > > an old generation device may have a mdev type xxx-v4-yyy, > while a newer > > > > > > > > generation device may be of mdev type xxx-v5-yyy. > > > > > > > > with the compatible_type traits, the old generation device is > > > > > > > > still > > > > > > > > able to be regarded as compatible to newer generation device > even their > > > > > > > > mdev types are not equal. > > > > > > > > > > > > > > If you want to support migration from v4 to v5, can't the > (presumably > > > > > > > newer) driver that supports v5 simply register the v4 type as > > > > > > > well, > so > > > > > > > that the mdev can be created as v4? (Just like QEMU versioned > machine > > > > > > > types work.) > > > > > > > > > > > > yes, it should work in some conditions. > > > > > > but it may not be that good in some cases when v5 and v4 in the > name string > > > > > > of mdev type identify hardware generation (e.g. v4 for gen8, and v5 > for > > > > > > gen9) > > > > > > > > > > > > e.g. > > > > > > (1). when src mdev type is v4 and target mdev type is v5 as > > > > > > software does not support it initially, and v4 and v5 identify > hardware > > > > > > differences. > > > > > > > > > > My first hunch here is: Don't introduce types that may be compatible > > > > > later. Either make them compatible, or make them distinct by design, > > > > > and possibly add a different, compatible type later. > > > > > > > > > > > then after software upgrade, v5 is now compatible to v4, should the > > > > > > software now downgrade mdev type from v5 to v4? > > > > > > not sure if moving hardware generation info into a separate > attribute > > > > > > from mdev type name is better. e.g. remove v4, v5 in mdev type, > while use > > > > > > compatible_pci_ids to identify compatibility. > > > > > > > > > > If the generations are compatible, don't mention it in the mdev type. > > > > > If they aren't, use distinct types, so that management software > doesn't > > > > > have to guess. At least that would be my naive approach here. > > [*] > > > > > yep that is what i would prefer to see too. > > > > > > > > > > > > > > > > > (2) name string of mdev type is composed by "driver_name + > type_name". > > > > > > in some devices, e.g. qat, different generations of devices are > binding to > > > > > > drivers of different names, e.g. "qat-v4", "qat-v5". > > > > > > then though type_name is equal, mdev type is not equal. e.g. > > > > > > "qat-v4-type1", "qat-v5-type1". > > > > > > > > > > I guess that shows a shortcoming of that "driver_name + type_name" > > > > > approach? Or maybe I'm just confused. > > > > yes i really dont like haveing the version in the mdev-type name > > > > i would stongly perfger just qat-type-1 wehere qat is just there as a > > > > way > of namespacing. > > > > although symmetric-cryto, asymmetric-cryto and compression woudl be > a better name then type-1, type-2, type-3 if > > > > that is what they would end up mapping too. e.g. qat-compression or > qat-aes is a much better name then type-1 > > > > higher layers of software are unlikely to parse the mdev names but as a > human looking at them its much eaiser to > > > > understand if the names are meaningful. the qat prefix i think is > important however to make sure that your mdev-types > > > > dont colide with other vendeors mdev types. so i woudl encurage all > vendors to prefix there mdev types with etiher the > > > > device name or the vendor. > > > > > > +1 to all this, the mdev type is meant to indicate a software > > > compatible interface, if different hardware versions can be software > > > compatible, then don't make the job of finding a compatible device > > > harder. The full type is a combination of the vendor driver name plus > > > the vendor provided type name specifically in order to provide a type > > > namespace per vendor driver. That's done at the mdev core level. > > > Thanks, > > > > hi Alex, > > got it. so do you suggest that vendors use consistent driver name over > > generations of devices? > > for qat, they create different modules for each generation. This > > practice is not good if they want to support migration between devices > > of different generations, right?
Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
11.09.2020 12:38, Greg Kurz wrote: s/startus/status On Wed, 9 Sep 2020 21:59:27 +0300 Vladimir Sementsov-Ogievskiy wrote: It's better to return status together with setting errp. It makes possible to avoid error propagation. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/qcow2.h| 2 +- block/qcow2-bitmap.c | 13 ++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index e7e662533b..49824be5c6 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, Qcow2BitmapInfoList **info_list, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index f58923fce3..5eeff1cb1c 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1524,9 +1524,10 @@ out: * readonly to begin with, and whether we opened directly or reopened to that * state shouldn't matter for the state we get afterward. */ -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored, Error **errp) { +ERRP_GUARD(); Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes an error_prepend(errp, ...) not visible in the patch context. Ah yes. Actually this is occasional thing I didn't want to include into this patch (and int this part I). So we can just drop it and leave for part II or part III, or add a note into commit message Anyway, Reviewed-by: Greg Kurz Thanks a lot for reviewing :) Hmm.. With this series I understand the following: 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because: - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series - reviewing is the hardest part of the process So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations. 2. So, the process turns into following steps: - apply scripts/coccinelle/errp-guard.cocci - look through patches and do obvious refactorings (like this series) - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple) 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:) The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers). So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements.. BdrvDirtyBitmap *bitmap; BDRVQcow2State *s = bs->opaque; uint32_t new_nb_bitmaps = s->nb_bitmaps; @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, errp); if (bm_list == NULL) { -return; +return false; } } @@ -1661,7 +1662,7 @@ success: } bitmap_list_free(bm_list); -return; +return true; fail: QSIMPLEQ_FOREACH(bm, bm_list, entry) { @@ -1679,16 +1680,14 @@ fail: } bitmap_list_free(bm_list); +return false; } int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) { BdrvDirtyBitmap *bitmap; -Error *local_err = NULL; -qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err); -if (local_err != NULL) { -error_propagate(errp, local_err); +if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) { return -EINVAL; } -- Best regards, Vladimir
Re: [PATCH 0/8] virtiofsd: Announce submounts to the guest
On Wed, Sep 09, 2020 at 08:40:20PM +0200, Max Reitz wrote: > We want to (be able to) announce the host mount structure of the shared > directory to the guest so it can replicate that structure. This ensures > that whenever the combination of st_dev and st_ino is unique on the > host, it will be unique in the guest as well. Great, thank you for solving the long-standing inode collision problem! Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v5 2/8] s390/sclp: rework sclp boundary checks
On Thu, 10 Sep 2020 19:45:01 +0200 Thomas Huth wrote: > On 10/09/2020 11.36, Collin Walling wrote: > > Rework the SCLP boundary check to account for different SCLP commands > > (eventually) allowing different boundary sizes. > > > > Signed-off-by: Collin Walling > > Acked-by: Janosch Frank > > Reviewed-by: Cornelia Huck > > --- > > hw/s390x/sclp.c | 19 ++- > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > > index 28b973de8f..69a8724dc7 100644 > > --- a/hw/s390x/sclp.c > > +++ b/hw/s390x/sclp.c > > @@ -49,6 +49,18 @@ static inline bool sclp_command_code_valid(uint32_t code) > > return false; > > } > > > > +static bool sccb_verify_boundary(uint64_t sccb_addr, uint16_t len) > > Maybe it would be good to add a comment in front of the function to say > that len must be big endian? What about renaming it to sccb_h_len or so? That would make it more clear that the parameter is not just some random length. > > Thomas > > > +{ > > +uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(len) - 1; > > +uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE; > > + > > +if (sccb_max_addr < sccb_boundary) { > > +return true; > > +} > > + > > +return false; > > +} > > + > > static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int > > *count) > > { > > uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 }; > > @@ -229,6 +241,11 @@ int sclp_service_call_protected(CPUS390XState *env, > > uint64_t sccb, > > goto out_write; > > } > > > > +if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { ...name inspired by the 'h' in here. > > +work_sccb.h.response_code = > > cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > > +goto out_write; > > +} > > + > > sclp_c->execute(sclp, &work_sccb, code); > > out_write: > > s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > > @@ -274,7 +291,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t > > sccb, uint32_t code) > > goto out_write; > > } > > > > -if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + > > PAGE_SIZE)) { > > +if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { > > work_sccb.h.response_code = > > cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > > goto out_write; > > } > > >
Re: [Bug 1895080] [NEW] pgb_reserved_va: Assertion `addr == test' failed
No, it's not set by CentOS-7.5. Does it mean that we just cannot run the ELF in such a case? I've tried many times, the assert always fails. Maybe, we can blame CentOS-7.5. BTW: with the option "-p 65536", the case runs successfully. On Fri, Sep 11, 2020 at 5:50 PM Alex Bennée <1895...@bugs.launchpad.net> wrote: > Alex Bennée writes: > > > Laurent Vivier writes: > > > > >>> Then trying qemu-riscv32 with a simple ELF, I get: > >>> linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' > failed. > >>> > >>> strace shows that: > >>> mmap(0x1000, 4294963200, PROT_NONE, > MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 > >>> write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: > ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == > test' failed. > >>> ) = 103 > >>> > >>> The source code is in the function pgb_reserved_va (linux- > >>> user/elfload.c). I think mmap cannot guarantee that the returned > pointer > >>> (test) equals to the parameter of addr. So is this a bug to assert > (addr > >>> == test)? > >> > > I'm assuming CentOS 7.5 actually has a definition for > > MAP_FIXED_NOREPLACE which should ensure we get what we asked for - > > otherwise we are in the position of hoping the kernel honours what we > > asked for. > > Doh re-reading I see it's not set in the strace output. Maybe we should > promote the assert case to the failure leg so we have: > > if (addr == MAP_FAILED || addr != test) { > error_report(...) > } > > so we at least fail with a user friendly error rather than an abort? > > -- > Alex Bennée > > -- > You received this bug notification because you are subscribed to the bug > report. > https://bugs.launchpad.net/bugs/1895080 > > Title: > pgb_reserved_va: Assertion `addr == test' failed > > Status in QEMU: > New > > Bug description: > This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head > (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- > user. > > Firstly, compile fails: > Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o > ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ > undeclared here (not in a function) >FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), > > I have to add below include to linux-user/strace.c > diff --git a/linux-user/strace.c b/linux-user/strace.c > index 11fea14fba..22e51d4a8a 100644 > --- a/linux-user/strace.c > +++ b/linux-user/strace.c > @@ -7,6 +7,7 @@ >#include >#include >#include > +#include >#include >#include >#include > > Then trying qemu-riscv32 with a simple ELF, I get: > linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' > failed. > > strace shows that: > mmap(0x1000, 4294963200, PROT_NONE, > MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 > write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: > ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == > test' failed. > ) = 103 > > The source code is in the function pgb_reserved_va (linux- > user/elfload.c). I think mmap cannot guarantee that the returned > pointer (test) equals to the parameter of addr. So is this a bug to > assert (addr == test)? > > Attached configure script and test ELF file. > > Thanks. > > To manage notifications about this bug go to: > https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions > -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1895080 Title: pgb_reserved_va: Assertion `addr == test' failed Status in QEMU: New Bug description: This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- user. Firstly, compile fails: Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ undeclared here (not in a function) FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), I have to add below include to linux-user/strace.c diff --git a/linux-user/strace.c b/linux-user/strace.c index 11fea14fba..22e51d4a8a 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include Then trying qemu-riscv32 with a simple ELF, I get: linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. strace shows that: mmap(0x1000, 4294963200, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' failed. ) = 103 The source code is in the function pgb_reserved_va (linux- user/elfload.c). I think mmap cannot guarantee that the returned poin
Re: [PATCH] PoC: Rust binding for QAPI (qemu-ga only, for now)
On Thu, Sep 10, 2020 at 7:49 PM wrote: > The usage is relatively simple: > > - from_qemu_none(ptr: *const sys::P) -> T > Return a Rust type T for a const ffi pointer P. > > - from_qemu_full(ptr: *mut sys::P) -> T > Return a Rust type T for a ffi pointer P, taking ownership. > > - T::to_qemu_none() -> Stash > Returns a borrowed ffi pointer P (using a Stash to destroy "glue" > storage data, if any). > > - T::to_qemu_full() -> P > Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) I know these come from glib-rs, but still the names are awful. :) What about: - an unsafe variant of From/Into for from_qemu_full: trait UnsafeFrom { unsafe fn unsafe_from(_: T) -> Self; } trait UnsafeInto { unsafe fn unsafe_into(self) -> T; } impl UnsafeInto for T where U: UnsafeFrom { unsafe fn unsafe_into(self) -> U { U::unsafe_from(self) } } Example: impl UnsafeFrom<*mut c_char> for String { unsafe fn unsafe_from(ptr: *mut c_char) -> Self { let res = Self::new_from_foreign(ptr); libc::free(ptr as *mut c_void); res } } - likewise, a generic IntoRaw trait for to_qemu_full: trait IntoRaw { fn into_raw(self) -> *mut T; } Example: impl IntoRaw for String { fn into_raw(self) -> *mut c_char { unsafe { libc::strndup(self.as_ptr() as *const c_char, self.len() as size_t) } } } - and a simpler/nicer version of Stash, from_qemu_none and to_qemu_none like this: pub struct BorrowedPointer<'a, P, T: 'a> { pub native: *const P, pub storage: T, _marker: PhantomData<&'a T>, } impl<'a, P: Copy, T: 'a> BorrowedPointer<'a, P, T> { fn new(native: *const P, storage: T) -> Self { BorrowedPointer { native, storage, _marker: PhantomData } } fn as_ptr(&self) -> *const P { self.native } } trait ForeignConvertible<'a> { type Native: Copy; type Storage: 'a; unsafe fn new_from_foreign(p: *const Self::Native) -> Self; fn as_foreign(&'a self) -> BorrowedPointer<'a, Self::Native, Self::Storage>; } Implemented like this: impl ForeignConvertible<'_> for String { type Native = c_char; type Storage = CString; unsafe fn new_from_foreign(p: *const c_char) -> Self { let cstr = CStr::from_ptr(p); String::from_utf8_lossy(cstr.to_bytes()).into_owned() } fn as_foreign(&self) -> BorrowedPointer { let tmp = CString::new(&self[..]).unwrap(); BorrowedPointer::new(tmp.as_ptr(), tmp) } } and possibly: impl<'a, P: Copy, T: 'a> BorrowedMutPointer<'a, P, T> { fn new(native: *mut P, storage: T) -> Self { BorrowedMutPointer { native, storage, _marker: PhantomData } } fn as_ptr(&self) -> *const P { self.native } fn as_mut_ptr(&mut self) -> *mut P { self.native } } trait ForeignMutConvertible<'a>: ForeignConvertible<'a> { fn as_foreign_mut(&self) -> BorrowedMutPointer; } I placed the source code for the above at https://github.com/bonzini/rust-ptr I'll look later at the rest of the code. It's quite big. :) Paolo
Re: [PATCH v5 5/8] s390/sclp: use cpu offset to locate cpu entries
On Thu, 10 Sep 2020 05:36:52 -0400 Collin Walling wrote: > The start of the CPU entry region in the Read SCP Info response data is > denoted by the offset_cpu field. As such, QEMU needs to begin creating > entries at this address. > > This is in preparation for when Read SCP Info inevitably introduces new > bytes that push the start of the CPUEntry field further away. > > Read CPU Info is unlikely to ever change, so let's not bother > accounting for the offset there. > > Signed-off-by: Collin Walling > --- > hw/s390x/sclp.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Cornelia Huck
Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32
On Fri, 11 Sep 2020 at 02:14, zou xu wrote: > > From 533ed02427bdaf0265f62fcb4f961854a41b7037 Mon Sep 17 00:00:00 2001 > From: ZouXu > Date: Wed, 9 Sep 2020 21:59:13 +0800 > Subject: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for > mips32 > > Those MIPS32 instructions can cause store operation: > SB/SH/SW/SD/SWL/SWR/SDL/SDR/CACHE > SC/SCD/SWC1/SWC2/SDC1/SDC2 > --- > accel/tcg/user-exec.c | 34 +++--- > 1 file changed, 31 insertions(+), 3 deletions(-) > > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c > index bb039eb32d..b5ad721aa5 100644 > --- a/accel/tcg/user-exec.c > +++ b/accel/tcg/user-exec.c > @@ -708,10 +708,38 @@ int cpu_signal_handler(int host_signum, void *pinfo, > siginfo_t *info = pinfo; > ucontext_t *uc = puc; > greg_t pc = uc->uc_mcontext.pc; > -int is_write; > +int is_write = 0; > + > +/* Detect store by reading the instruction at the program counter. */ > +uint32_t insn = *(uint32_t *)pc; > +switch(insn>>29) { > +case 0x5: > +switch((insn>>26) & 0x7) { Here we mask to get a 3-bit field... > +case 0x0: /* SB */ > +case 0x1: /* SH */ > +case 0x2: /* SWL */ > +case 0x3: /* SW */ > +case 0x4: /* SDL */ > +case 0x5: /* SDR */ > +case 0x6: /* SWR */ > +case 0x7: /* CACHE */ > +is_write = 1; ...but here all 8 cases are handled identically. Is there a typo/logic error here, or should this really just be case 0x5: /* SB, SH, SWL, SW, SDL, SDR, SWR, CACHE */ is_write = 1; ? Is CACHE really a write insn ? > +} > +break; > +case 0x7: > +switch((insn>>26) & 0x7) { > +case 0x0: /* SC */ > +case 0x1: /* SWC1 */ > +case 0x2: /* SWC2 */ > +case 0x4: /* SCD */ > +case 0x5: /* SDC1 */ > +case 0x6: /* SDC2 */ > +case 0x7: /* SD */ > +is_write = 1; > +} > +break; > +} thanks -- PMM
Re: flatview_write_continue global mutex deadlock
On Thu, Sep 03, 2020 at 06:42:12PM +0300, Vladimir Sementsov-Ogievskiy wrote: > But still, is it OK to do blk_drain holding the global mutex? Drain may take > a relatively long time, and vm is not responding due to global mutex locked > in cpu thread.. This is a fundamental problem in QEMU. Unfortunately I/O requests cannot be canceled quickly (some protocols do not support cancelation at all). There are code paths like device reset or monitor commands where drain can hang for an unbounded amount of time :(. Stefan signature.asc Description: PGP signature
Re: [PATCH] PoC: Rust binding for QAPI (qemu-ga only, for now)
On Thu, Sep 10, 2020 at 09:48:50PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > Hi, > > Among the QEMU developers, there is a desire to use Rust. (see previous > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm > related projects and other experiments). > > Thanks to our QAPI type system and the associate code generator, it is > relatively straightforward to create Rust bindings for the generated C > types (also called sys/ffi binding) and functions. (rust-bindgen could > probably do a similar job, but it would probably bring other issues). > This provides an important internal API already. > > Slightly more complicated is to expose a Rust API for those, and provide > convenient conversions C<->Rust. Taking inspiration from glib-rs > binding, I implemented a simplified version of the FromGlib/ToGlib > traits, with simpler ownership model, sufficient for QAPI needs. > > The usage is relatively simple: > > - from_qemu_none(ptr: *const sys::P) -> T > Return a Rust type T for a const ffi pointer P. > > - from_qemu_full(ptr: *mut sys::P) -> T > Return a Rust type T for a ffi pointer P, taking ownership. > > - T::to_qemu_none() -> Stash > Returns a borrowed ffi pointer P (using a Stash to destroy "glue" > storage data, if any). > > - T::to_qemu_full() -> P > Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) > > With those traits, it's relatively easy to implement the QMP callbacks. > With enough interest, we could eventually start rewriting QGA in > Rust, as it is a simple service. See qga/qmp.rs for some examples. > We could also try to tackle qemu itself. > > Finally, given that the QAPI types are easy to serialize, it was simple > to use "serde" on them, and provide a D-Bus interface for QMP with zbus. > (a similar approach could probably be taken for other protocols, that > could be dynamically loaded... anyone like protobuf better?) > > This PoC modifies qemu-ga to provide the interface on the session bus: > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > ... > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1 > ... > > Note: the generated code doesn't work with the qemu schema, there is a > couple of fixme/todo left. > > Shameful pain point: meson & cargo don't play nicely together. Do we actually need/want it to be in the same monolithic repo as qemu, as opposed to a qemu-qapi-rust repo ? Trying to weld together different build systems is never that attractive. The language specific build systems generally are much simpler if they're self contained. From a distro POV it can be better if the language bindings are self contained, as you don't neccessarily want to build the language binding in the same environment as the main app. For example with modules in Fedora or RHEL, there can be multiple parallel versions of a language runtime, and thus language bindings would be built separately from QEMU. IIUC, you're generating stuff from the QEMU schemas. One way we can deal with this is to actually install the QEMU schemas into /usr/share. Distros would have an "qemu-devel" package that provided the schemas and the QAPI base tools which can then be used by separate bindings. 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 v5 6/8] s390/sclp: add extended-length sccb support for kvm guest
On Thu, 10 Sep 2020 05:36:53 -0400 Collin Walling wrote: > As more features and facilities are added to the Read SCP Info (RSCPI) > response, more space is required to store them. The space used to store > these new features intrudes on the space originally used to store CPU > entries. This means as more features and facilities are added to the > RSCPI response, less space can be used to store CPU entries. > > With the Extended-Length SCCB (ELS) facility, a KVM guest can execute > the RSCPI command and determine if the SCCB is large enough to store a > complete reponse. If it is not large enough, then the required length > will be set in the SCCB header. > > The caller of the SCLP command is responsible for creating a > large-enough SCCB to store a complete response. Proper checking should > be in place, and the caller should execute the command once-more with > the large-enough SCCB. > > This facility also enables an extended SCCB for the Read CPU Info > (RCPUI) command. > > When this facility is enabled, the boundary violation response cannot > be a result from the RSCPI, RSCPI Forced, or RCPUI commands. > > In order to tolerate kernels that do not yet have full support for this > feature, a "fixed" offset to the start of the CPU Entries within the > Read SCP Info struct is set to allow for the original 248 max entries > when this feature is disabled. > > Additionally, this is introduced as a CPU feature to protect the guest > from migrating to a machine that does not support storing an extended > SCCB. This could otherwise hinder the VM from being able to read all > available CPU entries after migration (such as during re-ipl). > > Signed-off-by: Collin Walling > --- > hw/s390x/sclp.c | 43 + > include/hw/s390x/sclp.h | 1 + > target/s390x/cpu_features_def.h.inc | 1 + > target/s390x/gen-features.c | 1 + > target/s390x/kvm.c | 8 ++ > 5 files changed, 48 insertions(+), 6 deletions(-) Acked-by: Cornelia Huck
Re: [PULL v2 01/46] qemu-iotests: move check-block back to Makefiles
On 06.09.20 19:53, Paolo Bonzini wrote: > check-block has its own test harness, unlike every other test. If > we capture its output, as is in general nicer to do without V=1, > there will be no sign of progress. So for lack of a better option > just move the invocation of the test back to Makefile rules. > > As a side effect, this will also fix "make check" in --disable-tools > builds, as they were trying to run qemu-iotests without having > made qemu-img before. > > Signed-off-by: Paolo Bonzini > --- > meson.build| 1 - > tests/Makefile.include | 15 --- > tests/qemu-iotests/meson.build | 4 > 3 files changed, 12 insertions(+), 8 deletions(-) I’m not entirely sure why (or I would’ve sent a patch myself), but this breaks all iotests using the socket_scm_helper program unless one runs make check-block before (or make tests/qemu-iotests/socket_scm_helper specifically). It seems like the socket_scm_helper is now only built as a dependency of check-block, instead of all the time. That’s a bit of a shame. (The obvious workaround of course is to specifically build the socket_scm_helper, but that doesn’t seem right.) As a reproducer: [delete build directory, or most importantly the socket_scm_helper] [configure] [make] $ cd tests/qemu-iotests $ ./check -nbd 147 [...] +qemu.machine.QEMUMachineError: socket_scm_helper does not exist [...] Max signature.asc Description: OpenPGP digital signature
Re: [PATCH] PoC: Rust binding for QAPI (qemu-ga only, for now)
Hi On Fri, Sep 11, 2020 at 2:47 PM Daniel P. Berrangé wrote: > On Thu, Sep 10, 2020 at 09:48:50PM +0400, marcandre.lur...@redhat.com > wrote: > > From: Marc-André Lureau > > > > Hi, > > > > Among the QEMU developers, there is a desire to use Rust. (see previous > > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm > > related projects and other experiments). > > > > Thanks to our QAPI type system and the associate code generator, it is > > relatively straightforward to create Rust bindings for the generated C > > types (also called sys/ffi binding) and functions. (rust-bindgen could > > probably do a similar job, but it would probably bring other issues). > > This provides an important internal API already. > > > > Slightly more complicated is to expose a Rust API for those, and provide > > convenient conversions C<->Rust. Taking inspiration from glib-rs > > binding, I implemented a simplified version of the FromGlib/ToGlib > > traits, with simpler ownership model, sufficient for QAPI needs. > > > > The usage is relatively simple: > > > > - from_qemu_none(ptr: *const sys::P) -> T > > Return a Rust type T for a const ffi pointer P. > > > > - from_qemu_full(ptr: *mut sys::P) -> T > > Return a Rust type T for a ffi pointer P, taking ownership. > > > > - T::to_qemu_none() -> Stash > > Returns a borrowed ffi pointer P (using a Stash to destroy "glue" > > storage data, if any). > > > > - T::to_qemu_full() -> P > > Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) > > > > With those traits, it's relatively easy to implement the QMP callbacks. > > With enough interest, we could eventually start rewriting QGA in > > Rust, as it is a simple service. See qga/qmp.rs for some examples. > > We could also try to tackle qemu itself. > > > > Finally, given that the QAPI types are easy to serialize, it was simple > > to use "serde" on them, and provide a D-Bus interface for QMP with zbus. > > (a similar approach could probably be taken for other protocols, that > > could be dynamically loaded... anyone like protobuf better?) > > > > This PoC modifies qemu-ga to provide the interface on the session bus: > > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v > > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > > ... > > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1 > > ... > > > > Note: the generated code doesn't work with the qemu schema, there is a > > couple of fixme/todo left. > > > > Shameful pain point: meson & cargo don't play nicely together. > > Do we actually need/want it to be in the same monolithic repo > as qemu, as opposed to a qemu-qapi-rust repo ? > > Trying to weld together different build systems is never that > attractive. The language specific build systems generally are > much simpler if they're self contained. From a distro POV it > can be better if the language bindings are self contained, as > you don't neccessarily want to build the language binding in > the same environment as the main app. For example with modules > in Fedora or RHEL, there can be multiple parallel versions of > a language runtime, and thus language bindings would be built > separately from QEMU. > > IIUC, you're generating stuff from the QEMU schemas. One way > we can deal with this is to actually install the QEMU schemas > into /usr/share. Distros would have an "qemu-devel" package > that provided the schemas and the QAPI base tools which > can then be used by separate bindings. > > - the schema is configured at build time. The C QAPI types depend on build conditions. (although I don't take that into account yet, but that shouldn't be too hard to add) - one of the goals is to start rewriting some part of QEMU in Rust. Here, I started with qemu-ga because it's an "easy" target. Generating binding for QMP schema (the protocol) is slightly easier, since the JSON messages are loosely typed. Yet, we would probably recommend you do it from qmp introspect output, at runtime. How close that is from the original QAPI schema, I can't tell, I never really looked or used it (it's not human friendly to start with, iirc) Now, I am confident various people are trying to improve the situation wrt meson+cargo integration, I'll do some more research. thanks
Re: [PULL v2 01/46] qemu-iotests: move check-block back to Makefiles
On 11.09.20 12:58, Max Reitz wrote: > On 06.09.20 19:53, Paolo Bonzini wrote: >> check-block has its own test harness, unlike every other test. If >> we capture its output, as is in general nicer to do without V=1, >> there will be no sign of progress. So for lack of a better option >> just move the invocation of the test back to Makefile rules. >> >> As a side effect, this will also fix "make check" in --disable-tools >> builds, as they were trying to run qemu-iotests without having >> made qemu-img before. >> >> Signed-off-by: Paolo Bonzini >> --- >> meson.build| 1 - >> tests/Makefile.include | 15 --- >> tests/qemu-iotests/meson.build | 4 >> 3 files changed, 12 insertions(+), 8 deletions(-) > > I’m not entirely sure why (or I would’ve sent a patch myself) On closer inspection it seems like it’s because of the “build_by_default: false”, which seems like a rather conscious decision. Was I only lucky that the socket_scm_helper was built by default so far? Should I have explicitly built it all this time? Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 2/2] qcow2: Skip copy-on-write when allocating a zero cluster
11.09.2020 13:04, Alberto Garcia wrote: On Fri 11 Sep 2020 11:34:37 AM CEST, Vladimir Sementsov-Ogievskiy wrote: -if (!is_zero_cow(bs, m)) { +ret = is_zero_cow(bs, m); +if (ret < 0) { +return ret; It's a common practice to treat block-status errors as "unknown" status and not error-out immediately: - really, it's not critical, we can continue assuming non-zero - if there are real problems with IO, we'll most probably fail on real read or write operation, and report its status, which seems better for user than block-status error But what's the problem exactly, does this complicate the code too much? :-? Of course not :) Hmm. OK, I don't know, I'm just used to this practice in block jobs. Patch is correct as is: Reviewed-by: Vladimir Sementsov-Ogievskiy So, I'd keep existing logic in handle_alloc_space(). And, if you agree and resend, probably good to split this patch into two, one for block.h/io.c and one for qcow2.c (still, I'm OK with it as one patch). Sure, I can split the patch if I have to resend it. Berto -- Best regards, Vladimir
Re: [PULL v2 01/46] qemu-iotests: move check-block back to Makefiles
On 11/09/20 12:58, Max Reitz wrote: > It seems like the socket_scm_helper is now only built as a dependency of > check-block, instead of all the time. That’s a bit of a shame. > (The obvious workaround of course is to specifically build the > socket_scm_helper, but that doesn’t seem right.) Or just remove the build_by_default: false here: socket_scm_helper = executable('socket_scm_helper', 'socket_scm_helper.c', build_by_default: false) I guess now is a good time to decide which executables to build by default (static_libraries should never be built by default) and document it in docs/devel/build-system.rst. Right now, the only executables that aren't built by default are: - rdmacm-mux and vhost-user-blk because they're broken - gen-features because it's built anyway for s390 targets - vhost-user-bridge, and that probably should be changed - socket_scm_helper, which could/should be changed too - fptest, not sure why that works at all O:-) Tests are built by default (and they trigger coverity quite a bit). We will be able to fix that, and at the same time respect the "tests(depends: ...)" argument instead of just having "check: all", when meson 0.56.0 comes out. Paolo signature.asc Description: OpenPGP digital signature
Re: [PULL v2 01/46] qemu-iotests: move check-block back to Makefiles
On 11/09/20 13:05, Max Reitz wrote: > On closer inspection it seems like it’s because of the > “build_by_default: false”, which seems like a rather conscious decision. > Was I only lucky that the socket_scm_helper was built by default so > far? Should I have explicitly built it all this time? Yes, you were lucky but that's not a reason not to change it. The problem with touching a whole build system is that you are not going to test everybody's usecase, and yours is definitely reasonable and not "M-x butterfly" territory. Paolo signature.asc Description: OpenPGP digital signature
Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
On Fri, 11 Sep 2020 13:18:32 +0300 Vladimir Sementsov-Ogievskiy wrote: > 11.09.2020 12:38, Greg Kurz wrote: > > s/startus/status > > > > On Wed, 9 Sep 2020 21:59:27 +0300 > > Vladimir Sementsov-Ogievskiy wrote: > > > >> It's better to return status together with setting errp. It makes > >> possible to avoid error propagation. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy > >> --- > >> block/qcow2.h| 2 +- > >> block/qcow2-bitmap.c | 13 ++--- > >> 2 files changed, 7 insertions(+), 8 deletions(-) > >> > >> diff --git a/block/qcow2.h b/block/qcow2.h > >> index e7e662533b..49824be5c6 100644 > >> --- a/block/qcow2.h > >> +++ b/block/qcow2.h > >> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > >> Qcow2BitmapInfoList **info_list, Error > >> **errp); > >> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > >> int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); > >> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > >> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > >> bool release_stored, Error > >> **errp); > >> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); > >> bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, > >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > >> index f58923fce3..5eeff1cb1c 100644 > >> --- a/block/qcow2-bitmap.c > >> +++ b/block/qcow2-bitmap.c > >> @@ -1524,9 +1524,10 @@ out: > >>* readonly to begin with, and whether we opened directly or reopened to > >> that > >>* state shouldn't matter for the state we get afterward. > >>*/ > >> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > >> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > >> bool release_stored, Error > >> **errp) > >> { > >> +ERRP_GUARD(); > > > > Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes > > an error_prepend(errp, ...) not visible in the patch context. > > Ah yes. Actually this is occasional thing I didn't want to include into this > patch > (and int this part I). So we can just drop it and leave for part II or part > III, > or add a note into commit message > > > > > Anyway, > > > > Reviewed-by: Greg Kurz > > Thanks a lot for reviewing :) > Don't mention it :) > Hmm.. With this series I understand the following: > > 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to > the whole code-base, because: > >- it produces a lot of "if (*errp)" in places where it is really simple to > avoid error propagation at all, like in this series >- reviewing is the hardest part of the process > > So, if we have to review these changes anyway, it's better to invest a bit > more time into patch creation, and make code correspond to our modern error > API recommendations. > > 2. So, the process turns into following steps: > >- apply scripts/coccinelle/errp-guard.cocci >- look through patches and do obvious refactorings (like this series) >- keep ERRP_GUARD where necessary (appending info to error, or where > refactoring of function return status is too invasive and not simple) > I've started to follow this process for the spapr code and, indeed, I can come up with better changes by refactoring some code manually. Some of these changes are not that obvious that they could be made by someone who doesn't know the code, so I tend to agree with your arguments in 1. This is also the reason I didn't review patches 10, 13 and 14 because they looked like I should understand the corresponding code a bit more. > 3. Obviously, that's too much for me :) Of course, I will invest some time > into making the series like this one, and reviewing them, but I can't do it > for weeks and months. (My original сunning plan to simply push ~100 generated > commits with my s-o-b and become the greatest contributor failed:) > Ha ha :D ... as a consolation prize, maybe we can reach a fair number of r-b by reviewing each other's _simple_ cleanups ;-) > The good thing is that now, with ERRP_GUARD finally merged, we can produce > parallel series like this, and they will be processed in parallel by > different maintainers (and Markus will have to merge series for subsystems > with unavailable maintainers). > This sounds nice. My only concern would be to end up fixing code nobody uses or cares for, so I guess it would be better that active maintainers or supporters give impetus on that. > So, everybody is welcome to the process [2]. Probably we want to make a > separate announcement in a list with short recommendations and instructions? > But who read announcements.. > I don't :) but the very massive series that were posted on the topic the last few months look like an announcement to me, at least for active maintain
Re: [PATCH] PoC: Rust binding for QAPI (qemu-ga only, for now)
On 11/09/20 12:46, Daniel P. Berrangé wrote: > Do we actually need/want it to be in the same monolithic repo > as qemu, as opposed to a qemu-qapi-rust repo ? I think QAPI and qemu-ga should be separate repos altogether. QAPI should be included as a submodule in both qemu and qemu-ga. qemu-ga instead has absolutely no dependency on QEMU and viceversa, and is a prime candidate for removing all traces of the configure script and being a pure meson project. Paolo
Re: [PATCH] PoC: Rust binding for QAPI (qemu-ga only, for now)
On Fri, Sep 11, 2020 at 01:28:13PM +0200, Paolo Bonzini wrote: > On 11/09/20 12:46, Daniel P. Berrangé wrote: > > Do we actually need/want it to be in the same monolithic repo > > as qemu, as opposed to a qemu-qapi-rust repo ? > > I think QAPI and qemu-ga should be separate repos altogether. QAPI > should be included as a submodule in both qemu and qemu-ga. qemu-ga > instead has absolutely no dependency on QEMU and viceversa, and is a > prime candidate for removing all traces of the configure script and > being a pure meson project. Yeah, qemu-ga has always been an odd fit, since it is a guest OS component rather than host OS. 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 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
11.09.2020 14:21, Greg Kurz wrote: On Fri, 11 Sep 2020 13:18:32 +0300 Vladimir Sementsov-Ogievskiy wrote: 11.09.2020 12:38, Greg Kurz wrote: s/startus/status On Wed, 9 Sep 2020 21:59:27 +0300 Vladimir Sementsov-Ogievskiy wrote: It's better to return status together with setting errp. It makes possible to avoid error propagation. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/qcow2.h| 2 +- block/qcow2-bitmap.c | 13 ++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index e7e662533b..49824be5c6 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, Qcow2BitmapInfoList **info_list, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index f58923fce3..5eeff1cb1c 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1524,9 +1524,10 @@ out: * readonly to begin with, and whether we opened directly or reopened to that * state shouldn't matter for the state we get afterward. */ -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored, Error **errp) { +ERRP_GUARD(); Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes an error_prepend(errp, ...) not visible in the patch context. Ah yes. Actually this is occasional thing I didn't want to include into this patch (and int this part I). So we can just drop it and leave for part II or part III, or add a note into commit message Anyway, Reviewed-by: Greg Kurz Thanks a lot for reviewing :) Don't mention it :) Hmm.. With this series I understand the following: 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because: - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series - reviewing is the hardest part of the process So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations. 2. So, the process turns into following steps: - apply scripts/coccinelle/errp-guard.cocci - look through patches and do obvious refactorings (like this series) - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple) I've started to follow this process for the spapr code and, indeed, I can come up with better changes by refactoring some code manually. Some of these changes are not that obvious that they could be made by someone who doesn't know the code, so I tend to agree with your arguments in 1. This is also the reason I didn't review patches 10, 13 and 14 because they looked like I should understand the corresponding code a bit more. 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:) Ha ha :D ... as a consolation prize, maybe we can reach a fair number of r-b by reviewing each other's _simple_ cleanups ;-) The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers). This sounds nice. My only concern would be to end up fixing code nobody uses or cares for, so I guess it would be better that active maintainers or supporters give impetus on that. So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements.. I don't :) but the very massive series that were posted on the topic the last few months look like an announcement to me, at least for active maintainers and supporters. Aha, I know. Better than announcement is improving checkpatch. BdrvDirtyBitmap *bitmap; BDRVQcow2State *s = bs->opaque; uint32_t new_nb_bitmaps = s->nb_bitmaps; @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_di
Re: [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP
Kevin: While we're still arguing about details of the last commit; can we get the first few commits in - they seem to be generally nice cleanups/error handling. Dave * Daniel P. Berrangé (berra...@redhat.com) wrote: > v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00866.html > v2: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg07523.html > > When QMP was first introduced some 10+ years ago now, the snapshot > related commands (savevm/loadvm/delvm) were not converted. This was > primarily because their implementation causes blocking of the thread > running the monitor commands. This was (and still is) considered > undesirable behaviour both in HMP and QMP. > > In theory someone was supposed to fix this flaw at some point in the > past 10 years and bring them into the QMP world. Sadly, thus far it > hasn't happened as people always had more important things to work > on. Enterprise apps were much more interested in external snapshots > than internal snapshots as they have many more features. > > Meanwhile users still want to use internal snapshots as there is > a certainly simplicity in having everything self-contained in one > image, even though it has limitations. Thus the apps that end up > executing the savevm/loadvm/delvm via the "human-monitor-command" > QMP command. > > IOW, the problematic blocking behaviour that was one of the reasons > for not having savevm/loadvm/delvm in QMP is experienced by applications > regardless. By not portting the commands to QMP due to one design flaw, > we've forced apps and users to suffer from other design flaws of HMP ( > bad error reporting, strong type checking of args, no introspection) for > an additional 10 years. This feels rather sub-optimal :-( > > In practice users don't appear to care strongly about the fact that these > commands block the VM while they run. I might have seen one bug report > about it, but it certainly isn't something that comes up as a frequent > topic except among us QEMU maintainers. Users do care about having > access to the snapshot feature. > > Where I am seeing frequent complaints is wrt the use of OVMF combined > with snapshots which has some serious pain points. This is getting worse > as the push to ditch legacy BIOS in favour of UEFI gain momentum both > across OS vendors and mgmt apps. Solving it requires new parameters to > the commands, but doing this in HMP is super unappealing. > > After 10 years, I think it is time for us to be a little pragmatic about > our handling of snapshots commands. My desire is that libvirt should never > use "human-monitor-command" under any circumstances, because of the > inherant flaws in HMP as a protocol for machine consumption. > > Thus in this series I'm proposing a fairly direct mapping of the existing > HMP commands for savevm/loadvm/delvm into QMP as a first step. This does > not solve the blocking thread problem, but it does put in a place a > design using the jobs framework which can facilitate solving it later. > It does also solve the error reporting, type checking and introspection > problems inherant to HMP. So we're winning on 3 out of the 4 problems, > and pushed apps to a QMP design that will let us solve the last > remaining problem. > > With a QMP variant, we reasonably deal with the problems related to OVMF: > > - The logic to pick which disk to store the vmstate in is not >satsifactory. > >The first block driver state cannot be assumed to be the root disk >image, it might be OVMF varstore and we don't want to store vmstate >in there. > > - The logic to decide which disks must be snapshotted is hardwired >to all disks which are writable > >Again with OVMF there might be a writable varstore, but this can be >raw rather than qcow2 format, and thus unable to be snapshotted. >While users might wish to snapshot their varstore, in some/many/most >cases it is entirely uneccessary. Users are blocked from snapshotting >their VM though due to this varstore. > > These are solved by adding two parameters to the commands. The first is > a block device node name that identifies the image to store vmstate in, > and the second is a list of node names to include for the snapshots. > If the list of nodes isn't given, it falls back to the historical > behaviour of using all disks matching some undocumented criteria. > > In the block code I've only dealt with node names for block devices, as > IIUC, this is all that libvirt should need in the -blockdev world it now > lives in. IOW, I've made not attempt to cope with people wanting to use > these QMP commands in combination with -drive args, as libvirt will > never use -drive with a QEMU new enough to have these new commands. > > The main limitations of this current impl > > - The snapshot process runs serialized in the main thread. ie QEMU >guest execution is blocked for the duration. The job framework >lets us fix this in future without changing the
Re: [Bug 1895080] [NEW] pgb_reserved_va: Assertion `addr == test' failed
Hansni Bu <1895...@bugs.launchpad.net> writes: > No, it's not set by CentOS-7.5. > Does it mean that we just cannot run the ELF in such a case? I've tried > many times, the assert always fails. Maybe, we can blame CentOS-7.5. The trouble is without MAP_FIXED_NOREPLACE we are at the mercy of the host kernel to allow the address request to be honoured. A plain MAP_FIXED won't do as it can clober existing mappings. In theory a suitable hole has been identified but sometimes the kernel makes a decision to offset the suggested mapping for it's own reasons. > BTW: with the option "-p 65536", the case runs successfully. Hmm interesting. I wonder if we are seeing a fail due to mmap_min_addr? What does: /proc/sys/vm/mmap_min_addr give you on the system? You can manually set the reserved_va and the base address using -R and -B although that is more of a developer work around. I think moving the assert to the condition above would be an improvement just because it tells us what the requested base address was and what the kernel decided to give us. > > On Fri, Sep 11, 2020 at 5:50 PM Alex Bennée <1895...@bugs.launchpad.net> > wrote: > >> Alex Bennée writes: >> >> > Laurent Vivier writes: >> > >> >> >>> Then trying qemu-riscv32 with a simple ELF, I get: >> >>> linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' >> failed. >> >>> >> >>> strace shows that: >> >>> mmap(0x1000, 4294963200, PROT_NONE, >> MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 >> >>> write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: >> ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == >> test' failed. >> >>> ) = 103 >> >>> >> >>> The source code is in the function pgb_reserved_va (linux- >> >>> user/elfload.c). I think mmap cannot guarantee that the returned >> pointer >> >>> (test) equals to the parameter of addr. So is this a bug to assert >> (addr >> >>> == test)? >> >> >> > I'm assuming CentOS 7.5 actually has a definition for >> > MAP_FIXED_NOREPLACE which should ensure we get what we asked for - >> > otherwise we are in the position of hoping the kernel honours what we >> > asked for. >> >> Doh re-reading I see it's not set in the strace output. Maybe we should >> promote the assert case to the failure leg so we have: >> >> if (addr == MAP_FAILED || addr != test) { >> error_report(...) >> } >> >> so we at least fail with a user friendly error rather than an abort? >> >> -- >> Alex Bennée >> >> -- >> You received this bug notification because you are subscribed to the bug >> report. >> https://bugs.launchpad.net/bugs/1895080 >> >> Title: >> pgb_reserved_va: Assertion `addr == test' failed >> >> Status in QEMU: >> New >> >> Bug description: >> This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head >> (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- >> user. >> >> Firstly, compile fails: >> Compiling C object libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o >> ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ >> undeclared here (not in a function) >>FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), >> >> I have to add below include to linux-user/strace.c >> diff --git a/linux-user/strace.c b/linux-user/strace.c >> index 11fea14fba..22e51d4a8a 100644 >> --- a/linux-user/strace.c >> +++ b/linux-user/strace.c >> @@ -7,6 +7,7 @@ >>#include >>#include >>#include >> +#include >>#include >>#include >>#include >> >> Then trying qemu-riscv32 with a simple ELF, I get: >> linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' >> failed. >> >> strace shows that: >> mmap(0x1000, 4294963200, PROT_NONE, >> MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 >> write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: >> ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == >> test' failed. >> ) = 103 >> >> The source code is in the function pgb_reserved_va (linux- >> user/elfload.c). I think mmap cannot guarantee that the returned >> pointer (test) equals to the parameter of addr. So is this a bug to >> assert (addr == test)? >> >> Attached configure script and test ELF file. >> >> Thanks. >> >> To manage notifications about this bug go to: >> https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions >> -- Alex Bennée
转发: [PATCH RESEND] Fix video playback slowdown when spice client no audio enabled
Because I don't received any feedback, so i have decide resend again. with more detail. @DirtY.iCE.hu, Can you review this patch? or someone else has time ? Issue: video playback slowdown, using spice client --spice-disable-audio option How to reproduce: 1. compile qemu master for x86_64 target with spice enabled. 2. laucher a windows 7 virtual machine with some like: qemu-system-x86_64 --enable-kvm -M q35,accel=kvm -display none -spice port=5907,addr=0.0.0.0,disable-ticketing -device intel-hda,id=s0 -device hda-duplex,id=s0-codec0,bus=s0.0,cad=0 3. connect the virtual machine using official remote-viewer without spice audio channel. which is: remote-viewer spice://localhost:5907 --spice-disable-audio 4. open windows sample video using default media player. video location: C:\Users\Public\Videos\Sample Videos\Wildlife.wmv You will see the video playback speed is very slow. I HAVE TEST QEMU 4.2.1 DOES NOT HAVE THIS ISSUE. And I believe this issue was indroduced by this commit: https://github.com/qemu/qemu/commit/fb35c2cec58985f0b8d2733f1b91927542eeb3fd best regards 发件人: zhou qi 发送时间: 2020年9月10日 19:17 收件人: qemu-devel@nongnu.org 主题: [PATCH] Fix video playback slowdown when spice client no audio enabled From e8c2e283f0954de255a32ea70d577d5e61992399 Mon Sep 17 00:00:00 2001 From: Qi Zhou Date: Thu, 10 Sep 2020 19:09:29 +0800 Subject: [PATCH] Fix video playback slowdown when spice client no audio enabled You will get video playback slowdown on the following cases 1. use official spice client with audio channel disabled, use --spice-disable-audio option 2. thirtpart client doesn't implement audio channel ref: https://github.com/qemu/qemu/commit/fb35c2cec58985f0b8d2733f1b91927542eeb3fd Signed-off-by: Qi Zhou --- audio/audio.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/audio/audio.c b/audio/audio.c index ce8c6dec5f..50febe190f 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -1091,7 +1091,11 @@ static size_t audio_pcm_hw_run_out(HWVoiceOut *hw, size_t live) while (live) { size_t size, decr, proc; void *buf = hw->pcm_ops->get_buffer_out(hw, &size); - if (!buf || size == 0) { + + if (!buf) { + hw->mix_buf->pos = (hw->mix_buf->pos + live) % hw->mix_buf->size; + return clipped + live; + } else if ( size == 0) { break; } -- 2.17.1
[PATCH] hw/arm/pxa2xx: Add missing fallthrough comment
Let's make this file compilable with -Werror=implicit-fallthrough : Looking at the code, it seems like the fallthrough is intended here, so we should add the corresponding "/* fallthrough */" comment here. Signed-off-by: Thomas Huth --- hw/arm/pxa2xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c index 76975d17a4..c1f45b2adf 100644 --- a/hw/arm/pxa2xx.c +++ b/hw/arm/pxa2xx.c @@ -443,7 +443,7 @@ static void pxa2xx_mm_write(void *opaque, hwaddr addr, s->mm_regs[addr >> 2] = value; break; } - +/* fallthrough */ default: qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad write offset 0x%"HWADDR_PRIx"\n", -- 2.18.2
Re: [PATCH] riscv: sifive_test: Allow 16-bit writes to memory region
Quoting Alistair Francis (2020-09-10 13:10:43) > On Thu, Sep 3, 2020 at 2:05 PM Michael Roth wrote: > > > > Quoting Alistair Francis (2020-09-01 18:59:29) > > > On Mon, Aug 31, 2020 at 10:59 PM Nathan Chancellor > > > wrote: > > > > > > > > When shutting down the machine running a mainline Linux kernel, the > > > > following error happens: > > > > > > > > $ build/riscv64-softmmu/qemu-system-riscv64 -bios default -M virt \ > > > > -display none -initrd rootfs.cpio -kernel Image -m 512m \ > > > > -nodefaults -serial mon:stdio > > > > ... > > > > Requesting system poweroff > > > > [4.999630] reboot: Power down > > > > sbi_trap_error: hart0: trap handler failed (error -2) > > > > sbi_trap_error: hart0: mcause=0x0007 > > > > mtval=0x0010 > > > > sbi_trap_error: hart0: mepc=0x8000d4cc > > > > mstatus=0x1822 > > > > sbi_trap_error: hart0: ra=0x8000999e sp=0x80015c78 > > > > sbi_trap_error: hart0: gp=0xffe000e76610 tp=0xffe0081b89c0 > > > > sbi_trap_error: hart0: s0=0x80015c88 s1=0x0040 > > > > sbi_trap_error: hart0: a0=0x a1=0x80004024 > > > > sbi_trap_error: hart0: a2=0x80004024 a3=0x80004024 > > > > sbi_trap_error: hart0: a4=0x0010 a5=0x > > > > sbi_trap_error: hart0: a6=0x4024 a7=0x80011158 > > > > sbi_trap_error: hart0: s2=0x s3=0x80016000 > > > > sbi_trap_error: hart0: s4=0x s5=0x > > > > sbi_trap_error: hart0: s6=0x0001 s7=0x > > > > sbi_trap_error: hart0: s8=0x s9=0x > > > > sbi_trap_error: hart0: s10=0x s11=0x0008 > > > > sbi_trap_error: hart0: t0=0x t1=0x > > > > sbi_trap_error: hart0: t2=0x t3=0x > > > > sbi_trap_error: hart0: t4=0x t5=0x > > > > sbi_trap_error: hart0: t6=0x > > > > > > > > The kernel does a 16-bit write when powering off the machine, which > > > > was allowed before commit 5d971f9e67 ("memory: Revert "memory: accept > > > > mismatching sizes in memory_region_access_valid""). Make min_access_size > > > > match reality so that the machine can shut down properly now. > > > > > > > > Cc: qemu-sta...@nongnu.org > > > > Fixes: 88a07990fa ("SiFive RISC-V Test Finisher") > > > > Fixes: 5d971f9e67 ("memory: Revert "memory: accept mismatching sizes in > > > > memory_region_access_valid"") > > > > Signed-off-by: Nathan Chancellor > > > > > > Thanks! > > > > > > Applied to riscv-to-apply.next > > > > FYI, I'm hoping to pull this patch into the upcoming 5.0.1 stable > > release. The freeze is scheduled for 2020-09-20, I will apply this if it > > hits master before then. > > I just sent a PR with this patch. Is this still on track to make it into > 5.0.1? Since it's not likely to invalidate any testing on my end outside of the ones built into QEMU I can probably still slip it in if it hits master by Monday, or maybe just grab it from your tree. > > Alistair > > > > > > > > > Alistair > > > > > > > --- > > > > > > > > Please let me know if the tags are wrong or inappropriate, this is my > > > > first QEMU patch. > > > > > > > > hw/riscv/sifive_test.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c > > > > index 0c78fb2c93..8c70dd69df 100644 > > > > --- a/hw/riscv/sifive_test.c > > > > +++ b/hw/riscv/sifive_test.c > > > > @@ -59,7 +59,7 @@ static const MemoryRegionOps sifive_test_ops = { > > > > .write = sifive_test_write, > > > > .endianness = DEVICE_NATIVE_ENDIAN, > > > > .valid = { > > > > -.min_access_size = 4, > > > > +.min_access_size = 2, > > > > .max_access_size = 4 > > > > } > > > > }; > > > > > > > > base-commit: 2f4c51c0f384d7888a04b4815861e6d5fd244d75 > > > > -- > > > > 2.28.0 > > > > > > > > > > >
[PATCH] hw: usb: hcd-ohci: check len and frame_number variables
From: Prasad J Pandit While servicing the OHCI transfer descriptors(TD), OHCI host controller derives variables 'start_addr', 'end_addr', 'len' etc. from values supplied by the host controller driver. Host controller driver may supply values such that using above variables leads to out-of-bounds access or loop issues. Add checks to avoid them. AddressSanitizer: stack-buffer-overflow on address 0x7ffd53af76a0 READ of size 2 at 0x7ffd53af76a0 thread T0 #0 ohci_service_iso_td ../hw/usb/hcd-ohci.c:734 #1 ohci_service_ed_list ../hw/usb/hcd-ohci.c:1180 #2 ohci_process_lists ../hw/usb/hcd-ohci.c:1214 #3 ohci_frame_boundary ../hw/usb/hcd-ohci.c:1257 #4 timerlist_run_timers ../util/qemu-timer.c:572 #5 qemu_clock_run_timers ../util/qemu-timer.c:586 #6 qemu_clock_run_all_timers ../util/qemu-timer.c:672 #7 main_loop_wait ../util/main-loop.c:527 #8 qemu_main_loop ../softmmu/vl.c:1676 #9 main ../softmmu/main.c:50 Reported-by: Gaoning Pan Reported-by: Yongkang Jia Reported-by: Yi Ren Signed-off-by: Prasad J Pandit --- hw/usb/hcd-ohci.c | 29 +++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 1e6e85e86a..76fb9282e3 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -691,6 +691,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, the next ISO TD of the same ED */ trace_usb_ohci_iso_td_relative_frame_number_big(relative_frame_number, frame_count); +if (OHCI_CC_DATAOVERRUN == OHCI_BM(iso_td.flags, TD_CC)) { +/* avoid infinite loop */ +return 1; +} + OHCI_SET_BM(iso_td.flags, TD_CC, OHCI_CC_DATAOVERRUN); ed->head &= ~OHCI_DPTR_MASK; ed->head |= (iso_td.next & OHCI_DPTR_MASK); @@ -731,7 +736,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, } start_offset = iso_td.offset[relative_frame_number]; -next_offset = iso_td.offset[relative_frame_number + 1]; +if (relative_frame_number < frame_count) { +next_offset = iso_td.offset[relative_frame_number + 1]; +} else { +next_offset = iso_td.be; +} if (!(OHCI_BM(start_offset, TD_PSW_CC) & 0xe) || ((relative_frame_number < frame_count) && @@ -764,7 +773,12 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, } } else { /* Last packet in the ISO TD */ -end_addr = iso_td.be; +end_addr = next_offset; +} + +if (start_addr > end_addr) { +trace_usb_ohci_iso_td_bad_cc_overrun(start_addr, end_addr); +return 1; } if ((start_addr & OHCI_PAGE_MASK) != (end_addr & OHCI_PAGE_MASK)) { @@ -773,6 +787,9 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed, } else { len = end_addr - start_addr + 1; } +if (len > sizeof(ohci->usb_buf)) { +len = sizeof(ohci->usb_buf); +} if (len && dir != OHCI_TD_DIR_IN) { if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, @@ -975,8 +992,16 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) if ((td.cbp & 0xf000) != (td.be & 0xf000)) { len = (td.be & 0xfff) + 0x1001 - (td.cbp & 0xfff); } else { +if (td.cbp > td.be) { +trace_usb_ohci_iso_td_bad_cc_overrun(td.cbp, td.be); +ohci_die(ohci); +return 1; +} len = (td.be - td.cbp) + 1; } +if (len > sizeof(ohci->usb_buf)) { +len = sizeof(ohci->usb_buf); +} pktlen = len; if (len && dir != OHCI_TD_DIR_IN) { -- 2.26.2
[PATCH] cphp: remove deprecated cpu-add command(s)
theses were deprecatedince since 4.0, remove both HMP and QMP variants. Users should use device_add commnad instead. To get list of possible CPUs and options, use 'info hotpluggable-cpus' HMP or query-hotpluggable-cpus QMP command. Signed-off-by: Igor Mammedov --- include/hw/boards.h | 1 - include/hw/i386/pc.h| 1 - include/monitor/hmp.h | 1 - docs/system/deprecated.rst | 25 + hmp-commands.hx | 15 -- hw/core/machine-hmp-cmds.c | 12 - hw/core/machine-qmp-cmds.c | 12 - hw/i386/pc.c| 27 -- hw/i386/pc_piix.c | 1 - hw/s390x/s390-virtio-ccw.c | 12 - qapi/machine.json | 24 - tests/qtest/cpu-plug-test.c | 100 tests/qtest/test-hmp.c | 1 - 13 files changed, 21 insertions(+), 211 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index bc5b82ad20..2163843bdb 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -173,7 +173,6 @@ struct MachineClass { void (*init)(MachineState *state); void (*reset)(MachineState *state); void (*wakeup)(MachineState *state); -void (*hot_add_cpu)(MachineState *state, const int64_t id, Error **errp); int (*kvm_type)(MachineState *machine, const char *arg); void (*smp_parse)(MachineState *ms, QemuOpts *opts); diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index fe52e165b2..ca8ff6cd27 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -137,7 +137,6 @@ extern int fd_bootchk; void pc_acpi_smi_interrupt(void *opaque, int irq, int level); -void pc_hot_add_cpu(MachineState *ms, const int64_t id, Error **errp); void pc_smp_parse(MachineState *ms, QemuOpts *opts); void pc_guest_info_init(PCMachineState *pcms); diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index c986cfd28b..642e9e91f9 100644 --- a/include/monitor/hmp.h +++ b/include/monitor/hmp.h @@ -89,7 +89,6 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict); void hmp_chardev_change(Monitor *mon, const QDict *qdict); void hmp_chardev_remove(Monitor *mon, const QDict *qdict); void hmp_chardev_send_break(Monitor *mon, const QDict *qdict); -void hmp_cpu_add(Monitor *mon, const QDict *qdict); void hmp_object_add(Monitor *mon, const QDict *qdict); void hmp_object_del(Monitor *mon, const QDict *qdict); void hmp_info_memdev(Monitor *mon, const QDict *qdict); diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 851dbdeb8a..122717cfee 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -284,13 +284,6 @@ The ``query-cpus`` command is replaced by the ``query-cpus-fast`` command. The ``arch`` output member of the ``query-cpus-fast`` command is replaced by the ``target`` output member. -``cpu-add`` (since 4.0) -''' - -Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See -documentation of ``query-hotpluggable-cpus`` for additional -details. - ``query-events`` (since 4.0) @@ -306,12 +299,6 @@ the 'wait' field, which is only applicable to sockets in server mode Human Monitor Protocol (HMP) commands - -``cpu-add`` (since 4.0) -''' - -Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See -documentation of ``query-hotpluggable-cpus`` for additional details. - ``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, ``acl_remove`` (since 4.0.0) '' @@ -514,6 +501,12 @@ QEMU Machine Protocol (QMP) commands The "autoload" parameter has been ignored since 2.12.0. All bitmaps are automatically loaded from qcow2 images. +``cpu-add`` (removed in 5.2) + + +Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See +documentation of ``query-hotpluggable-cpus`` for additional details. + Human Monitor Protocol (HMP) commands - @@ -523,6 +516,12 @@ The ``hub_id`` parameter of ``hostfwd_add`` / ``hostfwd_remove`` (removed in 5.0 The ``[hub_id name]`` parameter tuple of the 'hostfwd_add' and 'hostfwd_remove' HMP commands has been replaced by ``netdev_id``. +``cpu-add`` (removed in 5.2) + + +Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See +documentation of ``query-hotpluggable-cpus`` for additional details. + Guest Emulator ISAs --- diff --git a/hmp-commands.hx b/hmp-commands.hx index 60f395c276..d1e3e0e1c6 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1761,21 +1761,6 @@ SRST Executes a qemu-io command on the given block device. ERST -{ -.name = "cpu-add", -.args_type = "id:i", -.params = "id", -.help = "add cpu (deprecated, use device_add instead)", -.cmd= hmp
Re: [Bug 1895080] [NEW] pgb_reserved_va: Assertion `addr == test' failed
> > No, it's not set by CentOS-7.5. > > Does it mean that we just cannot run the ELF in such a case? I've tried > > many times, the assert always fails. Maybe, we can blame CentOS-7.5. > > The trouble is without MAP_FIXED_NOREPLACE we are at the mercy of the > host kernel to allow the address request to be honoured. A plain > MAP_FIXED won't do as it can clober existing mappings. In theory a > suitable hole has been identified but sometimes the kernel makes a > decision to offset the suggested mapping for it's own reasons. > MAP_FIXED_NOREPLACE is quite a new feature. > > BTW: with the option "-p 65536", the case runs successfully. > > Hmm interesting. I wonder if we are seeing a fail due to mmap_min_addr? > What does: > > /proc/sys/vm/mmap_min_addr > > give you on the system? > It gives me 4096. And guest_base has this value. Maybe that's the strange point. mmap_min_addr give us 0x1000. While we are requesting this address, the kernel gives us 0x1. > > You can manually set the reserved_va and the base address using -R and > -B although that is more of a developer work around. I think moving the > assert to the condition above would be an improvement just because it > tells us what the requested base address was and what the kernel decided > to give us. > Setting guest_base with -B to 0x1 works. Tried some -R values, no luck. Agree to print a more hintful message. > > > > > On Fri, Sep 11, 2020 at 5:50 PM Alex Bennée <1895...@bugs.launchpad.net> > > wrote: > > > >> Alex Bennée writes: > >> > >> > Laurent Vivier writes: > >> > > >> > >> >>> Then trying qemu-riscv32 with a simple ELF, I get: > >> >>> linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' > >> failed. > >> >>> > >> >>> strace shows that: > >> >>> mmap(0x1000, 4294963200, PROT_NONE, > >> MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 > >> >>> write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: > >> ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr > == > >> test' failed. > >> >>> ) = 103 > >> >>> > >> >>> The source code is in the function pgb_reserved_va (linux- > >> >>> user/elfload.c). I think mmap cannot guarantee that the returned > >> pointer > >> >>> (test) equals to the parameter of addr. So is this a bug to assert > >> (addr > >> >>> == test)? > >> >> > >> > I'm assuming CentOS 7.5 actually has a definition for > >> > MAP_FIXED_NOREPLACE which should ensure we get what we asked for - > >> > otherwise we are in the position of hoping the kernel honours what we > >> > asked for. > >> > >> Doh re-reading I see it's not set in the strace output. Maybe we should > >> promote the assert case to the failure leg so we have: > >> > >> if (addr == MAP_FAILED || addr != test) { > >> error_report(...) > >> } > >> > >> so we at least fail with a user friendly error rather than an abort? > >> > >> -- > >> Alex Bennée > >> > >> -- > >> You received this bug notification because you are subscribed to the bug > >> report. > >> https://bugs.launchpad.net/bugs/1895080 > >> > >> Title: > >> pgb_reserved_va: Assertion `addr == test' failed > >> > >> Status in QEMU: > >> New > >> > >> Bug description: > >> This problem occurs on CentOS-7.5 (64-bit) with qemu-5.1.0, qemu head > >> (commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a) for riscv32-linux- > >> user. > >> > >> Firstly, compile fails: > >> Compiling C object > libqemu-riscv32-linux-user.fa.p/linux-user_strace.c.o > >> ../qemu.git/linux-user/strace.c:1210:18: error: ‘FALLOC_FL_KEEP_SIZE’ > >> undeclared here (not in a function) > >>FLAG_GENERIC(FALLOC_FL_KEEP_SIZE), > >> > >> I have to add below include to linux-user/strace.c > >> diff --git a/linux-user/strace.c b/linux-user/strace.c > >> index 11fea14fba..22e51d4a8a 100644 > >> --- a/linux-user/strace.c > >> +++ b/linux-user/strace.c > >> @@ -7,6 +7,7 @@ > >>#include > >>#include > >>#include > >> +#include > >>#include > >>#include > >>#include > >> > >> Then trying qemu-riscv32 with a simple ELF, I get: > >> linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr == test' > >> failed. > >> > >> strace shows that: > >> mmap(0x1000, 4294963200, PROT_NONE, > >> MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x1 > >> write(2, "qemu-riscv32: ../qemu.git/linux-"..., 103qemu-riscv32: > >> ../qemu.git/linux-user/elfload.c:2341: pgb_reserved_va: Assertion `addr > == > >> test' failed. > >> ) = 103 > >> > >> The source code is in the function pgb_reserved_va (linux- > >> user/elfload.c). I think mmap cannot guarantee that the returned > >> pointer (test) equals to the parameter of addr. So is this a bug to > >> assert (addr == test)? > >> > >> Attached configure script and test ELF file. > >> > >> Thanks. > >> > >> To manage notifications about this bug go to: > >> https://bugs.launchpad.net/qemu/+bug/1895080/+subscriptions > >> > > > -- > Alex Bennée > > --
Re: [PATCH] hw/arm/pxa2xx: Add missing fallthrough comment
On 9/11/20 2:18 PM, Thomas Huth wrote: > Let's make this file compilable with -Werror=implicit-fallthrough : > Looking at the code, it seems like the fallthrough is intended here, > so we should add the corresponding "/* fallthrough */" comment here. > > Signed-off-by: Thomas Huth > --- > hw/arm/pxa2xx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c > index 76975d17a4..c1f45b2adf 100644 > --- a/hw/arm/pxa2xx.c > +++ b/hw/arm/pxa2xx.c > @@ -443,7 +443,7 @@ static void pxa2xx_mm_write(void *opaque, hwaddr addr, > s->mm_regs[addr >> 2] = value; > break; > } > - > +/* fallthrough */ > default: > qemu_log_mask(LOG_GUEST_ERROR, >"%s: Bad write offset 0x%"HWADDR_PRIx"\n", > I keep rebasing this one from Stefan, not sure why never upstreamed :/ https://repo.or.cz/qemu/ar7.git/blobdiff/2a9c928409..5ebb514633:/hw/arm/pxa2xx.c Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé
[PATCH 1/2] meson: fix MSI rule
From: Marc-André Lureau The environment variables can't be passed through an env: argument yet (meson#2723), use 'env' as suggested in: https://github.com/mesonbuild/meson/issues/2723#issuecomment-348630957 Signed-off-by: Marc-André Lureau --- qga/meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/qga/meson.build b/qga/meson.build index e5c5778a3e..c10a0526b2 100644 --- a/qga/meson.build +++ b/qga/meson.build @@ -70,6 +70,7 @@ if targetos == 'windows' output: 'qemu-ga-@0@.msi'.format(config_host['ARCH']), depends: deps, command: [ + find_program('env'), 'QEMU_GA_VERSION=' + config_host['QEMU_GA_VERSION'], 'QEMU_GA_MANUFACTURER=' + config_host['QEMU_GA_MANUFACTURER'], 'QEMU_GA_DISTRO=' + config_host['QEMU_GA_DISTRO'], -- 2.26.2
[PATCH 2/2] meson: error out if qemu_suffix starts with /
From: Marc-André Lureau Since the variable is used for path concatenation, the result would ignore the prefix directory altogether. Signed-off-by: Marc-André Lureau --- meson.build | 5 + 1 file changed, 5 insertions(+) diff --git a/meson.build b/meson.build index 5421eca66a..cbe1cb51b3 100644 --- a/meson.build +++ b/meson.build @@ -17,6 +17,11 @@ config_host = keyval.load(meson.current_build_dir() / 'config-host.mak') enable_modules = 'CONFIG_MODULES' in config_host enable_static = 'CONFIG_STATIC' in config_host build_docs = 'BUILD_DOCS' in config_host + +if get_option('qemu_suffix').startswith('/') + error('qemu_suffix cannot start with a /') +endif + qemu_datadir = get_option('datadir') / get_option('qemu_suffix') qemu_docdir = get_option('docdir') / get_option('qemu_suffix') config_host_data = configuration_data() -- 2.26.2
Re: [PATCH] cphp: remove deprecated cpu-add command(s)
What does cphp in the subject mean? On 11/09/2020 14.33, Igor Mammedov wrote: > theses were deprecatedince since 4.0, remove both HMP and QMP variants. deprecated since > Users should use device_add commnad instead. To get list of command > possible CPUs and options, use 'info hotpluggable-cpus' HMP > or query-hotpluggable-cpus QMP command. > > Signed-off-by: Igor Mammedov > --- > include/hw/boards.h | 1 - > include/hw/i386/pc.h| 1 - > include/monitor/hmp.h | 1 - > docs/system/deprecated.rst | 25 + > hmp-commands.hx | 15 -- > hw/core/machine-hmp-cmds.c | 12 - > hw/core/machine-qmp-cmds.c | 12 - > hw/i386/pc.c| 27 -- > hw/i386/pc_piix.c | 1 - > hw/s390x/s390-virtio-ccw.c | 12 - > qapi/machine.json | 24 - > tests/qtest/cpu-plug-test.c | 100 > tests/qtest/test-hmp.c | 1 - > 13 files changed, 21 insertions(+), 211 deletions(-) With the typos fixed: Reviewed-by: Thomas Huth
Re: [PATCH v1 2/3] tests/acpi: unit test for 'acpi-pci-hotplug-with-bridge-support' bridge flag
On Thu, 10 Sep 2020 22:34:20 +0530 Ani Sinha wrote: > On Sep 5, 2020, 16:05 +0530, Ani Sinha , wrote: > > This change adds a new unit test for the global flag > > 'acpi-pci-hotplug-with-bridge-support' which is available for cold plugged > > pci > > bridges in i440fx. The flag can be used to turn off acpi based hotplug > > support > > for all the slots of the pci bus. > > > > Tested on the upstream qemu master branch on top of tag v5.1.0 > > Can someone please review this? Hi, Are there other patches of yours, that should be applied/reviewed before this one? > > > > Signed-off-by: Ani Sinha > > > > > > --- > > tests/qtest/bios-tables-test.c | 15 +++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > > index a2c0070306..e52a36e775 100644 > > --- a/tests/qtest/bios-tables-test.c > > +++ b/tests/qtest/bios-tables-test.c > > @@ -723,6 +723,20 @@ static void test_acpi_piix4_root_hotplug(void) > > free_test_data(&data); > > } > > > > +static void test_acpi_piix4_bridge_hotplug(void) > > +{ > > + test_data data; > > + > > + memset(&data, 0, sizeof(data)); > > + data.machine = MACHINE_PC; > > + data.variant = ".hpbridge"; > > + data.required_struct_types = base_required_struct_types; > > + data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); > > + test_acpi_one("-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off " > > + "-device pci-bridge,chassis_nr=1", &data); > > + free_test_data(&data); > > +} > > + > > static void test_acpi_q35_tcg(void) > > { > > test_data data; > > @@ -1117,6 +1131,7 @@ int main(int argc, char *argv[]) > > qtest_add_func("acpi/piix4", test_acpi_piix4_tcg); > > qtest_add_func("acpi/piix4/bridge", test_acpi_piix4_tcg_bridge); > > qtest_add_func("acpi/piix4/hotplug", test_acpi_piix4_root_hotplug); > > + qtest_add_func("acpi/piix4/brhotplug", test_acpi_piix4_bridge_hotplug); > > qtest_add_func("acpi/q35", test_acpi_q35_tcg); > > qtest_add_func("acpi/q35/bridge", test_acpi_q35_tcg_bridge); > > qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); > > -- > > 2.17.1 > >
Re: [for-5.2 v4 10/10] s390: Recognize host-trust-limitation option
On Fri, 11 Sep 2020 10:07:18 +1000 David Gibson wrote: > On Thu, Sep 10, 2020 at 08:29:24PM +0200, Halil Pasic wrote: > > On Thu, 10 Sep 2020 13:36:09 +0200 > > Cornelia Huck wrote: > > > > > On Mon, 7 Sep 2020 17:22:53 +0200 > > > Halil Pasic wrote: > > > > > > > On Fri, 24 Jul 2020 12:57:44 +1000 > > > > David Gibson wrote: > > > > > > > > > At least some s390 cpu models support "Protected Virtualization" (PV), > > > > > a mechanism to protect guests from eavesdropping by a compromised > > > > > hypervisor. > > > > > > > > > > This is similar in function to other mechanisms like AMD's SEV and > > > > > POWER's PEF, which are controlled bythe "host-trust-limitation" > > > > > machine option. s390 is a slightly special case, because we already > > > > > supported PV, simply by using a CPU model with the required feature > > > > > (S390_FEAT_UNPACK). > > > > > > > > > > To integrate this with the option used by other platforms, we > > > > > implement the following compromise: > > > > > > > > > > - When the host-trust-limitation option is set, s390 will recognize > > > > >it, verify that the CPU can support PV (failing if not) and set > > > > >virtio default options necessary for encrypted or protected guests, > > > > >as on other platforms. i.e. if host-trust-limitation is set, we > > > > >will either create a guest capable of entering PV mode, or fail > > > > >outright > > > > > > > > Shouldn't we also fail outright if the virtio features are not PV > > > > compatible (invalid configuration)? > > > > > > > > I would like to see something like follows as a part of this series. > > > > 8<-- > > > > From: Halil Pasic > > > > Date: Mon, 7 Sep 2020 15:00:17 +0200 > > > > Subject: [PATCH] virtio: handle host trust limitation > > > > > > > > If host_trust_limitation_enabled() returns true, then emulated virtio > > > > devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not > > > > capable of accessing all of the guest memory. Otherwise we are in > > > > violation of the virtio specification. > > > > > > > > Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is > > > > obligatory but missing. > > > > > > > > Signed-off-by: Halil Pasic > > > > --- > > > > hw/virtio/virtio.c | 7 +++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > > index 5bd2a2f621..19b4b0a37a 100644 > > > > --- a/hw/virtio/virtio.c > > > > +++ b/hw/virtio/virtio.c > > > > @@ -27,6 +27,7 @@ > > > > #include "hw/virtio/virtio-access.h" > > > > #include "sysemu/dma.h" > > > > #include "sysemu/runstate.h" > > > > +#include "exec/host-trust-limitation.h" > > > > > > > > /* > > > > * The alignment to use between consumer and producer parts of vring. > > > > @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState > > > > *dev, Error **errp) > > > > /* Devices should either use vmsd or the load/save methods */ > > > > assert(!vdc->vmsd || !vdc->load); > > > > > > > > +if (host_trust_limitation_enabled(MACHINE(qdev_get_machine())) > > > > +&& !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { > > > > +error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are > > > > not compatible with host trust imitation"); > > > > +error_propagate(errp, err); > > > > +return; > > > > > > How can we get here? I assume only if the user explicitly turned the > > > feature off while turning HTL on, as otherwise patch 9 should have > > > taken care of it? > > > > > > > Yes, we can get here only if iommu_platform is explicitly turned off. > > Right.. my assumption was that if you really want to specify > contradictory options, you get to keep both pieces. Or, more > seriously, there might be some weird experimental cases where this > combination could do something useful if you really know what you're > doing, and explicitly telling qemu to do this implies you know what > you're doing. > According to Michael, the correctness of a hypervisor is depending on this (if device has restricted access to guest memory, but does not present F_ACCESS_PLATFORM then the hypervisor is buggy). Also a hotplug of such a misconfigured device is at the moment likely bring down the guest on s390x. The only scenario in which the guest has protected memory and the device is able to access it, is a trusted HW device. But then we would need F_ACCESS_PLATFORM because it is a HW device. So I consider this combination doing something useful very unlikely. Does anybody have a scenario where this combination is legit and useful? If such a scenario emerges later, I think the check can be refined or dropped. Regards, Halil pgpfY85DbMlX2.pgp Description: OpenPGP digital signature
Re: [PATCH v9 11/14] hw/arm: Wire up BMC boot flash for npcm750-evb and quanta-gsj
On 9/11/20 7:20 AM, Havard Skinnemoen via wrote: > This allows these NPCM7xx-based boards to boot from a flash image, e.g. > one built with OpenBMC. For example like this: > > IMAGE=${OPENBMC}/build/tmp/deploy/images/gsj/image-bmc > qemu-system-arm -machine quanta-gsj -nographic \ > -drive file=${IMAGE},if=mtd,bus=0,unit=0,format=raw,snapshot=on > > Reviewed-by: Tyrone Ting > Reviewed-by: Cédric Le Goater > Tested-by: Cédric Le Goater > Tested-by: Philippe Mathieu-Daudé > Signed-off-by: Havard Skinnemoen > --- > hw/arm/npcm7xx_boards.c | 20 > 1 file changed, 20 insertions(+) > > diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c > index b4c772d6b5..79e2e2744c 100644 > --- a/hw/arm/npcm7xx_boards.c > +++ b/hw/arm/npcm7xx_boards.c > @@ -20,6 +20,7 @@ > #include "hw/arm/npcm7xx.h" > #include "hw/core/cpu.h" > #include "hw/loader.h" > +#include "hw/qdev-properties.h" > #include "qapi/error.h" > #include "qemu-common.h" > #include "qemu/units.h" > @@ -55,6 +56,22 @@ static void npcm7xx_load_bootrom(MachineState *machine, > NPCM7xxState *soc) > } > } > > +static void npcm7xx_connect_flash(NPCM7xxFIUState *fiu, int cs_no, > + const char *flash_type, DriveInfo *dinfo) > +{ > +DeviceState *flash; > +qemu_irq flash_cs; > + > +flash = qdev_new(flash_type); > +if (dinfo) { > +qdev_prop_set_drive(flash, "drive", blk_by_legacy_dinfo(dinfo)); > +} > +qdev_realize_and_unref(flash, BUS(fiu->spi), &error_fatal); > + > +flash_cs = qdev_get_gpio_in_named(flash, SSI_GPIO_CS, 0); > +qdev_connect_gpio_out_named(DEVICE(fiu), "cs", cs_no, flash_cs); > +} > + > static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram) > { > memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram); > @@ -92,6 +109,7 @@ static void npcm750_evb_init(MachineState *machine) > qdev_realize(DEVICE(soc), NULL, &error_fatal); > > npcm7xx_load_bootrom(machine, soc); > +npcm7xx_connect_flash(&soc->fiu[0], 0, "w25q256", drive_get(IF_MTD, 0, > 0)); Nitpicking: add definition for '0' magic number (consider as future cleanup). > npcm7xx_load_kernel(machine, soc); > } > > @@ -104,6 +122,8 @@ static void quanta_gsj_init(MachineState *machine) > qdev_realize(DEVICE(soc), NULL, &error_fatal); > > npcm7xx_load_bootrom(machine, soc); > +npcm7xx_connect_flash(&soc->fiu[0], 0, "mx25l25635e", > + drive_get(IF_MTD, 0, 0)); Reviewed-by: Philippe Mathieu-Daudé > npcm7xx_load_kernel(machine, soc); > } > >
Re: [PATCH v9 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
On 9/11/20 7:20 AM, Havard Skinnemoen via wrote: > This should be fully reviewed now, but the Timer patch may deserve another > look, as I fixed a few bugs in it. Huge thanks to everyone who reviewed and/or > tested this patchset, it has clearly improved a lot since I started. Yes, all patches reviewed. Very good quality. I also learned while reviewing your patches, so thanks for your contribution :) > > I also pushed this and the previous patchsets to my qemu fork on github. The > branches are named npcm7xx-v[1-9]. > > https://github.com/hskinnemoen/qemu > > This patch series models enough of the Nuvoton NPCM730 and NPCM750 SoCs to > boot > an OpenBMC image built for quanta-gsj. This includes device models for: > > - Global Configuration Registers > - Clock Control > - Timers > - Fuses > - Memory Controller > - Flash Controller
Re: [PATCH] hw/arm/pxa2xx: Add missing fallthrough comment
On 11/09/2020 14.41, Philippe Mathieu-Daudé wrote: > On 9/11/20 2:18 PM, Thomas Huth wrote: >> Let's make this file compilable with -Werror=implicit-fallthrough : >> Looking at the code, it seems like the fallthrough is intended here, >> so we should add the corresponding "/* fallthrough */" comment here. >> >> Signed-off-by: Thomas Huth >> --- >> hw/arm/pxa2xx.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c >> index 76975d17a4..c1f45b2adf 100644 >> --- a/hw/arm/pxa2xx.c >> +++ b/hw/arm/pxa2xx.c >> @@ -443,7 +443,7 @@ static void pxa2xx_mm_write(void *opaque, hwaddr addr, >> s->mm_regs[addr >> 2] = value; >> break; >> } >> - >> +/* fallthrough */ >> default: >> qemu_log_mask(LOG_GUEST_ERROR, >>"%s: Bad write offset 0x%"HWADDR_PRIx"\n", >> > > I keep rebasing this one from Stefan, not sure why never upstreamed :/ > https://repo.or.cz/qemu/ar7.git/blobdiff/2a9c928409..5ebb514633:/hw/arm/pxa2xx.c Are there more patches like this in that tree? If so, could you maybe send them upstream? > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé Thanks! Thomas
Re: [PATCH] hw/arm/pxa2xx: Add missing fallthrough comment
Le 11/09/2020 à 14:41, Philippe Mathieu-Daudé a écrit : > On 9/11/20 2:18 PM, Thomas Huth wrote: >> Let's make this file compilable with -Werror=implicit-fallthrough : >> Looking at the code, it seems like the fallthrough is intended here, >> so we should add the corresponding "/* fallthrough */" comment here. >> >> Signed-off-by: Thomas Huth >> --- >> hw/arm/pxa2xx.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c >> index 76975d17a4..c1f45b2adf 100644 >> --- a/hw/arm/pxa2xx.c >> +++ b/hw/arm/pxa2xx.c >> @@ -443,7 +443,7 @@ static void pxa2xx_mm_write(void *opaque, hwaddr addr, >> s->mm_regs[addr >> 2] = value; >> break; >> } >> - >> +/* fallthrough */ >> default: >> qemu_log_mask(LOG_GUEST_ERROR, >>"%s: Bad write offset 0x%"HWADDR_PRIx"\n", >> > > I keep rebasing this one from Stefan, not sure why never upstreamed :/ > https://repo.or.cz/qemu/ar7.git/blobdiff/2a9c928409..5ebb514633:/hw/arm/pxa2xx.c Has it been sent to qemu-trivial? I don't find it in my folder. > > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé > > Applied to my trivial-patches branch. Thanks, Laurent
[PATCH] target/i386/kvm: Add missing fallthrough comment
Let's make this file compilable with -Werror=implicit-fallthrough : Looking at the code, it seems like the fallthrough is intended here, so we should add the corresponding "/* fallthrough */" comment here. Signed-off-by: Thomas Huth --- target/i386/kvm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 205b68bc0c..283d3305d5 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -1568,6 +1568,7 @@ int kvm_arch_init_vcpu(CPUState *cs) if (env->nr_dies < 2) { break; } +/* fallthrough */ case 4: case 0xb: case 0xd: -- 2.18.2
Re: VHOST_USER_PROTOCOL_F_VDPA
On Thu, Sep 10, 2020 at 10:55:00AM +0200, Maxime Coquelin wrote: > I understand the need and like the idea. If no one plans to work on this within the next 6 months it could be an internship project since the Outreachy December-March round is coming up. The time requirement is typically ~5 hours per week from the mentor. Jason, Maxime, Cindy, Michael: Are you interested in mentoring this project idea? Internships focus mostly implementing existing designs since the intern doesn't have enough background knowledge or time to design some of this complexity from scratch. As a starting point, the vhost-user protocol can be extended with all the vDPA messages. Redundant vhost-user messages from the existing protocol can be dropped/deprecated (i.e. device-specific messages that were introduced to work around the lack of the VIRTIO device lifecycle). Fully embracing the vDPA/VIRTIO lifecycle eliminates the need for per-type QEMU devices (vhost-user-net, vhost-user-blk, etc). Instead per-transport devices are needed (-device vdpa-user-{pci,ccw,mmio}). This means new device types can be added later without writing all the boilerplate -device vhost-user-foo-{pci,ccw,mmio} code! The internship could focus on implementing -device vdpa-user-pci and a libvdpa-user test/example. The vhost-user spec changes should probably be agreed by the community beforehand so the intern doesn't need to worry about the protocol design (there's plenty of implementation work to do!). This is one way to break this down into an internship project, but there are other approaches. Maybe you prefer to extend vhost-user more gradually (e.g. no -device vdpa-user-pci and the existing per-type device approach is continued) while still adding the vDPA protocol messages... Anyway, if you'd like to mentor this project in Outreachy December-March please add a project idea to the QEMU wiki: https://wiki.qemu.org/Outreachy_2020_DecemberMarch Stefan signature.asc Description: PGP signature
Re: [PATCH] PoC: Rust binding for QAPI (qemu-ga only, for now)
On 11/09/20 12:24, Paolo Bonzini wrote: >> >> - from_qemu_none(ptr: *const sys::P) -> T >> Return a Rust type T for a const ffi pointer P. >> >> - from_qemu_full(ptr: *mut sys::P) -> T >> Return a Rust type T for a ffi pointer P, taking ownership. >> >> - T::to_qemu_none() -> Stash >> Returns a borrowed ffi pointer P (using a Stash to destroy "glue" >> storage data, if any). >> >> - T::to_qemu_full() -> P >> Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) > > I know these come from glib-rs, but still the names are awful. :) After studying a bit I managed to give a rational explanation of the above gut feeling. Conversions in Rust usually follow this naming convention: Name Type Price --- as_*, borrow Borrowed -> Borrowed Cheap to_*, constructor Borrowed -> Owned Expensive from, into_* Owned -> OwnedAny and we have from_qemu_noneBorrowed -> Owned to_qemu_none Borrowed -> Borrowed from_qemu_fullOwned -> Owned to_qemu_full Owned -> Owned So - from_qemu_none should be a "to_*" or "constructor" conversion (I used new_from_foreign) - to_qemu_none, even though in some cases it can be expensive, should be an "as_*" conversion (as_foreign). - from_qemu_full and to_qemu_full should be "from" or "into_*" to_qemu_full() could also be split into a more expensive but flexible "to" variant and the cheaper "into" variant. Just for the sake of example, I updated my demo by replacing the IntoRaw trait with these two: trait ToForeign { fn to_foreign(&self) -> *mut T; } trait IntoForeign { fn into_foreign(self) -> *mut T; } where the example implementations show the different cost quite clearly: impl ToForeign for String { fn to_foreign(&self) -> *mut c_char { unsafe { libc::strndup(self.as_ptr() as *const c_char, self.len() as size_t) } } } impl IntoForeign for String { fn into_foreign(self) -> *mut c_char { let ptr = self.as_ptr(); forget(self); ptr as *mut _ } } Thanks, Paolo
Re: [PATCH v1 2/3] tests/acpi: unit test for 'acpi-pci-hotplug-with-bridge-support' bridge flag
On Sep 11, 2020, 18:15 +0530, Igor Mammedov , wrote: > On Thu, 10 Sep 2020 22:34:20 +0530 > Ani Sinha wrote: > > > On Sep 5, 2020, 16:05 +0530, Ani Sinha , wrote: > > > This change adds a new unit test for the global flag > > > 'acpi-pci-hotplug-with-bridge-support' which is available for cold > > > plugged pci > > > bridges in i440fx. The flag can be used to turn off acpi based hotplug > > > support > > > for all the slots of the pci bus. > > > > > > Tested on the upstream qemu master branch on top of tag v5.1.0 > > > > Can someone please review this? > Hi, > Are there other patches of yours, > that should be applied/reviewed before this one? Yes please see “i440fx/acpi: Do not add hotplug related amls for cold plugged bridges” > > > > > > Signed-off-by: Ani Sinha > > > > > > > > > --- > > > tests/qtest/bios-tables-test.c | 15 +++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/tests/qtest/bios-tables-test.c > > > b/tests/qtest/bios-tables-test.c > > > index a2c0070306..e52a36e775 100644 > > > --- a/tests/qtest/bios-tables-test.c > > > +++ b/tests/qtest/bios-tables-test.c > > > @@ -723,6 +723,20 @@ static void test_acpi_piix4_root_hotplug(void) > > > free_test_data(&data); > > > } > > > > > > +static void test_acpi_piix4_bridge_hotplug(void) > > > +{ > > > + test_data data; > > > + > > > + memset(&data, 0, sizeof(data)); > > > + data.machine = MACHINE_PC; > > > + data.variant = ".hpbridge"; > > > + data.required_struct_types = base_required_struct_types; > > > + data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); > > > + test_acpi_one("-global > > > PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off " > > > + "-device pci-bridge,chassis_nr=1", &data); > > > + free_test_data(&data); > > > +} > > > + > > > static void test_acpi_q35_tcg(void) > > > { > > > test_data data; > > > @@ -1117,6 +1131,7 @@ int main(int argc, char *argv[]) > > > qtest_add_func("acpi/piix4", test_acpi_piix4_tcg); > > > qtest_add_func("acpi/piix4/bridge", test_acpi_piix4_tcg_bridge); > > > qtest_add_func("acpi/piix4/hotplug", test_acpi_piix4_root_hotplug); > > > + qtest_add_func("acpi/piix4/brhotplug", test_acpi_piix4_bridge_hotplug); > > > qtest_add_func("acpi/q35", test_acpi_q35_tcg); > > > qtest_add_func("acpi/q35/bridge", test_acpi_q35_tcg_bridge); > > > qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64); > > > -- > > > 2.17.1 > > > >
[PATCH] smp: drop support for deprecated (invalid topologies)
it's was deprecated since 3.1 Support for invalid topologies is removed, the user must ensure that topologies described with -smp include all possible cpus, i.e. (sockets * cores * threads) == maxcpus or QEMU will exit with error. Signed-off-by: Igor Mammedov --- docs/system/deprecated.rst | 26 +- hw/core/machine.c | 16 hw/i386/pc.c | 16 3 files changed, 21 insertions(+), 37 deletions(-) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 122717cfee..d737728fab 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -47,19 +47,6 @@ The 'file' driver for drives is no longer appropriate for character or host devices and will only accept regular files (S_IFREG). The correct driver for these file types is 'host_cdrom' or 'host_device' as appropriate. -``-smp`` (invalid topologies) (since 3.1) -' - -CPU topology properties should describe whole machine topology including -possible CPUs. - -However, historically it was possible to start QEMU with an incorrect topology -where *n* <= *sockets* * *cores* * *threads* < *maxcpus*, -which could lead to an incorrect topology enumeration by the guest. -Support for invalid topologies will be removed, the user must ensure -topologies described with -smp include all possible cpus, i.e. -*sockets* * *cores* * *threads* = *maxcpus*. - ``-vnc acl`` (since 4.0.0) '' @@ -618,6 +605,19 @@ New machine versions (since 5.1) will not accept the option but it will still work with old machine types. User can check the QAPI schema to see if the legacy option is supported by looking at MachineInfo::numa-mem-supported property. +``-smp`` (invalid topologies) (removed 5.2) +''' + +CPU topology properties should describe whole machine topology including +possible CPUs. + +However, historically it was possible to start QEMU with an incorrect topology +where *n* <= *sockets* * *cores* * *threads* < *maxcpus*, +which could lead to an incorrect topology enumeration by the guest. +Support for invalid topologies is removed, the user must ensure +topologies described with -smp include all possible cpus, i.e. +*sockets* * *cores* * *threads* = *maxcpus*. + Block devices - diff --git a/hw/core/machine.c b/hw/core/machine.c index ea26d61237..09aee4ea52 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -754,23 +754,15 @@ static void smp_parse(MachineState *ms, QemuOpts *opts) exit(1); } -if (sockets * cores * threads > ms->smp.max_cpus) { -error_report("cpu topology: " - "sockets (%u) * cores (%u) * threads (%u) > " - "maxcpus (%u)", +if (sockets * cores * threads != ms->smp.max_cpus) { +error_report("Invalid CPU topology: " + "sockets (%u) * cores (%u) * threads (%u) " + "!= maxcpus (%u)", sockets, cores, threads, ms->smp.max_cpus); exit(1); } -if (sockets * cores * threads != ms->smp.max_cpus) { -warn_report("Invalid CPU topology deprecated: " -"sockets (%u) * cores (%u) * threads (%u) " -"!= maxcpus (%u)", -sockets, cores, threads, -ms->smp.max_cpus); -} - ms->smp.cpus = cpus; ms->smp.cores = cores; ms->smp.threads = threads; diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d071da787b..fbde6b04e6 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -746,23 +746,15 @@ void pc_smp_parse(MachineState *ms, QemuOpts *opts) exit(1); } -if (sockets * dies * cores * threads > ms->smp.max_cpus) { -error_report("cpu topology: " - "sockets (%u) * dies (%u) * cores (%u) * threads (%u) > " - "maxcpus (%u)", +if (sockets * dies * cores * threads != ms->smp.max_cpus) { +error_report("Invalid CPU topology deprecated: " + "sockets (%u) * dies (%u) * cores (%u) * threads (%u) " + "!= maxcpus (%u)", sockets, dies, cores, threads, ms->smp.max_cpus); exit(1); } -if (sockets * dies * cores * threads != ms->smp.max_cpus) { -warn_report("Invalid CPU topology deprecated: " -"sockets (%u) * dies (%u) * cores (%u) * threads (%u) " -"!= maxcpus (%u)", -sockets, dies, cores, threads, -ms->smp.max_cpus); -} - ms->smp.cpus = cpus; ms->smp.cores = cores; ms->smp.threads = threads; -- 2.27.0
Re: [PATCH v5 6/8] s390/sclp: add extended-length sccb support for kvm guest
On 10/09/2020 11.36, Collin Walling wrote: > As more features and facilities are added to the Read SCP Info (RSCPI) > response, more space is required to store them. The space used to store > these new features intrudes on the space originally used to store CPU > entries. This means as more features and facilities are added to the > RSCPI response, less space can be used to store CPU entries. > > With the Extended-Length SCCB (ELS) facility, a KVM guest can execute > the RSCPI command and determine if the SCCB is large enough to store a > complete reponse. If it is not large enough, then the required length > will be set in the SCCB header. > > The caller of the SCLP command is responsible for creating a > large-enough SCCB to store a complete response. Proper checking should > be in place, and the caller should execute the command once-more with > the large-enough SCCB. > > This facility also enables an extended SCCB for the Read CPU Info > (RCPUI) command. > > When this facility is enabled, the boundary violation response cannot > be a result from the RSCPI, RSCPI Forced, or RCPUI commands. > > In order to tolerate kernels that do not yet have full support for this > feature, a "fixed" offset to the start of the CPU Entries within the > Read SCP Info struct is set to allow for the original 248 max entries > when this feature is disabled. > > Additionally, this is introduced as a CPU feature to protect the guest > from migrating to a machine that does not support storing an extended > SCCB. This could otherwise hinder the VM from being able to read all > available CPU entries after migration (such as during re-ipl). > > Signed-off-by: Collin Walling > --- [...] > /* Provide information about the configuration, CPUs and storage */ > static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > { > @@ -89,10 +112,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > int rnsize, rnmax; > IplParameterBlock *ipib = s390_ipl_get_iplb(); > int required_len = SCCB_REQ_LEN(ReadInfo, machine->possible_cpus->len); > -int offset_cpu = offsetof(ReadInfo, entries); > +int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ? > + offsetof(ReadInfo, entries) : > + SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET; Sorry, but I'm having somewhat trouble to understand this... What's the difference between offsetof(ReadInfo, entries) and SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET ? Aren't both terms resulting in the value 128 ? Thomas
[PATCH] tests: add missing genh dependency
Fix high-parallelism builds by forcing all generated headers to be created before tests are compiled. Reported-by: Claudio Fontana Signed-off-by: Paolo Bonzini --- tests/meson.build | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/meson.build b/tests/meson.build index 3683512dca..e2b915ea8f 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -56,7 +56,7 @@ test_qapi_files = custom_target('Test QAPI files', # perhaps change qapi_gen to replace / with _, like Meson itself does? subdir('include') -libtestqapi = static_library('testqapi', sources: [test_qapi_files, test_qapi_outputs_extra]) +libtestqapi = static_library('testqapi', sources: [test_qapi_files, genh, test_qapi_outputs_extra]) testqapi = declare_dependency(link_with: libtestqapi) testblock = declare_dependency(dependencies: [block], sources: 'iothread.c') @@ -223,7 +223,7 @@ foreach test_name, extra: tests src += test_ss.all_sources() deps += test_ss.all_dependencies() endif - exe = executable(test_name, src, dependencies: deps) + exe = executable(test_name, src, genh, dependencies: deps) test(test_name, exe, depends: test_deps.get(test_name, []), -- 2.26.2