[PATCH v3 0/2] Enhance maximum priority support of PLIC

[Jim Shu]

This patchset fixes hard-coded maximum priority of interrupt priority
register and also changes this register to WARL field to align the PLIC
spec.

Changelog:

v3:
  * fix opposite of power-of-2 max priority checking expression.

v2:
  * change interrupt priority register to WARL field.

Jim Shu (2):
  hw/intc: sifive_plic: fix hard-coded max priority level
  hw/intc: sifive_plic: change interrupt priority register to WARL field

 hw/intc/sifive_plic.c | 25 ++---
 1 file changed, 22 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v3 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field

[Jim Shu]

PLIC spec [1] requires interrupt source priority registers are WARL
field and the number of supported priority is power-of-2 to simplify SW
discovery.

Existing QEMU RISC-V machine (e.g. shakti_c) don't strictly follow PLIC
spec, whose number of supported priority is not power-of-2. Just change
each bit of interrupt priority register to WARL field when the number of
supported priority is power-of-2.

[1] 
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities

Signed-off-by: Jim Shu 
---
 hw/intc/sifive_plic.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index f864efa761..c2dfacf028 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr addr, 
uint64_t value,
 if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
 uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
 
-if (value <= plic->num_priorities) {
+if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
+/*
+ * if "num_priorities + 1" is power-of-2, make each register bit of
+ * interrupt priority WARL (Write-Any-Read-Legal). Just filter
+ * out the access to unsupported priority bits.
+ */
+plic->source_priority[irq] = value % (plic->num_priorities + 1);
+sifive_plic_update(plic);
+} else if (value <= plic->num_priorities) {
 plic->source_priority[irq] = value;
 sifive_plic_update(plic);
 }
