Re: [PULL 0/3] Update meson version

[Paolo Bonzini]

On 22/09/21 20:22, Peter Maydell wrote:

I had another go at this. As far as I can tell it causes all
the vm-build-{openbsd,netbsd,freebsd} build-and-tests to hang.
At any rate the VMs are sat there eating host CPU.
Does 'make vm-build-freebsd' etc work for you ?


It has been hanging for some time but I hadn't debugged it until now. 
It's caused by a missing dependency that causes bios-tables-test to fail 
(badly).  It's plausible that the meson upgrade triggered a different 
build ordering for make/ninja, and made it hang for you as well.


There was also a related bug that caused the test to hang if bzip2 was 
not available.


Paolo

[PATCH v5 17/20] nubus-bridge: make slot_available_mask a qdev property

[Mark Cave-Ayland]

This is to allow Macintosh machines to further specify which slots are available
since the number of addressable slots may not match the number of physical slots
present in the machine.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bridge.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 1adda7f5a6..2c7c4ee121 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -21,11 +21,18 @@ static void nubus_bridge_init(Object *obj)
 qbus_create_inplace(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL);
 }
 
+static Property nubus_bridge_properties[] = {
+DEFINE_PROP_UINT32("slot-available-mask", NubusBridge,
+   bus.slot_available_mask, 0x),
+DEFINE_PROP_END_OF_LIST()
+};
+
 static void nubus_bridge_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->fw_name = "nubus";
+device_class_set_props(dc, nubus_bridge_properties);
 }
 
 static const TypeInfo nubus_bridge_info = {
-- 
2.20.1

[PATCH v5 19/20] q800: wire up nubus IRQs

[Mark Cave-Ayland]

Nubus IRQs are routed to the CPU through the VIA2 device so wire up the IRQs
using gpios accordingly.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/m68k/q800.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 074acf4fdc..5bc9df58a0 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -398,6 +398,12 @@ static void q800_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
 MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
+for (i = 0; i < VIA2_NUBUS_IRQ_NB; i++) {
+qdev_connect_gpio_out(dev, 9 + i,
+  qdev_get_gpio_in_named(via2_dev, "nubus-irq",
+ VIA2_NUBUS_IRQ_9 + i));
+}
+
 nubus = _BRIDGE(dev)->bus;
 
 /* framebuffer in nubus slot #9 */
-- 
2.20.1

[PATCH v5 20/20] q800: configure nubus available slots for Quadra 800

[Mark Cave-Ayland]

Slot 0x9 is reserved for use by the in-built framebuffer whilst only slots
0xc, 0xd and 0xe physically exist on the Quadra 800.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/m68k/q800.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 5bc9df58a0..09b3366024 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -78,6 +78,13 @@
 
 #define MAC_CLOCK  3686418
 
+/*
+ * Slot 0x9 is reserved for use by the in-built framebuffer whilst only
+ * slots 0xc, 0xd and 0xe physically exist on the Quadra 800
+ */
+#define Q800_NUBUS_SLOTS_AVAILABLE(BIT(0x9) | BIT(0xc) | BIT(0xd) | \
+   BIT(0xe))
+
 /*
  * The GLUE (General Logic Unit) is an Apple custom integrated circuit chip
  * that performs a variety of functions (RAM management, clock generation, 
...).
@@ -392,6 +399,8 @@ static void q800_init(MachineState *machine)
 /* NuBus */
 
 dev = qdev_new(TYPE_MAC_NUBUS_BRIDGE);
+qdev_prop_set_uint32(dev, "slot-available-mask",
+ Q800_NUBUS_SLOTS_AVAILABLE);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
 MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE);
-- 
2.20.1

Re: [PATCH] docs/nvdimm: Update nvdimm option value in machine example

[Laurent Vivier]

Le 23/09/2021 à 11:47, Pankaj Gupta a écrit :
> ping

Could you post your patch with an email address ("From") that is the same as 
the "Signed-off-by"?

https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line

Thanks,
Laurent

>> Update nvdimm option value in example command from "-machine pc,nvdimm"
>> to "-machine pc,nvdimm=on" as former complains with the below error:
>>
>> "qemu-system-x86_64: -machine pc,nvdimm: Expected '=' after parameter 
>> 'nvdimm'"
>>
>> Signed-off-by: Pankaj Gupta 
>> ---
>>  docs/nvdimm.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
>> index 0aae682be3..fd7773dc5a 100644
>> --- a/docs/nvdimm.txt
>> +++ b/docs/nvdimm.txt
>> @@ -15,7 +15,7 @@ backend (i.e. memory-backend-file and memory-backend-ram). 
>> A simple
>>  way to create a vNVDIMM device at startup time is done via the
>>  following command line options:
>>
>> - -machine pc,nvdimm
>> + -machine pc,nvdimm=on
>>   -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
>>   -object 
>> memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
>>   -device nvdimm,id=nvdimm1,memdev=mem1,unarmed=off
>> --
>> 2.25.1
>>
> 

Re: [PATCH] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Peter Maydell]

On Thu, 23 Sept 2021 at 11:32, Paolo Bonzini  wrote:
>
> On 23/09/21 12:00, Philippe Mathieu-Daudé wrote:
> > See also "blobs: Only install required (system emulation) files":
> > https://lore.kernel.org/qemu-devel/20210323155132.238193-1-f4...@amsat.org/
>
> Nice and makes a lot of sense.  Regarding "there is probably a nicer way
> to do this with Meson", I would do without all the variables and do
> something like
>
> foreach target : target_dirs
>if target in ['...']
>  blobs_ss.add('...')
>elif target in ['...']
>  blobs_ss.add('...')
>endif
> endforeach
>
> directly in pc-bios/meson.build.

Is it possible also to have meson handle the "symlink the blob from
the source dir to the build dir" which currently configure is doing ?
That would mean we could avoid having effectively two lists of blobs.
(Somebody would need to cross-check that all the blobs the wildcards in
configure are linking in are listed in the meson list.) I guess
the question is whether other parts of the build system assume those
links already exist (ie they don't explicitly declare a dependency
on the existence of the blob and would need to change to do so).

thanks
-- PMM

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[Kevin Wolf]

Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> Add a warning for when 'iotests' runs against a qemu namespace that
> isn't the one in the source tree. This might occur if you have
> (accidentally) installed the Python namespace package to your local
> packages.
> 
> (I'm not going to say that this is because I bit myself with this,
> but. You can fill in the blanks.)
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/testenv.py | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 88104dace90..8a43b193af5 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -16,6 +16,8 @@
>  # along with this program.  If not, see .
>  #
>  
> +import importlib.util
> +import logging
>  import os
>  import sys
>  import tempfile
> @@ -25,7 +27,7 @@
>  import random
>  import subprocess
>  import glob
> -from typing import List, Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional, ContextManager, cast
>  
>  DEF_GDB_OPTIONS = 'localhost:12345'
>  
> @@ -112,6 +114,22 @@ def init_directories(self) -> None:
>  # Path where qemu goodies live in this source tree.
>  qemu_srctree_path = Path(__file__, '../../../python').resolve()
>  
> +# warn if we happen to be able to find 'qemu' packages from an
> +# unexpected location (i.e. the package is already installed in
> +# the user's environment)
> +qemu_spec = importlib.util.find_spec('qemu.qmp')
> +if qemu_spec:
> +spec_path = Path(cast(str, qemu_spec.origin))

You're casting Optional[str] to str here. The source type is not obvious
from the local code, so unless you spend some time actively figuring it
out, this could be casting anything to str. But even knowing this, just
casting Optional away looks fishy anyway.

Looking up the ModuleSpec docs, it seems okay to assume that it's never
None in our case, but wouldn't it be much cleaner to just 'assert
qemu_spec.origin' first and then use it without the cast?

