Re: [PATCH RFCv1 05/10] hw/arm/virt: Add VIRT_NESTED_SMMU
On Tue, Jul 09, 2024 at 10:59:30AM GMT, Nicolin Chen wrote:
> On Tue, Jul 09, 2024 at 03:26:58PM +0200, Eric Auger wrote:
> > > +/* Build PCI Expander Bridge + Root Port from the top of PCI_BUS_MAX
> > > */
> > > +if (vms->num_nested_smmus) {
> > > +/* VIRT_NESTED_SMMU must hold all vSMMUs */
> > > +g_assert(vms->num_nested_smmus <=
> > > + vms->memmap[VIRT_NESTED_SMMU].size / SMMU_IO_LEN);
> > > +
> > > +vms->nested_smmu_phandle = g_new0(uint32_t,
> > > vms->num_nested_smmus);
> > > +
> > > +for (i = 0; i < vms->num_nested_smmus; i++) {
> > > +DeviceState *smmu_dev;
> > > +PCIBus *pxb_bus;
> > > +
> > > +pxb_bus = create_pcie_expander_bridge(vms, i);
> > > +g_assert(pxb_bus);
> > > +
> > > +vms->nested_smmu_phandle[i] =
> > > qemu_fdt_alloc_phandle(ms->fdt);
> > > +smmu_dev = create_nested_smmu(vms, pxb_bus, i);
> > > +g_assert(smmu_dev);
> > > +
> > > +qemu_fdt_setprop_cells(ms->fdt, nodename, "iommu-map", 0x0,
> > > + vms->nested_smmu_phandle[i], 0x0,
> > > 0x1);
>
> > I think libvirt is supposed to create the pcie bus topology instead and
> > it shall not be created by qemu behind the scene.
> > The pcie elements you create here are not visible to libvirt and I guess
> > they may collide with elements explicitly created by libvirt at a given
> > pci bdf.
>
> Yea, the bdf conflict is a concern. So I allocated the bdf list
> from the top of the bus number... one of the reasons of doing
> this is to ease users so they don't need to deal with the over-
> complicated topology. I will try libvirt and see how it goes.
libvirt developer here. Eric is absolutely right that creating PCI
controllers implicitly will not fly, as libvirt's PCI address
allocation algorithm would not know about them and would produce
incorrect output as a result.
> > I think it would make more sense to be able to attach an smmu instance
> > to a given pci root or pxb either by adding an iommu id to a given
> > pxb-pcie option
> >
> > -device
> > pxb-pcie,bus_nr=100,id=pci.12,numa_node=0,bus=pcie.0,addr=0x3,iommu=
> > or
> > adding a list of pxb ids to the iommu option. It is unfortunate the
> > iommu option is a machine option.
>
> Yes. I had thought about that too, but the virt-machine code
> creates all the instance at this moment...
>
> > platform bus framework could be considered to dynamically allocate them
> > using the -device option. This has been used along with dt generation
> > but with ACPI this would need to be studied. However at the time the
> > smmu was integrated the machine option was prefered.
> >
> > Maybe using the 1st option would allow to infer that if there are
> > different iommu ids this implies that several IOMMU instances need to be
> > created.
>
> Yea, I like the idea of creating iommu instance with a "-device"
> string.
This sounds like the best option to me as well. Incidentally, it's
also how the intel-iommu device works. From libvirt's test suite:
-device '{"driver":"intel-iommu","id":"iommu0"}'
Looks like a sensible model to follow. You could then associate each
specific IOMMU instance to a PCI controller using an attribute, as
Eric suggested above.
> One more question. Let's say we have 2 smmus/pxb-buses:
> [ pxb0] <---> vSMMU0/pSMMU0 [ devA, devB, devC ]
> [ pxb1] <---> vSMMU1/pSMMU1 [ devD, devE, devF ]
> How would a user know that devA/devB should be attached to pxb0
> without doing like devA->pxb0 and devB->pxb1? Should QEMU just
> error out until the user associate them correctly? Or they may
> rely on libvirt to figure that out, i.e. moving the iommu node
> matching from QEMU to libvirt?
IIUC having devices associated to the "correct" SMMU is only a
requirement for optimal performance, not pure functionality, right?
In other words, getting the association wrong will make things slower
but they will keep working.
Is a 1:1 match between pSMMU and vSMMU mandatory? On a system with 4
SMMUs, could I create a VMs with
vSMMU0 <-> pSMMU0
vSMMU1 <-> pSMMU1
and another one with
vSMMU0 <-> pSMMU2
vSMMU1 <-> pSMMU3
assuming of course that devices are assigned to the correct VM?
How is the association between vSMMU and pSMMU created anyway? Is
that something that the user can control, or is it done automatically
somehow?
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v4 0/3] hw/riscv/virt: pflash improvements
On Mon, Nov 20, 2023 at 08:06:19PM GMT, Sunil V L wrote: > On Mon, Nov 20, 2023 at 02:29:28PM +0000, Andrea Bolognani wrote: > > On Fri, May 26, 2023 at 11:10:12AM +0200, Andrew Jones wrote: > > > > > > So, are edk2 users the only ones who would (temporarily) need to > > > > > > manually turn ACPI off if virt-manager started enabling it by > > > > > > default? > > > > > > > > > > I assume so, but I'm not tracking firmware status. If the firmware > > > > > doesn't extract the ACPI tables from QEMU and present them to the > > > > > guest (afaik only edk2 does that), then the guest kernel falls back > > > > > to DT, which is why it's working for you. > > > > > > > > > > I suppose we should wait until Linux merges the ACPI patches, before > > > > > adding RISC-V to the libvirt capabilities ACPI list. > > > > > > > > That sounds reasonable to me, but note that 1) the libvirt change > > > > might take a while to propagate to distros and 2) someone will have > > > > to remind me to prepare such a patch when the time comes ;) > > > > > > Initial ACPI support will probably be merged for 6.4. So maybe it is > > > time to get the libvirt side of things going. > > > > Randomly remembered about this. Did ACPI support make it into 6.4 > > after all? Is now a good time to change libvirt? > > Hi Andrea, > > Not yet. While basic ACPI changes are merged, the interrupt controller > support is still going on. Looks like it will take few merge windows to > get ACPI fully supported. So, we still need to wait for libvirt change. Hey, I've been working on making RISC-V support a bit smoother across the virtualization stack recently, and I just so happened to remember that this topic was still pending. I've tried manually switching ACPI on for an existing Fedora RISC-V guest running under TCG and booting via UEFI, which promptly made it stop working, so I assume the necessary bits haven't made it into the kernel yet. Is anyone actually tracking that work? We've been waiting for it to land for a fairly long time at this point... Thanks. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH V2 1/1] loongarch: Change the UEFI loading mode to loongarch
On Thu, Feb 22, 2024 at 04:10:20PM +0100, Philippe Mathieu-Daudé wrote: > On 19/2/24 11:34, Xianglai Li wrote: > > The UEFI loading mode in loongarch is very different > > from that in other architectures:loongarch's UEFI code > > is in rom, while other architectures' UEFI code is in flash. > > > > loongarch UEFI can be loaded as follows: > > -machine virt,pflash=pflash0-format > > -bios ./QEMU_EFI.fd > > > > Other architectures load UEFI using the following methods: > > -machine virt,pflash0=pflash0-format,pflash1=pflash1-format > > > > loongarch's UEFI loading method makes qemu and libvirt incompatible > > when using NVRAM, and the cost of loongarch's current loading method > > far outweighs the benefits, so we decided to use the same UEFI loading > > scheme as other architectures. > > This is unfortunate, since LoongArch was a fresh new target added, > we had the possibility to make this right. Are you saying libvirt > didn't accept to add support for the correct HW behavior which is > to simply load a ROM instead of a PNOR flash device? Could you > point me to the libvirt discussion please? libvirt is very good at > supporting a broad range of legacy options, so I'm surprise 'Doing > The Right Thing' is too costly. > > What is really the problem here, is it your use of the the -bios > CLI option? Hi Philippe, the thread is here: https://lists.libvirt.org/archives/list/de...@lists.libvirt.org/thread/7PV3IXWNX3UXQN2BNV5UA5ASVXNVOQIF/ Unfortunately hyperkitty makes it impossible to link to a subthread directly, so you're going to have to scroll around. The relevant part of the discussion happens entirely as reply to the cover letter. You were actually CC'd to that subthread right after my first reply, so you should be able to find the relevant messages locally as well, which is probably going to be more convenient. In short, the discussion is similar to the one we had a while ago about RISC-V, and my argument in favor of this change is largely the same: barring exceptional circumstances, the overall (maintenance, cognitive) cost of straying from the established norm, now spanning three existing architectures, likely outweighs the benefits. -- Andrea Bolognani / Red Hat / Virtualization
Re: Re: [PATCH V2] loongarch: Change the UEFI loading mode to loongarch
On Tue, Feb 06, 2024 at 05:38:35AM -0800, Andrea Bolognani wrote: > On Tue, Feb 06, 2024 at 10:10:02AM +0800, Xianglai Li wrote: > > The UEFI loading mode in loongarch is very different > > from that in other architectures:loongarch's UEFI code > > is in rom, while other architectures' UEFI code is in flash. > > > > loongarch UEFI can be loaded as follows: > > -machine virt,pflash=pflash0-format > > -bios ./QEMU_EFI.fd > > > > Other architectures load UEFI using the following methods: > > -machine virt,pflash0=pflash0-format,pflash1=pflash1-format > > > > loongarch's UEFI loading method makes qemu and libvirt incompatible > > when using NVRAM, and the cost of loongarch's current loading method > > far outweighs the benefits, so we decided to use the same UEFI loading > > scheme as other architectures. > > > > Cc: Andrea Bolognani > > Cc: maob...@loongson.cn > > Cc: Philippe Mathieu-Daudé > > Cc: Song Gao > > Cc: zhaotian...@loongson.cn > > Signed-off-by: Xianglai Li > > --- > > hw/loongarch/acpi-build.c | 29 +-- > > hw/loongarch/virt.c | 101 ++-- > > include/hw/loongarch/virt.h | 10 ++-- > > 3 files changed, 107 insertions(+), 33 deletions(-) > > For future reference, it's usually good practice to keep track of > changes between subsequent versions of the same patchset. > > Can you please confirm that the build of edk2 added with [1] is > intended to work with a version of QEMU that contains these changes? > I'd like to test things out as soon as I get a moment. I've tried it now with libvirt and everything worked just as I expected it to, so Tested-by: Andrea Bolognani Were changes to edk2 necessary to make it boot from pflash instead of rom? If so, have those patches already been posted? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH V2] loongarch: Change the UEFI loading mode to loongarch
On Tue, Feb 06, 2024 at 10:10:02AM +0800, Xianglai Li wrote: > The UEFI loading mode in loongarch is very different > from that in other architectures:loongarch's UEFI code > is in rom, while other architectures' UEFI code is in flash. > > loongarch UEFI can be loaded as follows: > -machine virt,pflash=pflash0-format > -bios ./QEMU_EFI.fd > > Other architectures load UEFI using the following methods: > -machine virt,pflash0=pflash0-format,pflash1=pflash1-format > > loongarch's UEFI loading method makes qemu and libvirt incompatible > when using NVRAM, and the cost of loongarch's current loading method > far outweighs the benefits, so we decided to use the same UEFI loading > scheme as other architectures. > > Cc: Andrea Bolognani > Cc: maob...@loongson.cn > Cc: Philippe Mathieu-Daudé > Cc: Song Gao > Cc: zhaotian...@loongson.cn > Signed-off-by: Xianglai Li > --- > hw/loongarch/acpi-build.c | 29 +-- > hw/loongarch/virt.c | 101 ++-- > include/hw/loongarch/virt.h | 10 ++-- > 3 files changed, 107 insertions(+), 33 deletions(-) For future reference, it's usually good practice to keep track of changes between subsequent versions of the same patchset. Can you please confirm that the build of edk2 added with [1] is intended to work with a version of QEMU that contains these changes? I'd like to test things out as soon as I get a moment. Thanks. [1] https://github.com/lixianglai/LoongarchVirtFirmware/commit/985ce19438d9544968c7e921c6acf2c74fd4713e -- Andrea Bolognani / Red Hat / Virtualization
Re: Re: [libvirt PATCH V2 0/4] add loongarch support for libvirt
filename": "/usr/share/edk2/loongarch64/QEMU_EFI.fd",
> > > "format": "raw"
> > > },
> > > "nvram-template": {
> > > "filename": "/usr/share/edk2/loongarch64/QEMU_VARS.fd",
> > > "format": "raw"
> > > }
> > > },
> > > "targets": [
> > > {
> > > "architecture": "loongarch64",
> > > "machines": [
> > > "virt",
> > > "virt-*"
> > > ]
> > > }
> > > ],
> > > "features": [
> > > "acpi"
> > > ]
> > >
> > >
> > > ---
> > >
> > > Then add the parsing of the new interface types in libvirt and load
> > > QEMU_CODE.fd as -bios and QEMU_VARS.fd as nvram
> > >
> > > when creating the command line, generating commands like the following:
> > >
> > >
> > > ---
> > >
> > > -blockdev
> > > '{"driver":"file","filename":"/usr/share/edk2/loongarch64/QEMU_VARS.fd/","node-name":"libvirt-pflash0-storage","auto-read-only":false,"discard":"unmap"}'
> > > \
> > > -blockdev
> > > '{"node-name":"libvirt-pflash0-format","read-only":false,"driver":"raw","file":"libvirt-pflash0-storage"}'
> > > \
> > > -machine virt,pflash=libvirt-pflash0-format \
> > > -bios /usr/share/edk2/loongarch64///QEMU_EFI.fd \
> > >
> > > ---
> > >
> > > Option 2:
> > >
> > > Solution 2 mainly starts from qemu. Now the rom that bios is loaded into
> > > is
> > > a memory region that cannot be configured with attributes,
> > >
> > > so we imagine abstracting rom as a device, creating it during machine
> > > initialization and setting "pflash0" attribute for it.
> > >
> > > Then create a pflash and set its property to "pflash1", so our startup
> > > command will look like this:
> > >
> > >
> > > ---
> > >
> > > -blockdev
> > > '{"driver":"file","filename":"/usr/share/edk2/loongarch64/QEMU_EFI.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
> > > \
> > > -blockdev
> > > '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}'
> > > \
> > > -blockdev
> > > '{"driver":"file","filename":"/usr/share/edk2/loongarch64/QEMU_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}'
> > > \
> > > -blockdev
> > > '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}'
> > > \
> > > -machine
> > > virt,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format \
> > >
> > >
> > > -------
> > >
> > > This way, without modifying libvirt, QEMU_CODE.fd can be loaded into the
> > > rom,
> > >
> > > but it is still a little strange that it is clearly rom but set a
> > > "pflash0"
> > > attribute, which can be confusing.
> >
> > We recently had a very similar discussion regarding EFI booting on
> > RISC-V.
> >
> > Personally I would prefer to see the approach with two pflash
> > devices, one read-only and one read/write, adopted. This is pretty
> > much the de-facto standard across architectures: x86_64, aarch64 and
> > riscv64 all boot edk2 this way.
> >
> > I understand the desire to simplify things where possible, and I am
> > sympathetic towards it. If we could boot from just rom, without using
> > pflash at all, I would definitely see the appeal. However, as noted
> > earlier, in the case of EFI having some read/write storage space is
> > necessary to expose the full functionality, so going without it is
> > not really an option.
> >
> > With all the above in mind, the cost of loongarch64 doing things
> > differently from other architectures seems like it would outweight
> > the benefits, and I strongly advise against it.
>
> Hi Andrea :
>
> So, just to be clear, you're not suggesting either of the options I
> suggested above,
>
> are you? And still recommend that we use a two-piece pflash solution
> similar to other architectures,
>
> right?
I thought that the second solution that you proposed above was the
same as other architectures, but reading again I think what you're
suggesting is that you'd have some logic in QEMU that ensures what
you ask to be loaded in pflash0 (CODE) would be loaded into ROM, and
what you ask to be loaded in pflash1 (VARS) would instead go into the
single pflash device for the machine?
If that's a correct understanding, my vote is emphatically against
that approach. The QEMU interface should be as "do exactly as I said"
as possible, with little to no cleverness to it. This is especially
true for a young architecture such as loongarch64, where you don't
need to be concerned about causing breakages for all the users
accumulated over decades.
> Hi Philippe :
>
> I look forward to your reply and the comments of other members of the qemu
> community very much.
>
> If everyone has no opinions,
>
> I will submit a patch to the community to change the loading mode of qemu
> under loongarch architecture to UEFI with two pieces of pflash.
I think this is the way to go.
Note that changing this will likely requires adaptations in edk2 too.
That was the case when riscv64 made the same change.
--
Andrea Bolognani / Red Hat / Virtualization
Re: Re: [libvirt PATCH V2 0/4] add loongarch support for libvirt
w"
> },
> "nvram-template": {
> "filename": "/usr/share/edk2/loongarch64/QEMU_VARS.fd",
> "format": "raw"
> }
> },
> "targets": [
> {
> "architecture": "loongarch64",
> "machines": [
> "virt",
> "virt-*"
> ]
> }
> ],
> "features": [
> "acpi"
> ]
>
>
> ---
>
> Then add the parsing of the new interface types in libvirt and load
> QEMU_CODE.fd as -bios and QEMU_VARS.fd as nvram
>
> when creating the command line, generating commands like the following:
>
>
> ---
>
> -blockdev
> '{"driver":"file","filename":"/usr/share/edk2/loongarch64/QEMU_VARS.fd/","node-name":"libvirt-pflash0-storage","auto-read-only":false,"discard":"unmap"}'
> \
> -blockdev
> '{"node-name":"libvirt-pflash0-format","read-only":false,"driver":"raw","file":"libvirt-pflash0-storage"}'
> \
> -machine virt,pflash=libvirt-pflash0-format \
> -bios /usr/share/edk2/loongarch64///QEMU_EFI.fd \
>
> ---
>
>Option 2:
>
> Solution 2 mainly starts from qemu. Now the rom that bios is loaded into is
> a memory region that cannot be configured with attributes,
>
> so we imagine abstracting rom as a device, creating it during machine
> initialization and setting "pflash0" attribute for it.
>
> Then create a pflash and set its property to "pflash1", so our startup
> command will look like this:
>
>
> ---
>
> -blockdev
> '{"driver":"file","filename":"/usr/share/edk2/loongarch64/QEMU_EFI.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
> \
> -blockdev
> '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}'
> \
> -blockdev
> '{"driver":"file","filename":"/usr/share/edk2/loongarch64/QEMU_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}'
> \
> -blockdev
> '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}'
> \
> -machine virt,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format \
>
>
> ---
>
> This way, without modifying libvirt, QEMU_CODE.fd can be loaded into the
> rom,
>
> but it is still a little strange that it is clearly rom but set a "pflash0"
> attribute, which can be confusing.
We recently had a very similar discussion regarding EFI booting on
RISC-V.
Personally I would prefer to see the approach with two pflash
devices, one read-only and one read/write, adopted. This is pretty
much the de-facto standard across architectures: x86_64, aarch64 and
riscv64 all boot edk2 this way.
I understand the desire to simplify things where possible, and I am
sympathetic towards it. If we could boot from just rom, without using
pflash at all, I would definitely see the appeal. However, as noted
earlier, in the case of EFI having some read/write storage space is
necessary to expose the full functionality, so going without it is
not really an option.
With all the above in mind, the cost of loongarch64 doing things
differently from other architectures seems like it would outweight
the benefits, and I strongly advise against it.
--
Andrea Bolognani / Red Hat / Virtualization
Re: Re: [PATCH v2] tests/docker: Add sqlite3 module to openSUSE Leap container
On Fri, Jan 19, 2024 at 10:12:37AM +0300, Michael Tokarev wrote: > 17.01.2024 19:42, Fabiano Rosas : > > Avocado needs sqlite3: > > > --- a/tests/docker/dockerfiles/opensuse-leap.docker > > +++ b/tests/docker/dockerfiles/opensuse-leap.docker > > @@ -90,6 +90,7 @@ RUN zypper update -y && \ > > pcre-devel-static \ > > pipewire-devel \ > > pkgconfig \ > > + python311 \ > > python311-base \ > > python311-pip \ > > Isn't python311 already pulls in python311-base? Yes, but lcitool doesn't know that :) The information that is provided to the tool is: * the python3 interpreter is in the python311-base package; * the venv module is also in the python311-base package; * the sqlite3 module is in the python311 package; (via tests/lcitool/mappings.yml), as well as: * QEMU needs the python3 interpreter plus the venv and sqlite3 packages to build; (via tests/lcitool/projects/qemu.yml). Based on that, it concludes that the python311-base and python311 package need to be installed. Notice how the former shows up only once in the Dockerfile, despite being mentioned twice in the list of dependencies for the project, because some basic deduplication is applied. Of course the extra line doesn't matter at all in practice: # zypper install python311 The following 4 NEW packages are going to be installed: libexpat1 libpython3_11-1_0 python311 python311-base 4 new packages to install. Overall download size: 12.8 MiB. Already cached: 0 B. After the operation, additional 50.4 MiB will be used. # zypper install python311 python311-base The following 4 NEW packages are going to be installed: libexpat1 libpython3_11-1_0 python311 python311-base 4 new packages to install. Overall download size: 12.8 MiB. Already cached: 0 B. After the operation, additional 50.4 MiB will be used. This is the main reason why we never really bothered trying to avoid it. Hope this helps! -- Andrea Bolognani / Red Hat / Virtualization
Re: Re: [PATCH] tests/docker: Add sqlite3 module to openSUSE Leap container
On Wed, Jan 17, 2024 at 08:18:27AM +0100, Thomas Huth wrote:
> On 17/01/2024 00.09, Fabiano Rosas wrote:
> > Avocado needs sqlite3:
> >
> >Failed to load plugin from module "avocado.plugins.journal":
> >ImportError("Module 'sqlite3' is not installed.
> >Use: sudo zypper install python311 to install it")
> >
> > Include the appropriate package in the dockerfile.
> >
> > From 'zypper info python311':
> >"This package supplies rich command line features provided by
> >readline, and sqlite3 support for the interpreter core, thus forming
> >a so called "extended" runtime."
Weird choice on Python's part to have sqlite3 support as part of the
standard library IMO, but that's "batteries included" for you :)
> > +++ b/tests/docker/dockerfiles/opensuse-leap.docker
> > @@ -90,6 +90,7 @@ RUN zypper update -y && \
> > pcre-devel-static \
> > pipewire-devel \
> > pkgconfig \
> > + python311 \
> > python311-base \
> > python311-pip \
> > python311-setuptools \
>
> AFAIK tests/docker/dockerfiles/opensuse-leap.docker is auto-generated, so
> this will be lost once somebody else runs lcitool again...
>
> I don't really have a clue, but I guess this has to be fixed in the upstream
> lcitool first ( https://gitlab.com/libvirt/libvirt-ci ), and then we need to
> update our lcitool status in QEMU afterwards. Maybe Daniel can advise for
> the right stteps here...?
It looks like a bunch of mappings are maintained in
tests/lcitool/mappings.yml instead of the main lcitool repository. So
I think you need to apply the diff below, then run
$ git submodule update --init tests/lcitool/libvirt-ci
$ tests/lcitool/refresh
to propagate the changes to the generated files.
diff --git a/tests/lcitool/mappings.yml b/tests/lcitool/mappings.yml
index 0b908882f1..407c03301b 100644
--- a/tests/lcitool/mappings.yml
+++ b/tests/lcitool/mappings.yml
@@ -59,6 +59,10 @@ mappings:
CentOSStream8:
OpenSUSELeap15:
+ python3-sqlite3:
+CentOSStream8: python38
+OpenSUSELeap15: python311
+
python3-tomli:
# test using tomllib
apk:
diff --git a/tests/lcitool/projects/qemu.yml b/tests/lcitool/projects/qemu.yml
index 82092c9f17..149b15de57 100644
--- a/tests/lcitool/projects/qemu.yml
+++ b/tests/lcitool/projects/qemu.yml
@@ -97,6 +97,7 @@ packages:
- python3-pip
- python3-sphinx
- python3-sphinx-rtd-theme
+ - python3-sqlite3
- python3-tomli
- python3-venv
- rpm2cpio
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v4 0/3] hw/riscv/virt: pflash improvements
On Fri, May 26, 2023 at 11:10:12AM +0200, Andrew Jones wrote: > On Fri, May 26, 2023 at 04:42:57AM -0400, Andrea Bolognani wrote: > > On Fri, May 26, 2023 at 10:34:36AM +0200, Andrew Jones wrote: > > > On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote: > > > > So, are edk2 users the only ones who would (temporarily) need to > > > > manually turn ACPI off if virt-manager started enabling it by > > > > default? > > > > > > I assume so, but I'm not tracking firmware status. If the firmware > > > doesn't extract the ACPI tables from QEMU and present them to the > > > guest (afaik only edk2 does that), then the guest kernel falls back > > > to DT, which is why it's working for you. > > > > > > I suppose we should wait until Linux merges the ACPI patches, before > > > adding RISC-V to the libvirt capabilities ACPI list. > > > > That sounds reasonable to me, but note that 1) the libvirt change > > might take a while to propagate to distros and 2) someone will have > > to remind me to prepare such a patch when the time comes ;) > > Initial ACPI support will probably be merged for 6.4. So maybe it is > time to get the libvirt side of things going. Randomly remembered about this. Did ACPI support make it into 6.4 after all? Is now a good time to change libvirt? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
On Thu, Nov 09, 2023 at 08:01:48PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:34:56AM -0800, Andrea Bolognani wrote:
> > We should call this Null instead of IsNull, and also make it a
> > pointer similarly to what I just suggested for union branches
> > that don't have additional data attached to them.
>
> I don't have a strong argument here against what you suggest, I
> just think that the usage of bool is simpler.
>
> The test I have here [0] would have code changing to something
> like:
>
> When is null:
>
> - val := {IsNull: true}
> + myNull := JSONNull{}
> + val := {Null: }
>
> So you need a instance to get a pointer.
>
> When is absent (no fields set), no changes as both bool defaults
> to false and pointer to nil.
>
> The code handling this would change from:
>
> - if u.IsNull {
> + if u.Null != nil {
> ...
> }
>
> We don't have the same issues as Union, under the hood we Marshal
> to/Unmarshal from "null" and that would not change.
>
> [0]
> https://gitlab.com/victortoso/qapi-go/-/blob/main/test/type_or_null_test.go
>
> I can change this in the next iteration.
No, leave the type alone. But I still think the name should probably
be Null instead of IsNull.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
On Thu, Nov 09, 2023 at 07:35:04PM +0100, Victor Toso wrote: > On Thu, Nov 09, 2023 at 09:29:28AM -0800, Andrea Bolognani wrote: > > Additionally, this would allow client code that *looks* at the > > union to keep working even if actual data is later added to the > > branch; client code that *creates* the union would need to be > > updated, of course, but that would be the case regardless. > > I think it is better to not have code that is working to keep > working in this case where Spice is implemented. > > Implementing Spice here would mean that a struct type > SetPasswordOptionsSpice was created but because the code handling > it before was using struct type Empty, it will not handle the new > struct, leading to possible runtime errors (e.g: not handling > username/password) > > A bool would be simpler, triggering compile time errors. You've convinced me :) Let's leave it like this for now. Once we start seriously thinking about compatibility across versions then we might have to reconsider this, but it's going to be part of a much bigger, much more fun conversation ;) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
On Thu, Nov 09, 2023 at 08:13:38PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 09:59:50AM -0800, Andrea Bolognani wrote:
> > Now, I'm not sure I would go as far as suggesting that the
> > GetName() function should be completely removed, but maybe we
> > can try leaving it out from the initial version and see if
> > people start screaming?
>
> It might be useful for debugging too. I would rather log
> e.GetName() than the string version of the type but if that's the
> only reason we needed, I agree on removing for now.
I think the upside is too small considering the potential for abuse.
> > API-wise, I'm not a fan of the fact that we're forcing users to call
> > (Un)MarshalEvent instead of the standard (Un)MarshalJSON. If we add
> > something like
> >
> > func GetEventType(data []byte) (Event, error) {
> >
> > it becomes feasible to stick with standard functions. We can of
> > course keep the (Un)MarshalEvent functions around for convenience,
> > but I don't think they should be the only available API.
>
> I agree. I'll change it. Perhaps we shouldn't use
> (Un)MarshalEvent at this layer at all. Probably the same for
> (Un)MarshalCommand.
Yeah, what I wrote for events applies 1:1 to commands as well.
Up to you whether or not you want to keep around the convenience
functions. It might indeed be fine to drop them for now and consider
reintroducing them later if it turns out that it really helps making
client code less clunky.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
On Thu, Nov 09, 2023 at 08:31:01PM +0100, Victor Toso wrote:
> On Thu, Nov 09, 2023 at 10:24:20AM -0800, Andrea Bolognani wrote:
> > On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote:
> > > Example:
> > > qapi:
> > > | { 'command': 'query-sev', 'returns': 'SevInfo',
> > > | 'if': 'TARGET_I386' }
> > >
> > > go:
> > > | type QuerySevCommandReturn struct {
> > > | MessageId string `json:"id,omitempty"`
> > > | Result*SevInfo `json:"return"`
> > > | Error *QapiError `json:"error,omitempty"`
> > > | }
> > >
> > > usage:
> > > | // One can use QuerySevCommandReturn directly or
> > > | // command's interface GetReturnType() instead.
> >
> > I'm not convinced this function is particularly useful. I know
> > that I've suggested something similar for events, but the usage
> > scenarios are different.
>
> I think that I wanted to expose knowledge we had in the parser,
> not necessarily useful or needed indeed. At the very least, I
> agree that at this layer, we just want Command and ComandReturn
> types to be generated and properly (un)mashalled.
>
> One downside is for testing.
>
> If we have a list of commands, I can just iterate over them
> Unmarshal to a command interface variable, fetch the return type
> and do some comparisons to see if all is what we expected. See:
>
> https://gitlab.com/victortoso/qapi-go/-/blob/main/test/examples_test.go#L61
>
> Not saying we should keep it for tests, but it is useful :)
That code is quite dense and I'm not going to dig into it now :)
Anyway, I don't have a problem with keeping this type-safe
introspection feature around. Maybe call it GetCommandReturnType
though.
> > This produces
> >
> > type QueryAudiodevsCommandReturn struct {
> > MessageId string `json:"id,omitempty"`
> > Error *QAPIError `json:"error,omitempty"`
> > Result[]Audiodev `json:"return"`
> > }
> >
> > when the return type is an array. Is that the correct behavior? I
> > haven't thought too hard about it, but it seems odd so I though I'd
> > bring it up.
>
> Hm, the schema for it is
>
> ##
> # @query-audiodevs:
> #
> # Returns information about audiodev configuration
> #
> # Returns: array of @Audiodev
> #
> # Since: 8.0
> ##
> { 'command': 'query-audiodevs',
> 'returns': ['Audiodev'] }
>
> So, I think it is correct. Would you expect it to be an object
> wrapping the array or I'm missing what you find odd.
It works as expected if there is a result, but in the case of error:
in := `{ "error": {"class": "errorClass", "desc": "errorDesc" }}`
out := qapi.QueryAudiodevsCommandReturn{}
err := json.Unmarshal([]byte(in), )
if err != nil {
panic(err)
}
fmt.Printf("result=%v error=%v\n", out.Result, out.Error)
the output will be
result=[] error=errorDesc
Note how result is an empty array instead of a nil pointer. If we
unmarshal the same JSON into QueryReplayCommandReturn instead, the
output becomes
result= error=errorDesc
The latter behavior seems more correct to me, based on how we deal
with unions and alternates.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
On Mon, Oct 16, 2023 at 05:26:53PM +0200, Victor Toso wrote: > This patch series intent is to introduce a generator that produces a Go > module for Go applications to interact over QMP with QEMU. > > This is the second iteration: > v1: https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg06734.html > > I've pushed this series in my gitlab fork: > https://gitlab.com/victortoso/qemu/-/tree/qapi-golang-v2 > > I've also generated the qapi-go module over QEMU tags: v7.0.0, v7.1.0, > v7.2.6, v8.0.0 and v8.1.0, see the commits history here: > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-tags > > I've also generated the qapi-go module over each commit of this series, > see the commits history here (using previous refered qapi-golang-v2) > https://gitlab.com/victortoso/qapi-go/-/commits/qapi-golang-v2-by-patch I've provided feedback on the various facets of the API in response to the corresponding patch. Note that I've only addressed concerns about the consumer-facing API: I have some notes about the implementation as well, but that's something that we should be able to easily change transparently so I didn't give it priority. Overall, I think that the current API is quite close to being a solid PoC that can be used as a starting point for further development. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 10/11] qapi: golang: Add CommandResult type to Go
On Mon, Oct 16, 2023 at 05:27:03PM +0200, Victor Toso wrote:
> This patch adds a struct type in Go that will handle return values
> for QAPI's command types.
>
> The return value of a Command is, encouraged to be, QAPI's complex
> types or an Array of those.
>
> Every Command has a underlying CommandResult. The EmptyCommandReturn
> is for those that don't expect any data e.g: `{ "return": {} }`.
>
> All CommandReturn types implement the CommandResult interface.
I guess EmptyCommandReturn is something that you have scrapped in
between revisions, because I see no reference to it in the current
incarnation of the code. Same thing with CommandResult, unless that's
just a typo for CommandReturn?
> Example:
> qapi:
> | { 'command': 'query-sev', 'returns': 'SevInfo',
> | 'if': 'TARGET_I386' }
>
> go:
> | type QuerySevCommandReturn struct {
> | MessageId string `json:"id,omitempty"`
> | Result*SevInfo `json:"return"`
> | Error *QapiError `json:"error,omitempty"`
> | }
>
> usage:
> | // One can use QuerySevCommandReturn directly or
> | // command's interface GetReturnType() instead.
I'm not convinced this function is particularly useful. I know that
I've suggested something similar for events, but the usage scenarios
are different.
For events, you're going to have some loop listening for them and you
can't know in advance what event you're going to receive at any given
time.
In contrast, a return value will be received as a direct consequence
of running a command, and since the caller knows what command it
called it's fair to assume that it also knows its return type.
> +if ret_type:
> +marshal_empty = ""
> +ret_type_name = qapi_schema_type_to_go_type(ret_type.name)
> +isptr = "*" if ret_type_name[0] not in "*[" else ""
> +retargs.append(
> +{
> +"name": "Result",
> +"type": f"{isptr}{ret_type_name}",
> +"tag": """`json:"return"`""",
> +}
> +)
This produces
type QueryAudiodevsCommandReturn struct {
MessageId string `json:"id,omitempty"`
Error *QAPIError `json:"error,omitempty"`
Result[]Audiodev `json:"return"`
}
when the return type is an array. Is that the correct behavior? I
haven't thought too hard about it, but it seems odd so I though I'd
bring it up.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 08/11] qapi: golang: Generate qapi's event types in Go
On Mon, Oct 16, 2023 at 05:27:01PM +0200, Victor Toso wrote:
> This patch handles QAPI event types and generates data structures in
> Go that handles it.
>
> We also define a Event interface and two helper functions MarshalEvent
> and UnmarshalEvent.
>
> Example:
> qapi:
> | { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> | 'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
>
> go:
> | type MemoryDeviceSizeChangeEvent struct {
> | MessageTimestamp Timestamp `json:"-"`
> | Id *string `json:"id,omitempty"`
> | Size uint64`json:"size"`
> | QomPath string`json:"qom-path"`
> | }
>
> usage:
> | input := `{"event":"MEMORY_DEVICE_SIZE_CHANGE",` +
> | `"timestamp":{"seconds":1588168529,"microseconds":201316},` +
> |
> `"data":{"id":"vm0","size":1073741824,"qom-path":"/machine/unattached/device[2]"}}`
> | e, err := UnmarshalEvent([]byte(input)
> | if err != nil {
> | panic(err)
> | }
> | if e.GetName() == `MEMORY_DEVICE_SIZE_CHANGE` {
> | m := e.(*MemoryDeviceSizeChangeEvent)
> | // m.QomPath == "/machine/unattached/device[2]"
> | }
I don't think we should encourage people to perform string
comparisons, as it completely sidesteps Go's type system and is thus
error-prone. Safer version:
switch m := e.(type) {
case *MemoryDeviceSizeChangeEvent:
// m.QomPath == "/machine/unattached/device[2]"
}
Now, I'm not sure I would go as far as suggesting that the GetName()
function should be completely removed, but maybe we can try leaving
it out from the initial version and see if people start screaming?
API-wise, I'm not a fan of the fact that we're forcing users to call
(Un)MarshalEvent instead of the standard (Un)MarshalJSON. If we add
something like
func GetEventType(data []byte) (Event, error) {
type event struct {
Name string `json:"event"`
}
tmp := event{}
if err := json.Unmarshal(data, ); err != nil {
return nil, err
}
switch tmp.Name {
case "MEMORY_DEVICE_SIZE_CHANGE":
return {}, nil
...
}
return nil, fmt.Errorf("unrecognized event '%s'", tmp.Name)
}
it becomes feasible to stick with standard functions. We can of
course keep the (Un)MarshalEvent functions around for convenience,
but I don't think they should be the only available API.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> [*] The exception for optional fields as default is to Types that can
> accept JSON Null as a value. For this case, we translate NULL to a
> member type called IsNull, which is boolean in Go. This will be
> explained better in the documentation patch of this series but the
> main rationale is around Marshaling to and from JSON and Go data
> structures.
>
> Example:
>
> qapi:
> | { 'alternate': 'StrOrNull',
> | 'data': { 's': 'str',
> | 'n': 'null' } }
>
> go:
> | type StrOrNull struct {
> | S *string
> | IsNull bool
> | }
We should call this Null instead of IsNull, and also make it a
pointer similarly to what I just suggested for union branches that
don't have additional data attached to them.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 07/11] qapi: golang: Generate qapi's union types in Go
On Mon, Oct 16, 2023 at 05:27:00PM +0200, Victor Toso wrote:
> This patch handles QAPI union types and generates the equivalent data
> structures and methods in Go to handle it.
>
> The QAPI union type has two types of fields: The @base and the
> @Variants members. The @base fields can be considered common members
> for the union while only one field maximum is set for the @Variants.
>
> In the QAPI specification, it defines a @discriminator field, which is
> an Enum type. The purpose of the @discriminator is to identify which
> @variant type is being used.
>
> For the @discriminator's enum that are not handled by the QAPI Union,
> we add in the Go struct a separate block as "Unbranched enum fields".
> The rationale for this extra block is to allow the user to pass that
> enum value under the discriminator, without extra payload.
>
> The union types implement the Marshaler and Unmarshaler interfaces to
> seamless decode from JSON objects to Golang structs and vice versa.
>
> qapi:
> | { 'union': 'SetPasswordOptions',
> | 'base': { 'protocol': 'DisplayProtocol',
> | 'password': 'str',
> | '*connected': 'SetPasswordAction' },
> | 'discriminator': 'protocol',
> | 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
>
> go:
> | type SetPasswordOptions struct {
> |Password string `json:"password"`
> |Connected *SetPasswordAction `json:"connected,omitempty"`
> |// Variants fields
> |Vnc *SetPasswordOptionsVnc `json:"-"`
> |// Unbranched enum fields
> |Spice bool `json:"-"`
> | }
Instead of using bool for these, can we denote a special type? For
example
type Empty struct{}
We could then do
u := SetPasswordOptions{
Password: "...",
Spice: {},
}
The benefit I have in mind is that you'd be able to check which
variant field is set consistently:
if u.Vnc != nil {
...
}
if u.Spice != nil {
...
}
Additionally, this would allow client code that *looks* at the union
to keep working even if actual data is later added to the branch;
client code that *creates* the union would need to be updated, of
course, but that would be the case regardless.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
On Mon, Nov 06, 2023 at 04:52:12PM +0100, Victor Toso wrote: > On Mon, Nov 06, 2023 at 07:28:04AM -0800, Andrea Bolognani wrote: > > On a partially related note: while I haven't yet looked closely at > > how much effort you've dedicated to producing pretty output, from a > > quick look at generate_struct_type() it seems that the answer is "not > > zero". I think it would be fine to simplify things there and produce > > ugly output, under the assumption that gofmt will be called on the > > generated code immediately afterwards. The C generator doesn't have > > this luxury, but we should take advantage of it. > > Yes, I wholeheartedly agree. The idea of the generator producing > a well formatted Go code came from previous review. I didn't want > to introduce gofmt and friends to QEMU build system, perhaps it > wasn't a big deal but I find it foreign to QEMU for a generated > code that QEMU itself would not use. > > See: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01188.html Noted. Whether or not requiring a pass through gofmt is an issue kind of depends on how we end up shipping these files. What seems the most probable to me is that we'll have a separate repo where we dump the generated files and that Go users will consume via a regular 'import'. Your existing qapi-go repo follows this model. In this context, gofmt is never going to be called as part of the QEMU build process so it doesn't really matter. But maybe there was a discussion around this that I've missed :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 04/11] qapi: golang: Generate qapi's alternate types in Go
On Mon, Oct 16, 2023 at 05:26:57PM +0200, Victor Toso wrote:
> This patch handles QAPI alternate types and generates data structures
> in Go that handles it.
>
> Alternate types are similar to Union but without a discriminator that
> can be used to identify the underlying value on the wire. It is needed
> to infer it. In Go, most of the types [*] are mapped as optional
> fields and Marshal and Unmarshal methods will be handling the data
> checks.
>
> Example:
>
> qapi:
> | { 'alternate': 'BlockdevRef',
> | 'data': { 'definition': 'BlockdevOptions',
> | 'reference': 'str' } }
>
> go:
> | type BlockdevRef struct {
> | Definition *BlockdevOptions
> | Reference *string
> | }
>
> usage:
> | input := `{"driver":"qcow2","data-file":"/some/place/my-image"}`
> | k := BlockdevRef{}
> | err := json.Unmarshal([]byte(input), )
> | if err != nil {
> | panic(err)
> | }
> | // *k.Definition.Qcow2.DataFile.Reference == "/some/place/my-image"
>
> [*] The exception for optional fields as default is to Types that can
> accept JSON Null as a value. For this case, we translate NULL to a
> member type called IsNull, which is boolean in Go. This will be
> explained better in the documentation patch of this series but the
> main rationale is around Marshaling to and from JSON and Go data
> structures.
These usage examples are great; in fact, I think they're too good to
be relegated to the commit messages. I would like to see them in the
actual documentation.
At the same time, the current documentation seems to focus a lot on
internals rather than usage. I think we really need two documents
here:
* one for users of the library, with lots of usage examples
(ideally at least one for JSON->Go and one for Go->JSON for each
class of QAPI object) and little-to-no peeking behind the
curtains;
* one for QEMU developers / QAPI maintainers, which goes into
detail regarding the internals and explains the various design
choices and trade-offs.
Some parts of the latter should probably be code comments instead. A
reasonable balance will have to be found.
> diff --git a/scripts/qapi/golang.py b/scripts/qapi/golang.py
> +TEMPLATE_HELPER = """
> +// Creates a decoder that errors on unknown Fields
> +// Returns nil if successfully decoded @from payload to @into type
> +// Returns error if failed to decode @from payload to @into type
> +func StrictDecode(into interface{}, from []byte) error {
> +\tdec := json.NewDecoder(strings.NewReader(string(from)))
> +\tdec.DisallowUnknownFields()
> +
> +\tif err := dec.Decode(into); err != nil {
> +\t\treturn err
> +\t}
> +\treturn nil
> +}
> +"""
I think the use of \t here makes things a lot less readable. Can't
you just do
TEMPLATE_HELPER = """
func StrictDecode() {
dec := ...
if err := ... {
return err
}
return nil
}
"""
I would actually recommend the use of textwrap.dedent() to make
things less awkward:
TEMPLATE_HELPER = textwrap.dedent("""
func StrictDecode() {
dec := ...
if err := ... {
return err
}
return nil
}
"""
This is particularly useful for blocks of Go code that are not
declared at the top level...
> +unmarshal_check_fields = f"""
> +\t// Check for json-null first
> +\tif string(data) == "null" {{
> +\t\treturn errors.New(`null not supported for {name}`)
> +\t}}"""
... such as this one, which could be turned into:
unmarshal_check_fields = textwrap.dedent(f"""
// Check for json-null first
if string(data) == "null" {{
return errors.New(`null not supported for {name}`)
}}
""")
Much more manageable, don't you think? :)
On a partially related note: while I haven't yet looked closely at
how much effort you've dedicated to producing pretty output, from a
quick look at generate_struct_type() it seems that the answer is "not
zero". I think it would be fine to simplify things there and produce
ugly output, under the assumption that gofmt will be called on the
generated code immediately afterwards. The C generator doesn't have
this luxury, but we should take advantage of it.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
On Tue, Oct 31, 2023 at 09:42:10AM -0700, Andrea Bolognani wrote:
> On Fri, Oct 27, 2023 at 07:33:30PM +0200, Victor Toso wrote:
> > Hi,
> >
> > Daniel & Andrea, it would be great to have your take on the Go
> > side of this series. If we can agree with an acceptable
> > 'unstable' version of Go modules, we can start building on top of
> > this:
> > - libraries/tools in Go to interact with QEMU
> > - qapi specs to fix limitations (e.g: Data type names)
> > - scripts/qapi library wrt to generating interfaces in other
> >languages other than C
> >
> > I would love to have this prior to 8.2 feature freeze if the
> > idea and current code meet your expectations.
>
> Apologies for not providing any feedback so far. I'll do my best to
> get around to it by the end of the week.
Layering apologies on top of apologies: I started looking into this,
but I have since realized that I need some more time to page all the
months-old context back in and digest the whole thing. I'll continue
next week.
As an appetizer, one thing that I've noticed: you seem to ignore it
when gen:false is included in a command definition, so we get
type DeviceAddCommand struct {
MessageId string `json:"-"`
Driverstring `json:"driver"`
Bus *string `json:"bus,omitempty"`
Id*string `json:"id,omitempty"`
}
which I don't think will work as it can't handle even the example
used to document the command
{ "execute": "device_add",
"arguments": { "driver": "e1000", "id": "net1",
"bus": "pci.0",
"mac": "52:54:00:12:34:56" } }
This command will probably require an ad-hoc implementation.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 00/11] qapi-go: add generator for Golang interface
On Fri, Oct 27, 2023 at 07:33:30PM +0200, Victor Toso wrote: > Hi, > > Daniel & Andrea, it would be great to have your take on the Go > side of this series. If we can agree with an acceptable > 'unstable' version of Go modules, we can start building on top of > this: > - libraries/tools in Go to interact with QEMU > - qapi specs to fix limitations (e.g: Data type names) > - scripts/qapi library wrt to generating interfaces in other >languages other than C > > I would love to have this prior to 8.2 feature freeze if the > idea and current code meet your expectations. Apologies for not providing any feedback so far. I'll do my best to get around to it by the end of the week. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v6 09/23] ui/console: allow to override the default VC
On Wed, Oct 25, 2023 at 11:08:03PM +0400, marcandre.lur...@redhat.com wrote: > If a display is backed by a specialized VC, allow to override the > default "vc:80Cx24C". For that, set the dpy.type just before creating > the default serial/parallel/monitor. > > As suggested by Paolo, if the display doesn't implement a VC (get_vc() > returns NULL), use a fallback that will use a muxed console on stdio. > > This changes the behaviour of "qemu -display none", to create a muxed > serial/monitor by default. Have you verified that this doesn't break libvirt? We use '-display none' for every single VM we run. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support
On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote: > On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote: > > On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote: > > > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote: > > > > On 10/23/23 05:16, Andrew Jones wrote: > > > > > Hmm, I'm not sure I agree with special-casing profiles like this. I > > > > > think > > > > > the left-to-right processing should be consistent for all. I'm also > > > > > not > > > > > sure we should always warn when disabling a profile. For example, if a > > > > > user does > > > > > > > > > > -cpu rv64,rva22u64=true,rva22u64=false > > > > > > > > > > then they'll get a warning, even though all they're doing is > > > > > restoring the > > > > > cpu model. While that looks like an odd thing to do, a script may be > > > > > adding the rva22u64=true and the rva22u64=false is the user input > > > > > which > > > > > undoes what the script did. > > > > > > > > QEMU options do not work with a "the user enabled then disabled the > > > > same option, > > > > thus it'll count as nothing happened" logic. The last instance of the > > > > option will > > > > overwrite all previous instances. In the example you mentioned above > > > > the user would > > > > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter > > > > if the > > > > same profile was enabled beforehand. > > > > > > Yup, I'm aware, but I keep thinking that we'll only be using profiles with > > > a base cpu type. If you start with nothing (a base) and then add a profile > > > and take the same one away, you shouldn't be taking away anything else. I > > > agree that if you use a profile on some cpu type that already enabled a > > > bunch of stuff itself, then disabling a profile would potentially remove > > > some of those too, but mixing cpu types that have their own extensions and > > > profiles seems like a great way to confuse oneself as to what extensions > > > will be present. IOW, we should be adding a base cpu type at the same > > > time we're adding these profiles. > > > > The question that keep bouncing around my head is: why would we even > > allow disabling profiles? > > > > It seems to me that it only makes things more complicated, and I > > really can't see the use case for it. > > > > Enabling additional features on top of a profile? There's obvious > > value in that, so that you can model hardware that implements > > optional and proprietary extensions. Enabling multiple profiles? > > You've convinced me that it's useful. But disabling profiles, I just > > don't see it. I believe Alistair was similarly unconvinced. > > The only value I see in allowing a profile to be disabled is to undo the > enabling of the profile by specifying the profile as 'off' to the right of > it being specified as 'on'. That may seem pointless, but scripts take > advantage of being able to do that. Besides that one possible use case, > there isn't much use in disabling profiles, but treating profile > properties like every other boolean property makes the UI consistent and > should actually simplify the code. The code might be simpler, but the result is an additional burden on the user, as the interactions between the various flags become much more nuanced and less intuitive. I'm not convinced the trade-off is a worthwhile one. For the script override scenario, fair enough, but once again I feel that we're making things much worse in the general case in order to cater to a much narrower one. Script authors will naturally learn to avoid hardcoding profile enablement once users have reported enough failures resulting from that. > > > > > As far as warnings go, it'd be nice to warn when mandatory profile > > > > > extensions are disabled from an enabled profile. Doing that might be > > > > > useful for debug, but users which do it without being aware they're > > > > > "breaking" the profile may learn from that warning. Note, the warning > > > > > should only come when the profile is actually enabled and when the > > > > > extension would actually be disabled, i.e. > > > > > > > > > > -cpu rv64,rva22u64=true,c=off > > > > > > > > > > should warn > > > > > > > > >
Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support
On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote: > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote: > > On 10/23/23 05:16, Andrew Jones wrote: > > > Hmm, I'm not sure I agree with special-casing profiles like this. I think > > > the left-to-right processing should be consistent for all. I'm also not > > > sure we should always warn when disabling a profile. For example, if a > > > user does > > > > > > -cpu rv64,rva22u64=true,rva22u64=false > > > > > > then they'll get a warning, even though all they're doing is restoring the > > > cpu model. While that looks like an odd thing to do, a script may be > > > adding the rva22u64=true and the rva22u64=false is the user input which > > > undoes what the script did. > > > > QEMU options do not work with a "the user enabled then disabled the same > > option, > > thus it'll count as nothing happened" logic. The last instance of the > > option will > > overwrite all previous instances. In the example you mentioned above the > > user would > > disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if > > the > > same profile was enabled beforehand. > > Yup, I'm aware, but I keep thinking that we'll only be using profiles with > a base cpu type. If you start with nothing (a base) and then add a profile > and take the same one away, you shouldn't be taking away anything else. I > agree that if you use a profile on some cpu type that already enabled a > bunch of stuff itself, then disabling a profile would potentially remove > some of those too, but mixing cpu types that have their own extensions and > profiles seems like a great way to confuse oneself as to what extensions > will be present. IOW, we should be adding a base cpu type at the same > time we're adding these profiles. The question that keep bouncing around my head is: why would we even allow disabling profiles? It seems to me that it only makes things more complicated, and I really can't see the use case for it. Enabling additional features on top of a profile? There's obvious value in that, so that you can model hardware that implements optional and proprietary extensions. Enabling multiple profiles? You've convinced me that it's useful. But disabling profiles, I just don't see it. I believe Alistair was similarly unconvinced. > > > As far as warnings go, it'd be nice to warn when mandatory profile > > > extensions are disabled from an enabled profile. Doing that might be > > > useful for debug, but users which do it without being aware they're > > > "breaking" the profile may learn from that warning. Note, the warning > > > should only come when the profile is actually enabled and when the > > > extension would actually be disabled, i.e. > > > > > > -cpu rv64,rva22u64=true,c=off > > > > > > should warn > > > > > > -cpu rv64,c=off,rva22u64=true > > > > > > should not warn (rva22u64 overrides c=off since it's to the right) > > > > > > -cpu rv64,rva22u64=true,rva22u64=false,c=off > > > > > > should not warn (rva22u64 is not enabled) I think these should be hard errors, not warnings. If you're enabling a profile and then disabling an extension that's mandatory for that profile, you've invalidated the profile. You've asked for a configuration that doesn't make any sense: you can't have a CPU that both implements a profile and lacks one of its mandatory extensions. QEMU users could easily miss the warning. libvirt users won't see it at all. It's a user error and it needs to be treated as such IMO. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 0/6] riscv: RVA22U64 profile support
On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> Based-on: 20230926183109.165878-1-dbarb...@ventanamicro.com
> ("[PATCH 0/2] riscv: add extension properties for all cpus")
>
> Hi,
>
> These patches implements the base profile support for qemu-riscv and the
> first profile, RVA22U64.
>
> As discussed in this thread [1] we're aiming for a flag that enables all
> mandatory extensions of a profile. Optional extensions were left behind
> and must be enabled by hand if desired. Since this is the first profile
> we're adding, we'll need to add the base framework as well.
>
> The RVA22U64 profile was chosen because qemu-riscv implements all its
> extensions, both mandatory and optional. That includes 'zicntr' and
> 'zihpm', which we support for awhile but aren't adverting to userspace.
>
> Other design decisions made:
>
> - disabling a profile flag does nothing, i.e. we won't mass disable
> mandatory extensions of the rva22U64 profile if the user sets
> rva22u64=false;
Wouldn't it make more sense to error out when this is requested?
Silently ignoring an explicit request made by the user is pretty much
never a good idea in my experience.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v6 3/3] docs/system: riscv: Add pflash usage details
On Wed, May 31, 2023 at 07:53:00PM +0530, Sunil V L wrote: > +Using flash devices > +--- > + > +When KVM is not enabled, the first flash device (pflash0) can contain either > +the ROM code or S-mode payload firmware code. If the pflash0 contains the > +ROM code, -bios should be set to none. If -bios is not set to > +none, pflash0 is assumed to contain S-mode payload code. > + > +When KVM is enabled, pflash0 is always assumed to contain the S-mode payload > +firmware. > + > +Firmware images used for pflash should be of size 32 MiB. > + > +To boot as ROM code: > + > +.. code-block:: bash > + > + $ qemu-system-riscv64 -bios none \ > + -blockdev > node-name=pflash0,driver=file,read-only=on,filename= \ > + -M virt,pflash0=pflash0 \ > + ... other args > + > +To boot as read-only S-mode payload: > + > +.. code-block:: bash > + > + $ qemu-system-riscv64 \ > + -blockdev > node-name=pflash0,driver=file,read-only=on,filename= \ > + -blockdev node-name=pflash1,driver=file,filename= \ > + -M virt,pflash0=pflash0,pflash1=pflash1 \ > + ... other args > + > +To boot as read-only S-mode payload in KVM guest: > + > +.. code-block:: bash > + > + $ qemu-system-riscv64 \ > + -blockdev > node-name=pflash0,driver=file,read-only=on,filename= \ > + -blockdev node-name=pflash1,driver=file,filename= \ > + -M virt,pflash0=pflash0,pflash1=pflash1 \ > + --enable-kvm \ > + ... other args I feel that this, while accurate, has gotten more complicated than it needs to be. We're also putting the least common scenario front and center instead of opening with the one that most people are going to be using. Below is how I suggest reworking it. What do you think? Using flash devices --- By default, the first flash device (pflash0) is expected to contain S-mode firmware code. It can be configured as read-only, with the second flash device (pflash1) available to store configuration data. For example, booting edk2 looks like ..code-block:: bash $ qemu-system-riscv64 \ -blockdev node-name=pflash0,driver=file,read-only=on,filename= \ -blockdev node-name=pflash1,driver=file,filename= \ -M virt,pflash0=pflash0,pflash1=pflash1 \ ... other args For TCG guests only, it is also possible to boot M-mode firmware from the first flash device (pflash0) by additionally passing ``-bios none``, as in ..code-block:: bash $ qemu-system-riscv64 \ -bios none \ -blockdev node-name=pflash0,driver=file,read-only=on,filename= \ -M virt,pflash0=pflash0 \ ... other args Firmware images used for pflash must be exactly 32 MiB in size. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v5 0/3] hw/riscv/virt: pflash improvements
On Wed, May 31, 2023 at 10:46:17AM +0530, Anup Patel wrote: > On Fri, May 26, 2023 at 5:41 PM Sunil V L wrote: > > hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none" > > riscv/virt: Support using pflash via -blockdev option > > docs/system: riscv: Add pflash usage details > > In case of KVM guests, there is no M-mode so pflash0 will always > contain S-mode FW. > > I suggest improving this series to consider KVM guests as well > such that the same EDK2 S-mode works for KVM and TCG guests. After these patches are applied, pflash0 is assumed to contain S-mode code *unless* you go out of your way and add -bios none to the command line. It seems to me that this default behavior will work fine for KVM guests too, based on what you wrote above. Isn't that the case? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v7 0/1] arm: enable MTE for QEMU + kvm
On Mon, May 22, 2023 at 02:04:28PM +0200, Cornelia Huck wrote: > On Fri, Apr 28 2023, Cornelia Huck wrote: > > Another open problem is mte vs mte3: tcg emulates mte3, kvm gives the guest > > whatever the host supports. Without migration support, this is not too much > > of a problem yet, but for compatibility handling, we'll need a way to keep > > QEMU from handing out mte3 for guests that might be migrated to a mte3-less > > host. We could tack this unto the mte property (specifying the version or > > max supported), or we could handle this via cpu properties if we go with > > handling compatibility via cpu models (sorting this out for kvm is probably > > going to be interesting in general.) In any case, I think we'll need a way > > to inform kvm of it. > > Before I start to figure out the initialization breakage, I think it > might be worth pointing to this open issue again. As Andrea mentioned in > https://listman.redhat.com/archives/libvir-list/2023-May/239926.html, > libvirt wants to provide a stable guest ABI, not only in the context of > migration compatibility (which we can handwave away via the migration > blocker.) Yeah, in order to guarantee a stable guest ABI it's critical that libvirt can ask for a *specific* version of MTE (MTE or MTE3) and either get exactly that version, or an error on QEMU's side. > The part I'm mostly missing right now is how to tell KVM to not present > mte3 to a guest while running on a mte3 capable host (i.e. the KVM > interface for that; it's more a case of "we don't have it right now", > though.) I'd expect it to be on the cpu level, rather than on the vm > level, but it's not there yet; we also probably want something that's > not fighting whatever tcg (or other accels) end up doing. > > I see several options here: > - Continue to ignore mte3 and its implications for now. The big risk is > that someone might end up implementing support for MTE in libvirt again, > with the same stable guest ABI issues as for this version. > - Add a "version" qualifier to the mte machine prop (probably with > semantics similar to the gic stuff), with the default working with tcg > as it does right now (i.e. defaulting to mte3). KVM would only support > "no mte" or "same as host" (with no stable guest ABI guarantees) for > now. I'm not sure how hairy this might get if we end up with a per-cpu > configuration of mte (and other features) with kvm. > - Add cpu properties for mte and mte3. I think we've been there before > :) It would likely match any KVM infrastructure well, but we gain > interactions with the machine property. Also, there's a lot in the > whole CPU model area that need proper figuring out first... if we go > that route, we won't be able to add MTE support with KVM anytime soon, > I fear. > > The second option might be the most promising, except for potential > future headaches; but a lot depends on where we want to be going with > cpu models for KVM in general. What are the arguments for/against making MTE a machine type option or CPU feature flag? IIUC on real hardware you get "mte" or "mte3" listed in /proc/cpuinfo, so a CPU feature would seem a pretty natural fit to me, but I seem to recall that Peter was pushing for keeping it a machine property instead. Working off the assumption that Peter knows what he's doing :) can we do something like this? * introduce a new machine type property mte-version, which accepts either a specific version (2 for MTE and 3 for MTE3), an abstract setting (max and host) or a way to disable MTE entirely (none); * turn the existing mte machine type option into an alias with the mapping mte=off -> mte-version=none mte=on -> mte-version=max for TCG mte=on -> mte-version=host for KVM and deprecate it; * optionally introduce a new QMP command query-mte-capabilities that can be used by libvirt to figure out ahead of time which MTE versions are available for use on the current hardware. Yes, this is basically a shameless rip-off of how GIC is handled :) I'm pretty satisfied with how that works and see no reason to reinvent the wheel. Note that it's perfectly fine if the lack of KVM-level APIs results in mte-version=2 being rejected on MTE3-capable hardware for now! What's important is that you don't get a different MTE version than what you asked for. I assume that the existing KVM API for enabling MTE have good enough granularity to make this work? If not, that's going to be a problem :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v5 0/3] hw/riscv/virt: pflash improvements
On Fri, May 26, 2023 at 05:40:03PM +0530, Sunil V L wrote: > This series improves the pflash usage in RISC-V virt machine with solutions to > below issues. > > 1) Currently the first pflash is reserved for ROM/M-mode firmware code. But > S-mode > payload firmware like EDK2 need both pflash devices to have separate code and > variable > store so that OS distros can keep the FW code as read-only. > > The issue is reported at > https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6 > > 2) The latest way of using pflash devices in other architectures and libvirt > is by using -blockdev and machine options. However, currently this method is > not working in RISC-V. > > With above issues fixed, added documentation on how to use pflash devices > in RISC-V virt machine. > > This patch series is based on Alistair's riscv-to-apply.next branch. > > Changes since v4: > 1) Updated patch 2 to avoid accessing private field as per feedback > from Philippe. > 2) Updated documentation patch to add read-only for ROM usage. > 3) Rebased to latest riscv-to-apply.next branch and updated tags. Still works great :) Tested-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v4 0/3] hw/riscv/virt: pflash improvements
On Fri, May 26, 2023 at 10:34:36AM +0200, Andrew Jones wrote: > On Fri, May 26, 2023 at 03:49:11AM -0400, Andrea Bolognani wrote: > > So, are edk2 users the only ones who would (temporarily) need to > > manually turn ACPI off if virt-manager started enabling it by > > default? > > I assume so, but I'm not tracking firmware status. If the firmware > doesn't extract the ACPI tables from QEMU and present them to the > guest (afaik only edk2 does that), then the guest kernel falls back > to DT, which is why it's working for you. > > I suppose we should wait until Linux merges the ACPI patches, before > adding RISC-V to the libvirt capabilities ACPI list. That sounds reasonable to me, but note that 1) the libvirt change might take a while to propagate to distros and 2) someone will have to remind me to prepare such a patch when the time comes ;) > Then, is it > possible to use something like libosinfo to inform virt-manager > when it should enable ACPI and when not? Later distro images, with > later kernels, will want to use ACPI by default, but older images > will still need to use DT. Something like that would definitely be possible, but I don't think the scaffolding for it exists at the moment, so someone would have to wire it up across the stack. Given how relatively immature the RISC-V distro ecosystem is at the moment, I think it's fine to do nothing and wait for the problem to go away on its own :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v4 0/3] hw/riscv/virt: pflash improvements
On Fri, May 26, 2023 at 08:39:07AM +0200, Andrew Jones wrote: > On Thu, May 25, 2023 at 11:03:52AM -0700, Andrea Bolognani wrote: > > With these patches applied, libvirt built from the master branch, > > edk2 built from your branch and a JSON firmware descriptor for it > > installed (attached), it's finally possible to boot an unmodified > > openSUSE Tumbleweed RISC-V disk image by simply including > > > > > > Hi Andrea, > > I'm a bit concerned that we don't also need to add some XML in order to > disable ACPI right now. RISC-V guest kernels will support ACPI in the > near future. Ideally a default libvirt VM using edk2 will also use ACPI. > Will there be a problem with changing that default later? If so, then > I'd change it now and continue burdening developers a bit longer by > requiring them to explicitly disable it. libvirt doesn't enable ACPI by default on any architecture, not even x86_64. virt-manager will enable it by default if it's advertised as available on the architecture in the capabilities XML. However, it looks like the corresponding code in libvirt is not as dynamic as I would have assumed: instead, we hardcode the list of architectures that advertise ACPI support available, and at the moment that list does *not* include RISC-V :) I think it would make sense to fix this, but I want to make sure I understand the impact. Is this just an UEFI thing? All my other RISC-V guests (Fedora, Ubuntu, FreeBSD) boot just fine when I turn ACPI on. In fact, even the openSUSE one works with ACPI on, as long as the UEFI implementation used is the U-Boot one rather than edk2. So, are edk2 users the only ones who would (temporarily) need to manually turn ACPI off if virt-manager started enabling it by default? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v4 0/3] hw/riscv/virt: pflash improvements
On Thu, May 25, 2023 at 10:18:00PM +0530, Sunil V L wrote: > This series improves the pflash usage in RISC-V virt machine with solutions to > below issues. > > 1) Currently the first pflash is reserved for ROM/M-mode firmware code. But > S-mode > payload firmware like EDK2 need both pflash devices to have separate code and > variable > store so that OS distros can keep the FW code as read-only. > > The issue is reported at > https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6 > > 2) The latest way of using pflash devices in other architectures and libvirt > is by using -blockdev and machine options. However, currently this method is > not working in RISC-V. > > With above issues fixed, added documentation on how to use pflash devices > in RISC-V virt machine. > > This patch series is based on Alistair's riscv-to-apply.next branch. > > Changes since v3: > 1) Converted single patch to a series with a cover letter since there > are > multiple patches now. > 2) Added a new patch to enable pflash usage via -blockdev option. > 3) Separated the documentation change into new patch and updated the > documentation to mention only -blockdev option which seems to be the > recommended way of using pflash. Success! \o/ With these patches applied, libvirt built from the master branch, edk2 built from your branch and a JSON firmware descriptor for it installed (attached), it's finally possible to boot an unmodified openSUSE Tumbleweed RISC-V disk image by simply including in the domain XML, which is the same experience you'd have with x86_64 or aarch64. That's a *massive* leap forward when it comes to giving developers convenient access to a reasonable RISC-V environment they can play around with! Thanks a lot for your work on this, and I can't wait to see it all merged :) Tested-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization 50-edk2-riscv64-raw.json Description: application/json
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Thu, May 25, 2023 at 08:32:46PM +0530, Sunil V L wrote: > On Thu, May 25, 2023 at 01:43:28PM +0000, Andrea Bolognani wrote: > > I have also tried booting an openSUSE Tumbleweed "JeOS" image, since > > that's the only distro I'm aware of that uses UEFI boot on RISC-V at > > this point - though they use U-Boot's UEFI support rather than edk2. > > > > During that attempt, I ended up in the edk2 shell. Running > > > > fs0:\efi\boot\bootriscv64.efi > > > > brings up GRUB just fine, but selecting the default boot entry > > results in > > > > Loading Linux 6.3.2-1-default ... > > Loading initial ramdisk ... > > EFI stub: Booting Linux Kernel... > > EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path > > EFI stub: Generating empty DTB > > EFI stub: Exiting boot services... > > > > being printed, after which it's back to OpenSBI and from there to > > edk2 again. > > Thanks!. Please add -machine acpi=off in qemu command to boot the > kernel. Yup, that worked! It booted all the way to the login prompt :) Note that libvirt automatically adds acpi=off, so that won't be a concern for libvirt users. > > > > Going further and testing libvirt integration. After hacking around > > > > other issues, I finally stumbled upon this error: > > > > > > > > qemu-system-riscv64: Property 'virt-machine.pflash0' not found > > > > > > Thanks!. This needs some investigation. Let me look into supporting > > > this. > > > > Yes please! It's critical to libvirt integration. Feel free to CC me > > when you post patches and I'll gladly test them. > > Sure, I have the fix ready. I need to convert this into a patch series > now. Will send it soon and thanks in advance for helping with testing. Excellent! Looking forward to it :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Wed, May 24, 2023 at 11:03:52PM +0530, Sunil V L wrote: > On Wed, May 24, 2023 at 03:50:34PM +0000, Andrea Bolognani wrote: > > First off, the only RISC-V edk2 build readily accessible to me (from > > the edk2-riscv64 Fedora package) is configured to work off a R/W > > pflash1. You said that you have edk2 patches making R/O CODE pflash0 > > and R/W VARS pflash1 ready. Any chance you could make either the > > build output, or the patches and some hints on how to build edk2 > > after applying them, somewhere? > > Please build EDK2 using the branch > https://github.com/vlsunil/edk2/tree/separate_code_vars. > > The instructions to build is in > https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support#build-edk2 > > However, now it will create two images for code and vars. Following your pointers, I was able to build suitable edk2 images and verify that they work with a patched QEMU, so Tested-by: Andrea Bolognani Note, however, that said testing was limited to verifying that edk2 would come up. I have also tried booting an openSUSE Tumbleweed "JeOS" image, since that's the only distro I'm aware of that uses UEFI boot on RISC-V at this point - though they use U-Boot's UEFI support rather than edk2. During that attempt, I ended up in the edk2 shell. Running fs0:\efi\boot\bootriscv64.efi brings up GRUB just fine, but selecting the default boot entry results in Loading Linux 6.3.2-1-default ... Loading initial ramdisk ... EFI stub: Booting Linux Kernel... EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path EFI stub: Generating empty DTB EFI stub: Exiting boot services... being printed, after which it's back to OpenSBI and from there to edk2 again. I feel that this has probably more to do with the way the openSUSE image has been built than edk2, but I thought I'd report my experience nonetheless in case there's any useful information that can be gathered from it :) > > Going further and testing libvirt integration. After hacking around > > other issues, I finally stumbled upon this error: > > > > qemu-system-riscv64: Property 'virt-machine.pflash0' not found > > Thanks!. This needs some investigation. Let me look into supporting > this. Yes please! It's critical to libvirt integration. Feel free to CC me when you post patches and I'll gladly test them. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Tue, May 23, 2023 at 03:58:05PM +0530, Sunil V L wrote:
> Currently, virt machine supports two pflash instances each with
> 32MB size. However, the first pflash is always assumed to
> contain M-mode firmware and reset vector is set to this if
> enabled. Hence, for S-mode payloads like EDK2, only one pflash
> instance is available for use. This means both code and NV variables
> of EDK2 will need to use the same pflash.
>
> The OS distros keep the EDK2 FW code as readonly. When non-volatile
> variables also need to share the same pflash, it is not possible
> to keep it as readonly since variables need write access.
>
> To resolve this issue, the code and NV variables need to be separated.
> But in that case we need an extra flash. Hence, modify the convention
> such that pflash0 will contain the M-mode FW only when "-bios none"
> option is used. Otherwise, pflash0 will contain the S-mode payload FW.
> This enables both pflash instances available for EDK2 use.
>
> Example usage:
> 1) pflash0 containing M-mode FW
> qemu-system-riscv64 -bios none -pflash -machine virt
> or
> qemu-system-riscv64 -bios none \
> -drive file=,if=pflash,format=raw,unit=0 -machine virt
>
> 2) pflash0 containing S-mode payload like EDK2
> qemu-system-riscv64 -pflash -pflash -machine
> virt
> or
> qemu-system-riscv64 -bios \
> -pflash \
> -pflash \
> -machine virt
> or
> qemu-system-riscv64 -bios \
> -drive file=,if=pflash,format=raw,unit=0,readonly=on \
> -drive file=,if=pflash,format=raw,unit=1 \
> -machine virt
I wanted to test this, both directly with QEMU and through libvirt,
but I'm hitting two roadblocks.
First off, the only RISC-V edk2 build readily accessible to me (from
the edk2-riscv64 Fedora package) is configured to work off a R/W
pflash1. You said that you have edk2 patches making R/O CODE pflash0
and R/W VARS pflash1 ready. Any chance you could make either the
build output, or the patches and some hints on how to build edk2
after applying them, somewhere?
Going further and testing libvirt integration. After hacking around
other issues, I finally stumbled upon this error:
qemu-system-riscv64: Property 'virt-machine.pflash0' not found
This is because a few years back libvirt has stopped using -drive
if=pflash for configuring pflash devices, and these days it generates
something along the lines of
-blockdev
'{"driver":"file","filename":"...","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
-blockdev
'{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}'
-machine virt,pflash0=libvirt-pflash0-format
instead, which both aarch64/virt and x86_64/q35 machine types are
perfectly happy with.
I've tried to patch hw/riscv/virt.c to get the above working, and
almost managed to succeed :) but eventually my unfamiliarity with the
internals of QEMU caught up with me. Can you please look into making
this work?
Thanks!
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Fri, May 19, 2023 at 06:40:37PM +0200, Philippe Mathieu-Daudé wrote: > > On 18/5/23 08:03, Sunil V L wrote: > > > Agree. While I agree with Philippe, I think we better solve the current > > > problem by going back to v1 of the patch. > > BTW clarifying, I'm not rejecting this particular patch; I was just > trying to correct the idea than "doing what other architectures do" > is always the right approach ;) Definitely agree with that :) But there's a cost associated with going off the beaten path, which the benefits must then outweight. There seems to be rough consensus of that not being the case here. So, can we get v1 merged? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Tue, Apr 25, 2023 at 03:55:45PM +0530, Sunil V L wrote: > qemu-system-riscv64 -bios \ > -drive file=,if=pflash,format=raw,unit=0 \ > -drive file=,if=pflash,format=raw,unit=1,readonly=on \ > -machine virt I've noticed that edk2 for RISC-V, at least in the form it is currently packaged for Fedora, doesn't seem to have separate CODE and VARS files: $ ls /usr/share/edk2/riscv/* | cat /usr/share/edk2/riscv/RISCV_VIRT.fd /usr/share/edk2/riscv/RISCV_VIRT.raw Is that something that needs to be addressed in upstream edk2? If so, will you be looking into it? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Thu, May 18, 2023 at 02:53:05PM +1000, Alistair Francis wrote: > On Wed, May 17, 2023 at 6:45 PM Andrea Bolognani wrote: > > On Wed, May 17, 2023 at 02:57:12PM +1000, Alistair Francis wrote: > At one point we loaded Oreboot in in flash and booted from that. I > think Oreboot then loaded OpenSBI into memory. The idea was to mimic > what a physical board would do, so we could allow testing. > > It doesn't look like it's used any more. > > > Either way, assuming that there's a genuine reason why pflash must be > > used, I think the behavior implemented in v1 (pflash0 is M-mode when > > -bios none is used, S-mode otherwise) maps very well conceptually, > > and results in behavior matching that of other architectures out of > > the box. That's good enough for me :) I was just wondering whether we > > could keep things even simpler. > > I don't see a reason to remove the boot from pflash if no -bios > argument is supplied. If there is a good reason to, I think we can > (via deprecation) but the current functionality seems fine to me. Aligning with other architectures. In the short term, we cause some minor pain to existing users, although as noted above for the oreboot case there might not even be any people still depending on this functionality at this point. In the long run, we guarantee smooth operation across the stack by not being different for being different's sake (e.g. requiring pflash0 and pflash1 to be swapped compared to x86 and Arm, which is a configuration that falls outside what can be described by the firmware.json standard and thus can't be automatically applied by libvirt). -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Wed, May 17, 2023 at 02:47:42PM +0200, Philippe Mathieu-Daudé wrote: > > > On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > > > > On amd64 and arm64 unit=0 is used for code and unit=1 is used for > > > > variables. > > > > Shouldn't riscv64 do the same? > > > > Good catch, I had missed that! > > This is a design mistake spreading. > > What EDK2 maintainers want is one Read-Only + Exec region for CODE and > one Read-Write + NoExec region for VARS. > > QEMU never implemented correctly pflash bank (multiple sectors) write > protected. Does setting -drive if=pflash,unit=0,readonly=on not do the trick? > When EDK2 (x86, OVMF) was tried on QEMU, QEMU was using a single pflash. > To separate CODE/VARS, a second pflash was added, the first one being > "locked" into Read-Only mode. Using a pflash allowed the firmware to > identify device size using pflash CFI commands. > > Then this design was copied to the ARM virt board for EDK2 needs. > > In retrospective, this design was declared a mistake, since a simple > ROM region for the CODE is sufficient, and much simpler [*]. > > Thankfully the Loongarch64 virt machine started cleanly avoiding the > previous design flaw. It provides a ROM for CODE and pflash for VARS. Based on the documentation both in QEMU and edk2, it looks like a single file is used? I haven't seen an example where the pflash is used to provide a R/W area for VARS. Note that the current version of the firmware.json standard doesn't include a way to describe builds that have to be consumed by loading the CODE via -bios and the VARS via pflash. This is likely an artifact of codifying existing usage (x86/arm) and could probably be fixed, but the point remains that there is currently no way to represent such a build in a way that makes it possible for consumers such as libvirt to automatically pick it up. > [*] Having 2 distinct pflash is useful for non-virt machines where the > firmware might want to (re)program the CODE region, in the "capsule > update" scenario. This scenario is irrelevant for virt machines, > since a guest will never update its CODE. CODE is updated by the > host. Yeah, this makes sense. But I don't understand what's wrong with using a R/O pflash for CODE as opposed to -bios? What makes that approach so problematic? You're still going to need to use pflash for VARS anyway... -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Wed, May 17, 2023 at 02:57:12PM +1000, Alistair Francis wrote: > On Mon, May 8, 2023 at 9:45 PM Andrea Bolognani wrote: > > > > Taking a step back, what is even the use case for having M-mode code > > > > in pflash0? If you want to use an M-mode firmware, can't you just use > > > > -bios instead? In other words, can we change the behavior so that > > > > pflash being present always mean loading S-mode firmware off it? > > It was originally added to support Oreboot (the Rust version of > Coreboot). The idea was that Oreboot (ROM) would be in flash and then > go from there. > > It also applies to other ROM code that a user might want to test that > runs before OpenSBI. Is there a reason why these would have to be loaded into pflash instead of being passed to -bios? From a quick look at the documentation for oreboot[1], it looks like they're doing the latter. Either way, assuming that there's a genuine reason why pflash must be used, I think the behavior implemented in v1 (pflash0 is M-mode when -bios none is used, S-mode otherwise) maps very well conceptually, and results in behavior matching that of other architectures out of the box. That's good enough for me :) I was just wondering whether we could keep things even simpler. [1] https://github.com/oreboot/oreboot/blob/main/src/mainboard/emulation/qemu-riscv/QEMU.md -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Mon, May 08, 2023 at 04:53:46PM +0530, Sunil V L wrote: > On Mon, May 08, 2023 at 03:00:02AM -0700, Andrea Bolognani wrote: > > I think that it's more important to align with other architectures. > > > > The number of people currently running edk2 on RISC-V is probably > > vanishingly small, and in my opinion requiring them to tweak their > > command lines a bit is a fair price to pay to avoid having to carry a > > subtle difference between architectures for years to come. > > It is not just tweaking the command line. The current EDK2 will not work > anymore if code is moved to plfash 0 since EDK2 assumed its entry point > is in pflash1. I agree there may not be too many users but if we have > to align with other archs, there will be combinations of qemu and > edk2 versions which won't work. Right. > > With that in mind, my preference would be to go back to v1. > > Thanks!. If this is the preference, we can request people to use proper > versions of EDK2 with different qemu versions. Yeah, in the (not so) long run this will just not matter, as the versions of edk2 and QEMU available to people will all implement the new behavior. Better to optimize for the long future ahead of us rather than causing ongoing pain for the sake of the few users of a work-in-progress board. > > Taking a step back, what is even the use case for having M-mode code > > in pflash0? If you want to use an M-mode firmware, can't you just use > > -bios instead? In other words, can we change the behavior so that > > pflash being present always mean loading S-mode firmware off it? > > TBH, I don't know. I am sure Alistair would know since it was added in > https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94 > I don't think opensbi can be launched from pflash. So, it may be some > other use case which I am now aware of. > > I will be happy if this can be avoided by using -bios. The actual commit would be [1], from late 2019. Things might have changed in the intervening ~3.5 years. Let's wait to hear from Alistair :) [1] https://github.com/qemu/qemu/commit/2738b3b555efaf206b814677966e8e3510c64a8a -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"
On Mon, May 08, 2023 at 11:37:43AM +0530, Sunil V L wrote: > On Mon, May 08, 2023 at 07:37:23AM +0200, Heinrich Schuchardt wrote: > > On 4/25/23 12:25, Sunil V L wrote: > > > Currently, virt machine supports two pflash instances each with > > > 32MB size. However, the first pflash is always assumed to > > > contain M-mode firmware and reset vector is set to this if > > > enabled. Hence, for S-mode payloads like EDK2, only one pflash > > > instance is available for use. This means both code and NV variables > > > of EDK2 will need to use the same pflash. > > > > > > The OS distros keep the EDK2 FW code as readonly. When non-volatile > > > variables also need to share the same pflash, it is not possible > > > to keep it as readonly since variables need write access. > > > > > > To resolve this issue, the code and NV variables need to be separated. > > > But in that case we need an extra flash. Hence, modify the convention > > > such that pflash0 will contain the M-mode FW only when "-bios none" > > > option is used. Otherwise, pflash0 will contain the S-mode payload FW. > > > This enables both pflash instances available for EDK2 use. > > > > > > Example usage: > > > 1) pflash0 containing M-mode FW > > > qemu-system-riscv64 -bios none -pflash -machine virt > > > or > > > qemu-system-riscv64 -bios none \ > > > -drive file=,if=pflash,format=raw,unit=0 -machine virt > > > > > > 2) pflash0 containing S-mode payload like EDK2 > > > qemu-system-riscv64 -pflash -pflash > > > -machine virt > > > or > > > qemu-system-riscv64 -bios \ > > > -pflash \ > > > -pflash \ > > > > On amd64 and arm64 unit=0 is used for code and unit=1 is used for variables. > > Shouldn't riscv64 do the same? Good catch, I had missed that! > Is that a requirement from distros perspective? That was my original v1 > design. > > But the reason why I kept unit0 for variables, it helps in keeping current > EDK2 usage model work. Otherwise, current EDK2 will break if we change > the code to unit 0. I think that it's more important to align with other architectures. The number of people currently running edk2 on RISC-V is probably vanishingly small, and in my opinion requiring them to tweak their command lines a bit is a fair price to pay to avoid having to carry a subtle difference between architectures for years to come. With that in mind, my preference would be to go back to v1. > Second, since unit 0 for RISC-V is currently assumed to start in M-mode fw > which is secure, I think it makes sense to keep variables also in unit > 0. If you're storing variables rather than code in pflash0, does it even make sense to talk about M-mode and S-mode? Taking a step back, what is even the use case for having M-mode code in pflash0? If you want to use an M-mode firmware, can't you just use -bios instead? In other words, can we change the behavior so that pflash being present always mean loading S-mode firmware off it? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] hw/riscv: virt: Enable booting M-mode or S-mode FW from pflash0
On Fri, Apr 21, 2023 at 11:31:44PM +0200, Philippe Mathieu-Daudé wrote: > On 21/4/23 18:48, Andrea Bolognani wrote: > > For what it's worth, this change seems to go in the right direction > > by making things similar to other architectures (x86, Arm) so I'd > > love to see it happen. > > Unfortunately another arch that followed the bad example of using > a R/W device for the CODE region and not a simple ROM. I'm not sure if that mitigates your concern, but at least when using libvirt you're usually going to get one R/O pflash for the CODE part and one R/W pflash for the VARS part. This is the case today on x86_64 and aarch64, and with this change the same would be true on riscv64 going forward. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] hw/riscv: virt: Enable booting M-mode or S-mode FW from pflash0
On Fri, Apr 21, 2023 at 04:36:15PM +0200, Heinrich Schuchardt wrote: > On 4/21/23 06:33, Sunil V L wrote: > > Currently, virt machine supports two pflash instances each with > > 32MB size. However, the first pflash is always assumed to > > contain M-mode firmware and reset vector is set to this if > > enabled. Hence, for S-mode payloads like EDK2, only one pflash > > instance is available for use. This means both code and NV variables > > of EDK2 will need to use the same pflash. > > > > The OS distros keep the EDK2 FW code as readonly. When non-volatile > > variables also need to share the same pflash, it is not possible > > to keep it as readonly since variables need write access. > > > > To resolve this issue, the code and NV variables need to be separated. > > But in that case we need an extra flash. Hence, modify the convention > > such that pflash0 will contain the M-mode FW only when "-bios none" > > option is used. Otherwise, pflash0 will contain the S-mode payload FW. > > This enables both pflash instances available for EDK2 use. > > > > Example usage: > > 1) pflash0 containing M-mode FW > > qemu-system-riscv64 -bios none -pflash -machine virt > > or > > qemu-system-riscv64 -bios none \ > > -drive file=,if=pflash,format=raw,unit=0 -machine virt > > > > 2) pflash0 containing S-mode payload like EDK2 > > qemu-system-riscv64 -pflash -pflash -machine > > virt > > or > > qemu-system-riscv64 -bios \ > > -pflash \ > > -pflash \ > > -machine virt > > or > > qemu-system-riscv64 -bios \ > > -drive file=,if=pflash,format=raw,unit=0,readonly=on \ > > -drive file=,if=pflash,format=raw,unit=1 \ > > -machine virt > > > > Signed-off-by: Sunil V L > > Reported-by: Heinrich Schuchardt > > QEMU 7.2 (and possibly 8.0 to be released) contains the old behavior. > > Changed use of command line parameters should depend on the version of > the virt machine, i.e. virt-7.2 should use the old behavior and virt as > alias for virt-8.0 should use the new behavior. Please, have a look at > the option handling in hw/arm/virt.c and macro DEFINE_VIRT_MACHINE(). I would normally agree with you, but note that RISC-V doesn't have versioned machine types yet, so this kind of breakage is not necessarily unexpected. >From libvirt's point of view, being able to detect whether the new behavior is implemented by looking for some machine type property would be enough to handle the transition smoothly. That would of course not help people running QEMU directly. For what it's worth, this change seems to go in the right direction by making things similar to other architectures (x86, Arm) so I'd love to see it happen. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v6 1/2] arm/kvm: add support for MTE
On Thu, Mar 02, 2023 at 02:26:06PM +0100, Cornelia Huck wrote: > On Wed, Mar 01 2023, Andrea Bolognani wrote: > > Note that, from libvirt's point of view, there's no advantage to > > doing things that way instead of what you already have. Handling the > > additional machine property is a complete non-issue. But it would > > make things nicer for people running QEMU directly, I think. > > I'm tempted to simply consider this to be another wart of the QEMU > command line :) Fine by me! Papering over such idiosyncrasies is part of libvirt's core mission after all :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v6 1/2] arm/kvm: add support for MTE
On Wed, Mar 01, 2023 at 03:15:17PM +0100, Cornelia Huck wrote: > On Wed, Mar 01 2023, Andrea Bolognani wrote: > > I'm actually a bit confused. The documentation for the mte property > > of the virt machine type says > > > > mte > > Set on/off to enable/disable emulating a guest CPU which implements > > the Arm Memory Tagging Extensions. The default is off. > > > > So why is there a need to have a CPU property in addition to the > > existing machine type property? > > I think the state prior to my patches is actually a bit confusing: the > user needs to set a machine type property (causing tag memory to be > allocated), which in turn enables a cpu feature. Supporting the machine > type property for KVM does not make much sense IMHO: we don't allocate > tag memory for KVM (in fact, that would not work). We have to keep the > previous behaviour, and explicitly instructing QEMU to create cpus with > a certain feature via a cpu property makes the most sense to me. I agree that a CPU feature makes more sense. > We might want to tweak the documentation for the machine property to > indicate that it creates tag memory and only implicitly enables mte but > is a pre-req for it -- thoughts? I wonder if it would be possible to flip things around, so that the machine property is retained with its existing behavior for backwards compatibility, but both for KVM and for TCG the CPU property can be used on its own? Basically, keeping the default of machine.mte to off when cpu.mte is not specified, but switching it to on when it is. This way, you'd be able to simply use -machine virt -cpu xxx,mte=on to enable MTE, regardless of whether you're using KVM or TCG, instead of requiring the above for KVM and -machine virt,mte=on -cpu xxx for TCG. Note that, from libvirt's point of view, there's no advantage to doing things that way instead of what you already have. Handling the additional machine property is a complete non-issue. But it would make things nicer for people running QEMU directly, I think. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v6 1/2] arm/kvm: add support for MTE
On Wed, Mar 01, 2023 at 11:17:40AM +0100, Cornelia Huck wrote: > On Tue, Feb 28 2023, Andrea Bolognani wrote: > > On Tue, Feb 28, 2023 at 04:02:15PM +0100, Cornelia Huck wrote: > >> +MTE CPU Property > >> + > >> + > >> +The ``mte`` property controls the Memory Tagging Extension. For TCG, it > >> requires > >> +presence of tag memory (which can be turned on for the ``virt`` machine > >> via > >> +``mte=on``). For KVM, it requires the ``KVM_CAP_ARM_MTE`` capability; > >> until > >> +proper migration support is implemented, enabling MTE will install a > >> migration > >> +blocker. > > > > Is it okay to use -machine virt,mte=on unconditionally for both KVM > > and TCG guests when MTE support is requested, or will that not work > > for the former? > > QEMU will error out if you try this with KVM (basically, same behaviour > as before.) Is that a problem for libvirt, or merely a bit inconvinient? I'm actually a bit confused. The documentation for the mte property of the virt machine type says mte Set on/off to enable/disable emulating a guest CPU which implements the Arm Memory Tagging Extensions. The default is off. So why is there a need to have a CPU property in addition to the existing machine type property? >From the libvirt integration point of view, setting the machine type property only for TCG is not a problem. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v6 1/2] arm/kvm: add support for MTE
On Tue, Feb 28, 2023 at 04:02:15PM +0100, Cornelia Huck wrote: > Introduce a new cpu feature flag to control MTE support. To preserve > backwards compatibility for tcg, MTE will continue to be enabled as > long as tag memory has been provided. > > If MTE has been enabled, we need to disable migration, as we do not > yet have a way to migrate the tags as well. Therefore, MTE will stay > off with KVM unless requested explicitly. > > Signed-off-by: Cornelia Huck > --- > docs/system/arm/cpu-features.rst | 21 ++ > hw/arm/virt.c| 2 +- > target/arm/cpu.c | 18 ++--- > target/arm/cpu.h | 1 + > target/arm/cpu64.c | 110 +++ > target/arm/internals.h | 1 + > target/arm/kvm.c | 29 > target/arm/kvm64.c | 5 ++ > target/arm/kvm_arm.h | 19 ++ > target/arm/monitor.c | 1 + > 10 files changed, 194 insertions(+), 13 deletions(-) I've given a quick look with libvirt integration in mind, and everything seem fine. Specifically, MTE is advertised in the output of qom-list-properties both for max-arm-cpu and the latest virt-X.Y-machine, which means that libvirt can easily and reliably figure out whether MTE support is available. > +MTE CPU Property > + > + > +The ``mte`` property controls the Memory Tagging Extension. For TCG, it > requires > +presence of tag memory (which can be turned on for the ``virt`` machine via > +``mte=on``). For KVM, it requires the ``KVM_CAP_ARM_MTE`` capability; until > +proper migration support is implemented, enabling MTE will install a > migration > +blocker. Is it okay to use -machine virt,mte=on unconditionally for both KVM and TCG guests when MTE support is requested, or will that not work for the former? > +If not specified explicitly via ``on`` or ``off``, MTE will be available > +according to the following rules: > + > +* When TCG is used, MTE will be available if and only if tag memory is > available; > + i.e. it preserves the behaviour prior to the introduction of the feature. > + > +* When KVM is used, MTE will default to off, so that migration will not > + unintentionally be blocked. This might change in a future QEMU version. If and when this changes, we should ensure that the new default behavior doesn't affect existing machine types, otherwise we will break guest ABI for existing VMs. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI
On Tue, Feb 07, 2023 at 08:20:31PM +0100, Andrew Jones wrote:
> On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote:
> > Well, it looks like -no-acpi will come for free after all, thanks to
> > the code you pasted above, as long as the machine property follows
> > the convention established by x86, arm and (I just noticed this one)
> > loongarch.
>
> Not quite, because we also have
>
> $ grep -A1 '"no-acpi"' qemu-options.hx
> DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
>"-no-acpidisable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM)
>
> So that means neither riscv nor loongarch get -no-acpi just by adding
> the "acpi" machine property.
>
> If the plan is to deprecate -no-acpi, then riscv can be like loongarch
> and only have the "acpi" property from the start.
Makes sense.
For the libvirt integration, do I understand correctly that the
machine property was added by commit
commit 17e89077b7e3bc1d96540e21ddc7451c3e077049
Author: Gerd Hoffmann
Date: Fri Mar 20 11:01:36 2020 +0100
acpi: add acpi=OnOffAuto machine property to x86 and arm virt
Remove the global acpi_enabled bool and replace it with an
acpi OnOffAuto machine property.
qemu throws an error now if you use -no-acpi while the machine
type you are using doesn't support acpi in the first place.
Signed-off-by: Gerd Hoffmann
Message-Id: <20200320100136.11717-1-kra...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé
Acked-by: Paolo Bonzini
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
included in QEMU 5.0? libvirt still officially supports QEMU 4.2, so
we'll have to make the use of the machine property conditional.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI
On Tue, Feb 07, 2023 at 03:02:19PM +0100, Thomas Huth wrote: > On 07/02/2023 14.56, Andrea Bolognani wrote: > > It looks like i440fx and q35 both have an 'acpi' machine property. Is > > -no-acpi just sugar for acpi=off? > > Yes, it is, see softmmu/vl.c: > > case QEMU_OPTION_no_acpi: > qdict_put_str(machine_opts_dict, "acpi", "off"); > break; > > > Is it considered desirable to get rid of -no-acpi? > > Sounds like a good idea, indeed! > > > If so, we should follow the usual deprecation > > process and get rid of it after libvirt has had a chance to adapt. > > > > In the scenario described above, it would make sense for RISC-V to > > only offer the machine type option (assuming that -no-acpi doesn't > > come for free with that) instead of putting additional effort into > > implementing an interface that is already on its way out. > > I agree. IMHO the machine parameter should be enough, no need to introduce > "-no-acpi" here. Well, it looks like -no-acpi will come for free after all, thanks to the code you pasted above, as long as the machine property follows the convention established by x86, arm and (I just noticed this one) loongarch. So I guess the -no-acpi deprecation can be handled separately, and the only thing that needs changing in the current patch is making sure that ACPI is opt-out rather than opt-in, as is the case for other architectures :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI
On Tue, Feb 07, 2023 at 11:23:53AM +0100, Andrew Jones wrote: > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote: > > On 6/2/23 11:54, Andrea Bolognani wrote: > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > > > > +object_class_property_add(oc, "acpi", "OnOffAuto", > > > > + virt_get_acpi, virt_set_acpi, > > > > + NULL, NULL); > > > > +object_class_property_set_description(oc, "acpi", > > > > + "Enable ACPI"); > > > > > > The way this works on other architectures (x86_64, aarch64) is that > > > you get ACPI by default and can use -no-acpi to disable it if > > > desired. Can we have the same on RISC-V, for consistency? > > > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and > > has a high maintenance cost / burden. > > Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically. > If -no-acpi is problematic for some reason, then something like > '-machine virt,acpi=off' would be sufficient for switching to DT too. I would greatly prefer it if the command line interface could be kept consistent across architectures. It looks like i440fx and q35 both have an 'acpi' machine property. Is -no-acpi just sugar for acpi=off? Is it considered desirable to get rid of -no-acpi? If so, we should follow the usual deprecation process and get rid of it after libvirt has had a chance to adapt. In the scenario described above, it would make sense for RISC-V to only offer the machine type option (assuming that -no-acpi doesn't come for free with that) instead of putting additional effort into implementing an interface that is already on its way out. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI
On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote: > +object_class_property_add(oc, "acpi", "OnOffAuto", > + virt_get_acpi, virt_set_acpi, > + NULL, NULL); > +object_class_property_set_description(oc, "acpi", > + "Enable ACPI"); The way this works on other architectures (x86_64, aarch64) is that you get ACPI by default and can use -no-acpi to disable it if desired. Can we have the same on RISC-V, for consistency? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] hw/virtio/virtio-iommu-pci: Enforce the device is plugged on the root bus
On Wed, Oct 12, 2022 at 12:34:48PM -0400, Eric Auger wrote: > In theory the virtio-iommu-pci could be plugged anywhere in the PCIe > topology and as long as the dt/acpi info are properly built this should > work. However at the moment we fail to do that because the > virtio-iommu-pci BDF is not computed at plug time and in that case > vms->virtio_iommu_bdf gets an incorrect value. > > For instance if the virtio-iommu-pci is plugged onto a pcie root port > and the virtio-iommu protects a virtio-block-pci device the guest does > not boot. > > So let's do not pretend we do support this case and fail the initialize() > if we detect the virtio-iommu-pci is plugged anywhere else than on the > root bus. Anyway this ability is not needed. > > Signed-off-by: Eric Auger > --- > hw/virtio/virtio-iommu-pci.c | 12 ++-- > 1 file changed, 10 insertions(+), 2 deletions(-) FYI libvirt will already reject any attempts to place the device anywhere but directly on pcie.0, so from our point of view merging this patch is perfectly fine. -- Andrea Bolognani / Red Hat / Virtualization
Re: [qemu-web PATCH] Add signing pubkey for python-qemu-qmp package
On Thu, Aug 18, 2022 at 12:53:58PM -0400, John Snow wrote: > Add the pubkey currently used for signing PyPI releases of qemu.qmp to a > stable location where it can be referenced by e.g. Fedora RPM specfiles. > > At present, the key happens to just simply be my own -- but future > releases may be signed by a different key. In that case, we can > increment '1.txt' to '2.txt' and so on. The old keys should be left in > place. > > The format for the keyfile was chosen by copying what OpenStack was > doing: > https://releases.openstack.org/_static/0x2426b928085a020d8a90d0d879ab7008d0896c8a.txt > > Generated with: > > gpg --with-fingerprint --list-keys js...@redhat.com > pubkey > > gpg --armor --export js...@redhat.com >> pubkey You might want to pass --export-options export-minimal to the second command in order to obtain a significantly smaller file that can still serve the intended purpose. -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
On Wed, Aug 17, 2022 at 04:04:19PM +0200, Victor Toso wrote:
> On Tue, Jul 05, 2022 at 08:45:06AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:26PM +0200, Victor Toso wrote:
> > > func (s *BlockdevRef) UnmarshalJSON(data []byte) error {
> > > // Check for json-null first
> > > if string(data) == "null" {
> > > return errors.New(`null not supported for BlockdevRef`)
> > > }
> > > // Check for BlockdevOptions
> > > {
> > > s.Definition = new(BlockdevOptions)
> > > if err := StrictDecode(s.Definition, data); err == nil {
> > > return nil
> > > }
> >
> > The use of StrictDecode() here means that we won't be able to
> > parse an alternate produced by a version of QEMU where
> > BlockdevOptions has gained additional fields, doesn't it?
>
> That's correct. This means that with this RFCv2 proposal, qapi-go
> based on qemu version 7.1 might not be able to decode a qmp
> message from qemu version 7.2 if it has introduced a new field.
>
> This needs fixing, not sure yet the way to go.
>
> > Considering that we will happily parse such a BlockdevOptions
> > outside of the context of BlockdevRef, I think we should be
> > consistent and allow the same to happen here.
>
> StrictDecode is only used with alternates because, unlike unions,
> Alternate types don't have a 'discriminator' field that would
> allow us to know what data type to expect.
>
> With this in mind, theoretically speaking, we could have very
> similar struct types as Alternate fields and we have to find on
> runtime which type is that underlying byte stream.
>
> So, to reply to your suggestion, if we allow BlockdevRef without
> StrictDecode we might find ourselves in a situation that it
> matched a few fields of BlockdevOptions but it the byte stream
> was actually another type.
IIUC your concern is that the QAPI schema could gain a new type,
TotallyNotBlockdevOptions, which looks exactly like BlockdevOptions
except for one or more extra fields.
If QEMU then produced a JSON like
{ "description": { /* a TotallyNotBlockdevOptions here */ } }
and we'd try to deserialize it with Go code like
ref := BlockdevRef{}
json.Unmarsal()
we'd end up mistakenly parsing the TotallyNotBlockdevOptions as a
valid BlockdevOptions, dropping the extra fields in the process.
Does that correctly describe the reason why you feel that the use of
StrictDecode is necessary?
If so, I respectfully disagree :)
If the client code is expecting a BlockdevRef as the return value of
a command and QEMU is producing something that is *not* a BlockdevRef
instead, that's an obvious bug in QEMU. If the client code is
expecting a BlockdevRef as the return value of a command that is
specified *not* to return a BlockdevRef, that's an obvious bug in the
client code.
In neither case it should be the responsibility of the SDK to
second-guess the declared intent, especially when it's perfectly
valid for a type to be extended in a backwards-compatible way by
adding fields to it.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
On Fri, Aug 19, 2022 at 09:20:52AM +0200, Victor Toso wrote:
> > > On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> > > > On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > > > > You're right, that is undesirable. What about something like this?
> > > > >
> > > > > type GuestPanicInformation struct {
> > > > > HyperV *GuestPanicInformationHyperV
> > > > > S390 *GuestPanicInformationS390
> > > > > }
> > > > >
> > > > > type jsonGuestPanicInformation struct {
> > > > > Discriminator string `json:"type"`
> > > > > HyperV*GuestPanicInformationHyperV `json:"hyper-v"`
> > > > > S390 *GuestPanicInformationS390 `json:"s390"`
> > > > > }
> > > >
> > > > It can possibly be even simpler with just embedding the real
> > > > struct
> > > >
> > > >type jsonGuestPanicInformation struct {
> > > >Discriminator string
> > > >GuestPanicInformation
> > > >}
> >
> > Similar to what I said in previous email (same thread) to Andrea,
> > this would not work because the end result does not match with
> > QAPI spec, where HyperV or S390 fields should be at the same
> > level as 'type'.
Yeah, you're absolutely correct, I was misreading the schema and
suggested an implementation that couldn't possibly work.
> > If we embed either HyperV or S390, then it should work, like:
> >
> > tmp := struct {
> > GuestPanicInformationHyperV
> > Discriminator string "type"
> > }{}
>
> For the same reason, I could not use json.RawMessage, sadly.
>
> As Andrea pointed out before, json.RawMessage is just an alias
> for []byte. We need a field for that (so, it can't be embed) and
> the contents of the JSON need to match that field's name.
Right. It would work if unions looked like
{ "type": "...", "data": { ... }}
with the "data" part having different fields based on the value of
"type", but not for the flat JSON dictionaries that are used for QAPI
unions.
> func (s QCryptoBlockOpenOptions) MarshalJSON() ([]byte, error) {
> var bytes []byte
> var err error
> if s.Qcow != nil {
> tmp := struct {
> QCryptoBlockOptionsQCow
> Discriminator string `json:"format"`
> }{
> QCryptoBlockOptionsQCow: *s.Qcow,
> Discriminator: "qcow",
> }
> if bytes, err = json.Marshal(tmp); err != nil {
> return nil, err
> }
> }
> if s.Luks != nil {
> if len(bytes) != 0 {
> return nil, errors.New(`multiple fields set for
> QCryptoBlockOpenOptions`)
> }
Why are you checking this here? I would just have a check like
if s.Qcow != nil && s.Luks != nil {
return nil, errors.New(`multiple fields set for QCryptoBlockOpenOptions`)
}
at the top of the function, so that you can abort early and cheaply
if the user has provided invalid input instead of having to wait
after one of the fields has already been serialized.
> tmp := struct {
> QCryptoBlockOptionsLUKS
> Discriminator string `json:"format"`
> }{
> QCryptoBlockOptionsLUKS: *s.Luks,
> Discriminator: "luks",
> }
> if bytes, err = json.Marshal(tmp); err != nil {
> return nil, err
> }
> }
> if len(bytes) == 0 {
> return nil, errors.New(`null not supported for
> QCryptoBlockOpenOptions`)
> }
Similarly, this should be checked early. So the complete check could
be turned into
if (s.Qcow != nil && s.Luks != nil) || (s.Qcow == nil && s.Luks == nil) {
return nil, errors.New("must set exactly one field")
}
You should have enough information to produce such a check when
generating the function, right? If making sure all possible
combinations are covered up ends up being too complicated, something
like
var setFields int
if s.Field1 != nil {
setFields += 1
}
if s.Field2 != nil {
setFields += 1
}
// ...
if setFields != 1 {
return nil, errors.New("must set exactly one field")
}
w
Re: [PATCH 2/2] docs: build-platforms: Clarify stance on minor releases and backports
On Tue, Jun 14, 2022 at 06:42:58AM -0700, Andrea Bolognani wrote: > On Wed, May 04, 2022 at 09:23:28AM +0100, Daniel P. Berrangé wrote: > > On Wed, May 04, 2022 at 01:01:03AM -0700, Andrea Bolognani wrote: > > > On Wed, Apr 20, 2022 at 09:18:47AM -0700, Andrea Bolognani wrote: > > > > On Wed, Apr 20, 2022 at 05:15:08PM +0100, Daniel P. Berrangé wrote: > > > > > On Wed, Apr 20, 2022 at 06:03:11PM +0200, Andrea Bolognani wrote: > > > > > > These changes match those made in the following libvirt commits: > > > > > > > > > > > > 2ac78307af docs: Clarify our stance on backported packages > > > > > > 78cffd450a docs: Spell out our policy concerning minor releases > > > > > > > > > > > > Since QEMU's platform support policy is based on libvirt's, it > > > > > > makes sense to mirror these recent changes made to the latter. > > > > > > > > > > > > The policy is not altered significantly - we're simply spelling > > > > > > out some rules that were likely already being implicitly > > > > > > enforced. > > > > > > > > > > Indeed, I think that's basically defacto the case already. > > > > > > > > > > Reviewed-by: Daniel P. Berrangé > > > > > > > > Thanks! Are you going to bring these in through one of your trees, or > > > > do I need to bug someone else so that they will pick them up? :) > > > > > > I see these haven't gone in yet. Anything I can/should do to make > > > that happen? > > > > The tragedy of QEMU not having a central docs maintainer. I'll queue > > this one for my next pull request. > > Still doesn't seem to have been merged. Not pressuring you or > anything, just making sure it doesn't slip through the cracks :) I'm still not seeing this in the tree. I figure with 7.1 coming up you might be preparing a pull request at some point in the near future so I though I'd ping again :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
On Wed, Jul 06, 2022 at 04:07:43PM +0100, Daniel P. Berrangé wrote: > On Wed, Jul 06, 2022 at 09:53:43AM -0500, Andrea Bolognani wrote: > > Yeah but we're generating structs for all possible events ourselves > > and we don't really expect external implementations of this > > interface so I don't see what having this getter buys us over the > > guarantee, that we have by construction, that all events will have a > > public member with a specific name that contains the timestamp. > > Code doesn't neccessarily want to deal with the per-event type > structs. It is credible to want to work with the abstract 'Event' > interface in places and being able to get the Timestamp from that, > without figuring out what specific event struct to cast it to first. Makes sense. > > I still don't like the fact that we have to force clients to use a > > non-standard API, or that the API for events has to be different from > > that for unions. But Go won't let you add method implementations to > > an interface, so we're kinda stuck. > > I think this specific case is out of scope for the "standard" JSON > APIs, and somewhere you'd expect APIs to do their own thing a layer > above, which is what we're doing here. > > More importantly though what we're generating in terms of QAPI derived > APIs should be thought of as only the first step. Ultimately I would > not expect clients to be marshalling / unmarshalling these structs at > all. So from that POV I think the question of "non-standard" APIs is > largely irrelevant. > > Instead we would be providing a "QMPClient" type with APIs for sending/ > receiving instances of the 'Command'/'Response' interfaces, along with > callback(s) that receives instances of the 'Event' interface. Any JSON > marshalling is hidden behind the scenes as an internal imlp detail. I will note that we want the marshalling/unmarshalling part and the wire transfer part to be somewhat loosely coupled, to allow for use cases that are not covered by the high-level client API, but overall I now agree with you that for most users this shouldn't ultimately matter :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
On Tue, Jul 05, 2022 at 05:47:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 08:45:54AM -0700, Andrea Bolognani wrote:
> > On Fri, Jun 17, 2022 at 02:19:29PM +0200, Victor Toso wrote:
> > > func (s *AcpiDeviceOstEvent) GetTimestamp() Timestamp {
> > > return s.EventTimestamp
> > > }
> >
> > Does this even need a getter? The struct member is public, and Go
> > code seems to favor direct member access.
>
> It satisfies the 'Event' interface, so you can fetch timestamp
> without needing to know the specific event struct you're dealing
> with.
Yeah but we're generating structs for all possible events ourselves
and we don't really expect external implementations of this
interface so I don't see what having this getter buys us over the
guarantee, that we have by construction, that all events will have a
public member with a specific name that contains the timestamp.
> I don't think this kind of detail really needs to be exposed to
> clients. Also parsing the same json doc twice just feels wrong.
>
> I think using the dummy union structs is the right approach, but
> I'd just call it 'EventFromJSON' rather than 'UnmarshalJSON' to
> make it clear this is different from a normal 'UnmarshalJSON'
> method.
Fair point on avoiding parsing the JSON twice.
I still don't like the fact that we have to force clients to use a
non-standard API, or that the API for events has to be different from
that for unions. But Go won't let you add method implementations to
an interface, so we're kinda stuck.
The only alternative I can think of would be to define Event as
type Event struct {
Shutdown *ShutdownEvent
Reset*ResetEvent
// and so on
}
which wouldn't be too bad from client code, as you'd only have to
change from
e, err := EventFromJSON(in)
switch v := e.(type) {
case ShutdownEvent:
// do something with v
case ResetEvent:
// do something with v
// and so on
}
to
err := json.UnmarshalJSON(in, )
if e.Shutdown != nil {
// do something with e.Shutdown
} else if e.Reset != nil {
// do something with e.Reset
} // and so on
but that would mean each time we have to parse an event we'd end up
allocating room for ~50 pointers and only actually using one, which
is a massive waste.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
On Wed, Jul 06, 2022 at 10:48:06AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 06, 2022 at 10:37:54AM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 06, 2022 at 04:28:16AM -0500, Andrea Bolognani wrote:
> > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > tmp := jsonGuestPanicInformation{}
> > >
> > > err := json.Unmarshal(data, )
> > > if err != nil {
> > > return err
> > > }
> > >
> > > switch tmp.Discriminator {
> > > case "hyper-v":
> > > if tmp.HyperV == nil {
> > > return errors.New("...")
> > > }
> > > s.HyperV = tmp.HyperV
> > > case "s390":
> > > if tmp.S390 == nil {
> > > return errors.New("...")
> > > }
> > > s.S390 = tmp.S390
> > > }
> >
> > I'm not actually sure this works, because the first json.Unmarshal
> > call won't know which branch to try unmarhsalling. So it might be
> > unavoidable to parse twice. With the XML parser this wouldn't be
> > a problem as it has separated the parse phase and then fills the
> > struct after.
It does, because the struct it's filling with data
(jsonGuestPanicInformation) covers all possible cases. We then pick
only the part we care about and transfer it to the user-provided
return location.
> Right afer sending, I remember how this is supposed to be done. It
> involves use of 'json.RawMessage' eg examples at:
>
> https://pkg.go.dev/encoding/json#example-RawMessage-Unmarshal
>
> So it would look like:
>
>type GuestPanicInformation struct {
>HyperV *GuestPanicInformationHyperV
>S390 *GuestPanicInformationS390
>}
>
>type jsonGuestPanicInformation struct {
>Discriminator string `json:"type"`
>Payload *json.RawMessage
>}
>
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> var p *json.RawMesage
> var err error
> if s.HyperV != nil {
> d = "hyper-v"
> p, err = json.Marshal(s.HyperV)
> } else if s.S390 != nil {
> d = "s390"
> p, err = json.Marshal(s.S390)
> } else {
> err = fmt.Errorf("No payload defined")
> }
> if err != nil {
> return []byte{}, err
> }
>
> return json.Marshal(jsonGuestPanicInformation{d, p}), nil
> }
>
>func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
>tmp := jsonGuestPanicInformation{}
>
>err := json.Unmarshal(data, )
>if err != nil {
>return err
>}
>
>switch tmp.Discriminator {
>case "hyper-v":
>s.HyperV := GuestPanicInformationHyperV{}
>err := json.Unmarshal(tmp.Payload, s.HyperV)
>if err != nil {
> return err
>}
>case "s390":
>s.S390 := GuestPanicInformationS390{}
>err := json.Unmarshal(tmp.Payload, s.S390)
>if err != nil {
> return err
>}
>}
>
>return fmt.Errorf("Unknown type '%s'", tmp.Discriminator)
> }
I guess this version would work too, but I think it might still
perform more computation than the one I suggested?
json.RawMessage is a type alias for []byte, so I think the first call
to json.Unmarshal will have to go through the message to figure out
its structure and the contents of the discriminator field, then
tweak the original input so that only the part that hasn't been
consumed yet is returned. The second call to json.Marshal will then
parse that part, which means that the "inner" chunk of JSON ends up
being processed twice. In the approach I suggested, you go over the
entire JSON in one go.
Things might even out when you take into account allocating and
copying data around though. I'm not particularly interested in
splitting hair when it comes to the most efficient approach at this
point in time :) Knowing that we're not going through the entire JSON
twice is good enough.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
On Tue, Jul 05, 2022 at 05:35:26PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 08:45:30AM -0700, Andrea Bolognani wrote:
> > All this string manipulation looks sketchy. Is there some reason that
> > I'm not seeing preventing you for doing something like the untested
> > code below?
> >
> > func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> > if s.HyperV != nil {
> > type union struct {
> > Discriminator string `json:"type"`
> > HyperVGuestPanicInformationHyperV `json:"hyper-v"`
> > }
> > tmp := union {
> > Discriminator: "hyper-v",
> > HyperV:s.HyperV,
> > }
> > return json.Marshal(tmp)
> > } else if s.S390 != nil {
> > type union struct {
> > Discriminator string `json:"type"`
> > S390 GuestPanicInformationHyperV `json:"s390"`
> > }
> > tmp := union {
> > Discriminator: "s390",
> > S390: s.S390,
> > }
> > return json.Marshal(tmp)
> > }
> > return nil, errors.New("...")
> > }
>
> Using these dummy structs is the way I've approached the
> discriminated union issue in the libvirt Golang XML bindings
> and it works well. It is the bit I like the least, but it was
> the lesser of many evils, and on the plus side in the QEMU case
> it'll be auto-generated code.
It appears to be the standard way to approach the problem in Go. It
sort of comes naturally given how the APIs for marshal/unmarshal have
been defined.
> > > func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> > > type Alias GuestPanicInformation
> > > peek := struct {
> > > Alias
> > > Driver string `json:"type"`
> > > }{}
> > >
> > > if err := json.Unmarshal(data, ); err != nil {
> > > return err
> > > }
> > > *s = GuestPanicInformation(peek.Alias)
> > >
> > > switch peek.Driver {
> > >
> > > case "hyper-v":
> > > s.HyperV = new(GuestPanicInformationHyperV)
> > > if err := json.Unmarshal(data, s.HyperV); err != nil {
> > > s.HyperV = nil
> > > return err
> > > }
> > > case "s390":
> > > s.S390 = new(GuestPanicInformationS390)
> > > if err := json.Unmarshal(data, s.S390); err != nil {
> > > s.S390 = nil
> > > return err
> > > }
> > > }
> > > // Unrecognizer drivers are silently ignored.
> > > return nil
> >
> > This looks pretty reasonable, but since you're only using "peek" to
> > look at the discriminator you should be able to leave out the Alias
> > type entirely and perform the initial Unmarshal operation while
> > ignoring all other fields.
>
> Once you've defined the dummy structs for the Marshall case
> though, you might as well use them for Unmarshall too, so you're
> not parsing the JSON twice.
You're right, that is undesirable. What about something like this?
type GuestPanicInformation struct {
HyperV *GuestPanicInformationHyperV
S390 *GuestPanicInformationS390
}
type jsonGuestPanicInformation struct {
Discriminator string `json:"type"`
HyperV*GuestPanicInformationHyperV `json:"hyper-v"`
S390 *GuestPanicInformationS390 `json:"s390"`
}
func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
if (s.HyperV != nil && s.S390 != nil) ||
(s.HyperV == nil && s.S390 == nil) {
// client hasn't filled in the struct properly
return nil, errors.New("...")
}
tmp := jsonGuestPanicInformation{}
if s.HyperV != nil {
tmp.Discriminator = "hyper-v"
tmp.HyperV = s.HyperV
} else if s.S390 != nil {
tmp.Discriminator = "s390"
tmp.S390 = s.S390
}
return json.Marshal(tmp)
}
func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
tmp := jsonGuestPanicInformation{}
err := json.Unmarshal(data, )
if err != nil {
return err
}
switch tmp.Discriminator {
case "hyper-v":
if tmp.HyperV == nil {
Re: [RFC PATCH v2 5/8] qapi: golang: Generate qapi's event types in Go
return nil, errors.New(fmt.Sprintf("Failed to unmarshal: %s",
> string(data)))
> }
> event.Data.EventTimestamp = base.EventTimestamp
> return , nil
>
> // more cases here
> }
> return nil, errors.New("Failed to recognize event")
> }
While I understand the need to have some way to figure out exactly
what type of event we're looking at before being able to unmarshal
the JSON data, I don't like how we force users to go through a
non-standard API for it.
Here's how I think we should do it:
func GetEventType(data []byte) (Event, error) {
type event struct {
Name string `json:"event"`
}
tmp := event{}
if err := json.Unmarshal(data, ); err != nil {
return nil, errors.New(fmt.Sprintf("Failed to decode event:
%s", string(data)))
}
switch tmp.Name {
case "ACPI_DEVICE_OST":
return {}, nil
// more cases here
}
return nil, errors.New("Failed to recognize event")
}
func (s *AcpiDeviceOstEvent) UnmarshalJSON(data []byte) error {
type eventData struct {
Info ACPIOSTInfo `json:"info"`
}
type event struct {
Name string`json:"event"`
EventTimestamp Timestamp `json:"timestamp"`
Data eventData `json:"data"`
}
tmp := event{}
err := json.Unmarshal(data, )
if err != nil {
return err
}
if tmp.Name != "ACPI_DEVICE_OST" {
return errors.New("name")
}
s.EventTimestamp = tmp.EventTimestamp
s.Info = tmp.Data.Info
return nil
}
This way, client code can look like
in :=
`{"event":"ACPI_DEVICE_OST","timestamp":{"seconds":1265044230,"microseconds":450486},"data":{"info":{"device":"d1","slot":"0","slot-type":"DIMM","source":1,"status":0}}}`
e, err := qapi.GetEventType([]byte(in))
if err != nil {
panic(err)
}
// e is of type AcpiDeviceOstEvent
err = json.Unmarshal([]byte(in), )
if err != nil {
panic(err)
}
where only the first part (figuring out the type of the event) needs
custom APIs, and the remaining code looks just like your average JSON
handling.
Speaking of client code, in the commit message you have
> input := `{"event":"MEMORY_DEVICE_SIZE_CHANGE",` +
> `"timestamp":{"seconds":1588168529,"microseconds":201316},` +
>
> `"data":{"id":"vm0","size":1073741824,"qom-path":"/machine/unattached/device[2]"}}`
> e, err := UnmarshalEvent([]byte(input)
> if err != nil {
> panic(err)
> }
> if e.GetName() == `MEMORY_DEVICE_SIZE_CHANGE` {
> m := e.(*MemoryDeviceSizeChangeEvent)
> // m.QomPath == "/machine/unattached/device[2]"
> }
I don't think we should encourage the use of string comparison for
the purpose of going from a generic Event instance to a specific one:
a better way would be to use Go's type switch feature, such as
switch m := e.(type) {
case *MemoryDeviceSizeChangeEvent:
// m.QomPath == "/machine/unattached/device[2]"
}
In fact, I don't really see a reason to provide the Name() getter
outside of possibly diagnostics, and given the potential of it being
misused I would argue that it might be better not to have it.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 7/8] qapi: golang: Add CommandResult type to Go
On Fri, Jun 17, 2022 at 02:19:31PM +0200, Victor Toso wrote:
> +type EmptyCommandReturn struct {
> +CommandId string `json:"id,omitempty"`
> +Error *QapiError `json:"error,omitempty"`
> +Name string `json:"-"`
> +}
Do we need a specific type for this? Can't we just generate an
appropriately-named type for each of the commands that don't return
any data? It's not like we would have to write that code manually :)
> +func (r *EmptyCommandReturn) GetCommandName() string {
> +return r.Name
> +}
Just like Event.GetName() and Command.GetName(), I'm not convinced we
should have this.
Of course, all the comments about how marshalling and unmarshalling
are handled made for events also apply here.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 0/8] qapi: add generator for Golang interface
I've commented in detail to the single patches, just a couple of
additional points.
On Fri, Jun 17, 2022 at 02:19:24PM +0200, Victor Toso wrote:
> * 7) Flat structs by removing embed types. Discussion with Andrea
> Thread:
> https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg01590.html
>
> No one required it but I decided to give it a try. Major issue that
> I see with this approach is to have generated a few 'Base' structs
> that are now useless. Overall, less nested structs seems better to
> me. Opnions?
>
> Example:
> | /* This is now useless, should be removed? */
> | type InetSocketAddressBase struct {
> | Host string `json:"host"`
> | Port string `json:"port"`
> | }
Can we somehow keep track, in the generator, of types that are only
used as building blocks for other types, and prevent them from
showing up in the generated code?
Finally, looking at the repository containing the generated code I
see that the generated type are sorted by kind, e.g. all unions are
in a file, all events in another one and so on. I believe the
structure should match more closely that of the QAPI schema, so e.g.
block-related types should all go in one file, net-related types in
another one and so on.
Looking forward to the next iteration :)
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
On Fri, Jun 17, 2022 at 02:19:28PM +0200, Victor Toso wrote:
> qapi:
> | { 'union': 'SetPasswordOptions',
> | 'base': { 'protocol': 'DisplayProtocol',
> | 'password': 'str',
> | '*connected': 'SetPasswordAction' },
> | 'discriminator': 'protocol',
> | 'data': { 'vnc': 'SetPasswordOptionsVnc' } }
>
> go:
> | type SetPasswordOptions struct {
> | // Base fields
> | Password string `json:"password"`
> | Connected *SetPasswordAction `json:"connected,omitempty"`
> |
> | // Variants fields
> | Vnc *SetPasswordOptionsVnc `json:"-"`
> | }
Generated code:
> type GuestPanicInformation struct {
> // Variants fields
> HyperV *GuestPanicInformationHyperV `json:"-"`
> S390 *GuestPanicInformationS390 `json:"-"`
> }
>
> func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
> type Alias GuestPanicInformation
> alias := Alias(s)
> base, err := json.Marshal(alias)
> if err != nil {
> return nil, err
> }
>
> driver := ""
> payload := []byte{}
> if s.HyperV != nil {
> driver = "hyper-v"
> payload, err = json.Marshal(s.HyperV)
> } else if s.S390 != nil {
> driver = "s390"
> payload, err = json.Marshal(s.S390)
> }
>
> if err != nil {
> return nil, err
> }
>
> if len(base) == len("{}") {
> base = []byte("")
> } else {
> base = append([]byte{','}, base[1:len(base)-1]...)
> }
>
> if len(payload) == 0 || len(payload) == len("{}") {
> payload = []byte("")
> } else {
> payload = append([]byte{','}, payload[1:len(payload)-1]...)
> }
>
> result := fmt.Sprintf(`{"type":"%s"%s%s}`, driver, base, payload)
> return []byte(result), nil
All this string manipulation looks sketchy. Is there some reason that
I'm not seeing preventing you for doing something like the untested
code below?
func (s GuestPanicInformation) MarshalJSON() ([]byte, error) {
if s.HyperV != nil {
type union struct {
Discriminator string `json:"type"`
HyperVGuestPanicInformationHyperV `json:"hyper-v"`
}
tmp := union {
Discriminator: "hyper-v",
HyperV:s.HyperV,
}
return json.Marshal(tmp)
} else if s.S390 != nil {
type union struct {
Discriminator string `json:"type"`
S390 GuestPanicInformationHyperV `json:"s390"`
}
tmp := union {
Discriminator: "s390",
S390: s.S390,
}
return json.Marshal(tmp)
}
return nil, errors.New("...")
}
> func (s *GuestPanicInformation) UnmarshalJSON(data []byte) error {
> type Alias GuestPanicInformation
> peek := struct {
> Alias
> Driver string `json:"type"`
> }{}
>
> if err := json.Unmarshal(data, ); err != nil {
> return err
> }
> *s = GuestPanicInformation(peek.Alias)
>
> switch peek.Driver {
>
> case "hyper-v":
> s.HyperV = new(GuestPanicInformationHyperV)
> if err := json.Unmarshal(data, s.HyperV); err != nil {
> s.HyperV = nil
> return err
> }
> case "s390":
> s.S390 = new(GuestPanicInformationS390)
> if err := json.Unmarshal(data, s.S390); err != nil {
> s.S390 = nil
> return err
> }
> }
> // Unrecognizer drivers are silently ignored.
> return nil
This looks pretty reasonable, but since you're only using "peek" to
look at the discriminator you should be able to leave out the Alias
type entirely and perform the initial Unmarshal operation while
ignoring all other fields.
The comments made elsewhere about potentially being more strict and
rejecting invalid JSON also apply here.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v2 2/8] qapi: golang: Generate qapi's alternate types in Go
not reject that? Of course then the client
code will have to look like
if r.Definition != nil {
// do something with r.Definition
} else if r.Reference != nil {
// do something with r.Reference
} else {
// we don't recognize this - error out
}
but the same is going to be true for enums, events and everything
else that can be extended in a backwards-compatible fashion.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/2] docs: build-platforms: Clarify stance on minor releases and backports
On Wed, May 04, 2022 at 09:23:28AM +0100, Daniel P. Berrangé wrote: > On Wed, May 04, 2022 at 01:01:03AM -0700, Andrea Bolognani wrote: > > On Wed, Apr 20, 2022 at 09:18:47AM -0700, Andrea Bolognani wrote: > > > On Wed, Apr 20, 2022 at 05:15:08PM +0100, Daniel P. Berrangé wrote: > > > > On Wed, Apr 20, 2022 at 06:03:11PM +0200, Andrea Bolognani wrote: > > > > > These changes match those made in the following libvirt commits: > > > > > > > > > > 2ac78307af docs: Clarify our stance on backported packages > > > > > 78cffd450a docs: Spell out our policy concerning minor releases > > > > > > > > > > Since QEMU's platform support policy is based on libvirt's, it > > > > > makes sense to mirror these recent changes made to the latter. > > > > > > > > > > The policy is not altered significantly - we're simply spelling > > > > > out some rules that were likely already being implicitly > > > > > enforced. > > > > > > > > Indeed, I think that's basically defacto the case already. > > > > > > > > Reviewed-by: Daniel P. Berrangé > > > > > > Thanks! Are you going to bring these in through one of your trees, or > > > do I need to bug someone else so that they will pick them up? :) > > > > I see these haven't gone in yet. Anything I can/should do to make > > that happen? > > The tragedy of QEMU not having a central docs maintainer. I'll queue > this one for my next pull request. Still doesn't seem to have been merged. Not pressuring you or anything, just making sure it doesn't slip through the cracks :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 18, 2022 at 02:30:11PM +0200, Markus Armbruster wrote: > Victor Toso writes: > > IMHO, at this moment, qapi-go is targeting communicating with > > QEMU and handling multiple QEMU versions seems reasonable to me. > > It's targeting communicating in *QMP*. QMP is designed to support > communicating with a range of QEMU versions. Full compatibility is > promised for a narrow range. Outside that range, graceful degradation. > > *If* you want to widen the full compatibility range, do it in *QMP*. Or > do it on top of QEMU, e.g. in libvirt. > > > Perhaps libvirt can use qapi-go in the future or other generated > > interface. That would be cool. > > "Would be cool" and a dollar buys you a cup of bad coffee. > > Is it a good use of our limited resources? > > How much will it delay delivery of Go bindings compared to less > ambitious version? Yeah, this thread has basically branched to cover three topics: 1. what an MVP Go interface for QMP should look like; 2. how to make sure said interface uses pretty names; 3. how to make it work across multiple QEMU versions. All of these are important in the long run, but as far as I'm concerned only 1. is an actual blocker to making progress. If we get to the point where we can generate a reasonably complete and well-typed Go interface that can be used to communicate with a single version of QEMU, we should just plaster EXPERIMENTAL all over it and get it merged. Basically get the MVP done and then iterate over it in-tree rather than trying to get everything perfect from the start. Sounds reasonable? -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC 0/3] Introduce a new Qemu machine for RISC-V
On Mon, May 23, 2022 at 08:16:40PM -0700, Atish Patra wrote: > On Sun, May 22, 2022 at 10:59 PM Alistair Francis > wrote: > > On Wed, May 18, 2022 at 4:38 PM Atish Patra wrote: > > > 1. virt machine is not well documented and already bloated. There is > > > no specification for virt machine as such. > > > Putting restrictions after a certain release will lead to confusion. > > > 2. Do we support existing MMIO devices after that specific version or not > > > ? > > > > Yeah, so I guess this doesn't achieve the same outcome you want. I > > would say we would still include some MMIO devices, like UART for > > example. > > Why ? We can just rely on the pcie based uart (virtio-serial-pci or > serial-pci) should be enough. > The only MMIO devices that should be allowed are the ones that can't > be behind pcie. IIRC virtio-serial is initialized too late to catch messages produced very early by the firmware (and possibly the kernel), which means it's okay for regular usage but not when trying to debug an entire class of boot issues. Either way, it looks like you wouldn't be able to completely get rid of MMIO even if you introduced a new virt-pcie machine type. That's the same for the aarch64 virt machine. I agree with Dan that we should follow the example set by that architecture - it has worked out pretty well. If there is a desire to reduce the complexity of the "standard" machine type, can we just take the current virt machine type and rename it to something else? And have your simpler code take over the virt name? Sure, it will cause some pain in the short term, but the RISC-V ecosystem is still young enough for it to not be a deal breaker. -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Wed, May 11, 2022 at 04:50:35PM +0100, Daniel P. Berrangé wrote:
> This would lead to a situation where every struct is duplicated
> for every version, even though 90% of the time they'll be identical
> across multiple versions. This is not very ammenable to the desire
> to be able to dynamically choose per-command which version you
> want based on which version of QEMU you're connected to.
>
> ie
>
> var cmd Command
> if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> cmd = BlockResizeArguments{
>V700: {
> NodeName: node,
> Size: 1 * GiB
> }
> }
> } else {
> cmd = BlockResizeArguments{
>V520: {
> Device: dev,
> Size: 1 * GiB
> }
> }
> }
>
> And of course the HasVersion check is going to be different
> for each command that matters.
>
> Having said that, this perhaps shows the nested structs are
> overkill. We could have
>
> var cmd Command
> if qmp.HasVersion(qemu.Version(7, 0, 0)) {
> cmd = {
> NodeName: node,
> Size: 1 * GiB
> }
> } else {
> cmd = {
> Device: dev,
> Size: 1 * GiB
> }
> }
Right, making the decision at the import level would require adding a
level of indirection and make this kind of dynamic logic awkward.
We shouldn't force users to sprinkle version numbers everywhere
though, especially since different commands will change at different
points in time. It should be possible to do something like
if !qmp.HasAPI(600) {
panic("update QEMU")
}
cmd := { // really BlockResizeArguments520
Device: device,
Size: size,
}
or even
if !qmp.HasAPI(qmp.API.Latest) {
panic("update QEMU")
}
cmd := {
NodeName: nodeName,
Size: size,
}
Should be easy enough to achieve with type aliases.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 01:51:05PM +0100, Daniel P. Berrangé wrote:
> In 7.0.0 we can now generate
>
>type BlockResizeArguments struct {
>V500 *BlockResizeArgumentsV500
>V520 *BlockResizeArgumentsV520
>V700 *BlockResizeArgumentsV700
>}
>
>type BlockResizeArgumentsV500 struct {
>Device string
>Size int
>}
>
>type BlockResizeArgumentsV520 struct {
>Device *string
>NodeName *string
>Size int
>}
>
>type BlockResizeArgumentsV700 struct {
>NodeName string
>Size int
>}
>
> App can use the same as before, or switch to
>
> node := "nodedev0"
> cmd := BlockResizeArguments{
> V700: {
> NodeName: node,
> Size: 1 * GiB
> }
> }
This honestly looks pretty unwieldy.
If the application already knows it's targeting a specific version of
the QEMU API, which for the above code to make any sense it will have
to, couldn't it do something like
import qemu .../qemu/v700
at the beginning of the file and then use regular old
cmd := qemu.BlockResizeArguments{
NodeName: nodeName,
Size: size,
}
instead?
--
Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Mon, May 09, 2022 at 12:21:10PM +0200, Victor Toso wrote:
> On Tue, Apr 19, 2022 at 11:12:28AM -0700, Andrea Bolognani wrote:
> > Based on the example you have in the README and how commands are
> > defined, invoking (a simplified version of) the trace-event-get-state
> > command would look like
> >
> > cmd := Command{
> > Name: "trace-event-get-state",
> > Arg: TraceEventGetStateCommand{
> > Name: "qemu_memalign",
> > },
> > }
> > qmp_input, _ := json.Marshal()
> > // qmp_input now contains
> > //
> > {"execute":"trace-event-get-state","arguments":{"name":"qemu_memalign"}}
> > // do something with it
> >
> > qmp_output :=
> > ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
> > ret := cmd.GetReturnType()
> > _ = json.Unmarshal(qmp_output, )
> > // ret is a CommandResult instance whose Value member can be cast
> > // to a TraceEventInfo struct
> >
> > First of all, from an application's point of view there are way too
> > many steps involved:
>
> It can actually get worse. I've used a lot of nested struct to
> define a Base type for a given Type. In Go, If you try to
> initialize a Type that has a nested Struct, you'll need to use
> the nested struct Type as field name and this is too verbose.
>
> See https://github.com/golang/go/issues/29438 (merged with:
> https://github.com/golang/go/issues/12854)
>
> The main reason that I kept it is because it maps very well with
> the over-the-wire protocol.
Right, I had not realized how bad things really were :)
I've noticed the use of base types and while I didn't bring it up in
my initial message because the other concerns seemed of much higher
importance, I actually wondered whether we need to expose them to
users of the Go SDK.
I think we should flatten things. That's what happens with the C
generator already, for example in
struct InetSocketAddress {
/* Members inherited from InetSocketAddressBase: */
char *host;
char *port;
/* Own members: */
bool has_numeric;
bool numeric;
/* ... */
};
This representation mirrors the wire protocol perfectly, so I see no
reason not to adopt it. Am I missing something?
> > performing this operation should really be as
> > simple as
> >
> > ret, _ := qmp.TraceEventGetState("qemu_memalign")
> > // ret is a TraceEventInfo instance
> >
> > That's the end state we should be working towards.
> >
> > Of course that assumes that the "qmp" object knows where the
> > QMP socket is, knows how to talk the QMP protocol,
> > transparently deals with serializing and deserializing data...
> > Plus, in some case you might want to deal with the wire
> > transfer yourself in an application-specific manner. So it
> > makes sense to have the basic building blocks available and
> > then build the more ergonomic SDK on top of that - with only
> > the first part being in scope for this series.
>
> Right. Indeed, I thought a bit about what I want to fit into the
> code generator that will reside in QEMU and what we might want to
> develop on top of that.
>
> The goal for this series really is generating the data types that
> can be converted to/from QMP messages.
That's perfectly fine, and in fact I believe that splitting the whole
endeavor into three parts - QMP protocol implementation, QAPI types
serialization/deserialization, and a high-level SDK that gives easy
access to the previous two - is the best approach.
> > qmp_output :=
> > ([]byte)(`{"return":{"name":"qemu_memalign","state":"disabled"}}`)
> > ret := TraceEventInfo{}
> > _ = json.Unmarshal(qmp_output, )
> > // ret is a TraceEventInfo instance
> >
> > The advantages over the current implementation is that the compiler
> > will prevent you from doing something silly like passing the wrong
> > set of arguments to a commmand, and that the application has to
> > explicitly spell out what kind of object it expects to get as output.
>
> I think that, if we know all types that we can have at QAPI spec,
> the process of marshalling and unmarshalling should verify it.
> So, even if we don't change the expected interface as suggested,
> that work needs to be done. For some types, I've already did it,
> like for Unions and Alternate types.
>
> Example:
> https://gitlab.com/victortoso/qapi-go/-/blob/main/pkg/qapi/unions.go#L28
>
> This union type can have 4 values for the Any interface
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 10, 2022 at 10:52:34AM +0100, Daniel P. Berrangé wrote:
> On Mon, May 02, 2022 at 10:01:41AM -0400, Andrea Bolognani wrote:
> > Revised proposal for the annotation:
> >
> > ns:word-WORD-WoRD-123Word
>
> Ugly, but we should only need this in the fairly niche scenarios,
> so not too pain ful to add a handful of these:
>
> Essentially if have the schema use CamelCase with UPPERCASE
> acronyms, and declare two rules:
>
> 1. Split on every boundary from lower to upper
> 2. Acronym detection if there's a sequence of 3 uppercase
> letters, then split before the last uppercase.
That should cover most of the type names, but we're still going to
need to help the parser out when it comes to detecting acronyms in
other contexts, such as all instances of the word "VNC" below:
{ 'enum': 'DisplayProtocol',
'data': [ 'vnc', ... ] }
{ 'command': 'query-vnc-servers', ... }
{ 'event': 'VNC_DISCONNECTED', ... }
> QAuthZListPolicy
>
> Rule 1: QAuth + ZList + Policy
> Rule 2: QAuth + ZList + Policy
>
> so only the last case needs ns:QAuthZ-List-Policy annotation, whcih
> doesn't feel like a big burden
Note that in my proposal the ns: part would be used exactly for cases
like this one to separate the namespace part which, as you said in
your other reply, needs to be preserved when generating C code but
can be safely dropped when targeting a language that has actual
namespace support. So the annotation would look like
Q:AuthZ-List-Policy
in this specific case. The ns: part would be optional, as a namespace
is not embedded in most of the names.
It's also interesting to see how "AuthZ" is capitalized in various Go
modules that implement the concept:
https://pkg.go.dev/search?limit=50=symbol=authz
Most use "Authz" rather than "AuthZ". If we get to the point where
the only bit of weirdness is how we spell this specific word, I think
I'll be able to live with it :)
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/2] docs: build-platforms: Clarify stance on minor releases and backports
On Wed, Apr 20, 2022 at 09:18:47AM -0700, Andrea Bolognani wrote: > On Wed, Apr 20, 2022 at 05:15:08PM +0100, Daniel P. Berrangé wrote: > > On Wed, Apr 20, 2022 at 06:03:11PM +0200, Andrea Bolognani wrote: > > > These changes match those made in the following libvirt commits: > > > > > > 2ac78307af docs: Clarify our stance on backported packages > > > 78cffd450a docs: Spell out our policy concerning minor releases > > > > > > Since QEMU's platform support policy is based on libvirt's, it > > > makes sense to mirror these recent changes made to the latter. > > > > > > The policy is not altered significantly - we're simply spelling > > > out some rules that were likely already being implicitly > > > enforced. > > > > Indeed, I think that's basically defacto the case already. > > > > Reviewed-by: Daniel P. Berrangé > > Thanks! Are you going to bring these in through one of your trees, or > do I need to bug someone else so that they will pick them up? :) I see these haven't gone in yet. Anything I can/should do to make that happen? Note that 1/2 is no longer applicable, since 4a89bf188a31 has already fixed the issue in the meantime. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] qapi: Fix incorrect Since tags
On Tue, May 03, 2022 at 10:44:37AM +0200, Markus Armbruster wrote: > I posted a more complete fix as "[PATCH] qapi: Fix malformed "Since:" > section tags", and you even reviewed it :) Oh boy, I clearly need more coffee. Sorry for the noise :/ -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Tue, May 03, 2022 at 09:57:27AM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > > I still feel that 1) users of a language SDK will ideally not need to > > look at the QAPI schema or wire chatter too often > > I think the most likely point of contact is the QEMU QMP Reference > Manual. Note that there isn't anything preventing us from including the original QAPI name in the documentation for the corresponding Go symbol, or even a link to the reference manual. So we could make jumping from the Go API documentation, which is what a Go programmer will be looking at most of the time, to the QMP documentation pretty much effortless. > My point is: a name override feature like the one you propose needs to > be used with discipline and restraint. Adds to reviewers' mental load. > Needs to be worth it. I'm not saying it isn't, I'm just pointing out a > cost. Yeah, I get that. Note that I'm not suggesting it should be possible for a name to be completely overridden - I just want to make it possible for a human to provide the name parsing algorithm solutions to those problems it can't figure out on its own. We could prevent that feature from being misused by verifying that the symbol the annotation is attached to can be derived from the list of words provided. That way, something like # SOMEName (completely-DIFFERENT-name) would be rejected and we would avoid misuse. > Wild idea: assume all lower case, but keep a list of exceptions. That could actually work reasonably well for QEMU because we only need to handle correctly what's in the schema, not arbitrary input. There's always the risk of the list of exceptions getting out of sync with the needs of the schema, but there's similarly no guarantee that annotations are going to be introduced when they are necessary, so it's mostly a wash. The only slight advantage of the annotation approach would be that it might be easier to notice it being missing because it's close to the name it refers to, while the list of exceptions is tucked away in a script far away from it. > The QAPI schema language uses three naming styles: > > * lower-case-with-hyphens for command and member names > > Many names use upper case and '_'. See pragma command-name-exceptions > and member-name-exceptions. Looking at the output generated by Victor's WIP script, it looks like these are already handled as nicely as those that don't fall under any exception. > Some (many?) names lack separators between words (example: logappend). > > * UPPER_CASE_WITH_UNDERSCORE for event names > > * CamelCase for type names > > Capitalization of words is inconsistent in places (example: VncInfo > vs. DisplayReloadOptionsVNC). > > What style conversions will we need for Go? Any other conversions come > to mind? > > What problems do these conversions have? Go uses CamelCase for pretty much everything: types, methods, constants... There's one slight wrinkle, in that the case of the first letter decides whether it's going to be a PublicName or a privateName. We can't do anything about that, but it shouldn't really affect us that much because we'll want all QAPI names to be public. So the issues preventing us from producing a "perfect" Go API are 1. inconsistent capitalization in type names -> could be addressed by simply changing the schema, as type names do not travel on the wire 2. missing dashes in certain command/member names -> leads to Incorrectcamelcase. Kevin's work is supposed to address this 3. inability to know which parts of a lower-case-name or UPPER_CASE_NAME are acronyms or are otherwise supposed to be capitalized in a specific way -> leads to WeirdVncAndDbusCapitalization. There's currently no way, either implemented or planned, to avoid this In addition to these I'm also thinking that QKeyCode and all the QCrypto stuff should probably lose their prefixes. Note that 3 shouldn't be an issue for Rust and addressing 1 would actually make things worse for that language, because at the moment at least *some* of the types follow its expected naming rules :) > > Revised proposal for the annotation: > > > > ns:word-WORD-WoRD-123Word > > > > Words are always separated by dashes; "regular" words are entirely > > lowercase, while the presence of even a single uppercase letter in a > > word denotes the fact that its case should be preserved when the > > naming conventions of the target language allow that. > > Is a word always capitalized the same for a single target language? Or > could capitalization depend on context? I'm not aware of any language that would adopt more than a single style of capitalization, outside of course the obvious lower_case_name or UPPER_CASE_NAME scenarios where the original capitalization stops being relevant. -- Andrea Bolognani / Red Hat / Virtualization
[PATCH v2 3/8] qapi: Add missing separators between sections
This only affects readability. The generated documentation
doesn't change.
Signed-off-by: Andrea Bolognani
Reviewed-by: Markus Armbruster
---
qapi/block-core.json | 5 +
qapi/block.json | 1 +
qapi/crypto.json | 7 +++
qapi/machine.json| 1 +
qapi/migration.json | 4
5 files changed, 18 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b66494e8c5..34dae298ee 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1744,6 +1744,7 @@
# Since: 2.3
#
# Example:
+#
# -> { "execute": "blockdev-backup",
# "arguments": { "device": "src-id",
# "sync": "full",
@@ -2008,6 +2009,7 @@
# @on-target-error: the action to take on an error on the target,
# default 'report' (no limitations, since this applies to
# a different block device than @device).
+#
# @unmap: Whether to try to unmap target sectors where source has
# only zero. If true, and target unallocated sectors will read as zero,
# target image sectors will be unmapped; otherwise, zeroes will be
@@ -2029,6 +2031,7 @@
#When true, this job will automatically disappear from the
query
#list without user intervention.
#Defaults to true. (Since 3.1)
+#
# Since: 1.3
##
{ 'struct': 'DriveMirror',
@@ -2342,6 +2345,7 @@
#When true, this job will automatically disappear from the
query
#list without user intervention.
#Defaults to true. (Since 3.1)
+#
# Returns: nothing on success.
#
# Since: 2.6
@@ -4139,6 +4143,7 @@
# @throttle-group: the name of the throttle-group object to use. It
# must already exist.
# @file: reference to or definition of the data source block device
+#
# Since: 2.11
##
{ 'struct': 'BlockdevOptionsThrottle',
diff --git a/qapi/block.json b/qapi/block.json
index 3f100d4887..e0f7898ed1 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -105,6 +105,7 @@
#
# Returns: - Nothing on success
# - If @device is not a valid block device, DeviceNotFound
+#
# Notes:Ejecting a device with no media results in success
#
# Since: 0.14
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 1ec54c15ca..829e70a168 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -32,6 +32,7 @@
#
# @raw: raw bytes. When encoded in JSON only valid UTF-8 sequences can be used
# @base64: arbitrary base64 encoded binary data
+#
# Since: 2.6
##
{ 'enum': 'QCryptoSecretFormat',
@@ -51,6 +52,7 @@
# @sha384: SHA-384. (since 2.7)
# @sha512: SHA-512. (since 2.7)
# @ripemd160: RIPEMD-160. (since 2.7)
+#
# Since: 2.6
##
{ 'enum': 'QCryptoHashAlgorithm',
@@ -75,6 +77,7 @@
# @twofish-128: Twofish with 128 bit / 16 byte keys
# @twofish-192: Twofish with 192 bit / 24 byte keys
# @twofish-256: Twofish with 256 bit / 32 byte keys
+#
# Since: 2.6
##
{ 'enum': 'QCryptoCipherAlgorithm',
@@ -95,6 +98,7 @@
# @cbc: Cipher Block Chaining
# @xts: XEX with tweaked code book and ciphertext stealing
# @ctr: Counter (Since 2.8)
+#
# Since: 2.6
##
{ 'enum': 'QCryptoCipherMode',
@@ -114,6 +118,7 @@
# @plain: 64-bit sector number truncated to 32-bits
# @plain64: 64-bit sector number
# @essiv: 64-bit sector number encrypted with a hash of the encryption key
+#
# Since: 2.6
##
{ 'enum': 'QCryptoIVGenAlgorithm',
@@ -170,6 +175,7 @@
# @key-secret: the ID of a QCryptoSecret object providing the
# decryption key. Mandatory except when probing image for
# metadata only.
+#
# Since: 2.6
##
{ 'struct': 'QCryptoBlockOptionsLUKS',
@@ -194,6 +200,7 @@
# @iter-time: number of milliseconds to spend in
# PBKDF passphrase processing. Currently defaults
# to 2000. (since 2.8)
+#
# Since: 2.6
##
{ 'struct': 'QCryptoBlockCreateOptionsLUKS',
diff --git a/qapi/machine.json b/qapi/machine.json
index d25a481ce4..9ec17b3992 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -299,6 +299,7 @@
#returning does not indicate that a guest has accepted the request or
#that it has shut down. Many guests will respond to this command by
#prompting the user in some way.
+#
# Example:
#
# -> { "execute": "system_powerdown" }
diff --git a/qapi/migration.json b/qapi/migration.json
index 409eb086a2..fc1c157d3f 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1422,7 +1422,9 @@
# @state: The state the migration is currently expected to be in
#
# Returns: nothing on success
+#
# Since: 2.11
+#
# Example:
#
# -> { "execute": "migrate-continue" , "arguments":
@@ -1736,6 +1738,7 @@
# Since: 4.2
#
# Example:
+#
# <- { "event": "UNPLUG_PRIMARY",
# "data": { "device-id": "hostdev0" },
# &
[PATCH] qapi: Fix incorrect Since tags
The missing colon causes them to be interpreted as regular
text.
Signed-off-by: Andrea Bolognani
---
qapi/crypto.json | 2 +-
qapi/misc.json | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 1ec54c15ca..529aa730d4 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -358,7 +358,7 @@
# Defaults to the same secret that was used to open the image
#
#
-# Since 5.1
+# Since: 5.1
##
{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
'data': { 'state': 'QCryptoBlockLUKSKeyslotState',
diff --git a/qapi/misc.json b/qapi/misc.json
index b83cc39029..f8a9feda30 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -188,7 +188,7 @@
# Features:
# @unstable: This command is experimental.
#
-# Since 3.0
+# Since: 3.0
#
# Returns: nothing
#
--
2.35.1
Re: [PATCH 6/7] qapi: Drop unnecessary horizontal spacing in comments
On Mon, May 02, 2022 at 07:24:53PM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > > On Mon, May 02, 2022 at 10:50:07AM +0200, Markus Armbruster wrote: > >> I doubt changing to a different alignment now is useful. The next > >> patch, however, drops the alignment entirely. Possibly useful. > >> > >> Thoughts? > > > > My rationale for splitting things the way I did is that, if dropping > > the horizontal alignment entirely was not considered desirable, we > > could at least get rid of the extra whitespace. > > Understood. > > > But if you think that > > the benefit from the half measure doesn't offset the cost of the > > churn it causes, I'm happy to drop these hunks and go straight from > > the current status to no horizontal alignment at all in one fell > > swoop with the next patch. > > Show us the patches, and then we can decide whether the improvement is > worth the churn. The way things are split in the respin should allow us to pick up the obviously desirable changes and then decide how far we want to go with the rest. -- Andrea Bolognani / Red Hat / Virtualization
[PATCH v2 5/8] qapi: Drop unnecessary empty lines outside of comments
Signed-off-by: Andrea Bolognani
Reviewed-by: Markus Armbruster
---
qapi/audio.json | 1 -
qapi/block-core.json | 11 ---
qapi/block.json | 3 ---
qapi/char.json | 1 -
qapi/control.json| 1 -
qapi/crypto.json | 12
qapi/job.json| 1 -
qapi/machine-target.json | 1 -
qapi/machine.json| 1 -
qapi/misc-target.json| 4
qapi/run-state.json | 1 -
qapi/ui.json | 1 -
12 files changed, 38 deletions(-)
diff --git a/qapi/audio.json b/qapi/audio.json
index 0785e70a50..8099e3d7f1 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -352,7 +352,6 @@
'*out': 'AudiodevPerDirectionOptions',
'*path': 'str' } }
-
##
# @AudioFormat:
#
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 27832a1244..2bce5bb0ae 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -739,7 +739,6 @@
##
{ 'command': 'query-block', 'returns': ['BlockInfo'] }
-
##
# @BlockDeviceTimedStats:
#
@@ -1512,7 +1511,6 @@
{ 'command': 'blockdev-snapshot-sync',
'data': 'BlockdevSnapshotSync' }
-
##
# @blockdev-snapshot:
#
@@ -1751,7 +1749,6 @@
{ 'command': 'blockdev-backup', 'boxed': true,
'data': 'BlockdevBackup' }
-
##
# @query-named-block-nodes:
#
@@ -3067,7 +3064,6 @@
'base': 'BlockdevOptionsGenericFormat',
'data': { '*key-secret': 'str' } }
-
##
# @BlockdevOptionsGenericCOWFormat:
#
@@ -3182,8 +3178,6 @@
'base': 'BlockdevOptionsGenericCOWFormat',
'data': { '*encrypt': 'BlockdevQcowEncryption' } }
-
-
##
# @BlockdevQcow2EncryptionFormat:
#
@@ -3357,7 +3351,6 @@
'*user': 'str',
'*host-key-check': 'SshHostKeyCheck' } }
-
##
# @BlkdebugEvent:
#
@@ -3721,7 +3714,6 @@
'*header-digest': 'IscsiHeaderDigest',
'*timeout': 'int' } }
-
##
# @RbdAuthMode:
#
@@ -4564,7 +4556,6 @@
{ 'enum': 'BlockdevQcow2Version',
'data': [ 'v2', 'v3' ] }
-
##
# @Qcow2CompressionType:
#
@@ -4738,7 +4729,6 @@
'*toolsversion':'str',
'*zeroed-grain':'bool' } }
-
##
# @BlockdevCreateOptionsSsh:
#
@@ -4973,7 +4963,6 @@
{ 'enum': 'BlockErrorAction',
'data': [ 'ignore', 'report', 'stop' ] }
-
##
# @BLOCK_IMAGE_CORRUPTED:
#
diff --git a/qapi/block.json b/qapi/block.json
index 5de15c6070..41b73c9934 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -286,7 +286,6 @@
'data': { 'id': 'str',
'node-name': 'str'} }
-
##
# @BlockdevChangeReadOnlyMode:
#
@@ -304,7 +303,6 @@
{ 'enum': 'BlockdevChangeReadOnlyMode',
'data': ['retain', 'read-only', 'read-write'] }
-
##
# @blockdev-change-medium:
#
@@ -375,7 +373,6 @@
'*force': 'bool',
'*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
-
##
# @DEVICE_TRAY_MOVED:
#
diff --git a/qapi/char.json b/qapi/char.json
index f0fd0d1c9f..8414ef2bc2 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -329,7 +329,6 @@
'data': { '*signal': 'bool' },
'base': 'ChardevCommon' }
-
##
# @ChardevSpiceChannel:
#
diff --git a/qapi/control.json b/qapi/control.json
index 8c9122ef7a..53461cec05 100644
--- a/qapi/control.json
+++ b/qapi/control.json
@@ -68,7 +68,6 @@
{ 'struct': 'VersionTriple',
'data': {'major': 'int', 'minor': 'int', 'micro': 'int'} }
-
##
# @VersionInfo:
#
diff --git a/qapi/crypto.json b/qapi/crypto.json
index aebe390ab7..ff33e1fe1f 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -24,7 +24,6 @@
'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT',
'data': ['client', 'server']}
-
##
# @QCryptoSecretFormat:
#
@@ -39,7 +38,6 @@
'prefix': 'QCRYPTO_SECRET_FORMAT',
'data': ['raw', 'base64']}
-
##
# @QCryptoHashAlgorithm:
#
@@ -59,7 +57,6 @@
'prefix': 'QCRYPTO_HASH_ALG',
'data': ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512', 'ripemd160']}
-
##
# @QCryptoCipherAlgorithm:
#
@@ -88,7 +85,6 @@
'serpent-128', 'serpent-192', 'serpent-256',
'twofish-128', 'twofish-192', 'twofish-256']}
-
##
# @QCryptoCipherMode:
#
@@ -105,7 +101,6 @@
'prefix': 'QCRYPTO_CIPHER_MODE',
'data': ['ecb', 'cbc', 'xts', 'ctr']}
-
##
# @QCryptoIVGenAlgorithm:
#
@@ -181,7 +176,6 @@
{ 'struct': 'QCryptoBlockOptionsLUKS',
'data': { '*key-secret': 'str' }}
-
##
# @QCryptoBlockCreateOptionsLUKS:
#
@@ -212,7 +206,6 @@
'*hash-alg': 'QCryptoHashAlgorithm',
'*iter-time': 'int'}}
-
##
# @QCryptoBlockOpenOptions:
#
@@ -227,7 +220,6 @@
'data': { 'qcow': 'QCryptoBlockOptionsQCow',
'luks': 'QCryptoBlockOptionsLUKS' } }
-
##
# @QCryptoBlockCreateOptions:
#
@@ -242,7 +234,6 @@
'data': { 'qcow': 'QCryptoBlockOptionsQCow',
'luks': 'QCryptoBlockCreateOptionsLUKS' } }
-
##
# @QCryptoBlockInfoBase:
#
@@ -256,7 +247,6 @@
{ 'struct': 'QCryptoBlockInfoBase',
'data': { 'format': 'QCryptoBlockFormat' }}
-
##
# @QCryptoBlockInfoLUKSSlot:
#
@@ -276,7
[PATCH v2 2/8] qapi: Fix comment indentation
It should start on the very first column.
Signed-off-by: Andrea Bolognani
Reviewed-by: Markus Armbruster
---
qapi/ui.json | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/qapi/ui.json b/qapi/ui.json
index 059302a5ef..43e62efd76 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1250,21 +1250,21 @@
'*p2p': 'bool',
'*audiodev': 'str' } }
- ##
- # @DisplayGLMode:
- #
- # Display OpenGL mode.
- #
- # @off: Disable OpenGL (default).
- # @on: Use OpenGL, pick context type automatically.
- # Would better be named 'auto' but is called 'on' for backward
- # compatibility with bool type.
- # @core: Use OpenGL with Core (desktop) Context.
- # @es: Use OpenGL with ES (embedded systems) Context.
- #
- # Since: 3.0
- #
- ##
+##
+# @DisplayGLMode:
+#
+# Display OpenGL mode.
+#
+# @off: Disable OpenGL (default).
+# @on: Use OpenGL, pick context type automatically.
+# Would better be named 'auto' but is called 'on' for backward
+# compatibility with bool type.
+# @core: Use OpenGL with Core (desktop) Context.
+# @es: Use OpenGL with ES (embedded systems) Context.
+#
+# Since: 3.0
+#
+##
{ 'enum': 'DisplayGLMode',
'data': [ 'off', 'on', 'core', 'es' ] }
--
2.35.1
[PATCH v2 0/8] qapi: Primarily whitespace tweaks
If patch 8/8 is accepted, 7/8 should be squashed into it to reduce churn. Changes from [v1]: * use a more fine-grained split for whitespace changes. [v1] https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg05406.html Andrea Bolognani (8): qapi: Drop stray trailing symbol qapi: Fix comment indentation qapi: Add missing separators between sections qapi: Drop unnecessary empty lines in comments qapi: Drop unnecessary empty lines outside of comments qapi: Drop unnecessary whitespace in comments qapi: Reduce whitespace used for alignment in comments qapi: Stop using whitespace for alignment in comments qapi/audio.json | 1 - qapi/block-core.json | 97 ++-- qapi/block-export.json | 2 +- qapi/block.json | 13 +++--- qapi/char.json | 10 ++--- qapi/common.json | 2 - qapi/control.json| 13 +++--- qapi/crypto.json | 62 - qapi/dump.json | 4 +- qapi/job.json| 1 - qapi/machine-target.json | 1 - qapi/machine.json| 12 +++-- qapi/migration.json | 19 qapi/misc-target.json| 13 ++ qapi/misc.json | 6 +-- qapi/replay.json | 1 - qapi/run-state.json | 10 ++--- qapi/sockets.json| 6 +-- qapi/ui.json | 70 ++--- 19 files changed, 136 insertions(+), 207 deletions(-) -- 2.35.1
[PATCH v2 4/8] qapi: Drop unnecessary empty lines in comments
Signed-off-by: Andrea Bolognani
Reviewed-by: Markus Armbruster
---
qapi/block-core.json | 4
qapi/block.json | 1 -
qapi/char.json| 1 -
qapi/common.json | 2 --
qapi/control.json | 2 --
qapi/crypto.json | 1 -
qapi/machine.json | 2 --
qapi/migration.json | 7 ---
qapi/misc-target.json | 3 ---
qapi/replay.json | 1 -
qapi/run-state.json | 3 ---
qapi/ui.json | 22 --
12 files changed, 49 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 34dae298ee..27832a1244 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -237,7 +237,6 @@
# information (since 1.7)
#
# Since: 1.3
-#
##
{ 'struct': 'ImageInfo',
'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
@@ -288,7 +287,6 @@
# supports it
#
# Since: 1.4
-#
##
{ 'struct': 'ImageCheck',
'data': {'filename': 'str', 'format': 'str', 'check-errors': 'int',
@@ -328,7 +326,6 @@
# @filename: filename that is referred to by @offset
#
# Since: 2.6
-#
##
{ 'struct': 'MapEntry',
'data': {'start': 'int', 'length': 'int', 'data': 'bool',
@@ -445,7 +442,6 @@
# has one or more dirty bitmaps) (Since 4.2)
#
# Since: 0.14
-#
##
{ 'struct': 'BlockDeviceInfo',
'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': 'str',
diff --git a/qapi/block.json b/qapi/block.json
index e0f7898ed1..5de15c6070 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -300,7 +300,6 @@
# @read-write: Makes the device writable
#
# Since: 2.3
-#
##
{ 'enum': 'BlockdevChangeReadOnlyMode',
'data': ['retain', 'read-only', 'read-write'] }
diff --git a/qapi/char.json b/qapi/char.json
index 7b42151575..f0fd0d1c9f 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -413,7 +413,6 @@
# @clipboard: enable/disable clipboard, default is disabled.
#
# Since: 6.1
-#
##
{ 'struct': 'ChardevQemuVDAgent',
'data': { '*mouse': 'bool',
diff --git a/qapi/common.json b/qapi/common.json
index 412cc4f5ae..356db3f670 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -192,7 +192,6 @@
# Keys to toggle input-linux between host and guest.
#
# Since: 4.0
-#
##
{ 'enum': 'GrabToggleKeys',
'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
@@ -204,7 +203,6 @@
# @human-readable-text: Formatted output intended for humans.
#
# Since: 6.2
-#
##
{ 'struct': 'HumanReadableText',
'data': { 'human-readable-text': 'str' } }
diff --git a/qapi/control.json b/qapi/control.json
index 71a838d49e..8c9122ef7a 100644
--- a/qapi/control.json
+++ b/qapi/control.json
@@ -33,7 +33,6 @@
#all the QMP capabilities will be turned off by default.
#
# Since: 0.13
-#
##
{ 'command': 'qmp_capabilities',
'data': { '*enable': [ 'QMPCapability' ] },
@@ -49,7 +48,6 @@
# (Please refer to qmp-spec.txt for more information on OOB)
#
# Since: 2.12
-#
##
{ 'enum': 'QMPCapability',
'data': [ 'oob' ] }
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 829e70a168..aebe390ab7 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -364,7 +364,6 @@
# password to use to retrieve current master key.
# Defaults to the same secret that was used to open the image
#
-#
# Since 5.1
##
{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
diff --git a/qapi/machine.json b/qapi/machine.json
index 9ec17b3992..20b1f0c748 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -77,7 +77,6 @@
# additional fields will be listed (since 3.0)
#
# Since: 2.12
-#
##
{ 'union' : 'CpuInfoFast',
'base' : { 'cpu-index': 'int',
@@ -1020,7 +1019,6 @@
# Formula used: logical_vm_size = vm_ram_size - balloon_size
#
# Since: 0.14
-#
##
{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
diff --git a/qapi/migration.json b/qapi/migration.json
index fc1c157d3f..dd4dde6361 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -151,7 +151,6 @@
# (since 4.2)
#
# Since: 2.3
-#
##
{ 'enum': 'MigrationStatus',
'data': [ 'none', 'setup', 'cancelling', 'cancelled',
@@ -166,7 +165,6 @@
# @transferred: amount of bytes transferred to the target VM by VFIO devices
#
# Since: 5.2
-#
##
{ 'struct': 'VfioStats',
'data': {'transferred': 'int' } }
@@ -546,7 +544,6 @@
# @zstd: use zstd compression method.
#
# Since: 5.0
-#
##
{ 'enum': 'MultiFDCompression',
'data': [ 'none', 'zlib',
@@ -1757,7 +1754,6 @@
# @dirty-rate: dirty rate.
#
# Since: 6.2
-#
##
{ 'struct': 'DirtyRateVcpu',
'data': { 'id': 'int', 'dirty-rate': 'int64' } }
@@ -1774,7 +1770,6 @@
# @measured: the dirtyrate thread has measured and results are available.
#
# Since: 5.2
-#
##
{ 'enum': 'DirtyRateStatus',
'data': [ 'unstarted', 'measuring', 'measured'] }
@@ -1791,7 +1786,6 @@
# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
#
# Since: 6.2
-#
##
{ 'enum
[PATCH v2 8/8] qapi: Stop using whitespace for alignment in comments
Perfectly aligned things look pretty, but keeping them that way as the schema evolves requires churn, and in some cases newly-added lines are not aligned properly. Overall, trying to align things is just not worth the trouble. Signed-off-by: Andrea Bolognani --- qapi/block-core.json | 43 +-- qapi/block.json | 6 +++--- qapi/char.json | 6 +++--- qapi/control.json| 6 +++--- qapi/crypto.json | 30 +++--- qapi/migration.json | 8 qapi/sockets.json| 4 ++-- qapi/ui.json | 13 ++--- 8 files changed, 57 insertions(+), 59 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 5fd66ea676..f0383c7925 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -338,8 +338,8 @@ # Cache mode information for a block device # # @writeback: true if writeback mode is enabled -# @direct:true if the host page cache is bypassed (O_DIRECT) -# @no-flush: true if flush requests are ignored for the device +# @direct: true if the host page cache is bypassed (O_DIRECT) +# @no-flush: true if flush requests are ignored for the device # # Since: 2.3 ## @@ -,12 +,11 @@ ## # @BlockdevOptionsSsh: # -# @server: host address +# @server: host address # -# @path: path to the image on the host +# @path: path to the image on the host # -# @user: user as which to connect, defaults to current -# local user name +# @user: user as which to connect, defaults to current local user name # # @host-key-check: Defines how and what to check the host key against # (default: known_hosts) @@ -4662,18 +4661,18 @@ # # Subformat options for VMDK images # -# @monolithicSparse: Single file image with sparse cluster allocation +# @monolithicSparse: Single file image with sparse cluster allocation # -# @monolithicFlat: Single flat data image and a descriptor file +# @monolithicFlat: Single flat data image and a descriptor file # # @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent #files, in addition to a descriptor file # -# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent -#files, in addition to a descriptor file +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent +# files, in addition to a descriptor file # -# @streamOptimized: Single file image sparse cluster allocation, optimized -#for streaming over network. +# @streamOptimized: Single file image sparse cluster allocation, optimized +# for streaming over network. # # Since: 4.0 ## @@ -4764,7 +4763,7 @@ # @BlockdevVhdxSubformat: # # @dynamic: Growing image file -# @fixed: Preallocated fixed-size image file +# @fixed: Preallocated fixed-size image file # # Since: 2.12 ## @@ -4802,7 +4801,7 @@ # @BlockdevVpcSubformat: # # @dynamic: Growing image file -# @fixed: Preallocated fixed-size image file +# @fixed: Preallocated fixed-size image file # # Since: 2.12 ## @@ -4865,7 +4864,7 @@ # Starts a job to create an image format on a given node. The job is # automatically finalized, but a manual job-dismiss is required. # -# @job-id: Identifier for the newly created job. +# @job-id: Identifier for the newly created job. # # @options: Options for the image creation. # @@ -4923,17 +4922,17 @@ # Starts a job to amend format specific options of an existing open block device # The job is automatically finalized, but a manual job-dismiss is required. # -# @job-id:Identifier for the newly created job. +# @job-id: Identifier for the newly created job. # # @node-name: Name of the block node to work on # -# @options: Options (driver specific) +# @options: Options (driver specific) # -# @force: Allow unsafe operations, format specific -# For luks that allows erase of the last active keyslot -# (permanent loss of data), -# and replacement of an active keyslot -# (possible loss of data if IO error happens) +# @force: Allow unsafe operations, format specific +# For luks that allows erase of the last active keyslot +# (permanent loss of data), +# and replacement of an active keyslot +# (possible loss of data if IO error happens) # # Features: # @unstable: This command is experimental. diff --git a/qapi/block.json b/qapi/block.json index 96f557b3bb..19326641ac 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -50,9 +50,9 @@ # # Type of Floppy drive to be emulated by the Floppy Disk Controller. # -# @144: 1.44MB 3.5" drive -# @288: 2.88MB 3.5" drive -# @120: 1.2MB 5.25" drive +# @144: 1.44MB 3.5" drive +# @288: 2.88MB 3.5" drive +# @120: 1.2MB 5.25" drive # @none: No drive connected # @auto: Automatically determined by
[PATCH v2 7/8] qapi: Reduce whitespace used for alignment in comments
Use the minimum number of spaces necessary. Signed-off-by: Andrea Bolognani --- qapi/block-core.json | 38 +++--- qapi/control.json| 10 +- qapi/crypto.json | 42 +- qapi/ui.json | 16 4 files changed, 53 insertions(+), 53 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index e110af2f1d..5fd66ea676 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -337,9 +337,9 @@ # # Cache mode information for a block device # -# @writeback: true if writeback mode is enabled -# @direct: true if the host page cache is bypassed (O_DIRECT) -# @no-flush:true if flush requests are ignored for the device +# @writeback: true if writeback mode is enabled +# @direct:true if the host page cache is bypassed (O_DIRECT) +# @no-flush: true if flush requests are ignored for the device # # Since: 2.3 ## @@ -,15 +,15 @@ ## # @BlockdevOptionsSsh: # -# @server: host address +# @server: host address # -# @path:path to the image on the host +# @path: path to the image on the host # -# @user:user as which to connect, defaults to current -# local user name +# @user: user as which to connect, defaults to current +# local user name # -# @host-key-check: Defines how and what to check the host key against -# (default: known_hosts) +# @host-key-check: Defines how and what to check the host key against +# (default: known_hosts) # # Since: 2.9 ## @@ -4865,9 +4865,9 @@ # Starts a job to create an image format on a given node. The job is # automatically finalized, but a manual job-dismiss is required. # -# @job-id: Identifier for the newly created job. +# @job-id: Identifier for the newly created job. # -# @options: Options for the image creation. +# @options: Options for the image creation. # # Since: 3.0 ## @@ -4923,17 +4923,17 @@ # Starts a job to amend format specific options of an existing open block device # The job is automatically finalized, but a manual job-dismiss is required. # -# @job-id: Identifier for the newly created job. +# @job-id:Identifier for the newly created job. # -# @node-name: Name of the block node to work on +# @node-name: Name of the block node to work on # -# @options: Options (driver specific) +# @options: Options (driver specific) # -# @force: Allow unsafe operations, format specific -# For luks that allows erase of the last active keyslot -# (permanent loss of data), -# and replacement of an active keyslot -# (possible loss of data if IO error happens) +# @force: Allow unsafe operations, format specific +# For luks that allows erase of the last active keyslot +# (permanent loss of data), +# and replacement of an active keyslot +# (possible loss of data if IO error happens) # # Features: # @unstable: This command is experimental. diff --git a/qapi/control.json b/qapi/control.json index 53461cec05..7107f55db3 100644 --- a/qapi/control.json +++ b/qapi/control.json @@ -192,14 +192,14 @@ # # Options to be used for adding a new monitor. # -# @id: Name of the monitor +# @id: Name of the monitor # -# @mode:Selects the monitor mode (default: readline in the system -# emulator, control in qemu-storage-daemon) +# @mode:Selects the monitor mode (default: readline in the system +# emulator, control in qemu-storage-daemon) # -# @pretty: Enables pretty printing (QMP only) +# @pretty: Enables pretty printing (QMP only) # -# @chardev: Name of a character device to expose the monitor on +# @chardev: Name of a character device to expose the monitor on # # Since: 5.0 ## diff --git a/qapi/crypto.json b/qapi/crypto.json index ff33e1fe1f..64e2ce81f1 100644 --- a/qapi/crypto.json +++ b/qapi/crypto.json @@ -311,8 +311,8 @@ # # Defines state of keyslots that are affected by the update # -# @active:The slots contain the given password and marked as active -# @inactive: The slots are erased (contain garbage) and marked as inactive +# @active: The slots contain the given password and marked as active +# @inactive: The slots are erased (contain garbage) and marked as inactive # # Since: 5.1 ## @@ -327,30 +327,30 @@ # # @state: the desired state of the keyslots # -# @new-secret:The ID of a QCryptoSecret object providing the password to be -# written into added active keyslots +# @new-secret: The ID of a QCryptoSecret object providing the password to be +# written into added active keyslots # -# @old-secret:Optional (for deactivation only) -# If given will deactivate all keyslots
[PATCH v2 6/8] qapi: Drop unnecessary whitespace in comments
The only instances that get changed are those in which the
additional whitespace was not (or couldn't possibly be) used for
alignment purposes.
Signed-off-by: Andrea Bolognani
---
qapi/block-core.json | 24
qapi/block-export.json | 2 +-
qapi/block.json| 2 +-
qapi/char.json | 2 +-
qapi/dump.json | 4 ++--
qapi/machine.json | 8
qapi/misc-target.json | 6 +++---
qapi/misc.json | 6 +++---
qapi/run-state.json| 4 ++--
qapi/sockets.json | 2 +-
qapi/ui.json | 2 +-
11 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2bce5bb0ae..e110af2f1d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -604,7 +604,7 @@
# @inserted: @BlockDeviceInfo describing the device if media is
#present
#
-# Since: 0.14
+# Since: 0.14
##
{ 'struct': 'BlockInfo',
'data': {'device': 'str', '*qdev': 'str', 'type': 'str', 'removable': 'bool',
@@ -795,9 +795,9 @@
#
# Statistics of a virtual block device or a block backing device.
#
-# @rd_bytes: The number of bytes read by the device.
+# @rd_bytes: The number of bytes read by the device.
#
-# @wr_bytes: The number of bytes written by the device.
+# @wr_bytes: The number of bytes written by the device.
#
# @unmap_bytes: The number of bytes unmapped by the device (Since 4.2)
#
@@ -970,7 +970,7 @@
# @qdev: The qdev ID, or if no ID is assigned, the QOM path of the block
#device. (since 3.0)
#
-# @stats: A @BlockDeviceStats for the device.
+# @stats: A @BlockDeviceStats for the device.
#
# @driver-specific: Optional driver-specific stats. (Since 4.2)
#
@@ -1275,7 +1275,7 @@
#
# @node-name: graph node name to get the image resized (Since 2.0)
#
-# @size: new image size in bytes
+# @size: new image size in bytes
#
# Returns: - nothing on success
# - If @device is not a valid block device, DeviceNotFound
@@ -1960,8 +1960,8 @@
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
-# @device: the device name or node-name of a root node whose writes should be
-# mirrored.
+# @device: the device name or node-name of a root node whose writes should be
+# mirrored.
#
# @target: the target of the new image. If the file exists, or if it
# is a device, the existing file/device will be used as the new
@@ -1981,7 +1981,7 @@
# @mode: whether and how QEMU should create a new image, default is
#'absolute-paths'.
#
-# @speed: the maximum speed, in bytes per second
+# @speed: the maximum speed, in bytes per second
#
# @sync: what parts of the disk image should be copied to the destination
#(all the disk, only the sectors allocated in the topmost image, or
@@ -2296,7 +2296,7 @@
#broken Quorum files. By default, @device is replaced, although
#implicitly created filters on it are kept.
#
-# @speed: the maximum speed, in bytes per second
+# @speed: the maximum speed, in bytes per second
#
# @sync: what parts of the disk image should be copied to the destination
#(all the disk, only the sectors allocated in the topmost image, or
@@ -4548,8 +4548,8 @@
##
# @BlockdevQcow2Version:
#
-# @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
-# @v3: The extended QCOW2 format as introduced in qemu 1.1 (version 3)
+# @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
+# @v3: The extended QCOW2 format as introduced in qemu 1.1 (version 3)
#
# Since: 2.12
##
@@ -4905,7 +4905,7 @@
#
# Options for amending an image format
#
-# @driver: Block driver of the node to amend.
+# @driver: Block driver of the node to amend.
#
# Since: 5.1
##
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 1de16d2589..53013b03ff 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -387,7 +387,7 @@
# block-export-del command, but before the shutdown has
# completed)
#
-# Since: 5.2
+# Since: 5.2
##
{ 'struct': 'BlockExportInfo',
'data': { 'id': 'str',
diff --git a/qapi/block.json b/qapi/block.json
index 41b73c9934..96f557b3bb 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -106,7 +106,7 @@
# Returns: - Nothing on success
# - If @device is not a valid block device, DeviceNotFound
#
-# Notes:Ejecting a device with no media results in success
+# Notes: Ejecting a device with no media results in success
#
# Since: 0.14
#
diff --git a/qapi/char.json b/qapi/char.json
index 8414ef2bc2..a40fe4b7bd 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -216,7 +216,7 @@
#
# Configuration info for file chardevs.
#
-# @in: The name of the input file
+# @in: The name of the input file
# @out: The name of the output file
# @append: Open the file in append mode (default false to
# truncate
[PATCH v2 1/8] qapi: Drop stray trailing symbol
Signed-off-by: Andrea Bolognani Reviewed-by: Markus Armbruster --- qapi/run-state.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qapi/run-state.json b/qapi/run-state.json index 8124220bd9..15d6c9a2ed 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -348,7 +348,7 @@ # # @poweroff: Shutdown the VM and exit # -# @pause: pause the VM# +# @pause: pause the VM # # Since: 6.0 ## -- 2.35.1
Re: [PATCH 0/7] qapi: Primarily whitespace tweaks
On Mon, May 02, 2022 at 02:43:52PM +0200, Markus Armbruster wrote: > PATCH 1-5 queued, because no-brainers :) Thanks! How do you want me to handle respinning 6/7 and 7/7? Send out the entire series again with those two patches tweaked, wait for your pull request to make it into the tree, something else... ? What about the changes you suggested to the commit message of 3/7? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 6/7] qapi: Drop unnecessary horizontal spacing in comments
On Mon, May 02, 2022 at 10:50:07AM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > > -# @writeback: true if writeback mode is enabled > > -# @direct: true if the host page cache is bypassed (O_DIRECT) > > -# @no-flush:true if flush requests are ignored for the device > > +# @writeback: true if writeback mode is enabled > > +# @direct:true if the host page cache is bypassed (O_DIRECT) > > +# @no-flush: true if flush requests are ignored for the device > > I'm no fan of horizontally aligning descriptions, because when you add a > longer name, you either realign (I hate the churn) or live with the > inconsistency (I hate that, too). We seem to be in violent agreement on the topic, but it's apparent that other people feel diffently :) > I doubt changing to a different alignment now is useful. The next > patch, however, drops the alignment entirely. Possibly useful. > > Thoughts? My rationale for splitting things the way I did is that, if dropping the horizontal alignment entirely was not considered desirable, we could at least get rid of the extra whitespace. But if you think that the benefit from the half measure doesn't offset the cost of the churn it causes, I'm happy to drop these hunks and go straight from the current status to no horizontal alignment at all in one fell swoop with the next patch. > > -# Since: 0.14 > > +# Since: 0.14 > > This one is TAG: TEXT, whereas the one above is a multiple @NAME: > DESCRIPTION. Extra space in the latter can provide alignment. Extra > space in the former is always redundant. I'd take a patch dropping > these obviously redundant spaces without debate :) Okay, I'll respin this so that the first patch drops all extra whitespace in contexts where horizontal alignment is either not attempted or not possible, and the second one implements the more controversial changes. -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Mon, May 02, 2022 at 01:46:23PM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > >> > The wire protocol would still retain the unappealing name, but at > >> > least client libraries could hide the uglyness from users. > >> > >> At the price of mild inconsistency between the library interface and > >> QMP. > > > > That's fine, and in fact it already happens all the time when QAPI > > names (log-append) are translated to C identifiers (log_append). > > There's a difference between trivial translations like "replace '-' by > '_'" and arbitrary replacement like the one for enumeration constants > involving 'prefix'. Fair enough. I still feel that 1) users of a language SDK will ideally not need to look at the QAPI schema or wire chatter too often and 2) even when that ends up being necessary, figuring out that LogAppend and logappend are the same thing is not going to be an unreasonable hurdle, especially when the status quo would be to work with Logappend instead. > > The point is that, if we want to provide a language interface that > > feels natural, we need a way to mark parts of a QAPI symbol's name in > > a way that makes it possible for the generator to know they're > > acronyms and treat them in an appropriate, language-specific manner. > > It's not just acronyms. Consider IAmALittleTeapot. If you can assume > that only beginning of words are capitalized, even for acronyms, you can > split this into words without trouble. You can't recover correct case, > though: "i am a little teapot" is wrong. Is there any scenario in which we would care though? We're in the business of translating identifiers from one machine representation to another, so once it has been split up correctly into the words that compose it (which in your example above it has) then we don't really care about anything else unless acronyms are involved. In other words, we can obtain the list of words "i am a little teapot" programmatically both from IAmALittleTeapot and i-am-a-little-teapot, and in both cases we can then generate IAmALittleTeapot or I_AM_A_LITTLE_TEAPOT or i_am_a_little_teapot or whatever is appropriate for the context and target language, but the fact that in a proper English sentence "I" would have to be capitalized doesn't really enter the picture. > "Split before capital letter" falls apart when you have characters that > cannot be capitalized: Point3d. > > Camel case is hopeless. I would argue that it works quite well for most scenarios, but there are some corner cases where it's clearly not good enough. If we can define a way to clue in the generator about "Point3d" having to be interpreted as "point 3d" and "VNCProps" as "vnc props", then we are golden. That wouldn't be necessary for simple cases that are already handled correctly. A more radical idea would be to start using dash-notation for types too. That'd remove the word splitting issue altogether, at the cost of the schema being (possibly) harder to read and more distanced from the generated code. You'd still only be able to generate VncProps from vnc-props though. > > The obvious way to implement this would be with an annotation along > > the lines of the one I proposed. Other ideas? > > I'm afraid having the schema spell out names in multiple naming > conventions could be onerous. How many names will need it? I don't have hard data on this. I could try extracting it, but that might end up being a bigger job than I had anticipated. My guess is that the number of cases where the naive algorithm can't split words correctly is relatively small compared to the size of the entire QAPI schema. Fair warning: I have made incorrect guesses in the past ;) > Times how many naming conventions? Yeah, I don't think requiring all possible permutations to be spelled out in the schema is the way to go. That's exactly why my proposal was to offer a way to inject the semantic information that the parser can't figure out itself. Once you have a way to inform the generator that "VNCProps" is made of the two words "vnc" and "props", and that "vnc" is an acronym, then it can generate an identifier appropriate for the target language without having to spell out anywhere that such an identifier would be VNCProps for Go and VncProps for Rust. By the way, while looking around I realized that we also have to take into account things like D-Bus: the QAPI type ChardevDBus, for example, would probably translate verbatim to Go but have to be changed to ChardevDbus for Rust. Fun :) Revised proposal for the annotation: ns:word-WORD-WoRD-123Word Words are always separated by dashes; "regular" words are entirely lowercase, while the presence of even a singl
Re: [RFC PATCH v1 0/8] qapi: add generator for Golang interface
On Mon, May 02, 2022 at 09:21:36AM +0200, Markus Armbruster wrote: > Andrea Bolognani writes: > > The wire protocol would still retain the unappealing name, but at > > least client libraries could hide the uglyness from users. > > At the price of mild inconsistency between the library interface and > QMP. That's fine, and in fact it already happens all the time when QAPI names (log-append) are translated to C identifiers (log_append). > We could clean up QMP if we care, keeping around the old names for > compatibility. See also Kevin's work on QAPI aliases. Which is much > more ambitious, though. I wasn't aware of that effort. Personally I'm always in favor of cleaning up inconsistencies, so I am automatically a fan :) That said, the idea of exposing a sub-par Go API until such massive undertaking can be completed is not terribly appealing. And it would not address every facet of the issue (see below). > > Capitalization of these acronyms is inconsistent across the schema, > > Common issue with camel-cased compounds containing acronyms, because > either way is ugly. Agreed :) But consistent ugliness is still preferable to inconsistent ugliness. > > with one of the two forms disagreeing with Go naming expectations. > > Pardon my ignorance: What are Go's expectations? Acronyms are usually all upper case: https://pkg.go.dev/net/http#ParseHTTPVersion https://pkg.go.dev/net/http#ProxyURL https://pkg.go.dev/crypto/tls#NewLRUClientSessionCache The same seems to be true of Python: https://docs.python.org/3/library/http.html#http.HTTPStatus https://docs.python.org/3/library/urllib.error.html#urllib.error.URLError https://docs.python.org/3/library/xmlrpc.server.html#xmlrpc.server.SimpleXMLRPCServer Rust, on the other hand, seems to prefer only capitalizing the first letter of a word, no matter if it's an acronym: https://doc.rust-lang.org/std/net/struct.TcpStream.html https://doc.rust-lang.org/std/net/struct.Ipv4Addr.html https://doc.rust-lang.org/std/ffi/struct.OsString.html Whether different naming conventions are used for types, functions and struct members is also language-dependent. > > In this case we might be able to just change the schema without > > introducing backwards compatibility issues, though? Type names are > > not actually transmitted on the wire IIUC. > > Correct! That's great, but even if we decided to change all type names so that the schema is internally consistent and follows a naming convention that's reasonable for C, Go and Python, we'd still leave the Rust interface looking weird... There's no one-size-fits-all name, unfortunately. > > Of course such annotations would only be necessary to deal with > > identifiers that are not already following the expected naming > > conventions and when MLAs are involved. > > Pardon my ignorance some more: what are MLAs? Multi Letter Acronyms. Which are actually just called "acronyms" I guess? O:-) The point is that, if we want to provide a language interface that feels natural, we need a way to mark parts of a QAPI symbol's name in a way that makes it possible for the generator to know they're acronyms and treat them in an appropriate, language-specific manner. The obvious way to implement this would be with an annotation along the lines of the one I proposed. Other ideas? -- Andrea Bolognani / Red Hat / Virtualization
[PATCH 4/7] qapi: Drop unnecessary empty lines in comments
Signed-off-by: Andrea Bolognani
---
qapi/block-core.json | 4
qapi/block.json | 1 -
qapi/char.json| 1 -
qapi/common.json | 2 --
qapi/control.json | 2 --
qapi/crypto.json | 1 -
qapi/machine.json | 2 --
qapi/migration.json | 7 ---
qapi/misc-target.json | 3 ---
qapi/replay.json | 1 -
qapi/run-state.json | 3 ---
qapi/ui.json | 22 --
12 files changed, 49 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 34dae298ee..27832a1244 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -237,7 +237,6 @@
# information (since 1.7)
#
# Since: 1.3
-#
##
{ 'struct': 'ImageInfo',
'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
@@ -288,7 +287,6 @@
# supports it
#
# Since: 1.4
-#
##
{ 'struct': 'ImageCheck',
'data': {'filename': 'str', 'format': 'str', 'check-errors': 'int',
@@ -328,7 +326,6 @@
# @filename: filename that is referred to by @offset
#
# Since: 2.6
-#
##
{ 'struct': 'MapEntry',
'data': {'start': 'int', 'length': 'int', 'data': 'bool',
@@ -445,7 +442,6 @@
# has one or more dirty bitmaps) (Since 4.2)
#
# Since: 0.14
-#
##
{ 'struct': 'BlockDeviceInfo',
'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': 'str',
diff --git a/qapi/block.json b/qapi/block.json
index e0f7898ed1..5de15c6070 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -300,7 +300,6 @@
# @read-write: Makes the device writable
#
# Since: 2.3
-#
##
{ 'enum': 'BlockdevChangeReadOnlyMode',
'data': ['retain', 'read-only', 'read-write'] }
diff --git a/qapi/char.json b/qapi/char.json
index 7b42151575..f0fd0d1c9f 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -413,7 +413,6 @@
# @clipboard: enable/disable clipboard, default is disabled.
#
# Since: 6.1
-#
##
{ 'struct': 'ChardevQemuVDAgent',
'data': { '*mouse': 'bool',
diff --git a/qapi/common.json b/qapi/common.json
index 412cc4f5ae..356db3f670 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -192,7 +192,6 @@
# Keys to toggle input-linux between host and guest.
#
# Since: 4.0
-#
##
{ 'enum': 'GrabToggleKeys',
'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
@@ -204,7 +203,6 @@
# @human-readable-text: Formatted output intended for humans.
#
# Since: 6.2
-#
##
{ 'struct': 'HumanReadableText',
'data': { 'human-readable-text': 'str' } }
diff --git a/qapi/control.json b/qapi/control.json
index 71a838d49e..8c9122ef7a 100644
--- a/qapi/control.json
+++ b/qapi/control.json
@@ -33,7 +33,6 @@
#all the QMP capabilities will be turned off by default.
#
# Since: 0.13
-#
##
{ 'command': 'qmp_capabilities',
'data': { '*enable': [ 'QMPCapability' ] },
@@ -49,7 +48,6 @@
# (Please refer to qmp-spec.txt for more information on OOB)
#
# Since: 2.12
-#
##
{ 'enum': 'QMPCapability',
'data': [ 'oob' ] }
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 829e70a168..aebe390ab7 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -364,7 +364,6 @@
# password to use to retrieve current master key.
# Defaults to the same secret that was used to open the image
#
-#
# Since 5.1
##
{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
diff --git a/qapi/machine.json b/qapi/machine.json
index 9ec17b3992..20b1f0c748 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -77,7 +77,6 @@
# additional fields will be listed (since 3.0)
#
# Since: 2.12
-#
##
{ 'union' : 'CpuInfoFast',
'base' : { 'cpu-index': 'int',
@@ -1020,7 +1019,6 @@
# Formula used: logical_vm_size = vm_ram_size - balloon_size
#
# Since: 0.14
-#
##
{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
diff --git a/qapi/migration.json b/qapi/migration.json
index fc1c157d3f..dd4dde6361 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -151,7 +151,6 @@
# (since 4.2)
#
# Since: 2.3
-#
##
{ 'enum': 'MigrationStatus',
'data': [ 'none', 'setup', 'cancelling', 'cancelled',
@@ -166,7 +165,6 @@
# @transferred: amount of bytes transferred to the target VM by VFIO devices
#
# Since: 5.2
-#
##
{ 'struct': 'VfioStats',
'data': {'transferred': 'int' } }
@@ -546,7 +544,6 @@
# @zstd: use zstd compression method.
#
# Since: 5.0
-#
##
{ 'enum': 'MultiFDCompression',
'data': [ 'none', 'zlib',
@@ -1757,7 +1754,6 @@
# @dirty-rate: dirty rate.
#
# Since: 6.2
-#
##
{ 'struct': 'DirtyRateVcpu',
'data': { 'id': 'int', 'dirty-rate': 'int64' } }
@@ -1774,7 +1770,6 @@
# @measured: the dirtyrate thread has measured and results are available.
#
# Since: 5.2
-#
##
{ 'enum': 'DirtyRateStatus',
'data': [ 'unstarted', 'measuring', 'measured'] }
@@ -1791,7 +1786,6 @@
# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
#
# Since: 6.2
-#
##
{ 'enum': 'DirtyRateMeasureMode',
'data': ['page
[PATCH 3/7] qapi: Add missing separators between sections
Signed-off-by: Andrea Bolognani
---
qapi/block-core.json | 5 +
qapi/block.json | 1 +
qapi/crypto.json | 7 +++
qapi/machine.json| 1 +
qapi/migration.json | 4
5 files changed, 18 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b66494e8c5..34dae298ee 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1744,6 +1744,7 @@
# Since: 2.3
#
# Example:
+#
# -> { "execute": "blockdev-backup",
# "arguments": { "device": "src-id",
# "sync": "full",
@@ -2008,6 +2009,7 @@
# @on-target-error: the action to take on an error on the target,
# default 'report' (no limitations, since this applies to
# a different block device than @device).
+#
# @unmap: Whether to try to unmap target sectors where source has
# only zero. If true, and target unallocated sectors will read as zero,
# target image sectors will be unmapped; otherwise, zeroes will be
@@ -2029,6 +2031,7 @@
#When true, this job will automatically disappear from the
query
#list without user intervention.
#Defaults to true. (Since 3.1)
+#
# Since: 1.3
##
{ 'struct': 'DriveMirror',
@@ -2342,6 +2345,7 @@
#When true, this job will automatically disappear from the
query
#list without user intervention.
#Defaults to true. (Since 3.1)
+#
# Returns: nothing on success.
#
# Since: 2.6
@@ -4139,6 +4143,7 @@
# @throttle-group: the name of the throttle-group object to use. It
# must already exist.
# @file: reference to or definition of the data source block device
+#
# Since: 2.11
##
{ 'struct': 'BlockdevOptionsThrottle',
diff --git a/qapi/block.json b/qapi/block.json
index 3f100d4887..e0f7898ed1 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -105,6 +105,7 @@
#
# Returns: - Nothing on success
# - If @device is not a valid block device, DeviceNotFound
+#
# Notes:Ejecting a device with no media results in success
#
# Since: 0.14
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 1ec54c15ca..829e70a168 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -32,6 +32,7 @@
#
# @raw: raw bytes. When encoded in JSON only valid UTF-8 sequences can be used
# @base64: arbitrary base64 encoded binary data
+#
# Since: 2.6
##
{ 'enum': 'QCryptoSecretFormat',
@@ -51,6 +52,7 @@
# @sha384: SHA-384. (since 2.7)
# @sha512: SHA-512. (since 2.7)
# @ripemd160: RIPEMD-160. (since 2.7)
+#
# Since: 2.6
##
{ 'enum': 'QCryptoHashAlgorithm',
@@ -75,6 +77,7 @@
# @twofish-128: Twofish with 128 bit / 16 byte keys
# @twofish-192: Twofish with 192 bit / 24 byte keys
# @twofish-256: Twofish with 256 bit / 32 byte keys
+#
# Since: 2.6
##
{ 'enum': 'QCryptoCipherAlgorithm',
@@ -95,6 +98,7 @@
# @cbc: Cipher Block Chaining
# @xts: XEX with tweaked code book and ciphertext stealing
# @ctr: Counter (Since 2.8)
+#
# Since: 2.6
##
{ 'enum': 'QCryptoCipherMode',
@@ -114,6 +118,7 @@
# @plain: 64-bit sector number truncated to 32-bits
# @plain64: 64-bit sector number
# @essiv: 64-bit sector number encrypted with a hash of the encryption key
+#
# Since: 2.6
##
{ 'enum': 'QCryptoIVGenAlgorithm',
@@ -170,6 +175,7 @@
# @key-secret: the ID of a QCryptoSecret object providing the
# decryption key. Mandatory except when probing image for
# metadata only.
+#
# Since: 2.6
##
{ 'struct': 'QCryptoBlockOptionsLUKS',
@@ -194,6 +200,7 @@
# @iter-time: number of milliseconds to spend in
# PBKDF passphrase processing. Currently defaults
# to 2000. (since 2.8)
+#
# Since: 2.6
##
{ 'struct': 'QCryptoBlockCreateOptionsLUKS',
diff --git a/qapi/machine.json b/qapi/machine.json
index d25a481ce4..9ec17b3992 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -299,6 +299,7 @@
#returning does not indicate that a guest has accepted the request or
#that it has shut down. Many guests will respond to this command by
#prompting the user in some way.
+#
# Example:
#
# -> { "execute": "system_powerdown" }
diff --git a/qapi/migration.json b/qapi/migration.json
index 409eb086a2..fc1c157d3f 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1422,7 +1422,9 @@
# @state: The state the migration is currently expected to be in
#
# Returns: nothing on success
+#
# Since: 2.11
+#
# Example:
#
# -> { "execute": "migrate-continue" , "arguments":
@@ -1736,6 +1738,7 @@
# Since: 4.2
#
# Example:
+#
# <- { "event": "UNPLUG_PRIMARY",
# "data": { "device-id": "hostdev0" },
# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
@@ -1845,6 +1848,7 @@
# Since: 5.2
#
# Example:
+#
# {"execute": "calc-dirty-rate", "arguments": {"calc-time": 1,
#'sample-pages': 512} }
#
--
2.35.1
[PATCH 0/7] qapi: Primarily whitespace tweaks
The last patch could very reasonably be squashed into the previous one, but since the changes could be considered more controversial I thought it would be better if the two could be reviewed and judged separately. Andrea Bolognani (7): qapi: Drop stray trailing symbol qapi: Fix comment indentation qapi: Add missing separators between sections qapi: Drop unnecessary empty lines in comments qapi: Drop unnecessary empty lines outside of comments qapi: Drop unnecessary horizontal spacing in comments qapi: Drop more unnecessary horizontal spacing in comments qapi/audio.json | 1 - qapi/block-core.json | 97 ++-- qapi/block-export.json | 2 +- qapi/block.json | 13 +++--- qapi/char.json | 10 ++--- qapi/common.json | 2 - qapi/control.json| 13 +++--- qapi/crypto.json | 62 - qapi/dump.json | 4 +- qapi/job.json| 1 - qapi/machine-target.json | 1 - qapi/machine.json| 12 +++-- qapi/migration.json | 19 qapi/misc-target.json| 13 ++ qapi/misc.json | 6 +-- qapi/replay.json | 1 - qapi/run-state.json | 10 ++--- qapi/sockets.json| 6 +-- qapi/ui.json | 70 ++--- 19 files changed, 136 insertions(+), 207 deletions(-) -- 2.35.1
[PATCH 7/7] qapi: Drop more unnecessary horizontal spacing in comments
Vertical alignment is sacrificed in the process. Signed-off-by: Andrea Bolognani --- qapi/block-core.json | 43 +-- qapi/block.json | 6 +++--- qapi/char.json | 6 +++--- qapi/control.json| 6 +++--- qapi/crypto.json | 32 qapi/migration.json | 8 qapi/ui.json | 13 ++--- 7 files changed, 56 insertions(+), 58 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 5fd66ea676..f0383c7925 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -338,8 +338,8 @@ # Cache mode information for a block device # # @writeback: true if writeback mode is enabled -# @direct:true if the host page cache is bypassed (O_DIRECT) -# @no-flush: true if flush requests are ignored for the device +# @direct: true if the host page cache is bypassed (O_DIRECT) +# @no-flush: true if flush requests are ignored for the device # # Since: 2.3 ## @@ -,12 +,11 @@ ## # @BlockdevOptionsSsh: # -# @server: host address +# @server: host address # -# @path: path to the image on the host +# @path: path to the image on the host # -# @user: user as which to connect, defaults to current -# local user name +# @user: user as which to connect, defaults to current local user name # # @host-key-check: Defines how and what to check the host key against # (default: known_hosts) @@ -4662,18 +4661,18 @@ # # Subformat options for VMDK images # -# @monolithicSparse: Single file image with sparse cluster allocation +# @monolithicSparse: Single file image with sparse cluster allocation # -# @monolithicFlat: Single flat data image and a descriptor file +# @monolithicFlat: Single flat data image and a descriptor file # # @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent #files, in addition to a descriptor file # -# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent -#files, in addition to a descriptor file +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent +# files, in addition to a descriptor file # -# @streamOptimized: Single file image sparse cluster allocation, optimized -#for streaming over network. +# @streamOptimized: Single file image sparse cluster allocation, optimized +# for streaming over network. # # Since: 4.0 ## @@ -4764,7 +4763,7 @@ # @BlockdevVhdxSubformat: # # @dynamic: Growing image file -# @fixed: Preallocated fixed-size image file +# @fixed: Preallocated fixed-size image file # # Since: 2.12 ## @@ -4802,7 +4801,7 @@ # @BlockdevVpcSubformat: # # @dynamic: Growing image file -# @fixed: Preallocated fixed-size image file +# @fixed: Preallocated fixed-size image file # # Since: 2.12 ## @@ -4865,7 +4864,7 @@ # Starts a job to create an image format on a given node. The job is # automatically finalized, but a manual job-dismiss is required. # -# @job-id: Identifier for the newly created job. +# @job-id: Identifier for the newly created job. # # @options: Options for the image creation. # @@ -4923,17 +4922,17 @@ # Starts a job to amend format specific options of an existing open block device # The job is automatically finalized, but a manual job-dismiss is required. # -# @job-id:Identifier for the newly created job. +# @job-id: Identifier for the newly created job. # # @node-name: Name of the block node to work on # -# @options: Options (driver specific) +# @options: Options (driver specific) # -# @force: Allow unsafe operations, format specific -# For luks that allows erase of the last active keyslot -# (permanent loss of data), -# and replacement of an active keyslot -# (possible loss of data if IO error happens) +# @force: Allow unsafe operations, format specific +# For luks that allows erase of the last active keyslot +# (permanent loss of data), +# and replacement of an active keyslot +# (possible loss of data if IO error happens) # # Features: # @unstable: This command is experimental. diff --git a/qapi/block.json b/qapi/block.json index 96f557b3bb..19326641ac 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -50,9 +50,9 @@ # # Type of Floppy drive to be emulated by the Floppy Disk Controller. # -# @144: 1.44MB 3.5" drive -# @288: 2.88MB 3.5" drive -# @120: 1.2MB 5.25" drive +# @144: 1.44MB 3.5" drive +# @288: 2.88MB 3.5" drive +# @120: 1.2MB 5.25" drive # @none: No drive connected # @auto: Automatically determined by inserted media at boot # diff --git a/qapi/char.json b/qapi/char.json index a40fe4b7bd..923dc5056d 100644 --- a/qapi/char.json +++ b/qapi/char.json @@ -376,10 +376,10 @@ # # Configuration info for virtual
[PATCH 6/7] qapi: Drop unnecessary horizontal spacing in comments
Care was taken not to break vertical alignment.
Signed-off-by: Andrea Bolognani
---
qapi/block-core.json | 62 +-
qapi/block-export.json | 2 +-
qapi/block.json| 2 +-
qapi/char.json | 2 +-
qapi/control.json | 10 +++
qapi/crypto.json | 44 +++---
qapi/dump.json | 4 +--
qapi/machine.json | 8 +++---
qapi/misc-target.json | 6 ++--
qapi/misc.json | 6 ++--
qapi/run-state.json| 4 +--
qapi/sockets.json | 6 ++--
qapi/ui.json | 18 ++--
13 files changed, 87 insertions(+), 87 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2bce5bb0ae..5fd66ea676 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -337,9 +337,9 @@
#
# Cache mode information for a block device
#
-# @writeback: true if writeback mode is enabled
-# @direct: true if the host page cache is bypassed (O_DIRECT)
-# @no-flush:true if flush requests are ignored for the device
+# @writeback: true if writeback mode is enabled
+# @direct:true if the host page cache is bypassed (O_DIRECT)
+# @no-flush: true if flush requests are ignored for the device
#
# Since: 2.3
##
@@ -604,7 +604,7 @@
# @inserted: @BlockDeviceInfo describing the device if media is
#present
#
-# Since: 0.14
+# Since: 0.14
##
{ 'struct': 'BlockInfo',
'data': {'device': 'str', '*qdev': 'str', 'type': 'str', 'removable': 'bool',
@@ -795,9 +795,9 @@
#
# Statistics of a virtual block device or a block backing device.
#
-# @rd_bytes: The number of bytes read by the device.
+# @rd_bytes: The number of bytes read by the device.
#
-# @wr_bytes: The number of bytes written by the device.
+# @wr_bytes: The number of bytes written by the device.
#
# @unmap_bytes: The number of bytes unmapped by the device (Since 4.2)
#
@@ -970,7 +970,7 @@
# @qdev: The qdev ID, or if no ID is assigned, the QOM path of the block
#device. (since 3.0)
#
-# @stats: A @BlockDeviceStats for the device.
+# @stats: A @BlockDeviceStats for the device.
#
# @driver-specific: Optional driver-specific stats. (Since 4.2)
#
@@ -1275,7 +1275,7 @@
#
# @node-name: graph node name to get the image resized (Since 2.0)
#
-# @size: new image size in bytes
+# @size: new image size in bytes
#
# Returns: - nothing on success
# - If @device is not a valid block device, DeviceNotFound
@@ -1960,8 +1960,8 @@
# @job-id: identifier for the newly-created block job. If
# omitted, the device name will be used. (Since 2.7)
#
-# @device: the device name or node-name of a root node whose writes should be
-# mirrored.
+# @device: the device name or node-name of a root node whose writes should be
+# mirrored.
#
# @target: the target of the new image. If the file exists, or if it
# is a device, the existing file/device will be used as the new
@@ -1981,7 +1981,7 @@
# @mode: whether and how QEMU should create a new image, default is
#'absolute-paths'.
#
-# @speed: the maximum speed, in bytes per second
+# @speed: the maximum speed, in bytes per second
#
# @sync: what parts of the disk image should be copied to the destination
#(all the disk, only the sectors allocated in the topmost image, or
@@ -2296,7 +2296,7 @@
#broken Quorum files. By default, @device is replaced, although
#implicitly created filters on it are kept.
#
-# @speed: the maximum speed, in bytes per second
+# @speed: the maximum speed, in bytes per second
#
# @sync: what parts of the disk image should be copied to the destination
#(all the disk, only the sectors allocated in the topmost image, or
@@ -,15 +,15 @@
##
# @BlockdevOptionsSsh:
#
-# @server: host address
+# @server: host address
#
-# @path:path to the image on the host
+# @path: path to the image on the host
#
-# @user:user as which to connect, defaults to current
-# local user name
+# @user: user as which to connect, defaults to current
+# local user name
#
-# @host-key-check: Defines how and what to check the host key against
-# (default: known_hosts)
+# @host-key-check: Defines how and what to check the host key against
+# (default: known_hosts)
#
# Since: 2.9
##
@@ -4548,8 +4548,8 @@
##
# @BlockdevQcow2Version:
#
-# @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
-# @v3: The extended QCOW2 format as introduced in qemu 1.1 (version 3)
+# @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
+# @v3: The extended QCOW2 format as introduced in qemu 1.1 (version 3)
#
# Since: 2.12
##
@@ -4865,9 +4865,9 @@
# Starts a job to create an image format on a given node. The job is
# automatically finalized, but a manual job-dismiss
[PATCH 5/7] qapi: Drop unnecessary empty lines outside of comments
Signed-off-by: Andrea Bolognani
---
qapi/audio.json | 1 -
qapi/block-core.json | 11 ---
qapi/block.json | 3 ---
qapi/char.json | 1 -
qapi/control.json| 1 -
qapi/crypto.json | 12
qapi/job.json| 1 -
qapi/machine-target.json | 1 -
qapi/machine.json| 1 -
qapi/misc-target.json| 4
qapi/run-state.json | 1 -
qapi/ui.json | 1 -
12 files changed, 38 deletions(-)
diff --git a/qapi/audio.json b/qapi/audio.json
index 0785e70a50..8099e3d7f1 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -352,7 +352,6 @@
'*out': 'AudiodevPerDirectionOptions',
'*path': 'str' } }
-
##
# @AudioFormat:
#
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 27832a1244..2bce5bb0ae 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -739,7 +739,6 @@
##
{ 'command': 'query-block', 'returns': ['BlockInfo'] }
-
##
# @BlockDeviceTimedStats:
#
@@ -1512,7 +1511,6 @@
{ 'command': 'blockdev-snapshot-sync',
'data': 'BlockdevSnapshotSync' }
-
##
# @blockdev-snapshot:
#
@@ -1751,7 +1749,6 @@
{ 'command': 'blockdev-backup', 'boxed': true,
'data': 'BlockdevBackup' }
-
##
# @query-named-block-nodes:
#
@@ -3067,7 +3064,6 @@
'base': 'BlockdevOptionsGenericFormat',
'data': { '*key-secret': 'str' } }
-
##
# @BlockdevOptionsGenericCOWFormat:
#
@@ -3182,8 +3178,6 @@
'base': 'BlockdevOptionsGenericCOWFormat',
'data': { '*encrypt': 'BlockdevQcowEncryption' } }
-
-
##
# @BlockdevQcow2EncryptionFormat:
#
@@ -3357,7 +3351,6 @@
'*user': 'str',
'*host-key-check': 'SshHostKeyCheck' } }
-
##
# @BlkdebugEvent:
#
@@ -3721,7 +3714,6 @@
'*header-digest': 'IscsiHeaderDigest',
'*timeout': 'int' } }
-
##
# @RbdAuthMode:
#
@@ -4564,7 +4556,6 @@
{ 'enum': 'BlockdevQcow2Version',
'data': [ 'v2', 'v3' ] }
-
##
# @Qcow2CompressionType:
#
@@ -4738,7 +4729,6 @@
'*toolsversion':'str',
'*zeroed-grain':'bool' } }
-
##
# @BlockdevCreateOptionsSsh:
#
@@ -4973,7 +4963,6 @@
{ 'enum': 'BlockErrorAction',
'data': [ 'ignore', 'report', 'stop' ] }
-
##
# @BLOCK_IMAGE_CORRUPTED:
#
diff --git a/qapi/block.json b/qapi/block.json
index 5de15c6070..41b73c9934 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -286,7 +286,6 @@
'data': { 'id': 'str',
'node-name': 'str'} }
-
##
# @BlockdevChangeReadOnlyMode:
#
@@ -304,7 +303,6 @@
{ 'enum': 'BlockdevChangeReadOnlyMode',
'data': ['retain', 'read-only', 'read-write'] }
-
##
# @blockdev-change-medium:
#
@@ -375,7 +373,6 @@
'*force': 'bool',
'*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
-
##
# @DEVICE_TRAY_MOVED:
#
diff --git a/qapi/char.json b/qapi/char.json
index f0fd0d1c9f..8414ef2bc2 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -329,7 +329,6 @@
'data': { '*signal': 'bool' },
'base': 'ChardevCommon' }
-
##
# @ChardevSpiceChannel:
#
diff --git a/qapi/control.json b/qapi/control.json
index 8c9122ef7a..53461cec05 100644
--- a/qapi/control.json
+++ b/qapi/control.json
@@ -68,7 +68,6 @@
{ 'struct': 'VersionTriple',
'data': {'major': 'int', 'minor': 'int', 'micro': 'int'} }
-
##
# @VersionInfo:
#
diff --git a/qapi/crypto.json b/qapi/crypto.json
index aebe390ab7..ff33e1fe1f 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -24,7 +24,6 @@
'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT',
'data': ['client', 'server']}
-
##
# @QCryptoSecretFormat:
#
@@ -39,7 +38,6 @@
'prefix': 'QCRYPTO_SECRET_FORMAT',
'data': ['raw', 'base64']}
-
##
# @QCryptoHashAlgorithm:
#
@@ -59,7 +57,6 @@
'prefix': 'QCRYPTO_HASH_ALG',
'data': ['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512', 'ripemd160']}
-
##
# @QCryptoCipherAlgorithm:
#
@@ -88,7 +85,6 @@
'serpent-128', 'serpent-192', 'serpent-256',
'twofish-128', 'twofish-192', 'twofish-256']}
-
##
# @QCryptoCipherMode:
#
@@ -105,7 +101,6 @@
'prefix': 'QCRYPTO_CIPHER_MODE',
'data': ['ecb', 'cbc', 'xts', 'ctr']}
-
##
# @QCryptoIVGenAlgorithm:
#
@@ -181,7 +176,6 @@
{ 'struct': 'QCryptoBlockOptionsLUKS',
'data': { '*key-secret': 'str' }}
-
##
# @QCryptoBlockCreateOptionsLUKS:
#
@@ -212,7 +206,6 @@
'*hash-alg': 'QCryptoHashAlgorithm',
'*iter-time': 'int'}}
-
##
# @QCryptoBlockOpenOptions:
#
@@ -227,7 +220,6 @@
'data': { 'qcow': 'QCryptoBlockOptionsQCow',
'luks': 'QCryptoBlockOptionsLUKS' } }
-
##
# @QCryptoBlockCreateOptions:
#
@@ -242,7 +234,6 @@
'data': { 'qcow': 'QCryptoBlockOptionsQCow',
'luks': 'QCryptoBlockCreateOptionsLUKS' } }
-
##
# @QCryptoBlockInfoBase:
#
@@ -256,7 +247,6 @@
{ 'struct': 'QCryptoBlockInfoBase',
'data': { 'format': 'QCryptoBlockFormat' }}
-
##
# @QCryptoBlockInfoLUKSSlot:
#
@@ -276,7 +266,6 @@
'*stripes
[PATCH 1/7] qapi: Drop stray trailing symbol
Signed-off-by: Andrea Bolognani --- qapi/run-state.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qapi/run-state.json b/qapi/run-state.json index 8124220bd9..15d6c9a2ed 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -348,7 +348,7 @@ # # @poweroff: Shutdown the VM and exit # -# @pause: pause the VM# +# @pause: pause the VM # # Since: 6.0 ## -- 2.35.1
