Re: [Qemu-devel] [PATCH] hw/pci-host/ppce500: Use DeviceState::realize rather than SysBusDevice::init
On 10/2/18 11:53 PM, Philippe Mathieu-Daudé wrote:
> Move from the legacy SysBusDevice::init method to using DeviceState::realize.
>
> Signed-off-by: Philippe Mathieu-Daudé
That patch was already reviewed by you and merged by David :)
C.
> ---
> hw/pci-host/ppce500.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index eb75e080fc..b8f8c112e6 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -436,8 +436,9 @@ static AddressSpace *e500_pcihost_set_iommu(PCIBus *bus,
> void *opaque,
> return >bm_as;
> }
>
> -static int e500_pcihost_initfn(SysBusDevice *dev)
> +static void e500_pcihost_realize(DeviceState *dev, Error **errp)
> {
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> PCIHostState *h;
> PPCE500PCIState *s;
> PCIBus *b;
> @@ -447,7 +448,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> s = PPC_E500_PCI_HOST_BRIDGE(dev);
>
> for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> -sysbus_init_irq(dev, >irq[i]);
> +sysbus_init_irq(sbd, >irq[i]);
> }
>
> for (i = 0; i < PCI_NUM_PINS; i++) {
> @@ -460,7 +461,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> /* PIO lives at the bottom of our bus space */
> memory_region_add_subregion_overlap(>busmem, 0, >pio, -2);
>
> -b = pci_register_root_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> +b = pci_register_root_bus(dev, NULL, mpc85xx_pci_set_irq,
>mpc85xx_pci_map_irq, s, >busmem, >pio,
>PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
> h->bus = b;
> @@ -483,10 +484,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> memory_region_add_subregion(>container, PCIE500_CFGADDR,
> >conf_mem);
> memory_region_add_subregion(>container, PCIE500_CFGDATA,
> >data_mem);
> memory_region_add_subregion(>container, PCIE500_REG_BASE, >iomem);
> -sysbus_init_mmio(dev, >container);
> +sysbus_init_mmio(sbd, >container);
> pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
> -
> -return 0;
> }
>
> static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
> @@ -526,9 +525,8 @@ static Property pcihost_properties[] = {
> static void e500_pcihost_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> -k->init = e500_pcihost_initfn;
> +dc->realize = e500_pcihost_realize;
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> dc->props = pcihost_properties;
> dc->vmsd = _ppce500_pci;
>
Re: [Qemu-devel] [RFC PATCH 0/3] acceptance tests: Test firmware checking debug console output
On 9/28/18 6:51 AM, Laszlo Ersek wrote:
> Hi Phil,
>
> (+Daniel, +Kashyap)
>
> On 09/28/18 02:30, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> This RFC series add simple acceptance tests which boot SeaBIOS and
>> EDK2 on Q35 and virt/aarch64.
>>
>> It is more of a proof of concept (to motivate the Avocado team ;) ).
>>
>> Regards,
>>
>> Phil.
>>
>> Philippe Mathieu-Daudé (3):
>> acceptance tests: Add SeaBIOS boot and debug console checking test
>> acceptance tests: Add EDK2 OVMF boot and debug console checking test
>> acceptance tests: Add EDK2 AAVMF boot and console checking test
>>
>> tests/acceptance/boot_firmware.py | 167 ++
>> 1 file changed, 167 insertions(+)
>> create mode 100644 tests/acceptance/boot_firmware.py
>>
>
> I'm not experienced with Avocado, so I'm basically reading the patches
> as a "story". My comments are made at that level too. :)
>
> * In the blurb, you say "Q35". But the first two patches have
>
> vm.set_machine('pc')
>
> * Please don't call the edk2 ArmVirtQemu platform AAVMF in upstream
> patches :) Call it ArmVirtQemu pls.
>
> * Finding the right way to launch OVMF and/or ArmVirtQemu firmware
> images is complicated. (The right way is definitely not "-bios"!)
>
> The general idea is that you need three files (and two pflash chips);
> (a) a firmware executable mapped read-only, and (b) a variable store
> file, mapped read-write, that was first copied from (c) a read-only
> variable store *template* that is never itself mapped. And, this is
> not the whole story.
>
> Figuring out the options is complicated enough (for management tools
> as well) that Daniel made us define a metadata schema for describing
> firmware packages. Please see:
>
> docs/interop/firmware.json
>
> I'm not necessarily suggesting that Avocado be able to parse the
> firmware descriptor metafiles that conform to this schema. I'm just
> pointing out that the QEMU command line will depend on the exact build
> of the firmware image under test. The pathname
> "/usr/share/OVMF/OVMF_CODE.fd" and the URL
> "https://snapshots.linaro.org/.../QEMU_EFI.img.gz; don't give us
> enough information.
>
> Therefore, if we want to keep the test case simple (= hard-wire the
> command lines), then we'll have to refer to OVMF and ArmVirtQemu
> images with precisely known build configs.
>
> * Looking for debug console messages as "vital signs" is brittle. For
> example, the line "DetectSmbiosVersion: SMBIOS version from QEMU:
> 0x0208" will change if QEMU changes the SMBIOS version number in the
> SMBIOS anchor that it generates. It's likely better to make the
> firmware "do" something.
>
> The simplest I can imagine is: prepare a virtual disk with a
> "startup.nsh" UEFI shell script on it. The script can print a known
> fixed string, and then power down the VM. (See the UEFI Shell
> Specification for commands; .)
>
> I'm not sure if Avocado provides disk image preparation utilities, but
> perhaps (a) we could use the vvfat driver (*shudder*) or (b) we could
> preformat a small image, and track it as a binary file in git.
>
So far we've added support for generating ISO images (with pure Python).
I'm not sure if that's useful here. We can think about trying to add
the same thing for vvfat.
- Cleber.
Re: [Qemu-devel] [RFC PATCH 3/3] acceptance tests: Add EDK2 AAVMF boot and console checking test
On 9/27/18 8:30 PM, Philippe Mathieu-Daudé wrote:
> This test boots EDK2 AAVMF and check the debug console (PL011) reports enough
> information on the initialized devices.
>
> Example:
>
> $ avocado --show=console run tests/acceptance/boot_firmware.py
> --cit-parameter-file aarch64.cit
>
> having aarch64.cit:
>
> [parameters]
> qemu_bin: aarch64-softmmu/qemu-system-aarch64
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> - requires (even for single parameter):
> $ pip install avocado-framework-plugin-varianter-cit
>
Avocado 65.0 now features a core parameter passing option (one that
doesn't depend on the varianter). Command line option is documented as:
-p NAME_VALUE, --test-parameter NAME_VALUE
Parameter name and value to pass to all tests. This is
only applicable when not using a varianter plugin.
This option format must be given in the NAME=VALUE
format, and may be given any number of times, or per
parameter.
> - use gzip kludge (avocado only support tarballs)
>
I'll finish the work to get that into the avocado.utils.archive module.
> - can not set $arch param since pick_default_qemu_bin() forces to host
> os.uname()[4]
This is definitely a common requirement, and something to be addressed
ASAP. I have a patch on my tree (pretty simple change) that I'll be
sending shortly.
> ---
> tests/acceptance/boot_firmware.py | 43 +++
> 1 file changed, 43 insertions(+)
>
> diff --git a/tests/acceptance/boot_firmware.py
> b/tests/acceptance/boot_firmware.py
> index 05f6728212..4f5554c0ad 100644
> --- a/tests/acceptance/boot_firmware.py
> +++ b/tests/acceptance/boot_firmware.py
> @@ -10,6 +10,7 @@
> # SPDX-License-Identifier: GPL-2.0-or-later
>
> import os
> +import gzip
> import time
> import shutil
> import logging
> @@ -122,3 +123,45 @@ class BootFirmwareX86(Test):
> if line + '\r\n' not in lines:
> self.fail('missing: %s' % line)
> shutil.rmtree(tmpdirname, ignore_errors=True)
> +
> +
> +class BootFirmwareAarch64(Test):
> +"""
> +Boots the EDK2 firmware on a default virt machine and checks the console
> is
> +operational
> +
> +:avocado: enable
> +:avocado: tags=arch:aarch64
Yay, I like where you're going here. This matches what I said before
about having some standardization on the tags.
On a related topic, sometimes tests can be written in an abstract way
that parameters only will allow the same test to be reused in different
arches, but, sometimes we may need to explicit include/exclude some
tests on some executions - then tags could be a foundation for that.
> +:avocado: tags=aarch64,quick
> +"""
> +
> +timeout = 15
> +
> +def test_aavmf(self):
> +tmpdirname = tempfile.mkdtemp()
> +image_url = ('http://snapshots.linaro.org/components/kernel/'
> +'leg-virt-tianocore-edk2-upstream/latest/'
> +'QEMU-AARCH64/DEBUG_GCC5/QEMU_EFI.img.gz')
> +image_path_gz = self.fetch_asset(image_url)
> +image_path = os.path.join(tmpdirname, 'flash.img')
> +with gzip.open(image_path_gz) as gz, open(image_path, 'wb') as img:
> +shutil.copyfileobj(gz, img)
> +
> +self.vm.set_machine('virt')
> +self.vm.set_console()
> +self.vm.add_args('-nographic',
> + '-cpu', 'cortex-a57',
> + '-m', '1G',
Is there a reason for defining this exact amount of memory? My question
has to do with the idea of setting some "sane" hardware defaults at the
base test level (to avoid boiler plate code).
> + '-pflash', image_path)
> +self.vm.launch()
> +console = self.vm.console_socket.makefile()
> +serial_logger = logging.getLogger('serial')
> +
> +# serial console checks
> +while True:
> +msg = console.readline()
> +if not '\x1b' in msg: # ignore ANSI sequences
> +serial_logger.debug(msg.strip())
> +if 'Start PXE over IPv4InstallProtocolInterface:' in msg:
> +break
> +shutil.rmtree(tmpdirname, ignore_errors=True)
>
Again, seems like a candidate for the ideas presented on the previous
patches.
Thanks for sending those, lots of interesting opportunities.
- Cleber.
Re: [Qemu-devel] [RFC PATCH 2/3] acceptance tests: Add EDK2 OVMF boot and debug console checking test
On 9/27/18 8:30 PM, Philippe Mathieu-Daudé wrote:
> This test boots OVMF and check the debug console (I/O port on the ISA bus)
> report enough information on the initialized devices.
>
> Example:
>
> $ avocado --show=QMP,serial run tests/acceptance/boot_firmware.py
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> - how to refactor common code from test_seabios() (previous patch)?
> ---
> tests/acceptance/boot_firmware.py | 52 +++
> 1 file changed, 52 insertions(+)
>
> diff --git a/tests/acceptance/boot_firmware.py
> b/tests/acceptance/boot_firmware.py
> index a41e04936d..05f6728212 100644
> --- a/tests/acceptance/boot_firmware.py
> +++ b/tests/acceptance/boot_firmware.py
> @@ -70,3 +70,55 @@ class BootFirmwareX86(Test):
> if line + '\n' not in lines:
> self.fail('missing: %s' % line)
> shutil.rmtree(tmpdirname, ignore_errors=True)
Almost every comment on the previous patch applies here of course.
Because of that, it may make sense to generalize some of these patterns
into utility methods and/or, move some common setup code into a setUp()
method.
> +
> +def test_ovmf(self):
> +tmpdirname = tempfile.mkdtemp()
> +debugcon_path = os.path.join(tmpdirname, 'debugconsole.log')
> +serial_logger = logging.getLogger('serial')
> +debugcon_logger = logging.getLogger('debugcon')
> +
> +self.vm.set_machine('pc')
> +self.vm.set_console()
> +self.vm.add_args('-nographic',
> + '-net', 'none',
> + '-global', 'isa-debugcon.iobase=0x402',
> + '-debugcon', 'file:%s' % debugcon_path,
For instance, the setting up of the machine type, console and these
common arguments seem to be good candidates to be moved to a common
setUp() method.
> + '--bios', '/usr/share/OVMF/OVMF_CODE.fd')
This would be the difference between the arguments of this and the
seabios test, so it could be given by itself here (with the rest on
setUp()).
> +self.vm.launch()
> +console = self.vm.console_socket.makefile()
> +
> +# serial console checks
> +timeout = time.time() + 15
> +while True:
> +msg = console.readline()
> +if not '\x1b' in msg: # ignore ANSI sequences
> +serial_logger.debug(msg.strip())
> +if 'EDK II' in msg:
> +break
> +if time.time() > timeout:
> +self.fail('OVMF failed to boot?')
> +
This seems to be a code block that could become something like:
def check_boot(self, pattern, timeout):
...
self.fail("Failed to boot")
IMO, a generic fail reason is enough, given that it's associated with
the "test_ovmf".
> +# debug console checks
> +expected = [
> +'SEC: Normal boot',
> +'S3 support was detected on QEMU',
> +'Platform PEI Firmware Volume Initialization',
> +'DXE IPL Entry',
> +'Installing FVB for EMU Variable support',
> +'DetectSmbiosVersion: SMBIOS version from QEMU: 0x0208',
> +'SmbiosCreateTable: Initialize 32-bit entry point structure',
> +'PlatformBootManagerBeforeConsole',
> +'OnRootBridgesConnected: root bridges have been connected, '
> +'installing ACPI tables',
> +'Found LPC Bridge device',
> +'PlatformBootManagerAfterConsole',
> +'EfiBootManagerConnectAll',
> +'[Bds]Booting EFI Internal Shell',
> +]
> +lines = open(debugcon_path).readlines()
> +for msg in lines:
> +debugcon_logger.debug(msg.strip())
> +for line in expected:
> +if line + '\r\n' not in lines:
> +self.fail('missing: %s' % line)
> +shutil.rmtree(tmpdirname, ignore_errors=True)
>
And this could also become something like:
def check_console(self, patterns):
with open(self.test_logs["debugcon"]) as debugcon:
content = debugcon.readlines()
for expected in patterns:
self.assertIn(expected, content)
Assuming "self.test_logs" is a byproduct of the "self.log_file()"
suggestion for the new feature given previously.
Re: [Qemu-devel] [RFC PATCH 1/3] acceptance tests: Add SeaBIOS boot and debug console checking test
On 10/3/18 1:26 AM, Cleber Rosa wrote: [...] >> +:avocado: enable >> +:avocado: tags=x86_64,quick > > Eventually, we'll need to have a predictable definition to, at least, > some of the tags. I mean, I agree this *should* be a quick by test > (considering it's a functional test), but eventually we may want to set > some ranges for what we consider "quick", "slow" or "I'm gonna take my > time, don't wait on me". :) Maybe we can set a timeout tag and let the user choose how long he is willing to wait: :avocado: max_time=20s >> +""" >> + >> +timeout = 15 >> + >> +def test_seabios(self): [...]
Re: [Qemu-devel] [PATCH v5 1/9] hw/core/clock-port: introduce clock port objects
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Introduce clock port objects: ClockIn and ClockOut.
>
> Theses ports may be used to distribute a clock from a object to several
> other objects. The ClockIn object contains the current state of the
> clock: the frequency.
>
> A ClockIn may be connected to a ClockOut so that it receives update,
> through the callback, whenever the Clockout is updated using the
> ClockOut's set function.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde
> ---
> Makefile.objs | 1 +
> include/hw/clock-port.h | 136 ++
> hw/core/clock-port.c| 159
> hw/core/Makefile.objs | 1 +
> hw/core/trace-events| 7 ++
> 5 files changed, 304 insertions(+)
> create mode 100644 include/hw/clock-port.h
> create mode 100644 hw/core/clock-port.c
> create mode 100644 hw/core/trace-events
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ce9c79235e..b29747075f 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -210,6 +210,7 @@ trace-events-subdirs += hw/audio
> trace-events-subdirs += hw/block
> trace-events-subdirs += hw/block/dataplane
> trace-events-subdirs += hw/char
> +trace-events-subdirs += hw/core
> trace-events-subdirs += hw/display
> trace-events-subdirs += hw/dma
> trace-events-subdirs += hw/hppa
> diff --git a/include/hw/clock-port.h b/include/hw/clock-port.h
> new file mode 100644
> index 00..8266549350
> --- /dev/null
> +++ b/include/hw/clock-port.h
> @@ -0,0 +1,136 @@
> +#ifndef CLOCK_PORT_H
> +#define CLOCK_PORT_H
> +
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "qemu/queue.h"
> +#include "migration/vmstate.h"
> +
> +#define TYPE_CLOCK_IN "clock-in"
> +#define CLOCK_IN(obj) OBJECT_CHECK(ClockIn, (obj), TYPE_CLOCK_IN)
> +#define TYPE_CLOCK_OUT "clock-out"
> +#define CLOCK_OUT(obj) OBJECT_CHECK(ClockOut, (obj), TYPE_CLOCK_OUT)
> +
> +typedef void ClockCallback(void *opaque);
> +
> +typedef struct ClockOut ClockOut;
> +typedef struct ClockIn ClockIn;
> +
> +struct ClockIn {
> +/*< private >*/
> +Object parent_obj;
> +/*< private >*/
> +uint64_t frequency;
> +char *canonical_path; /* clock path cache */
> +ClockOut *driver; /* clock output controlling this clock */
> +ClockCallback *callback; /* local callback */
> +void *callback_opaque; /* opaque argument for the callback */
> +QLIST_ENTRY(ClockIn) sibling; /* entry in a followers list */
> +};
> +
> +struct ClockOut {
> +/*< private >*/
> +Object parent_obj;
> +/*< private >*/
> +char *canonical_path; /* clock path cache */
> +QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
> +};
> +
> +extern const VMStateDescription vmstate_clockin;
> +
> +/*
> + * vmstate description entry to be added in device vmsd.
> + */
> +#define VMSTATE_CLOCKIN(_field, _state) \
> +VMSTATE_CLOCKIN_V(_field, _state, 0)
> +#define VMSTATE_CLOCKIN_V(_field, _state, _version) \
> +VMSTATE_STRUCT_POINTER_V(_field, _state, _version, vmstate_clockin,
> ClockIn)
> +
> +/**
> + * clock_out_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_out_setup_canonical_path(ClockOut *clk);
> +
> +/**
> + * clock_in_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_in_setup_canonical_path(ClockIn *clk);
> +
> +/**
> + * clock_add_callback:
> + * @clk: the clock to register the callback into
> + * @cb: the callback function
> + * @opaque: the argument to the callback
> + *
> + * Register a callback called on every clock update.
> + */
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque);
> +
> +/**
> + * clock_clear_callback:
> + * @clk: the clock to delete the callback from
> + *
> + * Unregister the callback registered with clock_set_callback.
> + */
> +void clock_clear_callback(ClockIn *clk);
> +
> +/**
> + * clock_init_frequency:
> + * @clk: the clock to initialize.
> + * @freq: the clock's frequency in Hz or 0 if unclocked.
> + *
> + * Initialize the local cached frequency value of @clk to @freq.
> + * Note: this function must only be called during device inititialization
> + * or migration.
> + */
> +void clock_init_frequency(ClockIn *clk, uint64_t freq);
> +
> +/**
> + * clock_connect:
> + * @clkin: the drived clock.
> + * @clkout: the driving clock.
> + *
> + * Setup @clkout to drive @clkin: Any @clkout update will be propagated
> + * to @clkin.
> + */
> +void clock_connect(ClockIn *clkin, ClockOut *clkout);
> +
> +/**
> + * clock_set_frequency:
> + * @clk: the clock to update.
> + * @freq: the new clock's frequency in Hz or 0 if unclocked.
> + *
> + * Update the @clk to the new @freq.
> + * This change will be propagated through registered clock inputs.
> + */
> +void clock_set_frequency(ClockOut *clk, uint64_t
Re: [Qemu-devel] [PATCH v5 5/9] docs/clocks: add device's clock documentation
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add the documentation about the clock inputs and outputs in devices.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde
> ---
> docs/devel/clock.txt | 163 +++
> 1 file changed, 163 insertions(+)
> create mode 100644 docs/devel/clock.txt
>
> diff --git a/docs/devel/clock.txt b/docs/devel/clock.txt
> new file mode 100644
> index 00..6dd8abdee6
> --- /dev/null
> +++ b/docs/devel/clock.txt
> @@ -0,0 +1,163 @@
> +
> +What are device's clocks
> +
> +
> +Clocks are ports representing input and output clocks of a device. They are
> QOM
> +objects developed for the purpose of modeling the distribution of clocks in
> +QEMU.
> +
> +This allows us to model the clock distribution of a platform and detect
> +configuration errors in the clock tree such as badly configured PLL, clock
> +source selection or disabled clock.
> +
> +The objects are CLOCK_IN for the input and CLOCK_OUT for the output.
A simple ASCII diagram would help to make this clearer.
> +
> +The clock value: ClockState
> +===
> +
> +The ClockState is the structure carried by the CLOCK_OUT and CLOCK_IN
> objects.
This is only true for ClockIn, right?
> +It contains one integer field representing the frequency of the clock in
> Hertz.
unsigned
> +
> +It only simulates the clock by transmitting the frequency value and
> +doesn't model the signal itself such as pin toggle or duty cycle.
> +The special value 0 as a frequency is legal and represent the clock being
> +inactive or gated.
OK.
> +
> +Adding clocks to a device
> +=
> +
> +Adding clocks to a device must be done during the init phase of the Device
> +object.
> +
> +To add an input clock to a device, the function qdev_init_clock_in must be
> used.
> +It takes the name, a callback, and an opaque parameter for the clock.
> +Output is more simple, only the name is required. Typically:
> +qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
> +qdev_init_clock_out(DEVICE(dev), "clk-out");
> +
> +Both functions return the created CLOCK_IN/OUT pointer, which should be saved
ClockIn/ClockOut
> +in the device's state structure.
> +
> +Theses objects will be automatically deleted by the qom reference mechanism.
QOM
> +
> +Note that it is possible to create a static array describing clock inputs and
> +outputs. The function qdev_init_clocks must be called with the array as
> +parameters to initialize the clocks: it has the same behaviour as calling the
> +qdev_init_clock/out for each clock in the array.
> +
> +Unconnected input clocks
> +
> +
> +Unconnected input clocks have a default frequency value of 0. It means the
> +clock will be considered as disabled. If this is not the wanted behaviour,
> +clock_init_frequency should be called on the ClockIn object during device
> init.
> +For example:
> +clk = qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback, dev);
> +clock_init_frequency(clk, 100 * 1000 * 1000); // init value is 100Mhz
> +
> +Forwarding clocks
> +=
> +
> +Sometimes, one needs to forward, or inherit, a clock from another device.
> +Typically, when doing device composition, a device might expose a
> sub-device's
> +clock without interfering with it.
> +The function qdev_pass_clock can be used to achieve this behaviour. Note,
> that
> +it is possible to expose the clock under a different name. This works for
> both
> +inputs or outputs.
> +
> +For example, if device B is a child of device A, device_a_instance_init may
> +do something like this:
> +void device_a_instance_init(Object *obj)
> +{
> +AState *A = DEVICE_A(obj);
> +BState *B;
> +[...] /* create B object as child of A */
> +qdev_pass_clock(A, "b_clk", B, "clk");
> +/*
> + * Now A has a clock "b_clk" which forwards to
> + * the "clk" of its child B.
> + */
> +}
> +
> +This function does not returns any clock object. It is not possible to add
> +a callback on a forwarded input clock.
> +
> +Connecting two clocks together
> +==
> +
> +Let's say we have 2 devices A and B. A has an output clock named "clkout"
> and B
> +has an input clock named "clkin".
Maybe "clk_in" "clk_out" to avoid confusion with ClkIn and ClkOut.
> +
> +The clocks are connected together using the function qdev_connect_clock:
> +qdev_connect_clock(B, "clkin", A, "clkout", _abort);
> +The device which has the input must be the first argument.
> +
> +It is possible to connect several input clocks to the same output. Every
> +input callback will be called when the output changes.
> +
> +It is not possible to disconnect a clock or to change the clock connection
> +after it is done.
> +
> +Changing a clock output
> +===
> +
> +A device can change its outputs using the clock_set function. It will trigger
Re: [Qemu-devel] [PATCH v5 2/9] qdev: add clock input support to devices.
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add functions to easily add input or output clocks to a device.
> The clock port objects are added as children of the device.
>
> A function allows to connect two clocks together.
> It should be called by some toplevel to make a connection between
> 2 (sub-)devices.
>
> Also add a function which forwards a port to another device. This
> function allows, in the case of device composition, to expose a
> sub-device clock port as its own clock port.
> This is really an alias: when forwarding an input, only one callback can
> be registered on it since there is only one Clockin object behind all
> aliases.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde
> ---
> include/hw/qdev-clock.h | 62 ++
> include/hw/qdev-core.h | 14
> include/hw/qdev.h | 1 +
> hw/core/qdev-clock.c| 140
> hw/core/qdev.c | 29 +
> hw/core/Makefile.objs | 2 +-
> 6 files changed, 247 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/qdev-clock.h
> create mode 100644 hw/core/qdev-clock.c
>
> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
> new file mode 100644
> index 00..d76aa9f479
> --- /dev/null
> +++ b/include/hw/qdev-clock.h
> @@ -0,0 +1,62 @@
> +#ifndef QDEV_CLOCK_H
> +#define QDEV_CLOCK_H
> +
> +#include "hw/clock-port.h"
> +#include "hw/qdev-core.h"
> +#include "qapi/error.h"
> +
> +/**
> + * qdev_init_clock_in:
> + * @dev: the device in which to add a clock
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.
> + * @opaque: argument for the callback
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a input clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + * The callback will be called with @dev as opaque parameter.
> + */
> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
> +ClockCallback *callback, void *opaque);
> +
> +/**
> + * qdev_init_clock_out:
> + * @dev: the device to add a clock to
> + * @name: the name of the clock (can't be NULL).
> + * @callback: optional callback to be called on update or NULL.
> + * @returns: a pointer to the newly added clock
> + *
> + * Add a output clock to device @dev as a clock named @name.
> + * This adds a child<> property.
> + */
> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name);
> +
> +/**
> + * qdev_pass_clock:
> + * @dev: the device to forward the clock to
> + * @name: the name of the clock to be added (can't be NULL)
> + * @container: the device which already has the clock
> + * @cont_name: the name of the clock in the container device
> + *
> + * Add a clock @name to @dev which forward to the clock @cont_name in
> @container
> + */
> +void qdev_pass_clock(DeviceState *dev, const char *name,
> +DeviceState *container, const char *cont_name);
Indent ;)
> +
> +/**
> + * qdev_connect_clock:
> + * @dev: the drived clock device.
> + * @name: the drived clock name.
> + * @driver: the driving clock device.
> + * @driver_name: the driving clock name.
> + * @errp: error report
> + *
> + * Setup @driver_name output clock of @driver to drive @name input clock of
> + * @dev. Errors are trigerred if clock does not exists
> + */
> +void qdev_connect_clock(DeviceState *dev, const char *name,
> +DeviceState *driver, const char *driver_name,
> +Error **errp);
> +
> +#endif /* QDEV_CLOCK_H */
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index f1fd0f8736..e6014d3a41 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -127,6 +127,19 @@ struct NamedGPIOList {
> QLIST_ENTRY(NamedGPIOList) node;
> };
>
> +typedef struct NamedClockList NamedClockList;
> +
> +typedef struct ClockIn ClockIn;
> +typedef struct ClockOut ClockOut;
> +
> +struct NamedClockList {
> +char *name;
> +bool forward;
> +ClockIn *in;
> +ClockOut *out;
> +QLIST_ENTRY(NamedClockList) node;
> +};
> +
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> @@ -147,6 +160,7 @@ struct DeviceState {
> int hotplugged;
> BusState *parent_bus;
> QLIST_HEAD(, NamedGPIOList) gpios;
> +QLIST_HEAD(, NamedClockList) clocks;
> QLIST_HEAD(, BusState) child_bus;
> int num_child_bus;
> int instance_id_alias;
> diff --git a/include/hw/qdev.h b/include/hw/qdev.h
> index 5cb8b080a6..b031da7b41 100644
> --- a/include/hw/qdev.h
> +++ b/include/hw/qdev.h
> @@ -4,5 +4,6 @@
> #include "hw/hw.h"
> #include "hw/qdev-core.h"
> #include "hw/qdev-properties.h"
> +#include "hw/qdev-clock.h"
>
> #endif
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> new file mode 100644
> index 00..f0e4839aed
> --- /dev/null
> +++
[Qemu-devel] [PATCH v2] bitmap: Update count after a merge
From: Eric Blake We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C. In the worst case, we may falsely regard the bitmap as empty when it has had new writes merged into it. Fixes: be58721db CC: qemu-sta...@nongnu.org Signed-off-by: Eric Blake Signed-off-by: John Snow --- v2: based off of Eric's cover letter, now rebased properly on top of the jsnow/bitmaps staging branch to use the correct bitmap target (result). util/hbitmap.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/hbitmap.c b/util/hbitmap.c index d5aca5159f..8d402c59d9 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -759,6 +759,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) } } +/* Recompute the dirty count */ +result->count = hb_count_between(result, 0, result->size - 1); + return true; } -- 2.14.4
Re: [Qemu-devel] [PATCH v5 9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add the connection between the slcr's output clocks and the uarts inputs.
>
> Signed-off-by: Damien Hedde
> ---
> hw/arm/xilinx_zynq.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index f1496d2927..88f61c6a18 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -166,7 +166,7 @@ static void zynq_init(MachineState *machine)
> MemoryRegion *address_space_mem = get_system_memory();
> MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
> MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
> -DeviceState *dev;
> +DeviceState *dev, *slcr;
> SysBusDevice *busdev;
> qemu_irq pic[64];
> int n;
> @@ -212,9 +212,10 @@ static void zynq_init(MachineState *machine)
>1, 0x0066, 0x0022, 0x, 0x, 0x0555, 0x2aa,
>0);
>
> -dev = qdev_create(NULL, "xilinx,zynq_slcr");
> -qdev_init_nofail(dev);
> -sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF800);
> +/* Create slcr, keep a pointer to connect clocks */
> +slcr = qdev_create(NULL, "xilinx,zynq_slcr");
> +qdev_init_nofail(slcr);
> +sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF800);
>
> dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
> qdev_prop_set_uint32(dev, "num-cpu", 1);
> @@ -235,8 +236,12 @@ static void zynq_init(MachineState *machine)
> sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
> sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
>
> -cadence_uart_create(0xE000, pic[59 - IRQ_OFFSET], serial_hd(0));
> -cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
> +dev = cadence_uart_create(0xE000, pic[59 - IRQ_OFFSET],
> serial_hd(0));
> +qdev_connect_clock(dev, "busclk", slcr, "uart0_amba_clk", _abort);
> +qdev_connect_clock(dev, "refclk", slcr, "uart0_ref_clk", _abort);
> +dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET],
> serial_hd(1));
> +qdev_connect_clock(dev, "busclk", slcr, "uart1_amba_clk", _abort);
> +qdev_connect_clock(dev, "refclk", slcr, "uart1_ref_clk", _abort);
>
> sysbus_create_varargs("cadence_ttc", 0xF8001000,
> pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET],
> NULL);
>
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
[Qemu-devel] [RFC PATCH 1/3] acceptance tests: Add SeaBIOS boot and debug console checking test
On 9/27/18 8:30 PM, Philippe Mathieu-Daudé wrote:
> This test boots SeaBIOS and check the debug console (I/O port on the ISA bus)
> reports enough information on the initialized devices.
>
> Example:
>
> $ avocado --show=debugcon run tests/acceptance/boot_firmware.py
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> - can we avoid the time.time() 2nd timeout check?
>
The problem here is that the "draining" of the console is done
synchronously and at the same time the "if 'No bootable device.' in msg"
check is performed.
If we split those, it could become a simple function that can be given
to wait_for():
https://avocado-framework.readthedocs.io/en/65.0/api/utils/avocado.utils.html#avocado.utils.wait.wait_for
Something like:
if not wait_for(check_bootable(), 5):
self.fail("Reason")
Greatly simplifying the code.
> - can we avoid tempfile.mkdtemp() + shutil.rmtree() with Python2?
> (using context manager)
Yep, workdir is your friend. More on that later.
> ---
> tests/acceptance/boot_firmware.py | 72 +++
> 1 file changed, 72 insertions(+)
> create mode 100644 tests/acceptance/boot_firmware.py
>
> diff --git a/tests/acceptance/boot_firmware.py
> b/tests/acceptance/boot_firmware.py
> new file mode 100644
> index 00..a41e04936d
> --- /dev/null
> +++ b/tests/acceptance/boot_firmware.py
> @@ -0,0 +1,72 @@
> +# coding=utf-8
> +#
> +# Functional test that boots SeaBIOS and checks the console
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +# Philippe Mathieu-Daudé
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import os
> +import time
> +import shutil
> +import logging
> +import tempfile
> +
> +from avocado_qemu import Test
> +
> +
> +class BootFirmwareX86(Test):
> +"""
> +Boots a firmware on a default PC machine and checks the debug console is
> +operational
> +
> +:avocado: enable
> +:avocado: tags=x86_64,quick
Eventually, we'll need to have a predictable definition to, at least,
some of the tags. I mean, I agree this *should* be a quick by test
(considering it's a functional test), but eventually we may want to set
some ranges for what we consider "quick", "slow" or "I'm gonna take my
time, don't wait on me". :)
> +"""
> +
> +timeout = 15
> +
> +def test_seabios(self):
> +tmpdirname = tempfile.mkdtemp()
Every Avocado test has a "workdir" that can be used for temporary
content. That property points to a directory will be discarded after
the test has finished. Documentation is here:
https://avocado-framework.readthedocs.io/en/65.0/api/test/avocado.html#avocado.Test.workdir
> +debugcon_path = os.path.join(tmpdirname, 'debugconsole.log')
> +serial_logger = logging.getLogger('serial')
> +debugcon_logger = logging.getLogger('debugcon')
> +
> +self.vm.set_machine('pc')
> +self.vm.set_console()
> +self.vm.add_args('-nographic',
> + '-net', 'none',
> + '-global', 'isa-debugcon.iobase=0x402',
> + '-debugcon', 'file:%s' % debugcon_path)
> +self.vm.launch()
> +console = self.vm.console_socket.makefile()
> +
> +# serial console checks
> +timeout = time.time() + 5
> +while True:
> +msg = console.readline()
> +if not '\x1b' in msg: # ignore ANSI sequences
> +serial_logger.debug(msg.strip())
> +if 'No bootable device.' in msg:
> +break
> +if time.time() > timeout:
> +self.fail('SeaBIOS failed to boot?')
It looks like tests such as this could benefit from having an async
method of draining a given file (descriptor or object) and putting that
into a log. Then, the boilerplate code of reading from a file and
writing to a log could be avoided.
In theory, Avocado has such a tool:
https://avocado-framework.readthedocs.io/en/65.0/api/utils/avocado.utils.html#avocado.utils.process.FDDrainer
But it needs some TLC when it comes to being used on a test to really
remove boilerplate code. For instance, instead of having to do:
debugcon_path = os.path.join(self.workdir, 'debug_console.log')
debugcon_logger = logging.getLogger('debugcon')
...
lines = open(debugcon_path).readlines()
for msg in lines:
debugcon_logger.debug(msg.strip())
One should be able to do:
# debug_console.log is created with the content of debugcon_path
self.log_file(debugcon_path, "debug_console")
> +
> +# debug console checks
> +expected = [
> +'Running on QEMU (i440fx)',
> +'Turning on vga text mode console',
> +'Found 1 lpt ports',
> +'Found 1 serial ports',
> +'PS2 keyboard initialized',
> +]
> +lines = open(debugcon_path).readlines()
> +for msg in lines:
> +debugcon_logger.debug(msg.strip())
> +for line in
Re: [Qemu-devel] [PATCH v5 8/9] hw/char/cadence_uart: add clock support
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add bus interface and uart reference clock inputs.
>
> Note: it is hard to find out from the doc what is the behavior when only one
> of the clock is disabled.
>
> The implemented behaviour is that register access needs both clock being
> active.
>
> The bus interface control the mmios visibility
This sentence sounds odd.
>
> The reference clock controls the baudrate generation. If it disabled,
> any input characters and events are ignored. Also register accesses are
> conditioned to the clock being enabled (but is it really the case in
> reality ?) and a guest error is triggerred if that is not the case.
>
> If theses clocks remains unconnected, the uart behaves as before
> (default to 50MHz ref clock).
>
> Signed-off-by: Damien Hedde
> ---
> include/hw/char/cadence_uart.h | 3 ++
> hw/char/cadence_uart.c | 92 --
> hw/char/trace-events | 3 ++
> 3 files changed, 93 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
> index 118e3f10de..fd1d4725f4 100644
> --- a/include/hw/char/cadence_uart.h
> +++ b/include/hw/char/cadence_uart.h
> @@ -21,6 +21,7 @@
> #include "hw/sysbus.h"
> #include "chardev/char-fe.h"
> #include "qemu/timer.h"
> +#include "hw/clock-port.h"
>
> #define CADENCE_UART_RX_FIFO_SIZE 16
> #define CADENCE_UART_TX_FIFO_SIZE 16
> @@ -47,6 +48,8 @@ typedef struct {
> CharBackend chr;
> qemu_irq irq;
> QEMUTimer *fifo_trigger_handle;
> +ClockIn *refclk;
> +ClockIn *busclk;
> } CadenceUARTState;
>
> static inline DeviceState *cadence_uart_create(hwaddr addr,
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index fbdbd463bb..feb5cee4d7 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -28,6 +28,7 @@
> #include "qemu/timer.h"
> #include "qemu/log.h"
> #include "hw/char/cadence_uart.h"
> +#include "trace.h"
>
> #ifdef CADENCE_UART_ERR_DEBUG
> #define DB_PRINT(...) do { \
> @@ -94,7 +95,7 @@
> #define LOCAL_LOOPBACK (0x2 << UART_MR_CHMODE_SH)
> #define REMOTE_LOOPBACK(0x3 << UART_MR_CHMODE_SH)
>
> -#define UART_INPUT_CLK 5000
> +#define UART_DEFAULT_REF_CLK (50 * 1000 * 1000)
>
> #define R_CR (0x00/4)
> #define R_MR (0x04/4)
> @@ -165,15 +166,30 @@ static void uart_send_breaks(CadenceUARTState *s)
>_enabled);
> }
>
> +static unsigned int uart_input_clk(CadenceUARTState *s)
> +{
> +return clock_get_frequency(s->refclk);
> +}
> +
> static void uart_parameters_setup(CadenceUARTState *s)
> {
> QEMUSerialSetParams ssp;
> unsigned int baud_rate, packet_size;
>
> baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
> -UART_INPUT_CLK / 8 : UART_INPUT_CLK;
> +uart_input_clk(s) / 8 : uart_input_clk(s);
> +baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
Safe because s->r[R_BRGR] >= 1, OK.
> +trace_cadence_uart_baudrate(baud_rate);
> +
> +ssp.speed = baud_rate;
> +if (ssp.speed == 0) {
> +/*
> + * Avoid division-by-zero below.
> + * TODO: find something better
> + */
> +ssp.speed = 1;
> +}
>
> -ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
> packet_size = 1;
>
> switch (s->r[R_MR] & UART_MR_PAR) {
> @@ -337,6 +353,11 @@ static void uart_receive(void *opaque, const uint8_t
> *buf, int size)
> CadenceUARTState *s = opaque;
> uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
>
> +/* ignore characters when unclocked */
> +if (!clock_is_enabled(s->refclk)) {
> +return;
> +}
> +
> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
> uart_write_rx_fifo(opaque, buf, size);
> }
> @@ -350,6 +371,11 @@ static void uart_event(void *opaque, int event)
> CadenceUARTState *s = opaque;
> uint8_t buf = '\0';
>
> +/* ignore events when unclocked */
> +if (!clock_is_enabled(s->refclk)) {
> +return;
> +}
> +
> if (event == CHR_EVENT_BREAK) {
> uart_write_rx_fifo(opaque, , 1);
> }
> @@ -382,6 +408,14 @@ static void uart_write(void *opaque, hwaddr offset,
> {
> CadenceUARTState *s = opaque;
>
> +/* ignore accesses when bus or ref clock is disabled */
> +if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> +"cadence_uart: Trying to write register 0x%x"
> +" while clock is disabled\n", (unsigned) offset);
> +return;
> +}
> +
> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
> offset >>= 2;
> if (offset >= CADENCE_UART_R_MAX) {
> @@ -440,6 +474,14 @@ static uint64_t uart_read(void *opaque, hwaddr offset,
> CadenceUARTState *s = opaque;
> uint32_t c = 0;
>
> +/* ignore accesses when bus
[Qemu-devel] [PATCH v4 3/6] block/dirty-bitmaps: allow clear on disabled bitmaps
Similarly to merge, it's OK to allow clear operations on disabled
bitmaps, as this condition only means that they are not recording
new writes. We are free to clear it if the user requests it.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 1 -
blockdev.c | 8
2 files changed, 9 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8a6e07930f..035f97e5c2 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -625,7 +625,6 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
{
-assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
bdrv_dirty_bitmap_lock(bitmap);
if (!out) {
diff --git a/blockdev.c b/blockdev.c
index d775f228fe..cccd36b5e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2012,9 +2012,6 @@ static void
block_dirty_bitmap_clear_prepare(BlkActionState *common,
if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
error_setg(errp, "Cannot modify a bitmap in use by another operation");
return;
-} else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
-error_setg(errp, "Cannot clear a disabled bitmap");
-return;
} else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
error_setg(errp, "Cannot clear a readonly bitmap");
return;
@@ -2917,11 +2914,6 @@ void qmp_block_dirty_bitmap_clear(const char *node,
const char *name,
"Bitmap '%s' is currently in use by another operation"
" and cannot be cleared", name);
return;
-} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
-error_setg(errp,
- "Bitmap '%s' is currently disabled and cannot be cleared",
- name);
-return;
} else if (bdrv_dirty_bitmap_readonly(bitmap)) {
error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared",
name);
return;
--
2.14.4
[Qemu-devel] [PATCH v4 1/6] block/dirty-bitmaps: add user_locked status checker
Instead of both frozen and qmp_locked checks, wrap it into one check.
frozen implies the bitmap is split in two (for backup), and shouldn't
be modified. qmp_locked implies it's being used by another operation,
like being exported over NBD. In both cases it means we shouldn't allow
the user to modify it in any meaningful way.
Replace any usages where we check both frozen and qmp_locked with the
new check.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 6 ++
blockdev.c | 29 -
include/block/dirty-bitmap.h | 1 +
migration/block-dirty-bitmap.c | 10 ++
4 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8ac933cf1c..85bc668f6a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
return bitmap->successor;
}
+/* Both conditions disallow user-modification via QMP. */
+bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
+return (bdrv_dirty_bitmap_frozen(bitmap) ||
+bdrv_dirty_bitmap_qmp_locked(bitmap));
+}
+
void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
{
qemu_mutex_lock(bitmap->mutex);
diff --git a/blockdev.c b/blockdev.c
index 670ae5bbde..d775f228fe 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2009,11 +2009,8 @@ static void
block_dirty_bitmap_clear_prepare(BlkActionState *common,
return;
}
-if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
-error_setg(errp, "Cannot modify a frozen bitmap");
-return;
-} else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
-error_setg(errp, "Cannot modify a locked bitmap");
+if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+error_setg(errp, "Cannot modify a bitmap in use by another operation");
return;
} else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
error_setg(errp, "Cannot clear a disabled bitmap");
@@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char *node,
const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (bdrv_dirty_bitmap_user_locked(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be removed",
- name);
-return;
-} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be removed",
- name);
+ "Bitmap '%s' is currently in use by another operation and"
+ " cannot be removed", name);
return;
}
@@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const char *node,
const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (bdrv_dirty_bitmap_user_locked(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be modified",
- name);
-return;
-} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be modified",
- name);
+ "Bitmap '%s' is currently in use by another operation"
+ " and cannot be cleared", name);
return;
} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
error_setg(errp,
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 201ff7f20b..14639439a2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -94,6 +94,7 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap);
bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 477826330c..d49c581023 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void)
goto fail;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
-error_report("Can't migrate frozen dirty bitmap: '%s",
- bdrv_dirty_bitmap_name(bitmap));
-goto fail;
-}
-
-if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-error_report("Can't migrate locked dirty bitmap: '%s",
+if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+error_report("Can't
[Qemu-devel] [PATCH v4 4/6] block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
We're not being consistent about this. If it's in use by an operation,
the user should not be able to change the behavior of that bitmap.
Signed-off-by: John Snow
---
blockdev.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index cccd36b5e7..d0febfca79 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2058,6 +2058,13 @@ static void
block_dirty_bitmap_enable_prepare(BlkActionState *common,
return;
}
+if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+error_setg(errp,
+ "Bitmap '%s' is currently in use by another operation"
+ " and cannot be enabled", action->name);
+return;
+}
+
state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
bdrv_enable_dirty_bitmap(state->bitmap);
}
@@ -2092,6 +2099,13 @@ static void
block_dirty_bitmap_disable_prepare(BlkActionState *common,
return;
}
+if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+error_setg(errp,
+ "Bitmap '%s' is currently in use by another operation"
+ " and cannot be disabled", action->name);
+return;
+}
+
state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
bdrv_disable_dirty_bitmap(state->bitmap);
}
@@ -2933,10 +2947,10 @@ void qmp_x_block_dirty_bitmap_enable(const char *node,
const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (bdrv_dirty_bitmap_user_locked(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be enabled",
- name);
+ "Bitmap '%s' is currently in use by another operation"
+ " and cannot be enabled", name);
return;
}
@@ -2954,10 +2968,10 @@ void qmp_x_block_dirty_bitmap_disable(const char *node,
const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (bdrv_dirty_bitmap_user_locked(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be disabled",
- name);
+ "Bitmap '%s' is currently in use by another operation"
+ " and cannot be disabled", name);
return;
}
--
2.14.4
[Qemu-devel] [PATCH v4 2/6] block/dirty-bitmaps: fix merge permissions
In prior commits that made merge transactionable, we removed the
assertion that merge cannot operate on disabled bitmaps. In addition,
we want to make sure that we are prohibiting merges to "locked" bitmaps.
Use the new user_locked function to check.
Reported-by: Eric Blake
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 85bc668f6a..8a6e07930f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const
BdrvDirtyBitmap *src,
qemu_mutex_lock(dest->mutex);
-if (bdrv_dirty_bitmap_frozen(dest)) {
-error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
- dest->name);
+if (bdrv_dirty_bitmap_user_locked(dest)) {
+error_setg(errp, "Bitmap '%s' is currently in use by another"
+" operation and cannot be modified", dest->name);
goto out;
}
--
2.14.4
[Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions
based on: jsnow/bitmaps staging branch This series builds on a previous standalone patch and adjusts the permission for all (or most) of the QMP bitmap commands. V4: - Replace "in-use" with "in use" - Replace "user_modifiable" version with "user_locked" - Remove more usages of frozen-and-or-locked in NBD. John Snow (6): block/dirty-bitmaps: add user_locked status checker block/dirty-bitmaps: fix merge permissions block/dirty-bitmaps: allow clear on disabled bitmaps block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps block/backup: prohibit backup from using in use bitmaps nbd: forbid use of frozen bitmaps block/dirty-bitmap.c | 13 +--- blockdev.c | 75 +++--- include/block/dirty-bitmap.h | 1 + migration/block-dirty-bitmap.c | 10 ++ nbd/server.c | 4 +-- 5 files changed, 48 insertions(+), 55 deletions(-) -- 2.14.4
[Qemu-devel] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps
If the bitmap is frozen, we shouldn't touch it.
Signed-off-by: John Snow
---
blockdev.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index d0febfca79..098d4c337f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup,
JobTxn *txn,
bdrv_unref(target_bs);
goto out;
}
-if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
+if (bdrv_dirty_bitmap_user_locked(bmap)) {
error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be used for
"
- "backup", backup->bitmap);
+ "Bitmap '%s' is currently in use by another operation"
+ " and cannot be used for backup", backup->bitmap);
goto out;
}
}
@@ -3620,10 +3620,10 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup,
JobTxn *txn,
error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
goto out;
}
-if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
+if (bdrv_dirty_bitmap_user_locked(bmap)) {
error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be used for
"
- "backup", backup->bitmap);
+ "Bitmap '%s' is currently in use by another operation"
+ " and cannot be used for backup", backup->bitmap);
goto out;
}
}
--
2.14.4
[Qemu-devel] [PATCH v4 6/6] nbd: forbid use of frozen bitmaps
Whether it's "locked" or "frozen", it's in use and should
not be allowed for the purposes of this operation.
Signed-off-by: John Snow
---
nbd/server.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index c3dd402b45..940b7aa0cd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2478,8 +2478,8 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap,
return;
}
-if (bdrv_dirty_bitmap_qmp_locked(bm)) {
-error_setg(errp, "Bitmap '%s' is locked", bitmap);
+if (bdrv_dirty_bitmap_user_locked(bm)) {
+error_setg(errp, "Bitmap '%s' is in use", bitmap);
return;
}
--
2.14.4
Re: [Qemu-devel] [PATCH v5 7/9] hw/misc/zynq_slcr: add clock generation for uarts
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> Add 2 clock outputs for each uart (uart0 & 1):
> + the reference clock
> + the bus interface clock
>
> The clock frequencies are computed using the internal pll & uart configuration
> registers.
>
> All clocks depend on the main input clock (ps_clk), which is hard-coded
> to 33.33MHz (zcu102 evaluation board frequency and frequency specified
> in Xilinx's device tree file)
What about adding in zynq_init:
ClockOut *ps_clk = qdev_init_clock_out(DEVICE(machine), "ps_clk");
clock_set_frequency(ps_clk, PS_REF_CLK_FREQUENCY);
and later:
qdev_connect_clock(slcr, "ps_clk",
DEVICE(machine), "ps_clk_in", _abort);
That would be a more complete example of clock tree.
>
> Signed-off-by: Damien Hedde
> ---
> hw/misc/zynq_slcr.c | 125
> 1 file changed, 125 insertions(+)
>
> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> index baa13d1316..0b599ebd97 100644
> --- a/hw/misc/zynq_slcr.c
> +++ b/hw/misc/zynq_slcr.c
> @@ -33,6 +33,8 @@
> } \
> } while (0)
>
> +#define INPUT_PS_REF_CLK_FREQUENCY ()
> +
> #define XILINX_LOCK_KEY 0x767b
> #define XILINX_UNLOCK_KEY 0xdf0d
>
> @@ -44,15 +46,27 @@ REG32(LOCKSTA, 0x00c)
> REG32(ARM_PLL_CTRL, 0x100)
> REG32(DDR_PLL_CTRL, 0x104)
> REG32(IO_PLL_CTRL, 0x108)
> +/* fields for [ARM|DDR|IO_PLL]_CTRL registers */
> +FIELD(xxx_PLL_CTRL, PLL_RESET, 0, 1)
> +FIELD(xxx_PLL_CTRL, PLL_PWRDWN, 1, 1)
> +FIELD(xxx_PLL_CTRL, PLL_BYPASS_QUAL, 3, 1)
> +FIELD(xxx_PLL_CTRL, PLL_BYPASS_FORCE, 4, 1)
> +FIELD(xxx_PLL_CTRL, PLL_FPDIV, 12, 7)
> REG32(PLL_STATUS, 0x10c)
> REG32(ARM_PLL_CFG, 0x110)
> REG32(DDR_PLL_CFG, 0x114)
> REG32(IO_PLL_CFG, 0x118)
>
> REG32(ARM_CLK_CTRL, 0x120)
> +FIELD(ARM_CLK_CTRL, SRCSEL, 4, 2)
> +FIELD(ARM_CLK_CTRL, DIVISOR, 8, 6)
> +FIELD(ARM_CLK_CTRL, CPU_1XCLKACT, 27, 1)
> +FIELD(ARM_CLK_CTRL, CPU_PERI_CLKACT, 28, 1)
> REG32(DDR_CLK_CTRL, 0x124)
> REG32(DCI_CLK_CTRL, 0x128)
> REG32(APER_CLK_CTRL, 0x12c)
> +FIELD(APER_CLK_CTRL, UART0_CPU1XCLKACT, 20, 1)
> +FIELD(APER_CLK_CTRL, UART1_CPU1XCLKACT, 21, 1)
> REG32(USB0_CLK_CTRL, 0x130)
> REG32(USB1_CLK_CTRL, 0x134)
> REG32(GEM0_RCLK_CTRL, 0x138)
> @@ -63,12 +77,19 @@ REG32(SMC_CLK_CTRL, 0x148)
> REG32(LQSPI_CLK_CTRL, 0x14c)
> REG32(SDIO_CLK_CTRL, 0x150)
> REG32(UART_CLK_CTRL, 0x154)
> +FIELD(UART_CLK_CTRL, CLKACT0, 0, 1)
> +FIELD(UART_CLK_CTRL, CLKACT1, 1, 1)
> +FIELD(UART_CLK_CTRL, SRCSEL, 4, 2)
> +FIELD(UART_CLK_CTRL, DIVISOR, 8, 6)
> REG32(SPI_CLK_CTRL, 0x158)
> REG32(CAN_CLK_CTRL, 0x15c)
> REG32(CAN_MIOCLK_CTRL, 0x160)
> REG32(DBG_CLK_CTRL, 0x164)
> REG32(PCAP_CLK_CTRL, 0x168)
> REG32(TOPSW_CLK_CTRL, 0x16c)
> +/* common fields to lots of *_CLK_CTRL registers */
> +FIELD(xxx_CLK_CTRL, SRCSEL, 4, 2)
> +FIELD(xxx_CLK_CTRL, DIVISOR, 8, 6)
>
> #define FPGA_CTRL_REGS(n, start) \
> REG32(FPGA ## n ## _CLK_CTRL, (start)) \
> @@ -178,8 +199,92 @@ typedef struct ZynqSLCRState {
> MemoryRegion iomem;
>
> uint32_t regs[ZYNQ_SLCR_NUM_REGS];
> +
> +ClockOut *uart0_amba_clk;
> +ClockOut *uart1_amba_clk;
> +ClockOut *uart0_ref_clk;
> +ClockOut *uart1_ref_clk;
> } ZynqSLCRState;
>
> +/*
> + * return the output frequency of ARM/DDR/IO pll
> + * using input frequency and PLL_CTRL register
> + */
> +static uint64_t zynq_slcr_compute_pll(uint64_t input, uint32_t ctrl_reg)
> +{
> +uint32_t mult = ((ctrl_reg & R_xxx_PLL_CTRL_PLL_FPDIV_MASK) >>
> +R_xxx_PLL_CTRL_PLL_FPDIV_SHIFT);
> +
> +/* first, check if pll is bypassed */
> +if (ctrl_reg & R_xxx_PLL_CTRL_PLL_BYPASS_FORCE_MASK) {
> +return input;
> +}
> +
> +/* is pll disabled ? */
> +if (ctrl_reg & (R_xxx_PLL_CTRL_PLL_RESET_MASK |
> +R_xxx_PLL_CTRL_PLL_PWRDWN_MASK)) {
> +return 0;
> +}
> +
> +return input * mult;
> +}
> +
> +/*
> + * return the output frequency of a clock given:
> + * + the pll's frequencies in an array corresponding to mux's indexes
> + * + the register xxx_CLK_CTRL value
> + * + enable bit index in ctrl register
> + *
> + * This function make the assumption that ctrl_reg value is organized as
> follow:
> + * + bits[13:8] clock divisor
> + * + bits[5:4] clock mux selector (index in array)
> + * + bits[index] clock enable
> + */
> +static uint64_t zynq_slcr_compute_clock(const uint64_t plls[],
> +uint32_t ctrl_reg,
> +unsigned index)
> +{
> +uint32_t divisor = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, DIVISOR);
> +uint32_t srcsel = FIELD_EX32(ctrl_reg, xxx_CLK_CTRL, SRCSEL);
> +
> +if ((ctrl_reg & (1u << index)) == 0) {
> +return 0;
> +}
> +
> +return plls[srcsel] / (divisor ? divisor : 1u);
> +}
> +
> +#define ZYNQ_CLOCK(_state, _plls, _reg, _enable_field) \
> +
Re: [Qemu-devel] [PATCH v4 0/6] dirty-bitmaps: fix QMP command permissions
On 10/02/2018 07:02 PM, John Snow wrote: > based on: jsnow/bitmaps staging branch > > This series builds on a previous standalone patch and adjusts > the permission for all (or most) of the QMP bitmap commands. > > V4: > - Replace "in-use" with "in use" > - Replace "user_modifiable" version with "user_locked" > - Remove more usages of frozen-and-or-locked in NBD. > > John Snow (6): > block/dirty-bitmaps: add user_locked status checker > block/dirty-bitmaps: fix merge permissions > block/dirty-bitmaps: allow clear on disabled bitmaps > block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps > block/backup: prohibit backup from using in use bitmaps > nbd: forbid use of frozen bitmaps > > block/dirty-bitmap.c | 13 +--- > blockdev.c | 75 > +++--- > include/block/dirty-bitmap.h | 1 + > migration/block-dirty-bitmap.c | 10 ++ > nbd/server.c | 4 +-- > 5 files changed, 48 insertions(+), 55 deletions(-) > Planning on submitting tests for 3.1, but with the merge queue opened up again I'm pretty keen on getting some stable commit IDs for these so far. --js
Re: [Qemu-devel] [PATCH v5 3/9] qdev-monitor: print the device's clock with info qtree
Hi Damien,
On 10/2/18 4:24 PM, Damien Hedde wrote:
> This prints the clocks attached to a DeviceState when using "info qtree"
> monitor
> command. For every clock, it displays the direction, the name and if the
> clock is forwarded. For input clock, it displays also the frequency.
What would also be really useful (during development mostly)
is a "info clktree" monitor command.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde
> ---
> qdev-monitor.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 61e0300991..8c39a3a65b 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -682,6 +682,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev,
> int indent)
> ObjectClass *class;
> BusState *child;
> NamedGPIOList *ngl;
> +NamedClockList *clk;
>
> qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
> dev->id ? dev->id : "");
> @@ -696,6 +697,17 @@ static void qdev_print(Monitor *mon, DeviceState *dev,
> int indent)
> ngl->num_out);
> }
> }
> +QLIST_FOREACH(clk, >clocks, node) {
> +if (clk->out) {
> +qdev_printf("clock-out%s \"%s\"\n",
> +clk->forward ? " (fw)" : "",
> +clk->name);
> +} else {
> +qdev_printf("clock-in%s \"%s\" freq=%" PRIu64 "Hz\n",
IMHO 'freq_hz=%" PRIu64 "\n"' is easier to read.
However if we plan to add/use a qemu_strtohz() similar to
qemu_strtosz_metric(), that would be fine.
> +clk->forward ? " (fw)" : "",
> +clk->name, clock_get_frequency(clk->in));
> +}
> +}
> class = object_get_class(OBJECT(dev));
> do {
> qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
>
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
[Qemu-devel] [PATCH] hw/pci-host/ppce500: Use DeviceState::realize rather than SysBusDevice::init
Move from the legacy SysBusDevice::init method to using DeviceState::realize.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/pci-host/ppce500.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index eb75e080fc..b8f8c112e6 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -436,8 +436,9 @@ static AddressSpace *e500_pcihost_set_iommu(PCIBus *bus,
void *opaque,
return >bm_as;
}
-static int e500_pcihost_initfn(SysBusDevice *dev)
+static void e500_pcihost_realize(DeviceState *dev, Error **errp)
{
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
PCIHostState *h;
PPCE500PCIState *s;
PCIBus *b;
@@ -447,7 +448,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
s = PPC_E500_PCI_HOST_BRIDGE(dev);
for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
-sysbus_init_irq(dev, >irq[i]);
+sysbus_init_irq(sbd, >irq[i]);
}
for (i = 0; i < PCI_NUM_PINS; i++) {
@@ -460,7 +461,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
/* PIO lives at the bottom of our bus space */
memory_region_add_subregion_overlap(>busmem, 0, >pio, -2);
-b = pci_register_root_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
+b = pci_register_root_bus(dev, NULL, mpc85xx_pci_set_irq,
mpc85xx_pci_map_irq, s, >busmem, >pio,
PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
h->bus = b;
@@ -483,10 +484,8 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
memory_region_add_subregion(>container, PCIE500_CFGADDR, >conf_mem);
memory_region_add_subregion(>container, PCIE500_CFGDATA, >data_mem);
memory_region_add_subregion(>container, PCIE500_REG_BASE, >iomem);
-sysbus_init_mmio(dev, >container);
+sysbus_init_mmio(sbd, >container);
pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
-
-return 0;
}
static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
@@ -526,9 +525,8 @@ static Property pcihost_properties[] = {
static void e500_pcihost_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-k->init = e500_pcihost_initfn;
+dc->realize = e500_pcihost_realize;
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
dc->props = pcihost_properties;
dc->vmsd = _ppce500_pci;
--
2.19.0
Re: [Qemu-devel] [PATCH] softfloat: Fix division
On Tue, Oct 02, 2018 at 14:53:47 -0500, Richard Henderson wrote: > The __udiv_qrnnd primitive that we nicked from gmp requires its > inputs to be normalized. We were not doing that. Because the > inputs are nearly normalized already, finishing that is trivial. > Inline div128to64 into its one user, because it is no longer a > general-purpose routine. > > Fixes: cf07323d494 > Fixes: https://bugs.launchpad.net/qemu/+bug/1793119 > Signed-off-by: Richard Henderson Thanks! Tested-by: Emilio G. Cota E.
[Qemu-devel] [PATCH 2/3] cputlb: serialize tlb updates with env->tlb_lock
Currently we rely on atomic operations for cross-CPU invalidations.
There are two cases that these atomics miss: cross-CPU invalidations
can race with either (1) vCPU threads flushing their TLB, which
happens via memset, or (2) vCPUs calling tlb_reset_dirty on their TLB,
which updates .addr_write with a regular store. This results in
undefined behaviour, since we're mixing regular and atomic ops
on concurrent accesses.
Fix it by using tlb_lock, a per-vCPU lock. All updaters of tlb_table
and the corresponding victim cache now hold the lock.
The readers that do not hold tlb_lock must use atomic reads when
reading .addr_write, since this field can be updated by other threads;
the conversion to atomic reads is done in the next patch.
Note that an alternative fix would be to expand the use of atomic ops.
However, in the case of TLB flushes this would have a huge performance
impact, since (1) TLB flushes can happen very frequently and (2) we
currently use a full memory barrier to flush each TLB entry, and a TLB
has many entries. Instead, acquiring the lock is barely slower than a
full memory barrier since it is uncontended, and with a single lock
acquisition we can flush the entire TLB.
Signed-off-by: Emilio G. Cota
---
include/exec/cpu-defs.h | 2 +
accel/tcg/cputlb.c | 108 ++--
2 files changed, 50 insertions(+), 60 deletions(-)
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index a171ffc1a4..bcc40c8ef5 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -142,6 +142,8 @@ typedef struct CPUIOTLBEntry {
#define CPU_COMMON_TLB \
/* The meaning of the MMU modes is defined in the target code. */ \
+/* tlb_lock serializes updates to tlb_table and tlb_v_table */ \
+QemuMutex tlb_lock; \
CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE]; \
CPUTLBEntry tlb_v_table[NB_MMU_MODES][CPU_VTLB_SIZE]; \
CPUIOTLBEntry iotlb[NB_MMU_MODES][CPU_TLB_SIZE];\
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 502eea2850..01b33f9d28 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -75,6 +75,9 @@ QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16);
void tlb_init(CPUState *cpu)
{
+CPUArchState *env = cpu->env_ptr;
+
+qemu_mutex_init(>tlb_lock);
}
/* flush_all_helper: run fn across all cpus
@@ -129,8 +132,11 @@ static void tlb_flush_nocheck(CPUState *cpu)
atomic_set(>tlb_flush_count, env->tlb_flush_count + 1);
tlb_debug("(count: %zu)\n", tlb_flush_count());
+qemu_mutex_lock(>tlb_lock);
memset(env->tlb_table, -1, sizeof(env->tlb_table));
memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table));
+qemu_mutex_unlock(>tlb_lock);
+
cpu_tb_jmp_cache_clear(cpu);
env->vtlb_index = 0;
@@ -454,72 +460,48 @@ void tlb_unprotect_code(ram_addr_t ram_addr)
* most usual is detecting writes to code regions which may invalidate
* generated code.
*
- * Because we want other vCPUs to respond to changes straight away we
- * update the te->addr_write field atomically. If the TLB entry has
- * been changed by the vCPU in the mean time we skip the update.
+ * Other vCPUs might be reading their TLBs during guest execution, so we update
+ * te->addr_write with atomic_set. We don't need to worry about this for
+ * oversized guests as MTTCG is disabled for them.
*
- * As this function uses atomic accesses we also need to ensure
- * updates to tlb_entries follow the same access rules. We don't need
- * to worry about this for oversized guests as MTTCG is disabled for
- * them.
+ * Called with tlb_lock held.
*/
-
-static void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, uintptr_t start,
- uintptr_t length)
+static void tlb_reset_dirty_range_locked(CPUTLBEntry *tlb_entry,
+ uintptr_t start, uintptr_t length)
{
-#if TCG_OVERSIZED_GUEST
uintptr_t addr = tlb_entry->addr_write;
if ((addr & (TLB_INVALID_MASK | TLB_MMIO | TLB_NOTDIRTY)) == 0) {
addr &= TARGET_PAGE_MASK;
addr += tlb_entry->addend;
if ((addr - start) < length) {
+#if TCG_OVERSIZED_GUEST
tlb_entry->addr_write |= TLB_NOTDIRTY;
-}
-}
#else
-/* paired with atomic_mb_set in tlb_set_page_with_attrs */
-uintptr_t orig_addr = atomic_mb_read(_entry->addr_write);
-uintptr_t addr = orig_addr;
-
-if ((addr & (TLB_INVALID_MASK | TLB_MMIO | TLB_NOTDIRTY)) == 0) {
-addr &= TARGET_PAGE_MASK;
-addr += atomic_read(_entry->addend);
-if ((addr - start) < length) {
-uintptr_t notdirty_addr = orig_addr | TLB_NOTDIRTY;
-atomic_cmpxchg(_entry->addr_write, orig_addr, notdirty_addr);
+atomic_set(_entry->addr_write,
+ tlb_entry->addr_write | TLB_NOTDIRTY);
+#endif
}
}
-#endif
}
-/* For atomic correctness
[Qemu-devel] [PATCH 0/3] per-TLB lock
This series introduces a per-TLB lock. This removes existing UB (e.g. memset racing with cmpxchg on another thread while flushing), and in my opinion makes the TLB code simpler to understand. I had a bit of trouble finding the best place to initialize the mutex, since it has to be called before tlb_flush, and tlb_flush is called quite early during cpu initialization. I settled on cpu_exec_realizefn, since then cpu->env_ptr has been set but tlb_flush hasn't yet been called. Perf-wise this change does have a small impact (~2% slowdown for the aarch64 bootup+shutdown test; 1.2% comes from using atomic_read consistently), but I think this is a fair price for avoiding UB. Numbers below. Initially I tried using atomics instead of memset for flushing (i.e. no mutex), and the slowdown is close to 2X due to the repeated (full) memory barriers. That's when I turned to using a lock. Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz - Before this series: Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs): 7464.797838 task-clock (msec) #0.998 CPUs utilized ( +- 0.14% ) 31,473,652,436 cycles#4.216 GHz ( +- 0.14% ) 57,032,288,549 instructions #1.81 insns per cycle ( +- 0.08% ) 10,239,975,873 branches # 1371.769 M/sec ( +- 0.07% ) 172,150,358 branch-misses #1.68% of all branches ( +- 0.12% ) 7.482009203 seconds time elapsed ( +- 0.18% ) - After: Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs): 7621.625434 task-clock (msec) #0.999 CPUs utilized ( +- 0.10% ) 32,149,898,976 cycles#4.218 GHz ( +- 0.10% ) 58,168,454,452 instructions #1.81 insns per cycle ( +- 0.10% ) 10,486,183,612 branches # 1375.846 M/sec ( +- 0.10% ) 173,900,633 branch-misses #1.66% of all branches ( +- 0.11% ) 7.632067213 seconds time elapsed ( +- 0.10% ) This series is checkpatch-clean. You can fetch the code from: https://github.com/cota/qemu/tree/tlb-lock Thanks, Emilio
[Qemu-devel] [PATCH 3/3] cputlb: read CPUTLBEntry.addr_write atomically
Updates can come from other threads, so readers that do not
take tlb_lock must use atomic_read to avoid undefined behaviour.
Signed-off-by: Emilio G. Cota
---
accel/tcg/softmmu_template.h | 16 ++--
include/exec/cpu_ldst.h | 2 +-
include/exec/cpu_ldst_template.h | 2 +-
accel/tcg/cputlb.c | 15 +--
4 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
index f060a693d4..1e50263871 100644
--- a/accel/tcg/softmmu_template.h
+++ b/accel/tcg/softmmu_template.h
@@ -277,7 +277,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
{
unsigned mmu_idx = get_mmuidx(oi);
int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
+target_ulong tlb_addr =
+atomic_read(>tlb_table[mmu_idx][index].addr_write);
unsigned a_bits = get_alignment_bits(get_memop(oi));
uintptr_t haddr;
@@ -292,7 +293,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE,
mmu_idx, retaddr);
}
-tlb_addr = env->tlb_table[mmu_idx][index].addr_write &
~TLB_INVALID_MASK;
+tlb_addr = atomic_read(>tlb_table[mmu_idx][index].addr_write) &
+~TLB_INVALID_MASK;
}
/* Handle an IO access. */
@@ -321,7 +323,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
cannot evict the first. */
page2 = (addr + DATA_SIZE) & TARGET_PAGE_MASK;
index2 = (page2 >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-tlb_addr2 = env->tlb_table[mmu_idx][index2].addr_write;
+tlb_addr2 = atomic_read(>tlb_table[mmu_idx][index2].addr_write);
if (!tlb_hit_page(tlb_addr2, page2)
&& !VICTIM_TLB_HIT(addr_write, page2)) {
tlb_fill(ENV_GET_CPU(env), page2, DATA_SIZE, MMU_DATA_STORE,
@@ -354,7 +356,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
{
unsigned mmu_idx = get_mmuidx(oi);
int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
+target_ulong tlb_addr =
+atomic_read(>tlb_table[mmu_idx][index].addr_write);
unsigned a_bits = get_alignment_bits(get_memop(oi));
uintptr_t haddr;
@@ -369,7 +372,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE,
mmu_idx, retaddr);
}
-tlb_addr = env->tlb_table[mmu_idx][index].addr_write &
~TLB_INVALID_MASK;
+tlb_addr = atomic_read(>tlb_table[mmu_idx][index].addr_write) &
+~TLB_INVALID_MASK;
}
/* Handle an IO access. */
@@ -398,7 +402,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
cannot evict the first. */
page2 = (addr + DATA_SIZE) & TARGET_PAGE_MASK;
index2 = (page2 >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-tlb_addr2 = env->tlb_table[mmu_idx][index2].addr_write;
+tlb_addr2 = atomic_read(>tlb_table[mmu_idx][index2].addr_write);
if (!tlb_hit_page(tlb_addr2, page2)
&& !VICTIM_TLB_HIT(addr_write, page2)) {
tlb_fill(ENV_GET_CPU(env), page2, DATA_SIZE, MMU_DATA_STORE,
diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 41ed0526e2..9581587ce1 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -426,7 +426,7 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env,
abi_ptr addr,
tlb_addr = tlbentry->addr_read;
break;
case 1:
-tlb_addr = tlbentry->addr_write;
+tlb_addr = atomic_read(>addr_write);
break;
case 2:
tlb_addr = tlbentry->addr_code;
diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 4db2302962..ba7a11123c 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -176,7 +176,7 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX),
_ra)(CPUArchState *env,
addr = ptr;
page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
mmu_idx = CPU_MMU_INDEX;
-if (unlikely(env->tlb_table[mmu_idx][page_index].addr_write !=
+if (unlikely(atomic_read(>tlb_table[mmu_idx][page_index].addr_write)
!=
(addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1) {
oi = make_memop_idx(SHIFT, mmu_idx);
glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX)(env, addr, v, oi,
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 01b33f9d28..77091474ad 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -249,7 +249,7 @@ static inline bool tlb_hit_page_anyprot(CPUTLBEntry
*tlb_entry,
[Qemu-devel] [PATCH v2 12/12] hw/mips/malta: Remove unuseful code
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
hw/mips/mips_malta.c | 13 -
1 file changed, 13 deletions(-)
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 40041d5ec0..89ca6db94b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1422,23 +1422,10 @@ void mips_malta_init(MachineState *machine)
pci_vga_init(pci_bus);
}
-static int mips_malta_sysbus_device_init(SysBusDevice *sysbusdev)
-{
-return 0;
-}
-
-static void mips_malta_class_init(ObjectClass *klass, void *data)
-{
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-
-k->init = mips_malta_sysbus_device_init;
-}
-
static const TypeInfo mips_malta_device = {
.name = TYPE_MIPS_MALTA,
.parent= TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(MaltaState),
-.class_init= mips_malta_class_init,
};
static void mips_malta_machine_init(MachineClass *mc)
--
2.19.0
Re: [Qemu-devel] [PATCH 11/15] hw/sparc/sun4m: Replace 'empty_slot' by 'unimplemented_device'
Hi Artyom,
On 10/2/18 10:14 AM, Artyom Tarasenko wrote:
> Hi Philippe,
>
> On Tue, Oct 2, 2018 at 12:10 AM Philippe Mathieu-Daudé
> wrote:
>>
>> The TYPE_EMPTY_SLOT and TYPE_UNIMPLEMENTED_DEVICE are identical devices,
>> however the later use more recent APIs and is more widely used.
>>
>> Replace 'empty_slot' by 'unimplemented_device' to simplify devices code
>> maintenance.
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> default-configs/sparc-softmmu.mak | 1 -
>> hw/sparc/sun4m.c | 24 ++--
>> 2 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/default-configs/sparc-softmmu.mak
>> b/default-configs/sparc-softmmu.mak
>> index 12f97eeb20..7369b54467 100644
>> --- a/default-configs/sparc-softmmu.mak
>> +++ b/default-configs/sparc-softmmu.mak
>> @@ -8,7 +8,6 @@ CONFIG_ESCC=y
>> CONFIG_M48T59=y
>> CONFIG_PTIMER=y
>> CONFIG_FDC=y
>> -CONFIG_EMPTY_SLOT=y
>> CONFIG_PCNET_COMMON=y
>> CONFIG_LANCE=y
>> CONFIG_TCX=y
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index 3c29b68e67..ca37acf7af 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -42,7 +42,7 @@
>> #include "hw/nvram/chrp_nvram.h"
>> #include "hw/nvram/fw_cfg.h"
>> #include "hw/char/escc.h"
>> -#include "hw/empty_slot.h"
>> +#include "hw/misc/unimp.h"
>> #include "hw/loader.h"
>> #include "elf.h"
>> #include "trace.h"
>> @@ -863,7 +863,8 @@ static void sun4m_hw_init(const struct sun4m_hwdef
>> *hwdef,
>> ram_init(0, machine->ram_size, hwdef->max_mem);
>> /* models without ECC don't trap when missing ram is accessed */
>> if (!hwdef->ecc_base) {
>> -empty_slot_init(machine->ram_size, hwdef->max_mem -
>> machine->ram_size);
>> +create_unimplemented_device("ecc", machine->ram_size,
>> +hwdef->max_mem - machine->ram_size);
>
> In this particular case of find the name UNIMPLEMENTED_DEVICE a bit
> misleading: there is nothing to implement for non-installed memory
> SIMMs. It's rather an unconnected device. And the name "ecc" is
> misleading too. These machines are not supposed to have any ECC.
> Maybe "unconnected-ram", "empty-ram-slot" or "empty-slot"?
OK. I'll rework on this patch later and resend in another series.
Thanks for your review!
Phil.
> Regards,
> Artyom
>
>> }
>>
>> prom_init(hwdef->slavio_base, bios_name);
>> @@ -892,9 +893,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef
>> *hwdef,
>> if (hwdef->iommu_pad_base) {
>> /* On the real hardware (SS-5, LX) the MMU is not padded, but
>> aliased.
>> Software shouldn't use aliased addresses, neither should it crash
>> - when does. Using empty_slot instead of aliasing can help with
>> - debugging such accesses */
>> -empty_slot_init(hwdef->iommu_pad_base,hwdef->iommu_pad_len);
>> + when does. Using the 'unimplemented device' instead of aliasing
>> can
>> + help with debugging such accesses */
>> +create_unimplemented_device("iommu.alias", hwdef->iommu_pad_base,
>> +hwdef->iommu_pad_len);
>> }
>>
>> sparc32_dma_init(hwdef->dma_base,
>> @@ -944,12 +946,13 @@ static void sun4m_hw_init(const struct sun4m_hwdef
>> *hwdef,
>> for (i = num_vsimms; i < MAX_VSIMMS; i++) {
>> /* vsimm registers probed by OBP */
>> if (hwdef->vsimm[i].reg_base) {
>> -empty_slot_init(hwdef->vsimm[i].reg_base, 0x2000);
>> +create_unimplemented_device("vsimm", hwdef->vsimm[i].reg_base,
>> +0x2000);
>> }
>> }
>>
>> if (hwdef->sx_base) {
>> -empty_slot_init(hwdef->sx_base, 0x2000);
>> +create_unimplemented_device("sx", hwdef->sx_base, 0x2000);
>> }
>>
>> nvram = m48t59_init(slavio_irq[0], hwdef->nvram_base, 0, 0x2000, 1968,
>> 8);
>> @@ -1012,14 +1015,15 @@ static void sun4m_hw_init(const struct sun4m_hwdef
>> *hwdef,
>> if (hwdef->dbri_base) {
>> /* ISDN chip with attached CS4215 audio codec */
>> /* prom space */
>> -empty_slot_init(hwdef->dbri_base+0x1000, 0x30);
>> +create_unimplemented_device("dbri.prom", hwdef->dbri_base + 0x1000,
>> +0x30);
>> /* reg space */
>> -empty_slot_init(hwdef->dbri_base+0x1, 0x100);
>> +create_unimplemented_device("dbri", hwdef->dbri_base + 0x1,
>> 0x100);
>> }
>>
>> if (hwdef->bpp_base) {
>> /* parallel port */
>> -empty_slot_init(hwdef->bpp_base, 0x20);
>> +create_unimplemented_device("bpp", hwdef->bpp_base, 0x20);
>> }
>>
>> kernel_size = sun4m_load_kernel(machine->kernel_filename,
>> --
>> 2.19.0
>>
>
>
[Qemu-devel] [PATCH 1/3] exec: introduce tlb_init
Paves the way for the addition of a per-TLB lock.
Signed-off-by: Emilio G. Cota
---
include/exec/exec-all.h | 8
accel/tcg/cputlb.c | 4
exec.c | 1 +
3 files changed, 13 insertions(+)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 5f78125582..815e5b1e83 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -99,6 +99,11 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
/* cputlb.c */
+/**
+ * tlb_init - initialize a CPU's TLB
+ * @cpu: CPU whose TLB should be initialized
+ */
+void tlb_init(CPUState *cpu);
/**
* tlb_flush_page:
* @cpu: CPU whose TLB should be flushed
@@ -258,6 +263,9 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
uintptr_t retaddr);
#else
+static inline void tlb_init(CPUState *cpu)
+{
+}
static inline void tlb_flush_page(CPUState *cpu, target_ulong addr)
{
}
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f4702ce91f..502eea2850 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -73,6 +73,10 @@ QEMU_BUILD_BUG_ON(sizeof(target_ulong) >
sizeof(run_on_cpu_data));
QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16);
#define ALL_MMUIDX_BITS ((1 << NB_MMU_MODES) - 1)
+void tlb_init(CPUState *cpu)
+{
+}
+
/* flush_all_helper: run fn across all cpus
*
* If the wait flag is set then the src cpu's helper will be queued as
diff --git a/exec.c b/exec.c
index 6826c8337d..595deae346 100644
--- a/exec.c
+++ b/exec.c
@@ -965,6 +965,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
tcg_target_initialized = true;
cc->tcg_initialize();
}
+tlb_init(cpu);
#ifndef CONFIG_USER_ONLY
if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
--
2.17.1
[Qemu-devel] [PATCH v2 09/12] hw/sparc64/niagara: Model the I/O Bridge with the 'unimplemented_device'
Since the I/O Bridge device is not implemented, Use the
TYPE_UNIMPLEMENTED_DEVICE which suits better: if the user
asks for 'unimp' warnings via the -d option then all accesses
will generate logging.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Artyom Tarasenko
---
default-configs/sparc64-softmmu.mak | 1 -
hw/sparc64/niagara.c| 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/default-configs/sparc64-softmmu.mak
b/default-configs/sparc64-softmmu.mak
index 52edafe547..ce63d47046 100644
--- a/default-configs/sparc64-softmmu.mak
+++ b/default-configs/sparc64-softmmu.mak
@@ -16,5 +16,4 @@ CONFIG_SIMBA=y
CONFIG_SUNHME=y
CONFIG_MC146818RTC=y
CONFIG_ISA_TESTDEV=y
-CONFIG_EMPTY_SLOT=y
CONFIG_SUN4V_RTC=y
diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
index 4fa8cb2904..f8a856f611 100644
--- a/hw/sparc64/niagara.c
+++ b/hw/sparc64/niagara.c
@@ -29,7 +29,7 @@
#include "hw/hw.h"
#include "hw/boards.h"
#include "hw/char/serial.h"
-#include "hw/empty_slot.h"
+#include "hw/misc/unimp.h"
#include "hw/loader.h"
#include "hw/sparc/sparc64.h"
#include "hw/timer/sun4v-rtc.h"
@@ -161,7 +161,7 @@ static void niagara_init(MachineState *machine)
serial_mm_init(sysmem, NIAGARA_UART_BASE, 0, NULL, 115200,
serial_hd(0), DEVICE_BIG_ENDIAN);
}
-empty_slot_init(NIAGARA_IOBBASE, NIAGARA_IOBSIZE);
+create_unimplemented_device("sun4v-iob", NIAGARA_IOBBASE, NIAGARA_IOBSIZE);
sun4v_rtc_init(NIAGARA_RTC_BASE);
}
--
2.19.0
[Qemu-devel] [PATCH v2 10/12] hw/alpha/typhoon: Remove unuseful code
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
hw/alpha/typhoon.c | 13 -
1 file changed, 13 deletions(-)
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index d74b5b55e1..8004afe45b 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -932,23 +932,10 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus
**isa_bus,
return b;
}
-static int typhoon_pcihost_init(SysBusDevice *dev)
-{
-return 0;
-}
-
-static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
-{
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-
-k->init = typhoon_pcihost_init;
-}
-
static const TypeInfo typhoon_pcihost_info = {
.name = TYPE_TYPHOON_PCI_HOST_BRIDGE,
.parent= TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(TyphoonState),
-.class_init= typhoon_pcihost_class_init,
};
static void typhoon_iommu_memory_region_class_init(ObjectClass *klass,
--
2.19.0
[Qemu-devel] [PATCH v2 11/12] hw/hppa/dino: Remove unuseful code
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
hw/hppa/dino.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
index 564b938e3a..31e09942b5 100644
--- a/hw/hppa/dino.c
+++ b/hw/hppa/dino.c
@@ -488,17 +488,10 @@ PCIBus *dino_init(MemoryRegion *addr_space,
return b;
}
-static int dino_pcihost_init(SysBusDevice *dev)
-{
-return 0;
-}
-
static void dino_pcihost_class_init(ObjectClass *klass, void *data)
{
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
DeviceClass *dc = DEVICE_CLASS(klass);
-k->init = dino_pcihost_init;
dc->vmsd = _dino;
}
--
2.19.0
[Qemu-devel] [PATCH v2 07/12] hw/mips/gt64xxx_pci: Convert gt64120_reset() function into Device reset method
Convert the gt64120_reset() function into a proper Device reset method.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Maydell
Reviewed-by: Cédric Le Goater
---
hw/mips/gt64xxx_pci.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 24ad0ad024..dcd1a66329 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -992,9 +992,9 @@ static void gt64120_pci_set_irq(void *opaque, int irq_num,
int level)
}
-static void gt64120_reset(void *opaque)
+static void gt64120_reset(DeviceState *dev)
{
-GT64120State *s = opaque;
+GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
/* FIXME: Malta specific hw assumptions ahead */
@@ -1184,16 +1184,6 @@ PCIBus *gt64120_register(qemu_irq *pic)
return phb->bus;
}
-static int gt64120_init(SysBusDevice *dev)
-{
-GT64120State *s;
-
-s = GT64120_PCI_HOST_BRIDGE(dev);
-
-qemu_register_reset(gt64120_reset, s);
-return 0;
-}
-
static void gt64120_pci_realize(PCIDevice *d, Error **errp)
{
/* FIXME: Malta specific hw assumptions ahead */
@@ -1241,9 +1231,8 @@ static const TypeInfo gt64120_pci_info = {
static void gt64120_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
-sdc->init = gt64120_init;
+dc->reset = gt64120_reset;
dc->vmsd = _gt64120;
}
--
2.19.0
[Qemu-devel] [PATCH v2 08/12] hw/mips/gt64xxx_pci: Mark as bridge device
The gt64120 is currently listed as uncategorized device.
Mark it as bridge device.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Maydell
Reviewed-by: Cédric Le Goater
---
hw/mips/gt64xxx_pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index dcd1a66329..1cd8aac658 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1232,6 +1232,7 @@ static void gt64120_class_init(ObjectClass *klass, void
*data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
dc->reset = gt64120_reset;
dc->vmsd = _gt64120;
}
--
2.19.0
[Qemu-devel] [PATCH v2 02/12] hw/timer/sun4v-rtc: Convert from DPRINTF() macro to trace events
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Artyom Tarasenko
Reviewed-by: Cédric Le Goater
---
hw/timer/sun4v-rtc.c | 13 +++--
hw/timer/trace-events | 4
2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/hw/timer/sun4v-rtc.c b/hw/timer/sun4v-rtc.c
index 310523225f..13be94f8da 100644
--- a/hw/timer/sun4v-rtc.c
+++ b/hw/timer/sun4v-rtc.c
@@ -14,15 +14,8 @@
#include "hw/sysbus.h"
#include "qemu/timer.h"
#include "hw/timer/sun4v-rtc.h"
+#include "trace.h"
-//#define DEBUG_SUN4V_RTC
-
-#ifdef DEBUG_SUN4V_RTC
-#define DPRINTF(fmt, ...) \
-do { printf("sun4v_rtc: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) do {} while (0)
-#endif
#define TYPE_SUN4V_RTC "sun4v_rtc"
#define SUN4V_RTC(obj) OBJECT_CHECK(Sun4vRtc, (obj), TYPE_SUN4V_RTC)
@@ -41,14 +34,14 @@ static uint64_t sun4v_rtc_read(void *opaque, hwaddr addr,
/* accessing the high 32 bits */
val >>= 32;
}
-DPRINTF("read from " TARGET_FMT_plx " val %lx\n", addr, val);
+trace_sun4v_rtc_read(addr, val);
return val;
}
static void sun4v_rtc_write(void *opaque, hwaddr addr,
uint64_t val, unsigned size)
{
-DPRINTF("write 0x%x to " TARGET_FMT_plx "\n", (unsigned)val, addr);
+trace_sun4v_rtc_read(addr, val);
}
static const MemoryRegionOps sun4v_rtc_ops = {
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index ca9ad6321a..75bd3b1042 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -66,5 +66,9 @@ cmsdk_apb_dualtimer_read(uint64_t offset, uint64_t data,
unsigned size) "CMSDK A
cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size)
"CMSDK APB dualtimer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
+# hw/timer/sun4v-rtc.c
+sun4v_rtc_read(uint64_t addr, uint64_t value) "read: addr 0x%" PRIx64 " value
0x%" PRIx64
+sun4v_rtc_write(uint64_t addr, uint64_t value) "write: addr 0x%" PRIx64 "
value 0x%" PRIx64
+
# hw/timer/xlnx-zynqmp-rtc.c
xlnx_zynqmp_rtc_gettime(int year, int month, int day, int hour, int min, int
sec) "Get time from host: %d-%d-%d %2d:%02d:%02d"
--
2.19.0
[Qemu-devel] [PATCH v2 06/12] hw/pci-host/bonito: Use DeviceState::realize rather than SysBusDevice::init
Move from the legacy SysBusDevice::init method to using DeviceState::realize.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/pci-host/bonito.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 9868e2eccc..9f33582706 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -595,7 +595,7 @@ static const VMStateDescription vmstate_bonito = {
}
};
-static int bonito_pcihost_initfn(SysBusDevice *dev)
+static void bonito_pcihost_realize(DeviceState *dev, Error **errp)
{
PCIHostState *phb = PCI_HOST_BRIDGE(dev);
@@ -603,8 +603,6 @@ static int bonito_pcihost_initfn(SysBusDevice *dev)
pci_bonito_set_irq, pci_bonito_map_irq,
dev, get_system_memory(), get_system_io(),
0x28, 32, TYPE_PCI_BUS);
-
-return 0;
}
static void bonito_realize(PCIDevice *dev, Error **errp)
@@ -684,7 +682,6 @@ PCIBus *bonito_init(qemu_irq *pic)
pcihost->pic = pic;
qdev_init_nofail(dev);
-/* set the pcihost pointer before bonito_initfn is called */
d = pci_create(phb->bus, PCI_DEVFN(0, 0), TYPE_PCI_BONITO);
s = PCI_BONITO(d);
s->pcihost = pcihost;
@@ -726,9 +723,9 @@ static const TypeInfo bonito_info = {
static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
{
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
-k->init = bonito_pcihost_initfn;
+dc->realize = bonito_pcihost_realize;
}
static const TypeInfo bonito_pcihost_info = {
--
2.19.0
[Qemu-devel] [PATCH v2 05/12] hw/sh4/sh_pci: Use DeviceState::realize rather than SysBusDevice::init
Move from the legacy SysBusDevice::init method to using DeviceState::realize.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sh4/sh_pci.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index 4ec2e35500..379d0685ed 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -120,16 +120,15 @@ static void sh_pci_set_irq(void *opaque, int irq_num, int
level)
qemu_set_irq(pic[irq_num], level);
}
-static int sh_pci_device_init(SysBusDevice *dev)
+static void sh_pci_device_realize(DeviceState *dev, Error **errp)
{
-PCIHostState *phb;
-SHPCIState *s;
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+SHPCIState *s = SH_PCI_HOST_BRIDGE(dev);
+PCIHostState *phb = PCI_HOST_BRIDGE(s);
int i;
-s = SH_PCI_HOST_BRIDGE(dev);
-phb = PCI_HOST_BRIDGE(s);
for (i = 0; i < 4; i++) {
-sysbus_init_irq(dev, >irq[i]);
+sysbus_init_irq(sbd, >irq[i]);
}
phb->bus = pci_register_root_bus(DEVICE(dev), "pci",
sh_pci_set_irq, sh_pci_map_irq,
@@ -143,13 +142,12 @@ static int sh_pci_device_init(SysBusDevice *dev)
>memconfig_p4, 0, 0x224);
memory_region_init_alias(>isa, OBJECT(s), "sh_pci.isa",
get_system_io(), 0, 0x4);
-sysbus_init_mmio(dev, >memconfig_p4);
-sysbus_init_mmio(dev, >memconfig_a7);
+sysbus_init_mmio(sbd, >memconfig_p4);
+sysbus_init_mmio(sbd, >memconfig_a7);
s->iobr = 0xfe24;
memory_region_add_subregion(get_system_memory(), s->iobr, >isa);
s->dev = pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "sh_pci_host");
-return 0;
}
static void sh_pci_host_realize(PCIDevice *d, Error **errp)
@@ -187,9 +185,9 @@ static const TypeInfo sh_pci_host_info = {
static void sh_pci_device_class_init(ObjectClass *klass, void *data)
{
-SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
-sdc->init = sh_pci_device_init;
+dc->realize = sh_pci_device_realize;
}
static const TypeInfo sh_pci_device_info = {
--
2.19.0
[Qemu-devel] [PATCH v2 00/12] another SysBusDevice::init to Device::realize cleanup
Peter suggested [1] another crusade for this merge window, then Cédric jumped on his horse [2]. My turn on my dromedary. since v1: - let the empty_slot - sh4_pci and bonito use DeviceState::realize instead of PCIDevice::realize - reword niagara-iob commit message v1: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00220.html - convert few devices to DeviceState::realize, - kill the empty_slot device, - remove unuseful class_init() code [RFC, do we want to keep this?] - few other minor fixes catched while editing Regards, Phil. [1] https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg03605.html [2] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg1.html 001/12:[] [--] 'trace-events: Fix copy/paste typo' 002/12:[] [--] 'hw/timer/sun4v-rtc: Convert from DPRINTF() macro to trace events' 003/12:[] [--] 'hw/timer/sun4v-rtc: Use DeviceState::realize rather than SysBusDevice::init' 004/12:[] [--] 'hw/ssi/xilinx_spi: Use DeviceState::realize rather than SysBusDevice::init' 005/12:[0006] [FC] 'hw/sh4/sh_pci: Use DeviceState::realize rather than SysBusDevice::init' 006/12:[0006] [FC] 'hw/pci-host/bonito: Use DeviceState::realize rather than SysBusDevice::init' 007/12:[] [--] 'hw/mips/gt64xxx_pci: Convert gt64120_reset() function into Device reset method' 008/12:[] [--] 'hw/mips/gt64xxx_pci: Mark as bridge device' 009/12:[] [--] 'hw/sparc64/niagara: Model the I/O Bridge with the 'unimplemented_device'' 010/12:[] [--] 'hw/alpha/typhoon: Remove unuseful code' 011/12:[] [--] 'hw/hppa/dino: Remove unuseful code' 012/12:[] [--] 'hw/mips/malta: Remove unuseful code' Philippe Mathieu-Daudé (12): trace-events: Fix copy/paste typo hw/timer/sun4v-rtc: Convert from DPRINTF() macro to trace events hw/timer/sun4v-rtc: Use DeviceState::realize rather than SysBusDevice::init hw/ssi/xilinx_spi: Use DeviceState::realize rather than SysBusDevice::init hw/sh4/sh_pci: Use DeviceState::realize rather than SysBusDevice::init hw/pci-host/bonito: Use DeviceState::realize rather than SysBusDevice::init hw/mips/gt64xxx_pci: Convert gt64120_reset() function into Device reset method hw/mips/gt64xxx_pci: Mark as bridge device hw/sparc64/niagara: Model the I/O Bridge with the 'unimplemented_device' hw/alpha/typhoon: Remove unuseful code hw/hppa/dino: Remove unuseful code hw/mips/malta: Remove unuseful code default-configs/sparc64-softmmu.mak | 1 - hw/alpha/typhoon.c | 13 - hw/hppa/dino.c | 7 --- hw/mips/gt64xxx_pci.c | 18 -- hw/mips/mips_malta.c| 13 - hw/pci-host/bonito.c| 9 +++-- hw/sh4/sh_pci.c | 20 +--- hw/sparc64/niagara.c| 4 ++-- hw/ssi/xilinx_spi.c | 9 +++-- hw/timer/sun4v-rtc.c| 23 --- hw/timer/trace-events | 6 +- 11 files changed, 34 insertions(+), 89 deletions(-) -- 2.19.0
[Qemu-devel] [PATCH v2 01/12] trace-events: Fix copy/paste typo
Missed while reviewing 5dd85b4b486. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Peter Maydell Reviewed-by: Cédric Le Goater --- hw/timer/trace-events | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/timer/trace-events b/hw/timer/trace-events index fa4213df5b..ca9ad6321a 100644 --- a/hw/timer/trace-events +++ b/hw/timer/trace-events @@ -56,7 +56,7 @@ systick_timer_tick(void) "systick reload" systick_read(uint64_t addr, uint32_t value, unsigned size) "systick read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u" systick_write(uint64_t addr, uint32_t value, unsigned size) "systick write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u" -# hw/char/cmsdk_apb_timer.c +# hw/timer/cmsdk_apb_timer.c cmsdk_apb_timer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB timer read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u" cmsdk_apb_timer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB timer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u" cmsdk_apb_timer_reset(void) "CMSDK APB timer: reset" -- 2.19.0
[Qemu-devel] [PATCH v2 03/12] hw/timer/sun4v-rtc: Use DeviceState::realize rather than SysBusDevice::init
Move from the legacy SysBusDevice::init method to using DeviceState::realize.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Reviewed-by: Cédric Le Goater
---
hw/timer/sun4v-rtc.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/timer/sun4v-rtc.c b/hw/timer/sun4v-rtc.c
index 13be94f8da..4e7f6a1eff 100644
--- a/hw/timer/sun4v-rtc.c
+++ b/hw/timer/sun4v-rtc.c
@@ -63,21 +63,21 @@ void sun4v_rtc_init(hwaddr addr)
sysbus_mmio_map(s, 0, addr);
}
-static int sun4v_rtc_init1(SysBusDevice *dev)
+static void sun4v_rtc_realize(DeviceState *dev, Error **errp)
{
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
Sun4vRtc *s = SUN4V_RTC(dev);
memory_region_init_io(>iomem, OBJECT(s), _rtc_ops, s,
"sun4v-rtc", 0x08ULL);
-sysbus_init_mmio(dev, >iomem);
-return 0;
+sysbus_init_mmio(sbd, >iomem);
}
static void sun4v_rtc_class_init(ObjectClass *klass, void *data)
{
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
-k->init = sun4v_rtc_init1;
+dc->realize = sun4v_rtc_realize;
}
static const TypeInfo sun4v_rtc_info = {
--
2.19.0
[Qemu-devel] [PATCH v2 04/12] hw/ssi/xilinx_spi: Use DeviceState::realize rather than SysBusDevice::init
Move from the legacy SysBusDevice::init method to using DeviceState::realize.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Reviewed-by: Cédric Le Goater
---
hw/ssi/xilinx_spi.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
index 83585bc8b2..3dae303d5b 100644
--- a/hw/ssi/xilinx_spi.c
+++ b/hw/ssi/xilinx_spi.c
@@ -319,9 +319,9 @@ static const MemoryRegionOps spi_ops = {
}
};
-static int xilinx_spi_init(SysBusDevice *sbd)
+static void xilinx_spi_realize(DeviceState *dev, Error **errp)
{
-DeviceState *dev = DEVICE(sbd);
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
XilinxSPI *s = XILINX_SPI(dev);
int i;
@@ -344,8 +344,6 @@ static int xilinx_spi_init(SysBusDevice *sbd)
fifo8_create(>tx_fifo, FIFO_CAPACITY);
fifo8_create(>rx_fifo, FIFO_CAPACITY);
-
-return 0;
}
static const VMStateDescription vmstate_xilinx_spi = {
@@ -368,9 +366,8 @@ static Property xilinx_spi_properties[] = {
static void xilinx_spi_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
-SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-k->init = xilinx_spi_init;
+dc->realize = xilinx_spi_realize;
dc->reset = xlx_spi_reset;
dc->props = xilinx_spi_properties;
dc->vmsd = _xilinx_spi;
--
2.19.0
Re: [Qemu-devel] [PATCH 06/15] hw/pci-host/bonito: Use DeviceState::realize rather than SysBusDevice::init
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote:
> Move from the legacy SysBusDevice::init method to using DeviceState::realize.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/pci-host/bonito.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index 9868e2eccc..03d1ec33e3 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -595,7 +595,7 @@ static const VMStateDescription vmstate_bonito = {
> }
> };
>
> -static int bonito_pcihost_initfn(SysBusDevice *dev)
> +static void bonito_pcihost_realize(PCIDevice *dev, Error **errp)
This patch is incorrect, see:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00378.html
> {
> PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>
> @@ -603,8 +603,6 @@ static int bonito_pcihost_initfn(SysBusDevice *dev)
> pci_bonito_set_irq, pci_bonito_map_irq,
> dev, get_system_memory(),
> get_system_io(),
> 0x28, 32, TYPE_PCI_BUS);
> -
> -return 0;
> }
>
> static void bonito_realize(PCIDevice *dev, Error **errp)
> @@ -684,7 +682,6 @@ PCIBus *bonito_init(qemu_irq *pic)
> pcihost->pic = pic;
> qdev_init_nofail(dev);
>
> -/* set the pcihost pointer before bonito_initfn is called */
> d = pci_create(phb->bus, PCI_DEVFN(0, 0), TYPE_PCI_BONITO);
> s = PCI_BONITO(d);
> s->pcihost = pcihost;
> @@ -726,9 +723,9 @@ static const TypeInfo bonito_info = {
>
> static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
> {
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -k->init = bonito_pcihost_initfn;
> +k->realize = bonito_pcihost_realize;
> }
>
> static const TypeInfo bonito_pcihost_info = {
>
Re: [Qemu-devel] [PATCH 05/15] hw/sh4/sh_pci: Use DeviceState::realize rather than SysBusDevice::init
On 10/2/18 10:37 PM, Marcel Apfelbaum wrote: > Hi Philippe, Peter > > On 10/2/18 10:59 PM, Philippe Mathieu-Daudé wrote: >> On 10/2/18 3:13 PM, Peter Maydell wrote: >>> On 1 October 2018 at 23:09, Philippe Mathieu-Daudé >>> wrote: Move from the legacy SysBusDevice::init method to using DeviceState::realize. >>> Comment says DeviceState::realize but the code is using >>> PCIDevice::realize ? >>> >>> I didn't realize pci devices had their own realize method: >>> what's the difference between it and plain old DeviceState::realize, > > PCIDevice:realize handles PCI specific "realization" tasks: > * setup of the PCI configuration space > * PCI BARs configuration > * MSI initalization > *... > > The PCIDevice's DeviceState::realize function named 'pci_qdev_realize' > calls PCIDevice:realize after it runs some generic initialization code. > > It is possible we have this design so we would be able to move from > "qdev" to QOM. Thanks Marcel, so this patch is incorrect. TYPE_SH_PCI_HOST_BRIDGE inherits TYPE_PCI_HOST_BRIDGE which inherits TYPE_SYS_BUS_DEVICE which inherits TYPE_DEVICE, so can implement DeviceState::realize but not PCIDevice::realize. TYPE_"sh_pci_host" inherits TYPE_PCI_DEVICE, so can implement PCIDevice::realize. I'll respin. > Thanks, > Marcel > >>> which they also have, since PCIDeviceClass inherits from DeviceClass ? >> Cc'ing Michael and Marcel.
Re: [Qemu-devel] [PATCH v3] qemu-img.c: add help for each command
On 9/25/18 10:39 AM, John Arbuckle wrote:
Add the ability for the user to display help for a certain command.
Example: qemu-img create --help
What is printed is all the options available to this command and an example.
Signed-off-by: John Arbuckle
---
v3 changes:
Fixed a bug that caused qemu-img to crash when running a command without
options.
Hmm, I just reviewed v2 without noticing that you had posted v3.
v2 changes:
Removed block of string comparison code for each command.
Added a help_func field to the img_cmd_t struct.
Made strings longer than 80 characters wide.
qemu-img.c | 660 +++--
1 file changed, 559 insertions(+), 101 deletions(-)
My v2 comment about splitting this into more reviewable portions still
applies.
+++ b/qemu-img.c
@@ -52,6 +52,7 @@
typedef struct img_cmd_t {
const char *name;
int (*handler)(int argc, char **argv);
+void (*help_func)(void);
} img_cmd_t;
As does my complaint that you want to track the command synopses here in
this struct for reuse in multiple contexts later, rather than...
+static void help_amend(void)
+{
+const char *msg =
+"\n"
+"usage: qemu-img amend [--object objectdef] [--image-opts] [-p] [-q] [-f
fmt]\n"
+"[-t cache] -o options filename\n"
repeating things that are in qemu-img-cmds.h and risking them getting
out of sync.
-static const img_cmd_t img_cmds[] = {
+static img_cmd_t img_cmds[] = {
#define DEF(option, callback, arg_string)\
{ option, callback },
#include "qemu-img-cmds.h"
@@ -4894,6 +5334,12 @@ static const img_cmd_t img_cmds[] = {
{ NULL, NULL, },
};
+/* These functions will be added to the img_cmds array */
+void (*help_functions[])(void) = {help_amend, help_bench, help_check,\
+help_commit, help_compare, help_convert, help_create, help_dd, help_info,\
+help_map, help_measure, help_snapshot, help_rebase, help_resize};
And my biggest gripe, that this approach to initialization is dead
wrong, is still applicable.
Overall, I like the end result that you are trying to reach. (I'm not
always a fan of having to run 'command --help' followed by 'command
subcommand --help' to learn everything, but it sure beats a wall of
text, and we have at least 'git' and 'cvs' as examples of programs that
have used that approach, so we are not inventing something new). But
you'll need a v4, and preferably broken up into something easier to
review (one patch per command, rather than a wholesale rewrite in go),
if you want to get it in.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2] qemu-img.c: add help for each command
On 9/15/18 11:10 AM, John Arbuckle wrote:
Add the ability for the user to display help for a certain command.
Example: qemu-img create --help
What is printed is all the options available to this command and an example.
Signed-off-by: John Arbuckle
---
v2 changes:
Removed block of string comparison code for each command.
Added a help_func field to the img_cmd_t struct.
Made strings longer than 80 characters wide.
qemu-img.c | 659 +++--
1 file changed, 558 insertions(+), 101 deletions(-)
This is a pretty big patch to review in one sitting. Is it possible for
you to divide it into smaller portions?
diff --git a/qemu-img.c b/qemu-img.c
index 1acddf693c..7b9f6101b3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -52,6 +52,7 @@
typedef struct img_cmd_t {
const char *name;
int (*handler)(int argc, char **argv);
+void (*help_func)(void);
} img_cmd_t;
Perhaps by an initial patch that adds .help_func but permits it be NULL
(falling back to full normal help for commands not yet converted); then
converts one command per patch, and finally requires .help_func to be
non-NULL.
-/* Please keep in synch with qemu-img.texi */
Why did this comment disappear?
-static void QEMU_NORETURN help(void)
+/* Prints an overview of available help options */
+static void help(void)
{
const char *help_msg =
- QEMU_IMG_VERSION
- "usage: qemu-img [standard options] command [command options]\n"
- "QEMU disk image utility\n"
- "\n"
- "'-h', '--help' display this help and exit\n"
- "'-V', '--version'output version information and exit\n"
...
-printf("%s\nSupported formats:", help_msg);
-bdrv_iterate_format(format_print, NULL);
-printf("\n\n" QEMU_HELP_BOTTOM "\n");
-exit(EXIT_SUCCESS);
+QEMU_IMG_VERSION
+"usage: qemu-img [standard options] command [command options]\n"
+"QEMU disk image utility\n"
+"\n"
+"'-h', '--help' display this help and exit\n"
+"'-V', '--version'output version information and exit\n"
+"'-T', '--trace'
[[enable=]][,events=][,file=]\n"
Why the re-indentation? That also makes it slightly harder to review.
+" specify tracing options\n"
+"\n"
+"Command:\n"
+"\tamend Change options of an existing disk image\n"
+"\tbench Run benchmarks on a given disk image\n"
+"\tcheck Check the disk image for consistency or repair it\n"
+"\tcommit Merge the disk image into its backing file\n"
+"\tcompareCheck if two images have the same content\n"
+"\tconvertConvert an image file or snapshot into another
file\n"
+"\tcreate Create a new disk image\n"
+"\tdd Copies and converts an input file into another
file\n"
+"\tinfo Gives information about the specified image file\n"
+"\tmapDump the metadata of image filename\n"
+"\tmeasureCalculate the file size required for a new image\n"
+"\tsnapshot List, apply, create or delete snapshots in a file\n"
+"\trebase Changes the backing file of an image file\n"
+"\tresize Resize an image file\n"
+"\n\nRun 'qemu-img --help' for details.\n"
I'd spell this:
"\n"
"\n"
"Run ...\n"
(that is, exactly one \n per line, always as the last thing in a string,
so that it is easier to see in the source how line-breaks will be added
in the output)
+"See for how to report bugs.\n"
+"More information on the QEMU project at .\n";
+printf("%s\n", help_msg);
+}
+
+static void help_amend(void)
+{
+const char *msg =
+"\n"
+"usage: qemu-img amend [--object objectdef] [--image-opts] [-p] [-q] [-f
fmt]\n"
+"[-t cache] -o options filename\n"
Why no indentation on the continued option line?
At this point, I'm skipping the various individual commands (as I didn't
have time to check each one for accuracy - again, an argument that
splitting into a series that converts one command per patch may be
easier to review than doing wholesale conversion in a single patch), and
moving on to the real thing that stood out to me.
@@ -4886,7 +5326,7 @@ out:
return ret;
}
-static const img_cmd_t img_cmds[] = {
+static img_cmd_t img_cmds[] = {
#define DEF(option, callback, arg_string)\
{ option, callback },
#include "qemu-img-cmds.h"
@@ -4894,6 +5334,12 @@ static const img_cmd_t img_cmds[] = {
{ NULL, NULL, },
};
+/* These functions will be added to the img_cmds array */
+void (*help_functions[])(void) = {help_amend, help_bench, help_check,\
+help_commit, help_compare, help_convert, help_create, help_dd, help_info,\
+help_map, help_measure, help_snapshot,
Re: [Qemu-devel] [PATCH v6 00/27] qapi: add #if pre-processor conditions to generated code (part 2)
Hi
On Fri, Jul 6, 2018 at 2:59 PM Marc-André Lureau
wrote:
>
> Hi,
>
> This is the second part of the "add #if pre-processor conditions to
> generated code" series, adding schema member conditions (roughly
> 16-38/49).
>
> Members can be exploded as dictionnary with 'type'/'if' keys:
>
> { 'struct': 'TestIfStruct', 'data':
> { 'foo': 'int',
> 'bar': { 'type': 'int', 'if': 'defined(TEST_IF_STRUCT_BAR)'} } }
>
> Enum values can be exploded as dictionnary with 'type'/'if' keys:
>
> { 'enum': 'TestIfEnum', 'data':
> [ 'foo',
> { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ] }
>
> v6:
> - subset of v5 series: add schema member conditions
> - split "qapi: factor out check_known_keys()", error report improvements
> - add a patch to "qapi: include osdep.h in type headers" to avoid
> potential build configuration change issues
> - rebased, on top of Markus's qapi-next branch (first 4 patches,
> included for patchew testing)
>
> Marc-André Lureau (26):
> qmp-shell: learn to send commands with quoted arguments
> json: remove useless return value from lexer/parser
> tests: change /0.15/* tests to /qmp/*
> qapi: include osdep.h in type headers
> qapi: do not define enumeration value explicitly
> qapi: rename QAPISchemaEnumType.values to .members
> qapi: change enum visitor and gen_enum* to take QAPISchemaMember
> tests: print enum type members more like object type members
> qapi: factor out checking for keys
> qapi: improve reporting of unknown or missing keys
> qapi: add a dictionnary form with 'name' key for enum members
> qapi: add 'if' to enum members
> qapi-event: add 'if' condition to implicit event enum
> qapi: rename allow_dict to allow_implicit
> qapi: add a dictionary form with 'type' key for members
> qapi: add 'if' to implicit struct members
> qapi: add an error in case a discriminator is conditionnal
> qapi: add 'if' on union members
> qapi: add 'if' to alternate members
> qapi: add #if conditions to generated code members
> docs: document schema configuration
> qapi: add 'If:' condition to enum values documentation
> qapi: add 'If:' condition to struct members documentation
> qapi: add condition to variants documentation
> qapi: add more conditions to SPICE
> qapi: add conditions to REPLICATION type/commands on the schema
>
> Markus Armbruster (1):
> qapi: Fix some pycodestyle-3 complaints
Rebased series updated on github branch:
https://github.com/elmarco/qemu/commits/qapi-if
(mostly unchanged, except patch "add #if conditions to generated code
members" which had to accommodate with Eric's introspection comment
patch)
thanks
> qapi/block-core.json | 13 +-
> qapi/char.json| 16 +-
> qapi/migration.json | 12 +-
> scripts/qapi/common.py| 256 --
> scripts/qapi/doc.py | 16 +-
> scripts/qapi/events.py| 2 +-
> scripts/qapi/introspect.py| 16 +-
> scripts/qapi/types.py | 11 +-
> scripts/qapi/visit.py | 10 +-
> include/qapi/qmp/json-lexer.h | 4 +-
> include/qapi/qmp/json-streamer.h | 4 +-
> migration/colo.c | 16 +-
> monitor.c | 5 -
> qobject/json-lexer.c | 23 +-
> qobject/json-streamer.c | 8 +-
> tests/test-qmp-cmds.c | 10 +-
> docs/devel/qapi-code-gen.txt | 19 ++
> scripts/qmp/qmp-shell | 3 +-
> tests/Makefile.include| 8 +-
> tests/qapi-schema/alternate-base.err | 1 +
> tests/qapi-schema/alternate-invalid-dict.err | 1 +
> ...ember.exit => alternate-invalid-dict.exit} | 0
> tests/qapi-schema/alternate-invalid-dict.json | 4 +
> ...-member.out => alternate-invalid-dict.out} | 0
> tests/qapi-schema/comments.out| 14 +-
> tests/qapi-schema/doc-bad-section.out | 13 +-
> tests/qapi-schema/doc-good.json | 11 +-
> tests/qapi-schema/doc-good.out| 22 +-
> tests/qapi-schema/doc-good.texi | 8 +-
> tests/qapi-schema/double-type.err | 1 +
> tests/qapi-schema/empty.out | 9 +-
> tests/qapi-schema/enum-bad-member.err | 1 +
> tests/qapi-schema/enum-bad-member.exit| 1 +
> tests/qapi-schema/enum-bad-member.json| 2 +
> tests/qapi-schema/enum-bad-member.out | 0
> .../qapi-schema/enum-dict-member-unknown.err | 2 +
> .../qapi-schema/enum-dict-member-unknown.exit | 1 +
> .../qapi-schema/enum-dict-member-unknown.json | 2 +
> .../qapi-schema/enum-dict-member-unknown.out | 0
> tests/qapi-schema/enum-dict-member.err| 1 -
>
Re: [Qemu-devel] [PATCH 05/15] hw/sh4/sh_pci: Use DeviceState::realize rather than SysBusDevice::init
Hi Philippe, Peter On 10/2/18 10:59 PM, Philippe Mathieu-Daudé wrote: On 10/2/18 3:13 PM, Peter Maydell wrote: On 1 October 2018 at 23:09, Philippe Mathieu-Daudé wrote: Move from the legacy SysBusDevice::init method to using DeviceState::realize. Comment says DeviceState::realize but the code is using PCIDevice::realize ? I didn't realize pci devices had their own realize method: what's the difference between it and plain old DeviceState::realize, PCIDevice:realize handles PCI specific "realization" tasks: * setup of the PCI configuration space * PCI BARs configuration * MSI initalization *... The PCIDevice's DeviceState::realize function named 'pci_qdev_realize' calls PCIDevice:realize after it runs some generic initialization code. It is possible we have this design so we would be able to move from "qdev" to QOM. Thanks, Marcel which they also have, since PCIDeviceClass inherits from DeviceClass ? Cc'ing Michael and Marcel.
Re: [Qemu-devel] [PATCH v3 00/18] fleecing-hook driver for backup
On 10/1/18 5:29 AM, Vladimir Sementsov-Ogievskiy wrote: v2 was "[RFC v2] new, node-graph-based fleecing and backup" Hi all! These series introduce fleecing-hook driver. It's a filter-node, which do copy-before-write operation. Mirror uses filter-node for handling guest writes, let's move to filter-node (from write-notifiers) for backup too (patch 18) Proposed filter driver is complete and separate: it can be used standalone, as fleecing provider (instead of backup(sync=none)). (old-style fleecing based on backup(sync=none) is supported too), look at patch 16. I haven't had time to look at this series in any sort of depth yet, but it reminds me of a question I just ran into with my libvirt code: What happens if we want to have two parallel clients both reading off different backup/fleece nodes at once? Right now, 'nbd-server-start' is hard-coded to at most one NBD server, and 'nbd-server-add' is hardcoded to adding an export to the one-and-only NBD server. But it would be a lot nicer if you could pick different ports for different clients (or even mix TCP and Unix sockets), so that independent backup jobs can both operate in parallel via different NBD servers both under control of the same qemu process, instead of the second client having to wait for the first client to disconnect so that the first NBD server can stop. In the meantime, you can be somewhat careful by controlling which export names are exposed over NBD, but even with nbd-server-start using "tls-creds", all clients can see one another's exports via NBD_OPT_LIST, and you are relying on the clients being well-behaved, vs. the nicer ability to spawn multiple NBD servers, then control which exports are exposed over which servers, and where distinct servers could even have different tls-creds. To get to that point, we'd need to enhance nbd-server-start to return a server id, and allow nbd-server-add and friends to take an optional parameter of a server id (for back-compat, if the server id is not provided, it operates on the first one). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH] tests: Disable test-bdrv-drain and test-replication
On 10/01/2018 09:20 AM, Peter Maydell wrote: > The test-bdrv-drain and test-replication tests have > intermittent errors which make my build testing process > fail way too often. Disable them both for the moment. > > Signed-off-by: Peter Maydell > --- > I'm having trouble making forward progress with applying > merges because of these flaky tests :-( > > tests/Makefile.include | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index d0c0a92e67d..1cb1e1a1da7 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -91,7 +91,7 @@ gcov-files-test-thread-pool-y = thread-pool.c > gcov-files-test-hbitmap-y = util/hbitmap.c > check-unit-y += tests/test-hbitmap$(EXESUF) > gcov-files-test-hbitmap-y = blockjob.c > -check-unit-y += tests/test-bdrv-drain$(EXESUF) > +#check-unit-y += tests/test-bdrv-drain$(EXESUF) > check-unit-y += tests/test-blockjob$(EXESUF) > check-unit-y += tests/test-blockjob-txn$(EXESUF) > check-unit-y += tests/test-block-backend$(EXESUF) > @@ -167,7 +167,7 @@ check-unit-y += tests/test-crypto-xts$(EXESUF) > check-unit-y += tests/test-crypto-block$(EXESUF) > check-unit-y += tests/test-logging$(EXESUF) > gcov-files-test-logging-y = util/log.c > -check-unit-$(CONFIG_REPLICATION) += tests/test-replication$(EXESUF) > +#check-unit-$(CONFIG_REPLICATION) += tests/test-replication$(EXESUF) > check-unit-y += tests/test-bufferiszero$(EXESUF) > gcov-files-check-bufferiszero-y = util/bufferiszero.c > check-unit-y += tests/test-uuid$(EXESUF) > Should we leave some kind of breadcrumb to remind ourselves to fix or re-enable these for the 3.1 RC window? --js
Re: [Qemu-devel] [PATCH 05/15] hw/sh4/sh_pci: Use DeviceState::realize rather than SysBusDevice::init
On 10/2/18 3:13 PM, Peter Maydell wrote: > On 1 October 2018 at 23:09, Philippe Mathieu-Daudé wrote: >> Move from the legacy SysBusDevice::init method to using DeviceState::realize. > > Comment says DeviceState::realize but the code is using > PCIDevice::realize ? > > I didn't realize pci devices had their own realize method: > what's the difference between it and plain old DeviceState::realize, > which they also have, since PCIDeviceClass inherits from DeviceClass ? Cc'ing Michael and Marcel.
[Qemu-devel] [PATCH] softfloat: Fix division
The __udiv_qrnnd primitive that we nicked from gmp requires its
inputs to be normalized. We were not doing that. Because the
inputs are nearly normalized already, finishing that is trivial.
Inline div128to64 into its one user, because it is no longer a
general-purpose routine.
Fixes: cf07323d494
Fixes: https://bugs.launchpad.net/qemu/+bug/1793119
Signed-off-by: Richard Henderson
---
include/fpu/softfloat-macros.h | 48 ---
fpu/softfloat.c| 72 ++
2 files changed, 64 insertions(+), 56 deletions(-)
diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index 35e1603a5e..f29426ac46 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -625,54 +625,6 @@ static inline uint64_t estimateDiv128To64(uint64_t a0,
uint64_t a1, uint64_t b)
}
-/* From the GNU Multi Precision Library - longlong.h __udiv_qrnnd
- * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
- *
- * Licensed under the GPLv2/LGPLv3
- */
-static inline uint64_t div128To64(uint64_t n0, uint64_t n1, uint64_t d)
-{
-uint64_t d0, d1, q0, q1, r1, r0, m;
-
-d0 = (uint32_t)d;
-d1 = d >> 32;
-
-r1 = n1 % d1;
-q1 = n1 / d1;
-m = q1 * d0;
-r1 = (r1 << 32) | (n0 >> 32);
-if (r1 < m) {
-q1 -= 1;
-r1 += d;
-if (r1 >= d) {
-if (r1 < m) {
-q1 -= 1;
-r1 += d;
-}
-}
-}
-r1 -= m;
-
-r0 = r1 % d1;
-q0 = r1 / d1;
-m = q0 * d0;
-r0 = (r0 << 32) | (uint32_t)n0;
-if (r0 < m) {
-q0 -= 1;
-r0 += d;
-if (r0 >= d) {
-if (r0 < m) {
-q0 -= 1;
-r0 += d;
-}
-}
-}
-r0 -= m;
-
-/* Return remainder in LSB */
-return (q1 << 32) | q0 | (r0 != 0);
-}
-
/*
| Returns an approximation to the square root of the 32-bit significand given
| by `a'. Considered as an integer, `a' must be at least 2^31. If bit 0 of
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 9405f12a03..93edc06996 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -1112,19 +1112,75 @@ static FloatParts div_floats(FloatParts a, FloatParts
b, float_status *s)
bool sign = a.sign ^ b.sign;
if (a.cls == float_class_normal && b.cls == float_class_normal) {
-uint64_t temp_lo, temp_hi;
+uint64_t n0, n1, d0, d1, q0, q1, r1, r0, m, d;
int exp = a.exp - b.exp;
+
+/*
+ * We want the 2*N / N-bit division to produce exactly an N-bit
+ * result, so that we do not have to renormalize afterward.
+ * If A < B, then division would produce an (N-1)-bit result;
+ * shift A left by one to produce the an N-bit result, and
+ * decrement the exponent to match.
+ *
+ * The udiv_qrnnd algorithm that we're using requires normalization,
+ * i.e. the msb of the denominator must be set. Since we know that
+ * DECOMPOSED_BINARY_POINT is msb-1, everything must be shifted left
+ * by one more.
+ */
if (a.frac < b.frac) {
exp -= 1;
-shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT + 1,
- _hi, _lo);
+shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT + 2, ,
);
} else {
-shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT,
- _hi, _lo);
+shortShift128Left(0, a.frac, DECOMPOSED_BINARY_POINT + 1, ,
);
}
-/* LSB of quot is set if inexact which roundandpack will use
- * to set flags. Yet again we re-use a for the result */
-a.frac = div128To64(temp_lo, temp_hi, b.frac);
+d = b.frac << 1;
+
+/*
+ * From the GNU Multi Precision Library - longlong.h __udiv_qrnnd
+ * (https://gmplib.org/repo/gmp/file/tip/longlong.h)
+ * Licensed under the GPLv2/LGPLv3.
+ */
+d0 = (uint32_t)d;
+d1 = d >> 32;
+
+r1 = n1 % d1;
+q1 = n1 / d1;
+m = q1 * d0;
+r1 = (r1 << 32) | (n0 >> 32);
+if (r1 < m) {
+q1 -= 1;
+r1 += d;
+if (r1 >= d) {
+if (r1 < m) {
+q1 -= 1;
+r1 += d;
+}
+}
+}
+r1 -= m;
+
+r0 = r1 % d1;
+q0 = r1 / d1;
+m = q0 * d0;
+r0 = (r0 << 32) | (uint32_t)n0;
+if (r0 < m) {
+q0 -= 1;
+r0 += d;
+if (r0 >= d) {
+if (r0 < m) {
+q0 -= 1;
+r0 += d;
+}
+}
+}
+r0 -= m;
+/* End of __udiv_qrnnd. */
+
+/* Adjust remainder for normalization step. */
+r0 >>= 1;
+
+/* Set lsb if
Re: [Qemu-devel] [PATCH 09/15] hw/mips/malta: Replace 'empty_slot' by 'unimplemented_device'
On 10/2/18 3:23 PM, Peter Maydell wrote:
> On 1 October 2018 at 23:09, Philippe Mathieu-Daudé wrote:
>> The TYPE_EMPTY_SLOT and TYPE_UNIMPLEMENTED_DEVICE are identical devices,
>> however the later use more recent APIs and is more widely used.
>>
>> Replace 'empty_slot' by 'unimplemented_device' to simplify devices code
>> maintenance.
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> default-configs/mips-softmmu-common.mak | 1 -
>> hw/mips/mips_malta.c| 4 ++--
>> 2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/default-configs/mips-softmmu-common.mak
>> b/default-configs/mips-softmmu-common.mak
>> index fae2347ee7..f9d664e120 100644
>> --- a/default-configs/mips-softmmu-common.mak
>> +++ b/default-configs/mips-softmmu-common.mak
>> @@ -32,7 +32,6 @@ CONFIG_PFLASH_CFI01=y
>> CONFIG_I8259=y
>> CONFIG_MC146818RTC=y
>> CONFIG_ISA_TESTDEV=y
>> -CONFIG_EMPTY_SLOT=y
>> CONFIG_MIPS_CPS=y
>> CONFIG_MIPS_ITU=y
>> CONFIG_I2C=y
>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>> index 40041d5ec0..4ccfa87c35 100644
>> --- a/hw/mips/mips_malta.c
>> +++ b/hw/mips/mips_malta.c
>> @@ -53,7 +53,7 @@
>> #include "sysemu/qtest.h"
>> #include "qapi/error.h"
>> #include "qemu/error-report.h"
>> -#include "hw/empty_slot.h"
>> +#include "hw/misc/unimp.h"
>> #include "sysemu/kvm.h"
>> #include "exec/semihost.h"
>> #include "hw/mips/cps.h"
>> @@ -1216,7 +1216,7 @@ void mips_malta_init(MachineState *machine)
>> /* The whole address space decoded by the GT-64120A doesn't generate
>> exception when accessing invalid memory. Create an empty slot to
>> emulate this feature. */
>> -empty_slot_init(0, 0x2000);
>> +create_unimplemented_device("gt64120-SysAD", 0, 0x2000);
>>
>> qdev_init_nofail(dev);
>
> Not sure about this one. unimplemented_device means "there should
> be something here, but QEMU's model is incomplete", and if the user
> asks for 'unimp' warnings via the -d option then all accesses will
> generate logging. In this MIPS board, we're modelling the hardware's
> actual behaviour, and we shouldn't generate debug messages that
> imply that QEMU has unimplemented functionality here.
>
> If we were writing a model of the Malta board from scratch we'd
> probably do it by having the GT-64120A be modelled as a device
> which was a container object that instantiated all its various
> bits and pieces and mapped them into a MemoryRegion that spanned
> the whole 1GB size of that part of the address space. We could
> then give that MR a background region with the "RAZ/WI" behaviour
> the hardware requires.
Is 'git revert c7c3c9f8' a good example of your explanation?
[Qemu-devel] [Bug 1795527] Re: Malformed audio and video output stuttering after upgrade to QEMU 3.0
Hi, Hmm - if you say that changing hte -achine back to pc-i440fx-2.11 is helping then it should be something related to one of the compatibility entries for the machine type; but I don't see anything obvious related to audio or timing. To narrow it down a little, is pc-i440fx-2.12 good or bad? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1795527 Title: Malformed audio and video output stuttering after upgrade to QEMU 3.0 Status in QEMU: New Bug description: My host is an x86_64 Arch Linux OS with a recompiled 4.18.10 hardened kernel, running a few KVM guests with varying OSes and configurations managed through a Libvirt stack. Among these guests I have two Windows 10 VMs with VGA passthrough and PulseAudio-backed virtual audio devices. After upgrading to QEMU 3.0.0, both of the Win10 guests started showing corrupted audio output in the form of unnatural reproduction speed and occasional but consistently misplaced audio fragments originating from what seems to be a circular buffer wrapping over itself (misbehaviour detected by starting some games with known OSTs and dialogues: soundtracks sound accelerated and past dialogue lines start replaying middle-sentence until the next line starts playing). In addition, the video output of the malfunctioning VMs regularly stutters roughly twice a second for a fraction of a second (sync'ed with the suspected buffer wrapping and especially pronounced during not-pre-rendered cutscenes), toghether with mouse freezes that look like actual input misses more than simple lack of screen refreshes. The issue was succesfully reproduced without the managing stack, directly with the following command line, on the most capable Windows guest: QEMU_AUDIO_DRV=pa QEMU_PA_SERVER=127.0.0.1 /usr/bin/qemu-system-x86_64 -name guest=win10_gms,debug-threads=on \ -machine pc-i440fx-3.0,accel=kvm,usb=off,vmport=off,dump-guest-core=off \ -cpu host,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff,hv_vendor_id=123456789abc,kvm=off \ -drive file=/usr/share/ovmf/x64/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ -drive file=/var/lib/libvirt/qemu/nvram/win10_gms_VARS.fd,if=pflash,format=raw,unit=1 \ -m 5120 \ -realtime mlock=off \ -smp 3,sockets=1,cores=3,threads=1 \ -uuid 39b56ee2-6bae-4009-9108-7be26d5d63ac \ -display none \ -no-user-config \ -nodefaults \ -rtc base=localtime,driftfix=slew \ -global kvm-pit.lost_tick_policy=delay \ -no-hpet \ -no-shutdown \ -global PIIX4_PM.disable_s3=1 \ -global PIIX4_PM.disable_s4=1 \ -boot strict=on \ -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 \ -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 \ -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 \ -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2 \ -device ahci,id=sata0,bus=pci.0,addr=0x9 \ -drive file=/dev/vms/win10_gaming,format=raw,if=none,id=drive-virtio-disk0,cache=none,aio=native \ -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on \ -drive file=/dev/sr0,format=raw,if=none,id=drive-sata0-0-0,media=cdrom,readonly=on \ -device ide-cd,bus=sata0.0,drive=drive-sata0-0-0,id=sata0-0-0 \ -device intel-hda,id=sound0,bus=pci.0,addr=0x3 \ -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 \ -device usb-host,hostbus=2,hostaddr=3,id=hostdev0,bus=usb.0,port=1 \ -device vfio-pci,host=01:00.0,id=hostdev1,bus=pci.0,addr=0x6 \ -device vfio-pci,host=01:00.1,id=hostdev2,bus=pci.0,addr=0x7 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ -msg timestamp=on By "purposedly misconfiguring" the codepaths and replacing "pc-i440fx-3.0" with "pc-i440fx-2.11" (basically reverting the config changes I needed to do in
Re: [Qemu-devel] [PATCH] hw/s390x/s390-pci-bus: Convert sysbus init function to realize function
On 10/2/18 9:48 AM, Thomas Huth wrote:
> The SysBusDeviceClass->init() interface is considered as a legacy interface
> and there are currently some efforts going on to get rid of it. Thus let's
> convert the init function in the s390x code to realize() instead.
>
> Signed-off-by: Thomas Huth
Reviewed-by: Philippe Mathieu-Daudé
> ---
> hw/s390x/s390-pci-bus.c | 34 +-
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e3e0ebb..e42e1b8 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -692,27 +692,35 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus
> *bus, int32_t devfn)
> object_unref(OBJECT(iommu));
> }
>
> -static int s390_pcihost_init(SysBusDevice *dev)
> +static void s390_pcihost_realize(DeviceState *dev, Error **errp)
> {
> PCIBus *b;
> BusState *bus;
> PCIHostState *phb = PCI_HOST_BRIDGE(dev);
> S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
> +Error *local_err = NULL;
>
> DPRINTF("host_init\n");
>
> -b = pci_register_root_bus(DEVICE(dev), NULL,
> - s390_pci_set_irq, s390_pci_map_irq, NULL,
> - get_system_memory(), get_system_io(), 0, 64,
> - TYPE_PCI_BUS);
> +b = pci_register_root_bus(dev, NULL, s390_pci_set_irq, s390_pci_map_irq,
> + NULL, get_system_memory(), get_system_io(), 0,
> + 64, TYPE_PCI_BUS);
> pci_setup_iommu(b, s390_pci_dma_iommu, s);
>
> bus = BUS(b);
> -qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
> +qbus_set_hotplug_handler(bus, dev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> phb->bus = b;
>
> -s->bus = S390_PCI_BUS(qbus_create(TYPE_S390_PCI_BUS, DEVICE(s), NULL));
> -qbus_set_hotplug_handler(BUS(s->bus), DEVICE(s), NULL);
> +s->bus = S390_PCI_BUS(qbus_create(TYPE_S390_PCI_BUS, dev, NULL));
> +qbus_set_hotplug_handler(BUS(s->bus), dev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
>
> s->iommu_table = g_hash_table_new_full(g_int64_hash, g_int64_equal,
> NULL, g_free);
> @@ -722,9 +730,10 @@ static int s390_pcihost_init(SysBusDevice *dev)
> QTAILQ_INIT(>zpci_devs);
>
> css_register_io_adapters(CSS_IO_ADAPTER_PCI, true, false,
> - S390_ADAPTER_SUPPRESSIBLE, _abort);
> -
> -return 0;
> + S390_ADAPTER_SUPPRESSIBLE, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +}
> }
>
> static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
> @@ -1018,12 +1027,11 @@ static void s390_pcihost_reset(DeviceState *dev)
>
> static void s390_pcihost_class_init(ObjectClass *klass, void *data)
> {
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> DeviceClass *dc = DEVICE_CLASS(klass);
> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>
> dc->reset = s390_pcihost_reset;
> -k->init = s390_pcihost_init;
> +dc->realize = s390_pcihost_realize;
> hc->plug = s390_pcihost_hot_plug;
> hc->unplug = s390_pcihost_hot_unplug;
> msi_nonbroken = true;
>
Re: [Qemu-devel] [PATCH] vhost-user: Don't ask for reply on postcopy mem table set
On 10/02/2018 04:09 PM, Ilya Maximets wrote:
According to documentation, NEED_REPLY_MASK should not be set
for VHOST_USER_SET_MEM_TABLE request in postcopy mode.
This restriction was mistakenly applied to 'reply_supported'
variable, which is local and used only for non-postcopy case.
CC: Dr. David Alan Gilbert
Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu")
Signed-off-by: Ilya Maximets
---
hw/virtio/vhost-user.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
Reviewed-by: Maxime Coquelin
Thanks,
Maxime
Re: [Qemu-devel] [PATCH v4 10/10] qmp: common 'id' handling & make QGA conform to QMP spec
Hi On Sat, Sep 1, 2018 at 4:06 PM Marc-André Lureau wrote: > > On Sat, Sep 1, 2018 at 12:59 PM, Markus Armbruster wrote: > > Marc-André Lureau writes: > > > >> Let qmp_dispatch() copy the 'id' field. That way any qmp client will > >> conform to the specification, including QGA. Furthermore, it > >> simplifies the work for qemu monitor. > >> > >> Signed-off-by: Marc-André Lureau > >> Reviewed-by: Markus Armbruster > >> Reviewed-by: Michael Roth > >> --- > >> monitor.c | 33 - > >> qapi/qmp-dispatch.c | 10 -- > >> tests/test-qga.c| 13 + > >> 3 files changed, 25 insertions(+), 31 deletions(-) > > > > Let's squash in > > > > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt > > index 8f7da0245d..5fdfb2a4d0 100644 > > --- a/docs/interop/qmp-spec.txt > > +++ b/docs/interop/qmp-spec.txt > > @@ -109,6 +109,7 @@ or > >command execution, it is optional and will be part of the response > >if provided. The "id" member can be any json-value. A json-number > >incremented for each successive command works fine. > > +- Older versions of the QEMU Guest agent do not support "id". > > Good idea. Are you taking and updating the patch, or Michael? > any of you taking the patch? thanks -- Marc-André Lureau
Re: [Qemu-devel] [PATCH v4 09/10] qga: process_event() simplification
Hi On Thu, Aug 30, 2018 at 6:03 PM Markus Armbruster wrote: > > Marc-André Lureau writes: > > > Simplify the code around qmp_dispatch(): > > - rely on qmp_dispatch/check_obj() for message checking > > - have a single send_response() point > > - constify send_response() argument > > > > It changes a couple of error messages: > > > > * When @req isn't a dictionary, from > > Invalid JSON syntax > > to > > QMP input must be a JSON object > > > > * When @req lacks member "execute", from > > this feature or command is not currently supported > > to > > QMP input lacks member 'execute' > > > > Signed-off-by: Marc-André Lureau > > Reviewed-by: Michael Roth > > Reviewed-by: Markus Armbruster > Can someone take the patch? thanks -- Marc-André Lureau
Re: [Qemu-devel] ideas for improving TLB performance (help with TCG backend wanted)
On Tue, Oct 02, 2018 at 07:48:20 +0100, Alex Bennée wrote: > > Emilio G. Cota writes: > > > On Thu, Sep 20, 2018 at 01:19:51 +0100, Alex Bennée wrote: > >> If we are going to have an indirection then we can also drop the > >> requirement to scale the TLB according to the number of MMU indexes we > >> have to support. It's fairly wasteful when a bunch of them are almost > >> never used unless you are running stuff that uses them. > > > > So with dynamic TLB sizing, what you're suggesting here is to resize > > each MMU array independently (depending on their use rate) instead > > of using a single "TLB size" for all MMU indexes. Am I understanding > > your point correctly? > > Not quite - I think it would overly complicate the lookup to have a > differently sized TLB lookup for each mmu index - even if their usage > patterns are different. It just adds a load to get the mask, which will most likely be in the L1. The value is not used after 3 instructions later, when the L1 read will have completed. > I just meant that if we already have the cost of an indirection we don't > have to ensure: > > CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE]; > CPUIOTLBEntry iotlb[NB_MMU_MODES][CPU_TLB_SIZE]; > > restrict their sizes so any entry in the 2D array can be indexed > directly from env. Currently CPU_TLB_SIZE/CPU_TLB_BITS is restricted by > the number of NB_MMU_MODES we have to support. But if each can be > flushed and managed separately we can have: > > CPUTLBEntry *tlb_table[NB_MMU_MODES]; > > And size CPU_TLB_SIZE for the maximum offset we can mange in the lookup > code. This is mainly driven by the varying > TCG_TARGET_TLB_DISPLACEMENT_BITS each backend has available to it. What I implemented is what you suggest, but with dynamic resizing based on usage. I'm keeping the current CPU_TLB_SIZE as the minimum size, and took Pranith's TCG_TARGET_TLB_MAX_INDEX_BITS definitions (from 2017) to limit the max tlb size per mmu. I'll prepare an RFC. Thanks, Emilio
Re: [Qemu-devel] [PATCH] vhost-user: Don't ask for reply on postcopy mem table set
* Ilya Maximets (i.maxim...@samsung.com) wrote:
> According to documentation, NEED_REPLY_MASK should not be set
> for VHOST_USER_SET_MEM_TABLE request in postcopy mode.
> This restriction was mistakenly applied to 'reply_supported'
> variable, which is local and used only for non-postcopy case.
>
> CC: Dr. David Alan Gilbert
> Fixes: 9bb38019942c ("vhost+postcopy: Send address back to qemu")
> Signed-off-by: Ilya Maximets
Yes I think that's right; the 2nd part (in vhost_user_set_mem_table)
is easy; that's just left overs from before I split it into two.
The postcopy side we've already got the reply from the client
with the mappings, and now we've sent the OK at the end;
so there's no need for the final reply.
Reviewed-by: Dr. David Alan Gilbert
> ---
> hw/virtio/vhost-user.c | 13 +
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index b041343632..c442daa562 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -374,8 +374,6 @@ static int vhost_user_set_mem_table_postcopy(struct
> vhost_dev *dev,
> int fds[VHOST_MEMORY_MAX_NREGIONS];
> int i, fd;
> size_t fd_num = 0;
> -bool reply_supported = virtio_has_feature(dev->protocol_features,
> -
> VHOST_USER_PROTOCOL_F_REPLY_ACK);
> VhostUserMsg msg_reply;
> int region_i, msg_i;
>
> @@ -384,10 +382,6 @@ static int vhost_user_set_mem_table_postcopy(struct
> vhost_dev *dev,
> .hdr.flags = VHOST_USER_VERSION,
> };
>
> -if (reply_supported) {
> -msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> -}
> -
> if (u->region_rb_len < dev->mem->nregions) {
> u->region_rb = g_renew(RAMBlock*, u->region_rb, dev->mem->nregions);
> u->region_rb_offset = g_renew(ram_addr_t, u->region_rb_offset,
> @@ -503,10 +497,6 @@ static int vhost_user_set_mem_table_postcopy(struct
> vhost_dev *dev,
> return -1;
> }
>
> -if (reply_supported) {
> -return process_message_reply(dev, );
> -}
> -
> return 0;
> }
>
> @@ -519,8 +509,7 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
> size_t fd_num = 0;
> bool do_postcopy = u->postcopy_listen && u->postcopy_fd.handler;
> bool reply_supported = virtio_has_feature(dev->protocol_features,
> - VHOST_USER_PROTOCOL_F_REPLY_ACK) &&
> - !do_postcopy;
> +
> VHOST_USER_PROTOCOL_F_REPLY_ACK);
>
> if (do_postcopy) {
> /* Postcopy has enough differences that it's best done in it's own
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Qemu-devel] [PULL 24/80] change get_image_size return type to int64_t
From: Li Zhijian
Previously, if the size of initrd >=2G, qemu exits with error:
root@haswell-OptiPlex-9020:/home/lizj#
/home/lizhijian/lkp/qemu-colo/x86_64-softmmu/qemu-system-x86_64 -kernel
./vmlinuz-4.16.0-rc4 -initrd large.cgz -nographic
qemu: error reading initrd large.cgz: No such file or directory
root@haswell-OptiPlex-9020:/home/lizj# du -sh large.cgz
2.5Glarge.cgz
this patch changes the caller side that use this function to calculate
size of initrd file as well.
v2: update error message and int64_t printing format
Signed-off-by: Li Zhijian
Message-Id: <1536833233-14121-1-git-send-email-lizhij...@cn.fujitsu.com>
Signed-off-by: Paolo Bonzini
---
hw/alpha/dp264.c| 3 ++-
hw/core/loader.c| 5 +++--
hw/hppa/machine.c | 2 +-
hw/i386/pc.c| 7 ++-
hw/mips/mips_fulong2e.c | 6 +++---
hw/mips/mips_malta.c| 6 +++---
hw/mips/mips_mipssim.c | 3 +--
hw/mips/mips_r4k.c | 6 +++---
hw/moxie/moxiesim.c | 2 +-
include/hw/loader.h | 2 +-
10 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 80b987f..dd62f2a 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -150,7 +150,8 @@ static void clipper_init(MachineState *machine)
}
if (initrd_filename) {
-long initrd_base, initrd_size;
+long initrd_base;
+int64_t initrd_size;
initrd_size = get_image_size(initrd_filename);
if (initrd_size < 0) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 390987a..aa0b3fc 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -61,9 +61,10 @@
static int roms_loaded;
/* return the size or -1 if error */
-int get_image_size(const char *filename)
+int64_t get_image_size(const char *filename)
{
-int fd, size;
+int fd;
+int64_t size;
fd = open(filename, O_RDONLY | O_BINARY);
if (fd < 0)
return -1;
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 0fb8fb8..ac6dd7f 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -191,7 +191,7 @@ static void machine_hppa_init(MachineState *machine)
if (initrd_filename) {
ram_addr_t initrd_base;
-long initrd_size;
+int64_t initrd_size;
initrd_size = get_image_size(initrd_filename);
if (initrd_size < 0) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0314845..cd5029c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -838,7 +838,8 @@ static void load_linux(PCMachineState *pcms,
FWCfgState *fw_cfg)
{
uint16_t protocol;
-int setup_size, kernel_size, initrd_size = 0, cmdline_size;
+int setup_size, kernel_size, cmdline_size;
+int64_t initrd_size = 0;
int dtb_size, setup_data_offset;
uint32_t initrd_max;
uint8_t header[8192], *setup, *kernel, *initrd_data;
@@ -974,6 +975,10 @@ static void load_linux(PCMachineState *pcms,
fprintf(stderr, "qemu: error reading initrd %s: %s\n",
initrd_filename, strerror(errno));
exit(1);
+} else if (initrd_size >= initrd_max) {
+fprintf(stderr, "qemu: initrd is too large, cannot support."
+"(max: %"PRIu32", need %"PRId64")\n", initrd_max,
initrd_size);
+exit(1);
}
initrd_addr = (initrd_max-initrd_size) & ~4095;
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index c1694c8..2fbba32 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -104,9 +104,9 @@ static void GCC_FMT_ATTR(3, 4) prom_set(uint32_t* prom_buf,
int index,
static int64_t load_kernel (CPUMIPSState *env)
{
-int64_t kernel_entry, kernel_low, kernel_high;
+int64_t kernel_entry, kernel_low, kernel_high, initrd_size;
int index = 0;
-long kernel_size, initrd_size;
+long kernel_size;
ram_addr_t initrd_offset;
uint32_t *prom_buf;
long prom_size;
@@ -150,7 +150,7 @@ static int64_t load_kernel (CPUMIPSState *env)
prom_set(prom_buf, index++, "%s", loaderparams.kernel_filename);
if (initrd_size > 0) {
-prom_set(prom_buf, index++, "rd_start=0x%" PRIx64 " rd_size=%li %s",
+prom_set(prom_buf, index++, "rd_start=0x%" PRIx64 " rd_size=%" PRId64
" %s",
cpu_mips_phys_to_kseg0(NULL, initrd_offset), initrd_size,
loaderparams.kernel_cmdline);
} else {
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 40041d5..29b90ba 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -995,8 +995,8 @@ static void GCC_FMT_ATTR(3, 4) prom_set(uint32_t* prom_buf,
int index,
/* Kernel */
static int64_t load_kernel (void)
{
-int64_t kernel_entry, kernel_high;
-long kernel_size, initrd_size;
+int64_t kernel_entry, kernel_high, initrd_size;
+long kernel_size;
ram_addr_t initrd_offset;
int big_endian;
uint32_t *prom_buf;
@@ -1070,7 +1070,7 @@ static
[Qemu-devel] [PULL 53/80] test-char: fix random socket test failure
From: Marc-André Lureau
Peter reported a test failure on FreeBSD with the new reconnect test:
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
gtester -k --verbose -m=quick tests/test-char
TEST: tests/test-char... (pid=16190)
/char/null: OK
/char/invalid: OK
/char/ringbuf: OK
/char/mux: OK
/char/stdio: OK
/char/pipe: OK
/char/file: OK
/char/file-fifo: OK
/char/udp: OK
/char/serial:OK
/char/hotswap: OK
/char/socket/basic: OK
/char/socket/reconnect: FAIL
GTester: last random seed: R02S521380d9c12f1dac3ad1763bf5665c27
(pid=16367)
/char/socket/fdpass: OK
FAIL: tests/test-char
**
ERROR:tests/test-char.c:353:char_socket_test_common: assertion failed:
(object_property_get_bool(OBJECT(chr_client), "connected",
_abort))
It turns out that the socket test code checks both server and client
connection states, but doesn't wait for both.
Wait for the client side as well.
Signed-off-by: Marc-André Lureau
Reviewed-by: Paolo Bonzini
Message-Id: <20180823143125.16767-5-marcandre.lur...@redhat.com>
Signed-off-by: Paolo Bonzini
---
tests/test-char.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/tests/test-char.c b/tests/test-char.c
index 2a2ff32..431577a 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -347,6 +347,12 @@ static void char_socket_test_common(Chardev *chr)
g_assert_cmpint(id, >, 0);
main_loop();
+d.chr = chr_client;
+id = g_idle_add(char_socket_test_idle, );
+g_source_set_name_by_id(id, "test-idle");
+g_assert_cmpint(id, >, 0);
+main_loop();
+
g_assert(object_property_get_bool(OBJECT(chr), "connected", _abort));
g_assert(object_property_get_bool(OBJECT(chr_client),
"connected", _abort));
@@ -356,6 +362,7 @@ static void char_socket_test_common(Chardev *chr)
object_unparent(OBJECT(chr_client));
+d.chr = chr;
d.conn_expected = false;
g_idle_add(char_socket_test_idle, );
main_loop();
--
1.8.3.1
[Qemu-devel] [PULL v4 00/80] Misc patches for 2018-09-30
The following changes since commit 3892f1f1a963e59dfe012cd9d461d33b2986fa3b: Merge remote-tracking branch 'remotes/dgibson/tags/libfdt-20181002' into staging (2018-10-02 09:54:44 +0100) are available in the git repository at: git://github.com/bonzini/qemu.git tags/for-upstream for you to fetch changes up to 97866508669c4a75f531bfa94f8267900fcbb5dc: hw/scsi/mptendian: Avoid taking address of fields in packed structs (2018-10-02 19:09:14 +0200) * configure fix for environment variables (Daniel) * fix memory leaks (Alex) * x86_64 MTTCG fixes (Emilio) * introduce atomic64 (Emilio) * Fix for virtio hang (Fam, myself) * SH serial port fix (Geert) * Deprecate rotation_rate for scsi-block (Fam) * Extend memory-backend-file availability to all POSIX hosts (Hikaru) * Memory API cleanups and fixes (Igor, Li Qiang, Peter, Philippe) * MSI/IOMMU fix (Jan) * Socket reconnection fixes (Marc-André) * icount fixes (Emilio, myself) * QSP fixes for Coverity (myself) * Some record/replay improovements (Pavel) * Packed struct fixes (Peter) * Windows dump fixes and elf2dmp (Viktor) * kbmclock fix (Yongji) Alex Bennée (1): cpus: fix TCG kick timer leak Daniel P. Berrangé (1): configure: preserve various environment variables in config.status Emilio G. Cota (22): atomic: fix comment s/x64_64/x86_64/ cpus: initialize timers_state.vm_clock_lock cacheinfo: add i/d cache_linesize_log util: add atomic64 tests: add atomic64-bench qsp: use atomic64 accessors test-rcu-list: access n_reclaims and n_nodes_removed with atomic64 cpus: access .qemu_icount with atomic64 cpus: access .qemu_icount_bias with atomic64 target/i386: move cpu_cc_srcT to DisasContext target/i386: move cpu_A0 to DisasContext target/i386: move cpu_T0 to DisasContext target/i386: move cpu_T1 to DisasContext target/i386: move cpu_tmp0 to DisasContext target/i386: move cpu_tmp4 to DisasContext target/i386: move cpu_ptr0 to DisasContext target/i386: move cpu_ptr1 to DisasContext target/i386: move cpu_tmp2_i32 to DisasContext target/i386: move cpu_tmp3_i32 to DisasContext target/i386: move cpu_tmp1_i64 to DisasContext target/i386: move x86_64_hregs to DisasContext configure: enable mttcg for i386 and x86_64 Fam Zheng (2): virtio: Return true from virtio_queue_empty if broken scsi-block: Deprecate rotation_rate Geert Uytterhoeven (1): hw/char/sh_serial: Add timeout handling to unbreak serial input Hikaru Nishida (1): hostmem-file: make available memory-backend-file on POSIX-based hosts Igor Mammedov (1): memory: cleanup side effects of memory_region_init_foo() on failure Jan Kiszka (1): kvm: x86: Fix kvm_arch_fixup_msi_route for remap-less case Li Qiang (5): fw_cfg_mem: add read memory region callback hw: debugexit: add read callback hw: pc-testdev: add read memory region callback hw: hyperv_testdev: add read callback hw: edu: replace device name with macro Li Zhijian (1): change get_image_size return type to int64_t Liran Alon (1): i386: Compile CPUX86State xsave_buf only when support KVM or HVF Marc-André Lureau (10): hostmem-memfd: add checks before adding hostmem-memfd & properties util: add qemu_write_pidfile() util: use fcntl() for qemu_write_pidfile() locking Delete PID file on exit Revert "chardev: tcp: postpone TLS work until machine done" Revert "chardev: tcp: postpone async connection setup" char-socket: update all ioc handlers when changing context test-char: fix random socket test failure test-char: add socket reconnect test qom/object: add some interface asserts Mark Cave-Ayland (1): lsi53c895a: convert to trace-events Paolo Bonzini (9): qsp: hide indirect function calls from Coverity es1370: fix ADC_FRAMEADR and ADC_FRAMECNT cpus: take seqlock across qemu_icount updates serial: fix DLL writes char-pty: remove unnecessary #ifdef target/i386: unify masking of interrupts target/i386: rename HF_SVMI_MASK to HF_GUEST_MASK hvf: drop unused variable virtio: do not take address of packed members Pavel Dovgalyuk (10): ps2: prevent changing irq state on save and load replay: wake up vCPU when replaying replay: flush events when exiting translator: fix breakpoint processing replay: allow loading any snapshots before recording timer: introduce new virtual clock slirp: fix ipv6 timers ui: fix virtual timers target/i386: fix translation for icount mode replay: replay BH for IDE trim operation Peter Maydell (4): memory: Remove old_mmio accessors hw/nvram/fw_cfg: Use memberwise copy o
[Qemu-devel] Have multiple virtio-net devices, but only one of them receives all traffic
> Hi, > > I'm using QEMU 3.0.0 and Linux kernel 4.15.0 on x86 machines. I'm > observing pretty weird behavior when I have multiple virtio-net > devices. My KVM VM has two virtio-net devices (vhost=off) and I'm > using a Linux bridge in the host. The two devices have different > MAC/IP addresses. > > When I tried to access the VM using two different IP addresses (e.g > ping or ssh), I found that only one device in VM gets all incoming > network traffic while I expected that two devices get traffic for > their own IP addresses. > > I checked it in the several ways. > > 1) I did ping with two IP addresses from the host/other physical > machines in the same subnet, and only one device's interrupt count is > increased. > 2) I checked the ARP table from the ping sources, and two different IP > addresses have the same MAC address. In fact, I dumped ARP messages > using tcpdump, and the VM (or the bridge?) replied with the same MAC > address for two different IP addresses as attached below. > 3) I monitored the host bridge (# bridge monitor) and found that only > one device's MAC address is registered. > > It looks like one device's IP/MAC address is not advertised properly, > but I'm not really sure. When I turned off the device getting all the > traffic, then the other device starts getting incoming packets; the > device's MAC address is registered in the host bridge. The active > device only gets traffic for its own IP address, of course. > > Here's the tcpdump result. IP 10.10.1.100 and 10.10.1.221 are VM's IP > addresses. IP 10.10.1.221 is assigned to a device having > 52:54:00:12:34:58, but the log shows it is advertised as having > ...:57. I assume that both devices are in the same subnet. In this case kernel inside guest confused about to which port packets from this subnet should go. Look at the routing table (route -n) and you'll see that it uses only one port for sending packets. You just can't have two interfaces in the same subnet in Linux/any other OS. There is nothing QEMU specific here. > > 23:24:10.983700 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has > 10.10.1.100 tell kvm-dest-link-1, length 46 > 23:24:10.983771 ARP, Ethernet (len 6), IPv4 (len 4), Reply 10.10.1.100 > is-at 52:54:00:12:34:57 (oui Unknown), length 28 > 23:24:17.794811 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has > 10.10.1.211 tell kvm-dest-link-1, length 46 > 23:24:17.794869 ARP, Ethernet (len 6), IPv4 (len 4), Reply 10.10.1.211 > is-at 52:54:00:12:34:57 (oui Unknown), length 28 > > I would appreciate any help! > > Thanks, > Jintack
Re: [Qemu-devel] racing between pause_all_vcpus() and qemu_cpu_stop()
On 2 October 2018 at 17:46, Paolo Bonzini wrote: > On 02/10/2018 12:34, Peter Maydell wrote: >> Maybe I just don't understand what you're suggesting should be >> done via run-on-cpu. But it seems to me that the problem here >> is that cpu_stop_current() should not call qemu_cpu_stop() >> immediately, but instead arrange that this vCPU calls qemu_cpu_stop() >> when it gets back out of the execution loop. > > It will already do it in qemu_wait_io_event_common. So perhaps it's > enough to do cpu_exit(cpu) in pause_all_vcpus() and cpu_stop_current(), > instead of cpu_stop_current() which incorrectly sets cpu->stopped to true? Sounds plausible in the cpu_stop_current() case, but for pause_all_vcpus(), we need to (continue to) do something that makes the current CPU immediately set cpu->stopped to true, because the second part of the function is going to wait until all cpus (including this one) have cpu->stopped set. (That is, if the vcpu thread calls pause_all_vcpus() then we can't say "wait for this thread to get back out to the main loop": that's a deadlock. We could leave pause_all_vcpus() as it is, or if we cared we'd need to rewrite all the places that call pause_all_vcpus() from the vcpu thread to do something else instead. That's basically kvmvapic.c -- which could probably be rewritten to do something else than its current pause_all_vcpus/do stuff/resume_all_vcpus sequence -- plus vm_stop(), which has a related FIXME comment in it already.) thanks -- PMM
Re: [Qemu-devel] [PATCH] tests: check-qjson: fix a memory leak
On 10/2/18 11:26 AM, Li Qiang wrote:
Spotted by ASAN.
=
==17531==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 1120 byte(s) in 28 object(s) allocated from:
#0 0x7f9fb85eeb50 in __interceptor_malloc
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
#1 0x7f9fb824b8d8 in g_malloc
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x518d8)
#2 0x5654ae94f073 in qstring_from_str qobject/qstring.c:66
#3 0x5654ae94ede7 in qstring_new qobject/qstring.c:23
#4 0x5654ae953b6b in parse_string qobject/json-parser.c:143
#5 0x5654ae955e3e in parse_literal qobject/json-parser.c:484
#6 0x5654ae956227 in parse_value qobject/json-parser.c:547
#7 0x5654ae9565c7 in json_parser_parse qobject/json-parser.c:573
#8 0x5654ae952d5d in json_message_process_token qobject/json-streamer.c:92
#9 0x5654ae9608fa in json_lexer_feed_char qobject/json-lexer.c:313
#10 0x5654ae960ea5 in json_lexer_feed qobject/json-lexer.c:350
#11 0x5654ae9530de in json_message_parser_feed qobject/json-streamer.c:121
#12 0x5654ae950b6e in qobject_from_jsonv qobject/qjson.c:69
#13 0x5654ae950d30 in qobject_from_json qobject/qjson.c:83
#14 0x5654ae9449af in from_json_str tests/check-qjson.c:30
#15 0x5654ae946399 in utf8_string tests/check-qjson.c:781
#16 0x7f9fb826cc39 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72c39)
Signed-off-by: Li Qiang
---
tests/check-qjson.c | 1 +
1 file changed, 1 insertion(+)
Latent in commit 6ad8444f6, became a bug in commit c473c379.
Reviewed-by: Eric Blake
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index cc13f3d41e..d876a7a96e 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -780,6 +780,7 @@ static void utf8_string(void)
if (!strstr(json_out, "\\uFFFD")) {
str = from_json_str(json_out, j, _abort);
g_assert_cmpstr(qstring_get_try_str(str), ==, utf8_in);
+qobject_unref(str);
}
}
}
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] tests: check-qjson: fix a memory leak
Hi
On Tue, Oct 2, 2018 at 8:27 PM Li Qiang wrote:
>
> Spotted by ASAN.
>
> =
> ==17531==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 1120 byte(s) in 28 object(s) allocated from:
> #0 0x7f9fb85eeb50 in __interceptor_malloc
> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
> #1 0x7f9fb824b8d8 in g_malloc
> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x518d8)
> #2 0x5654ae94f073 in qstring_from_str qobject/qstring.c:66
> #3 0x5654ae94ede7 in qstring_new qobject/qstring.c:23
> #4 0x5654ae953b6b in parse_string qobject/json-parser.c:143
> #5 0x5654ae955e3e in parse_literal qobject/json-parser.c:484
> #6 0x5654ae956227 in parse_value qobject/json-parser.c:547
> #7 0x5654ae9565c7 in json_parser_parse qobject/json-parser.c:573
> #8 0x5654ae952d5d in json_message_process_token qobject/json-streamer.c:92
> #9 0x5654ae9608fa in json_lexer_feed_char qobject/json-lexer.c:313
> #10 0x5654ae960ea5 in json_lexer_feed qobject/json-lexer.c:350
> #11 0x5654ae9530de in json_message_parser_feed qobject/json-streamer.c:121
> #12 0x5654ae950b6e in qobject_from_jsonv qobject/qjson.c:69
> #13 0x5654ae950d30 in qobject_from_json qobject/qjson.c:83
> #14 0x5654ae9449af in from_json_str tests/check-qjson.c:30
> #15 0x5654ae946399 in utf8_string tests/check-qjson.c:781
> #16 0x7f9fb826cc39 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72c39)
>
> Signed-off-by: Li Qiang
already sent and queued by Markus:
https://patchew.org/QEMU/20180901211917.10372-1-marcandre.lur...@redhat.com/
> ---
> tests/check-qjson.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index cc13f3d41e..d876a7a96e 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -780,6 +780,7 @@ static void utf8_string(void)
> if (!strstr(json_out, "\\uFFFD")) {
> str = from_json_str(json_out, j, _abort);
> g_assert_cmpstr(qstring_get_try_str(str), ==, utf8_in);
> +qobject_unref(str);
> }
> }
> }
> --
> 2.17.1
>
>
>
--
Marc-André Lureau
[Qemu-devel] [PATCH 10/13] target/arm: Add v8M stack checks for T32 load/store single
Add v8M stack checks for the instructions in the T32
"load/store single" encoding class: these are the
"immediate pre-indexed" and "immediate, post-indexed"
LDR and STR instructions.
Signed-off-by: Peter Maydell
---
target/arm/translate.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 3fb378a492d..65df8d6975c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11624,7 +11624,6 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t
insn)
imm = -imm;
/* Fall through. */
case 0xf: /* Pre-increment. */
-tcg_gen_addi_i32(addr, addr, imm);
writeback = 1;
break;
default:
@@ -11636,6 +11635,28 @@ static void disas_thumb2_insn(DisasContext *s,
uint32_t insn)
issinfo = writeback ? ISSInvalid : rs;
+if (s->v8m_stackcheck && rn == 13 && writeback) {
+/*
+ * Stackcheck. Here we know 'addr' is the current SP;
+ * if imm is +ve we're moving SP up, else down. It is
+ * UNKNOWN whether the limit check triggers when SP starts
+ * below the limit and ends up above it; we chose to do so.
+ */
+if ((int32_t)imm < 0) {
+TCGv_i32 newsp = tcg_temp_new_i32();
+
+tcg_gen_addi_i32(newsp, addr, imm);
+gen_helper_v8m_stackcheck(cpu_env, newsp);
+tcg_temp_free_i32(newsp);
+} else {
+gen_helper_v8m_stackcheck(cpu_env, addr);
+}
+}
+
+if (writeback && !postinc) {
+tcg_gen_addi_i32(addr, addr, imm);
+}
+
if (insn & (1 << 20)) {
/* Load. */
tmp = tcg_temp_new_i32();
--
2.19.0
Re: [Qemu-devel] racing between pause_all_vcpus() and qemu_cpu_stop()
On 02/10/2018 12:34, Peter Maydell wrote: > On 2 October 2018 at 10:59, Paolo Bonzini wrote: >> On 02/10/2018 11:04, Peter Maydell wrote: >>> On 2 October 2018 at 09:58, Paolo Bonzini wrote: First, the reset code should indeed use run_on_cpu (it need not be safe i.e. stop-the-world; just run it in the vCPU thread). It certainly doesn't do this right now. >>> >>> I don't understand this part. We're resetting the entire world: >>> surely we need to stop the entire world first ? >> >> Most of the world is stopped because it only runs with BQL taken. vCPU >> isn't, so we ensure it is stopped by: 1) using run_on_cpu to synchronize >> with the executed TBs (or KVM_RUN) 2) ensuring the execution loop is >> paused after reset, which is the cpu_can_run part that you snipped. > > Maybe I just don't understand what you're suggesting should be > done via run-on-cpu. But it seems to me that the problem here > is that cpu_stop_current() should not call qemu_cpu_stop() > immediately, but instead arrange that this vCPU calls qemu_cpu_stop() > when it gets back out of the execution loop. It will already do it in qemu_wait_io_event_common. So perhaps it's enough to do cpu_exit(cpu) in pause_all_vcpus() and cpu_stop_current(), instead of cpu_stop_current() which incorrectly sets cpu->stopped to true? Paolo
[Qemu-devel] [PATCH 08/13] target/arm: Add v8M stack checks for LDRD/STRD (imm)
Add the v8M stack checks for:
* LDRD (immediate)
* STRD (immediate)
Loads and stores are more complicated than ADD/SUB/MOV, because we
must ensure that memory accesses below the stack limit are not
performed, so we can't simply do the check when we actually update
SP.
For these instructions, if the stack limit check triggers
we must not:
* perform any memory access below the SP limit
* update PC, SP or the load/store base register
but it is IMPDEF whether we:
* perform any accesses above or equal to the SP limit
* update destination registers for loads
For QEMU we choose to always check the limit before doing any other
part of the load or store, so we won't update any registers or
perform any memory accesses.
It is UNKNOWN whether the limit check triggers for a load or store
where the initial SP value is below the limit and one of the stores
would be below the limit, but the writeback moves SP to above the
limit. For QEMU we choose to trigger the check in this situation.
Note that limit checks happen only for loads and stores which update
SP via writeback; they do not happen for loads and stores which
simply use SP as a base register.
Signed-off-by: Peter Maydell
---
target/arm/translate.c | 27 +--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index fcb33b8a503..c16d6075d94 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10278,6 +10278,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t
insn)
* 0b_1001_x11x_____
* - load/store dual (pre-indexed)
*/
+bool wback = extract32(insn, 21, 1);
+
if (rn == 15) {
if (insn & (1 << 21)) {
/* UNPREDICTABLE */
@@ -10289,8 +10291,29 @@ static void disas_thumb2_insn(DisasContext *s,
uint32_t insn)
addr = load_reg(s, rn);
}
offset = (insn & 0xff) * 4;
-if ((insn & (1 << 23)) == 0)
+if ((insn & (1 << 23)) == 0) {
offset = -offset;
+}
+
+if (s->v8m_stackcheck && rn == 13 && wback) {
+/*
+ * Here 'addr' is the current SP; if offset is +ve we're
+ * moving SP up, else down. It is UNKNOWN whether the limit
+ * check triggers when SP starts below the limit and ends
+ * up above it; check whichever of the current and final
+ * SP is lower, so QEMU will trigger in that situation.
+ */
+if ((int32_t)offset < 0) {
+TCGv_i32 newsp = tcg_temp_new_i32();
+
+tcg_gen_addi_i32(newsp, addr, offset);
+gen_helper_v8m_stackcheck(cpu_env, newsp);
+tcg_temp_free_i32(newsp);
+} else {
+gen_helper_v8m_stackcheck(cpu_env, addr);
+}
+}
+
if (insn & (1 << 24)) {
tcg_gen_addi_i32(addr, addr, offset);
offset = 0;
@@ -10314,7 +10337,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t
insn)
gen_aa32_st32(s, tmp, addr, get_mem_index(s));
tcg_temp_free_i32(tmp);
}
-if (insn & (1 << 21)) {
+if (wback) {
/* Base writeback. */
tcg_gen_addi_i32(addr, addr, offset - 4);
store_reg(s, rn, addr);
--
2.19.0
[Qemu-devel] [PATCH 13/13] target/arm: Add v8M stack checks for MSR to SP_NS
Updating the NS stack pointer via MSR to SP_NS should include
a check whether the new SP value is below the stack limit.
No other kinds of update to the various stack pointer and
limit registers via MSR should perform a check.
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 074f7616272..712828674fa 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10963,11 +10963,23 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t
maskreg, uint32_t val)
* currently in handler mode or not, using the NS CONTROL.SPSEL.
*/
bool spsel = env->v7m.control[M_REG_NS] & R_V7M_CONTROL_SPSEL_MASK;
+bool is_psp = !arm_v7m_is_handler_mode(env) && spsel;
+uint32_t limit;
if (!env->v7m.secure) {
return;
}
-if (!arm_v7m_is_handler_mode(env) && spsel) {
+
+limit = is_psp ? env->v7m.psplim[false] : env->v7m.msplim[false];
+
+if (val < limit) {
+CPUState *cs = CPU(arm_env_get_cpu(env));
+
+cpu_restore_state(cs, GETPC(), true);
+raise_exception(env, EXCP_STKOF, 0, 1);
+}
+
+if (is_psp) {
env->v7m.other_ss_psp = val;
} else {
env->v7m.other_ss_msp = val;
--
2.19.0
[Qemu-devel] [PATCH 12/13] target/arm: Add v8M stack checks for VLDM/VSTM
Add the v8M stack checks for the VLDM/VSTM
(aka VPUSH/VPOP) instructions. This code is currently
unreachable because we haven't yet implemented M profile
floating point support, but since the change is simple,
we add it now because otherwise we're likely to forget to
do it later.
Signed-off-by: Peter Maydell
---
target/arm/translate.c | 12
1 file changed, 12 insertions(+)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ef64d2559de..2d3a1be518b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4229,6 +4229,18 @@ static int disas_vfp_insn(DisasContext *s, uint32_t insn)
if (insn & (1 << 24)) /* pre-decrement */
tcg_gen_addi_i32(addr, addr, -((insn & 0xff) << 2));
+if (rn == 13 && w && s->v8m_stackcheck) {
+/*
+ * Here 'addr' is the lowest address we will store to,
+ * and is either the old SP (if post-increment) or
+ * the new SP (if pre-decrement). For post-increment
+ * where the old value is below the limit and the new
+ * value is above, it is UNKNOWN whether the limit check
+ * triggers; we choose to trigger.
+ */
+gen_helper_v8m_stackcheck(cpu_env, addr);
+}
+
if (dp)
offset = 8;
else
--
2.19.0
[Qemu-devel] [PATCH 11/13] target/arm: Add v8M stack checks for Thumb push/pop
Add v8M stack checks for the 16-bit Thumb push/pop
encodings: STMDB, STMFD, LDM, LDMIA, LDMFD.
Signed-off-by: Peter Maydell
---
target/arm/translate.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 65df8d6975c..ef64d2559de 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12251,7 +12251,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t
insn)
store_reg(s, rd, tmp);
break;
case 4: case 5: case 0xc: case 0xd:
-/* push/pop */
+/*
+ * 0b1011_x10x__
+ * - push/pop
+ */
addr = load_reg(s, 13);
if (insn & (1 << 8))
offset = 4;
@@ -12264,6 +12267,17 @@ static void disas_thumb_insn(DisasContext *s, uint32_t
insn)
if ((insn & (1 << 11)) == 0) {
tcg_gen_addi_i32(addr, addr, -offset);
}
+
+if (s->v8m_stackcheck) {
+/*
+ * Here 'addr' is the lower of "old SP" and "new SP";
+ * if this is a pop that starts below the limit and ends
+ * above it, it is UNKNOWN whether the limit check triggers;
+ * we choose to trigger.
+ */
+gen_helper_v8m_stackcheck(cpu_env, addr);
+}
+
for (i = 0; i < 8; i++) {
if (insn & (1 << i)) {
if (insn & (1 << 11)) {
--
2.19.0
[Qemu-devel] [PATCH 06/13] target/arm: Add v8M stack checks on exception entry
Add checks for breaches of the v8M stack limit when the
stack pointer is decremented to push the exception frame
for exception entry.
Note that the exception-entry case is unique in that the
stack pointer is updated to be the limit value if the limit
is hit (per rule R_ZLZG).
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 54 ++---
1 file changed, 46 insertions(+), 8 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index ef8c244fb84..a10dff01a90 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6839,6 +6839,8 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t
lr, bool dotailchain,
uint32_t frameptr;
ARMMMUIdx mmu_idx;
bool stacked_ok;
+uint32_t limit;
+bool want_psp;
if (dotailchain) {
bool mode = lr & R_V7M_EXCRET_MODE_MASK;
@@ -6848,12 +6850,34 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t
lr, bool dotailchain,
mmu_idx = arm_v7m_mmu_idx_for_secstate_and_priv(env, M_REG_S, priv);
frame_sp_p = get_v7m_sp_ptr(env, M_REG_S, mode,
lr & R_V7M_EXCRET_SPSEL_MASK);
+want_psp = mode && (lr & R_V7M_EXCRET_SPSEL_MASK);
+if (want_psp) {
+limit = env->v7m.psplim[M_REG_S];
+} else {
+limit = env->v7m.msplim[M_REG_S];
+}
} else {
mmu_idx = core_to_arm_mmu_idx(env, cpu_mmu_index(env, false));
frame_sp_p = >regs[13];
+limit = v7m_sp_limit(env);
}
frameptr = *frame_sp_p - 0x28;
+if (frameptr < limit) {
+/*
+ * Stack limit failure: set SP to the limit value, and generate
+ * STKOF UsageFault. Stack pushes below the limit must not be
+ * performed. It is IMPDEF whether pushes above the limit are
+ * performed; we choose not to.
+ */
+qemu_log_mask(CPU_LOG_INT,
+ "...STKOF during callee-saves register stacking\n");
+env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
+armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
+env->v7m.secure);
+*frame_sp_p = limit;
+return true;
+}
/* Write as much of the stack frame as we can. A write failure may
* cause us to pend a derived exception.
@@ -6877,10 +6901,7 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t
lr, bool dotailchain,
v7m_stack_write(cpu, frameptr + 0x24, env->regs[11], mmu_idx,
ignore_faults);
-/* Update SP regardless of whether any of the stack accesses failed.
- * When we implement v8M stack limit checking then this attempt to
- * update SP might also fail and result in a derived exception.
- */
+/* Update SP regardless of whether any of the stack accesses failed. */
*frame_sp_p = frameptr;
return !stacked_ok;
@@ -7028,6 +7049,26 @@ static bool v7m_push_stack(ARMCPU *cpu)
frameptr -= 0x20;
+if (arm_feature(env, ARM_FEATURE_V8)) {
+uint32_t limit = v7m_sp_limit(env);
+
+if (frameptr < limit) {
+/*
+ * Stack limit failure: set SP to the limit value, and generate
+ * STKOF UsageFault. Stack pushes below the limit must not be
+ * performed. It is IMPDEF whether pushes above the limit are
+ * performed; we choose not to.
+ */
+qemu_log_mask(CPU_LOG_INT,
+ "...STKOF during stacking\n");
+env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
+armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
+env->v7m.secure);
+env->regs[13] = limit;
+return true;
+}
+}
+
/* Write as much of the stack frame as we can. If we fail a stack
* write this will result in a derived exception being pended
* (which may be taken in preference to the one we started with
@@ -7043,10 +7084,7 @@ static bool v7m_push_stack(ARMCPU *cpu)
v7m_stack_write(cpu, frameptr + 24, env->regs[15], mmu_idx, false) &&
v7m_stack_write(cpu, frameptr + 28, xpsr, mmu_idx, false);
-/* Update SP regardless of whether any of the stack accesses failed.
- * When we implement v8M stack limit checking then this attempt to
- * update SP might also fail and result in a derived exception.
- */
+/* Update SP regardless of whether any of the stack accesses failed. */
env->regs[13] = frameptr;
return !stacked_ok;
--
2.19.0
[Qemu-devel] [PATCH 09/13] target/arm: Add v8M stack checks for Thumb2 LDM/STM
Add the v8M stack checks for:
* LDM (T2 encoding)
* STM (T2 encoding)
This includes the 32-bit encodings of the instructions listed
in v8M ARM ARM rule R_YVWT as
* LDM, LDMIA, LDMFD
* LDMDB, LDMEA
* POP (multiple registers)
* PUSH (muliple registers)
* STM, STMIA, STMEA
* STMDB, STMFD
We perform the stack limit before doing any other part
of the load or store.
Signed-off-by: Peter Maydell
---
target/arm/translate.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index c16d6075d94..3fb378a492d 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10524,6 +10524,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t
insn)
} else {
int i, loaded_base = 0;
TCGv_i32 loaded_var;
+bool wback = extract32(insn, 21, 1);
/* Load/store multiple. */
addr = load_reg(s, rn);
offset = 0;
@@ -10531,10 +10532,26 @@ static void disas_thumb2_insn(DisasContext *s,
uint32_t insn)
if (insn & (1 << i))
offset += 4;
}
+
if (insn & (1 << 24)) {
tcg_gen_addi_i32(addr, addr, -offset);
}
+if (s->v8m_stackcheck && rn == 13 && wback) {
+/*
+ * If the writeback is incrementing SP rather than
+ * decrementing it, and the initial SP is below the
+ * stack limit but the final written-back SP would
+ * be above, then then we must not perform any memory
+ * accesses, but it is IMPDEF whether we generate
+ * an exception. We choose to do so in this case.
+ * At this point 'addr' is the lowest address, so
+ * either the original SP (if incrementing) or our
+ * final SP (if decrementing), so that's what we check.
+ */
+gen_helper_v8m_stackcheck(cpu_env, addr);
+}
+
loaded_var = NULL;
for (i = 0; i < 16; i++) {
if ((insn & (1 << i)) == 0)
@@ -10562,7 +10579,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t
insn)
if (loaded_base) {
store_reg(s, rn, loaded_var);
}
-if (insn & (1 << 21)) {
+if (wback) {
/* Base register writeback. */
if (insn & (1 << 24)) {
tcg_gen_addi_i32(addr, addr, -offset);
--
2.19.0
[Qemu-devel] [PATCH 04/13] target/arm: Add v8M stack checks on ADD/SUB/MOV of SP
Add code to insert calls to a helper function to do the stack
limit checking when we handle these forms of instruction
that write to SP:
* ADD (SP plus immediate)
* ADD (SP plus register)
* SUB (SP minus immediate)
* SUB (SP minus register)
* MOV (register)
Signed-off-by: Peter Maydell
---
target/arm/helper.h| 2 ++
target/arm/internals.h | 14
target/arm/op_helper.c | 19 ++
target/arm/translate.c | 80 +-
4 files changed, 106 insertions(+), 9 deletions(-)
diff --git a/target/arm/helper.h b/target/arm/helper.h
index 59e8c3bd1b9..8c9590091b0 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -69,6 +69,8 @@ DEF_HELPER_2(v7m_blxns, void, env, i32)
DEF_HELPER_3(v7m_tt, i32, env, i32, i32)
+DEF_HELPER_2(v8m_stackcheck, void, env, i32)
+
DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
DEF_HELPER_2(get_cp_reg, i32, env, ptr)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index bc4c01ccd92..966a8131623 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -811,4 +811,18 @@ static inline bool v7m_using_psp(CPUARMState *env)
return !arm_v7m_is_handler_mode(env) &&
env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
}
+
+/**
+ * v7m_sp_limit: Return SP limit for current CPU state
+ * Return the SP limit value for the current CPU security state
+ * and stack pointer.
+ */
+static inline uint32_t v7m_sp_limit(CPUARMState *env)
+{
+if (v7m_using_psp(env)) {
+return env->v7m.psplim[env->v7m.secure];
+} else {
+return env->v7m.msplim[env->v7m.secure];
+}
+}
#endif
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 952b8d122b7..38f885b290f 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -238,6 +238,25 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr
physaddr,
#endif /* !defined(CONFIG_USER_ONLY) */
+void HELPER(v8m_stackcheck)(CPUARMState *env, uint32_t newvalue)
+{
+/*
+ * Perform the v8M stack limit check for SP updates from translated code,
+ * raising an exception if the limit is breached.
+ */
+if (newvalue < v7m_sp_limit(env)) {
+CPUState *cs = CPU(arm_env_get_cpu(env));
+
+/*
+ * Stack limit exceptions are a rare case, so rather than syncing
+ * PC/condbits before the call, we use cpu_restore_state() to
+ * get them right before raising the exception.
+ */
+cpu_restore_state(cs, GETPC(), true);
+raise_exception(env, EXCP_STKOF, 0, 1);
+}
+}
+
uint32_t HELPER(add_setq)(CPUARMState *env, uint32_t a, uint32_t b)
{
uint32_t res = a + b;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 751d5811cee..25a8fe672f5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -239,6 +239,23 @@ static void store_reg(DisasContext *s, int reg, TCGv_i32
var)
tcg_temp_free_i32(var);
}
+/*
+ * Variant of store_reg which applies v8M stack-limit checks before updating
+ * SP. If the check fails this will result in an exception being taken.
+ * We disable the stack checks for CONFIG_USER_ONLY because we have
+ * no idea what the stack limits should be in that case.
+ * If stack checking is not being done this just acts like store_reg().
+ */
+static void store_sp_checked(DisasContext *s, TCGv_i32 var)
+{
+#ifndef CONFIG_USER_ONLY
+if (s->v8m_stackcheck) {
+gen_helper_v8m_stackcheck(cpu_env, var);
+}
+#endif
+store_reg(s, 13, var);
+}
+
/* Value extensions. */
#define gen_uxtb(var) tcg_gen_ext8u_i32(var, var)
#define gen_uxth(var) tcg_gen_ext16u_i32(var, var)
@@ -10583,7 +10600,13 @@ static void disas_thumb2_insn(DisasContext *s,
uint32_t insn)
if (gen_thumb2_data_op(s, op, conds, 0, tmp, tmp2))
goto illegal_op;
tcg_temp_free_i32(tmp2);
-if (rd != 15) {
+if (rd == 13 &&
+((op == 2 && rn == 15) ||
+ (op == 8 && rn == 13) ||
+ (op == 13 && rn == 13))) {
+/* MOV SP, ... or ADD SP, SP, ... or SUB SP, SP, ... */
+store_sp_checked(s, tmp);
+} else if (rd != 15) {
store_reg(s, rd, tmp);
} else {
tcg_temp_free_i32(tmp);
@@ -11267,8 +11290,15 @@ static void disas_thumb2_insn(DisasContext *s,
uint32_t insn)
gen_jmp(s, s->pc + offset);
}
} else {
-/* Data processing immediate. */
+/*
+ * 0b_0xxx__0xxx__
+ * - Data-processing (modified immediate, plain binary immediate)
+ */
if (insn & (1 << 25)) {
+/*
+ * 0b_0x1x__0xxx__
+ * - Data-processing (plain binary immediate)
+ */
if (insn & (1 <<
[Qemu-devel] [PATCH 07/13] target/arm: Add v8M stack limit checks on NS function calls
Check the v8M stack limits when pushing the frame for a
non-secure function call via BLXNS.
In order to be able to generate the exception we need to
promote raise_exception() from being local to op_helper.c
so we can call it from helper.c.
Signed-off-by: Peter Maydell
---
target/arm/internals.h | 9 +
target/arm/helper.c| 4
target/arm/op_helper.c | 4 ++--
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 966a8131623..aa124a06a9d 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -94,6 +94,15 @@ FIELD(V7M_EXCRET, RES1, 7, 25) /* including the must-be-1
prefix */
#define M_FAKE_FSR_NSC_EXEC 0xf /* NS executing in S memory */
#define M_FAKE_FSR_SFAULT 0xe /* SecureFault INVTRAN, INVEP or AUVIOL */
+/**
+ * raise_exception: Raise the specified exception.
+ * Raise a guest exception with the specified value, syndrome register
+ * and target exception level. This should be called from helper functions,
+ * and never returns because we will longjump back up to the CPU main loop.
+ */
+void QEMU_NORETURN raise_exception(CPUARMState *env, uint32_t excp,
+ uint32_t syndrome, uint32_t target_el);
+
/*
* For AArch64, map a given EL to an index in the banked_spsr array.
* Note that this mapping and the AArch32 mapping defined in bank_number()
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a10dff01a90..074f7616272 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6710,6 +6710,10 @@ void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
"BLXNS with misaligned SP is UNPREDICTABLE\n");
}
+if (sp < v7m_sp_limit(env)) {
+raise_exception(env, EXCP_STKOF, 0, 1);
+}
+
saved_psr = env->v7m.exception;
if (env->v7m.control[M_REG_S] & R_V7M_CONTROL_SFPA_MASK) {
saved_psr |= XPSR_SFPA;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 38f885b290f..de0d3984ea4 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -28,8 +28,8 @@
#define SIGNBIT (uint32_t)0x8000
#define SIGNBIT64 ((uint64_t)1 << 63)
-static void raise_exception(CPUARMState *env, uint32_t excp,
-uint32_t syndrome, uint32_t target_el)
+void raise_exception(CPUARMState *env, uint32_t excp,
+ uint32_t syndrome, uint32_t target_el)
{
CPUState *cs = CPU(arm_env_get_cpu(env));
--
2.19.0
[Qemu-devel] [PATCH 01/13] target/arm: Define new TBFLAG for v8M stack checking
The Arm v8M architecture includes hardware stack limit checking.
When certain instructions update the stack pointer, if the new
value of SP is below the limit set in the associated limit register
then an exception is taken. Add a TB flag that tracks whether
the limit-checking code needs to be emitted.
Signed-off-by: Peter Maydell
---
target/arm/cpu.h | 7 +++
target/arm/translate.h | 1 +
target/arm/helper.c| 10 ++
target/arm/translate.c | 1 +
4 files changed, 19 insertions(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 65c0fa0a659..d2c1d005ed7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1336,8 +1336,10 @@ FIELD(V7M_CCR, UNALIGN_TRP, 3, 1)
FIELD(V7M_CCR, DIV_0_TRP, 4, 1)
FIELD(V7M_CCR, BFHFNMIGN, 8, 1)
FIELD(V7M_CCR, STKALIGN, 9, 1)
+FIELD(V7M_CCR, STKOFHFNMIGN, 10, 1)
FIELD(V7M_CCR, DC, 16, 1)
FIELD(V7M_CCR, IC, 17, 1)
+FIELD(V7M_CCR, BP, 18, 1)
/* V7M SCR bits */
FIELD(V7M_SCR, SLEEPONEXIT, 1, 1)
@@ -2842,6 +2844,9 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState
*env)
/* For M profile only, Handler (ie not Thread) mode */
#define ARM_TBFLAG_HANDLER_SHIFT21
#define ARM_TBFLAG_HANDLER_MASK (1 << ARM_TBFLAG_HANDLER_SHIFT)
+/* For M profile only, whether we should generate stack-limit checks */
+#define ARM_TBFLAG_STACKCHECK_SHIFT 22
+#define ARM_TBFLAG_STACKCHECK_MASK (1 << ARM_TBFLAG_STACKCHECK_SHIFT)
/* Bit usage when in AArch64 state */
#define ARM_TBFLAG_TBI0_SHIFT 0/* TBI0 for EL0/1 or TBI for EL2/3 */
@@ -2884,6 +2889,8 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState
*env)
(((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
#define ARM_TBFLAG_HANDLER(F) \
(((F) & ARM_TBFLAG_HANDLER_MASK) >> ARM_TBFLAG_HANDLER_SHIFT)
+#define ARM_TBFLAG_STACKCHECK(F) \
+(((F) & ARM_TBFLAG_STACKCHECK_MASK) >> ARM_TBFLAG_STACKCHECK_SHIFT)
#define ARM_TBFLAG_TBI0(F) \
(((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
#define ARM_TBFLAG_TBI1(F) \
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 45f04244be8..c1b65f3efb0 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -38,6 +38,7 @@ typedef struct DisasContext {
int vec_stride;
bool v7m_handler_mode;
bool v8m_secure; /* true if v8M and we're in Secure mode */
+bool v8m_stackcheck; /* true if we need to perform v8M stack limit checks
*/
/* Immediate value in AArch32 SVC insn; must be set if is_jmp == DISAS_SWI
* so that top level loop can generate correct syndrome information.
*/
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5e721a65272..6ed8631dbee 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12667,6 +12667,16 @@ void cpu_get_tb_cpu_state(CPUARMState *env,
target_ulong *pc,
flags |= ARM_TBFLAG_HANDLER_MASK;
}
+/* v8M always applies stack limit checks unless CCR.STKOFHFNMIGN is
+ * suppressing them because the requested execution priority is less than
0.
+ */
+if (arm_feature(env, ARM_FEATURE_V8) &&
+arm_feature(env, ARM_FEATURE_M) &&
+!((mmu_idx & ARM_MMU_IDX_M_NEGPRI) &&
+ (env->v7m.ccr[env->v7m.secure] & R_V7M_CCR_STKOFHFNMIGN_MASK))) {
+flags |= ARM_TBFLAG_STACKCHECK_MASK;
+}
+
*pflags = flags;
*cs_base = 0;
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index c6a5d2ac444..751d5811cee 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12451,6 +12451,7 @@ static void arm_tr_init_disas_context(DisasContextBase
*dcbase, CPUState *cs)
dc->v7m_handler_mode = ARM_TBFLAG_HANDLER(dc->base.tb->flags);
dc->v8m_secure = arm_feature(env, ARM_FEATURE_M_SECURITY) &&
regime_is_secure(env, dc->mmu_idx);
+dc->v8m_stackcheck = ARM_TBFLAG_STACKCHECK(dc->base.tb->flags);
dc->cp_regs = cpu->cp_regs;
dc->features = env->features;
--
2.19.0
[Qemu-devel] [PATCH 00/13] target/arm: Implement v8M stack limit checks
This patchset implements the v8M stack limit checking feature, which is the last missing piece of the v8M architectural support. Note that the stack limit triggers when the SP value is changed to something below the limit, not when a load or store is performed below the limit. It's also done only for certain instructions that update SP, not for every possible way to change SP. For loads and stores which do writeback to SP there are also some rules about what parts of the load/store are permitted to happen if the check triggers -- we keep things simple by taking the approach of doing the check first so that no accesses are done. We take a straightforward approach to implementing the checks: generating a call to a helper function which does the comparison and might raise an exception. This obviously imposes some overhead for the common case where the limit isn't being breached, but generating code for a compare-and-conditionally-call seemed too tricky to insert into the existing code... thanks -- PMM Peter Maydell (13): target/arm: Define new TBFLAG for v8M stack checking target/arm: Define new EXCP type for v8M stack overflows target/arm: Move v7m_using_psp() to internals.h target/arm: Add v8M stack checks on ADD/SUB/MOV of SP target/arm: Add some comments in Thumb decode target/arm: Add v8M stack checks on exception entry target/arm: Add v8M stack limit checks on NS function calls target/arm: Add v8M stack checks for LDRD/STRD (imm) target/arm: Add v8M stack checks for Thumb2 LDM/STM target/arm: Add v8M stack checks for T32 load/store single target/arm: Add v8M stack checks for Thumb push/pop target/arm: Add v8M stack checks for VLDM/VSTM target/arm: Add v8M stack checks for MSR to SP_NS target/arm/cpu.h | 9 ++ target/arm/helper.h| 2 + target/arm/internals.h | 38 target/arm/translate.h | 1 + target/arm/helper.c| 99 - target/arm/op_helper.c | 23 - target/arm/translate.c | 198 + 7 files changed, 330 insertions(+), 40 deletions(-) -- 2.19.0
[Qemu-devel] [PATCH 05/13] target/arm: Add some comments in Thumb decode
Add some comments to the Thumb decoder indicating what bits
of the instruction have been decoded at various points in
the code.
This is not an exhaustive set of comments; we're gradually
adding comments as we work with particular bits of the code.
Signed-off-by: Peter Maydell
---
Specifically, I figured these out as I was going through looking
for the insns which write SP. These comments turn out not to
be relevant to those instructions, but I don't want to throw
them away.
---
target/arm/translate.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 25a8fe672f5..fcb33b8a503 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10623,6 +10623,10 @@ static void disas_thumb2_insn(DisasContext *s,
uint32_t insn)
tmp2 = load_reg(s, rm);
if ((insn & 0x70) != 0)
goto illegal_op;
+/*
+ * 0b_1010_0xxx_____:
+ * - MOV, MOVS (register-shifted register), flagsetting
+ */
op = (insn >> 21) & 3;
logic_cc = (insn & (1 << 20)) != 0;
gen_arm_shift_reg(tmp, op, tmp2, logic_cc);
@@ -11674,7 +11678,11 @@ static void disas_thumb_insn(DisasContext *s, uint32_t
insn)
rd = insn & 7;
op = (insn >> 11) & 3;
if (op == 3) {
-/* add/subtract */
+/*
+ * 0b0001_1xxx__
+ * - Add, subtract (three low registers)
+ * - Add, subtract (two low registers and immediate)
+ */
rn = (insn >> 3) & 7;
tmp = load_reg(s, rn);
if (insn & (1 << 10)) {
@@ -11711,7 +11719,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t
insn)
}
break;
case 2: case 3:
-/* arithmetic large immediate */
+/*
+ * 0b001x___
+ * - Add, subtract, compare, move (one low register and immediate)
+ */
op = (insn >> 11) & 3;
rd = (insn >> 8) & 0x7;
if (op == 0) { /* mov */
@@ -11848,7 +11859,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t
insn)
break;
}
-/* data processing register */
+/*
+ * 0b0100_00xx__
+ * - Data-processing (two low registers)
+ */
rd = insn & 7;
rm = (insn >> 3) & 7;
op = (insn >> 6) & 0xf;
--
2.19.0
[Qemu-devel] [PATCH 02/13] target/arm: Define new EXCP type for v8M stack overflows
Define EXCP_STKOF, and arrange for it to cause us to take
a UsageFault with CFSR.STKOF set.
Signed-off-by: Peter Maydell
---
target/arm/cpu.h| 2 ++
target/arm/helper.c | 5 +
2 files changed, 7 insertions(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d2c1d005ed7..318792823b9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -56,6 +56,7 @@
#define EXCP_SEMIHOST 16 /* semihosting call */
#define EXCP_NOCP 17 /* v7M NOCP UsageFault */
#define EXCP_INVSTATE 18 /* v7M INVSTATE UsageFault */
+#define EXCP_STKOF 19 /* v8M STKOF UsageFault */
/* NB: add new EXCP_ defines to the array in arm_log_exception() too */
#define ARMV7M_EXCP_RESET 1
@@ -1380,6 +1381,7 @@ FIELD(V7M_CFSR, UNDEFINSTR, 16 + 0, 1)
FIELD(V7M_CFSR, INVSTATE, 16 + 1, 1)
FIELD(V7M_CFSR, INVPC, 16 + 2, 1)
FIELD(V7M_CFSR, NOCP, 16 + 3, 1)
+FIELD(V7M_CFSR, STKOF, 16 + 4, 1)
FIELD(V7M_CFSR, UNALIGNED, 16 + 8, 1)
FIELD(V7M_CFSR, DIVBYZERO, 16 + 9, 1)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6ed8631dbee..c303dc453f1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7511,6 +7511,7 @@ static void arm_log_exception(int idx)
[EXCP_SEMIHOST] = "Semihosting call",
[EXCP_NOCP] = "v7M NOCP UsageFault",
[EXCP_INVSTATE] = "v7M INVSTATE UsageFault",
+[EXCP_STKOF] = "v8M STKOF UsageFault",
};
if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
@@ -7666,6 +7667,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVSTATE_MASK;
break;
+case EXCP_STKOF:
+armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
+env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_STKOF_MASK;
+break;
case EXCP_SWI:
/* The PC already points to the next instruction. */
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC, env->v7m.secure);
--
2.19.0
[Qemu-devel] [PATCH 03/13] target/arm: Move v7m_using_psp() to internals.h
We're going to want v7m_using_psp() in op_helper.c in the
next patch, so move it from helper.c to internals.h.
Signed-off-by: Peter Maydell
---
target/arm/internals.h | 15 +++
target/arm/helper.c| 12
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index dc9357766c9..bc4c01ccd92 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -796,4 +796,19 @@ static inline uint32_t arm_debug_exception_fsr(CPUARMState
*env)
}
}
+/**
+ * v7m_using_psp: Return true if using process stack pointer
+ * Return true if the CPU is currently using the process stack
+ * pointer, or false if it is using the main stack pointer.
+ */
+static inline bool v7m_using_psp(CPUARMState *env)
+{
+/* Handler mode always uses the main stack; for thread mode
+ * the CONTROL.SPSEL bit determines the answer.
+ * Note that in v7M it is not possible to be in Handler mode with
+ * CONTROL.SPSEL non-zero, but in v8M it is, so we must check both.
+ */
+return !arm_v7m_is_handler_mode(env) &&
+env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
+}
#endif
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c303dc453f1..ef8c244fb84 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6554,18 +6554,6 @@ pend_fault:
return false;
}
-/* Return true if we're using the process stack pointer (not the MSP) */
-static bool v7m_using_psp(CPUARMState *env)
-{
-/* Handler mode always uses the main stack; for thread mode
- * the CONTROL.SPSEL bit determines the answer.
- * Note that in v7M it is not possible to be in Handler mode with
- * CONTROL.SPSEL non-zero, but in v8M it is, so we must check both.
- */
-return !arm_v7m_is_handler_mode(env) &&
-env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK;
-}
-
/* Write to v7M CONTROL.SPSEL bit for the specified security bank.
* This may change the current stack pointer between Main and Process
* stack pointers if it is done for the CONTROL register for the current
--
2.19.0
[Qemu-devel] [PATCH] tests: check-qjson: fix a memory leak
Spotted by ASAN.
=
==17531==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 1120 byte(s) in 28 object(s) allocated from:
#0 0x7f9fb85eeb50 in __interceptor_malloc
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50)
#1 0x7f9fb824b8d8 in g_malloc
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x518d8)
#2 0x5654ae94f073 in qstring_from_str qobject/qstring.c:66
#3 0x5654ae94ede7 in qstring_new qobject/qstring.c:23
#4 0x5654ae953b6b in parse_string qobject/json-parser.c:143
#5 0x5654ae955e3e in parse_literal qobject/json-parser.c:484
#6 0x5654ae956227 in parse_value qobject/json-parser.c:547
#7 0x5654ae9565c7 in json_parser_parse qobject/json-parser.c:573
#8 0x5654ae952d5d in json_message_process_token qobject/json-streamer.c:92
#9 0x5654ae9608fa in json_lexer_feed_char qobject/json-lexer.c:313
#10 0x5654ae960ea5 in json_lexer_feed qobject/json-lexer.c:350
#11 0x5654ae9530de in json_message_parser_feed qobject/json-streamer.c:121
#12 0x5654ae950b6e in qobject_from_jsonv qobject/qjson.c:69
#13 0x5654ae950d30 in qobject_from_json qobject/qjson.c:83
#14 0x5654ae9449af in from_json_str tests/check-qjson.c:30
#15 0x5654ae946399 in utf8_string tests/check-qjson.c:781
#16 0x7f9fb826cc39 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72c39)
Signed-off-by: Li Qiang
---
tests/check-qjson.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index cc13f3d41e..d876a7a96e 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -780,6 +780,7 @@ static void utf8_string(void)
if (!strstr(json_out, "\\uFFFD")) {
str = from_json_str(json_out, j, _abort);
g_assert_cmpstr(qstring_get_try_str(str), ==, utf8_in);
+qobject_unref(str);
}
}
}
--
2.17.1
Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs
On 2 October 2018 at 16:48, Cédric Le Goater wrote: > On 10/2/18 12:56 PM, Peter Maydell wrote: >> Rather than having the DMA device directly grab the system_memory >> MR like this, it's better to have the device have a MemoryRegion >> property, which the SoC sets with whatever the DMA device should >> be able to see. > > ok. I see, but it seems I have a chicken & egg problem. > > The MemoryRegion I would liked to grab is the bmc->ram one in aspeed.c > but it is initialized after the SoC is. I don't know how to lazy bind > this region in the Aspeed SMC model using the Aspeed SoC model as a > proxy to pass the region property through a link or/and alias. > > If I could find a way, the model would be much cleaner. Usually what you want to pass is not the RAM region directly but some container object (which contains the RAM region among other things). This is often a container that you're passing to the SoC anyway so it can put its devices in it. (The direct answer to your chicken-and-egg problem is that you can create a container before creating the SoC, pass that to the SoC, then create the RAM afterwards and put it in the container after. But that feels a bit like a workaround for a less than ideal structure, somehow.) thanks -- PMM
Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional
* Denis V. Lunev (d...@openvz.org) wrote:
> According to PCI specification subsystem id and subsystem vendor id are
> optinal and could be abscent in Type1 header and can be found on
> different offsets within Type0 and Type2 headers.
>
> Thus we should make this data optional in struct PciDeviceId and skip
> reporting them via HMP if the information is not available.
>
> Additional (wrong information) about PCI bridges (Type1 devices) has been
> added in 5383a705 and fortunately not released. This patch fixes that
> problem. The problem was spotted by Markus.
>
> Signed-off-by: Denis V. Lunev
Looks good to me, thanks
Reviewed-by: Dr. David Alan Gilbert
> CC: "Dr. David Alan Gilbert"
> CC: Eric Blake
> CC: Markus Armbruster
> ---
> hmp.c | 6 --
> hw/pci/pci.c | 13 ++---
> qapi/misc.json | 4 ++--
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 3a9f797677..55633d29a3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -824,8 +824,10 @@ static void hmp_info_pci_device(Monitor *mon, const
> PciDeviceInfo *dev)
>
> monitor_printf(mon, ": PCI device %04" PRIx64 ":%04" PRIx64 "\n",
> dev->id->vendor, dev->id->device);
> -monitor_printf(mon, " PCI subsystem %04" PRIx64 ":%04" PRIx64 "\n",
> - dev->id->subsystem_vendor, dev->id->subsystem);
> +if (dev->id->has_subsystem_vendor && dev->id->has_subsystem) {
> +monitor_printf(mon, " PCI subsystem %04" PRIx64 ":%04" PRIx64
> "\n",
> + dev->id->subsystem_vendor, dev->id->subsystem);
> +}
>
> if (dev->has_irq) {
> monitor_printf(mon, " IRQ %" PRId64 ".\n", dev->irq);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 51d0dec466..b937f0dc0a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1737,9 +1737,6 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice
> *dev, PCIBus *bus,
> info->id = g_new0(PciDeviceId, 1);
> info->id->vendor = pci_get_word(dev->config + PCI_VENDOR_ID);
> info->id->device = pci_get_word(dev->config + PCI_DEVICE_ID);
> -info->id->subsystem = pci_get_word(dev->config + PCI_SUBSYSTEM_ID);
> -info->id->subsystem_vendor =
> -pci_get_word(dev->config + PCI_SUBSYSTEM_VENDOR_ID);
> info->regions = qmp_query_pci_regions(dev);
> info->qdev_id = g_strdup(dev->qdev.id ? dev->qdev.id : "");
>
> @@ -1752,6 +1749,16 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice
> *dev, PCIBus *bus,
> if (type == PCI_HEADER_TYPE_BRIDGE) {
> info->has_pci_bridge = true;
> info->pci_bridge = qmp_query_pci_bridge(dev, bus, bus_num);
> +} else if (type == PCI_HEADER_TYPE_NORMAL) {
> +info->id->has_subsystem = info->id->has_subsystem_vendor = true;
> +info->id->subsystem = pci_get_word(dev->config + PCI_SUBSYSTEM_ID);
> +info->id->subsystem_vendor =
> +pci_get_word(dev->config + PCI_SUBSYSTEM_VENDOR_ID);
> +} else if (type == PCI_HEADER_TYPE_CARDBUS) {
> +info->id->has_subsystem = info->id->has_subsystem_vendor = true;
> +info->id->subsystem = pci_get_word(dev->config +
> PCI_CB_SUBSYSTEM_ID);
> +info->id->subsystem_vendor =
> +pci_get_word(dev->config + PCI_CB_SUBSYSTEM_VENDOR_ID);
> }
>
> return info;
> diff --git a/qapi/misc.json b/qapi/misc.json
> index ada9af5add..95a6ed022d 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -839,8 +839,8 @@
> # Since: 2.4
> ##
> { 'struct': 'PciDeviceId',
> - 'data': {'device': 'int', 'vendor': 'int', 'subsystem': 'int',
> -'subsystem-vendor': 'int'} }
> + 'data': {'device': 'int', 'vendor': 'int', '*subsystem': 'int',
> +'*subsystem-vendor': 'int'} }
>
> ##
> # @PciDeviceInfo:
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 08/15] hw/mips/gt64xxx_pci: Mark as bridge device
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote:
> The gt64120 is currently listed as uncategorized device.
> Mark it as bridge device.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
> hw/mips/gt64xxx_pci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index dcd1a66329..1cd8aac658 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1232,6 +1232,7 @@ static void gt64120_class_init(ObjectClass *klass, void
> *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> +set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> dc->reset = gt64120_reset;
> dc->vmsd = _gt64120;
> }
>
Re: [Qemu-devel] [PATCH 05/15] hw/sh4/sh_pci: Use DeviceState::realize rather than SysBusDevice::init
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote:
> Move from the legacy SysBusDevice::init method to using DeviceState::realize.
>
> Signed-off-by: Philippe Mathieu-Daudé
one question below,
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
> hw/sh4/sh_pci.c | 20 +---
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index 4ec2e35500..84c52df067 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -120,16 +120,15 @@ static void sh_pci_set_irq(void *opaque, int irq_num,
> int level)
> qemu_set_irq(pic[irq_num], level);
> }
>
> -static int sh_pci_device_init(SysBusDevice *dev)
> +static void sh_pci_device_realize(PCIDevice *dev, Error **errp)
> {
> -PCIHostState *phb;
> -SHPCIState *s;
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +SHPCIState *s = SH_PCI_HOST_BRIDGE(dev);
> +PCIHostState *phb = PCI_HOST_BRIDGE(s);
> int i;
>
> -s = SH_PCI_HOST_BRIDGE(dev);
> -phb = PCI_HOST_BRIDGE(s);
> for (i = 0; i < 4; i++) {
is that PCI_NUM_PINS ?
> -sysbus_init_irq(dev, >irq[i]);
> +sysbus_init_irq(sbd, >irq[i]);
> }
> phb->bus = pci_register_root_bus(DEVICE(dev), "pci",
> sh_pci_set_irq, sh_pci_map_irq,
> @@ -143,13 +142,12 @@ static int sh_pci_device_init(SysBusDevice *dev)
> >memconfig_p4, 0, 0x224);
> memory_region_init_alias(>isa, OBJECT(s), "sh_pci.isa",
> get_system_io(), 0, 0x4);
> -sysbus_init_mmio(dev, >memconfig_p4);
> -sysbus_init_mmio(dev, >memconfig_a7);
> +sysbus_init_mmio(sbd, >memconfig_p4);
> +sysbus_init_mmio(sbd, >memconfig_a7);
> s->iobr = 0xfe24;
> memory_region_add_subregion(get_system_memory(), s->iobr, >isa);
>
> s->dev = pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "sh_pci_host");
> -return 0;
> }
>
> static void sh_pci_host_realize(PCIDevice *d, Error **errp)
> @@ -187,9 +185,9 @@ static const TypeInfo sh_pci_host_info = {
>
> static void sh_pci_device_class_init(ObjectClass *klass, void *data)
> {
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -sdc->init = sh_pci_device_init;
> +k->realize = sh_pci_device_realize;
> }
>
> static const TypeInfo sh_pci_device_info = {
>
Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs
With the links, [ ... ] >> Otherwise, patch looks good, though I don't know enough about >> the device/SoC to review those details. > > For the moment the only use of these registers is in the Aspeed custom > u-boot of the SDK : https://github.com/openbmc/u-boot/blob/v2016.07-aspeed-openbmc/arch/arm/mach-aspeed/platform_g5.S#L2314 > or in the rewrite I proposed in mainline : http://patchwork.ozlabs.org/patch/972868/ C.
Re: [Qemu-devel] [PATCH v2 00/11] aspeed: misc fixes and enhancements (SMC)
On 9/25/18 2:30 PM, Peter Maydell wrote: > On 25 September 2018 at 13:20, Peter Maydell wrote: >> On 21 September 2018 at 17:19, Cédric Le Goater wrote: >>> Hello, >>> >>> This series adds a couple of cleanups and two main features to the >>> SMC controller of the Aspeed machines : >>> >>> - a 'execute-in-place' property to boot directly from a memory region >>>alias of the FMC flash module using MMIO execution. This is not >>>activated by default because boot time needs to be improved on >>>recent firmwares. It also breaks migration compatibility. >>> >>> - support for DMA to access the flash modules. Our primary need is >>>the checksum calculation which is used to evaluate the best clock >>>settings for reads. >> >> I've picked out the easy parts of this series and added >> them to target-arm.next: >> >>> aspeed/timer: fix compile breakage with clang 3.4.2 >>> hw/arm/aspeed: change the FMC flash model of the AST2500 evb >>> hw/arm/aspeed: Add an Aspeed machine class >>> aspeed/smc: fix some alignment issues >> >> and left the rest for the moment (I've still got this series >> on my to-review list). > > PS: review from people familiar with the SoC would also be helpful. Joel has reviewed the u-boot patches. May be, he can chime in ? Thanks, C.
Re: [Qemu-devel] [PATCH 04/15] hw/ssi/xilinx_spi: Use DeviceState::realize rather than SysBusDevice::init
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote:
> Move from the legacy SysBusDevice::init method to using DeviceState::realize.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
> hw/ssi/xilinx_spi.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
> index 83585bc8b2..3dae303d5b 100644
> --- a/hw/ssi/xilinx_spi.c
> +++ b/hw/ssi/xilinx_spi.c
> @@ -319,9 +319,9 @@ static const MemoryRegionOps spi_ops = {
> }
> };
>
> -static int xilinx_spi_init(SysBusDevice *sbd)
> +static void xilinx_spi_realize(DeviceState *dev, Error **errp)
> {
> -DeviceState *dev = DEVICE(sbd);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> XilinxSPI *s = XILINX_SPI(dev);
> int i;
>
> @@ -344,8 +344,6 @@ static int xilinx_spi_init(SysBusDevice *sbd)
>
> fifo8_create(>tx_fifo, FIFO_CAPACITY);
> fifo8_create(>rx_fifo, FIFO_CAPACITY);
> -
> -return 0;
> }
>
> static const VMStateDescription vmstate_xilinx_spi = {
> @@ -368,9 +366,8 @@ static Property xilinx_spi_properties[] = {
> static void xilinx_spi_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> -k->init = xilinx_spi_init;
> +dc->realize = xilinx_spi_realize;
> dc->reset = xlx_spi_reset;
> dc->props = xilinx_spi_properties;
> dc->vmsd = _xilinx_spi;
>
Re: [Qemu-devel] [PATCH 10/15] hw/sparc64/niagara: Replace 'empty_slot' by 'unimplemented_device'
On Tue, Oct 2, 2018 at 3:24 PM Peter Maydell wrote:
>
> On 1 October 2018 at 23:09, Philippe Mathieu-Daudé wrote:
> > The TYPE_EMPTY_SLOT and TYPE_UNIMPLEMENTED_DEVICE are identical devices,
> > however the later use more recent APIs and is more widely used.
> >
> > Replace 'empty_slot' by 'unimplemented_device' to simplify devices code
> > maintenance.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
>
> > @@ -161,7 +161,7 @@ static void niagara_init(MachineState *machine)
> > serial_mm_init(sysmem, NIAGARA_UART_BASE, 0, NULL, 115200,
> > serial_hd(0), DEVICE_BIG_ENDIAN);
> > }
> > -empty_slot_init(NIAGARA_IOBBASE, NIAGARA_IOBSIZE);
> > +create_unimplemented_device("sun4v-iob", NIAGARA_IOBBASE,
> > NIAGARA_IOBSIZE);
> > sun4v_rtc_init(NIAGARA_RTC_BASE);
> > }
>
> Is this actually an unimplemented (missing) device, or are we
> implementing hardware-defined "no bus errors when this range is
> touched" behaviour ?
In this case it's really an unimplemented device. But in sun4m (patch
11/15) it's
"no bus errors when this range is touched" behaviour.
Artyom
--
Regards,
Artyom Tarasenko
SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu
Re: [Qemu-devel] [PATCH 02/15] hw/timer/sun4v-rtc: Convert from DPRINTF() macro to trace events
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
> hw/timer/sun4v-rtc.c | 13 +++--
> hw/timer/trace-events | 4
> 2 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/hw/timer/sun4v-rtc.c b/hw/timer/sun4v-rtc.c
> index 310523225f..13be94f8da 100644
> --- a/hw/timer/sun4v-rtc.c
> +++ b/hw/timer/sun4v-rtc.c
> @@ -14,15 +14,8 @@
> #include "hw/sysbus.h"
> #include "qemu/timer.h"
> #include "hw/timer/sun4v-rtc.h"
> +#include "trace.h"
>
> -//#define DEBUG_SUN4V_RTC
> -
> -#ifdef DEBUG_SUN4V_RTC
> -#define DPRINTF(fmt, ...) \
> -do { printf("sun4v_rtc: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do {} while (0)
> -#endif
>
> #define TYPE_SUN4V_RTC "sun4v_rtc"
> #define SUN4V_RTC(obj) OBJECT_CHECK(Sun4vRtc, (obj), TYPE_SUN4V_RTC)
> @@ -41,14 +34,14 @@ static uint64_t sun4v_rtc_read(void *opaque, hwaddr addr,
> /* accessing the high 32 bits */
> val >>= 32;
> }
> -DPRINTF("read from " TARGET_FMT_plx " val %lx\n", addr, val);
> +trace_sun4v_rtc_read(addr, val);
> return val;
> }
>
> static void sun4v_rtc_write(void *opaque, hwaddr addr,
> uint64_t val, unsigned size)
> {
> -DPRINTF("write 0x%x to " TARGET_FMT_plx "\n", (unsigned)val, addr);
> +trace_sun4v_rtc_read(addr, val);
> }
>
> static const MemoryRegionOps sun4v_rtc_ops = {
> diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> index ca9ad6321a..75bd3b1042 100644
> --- a/hw/timer/trace-events
> +++ b/hw/timer/trace-events
> @@ -66,5 +66,9 @@ cmsdk_apb_dualtimer_read(uint64_t offset, uint64_t data,
> unsigned size) "CMSDK A
> cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size)
> "CMSDK APB dualtimer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
> cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
>
> +# hw/timer/sun4v-rtc.c
> +sun4v_rtc_read(uint64_t addr, uint64_t value) "read: addr 0x%" PRIx64 "
> value 0x%" PRIx64
> +sun4v_rtc_write(uint64_t addr, uint64_t value) "write: addr 0x%" PRIx64 "
> value 0x%" PRIx64
> +
> # hw/timer/xlnx-zynqmp-rtc.c
> xlnx_zynqmp_rtc_gettime(int year, int month, int day, int hour, int min, int
> sec) "Get time from host: %d-%d-%d %2d:%02d:%02d"
>
Re: [Qemu-devel] [PATCH 07/15] hw/mips/gt64xxx_pci: Convert gt64120_reset() function into Device reset method
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote:
> Convert the gt64120_reset() function into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
> hw/mips/gt64xxx_pci.c | 17 +++--
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 24ad0ad024..dcd1a66329 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -992,9 +992,9 @@ static void gt64120_pci_set_irq(void *opaque, int
> irq_num, int level)
> }
>
>
> -static void gt64120_reset(void *opaque)
> +static void gt64120_reset(DeviceState *dev)
> {
> -GT64120State *s = opaque;
> +GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
>
> /* FIXME: Malta specific hw assumptions ahead */
>
> @@ -1184,16 +1184,6 @@ PCIBus *gt64120_register(qemu_irq *pic)
> return phb->bus;
> }
>
> -static int gt64120_init(SysBusDevice *dev)
> -{
> -GT64120State *s;
> -
> -s = GT64120_PCI_HOST_BRIDGE(dev);
> -
> -qemu_register_reset(gt64120_reset, s);
> -return 0;
> -}
> -
> static void gt64120_pci_realize(PCIDevice *d, Error **errp)
> {
> /* FIXME: Malta specific hw assumptions ahead */
> @@ -1241,9 +1231,8 @@ static const TypeInfo gt64120_pci_info = {
> static void gt64120_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>
> -sdc->init = gt64120_init;
> +dc->reset = gt64120_reset;
> dc->vmsd = _gt64120;
> }
>
>
Re: [Qemu-devel] [PATCH v2 04/11] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0
On 10/2/18 12:49 PM, Peter Maydell wrote: > On 21 September 2018 at 17:19, Cédric Le Goater wrote: >> The overhead for the OpenBMC firmware images using the a custom U-Boot >> is around 2 seconds, which is fine, but with a U-Boot from mainline, >> it takes an extra 50 seconds or so to reach Linux. A quick survey on >> the number of reads performed on the flash memory region gives the >> following figures : >> >> OpenBMC U-Boot 922478 (~ 3.5 MBytes) >> Mainline U-Boot 20569977 (~ 80 MBytes) >> >> QEMU must be trashing the TCG TBs and reloading text very often. Some >> addresses are read more than 250.000 times. Until we find a solution >> to improve boot time, execution from MMIO is not activated by default. >> >> Setting this option also breaks migration compatibility. >> >> Signed-off-by: Cédric Le Goater > > Reviewed-by: Peter Maydell The DRAM training in mainline u-boot is also significantly slower on real HW. I haven't spotted the exact area but the code is jumping back and forth. This patch should help tracking the culprit. Thanks, C.
Re: [Qemu-devel] [PATCH 01/15] trace-events: Fix copy/paste typo
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote: > Missed while reviewing 5dd85b4b486. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Thanks, C. > --- > hw/timer/trace-events | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/timer/trace-events b/hw/timer/trace-events > index fa4213df5b..ca9ad6321a 100644 > --- a/hw/timer/trace-events > +++ b/hw/timer/trace-events > @@ -56,7 +56,7 @@ systick_timer_tick(void) "systick reload" > systick_read(uint64_t addr, uint32_t value, unsigned size) "systick read > addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u" > systick_write(uint64_t addr, uint32_t value, unsigned size) "systick write > addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u" > > -# hw/char/cmsdk_apb_timer.c > +# hw/timer/cmsdk_apb_timer.c > cmsdk_apb_timer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK > APB timer read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u" > cmsdk_apb_timer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK > APB timer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u" > cmsdk_apb_timer_reset(void) "CMSDK APB timer: reset" >
Re: [Qemu-devel] [PATCH 03/15] hw/timer/sun4v-rtc: Use DeviceState::realize rather than SysBusDevice::init
On 10/2/18 12:09 AM, Philippe Mathieu-Daudé wrote:
> Move from the legacy SysBusDevice::init method to using DeviceState::realize.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
> ---
> hw/timer/sun4v-rtc.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/timer/sun4v-rtc.c b/hw/timer/sun4v-rtc.c
> index 13be94f8da..4e7f6a1eff 100644
> --- a/hw/timer/sun4v-rtc.c
> +++ b/hw/timer/sun4v-rtc.c
> @@ -63,21 +63,21 @@ void sun4v_rtc_init(hwaddr addr)
> sysbus_mmio_map(s, 0, addr);
> }
>
> -static int sun4v_rtc_init1(SysBusDevice *dev)
> +static void sun4v_rtc_realize(DeviceState *dev, Error **errp)
> {
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> Sun4vRtc *s = SUN4V_RTC(dev);
>
> memory_region_init_io(>iomem, OBJECT(s), _rtc_ops, s,
>"sun4v-rtc", 0x08ULL);
> -sysbus_init_mmio(dev, >iomem);
> -return 0;
> +sysbus_init_mmio(sbd, >iomem);
> }
>
> static void sun4v_rtc_class_init(ObjectClass *klass, void *data)
> {
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +DeviceClass *dc = DEVICE_CLASS(klass);
>
> -k->init = sun4v_rtc_init1;
> +dc->realize = sun4v_rtc_realize;
> }
>
> static const TypeInfo sun4v_rtc_info = {
>
Re: [Qemu-devel] [PATCH v2 00/11] aspeed: misc fixes and enhancements (SMC)
On 10/2/18 12:57 PM, Peter Maydell wrote: > On 25 September 2018 at 15:10, Cédric Le Goater wrote: >> On 9/25/18 2:20 PM, Peter Maydell wrote: >>> On 21 September 2018 at 17:19, Cédric Le Goater wrote: Hello, This series adds a couple of cleanups and two main features to the SMC controller of the Aspeed machines : - a 'execute-in-place' property to boot directly from a memory region alias of the FMC flash module using MMIO execution. This is not activated by default because boot time needs to be improved on recent firmwares. It also breaks migration compatibility. - support for DMA to access the flash modules. Our primary need is the checksum calculation which is used to evaluate the best clock settings for reads. >>> >>> I've picked out the easy parts of this series and added >>> them to target-arm.next: >>> aspeed/timer: fix compile breakage with clang 3.4.2 hw/arm/aspeed: change the FMC flash model of the AST2500 evb hw/arm/aspeed: Add an Aspeed machine class aspeed/smc: fix some alignment issues >>> >>> and left the rest for the moment (I've still got this series >>> on my to-review list). >> >> I will wait for your comments on the DMA part before resending. > > I've now commented on that patch. I think I'm done with review on > this version, unless there's something specific you want more > input on ? No, a part from the chicken & egg problem on the RAM memory region may be if you have some idea. I will dig in before re-sending anyhow. Thanks, C.
Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs
On 10/2/18 12:56 PM, Peter Maydell wrote:
> On 21 September 2018 at 17:19, Cédric Le Goater wrote:
>> The FMC controller on the Aspeed SoCs support DMA to access the flash
>> modules. It can operate in a normal mode, to copy to or from the flash
>> module mapping window, or in a checksum calculation mode, to evaluate
>> the best clock settings for reads.
>>
>> The model introduces a custom address space for DMAs populated with
>> the required regions : an alias region on the AHB window for the flash
>> devices and another alias on the SDRAM.
>>
>> Our primary need is to support the checksum calculation mode and the
>> model only implements synchronous DMA accesses. Something to improve
>> in the future.
>>
>> Signed-off-by: Cédric Le Goater
>
>
>> +static void aspeed_smc_dma_rw(AspeedSMCState *s)
>> +{
>> +MemTxResult result;
>> +uint32_t data;
>> +
>> +while (s->regs[R_DMA_LEN]) {
>> +if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
>> +result = address_space_read(>dma_as,
>> s->regs[R_DMA_DRAM_ADDR],
>> +MEMTXATTRS_UNSPECIFIED,
>> +(uint8_t *), 4);
>> +if (result != MEMTX_OK) {
>> +qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed
>> @%08x\n",
>> + __func__, s->regs[R_DMA_DRAM_ADDR]);
>> +return;
>> +}
>
> Does the device really not report DMA read/write failures via
> a status register bit or similar ?
The Interrupt Control and Status Register (FM0C8) has a DMA Status
BIT(11) to indicate that a DMA transfer is in progress or not. Nothing
for errors.
There are a SPI command abort status BIT(10) and a SPI Write Address
Protected status BIT(11) but these are for the command and address
filters.
>> +
>> +/*
>> + * Populate our custom address space for DMAs with only the regions we
>> + * need : the AHB window for the flash devices and the SDRAM.
>> + */
>> +static void aspeed_smc_dma_setup(AspeedSMCState *s)
>> +{
>> +char name[32];
>> +MemoryRegion *sysmem = get_system_memory();
>> +MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
>> +MemoryRegion *sdram_alias = g_new(MemoryRegion, 1);
>> +
>> +snprintf(name, sizeof(name), "%s-dma", s->ctrl->name);
>
> I would suggest using g_strdup_printf()/g_free(), since it's not
> immediately obvious here that s->ctrl->name is guaranteed
> to fit into the fixed-size array.
yes. sure.
>> +memory_region_init(>dma_mr, OBJECT(s), name,
>> + s->sdram_base + s->max_ram_size);
>> +address_space_init(>dma_as, >dma_mr, name);
>> +
>> +snprintf(name, sizeof(name), "%s.flash", s->ctrl->name);
>> +memory_region_init_alias(flash_alias, OBJECT(s), name, >mmio_flash,
>> + 0, s->ctrl->flash_window_size);
>> +memory_region_add_subregion(>dma_mr, s->ctrl->flash_window_base,
>> +flash_alias);
>> +
>> +memory_region_init_alias(sdram_alias, OBJECT(s), "ram", sysmem,
>> + s->sdram_base, s->max_ram_size);
>> +memory_region_add_subregion(>dma_mr, s->sdram_base, sdram_alias);
>
> Rather than having the DMA device directly grab the system_memory
> MR like this, it's better to have the device have a MemoryRegion
> property, which the SoC sets with whatever the DMA device should
> be able to see.
ok. I see, but it seems I have a chicken & egg problem.
The MemoryRegion I would liked to grab is the bmc->ram one in aspeed.c
but it is initialized after the SoC is. I don't know how to lazy bind
this region in the Aspeed SMC model using the Aspeed SoC model as a
proxy to pass the region property through a link or/and alias.
If I could find a way, the model would be much cleaner.
> Otherwise, patch looks good, though I don't know enough about
> the device/SoC to review those details.
For the moment the only use of these registers is in the Aspeed custom
u-boot of the SDK :
or in the rewrite I proposed in mainline :
Thanks,
C.
Re: [Qemu-devel] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler
>> Inside object_unparent(), the call flow of unrealize steps is defined. >> By moving the "real unplug" part into "do_unplug" and therefor >> essentially calling it when unrealizing, we could generalize this for >> all unplug handlers. >> I think, order of realization and therefore the order of hotplug handler >> calls is strictly defined already. Same applies to unrealization if we >> would factor the essential parts out into e.g. "do_unplug". That order >> is strictly encoded in device_set_realized() and bus_set_realized(). > > I don't see any benefits in adding do_unplug() in this case, > it would essentially replace hotplug_handler_unplug() in event origin > point with object_unparent() and renaming unplug() to do_unplug(). > > As result object_unparent() will start do more than unparenting and > destroying the device and a point where unplug originates becomes > poorly documented/lost for a reader among other object_unparent() calls. > > hotplug handlers are controller businesses and not the device one so > hiding (generalizing) it inside of device.realize() doesn't look > the right this to do, I'd rather keep these things separate as much > as possible. As long as we find another way to make this work I am happy. What I propose here works (and in my view does not violate any concepts, it just extends hotunplug handlers to device hierarchies getting unplugged). But I also understand the potential problems you think we could have in the future. Let's see if we can make your suggestion work. > >>> If I remember right, the suggested and partially implemented idea >>> in one of your previous series was to override default hotplug >>> handler with a machine one for plugged in device [1][2]. >>> However impl. wasn't exactly what I've suggested since it matches >>> all memory-devices. >>> >>> 1) qdev: let machine hotplug handler to override bus hotplug handler >>> 2) pc: route all memory devices through the machine hotplug handler >>> >>> So lets reiterate, we have TYPE_VIRTIO_PMEM and TYPE_VIRTIO_PMEM_PCI >>> the former implements TYPE_MEMORY_DEVICE interface and the later is >>> a wrapper PCI/whatnot device shim. >>> So when you plug that composite device you'd get 2 independent >>> plug hooks called, which makes it unrelable/broken design. >> >> Can you elaborate why this is broken? I don't consider the >> realize/unrealize order broken, and that is where we plug into. But yes, >> we logically plug a device hierarchy and therefore get a separate >> hotplug handler calls. > > 1st: > > consider we have a composite device A that contains B explicitly > manged by A (un)realizefn(). Now if we use your model of independent > callbacks we have only following fixed plug flow possible: >a>realize() > ::device_set_realized() > a:realizefn() > b->realize() >::device_set_realized() >b:realizefn() >hotplug_handler_plug(b) => b_hotplug_callback > ... > hotplug_handler_plug(a) => a_hotplug_callback > > that limits composite device wiring to external components to > the only possible order That why we have pre_plug, plug and post_plug handlers for I guess ... > 1st: b_hotplug_callback() + 2nd: a_hotplug_callback > and other way around for unplug() > > That however might not be order we need to do wiring/teardown though, > depending on composite device and controllers it might be necessary to > call callbacks in opposite order or mix both into one to do the wiring > correctly. And to do that we would need drop (move) b_hotplug_callback() > into a_hotplug_callback(), i.e. make it the way I've originally suggested. > > hotplug callback call flow might be different in child_bus case > (well, it's different in current code) and it might change in future > (ex: I'm looking into dropping hotplug_handler_post_plug() and > moving hotplug_handler_plug() to a later point to address the same > issue as commits 25e89788/8449bcf94 but without post_plug callback). .. however that sounds like a good idea, I was wondering the same why we actually need the post_plug handler. > > It's more robust for devices do not depend heavily on specific order > and define its plug sequence explicitly outside of device core. > This way it won't break apart when device core code is amended. > > 2nd: > from user point of view (and API) when composite device is created > we are dealing with 1 device (even if it's composed of many others internally, > it's not an external user business). The same should apply to hotplug > handlers. i.e. wire that device using whatever controllers are necessary > but do not jump through layers inside of device from external code > which hotplug handlers are. It somehow looks strange to have plug handler scattered all over device_set_realize(), while unplug handlers are at a completely different place and not involved in unrealize(). This makes one think that hotplugging a device hierarchy
Re: [Qemu-devel] [PATCH v9 3/6] s390x/kvm: enable AP instruction interpretation for guest
On 27/09/2018 09:52, David Hildenbrand wrote:
On 27/09/2018 00:54, Tony Krowiak wrote:
From: Tony Krowiak
Let's use the KVM_SET_DEVICE_ATTR ioctl to enable hardware
interpretation of AP instructions executed on the guest.
If the S390_FEAT_AP feature is switched on for the guest,
AP instructions must be interpreted by default; otherwise,
they will be intercepted.
This attribute setting may be overridden by a device. For example,
a device may want to provide AP instructions to the guest (i.e.,
S390_FEAT_AP turned on), but it may want to emulate them. In this
case, the AP instructions executed on the guest must be
intercepted; so when the device is realized, it must disable
interpretation.
Signed-off-by: Tony Krowiak
---
target/s390x/kvm.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 5277acd79a2c..d55d24abfd78 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2301,6 +2301,16 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model,
Error **errp)
S390_FEAT_MAX);
}
+static void kvm_s390_configure_apie(bool interpret)
+{
+uint64_t attr = interpret ? KVM_S390_VM_CRYPTO_ENABLE_APIE :
+KVM_S390_VM_CRYPTO_DISABLE_APIE;
+
+if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, attr)) {
+kvm_s390_set_attr(attr);
Wrong indentation, apart from that
Reviewed-by: David Hildenbrand
+}
+}
+
void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
{
struct kvm_s390_vm_cpu_processor prop = {
@@ -2350,6 +2360,10 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model,
Error **errp)
if (test_bit(S390_FEAT_CMM, model->features)) {
kvm_s390_enable_cmma();
}
+
+if (test_bit(S390_FEAT_AP, model->features)) {
+kvm_s390_configure_apie(true);
+}
}
void kvm_s390_restart_interrupt(S390CPU *cpu)
Tested-by: Pierre Morel
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