@@ -207,7 +215,16 @@ static void sifive_plic_write(void *opaque, hwaddr addr, 
uint64_t value,
 uint32_t contextid = (addr & (plic->context_stride - 1));
 
 if (contextid == 0) {
-if (value <= plic->num_priorities) {
+if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
+/*
+ * if "num_priorities + 1" is power-of-2, each register bit of
+ * interrupt priority is WARL (Write-Any-Read-Legal). Just
+ * filter out the access to unsupported priority bits.
+ */
+plic->target_priority[addrid] = value %
+(plic->num_priorities + 1);
+sifive_plic_update(plic);
+} else if (value <= plic->num_priorities) {
 plic->target_priority[addrid] = value;
 sifive_plic_update(plic);
 }
-- 
2.17.1

[PATCH v3 1/2] hw/intc: sifive_plic: fix hard-coded max priority level

[Jim Shu]

The maximum priority level is hard-coded when writing to interrupt
priority register. However, when writing to priority threshold register,
the maximum priority level is from num_priorities Property which is
configured by platform.

Also change interrupt priority register to use num_priorities Property
in maximum priority level.

Signed-off-by: Emmanuel Blot 
Signed-off-by: Jim Shu 
Reviewed-by: Frank Chang 
---
 hw/intc/sifive_plic.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index af4ae3630e..f864efa761 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -180,8 +180,10 @@ static void sifive_plic_write(void *opaque, hwaddr addr, 
uint64_t value,
 if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
 uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
 
-plic->source_priority[irq] = value & 7;
-sifive_plic_update(plic);
+if (value <= plic->num_priorities) {
+plic->source_priority[irq] = value;
+sifive_plic_update(plic);
+}
 } else if (addr_between(addr, plic->pending_base,
 plic->num_sources >> 3)) {
 qemu_log_mask(LOG_GUEST_ERROR,
-- 
2.17.1

Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field

[Jim Shu]

Hi Clément,

> > > @@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr 
> > > addr, uint64_t value,
> > >  if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) 
> > > {
> > >  uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
> > >
> > > -if (value <= plic->num_priorities) {
> > > +if ((plic->num_priorities + 1) & (plic->num_priorities)) {
> >
> > That's the opposite. If n is a power of 2, n & (n-1) == 0 (eg 8 & 7 ==
> >  0, 9 & 8 == 8).
> > Note that n must be positive too. But I'm not sure it matters here.
> > I'll let you decide.
> >

num_priorities is a uint32_t variable so that n is always positive.

[RFC 7/7] migration: call qemu_savevm_state_pending_exact() with the guest stopped

[Juan Quintela]

HACK ahead.

There are devices that require the guest to be stopped to tell us what
is the size of its state.  So we need to stop the vm "before" we
cal the functions.

It is a hack because:
- we are "starting" the guest again to stop it in migration_complete()
  I know, I know, but it is not trivial to get all the information
  easily to migration_complete(), so this hack.

- auto_converge test fails with this hack.  I think that it is related
  to previous problem.  We start the guest when it is supposed to be
  stopped for convergence reasons.

- All experiments that I did to do the proper thing failed with having
  the iothread_locked() or try to unlock() it when not locked.

- several of the pending functions are using the iothread lock
  themselves, so I need to split it to have two versions (one for the
  _estimate() case with the iothread lock), and another for the
  _exact() case without the iothread_lock().  I want comments about
  this approach before I try to continue on this direction.

Signed-off-by: Juan Quintela 
---
 migration/migration.c| 13 +
 tests/qtest/migration-test.c |  3 ++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 35e512887a..7374884818 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3742,7 +3742,20 @@ static MigIterateState 
migration_iteration_run(MigrationState *s)
 trace_migrate_pending_estimate(pending_size, s->threshold_size, pend_pre, 
pend_post);
 
 if (pend_pre <= s->threshold_size) {
+int old_state = s->state;
+qemu_mutex_lock_iothread();
+// is this really necessary?  it works for me both ways.
+qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
+s->vm_was_running = runstate_is_running();
+vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+qemu_mutex_unlock_iothread();
 qemu_savevm_state_pending_exact(_pre, _post);
+qemu_mutex_lock_iothread();
+runstate_set(old_state);
+if (s->vm_was_running) {
+vm_start();
+}
+qemu_mutex_unlock_iothread();
 pending_size = pend_pre + pend_post;
 trace_migrate_pending_exact(pending_size, s->threshold_size, pend_pre, 
pend_post);
 }
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 0d153d6b5e..0541a842ec 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2564,7 +2564,8 @@ int main(int argc, char **argv)
 qtest_add_func("/migration/validate_uuid_dst_not_set",
test_validate_uuid_dst_not_set);
 
-qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
+if (0)
+qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
 qtest_add_func("/migration/multifd/tcp/plain/none",
test_multifd_tcp_none);
 qtest_add_func("/migration/multifd/tcp/plain/cancel",
-- 
2.37.2

[RFC 6/7] migration: simplify migration_iteration_run()

[Juan Quintela]

Signed-off-by: Juan Quintela 
---
 migration/migration.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 97fefd579e..35e512887a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3747,23 +3747,23 @@ static MigIterateState 
migration_iteration_run(MigrationState *s)
 trace_migrate_pending_exact(pending_size, s->threshold_size, pend_pre, 
pend_post);
 }
 
-if (pending_size && pending_size >= s->threshold_size) {
-/* Still a significant amount to transfer */
-if (!in_postcopy && pend_pre <= s->threshold_size &&
-qatomic_read(>start_postcopy)) {
-if (postcopy_start(s)) {
-error_report("%s: postcopy failed to start", __func__);
-}
-return MIG_ITERATE_SKIP;
-}
-/* Just another iteration step */
-qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
-} else {
+if (pending_size < s->threshold_size) {
 trace_migration_thread_low_pending(pending_size);
 migration_completion(s);
 return MIG_ITERATE_BREAK;
 }
 
+/* Still a significant amount to transfer */
+if (!in_postcopy && pend_pre <= s->threshold_size &&
+qatomic_read(>start_postcopy)) {
+if (postcopy_start(s)) {
+error_report("%s: postcopy failed to start", __func__);
+}
+return MIG_ITERATE_SKIP;
+}
+
+/* Just another iteration step */
+qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
 return MIG_ITERATE_RESUME;
 }
 
-- 
2.37.2

[RFC 2/7] migration: No save_live_pending() method uses the QEMUFile parameter

[Juan Quintela]

So remove it everywhere.

Signed-off-by: Juan Quintela 
---
 include/migration/register.h   | 6 ++
 migration/savevm.h | 2 +-
 hw/s390x/s390-stattrib.c   | 2 +-
 hw/vfio/migration.c| 6 ++
 migration/block-dirty-bitmap.c | 5 ++---
 migration/block.c  | 2 +-
 migration/migration.c  | 3 +--
 migration/ram.c| 2 +-
 migration/savevm.c | 5 ++---
 9 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index 1950fee6a8..5b5424ed8f 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -46,10 +46,8 @@ typedef struct SaveVMHandlers {
 
 /* This runs outside the iothread lock!  */
 int (*save_setup)(QEMUFile *f, void *opaque);
-void (*save_live_pending)(QEMUFile *f, void *opaque,
-  uint64_t threshold_size,
-  uint64_t *rest_precopy,
-  uint64_t *rest_postcopy);
+void (*save_live_pending)(void *opaque,  uint64_t threshold_size,
+  uint64_t *rest_precopy, uint64_t *rest_postcopy);
 /* Note for save_live_pending:
  * - res_precopy is for data which must be migrated in precopy
  * phase or in stopped state, in other words - before target
diff --git a/migration/savevm.h b/migration/savevm.h
index 9bd55c336c..98fae6f9b3 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -40,7 +40,7 @@ void qemu_savevm_state_cleanup(void);
 void qemu_savevm_state_complete_postcopy(QEMUFile *f);
 int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
bool inactivate_disks);
-void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size,
+void qemu_savevm_state_pending(uint64_t max_size,
uint64_t *res_precopy, uint64_t *res_postcopy);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index ee60b53da4..9b74eeadf3 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -182,7 +182,7 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
 return 0;
 }
 
-static void cmma_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
+static void cmma_save_pending(void *opaque, uint64_t max_size,
   uint64_t *res_precopy, uint64_t *res_postcopy)
 {
 S390StAttribState *sas = S390_STATTRIB(opaque);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 3423f113f0..760d5f3c5c 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -456,10 +456,8 @@ static void vfio_save_cleanup(void *opaque)
 trace_vfio_save_cleanup(vbasedev->name);
 }
 
-static void vfio_save_pending(QEMUFile *f, void *opaque,
-  uint64_t threshold_size,
-  uint64_t *res_precopy,
-  uint64_t *res_postcopy)
+static void vfio_save_pending(void *opaque,  uint64_t threshold_size,
+  uint64_t *res_precopy, uint64_t *res_postcopy)
 {
 VFIODevice *vbasedev = opaque;
 VFIOMigration *migration = vbasedev->migration;
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index dfea546330..a445bdc3c3 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -761,9 +761,8 @@ static int dirty_bitmap_save_complete(QEMUFile *f, void 
*opaque)
 return 0;
 }
 
-static void dirty_bitmap_save_pending(QEMUFile *f, void *opaque,
-  uint64_t max_size,
-  uint64_t *res_precopy,
+static void dirty_bitmap_save_pending(void *opaque, uint64_t max_size,
+  uint64_t *res_precopy, 
   uint64_t *res_postcopy)
 {
 DBMSaveState *s = &((DBMState *)opaque)->save;
diff --git a/migration/block.c b/migration/block.c
index 4ae8f837b0..b3d680af75 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -862,7 +862,7 @@ static int block_save_complete(QEMUFile *f, void *opaque)
 return 0;
 }
 
-static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
+static void block_save_pending(void *opaque, uint64_t max_size,
uint64_t *res_precopy,
uint64_t *res_postcopy)
 {
diff --git a/migration/migration.c b/migration/migration.c
index 440aa62f16..038fc58a96 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3737,8 +3737,7 @@ static MigIterateState 
migration_iteration_run(MigrationState *s)
 uint64_t pending_size, pend_pre, pend_post;
 bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
 
-qemu_savevm_state_pending(s->to_dst_file, s->threshold_size, _pre,
-  

[RFC 4/7] migration: Split save_live_pending() into state_pending_*

[Juan Quintela]

We split the function into to:

- state_pending_estimate: We estimate the remaining state size without
  stopping the machine.

- state pending_exact: We calculate the exact amount of remaining
  state.

The only "device" that implements different functions for _estimate()
and _exact() is ram.

Signed-off-by: Juan Quintela 
---
 docs/devel/migration.rst   | 18 ++
 docs/devel/vfio-migration.rst  |  4 ++--
 include/migration/register.h   | 12 
 migration/savevm.h |  8 ++--
 hw/s390x/s390-stattrib.c   |  7 ---
 hw/vfio/migration.c|  9 +
 migration/block-dirty-bitmap.c | 11 ++-
 migration/block.c  | 11 ++-
 migration/migration.c  | 13 +
 migration/ram.c| 31 ---
 migration/savevm.c | 34 +-
 hw/vfio/trace-events   |  2 +-
 migration/trace-events |  7 ---
 13 files changed, 114 insertions(+), 53 deletions(-)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 3e9656d8e0..6f65c23b47 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -482,15 +482,17 @@ An iterative device must provide:
   - A ``load_setup`` function that initialises the data structures on the
 destination.
 
-  - A ``save_live_pending`` function that is called repeatedly and must
-indicate how much more data the iterative data must save.  The core
-migration code will use this to determine when to pause the CPUs
-and complete the migration.
+  - A ``state_pending_exact`` function that indicates how much more
+data we must save.  The core migration code will use this to
+determine when to pause the CPUs and complete the migration.
 
-  - A ``save_live_iterate`` function (called after ``save_live_pending``
-when there is significant data still to be sent).  It should send
-a chunk of data until the point that stream bandwidth limits tell it
-to stop.  Each call generates one section.
+  - A ``state_pending_estimate`` function that indicates how much more
+data we must save.  When the estimated amount is smaller than the
+threshold, we call ``state_pending_exact``.
+
+  - A ``save_live_iterate`` function should send a chunk of data until
+the point that stream bandwidth limits tell it to stop.  Each call
+generates one section.
 
   - A ``save_live_complete_precopy`` function that must transmit the
 last section for the device containing any remaining data.
diff --git a/docs/devel/vfio-migration.rst b/docs/devel/vfio-migration.rst
index 9ff6163c88..673057c90d 100644
--- a/docs/devel/vfio-migration.rst
+++ b/docs/devel/vfio-migration.rst
@@ -28,7 +28,7 @@ VFIO implements the device hooks for the iterative approach 
as follows:
 * A ``load_setup`` function that sets up the migration region on the
   destination and sets _RESUMING flag in the VFIO device state.
 
-* A ``save_live_pending`` function that reads pending_bytes from the vendor
+* A ``state_pending_exact`` function that reads pending_bytes from the vendor
   driver, which indicates the amount of data that the vendor driver has yet to
   save for the VFIO device.
 
@@ -114,7 +114,7 @@ Live migration save path
 (RUNNING, _SETUP, _RUNNING|_SAVING)
   |
 (RUNNING, _ACTIVE, _RUNNING|_SAVING)
- If device is active, get pending_bytes by .save_live_pending()
+ If device is active, get pending_bytes by .state_pending_exact()
   If total pending_bytes >= threshold_size, call .save_live_iterate()
   Data of VFIO device for pre-copy phase is copied
 Iterate till total pending bytes converge and are less than threshold
diff --git a/include/migration/register.h b/include/migration/register.h
index 5b5424ed8f..313b8e1c3b 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -46,9 +46,7 @@ typedef struct SaveVMHandlers {
 
 /* This runs outside the iothread lock!  */
 int (*save_setup)(QEMUFile *f, void *opaque);
-void (*save_live_pending)(void *opaque,  uint64_t threshold_size,
-  uint64_t *rest_precopy, uint64_t *rest_postcopy);
-/* Note for save_live_pending:
+/* Note for state_pending_*:
  * - res_precopy is for data which must be migrated in precopy
  * phase or in stopped state, in other words - before target
  * vm start
@@ -59,7 +57,13 @@ typedef struct SaveVMHandlers {
  * Sum of res_precopy and res_postcopy is the whole amount of
  * pending data.
  */
-
+/* This calculate the exact remaining data to transfer */
+void (*state_pending_exact)(void *opaque,  uint64_t threshold_size,
+uint64_t *rest_precopy, uint64_t 
*rest_postcopy);
+/* This estimates the remaining data to transfer */
+void 

[RFC 3/7] migration: Block migration comment or code is wrong

[Juan Quintela]

And it appears that what is wrong is the code. During bulk stage we
need to make sure that some block is dirty, but no games with
max_size at all.

Signed-off-by: Juan Quintela 
---
 migration/block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index b3d680af75..39ce4003c6 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -879,8 +879,8 @@ static void block_save_pending(void *opaque, uint64_t 
max_size,
 blk_mig_unlock();
 
 /* Report at least one block pending during bulk phase */
-if (pending <= max_size && !block_mig_state.bulk_completed) {
-pending = max_size + BLK_MIG_BLOCK_SIZE;
+if (!pending && !block_mig_state.bulk_completed) {
+pending = BLK_MIG_BLOCK_SIZE;
 }
 
 trace_migration_block_save_pending(pending);
-- 
2.37.2

[RFC 5/7] migration: Remove unused threshold_size parameter

[Juan Quintela]

Until previous commit, save_live_pending() was used for ram.  Now with
the split into state_pending_estimate() and state_pending_exact() it
is not needed anymore, so remove them.

Signed-off-by: Juan Quintela 
---
 include/migration/register.h   |  7 +++
 migration/savevm.h |  6 ++
 hw/vfio/migration.c|  6 +++---
 migration/block-dirty-bitmap.c |  3 +--
 migration/block.c  |  3 +--
 migration/migration.c  |  4 ++--
 migration/ram.c|  6 ++
 migration/savevm.c | 12 
 8 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index 313b8e1c3b..5f08204fb2 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -58,11 +58,10 @@ typedef struct SaveVMHandlers {
  * pending data.
  */
 /* This calculate the exact remaining data to transfer */
-void (*state_pending_exact)(void *opaque,  uint64_t threshold_size,
-uint64_t *rest_precopy, uint64_t 
*rest_postcopy);
+void (*state_pending_exact)(void *opaque, uint64_t *rest_precopy,
+uint64_t *rest_postcopy);
 /* This estimates the remaining data to transfer */
-void (*state_pending_estimate)(void *opaque,  uint64_t threshold_size,
-   uint64_t *rest_precopy,
+void (*state_pending_estimate)(void *opaque, uint64_t *rest_precopy,
uint64_t *rest_postcopy);
 
 LoadStateHandler *load_state;
diff --git a/migration/savevm.h b/migration/savevm.h
index 613f85e717..24f2d2a28b 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -40,11 +40,9 @@ void qemu_savevm_state_cleanup(void);
 void qemu_savevm_state_complete_postcopy(QEMUFile *f);
 int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
bool inactivate_disks);
-void qemu_savevm_state_pending_exact(uint64_t max_size,
- uint64_t *res_precopy,
+void qemu_savevm_state_pending_exact(uint64_t *res_precopy,
  uint64_t *res_postcopy);
-void qemu_savevm_state_pending_estimate(uint64_t max_size,
-uint64_t *res_precopy,
+void qemu_savevm_state_pending_estimate(uint64_t *res_precopy,
 uint64_t *res_postcopy);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 680cf4df6e..d9598ce070 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -456,8 +456,8 @@ static void vfio_save_cleanup(void *opaque)
 trace_vfio_save_cleanup(vbasedev->name);
 }
 
-static void vfio_state_pending(void *opaque,  uint64_t threshold_size,
-   uint64_t *res_precopy, uint64_t *res_postcopy)
+static void vfio_state_pending(void *opaque, uint64_t *res_precopy,
+   uint64_t *res_postcopy)
 {
 VFIODevice *vbasedev = opaque;
 VFIOMigration *migration = vbasedev->migration;
@@ -511,7 +511,7 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque)
 }
 
 /*
- * Reset pending_bytes as .save_live_pending is not called during savevm or
+ * Reset pending_bytes as .state_pending_* is not called during savevm or
  * snapshot case, in such case vfio_update_pending() at the start of this
  * function updates pending_bytes.
  */
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 5b24007650..8a11577252 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -761,8 +761,7 @@ static int dirty_bitmap_save_complete(QEMUFile *f, void 
*opaque)
 return 0;
 }
 
-static void dirty_bitmap_state_pending(void *opaque, uint64_t max_size,
-   uint64_t *res_precopy,
+static void dirty_bitmap_state_pending(void *opaque, uint64_t *res_precopy,
uint64_t *res_postcopy)
 {
 DBMSaveState *s = &((DBMState *)opaque)->save;
diff --git a/migration/block.c b/migration/block.c
index 8e6ad1c468..4f1f7c0f8d 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -862,8 +862,7 @@ static int block_save_complete(QEMUFile *f, void *opaque)
 return 0;
 }
 
-static void block_state_pending(void *opaque, uint64_t max_size,
-uint64_t *res_precopy,
+static void block_state_pending(void *opaque, uint64_t *res_precopy,
 uint64_t *res_postcopy)
 {
 /* Estimate pending number of bytes to send */
diff --git a/migration/migration.c b/migration/migration.c
index 4676568699..97fefd579e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3737,12 +3737,12 @@ static MigIterateState 

[RFC 1/7] migration: Remove res_compatible parameter

[Juan Quintela]

It was only used for RAM, and in that case, it means that this amount
of data was sent for memory.  Just delete the field in all callers.

Signed-off-by: Juan Quintela 
---
 include/migration/register.h   | 20 ++--
 migration/savevm.h |  4 +---
 hw/s390x/s390-stattrib.c   |  6 ++
 hw/vfio/migration.c| 10 --
 migration/block-dirty-bitmap.c |  7 +++
 migration/block.c  |  7 +++
 migration/migration.c  |  9 -
 migration/ram.c|  8 +++-
 migration/savevm.c | 14 +-
 hw/vfio/trace-events   |  2 +-
 migration/trace-events |  2 +-
 11 files changed, 37 insertions(+), 52 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index c1dcff0f90..1950fee6a8 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -48,18 +48,18 @@ typedef struct SaveVMHandlers {
 int (*save_setup)(QEMUFile *f, void *opaque);
 void (*save_live_pending)(QEMUFile *f, void *opaque,
   uint64_t threshold_size,
-  uint64_t *res_precopy_only,
-  uint64_t *res_compatible,
-  uint64_t *res_postcopy_only);
+  uint64_t *rest_precopy,
+  uint64_t *rest_postcopy);
 /* Note for save_live_pending:
- * - res_precopy_only is for data which must be migrated in precopy phase
- * or in stopped state, in other words - before target vm start
- * - res_compatible is for data which may be migrated in any phase
- * - res_postcopy_only is for data which must be migrated in postcopy phase
- * or in stopped state, in other words - after source vm stop
+ * - res_precopy is for data which must be migrated in precopy
+ * phase or in stopped state, in other words - before target
+ * vm start
+ * - res_postcopy is for data which must be migrated in postcopy
+ * phase or in stopped state, in other words - after source vm
+ * stop
  *
- * Sum of res_postcopy_only, res_compatible and res_postcopy_only is the
- * whole amount of pending data.
+ * Sum of res_precopy and res_postcopy is the whole amount of
+ * pending data.
  */
 
 
diff --git a/migration/savevm.h b/migration/savevm.h
index 6461342cb4..9bd55c336c 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -41,9 +41,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f);
 int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
bool inactivate_disks);
 void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size,
-   uint64_t *res_precopy_only,
-   uint64_t *res_compatible,
-   uint64_t *res_postcopy_only);
+   uint64_t *res_precopy, uint64_t *res_postcopy);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
 int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 9eda1c3b2a..ee60b53da4 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -183,16 +183,14 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
 }
 
 static void cmma_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
-  uint64_t *res_precopy_only,
-  uint64_t *res_compatible,
-  uint64_t *res_postcopy_only)
+  uint64_t *res_precopy, uint64_t *res_postcopy)
 {
 S390StAttribState *sas = S390_STATTRIB(opaque);
 S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
 long long res = sac->get_dirtycount(sas);
 
 if (res >= 0) {
-*res_precopy_only += res;
+*res_precopy += res;
 }
 }
 
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 3de4252111..3423f113f0 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -458,9 +458,8 @@ static void vfio_save_cleanup(void *opaque)
 
 static void vfio_save_pending(QEMUFile *f, void *opaque,
   uint64_t threshold_size,
-  uint64_t *res_precopy_only,
-  uint64_t *res_compatible,
-  uint64_t *res_postcopy_only)
+  uint64_t *res_precopy,
+  uint64_t *res_postcopy)
 {
 VFIODevice *vbasedev = opaque;
 VFIOMigration *migration = vbasedev->migration;
@@ -471,10 +470,9 @@ static void vfio_save_pending(QEMUFile *f, void *opaque,
 return;
 }
 
-*res_precopy_only += migration->pending_bytes;
+*res_precopy += migration->pending_bytes;
 
-

[RFC 0/7] migration patches for VFIO

[Juan Quintela]

Hi

VFIO migration has several requirements:
- the size of the state is only known when the guest is stopped
- they need to send possible lots of data.

this series only address the 1st set of problems.

What they do:
- res_compatible parameter was not used anywhere, just add that information to 
res_postcopy.
- Remove QEMUFILE parameter from save_live_pending
- Split save_live_pending into
  * save_pending_estimate(): the pending state size without trying too hard
  * save_pending_exact(): the real pending state size, it is called with the 
guest stopped.
- Now save_pending_* don't need the threshold parameter
- HACK a way to stop the guest before moving there.

ToDo:
- autoconverge test is broken, no real clue why, but it is possible that the 
test is wrong.

- Make an artifact to be able to send massive amount of data in the save state 
stage (probably more multifd channels).

- Be able to not having to start the guest between cheking the state pending 
size and migration_completion().

Please review.

Thanks, Juan.

Juan Quintela (7):
  migration: Remove res_compatible parameter
  migration: No save_live_pending() method uses the QEMUFile parameter
  migration: Block migration comment or code is wrong
  migration: Split save_live_pending() into state_pending_*
  migration: Remove unused threshold_size parameter
  migration: simplify migration_iteration_run()
  migration: call qemu_savevm_state_pending_exact() with the guest
stopped

 docs/devel/migration.rst   | 18 ++--
 docs/devel/vfio-migration.rst  |  4 +--
 include/migration/register.h   | 29 ++-
 migration/savevm.h |  8 +++---
 hw/s390x/s390-stattrib.c   | 11 ---
 hw/vfio/migration.c| 17 +--
 migration/block-dirty-bitmap.c | 14 -
 migration/block.c  | 17 ++-
 migration/migration.c  | 52 ++
 migration/ram.c| 35 ---
 migration/savevm.c | 37 +---
 tests/qtest/migration-test.c   |  3 +-
 hw/vfio/trace-events   |  2 +-
 migration/trace-events |  7 +++--
 14 files changed, 148 insertions(+), 106 deletions(-)

-- 
2.37.2

Re: [PATCH v1 1/7] contrib/gitdm: add Simon to individual contributors

[Simon Safar]

Hi Alex,

On Mon, Sep 26, 2022, at 6:46 AM, Alex Bennée wrote:
> Please confirm this is the correct mapping for you.

it's the correct mapping, thanks for adding it! (... & sorry for the multi-day 
latency!)

Reviewed-by: Simon Safar 

> 
> Signed-off-by: Alex Bennée 
> Cc: Simon Safar 
> ---
> contrib/gitdm/group-map-individuals | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/contrib/gitdm/group-map-individuals 
> b/contrib/gitdm/group-map-individuals
> index f816aa8770..d5b05041bc 100644
> --- a/contrib/gitdm/group-map-individuals
> +++ b/contrib/gitdm/group-map-individuals
> @@ -34,3 +34,4 @@ bmeng...@gmail.com
>  liq...@gmail.com
>  chetan4wind...@gmail.com
>  akihiko.od...@gmail.com
> +si...@simonsafar.com
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 1/3] util/main-loop: Fix maximum number of wait objects for win32

[Bin Meng]

Hi Paolo,

On Sun, Sep 25, 2022 at 9:07 AM Bin Meng  wrote:
>
> Hi Paolo,
>
> On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau
>  wrote:
> >
> > Hi
> >
> > On Wed, Aug 24, 2022 at 12:52 PM Bin Meng  wrote:
> >>
> >> From: Bin Meng 
> >>
> >> The maximum number of wait objects for win32 should be
> >> MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1.
> >>
> >> Signed-off-by: Bin Meng 
> >> ---
> >>
> >> Changes in v3:
> >> - move the check of adding the same HANDLE twice to a separete patch
> >>
> >> Changes in v2:
> >> - fix the logic in qemu_add_wait_object() to avoid adding
> >>   the same HANDLE twice
> >>
> >>  util/main-loop.c | 11 +++
> >>  1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/util/main-loop.c b/util/main-loop.c
> >> index f00a25451b..cb018dc33c 100644
> >> --- a/util/main-loop.c
> >> +++ b/util/main-loop.c
> >> @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void 
> >> *opaque)
> >>  /* Wait objects support */
> >>  typedef struct WaitObjects {
> >>  int num;
> >> -int revents[MAXIMUM_WAIT_OBJECTS + 1];
> >> -HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
> >> -WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1];
> >> -void *opaque[MAXIMUM_WAIT_OBJECTS + 1];
> >> +int revents[MAXIMUM_WAIT_OBJECTS];
> >> +HANDLE events[MAXIMUM_WAIT_OBJECTS];
> >> +WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS];
> >> +void *opaque[MAXIMUM_WAIT_OBJECTS];
> >>  } WaitObjects;
> >>
> >>  static WaitObjects wait_objects = {0};
> >> @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, 
> >> WaitObjectFunc *func, void *opaque)
> >>  if (w->events[i] == handle) {
> >>  found = 1;
> >>  }
> >> +if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> >> +break;
> >> +}
> >
> >
> > hmm
> >
> >>
> >>  if (found) {
> >>  w->events[i] = w->events[i + 1];
> >>  w->func[i] = w->func[i + 1];
> >
> >
> > The way deletion works is by moving the i+1 element (which is always zeroed 
> > for i == MAXIMUM_WAIT_OBJECTS) to i.
> >
> > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the 
> > last value, and instead rely simply on updated w->num:
> >
> > if (found) {
> > w->num--;
> > }
> >
> >  So your patch looks ok to me, but I prefer the current code.
> >
> > Paolo, what do you say?
>
> Ping?
>

Ping?

Regards,
Bin

Re: [PATCH] RISC-V: Add support for Ztso

[Palmer Dabbelt]

On Thu, 29 Sep 2022 12:16:48 PDT (-0700), dgilb...@redhat.com wrote:

* Palmer Dabbelt (pal...@rivosinc.com) wrote:

Ztso, the RISC-V extension that provides the TSO memory model, was
recently frozen.  This provides support for Ztso on targets that are
themselves TSO.

Signed-off-by: Palmer Dabbelt 

---




diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index 00fcbe297d..2a43d54fcd 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -236,6 +236,7 @@ static inline void tb_target_set_jmp_target(uintptr_t 
tc_ptr, uintptr_t jmp_rx,
 #include "tcg/tcg-mo.h"

 #define TCG_TARGET_DEFAULT_MO (TCG_MO_ALL & ~TCG_MO_ST_LD)
+#define TCG_TARGET_SUPPORTS_MCTCG_RVTSO 1


Is x86's brand of memory ordering strong enough for Ztso?
I thought x86 had an optimisation where it was allowed to store forward
within the current CPU causing stores not to be quite strictly ordered.


I'm actually not sure: my understanding of the Intel memory model was 
that there's a bunch of subtle bits that don't match the various TSO 
formalizations, but the RISC-V folks are pretty adamant that Intel is 
exactly TSO.  I've gotten yelled at enough times on this one that I kind 
of just stopped caring, but that's not a good reason to have broken code 
so I'm happy to go fix it.


That said, when putting together the v2 (which has TCG barriers in the 
RISC-V front-end) I couldn't even really figure out how the TCG memory 
model works in any formal capacity -- I essentially just added the 
fences necessary for Ztso on RVWMO, but that's not a good proxy for Ztso 
on arm64 (and I guess not on x86, either).  Also happy to go take a 
crack at that one, but I'm not really a formal memory model person so it 
might not be the best result.




Dave


 #define TCG_TARGET_HAS_MEMORY_BSWAP  have_movbe

diff --git a/tcg/s390x/tcg-target.h b/tcg/s390x/tcg-target.h
index 23e2063667..f423c124a0 100644
--- a/tcg/s390x/tcg-target.h
+++ b/tcg/s390x/tcg-target.h
@@ -171,6 +171,7 @@ extern uint64_t s390_facilities[3];
 #define TCG_TARGET_HAS_MEMORY_BSWAP   1

 #define TCG_TARGET_DEFAULT_MO (TCG_MO_ALL & ~TCG_MO_ST_LD)
+#define TCG_TARGET_SUPPORTS_MCTCG_RVTSO 1

 static inline void tb_target_set_jmp_target(uintptr_t tc_ptr, uintptr_t jmp_rx,
 uintptr_t jmp_rw, uintptr_t addr)
--
2.34.1

[PATCH v2 9/9] target/i386: Use probe_access_full for final stage2 translation

[Richard Henderson]

Rather than recurse directly on mmu_translate, go through the
same softmmu lookup that we did for the page table walk.
This centralizes all knowledge of MMU_NESTED_IDX, with respect
to setup of TranslationParams, to get_physical_address.

Signed-off-by: Richard Henderson 
---
 target/i386/tcg/sysemu/excp_helper.c | 40 +++-
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index e8457e9b21..d51b5d7431 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -143,7 +143,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 .err = err,
 .ptw_idx = in->ptw_idx,
 };
-hwaddr pte_addr;
+hwaddr pte_addr, paddr;
 uint32_t pkr;
 int page_size;
 
@@ -420,33 +420,47 @@ do_check_protect_pse36:
 }
 
 /* align to page_size */