> +try:
> +_ = spec_path.relative_to(qemu_srctree_path)
> +except ValueError:
> +self._logger.warning(
> +"WARNING: 'qemu' package will be imported from outside "
> +"the source tree!")
> +self._logger.warning(
> +"Importing from: '%s'",
> +spec_path.parents[1])
> +
>  self.pythonpath = os.getenv('PYTHONPATH')
>  self.pythonpath = os.pathsep.join(filter(None, (
>  self.source_iotests,
> @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: 
> str,
>  
>  self.build_root = os.path.join(self.build_iotests, '..', '..')
>  
> +self._logger = logging.getLogger('qemu.iotests')
>  self.init_directories()
>  self.init_binaries()

Looks good otherwise.

Kevin

Re: [PULL 18/28] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

[Paolo Bonzini]

On 22/09/21 21:51, Halil Pasic wrote:

We have figured out what is going on here. The problem seems to be
specific to linux aio, which seems to limit the size of the iovec to
1024 (UIO_MAXIOV).


Hi Halil,

I'll send a patch shortly to fix this issue.  Sorry about the delay as I 
was busy with KVM Forum and all that. :(


Paolo

[PATCH v2 1/2] meson: unpack edk2 firmware even if --disable-blobs

[Paolo Bonzini]

The edk2 firmware blobs are needed to run bios-tables-test.  Unpack
them if any UEFI-enabled target is selected, so that the test can run.
This is a bit more than is actually necessary, since bios-tables-test
does not run for all UEFI-enabled targets, but it is the easiest
way to write this logic.

Signed-off-by: Paolo Bonzini 
---
 meson.build | 16 
 pc-bios/descriptors/meson.build |  4 ++--
 pc-bios/meson.build |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/meson.build b/meson.build
index 3945a6cc2d..4cd8b67ad0 100644
--- a/meson.build
+++ b/meson.build
@@ -95,15 +95,15 @@ if targetos != 'darwin'
 endif
 
 edk2_targets = [ 'arm-softmmu', 'aarch64-softmmu', 'i386-softmmu', 
'x86_64-softmmu' ]
-install_edk2_blobs = false
-if get_option('install_blobs')
-  foreach target : target_dirs
-install_edk2_blobs = install_edk2_blobs or target in edk2_targets
-  endforeach
-endif
+unpack_edk2_blobs = false
+foreach target : edk2_targets
+  if target in target_dirs
+bzip2 = find_program('bzip2', required: get_option('install_blobs'))
+unpack_edk2_blobs = bzip2.found()
+break
+  endif
+endforeach
 
-bzip2 = find_program('bzip2', required: install_edk2_blobs)
-
 ##
 # Compiler flags #
 ##
diff --git a/pc-bios/descriptors/meson.build b/pc-bios/descriptors/meson.build
index 29efa16d99..66f85d01c4 100644
--- a/pc-bios/descriptors/meson.build
+++ b/pc-bios/descriptors/meson.build
@@ -1,4 +1,4 @@
-if install_edk2_blobs
+if unpack_edk2_blobs and get_option('install_blobs')
   foreach f: [
 '50-edk2-i386-secure.json',
 '50-edk2-x86_64-secure.json',
@@ -10,7 +10,7 @@ if install_edk2_blobs
 configure_file(input: files(f),
output: f,
configuration: {'DATADIR': get_option('prefix') / 
qemu_datadir},
-   install: get_option('install_blobs'),
+   install: true,
install_dir: qemu_datadir / 'firmware')
   endforeach
 endif
diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index f2b32598af..a3b3d87891 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -1,4 +1,4 @@
-if install_edk2_blobs
+if unpack_edk2_blobs
   fds = [
 'edk2-aarch64-code.fd',
 'edk2-arm-code.fd',
-- 
2.27.0

Re: [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready

[Ani Sinha]

On Thu, 23 Sep 2021, Ani Sinha wrote:

>
>
> On Wed, 22 Sep 2021, Damien Hedde wrote:
>
> > Skip the sysbus device type per-machine allow-list check before the
> > MACHINE_INIT_PHASE_READY phase.
> >
> > This patch permits adding any sysbus device (it still needs to be
> > user_creatable) when using the -preconfig experimental option.
> >
> > Signed-off-by: Damien Hedde 
> > ---
> >
> > This commit is RFC. Depending on the condition to allow a device
> > to be added, it may change.
> > ---
> >  softmmu/qdev-monitor.c | 9 +++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index f1c9242855..73b991adda 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char 
> > **driver, Error **errp)
> >  return NULL;
> >  }
> >
> > -if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
> > -/* sysbus devices need to be allowed by the machine */
> > +if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) &&
> > +phase_check(MACHINE_INIT_PHASE_READY)) {
> > +/*
> > + * Sysbus devices need to be allowed by the machine.
> > + * We only check that after the machine is ready in order to let
> > + * us add any user_creatable sysbus device during machine creation.
> > + */
> >  MachineClass *mc = 
> > MACHINE_CLASS(object_get_class(qdev_get_machine()));
> >  if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
> >  error_setg(errp, "'%s' is not an allowed pluggable sysbus 
> > device "
>
> Since now you are adding the state of the machine creation in the
> valiation condition, the failure error message becomes misleading.
> Better to do this I think :
>
> if (object class is TYPE_SYS_BUS_DEVICE)
> {
>   if (!phase_check(MACHINE_INIT_PHASE_READY))
> {
>   // error out here saying the state of the machine creation is too
> early
> }
>
> // do the other check on dynamic sysbus
>
> }

The other thing to consider is whether we should put the machine phaze
check at a higher level, at qdev_device_add() perhaps. One might envison
that different device types may be allowed to be added at different
stages of machine creation. So this check needs be more generalized for
the future.

Re: [PATCH v3 0/3] tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges for q35

[Ani Sinha]

ping ...

On Mon, 20 Sep 2021, Ani Sinha wrote:

> This patchset adds a unit test to exercize acpi hotplug support for 
> multifunction
> bridges on q35 machines. This support was added with the commit:
>
> d7346e614f4ec ("acpi: x86: pcihp: add support hotplug on multifunction 
> bridges")
>
> changelist:
> v1 : initial RFC patch.
> v2: incorporated some of the feedbacks from Igor.
> v3: forgot to add the ASL diff for patch 3 in commit log for v2. Added now.
>
> Ani Sinha (3):
>   tests/acpi/bios-tables-test: add and allow changes to a new q35 DSDT
> table blob
>   tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges
> for q35
>   tests/acpi/bios-tables-test: update DSDT blob for multifunction bridge
> test
>
>  tests/data/acpi/q35/DSDT.multi-bridge | Bin 0 -> 8435 bytes
>  tests/qtest/bios-tables-test.c|  18 ++
>  2 files changed, 18 insertions(+)
>  create mode 100644 tests/data/acpi/q35/DSDT.multi-bridge
>
> --
> 2.25.1
>
>

Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * David Hildenbrand (da...@redhat.com) wrote:
>> We want to rate-limit MEMORY_DEVICE_SIZE_CHANGE events per device,
>> otherwise we can lose some events for devices. As we might not always have
>> a device id, add the qom-path to the event and use that to rate-limit
>> per device.
>> 
>> This was noticed by starting a VM with two virtio-mem devices that each
>> have a requested size > 0. The Linux guest will initialize both devices
>> in parallel, resulting in losing MEMORY_DEVICE_SIZE_CHANGE events for
>> one of the devices.
>> 
>> Fixes: 722a3c783ef4 ("virtio-pci: Send qapi events when the virtio-mem size 
>> changes")
>> Suggested-by: Markus Armbruster 
>> Cc: "Dr. David Alan Gilbert" 
>> Cc: Markus Armbruster 
>> Cc: Michael S. Tsirkin 
>> Cc: Eric Blake 
>> Cc: Igor Mammedov 
>> Cc: Michal Privoznik 
>> Signed-off-by: David Hildenbrand 
>> ---
>> 
>> Follow up of:
>> https://lkml.kernel.org/r/20210921102434.24273-1-da...@redhat.com
>> 
>> v1 -> v2:
>> - Add the qom-path and use that identifier to rate-limit per device
>> - Rephrase subject/description
>> 
>> ---
>>  hw/virtio/virtio-mem-pci.c | 3 ++-
>>  monitor/monitor.c  | 9 +
>>  qapi/machine.json  | 5 -
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
>> index fa5395cd88..dd5085497f 100644
>> --- a/hw/virtio/virtio-mem-pci.c
>> +++ b/hw/virtio/virtio-mem-pci.c
>> @@ -87,6 +87,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
>> *notifier, void *data)
>>  VirtIOMEMPCI *pci_mem = container_of(notifier, VirtIOMEMPCI,
>>   size_change_notifier);
>>  DeviceState *dev = DEVICE(pci_mem);
>> +const char * qom_path = object_get_canonical_path(OBJECT(dev));
>>  const uint64_t * const size_p = data;
>>  const char *id = NULL;
>>  
>> @@ -94,7 +95,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
>> *notifier, void *data)
>>  id = g_strdup(dev->id);
>>  }
>>  
>> -qapi_event_send_memory_device_size_change(!!id, id, *size_p);
>> +qapi_event_send_memory_device_size_change(!!id, id, *size_p, qom_path);
>>  }
>>  
>>  static void virtio_mem_pci_class_init(ObjectClass *klass, void *data)
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 46a171bca6..21c7a68758 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -474,6 +474,10 @@ static unsigned int qapi_event_throttle_hash(const void 
>> *key)
>>  hash += g_str_hash(qdict_get_str(evstate->data, "node-name"));
>>  }
>>  
>> +if (evstate->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {
>> +hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
>> +}
>
> It makes me wonder if all these entries could turn into:
>   str = qdict_get_try_str(qdict, "qom-path");
>   if (str) {
>   hash += g_str_hash(str);
>   }
>
> and then stop worrying about checking each eventtype there?

Prone to accidental capture when we add to event data later on.  I feel
it's better to be explicit.

Re: [PATCH v5 05/20] nubus: move slot bitmap checks from NubusDevice realize() to BusClass check_address()

[Laurent Vivier]

Le 23/09/2021 à 11:12, Mark Cave-Ayland a écrit :
> Allow Nubus to manage the slot allocations itself using the BusClass 
> check_address()
> virtual function rather than managing this during NubusDevice realize().
> 
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Laurent Vivier 
> ---
>  hw/nubus/nubus-bus.c| 30 ++
>  hw/nubus/nubus-device.c | 22 --
>  2 files changed, 30 insertions(+), 22 deletions(-)
> 
>

Reviewed-by: Laurent Vivier 

Re: [PATCH] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Paolo Bonzini]

On 23/09/21 10:22, Daniel P. Berrangé wrote:

On Thu, Sep 23, 2021 at 04:15:55AM -0400, Paolo Bonzini wrote:

Skip the test if bzip2 is not available, and run it after they are
uncompressed.

Signed-off-by: Paolo Bonzini 
---
  pc-bios/meson.build | 3 ++-
  tests/qtest/meson.build | 6 +++---
  2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index f2b32598af..975565198e 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -10,8 +10,9 @@ if install_edk2_blobs
  'edk2-x86_64-secure-code.fd',
]
  
+  roms = []

foreach f : fds
-custom_target(f,
+roms += custom_target(f,
build_by_default: have_system,
output: f,
input: '@0@.bz2'.format(f),
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e1f4df3df8..6d8100c9de 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -68,12 +68,12 @@ qtests_i386 = \
(config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) 
+  \
(config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? 
['fuzz-e1000e-test'] : []) +   \
(config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +   
  \
+  (install_edk2_blobs ? ['bios-tables-test'] : []) +   
 \
qtests_pci +
  \
['fdc-test',
 'ide-test',
 'hd-geo-test',
 'boot-order-test',
-   'bios-tables-test',
 'rtc-test',
 'i440fx-test',
 'fw_cfg-test',
@@ -180,7 +180,7 @@ qtests_arm = \
  
  # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test unconditional

  qtests_aarch64 = \
-  (cpu != 'arm' ? ['bios-tables-test'] : []) + 
 \
+  (cpu != 'arm' and install_edk2_blobs ? ['bios-tables-test'] : []) +  
 \
(config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-test'] : []) +\
(config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-swtpm-test'] : []) +  \
['arm-cpu-features',
@@ -269,7 +269,7 @@ foreach dir : target_dirs
qtest_emulator = emulators['qemu-system-' + target_base]
target_qtests = get_variable('qtests_' + target_base, []) + qtests_generic
  
-  test_deps = []

+  test_deps = roms


Shouldn't this be

   if install_edk2_blobs
  test_deps += roms
   endif


That, or (better) move the "roms = []" initializer outside the "if 
install_edk2_blobs".


Also, right now bios-tables-test hangs (before the patch) or is skipped 
(after) if --disable-blobs is used on the configure command line.  We 
can do the unpack in that case and skip the installation.  This is not 
really necessary to fix the issues that Peter saw in vm-build-freebsd, 
but it does not hurt either.


Paolo

Re: [PATCH] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Paolo Bonzini]

On 23/09/21 12:00, Philippe Mathieu-Daudé wrote:

See also "blobs: Only install required (system emulation) files":
https://lore.kernel.org/qemu-devel/20210323155132.238193-1-f4...@amsat.org/


Nice and makes a lot of sense.  Regarding "there is probably a nicer way 
to do this with Meson", I would do without all the variables and do 
something like


foreach target : target_dirs
  if target in ['...']
blobs_ss.add('...')
  elif target in ['...']
blobs_ss.add('...')
  endif
endforeach

directly in pc-bios/meson.build.

Paolo

Re: [PATCH v5 16/20] nubus-bridge: embed the NubusBus object directly within nubus-bridge

[Laurent Vivier]

Le 23/09/2021 à 11:13, Mark Cave-Ayland a écrit :
> Since nubus-bridge is a container for NubusBus then it should be embedded
> directly within the bridge device using qbus_create_inplace().
> 
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/m68k/q800.c  | 2 +-
>  hw/nubus/mac-nubus-bridge.c | 9 +
>  hw/nubus/nubus-bridge.c | 3 ++-
>  include/hw/nubus/nubus.h| 2 +-
>  4 files changed, 9 insertions(+), 7 deletions(-)
> 


Reviewed-by: Laurent Vivier 

[PATCH v2 0/2] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Paolo Bonzini]

Compared to v1, this adds another patch in front, so that tests are not
disabled unnecessarily by --disable-blobs.

Paolo Bonzini (2):
  meson: unpack edk2 firmware even if --disable-blobs
  tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

 meson.build | 14 +++---
 pc-bios/descriptors/meson.build |  4 ++--
 pc-bios/meson.build |  5 +++--
 tests/qtest/meson.build |  6 +++---
 4 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.27.0

>From ae2af96bfb54cdef880b5bf3eb651574dba1c5d9 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini 
Date: Thu, 23 Sep 2021 06:25:23 -0400
Subject: [PATCH v2 1/2] meson: unpack edk2 firmware even if --disable-blobs

The edk2 firmware blobs are needed to run bios-tables-test.  Unpack
them if any UEFI-enabled target is selected, so that the test can run.
This is a bit more than is actually necessary, since bios-tables-test
does not run for all UEFI-enabled targets, but it is the easiest
way to write this logic.

Signed-off-by: Paolo Bonzini 
---
 meson.build | 16 
 pc-bios/descriptors/meson.build |  4 ++--
 pc-bios/meson.build |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/meson.build b/meson.build
index 3945a6cc2d..4cd8b67ad0 100644
--- a/meson.build
+++ b/meson.build
@@ -95,15 +95,15 @@ if targetos != 'darwin'
 endif
 
 edk2_targets = [ 'arm-softmmu', 'aarch64-softmmu', 'i386-softmmu', 
'x86_64-softmmu' ]
-install_edk2_blobs = false
-if get_option('install_blobs')
-  foreach target : target_dirs
-install_edk2_blobs = install_edk2_blobs or target in edk2_targets
-  endforeach
-endif
+unpack_edk2_blobs = false
+foreach target : edk2_targets
+  if target in target_dirs
+bzip2 = find_program('bzip2', required: get_option('install_blobs'))
+unpack_edk2_blobs = bzip2.found()
+break
+  endif
+endforeach
 
-bzip2 = find_program('bzip2', required: install_edk2_blobs)
-
 ##
 # Compiler flags #
 ##
diff --git a/pc-bios/descriptors/meson.build b/pc-bios/descriptors/meson.build
index 29efa16d99..66f85d01c4 100644
--- a/pc-bios/descriptors/meson.build
+++ b/pc-bios/descriptors/meson.build
@@ -1,4 +1,4 @@
-if install_edk2_blobs
+if unpack_edk2_blobs and get_option('install_blobs')
   foreach f: [
 '50-edk2-i386-secure.json',
 '50-edk2-x86_64-secure.json',
@@ -10,7 +10,7 @@ if install_edk2_blobs
 configure_file(input: files(f),
output: f,
configuration: {'DATADIR': get_option('prefix') / 
qemu_datadir},
-   install: get_option('install_blobs'),
+   install: true,
install_dir: qemu_datadir / 'firmware')
   endforeach
 endif
diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index f2b32598af..a3b3d87891 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -1,4 +1,4 @@
-if install_edk2_blobs
+if unpack_edk2_blobs
   fds = [
 'edk2-aarch64-code.fd',
 'edk2-arm-code.fd',
-- 
2.27.0


>From 18a1be0cea50d3321612da484335842ac9961d6b Mon Sep 17 00:00:00 2001
From: Paolo Bonzini 
Date: Thu, 23 Sep 2021 04:09:08 -0400
Subject: [PATCH v2 2/2] tests: qtest: bios-tables-test depends on the unpacked
 edk2 ROMs

Skip the test if bzip2 is not available, and run it after they are
uncompressed.

Signed-off-by: Paolo Bonzini 
---
 pc-bios/meson.build | 3 ++-
 tests/qtest/meson.build | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index a3b3d87891..a44c9bc127 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -1,3 +1,4 @@
+roms = []
 if unpack_edk2_blobs
   fds = [
 'edk2-aarch64-code.fd',
@@ -11,7 +12,7 @@ if unpack_edk2_blobs
   ]
 
   foreach f : fds
-custom_target(f,
+roms += custom_target(f,
   build_by_default: have_system,
   output: f,
   input: '@0@.bz2'.format(f),
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e1f4df3df8..c9d8458062 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -68,12 +68,12 @@ qtests_i386 = \
   (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + 
 \
   (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? 
['fuzz-e1000e-test'] : []) +   \
   (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +
 \
+  (unpack_edk2_blobs ? ['bios-tables-test'] : []) +
 \
   qtests_pci + 
 \
   ['fdc-test',
'ide-test',
'hd-geo-test',
'boot-order-test',
-   'bios-tables-test',
'rtc-test',
'i440fx-test',
'fw_cfg-test',
@@ -180,7 +180,7 @@ qtests_arm = \
 
 # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test 
unconditional
 qtests_aarch64 = \
-  (cpu != 'arm' ? 

[PATCH] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Paolo Bonzini]

Skip the test if bzip2 is not available, and run it after they are
uncompressed.

Signed-off-by: Paolo Bonzini 
---
 pc-bios/meson.build | 3 ++-
 tests/qtest/meson.build | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index f2b32598af..975565198e 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -10,8 +10,9 @@ if install_edk2_blobs
 'edk2-x86_64-secure-code.fd',
   ]
 
+  roms = []
   foreach f : fds
-custom_target(f,
+roms += custom_target(f,
   build_by_default: have_system,
   output: f,
   input: '@0@.bz2'.format(f),
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e1f4df3df8..6d8100c9de 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -68,12 +68,12 @@ qtests_i386 = \
   (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + 
 \
   (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? 
['fuzz-e1000e-test'] : []) +   \
   (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +
 \
+  (install_edk2_blobs ? ['bios-tables-test'] : []) +   
 \
   qtests_pci + 
 \
   ['fdc-test',
'ide-test',
'hd-geo-test',
'boot-order-test',
-   'bios-tables-test',
'rtc-test',
'i440fx-test',
'fw_cfg-test',
@@ -180,7 +180,7 @@ qtests_arm = \
 
 # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test 
unconditional
 qtests_aarch64 = \
-  (cpu != 'arm' ? ['bios-tables-test'] : []) + 
 \
+  (cpu != 'arm' and install_edk2_blobs ? ['bios-tables-test'] : []) +  
 \
   (config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-test'] : []) +\
   (config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-swtpm-test'] : []) +  \
   ['arm-cpu-features',
@@ -269,7 +269,7 @@ foreach dir : target_dirs
   qtest_emulator = emulators['qemu-system-' + target_base]
   target_qtests = get_variable('qtests_' + target_base, []) + qtests_generic
 
-  test_deps = []
+  test_deps = roms
   qtest_env = environment()
   if have_tools
 qtest_env.set('QTEST_QEMU_IMG', './qemu-img')
-- 
2.27.0

Re: [PATCH v5 18/20] nubus: add support for slot IRQs

[Philippe Mathieu-Daudé]

On 9/23/21 11:13, Mark Cave-Ayland wrote:

Each Nubus slot has an IRQ line that can be used to request service from the
CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using 
qdev
gpios accordingly, and introduce a new nubus_set_irq() function that can be used
by Nubus devices to control the slot IRQ.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bridge.c  | 2 ++
  hw/nubus/nubus-device.c  | 8 
  include/hw/nubus/nubus.h | 6 ++
  3 files changed, 16 insertions(+)



  static Property nubus_bridge_properties[] = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 280f40e88a..0f1852f671 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -10,12 +10,20 @@
  
  #include "qemu/osdep.h"

  #include "qemu/datadir.h"
+#include "hw/irq.h"
  #include "hw/loader.h"
  #include "hw/nubus/nubus.h"
  #include "qapi/error.h"
  #include "qemu/error-report.h"
  
  
+void nubus_set_irq(NubusDevice *nd, int level)

+{
+NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd)));
+


A trace-event could be helpful here, otherwise:

Reviewed-by: Philippe Mathieu-Daudé 


+qemu_set_irq(nubus->irqs[nd->slot], level);
+}

[PATCH v2] docs/nvdimm: Update nvdimm option value in machine example

[Pankaj Gupta]

Update nvdimm option value in example command from "-machine pc,nvdimm"
to "-machine pc,nvdimm=on" as former complains with the below error:

"qemu-system-x86_64: -machine pc,nvdimm: Expected '=' after parameter 'nvdimm'"

Reviewed-by: Laurent Vivier 
Signed-off-by: Pankaj Gupta 
---
 docs/nvdimm.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index 0aae682be3..fd7773dc5a 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -15,7 +15,7 @@ backend (i.e. memory-backend-file and memory-backend-ram). A 
simple
 way to create a vNVDIMM device at startup time is done via the
 following command line options:
 
- -machine pc,nvdimm
+ -machine pc,nvdimm=on
  -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
  -object 
memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
  -device nvdimm,id=nvdimm1,memdev=mem1,unarmed=off
-- 
2.25.1

Re: [PATCH v5 12/20] nubus: move nubus to its own 32-bit address space

[Laurent Vivier]

Le 23/09/2021 à 11:13, Mark Cave-Ayland a écrit :
> According to "Designing Cards and Drivers for the Macintosh Family" the Nubus
> has its own 32-bit address space based upon physical slot addressing.
> 
> Move Nubus to its own 32-bit address space and then use memory region aliases
> to map available slot and super slot ranges into the q800 system address
> space via the Macintosh Nubus bridge.
> 
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/m68k/q800.c  |  9 -
>  hw/nubus/mac-nubus-bridge.c | 16 ++--
>  hw/nubus/nubus-bus.c| 18 ++
>  include/hw/nubus/mac-nubus-bridge.h |  2 ++
>  include/hw/nubus/nubus.h|  6 ++
>  5 files changed, 44 insertions(+), 7 deletions(-)
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Peter Maydell]

On Thu, 23 Sept 2021 at 11:29, Philippe Mathieu-Daudé  wrote:
>
> On 9/23/21 07:16, Bin Meng wrote:> On Sun, Sep 19, 2021 at 2:07 AM
> Philippe Mathieu-Daudé  wrote:
> >> +static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
> >> +
> >> +qdev_prop_set_uint8(DEVICE(>serial_mm), "regshift", 2);
> >> +qdev_prop_set_uint32(DEVICE(>serial_mm), "baudbase", 399193);
> >> +qdev_prop_set_uint8(DEVICE(>serial_mm), "endianness",
> >> +DEVICE_LITTLE_ENDIAN);
> >
> > It looks like serial_mm_init() does one more thing:
> >
> >  qdev_set_legacy_instance_id(DEVICE(smm), base, 2);
> >
> > I am not sure what that is.
>
> I'll defer on Paolo / Marc-André for that part, I think this is for
> migrating legacy (x86?) machines, which is not the case.

Yes, this is only needed for backwards-compatibility of incoming
migration data with old versions of QEMU which implemented migration
of devices with hand-rolled code. If a device didn't previously
handle migration at all then it should not now be setting the
legacy instance ID.

Speaking of migration, I notice that this QOM conversion does
not add a vmstate, which usually we do as part of the QOM conversion.
The device is also missing a reset method.

thanks
-- PMM

Re: [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed

[Ani Sinha]

On Wed, 22 Sep 2021, Damien Hedde wrote:

> Right now the allowance check for adding a sysbus device using
> -device cli option (or device_add qmp command) is done well after
> the device has been created. It is done during the machine init done
> notifier: machine_init_notify() in hw/core/machine.c
>
> This new function will allow us to check if a sysbus device type is
> allowed to be dynamically created by the machine during the device
> creation time.
>
> Also make device_is_dynamic_sysbus() use the new function.
>
> Signed-off-by: Damien Hedde 
> ---
>
> In the context of our series, we need to be able to do the check at
> device creation time to allow doing it depending on the current
> MACHINE_INIT phase.
> ---
>  include/hw/boards.h | 17 +
>  hw/core/machine.c   | 15 ---
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 463a5514f9..934443c1cd 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -51,6 +51,23 @@ void machine_set_cpu_numa_node(MachineState *machine,
>   */
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
> *type);
>
> +/**
> + * machine_class_is_dynamic_sysbus_dev_allowed: Check if type is an allowed
> + * sysbus device type for the machine class.
> + * @mc: Machine class
> + * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
> + *
> + * Returns: true if @type is a type in the machine's list of
> + * dynamically pluggable sysbus devices; otherwise false.
> + *
> + * Check if the QOM type @type is in the list of allowed sysbus device
> + * types (see machine_class_allowed_dynamic_sysbus_dev()).
> + * Note that if @type is a subtype of a type which is in the list, it is
> + * allowed too.
> + */
> +bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
> + const char *type);
> +

How about renaming this to device_type_is_allowed_dynamic_sysbus() ?

>  /**
>   * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
>   * @mc: Machine class
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 9125c9aad0..1a18912dc8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -545,18 +545,27 @@ void 
> machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>
>  bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
>  {
> -bool allowed = false;
> -strList *wl;
>  Object *obj = OBJECT(dev);
>
>  if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
>  return false;
>  }
>
> +return machine_class_is_dynamic_sysbus_dev_allowed(mc,
> +object_get_typename(obj));
> +}
> +
> +bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
> + const char *type)
> +{
> +bool allowed = false;
> +strList *wl;
> +ObjectClass *klass = object_class_by_name(type);
> +
>  for (wl = mc->allowed_dynamic_sysbus_devices;
>   !allowed && wl;
>   wl = wl->next) {
> -allowed |= !!object_dynamic_cast(obj, wl->value);
> +allowed |= !!object_class_dynamic_cast(klass, wl->value);
>  }
>
>  return allowed;
> --
> 2.33.0
>
>

Re: [PATCH] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Philippe Mathieu-Daudé]

On 9/23/21 12:41, Peter Maydell wrote:

On Thu, 23 Sept 2021 at 11:29, Philippe Mathieu-Daudé  wrote:

On 9/23/21 07:16, Bin Meng wrote:> On Sun, Sep 19, 2021 at 2:07 AM
Philippe Mathieu-Daudé  wrote:

+static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
+{
+MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
+
+qdev_prop_set_uint8(DEVICE(>serial_mm), "regshift", 2);
+qdev_prop_set_uint32(DEVICE(>serial_mm), "baudbase", 399193);
+qdev_prop_set_uint8(DEVICE(>serial_mm), "endianness",
+DEVICE_LITTLE_ENDIAN);


It looks like serial_mm_init() does one more thing:

  qdev_set_legacy_instance_id(DEVICE(smm), base, 2);

I am not sure what that is.


I'll defer on Paolo / Marc-André for that part, I think this is for
migrating legacy (x86?) machines, which is not the case.


Yes, this is only needed for backwards-compatibility of incoming
migration data with old versions of QEMU which implemented migration
of devices with hand-rolled code. If a device didn't previously
handle migration at all then it should not now be setting the
legacy instance ID.


Thanks. I'll try to add that in the documentation somewhere.


Speaking of migration, I notice that this QOM conversion does
not add a vmstate, which usually we do as part of the QOM conversion.
The device is also missing a reset method.


OK, I'll add that in a previous patch.

Thanks,

Phil.

[PATCH v2 2/2] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Paolo Bonzini]

Skip the test if bzip2 is not available, and run it after they are
uncompressed.

Signed-off-by: Paolo Bonzini 
---
 pc-bios/meson.build | 3 ++-
 tests/qtest/meson.build | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index a3b3d87891..a44c9bc127 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -1,3 +1,4 @@
+roms = []
 if unpack_edk2_blobs
   fds = [
 'edk2-aarch64-code.fd',
@@ -11,7 +12,7 @@ if unpack_edk2_blobs
   ]
 
   foreach f : fds
-custom_target(f,
+roms += custom_target(f,
   build_by_default: have_system,
   output: f,
   input: '@0@.bz2'.format(f),
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e1f4df3df8..c9d8458062 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -68,12 +68,12 @@ qtests_i386 = \
   (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + 
 \
   (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? 
['fuzz-e1000e-test'] : []) +   \
   (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +
 \
+  (unpack_edk2_blobs ? ['bios-tables-test'] : []) +
 \
   qtests_pci + 
 \
   ['fdc-test',
'ide-test',
'hd-geo-test',
'boot-order-test',
-   'bios-tables-test',
'rtc-test',
'i440fx-test',
'fw_cfg-test',
@@ -180,7 +180,7 @@ qtests_arm = \
 
 # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test 
unconditional
 qtests_aarch64 = \
-  (cpu != 'arm' ? ['bios-tables-test'] : []) + 
 \
+  (cpu != 'arm' and unpack_edk2_blobs ? ['bios-tables-test'] : []) +   
 \
   (config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-test'] : []) +\
   (config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-swtpm-test'] : []) +  \
   ['arm-cpu-features',
@@ -269,7 +269,7 @@ foreach dir : target_dirs
   qtest_emulator = emulators['qemu-system-' + target_base]
   target_qtests = get_variable('qtests_' + target_base, []) + qtests_generic
 
-  test_deps = []
+  test_deps = roms
   qtest_env = environment()
   if have_tools
 qtest_env.set('QTEST_QEMU_IMG', './qemu-img')
-- 
2.27.0

Re: [PATCH 8/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

23.09.2021 13:09, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Thanks a lot for reviewing!

20.09.2021 09:44, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


Add command that can add and remove filters.

Key points of functionality:

What the command does is simply replace some BdrvChild.bs by some other
nodes. The tricky thing is selecting there BdrvChild objects.
To be able to select any kind of BdrvChild we use a generic parent_id,
which may be a node-name, or qdev id or block export id. In future we
may support block jobs.

Any kind of ambiguity leads to error. If we have both device named
device0 and block export named device0 and they both point to same BDS,
user can't replace root child of one of these parents. So, to be able
to do replacements, user should avoid duplicating names in different
parent namespaces.

So, command allows to replace any single child in the graph.

On the other hand we want to realize a kind of bdrv_replace_node(),
which works well when we want to replace all parents of some node. For
this kind of task @parents-mode argument implemented.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   qapi/block-core.json | 78 +
   block.c  | 82 
   2 files changed, 160 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 675d8265eb..8059b96341 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5433,3 +5433,81 @@
   { 'command': 'blockdev-snapshot-delete-internal-sync',
 'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
 'returns': 'SnapshotInfo' }
+
+##
+# @BlockdevReplaceParentsMode:
+#
+# Alternative (to directly set @parent) way to chose parents in
+# @blockdev-replace
+#
+# @exactly-one: Exactly one parent should match a condition, otherwise
+#   @blockdev-replace fails.
+#
+# @all: All matching parents are taken into account. If replacing lead
+#   to loops in block graph, @blockdev-replace fails.
+#
+# @auto: Same as @all, but automatically skip replacing parents if it
+#leads to loops in block graph.
+#
+# Since: 6.2
+##
+{ 'enum': 'BlockdevReplaceParentsMode',
+  'data': ['exactly-one', 'all', 'auto'] }
+
+##
+# @BlockdevReplace:
+#
+# Declaration of one replacement.


Replacement of what?  A node in the block graph?


A specific child node in one or in several edges


Spell that out in the doc comment, please.




+#
+# @parent: id of parent. It may be qdev or block export or simple
+#  node-name.


It may also be a QOM path, because find_device_state() interprets
arguments starting with '/' as QOM paths.

When is a node name "simple"?

Suggest: It may be a qdev ID, a QOM path, a block export ID, or a node
name.


OK



The trouble is of course that we're merging three separate name spaces.


Yes. Alternatively we can also add an enum of node-type [bds, device, export[, 
job]], and select graph nodes more explicit (by pair of id/path/name and type)

But if we have to use these things in one context, it seems good to enforce 
users use different names for them. And in this way, we can avoid strict typing.


Is there precedence in QMP for merging ID name spaces, or for selecting
a name space?


Hmm, I didn't see neither of it.




Aside: a single name space for IDs would be so much saner, but we
screwed that up long ago.


Throwing some of the multiple name spaces together some of the time
feels like another mistake.




 If id is ambiguous (for example node-name of
+#  some BDS equals to block export name), blockdev-replace
+#  fails.


Is there a way out of this situation, or are is replacement simply
impossible then?


In my idea, it's simply impossible. If someone want to use this new interface, 
he should care to use different names for different things.


Reminds me of device_del, which simply could not delete a device without
an ID.  Made many users go "oh" (or possibly a more colorful version
thereof), until daniel fixed it in commit 6287d827d4 "monitor: allow
device_del to accept QOM paths" for v2.5.




 If not specified, blockdev-replace goes through
+#  @parents-mode scenario, see below. Note, that @parent and
+#  @parents-mode can't be specified simultaneously.


What if neither is specified?  Hmm, @parents-mode has a default, so
that's what we get.


+#  If @parent is specified, only one edge is selected. If
+#  several edges match the condition, blockdev-replace fails.
+#
+# @edge: name of the child. If omitted, any child name matches.
+#
+# @child: node-name of the child. If omitted, any child matches.
+# Must be present if @parent is not specified.


Is @child useful when @parent is present?


You may specify @child and @parent, to replace child in specific edge. Or 
@parent and @edge. Or all three fields: just to be strict.



What's the 

Re: [PATCH 1/2] docs/devel/testing: add instruction to run a single acceptance test

[Willian Rampazzo]

On Thu, Sep 23, 2021 at 5:27 AM Daniel P. Berrangé  wrote:
>
> On Wed, Sep 22, 2021 at 04:03:23PM -0300, Willian Rampazzo wrote:
> > Add instructions to the Acceptance tests section about running a
> > single test file or a test within the test file.
> >
> > Signed-off-by: Willian Rampazzo 
> > ---
> >  docs/devel/testing.rst | 14 ++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> > index 4a0abbf23d..b03df34f7b 100644
> > --- a/docs/devel/testing.rst
> > +++ b/docs/devel/testing.rst
> > @@ -740,6 +740,20 @@ may be invoked by running:
> >
> >tests/venv/bin/avocado run $OPTION1 $OPTION2 tests/acceptance/
> >
> > +It is also possible to run tests from a single file or a single test
> > +within a test file. To run tests from a single file within the build
> > +tree, use:
> > +
> > + .. code::
> > +
> > +  tests/venv/bin/avocado run tests/acceptance/$TESTFILE
>
> Before running this users may well need to run
>
>make check-venv
>
> we can't assume they have previously done "make check-acceptance"
> as they're possibly just reproducing a failure from gitlab
> CI locally, not running the whole suite.

Indeed!

>
> > +
> > +To run a single test within a test file, use:
> > +
> > + .. code::
> > +
> > +  tests/venv/bin/avocado run 
> > tests/acceptance/$TESTFILE:$TESTCLASS.$TESTNAME
> > +
>
> Valid test names are visible in the output from any previous execution
> of avocado, and can also be queried using
>
>   tests/venv/bin/avocado list tests/acceptance
>

Ack.

> >  Manual Installation
> >  ---
> >
> > --
> > 2.31.1
> >
>
> 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 v6 17/21] LoongArch Linux User Emulation

[Laurent Vivier]

Le 17/09/2021 à 10:12, Song Gao a écrit :
> Implementation of linux user emulation for LoongArch.
> 
> Signed-off-by: Song Gao 
> Signed-off-by: XiaoJuan Yang 
> ---
>  accel/tcg/user-exec.c  |  15 ++
>  configure  |   5 +
>  include/elf.h  |   2 +
>  linux-user/elfload.c   |  58 ++
>  linux-user/host/loongarch/hostdep.h|  11 +
>  linux-user/loongarch64/cpu_loop.c  |  97 +
>  linux-user/loongarch64/signal.c| 162 +++
>  linux-user/loongarch64/sockbits.h  |   1 +
>  linux-user/loongarch64/syscall_nr.h| 312 
> +
>  linux-user/loongarch64/target_cpu.h|  35 
>  linux-user/loongarch64/target_elf.h|  14 ++
>  linux-user/loongarch64/target_errno_defs.h |   7 +
>  linux-user/loongarch64/target_fcntl.h  |  12 ++
>  linux-user/loongarch64/target_signal.h |  30 +++
>  linux-user/loongarch64/target_structs.h|  49 +
>  linux-user/loongarch64/target_syscall.h|  46 +
>  linux-user/loongarch64/termbits.h  |   1 +
>  linux-user/syscall_defs.h  |  10 +-
>  18 files changed, 863 insertions(+), 4 deletions(-)
>  create mode 100644 linux-user/host/loongarch/hostdep.h
>  create mode 100644 linux-user/loongarch64/cpu_loop.c
>  create mode 100644 linux-user/loongarch64/signal.c
>  create mode 100644 linux-user/loongarch64/sockbits.h
>  create mode 100644 linux-user/loongarch64/syscall_nr.h
>  create mode 100644 linux-user/loongarch64/target_cpu.h
>  create mode 100644 linux-user/loongarch64/target_elf.h
>  create mode 100644 linux-user/loongarch64/target_errno_defs.h
>  create mode 100644 linux-user/loongarch64/target_fcntl.h
>  create mode 100644 linux-user/loongarch64/target_signal.h
>  create mode 100644 linux-user/loongarch64/target_structs.h
>  create mode 100644 linux-user/loongarch64/target_syscall.h
>  create mode 100644 linux-user/loongarch64/termbits.h
> 
...

> diff --git a/linux-user/loongarch64/syscall_nr.h 
> b/linux-user/loongarch64/syscall_nr.h
> new file mode 100644
> index 000..8fbf287
> --- /dev/null
> +++ b/linux-user/loongarch64/syscall_nr.h
> @@ -0,0 +1,312 @@
> +/*
> + * This file contains the system call numbers.
> + * Do not modify.
> + * This file is generated by scripts/gensyscalls.sh
> + */

Where are the changes to scripts/gensyscalls.sh?

You need to add something like:

generate_syscall_nr loongarch 64 "$output/linux-user/loongarch64/syscall_nr.h"


Thanks,
Laurent

Re: [PATCH v6 17/21] LoongArch Linux User Emulation

[Song Gao]

Hi, Laurent.

On 09/23/2021 02:53 PM, Laurent Vivier wrote:
>> diff --git a/linux-user/loongarch64/syscall_nr.h 
>> b/linux-user/loongarch64/syscall_nr.h
>> new file mode 100644
>> index 000..8fbf287
>> --- /dev/null
>> +++ b/linux-user/loongarch64/syscall_nr.h
>> @@ -0,0 +1,312 @@
>> +/*
>> + * This file contains the system call numbers.
>> + * Do not modify.
>> + * This file is generated by scripts/gensyscalls.sh
>> + */
> Where are the changes to scripts/gensyscalls.sh?
> > You need to add something like:
> 
> generate_syscall_nr loongarch 64 "$output/linux-user/loongarch64/syscall_nr.h"

I‘ll add it in v7.

Thanks.
Song Gao.

[PATCH v5 06/20] nubus: implement BusClass get_dev_path()

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index d4daaa36f2..df93a07dfe 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -96,6 +96,21 @@ static void nubus_init(Object *obj)
  NUBUS_SLOT_NB);
 }
 
+static char *nubus_get_dev_path(DeviceState *dev)
+{
+NubusDevice *nd = NUBUS_DEVICE(dev);
+BusState *bus = qdev_get_parent_bus(dev);
+char *p = qdev_get_dev_path(bus->parent);
+
+if (p) {
+char *ret = g_strdup_printf("%s/%s/%02x", p, bus->name, nd->slot);
+g_free(p);
+return ret;
+} else {
+return g_strdup_printf("%s/%02x", bus->name, nd->slot);
+}
+}
+
 static bool nubus_check_address(BusState *bus, DeviceState *dev, Error **errp)
 {
 NubusDevice *nd = NUBUS_DEVICE(dev);
@@ -131,6 +146,7 @@ static void nubus_class_init(ObjectClass *oc, void *data)
 
 bc->realize = nubus_realize;
 bc->check_address = nubus_check_address;
+bc->get_dev_path = nubus_get_dev_path;
 }
 
 static const TypeInfo nubus_bus_info = {
-- 
2.20.1

Re: [RFC PATCH] spapr/xive: Allocate vCPU IPIs from local context

[Greg Kurz]

On Wed, 22 Sep 2021 16:41:20 +0200
Cédric Le Goater  wrote:

> When QEMU switches to the XIVE interrupt mode, it creates all possible
> guest interrupts at the level of the KVM device. These interrupts are
> backed by real HW interrupts from the IPI interrupt pool of the XIVE
> controller.
> 
> Currently, this is done from the QEMU main thread, which results in
> allocating all interrupts from the chip on which QEMU is running. IPIs
> are not distributed across the system and the load is not well
> balanced across the interrupt controllers.
> 
> To improve distribution on the system, we should try to allocate the
> underlying HW IPI from the vCPU context. This also benefits to
> configurations using CPU pinning. In this case, the HW IPI is
> allocated on the local chip, rerouting between interrupt controllers
> is reduced and performance improved.
> 
> This moves the initialization of the vCPU IPIs from reset time to the
> H_INT_SET_SOURCE_CONFIG hcall which is called from the vCPU context.
> But this needs some extra checks in the sequences getting and setting
> the source states to make sure a valid HW IPI is backing the guest
> interrupt. For that, we check if a target was configured in the END in
> case of a vCPU IPI.
> 
> Signed-off-by: Cédric Le Goater 
> ---
> 
>  I have tested different SMT configurations, kernel_irqchip=off/on,
>  did some migrations, CPU hotplug, etc. It's not enough and I would
>  like more testing but, at least, it is not making anymore the bad
>  assumption vCPU id = IPI number.
> 

Yeah, the IPI number is provided by the guest, so h_int_set_source_config()
is really the only place where we can know the IPI number of a given vCPU.

>  Comments ? 
> 

LGTM but I didn't check if more users of xive_end_is_valid() should
be converted to using xive_source_is_initialized().

Any chance you have some perf numbers to share ?

>  hw/intc/spapr_xive.c | 17 +
>  hw/intc/spapr_xive_kvm.c | 36 +++-
>  2 files changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 6f31ce74f198..2dc594a720b1 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1089,6 +1089,23 @@ static target_ulong h_int_set_source_config(PowerPCCPU 
> *cpu,
>  if (spapr_xive_in_kernel(xive)) {
>  Error *local_err = NULL;
>  
> +/*
> + * Initialize the vCPU IPIs from the vCPU context to allocate
> + * the backing HW IPI on the local chip. This improves
> + * distribution of the IPIs in the system and when the vCPUs
> + * are pinned, it reduces rerouting between interrupt
> + * controllers for better performance.
> + */
> +if (lisn < SPAPR_XIRQ_BASE) {
> +XiveSource *xsrc = >source;
> +
> +kvmppc_xive_source_reset_one(xsrc, lisn, _err);
> +if (local_err) {
> +error_report_err(local_err);
> +return H_HARDWARE;
> +}
> +}
> +
>  kvmppc_xive_set_source_config(xive, lisn, _eas, _err);
>  if (local_err) {
>  error_report_err(local_err);
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 53731d158625..1607a59e9483 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -254,7 +254,12 @@ static int kvmppc_xive_source_reset(XiveSource *xsrc, 
> Error **errp)
>  SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
>  int i;
>  
> -for (i = 0; i < xsrc->nr_irqs; i++) {
> +/*
> + * vCPU IPIs are initialized at the KVM level when configured by
> + * H_INT_SET_SOURCE_CONFIG.
> + */
> +
> +for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) {
>  int ret;
>  
>  if (!xive_eas_is_valid(>eat[i])) {
> @@ -342,6 +347,27 @@ uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, 
> uint32_t offset,
>  }
>  }
>  
> +static bool xive_source_is_initialized(SpaprXive *xive, int lisn)
> +{
> +assert(lisn < xive->nr_irqs);
> +
> +if (!xive_eas_is_valid(>eat[lisn])) {
> +return false;
> +}
> +
> +/*
> + * vCPU IPIs are initialized at the KVM level when configured by
> + * H_INT_SET_SOURCE_CONFIG, in which case, we should have a valid
> + * target (server, priority) in the END.
> + */
> +if (lisn < SPAPR_XIRQ_BASE) {
> +return !!xive_get_field64(EAS_END_INDEX, xive->eat[lisn].w);
> +}
> +
> +/* Device sources */
> +return true;
> +}
> +
>  static void kvmppc_xive_source_get_state(XiveSource *xsrc)
>  {
>  SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> @@ -350,7 +376,7 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
>  for (i = 0; i < xsrc->nr_irqs; i++) {
>  uint8_t pq;
>  
> -if (!xive_eas_is_valid(>eat[i])) {
> +if (!xive_source_is_initialized(xive, i)) {
>  continue;
>  }
>  
> @@ -533,7 +559,7 @@ static void 

[PATCH v5 00/20] nubus: bus, device, bridge, IRQ and address space improvements

[Mark Cave-Ayland]

This patchset is the next set of changes required to boot MacOS on the q800 
machine. The
main aim of these patches is to improve the Nubus support so that devices can 
be plugged
into the Nubus from the command line i.e.

-device nubus-macfb[,slot=num][,romfile=decl.rom]

At the moment the only device that can be plugged into the Nubus is the macfb 
framebuffer
however with these changes it is possible to take a ROM from a real Nubus card 
and
attempt to use it in QEMU, and also allow for future interfaces such as virtio.

Patches 1 to 6 move the logic which manages bus addresses from the NubusDevice 
into
the NubusBus itself, including the introduction of a bitmap to manage available
slots on the bus.

Patches 7 and 8 change the handling for unassigned (empty) slots to generate a 
bus
fault and add trace events to allow logging of empty slot accesses during Nubus
enumeration.

Patches 9 to 11 remove the existing stubs for generating the format block (the 
epilogue
of the Nubus device embedded ROM consisting of metadata and a checksum) and 
replace them
with a romfile device property to allow the entire Nubus ROM to be loaded from 
a file
into the ROM area, similar to a PCI option ROM.

Patch 12 moves the Nubus into its own separate address space whilst patches 13 
to 17
update the NubusBridge (and MacNubusBridge) devices to allow machines to map the
required slots from the Nubus address space using sysbus_mmio_map().

Finally patches 18 to 20 add support for Nubus IRQs and wire them up 
appropriately for
the q800 machine through VIA2, which is required for the next set of macfb 
updates.

Signed-off-by: Mark Cave-Ayland 


v5:
- Rebase onto master
- Add R-B tags from Laurent
- Introduce NUBUS_FIRST_SLOT/NUBUS_LAST_SLOT and 
MAC_NUBUS_FIRST_SLOT/MAC_NUBUS_LAST_SLOT
  and fix up NUBUS_SUPER_SLOT_NB/NUBUS_SLOT_NB in patch 4
- Fix super slot offset calculation in patch 4
- Squash original patch 3 ("nubus-device: add device slot parameter") into 
patch 4
  ("nubus: use bitmap to manage available slots")
- Add new patch 1 ("nubus: add comment indicating reference documents") 
containing
  documentation references
- Drop "nubus->slot_available_mask = MAKE_64BIT_MASK(0, 16);" from nubus_init() 
in patch 17
  
v4:
- Rebase onto master
- Pass _abort to memory_region_init_rom() in patch 11
- Change warn_error() to error_setg() and tweak message in patch 11

v3:
- Rebase onto master
- Add Phil's R-B for patch 7
- Move NUBUS_FIRST_SLOT/NUBUS_LAST_SLOT check to end of nubus_device_realize() 
in patch 4
- Use BIT() macro in patches 4 and 20

v2:
- Rebase onto master
- Tweak the cover letter by adding the optional slot parameter in the -device 
example
- Add R-B tags from Phil
- Document the increase in max_access_size in patch 7
- Change the maximum declaration ROM size to 128KiB using (128 * KiB) in patch 
11
- use MAKE_64BIT_MASK() in patches 4 and 16


Mark Cave-Ayland (20):
  nubus: add comment indicating reference documents
  nubus-device: rename slot_nb variable to slot
  nubus-device: expose separate super slot memory region
  nubus: use bitmap to manage available slots
  nubus: move slot bitmap checks from NubusDevice realize() to BusClass
check_address()
  nubus: implement BusClass get_dev_path()
  nubus: add trace-events for empty slot accesses
  nubus: generate bus error when attempting to access empty slots
  macfb: don't register declaration ROM
  nubus-device: remove nubus_register_rom() and
nubus_register_format_block()
  nubus-device: add romfile property for loading declaration ROMs
  nubus: move nubus to its own 32-bit address space
  nubus-bridge: introduce separate NubusBridge structure
  mac-nubus-bridge: rename MacNubusState to MacNubusBridge
  nubus: move NubusBus from mac-nubus-bridge to nubus-bridge
  nubus-bridge: embed the NubusBus object directly within nubus-bridge
  nubus-bridge: make slot_available_mask a qdev property
  nubus: add support for slot IRQs
  q800: wire up nubus IRQs
  q800: configure nubus available slots for Quadra 800

 hw/display/macfb.c  |   6 -
 hw/m68k/q800.c  |  26 +++-
 hw/nubus/mac-nubus-bridge.c |  34 -
 hw/nubus/nubus-bridge.c |  23 ++-
 hw/nubus/nubus-bus.c| 121 ---
 hw/nubus/nubus-device.c | 227 
 hw/nubus/trace-events   |   7 +
 hw/nubus/trace.h|   1 +
 include/hw/nubus/mac-nubus-bridge.h |  13 +-
 include/hw/nubus/nubus.h|  49 +++---
 meson.build |   1 +
 11 files changed, 279 insertions(+), 229 deletions(-)
 create mode 100644 hw/nubus/trace-events
 create mode 100644 hw/nubus/trace.h

-- 
2.20.1

[PATCH v5 05/20] nubus: move slot bitmap checks from NubusDevice realize() to BusClass check_address()

[Mark Cave-Ayland]

Allow Nubus to manage the slot allocations itself using the BusClass 
check_address()
virtual function rather than managing this during NubusDevice realize().

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c| 30 ++
 hw/nubus/nubus-device.c | 22 --
 2 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 3cd7534864..d4daaa36f2 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -96,11 +96,41 @@ static void nubus_init(Object *obj)
  NUBUS_SLOT_NB);
 }
 
+static bool nubus_check_address(BusState *bus, DeviceState *dev, Error **errp)
+{
+NubusDevice *nd = NUBUS_DEVICE(dev);
+NubusBus *nubus = NUBUS_BUS(bus);
+uint16_t s;
+
+if (nd->slot == -1) {
+/* No slot specified, find first available free slot */
+s = ctz32(nubus->slot_available_mask);
+if (s != 32) {
+nd->slot = s;
+} else {
+error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+return false;
+}
+} else {
+/* Slot specified, make sure the slot is available */
+if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+return false;
+}
+}
+
+nubus->slot_available_mask &= ~BIT(nd->slot);
+return true;
+}
+
 static void nubus_class_init(ObjectClass *oc, void *data)
 {
 BusClass *bc = BUS_CLASS(oc);
 
 bc->realize = nubus_realize;
+bc->check_address = nubus_check_address;
 }
 
 static const TypeInfo nubus_bus_info = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 562650a05b..516b13d2d5 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -160,28 +160,6 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 NubusDevice *nd = NUBUS_DEVICE(dev);
 char *name;
 hwaddr slot_offset;
-uint16_t s;
-
-if (nd->slot == -1) {
-/* No slot specified, find first available free slot */
-s = ctz32(nubus->slot_available_mask);
-if (s != 32) {
-nd->slot = s;
-} else {
-error_setg(errp, "Cannot register nubus card, no free slot "
- "available");
-return;
-}
-} else {
-/* Slot specified, make sure the slot is available */
-if (!(nubus->slot_available_mask & BIT(nd->slot))) {
-error_setg(errp, "Cannot register nubus card, slot %d is "
- "unavailable or already occupied", nd->slot);
-return;
-}
-}
-
-nubus->slot_available_mask &= ~BIT(nd->slot);
 
 /* Super */
 slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
-- 
2.20.1

Re: [PATCH v5 04/20] nubus: use bitmap to manage available slots

[Philippe Mathieu-Daudé]

On 9/23/21 11:12, Mark Cave-Ayland wrote:

Convert nubus_device_realize() to use a bitmap to manage available slots to 
allow
for future Nubus devices to be plugged into arbitrary slots from the command 
line
using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh
machines as documented in "Desigining Cards and Drivers for the Macintosh 
Family".

Signed-off-by: Mark Cave-Ayland 
---
  hw/nubus/mac-nubus-bridge.c |  4 
  hw/nubus/nubus-bus.c|  5 +++--
  hw/nubus/nubus-device.c | 32 +++--
  include/hw/nubus/mac-nubus-bridge.h |  4 
  include/hw/nubus/nubus.h| 13 ++--
  5 files changed, 43 insertions(+), 15 deletions(-)



diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 4e23df1280..562650a05b 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
  NubusDevice *nd = NUBUS_DEVICE(dev);
  char *name;
  hwaddr slot_offset;
-
-if (nubus->current_slot < NUBUS_FIRST_SLOT ||
-nubus->current_slot > NUBUS_LAST_SLOT) {
-error_setg(errp, "Cannot register nubus card, not enough slots");
-return;
+uint16_t s;
+
+if (nd->slot == -1) {
+/* No slot specified, find first available free slot */
+s = ctz32(nubus->slot_available_mask);


Nitpicking:

   int s = ctz32(nubus->slot_available_mask);

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé 


+if (s != 32) {
+nd->slot = s;
+} else {
+error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+return;
+}
+} else {
+/* Slot specified, make sure the slot is available */
+if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+return;
+}
  }

Re: [PATCH 2/2] tests/Makefile: add TESTFILES option to make check-acceptance

[Daniel P . Berrangé]

On Thu, Sep 23, 2021 at 11:34:18AM +0200, Philippe Mathieu-Daudé wrote:
> On 9/22/21 21:46, Willian Rampazzo wrote:
> > On Wed, Sep 22, 2021 at 4:08 PM Philippe Mathieu-Daudé
> >  wrote:
> > > 
> > > On 9/22/21 21:03, Willian Rampazzo wrote:
> > > > Add the possibility of running all the tests from a single file, or
> > > > multiple files, running a single test within a file or multiple tests
> > > > within multiple files using `make check-acceptance` and the TESTFILES
> > > > environment variable.
> > > > 
> > > > Signed-off-by: Willian Rampazzo 
> > > > ---
> > > >docs/devel/testing.rst | 27 +++
> > > >tests/Makefile.include |  5 -
> > > >2 files changed, 31 insertions(+), 1 deletion(-)
> > > 
> > > > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > > > index 6e16c05f10..82d7ef7a20 100644
> > > > --- a/tests/Makefile.include
> > > > +++ b/tests/Makefile.include
> > > > @@ -88,6 +88,9 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
> > > >TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
> > > >TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
> > > >TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
> > > > +ifndef TESTFILES
> > > > + TESTFILES=tests/acceptance
> > > > +endif
> > > ># Controls the output generated by Avocado when running tests.
> > > ># Any number of command separated loggers are accepted.  For more
> > > ># information please refer to "avocado --help".
> > > > @@ -130,7 +133,7 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR) 
> > > > get-vm-images
> > > >--show=$(AVOCADO_SHOW) run 
> > > > --job-results-dir=$(TESTS_RESULTS_DIR) \
> > > >--filter-by-tags-include-empty 
> > > > --filter-by-tags-include-empty-key \
> > > >$(AVOCADO_TAGS) \
> > > > -$(if $(GITLAB_CI),,--failfast) tests/acceptance, \
> > > > +$(if $(GITLAB_CI),,--failfast) $(TESTFILES), \
> > > 
> > > Since this is Avocado specific, maybe call the variable
> > > AVOCADO_TESTFILES (similar to AVOCADO_TAGS)?
> > 
> > I don't see a problem with changing that to AVOCADO_TESTFILES. I was
> > trying to make things shorter and easy to remember. If the too-long
> > variable name is not a problem, I can change that.
> 
> This is the generic tests/Makefile, so $TESTFILES might be confusing,
> which is why I prefer the explicit AVOCADO_ prefix (AVOCADO_SHOW,
> AVOCADO_TAGS).

IIUC, this is not actually just test files - it is test files plus the
test names. So better just  $(AVOCADO_TESTS)


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 v5 17/20] nubus-bridge: make slot_available_mask a qdev property

[Philippe Mathieu-Daudé]

On 9/23/21 11:13, Mark Cave-Ayland wrote:

This is to allow Macintosh machines to further specify which slots are available
since the number of addressable slots may not match the number of physical slots
present in the machine.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bridge.c | 7 +++
  1 file changed, 7 insertions(+)



+static Property nubus_bridge_properties[] = {
+DEFINE_PROP_UINT32("slot-available-mask", NubusBridge,
+   bus.slot_available_mask, 0x),


What about using DEFINE_PROP_UINT16() here and uint16_t in
patch 04/20 "nubus: use bitmap to manage available slots"?

Regardless:
Reviewed-by: Philippe Mathieu-Daudé 


+DEFINE_PROP_END_OF_LIST()
+};

Re: [PATCH 2/2] tests/Makefile: add TESTFILES option to make check-acceptance

[Willian Rampazzo]

On Thu, Sep 23, 2021 at 6:42 AM Daniel P. Berrangé  wrote:
>
> On Thu, Sep 23, 2021 at 11:34:18AM +0200, Philippe Mathieu-Daudé wrote:
> > On 9/22/21 21:46, Willian Rampazzo wrote:
> > > On Wed, Sep 22, 2021 at 4:08 PM Philippe Mathieu-Daudé
> > >  wrote:
> > > >
> > > > On 9/22/21 21:03, Willian Rampazzo wrote:
> > > > > Add the possibility of running all the tests from a single file, or
> > > > > multiple files, running a single test within a file or multiple tests
> > > > > within multiple files using `make check-acceptance` and the TESTFILES
> > > > > environment variable.
> > > > >
> > > > > Signed-off-by: Willian Rampazzo 
> > > > > ---
> > > > >docs/devel/testing.rst | 27 +++
> > > > >tests/Makefile.include |  5 -
> > > > >2 files changed, 31 insertions(+), 1 deletion(-)
> > > >
> > > > > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > > > > index 6e16c05f10..82d7ef7a20 100644
> > > > > --- a/tests/Makefile.include
> > > > > +++ b/tests/Makefile.include
> > > > > @@ -88,6 +88,9 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
> > > > >TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
> > > > >TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
> > > > >TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
> > > > > +ifndef TESTFILES
> > > > > + TESTFILES=tests/acceptance
> > > > > +endif
> > > > ># Controls the output generated by Avocado when running tests.
> > > > ># Any number of command separated loggers are accepted.  For more
> > > > ># information please refer to "avocado --help".
> > > > > @@ -130,7 +133,7 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR) 
> > > > > get-vm-images
> > > > >--show=$(AVOCADO_SHOW) run 
> > > > > --job-results-dir=$(TESTS_RESULTS_DIR) \
> > > > >--filter-by-tags-include-empty 
> > > > > --filter-by-tags-include-empty-key \
> > > > >$(AVOCADO_TAGS) \
> > > > > -$(if $(GITLAB_CI),,--failfast) tests/acceptance, \
> > > > > +$(if $(GITLAB_CI),,--failfast) $(TESTFILES), \
> > > >
> > > > Since this is Avocado specific, maybe call the variable
> > > > AVOCADO_TESTFILES (similar to AVOCADO_TAGS)?
> > >
> > > I don't see a problem with changing that to AVOCADO_TESTFILES. I was
> > > trying to make things shorter and easy to remember. If the too-long
> > > variable name is not a problem, I can change that.
> >
> > This is the generic tests/Makefile, so $TESTFILES might be confusing,
> > which is why I prefer the explicit AVOCADO_ prefix (AVOCADO_SHOW,
> > AVOCADO_TAGS).
>
> IIUC, this is not actually just test files - it is test files plus the
> test names. So better just  $(AVOCADO_TESTS)
>

Ack. I'll send another version soon.

>
> 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 1/2] meson: introduce modules_arch

[Paolo Bonzini]

On 21/09/21 15:46, Jose R. Ziviani wrote:

Alternatively, you could C-ify the contents of config-devices.mak, and embed
them in the per-arch modinfo-*.c; and record CONFIG_* symbols for each
module (e.g. '{ "CONFIG_QXL", "hw-display-qxl" }' from a
'module_config("CONFIG_QXL")' line in the global modinfo.c file.  Then
before loading a module you do a binary search on the per-arch
config-devices array.

With a per-arch modinfo-*.c we don't even need a modinfo.c global, do
we?

Each target could be linked to its own modinfo-target.c only.


Yes, I suppose you don't need it.  However, you may want to use 
different Python scripts to generate modinfo-*.c (currently from 
config-devices.mak only) and modinfo.c (from compile_commands.json and 
various sources), so it may be handy to separate them.


Paolo

Re: [PATCH] hw/loader: Restrict PC_ROM_* definitions to hw/i386/pc

[Laurent Vivier]

Le 17/09/2021 à 20:59, Philippe Mathieu-Daudé a écrit :
> The PC_ROM_* definitions are only used by the PC machine,
> and are irrelevant to the other architectures / machines.
> Reduce their scope by moving them to hw/i386/pc.c.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/loader.h | 6 --
>  hw/i386/pc.c| 6 ++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index cbfc1848737..81104cb02fe 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -336,12 +336,6 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
>  #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)  \
>  rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
>  
> -#define PC_ROM_MIN_VGA 0xc
> -#define PC_ROM_MIN_OPTION  0xc8000
> -#define PC_ROM_MAX 0xe
> -#define PC_ROM_ALIGN   0x800
> -#define PC_ROM_SIZE(PC_ROM_MAX - PC_ROM_MIN_VGA)
> -
>  int rom_add_vga(const char *file);
>  int rom_add_option(const char *file, int32_t bootindex);
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7e523b913ca..557d49c9f8f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -843,6 +843,12 @@ void xen_load_linux(PCMachineState *pcms)
>  x86ms->fw_cfg = fw_cfg;
>  }
>  
> +#define PC_ROM_MIN_VGA 0xc
> +#define PC_ROM_MIN_OPTION  0xc8000
> +#define PC_ROM_MAX 0xe
> +#define PC_ROM_ALIGN   0x800
> +#define PC_ROM_SIZE(PC_ROM_MAX - PC_ROM_MIN_VGA)
> +
>  void pc_memory_init(PCMachineState *pcms,
>  MemoryRegion *system_memory,
>  MemoryRegion *rom_memory,
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device

[David Hildenbrand]

On 23.09.21 09:34, Markus Armbruster wrote:

David Hildenbrand  writes:


We want to rate-limit MEMORY_DEVICE_SIZE_CHANGE events per device,
otherwise we can lose some events for devices. As we might not always have
a device id, add the qom-path to the event and use that to rate-limit
per device.


There are actually two reasons for adding qom-path.  One, you need it to
fix the rate limiting.  But adding to an external interface just to fix
an internal issue would be questionable.  Fortunately, there's also two:
make the event useful regardless of whether the user gave it an ID.  If
you have to respin, consider working two into the commit message.

I'd split this patch into "add qom-path" and "fix rate limiting".
Suggestion, not demand.


Sure, makes sense.




This was noticed by starting a VM with two virtio-mem devices that each
have a requested size > 0. The Linux guest will initialize both devices
in parallel, resulting in losing MEMORY_DEVICE_SIZE_CHANGE events for
one of the devices.

Fixes: 722a3c783ef4 ("virtio-pci: Send qapi events when the virtio-mem size 
changes")
Suggested-by: Markus Armbruster 
Cc: "Dr. David Alan Gilbert" 
Cc: Markus Armbruster 
Cc: Michael S. Tsirkin 
Cc: Eric Blake 
Cc: Igor Mammedov 
Cc: Michal Privoznik 
Signed-off-by: David Hildenbrand 
---

Follow up of:
 https://lkml.kernel.org/r/20210921102434.24273-1-da...@redhat.com

v1 -> v2:
- Add the qom-path and use that identifier to rate-limit per device
- Rephrase subject/description

---
  hw/virtio/virtio-mem-pci.c | 3 ++-
  monitor/monitor.c  | 9 +
  qapi/machine.json  | 5 -
  3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
index fa5395cd88..dd5085497f 100644
--- a/hw/virtio/virtio-mem-pci.c
+++ b/hw/virtio/virtio-mem-pci.c
@@ -87,6 +87,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
*notifier, void *data)
  VirtIOMEMPCI *pci_mem = container_of(notifier, VirtIOMEMPCI,
   size_change_notifier);
  DeviceState *dev = DEVICE(pci_mem);
+const char * qom_path = object_get_canonical_path(OBJECT(dev));


No space after this *, please.


Whoops :)




  const uint64_t * const size_p = data;
  const char *id = NULL;
  
@@ -94,7 +95,7 @@ static void virtio_mem_pci_size_change_notify(Notifier *notifier, void *data)

  id = g_strdup(dev->id);
  }
  
-qapi_event_send_memory_device_size_change(!!id, id, *size_p);

+qapi_event_send_memory_device_size_change(!!id, id, *size_p, qom_path);


Doesn't this leak @qom_path?



I was asking myself the same question, but ended up essentially copying 
what hw/core/machine.c:machine_query_hotpluggable_cpus() does.


object_get_canonical_path() will end up doing a g_strdup(), just like we 
do with id here. I assume qapi code will end up freeing both strings, right?


[...]

  
  ##


With the two code remarks addressed:
Reviewed-by: Markus Armbruster 



Thanks, I'll respin!

--
Thanks,

David / dhildenb

Re: [PATCH] 9pfs: fix wrong I/O block size in Rgetattr

[Greg Kurz]

On Wed, 22 Sep 2021 17:55:02 +0200
Christian Schoenebeck  wrote:

> On Mittwoch, 22. September 2021 17:42:08 CEST Philippe Mathieu-Daudé wrote:
> > On 9/22/21 15:13, Christian Schoenebeck wrote:
> > > When client sent a 9p Tgetattr request then the wrong I/O block
> > > size value was returned by 9p server; instead of host file
> > > system's I/O block size it should rather return an I/O block
> > > size according to 9p session's 'msize' value, because the value
> > > returned to client should be an "optimum" block size for I/O
> > > (i.e. to maximize performance), it should not reflect the actual
> > > physical block size of the underlying storage media.
> > > 
> > > The I/O block size of a host filesystem is typically 4k, so the
> > > value returned was far too low for good 9p I/O performance.
> > > 
> > > This patch adds stat_to_iounit() with a similar approach as the
> > > existing get_iounit() function.
> > > 
> > > Signed-off-by: Christian Schoenebeck 
> > > ---
> > > 
> > >   hw/9pfs/9p.c | 21 -
> > >   1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index c857b31321..708b030474 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -1262,6 +1262,25 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU
> > > *pdu, V9fsPath *path,> 
> > >   #define P9_STATS_ALL   0x3fffULL /* Mask for All fields
> > >   above */> 
> > > +static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat
> > > *stbuf) +{
> > > +int32_t iounit = 0;
> > > +V9fsState *s = pdu->s;
> > > +
> > > +/*
> > > + * iounit should be multiples of st_blksize (host filesystem block
> > > size) + * as well as less than (client msize - P9_IOHDRSZ)
> > > + */
> > > +if (stbuf->st_blksize) {
> > > +iounit = stbuf->st_blksize;
> > > +iounit *= (s->msize - P9_IOHDRSZ) / stbuf->st_blksize;
> > 
> > Is that:
> > 
> >iounit = QEMU_ALIGN_DOWN(s->msize - P9_IOHDRSZ, stbuf->st_blksize);
> > 
> > ?
> > 
> 
> Yes it is, thanks for the hint! :)
> 
> I actually just took the equivalent, already existing code from get_iounit():
> https://github.com/qemu/qemu/blob/2c3e83f92d93fbab071b8a96b8ab769b01902475/hw/9pfs/9p.c#L1880
> 
> Would it be OK to do that subsequently with cleanup patches? My plan was to
> first address this with one patch, and addressing the cleanup issues
> separately later on, because this patch is required for testing the following
> kernel patches:
> https://lore.kernel.org/netdev/cover.1632156835.git.linux_...@crudebyte.com/
> 
> And I wanted to keep things simple by only requiring one patch on QEMU side
> for now.
> 

Fair enough and you're the maintainer anyway so this is your
call. :-)

Subsequent cleanup would be to switch to QEMU_ALIGN_DOWN() like
Philippe suggested but also to consolidate the logic in a common
helper in order to avoid the code duplication.

The patch is correct and simple enough to be merged as is :

Reviewed-by: Greg Kurz 

> 
> > > +}
> > > +if (!iounit) {
> > > +iounit = s->msize - P9_IOHDRSZ;
> > > +}
> > > +return iounit;
> > > +}
> > > +
> > > 
> > >   static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> > >   
> > >   V9fsStatDotl *v9lstat)
> > >   
> > >   {
> > > 
> > > @@ -1273,7 +1292,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
> > > struct stat *stbuf,> 
> > >   v9lstat->st_gid = stbuf->st_gid;
> > >   v9lstat->st_rdev = stbuf->st_rdev;
> > >   v9lstat->st_size = stbuf->st_size;
> > > 
> > > -v9lstat->st_blksize = stbuf->st_blksize;
> > > +v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > > 
> > >   v9lstat->st_blocks = stbuf->st_blocks;
> > >   v9lstat->st_atime_sec = stbuf->st_atime;
> > >   v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
> 
> 

Re: [PATCH v6 00/21] Add LoongArch linux-user emulation support

[Song Gao]

Hi, Richard.

On 09/17/2021 04:12 PM, Song Gao wrote:
> Based-on: <20210822035537.283193-6-richard.hender...@linaro.org>

This patch failed in applying to current master. I am not sure how to use it 
...  


Thanks.
Song Gao 

Recent qemu patch results in aio failures with host DASD disks resulting in guest I/O errors

[Christian Borntraeger]

Am 22.09.21 um 21:51 schrieb Halil Pasic:

On Mon, 6 Sep 2021 16:24:20 +0200
Halil Pasic  wrote:


On Fri, 25 Jun 2021 16:18:12 +0200
Paolo Bonzini  wrote:


bs->sg is only true for character devices, but block devices can also
be used with scsi-block and scsi-generic.  Unfortunately BLKSECTGET
returns bytes in an int for /dev/sgN devices, and sectors in a short
for block devices, so account for that in the code.

The maximum transfer also need not be a power of 2 (for example I have
seen disks with 1280 KiB maximum transfer) so there's no need to pass
the result through pow2floor.

Signed-off-by: Paolo Bonzini 


We have found that this patch leads to in guest I/O errors when DASD
is used as a source device. I.e. libvirt domain xml wise something like:

 
   
   
   
   
   
 

I don't think it is the fault of this patch: it LGTM. But it correlates
100%, furthermore the problem seems to be related to the value of
bl.max_iov which now comes from sysfs.

We are still investigating what is actually wrong. Just wanted to give
everybody a heads-up that this does seem to cause a nasty regression on
s390x, even if the code itself is perfect.



We have figured out what is going on here. The problem seems to be
specific to linux aio, which seems to limit the size of the iovec to
1024 (UIO_MAXIOV).

Because of this requests get rejected with -EINVAL by the io_submit()
syscall. Here comes a real world example:

(gdb) p *laiocb
$5 = {co = 0x3ff700072c0, ctx = 0x3fe60002650, iocb = {data = 0x0, aio_rw_flags 
= 0, key = 0,
 aio_lio_opcode = 8, aio_reqprio = 0, aio_fildes = 38, u = {c = {buf = 
0x3ff70055bc0,
 nbytes = 1274, offset = 19977953280, __pad3 = 0, flags = 1, resfd = 
43}, v = {
 vec = 0x3ff70055bc0, nr = 0, offset = 19977953280}, poll = {__pad1 = 
1023,
 events = 1879399360}, saddr = {addr = 0x3ff70055bc0, len = 0}}}, ret = 
-22,
   nbytes = 20586496, qiov = 0x3ff700382f8, is_read = false, next = {sqe_next = 
0x0}}

On the host kernel side, the -EINVAL comes from this line:
https://elixir.bootlin.com/linux/v5.15-rc2/source/lib/iov_iter.c#L1863
in iovec_from_user() roughly via: __do_sys_io_submit()->
io_submit_one() -> aio_write() -> aio_setup_rw() -> __import_iovec().

Based on the offline discussion with the DASD maintainers, and on the
linux in source tree documentation (Documentation/block/queue-sysfs.rst
und Documentation/block/biodoc.rst), I believe that the DASD driver is
not wrong when advertising the value 65535 for queue/max_segments.

I believe QEMU jumps to the wrong conclusion in blk_get_max_iov() or
in virtio_blk_submit_multireq(), I can't really tell because I'm not
sure what the semantic of blk_get_max_iov() is. But if, I had to, I would
bet that blk_get_max_iov() returns the wrong value, when linux aio is
used. I'm not sure what is the exact relationship between max_segments
and max_iov.

One idea on how to fix this would be, to cap max_iov to UIO_MAXIOV
(unconditionally, or when linux aio is used). But I have to admit, I
don't have clarity. I couldn't even find documentation that states
this limitation of linux aio (I didn't look for it too hard though), so
I can not even be sure this is a QEMU problem.

That is why I decided to write this up first, and start a discussion on
who is playing foul with the most relevant people posted. I intend to try
myself fixing the problem once my understanding of it reaches a
sufficient level.

Thanks in advance for your contribution!


Changing subject to reflect what we see.
For reference the QEMU patch is

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=18473467d55a20d643b6c9b3a52de42f705b4d35;hp=24b36e9813ec15da7db62e3b3621730710c5f020

Re: [PATCH 2/2] tests/Makefile: add TESTFILES option to make check-acceptance

[Philippe Mathieu-Daudé]

On 9/22/21 21:46, Willian Rampazzo wrote:

On Wed, Sep 22, 2021 at 4:08 PM Philippe Mathieu-Daudé
 wrote:


On 9/22/21 21:03, Willian Rampazzo wrote:

Add the possibility of running all the tests from a single file, or
multiple files, running a single test within a file or multiple tests
within multiple files using `make check-acceptance` and the TESTFILES
environment variable.

Signed-off-by: Willian Rampazzo 
---
   docs/devel/testing.rst | 27 +++
   tests/Makefile.include |  5 -
   2 files changed, 31 insertions(+), 1 deletion(-)



diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6e16c05f10..82d7ef7a20 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -88,6 +88,9 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
   TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
   TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
   TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
+ifndef TESTFILES
+ TESTFILES=tests/acceptance
+endif
   # Controls the output generated by Avocado when running tests.
   # Any number of command separated loggers are accepted.  For more
   # information please refer to "avocado --help".
@@ -130,7 +133,7 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR) 
get-vm-images
   --show=$(AVOCADO_SHOW) run 
--job-results-dir=$(TESTS_RESULTS_DIR) \
   --filter-by-tags-include-empty 
--filter-by-tags-include-empty-key \
   $(AVOCADO_TAGS) \
-$(if $(GITLAB_CI),,--failfast) tests/acceptance, \
+$(if $(GITLAB_CI),,--failfast) $(TESTFILES), \


Since this is Avocado specific, maybe call the variable
AVOCADO_TESTFILES (similar to AVOCADO_TAGS)?


I don't see a problem with changing that to AVOCADO_TESTFILES. I was
trying to make things shorter and easy to remember. If the too-long
variable name is not a problem, I can change that.


This is the generic tests/Makefile, so $TESTFILES might be confusing,
which is why I prefer the explicit AVOCADO_ prefix (AVOCADO_SHOW,
AVOCADO_TAGS).

Thomas, do you have a preference?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] docs/nvdimm: Update nvdimm option value in machine example

[Pankaj Gupta]

ping

> Update nvdimm option value in example command from "-machine pc,nvdimm"
> to "-machine pc,nvdimm=on" as former complains with the below error:
>
> "qemu-system-x86_64: -machine pc,nvdimm: Expected '=' after parameter 
> 'nvdimm'"
>
> Signed-off-by: Pankaj Gupta 
> ---
>  docs/nvdimm.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index 0aae682be3..fd7773dc5a 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -15,7 +15,7 @@ backend (i.e. memory-backend-file and memory-backend-ram). 
> A simple
>  way to create a vNVDIMM device at startup time is done via the
>  following command line options:
>
> - -machine pc,nvdimm
> + -machine pc,nvdimm=on
>   -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
>   -object 
> memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
>   -device nvdimm,id=nvdimm1,memdev=mem1,unarmed=off
> --
> 2.25.1
>

Re: [PATCH 1/2] tests/qapi-schema: Use Python OSError instead of outmoded IOError

[Philippe Mathieu-Daudé]

On 9/23/21 11:33, Markus Armbruster wrote:

John Snow  writes:


On Wed, Sep 22, 2021 at 8:56 AM Markus Armbruster  wrote:


Signed-off-by: Markus Armbruster 
---
  tests/qapi-schema/test-qapi.py | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qapi-schema/test-qapi.py
b/tests/qapi-schema/test-qapi.py
index 73cffae2b6..2e384f5efd 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -154,7 +154,7 @@ def test_and_diff(test_name, dir_name, update):
  errfp = open(os.path.join(dir_name, test_name + '.err'), mode)
  expected_out = outfp.readlines()
  expected_err = errfp.readlines()
-except IOError as err:
+except OSError as err:
  print("%s: can't open '%s': %s"
% (sys.argv[0], err.filename, err.strerror),
file=sys.stderr)
@@ -180,7 +180,7 @@ def test_and_diff(test_name, dir_name, update):
  errfp.truncate(0)
  errfp.seek(0)
  errfp.writelines(actual_err)
-except IOError as err:
+except OSError as err:
  print("%s: can't write '%s': %s"
% (sys.argv[0], err.filename, err.strerror),
file=sys.stderr)
--
2.31.1



If you're happy with the expanded scope of the exception-catcher, I am too.


https://docs.python.org/3.6/library/exceptions.html has

 Changed in version 3.3: EnvironmentError, IOError, WindowsError,
 socket.error, select.error and mmap.error have been merged into
 OSError, and the constructor may return a subclass.

and

 The following exceptions are kept for compatibility with previous
 versions; starting from Python 3.3, they are aliases of OSError.

 exception EnvironmentError

 exception IOError

 exception WindowsError

 Only available on Windows.


With that information amended to the description:
Reviewed-by: Philippe Mathieu-Daudé 


So unless I'm misunderstanding something (which is quite possible),
we're catching exactly the same exceptions as before, we just switch to
their preferred name.


Reviewed-by: John Snow 


Thanks!

Re: [PATCH v5 01/20] nubus: add comment indicating reference documents

[Laurent Vivier]

Le 23/09/2021 à 11:12, Mark Cave-Ayland a écrit :
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/nubus/nubus-bus.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
> index 5c13452308..f4410803ff 100644
> --- a/hw/nubus/nubus-bus.c
> +++ b/hw/nubus/nubus-bus.c
> @@ -8,6 +8,14 @@
>   *
>   */
>  
> +/*
> + * References:
> + *   Nubus Specification (TI)
> + * http://www.bitsavers.org/pdf/ti/nubus/2242825-0001_NuBus_Spec1983.pdf
> + *
> + *   Designing Cards and Drivers for the Macintosh Family (Apple)
> + */
> +
>  #include "qemu/osdep.h"
>  #include "hw/nubus/nubus.h"
>  #include "qapi/error.h"
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Philippe Mathieu-Daudé]

On 9/23/21 10:22, Daniel P. Berrangé wrote:

On Thu, Sep 23, 2021 at 04:15:55AM -0400, Paolo Bonzini wrote:

Skip the test if bzip2 is not available, and run it after they are
uncompressed.

Signed-off-by: Paolo Bonzini 
---
  pc-bios/meson.build | 3 ++-
  tests/qtest/meson.build | 6 +++---
  2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index f2b32598af..975565198e 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -10,8 +10,9 @@ if install_edk2_blobs
  'edk2-x86_64-secure-code.fd',
]
  
+  roms = []

foreach f : fds
-custom_target(f,
+roms += custom_target(f,
build_by_default: have_system,
output: f,
input: '@0@.bz2'.format(f),
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index e1f4df3df8..6d8100c9de 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -68,12 +68,12 @@ qtests_i386 = \
(config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) 
+  \
(config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? 
['fuzz-e1000e-test'] : []) +   \
(config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +   
  \
+  (install_edk2_blobs ? ['bios-tables-test'] : []) +   
 \
qtests_pci +
  \
['fdc-test',
 'ide-test',
 'hd-geo-test',
 'boot-order-test',
-   'bios-tables-test',
 'rtc-test',
 'i440fx-test',
 'fw_cfg-test',
@@ -180,7 +180,7 @@ qtests_arm = \
  
  # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test unconditional

  qtests_aarch64 = \
-  (cpu != 'arm' ? ['bios-tables-test'] : []) + 
 \
+  (cpu != 'arm' and install_edk2_blobs ? ['bios-tables-test'] : []) +  
 \
(config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-test'] : []) +\
(config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
['tpm-tis-device-swtpm-test'] : []) +  \
['arm-cpu-features',
@@ -269,7 +269,7 @@ foreach dir : target_dirs
qtest_emulator = emulators['qemu-system-' + target_base]
target_qtests = get_variable('qtests_' + target_base, []) + qtests_generic
  
-  test_deps = []

+  test_deps = roms


Shouldn't this be

   if install_edk2_blobs
  test_deps += roms
   endif


See also "blobs: Only install required (system emulation) files":
https://lore.kernel.org/qemu-devel/20210323155132.238193-1-f4...@amsat.org/

Re: [PATCH v5 14/20] mac-nubus-bridge: rename MacNubusState to MacNubusBridge

[Laurent Vivier]

Le 23/09/2021 à 11:13, Mark Cave-Ayland a écrit :
> This better reflects that the mac-nubus-bridge device is derived from the
> nubus-bridge device, and that the structure represents the state of the bridge
> device and not the Nubus itself. Also update the comment in the file header to
> reflect that mac-nubus-bridge is specific to the Macintosh.
> 
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/nubus/mac-nubus-bridge.c | 8 +---
>  include/hw/nubus/mac-nubus-bridge.h | 4 ++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 


Reviewed-by: Laurent Vivier 

(it could be merged with previous one)

Re: [PATCH 2/3] scsi: make io_timeout configurable

[Paolo Bonzini]

On 22/09/21 17:47, Philippe Mathieu-Daudé wrote:





@@ -637,7 +639,7 @@ static int get_stream_blocksize(BlockBackend *blk)
  cmd[0] = MODE_SENSE;
  cmd[4] = sizeof(buf);
-    ret = scsi_SG_IO_FROM_DEV(blk, cmd, sizeof(cmd), buf, sizeof(buf));
+    ret = scsi_SG_IO_FROM_DEV(blk, cmd, sizeof(cmd), buf, 
sizeof(buf), 6);


Why is this timeout hardcoded? Due to the /* XXX */ comment?


This command is only invoked at startup and involves no I/O, so 6 
seconds should be plenty.


Paolo

Re: [PATCH 1/2] docs/devel/testing: add instruction to run a single acceptance test

[Daniel P . Berrangé]

On Wed, Sep 22, 2021 at 04:03:23PM -0300, Willian Rampazzo wrote:
> Add instructions to the Acceptance tests section about running a
> single test file or a test within the test file.
> 
> Signed-off-by: Willian Rampazzo 
> ---
>  docs/devel/testing.rst | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 4a0abbf23d..b03df34f7b 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -740,6 +740,20 @@ may be invoked by running:
>  
>tests/venv/bin/avocado run $OPTION1 $OPTION2 tests/acceptance/
>  
> +It is also possible to run tests from a single file or a single test
> +within a test file. To run tests from a single file within the build
> +tree, use:
> +
> + .. code::
> +
> +  tests/venv/bin/avocado run tests/acceptance/$TESTFILE

Before running this users may well need to run

   make check-venv

we can't assume they have previously done "make check-acceptance"
as they're possibly just reproducing a failure from gitlab
CI locally, not running the whole suite.

> +
> +To run a single test within a test file, use:
> +
> + .. code::
> +
> +  tests/venv/bin/avocado run tests/acceptance/$TESTFILE:$TESTCLASS.$TESTNAME
> +

Valid test names are visible in the output from any previous execution
of avocado, and can also be queried using

  tests/venv/bin/avocado list tests/acceptance

>  Manual Installation
>  ---
>  
> -- 
> 2.31.1
> 

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 :|

[PATCH v5 01/20] nubus: add comment indicating reference documents

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland 
---
 hw/nubus/nubus-bus.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 5c13452308..f4410803ff 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -8,6 +8,14 @@
  *
  */
 
+/*
+ * References:
+ *   Nubus Specification (TI)
+ * http://www.bitsavers.org/pdf/ti/nubus/2242825-0001_NuBus_Spec1983.pdf
+ *
+ *   Designing Cards and Drivers for the Macintosh Family (Apple)
+ */
+
 #include "qemu/osdep.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
-- 
2.20.1

[PATCH v5 12/20] nubus: move nubus to its own 32-bit address space

[Mark Cave-Ayland]

According to "Designing Cards and Drivers for the Macintosh Family" the Nubus
has its own 32-bit address space based upon physical slot addressing.

Move Nubus to its own 32-bit address space and then use memory region aliases
to map available slot and super slot ranges into the q800 system address
space via the Macintosh Nubus bridge.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/m68k/q800.c  |  9 -
 hw/nubus/mac-nubus-bridge.c | 16 ++--
 hw/nubus/nubus-bus.c| 18 ++
 include/hw/nubus/mac-nubus-bridge.h |  2 ++
 include/hw/nubus/nubus.h|  6 ++
 5 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 5ba87f789c..a07912b87c 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -67,9 +67,6 @@
 #define ASC_BASE  (IO_BASE + 0x14000)
 #define SWIM_BASE (IO_BASE + 0x1E000)
 
-#define NUBUS_SUPER_SLOT_BASE 0x6000
-#define NUBUS_SLOT_BASE   0xf000
-
 #define SONIC_PROM_SIZE   0x1000
 
 /*
@@ -396,8 +393,10 @@ static void q800_init(MachineState *machine)
 
 dev = qdev_new(TYPE_MAC_NUBUS_BRIDGE);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
-sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, NUBUS_SUPER_SLOT_BASE);
-sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
+MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
+MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
 nubus = MAC_NUBUS_BRIDGE(dev)->bus;
 
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 3f075789e9..3af4f5d396 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -22,8 +22,20 @@ static void mac_nubus_bridge_init(Object *obj)
 s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
   MAC_NUBUS_SLOT_NB);
 
-sysbus_init_mmio(sbd, >bus->super_slot_io);
-sysbus_init_mmio(sbd, >bus->slot_io);
+/* Aliases for slots 0x9 to 0xe */
+memory_region_init_alias(>super_slot_alias, obj, "super-slot-alias",
+ >bus->nubus_mr,
+ MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE,
+ MAC_NUBUS_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
+
+memory_region_init_alias(>slot_alias, obj, "slot-alias",
+ >bus->nubus_mr,
+ NUBUS_SLOT_BASE +
+ MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE,
+ MAC_NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
+
+sysbus_init_mmio(sbd, >super_slot_alias);
+sysbus_init_mmio(sbd, >slot_alias);
 }
 
 static void mac_nubus_bridge_class_init(ObjectClass *klass, void *data)
diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 3db8272524..40d9068c59 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -78,25 +78,42 @@ static const MemoryRegionOps nubus_super_slot_ops = {
 },
 };
 
+static void nubus_unrealize(BusState *bus)
+{
+NubusBus *nubus = NUBUS_BUS(bus);
+
+address_space_destroy(>nubus_as);
+}
+
 static void nubus_realize(BusState *bus, Error **errp)
 {
+NubusBus *nubus = NUBUS_BUS(bus);
+
 if (!nubus_find()) {
 error_setg(errp, "at most one %s device is permitted", TYPE_NUBUS_BUS);
 return;
 }
+
+address_space_init(>nubus_as, >nubus_mr, "nubus");
 }
 
 static void nubus_init(Object *obj)
 {
 NubusBus *nubus = NUBUS_BUS(obj);
 
+memory_region_init(>nubus_mr, obj, "nubus", 0x1);
+
 memory_region_init_io(>super_slot_io, obj, _super_slot_ops,
   nubus, "nubus-super-slots",
   (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE);
+memory_region_add_subregion(>nubus_mr, 0x0, >super_slot_io);
 
 memory_region_init_io(>slot_io, obj, _slot_ops,
   nubus, "nubus-slots",
   NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
+memory_region_add_subregion(>nubus_mr,
+(NUBUS_SUPER_SLOT_NB + 1) *
+NUBUS_SUPER_SLOT_SIZE, >slot_io);
 
 nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT,
  NUBUS_SLOT_NB);
@@ -151,6 +168,7 @@ static void nubus_class_init(ObjectClass *oc, void *data)
 BusClass *bc = BUS_CLASS(oc);
 
 bc->realize = nubus_realize;
+bc->unrealize = nubus_unrealize;
 bc->check_address = nubus_check_address;
 bc->get_dev_path = nubus_get_dev_path;
 }
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index 118d67267d..04451d357c 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -23,6 +23,8 @@ 

Re: [PATCH 8/8] qapi: add blockdev-replace command

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> Thanks a lot for reviewing!
>
> 20.09.2021 09:44, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>> 
>>> Add command that can add and remove filters.
>>>
>>> Key points of functionality:
>>>
>>> What the command does is simply replace some BdrvChild.bs by some other
>>> nodes. The tricky thing is selecting there BdrvChild objects.
>>> To be able to select any kind of BdrvChild we use a generic parent_id,
>>> which may be a node-name, or qdev id or block export id. In future we
>>> may support block jobs.
>>>
>>> Any kind of ambiguity leads to error. If we have both device named
>>> device0 and block export named device0 and they both point to same BDS,
>>> user can't replace root child of one of these parents. So, to be able
>>> to do replacements, user should avoid duplicating names in different
>>> parent namespaces.
>>>
>>> So, command allows to replace any single child in the graph.
>>>
>>> On the other hand we want to realize a kind of bdrv_replace_node(),
>>> which works well when we want to replace all parents of some node. For
>>> this kind of task @parents-mode argument implemented.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   qapi/block-core.json | 78 +
>>>   block.c  | 82 
>>>   2 files changed, 160 insertions(+)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 675d8265eb..8059b96341 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -5433,3 +5433,81 @@
>>>   { 'command': 'blockdev-snapshot-delete-internal-sync',
>>> 'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
>>> 'returns': 'SnapshotInfo' }
>>> +
>>> +##
>>> +# @BlockdevReplaceParentsMode:
>>> +#
>>> +# Alternative (to directly set @parent) way to chose parents in
>>> +# @blockdev-replace
>>> +#
>>> +# @exactly-one: Exactly one parent should match a condition, otherwise
>>> +#   @blockdev-replace fails.
>>> +#
>>> +# @all: All matching parents are taken into account. If replacing lead
>>> +#   to loops in block graph, @blockdev-replace fails.
>>> +#
>>> +# @auto: Same as @all, but automatically skip replacing parents if it
>>> +#leads to loops in block graph.
>>> +#
>>> +# Since: 6.2
>>> +##
>>> +{ 'enum': 'BlockdevReplaceParentsMode',
>>> +  'data': ['exactly-one', 'all', 'auto'] }
>>> +
>>> +##
>>> +# @BlockdevReplace:
>>> +#
>>> +# Declaration of one replacement.
>> 
>> Replacement of what?  A node in the block graph?
>
> A specific child node in one or in several edges

Spell that out in the doc comment, please.

>> 
>>> +#
>>> +# @parent: id of parent. It may be qdev or block export or simple
>>> +#  node-name.
>> 
>> It may also be a QOM path, because find_device_state() interprets
>> arguments starting with '/' as QOM paths.
>> 
>> When is a node name "simple"?
>> 
>> Suggest: It may be a qdev ID, a QOM path, a block export ID, or a node
>> name.
>
> OK
>
>> 
>> The trouble is of course that we're merging three separate name spaces.
>
> Yes. Alternatively we can also add an enum of node-type [bds, device, 
> export[, job]], and select graph nodes more explicit (by pair of id/path/name 
> and type)
>
> But if we have to use these things in one context, it seems good to enforce 
> users use different names for them. And in this way, we can avoid strict 
> typing.

Is there precedence in QMP for merging ID name spaces, or for selecting
a name space?

>> Aside: a single name space for IDs would be so much saner, but we
>> screwed that up long ago.

Throwing some of the multiple name spaces together some of the time
feels like another mistake.

>> 
>>> If id is ambiguous (for example node-name of
>>> +#  some BDS equals to block export name), blockdev-replace
>>> +#  fails.
>> 
>> Is there a way out of this situation, or are is replacement simply
>> impossible then?
>
> In my idea, it's simply impossible. If someone want to use this new 
> interface, he should care to use different names for different things.

Reminds me of device_del, which simply could not delete a device without
an ID.  Made many users go "oh" (or possibly a more colorful version
thereof), until daniel fixed it in commit 6287d827d4 "monitor: allow
device_del to accept QOM paths" for v2.5.

>> 
>>> If not specified, blockdev-replace goes through
>>> +#  @parents-mode scenario, see below. Note, that @parent and
>>> +#  @parents-mode can't be specified simultaneously.
>> 
>> What if neither is specified?  Hmm, @parents-mode has a default, so
>> that's what we get.
>> 
>>> +#  If @parent is specified, only one edge is selected. If
>>> +#  several edges match the condition, blockdev-replace fails.
>>> +#
>>> +# @edge: name of the child. If omitted, any child name matches.
>>> +#
>>> +# @child: node-name of 

Re: [PATCH 2/3] scsi: make io_timeout configurable

[Hannes Reinecke]

On 9/22/21 5:47 PM, Philippe Mathieu-Daudé wrote:

Hi Hannes,

On 11/16/20 19:31, Hannes Reinecke wrote:

The current code sets an infinite timeout on SG_IO requests,
causing the guest to stall if the host experiences a frame
loss.
This patch adds an 'io_timeout' parameter for SCSIDevice to
make the SG_IO timeout configurable, and also shortens the
default timeout to 30 seconds to avoid infinite stalls.

Signed-off-by: Hannes Reinecke 
---
  hw/scsi/scsi-disk.c    |  6 --
  hw/scsi/scsi-generic.c | 17 +++--
  include/hw/scsi/scsi.h |  4 +++-
  3 files changed, 18 insertions(+), 9 deletions(-)


  int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t 
cmd_size,

-    uint8_t *buf, uint8_t buf_size)
+    uint8_t *buf, uint8_t buf_size, uint32_t 
timeout)

  {
  sg_io_hdr_t io_header;
  uint8_t sensebuf[8];
@@ -520,7 +522,7 @@ int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t 
*cmd, uint8_t cmd_size,

  io_header.cmd_len = cmd_size;
  io_header.mx_sb_len = sizeof(sensebuf);
  io_header.sbp = sensebuf;
-    io_header.timeout = 6000; /* XXX */
+    io_header.timeout = timeout * 1000;



@@ -637,7 +639,7 @@ static int get_stream_blocksize(BlockBackend *blk)
  cmd[0] = MODE_SENSE;
  cmd[4] = sizeof(buf);
-    ret = scsi_SG_IO_FROM_DEV(blk, cmd, sizeof(cmd), buf, sizeof(buf));
+    ret = scsi_SG_IO_FROM_DEV(blk, cmd, sizeof(cmd), buf, 
sizeof(buf), 6);


Why is this timeout hardcoded? Due to the /* XXX */ comment?



60 seconds is the default command timeout on linux.
And the problem is that the guest might set a command timeout on the 
comands being send from the guests user space, but the guest is unable

to communicate this timeout to the host.

Really, one should fix up the virtio spec here to allow for a 'timeout' 
field. But in the absence of that being able to configure it is the next 
best attempt.


Cheers,

Hannes
--
Dr. Hannes ReineckeKernel Storage Architect
h...@suse.de  +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH v5 11/20] nubus-device: add romfile property for loading declaration ROMs

[Mark Cave-Ayland]

The declaration ROM is located at the top-most address of the standard slot
space.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-device.c  | 44 +++-
 include/hw/nubus/nubus.h |  6 ++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index d4932d64a2..280f40e88a 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -9,16 +9,21 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/datadir.h"
+#include "hw/loader.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 
 
 static void nubus_device_realize(DeviceState *dev, Error **errp)
 {
 NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev));
 NubusDevice *nd = NUBUS_DEVICE(dev);
-char *name;
+char *name, *path;
 hwaddr slot_offset;
+int64_t size;
+int ret;
 
 /* Super */
 slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
@@ -38,10 +43,47 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 memory_region_add_subregion(>slot_io, slot_offset,
 >slot_mem);
 g_free(name);
+
+/* Declaration ROM */
+if (nd->romfile != NULL) {
+path = qemu_find_file(QEMU_FILE_TYPE_BIOS, nd->romfile);
+if (path == NULL) {
+path = g_strdup(nd->romfile);
+}
+
+size = get_image_size(path);
+if (size < 0) {
+error_setg(errp, "failed to find romfile \"%s\"", nd->romfile);
+g_free(path);
+return;
+} else if (size == 0) {
+error_setg(errp, "romfile \"%s\" is empty", nd->romfile);
+g_free(path);
+return;
+} else if (size > NUBUS_DECL_ROM_MAX_SIZE) {
+error_setg(errp, "romfile \"%s\" too large (maximum size 128K)",
+   nd->romfile);
+g_free(path);
+return;
+}
+
+name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
+memory_region_init_rom(>decl_rom, OBJECT(dev), name, size,
+   _abort);
+ret = load_image_mr(path, >decl_rom);
+g_free(path);
+if (ret < 0) {
+error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
+return;
+}
+memory_region_add_subregion(>slot_mem, NUBUS_SLOT_SIZE - size,
+>decl_rom);
+}
 }
 
 static Property nubus_device_properties[] = {
 DEFINE_PROP_INT32("slot", NubusDevice, slot, -1),
+DEFINE_PROP_STRING("romfile", NubusDevice, romfile),
 DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 2e629ffcea..73a9d8cb7f 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -12,6 +12,7 @@
 #include "hw/qdev-properties.h"
 #include "exec/address-spaces.h"
 #include "qom/object.h"
+#include "qemu/units.h"
 
 #define NUBUS_SUPER_SLOT_SIZE 0x1000U
 #define NUBUS_SUPER_SLOT_NB   0xe
@@ -38,12 +39,17 @@ struct NubusBus {
 uint32_t slot_available_mask;
 };
 
+#define NUBUS_DECL_ROM_MAX_SIZE(128 * KiB)
+
 struct NubusDevice {
 DeviceState qdev;
 
 int32_t slot;
 MemoryRegion super_slot_mem;
 MemoryRegion slot_mem;
+
+char *romfile;
+MemoryRegion decl_rom;
 };
 
 #endif
-- 
2.20.1

[PATCH v5 10/20] nubus-device: remove nubus_register_rom() and nubus_register_format_block()

[Mark Cave-Ayland]

Since there is no need to generate a dummy declaration ROM, remove both
nubus_register_rom() and nubus_register_format_block(). These will shortly be
replaced with a mechanism to optionally load a declaration ROM from disk to
allow real images to be used within QEMU.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-device.c  | 143 ---
 include/hw/nubus/nubus.h |  19 --
 2 files changed, 162 deletions(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 516b13d2d5..d4932d64a2 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -13,147 +13,6 @@
 #include "qapi/error.h"
 
 
-/* The Format Block Structure */
-
-#define FBLOCK_DIRECTORY_OFFSET 0
-#define FBLOCK_LENGTH   4
-#define FBLOCK_CRC  8
-#define FBLOCK_REVISION_LEVEL   12
-#define FBLOCK_FORMAT   13
-#define FBLOCK_TEST_PATTERN 14
-#define FBLOCK_RESERVED 18
-#define FBLOCK_BYTE_LANES   19
-
-#define FBLOCK_SIZE 20
-#define FBLOCK_PATTERN_VAL  0x5a932bc7
-
-static uint64_t nubus_fblock_read(void *opaque, hwaddr addr, unsigned int size)
-{
-NubusDevice *dev = opaque;
-uint64_t val;
-
-#define BYTE(v, b) (((v) >> (24 - 8 * (b))) & 0xff)
-switch (addr) {
-case FBLOCK_BYTE_LANES:
-val = dev->byte_lanes;
-val |= (val ^ 0xf) << 4;
-break;
-case FBLOCK_RESERVED:
-val = 0x00;
-break;
-case FBLOCK_TEST_PATTERN...FBLOCK_TEST_PATTERN + 3:
-val = BYTE(FBLOCK_PATTERN_VAL, addr - FBLOCK_TEST_PATTERN);
-break;
-case FBLOCK_FORMAT:
-val = dev->rom_format;
-break;
-case FBLOCK_REVISION_LEVEL:
-val = dev->rom_rev;
-break;
-case FBLOCK_CRC...FBLOCK_CRC + 3:
-val = BYTE(dev->rom_crc, addr - FBLOCK_CRC);
-break;
-case FBLOCK_LENGTH...FBLOCK_LENGTH + 3:
-val = BYTE(dev->rom_length, addr - FBLOCK_LENGTH);
-break;
-case FBLOCK_DIRECTORY_OFFSET...FBLOCK_DIRECTORY_OFFSET + 3:
-val = BYTE(dev->directory_offset, addr - FBLOCK_DIRECTORY_OFFSET);
-break;
-default:
-val = 0;
-break;
-}
-return val;
-}
-
-static void nubus_fblock_write(void *opaque, hwaddr addr, uint64_t val,
-   unsigned int size)
-{
-/* read only */
-}
-
-static const MemoryRegionOps nubus_format_block_ops = {
-.read = nubus_fblock_read,
-.write = nubus_fblock_write,
-.endianness = DEVICE_BIG_ENDIAN,
-.valid = {
-.min_access_size = 1,
-.max_access_size = 1,
-}
-};
-
-static void nubus_register_format_block(NubusDevice *dev)
-{
-char *fblock_name;
-
-fblock_name = g_strdup_printf("nubus-slot-%d-format-block",
-  dev->slot);
-
-hwaddr fblock_offset = memory_region_size(>slot_mem) - FBLOCK_SIZE;
-memory_region_init_io(>fblock_io, NULL, _format_block_ops,
-  dev, fblock_name, FBLOCK_SIZE);
-memory_region_add_subregion(>slot_mem, fblock_offset,
->fblock_io);
-
-g_free(fblock_name);
-}
-
-static void mac_nubus_rom_write(void *opaque, hwaddr addr, uint64_t val,
-   unsigned int size)
-{
-/* read only */
-}
-
-static uint64_t mac_nubus_rom_read(void *opaque, hwaddr addr,
-unsigned int size)
-{
-NubusDevice *dev = opaque;
-
-return dev->rom[addr];
-}
-
-static const MemoryRegionOps mac_nubus_rom_ops = {
-.read  = mac_nubus_rom_read,
-.write = mac_nubus_rom_write,
-.endianness = DEVICE_BIG_ENDIAN,
-.valid = {
-.min_access_size = 1,
-.max_access_size = 1,
-},
-};
-
-
-void nubus_register_rom(NubusDevice *dev, const uint8_t *rom, uint32_t size,
-int revision, int format, uint8_t byte_lanes)
-{
-hwaddr rom_offset;
-char *rom_name;
-
-/* FIXME : really compute CRC */
-dev->rom_length = 0;
-dev->rom_crc = 0;
-
-dev->rom_rev = revision;
-dev->rom_format = format;
-
-dev->byte_lanes = byte_lanes;
-dev->directory_offset = -size;
-
-/* ROM */
-
-dev->rom = rom;
-rom_name = g_strdup_printf("nubus-slot-%d-rom", dev->slot);
-memory_region_init_io(>rom_io, NULL, _nubus_rom_ops,
-  dev, rom_name, size);
-memory_region_set_readonly(>rom_io, true);
-
-rom_offset = memory_region_size(>slot_mem) - FBLOCK_SIZE +
- dev->directory_offset;
-memory_region_add_subregion(>slot_mem, rom_offset, >rom_io);
-
-g_free(rom_name);
-}
-
 static void nubus_device_realize(DeviceState *dev, Error **errp)
 {
 NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev));
@@ -179,8 +38,6 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 memory_region_add_subregion(>slot_io, slot_offset,
 >slot_mem);
 

[PATCH v5 08/20] nubus: generate bus error when attempting to access empty slots

[Mark Cave-Ayland]

According to "Designing Cards and Drivers for the Macintosh Family" any attempt
to access an unimplemented address location on Nubus generates a bus error. 
MacOS
uses a custom bus error handler to detect empty Nubus slots, and with the 
current
implementation assumes that all slots are occupied as the Nubus transactions
never fail.

Switch nubus_slot_ops and nubus_super_slot_ops over to use 
{read,write}_with_attrs
and hard-code them to return MEMTX_DECODE_ERROR so that unoccupied Nubus slots
will generate the expected bus error.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c | 34 ++
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index b398423c99..3db8272524 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -28,23 +28,23 @@ static NubusBus *nubus_find(void)
 return NUBUS_BUS(object_resolve_path_type("", TYPE_NUBUS_BUS, NULL));
 }
 
-static void nubus_slot_write(void *opaque, hwaddr addr, uint64_t val,
- unsigned int size)
+static MemTxResult nubus_slot_write(void *opaque, hwaddr addr, uint64_t val,
+unsigned size, MemTxAttrs attrs)
 {
-/* read only */
 trace_nubus_slot_write(addr, val, size);
+return MEMTX_DECODE_ERROR;
 }
 
-static uint64_t nubus_slot_read(void *opaque, hwaddr addr,
-unsigned int size)
+static MemTxResult nubus_slot_read(void *opaque, hwaddr addr, uint64_t *data,
+   unsigned size, MemTxAttrs attrs)
 {
 trace_nubus_slot_read(addr, size);
-return 0;
+return MEMTX_DECODE_ERROR;
 }
 
 static const MemoryRegionOps nubus_slot_ops = {
-.read  = nubus_slot_read,
-.write = nubus_slot_write,
+.read_with_attrs  = nubus_slot_read,
+.write_with_attrs = nubus_slot_write,
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
@@ -52,23 +52,25 @@ static const MemoryRegionOps nubus_slot_ops = {
 },
 };
 
-static void nubus_super_slot_write(void *opaque, hwaddr addr, uint64_t val,
-   unsigned int size)
+static MemTxResult nubus_super_slot_write(void *opaque, hwaddr addr,
+  uint64_t val, unsigned size,
+  MemTxAttrs attrs)
 {
-/* read only */
 trace_nubus_super_slot_write(addr, val, size);
+return MEMTX_DECODE_ERROR;
 }
 
-static uint64_t nubus_super_slot_read(void *opaque, hwaddr addr,
-  unsigned int size)
+static MemTxResult nubus_super_slot_read(void *opaque, hwaddr addr,
+ uint64_t *data, unsigned size,
+ MemTxAttrs attrs)
 {
 trace_nubus_super_slot_read(addr, size);
-return 0;
+return MEMTX_DECODE_ERROR;
 }
 
 static const MemoryRegionOps nubus_super_slot_ops = {
-.read  = nubus_super_slot_read,
-.write = nubus_super_slot_write,
+.read_with_attrs = nubus_super_slot_read,
+.write_with_attrs = nubus_super_slot_write,
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
-- 
2.20.1

[PATCH v5 03/20] nubus-device: expose separate super slot memory region

[Mark Cave-Ayland]

According to "Designing Cards and Drivers for the Macintosh Family" each 
physical
nubus slot can access 2 separate address ranges: a super slot memory region 
which
is 256MB and a standard slot memory region which is 16MB.

Currently a Nubus device uses the physical slot number to determine whether it 
is
using a standard slot memory region or a super slot memory region rather than
exposing both memory regions for use as required.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/nubus/nubus-device.c  | 36 ++--
 include/hw/nubus/nubus.h |  1 +
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index be01269563..4e23df1280 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -168,26 +168,26 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 }
 
 nd->slot = nubus->current_slot++;
-name = g_strdup_printf("nubus-slot-%d", nd->slot);
-
-if (nd->slot < NUBUS_FIRST_SLOT) {
-/* Super */
-slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE;
-
-memory_region_init(>slot_mem, OBJECT(dev), name,
-   NUBUS_SUPER_SLOT_SIZE);
-memory_region_add_subregion(>super_slot_io, slot_offset,
->slot_mem);
-} else {
-/* Normal */
-slot_offset = nd->slot * NUBUS_SLOT_SIZE;
-
-memory_region_init(>slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE);
-memory_region_add_subregion(>slot_io, slot_offset,
->slot_mem);
-}
 
+/* Super */
+slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
+
+name = g_strdup_printf("nubus-super-slot-%x", nd->slot);
+memory_region_init(>super_slot_mem, OBJECT(dev), name,
+   NUBUS_SUPER_SLOT_SIZE);
+memory_region_add_subregion(>super_slot_io, slot_offset,
+>super_slot_mem);
+g_free(name);
+
+/* Normal */
+slot_offset = nd->slot * NUBUS_SLOT_SIZE;
+
+name = g_strdup_printf("nubus-slot-%x", nd->slot);
+memory_region_init(>slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE);
+memory_region_add_subregion(>slot_io, slot_offset,
+>slot_mem);
 g_free(name);
+
 nubus_register_format_block(nd);
 }
 
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 424309dd73..89b0976aaa 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -43,6 +43,7 @@ struct NubusDevice {
 DeviceState qdev;
 
 int slot;
+MemoryRegion super_slot_mem;
 MemoryRegion slot_mem;
 
 /* Format Block */
-- 
2.20.1

Re: [PATCH 1/2] tests/qapi-schema: Use Python OSError instead of outmoded IOError

[Markus Armbruster]

John Snow  writes:

> On Wed, Sep 22, 2021 at 8:56 AM Markus Armbruster  wrote:
>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  tests/qapi-schema/test-qapi.py | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qapi-schema/test-qapi.py
>> b/tests/qapi-schema/test-qapi.py
>> index 73cffae2b6..2e384f5efd 100755
>> --- a/tests/qapi-schema/test-qapi.py
>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -154,7 +154,7 @@ def test_and_diff(test_name, dir_name, update):
>>  errfp = open(os.path.join(dir_name, test_name + '.err'), mode)
>>  expected_out = outfp.readlines()
>>  expected_err = errfp.readlines()
>> -except IOError as err:
>> +except OSError as err:
>>  print("%s: can't open '%s': %s"
>>% (sys.argv[0], err.filename, err.strerror),
>>file=sys.stderr)
>> @@ -180,7 +180,7 @@ def test_and_diff(test_name, dir_name, update):
>>  errfp.truncate(0)
>>  errfp.seek(0)
>>  errfp.writelines(actual_err)
>> -except IOError as err:
>> +except OSError as err:
>>  print("%s: can't write '%s': %s"
>>% (sys.argv[0], err.filename, err.strerror),
>>file=sys.stderr)
>> --
>> 2.31.1
>>
>>
> If you're happy with the expanded scope of the exception-catcher, I am too.

https://docs.python.org/3.6/library/exceptions.html has

Changed in version 3.3: EnvironmentError, IOError, WindowsError,
socket.error, select.error and mmap.error have been merged into
OSError, and the constructor may return a subclass.

and

The following exceptions are kept for compatibility with previous
versions; starting from Python 3.3, they are aliases of OSError.

exception EnvironmentError

exception IOError

exception WindowsError

Only available on Windows.

So unless I'm misunderstanding something (which is quite possible),
we're catching exactly the same exceptions as before, we just switch to
their preferred name.

> Reviewed-by: John Snow 

Thanks!

Re: [PATCH] docs/nvdimm: Update nvdimm option value in machine example

[Pankaj Gupta]

> > ping
>
> Could you post your patch with an email address ("From") that is the same as 
> the "Signed-off-by"?

Sure sent a V2.

Thank you,
Pankaj

>
> https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line
>
> Thanks,
> Laurent
>
> >> Update nvdimm option value in example command from "-machine pc,nvdimm"
> >> to "-machine pc,nvdimm=on" as former complains with the below error:
> >>
> >> "qemu-system-x86_64: -machine pc,nvdimm: Expected '=' after parameter 
> >> 'nvdimm'"
> >>
> >> Signed-off-by: Pankaj Gupta 
> >> ---
> >>  docs/nvdimm.txt | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> >> index 0aae682be3..fd7773dc5a 100644
> >> --- a/docs/nvdimm.txt
> >> +++ b/docs/nvdimm.txt
> >> @@ -15,7 +15,7 @@ backend (i.e. memory-backend-file and 
> >> memory-backend-ram). A simple
> >>  way to create a vNVDIMM device at startup time is done via the
> >>  following command line options:
> >>
> >> - -machine pc,nvdimm
> >> + -machine pc,nvdimm=on
> >>   -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
> >>   -object 
> >> memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
> >>   -device nvdimm,id=nvdimm1,memdev=mem1,unarmed=off
> >> --
> >> 2.25.1
> >>
> >
>

Re: [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv

[Philippe Mathieu-Daudé]

On 9/23/21 02:16, John Snow wrote:

We can drop the sys.path hacking in various places by doing
this. Additionally, by doing it in one place right up top, we can print
interesting warnings in case the environment does not look correct.

If we ever decide to change how the environment is crafted, all of the
"help me find my python packages" goop is all in one place, right in one
function.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/235|  2 --
  tests/qemu-iotests/297|  6 --
  tests/qemu-iotests/300|  7 +++
  tests/qemu-iotests/iotests.py |  2 --
  tests/qemu-iotests/testenv.py | 14 +-
  tests/qemu-iotests/tests/mirror-top-perms |  7 +++
  6 files changed, 15 insertions(+), 23 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 15/20] nubus: move NubusBus from mac-nubus-bridge to nubus-bridge

[Laurent Vivier]

Le 23/09/2021 à 11:13, Mark Cave-Ayland a écrit :
> Now that Nubus has its own address space rather than mapping directly into the
> system bus, move the Nubus reference from MacNubusBridge to NubusBridge.
> 
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/m68k/q800.c  |  2 +-
>  hw/nubus/mac-nubus-bridge.c | 11 +--
>  hw/nubus/nubus-bridge.c |  9 +
>  include/hw/nubus/mac-nubus-bridge.h |  1 -
>  include/hw/nubus/nubus.h|  2 ++
>  5 files changed, 17 insertions(+), 8 deletions(-)
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH 3/6] iotests/linters: check mypy files all at once

[Philippe Mathieu-Daudé]

On 9/23/21 02:16, John Snow wrote:

We can circumvent the '__main__' redefinition problem by passing
--scripts-are-modules. Take mypy out of the loop per-filename and check
everything in one go: it's quite a bit faster.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
  tests/qemu-iotests/297 | 44 +++---
  1 file changed, 20 insertions(+), 24 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Paolo Bonzini]

On 23/09/21 12:44, Peter Maydell wrote:

On Thu, 23 Sept 2021 at 11:32, Paolo Bonzini  wrote:


On 23/09/21 12:00, Philippe Mathieu-Daudé wrote:

See also "blobs: Only install required (system emulation) files":
https://lore.kernel.org/qemu-devel/20210323155132.238193-1-f4...@amsat.org/


Nice and makes a lot of sense.  Regarding "there is probably a nicer way
to do this with Meson", I would do without all the variables and do
something like

foreach target : target_dirs
if target in ['...']
  blobs_ss.add('...')
elif target in ['...']
  blobs_ss.add('...')
endif
endforeach

directly in pc-bios/meson.build.


Is it possible also to have meson handle the "symlink the blob from
the source dir to the build dir" which currently configure is doing ?


Yes, though I would have to check the details on how to do that best 
(for example whether to keep them at configure time or move them to make).


By the way, I think a lot of the DIRS in configure are not needed 
anymore; meson would create them anyway and they're not needed by the 
LINKS loop below.



That would mean we could avoid having effectively two lists of blobs.
(Somebody would need to cross-check that all the blobs the wildcards in
configure are linking in are listed in the meson list.) I guess
the question is whether other parts of the build system assume those
links already exist (ie they don't explicitly declare a dependency
on the existence of the blob and would need to change to do so).


Yeah, that's also why in my patch I didn't bother adding the roms 
dependency to bios-tables-test only.  It's less prone to failure if 
they're just built before any qtest is run.


Paolo

Re: [PATCH v3 24/35] acpi: x86: madt: use build_append_int_noprefix() API to compose MADT table

[Igor Mammedov]

On Wed, 22 Sep 2021 17:37:40 +0200
Eric Auger  wrote:

> Hi,
> On 9/22/21 5:30 PM, Igor Mammedov wrote:
> > On Wed, 22 Sep 2021 12:20:54 +0200
> > Eric Auger  wrote:
> >   
> >> On 9/7/21 4:48 PM, Igor Mammedov wrote:  
> >>> Drop usage of packed structures and explicit endian conversions
> >>> when building MADT table for arm/x86 and use endian agnostic
> >>> build_append_int_noprefix() API to build it.
> >>>
> >>> Signed-off-by: Igor Mammedov 
> >>> ---
> >>> CC: marcel.apfelb...@gmail.com
> >>> CC: eau...@redhat.com
> >>> ---
> >>>  include/hw/acpi/acpi-defs.h |  64 --
> >>>  hw/i386/acpi-common.c   | 127 ++--
> >>>  2 files changed, 65 insertions(+), 126 deletions(-)
> >>>
> >>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> >>> index af4fa412a5..3f174ba208 100644
> >>> --- a/include/hw/acpi/acpi-defs.h
> >>> +++ b/include/hw/acpi/acpi-defs.h
> >>> @@ -165,17 +165,6 @@ typedef struct AcpiFacsDescriptorRev1 
> >>> AcpiFacsDescriptorRev1;
> >>>  
> >>>  /* Values for Type in APIC sub-headers */
> >>>  
> >>> -#define ACPI_APIC_PROCESSOR  0
> >>> -#define ACPI_APIC_IO 1
> >>> -#define ACPI_APIC_XRUPT_OVERRIDE 2
> >>> -#define ACPI_APIC_NMI3
> >>> -#define ACPI_APIC_LOCAL_NMI  4
> >>> -#define ACPI_APIC_ADDRESS_OVERRIDE   5
> >>> -#define ACPI_APIC_IO_SAPIC   6
> >>> -#define ACPI_APIC_LOCAL_SAPIC7
> >>> -#define ACPI_APIC_XRUPT_SOURCE   8
> >>> -#define ACPI_APIC_LOCAL_X2APIC   9
> >>> -#define ACPI_APIC_LOCAL_X2APIC_NMI  10
> >>>  #define ACPI_APIC_GENERIC_CPU_INTERFACE 11
> >>>  #define ACPI_APIC_GENERIC_DISTRIBUTOR   12
> >>>  #define ACPI_APIC_GENERIC_MSI_FRAME 13
> >>> @@ -192,59 +181,6 @@ typedef struct AcpiFacsDescriptorRev1 
> >>> AcpiFacsDescriptorRev1;
> >>>  
> >>>  /* Sub-structures for MADT */
> >>>  
> >>> -struct AcpiMadtProcessorApic {
> >>> -ACPI_SUB_HEADER_DEF
> >>> -uint8_t  processor_id;   /* ACPI processor id */
> >>> -uint8_t  local_apic_id;  /* Processor's local APIC id */
> >>> -uint32_t flags;
> >>> -} QEMU_PACKED;
> >>> -typedef struct AcpiMadtProcessorApic AcpiMadtProcessorApic;
> >>> -
> >>> -struct AcpiMadtIoApic {
> >>> -ACPI_SUB_HEADER_DEF
> >>> -uint8_t  io_apic_id; /* I/O APIC ID */
> >>> -uint8_t  reserved;   /* Reserved - must be zero */
> >>> -uint32_t address;/* APIC physical address */
> >>> -uint32_t interrupt;  /* Global system interrupt where 
> >>> INTI
> >>> - * lines start */
> >>> -} QEMU_PACKED;
> >>> -typedef struct AcpiMadtIoApic AcpiMadtIoApic;
> >>> -
> >>> -struct AcpiMadtIntsrcovr {
> >>> -ACPI_SUB_HEADER_DEF
> >>> -uint8_t  bus;
> >>> -uint8_t  source;
> >>> -uint32_t gsi;
> >>> -uint16_t flags;
> >>> -} QEMU_PACKED;
> >>> -typedef struct AcpiMadtIntsrcovr AcpiMadtIntsrcovr;
> >>> -
> >>> -struct AcpiMadtLocalNmi {
> >>> -ACPI_SUB_HEADER_DEF
> >>> -uint8_t  processor_id;   /* ACPI processor id */
> >>> -uint16_t flags;  /* MPS INTI flags */
> >>> -uint8_t  lint;   /* Local APIC LINT# */
> >>> -} QEMU_PACKED;
> >>> -typedef struct AcpiMadtLocalNmi AcpiMadtLocalNmi;
> >>> -
> >>> -struct AcpiMadtProcessorX2Apic {
> >>> -ACPI_SUB_HEADER_DEF
> >>> -uint16_t reserved;
> >>> -uint32_t x2apic_id;  /* Processor's local x2APIC ID */
> >>> -uint32_t flags;
> >>> -uint32_t uid;/* Processor object _UID */
> >>> -} QEMU_PACKED;
> >>> -typedef struct AcpiMadtProcessorX2Apic AcpiMadtProcessorX2Apic;
> >>> -
> >>> -struct AcpiMadtLocalX2ApicNmi {
> >>> -ACPI_SUB_HEADER_DEF
> >>> -uint16_t flags;  /* MPS INTI flags */
> >>> -uint32_t uid;/* Processor object _UID */
> >>> -uint8_t  lint;   /* Local APIC LINT# */
> >>> -uint8_t  reserved[3];/* Local APIC LINT# */
> >>> -} QEMU_PACKED;
> >>> -typedef struct AcpiMadtLocalX2ApicNmi AcpiMadtLocalX2ApicNmi;
> >>> -
> >>>  struct AcpiMadtGenericCpuInterface {
> >>>  ACPI_SUB_HEADER_DEF
> >>>  uint16_t reserved;
> >>> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> >>> index 7983a13a93..aa7b5c357e 100644
> >>> --- a/hw/i386/acpi-common.c
> >>> +++ b/hw/i386/acpi-common.c
> >>> @@ -38,7 +38,9 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> >>> bool force_enabled)
> >>>  {
> >>>  uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> >>> -uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ? 
> >>> 1 : 0;
> >>> +/* Flags – Local APIC Flags */
> >> nit: move that comment at the place of the build_append_int_noprefix  
> > 
> > that place(s) already has comment in expected format, here is just reminder
> > about what is initialized here.
> >   
> >>> +   

[PATCH v5 13/20] nubus-bridge: introduce separate NubusBridge structure

[Mark Cave-Ayland]

This is to allow the Nubus bridge to store its own additional state. Also update
the comment in the file header to reflect that nubus-bridge is not specific to
the Macintosh.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/nubus/nubus-bridge.c | 4 ++--
 include/hw/nubus/mac-nubus-bridge.h | 2 +-
 include/hw/nubus/nubus.h| 6 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index cd8c6a91eb..95662568c5 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -1,5 +1,5 @@
 /*
- * QEMU Macintosh Nubus
+ * QEMU Nubus
  *
  * Copyright (c) 2013-2018 Laurent Vivier 
  *
@@ -22,7 +22,7 @@ static void nubus_bridge_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo nubus_bridge_info = {
 .name  = TYPE_NUBUS_BRIDGE,
 .parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(SysBusDevice),
+.instance_size = sizeof(NubusBridge),
 .class_init= nubus_bridge_class_init,
 };
 
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index 04451d357c..fa454f5fbe 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -20,7 +20,7 @@
 OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE)
 
 struct MacNubusState {
-SysBusDevice sysbus_dev;
+NubusBridge parent_obj;
 
 NubusBus *bus;
 MemoryRegion super_slot_alias;
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index f2c8c22c05..43bde7dd69 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -10,6 +10,7 @@
 #define HW_NUBUS_NUBUS_H
 
 #include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "qom/object.h"
 #include "qemu/units.h"
@@ -32,6 +33,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE)
 OBJECT_DECLARE_SIMPLE_TYPE(NubusBus, NUBUS_BUS)
 
 #define TYPE_NUBUS_BRIDGE "nubus-bridge"
+OBJECT_DECLARE_SIMPLE_TYPE(NubusBridge, NUBUS_BRIDGE);
 
 struct NubusBus {
 BusState qbus;
@@ -58,4 +60,8 @@ struct NubusDevice {
 MemoryRegion decl_rom;
 };
 
+struct NubusBridge {
+SysBusDevice parent_obj;
+};
+
 #endif
-- 
2.20.1

[PATCH v5 14/20] mac-nubus-bridge: rename MacNubusState to MacNubusBridge

[Mark Cave-Ayland]

This better reflects that the mac-nubus-bridge device is derived from the
nubus-bridge device, and that the structure represents the state of the bridge
device and not the Nubus itself. Also update the comment in the file header to
reflect that mac-nubus-bridge is specific to the Macintosh.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/nubus/mac-nubus-bridge.c | 8 +---
 include/hw/nubus/mac-nubus-bridge.h | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 3af4f5d396..e241c581b5 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -1,5 +1,7 @@
 /*
- *  Copyright (c) 2013-2018 Laurent Vivier 
+ * QEMU Macintosh Nubus
+ *
+ * Copyright (c) 2013-2018 Laurent Vivier 
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -13,7 +15,7 @@
 
 static void mac_nubus_bridge_init(Object *obj)
 {
-MacNubusState *s = MAC_NUBUS_BRIDGE(obj);
+MacNubusBridge *s = MAC_NUBUS_BRIDGE(obj);
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
 s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
@@ -49,7 +51,7 @@ static const TypeInfo mac_nubus_bridge_info = {
 .name  = TYPE_MAC_NUBUS_BRIDGE,
 .parent= TYPE_NUBUS_BRIDGE,
 .instance_init = mac_nubus_bridge_init,
-.instance_size = sizeof(MacNubusState),
+.instance_size = sizeof(MacNubusBridge),
 .class_init= mac_nubus_bridge_class_init,
 };
 
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index fa454f5fbe..b595e1b7ef 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -17,9 +17,9 @@
 #define MAC_NUBUS_SLOT_NB(MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1)
 
 #define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge"
-OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE)
+OBJECT_DECLARE_SIMPLE_TYPE(MacNubusBridge, MAC_NUBUS_BRIDGE)
 
-struct MacNubusState {
+struct MacNubusBridge {
 NubusBridge parent_obj;
 
 NubusBus *bus;
-- 
2.20.1

[PATCH v5 16/20] nubus-bridge: embed the NubusBus object directly within nubus-bridge

[Mark Cave-Ayland]

Since nubus-bridge is a container for NubusBus then it should be embedded
directly within the bridge device using qbus_create_inplace().

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/m68k/q800.c  | 2 +-
 hw/nubus/mac-nubus-bridge.c | 9 +
 hw/nubus/nubus-bridge.c | 3 ++-
 include/hw/nubus/nubus.h| 2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 9bdea1a362..074acf4fdc 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -398,7 +398,7 @@ static void q800_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
 MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
-nubus = NUBUS_BRIDGE(dev)->bus;
+nubus = _BRIDGE(dev)->bus;
 
 /* framebuffer in nubus slot #9 */
 
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index db8640eed2..a0da5a8b2f 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -18,19 +18,20 @@ static void mac_nubus_bridge_init(Object *obj)
 MacNubusBridge *s = MAC_NUBUS_BRIDGE(obj);
 NubusBridge *nb = NUBUS_BRIDGE(obj);
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+NubusBus *bus = >bus;
 
 /* Macintosh only has slots 0x9 to 0xe available */
-nb->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
-   MAC_NUBUS_SLOT_NB);
+bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
+   MAC_NUBUS_SLOT_NB);
 
 /* Aliases for slots 0x9 to 0xe */
 memory_region_init_alias(>super_slot_alias, obj, "super-slot-alias",
- >bus->nubus_mr,
+ >nubus_mr,
  MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
 
 memory_region_init_alias(>slot_alias, obj, "slot-alias",
- >bus->nubus_mr,
+ >nubus_mr,
  NUBUS_SLOT_BASE +
  MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 3b68d4435c..1adda7f5a6 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -16,8 +16,9 @@
 static void nubus_bridge_init(Object *obj)
 {
 NubusBridge *s = NUBUS_BRIDGE(obj);
+NubusBus *bus = >bus;
 
-s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
+qbus_create_inplace(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL);
 }
 
 static void nubus_bridge_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 607cdef968..3620247be2 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -63,7 +63,7 @@ struct NubusDevice {
 struct NubusBridge {
 SysBusDevice parent_obj;
 
-NubusBus *bus;
+NubusBus bus;
 };
 
 #endif
-- 
2.20.1

[PATCH v5 18/20] nubus: add support for slot IRQs

[Mark Cave-Ayland]

Each Nubus slot has an IRQ line that can be used to request service from the
CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using 
qdev
gpios accordingly, and introduce a new nubus_set_irq() function that can be used
by Nubus devices to control the slot IRQ.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bridge.c  | 2 ++
 hw/nubus/nubus-device.c  | 8 
 include/hw/nubus/nubus.h | 6 ++
 3 files changed, 16 insertions(+)

diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 2c7c4ee121..0366d925a9 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -19,6 +19,8 @@ static void nubus_bridge_init(Object *obj)
 NubusBus *bus = >bus;
 
 qbus_create_inplace(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL);
+
+qdev_init_gpio_out(DEVICE(s), bus->irqs, NUBUS_IRQS);
 }
 
 static Property nubus_bridge_properties[] = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 280f40e88a..0f1852f671 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -10,12 +10,20 @@
 
 #include "qemu/osdep.h"
 #include "qemu/datadir.h"
+#include "hw/irq.h"
 #include "hw/loader.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 
 
+void nubus_set_irq(NubusDevice *nd, int level)
+{
+NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd)));
+
+qemu_set_irq(nubus->irqs[nd->slot], level);
+}
+
 static void nubus_device_realize(DeviceState *dev, Error **errp)
 {
 NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev));
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 3620247be2..65d7b078b8 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -26,6 +26,8 @@
 #define NUBUS_LAST_SLOT   0xf
 #define NUBUS_SLOT_NB (NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1)
 
