Re: [PATCH] failover: allow to pause the VM during the migration
On 30/09/2021 22:17, Laine Stump wrote: On 9/30/21 1:09 PM, Laurent Vivier wrote: If we want to save a snapshot of a VM to a file, we used to follow the following steps: 1- stop the VM: (qemu) stop 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" 3- resume the VM: (qemu) cont After that we can restore the snapshot with: qemu-system-x86_64 ... -incoming "exec:cat snapshot" (qemu) cont This is the basics of what libvirt does for a snapshot, and steps 1+2 are what it does for a "managedsave" (where it saves the snapshot to disk and then terminates the qemu process, for later re-animation). In those cases, it seems like this new parameter could work for us - instead of explicitly pausing the guest prior to migrating it to disk, we would set this new parameter to on, then directly migrate-to-disk (relying on qemu to do the pause). Care will need to be taken to assure that error recovery behaves the same though. In case of error, the VM is restarted like it's done for a standard migration. I can change that if you need. An other point is the VM state sent to the migration stream is "paused", it means that machine needs to be resumed after the stream is loaded (from the file or on destination in the case of a real migration), but it can be also changed to be "running" so the machine will be resumed automatically at the end of the file loading (or real migration) There are a couple of cases when libvirt apparently *doesn't* pause the guest during the migrate-to-disk, both having to do with saving a coredump of the guest. Since I really have no idea of how common/important that is (or even if my assessment of the code is correct), I'm Cc'ing this patch to libvir-list to make sure it catches the attention of someone who knows the answers and implications. It's an interesting point I need to test and think about: in case of a coredump I guess the machine is crashed and doesn't answer to the unplug request and so the failover unplug cannot be done. For the moment the migration will hang until it is canceled. IT can be annoying if we want to debug the cause of the crash... But when failover is configured, it doesn't work anymore. As the failover needs to ask the guest OS to unplug the card the machine cannot be paused. This patch introduces a new migration parameter, "pause-vm", that asks the migration to pause the VM during the migration startup phase after the the card is unplugged. Once the migration is done, we only need to resume the VM with "cont" and the card is plugged back: 1- set the parameter: (qemu) migrate_set_parameter pause-vm on 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" The primary failover card (VFIO) is unplugged and the VM is paused. 3- resume the VM: (qemu) cont The VM restarts and the primary failover card is plugged back The VM state sent in the migration stream is "paused", it means when the snapshot is loaded or if the stream is sent to a destination QEMU, the VM needs to be resumed manually. Signed-off-by: Laurent Vivier --- qapi/migration.json | 20 +++--- include/hw/virtio/virtio-net.h | 1 + hw/net/virtio-net.c | 33 ++ migration/migration.c | 37 +- monitor/hmp-cmds.c | 8 5 files changed, 95 insertions(+), 4 deletions(-) ... Thanks, Laurent
Re: [PATCH] failover: fix unplug pending detection
On 01/10/2021 07:19, Ani Sinha wrote: On Thu, 30 Sep 2021, Laurent Vivier wrote: On 30/09/2021 11:24, Ani Sinha wrote: On Thu, 30 Sep 2021, Laurent Vivier wrote: Failover needs to detect the end of the PCI unplug to start migration after the VFIO card has been unplugged. To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in pcie_unplug_device(). But since 17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") we have switched to ACPI unplug and these functions are not called anymore and the flag not set. So failover migration is not able to detect if card is really unplugged and acts as it's done as soon as it's started. So it doesn't wait the end of the unplug to start the migration. We don't see any problem when we test that because ACPI unplug is faster than PCIe native hotplug and when the migration really starts the unplug operation is already done. See c000a9bd06ea ("pci: mark device having guest unplug request pending") a99c4da9fc2a ("pci: mark devices partially unplugged") Ok so I have a basic question about partially_hotplugged flag in the device struct (there were no comments added in a99c4da9fc2a39847 explaining it). It seems we return early from pcie_unplug_device() when this flag is set from failover_unplug_primary() in virtio-net. What is the purpose of this flag? It seems we are not doing a full unplug of the primary device? Yes, to be able to plug it back in case of migration failure we must keep all the data structures. Ok so two things here: (a) could you please add a comment to PCIDevice struct in pci.h to clarify what the flag actually means, why it is there and what it is supposed to do. Will be in v3. (b) the naming of the variable could be something like do_partial_unplug or some such. This could be a separate patch. OK, I'll do that on a separate patch: I'm already working on a patch series moving most of the failover code to PCI files (hotplug is a PCI feature not a virtio one). https://patchew.org/QEMU/20210820142002.152994-1-lviv...@redhat.com/ > But reading the code again it seems this part should be in acpi_pcihp_eject_slot() rather than in acpi_pcihp_device_unplug_cb() to prevent the hotplug_handler_unplug()/object_unparent() rather than the qdev_unrealize() (to be like in pcie.c). Correct. You need to place the check earlier so as to be equivalent to what the native hotplug code does. Thanks, Laurent
Re: [PATCH v2] failover: fix unplug pending detection
On Thu, 30 Sep 2021, Laurent Vivier wrote: > Failover needs to detect the end of the PCI unplug to start migration > after the VFIO card has been unplugged. > > To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset in > pcie_unplug_device(). > > But since > 17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") > we have switched to ACPI unplug and these functions are not called anymore > and the flag not set. So failover migration is not able to detect if card > is really unplugged and acts as it's done as soon as it's started. So it > doesn't wait the end of the unplug to start the migration. We don't see any > problem when we test that because ACPI unplug is faster than PCIe native > hotplug and when the migration really starts the unplug operation is > already done. > > See c000a9bd06ea ("pci: mark device having guest unplug request pending") > a99c4da9fc2a ("pci: mark devices partially unplugged") > > Signed-off-by: Laurent Vivier Modulo the comment suggestion below, Reviewed-by: Ani Sinha > --- > > Notes: > v2: move partially_hotplugged to acpi_pcihp_eject_slot() > > hw/acpi/pcihp.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index f610a25d2ef9..7bbf13492a4f 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -222,9 +222,13 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, > unsigned bsel, unsigned slo > PCIDevice *dev = PCI_DEVICE(qdev); > if (PCI_SLOT(dev->devfn) == slot) { > if (!acpi_pcihp_pc_no_hotplug(s, dev)) { > -hotplug_ctrl = qdev_get_hotplug_handler(qdev); > -hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort); > -object_unparent(OBJECT(qdev)); > +if (dev->partially_hotplugged) { Please add a comment here as to why you are skipping full unplug and what this flag signifies and where it is set from. > +qdev->pending_deleted_event = false; > +} else { > +hotplug_ctrl = qdev_get_hotplug_handler(qdev); > +hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort); > +object_unparent(OBJECT(qdev)); > +} > } > } > } > @@ -396,6 +400,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler > *hotplug_dev, > return; > } > > +pdev->qdev.pending_deleted_event = true; > s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); > acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > } > -- > 2.31.1 > >
Re: [PATCH] failover: fix unplug pending detection
On Thu, 30 Sep 2021, Laurent Vivier wrote: > On 30/09/2021 11:24, Ani Sinha wrote: > > > > > > On Thu, 30 Sep 2021, Laurent Vivier wrote: > > > > > Failover needs to detect the end of the PCI unplug to start migration > > > after the VFIO card has been unplugged. > > > > > > To do that, a flag is set in pcie_cap_slot_unplug_request_cb() and reset > > > in > > > pcie_unplug_device(). > > > > > > But since > > > 17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on > > > Q35") > > > we have switched to ACPI unplug and these functions are not called anymore > > > and the flag not set. So failover migration is not able to detect if card > > > is really unplugged and acts as it's done as soon as it's started. So it > > > doesn't wait the end of the unplug to start the migration. We don't see > > > any > > > problem when we test that because ACPI unplug is faster than PCIe native > > > hotplug and when the migration really starts the unplug operation is > > > already done. > > > > > > See c000a9bd06ea ("pci: mark device having guest unplug request pending") > > > a99c4da9fc2a ("pci: mark devices partially unplugged") > > > > Ok so I have a basic question about partially_hotplugged flag in the > > device struct (there were no comments added in a99c4da9fc2a39847 > > explaining it). It seems we return early from pcie_unplug_device() when > > this flag is set from failover_unplug_primary() in virtio-net. What is the > > purpose of this flag? It seems we are not doing a full unplug of the > > primary device? > > Yes, to be able to plug it back in case of migration failure we must keep all > the data structures. Ok so two things here: (a) could you please add a comment to PCIDevice struct in pci.h to clarify what the flag actually means, why it is there and what it is supposed to do. (b) the naming of the variable could be something like do_partial_unplug or some such. This could be a separate patch. > > But reading the code again it seems this part should be in > acpi_pcihp_eject_slot() rather than in acpi_pcihp_device_unplug_cb() to > prevent the hotplug_handler_unplug()/object_unparent() rather than the > qdev_unrealize() (to be like in pcie.c). Correct. You need to place the check earlier so as to be equivalent to what the native hotplug code does.
Re: [PATCH 1/7] docs: name included files ".rst.inc"
Emacs recognizes .rst, but doesn't recognize .rst.inc. Sure we want file names that defeat common tooling?
Re: OHCI/usb pass through
Hi, > [...] > /* Active packets. */ > uint32_t old_ctl; > USBPacket usb_packet; > uint8_t usb_buf[8192]; > uint32_t async_td; > bool async_complete; > > void (*ohci_die)(struct OHCIState *ohci); > } OHCIState; > > Then everything in hcd-ohci seems to reuse ohci->usb_packet and I wonder if > it can happen that it's overwritten while an async packet is still using it. Plausible theory. That also nicely explains why you need traffic on two endpoints at the same time to trigger it. > In any case to both fix the device model and to avoid this possible problem > described above it seems we would need to ditch the packet and async_td > fields from OHCIState and move them to the endpoint to allow one active > packet per endpoint. Either that, or maintain a linked list of packets. > We can get the endpoint from a packet and from ohci so > I wonder if we can get the active packet from ep->queue (and how to do that) I think ohci never looks beyond the active td so there should never be more than one packet on the list. HTH, Gerd
Re: TCG Floating Point Support (Work in Progress)
Thank you Alex, for your quick and thoughtful response. > I've not reviewed the code as it is a rather large diff. For your proper > submission could you please ensure that your patch series is broken up > into discreet commits to aid review. Of course. > The phrase "if the user discovers some issues" is a bit of a red flag. > We should always be striving for correct emulation of floating point. I agree. This is an option that I added for use during feature development. Ultimately I would like not to have such an option, and for it to always *just work*. > Indeed we have recently got the code base to the point we pass all of > the Berkey softfloat test suite. This can be checked by running: > > make check-softfloat > > However the test code links directly to the softfloat code so won't work > with direct code execution. I had planned to leverage the existing soft float test suite, and I think this can be done with the right harnessing. It would be nice to have a mechanism to test translation of individual TCG ops, e.g. be able to run translated blocks from test code and evaluate their output. I'm not sure if any such op level testing like that is being done. There are also guest tests in tests/tcg, which could also be expanded to include more FP tests. > The existing 32/64 bit hardfloat > optimisations work within the helpers. While generating direct code is > appealing to avoid the cost of helper calls it's fairly well cached and > predicted code. Experience with the initial hardfloat code showed the > was still a performance win even with the cost of the helper call. Unfortunately, even with the existing hardfloat support, the overhead of the helper calls is still too costly for my particular application. > I don't think you'll see the same behaviour emulating an x87 on anything > that isn't an x87 because the boundaries for things like inexact > calculation will be different. Indeed if you look at the existing > hardfloat code function can_use_fpu() you will see we only call the host > processor function if the inexact bit is already set. Other wrappers > have even more checks for normal numbers. Anything that needs NaN > handling will fallback to the correct softfloat code. Fair points. Bit-perfect x87 emulation with this approach may be ultimately unachievable; and I'm still evaluating the cases when this will not work. This has been a learning experience for me, and I gladly welcome expert input in this matter. Personally, I would accept minor accuracy differences in trade for significant performance advantage in emulation of game code, but not for scientific applications, which I understand may diminish upstream appeal of this x87 translation work. > I think there will be a wariness to merge anything that only works for a > single frontend/backend combination. Running translated x86 on x86 is > not the common case for TCG ;-) Understood; initially this works on a single frontend/backend combination, with eventual translation to other guests and hosts. It will be a long road, but my plan next is to produce a translation for AArch64 systems. > These are the things that make correct handling of floating point hard. Agreed! > I'll happily review patches on the list that provide for an accelerated > FPU experience as long as the correctness is maintained. Thank you! Matt On Thu, Sep 30, 2021 at 2:38 AM Alex Bennée wrote: > > > Matt writes: > > > Hello-- > > > > I'm excited to share that I have been developing support for TCG > > floating point operations; specifically, to accelerate emulation of > > x86 guest code which heavily exercises the x87 FPU for a game console > > emulator project based on QEMU. So far, this work has shown great > > promise, demonstrating some dramatic performance improvements in > > emulation of x87 heavy code. > > I've not reviewed the code as it is a rather large diff. For your proper > submission could you please ensure that your patch series is broken up > into discreet commits to aid review. It also aids bisection if > regressions are identified. > > > The feature works in concert with unaccelerated x87 FPU helpers, and > > also allows total soft float helper fallback if the user discovers > > some issue with the hard float implementation. > > The phrase "if the user discovers some issues" is a bit of a red flag. > We should always be striving for correct emulation of floating point. > Indeed we have recently got the code base to the point we pass all of > the Berkey softfloat test suite. This can be checked by running: > > make check-softfloat > > However the test code links directly to the softfloat code so won't work > with direct code execution. The existing 32/64 bit hardfloat > optimisations work within the helpers. While generating direct code is > appealing to avoid the cost of helper calls it's fairly well cached and > predicted code. Experience with the initial hardfloat code showed the > was still a performance win even with the cost of t
Re: [PATCH 3/3] dtc: Update to version 1.6.1
On Thu, Sep 30, 2021 at 09:10:12AM +0200, Thomas Huth wrote: > On 27/08/2021 14.09, Thomas Huth wrote: > > The dtc submodule is currently pointing to non-release commit. It's nicer > > if submodules point to release versions instead and since dtc 1.6.1 is > > available now, let's update to that version. > > > > Signed-off-by: Thomas Huth > > --- > > dtc | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/dtc b/dtc > > index 85e5d83984..b6910bec11 16 > > --- a/dtc > > +++ b/dtc > > @@ -1 +1 @@ > > -Subproject commit 85e5d839847af54efab170f2b1331b2a6421e647 > > +Subproject commit b6910bec11614980a21e46fbccc35934b671bd81 > > Ping! > > David, could you please pick up this patch if you don't mind it? Uhh... I'm dtc upstream maintainer, but I haven't typically been handling the dtc submodule in qemu. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH 3/3] dtc: Update to version 1.6.1
On Thu, Sep 30, 2021 at 01:56:33PM +0200, Greg Kurz wrote: > On Thu, 30 Sep 2021 09:10:12 +0200 > Thomas Huth wrote: > > > On 27/08/2021 14.09, Thomas Huth wrote: > > > The dtc submodule is currently pointing to non-release commit. It's nicer > > > if submodules point to release versions instead and since dtc 1.6.1 is > > > available now, let's update to that version. > > > > > > Signed-off-by: Thomas Huth > > > --- > > > > dtc | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/dtc b/dtc > > > index 85e5d83984..b6910bec11 16 > > > --- a/dtc > > > +++ b/dtc > > > @@ -1 +1 @@ > > > -Subproject commit 85e5d839847af54efab170f2b1331b2a6421e647 > > > +Subproject commit b6910bec11614980a21e46fbccc35934b671bd81 > > > > Ping! > > > > David, could you please pick up this patch if you don't mind it? > > > > Thanks, > >Thomas > > > > Hi Thomas ! > > greg@bahia:[main]dtc$ git log --oneline > 85e5d839847af54efab170f2b1331b2a6421e647..v1.6.1 > b6910bec1161 Bump version to v1.6.1 > 21d61d18f968 Fix CID 1461557 > 4c2ef8f4d14c checks: Introduce is_multiple_of() > e59ca36fb70e Make handling of cpp line information more tolerant > 0c3fd9b6aceb checks: Drop interrupt_cells_is_cell check > 6b3081abc4ac checks: Add check_is_cell() for all phandle+arg properties > 2dffc192a77f yamltree: Remove marker ordering dependency > 61e513439e40 pylibfdt: Rework "avoid unused variable warning" lines > c8bddd106095 tests: add a positive gpio test case > ad4abfadb687 checks: replace strstr and strrchr with strends > 09c6a6e88718 dtc.h: add strends for suffix matching > 9bb9b8d0b4a0 checks: tigthen up nr-gpios prop exception > b07b62ee3342 libfdt: Add FDT alignment check to fdt_check_header() > a2def5479950 libfdt: Check that the root-node name is empty > 4ca61f84dc21 libfdt: Check that there is only one root node > 34d708249a91 dtc: Remove -O dtbo support > 8e7ff260f755 libfdt: Fix a possible "unchecked return value" warning > 88875268c05c checks: Warn on node-name and property name being the same > 9d2279e7e6ee checks: Change node-name check to match devicetree spec > f527c867a8c6 util: limit gnu_printf format attribute to gcc >= 4.4.0 > 183df9e9c2b9 gitignore: Ignore the swp files > 0db6d09584e1 gitignore: Add cscope files > 307afa1a7be8 Update Jon Loeliger's email > ca16a723fa9d fdtdump: Fix gcc11 warning > 64990a272e8f srcpos: increase MAX_SRCFILE_DEPTH > 163f0469bf2e dtc: Allow overlays to have .dtbo extension > 3b01518e688d Set last_comp_version correctly in new dtb and fix potential > version issues in fdt_open_into > f7e5737f26aa tests: Fix overlay_overlay_nosugar test case > 7cd5d5fe43d5 libfdt: Tweak description of assume-aligned load helpers > a7c404099349 libfdt: Internally perform potentially unaligned loads > bab85e48a6f4 meson: increase default timeout for tests > f8b46098824d meson: do not assume python is installed, skip tests > 30a56bce4f0b meson: fix -Wall warning > 5e735860c478 libfdt: Check for 8-byte address alignment in fdt_ro_probe_() > 67849a327927 build-sys: add meson build > 05874d08212d pylibfdt: allow build out of tree > 3bc3a6b9fe0c dtc: Fix signedness comparisons warnings: Wrap (-1) > e1147b159e92 dtc: Fix signedness comparisons warnings: change types > 04cf1fdc0fcf convert-dtsv0: Fix signedness comparisons warning > b30013edb878 libfdt: Fix kernel-doc comments > cbca977ea121 checks: Allow PCI bridge child nodes without an address > 73e0f143b73d libfdt: fdt_strerror(): Fix comparison warning > 6c2be7d85315 libfdt: fdt_get_string(): Fix sequential write comparison > warnings > 82525f41d59e libfdt: libfdt_wip: Fix comparison warning > fb1f65f15832 libfdt: fdt_create_with_flags(): Fix comparison warning > f28aa271000b libfdt: fdt_move(): Fix comparison warnings > 3d7c6f44195a libfdt: fdt_add_string_(): Fix comparison warning > 10f682788c30 libfdt: fdt_node_offset_by_phandle(): Fix comparison warning > 07158f4cf2a2 libfdt: overlay: Fix comparison warning > ce9e1f25a7de libfdt: fdt_resize(): Fix comparison warning > faa76fc10bc5 libfdt: fdt_splice_(): Fix comparison warning > 54dca0985316 libfdt: fdt_get_string(): Fix comparison warnings > f8e11e61624e libfdt: fdt_grab_space_(): Fix comparison warning > 0c43d4d7bf5a libfdt: fdt_mem_rsv(): Fix comparison warnings > 442ea3dd1579 libfdt: fdt_offset_ptr(): Fix comparison warnings > ca19c3db2bf6 Makefile: Specify cflags for libyaml > 7bb86f1c0956 libfdt: fix fdt_check_node_offset_ w/ VALID_INPUT > 3d522abc7571 dtc: Include stdlib.h in util.h > 808cdaaf524f dtc: Avoid UB when shifting > 3e3138b4a956 libfdt: fix fdt_check_full buffer overrun > 9d7888cbf19c dtc: Consider one-character strings as strings > 8259d59f59de checks: Improve i2c reg property checking > fdabcf2980a4 checks: Remove warning for I2C_OWN_SLAVE_ADDRESS > 2478b1652c8d libfdt: add extern "C" for C++ > f68bfc2668b2 libfdt: trivial typo fix > 7be250b4d059 libfdt: Correct condition for reordering blocks > 81e0919a3e21 checks: Add interrupt provider
OHCI/usb pass through
Hello, We're trying to find out why passing through an usb sound card fails with MacOS/OS X on mac99 and came across some things in OHCI that I don't understand so some help from those who know more about USB handling in QEMU or OHCI would be needed. From traces Howard collected we see that a packet is submitted to libusb which does not complete immediately so it gets recorded as async but never seems to complete afterwards. Meanwhile some isochronous traffic is happening on a different endpoint but it is then rejected with too many pending packets due to the waiting async packet and things seem to stop at this point. There is a comment in hw/usb/hcd-ohci.c:1031 in ohci_service_td() that says this is something that is not modelled correctly as it should allow active packets per endpoint while we only have one packet per controller (but maybe there are other problems than this too). The problem seems to be that currently we have this active packet recorded in OHCIState in these fields: [...] /* Active packets. */ uint32_t old_ctl; USBPacket usb_packet; uint8_t usb_buf[8192]; uint32_t async_td; bool async_complete; void (*ohci_die)(struct OHCIState *ohci); } OHCIState; Then everything in hcd-ohci seems to reuse ohci->usb_packet and I wonder if it can happen that it's overwritten while an async packet is still using it. It seems to be reset calling usb_packet_setup() in two places: ohci_service_td() and ohci_service_iso_td(). While ohci_service_td() checks for ohci->async_td, ohci_service_iso_td() doesn't seem to so maybe it can break the pending async packet if an isochronous request comes in while the other endpoint is waiting for the async packet. If so maybe when the completion is called it won't notice because ohci->usb_packet is already a different packet overwritten by ohci_service_iso_td(). Did I miss some other checks elsewhere that prevent this from happening? (I don't know how USB is handled in QEMU or how OHCI works so it could be I'm not understing this correctly.) In any case to both fix the device model and to avoid this possible problem described above it seems we would need to ditch the packet and async_td fields from OHCIState and move them to the endpoint to allow one active packet per endpoint. We can get the endpoint from a packet and from ohci so I wonder if we can get the active packet from ep->queue (and how to do that) and then can we find out if it's waiting by checking if this packet's status is USB_PACKET_ASYNC so we don't need to keep track of these in OHCIState. I don't understand this code enough to try to do it but maybe some hints could help. Thanks, BALATON Zoltan
Re: [PATCH 1/3] hw/intc/arm_gicv3: Move checking of redist-region-count to arm_gicv3_common_realize
On 9/30/21 17:08, Peter Maydell wrote: > The GICv3 devices have an array property redist-region-count. > Currently we check this for errors (bad values) in > gicv3_init_irqs_and_mmio(), just before we use it. Move this error > checking to the arm_gicv3_common_realize() function, where we > sanity-check all of the other base-class properties. (This will > always be before gicv3_init_irqs_and_mmio() is called, because > that function is called in the subclass realize methods, after > they have called the parent-class realize.) > > The motivation for this refactor is: > * we would like to use the redist_region_count[] values in >arm_gicv3_common_realize() in a subsequent patch, so we need >to have already done the sanity-checking first > * this removes the only use of the Error** argument to >gicv3_init_irqs_and_mmio(), so we can remove some error-handling >boilerplate > > Signed-off-by: Peter Maydell > --- > include/hw/intc/arm_gicv3_common.h | 2 +- > hw/intc/arm_gicv3.c| 6 +- > hw/intc/arm_gicv3_common.c | 26 +- > hw/intc/arm_gicv3_kvm.c| 6 +- > 4 files changed, 16 insertions(+), 24 deletions(-) > > diff --git a/include/hw/intc/arm_gicv3_common.h > b/include/hw/intc/arm_gicv3_common.h > index aa4f0d67703..cb2b0d0ad45 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -306,6 +306,6 @@ struct ARMGICv3CommonClass { > }; > > void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, > - const MemoryRegionOps *ops, Error **errp); > + const MemoryRegionOps *ops); > > #endif > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > index 3f24707838c..bcf54a5f0a5 100644 > --- a/hw/intc/arm_gicv3.c > +++ b/hw/intc/arm_gicv3.c > @@ -393,11 +393,7 @@ static void arm_gic_realize(DeviceState *dev, Error > **errp) > return; > } > > -gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops, &local_err); > -if (local_err) { > -error_propagate(errp, local_err); > -return; > -} > +gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops); > > gicv3_init_cpuif(s); > } > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index 223db16feca..8e47809398b 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -250,22 +250,11 @@ static const VMStateDescription vmstate_gicv3 = { > }; > > void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, > - const MemoryRegionOps *ops, Error **errp) > + const MemoryRegionOps *ops) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(s); > -int rdist_capacity = 0; > int i; > > -for (i = 0; i < s->nb_redist_regions; i++) { > -rdist_capacity += s->redist_region_count[i]; > -} > -if (rdist_capacity < s->num_cpu) { > -error_setg(errp, "Capacity of the redist regions(%d) " > - "is less than number of vcpus(%d)", > - rdist_capacity, s->num_cpu); > -return; > -} > - > /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU. > * GPIO array layout is thus: > * [0..N-1] spi > @@ -308,7 +297,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, > qemu_irq_handler handler, > static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) > { > GICv3State *s = ARM_GICV3_COMMON(dev); > -int i; > +int i, rdist_capacity; > > /* revision property is actually reserved and currently used only in > order > * to keep the interface compatible with GICv2 code, avoiding extra > @@ -350,6 +339,17 @@ static void arm_gicv3_common_realize(DeviceState *dev, > Error **errp) > return; > } > > +rdist_capacity = 0; > +for (i = 0; i < s->nb_redist_regions; i++) { > +rdist_capacity += s->redist_region_count[i]; > +} > +if (rdist_capacity < s->num_cpu) { > +error_setg(errp, "Capacity of the redist regions(%d) " > + "is less than number of vcpus(%d)", > + rdist_capacity, s->num_cpu); > +return; > +} > + > s->cpu = g_new0(GICv3CPUState, s->num_cpu); > > for (i = 0; i < s->num_cpu; i++) { > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c > index 5c09f00dec2..ab58c73306d 100644 > --- a/hw/intc/arm_gicv3_kvm.c > +++ b/hw/intc/arm_gicv3_kvm.c > @@ -787,11 +787,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, > Error **errp) > return; > } > > -gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL, &local_err); > -if (local_err) { > -error_propagate(errp, local_err); > -return; > -} > +gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); > > for (i = 0; i < s->num_cpu; i++) { > ARMCPU *cpu = ARM_CPU(qemu_get_cp
Re: [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit
Hi Taylor, On 9/30/21 23:16, Taylor Simpson wrote: > When a packet has 2 stores, either both commit or neither commit. > At the beginning of gen_commit_packet, we check for multiple stores. > If there are multiple stores, call a helper that will probe each of > them before proceeding with the commit. > > Note that we don't call the probe helper for packets with only one > store. Therefore, we call process_store_log before anything else > involved in committing the packet. > > Test case added in tests/tcg/hexagon/hex_sigsegv.c > > Signed-off-by: Taylor Simpson > > *** Changes in v2 *** > Address feedback from Richard Henderson > - Since we know the value of all the mask at translation time, call > specialized helper > - dczeroa has to be the only store operation in a packet, so we go > ahead and process that first > - When there are two scalar stores, we probe the one in slot 0 - the > call to process_store_log will do slot 1 first, so we don't need > to probe > --- > target/hexagon/helper.h | 2 + > target/hexagon/op_helper.c| 16 ++ > target/hexagon/translate.c| 32 +++- > tests/tcg/hexagon/hex_sigsegv.c | 106 > ++ > tests/tcg/hexagon/Makefile.target | 1 + > 5 files changed, 155 insertions(+), 2 deletions(-) > create mode 100644 tests/tcg/hexagon/hex_sigsegv.c > > diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h > index ca201fb..89de2a3 100644 > --- a/target/hexagon/helper.h > +++ b/target/hexagon/helper.h > @@ -89,3 +89,5 @@ DEF_HELPER_4(sffms_lib, f32, env, f32, f32, f32) > > DEF_HELPER_3(dfmpyfix, f64, env, f64, f64) > DEF_HELPER_4(dfmpyhh, f64, env, f64, f64, f64) > + > +DEF_HELPER_2(probe_pkt_scalar_store_s0, void, env, int) > diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c > index 61d5cde..af32de4 100644 > --- a/target/hexagon/op_helper.c > +++ b/target/hexagon/op_helper.c > @@ -377,6 +377,22 @@ int32_t HELPER(vacsh_pred)(CPUHexagonState *env, > return PeV; > } > > +static void probe_store(CPUHexagonState *env, int slot, int mmu_idx) > +{ > +if (!(env->slot_cancelled & (1 << slot))) { > +size1u_t width = env->mem_log_stores[slot].width; > +target_ulong va = env->mem_log_stores[slot].va; > +uintptr_t ra = GETPC(); > +probe_write(env, va, width, mmu_idx, ra); > +} Matter of taste probably: if (env->slot_cancelled & (1 << slot) { return; } probe_write(env, env->mem_log_stores[slot].va, env->mem_log_stores[slot].width, mmu_idx, GETPC()); > +} > + > +/* Called during packet commit when there are two scalar stores */ > +void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int mmu_idx) > +{ > +probe_store(env, 0, mmu_idx); > +} > + > /* > * mem_noshuf > * Section 5.5 of the Hexagon V67 Programmer's Reference Manual > diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c > index 6fb4e68..8fc2c83 100644 > --- a/target/hexagon/translate.c > +++ b/target/hexagon/translate.c > @@ -471,10 +471,38 @@ static void update_exec_counters(DisasContext *ctx, > Packet *pkt) > > static void gen_commit_packet(DisasContext *ctx, Packet *pkt) > { > +/* > + * If there is more than one store in a packet, make sure they are all OK > + * before proceeding with the rest of the packet commit. > + * > + * dczeroa has to be the only store operation in the packet, so we go > + * ahead and process that first. > + * > + * When there are two scalar stores, we probe the one in slot 0. > + * > + * Note that we don't call the probe helper for packets with only one > + * store. Therefore, we call process_store_log before anything else > + * involved in committing the packet. > + */ > +bool has_store_s0 = pkt->pkt_has_store_s0; > +bool has_store_s1 = (pkt->pkt_has_store_s1 && !ctx->s1_store_processed); > +if (pkt->pkt_has_dczeroa) { > +/* > + * The dczeroa will be the store in slot 0, check that we don't have > + * a store in slot 1. > + */ > +g_assert(has_store_s0 && !has_store_s1); > +process_dczeroa(ctx, pkt); > +} else if (has_store_s0 && has_store_s1) { > +TCGv mem_idx = tcg_const_tl(ctx->mem_idx); > +gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx); > +tcg_temp_free(mem_idx); The index is read-only, so you can avoid the temporary: gen_helper_probe_pkt_scalar_store_s0(cpu_env, tcg_constant_tl(ctx->mem_idx)); Maybe add a (better) comment here: } else { /* default path, all constraints OK, we are good */ > +} > + > +process_store_log(ctx, pkt); > + > gen_reg_writes(ctx); > gen_pred_writes(ctx, pkt); > -process_store_log(ctx, pkt); > -process_dczeroa(ctx, pkt); > update_exec_counters(ctx, pkt); > if (HEX_DEBUG) { >
Running 297 from GitLab CI
Hiya, I was talking this over with Hanna in review to '[PATCH v3 00/16] python/iotests: Run iotest linters during Python CI' [1] and I have some doubt about what you'd personally like to see happen, here. In a nutshell, I split out 'linters.py' from 297 and keep all of the iotest-bits in 297 and all of the generic "run the linters" bits in linters.py, then I run linters.py from the GitLab python CI jobs. I did this so that iotest #297 would continue to work exactly as it had, but trying to serve "two masters" in the form of two test suites means some non-beautiful design decisions. Hanna suggested we just outright drop test 297 to possibly improve the factoring of the tests. I don't want to do that unless you give it the go-ahead, though. I wanted to hear your feelings on if we still want to keep 297 around or not. --js [1] https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05787.html
[PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit
When a packet has 2 stores, either both commit or neither commit. At the beginning of gen_commit_packet, we check for multiple stores. If there are multiple stores, call a helper that will probe each of them before proceeding with the commit. Note that we don't call the probe helper for packets with only one store. Therefore, we call process_store_log before anything else involved in committing the packet. Test case added in tests/tcg/hexagon/hex_sigsegv.c Signed-off-by: Taylor Simpson *** Changes in v2 *** Address feedback from Richard Henderson - Since we know the value of all the mask at translation time, call specialized helper - dczeroa has to be the only store operation in a packet, so we go ahead and process that first - When there are two scalar stores, we probe the one in slot 0 - the call to process_store_log will do slot 1 first, so we don't need to probe --- target/hexagon/helper.h | 2 + target/hexagon/op_helper.c| 16 ++ target/hexagon/translate.c| 32 +++- tests/tcg/hexagon/hex_sigsegv.c | 106 ++ tests/tcg/hexagon/Makefile.target | 1 + 5 files changed, 155 insertions(+), 2 deletions(-) create mode 100644 tests/tcg/hexagon/hex_sigsegv.c diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h index ca201fb..89de2a3 100644 --- a/target/hexagon/helper.h +++ b/target/hexagon/helper.h @@ -89,3 +89,5 @@ DEF_HELPER_4(sffms_lib, f32, env, f32, f32, f32) DEF_HELPER_3(dfmpyfix, f64, env, f64, f64) DEF_HELPER_4(dfmpyhh, f64, env, f64, f64, f64) + +DEF_HELPER_2(probe_pkt_scalar_store_s0, void, env, int) diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c index 61d5cde..af32de4 100644 --- a/target/hexagon/op_helper.c +++ b/target/hexagon/op_helper.c @@ -377,6 +377,22 @@ int32_t HELPER(vacsh_pred)(CPUHexagonState *env, return PeV; } +static void probe_store(CPUHexagonState *env, int slot, int mmu_idx) +{ +if (!(env->slot_cancelled & (1 << slot))) { +size1u_t width = env->mem_log_stores[slot].width; +target_ulong va = env->mem_log_stores[slot].va; +uintptr_t ra = GETPC(); +probe_write(env, va, width, mmu_idx, ra); +} +} + +/* Called during packet commit when there are two scalar stores */ +void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int mmu_idx) +{ +probe_store(env, 0, mmu_idx); +} + /* * mem_noshuf * Section 5.5 of the Hexagon V67 Programmer's Reference Manual diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c index 6fb4e68..8fc2c83 100644 --- a/target/hexagon/translate.c +++ b/target/hexagon/translate.c @@ -471,10 +471,38 @@ static void update_exec_counters(DisasContext *ctx, Packet *pkt) static void gen_commit_packet(DisasContext *ctx, Packet *pkt) { +/* + * If there is more than one store in a packet, make sure they are all OK + * before proceeding with the rest of the packet commit. + * + * dczeroa has to be the only store operation in the packet, so we go + * ahead and process that first. + * + * When there are two scalar stores, we probe the one in slot 0. + * + * Note that we don't call the probe helper for packets with only one + * store. Therefore, we call process_store_log before anything else + * involved in committing the packet. + */ +bool has_store_s0 = pkt->pkt_has_store_s0; +bool has_store_s1 = (pkt->pkt_has_store_s1 && !ctx->s1_store_processed); +if (pkt->pkt_has_dczeroa) { +/* + * The dczeroa will be the store in slot 0, check that we don't have + * a store in slot 1. + */ +g_assert(has_store_s0 && !has_store_s1); +process_dczeroa(ctx, pkt); +} else if (has_store_s0 && has_store_s1) { +TCGv mem_idx = tcg_const_tl(ctx->mem_idx); +gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx); +tcg_temp_free(mem_idx); +} + +process_store_log(ctx, pkt); + gen_reg_writes(ctx); gen_pred_writes(ctx, pkt); -process_store_log(ctx, pkt); -process_dczeroa(ctx, pkt); update_exec_counters(ctx, pkt); if (HEX_DEBUG) { TCGv has_st0 = diff --git a/tests/tcg/hexagon/hex_sigsegv.c b/tests/tcg/hexagon/hex_sigsegv.c new file mode 100644 index 000..dc2b349 --- /dev/null +++ b/tests/tcg/hexagon/hex_sigsegv.c @@ -0,0 +1,106 @@ +/* + * Copyright(c) 2021 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more
Re: [PATCH v3] nbd/server: Add --selinux-label option
On Thu, Sep 30, 2021 at 11:54:58AM +0300, Vladimir Sementsov-Ogievskiy wrote: > 9/30/21 11:47, Richard W.M. Jones wrote: > > Under SELinux, Unix domain sockets have two labels. One is on the > > disk and can be set with commands such as chcon(1). There is a > > different label stored in memory (called the process label). This can > > only be set by the process creating the socket. When using SELinux + > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > you must set both labels correctly first. > > > > For qemu-nbd the options to set the second label are awkward. You can > > create the socket in a wrapper program and then exec into qemu-nbd. > > Or you could try something with LD_PRELOAD. > > > > This commit adds the ability to set the label straightforwardly on the > > command line, via the new --selinux-label flag. (The name of the flag > > is the same as the equivalent nbdkit option.) > > > > A worked example showing how to use the new option can be found in > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Signed-off-by: Richard W.M. Jones > > Reviewed-by: Daniel P. Berrangé > > Signed-off-by: Eric Blake > > this should be Reviewed-by? In this case, I think S-o-b is actually correct: I did make some tweaks to Rich's v2 while preparing my pull request, so Rich is crediting my addition to his work. And at the time of my pull request that included his v2 (before it got dropped for 32-bit build problems), I had not actually sent my R-b, because I was already trusting the R-b present from other reviewers. Oddly enough, even if Rich had dropped my S-o-b line, it will still eventually reappear, since I'll be queuing this patch through my NBD tree which requires me to touch it again. So already having it now doesn't hurt. [Many of the patches that go through my tree end up with both my R-b and S-o-b; but there are patches where I have S-o-b but not R-b, because I trusted the review of others, and view the act of a careful review as orthogonal from the responsibility of touching a patch enough to include it in a pull request] > > > --- > > configure | 8 +++- > > meson.build | 10 - > > meson_options.txt | 3 ++ > > qemu-nbd.c| 39 +++ > > [..] > > > } > > @@ -938,6 +952,19 @@ int main(int argc, char **argv) > > } else { > > backlog = MIN(shared, SOMAXCONN); > > } > > +if (sockpath && selinux_label) { > > 1. Why only for unix sockets? > > 2. If [1] is intentional, why silently ignore the new option for ip sockets, > shouldn't error-out instead? > Good point, and I missed it in v2, in spite of my touching that patch to avoid silent ignoring when selinux support was not compiled in. So at this point, I'm less certain whether it is smarter to reject --selinux-label on non-unix sockets, or whether we try to do the labeling regardless of socket type; and thus consider it premature for me to give R-b until we have that resolved. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH v4 13/13] qapi/parser: enable pylint checks
Signed-off-by: John Snow --- This can be merged with the previous commit, if desired. Signed-off-by: John Snow --- scripts/qapi/pylintrc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index 5b7dbc58ad8..b259531a726 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -2,8 +2,7 @@ # Add files or directories matching the regex patterns to the ignore list. # The regex matches against base names, not paths. -ignore-patterns=parser.py, -schema.py, +ignore-patterns=schema.py, [MESSAGES CONTROL] -- 2.31.1
[PATCH v4 12/13] qapi/parser: Silence too-few-public-methods warning
Eh. Not worth the fuss today. There are bigger fish to fry. Signed-off-by: John Snow --- scripts/qapi/parser.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 0265b47b955..1b006cdc133 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -461,8 +461,10 @@ class QAPIDoc: """ class Section: +# pylint: disable=too-few-public-methods def __init__(self, parser: QAPISchemaParser, name: Optional[str] = None, indent: int = 0): + # parser, for error messages about indentation self._parser = parser # optional section name (argument/member or section name) @@ -498,6 +500,7 @@ class NullSection(Section): """ Immutable dummy section for use at the end of a doc block. """ +# pylint: disable=too-few-public-methods def append(self, line: str) -> None: assert False, "Text appended after end_comment() called." -- 2.31.1
[PATCH v4 11/13] qapi/parser: enable mypy checks
Signed-off-by: John Snow --- As always, this can be merged with the previous commit. Signed-off-by: John Snow --- scripts/qapi/mypy.ini | 5 - 1 file changed, 5 deletions(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 54ca4483d6d..66253564297 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -3,11 +3,6 @@ strict = True disallow_untyped_calls = False python_version = 3.6 -[mypy-qapi.parser] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False - [mypy-qapi.schema] disallow_untyped_defs = False disallow_incomplete_defs = False -- 2.31.1
[PATCH v4 07/13] qapi/parser: Introduce NullSection
Here's the weird bit. QAPIDoc generally expects -- virtually everywhere -- that it will always have a current section. The sole exception to this is in the case that end_comment() is called, which leaves us with *no* section. However, in this case, we also don't expect to actually ever mutate the comment contents ever again. NullSection is just a Null-object that allows us to maintain the invariant that we *always* have a current section, enforced by static typing -- allowing us to type that field as QAPIDoc.Section instead of the more ambiguous Optional[QAPIDoc.Section]. end_section is renamed to switch_section and now accepts as an argument the new section to activate, clarifying that no callers ever just unilaterally end a section; they only do so when starting a new section. Signed-off-by: John Snow --- For my money: Optional types can be a nuisance because an unfamiliar reader may wonder in what circumstances the field may be unset. This makes the condition quite a bit more explicit and statically provable. Doing it in this way (and not by creating a mutable dummy section) will also continue to reject (rather noisily) any erroneous attempts to append additional lines after end_comment() has been called. Also, this section isn't indexed into .sections[] and isn't really visible in any way to external users of the class, so it seems like a harmless and low-cost way to formalize the "life cycle" of a QAPIDoc parser. Clean and clear as I can make it, in as few lines as I could muster. Signed-off-by: John Snow --- scripts/qapi/parser.py | 27 --- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 82f1d952b13..40c5da4b172 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0): def connect(self, member): self.member = member +class NullSection(Section): +""" +Immutable dummy section for use at the end of a doc block. +""" +def append(self, line): +assert False, "Text appended after end_comment() called." + def __init__(self, parser, info): # self._parser is used to report errors with QAPIParseError. The # resulting error position depends on the state of the parser. @@ -525,7 +532,7 @@ def append(self, line): self._append_line(line) def end_comment(self): -self._end_section() +self._switch_section(QAPIDoc.NullSection(self._parser)) @staticmethod def _is_section_tag(name): @@ -699,9 +706,9 @@ def _start_symbol_section(self, symbols_dict, name, indent): raise QAPIParseError(self._parser, "'%s' parameter name duplicated" % name) assert not self.sections -self._end_section() -self._section = QAPIDoc.ArgSection(self._parser, name, indent) -symbols_dict[name] = self._section +new_section = QAPIDoc.ArgSection(self._parser, name, indent) +self._switch_section(new_section) +symbols_dict[name] = new_section def _start_args_section(self, name, indent): self._start_symbol_section(self.args, name, indent) @@ -713,13 +720,11 @@ def _start_section(self, name=None, indent=0): if name in ('Returns', 'Since') and self.has_section(name): raise QAPIParseError(self._parser, "duplicated '%s' section" % name) -self._end_section() -self._section = QAPIDoc.Section(self._parser, name, indent) -self.sections.append(self._section) - -def _end_section(self): -assert self._section is not None +new_section = QAPIDoc.Section(self._parser, name, indent) +self._switch_section(new_section) +self.sections.append(new_section) +def _switch_section(self, new_section): text = self._section.text = self._section.text.strip() # Only the 'body' section is allowed to have an empty body. @@ -732,7 +737,7 @@ def _end_section(self): self._parser, "empty doc section '%s'" % self._section.name) -self._section = None +self._section = new_section def _append_freeform(self, line): match = re.match(r'(@\S+:)', line) -- 2.31.1
[PATCH v4 09/13] qapi/parser: add type hint annotations (QAPIDoc)
Annotations do not change runtime behavior. This commit consists of only annotations. Signed-off-by: John Snow --- scripts/qapi/parser.py | 67 -- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 75582ddb003..73c1c4ef599 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -37,6 +37,9 @@ from .schema import QAPISchemaFeature, QAPISchemaMember +#: Represents a single Top Level QAPI schema expression. +TopLevelExpr = Dict[str, object] + # Return value alias for get_expr(). _ExprValue = Union[List[object], Dict[str, object], str, bool] @@ -454,7 +457,8 @@ class QAPIDoc: """ class Section: -def __init__(self, parser, name=None, indent=0): +def __init__(self, parser: QAPISchemaParser, + name: Optional[str] = None, indent: int = 0): # parser, for error messages about indentation self._parser = parser # optional section name (argument/member or section name) @@ -463,7 +467,7 @@ def __init__(self, parser, name=None, indent=0): # the expected indent level of the text of this section self._indent = indent -def append(self, line): +def append(self, line: str) -> None: # Strip leading spaces corresponding to the expected indent level # Blank lines are always OK. if line: @@ -478,7 +482,8 @@ def append(self, line): self.text += line.rstrip() + '\n' class ArgSection(Section): -def __init__(self, parser, name, indent=0): +def __init__(self, parser: QAPISchemaParser, + name: str, indent: int = 0): super().__init__(parser, name, indent) self.member: Optional['QAPISchemaMember'] = None @@ -489,35 +494,34 @@ class NullSection(Section): """ Immutable dummy section for use at the end of a doc block. """ -def append(self, line): +def append(self, line: str) -> None: assert False, "Text appended after end_comment() called." -def __init__(self, parser, info): +def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo): # self._parser is used to report errors with QAPIParseError. The # resulting error position depends on the state of the parser. # It happens to be the beginning of the comment. More or less # servicable, but action at a distance. self._parser = parser self.info = info -self.symbol = None +self.symbol: Optional[str] = None self.body = QAPIDoc.Section(parser) -# dict mapping parameter name to ArgSection -self.args = OrderedDict() -self.features = OrderedDict() -# a list of Section -self.sections = [] +# dicts mapping parameter/feature names to their ArgSection +self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict() +self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict() +self.sections: List[QAPIDoc.Section] = [] # the current section self._section = self.body self._append_line = self._append_body_line -def has_section(self, name): +def has_section(self, name: str) -> bool: """Return True if we have a section with this name.""" for i in self.sections: if i.name == name: return True return False -def append(self, line): +def append(self, line: str) -> None: """ Parse a comment line and add it to the documentation. @@ -538,18 +542,18 @@ def append(self, line): line = line[1:] self._append_line(line) -def end_comment(self): +def end_comment(self) -> None: self._switch_section(QAPIDoc.NullSection(self._parser)) @staticmethod -def _is_section_tag(name): +def _is_section_tag(name: str) -> bool: return name in ('Returns:', 'Since:', # those are often singular or plural 'Note:', 'Notes:', 'Example:', 'Examples:', 'TODO:') -def _append_body_line(self, line): +def _append_body_line(self, line: str) -> None: """ Process a line of documentation text in the body section. @@ -591,7 +595,7 @@ def _append_body_line(self, line): # This is a free-form documentation block self._append_freeform(line) -def _append_args_line(self, line): +def _append_args_line(self, line: str) -> None: """ Process a line of documentation text in an argument section. @@ -637,7 +641,7 @@ def _append_args_line(self, line): self._append_freeform(line) -def _append_features_line(self, line): +def _append_features_line(self, line: str) -> None: name =
[PATCH v4 04/13] qapi: Add spaces after symbol declaration for consistency
Several QGA definitions omit a blank line after the symbol declaration. This works OK currently, but it's the only place where we do this. Adjust it for consistency. Future commits may wind up enforcing this formatting. Signed-off-by: John Snow --- This isn't strictly necessary and I don't actually get around to enforcing it in this series, but I figured I'd share it with the list anyway. We can just drop this patch but I wanted to see your thoughts. Signed-off-by: John Snow --- qapi/block-core.json| 1 + qga/qapi-schema.json| 3 +++ tests/qapi-schema/doc-good.json | 8 3 files changed, 12 insertions(+) diff --git a/qapi/block-core.json b/qapi/block-core.json index 4114f8b6fc3..52a6dae9522 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3132,6 +3132,7 @@ ## # @BlockdevQcow2EncryptionFormat: +# # @aes: AES-CBC with plain64 initialization vectors # # Since: 2.10 diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index c60f5e669d7..94e4aacdcc6 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -1140,6 +1140,7 @@ ## # @GuestExec: +# # @pid: pid of child process in guest OS # # Since: 2.5 @@ -1171,6 +1172,7 @@ ## # @GuestHostName: +# # @host-name: Fully qualified domain name of the guest OS # # Since: 2.10 @@ -1197,6 +1199,7 @@ ## # @GuestUser: +# # @user: Username # @domain: Logon domain (windows only) # @login-time: Time of login of this user on the computer. If multiple diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json index a20acffd8b9..86dc25d2bd8 100644 --- a/tests/qapi-schema/doc-good.json +++ b/tests/qapi-schema/doc-good.json @@ -53,6 +53,7 @@ ## # @Enum: +# # @one: The _one_ {and only} # # Features: @@ -67,6 +68,7 @@ ## # @Base: +# # @base1: # the first member ## @@ -75,6 +77,7 @@ ## # @Variant1: +# # A paragraph # # Another paragraph (but no @var: line) @@ -91,11 +94,13 @@ ## # @Variant2: +# ## { 'struct': 'Variant2', 'data': {} } ## # @Object: +# # Features: # @union-feat1: a feature ## @@ -109,6 +114,7 @@ ## # @Alternate: +# # @i: an integer # @b is undocumented # @@ -126,6 +132,7 @@ ## # @cmd: +# # @arg1: the first argument # # @arg2: the second @@ -175,6 +182,7 @@ ## # @EVT_BOXED: +# # Features: # @feat3: a feature ## -- 2.31.1
[PATCH v4 10/13] qapi/parser: Add FIXME for consolidating JSON-related types
The fix for this comment is forthcoming in a future commit, but this will keep me honest. The linting configuration in ./python/setup.cfg prohibits 'FIXME' comments. A goal of this long-running series is to move ./scripts/qapi to ./python/qemu/qapi so that the QAPI generator is regularly type-checked by GitLab CI. This comment is a time-bomb to force me to address this issue prior to that step. Signed-off-by: John Snow --- scripts/qapi/parser.py | 4 1 file changed, 4 insertions(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 73c1c4ef599..0265b47b955 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -43,6 +43,10 @@ # Return value alias for get_expr(). _ExprValue = Union[List[object], Dict[str, object], str, bool] +# FIXME: Consolidate and centralize definitions for TopLevelExpr, +# _ExprValue, _JSONValue, and _JSONObject; currently scattered across +# several modules. + class QAPIParseError(QAPISourceError): """Error class for all QAPI schema parsing errors.""" -- 2.31.1
[PATCH v4 06/13] qapi/parser: clarify _end_section() logic
The "if self._section" clause in end_section is mysterious: In which circumstances might we end a section when we don't have one? QAPIDoc always expects there to be a "current section", only except after a call to end_comment(). This actually *shouldn't* ever be 'None', so let's remove that logic so I don't wonder why it's like this again in three months. Signed-off-by: John Snow --- scripts/qapi/parser.py | 22 +++--- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 23898ab1dcd..82f1d952b13 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -718,13 +718,21 @@ def _start_section(self, name=None, indent=0): self.sections.append(self._section) def _end_section(self): -if self._section: -text = self._section.text = self._section.text.strip() -if self._section.name and (not text or text.isspace()): -raise QAPIParseError( -self._parser, -"empty doc section '%s'" % self._section.name) -self._section = None +assert self._section is not None + +text = self._section.text = self._section.text.strip() + +# Only the 'body' section is allowed to have an empty body. +# All other sections, including anonymous ones, must have text. +if self._section != self.body and not text: +# We do not create anonymous sections unless there is +# something to put in them; this is a parser bug. +assert self._section.name +raise QAPIParseError( +self._parser, +"empty doc section '%s'" % self._section.name) + +self._section = None def _append_freeform(self, line): match = re.match(r'(@\S+:)', line) -- 2.31.1
[PATCH v4 08/13] qapi/parser: add import cycle workaround
Adding static types causes a cycle in the QAPI generator: [schema -> expr -> parser -> schema]. It exists because the QAPIDoc class needs the names of types defined by the schema module, but the schema module needs to import both expr.py/parser.py to do its actual parsing. Ultimately, the layering violation is that parser.py should not have any knowledge of specifics of the Schema. QAPIDoc performs double-duty here both as a parser *and* as a finalized object that is part of the schema. In this patch, add the offending type hints alongside the workaround to avoid the cycle becoming a problem at runtime. See https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles for more information on this workaround technique. I see three ultimate resolutions here: (1) Just keep this patch and use the TYPE_CHECKING trick to eliminate the cycle which is only present during static analysis. (2) Don't bother to annotate connect_member() et al, give them 'object' or 'Any'. I don't particularly like this, because it diminishes the usefulness of type hints for documentation purposes. Still, it's an extremely quick fix. (3) Reimplement doc <--> definition correlation directly in schema.py, integrating doc fields directly into QAPISchemaMember and relieving the QAPIDoc class of the responsibility. Users of the information would instead visit the members first and retrieve their documentation instead of the inverse operation -- visiting the documentation and retrieving their members. My preference is (3), but in the short-term (1) is the easiest way to have my cake (strong type hints) and eat it too (Not have import cycles). Do (1) for now, but plan for (3). Signed-off-by: John Snow --- scripts/qapi/parser.py | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 40c5da4b172..75582ddb003 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -18,6 +18,7 @@ import os import re from typing import ( +TYPE_CHECKING, Dict, List, Optional, @@ -30,6 +31,12 @@ from .source import QAPISourceInfo +if TYPE_CHECKING: +# pylint: disable=cyclic-import +# TODO: Remove cycle. [schema -> expr -> parser -> schema] +from .schema import QAPISchemaFeature, QAPISchemaMember + + # Return value alias for get_expr(). _ExprValue = Union[List[object], Dict[str, object], str, bool] @@ -473,9 +480,9 @@ def append(self, line): class ArgSection(Section): def __init__(self, parser, name, indent=0): super().__init__(parser, name, indent) -self.member = None +self.member: Optional['QAPISchemaMember'] = None -def connect(self, member): +def connect(self, member: 'QAPISchemaMember') -> None: self.member = member class NullSection(Section): @@ -747,14 +754,14 @@ def _append_freeform(self, line): % match.group(1)) self._section.append(line) -def connect_member(self, member): +def connect_member(self, member: 'QAPISchemaMember') -> None: if member.name not in self.args: # Undocumented TODO outlaw self.args[member.name] = QAPIDoc.ArgSection(self._parser, member.name) self.args[member.name].connect(member) -def connect_feature(self, feature): +def connect_feature(self, feature: 'QAPISchemaFeature') -> None: if feature.name not in self.features: raise QAPISemError(feature.info, "feature '%s' lacks documentation" -- 2.31.1
[PATCH v4 00/13] qapi: static typing conversion, pt5b
Hello darkness my old friend; This is part five (b), and focuses on QAPIDoc in parser.py. GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5b CI: https://gitlab.com/jsnow/qemu/-/pipelines/380464863 Note: intentional trailing whitespace in a QAPI schema test causes a warning on the `check-patch` CI test. Ignore it. Requirements: - Python 3.6+ - mypy >= 0.770 - pylint >= 2.6.0 (2.7.0+ when using Python 3.9+) Every commit should pass with: - `isort -c qapi/` - `flake8 qapi/` - `pylint --rcfile=qapi/pylintrc qapi/` - `mypy --config-file=qapi/mypy.ini qapi/` V4: 005/13:[0006] [FC] 'qapi/parser: remove FIXME comment from _append_body_line' 006/13:[down] 'qapi/parser: clarify _end_section() logic' 007/13:[0004] [FC] 'qapi/parser: Introduce NullSection' 010/13:[down] 'qapi/parser: Add FIXME for consolidating JSON-related types' - Dropped formerly-patch-05, for now. - Added a new FIXME comment to keep myself honest for pt5c O:-) - (05) Adjusted a commend and the parser error message - (06) Changed commit title for 06 ("simplify" => "clarify") - (07) Adjust comment. John Snow (13): qapi/pylintrc: ignore 'consider-using-f-string' warning qapi/gen: use dict.items() to iterate over _modules qapi/parser: fix unused check_args_section arguments qapi: Add spaces after symbol declaration for consistency qapi/parser: remove FIXME comment from _append_body_line qapi/parser: clarify _end_section() logic qapi/parser: Introduce NullSection qapi/parser: add import cycle workaround qapi/parser: add type hint annotations (QAPIDoc) qapi/parser: Add FIXME for consolidating JSON-related types qapi/parser: enable mypy checks qapi/parser: Silence too-few-public-methods warning qapi/parser: enable pylint checks qapi/block-core.json | 1 + qga/qapi-schema.json | 3 + scripts/qapi/gen.py| 3 +- scripts/qapi/mypy.ini | 5 - scripts/qapi/parser.py | 152 - scripts/qapi/pylintrc | 4 +- tests/qapi-schema/doc-bad-feature.err | 2 +- tests/qapi-schema/doc-empty-symbol.err | 2 +- tests/qapi-schema/doc-good.json| 8 ++ 9 files changed, 114 insertions(+), 66 deletions(-) -- 2.31.1
[PATCH v4 03/13] qapi/parser: fix unused check_args_section arguments
Pylint informs us we're not using these arguments. Oops, it's right. Correct the error message and remove the remaining unused parameter. Fix test output now that the error message is improved. Fixes: e151941d1b Signed-off-by: John Snow --- scripts/qapi/parser.py| 16 +--- tests/qapi-schema/doc-bad-feature.err | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index f03ba2cfec8..bfd2dbfd9a2 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -753,16 +753,18 @@ def check_expr(self, expr): def check(self): -def check_args_section(args, info, what): +def check_args_section(args, what): bogus = [name for name, section in args.items() if not section.member] if bogus: raise QAPISemError( self.info, -"documented member%s '%s' %s not exist" -% ("s" if len(bogus) > 1 else "", - "', '".join(bogus), - "do" if len(bogus) > 1 else "does")) +"documented %s%s '%s' %s not exist" % ( +what, +"s" if len(bogus) > 1 else "", +"', '".join(bogus), +"do" if len(bogus) > 1 else "does" +)) -check_args_section(self.args, self.info, 'members') -check_args_section(self.features, self.info, 'features') +check_args_section(self.args, 'member') +check_args_section(self.features, 'feature') diff --git a/tests/qapi-schema/doc-bad-feature.err b/tests/qapi-schema/doc-bad-feature.err index e4c62adfa3e..49d1746c3d1 100644 --- a/tests/qapi-schema/doc-bad-feature.err +++ b/tests/qapi-schema/doc-bad-feature.err @@ -1 +1 @@ -doc-bad-feature.json:3: documented member 'a' does not exist +doc-bad-feature.json:3: documented feature 'a' does not exist -- 2.31.1
[PATCH v4 01/13] qapi/pylintrc: ignore 'consider-using-f-string' warning
Pylint 2.11.x adds this warning. We're not yet ready to pursue that conversion, so silence it for now. Signed-off-by: John Snow --- scripts/qapi/pylintrc | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index c5275d5f59b..5b7dbc58ad8 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -23,6 +23,7 @@ disable=fixme, too-many-branches, too-many-statements, too-many-instance-attributes, +consider-using-f-string, [REPORTS] -- 2.31.1
[PATCH v4 05/13] qapi/parser: remove FIXME comment from _append_body_line
True, we do not check the validity of this symbol -- but we don't check the validity of definition names during parse, either -- that happens later, during the expr check. I don't want to introduce a dependency on expr.py:check_name_str here and introduce a cycle. Instead, rest assured that a documentation block is required for each definition. This requirement uses the names of each section to ensure that we fulfilled this requirement. e.g., let's say that block-core.json has a comment block for "Snapshot!Info" by accident. We'll see this error message: In file included from ../../qapi/block.json:8: ../../qapi/block-core.json: In struct 'SnapshotInfo': ../../qapi/block-core.json:38: documentation comment is for 'Snapshot!Info' That's a pretty decent error message. Now, let's say that we actually mangle it twice, identically: ../../qapi/block-core.json: In struct 'Snapshot!Info': ../../qapi/block-core.json:38: struct has an invalid name That's also pretty decent. If we forget to fix it in both places, we'll just be back to the first error. Therefore, let's just drop this FIXME and adjust the error message to not imply a more thorough check than is actually performed. Signed-off-by: John Snow --- scripts/qapi/parser.py | 6 -- tests/qapi-schema/doc-empty-symbol.err | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index bfd2dbfd9a2..23898ab1dcd 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -556,9 +556,11 @@ def _append_body_line(self, line): if not line.endswith(':'): raise QAPIParseError(self._parser, "line should end with ':'") self.symbol = line[1:-1] -# FIXME invalid names other than the empty string aren't flagged +# Invalid names are not checked here, but the name provided MUST +# match the following definition, which *is* validated in expr.py. if not self.symbol: -raise QAPIParseError(self._parser, "invalid name") +raise QAPIParseError( +self._parser, "name required after '@'") elif self.symbol: # This is a definition documentation block if name.startswith('@') and name.endswith(':'): diff --git a/tests/qapi-schema/doc-empty-symbol.err b/tests/qapi-schema/doc-empty-symbol.err index 81b90e882a7..aa51be41b2d 100644 --- a/tests/qapi-schema/doc-empty-symbol.err +++ b/tests/qapi-schema/doc-empty-symbol.err @@ -1 +1 @@ -doc-empty-symbol.json:4:1: invalid name +doc-empty-symbol.json:4:1: name required after '@' -- 2.31.1
[PATCH v4 02/13] qapi/gen: use dict.items() to iterate over _modules
New pylint warning. I could silence it, but this is the only occurrence in the entire tree, including everything in iotests/ and python/. Easier to just change this one instance. (The warning is emitted in cases where you are fetching the values anyway, so you may as well just take advantage of the iterator to avoid redundant lookups.) Signed-off-by: John Snow --- scripts/qapi/gen.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index ab26d5c937a..2ec1e7b3b68 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -296,10 +296,9 @@ def _temp_module(self, name: str) -> Iterator[None]: self._current_module = old_module def write(self, output_dir: str, opt_builtins: bool = False) -> None: -for name in self._module: +for name, (genc, genh) in self._module.items(): if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: continue -(genc, genh) = self._module[name] genc.write(output_dir) genh.write(output_dir) -- 2.31.1
Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
On Wed, Sep 29, 2021 at 01:30:50AM -0400, ~farzon wrote: > From: Farzon Lotfi Food for thought: your git/mail configuration used one address for the envelope (name '~farzon' as user 'farzon@') and another as the patch author (name 'Farzon Lotfi' as user 'hi@'). Since you own your domain (with its own perks), you can get away with it, but it looks a bit less professional to need a second From: line to override the mail author (which is more commonly needed to work around overly strict DKIM settings), compared to just sending your mail from the desired full-name author in the first place. But since your Signed-off-by tag is correct, this is not a show-stopper to applying your patch. However, my next comment does require a respin before your patch would be ready. Your Subject: line is too long, as evidenced by your choice of using sentences. It should really be 'category: short description' all within 60 characters or so (when 'git log' displays indentation, a short commit id, and then your subject, you don't want your subject truncated). It feels like some of your subject should instead be part of the commit body, where you currently have only... > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371 ...because that one line as a body is rather sparse. While the URL is nice (it is a lifesaver for tracking whether a particular bug has a known patch), it does not tell me at a glance what is behind that URL, and I don't want to have to fire up my browser to learn about your patch. In general, the subject should be a short "what", and the commit body should be "why" a maintainer should apply it. I'd suggest the following: block: Replace TABs with space Bring the block file in line with the QEMU coding style, with spaces for indentation. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371 > > Signed-off-by: Farzon Lotfi > --- -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
On Thu, Sep 30, 2021 at 02:55:16PM +0200, Kevin Wolf wrote: > > > When we're changing these lines anyway, let's make sure to have > > > consistent alignment with the surrounding code. So I would prefer > > > something like: > > > > > > +.bdrv_close = parallels_close, > > > .bdrv_child_perm = bdrv_default_perms, > > > > > > Rather than: > > > > > > +.bdrv_close = parallels_close, > > > .bdrv_child_perm = bdrv_default_perms, > > > > > > In most cases, there are already inconsistencies in the BlockDriver > > > definitions, but let's use the chance to make it a little better. > > > > > > Or may be drop alignment around '=' at all, to have > > > >.bdrv_child_perm = bdrv_default_perms, > >.bdrv_co_block_status = parallels_co_block_status, > >.bdrv_has_zero_init = bdrv_has_zero_init_1, > > > > ? > > You're right that this would make it easy to keep things consistent, but > I think it hurts readability a lot, even compared to the current, often > inconsistent state. I agree that the alignment adds a bit of readability, but also understand that it adds a maintenacne burden. Thus, in code I manage, I'm fine with either style (no extra spaces, or '=' lined up); and can live with different styles in different files (which I then will honor when doing a grunt-work patch that touches all of the block drivers). But what I don't like is when a single file cannot be consistent with itself on which of those two styles it is using - a file that uses aligned '=' really needs to put that '=' far enough to the right that an added long-named member doesn't cause frequent reindentation of the rest of the members. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH] failover: allow to pause the VM during the migration
On 9/30/21 1:09 PM, Laurent Vivier wrote: If we want to save a snapshot of a VM to a file, we used to follow the following steps: 1- stop the VM: (qemu) stop 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" 3- resume the VM: (qemu) cont After that we can restore the snapshot with: qemu-system-x86_64 ... -incoming "exec:cat snapshot" (qemu) cont This is the basics of what libvirt does for a snapshot, and steps 1+2 are what it does for a "managedsave" (where it saves the snapshot to disk and then terminates the qemu process, for later re-animation). In those cases, it seems like this new parameter could work for us - instead of explicitly pausing the guest prior to migrating it to disk, we would set this new parameter to on, then directly migrate-to-disk (relying on qemu to do the pause). Care will need to be taken to assure that error recovery behaves the same though. There are a couple of cases when libvirt apparently *doesn't* pause the guest during the migrate-to-disk, both having to do with saving a coredump of the guest. Since I really have no idea of how common/important that is (or even if my assessment of the code is correct), I'm Cc'ing this patch to libvir-list to make sure it catches the attention of someone who knows the answers and implications. But when failover is configured, it doesn't work anymore. As the failover needs to ask the guest OS to unplug the card the machine cannot be paused. This patch introduces a new migration parameter, "pause-vm", that asks the migration to pause the VM during the migration startup phase after the the card is unplugged. Once the migration is done, we only need to resume the VM with "cont" and the card is plugged back: 1- set the parameter: (qemu) migrate_set_parameter pause-vm on 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" The primary failover card (VFIO) is unplugged and the VM is paused. 3- resume the VM: (qemu) cont The VM restarts and the primary failover card is plugged back The VM state sent in the migration stream is "paused", it means when the snapshot is loaded or if the stream is sent to a destination QEMU, the VM needs to be resumed manually. Signed-off-by: Laurent Vivier --- qapi/migration.json| 20 +++--- include/hw/virtio/virtio-net.h | 1 + hw/net/virtio-net.c| 33 ++ migration/migration.c | 37 +- monitor/hmp-cmds.c | 8 5 files changed, 95 insertions(+), 4 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index 88f07baedd06..86284d96ad2a 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -743,6 +743,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## { 'enum': 'MigrationParameter', @@ -758,7 +762,7 @@ 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', 'multifd-zlib-level' ,'multifd-zstd-level', - 'block-bitmap-mapping' ] } + 'block-bitmap-mapping', 'pause-vm' ] } ## # @MigrateSetParameters: @@ -903,6 +907,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## # TODO either fuse back into MigrationParameters, or make @@ -934,7 +942,8 @@ '*multifd-compression': 'MultiFDCompression', '*multifd-zlib-level': 'uint8', '*multifd-zstd-level': 'uint8', -'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } } +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ], +'*pause-vm': 'bool' } } ## # @migrate-set-parameters: @@ -1099,6 +1108,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -1128,7 +1141,8 @@ '*multifd-compression': 'MultiFDCompression', '*multifd-zlib-level': 'uint8',
Re: [PULL v2 00/33] x86 and misc changes for 2021-09-28
On Thu, 30 Sept 2021 at 16:00, Paolo Bonzini wrote: > > The following changes since commit ba0fa56bc06e563de68d2a2bf3ddb0cfea1be4f9: > > Merge remote-tracking branch > 'remotes/vivier/tags/q800-for-6.2-pull-request' into staging (2021-09-29 > 21:20:49 +0100) > > are available in the Git repository at: > > https://gitlab.com/bonzini/qemu.git tags/for-upstream > > for you to fetch changes up to c1de5858bd39b299d3d8baec38b0376bed7f19e8: > > meson_options.txt: Switch the default value for the vnc option to 'auto' > (2021-09-30 15:30:25 +0200) > > > * SGX implementation for x86 > * Miscellaneous bugfixes > * Fix dependencies from ROMs to qtests > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.2 for any user-visible changes. -- PMM
Re: [PATCH 14/16] tests/acceptance/ppc_prep_40p.py: NetBSD 7.1.2 location update
On 9/30/21 21:19, Reinoud Zandijk wrote: > On Mon, Sep 27, 2021 at 05:26:43PM +0200, Philippe Mathieu-Daudé wrote: >> On 9/24/21 20:55, Cleber Rosa wrote: >>> The NetBSD-7.1.2-prep.iso is no longer available on the CDN, but it's >>> still available in the archive. >>> >>> Let's update its location so that users without the file on cache can >>> still fetch it and run the test. >>> >>> Signed-off-by: Cleber Rosa >>> --- >>> tests/acceptance/ppc_prep_40p.py | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> Reviewed-by: Philippe Mathieu-Daudé >> Tested-by: Philippe Mathieu-Daudé > > No objections but is there a reason why it is still using NetBSD 7.1.2? I am > not a ppc guy but as a NetBSD developer just curious. We know that QEMU 40p machine successfully boots NetBSD 7.1.2 and we don't want regression with it. It could boot more recent versions but I haven't tested. Tests are welcome :) There should be quite easy to add. Regards, Phil.
Re: [PATCH 14/16] tests/acceptance/ppc_prep_40p.py: NetBSD 7.1.2 location update
On Mon, Sep 27, 2021 at 05:26:43PM +0200, Philippe Mathieu-Daudé wrote: > On 9/24/21 20:55, Cleber Rosa wrote: > > The NetBSD-7.1.2-prep.iso is no longer available on the CDN, but it's > > still available in the archive. > > > > Let's update its location so that users without the file on cache can > > still fetch it and run the test. > > > > Signed-off-by: Cleber Rosa > > --- > > tests/acceptance/ppc_prep_40p.py | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé No objections but is there a reason why it is still using NetBSD 7.1.2? I am not a ppc guy but as a NetBSD developer just curious. Reinoud
Re: [PATCH v1] Use CLOCK_MONOTONIC_RAW if available for get_clock().
Peter, Thanks for the quick response. I've informally socialized the issue and will update this thread when I get more information. That aside, I'd find using CLOCK_MONOTONIC_RAW valuable if, e.g., I wanted test an NTP daemon inside of a guest and didn't want the host providing an already-adjusted timebase. Would the behavior from my patch be more appropriate as a command-line option? v/r Joe On 9/30/21, 12:11 PM, "Peter Maydell" wrote: On Thu, 30 Sept 2021 at 17:04, Joe Tanen wrote: > > CLOCK_MONOTONIC_RAW provides an unadjusted system clock on some platforms, > which is closer in spirit to providing a guest with a raw hardware clock than > CLOCK_MONOTONIC. > > Using CLOCK_MONOTONIC_RAW also works around a current issue in OSX where > CLOCK_MONOTONIC has been observed to go backwards. > > Since CLOCK_MONOTONIC_RAW might not be available on all platforms, revert to > using CLOCK_MONOTONIC if it is not present. > > Signed-off-by: Joe Tanen I'm not sure we want to change behaviour everywhere to work around an OSX bug, though... Has this bug been reported to Apple ? Is there some kind of bug report ID or URL we can quote in the commit message ? -- PMM
Re: [PATCH v3] nbd/server: Add --selinux-label option
9/30/21 21:37, Richard W.M. Jones wrote: On Thu, Sep 30, 2021 at 02:00:11PM -0300, Willian Rampazzo wrote: On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy wrote: 9/30/21 11:47, Richard W.M. Jones wrote: Under SELinux, Unix domain sockets have two labels. One is on the disk and can be set with commands such as chcon(1). There is a different label stored in memory (called the process label). This can only be set by the process creating the socket. When using SELinux + SVirt and wanting qemu to be able to connect to a qemu-nbd instance, you must set both labels correctly first. For qemu-nbd the options to set the second label are awkward. You can create the socket in a wrapper program and then exec into qemu-nbd. Or you could try something with LD_PRELOAD. This commit adds the ability to set the label straightforwardly on the command line, via the new --selinux-label flag. (The name of the flag is the same as the equivalent nbdkit option.) A worked example showing how to use the new option can be found in this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 Signed-off-by: Richard W.M. Jones Reviewed-by: Daniel P. Berrangé Signed-off-by: Eric Blake this should be Reviewed-by? Maybe, because of this: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html I got confused with this v3. Yes, I'd somehow lost the original patch and picked it up from Eric's queue to make v3. Than it's probably correct. Still a bit strange to send own patch with another s-o-b in the end. Having said that I'm not sure what the objection above means. Do you mean Eric's tag should be Reviewed-by instead of S-o-b? (And why?) I thought you just accidentally used s-o-b instead of r-b. -- Best regards, Vladimir
[PATCH 2/3] virtio-iommu: Default to bypass during boot
Currently the virtio-iommu device must be programmed before it allows DMA from any PCI device. This can make the VM entirely unusable when a virtio-iommu driver isn't present, for example in a bootloader that loads the OS from storage. Similarly to the other vIOMMU implementations, default to DMA bypassing the IOMMU during boot. Add a "boot-bypass" option that lets users change this behavior. Signed-off-by: Jean-Philippe Brucker --- include/hw/virtio/virtio-iommu.h | 1 + hw/virtio/virtio-iommu.c | 28 +++- hw/virtio/trace-events | 4 ++-- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h index 273e35c04b..4c66989ca4 100644 --- a/include/hw/virtio/virtio-iommu.h +++ b/include/hw/virtio/virtio-iommu.h @@ -58,6 +58,7 @@ struct VirtIOIOMMU { GTree *domains; QemuMutex mutex; GTree *endpoints; +bool boot_bypass; }; #endif diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 1b23e8e18c..82edeaa101 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -728,8 +728,7 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr, .perm = IOMMU_NONE, }; -bypass_allowed = virtio_vdev_has_feature(&s->parent_obj, - VIRTIO_IOMMU_F_BYPASS); +bypass_allowed = s->config.bypass; sid = virtio_iommu_get_bdf(sdev); @@ -828,7 +827,8 @@ static void virtio_iommu_get_config(VirtIODevice *vdev, uint8_t *config_data) config->input_range.start, config->input_range.end, config->domain_range.end, - config->probe_size); + config->probe_size, + config->bypass); memcpy(config_data, &dev->config, sizeof(struct virtio_iommu_config)); } @@ -836,13 +836,29 @@ static void virtio_iommu_set_config(VirtIODevice *vdev, const uint8_t *config_data) { struct virtio_iommu_config config; +VirtIOIOMMU *dev = VIRTIO_IOMMU(vdev); memcpy(&config, config_data, sizeof(struct virtio_iommu_config)); + +if (config.bypass != dev->config.bypass) { +if (!virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS_CONFIG)) { +virtio_error(vdev, "cannot set config.bypass"); +return; +} +if (config.bypass != 0 && config.bypass != 1) { +warn_report("invalid config.bypass value '%d'", config.bypass); +dev->config.bypass = 0; +return; +} +dev->config.bypass = config.bypass; +} + trace_virtio_iommu_set_config(config.page_size_mask, config.input_range.start, config.input_range.end, config.domain_range.end, - config.probe_size); + config.probe_size, + config.bypass); } static uint64_t virtio_iommu_get_features(VirtIODevice *vdev, uint64_t f, @@ -986,6 +1002,7 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp) s->config.input_range.end = -1UL; s->config.domain_range.end = 32; s->config.probe_size = VIOMMU_PROBE_SIZE; +s->config.bypass = s->boot_bypass; virtio_add_feature(&s->features, VIRTIO_RING_F_EVENT_IDX); virtio_add_feature(&s->features, VIRTIO_RING_F_INDIRECT_DESC); @@ -993,9 +1010,9 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp) virtio_add_feature(&s->features, VIRTIO_IOMMU_F_INPUT_RANGE); virtio_add_feature(&s->features, VIRTIO_IOMMU_F_DOMAIN_RANGE); virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MAP_UNMAP); -virtio_add_feature(&s->features, VIRTIO_IOMMU_F_BYPASS); virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MMIO); virtio_add_feature(&s->features, VIRTIO_IOMMU_F_PROBE); +virtio_add_feature(&s->features, VIRTIO_IOMMU_F_BYPASS_CONFIG); qemu_mutex_init(&s->mutex); @@ -1169,6 +1186,7 @@ static const VMStateDescription vmstate_virtio_iommu = { static Property virtio_iommu_properties[] = { DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus, "PCI", PCIBus *), +DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index 8ed19e9d0c..6bc3821ba3 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -90,8 +90,8 @@ virtio_mmio_setting_irq(int level) "virtio_mmio setting IRQ %d" virtio_iommu_device_reset(void) "reset!" virtio_iommu_get_features(uint64_t features) "device supports features=0x%"PRIx64 virtio_iommu_device_status(uint8_t status) "driver statu
[PATCH 0/3] virtio-iommu: Support VIRTIO_IOMMU_F_BYPASS_CONFIG
Replace the VIRTIO_IOMMU_F_BYPASS feature with VIRTIO_IOMMU_F_BYPASS_CONFIG, which enables a config space bit to switch global bypass on and off. Add a boot-bypass option, which defaults to 'on' to be in line with other vIOMMUs and to allow running firmware/bootloader that are unaware of the IOMMU. See the spec change for more rationale https://lists.oasis-open.org/archives/virtio-dev/202109/msg00137.html Jean-Philippe Brucker (3): NOMERGE: virtio-iommu: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG virtio-iommu: Default to bypass during boot virtio-iommu: Support bypass domain include/hw/virtio/virtio-iommu.h | 1 + include/standard-headers/linux/virtio_iommu.h | 10 +++- hw/virtio/virtio-iommu.c | 60 --- hw/virtio/trace-events| 4 +- 4 files changed, 64 insertions(+), 11 deletions(-) -- 2.33.0
[PATCH 3/3] virtio-iommu: Support bypass domain
The driver can create a bypass domain by passing the VIRTIO_IOMMU_ATTACH_F_BYPASS flag on the ATTACH request. Bypass domains perform slightly better than domains with identity mappings since they skip translation. Signed-off-by: Jean-Philippe Brucker --- hw/virtio/virtio-iommu.c | 32 ++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 82edeaa101..4f0207a3eb 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -42,6 +42,7 @@ typedef struct VirtIOIOMMUDomain { uint32_t id; +bool bypass; GTree *mappings; QLIST_HEAD(, VirtIOIOMMUEndpoint) endpoint_list; } VirtIOIOMMUDomain; @@ -257,12 +258,16 @@ static void virtio_iommu_put_endpoint(gpointer data) } static VirtIOIOMMUDomain *virtio_iommu_get_domain(VirtIOIOMMU *s, - uint32_t domain_id) + uint32_t domain_id, + bool bypass) { VirtIOIOMMUDomain *domain; domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id)); if (domain) { +if (domain->bypass != bypass) { +return NULL; +} return domain; } domain = g_malloc0(sizeof(*domain)); @@ -270,6 +275,7 @@ static VirtIOIOMMUDomain *virtio_iommu_get_domain(VirtIOIOMMU *s, domain->mappings = g_tree_new_full((GCompareDataFunc)interval_cmp, NULL, (GDestroyNotify)g_free, (GDestroyNotify)g_free); +domain->bypass = bypass; g_tree_insert(s->domains, GUINT_TO_POINTER(domain_id), domain); QLIST_INIT(&domain->endpoint_list); trace_virtio_iommu_get_domain(domain_id); @@ -333,11 +339,16 @@ static int virtio_iommu_attach(VirtIOIOMMU *s, { uint32_t domain_id = le32_to_cpu(req->domain); uint32_t ep_id = le32_to_cpu(req->endpoint); +uint32_t flags = le32_to_cpu(req->flags); VirtIOIOMMUDomain *domain; VirtIOIOMMUEndpoint *ep; trace_virtio_iommu_attach(domain_id, ep_id); +if (flags & ~VIRTIO_IOMMU_ATTACH_F_BYPASS) { +return VIRTIO_IOMMU_S_INVAL; +} + ep = virtio_iommu_get_endpoint(s, ep_id); if (!ep) { return VIRTIO_IOMMU_S_NOENT; @@ -355,7 +366,12 @@ static int virtio_iommu_attach(VirtIOIOMMU *s, } } -domain = virtio_iommu_get_domain(s, domain_id); +domain = virtio_iommu_get_domain(s, domain_id, + flags & VIRTIO_IOMMU_ATTACH_F_BYPASS); +if (!domain) { +/* Incompatible flags */ +return VIRTIO_IOMMU_S_INVAL; +} QLIST_INSERT_HEAD(&domain->endpoint_list, ep, next); ep->domain = domain; @@ -418,6 +434,10 @@ static int virtio_iommu_map(VirtIOIOMMU *s, return VIRTIO_IOMMU_S_NOENT; } +if (domain->bypass) { +return VIRTIO_IOMMU_S_INVAL; +} + interval = g_malloc0(sizeof(*interval)); interval->low = virt_start; @@ -463,6 +483,11 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s, if (!domain) { return VIRTIO_IOMMU_S_NOENT; } + +if (domain->bypass) { +return VIRTIO_IOMMU_S_INVAL; +} + interval.low = virt_start; interval.high = virt_end; @@ -779,6 +804,9 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr, entry.perm = flag; } goto unlock; +} else if (ep->domain->bypass) { +entry.perm = flag; +goto unlock; } found = g_tree_lookup_extended(ep->domain->mappings, (gpointer)(&interval), -- 2.33.0
[PATCH 1/3] NOMERGE: virtio-iommu: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
Pull VIRTIO_IOMMU_F_BYPASS_CONFIG changes from Linux (not upstream yet). Signed-off-by: Jean-Philippe Brucker --- include/standard-headers/linux/virtio_iommu.h | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/include/standard-headers/linux/virtio_iommu.h b/include/standard-headers/linux/virtio_iommu.h index b9443b83a1..d14808e3fb 100644 --- a/include/standard-headers/linux/virtio_iommu.h +++ b/include/standard-headers/linux/virtio_iommu.h @@ -13,9 +13,10 @@ #define VIRTIO_IOMMU_F_INPUT_RANGE 0 #define VIRTIO_IOMMU_F_DOMAIN_RANGE1 #define VIRTIO_IOMMU_F_MAP_UNMAP 2 -#define VIRTIO_IOMMU_F_BYPASS 3 +#define VIRTIO_IOMMU_F_BYPASS 3 /* Deprecated */ #define VIRTIO_IOMMU_F_PROBE 4 #define VIRTIO_IOMMU_F_MMIO5 +#define VIRTIO_IOMMU_F_BYPASS_CONFIG 6 struct virtio_iommu_range_64 { uint64_tstart; @@ -36,6 +37,8 @@ struct virtio_iommu_config { struct virtio_iommu_range_32domain_range; /* Probe buffer size */ uint32_tprobe_size; + uint8_t bypass; + uint8_t reserved[7]; }; /* Request types */ @@ -66,11 +69,14 @@ struct virtio_iommu_req_tail { uint8_t reserved[3]; }; +#define VIRTIO_IOMMU_ATTACH_F_BYPASS (1 << 0) + struct virtio_iommu_req_attach { struct virtio_iommu_req_headhead; uint32_tdomain; uint32_tendpoint; - uint8_t reserved[8]; + uint32_tflags; + uint8_t reserved[4]; struct virtio_iommu_req_tailtail; }; -- 2.33.0
Re: [PATCH v3] nbd/server: Add --selinux-label option
On Thu, Sep 30, 2021 at 02:00:11PM -0300, Willian Rampazzo wrote: > On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy > wrote: > > > > 9/30/21 11:47, Richard W.M. Jones wrote: > > > Under SELinux, Unix domain sockets have two labels. One is on the > > > disk and can be set with commands such as chcon(1). There is a > > > different label stored in memory (called the process label). This can > > > only be set by the process creating the socket. When using SELinux + > > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > > you must set both labels correctly first. > > > > > > For qemu-nbd the options to set the second label are awkward. You can > > > create the socket in a wrapper program and then exec into qemu-nbd. > > > Or you could try something with LD_PRELOAD. > > > > > > This commit adds the ability to set the label straightforwardly on the > > > command line, via the new --selinux-label flag. (The name of the flag > > > is the same as the equivalent nbdkit option.) > > > > > > A worked example showing how to use the new option can be found in > > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > Signed-off-by: Richard W.M. Jones > > > Reviewed-by: Daniel P. Berrangé > > > Signed-off-by: Eric Blake > > > > this should be Reviewed-by? > > Maybe, because of this: > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html > > I got confused with this v3. Yes, I'd somehow lost the original patch and picked it up from Eric's queue to make v3. Having said that I'm not sure what the objection above means. Do you mean Eric's tag should be Reviewed-by instead of S-o-b? (And why?) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Re: [PATCH v3 05/13] qapi/parser: improve detection of '@symbol:' preface
On Thu, Sep 30, 2021 at 4:42 AM Markus Armbruster wrote: > John Snow writes: > > > Leading and trailing whitespace are now discarded, addressing the FIXME > > comment. A new error is raised to detect this accidental case. > > > > Parsing for args sections is left alone here; the 'name' variable is > > moved into the only block where it is used. > > > > Signed-off-by: John Snow > > > > --- > > > > Tangentially related to delinting in that removing 'FIXME' comments is a > > goal for pylint. My goal is to allow 'TODO' to be checked in, but > > 'FIXME' should be fixed prior to inclusion. > > > > Arbitrary, but that's life for you. > > > > Signed-off-by: John Snow > > --- > > scripts/qapi/parser.py | 13 - > > tests/qapi-schema/doc-whitespace-leading-symbol.err | 1 + > > .../qapi-schema/doc-whitespace-leading-symbol.json | 6 ++ > > tests/qapi-schema/doc-whitespace-leading-symbol.out | 0 > > .../qapi-schema/doc-whitespace-trailing-symbol.err | 1 + > > .../qapi-schema/doc-whitespace-trailing-symbol.json | 6 ++ > > .../qapi-schema/doc-whitespace-trailing-symbol.out | 0 > > tests/qapi-schema/meson.build | 2 ++ > > 8 files changed, 24 insertions(+), 5 deletions(-) > > create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.err > > create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.json > > create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.out > > create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.err > > create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.json > > create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.out > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index bfd2dbfd9a2..2f93a752f66 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -549,18 +549,21 @@ def _append_body_line(self, line): > > > > Else, append the line to the current section. > > """ > > -name = line.split(' ', 1)[0] > > -# FIXME not nice: things like '# @foo:' and '# @foo: ' aren't > > -# recognized, and get silently treated as ordinary text > > -if not self.symbol and not self.body.text and > line.startswith('@'): > > -if not line.endswith(':'): > > +stripped = line.strip() > > + > > +if not self.symbol and not self.body.text and > stripped.startswith('@'): > > +if not stripped.endswith(':'): > > raise QAPIParseError(self._parser, "line should end > with ':'") > > +if not stripped == line: > > +raise QAPIParseError( > > +self._parser, "extra whitespace around symbol > declaration") > > This rejects both leading and trailing whitespace. Rejecting leading > whitespace is good. Rejecting trailing whitespace feels a bit pedantic, > and it might not extend to the related case I'll point out below. > > err'd on the conservative side. Wasn't sure how permissive we really wanted to be. > Have you considered a regexp instead? Say > >match = re.match(r'(\s*)@([^:]*)(:?)(\s*)(.*)$', line) > > Then match.group(n) is > > n=1 leading whitespace, if any > n=2 symbol > n=3 trailing colon, if any > n=4 trailing whitespace, if any > n=5 trailing text, if any > > Omit the subgroups you don't need. > > Sensible, for a more comprehensive refactoring. > > self.symbol = line[1:-1] > > # FIXME invalid names other than the empty string aren't > flagged > > if not self.symbol: > > raise QAPIParseError(self._parser, "invalid name") > > elif self.symbol: > > # This is a definition documentation block > > +name = line.split(' ', 1)[0] > > if name.startswith('@') and name.endswith(':'): > > self._append_line = self._append_args_line > > self._append_args_line(line) > > Same issue here, and in _append_args_line(). To reproduce, I hacked up > doc-good.json like so > > diff --git a/tests/qapi-schema/doc-good.json > b/tests/qapi-schema/doc-good.json > index 86dc25d2bd..977fcbad48 100644 > --- a/tests/qapi-schema/doc-good.json > +++ b/tests/qapi-schema/doc-good.json > @@ -133,7 +133,7 @@ > ## > # @cmd: > # > -# @arg1: the first argument > +# @arg1: the first argument > # > # @arg2: the second > #argument > > and got > > $ PYTHONPATH=/work/armbru/qemu/scripts python3 > /work/armbru/qemu/tests/qapi-schema/test-qapi.py -d tests/qapi-schema > doc-good.json > doc-good FAIL > --- tests/qapi-schema/doc-good.out > +++ > @@ -149,12 +149,12 @@ > == Another subsection > doc symbol=cmd > body= > - > -arg=arg1 > -the first argument > +@arg1: the first argument > arg=arg2 >
Re: [PATCH v3 06/13] qapi/parser: remove FIXME comment from _append_body_line
On Thu, Sep 30, 2021 at 4:47 AM Markus Armbruster wrote: > John Snow writes: > > > True, we do not check the validity of this symbol -- but we don't check > > the validity of definition names during parse, either -- that happens > > later, during the expr check. I don't want to introduce a dependency on > > expr.py:check_name_str here and introduce a cycle. > > > > Instead, rest assured that a documentation block is required for each > > definition. This requirement uses the names of each section to ensure > > that we fulfilled this requirement. > > > > e.g., let's say that block-core.json has a comment block for > > "Snapshot!Info" by accident. We'll see this error message: > > > > In file included from ../../qapi/block.json:8: > > ../../qapi/block-core.json: In struct 'SnapshotInfo': > > ../../qapi/block-core.json:38: documentation comment is for > 'Snapshot!Info' > > > > That's a pretty decent error message. > > > > Now, let's say that we actually mangle it twice, identically: > > > > ../../qapi/block-core.json: In struct 'Snapshot!Info': > > ../../qapi/block-core.json:38: struct has an invalid name > > > > That's also pretty decent. If we forget to fix it in both places, we'll > > just be back to the first error. > > > > Therefore, let's just drop this FIXME and adjust the error message to > > not imply a more thorough check than is actually performed. > > > > Signed-off-by: John Snow > > --- > > scripts/qapi/parser.py | 6 -- > > tests/qapi-schema/doc-empty-symbol.err | 2 +- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 2f93a752f66..52748e8e462 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -558,9 +558,11 @@ def _append_body_line(self, line): > > raise QAPIParseError( > > self._parser, "extra whitespace around symbol > declaration") > > self.symbol = line[1:-1] > > -# FIXME invalid names other than the empty string aren't > flagged > > +# Invalid names are not checked here, but the name provided > MUST > > +# match the following definition, which *is* validated. > > if not self.symbol: > > -raise QAPIParseError(self._parser, "invalid name") > > +raise QAPIParseError( > > +self._parser, "doc symbol name cannot be blank") > > "blank" feels like "empty or just whitespace" to me. We actually mean > "empty". > > What about "name required after @"? > > Sure, yeah. Updated. > > elif self.symbol: > > # This is a definition documentation block > > name = line.split(' ', 1)[0] > > diff --git a/tests/qapi-schema/doc-empty-symbol.err > b/tests/qapi-schema/doc-empty-symbol.err > > index 81b90e882a7..a4981ee28ea 100644 > > --- a/tests/qapi-schema/doc-empty-symbol.err > > +++ b/tests/qapi-schema/doc-empty-symbol.err > > @@ -1 +1 @@ > > -doc-empty-symbol.json:4:1: invalid name > > +doc-empty-symbol.json:4:1: doc symbol name cannot be blank > >
Re: [PATCH v3 09/13] qapi/parser: add import cycle workaround
On Thu, Sep 30, 2021 at 5:45 AM Markus Armbruster wrote: > John Snow writes: > > > There is a cycle that exists in the QAPI generator: [schema -> expr -> > > "There is" or "there will be once we add strong type hints"? > > "There exists in my mind-palace a cycle where, ..." (Will adjust the commit message.) > > parser -> schema]. It exists because the QAPIDoc class needs the names > > of types defined by the schema module, but the schema module needs to > > import both expr.py/parser.py to do its actual parsing. > > > > Ultimately, the layering violation is that parser.py should not have any > > knowledge of specifics of the Schema. QAPIDoc performs double-duty here > > both as a parser *and* as a finalized object that is part of the schema. > > > > I see three paths here: > > > > (1) Just use the TYPE_CHECKING trick to eliminate the cycle which is only > > present during static analysis. > > > > (2) Don't bother to annotate connect_member() et al, give them 'object' > > or 'Any'. I don't particularly like this, because it diminishes the > > usefulness of type hints for documentation purposes. Still, it's an > > extremely quick fix. > > > > (3) Reimplement doc <--> definition correlation directly in schema.py, > > integrating doc fields directly into QAPISchemaMember and relieving > > the QAPIDoc class of the responsibility. Users of the information > > would instead visit the members first and retrieve their > > documentation instead of the inverse operation -- visiting the > > documentation and retrieving their members. > > > > I prefer (3), but (1) is the easiest way to have my cake (strong type > > hints) and eat it too (Not have import cycles). Do (1) for now, but plan > > for (3). See also: > > > https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles > > > > Signed-off-by: John Snow > > --- > > scripts/qapi/parser.py | 15 +++ > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 123fc2f099c..30b1d98df0b 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -18,6 +18,7 @@ > > import os > > import re > > from typing import ( > > +TYPE_CHECKING, > > Dict, > > List, > > Optional, > > @@ -30,6 +31,12 @@ > > from .source import QAPISourceInfo > > > > > > +if TYPE_CHECKING: > > +# pylint: disable=cyclic-import > > +# TODO: Remove cycle. [schema -> expr -> parser -> schema] > > +from .schema import QAPISchemaFeature, QAPISchemaMember > > + > > + > > # Return value alias for get_expr(). > > _ExprValue = Union[List[object], Dict[str, object], str, bool] > > > > @@ -473,9 +480,9 @@ def append(self, line): > > class ArgSection(Section): > > def __init__(self, parser, name, indent=0): > > super().__init__(parser, name, indent) > > -self.member = None > > +self.member: Optional['QAPISchemaMember'] = None > > > > -def connect(self, member): > > +def connect(self, member: 'QAPISchemaMember') -> None: > > self.member = member > > > > class NullSection(Section): > > @@ -750,14 +757,14 @@ def _append_freeform(self, line): > > % match.group(1)) > > self._section.append(line) > > > > -def connect_member(self, member): > > +def connect_member(self, member: 'QAPISchemaMember') -> None: > > if member.name not in self.args: > > # Undocumented TODO outlaw > > self.args[member.name] = QAPIDoc.ArgSection(self._parser, > > member.name) > > self.args[member.name].connect(member) > > > > -def connect_feature(self, feature): > > +def connect_feature(self, feature: 'QAPISchemaFeature') -> None: > > if feature.name not in self.features: > > raise QAPISemError(feature.info, > > "feature '%s' lacks documentation" > > This adds just the type hints that cause the cycle. I like that, > because it illustrates the cycle. Would be nice if the commit message > mentioned this, perhaps > > I prefer (3), but (1) is the easiest way to have my cake (strong type > hints) and eat it too (Not have import cycles). Do (1) for now, but plan > for (3). Also add the type hints that cause the cycle right away to > illustrate. See also: > > https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles > > Slightly nicer, I think, would be swapping this and the next patch. > Then that one's commit message needs to say something like "except for a > few problematic ones, which the next commit will add". Worthwhile? Up > to you. > > Doing it the other way around means you can't squash the mypy patch into the bulk-type-hints patch, but I think the git log usefulness is not better or worse either way around. (Reviewer usefulness is
[PATCH] failover: allow to pause the VM during the migration
If we want to save a snapshot of a VM to a file, we used to follow the following steps: 1- stop the VM: (qemu) stop 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" 3- resume the VM: (qemu) cont After that we can restore the snapshot with: qemu-system-x86_64 ... -incoming "exec:cat snapshot" (qemu) cont But when failover is configured, it doesn't work anymore. As the failover needs to ask the guest OS to unplug the card the machine cannot be paused. This patch introduces a new migration parameter, "pause-vm", that asks the migration to pause the VM during the migration startup phase after the the card is unplugged. Once the migration is done, we only need to resume the VM with "cont" and the card is plugged back: 1- set the parameter: (qemu) migrate_set_parameter pause-vm on 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" The primary failover card (VFIO) is unplugged and the VM is paused. 3- resume the VM: (qemu) cont The VM restarts and the primary failover card is plugged back The VM state sent in the migration stream is "paused", it means when the snapshot is loaded or if the stream is sent to a destination QEMU, the VM needs to be resumed manually. Signed-off-by: Laurent Vivier --- qapi/migration.json| 20 +++--- include/hw/virtio/virtio-net.h | 1 + hw/net/virtio-net.c| 33 ++ migration/migration.c | 37 +- monitor/hmp-cmds.c | 8 5 files changed, 95 insertions(+), 4 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index 88f07baedd06..86284d96ad2a 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -743,6 +743,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## { 'enum': 'MigrationParameter', @@ -758,7 +762,7 @@ 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', 'multifd-zlib-level' ,'multifd-zstd-level', - 'block-bitmap-mapping' ] } + 'block-bitmap-mapping', 'pause-vm' ] } ## # @MigrateSetParameters: @@ -903,6 +907,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## # TODO either fuse back into MigrationParameters, or make @@ -934,7 +942,8 @@ '*multifd-compression': 'MultiFDCompression', '*multifd-zlib-level': 'uint8', '*multifd-zstd-level': 'uint8', -'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } } +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ], +'*pause-vm': 'bool' } } ## # @migrate-set-parameters: @@ -1099,6 +1108,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -1128,7 +1141,8 @@ '*multifd-compression': 'MultiFDCompression', '*multifd-zlib-level': 'uint8', '*multifd-zstd-level': 'uint8', -'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } } +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ], +'*pause-vm': 'bool' } } ## # @query-migrate-parameters: diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h index 824a69c23f06..a6c186e28b45 100644 --- a/include/hw/virtio/virtio-net.h +++ b/include/hw/virtio/virtio-net.h @@ -210,6 +210,7 @@ struct VirtIONet { bool failover; DeviceListener primary_listener; Notifier migration_state; +VMChangeStateEntry *vm_state; VirtioNetRssData rss_data; struct NetRxPkt *rx_pkt; struct EBPFRSSContext ebpf_rss; diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index f205331dcf8c..c83364eac47b 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -39,6 +39,7 @@ #include "migration/misc.h" #include "standard-headers/linux/ethtool.h" #include "sysemu/sysemu.h" +#include "sysemu/runstate.h" #include "trace.h" #include "mon
Re: [PATCH 2/2] tests/docker: Fix fedora-i386-cross
On Thu, Sep 30, 2021 at 12:36:36PM -0400, Richard Henderson wrote: > By using PKG_CONFIG_PATH instead of PKG_CONFIG_LIBDIR, > we were still including the 64-bit packages. Install > pcre-devel.i686 to fill a missing glib2 dependency. > > By using --extra-cflags instead of --cpu, we incorrectly > use the wrong probing during meson. > > Cc: Alex Bennée > Cc: Paolo Bonzini > Cc: Daniel P. Berrangé > Cc: Richard W.M. Jones > Signed-off-by: Richard Henderson > --- > tests/docker/dockerfiles/fedora-i386-cross.docker | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3] nbd/server: Add --selinux-label option
On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy wrote: > > 9/30/21 11:47, Richard W.M. Jones wrote: > > Under SELinux, Unix domain sockets have two labels. One is on the > > disk and can be set with commands such as chcon(1). There is a > > different label stored in memory (called the process label). This can > > only be set by the process creating the socket. When using SELinux + > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > you must set both labels correctly first. > > > > For qemu-nbd the options to set the second label are awkward. You can > > create the socket in a wrapper program and then exec into qemu-nbd. > > Or you could try something with LD_PRELOAD. > > > > This commit adds the ability to set the label straightforwardly on the > > command line, via the new --selinux-label flag. (The name of the flag > > is the same as the equivalent nbdkit option.) > > > > A worked example showing how to use the new option can be found in > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Signed-off-by: Richard W.M. Jones > > Reviewed-by: Daniel P. Berrangé > > Signed-off-by: Eric Blake > > this should be Reviewed-by? Maybe, because of this: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html I got confused with this v3.
Re: [PATCH 1/2] tests/docker: Remove fedora-i386-cross from DOCKER_PARTIAL_IMAGES
On Thu, Sep 30, 2021 at 12:36:35PM -0400, Richard Henderson wrote: > The image was upgraded to a full image in ee381b7fe146. > This makes it possible to use docker-test@image syntax > with this container. > > Cc: Thomas Huth > Cc: Alex Bennée > Signed-off-by: Richard Henderson > --- > tests/docker/Makefile.include | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 08/13] qapi/parser: Introduce NullSection
On Thu, Sep 30, 2021 at 5:35 AM Markus Armbruster wrote: > John Snow writes: > > > Here's the weird bit. QAPIDoc generally expects -- virtually everywhere > > -- that it will always have a current section. The sole exception to > > this is in the case that end_comment() is called, which leaves us with > > *no* section. However, in this case, we also don't expect to actually > > ever mutate the comment contents ever again. > > > > NullSection is just a Null-object that allows us to maintain the > > invariant that we *always* have a current section, enforced by static > > typing -- allowing us to type that field as QAPIDoc.Section instead of > > the more ambiguous Optional[QAPIDoc.Section]. > > > > end_section is renamed to switch_section and now accepts as an argument > > the new section to activate, clarifying that no callers ever just > > unilaterally end a section; they only do so when starting a new section. > > > > Signed-off-by: John Snow > > > > --- > > > > For my money: Optional types can be a nuisance because an unfamiliar > > reader may wonder in what circumstances the field may be unset. This > > makes the condition quite a bit more explicit and statically provable. > > > > Doing it in this way (and not by creating a dummy section) will also > ("And not by creating a [mutable] dummy section" ... but this wasn't destined for the git log anyway.) > > continue to reject (rather noisily) any erroneous attempts to append > > additional lines after end_comment() has been called. > > > > Also, this section isn't indexed into .sections[] and isn't really > > visible in any way to external users of the class, so it seems like a > > harmless and low-cost way to formalize the "life cycle" of a QAPIDoc > > parser. > > > > Clean and clear as I can make it, in as few lines as I could muster. > > > > Signed-off-by: John Snow > > --- > > scripts/qapi/parser.py | 27 --- > > 1 file changed, 16 insertions(+), 11 deletions(-) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 1fdc5bc7056..123fc2f099c 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0): > > def connect(self, member): > > self.member = member > > > > +class NullSection(Section): > > +""" > > +Empty section that signifies the end of a doc block. > > What about "Dummy section for use at the end of a doc block"? > > Sure. "Immutable dummy section for use at the end of a doc block." > > +""" > > +def append(self, line): > > +assert False, "BUG: Text appended after end_comment() > called." > > How can a failing assertion *not* be a bug? > > Haha. I'll drop the prefix. (I'll update my branch with these tiny edits and you can decide what you'd like to do with that.) > > + > > def __init__(self, parser, info): > > # self._parser is used to report errors with QAPIParseError. > The > > # resulting error position depends on the state of the parser. > > @@ -525,7 +532,7 @@ def append(self, line): > > self._append_line(line) > > > > def end_comment(self): > > -self._end_section() > > +self._switch_section(QAPIDoc.NullSection(self._parser)) > > > > @staticmethod > > def _is_section_tag(name): > > @@ -702,9 +709,9 @@ def _start_symbol_section(self, symbols_dict, name, > indent): > > raise QAPIParseError(self._parser, > > "'%s' parameter name duplicated" % > name) > > assert not self.sections > > -self._end_section() > > -self._section = QAPIDoc.ArgSection(self._parser, name, indent) > > -symbols_dict[name] = self._section > > +new_section = QAPIDoc.ArgSection(self._parser, name, indent) > > +self._switch_section(new_section) > > +symbols_dict[name] = new_section > > > > def _start_args_section(self, name, indent): > > self._start_symbol_section(self.args, name, indent) > > @@ -716,13 +723,11 @@ def _start_section(self, name=None, indent=0): > > if name in ('Returns', 'Since') and self.has_section(name): > > raise QAPIParseError(self._parser, > > "duplicated '%s' section" % name) > > -self._end_section() > > -self._section = QAPIDoc.Section(self._parser, name, indent) > > -self.sections.append(self._section) > > - > > -def _end_section(self): > > -assert self._section is not None > > +new_section = QAPIDoc.Section(self._parser, name, indent) > > +self._switch_section(new_section) > > +self.sections.append(new_section) > > > > +def _switch_section(self, new_section): > > text = self._section.text = self._section.text.strip() > > > > # Only the 'body' section is allowed to have an empty body. > > @@ -735,7 +740,7 @@ def _end_section(se
Re: [PATCH 2/2] tests/docker: Fix fedora-i386-cross
On Thu, Sep 30, 2021 at 12:36:36PM -0400, Richard Henderson wrote: > By using PKG_CONFIG_PATH instead of PKG_CONFIG_LIBDIR, > we were still including the 64-bit packages. Install > pcre-devel.i686 to fill a missing glib2 dependency. > > By using --extra-cflags instead of --cpu, we incorrectly > use the wrong probing during meson. > > Cc: Alex Bennée > Cc: Paolo Bonzini > Cc: Daniel P. Berrangé > Cc: Richard W.M. Jones > Signed-off-by: Richard Henderson > --- > tests/docker/dockerfiles/fedora-i386-cross.docker | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tests/docker/dockerfiles/fedora-i386-cross.docker > b/tests/docker/dockerfiles/fedora-i386-cross.docker > index dbb8195eb1..820740d5be 100644 > --- a/tests/docker/dockerfiles/fedora-i386-cross.docker > +++ b/tests/docker/dockerfiles/fedora-i386-cross.docker > @@ -17,12 +17,13 @@ ENV PACKAGES \ > glibc-static.i686 \ > gnutls-devel.i686 \ > nettle-devel.i686 \ > +pcre-devel.i686 \ > perl-Test-Harness \ > pixman-devel.i686 \ > zlib-devel.i686 > > -ENV QEMU_CONFIGURE_OPTS --extra-cflags=-m32 --disable-vhost-user > -ENV PKG_CONFIG_PATH /usr/lib/pkgconfig > +ENV QEMU_CONFIGURE_OPTS --cpu=i386 --disable-vhost-user > +ENV PKG_CONFIG_LIBDIR /usr/lib/pkgconfig > > RUN dnf install -y $PACKAGES > RUN rpm -q $PACKAGES | sort > /packages.txt While I'm not able to directly test this docker file, I did run the equivalent commands (PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig ../configure --cpu=i386 [etc]) and successfully build a 32-bit qemu binary on Fedora 64-bit host with the multilib libraries installed. Therefore I'm pretty confident it should work: Reviewed-by: Richard W.M. Jones Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Re: [PULL 00/44] ppc-for-6.2 queue 20210930
On Thu, 30 Sept 2021 at 06:44, David Gibson wrote: > > The following changes since commit 6b54a31bf7b403672a798b6443b1930ae6c74dea: > > Merge remote-tracking branch > 'remotes/jsnow-gitlab/tags/python-pull-request' into staging (2021-09-28 > 13:07:32 +0100) > > are available in the Git repository at: > > https://gitlab.com/dgibson/qemu.git tags/ppc-for-6.2-20210930 > > for you to fetch changes up to 85d887be82905aa81b5d3d6c483ff0fa9958382b: > > MAINTAINERS: Demote sPAPR from "Supported" to "Maintained" (2021-09-30 > 12:26:06 +1000) > > > ppc patch queue for 2021-09-30 > > Here's the next batch of ppc related patches for qemu-6.2. Highlights > are: > * Fixes for several TCG math instructions from the El Dorado Institute > * A number of improvements to the powernv machine type > * Support for a new DEVICE_UNPLUG_GUEST_ERROR QAPI event from Daniel >Barboza > * Support for the new FORM2 PAPR NUMA representation. This allows >more specific NUMA distances, as well as asymmetric configurations > * Fix for 64-bit decrementer (used on MicroWatt CPUs) > * Assorted fixes and cleanups > * A number of updates to MAINTAINERS > > Note that the DEVICE_UNPLUG_GUEST_ERROR stuff includes changes to > files outside my normal area, but has suitable Acks. > > The MAINTAINERS updates are mostly about marking minor platforms > unmaintained / orphaned, and moving some pieces away from myself and > Greg. As we move onto other projects, we're going to need to drop > more of the ppc maintainership, though we're hoping we can avoid too > abrupt a change. Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.2 for any user-visible changes. -- PMM
[PATCH 1/2] tests/docker: Remove fedora-i386-cross from DOCKER_PARTIAL_IMAGES
The image was upgraded to a full image in ee381b7fe146. This makes it possible to use docker-test@image syntax with this container. Cc: Thomas Huth Cc: Alex Bennée Signed-off-by: Richard Henderson --- tests/docker/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index ff5d732889..0806c6f726 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -184,7 +184,7 @@ DOCKER_PARTIAL_IMAGES += debian-riscv64-cross DOCKER_PARTIAL_IMAGES += debian-sh4-cross debian-sparc64-cross DOCKER_PARTIAL_IMAGES += debian-tricore-cross DOCKER_PARTIAL_IMAGES += debian-xtensa-cross -DOCKER_PARTIAL_IMAGES += fedora-i386-cross fedora-cris-cross +DOCKER_PARTIAL_IMAGES += fedora-cris-cross # Rules for building linux-user powered images # -- 2.25.1
[PATCH 2/2] tests/docker: Fix fedora-i386-cross
By using PKG_CONFIG_PATH instead of PKG_CONFIG_LIBDIR, we were still including the 64-bit packages. Install pcre-devel.i686 to fill a missing glib2 dependency. By using --extra-cflags instead of --cpu, we incorrectly use the wrong probing during meson. Cc: Alex Bennée Cc: Paolo Bonzini Cc: Daniel P. Berrangé Cc: Richard W.M. Jones Signed-off-by: Richard Henderson --- tests/docker/dockerfiles/fedora-i386-cross.docker | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/docker/dockerfiles/fedora-i386-cross.docker b/tests/docker/dockerfiles/fedora-i386-cross.docker index dbb8195eb1..820740d5be 100644 --- a/tests/docker/dockerfiles/fedora-i386-cross.docker +++ b/tests/docker/dockerfiles/fedora-i386-cross.docker @@ -17,12 +17,13 @@ ENV PACKAGES \ glibc-static.i686 \ gnutls-devel.i686 \ nettle-devel.i686 \ +pcre-devel.i686 \ perl-Test-Harness \ pixman-devel.i686 \ zlib-devel.i686 -ENV QEMU_CONFIGURE_OPTS --extra-cflags=-m32 --disable-vhost-user -ENV PKG_CONFIG_PATH /usr/lib/pkgconfig +ENV QEMU_CONFIGURE_OPTS --cpu=i386 --disable-vhost-user +ENV PKG_CONFIG_LIBDIR /usr/lib/pkgconfig RUN dnf install -y $PACKAGES RUN rpm -q $PACKAGES | sort > /packages.txt -- 2.25.1
[PATCH 0/2] tests/docker: Fix fedora-i386-cross
The meson + pkg-config probing issues came up wrt a recent NBD pull request at the same time as I am trying to reproduce a gitlab-ci failure with fedora-i386-cross, and ran into something apparently related. r~ Richard Henderson (2): tests/docker: Remove fedora-i386-cross from DOCKER_PARTIAL_IMAGES tests/docker: Fix fedora-i386-cross tests/docker/Makefile.include | 2 +- tests/docker/dockerfiles/fedora-i386-cross.docker | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) -- 2.25.1
Re: [RFC PATCH 1/1] hw: aspeed_adc: Add initial Aspeed ADC support
FYI, these series was sent by Andrew in 2017 and I have been keeping it alive since in the aspeed-x.y branches : * memory: Support unaligned accesses on aligned-only models https://github.com/legoater/qemu/commit/1960ba6bde27b91edb5336985a9210260a4c8938 That was requested by Phil I think. * hw/adc: Add basic Aspeed ADC model https://github.com/legoater/qemu/commit/1eff7b1cf10d1777635f7d2cef8ecb441cc607c4 This is the initial patch. I added multi-engine support recently for the fuji. * hw/arm: Integrate ADC model into Aspeed SoC https://github.com/legoater/qemu/commit/3052f9d8ccdaf78b753e53574b7e8cc2ee01429f That one is trivial. Overall comments : I prefer the 'regs' array approach of your proposal. Ok (I was just following patterns from aspeed_scu.c), I’ll keep that aspect. I think the AspeedADCEngine should appear as a QOM object. Check the patches above. I see, I’ll make sure to test this. The engines are behind the same BAR and they share the IRQ. So it makes sense I think. And it shows up nicely under the monitor : ... 1e6e7000-1e6e7fff (prio 0, i/o): aspeed.xdma 1e6e9000-1e6e9fff (prio 0, i/o): aspeed.adc 1e6e9000-1e6e90ff (prio 0, i/o): aspeed.adc.engine.0 1e6e9100-1e6e91ff (prio 0, i/o): aspeed.adc.engine.1 1e70-1e700fff (prio -1000, i/o): aspeed.video ... /adc (aspeed.adc-ast2600) /aspeed.adc[0] (memory-region) /engine[0] (aspeed.adc.engine) /aspeed.adc.engine.0[0] (memory-region) /engine[1] (aspeed.adc.engine) /aspeed.adc.engine.1[0] (memory-region) /unnamed-gpio-in[0] (irq) /unnamed-gpio-in[1] (irq) To move on, maybe, you could rework the initial series and take ownership ? Yeah definitely! I’ll resubmit once I’ve reworked it. I don’t intend to include the unaligned-access support though, at least not w/ the ADC changes. Yeah. This can come later. Thanks, C.
Re: Moving QEMU downloads to GitLab Releases?
Hello! I'd be happy to help with this. I'm mostly a consumer of QEMU, but greatly appreciate all the work this community has done, and was able to contribute a little by helping with QEMU advent this past year. I would be happy to help streamline some of this activities if that would be welcome, and would gratefully contribute time and resources. Hosting and serving data like this has been core to my recent experience. I would be happy to suggest and build out a distribution strategy for these packages, and believe I could cut some costs, and even convince a small consultancy I am a part of here that uses QEMU to foot a reasonable bill. A brief introduction, since I haven't had the pleasure of attending FOSDEM or any other QEMU meetups: I am a startup-oriented Cloud Security Architect, based out of Atlanta, previously with companies like DataStax, but now working on AWS video pipelines for a startup here. Thanks, and hopefully I can be of service! Eldon On Thu, Sep 30, 2021 at 03:28:53PM +0100, Stefan Hajnoczi wrote: > On Thu, Sep 30, 2021 at 3:08 PM Stefan Hajnoczi wrote: > > > > Hi Mike, > > QEMU downloads are currently hosted on qemu.org's Apache web server. > > Paolo and I were discussing ways to reduce qemu.org network traffic to > > save money and eventually turn off the qemu.org server since there is no > > full-time sysadmin for it. I'd like to discuss moving QEMU downloads to > > GitLab Releases. > > Daniel Berrange posted this in another discussion: > > "gitlab releases have a per-file size limit that is somewhere on the > order of 10 MB IIRC. Our release tarballs are 100+ MB, so I don't > believe that's going to be viable. > > The gitlab package registry doesn't directly support plain file > downloads afaik, and is also size limited to 50 MB per package > > I'd love to find a good solution for large release artifact hosting, > since I need a better solution for virt-viewer windows MSI releases > wich are similarly large to QEMUs. For that I'm using pagure.io > provided by Fedora, but I don't have confidence in that service > surviving long term." > > So it looks like GitLab Releases won't work for QEMU :(. > > Stefan >
[PATCH 07/13] virtiofsd: Release file locks using F_UNLCK
We are emulating posix locks for guest using open file description locks in virtiofsd. When any of the fd is closed in guest, we find associated OFD lock fd (if there is one) and close it to release all the locks. Assumption here is that there is no other thread using lo_inode_plock structure or plock->fd, hence it is safe to do so. But now we are about to introduce blocking variant of locks (SETLKW), and that means we might be waiting to a lock to be available and using plock->fd. And that means there are still users of plock structure. So release locks using fcntl(SETLK, F_UNLCK) instead of closing fd and plock will be freed later when lo_inode is being freed. Signed-off-by: Vivek Goyal Signed-off-by: Ioannis Angelakopoulos --- tools/virtiofsd/passthrough_ll.c | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 38b2af8599..6928662e22 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -1557,9 +1557,6 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) lo_map_remove(&lo->ino_map, inode->fuse_ino); g_hash_table_remove(lo->inodes, &inode->key); if (lo->posix_lock) { -if (g_hash_table_size(inode->posix_locks)) { -fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n"); -} g_hash_table_destroy(inode->posix_locks); pthread_mutex_destroy(&inode->plock_mutex); } @@ -2266,6 +2263,8 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) (void)ino; struct lo_inode *inode; struct lo_data *lo = lo_data(req); +struct lo_inode_plock *plock; +struct flock flock; inode = lo_inode(req, ino); if (!inode) { @@ -2282,8 +2281,22 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) /* An fd is going away. Cleanup associated posix locks */ if (lo->posix_lock) { pthread_mutex_lock(&inode->plock_mutex); -g_hash_table_remove(inode->posix_locks, +plock = g_hash_table_lookup(inode->posix_locks, GUINT_TO_POINTER(fi->lock_owner)); + +if (plock) { +/* + * An fd is being closed. For posix locks, this means + * drop all the associated locks. + */ +memset(&flock, 0, sizeof(struct flock)); +flock.l_type = F_UNLCK; +flock.l_whence = SEEK_SET; +/* Unlock whole file */ +flock.l_start = flock.l_len = 0; +fcntl(plock->fd, F_OFD_SETLK, &flock); +} + pthread_mutex_unlock(&inode->plock_mutex); } res = close(dup(lo_fi_fd(req, fi))); -- 2.31.1
Re: [PATCH v1] Use CLOCK_MONOTONIC_RAW if available for get_clock().
On Thu, 30 Sept 2021 at 17:04, Joe Tanen wrote: > > CLOCK_MONOTONIC_RAW provides an unadjusted system clock on some platforms, > which is closer in spirit to providing a guest with a raw hardware clock than > CLOCK_MONOTONIC. > > Using CLOCK_MONOTONIC_RAW also works around a current issue in OSX where > CLOCK_MONOTONIC has been observed to go backwards. > > Since CLOCK_MONOTONIC_RAW might not be available on all platforms, revert to > using CLOCK_MONOTONIC if it is not present. > > Signed-off-by: Joe Tanen I'm not sure we want to change behaviour everywhere to work around an OSX bug, though... Has this bug been reported to Apple ? Is there some kind of bug report ID or URL we can quote in the commit message ? -- PMM
[PATCH 08/13] virtiofsd: Create a notification queue
Add a notification queue which will be used to send async notifications for file lock availability. Signed-off-by: Vivek Goyal Signed-off-by: Ioannis Angelakopoulos --- hw/virtio/vhost-user-fs-pci.c | 4 +- hw/virtio/vhost-user-fs.c | 62 +-- include/hw/virtio/vhost-user-fs.h | 2 + tools/virtiofsd/fuse_i.h | 1 + tools/virtiofsd/fuse_virtio.c | 70 +++ 5 files changed, 116 insertions(+), 23 deletions(-) diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c index 2ed8492b3f..cdb9471088 100644 --- a/hw/virtio/vhost-user-fs-pci.c +++ b/hw/virtio/vhost-user-fs-pci.c @@ -41,8 +41,8 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) DeviceState *vdev = DEVICE(&dev->vdev); if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { -/* Also reserve config change and hiprio queue vectors */ -vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2; +/* Also reserve config change, hiprio and notification queue vectors */ +vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 3; } qdev_realize(vdev, BUS(&vpci_dev->bus), errp); diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c index d1efbc5b18..6bafcf0243 100644 --- a/hw/virtio/vhost-user-fs.c +++ b/hw/virtio/vhost-user-fs.c @@ -31,6 +31,7 @@ static const int user_feature_bits[] = { VIRTIO_F_NOTIFY_ON_EMPTY, VIRTIO_F_RING_PACKED, VIRTIO_F_IOMMU_PLATFORM, +VIRTIO_FS_F_NOTIFICATION, VHOST_INVALID_FEATURE_BIT }; @@ -147,7 +148,7 @@ static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq) */ } -static void vuf_create_vqs(VirtIODevice *vdev) +static void vuf_create_vqs(VirtIODevice *vdev, bool notification_vq) { VHostUserFS *fs = VHOST_USER_FS(vdev); unsigned int i; @@ -155,6 +156,15 @@ static void vuf_create_vqs(VirtIODevice *vdev) /* Hiprio queue */ fs->hiprio_vq = virtio_add_queue(vdev, fs->conf.queue_size, vuf_handle_output); +/* + * Notification queue. Feature negotiation happens later. So at this + * point of time we don't know if driver will use notification queue + * or not. + */ +if (notification_vq) { +fs->notification_vq = virtio_add_queue(vdev, fs->conf.queue_size, + vuf_handle_output); +} /* Request queues */ fs->req_vqs = g_new(VirtQueue *, fs->conf.num_request_queues); @@ -163,8 +173,12 @@ static void vuf_create_vqs(VirtIODevice *vdev) vuf_handle_output); } -/* 1 high prio queue, plus the number configured */ -fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; +/* 1 high prio queue, 1 notification queue plus the number configured */ +if (notification_vq) { +fs->vhost_dev.nvqs = 2 + fs->conf.num_request_queues; +} else { +fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; +} fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); } @@ -176,6 +190,11 @@ static void vuf_cleanup_vqs(VirtIODevice *vdev) virtio_delete_queue(fs->hiprio_vq); fs->hiprio_vq = NULL; +if (fs->notification_vq) { +virtio_delete_queue(fs->notification_vq); +} +fs->notification_vq = NULL; + for (i = 0; i < fs->conf.num_request_queues; i++) { virtio_delete_queue(fs->req_vqs[i]); } @@ -194,9 +213,43 @@ static uint64_t vuf_get_features(VirtIODevice *vdev, { VHostUserFS *fs = VHOST_USER_FS(vdev); +virtio_add_feature(&features, VIRTIO_FS_F_NOTIFICATION); + return vhost_get_features(&fs->vhost_dev, user_feature_bits, features); } +static void vuf_set_features(VirtIODevice *vdev, uint64_t features) +{ +VHostUserFS *fs = VHOST_USER_FS(vdev); + +if (virtio_has_feature(features, VIRTIO_FS_F_NOTIFICATION)) { +fs->notify_enabled = true; +/* + * If guest first booted with no notification queue support and + * later rebooted with kernel which supports notification, we + * can end up here + */ +if (!fs->notification_vq) { +vuf_cleanup_vqs(vdev); +vuf_create_vqs(vdev, true); +} +return; +} + +fs->notify_enabled = false; +if (!fs->notification_vq) { +return; +} +/* + * Driver does not support notification queue. Reconfigure queues + * and do not create notification queue. + */ +vuf_cleanup_vqs(vdev); + +/* Create queues again */ +vuf_create_vqs(vdev, false); +} + static void vuf_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask) { @@ -262,7 +315,7 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) virtio_init(vdev, "vhost-user-fs", VIRTIO_ID_FS, sizeof(struct virtio_fs_conf
[PATCH 05/13] virtiofsd: Add a helper to stop all queues
Use a helper to stop all the queues. Later in the patch series I am planning to use this helper at one more place later in the patch series. Signed-off-by: Vivek Goyal --- tools/virtiofsd/fuse_virtio.c | 22 +- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index fcf12db9cd..baead08b28 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -740,6 +740,18 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx) vud->qi[qidx] = NULL; } +static void stop_all_queues(struct fv_VuDev *vud) +{ +for (int i = 0; i < vud->nqueues; i++) { +if (!vud->qi[i]) { +continue; +} + +fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i); +fv_queue_cleanup_thread(vud, i); +} +} + /* Callback from libvhost-user on start or stop of a queue */ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) { @@ -870,15 +882,7 @@ int virtio_loop(struct fuse_session *se) * Make sure all fv_queue_thread()s quit on exit, as we're about to * free virtio dev and fuse session, no one should access them anymore. */ -for (int i = 0; i < se->virtio_dev->nqueues; i++) { -if (!se->virtio_dev->qi[i]) { -continue; -} - -fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i); -fv_queue_cleanup_thread(se->virtio_dev, i); -} - +stop_all_queues(se->virtio_dev); fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__); return 0; -- 2.31.1
[PATCH 04/13] virtiofsd: Add a helper to send element on virtqueue
We have open coded logic to take locks and push element on virtqueue at three places. Add a helper and use it everywhere. Code is easier to read and less number of lines of code. Signed-off-by: Vivek Goyal --- tools/virtiofsd/fuse_virtio.c | 45 ++- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index da7b6a76bf..fcf12db9cd 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -243,6 +243,21 @@ static void vu_dispatch_unlock(struct fv_VuDev *vud) assert(ret == 0); } +static void vq_send_element(struct fv_QueueInfo *qi, VuVirtqElement *elem, +ssize_t len) +{ +struct fuse_session *se = qi->virtio_dev->se; +VuDev *dev = &se->virtio_dev->dev; +VuVirtq *q = vu_get_queue(dev, qi->qidx); + +vu_dispatch_rdlock(qi->virtio_dev); +pthread_mutex_lock(&qi->vq_lock); +vu_queue_push(dev, q, elem, len); +vu_queue_notify(dev, q); +pthread_mutex_unlock(&qi->vq_lock); +vu_dispatch_unlock(qi->virtio_dev); +} + /* * Called back by ll whenever it wants to send a reply/message back * The 1st element of the iov starts with the fuse_out_header @@ -253,8 +268,6 @@ int virtio_send_msg(struct fuse_session *se, struct fuse_chan *ch, { FVRequest *req = container_of(ch, FVRequest, ch); struct fv_QueueInfo *qi = ch->qi; -VuDev *dev = &se->virtio_dev->dev; -VuVirtq *q = vu_get_queue(dev, qi->qidx); VuVirtqElement *elem = &req->elem; int ret = 0; @@ -296,13 +309,7 @@ int virtio_send_msg(struct fuse_session *se, struct fuse_chan *ch, copy_iov(iov, count, in_sg, in_num, tosend_len); -vu_dispatch_rdlock(qi->virtio_dev); -pthread_mutex_lock(&qi->vq_lock); -vu_queue_push(dev, q, elem, tosend_len); -vu_queue_notify(dev, q); -pthread_mutex_unlock(&qi->vq_lock); -vu_dispatch_unlock(qi->virtio_dev); - +vq_send_element(qi, elem, tosend_len); req->reply_sent = true; err: @@ -321,8 +328,6 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, { FVRequest *req = container_of(ch, FVRequest, ch); struct fv_QueueInfo *qi = ch->qi; -VuDev *dev = &se->virtio_dev->dev; -VuVirtq *q = vu_get_queue(dev, qi->qidx); VuVirtqElement *elem = &req->elem; int ret = 0; g_autofree struct iovec *in_sg_cpy = NULL; @@ -430,12 +435,7 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, out_sg->len = tosend_len; } -vu_dispatch_rdlock(qi->virtio_dev); -pthread_mutex_lock(&qi->vq_lock); -vu_queue_push(dev, q, elem, tosend_len); -vu_queue_notify(dev, q); -pthread_mutex_unlock(&qi->vq_lock); -vu_dispatch_unlock(qi->virtio_dev); +vq_send_element(qi, elem, tosend_len); req->reply_sent = true; return 0; } @@ -447,7 +447,6 @@ static void fv_queue_worker(gpointer data, gpointer user_data) { struct fv_QueueInfo *qi = user_data; struct fuse_session *se = qi->virtio_dev->se; -struct VuDev *dev = &qi->virtio_dev->dev; FVRequest *req = data; VuVirtqElement *elem = &req->elem; struct fuse_buf fbuf = {}; @@ -589,17 +588,9 @@ out: /* If the request has no reply, still recycle the virtqueue element */ if (!req->reply_sent) { -struct VuVirtq *q = vu_get_queue(dev, qi->qidx); - fuse_log(FUSE_LOG_DEBUG, "%s: elem %d no reply sent\n", __func__, elem->index); - -vu_dispatch_rdlock(qi->virtio_dev); -pthread_mutex_lock(&qi->vq_lock); -vu_queue_push(dev, q, elem, 0); -vu_queue_notify(dev, q); -pthread_mutex_unlock(&qi->vq_lock); -vu_dispatch_unlock(qi->virtio_dev); +vq_send_element(qi, elem, 0); } pthread_mutex_destroy(&req->ch.lock); -- 2.31.1
Re: [RFC PATCH 1/1] hw: aspeed_adc: Add initial Aspeed ADC support
> On Sep 29, 2021, at 11:22 PM, Cédric Le Goater wrote: > > Hello Peter, > > If you run ./scripts/get_maintainer.pl on the patch, it will build > the list of persons and mailing list to send to. Oh, sorry about that, I’ll cc everyone properly when I resubmit this. > > On 9/30/21 02:42, p...@fb.com wrote: >> From: Peter Delevoryas >> This change sets up Aspeed SoC ADC emulation, so that most ADC drivers >> will pass the initialization sequence and load successfully. In the >> future, we can extend this to emulate more features. >> The initialization sequence is: >> 1. Set `ADC00` to `0xF`. >> 2. Wait for bit 8 of `ADC00` to be set. >> I also added the sequence for enabling "Auto compensating sensing mode": >> 1. Set `ADC00` to `0x2F` (set bit 5). >> 2. Wait for bit 5 of `ADC00` to be reset (to zero). >> 3. ... >> 4. ... >> Fuji (AST2600): >> Before: >> [ 56.185778] aspeed_adc: probe of 1e6e9000.adc failed with error -110 >> [ 56.687936] aspeed_adc: probe of 1e6e9100.adc failed with error -110 >> After: >> aspeed_adc_read 0x0c read 0x >> aspeed_adc_read 0x0c read 0x >> aspeed_adc_write 0x00 write 0x000f >> aspeed_adc_read 0x00 read 0x010f >> aspeed_adc_read 0x00 read 0x010f >> [ 55.885164] aspeed_adc 1e6e9000.adc: trim 8 >> aspeed_adc_read 0xc4 read 0x >> aspeed_adc_write 0xc4 write 0x0008 >> aspeed_adc_write 0x00 write 0x011f >> aspeed_adc_write 0x00 write 0x1011f >> aspeed_adc_read 0x10 read 0x >> aspeed_adc_write 0x00 write 0x010f >> [ 55.886509] aspeed_adc 1e6e9000.adc: cv 512 >> aspeed_adc_write 0x00 write 0x010f >> aspeed_adc_read 0x0c read 0x >> aspeed_adc_read 0x0c read 0x >> aspeed_adc_write 0x00 write 0x000f >> aspeed_adc_read 0x00 read 0x010f >> aspeed_adc_read 0x00 read 0x010f >> [ 55.890609] aspeed_adc 1e6e9100.adc: trim 8 >> aspeed_adc_read 0xc4 read 0x >> aspeed_adc_write 0xc4 write 0x0008 >> aspeed_adc_write 0x00 write 0x011f >> aspeed_adc_write 0x00 write 0x1011f >> aspeed_adc_read 0x10 read 0x >> aspeed_adc_write 0x00 write 0x010f >> [ 55.891863] aspeed_adc 1e6e9100.adc: cv 512 >> aspeed_adc_write 0x00 write 0x010f >> YosemiteV2 (AST2500): >> Before: >> [ 20.561588] ast_adc ast_adc.0: ast_adc_probe >> [ 20.563741] hwmon hwmon0: write offset: c4, val: 8 >> [ 20.563925] hwmon hwmon0: write offset: c, val: 40 >> [ 20.564099] hwmon hwmon0: write offset: 0, val: f >> [ 21.066110] ast_adc: driver init failed (ret=-110)! >> [ 21.066635] ast_adc: probe of ast_adc.0 failed with error -110 >> After: >> aspeed_adc_write 0xc4 write 0x0008 >> aspeed_adc_write 0x0c write 0x0040 >> aspeed_adc_write 0x00 write 0x000f >> aspeed_adc_read 0x00 read 0x010f >> aspeed_adc_write 0x00 write 0x002f >> aspeed_adc_read 0x00 read 0x000f >> aspeed_adc_read 0xc4 read 0x0008 >> [ 19.602033] ast_adc: driver successfully loaded. > > > FYI, these series was sent by Andrew in 2017 and I have been keeping > it alive since in the aspeed-x.y branches : > > * memory: Support unaligned accesses on aligned-only models > > https://github.com/legoater/qemu/commit/1960ba6bde27b91edb5336985a9210260a4c8938 > > That was requested by Phil I think. > > * hw/adc: Add basic Aspeed ADC model > > https://github.com/legoater/qemu/commit/1eff7b1cf10d1777635f7d2cef8ecb441cc607c4 > > This is the initial patch. I added multi-engine support recently > for the fuji. > > * hw/arm: Integrate ADC model into Aspeed SoC > > https://github.com/legoater/qemu/commit/3052f9d8ccdaf78b753e53574b7e8cc2ee01429f > > That one is trivial. > > > Overall comments : > > I prefer the 'regs' array approach of your proposal. Ok (I was just following patterns from aspeed_scu.c), I’ll keep that aspect. > > I think the AspeedADCEngine should appear as a QOM object. Check > the patches above. I see, I’ll make sure to test this. > > To move on, maybe, you could rework the initial series and take > ownership ? > Yeah definitely! I’ll resubmit once I’ve reworked it. I don’t intend to include the unaligned-access support though, at least not w/ the ADC changes. > > Some more below, > > >> Signed-off-by: Peter Delevoryas >> --- >> hw/adc/aspeed_adc.c | 205 >> hw/adc/meson.build | 1 + >> hw/adc/trace-events | 4 + >> hw/arm/aspeed_ast2600.c | 18 >> hw/arm/aspeed_soc.c | 17 +++ >> include/hw/adc/aspeed_adc.h | 48 + >> include/hw/arm/aspeed_soc.h | 5 + >> 7 files changed, 298 insertions(+) >> create mode 100644 hw/adc/aspeed_adc.c >> create mode 100644 include/hw/adc/aspeed_adc.h >> diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c >> new file mode 100644 >> index 00..590936148b >> --- /dev/null >> +++ b/hw/adc/aspeed_adc.c >> @@ -0,0 +1,205 @@ >> +/* >> +
[PATCH 02/13] virtiofsd: fuse.h header file changes for lock notification
This change comes from fuse.h kernel header file udpate. Hence keeping it in a separate patch. Signed-off-by: Vivek Goyal --- include/standard-headers/linux/fuse.h | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/include/standard-headers/linux/fuse.h b/include/standard-headers/linux/fuse.h index cce105bfba..0b6218d569 100644 --- a/include/standard-headers/linux/fuse.h +++ b/include/standard-headers/linux/fuse.h @@ -181,6 +181,8 @@ * - add FUSE_OPEN_KILL_SUIDGID * - extend fuse_setxattr_in, add FUSE_SETXATTR_EXT * - add FUSE_SETXATTR_ACL_KILL_SGID + * 7.35 + * - add FUSE_NOTIFY_LOCK */ #ifndef _LINUX_FUSE_H @@ -212,7 +214,7 @@ #define FUSE_KERNEL_VERSION 7 /** Minor version number of this interface */ -#define FUSE_KERNEL_MINOR_VERSION 33 +#define FUSE_KERNEL_MINOR_VERSION 35 /** The node ID of the root inode */ #define FUSE_ROOT_ID 1 @@ -521,6 +523,7 @@ enum fuse_notify_code { FUSE_NOTIFY_STORE = 4, FUSE_NOTIFY_RETRIEVE = 5, FUSE_NOTIFY_DELETE = 6, + FUSE_NOTIFY_LOCK = 7, FUSE_NOTIFY_CODE_MAX, }; @@ -912,6 +915,12 @@ struct fuse_notify_retrieve_in { uint64_tdummy4; }; +struct fuse_notify_lock_out { + uint64_tunique; + int32_t error; + int32_t padding; +}; + /* Device ioctls: */ #define FUSE_DEV_IOC_MAGIC 229 #define FUSE_DEV_IOC_CLONE _IOR(FUSE_DEV_IOC_MAGIC, 0, uint32_t) -- 2.31.1
[PATCH v1] Use CLOCK_MONOTONIC_RAW if available for get_clock().
CLOCK_MONOTONIC_RAW provides an unadjusted system clock on some platforms, which is closer in spirit to providing a guest with a raw hardware clock than CLOCK_MONOTONIC. Using CLOCK_MONOTONIC_RAW also works around a current issue in OSX where CLOCK_MONOTONIC has been observed to go backwards. Since CLOCK_MONOTONIC_RAW might not be available on all platforms, revert to using CLOCK_MONOTONIC if it is not present. Signed-off-by: Joe Tanen --- include/qemu/timer.h | 3 ++- util/qemu-timer-common.c | 11 +++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/qemu/timer.h b/include/qemu/timer.h index 88ef114689..fb8f5074df 100644 --- a/include/qemu/timer.h +++ b/include/qemu/timer.h @@ -828,12 +828,13 @@ static inline int64_t get_clock(void) #else extern int use_rt_clock; +extern clockid_t rt_clock; static inline int64_t get_clock(void) { if (use_rt_clock) { struct timespec ts; -clock_gettime(CLOCK_MONOTONIC, &ts); +clock_gettime(rt_clock, &ts); return ts.tv_sec * 10LL + ts.tv_nsec; } else { /* XXX: using gettimeofday leads to problems if the date diff --git a/util/qemu-timer-common.c b/util/qemu-timer-common.c index cc1326f726..5039c5406c 100644 --- a/util/qemu-timer-common.c +++ b/util/qemu-timer-common.c @@ -49,13 +49,24 @@ static void __attribute__((constructor)) init_get_clock(void) #else int use_rt_clock; +clockid_t rt_clock; static void __attribute__((constructor)) init_get_clock(void) { struct timespec ts; use_rt_clock = 0; +#if (defined(__APPLE__) || defined(__linux__)) && defined(CLOCK_MONOTONIC_RAW) +/* CLOCK_MONOTONIC_RAW is not available on all platforms or with all + * compiler flags. + */ +if (clock_gettime(CLOCK_MONOTONIC_RAW, &ts) == 0) { +rt_clock = CLOCK_MONOTONIC_RAW; +use_rt_clock = 1; +} else +#endif if (clock_gettime(CLOCK_MONOTONIC, &ts) == 0) { +rt_clock = CLOCK_MONOTONIC; use_rt_clock = 1; } clock_start = get_clock(); -- 2.31.1
[PATCH 10/13] virtiofsd: Custom threadpool for remote blocking posix locks requests
Add a new custom threadpool using posix threads that specifically service locking requests. In the case of a fcntl(SETLKW) request, if the guest is waiting for a lock or locks and issues a hard-reboot through SYSRQ then virtiofsd unblocks the blocked threads by sending a signal to them and waking them up. The current threadpool (GThreadPool) is not adequate to service the locking requests that result in a thread blocking. That is because GLib does not provide an API to cancel the request while it is serviced by a thread. In addition, a user might be running virtiofsd without a threadpool (--thread-pool-size=0), thus a locking request that blocks, will block the main virtqueue thread that services requests from servicing any other requests. The only exception occurs when the lock is of type F_UNLCK. In this case the request is serviced by the main virtqueue thread or a GThreadPool thread to avoid a deadlock, when all the threads in the custom threadpool are blocked. Then virtiofsd proceeds to cleanup the state of the threads, release them back to the system and re-initialize. Signed-off-by: Ioannis Angelakopoulos Signed-off-by: Vivek Goyal --- tools/virtiofsd/fuse_virtio.c | 90 ++- tools/virtiofsd/meson.build | 1 + tools/virtiofsd/passthrough_seccomp.c | 1 + tools/virtiofsd/tpool.c | 331 ++ tools/virtiofsd/tpool.h | 18 ++ 5 files changed, 440 insertions(+), 1 deletion(-) create mode 100644 tools/virtiofsd/tpool.c create mode 100644 tools/virtiofsd/tpool.h diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index 3b720c5d4a..c67c2e0e7a 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -20,6 +20,7 @@ #include "fuse_misc.h" #include "fuse_opt.h" #include "fuse_virtio.h" +#include "tpool.h" #include #include @@ -612,6 +613,60 @@ out: free(req); } +/* + * If the request is a locking request, use a custom locking thread pool. + */ +static bool use_lock_tpool(gpointer data, gpointer user_data) +{ +struct fv_QueueInfo *qi = user_data; +struct fuse_session *se = qi->virtio_dev->se; +FVRequest *req = data; +VuVirtqElement *elem = &req->elem; +struct fuse_buf fbuf = {}; +struct fuse_in_header *inhp; +struct fuse_lk_in *lkinp; +size_t lk_req_len; +/* The 'out' part of the elem is from qemu */ +unsigned int out_num = elem->out_num; +struct iovec *out_sg = elem->out_sg; +size_t out_len = iov_size(out_sg, out_num); +bool use_custom_tpool = false; + +/* + * If notifications are not enabled, no point in using cusotm lock + * thread pool. + */ +if (!se->notify_enabled) { +return false; +} + +assert(se->bufsize > sizeof(struct fuse_in_header)); +lk_req_len = sizeof(struct fuse_in_header) + sizeof(struct fuse_lk_in); + +if (out_len < lk_req_len) { +return false; +} + +fbuf.mem = g_malloc(se->bufsize); +copy_from_iov(&fbuf, out_num, out_sg, lk_req_len); + +inhp = fbuf.mem; +if (inhp->opcode != FUSE_SETLKW) { +goto out; +} + +lkinp = fbuf.mem + sizeof(struct fuse_in_header); +if (lkinp->lk.type == F_UNLCK) { +goto out; +} + +/* Its a blocking lock request. Use custom thread pool */ +use_custom_tpool = true; +out: +g_free(fbuf.mem); +return use_custom_tpool; +} + /* Thread function for individual queues, created when a queue is 'started' */ static void *fv_queue_thread(void *opaque) { @@ -619,6 +674,7 @@ static void *fv_queue_thread(void *opaque) struct VuDev *dev = &qi->virtio_dev->dev; struct VuVirtq *q = vu_get_queue(dev, qi->qidx); struct fuse_session *se = qi->virtio_dev->se; +struct fv_ThreadPool *lk_tpool = NULL; GThreadPool *pool = NULL; GList *req_list = NULL; @@ -631,6 +687,24 @@ static void *fv_queue_thread(void *opaque) fuse_log(FUSE_LOG_ERR, "%s: g_thread_pool_new failed\n", __func__); return NULL; } + +} + +/* + * Create the custom thread pool to handle blocking locking requests. + * Do not create for hiprio queue (qidx=0). + */ +if (qi->qidx) { +fuse_log(FUSE_LOG_DEBUG, "%s: Creating a locking thread pool for" + " Queue %d with size %d\n", __func__, qi->qidx, 4); +lk_tpool = fv_thread_pool_init(4); +if (!lk_tpool) { +fuse_log(FUSE_LOG_ERR, "%s: fv_thread_pool failed\n", __func__); +if (pool) { +g_thread_pool_free(pool, FALSE, TRUE); +} +return NULL; +} } fuse_log(FUSE_LOG_INFO, "%s: Start for queue %d kick_fd %d\n", __func__, @@ -703,7 +777,17 @@ static void *fv_queue_thread(void *opaque) req->reply_sent = false; -if (!se->thread_pool_size) { +/* + * In every case we get the opcode of the request and check if it +
Re: [PATCH 6/7] docs: move gcov section at the end of testing.rst
On Thu, 30 Sept 2021 at 14:33, Paolo Bonzini wrote: > > gcov testing applies to all tests, not just make check. Move it > out of the make check section. > > Signed-off-by: Paolo Bonzini > --- > docs/devel/testing.rst | 38 +++--- > 1 file changed, 19 insertions(+), 19 deletions(-) > Reviewed-by: Peter Maydell thanks -- PMM
[PATCH 11/13] virtiofsd: Shutdown notification queue in the end
So far we did not have the notion of cross queue traffic. That is, we get request on a queue and send back response on same queue. So if a request be being processed and at the same time a stop queue request comes in, we wait for all pending requests to finish and then queue is stopped and associated data structure cleaned. But with notification queue, now it is possible that we get a locking request on request queue and send the notification back on a different queue (notificaiton queue). This means, we need to make sure that notifiation queue has not already been shutdown or is not being shutdown in parallel while we are trying to send a notification back. Otherwise bad things are bound to happen. One way to solve this problem is that stop notification queue in the end. First stop hiprio and all request queues. That means by the time we are trying to stop notification queue, we know no other request can be in progress which can try to send something on notification queue. But problem is that currently we don't have any control on in what order queues should be stopped. If there was a notion of whole device being stopped, then we could decide in what order queues should be stopped. Stefan mentioned that there is a command to stop whole device VHOST_USER_SET_STATUS but it is not implemented in libvhost-user yet. Also we probably could not move away from per queue stop logic we have as of now. As an alternative, he said if we stop all queue when qidx 0 is being stopped, it should be fine and we can solve the issue of notification queue shutdown order. So in this patch I am shutting down all queues when queue 0 is being shutdown. And also changed shutdown order in such a way that notification queue is shutdown last. Suggested-by: Stefan Hajnoczi Signed-off-by: Vivek Goyal --- tools/virtiofsd/fuse_virtio.c | 27 ++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index c67c2e0e7a..a87e88e286 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -826,6 +826,11 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx) assert(qidx < vud->nqueues); ourqi = vud->qi[qidx]; +/* Queue is already stopped */ +if (!ourqi) { +return; +} + /* qidx == 1 is the notification queue if notifications are enabled */ if (!se->notify_enabled || qidx != 1) { /* Kill the thread */ @@ -847,14 +852,25 @@ static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx) static void stop_all_queues(struct fv_VuDev *vud) { +struct fuse_session *se = vud->se; + for (int i = 0; i < vud->nqueues; i++) { if (!vud->qi[i]) { continue; } +/* Shutdown notification queue in the end */ +if (se->notify_enabled && i == 1) { +continue; +} fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i); fv_queue_cleanup_thread(vud, i); } + +if (se->notify_enabled) { +fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, 1); +fv_queue_cleanup_thread(vud, 1); +} } /* Callback from libvhost-user on start or stop of a queue */ @@ -934,7 +950,16 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) * the queue thread doesn't block in virtio_send_msg(). */ vu_dispatch_unlock(vud); -fv_queue_cleanup_thread(vud, qidx); + +/* + * If queue 0 is being shutdown, treat it as if device is being + * shutdown and stop all queues. + */ +if (qidx == 0) { +stop_all_queues(vud); +} else { +fv_queue_cleanup_thread(vud, qidx); +} vu_dispatch_wrlock(vud); } } -- 2.31.1
[PATCH 13/13] virtiofsd, seccomp: Add clock_nanosleep() to allow list
g_usleep() calls nanosleep() and that now seems to call clock_nanosleep() syscall. Now these patches are making use of g_usleep(). So add clock_nanosleep() to list of allowed syscalls. Signed-off-by: Vivek Goyal --- tools/virtiofsd/passthrough_seccomp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c index cd24b40b78..03080806c0 100644 --- a/tools/virtiofsd/passthrough_seccomp.c +++ b/tools/virtiofsd/passthrough_seccomp.c @@ -117,6 +117,7 @@ static const int syscall_allowlist[] = { SCMP_SYS(writev), SCMP_SYS(umask), SCMP_SYS(nanosleep), +SCMP_SYS(clock_nanosleep), }; /* Syscalls used when --syslog is enabled */ -- 2.31.1
Re: [PATCH 3/7] docs: put "make" information together in build-system.rst
On Thu, 30 Sept 2021 at 14:33, Paolo Bonzini wrote: > > Signed-off-by: Paolo Bonzini > --- > docs/devel/build-system.rst | 21 ++--- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst > index 3baec158f2..0f636d620e 100644 > --- a/docs/devel/build-system.rst > +++ b/docs/devel/build-system.rst > @@ -380,6 +380,16 @@ phony target, while benchmarks are run with ``make > bench``. Meson test > suites such as ``unit`` can be ran with ``make check-unit`` too. It is also > possible to run tests defined in meson.build with ``meson test``. > > +Useful make targets > +--- > + > +``help`` > + Print a help message for the most common build targets. > + > +``print-VAR`` > + Print the value of the variable VAR. Useful for debugging the build > + system. > + Reviewed-by: Peter Maydell thanks -- PMM
Re: [PULL v2 00/19] NBD patches through 2021-09-27
On Wed, 29 Sept 2021 at 22:13, Eric Blake wrote: > > The following changes since commit 6b54a31bf7b403672a798b6443b1930ae6c74dea: > > Merge remote-tracking branch > 'remotes/jsnow-gitlab/tags/python-pull-request' into staging (2021-09-28 > 13:07:32 +0100) > > are available in the Git repository at: > > https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2021-09-27-v2 > > for you to fetch changes up to 1af7737871fb3b66036f5e520acb0a98fc2605f7: > > block/nbd: check that received handle is valid (2021-09-29 13:46:33 -0500) > > v2: defer problematic selinux patch; sending cover letter only since > remaining patches are unchanged > > > nbd patches for 2021-09-27 > > - Vladimir Sementsov-Ogievskiy: Rework coroutines of qemu NBD client > to improve reconnect support > - Eric Blake: Relax server in regards to NBD_OPT_LIST_META_CONTEXT > - Vladimir Sementsov-Ogievskiy: Plumb up 64-bit bulk-zeroing support > in block layer, in preparation for future NBD spec extensions > - Nir Soffer: Default to writeback cache in qemu-nbd Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.2 for any user-visible changes. -- PMM
[PATCH 12/13] virtiofsd: Implement blocking posix locks
As of now we don't support fcntl(F_SETLKW) and if we see one, we return -EOPNOTSUPP. Change that by accepting these requests and returning a reply immediately asking caller to wait. Once lock is available, send a notification to the waiter indicating lock is available. In response to lock request, we are returning error value as "1", which signals to client to queue the lock request internally and later client will get a notification which will signal lock is taken (or error). And then fuse client should wake up the guest process. Signed-off-by: Vivek Goyal Signed-off-by: Ioannis Angelakopoulos --- tools/virtiofsd/fuse_lowlevel.c | 37 - tools/virtiofsd/fuse_lowlevel.h | 26 tools/virtiofsd/fuse_virtio.c| 50 --- tools/virtiofsd/passthrough_ll.c | 70 4 files changed, 167 insertions(+), 16 deletions(-) diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c index e4679c73ab..2e7f4b786d 100644 --- a/tools/virtiofsd/fuse_lowlevel.c +++ b/tools/virtiofsd/fuse_lowlevel.c @@ -179,8 +179,8 @@ int fuse_send_reply_iov_nofree(fuse_req_t req, int error, struct iovec *iov, .unique = req->unique, .error = error, }; - -if (error <= -1000 || error > 0) { +/* error = 1 has been used to signal client to wait for notificaiton */ +if (error <= -1000 || error > 1) { fuse_log(FUSE_LOG_ERR, "fuse: bad error value: %i\n", error); out.error = -ERANGE; } @@ -290,6 +290,11 @@ int fuse_reply_err(fuse_req_t req, int err) return send_reply(req, -err, NULL, 0); } +int fuse_reply_wait(fuse_req_t req) +{ +return send_reply(req, 1, NULL, 0); +} + void fuse_reply_none(fuse_req_t req) { fuse_free_req(req); @@ -2165,6 +2170,34 @@ static void do_destroy(fuse_req_t req, fuse_ino_t nodeid, send_reply_ok(req, NULL, 0); } +static int send_notify_iov(struct fuse_session *se, int notify_code, + struct iovec *iov, int count) +{ +struct fuse_out_header out; +if (!se->got_init) { +return -ENOTCONN; +} +out.unique = 0; +out.error = notify_code; +iov[0].iov_base = &out; +iov[0].iov_len = sizeof(struct fuse_out_header); +return fuse_send_msg(se, NULL, iov, count); +} + +int fuse_lowlevel_notify_lock(struct fuse_session *se, uint64_t unique, + int32_t error) +{ +struct fuse_notify_lock_out outarg = {0}; +struct iovec iov[2]; + +outarg.unique = unique; +outarg.error = -error; + +iov[1].iov_base = &outarg; +iov[1].iov_len = sizeof(outarg); +return send_notify_iov(se, FUSE_NOTIFY_LOCK, iov, 2); +} + int fuse_lowlevel_notify_store(struct fuse_session *se, fuse_ino_t ino, off_t offset, struct fuse_bufvec *bufv) { diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h index c55c0ca2fc..64624b48dc 100644 --- a/tools/virtiofsd/fuse_lowlevel.h +++ b/tools/virtiofsd/fuse_lowlevel.h @@ -1251,6 +1251,22 @@ struct fuse_lowlevel_ops { */ int fuse_reply_err(fuse_req_t req, int err); +/** + * Ask caller to wait for lock. + * + * Possible requests: + * setlkw + * + * If caller sends a blocking lock request (setlkw), then reply to caller + * that wait for lock to be available. Once lock is available caller will + * receive a notification with request's unique id. Notification will + * carry info whether lock was successfully obtained or not. + * + * @param req request handle + * @return zero for success, -errno for failure to send reply + */ +int fuse_reply_wait(fuse_req_t req); + /** * Don't send reply * @@ -1685,6 +1701,16 @@ int fuse_lowlevel_notify_delete(struct fuse_session *se, fuse_ino_t parent, int fuse_lowlevel_notify_store(struct fuse_session *se, fuse_ino_t ino, off_t offset, struct fuse_bufvec *bufv); +/** + * Notify event related to previous lock request + * + * @param se the session object + * @param unique the unique id of the request which requested setlkw + * @param error zero for success, -errno for the failure + */ +int fuse_lowlevel_notify_lock(struct fuse_session *se, uint64_t unique, + int32_t error); + /* * Utility functions */ diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index a87e88e286..bb2d4456fc 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -273,6 +273,23 @@ static void vq_send_element(struct fv_QueueInfo *qi, VuVirtqElement *elem, vu_dispatch_unlock(qi->virtio_dev); } +/* Returns NULL if queue is empty */ +static FVRequest *vq_pop_notify_elem(struct fv_QueueInfo *qi) +{ +struct fuse_session *se = qi->virtio_dev->se; +VuDev *dev = &se->virtio_dev->dev; +VuVirtq *q = vu_get_queue(dev, qi->qidx); +FVRequest *req; + +vu_dispatch_rdlock(qi->virtio_dev); +pthread_mutex_lock(&qi->vq_lock); +/* Pop an el
Re: [PATCH 2/7] docs: move notes inside the body of the document
On Thu, 30 Sept 2021 at 14:33, Paolo Bonzini wrote: > > Make all documents start with a heading. > > Signed-off-by: Paolo Bonzini > --- > docs/devel/multi-process.rst | 20 +++- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/docs/devel/multi-process.rst b/docs/devel/multi-process.rst > index 69699329d6..e5758a79ab 100644 > --- a/docs/devel/multi-process.rst > +++ b/docs/devel/multi-process.rst > @@ -1,15 +1,17 @@ > -This is the design document for multi-process QEMU. It does not > -necessarily reflect the status of the current implementation, which > -may lack features or be considerably different from what is described > -in this document. This document is still useful as a description of > -the goals and general direction of this feature. > - > -Please refer to the following wiki for latest details: > -https://wiki.qemu.org/Features/MultiProcessQEMU > - > Multi-process QEMU > === > > +.. note:: > + > + This is the design document for multi-process QEMU. It does not > + necessarily reflect the status of the current implementation, which > + may lack features or be considerably different from what is described > + in this document. This document is still useful as a description of > + the goals and general direction of this feature. > + > + Please refer to the following wiki for latest details: > + https://wiki.qemu.org/Features/MultiProcessQEMU > + > QEMU is often used as the hypervisor for virtual machines running in the > Oracle cloud. Since one of the advantages of cloud computing is the > ability to run many VMs from different tenants in the same cloud Reviewed-by: Peter Maydell (side note, the wiki page was last updated in August 2020, which suggests maybe it's not that useful to refer people to it.) thanks -- PMM
[PULL 15/22] target/arm: Don't put FPEXC and FPSID in org.gnu.gdb.arm.vfp XML
Currently we send VFP XML which includes D0..D15 or D0..D31, plus FPSID, FPSCR and FPEXC. The upstream GDB tolerates this, but its definition of this XML feature does not include FPSID or FPEXC. In particular, for M-profile cores there are no FPSID or FPEXC registers, so advertising those is wrong. Move FPSID and FPEXC into their own bit of XML which we only send for A and R profile cores. This brings our definition of the XML org.gnu.gdb.arm.vfp feature into line with GDB's own (at least for non-Neon cores...) and means we don't claim to have FPSID and FPEXC on M-profile. (It seems unlikely to me that any gdbstub users really care about being able to look at FPEXC and FPSID; but we've supplied them to gdb for a decade and it's not hard to keep doing so.) Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20210921162901.17508-5-peter.mayd...@linaro.org --- configs/targets/aarch64-softmmu.mak | 2 +- configs/targets/arm-linux-user.mak | 2 +- configs/targets/arm-softmmu.mak | 2 +- configs/targets/armeb-linux-user.mak | 2 +- target/arm/gdbstub.c | 56 gdb-xml/arm-neon.xml | 2 - gdb-xml/arm-vfp-sysregs.xml | 17 + gdb-xml/arm-vfp.xml | 2 - gdb-xml/arm-vfp3.xml | 2 - 9 files changed, 61 insertions(+), 26 deletions(-) create mode 100644 gdb-xml/arm-vfp-sysregs.xml diff --git a/configs/targets/aarch64-softmmu.mak b/configs/targets/aarch64-softmmu.mak index 7703127674e..13d40b55e6d 100644 --- a/configs/targets/aarch64-softmmu.mak +++ b/configs/targets/aarch64-softmmu.mak @@ -1,5 +1,5 @@ TARGET_ARCH=aarch64 TARGET_BASE_ARCH=arm TARGET_SUPPORTS_MTTCG=y -TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml +TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-vfp-sysregs.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml TARGET_NEED_FDT=y diff --git a/configs/targets/arm-linux-user.mak b/configs/targets/arm-linux-user.mak index e741ffd4d30..acecc339e38 100644 --- a/configs/targets/arm-linux-user.mak +++ b/configs/targets/arm-linux-user.mak @@ -1,6 +1,6 @@ TARGET_ARCH=arm TARGET_SYSTBL_ABI=common,oabi TARGET_SYSTBL=syscall.tbl -TARGET_XML_FILES= gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml +TARGET_XML_FILES= gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-vfp-sysregs.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml TARGET_HAS_BFLT=y CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y diff --git a/configs/targets/arm-softmmu.mak b/configs/targets/arm-softmmu.mak index 84a98f48186..f6c95ba07a4 100644 --- a/configs/targets/arm-softmmu.mak +++ b/configs/targets/arm-softmmu.mak @@ -1,4 +1,4 @@ TARGET_ARCH=arm TARGET_SUPPORTS_MTTCG=y -TARGET_XML_FILES= gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml +TARGET_XML_FILES= gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-vfp-sysregs.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml TARGET_NEED_FDT=y diff --git a/configs/targets/armeb-linux-user.mak b/configs/targets/armeb-linux-user.mak index 255e44e8b0a..662c73d8fbd 100644 --- a/configs/targets/armeb-linux-user.mak +++ b/configs/targets/armeb-linux-user.mak @@ -2,6 +2,6 @@ TARGET_ARCH=arm TARGET_SYSTBL_ABI=common,oabi TARGET_SYSTBL=syscall.tbl TARGET_WORDS_BIGENDIAN=y -TARGET_XML_FILES= gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml +TARGET_XML_FILES= gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-vfp-sysregs.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml TARGET_HAS_BFLT=y CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index cbf156d192f..e0dcb33e325 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -144,11 +144,7 @@ static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg) } switch (reg - nregs) { case 0: -return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); -case 1: return gdb_get_reg32(buf, vfp_get_fpscr(env)); -case 2: -return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]); } return 0; } @@ -172,13 +168,31 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg) } } switch (reg - nregs) { +case 0: +vfp_set_fpscr(env, ldl_p(buf)); +return 4; +} +return 0; +} + +static int vfp_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg) +{ +switch (reg) { +case 0: +return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); +case 1: +return gdb_get_reg3
[PATCH 09/13] virtiofsd: Specify size of notification buffer using config space
Daemon specifies size of notification buffer needed and that should be done using config space. Only ->notify_buf_size value of config space comes from daemon. Rest of it is filled by qemu device emulation code. Signed-off-by: Vivek Goyal Signed-off-by: Ioannis Angelakopoulos --- hw/virtio/vhost-user-fs.c | 27 +++ include/hw/virtio/vhost-user-fs.h | 2 ++ include/standard-headers/linux/virtio_fs.h | 2 ++ tools/virtiofsd/fuse_virtio.c | 31 ++ 4 files changed, 62 insertions(+) diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c index 6bafcf0243..68a94708b4 100644 --- a/hw/virtio/vhost-user-fs.c +++ b/hw/virtio/vhost-user-fs.c @@ -36,15 +36,41 @@ static const int user_feature_bits[] = { VHOST_INVALID_FEATURE_BIT }; +static int vhost_user_fs_handle_config_change(struct vhost_dev *dev) +{ +return 0; +} + +const VhostDevConfigOps fs_ops = { +.vhost_dev_config_notifier = vhost_user_fs_handle_config_change, +}; + static void vuf_get_config(VirtIODevice *vdev, uint8_t *config) { VHostUserFS *fs = VHOST_USER_FS(vdev); struct virtio_fs_config fscfg = {}; +Error *local_err = NULL; +int ret; + +/* + * As of now we only get notification buffer size from device. And that's + * needed only if notification queue is enabled. + */ +if (fs->notify_enabled) { +ret = vhost_dev_get_config(&fs->vhost_dev, (uint8_t *)&fs->fscfg, + sizeof(struct virtio_fs_config), + &local_err); +if (ret) { +error_report_err(local_err); +return; +} +} memcpy((char *)fscfg.tag, fs->conf.tag, MIN(strlen(fs->conf.tag) + 1, sizeof(fscfg.tag))); virtio_stl_p(vdev, &fscfg.num_request_queues, fs->conf.num_request_queues); +virtio_stl_p(vdev, &fscfg.notify_buf_size, fs->fscfg.notify_buf_size); memcpy(config, &fscfg, sizeof(fscfg)); } @@ -316,6 +342,7 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) sizeof(struct virtio_fs_config)); vuf_create_vqs(vdev, true); +vhost_dev_set_config_notifier(&fs->vhost_dev, &fs_ops); ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user, VHOST_BACKEND_TYPE_USER, 0, errp); if (ret < 0) { diff --git a/include/hw/virtio/vhost-user-fs.h b/include/hw/virtio/vhost-user-fs.h index 95dc0dd402..3b114ee260 100644 --- a/include/hw/virtio/vhost-user-fs.h +++ b/include/hw/virtio/vhost-user-fs.h @@ -14,6 +14,7 @@ #ifndef _QEMU_VHOST_USER_FS_H #define _QEMU_VHOST_USER_FS_H +#include "standard-headers/linux/virtio_fs.h" #include "hw/virtio/virtio.h" #include "hw/virtio/vhost.h" #include "hw/virtio/vhost-user.h" @@ -37,6 +38,7 @@ struct VHostUserFS { struct vhost_virtqueue *vhost_vqs; struct vhost_dev vhost_dev; VhostUserState vhost_user; +struct virtio_fs_config fscfg; VirtQueue **req_vqs; VirtQueue *hiprio_vq; VirtQueue *notification_vq; diff --git a/include/standard-headers/linux/virtio_fs.h b/include/standard-headers/linux/virtio_fs.h index b7f015186e..867d18acf6 100644 --- a/include/standard-headers/linux/virtio_fs.h +++ b/include/standard-headers/linux/virtio_fs.h @@ -17,6 +17,8 @@ struct virtio_fs_config { /* Number of request queues */ uint32_t num_request_queues; + /* Size of notification buffer */ + uint32_t notify_buf_size; } QEMU_PACKED; /* For the id field in virtio_pci_shm_cap */ diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index f5b87a508a..3b720c5d4a 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -856,6 +856,35 @@ static bool fv_queue_order(VuDev *dev, int qidx) return false; } +static uint64_t fv_get_protocol_features(VuDev *dev) +{ +return 1ull << VHOST_USER_PROTOCOL_F_CONFIG; +} + +static int fv_get_config(VuDev *dev, uint8_t *config, uint32_t len) +{ +struct virtio_fs_config fscfg = {}; +unsigned notify_size, roundto = 64; +union fuse_notify_union { +struct fuse_notify_poll_wakeup_out wakeup_out; +struct fuse_notify_inval_inode_out inode_out; +struct fuse_notify_inval_entry_out entry_out; +struct fuse_notify_delete_out delete_out; +struct fuse_notify_store_outstore_out; +struct fuse_notify_retrieve_out retrieve_out; +}; + +notify_size = sizeof(struct fuse_out_header) + + sizeof(union fuse_notify_union); +notify_size = ((notify_size + roundto) / roundto) * roundto; + +fscfg.notify_buf_size = notify_size; +memcpy(config, &fscfg, len); +fuse_log(FUSE_LOG_DEBUG, "%s:Setting notify_buf_size=%d\n", __func__, + fscfg.notify_buf_size); +return 0; +} + static const VuDevIface fv_iface = { .get_features = fv_get_features, .set_features = fv_set_featu
[PATCH 00/13] virtiofsd: Support notification queue and
Hi, Here are the patches to support notification queue and blocking posix locks. One of the biggest change since las time has been creation of custom thread pool for handling locking requests. Thanks to Ioannis for doing most of the work on custom thread pool. I have posted corresponding kernel changes here. https://lore.kernel.org/linux-fsdevel/20210930143850.1188628-1-vgo...@redhat.com/T/#mb2d0fbfdb580ef33b6e812d0acbd16333b11f2cf Any feedback is welcome. Thanks Vivek Vivek Goyal (13): virtio_fs.h: Add notification queue feature bit virtiofsd: fuse.h header file changes for lock notification virtiofsd: Remove unused virtio_fs_config definition virtiofsd: Add a helper to send element on virtqueue virtiofsd: Add a helper to stop all queues vhost-user-fs: Use helpers to create/cleanup virtqueue virtiofsd: Release file locks using F_UNLCK virtiofsd: Create a notification queue virtiofsd: Specify size of notification buffer using config space virtiofsd: Custom threadpool for remote blocking posix locks requests virtiofsd: Shutdown notification queue in the end virtiofsd: Implement blocking posix locks virtiofsd, seccomp: Add clock_nanosleep() to allow list hw/virtio/vhost-user-fs-pci.c | 4 +- hw/virtio/vhost-user-fs.c | 158 -- include/hw/virtio/vhost-user-fs.h | 4 + include/standard-headers/linux/fuse.h | 11 +- include/standard-headers/linux/virtio_fs.h | 5 + tools/virtiofsd/fuse_i.h | 1 + tools/virtiofsd/fuse_lowlevel.c| 37 ++- tools/virtiofsd/fuse_lowlevel.h| 26 ++ tools/virtiofsd/fuse_virtio.c | 339 + tools/virtiofsd/meson.build| 1 + tools/virtiofsd/passthrough_ll.c | 91 +- tools/virtiofsd/passthrough_seccomp.c | 2 + tools/virtiofsd/tpool.c| 331 tools/virtiofsd/tpool.h| 18 ++ 14 files changed, 915 insertions(+), 113 deletions(-) create mode 100644 tools/virtiofsd/tpool.c create mode 100644 tools/virtiofsd/tpool.h -- 2.31.1
[PULL 20/22] qbus: Rename qbus_create() to qbus_new()
Rename the "allocate and return" qbus creation function to qbus_new(), to bring it into line with our _init vs _new convention. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Michael S. Tsirkin Reviewed-by: Corey Minyard Message-id: 20210923121153.23754-6-peter.mayd...@linaro.org --- include/hw/qdev-core.h | 2 +- hw/core/bus.c | 2 +- hw/hyperv/vmbus.c | 2 +- hw/i2c/core.c | 2 +- hw/isa/isa-bus.c| 2 +- hw/misc/auxbus.c| 2 +- hw/pci/pci.c| 2 +- hw/ppc/spapr_vio.c | 2 +- hw/s390x/ap-bridge.c| 2 +- hw/s390x/css-bridge.c | 2 +- hw/s390x/s390-pci-bus.c | 2 +- hw/ssi/ssi.c| 2 +- hw/xen/xen-bus.c| 2 +- hw/xen/xen-legacy-backend.c | 2 +- 14 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index ebca8cf9fca..4ff19c714bd 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -680,7 +680,7 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque); void qbus_init(void *bus, size_t size, const char *typename, DeviceState *parent, const char *name); -BusState *qbus_create(const char *typename, DeviceState *parent, const char *name); +BusState *qbus_new(const char *typename, DeviceState *parent, const char *name); bool qbus_realize(BusState *bus, Error **errp); void qbus_unrealize(BusState *bus); diff --git a/hw/core/bus.c b/hw/core/bus.c index cec49985024..c7831b5293b 100644 --- a/hw/core/bus.c +++ b/hw/core/bus.c @@ -159,7 +159,7 @@ void qbus_init(void *bus, size_t size, const char *typename, qbus_init_internal(bus, parent, name); } -BusState *qbus_create(const char *typename, DeviceState *parent, const char *name) +BusState *qbus_new(const char *typename, DeviceState *parent, const char *name) { BusState *bus; diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c index c9887d5a7bc..dbce3b35fba 100644 --- a/hw/hyperv/vmbus.c +++ b/hw/hyperv/vmbus.c @@ -2729,7 +2729,7 @@ static void vmbus_bridge_realize(DeviceState *dev, Error **errp) return; } -bridge->bus = VMBUS(qbus_create(TYPE_VMBUS, dev, "vmbus")); +bridge->bus = VMBUS(qbus_new(TYPE_VMBUS, dev, "vmbus")); } static char *vmbus_bridge_ofw_unit_address(const SysBusDevice *dev) diff --git a/hw/i2c/core.c b/hw/i2c/core.c index 416372ad00c..0e7d2763b9e 100644 --- a/hw/i2c/core.c +++ b/hw/i2c/core.c @@ -60,7 +60,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name) { I2CBus *bus; -bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name)); +bus = I2C_BUS(qbus_new(TYPE_I2C_BUS, parent, name)); QLIST_INIT(&bus->current_devs); vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_i2c_bus, bus); return bus; diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c index cffaa35e9cf..6c31398dda6 100644 --- a/hw/isa/isa-bus.c +++ b/hw/isa/isa-bus.c @@ -64,7 +64,7 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space, sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); } -isabus = ISA_BUS(qbus_create(TYPE_ISA_BUS, dev, NULL)); +isabus = ISA_BUS(qbus_new(TYPE_ISA_BUS, dev, NULL)); isabus->address_space = address_space; isabus->address_space_io = address_space_io; return isabus; diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c index 434ff8d910d..8a8012f5f08 100644 --- a/hw/misc/auxbus.c +++ b/hw/misc/auxbus.c @@ -65,7 +65,7 @@ AUXBus *aux_bus_init(DeviceState *parent, const char *name) AUXBus *bus; Object *auxtoi2c; -bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name)); +bus = AUX_BUS(qbus_new(TYPE_AUX_BUS, parent, name)); auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c", &error_abort, NULL); diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 14cb15a0aa1..186758ee11f 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -478,7 +478,7 @@ PCIBus *pci_root_bus_new(DeviceState *parent, const char *name, { PCIBus *bus; -bus = PCI_BUS(qbus_create(typename, parent, name)); +bus = PCI_BUS(qbus_new(typename, parent, name)); pci_root_bus_internal_init(bus, parent, address_space_mem, address_space_io, devfn_min); return bus; diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c index b59452bcd62..b975ed29cad 100644 --- a/hw/ppc/spapr_vio.c +++ b/hw/ppc/spapr_vio.c @@ -577,7 +577,7 @@ SpaprVioBus *spapr_vio_bus_init(void) sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); /* Create bus on bridge device */ -qbus = qbus_create(TYPE_SPAPR_VIO_BUS, dev, "spapr-vio"); +qbus = qbus_new(TYPE_SPAPR_VIO_BUS, dev, "spapr-vio"); bus = SPAPR_VIO_BUS(qbus); bus->next_reg = SPAPR_VIO_REG_BASE; diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c index 8bcf8ece9dd..ef8fa2b15be 100644 --- a/hw/s390x/ap-bridg
[PULL 22/22] hw/arm: sabrelite: Connect SPI flash CS line to GPIO3_19
From: Xuzhou Cheng The Linux spi-imx driver does not work on QEMU. The reason is that the state of m25p80 loops in STATE_READING_DATA state after receiving RDSR command, the new command is ignored. Before sending a new command, CS line should be pulled high to make the state of m25p80 back to IDLE. Currently the SPI flash CS line is connected to the SPI controller, but on the real board, it's connected to GPIO3_19. This matches the ecspi1 device node in the board dts. ecspi1 node in imx6qdl-sabrelite.dtsi: &ecspi1 { cs-gpios = <&gpio3 19 GPIO_ACTIVE_LOW>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_ecspi1>; status = "okay"; flash: m25p80@0 { compatible = "sst,sst25vf016b", "jedec,spi-nor"; spi-max-frequency = <2000>; reg = <0>; }; }; Should connect the SSI_GPIO_CS to GPIO3_19 when adding a spi-nor to spi1 on sabrelite machine. Verified this patch on Linux v5.14. Logs: # echo "01234567899876543210" > test # mtd_debug erase /dev/mtd0 0x0 0x1000 Erased 4096 bytes from address 0x in flash # mtd_debug write /dev/mtdblock0 0x0 20 test Copied 20 bytes from test to address 0x in flash # mtd_debug read /dev/mtdblock0 0x0 20 test_out Copied 20 bytes from address 0x in flash to test_out # cat test_out 01234567899876543210# Signed-off-by: Xuzhou Cheng Reported-by: Guenter Roeck Reviewed-by: Bin Meng Reviewed-by: Philippe Mathieu-Daudé Message-id: 20210927142825.491-1-xchengl...@gmail.com Signed-off-by: Peter Maydell --- hw/arm/sabrelite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c index 29fc777b613..553608e5835 100644 --- a/hw/arm/sabrelite.c +++ b/hw/arm/sabrelite.c @@ -87,7 +87,7 @@ static void sabrelite_init(MachineState *machine) qdev_realize_and_unref(flash_dev, BUS(spi_bus), &error_fatal); cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0); -sysbus_connect_irq(SYS_BUS_DEVICE(spi_dev), 1, cs_line); +qdev_connect_gpio_out(DEVICE(&s->gpio[2]), 19, cs_line); } } } -- 2.20.1
[PATCH 01/13] virtio_fs.h: Add notification queue feature bit
This change will ultimately come from kernel as kernel header file update when kernel patches get merged. Signed-off-by: Vivek Goyal --- include/standard-headers/linux/virtio_fs.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/standard-headers/linux/virtio_fs.h b/include/standard-headers/linux/virtio_fs.h index a32fe8a64c..b7f015186e 100644 --- a/include/standard-headers/linux/virtio_fs.h +++ b/include/standard-headers/linux/virtio_fs.h @@ -8,6 +8,9 @@ #include "standard-headers/linux/virtio_config.h" #include "standard-headers/linux/virtio_types.h" +/* Feature bits. Notification queue supported */ +#define VIRTIO_FS_F_NOTIFICATION 0 + struct virtio_fs_config { /* Filesystem name (UTF-8, not NUL-terminated, padded with NULs) */ uint8_t tag[36]; -- 2.31.1
[PATCH 06/13] vhost-user-fs: Use helpers to create/cleanup virtqueue
Add helpers to create/cleanup virtuqueues and use those helpers. I will need to reconfigure queues in later patches and using helpers will allow reusing the code. Signed-off-by: Vivek Goyal --- hw/virtio/vhost-user-fs.c | 87 +++ 1 file changed, 52 insertions(+), 35 deletions(-) diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c index c595957983..d1efbc5b18 100644 --- a/hw/virtio/vhost-user-fs.c +++ b/hw/virtio/vhost-user-fs.c @@ -139,6 +139,55 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status) } } +static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq) +{ +/* + * Not normally called; it's the daemon that handles the queue; + * however virtio's cleanup path can call this. + */ +} + +static void vuf_create_vqs(VirtIODevice *vdev) +{ +VHostUserFS *fs = VHOST_USER_FS(vdev); +unsigned int i; + +/* Hiprio queue */ +fs->hiprio_vq = virtio_add_queue(vdev, fs->conf.queue_size, + vuf_handle_output); + +/* Request queues */ +fs->req_vqs = g_new(VirtQueue *, fs->conf.num_request_queues); +for (i = 0; i < fs->conf.num_request_queues; i++) { +fs->req_vqs[i] = virtio_add_queue(vdev, fs->conf.queue_size, + vuf_handle_output); +} + +/* 1 high prio queue, plus the number configured */ +fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; +fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); +} + +static void vuf_cleanup_vqs(VirtIODevice *vdev) +{ +VHostUserFS *fs = VHOST_USER_FS(vdev); +unsigned int i; + +virtio_delete_queue(fs->hiprio_vq); +fs->hiprio_vq = NULL; + +for (i = 0; i < fs->conf.num_request_queues; i++) { +virtio_delete_queue(fs->req_vqs[i]); +} + +g_free(fs->req_vqs); +fs->req_vqs = NULL; + +fs->vhost_dev.nvqs = 0; +g_free(fs->vhost_dev.vqs); +fs->vhost_dev.vqs = NULL; +} + static uint64_t vuf_get_features(VirtIODevice *vdev, uint64_t features, Error **errp) @@ -148,14 +197,6 @@ static uint64_t vuf_get_features(VirtIODevice *vdev, return vhost_get_features(&fs->vhost_dev, user_feature_bits, features); } -static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq) -{ -/* - * Not normally called; it's the daemon that handles the queue; - * however virtio's cleanup path can call this. - */ -} - static void vuf_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask) { @@ -175,7 +216,6 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) { VirtIODevice *vdev = VIRTIO_DEVICE(dev); VHostUserFS *fs = VHOST_USER_FS(dev); -unsigned int i; size_t len; int ret; @@ -222,18 +262,7 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) virtio_init(vdev, "vhost-user-fs", VIRTIO_ID_FS, sizeof(struct virtio_fs_config)); -/* Hiprio queue */ -fs->hiprio_vq = virtio_add_queue(vdev, fs->conf.queue_size, vuf_handle_output); - -/* Request queues */ -fs->req_vqs = g_new(VirtQueue *, fs->conf.num_request_queues); -for (i = 0; i < fs->conf.num_request_queues; i++) { -fs->req_vqs[i] = virtio_add_queue(vdev, fs->conf.queue_size, vuf_handle_output); -} - -/* 1 high prio queue, plus the number configured */ -fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; -fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); +vuf_create_vqs(vdev); ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user, VHOST_BACKEND_TYPE_USER, 0, errp); if (ret < 0) { @@ -244,13 +273,8 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) err_virtio: vhost_user_cleanup(&fs->vhost_user); -virtio_delete_queue(fs->hiprio_vq); -for (i = 0; i < fs->conf.num_request_queues; i++) { -virtio_delete_queue(fs->req_vqs[i]); -} -g_free(fs->req_vqs); +vuf_cleanup_vqs(vdev); virtio_cleanup(vdev); -g_free(fs->vhost_dev.vqs); return; } @@ -258,7 +282,6 @@ static void vuf_device_unrealize(DeviceState *dev) { VirtIODevice *vdev = VIRTIO_DEVICE(dev); VHostUserFS *fs = VHOST_USER_FS(dev); -int i; /* This will stop vhost backend if appropriate. */ vuf_set_status(vdev, 0); @@ -267,14 +290,8 @@ static void vuf_device_unrealize(DeviceState *dev) vhost_user_cleanup(&fs->vhost_user); -virtio_delete_queue(fs->hiprio_vq); -for (i = 0; i < fs->conf.num_request_queues; i++) { -virtio_delete_queue(fs->req_vqs[i]); -} -g_free(fs->req_vqs); +vuf_cleanup_vqs(vdev); virtio_cleanup(vdev); -g_free(fs->vhost_dev.vqs); -fs->vhost_dev.vqs = NULL; } static const VMStateDescription vuf_vmstate = { -- 2.31.1
[PULL 19/22] qbus: Rename qbus_create_inplace() to qbus_init()
Rename qbus_create_inplace() to qbus_init(); this is more in line with our usual naming convention for functions that in-place initialize objects. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Michael S. Tsirkin Message-id: 20210923121153.23754-5-peter.mayd...@linaro.org --- include/hw/qdev-core.h| 4 ++-- hw/audio/intel-hda.c | 2 +- hw/block/fdc.c| 2 +- hw/block/swim.c | 3 +-- hw/char/virtio-serial-bus.c | 4 ++-- hw/core/bus.c | 11 ++- hw/core/sysbus.c | 10 ++ hw/gpio/bcm2835_gpio.c| 3 +-- hw/ide/qdev.c | 2 +- hw/ipack/ipack.c | 2 +- hw/misc/mac_via.c | 4 ++-- hw/misc/macio/cuda.c | 4 ++-- hw/misc/macio/macio.c | 4 ++-- hw/misc/macio/pmu.c | 4 ++-- hw/nubus/nubus-bridge.c | 2 +- hw/nvme/ctrl.c| 4 ++-- hw/nvme/subsys.c | 3 +-- hw/pci/pci.c | 2 +- hw/pci/pci_bridge.c | 4 ++-- hw/s390x/event-facility.c | 4 ++-- hw/s390x/virtio-ccw.c | 3 +-- hw/scsi/scsi-bus.c| 2 +- hw/sd/allwinner-sdhost.c | 4 ++-- hw/sd/bcm2835_sdhost.c| 4 ++-- hw/sd/pl181.c | 3 +-- hw/sd/pxa2xx_mmci.c | 4 ++-- hw/sd/sdhci.c | 3 +-- hw/sd/ssi-sd.c| 3 +-- hw/usb/bus.c | 2 +- hw/usb/dev-smartcard-reader.c | 3 +-- hw/virtio/virtio-mmio.c | 3 +-- hw/virtio/virtio-pci.c| 3 +-- 32 files changed, 54 insertions(+), 61 deletions(-) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 34c8a7506a1..ebca8cf9fca 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -678,8 +678,8 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id); typedef int (qbus_walkerfn)(BusState *bus, void *opaque); typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque); -void qbus_create_inplace(void *bus, size_t size, const char *typename, - DeviceState *parent, const char *name); +void qbus_init(void *bus, size_t size, const char *typename, + DeviceState *parent, const char *name); BusState *qbus_create(const char *typename, DeviceState *parent, const char *name); bool qbus_realize(BusState *bus, Error **errp); void qbus_unrealize(BusState *bus); diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 4330213fff1..8ce9df64e3e 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -52,7 +52,7 @@ void hda_codec_bus_init(DeviceState *dev, HDACodecBus *bus, size_t bus_size, hda_codec_response_func response, hda_codec_xfer_func xfer) { -qbus_create_inplace(bus, bus_size, TYPE_HDA_BUS, dev, NULL); +qbus_init(bus, bus_size, TYPE_HDA_BUS, dev, NULL); bus->response = response; bus->xfer = xfer; } diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 9014cd30b3a..fa933cd3263 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -77,7 +77,7 @@ static const TypeInfo floppy_bus_info = { static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev) { -qbus_create_inplace(bus, sizeof(FloppyBus), TYPE_FLOPPY_BUS, dev, NULL); +qbus_init(bus, sizeof(FloppyBus), TYPE_FLOPPY_BUS, dev, NULL); bus->fdc = fdc; } diff --git a/hw/block/swim.c b/hw/block/swim.c index 509c2f49003..333da08ce09 100644 --- a/hw/block/swim.c +++ b/hw/block/swim.c @@ -421,8 +421,7 @@ static void sysbus_swim_realize(DeviceState *dev, Error **errp) Swim *sys = SWIM(dev); SWIMCtrl *swimctrl = &sys->ctrl; -qbus_create_inplace(&swimctrl->bus, sizeof(SWIMBus), TYPE_SWIM_BUS, dev, -NULL); +qbus_init(&swimctrl->bus, sizeof(SWIMBus), TYPE_SWIM_BUS, dev, NULL); swimctrl->bus.ctrl = swimctrl; } diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c index dd6bc27b3b5..f01ec2137c9 100644 --- a/hw/char/virtio-serial-bus.c +++ b/hw/char/virtio-serial-bus.c @@ -1048,8 +1048,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp) config_size); /* Spawn a new virtio-serial bus on which the ports will ride as devices */ -qbus_create_inplace(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS, -dev, vdev->bus_name); +qbus_init(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS, + dev, vdev->bus_name); qbus_set_hotplug_handler(BUS(&vser->bus), OBJECT(vser)); vser->bus.vser = vser; QTAILQ_INIT(&vser->ports); diff --git a/hw/core/bus.c b/hw/core/bus.c index 9cfbc3a6877..cec49985024 100644 --- a/hw/core/bus.c +++ b/hw/core/bus.c @@ -99,7 +99,8 @@ static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb, } } -static void qbus_init(BusState *bus, DeviceState *parent, const char *name) +stati
[PULL 18/22] pci: Rename pci_root_bus_new_inplace() to pci_root_bus_init()
Rename the pci_root_bus_new_inplace() function to pci_root_bus_init(); this brings the bus type in to line with a "_init for in-place init, _new for allocate-and-return" convention. To do this we need to rename the implementation-internal function that was using the pci_root_bus_init() name to pci_root_bus_internal_init(). Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Michael S. Tsirkin Message-id: 20210923121153.23754-4-peter.mayd...@linaro.org --- include/hw/pci/pci.h| 10 +- hw/pci-host/raven.c | 4 ++-- hw/pci-host/versatile.c | 6 +++--- hw/pci/pci.c| 26 +- 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index d0f4266e372..7fc90132cf1 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -403,11 +403,11 @@ OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS) bool pci_bus_is_express(PCIBus *bus); -void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent, - const char *name, - MemoryRegion *address_space_mem, - MemoryRegion *address_space_io, - uint8_t devfn_min, const char *typename); +void pci_root_bus_init(PCIBus *bus, size_t bus_size, DeviceState *parent, + const char *name, + MemoryRegion *address_space_mem, + MemoryRegion *address_space_io, + uint8_t devfn_min, const char *typename); PCIBus *pci_root_bus_new(DeviceState *parent, const char *name, MemoryRegion *address_space_mem, MemoryRegion *address_space_io, diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c index 3be27f0a14d..6e514f75eb8 100644 --- a/hw/pci-host/raven.c +++ b/hw/pci-host/raven.c @@ -300,8 +300,8 @@ static void raven_pcihost_initfn(Object *obj) memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR, &s->pci_io_non_contiguous, 1); memory_region_add_subregion(address_space_mem, 0xc000, &s->pci_memory); -pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL, - &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS); +pci_root_bus_init(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL, + &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS); /* Bus master address space */ memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB); diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c index 3553277f941..f66384fa02d 100644 --- a/hw/pci-host/versatile.c +++ b/hw/pci-host/versatile.c @@ -405,9 +405,9 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp) memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 4 * GiB); memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 4 * GiB); -pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), dev, "pci", - &s->pci_mem_space, &s->pci_io_space, - PCI_DEVFN(11, 0), TYPE_PCI_BUS); +pci_root_bus_init(&s->pci_bus, sizeof(s->pci_bus), dev, "pci", + &s->pci_mem_space, &s->pci_io_space, + PCI_DEVFN(11, 0), TYPE_PCI_BUS); h->bus = &s->pci_bus; object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_VERSATILE_PCI_HOST); diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 23d2ae2ab23..19881c84f23 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -432,10 +432,10 @@ bool pci_bus_bypass_iommu(PCIBus *bus) return host_bridge->bypass_iommu; } -static void pci_root_bus_init(PCIBus *bus, DeviceState *parent, - MemoryRegion *address_space_mem, - MemoryRegion *address_space_io, - uint8_t devfn_min) +static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent, + MemoryRegion *address_space_mem, + MemoryRegion *address_space_io, + uint8_t devfn_min) { assert(PCI_FUNC(devfn_min) == 0); bus->devfn_min = devfn_min; @@ -460,15 +460,15 @@ bool pci_bus_is_express(PCIBus *bus) return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS); } -void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent, - const char *name, - MemoryRegion *address_space_mem, - MemoryRegion *address_space_io, - uint8_t devfn_min, const char *typename) +void pci_root_bus_init(PCIBus *bus, size_t bus_size, DeviceState *parent, + const char *name, + MemoryRegion *address_space_mem, + MemoryR
[PULL 16/22] scsi: Replace scsi_bus_new() with scsi_bus_init(), scsi_bus_init_named()
The function scsi_bus_new() creates a new SCSI bus; callers can either pass in a name argument to specify the name of the new bus, or they can pass in NULL to allow the bus to be given an automatically generated unique name. Almost all callers want to use the autogenerated name; the only exception is the virtio-scsi device. Taking a name argument that should almost always be NULL is an easy-to-misuse API design -- it encourages callers to think perhaps they should pass in some standard name like "scsi" or "scsi-bus". We don't do this anywhere for SCSI, but we do (incorrectly) do it for other bus types such as i2c. The function name also implies that it will return a newly allocated object, when it in fact does in-place allocation. We more commonly name such functions foo_init(), with foo_new() being the allocate-and-return variant. Replace all the scsi_bus_new() callsites with either: * scsi_bus_init() for the usual case where the caller wants an autogenerated bus name * scsi_bus_init_named() for the rare case where the caller needs to specify the bus name and document that for the _named() version it's then the caller's responsibility to think about uniqueness of bus names. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Michael S. Tsirkin Acked-by: Paolo Bonzini Message-id: 20210923121153.23754-2-peter.mayd...@linaro.org --- include/hw/scsi/scsi.h | 30 -- hw/scsi/esp-pci.c| 2 +- hw/scsi/esp.c| 2 +- hw/scsi/lsi53c895a.c | 2 +- hw/scsi/megasas.c| 3 +-- hw/scsi/mptsas.c | 2 +- hw/scsi/scsi-bus.c | 4 ++-- hw/scsi/spapr_vscsi.c| 3 +-- hw/scsi/virtio-scsi.c| 4 ++-- hw/scsi/vmw_pvscsi.c | 3 +-- hw/usb/dev-storage-bot.c | 3 +-- hw/usb/dev-storage-classic.c | 4 ++-- hw/usb/dev-uas.c | 3 +-- 13 files changed, 43 insertions(+), 22 deletions(-) diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index 0b726bc78c6..a567a5ed86b 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -146,8 +146,34 @@ struct SCSIBus { const SCSIBusInfo *info; }; -void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host, - const SCSIBusInfo *info, const char *bus_name); +/** + * scsi_bus_init_named: Initialize a SCSI bus with the specified name + * @bus: SCSIBus object to initialize + * @bus_size: size of @bus object + * @host: Device which owns the bus (generally the SCSI controller) + * @info: structure defining callbacks etc for the controller + * @bus_name: Name to use for this bus + * + * This in-place initializes @bus as a new SCSI bus with a name + * provided by the caller. It is the caller's responsibility to make + * sure that name does not clash with the name of any other bus in the + * system. Unless you need the new bus to have a specific name, you + * should use scsi_bus_new() instead. + */ +void scsi_bus_init_named(SCSIBus *bus, size_t bus_size, DeviceState *host, + const SCSIBusInfo *info, const char *bus_name); + +/** + * scsi_bus_init: Initialize a SCSI bus + * + * This in-place-initializes @bus as a new SCSI bus and gives it + * an automatically generated unique name. + */ +static inline void scsi_bus_init(SCSIBus *bus, size_t bus_size, + DeviceState *host, const SCSIBusInfo *info) +{ +scsi_bus_init_named(bus, bus_size, host, info, NULL); +} static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d) { diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c index 9db10b1a487..dac054aeed4 100644 --- a/hw/scsi/esp-pci.c +++ b/hw/scsi/esp-pci.c @@ -388,7 +388,7 @@ static void esp_pci_scsi_realize(PCIDevice *dev, Error **errp) pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->io); s->irq = pci_allocate_irq(dev); -scsi_bus_new(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info, NULL); +scsi_bus_init(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info); } static void esp_pci_scsi_exit(PCIDevice *d) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 4ac21147888..8454ed17735 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -1348,7 +1348,7 @@ static void sysbus_esp_realize(DeviceState *dev, Error **errp) qdev_init_gpio_in(dev, sysbus_esp_gpio_demux, 2); -scsi_bus_new(&s->bus, sizeof(s->bus), dev, &esp_scsi_info, NULL); +scsi_bus_init(&s->bus, sizeof(s->bus), dev, &esp_scsi_info); } static void sysbus_esp_hard_reset(DeviceState *dev) diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index e2c19180a0d..85e907a7854 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2309,7 +2309,7 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->ram_io); QTAILQ_INIT(&s->queue); -scsi_bus_new(&s->bus, sizeof(s->bus), d, &lsi_scsi_info, NULL); +scsi_bus_init(&s->bus
[PULL 17/22] ipack: Rename ipack_bus_new_inplace() to ipack_bus_init()
Rename ipack_bus_new_inplace() to ipack_bus_init(), to bring it in to line with a "_init for in-place init, _new for allocate-and-return" convention. Drop the 'name' argument, because the only caller does not pass in a name. If a future caller does need to specify the bus name, we should create an ipack_bus_init_named() function at that point. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Michael S. Tsirkin Message-id: 20210923121153.23754-3-peter.mayd...@linaro.org --- include/hw/ipack/ipack.h | 8 hw/ipack/ipack.c | 10 +- hw/ipack/tpci200.c | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/include/hw/ipack/ipack.h b/include/hw/ipack/ipack.h index 75014e74ae1..cbcdda509d3 100644 --- a/include/hw/ipack/ipack.h +++ b/include/hw/ipack/ipack.h @@ -73,9 +73,9 @@ extern const VMStateDescription vmstate_ipack_device; VMSTATE_STRUCT(_field, _state, 1, vmstate_ipack_device, IPackDevice) IPackDevice *ipack_device_find(IPackBus *bus, int32_t slot); -void ipack_bus_new_inplace(IPackBus *bus, size_t bus_size, - DeviceState *parent, - const char *name, uint8_t n_slots, - qemu_irq_handler handler); +void ipack_bus_init(IPackBus *bus, size_t bus_size, +DeviceState *parent, +uint8_t n_slots, +qemu_irq_handler handler); #endif diff --git a/hw/ipack/ipack.c b/hw/ipack/ipack.c index f19ecaeb1cf..d28e7f6449e 100644 --- a/hw/ipack/ipack.c +++ b/hw/ipack/ipack.c @@ -30,12 +30,12 @@ IPackDevice *ipack_device_find(IPackBus *bus, int32_t slot) return NULL; } -void ipack_bus_new_inplace(IPackBus *bus, size_t bus_size, - DeviceState *parent, - const char *name, uint8_t n_slots, - qemu_irq_handler handler) +void ipack_bus_init(IPackBus *bus, size_t bus_size, +DeviceState *parent, +uint8_t n_slots, +qemu_irq_handler handler) { -qbus_create_inplace(bus, bus_size, TYPE_IPACK_BUS, parent, name); +qbus_create_inplace(bus, bus_size, TYPE_IPACK_BUS, parent, NULL); bus->n_slots = n_slots; bus->set_irq = handler; } diff --git a/hw/ipack/tpci200.c b/hw/ipack/tpci200.c index d107e134c4e..1f764fc85ba 100644 --- a/hw/ipack/tpci200.c +++ b/hw/ipack/tpci200.c @@ -611,8 +611,8 @@ static void tpci200_realize(PCIDevice *pci_dev, Error **errp) pci_register_bar(&s->dev, 4, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->las2); pci_register_bar(&s->dev, 5, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->las3); -ipack_bus_new_inplace(&s->bus, sizeof(s->bus), DEVICE(pci_dev), NULL, - N_MODULES, tpci200_set_irq); +ipack_bus_init(&s->bus, sizeof(s->bus), DEVICE(pci_dev), + N_MODULES, tpci200_set_irq); } static const VMStateDescription vmstate_tpci200 = { -- 2.20.1
[PATCH 03/13] virtiofsd: Remove unused virtio_fs_config definition
"struct virtio_fs_config" definition seems to be unused in fuse_virtio.c. Remove it. Signed-off-by: Vivek Goyal --- tools/virtiofsd/fuse_virtio.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index 8f4fd165b9..da7b6a76bf 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -82,12 +82,6 @@ struct fv_VuDev { struct fv_QueueInfo **qi; }; -/* From spec */ -struct virtio_fs_config { -char tag[36]; -uint32_t num_queues; -}; - /* Callback from libvhost-user */ static uint64_t fv_get_features(VuDev *dev) { -- 2.31.1
[PULL 14/22] target/arm: Move gdbstub related code out of helper.c
Currently helper.c includes some code which is part of the arm target's gdbstub support. This code has a better home: in gdbstub.c and gdbstub64.c. Move it there. Because aarch64_fpu_gdb_get_reg() and aarch64_fpu_gdb_set_reg() move into gdbstub64.c, this means that they're now compiled only for TARGET_AARCH64 rather than always. That is the only case when they would ever be used, but it does mean that the ifdef in arm_cpu_register_gdb_regs_for_features() needs to be adjusted to match. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Message-id: 20210921162901.17508-4-peter.mayd...@linaro.org --- target/arm/internals.h | 7 ++ target/arm/gdbstub.c | 130 target/arm/gdbstub64.c | 140 + target/arm/helper.c| 271 - 4 files changed, 277 insertions(+), 271 deletions(-) diff --git a/target/arm/internals.h b/target/arm/internals.h index 9fbb3649682..3612107ab28 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -1270,4 +1270,11 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env) return (1 << 31) | ((1 << pmu_num_counters(env)) - 1); } +#ifdef TARGET_AARCH64 +int arm_gdb_get_svereg(CPUARMState *env, GByteArray *buf, int reg); +int arm_gdb_set_svereg(CPUARMState *env, uint8_t *buf, int reg); +int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg); +int aarch64_fpu_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg); +#endif + #endif diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index 826601b3415..cbf156d192f 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -19,6 +19,7 @@ */ #include "qemu/osdep.h" #include "cpu.h" +#include "internals.h" #include "exec/gdbstub.h" typedef struct RegisterSysregXmlParam { @@ -124,6 +125,98 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) return 0; } +static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg) +{ +ARMCPU *cpu = env_archcpu(env); +int nregs = cpu_isar_feature(aa32_simd_r32, cpu) ? 32 : 16; + +/* VFP data registers are always little-endian. */ +if (reg < nregs) { +return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg)); +} +if (arm_feature(env, ARM_FEATURE_NEON)) { +/* Aliases for Q regs. */ +nregs += 16; +if (reg < nregs) { +uint64_t *q = aa32_vfp_qreg(env, reg - 32); +return gdb_get_reg128(buf, q[0], q[1]); +} +} +switch (reg - nregs) { +case 0: +return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); +case 1: +return gdb_get_reg32(buf, vfp_get_fpscr(env)); +case 2: +return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]); +} +return 0; +} + +static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg) +{ +ARMCPU *cpu = env_archcpu(env); +int nregs = cpu_isar_feature(aa32_simd_r32, cpu) ? 32 : 16; + +if (reg < nregs) { +*aa32_vfp_dreg(env, reg) = ldq_le_p(buf); +return 8; +} +if (arm_feature(env, ARM_FEATURE_NEON)) { +nregs += 16; +if (reg < nregs) { +uint64_t *q = aa32_vfp_qreg(env, reg - 32); +q[0] = ldq_le_p(buf); +q[1] = ldq_le_p(buf + 8); +return 16; +} +} +switch (reg - nregs) { +case 0: +env->vfp.xregs[ARM_VFP_FPSID] = ldl_p(buf); +return 4; +case 1: +vfp_set_fpscr(env, ldl_p(buf)); +return 4; +case 2: +env->vfp.xregs[ARM_VFP_FPEXC] = ldl_p(buf) & (1 << 30); +return 4; +} +return 0; +} + +/** + * arm_get/set_gdb_*: get/set a gdb register + * @env: the CPU state + * @buf: a buffer to copy to/from + * @reg: register number (offset from start of group) + * + * We return the number of bytes copied + */ + +static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg) +{ +ARMCPU *cpu = env_archcpu(env); +const ARMCPRegInfo *ri; +uint32_t key; + +key = cpu->dyn_sysreg_xml.data.cpregs.keys[reg]; +ri = get_arm_cp_reginfo(cpu->cp_regs, key); +if (ri) { +if (cpreg_field_is_64bit(ri)) { +return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri)); +} else { +return gdb_get_reg32(buf, (uint32_t)read_raw_cp_reg(env, ri)); +} +} +return 0; +} + +static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg) +{ +return 0; +} + static void arm_gen_one_xml_sysreg_tag(GString *s, DynamicGDBXMLInfo *dyn_xml, ARMCPRegInfo *ri, uint32_t ri_key, int bitsize, int regnum) @@ -319,3 +412,40 @@ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) } return NULL; } + +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) +{ +CPUState *cs = CPU(cpu); +CPUARMState *env = &c
Re: Strange qemu6 regression cauing disabled usb controller.
On Thu, Sep 30, 2021 at 04:05:52PM +0100, Daniel P. Berrangé wrote: On Thu, Sep 30, 2021 at 03:48:44PM +0200, Remy Noel wrote: Co-incidentally we've just had another bug report filed today that suggests 7bed89958bfbf40df9ca681cefbdca63abdde39d as a buggy commit causing deadlock in QEMU https://gitlab.com/qemu-project/qemu/-/issues/650 Thanks, although, in this issue, the deadlock seems to be caused by the rcu while in the usb one, it is caused by the iothread unlock (or it may highlight an existing issue) even without rcu waiting.
[PULL 12/22] configs: Don't include 32-bit-only GDB XML in aarch64 linux configs
The aarch64-linux QEMU usermode binaries can never run 32-bit code, so they do not need to include the GDB XML for it. (arm_cpu_register_gdb_regs_for_features() will not use these XML files if the CPU has ARM_FEATURE_AARCH64, so we will not advertise to gdb that we have them.) Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20210921162901.17508-2-peter.mayd...@linaro.org --- configs/targets/aarch64-linux-user.mak| 2 +- configs/targets/aarch64_be-linux-user.mak | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/configs/targets/aarch64-linux-user.mak b/configs/targets/aarch64-linux-user.mak index 4713253709f..d0c603c54ec 100644 --- a/configs/targets/aarch64-linux-user.mak +++ b/configs/targets/aarch64-linux-user.mak @@ -1,5 +1,5 @@ TARGET_ARCH=aarch64 TARGET_BASE_ARCH=arm -TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml +TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml TARGET_HAS_BFLT=y CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y diff --git a/configs/targets/aarch64_be-linux-user.mak b/configs/targets/aarch64_be-linux-user.mak index fae831558da..d3ee10c00f3 100644 --- a/configs/targets/aarch64_be-linux-user.mak +++ b/configs/targets/aarch64_be-linux-user.mak @@ -1,6 +1,6 @@ TARGET_ARCH=aarch64 TARGET_BASE_ARCH=arm TARGET_WORDS_BIGENDIAN=y -TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml +TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml TARGET_HAS_BFLT=y CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y -- 2.20.1
[PULL 13/22] target/arm: Fix coding style issues in gdbstub code in helper.c
We're going to move this code to a different file; fix the coding style first so checkpatch doesn't complain. This includes deleting the spurious 'break' statements after returns in the vfp_gdb_get_reg() function. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Message-id: 20210921162901.17508-3-peter.mayd...@linaro.org --- target/arm/helper.c | 23 --- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 62742214473..84bba9a2244 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -72,9 +72,12 @@ static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg) } } switch (reg - nregs) { -case 0: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); break; -case 1: return gdb_get_reg32(buf, vfp_get_fpscr(env)); break; -case 2: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]); break; +case 0: +return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); +case 1: +return gdb_get_reg32(buf, vfp_get_fpscr(env)); +case 2: +return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]); } return 0; } @@ -98,9 +101,15 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg) } } switch (reg - nregs) { -case 0: env->vfp.xregs[ARM_VFP_FPSID] = ldl_p(buf); return 4; -case 1: vfp_set_fpscr(env, ldl_p(buf)); return 4; -case 2: env->vfp.xregs[ARM_VFP_FPEXC] = ldl_p(buf) & (1 << 30); return 4; +case 0: +env->vfp.xregs[ARM_VFP_FPSID] = ldl_p(buf); +return 4; +case 1: +vfp_set_fpscr(env, ldl_p(buf)); +return 4; +case 2: +env->vfp.xregs[ARM_VFP_FPEXC] = ldl_p(buf) & (1 << 30); +return 4; } return 0; } @@ -119,7 +128,7 @@ static int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg) return gdb_get_reg32(buf, vfp_get_fpsr(env)); case 33: /* FPCR */ -return gdb_get_reg32(buf,vfp_get_fpcr(env)); +return gdb_get_reg32(buf, vfp_get_fpcr(env)); default: return 0; } -- 2.20.1
[PULL 06/22] hw/nvram: Introduce Xilinx battery-backed ram
From: Tong Ho This device is present in Versal and ZynqMP product families to store a 256-bit encryption key. Co-authored-by: Edgar E. Iglesias Co-authored-by: Sai Pavan Boddu Signed-off-by: Edgar E. Iglesias Signed-off-by: Sai Pavan Boddu Signed-off-by: Tong Ho Message-id: 20210917052400.1249094-5-tong...@xilinx.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- include/hw/nvram/xlnx-bbram.h | 54 hw/nvram/xlnx-bbram.c | 545 ++ hw/nvram/Kconfig | 4 + hw/nvram/meson.build | 1 + 4 files changed, 604 insertions(+) create mode 100644 include/hw/nvram/xlnx-bbram.h create mode 100644 hw/nvram/xlnx-bbram.c diff --git a/include/hw/nvram/xlnx-bbram.h b/include/hw/nvram/xlnx-bbram.h new file mode 100644 index 000..87d59ef3c0c --- /dev/null +++ b/include/hw/nvram/xlnx-bbram.h @@ -0,0 +1,54 @@ +/* + * QEMU model of the Xilinx BBRAM Battery Backed RAM + * + * Copyright (c) 2015-2021 Xilinx Inc. + * + * Written by Edgar E. Iglesias + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +#ifndef XLNX_BBRAM_H +#define XLNX_BBRAM_H + +#include "sysemu/block-backend.h" +#include "hw/qdev-core.h" +#include "hw/irq.h" +#include "hw/sysbus.h" +#include "hw/register.h" + +#define RMAX_XLNX_BBRAM ((0x4c / 4) + 1) + +#define TYPE_XLNX_BBRAM "xlnx,bbram-ctrl" +OBJECT_DECLARE_SIMPLE_TYPE(XlnxBBRam, XLNX_BBRAM); + +struct XlnxBBRam { +SysBusDevice parent_obj; +qemu_irq irq_bbram; + +BlockBackend *blk; + +uint32_t crc_zpads; +bool bbram8_wo; +bool blk_ro; + +uint32_t regs[RMAX_XLNX_BBRAM]; +RegisterInfo regs_info[RMAX_XLNX_BBRAM]; +}; + +#endif diff --git a/hw/nvram/xlnx-bbram.c b/hw/nvram/xlnx-bbram.c new file mode 100644 index 000..b70828e5bf1 --- /dev/null +++ b/hw/nvram/xlnx-bbram.c @@ -0,0 +1,545 @@ +/* + * QEMU model of the Xilinx BBRAM Battery Backed RAM + * + * Copyright (c) 2014-2021 Xilinx Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "qemu/osdep.h" +#include "hw/nvram/xlnx-bbram.h" + +#include "qemu/error-report.h" +#include "qemu/log.h" +#include "qapi/error.h" +#include "sysemu/blockdev.h" +#include "migration/vmstate.h" +#include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" +#include "hw/nvram/xlnx-efuse.h" + +#ifndef XLNX_BBRAM_ERR_DEBUG +#define XLNX_BBRAM_ERR_DEBUG 0 +#endif + +REG32(BBRAM_STATUS, 0x0) +FIELD(BBRAM_STATUS, AES_CRC_PASS, 9, 1) +FIELD(BBRAM_STATUS, AES_CRC_DONE, 8, 1) +FIELD(BBRAM_STATUS, BBRAM_ZEROIZED, 4, 1) +FIELD(BBRAM_STATUS, PGM_MODE, 0, 1) +REG32(BBRAM_CTRL, 0x4) +FIELD(BBRAM_CTRL, ZEROIZE, 0, 1) +REG32(PGM_MODE, 0x8) +REG32(BBRAM_AES_CRC, 0xc) +REG32(BBRAM_0, 0x10) +REG32(BBRAM_1, 0x14) +REG32(BBRAM_2, 0x18) +REG32(BBRAM_3, 0x1c) +REG32(BBRAM_4, 0x20) +REG32(BBRAM_5, 0x24) +REG32(BBRAM_6, 0x28) +REG32(BBRAM_7, 0x2c) +REG32(BBRAM_8,
[PULL 08/22] hw/arm: xlnx-versal-virt: Add Xilinx eFUSE device
From: Tong Ho Connect the support for Versal eFUSE one-time field-programmable bit array. The command argument: -drive if=pflash,index=1,... Can be used to optionally connect the bit array to a backend storage, such that field-programmed values in one invocation can be made available to next invocation. The backend storage must be a seekable binary file, and its size must be 3072 bytes or larger. A file with all binary 0's is a 'blank'. Signed-off-by: Tong Ho Message-id: 20210917052400.1249094-7-tong...@xilinx.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- include/hw/arm/xlnx-versal.h | 10 +++ hw/arm/xlnx-versal-virt.c| 52 hw/arm/xlnx-versal.c | 39 +++ hw/arm/Kconfig | 1 + 4 files changed, 102 insertions(+) diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h index 1cac6133383..895ba12c61e 100644 --- a/include/hw/arm/xlnx-versal.h +++ b/include/hw/arm/xlnx-versal.h @@ -25,6 +25,7 @@ #include "hw/usb/xlnx-usb-subsystem.h" #include "hw/misc/xlnx-versal-xramc.h" #include "hw/nvram/xlnx-bbram.h" +#include "hw/nvram/xlnx-versal-efuse.h" #define TYPE_XLNX_VERSAL "xlnx-versal" OBJECT_DECLARE_SIMPLE_TYPE(Versal, XLNX_VERSAL) @@ -81,6 +82,9 @@ struct Versal { XlnxZynqMPRTC rtc; XlnxBBRam bbram; +XlnxEFuse efuse; +XlnxVersalEFuseCtrl efuse_ctrl; +XlnxVersalEFuseCache efuse_cache; } pmc; struct { @@ -110,6 +114,7 @@ struct Versal { #define VERSAL_BBRAM_APB_IRQ_0 121 #define VERSAL_RTC_APB_ERR_IRQ 121 #define VERSAL_SD0_IRQ_0 126 +#define VERSAL_EFUSE_IRQ 139 #define VERSAL_RTC_ALARM_IRQ 142 #define VERSAL_RTC_SECONDS_IRQ 143 @@ -177,6 +182,11 @@ struct Versal { #define MM_PMC_SD0_SIZE 0x1 #define MM_PMC_BBRAM_CTRL 0xf11f #define MM_PMC_BBRAM_CTRL_SIZE 0x00050 +#define MM_PMC_EFUSE_CTRL 0xf124 +#define MM_PMC_EFUSE_CTRL_SIZE 0x00104 +#define MM_PMC_EFUSE_CACHE 0xf125 +#define MM_PMC_EFUSE_CACHE_SIZE 0x00C00 + #define MM_PMC_CRP 0xf126U #define MM_PMC_CRP_SIZE 0x1 #define MM_PMC_RTC 0xf12a diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c index e1c5ead475a..d2f55e29b64 100644 --- a/hw/arm/xlnx-versal-virt.c +++ b/hw/arm/xlnx-versal-virt.c @@ -376,6 +376,41 @@ static void fdt_add_bbram_node(VersalVirt *s) g_free(name); } +static void fdt_add_efuse_ctrl_node(VersalVirt *s) +{ +const char compat[] = TYPE_XLNX_VERSAL_EFUSE_CTRL; +const char interrupt_names[] = "pmc_efuse"; +char *name = g_strdup_printf("/pmc_efuse@%x", MM_PMC_EFUSE_CTRL); + +qemu_fdt_add_subnode(s->fdt, name); + +qemu_fdt_setprop_cells(s->fdt, name, "interrupts", + GIC_FDT_IRQ_TYPE_SPI, VERSAL_EFUSE_IRQ, + GIC_FDT_IRQ_FLAGS_LEVEL_HI); +qemu_fdt_setprop(s->fdt, name, "interrupt-names", + interrupt_names, sizeof(interrupt_names)); +qemu_fdt_setprop_sized_cells(s->fdt, name, "reg", + 2, MM_PMC_EFUSE_CTRL, + 2, MM_PMC_EFUSE_CTRL_SIZE); +qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat)); +g_free(name); +} + +static void fdt_add_efuse_cache_node(VersalVirt *s) +{ +const char compat[] = TYPE_XLNX_VERSAL_EFUSE_CACHE; +char *name = g_strdup_printf("/xlnx_pmc_efuse_cache@%x", + MM_PMC_EFUSE_CACHE); + +qemu_fdt_add_subnode(s->fdt, name); + +qemu_fdt_setprop_sized_cells(s->fdt, name, "reg", + 2, MM_PMC_EFUSE_CACHE, + 2, MM_PMC_EFUSE_CACHE_SIZE); +qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat)); +g_free(name); +} + static void fdt_nop_memory_nodes(void *fdt, Error **errp) { Error *err = NULL; @@ -542,6 +577,18 @@ static void bbram_attach_drive(XlnxBBRam *dev) } } +static void efuse_attach_drive(XlnxEFuse *dev) +{ +DriveInfo *dinfo; +BlockBackend *blk; + +dinfo = drive_get_by_index(IF_PFLASH, 1); +blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; +if (blk) { +qdev_prop_set_drive(DEVICE(dev), "drive", blk); +} +} + static void sd_plugin_card(SDHCIState *sd, DriveInfo *di) { BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL; @@ -603,6 +650,8 @@ static void versal_virt_init(MachineState *machine) fdt_add_sd_nodes(s); fdt_add_rtc_node(s); fdt_add_bbram_node(s); +fdt_add_efuse_ctrl_node(s); +fdt_add_efuse_cache_node(s); fdt_add_cpu_nodes(s, psci_conduit); fdt_add_clk_node(s, "/clk125", 12500, s->phandle.clk_125Mhz); fdt_add_clk_node(s, "/clk25", 2500, s->phandle.clk_25Mhz); @@ -615,6 +664,9 @@ static void versal_virt_init(MachineState *machi
[PULL 09/22] hw/arm: xlnx-zcu102: Add Xilinx BBRAM device
From: Tong Ho Connect the support for Xilinx ZynqMP Battery-Backed RAM (BBRAM) The command argument: -drive if=pflash,index=2,... Can be used to optionally connect the bbram to a backend storage, such that field-programmed values in one invocation can be made available to next invocation. The backend storage must be a seekable binary file, and its size must be 36 bytes or larger. A file with all binary 0's is a 'blank'. Signed-off-by: Tong Ho Message-id: 20210917052400.1249094-8-tong...@xilinx.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- include/hw/arm/xlnx-zynqmp.h | 2 ++ hw/arm/xlnx-zcu102.c | 15 +++ hw/arm/xlnx-zynqmp.c | 20 hw/Kconfig | 1 + 4 files changed, 38 insertions(+) diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h index c84fe15996e..067e8a5238a 100644 --- a/include/hw/arm/xlnx-zynqmp.h +++ b/include/hw/arm/xlnx-zynqmp.h @@ -36,6 +36,7 @@ #include "qom/object.h" #include "net/can_emu.h" #include "hw/dma/xlnx_csu_dma.h" +#include "hw/nvram/xlnx-bbram.h" #define TYPE_XLNX_ZYNQMP "xlnx-zynqmp" OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP) @@ -100,6 +101,7 @@ struct XlnxZynqMPState { MemoryRegion *ddr_ram; MemoryRegion ddr_ram_low, ddr_ram_high; +XlnxBBRam bbram; MemoryRegion mr_unimp[XLNX_ZYNQMP_NUM_UNIMP_AREAS]; diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c index 6c6cb02e861..b247c5779bf 100644 --- a/hw/arm/xlnx-zcu102.c +++ b/hw/arm/xlnx-zcu102.c @@ -98,6 +98,18 @@ static void zcu102_modify_dtb(const struct arm_boot_info *binfo, void *fdt) } } +static void bbram_attach_drive(XlnxBBRam *dev) +{ +DriveInfo *dinfo; +BlockBackend *blk; + +dinfo = drive_get_by_index(IF_PFLASH, 2); +blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; +if (blk) { +qdev_prop_set_drive(DEVICE(dev), "drive", blk); +} +} + static void xlnx_zcu102_init(MachineState *machine) { XlnxZCU102 *s = ZCU102_MACHINE(machine); @@ -136,6 +148,9 @@ static void xlnx_zcu102_init(MachineState *machine) qdev_realize(DEVICE(&s->soc), NULL, &error_fatal); +/* Attach bbram backend, if given */ +bbram_attach_drive(&s->soc.bbram); + /* Create and plug in the SD cards */ for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) { BusState *bus; diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index 4e5a471e30b..1e8e2ddcc27 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -66,6 +66,9 @@ #define RTC_ADDR0xffa6 #define RTC_IRQ 26 +#define BBRAM_ADDR 0xffcd +#define BBRAM_IRQ 11 + #define SDHCI_CAPABILITIES 0x280737ec6481 /* Datasheet: UG1085 (v1.7) */ static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = { @@ -226,6 +229,22 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s, qdev_realize(DEVICE(&s->rpu_cluster), NULL, &error_fatal); } +static void xlnx_zynqmp_create_bbram(XlnxZynqMPState *s, qemu_irq *gic) +{ +SysBusDevice *sbd; + +object_initialize_child_with_props(OBJECT(s), "bbram", &s->bbram, + sizeof(s->bbram), TYPE_XLNX_BBRAM, + &error_fatal, + "crc-zpads", "1", + NULL); +sbd = SYS_BUS_DEVICE(&s->bbram); + +sysbus_realize(sbd, &error_fatal); +sysbus_mmio_map(sbd, 0, BBRAM_ADDR); +sysbus_connect_irq(sbd, 0, gic[BBRAM_IRQ]); +} + static void xlnx_zynqmp_create_unimp_mmio(XlnxZynqMPState *s) { static const struct UnimpInfo { @@ -626,6 +645,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) sysbus_mmio_map(SYS_BUS_DEVICE(&s->rtc), 0, RTC_ADDR); sysbus_connect_irq(SYS_BUS_DEVICE(&s->rtc), 0, gic_spi[RTC_IRQ]); +xlnx_zynqmp_create_bbram(s, gic_spi); xlnx_zynqmp_create_unimp_mmio(s); for (i = 0; i < XLNX_ZYNQMP_NUM_GDMA_CH; i++) { diff --git a/hw/Kconfig b/hw/Kconfig index 8cb7664d705..b6fb6a45074 100644 --- a/hw/Kconfig +++ b/hw/Kconfig @@ -81,3 +81,4 @@ config XLNX_ZYNQMP select REGISTER select CAN_BUS select PTIMER +select XLNX_BBRAM -- 2.20.1
[PULL 07/22] hw/arm: xlnx-versal-virt: Add Xilinx BBRAM device
From: Tong Ho Connect the support for Versal Battery-Backed RAM (BBRAM) The command argument: -drive if=pflash,index=0,... Can be used to optionally connect the bbram to a backend storage, such that field-programmed values in one invocation can be made available to next invocation. The backend storage must be a seekable binary file, and its size must be 36 bytes or larger. A file with all binary 0's is a 'blank'. Signed-off-by: Tong Ho Message-id: 20210917052400.1249094-6-tong...@xilinx.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- include/hw/arm/xlnx-versal.h | 5 + hw/arm/xlnx-versal-virt.c| 36 hw/arm/xlnx-versal.c | 18 ++ hw/arm/Kconfig | 1 + 4 files changed, 60 insertions(+) diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h index 9b790517478..1cac6133383 100644 --- a/include/hw/arm/xlnx-versal.h +++ b/include/hw/arm/xlnx-versal.h @@ -24,6 +24,7 @@ #include "qom/object.h" #include "hw/usb/xlnx-usb-subsystem.h" #include "hw/misc/xlnx-versal-xramc.h" +#include "hw/nvram/xlnx-bbram.h" #define TYPE_XLNX_VERSAL "xlnx-versal" OBJECT_DECLARE_SIMPLE_TYPE(Versal, XLNX_VERSAL) @@ -79,6 +80,7 @@ struct Versal { } iou; XlnxZynqMPRTC rtc; +XlnxBBRam bbram; } pmc; struct { @@ -105,6 +107,7 @@ struct Versal { #define VERSAL_GEM1_WAKE_IRQ_0 59 #define VERSAL_ADMA_IRQ_0 60 #define VERSAL_XRAM_IRQ_0 79 +#define VERSAL_BBRAM_APB_IRQ_0 121 #define VERSAL_RTC_APB_ERR_IRQ 121 #define VERSAL_SD0_IRQ_0 126 #define VERSAL_RTC_ALARM_IRQ 142 @@ -172,6 +175,8 @@ struct Versal { #define MM_PMC_SD0 0xf104U #define MM_PMC_SD0_SIZE 0x1 +#define MM_PMC_BBRAM_CTRL 0xf11f +#define MM_PMC_BBRAM_CTRL_SIZE 0x00050 #define MM_PMC_CRP 0xf126U #define MM_PMC_CRP_SIZE 0x1 #define MM_PMC_RTC 0xf12a diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c index 5bca360dcec..e1c5ead475a 100644 --- a/hw/arm/xlnx-versal-virt.c +++ b/hw/arm/xlnx-versal-virt.c @@ -356,6 +356,26 @@ static void fdt_add_rtc_node(VersalVirt *s) g_free(name); } +static void fdt_add_bbram_node(VersalVirt *s) +{ +const char compat[] = TYPE_XLNX_BBRAM; +const char interrupt_names[] = "bbram-error"; +char *name = g_strdup_printf("/bbram@%x", MM_PMC_BBRAM_CTRL); + +qemu_fdt_add_subnode(s->fdt, name); + +qemu_fdt_setprop_cells(s->fdt, name, "interrupts", + GIC_FDT_IRQ_TYPE_SPI, VERSAL_BBRAM_APB_IRQ_0, + GIC_FDT_IRQ_FLAGS_LEVEL_HI); +qemu_fdt_setprop(s->fdt, name, "interrupt-names", + interrupt_names, sizeof(interrupt_names)); +qemu_fdt_setprop_sized_cells(s->fdt, name, "reg", + 2, MM_PMC_BBRAM_CTRL, + 2, MM_PMC_BBRAM_CTRL_SIZE); +qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat)); +g_free(name); +} + static void fdt_nop_memory_nodes(void *fdt, Error **errp) { Error *err = NULL; @@ -510,6 +530,18 @@ static void create_virtio_regions(VersalVirt *s) } } +static void bbram_attach_drive(XlnxBBRam *dev) +{ +DriveInfo *dinfo; +BlockBackend *blk; + +dinfo = drive_get_by_index(IF_PFLASH, 0); +blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; +if (blk) { +qdev_prop_set_drive(DEVICE(dev), "drive", blk); +} +} + static void sd_plugin_card(SDHCIState *sd, DriveInfo *di) { BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL; @@ -570,6 +602,7 @@ static void versal_virt_init(MachineState *machine) fdt_add_usb_xhci_nodes(s); fdt_add_sd_nodes(s); fdt_add_rtc_node(s); +fdt_add_bbram_node(s); fdt_add_cpu_nodes(s, psci_conduit); fdt_add_clk_node(s, "/clk125", 12500, s->phandle.clk_125Mhz); fdt_add_clk_node(s, "/clk25", 2500, s->phandle.clk_25Mhz); @@ -579,6 +612,9 @@ static void versal_virt_init(MachineState *machine) memory_region_add_subregion_overlap(get_system_memory(), 0, &s->soc.fpd.apu.mr, 0); +/* Attach bbram backend, if given */ +bbram_attach_drive(&s->soc.pmc.bbram); + /* Plugin SD cards. */ for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) { sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD)); diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c index 547a26603a3..23451ae0126 100644 --- a/hw/arm/xlnx-versal.c +++ b/hw/arm/xlnx-versal.c @@ -314,6 +314,23 @@ static void versal_create_xrams(Versal *s, qemu_irq *pic) } } +static void versal_create_bbram(Versal *s, qemu_irq *pic) +{ +SysBusDevice *sbd; + +object_initialize_child_with_props(OBJECT(s), "bbram", &s->pmc.bbram, + sizeof(s->pmc.bbram)
[PULL 03/22] hw/nvram: Introduce Xilinx eFuse QOM
From: Tong Ho This introduces the QOM for Xilinx eFuse, an one-time field-programmable storage bit array. The actual mmio interface to the array varies by device families and will be provided in different change-sets. Co-authored-by: Edgar E. Iglesias Co-authored-by: Sai Pavan Boddu Signed-off-by: Edgar E. Iglesias Signed-off-by: Sai Pavan Boddu Signed-off-by: Tong Ho Message-id: 20210917052400.1249094-2-tong...@xilinx.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- include/hw/nvram/xlnx-efuse.h | 132 hw/nvram/xlnx-efuse-crc.c | 119 +++ hw/nvram/xlnx-efuse.c | 280 ++ hw/nvram/Kconfig | 7 + hw/nvram/meson.build | 2 + 5 files changed, 540 insertions(+) create mode 100644 include/hw/nvram/xlnx-efuse.h create mode 100644 hw/nvram/xlnx-efuse-crc.c create mode 100644 hw/nvram/xlnx-efuse.c diff --git a/include/hw/nvram/xlnx-efuse.h b/include/hw/nvram/xlnx-efuse.h new file mode 100644 index 000..58414e468b5 --- /dev/null +++ b/include/hw/nvram/xlnx-efuse.h @@ -0,0 +1,132 @@ +/* + * QEMU model of the Xilinx eFuse core + * + * Copyright (c) 2015 Xilinx Inc. + * + * Written by Edgar E. Iglesias + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#ifndef XLNX_EFUSE_H +#define XLNX_EFUSE_H + +#include "sysemu/block-backend.h" +#include "hw/qdev-core.h" + +#define TYPE_XLNX_EFUSE "xlnx,efuse" +OBJECT_DECLARE_SIMPLE_TYPE(XlnxEFuse, XLNX_EFUSE); + +struct XlnxEFuse { +DeviceState parent_obj; +BlockBackend *blk; +bool blk_ro; +uint32_t *fuse32; + +DeviceState *dev; + +bool init_tbits; + +uint8_t efuse_nr; +uint32_t efuse_size; + +uint32_t *ro_bits; +uint32_t ro_bits_cnt; +}; + +/** + * xlnx_efuse_calc_crc: + * @data: an array of 32-bit words for which the CRC should be computed + * @u32_cnt: the array size in number of 32-bit words + * @zpads: the number of 32-bit zeros prepended to @data before computation + * + * This function is used to compute the CRC for an array of 32-bit words, + * using a Xilinx-specific data padding. + * + * Returns: the computed 32-bit CRC + */ +uint32_t xlnx_efuse_calc_crc(const uint32_t *data, unsigned u32_cnt, + unsigned zpads); + +/** + * xlnx_efuse_get_bit: + * @s: the efuse object + * @bit: the efuse bit-address to read the data + * + * Returns: the bit, 0 or 1, at @bit of object @s + */ +bool xlnx_efuse_get_bit(XlnxEFuse *s, unsigned int bit); + +/** + * xlnx_efuse_set_bit: + * @s: the efuse object + * @bit: the efuse bit-address to be written a value of 1 + * + * Returns: true on success, false on failure + */ +bool xlnx_efuse_set_bit(XlnxEFuse *s, unsigned int bit); + +/** + * xlnx_efuse_k256_check: + * @s: the efuse object + * @crc: the 32-bit CRC to be compared with + * @start: the efuse bit-address (which must be multiple of 32) of the + * start of a 256-bit array + * + * This function computes the CRC of a 256-bit array starting at @start + * then compares to the given @crc + * + * Returns: true of @crc == computed, false otherwise + */ +bool xlnx_efuse_k256_check(XlnxEFuse *s, uint32_t crc, unsigned start); + +/** + * xlnx_efuse_tbits_check: + * @s: the efuse object + * + * This function inspects a number of efuse bits at specific addresses + * to see if they match a validation pattern. Each pattern is a group + * of 4 bits, and there are 3 groups. + * + * Returns: a 3-bit mask, where a bit of '1' means the corresponding + * group has a valid pattern. + */ +uint32_t xlnx_efuse_tbits_check(XlnxEFuse *s); + +/** + * xlnx_efuse_get_row: + * @s: the efuse object + * @bit: the efuse bit address for which a 32-bit value is read + * + * Returns: the entire 32 bits of the efuse, starting at a bit + * address that is multiple of 32 and contains the bit at @bit + */ +static inline uint32_t xlnx_efuse_get_row(XlnxEFuse *s, unsigned int bit) +{ +
[PULL 11/22] docs/system/arm: xlnx-versal-virt: BBRAM and eFUSE Usage
From: Tong Ho Add BBRAM and eFUSE usage to the Xilinx Versal Virt board document. Signed-off-by: Tong Ho Message-id: 20210917052400.1249094-10-tong...@xilinx.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- docs/system/arm/xlnx-versal-virt.rst | 49 1 file changed, 49 insertions(+) diff --git a/docs/system/arm/xlnx-versal-virt.rst b/docs/system/arm/xlnx-versal-virt.rst index 27f73500d95..92ad10d2da4 100644 --- a/docs/system/arm/xlnx-versal-virt.rst +++ b/docs/system/arm/xlnx-versal-virt.rst @@ -32,6 +32,8 @@ Implemented devices: - OCM (256KB of On Chip Memory) - XRAM (4MB of on chip Accelerator RAM) - DDR memory +- BBRAM (36 bytes of Battery-backed RAM) +- eFUSE (3072 bytes of one-time field-programmable bit array) QEMU does not yet model any other devices, including the PL and the AI Engine. @@ -175,3 +177,50 @@ Run the following at the U-Boot prompt: fdt set /chosen/dom0 reg <0x 0x4000 0x0 0x0310> booti 3000 - 2000 +BBRAM File Backend +"" +BBRAM can have an optional file backend, which must be a seekable +binary file with a size of 36 bytes or larger. A file with all +binary 0s is a 'blank'. + +To add a file-backend for the BBRAM: + +.. code-block:: bash + + -drive if=pflash,index=0,file=versal-bbram.bin,format=raw + +To use a different index value, N, from default of 0, add: + +.. code-block:: bash + + -global xlnx,bbram-ctrl.drive-index=N + +eFUSE File Backend +"" +eFUSE can have an optional file backend, which must be a seekable +binary file with a size of 3072 bytes or larger. A file with all +binary 0s is a 'blank'. + +To add a file-backend for the eFUSE: + +.. code-block:: bash + + -drive if=pflash,index=1,file=versal-efuse.bin,format=raw + +To use a different index value, N, from default of 1, add: + +.. code-block:: bash + + -global xlnx,efuse.drive-index=N + +.. warning:: + In actual physical Versal, BBRAM and eFUSE contain sensitive data. + The QEMU device models do **not** encrypt nor obfuscate any data + when holding them in models' memory or when writing them to their + file backends. + + Thus, a file backend should be used with caution, and 'format=luks' + is highly recommended (albeit with usage complexity). + + Better yet, do not use actual product data when running guest image + on this Xilinx Versal Virt board. -- 2.20.1
[PULL 02/22] arm: tcg: Adhere to SMCCC 1.3 section 5.2
From: Alexander Graf The SMCCC 1.3 spec section 5.2 says The Unknown SMC Function Identifier is a sign-extended value of (-1) that is returned in the R0, W0 or X0 registers. An implementation must return this error code when it receives: * An SMC or HVC call with an unknown Function Identifier * An SMC or HVC call for a removed Function Identifier * An SMC64/HVC64 call from AArch32 state To comply with these statements, let's always return -1 when we encounter an unknown HVC or SMC call. Signed-off-by: Alexander Graf Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/psci.c | 35 ++- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/target/arm/psci.c b/target/arm/psci.c index 6709e280133..b279c0b9a45 100644 --- a/target/arm/psci.c +++ b/target/arm/psci.c @@ -27,15 +27,13 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type) { -/* Return true if the r0/x0 value indicates a PSCI call and - * the exception type matches the configured PSCI conduit. This is - * called before the SMC/HVC instruction is executed, to decide whether - * we should treat it as a PSCI call or with the architecturally +/* + * Return true if the exception type matches the configured PSCI conduit. + * This is called before the SMC/HVC instruction is executed, to decide + * whether we should treat it as a PSCI call or with the architecturally * defined behaviour for an SMC or HVC (which might be UNDEF or trap * to EL2 or to EL3). */ -CPUARMState *env = &cpu->env; -uint64_t param = is_a64(env) ? env->xregs[0] : env->regs[0]; switch (excp_type) { case EXCP_HVC: @@ -52,27 +50,7 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type) return false; } -switch (param) { -case QEMU_PSCI_0_2_FN_PSCI_VERSION: -case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE: -case QEMU_PSCI_0_2_FN_AFFINITY_INFO: -case QEMU_PSCI_0_2_FN64_AFFINITY_INFO: -case QEMU_PSCI_0_2_FN_SYSTEM_RESET: -case QEMU_PSCI_0_2_FN_SYSTEM_OFF: -case QEMU_PSCI_0_1_FN_CPU_ON: -case QEMU_PSCI_0_2_FN_CPU_ON: -case QEMU_PSCI_0_2_FN64_CPU_ON: -case QEMU_PSCI_0_1_FN_CPU_OFF: -case QEMU_PSCI_0_2_FN_CPU_OFF: -case QEMU_PSCI_0_1_FN_CPU_SUSPEND: -case QEMU_PSCI_0_2_FN_CPU_SUSPEND: -case QEMU_PSCI_0_2_FN64_CPU_SUSPEND: -case QEMU_PSCI_0_1_FN_MIGRATE: -case QEMU_PSCI_0_2_FN_MIGRATE: -return true; -default: -return false; -} +return true; } void arm_handle_psci_call(ARMCPU *cpu) @@ -194,10 +172,9 @@ void arm_handle_psci_call(ARMCPU *cpu) break; case QEMU_PSCI_0_1_FN_MIGRATE: case QEMU_PSCI_0_2_FN_MIGRATE: +default: ret = QEMU_PSCI_RET_NOT_SUPPORTED; break; -default: -g_assert_not_reached(); } err: -- 2.20.1
[PULL 21/22] ide: Rename ide_bus_new() to ide_bus_init()
The function ide_bus_new() does an in-place initialization. Rename it to ide_bus_init() to follow our _init vs _new convention. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Michael S. Tsirkin Reviewed-by: Corey Minyard Reviewed-by: John Snow Acked-by: John Snow (Feel free to merge.) Message-id: 20210923121153.23754-7-peter.mayd...@linaro.org --- include/hw/ide/internal.h | 4 ++-- hw/ide/ahci.c | 2 +- hw/ide/cmd646.c | 2 +- hw/ide/isa.c | 2 +- hw/ide/macio.c| 2 +- hw/ide/microdrive.c | 2 +- hw/ide/mmio.c | 2 +- hw/ide/piix.c | 2 +- hw/ide/qdev.c | 2 +- hw/ide/sii3112.c | 2 +- hw/ide/via.c | 2 +- 11 files changed, 12 insertions(+), 12 deletions(-) diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index 79172217ccb..97e7e59dc58 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -648,8 +648,8 @@ void ide_atapi_cmd(IDEState *s); void ide_atapi_cmd_reply_end(IDEState *s); /* hw/ide/qdev.c */ -void ide_bus_new(IDEBus *idebus, size_t idebus_size, DeviceState *dev, - int bus_id, int max_units); +void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev, + int bus_id, int max_units); IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive); int ide_handle_rw_error(IDEState *s, int error, int op); diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index f2c51574839..a94c6e26fb0 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1548,7 +1548,7 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports) for (i = 0; i < s->ports; i++) { AHCIDevice *ad = &s->dev[i]; -ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1); +ide_bus_init(&ad->port, sizeof(ad->port), qdev, i, 1); ide_init2(&ad->port, irqs[i]); ad->hba = s; diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index c2546314855..94c576262c1 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -293,7 +293,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) qdev_init_gpio_in(ds, cmd646_set_irq, 2); for (i = 0; i < 2; i++) { -ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2); +ide_bus_init(&d->bus[i], sizeof(d->bus[i]), ds, i, 2); ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i)); bmdma_init(&d->bus[i], &d->bmdma[i], d); diff --git a/hw/ide/isa.c b/hw/ide/isa.c index 6bc19de2265..24bbde24c2b 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) ISADevice *isadev = ISA_DEVICE(dev); ISAIDEState *s = ISA_IDE(dev); -ide_bus_new(&s->bus, sizeof(s->bus), dev, 0, 2); +ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); isa_init_irq(isadev, &s->irq, s->isairq); ide_init2(&s->bus, s->irq); diff --git a/hw/ide/macio.c b/hw/ide/macio.c index b270a101632..b03d401ceb5 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -449,7 +449,7 @@ static void macio_ide_initfn(Object *obj) SysBusDevice *d = SYS_BUS_DEVICE(obj); MACIOIDEState *s = MACIO_IDE(obj); -ide_bus_new(&s->bus, sizeof(s->bus), DEVICE(obj), 0, 2); +ide_bus_init(&s->bus, sizeof(s->bus), DEVICE(obj), 0, 2); memory_region_init_io(&s->mem, obj, &pmac_ide_ops, s, "pmac-ide", 0x1000); sysbus_init_mmio(d, &s->mem); sysbus_init_irq(d, &s->real_ide_irq); diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c index 58a14fea363..6df9b4cbbe1 100644 --- a/hw/ide/microdrive.c +++ b/hw/ide/microdrive.c @@ -605,7 +605,7 @@ static void microdrive_init(Object *obj) { MicroDriveState *md = MICRODRIVE(obj); -ide_bus_new(&md->bus, sizeof(md->bus), DEVICE(obj), 0, 1); +ide_bus_init(&md->bus, sizeof(md->bus), DEVICE(obj), 0, 1); } static void microdrive_class_init(ObjectClass *oc, void *data) diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c index 36e2f4790ab..fb2ebd4847f 100644 --- a/hw/ide/mmio.c +++ b/hw/ide/mmio.c @@ -142,7 +142,7 @@ static void mmio_ide_initfn(Object *obj) SysBusDevice *d = SYS_BUS_DEVICE(obj); MMIOState *s = MMIO_IDE(obj); -ide_bus_new(&s->bus, sizeof(s->bus), DEVICE(obj), 0, 2); +ide_bus_init(&s->bus, sizeof(s->bus), DEVICE(obj), 0, 2); sysbus_init_irq(d, &s->irq); } diff --git a/hw/ide/piix.c b/hw/ide/piix.c index d3e738320be..ce89fd0aa36 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -137,7 +137,7 @@ static int pci_piix_init_ports(PCIIDEState *d) int i, ret; for (i = 0; i < 2; i++) { -ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2); +ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2); ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase, port_info[i].iobase2); if (ret) { diff --git
[PULL 01/22] allwinner-h3: Switch to SMC as PSCI conduit
From: Alexander Graf The Allwinner H3 SoC uses Cortex-A7 cores which support virtualization. However, today we are configuring QEMU to use HVC as PSCI conduit. That means HVC calls get trapped into QEMU instead of the guest's own emulated CPU and thus break the guest's ability to execute virtualization. Fix this by moving to SMC as conduit, freeing up HYP completely to the VM. Signed-off-by: Alexander Graf Message-id: 20210920203931.66527-1-ag...@csgraf.de Fixes: 740dafc0ba0 ("hw/arm: add Allwinner H3 System-on-Chip") Reviewed-by: Niek Linnenbank Tested-by: Niek Linnenbank Reviewed-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Peter Maydell --- hw/arm/allwinner-h3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c index 27f10701453..f9b7ed18711 100644 --- a/hw/arm/allwinner-h3.c +++ b/hw/arm/allwinner-h3.c @@ -237,7 +237,7 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp) /* Provide Power State Coordination Interface */ qdev_prop_set_int32(DEVICE(&s->cpus[i]), "psci-conduit", -QEMU_PSCI_CONDUIT_HVC); +QEMU_PSCI_CONDUIT_SMC); /* Disable secondary CPUs */ qdev_prop_set_bit(DEVICE(&s->cpus[i]), "start-powered-off", -- 2.20.1
[PULL 10/22] hw/arm: xlnx-zcu102: Add Xilinx eFUSE device
From: Tong Ho Connect the support for ZynqMP eFUSE one-time field-programmable bit array. The command argument: -drive if=pflash,index=3,... Can be used to optionally connect the bit array to a backend storage, such that field-programmed values in one invocation can be made available to next invocation. The backend storage must be a seekable binary file, and its size must be 768 bytes or larger. A file with all binary 0's is a 'blank'. Signed-off-by: Tong Ho Message-id: 20210917052400.1249094-9-tong...@xilinx.com Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- include/hw/arm/xlnx-zynqmp.h | 3 +++ hw/arm/xlnx-zcu102.c | 15 +++ hw/arm/xlnx-zynqmp.c | 29 + hw/Kconfig | 1 + 4 files changed, 48 insertions(+) diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h index 067e8a5238a..062e637fe49 100644 --- a/include/hw/arm/xlnx-zynqmp.h +++ b/include/hw/arm/xlnx-zynqmp.h @@ -37,6 +37,7 @@ #include "net/can_emu.h" #include "hw/dma/xlnx_csu_dma.h" #include "hw/nvram/xlnx-bbram.h" +#include "hw/nvram/xlnx-zynqmp-efuse.h" #define TYPE_XLNX_ZYNQMP "xlnx-zynqmp" OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP) @@ -102,6 +103,8 @@ struct XlnxZynqMPState { MemoryRegion *ddr_ram; MemoryRegion ddr_ram_low, ddr_ram_high; XlnxBBRam bbram; +XlnxEFuse efuse; +XlnxZynqMPEFuse efuse_ctrl; MemoryRegion mr_unimp[XLNX_ZYNQMP_NUM_UNIMP_AREAS]; diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c index b247c5779bf..3dc2b5e8ca4 100644 --- a/hw/arm/xlnx-zcu102.c +++ b/hw/arm/xlnx-zcu102.c @@ -110,6 +110,18 @@ static void bbram_attach_drive(XlnxBBRam *dev) } } +static void efuse_attach_drive(XlnxEFuse *dev) +{ +DriveInfo *dinfo; +BlockBackend *blk; + +dinfo = drive_get_by_index(IF_PFLASH, 3); +blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; +if (blk) { +qdev_prop_set_drive(DEVICE(dev), "drive", blk); +} +} + static void xlnx_zcu102_init(MachineState *machine) { XlnxZCU102 *s = ZCU102_MACHINE(machine); @@ -151,6 +163,9 @@ static void xlnx_zcu102_init(MachineState *machine) /* Attach bbram backend, if given */ bbram_attach_drive(&s->soc.bbram); +/* Attach efuse backend, if given */ +efuse_attach_drive(&s->soc.efuse); + /* Create and plug in the SD cards */ for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) { BusState *bus; diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index 1e8e2ddcc27..1c52a575aad 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -69,6 +69,9 @@ #define BBRAM_ADDR 0xffcd #define BBRAM_IRQ 11 +#define EFUSE_ADDR 0xffcc +#define EFUSE_IRQ 87 + #define SDHCI_CAPABILITIES 0x280737ec6481 /* Datasheet: UG1085 (v1.7) */ static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = { @@ -245,6 +248,31 @@ static void xlnx_zynqmp_create_bbram(XlnxZynqMPState *s, qemu_irq *gic) sysbus_connect_irq(sbd, 0, gic[BBRAM_IRQ]); } +static void xlnx_zynqmp_create_efuse(XlnxZynqMPState *s, qemu_irq *gic) +{ +Object *bits = OBJECT(&s->efuse); +Object *ctrl = OBJECT(&s->efuse_ctrl); +SysBusDevice *sbd; + +object_initialize_child(OBJECT(s), "efuse-ctrl", &s->efuse_ctrl, +TYPE_XLNX_ZYNQMP_EFUSE); + +object_initialize_child_with_props(ctrl, "xlnx-efuse@0", bits, + sizeof(s->efuse), + TYPE_XLNX_EFUSE, &error_abort, + "efuse-nr", "3", + "efuse-size", "2048", + NULL); + +qdev_realize(DEVICE(bits), NULL, &error_abort); +object_property_set_link(ctrl, "efuse", bits, &error_abort); + +sbd = SYS_BUS_DEVICE(ctrl); +sysbus_realize(sbd, &error_abort); +sysbus_mmio_map(sbd, 0, EFUSE_ADDR); +sysbus_connect_irq(sbd, 0, gic[EFUSE_IRQ]); +} + static void xlnx_zynqmp_create_unimp_mmio(XlnxZynqMPState *s) { static const struct UnimpInfo { @@ -646,6 +674,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) sysbus_connect_irq(SYS_BUS_DEVICE(&s->rtc), 0, gic_spi[RTC_IRQ]); xlnx_zynqmp_create_bbram(s, gic_spi); +xlnx_zynqmp_create_efuse(s, gic_spi); xlnx_zynqmp_create_unimp_mmio(s); for (i = 0; i < XLNX_ZYNQMP_NUM_GDMA_CH; i++) { diff --git a/hw/Kconfig b/hw/Kconfig index b6fb6a45074..ad20cce0a95 100644 --- a/hw/Kconfig +++ b/hw/Kconfig @@ -82,3 +82,4 @@ config XLNX_ZYNQMP select CAN_BUS select PTIMER select XLNX_BBRAM +select XLNX_EFUSE_ZYNQMP -- 2.20.1
[PATCH 2/3] hw/intc/arm_gicv3: Set GICR_TYPER.Last correctly when nb_redist_regions > 1
The 'Last' bit in the GICR_TYPER GICv3 redistributor register is supposed to be set to 1 if this is the last redistributor in a series of contiguous redistributor pages. Currently we set Last only for the redistributor for CPU (num_cpu - 1). This only works if there is a single redistributor region; if there are multiple redistributor regions then we need to set the Last bit for the last redistributor in each region. This doesn't cause any problems currently because only the KVM GICv3 supports multiple redistributor regions, and it ignores the value in GICv3State::gicr_typer. But we need to fix this before we can enable support for multiple regions in the emulated GICv3. Signed-off-by: Peter Maydell --- hw/intc/arm_gicv3_common.c | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index 8e47809398b..8de9205b386 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -297,7 +297,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) { GICv3State *s = ARM_GICV3_COMMON(dev); -int i, rdist_capacity; +int i, rdist_capacity, cpuidx; /* revision property is actually reserved and currently used only in order * to keep the interface compatible with GICv2 code, avoiding extra @@ -355,7 +355,6 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) for (i = 0; i < s->num_cpu; i++) { CPUState *cpu = qemu_get_cpu(i); uint64_t cpu_affid; -int last; s->cpu[i].cpu = cpu; s->cpu[i].gic = s; @@ -375,7 +374,6 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) * PLPIS == 0 (physical LPIs not supported) */ cpu_affid = object_property_get_uint(OBJECT(cpu), "mp-affinity", NULL); -last = (i == s->num_cpu - 1); /* The CPU mp-affinity property is in MPIDR register format; squash * the affinity bytes into 32 bits as the GICR_TYPER has them. @@ -384,13 +382,22 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) (cpu_affid & 0xFF); s->cpu[i].gicr_typer = (cpu_affid << 32) | (1 << 24) | -(i << 8) | -(last << 4); +(i << 8); if (s->lpi_enable) { s->cpu[i].gicr_typer |= GICR_TYPER_PLPIS; } } + +/* + * Now go through and set GICR_TYPER.Last for the final + * redistributor in each region. + */ +cpuidx = 0; +for (i = 0; i < s->nb_redist_regions; i++) { +cpuidx += s->redist_region_count[i]; +s->cpu[cpuidx - 1].gicr_typer |= GICR_TYPER_LAST; +} } static void arm_gicv3_finalize(Object *obj) -- 2.20.1