rom_bar is tristate but was defined as uint32_t so convert it into
OnOffAuto.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
docs/igd-assign.txt | 2 +-
include/hw/pci/pci_device.h | 2 +-
hw/pci/pci.c | 4 ++--
hw/vfio/pci-quirks.c | 2 +-
hw/vfio/pci.c | 11 +++++------
hw/xen/xen_pt_load_rom.c | 4 ++--
tests/qtest/virtio-net-failover.c | 32 ++++++++++++++++----------------
7 files changed, 28 insertions(+), 29 deletions(-)
diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt
index e17bb50789ad..35c6c8e28493 100644
--- a/docs/igd-assign.txt
+++ b/docs/igd-assign.txt
@@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci:
ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at
PCI address 1f.0.
* The IGD device must have a VGA ROM, either provided via the romfile
- option or loaded automatically through vfio (standard). rombar=0
+ option or loaded automatically through vfio (standard). rombar=off
will disable legacy mode support.
* Hotplug of the IGD device is not supported.
* The IGD device must be a SandyBridge or newer model device.
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index ca151325085d..49b341ce2e27 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -147,7 +147,7 @@ struct PCIDevice {
uint32_t romsize;
bool has_rom;
MemoryRegion rom;
- uint32_t rom_bar;
+ OnOffAuto rom_bar;
/* INTx routing notifier */
PCIINTxRoutingNotifier intx_routing_notifier;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1eb6abf534ca..901f5460d774 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -71,7 +71,7 @@ static Property pci_props[] = {
DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, UINT32_MAX),
- DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
+ DEFINE_PROP_ON_OFF_AUTO("rombar", PCIDevice, rom_bar, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present,
@@ -2334,7 +2334,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
return;
}
- if (!pdev->rom_bar) {
+ if (pdev->rom_bar == ON_OFF_AUTO_OFF) {
/*
* Load rom via fw_cfg instead of creating a rom bar,
* for 0.11 compatibility.
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 39dae72497e0..0e920ed0691a 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -33,7 +33,7 @@
* execution as noticed with the BCM 57810 card for lack of a
* more better way to handle such issues.
* The user can still override by specifying a romfile or
- * rombar=1.
+ * rombar=on.
* Please see https://bugs.launchpad.net/qemu/+bug/1284874
* for an analysis of the 57810 card hang. When adding
* a new vendor id/device id combination below, please also add
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 74a79bdf61f9..4c4d9dc81efb 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -902,7 +902,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
error_report("vfio-pci: Cannot read device rom at "
"%s", vdev->vbasedev.name);
error_printf("Device option ROM contents are probably invalid "
- "(check dmesg).\nSkip option ROM probe with rombar=0, "
+ "(check dmesg).\nSkip option ROM probe with rombar=off, "
"or load from file with romfile=\n");
return;
}
@@ -1012,11 +1012,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
{
uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
- DeviceState *dev = DEVICE(vdev);
char *name;
int fd = vdev->vbasedev.fd;
- if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
+ if (vdev->pdev.romfile || vdev->pdev.rom_bar == ON_OFF_AUTO_OFF) {
/* Since pci handles romfile, just print a message and return */
if (vfio_opt_rom_in_denylist(vdev) && vdev->pdev.romfile) {
warn_report("Device at %s is known to cause system instability"
@@ -1046,17 +1045,17 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
}
if (vfio_opt_rom_in_denylist(vdev)) {
- if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
+ if (vdev->pdev.rom_bar == ON_OFF_AUTO_ON) {
warn_report("Device at %s is known to cause system instability"
" issues during option rom execution",
vdev->vbasedev.name);
error_printf("Proceeding anyway since user specified"
- " non zero value for rombar\n");
+ " on for rombar\n");
} else {
warn_report("Rom loading for device at %s has been disabled"
" due to system instability issues",
vdev->vbasedev.name);
- error_printf("Specify rombar=1 or romfile to force\n");
+ error_printf("Specify rombar=on or romfile to force\n");
return;
}
}
diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
index 6bc64acd3352..025a6b25a916 100644
--- a/hw/xen/xen_pt_load_rom.c
+++ b/hw/xen/xen_pt_load_rom.c
@@ -26,7 +26,7 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev,
Object *owner = OBJECT(dev);
/* If loading ROM from file, pci handles it */
- if (dev->romfile || !dev->rom_bar) {
+ if (dev->romfile || dev->rom_bar == ON_OFF_AUTO_OFF) {
return NULL;
}
@@ -71,7 +71,7 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev,
if (!fread(ptr, 1, st.st_size, fp)) {
error_report("pci-assign: Cannot read from host %s", rom_file);
error_printf("Device option ROM contents are probably invalid "
- "(check dmesg).\nSkip option ROM probe with rombar=0, "
+ "(check dmesg).\nSkip option ROM probe with rombar=off, "
"or load from file with romfile=\n");
goto close_rom;
}
diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
index 73dfabc2728b..f65b97683fb6 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -568,7 +568,7 @@ static void test_hotplug_2_reverse(void)
"{'bus': 'root0',"
"'failover': true,"
"'netdev': 'hs0',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_STANDBY0"'}");
@@ -655,7 +655,7 @@ static void test_migrate_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -765,7 +765,7 @@ static void test_migrate_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -819,7 +819,7 @@ static void test_off_migrate_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -887,7 +887,7 @@ static void test_off_migrate_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -938,7 +938,7 @@ static void test_guest_off_migrate_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1014,7 +1014,7 @@ static void test_guest_off_migrate_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1065,7 +1065,7 @@ static void test_migrate_guest_off_abort(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1170,7 +1170,7 @@ static void test_migrate_abort_wait_unplug(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1259,7 +1259,7 @@ static void test_migrate_abort_active(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1358,7 +1358,7 @@ static void test_migrate_off_abort(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1450,7 +1450,7 @@ static void test_migrate_abort_timeout(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1543,7 +1543,7 @@ static void test_multi_out(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1574,7 +1574,7 @@ static void test_multi_out(gconstpointer opaque)
"{'bus': 'root3',"
"'failover_pair_id': 'standby1',"
"'netdev': 'hs3',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY1"'}");
@@ -1713,7 +1713,7 @@ static void test_multi_in(gconstpointer opaque)
"{'bus': 'root1',"
"'failover_pair_id': 'standby0',"
"'netdev': 'hs1',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY0"'}");
@@ -1737,7 +1737,7 @@ static void test_multi_in(gconstpointer opaque)
"{'bus': 'root3',"
"'failover_pair_id': 'standby1',"
"'netdev': 'hs3',"
- "'rombar': 0,"
+ "'rombar': 'off',"
"'romfile': '',"
"'mac': '"MAC_PRIMARY1"'}");
--
2.45.2On Thu, Jun 27, 2024 at 03:08:00PM +0900, Akihiko Odaki wrote: > rom_bar is tristate but was defined as uint32_t so convert it into > OnOffAuto. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Commit log should explain why this is an improvement, not just what's done. > diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt > index e17bb50789ad..35c6c8e28493 100644 > --- a/docs/igd-assign.txt > +++ b/docs/igd-assign.txt > @@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci: > ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at > PCI address 1f.0. > * The IGD device must have a VGA ROM, either provided via the romfile > - option or loaded automatically through vfio (standard). rombar=0 > + option or loaded automatically through vfio (standard). rombar=off > will disable legacy mode support. > * Hotplug of the IGD device is not supported. > * The IGD device must be a SandyBridge or newer model device. ... > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c > index 39dae72497e0..0e920ed0691a 100644 > --- a/hw/vfio/pci-quirks.c > +++ b/hw/vfio/pci-quirks.c > @@ -33,7 +33,7 @@ > * execution as noticed with the BCM 57810 card for lack of a > * more better way to handle such issues. > * The user can still override by specifying a romfile or > - * rombar=1. > + * rombar=on. > * Please see https://bugs.launchpad.net/qemu/+bug/1284874 > * for an analysis of the 57810 card hang. When adding > * a new vendor id/device id combination below, please also add So we are apparently breaking a bunch of users who followed documentation to the dot. Why is this a good idea? -- MST
On Tue, 2 Jul 2024, Michael S. Tsirkin wrote: > On Thu, Jun 27, 2024 at 03:08:00PM +0900, Akihiko Odaki wrote: >> rom_bar is tristate but was defined as uint32_t so convert it into >> OnOffAuto. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > Commit log should explain why this is an improvement, > not just what's done. > > >> diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt >> index e17bb50789ad..35c6c8e28493 100644 >> --- a/docs/igd-assign.txt >> +++ b/docs/igd-assign.txt >> @@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci: >> ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at >> PCI address 1f.0. >> * The IGD device must have a VGA ROM, either provided via the romfile >> - option or loaded automatically through vfio (standard). rombar=0 >> + option or loaded automatically through vfio (standard). rombar=off >> will disable legacy mode support. >> * Hotplug of the IGD device is not supported. >> * The IGD device must be a SandyBridge or newer model device. > > ... > >> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c >> index 39dae72497e0..0e920ed0691a 100644 >> --- a/hw/vfio/pci-quirks.c >> +++ b/hw/vfio/pci-quirks.c >> @@ -33,7 +33,7 @@ >> * execution as noticed with the BCM 57810 card for lack of a >> * more better way to handle such issues. >> * The user can still override by specifying a romfile or >> - * rombar=1. >> + * rombar=on. >> * Please see https://bugs.launchpad.net/qemu/+bug/1284874 >> * for an analysis of the 57810 card hang. When adding >> * a new vendor id/device id combination below, please also add > > > So we are apparently breaking a bunch of users who followed > documentation to the dot. Why is this a good idea? On/off is clearer than 1/0. But isn't 1/0 a synonym for on/off so previous command lines would still work? Regards, BALATON Zoltan
On Wed, Jul 03, 2024 at 04:15:23AM +0200, BALATON Zoltan wrote:
> On Tue, 2 Jul 2024, Michael S. Tsirkin wrote:
> > On Thu, Jun 27, 2024 at 03:08:00PM +0900, Akihiko Odaki wrote:
> > > rom_bar is tristate but was defined as uint32_t so convert it into
> > > OnOffAuto.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >
> > Commit log should explain why this is an improvement,
> > not just what's done.
> >
> >
> > > diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt
> > > index e17bb50789ad..35c6c8e28493 100644
> > > --- a/docs/igd-assign.txt
> > > +++ b/docs/igd-assign.txt
> > > @@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci:
> > > ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at
> > > PCI address 1f.0.
> > > * The IGD device must have a VGA ROM, either provided via the romfile
> > > - option or loaded automatically through vfio (standard). rombar=0
> > > + option or loaded automatically through vfio (standard). rombar=off
> > > will disable legacy mode support.
> > > * Hotplug of the IGD device is not supported.
> > > * The IGD device must be a SandyBridge or newer model device.
> >
> > ...
> >
> > > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > > index 39dae72497e0..0e920ed0691a 100644
> > > --- a/hw/vfio/pci-quirks.c
> > > +++ b/hw/vfio/pci-quirks.c
> > > @@ -33,7 +33,7 @@
> > > * execution as noticed with the BCM 57810 card for lack of a
> > > * more better way to handle such issues.
> > > * The user can still override by specifying a romfile or
> > > - * rombar=1.
> > > + * rombar=on.
> > > * Please see https://bugs.launchpad.net/qemu/+bug/1284874
> > > * for an analysis of the 57810 card hang. When adding
> > > * a new vendor id/device id combination below, please also add
> >
> >
> > So we are apparently breaking a bunch of users who followed
> > documentation to the dot. Why is this a good idea?
>
> On/off is clearer than 1/0. But isn't 1/0 a synonym for on/off so previous
> command lines would still work?
>
> Regards,
> BALATON Zoltan
I see nothing in code that would make it so:
const QEnumLookup OnOffAuto_lookup = {
.array = (const char *const[]) {
[ON_OFF_AUTO_AUTO] = "auto",
[ON_OFF_AUTO_ON] = "on",
[ON_OFF_AUTO_OFF] = "off",
},
.size = ON_OFF_AUTO__MAX
};
I also tried with an existing property:
$ ./qemu-system-x86_64 -device intel-hda,msi=0
qemu-system-x86_64: -device intel-hda,msi=0: Parameter 'msi' does not accept value '0'
--
MST
On Wed, 3 Jul 2024, Michael S. Tsirkin wrote:
> On Wed, Jul 03, 2024 at 04:15:23AM +0200, BALATON Zoltan wrote:
>> On Tue, 2 Jul 2024, Michael S. Tsirkin wrote:
>>> On Thu, Jun 27, 2024 at 03:08:00PM +0900, Akihiko Odaki wrote:
>>>> rom_bar is tristate but was defined as uint32_t so convert it into
>>>> OnOffAuto.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>
>>> Commit log should explain why this is an improvement,
>>> not just what's done.
>>>
>>>
>>>> diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt
>>>> index e17bb50789ad..35c6c8e28493 100644
>>>> --- a/docs/igd-assign.txt
>>>> +++ b/docs/igd-assign.txt
>>>> @@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci:
>>>> ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at
>>>> PCI address 1f.0.
>>>> * The IGD device must have a VGA ROM, either provided via the romfile
>>>> - option or loaded automatically through vfio (standard). rombar=0
>>>> + option or loaded automatically through vfio (standard). rombar=off
>>>> will disable legacy mode support.
>>>> * Hotplug of the IGD device is not supported.
>>>> * The IGD device must be a SandyBridge or newer model device.
>>>
>>> ...
>>>
>>>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
>>>> index 39dae72497e0..0e920ed0691a 100644
>>>> --- a/hw/vfio/pci-quirks.c
>>>> +++ b/hw/vfio/pci-quirks.c
>>>> @@ -33,7 +33,7 @@
>>>> * execution as noticed with the BCM 57810 card for lack of a
>>>> * more better way to handle such issues.
>>>> * The user can still override by specifying a romfile or
>>>> - * rombar=1.
>>>> + * rombar=on.
>>>> * Please see https://bugs.launchpad.net/qemu/+bug/1284874
>>>> * for an analysis of the 57810 card hang. When adding
>>>> * a new vendor id/device id combination below, please also add
>>>
>>>
>>> So we are apparently breaking a bunch of users who followed
>>> documentation to the dot. Why is this a good idea?
>>
>> On/off is clearer than 1/0. But isn't 1/0 a synonym for on/off so previous
>> command lines would still work?
>>
>> Regards,
>> BALATON Zoltan
>
> I see nothing in code that would make it so:
>
>
> const QEnumLookup OnOffAuto_lookup = {
> .array = (const char *const[]) {
> [ON_OFF_AUTO_AUTO] = "auto",
> [ON_OFF_AUTO_ON] = "on",
> [ON_OFF_AUTO_OFF] = "off",
> },
> .size = ON_OFF_AUTO__MAX
> };
>
> I also tried with an existing property:
>
> $ ./qemu-system-x86_64 -device intel-hda,msi=0
> qemu-system-x86_64: -device intel-hda,msi=0: Parameter 'msi' does not accept value '0'
Then it was probably bit properties that also accept 0/1, on/off,
true/false. Maybe similar aliases could be added to on/off/auto?
In any case when I first saw rombar I thought it would set the BAR of the
ROM so wondered why it's 1 and not 5 or 6 or an offset. So on/off is
clearer in this case.
Regards,
BALATON Zoltan
On Wed, 3 Jul 2024 13:00:21 +0200 (CEST)
BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Wed, 3 Jul 2024, Michael S. Tsirkin wrote:
> > On Wed, Jul 03, 2024 at 04:15:23AM +0200, BALATON Zoltan wrote:
> >> On Tue, 2 Jul 2024, Michael S. Tsirkin wrote:
> >>> On Thu, Jun 27, 2024 at 03:08:00PM +0900, Akihiko Odaki wrote:
> >>>> rom_bar is tristate but was defined as uint32_t so convert it into
> >>>> OnOffAuto.
> >>>>
> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>
> >>> Commit log should explain why this is an improvement,
> >>> not just what's done.
> >>>
> >>>
> >>>> diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt
> >>>> index e17bb50789ad..35c6c8e28493 100644
> >>>> --- a/docs/igd-assign.txt
> >>>> +++ b/docs/igd-assign.txt
> >>>> @@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci:
> >>>> ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at
> >>>> PCI address 1f.0.
> >>>> * The IGD device must have a VGA ROM, either provided via the romfile
> >>>> - option or loaded automatically through vfio (standard). rombar=0
> >>>> + option or loaded automatically through vfio (standard). rombar=off
> >>>> will disable legacy mode support.
> >>>> * Hotplug of the IGD device is not supported.
> >>>> * The IGD device must be a SandyBridge or newer model device.
> >>>
> >>> ...
> >>>
> >>>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> >>>> index 39dae72497e0..0e920ed0691a 100644
> >>>> --- a/hw/vfio/pci-quirks.c
> >>>> +++ b/hw/vfio/pci-quirks.c
> >>>> @@ -33,7 +33,7 @@
> >>>> * execution as noticed with the BCM 57810 card for lack of a
> >>>> * more better way to handle such issues.
> >>>> * The user can still override by specifying a romfile or
> >>>> - * rombar=1.
> >>>> + * rombar=on.
> >>>> * Please see https://bugs.launchpad.net/qemu/+bug/1284874
> >>>> * for an analysis of the 57810 card hang. When adding
> >>>> * a new vendor id/device id combination below, please also add
> >>>
> >>>
> >>> So we are apparently breaking a bunch of users who followed
> >>> documentation to the dot. Why is this a good idea?
> >>
> >> On/off is clearer than 1/0. But isn't 1/0 a synonym for on/off so previous
> >> command lines would still work?
> >>
> >> Regards,
> >> BALATON Zoltan
> >
> > I see nothing in code that would make it so:
> >
> >
> > const QEnumLookup OnOffAuto_lookup = {
> > .array = (const char *const[]) {
> > [ON_OFF_AUTO_AUTO] = "auto",
> > [ON_OFF_AUTO_ON] = "on",
> > [ON_OFF_AUTO_OFF] = "off",
> > },
> > .size = ON_OFF_AUTO__MAX
> > };
> >
> > I also tried with an existing property:
> >
> > $ ./qemu-system-x86_64 -device intel-hda,msi=0
> > qemu-system-x86_64: -device intel-hda,msi=0: Parameter 'msi' does not accept value '0'
>
> Then it was probably bit properties that also accept 0/1, on/off,
> true/false. Maybe similar aliases could be added to on/off/auto?
>
> In any case when I first saw rombar I thought it would set the BAR of the
> ROM so wondered why it's 1 and not 5 or 6 or an offset. So on/off is
> clearer in this case.
There's only one PCI spec defined offset for the ROM BAR. Yes, the
option could be more clear but relocating the ROM to a different
regular BAR offset is invalid. Thanks,
Alex
On Wed, Jul 03, 2024 at 01:00:21PM +0200, BALATON Zoltan wrote:
> On Wed, 3 Jul 2024, Michael S. Tsirkin wrote:
> > On Wed, Jul 03, 2024 at 04:15:23AM +0200, BALATON Zoltan wrote:
> > > On Tue, 2 Jul 2024, Michael S. Tsirkin wrote:
> > > > On Thu, Jun 27, 2024 at 03:08:00PM +0900, Akihiko Odaki wrote:
> > > > > rom_bar is tristate but was defined as uint32_t so convert it into
> > > > > OnOffAuto.
> > > > >
> > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > >
> > > > Commit log should explain why this is an improvement,
> > > > not just what's done.
> > > >
> > > >
> > > > > diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt
> > > > > index e17bb50789ad..35c6c8e28493 100644
> > > > > --- a/docs/igd-assign.txt
> > > > > +++ b/docs/igd-assign.txt
> > > > > @@ -35,7 +35,7 @@ IGD has two different modes for assignment using vfio-pci:
> > > > > ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at
> > > > > PCI address 1f.0.
> > > > > * The IGD device must have a VGA ROM, either provided via the romfile
> > > > > - option or loaded automatically through vfio (standard). rombar=0
> > > > > + option or loaded automatically through vfio (standard). rombar=off
> > > > > will disable legacy mode support.
> > > > > * Hotplug of the IGD device is not supported.
> > > > > * The IGD device must be a SandyBridge or newer model device.
> > > >
> > > > ...
> > > >
> > > > > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > > > > index 39dae72497e0..0e920ed0691a 100644
> > > > > --- a/hw/vfio/pci-quirks.c
> > > > > +++ b/hw/vfio/pci-quirks.c
> > > > > @@ -33,7 +33,7 @@
> > > > > * execution as noticed with the BCM 57810 card for lack of a
> > > > > * more better way to handle such issues.
> > > > > * The user can still override by specifying a romfile or
> > > > > - * rombar=1.
> > > > > + * rombar=on.
> > > > > * Please see https://bugs.launchpad.net/qemu/+bug/1284874
> > > > > * for an analysis of the 57810 card hang. When adding
> > > > > * a new vendor id/device id combination below, please also add
> > > >
> > > >
> > > > So we are apparently breaking a bunch of users who followed
> > > > documentation to the dot. Why is this a good idea?
> > >
> > > On/off is clearer than 1/0. But isn't 1/0 a synonym for on/off so previous
> > > command lines would still work?
> > >
> > > Regards,
> > > BALATON Zoltan
> >
> > I see nothing in code that would make it so:
> >
> >
> > const QEnumLookup OnOffAuto_lookup = {
> > .array = (const char *const[]) {
> > [ON_OFF_AUTO_AUTO] = "auto",
> > [ON_OFF_AUTO_ON] = "on",
> > [ON_OFF_AUTO_OFF] = "off",
> > },
> > .size = ON_OFF_AUTO__MAX
> > };
> >
> > I also tried with an existing property:
> >
> > $ ./qemu-system-x86_64 -device intel-hda,msi=0
> > qemu-system-x86_64: -device intel-hda,msi=0: Parameter 'msi' does not accept value '0'
>
> Then it was probably bit properties that also accept 0/1, on/off,
> true/false.
I mean, the code is open, why do you keep guessing?
No, these reuse the bool parsing logic:
static void prop_get_bit(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
Property *prop = opaque;
uint32_t *p = object_field_prop_ptr(obj, prop);
bool value = (*p & qdev_get_prop_mask(prop)) != 0;
visit_type_bool(v, name, &value, errp);
}
and that never accepted 0 or 1:
bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error **errp)
{
if (g_str_equal(value, "on") ||
g_str_equal(value, "yes") ||
g_str_equal(value, "true") ||
g_str_equal(value, "y")) {
*obj = true;
return true;
}
if (g_str_equal(value, "off") ||
g_str_equal(value, "no") ||
g_str_equal(value, "false") ||
g_str_equal(value, "n")) {
*obj = false;
return true;
}
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
"'on' or 'off'");
return false;
}
> Maybe similar aliases could be added to on/off/auto?
Could be, but even then switching to that would mean that user sets 1
but query returns "on". Might or might not surprise some users.
Adding true/false yes/no y/n aliases to on/off/auto might make sense
though, for consistency. Donnu if QAPI guys will agree, though,
and not directly related to this patchset.
One other idea is to add a generic way to detect that a property is set
by user. This requirement comes up, once in a while.
> In any case when I first saw rombar I thought it would set the BAR of the
> ROM so wondered why it's 1 and not 5 or 6 or an offset. So on/off is clearer
> in this case.
>
> Regards,
> BALATON Zoltan
I agree here, but it's been here for a long time.
--
MST
© 2016 - 2025 Red Hat, Inc.