+#define NUBUS_IRQS16
+
 #define TYPE_NUBUS_DEVICE "nubus-device"
 OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE)
 
@@ -45,6 +47,8 @@ struct NubusBus {
 MemoryRegion slot_io;
 
 uint32_t slot_available_mask;
+
+qemu_irq irqs[NUBUS_IRQS];
 };
 
 #define NUBUS_DECL_ROM_MAX_SIZE(128 * KiB)
@@ -60,6 +64,8 @@ struct NubusDevice {
 MemoryRegion decl_rom;
 };
 
+void nubus_set_irq(NubusDevice *nd, int level);
+
 struct NubusBridge {
 SysBusDevice parent_obj;
 
-- 
2.20.1

Re: [RFC v7] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Wed, Sep 22, 2021 at 10:36:24AM -0700, Jiang Wang . wrote:

On Wed, Sep 22, 2021 at 2:23 AM Stefano Garzarella  wrote:


On Wed, Sep 22, 2021 at 12:00:24AM +, Jiang Wang wrote:
>Datagram sockets are connectionless and unreliable.
>The sender does not know the capacity of the receiver
>and may send more packets than the receiver can handle.
>
>Add two more dedicate virtqueues for datagram sockets,
>so that it will not unfairly steal resources from
>stream and future connection-oriented sockets.
>
>The two new virtqueues are enabled by default and will
>be removed if the guest does not support. This will help
>migration work.
>
>btw: enable_dgram argument in vhost_vsock_common_realize
>is redundant for now, but will be used later when we
>want to disable DGRAM feature bit for old versions.
>
>Signed-off-by: Jiang Wang 
>---
>v1 -> v2: use qemu cmd option to control number of queues,
>removed configuration settings for dgram.
>v2 -> v3: use ioctl to get features and decide number of
>virt queues, instead of qemu cmd option.
>v3 -> v4: change DGRAM feature bit value to 2. Add an argument
>in vhost_vsock_common_realize to indicate dgram is supported or not.
>v4 -> v5: don't open dev to get vhostfd. Removed leftover definition of
>enable_dgram
>v5 -> v6: fix style errors. Imporve error handling of
>vhost_vsock_dgram_supported. Rename MAX_VQS_WITH_DGRAM and another one.
>v6 -> v7: Always enable dgram for vhost-user and vhost kernel.
>Delete unused virtqueues at the beginning of
>vhost_vsock_common_start for migration. Otherwise, migration will fail.
>
> hw/virtio/vhost-user-vsock.c  |  2 +-
> hw/virtio/vhost-vsock-common.c| 32 +--
> hw/virtio/vhost-vsock.c   |  6 +++-
> include/hw/virtio/vhost-vsock-common.h|  6 ++--
> include/hw/virtio/vhost-vsock.h   |  3 ++
> include/standard-headers/linux/virtio_vsock.h |  1 +
> 6 files changed, 43 insertions(+), 7 deletions(-)
>
>diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>index 6095ed7349..9823a2f3bd 100644
>--- a/hw/virtio/vhost-user-vsock.c
>+++ b/hw/virtio/vhost-user-vsock.c
>@@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev, Error 
**errp)
> return;
> }
>
>-vhost_vsock_common_realize(vdev, "vhost-user-vsock");
>+vhost_vsock_common_realize(vdev, "vhost-user-vsock", true);
>
> vhost_dev_set_config_notifier(>vhost_dev, _ops);
>
>diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>index 4ad6e234ad..7d89b4d242 100644
>--- a/hw/virtio/vhost-vsock-common.c
>+++ b/hw/virtio/vhost-vsock-common.c
>@@ -26,6 +26,18 @@ int vhost_vsock_common_start(VirtIODevice *vdev)
> int ret;
> int i;
>
>+if (!virtio_has_feature(vdev->guest_features, VIRTIO_VSOCK_F_DGRAM)) {
>+struct vhost_virtqueue *vqs;
>+virtio_delete_queue(vvc->dgram_recv_vq);
>+virtio_delete_queue(vvc->dgram_trans_vq);
>+

Are you sure it works removing queues here?
The event_queue would always be #4, but the guest will use #2 which
we're removing.


Yes, this works. In fact, I should include this in v6 and my tests done
in my previous emails have these changes. But before I submitted the patch,
I thought this code was redundant and removed it without further testing.


Just tried on an host that doesn't support F_DGRAM and I have the 
following errors:

qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=6: vhost_set_vring_call 
failed: No buffer space available (105)
qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=6: Failed to initialize 
virtqueue 2: No buffer space available

I thinks we should re-add the feature discovery before call 
vhost_dev_init().




To explain it, I think the event queue number does not matter for the
vhost and qemu. The vhost-vsock kernel module does not allocate any
data structure for the event queue.  Qemu also only allocates an array of
size 2 or 4 for the tx, rx queues. The event queue is handled separately.

The event queue number only shows up in the spec, but in real code, I don't
see anywhere that the event queue number is used explicitly or implicitly.
The Event queue looks independent of tx, rx queues.


Yep, it is used in the linux driver. Look at 
virtio_transport_event_work(), it uses VSOCK_VQ_EVENT (2).


The main test to do is to migrate a guest with active connections that 
doesn't support F_DGRAM on an host that support it and check if, after 
the migration, the connections are reset and the CID updated.


I think is not working now.




>+vqs = vvc->vhost_dev.vqs;
>+vvc->vhost_dev.nvqs = MAX_VQS_WITHOUT_DGRAM;
>+vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue,
>+   vvc->vhost_dev.nvqs);
>+g_free(vqs);
>+}
>+
> if (!k->set_guest_notifiers) {
> error_report("binding does not support guest notifiers");
>

Re: [PATCH v5 05/20] nubus: move slot bitmap checks from NubusDevice realize() to BusClass check_address()

[Philippe Mathieu-Daudé]

On 9/23/21 11:12, Mark Cave-Ayland wrote:

Allow Nubus to manage the slot allocations itself using the BusClass 
check_address()
virtual function rather than managing this during NubusDevice realize().

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bus.c| 30 ++
  hw/nubus/nubus-device.c | 22 --
  2 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 3cd7534864..d4daaa36f2 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -96,11 +96,41 @@ static void nubus_init(Object *obj)
   NUBUS_SLOT_NB);
  }
  
