Re: [Qemu-devel] [PATCH RFC 1/4] intel_iommu: Sanity check vfio-pci config on machine init done
Hi Peter,
On 8/12/19 9:45 AM, Peter Xu wrote:
> This check was previously only happened when the IOMMU is enabled in
s/happened/happening
> the guest. It was always too late because the enabling of IOMMU
> normally only happens during the boot of guest OS. It means that we
> can bail out and exit directly during the guest OS boots if the
> configuration of devices are not supported. Or, if the guest didn't
> enable vIOMMU at all, then the user can use the guest normally but as
> long as it reconfigure the guest OS to enable the vIOMMU then reboot,
reconfigures, and then reboots
> the user will see the panic right after the reset when the next boot
> starts.
>
> Let's make this failure even earlier so that we force the user to use
> caching-mode for vfio-pci devices when with the vIOMMU. So the user
> won't get surprise at least during execution of the guest, which seems
> a bit nicer.
>
> This will affect some user who didn't enable vIOMMU in the guest OS
> but was using vfio-pci and the vtd device in the past. However I hope
> it's not a majority because not enabling vIOMMU with the device
> attached is actually meaningless.
>
> We still keep the old assertion for safety so far because the hotplug
> path could still reach it, so far.
>
> Signed-off-by: Peter Xu
> ---
> hw/i386/intel_iommu.c | 38 +++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index de86f53b4e..642dd595ed 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -61,6 +61,13 @@
> static void vtd_address_space_refresh_all(IntelIOMMUState *s);
> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
>
> +static void vtd_panic_require_caching_mode(void)
> +{
> +error_report("We need to set caching-mode=on for intel-iommu to enable "
> + "device assignment with IOMMU protection.");
> +exit(1);
> +}
> +
> static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
> uint64_t wmask, uint64_t w1cmask)
> {
> @@ -2926,9 +2933,7 @@ static void
> vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> IntelIOMMUState *s = vtd_as->iommu_state;
>
> if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
> -error_report("We need to set caching-mode=on for intel-iommu to
> enable "
> - "device assignment with IOMMU protection.");
> -exit(1);
> +vtd_panic_require_caching_mode();
> }
>
> /* Update per-address-space notifier flags */
> @@ -3696,6 +3701,32 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> Error **errp)
> return true;
> }
>
> +static int vtd_machine_done_notify_one(Object *child, void *unused)
> +{
> +IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
> +
> +/*
> + * We hard-coded here because vfio-pci is the only special case
> + * here. Let's be more elegant in the future when we can, but so
> + * far there seems to be no better way.
> + */
> +if (object_dynamic_cast(child, "vfio-pci") && !iommu->caching_mode) {
> +vtd_panic_require_caching_mode();
> +}
> +
> +return 0;
> +}
> +
> +static void vtd_machine_done_hook(Notifier *notifier, void *unused)
> +{
> +object_child_foreach_recursive(object_get_root(),
> + vtd_machine_done_notify_one, NULL);
> +}
> +
> +static Notifier vtd_machine_done_notify = {
> +.notify = vtd_machine_done_hook,
> +};
> +
> static void vtd_realize(DeviceState *dev, Error **errp)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> @@ -3741,6 +3772,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
> pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
> /* Pseudo address space under root PCI bus. */
> pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
> +qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
This does not compile anymore on master. I think sysemu/sysemu.h needs
to be included as declaration of qemu_add_machine_init_done_notifier is
not found.
Besides
Reviewed-by: Eric Auger
Thanks
Eric
> }
>
> static void vtd_class_init(ObjectClass *klass, void *data)
>
Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc: fix signal delivery for ppc64abi32
David Gibson writes: > On Wed, Sep 11, 2019 at 10:33:45AM -0400, Richard Henderson wrote: >> On 9/11/19 5:39 AM, Alex Bennée wrote: >> > We were incorrectly setting NIP resulting in a segfault. This fixes >> > linux-test for this ABI. >> >> Perhaps better: >> We were incorrectly using the 64-bit AIX ABI instead of the 32-bit SYSV ABI >> for >> setting NIP for the signal handler. > > Applied to ppc-for-4.2, with Richard's updated description. > > For future reference, it's better to directly CC me on such patches. > I only barely keep on top of the mailing lists, so if you just send it > there it's likely to be some time before I pick it up, or even get > lost entirely. Should you be added to linux-user/ppc/ in MAINTAINERS? > >> >> ? >> >> > >> > Signed-off-by: Alex Bennée >> > --- >> > linux-user/ppc/signal.c | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> >> Anyway, >> Reviewed-by: Richard Henderson >> -- Alex Bennée
Re: [Qemu-devel] [PATCH RFC 2/4] qdev/machine: Introduce hotplug_allowed hook
Hi Peter,
On 8/12/19 9:45 AM, Peter Xu wrote:
> Introduce this new per-machine hook to give any machine class a chance
> to do a sanity check on the to-be-hotplugged device as a sanity test.
> This will be used for x86 to try to detect some illegal configuration
> of devices, e.g., possible conflictions between vfio-pci and x86
conflicts
> vIOMMU.
>
> Signed-off-by: Peter Xu
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> hw/core/qdev.c | 17 +
> include/hw/boards.h| 9 +
> include/hw/qdev-core.h | 1 +
> qdev-monitor.c | 7 +++
> 4 files changed, 34 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 94ebc0a4a1..d792b43c37 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -236,6 +236,23 @@ HotplugHandler
> *qdev_get_machine_hotplug_handler(DeviceState *dev)
> return NULL;
> }
>
> +bool qdev_hotplug_allowed(DeviceState *dev, Error **errp)
> +{
> +MachineState *machine;
> +MachineClass *mc;
> +Object *m_obj = qdev_get_machine();
> +
> +if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> +machine = MACHINE(m_obj);
> +mc = MACHINE_GET_CLASS(machine);
> +if (mc->hotplug_allowed) {
> +return mc->hotplug_allowed(machine, dev, errp);
> +}
> +}
> +
> +return true;
> +}
> +
> HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev)
> {
> if (dev->parent_bus) {
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index a71d1a53a5..1cf63be45d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -166,6 +166,13 @@ typedef struct {
> *The function pointer to hook different machine specific functions for
> *parsing "smp-opts" from QemuOpts to MachineState::CpuTopology and more
> *machine specific topology fields, such as smp_dies for PCMachine.
> + * @hotplug_allowed:
> + *If the hook is provided, then it'll be called for each device
> + *hotplug to check whether the device hotplug is allowed. Return
> + *true to grant allowance or false to reject the hotplug. When
> + *false is returned, an error must be set to show the reason of
> + *the rejection. If the hook is not provided, all hotplug will be
> + *allowed.
> */
> struct MachineClass {
> /*< private >*/
> @@ -223,6 +230,8 @@ struct MachineClass {
>
> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> DeviceState *dev);
> +bool (*hotplug_allowed)(MachineState *state, DeviceState *dev,
> +Error **errp);
> CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState
> *machine,
> unsigned cpu_index);
> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 136df7774c..88e7ec4b60 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -284,6 +284,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int
> alias_id,
> int required_for_version);
> HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
> HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> +bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
> /**
> * qdev_get_hotplug_handler: Get handler responsible for device wiring
> *
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 58222c2211..6c80602771 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -614,6 +614,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error
> **errp)
> /* create device */
> dev = DEVICE(object_new(driver));
>
> +/* Check whether the hotplug is allowed by the machine */
> +if (qdev_hotplug && !qdev_hotplug_allowed(dev, &err)) {
> +/* Error must be set in the machine hook */
> +assert(err);
> +goto err_del_dev;
> +}
> +
> if (bus) {
> qdev_set_parent_bus(dev, bus);
> } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
>
Re: [Qemu-devel] [PATCH RFC 3/4] pc/q35: Disallow vfio-pci hotplug without VT-d caching mode
Hi Peter,
On 8/12/19 9:45 AM, Peter Xu wrote:
> Instead of bailing out when trying to hotplug a vfio-pci device with
> below configuration:
>
> -device intel-iommu,caching-mode=off
>
> With this we can return a warning message to the user via QMP/HMP and
> the VM will continue to work after failing the hotplug:
>
> (qemu) device_add vfio-pci,bus=root.3,host=05:00.0,id=vfio1
> Error: Device assignment is not allowed without enabling caching-mode=on
> for Intel IOMMU.
>
> Signed-off-by: Peter Xu
Reviewed-by: Eric Auger
Eric
> ---
> hw/i386/pc.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 549c437050..4ea00c7bd2 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2905,6 +2905,26 @@ static void x86_nmi(NMIState *n, int cpu_index, Error
> **errp)
> }
> }
>
> +
> +static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error
> **errp)
> +{
> +X86IOMMUState *iommu = x86_iommu_get_default();
> +IntelIOMMUState *intel_iommu;
> +
> +if (iommu &&
> +object_dynamic_cast((Object *)iommu, TYPE_INTEL_IOMMU_DEVICE) &&
> +object_dynamic_cast((Object *)dev, "vfio-pci")) {
> +intel_iommu = INTEL_IOMMU_DEVICE(iommu);
> +if (!intel_iommu->caching_mode) {
> +error_setg(errp, "Device assignment is not allowed without "
> + "enabling caching-mode=on for Intel IOMMU.");
> +return false;
> +}
> +}
> +
> +return true;
> +}
> +
> static void pc_machine_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -2929,6 +2949,7 @@ static void pc_machine_class_init(ObjectClass *oc, void
> *data)
> pcmc->pvh_enabled = true;
> assert(!mc->get_hotplug_handler);
> mc->get_hotplug_handler = pc_get_hotplug_handler;
> +mc->hotplug_allowed = pc_hotplug_allowed;
> mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
> mc->get_default_cpu_node_id = pc_get_default_cpu_node_id;
> mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
>
Re: [Qemu-devel] [PATCH RFC 4/4] intel_iommu: Remove the caching-mode check during flag change
Hi Peter,
On 8/12/19 9:45 AM, Peter Xu wrote:
> That's never a good place to stop QEMU process... Since now we have
> both the machine done sanity check and also the hotplug handler, we
> can safely remove this to avoid that.
>
> Signed-off-by: Peter Xu
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> hw/i386/intel_iommu.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 642dd595ed..93b26b633b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2932,10 +2932,6 @@ static void
> vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> IntelIOMMUState *s = vtd_as->iommu_state;
>
> -if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
> -vtd_panic_require_caching_mode();
> -}
> -
> /* Update per-address-space notifier flags */
> vtd_as->notifier_flags = new;
>
>
Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()
On 16/09/19 04:02, Wei Yang wrote: > On Fri, Sep 13, 2019 at 11:12:05AM +0200, Paolo Bonzini wrote: >> On 13/09/19 01:02, Wei Yang wrote: >>> It shows PHYS_MAP_NODE_NIL may represents more node the tree could hold. >> >> Which is good, it means the assert can trigger. >> > > Per my understanding, it means the assert can't trigger. You're right, sorry. That's what I meant. Paolo >>> The assert here is not harmful, while maybe we can have a better way to >>> handle >>> it? >> >> I don't know... The assert just says "careful, someone treats >> PHYS_MAP_NODE_NIL specially!". It's documentation too. >> >> Paolo >
Re: [Qemu-devel] [Qemu-riscv] [PATCH v8 18/32] riscv: sifive_u: Set the minimum number of cpus to 2
Hi Jonathan, On Mon, Sep 16, 2019 at 1:40 AM Jonathan Behrens wrote: > > Has there been testing with "-smp 2"? A while back I thought I read that the > included uboot firmware was using a hard-coded device tree that indicated 4+1 > CPUs, which I would have expected to cause Linux boot issues? > No, U-Boot is using DTB that was passed from previous stage firmware - OpenSBI. On a real board this is 4 + 1. On QEMU, DTB is dynamically generated per "-smp n" settings. So there should be no problem. Regards, Bin
Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare
Am 13.09.2019 um 21:54 hat John Snow geschrieben: > > > On 9/13/19 11:25 AM, Sergio Lopez wrote: > > do_drive_backup() already acquires the AioContext, so release it > > before the call. > > > > Signed-off-by: Sergio Lopez > > --- > > blockdev.c | 6 +- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/blockdev.c b/blockdev.c > > index fbef6845c8..3927fdab80 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState > > *common, Error **errp) > > > > aio_context = bdrv_get_aio_context(bs); > > aio_context_acquire(aio_context); > > - Are you removing this unrelated empty line intentionally? > > /* Paired with .clean() */ > > bdrv_drained_begin(bs); > > Do we need to make this change to blockdev_backup_prepare as well? Actually, the whole structure feels a bit wrong. We get the bs here and take its lock, then release the lock again and forget the reference, only to do both things again inside do_drive_backup(). The way snapshots work is that the "normal" snapshot commands are wrappers that turn it into a single-entry transaction. Then you have only one code path where you can resolve the ID and take the lock just once. So maybe backup should work like this, too? Kevin
Re: [Qemu-devel] [PATCH 3/4] iotests: Add @error to wait_until_completed
On 14.09.19 00:53, John Snow wrote:
>
>
> On 9/12/19 9:56 AM, Max Reitz wrote:
>> Callers can use this new parameter to expect failure during the
>> completion process.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/qemu-iotests/iotests.py | 18 --
>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index b26271187c..300347c7c8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -745,15 +745,20 @@ class QMPTestCase(unittest.TestCase):
>> self.assert_no_active_block_jobs()
>> return result
>>
>> -def wait_until_completed(self, drive='drive0', check_offset=True,
>> wait=60.0):
>> +def wait_until_completed(self, drive='drive0', check_offset=True,
>> wait=60.0,
>> + error=None):
>> '''Wait for a block job to finish, returning the event'''
>> while True:
>> for event in self.vm.get_qmp_events(wait=wait):
>> if event['event'] == 'BLOCK_JOB_COMPLETED':
>> self.assert_qmp(event, 'data/device', drive)
>> -self.assert_qmp_absent(event, 'data/error')
>> -if check_offset:
>> -self.assert_qmp(event, 'data/offset',
>> event['data']['len'])
>> +if error is None:
>> +self.assert_qmp_absent(event, 'data/error')
>> +if check_offset:
>> +self.assert_qmp(event, 'data/offset',
>> +event['data']['len'])
>> +else:
>> +self.assert_qmp(event, 'data/error', error)
>> self.assert_no_active_block_jobs()
>> return event
>> elif event['event'] == 'JOB_STATUS_CHANGE':
>> @@ -771,7 +776,8 @@ class QMPTestCase(unittest.TestCase):
>> self.assert_qmp(event, 'data/type', 'mirror')
>> self.assert_qmp(event, 'data/offset', event['data']['len'])
>>
>> -def complete_and_wait(self, drive='drive0', wait_ready=True):
>> +def complete_and_wait(self, drive='drive0', wait_ready=True,
>> + completion_error=None):
>> '''Complete a block job and wait for it to finish'''
>> if wait_ready:
>> self.wait_ready(drive=drive)
>> @@ -779,7 +785,7 @@ class QMPTestCase(unittest.TestCase):
>> result = self.vm.qmp('block-job-complete', device=drive)
>> self.assert_qmp(result, 'return', {})
>>
>> -event = self.wait_until_completed(drive=drive)
>> +event = self.wait_until_completed(drive=drive,
>> error=completion_error)
>> self.assert_qmp(event, 'data/type', 'mirror')
>>
>> def pause_wait(self, job_id='job0'):
>>
>
> toot toot more optional parameters. lay them at the altar of
> noncommittal python design.
>
> I completely forget what the difference between unittest.TestCase and
> qtest.QEMUQtestMachine is and why they each have job management methods.
>
> Well, OK: the VM one is a simple subclass of the general-purpose VM
> machine to add some more useful stuff. the unittest one implements some
> general-purpose behavior with asserts that only work in the unittest world.
Yes. run_job doesn’t assert anything (well, I could check the returned
error, but that’s it), it’s just based on comparison to the reference
output. That’s not so useful for unittest-style tests, so it’s better
to use complete_and_wait() etc. there.
> Still,
>
> It's a little fun that we've got run_job as well as cancel_and_wait,
> wait_until_completed, wait_ready, wait_ready_and_cancel, pause_wait and
> pause_job and they all seem to implement job run-state logic management
> a little differently.
At the end of the day, it’s all just test code. It doesn’t hurt too
much for it to have duplicated code and be generally messy.
> Probably no bugs there, I bet.
Hm. The functions are not that complex.
> *cough* Not your fault, anyway, so please take this accolade:
>
> Reviewed-by: John Snow
Thanks!
> (it's probably my fault)
I can only imagine by nonassistance of code in danger.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC 1/4] intel_iommu: Sanity check vfio-pci config on machine init done
On Mon, Sep 16, 2019 at 09:11:50AM +0200, Auger Eric wrote:
> > static void vtd_realize(DeviceState *dev, Error **errp)
> > {
> > MachineState *ms = MACHINE(qdev_get_machine());
> > @@ -3741,6 +3772,7 @@ static void vtd_realize(DeviceState *dev, Error
> > **errp)
> > pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
> > /* Pseudo address space under root PCI bus. */
> > pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
> > +qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
> This does not compile anymore on master. I think sysemu/sysemu.h needs
> to be included as declaration of qemu_add_machine_init_done_notifier is
> not found.
Indeed. It's probably because we've changed some header inclusions
recently between each other. I'll repost a new version with it added.
>
> Besides
> Reviewed-by: Eric Auger
Thanks for the reviews!
--
Peter Xu
[Qemu-devel] [PATCH v2 1/4] intel_iommu: Sanity check vfio-pci config on machine init done
This check was previously only happened when the IOMMU is enabled in
the guest. It was always too late because the enabling of IOMMU
normally only happens during the boot of guest OS. It means that we
can bail out and exit directly during the guest OS boots if the
configuration of devices are not supported. Or, if the guest didn't
enable vIOMMU at all, then the user can use the guest normally but as
long as it reconfigure the guest OS to enable the vIOMMU then reboot,
the user will see the panic right after the reset when the next boot
starts.
Let's make this failure even earlier so that we force the user to use
caching-mode for vfio-pci devices when with the vIOMMU. So the user
won't get surprise at least during execution of the guest, which seems
a bit nicer.
This will affect some user who didn't enable vIOMMU in the guest OS
but was using vfio-pci and the vtd device in the past. However I hope
it's not a majority because not enabling vIOMMU with the device
attached is actually meaningless.
We still keep the old assertion for safety so far because the hotplug
path could still reach it, so far.
Signed-off-by: Peter Xu
---
hw/i386/intel_iommu.c | 38 +++---
1 file changed, 35 insertions(+), 3 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 75ca6f9c70..17fc309b3d 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -64,6 +64,13 @@
static void vtd_address_space_refresh_all(IntelIOMMUState *s);
static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
+static void vtd_panic_require_caching_mode(void)
+{
+error_report("We need to set caching-mode=on for intel-iommu to enable "
+ "device assignment with IOMMU protection.");
+exit(1);
+}
+
static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
uint64_t wmask, uint64_t w1cmask)
{
@@ -2929,9 +2936,7 @@ static void
vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
IntelIOMMUState *s = vtd_as->iommu_state;
if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
-error_report("We need to set caching-mode=on for intel-iommu to enable
"
- "device assignment with IOMMU protection.");
-exit(1);
+vtd_panic_require_caching_mode();
}
/* Update per-address-space notifier flags */
@@ -3699,6 +3704,32 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error
**errp)
return true;
}
+static int vtd_machine_done_notify_one(Object *child, void *unused)
+{
+IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
+
+/*
+ * We hard-coded here because vfio-pci is the only special case
+ * here. Let's be more elegant in the future when we can, but so
+ * far there seems to be no better way.
+ */
+if (object_dynamic_cast(child, "vfio-pci") && !iommu->caching_mode) {
+vtd_panic_require_caching_mode();
+}
+
+return 0;
+}
+
+static void vtd_machine_done_hook(Notifier *notifier, void *unused)
+{
+object_child_foreach_recursive(object_get_root(),
+ vtd_machine_done_notify_one, NULL);
+}
+
+static Notifier vtd_machine_done_notify = {
+.notify = vtd_machine_done_hook,
+};
+
static void vtd_realize(DeviceState *dev, Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
@@ -3744,6 +3775,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
/* Pseudo address space under root PCI bus. */
pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
+qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
}
static void vtd_class_init(ObjectClass *klass, void *data)
--
2.21.0
[Qemu-devel] [PATCH v2 3/4] pc/q35: Disallow vfio-pci hotplug without VT-d caching mode
Instead of bailing out when trying to hotplug a vfio-pci device with
below configuration:
-device intel-iommu,caching-mode=off
With this we can return a warning message to the user via QMP/HMP and
the VM will continue to work after failing the hotplug:
(qemu) device_add vfio-pci,bus=root.3,host=05:00.0,id=vfio1
Error: Device assignment is not allowed without enabling caching-mode=on for
Intel IOMMU.
Signed-off-by: Peter Xu
---
hw/i386/pc.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bad866fe44..0a6fa6e549 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2944,6 +2944,26 @@ static void x86_nmi(NMIState *n, int cpu_index, Error
**errp)
}
}
+
+static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error
**errp)
+{
+X86IOMMUState *iommu = x86_iommu_get_default();
+IntelIOMMUState *intel_iommu;
+
+if (iommu &&
+object_dynamic_cast((Object *)iommu, TYPE_INTEL_IOMMU_DEVICE) &&
+object_dynamic_cast((Object *)dev, "vfio-pci")) {
+intel_iommu = INTEL_IOMMU_DEVICE(iommu);
+if (!intel_iommu->caching_mode) {
+error_setg(errp, "Device assignment is not allowed without "
+ "enabling caching-mode=on for Intel IOMMU.");
+return false;
+}
+}
+
+return true;
+}
+
static void pc_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -2968,6 +2988,7 @@ static void pc_machine_class_init(ObjectClass *oc, void
*data)
pcmc->pvh_enabled = true;
assert(!mc->get_hotplug_handler);
mc->get_hotplug_handler = pc_get_hotplug_handler;
+mc->hotplug_allowed = pc_hotplug_allowed;
mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
mc->get_default_cpu_node_id = pc_get_default_cpu_node_id;
mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
--
2.21.0
[Qemu-devel] [PATCH v2 2/4] qdev/machine: Introduce hotplug_allowed hook
Introduce this new per-machine hook to give any machine class a chance
to do a sanity check on the to-be-hotplugged device as a sanity test.
This will be used for x86 to try to detect some illegal configuration
of devices, e.g., possible conflictions between vfio-pci and x86
vIOMMU.
Signed-off-by: Peter Xu
---
hw/core/qdev.c | 17 +
include/hw/boards.h| 9 +
include/hw/qdev-core.h | 1 +
qdev-monitor.c | 7 +++
4 files changed, 34 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 60d66c2f39..cbad6c1d55 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -237,6 +237,23 @@ HotplugHandler
*qdev_get_machine_hotplug_handler(DeviceState *dev)
return NULL;
}
+bool qdev_hotplug_allowed(DeviceState *dev, Error **errp)
+{
+MachineState *machine;
+MachineClass *mc;
+Object *m_obj = qdev_get_machine();
+
+if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
+machine = MACHINE(m_obj);
+mc = MACHINE_GET_CLASS(machine);
+if (mc->hotplug_allowed) {
+return mc->hotplug_allowed(machine, dev, errp);
+}
+}
+
+return true;
+}
+
HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev)
{
if (dev->parent_bus) {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2289536e48..be18a5c032 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -166,6 +166,13 @@ typedef struct {
*The function pointer to hook different machine specific functions for
*parsing "smp-opts" from QemuOpts to MachineState::CpuTopology and more
*machine specific topology fields, such as smp_dies for PCMachine.
+ * @hotplug_allowed:
+ *If the hook is provided, then it'll be called for each device
+ *hotplug to check whether the device hotplug is allowed. Return
+ *true to grant allowance or false to reject the hotplug. When
+ *false is returned, an error must be set to show the reason of
+ *the rejection. If the hook is not provided, all hotplug will be
+ *allowed.
*/
struct MachineClass {
/*< private >*/
@@ -224,6 +231,8 @@ struct MachineClass {
HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
DeviceState *dev);
+bool (*hotplug_allowed)(MachineState *state, DeviceState *dev,
+Error **errp);
CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
unsigned cpu_index);
const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index de70b7a19a..aa123f88cb 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -280,6 +280,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int
alias_id,
int required_for_version);
HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
+bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
/**
* qdev_get_hotplug_handler: Get handler responsible for device wiring
*
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 8fe5c2cad2..148df9cacf 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -615,6 +615,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
/* create device */
dev = DEVICE(object_new(driver));
+/* Check whether the hotplug is allowed by the machine */
+if (qdev_hotplug && !qdev_hotplug_allowed(dev, &err)) {
+/* Error must be set in the machine hook */
+assert(err);
+goto err_del_dev;
+}
+
if (bus) {
qdev_set_parent_bus(dev, bus);
} else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
--
2.21.0
[Qemu-devel] [PATCH v2 0/4] intel_iommu: Do sanity check of vfio-pci earlier
v2: - rebase to master [Eric] - add r-bs for Eric - remove RFC tag The VT-d code has some defects, one of them is that we cannot detect the misuse of vIOMMU and vfio-pci early enough. For example, logically this is not allowed: -device intel-iommu,caching-mode=off \ -device vfio-pci,host=05:00.0 Because the caching mode is required to make vfio-pci devices functional. Previously we did this sanity check in vtd_iommu_notify_flag_changed() as when the memory regions change their attributes. However that's too late in most cases! Because the memory region layouts will only change after IOMMU is enabled, and that's in most cases during the guest OS boots. So when the configuration is wrong, we will only bail out during the guest boots rather than simply telling the user before QEMU starts. The same problem happens on device hotplug, say, when we have this: -device intel-iommu,caching-mode=off Then we do something like: (HMP) device_add vfio-pci,host=05:00.0,bus=pcie.1 If at that time the vIOMMU is enabled in the guest then the QEMU process will simply quit directly due to this hotplug event. This is a bit insane... This series tries to solve above two problems by introducing two sanity checks upon these places separately: - machine done - hotplug device This is a bit awkward but I hope this could be better than before. There is of course other solutions like hard-code the check into vfio-pci but I feel it even more unpretty. I didn't think out any better way to do this, if there is please kindly shout out. Please have a look to see whether this would be acceptable, thanks. Peter Xu (4): intel_iommu: Sanity check vfio-pci config on machine init done qdev/machine: Introduce hotplug_allowed hook pc/q35: Disallow vfio-pci hotplug without VT-d caching mode intel_iommu: Remove the caching-mode check during flag change hw/core/qdev.c | 17 + hw/i386/intel_iommu.c | 40 ++-- hw/i386/pc.c | 21 + include/hw/boards.h| 9 + include/hw/qdev-core.h | 1 + qdev-monitor.c | 7 +++ 6 files changed, 89 insertions(+), 6 deletions(-) -- 2.21.0
[Qemu-devel] [PATCH v2 4/4] intel_iommu: Remove the caching-mode check during flag change
That's never a good place to stop QEMU process... Since now we have
both the machine done sanity check and also the hotplug handler, we
can safely remove this to avoid that.
Signed-off-by: Peter Xu
---
hw/i386/intel_iommu.c | 4
1 file changed, 4 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 17fc309b3d..070816c355 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2935,10 +2935,6 @@ static void
vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
IntelIOMMUState *s = vtd_as->iommu_state;
-if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
-vtd_panic_require_caching_mode();
-}
-
/* Update per-address-space notifier flags */
vtd_as->notifier_flags = new;
--
2.21.0
Re: [Qemu-devel] [PATCH v2] util/hbitmap: strict hbitmap_reset
Am 13.09.2019 um 20:49 hat John Snow geschrieben: > On 9/12/19 4:20 AM, Vladimir Sementsov-Ogievskiy wrote: > > Also, I'm not sure about "are" suggested by Max. "are" is for plural, but > > here I meant > > one object: sum of @start and @count. > > > > There's not great agreement universally about how to treat things like > collective nouns. Sometimes "Data" is singular, but sometimes it's > plural. "It depends." > > In this case, "start + count" refers to one sum, but two constituent > pieces, so it's functioning like a collective noun. > > We might say "a + b (together) /are/ ..." but also "the sum of a + b /is/". > > > So, you may use exactly "Sum of @start and @count is" or "(@start + @count) > > sum is" or > > just "(@start + @count) is", whichever you like more. > > > > I like using "the sum of @x and @y is" for being grammatically unambiguous. > > updated and pushed. > > (Sorry about my language again! --js) Time to revive https://patchwork.kernel.org/patch/8725621/? ;-) Kevin
Re: [Qemu-devel] [PATCH v2 0/4] intel_iommu: Do sanity check of vfio-pci earlier
On Mon, Sep 16, 2019 at 03:58:35PM +0800, Peter Xu wrote: > v2: > - rebase to master [Eric] > - add r-bs for Eric > - remove RFC tag No... this is the new cover letter with the old commits... I'll post again. Sorry for the noise. -- Peter Xu
[Qemu-devel] [PATCH v3 0/4] intel_iommu: Do sanity check of vfio-pci earlier
v3: - repost with the correct tree v2: - rebase to master [Eric] - add r-bs for Eric - remove RFC tag The VT-d code has some defects, one of them is that we cannot detect the misuse of vIOMMU and vfio-pci early enough. For example, logically this is not allowed: -device intel-iommu,caching-mode=off \ -device vfio-pci,host=05:00.0 Because the caching mode is required to make vfio-pci devices functional. Previously we did this sanity check in vtd_iommu_notify_flag_changed() as when the memory regions change their attributes. However that's too late in most cases! Because the memory region layouts will only change after IOMMU is enabled, and that's in most cases during the guest OS boots. So when the configuration is wrong, we will only bail out during the guest boots rather than simply telling the user before QEMU starts. The same problem happens on device hotplug, say, when we have this: -device intel-iommu,caching-mode=off Then we do something like: (HMP) device_add vfio-pci,host=05:00.0,bus=pcie.1 If at that time the vIOMMU is enabled in the guest then the QEMU process will simply quit directly due to this hotplug event. This is a bit insane... This series tries to solve above two problems by introducing two sanity checks upon these places separately: - machine done - hotplug device This is a bit awkward but I hope this could be better than before. There is of course other solutions like hard-code the check into vfio-pci but I feel it even more unpretty. I didn't think out any better way to do this, if there is please kindly shout out. Please have a look to see whether this would be acceptable, thanks. Peter Xu (4): intel_iommu: Sanity check vfio-pci config on machine init done qdev/machine: Introduce hotplug_allowed hook pc/q35: Disallow vfio-pci hotplug without VT-d caching mode intel_iommu: Remove the caching-mode check during flag change hw/core/qdev.c | 17 + hw/i386/intel_iommu.c | 41 +++-- hw/i386/pc.c | 21 + include/hw/boards.h| 9 + include/hw/qdev-core.h | 1 + qdev-monitor.c | 7 +++ 6 files changed, 90 insertions(+), 6 deletions(-) -- 2.21.0
[Qemu-devel] [PATCH v3 1/4] intel_iommu: Sanity check vfio-pci config on machine init done
This check was previously only happened when the IOMMU is enabled in
the guest. It was always too late because the enabling of IOMMU
normally only happens during the boot of guest OS. It means that we
can bail out and exit directly during the guest OS boots if the
configuration of devices are not supported. Or, if the guest didn't
enable vIOMMU at all, then the user can use the guest normally but as
long as it reconfigure the guest OS to enable the vIOMMU then reboot,
the user will see the panic right after the reset when the next boot
starts.
Let's make this failure even earlier so that we force the user to use
caching-mode for vfio-pci devices when with the vIOMMU. So the user
won't get surprise at least during execution of the guest, which seems
a bit nicer.
This will affect some user who didn't enable vIOMMU in the guest OS
but was using vfio-pci and the vtd device in the past. However I hope
it's not a majority because not enabling vIOMMU with the device
attached is actually meaningless.
We still keep the old assertion for safety so far because the hotplug
path could still reach it, so far.
Reviewed-by: Eric Auger
Signed-off-by: Peter Xu
---
hw/i386/intel_iommu.c | 39 ---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 75ca6f9c70..bed8ffe446 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "hw/i386/x86-iommu.h"
#include "hw/pci-host/q35.h"
#include "sysemu/kvm.h"
+#include "sysemu/sysemu.h"
#include "hw/i386/apic_internal.h"
#include "kvm_i386.h"
#include "migration/vmstate.h"
@@ -64,6 +65,13 @@
static void vtd_address_space_refresh_all(IntelIOMMUState *s);
static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
+static void vtd_panic_require_caching_mode(void)
+{
+error_report("We need to set caching-mode=on for intel-iommu to enable "
+ "device assignment with IOMMU protection.");
+exit(1);
+}
+
static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
uint64_t wmask, uint64_t w1cmask)
{
@@ -2929,9 +2937,7 @@ static void
vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
IntelIOMMUState *s = vtd_as->iommu_state;
if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
-error_report("We need to set caching-mode=on for intel-iommu to enable
"
- "device assignment with IOMMU protection.");
-exit(1);
+vtd_panic_require_caching_mode();
}
/* Update per-address-space notifier flags */
@@ -3699,6 +3705,32 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error
**errp)
return true;
}
+static int vtd_machine_done_notify_one(Object *child, void *unused)
+{
+IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
+
+/*
+ * We hard-coded here because vfio-pci is the only special case
+ * here. Let's be more elegant in the future when we can, but so
+ * far there seems to be no better way.
+ */
+if (object_dynamic_cast(child, "vfio-pci") && !iommu->caching_mode) {
+vtd_panic_require_caching_mode();
+}
+
+return 0;
+}
+
+static void vtd_machine_done_hook(Notifier *notifier, void *unused)
+{
+object_child_foreach_recursive(object_get_root(),
+ vtd_machine_done_notify_one, NULL);
+}
+
+static Notifier vtd_machine_done_notify = {
+.notify = vtd_machine_done_hook,
+};
+
static void vtd_realize(DeviceState *dev, Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
@@ -3744,6 +3776,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
/* Pseudo address space under root PCI bus. */
pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
+qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
}
static void vtd_class_init(ObjectClass *klass, void *data)
--
2.21.0
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] trace: Forbid trailing newline in event format
Am 13.09.2019 um 12:52 hat Philippe Mathieu-Daudé geschrieben: > Hi Stefan, > > I'v been confused by trailing newline in trace reports, > so this series aims to fix this, by cleaning current > formats and add a check to catch new one introduced. Good idea. Reviewed-by: Kevin Wolf
[Qemu-devel] [PATCH v3 2/4] qdev/machine: Introduce hotplug_allowed hook
Introduce this new per-machine hook to give any machine class a chance
to do a sanity check on the to-be-hotplugged device as a sanity test.
This will be used for x86 to try to detect some illegal configuration
of devices, e.g., possible conflictions between vfio-pci and x86
vIOMMU.
Reviewed-by: Eric Auger
Signed-off-by: Peter Xu
---
hw/core/qdev.c | 17 +
include/hw/boards.h| 9 +
include/hw/qdev-core.h | 1 +
qdev-monitor.c | 7 +++
4 files changed, 34 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 60d66c2f39..cbad6c1d55 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -237,6 +237,23 @@ HotplugHandler
*qdev_get_machine_hotplug_handler(DeviceState *dev)
return NULL;
}
+bool qdev_hotplug_allowed(DeviceState *dev, Error **errp)
+{
+MachineState *machine;
+MachineClass *mc;
+Object *m_obj = qdev_get_machine();
+
+if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
+machine = MACHINE(m_obj);
+mc = MACHINE_GET_CLASS(machine);
+if (mc->hotplug_allowed) {
+return mc->hotplug_allowed(machine, dev, errp);
+}
+}
+
+return true;
+}
+
HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev)
{
if (dev->parent_bus) {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2289536e48..be18a5c032 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -166,6 +166,13 @@ typedef struct {
*The function pointer to hook different machine specific functions for
*parsing "smp-opts" from QemuOpts to MachineState::CpuTopology and more
*machine specific topology fields, such as smp_dies for PCMachine.
+ * @hotplug_allowed:
+ *If the hook is provided, then it'll be called for each device
+ *hotplug to check whether the device hotplug is allowed. Return
+ *true to grant allowance or false to reject the hotplug. When
+ *false is returned, an error must be set to show the reason of
+ *the rejection. If the hook is not provided, all hotplug will be
+ *allowed.
*/
struct MachineClass {
/*< private >*/
@@ -224,6 +231,8 @@ struct MachineClass {
HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
DeviceState *dev);
+bool (*hotplug_allowed)(MachineState *state, DeviceState *dev,
+Error **errp);
CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
unsigned cpu_index);
const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index de70b7a19a..aa123f88cb 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -280,6 +280,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int
alias_id,
int required_for_version);
HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
+bool qdev_hotplug_allowed(DeviceState *dev, Error **errp);
/**
* qdev_get_hotplug_handler: Get handler responsible for device wiring
*
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 8fe5c2cad2..148df9cacf 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -615,6 +615,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
/* create device */
dev = DEVICE(object_new(driver));
+/* Check whether the hotplug is allowed by the machine */
+if (qdev_hotplug && !qdev_hotplug_allowed(dev, &err)) {
+/* Error must be set in the machine hook */
+assert(err);
+goto err_del_dev;
+}
+
if (bus) {
qdev_set_parent_bus(dev, bus);
} else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
--
2.21.0
[Qemu-devel] [PATCH v3 4/4] intel_iommu: Remove the caching-mode check during flag change
That's never a good place to stop QEMU process... Since now we have
both the machine done sanity check and also the hotplug handler, we
can safely remove this to avoid that.
Reviewed-by: Eric Auger
Signed-off-by: Peter Xu
---
hw/i386/intel_iommu.c | 4
1 file changed, 4 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index bed8ffe446..f1de8fdb75 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2936,10 +2936,6 @@ static void
vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
IntelIOMMUState *s = vtd_as->iommu_state;
-if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
-vtd_panic_require_caching_mode();
-}
-
/* Update per-address-space notifier flags */
vtd_as->notifier_flags = new;
--
2.21.0
Re: [Qemu-devel] [PATCH v2] virtio-blk: schedule virtio_notify_config to run on main context
Am 13.09.2019 um 12:56 hat Sergio Lopez geschrieben:
> virtio_notify_config() needs to acquire the global mutex, which isn't
> allowed from an iothread, and may lead to a deadlock like this:
>
> - main thead
> * Has acquired: qemu_global_mutex.
> * Is trying the acquire: iothread AioContext lock via
> AIO_WAIT_WHILE (after aio_poll).
>
> - iothread
> * Has acquired: AioContext lock.
> * Is trying to acquire: qemu_global_mutex (via
> virtio_notify_config->prepare_mmio_access).
>
> If virtio_blk_resize() is called from an iothread, schedule
> virtio_notify_config() to be run in the main context BH.
>
> Signed-off-by: Sergio Lopez
> ---
> Changelog
>
> v2:
> - Use aio_bh_schedule_oneshot instead of scheduling a coroutine
>(thanks Kevin Wolf).
> - Switch from RFC to v2 patch.
> ---
> hw/block/virtio-blk.c | 21 -
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 18851601cb..669dc60f5b 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -16,6 +16,7 @@
> #include "qemu/iov.h"
> #include "qemu/module.h"
> #include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> #include "trace.h"
> #include "hw/block/block.h"
> #include "hw/qdev-properties.h"
> @@ -1086,11 +1087,29 @@ static int virtio_blk_load_device(VirtIODevice *vdev,
> QEMUFile *f,
> return 0;
> }
>
> +static void virtio_resize_cb(void *opaque)
> +{
> +VirtIODevice *vdev = opaque;
> +
> +assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> +virtio_notify_config(vdev);
> +}
> +
> static void virtio_blk_resize(void *opaque)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
>
> -virtio_notify_config(vdev);
> +if (qemu_get_current_aio_context() != qemu_get_aio_context()) {
> +/*
> + * virtio_notify_config() needs to acquire the global mutex,
> + * so it can't be called from an iothread. Instead, schedule
> + * it to be run in the main context BH.
> + */
> +aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +virtio_resize_cb, vdev);
> +} else {
> +virtio_notify_config(vdev);
Let's call virtio_resize_cb() instead to keep both code paths the same.
Otherwise, we might add more code to virtio_resize_cb() later and miss
that it must be duplicated here.
Kevin
[Qemu-devel] [PATCH v3 3/4] pc/q35: Disallow vfio-pci hotplug without VT-d caching mode
Instead of bailing out when trying to hotplug a vfio-pci device with
below configuration:
-device intel-iommu,caching-mode=off
With this we can return a warning message to the user via QMP/HMP and
the VM will continue to work after failing the hotplug:
(qemu) device_add vfio-pci,bus=root.3,host=05:00.0,id=vfio1
Error: Device assignment is not allowed without enabling caching-mode=on for
Intel IOMMU.
Reviewed-by: Eric Auger
Signed-off-by: Peter Xu
---
hw/i386/pc.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bad866fe44..0a6fa6e549 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2944,6 +2944,26 @@ static void x86_nmi(NMIState *n, int cpu_index, Error
**errp)
}
}
+
+static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error
**errp)
+{
+X86IOMMUState *iommu = x86_iommu_get_default();
+IntelIOMMUState *intel_iommu;
+
+if (iommu &&
+object_dynamic_cast((Object *)iommu, TYPE_INTEL_IOMMU_DEVICE) &&
+object_dynamic_cast((Object *)dev, "vfio-pci")) {
+intel_iommu = INTEL_IOMMU_DEVICE(iommu);
+if (!intel_iommu->caching_mode) {
+error_setg(errp, "Device assignment is not allowed without "
+ "enabling caching-mode=on for Intel IOMMU.");
+return false;
+}
+}
+
+return true;
+}
+
static void pc_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -2968,6 +2988,7 @@ static void pc_machine_class_init(ObjectClass *oc, void
*data)
pcmc->pvh_enabled = true;
assert(!mc->get_hotplug_handler);
mc->get_hotplug_handler = pc_get_hotplug_handler;
+mc->hotplug_allowed = pc_hotplug_allowed;
mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
mc->get_default_cpu_node_id = pc_get_default_cpu_node_id;
mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
--
2.21.0
Re: [Qemu-devel] [Qemu-trivial] [PATCH] configure: Add xkbcommon configure options
Le 14/09/2019 à 16:51, James Le Cuirot a écrit : > This dependency is currently "automagic", which is bad for distributions. > > Signed-off-by: James Le Cuirot > --- > configure | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/configure b/configure > index 30aad233d1..30544f52e6 100755 > --- a/configure > +++ b/configure > @@ -1521,6 +1521,10 @@ for opt do >;; >--disable-libpmem) libpmem=no >;; > + --enable-xkbcommon) xkbcommon=yes > + ;; > + --disable-xkbcommon) xkbcommon=no > + ;; >*) >echo "ERROR: unknown option $opt" >echo "Try '$0 --help' for more information" > @@ -1804,6 +1808,7 @@ disabled with --disable-FEATURE, default is enabled if > available: >capstonecapstone disassembler support >debug-mutex mutex debugging support >libpmem libpmem support > + xkbcommon xkbcommon support > > NOTE: The object files are built at the place where configure is launched > EOF > Reviewed-by: Laurent Vivier cc: Gerd Hoffmann
Re: [Qemu-devel] [PATCH v2 2/3] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap
14.09.2019 1:01, John Snow wrote:
>
>
> On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It's more comfortable to not deal with local_err.
>>
>
> I agree.
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/qcow2.h| 5 ++---
>> include/block/block_int.h| 6 +++---
>> include/block/dirty-bitmap.h | 5 ++---
>> block/dirty-bitmap.c | 9 +
>> block/qcow2-bitmap.c | 20 +++-
>> blockdev.c | 7 +++
>> 6 files changed, 26 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 998bcdaef1..99ee88f802 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState
>> *bs,
>> const char *name,
>> uint32_t granularity,
>> Error **errp);
>> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp);
>> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char
>> *name,
>> + Error **errp);
>>
>> ssize_t coroutine_fn
>> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 0422acdf1c..503ac9e3cd 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -556,9 +556,9 @@ struct BlockDriver {
>> const char *name,
>> uint32_t granularity,
>> Error **errp);
>> -void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> -const char *name,
>> -Error **errp);
>> +int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> + const char *name,
>> + Error **errp);
>>
>> /**
>>* Register/unregister a buffer for I/O. For example, when the driver
>> is
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 4b4b731b46..07503b03b5 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap,
>> uint32_t flags,
>> Error **errp);
>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
>> *bitmap);
>> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp);
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char
>> *name,
>> +Error **errp);
>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>> void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>> void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 8f42015db9..a52b83b619 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState
>> *bs)
>>* not fail.
>>* This function doesn't release corresponding BdrvDirtyBitmap.
>>*/
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp)
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char
>> *name,
>> +Error **errp)
>> {
>> if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
>> -bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>> +return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>> }
>> +
>
> But is it a problem if we return an error code without setting errp now?
> If this is for the sake of not having to deal with local_err, we should
> make sure that a non-zero return means that errp is set. Right?
Oops, of course, I should set errp here
>
>> +return -ENOTSUP;
>> }
>>
>> bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char
>> *name,
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index b2487101ed..1aaedb3b55 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1404,11 +1404,10 @@ static Qcow2Bitmap
>> *find_bitmap_by_name(Qcow2BitmapList *bm_list,
>> return NULL;
>> }
Re: [Qemu-devel] vhost, iova, and dirty page tracking
On 2019/9/16 上午9:51, Tian, Kevin wrote: Hi, Jason We had a discussion about dirty page tracking in VFIO, when vIOMMU is enabled: https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg02690.html It's actually a similar model as vhost - Qemu cannot interpose the fast-path DMAs thus relies on the kernel part to track and report dirty page information. Currently Qemu tracks dirty pages in GFN level, thus demanding a translation from IOVA to GPA. Then the open in our discussion is where this translation should happen. Doing the translation in kernel implies a device iotlb flavor, which is what vhost implements today. It requires potentially large tracking structures in the host kernel, but leveraging the existing log_sync flow in Qemu. On the other hand, Qemu may perform log_sync for every removal of IOVA mapping and then do the translation itself, then avoiding the GPA awareness in the kernel side. It needs some change to current Qemu log-sync flow, and may bring more overhead if IOVA is frequently unmapped. So we'd like to hear about your opinions, especially about how you came down to the current iotlb approach for vhost. We don't consider too much in the point when introducing vhost. And before IOTLB, vhost has already know GPA through its mem table (GPA->HVA). So it's nature and easier to track dirty pages at GPA level then it won't any changes in the existing ABI. For VFIO case, the only advantages of using GPA is that the log can then be shared among all the devices that belongs to the VM. Otherwise syncing through IOVA is cleaner. Thanks p.s. Alex's comment is also copied here from original thread. So vhost must then be configuring a listener across system memory rather than only against the device AddressSpace like we do in vfio, such that it get's log_sync() callbacks for the actual GPA space rather than only the IOVA space. OTOH, QEMU could understand that the device AddressSpace has a translate function and apply the IOVA dirty bits to the system memory AddressSpace. Wouldn't it make more sense for QEMU to perform a log_sync() prior to removing a MemoryRegionSection within an AddressSpace and update the GPA rather than pushing GPA awareness and potentially large tracking structures into the host kernel? Thanks Kevin
Re: [Qemu-devel] [PATCH v2 6/6] .travis.yml: Split enterprise vs. hobbyist acceptance test job
Philippe Mathieu-Daudé writes:
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> .travis.yml | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 69a37f7387..753276eb33 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -265,9 +265,23 @@ matrix:
> - "3.6"
>
>
> -# Acceptance (Functional) tests
> +# Acceptance (Functional) tests [enterprise]
> - env:
> -- CONFIG="--python=/usr/bin/python3
> --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu"
> +- CONFIG="--python=/usr/bin/python3
> --target-list=x86_64-softmmu,mips64el-softmmu,aarch64-softmmu,s390x-softmmu,ppc64-softmmu"
You could use:
--target-list=${MAIN_SOFTMMU_TARGETS}
and
--disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}
> +- TEST_CMD="make check-acceptance"
> + after_failure:
> +- cat tests/results/latest/job.log
> + addons:
> +apt:
> + packages:
> +- python3-pil
> +- python3-pip
> +- python3.5-venv
> +
> +
> +# Acceptance (Functional) tests [hobbyist]
> +- env:
> +- CONFIG="--python=/usr/bin/python3
> --target-list=mips-softmmu,arm-softmmu,alpha-softmmu,ppc-softmmu,m68k-softmmu"
> - TEST_CMD="make check-acceptance"
>after_failure:
> - cat tests/results/latest/job.log
--
Alex Bennée
Re: [Qemu-devel] [PATCH v3 3/6] vmstate: replace DeviceState with VMStateIf
* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> Replace DeviceState dependency with VMStateIf on vmstate API.
>
> Signed-off-by: Marc-André Lureau
> ---
> docs/devel/migration.rst | 2 +-
> hw/block/onenand.c | 2 +-
> hw/core/qdev.c | 7 ---
> hw/ide/cmd646.c | 2 +-
> hw/ide/isa.c | 2 +-
> hw/ide/piix.c| 2 +-
> hw/ide/via.c | 2 +-
> hw/misc/max111x.c| 2 +-
> hw/net/eepro100.c| 4 ++--
> hw/net/vmxnet3.c | 2 +-
> hw/nvram/eeprom93xx.c| 4 ++--
> hw/ppc/spapr_drc.c | 9 +
> hw/ppc/spapr_iommu.c | 4 ++--
> hw/s390x/s390-skeys.c| 2 +-
> include/migration/register.h | 4 ++--
> include/migration/vmstate.h | 8
> migration/savevm.c | 26 +-
> stubs/vmstate.c | 4 ++--
> 18 files changed, 45 insertions(+), 43 deletions(-)
>
> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> index f7668ae389..c9932194d9 100644
> --- a/docs/devel/migration.rst
> +++ b/docs/devel/migration.rst
> @@ -183,7 +183,7 @@ another to load the state back.
>
> .. code:: c
>
> - int register_savevm_live(DeviceState *dev,
> + int register_savevm_live(VMStateIf *obj,
> const char *idstr,
> int instance_id,
> int version_id,
> diff --git a/hw/block/onenand.c b/hw/block/onenand.c
> index fcc5a69b90..9c233c12e4 100644
> --- a/hw/block/onenand.c
> +++ b/hw/block/onenand.c
> @@ -822,7 +822,7 @@ static void onenand_realize(DeviceState *dev, Error
> **errp)
> onenand_mem_setup(s);
> sysbus_init_irq(sbd, &s->intr);
> sysbus_init_mmio(sbd, &s->container);
> -vmstate_register(dev,
> +vmstate_register(VMSTATE_IF(dev),
> ((s->shift & 0x7f) << 24)
> | ((s->id.man & 0xff) << 16)
> | ((s->id.dev & 0xff) << 8)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 7e083dfcae..ad18c0383d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -849,7 +849,8 @@ static void device_set_realized(Object *obj, bool value,
> Error **errp)
> dev->canonical_path = object_get_canonical_path(OBJECT(dev));
>
> if (qdev_get_vmsd(dev)) {
> -if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev),
> dev,
> +if (vmstate_register_with_alias_id(VMSTATE_IF(dev),
> + -1, qdev_get_vmsd(dev), dev,
> dev->instance_id_alias,
>
> dev->alias_required_for_version,
> &local_err) < 0) {
> @@ -884,7 +885,7 @@ static void device_set_realized(Object *obj, bool value,
> Error **errp)
> local_errp);
> }
> if (qdev_get_vmsd(dev)) {
> -vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
> +vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
> }
> if (dc->unrealize) {
> local_errp = local_err ? NULL : &local_err;
> @@ -908,7 +909,7 @@ child_realize_fail:
> }
>
> if (qdev_get_vmsd(dev)) {
> -vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
> +vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
> }
>
> post_realize_fail:
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index f3ccd11c79..01a982acbc 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -301,7 +301,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error
> **errp)
> ide_register_restart_cb(&d->bus[i]);
> }
>
> -vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
> +vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
> qemu_register_reset(cmd646_reset, d);
> }
>
> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> index 7b6e283679..9c7f88b2d5 100644
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -75,7 +75,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error
> **errp)
> ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
> isa_init_irq(isadev, &s->irq, s->isairq);
> ide_init2(&s->bus, s->irq);
> -vmstate_register(dev, 0, &vmstate_ide_isa, s);
> +vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
> ide_register_restart_cb(&s->bus);
> }
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index fba6bc8bff..cbaee9e2cc 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -159,7 +159,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
> **errp)
> bmdma_setup_bar(d);
> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>
> -vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
> +vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>
Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
gentle reminder
On Thu, Aug 29, 2019 at 06:03:21PM +0200, Tomáš Golembiovský wrote:
> Add command for reporting devices on Windows guest. The intent is not so
> much to report the devices but more importantly the driver (and its
> version) that is assigned to the device.
>
> Signed-off-by: Tomáš Golembiovský
> ---
> qga/commands-posix.c | 11 +++
> qga/commands-win32.c | 195 ++-
> qga/qapi-schema.json | 32 +++
> 3 files changed, 237 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index dfc05f5b8a..9adf8bb520 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2709,6 +2709,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>
> return 0;
> }
> +
> #endif /* CONFIG_FSFREEZE */
>
> #if !defined(CONFIG_FSTRIM)
> @@ -2757,6 +2758,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
> blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
> #endif
>
> +blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
> +
> return blacklist;
> }
>
> @@ -2977,3 +2980,11 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
> return info;
> }
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +error_setg(errp, QERR_UNSUPPORTED);
> +
> +return NULL;
> +}
> +
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..0bb93422c7 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -21,10 +21,11 @@
> #ifdef CONFIG_QGA_NTDDSCSI
> #include
> #include
> +#endif
> #include
> #include
> #include
> -#endif
> +#include
> #include
> #include
> #include
> @@ -38,6 +39,36 @@
> #include "qemu/host-utils.h"
> #include "qemu/base64.h"
>
> +
> +/* The following should be in devpkey.h, but it isn't */
> +DEFINE_DEVPROPKEY(DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1, 0x02,
> +0x60, 0x8c, 0x9e, 0xeb, 0xac, 10); /* DEVPROP_TYPE_STRING */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd,
> 0x80,
> +0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> +/* DEVPROP_TYPE_STRING_LIST */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094,
> 0xad,
> +0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2); /* DEVPROP_TYPE_FILETIME */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d, 0x4094,
> +0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
> +/* DEVPROP_TYPE_STRING */
> +/* The following should be in sal.h, but it isn't */
> +#ifndef _Out_writes_bytes_opt_
> +#define _Out_writes_bytes_opt_(s)
> +#endif
> +/* The following shoud be in cfgmgr32.h, but it isn't */
> +#ifndef CM_Get_DevNode_Property
> +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> +_In_ DEVINST dnDevInst,
> +_In_ CONST DEVPROPKEY* PropertyKey,
> +_Out_ DEVPROPTYPE * PropertyType,
> +_Out_writes_bytes_opt_(*PropertyBufferSize) PBYTE PropertyBuffer,
> +_Inout_ PULONG PropertyBufferSize,
> +_In_ ULONG ulFlags
> +);
> +#define CM_Get_DevNode_Property CM_Get_DevNode_PropertyW
> +#endif
> +
> +
> #ifndef SHTDN_REASON_FLAG_PLANNED
> #define SHTDN_REASON_FLAG_PLANNED 0x8000
> #endif
> @@ -2234,3 +2265,165 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
> return info;
> }
> +
> +/*
> + * Safely get device property. Returned strings are using wide characters.
> + * Caller is responsible for freeing the buffer.
> + */
> +static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
> +PDEVPROPTYPE propType)
> +{
> +CONFIGRET cr;
> +LPBYTE buffer = NULL;
> +ULONG bufferLen = 0;
> +
> +/* First query for needed space */
> +cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +buffer, &bufferLen, 0);
> +if ((cr != CR_SUCCESS) && (cr != CR_BUFFER_SMALL)) {
> +
> +g_debug(
> +"failed to get size of device property, device error code=%lx",
> +cr);
> +return NULL;
> +}
> +buffer = (LPBYTE)g_malloc(bufferLen);
> +cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +buffer, &bufferLen, 0);
> +if (cr != CR_SUCCESS) {
> +g_free(buffer);
> +g_debug(
> +"failed to get device property, device error code=%lx", cr);
> +return NULL;
> +}
> +return buffer;
> +}
> +
> +/*
> + *
> https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
> + */
> +#define DEVICE_PCI_RE "PCIVEN_(1AF4|1B36)&DEV_([0-9A-B]{4})(&|$)"
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +GuestDeviceInfoList *head = NULL, *cur_item = NULL, *item = NULL;
> +HDEVINFO dev_info = INVALID_HANDLE_VALUE;
> +SP_DEVINFO_DATA dev_info_data;
> +int i;
> +GError *gerr = NULL;
> +GRegex *device_pci_re = NULL;
> +
> +device_
[Qemu-devel] [PATCH v1 1/1] riscv: pmp: Allow valid instruction fetches at the start of a PMP range
Allow Qemu guest code to execute from the very start of a PMP range without
faulting.
When an instruction is fetched from the first word of a PMP range,
pmp_hart_has_privs()
is called with a size of zero. This causes pmp_is_in_range() to be called with
an address
lower than addr, when obtaining a value for e, and a fault is incorrectly
generated.
This fault was observed by creating a PMP range with RWX access, filling the
range with
valid code from its base address, and then jumping to the first instruction at
the base address.
Qemu generates an instruction access fault: the correct behavior is to allow
the instruction fetch.
This patch checks a size is non-zero before applying a calculation operation
that would bring it
below addr, and thus erroneously raise a fault.
Signed-off-by: Chris Williams mailto:diodes...@tuta.io>>
---
target/riscv/pmp.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 958c7502a0..06f2cd52f0 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -242,10 +242,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
addr,
}
/* 1.10 draft priv spec states there is an implicit order
- from low to high */
+ from low to high. Also, catch attempts to check a request
+ size of zero, and ensure it does not accidentally fail by
+ checking for an address *below* addr */
for (i = 0; i < MAX_RISCV_PMPS; i++) {
s = pmp_is_in_range(env, i, addr);
- e = pmp_is_in_range(env, i, addr + size - 1);
+ e = pmp_is_in_range(env, i, (size == 0) ? addr : addr + size - 1);
/* partially inside */
if ((s + e) == 1) {
--
2.20.1
Re: [Qemu-devel] [PULL 0/4] MIPS queue for September 12th, 2019
On Thu, 12 Sep 2019 at 17:30, Aleksandar Markovic wrote: > > From: Aleksandar Markovic > > The following changes since commit 6d2fdde42c3344099262431df6a3f429c509291d: > > Merge remote-tracking branch > 'remotes/stsquad/tags/pull-testing-next-100919-2' into staging (2019-09-10 > 14:52:09 +0100) > > are available in the git repository at: > > https://github.com/AMarkovic/qemu tags/mips-queue-sep-12-2019 > > for you to fetch changes up to d1cc1533509012916dceeb7f23accda8a9fee85c: > > target/mips: gdbstub: Revert commit 8e0b373 (2019-09-12 18:25:34 +0200) > > > > MIPS queue for September 12th, 2019 > > Highlights: > > - switch to using do_transaction_failed() hook for MIPS' Jazz boards > - revert a commit that caused problems with gdb's packet 'g' for MIPS Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2 for any user-visible changes. -- PMM
Re: [Qemu-devel] [PATCH v7 2/3] block/qcow2: refactor encryption code
15.09.2019 23:36, Maxim Levitsky wrote:
> * Change the qcow2_co_{encrypt|decrypt} to just receive full host and
>guest offsets and use this function directly instead of calling
>do_perform_cow_encrypt (which is removed by that patch).
>
> * Adjust qcow2_co_encdec to take full host and guest offsets as well.
>
> * Document the qcow2_co_{encrypt|decrypt} arguments
>to prevent the bug fixed in former commit from hopefully
>happening again.
>
> Signed-off-by: Maxim Levitsky
> ---
> block/qcow2-cluster.c | 41 ++--
> block/qcow2-threads.c | 63 +--
> block/qcow2.c | 5 ++--
> block/qcow2.h | 8 +++---
> 4 files changed, 70 insertions(+), 47 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index bfeb0241d7..a2d4909024 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
[..]
> static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
>uint64_t cluster_offset,
>unsigned offset_in_cluster,
> @@ -891,11 +869,20 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta
> *m)
>
> /* Encrypt the data if necessary before writing it */
> if (bs->encrypted) {
> -if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -start->offset, start_buffer,
> -start->nb_bytes) ||
> -!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -end->offset, end_buffer, end->nb_bytes))
> {
> +ret = qcow2_co_encrypt(bs,
> + m->alloc_offset + start->offset,
> + m->offset + start->offset,
> + start_buffer, start->nb_bytes);
> +if (ret < 0) {
> +ret = -EIO;
> +goto fail;
Just go to fail I think, don't reassign ret variable.
> +}
> +
> +ret = qcow2_co_encrypt(bs,
> + m->alloc_offset + end->offset,
> + m->offset + end->offset,
> + end_buffer, end->nb_bytes);
> +if (ret < 0) {
> ret = -EIO;
and here.
with these two places fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy
(I think, these simple changes may be applied while queuing, so you don't
need to resend, if there no other reasons)
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v2 6/6] .travis.yml: Split enterprise vs. hobbyist acceptance test job
On 9/16/19 10:43 AM, Alex Bennée wrote:
> Philippe Mathieu-Daudé writes:
>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> .travis.yml | 18 --
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 69a37f7387..753276eb33 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -265,9 +265,23 @@ matrix:
>> - "3.6"
>>
>>
>> -# Acceptance (Functional) tests
>> +# Acceptance (Functional) tests [enterprise]
>> - env:
>> -- CONFIG="--python=/usr/bin/python3
>> --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu"
>> +- CONFIG="--python=/usr/bin/python3
>> --target-list=x86_64-softmmu,mips64el-softmmu,aarch64-softmmu,s390x-softmmu,ppc64-softmmu"
>
> You could use:
>
> --target-list=${MAIN_SOFTMMU_TARGETS}
>
> and
>
> --disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}
I like the idea, but this variable is slighly different:
MAIN_SOFTMMU_TARGETS="aarch64-softmmu,arm-softmmu,i386-softmmu,mips-softmmu,mips64-softmmu,ppc64-softmmu,riscv64-softmmu,s390x-softmmu,x86_64-softmmu"
Are you suggesting we use MAIN_SOFTMMU_TARGETS for 'Enterprise' targets?
As long as mips64-softmmu is present, I think we can remove mips-softmmu
from MAIN_TARGETS. Maybe i386-softmmu can be removed too.
That would give the following hobbyist list:
i386-softmmu,mips-softmmu,alpha-softmmu,ppc-softmmu,m68k-softmmu
(I plan to add hppa-softmmu there too).
>> +- TEST_CMD="make check-acceptance"
>> + after_failure:
>> +- cat tests/results/latest/job.log
>> + addons:
>> +apt:
>> + packages:
>> +- python3-pil
>> +- python3-pip
>> +- python3.5-venv
>> +
>> +
>> +# Acceptance (Functional) tests [hobbyist]
>> +- env:
>> +- CONFIG="--python=/usr/bin/python3
>> --target-list=mips-softmmu,arm-softmmu,alpha-softmmu,ppc-softmmu,m68k-softmmu"
>> - TEST_CMD="make check-acceptance"
>>after_failure:
>> - cat tests/results/latest/job.log
>
>
> --
> Alex Bennée
>
Re: [Qemu-devel] [PATCH v2 0/6] tests/acceptance: Add tests for the PReP/40p machine
Hi David, On 9/16/19 2:42 AM, David Gibson wrote: > On Sun, Sep 15, 2019 at 11:19:34PM +0200, Philippe Mathieu-Daudé wrote: >> Quick tests worth to avoid regressions with the 40p machine. >> idea from the "Maintainers, please tell us how to boot your machines" >> thread: >> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg04177.html >> >> v2: Split Travis job, added Hervé R-b tag >> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05896.html >> >> Regards, >> >> Phil. > > I'm guessing you're expecting these to go in via the testing tree, in > which case > > Acked-by: David Gibson Thanks, appreciated :) > Or do you want me to take them via the ppc tree? I think the 'testing tree' should focus on the CI/testing infrastructure, while each subsystem maintainers should care about the tests covering their subsystem (the testing tree maintainers might not have the required knowledge to be sure a test is correctly implemented). In this particular case I assume you don't have much knowledge of that PPC machine, which is a hobbyist one, but since you are the PPC maintainer, I'd rather see this going via your tree :) Alex/Cleber/Eduardo, any comment on this position? Thanks, Phil. >> Philippe Mathieu-Daudé (6): >> tests/acceptance: Add test that runs NetBSD 4.0 installer on PRep/40p >> tests/acceptance: Test Open Firmware on the PReP/40p >> tests/acceptance: Test OpenBIOS on the PReP/40p >> tests/acceptance: Test Sandalfoot initrd on the PReP/40p >> .travis.yml: Let the avocado job run the 40p tests >> .travis.yml: Split enterprise vs. hobbyist acceptance test job >> >> .travis.yml | 18 +++- >> MAINTAINERS | 1 + >> tests/acceptance/ppc_prep_40p.py | 150 +++ >> 3 files changed, 167 insertions(+), 2 deletions(-) >> create mode 100644 tests/acceptance/ppc_prep_40p.py >> >
[Qemu-devel] [PATCH 0/3] Add acceptance test for migration
Add new test for migration that bringup vm with different machine types and migrate it, introduce new API in avocado_qemu to query all the machine types supported by qemu. Test run: # avocado run migration.py JOB ID : ef54f57a073eb267d2347e32225f2adbe27969de JOB LOG: /home/bala/avocado-fvt-wrapper/results/job-2019-08-05T13.54-ef54f57/job.log (1/2) migration.py:Migration.test_migration_with_tcp_localhost: PASS (0.54 s) (2/2) migration.py:Migration.test_migration_with_machine_types: PASS (5.21 s) RESULTS: PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0 JOB TIME : 5.86 s Currently acceptance test for migration error out as we check `query-migrate` in target after migration which is not appropriate. Balamuruhan S (3): tests/acceptance/migration: fix post migration check tests/acceptance/avocado_qemu: add method to get supported machine types tests/acceptance/migration: test to migrate will all machine types tests/acceptance/avocado_qemu/__init__.py | 6 ++ tests/acceptance/migration.py | 27 ++- 2 files changed, 32 insertions(+), 1 deletion(-) -- 2.14.5
[Qemu-devel] [PATCH 2/3] tests/acceptance/avocado_qemu: add method to get supported machine types
add `get_machine_types()` to return list of supported machine types
by the qemu binary.
Signed-off-by: Balamuruhan S
---
tests/acceptance/avocado_qemu/__init__.py | 6 ++
1 file changed, 6 insertions(+)
diff --git a/tests/acceptance/avocado_qemu/__init__.py
b/tests/acceptance/avocado_qemu/__init__.py
index bd41e0443c..42a1b572bd 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -85,6 +85,12 @@ class Test(avocado.Test):
self._vms[name] = self._new_vm(*args)
return self._vms[name]
+def get_machine_types(self):
+cmd = "%s -machine ?" % self.qemu_bin
+output = avocado.utils.process.getoutput(cmd).split("\n")
+output.remove("Supported machines are:")
+return [each.split()[0] for each in output]
+
def tearDown(self):
for vm in self._vms.values():
vm.shutdown()
--
2.14.5
[Qemu-devel] [PATCH 3/3] tests/acceptance/migration: test to migrate will all machine types
add migration test to query machine types supported by qemu binary
and migrate vm will all supported type.
Signed-off-by: Balamuruhan S
---
tests/acceptance/migration.py | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
index 0f3553c8f0..74416ccc21 100644
--- a/tests/acceptance/migration.py
+++ b/tests/acceptance/migration.py
@@ -49,3 +49,29 @@ class Migration(Test):
self.assertEqual(dest_vm.command('query-status')['status'], 'running')
self.assertEqual(source_vm.command('query-status')['status'],
'postmigrate')
+
+
+def test_migration_with_machine_types(self):
+migration_port = self._get_free_port()
+for machine in self.get_machine_types():
+if 'pseries' in machine:
+print("migrating with machine type - {}".format(machine))
+source_vm = self.get_vm('-M', '{},cap-htm=off'.format(machine))
+dest_uri = 'tcp:localhost:%u' % migration_port
+dest_vm = self.get_vm('-M', '{},cap-htm=off'.format(machine),
+ '-incoming', dest_uri)
+dest_vm.launch()
+source_vm.launch()
+source_vm.qmp('migrate', uri=dest_uri)
+wait.wait_for(
+self.migration_finished,
+timeout=self.timeout,
+step=0.1,
+args=(source_vm,)
+)
+self.assertEqual(source_vm.command('query-migrate')['status'],
+ 'completed')
+self.assertEqual(dest_vm.command('query-status')['status'],
+ 'running')
+self.assertEqual(source_vm.command('query-status')['status'],
+ 'postmigrate')
--
2.14.5
[Qemu-devel] [PATCH 1/3] tests/acceptance/migration: fix post migration check
assert `query-migrate` in target doesn't give migration
status and test errors even if migration succeeds.
In target:
{'execute': 'query-migrate'}
{"return": {}}
Signed-off-by: Balamuruhan S
---
tests/acceptance/migration.py | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
index a44c1ae58f..0f3553c8f0 100644
--- a/tests/acceptance/migration.py
+++ b/tests/acceptance/migration.py
@@ -44,7 +44,8 @@ class Migration(Test):
step=0.1,
args=(source_vm,)
)
-self.assertEqual(dest_vm.command('query-migrate')['status'],
'completed')
-self.assertEqual(source_vm.command('query-migrate')['status'],
'completed')
+self.assertEqual(source_vm.command('query-migrate')['status'],
+ 'completed')
self.assertEqual(dest_vm.command('query-status')['status'], 'running')
-self.assertEqual(source_vm.command('query-status')['status'],
'postmigrate')
+self.assertEqual(source_vm.command('query-status')['status'],
+ 'postmigrate')
--
2.14.5
Re: [Qemu-devel] [PATCH v2 2/6] tests/acceptance: Test Open Firmware on the PReP/40p
On Sun, Sep 15, 2019 at 11:19 PM Philippe Mathieu-Daudé wrote:
>
> User case from:
> https://tyom.blogspot.com/2019/04/aixprep-under-qemu-how-to.html
>
> Signed-off-by: Philippe Mathieu-Daudé
Thanks a lot for this one, Philippe!
Acked-by: Artyom Tarasenko
> ---
> tests/acceptance/ppc_prep_40p.py | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/tests/acceptance/ppc_prep_40p.py
> b/tests/acceptance/ppc_prep_40p.py
> index 53f2d2ecf0..a0eac40d9f 100644
> --- a/tests/acceptance/ppc_prep_40p.py
> +++ b/tests/acceptance/ppc_prep_40p.py
> @@ -61,3 +61,24 @@ class IbmPrep40pMachine(Test):
> os_banner = 'NetBSD 4.0 (GENERIC) #0: Sun Dec 16 00:49:40 PST 2007'
> self.wait_for_console_pattern(os_banner)
> self.wait_for_console_pattern('Model: IBM PPS Model 6015')
> +
> +def test_openfirmware(self):
> +"""
> +:avocado: tags=arch:ppc
> +:avocado: tags=machine:40p
> +"""
> +bios_url = ('https://github.com/artyom-tarasenko/openfirmware/'
> +'releases/download/40p-20190413/q40pofw-serial.rom')
> +bios_hash = '880c80172ea5b2247c0ac2a8bf36bbe385192c72'
> +bios_path = self.fetch_asset(bios_url, asset_hash=bios_hash)
> +
> +self.vm.set_machine('40p')
> +self.vm.set_console()
> +self.vm.add_args('-bios', bios_path)
> +
> +self.vm.launch()
> +self.wait_for_console_pattern('QEMU PReP/40p')
> +fw_banner = 'Open Firmware, built April 13, 2019 09:29:23'
> +self.wait_for_console_pattern(fw_banner)
> +prompt_msg = 'Type any key to interrupt automatic startup'
> +self.wait_for_console_pattern(prompt_msg)
> --
> 2.20.1
>
--
Regards,
Artyom Tarasenko
SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu
Re: [Qemu-devel] [PATCH v2 1/6] tests/acceptance: Add test that runs NetBSD 4.0 installer on PRep/40p
On Sun, Sep 15, 2019 at 11:19 PM Philippe Mathieu-Daudé wrote:
>
> As of this commit, NetBSD 4.0 is very old. However it is enough to
> test the PRep/40p machine.
Not just it's enough, it's also the NetBSD release which definitely
was tested on physical 40p machines.
(It already has reviewed-by maintainer, so I don't suppose you'll need
it, but just in case,
Acked-by: Artyom Tarasenko
)
>
> User case from:
> http://mail-index.netbsd.org/port-prep/2017/04/11/msg000112.html
>
> Reviewed-by: Hervé Poussineau
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> Installers after 4.0 doesn't work anymore, not sure if this is a
> problem from the QEMU model or from NetBSD.
> ---
> MAINTAINERS | 1 +
> tests/acceptance/ppc_prep_40p.py | 63
> 2 files changed, 64 insertions(+)
> create mode 100644 tests/acceptance/ppc_prep_40p.py
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 50eaf005f4..ce809c7dee 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1068,6 +1068,7 @@ F: hw/timer/m48t59-isa.c
> F: include/hw/isa/pc87312.h
> F: include/hw/timer/m48t59.h
> F: pc-bios/ppc_rom.bin
> +F: tests/acceptance/machine_ppc_prep_40p.py
>
> sPAPR
> M: David Gibson
> diff --git a/tests/acceptance/ppc_prep_40p.py
> b/tests/acceptance/ppc_prep_40p.py
> new file mode 100644
> index 00..53f2d2ecf0
> --- /dev/null
> +++ b/tests/acceptance/ppc_prep_40p.py
> @@ -0,0 +1,63 @@
> +# Functional test that boots a PReP/40p machine and checks its serial
> console.
> +#
> +# Copyright (c) Philippe Mathieu-Daudé
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +
> +import os
> +import logging
> +
> +from avocado import skipIf
> +from avocado_qemu import Test
> +
> +
> +class IbmPrep40pMachine(Test):
> +
> +timeout = 60
> +
> +# TODO refactor to MachineTest
> +def wait_for_console_pattern(self, success_message,
> failure_message=None):
> +"""
> +Waits for messages to appear on the console, while logging the
> content
> +
> +:param success_message: if this message appears, test succeeds
> +:param failure_message: if this message appears, test fails
> +"""
> +console = self.vm.console_socket.makefile()
> +console_logger = logging.getLogger('console')
> +while True:
> +msg = console.readline().strip()
> +if not msg:
> +continue
> +console_logger.debug(msg)
> +if success_message in msg:
> +break
> +if failure_message and failure_message in msg:
> +fail = 'Failure message found in console: %s' %
> failure_message
> +self.fail(fail)
> +
> +@skipIf(os.getenv('CONTINUOUS_INTEGRATION'), 'Running on Travis-CI')
> +def test_factory_firmware_and_netbsd(self):
> +"""
> +:avocado: tags=arch:ppc
> +:avocado: tags=machine:40p
> +:avocado: tags=slowness:high
> +"""
> +bios_url = ('ftp://ftp.boulder.ibm.com/rs6000/firmware/'
> +'7020-40p/P12H0456.IMG')
> +bios_hash = '1775face4e6dc27f3a6ed955ef6eb331bf817f03'
> +bios_path = self.fetch_asset(bios_url, asset_hash=bios_hash)
> +drive_url = ('https://ftp.netbsd.org/pub/NetBSD/NetBSD-archive/'
> + 'NetBSD-4.0/prep/installation/floppy/generic_com0.fs')
> +drive_hash = 'dbcfc09912e71bd5f0d82c7c1ee43082fb596ceb'
> +drive_path = self.fetch_asset(drive_url, asset_hash=drive_hash)
> +
> +self.vm.set_machine('40p')
> +self.vm.set_console()
> +self.vm.add_args('-bios', bios_path,
> + '-fda', drive_path)
> +self.vm.launch()
> +os_banner = 'NetBSD 4.0 (GENERIC) #0: Sun Dec 16 00:49:40 PST 2007'
> +self.wait_for_console_pattern(os_banner)
> +self.wait_for_console_pattern('Model: IBM PPS Model 6015')
> --
> 2.20.1
>
--
Regards,
Artyom Tarasenko
SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu
Re: [Qemu-devel] [PATCH v2 6/6] .travis.yml: Split enterprise vs. hobbyist acceptance test job
Philippe Mathieu-Daudé writes:
> On 9/16/19 10:43 AM, Alex Bennée wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> ---
>>> .travis.yml | 18 --
>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/.travis.yml b/.travis.yml
>>> index 69a37f7387..753276eb33 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -265,9 +265,23 @@ matrix:
>>> - "3.6"
>>>
>>>
>>> -# Acceptance (Functional) tests
>>> +# Acceptance (Functional) tests [enterprise]
>>> - env:
>>> -- CONFIG="--python=/usr/bin/python3
>>> --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu"
>>> +- CONFIG="--python=/usr/bin/python3
>>> --target-list=x86_64-softmmu,mips64el-softmmu,aarch64-softmmu,s390x-softmmu,ppc64-softmmu"
>>
>> You could use:
>>
>> --target-list=${MAIN_SOFTMMU_TARGETS}
>>
>> and
>>
>> --disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}
>
> I like the idea, but this variable is slighly different:
>
> MAIN_SOFTMMU_TARGETS="aarch64-softmmu,arm-softmmu,i386-softmmu,mips-softmmu,mips64-softmmu,ppc64-softmmu,riscv64-softmmu,s390x-softmmu,x86_64-softmmu"
>
> Are you suggesting we use MAIN_SOFTMMU_TARGETS for 'Enterprise'
> targets?
Broadly - I'm note sure how we want to make the distinction in QEMU. We
have targets which are actively maintained and represent current
architectures that people still ship software to run on. The other
targets are machines that are either esoteric designs that exist in
particular niches or of academic interest and the old architecture kept
alive out of a sense of nostalgia.
>
> As long as mips64-softmmu is present, I think we can remove mips-softmmu
> from MAIN_TARGETS. Maybe i386-softmmu can be removed too.
Well the MIPS targets are actively maintained although for 32 bit you
don't see many distros for them these days. There might be an argument
for demoting i386-softmmu as I'm guessing there isn't much 32 bit kvm in
the enterprise right now (although It would be unsurprising to be told
otherwise).
>
> That would give the following hobbyist list:
>
> i386-softmmu,mips-softmmu,alpha-softmmu,ppc-softmmu,m68k-softmmu
I'm not sure about the hobbyist moniker as pretty much any of these TCG
targets is more actively maintained than the x86 which is mostly all
about the HW virtualisation these days. Trouble is I can't think of
another name that doesn't sound condescending:
HISTORICAL_SOFTMMU_TARGETS
LEGACY_SOFTMMU_TARGETS
FASTERTHANNATIVE_SOFTMMU_TARGETS
ARCHIVISTS_SOFTMMU_TARGETS
? As ever naming things is hard
>
> (I plan to add hppa-softmmu there too).
>
>>> +- TEST_CMD="make check-acceptance"
>>> + after_failure:
>>> +- cat tests/results/latest/job.log
>>> + addons:
>>> +apt:
>>> + packages:
>>> +- python3-pil
>>> +- python3-pip
>>> +- python3.5-venv
>>> +
>>> +
>>> +# Acceptance (Functional) tests [hobbyist]
>>> +- env:
>>> +- CONFIG="--python=/usr/bin/python3
>>> --target-list=mips-softmmu,arm-softmmu,alpha-softmmu,ppc-softmmu,m68k-softmmu"
>>> - TEST_CMD="make check-acceptance"
>>>after_failure:
>>> - cat tests/results/latest/job.log
>>
>>
>> --
>> Alex Bennée
>>
--
Alex Bennée
[Qemu-devel] [PATCH v2 0/2] trace: Forbid trailing newline in event format
Hi Stefan, I'v been confused by trailing newline in trace reports, so this series aims to fix this, by cleaning current formats and add a check to catch new one introduced. v2: - Use regex format (easier to review) - Added R-b Regards, Phil. Philippe Mathieu-Daudé (2): trace: Remove trailing newline in events trace: Forbid event format ending with newline character docs/devel/tracing.txt| 2 ++ hw/misc/trace-events | 10 +- hw/scsi/trace-events | 2 +- hw/sd/trace-events| 2 +- nbd/trace-events | 4 ++-- net/trace-events | 6 +++--- scripts/tracetool/__init__.py | 3 +++ 7 files changed, 17 insertions(+), 12 deletions(-) -- 2.20.1
[Qemu-devel] [PATCH v2 1/2] trace: Remove trailing newline in events
While the tracing frawework does not forbid trailing newline in
events format string, using them lead to confuse output.
It is the responsibility of the backend to properly end an event
line.
Some of our formats have trailing newlines, remove them.
Reviewed-by: John Snow
Reviewed-by: Kevin Wolf
Signed-off-by: Philippe Mathieu-Daudé
---
hw/misc/trace-events | 10 +-
hw/scsi/trace-events | 2 +-
hw/sd/trace-events | 2 +-
nbd/trace-events | 4 ++--
net/trace-events | 6 +++---
5 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index c1ea1aa437..74276225f8 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -118,11 +118,11 @@ iotkit_secctl_ns_read(uint32_t offset, uint64_t data,
unsigned size) "IoTKit Sec
iotkit_secctl_ns_write(uint32_t offset, uint64_t data, unsigned size) "IoTKit
SecCtl NS regs write: offset 0x%x data 0x%" PRIx64 " size %u"
# imx6ul_ccm.c
-ccm_entry(void) "\n"
-ccm_freq(uint32_t freq) "freq = %d\n"
-ccm_clock_freq(uint32_t clock, uint32_t freq) "(Clock = %d) = %d\n"
-ccm_read_reg(const char *reg_name, uint32_t value) "reg[%s] <= 0x%" PRIx32 "\n"
-ccm_write_reg(const char *reg_name, uint32_t value) "reg[%s] => 0x%" PRIx32
"\n"
+ccm_entry(void) ""
+ccm_freq(uint32_t freq) "freq = %d"
+ccm_clock_freq(uint32_t clock, uint32_t freq) "(Clock = %d) = %d"
+ccm_read_reg(const char *reg_name, uint32_t value) "reg[%s] <= 0x%" PRIx32
+ccm_write_reg(const char *reg_name, uint32_t value) "reg[%s] => 0x%" PRIx32
# iotkit-sysinfo.c
iotkit_sysinfo_read(uint64_t offset, uint64_t data, unsigned size) "IoTKit
SysInfo read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
index 452b5994e6..b0820052f8 100644
--- a/hw/scsi/trace-events
+++ b/hw/scsi/trace-events
@@ -28,7 +28,7 @@ mptsas_mmio_read(void *dev, uint32_t addr, uint32_t val) "dev
%p addr 0x%08x val
mptsas_mmio_unhandled_read(void *dev, uint32_t addr) "dev %p addr 0x%08x"
mptsas_mmio_unhandled_write(void *dev, uint32_t addr, uint32_t val) "dev %p
addr 0x%08x value 0x%x"
mptsas_mmio_write(void *dev, uint32_t addr, uint32_t val) "dev %p addr 0x%08x
value 0x%x"
-mptsas_process_message(void *dev, int msg, uint32_t ctx) "dev %p cmd %d
context 0x%08x\n"
+mptsas_process_message(void *dev, int msg, uint32_t ctx) "dev %p cmd %d
context 0x%08x"
mptsas_process_scsi_io_request(void *dev, int bus, int target, int lun,
uint64_t len) "dev %p dev %d:%d:%d length %"PRIu64""
mptsas_reset(void *dev) "dev %p "
mptsas_scsi_overflow(void *dev, uint32_t ctx, uint64_t req, uint64_t found)
"dev %p context 0x%08x: %"PRIu64"/%"PRIu64""
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 52971dc033..efcff666a2 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -4,7 +4,7 @@
bcm2835_sdhost_read(uint64_t offset, uint64_t data, unsigned size) "offset
0x%" PRIx64 " data 0x%" PRIx64 " size %u"
bcm2835_sdhost_write(uint64_t offset, uint64_t data, unsigned size) "offset
0x%" PRIx64 " data 0x%" PRIx64 " size %u"
bcm2835_sdhost_edm_change(const char *why, uint32_t edm) "(%s) EDM now 0x%x"
-bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x\n"
+bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x"
# core.c
sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg) "@%s CMD%02d
arg 0x%08x"
diff --git a/nbd/trace-events b/nbd/trace-events
index f6cde96790..a955918e97 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -61,8 +61,8 @@ nbd_negotiate_begin(void) "Beginning negotiation"
nbd_negotiate_new_style_size_flags(uint64_t size, unsigned flags) "advertising
size %" PRIu64 " and flags 0x%x"
nbd_negotiate_success(void) "Negotiation succeeded"
nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t
from, uint32_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = 0x%" PRIx16
", .type = 0x%" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }"
-nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching
clients to AIO context %p\n"
-nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients
from AIO context %p\n"
+nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching
clients to AIO context %p"
+nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients
from AIO context %p"
nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname,
int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s),
len = %d"
nbd_co_send_structured_done(uint64_t handle) "Send structured reply done:
handle = %" PRIu64
nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data,
size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset =
%" PRIu64 ", data = %p, len = %zu"
diff --git a/net/trace-events b/net/trace-events
index ac57056497..02c13fd0ba 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -17,9 +17,9 @@ colo_compare_icmp_m
[Qemu-devel] [PATCH v2 2/2] trace: Forbid event format ending with newline character
Event format ending with newlines confuse the trace reports.
Forbid them.
Add a check to refuse new format added with trailing newline:
$ make
[...]
GEN hw/misc/trace.h
Traceback (most recent call last):
File "scripts/tracetool.py", line 152, in
main(sys.argv)
File "scripts/tracetool.py", line 143, in main
events.extend(tracetool.read_events(fh, arg))
File "scripts/tracetool/__init__.py", line 367, in read_events
event = Event.build(line)
File "scripts/tracetool/__init__.py", line 281, in build
raise ValueError("Event format can not end with a newline character")
ValueError: Error at hw/misc/trace-events:121: Event format can not end with
a newline character
Reviewed-by: John Snow
Reviewed-by: Kevin Wolf
Signed-off-by: Philippe Mathieu-Daudé
---
v2: "\\n\"" -> r'\n"' (jsnow)
---
docs/devel/tracing.txt| 2 ++
scripts/tracetool/__init__.py | 3 +++
2 files changed, 5 insertions(+)
diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index 76e492a489..8231bbf5d1 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -112,6 +112,8 @@ Trace events should use types as follows:
Format strings should reflect the types defined in the trace event. Take
special care to use PRId64 and PRIu64 for int64_t and uint64_t types,
respectively. This ensures portability between 32- and 64-bit platforms.
+Format strings must not end with a newline character. It is the responsibility
+of backends to adapt line ending for proper logging.
Each event declaration will start with the event name, then its arguments,
finally a format string for pretty-printing. For example:
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 6fca674936..04279fa62e 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -277,6 +277,9 @@ class Event(object):
if fmt.find("%m") != -1 or fmt_trans.find("%m") != -1:
raise ValueError("Event format '%m' is forbidden, pass the error "
"as an explicit trace argument")
+if fmt.endswith(r'\n"'):
+raise ValueError("Event format must not end with a newline "
+ "character")
if len(fmt_trans) > 0:
fmt = [fmt_trans, fmt]
--
2.20.1
Re: [Qemu-devel] [PATCH v3 2/6] vmstate: add qom interface to get id
* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> Add an interface to get the instance id, instead of depending on
> Device and qdev_get_dev_path().
>
> Signed-off-by: Marc-André Lureau
> ---
> hw/core/Makefile.objs| 1 +
> hw/core/qdev.c | 14 ++
> hw/core/vmstate-if.c | 23 +++
> include/hw/vmstate-if.h | 32
> include/migration/register.h | 2 ++
> include/migration/vmstate.h | 2 ++
> tests/Makefile.include | 1 +
> 7 files changed, 75 insertions(+)
> create mode 100644 hw/core/vmstate-if.c
> create mode 100644 include/hw/vmstate-if.h
>
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index fd0550d1d9..0edd9e635d 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -9,6 +9,7 @@ common-obj-y += hotplug.o
> common-obj-$(CONFIG_SOFTMMU) += nmi.o
> common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
> common-obj-y += cpu.o
> +common-obj-y += vmstate-if.o
>
> common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
> common-obj-$(CONFIG_XILINX_AXI) += stream.o
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 60d66c2f39..7e083dfcae 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1047,9 +1047,18 @@ static void device_unparent(Object *obj)
> }
> }
>
> +static char *
> +device_vmstate_if_get_id(VMStateIf *obj)
> +{
> +DeviceState *dev = DEVICE(obj);
> +
> +return qdev_get_dev_path(dev);
> +}
> +
> static void device_class_init(ObjectClass *class, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(class);
> +VMStateIfClass *vc = VMSTATE_IF_CLASS(class);
>
> class->unparent = device_unparent;
>
> @@ -1061,6 +1070,7 @@ static void device_class_init(ObjectClass *class, void
> *data)
> */
> dc->hotpluggable = true;
> dc->user_creatable = true;
> +vc->get_id = device_vmstate_if_get_id;
> }
>
> void device_class_set_parent_reset(DeviceClass *dc,
> @@ -1118,6 +1128,10 @@ static const TypeInfo device_type_info = {
> .class_init = device_class_init,
> .abstract = true,
> .class_size = sizeof(DeviceClass),
> +.interfaces = (InterfaceInfo[]) {
> +{ TYPE_VMSTATE_IF },
> +{ }
> +}
> };
>
> static void qdev_register_types(void)
> diff --git a/hw/core/vmstate-if.c b/hw/core/vmstate-if.c
> new file mode 100644
> index 00..d0b2392b0a
> --- /dev/null
> +++ b/hw/core/vmstate-if.c
> @@ -0,0 +1,23 @@
> +/*
> + * VMState interface
> + *
> + * Copyright (c) 2009-2017 Red Hat Inc
You might want to update the years; but other than from the migration
side this looks OK to me, but it needs checking by a QOM person.
Acked-by: Dr. David Alan Gilbert
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/vmstate-if.h"
> +
> +static const TypeInfo vmstate_if_info = {
> +.name = TYPE_VMSTATE_IF,
> +.parent = TYPE_INTERFACE,
> +.class_size = sizeof(VMStateIfClass),
> +};
> +
> +static void vmstate_register_types(void)
> +{
> +type_register_static(&vmstate_if_info);
> +}
> +
> +type_init(vmstate_register_types);
> diff --git a/include/hw/vmstate-if.h b/include/hw/vmstate-if.h
> new file mode 100644
> index 00..92682f5bc2
> --- /dev/null
> +++ b/include/hw/vmstate-if.h
> @@ -0,0 +1,32 @@
> +#ifndef VMSTATE_IF_H
> +#define VMSTATE_IF_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_VMSTATE_IF "vmstate-if"
> +
> +#define VMSTATE_IF_CLASS(klass) \
> +OBJECT_CLASS_CHECK(VMStateIfClass, (klass), TYPE_VMSTATE_IF)
> +#define VMSTATE_IF_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(VMStateIfClass, (obj), TYPE_VMSTATE_IF)
> +#define VMSTATE_IF(obj) \
> +INTERFACE_CHECK(VMStateIf, (obj), TYPE_VMSTATE_IF)
> +
> +typedef struct VMStateIf VMStateIf;
> +
> +typedef struct VMStateIfClass {
> +InterfaceClass parent_class;
> +
> +char * (*get_id)(VMStateIf *obj);
> +} VMStateIfClass;
> +
> +static inline char *vmstate_if_get_id(VMStateIf *vmif)
> +{
> +if (!vmif) {
> +return NULL;
> +}
> +
> +return VMSTATE_IF_GET_CLASS(vmif)->get_id(vmif);
> +}
> +
> +#endif /* VMSTATE_IF_H */
> diff --git a/include/migration/register.h b/include/migration/register.h
> index 3d0b9833c6..74f1578b29 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -14,6 +14,8 @@
> #ifndef MIGRATION_REGISTER_H
> #define MIGRATION_REGISTER_H
>
> +#include "hw/vmstate-if.h"
> +
> typedef struct SaveVMHandlers {
> /* This runs inside the iothread lock. */
> SaveStateHandler *save_state;
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1fbfd099dd..bdcf8a1652 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -27,6 +27,
Re: [Qemu-devel] [PATCH v2 0/6] tests/acceptance: Add tests for the PReP/40p machine
Philippe Mathieu-Daudé writes: > Hi David, > > On 9/16/19 2:42 AM, David Gibson wrote: >> On Sun, Sep 15, 2019 at 11:19:34PM +0200, Philippe Mathieu-Daudé wrote: >>> Quick tests worth to avoid regressions with the 40p machine. >>> idea from the "Maintainers, please tell us how to boot your machines" >>> thread: >>> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg04177.html >>> >>> v2: Split Travis job, added Hervé R-b tag >>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05896.html >>> >>> Regards, >>> >>> Phil. >> >> I'm guessing you're expecting these to go in via the testing tree, in >> which case >> >> Acked-by: David Gibson > > Thanks, appreciated :) > >> Or do you want me to take them via the ppc tree? > > I think the 'testing tree' should focus on the CI/testing > infrastructure, while each subsystem maintainers should care about the > tests covering their subsystem (the testing tree maintainers might not > have the required knowledge to be sure a test is correctly implemented). > > In this particular case I assume you don't have much knowledge of that > PPC machine, which is a hobbyist one, but since you are the PPC > maintainer, I'd rather see this going via your tree :) > > Alex/Cleber/Eduardo, any comment on this position? Once we have a .travis.yml I'm happy with it can go in via another tree no problem. See other thread > > Thanks, > > Phil. > >>> Philippe Mathieu-Daudé (6): >>> tests/acceptance: Add test that runs NetBSD 4.0 installer on PRep/40p >>> tests/acceptance: Test Open Firmware on the PReP/40p >>> tests/acceptance: Test OpenBIOS on the PReP/40p >>> tests/acceptance: Test Sandalfoot initrd on the PReP/40p >>> .travis.yml: Let the avocado job run the 40p tests >>> .travis.yml: Split enterprise vs. hobbyist acceptance test job >>> >>> .travis.yml | 18 +++- >>> MAINTAINERS | 1 + >>> tests/acceptance/ppc_prep_40p.py | 150 +++ >>> 3 files changed, 167 insertions(+), 2 deletions(-) >>> create mode 100644 tests/acceptance/ppc_prep_40p.py >>> >> -- Alex Bennée
Re: [Qemu-devel] [PATCH v6 28/42] stream: Deal with filters
On 13.09.19 16:16, Kevin Wolf wrote:
> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>> Because of the recent changes that make the stream job independent of
>> the base node and instead track the node above it, we have to split that
>> "bottom" node into two cases: The bottom COW node, and the node directly
>> above the base node (which may be an R/W filter or the bottom COW node).
>>
>> Signed-off-by: Max Reitz
>> ---
>> qapi/block-core.json | 4
>> block/stream.c | 52
>> blockdev.c | 2 +-
>> 3 files changed, 38 insertions(+), 20 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 38c4dbd7c3..3c54717870 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2516,6 +2516,10 @@
>> # On successful completion the image file is updated to drop the backing
>> file
>> # and the BLOCK_JOB_COMPLETED event is emitted.
>> #
>> +# In case @device is a filter node, block-stream modifies the first
>> non-filter
>> +# overlay node below it to point to base's backing node (or NULL if @base
>> was
>> +# not specified) instead of modifying @device itself.
>> +#
>> # @job-id: identifier for the newly-created block job. If
>> # omitted, the device name will be used. (Since 2.7)
>> #
>> diff --git a/block/stream.c b/block/stream.c
>> index 4c8b89884a..bd4a351dae 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -31,7 +31,8 @@ enum {
>>
>> typedef struct StreamBlockJob {
>> BlockJob common;
>> -BlockDriverState *bottom;
>> +BlockDriverState *bottom_cow_node;
>> +BlockDriverState *above_base;
>
> Confusing naming, especially because in commit you used above_base for
> what is bottom_cow_node here. Vladimir already suggested using
> base_overlay consistently, so we should do this here too (for
> bottom_cow_node). above_base can keep its name because the different
> above_base in commit is going to be renamed).
Sure.
>> BlockdevOnError on_error;
>> char *backing_file_str;
>> bool bs_read_only;
>> @@ -54,7 +55,7 @@ static void stream_abort(Job *job)
>>
>> if (s->chain_frozen) {
>> BlockJob *bjob = &s->common;
>> -bdrv_unfreeze_chain(blk_bs(bjob->blk), s->bottom);
>> +bdrv_unfreeze_chain(blk_bs(bjob->blk), s->above_base);
>> }
>> }
>>
>> @@ -63,14 +64,15 @@ static int stream_prepare(Job *job)
>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> BlockJob *bjob = &s->common;
>> BlockDriverState *bs = blk_bs(bjob->blk);
>> -BlockDriverState *base = backing_bs(s->bottom);
>> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
>> +BlockDriverState *base = bdrv_filtered_bs(s->above_base);
>> Error *local_err = NULL;
>> int ret = 0;
>>
>> -bdrv_unfreeze_chain(bs, s->bottom);
>> +bdrv_unfreeze_chain(bs, s->above_base);
>> s->chain_frozen = false;
>>
>> -if (bs->backing) {
>> +if (bdrv_filtered_cow_child(unfiltered_bs)) {
>> const char *base_id = NULL, *base_fmt = NULL;
>> if (base) {
>> base_id = s->backing_file_str;
>> @@ -78,8 +80,8 @@ static int stream_prepare(Job *job)
>> base_fmt = base->drv->format_name;
>> }
>> }
>> -bdrv_set_backing_hd(bs, base, &local_err);
>> -ret = bdrv_change_backing_file(bs, base_id, base_fmt);
>> +bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
>> +ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
>> if (local_err) {
>> error_report_err(local_err);
>> return -EPERM;
>> @@ -110,7 +112,8 @@ static int coroutine_fn stream_run(Job *job, Error
>> **errp)
>> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>> BlockBackend *blk = s->common.blk;
>> BlockDriverState *bs = blk_bs(blk);
>> -bool enable_cor = !backing_bs(s->bottom);
>> +BlockDriverState *unfiltered_bs = bdrv_skip_rw_filters(bs);
>> +bool enable_cor = !bdrv_filtered_bs(s->above_base);
>> int64_t len;
>> int64_t offset = 0;
>> uint64_t delay_ns = 0;
>> @@ -119,7 +122,7 @@ static int coroutine_fn stream_run(Job *job, Error
>> **errp)
>> int64_t n = 0; /* bytes */
>> void *buf;
>>
>> -if (bs == s->bottom) {
>> +if (unfiltered_bs == s->bottom_cow_node) {
>> /* Nothing to stream */
>> return 0;
>> }
>> @@ -154,13 +157,14 @@ static int coroutine_fn stream_run(Job *job, Error
>> **errp)
>>
>> copy = false;
>>
>> -ret = bdrv_is_allocated(bs, offset, STREAM_BUFFER_SIZE, &n);
>> +ret = bdrv_is_allocated(unfiltered_bs, offset, STREAM_BUFFER_SIZE,
>> &n);
>> if (ret == 1) {
>> /* Allocated in the top, no need to copy. */
>> } else if (ret >= 0) {
>> /* Copy if allocated in the intermediate images. Limit to the
>
Re: [Qemu-devel] [PATCH v2 0/6] tests/acceptance: Add tests for the PReP/40p machine
On 9/16/19 11:52 AM, Alex Bennée wrote: > > Philippe Mathieu-Daudé writes: > >> Hi David, >> >> On 9/16/19 2:42 AM, David Gibson wrote: >>> On Sun, Sep 15, 2019 at 11:19:34PM +0200, Philippe Mathieu-Daudé wrote: Quick tests worth to avoid regressions with the 40p machine. idea from the "Maintainers, please tell us how to boot your machines" thread: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg04177.html v2: Split Travis job, added Hervé R-b tag v1: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05896.html Regards, Phil. >>> >>> I'm guessing you're expecting these to go in via the testing tree, in >>> which case >>> >>> Acked-by: David Gibson >> >> Thanks, appreciated :) >> >>> Or do you want me to take them via the ppc tree? >> >> I think the 'testing tree' should focus on the CI/testing >> infrastructure, while each subsystem maintainers should care about the >> tests covering their subsystem (the testing tree maintainers might not >> have the required knowledge to be sure a test is correctly implemented). >> >> In this particular case I assume you don't have much knowledge of that >> PPC machine, which is a hobbyist one, but since you are the PPC >> maintainer, I'd rather see this going via your tree :) >> >> Alex/Cleber/Eduardo, any comment on this position? > > Once we have a .travis.yml I'm happy with it can go in via another tree > no problem. See other thread Good :) David can take patches 1-5 (I tagged patch 6 as RFC but messed something with git-publish and lost it when I sent this series). Thanks! Philippe Mathieu-Daudé (6): tests/acceptance: Add test that runs NetBSD 4.0 installer on PRep/40p tests/acceptance: Test Open Firmware on the PReP/40p tests/acceptance: Test OpenBIOS on the PReP/40p tests/acceptance: Test Sandalfoot initrd on the PReP/40p .travis.yml: Let the avocado job run the 40p tests .travis.yml: Split enterprise vs. hobbyist acceptance test job .travis.yml | 18 +++- MAINTAINERS | 1 + tests/acceptance/ppc_prep_40p.py | 150 +++ 3 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 tests/acceptance/ppc_prep_40p.py >>> > > > -- > Alex Bennée >
Re: [Qemu-devel] [PATCH v3 5/6] docs: start a document to describe D-Bus usage
(Copying in Stefan since he was looking at DBus for virtiofs) * Marc-André Lureau (marcandre.lur...@redhat.com) wrote: > Signed-off-by: Marc-André Lureau > --- > docs/interop/dbus.rst | 73 ++ > docs/interop/index.rst | 1 + > 2 files changed, 74 insertions(+) > create mode 100644 docs/interop/dbus.rst > > diff --git a/docs/interop/dbus.rst b/docs/interop/dbus.rst > new file mode 100644 > index 00..c08f026edc > --- /dev/null > +++ b/docs/interop/dbus.rst > @@ -0,0 +1,73 @@ > += > +D-Bus > += > + > +Introduction > + > + > +QEMU may be running with various helper processes involved: > + - vhost-user* processes (gpu, virtfs, input, etc...) > + - TPM emulation (or other devices) > + - user networking (slirp) > + - network services (DHCP/DNS, samba/ftp etc) > + - background tasks (compression, streaming etc) > + - client UI > + - admin & cli > + > +Having several processes allows stricter security rules, as well as > +greater modularity. > + > +While QEMU itself uses QMP as primary IPC (and Spice/VNC for remote > +display), D-Bus is the de facto IPC of choice on Unix systems. The > +wire format is machine friendly, good bindings exist for various > +languages, and there are various tools available. > + > +Using a bus, helper processes can discover and communicate with each > +other easily, without going through QEMU. The bus topology is also > +easier to apprehend and debug than a mesh. However, it is wise to > +consider the security aspects of it. > + > +Security > + > + > +A QEMU D-Bus bus should be private to a single VM. Thus, only > +cooperative tasks are running on the same bus to serve the VM. > + > +D-Bus, the protocol and standard, doesn't have mechanisms to enforce > +security between peers once the connection is established. Peers may > +have additional mechanisms to enforce security rules, based for > +example on UNIX credentials. > + > +dbus-daemon can enforce various policies based on the UID/GID of the > +processes that are connected to it. It is thus a good idea to run > +helpers as different UID from QEMU and set appropriate policies (so > +helper processes are only allowed to talk to qemu for example). > + > +For example, this allows only ``qemu`` user to talk to ``qemu-helper`` > +``org.qemu.Helper1`` service: > + > +.. code:: xml > + > + > + > + > + > + > + > + > + > + > + > +dbus-daemon can also perfom SELinux checks based on the security > +context of the source and the target. For example, ``virtiofs_t`` > +could be allowed to send a message to ``svirt_t``, but ``virtiofs_t`` > +wouldn't be allowed to send a message to ``virtiofs_t``. I think we need to start thinking about this more now rather than 'can'. . Dave > +Guidelines > +== > + > +When implementing new D-Bus interfaces, it is recommended to follow > +the "D-Bus API Design Guidelines": > +https://dbus.freedesktop.org/doc/dbus-api-design.html > + > +The "org.qemu*" prefix is reserved for the QEMU project. > diff --git a/docs/interop/index.rst b/docs/interop/index.rst > index b4bfcab417..fa4478ce2e 100644 > --- a/docs/interop/index.rst > +++ b/docs/interop/index.rst > @@ -13,6 +13,7 @@ Contents: > :maxdepth: 2 > > bitmaps > + dbus > live-block-operations > pr-helper > vhost-user > -- > 2.23.0 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2 5/6] .travis.yml: Let the avocado job run the 40p tests
Philippe Mathieu-Daudé writes: > Signed-off-by: Philippe Mathieu-Daudé Acked-by: Alex Bennée > --- > If this list continues to grow we can > - split it (as other jobs) > - move them to GitLab where we can have multi-stage jobs, > avocado tests run on top of build jobs. > --- > .travis.yml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/.travis.yml b/.travis.yml > index d0b9e099b9..69a37f7387 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -267,7 +267,7 @@ matrix: > > # Acceptance (Functional) tests > - env: > -- CONFIG="--python=/usr/bin/python3 > --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc64-softmmu,m68k-softmmu" > +- CONFIG="--python=/usr/bin/python3 > --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu" > - TEST_CMD="make check-acceptance" >after_failure: > - cat tests/results/latest/job.log -- Alex Bennée
Re: [Qemu-devel] [PATCH v2 6/6] .travis.yml: Split enterprise vs. hobbyist acceptance test job
On 9/16/19 11:46 AM, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé writes:
>
>> On 9/16/19 10:43 AM, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé writes:
>>>
Signed-off-by: Philippe Mathieu-Daudé
---
.travis.yml | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index 69a37f7387..753276eb33 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -265,9 +265,23 @@ matrix:
- "3.6"
-# Acceptance (Functional) tests
+# Acceptance (Functional) tests [enterprise]
- env:
-- CONFIG="--python=/usr/bin/python3
--target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu"
+- CONFIG="--python=/usr/bin/python3
--target-list=x86_64-softmmu,mips64el-softmmu,aarch64-softmmu,s390x-softmmu,ppc64-softmmu"
>>>
>>> You could use:
>>>
>>> --target-list=${MAIN_SOFTMMU_TARGETS}
>>>
>>> and
>>>
>>> --disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}
>>
>> I like the idea, but this variable is slighly different:
>>
>> MAIN_SOFTMMU_TARGETS="aarch64-softmmu,arm-softmmu,i386-softmmu,mips-softmmu,mips64-softmmu,ppc64-softmmu,riscv64-softmmu,s390x-softmmu,x86_64-softmmu"
>>
>> Are you suggesting we use MAIN_SOFTMMU_TARGETS for 'Enterprise'
>> targets?
>
> Broadly - I'm note sure how we want to make the distinction in QEMU. We
> have targets which are actively maintained and represent current
> architectures that people still ship software to run on. The other
> targets are machines that are either esoteric designs that exist in
> particular niches or of academic interest and the old architecture kept
> alive out of a sense of nostalgia.
>
>>
>> As long as mips64-softmmu is present, I think we can remove mips-softmmu
>> from MAIN_TARGETS. Maybe i386-softmmu can be removed too.
>
> Well the MIPS targets are actively maintained although for 32 bit you
> don't see many distros for them these days. There might be an argument
> for demoting i386-softmmu as I'm guessing there isn't much 32 bit kvm in
> the enterprise right now (although It would be unsurprising to be told
> otherwise).
>
>>
>> That would give the following hobbyist list:
>>
>> i386-softmmu,mips-softmmu,alpha-softmmu,ppc-softmmu,m68k-softmmu
>
> I'm not sure about the hobbyist moniker as pretty much any of these TCG
> targets is more actively maintained than the x86 which is mostly all
> about the HW virtualisation these days. Trouble is I can't think of
> another name that doesn't sound condescending:
>
> HISTORICAL_SOFTMMU_TARGETS
> LEGACY_SOFTMMU_TARGETS
> FASTERTHANNATIVE_SOFTMMU_TARGETS
> ARCHIVISTS_SOFTMMU_TARGETS
>
> ? As ever naming things is hard
:) I'm not trying to classify tests, I simply want users/maintainers to
not be reluctant/scared to add tests because "CI takes too long".
Also, I find it practical to have small jobs, what I do when testing a
series when I know it only affects a set of jobs, I push and directly
click "cancel all jobs" on the Travis web, then I just click on "restart
this job" on the set I'm interested in.
This way I can quickly figure out the series is OK or rework it.
>> (I plan to add hppa-softmmu there too).
>>
+- TEST_CMD="make check-acceptance"
+ after_failure:
+- cat tests/results/latest/job.log
+ addons:
+apt:
+ packages:
+- python3-pil
+- python3-pip
+- python3.5-venv
+
+
+# Acceptance (Functional) tests [hobbyist]
+- env:
+- CONFIG="--python=/usr/bin/python3
--target-list=mips-softmmu,arm-softmmu,alpha-softmmu,ppc-softmmu,m68k-softmmu"
- TEST_CMD="make check-acceptance"
after_failure:
- cat tests/results/latest/job.log
>>>
>>>
>>> --
>>> Alex Bennée
>>>
>
>
> --
> Alex Bennée
>
Re: [Qemu-devel] [PATCH v6 25/42] mirror: Deal with filters
On 13.09.19 14:55, Kevin Wolf wrote:
> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>> This includes some permission limiting (for example, we only need to
>> take the RESIZE permission for active commits where the base is smaller
>> than the top).
>>
>> Signed-off-by: Max Reitz
>> ---
>> block/mirror.c | 117 ++---
>> blockdev.c | 47 +---
>> 2 files changed, 131 insertions(+), 33 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 54bafdf176..6ddbfb9708 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -42,6 +42,7 @@ typedef struct MirrorBlockJob {
>> BlockBackend *target;
>> BlockDriverState *mirror_top_bs;
>> BlockDriverState *base;
>> +BlockDriverState *base_overlay;
>>
>> /* The name of the graph node to replace */
>> char *replaces;
>> @@ -665,8 +666,10 @@ static int mirror_exit_common(Job *job)
>> &error_abort);
>> if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>> BlockDriverState *backing = s->is_none_mode ? src : s->base;
>> -if (backing_bs(target_bs) != backing) {
>> -bdrv_set_backing_hd(target_bs, backing, &local_err);
>> +BlockDriverState *unfiltered_target =
>> bdrv_skip_rw_filters(target_bs);
>> +
>> +if (bdrv_filtered_cow_bs(unfiltered_target) != backing) {
>> +bdrv_set_backing_hd(unfiltered_target, backing, &local_err);
>> if (local_err) {
>> error_report_err(local_err);
>> ret = -EPERM;
>> @@ -715,7 +718,7 @@ static int mirror_exit_common(Job *job)
>> * valid.
>> */
>> block_job_remove_all_bdrv(bjob);
>> -bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs),
>> &error_abort);
>> +bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs,
>> &error_abort);
>>
>> /* We just changed the BDS the job BB refers to (with either or both of
>> the
>> * bdrv_replace_node() calls), so switch the BB back so the cleanup does
>> @@ -812,7 +815,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob
>> *s)
>> return 0;
>> }
>>
>> -ret = bdrv_is_allocated_above(bs, base, false, offset, bytes,
>> &count);
>> +ret = bdrv_is_allocated_above(bs, s->base_overlay, true, offset,
>> bytes,
>> + &count);
>> if (ret < 0) {
>> return ret;
>> }
>> @@ -908,7 +912,7 @@ static int coroutine_fn mirror_run(Job *job, Error
>> **errp)
>> } else {
>> s->target_cluster_size = BDRV_SECTOR_SIZE;
>> }
>> -if (backing_filename[0] && !target_bs->backing &&
>> +if (backing_filename[0] && !bdrv_backing_chain_next(target_bs) &&
>> s->granularity < s->target_cluster_size) {
>> s->buf_size = MAX(s->buf_size, s->target_cluster_size);
>> s->cow_bitmap = bitmap_new(length);
>> @@ -1088,8 +1092,9 @@ static void mirror_complete(Job *job, Error **errp)
>> if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
>> int ret;
>>
>> -assert(!target->backing);
>> -ret = bdrv_open_backing_file(target, NULL, "backing", errp);
>> +assert(!bdrv_backing_chain_next(target));
>> +ret = bdrv_open_backing_file(bdrv_skip_rw_filters(target), NULL,
>> + "backing", errp);
>> if (ret < 0) {
>> return;
>> }
>> @@ -1531,8 +1536,8 @@ static BlockJob *mirror_start_job(
>> MirrorBlockJob *s;
>> MirrorBDSOpaque *bs_opaque;
>> BlockDriverState *mirror_top_bs;
>> -bool target_graph_mod;
>> bool target_is_backing;
>> +uint64_t target_perms, target_shared_perms;
>> Error *local_err = NULL;
>> int ret;
>>
>> @@ -1551,7 +1556,7 @@ static BlockJob *mirror_start_job(
>> buf_size = DEFAULT_MIRROR_BUF_SIZE;
>> }
>>
>> -if (bs == target) {
>> +if (bdrv_skip_rw_filters(bs) == bdrv_skip_rw_filters(target)) {
>> error_setg(errp, "Can't mirror node into itself");
>> return NULL;
>> }
>> @@ -1615,15 +1620,50 @@ static BlockJob *mirror_start_job(
>> * In the case of active commit, things look a bit different, though,
>> * because the target is an already populated backing file in active
>> use.
>> * We can allow anything except resize there.*/
>> +
>> +target_perms = BLK_PERM_WRITE;
>> +target_shared_perms = BLK_PERM_WRITE_UNCHANGED;
>> +
>> target_is_backing = bdrv_chain_contains(bs, target);
>> -target_graph_mod = (backing_mode != MIRROR_LEAVE_BACKING_CHAIN);
>> +if (target_is_backing) {
>> +int64_t bs_size, target_size;
>> +bs_size = bdrv_getlength(bs);
>> +if (bs_size < 0) {
>> +error_setg_errno(errp, -bs_size,
>> + "Could not inquire top image size");
>> +goto fail;
Re: [Qemu-devel] [PATCH-for-4.2 v10 05/11] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
Hi Igor,
> -Original Message-
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: 11 September 2019 14:07
> To: Shameerali Kolothum Thodi
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org;
> shannon.zha...@gmail.com; sa...@linux.intel.com;
> sebastien.bo...@intel.com; xuwei (O) ;
> ler...@redhat.com; ard.biesheu...@linaro.org; Linuxarm
>
> Subject: Re: [PATCH-for-4.2 v10 05/11] hw/arm/virt: Enable device memory
> cold/hot plug with ACPI boot
>
> On Wed, 4 Sep 2019 09:56:23 +0100
> Shameer Kolothum wrote:
>
> [...]
> > @@ -730,6 +733,19 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> >vms->highmem, vms->highmem_ecam);
> > acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
> > (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
> > +if (vms->acpi_dev) {
> > +build_ged_aml(scope, "\\_SB."GED_DEVICE,
> > + HOTPLUG_HANDLER(vms->acpi_dev),
> > + irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE,
> AML_SYSTEM_MEMORY,
> > + memmap[VIRT_ACPI_GED].base);
> > +}
> > +
> > +if (vms->acpi_dev && ms->ram_slots) {
> > +build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB",
> NULL,
> > + AML_SYSTEM_MEMORY,
> > +
> memmap[VIRT_PCDIMM_ACPI].base);
> > +}
> one more thing (though non critical), if ms->ram_slots == 0
> makes IASL spew a warning
>
> External (_SB_.MHPC.MSCN, MethodObj)// Warning: Unknown
> method, guessing 0 arguments
>
> In general non-existing references within methods are fine if they aren't ever
> used.
> however we can be a little bit less sloppy.
> Below you advertise "event = ACPI_GED_MEM_HOTPLUG_EVT", and then here
> suddenly
> don't generate essential AML part for it here.
Ok.
> For consistency if above is conditioned on ms->ram_slots != 0, probably
> it would be better to move that condition where you set 'event' value and
> check property value above instead of ms->ram_slots
I understand the concern here, but not sure I get the suggestion to check
the "property" instead of ms->ram_slots correctly.
Is this what you have in mind?
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 538b3bbefa..5c9269dca1 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -742,10 +742,15 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
memmap[VIRT_ACPI_GED].base);
}
-if (vms->acpi_dev && ms->ram_slots) {
-build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
- AML_SYSTEM_MEMORY,
- memmap[VIRT_PCDIMM_ACPI].base);
+if (vms->acpi_dev) {
+uint32_t event = object_property_get_uint(OBJECT(vms->acpi_dev),
+ "ged-event", NULL);
+
+if (event & ACPI_GED_MEM_HOTPLUG_EVT) {
+build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
+ AML_SYSTEM_MEMORY,
+ memmap[VIRT_PCDIMM_ACPI].base);
+}
}
acpi_dsdt_add_power_button(scope);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index bc152ea2b0..6b024b16df 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -534,8 +534,13 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq
*pic)
{
DeviceState *dev;
+MachineState *ms = MACHINE(vms);
int irq = vms->irqmap[VIRT_ACPI_GED];
-uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
+uint32_t event = 0;
+
+if (ms->ram_slots) {
+event = ACPI_GED_MEM_HOTPLUG_EVT;
+}
dev = qdev_create(NULL, TYPE_ACPI_GED);
qdev_prop_set_uint32(dev, "ged-event", event);
---8---
Please let me know.
Thanks,
Shameer
> [...]
> > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,
> qemu_irq *pic)
> > +{
> > +DeviceState *dev;
> > +int irq = vms->irqmap[VIRT_ACPI_GED];
> > +uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > +
> > +dev = qdev_create(NULL, TYPE_ACPI_GED);
> > +qdev_prop_set_uint32(dev, "ged-event", event);
> > +qdev_init_nofail(dev);
> > +
> > +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> vms->memmap[VIRT_ACPI_GED].base);
> > +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1,
> vms->memmap[VIRT_PCDIMM_ACPI].base);
> > +
> > +sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irq]);
> > +
> > +return dev;
> > +}
> > +
> [...]
Re: [Qemu-devel] [PATCH v2 26/28] s390x/tcg: MVST: Fault-safe handling
On 11.09.19 23:52, Richard Henderson wrote:
> On 9/6/19 3:57 AM, David Hildenbrand wrote:
>> +/*
>> + * Our access should not exceed single pages, as we must not report
>> access
>> + * exceptions exceeding the actually copied range (which we don't know
>> at
>> + * this point). We might over-indicate watchpoints within the pages
>> + * (if we ever care, we have to limit processing to a single byte).
>> + */
>> +srca = access_prepare(env, s, len, MMU_DATA_LOAD, ra);
>> +desta = access_prepare(env, d, len, MMU_DATA_STORE, ra);
>> +for (i = 0; i < len; i++) {
>> +const uint8_t v = access_get_byte(env, &srca, i, ra);
>> +
>> +access_set_byte(env, &desta, i, v, ra);
>> if (v == c) {
>> -set_address_zero(env, r1, d + len);
>> +set_address_zero(env, r1, d + i);
>> return 1;
>> }
>
> Worth using memchr + memmove w/ nondestructive overlap?
In theory yes, however, the issue is that we would have multiple
accesses, which is not documented for this instruction. In case the
memory is modified between memchr + memmove by another CPU, we could
have an inconsistent instruction result. Unlikely but possible :)
>
> That said,
> Reviewed-by: Richard Henderson
>
> r~
>
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [PATCH v3 6/6] Add dbus-vmstate object
* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> When instanciated, this object will connect to the given D-Bus
> bus. During migration, it will take the data from org.qemu.VMState1
> instances.
>
> See documentation for further details.
>
> Signed-off-by: Marc-André Lureau
I could do with this being in smaller chunks; it's a bit of a big
uncommented patch.
Dave
> ---
> MAINTAINERS | 6 +
> backends/Makefile.objs| 4 +
> backends/dbus-vmstate.c | 530 ++
> configure | 7 +
> docs/interop/dbus-vmstate.rst | 68 +
> docs/interop/index.rst| 1 +
> tests/Makefile.include| 19 +-
> tests/dbus-vmstate-daemon.sh | 95 ++
> tests/dbus-vmstate-test.c | 386 +
> tests/dbus-vmstate1.xml | 12 +
> 10 files changed, 1127 insertions(+), 1 deletion(-)
> create mode 100644 backends/dbus-vmstate.c
> create mode 100644 docs/interop/dbus-vmstate.rst
> create mode 100755 tests/dbus-vmstate-daemon.sh
> create mode 100644 tests/dbus-vmstate-test.c
> create mode 100644 tests/dbus-vmstate1.xml
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 50eaf005f4..f641870c40 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2153,6 +2153,12 @@ F: tests/migration-test.c
> F: docs/devel/migration.rst
> F: qapi/migration.json
>
> +DBus VMState
> +M: Marc-André Lureau
> +S: Maintained
> +F: backends/dbus-vmstate.c
> +F: tests/dbus-vmstate*
> +
> Seccomp
> M: Eduardo Otubo
> S: Supported
> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index f0691116e8..28a847cd57 100644
> --- a/backends/Makefile.objs
> +++ b/backends/Makefile.objs
> @@ -17,3 +17,7 @@ endif
> common-obj-$(call land,$(CONFIG_VHOST_USER),$(CONFIG_VIRTIO)) += vhost-user.o
>
> common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
> +
> +common-obj-$(CONFIG_GIO) += dbus-vmstate.o
> +dbus-vmstate.o-cflags = $(GIO_CFLAGS)
> +dbus-vmstate.o-libs = $(GIO_LIBS)
> diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
> new file mode 100644
> index 00..559f5430c5
> --- /dev/null
> +++ b/backends/dbus-vmstate.c
> @@ -0,0 +1,530 @@
> +/*
> + * QEMU dbus-vmstate
> + *
> + * Copyright (C) 2019 Red Hat Inc
> + *
> + * Authors:
> + * Marc-André Lureau
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "qapi/qmp/qerror.h"
> +#include "migration/vmstate.h"
> +#include
> +
> +typedef struct DBusVMState DBusVMState;
> +typedef struct DBusVMStateClass DBusVMStateClass;
> +
> +#define TYPE_DBUS_VMSTATE "dbus-vmstate"
> +#define DBUS_VMSTATE(obj)\
> +OBJECT_CHECK(DBusVMState, (obj), TYPE_DBUS_VMSTATE)
> +#define DBUS_VMSTATE_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(DBusVMStateClass, (obj), TYPE_DBUS_VMSTATE)
> +#define DBUS_VMSTATE_CLASS(klass)\
> +OBJECT_CLASS_CHECK(DBusVMStateClass, (klass), TYPE_DBUS_VMSTATE)
> +
> +struct DBusVMStateClass {
> +ObjectClass parent_class;
> +};
> +
> +struct DBusVMState {
> +Object parent;
> +
> +GDBusConnection *bus;
> +char *dbus_addr;
> +char *id_list;
> +
> +uint32_t data_size;
> +uint8_t *data;
> +};
> +
> +static const GDBusPropertyInfo vmstate_property_info[] = {
> +{ -1, (char *) "Id", (char *) "s",
> + G_DBUS_PROPERTY_INFO_FLAGS_READABLE, NULL },
> +};
> +
> +static const GDBusPropertyInfo * const vmstate_property_info_pointers[] = {
> +&vmstate_property_info[0],
> +NULL
> +};
> +
> +static const GDBusInterfaceInfo vmstate1_interface_info = {
> +-1,
> +(char *) "org.qemu.VMState1",
> +(GDBusMethodInfo **) NULL,
> +(GDBusSignalInfo **) NULL,
> +(GDBusPropertyInfo **) &vmstate_property_info_pointers,
> +NULL,
> +};
> +
> +#define DBUS_VMSTATE_SIZE_LIMIT (1 * MiB)
> +
> +static char **
> +dbus_get_vmstate1_names(DBusVMState *self, GError **err)
> +{
> +g_autoptr(GDBusProxy) proxy = NULL;
> +g_autoptr(GVariant) result = NULL;
> +g_autoptr(GVariant) child = NULL;
> +
> +proxy = g_dbus_proxy_new_sync(self->bus, G_DBUS_PROXY_FLAGS_NONE, NULL,
> + "org.freedesktop.DBus",
> + "/org/freedesktop/DBus",
> + "org.freedesktop.DBus",
> + NULL, err);
> +if (!proxy) {
> +return NULL;
> +}
> +
> +result = g_dbus_proxy_call_sync(proxy, "ListQueuedOwners",
> +g_variant_new("(s)",
> "org.qemu.VMState1"),
> +G_DBUS_CALL_FLAGS_NO_AUTO_START,
> +-1, NULL, err);
Re: [Qemu-devel] [PULL 3/8] m68k: Add NeXTcube machine
On Sat, 7 Sep 2019 at 16:47, Thomas Huth wrote:
>
> It is still quite incomplete (no SCSI, no floppy emulation, no network,
> etc.), but the firmware already shows up the debug monitor prompt in the
> framebuffer display, so at least the very basics are already working.
>
> This code has been taken from Bryce Lanham's GSoC 2011 NeXT branch at
>
> https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-cube.c
>
> and altered quite a bit to fit the latest interface and coding conventions
> of the current QEMU.
>
> Tested-by: Philippe Mathieu-Daudé
> Message-Id: <20190831074519.32613-4-h...@tuxfamily.org>
> Signed-off-by: Thomas Huth +static void nextscr2_write(NeXTState *s, uint32_t val, int size)
> +{
> +static int led;
> +static int phase;
> +static uint8_t old_scr2;
> +static uint8_t rtc_command;
> +static uint8_t rtc_value;
> +static uint8_t rtc_status = 0x90;
> +static uint8_t rtc_return;
> +uint8_t scr2_2;
> +
> +/* read the status 0x31 */
> +if (rtc_command == 0x31) {
> +scr2_2 = scr2_2 & (~SCR2_RTDATA);
> +/* for now 0x00 */
> +if (0x00 & (0x80 >> (phase - 8))) {
0 & anything can never be true, so the line below here is dead code.
> +scr2_2 |= SCR2_RTDATA;
> +}
> +rtc_return = (rtc_return << 1) |
> + ((scr2_2 & SCR2_RTDATA) ? 1 : 0);
> +}
> +
Incidentally, I see that this file has quite a lot of
what seems to be essentially device emulation code in it
(a bunch of IO MemoryRegions defined locally) -- ideally
these could be split out into proper device objects at
some point.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 5/6] docs: start a document to describe D-Bus usage
Hi On Mon, Sep 16, 2019 at 2:02 PM Dr. David Alan Gilbert wrote: > > (Copying in Stefan since he was looking at DBus for virtiofs) > > * Marc-André Lureau (marcandre.lur...@redhat.com) wrote: > > Signed-off-by: Marc-André Lureau > > --- > > docs/interop/dbus.rst | 73 ++ > > docs/interop/index.rst | 1 + > > 2 files changed, 74 insertions(+) > > create mode 100644 docs/interop/dbus.rst > > > > diff --git a/docs/interop/dbus.rst b/docs/interop/dbus.rst > > new file mode 100644 > > index 00..c08f026edc > > --- /dev/null > > +++ b/docs/interop/dbus.rst > > @@ -0,0 +1,73 @@ > > += > > +D-Bus > > += > > + > > +Introduction > > + > > + > > +QEMU may be running with various helper processes involved: > > + - vhost-user* processes (gpu, virtfs, input, etc...) > > + - TPM emulation (or other devices) > > + - user networking (slirp) > > + - network services (DHCP/DNS, samba/ftp etc) > > + - background tasks (compression, streaming etc) > > + - client UI > > + - admin & cli > > + > > +Having several processes allows stricter security rules, as well as > > +greater modularity. > > + > > +While QEMU itself uses QMP as primary IPC (and Spice/VNC for remote > > +display), D-Bus is the de facto IPC of choice on Unix systems. The > > +wire format is machine friendly, good bindings exist for various > > +languages, and there are various tools available. > > + > > +Using a bus, helper processes can discover and communicate with each > > +other easily, without going through QEMU. The bus topology is also > > +easier to apprehend and debug than a mesh. However, it is wise to > > +consider the security aspects of it. > > + > > +Security > > + > > + > > +A QEMU D-Bus bus should be private to a single VM. Thus, only > > +cooperative tasks are running on the same bus to serve the VM. > > + > > +D-Bus, the protocol and standard, doesn't have mechanisms to enforce > > +security between peers once the connection is established. Peers may > > +have additional mechanisms to enforce security rules, based for > > +example on UNIX credentials. > > + > > +dbus-daemon can enforce various policies based on the UID/GID of the > > +processes that are connected to it. It is thus a good idea to run > > +helpers as different UID from QEMU and set appropriate policies (so > > +helper processes are only allowed to talk to qemu for example). > > + > > +For example, this allows only ``qemu`` user to talk to ``qemu-helper`` > > +``org.qemu.Helper1`` service: > > + > > +.. code:: xml > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > +dbus-daemon can also perfom SELinux checks based on the security > > +context of the source and the target. For example, ``virtiofs_t`` > > +could be allowed to send a message to ``svirt_t``, but ``virtiofs_t`` > > +wouldn't be allowed to send a message to ``virtiofs_t``. > > I think we need to start thinking about this more now rather than > 'can'. . Do you have a specific question we can answer or guide for qemu? Is there something we have to document or implement? Since qemu is not managing the extra processes or applying policies, I don't know what else could be done. From qemu pov, it can rely on management layer to trust the bus and the helpers, similar to trusting the system in general. > Dave > > > +Guidelines > > +== > > + > > +When implementing new D-Bus interfaces, it is recommended to follow > > +the "D-Bus API Design Guidelines": > > +https://dbus.freedesktop.org/doc/dbus-api-design.html > > + > > +The "org.qemu*" prefix is reserved for the QEMU project. > > diff --git a/docs/interop/index.rst b/docs/interop/index.rst > > index b4bfcab417..fa4478ce2e 100644 > > --- a/docs/interop/index.rst > > +++ b/docs/interop/index.rst > > @@ -13,6 +13,7 @@ Contents: > > :maxdepth: 2 > > > > bitmaps > > + dbus > > live-block-operations > > pr-helper > > vhost-user > > -- > > 2.23.0 > > > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > -- Marc-André Lureau
Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare
Kevin Wolf writes: > Am 13.09.2019 um 21:54 hat John Snow geschrieben: >> >> >> On 9/13/19 11:25 AM, Sergio Lopez wrote: >> > do_drive_backup() already acquires the AioContext, so release it >> > before the call. >> > >> > Signed-off-by: Sergio Lopez >> > --- >> > blockdev.c | 6 +- >> > 1 file changed, 1 insertion(+), 5 deletions(-) >> > >> > diff --git a/blockdev.c b/blockdev.c >> > index fbef6845c8..3927fdab80 100644 >> > --- a/blockdev.c >> > +++ b/blockdev.c >> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState >> > *common, Error **errp) >> > >> > aio_context = bdrv_get_aio_context(bs); >> > aio_context_acquire(aio_context); >> > - > > Are you removing this unrelated empty line intentionally? Yes. In the sense of that whole set of lines being a "open drained section" block. >> > /* Paired with .clean() */ >> > bdrv_drained_begin(bs); >> >> Do we need to make this change to blockdev_backup_prepare as well? > > Actually, the whole structure feels a bit wrong. We get the bs here and > take its lock, then release the lock again and forget the reference, > only to do both things again inside do_drive_backup(). > > The way snapshots work is that the "normal" snapshot commands are > wrappers that turn it into a single-entry transaction. Then you have > only one code path where you can resolve the ID and take the lock just > once. So maybe backup should work like this, too? I'm neither opposed nor in favor, but I think this is outside the scope of this patch series. Sergio. signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare
John Snow writes:
> On 9/13/19 11:25 AM, Sergio Lopez wrote:
>> do_drive_backup() already acquires the AioContext, so release it
>> before the call.
>>
>> Signed-off-by: Sergio Lopez
>> ---
>> blockdev.c | 6 +-
>> 1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index fbef6845c8..3927fdab80 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState
>> *common, Error **errp)
>>
>> aio_context = bdrv_get_aio_context(bs);
>> aio_context_acquire(aio_context);
>> -
>> /* Paired with .clean() */
>> bdrv_drained_begin(bs);
>
> Do we need to make this change to blockdev_backup_prepare as well?
Yes, we do. Thanks.
>> +aio_context_release(aio_context);
>>
>> state->bs = bs;
>>
>> state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> -goto out;
>> }
>> -
>> -out:
>> -aio_context_release(aio_context);
>> }
>>
>> static void drive_backup_commit(BlkActionState *common)
>>
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v3] virtio-blk: schedule virtio_notify_config to run on main context
virtio_notify_config() needs to acquire the global mutex, which isn't
allowed from an iothread, and may lead to a deadlock like this:
- main thead
* Has acquired: qemu_global_mutex.
* Is trying the acquire: iothread AioContext lock via
AIO_WAIT_WHILE (after aio_poll).
- iothread
* Has acquired: AioContext lock.
* Is trying to acquire: qemu_global_mutex (via
virtio_notify_config->prepare_mmio_access).
If virtio_blk_resize() is called from an iothread, schedule
virtio_notify_config() to be run in the main context BH.
Signed-off-by: Sergio Lopez
---
Changelog
v3:
- Unconditionally schedule the work to be done in the main context BH
(thanks John Snow and Kevin Wolf).
v2:
- Use aio_bh_schedule_oneshot instead of scheduling a coroutine
(thanks Kevin Wolf).
- Switch from RFC to v2 patch.
---
hw/block/virtio-blk.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 18851601cb..0163285f6f 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -16,6 +16,7 @@
#include "qemu/iov.h"
#include "qemu/module.h"
#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
#include "trace.h"
#include "hw/block/block.h"
#include "hw/qdev-properties.h"
@@ -1086,11 +1087,25 @@ static int virtio_blk_load_device(VirtIODevice *vdev,
QEMUFile *f,
return 0;
}
+static void virtio_resize_cb(void *opaque)
+{
+VirtIODevice *vdev = opaque;
+
+assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+virtio_notify_config(vdev);
+}
+
static void virtio_blk_resize(void *opaque)
{
VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
-virtio_notify_config(vdev);
+/*
+ * virtio_notify_config() needs to acquire the global mutex,
+ * so it can't be called from an iothread. Instead, schedule
+ * it to be run in the main context BH.
+ */
+aio_bh_schedule_oneshot(qemu_get_aio_context(),
+virtio_resize_cb, vdev);
}
static const BlockDevOps virtio_block_ops = {
--
2.21.0
Re: [Qemu-devel] [RFC v2 1/2] docs: vhost-user: add in-band kick/call messages
Hi Michael, I had just wanted to prepare a resend, but > > Hmm I don't like this. I propose that with > > VHOST_USER_PROTOCOL_F_IN_BAND_NOTIFICATIONS > > we just don't allow VHOST_USER_SET_VRING_CALL (if you think it's > > important to allow them, we can say that we do not require them). > > You can't actually skip SET_VRING_CALL, it's necessary to start a vring, > so libvhost-user for example calls dev->iface->queue_set_started() only > in this case. The docs in the "Starting and stopping rings" section also > explain this. [...] > See above. But I guess we could put a flag into bit 9 indicating that > you want to use messages instead of polling or a file descriptor, if you > prefer. Personally, I don't think it matters since right now I can see the in- band notification as being really necessary/useful only for simulation work, and in that case no polling will be doable. If you do think it's important to not make the two mutually exclusive, how would you prefer to have this handled? With a new flag, e.g. in bit 9, indicating "use inband signalling instead of polling or eventfd"? Thanks, johannes
Re: [Qemu-devel] [PATCH v2 20/28] s390x/tcg: OC: Fault-safe handling
On 11.09.19 23:26, Richard Henderson wrote: > On 9/6/19 3:57 AM, David Hildenbrand wrote: >> +srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, ra); >> +desta = access_prepare(env, dest, l, MMU_DATA_STORE, ra); > > We should find a way to perform this in one step. > RWM isn't uncommon... Yes, like converting MMU_* into flags we can then process in probe_access(). > > That said, > Reviewed-by: Richard Henderson > > r~ > -- Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH] configure: Add xkbcommon configure options
On 9/14/19 4:51 PM, James Le Cuirot wrote: > This dependency is currently "automagic", which is bad for distributions. > Fixes: 6a021536e23 Reviewed-by: Philippe Mathieu-Daudé > Signed-off-by: James Le Cuirot > --- > configure | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/configure b/configure > index 30aad233d1..30544f52e6 100755 > --- a/configure > +++ b/configure > @@ -1521,6 +1521,10 @@ for opt do >;; >--disable-libpmem) libpmem=no >;; > + --enable-xkbcommon) xkbcommon=yes > + ;; > + --disable-xkbcommon) xkbcommon=no > + ;; >*) >echo "ERROR: unknown option $opt" >echo "Try '$0 --help' for more information" > @@ -1804,6 +1808,7 @@ disabled with --disable-FEATURE, default is enabled if > available: >capstonecapstone disassembler support >debug-mutex mutex debugging support >libpmem libpmem support > + xkbcommon xkbcommon support > > NOTE: The object files are built at the place where configure is launched > EOF >
Re: [Qemu-devel] [PATCH v3] ui: add an embedded Barrier client
Ping?
Le 06/09/2019 à 10:38, Laurent Vivier a écrit :
> This allows to receive mouse and keyboard events from
> a Barrier server.
>
> This is enabled by adding the following parameter on the
> command line
>
> ... -object input-barrier,id=$id,name=$name ...
>
> Where $name is the name declared in the screens section of barrier.conf
>
> The barrier server (barriers) must be configured and must run on the
> local host.
>
> For instance:
>
> section: screens
> localhost:
> ...
> VM-1:
> ...
> end
>
> section: links
> localhost:
> right = VM-1
> VM-1:
> left = localhost
> end
>
> Then on the QEMU command line:
>
> ... -object input-barrier,id=barrie0,name=VM-1 ...
>
> When the mouse will move out of the screen of the local host on
> the right, the mouse and the keyboard will be grabbed and all
> related events will be send to the guest OS.
>
> This is usefull when qemu is configured without emulated graphic card
> but with a VFIO attached graphic card.
>
> More information about Barrier can be found at:
>
> https://github.com/debauchee/barrier
>
> This avoids to install the Barrier server in the guest OS,
> for instance when it is not supported or during the installation.
>
> Signed-off-by: Laurent Vivier
> ---
>
> Notes:
> v3: remove typo in barrier.txt
>
> v2: use qemu_input_map_xorgkbd_to_qcode[] to convert button field
> use keyboard_layout to convert keys if button field is not available
> fix some command in the protocol
> add ui/input-barrier.h
> rework read/write functions
> add server/port properties
> add display properties
> rework protocol
> allow to reconnect (object_del + object_add)
> add documentation
>
> docs/barrier.txt | 370 ++
> ui/Makefile.objs | 1 +
> ui/input-barrier.c | 750 +
> ui/input-barrier.h | 112 +++
> 4 files changed, 1233 insertions(+)
> create mode 100644 docs/barrier.txt
> create mode 100644 ui/input-barrier.c
> create mode 100644 ui/input-barrier.h
>
> diff --git a/docs/barrier.txt b/docs/barrier.txt
> new file mode 100644
> index ..b21d15015d96
> --- /dev/null
> +++ b/docs/barrier.txt
> @@ -0,0 +1,370 @@
> +QEMU Barrier Client
> +
> +
> +* About
> +
> +Barrier is a KVM (Keyboard-Video-Mouse) software forked from Symless's
> +synergy 1.9 codebase.
> +
> +See https://github.com/debauchee/barrier
> +
> +* QEMU usage
> +
> +Generally, mouse and keyboard are grabbed through the QEMU video
> +interface emulation.
> +
> +But when we want to use a video graphic adapter via a PCI passthrough
> +there is no way to provide the keyboard and mouse inputs to the VM
> +except by plugging a second set of mouse and keyboard to the host
> +or by installing a KVM software in the guest OS.
> +
> +The QEMU Barrier client avoids this by implementing directly the Barrier
> +protocol into QEMU.
> +
> +This protocol is enabled by adding an input-barrier object to QEMU.
> +
> +Syntax: input-barrier,id=,name=
> +[,server=][,port=]
> +[,x-origin=][,y-origin=]
> +[,width=][,height=]
> +
> +The object can be added on the QEMU command line, for instance with:
> +
> +... -object input-barrier,id=barrier0,name=VM-1 ...
> +
> +where VM-1 is the name the display configured int the Barrier server
> +on the host providing the mouse and the keyboard events.
> +
> +by default is "localhost", port is 24800,
> + and are set to 0, and to
> +1920 and 1080.
> +
> +If Barrier server is stopped QEMU needs to be reconnected manually,
> +by removing and re-adding the input-barrier object, for instance
> +with the help of the HMP monitor:
> +
> +(qemu) object_del barrier0
> +(qemu) object_add input-barrier,id=barrier0,name=VM-1
> +
> +* Message format
> +
> +Message format between the server and client is in two parts:
> +
> +1- the payload length is a 32bit integer in network endianness,
> +2- the payload
> +
> +The payload starts with a 4byte string (without NUL) which is the
> +command. The first command between the server and the client
> +is the only command not encoded on 4 bytes ("Barrier").
> +The remaining part of the payload is decoded according to the command.
> +
> +* Protocol Description (from barrier/src/lib/barrier/protocol_types.h)
> +
> +- barrierCmdHello "Barrier"
> +
> + Direction: server -> client
> + Parameters: { int16_t minor, int16_t major }
> + Description:
> +
> + Say hello to client
> + minor = protocol major version number supported by server
> + major = protocol minor version number supported by server
> +
> +- barrierCmdHelloBack
Re: [Qemu-devel] [PULL 00/12] target-arm queue
On Fri, 13 Sep 2019 at 16:49, Peter Maydell wrote: > > target-arm queue: mostly aspeed changes from Cédric. > > thanks > -- PMM > > The following changes since commit 85182c96de61f0b600bbe834d5a23e713162e892: > > Merge remote-tracking branch > 'remotes/dgilbert/tags/pull-migration-20190912a' into staging (2019-09-13 > 14:37:48 +0100) > > are available in the Git repository at: > > https://git.linaro.org/people/pmaydell/qemu-arm.git > tags/pull-target-arm-20190913 > > for you to fetch changes up to 27a296fce9821e3608d537756cffa6e43a46df3b: > > qemu-ga: Convert invocation documentation to rST (2019-09-13 16:05:01 +0100) > > > target-arm queue: > * aspeed: add a GPIO controller to the SoC > * aspeed: Various refactorings > * aspeed: Improve DMA controller modelling > * atomic_template: fix indentation in GEN_ATOMIC_HELPER > * qemu-ga: Convert invocation documentation to rST > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2 for any user-visible changes. -- PMM
Re: [Qemu-devel] [PATCH] qga: add command guest-get-devices for reporting VirtIO devices
Hi
On Thu, Aug 29, 2019 at 8:06 PM Tomáš Golembiovský wrote:
>
> Add command for reporting devices on Windows guest. The intent is not so
> much to report the devices but more importantly the driver (and its
> version) that is assigned to the device.
>
> Signed-off-by: Tomáš Golembiovský
> ---
> qga/commands-posix.c | 11 +++
> qga/commands-win32.c | 195 ++-
> qga/qapi-schema.json | 32 +++
> 3 files changed, 237 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index dfc05f5b8a..9adf8bb520 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2709,6 +2709,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>
> return 0;
> }
> +
> #endif /* CONFIG_FSFREEZE */
extra line
>
> #if !defined(CONFIG_FSTRIM)
> @@ -2757,6 +2758,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
> blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
> #endif
>
> +blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
> +
> return blacklist;
> }
>
> @@ -2977,3 +2980,11 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
> return info;
> }
> +
> +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> +{
> +error_setg(errp, QERR_UNSUPPORTED);
> +
> +return NULL;
> +}
> +
extra EOF line
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..0bb93422c7 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -21,10 +21,11 @@
> #ifdef CONFIG_QGA_NTDDSCSI
> #include
> #include
> +#endif
> #include
> #include
> #include
> -#endif
> +#include
> #include
> #include
> #include
> @@ -38,6 +39,36 @@
> #include "qemu/host-utils.h"
> #include "qemu/base64.h"
>
> +
> +/* The following should be in devpkey.h, but it isn't */
> +DEFINE_DEVPROPKEY(DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5, 0xf1, 0x02,
> +0x60, 0x8c, 0x9e, 0xeb, 0xac, 10); /* DEVPROP_TYPE_STRING */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c, 0x4efd,
> 0x80,
> +0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> +/* DEVPROP_TYPE_STRING_LIST */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d, 0x4094,
> 0xad,
> +0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2); /* DEVPROP_TYPE_FILETIME */
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d, 0x4094,
> +0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
perhaps use a qemu prefix to avoid future clash?
> +/* DEVPROP_TYPE_STRING */
> +/* The following should be in sal.h, but it isn't */
> +#ifndef _Out_writes_bytes_opt_
> +#define _Out_writes_bytes_opt_(s)
> +#endif
It got added in da215fcf5f001d1fdedf82e2f1407e8ff0b6d453
('include/sal: Update sal definitions').
#ifndef protects it, ok
> +/* The following shoud be in cfgmgr32.h, but it isn't */
> +#ifndef CM_Get_DevNode_Property
> +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> +_In_ DEVINST dnDevInst,
> +_In_ CONST DEVPROPKEY* PropertyKey,
> +_Out_ DEVPROPTYPE * PropertyType,
> +_Out_writes_bytes_opt_(*PropertyBufferSize) PBYTE PropertyBuffer,
> +_Inout_ PULONG PropertyBufferSize,
> +_In_ ULONG ulFlags
> +);
> +#define CM_Get_DevNode_Property CM_Get_DevNode_PropertyW
> +#endif
> +
#ifndef should protect it from future clash, ok
Did you open bugs for mingw64 updates?
> +
> #ifndef SHTDN_REASON_FLAG_PLANNED
> #define SHTDN_REASON_FLAG_PLANNED 0x8000
> #endif
> @@ -2234,3 +2265,165 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
> return info;
> }
> +
> +/*
> + * Safely get device property. Returned strings are using wide characters.
> + * Caller is responsible for freeing the buffer.
> + */
> +static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
> +PDEVPROPTYPE propType)
> +{
> +CONFIGRET cr;
> +LPBYTE buffer = NULL;
> +ULONG bufferLen = 0;
> +
> +/* First query for needed space */
> +cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +buffer, &bufferLen, 0);
I think qemu-ga win32 code generally prefers to be explicit about A()
vs W(), call the W function explicitely, remove the generic define.
> +if ((cr != CR_SUCCESS) && (cr != CR_BUFFER_SMALL)) {
This file is not a good example, but in general we avoid the extra
parenthesis. Please drop them.
> +
> +g_debug(
> +"failed to get size of device property, device error code=%lx",
> +cr);
That file uses a mix of slog and g_debug..
I think slog() is higher level and preferable here. At least, try to
make it fit on one line would be nice.
> +return NULL;
> +}
> +buffer = (LPBYTE)g_malloc(bufferLen);
I'd suggest g_malloc0(len+1) to avoid surprises.
Drop the cast.
> +cr = CM_Get_DevNode_Property(devInst, propName, propType,
> +buffer, &bufferLen, 0);
> +if
Re: [Qemu-devel] [PATCH v7 1/3] block/qcow2: Fix corruption introduced by commit 8ac0f15f335
On 15.09.19 22:36, Maxim Levitsky wrote: > This fixes subtle corruption introduced by luks threaded encryption > in commit 8ac0f15f335 > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922 > > The corruption happens when we do a write that >* writes to two or more unallocated clusters at once >* doesn't fully cover the first sector >* doesn't fully cover the last sector >* uses luks encryption > > In this case, when allocating the new clusters we COW both areas > prior to the write and after the write, and we encrypt them. > > The above mentioned commit accidentally made it so we encrypt the > second COW area using the physical cluster offset of the first area. > > The problem is that offset_in_cluster in do_perform_cow_encrypt > can be larger that the cluster size, thus cluster_offset > will no longer point to the start of the cluster at which encrypted > area starts. > > Next patch in this series will refactor the code to avoid all these > assumptions. > > In the bugreport that was triggered by rebasing a luks image to new, > zero filled base, which lot of such writes, and causes some files > with zero areas to contain garbage there instead. > But as described above it can happen elsewhere as well > > > Signed-off-by: Maxim Levitsky > Reviewed-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-cluster.c | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v7 2/3] block/qcow2: refactor encryption code
On 15.09.19 22:36, Maxim Levitsky wrote:
> * Change the qcow2_co_{encrypt|decrypt} to just receive full host and
> guest offsets and use this function directly instead of calling
> do_perform_cow_encrypt (which is removed by that patch).
>
> * Adjust qcow2_co_encdec to take full host and guest offsets as well.
>
> * Document the qcow2_co_{encrypt|decrypt} arguments
> to prevent the bug fixed in former commit from hopefully
> happening again.
>
> Signed-off-by: Maxim Levitsky
> ---
> block/qcow2-cluster.c | 41 ++--
> block/qcow2-threads.c | 63 +--
> block/qcow2.c | 5 ++--
> block/qcow2.h | 8 +++---
> 4 files changed, 70 insertions(+), 47 deletions(-)
Looks good to me, but there are conflicts with two series I’ve already
taken to my branch: Namely “qcow2: async handling of fragmented io” by
Vladimir and “Alignment checks cleanup” by Nir.
Unfortunately, those conflicts (while not difficult to resolve) are not
quite trivial, so I’d rather not resolve them myself.
I’ll send a pull request today, but until they are in master, you could
rebase on my branch (either of
- https://git.xanclic.moe/XanClic/qemu.git block
- https://github.com/XanClic/qemu.git block
, whichever you prefer that works).
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v7 3/3] qemu-iotests: Add test for bz #1745922
On 15.09.19 22:36, Maxim Levitsky wrote: > Signed-off-by: Maxim Levitsky > Tested-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/263 | 91 ++ > tests/qemu-iotests/263.out | 40 + > tests/qemu-iotests/group | 1 + > 3 files changed, 132 insertions(+) > create mode 100755 tests/qemu-iotests/263 > create mode 100644 tests/qemu-iotests/263.out Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 5/6] docs: start a document to describe D-Bus usage
* Marc-André Lureau (marcandre.lur...@gmail.com) wrote: > Hi > > On Mon, Sep 16, 2019 at 2:02 PM Dr. David Alan Gilbert > wrote: > > > > (Copying in Stefan since he was looking at DBus for virtiofs) > > > > * Marc-André Lureau (marcandre.lur...@redhat.com) wrote: > > > Signed-off-by: Marc-André Lureau > > > --- > > > docs/interop/dbus.rst | 73 ++ > > > docs/interop/index.rst | 1 + > > > 2 files changed, 74 insertions(+) > > > create mode 100644 docs/interop/dbus.rst > > > > > > diff --git a/docs/interop/dbus.rst b/docs/interop/dbus.rst > > > new file mode 100644 > > > index 00..c08f026edc > > > --- /dev/null > > > +++ b/docs/interop/dbus.rst > > > @@ -0,0 +1,73 @@ > > > += > > > +D-Bus > > > += > > > + > > > +Introduction > > > + > > > + > > > +QEMU may be running with various helper processes involved: > > > + - vhost-user* processes (gpu, virtfs, input, etc...) > > > + - TPM emulation (or other devices) > > > + - user networking (slirp) > > > + - network services (DHCP/DNS, samba/ftp etc) > > > + - background tasks (compression, streaming etc) > > > + - client UI > > > + - admin & cli > > > + > > > +Having several processes allows stricter security rules, as well as > > > +greater modularity. > > > + > > > +While QEMU itself uses QMP as primary IPC (and Spice/VNC for remote > > > +display), D-Bus is the de facto IPC of choice on Unix systems. The > > > +wire format is machine friendly, good bindings exist for various > > > +languages, and there are various tools available. > > > + > > > +Using a bus, helper processes can discover and communicate with each > > > +other easily, without going through QEMU. The bus topology is also > > > +easier to apprehend and debug than a mesh. However, it is wise to > > > +consider the security aspects of it. > > > + > > > +Security > > > + > > > + > > > +A QEMU D-Bus bus should be private to a single VM. Thus, only > > > +cooperative tasks are running on the same bus to serve the VM. > > > + > > > +D-Bus, the protocol and standard, doesn't have mechanisms to enforce > > > +security between peers once the connection is established. Peers may > > > +have additional mechanisms to enforce security rules, based for > > > +example on UNIX credentials. > > > + > > > +dbus-daemon can enforce various policies based on the UID/GID of the > > > +processes that are connected to it. It is thus a good idea to run > > > +helpers as different UID from QEMU and set appropriate policies (so > > > +helper processes are only allowed to talk to qemu for example). > > > + > > > +For example, this allows only ``qemu`` user to talk to ``qemu-helper`` > > > +``org.qemu.Helper1`` service: > > > + > > > +.. code:: xml > > > + > > > + > > > + > > > + > > > + > > > + > > > + > > > + > > > + > > > + > > > + > > > +dbus-daemon can also perfom SELinux checks based on the security > > > +context of the source and the target. For example, ``virtiofs_t`` > > > +could be allowed to send a message to ``svirt_t``, but ``virtiofs_t`` > > > +wouldn't be allowed to send a message to ``virtiofs_t``. > > > > I think we need to start thinking about this more now rather than > > 'can'. . > > Do you have a specific question we can answer or guide for qemu? Is > there something we have to document or implement? > > Since qemu is not managing the extra processes or applying policies, I > don't know what else could be done. From qemu pov, it can rely on > management layer to trust the bus and the helpers, similar to trusting > the system in general. Well pretty much the same questions I asked in the discussion on v2; what is the supported configuration to ensure that one helper that's been compromised can't attack the others and qemu? Dave > > Dave > > > > > +Guidelines > > > +== > > > + > > > +When implementing new D-Bus interfaces, it is recommended to follow > > > +the "D-Bus API Design Guidelines": > > > +https://dbus.freedesktop.org/doc/dbus-api-design.html > > > + > > > +The "org.qemu*" prefix is reserved for the QEMU project. > > > diff --git a/docs/interop/index.rst b/docs/interop/index.rst > > > index b4bfcab417..fa4478ce2e 100644 > > > --- a/docs/interop/index.rst > > > +++ b/docs/interop/index.rst > > > @@ -13,6 +13,7 @@ Contents: > > > :maxdepth: 2 > > > > > > bitmaps > > > + dbus > > > live-block-operations > > > pr-helper > > > vhost-user > > > -- > > > 2.23.0 > > > > > -- > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > > > > > -- > Marc-André Lureau -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.2 v5 1/2] kvm: s390: split too big memory section on several memslots
On Tue, 3 Sep 2019 08:57:38 +0200
Christian Borntraeger wrote:
> On 02.09.19 15:49, Igor Mammedov wrote:
> > On Fri, 30 Aug 2019 18:19:29 +0200
> > Christian Borntraeger wrote:
> >
> >> On 30.08.19 11:41, Igor Mammedov wrote:
> >>> On Thu, 29 Aug 2019 14:41:13 +0200
> >>> Christian Borntraeger wrote:
> >>>
> On 29.08.19 14:31, Igor Mammedov wrote:
> > On Thu, 29 Aug 2019 14:07:44 +0200
> > Christian Borntraeger wrote:
> >
> >> On 29.08.19 14:04, Igor Mammedov wrote:
> >>> On Thu, 29 Aug 2019 08:47:49 +0200
> >>> Christian Borntraeger wrote:
> >>>
> On 27.08.19 14:56, Igor Mammedov wrote:
> > On Tue, 20 Aug 2019 18:07:27 +0200
> > Cornelia Huck wrote:
> >
> >> On Wed, 7 Aug 2019 11:32:41 -0400
> >> Igor Mammedov wrote:
> >>
> >>> Max memslot size supported by kvm on s390 is 8Tb,
> >>> move logic of splitting RAM in chunks upto 8T to KVM code.
> >>>
> >>> This way it will hide KVM specific restrictions in KVM code
> >>> and won't affect baord level design decisions. Which would allow
> >>> us to avoid misusing memory_region_allocate_system_memory() API
> >>> and eventually use a single hostmem backend for guest RAM.
> >>>
> >>> Signed-off-by: Igor Mammedov
> >>> ---
> >>> v5:
> >>> * move computation 'size -= slot_size' inside of loop body
> >>> (David Hildenbrand )
> >>> v4:
> >>> * fix compilation issue
> >>> (Christian Borntraeger )
> >>> * advance HVA along with GPA in kvm_set_phys_mem()
> >>> (Christian Borntraeger )
> >>>
> >>> patch prepares only KVM side for switching to single RAM memory
> >>> region
> >>> another patch will take care of dropping manual RAM partitioning
> >>> in
> >>> s390 code.
> >>
> >> I may have lost track a bit -- what is the status of this patch
> >> (and
> >> the series)?
> >
> > Christian,
> >
> > could you test it on a host that have sufficient amount of RAM?
> >
>
>
> This version looks good. I was able to start a 9TB guest.
> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0,
> guest_phys_addr=0, memory_size=8796091973632,
> userspace_addr=0x3ffee70}) = 0
> [pid 215723] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=1, flags=0,
> guest_phys_addr=0x7f0, memory_size=1099512676352,
> userspace_addr=0xbffee60}) = 0
> >>
> The only question is if we want to fix the weird alignment
> (0x7f0) when
> we already add a migration barrier for uber-large guests.
> Maybe we could split at 4TB to avoid future problem with larger page
> sizes?
> >>> That probably should be a separate patch on top.
> >>
> >> Right. The split in KVM code is transparent to migration and other
> >> parts of QEMU, correct?
> >
> > it should not affect other QEMU parts and migration (to my limited
> > understanding of it),
> > we are passing to KVM memory slots upto KVM_SLOT_MAX_BYTES as we were
> > doing before by
> > creating several memory regions instead of one as described in [2/2]
> > commit message.
> >
> > Also could you also test migration of +9Tb guest, to check that nothing
> > where broken by
> > accident in QEMU migration code?
>
> I only have one server that is large enough :-/
> >>> Could you test offline migration on it (to a file and restore from it)?
> >>>
> >>
> >> I tested migration with a hacked QEMU (basically split in KVM code at 1GB
> >> instead of 8TB) and
> >> the restore from file failed with data corruption in the guest. The
> >> current code
> >> does work when I use small memslots. No idea yet what is wrong.
> >
> > I've tested 2Gb (max, I can test) guest (also hacked up version)
> > and it worked for me.
> > How do you test it and detect corruption so I could try to reproduce it
> > locally?
> > (given it worked before, there is no much hope but I could try)
>
> I basically started a guest with just kernel and ramdisk on the command line
> and
> then in the monitor I did
> migrate "exec: cat > savefile"
> and then I restarted the guest with
> -incoming "exec: cat savefile"
>
> the guest then very quickly crashed with random kernel oopses.
Well, I wasn't able to reproduce that. So I've looked at the kvm part of
migration and it turned out migration didn't even start as KVM was
concerned.
Issue was in that migration related kvm code parts assumed 1:1
relation between MemorySection a
[Qemu-devel] [PATCH v6 0/2] s390: stop abusing memory_region_allocate_system_memory()
Changelog: since v5: - [1/2] fix migration that wasn't starting and make sure that KVM part is able to handle 1:n MemorySection:memslot arrangement since v3: - fix compilation issue - advance HVA along with GPA in kvm_set_phys_mem() since v2: - break migration from old QEMU (since 2.12-4.1) for guest with >8TB RAM and drop migratable aliases patch as was agreed during v2 review - drop 4.2 machines patch as it's not prerequisite anymore since v1: - include 4.2 machines patch for adding compat RAM layout on top - 2/4 add missing in v1 patch for splitting too big MemorySection on several memslots - 3/4 amend code path on alias destruction to ensure that RAMBlock is cleaned properly - 4/4 add compat machine code to keep old layout (migration-wise) for 4.1 and older machines While looking into unifying guest RAM allocation to use hostmem backends for initial RAM (especially when -mempath is used) and retiring memory_region_allocate_system_memory() API, leaving only single hostmem backend, I was inspecting how currently it is used by boards and it turns out several boards abuse it by calling the function several times (despite documented contract forbiding it). s390 is one of such boards where KVM limitation on memslot size got propagated to board design and memory_region_allocate_system_memory() was abused to satisfy KVM requirement for max RAM chunk where memory region alias would suffice. Unfortunately, memory_region_allocate_system_memory() usage created migration dependency where guest RAM is transferred in migration stream as several RAMBlocks if it's more than KVM_SLOT_MAX_BYTES. During v2 review it was agreed to ignore migration breakage (documenting it in release notes) and leaving only KVM fix. In order to replace these several RAM chunks with a single memdev and keep it working with KVM memslot size limit, following was done: * [1/2] split too big RAM chunk inside of KVM code on several memory slots if necessary * [2/2] drop manual ram splitting in s390 code Igor Mammedov (2): kvm: s390: split too big memory section on several memslots s390: do not call memory_region_allocate_system_memory() multiple times include/sysemu/kvm_int.h | 1 + accel/kvm/kvm-all.c| 239 ++--- hw/s390x/s390-virtio-ccw.c | 30 + target/s390x/kvm.c | 12 ++ 4 files changed, 162 insertions(+), 120 deletions(-) -- 2.18.1
[Qemu-devel] [PATCH v6 1/2] kvm: s390: split too big memory section on several memslots
Max memslot size supported by kvm on s390 is 8Tb,
move logic of splitting RAM in chunks upto 8T to KVM code.
This way it will hide KVM specific restrictions in KVM code
and won't affect board level design decisions. Which would allow
us to avoid misusing memory_region_allocate_system_memory() API
and eventually use a single hostmem backend for guest RAM.
Signed-off-by: Igor Mammedov
---
Since there is quite a bit of migration related code churn in the patch
CCing migration maintainer
CC: dgilb...@redhat.com
v6:
* KVM's migration code was assuming 1:1 relation between
memslot and MemorySection, which becomes not true fo s390x
with this patch. As result migration was broken and
dirty logging wasn't even started with when a MemorySection
was split on several memslots. Amend related KVM dirty
log tracking code to account for split MemorySection.
v5:
* move computation 'size -= slot_size' inside of loop body
(David Hildenbrand )
v4:
* fix compilation issue
(Christian Borntraeger )
* advance HVA along with GPA in kvm_set_phys_mem()
(Christian Borntraeger )
patch prepares only KVM side for switching to single RAM memory region
another patch will take care of dropping manual RAM partitioning in
s390 code.
include/sysemu/kvm_int.h | 1 +
accel/kvm/kvm-all.c| 239 ++---
hw/s390x/s390-virtio-ccw.c | 9 --
target/s390x/kvm.c | 12 ++
4 files changed, 159 insertions(+), 102 deletions(-)
diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index 72b2d1b3ae..ac2d1f8b56 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -41,4 +41,5 @@ typedef struct KVMMemoryListener {
void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
AddressSpace *as, int as_id);
+void kvm_set_max_memslot_size(hwaddr max_slot_size);
#endif
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b09bad0804..0635903408 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -140,6 +140,7 @@ bool kvm_direct_msi_allowed;
bool kvm_ioeventfd_any_length_allowed;
bool kvm_msi_use_devid;
static bool kvm_immediate_exit;
+static hwaddr kvm_max_slot_size = ~0;
static const KVMCapabilityInfo kvm_required_capabilites[] = {
KVM_CAP_INFO(USER_MEMORY),
@@ -437,7 +438,7 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml,
KVMSlot *mem,
static int kvm_section_update_flags(KVMMemoryListener *kml,
MemoryRegionSection *section)
{
-hwaddr start_addr, size;
+hwaddr start_addr, size, slot_size;
KVMSlot *mem;
int ret = 0;
@@ -448,13 +449,18 @@ static int kvm_section_update_flags(KVMMemoryListener
*kml,
kvm_slots_lock(kml);
-mem = kvm_lookup_matching_slot(kml, start_addr, size);
-if (!mem) {
-/* We don't have a slot if we want to trap every access. */
-goto out;
-}
+while (size && !ret) {
+slot_size = kvm_max_slot_size < size ? kvm_max_slot_size : size;
+mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
+if (!mem) {
+/* We don't have a slot if we want to trap every access. */
+goto out;
+}
-ret = kvm_slot_update_flags(kml, mem, section->mr);
+ret = kvm_slot_update_flags(kml, mem, section->mr);
+start_addr += slot_size;
+size -= slot_size;
+}
out:
kvm_slots_unlock(kml);
@@ -497,11 +503,14 @@ static void kvm_log_stop(MemoryListener *listener,
/* get kvm's dirty pages bitmap and update qemu's */
static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
+ hwaddr offset_in_section,
+ hwaddr range_size,
unsigned long *bitmap)
{
ram_addr_t start = section->offset_within_region +
+ offset_in_section +
memory_region_get_ram_addr(section->mr);
-ram_addr_t pages = int128_get64(section->size) / getpagesize();
+ram_addr_t pages = range_size / getpagesize();
cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
return 0;
@@ -527,11 +536,13 @@ static int
kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
struct kvm_dirty_log d = {};
KVMSlot *mem;
hwaddr start_addr, size;
+hwaddr slot_size, slot_offset = 0;
int ret = 0;
size = kvm_align_section(section, &start_addr);
-if (size) {
-mem = kvm_lookup_matching_slot(kml, start_addr, size);
+while (size) {
+slot_size = kvm_max_slot_size < size ? kvm_max_slot_size : size;
+mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
if (!mem) {
/* We don't have a slot if we want to trap every access. */
goto out;
@@ -549,11 +560,11 @@ static int
kvm_physical_sync_dirty_bitmap(KVMMemoryListe
[Qemu-devel] [PATCH v6 2/2] s390: do not call memory_region_allocate_system_memory() multiple times
s390 was trying to solve limited KVM memslot size issue by abusing
memory_region_allocate_system_memory(), which breaks API contract
where the function might be called only once.
Beside an invalid use of API, the approach also introduced migration
issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
migration stream as separate RAMBlocks.
After discussion [1], it was agreed to break migration from older
QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
and considered to be not actually used downstream).
Migration should keep working for guests with less than 8TB and for
more than 8TB with QEMU 4.2 and newer binary.
In case user tries to migrate more than 8TB guest, between incompatible
QEMU versions, migration should fail gracefully due to non-exiting
RAMBlock ID or RAMBlock size mismatch.
Taking in account above and that now KVM code is able to split too
big MemorySection into several memslots, stop abusing
memory_region_allocate_system_memory()
and use only one memory region for RAM.
1) [PATCH RFC v2 4/4] s390: do not call memory_region_allocate_system_memory()
multiple times
Signed-off-by: Igor Mammedov
Reviewed-by: David Hildenbrand
---
v3:
- drop migration compat code
PS:
I don't have access to a suitable system to test it.
---
hw/s390x/s390-virtio-ccw.c | 21 +++--
1 file changed, 3 insertions(+), 18 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index baa95aad38..18ad279a00 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -157,27 +157,12 @@ static void virtio_ccw_register_hcalls(void)
static void s390_memory_init(ram_addr_t mem_size)
{
MemoryRegion *sysmem = get_system_memory();
-ram_addr_t chunk, offset = 0;
-unsigned int number = 0;
+MemoryRegion *ram = g_new(MemoryRegion, 1);
Error *local_err = NULL;
-gchar *name;
/* allocate RAM for core */
-name = g_strdup_printf("s390.ram");
-while (mem_size) {
-MemoryRegion *ram = g_new(MemoryRegion, 1);
-uint64_t size = mem_size;
-
-/* KVM does not allow memslots >= 8 TB */
-chunk = MIN(size, KVM_SLOT_MAX_BYTES);
-memory_region_allocate_system_memory(ram, NULL, name, chunk);
-memory_region_add_subregion(sysmem, offset, ram);
-mem_size -= chunk;
-offset += chunk;
-g_free(name);
-name = g_strdup_printf("s390.ram.%u", ++number);
-}
-g_free(name);
+memory_region_allocate_system_memory(ram, NULL, "s390.ram", mem_size);
+memory_region_add_subregion(sysmem, 0, ram);
/*
* Configure the maximum page size. As no memory devices were created
--
2.18.1
Re: [Qemu-devel] [PATCH v4 0/5] qcow2: async handling of fragmented io
On 13.09.19 10:58, Max Reitz wrote: > On 16.08.19 17:30, Vladimir Sementsov-Ogievskiy wrote: >> Hi all! >> >> Here is an asynchronous scheme for handling fragmented qcow2 >> reads and writes. Both qcow2 read and write functions loops through >> sequential portions of data. The series aim it to parallelize these >> loops iterations. >> It improves performance for fragmented qcow2 images, I've tested it >> as described below. > > Thanks, I’ve changed two things: > - Replaced assert((x & (BDRV_SECTOR_SIZE - 1)) == 0) by > assert(QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE)) in patch 3 (conflict with > “block: Use QEMU_IS_ALIGNED”), and > - Replaced the remaining instance of “qcow2_co_do_pwritev()” by > “qcow2_co_pwritev_task()” in a comment in patch 4 > > and applied the series to my block branch: > > https://git.xanclic.moe/XanClic/qemu/commits/branch/block Unfortunately, I’ll have to unstage the series for now because the fix to 026’s reference output isn’t stable. When running the test in parallel (I can reproduce it with four instances on my machine with two cores + HT), I get failures like: 026 fail [15:21:09] [15:21:37] (last: 18s) output mismatch (see 026.out.bad) --- tests/qemu-iotests/026.out 2019-09-16 14:49:20.720410701 +0200 +++ tests/qemu-iotests/026.out.bad 2019-09-16 15:21:37.180711936 +0200 @@ -563,7 +563,7 @@ qemu-io: Failed to flush the refcount block cache: No space left on device write failed: No space left on device -74 leaked clusters were found on the image. +522 leaked clusters were found on the image. This means waste of disk space, but no harm to data. Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 Failures: 026 Failed 1 of 1 iotests Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v6 1/2] kvm: s390: split too big memory section on several memslots
* Igor Mammedov (imamm...@redhat.com) wrote:
> Max memslot size supported by kvm on s390 is 8Tb,
> move logic of splitting RAM in chunks upto 8T to KVM code.
>
> This way it will hide KVM specific restrictions in KVM code
> and won't affect board level design decisions. Which would allow
> us to avoid misusing memory_region_allocate_system_memory() API
> and eventually use a single hostmem backend for guest RAM.
>
> Signed-off-by: Igor Mammedov
> ---
> Since there is quite a bit of migration related code churn in the patch
> CCing migration maintainer
>
> CC: dgilb...@redhat.com
CC'ing in peterx who knows this code better than me.
But if I'm reading it right, then the rest of the migration code can be
blissfully ignorant of these changes.
Dave
>
> v6:
> * KVM's migration code was assuming 1:1 relation between
> memslot and MemorySection, which becomes not true fo s390x
> with this patch. As result migration was broken and
> dirty logging wasn't even started with when a MemorySection
> was split on several memslots. Amend related KVM dirty
> log tracking code to account for split MemorySection.
> v5:
> * move computation 'size -= slot_size' inside of loop body
> (David Hildenbrand )
> v4:
> * fix compilation issue
> (Christian Borntraeger )
> * advance HVA along with GPA in kvm_set_phys_mem()
> (Christian Borntraeger )
>
> patch prepares only KVM side for switching to single RAM memory region
> another patch will take care of dropping manual RAM partitioning in
> s390 code.
>
>
> include/sysemu/kvm_int.h | 1 +
> accel/kvm/kvm-all.c| 239 ++---
> hw/s390x/s390-virtio-ccw.c | 9 --
> target/s390x/kvm.c | 12 ++
> 4 files changed, 159 insertions(+), 102 deletions(-)
>
> diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
> index 72b2d1b3ae..ac2d1f8b56 100644
> --- a/include/sysemu/kvm_int.h
> +++ b/include/sysemu/kvm_int.h
> @@ -41,4 +41,5 @@ typedef struct KVMMemoryListener {
> void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
>AddressSpace *as, int as_id);
>
> +void kvm_set_max_memslot_size(hwaddr max_slot_size);
> #endif
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b09bad0804..0635903408 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -140,6 +140,7 @@ bool kvm_direct_msi_allowed;
> bool kvm_ioeventfd_any_length_allowed;
> bool kvm_msi_use_devid;
> static bool kvm_immediate_exit;
> +static hwaddr kvm_max_slot_size = ~0;
>
> static const KVMCapabilityInfo kvm_required_capabilites[] = {
> KVM_CAP_INFO(USER_MEMORY),
> @@ -437,7 +438,7 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml,
> KVMSlot *mem,
> static int kvm_section_update_flags(KVMMemoryListener *kml,
> MemoryRegionSection *section)
> {
> -hwaddr start_addr, size;
> +hwaddr start_addr, size, slot_size;
> KVMSlot *mem;
> int ret = 0;
>
> @@ -448,13 +449,18 @@ static int kvm_section_update_flags(KVMMemoryListener
> *kml,
>
> kvm_slots_lock(kml);
>
> -mem = kvm_lookup_matching_slot(kml, start_addr, size);
> -if (!mem) {
> -/* We don't have a slot if we want to trap every access. */
> -goto out;
> -}
> +while (size && !ret) {
> +slot_size = kvm_max_slot_size < size ? kvm_max_slot_size : size;
> +mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
> +if (!mem) {
> +/* We don't have a slot if we want to trap every access. */
> +goto out;
> +}
>
> -ret = kvm_slot_update_flags(kml, mem, section->mr);
> +ret = kvm_slot_update_flags(kml, mem, section->mr);
> +start_addr += slot_size;
> +size -= slot_size;
> +}
>
> out:
> kvm_slots_unlock(kml);
> @@ -497,11 +503,14 @@ static void kvm_log_stop(MemoryListener *listener,
>
> /* get kvm's dirty pages bitmap and update qemu's */
> static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
> + hwaddr offset_in_section,
> + hwaddr range_size,
> unsigned long *bitmap)
> {
> ram_addr_t start = section->offset_within_region +
> + offset_in_section +
> memory_region_get_ram_addr(section->mr);
> -ram_addr_t pages = int128_get64(section->size) / getpagesize();
> +ram_addr_t pages = range_size / getpagesize();
>
> cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
> return 0;
> @@ -527,11 +536,13 @@ static int
> kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
> struct kvm_dirty_log d = {};
> KVMSlot *mem;
> hwaddr start_addr, size;
> +hwaddr slot_size, slot_offset = 0;
> int ret = 0;
>
> size = kvm_align_sect
Re: [Qemu-devel] [PULL 0/2] target/hppa updates
On Sun, 15 Sep 2019 at 14:49, Richard Henderson wrote: > > The following changes since commit 85182c96de61f0b600bbe834d5a23e713162e892: > > Merge remote-tracking branch > 'remotes/dgilbert/tags/pull-migration-20190912a' into staging (2019-09-13 > 14:37:48 +0100) > > are available in the Git repository at: > > https://github.com/rth7680/qemu.git tags/pull-hppa-20190915 > > for you to fetch changes up to a6deecce5b11827fff8a3de2142d02c5388aee1c: > > target/hppa: prevent trashing of temporary in do_depw_sar() (2019-09-14 > 15:39:24 -0400) > > > Two temp live across branch fixes. > > > Sven Schnelle (2): > target/hppa: prevent trashing of temporary in trans_mtctl() > target/hppa: prevent trashing of temporary in do_depw_sar() > > target/hppa/translate.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2 for any user-visible changes. -- PMM
Re: [Qemu-devel] [PATCH v7 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335
On 15.09.19 22:36, Maxim Levitsky wrote: > Commit 8ac0f15f335 accidently broke the COW of non changed areas > of newly allocated clusters, when the write spans multiple clusters, > and needs COW both prior and after the write. > This results in 'after' COW area being encrypted with wrong > sector address, which render it corrupted. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922 > > CC: qemu-stable > > V2: grammar, spelling and code style fixes. > V3: more fixes after the review. > V4: addressed review comments from Max Reitz, > and futher refactored the qcow2_co_encrypt to just take full host and > guest offset > which simplifies everything. > > V5: reworked the patches so one of them fixes the bug > only and other one is just refactoring > > V6: removed do_perform_cow_encrypt > > V7: removed do_perform_cow_encrypt take two, this > time I hopefully did that correctly :-) > Also updated commit names and messages a bit Luckily for you (maybe), Vladimir’s series doesn‘t quite pass the iotests for me, so unfortunately (I find it unfortunate) I had to remove it from my branch. Thus, the conflicts are much more tame and I felt comfortable taking the series to my branch (with the remaining trivial conflicts resolved, and with Vladimir’s suggestion applied): https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v3 02/29] s390x/tcg: MVCL: Zero out unused bits of address
We have to zero out unused bits in 24 and 31-bit addressing mode.
Provide a new helper.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 39ee9b3175..b02ad148e5 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -469,6 +469,25 @@ static inline uint64_t get_address(CPUS390XState *env, int
reg)
return wrap_address(env, env->regs[reg]);
}
+/*
+ * Store the address to the given register, zeroing out unused leftmost
+ * bits in bit positions 32-63 (24-bit and 31-bit mode only).
+ */
+static inline void set_address_zero(CPUS390XState *env, int reg,
+uint64_t address)
+{
+if (env->psw.mask & PSW_MASK_64) {
+env->regs[reg] = address;
+} else {
+if (!(env->psw.mask & PSW_MASK_32)) {
+address &= 0x00ff;
+} else {
+address &= 0x7fff;
+}
+env->regs[reg] = deposit64(env->regs[reg], 0, 32, address);
+}
+}
+
static inline void set_address(CPUS390XState *env, int reg, uint64_t address)
{
if (env->psw.mask & PSW_MASK_64) {
@@ -772,8 +791,8 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1,
uint32_t r2)
env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
env->regs[r2 + 1] = deposit64(env->regs[r2 + 1], 0, 24, srclen);
-set_address(env, r1, dest);
-set_address(env, r2, src);
+set_address_zero(env, r1, dest);
+set_address_zero(env, r2, src);
return cc;
}
--
2.21.0
[Qemu-devel] [PATCH v3 04/29] s390x/tcg: MVCL: Process max 4k bytes at a time
Process max 4k bytes at a time, writing back registers between the
accesses. The instruction is interruptible.
"For operands longer than 2K bytes, access exceptions are not
recognized for locations more than 2K bytes beyond the current location
being processed."
Note that on z/Architecture, 2k vs. 4k access cannot get differentiated as
long as pages are not crossed. This seems to be a leftover from ESA/390.
Simply stay within single pages.
MVCL handling is quite different than MVCLE/MVCLU handling, so split up
the handlers.
Defer interrupt handling, as that will require more thought, add a TODO
for that.
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 44 +--
1 file changed, 38 insertions(+), 6 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 223312a4b1..58ab2e48e3 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -798,19 +798,51 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1,
uint32_t r2)
uint64_t srclen = env->regs[r2 + 1] & 0xff;
uint64_t src = get_address(env, r2);
uint8_t pad = env->regs[r2 + 1] >> 24;
-uint32_t cc;
+uint32_t cc, cur_len;
if (is_destructive_overlap(env, dest, src, MIN(srclen, destlen))) {
cc = 3;
+} else if (srclen == destlen) {
+cc = 0;
+} else if (destlen < srclen) {
+cc = 1;
} else {
-cc = do_mvcl(env, &dest, &destlen, &src, &srclen, pad, 1, ra);
+cc = 2;
}
-env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
-env->regs[r2 + 1] = deposit64(env->regs[r2 + 1], 0, 24, srclen);
-set_address_zero(env, r1, dest);
-set_address_zero(env, r2, src);
+/* We might have to zero-out some bits even if there was no action. */
+if (unlikely(!destlen || cc == 3)) {
+set_address_zero(env, r2, src);
+set_address_zero(env, r1, dest);
+return cc;
+} else if (!srclen) {
+set_address_zero(env, r2, src);
+}
+/*
+ * Only perform one type of type of operation (move/pad) in one step.
+ * Stay within single pages.
+ */
+while (destlen) {
+cur_len = MIN(destlen, -(dest | TARGET_PAGE_MASK));
+if (!srclen) {
+fast_memset(env, dest, pad, cur_len, ra);
+} else {
+cur_len = MIN(MIN(srclen, -(src | TARGET_PAGE_MASK)), cur_len);
+
+fast_memmove(env, dest, src, cur_len, ra);
+src = wrap_address(env, src + cur_len);
+srclen -= cur_len;
+env->regs[r2 + 1] = deposit64(env->regs[r2 + 1], 0, 24, srclen);
+set_address_zero(env, r2, src);
+}
+dest = wrap_address(env, dest + cur_len);
+destlen -= cur_len;
+env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
+set_address_zero(env, r1, dest);
+
+/* TODO: Deliver interrupts. */
+}
return cc;
}
--
2.21.0
[Qemu-devel] [PATCH v3 06/29] s390x/tcg: MVC: Use is_destructive_overlap()
Let's use the new helper, that also detects destructive overlaps when
wrapping.
We'll make the remaining code (e.g., fast_memmove()) aware of wrapping
later.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 013e8d6045..c31cf49593 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -330,7 +330,7 @@ static uint32_t do_helper_mvc(CPUS390XState *env, uint32_t
l, uint64_t dest,
*/
if (dest == src + 1) {
fast_memset(env, dest, cpu_ldub_data_ra(env, src, ra), l, ra);
-} else if (dest < src || src + l <= dest) {
+} else if (!is_destructive_overlap(env, dest, src, l)) {
fast_memmove(env, dest, src, l, ra);
} else {
for (i = 0; i < l; i++) {
--
2.21.0
[Qemu-devel] [PATCH v3 01/29] s390x/tcg: Reset exception_index to -1 instead of 0
We use the marker "-1" for "no exception". s390_cpu_do_interrupt() might
get confused by that.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 29fcce426e..39ee9b3175 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1747,7 +1747,7 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1,
uint64_t a2)
if (env->int_pgm_code == PGM_PROTECTION) {
/* retry if reading is possible */
-cs->exception_index = 0;
+cs->exception_index = -1;
if (!s390_cpu_virt_mem_check_read(cpu, a1, 0, 1)) {
/* Fetching permitted; storing not permitted */
return 1;
@@ -1757,7 +1757,7 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1,
uint64_t a2)
switch (env->int_pgm_code) {
case PGM_PROTECTION:
/* Fetching not permitted; storing not permitted */
-cs->exception_index = 0;
+cs->exception_index = -1;
return 2;
case PGM_ADDRESSING:
case PGM_TRANS_SPEC:
@@ -1767,7 +1767,7 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1,
uint64_t a2)
}
/* Translation not available */
-cs->exception_index = 0;
+cs->exception_index = -1;
return 3;
}
--
2.21.0
[Qemu-devel] [PATCH v3 05/29] s390x/tcg: MVC: Increment the length once
Let's increment the length once.
While at it, cleanup the comment. The memset() example is given as a
programming note in the PoP, so drop the description.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 58ab2e48e3..013e8d6045 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -320,16 +320,20 @@ static uint32_t do_helper_mvc(CPUS390XState *env,
uint32_t l, uint64_t dest,
HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n",
__func__, l, dest, src);
-/* mvc and memmove do not behave the same when areas overlap! */
-/* mvc with source pointing to the byte after the destination is the
- same as memset with the first source byte */
+/* MVC always copies one more byte than specified - maximum is 256 */
+l++;
+
+/*
+ * "When the operands overlap, the result is obtained as if the operands
+ * were processed one byte at a time". Only non-destructive overlaps
+ * behave like memmove().
+ */
if (dest == src + 1) {
-fast_memset(env, dest, cpu_ldub_data_ra(env, src, ra), l + 1, ra);
-} else if (dest < src || src + l < dest) {
-fast_memmove(env, dest, src, l + 1, ra);
+fast_memset(env, dest, cpu_ldub_data_ra(env, src, ra), l, ra);
+} else if (dest < src || src + l <= dest) {
+fast_memmove(env, dest, src, l, ra);
} else {
-/* slow version with byte accesses which always work */
-for (i = 0; i <= l; i++) {
+for (i = 0; i < l; i++) {
uint8_t x = cpu_ldub_data_ra(env, src + i, ra);
cpu_stb_data_ra(env, dest + i, x, ra);
}
--
2.21.0
[Qemu-devel] [PATCH v3 00/29] s390x/tcg: mem_helper: Fault-safe handling
This series fixes a bunch of issues related to some mem helpers and makes sure that they are fault-safe, meaning no system state is modified in case a fault is triggered. I can spot tons of other issues with other mem helpers that will have to be fixed later. Also, fault-safe handling for some instructions (especially TR) might be harder to implement (you don't know what will actually be accessed upfront - we might need a buffer and go over inputs twice). Focusing on the MOVE instructions for now. Newer versions of glibc use memcpy() in memmove() for forward moves. The implementation makese use of MVC. The TCG implementation of MVC is currently not able to handle faults reliably when crossing pages. MVC can cross with 256 bytes at most two pages. In case we get a fault on the second page, we already moved data. When continuing after the fault we might try to move already overwritten data, which is very bad in case we have overlapping data on a forward move. Triggered for now only by rpmbuild (crashes when checking the spec file) and rpm (database corruptions). This fixes installing Fedora rawhide (31) under TCG. This was horrible to debug as it barely triggers and we fail at completely different places. Cc: Stefano Brivio Cc: Florian Weimer Cc: Dan Horák Cc: Cole Robinson v2 -> v3: - "s390x/tcg: MVCL: Zero out unused bits of address" -- Do single deposit for 24/31-bit - "s390x/tcg: MVCL: Process max 4k bytes at a time" -- Use max of 4k instead of 2k, limiting to single pages - "s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time" -- Limit to single pages - "s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode" -- Added - "s390x/tcg: MVCS/MVCP: Properly wrap the length" -- Properly use 32 instead of 31 bit. - "s390x/tcg: MVST: Fix storing back the addresses to registers" -- Read R0 implicitly - "s390x/tcg: Fault-safe memset" -- Speed up TLB_NOTDIRTY handling -- Move single-page access to helper function -- Pass access structure to access_memset() -- Replace access_prepare() by previous access_prepare_idx() - "s390x/tcg: Fault-safe memmove" -- Pass access structure to access_memmove() -- Speed up TLB_NOTDIRTY handling when accessing single bytes - The other fault-safe handling patches were adapted to work with the changed access functions. mmu_idx is now always passed to access_prepare() from the helpers. v1 -> v2: - Include many fixes - Fix more instructions - Use the new probe_access() function - Include "tests/tcg: target/s390x: Test MVO" David Hildenbrand (29): s390x/tcg: Reset exception_index to -1 instead of 0 s390x/tcg: MVCL: Zero out unused bits of address s390x/tcg: MVCL: Detect destructive overlaps s390x/tcg: MVCL: Process max 4k bytes at a time s390x/tcg: MVC: Increment the length once s390x/tcg: MVC: Use is_destructive_overlap() s390x/tcg: MVPG: Check for specification exceptions s390x/tcg: MVPG: Properly wrap the addresses s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time s390x/tcg: MVCS/MVCP: Check for special operation exceptions s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode s390x/tcg: MVCS/MVCP: Properly wrap the length s390x/tcg: MVST: Check for specification exceptions s390x/tcg: MVST: Fix storing back the addresses to registers s390x/tcg: Always use MMU_USER_IDX for CONFIG_USER_ONLY s390x/tcg: Fault-safe memset s390x/tcg: Fault-safe memmove s390x/tcg: MVCS/MVCP: Use access_memmove() s390x/tcg: MVC: Fault-safe handling on destructive overlaps s390x/tcg: MVCLU: Fault-safe handling s390x/tcg: OC: Fault-safe handling s390x/tcg: XC: Fault-safe handling s390x/tcg: NC: Fault-safe handling s390x/tcg: MVCIN: Fault-safe handling s390x/tcg: MVN: Fault-safe handling s390x/tcg: MVZ: Fault-safe handling s390x/tcg: MVST: Fault-safe handling s390x/tcg: MVO: Fault-safe handling tests/tcg: target/s390x: Test MVO target/s390x/cpu.h | 4 + target/s390x/helper.h | 2 +- target/s390x/insn-data.def | 2 +- target/s390x/mem_helper.c | 743 ++-- target/s390x/translate.c| 12 +- tests/tcg/s390x/Makefile.target | 1 + tests/tcg/s390x/mvo.c | 25 ++ 7 files changed, 564 insertions(+), 225 deletions(-) create mode 100644 tests/tcg/s390x/mvo.c -- 2.21.0
[Qemu-devel] [PATCH v3 09/29] s390x/tcg: MVCLU/MVCLE: Process max 4k bytes at a time
Let's stay within single pages.
... and indicate cc=3 in case there is work remaining. Keep unicode
padding simple.
While reworking, properly wrap the addresses.
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 54 ++-
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 746f647303..86238e0163 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -768,8 +768,8 @@ static inline uint32_t do_mvcl(CPUS390XState *env,
uint64_t *src, uint64_t *srclen,
uint16_t pad, int wordsize, uintptr_t ra)
{
-uint64_t len = MIN(*srclen, *destlen);
-uint32_t cc;
+int len = MIN(*destlen, -(*dest | TARGET_PAGE_MASK));
+int i, cc;
if (*destlen == *srclen) {
cc = 0;
@@ -779,32 +779,40 @@ static inline uint32_t do_mvcl(CPUS390XState *env,
cc = 2;
}
-/* Copy the src array */
-fast_memmove(env, *dest, *src, len, ra);
-*src += len;
-*srclen -= len;
-*dest += len;
-*destlen -= len;
+if (!*destlen) {
+return cc;
+}
-/* Pad the remaining area */
-if (wordsize == 1) {
-fast_memset(env, *dest, pad, *destlen, ra);
-*dest += *destlen;
-*destlen = 0;
+/*
+ * Only perform one type of type of operation (move/pad) at a time.
+ * Stay within single pages.
+ */
+if (*srclen) {
+/* Copy the src array */
+len = MIN(MIN(*srclen, -(*src | TARGET_PAGE_MASK)), len);
+*destlen -= len;
+*srclen -= len;
+fast_memmove(env, *dest, *src, len, ra);
+*src = wrap_address(env, *src + len);
+*dest = wrap_address(env, *dest + len);
+} else if (wordsize == 1) {
+/* Pad the remaining area */
+*destlen -= len;
+fast_memset(env, *dest, pad, len, ra);
+*dest = wrap_address(env, *dest + len);
} else {
-/* If remaining length is odd, pad with odd byte first. */
-if (*destlen & 1) {
-cpu_stb_data_ra(env, *dest, pad & 0xff, ra);
-*dest += 1;
-*destlen -= 1;
-}
-/* The remaining length is even, pad using words. */
-for (; *destlen; *dest += 2, *destlen -= 2) {
-cpu_stw_data_ra(env, *dest, pad, ra);
+/* The remaining length selects the padding byte. */
+for (i = 0; i < len; (*destlen)--, i++) {
+if (*destlen & 1) {
+cpu_stb_data_ra(env, *dest, pad, ra);
+} else {
+cpu_stb_data_ra(env, *dest, pad >> 8, ra);
+}
+*dest = wrap_address(env, *dest + 1);
}
}
-return cc;
+return *destlen ? 3 : cc;
}
/* move long */
--
2.21.0
[Qemu-devel] [PATCH v3 07/29] s390x/tcg: MVPG: Check for specification exceptions
Perform the checks documented in the PoP.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index c31cf49593..7dfa848744 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -672,6 +672,13 @@ uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c,
uint64_t s1, uint64_t s2)
/* move page */
uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t
r2)
{
+const bool f = extract64(r0, 11, 1);
+const bool s = extract64(r0, 10, 1);
+
+if ((f && s) || extract64(r0, 12, 4)) {
+s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, GETPC());
+}
+
/* ??? missing r0 handling, which includes access keys, but more
importantly optional suppression of the exception! */
fast_memmove(env, r1, r2, TARGET_PAGE_SIZE, GETPC());
--
2.21.0
[Qemu-devel] [PATCH v3 03/29] s390x/tcg: MVCL: Detect destructive overlaps
We'll have to zero-out unused bit positions, so make sure to write the
addresses back.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index b02ad148e5..223312a4b1 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -52,6 +52,19 @@ static inline bool psw_key_valid(CPUS390XState *env, uint8_t
psw_key)
return true;
}
+static bool is_destructive_overlap(CPUS390XState *env, uint64_t dest,
+ uint64_t src, uint32_t len)
+{
+if (!len || src == dest) {
+return false;
+}
+/* Take care of wrapping at the end of address space. */
+if (unlikely(wrap_address(env, src + len - 1) < src)) {
+return dest > src || dest <= wrap_address(env, src + len - 1);
+}
+return dest > src && dest <= src + len - 1;
+}
+
/* Reduce the length so that addr + len doesn't cross a page boundary. */
static inline uint32_t adj_len_to_page(uint32_t len, uint64_t addr)
{
@@ -787,7 +800,11 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1,
uint32_t r2)
uint8_t pad = env->regs[r2 + 1] >> 24;
uint32_t cc;
-cc = do_mvcl(env, &dest, &destlen, &src, &srclen, pad, 1, ra);
+if (is_destructive_overlap(env, dest, src, MIN(srclen, destlen))) {
+cc = 3;
+} else {
+cc = do_mvcl(env, &dest, &destlen, &src, &srclen, pad, 1, ra);
+}
env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
env->regs[r2 + 1] = deposit64(env->regs[r2 + 1], 0, 24, srclen);
--
2.21.0
[Qemu-devel] [PATCH v3 21/29] s390x/tcg: OC: Fault-safe handling
We can process a maximum of 256 bytes, crossing two pages.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 853b9557cf..0574c31d9a 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -383,17 +383,26 @@ uint32_t HELPER(xc)(CPUS390XState *env, uint32_t l,
uint64_t dest,
static uint32_t do_helper_oc(CPUS390XState *env, uint32_t l, uint64_t dest,
uint64_t src, uintptr_t ra)
{
+const int mmu_idx = cpu_mmu_index(env, false);
+S390Access srca1, srca2, desta;
uint32_t i;
uint8_t c = 0;
HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n",
__func__, l, dest, src);
-for (i = 0; i <= l; i++) {
-uint8_t x = cpu_ldub_data_ra(env, src + i, ra);
-x |= cpu_ldub_data_ra(env, dest + i, ra);
+/* OC always processes one more byte than specified - maximum is 256 */
+l++;
+
+srca1 = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
+desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+for (i = 0; i < l; i++) {
+const uint8_t x = access_get_byte(env, &srca1, i, ra) |
+ access_get_byte(env, &srca2, i, ra);
+
c |= x;
-cpu_stb_data_ra(env, dest + i, x, ra);
+access_set_byte(env, &desta, i, x, ra);
}
return c != 0;
}
--
2.21.0
[Qemu-devel] [PATCH v3 15/29] s390x/tcg: Always use MMU_USER_IDX for CONFIG_USER_ONLY
Although we basically ignore the index all the time for CONFIG_USER_ONLY,
let's simply skip all the checks and always return MMU_USER_IDX in
cpu_mmu_index() and get_mem_index().
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/cpu.h | 4
target/s390x/translate.c | 4
2 files changed, 8 insertions(+)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 79202c0980..163dae13d7 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -328,6 +328,9 @@ extern const VMStateDescription vmstate_s390_cpu;
static inline int cpu_mmu_index(CPUS390XState *env, bool ifetch)
{
+#ifdef CONFIG_USER_ONLY
+return MMU_USER_IDX;
+#else
if (!(env->psw.mask & PSW_MASK_DAT)) {
return MMU_REAL_IDX;
}
@@ -351,6 +354,7 @@ static inline int cpu_mmu_index(CPUS390XState *env, bool
ifetch)
default:
abort();
}
+#endif
}
static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index b0a2500e5f..a3e43ff9ec 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -318,6 +318,9 @@ static inline uint64_t ld_code4(CPUS390XState *env,
uint64_t pc)
static int get_mem_index(DisasContext *s)
{
+#ifdef CONFIG_USER_ONLY
+return MMU_USER_IDX;
+#else
if (!(s->base.tb->flags & FLAG_MASK_DAT)) {
return MMU_REAL_IDX;
}
@@ -333,6 +336,7 @@ static int get_mem_index(DisasContext *s)
tcg_abort();
break;
}
+#endif
}
static void gen_exception(int excp)
--
2.21.0
[Qemu-devel] [PATCH v3 11/29] s390x/tcg: MVCOS: Lengths are 32 bit in 24/31-bit mode
Triggered by a review comment from Richard, also MVCOS has a 32-bit
length in 24/31-bit addressing mode. Add a new helper.
Rename wrap_length() to wrap_length31().
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 20e1ac0ea9..320e9ee65c 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -528,7 +528,15 @@ static inline void set_address(CPUS390XState *env, int
reg, uint64_t address)
}
}
-static inline uint64_t wrap_length(CPUS390XState *env, uint64_t length)
+static inline uint64_t wrap_length32(CPUS390XState *env, uint64_t length)
+{
+if (!(env->psw.mask & PSW_MASK_64)) {
+return (uint32_t)length;
+}
+return length;
+}
+
+static inline uint64_t wrap_length31(CPUS390XState *env, uint64_t length)
{
if (!(env->psw.mask & PSW_MASK_64)) {
/* 24-Bit and 31-Bit mode */
@@ -539,7 +547,7 @@ static inline uint64_t wrap_length(CPUS390XState *env,
uint64_t length)
static inline uint64_t get_length(CPUS390XState *env, int reg)
{
-return wrap_length(env, env->regs[reg]);
+return wrap_length31(env, env->regs[reg]);
}
static inline void set_length(CPUS390XState *env, int reg, uint64_t length)
@@ -2378,7 +2386,7 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest,
uint64_t src,
s390_program_interrupt(env, PGM_PRIVILEGED, 6, ra);
}
-len = wrap_length(env, len);
+len = wrap_length32(env, len);
if (len > 4096) {
cc = 3;
len = 4096;
--
2.21.0
[Qemu-devel] [PATCH v3 08/29] s390x/tcg: MVPG: Properly wrap the addresses
We have to mask of any unused bits. While at it, document what exactly is missing. Reviewed-by: Richard Henderson Signed-off-by: David Hildenbrand --- target/s390x/mem_helper.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 7dfa848744..746f647303 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -679,8 +679,15 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2) s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, GETPC()); } -/* ??? missing r0 handling, which includes access keys, but more - importantly optional suppression of the exception! */ +r1 = wrap_address(env, r1 & TARGET_PAGE_MASK); +r2 = wrap_address(env, r2 & TARGET_PAGE_MASK); + +/* + * TODO: + * - Access key handling + * - CC-option with surpression of page-translation exceptions + * - Store r1/r2 register identifiers at real location 162 + */ fast_memmove(env, r1, r2, TARGET_PAGE_SIZE, GETPC()); return 0; /* data moved */ } -- 2.21.0
[Qemu-devel] [PATCH v3 18/29] s390x/tcg: MVCS/MVCP: Use access_memmove()
As we are moving between address spaces, we can use access_memmove_idx()
without checking for destructive overlaps (especially of real storage
locations):
"Each storage operand is processed left to right. The
storage-operand-consistency rules are the same as
for MOVE (MVC), except that when the operands
overlap in real storage, the use of the common real-
storage locations is not necessarily recognized."
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index e50cec9263..6b85f44e22 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -2086,8 +2086,9 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2)
{
const uint8_t psw_as = (env->psw.mask & PSW_MASK_ASC) >> PSW_SHIFT_ASC;
+S390Access srca, desta;
uintptr_t ra = GETPC();
-int cc = 0, i;
+int cc = 0;
HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n",
__func__, l, a1, a2);
@@ -2106,20 +2107,19 @@ uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l,
uint64_t a1, uint64_t a2)
return cc;
}
-/* XXX replace w/ memcpy */
-for (i = 0; i < l; i++) {
-uint8_t x = cpu_ldub_primary_ra(env, a2 + i, ra);
-cpu_stb_secondary_ra(env, a1 + i, x, ra);
-}
-
+/* TODO: Access key handling */
+srca = access_prepare(env, a2, l, MMU_DATA_LOAD, MMU_PRIMARY_IDX, ra);
+desta = access_prepare(env, a1, l, MMU_DATA_STORE, MMU_SECONDARY_IDX, ra);
+access_memmove(env, &desta, &srca, ra);
return cc;
}
uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2)
{
const uint8_t psw_as = (env->psw.mask & PSW_MASK_ASC) >> PSW_SHIFT_ASC;
+S390Access srca, desta;
uintptr_t ra = GETPC();
-int cc = 0, i;
+int cc = 0;
HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n",
__func__, l, a1, a2);
@@ -2138,12 +2138,10 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l,
uint64_t a1, uint64_t a2)
return cc;
}
-/* XXX replace w/ memcpy */
-for (i = 0; i < l; i++) {
-uint8_t x = cpu_ldub_secondary_ra(env, a2 + i, ra);
-cpu_stb_primary_ra(env, a1 + i, x, ra);
-}
-
+/* TODO: Access key handling */
+srca = access_prepare(env, a2, l, MMU_DATA_LOAD, MMU_SECONDARY_IDX, ra);
+desta = access_prepare(env, a1, l, MMU_DATA_STORE, MMU_PRIMARY_IDX, ra);
+access_memmove(env, &desta, &srca, ra);
return cc;
}
--
2.21.0
[Qemu-devel] [PATCH v3 29/29] tests/tcg: target/s390x: Test MVO
Let's add the simple test based on the example from the PoP.
Signed-off-by: David Hildenbrand
---
tests/tcg/s390x/Makefile.target | 1 +
tests/tcg/s390x/mvo.c | 25 +
2 files changed, 26 insertions(+)
create mode 100644 tests/tcg/s390x/mvo.c
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 151dc075aa..6a3bfa8b29 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -6,3 +6,4 @@ TESTS+=ipm
TESTS+=exrl-trt
TESTS+=exrl-trtr
TESTS+=pack
+TESTS+=mvo
diff --git a/tests/tcg/s390x/mvo.c b/tests/tcg/s390x/mvo.c
new file mode 100644
index 00..5546fe2a97
--- /dev/null
+++ b/tests/tcg/s390x/mvo.c
@@ -0,0 +1,25 @@
+#include
+#include
+
+int main(void)
+{
+uint8_t dest[6] = {0xff, 0x77, 0x88, 0x99, 0x0c, 0xff};
+uint8_t src[5] = {0xee, 0x12, 0x34, 0x56, 0xee};
+uint8_t expected[6] = {0xff, 0x01, 0x23, 0x45, 0x6c, 0xff};
+int i;
+
+asm volatile (
+"mvo 0(4,%[dest]),0(3,%[src])\n"
+:
+: [dest] "d" (dest + 1),
+ [src] "d" (src + 1)
+: "memory");
+
+for (i = 0; i < sizeof(expected); i++) {
+if (dest[i] != expected[i]) {
+fprintf(stderr, "bad data\n");
+return 1;
+}
+}
+return 0;
+}
--
2.21.0
[Qemu-devel] [PATCH v3 12/29] s390x/tcg: MVCS/MVCP: Properly wrap the length
... and don't perform any move in case the length is zero.
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 320e9ee65c..41d7336a1a 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1980,10 +1980,13 @@ uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l,
uint64_t a1, uint64_t a2)
s390_program_interrupt(env, PGM_SPECIAL_OP, ILEN_AUTO, ra);
}
+l = wrap_length32(env, l);
if (l > 256) {
/* max 256 */
l = 256;
cc = 3;
+} else if (!l) {
+return cc;
}
/* XXX replace w/ memcpy */
@@ -2009,10 +2012,13 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l,
uint64_t a1, uint64_t a2)
s390_program_interrupt(env, PGM_SPECIAL_OP, ILEN_AUTO, ra);
}
+l = wrap_length32(env, l);
if (l > 256) {
/* max 256 */
l = 256;
cc = 3;
+} else if (!l) {
+return cc;
}
/* XXX replace w/ memcpy */
--
2.21.0
[Qemu-devel] [PATCH v3 10/29] s390x/tcg: MVCS/MVCP: Check for special operation exceptions
Let's perform the documented checks.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 12
1 file changed, 12 insertions(+)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 86238e0163..20e1ac0ea9 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1960,12 +1960,18 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2)
{
+const uint8_t psw_as = (env->psw.mask & PSW_MASK_ASC) >> PSW_SHIFT_ASC;
uintptr_t ra = GETPC();
int cc = 0, i;
HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n",
__func__, l, a1, a2);
+if (!(env->psw.mask & PSW_MASK_DAT) || !(env->cregs[0] & CR0_SECONDARY) ||
+psw_as == AS_HOME || psw_as == AS_ACCREG) {
+s390_program_interrupt(env, PGM_SPECIAL_OP, ILEN_AUTO, ra);
+}
+
if (l > 256) {
/* max 256 */
l = 256;
@@ -1983,12 +1989,18 @@ uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l,
uint64_t a1, uint64_t a2)
uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2)
{
+const uint8_t psw_as = (env->psw.mask & PSW_MASK_ASC) >> PSW_SHIFT_ASC;
uintptr_t ra = GETPC();
int cc = 0, i;
HELPER_LOG("%s: %16" PRIx64 " %16" PRIx64 " %16" PRIx64 "\n",
__func__, l, a1, a2);
+if (!(env->psw.mask & PSW_MASK_DAT) || !(env->cregs[0] & CR0_SECONDARY) ||
+psw_as == AS_HOME || psw_as == AS_ACCREG) {
+s390_program_interrupt(env, PGM_SPECIAL_OP, ILEN_AUTO, ra);
+}
+
if (l > 256) {
/* max 256 */
l = 256;
--
2.21.0
[Qemu-devel] [PATCH v3 23/29] s390x/tcg: NC: Fault-safe handling
We can process a maximum of 256 bytes, crossing two pages.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index a2118a82e3..20fc17d44d 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -323,17 +323,26 @@ static int mmu_idx_from_as(uint8_t as)
static uint32_t do_helper_nc(CPUS390XState *env, uint32_t l, uint64_t dest,
uint64_t src, uintptr_t ra)
{
+const int mmu_idx = cpu_mmu_index(env, false);
+S390Access srca1, srca2, desta;
uint32_t i;
uint8_t c = 0;
HELPER_LOG("%s l %d dest %" PRIx64 " src %" PRIx64 "\n",
__func__, l, dest, src);
-for (i = 0; i <= l; i++) {
-uint8_t x = cpu_ldub_data_ra(env, src + i, ra);
-x &= cpu_ldub_data_ra(env, dest + i, ra);
+/* NC always processes one more byte than specified - maximum is 256 */
+l++;
+
+srca1 = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
+desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+for (i = 0; i < l; i++) {
+const uint8_t x = access_get_byte(env, &srca1, i, ra) &
+ access_get_byte(env, &srca2, i, ra);
+
c |= x;
-cpu_stb_data_ra(env, dest + i, x, ra);
+access_set_byte(env, &desta, i, x, ra);
}
return c != 0;
}
--
2.21.0
[Qemu-devel] [PATCH v3 13/29] s390x/tcg: MVST: Check for specification exceptions
Bit position 32-55 of general register 0 must be zero.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/mem_helper.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 41d7336a1a..ec27be174b 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -706,6 +706,9 @@ uint64_t HELPER(mvst)(CPUS390XState *env, uint64_t c,
uint64_t d, uint64_t s)
uintptr_t ra = GETPC();
uint32_t len;
+if (c & 0xff00ull) {
+s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
+}
c = c & 0xff;
d = wrap_address(env, d);
s = wrap_address(env, s);
--
2.21.0
[Qemu-devel] [PATCH v3 14/29] s390x/tcg: MVST: Fix storing back the addresses to registers
24 and 31-bit address space handling is wrong when it comes to storing
back the addresses to the register.
While at it, read gprs 0 implicitly.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/helper.h | 2 +-
target/s390x/insn-data.def | 2 +-
target/s390x/mem_helper.c | 26 +++---
target/s390x/translate.c | 8 ++--
4 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index e9aff83b05..56e8149866 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -20,7 +20,7 @@ DEF_HELPER_FLAGS_4(mvn, TCG_CALL_NO_WG, void, env, i32, i64,
i64)
DEF_HELPER_FLAGS_4(mvo, TCG_CALL_NO_WG, void, env, i32, i64, i64)
DEF_HELPER_FLAGS_4(mvpg, TCG_CALL_NO_WG, i32, env, i64, i64, i64)
DEF_HELPER_FLAGS_4(mvz, TCG_CALL_NO_WG, void, env, i32, i64, i64)
-DEF_HELPER_4(mvst, i64, env, i64, i64, i64)
+DEF_HELPER_3(mvst, i32, env, i32, i32)
DEF_HELPER_4(ex, void, env, i32, i64, i64)
DEF_HELPER_FLAGS_4(stam, TCG_CALL_NO_WG, void, env, i32, i64, i32)
DEF_HELPER_FLAGS_4(lam, TCG_CALL_NO_WG, void, env, i32, i64, i32)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index f421184fcd..449eee1662 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -637,7 +637,7 @@
/* MOVE PAGE */
C(0xb254, MVPG,RRE, Z, r1_o, r2_o, 0, 0, mvpg, 0)
/* MOVE STRING */
-C(0xb255, MVST,RRE, Z, r1_o, r2_o, 0, 0, mvst, 0)
+C(0xb255, MVST,RRE, Z, 0, 0, 0, 0, mvst, 0)
/* MOVE WITH OPTIONAL SPECIFICATION */
C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0)
/* MOVE WITH OFFSET */
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index ec27be174b..a24506676b 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -700,18 +700,18 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0,
uint64_t r1, uint64_t r2)
return 0; /* data moved */
}
-/* string copy (c is string terminator) */
-uint64_t HELPER(mvst)(CPUS390XState *env, uint64_t c, uint64_t d, uint64_t s)
+/* string copy */
+uint32_t HELPER(mvst)(CPUS390XState *env, uint32_t r1, uint32_t r2)
{
+const uint64_t d = get_address(env, r1);
+const uint64_t s = get_address(env, r2);
+const uint8_t c = env->regs[0];
uintptr_t ra = GETPC();
uint32_t len;
-if (c & 0xff00ull) {
+if (env->regs[0] & 0xff00ull) {
s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
}
-c = c & 0xff;
-d = wrap_address(env, d);
-s = wrap_address(env, s);
/* Lest we fail to service interrupts in a timely manner, limit the
amount of work we're willing to do. For now, let's cap at 8k. */
@@ -719,17 +719,13 @@ uint64_t HELPER(mvst)(CPUS390XState *env, uint64_t c,
uint64_t d, uint64_t s)
uint8_t v = cpu_ldub_data_ra(env, s + len, ra);
cpu_stb_data_ra(env, d + len, v, ra);
if (v == c) {
-/* Complete. Set CC=1 and advance R1. */
-env->cc_op = 1;
-env->retxl = s;
-return d + len;
+set_address_zero(env, r1, d + len);
+return 1;
}
}
-
-/* Incomplete. Set CC=3 and signal to advance R1 and R2. */
-env->cc_op = 3;
-env->retxl = s + len;
-return d + len;
+set_address_zero(env, r1, d + len);
+set_address_zero(env, r2, s + len);
+return 3;
}
/* load access registers r1 to r3 from memory at a2 */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 2927247c82..b0a2500e5f 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3488,9 +3488,13 @@ static DisasJumpType op_mvpg(DisasContext *s, DisasOps
*o)
static DisasJumpType op_mvst(DisasContext *s, DisasOps *o)
{
-gen_helper_mvst(o->in1, cpu_env, regs[0], o->in1, o->in2);
+TCGv_i32 t1 = tcg_const_i32(get_field(s->fields, r1));
+TCGv_i32 t2 = tcg_const_i32(get_field(s->fields, r2));
+
+gen_helper_mvst(cc_op, cpu_env, t1, t2);
+tcg_temp_free_i32(t1);
+tcg_temp_free_i32(t2);
set_cc_static(s);
-return_low128(o->in2);
return DISAS_NEXT;
}
--
2.21.0
Re: [Qemu-devel] [PATCH v7 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335
On Mon, 2019-09-16 at 15:39 +0200, Max Reitz wrote: > On 15.09.19 22:36, Maxim Levitsky wrote: > > Commit 8ac0f15f335 accidently broke the COW of non changed areas > > of newly allocated clusters, when the write spans multiple clusters, > > and needs COW both prior and after the write. > > This results in 'after' COW area being encrypted with wrong > > sector address, which render it corrupted. > > > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922 > > > > CC: qemu-stable > > > > V2: grammar, spelling and code style fixes. > > V3: more fixes after the review. > > V4: addressed review comments from Max Reitz, > > and futher refactored the qcow2_co_encrypt to just take full host and > > guest offset > > which simplifies everything. > > > > V5: reworked the patches so one of them fixes the bug > > only and other one is just refactoring > > > > V6: removed do_perform_cow_encrypt > > > > V7: removed do_perform_cow_encrypt take two, this > > time I hopefully did that correctly :-) > > Also updated commit names and messages a bit > > Luckily for you (maybe), Vladimir’s series doesn‘t quite pass the > iotests for me, so unfortunately (I find it unfortunate) I had to remove > it from my branch. Thus, the conflicts are much more tame and I felt > comfortable taking the series to my branch (with the remaining trivial > conflicts resolved, and with Vladimir’s suggestion applied): > > https://git.xanclic.moe/XanClic/qemu/commits/branch/block First of all, Thanks! I don't know if this is luckily for me since I already rebased my series on top of https://git.xanclic.moe/XanClic/qemu.git, and run all qcow2 iotests, and only tests 162 169 194 196 234 262 failed, and I know that 162 always fails due to that kernel change I talked about here few days ago, and rest for the AF_UNIX path len, which I need to do something about in the long term. I sometimes do a separate build in directory which path doesn't trigger this, and sometimes, when I know that I haven't done significant changes to the patches, I just let these tests fail. In long term, maybe even in a few days I'll allocate some time to rethink the build environment here to fix that permanently. Now I am rerunning the iotests just for fun, in short enough directory to see if I can reproduce the failure that you had. After looking in your report, that iotest 026 fails, it does pass here, but then I am only running these iotests on my laptop so I probably don't trigger the race you were able to. So thanks again! Best regards, Maxim Levitsky
