From: "Cédric Le Goater" <clg@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>,
marcandre.lureau@redhat.com, qemu-devel@nongnu.org
Cc: "Eduardo Habkost" <eduardo@habkost.net>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Yanan Wang" <wangyanan55@huawei.com>,
"Alex Williamson" <alex.williamson@redhat.com>,
"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH v5 3/3] hw/vfio: add ramfb migration support
Date: Tue, 10 Oct 2023 09:03:27 +0200 [thread overview]
Message-ID: <40e84da6-a8cc-b4ea-4a81-0d0fded690dc@redhat.com> (raw)
In-Reply-To: <a9f3a4e4-5084-897c-a9de-df80f5302e76@redhat.com>
On 10/9/23 12:19, Laszlo Ersek wrote:
> On 10/9/23 08:32, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
>>
>> Turn it off by default on machines <= 8.1 for compatibility reasons.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> hw/vfio/pci.h | 3 +++
>> hw/core/machine.c | 1 +
>> hw/vfio/display.c | 21 +++++++++++++++++++++
>> hw/vfio/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>> stubs/ramfb.c | 2 ++
>> 5 files changed, 71 insertions(+)
>>
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 2d836093a8..fd06695542 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -173,6 +173,7 @@ struct VFIOPCIDevice {
>> bool no_kvm_ioeventfd;
>> bool no_vfio_ioeventfd;
>> bool enable_ramfb;
>> + OnOffAuto ramfb_migrate;
>> bool defer_kvm_irq_routing;
>> bool clear_parent_atomics_on_exit;
>> VFIODisplay *dpy;
>> @@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
>> int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
>> void vfio_display_finalize(VFIOPCIDevice *vdev);
>>
>> +extern const VMStateDescription vfio_display_vmstate;
>> +
>> #endif /* HW_VFIO_VFIO_PCI_H */
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 6305f2d7a4..05aef2cf9f 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -35,6 +35,7 @@
>> GlobalProperty hw_compat_8_1[] = {
>> { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
>> { "ramfb", "x-migrate", "off" },
>> + { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
>> };
>> const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>>
>> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
>> index bec864f482..2739ba56ec 100644
>> --- a/hw/vfio/display.c
>> +++ b/hw/vfio/display.c
>> @@ -542,3 +542,24 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>> vfio_display_edid_exit(vdev->dpy);
>> g_free(vdev->dpy);
>> }
>> +
>> +static bool migrate_needed(void *opaque)
>> +{
>> + VFIODisplay *dpy = opaque;
>> + bool ramfb_exists = dpy->ramfb != NULL;
>> +
>> + /* see vfio_display_migration_needed() */
>> + assert(ramfb_exists);
>> + return ramfb_exists;
>> +}
>> +
>> +const VMStateDescription vfio_display_vmstate = {
>> + .name = "VFIODisplay",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = migrate_needed,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),
>> + VMSTATE_END_OF_LIST(),
>> + }
>> +};
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 3b2ca3c24c..e44ed21180 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int version_id)
>> return msix_present(pdev);
>> }
>>
>> +static bool vfio_display_migration_needed(void *opaque)
>> +{
>> + VFIOPCIDevice *vdev = opaque;
>> +
>> + /*
>> + * We need to migrate the VFIODisplay object if ramfb *migration* was
>> + * explicitly requested (in which case we enforced both ramfb=on and
>> + * display=on), or ramfb migration was left at the default "auto"
>> + * setting, and *ramfb* was explicitly requested (in which case we
>> + * enforced display=on).
>> + */
>> + return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
>> + (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
>> +}
>> +
>> +const VMStateDescription vmstate_vfio_display = {
>> + .name = "VFIOPCIDevice/VFIODisplay",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = vfio_display_migration_needed,
>> + .fields = (VMStateField[]){
>> + VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate, VFIODisplay),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> const VMStateDescription vmstate_vfio_pci_config = {
>> .name = "VFIOPCIDevice",
>> .version_id = 1,
>> @@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
>> VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>> VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
>> VMSTATE_END_OF_LIST()
>> + },
>> + .subsections = (const VMStateDescription*[]) {
>> + &vmstate_vfio_display,
>> + NULL
>> }
>> };
>>
>> @@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>> }
>> }
>>
>> + if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
>> + warn_report("x-ramfb-migrate=on but ramfb=off");
>> + vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
>
> the warning could give a hint about the resultant action taken (i.e.,
> forcing off x-ramfb-migrate), but don't repost just for that; it's a nit.
yes.
How about :
warn_report("x-ramfb-migrate=on but ramfb=off. Forcing x-ramfb-migrate to off.");
I can amend the patch if you agree.
Thanks,
C.
> My R-b stands.
>
> Thanks
> Laszlo
>
>> + }
>> + if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
>> + if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
>> + vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
>> + } else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
>> + error_setg(errp, "x-ramfb-migrate requires enable-migration");
>> + goto out_deregister;
>> + }
>> + }
>> +
>> if (!pdev->failover_pair_id) {
>> if (!vfio_migration_realize(vbasedev, errp)) {
>> goto out_deregister;
>> @@ -3484,6 +3527,7 @@ static const TypeInfo vfio_pci_dev_info = {
>>
>> static Property vfio_pci_dev_nohotplug_properties[] = {
>> DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
>> + DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate, ON_OFF_AUTO_AUTO),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
>> index 48143f3354..cf64733b10 100644
>> --- a/stubs/ramfb.c
>> +++ b/stubs/ramfb.c
>> @@ -2,6 +2,8 @@
>> #include "qapi/error.h"
>> #include "hw/display/ramfb.h"
>>
>> +const VMStateDescription ramfb_vmstate = {};
>> +
>> void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>> {
>> }
>
next prev parent reply other threads:[~2023-10-10 7:04 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-09 6:32 [PATCH v5 0/3] ramfb: migration support marcandre.lureau
2023-10-09 6:32 ` [PATCH v5 1/3] ramfb: add " marcandre.lureau
2023-10-09 6:32 ` [PATCH v5 2/3] ramfb-standalone: " marcandre.lureau
2023-10-09 6:32 ` [PATCH v5 3/3] hw/vfio: add ramfb " marcandre.lureau
2023-10-09 10:19 ` Laszlo Ersek
2023-10-10 7:03 ` Cédric Le Goater [this message]
2023-10-10 7:08 ` Marc-André Lureau
2023-10-10 12:48 ` [PATCH v5 0/3] ramfb: " Cédric Le Goater
2023-10-10 13:46 ` Marc-André Lureau
2023-10-11 17:09 ` Cédric Le Goater
2023-10-12 9:18 ` Gerd Hoffmann
2023-10-12 10:11 ` Cédric Le Goater
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=40e84da6-a8cc-b4ea-4a81-0d0fded690dc@redhat.com \
--to=clg@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=eduardo@habkost.net \
--cc=lersek@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=pbonzini@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=wangyanan55@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).