+static bool nubus_check_address(BusState *bus, DeviceState *dev, Error **errp)

+{
+NubusDevice *nd = NUBUS_DEVICE(dev);
+NubusBus *nubus = NUBUS_BUS(bus);
+uint16_t s;
+
+if (nd->slot == -1) {
+/* No slot specified, find first available free slot */
+s = ctz32(nubus->slot_available_mask);


Same comment than previous patch:

   int s = ctz32(nubus->slot_available_mask);

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 


+if (s != 32) {
+nd->slot = s;
+} else {
+error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+return false;
+}
+} else {
+/* Slot specified, make sure the slot is available */
+if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+return false;
+}
+}
+
+nubus->slot_available_mask &= ~BIT(nd->slot);
+return true;
+}

Re: [PATCH] docs/nvdimm: Update nvdimm option value in machine example

[Laurent Vivier]

Le 26/07/2021 à 09:11, Pankaj Gupta a écrit :
> Update nvdimm option value in example command from "-machine pc,nvdimm"
> to "-machine pc,nvdimm=on" as former complains with the below error:
> 
> "qemu-system-x86_64: -machine pc,nvdimm: Expected '=' after parameter 
> 'nvdimm'"
> 
> Signed-off-by: Pankaj Gupta 
> ---
>  docs/nvdimm.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index 0aae682be3..fd7773dc5a 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -15,7 +15,7 @@ backend (i.e. memory-backend-file and memory-backend-ram). 
> A simple
>  way to create a vNVDIMM device at startup time is done via the
>  following command line options:
>  
> - -machine pc,nvdimm
> + -machine pc,nvdimm=on
>   -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
>   -object 
> memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
>   -device nvdimm,id=nvdimm1,memdev=mem1,unarmed=off
> 