-out->paddr = (pte & a20_mask & PG_ADDRESS_MASK & ~(page_size - 1))
-   | (addr & (page_size - 1));
+paddr = (pte & a20_mask & PG_ADDRESS_MASK & ~(page_size - 1))
+  | (addr & (page_size - 1));
 
 if (in->ptw_idx == MMU_NESTED_IDX) {
-TranslateParams nested_in = {
-.addr = out->paddr,
-.access_type = access_type,
-.cr3 = env->nested_cr3,
-.pg_mode = env->nested_pg_mode,
-.mmu_idx = MMU_USER_IDX,
-.ptw_idx = MMU_PHYS_IDX,
-};
+CPUTLBEntryFull *full;
+int flags, nested_page_size;
 
-if (!mmu_translate(env, _in, out, err)) {
+flags = probe_access_full(env, paddr, access_type,
+  MMU_NESTED_IDX, true,
+  _trans.haddr, , 0);
+if (unlikely(flags & TLB_INVALID_MASK)) {
+err->exception_index = 0; /* unused */
+err->error_code = env->error_code;
+err->cr2 = paddr;
 err->stage2 = S2_GPA;
 return false;
 }
 
 /* Merge stage1 & stage2 protection bits. */
-prot &= out->prot;
+prot &= full->prot;
 
 /* Re-verify resulting protection. */
 if ((prot & (1 << access_type)) == 0) {
 goto do_fault_protect;
 }
+
+/* Merge stage1 & stage2 addresses to final physical address. */
+nested_page_size = 1 << full->lg_page_size;
+paddr = (full->phys_addr & ~(nested_page_size - 1))
+  | (paddr & (nested_page_size - 1));
+
+/*
+ * Use the larger of stage1 & stage2 page sizes, so that
+ * invalidation works.
+ */
+if (nested_page_size > page_size) {
+page_size = nested_page_size;
+}
 }
 
+out->paddr = paddr;
 out->prot = prot;
 out->page_size = page_size;
 return true;
-- 
2.34.1

[PATCH v2 8/9] target/i386: Use atomic operations for pte updates

[Richard Henderson]

Use probe_access_full in order to resolve to a host address,
which then lets us use a host cmpxchg to update the pte.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/279
Signed-off-by: Richard Henderson 
---
 target/i386/tcg/sysemu/excp_helper.c | 242 +++
 1 file changed, 168 insertions(+), 74 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index d6b7de6eea..e8457e9b21 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -27,8 +27,8 @@ typedef struct TranslateParams {
 target_ulong cr3;
 int pg_mode;
 int mmu_idx;
+int ptw_idx;
 MMUAccessType access_type;
-bool use_stage2;
 } TranslateParams;
 
 typedef struct TranslateResult {
@@ -50,43 +50,106 @@ typedef struct TranslateFault {
 TranslateFaultStage2 stage2;
 } TranslateFault;
 
-#define PTE_HPHYS(ADDR) \
-do {\
-if (in->use_stage2) {   \
-nested_in.addr = (ADDR);\
-if (!mmu_translate(env, _in, out, err)) {\
-err->stage2 = S2_GPT;   \
-return false;   \
-}   \
-(ADDR) = out->paddr;\
-}   \
-} while (0)
+typedef struct PTETranslate {
+CPUX86State *env;
+TranslateFault *err;
+int ptw_idx;
+void *haddr;
+hwaddr gaddr;
+} PTETranslate;
+
+static bool ptw_translate(PTETranslate *inout, hwaddr addr)
+{
+CPUTLBEntryFull *full;
+int flags;
+
+inout->gaddr = addr;
+flags = probe_access_full(inout->env, addr, MMU_DATA_STORE,
+  inout->ptw_idx, true, >haddr, , 0);
+
+if (unlikely(flags & TLB_INVALID_MASK)) {
+TranslateFault *err = inout->err;
+
+assert(inout->ptw_idx == MMU_NESTED_IDX);
+err->exception_index = 0; /* unused */
+err->error_code = inout->env->error_code;
+err->cr2 = addr;
+err->stage2 = S2_GPT;
+return false;
+}
+return true;
+}
+
+static inline uint32_t ptw_ldl(const PTETranslate *in)
+{
+if (likely(in->haddr)) {
+return ldl_p(in->haddr);
+}
+return cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
+}
+
+static inline uint64_t ptw_ldq(const PTETranslate *in)
+{
+if (likely(in->haddr)) {
+return ldq_p(in->haddr);
+}
+return cpu_ldq_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
+}
+
+/*
+ * Note that we can use a 32-bit cmpxchg for all page table entries,
+ * even 64-bit ones, because PG_PRESENT_MASK, PG_ACCESSED_MASK and
+ * PG_DIRTY_MASK are all in the low 32 bits.
+ */
+static bool ptw_setl_slow(const PTETranslate *in, uint32_t old, uint32_t new)
+{
+uint32_t cmp;
+
+/* Does x86 really perform a rmw cycle on mmio for ptw? */
+start_exclusive();
+cmp = cpu_ldl_mmuidx_ra(in->env, in->gaddr, in->ptw_idx, 0);
+if (cmp == old) {
+cpu_stl_mmuidx_ra(in->env, in->gaddr, new, in->ptw_idx, 0);
+}
+end_exclusive();
+return cmp == old;
+}
+
+static inline bool ptw_setl(const PTETranslate *in, uint32_t old, uint32_t set)
+{
+if (set & ~old) {
+uint32_t new = old | set;
+if (likely(in->haddr)) {
+old = cpu_to_le32(old);
+new = cpu_to_le32(new);
+return qatomic_cmpxchg((uint32_t *)in->haddr, old, new) == old;
+}
+return ptw_setl_slow(in, old, new);
+}
+return true;
+}
 
 static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
   TranslateResult *out, TranslateFault *err)
 {
-TranslateParams nested_in = {
-/* Use store for page table entries, to allow A/D flag updates. */
-.access_type = MMU_DATA_STORE,
-.cr3 = env->nested_cr3,
-.pg_mode = env->nested_pg_mode,
-.mmu_idx = MMU_USER_IDX,
-.use_stage2 = false,
-};
-
-CPUState *cs = env_cpu(env);
-X86CPU *cpu = env_archcpu(env);
 const int32_t a20_mask = x86_get_a20_mask(env);
 const target_ulong addr = in->addr;
 const int pg_mode = in->pg_mode;
 const bool is_user = (in->mmu_idx == MMU_USER_IDX);
 const MMUAccessType access_type = in->access_type;
-uint64_t ptep, pte;
+uint64_t ptep, pte, rsvd_mask;
+PTETranslate pte_trans = {
+.env = env,
+.err = err,
+.ptw_idx = in->ptw_idx,
+};
 hwaddr pte_addr;
-uint64_t rsvd_mask = PG_ADDRESS_MASK & ~MAKE_64BIT_MASK(0, cpu->phys_bits);
 uint32_t pkr;
 int page_size;
 
+ restart_all:
+rsvd_mask = ~MAKE_64BIT_MASK(0, env_archcpu(env)->phys_bits);
+rsvd_mask &= PG_ADDRESS_MASK;
 if (!(pg_mode & 

[PATCH v2 4/9] target/i386: Reorg GET_HPHYS

[Richard Henderson]

Replace with PTE_HPHYS for the page table walk, and a direct call
to mmu_translate for the final stage2 translation.  Hoist the check
for HF2_NPT_MASK out to get_physical_address, which avoids the
recursive call when stage2 is disabled.

We can now return all the way out to x86_cpu_tlb_fill before raising
an exception, which means probe works.

Signed-off-by: Richard Henderson 
---
 target/i386/tcg/sysemu/excp_helper.c | 123 +--
 1 file changed, 95 insertions(+), 28 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 00ce4cf253..816b307547 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -37,18 +37,43 @@ typedef struct TranslateResult {
 int page_size;
 } TranslateResult;
 
+typedef enum TranslateFaultStage2 {
+S2_NONE,
+S2_GPA,
+S2_GPT,
+} TranslateFaultStage2;
+
 typedef struct TranslateFault {
 int exception_index;
 int error_code;
 target_ulong cr2;
+TranslateFaultStage2 stage2;
 } TranslateFault;
 
-#define GET_HPHYS(cs, gpa, access_type, prot)  \
-   (in->use_stage2 ? get_hphys(cs, gpa, access_type, prot) : gpa)
+#define PTE_HPHYS(ADDR) \
+do {\
+if (in->use_stage2) {   \
+nested_in.addr = (ADDR);\
+if (!mmu_translate(env, _in, out, err)) {\
+err->stage2 = S2_GPT;   \
+return false;   \
+}   \
+(ADDR) = out->paddr;\
+}   \
+} while (0)
 
 static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
   TranslateResult *out, TranslateFault *err)
 {
+TranslateParams nested_in = {
+/* Use store for page table entries, to allow A/D flag updates. */
+.access_type = MMU_DATA_STORE,
+.cr3 = env->nested_cr3,
+.pg_mode = env->nested_pg_mode,
+.mmu_idx = MMU_USER_IDX,
+.use_stage2 = false,
+};
+
 CPUState *cs = env_cpu(env);
 X86CPU *cpu = env_archcpu(env);
 const int32_t a20_mask = x86_get_a20_mask(env);
@@ -79,7 +104,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 if (la57) {
 pml5e_addr = ((in->cr3 & ~0xfff) +
 (((addr >> 48) & 0x1ff) << 3)) & a20_mask;
-pml5e_addr = GET_HPHYS(cs, pml5e_addr, MMU_DATA_STORE, NULL);
+PTE_HPHYS(pml5e_addr);
 pml5e = x86_ldq_phys(cs, pml5e_addr);
 if (!(pml5e & PG_PRESENT_MASK)) {
 goto do_fault;
@@ -99,7 +124,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 
 pml4e_addr = ((pml5e & PG_ADDRESS_MASK) +
 (((addr >> 39) & 0x1ff) << 3)) & a20_mask;
-pml4e_addr = GET_HPHYS(cs, pml4e_addr, MMU_DATA_STORE, NULL);
+PTE_HPHYS(pml4e_addr);
 pml4e = x86_ldq_phys(cs, pml4e_addr);
 if (!(pml4e & PG_PRESENT_MASK)) {
 goto do_fault;
@@ -114,7 +139,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 ptep &= pml4e ^ PG_NX_MASK;
 pdpe_addr = ((pml4e & PG_ADDRESS_MASK) + (((addr >> 30) & 0x1ff) 
<< 3)) &
 a20_mask;
-pdpe_addr = GET_HPHYS(cs, pdpe_addr, MMU_DATA_STORE, NULL);
+PTE_HPHYS(pdpe_addr);
 pdpe = x86_ldq_phys(cs, pdpe_addr);
 if (!(pdpe & PG_PRESENT_MASK)) {
 goto do_fault;
@@ -140,7 +165,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 /* XXX: load them when cr3 is loaded ? */
 pdpe_addr = ((in->cr3 & ~0x1f) + ((addr >> 27) & 0x18)) &
 a20_mask;
-pdpe_addr = GET_HPHYS(cs, pdpe_addr, MMU_DATA_STORE, NULL);
+PTE_HPHYS(pdpe_addr);
 pdpe = x86_ldq_phys(cs, pdpe_addr);
 if (!(pdpe & PG_PRESENT_MASK)) {
 goto do_fault;
@@ -154,7 +179,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 
 pde_addr = ((pdpe & PG_ADDRESS_MASK) + (((addr >> 21) & 0x1ff) << 3)) &
 a20_mask;
-pde_addr = GET_HPHYS(cs, pde_addr, MMU_DATA_STORE, NULL);
+PTE_HPHYS(pde_addr);
 pde = x86_ldq_phys(cs, pde_addr);
 if (!(pde & PG_PRESENT_MASK)) {
 goto do_fault;
@@ -177,7 +202,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 }
 pte_addr = ((pde & PG_ADDRESS_MASK) + (((addr >> 12) & 0x1ff) << 3)) &
 a20_mask;
-pte_addr = 

[PATCH v2 2/9] target/i386: Direct call get_hphys from mmu_translate

[Richard Henderson]

Use a boolean to control the call to get_hphys instead
of passing a null function pointer.

Signed-off-by: Richard Henderson 
---
 target/i386/tcg/sysemu/excp_helper.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index eee59aa977..c9f6afba29 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -24,14 +24,10 @@
 
 #define PG_ERROR_OK (-1)
 
-typedef hwaddr (*MMUTranslateFunc)(CPUState *cs, hwaddr gphys, MMUAccessType 
access_type,
-   int *prot);
-
 #define GET_HPHYS(cs, gpa, access_type, prot)  \
-   (get_hphys_func ? get_hphys_func(cs, gpa, access_type, prot) : gpa)
+   (use_stage2 ? get_hphys(cs, gpa, access_type, prot) : gpa)
 
-static int mmu_translate(CPUState *cs, hwaddr addr,
- MMUTranslateFunc get_hphys_func,
+static int mmu_translate(CPUState *cs, hwaddr addr, bool use_stage2,
  uint64_t cr3, MMUAccessType access_type,
  int mmu_idx, int pg_mode,
  hwaddr *xlat, int *page_size, int *prot)
@@ -329,7 +325,7 @@ hwaddr get_hphys(CPUState *cs, hwaddr gphys, MMUAccessType 
access_type,
 return gphys;
 }
 
-exit_info_1 = mmu_translate(cs, gphys, NULL, env->nested_cr3,
+exit_info_1 = mmu_translate(cs, gphys, false, env->nested_cr3,
access_type, MMU_USER_IDX, env->nested_pg_mode,
, _size, _prot);
 if (exit_info_1 == PG_ERROR_OK) {
@@ -395,7 +391,7 @@ static int handle_mmu_fault(CPUState *cs, vaddr addr, int 
size,
 }
 }
 
-error_code = mmu_translate(cs, addr, get_hphys, env->cr[3], 
access_type,
+error_code = mmu_translate(cs, addr, true, env->cr[3], access_type,
mmu_idx, pg_mode,
, _size, );
 }
-- 
2.34.1

[PATCH v2 5/9] target/i386: Add MMU_PHYS_IDX and MMU_NESTED_IDX

[Richard Henderson]

These new mmu indexes will be helpful for improving
paging and code throughout the target.

Signed-off-by: Richard Henderson 
---
 target/i386/cpu-param.h  |  2 +-
 target/i386/cpu.h|  3 +
 target/i386/tcg/sysemu/excp_helper.c | 82 ++--
 target/i386/tcg/sysemu/svm_helper.c  |  3 +
 4 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/target/i386/cpu-param.h b/target/i386/cpu-param.h
index 9740bd7abd..abad52af20 100644
--- a/target/i386/cpu-param.h
+++ b/target/i386/cpu-param.h
@@ -23,6 +23,6 @@
 # define TARGET_VIRT_ADDR_SPACE_BITS  32
 #endif
 #define TARGET_PAGE_BITS 12
-#define NB_MMU_MODES 3
+#define NB_MMU_MODES 5
 
 #endif
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 82004b65b9..9a40b54ae5 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2144,6 +2144,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 #define MMU_KSMAP_IDX   0
 #define MMU_USER_IDX1
 #define MMU_KNOSMAP_IDX 2
+#define MMU_NESTED_IDX  3
+#define MMU_PHYS_IDX4
+
 static inline int cpu_mmu_index(CPUX86State *env, bool ifetch)
 {
 return (env->hflags & HF_CPL_MASK) == 3 ? MMU_USER_IDX :
diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 816b307547..494dc6d00c 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -448,41 +448,65 @@ static bool get_physical_address(CPUX86State *env, vaddr 
addr,
  MMUAccessType access_type, int mmu_idx,
  TranslateResult *out, TranslateFault *err)
 {
-if (!(env->cr[0] & CR0_PG_MASK)) {
-out->paddr = addr & x86_get_a20_mask(env);
+TranslateParams in;
+bool use_stage2 = env->hflags2 & HF2_NPT_MASK;
 
-#ifdef TARGET_X86_64
-if (!(env->hflags & HF_LMA_MASK)) {
-/* Without long mode we can only address 32bits in real mode */
-out->paddr = (uint32_t)out->paddr;
-}
-#endif
-out->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
-out->page_size = TARGET_PAGE_SIZE;
-return true;
-} else {
-TranslateParams in = {
-.addr = addr,
-.cr3 = env->cr[3],
-.pg_mode = get_pg_mode(env),
-.mmu_idx = mmu_idx,
-.access_type = access_type,
-.use_stage2 = env->hflags2 & HF2_NPT_MASK,
-};
+in.addr = addr;
+in.access_type = access_type;
 
-if (in.pg_mode & PG_MODE_LMA) {
-/* test virtual address sign extension */
-int shift = in.pg_mode & PG_MODE_LA57 ? 56 : 47;
-int64_t sext = (int64_t)addr >> shift;
-if (sext != 0 && sext != -1) {
-err->exception_index = EXCP0D_GPF;
-err->error_code = 0;
-err->cr2 = addr;
+switch (mmu_idx) {
+case MMU_PHYS_IDX:
+break;
+
+case MMU_NESTED_IDX:
+if (likely(use_stage2)) {
+in.cr3 = env->nested_cr3;
+in.pg_mode = env->nested_pg_mode;
+in.mmu_idx = MMU_USER_IDX;
+in.use_stage2 = false;
+
+if (!mmu_translate(env, , out, err)) {
+err->stage2 = S2_GPA;
 return false;
 }
+return true;
 }
-return mmu_translate(env, , out, err);
+break;
+
+default:
+in.cr3 = env->cr[3];
+in.mmu_idx = mmu_idx;
+in.use_stage2 = use_stage2;
+in.pg_mode = get_pg_mode(env);
+
+if (likely(in.pg_mode)) {
+if (in.pg_mode & PG_MODE_LMA) {
+/* test virtual address sign extension */
+int shift = in.pg_mode & PG_MODE_LA57 ? 56 : 47;
+int64_t sext = (int64_t)addr >> shift;
+if (sext != 0 && sext != -1) {
+err->exception_index = EXCP0D_GPF;
+err->error_code = 0;
+err->cr2 = addr;
+return false;
+}
+}
+return mmu_translate(env, , out, err);
+}
+break;
 }
+
+/* Translation disabled. */
+out->paddr = addr & x86_get_a20_mask(env);
+#ifdef TARGET_X86_64
+if (!(env->hflags & HF_LMA_MASK)) {
+/* Without long mode we can only address 32bits in real mode */
+out->paddr = (uint32_t)out->paddr;
+}
+#endif
+out->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+out->page_size = TARGET_PAGE_SIZE;
+return true;
 }
 
 bool x86_cpu_tlb_fill(CPUState *cs, vaddr addr, int size,
diff --git a/target/i386/tcg/sysemu/svm_helper.c 
b/target/i386/tcg/sysemu/svm_helper.c
index 2b6f450af9..85b7741d94 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -271,6 +271,8 @@ void helper_vmrun(CPUX86State *env, int aflag, int 
next_eip_addend)
 env->hflags2 |= HF2_NPT_MASK;
 
 env->nested_pg_mode = get_pg_mode(env) & PG_MODE_SVM_MASK;
+
+  

[PATCH v2 3/9] target/i386: Introduce structures for mmu_translate

[Richard Henderson]

Create TranslateParams for inputs, TranslateResults for successful
outputs, and TranslateFault for error outputs; return true on success.

Move stage1 error paths from handle_mmu_fault to x86_cpu_tlb_fill;
reorg the rest of handle_mmu_fault into get_physical_address.

Signed-off-by: Richard Henderson 
---
 target/i386/tcg/sysemu/excp_helper.c | 322 ++-
 1 file changed, 171 insertions(+), 151 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index c9f6afba29..00ce4cf253 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -22,30 +22,45 @@
 #include "exec/exec-all.h"
 #include "tcg/helper-tcg.h"
 
-#define PG_ERROR_OK (-1)
+typedef struct TranslateParams {
+target_ulong addr;
+target_ulong cr3;
+int pg_mode;
+int mmu_idx;
+MMUAccessType access_type;
+bool use_stage2;
+} TranslateParams;
+
+typedef struct TranslateResult {
+hwaddr paddr;
+int prot;
+int page_size;
+} TranslateResult;
+
+typedef struct TranslateFault {
+int exception_index;
+int error_code;
+target_ulong cr2;
+} TranslateFault;
 
 #define GET_HPHYS(cs, gpa, access_type, prot)  \
-   (use_stage2 ? get_hphys(cs, gpa, access_type, prot) : gpa)
+   (in->use_stage2 ? get_hphys(cs, gpa, access_type, prot) : gpa)
 
-static int mmu_translate(CPUState *cs, hwaddr addr, bool use_stage2,
- uint64_t cr3, MMUAccessType access_type,
- int mmu_idx, int pg_mode,
- hwaddr *xlat, int *page_size, int *prot)
+static bool mmu_translate(CPUX86State *env, const TranslateParams *in,
+  TranslateResult *out, TranslateFault *err)
 {
-X86CPU *cpu = X86_CPU(cs);
-CPUX86State *env = >env;
+CPUState *cs = env_cpu(env);
+X86CPU *cpu = env_archcpu(env);
+const int32_t a20_mask = x86_get_a20_mask(env);
+const target_ulong addr = in->addr;
+const int pg_mode = in->pg_mode;
+const bool is_user = (in->mmu_idx == MMU_USER_IDX);
+const MMUAccessType access_type = in->access_type;
 uint64_t ptep, pte;
-int32_t a20_mask;
-target_ulong pde_addr, pte_addr;
-int error_code = 0;
-bool is_dirty, is_write, is_user;
+hwaddr pde_addr, pte_addr;
 uint64_t rsvd_mask = PG_ADDRESS_MASK & ~MAKE_64BIT_MASK(0, cpu->phys_bits);
-uint32_t page_offset;
 uint32_t pkr;
-
-is_user = (mmu_idx == MMU_USER_IDX);
-is_write = (access_type == MMU_DATA_STORE);
-a20_mask = x86_get_a20_mask(env);
+int page_size;
 
 if (!(pg_mode & PG_MODE_NXE)) {
 rsvd_mask |= PG_NX_MASK;
@@ -62,7 +77,7 @@ static int mmu_translate(CPUState *cs, hwaddr addr, bool 
use_stage2,
 uint64_t pml4e_addr, pml4e;
 
 if (la57) {
-pml5e_addr = ((cr3 & ~0xfff) +
+pml5e_addr = ((in->cr3 & ~0xfff) +
 (((addr >> 48) & 0x1ff) << 3)) & a20_mask;
 pml5e_addr = GET_HPHYS(cs, pml5e_addr, MMU_DATA_STORE, NULL);
 pml5e = x86_ldq_phys(cs, pml5e_addr);
@@ -78,7 +93,7 @@ static int mmu_translate(CPUState *cs, hwaddr addr, bool 
use_stage2,
 }
 ptep = pml5e ^ PG_NX_MASK;
 } else {
-pml5e = cr3;
+pml5e = in->cr3;
 ptep = PG_NX_MASK | PG_USER_MASK | PG_RW_MASK;
 }
 
@@ -114,7 +129,7 @@ static int mmu_translate(CPUState *cs, hwaddr addr, bool 
use_stage2,
 }
 if (pdpe & PG_PSE_MASK) {
 /* 1 GB page */
-*page_size = 1024 * 1024 * 1024;
+page_size = 1024 * 1024 * 1024;
 pte_addr = pdpe_addr;
 pte = pdpe;
 goto do_check_protect;
@@ -123,7 +138,7 @@ static int mmu_translate(CPUState *cs, hwaddr addr, bool 
use_stage2,
 #endif
 {
 /* XXX: load them when cr3 is loaded ? */
-pdpe_addr = ((cr3 & ~0x1f) + ((addr >> 27) & 0x18)) &
+pdpe_addr = ((in->cr3 & ~0x1f) + ((addr >> 27) & 0x18)) &
 a20_mask;
 pdpe_addr = GET_HPHYS(cs, pdpe_addr, MMU_DATA_STORE, NULL);
 pdpe = x86_ldq_phys(cs, pdpe_addr);
@@ -150,7 +165,7 @@ static int mmu_translate(CPUState *cs, hwaddr addr, bool 
use_stage2,
 ptep &= pde ^ PG_NX_MASK;
 if (pde & PG_PSE_MASK) {
 /* 2 MB page */
-*page_size = 2048 * 1024;
+page_size = 2048 * 1024;
 pte_addr = pde_addr;
 pte = pde;
 goto do_check_protect;
@@ -172,12 +187,12 @@ static int mmu_translate(CPUState *cs, hwaddr addr, bool 
use_stage2,
 }
 /* combine pde and pte nx, user and rw protections */
 ptep &= pte ^ PG_NX_MASK;
-*page_size = 4096;
+page_size = 4096;
 } else {
 uint32_t pde;
 
 /* page directory entry */
-pde_addr 

[PATCH v2 0/9] target/i386: Use atomic operations for pte updates

[Richard Henderson]

Use atomic operations for pte updates, which is a long-standing
bug since our conversion to MTTCG.  Modulo rebase, this has one
change from v1, which is the new patch 9.


r~


Based-on: 20220930212622.108363-1-richard.hender...@linaro.org
("[PATCH v6 00/18] tcg: CPUTLBEntryFull and TARGET_TB_PCREL")


Richard Henderson (9):
  target/i386: Use MMUAccessType across excp_helper.c
  target/i386: Direct call get_hphys from mmu_translate
  target/i386: Introduce structures for mmu_translate
  target/i386: Reorg GET_HPHYS
  target/i386: Add MMU_PHYS_IDX and MMU_NESTED_IDX
  target/i386: Use MMU_NESTED_IDX for vmload/vmsave
  target/i386: Combine 5 sets of variables in mmu_translate
  target/i386: Use atomic operations for pte updates
  target/i386: Use probe_access_full for final stage2 translation

 target/i386/cpu-param.h  |   2 +-
 target/i386/cpu.h|   5 +-
 target/i386/tcg/sysemu/excp_helper.c | 706 +--
 target/i386/tcg/sysemu/svm_helper.c  | 234 +
 4 files changed, 581 insertions(+), 366 deletions(-)

-- 
2.34.1

[PATCH v2 6/9] target/i386: Use MMU_NESTED_IDX for vmload/vmsave

[Richard Henderson]

Use MMU_NESTED_IDX for each memory access, rather than
just a single translation to physical.  Adjust svm_save_seg
and svm_load_seg to pass in mmu_idx.

This removes the last use of get_hphys so remove it.

Signed-off-by: Richard Henderson 
---
 target/i386/cpu.h|   2 -
 target/i386/tcg/sysemu/excp_helper.c |  31 
 target/i386/tcg/sysemu/svm_helper.c  | 231 +++
 3 files changed, 126 insertions(+), 138 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 9a40b54ae5..10a5e79774 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2381,8 +2381,6 @@ static inline bool ctl_has_irq(CPUX86State *env)
 return (env->int_ctl & V_IRQ_MASK) && (int_prio >= tpr);
 }
 
-hwaddr get_hphys(CPUState *cs, hwaddr gphys, MMUAccessType access_type,
-int *prot);
 #if defined(TARGET_X86_64) && \
 defined(CONFIG_USER_ONLY) && \
 defined(CONFIG_LINUX)
diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 494dc6d00c..86b3014196 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -413,37 +413,6 @@ static G_NORETURN void raise_stage2(CPUX86State *env, 
TranslateFault *err,
 cpu_vmexit(env, SVM_EXIT_NPF, exit_info_1, retaddr);
 }
 
-hwaddr get_hphys(CPUState *cs, hwaddr gphys, MMUAccessType access_type,
- int *prot)
-{
-CPUX86State *env = cs->env_ptr;
-
-if (likely(!(env->hflags2 & HF2_NPT_MASK))) {
-return gphys;
-} else {
-TranslateParams in = {
-.addr = gphys,
-.cr3 = env->nested_cr3,
-.pg_mode = env->nested_pg_mode,
-.mmu_idx = MMU_USER_IDX,
-.access_type = access_type,
-.use_stage2 = false,
-};
-TranslateResult out;
-TranslateFault err;
-
-if (!mmu_translate(env, , , )) {
-err.stage2 = prot ? SVM_NPTEXIT_GPA : SVM_NPTEXIT_GPT;
-raise_stage2(env, , env->retaddr);
-}
-
-if (prot) {
-*prot &= out.prot;
-}
-return out.paddr;
-}
-}
-
 static bool get_physical_address(CPUX86State *env, vaddr addr,
  MMUAccessType access_type, int mmu_idx,
  TranslateResult *out, TranslateFault *err)
diff --git a/target/i386/tcg/sysemu/svm_helper.c 
b/target/i386/tcg/sysemu/svm_helper.c
index 85b7741d94..8e88567399 100644
--- a/target/i386/tcg/sysemu/svm_helper.c
+++ b/target/i386/tcg/sysemu/svm_helper.c
@@ -27,19 +27,19 @@
 
 /* Secure Virtual Machine helpers */
 
-static inline void svm_save_seg(CPUX86State *env, hwaddr addr,
-const SegmentCache *sc)
+static void svm_save_seg(CPUX86State *env, int mmu_idx, hwaddr addr,
+ const SegmentCache *sc)
 {
-CPUState *cs = env_cpu(env);
-
-x86_stw_phys(cs, addr + offsetof(struct vmcb_seg, selector),
- sc->selector);
-x86_stq_phys(cs, addr + offsetof(struct vmcb_seg, base),
- sc->base);
-x86_stl_phys(cs, addr + offsetof(struct vmcb_seg, limit),
- sc->limit);
-x86_stw_phys(cs, addr + offsetof(struct vmcb_seg, attrib),
- ((sc->flags >> 8) & 0xff) | ((sc->flags >> 12) & 0x0f00));
+cpu_stw_mmuidx_ra(env, addr + offsetof(struct vmcb_seg, selector),
+  sc->selector, mmu_idx, 0);
+cpu_stq_mmuidx_ra(env, addr + offsetof(struct vmcb_seg, base),
+  sc->base, mmu_idx, 0);
+cpu_stl_mmuidx_ra(env, addr + offsetof(struct vmcb_seg, limit),
+  sc->limit, mmu_idx, 0);
+cpu_stw_mmuidx_ra(env, addr + offsetof(struct vmcb_seg, attrib),
+  ((sc->flags >> 8) & 0xff)
+  | ((sc->flags >> 12) & 0x0f00),
+  mmu_idx, 0);
 }
 
 /*
@@ -52,29 +52,36 @@ static inline void svm_canonicalization(CPUX86State *env, 
target_ulong *seg_base
 *seg_base = long) *seg_base) << shift_amt) >> shift_amt);
 }
 
-static inline void svm_load_seg(CPUX86State *env, hwaddr addr,
-SegmentCache *sc)
+static void svm_load_seg(CPUX86State *env, int mmu_idx, hwaddr addr,
+ SegmentCache *sc)
 {
-CPUState *cs = env_cpu(env);
 unsigned int flags;
 
-sc->selector = x86_lduw_phys(cs,
- addr + offsetof(struct vmcb_seg, selector));
-sc->base = x86_ldq_phys(cs, addr + offsetof(struct vmcb_seg, base));
-sc->limit = x86_ldl_phys(cs, addr + offsetof(struct vmcb_seg, limit));
-flags = x86_lduw_phys(cs, addr + offsetof(struct vmcb_seg, attrib));
+sc->selector =
+cpu_lduw_mmuidx_ra(env, addr + offsetof(struct vmcb_seg, selector),
+   mmu_idx, 0);
+sc->base =
+cpu_ldq_mmuidx_ra(env, addr + offsetof(struct vmcb_seg, base),
+  mmu_idx, 0);
+sc->limit =
+

[PATCH v2 7/9] target/i386: Combine 5 sets of variables in mmu_translate

[Richard Henderson]

We don't need one variable set per translation level,
which requires copying into pte/pte_addr for huge pages.
Standardize on pte/pte_addr for all levels.

Signed-off-by: Richard Henderson 
---
 target/i386/tcg/sysemu/excp_helper.c | 178 ++-
 1 file changed, 91 insertions(+), 87 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 86b3014196..d6b7de6eea 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -82,7 +82,7 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 const bool is_user = (in->mmu_idx == MMU_USER_IDX);
 const MMUAccessType access_type = in->access_type;
 uint64_t ptep, pte;
-hwaddr pde_addr, pte_addr;
+hwaddr pte_addr;
 uint64_t rsvd_mask = PG_ADDRESS_MASK & ~MAKE_64BIT_MASK(0, cpu->phys_bits);
 uint32_t pkr;
 int page_size;
@@ -92,116 +92,122 @@ static bool mmu_translate(CPUX86State *env, const 
TranslateParams *in,
 }
 
 if (pg_mode & PG_MODE_PAE) {
-uint64_t pde, pdpe;
-target_ulong pdpe_addr;
-
 #ifdef TARGET_X86_64
 if (pg_mode & PG_MODE_LMA) {
-bool la57 = pg_mode & PG_MODE_LA57;
-uint64_t pml5e_addr, pml5e;
-uint64_t pml4e_addr, pml4e;
-
-if (la57) {
-pml5e_addr = ((in->cr3 & ~0xfff) +
-(((addr >> 48) & 0x1ff) << 3)) & a20_mask;
-PTE_HPHYS(pml5e_addr);
-pml5e = x86_ldq_phys(cs, pml5e_addr);
-if (!(pml5e & PG_PRESENT_MASK)) {
+if (pg_mode & PG_MODE_LA57) {
+/*
+ * Page table level 5
+ */
+pte_addr = ((in->cr3 & ~0xfff) +
+(((addr >> 48) & 0x1ff) << 3)) & a20_mask;
+PTE_HPHYS(pte_addr);
+pte = x86_ldq_phys(cs, pte_addr);
+if (!(pte & PG_PRESENT_MASK)) {
 goto do_fault;
 }
-if (pml5e & (rsvd_mask | PG_PSE_MASK)) {
+if (pte & (rsvd_mask | PG_PSE_MASK)) {
 goto do_fault_rsvd;
 }
-if (!(pml5e & PG_ACCESSED_MASK)) {
-pml5e |= PG_ACCESSED_MASK;
-x86_stl_phys_notdirty(cs, pml5e_addr, pml5e);
+if (!(pte & PG_ACCESSED_MASK)) {
+pte |= PG_ACCESSED_MASK;
+x86_stl_phys_notdirty(cs, pte_addr, pte);
 }
-ptep = pml5e ^ PG_NX_MASK;
+ptep = pte ^ PG_NX_MASK;
 } else {
-pml5e = in->cr3;
+pte = in->cr3;
 ptep = PG_NX_MASK | PG_USER_MASK | PG_RW_MASK;
 }
 
-pml4e_addr = ((pml5e & PG_ADDRESS_MASK) +
-(((addr >> 39) & 0x1ff) << 3)) & a20_mask;
-PTE_HPHYS(pml4e_addr);
-pml4e = x86_ldq_phys(cs, pml4e_addr);
-if (!(pml4e & PG_PRESENT_MASK)) {
+/*
+ * Page table level 4
+ */
+pte_addr = ((pte & PG_ADDRESS_MASK) +
+(((addr >> 39) & 0x1ff) << 3)) & a20_mask;
+PTE_HPHYS(pte_addr);
+pte = x86_ldq_phys(cs, pte_addr);
+if (!(pte & PG_PRESENT_MASK)) {
 goto do_fault;
 }
-if (pml4e & (rsvd_mask | PG_PSE_MASK)) {
+if (pte & (rsvd_mask | PG_PSE_MASK)) {
 goto do_fault_rsvd;
 }
-if (!(pml4e & PG_ACCESSED_MASK)) {
-pml4e |= PG_ACCESSED_MASK;
-x86_stl_phys_notdirty(cs, pml4e_addr, pml4e);
+if (!(pte & PG_ACCESSED_MASK)) {
+pte |= PG_ACCESSED_MASK;
+x86_stl_phys_notdirty(cs, pte_addr, pte);
 }
-ptep &= pml4e ^ PG_NX_MASK;
-pdpe_addr = ((pml4e & PG_ADDRESS_MASK) + (((addr >> 30) & 0x1ff) 
<< 3)) &
-a20_mask;
-PTE_HPHYS(pdpe_addr);
-pdpe = x86_ldq_phys(cs, pdpe_addr);
-if (!(pdpe & PG_PRESENT_MASK)) {
+ptep &= pte ^ PG_NX_MASK;
+
+/*
+ * Page table level 3
+ */
+pte_addr = ((pte & PG_ADDRESS_MASK) +
+(((addr >> 30) & 0x1ff) << 3)) & a20_mask;
+PTE_HPHYS(pte_addr);
+pte = x86_ldq_phys(cs, pte_addr);
+if (!(pte & PG_PRESENT_MASK)) {
 goto do_fault;
 }
-if (pdpe & rsvd_mask) {
+if (pte & rsvd_mask) {
 goto do_fault_rsvd;
 }
-ptep &= pdpe ^ PG_NX_MASK;
-if (!(pdpe & PG_ACCESSED_MASK)) {
-pdpe |= PG_ACCESSED_MASK;
-x86_stl_phys_notdirty(cs, pdpe_addr, pdpe);
+ptep &= pte ^ PG_NX_MASK;
+if (!(pte & 

[PATCH v2 1/9] target/i386: Use MMUAccessType across excp_helper.c

[Richard Henderson]

Replace int is_write1 and magic numbers with the proper
MMUAccessType access_type and enumerators.

Signed-off-by: Richard Henderson 
---
 target/i386/tcg/sysemu/excp_helper.c | 28 +++-
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 796dc2a1f3..eee59aa977 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -30,8 +30,10 @@ typedef hwaddr (*MMUTranslateFunc)(CPUState *cs, hwaddr 
gphys, MMUAccessType acc
 #define GET_HPHYS(cs, gpa, access_type, prot)  \
(get_hphys_func ? get_hphys_func(cs, gpa, access_type, prot) : gpa)
 
-static int mmu_translate(CPUState *cs, hwaddr addr, MMUTranslateFunc 
get_hphys_func,
- uint64_t cr3, int is_write1, int mmu_idx, int pg_mode,
+static int mmu_translate(CPUState *cs, hwaddr addr,
+ MMUTranslateFunc get_hphys_func,
+ uint64_t cr3, MMUAccessType access_type,
+ int mmu_idx, int pg_mode,
  hwaddr *xlat, int *page_size, int *prot)
 {
 X86CPU *cpu = X86_CPU(cs);
@@ -40,13 +42,13 @@ static int mmu_translate(CPUState *cs, hwaddr addr, 
MMUTranslateFunc get_hphys_f
 int32_t a20_mask;
 target_ulong pde_addr, pte_addr;
 int error_code = 0;
-int is_dirty, is_write, is_user;
+bool is_dirty, is_write, is_user;
 uint64_t rsvd_mask = PG_ADDRESS_MASK & ~MAKE_64BIT_MASK(0, cpu->phys_bits);
 uint32_t page_offset;
 uint32_t pkr;
 
 is_user = (mmu_idx == MMU_USER_IDX);
-is_write = is_write1 & 1;
+is_write = (access_type == MMU_DATA_STORE);
 a20_mask = x86_get_a20_mask(env);
 
 if (!(pg_mode & PG_MODE_NXE)) {
@@ -264,14 +266,14 @@ do_check_protect_pse36:
 }
 
 *prot &= pkr_prot;
-if ((pkr_prot & (1 << is_write1)) == 0) {
-assert(is_write1 != 2);
+if ((pkr_prot & (1 << access_type)) == 0) {
+assert(access_type != MMU_INST_FETCH);
 error_code |= PG_ERROR_PK_MASK;
 goto do_fault_protect;
 }
 }
 
-if ((*prot & (1 << is_write1)) == 0) {
+if ((*prot & (1 << access_type)) == 0) {
 goto do_fault_protect;
 }
 
@@ -297,7 +299,7 @@ do_check_protect_pse36:
 /* align to page_size */
 pte &= PG_ADDRESS_MASK & ~(*page_size - 1);
 page_offset = addr & (*page_size - 1);
-*xlat = GET_HPHYS(cs, pte + page_offset, is_write1, prot);
+*xlat = GET_HPHYS(cs, pte + page_offset, access_type, prot);
 return PG_ERROR_OK;
 
  do_fault_rsvd:
@@ -308,7 +310,7 @@ do_check_protect_pse36:
 error_code |= (is_write << PG_ERROR_W_BIT);
 if (is_user)
 error_code |= PG_ERROR_U_MASK;
-if (is_write1 == 2 &&
+if (access_type == MMU_INST_FETCH &&
 ((pg_mode & PG_MODE_NXE) || (pg_mode & PG_MODE_SMEP)))
 error_code |= PG_ERROR_I_D_MASK;
 return error_code;
@@ -353,7 +355,7 @@ hwaddr get_hphys(CPUState *cs, hwaddr gphys, MMUAccessType 
access_type,
  * 1  = generate PF fault
  */
 static int handle_mmu_fault(CPUState *cs, vaddr addr, int size,
-int is_write1, int mmu_idx)
+MMUAccessType access_type, int mmu_idx)
 {
 X86CPU *cpu = X86_CPU(cs);
 CPUX86State *env = >env;
@@ -365,7 +367,7 @@ static int handle_mmu_fault(CPUState *cs, vaddr addr, int 
size,
 
 #if defined(DEBUG_MMU)
 printf("MMU fault: addr=%" VADDR_PRIx " w=%d mmu=%d eip=" TARGET_FMT_lx 
"\n",
-   addr, is_write1, mmu_idx, env->eip);
+   addr, access_type, mmu_idx, env->eip);
 #endif
 
 if (!(env->cr[0] & CR0_PG_MASK)) {
@@ -393,7 +395,7 @@ static int handle_mmu_fault(CPUState *cs, vaddr addr, int 
size,
 }
 }
 
-error_code = mmu_translate(cs, addr, get_hphys, env->cr[3], is_write1,
+error_code = mmu_translate(cs, addr, get_hphys, env->cr[3], 
access_type,
mmu_idx, pg_mode,
, _size, );
 }
@@ -404,7 +406,7 @@ static int handle_mmu_fault(CPUState *cs, vaddr addr, int 
size,
 vaddr = addr & TARGET_PAGE_MASK;
 paddr &= TARGET_PAGE_MASK;
 
-assert(prot & (1 << is_write1));
+assert(prot & (1 << access_type));
 tlb_set_page_with_attrs(cs, vaddr, paddr, cpu_get_mem_attrs(env),
 prot, mmu_idx, page_size);
 return 0;
-- 
2.34.1

Re: [PATCH v2] target/sh4: Fix TB_FLAG_UNALIGN

[Richard Henderson]

Ping, or should I create a PR myself?

r~

On 9/1/22 07:15, Yoshinori Sato wrote:

On Thu, 01 Sep 2022 19:15:09 +0900,
Richard Henderson wrote:


The value previously chosen overlaps GUSA_MASK.

Rename all DELAY_SLOT_* and GUSA_* defines to emphasize
that they are included in TB_FLAGs.  Add aliases for the
FPSCR and SR bits that are included in TB_FLAGS, so that
we don't accidentally reassign those bits.

Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856
Signed-off-by: Richard Henderson 
---
  target/sh4/cpu.h| 56 +
  linux-user/sh4/signal.c |  6 +--
  target/sh4/cpu.c|  6 +--
  target/sh4/helper.c |  6 +--
  target/sh4/translate.c  | 90 ++---
  5 files changed, 88 insertions(+), 76 deletions(-)

diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h
index 9f15ef913c..727b829598 100644
--- a/target/sh4/cpu.h
+++ b/target/sh4/cpu.h
@@ -78,26 +78,33 @@
  #define FPSCR_RM_NEAREST   (0 << 0)
  #define FPSCR_RM_ZERO  (1 << 0)
  
-#define DELAY_SLOT_MASK0x7

-#define DELAY_SLOT (1 << 0)
-#define DELAY_SLOT_CONDITIONAL (1 << 1)
-#define DELAY_SLOT_RTE (1 << 2)
+#define TB_FLAG_DELAY_SLOT   (1 << 0)
+#define TB_FLAG_DELAY_SLOT_COND  (1 << 1)
+#define TB_FLAG_DELAY_SLOT_RTE   (1 << 2)
+#define TB_FLAG_PENDING_MOVCA(1 << 3)
+#define TB_FLAG_GUSA_SHIFT   4  /* [11:4] */
+#define TB_FLAG_GUSA_EXCLUSIVE   (1 << 12)
+#define TB_FLAG_UNALIGN  (1 << 13)
+#define TB_FLAG_SR_FD(1 << SR_FD)   /* 15 */
+#define TB_FLAG_FPSCR_PR FPSCR_PR   /* 19 */
+#define TB_FLAG_FPSCR_SZ FPSCR_SZ   /* 20 */
+#define TB_FLAG_FPSCR_FR FPSCR_FR   /* 21 */
+#define TB_FLAG_SR_RB(1 << SR_RB)   /* 29 */
+#define TB_FLAG_SR_MD(1 << SR_MD)   /* 30 */
  
-#define TB_FLAG_PENDING_MOVCA  (1 << 3)

-#define TB_FLAG_UNALIGN(1 << 4)
-
-#define GUSA_SHIFT 4
-#ifdef CONFIG_USER_ONLY
-#define GUSA_EXCLUSIVE (1 << 12)
-#define GUSA_MASK  ((0xff << GUSA_SHIFT) | GUSA_EXCLUSIVE)
-#else
-/* Provide dummy versions of the above to allow tests against tbflags
-   to be elided while avoiding ifdefs.  */
-#define GUSA_EXCLUSIVE 0
-#define GUSA_MASK  0
-#endif
-
-#define TB_FLAG_ENVFLAGS_MASK  (DELAY_SLOT_MASK | GUSA_MASK)
+#define TB_FLAG_DELAY_SLOT_MASK  (TB_FLAG_DELAY_SLOT |   \
+  TB_FLAG_DELAY_SLOT_COND |  \
+  TB_FLAG_DELAY_SLOT_RTE)
+#define TB_FLAG_GUSA_MASK((0xff << TB_FLAG_GUSA_SHIFT) | \
+  TB_FLAG_GUSA_EXCLUSIVE)
+#define TB_FLAG_FPSCR_MASK   (TB_FLAG_FPSCR_PR | \
+  TB_FLAG_FPSCR_SZ | \
+  TB_FLAG_FPSCR_FR)
+#define TB_FLAG_SR_MASK  (TB_FLAG_SR_FD | \
+  TB_FLAG_SR_RB | \
+  TB_FLAG_SR_MD)
+#define TB_FLAG_ENVFLAGS_MASK(TB_FLAG_DELAY_SLOT_MASK | \
+  TB_FLAG_GUSA_MASK)
  
  typedef struct tlb_t {

  uint32_t vpn; /* virtual page number */
@@ -258,7 +265,7 @@ static inline int cpu_mmu_index (CPUSH4State *env, bool 
ifetch)
  {
  /* The instruction in a RTE delay slot is fetched in privileged
 mode, but executed in user mode.  */
-if (ifetch && (env->flags & DELAY_SLOT_RTE)) {
+if (ifetch && (env->flags & TB_FLAG_DELAY_SLOT_RTE)) {
  return 0;
  } else {
  return (env->sr & (1u << SR_MD)) == 0 ? 1 : 0;
@@ -366,11 +373,10 @@ static inline void cpu_get_tb_cpu_state(CPUSH4State *env, 
target_ulong *pc,
  {
  *pc = env->pc;
  /* For a gUSA region, notice the end of the region.  */
-*cs_base = env->flags & GUSA_MASK ? env->gregs[0] : 0;
-*flags = env->flags /* TB_FLAG_ENVFLAGS_MASK: bits 0-2, 4-12 */
-| (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR))  /* Bits 19-21 */
-| (env->sr & ((1u << SR_MD) | (1u << SR_RB)))  /* Bits 29-30 */
-| (env->sr & (1u << SR_FD))/* Bit 15 */
+*cs_base = env->flags & TB_FLAG_GUSA_MASK ? env->gregs[0] : 0;
+*flags = env->flags
+| (env->fpscr & TB_FLAG_FPSCR_MASK)
+| (env->sr & TB_FLAG_SR_MASK)
  | (env->movcal_backup ? TB_FLAG_PENDING_MOVCA : 0); /* Bit 3 */
  #ifdef CONFIG_USER_ONLY
  *flags |= TB_FLAG_UNALIGN * !env_cpu(env)->prctl_unalign_sigbus;
diff --git a/linux-user/sh4/signal.c b/linux-user/sh4/signal.c
index f6a18bc6b5..c4ba962708 100644
--- a/linux-user/sh4/signal.c
+++ b/linux-user/sh4/signal.c
@@ -161,7 +161,7 @@ static void restore_sigcontext(CPUSH4State *regs, struct 
target_sigcontext *sc)
  __get_user(regs->fpul, >sc_fpul);
  
  regs->tra = -1;   

Re: access guest address from within instruction

[Richard Henderson]

On 10/2/22 07:52, BitFriends wrote:
my bad, then I was mislead by "Which is, in general, what you want for implementing a 
custom instruction". Also the code around me is full of gen instructions, so I thought 
that's what I should use.


So, when reading the doc I found out about the cpu_{ld,st}*_mmu functions. That sounds 
more what I want for a direct action, No?


That all depends on where you're putting the code.  Based on what you've written so far, 
I'd guess the answer is still no.  But you haven't been overly verbose about what you're 
trying to do.



r~



Regards

BitFriends

Richard Henderson mailto:richard.hender...@linaro.org>> 
schrieb am So., 2. Okt. 2022, 16:40:


On 10/2/22 02:20, BitFriends wrote:
 > I now came up with this code:
 >
 > TCGv_i64 res = 0;
 > TCGv_i64 addr = (TCGv_i64)(env->regs[R_EDI]);
 >
 > tcg_gen_qemu_ld_i64(res, addr, 0, MO_LEUQ);
 >
 > env->regs[R_EAX] = (target_ulong)res;
 >
 > However this crashes afterwards in test_bit. Maybe this is caused by an 
invalid
access?
 > Anything wrong about the code? This still gives some warnings, like 
TCGv_i32
expected (and
 > when you use TCGv_i32, it says TCGv_i64 expected) plus some casting 
warnings.

It is as if you did not read the second paragraph of my response at all.
tcg_gen_qemu_ld_i64 is for generating code, not performing a direct action.
Can you see how your code differs from *all* of the code around it?

r~

 >
 > Am Sa., 1. Okt. 2022 um 22:23 Uhr schrieb Richard Henderson
mailto:richard.hender...@linaro.org>
 > >>:
 >
 >     On 10/1/22 13:10, BitFriends wrote:
 >      > Hello,
 >      >
 >      > I am trying to create a custom instruction that accesses guest 
memory
specified by an
 >      > address in a register. I specifically want to read from that 
address. So I
tried to do
 >      > that using "tcg_gen_qemu_ld_i64(, env->regs[R_EDI], 0, 
MO_LEUQ);", but that
 >     doesn't
 >      > save any result in res.
 >
 >     This statement should have given you compilation errors, so I don't 
know what
you mean by
 >     "doesn't save any result".  There's clearly a disconnect between 
what you
describe and
 >     what you actually attempted.
 >
 >     Anyway, by the name you can see that function "gen"erates a "tcg" 
operation,
which is
 >     then
 >     later compiled by the jit, the output of which is later executed to 
produce a
result.
 >     Which is, in general, what you want for implementing a custom 
instruction.
 >
 >
 >     r~
 >

Re: access guest address from within instruction

[BitFriends]

thanks for the clarification, I will look at those insns. My instruction is
for some more advanced logging between guest and host, that should be done
quickly.

Regards

BitFriends

Peter Maydell  schrieb am So., 2. Okt. 2022,
16:45:

> On Sun, 2 Oct 2022 at 10:22, BitFriends  wrote:
> > I now came up with this code:
> >
> > TCGv_i64 res = 0;
> > TCGv_i64 addr = (TCGv_i64)(env->regs[R_EDI]);
> >
> > tcg_gen_qemu_ld_i64(res, addr, 0, MO_LEUQ);
> >
> > env->regs[R_EAX] = (target_ulong)res;
>
> This is wrong, because you cannot read or write env->regs[] at
> translate time. The "translate time" vs "run time" distinction
> in a JIT is critical to understand:
>
>  * translate time is when we read guest instructions,
>and output TCG ops. We do this once, the first time
>we encounter a particular piece of guest code. At
>this point you cannot directly access the state of the
>guest CPU. This is because the exact value of EDI
>will be *different* each time the generated code is run.
>  * run time is when we are actually emulating the guest
>CPU, by executing the code built from the TCG ops.
>At run time the CPU state is known and can be updated.
>
> You should look at how existing instructions in the x86
> translator generate code to read and write registers --
> you will see that they use cpu_regs[], which is an array
> of TCGv which correspond to the CPU registers (so they can
> say "generate code which will read EDI"), and they
> never use env->regs[] (which would be "read EDI right now").
>
> More generally, "read guest memory and get the result into
> a guest CPU register" is a common thing in existing x86
> instructions. So find how we implement one of those
> existing insns that's similar to what you want, and see
> how that is handled.
>
> (NB: so far you haven't said why your custom instruction
> would be any different from a normal load instruction
> that x86 already has...)
>
> thanks
> -- PMM
>

Re: access guest address from within instruction

[BitFriends]

my bad, then I was mislead by "Which is, in general, what you want for
implementing a custom instruction". Also the code around me is full of gen
instructions, so I thought that's what I should use.

So, when reading the doc I found out about the cpu_{ld,st}*_mmu functions.
That sounds more what I want for a direct action, No?

Regards

BitFriends

Richard Henderson  schrieb am So., 2. Okt.
2022, 16:40:

> On 10/2/22 02:20, BitFriends wrote:
> > I now came up with this code:
> >
> > TCGv_i64 res = 0;
> > TCGv_i64 addr = (TCGv_i64)(env->regs[R_EDI]);
> >
> > tcg_gen_qemu_ld_i64(res, addr, 0, MO_LEUQ);
> >
> > env->regs[R_EAX] = (target_ulong)res;
> >
> > However this crashes afterwards in test_bit. Maybe this is caused by an
> invalid access?
> > Anything wrong about the code? This still gives some warnings, like
> TCGv_i32 expected (and
> > when you use TCGv_i32, it says TCGv_i64 expected) plus some casting
> warnings.
>
> It is as if you did not read the second paragraph of my response at all.
> tcg_gen_qemu_ld_i64 is for generating code, not performing a direct action.
> Can you see how your code differs from *all* of the code around it?
>
> r~
>
> >
> > Am Sa., 1. Okt. 2022 um 22:23 Uhr schrieb Richard Henderson <
> richard.hender...@linaro.org
> > >:
> >
> > On 10/1/22 13:10, BitFriends wrote:
> >  > Hello,
> >  >
> >  > I am trying to create a custom instruction that accesses guest
> memory specified by an
> >  > address in a register. I specifically want to read from that
> address. So I tried to do
> >  > that using "tcg_gen_qemu_ld_i64(, env->regs[R_EDI], 0,
> MO_LEUQ);", but that
> > doesn't
> >  > save any result in res.
> >
> > This statement should have given you compilation errors, so I don't
> know what you mean by
> > "doesn't save any result".  There's clearly a disconnect between
> what you describe and
> > what you actually attempted.
> >
> > Anyway, by the name you can see that function "gen"erates a "tcg"
> operation, which is
> > then
> > later compiled by the jit, the output of which is later executed to
> produce a result.
> > Which is, in general, what you want for implementing a custom
> instruction.
> >
> >
> > r~
> >
>
>

Re: access guest address from within instruction

[Peter Maydell]

On Sun, 2 Oct 2022 at 10:22, BitFriends  wrote:
> I now came up with this code:
>
> TCGv_i64 res = 0;
> TCGv_i64 addr = (TCGv_i64)(env->regs[R_EDI]);
>
> tcg_gen_qemu_ld_i64(res, addr, 0, MO_LEUQ);
>
> env->regs[R_EAX] = (target_ulong)res;

This is wrong, because you cannot read or write env->regs[] at
translate time. The "translate time" vs "run time" distinction
in a JIT is critical to understand:

 * translate time is when we read guest instructions,
   and output TCG ops. We do this once, the first time
   we encounter a particular piece of guest code. At
   this point you cannot directly access the state of the
   guest CPU. This is because the exact value of EDI
   will be *different* each time the generated code is run.
 * run time is when we are actually emulating the guest
   CPU, by executing the code built from the TCG ops.
   At run time the CPU state is known and can be updated.

You should look at how existing instructions in the x86
translator generate code to read and write registers --
you will see that they use cpu_regs[], which is an array
of TCGv which correspond to the CPU registers (so they can
say "generate code which will read EDI"), and they
never use env->regs[] (which would be "read EDI right now").

More generally, "read guest memory and get the result into
a guest CPU register" is a common thing in existing x86
instructions. So find how we implement one of those
existing insns that's similar to what you want, and see
how that is handled.

(NB: so far you haven't said why your custom instruction
would be any different from a normal load instruction
that x86 already has...)

thanks
-- PMM

Re: access guest address from within instruction

[Richard Henderson]

On 10/2/22 02:20, BitFriends wrote:

I now came up with this code:

TCGv_i64 res = 0;
TCGv_i64 addr = (TCGv_i64)(env->regs[R_EDI]);

tcg_gen_qemu_ld_i64(res, addr, 0, MO_LEUQ);

env->regs[R_EAX] = (target_ulong)res;

However this crashes afterwards in test_bit. Maybe this is caused by an invalid access? 
Anything wrong about the code? This still gives some warnings, like TCGv_i32 expected (and 
when you use TCGv_i32, it says TCGv_i64 expected) plus some casting warnings.


It is as if you did not read the second paragraph of my response at all.
tcg_gen_qemu_ld_i64 is for generating code, not performing a direct action.
Can you see how your code differs from *all* of the code around it?

r~



Am Sa., 1. Okt. 2022 um 22:23 Uhr schrieb Richard Henderson >:


On 10/1/22 13:10, BitFriends wrote:
 > Hello,
 >
 > I am trying to create a custom instruction that accesses guest memory 
specified by an
 > address in a register. I specifically want to read from that address. So 
I tried to do
 > that using "tcg_gen_qemu_ld_i64(, env->regs[R_EDI], 0, MO_LEUQ);", 
but that
doesn't
 > save any result in res.

This statement should have given you compilation errors, so I don't know 
what you mean by
"doesn't save any result".  There's clearly a disconnect between what you 
describe and
what you actually attempted.

Anyway, by the name you can see that function "gen"erates a "tcg" 
operation, which is
then
later compiled by the jit, the output of which is later executed to produce 
a result.
Which is, in general, what you want for implementing a custom instruction.


r~

Re: access guest address from within instruction

[Alex Bennée]

BitFriends  writes:

> Hello,
>
> I am trying to create a custom instruction that accesses guest memory 
> specified by an address in a register. I specifically
> want to read from that address. So I tried to do that using 
> "tcg_gen_qemu_ld_i64(, env->regs[R_EDI], 0,
> MO_LEUQ);", but that doesn't save any result in res. So either my call is 
> wrong, or I need to translate that guest address
> to a usable host address. I can't find much about this stuff in the
> docs. Could anyone help me out with that?

I still think you could solve your problem using semihosting (which
exactly exposes a "fake" instruction to make semihosting calls to save
data on the host system).

>
> Cheers
>
> BitFriends


-- 
Alex Bennée

Re: Commit 'iomap: add support for dma aligned direct-io' causes qemu/KVM boot failures

[Keith Busch]

On Sun, Oct 02, 2022 at 11:59:42AM +0300, Maxim Levitsky wrote:
> On Thu, 2022-09-29 at 19:35 +0200, Paolo Bonzini wrote:
> > On 9/29/22 18:39, Christoph Hellwig wrote:
> > > On Thu, Sep 29, 2022 at 10:37:22AM -0600, Keith Busch wrote:
> > > > > I am aware, and I've submitted the fix to qemu here:
> > > > > 
> > > > >   
> > > > > https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00398.html
> > > > 
> > > > I don't think so. Memory alignment and length granularity are two 
> > > > completely
> > > > different concepts. If anything, the kernel's ABI had been that the 
> > > > length
> > > > requirement was also required for the memory alignment, not the other 
> > > > way
> > > > around. That usage will continue working with this kernel patch.
> 
> Yes, this is how I also understand it - for example for O_DIRECT on a file 
> which
> resides on 4K block device, you have to use page aligned buffers.
> 
> But here after the patch, 512 aligned buffer starts working as well - If I
> understand you correctly the ABI didn't guarantee that such usage would fail,
> but rather that it might fail.

The kernel patch will allow buffer alignment to work with whatever the hardware
reports it can support. It could even as low as byte aligned if that's the
hardware can use that.

The patch aligns direct-io with the same criteria blk_rq_map_user() has always
used to know if the user space buffer is compatible with the hardware's dma
requirements. Prior to this patch, the direct-io memory alignment was an
artificial software constraint, and that constraint creates a lot of
unnecessary memory pressure.

As has always been the case, each segment needs to be a logical block length
granularity. QEMU assumed a buffer's page offset also defined the logical block
size instead of using the actual logical block size that it had previously
discovered directly.

> If I understand that correctly, after the patch in question, 
> qemu is able to use just 512 bytes aligned buffer to read a single 4K block 
> from the disk,
> which supposed to fail but wasn't guarnteed to fail.
> 
> Later qemu it submits iovec which also reads a 4K block but in two parts,
> and if I understand that correctly, each part (iov) is considered
> to be a separate IO operation,  and thus each has to be in my case 4K in 
> size, 
> and its memory buffer *should* also be 4K aligned.
>
> (but it can work with smaller alignement as well).

Right. The iov length needs to match the logical block size. The iov's memory
offset needs to align to the queue's dma_alignment attribute. The memory
alignment may be smaller than a block size.
 
> Assuming that I understand all of this correctly, I agree with Paolo that 
> this is qemu
> bug, but I do fear that it can cause quite some problems for users,
> especially for users that use outdated qemu version.
> 
> It might be too much to ask, but maybe add a Kconfig option to keep legacy 
> behavier
> for those that need it?

Kconfig doesn't sound right.

The block layer exports all the attributes user space needs to know about for
direct io.

  iov length:/sys/block//queue/logical_block_size
  iov mem align: /sys/block//queue/dma_alignment

If you really want to change the behavior, I think maybe we could make the
dma_alignment attribute writeable (or perhaps add a new attribute specifically
for dio_alignment) so the user can request something larger.

Re: [PATCH] net: print a more actionable error when slirp is not found

[Marc-André Lureau]

Hi

On Fri, Sep 30, 2022 at 11:49 PM Christian Schoenebeck
 wrote:
>
> On Donnerstag, 29. September 2022 18:32:37 CEST Marc-André Lureau wrote:
> > From: Marc-André Lureau 
> >
> > If slirp is not found during compile-time, and not manually disabled,
> > print a friendly error message, as suggested in the "If your networking
> > is failing after updating to the latest git version of QEMU..." thread
> > by various people.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  meson.build |  4 
> >  net/net.c   | 19 +--
> >  2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 8dc661363f..4f69d7d0b4 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -657,6 +657,10 @@ if not get_option('slirp').auto() or have_system
> >endif
> >  endif
> >
> > +if get_option('slirp').disabled()
> > +  config_host_data.set('CONFIG_SLIRP_DISABLED', true)
> > +endif
> > +
> >  vde = not_found
> >  if not get_option('vde').auto() or have_system or have_tools
> >vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],
> > diff --git a/net/net.c b/net/net.c
> > index 2db160e063..e6072a5ddd 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -990,14 +990,29 @@ static int net_init_nic(const Netdev *netdev, const
> > char *name, return idx;
> >  }
> >
> > +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
> > +static int net_init_user(const Netdev *netdev, const char *name,
> > + NetClientState *peer, Error **errp)
> > +{
> > +#ifdef CONFIG_SLIRP
> > +return net_init_slirp(netdev, name, peer, errp);
> > +#else
> > +error_setg(errp,
> > +   "Type 'user' is not a supported netdev backend by this QEMU
> > build " +   "because the libslirp development files were not
> > found during build " +   "of QEMU.");
> > +#endif
> > +return -1;
> > +}
> > +#endif
>
> I just tried this, but somehow it is not working for me. net_init_user() is
> never called and therefore I don't get the error message. That should be
> working if the user launched QEMU without any networking arg, right?
>

That's because vl.c has:
if (default_net) {
...
#ifdef CONFIG_SLIRP
qemu_opts_parse(net, "user", true, _abort);
#endif

Iow, it doesn't try to use slirp by default if it's not found at
compile time. We can eventually change that, but that might break
existing users who don't build with slirp.

Alternatively, it could error out only if slirp was not explicitly
disabled at configure time.

> And still, I would find it better if there was also a clear build-time error
> if there was no libslirp and slirp feature was not explicitly disabled.

That's not the typical way we deal with dependencies, but I can try to
do that as well.

>
> >
> >  static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
> >  const Netdev *netdev,
> >  const char *name,
> >  NetClientState *peer, Error **errp) = {
> >  [NET_CLIENT_DRIVER_NIC]   = net_init_nic,
> > -#ifdef CONFIG_SLIRP
> > -[NET_CLIENT_DRIVER_USER]  = net_init_slirp,
> > +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
> > +[NET_CLIENT_DRIVER_USER]  = net_init_user,
> >  #endif
> >  [NET_CLIENT_DRIVER_TAP]   = net_init_tap,
> >  [NET_CLIENT_DRIVER_SOCKET]= net_init_socket,
>
>
>
>

[PATCH v2] m68k: write bootinfo as rom section and re-randomize on reboot

[Jason A. Donenfeld]

Rather than poking directly into RAM, add the bootinfo block as a proper
ROM, so that it's restored when rebooting the system. This way, if the
guest corrupts any of the bootinfo items, but then tries to reboot,
it'll still be restored back to normal as expected. This assumes the
bootinfo block won't exceed 1k.

Then, since the RNG seed needs to be fresh on each boot, regenerate the
RNG seed in the ROM when reseting the CPU.

Cc: Geert Uytterhoeven 
Cc: Laurent Vivier 
Signed-off-by: Jason A. Donenfeld 
---
 hw/m68k/bootinfo.h | 48 +-
 hw/m68k/q800.c | 65 --
 hw/m68k/virt.c | 45 +---
 3 files changed, 99 insertions(+), 59 deletions(-)

diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
index 897162b818..eb92937cf6 100644
--- a/hw/m68k/bootinfo.h
+++ b/hw/m68k/bootinfo.h
@@ -12,66 +12,66 @@
 #ifndef HW_M68K_BOOTINFO_H
 #define HW_M68K_BOOTINFO_H
 
-#define BOOTINFO0(as, base, id) \
+#define BOOTINFO0(base, id) \
 do { \
-stw_phys(as, base, id); \
+stw_p(base, id); \
 base += 2; \
-stw_phys(as, base, sizeof(struct bi_record)); \
+stw_p(base, sizeof(struct bi_record)); \
 base += 2; \
 } while (0)
 
-#define BOOTINFO1(as, base, id, value) \
+#define BOOTINFO1(base, id, value) \
 do { \
-stw_phys(as, base, id); \
+stw_p(base, id); \
 base += 2; \
-stw_phys(as, base, sizeof(struct bi_record) + 4); \
+stw_p(base, sizeof(struct bi_record) + 4); \
 base += 2; \
-stl_phys(as, base, value); \
+stl_p(base, value); \
 base += 4; \
 } while (0)
 
-#define BOOTINFO2(as, base, id, value1, value2) \
+#define BOOTINFO2(base, id, value1, value2) \
 do { \
-stw_phys(as, base, id); \
+stw_p(base, id); \
 base += 2; \
-stw_phys(as, base, sizeof(struct bi_record) + 8); \
+stw_p(base, sizeof(struct bi_record) + 8); \
 base += 2; \
-stl_phys(as, base, value1); \
+stl_p(base, value1); \
 base += 4; \
-stl_phys(as, base, value2); \
+stl_p(base, value2); \
 base += 4; \
 } while (0)
 
-#define BOOTINFOSTR(as, base, id, string) \
+#define BOOTINFOSTR(base, id, string) \
 do { \
 int i; \
-stw_phys(as, base, id); \
+stw_p(base, id); \
 base += 2; \
-stw_phys(as, base, \
+stw_p(base, \
  (sizeof(struct bi_record) + strlen(string) + \
   1 /* null termination */ + 3 /* padding */) & ~3); \
 base += 2; \
 for (i = 0; string[i]; i++) { \
-stb_phys(as, base++, string[i]); \
+stb_p(base++, string[i]); \
 } \
-stb_phys(as, base++, 0); \
-base = (base + 3) & ~3; \
+stb_p(base++, 0); \
+base = (void *)(((unsigned long)base + 3) & ~3); \
 } while (0)
 
-#define BOOTINFODATA(as, base, id, data, len) \
+#define BOOTINFODATA(base, id, data, len) \
 do { \
 int i; \
-stw_phys(as, base, id); \
+stw_p(base, id); \
 base += 2; \
-stw_phys(as, base, \
+stw_p(base, \
  (sizeof(struct bi_record) + len + \
   2 /* length field */ + 3 /* padding */) & ~3); \
 base += 2; \
-stw_phys(as, base, len); \
+stw_p(base, len); \
 base += 2; \
 for (i = 0; i < len; ++i) { \
-stb_phys(as, base++, data[i]); \
+stb_p(base++, data[i]); \
 } \
-base = (base + 3) & ~3; \
+base = (void *)(((unsigned long)base + 3) & ~3); \
 } while (0)
 #endif
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index a4590c2cb0..d94da38cde 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -321,11 +321,22 @@ static const TypeInfo glue_info = {
 },
 };
 
+typedef struct {
+M68kCPU *cpu;
+struct bi_record *rng_seed;
+} ResetInfo;
+
 static void main_cpu_reset(void *opaque)
 {
-M68kCPU *cpu = opaque;
+ResetInfo *reset_info = opaque;
+M68kCPU *cpu = reset_info->cpu;
 CPUState *cs = CPU(cpu);
 
+if (reset_info->rng_seed) {
+qemu_guest_getrandom_nofail((void *)reset_info->rng_seed->data + 2,
+be16_to_cpu(*(uint16_t *)reset_info->rng_seed->data));
+}
+
 cpu_reset(cs);
 cpu->env.aregs[7] = ldl_phys(cs->as, 0);
 cpu->env.pc = ldl_phys(cs->as, 4);
@@ -386,6 +397,7 @@ static void q800_init(MachineState *machine)
 NubusBus *nubus;
 DeviceState *glue;
 DriveInfo *dinfo;
+ResetInfo *reset_info;
 uint8_t rng_seed[32];
 
 linux_boot = (kernel_filename != NULL);
@@ -396,9 +408,12 @@ static void q800_init(MachineState *machine)
 exit(1);
 }
 
+reset_info = g_new0(ResetInfo, 1);
+
 /* init CPUs */
 cpu = M68K_CPU(cpu_create(machine->cpu_type));
-qemu_register_reset(main_cpu_reset, cpu);
+reset_info->cpu = 

Re: [PATCH v5 2/2] block: Refactor get_tmp_filename()

[Bin Meng]

Hi Kevin,

On Fri, Sep 30, 2022 at 6:13 PM Kevin Wolf  wrote:
>
> Am 28.09.2022 um 16:41 hat Bin Meng geschrieben:
> > From: Bin Meng 
> >
> > At present there are two callers of get_tmp_filename() and they are
> > inconsistent.
> >
> > One does:
> >
> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
> > ...
> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> >
> > while the other does:
> >
> > s->qcow_filename = g_malloc(PATH_MAX);
> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> >
> > As we can see different 'size' arguments are passed. There are also
> > platform specific implementations inside the function, and the use
> > of snprintf is really undesirable.
> >
> > The function name is also misleading. It creates a temporary file,
> > not just a filename.
> >
> > Refactor this routine by changing its name and signature to:
> >
> > char *create_tmp_file(Error **errp)
> >
> > and use g_file_open_tmp() for a consistent implementation.
> >
> > Signed-off-by: Bin Meng 
> > ---
> >
> > Changes in v5:
> > - minor change in the commit message
> > - add some notes in the function comment block
> > - add g_autofree for tmp_filename
> >
> > Changes in v4:
> > - Rename the function to create_tmp_file() and take "Error **errp" as
> >   a parameter, so that callers can pass errp all the way down to this
> >   routine.
> > - Commit message updated to reflect the latest change
> >
> > Changes in v3:
> > - Do not use errno directly, instead still let get_tmp_filename() return
> >   a negative number to indicate error
> >
> > Changes in v2:
> > - Use g_autofree and g_steal_pointer
> >
> >  include/block/block_int-common.h |  2 +-
> >  block.c  | 45 
> >  block/vvfat.c|  7 +++--
> >  3 files changed, 20 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/block/block_int-common.h 
> > b/include/block/block_int-common.h
> > index 8947abab76..d7c0a7e96f 100644
> > --- a/include/block/block_int-common.h
> > +++ b/include/block/block_int-common.h
> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild 
> > *child)
> >  }
> >
> >  int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> > -int get_tmp_filename(char *filename, int size);
> > +char *create_tmp_file(Error **errp);
> >  void bdrv_parse_filename_strip_prefix(const char *filename, const char 
> > *prefix,
> >QDict *options);
> >
> > diff --git a/block.c b/block.c
> > index 582c205307..bd3006d85d 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -860,35 +860,25 @@ int bdrv_probe_geometry(BlockDriverState *bs, 
> > HDGeometry *geo)
> >
> >  /*
> >   * Create a uniquely-named empty temporary file.
> > - * Return 0 upon success, otherwise a negative errno value.
> > + * Return the actual file name used upon success, otherwise NULL.
> > + * This string should be freed with g_free() when not needed any longer.
> > + *
> > + * Note: creating a temporary file for the caller to (re)open is
> > + * inherently racy. Use g_file_open_tmp() instead whenever practical.
> >   */
> > -int get_tmp_filename(char *filename, int size)
> > +char *create_tmp_file(Error **errp)
> >  {
> > -#ifdef _WIN32
> > -char temp_dir[MAX_PATH];
> > -/* GetTempFileName requires that its output buffer (4th param)
> > -   have length MAX_PATH or greater.  */
> > -assert(size >= MAX_PATH);
> > -return (GetTempPath(MAX_PATH, temp_dir)
> > -&& GetTempFileName(temp_dir, "qem", 0, filename)
>
> We're using different prefixes on Windows and on Linux. This patch
> unifies both paths to use the Linux name. Nobody should rely on the name
> of temporary files, so there is hope it won't break anything.
>
> > -? 0 : -GetLastError());
> > -#else
> > +g_autofree char *name = NULL;
> > +g_autoptr(GError) err = NULL;
> >  int fd;
> > -const char *tmpdir;
> > -tmpdir = getenv("TMPDIR");
> > -if (!tmpdir) {
> > -tmpdir = "/var/tmp";
> > -}
> > -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> > -return -EOVERFLOW;
> > -}
> > -fd = mkstemp(filename);
> > +
> > +fd = g_file_open_tmp("vl.XX", , );
>
> This implicitly reverts commit 69bef7931e8, g_file_open_tmp() uses /tmp
> as the default instead of /var/tmp as this function does before the
> patch.

Oops, thanks for the pointer. Commit message of 69bef7931e8 does not
explicitely explain why to change from /tmp to /var/tmp. Is that
because QEMU block codes write a huge size of data to this file in
/tmp?

> This is probably not a good idea, we should keep the /var/tmp default.
>
> But if we did want to do this, it's definitely a change in behaviour
> that should be mentioned in the commit message at least.
>

If we have to keep /var/tmp, how about this?

diff --git a/block.c 

Re: access guest address from within instruction

[BitFriends]

I now came up with this code:

TCGv_i64 res = 0;
TCGv_i64 addr = (TCGv_i64)(env->regs[R_EDI]);

tcg_gen_qemu_ld_i64(res, addr, 0, MO_LEUQ);

env->regs[R_EAX] = (target_ulong)res;

However this crashes afterwards in test_bit. Maybe this is caused by an
invalid access? Anything wrong about the code? This still gives some
warnings, like TCGv_i32 expected (and when you use TCGv_i32, it says
TCGv_i64 expected) plus some casting warnings.

Am Sa., 1. Okt. 2022 um 22:23 Uhr schrieb Richard Henderson <
richard.hender...@linaro.org>:

> On 10/1/22 13:10, BitFriends wrote:
> > Hello,
> >
> > I am trying to create a custom instruction that accesses guest memory
> specified by an
> > address in a register. I specifically want to read from that address. So
> I tried to do
> > that using "tcg_gen_qemu_ld_i64(, env->regs[R_EDI], 0, MO_LEUQ);",
> but that doesn't
> > save any result in res.
>
> This statement should have given you compilation errors, so I don't know
> what you mean by
> "doesn't save any result".  There's clearly a disconnect between what you
> describe and
> what you actually attempted.
>
> Anyway, by the name you can see that function "gen"erates a "tcg"
> operation, which is then
> later compiled by the jit, the output of which is later executed to
> produce a result.
> Which is, in general, what you want for implementing a custom instruction.
>
>
> r~
>


bt
Description: Binary data

Re: Commit 'iomap: add support for dma aligned direct-io' causes qemu/KVM boot failures

[Maxim Levitsky]

On Thu, 2022-09-29 at 19:35 +0200, Paolo Bonzini wrote:
> On 9/29/22 18:39, Christoph Hellwig wrote:
> > On Thu, Sep 29, 2022 at 10:37:22AM -0600, Keith Busch wrote:
> > > > I am aware, and I've submitted the fix to qemu here:
> > > > 
> > > >   https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00398.html
> > > 
> > > I don't think so. Memory alignment and length granularity are two 
> > > completely
> > > different concepts. If anything, the kernel's ABI had been that the length
> > > requirement was also required for the memory alignment, not the other way
> > > around. That usage will continue working with this kernel patch.

Yes, this is how I also understand it - for example for O_DIRECT on a file which
resides on 4K block device, you have to use page aligned buffers.

But here after the patch, 512 aligned buffer starts working as well - If I
understand you correctly the ABI didn't guarantee that such usage would fail,
but rather that it might fail.

> > 
> > Well, Linus does treat anything that breaks significant userspace
> > as a regression.  Qemu certainly is significant, but that might depend
> > on bit how common configurations hitting this issue are.
> 
> Seeing the QEMU patch, I agree that it's a QEMU bug though.  I'm 
> surprised it has ever worked.
> 
> It requires 4K sectors in the host but not in the guest, and can be 
> worked around (if not migrating) by disabling O_DIRECT.  I think it's 
> not that awful, but we probably should do some extra releases of QEMU 
> stable branches.
> 
> Paolo
> 

I must admit I am out of the loop on the exact requirements of the O_DIRECT.


If I understand that correctly, after the patch in question, 
qemu is able to use just 512 bytes aligned buffer to read a single 4K block 
from the disk,
which supposed to fail but wasn't guarnteed to fail.



Later qemu it submits iovec which also reads a 4K block but in two parts,
and if I understand that correctly, each part (iov) is considered
to be a separate IO operation,  and thus each has to be in my case 4K in size, 
and its memory buffer *should* also be 4K aligned.

(but it can work with smaller alignement as well).


Assuming that I understand all of this correctly, I agree with Paolo that this 
is qemu
bug, but I do fear that it can cause quite some problems for users,
especially for users that use outdated qemu version.

It might be too much to ask, but maybe add a Kconfig option to keep legacy 
behavier
for those that need it?

Best regards,
Maxim Levitsky