RE: [PATCH 1/3] colo-compare: return -1 if no packet is queued
> -Original Message-
> From: Li Zhijian
> Sent: Wednesday, September 23, 2020 2:18 PM
> To: Zhang, Chen ; jasow...@redhat.com
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH 1/3] colo-compare: return -1 if no packet is queued
>
>
>
> On 9/23/20 9:41 AM, Zhang, Chen wrote:
> >
> >> -Original Message-
> >> From: Li Zhijian
> >> Sent: Tuesday, September 22, 2020 5:55 PM
> >> To: Zhang, Chen ; jasow...@redhat.com
> >> Cc: qemu-devel@nongnu.org; Li Zhijian
> >> Subject: [PATCH 1/3] colo-compare: return -1 if no packet is queued
> >>
> >> Return 0 will trigger a packet comparison
> >>
> > Yes, we need active trigger a checkpoint to flush all the queued packets
> here.
> Previously, no new checkpoint will be triggered since no new packet is
> queued though colo_compare_connection() is called.
> actually we should send a notify to colo frame immediately, no need to
> compare them any more in order to less latency.
Yes, you are right. We can change this patch to directly send notify here.
Thanks
Zhang Chen
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 3a45d64175..23092e4496 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -285,10 +285,13 @@ static int packet_enqueue(CompareState *s, int
> mode, Connection **con)
> }
>
> if (!ret) {
> + /* queue is too long, do a checkpoint to release all queued
> +packets */
> + colo_compare_inconsistency_notify(s);
> trace_colo_compare_drop_packet(colo_mode[mode],
> "queue size too big, drop packet");
> packet_destroy(pkt, NULL);
> pkt = NULL;
> + return -1;
> }
>
> *con = conn;
>
>
> > Otherwise, we should drop all the packet after this time still next
> checkpoint.
> > So, I think original logic is a better choice.
> >
> > Thanks
> > Zhang Chen
> >
> >> Signed-off-by: Li Zhijian
> >> ---
> >> net/colo-compare.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> >> 3a45d64175..039b515611 100644
> >> --- a/net/colo-compare.c
> >> +++ b/net/colo-compare.c
> >> @@ -289,6 +289,7 @@ static int packet_enqueue(CompareState *s, int
> >> mode, Connection **con)
> >> "queue size too big, drop packet");
> >> packet_destroy(pkt, NULL);
> >> pkt = NULL;
> >> +return -1;
> >> }
> >>
> >> *con = conn;
> >> --
> >> 2.28.0
> >>
> >>
> >
> >
>
>
RE: [PATCH 3/4] net/colo-compare.c: Add secondary old packet detection
> -Original Message-
> From: Li Zhijian
> Sent: Tuesday, September 22, 2020 2:47 PM
> To: Zhang, Chen ; Jason Wang
> ; qemu-dev
> Cc: Zhang Chen
> Subject: Re: [PATCH 3/4] net/colo-compare.c: Add secondary old packet
> detection
>
>
>
> On 9/18/20 5:22 PM, Zhang Chen wrote:
> > From: Zhang Chen
> >
> > Detect queued secondary packet to sync VM state in time.
> >
> > Signed-off-by: Zhang Chen
> > ---
> > net/colo-compare.c | 25 -
> > 1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 3b72309d08..f7271b976f 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -641,19 +641,26 @@ void colo_compare_unregister_notifier(Notifier
> *notify)
> > static int colo_old_packet_check_one_conn(Connection *conn,
> > CompareState *s)
> > {
> > -GList *result = NULL;
> > -
> > -result = g_queue_find_custom(&conn->primary_list,
> > - &s->compare_timeout,
> > - (GCompareFunc)colo_old_packet_check_one);
> > +if (!g_queue_is_empty(&conn->primary_list)) {
> Looks we don't need to check is_empty
Re-checked glib code, it just checked the queue rather than inside content.
Maybe check empty before that will benefit performance.
Thanks
Zhang Chen
>
> > +if (g_queue_find_custom(&conn->primary_list,
> > +&s->compare_timeout,
> > +(GCompareFunc)colo_old_packet_check_one))
> > +goto out;
> > +}
> >
> > -if (result) {
> > -/* Do checkpoint will flush old packet */
> > -colo_compare_inconsistency_notify(s);
> > -return 0;
> > +if (!g_queue_is_empty(&conn->secondary_list)) {
> Ditto
>
> Thanks
> > +if (g_queue_find_custom(&conn->secondary_list,
> > +&s->compare_timeout,
> > +(GCompareFunc)colo_old_packet_check_one))
> > +goto out;
> > }
> >
> > return 1;
> > +
> > +out:
> > +/* Do checkpoint will flush old packet */
> > +colo_compare_inconsistency_notify(s);
> > +return 0;
> > }
> >
> > /*
>
>
Re: [PATCH v26 03/17] vfio: Add save and load functions for VFIO PCI devices
Hi Kirti,
A few trivial comments from the first read through.
On 2020/9/23 7:24, Kirti Wankhede wrote:
These functions save and restore PCI device specific data - config
space of PCI device.
Used VMStateDescription to save and restore interrupt state.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
hw/vfio/pci.c | 134 ++
hw/vfio/pci.h | 1 +
include/hw/vfio/vfio-common.h | 2 +
3 files changed, 137 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bffd5bfe3b78..9968cc553391 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -41,6 +41,7 @@
#include "trace.h"
#include "qapi/error.h"
#include "migration/blocker.h"
+#include "migration/qemu-file.h"
#define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
@@ -2401,11 +2402,142 @@ static Object *vfio_pci_get_object(VFIODevice *vbasedev)
return OBJECT(vdev);
}
+static int vfio_get_pci_irq_state(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field)
+{
+VFIOPCIDevice *vdev = container_of(pv, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+uint32_t interrupt_type;
+
+interrupt_type = qemu_get_be32(f);
+
+if (interrupt_type == VFIO_INT_MSI) {
+uint32_t msi_flags, msi_addr_lo, msi_addr_hi = 0, msi_data;
+bool msi_64bit;
+
+/* restore msi configuration */
+msi_flags = pci_default_read_config(pdev,
+pdev->msi_cap + PCI_MSI_FLAGS, 2);
+msi_64bit = (msi_flags & PCI_MSI_FLAGS_64BIT);
+
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
+ msi_flags & ~PCI_MSI_FLAGS_ENABLE, 2);
+
+msi_addr_lo = pci_default_read_config(pdev,
+pdev->msi_cap + PCI_MSI_ADDRESS_LO, 4);
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_ADDRESS_LO,
+ msi_addr_lo, 4);
+
+if (msi_64bit) {
+msi_addr_hi = pci_default_read_config(pdev,
+pdev->msi_cap + PCI_MSI_ADDRESS_HI, 4);
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_ADDRESS_HI,
+ msi_addr_hi, 4);
+}
+
+msi_data = pci_default_read_config(pdev,
+pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
PCI_MSI_DATA_32),
+2);
+
+vfio_pci_write_config(pdev,
+pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
PCI_MSI_DATA_32),
+msi_data, 2);
+
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
+ msi_flags | PCI_MSI_FLAGS_ENABLE, 2);
+} else if (interrupt_type == VFIO_INT_MSIX) {
+uint16_t offset;
Maybe rename it to 'control' to match the PCI term?
+
+msix_load(pdev, f);
+offset = pci_default_read_config(pdev,
+ pdev->msix_cap + PCI_MSIX_FLAGS + 1, 2);
+/* load enable bit and maskall bit */
+vfio_pci_write_config(pdev, pdev->msix_cap + PCI_MSIX_FLAGS + 1,
+ offset, 2);
+}
+return 0;
+}
+
+static int vfio_put_pci_irq_state(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, QJSON *vmdesc)
+{
+VFIOPCIDevice *vdev = container_of(pv, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+
+qemu_put_be32(f, vdev->interrupt);
+if (vdev->interrupt == VFIO_INT_MSIX) {
+msix_save(pdev, f);
+}
+
+return 0;
+}
+
+static const VMStateInfo vmstate_info_vfio_pci_irq_state = {
+.name = "VFIO PCI irq state",
+.get = vfio_get_pci_irq_state,
+.put = vfio_put_pci_irq_state,
+};
+
+const VMStateDescription vmstate_vfio_pci_config = {
+.name = "VFIOPCIDevice",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_INT32_POSITIVE_LE(version_id, VFIOPCIDevice),
+VMSTATE_BUFFER_UNSAFE_INFO(interrupt, VFIOPCIDevice, 1,
+ vmstate_info_vfio_pci_irq_state,
+ sizeof(int32_t)),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
+{
+VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+
+
Two blank lines.
+pci_device_save(pdev, f);
+vmstate_save_state(f, &vmstate_vfio_pci_config, vbasedev, NULL);
+}
+
+static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
+{
+VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+uint16_t pci_cmd;
+int ret, i;
+
+ret = pci_device_load(pdev, f);
+if (ret) {
+return ret;
+}
+
+/* retore pci bar configuration */
+pci_cmd = pci_default_read_confi
Re: [PATCH 8/9] audio: restore mixing-engine playback buffer size
> diff --git a/audio/sdlaudio.c b/audio/sdlaudio.c > index 21b7a0484b..cb931d0fda 100644 > --- a/audio/sdlaudio.c > +++ b/audio/sdlaudio.c > @@ -253,6 +253,7 @@ static void sdl_callback (void *opaque, Uint8 *buf, int > len) > return ret; \ > } > > +SDL_WRAPPER_FUNC(buffer_get_free, size_t, (HWVoiceOut *hw), (hw)) > SDL_WRAPPER_FUNC(get_buffer_out, void *, (HWVoiceOut *hw, size_t *size), > (hw, size), *size = 0, sdl_unlock) > SDL_WRAPPER_FUNC(put_buffer_out, size_t, Compiling C object libcommon.fa.p/audio_sdlaudio.c.o ../../audio/sdlaudio.c:256:65: error: macro "SDL_WRAPPER_FUNC" requires 6 arguments, but only 4 given 256 | SDL_WRAPPER_FUNC(buffer_get_free, size_t, (HWVoiceOut *hw), (hw)) | ^ ../../audio/sdlaudio.c:243: note: macro "SDL_WRAPPER_FUNC" defined here 243 | #define SDL_WRAPPER_FUNC(name, ret_type, args_decl, args, fail, unlock) \ | ../../audio/sdlaudio.c:256:17: error: expected ‘;’ before ‘static’ 256 | SDL_WRAPPER_FUNC(buffer_get_free, size_t, (HWVoiceOut *hw), (hw)) | ^ | ; ../../audio/sdlaudio.c:355:24: error: ‘sdl_buffer_get_free’ undeclared here (not in a function) 355 | .buffer_get_free = sdl_buffer_get_free, |^~~ make: *** [Makefile.ninja:1113: libcommon.fa.p/audio_sdlaudio.c.o] Error 1 (I think coreaudio has the same problem ...). take care, Gerd
Re: [PATCH v5 13/15] docs: convert replay.txt to rst
On 22.09.2020 16:13, Paolo Bonzini wrote: On 22/09/20 14:16, Pavel Dovgalyuk wrote: + +When you need to use snapshots with diskless virtual machine, +it must be started with 'orphan' qcow2 image. This image will be used +for storing VM snapshots. Here is the example of the command line for this: + + qemu-system-i386 -icount shift=3,rr=replay,rrfile=record.bin,rrsnapshot=init \ +-net none -drive file=empty.qcow2,if=none,id=rr + +empty.qcow2 drive does not connected to any virtual block device and used +for VM snapshots only. This is not rST. Are you sure you included the right patch. No problem though, I can just skip it. Ok, please skip it, I'll update it later. Pavel Dovgalyuk
Re: [PATCH 1/3] colo-compare: return -1 if no packet is queued
On 9/23/20 9:41 AM, Zhang, Chen wrote:
-Original Message-
From: Li Zhijian
Sent: Tuesday, September 22, 2020 5:55 PM
To: Zhang, Chen ; jasow...@redhat.com
Cc: qemu-devel@nongnu.org; Li Zhijian
Subject: [PATCH 1/3] colo-compare: return -1 if no packet is queued
Return 0 will trigger a packet comparison
Yes, we need active trigger a checkpoint to flush all the queued packets here.
Previously, no new checkpoint will be triggered since no new packet is queued
though colo_compare_connection() is called.
actually we should send a notify to colo frame immediately, no need to compare
them any more in order to less latency.
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 3a45d64175..23092e4496 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -285,10 +285,13 @@ static int packet_enqueue(CompareState *s, int mode,
Connection **con)
}
if (!ret) {
+ /* queue is too long, do a checkpoint to release all queued packets */
+ colo_compare_inconsistency_notify(s);
trace_colo_compare_drop_packet(colo_mode[mode],
"queue size too big, drop packet");
packet_destroy(pkt, NULL);
pkt = NULL;
+ return -1;
}
*con = conn;
Otherwise, we should drop all the packet after this time still next checkpoint.
So, I think original logic is a better choice.
Thanks
Zhang Chen
Signed-off-by: Li Zhijian
---
net/colo-compare.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/colo-compare.c b/net/colo-compare.c index
3a45d64175..039b515611 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -289,6 +289,7 @@ static int packet_enqueue(CompareState *s, int
mode, Connection **con)
"queue size too big, drop packet");
packet_destroy(pkt, NULL);
pkt = NULL;
+return -1;
}
*con = conn;
--
2.28.0
Re: [Bug 1896096] Re: Git version: Build process is broken in block_curl.c.o
Yes that's a silly mistake that invokes the linker with a dummy empty argument. I'll fix it and repost, thanks for testing on Arch! Paolo Il mar 22 set 2020, 23:41 Toolybird <1896...@bugs.launchpad.net> ha scritto: > ** Attachment added: "meson-log.txt" > > https://bugs.launchpad.net/qemu/+bug/1896096/+attachment/5413347/+files/meson-log.txt > > -- > You received this bug notification because you are subscribed to the bug > report. > https://bugs.launchpad.net/bugs/1896096 > > Title: > Git version: Build process is broken in block_curl.c.o > > To manage notifications about this bug go to: > https://bugs.launchpad.net/qemu/+bug/1896096/+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/1896096 Title: Git version: Build process is broken in block_curl.c.o Status in QEMU: Invalid Bug description: Gcc version: 10.2.0 Glusterfs: 8.1 Libguestfs: 1.42 Configure options used: configure \ --prefix=/usr \ --sysconfdir=/etc \ --localstatedir=/var \ --libexecdir=/usr/lib/qemu \ --extra-ldflags="$LDFLAGS" \ --smbd=/usr/bin/smbd \ --enable-modules \ --enable-sdl \ --disable-werror \ --enable-slirp=system \ --enable-xfsctl \ --audio-drv-list="pa alsa sdl" Error log attached. Here is the beginning: /usr/bin/ld: /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../lib/Scrt1.o: in function `_start': (.text+0x24): undefined reference to `main' /usr/bin/ld: libblock-curl.a(block_curl.c.o): in function `curl_block_init': To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1896096/+subscriptions
Re: [PATCH v1 1/8] s390x/tcg: Implement ADD HALFWORD (AGH)
On 22/09/2020 12.31, David Hildenbrand wrote:
> Easy, just like ADD HALFWORD IMMEDIATE (AGHI).
>
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/insn-data.def | 1 +
> target/s390x/translate.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index d79ae9e3f1..8dbeaf8c49 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -52,6 +52,7 @@
> /* ADD HALFWORD */
> C(0x4a00, AH, RX_a, Z, r1, m2_16s, new, r1_32, add, adds32)
> C(0xe37a, AHY, RXY_a, LD, r1, m2_16s, new, r1_32, add, adds32)
> +C(0xe338, AGH, RXY_a, MIE2,r1, m2_16s, r1, 0, add, adds64)
> /* ADD HALFWORD IMMEDIATE */
> C(0xa70a, AHI, RI_a, Z, r1, i2, new, r1_32, add, adds32)
> C(0xa70b, AGHI,RI_a, Z, r1, i2, r1, 0, add, adds64)
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index a777343821..21d77b7e74 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -6098,6 +6098,7 @@ enum DisasInsnEnum {
> #define FAC_AIS S390_FEAT_ADAPTER_INT_SUPPRESSION
> #define FAC_V S390_FEAT_VECTOR /* vector facility */
> #define FAC_VE S390_FEAT_VECTOR_ENH /* vector enhancements facility
> 1 */
> +#define FAC_MIE2S390_FEAT_MISC_INSTRUCTION_EXT /*
> miscellaneous-instruction-extensions facility 2 */
Maybe you should use "S390_FEAT_MISC_INSTRUCTION_EXT2" (i.e. with 2 at
the end) to avoid that it gets confused with the first MIE ?
Thomas
Re: [PATCH v2 0/2] block: deprecate the sheepdog driver
22.09.2020 21:11, Neal Gompa wrote: On Tue, Sep 22, 2020 at 1:43 PM Daniel P. Berrangé wrote: On Tue, Sep 22, 2020 at 01:09:06PM -0400, Neal Gompa wrote: On Tue, Sep 22, 2020 at 12:16 PM Daniel P. Berrangé wrote: 2 years back I proposed dropping the sheepdog mailing list from the MAINTAINERS file, but somehow the patch never got picked up: https://lists.gnu.org/archive/html/qemu-block/2018-03/msg01048.html So here I am with the same patch again. This time I go further and deprecate the sheepdog driver entirely. See the rationale in the second patch commit message. Daniel P. Berrangé (2): block: drop moderated sheepdog mailing list from MAINTAINERS file block: deprecate the sheepdog block driver MAINTAINERS| 1 - block/sheepdog.c | 15 +++ configure | 5 +++-- docs/system/deprecated.rst | 9 + 4 files changed, 27 insertions(+), 3 deletions(-) -- 2.26.2 I don't know of anyone shipping this other than Fedora, and I certainly don't use it there. Upstream looks like it's unmaintained now, with no commits in over two years: https://github.com/sheepdog/sheepdog/commits/master Can we also get a corresponding change to eliminate this from libvirt? This is only deprecation in QEMU, the feature still exists and is intended to be as fully functional as in previous releases. Assuming QEMU actually deletes the feature at end of the deprecation cycle, then libvirt would look at removing its own support. Does that stop us from deprecating it in libvirt though? I think not. Libvirt is not intended to support all Qemu features and I'm sure it doesn't. Amd it can safely deprecate features even if they are not-deprecated in Qemu. The only possible conflict is when Qemu wants to deprecate something that Libvirt wants to continue support for (or even can't live without). -- Best regards, Vladimir
Re: [PATCH v1 2/8] s390x/tcg: Implement SUBTRACT HALFWORD (SGH)
On 22/09/2020 12.31, David Hildenbrand wrote: > Easy to wire up. > > Signed-off-by: David Hildenbrand > --- > target/s390x/insn-data.def | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def > index 8dbeaf8c49..e851e9df5e 100644 > --- a/target/s390x/insn-data.def > +++ b/target/s390x/insn-data.def > @@ -883,6 +883,7 @@ > /* SUBTRACT HALFWORD */ > C(0x4b00, SH, RX_a, Z, r1, m2_16s, new, r1_32, sub, subs32) > C(0xe37b, SHY, RXY_a, LD, r1, m2_16s, new, r1_32, sub, subs32) > +C(0xe339, SGH, RXY_a, MIE2,r1, m2_16s, r1, 0, sub, subs64) > /* SUBTRACT HIGH */ > C(0xb9c9, SHHHR, RRF_a, HW, r2_sr32, r3_sr32, new, r1_32h, sub, > subs32) > C(0xb9d9, SHHLR, RRF_a, HW, r2_sr32, r3, new, r1_32h, sub, subs32) > Reviewed-by: Thomas Huth
Re: [PATCH v1 1/8] s390x/tcg: Implement ADD HALFWORD (AGH)
On 22/09/2020 12.31, David Hildenbrand wrote:
> Easy, just like ADD HALFWORD IMMEDIATE (AGHI).
>
> Signed-off-by: David Hildenbrand
> ---
> target/s390x/insn-data.def | 1 +
> target/s390x/translate.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index d79ae9e3f1..8dbeaf8c49 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -52,6 +52,7 @@
> /* ADD HALFWORD */
> C(0x4a00, AH, RX_a, Z, r1, m2_16s, new, r1_32, add, adds32)
> C(0xe37a, AHY, RXY_a, LD, r1, m2_16s, new, r1_32, add, adds32)
> +C(0xe338, AGH, RXY_a, MIE2,r1, m2_16s, r1, 0, add, adds64)
> /* ADD HALFWORD IMMEDIATE */
> C(0xa70a, AHI, RI_a, Z, r1, i2, new, r1_32, add, adds32)
> C(0xa70b, AGHI,RI_a, Z, r1, i2, r1, 0, add, adds64)
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index a777343821..21d77b7e74 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -6098,6 +6098,7 @@ enum DisasInsnEnum {
> #define FAC_AIS S390_FEAT_ADAPTER_INT_SUPPRESSION
> #define FAC_V S390_FEAT_VECTOR /* vector facility */
> #define FAC_VE S390_FEAT_VECTOR_ENH /* vector enhancements facility
> 1 */
> +#define FAC_MIE2S390_FEAT_MISC_INSTRUCTION_EXT /*
> miscellaneous-instruction-extensions facility 2 */
>
> static const DisasInsn insn_info[] = {
> #include "insn-data.def"
>
Looks right to me.
Reviewed-by: Thomas Huth
Re: [PATCH v2 2/3] Reduce the time of checkpoint for COLO
On 9/22/20 5:24 PM, leirao wrote:
we should set ram_bulk_stage to false after ram_state_init,
otherwise the bitmap will be unused in migration_bitmap_find_dirty.
all pages in ram cache will be flushed to the ram of secondary guest
for each checkpoint.
Signed-off-by: leirao
---
migration/ram.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 76d4fee..59ff0cf 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3018,6 +3018,18 @@ static void decompress_data_with_multi_threads(QEMUFile
*f,
qemu_mutex_unlock(&decomp_done_lock);
}
+ /*
+ * we must set ram_bulk_stage to fasle, otherwise in
a typo: s/fasle/false
Reviewed-by: Li Zhijian
+ * migation_bitmap_find_dirty the bitmap will be unused and
+ * all the pages in ram cache wil be flushed to the ram of
+ * secondary VM.
+ */
+static void colo_init_ram_state(void)
+{
+ram_state_init(&ram_state);
+ram_state->ram_bulk_stage = false;
+}
+
/*
* colo cache: this is for secondary VM, we cache the whole
* memory of the secondary VM, it is need to hold the global lock
@@ -3061,7 +3073,7 @@ int colo_init_ram_cache(void)
}
}
-ram_state_init(&ram_state);
+colo_init_ram_state();
return 0;
}
[PATCH] add maximum combined fw size as machine configuration option
From: Erich McMillan
Signed-off-by: Erich McMillan
---
hw/i386/pc.c | 50
hw/i386/pc_sysfw.c | 13 ++--
include/hw/i386/pc.h | 2 ++
3 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d11daacc23..89775e7d5b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1869,6 +1869,49 @@ static void pc_machine_set_max_ram_below_4g(Object *obj,
Visitor *v,
pcms->max_ram_below_4g = value;
}
+static void pc_machine_get_max_fw_size(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+PCMachineState *pcms = PC_MACHINE(obj);
+uint64_t value = pcms->max_fw_size;
+
+visit_type_size(v, name, &value, errp);
+}
+
+static void pc_machine_set_max_fw_size(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+PCMachineState *pcms = PC_MACHINE(obj);
+Error *error = NULL;
+uint64_t value;
+
+visit_type_size(v, name, &value, &error);
+if (error) {
+error_propagate(errp, error);
+return;
+}
+
+/*
+* We don't have a theoretically justifiable exact lower bound on the base
+* address of any flash mapping. In practice, the IO-APIC MMIO range is
+* [0xFEE0..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+* only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB
in
+* size.
+*/
+if (value > 16 * MiB) {
+error_setg(errp,
+ "User specified max allowed firmware size %" PRIu64 " is "
+ "greater than 16MiB. If combined firwmare size exceeds "
+ "16MiB the system may not boot, or experience intermittent"
+ "stability issues.",
+ value);
+}
+
+pcms->max_fw_size = value;
+}
+
static void pc_machine_initfn(Object *obj)
{
PCMachineState *pcms = PC_MACHINE(obj);
@@ -1884,6 +1927,7 @@ static void pc_machine_initfn(Object *obj)
pcms->smbus_enabled = true;
pcms->sata_enabled = true;
pcms->pit_enabled = true;
+pcms->max_fw_size = 8 * MiB; /* use default */
pc_system_flash_create(pcms);
pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
@@ -2004,6 +2048,12 @@ static void pc_machine_class_init(ObjectClass *oc, void
*data)
object_class_property_add_bool(oc, PC_MACHINE_PIT,
pc_machine_get_pit, pc_machine_set_pit);
+
+object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size",
+pc_machine_get_max_fw_size, pc_machine_set_max_fw_size,
+NULL, NULL);
+object_class_property_set_description(oc, PC_MACHINE_MAX_FW_SIZE,
+"Maximum combined firmware size");
}
static const TypeInfo pc_machine_info = {
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b6c0822fe3..22450ba0ef 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -39,15 +39,6 @@
#include "hw/block/flash.h"
#include "sysemu/kvm.h"
-/*
- * We don't have a theoretically justifiable exact lower bound on the base
- * address of any flash mapping. In practice, the IO-APIC MMIO range is
- * [0xFEE0..0xFEE01000] -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
- * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
- * size.
- */
-#define FLASH_SIZE_LIMIT (8 * MiB)
-
#define FLASH_SECTOR_SIZE 4096
static void pc_isa_bios_init(MemoryRegion *rom_memory,
@@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms,
}
if ((hwaddr)size != size
|| total_size > HWADDR_MAX - size
-|| total_size + size > FLASH_SIZE_LIMIT) {
+|| total_size + size > pcms->max_fw_size) {
error_report("combined size of system firmware exceeds "
"%" PRIu64 " bytes",
- FLASH_SIZE_LIMIT);
+ pcms->max_fw_size);
exit(1);
}
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index fe52e165b2..f7c8e7cbfe 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -43,6 +43,7 @@ struct PCMachineState {
bool smbus_enabled;
bool sata_enabled;
bool pit_enabled;
+uint64_t max_fw_size;
/* NUMA information: */
uint64_t numa_nodes;
@@ -59,6 +60,7 @@ struct PCMachineState {
#define PC_MACHINE_SMBUS"smbus"
#define PC_MACHINE_SATA "sata"
#define PC_MACHINE_PIT "pit"
+#define PC_MACHINE_MAX_FW_SIZE "max-fw-size"
/**
* PCMachineClass:
--
2.25.1
Re: [External] Re: [PATCH v2 3/3] target-i386: post memory failure event to uplayer
On 9/22/20 6:30 PM, Philippe Mathieu-Daudé wrote:
On 9/22/20 11:56 AM, zhenwei pi wrote:
Post memory failure event to uplayer to handle hardware memory
corrupted event. Rather than simple QEMU log, QEMU could report more
effective message to uplayer. For example, guest crashes by MCE,
selecting another host server is a better choice.
Signed-off-by: zhenwei pi
---
target/i386/helper.c | 15 +++
target/i386/kvm.c| 7 ++-
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index 0c7fd32491..47823c29e4 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -18,6 +18,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/qapi-events-run-state.h"
#include "cpu.h"
#include "exec/exec-all.h"
#include "qemu/qemu-print.h"
@@ -858,6 +859,7 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data
data)
CPUX86State *cenv = &cpu->env;
uint64_t *banks = cenv->mce_banks + 4 * params->bank;
char msg[64];
+MemoryFailureFlags mf_flags = {0};
bool need_reset = false;
cpu_synchronize_state(cs);
@@ -869,6 +871,12 @@ static void do_inject_x86_mce(CPUState *cs,
run_on_cpu_data data)
if (!(params->flags & MCE_INJECT_UNCOND_AO)
&& !(params->status & MCI_STATUS_AR)
&& (cenv->mcg_status & MCG_STATUS_MCIP)) {
+mf_flags.has_action_required = true;
+mf_flags.action_required = false;
+mf_flags.has_recursive = true;
+mf_flags.recursive = true;
+qapi_event_send_memory_failure(MEMORY_FAILURE_ACTION_GUEST_MCE_INJECT,
+ true, &mf_flags);
Can you extract a function such:
static void emit_guest_mce_failure(bool action_required, bool recursive)
{
...
}
To use as:
emit_guest_mce_failure(true, true);
return;
}
There are 2 field in struct MemoryFailureFlags currently, maybe more
fields need to be added in the future, and emit_guest_mce_failure need
more arguments too. So is it worth wrapping a function now?
@@ -909,6 +917,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data
data)
}
if (need_reset) {
+qapi_event_send_memory_failure(
+ MEMORY_FAILURE_ACTION_GUEST_MCE_FATAL, false, NULL);
monitor_printf(params->mon, "%s", msg);
qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
@@ -934,6 +944,11 @@ static void do_inject_x86_mce(CPUState *cs,
run_on_cpu_data data)
} else {
banks[1] |= MCI_STATUS_OVER;
}
+
+mf_flags.has_action_required = true;
+mf_flags.action_required = !!(params->status & MCI_STATUS_AR);
+qapi_event_send_memory_failure(MEMORY_FAILURE_ACTION_GUEST_MCE_INJECT,
+ true, &mf_flags);
And here:
emit_guest_mce_failure(params->status & MCI_STATUS_AR, false);
}
void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 9efb07e7c8..989889c291 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -14,6 +14,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qapi-events-run-state.h"
#include
#include
@@ -577,6 +578,8 @@ static void kvm_mce_inject(X86CPU *cpu, hwaddr paddr, int code)
static void hardware_memory_error(void *host_addr)
{
+qapi_event_send_memory_failure(MEMORY_FAILURE_ACTION_HYPERVISOR_FATAL,
+ false, NULL);
error_report("QEMU got Hardware memory error at addr %p", host_addr);
exit(1);
}
@@ -631,7 +634,9 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void
*addr)
hardware_memory_error(addr);
}
-/* Hope we are lucky for AO MCE */
+/* Hope we are lucky for AO MCE, just notify a event */
+qapi_event_send_memory_failure(MEMORY_FAILURE_ACTION_HYPERVISOR_IGNORE,
+ false, NULL);
}
static void kvm_reset_exception(CPUX86State *env)
--
zhenwei pi
[PATCH] target/i386: kvm: require KVM_CAP_IRQ_ROUTING
KVM_CAP_IRQ_ROUTING has been available for x86 since Linux 2.6.33.
We can make it a requirement since that was more than ten years ago.
Signed-off-by: Paolo Bonzini
---
hw/i386/fw_cfg.c | 2 +-
hw/i386/kvm/apic.c | 4 +---
hw/i386/microvm.c | 2 +-
hw/i386/pc.c | 2 +-
target/i386/kvm-stub.c | 5 -
target/i386/kvm.c | 14 --
target/i386/kvm_i386.h | 1 -
7 files changed, 8 insertions(+), 22 deletions(-)
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 33441ad484..e92e17fa07 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -123,7 +123,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
acpi_tables, acpi_tables_len);
#endif
-fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
+fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, true);
fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
&e820_reserve, sizeof(e820_reserve));
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 4eb2d77b87..284c0da6cd 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -225,9 +225,7 @@ static void kvm_apic_realize(DeviceState *dev, Error **errp)
memory_region_init_io(&s->io_memory, OBJECT(s), &kvm_apic_io_ops, s,
"kvm-apic-msi", APIC_SPACE_SIZE);
-if (kvm_has_gsi_routing()) {
-msi_nonbroken = true;
-}
+msi_nonbroken = true;
}
static void kvm_apic_unrealize(DeviceState *dev)
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index aedcae3426..d4bcc28471 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -225,7 +225,7 @@ static void microvm_memory_init(MicrovmMachineState *mms)
fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, machine->smp.cpus);
fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, machine->smp.max_cpus);
fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
-fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
+fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, true);
fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
&e820_reserve, sizeof(e820_reserve));
fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index da2ebc3fa0..9759d8acae 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -825,7 +825,7 @@ void pc_guest_info_init(PCMachineState *pcms)
MachineState *ms = MACHINE(pcms);
X86MachineState *x86ms = X86_MACHINE(pcms);
-x86ms->apic_xrupt_override = kvm_allows_irq0_override();
+x86ms->apic_xrupt_override = true;
pcms->numa_nodes = ms->numa_state->num_nodes;
pcms->node_mem = g_malloc0(pcms->numa_nodes *
sizeof *pcms->node_mem);
diff --git a/target/i386/kvm-stub.c b/target/i386/kvm-stub.c
index 872ef7df4c..92f49121b8 100644
--- a/target/i386/kvm-stub.c
+++ b/target/i386/kvm-stub.c
@@ -13,11 +13,6 @@
#include "cpu.h"
#include "kvm_i386.h"
-bool kvm_allows_irq0_override(void)
-{
-return 1;
-}
-
#ifndef __OPTIMIZE__
bool kvm_has_smm(void)
{
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 595e822161..3996cf9764 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -153,11 +153,6 @@ bool kvm_has_exception_payload(void)
return has_exception_payload;
}
-bool kvm_allows_irq0_override(void)
-{
-return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
-}
-
static bool kvm_x2apic_api_set_flags(uint64_t flags)
{
KVMState *s = KVM_STATE(current_accel());
@@ -4569,12 +4564,11 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
void kvm_arch_init_irq_routing(KVMState *s)
{
if (!kvm_check_extension(s, KVM_CAP_IRQ_ROUTING)) {
-/* If kernel can't do irq routing, interrupt source
- * override 0->2 cannot be set up as required by HPET.
- * So we have to disable it.
- */
-no_hpet = 1;
+error_report("Your kernel is too old. This version of QEMU requires "
+"KVM_CAP_IRQ_ROUTING support.");
+exit(1);
}
+
/* We know at this point that we're using the in-kernel
* irqchip, so we can use irqfds, and on x86 we know
* we can use msi via irqfd and GSI routing.
diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h
index 0fce4e51d2..a4a619cebb 100644
--- a/target/i386/kvm_i386.h
+++ b/target/i386/kvm_i386.h
@@ -32,7 +32,6 @@
#endif /* CONFIG_KVM */
-bool kvm_allows_irq0_override(void);
bool kvm_has_smm(void);
bool kvm_has_adjust_clock(void);
bool kvm_has_adjust_clock_stable(void);
--
2.26.2
[PATCH] target/i386: kvm: do not use kvm_check_extension to find paravirtual capabilities
Paravirtualized features have been listed in KVM_GET_SUPPORTED_CPUID since
Linux 2.6.35 (commit 84478c829d0f, "KVM: x86: export paravirtual cpuid flags
in KVM_GET_SUPPORTED_CPUID", 2010-05-19). It has been more than 10 years,
so remove the fallback code.
Signed-off-by: Paolo Bonzini
---
target/i386/kvm.c | 32
1 file changed, 32 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 3996cf9764..ccc65ad0f5 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -279,30 +279,6 @@ static struct kvm_cpuid2 *get_supported_cpuid(KVMState *s)
return cpuid;
}
-static const struct kvm_para_features {
-int cap;
-int feature;
-} para_features[] = {
-{ KVM_CAP_CLOCKSOURCE, KVM_FEATURE_CLOCKSOURCE },
-{ KVM_CAP_NOP_IO_DELAY, KVM_FEATURE_NOP_IO_DELAY },
-{ KVM_CAP_PV_MMU, KVM_FEATURE_MMU_OP },
-{ KVM_CAP_ASYNC_PF, KVM_FEATURE_ASYNC_PF },
-{ KVM_CAP_ASYNC_PF_INT, KVM_FEATURE_ASYNC_PF_INT },
-};
-
-static int get_para_features(KVMState *s)
-{
-int i, features = 0;
-
-for (i = 0; i < ARRAY_SIZE(para_features); i++) {
-if (kvm_check_extension(s, para_features[i].cap)) {
-features |= (1 << para_features[i].feature);
-}
-}
-
-return features;
-}
-
static bool host_tsx_broken(void)
{
int family, model, stepping;\
@@ -362,13 +338,11 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
uint32_t function,
struct kvm_cpuid2 *cpuid;
uint32_t ret = 0;
uint32_t cpuid_1_edx;
-bool found = false;
cpuid = get_supported_cpuid(s);
struct kvm_cpuid_entry2 *entry = cpuid_find_entry(cpuid, function, index);
if (entry) {
-found = true;
ret = cpuid_entry_get_reg(entry, reg);
}
@@ -443,12 +417,6 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
uint32_t function,
}
} else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
ret |= 1U << KVM_HINTS_REALTIME;
-found = 1;
-}
-
-/* fallback for older kernels */
-if ((function == KVM_CPUID_FEATURES) && !found) {
-ret = get_para_features(s);
}
return ret;
--
2.26.2
RE: [PATCH v1 0/3] Remove the limitation of Intel PT CPUID info
> > Hi Eduardo, > > This patch set will remove some limitations of Intel PT CPUID > > information. > > 1. The "IP payloads" feature will disable the Intel PT in guests and it > > will be > coming soon. > > 2. To make the live migration safe, we set the Intel PT CPUID as a > > constant > value(Icelake server CPUID). It will mask off the new feature of Intel PT. > > Isn't this series doing the opposite of 2? It replaces all constant CPUID > values > with kvm_arch_get_supported_cpuid(), making the feature unavailable in > migration-safe mode. Yes, This series will expose all the HW capabilities to KVM guest if the Intel PT is supported in the guest. > > Does it mean the plan is to drop intel-pt migration support entirely? I don't want to drop intel-pt live migration feature. As discussed with you before, the Intel PT feature includes some sub-features and may be different on each HW platform. Expose all the capabilities to the guest can't make live migration safe. Do you have any new proposals? Thanks, Luwei Kang > > > > > About this issue https://bugzilla.redhat.com/show_bug.cgi?id=1853972, > Intel PT is disabled in the guest by default, we should use "-cpu Icelake- > Server,+intel-pt" to enable the Intel PT. > > That's correct. The point of the BZ is that libvirt mode=host-model was > expected to include intel-pt automatically when available. With this series, > the > request in the BZ stops making sense (because intel-pt won't be migration-safe > anymore), but I'm not sure yet that's really the plan. > > > > > > Thanks, > > Luwei Kang > > > > > -Original Message- > > > From: Eduardo Habkost > > > Sent: Saturday, September 19, 2020 6:03 AM > > > To: Kang, Luwei > > > Cc: pbonz...@redhat.com; r...@twiddle.net; qemu-devel@nongnu.org; > > > Strong, Beeman ; Jiri Denemark > > > ; Robert Hoo > > > Subject: Re: [PATCH v1 0/3] Remove the limitation of Intel PT CPUID > > > info > > > > > > Hi Luwei Kang, > > > > > > I was looking for info on intel-pt and just saw this series, and it > > > was never reviewed or merged (sorry for missing it!). Is this still > > > the approach we want to follow for intel-pt? > > > > > > I'm CCing Jiri Denemark because this might be relevant for a libvirt > > > issue related to intel-pt we were investigating[1]. > > > > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1853972 > > > > > > > > > On Mon, Mar 30, 2020 at 09:56:09AM +, Kang, Luwei wrote: > > > > > -Original Message- > > > > > From: Kang, Luwei > > > > > Sent: Tuesday, February 25, 2020 5:38 AM > > > > > To: pbonz...@redhat.com; r...@twiddle.net; ehabk...@redhat.com > > > > > Cc: qemu-devel@nongnu.org; Strong, Beeman > > > ; > > > > > Kang, Luwei > > > > > Subject: [PATCH v1 0/3] Remove the limitation of Intel PT CPUID > > > > > info > > > > > > > > > > The Intel PT feature includes some > > > > > sub-features(CPUID.(EAX=14H,ECX=0H)) > > > > > and these sub-features are different on different HW platforms. > > > > > To make the live migration safety(get the same CPUID info with > > > > > same cpu model on different HW platform), the current Intel PT > > > > > CPUID information is set to a constant value(from ICELAKE Server). > > > > > > > > > > It will block the new feature in the later HW platform. what's > > > > > more, the support of "IP payloads" will disable the Intel PT in > > > > > KVM guest(patch 1) but it will come soon. > > > > > > > > > > This patchset remove this limitation and expose all the > > > > > capabilities to KVM guest. As it will break the live migration > > > > > safe, Intel PT will be masked as unmigratable. > > > > > > > > Ping. > > > > > > > > Thanks, > > > > Luwei Kang > > > > > > > > > > > > > > Luwei Kang (3): > > > > > i386: Remove the limitation of IP payloads for Intel PT > > > > > i386: Remove the CPUID limitation of Intel PT > > > > > i386: Mark the 'INTEL_PT' CPUID bit as unmigratable > > > > > > > > > > target/i386/cpu.c | 69 > > > > > --- > > > > > 1 file changed, 5 insertions(+), 64 deletions(-) > > > > > > > > > > -- > > > > > 1.8.3.1 > > > > > > > > > > -- > > > Eduardo > > > > -- > Eduardo
Re: [PATCH] bios-tables-test: Remove kernel-irqchip=off option
On 22/09/20 21:47, Eduardo Habkost wrote:
> We don't need to use kernel-irqchip=off for irq0 override if IRQ
> routing is supported by the host, which is the case since 2009
> (IRQ routing was added to KVM in Linux v2.6.30).
>
> This is a more straightforward fix for Launchpad bug #1896263, as
> it doesn't require increasing the complexity of the MSR code.
> kernel-irqchip=off is for debugging only and there's no need to
> increase the complexity of the code just to work around an issue
> that was already fixed in the kernel.
>
> Fixes: https://bugs.launchpad.net/bugs/1896263
> Signed-off-by: Eduardo Habkost
> ---
> tests/qtest/bios-tables-test.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index a9c8d478aee..27e8f0a1cb7 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -663,8 +663,7 @@ static void test_acpi_one(const char *params, test_data
> *data)
> data->uefi_fl1, data->uefi_fl2, data->cd, params ? params : "");
>
> } else {
> -/* Disable kernel irqchip to be able to override apic irq0. */
> -args = g_strdup_printf("-machine %s,kernel-irqchip=off %s -accel tcg
> "
> +args = g_strdup_printf("-machine %s %s -accel tcg "
> "-net none -display none %s "
> "-drive id=hd0,if=none,file=%s,format=raw "
> "-device %s,drive=hd0 ",
>
Queued, thanks.
Paolo
RE: [PATCH 3/3] colo-compare: check mark in mutual exclusion
> -Original Message-
> From: Li Zhijian
> Sent: Tuesday, September 22, 2020 5:55 PM
> To: Zhang, Chen ; jasow...@redhat.com
> Cc: qemu-devel@nongnu.org; Li Zhijian
> Subject: [PATCH 3/3] colo-compare: check mark in mutual exclusion
>
> Signed-off-by: Li Zhijian
Reviewed-by: Zhang Chen
By the way, we have other optimization here, will be sent in the future.
Thanks
Zhang Chen
> ---
> net/colo-compare.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 039b515611..19633fc684 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -481,13 +481,11 @@ sec:
> colo_release_primary_pkt(s, ppkt);
> g_queue_push_head(&conn->secondary_list, spkt);
> goto pri;
> -}
> -if (mark == COLO_COMPARE_FREE_SECONDARY) {
> +} else if (mark == COLO_COMPARE_FREE_SECONDARY) {
> conn->compare_seq = spkt->seq_end;
> packet_destroy(spkt, NULL);
> goto sec;
> -}
> -if (mark == (COLO_COMPARE_FREE_PRIMARY |
> COLO_COMPARE_FREE_SECONDARY)) {
> +} else if (mark == (COLO_COMPARE_FREE_PRIMARY |
> + COLO_COMPARE_FREE_SECONDARY)) {
> conn->compare_seq = ppkt->seq_end;
> colo_release_primary_pkt(s, ppkt);
> packet_destroy(spkt, NULL);
> --
> 2.28.0
>
>
RE: [PATCH 2/3] colo-compare: fix missing compare_seq initialization
> -Original Message-
> From: Li Zhijian
> Sent: Tuesday, September 22, 2020 5:55 PM
> To: Zhang, Chen ; jasow...@redhat.com
> Cc: qemu-devel@nongnu.org; Li Zhijian
> Subject: [PATCH 2/3] colo-compare: fix missing compare_seq initialization
>
> Signed-off-by: Li Zhijian
Reviewed-by: Zhang Chen
Thanks
Zhang Chen
> ---
> net/colo.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/net/colo.c b/net/colo.c
> index a6c66d829a..ef00609848 100644
> --- a/net/colo.c
> +++ b/net/colo.c
> @@ -133,14 +133,11 @@ void reverse_connection_key(ConnectionKey *key)
>
> Connection *connection_new(ConnectionKey *key) {
> -Connection *conn = g_slice_new(Connection);
> +Connection *conn = g_slice_new0(Connection);
>
> conn->ip_proto = key->ip_proto;
> conn->processing = false;
> -conn->offset = 0;
> conn->tcp_state = TCPS_CLOSED;
> -conn->pack = 0;
> -conn->sack = 0;
> g_queue_init(&conn->primary_list);
> g_queue_init(&conn->secondary_list);
>
> --
> 2.28.0
>
>
RE: [PATCH 1/3] colo-compare: return -1 if no packet is queued
> -Original Message- > From: Li Zhijian > Sent: Tuesday, September 22, 2020 5:55 PM > To: Zhang, Chen ; jasow...@redhat.com > Cc: qemu-devel@nongnu.org; Li Zhijian > Subject: [PATCH 1/3] colo-compare: return -1 if no packet is queued > > Return 0 will trigger a packet comparison > Yes, we need active trigger a checkpoint to flush all the queued packets here. Otherwise, we should drop all the packet after this time still next checkpoint. So, I think original logic is a better choice. Thanks Zhang Chen > Signed-off-by: Li Zhijian > --- > net/colo-compare.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > 3a45d64175..039b515611 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -289,6 +289,7 @@ static int packet_enqueue(CompareState *s, int > mode, Connection **con) > "queue size too big, drop packet"); > packet_destroy(pkt, NULL); > pkt = NULL; > +return -1; > } > > *con = conn; > -- > 2.28.0 > >
RE: [PATCH v2 3/3] Fix the qemu crash when guest shutdown in COLO mode
> -Original Message-
> From: Rao, Lei
> Sent: Tuesday, September 22, 2020 5:25 PM
> To: Zhang, Chen ; lizhij...@cn.fujitsu.com;
> jasow...@redhat.com; quint...@redhat.com; dgilb...@redhat.com;
> pbonz...@redhat.com
> Cc: qemu-devel@nongnu.org; Rao, Lei
> Subject: [PATCH v2 3/3] Fix the qemu crash when guest shutdown in COLO
> mode
>
> In COLO mode, if the startup parameters of QEMU include "no-shutdown",
> QEMU will crash when the guest shutdown. The root cause is when the guest
> shutdown, the state of VM will switch COLO to SHUTDOWN. When do
> checkpoint again, the state will be changed to COLO. But the state switch is
> undefined in runstate_transitions_def, we should add it.
> This patch fixes the following:
> qemu-system-x86_64: invalid runstate transition: 'shutdown' -> 'colo'
> Aborted
>
> Signed-off-by: leirao
Reviewed-by: Zhang Chen
Thanks
Zhang Chen
> ---
> softmmu/vl.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c index f7b1034..c21606c 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -631,6 +631,7 @@ static const RunStateTransition
> runstate_transitions_def[] = {
> { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
> { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
> { RUN_STATE_SHUTDOWN, RUN_STATE_PRELAUNCH },
> +{ RUN_STATE_SHUTDOWN, RUN_STATE_COLO },
>
> { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
> { RUN_STATE_RUNNING, RUN_STATE_SUSPENDED },
> --
> 1.8.3.1
RE: [PATCH v2 2/3] Reduce the time of checkpoint for COLO
> -Original Message-
> From: Rao, Lei
> Sent: Tuesday, September 22, 2020 5:25 PM
> To: Zhang, Chen ; lizhij...@cn.fujitsu.com;
> jasow...@redhat.com; quint...@redhat.com; dgilb...@redhat.com;
> pbonz...@redhat.com
> Cc: qemu-devel@nongnu.org; Rao, Lei
> Subject: [PATCH v2 2/3] Reduce the time of checkpoint for COLO
>
> we should set ram_bulk_stage to false after ram_state_init, otherwise the
> bitmap will be unused in migration_bitmap_find_dirty.
> all pages in ram cache will be flushed to the ram of secondary guest for each
> checkpoint.
>
> Signed-off-by: leirao
Reviewed-by: Zhang Chen
Thanks
Zhang Chen
> ---
> migration/ram.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/migration/ram.c b/migration/ram.c index 76d4fee..59ff0cf 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3018,6 +3018,18 @@ static void
> decompress_data_with_multi_threads(QEMUFile *f,
> qemu_mutex_unlock(&decomp_done_lock);
> }
>
> + /*
> + * we must set ram_bulk_stage to fasle, otherwise in
> + * migation_bitmap_find_dirty the bitmap will be unused and
> + * all the pages in ram cache wil be flushed to the ram of
> + * secondary VM.
> + */
> +static void colo_init_ram_state(void)
> +{
> +ram_state_init(&ram_state);
> +ram_state->ram_bulk_stage = false;
> +}
> +
> /*
> * colo cache: this is for secondary VM, we cache the whole
> * memory of the secondary VM, it is need to hold the global lock @@ -
> 3061,7 +3073,7 @@ int colo_init_ram_cache(void)
> }
> }
>
> -ram_state_init(&ram_state);
> +colo_init_ram_state();
> return 0;
> }
>
> --
> 1.8.3.1
RE: [PATCH v2 1/3] Optimize seq_sorter function for colo-compare
> -Original Message-
> From: Rao, Lei
> Sent: Tuesday, September 22, 2020 5:25 PM
> To: Zhang, Chen ; lizhij...@cn.fujitsu.com;
> jasow...@redhat.com; quint...@redhat.com; dgilb...@redhat.com;
> pbonz...@redhat.com
> Cc: qemu-devel@nongnu.org; Rao, Lei
> Subject: [PATCH v2 1/3] Optimize seq_sorter function for colo-compare
>
> The seq of tcp has been filled in fill_pkt_tcp_info, it can be used directly
> here.
>
> Signed-off-by: leirao
Reviewed-by: Zhang Chen
By the way, please add Zhijian's reviewed-by in new version of patches.
Thanks
Zhang Chen
> ---
> net/colo-compare.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 3a45d64..86980ce 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -196,11 +196,7 @@ static void
> colo_compare_inconsistency_notify(CompareState *s)
>
> static gint seq_sorter(Packet *a, Packet *b, gpointer data) {
> -struct tcp_hdr *atcp, *btcp;
> -
> -atcp = (struct tcp_hdr *)(a->transport_header);
> -btcp = (struct tcp_hdr *)(b->transport_header);
> -return ntohl(atcp->th_seq) - ntohl(btcp->th_seq);
> +return a->tcp_seq - b->tcp_seq;
> }
>
> static void fill_pkt_tcp_info(void *data, uint32_t *max_ack)
> --
> 1.8.3.1
Re: [PATCH v2 03/38] qapi: move generator entrypoint into module
On Tue, Sep 22, 2020 at 05:00:26PM -0400, John Snow wrote: > As part of delinting and adding type hints to the QAPI generator, it's > helpful for the entrypoint to be part of the package, only leaving a > very tiny entrypoint shim outside of the module. > > Signed-off-by: John Snow > --- > scripts/qapi-gen.py | 90 +++ > scripts/qapi/main.py | 91 > 2 files changed, 97 insertions(+), 84 deletions(-) > create mode 100644 scripts/qapi/main.py > Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa signature.asc Description: PGP signature
[PATCH v26 16/17] vfio: Make vfio-pci device migration capable
If the device is not a failover primary device, call
vfio_migration_probe() and vfio_migration_finalize() to enable
migration support for those devices that support it respectively to
tear it down again.
Removed vfio_pci_vmstate structure.
Removed migration blocker from VFIO PCI device specific structure and use
migration blocker from generic structure of VFIO device.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Reviewed-by: Dr. David Alan Gilbert
---
hw/vfio/pci.c | 28
hw/vfio/pci.h | 1 -
2 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 9968cc553391..2418a448ebca 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2872,17 +2872,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
return;
}
-if (!pdev->failover_pair_id) {
-error_setg(&vdev->migration_blocker,
-"VFIO device doesn't support migration");
-ret = migrate_add_blocker(vdev->migration_blocker, errp);
-if (ret) {
-error_free(vdev->migration_blocker);
-vdev->migration_blocker = NULL;
-return;
-}
-}
-
vdev->vbasedev.name = g_path_get_basename(vdev->vbasedev.sysfsdev);
vdev->vbasedev.ops = &vfio_pci_ops;
vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI;
@@ -3152,6 +3141,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
}
}
+if (!pdev->failover_pair_id) {
+ret = vfio_migration_probe(&vdev->vbasedev, errp);
+if (ret) {
+error_report("%s: Migration disabled", vdev->vbasedev.name);
+}
+}
+
vfio_register_err_notifier(vdev);
vfio_register_req_notifier(vdev);
vfio_setup_resetfn_quirk(vdev);
@@ -3166,11 +3162,6 @@ out_teardown:
vfio_bars_exit(vdev);
error:
error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
-if (vdev->migration_blocker) {
-migrate_del_blocker(vdev->migration_blocker);
-error_free(vdev->migration_blocker);
-vdev->migration_blocker = NULL;
-}
}
static void vfio_instance_finalize(Object *obj)
@@ -3182,10 +3173,6 @@ static void vfio_instance_finalize(Object *obj)
vfio_bars_finalize(vdev);
g_free(vdev->emulated_config_bits);
g_free(vdev->rom);
-if (vdev->migration_blocker) {
-migrate_del_blocker(vdev->migration_blocker);
-error_free(vdev->migration_blocker);
-}
/*
* XXX Leaking igd_opregion is not an oversight, we can't remove the
* fw_cfg entry therefore leaking this allocation seems like the safest
@@ -3213,6 +3200,7 @@ static void vfio_exitfn(PCIDevice *pdev)
}
vfio_teardown_msi(vdev);
vfio_bars_exit(vdev);
+vfio_migration_finalize(&vdev->vbasedev);
}
static void vfio_pci_reset(DeviceState *dev)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 9f46af7e153f..0e3782b8e38a 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -173,7 +173,6 @@ struct VFIOPCIDevice {
bool no_vfio_ioeventfd;
bool enable_ramfb;
VFIODisplay *dpy;
-Error *migration_blocker;
Notifier irqchip_change_notifier;
};
--
2.7.0
[PATCH v26 15/17] vfio: Add ioctl to get dirty pages bitmap during dma unmap.
With vIOMMU, IO virtual address range can get unmapped while in pre-copy
phase of migration. In that case, unmap ioctl should return pages pinned
in that range and QEMU should find its correcponding guest physical
addresses and report those dirty.
Suggested-by: Alex Williamson
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
Note: Comments from v25 for this patch are not addressed in this series.
https://www.mail-archive.com/qemu-devel@nongnu.org/msg714646.html
Need to investigate more on the points raised in previous version.
hw/vfio/common.c | 90 +---
1 file changed, 86 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index c36f275951c4..7eeaa368187a 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -315,11 +315,88 @@ static bool
vfio_devices_all_stopped_and_saving(VFIOContainer *container)
return true;
}
+static bool vfio_devices_all_running_and_saving(VFIOContainer *container)
+{
+VFIOGroup *group;
+VFIODevice *vbasedev;
+MigrationState *ms = migrate_get_current();
+
+if (!migration_is_setup_or_active(ms->state)) {
+return false;
+}
+
+QLIST_FOREACH(group, &container->group_list, container_next) {
+QLIST_FOREACH(vbasedev, &group->device_list, next) {
+if ((vbasedev->device_state & VFIO_DEVICE_STATE_SAVING) &&
+(vbasedev->device_state & VFIO_DEVICE_STATE_RUNNING)) {
+continue;
+} else {
+return false;
+}
+}
+}
+return true;
+}
+
+static int vfio_dma_unmap_bitmap(VFIOContainer *container,
+ hwaddr iova, ram_addr_t size,
+ IOMMUTLBEntry *iotlb)
+{
+struct vfio_iommu_type1_dma_unmap *unmap;
+struct vfio_bitmap *bitmap;
+uint64_t pages = TARGET_PAGE_ALIGN(size) >> TARGET_PAGE_BITS;
+int ret;
+
+unmap = g_malloc0(sizeof(*unmap) + sizeof(*bitmap));
+
+unmap->argsz = sizeof(*unmap) + sizeof(*bitmap);
+unmap->iova = iova;
+unmap->size = size;
+unmap->flags |= VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP;
+bitmap = (struct vfio_bitmap *)&unmap->data;
+
+/*
+ * cpu_physical_memory_set_dirty_lebitmap() expects pages in bitmap of
+ * TARGET_PAGE_SIZE to mark those dirty. Hence set bitmap_pgsize to
+ * TARGET_PAGE_SIZE.
+ */
+
+bitmap->pgsize = TARGET_PAGE_SIZE;
+bitmap->size = ROUND_UP(pages, sizeof(__u64) * BITS_PER_BYTE) /
+ BITS_PER_BYTE;
+
+if (bitmap->size > container->max_dirty_bitmap_size) {
+error_report("UNMAP: Size of bitmap too big 0x%llx", bitmap->size);
+ret = -E2BIG;
+goto unmap_exit;
+}
+
+bitmap->data = g_try_malloc0(bitmap->size);
+if (!bitmap->data) {
+ret = -ENOMEM;
+goto unmap_exit;
+}
+
+ret = ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, unmap);
+if (!ret) {
+cpu_physical_memory_set_dirty_lebitmap((uint64_t *)bitmap->data,
+iotlb->translated_addr, pages);
+} else {
+error_report("VFIO_UNMAP_DMA with DIRTY_BITMAP : %m");
+}
+
+g_free(bitmap->data);
+unmap_exit:
+g_free(unmap);
+return ret;
+}
+
/*
* DMA - Mapping and unmapping for the "type1" IOMMU interface used on x86
*/
static int vfio_dma_unmap(VFIOContainer *container,
- hwaddr iova, ram_addr_t size)
+ hwaddr iova, ram_addr_t size,
+ IOMMUTLBEntry *iotlb)
{
struct vfio_iommu_type1_dma_unmap unmap = {
.argsz = sizeof(unmap),
@@ -328,6 +405,11 @@ static int vfio_dma_unmap(VFIOContainer *container,
.size = size,
};
+if (iotlb && container->dirty_pages_supported &&
+vfio_devices_all_running_and_saving(container)) {
+return vfio_dma_unmap_bitmap(container, iova, size, iotlb);
+}
+
while (ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, &unmap)) {
/*
* The type1 backend has an off-by-one bug in the kernel (71a7d3d78e3c
@@ -375,7 +457,7 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr
iova,
* the VGA ROM space.
*/
if (ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map) == 0 ||
-(errno == EBUSY && vfio_dma_unmap(container, iova, size) == 0 &&
+(errno == EBUSY && vfio_dma_unmap(container, iova, size, NULL) == 0 &&
ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map) == 0)) {
return 0;
}
@@ -546,7 +628,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
}
}
-ret = vfio_dma_unmap(container, iova, iotlb->addr_mask + 1);
+ret = vfio_dma_unmap(container, iova, iotlb->addr_mask + 1, iotlb);
if (ret) {
error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
"0x%"HWADDR_PRIx") = %d (%m)",
@@ -857,7 +939,7 @@ static void vfio_listener_
[PATCH v26 14/17] vfio: Add vfio_listener_log_sync to mark dirty pages
vfio_listener_log_sync gets list of dirty pages from container using
VFIO_IOMMU_GET_DIRTY_BITMAP ioctl and mark those pages dirty when all
devices are stopped and saving state.
Return early for the RAM block section of mapped MMIO region.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
hw/vfio/common.c | 136 +++
hw/vfio/trace-events | 1 +
2 files changed, 137 insertions(+)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index dc56cded2d95..c36f275951c4 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -29,6 +29,7 @@
#include "hw/vfio/vfio.h"
#include "exec/address-spaces.h"
#include "exec/memory.h"
+#include "exec/ram_addr.h"
#include "hw/hw.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
@@ -37,6 +38,7 @@
#include "sysemu/reset.h"
#include "trace.h"
#include "qapi/error.h"
+#include "migration/migration.h"
VFIOGroupList vfio_group_list =
QLIST_HEAD_INITIALIZER(vfio_group_list);
@@ -287,6 +289,33 @@ const MemoryRegionOps vfio_region_ops = {
};
/*
+ * Device state interfaces
+ */
+
+static bool vfio_devices_all_stopped_and_saving(VFIOContainer *container)
+{
+VFIOGroup *group;
+VFIODevice *vbasedev;
+MigrationState *ms = migrate_get_current();
+
+if (!migration_is_setup_or_active(ms->state)) {
+return false;
+}
+
+QLIST_FOREACH(group, &container->group_list, container_next) {
+QLIST_FOREACH(vbasedev, &group->device_list, next) {
+if ((vbasedev->device_state & VFIO_DEVICE_STATE_SAVING) &&
+!(vbasedev->device_state & VFIO_DEVICE_STATE_RUNNING)) {
+continue;
+} else {
+return false;
+}
+}
+}
+return true;
+}
+
+/*
* DMA - Mapping and unmapping for the "type1" IOMMU interface used on x86
*/
static int vfio_dma_unmap(VFIOContainer *container,
@@ -851,9 +880,116 @@ static void vfio_listener_region_del(MemoryListener
*listener,
}
}
+static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
+ uint64_t size, ram_addr_t ram_addr)
+{
+struct vfio_iommu_type1_dirty_bitmap *dbitmap;
+struct vfio_iommu_type1_dirty_bitmap_get *range;
+uint64_t pages;
+int ret;
+
+dbitmap = g_malloc0(sizeof(*dbitmap) + sizeof(*range));
+
+dbitmap->argsz = sizeof(*dbitmap) + sizeof(*range);
+dbitmap->flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
+range = (struct vfio_iommu_type1_dirty_bitmap_get *)&dbitmap->data;
+range->iova = iova;
+range->size = size;
+
+/*
+ * cpu_physical_memory_set_dirty_lebitmap() expects pages in bitmap of
+ * TARGET_PAGE_SIZE to mark those dirty. Hence set bitmap's pgsize to
+ * TARGET_PAGE_SIZE.
+ */
+range->bitmap.pgsize = TARGET_PAGE_SIZE;
+
+pages = TARGET_PAGE_ALIGN(range->size) >> TARGET_PAGE_BITS;
+range->bitmap.size = ROUND_UP(pages, sizeof(__u64) * BITS_PER_BYTE) /
+ BITS_PER_BYTE;
+range->bitmap.data = g_try_malloc0(range->bitmap.size);
+if (!range->bitmap.data) {
+ret = -ENOMEM;
+goto err_out;
+}
+
+ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap);
+if (ret) {
+error_report("Failed to get dirty bitmap for iova: 0x%llx "
+"size: 0x%llx err: %d",
+range->iova, range->size, errno);
+goto err_out;
+}
+
+cpu_physical_memory_set_dirty_lebitmap((uint64_t *)range->bitmap.data,
+ram_addr, pages);
+
+trace_vfio_get_dirty_bitmap(container->fd, range->iova, range->size,
+range->bitmap.size, ram_addr);
+err_out:
+g_free(range->bitmap.data);
+g_free(dbitmap);
+
+return ret;
+}
+
+static int vfio_sync_dirty_bitmap(VFIOContainer *container,
+ MemoryRegionSection *section)
+{
+VFIOGuestIOMMU *giommu = NULL;
+ram_addr_t ram_addr;
+uint64_t iova, size;
+int ret = 0;
+
+if (memory_region_is_iommu(section->mr)) {
+
+QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
+if (MEMORY_REGION(giommu->iommu) == section->mr &&
+giommu->n.start == section->offset_within_region) {
+VFIOIovaRange *iova_range;
+
+QLIST_FOREACH(iova_range, &giommu->iova_list, next) {
+ret = vfio_get_dirty_bitmap(container, iova_range->iova,
+iova_range->size,
iova_range->ram_addr);
+if (ret) {
+break;
+}
+}
+break;
+}
+}
+
+} else {
+iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
+size = int128_get64(section->size);
+
+ram_addr = memory_region_get_ram_addr(section->mr) +
+ section->
[PATCH v26 13/17] vfio: create mapped iova list when vIOMMU is enabled
Create mapped iova list when vIOMMU is enabled. For each mapped iova
save translated address. Add node to list on MAP and remove node from
list on UNMAP.
This list is used to track dirty pages during migration.
Signed-off-by: Kirti Wankhede
---
hw/vfio/common.c | 58 ++-
include/hw/vfio/vfio-common.h | 8 ++
2 files changed, 60 insertions(+), 6 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index d4959c036dd1..dc56cded2d95 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -407,8 +407,8 @@ static bool
vfio_listener_skipped_section(MemoryRegionSection *section)
}
/* Called with rcu_read_lock held. */
-static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb, void **vaddr,
- bool *read_only)
+static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
+ ram_addr_t *ram_addr, bool *read_only)
{
MemoryRegion *mr;
hwaddr xlat;
@@ -439,8 +439,17 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb, void
**vaddr,
return false;
}
-*vaddr = memory_region_get_ram_ptr(mr) + xlat;
-*read_only = !writable || mr->readonly;
+if (vaddr) {
+*vaddr = memory_region_get_ram_ptr(mr) + xlat;
+}
+
+if (ram_addr) {
+*ram_addr = memory_region_get_ram_addr(mr) + xlat;
+}
+
+if (read_only) {
+*read_only = !writable || mr->readonly;
+}
return true;
}
@@ -450,7 +459,6 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
VFIOContainer *container = giommu->container;
hwaddr iova = iotlb->iova + giommu->iommu_offset;
-bool read_only;
void *vaddr;
int ret;
@@ -466,7 +474,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
rcu_read_lock();
if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
-if (!vfio_get_vaddr(iotlb, &vaddr, &read_only)) {
+ram_addr_t ram_addr;
+bool read_only;
+
+if (!vfio_get_xlat_addr(iotlb, &vaddr, &ram_addr, &read_only)) {
goto out;
}
/*
@@ -484,8 +495,28 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
"0x%"HWADDR_PRIx", %p) = %d (%m)",
container, iova,
iotlb->addr_mask + 1, vaddr, ret);
+} else {
+VFIOIovaRange *iova_range;
+
+iova_range = g_malloc0(sizeof(*iova_range));
+iova_range->iova = iova;
+iova_range->size = iotlb->addr_mask + 1;
+iova_range->ram_addr = ram_addr;
+
+QLIST_INSERT_HEAD(&giommu->iova_list, iova_range, next);
}
} else {
+VFIOIovaRange *iova_range, *tmp;
+
+QLIST_FOREACH_SAFE(iova_range, &giommu->iova_list, next, tmp) {
+if (iova_range->iova >= iova &&
+iova_range->iova + iova_range->size <= iova +
+ iotlb->addr_mask + 1) {
+QLIST_REMOVE(iova_range, next);
+g_free(iova_range);
+}
+}
+
ret = vfio_dma_unmap(container, iova, iotlb->addr_mask + 1);
if (ret) {
error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
@@ -642,6 +673,7 @@ static void vfio_listener_region_add(MemoryListener
*listener,
g_free(giommu);
goto fail;
}
+QLIST_INIT(&giommu->iova_list);
QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
memory_region_iommu_replay(giommu->iommu, &giommu->n);
@@ -740,6 +772,13 @@ static void vfio_listener_region_del(MemoryListener
*listener,
QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
if (MEMORY_REGION(giommu->iommu) == section->mr &&
giommu->n.start == section->offset_within_region) {
+VFIOIovaRange *iova_range, *tmp;
+
+QLIST_FOREACH_SAFE(iova_range, &giommu->iova_list, next, tmp) {
+QLIST_REMOVE(iova_range, next);
+g_free(iova_range);
+}
+
memory_region_unregister_iommu_notifier(section->mr,
&giommu->n);
QLIST_REMOVE(giommu, giommu_next);
@@ -1541,6 +1580,13 @@ static void vfio_disconnect_container(VFIOGroup *group)
QLIST_REMOVE(container, next);
QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, tmp) {
+VFIOIovaRange *iova_range, *itmp;
+
+QLIST_FOREACH_SAFE(iova_range, &giommu->iova_list, next, itmp) {
+QLIST_REMOVE(iova_range, next);
+g_free(iova_range);
+}
+
memory_region_unregister_iommu_notifier(
MEMORY_REGION(giommu->iommu), &giommu->n)
[PATCH v26 12/17] vfio: Add function to start and stop dirty pages tracking
Call VFIO_IOMMU_DIRTY_PAGES ioctl to start and stop dirty pages tracking
for VFIO devices.
Signed-off-by: Kirti Wankhede
Reviewed-by: Dr. David Alan Gilbert
---
hw/vfio/migration.c | 36
1 file changed, 36 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 4306f6316417..822b68b4e015 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -11,6 +11,7 @@
#include "qemu/main-loop.h"
#include "qemu/cutils.h"
#include
+#include
#include "sysemu/runstate.h"
#include "hw/vfio/vfio-common.h"
@@ -355,6 +356,32 @@ static int vfio_load_device_config_state(QEMUFile *f, void
*opaque)
return qemu_file_get_error(f);
}
+static int vfio_set_dirty_page_tracking(VFIODevice *vbasedev, bool start)
+{
+int ret;
+VFIOContainer *container = vbasedev->group->container;
+struct vfio_iommu_type1_dirty_bitmap dirty = {
+.argsz = sizeof(dirty),
+};
+
+if (start) {
+if (vbasedev->device_state & VFIO_DEVICE_STATE_SAVING) {
+dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_START;
+} else {
+return -EINVAL;
+}
+} else {
+dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP;
+}
+
+ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
+if (ret) {
+error_report("Failed to set dirty tracking flag 0x%x errno: %d",
+ dirty.flags, errno);
+}
+return ret;
+}
+
/* -- */
static int vfio_save_setup(QEMUFile *f, void *opaque)
@@ -386,6 +413,11 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
return ret;
}
+ret = vfio_set_dirty_page_tracking(vbasedev, true);
+if (ret) {
+return ret;
+}
+
qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
ret = qemu_file_get_error(f);
@@ -401,6 +433,8 @@ static void vfio_save_cleanup(void *opaque)
VFIODevice *vbasedev = opaque;
VFIOMigration *migration = vbasedev->migration;
+vfio_set_dirty_page_tracking(vbasedev, false);
+
if (migration->region.mmaps) {
vfio_region_unmap(&migration->region);
}
@@ -734,6 +768,8 @@ static void vfio_migration_state_notifier(Notifier
*notifier, void *data)
if (ret) {
error_report("%s: Failed to set state RUNNING", vbasedev->name);
}
+
+vfio_set_dirty_page_tracking(vbasedev, false);
}
}
--
2.7.0
[PATCH v26 10/17] memory: Set DIRTY_MEMORY_MIGRATION when IOMMU is enabled
mr->ram_block is NULL when mr->is_iommu is true, then fr.dirty_log_mask
wasn't set correctly due to which memory listener's log_sync doesn't
get called.
This patch returns log_mask with DIRTY_MEMORY_MIGRATION set when
IOMMU is enabled.
Signed-off-by: Kirti Wankhede
---
softmmu/memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index d030eb6f7cea..0c1460394ace 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1777,7 +1777,7 @@ bool memory_region_is_ram_device(MemoryRegion *mr)
uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
{
uint8_t mask = mr->dirty_log_mask;
-if (global_dirty_log && mr->ram_block) {
+if (global_dirty_log && (mr->ram_block || memory_region_is_iommu(mr))) {
mask |= (1 << DIRTY_MEMORY_MIGRATION);
}
return mask;
--
2.7.0
[PATCH v26 08/17] vfio: Add save state functions to SaveVMHandlers
Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy
functions. These functions handles pre-copy and stop-and-copy phase.
In _SAVING|_RUNNING device state or pre-copy phase:
- read pending_bytes. If pending_bytes > 0, go through below steps.
- read data_offset - indicates kernel driver to write data to staging
buffer.
- read data_size - amount of data in bytes written by vendor driver in
migration region.
- read data_size bytes of data from data_offset in the migration region.
- Write data packet to file stream as below:
{VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data,
VFIO_MIG_FLAG_END_OF_STATE }
In _SAVING device state or stop-and-copy phase
a. read config space of device and save to migration file stream. This
doesn't need to be from vendor driver. Any other special config state
from driver can be saved as data in following iteration.
b. read pending_bytes. If pending_bytes > 0, go through below steps.
c. read data_offset - indicates kernel driver to write data to staging
buffer.
d. read data_size - amount of data in bytes written by vendor driver in
migration region.
e. read data_size bytes of data from data_offset in the migration region.
f. Write data packet as below:
{VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data}
g. iterate through steps b to f while (pending_bytes > 0)
h. Write {VFIO_MIG_FLAG_END_OF_STATE}
When data region is mapped, its user's responsibility to read data from
data_offset of data_size before moving to next steps.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
hw/vfio/migration.c | 273 ++
hw/vfio/trace-events | 6 +
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 280 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 8e8adaa25779..4611bb972228 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -180,6 +180,154 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
uint32_t mask,
return 0;
}
+static void *get_data_section_size(VFIORegion *region, uint64_t data_offset,
+ uint64_t data_size, uint64_t *size)
+{
+void *ptr = NULL;
+uint64_t limit = 0;
+int i;
+
+if (!region->mmaps) {
+if (size) {
+*size = data_size;
+}
+return ptr;
+}
+
+for (i = 0; i < region->nr_mmaps; i++) {
+VFIOMmap *map = region->mmaps + i;
+
+if ((data_offset >= map->offset) &&
+(data_offset < map->offset + map->size)) {
+
+/* check if data_offset is within sparse mmap areas */
+ptr = map->mmap + data_offset - map->offset;
+if (size) {
+*size = MIN(data_size, map->offset + map->size - data_offset);
+}
+break;
+} else if ((data_offset < map->offset) &&
+ (!limit || limit > map->offset)) {
+/*
+ * data_offset is not within sparse mmap areas, find size of
+ * non-mapped area. Check through all list since region->mmaps list
+ * is not sorted.
+ */
+limit = map->offset;
+}
+}
+
+if (!ptr && size) {
+*size = limit ? limit - data_offset : data_size;
+}
+return ptr;
+}
+
+static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev, uint64_t *size)
+{
+VFIOMigration *migration = vbasedev->migration;
+VFIORegion *region = &migration->region;
+uint64_t data_offset = 0, data_size = 0, sz;
+int ret;
+
+ret = vfio_mig_read(vbasedev, &data_offset, sizeof(data_offset),
+region->fd_offset + offsetof(struct vfio_device_migration_info,
+ data_offset));
+if (ret < 0) {
+return ret;
+}
+
+ret = vfio_mig_read(vbasedev, &data_size, sizeof(data_size),
+region->fd_offset + offsetof(struct vfio_device_migration_info,
+ data_size));
+if (ret < 0) {
+return ret;
+}
+
+trace_vfio_save_buffer(vbasedev->name, data_offset, data_size,
+ migration->pending_bytes);
+
+qemu_put_be64(f, data_size);
+sz = data_size;
+
+while (sz) {
+void *buf = NULL;
+uint64_t sec_size;
+bool buf_allocated = false;
+
+buf = get_data_section_size(region, data_offset, sz, &sec_size);
+
+if (!buf) {
+buf = g_try_malloc(sec_size);
+if (!buf) {
+error_report("%s: Error allocating buffer ", __func__);
+return -ENOMEM;
+}
+buf_allocated = true;
+
+ret = vfio_mig_read(vbasedev, buf, sec_size,
+region->fd_offset + data_offset);
+if (ret < 0) {
+g_free(buf);
+return ret;
+}
+}
+
+qemu_put_buffer(f, buf, sec_size);
+
+
[PATCH v26 09/17] vfio: Add load state functions to SaveVMHandlers
Sequence during _RESUMING device state:
While data for this device is available, repeat below steps:
a. read data_offset from where user application should write data.
b. write data of data_size to migration region from data_offset.
c. write data_size which indicates vendor driver that data is written in
staging buffer.
For user, data is opaque. User should write data in the same order as
received.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Reviewed-by: Dr. David Alan Gilbert
---
hw/vfio/migration.c | 170 +++
hw/vfio/trace-events | 3 +
2 files changed, 173 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 4611bb972228..ffd70282dd0e 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -328,6 +328,33 @@ static int vfio_save_device_config_state(QEMUFile *f, void
*opaque)
return qemu_file_get_error(f);
}
+static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
+{
+VFIODevice *vbasedev = opaque;
+uint64_t data;
+
+if (vbasedev->ops && vbasedev->ops->vfio_load_config) {
+int ret;
+
+ret = vbasedev->ops->vfio_load_config(vbasedev, f);
+if (ret) {
+error_report("%s: Failed to load device config space",
+ vbasedev->name);
+return ret;
+}
+}
+
+data = qemu_get_be64(f);
+if (data != VFIO_MIG_FLAG_END_OF_STATE) {
+error_report("%s: Failed loading device config space, "
+ "end flag incorrect 0x%"PRIx64, vbasedev->name, data);
+return -EINVAL;
+}
+
+trace_vfio_load_device_config_state(vbasedev->name);
+return qemu_file_get_error(f);
+}
+
/* -- */
static int vfio_save_setup(QEMUFile *f, void *opaque)
@@ -502,12 +529,155 @@ static int vfio_save_complete_precopy(QEMUFile *f, void
*opaque)
return ret;
}
+static int vfio_load_setup(QEMUFile *f, void *opaque)
+{
+VFIODevice *vbasedev = opaque;
+VFIOMigration *migration = vbasedev->migration;
+int ret = 0;
+
+if (migration->region.mmaps) {
+ret = vfio_region_mmap(&migration->region);
+if (ret) {
+error_report("%s: Failed to mmap VFIO migration region %d: %s",
+ vbasedev->name, migration->region.nr,
+ strerror(-ret));
+error_report("%s: Falling back to slow path", vbasedev->name);
+}
+}
+
+ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_MASK,
+ VFIO_DEVICE_STATE_RESUMING);
+if (ret) {
+error_report("%s: Failed to set state RESUMING", vbasedev->name);
+}
+return ret;
+}
+
+static int vfio_load_cleanup(void *opaque)
+{
+vfio_save_cleanup(opaque);
+return 0;
+}
+
+static int vfio_load_state(QEMUFile *f, void *opaque, int version_id)
+{
+VFIODevice *vbasedev = opaque;
+VFIOMigration *migration = vbasedev->migration;
+int ret = 0;
+uint64_t data, data_size;
+
+data = qemu_get_be64(f);
+while (data != VFIO_MIG_FLAG_END_OF_STATE) {
+
+trace_vfio_load_state(vbasedev->name, data);
+
+switch (data) {
+case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
+{
+ret = vfio_load_device_config_state(f, opaque);
+if (ret) {
+return ret;
+}
+break;
+}
+case VFIO_MIG_FLAG_DEV_SETUP_STATE:
+{
+data = qemu_get_be64(f);
+if (data == VFIO_MIG_FLAG_END_OF_STATE) {
+return ret;
+} else {
+error_report("%s: SETUP STATE: EOS not found 0x%"PRIx64,
+ vbasedev->name, data);
+return -EINVAL;
+}
+break;
+}
+case VFIO_MIG_FLAG_DEV_DATA_STATE:
+{
+VFIORegion *region = &migration->region;
+uint64_t data_offset = 0, size;
+
+data_size = size = qemu_get_be64(f);
+if (data_size == 0) {
+break;
+}
+
+ret = vfio_mig_read(vbasedev, &data_offset, sizeof(data_offset),
+region->fd_offset +
+offsetof(struct vfio_device_migration_info,
+ data_offset));
+if (ret < 0) {
+return ret;
+}
+
+trace_vfio_load_state_device_data(vbasedev->name, data_offset,
+ data_size);
+
+while (size) {
+void *buf = NULL;
+uint64_t sec_size;
+bool buf_alloc = false;
+
+buf = get_data_section_size(region, data_offset, size,
+&sec_size);
+
+if (!buf) {
+buf = g_try_malloc(sec_size);
+
[PATCH v26 11/17] vfio: Get migration capability flags for container
Added helper functions to get IOMMU info capability chain.
Added function to get migration capability information from that
capability chain for IOMMU container.
Similar change was proposed earlier:
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg03759.html
Disable migration for devices if IOMMU module doesn't support migration
capability.
Signed-off-by: Kirti Wankhede
Cc: Shameer Kolothum
Cc: Eric Auger
---
hw/vfio/common.c | 90 +++
hw/vfio/migration.c | 7 +++-
include/hw/vfio/vfio-common.h | 3 ++
3 files changed, 91 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index c6e98b8d61be..d4959c036dd1 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1228,6 +1228,75 @@ static int vfio_init_container(VFIOContainer *container,
int group_fd,
return 0;
}
+static int vfio_get_iommu_info(VFIOContainer *container,
+ struct vfio_iommu_type1_info **info)
+{
+
+size_t argsz = sizeof(struct vfio_iommu_type1_info);
+
+*info = g_new0(struct vfio_iommu_type1_info, 1);
+again:
+(*info)->argsz = argsz;
+
+if (ioctl(container->fd, VFIO_IOMMU_GET_INFO, *info)) {
+g_free(*info);
+*info = NULL;
+return -errno;
+}
+
+if (((*info)->argsz > argsz)) {
+argsz = (*info)->argsz;
+*info = g_realloc(*info, argsz);
+goto again;
+}
+
+return 0;
+}
+
+static struct vfio_info_cap_header *
+vfio_get_iommu_info_cap(struct vfio_iommu_type1_info *info, uint16_t id)
+{
+struct vfio_info_cap_header *hdr;
+void *ptr = info;
+
+if (!(info->flags & VFIO_IOMMU_INFO_CAPS)) {
+return NULL;
+}
+
+for (hdr = ptr + info->cap_offset; hdr != ptr; hdr = ptr + hdr->next) {
+if (hdr->id == id) {
+return hdr;
+}
+}
+
+return NULL;
+}
+
+static void vfio_get_iommu_info_migration(VFIOContainer *container,
+ struct vfio_iommu_type1_info *info)
+{
+struct vfio_info_cap_header *hdr;
+struct vfio_iommu_type1_info_cap_migration *cap_mig;
+
+hdr = vfio_get_iommu_info_cap(info, VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION);
+if (!hdr) {
+return;
+}
+
+cap_mig = container_of(hdr, struct vfio_iommu_type1_info_cap_migration,
+header);
+
+/*
+ * cpu_physical_memory_set_dirty_lebitmap() expects pages in bitmap of
+ * TARGET_PAGE_SIZE to mark those dirty.
+ */
+if (cap_mig->pgsize_bitmap & TARGET_PAGE_SIZE) {
+container->dirty_pages_supported = true;
+container->max_dirty_bitmap_size = cap_mig->max_dirty_bitmap_size;
+container->dirty_pgsizes = cap_mig->pgsize_bitmap;
+}
+}
+
static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
Error **errp)
{
@@ -1297,6 +1366,7 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
container->space = space;
container->fd = fd;
container->error = NULL;
+container->dirty_pages_supported = false;
QLIST_INIT(&container->giommu_list);
QLIST_INIT(&container->hostwin_list);
@@ -1309,7 +1379,7 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
case VFIO_TYPE1v2_IOMMU:
case VFIO_TYPE1_IOMMU:
{
-struct vfio_iommu_type1_info info;
+struct vfio_iommu_type1_info *info;
/*
* FIXME: This assumes that a Type1 IOMMU can map any 64-bit
@@ -1318,15 +1388,19 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
* existing Type1 IOMMUs generally support any IOVA we're
* going to actually try in practice.
*/
-info.argsz = sizeof(info);
-ret = ioctl(fd, VFIO_IOMMU_GET_INFO, &info);
-/* Ignore errors */
-if (ret || !(info.flags & VFIO_IOMMU_INFO_PGSIZES)) {
+ret = vfio_get_iommu_info(container, &info);
+
+if (ret || !(info->flags & VFIO_IOMMU_INFO_PGSIZES)) {
/* Assume 4k IOVA page size */
-info.iova_pgsizes = 4096;
+info->iova_pgsizes = 4096;
}
-vfio_host_win_add(container, 0, (hwaddr)-1, info.iova_pgsizes);
-container->pgsizes = info.iova_pgsizes;
+vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
+container->pgsizes = info->iova_pgsizes;
+
+if (!ret) {
+vfio_get_iommu_info_migration(container, info);
+}
+g_free(info);
break;
}
case VFIO_SPAPR_TCE_v2_IOMMU:
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index ffd70282dd0e..4306f6316417 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -786,9 +786,14 @@ static int vfio_migration_init(VFIODevice *vbasedev,
int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
{
+VFIOContainer *container = vbasedev->group->container;
struct v
[PATCH v26 17/17] qapi: Add VFIO devices migration stats in Migration stats
Added amount of bytes transferred to the target VM by all VFIO devices
Signed-off-by: Kirti Wankhede
---
Note: Comments from v25 for this patch are not addressed yet.
https://www.mail-archive.com/qemu-devel@nongnu.org/msg715620.html
Alex, need more pointer on documentation part raised Markus Armbruster.
hw/vfio/common.c| 20
hw/vfio/migration.c | 10 ++
include/qemu/vfio-helpers.h | 3 +++
migration/migration.c | 14 ++
monitor/hmp-cmds.c | 6 ++
qapi/migration.json | 17 +
6 files changed, 70 insertions(+)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7eeaa368187a..286cdaac8674 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -39,6 +39,7 @@
#include "trace.h"
#include "qapi/error.h"
#include "migration/migration.h"
+#include "qemu/vfio-helpers.h"
VFIOGroupList vfio_group_list =
QLIST_HEAD_INITIALIZER(vfio_group_list);
@@ -292,6 +293,25 @@ const MemoryRegionOps vfio_region_ops = {
* Device state interfaces
*/
+bool vfio_mig_active(void)
+{
+VFIOGroup *group;
+VFIODevice *vbasedev;
+
+if (QLIST_EMPTY(&vfio_group_list)) {
+return false;
+}
+
+QLIST_FOREACH(group, &vfio_group_list, next) {
+QLIST_FOREACH(vbasedev, &group->device_list, next) {
+if (vbasedev->migration_blocker) {
+return false;
+}
+}
+}
+return true;
+}
+
static bool vfio_devices_all_stopped_and_saving(VFIOContainer *container)
{
VFIOGroup *group;
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 822b68b4e015..c4226fa8b183 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -28,6 +28,7 @@
#include "pci.h"
#include "trace.h"
#include "hw/hw.h"
+#include "qemu/vfio-helpers.h"
/*
* Flags used as delimiter:
@@ -40,6 +41,8 @@
#define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xef13ULL)
#define VFIO_MIG_FLAG_DEV_DATA_STATE(0xef14ULL)
+static int64_t bytes_transferred;
+
static inline int vfio_mig_access(VFIODevice *vbasedev, void *val, int count,
off_t off, bool iswrite)
{
@@ -289,6 +292,7 @@ static int vfio_save_buffer(QEMUFile *f, VFIODevice
*vbasedev, uint64_t *size)
*size = data_size;
}
+bytes_transferred += data_size;
return ret;
}
@@ -770,6 +774,7 @@ static void vfio_migration_state_notifier(Notifier
*notifier, void *data)
}
vfio_set_dirty_page_tracking(vbasedev, false);
+bytes_transferred = 0;
}
}
@@ -820,6 +825,11 @@ static int vfio_migration_init(VFIODevice *vbasedev,
/* -- */
+int64_t vfio_mig_bytes_transferred(void)
+{
+return bytes_transferred;
+}
+
int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
{
VFIOContainer *container = vbasedev->group->container;
diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
index 1f057c2b9e40..26a7df0767b1 100644
--- a/include/qemu/vfio-helpers.h
+++ b/include/qemu/vfio-helpers.h
@@ -29,4 +29,7 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index,
void *bar,
int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
int irq_type, Error **errp);
+bool vfio_mig_active(void);
+int64_t vfio_mig_bytes_transferred(void);
+
#endif
diff --git a/migration/migration.c b/migration/migration.c
index 58a5452471f9..b204bb1f6cd9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -56,6 +56,7 @@
#include "net/announce.h"
#include "qemu/queue.h"
#include "multifd.h"
+#include "qemu/vfio-helpers.h"
#define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */
@@ -996,6 +997,17 @@ static void populate_disk_info(MigrationInfo *info)
}
}
+static void populate_vfio_info(MigrationInfo *info)
+{
+#ifdef CONFIG_LINUX
+if (vfio_mig_active()) {
+info->has_vfio = true;
+info->vfio = g_malloc0(sizeof(*info->vfio));
+info->vfio->transferred = vfio_mig_bytes_transferred();
+}
+#endif
+}
+
static void fill_source_migration_info(MigrationInfo *info)
{
MigrationState *s = migrate_get_current();
@@ -1020,6 +1032,7 @@ static void fill_source_migration_info(MigrationInfo
*info)
populate_time_info(info, s);
populate_ram_info(info, s);
populate_disk_info(info);
+populate_vfio_info(info);
break;
case MIGRATION_STATUS_COLO:
info->has_status = true;
@@ -1028,6 +1041,7 @@ static void fill_source_migration_info(MigrationInfo
*info)
case MIGRATION_STATUS_COMPLETED:
populate_time_info(info, s);
populate_ram_info(info, s);
+populate_vfio_info(info);
break;
case MIGRATION_STATUS_FAILED:
info->has_status = true;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 7711726fd222..40d60d6a6651 100644
--- a/
[PATCH v26 06/17] vfio: Add migration state change notifier
Added migration state change notifier to get notification on migration state
change. These states are translated to VFIO device state and conveyed to vendor
driver.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Reviewed-by: Dr. David Alan Gilbert
---
hw/vfio/migration.c | 29 +
hw/vfio/trace-events | 5 +++--
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index a30d628ba963..f650fe9fc3c8 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -199,6 +199,28 @@ static void vfio_vmstate_change(void *opaque, int running,
RunState state)
}
}
+static void vfio_migration_state_notifier(Notifier *notifier, void *data)
+{
+MigrationState *s = data;
+VFIODevice *vbasedev = container_of(notifier, VFIODevice, migration_state);
+int ret;
+
+trace_vfio_migration_state_notifier(vbasedev->name,
+MigrationStatus_str(s->state));
+
+switch (s->state) {
+case MIGRATION_STATUS_CANCELLING:
+case MIGRATION_STATUS_CANCELLED:
+case MIGRATION_STATUS_FAILED:
+ret = vfio_migration_set_state(vbasedev,
+ ~(VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RESUMING),
+ VFIO_DEVICE_STATE_RUNNING);
+if (ret) {
+error_report("%s: Failed to set state RUNNING", vbasedev->name);
+}
+}
+}
+
static int vfio_migration_init(VFIODevice *vbasedev,
struct vfio_region_info *info)
{
@@ -221,6 +243,8 @@ static int vfio_migration_init(VFIODevice *vbasedev,
vbasedev->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
vbasedev);
+vbasedev->migration_state.notify = vfio_migration_state_notifier;
+add_migration_state_change_notifier(&vbasedev->migration_state);
return ret;
}
@@ -263,6 +287,11 @@ add_blocker:
void vfio_migration_finalize(VFIODevice *vbasedev)
{
+
+if (vbasedev->migration_state.notify) {
+remove_migration_state_change_notifier(&vbasedev->migration_state);
+}
+
if (vbasedev->vm_state) {
qemu_del_vm_change_state_handler(vbasedev->vm_state);
}
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 6524734bf7b4..bcb3fa7314d7 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -149,5 +149,6 @@ vfio_display_edid_write_error(void) ""
# migration.c
vfio_migration_probe(const char *name, uint32_t index) " (%s) Region %d"
-vfio_migration_set_state(char *name, uint32_t state) " (%s) state %d"
-vfio_vmstate_change(char *name, int running, const char *reason, uint32_t
dev_state) " (%s) running %d reason %s device state %d"
+vfio_migration_set_state(const char *name, uint32_t state) " (%s) state %d"
+vfio_vmstate_change(const char *name, int running, const char *reason,
uint32_t dev_state) " (%s) running %d reason %s device state %d"
+vfio_migration_state_notifier(const char *name, const char *state) " (%s)
state %s"
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 25e3b1a3b90a..49c7c7a0e29a 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -123,6 +123,7 @@ typedef struct VFIODevice {
VMChangeStateEntry *vm_state;
uint32_t device_state;
int vm_running;
+Notifier migration_state;
} VFIODevice;
struct VFIODeviceOps {
--
2.7.0
[PATCH v26 05/17] vfio: Add VM state change handler to know state of VM
VM state change handler gets called on change in VM's state. This is used to set
VFIO device state to _RUNNING.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Reviewed-by: Dr. David Alan Gilbert
---
hw/vfio/migration.c | 136 ++
hw/vfio/trace-events | 3 +-
include/hw/vfio/vfio-common.h | 4 ++
3 files changed, 142 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 2f760f1f9c47..a30d628ba963 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include
+#include "sysemu/runstate.h"
#include "hw/vfio/vfio-common.h"
#include "cpu.h"
#include "migration/migration.h"
@@ -22,6 +23,58 @@
#include "exec/ram_addr.h"
#include "pci.h"
#include "trace.h"
+#include "hw/hw.h"
+
+static inline int vfio_mig_access(VFIODevice *vbasedev, void *val, int count,
+ off_t off, bool iswrite)
+{
+int ret;
+
+ret = iswrite ? pwrite(vbasedev->fd, val, count, off) :
+pread(vbasedev->fd, val, count, off);
+if (ret < count) {
+error_report("vfio_mig_%s%d %s: failed at offset 0x%lx, err: %s",
+ iswrite ? "write" : "read", count * 8,
+ vbasedev->name, off, strerror(errno));
+return (ret < 0) ? ret : -EINVAL;
+}
+return 0;
+}
+
+static int vfio_mig_rw(VFIODevice *vbasedev, __u8 *buf, size_t count,
+ off_t off, bool iswrite)
+{
+int ret, done = 0;
+__u8 *tbuf = buf;
+
+while (count) {
+int bytes = 0;
+
+if (count >= 8 && !(off % 8)) {
+bytes = 8;
+} else if (count >= 4 && !(off % 4)) {
+bytes = 4;
+} else if (count >= 2 && !(off % 2)) {
+bytes = 2;
+} else {
+bytes = 1;
+}
+
+ret = vfio_mig_access(vbasedev, tbuf, bytes, off, iswrite);
+if (ret) {
+return ret;
+}
+
+count -= bytes;
+done += bytes;
+off += bytes;
+tbuf += bytes;
+}
+return done;
+}
+
+#define vfio_mig_read(f, v, c, o) vfio_mig_rw(f, (__u8 *)v, c, o, false)
+#define vfio_mig_write(f, v, c, o) vfio_mig_rw(f, (__u8 *)v, c, o, true)
static void vfio_migration_region_exit(VFIODevice *vbasedev)
{
@@ -70,6 +123,82 @@ err:
return ret;
}
+static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask,
+uint32_t value)
+{
+VFIOMigration *migration = vbasedev->migration;
+VFIORegion *region = &migration->region;
+off_t dev_state_off = region->fd_offset +
+ offsetof(struct vfio_device_migration_info,
device_state);
+uint32_t device_state;
+int ret;
+
+ret = vfio_mig_read(vbasedev, &device_state, sizeof(device_state),
+dev_state_off);
+if (ret < 0) {
+return ret;
+}
+
+device_state = (device_state & mask) | value;
+
+if (!VFIO_DEVICE_STATE_VALID(device_state)) {
+return -EINVAL;
+}
+
+ret = vfio_mig_write(vbasedev, &device_state, sizeof(device_state),
+ dev_state_off);
+if (ret < 0) {
+ret = vfio_mig_read(vbasedev, &device_state, sizeof(device_state),
+ dev_state_off);
+if (ret < 0) {
+return ret;
+}
+
+if (VFIO_DEVICE_STATE_IS_ERROR(device_state)) {
+hw_error("%s: Device is in error state 0x%x",
+ vbasedev->name, device_state);
+return -EFAULT;
+}
+}
+
+vbasedev->device_state = device_state;
+trace_vfio_migration_set_state(vbasedev->name, device_state);
+return 0;
+}
+
+static void vfio_vmstate_change(void *opaque, int running, RunState state)
+{
+VFIODevice *vbasedev = opaque;
+
+if ((vbasedev->vm_running != running)) {
+int ret;
+uint32_t value = 0, mask = 0;
+
+if (running) {
+value = VFIO_DEVICE_STATE_RUNNING;
+if (vbasedev->device_state & VFIO_DEVICE_STATE_RESUMING) {
+mask = ~VFIO_DEVICE_STATE_RESUMING;
+}
+} else {
+mask = ~VFIO_DEVICE_STATE_RUNNING;
+}
+
+ret = vfio_migration_set_state(vbasedev, mask, value);
+if (ret) {
+/*
+ * vm_state_notify() doesn't support reporting failure. If such
+ * error reporting support added in furure, migration should be
+ * aborted.
+ */
+error_report("%s: Failed to set device state 0x%x",
+ vbasedev->name, value & mask);
+}
+vbasedev->vm_running = running;
+trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
+ value & mask);
+}
+}
+
static int vfio_migration_init(VFIODevice *vbasedev,
[PATCH v26 04/17] vfio: Add migration region initialization and finalize function
Whether the VFIO device supports migration or not is decided based of
migration region query. If migration region query is successful and migration
region initialization is successful then migration is supported else
migration is blocked.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Acked-by: Dr. David Alan Gilbert
---
hw/vfio/meson.build | 1 +
hw/vfio/migration.c | 142 ++
hw/vfio/trace-events | 5 ++
include/hw/vfio/vfio-common.h | 9 +++
4 files changed, 157 insertions(+)
create mode 100644 hw/vfio/migration.c
diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build
index 37efa74018bc..da9af297a0c5 100644
--- a/hw/vfio/meson.build
+++ b/hw/vfio/meson.build
@@ -2,6 +2,7 @@ vfio_ss = ss.source_set()
vfio_ss.add(files(
'common.c',
'spapr.c',
+ 'migration.c',
))
vfio_ss.add(when: 'CONFIG_VFIO_PCI', if_true: files(
'display.c',
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
new file mode 100644
index ..2f760f1f9c47
--- /dev/null
+++ b/hw/vfio/migration.c
@@ -0,0 +1,142 @@
+/*
+ * Migration support for VFIO devices
+ *
+ * Copyright NVIDIA, Inc. 2020
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include
+
+#include "hw/vfio/vfio-common.h"
+#include "cpu.h"
+#include "migration/migration.h"
+#include "migration/qemu-file.h"
+#include "migration/register.h"
+#include "migration/blocker.h"
+#include "migration/misc.h"
+#include "qapi/error.h"
+#include "exec/ramlist.h"
+#include "exec/ram_addr.h"
+#include "pci.h"
+#include "trace.h"
+
+static void vfio_migration_region_exit(VFIODevice *vbasedev)
+{
+VFIOMigration *migration = vbasedev->migration;
+
+if (!migration) {
+return;
+}
+
+if (migration->region.size) {
+vfio_region_exit(&migration->region);
+vfio_region_finalize(&migration->region);
+}
+}
+
+static int vfio_migration_region_init(VFIODevice *vbasedev, int index)
+{
+VFIOMigration *migration = vbasedev->migration;
+Object *obj = NULL;
+int ret = -EINVAL;
+
+obj = vbasedev->ops->vfio_get_object(vbasedev);
+if (!obj) {
+return ret;
+}
+
+ret = vfio_region_setup(obj, vbasedev, &migration->region, index,
+"migration");
+if (ret) {
+error_report("%s: Failed to setup VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
+goto err;
+}
+
+if (!migration->region.size) {
+ret = -EINVAL;
+error_report("%s: Invalid region size of VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
+goto err;
+}
+
+return 0;
+
+err:
+vfio_migration_region_exit(vbasedev);
+return ret;
+}
+
+static int vfio_migration_init(VFIODevice *vbasedev,
+ struct vfio_region_info *info)
+{
+int ret = -EINVAL;
+
+if (!vbasedev->ops->vfio_get_object) {
+return ret;
+}
+
+vbasedev->migration = g_new0(VFIOMigration, 1);
+
+ret = vfio_migration_region_init(vbasedev, info->index);
+if (ret) {
+error_report("%s: Failed to initialise migration region",
+ vbasedev->name);
+g_free(vbasedev->migration);
+vbasedev->migration = NULL;
+}
+
+return ret;
+}
+
+/* -- */
+
+int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
+{
+struct vfio_region_info *info = NULL;
+Error *local_err = NULL;
+int ret;
+
+ret = vfio_get_dev_region_info(vbasedev, VFIO_REGION_TYPE_MIGRATION,
+ VFIO_REGION_SUBTYPE_MIGRATION, &info);
+if (ret) {
+goto add_blocker;
+}
+
+ret = vfio_migration_init(vbasedev, info);
+if (ret) {
+goto add_blocker;
+}
+
+g_free(info);
+trace_vfio_migration_probe(vbasedev->name, info->index);
+return 0;
+
+add_blocker:
+error_setg(&vbasedev->migration_blocker,
+ "VFIO device doesn't support migration");
+g_free(info);
+
+ret = migrate_add_blocker(vbasedev->migration_blocker, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+error_free(vbasedev->migration_blocker);
+vbasedev->migration_blocker = NULL;
+}
+return ret;
+}
+
+void vfio_migration_finalize(VFIODevice *vbasedev)
+{
+if (vbasedev->migration_blocker) {
+migrate_del_blocker(vbasedev->migration_blocker);
+error_free(vbasedev->migration_blocker);
+vbasedev->migration_blocker = NULL;
+}
+
+vfio_migration_region_exit(vbasedev);
+g_free(vbasedev->migration);
+}
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index a0c7b49a2ebc..8fe913175d85 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -145,3 +145,8
[PATCH v26 02/17] vfio: Add vfio_get_object callback to VFIODeviceOps
Hook vfio_get_object callback for PCI devices.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Suggested-by: Cornelia Huck
Reviewed-by: Cornelia Huck
---
hw/vfio/pci.c | 8
include/hw/vfio/vfio-common.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 0d83eb0e47bb..bffd5bfe3b78 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2394,10 +2394,18 @@ static void vfio_pci_compute_needs_reset(VFIODevice
*vbasedev)
}
}
+static Object *vfio_pci_get_object(VFIODevice *vbasedev)
+{
+VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+
+return OBJECT(vdev);
+}
+
static VFIODeviceOps vfio_pci_ops = {
.vfio_compute_needs_reset = vfio_pci_compute_needs_reset,
.vfio_hot_reset_multi = vfio_pci_hot_reset_multi,
.vfio_eoi = vfio_intx_eoi,
+.vfio_get_object = vfio_pci_get_object,
};
int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index dc95f527b583..fe99c36a693a 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -119,6 +119,7 @@ struct VFIODeviceOps {
void (*vfio_compute_needs_reset)(VFIODevice *vdev);
int (*vfio_hot_reset_multi)(VFIODevice *vdev);
void (*vfio_eoi)(VFIODevice *vdev);
+Object *(*vfio_get_object)(VFIODevice *vdev);
};
typedef struct VFIOGroup {
--
2.7.0
[PATCH v26 07/17] vfio: Register SaveVMHandlers for VFIO device
Define flags to be used as delimeter in migration file stream.
Added .save_setup and .save_cleanup functions. Mapped & unmapped migration
region from these functions at source during saving or pre-copy phase.
Set VFIO device state depending on VM's state. During live migration, VM is
running when .save_setup is called, _SAVING | _RUNNING state is set for VFIO
device. During save-restore, VM is paused, _SAVING state is set for VFIO device.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
hw/vfio/migration.c | 91
hw/vfio/trace-events | 2 ++
2 files changed, 93 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index f650fe9fc3c8..8e8adaa25779 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -8,12 +8,15 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "qemu/cutils.h"
#include
#include "sysemu/runstate.h"
#include "hw/vfio/vfio-common.h"
#include "cpu.h"
#include "migration/migration.h"
+#include "migration/vmstate.h"
#include "migration/qemu-file.h"
#include "migration/register.h"
#include "migration/blocker.h"
@@ -25,6 +28,17 @@
#include "trace.h"
#include "hw/hw.h"
+/*
+ * Flags used as delimiter:
+ * 0x => MSB 32-bit all 1s
+ * 0xef10 => emulated (virtual) function IO
+ * 0x => 16-bits reserved for flags
+ */
+#define VFIO_MIG_FLAG_END_OF_STATE (0xef11ULL)
+#define VFIO_MIG_FLAG_DEV_CONFIG_STATE (0xef12ULL)
+#define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xef13ULL)
+#define VFIO_MIG_FLAG_DEV_DATA_STATE(0xef14ULL)
+
static inline int vfio_mig_access(VFIODevice *vbasedev, void *val, int count,
off_t off, bool iswrite)
{
@@ -166,6 +180,65 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
uint32_t mask,
return 0;
}
+/* -- */
+
+static int vfio_save_setup(QEMUFile *f, void *opaque)
+{
+VFIODevice *vbasedev = opaque;
+VFIOMigration *migration = vbasedev->migration;
+int ret;
+
+trace_vfio_save_setup(vbasedev->name);
+
+qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
+
+if (migration->region.mmaps) {
+qemu_mutex_lock_iothread();
+ret = vfio_region_mmap(&migration->region);
+qemu_mutex_unlock_iothread();
+if (ret) {
+error_report("%s: Failed to mmap VFIO migration region %d: %s",
+ vbasedev->name, migration->region.nr,
+ strerror(-ret));
+error_report("%s: Falling back to slow path", vbasedev->name);
+}
+}
+
+ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_MASK,
+ VFIO_DEVICE_STATE_SAVING);
+if (ret) {
+error_report("%s: Failed to set state SAVING", vbasedev->name);
+return ret;
+}
+
+qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
+
+ret = qemu_file_get_error(f);
+if (ret) {
+return ret;
+}
+
+return 0;
+}
+
+static void vfio_save_cleanup(void *opaque)
+{
+VFIODevice *vbasedev = opaque;
+VFIOMigration *migration = vbasedev->migration;
+
+if (migration->region.mmaps) {
+vfio_region_unmap(&migration->region);
+}
+trace_vfio_save_cleanup(vbasedev->name);
+}
+
+static SaveVMHandlers savevm_vfio_handlers = {
+.save_setup = vfio_save_setup,
+.save_cleanup = vfio_save_cleanup,
+};
+
+/* -- */
+
static void vfio_vmstate_change(void *opaque, int running, RunState state)
{
VFIODevice *vbasedev = opaque;
@@ -225,6 +298,8 @@ static int vfio_migration_init(VFIODevice *vbasedev,
struct vfio_region_info *info)
{
int ret = -EINVAL;
+char id[256] = "";
+Object *obj;
if (!vbasedev->ops->vfio_get_object) {
return ret;
@@ -241,6 +316,22 @@ static int vfio_migration_init(VFIODevice *vbasedev,
return ret;
}
+obj = vbasedev->ops->vfio_get_object(vbasedev);
+
+if (obj) {
+DeviceState *dev = DEVICE(obj);
+char *oid = vmstate_if_get_id(VMSTATE_IF(dev));
+
+if (oid) {
+pstrcpy(id, sizeof(id), oid);
+pstrcat(id, sizeof(id), "/");
+g_free(oid);
+}
+}
+pstrcat(id, sizeof(id), "vfio");
+
+register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, &savevm_vfio_handlers,
+ vbasedev);
vbasedev->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
vbasedev);
vbasedev->migration_state.notify = vfio_migration_state_notifier;
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index bcb3fa7314d7..982d8dccb219 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -152,3 +152,5 @@ vfio_migration_probe(co
[PATCH v26 03/17] vfio: Add save and load functions for VFIO PCI devices
These functions save and restore PCI device specific data - config
space of PCI device.
Used VMStateDescription to save and restore interrupt state.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
hw/vfio/pci.c | 134 ++
hw/vfio/pci.h | 1 +
include/hw/vfio/vfio-common.h | 2 +
3 files changed, 137 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bffd5bfe3b78..9968cc553391 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -41,6 +41,7 @@
#include "trace.h"
#include "qapi/error.h"
#include "migration/blocker.h"
+#include "migration/qemu-file.h"
#define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
@@ -2401,11 +2402,142 @@ static Object *vfio_pci_get_object(VFIODevice
*vbasedev)
return OBJECT(vdev);
}
+static int vfio_get_pci_irq_state(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field)
+{
+VFIOPCIDevice *vdev = container_of(pv, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+uint32_t interrupt_type;
+
+interrupt_type = qemu_get_be32(f);
+
+if (interrupt_type == VFIO_INT_MSI) {
+uint32_t msi_flags, msi_addr_lo, msi_addr_hi = 0, msi_data;
+bool msi_64bit;
+
+/* restore msi configuration */
+msi_flags = pci_default_read_config(pdev,
+pdev->msi_cap + PCI_MSI_FLAGS, 2);
+msi_64bit = (msi_flags & PCI_MSI_FLAGS_64BIT);
+
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
+ msi_flags & ~PCI_MSI_FLAGS_ENABLE, 2);
+
+msi_addr_lo = pci_default_read_config(pdev,
+pdev->msi_cap + PCI_MSI_ADDRESS_LO, 4);
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_ADDRESS_LO,
+ msi_addr_lo, 4);
+
+if (msi_64bit) {
+msi_addr_hi = pci_default_read_config(pdev,
+pdev->msi_cap + PCI_MSI_ADDRESS_HI, 4);
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_ADDRESS_HI,
+ msi_addr_hi, 4);
+}
+
+msi_data = pci_default_read_config(pdev,
+pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
PCI_MSI_DATA_32),
+2);
+
+vfio_pci_write_config(pdev,
+pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
PCI_MSI_DATA_32),
+msi_data, 2);
+
+vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
+ msi_flags | PCI_MSI_FLAGS_ENABLE, 2);
+} else if (interrupt_type == VFIO_INT_MSIX) {
+uint16_t offset;
+
+msix_load(pdev, f);
+offset = pci_default_read_config(pdev,
+ pdev->msix_cap + PCI_MSIX_FLAGS + 1, 2);
+/* load enable bit and maskall bit */
+vfio_pci_write_config(pdev, pdev->msix_cap + PCI_MSIX_FLAGS + 1,
+ offset, 2);
+}
+return 0;
+}
+
+static int vfio_put_pci_irq_state(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, QJSON *vmdesc)
+{
+VFIOPCIDevice *vdev = container_of(pv, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+
+qemu_put_be32(f, vdev->interrupt);
+if (vdev->interrupt == VFIO_INT_MSIX) {
+msix_save(pdev, f);
+}
+
+return 0;
+}
+
+static const VMStateInfo vmstate_info_vfio_pci_irq_state = {
+.name = "VFIO PCI irq state",
+.get = vfio_get_pci_irq_state,
+.put = vfio_put_pci_irq_state,
+};
+
+const VMStateDescription vmstate_vfio_pci_config = {
+.name = "VFIOPCIDevice",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_INT32_POSITIVE_LE(version_id, VFIOPCIDevice),
+VMSTATE_BUFFER_UNSAFE_INFO(interrupt, VFIOPCIDevice, 1,
+ vmstate_info_vfio_pci_irq_state,
+ sizeof(int32_t)),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
+{
+VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+
+
+pci_device_save(pdev, f);
+vmstate_save_state(f, &vmstate_vfio_pci_config, vbasedev, NULL);
+}
+
+static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
+{
+VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+PCIDevice *pdev = &vdev->pdev;
+uint16_t pci_cmd;
+int ret, i;
+
+ret = pci_device_load(pdev, f);
+if (ret) {
+return ret;
+}
+
+/* retore pci bar configuration */
+pci_cmd = pci_default_read_config(pdev, PCI_COMMAND, 2);
+vfio_pci_write_config(pdev, PCI_COMMAND,
+pci_cmd & ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY), 2);
+for (i = 0; i < PCI_ROM_SLOT; i++) {
+
[PATCH v26 01/17] vfio: Add function to unmap VFIO region
This function will be used for migration region.
Migration region is mmaped when migration starts and will be unmapped when
migration is complete.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Reviewed-by: Cornelia Huck
---
hw/vfio/common.c | 32
hw/vfio/trace-events | 1 +
include/hw/vfio/vfio-common.h | 1 +
3 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 13471ae29436..c6e98b8d61be 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -924,6 +924,18 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev,
VFIORegion *region,
return 0;
}
+static void vfio_subregion_unmap(VFIORegion *region, int index)
+{
+trace_vfio_region_unmap(memory_region_name(®ion->mmaps[index].mem),
+region->mmaps[index].offset,
+region->mmaps[index].offset +
+region->mmaps[index].size - 1);
+memory_region_del_subregion(region->mem, ®ion->mmaps[index].mem);
+munmap(region->mmaps[index].mmap, region->mmaps[index].size);
+object_unparent(OBJECT(®ion->mmaps[index].mem));
+region->mmaps[index].mmap = NULL;
+}
+
int vfio_region_mmap(VFIORegion *region)
{
int i, prot = 0;
@@ -954,10 +966,7 @@ int vfio_region_mmap(VFIORegion *region)
region->mmaps[i].mmap = NULL;
for (i--; i >= 0; i--) {
-memory_region_del_subregion(region->mem,
®ion->mmaps[i].mem);
-munmap(region->mmaps[i].mmap, region->mmaps[i].size);
-object_unparent(OBJECT(®ion->mmaps[i].mem));
-region->mmaps[i].mmap = NULL;
+vfio_subregion_unmap(region, i);
}
return ret;
@@ -982,6 +991,21 @@ int vfio_region_mmap(VFIORegion *region)
return 0;
}
+void vfio_region_unmap(VFIORegion *region)
+{
+int i;
+
+if (!region->mem) {
+return;
+}
+
+for (i = 0; i < region->nr_mmaps; i++) {
+if (region->mmaps[i].mmap) {
+vfio_subregion_unmap(region, i);
+}
+}
+}
+
void vfio_region_exit(VFIORegion *region)
{
int i;
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 93a0bc2522f8..a0c7b49a2ebc 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -113,6 +113,7 @@ vfio_region_mmap(const char *name, unsigned long offset,
unsigned long end) "Reg
vfio_region_exit(const char *name, int index) "Device %s, region %d"
vfio_region_finalize(const char *name, int index) "Device %s, region %d"
vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region %s mmaps
enabled: %d"
+vfio_region_unmap(const char *name, unsigned long offset, unsigned long end)
"Region %s unmap [0x%lx - 0x%lx]"
vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas)
"Device %s region %d: %d sparse mmap entries"
vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end)
"sparse entry %d [0x%lx - 0x%lx]"
vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t
subtype) "%s index %d, %08x/%0x8"
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index c78f3ff5593c..dc95f527b583 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -171,6 +171,7 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev,
VFIORegion *region,
int index, const char *name);
int vfio_region_mmap(VFIORegion *region);
void vfio_region_mmaps_set_enabled(VFIORegion *region, bool enabled);
+void vfio_region_unmap(VFIORegion *region);
void vfio_region_exit(VFIORegion *region);
void vfio_region_finalize(VFIORegion *region);
void vfio_reset_handler(void *opaque);
--
2.7.0
[PATCH QEMU v25 00/17] Add migration support for VFIO devices
Hi, This Patch set adds migration support for VFIO devices in QEMU. This Patch set include patches as below: Patch 1-2: - Few code refactor Patch 3: - Added save and restore functions for PCI configuration space. Used pci_device_save() and pci_device_load() so that config space cache is saved and restored. Patch 4-9: - Generic migration functionality for VFIO device. * This patch set adds functionality for PCI devices, but can be extended to other VFIO devices. * Added all the basic functions required for pre-copy, stop-and-copy and resume phases of migration. * Added state change notifier and from that notifier function, VFIO device's state changed is conveyed to VFIO device driver. * During save setup phase and resume/load setup phase, migration region is queried and is used to read/write VFIO device data. * .save_live_pending and .save_live_iterate are implemented to use QEMU's functionality of iteration during pre-copy phase. * In .save_live_complete_precopy, that is in stop-and-copy phase, iteration to read data from VFIO device driver is implemented till pending bytes returned by driver are not zero. Patch 10 - Set DIRTY_MEMORY_MIGRATION flag in dirty log mask for migration with vIOMMU enabled. Patch 11: - Get migration capability from kernel module. Patch 12-14: - Add function to start and stop dirty pages tracking. - Add vfio_listerner_log_sync to mark dirty pages. Dirty pages bitmap is queried per container. All pages pinned by vendor driver through vfio_pin_pages external API has to be marked as dirty during migration. When there are CPU writes, CPU dirty page tracking can identify dirtied pages, but any page pinned by vendor driver can also be written by device. As of now there is no device which has hardware support for dirty page tracking. So all pages which are pinned by vendor driver should be considered as dirty. In Qemu, marking pages dirty is only done when device is in stop-and-copy phase because if pages are marked dirty during pre-copy phase and content is transfered from source to distination, there is no way to know newly dirtied pages from the point they were copied earlier until device stops. To avoid repeated copy of same content, pinned pages are marked dirty only during stop-and-copy phase. Patch 15: - With vIOMMU, IO virtual address range can get unmapped while in pre-copy phase of migration. In that case, unmap ioctl should return pages pinned in that range and QEMU should report corresponding guest physical pages dirty. Note: Comments from v25 for this patch are not addressed in this series. https://www.mail-archive.com/qemu-devel@nongnu.org/msg714646.html Patch 16: - Make VFIO PCI device migration capable. If migration region is not provided by driver, migration is blocked. Patch 17: - Added VFIO device stats to MigrationInfo Note: Comments from v25 for this patch are not addressed yet. https://www.mail-archive.com/qemu-devel@nongnu.org/msg715620.html Yet TODO: Since there is no device which has hardware support for system memmory dirty bitmap tracking, right now there is no other API from vendor driver to VFIO IOMMU module to report dirty pages. In future, when such hardware support will be implemented, an API will be required in kernel such that vendor driver could report dirty pages to VFIO module during migration phases. Below is the flow of state change for live migration where states in brackets represent VM state, migration state and VFIO device state as: (VM state, MIGRATION_STATUS, VFIO_DEVICE_STATE) Live migration save path: QEMU normal running state (RUNNING, _NONE, _RUNNING) | migrate_init spawns migration_thread. (RUNNING, _SETUP, _RUNNING|_SAVING) Migration thread then calls each device's .save_setup() | (RUNNING, _ACTIVE, _RUNNING|_SAVING) If device is active, get pending bytes by .save_live_pending() if pending bytes >= threshold_size, call save_live_iterate() Data of VFIO device for pre-copy phase is copied. Iterate till pending bytes converge and are less than threshold | On migration completion, vCPUs stops and calls .save_live_complete_precopy for each active device. VFIO device is then transitioned in _SAVING state. (FINISH_MIGRATE, _DEVICE, _SAVING) For VFIO device, iterate in .save_live_complete_precopy until pending data is 0. (FINISH_MIGRATE, _DEVICE, _STOPPED) | (FINISH_MIGRATE, _COMPLETED, STOPPED) Migraton thread schedule cleanup bottom half and exit Live migration resume path: Incomming migration calls .load_setup for each device (RESTORE_VM, _ACTIVE, STOPPED) | For each device, .load_state is called for that device section data | At the end, called .load_cleanup for each device and vCPUs are started.
Re: [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation
On Tue, Sep 22, 2020 at 05:00:25PM -0400, John Snow wrote:
> This is a minor re-work of the entrypoint script. It isolates a
> generate() method from the actual command-line mechanism.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi-gen.py | 87 -
> 1 file changed, 63 insertions(+), 24 deletions(-)
>
> diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> index 4b03f7d53b..59becba3e1 100644
> --- a/scripts/qapi-gen.py
> +++ b/scripts/qapi-gen.py
> @@ -1,9 +1,13 @@
> #!/usr/bin/env python3
> -# QAPI generator
> -#
> +
> # This work is licensed under the terms of the GNU GPL, version 2 or later.
> # See the COPYING file in the top-level directory.
>
> +"""
> +QAPI Generator
> +
> +This script is the main entry point for generating C code from the QAPI
> schema.
> +"""
>
> import argparse
> import re
> @@ -11,21 +15,65 @@
>
> from qapi.commands import gen_commands
> from qapi.doc import gen_doc
> +from qapi.error import QAPIError
> from qapi.events import gen_events
> from qapi.introspect import gen_introspect
> -from qapi.schema import QAPIError, QAPISchema
> +from qapi.schema import QAPISchema
> from qapi.types import gen_types
> from qapi.visit import gen_visit
>
>
> -def main(argv):
> +DEFAULT_OUTPUT_DIR = ''
> +DEFAULT_PREFIX = ''
I did not understand the purpose of these. If they're used only as
the default value for the command line option parsing, I'd suggest
dropping them.
> +
> +
> +def generate(schema_file: str,
> + output_dir: str,
> + prefix: str,
> + unmask: bool = False,
> + builtins: bool = False) -> None:
> +"""
> +generate uses a given schema to produce C code in the target directory.
> +
> +:param schema_file: The primary QAPI schema file.
> +:param output_dir: The output directory to store generated code.
> +:param prefix: Optional C-code prefix for symbol names.
> +:param unmask: Expose non-ABI names through introspection?
> +:param builtins: Generate code for built-in types?
> +
> +:raise QAPIError: On failures.
> +"""
> +match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
> +if match and match.end() != len(prefix):
Nice catch with the extra check here. Maybe worth mentioning and/or
splitting the change?
> +msg = "funny character '{:s}' in prefix '{:s}'".format(
> +prefix[match.end()], prefix)
> +raise QAPIError('', None, msg)
> +
> +schema = QAPISchema(schema_file)
> +gen_types(schema, output_dir, prefix, builtins)
> +gen_visit(schema, output_dir, prefix, builtins)
> +gen_commands(schema, output_dir, prefix)
> +gen_events(schema, output_dir, prefix)
> +gen_introspect(schema, output_dir, prefix, unmask)
> +gen_doc(schema, output_dir, prefix)
> +
> +
> +def main() -> int:
One extra Pythonic touch would be to use a bool here, and then:
sys.exit(0 if main() else 1)
But that's probably overkill.
> +"""
> +gapi-gen shell script entrypoint.
> +Expects arguments via sys.argv, see --help for details.
> +
> +:return: int, 0 on success, 1 on failure.
> +"""
> parser = argparse.ArgumentParser(
> description='Generate code from a QAPI schema')
> parser.add_argument('-b', '--builtins', action='store_true',
> help="generate code for built-in types")
> -parser.add_argument('-o', '--output-dir', action='store', default='',
> +parser.add_argument('-o', '--output-dir', action='store',
> +default=DEFAULT_OUTPUT_DIR,
> help="write output to directory OUTPUT_DIR")
> -parser.add_argument('-p', '--prefix', action='store', default='',
> +parser.add_argument('-p', '--prefix', action='store',
> +default=DEFAULT_PREFIX,
> help="prefix for symbols")
> parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
> dest='unmask',
> @@ -33,26 +81,17 @@ def main(argv):
> parser.add_argument('schema', action='store')
> args = parser.parse_args()
>
> -match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', args.prefix)
> -if match.end() != len(args.prefix):
> -print("%s: 'funny character '%s' in argument of --prefix"
> - % (sys.argv[0], args.prefix[match.end()]),
> - file=sys.stderr)
> -sys.exit(1)
> -
> try:
> -schema = QAPISchema(args.schema)
> +generate(args.schema,
> + output_dir=args.output_dir,
> + prefix=args.prefix,
> + unmask=args.unmask,
> + builtins=args.builtins)
> except QAPIError as err:
> -print(err, file=sys.stderr)
> -exit(1)
> -
Glad to see that this "quitter" is gone in favor of one and only
sys.exit().
- Cleber.
> -gen_types(schema, args.output_dir, args.prefix, args.builtins)
> -
Re: [PATCH v2 01/38] [DO-NOT-MERGE] qapi: add debugging tools
On Tue, Sep 22, 2020 at 05:00:24PM -0400, John Snow wrote:
> This adds some really childishly simple debugging tools. Maybe they're
> interesting for someone else, too?
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/debug.py | 78 +++
> 1 file changed, 78 insertions(+)
> create mode 100644 scripts/qapi/debug.py
>
> diff --git a/scripts/qapi/debug.py b/scripts/qapi/debug.py
> new file mode 100644
> index 00..bacf5ee180
> --- /dev/null
> +++ b/scripts/qapi/debug.py
> @@ -0,0 +1,78 @@
> +"""
> +Small debugging facilities for mypy static analysis work.
> +(C) 2020 John Snow, for Red Hat, Inc.
> +"""
> +
> +import inspect
> +import json
> +from typing import Dict, List, Any
> +from types import FrameType
> +
> +
> +OBSERVED_TYPES: Dict[str, List[str]] = {}
> +
> +
> +# You have no idea how long it took to find this return type...
> +def caller_frame() -> FrameType:
> +"""
> +Returns the stack frame of the caller's caller.
> +e.g. foo() -> caller() -> caller_frame() return's foo's stack frame.
> +"""
> +stack = inspect.stack()
> +caller = stack[2].frame
> +if caller is None:
> +msg = "Python interpreter does not support stack frame inspection"
> +raise RuntimeError(msg)
> +return caller
> +
> +
> +def _add_type_record(name: str, typestr: str) -> None:
> +seen = OBSERVED_TYPES.setdefault(name, [])
> +if typestr not in seen:
> +seen.append(typestr)
> +
> +
> +def record_type(name: str, value: Any, dict_names: bool = False) -> None:
> +"""
> +Record the type of a variable.
> +
> +:param name: The name of the variable
> +:param value: The value of the variable
> +"""
> +_add_type_record(name, str(type(value)))
> +
> +try:
> +for key, subvalue in value.items():
> +subname = f"{name}.{key}" if dict_names else
> f"{name}.[dict_value]"
> +_add_type_record(subname, str(type(subvalue)))
> +return
> +except AttributeError:
> +# (Wasn't a dict or anything resembling one.)
> +pass
> +
> +# str is iterable, but not in the way we want!
> +if isinstance(value, str):
> +return
> +
> +try:
> +for elem in value:
> +_add_type_record(f"{name}.[list_elem]", str(type(elem)))
> +except TypeError:
> +# (Wasn't a list or anything else iterable.)
> +pass
> +
> +
> +def show_types() -> None:
> +"""
> +Print all of the currently known variable types to stdout.
> +"""
> +print(json.dumps(OBSERVED_TYPES, indent=2))
> +
Maybe the following will be cheaper (no json conversion):
pprint.pprint(OBSERVED_TYPES, indent=2)
Other than that, I'd vote for including this if there's a bit more
documentation on how to use it, or an example script. Maybe there
already is, and I did not get to it yet.
- Cleber.
> +
> +def record_locals(show: bool = False, dict_names: bool = False) -> None:
> +caller = caller_frame()
> +name = caller.f_code.co_name
> +for key, value in caller.f_locals.items():
> +record_type(f"{name}.{key}", value, dict_names=dict_names)
> +if show:
> +show_types()
> --
> 2.26.2
>
signature.asc
Description: PGP signature
Re: [PATCH 0/3] Hyper-V Dynamic Memory Protocol driver (hv-balloon)
On 22.09.2020 09:26, David Hildenbrand wrote:
> On 22.09.20 00:22, Maciej S. Szmigiero wrote:
>> Hi David,
>>
>> Thank you for your comments.
>>
(...)
>>
>> The idea is to use virtual DIMM sticks for hot-adding extra memory at
>> runtime, while using ballooning for runtime adjustment of the guest
>> memory size within the current maximum.
>>
>> When the guest is rebooted the virtual DIMMs configuration is adjusted
>> by the software controlling QEMU (some are removed and / or some are
>> added) to give the guest the same effective memory size as it had before
>> the reboot.
>
> Okay, so while "the ACPI DIMM slot limit does not apply", the KVM memory
> slot limit (currently) applies, resulting in exactly the same behavior.
>
> The only (conceptual difference) I am able to spot is then a
> notification to the user on reboot, so the guest memory layout can be
> adjusted (which I consider very ugly, but it's the same thing when
> mixing ballooning and DIMMs - which is why it's usually never done).
If you want to shrink a guest at runtime you'll pretty much have to use
ballooning as {ACPI-based PC, virtual} DIMM stick sizes are far too
large to make anything but rough adjustments to the guest memory size.
In addition to that with ACPI-based PC DIMM hotplug it is the host that
chooses which particular DIMM stick to unplug while having no feedback
from the guest how much of each DIMM stick memory range is currently
in use and so will have to be copied somewhere else.
I know that this a source of significant hot removal slowdown, especially
when a "ripple effect" happens on removal:
1) There are 3 extra DIMMs plugged into the guest: A, B, C.
A and B are nearly empty, but C is nearly full.
2) The host does not know anything which DIMM is empty and which is full,
so it requests the guest to unplug the stick C,
3) The guest copies the content of the stick C to the stick B,
4) Once again, the host does not know anything which DIMM is empty and
which is full, so it requests the guest to unplug the stick B,
5) The guest now has to copy the same data from the stick B to the
stick A, once again.
With virtual DIMM sticks + this driver it is the guest which chooses
which particular pages to release, hopefully choosing the already unused
ones.
Once the whole memory behind a DIMM stick is released the host knows
that it can be unplugged now without any copying.
While it might seem like this will cause a lot of fragmentation in
practice Windows seems to try to give out the largest continuous range
of pages it is able to find.
One can also see in the hv_balloon client driver from the Linux kernel
that this driver tries to do 2 MB allocations for as long as it can
before giving out single pages.
The reason why ballooning and DIMMs wasn't being used together previously
is probably because virtio-balloon will (at least on Windows) mark the
ballooned out pages as in use inside the guest, preventing the removal
of the DIMM stick backing them.
In addition to the above, virtio-balloon is also very slow, as the whole
protocol operates on single pages only, not on page ranges.
There might also be some interference with Windows memory management
causing an extra slowdown in comparison to the native Windows DM
protocol.
If the KVM slot limit starts to be a problem in practice then we can
think what can be done about it.
It's always one obstacle less.
I see that the same KVM slot limit probably applies also for virtio-mem,
since it uses memory-backend-ram as its backing memory device, too,
right?
If not, then how do you map a totally new memory range into the guest
address space without consuming a KVM memory slot?
If that's somehow possible then maybe the same mechanism can simply be
reused for this driver.
> [...]
>
>>
>> So, yes, it will be a problem if the user expands their running guest
>> ~256 times, each time making it even bigger than previously, without
>> rebooting it even once, but this does seem to be an edge use case.
>
> IIRC, that's exactly what dynamic memory under Windows does in automatic
> mode, no? Monitor the guests, distribute memory accordingly - usually in
> smaller steps. But I am no expert on Hyper-V.
Yes, they call their automatic mode "Dynamic Memory" in recent Windows
versions.
This is a bit confusing because even if you disable this feature
the Hyper-V hypervisor will still provide this Dynamic Memory Protocol
service and use it to resize the guest on (user) demand.
Just it won't do such resize on its own but only when explicitly
requested.
Don't know if they internally have any limit that is similar to the KVM
memory slot limit, though.
>>
>> In the future it would be better to automatically turn the current
>> effective guest size into its boot memory size when the VM restarts
>> (the VM will then have no virtual DIMMs inserted after a reboot), but
>> doing this requires quite a few changes to QEMU, that's why it isn't
>> there yet.
>
> Will most probably never happen as
Re: [PATCH] MAINTAINERS: Change my role to reviewer of Python scripts
On 9/22/20 5:31 PM, Eduardo Habkost wrote: I'm stepping out as maintainer of ./scripts/*.py, but still willing to help review patches for Python code. Signed-off-by: Eduardo Habkost --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3d17cad19aa..242a2a6e82e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2356,8 +2356,8 @@ F: include/sysemu/cryptodev*.h F: backends/cryptodev*.c Python scripts -M: Eduardo Habkost M: Cleber Rosa +R: Eduardo Habkost S: Odd fixes F: python/qemu/*py F: scripts/*.py Reviewed-by: John Snow
[PATCH v2 0/1] MAINTAINERS: Add Python library stanza
John Snow (1): MAINTAINERS: Add Python library stanza MAINTAINERS | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) -- 2.26.2
[PATCH 24/25] qapi/schema.py: Add module docstring
Add some microdocumentation that gives a nice file-level overview of this 1300+ line file. Signed-off-by: John Snow --- scripts/qapi/schema.py | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 6ecbc2aa6b..baafe3babf 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -1,7 +1,37 @@ # -*- coding: utf-8 -*- -# -# QAPI schema internal representation -# +""" +QAPI Schema internal representation. + +The `QAPISchema` represents the fully realized QAPI schema. It builds a +listing of `QAPISchemaEntity` objects that are each associated with a +`QAPISchemaModule`. + +`QAPISchemaMember` represents a single member of a collection of +members, see the subclasses thereof for users. `QAPISchemaVariants` is a +simple collection of `QAPISchemaVariant`. + +The `QAPISchemaVisitor` can be extended and passed to QAPISchema.visit +to iterate over a schema and perform code generation tasks. + +The Python class hierarchy at a glance: + +`QAPISchemaEntity` + `QAPISchemaInclude` + `QAPISchemaCommand` + `QAPISchemaEvent` + `QAPISchemaType` +`QAPISchemaBuiltinType` +`QAPISchemaEnumType` +`QAPISchemaArrayType` +`QAPISchemaObjectType` +`QAPISchemaAlternateType` + +`QAPISchemaMember` + `QAPISchemaEnumMember` + `QAPISchemaFeature` + `QAPISchemaObjectTypeMember` +`QAPISchemaVariant` +""" # Copyright (c) 2015-2019 Red Hat Inc. # # Authors: -- 2.26.2
Re: [PATCH 00/26] qapi: static typing conversion, pt5
On 9/22/20 6:34 PM, John Snow wrote: based-on: <20200922210101.4081073-1-js...@redhat.com> ^ Copy-paste malfunction based-on: <20200922212115.4084301-1-js...@redhat.com> [PATCH 0/6] qapi: static typing conversion, pt4 Hi, this series adds static type hints to the QAPI module. This is part five! Part 5: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt5 Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6 - Requires Python 3.6+ - Requires mypy 0.770 or newer (for type analysis only) - Requires pylint 2.6.0 or newer (for lint checking only) This part of the series focuses on just parser.py. Type hints are added in patches that add *only* type hints and change no other behavior. Any necessary changes to behavior to accommodate typing are split out into their own tiny patches. Every commit should pass with: - flake8 qapi/ - pylint --rcfile=qapi/pylintrc qapi/ - mypy --config-file=qapi/mypy.ini qapi/ John Snow (26): qapi/parser.py: refactor parsing routine into method qapi/parser.py: group variable declarations in __init__ qapi/parser.py: use 'with' statement for opening files qapi/source.py: Add default arguments to QAPISourceInfo qapi/parser.py: start source info at line 0 qapi/parser.py: raise QAPIParseError during file opening qapi/parser.py: fully remove 'null' constant qapi/parser.py: Assert lexer value is a string qapi/parser.py: assert get_expr returns object in outer loop qapi/parser.py: assert object keys are strings qapi/parser.py: Convert several methods to @classmethod qapi/parser.py: add casts to pragma checks qapi/parser.py: add type hint annotations qapi/parser.py: add docstrings qapi/parser.py: add ParsedExpression type qapi/pragma.py: Move QAPISchemaPragma into its own module qapi/pragma.py: Move pragma parsing out of parser.py qapi/parser.py: Modify _include() to use parser state qapi/parser.py: add parent argument qapi/parser.py: remove unused check_args_section arguments qapi/parser.py: QAPIDoc: convert @staticmethod to @classmethod qapi/parser.py: add type hint annotations (QAPIDoc) qapi/parser.py: enable mypy checks qapi/parser.py: remove one and two-letter variables qapi/parser.py: Silence too-few-public-methods warning qapi/parser.py: enable pylint checks scripts/qapi/error.py | 8 +- scripts/qapi/expr.py | 56 ++-- scripts/qapi/mypy.ini | 5 - scripts/qapi/parser.py| 322 +- scripts/qapi/pragma.py| 68 + scripts/qapi/pylintrc | 3 +- scripts/qapi/schema.py| 6 +- scripts/qapi/source.py| 22 +- tests/qapi-schema/leading-comma-list.err | 2 +- tests/qapi-schema/trailing-comma-list.err | 2 +- 10 files changed, 301 insertions(+), 193 deletions(-) create mode 100644 scripts/qapi/pragma.py
[PATCH 22/25] qapi/schema.py: Ignore unused argument for check()
This is an interface with a default implementation. Pylint doesn't have
enough context to be aware of this.
Signed-off-by: John Snow
---
scripts/qapi/schema.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 271befea1c..6ecbc2aa6b 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -86,6 +86,7 @@ def c_name(self) -> str:
return c_name(self.name)
def check(self, schema: 'QAPISchema') -> None:
+# pylint: disable=unused-argument
assert not self._checked
seen: Dict[str, 'QAPISchemaMember'] = {}
for feature in self.features:
--
2.26.2
[PATCH 25/25] qapi/schema.py: Use python3 style super()
Signed-off-by: John Snow --- scripts/qapi/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index baafe3babf..6b47ca26e0 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -309,7 +309,7 @@ def doc_type(self) -> Optional[str]: return self.name def check(self, schema: 'QAPISchema') -> None: -QAPISchemaEntity.check(self, schema) +super().check(schema) if 'deprecated' in [f.name for f in self.features]: raise QAPISemError( self.info, "feature 'deprecated' is not supported for types") -- 2.26.2
[PATCH 08/25] qapi/schema.py: Allow alternate_type to assert
It is generally nicer to just let things fail, because it makes the
static type hints less infected with Optional[T], where a future
programmer using the library has to wonder what that means.
Let errors be errors.
Signed-off-by: John Snow
---
scripts/qapi/schema.py | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 15ff441660..a84d8549a4 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -199,7 +199,7 @@ def alternate_qtype(self):
'boolean': 'QTYPE_QBOOL',
'object': 'QTYPE_QDICT'
}
-return json2qtype.get(self.json_type())
+return json2qtype[self.json_type()]
def doc_type(self):
if self.is_implicit():
@@ -480,8 +480,9 @@ def check(self, schema):
types_seen = {}
for v in self.variants.variants:
v.check_clash(self.info, seen)
-qtype = v.type.alternate_qtype()
-if not qtype:
+try:
+qtype = v.type.alternate_qtype()
+except KeyError:
raise QAPISemError(
self.info,
"%s cannot use %s"
--
2.26.2
[PATCH v2 1/1] MAINTAINERS: Add Python library stanza
I'm proposing that I split the actual Python library off from the other miscellaneous python scripts we have and declare it maintained. Add myself as a maintainer of this folder, along with Cleber. v2: change python/* to python/, thanks Alex. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée Acked-by: Cleber Rosa Acked-by: Eduardo Habkost --- MAINTAINERS | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3d17cad19a..c0222ee645 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2355,11 +2355,18 @@ S: Maintained F: include/sysemu/cryptodev*.h F: backends/cryptodev*.c +Python library +M: John Snow +M: Cleber Rosa +R: Eduardo Habkost +S: Maintained +F: python/ +T: git https://gitlab.com/jsnow/qemu.git python + Python scripts M: Eduardo Habkost M: Cleber Rosa S: Odd fixes -F: python/qemu/*py F: scripts/*.py F: tests/*.py -- 2.26.2
[PATCH 07/25] qapi/schema.py: constrain tag_member type
Signed-off-by: John Snow --- scripts/qapi/schema.py | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index aaf20f776b..15ff441660 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -546,15 +546,18 @@ def set_defined_in(self, name): def check(self, schema, seen): if not self.tag_member: # flat union -self.tag_member = seen.get(c_name(self._tag_name)) +tag_member = seen.get(c_name(self._tag_name)) base = "'base'" # Pointing to the base type when not implicit would be # nice, but we don't know it here -if not self.tag_member or self._tag_name != self.tag_member.name: +if not tag_member or self._tag_name != tag_member.name: raise QAPISemError( self.info, "discriminator '%s' is not a member of %s" % (self._tag_name, base)) + +assert isinstance(tag_member, QAPISchemaObjectTypeMember) +self.tag_member = tag_member # Here we do: base_type = schema.lookup_type(self.tag_member.defined_in) assert base_type -- 2.26.2
Re: tools/virtiofs: Multi threading seems to hurt performance
On Tue, Sep 22, 2020 at 12:09:46PM +0100, Dr. David Alan Gilbert wrote: > > Do you have the numbers for: >epool >epool thread-pool-size=1 >spool Hi David, Ok, I re-ran my numbers again after upgrading to latest qemu and also upgraded host kernel to latest upstream. Apart from comparing I epool, spool and 1Thread, I also ran their numa variants. That is I launched qemu and virtiofsd on node 0 of machine (numactl --cpunodebind=0). Results are kind of mixed. Here are my takeaways. - Running on same numa node improves performance overall for exclusive, shared and exclusive-1T mode. - In general both shared pool and exclusive-1T mode seem to perform better than exclusive mode, except for the case of randwrite-libaio. In some cases (seqread-libaio, seqwrite-libaio, seqwrite-libaio-multi) exclusive pool performs better than exclusive-1T. - Looks like in some cases exclusive-1T performs better than shared pool. (randwrite-libaio, randwrite-psync-multi, seqwrite-psync-multi, seqwrite-psync, seqread-libaio-multi, seqread-psync-multi) Overall, I feel that both exlusive-1T and shared perform better than exclusive pool. Results between exclusive-1T and shared pool are mixed. It seems like in many cases exclusve-1T performs better. I would say that moving to "shared" pool seems like a reasonable option. Thanks Vivek NAMEWORKLOADBandwidth IOPS vtfs-none-epool seqread-psync 38(MiB/s) 9967 vtfs-none-epool-1T seqread-psync 66(MiB/s) 16k vtfs-none-spool seqread-psync 67(MiB/s) 16k vtfs-none-epool-numaseqread-psync 48(MiB/s) 12k vtfs-none-epool-1T-numa seqread-psync 74(MiB/s) 18k vtfs-none-spool-numaseqread-psync 74(MiB/s) 18k vtfs-none-epool seqread-psync-multi 204(MiB/s) 51k vtfs-none-epool-1T seqread-psync-multi 325(MiB/s) 81k vtfs-none-spool seqread-psync-multi 271(MiB/s) 67k vtfs-none-epool-numaseqread-psync-multi 253(MiB/s) 63k vtfs-none-epool-1T-numa seqread-psync-multi 349(MiB/s) 87k vtfs-none-spool-numaseqread-psync-multi 301(MiB/s) 75k vtfs-none-epool seqread-libaio 301(MiB/s) 75k vtfs-none-epool-1T seqread-libaio 273(MiB/s) 68k vtfs-none-spool seqread-libaio 334(MiB/s) 83k vtfs-none-epool-numaseqread-libaio 315(MiB/s) 78k vtfs-none-epool-1T-numa seqread-libaio 326(MiB/s) 81k vtfs-none-spool-numaseqread-libaio 335(MiB/s) 83k vtfs-none-epool seqread-libaio-multi202(MiB/s) 50k vtfs-none-epool-1T seqread-libaio-multi308(MiB/s) 77k vtfs-none-spool seqread-libaio-multi247(MiB/s) 61k vtfs-none-epool-numaseqread-libaio-multi238(MiB/s) 59k vtfs-none-epool-1T-numa seqread-libaio-multi307(MiB/s) 76k vtfs-none-spool-numaseqread-libaio-multi269(MiB/s) 67k vtfs-none-epool randread-psync 41(MiB/s) 10k vtfs-none-epool-1T randread-psync 67(MiB/s) 16k vtfs-none-spool randread-psync 64(MiB/s) 16k vtfs-none-epool-numarandread-psync 48(MiB/s) 12k vtfs-none-epool-1T-numa randread-psync 73(MiB/s) 18k vtfs-none-spool-numarandread-psync 72(MiB/s) 18k vtfs-none-epool randread-psync-multi207(MiB/s) 51k vtfs-none-epool-1T randread-psync-multi313(MiB/s) 78k vtfs-none-spool randread-psync-multi265(MiB/s) 66k vtfs-none-epool-numarandread-psync-multi253(MiB/s) 63k vtfs-none-epool-1T-numa randread-psync-multi340(MiB/s) 85k vtfs-none-spool-numarandread-psync-multi305(MiB/s) 76k vtfs-none-epool randread-libaio 305(MiB/s) 76k vtfs-none-epool-1T randread-libaio 308(MiB/s) 77k vtfs-none-spool randread-libaio 329(MiB/s) 82k vtfs-none-epool-numarandread-libaio 310(MiB/s) 77k vtfs-none-epool-1T-numa randread-libaio 328(MiB/s) 82k vtfs-none-spool-numarandread-libaio 339(MiB/s) 84k vtfs-none-epool randread-libaio-multi 265(MiB/s) 66k vtfs-none-epool-1T randread-libaio-multi 267(MiB/s) 66k
[PATCH 16/25] qapi/schema.py: enable checking
Signed-off-by: John Snow --- scripts/qapi/mypy.ini | 6 -- 1 file changed, 6 deletions(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 20ab509946..df60c18de1 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -1,10 +1,4 @@ [mypy] strict = True strict_optional = False -disallow_untyped_calls = False python_version = 3.6 - -[mypy-qapi.schema] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False -- 2.26.2
[PATCH 02/25] qapi/schema.py: Move meta-type into class instances
We are using meta as a class variable, but union types use this as a mutable value which changes the value for the class, not the instance. Use the empty string as the default/empty value for ease of typing. It is still false-ish, so it will work just fine. Signed-off-by: John Snow --- scripts/qapi/schema.py | 32 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 55434f5c82..0201b42095 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -17,7 +17,6 @@ import os import re from collections import OrderedDict -from typing import Optional from .common import c_name, POINTER_SUFFIX from .error import QAPISourceError, QAPISemError @@ -32,8 +31,6 @@ def visit(self, visitor: 'QAPISchemaVisitor') -> None: class QAPISchemaEntity(Visitable): -meta: Optional[str] = None - def __init__(self, name: str, info, doc, ifcond=None, features=None): assert name is None or isinstance(name, str) for f in features or []: @@ -51,6 +48,15 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None): self._ifcond = ifcond or [] self.features = features or [] self._checked = False +self._meta = '' + +@property +def meta(self) -> str: +return self._meta + +@meta.setter +def meta(self, value: str) -> None: +self._meta = value def c_name(self): return c_name(self.name) @@ -212,8 +218,6 @@ def describe(self): class QAPISchemaBuiltinType(QAPISchemaType): -meta = 'built-in' - def __init__(self, name, json_type, c_type): super().__init__(name, None, None) assert not c_type or isinstance(c_type, str) @@ -221,6 +225,7 @@ def __init__(self, name, json_type, c_type): 'value') self._json_type_name = json_type self._c_type_name = c_type +self._meta = 'built-in' def c_name(self): return self.name @@ -245,8 +250,6 @@ def visit(self, visitor): class QAPISchemaEnumType(QAPISchemaType): -meta = 'enum' - def __init__(self, name, info, doc, ifcond, features, members, prefix): super().__init__(name, info, doc, ifcond, features) for m in members: @@ -255,6 +258,7 @@ def __init__(self, name, info, doc, ifcond, features, members, prefix): assert prefix is None or isinstance(prefix, str) self.members = members self.prefix = prefix +self._meta = 'enum' def check(self, schema): super().check(schema) @@ -289,13 +293,12 @@ def visit(self, visitor): class QAPISchemaArrayType(QAPISchemaType): -meta = 'array' - def __init__(self, name, info, element_type): super().__init__(name, info, None) assert isinstance(element_type, str) self._element_type_name = element_type self.element_type = None +self._meta = 'array' def check(self, schema): super().check(schema) @@ -344,7 +347,7 @@ def __init__(self, name, info, doc, ifcond, features, # flat union has base, variants, and no local_members # simple union has local_members, variants, and no base super().__init__(name, info, doc, ifcond, features) -self.meta = 'union' if variants else 'struct' +self._meta = 'union' if variants else 'struct' assert base is None or isinstance(base, str) for m in local_members: assert isinstance(m, QAPISchemaObjectTypeMember) @@ -456,8 +459,6 @@ def visit(self, visitor): class QAPISchemaAlternateType(QAPISchemaType): -meta = 'alternate' - def __init__(self, name, info, doc, ifcond, features, variants): super().__init__(name, info, doc, ifcond, features) assert isinstance(variants, QAPISchemaVariants) @@ -465,6 +466,7 @@ def __init__(self, name, info, doc, ifcond, features, variants): variants.set_defined_in(name) variants.tag_member.set_defined_in(self.name) self.variants = variants +self._meta = 'alternate' def check(self, schema): super().check(schema) @@ -716,8 +718,6 @@ def __init__(self, name, info, typ, ifcond=None): class QAPISchemaCommand(QAPISchemaEntity): -meta = 'command' - def __init__(self, name, info, doc, ifcond, features, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig): @@ -733,6 +733,7 @@ def __init__(self, name, info, doc, ifcond, features, self.boxed = boxed self.allow_oob = allow_oob self.allow_preconfig = allow_preconfig +self._meta = 'command' def check(self, schema): super().check(schema) @@ -779,14 +780,13 @@ def visit(self, visitor): class QAPISchemaEvent(QAPISchemaEntity): -meta = 'event' - def __init__(self, name, info, doc, ifcond, features, arg_type, boxed):
[PATCH 18/25] qapi/schema.py: Add pylint warning suppressions
Signed-off-by: John Snow --- scripts/qapi/schema.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 8907bec0b5..61238c0686 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -39,6 +39,8 @@ class Visitable: """Abstract duck that suggests a class is visitable.""" +# pylint: disable=too-few-public-methods + def visit(self, visitor: 'QAPISchemaVisitor') -> None: raise NotImplementedError @@ -133,6 +135,7 @@ def visit_module(self, name: Optional[str]) -> None: pass def visit_needed(self, entity: QAPISchemaEntity) -> bool: +# pylint: disable=unused-argument, no-self-use # Default to visiting everything return True -- 2.26.2
[PATCH 17/25] qapi: Disable similarity checks in pylint entirely
The pylint similarity checks cannot distinguish parameter lists from other code; with the QAPISchemaVisitor interface having long lists of parameters, these similarity checks fire off in a way that's difficult to disable in a targeted way without littering the code with pylint pragmas. There is a change request filed to be able to ignore parameter lists, see: https://github.com/PyCQA/pylint/issues/3619 Signed-off-by: John Snow --- scripts/qapi/pylintrc | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index 5091a08f12..fb444e93bb 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -18,6 +18,7 @@ ignore-patterns=schema.py, # --disable=W". disable=fixme, missing-docstring, +similarities, # See https://github.com/PyCQA/pylint/issues/3619 too-many-arguments, too-many-branches, too-many-statements, -- 2.26.2
[PATCH 15/25] qapi/schema.py: add type hint annotations
Annotations do not change runtime behavior.
This commit *only* adds annotations.
Signed-off-by: John Snow
---
scripts/qapi/schema.py | 519 -
1 file changed, 361 insertions(+), 158 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b502eee9cb..8907bec0b5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -14,15 +14,27 @@
# TODO catching name collisions in generated code would be nice
-import os
-import re
from collections import OrderedDict
-from typing import cast, List
+import re
+import os
+from typing import (
+Any,
+Callable,
+Dict,
+List,
+Optional,
+Type,
+TypeVar,
+Union,
+cast,
+overload,
+)
from .common import c_name, POINTER_SUFFIX
from .error import QAPISourceError, QAPISemError
from .expr import check_exprs
-from .parser import QAPISchemaParser
+from .parser import QAPIDoc, QAPISchemaParser, ParsedExpression
+from .source import QAPISourceInfo
class Visitable:
@@ -32,13 +44,18 @@ def visit(self, visitor: 'QAPISchemaVisitor') -> None:
class QAPISchemaEntity(Visitable):
-def __init__(self, name: str, info, doc, ifcond=None, features=None):
+def __init__(self,
+ name: str,
+ info: Optional[QAPISourceInfo],
+ doc: Optional[QAPIDoc],
+ ifcond: Optional[Union[List[str], 'QAPISchemaType']] = None,
+ features: Optional[List['QAPISchemaFeature']] = None):
assert name is None or isinstance(name, str)
for f in features or []:
assert isinstance(f, QAPISchemaFeature)
f.set_defined_in(name)
self.name = name
-self._module = None
+self._module: Optional[QAPISchemaModule] = None
# For explicitly defined entities, info points to the (explicit)
# definition. For builtins (and their arrays), info is None.
# For implicitly defined entities, info points to a place that
@@ -59,105 +76,151 @@ def meta(self) -> str:
def meta(self, value: str) -> None:
self._meta = value
-def c_name(self):
+def c_name(self) -> str:
return c_name(self.name)
-def check(self, schema):
+def check(self, schema: 'QAPISchema') -> None:
assert not self._checked
-seen = {}
+seen: Dict[str, 'QAPISchemaMember'] = {}
for f in self.features:
f.check_clash(self.info, seen)
self._checked = True
-def connect_doc(self, doc=None):
+def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
doc = doc or self.doc
if doc:
for f in self.features:
doc.connect_feature(f)
-def check_doc(self):
+def check_doc(self) -> None:
if self.doc:
self.doc.check()
-def _set_module(self, schema, info):
+def _set_module(self,
+schema: 'QAPISchema',
+info: Optional[QAPISourceInfo]) -> None:
assert self._checked
self._module = schema.module_by_fname(info.fname if info else None)
self._module.add_entity(self)
-def set_module(self, schema):
+def set_module(self, schema: 'QAPISchema') -> None:
self._set_module(schema, self.info)
@property
-def ifcond(self):
+def ifcond(self) -> List[str]:
assert self._checked and isinstance(self._ifcond, list)
return self._ifcond
-def is_implicit(self):
+def is_implicit(self) -> bool:
return not self.info
-def visit(self, visitor):
+def visit(self, visitor: 'QAPISchemaVisitor') -> None:
assert self._checked
-def describe(self):
+def describe(self) -> str:
assert self.meta
return "%s '%s'" % (self.meta, self.name)
class QAPISchemaVisitor:
-def visit_begin(self, schema):
+def visit_begin(self, schema: 'QAPISchema') -> None:
pass
-def visit_end(self):
+def visit_end(self) -> None:
pass
-def visit_module(self, name):
+def visit_module(self, name: Optional[str]) -> None:
pass
-def visit_needed(self, entity):
+def visit_needed(self, entity: QAPISchemaEntity) -> bool:
# Default to visiting everything
return True
-def visit_include(self, name, info):
+def visit_include(self, name: str, info: QAPISourceInfo) -> None:
pass
-def visit_builtin_type(self, name, info, json_type):
+def visit_builtin_type(self, name: str,
+ info: Optional[QAPISourceInfo],
+ json_type: str) -> None:
pass
-def visit_enum_type(self, name, info, ifcond, features, members, prefix):
+def visit_enum_type(self,
+name: str,
+info: Optional[QAPISourceInfo],
+ifcond: List[str],
+features: List['QAPISc
[PATCH 00/25] qapi: static typing conversion, pt6
based-on: <2020093525.4085762-1-js...@redhat.com> [PATCH 00/26] qapi: static typing conversion, pt5 Hi, this series adds static type hints to the QAPI module. This is the final part, part six! Part 6 (Everything): https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6 - Requires Python 3.6+ - Requires mypy 0.770 or newer (for type analysis only) - Requires pylint 2.6.0 or newer (for lint checking only) This part of the series focuses on schema.py. Type hints are added in patches that add *only* type hints and change no other behavior. Any necessary changes to behavior to accommodate typing are split out into their own tiny patches. Every commit should pass with: - flake8 qapi/ - pylint --rcfile=qapi/pylintrc qapi/ - mypy --config-file=qapi/mypy.ini qapi/ John Snow (25): qapi/schema: add Visitable mixin qapi/schema.py: Move meta-type into class instances qapi/schema.py: add assert in stub methods qapi/schema.py: constrain QAPISchemaObjectType base type qapi/schema.py: constrain QAPISchemaObjectTypeMember arg_type type qapi/schema.py: constrain QAPISchemaEvent arg_type type qapi/schema.py: constrain tag_member type qapi/schema.py: Allow alternate_type to assert qapi/schema.py: remove superfluous assert qapi/schema.py: Add assertion to ifcond property qapi/schema.py: Constrain type of QAPISchemaObjectType members field qapi/schema.py: remove 'and' from non-bool rvalue expressions qapi/schema.py: Test type of self.ret_type instead of local temp qapi/schema.py: Assert variants of an object are also objects qapi/schema.py: add type hint annotations qapi/schema.py: enable checking qapi: Disable similarity checks in pylint entirely qapi/schema.py: Add pylint warning suppressions qapi/schema.py: Convert several methods to classmethods qapi/schema.py: Replace one-letter variable names qapi/schema.py: disable pylint line limit qapi/schema.py: Ignore unused argument for check() qapi/schema.py: enable pylint checks qapi/schema.py: Add module docstring qapi/schema.py: Use python3 style super() scripts/qapi/mypy.ini | 6 - scripts/qapi/pylintrc | 6 +- scripts/qapi/schema.py | 848 +++-- 3 files changed, 557 insertions(+), 303 deletions(-) -- 2.26.2
[PATCH 19/25] qapi/schema.py: Convert several methods to classmethods
If they don't use self and nothing that extends them needs self either,
they can be classmethods.
Signed-off-by: John Snow
---
scripts/qapi/schema.py | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 61238c0686..2d23ce04eb 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -1096,7 +1096,8 @@ def _def_predefineds(self) -> None:
self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
qtype_values, 'QTYPE'))
-def _make_features(self,
+@classmethod
+def _make_features(cls,
features: Optional[List[Dict[str, Any]]],
info: QAPISourceInfo) -> List[QAPISchemaFeature]:
if features is None:
@@ -1104,7 +1105,8 @@ def _make_features(self,
return [QAPISchemaFeature(f['name'], info, f.get('if'))
for f in features]
-def _make_enum_members(self,
+@classmethod
+def _make_enum_members(cls,
values: List[Dict[str, Any]],
info: Optional[QAPISourceInfo],
) -> List[QAPISchemaEnumMember]:
@@ -1213,7 +1215,8 @@ def _def_struct_type(self,
self._make_members(data, info),
None))
-def _make_variant(self,
+@classmethod
+def _make_variant(cls,
case: str,
typ: str,
ifcond: Optional[List[str]],
--
2.26.2
[PATCH 23/25] qapi/schema.py: enable pylint checks
Signed-off-by: John Snow --- scripts/qapi/pylintrc | 5 - 1 file changed, 5 deletions(-) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index fb444e93bb..539e5f65a0 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -1,10 +1,5 @@ [MASTER] -# Add files or directories matching the regex patterns to the ignore list. -# The regex matches against base names, not paths. -ignore-patterns=schema.py, - - [MESSAGES CONTROL] # Disable the message, report, category or checker with the given id(s). You -- 2.26.2
[PATCH 20/25] qapi/schema.py: Replace one-letter variable names
I hope you like butter, because here comes the churn!
Signed-off-by: John Snow
---
scripts/qapi/schema.py | 184 +
1 file changed, 95 insertions(+), 89 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 2d23ce04eb..a0e047c735 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -53,9 +53,11 @@ def __init__(self,
ifcond: Optional[Union[List[str], 'QAPISchemaType']] = None,
features: Optional[List['QAPISchemaFeature']] = None):
assert name is None or isinstance(name, str)
-for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
-f.set_defined_in(name)
+
+for feature in features or []:
+assert isinstance(feature, QAPISchemaFeature)
+feature.set_defined_in(name)
+
self.name = name
self._module: Optional[QAPISchemaModule] = None
# For explicitly defined entities, info points to the (explicit)
@@ -84,15 +86,15 @@ def c_name(self) -> str:
def check(self, schema: 'QAPISchema') -> None:
assert not self._checked
seen: Dict[str, 'QAPISchemaMember'] = {}
-for f in self.features:
-f.check_clash(self.info, seen)
+for feature in self.features:
+feature.check_clash(self.info, seen)
self._checked = True
def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
doc = doc or self.doc
if doc:
-for f in self.features:
-doc.connect_feature(f)
+for feature in self.features:
+doc.connect_feature(feature)
def check_doc(self) -> None:
if self.doc:
@@ -326,9 +328,9 @@ def __init__(self,
members: List['QAPISchemaEnumMember'],
prefix: Optional[str]):
super().__init__(name, info, doc, ifcond, features)
-for m in members:
-assert isinstance(m, QAPISchemaEnumMember)
-m.set_defined_in(name)
+for member in members:
+assert isinstance(member, QAPISchemaEnumMember)
+member.set_defined_in(name)
assert prefix is None or isinstance(prefix, str)
self.members = members
self.prefix = prefix
@@ -337,14 +339,14 @@ def __init__(self,
def check(self, schema: 'QAPISchema') -> None:
super().check(schema)
seen: Dict[str, 'QAPISchemaMember'] = {}
-for m in self.members:
-m.check_clash(self.info, seen)
+for member in self.members:
+member.check_clash(self.info, seen)
def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
super().connect_doc(doc)
doc = doc or self.doc
-for m in self.members:
-m.connect_doc(doc)
+for member in self.members:
+member.connect_doc(doc)
def is_implicit(self) -> bool:
# See QAPISchema._make_implicit_enum_type() and ._def_predefineds()
@@ -432,9 +434,9 @@ def __init__(self,
super().__init__(name, info, doc, ifcond, features)
self._meta = 'union' if variants else 'struct'
assert base is None or isinstance(base, str)
-for m in local_members:
-assert isinstance(m, QAPISchemaObjectTypeMember)
-m.set_defined_in(name)
+for member in local_members:
+assert isinstance(member, QAPISchemaObjectTypeMember)
+member.set_defined_in(name)
if variants is not None:
assert isinstance(variants, QAPISchemaVariants)
variants.set_defined_in(name)
@@ -471,9 +473,9 @@ def check(self, schema: 'QAPISchema') -> None:
self.base = base
self.base.check(schema)
self.base.check_clash(self.info, seen)
-for m in self.local_members:
-m.check(schema)
-m.check_clash(self.info, seen)
+for member in self.local_members:
+member.check(schema)
+member.check_clash(self.info, seen)
# check_clash is abstract, but local_members is asserted to be
# Sequence[QAPISchemaObjectTypeMember]. Cast to the narrower type.
@@ -493,16 +495,16 @@ def check_clash(self,
seen: Dict[str, 'QAPISchemaMember']) -> None:
assert self._checked
assert not self.variants # not implemented
-for m in self.members:
-m.check_clash(info, seen)
+for member in self.members:
+member.check_clash(info, seen)
def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
super().connect_doc(doc)
doc = doc or self.doc
if self.base and self.base.is_implicit():
self.base.connect_doc(doc)
-for m in self.local_members:
-m.connect_doc(doc)
+for member in self.local_members:
+member.connect_doc(doc)
@property
def ifc
[PATCH 01/25] qapi/schema: add Visitable mixin
Python doesn't have anything quite exactly like Traits, Interfaces, or Mixins; but we can approximate it. Add a 'Visitable' class that enforces a type signature for the visit() method; this way, mypy is ensuring that even for otherwise unrelated classes that do not inherit from a common base, this signature will always be forced to be compatible. Signed-off-by: John Snow --- scripts/qapi/schema.py | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 51af0449f5..55434f5c82 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -25,7 +25,13 @@ from .parser import QAPISchemaParser -class QAPISchemaEntity: +class Visitable: +"""Abstract duck that suggests a class is visitable.""" +def visit(self, visitor: 'QAPISchemaVisitor') -> None: +raise NotImplementedError + + +class QAPISchemaEntity(Visitable): meta: Optional[str] = None def __init__(self, name: str, info, doc, ifcond=None, features=None): @@ -136,7 +142,7 @@ def visit_event(self, name, info, ifcond, features, arg_type, boxed): pass -class QAPISchemaModule: +class QAPISchemaModule(Visitable): def __init__(self, name): self.name = name self._entity_list = [] @@ -812,7 +818,7 @@ def visit(self, visitor): self.arg_type, self.boxed) -class QAPISchema: +class QAPISchema(Visitable): def __init__(self, fname): self.fname = fname parser = QAPISchemaParser(fname) -- 2.26.2
[PATCH 11/25] qapi/schema.py: Constrain type of QAPISchemaObjectType members field
This must always be QAPISchemaObjectTypeMember, and we do check and enforce this -- but because the seen dict is invariant and check methods need to operate in terms of a more abstract type, we need to tell the type system to believe us. Signed-off-by: John Snow --- scripts/qapi/schema.py | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 62b1a7e890..57343a1002 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -17,6 +17,7 @@ import os import re from collections import OrderedDict +from typing import cast, List from .common import c_name, POINTER_SUFFIX from .error import QAPISourceError, QAPISemError @@ -391,7 +392,10 @@ def check(self, schema): for m in self.local_members: m.check(schema) m.check_clash(self.info, seen) -members = seen.values() + +# check_clash is abstract, but local_members is asserted to be +# Sequence[QAPISchemaObjectTypeMember]. Cast to the narrower type. +members = cast(List[QAPISchemaObjectTypeMember], list(seen.values())) if self.variants: self.variants.check(schema, seen) -- 2.26.2
[PATCH 21/25] qapi/schema.py: disable pylint line limit
It's a big file, but there's really nothing in here that doesn't belong in here -- and I don't think it's large enough to justify the one-module-per-class approach. Signed-off-by: John Snow --- scripts/qapi/schema.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index a0e047c735..271befea1c 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -12,6 +12,8 @@ # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. +# pylint: disable=too-many-lines ¯\_(ツ)_/¯ + # TODO catching name collisions in generated code would be nice from collections import OrderedDict -- 2.26.2
[PATCH 22/26] qapi/parser.py: add type hint annotations (QAPIDoc)
Annotations do not change runtime behavior.
This commit *only* adds annotations.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 68 +++---
1 file changed, 38 insertions(+), 30 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f5f40ffa16..cdb64ffc22 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -23,6 +23,7 @@
NamedTuple,
Optional,
Set,
+TYPE_CHECKING,
Type,
TypeVar,
Union,
@@ -32,6 +33,10 @@
from .pragma import PragmaError
from .source import QAPISourceInfo
+if TYPE_CHECKING:
+# pylint: disable=cyclic-import
+from .schema import QAPISchemaMember, QAPISchemaFeature
+
_Value = Union[List[object], 'OrderedDict[str, object]', str, bool]
# Necessary imprecision: mypy does not (yet?) support recursive types;
@@ -405,43 +410,43 @@ class QAPIDoc:
"""
class Section:
-def __init__(self, name=None):
+def __init__(self, name: Optional[str] = None):
# optional section name (argument/member or section name)
self.name = name
self.text = ''
-def append(self, line):
+def append(self, line: str) -> None:
self.text += line.rstrip() + '\n'
class ArgSection(Section):
-def __init__(self, name):
+def __init__(self, name: Optional[str] = None):
super().__init__(name)
-self.member = None
+self.member: Optional['QAPISchemaMember'] = None
-def connect(self, member):
+def connect(self, member: 'QAPISchemaMember') -> None:
self.member = member
-def __init__(self, info):
+def __init__(self, info: QAPISourceInfo):
self.info = info
-self.symbol = None
+self.symbol: Optional[str] = None
self.body = QAPIDoc.Section()
# dict mapping parameter name to ArgSection
-self.args = OrderedDict()
-self.features = OrderedDict()
+self.args: 'OrderedDict[str, QAPIDoc.ArgSection]' = OrderedDict()
+self.features: 'OrderedDict[str, QAPIDoc.ArgSection]' = OrderedDict()
# a list of Section
-self.sections = []
+self.sections: List[QAPIDoc.Section] = []
# the current section
self._section = self.body
self._append_line = self._append_body_line
-def has_section(self, name):
+def has_section(self, name: str) -> bool:
"""Return True if we have a section with this name."""
for i in self.sections:
if i.name == name:
return True
return False
-def append(self, line):
+def append(self, line: str) -> None:
"""
Parse a comment line and add it to the documentation.
@@ -462,18 +467,18 @@ def append(self, line):
line = line[1:]
self._append_line(line)
-def end_comment(self):
+def end_comment(self) -> None:
self._end_section()
@classmethod
-def _is_section_tag(cls, name):
+def _is_section_tag(cls, name: str) -> bool:
return name in ('Returns:', 'Since:',
# those are often singular or plural
'Note:', 'Notes:',
'Example:', 'Examples:',
'TODO:')
-def _append_body_line(self, line):
+def _append_body_line(self, line: str) -> None:
"""
Process a line of documentation text in the body section.
@@ -513,7 +518,7 @@ def _append_body_line(self, line):
# This is a free-form documentation block
self._append_freeform(line.strip())
-def _append_args_line(self, line):
+def _append_args_line(self, line: str) -> None:
"""
Process a line of documentation text in an argument section.
@@ -546,7 +551,7 @@ def _append_args_line(self, line):
self._append_freeform(line.strip())
-def _append_features_line(self, line):
+def _append_features_line(self, line: str) -> None:
name = line.split(' ', 1)[0]
if name.startswith('@') and name.endswith(':'):
@@ -565,7 +570,7 @@ def _append_features_line(self, line):
self._append_freeform(line.strip())
-def _append_various_line(self, line):
+def _append_various_line(self, line: str) -> None:
"""
Process a line of documentation text in an additional section.
@@ -591,7 +596,10 @@ def _append_various_line(self, line):
self._append_freeform(line)
-def _start_symbol_section(self, symbols_dict, name):
+def _start_symbol_section(
+self,
+symbols_dict: 'OrderedDict[str, QAPIDoc.ArgSection]',
+name: str) -> None:
# FIXME invalid names other than the empty string aren't flagged
if not name:
raise QAPIDocError("invalid parameter name")
@@ -602,20 +610,20 @@ def _start_symbol_section(self, symbols_dict, na
[PATCH 13/25] qapi/schema.py: Test type of self.ret_type instead of local temp
This is obscure: If we test the type on a local copy instead of the stored state AFTER the assignment, mypy does not constrain the type of the copy. If we test on the stored state, it works out fine. Corrects this warning: qapi/schema.py:887: error: "QAPISchemaType" has no attribute "element_type" Signed-off-by: John Snow --- scripts/qapi/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 943f234ee2..09c7ceab41 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -764,7 +764,7 @@ def check(self, schema): self._ret_type_name, self.info, "command's 'returns'") if self.name not in self.info.pragma.returns_whitelist: typ = self.ret_type -if isinstance(typ, QAPISchemaArrayType): +if isinstance(self.ret_type, QAPISchemaArrayType): typ = self.ret_type.element_type assert typ if not isinstance(typ, QAPISchemaObjectType): -- 2.26.2
[PATCH 14/25] qapi/schema.py: Assert variants of an object are also objects
The 'seen' dictionaries are invariant types and require the most abstracted type to maintain a consistent interface with other check_clash implementations. In this case, we happen to know (and require) that they will be object types, so add a runtime assertion to constrain that type. Corrects this warning: qapi/schema.py:718: error: Item "QAPISchemaType" of "Optional[QAPISchemaType]" has no attribute "check_clash" Signed-off-by: John Snow --- scripts/qapi/schema.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 09c7ceab41..b502eee9cb 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -620,6 +620,7 @@ def check_clash(self, info, seen): for v in self.variants: # Reset seen map for each variant, since qapi names from one # branch do not affect another branch +assert isinstance(v.type, QAPISchemaObjectType) v.type.check_clash(info, dict(seen)) -- 2.26.2
[PATCH 12/25] qapi/schema.py: remove 'and' from non-bool rvalue expressions
There's nothing inherently wrong with using 'and' in this manner, but the type returned is more loose than you might expect. Any value can be false-ish, so mypy assumes that the type of the expression must be the Union of both types, because a type can always be false-ish. To tighten the static analysis type, we have to use the slightly clunkier but more formally correct ternary. Fixes these errors: qapi/schema.py:103: error: Argument 1 to "module_by_fname" of "QAPISchema" has incompatible type "Union[QAPISourceInfo, str]"; expected "Optional[str]" qapi/schema.py:380: error: Argument 3 to "resolve_type" of "QAPISchema" has incompatible type "Union[QAPISourceInfo, str]"; expected "Union[str, Callable[[Optional[QAPISourceInfo]], str], None]" Signed-off-by: John Snow --- scripts/qapi/schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 57343a1002..943f234ee2 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -81,7 +81,7 @@ def check_doc(self): def _set_module(self, schema, info): assert self._checked -self._module = schema.module_by_fname(info and info.fname) +self._module = schema.module_by_fname(info.fname if info else None) self._module.add_entity(self) def set_module(self, schema): @@ -305,7 +305,7 @@ def check(self, schema): super().check(schema) self.element_type = schema.resolve_type( self._element_type_name, self.info, -self.info and self.info.defn_meta) +self.info.defn_meta if self.info else None) assert not isinstance(self.element_type, QAPISchemaArrayType) def set_module(self, schema): -- 2.26.2
[PATCH 05/25] qapi/schema.py: constrain QAPISchemaObjectTypeMember arg_type type
Re-order the check method slightly in order to provide stronger typing for the arg_type field. Signed-off-by: John Snow --- scripts/qapi/schema.py | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 3aa842be08..c9a62711c2 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -738,13 +738,14 @@ def __init__(self, name, info, doc, ifcond, features, def check(self, schema): super().check(schema) if self._arg_type_name: -self.arg_type = schema.resolve_type( +arg_type = schema.resolve_type( self._arg_type_name, self.info, "command's 'data'") -if not isinstance(self.arg_type, QAPISchemaObjectType): +if not isinstance(arg_type, QAPISchemaObjectType): raise QAPISemError( self.info, "command's 'data' cannot take %s" -% self.arg_type.describe()) +% arg_type.describe()) +self.arg_type = arg_type if self.arg_type.variants and not self.boxed: raise QAPISemError( self.info, -- 2.26.2
[PATCH 26/26] qapi/parser.py: enable pylint checks
Signed-off-by: John Snow --- scripts/qapi/pylintrc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index 88efbf71cb..5091a08f12 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -2,8 +2,7 @@ # Add files or directories matching the regex patterns to the ignore list. # The regex matches against base names, not paths. -ignore-patterns=parser.py, -schema.py, +ignore-patterns=schema.py, [MESSAGES CONTROL] -- 2.26.2
[PATCH 06/25] qapi/schema.py: constrain QAPISchemaEvent arg_type type
Signed-off-by: John Snow --- scripts/qapi/schema.py | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index c9a62711c2..aaf20f776b 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -792,13 +792,14 @@ def __init__(self, name, info, doc, ifcond, features, arg_type, boxed): def check(self, schema): super().check(schema) if self._arg_type_name: -self.arg_type = schema.resolve_type( +arg_type = schema.resolve_type( self._arg_type_name, self.info, "event's 'data'") -if not isinstance(self.arg_type, QAPISchemaObjectType): +if not isinstance(arg_type, QAPISchemaObjectType): raise QAPISemError( self.info, "event's 'data' cannot take %s" -% self.arg_type.describe()) +% arg_type.describe()) +self.arg_type = arg_type if self.arg_type.variants and not self.boxed: raise QAPISemError( self.info, -- 2.26.2
[PATCH 10/25] qapi/schema.py: Add assertion to ifcond property
ifcond's initialization allows a fairly confusing type, but we want to assert that after the QAPISchema is built, this always returns a List[str]. Add an assertion to allow us to say that. (Note: Technically, I only assert that it's a list -- but type hints that will be added later will make it clear that the only possible list we could have here is a list[str], and this assertion is sufficient to prove the point to mypy.) Signed-off-by: John Snow --- scripts/qapi/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index b35f741c6f..62b1a7e890 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -88,7 +88,7 @@ def set_module(self, schema): @property def ifcond(self): -assert self._checked +assert self._checked and isinstance(self._ifcond, list) return self._ifcond def is_implicit(self): -- 2.26.2
[PATCH 20/26] qapi/parser.py: remove unused check_args_section arguments
Signed-off-by: John Snow --- scripts/qapi/parser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index fa0ddad922..a3403d4017 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -650,7 +650,7 @@ def check_expr(self, expr): def check(self): -def check_args_section(args, info, what): +def check_args_section(args): bogus = [name for name, section in args.items() if not section.member] if bogus: @@ -661,5 +661,5 @@ def check_args_section(args, info, what): "', '".join(bogus), "do" if len(bogus) > 1 else "does")) -check_args_section(self.args, self.info, 'members') -check_args_section(self.features, self.info, 'features') +check_args_section(self.args) +check_args_section(self.features) -- 2.26.2
[PATCH 24/26] qapi/parser.py: remove one and two-letter variables
Standard pylint complaint: use more descriptibe variable names. OK,
fine.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index cdb64ffc22..818f8bc580 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -57,8 +57,8 @@ class QAPIParseError(QAPISourceError):
@classmethod
def make(cls: Type[T], parser: 'QAPISchemaParser', msg: str) -> T:
col = 1
-for ch in parser.src[parser.line_pos:parser.pos]:
-if ch == '\t':
+for char in parser.src[parser.line_pos:parser.pos]:
+if char == '\t':
col = (col + 7) % 8 + 1
else:
col += 1
@@ -100,14 +100,16 @@ def __init__(self, fname: str,
try:
with open(self._fname, 'r', encoding='utf-8') as fp:
self.src = fp.read()
-except IOError as e:
+except IOError as err:
msg = "can't read {kind:s} file '{fname:s}': {errmsg:s}".format(
kind='include' if parent else 'schema',
fname=self._fname,
-errmsg=e.strerror
+errmsg=err.strerror
)
context = parent_info if parent_info else self.info
-raise QAPIParseError(context, msg) from e
+raise QAPIParseError(context, msg) from err
+
+# Showtime!
self._parse()
def _parse(self) -> None:
@@ -245,25 +247,25 @@ def accept(self, skip_comment: bool = True) -> None:
string = ''
esc = False
while True:
-ch = self.src[self.cursor]
+char = self.src[self.cursor]
self.cursor += 1
-if ch == '\n':
+if char == '\n':
raise self._parse_error("missing terminating \"'\"")
if esc:
# Note: we recognize only \\ because we have
# no use for funny characters in strings
-if ch != '\\':
-raise self._parse_error(f"unknown escape \\{ch}")
+if char != '\\':
+raise self._parse_error(f"unknown escape \\{char}")
esc = False
-elif ch == '\\':
+elif char == '\\':
esc = True
continue
-elif ch == "'":
+elif char == "'":
self.val = string
return
-if ord(ch) < 32 or ord(ch) >= 127:
+if ord(char) < 32 or ord(char) >= 127:
raise self._parse_error("funny character in string")
-string += ch
+string += char
elif self.src.startswith('true', self.pos):
self.val = True
self.cursor += 3
--
2.26.2
[PATCH 09/25] qapi/schema.py: remove superfluous assert
We'll actually get a better error message by just letting the dictionary lookup fail. Signed-off-by: John Snow --- scripts/qapi/schema.py | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index a84d8549a4..b35f741c6f 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -892,7 +892,6 @@ def _make_module(self, fname): def module_by_fname(self, fname): name = self._module_name(fname) -assert name in self._module_dict return self._module_dict[name] def _def_include(self, expr, info, doc): -- 2.26.2
[PATCH 14/26] qapi/parser.py: add docstrings
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 38 ++
1 file changed, 38 insertions(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index d9aae4ddb7..490436b48a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -61,7 +61,15 @@ class QAPIDocError(QAPIError):
class QAPISchemaParser:
+"""
+Performs parsing of a QAPI schema source file.
+:param fname: Path to the source file
+:param previously_included: Set of absolute paths of previously included
+source files; these will not be parsed again.
+:param incl_info: QAPISourceInfo for the parent document;
+ Can be None for the parent document.
+"""
def __init__(self,
fname: str,
previously_included: Optional[Set[str]] = None,
@@ -97,6 +105,10 @@ def __init__(self,
self._parse()
def _parse(self) -> None:
+"""
+Parse the QAPI Schema Document.
+Build self.exprs, self.docs
+"""
cur_doc = None
# Prime the lexer:
@@ -216,6 +228,32 @@ def _pragma(cls,
raise QAPISemError(info, "unknown pragma '%s'" % name)
def accept(self, skip_comment: bool = True) -> None:
+"""
+Read the next lexeme.
+
+- `tok` is the current lexeme/token type.
+ It will always be a single char in `"[]{},:'tf#"`.
+- `pos` is the position of the first character in the lexeme.
+- `cursor` is the position of the next character.
+- `val` is the value of the lexeme.
+
+Single-char lexemes:
+ LBRACE, RBRACE, COLON, COMMA, LSQB, RSQB:
+`tok` holds the single-char value of the lexeme.
+
+Multi-char lexemes:
+ COMMENT - `tok` is `'#'`.
+`val` is a string including all chars until end-of-line.
+ (The '#' is excluded.)
+ STRING - `tok` is `"'"`.
+`val` is the string, excluding the quotes.
+ TRUE- `tok` is `"t"`. `val` is `True`.
+ FALSE - `tok` is `"f"`. `val` is `False`.
+
+NEWLINE and SPACE lexemes are consumed by the lexer directly.
+`line_pos` and `info` are advanced when NEWLINE is encountered.
+`tok` is set to `None` upon reaching EOF.
+"""
while True:
self.tok = self.src[self.cursor]
self.pos = self.cursor
--
2.26.2
[PATCH 21/26] qapi/parser.py: QAPIDoc: convert @staticmethod to @classmethod
For consistency: replace @staticmethod with @classmethod.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index a3403d4017..f5f40ffa16 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -465,8 +465,8 @@ def append(self, line):
def end_comment(self):
self._end_section()
-@staticmethod
-def _is_section_tag(name):
+@classmethod
+def _is_section_tag(cls, name):
return name in ('Returns:', 'Since:',
# those are often singular or plural
'Note:', 'Notes:',
--
2.26.2
Re: [PATCH v2 13/38] qapi/common.py: add type hint annotations
On Tue, Sep 22, 2020 at 05:00:36PM -0400, John Snow wrote:
> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/common.py | 27 ---
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 682e74fe65..0ce4a107e6 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,6 +12,7 @@
[...]
> @@ -176,7 +179,7 @@ def gen_if(ifcond):
> return ret
>
>
> -def gen_endif(ifcond):
> +def gen_endif(ifcond: Sequence[str]) -> str:
Does this need to require a Sequence? It looks like it could be
Iterable.
I don't think this should block the patch, though, so:
Reviewed-by: Eduardo Habkost
> ret = ''
> for ifc in reversed(ifcond):
> ret += mcgen('''
> @@ -185,7 +188,9 @@ def gen_endif(ifcond):
> return ret
>
>
> -def build_params(arg_type, boxed, extra=None):
> +def build_params(arg_type,
> + boxed: bool,
> + extra: Optional[str] = None) -> str:
> ret = ''
> sep = ''
> if boxed:
> --
> 2.26.2
>
--
Eduardo
[PATCH 04/25] qapi/schema.py: constrain QAPISchemaObjectType base type
Re-order check slightly so we can provide a stronger guarantee on the typing of the base field. Signed-off-by: John Snow --- scripts/qapi/schema.py | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index a53631e660..3aa842be08 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -378,14 +378,14 @@ def check(self, schema): seen = OrderedDict() if self._base_name: -self.base = schema.resolve_type(self._base_name, self.info, -"'base'") -if (not isinstance(self.base, QAPISchemaObjectType) -or self.base.variants): +base = schema.resolve_type(self._base_name, self.info, "'base'") +if (not isinstance(base, QAPISchemaObjectType) +or base.variants): raise QAPISemError( self.info, "'base' requires a struct type, %s isn't" -% self.base.describe()) +% base.describe()) +self.base = base self.base.check(schema) self.base.check_clash(self.info, seen) for m in self.local_members: -- 2.26.2
[PATCH 25/26] qapi/parser.py: Silence too-few-public-methods warning
Signed-off-by: John Snow --- scripts/qapi/parser.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 818f8bc580..e3481b02f2 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -412,6 +412,8 @@ class QAPIDoc: """ class Section: +# pylint: disable=too-few-public-methods + def __init__(self, name: Optional[str] = None): # optional section name (argument/member or section name) self.name = name -- 2.26.2
[PATCH 15/26] qapi/parser.py: add ParsedExpression type
This is an immutable, named, typed tuple; it's nicer than arbitrary
dicts for passing data around when using strict typing.
Turn parser.exprs into a list of ParsedExpressions instead, and adjust
expr.py to match.
Signed-off-by: John Snow
---
scripts/qapi/expr.py | 56 +++---
scripts/qapi/parser.py | 32 +---
scripts/qapi/schema.py | 6 ++---
3 files changed, 41 insertions(+), 53 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index cfd342aa04..f2059c505c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -46,7 +46,7 @@
from .common import c_name
from .error import QAPISemError
-from .parser import QAPIDoc
+from .parser import ParsedExpression
from .source import QAPISourceInfo
@@ -517,7 +517,7 @@ class ExpressionType(str, Enum):
}
-def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
+def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
"""
Validate and normalize a list of parsed QAPI schema expressions. [RW]
@@ -526,49 +526,33 @@ def check_exprs(exprs: List[_JSObject]) ->
List[_JSObject]:
:param exprs: The list of expressions to normalize/validate.
"""
-for expr_elem in exprs:
-# Expression
-assert isinstance(expr_elem['expr'], dict)
-expr: Expression = expr_elem['expr']
-for key in expr.keys():
-assert isinstance(key, str)
-
-# QAPISourceInfo
-assert isinstance(expr_elem['info'], QAPISourceInfo)
-info: QAPISourceInfo = expr_elem['info']
-
-# Optional[QAPIDoc]
-tmp = expr_elem.get('doc')
-assert tmp is None or isinstance(tmp, QAPIDoc)
-doc: Optional[QAPIDoc] = tmp
-
+for expr in exprs:
for kind in ExpressionType:
-if kind in expr:
+if kind in expr.expr:
meta = kind
break
else:
-raise QAPISemError(info, "expression is missing metatype")
+raise QAPISemError(expr.info, "expression is missing metatype")
if meta == ExpressionType.INCLUDE:
continue
-name = cast(str, expr[meta]) # asserted right below:
-check_name_is_str(name, info, "'%s'" % meta.value)
-info.set_defn(meta.value, name)
-check_defn_name_str(name, info, meta.value)
+name = cast(str, expr.expr[meta]) # asserted right below:
+check_name_is_str(name, expr.info, "'%s'" % meta.value)
+expr.info.set_defn(meta.value, name)
+check_defn_name_str(name, expr.info, meta.value)
-if doc:
-if doc.symbol != name:
-raise QAPISemError(
-info, "documentation comment is for '%s'" % doc.symbol)
-doc.check_expr(expr)
-elif info.pragma.doc_required:
-raise QAPISemError(info,
- "documentation comment required")
+if expr.doc:
+if expr.doc.symbol != name:
+msg = f"documentation comment is for '{expr.doc.symbol}'"
+raise QAPISemError(expr.info, msg)
+expr.doc.check_expr(expr.expr)
+elif expr.info.pragma.doc_required:
+raise QAPISemError(expr.info, "documentation comment required")
-_CHECK_FN[meta](expr, info)
-check_if(expr, info, meta.value)
-check_features(expr.get('features'), info)
-check_flags(expr, info)
+_CHECK_FN[meta](expr.expr, expr.info)
+check_if(expr.expr, expr.info, meta.value)
+check_features(expr.expr.get('features'), expr.info)
+check_flags(expr.expr, expr.info)
return exprs
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 490436b48a..f65afa4eb2 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -19,9 +19,8 @@
import os
import re
from typing import (
-Any,
-Dict,
List,
+NamedTuple,
Optional,
Set,
Type,
@@ -34,13 +33,18 @@
from .source import QAPISourceInfo
-Expression = Dict[str, Any]
_Value = Union[List[object], 'OrderedDict[str, object]', str, bool]
# Necessary imprecision: mypy does not (yet?) support recursive types;
# so we must stub out that recursion with 'object'.
# Note, we do not support numerics or null in this parser.
+class ParsedExpression(NamedTuple):
+expr: 'OrderedDict[str, object]'
+info: QAPISourceInfo
+doc: Optional['QAPIDoc']
+
+
class QAPIParseError(QAPISourceError):
"""Error class for all QAPI schema parsing errors."""
T = TypeVar('T', bound='QAPIParseError')
@@ -87,7 +91,7 @@ def __init__(self,
self.line_pos = 0
# Parser output:
-self.exprs: List[Expression] = []
+self.exprs: List[ParsedExpression] = []
self.docs: List[QAPIDoc] = []
# Showtime!
@@ -139,8 +143,7 @@ def _parse(self) -> None:
"value of 'includ
[PATCH 11/26] qapi/parser.py: Convert several methods to @classmethod
It's usually nicer to keep static methods as class methods -- this allows them to call other class methods, to be subclassed and extended, etc. Meanwhile, any method that doesn't utilize `self` can be a class method. Signed-off-by: John Snow --- scripts/qapi/parser.py | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 756c904257..75a693a9d7 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -139,15 +139,16 @@ def _parse(self): def _parse_error(self, msg: str) -> QAPIParseError: return QAPIParseError.make(self, msg) -@staticmethod -def reject_expr_doc(doc): +@classmethod +def reject_expr_doc(cls, doc): if doc and doc.symbol: raise QAPISemError( doc.info, "documentation for '%s' is not followed by the definition" % doc.symbol) -def _include(self, include, info, incl_fname, previously_included): +@classmethod +def _include(cls, include, info, incl_fname, previously_included): incl_abs_fname = os.path.abspath(incl_fname) # catch inclusion cycle inf = info @@ -162,7 +163,8 @@ def _include(self, include, info, incl_fname, previously_included): return QAPISchemaParser(incl_fname, previously_included, info) -def _pragma(self, name, value, info): +@classmethod +def _pragma(cls, name, value, info): if name == 'doc-required': if not isinstance(value, bool): raise QAPISemError(info, -- 2.26.2
[PATCH 23/26] qapi/parser.py: enable mypy checks
Signed-off-by: John Snow --- scripts/qapi/mypy.ini | 5 - 1 file changed, 5 deletions(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 4d341c6b2d..20ab509946 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -4,11 +4,6 @@ strict_optional = False disallow_untyped_calls = False python_version = 3.6 -[mypy-qapi.parser] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False - [mypy-qapi.schema] disallow_untyped_defs = False disallow_incomplete_defs = False -- 2.26.2
[PATCH 10/26] qapi/parser.py: assert object keys are strings
Since values can also be other data types, add an assertion to ensure
we're dealing with strings.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 1bc33e85ea..756c904257 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -256,6 +256,8 @@ def get_members(self):
raise self._parse_error("expected string or '}'")
while True:
key = self.val
+assert isinstance(key, str), f"expected str, got {type(key)!s}"
+
self.accept()
if self.tok != ':':
raise self._parse_error("expected ':'")
--
2.26.2
[PATCH 03/25] qapi/schema.py: add assert in stub methods
Instead of pass (an implicit return none), use raise NotImplementedError
to mark a function as abstract -- one that doesn't return. This allows
us to correctly type the stub.
Signed-off-by: John Snow
---
scripts/qapi/schema.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 0201b42095..a53631e660 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -176,8 +176,8 @@ def visit(self, visitor):
class QAPISchemaType(QAPISchemaEntity):
# Return the C type for common use.
# For the types we commonly box, this is a pointer type.
-def c_type(self):
-pass
+def c_type(self) -> str:
+raise NotImplementedError()
# Return the C type to be used in a parameter list.
def c_param_type(self):
@@ -187,8 +187,8 @@ def c_param_type(self):
def c_unboxed_type(self):
return self.c_type()
-def json_type(self):
-pass
+def json_type(self) -> str:
+raise NotImplementedError()
def alternate_qtype(self):
json2qtype = {
--
2.26.2
[PATCH 16/26] qapi/pragma.py: Move QAPISchemaPragma into its own module
Signed-off-by: John Snow --- scripts/qapi/pragma.py | 25 + scripts/qapi/source.py | 15 ++- 2 files changed, 27 insertions(+), 13 deletions(-) create mode 100644 scripts/qapi/pragma.py diff --git a/scripts/qapi/pragma.py b/scripts/qapi/pragma.py new file mode 100644 index 00..7f3db9ab87 --- /dev/null +++ b/scripts/qapi/pragma.py @@ -0,0 +1,25 @@ +# +# QAPI pragma information +# +# Copyright (c) 2020 John Snow, for Red Hat Inc. +# +# Authors: +# John Snow +# +# This work is licensed under the terms of the GNU GPL, version 2. +# See the COPYING file in the top-level directory. + +from typing import List + + +class QAPISchemaPragma: +# Replace with @dataclass in Python 3.7+ +# pylint: disable=too-few-public-methods + +def __init__(self) -> None: +# Are documentation comments required? +self.doc_required = False +# Whitelist of commands allowed to return a non-dictionary +self.returns_whitelist: List[str] = [] +# Whitelist of entities allowed to violate case conventions +self.name_case_whitelist: List[str] = [] diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py index d1de9e36b1..fe1424be03 100644 --- a/scripts/qapi/source.py +++ b/scripts/qapi/source.py @@ -11,20 +11,9 @@ import copy import sys -from typing import List, Optional, TypeVar +from typing import Optional, TypeVar - -class QAPISchemaPragma: -# Replace with @dataclass in Python 3.7+ -# pylint: disable=too-few-public-methods - -def __init__(self) -> None: -# Are documentation comments required? -self.doc_required = False -# Whitelist of commands allowed to return a non-dictionary -self.returns_whitelist: List[str] = [] -# Whitelist of entities allowed to violate case conventions -self.name_case_whitelist: List[str] = [] +from .pragma import QAPISchemaPragma class QAPISourceInfo: -- 2.26.2
[PATCH 19/26] qapi/parser.py: add parent argument
Instead of passing previously_included and info separately, we can pass
the parent parser itself. This cuts down on the number of parameters to
pass when creating a parser; and makes it easier to add new shared data
members between parent and child.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 77067b2f5d..fa0ddad922 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -68,26 +68,23 @@ class QAPISchemaParser:
"""
Performs parsing of a QAPI schema source file.
-:param fname: Path to the source file
-:param previously_included: Set of absolute paths of previously included
-source files; these will not be parsed again.
-:param incl_info: QAPISourceInfo for the parent document;
- Can be None for the parent document.
+:param fname: Path to the source file
+:param parent: Parent parser, if this is an included file.
"""
-def __init__(self,
- fname: str,
- previously_included: Optional[Set[str]] = None,
- incl_info: Optional[QAPISourceInfo] = None):
+def __init__(self, fname: str,
+ parent: Optional['QAPISchemaParser'] = None):
self._fname = fname
-self._included = previously_included or set()
+self._included: Set[str] = parent._included if parent else set()
self._included.add(os.path.abspath(self._fname))
+parent_info = parent.info if parent else None
# Lexer state (see `accept` for details):
self.tok: Optional[str] = None
self.pos = 0
self.cursor = 0
self.val: Optional[Union[bool, str]] = None
-self.info = QAPISourceInfo(self._fname, parent=incl_info)
+self.info: QAPISourceInfo = QAPISourceInfo(self._fname,
+ parent=parent_info)
self.line_pos = 0
# Parser output:
@@ -100,11 +97,11 @@ def __init__(self,
self.src = fp.read()
except IOError as e:
msg = "can't read {kind:s} file '{fname:s}': {errmsg:s}".format(
-kind='include' if incl_info else 'schema',
+kind='include' if parent else 'schema',
fname=self._fname,
errmsg=e.strerror
)
-context = incl_info or self.info
+context = parent_info if parent_info else self.info
raise QAPIParseError(context, msg) from e
self._parse()
@@ -193,7 +190,7 @@ def _include(self, include: str,
if incl_abs_fname in self._included:
return None
-return QAPISchemaParser(incl_fname, self._included, self.info)
+return QAPISchemaParser(incl_fname, self)
def accept(self, skip_comment: bool = True) -> None:
"""
--
2.26.2
[PATCH 09/26] qapi/parser.py: assert get_expr returns object in outer loop
get_expr can return many things, depending on where it is used. In the outer parsing loop, we expect and require it to return an object. Signed-off-by: John Snow --- scripts/qapi/parser.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 6774b6c736..1bc33e85ea 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -94,6 +94,9 @@ def _parse(self): continue expr = self.get_expr(False) +if not isinstance(expr, dict): +raise QAPISemError(info, "Expecting object statement") + if 'include' in expr: self.reject_expr_doc(cur_doc) if len(expr) != 1: -- 2.26.2
[PATCH 12/26] qapi/parser.py: add casts to pragma checks
This kind of type checking at runtime is not something mypy can introspect, so add a do-nothing cast to help mypy out. Signed-off-by: John Snow --- scripts/qapi/parser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 75a693a9d7..9a1007f779 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -17,7 +17,7 @@ import os import re from collections import OrderedDict -from typing import Type, TypeVar +from typing import List, Type, TypeVar, cast from .error import QAPIError, QAPISourceError, QAPISemError from .source import QAPISourceInfo @@ -176,14 +176,14 @@ def _pragma(cls, name, value, info): raise QAPISemError( info, "pragma returns-whitelist must be a list of strings") -info.pragma.returns_whitelist = value +info.pragma.returns_whitelist = cast(List[str], value) elif name == 'name-case-whitelist': if (not isinstance(value, list) or any([not isinstance(elt, str) for elt in value])): raise QAPISemError( info, "pragma name-case-whitelist must be a list of strings") -info.pragma.name_case_whitelist = value +info.pragma.name_case_whitelist = cast(List[str], value) else: raise QAPISemError(info, "unknown pragma '%s'" % name) -- 2.26.2
[PATCH 13/26] qapi/parser.py: add type hint annotations
Annotations do not change runtime behavior.
This commit *only* adds annotations.
Annotations for QAPIDoc are in a later commit.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 69 ++
1 file changed, 49 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 9a1007f779..d9aae4ddb7 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -14,18 +14,35 @@
# This work is licensed under the terms of the GNU GPL, version 2.
# See the COPYING file in the top-level directory.
+
+from collections import OrderedDict
import os
import re
-from collections import OrderedDict
-from typing import List, Type, TypeVar, cast
+from typing import (
+Any,
+Dict,
+List,
+Optional,
+Set,
+Type,
+TypeVar,
+Union,
+cast,
+)
from .error import QAPIError, QAPISourceError, QAPISemError
from .source import QAPISourceInfo
+Expression = Dict[str, Any]
+_Value = Union[List[object], 'OrderedDict[str, object]', str, bool]
+# Necessary imprecision: mypy does not (yet?) support recursive types;
+# so we must stub out that recursion with 'object'.
+# Note, we do not support numerics or null in this parser.
+
+
class QAPIParseError(QAPISourceError):
"""Error class for all QAPI schema parsing errors."""
-
T = TypeVar('T', bound='QAPIParseError')
@classmethod
@@ -45,22 +62,25 @@ class QAPIDocError(QAPIError):
class QAPISchemaParser:
-def __init__(self, fname, previously_included=None, incl_info=None):
+def __init__(self,
+ fname: str,
+ previously_included: Optional[Set[str]] = None,
+ incl_info: Optional[QAPISourceInfo] = None):
self._fname = fname
self._included = previously_included or set()
self._included.add(os.path.abspath(self._fname))
# Lexer state (see `accept` for details):
-self.tok = None
+self.tok: Optional[str] = None
self.pos = 0
self.cursor = 0
-self.val = None
+self.val: Optional[Union[bool, str]] = None
self.info = QAPISourceInfo(self._fname, parent=incl_info)
self.line_pos = 0
# Parser output:
-self.exprs = []
-self.docs = []
+self.exprs: List[Expression] = []
+self.docs: List[QAPIDoc] = []
# Showtime!
try:
@@ -76,7 +96,7 @@ def __init__(self, fname, previously_included=None,
incl_info=None):
raise QAPIParseError(context, msg) from e
self._parse()
-def _parse(self):
+def _parse(self) -> None:
cur_doc = None
# Prime the lexer:
@@ -140,7 +160,7 @@ def _parse_error(self, msg: str) -> QAPIParseError:
return QAPIParseError.make(self, msg)
@classmethod
-def reject_expr_doc(cls, doc):
+def reject_expr_doc(cls, doc: Optional['QAPIDoc']) -> None:
if doc and doc.symbol:
raise QAPISemError(
doc.info,
@@ -148,7 +168,12 @@ def reject_expr_doc(cls, doc):
% doc.symbol)
@classmethod
-def _include(cls, include, info, incl_fname, previously_included):
+def _include(cls,
+ include: str,
+ info: QAPISourceInfo,
+ incl_fname: str,
+ previously_included: Set[str]
+ ) -> Optional['QAPISchemaParser']:
incl_abs_fname = os.path.abspath(incl_fname)
# catch inclusion cycle
inf = info
@@ -164,7 +189,10 @@ def _include(cls, include, info, incl_fname,
previously_included):
return QAPISchemaParser(incl_fname, previously_included, info)
@classmethod
-def _pragma(cls, name, value, info):
+def _pragma(cls,
+name: str,
+value: object,
+info: QAPISourceInfo) -> None:
if name == 'doc-required':
if not isinstance(value, bool):
raise QAPISemError(info,
@@ -187,7 +215,7 @@ def _pragma(cls, name, value, info):
else:
raise QAPISemError(info, "unknown pragma '%s'" % name)
-def accept(self, skip_comment=True):
+def accept(self, skip_comment: bool = True) -> None:
while True:
self.tok = self.src[self.cursor]
self.pos = self.cursor
@@ -249,8 +277,8 @@ def accept(self, skip_comment=True):
self.src[self.cursor-1:])
raise self._parse_error("stray '%s'" % match.group(0))
-def get_members(self):
-expr = OrderedDict()
+def get_members(self) -> 'OrderedDict[str, object]':
+expr: 'OrderedDict[str, object]' = OrderedDict()
if self.tok == '}':
self.accept()
return expr
@@ -276,8 +304,8 @@ def get_members(self):
if self.tok != "'":
raise self._parse_error("expected string")
-def get_values(self):
-expr = []
+
[PATCH 17/26] qapi/pragma.py: Move pragma parsing out of parser.py
parser.py is a JSON parser at heart and shouldn't necessarily understand what it is parsing on a semantic level. Move pragma parsing to pragma.py, and leave the parser a little more happily ignorant. Note: the type annotation in error.py now creates a cyclic import, because error -> source -> pragma -> error. Use the magical mypy constant TYPE_CHECKING to avoid this cycle at runtime. pylint dislikes this cycle still, but it can be safely ignored. Signed-off-by: John Snow --- scripts/qapi/error.py | 8 +++--- scripts/qapi/parser.py | 41 --- scripts/qapi/pragma.py | 55 +- 3 files changed, 59 insertions(+), 45 deletions(-) diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py index ab6a0f6271..be5fd24218 100644 --- a/scripts/qapi/error.py +++ b/scripts/qapi/error.py @@ -11,9 +11,11 @@ # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. -from typing import Optional +from typing import Optional, TYPE_CHECKING -from .source import QAPISourceInfo +if TYPE_CHECKING: +# pylint: disable=cyclic-import +from .source import QAPISourceInfo class QAPIError(Exception): @@ -23,7 +25,7 @@ class QAPIError(Exception): class QAPISourceError(QAPIError): """Error class for all exceptions identifying a source location.""" def __init__(self, - info: QAPISourceInfo, + info: 'QAPISourceInfo', msg: str, col: Optional[int] = None): super().__init__() diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index f65afa4eb2..5b3a9b5da8 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -26,10 +26,10 @@ Type, TypeVar, Union, -cast, ) from .error import QAPIError, QAPISourceError, QAPISemError +from .pragma import PragmaError from .source import QAPISourceInfo @@ -151,14 +151,10 @@ def _parse(self) -> None: self.docs.extend(exprs_include.docs) elif "pragma" in expr: self.reject_expr_doc(cur_doc) -if len(expr) != 1: -raise QAPISemError(info, "invalid 'pragma' directive") -pragma = expr['pragma'] -if not isinstance(pragma, dict): -raise QAPISemError( -info, "value of 'pragma' must be an object") -for name, value in pragma.items(): -self._pragma(name, value, info) +try: +info.pragma.parse(expr) +except PragmaError as err: +raise QAPISemError(info, str(err)) from err else: if cur_doc and not cur_doc.symbol: raise QAPISemError( @@ -204,33 +200,6 @@ def _include(cls, return QAPISchemaParser(incl_fname, previously_included, info) -@classmethod -def _pragma(cls, -name: str, -value: object, -info: QAPISourceInfo) -> None: -if name == 'doc-required': -if not isinstance(value, bool): -raise QAPISemError(info, - "pragma 'doc-required' must be boolean") -info.pragma.doc_required = value -elif name == 'returns-whitelist': -if (not isinstance(value, list) -or any([not isinstance(elt, str) for elt in value])): -raise QAPISemError( -info, -"pragma returns-whitelist must be a list of strings") -info.pragma.returns_whitelist = cast(List[str], value) -elif name == 'name-case-whitelist': -if (not isinstance(value, list) -or any([not isinstance(elt, str) for elt in value])): -raise QAPISemError( -info, -"pragma name-case-whitelist must be a list of strings") -info.pragma.name_case_whitelist = cast(List[str], value) -else: -raise QAPISemError(info, "unknown pragma '%s'" % name) - def accept(self, skip_comment: bool = True) -> None: """ Read the next lexeme. diff --git a/scripts/qapi/pragma.py b/scripts/qapi/pragma.py index 7f3db9ab87..03ba3cac90 100644 --- a/scripts/qapi/pragma.py +++ b/scripts/qapi/pragma.py @@ -9,17 +9,60 @@ # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. -from typing import List +from typing import Mapping, Sequence + +from .error import QAPIError + + +class PragmaError(QAPIError): +"""For errors relating to Pragma validation.""" class QAPISchemaPragma: -# Replace with @dataclass in Python 3.7+ -# pylint: disable=too-few-public-methods - def __init__(self) -> None: # Are documentation comments required? self.doc_
[PATCH 06/26] qapi/parser.py: raise QAPIParseError during file opening
Now that we can have exception contexts that don't specify a specific
line, we can use that context to raise errors when opening the file.
If we don't have a parent context, we can simply use our own.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index fc0b1516cc..5a7233bc76 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -67,11 +67,13 @@ def __init__(self, fname, previously_included=None,
incl_info=None):
with open(self._fname, 'r', encoding='utf-8') as fp:
self.src = fp.read()
except IOError as e:
-raise QAPISemError(incl_info or QAPISourceInfo(None),
- "can't read %s file '%s': %s"
- % ("include" if incl_info else "schema",
- self._fname,
- e.strerror))
+msg = "can't read {kind:s} file '{fname:s}': {errmsg:s}".format(
+kind='include' if incl_info else 'schema',
+fname=self._fname,
+errmsg=e.strerror
+)
+context = incl_info or self.info
+raise QAPIParseError(context, msg) from e
self._parse()
def _parse(self):
--
2.26.2
[PATCH 08/26] qapi/parser.py: Assert lexer value is a string
The type checker can't constrain the token value to string in this case,
because it's only loosely correlated with the return token, which is
"stringly typed".
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 78355ca93f..6774b6c736 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -312,6 +312,7 @@ def _get_doc(self, info):
cur_doc = QAPIDoc(info)
self.accept(False)
while self.tok == '#':
+assert isinstance(self.val, str), "Expected str value"
if self.val.startswith('##'):
# End of doc comment
if self.val != '##':
--
2.26.2
[PATCH 01/26] qapi/parser.py: refactor parsing routine into method
For the sake of keeping __init__ easier to reason about, with fewer
variables in play.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 8be4570c31..55117f5754 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -46,28 +46,34 @@ class QAPIDocError(QAPIError):
class QAPISchemaParser:
def __init__(self, fname, previously_included=None, incl_info=None):
-previously_included = previously_included or set()
-previously_included.add(os.path.abspath(fname))
+self._fname = fname
+self._included = previously_included or set()
+self._included.add(os.path.abspath(self._fname))
+
+self.cursor = 0
+self.info = QAPISourceInfo(self._fname, 1, incl_info)
+self.line_pos = 0
+self.exprs = []
+self.docs = []
try:
-fp = open(fname, 'r', encoding='utf-8')
+fp = open(self._fname, 'r', encoding='utf-8')
self.src = fp.read()
except IOError as e:
raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
"can't read %s file '%s': %s"
% ("include" if incl_info else "schema",
- fname,
+ self._fname,
e.strerror))
+self._parse()
+def _parse(self):
+cur_doc = None
+
+# Prime the lexer:
if self.src == '' or self.src[-1] != '\n':
self.src += '\n'
-self.cursor = 0
-self.info = QAPISourceInfo(fname, 1, incl_info)
-self.line_pos = 0
-self.exprs = []
-self.docs = []
self.accept()
-cur_doc = None
while self.tok is not None:
info = self.info
@@ -86,12 +92,12 @@ def __init__(self, fname, previously_included=None,
incl_info=None):
if not isinstance(include, str):
raise QAPISemError(info,
"value of 'include' must be a string")
-incl_fname = os.path.join(os.path.dirname(fname),
+incl_fname = os.path.join(os.path.dirname(self._fname),
include)
self.exprs.append({'expr': {'include': incl_fname},
'info': info})
exprs_include = self._include(include, info, incl_fname,
- previously_included)
+ self._included)
if exprs_include:
self.exprs.extend(exprs_include.exprs)
self.docs.extend(exprs_include.docs)
--
2.26.2
[PATCH 18/26] qapi/parser.py: Modify _include() to use parser state
It doesn't need to take quite so many arguments.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 21 -
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 5b3a9b5da8..77067b2f5d 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -144,8 +144,7 @@ def _parse(self) -> None:
incl_fname = os.path.join(os.path.dirname(self._fname),
include)
self._add_expr(OrderedDict({'include': incl_fname}), info)
-exprs_include = self._include(include, info, incl_fname,
- self._included)
+exprs_include = self._include(include, incl_fname)
if exprs_include:
self.exprs.extend(exprs_include.exprs)
self.docs.extend(exprs_include.docs)
@@ -179,26 +178,22 @@ def reject_expr_doc(cls, doc: Optional['QAPIDoc']) ->
None:
"documentation for '%s' is not followed by the definition"
% doc.symbol)
-@classmethod
-def _include(cls,
- include: str,
- info: QAPISourceInfo,
- incl_fname: str,
- previously_included: Set[str]
- ) -> Optional['QAPISchemaParser']:
+def _include(self, include: str,
+ incl_fname: str) -> Optional['QAPISchemaParser']:
incl_abs_fname = os.path.abspath(incl_fname)
+
# catch inclusion cycle
-inf = info
+inf = self.info
while inf:
if incl_abs_fname == os.path.abspath(inf.fname):
-raise QAPISemError(info, "inclusion loop for %s" % include)
+raise QAPISemError(self.info, f"inclusion loop for {include}")
inf = inf.parent
# skip multiple include of the same file
-if incl_abs_fname in previously_included:
+if incl_abs_fname in self._included:
return None
-return QAPISchemaParser(incl_fname, previously_included, info)
+return QAPISchemaParser(incl_fname, self._included, self.info)
def accept(self, skip_comment: bool = True) -> None:
"""
--
2.26.2
[PATCH 07/26] qapi/parser.py: fully remove 'null' constant
Based on the docs, we don't support the null constant, and the code
agrees. There's a few remnants where callers check .tok for 'n', and
these can be removed.
Signed-off-by: John Snow
---
scripts/qapi/parser.py| 8
tests/qapi-schema/leading-comma-list.err | 2 +-
tests/qapi-schema/trailing-comma-list.err | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 5a7233bc76..78355ca93f 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -274,9 +274,9 @@ def get_values(self):
if self.tok == ']':
self.accept()
return expr
-if self.tok not in "{['tfn":
+if self.tok not in "{['tf":
raise self._parse_error(
-"expected '{', '[', ']', string, boolean or 'null'")
+"expected '{', '[', ']', string, or boolean")
while True:
expr.append(self.get_expr(True))
if self.tok == ']':
@@ -295,12 +295,12 @@ def get_expr(self, nested):
elif self.tok == '[':
self.accept()
expr = self.get_values()
-elif self.tok in "'tfn":
+elif self.tok in "'tf":
expr = self.val
self.accept()
else:
raise self._parse_error(
-"expected '{', '[', string, boolean or 'null'")
+"expected '{', '[', string, or boolean")
return expr
def _get_doc(self, info):
diff --git a/tests/qapi-schema/leading-comma-list.err
b/tests/qapi-schema/leading-comma-list.err
index 76eed2b5b3..0725d6529f 100644
--- a/tests/qapi-schema/leading-comma-list.err
+++ b/tests/qapi-schema/leading-comma-list.err
@@ -1 +1 @@
-leading-comma-list.json:2:13: expected '{', '[', ']', string, boolean or 'null'
+leading-comma-list.json:2:13: expected '{', '[', ']', string, or boolean
diff --git a/tests/qapi-schema/trailing-comma-list.err
b/tests/qapi-schema/trailing-comma-list.err
index ad2f2d7c97..bb5f8c3c90 100644
--- a/tests/qapi-schema/trailing-comma-list.err
+++ b/tests/qapi-schema/trailing-comma-list.err
@@ -1 +1 @@
-trailing-comma-list.json:2:36: expected '{', '[', string, boolean or 'null'
+trailing-comma-list.json:2:36: expected '{', '[', string, or boolean
--
2.26.2