Reviewed-by: Laurent Vivier 

cc: qemu-trivial

Re: [PATCH] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Philippe Mathieu-Daudé]

On 9/23/21 07:16, Bin Meng wrote:> On Sun, Sep 19, 2021 at 2:07 AM 
Philippe Mathieu-Daudé  wrote:


- Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
- Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
- Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
- Keep mchp_pfsoc_mmuart_create() behavior


Thanks for taking care of the updates!



Signed-off-by: Philippe Mathieu-Daudé 
---
  include/hw/char/mchp_pfsoc_mmuart.h | 16 --
  hw/char/mchp_pfsoc_mmuart.c | 77 +++--
  2 files changed, 73 insertions(+), 20 deletions(-)

diff --git a/include/hw/char/mchp_pfsoc_mmuart.h 
b/include/hw/char/mchp_pfsoc_mmuart.h
index f61990215f0..b484b7ea5e4 100644
--- a/include/hw/char/mchp_pfsoc_mmuart.h
+++ b/include/hw/char/mchp_pfsoc_mmuart.h
@@ -28,16 +28,22 @@
  #ifndef HW_MCHP_PFSOC_MMUART_H
  #define HW_MCHP_PFSOC_MMUART_H

+#include "hw/sysbus.h"
  #include "hw/char/serial.h"

  #define MCHP_PFSOC_MMUART_REG_SIZE  52

-typedef struct MchpPfSoCMMUartState {
-MemoryRegion iomem;
-hwaddr base;
-qemu_irq irq;
+#define TYPE_MCHP_PFSOC_UART "mchp.pfsoc.uart"
+OBJECT_DECLARE_SIMPLE_TYPE(MchpPfSoCMMUartState, MCHP_PFSOC_UART)

-SerialMM *serial;
+typedef struct MchpPfSoCMMUartState {
+/*< private >*/
+SysBusDevice parent_obj;
+
+/*< public >*/
+MemoryRegion iomem;
+
+SerialMM serial_mm;

  uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
  } MchpPfSoCMMUartState;
diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
index 2facf85c2d8..74404e047d4 100644
--- a/hw/char/mchp_pfsoc_mmuart.c
+++ b/hw/char/mchp_pfsoc_mmuart.c
@@ -22,8 +22,9 @@

  #include "qemu/osdep.h"
  #include "qemu/log.h"
-#include "chardev/char.h"
+#include "qapi/error.h"
  #include "hw/char/mchp_pfsoc_mmuart.h"
+#include "hw/qdev-properties.h"

  static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned 
size)
  {
@@ -63,23 +64,69 @@ static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
  },
  };

-MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
-hwaddr base, qemu_irq irq, Chardev *chr)
+static void mchp_pfsoc_mmuart_init(Object *obj)
  {
-MchpPfSoCMMUartState *s;
-
-s = g_new0(MchpPfSoCMMUartState, 1);
+SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(obj);

  memory_region_init_io(>iomem, NULL, _pfsoc_mmuart_ops, s,
"mchp.pfsoc.mmuart", 0x1000);
+sysbus_init_mmio(sbd, >iomem);

-s->base = base;
-s->irq = irq;
-
-s->serial = serial_mm_init(sysmem, base, 2, irq, 399193, chr,
-   DEVICE_LITTLE_ENDIAN);
-
-memory_region_add_subregion(sysmem, base + 0x20, >iomem);
-
-return s;
+object_initialize_child(obj, "serial-mm", >serial_mm, TYPE_SERIAL_MM);
+object_property_add_alias(obj, "chardev", OBJECT(>serial_mm), 
"chardev");


Do we have a common convention for what needs to be done in the
instance_init() call and what in the realize() call?


No official convention IFAIK, but Peter & Markus described it in a
thread 2 years ago, IIRC it was about the TYPE_ARMV7M model.

See armv7m_instance_init() and armv7m_realize().

stellaris_init() does:

nvic = qdev_new(TYPE_ARMV7M);
qdev_connect_clock_in(nvic, "cpuclk",
  qdev_get_clock_out(ssys_dev, "SYSCLK"));
(1) qdev_prop_set_uint32(nvic, "num-irq", NUM_IRQ_LINES);
(2) object_property_set_link(OBJECT(nvic), "memory",
 OBJECT(get_system_memory()), _abort);
(3) sysbus_realize_and_unref(SYS_BUS_DEVICE(nvic), _fatal);

static void armv7m_instance_init(Object *obj)
{
...
object_initialize_child(obj, "nvic", >nvic, TYPE_NVIC);
object_property_add_alias(obj, "num-irq",
  OBJECT(>nvic), "num-irq");

For (1) to set the "num-irq" property (aliased to TYPE_NVIC)
before realization (3), it has to be available (aliased) first,
thus has to be in the instance_init() handler.

When the child creation depends on a property which is set by
the parent, the property must be set before the realization,
then is available in the realize() handler. For example with (2):

static void armv7m_realize(DeviceState *dev, Error **errp)
{
...
if (!s->board_memory) {
error_setg(errp, "memory property was not set");
return;
}
memory_region_add_subregion_overlap(>container, 0,
s->board_memory, -1);

If your model only provides link/aliased properties, then it
requires a instance_init() handler. If no property is consumed
during realization, then to keep it simple you can avoid
implementing a realize() handler, having all setup in instance_init().

If your model only consumes properties (no link/alias), it must
implement a realize() handler, and similarly you can skip the
instance_init(), having all setup in realize().

Now instance_init() is always called, and can never 

Re: [PATCH] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Philippe Mathieu-Daudé]

On 9/23/21 12:52, Philippe Mathieu-Daudé wrote:

On 9/23/21 12:41, Peter Maydell wrote:
On Thu, 23 Sept 2021 at 11:29, Philippe Mathieu-Daudé 
 wrote:

On 9/23/21 07:16, Bin Meng wrote:> On Sun, Sep 19, 2021 at 2:07 AM
Philippe Mathieu-Daudé  wrote:

+static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
+{
+    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
+
+    qdev_prop_set_uint8(DEVICE(>serial_mm), "regshift", 2);
+    qdev_prop_set_uint32(DEVICE(>serial_mm), "baudbase", 399193);
+    qdev_prop_set_uint8(DEVICE(>serial_mm), "endianness",
+    DEVICE_LITTLE_ENDIAN);


It looks like serial_mm_init() does one more thing:

  qdev_set_legacy_instance_id(DEVICE(smm), base, 2);

I am not sure what that is.


I'll defer on Paolo / Marc-André for that part, I think this is for
migrating legacy (x86?) machines, which is not the case.


Yes, this is only needed for backwards-compatibility of incoming
migration data with old versions of QEMU which implemented migration
of devices with hand-rolled code. If a device didn't previously
handle migration at all then it should not now be setting the
legacy instance ID.


Thanks. I'll try to add that in the documentation somewhere.


Speaking of migration, I notice that this QOM conversion does
not add a vmstate, which usually we do as part of the QOM conversion.
The device is also missing a reset method.


Sigh, you reminded me of this series:
"hw: Mark devices with no migratable fields"
https://lore.kernel.org/qemu-devel/20210117192446.23753-19-f4...@amsat.org/



OK, I'll add that in a previous patch.

Thanks,

Phil.

Re: [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready

[Ani Sinha]

On Wed, 22 Sep 2021, Damien Hedde wrote:

> Skip the sysbus device type per-machine allow-list check before the
> MACHINE_INIT_PHASE_READY phase.
>
> This patch permits adding any sysbus device (it still needs to be
> user_creatable) when using the -preconfig experimental option.
>
> Signed-off-by: Damien Hedde 
> ---
>
> This commit is RFC. Depending on the condition to allow a device
> to be added, it may change.
> ---
>  softmmu/qdev-monitor.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index f1c9242855..73b991adda 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char 
> **driver, Error **errp)
>  return NULL;
>  }
>
> -if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
> -/* sysbus devices need to be allowed by the machine */
> +if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) &&
> +phase_check(MACHINE_INIT_PHASE_READY)) {
> +/*
> + * Sysbus devices need to be allowed by the machine.
> + * We only check that after the machine is ready in order to let
> + * us add any user_creatable sysbus device during machine creation.
> + */
>  MachineClass *mc = 
> MACHINE_CLASS(object_get_class(qdev_get_machine()));
>  if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
>  error_setg(errp, "'%s' is not an allowed pluggable sysbus device 
> "

Since now you are adding the state of the machine creation in the
valiation condition, the failure error message becomes misleading.
Better to do this I think :

if (object class is TYPE_SYS_BUS_DEVICE)
{
  if (!phase_check(MACHINE_INIT_PHASE_READY))
{
  // error out here saying the state of the machine creation is too
early
}

// do the other check on dynamic sysbus

}

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[Kevin Wolf]

Am 23.09.2021 um 12:57 hat Kevin Wolf geschrieben:
> Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> > Add a warning for when 'iotests' runs against a qemu namespace that
> > isn't the one in the source tree. This might occur if you have
> > (accidentally) installed the Python namespace package to your local
> > packages.
> > 
> > (I'm not going to say that this is because I bit myself with this,
> > but. You can fill in the blanks.)
> > 
> > Signed-off-by: John Snow 
> > ---
> >  tests/qemu-iotests/testenv.py | 21 -
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> > index 88104dace90..8a43b193af5 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -16,6 +16,8 @@
> >  # along with this program.  If not, see .
> >  #
> >  
> > +import importlib.util
> > +import logging
> >  import os
> >  import sys
> >  import tempfile
> > @@ -25,7 +27,7 @@
> >  import random
> >  import subprocess
> >  import glob
> > -from typing import List, Dict, Any, Optional, ContextManager
> > +from typing import List, Dict, Any, Optional, ContextManager, cast
> >  
> >  DEF_GDB_OPTIONS = 'localhost:12345'
> >  
> > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> >  # Path where qemu goodies live in this source tree.
> >  qemu_srctree_path = Path(__file__, '../../../python').resolve()
> >  
> > +# warn if we happen to be able to find 'qemu' packages from an
> > +# unexpected location (i.e. the package is already installed in
> > +# the user's environment)
> > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > +if qemu_spec:
> > +spec_path = Path(cast(str, qemu_spec.origin))
> 
> You're casting Optional[str] to str here. The source type is not obvious
> from the local code, so unless you spend some time actively figuring it
> out, this could be casting anything to str. But even knowing this, just
> casting Optional away looks fishy anyway.
> 
> Looking up the ModuleSpec docs, it seems okay to assume that it's never
> None in our case, but wouldn't it be much cleaner to just 'assert
> qemu_spec.origin' first and then use it without the cast?
> 
> > +try:
> > +_ = spec_path.relative_to(qemu_srctree_path)
> > +except ValueError:
> > +self._logger.warning(
> > +"WARNING: 'qemu' package will be imported from outside 
> > "
> > +"the source tree!")
> > +self._logger.warning(
> > +"Importing from: '%s'",
> > +spec_path.parents[1])
> > +
> >  self.pythonpath = os.getenv('PYTHONPATH')
> >  self.pythonpath = os.pathsep.join(filter(None, (
> >  self.source_iotests,
> > @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: 
> > str,
> >  
> >  self.build_root = os.path.join(self.build_iotests, '..', '..')
> >  
> > +self._logger = logging.getLogger('qemu.iotests')
> >  self.init_directories()
> >  self.init_binaries()
> 
> Looks good otherwise.

Well, actually there is a major problem: We'll pass the right PYTHONPATH
for the test scripts that we're calling, but this script itself doesn't
use it yet. So what I get is:

Traceback (most recent call last):
  File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 120, in 

env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
  File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 253, in 
__init__
self.init_directories()
  File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 120, in 
init_directories
qemu_spec = importlib.util.find_spec('qemu.qmp')
  File "/usr/lib64/python3.9/importlib/util.py", line 94, in find_spec
parent = __import__(parent_name, fromlist=['__path__'])
ModuleNotFoundError: No module named 'qemu'

Kevin

Re: [PATCH v2] monitor: Rate-limit MEMORY_DEVICE_SIZE_CHANGE qapi events per device

[Markus Armbruster]

David Hildenbrand  writes:

> We want to rate-limit MEMORY_DEVICE_SIZE_CHANGE events per device,
> otherwise we can lose some events for devices. As we might not always have
> a device id, add the qom-path to the event and use that to rate-limit
> per device.

There are actually two reasons for adding qom-path.  One, you need it to
fix the rate limiting.  But adding to an external interface just to fix
an internal issue would be questionable.  Fortunately, there's also two:
make the event useful regardless of whether the user gave it an ID.  If
you have to respin, consider working two into the commit message.

I'd split this patch into "add qom-path" and "fix rate limiting".
Suggestion, not demand.

> This was noticed by starting a VM with two virtio-mem devices that each
> have a requested size > 0. The Linux guest will initialize both devices
> in parallel, resulting in losing MEMORY_DEVICE_SIZE_CHANGE events for
> one of the devices.
>
> Fixes: 722a3c783ef4 ("virtio-pci: Send qapi events when the virtio-mem size 
> changes")
> Suggested-by: Markus Armbruster 
> Cc: "Dr. David Alan Gilbert" 
> Cc: Markus Armbruster 
> Cc: Michael S. Tsirkin 
> Cc: Eric Blake 
> Cc: Igor Mammedov 
> Cc: Michal Privoznik 
> Signed-off-by: David Hildenbrand 
> ---
>
> Follow up of:
> https://lkml.kernel.org/r/20210921102434.24273-1-da...@redhat.com
>
> v1 -> v2:
> - Add the qom-path and use that identifier to rate-limit per device
> - Rephrase subject/description
>
> ---
>  hw/virtio/virtio-mem-pci.c | 3 ++-
>  monitor/monitor.c  | 9 +
>  qapi/machine.json  | 5 -
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
> index fa5395cd88..dd5085497f 100644
> --- a/hw/virtio/virtio-mem-pci.c
> +++ b/hw/virtio/virtio-mem-pci.c
> @@ -87,6 +87,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
> *notifier, void *data)
>  VirtIOMEMPCI *pci_mem = container_of(notifier, VirtIOMEMPCI,
>   size_change_notifier);
>  DeviceState *dev = DEVICE(pci_mem);
> +const char * qom_path = object_get_canonical_path(OBJECT(dev));

No space after this *, please.

>  const uint64_t * const size_p = data;
>  const char *id = NULL;
>  
> @@ -94,7 +95,7 @@ static void virtio_mem_pci_size_change_notify(Notifier 
> *notifier, void *data)
>  id = g_strdup(dev->id);
>  }
>  
> -qapi_event_send_memory_device_size_change(!!id, id, *size_p);
> +qapi_event_send_memory_device_size_change(!!id, id, *size_p, qom_path);

Doesn't this leak @qom_path?

>  }
>  
>  static void virtio_mem_pci_class_init(ObjectClass *klass, void *data)
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 46a171bca6..21c7a68758 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -474,6 +474,10 @@ static unsigned int qapi_event_throttle_hash(const void 
> *key)
>  hash += g_str_hash(qdict_get_str(evstate->data, "node-name"));
>  }
>  
> +if (evstate->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {
> +hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
> +}
> +
>  return hash;
>  }
>  
> @@ -496,6 +500,11 @@ static gboolean qapi_event_throttle_equal(const void *a, 
> const void *b)
> qdict_get_str(evb->data, "node-name"));
>  }
>  
> +if (eva->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {
> +return !strcmp(qdict_get_str(eva->data, "qom-path"),
> +   qdict_get_str(evb->data, "qom-path"));
> +}
> +
>  return TRUE;
>  }
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 157712f006..2487c92f18 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1245,8 +1245,11 @@
>  # action).
>  #
>  # @id: device's ID
> +#
>  # @size: the new size of memory that the device provides
>  #
> +# @qom-path: path to the device object in the QOM tree (since 6.2)
> +#
>  # Note: this event is rate-limited.
>  #
>  # Since: 5.1
> @@ -1259,7 +1262,7 @@
>  #
>  ##
>  { 'event': 'MEMORY_DEVICE_SIZE_CHANGE',
> -  'data': { '*id': 'str', 'size': 'size' } }
> +  'data': { '*id': 'str', 'size': 'size', 'qom-path' : 'str'} }
>  
>  
>  ##

With the two code remarks addressed:
Reviewed-by: Markus Armbruster 

[PATCH v4 29/35] acpi: arm/virt: convert build_iort() to endian agnostic build_append_FOO() API

[Igor Mammedov]

Drop usage of packed structures and explicit endian conversions
when building IORT table use endian agnostic build_append_int_noprefix()
API to build it.

Signed-off-by: Igor Mammedov 
---
CC: Eric Auger 

v4:
  - (Eric Auger )
* keep nb_nodes
* encode 'Memory access properties' as separate entries according to
  Table 13
* keep some comments
v3:
  * practically rewritten, due to conflicts wiht bypass iommu feature

CC: drjo...@redhat.com
CC: peter.mayd...@linaro.org
CC: shannon.zha...@gmail.com
CC: qemu-...@nongnu.org
CC: wangxinga...@huawei.com
CC: Eric Auger 
---
 include/hw/acpi/acpi-defs.h |  71 
 hw/arm/virt-acpi-build.c| 164 
 2 files changed, 93 insertions(+), 142 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 195f90caf6..6f2f08a9de 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -188,75 +188,4 @@ struct AcpiGenericTimerTable {
 } QEMU_PACKED;
 typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
 
-/*
- * IORT node types
- */
-
-#define ACPI_IORT_NODE_HEADER_DEF   /* Node format common fields */ \
-uint8_t  type;  \
-uint16_t length;\
-uint8_t  revision;  \
-uint32_t reserved;  \
-uint32_t mapping_count; \
-uint32_t mapping_offset;
-
-/* Values for node Type above */
-enum {
-ACPI_IORT_NODE_ITS_GROUP = 0x00,
-ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
-ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
-ACPI_IORT_NODE_SMMU = 0x03,
-ACPI_IORT_NODE_SMMU_V3 = 0x04
-};
-
-struct AcpiIortIdMapping {
-uint32_t input_base;
-uint32_t id_count;
-uint32_t output_base;
-uint32_t output_reference;
-uint32_t flags;
-} QEMU_PACKED;
-typedef struct AcpiIortIdMapping AcpiIortIdMapping;
-
-struct AcpiIortMemoryAccess {
-uint32_t cache_coherency;
-uint8_t  hints;
-uint16_t reserved;
-uint8_t  memory_flags;
-} QEMU_PACKED;
-typedef struct AcpiIortMemoryAccess AcpiIortMemoryAccess;
-
-struct AcpiIortItsGroup {
-ACPI_IORT_NODE_HEADER_DEF
-uint32_t its_count;
-uint32_t identifiers[];
-} QEMU_PACKED;
-typedef struct AcpiIortItsGroup AcpiIortItsGroup;
-
-#define ACPI_IORT_SMMU_V3_COHACC_OVERRIDE 1
-
-struct AcpiIortSmmu3 {
-ACPI_IORT_NODE_HEADER_DEF
-uint64_t base_address;
-uint32_t flags;
-uint32_t reserved2;
-uint64_t vatos_address;
-uint32_t model;
-uint32_t event_gsiv;
-uint32_t pri_gsiv;
-uint32_t gerr_gsiv;
-uint32_t sync_gsiv;
-AcpiIortIdMapping id_mapping_array[];
-} QEMU_PACKED;
-typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
-
-struct AcpiIortRC {
-ACPI_IORT_NODE_HEADER_DEF
-AcpiIortMemoryAccess memory_properties;
-uint32_t ats_attribute;
-uint32_t pci_segment_number;
-AcpiIortIdMapping id_mapping_array[];
-} QEMU_PACKED;
-typedef struct AcpiIortRC AcpiIortRC;
-
 #endif
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 42ea460313..8c382915a9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -240,6 +240,28 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState 
*vms)
 }
 #endif
 
+#define ID_MAPPING_ENTRY_SIZE 20
+#define SMMU_V3_ENTRY_SIZE 60
+#define ROOT_COMPLEX_ENTRY_SIZE 32
+#define IORT_NODE_OFFSET 48
+
+static void build_iort_id_mapping(GArray *table_data, uint32_t input_base,
+  uint32_t id_count, uint32_t out_ref)
+{
+/* Identity RID mapping covering the whole input RID range */
+build_append_int_noprefix(table_data, input_base, 4); /* Input base */
+build_append_int_noprefix(table_data, id_count, 4); /* Number of IDs */
+build_append_int_noprefix(table_data, input_base, 4); /* Output base */
+build_append_int_noprefix(table_data, out_ref, 4); /* Output Reference */
+build_append_int_noprefix(table_data, 0, 4); /* Flags */
+}
+
+struct AcpiIortIdMapping {
+uint32_t input_base;
+uint32_t id_count;
+};
+typedef struct AcpiIortIdMapping AcpiIortIdMapping;
+
 /* Build the iort ID mapping to SMMUv3 for a given PCI host bridge */
 static int
 iort_host_bridges(Object *obj, void *opaque)
@@ -282,17 +304,16 @@ static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
 int i, nb_nodes, rc_mapping_count;
-AcpiIortIdMapping *idmap;
-AcpiIortItsGroup *its;
-AcpiIortSmmu3 *smmu;
-AcpiIortRC *rc;
-const uint32_t iort_node_offset = 48;
+const uint32_t iort_node_offset = IORT_NODE_OFFSET;
 size_t node_size, smmu_offset = 0;
+AcpiIortIdMapping *idmap;
 GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
 GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
 
 AcpiTable table = { .sig = "IORT", .rev = 0, .oem_id = vms->oem_id,
 .oem_table_id = vms->oem_table_id };
+/* Table 2 The IORT */
+acpi_table_begin(, table_data);
 
 

Re: [PATCH] tests: qtest: bios-tables-test depends on the unpacked edk2 ROMs

[Daniel P . Berrangé]

On Thu, Sep 23, 2021 at 04:15:55AM -0400, Paolo Bonzini wrote:
> Skip the test if bzip2 is not available, and run it after they are
> uncompressed.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  pc-bios/meson.build | 3 ++-
>  tests/qtest/meson.build | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/meson.build b/pc-bios/meson.build
> index f2b32598af..975565198e 100644
> --- a/pc-bios/meson.build
> +++ b/pc-bios/meson.build
> @@ -10,8 +10,9 @@ if install_edk2_blobs
>  'edk2-x86_64-secure-code.fd',
>]
>  
> +  roms = []
>foreach f : fds
> -custom_target(f,
> +roms += custom_target(f,
>build_by_default: have_system,
>output: f,
>input: '@0@.bz2'.format(f),
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index e1f4df3df8..6d8100c9de 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -68,12 +68,12 @@ qtests_i386 = \
>(config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) 
> +  \
>(config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? 
> ['fuzz-e1000e-test'] : []) +   \
>(config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +  
>\
> +  (install_edk2_blobs ? ['bios-tables-test'] : []) + 
>\
>qtests_pci +   
>\
>['fdc-test',
> 'ide-test',
> 'hd-geo-test',
> 'boot-order-test',
> -   'bios-tables-test',
> 'rtc-test',
> 'i440fx-test',
> 'fw_cfg-test',
> @@ -180,7 +180,7 @@ qtests_arm = \
>  
>  # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test 
> unconditional
>  qtests_aarch64 = \
> -  (cpu != 'arm' ? ['bios-tables-test'] : []) +   
>\
> +  (cpu != 'arm' and install_edk2_blobs ? ['bios-tables-test'] : []) +
>\
>(config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
> ['tpm-tis-device-test'] : []) +\
>(config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? 
> ['tpm-tis-device-swtpm-test'] : []) +  \
>['arm-cpu-features',
> @@ -269,7 +269,7 @@ foreach dir : target_dirs
>qtest_emulator = emulators['qemu-system-' + target_base]
>target_qtests = get_variable('qtests_' + target_base, []) + qtests_generic
>  
> -  test_deps = []
> +  test_deps = roms

Shouldn't this be

  if install_edk2_blobs
 test_deps += roms
  endif


>qtest_env = environment()
>if have_tools
>  qtest_env.set('QTEST_QEMU_IMG', './qemu-img')

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 :|

[PATCH v5 07/20] nubus: add trace-events for empty slot accesses

[Mark Cave-Ayland]

Increase the max_access_size to 4 bytes for empty Nubus slot and super slot
accesses to allow tracing of the Nubus enumeration process by the guest OS.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c  | 10 +++---
 hw/nubus/trace-events |  7 +++
 hw/nubus/trace.h  |  1 +
 meson.build   |  1 +
 4 files changed, 16 insertions(+), 3 deletions(-)
 create mode 100644 hw/nubus/trace-events
 create mode 100644 hw/nubus/trace.h

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index df93a07dfe..b398423c99 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
+#include "trace.h"
 
 
 static NubusBus *nubus_find(void)
@@ -31,12 +32,13 @@ static void nubus_slot_write(void *opaque, hwaddr addr, 
uint64_t val,
  unsigned int size)
 {
 /* read only */
+trace_nubus_slot_write(addr, val, size);
 }
 
-
 static uint64_t nubus_slot_read(void *opaque, hwaddr addr,
 unsigned int size)
 {
+trace_nubus_slot_read(addr, size);
 return 0;
 }
 
@@ -46,7 +48,7 @@ static const MemoryRegionOps nubus_slot_ops = {
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
-.max_access_size = 1,
+.max_access_size = 4,
 },
 };
 
@@ -54,11 +56,13 @@ static void nubus_super_slot_write(void *opaque, hwaddr 
addr, uint64_t val,
unsigned int size)
 {
 /* read only */
+trace_nubus_super_slot_write(addr, val, size);
 }
 
 static uint64_t nubus_super_slot_read(void *opaque, hwaddr addr,
   unsigned int size)
 {
+trace_nubus_super_slot_read(addr, size);
 return 0;
 }
 
@@ -68,7 +72,7 @@ static const MemoryRegionOps nubus_super_slot_ops = {
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
-.max_access_size = 1,
+.max_access_size = 4,
 },
 };
 
diff --git a/hw/nubus/trace-events b/hw/nubus/trace-events
new file mode 100644
index 00..e31833d694
--- /dev/null
+++ b/hw/nubus/trace-events
@@ -0,0 +1,7 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# nubus-bus.c
+nubus_slot_read(uint64_t addr, int size) "reading unassigned addr 0x%"PRIx64 " 
size %d"
+nubus_slot_write(uint64_t addr, uint64_t val, int size) "writing unassigned 
addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
+nubus_super_slot_read(uint64_t addr, int size) "reading unassigned addr 
0x%"PRIx64 " size %d"
+nubus_super_slot_write(uint64_t addr, uint64_t val, int size) "writing 
unassigned addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
diff --git a/hw/nubus/trace.h b/hw/nubus/trace.h
new file mode 100644
index 00..3749420da1
--- /dev/null
+++ b/hw/nubus/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-hw_nubus.h"
diff --git a/meson.build b/meson.build
index 15ef4d3c41..7bdbbbdf02 100644
--- a/meson.build
+++ b/meson.build
@@ -2142,6 +2142,7 @@ if have_system
 'hw/misc/macio',
 'hw/net',
 'hw/net/can',
+'hw/nubus',
 'hw/nvme',
 'hw/nvram',
 'hw/pci',
-- 
2.20.1

[PATCH v5 02/20] nubus-device: rename slot_nb variable to slot

[Mark Cave-Ayland]

This is in preparation for creating a qdev property of the same name.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-device.c  | 14 +++---
 include/hw/nubus/nubus.h |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index ffe78a8823..be01269563 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -87,7 +87,7 @@ static void nubus_register_format_block(NubusDevice *dev)
 char *fblock_name;
 
 fblock_name = g_strdup_printf("nubus-slot-%d-format-block",
-  dev->slot_nb);
+  dev->slot);
 
 hwaddr fblock_offset = memory_region_size(>slot_mem) - FBLOCK_SIZE;
 memory_region_init_io(>fblock_io, NULL, _format_block_ops,
@@ -142,7 +142,7 @@ void nubus_register_rom(NubusDevice *dev, const uint8_t 
*rom, uint32_t size,
 /* ROM */
 
 dev->rom = rom;
-rom_name = g_strdup_printf("nubus-slot-%d-rom", dev->slot_nb);
+rom_name = g_strdup_printf("nubus-slot-%d-rom", dev->slot);
 memory_region_init_io(>rom_io, NULL, _nubus_rom_ops,
   dev, rom_name, size);
 memory_region_set_readonly(>rom_io, true);
@@ -167,12 +167,12 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
-nd->slot_nb = nubus->current_slot++;
-name = g_strdup_printf("nubus-slot-%d", nd->slot_nb);
+nd->slot = nubus->current_slot++;
+name = g_strdup_printf("nubus-slot-%d", nd->slot);
 
-if (nd->slot_nb < NUBUS_FIRST_SLOT) {
+if (nd->slot < NUBUS_FIRST_SLOT) {
 /* Super */
-slot_offset = (nd->slot_nb - 6) * NUBUS_SUPER_SLOT_SIZE;
+slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE;
 
 memory_region_init(>slot_mem, OBJECT(dev), name,
NUBUS_SUPER_SLOT_SIZE);
@@ -180,7 +180,7 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 >slot_mem);
 } else {
 /* Normal */
-slot_offset = nd->slot_nb * NUBUS_SLOT_SIZE;
+slot_offset = nd->slot * NUBUS_SLOT_SIZE;
 
 memory_region_init(>slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE);
 memory_region_add_subregion(>slot_io, slot_offset,
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index e2b5cf260b..424309dd73 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -42,7 +42,7 @@ struct NubusBus {
 struct NubusDevice {
 DeviceState qdev;
 
-int slot_nb;
+int slot;
 MemoryRegion slot_mem;
 
 /* Format Block */
-- 
2.20.1

[PATCH v5 04/20] nubus: use bitmap to manage available slots

[Mark Cave-Ayland]

Convert nubus_device_realize() to use a bitmap to manage available slots to 
allow
for future Nubus devices to be plugged into arbitrary slots from the command 
line
using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh
machines as documented in "Desigining Cards and Drivers for the Macintosh 
Family".

Signed-off-by: Mark Cave-Ayland 
---
 hw/nubus/mac-nubus-bridge.c |  4 
 hw/nubus/nubus-bus.c|  5 +++--
 hw/nubus/nubus-device.c | 32 +++--
 include/hw/nubus/mac-nubus-bridge.h |  4 
 include/hw/nubus/nubus.h| 13 ++--
 5 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 7c329300b8..3f075789e9 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -18,6 +18,10 @@ static void mac_nubus_bridge_init(Object *obj)
 
 s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
 
+/* Macintosh only has slots 0x9 to 0xe available */
+s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
+  MAC_NUBUS_SLOT_NB);
+
 sysbus_init_mmio(sbd, >bus->super_slot_io);
 sysbus_init_mmio(sbd, >bus->slot_io);
 }
diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index f4410803ff..3cd7534864 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -86,13 +86,14 @@ static void nubus_init(Object *obj)
 
 memory_region_init_io(>super_slot_io, obj, _super_slot_ops,
   nubus, "nubus-super-slots",
-  NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
+  (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE);
 
 memory_region_init_io(>slot_io, obj, _slot_ops,
   nubus, "nubus-slots",
   NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
 
-nubus->current_slot = NUBUS_FIRST_SLOT;
+nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT,
+ NUBUS_SLOT_NB);
 }
 
 static void nubus_class_init(ObjectClass *oc, void *data)
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 4e23df1280..562650a05b 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 NubusDevice *nd = NUBUS_DEVICE(dev);
 char *name;
 hwaddr slot_offset;
-
-if (nubus->current_slot < NUBUS_FIRST_SLOT ||
-nubus->current_slot > NUBUS_LAST_SLOT) {
-error_setg(errp, "Cannot register nubus card, not enough slots");
-return;
+uint16_t s;
+
+if (nd->slot == -1) {
+/* No slot specified, find first available free slot */
+s = ctz32(nubus->slot_available_mask);
+if (s != 32) {
+nd->slot = s;
+} else {
+error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+return;
+}
+} else {
+/* Slot specified, make sure the slot is available */
+if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+return;
+}
 }
 
-nd->slot = nubus->current_slot++;
+nubus->slot_available_mask &= ~BIT(nd->slot);
 
 /* Super */
 slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
@@ -191,12 +205,18 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 nubus_register_format_block(nd);
 }
 
+static Property nubus_device_properties[] = {
+DEFINE_PROP_INT32("slot", NubusDevice, slot, -1),
+DEFINE_PROP_END_OF_LIST()
+};
+
 static void nubus_device_class_init(ObjectClass *oc, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(oc);
 
 dc->realize = nubus_device_realize;
 dc->bus_type = TYPE_NUBUS_BUS;
+device_class_set_props(dc, nubus_device_properties);
 }
 
 static const TypeInfo nubus_device_type_info = {
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index 36aa098dd4..118d67267d 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -12,6 +12,10 @@
 #include "hw/nubus/nubus.h"
 #include "qom/object.h"
 
+#define MAC_NUBUS_FIRST_SLOT 0x9
+#define MAC_NUBUS_LAST_SLOT  0xe
+#define MAC_NUBUS_SLOT_NB(MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1)
+
 #define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge"
 OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE)
 
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 89b0976aaa..988e4a2361 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -14,13 +14,12 @@
 #include "qom/object.h"
 
 #define NUBUS_SUPER_SLOT_SIZE 

[PATCH v5 09/20] macfb: don't register declaration ROM

[Mark Cave-Ayland]

The macfb device is an on-board framebuffer and so is initialised by the
system declaration ROM included within the MacOS toolbox ROM.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/display/macfb.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index d8183b9bbd..76808b69cc 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -383,10 +383,6 @@ static void macfb_sysbus_realize(DeviceState *dev, Error 
**errp)
 sysbus_init_mmio(SYS_BUS_DEVICE(s), >mem_vram);
 }
 
-const uint8_t macfb_rom[] = {
-255, 0, 0, 0,
-};
-
 static void macfb_nubus_realize(DeviceState *dev, Error **errp)
 {
 NubusDevice *nd = NUBUS_DEVICE(dev);
@@ -399,8 +395,6 @@ static void macfb_nubus_realize(DeviceState *dev, Error 
**errp)
 macfb_common_realize(dev, ms, errp);
 memory_region_add_subregion(>slot_mem, DAFB_BASE, >mem_ctrl);
 memory_region_add_subregion(>slot_mem, VIDEO_BASE, >mem_vram);
-
-nubus_register_rom(nd, macfb_rom, sizeof(macfb_rom), 1, 9, 0xf);
 }
 
 static void macfb_sysbus_reset(DeviceState *d)
-- 
2.20.1

[PATCH v5 15/20] nubus: move NubusBus from mac-nubus-bridge to nubus-bridge

[Mark Cave-Ayland]

Now that Nubus has its own address space rather than mapping directly into the
system bus, move the Nubus reference from MacNubusBridge to NubusBridge.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/m68k/q800.c  |  2 +-
 hw/nubus/mac-nubus-bridge.c | 11 +--
 hw/nubus/nubus-bridge.c |  9 +
 include/hw/nubus/mac-nubus-bridge.h |  1 -
 include/hw/nubus/nubus.h|  2 ++
 5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index a07912b87c..9bdea1a362 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -398,7 +398,7 @@ static void q800_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
 MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
-nubus = MAC_NUBUS_BRIDGE(dev)->bus;
+nubus = NUBUS_BRIDGE(dev)->bus;
 
 /* framebuffer in nubus slot #9 */
 
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index e241c581b5..db8640eed2 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -16,22 +16,21 @@
 static void mac_nubus_bridge_init(Object *obj)
 {
 MacNubusBridge *s = MAC_NUBUS_BRIDGE(obj);
+NubusBridge *nb = NUBUS_BRIDGE(obj);
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
-
 /* Macintosh only has slots 0x9 to 0xe available */
-s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
-  MAC_NUBUS_SLOT_NB);
+nb->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
+   MAC_NUBUS_SLOT_NB);
 
 /* Aliases for slots 0x9 to 0xe */
 memory_region_init_alias(>super_slot_alias, obj, "super-slot-alias",
- >bus->nubus_mr,
+ >bus->nubus_mr,
  MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
 
 memory_region_init_alias(>slot_alias, obj, "slot-alias",
- >bus->nubus_mr,
+ >bus->nubus_mr,
  NUBUS_SLOT_BASE +
  MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 95662568c5..3b68d4435c 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -12,6 +12,14 @@
 #include "hw/sysbus.h"
 #include "hw/nubus/nubus.h"
 
+
+static void nubus_bridge_init(Object *obj)
+{
+NubusBridge *s = NUBUS_BRIDGE(obj);
+
+s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
+}
+
 static void nubus_bridge_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -22,6 +30,7 @@ static void nubus_bridge_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo nubus_bridge_info = {
 .name  = TYPE_NUBUS_BRIDGE,
 .parent= TYPE_SYS_BUS_DEVICE,
+.instance_init = nubus_bridge_init,
 .instance_size = sizeof(NubusBridge),
 .class_init= nubus_bridge_class_init,
 };
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index b595e1b7ef..70ab50ab2d 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -22,7 +22,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(MacNubusBridge, MAC_NUBUS_BRIDGE)
 struct MacNubusBridge {
 NubusBridge parent_obj;
 
-NubusBus *bus;
 MemoryRegion super_slot_alias;
 MemoryRegion slot_alias;
 };
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 43bde7dd69..607cdef968 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -62,6 +62,8 @@ struct NubusDevice {
 
 struct NubusBridge {
 SysBusDevice parent_obj;
+
+NubusBus *bus;
 };
 
 #endif
-- 
2.20.1

Re: [PATCH v5 01/20] nubus: add comment indicating reference documents

[Philippe Mathieu-Daudé]

On 9/23/21 11:12, Mark Cave-Ayland wrote:

Signed-off-by: Mark Cave-Ayland 
---
  hw/nubus/nubus-bus.c | 8 
  1 file changed, 8 insertions(+)


Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

Re: [PATCH 2/2] tests/Makefile: add TESTFILES option to make check-acceptance

[Philippe Mathieu-Daudé]

On 9/23/21 11:42, Daniel P. Berrangé wrote:

On Thu, Sep 23, 2021 at 11:34:18AM +0200, Philippe Mathieu-Daudé wrote:

On 9/22/21 21:46, Willian Rampazzo wrote:

On Wed, Sep 22, 2021 at 4:08 PM Philippe Mathieu-Daudé
 wrote:


On 9/22/21 21:03, Willian Rampazzo wrote:

Add the possibility of running all the tests from a single file, or
multiple files, running a single test within a file or multiple tests
within multiple files using `make check-acceptance` and the TESTFILES
environment variable.

Signed-off-by: Willian Rampazzo 
---
docs/devel/testing.rst | 27 +++
tests/Makefile.include |  5 -
2 files changed, 31 insertions(+), 1 deletion(-)



diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6e16c05f10..82d7ef7a20 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -88,6 +88,9 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
+ifndef TESTFILES
+ TESTFILES=tests/acceptance
+endif
# Controls the output generated by Avocado when running tests.
# Any number of command separated loggers are accepted.  For more
# information please refer to "avocado --help".
@@ -130,7 +133,7 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR) 
get-vm-images
--show=$(AVOCADO_SHOW) run 
--job-results-dir=$(TESTS_RESULTS_DIR) \
--filter-by-tags-include-empty 
--filter-by-tags-include-empty-key \
$(AVOCADO_TAGS) \
-$(if $(GITLAB_CI),,--failfast) tests/acceptance, \
+$(if $(GITLAB_CI),,--failfast) $(TESTFILES), \


Since this is Avocado specific, maybe call the variable
AVOCADO_TESTFILES (similar to AVOCADO_TAGS)?


I don't see a problem with changing that to AVOCADO_TESTFILES. I was
trying to make things shorter and easy to remember. If the too-long
variable name is not a problem, I can change that.


This is the generic tests/Makefile, so $TESTFILES might be confusing,
which is why I prefer the explicit AVOCADO_ prefix (AVOCADO_SHOW,
AVOCADO_TAGS).


IIUC, this is not actually just test files - it is test files plus the
test names. So better just  $(AVOCADO_TESTS)


Good point, I agree.

Re: [PATCH v5 03/20] nubus-device: expose separate super slot memory region

[Laurent Vivier]

Le 23/09/2021 à 11:12, Mark Cave-Ayland a écrit :
> According to "Designing Cards and Drivers for the Macintosh Family" each 
> physical
> nubus slot can access 2 separate address ranges: a super slot memory region 
> which
> is 256MB and a standard slot memory region which is 16MB.
> 
> Currently a Nubus device uses the physical slot number to determine whether 
> it is
> using a standard slot memory region or a super slot memory region rather than
> exposing both memory regions for use as required.
> 
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/nubus/nubus-device.c  | 36 ++--
>  include/hw/nubus/nubus.h |  1 +
>  2 files changed, 19 insertions(+), 18 deletions(-)
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH v5 04/20] nubus: use bitmap to manage available slots

[Laurent Vivier]

Le 23/09/2021 à 11:12, Mark Cave-Ayland a écrit :
> Convert nubus_device_realize() to use a bitmap to manage available slots to 
> allow
> for future Nubus devices to be plugged into arbitrary slots from the command 
> line
> using a new qdev "slot" parameter for nubus devices.
> 
> Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh
> machines as documented in "Desigining Cards and Drivers for the Macintosh 
> Family".
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/nubus/mac-nubus-bridge.c |  4 
>  hw/nubus/nubus-bus.c|  5 +++--
>  hw/nubus/nubus-device.c | 32 +++--
>  include/hw/nubus/mac-nubus-bridge.h |  4 
>  include/hw/nubus/nubus.h| 13 ++--
>  5 files changed, 43 insertions(+), 15 deletions(-)
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH v3 03/30] tcg/loongarch64: Add the tcg-target.h file

[Philippe Mathieu-Daudé]

On 9/22/21 20:58, Richard Henderson wrote:

On 9/22/21 11:34 AM, Philippe Mathieu-Daudé wrote:


Here could go:

    TCG_TARGET_NB_REGS,


Not a fan of putting NFOO in the enumeration.
It interferes with -Wenum stuff.


Oh good point... TIL :)

Re: [PATCH v5 13/20] nubus-bridge: introduce separate NubusBridge structure

[Laurent Vivier]

Le 23/09/2021 à 11:13, Mark Cave-Ayland a écrit :
> This is to allow the Nubus bridge to store its own additional state. Also 
> update
> the comment in the file header to reflect that nubus-bridge is not specific to
> the Macintosh.
> 
> Signed-off-by: Mark Cave-Ayland 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/nubus/nubus-bridge.c | 4 ++--
>  include/hw/nubus/mac-nubus-bridge.h | 2 +-
>  include/hw/nubus/nubus.h| 6 ++
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 

Reviewed-by: Laurent Vivier 

I think it could be merged with next patch.

Re: [RFC PATCH v2 00/16] Initial support for machine creation via QMP

[Damien Hedde]

On 9/22/21 19:50, Philippe Mathieu-Daudé wrote:

Hi Damien,

On 9/22/21 18:13, Damien Hedde wrote:


The goal of this work is to bring dynamic machine creation to QEMU:
we want to setup a machine without compiling a specific machine C
code. It would ease supporting highly configurable platforms (for
example resulting from an automated design flow). The requirements
for such configuration include begin able to specify the number of
cores, available peripherals, emmory mapping, IRQ mapping, etc.

This series focuses on the first step: populating a machine with
devices during its creation. We propose patches to support this
using QMP commands. This is a working set of patches and improves
over the earlier rfc (posted in May):
https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html


Do you have a roadmap for the following steps? Or are you done with
this series?


Hi Philippe,

We would like to stick to this scope ("creating devices in a machine") 
for this particular series otherwise it will become very big and cover a 
huge scope.


We have some patches to "migrate" more devices to be early 
user-creatable (like the last 2 patches of this series) so that we can 
use more device when building a machine. But these are trivial and only 
depends on what is the condition to allow this. We plan to submit these 
when this series is done.


We plan to address other issues we have in others series of patches. We 
do not put a roadmap somewhere. But we can details this in a page in our 
github or in the qemu wiki if you think this is a good idea.

Here are the main remaining issues:
+ the base machine (we are using "none" here because it is "almost" 
empty and fit our needs but it has some limitations)

+ adding cpus
+ non-trivial memory mappings
+ solving backend (eg: net) connection issues



Yesterday I was thinking about this, and one thing I was wondering is
if it would be possible to have DeviceClass and MachineClass implement
a populate_fdt() handler, to automatically generate custom DTB for these
custom machines.

Maybe in your case you don't need that, as your framework generating the
QEMU machine also generates the DTB, or even parse a DTB to generate the
machine... :)


You are right, we do not need this. We indeed use a device tree file to 
describe the platform but this is an "extended" device tree (it has more 
info than needed for linux). If it was not the case, I think it would 
still be easier to generate it from the source platform description than 
using qemu as an intermediate.


It is probably possible but it is tricky to generate the dtb: mapping in 
dtb are not standardized and really depends on the node types.


For example, to generate the interrupt-map property of a device node. 
You need to know the interrupt mapping (which interrupt line goes in 
which interrupt controller) and also have info about the interrupt 
controller's cells format (eg: how many bytes do we need to specify the 
interrupt). For example for some controller, you have specify the 
interrupt kind (rising or falling edge, high or low active level).


So you'll probably need some special "get_dtb_interrupt_cell" method in 
interrupt controllers to generate these entries for you so that a device 
can ask dtb data to its controller.


Bus mappings also depend on the bus type, but since qemu devices are 
already organized on a bus-type basis, this is probably easier to solve.


You'll have similar issues with every mapping. But bus and interrupt are 
the most important ones.


Thanks,
Damien

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[Daniel P . Berrangé]

On Wed, Sep 22, 2021 at 08:16:21PM -0400, John Snow wrote:
> Add a warning for when 'iotests' runs against a qemu namespace that
> isn't the one in the source tree. This might occur if you have
> (accidentally) installed the Python namespace package to your local
> packages.

IIUC, it is/was a valid use case to run the iotests on arbitrary
QEMU outside the source tree ie a distro packaged binary set.

Are we not allowing the tests/qemu-iotests/* content to be
run outside the context of the main source tree for this
purpose ?

eg  consider if Fedora/RHEL builds put tests/qemu-iotests/* into
a 'qemu-iotests' RPM, which was installed and used with a distro
package python-qemu ?

> (I'm not going to say that this is because I bit myself with this,
> but. You can fill in the blanks.)
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/testenv.py | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 88104dace90..8a43b193af5 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -16,6 +16,8 @@
>  # along with this program.  If not, see .
>  #
>  
> +import importlib.util
> +import logging
>  import os
>  import sys
>  import tempfile
> @@ -25,7 +27,7 @@
>  import random
>  import subprocess
>  import glob
> -from typing import List, Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional, ContextManager, cast
>  
>  DEF_GDB_OPTIONS = 'localhost:12345'
>  
> @@ -112,6 +114,22 @@ def init_directories(self) -> None:
>  # Path where qemu goodies live in this source tree.
>  qemu_srctree_path = Path(__file__, '../../../python').resolve()
>  
> +# warn if we happen to be able to find 'qemu' packages from an
> +# unexpected location (i.e. the package is already installed in
> +# the user's environment)
> +qemu_spec = importlib.util.find_spec('qemu.qmp')
> +if qemu_spec:
> +spec_path = Path(cast(str, qemu_spec.origin))
> +try:
> +_ = spec_path.relative_to(qemu_srctree_path)
> +except ValueError:
> +self._logger.warning(
> +"WARNING: 'qemu' package will be imported from outside "
> +"the source tree!")
> +self._logger.warning(
> +"Importing from: '%s'",
> +spec_path.parents[1])

It feels to me like the scenario  we're blocking here is actally
the scenario we want to aim for.

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 :|

[PATCH 0/6] Improve consistency of bus init function names

[Peter Maydell]

Currently we have a bit of a mishmash of different function
names for bus creation. There are two basic patterns: you
can have a function that allocates and returns a new bus
object; or you can have a function that takes a pointer to
a bus object and initializes it in-place. We have to some
extent a convention for those: the allocate-and-return
function is 'foo_new()', and the 'init in-place' function
is 'foo_init()'. However many of our bus creation functions
don't follow that; some use 'foo_new' vs 'foo_new_inplace';
some use 'foo_new' for the in-place init version; and
the bottom level qbus functions are 'qbus_create' vs
'qbus_create_inplace'. This series tries to bring at least
scsi, ipack, pci, ide, and qbus into line with the
_new-vs-_init naming convention.

The other issue with bus creation functions is that some
of them take a 'name' argument which can be NULL, and some
do not. Generally "pass in a specific name" should be the
rare case, but our API design here is easy to misuse, and
so a lot of callsites (especially for i2c, sd, ssi) pass
in names when they should not. Untangling that mess is
going to be tricky (see other thread for more), but as
a first step, this series proposes a split between
foo_bus_new() and foo_bus_new_named() where the latter
takes a name parameter and the former does not. I do
this only for scsi (and implicitly ide, whose ide_bus_new
function already doesn't take a name argument) for the
moment, as the other bus types have more of a mess of
"pass name when they should not" callsites, so I didn't
want to put in too much work before finding out if we
had agreement on this as a naming convention.

There are definitely more buses that can be cleaned up
to follow the init vs new convention, but this series is
already touching 70 files and trying to do every bus in
one series seems like a recipe for merge conflicts.
So this seemed like enough to be going on with...

thanks
-- PMM

Peter Maydell (6):
  scsi: Replace scsi_bus_new() with scsi_bus_init(),
scsi_bus_init_named()
  ipack: Rename ipack_bus_new_inplace() to ipack_bus_init()
  pci: Rename pci_root_bus_new_inplace() to pci_root_bus_init()
  qbus: Rename qbus_create_inplace() to qbus_init()
  qbus: Rename qbus_create() to qbus_new()
  ide: Rename ide_bus_new() to ide_bus_init()

 include/hw/ide/internal.h |  4 ++--
 include/hw/ipack/ipack.h  |  8 
 include/hw/pci/pci.h  | 10 +-
 include/hw/qdev-core.h|  6 +++---
 include/hw/scsi/scsi.h| 30 --
 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 | 13 +++--
 hw/core/sysbus.c  | 10 ++
 hw/gpio/bcm2835_gpio.c|  3 +--
 hw/hyperv/vmbus.c |  2 +-
 hw/i2c/core.c |  2 +-
 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 |  4 ++--
 hw/ide/sii3112.c  |  2 +-
 hw/ide/via.c  |  2 +-
 hw/ipack/ipack.c  | 10 +-
 hw/ipack/tpci200.c|  4 ++--
 hw/isa/isa-bus.c  |  2 +-
 hw/misc/auxbus.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/mac-nubus-bridge.c   |  2 +-
 hw/nvme/ctrl.c|  4 ++--
 hw/nvme/subsys.c  |  3 +--
 hw/pci-host/raven.c   |  4 ++--
 hw/pci-host/versatile.c   |  6 +++---
 hw/pci/pci.c  | 30 +++---
 hw/pci/pci_bridge.c   |  4 ++--
 hw/ppc/spapr_vio.c|  2 +-
 hw/s390x/ap-bridge.c  |  2 +-
 hw/s390x/css-bridge.c |  2 +-
 hw/s390x/event-facility.c |  4 ++--
 hw/s390x/s390-pci-bus.c   |  2 +-
 hw/s390x/virtio-ccw.c |  3 +--
 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|  6 +++---
 hw/scsi/spapr_vscsi.c |  3 +--
 hw/scsi/virtio-scsi.c |  4 ++--
 hw/scsi/vmw_pvscsi.c  |  3 +--
 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/ssi/ssi.c  |  2 +-
 hw/usb/bus.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  3 +--
 hw/usb/dev-storage-bot.c  |  3 +--
 hw/usb/dev-storage-classic.c  |  4 ++--
 

Re: [PATCH v5 14/20] mac-nubus-bridge: rename MacNubusState to MacNubusBridge

[Mark Cave-Ayland]

On 23/09/2021 11:35, Laurent Vivier wrote:


Le 23/09/2021 à 11:13, Mark Cave-Ayland a écrit :

This better reflects that the mac-nubus-bridge device is derived from the
nubus-bridge device, and that the structure represents the state of the bridge
device and not the Nubus itself. Also update the comment in the file header to
reflect that mac-nubus-bridge is specific to the Macintosh.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
  hw/nubus/mac-nubus-bridge.c | 8 +---
  include/hw/nubus/mac-nubus-bridge.h | 4 ++--
  2 files changed, 7 insertions(+), 5 deletions(-)




Reviewed-by: Laurent Vivier 

(it could be merged with previous one)


I like to try and keep renames on a per-device basis if possible, even if it's just 
to help rebasing during development.


Other than that, is there anything else outstanding you think would require a 
v6 series?


ATB,

Mark.

Re: [Qemu-devel] [PULL 18/18] qapi: move RTC_CHANGE to the target schema

[Peter Maydell]

On Mon, 18 Feb 2019 at 14:19, Markus Armbruster  wrote:
>
> From: Marc-André Lureau 
>
> A few targets don't emit RTC_CHANGE, we could restrict the event to
> the tagets that do emit it.
>
> Note: There is a lot more of events & commands that we could restrict
> to capable targets, with the cost of some additional complexity, but
> the benefit of added correctness and better introspection.
>
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Markus Armbruster 
> Signed-off-by: Markus Armbruster 
> Message-Id: <20190214152251.2073-19-arm...@redhat.com>

Hi; I've just run into this starting from Eric's patch to add
RTC_CHANGE event support to the pl031 RTC. It seems kind of
awkward to me:

> diff --git a/qapi/target.json b/qapi/target.json
> index 5c41a0aee7..da7b4be51e 100644
> --- a/qapi/target.json
> +++ b/qapi/target.json
> @@ -7,6 +7,29 @@
>
>  { 'include': 'misc.json' }
>
> +##
> +# @RTC_CHANGE:
> +#
> +# Emitted when the guest changes the RTC time.
> +#
> +# @offset: offset between base RTC clock (as specified by -rtc base), and
> +#  new RTC clock value
> +#
> +# Note: This event is rate-limited.
> +#
> +# Since: 0.13.0
> +#
> +# Example:
> +#
> +# <-   { "event": "RTC_CHANGE",
> +#"data": { "offset": 78 },
> +#"timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
> +#
> +##
> +{ 'event': 'RTC_CHANGE',
> +  'data': { 'offset': 'int' },
> +  'if': 'defined(TARGET_ALPHA) || defined(TARGET_ARM) || 
> defined(TARGET_HPPA) || defined(TARGET_I386) || defined(TARGET_MIPS) || 
> defined(TARGET_MIPS64) || defined(TARGET_MOXIE) || defined(TARGET_PPC) || 
> defined(TARGET_PPC64) || defined(TARGET_S390X) || defined(TARGET_SH4) || 
> defined(TARGET_SPARC)' }
> +

Now we have a massive list of TARGET if conditions. As a general
principle if we can avoid long TARGET if-lists we should, because
it is yet another thing that needs updating when a new target
is added. In this case any new architecture that can handle an
ISA device would need to update this list. I pretty much guarantee
nobody is going to remember to do that.

It also doesn't actually usefully tell the thing on the other
end whether it can expect to see RTC_CHANGE events, because
whether it will actually get them depends not on the target
architecture but on the specific combination of machine type
and plugged-in devices. If there's a need for the other end of
the QMP connection to tell in advance whether it's going to get
RTC_CHANGE events then we should probably have an event or
something for that, and make all rtc-change aware RTC devices
cause QMP to send that event on startup (or otherwise register
themselves as being present).

It also means that now rtc devices that emit this event need to
change in meson.build from softmmu_ss to specific_ss, because the
qapi_event_send_rtc_change() prototype is in the generated
qapi/qapi-events-misc-target.h header, and that header uses
TARGET_* defines which are poisoned for softmmu compiles.
So instead of being able to build the RTC device once for
all targets, we need to build it over and over again for
each target.

Could we reconsider this change? It seems to me to be adding
complexity and build time and I don't really see the advantage
that compensates for that.

-- PMM

[PATCH 3/3] linux-aio: add `dev_max_batch` parameter to laio_io_unplug()

[Stefano Garzarella]

Between the submission of a request and the unplug, other devices
with larger limits may have been queued new requests without flushing
the batch.

Using the new `dev_max_batch` parameter, laio_io_unplug() can check
if the batch exceeds the device limit to flush the current batch.

Signed-off-by: Stefano Garzarella 
---
 include/block/raw-aio.h | 3 ++-
 block/file-posix.c  | 2 +-
 block/linux-aio.c   | 8 +---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index ebd042fa27..21fc10c4c9 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -56,7 +56,8 @@ int coroutine_fn laio_co_submit(BlockDriverState *bs, 
LinuxAioState *s, int fd,
 void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
 void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
 void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s);
+void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
+uint64_t dev_max_batch);
 #endif
 /* io_uring.c - Linux io_uring implementation */
 #ifdef CONFIG_LINUX_IO_URING
diff --git a/block/file-posix.c b/block/file-posix.c
index 614c725235..f31a75a715 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2125,7 +2125,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
 #ifdef CONFIG_LINUX_AIO
 if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
-laio_io_unplug(bs, aio);
+laio_io_unplug(bs, aio, s->aio_max_batch);
 }
 #endif
 #ifdef CONFIG_LINUX_IO_URING
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 88b44fee72..f53ae72e21 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -356,11 +356,13 @@ void laio_io_plug(BlockDriverState *bs, LinuxAioState *s)
 s->io_q.plugged++;
 }
 
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s)
+void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
+uint64_t dev_max_batch)
 {
 assert(s->io_q.plugged);
-if (--s->io_q.plugged == 0 &&
-!s->io_q.blocked && !QSIMPLEQ_EMPTY(>io_q.pending)) {
+if (s->io_q.in_queue >= laio_max_batch(s, dev_max_batch) ||
+(--s->io_q.plugged == 0 &&
+ !s->io_q.blocked && !QSIMPLEQ_EMPTY(>io_q.pending))) {
 ioq_submit(s);
 }
 }
-- 
2.31.1

Re: [PATCH 2/2] hmp: Drop a bogus sentence from set_password's documentation

[Laurent Vivier]

Le 09/09/2021 à 10:12, Markus Armbruster a écrit :
> Signed-off-by: Markus Armbruster 
> ---
>  hmp-commands.hx | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8e45bce2cd..cf723c69ac 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1522,12 +1522,11 @@ ERST
>  
>  SRST
>  ``set_password [ vnc | spice ] password [ action-if-connected ]``
> -  Change spice/vnc password.  Use zero to make the password stay valid
> -  forever.  *action-if-connected* specifies what should happen in
> -  case a connection is established: *fail* makes the password change
> -  fail.  *disconnect* changes the password and disconnects the
> -  client.  *keep* changes the password and keeps the connection up.
> -  *keep* is the default.
> +  Change spice/vnc password.  *action-if-connected* specifies what
> +  should happen in case a connection is established: *fail* makes the
> +  password change fail.  *disconnect* changes the password and
> +  disconnects the client.  *keep* changes the password and keeps the
> +  connection up.  *keep* is the default.
>  ERST
>  
>  {
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv

[Vladimir Sementsov-Ogievskiy]

23.09.2021 03:16, John Snow wrote:

We can drop the sys.path hacking in various places by doing
this. Additionally, by doing it in one place right up top, we can print
interesting warnings in case the environment does not look correct.

If we ever decide to change how the environment is crafted, all of the
"help me find my python packages" goop is all in one place, right in one
function.

Signed-off-by: John Snow 


Hurrah!!

Reviewed-by: Vladimir Sementsov-Ogievskiy 


---
  tests/qemu-iotests/235|  2 --
  tests/qemu-iotests/297|  6 --
  tests/qemu-iotests/300|  7 +++
  tests/qemu-iotests/iotests.py |  2 --
  tests/qemu-iotests/testenv.py | 14 +-
  tests/qemu-iotests/tests/mirror-top-perms |  7 +++
  6 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index 8aed45f9a76..4de920c3801 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -24,8 +24,6 @@ import os
  import iotests
  from iotests import qemu_img_create, qemu_io, file_path, log
  
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))

-
  from qemu.machine import QEMUMachine
  
  iotests.script_initialize(supported_fmts=['qcow2'])

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b04cba53667..467b712280e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,12 +68,6 @@ def run_linters():
  # Todo notes are fine, but fixme's or xxx's should probably just be
  # fixed (in tests, at least)
  env = os.environ.copy()
-qemu_module_path = os.path.join(os.path.dirname(__file__),
-'..', '..', 'python')
-try:
-env['PYTHONPATH'] += os.pathsep + qemu_module_path
-except KeyError:
-env['PYTHONPATH'] = qemu_module_path
  subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
 env=env, check=False)
  
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300

index fe94de84edd..10f9f2a8da6 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,12 +24,11 @@ import random
  import re
  from typing import Dict, List, Optional
  
-import iotests

-
-# Import qemu after iotests.py has amended sys.path
-# pylint: disable=wrong-import-order
  from qemu.machine import machine
  
+import iotests

+
+
  BlockBitmapMapping = List[Dict[str, object]]
  
  mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce06cf56304..b06ad76e0c5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -36,8 +36,6 @@
  
  from contextlib import contextmanager
  
-# pylint: disable=import-error, wrong-import-position

-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
  from qemu.machine import qtest
  from qemu.qmp import QMPMessage
  
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py

index 70da0d60c80..88104dace90 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -108,12 +108,16 @@ def init_directories(self) -> None:
   SAMPLE_IMG_DIR
   OUTPUT_DIR
  """
+
+# Path where qemu goodies live in this source tree.
+qemu_srctree_path = Path(__file__, '../../../python').resolve()
+
  self.pythonpath = os.getenv('PYTHONPATH')
-if self.pythonpath:
-self.pythonpath = self.source_iotests + os.pathsep + \
-self.pythonpath
-else:
-self.pythonpath = self.source_iotests
+self.pythonpath = os.pathsep.join(filter(None, (
+self.source_iotests,
+str(qemu_srctree_path),
+self.pythonpath,
+)))


That was simple:)

Hmm, after you this you have

self.pythonpath = ...
self.pythonpath = something other


If have to resend, you may just use os.getenv('PYTHONPATH') inside filter(), it 
seems to be even nicer.

  
  self.test_dir = os.getenv('TEST_DIR',

os.path.join(os.getcwd(), 'scratch'))
diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index 2fc8dd66e0a..73138a0ef91 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -20,13 +20,12 @@
  #
  
  import os

+
+import qemu
+
  import iotests
  from iotests import qemu_img
  
-# Import qemu after iotests.py has amended sys.path

-# pylint: disable=wrong-import-order
-import qemu
-
  
  image_size = 1 * 1024 * 1024

  source = os.path.join(iotests.test_dir, 'source.img')




--
Best regards,
Vladimir

Re: [PATCH 5/6] iotests/migrate-bitmaps-test: delint

[Vladimir Sementsov-Ogievskiy]

23.09.2021 03:16, John Snow wrote:

Mostly uninteresting stuff. Move the test injections under a function
named main() so that the variables used during that process aren't in
the global scope.

Signed-off-by: John Snow
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Hanna Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir