date:20230321


On 3/21/23 09:06, Alessandro Di Federico wrote:

On Mon, 20 Mar 2023 10:10:31 +
Alex Bennée  wrote:


+#define CPU_INTERRUPT_HARD0x0002


Out of curiosity, do we have a policy when to use `const` globals as
opposed to `#define`?
In theory, if a constant is never used in any preprocessor directive,
we could turn it into a global `const`.


No, this is not c++.


r~

[PATCH for 8.0 v2] igb: Save more Tx states

2023-03-21 Thread Akihiko Odaki

The current implementation of igb uses only part of a advanced Tx
context descriptor and first data descriptor because it misses some
features and sniffs the trait of the packet instead of respecting the
packet type specified in the descriptor. However, we will certainly
need the entire Tx context descriptor when we update igb to respect
these ignored fields. Save the entire context descriptor and first
data descriptor except the buffer address to prepare for such a change.

This also introduces the distinction of contexts with different
indexes, which was not present in e1000e but in igb.

Signed-off-by: Akihiko Odaki 
Reviewed-by: Sriram Yagnaraman 
---
V1 -> V2: Removed references to old variables in igb_reset()

Supersedes: <20230316155707.27007-1-akihiko.od...@daynix.com>

 hw/net/igb.c  | 25 ++---
 hw/net/igb_core.c | 39 +++
 hw/net/igb_core.h |  8 +++-
 3 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/hw/net/igb.c b/hw/net/igb.c
index c6d753df87..7c05896325 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -502,16 +502,27 @@ static int igb_post_load(void *opaque, int version_id)
 return igb_core_post_load(>core);
 }
 
-static const VMStateDescription igb_vmstate_tx = {
-.name = "igb-tx",
+static const VMStateDescription igb_vmstate_tx_ctx = {
+.name = "igb-tx-ctx",
 .version_id = 1,
 .minimum_version_id = 1,
 .fields = (VMStateField[]) {
-VMSTATE_UINT16(vlan, struct igb_tx),
-VMSTATE_UINT16(mss, struct igb_tx),
-VMSTATE_BOOL(tse, struct igb_tx),
-VMSTATE_BOOL(ixsm, struct igb_tx),
-VMSTATE_BOOL(txsm, struct igb_tx),
+VMSTATE_UINT32(vlan_macip_lens, struct e1000_adv_tx_context_desc),
+VMSTATE_UINT32(seqnum_seed, struct e1000_adv_tx_context_desc),
+VMSTATE_UINT32(type_tucmd_mlhl, struct e1000_adv_tx_context_desc),
+VMSTATE_UINT32(mss_l4len_idx, struct e1000_adv_tx_context_desc),
+}
+};
+
+static const VMStateDescription igb_vmstate_tx = {
+.name = "igb-tx",
+.version_id = 2,
+.minimum_version_id = 2,
+.fields = (VMStateField[]) {
+VMSTATE_STRUCT_ARRAY(ctx, struct igb_tx, 2, 0, igb_vmstate_tx_ctx,
+ struct e1000_adv_tx_context_desc),
+VMSTATE_UINT32(first_cmd_type_len, struct igb_tx),
+VMSTATE_UINT32(first_olinfo_status, struct igb_tx),
 VMSTATE_BOOL(first, struct igb_tx),
 VMSTATE_BOOL(skip_cp, struct igb_tx),
 VMSTATE_END_OF_LIST()
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index a7c7bfdc75..7708333c2a 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -389,8 +389,10 @@ igb_rss_parse_packet(IGBCore *core, struct NetRxPkt *pkt, 
bool tx,
 static bool
 igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)
 {
-if (tx->tse) {
-if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
+if (tx->first_cmd_type_len & E1000_ADVTXD_DCMD_TSE) {
+uint32_t idx = (tx->first_olinfo_status >> 4) & 1;
+uint32_t mss = tx->ctx[idx].mss_l4len_idx >> 16;
+if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
 return false;
 }
 
@@ -399,13 +401,13 @@ igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)
 return true;
 }
 
-if (tx->txsm) {
+if (tx->first_olinfo_status & E1000_ADVTXD_POTS_TXSM) {
 if (!net_tx_pkt_build_vheader(tx->tx_pkt, false, true, 0)) {
 return false;
 }
 }
 
-if (tx->ixsm) {
+if (tx->first_olinfo_status & E1000_ADVTXD_POTS_IXSM) {
 net_tx_pkt_update_ip_hdr_checksum(tx->tx_pkt);
 }
 
@@ -527,7 +529,7 @@ igb_process_tx_desc(IGBCore *core,
 {
 struct e1000_adv_tx_context_desc *tx_ctx_desc;
 uint32_t cmd_type_len;
-uint32_t olinfo_status;
+uint32_t idx;
 uint64_t buffer_addr;
 uint16_t length;
 
@@ -538,20 +540,19 @@ igb_process_tx_desc(IGBCore *core,
 E1000_ADVTXD_DTYP_DATA) {
 /* advanced transmit data descriptor */
 if (tx->first) {
-olinfo_status = le32_to_cpu(tx_desc->read.olinfo_status);
-
-tx->tse = !!(cmd_type_len & E1000_ADVTXD_DCMD_TSE);
-tx->ixsm = !!(olinfo_status & E1000_ADVTXD_POTS_IXSM);
-tx->txsm = !!(olinfo_status & E1000_ADVTXD_POTS_TXSM);
-
+tx->first_cmd_type_len = cmd_type_len;
+tx->first_olinfo_status = 
le32_to_cpu(tx_desc->read.olinfo_status);
 tx->first = false;
 }
 } else if ((cmd_type_len & E1000_ADVTXD_DTYP_CTXT) ==
E1000_ADVTXD_DTYP_CTXT) {
 /* advanced transmit context descriptor */
 tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
-tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
-tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
+idx =

Re: [PATCH for-8.0 v2 3/3] target/ppc: Fix helper_pminsn() prototype


On 3/21/23 09:16, Cédric Le Goater wrote:

From: Cédric Le Goater

GCC13 reports an error:

../target/ppc/excp_helper.c:2625:6: error: conflicting types for 
‘helper_pminsn’ due to enum/integer mismatch; have ‘void(CPUPPCState *, 
powerpc_pm_insn_t)’ {aka ‘void(struct CPUArchState *, powerpc_pm_insn_t)’} 
[-Werror=enum-int-mismatch]
  2625 | void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
   |  ^
In file included from /home/legoater/work/qemu/qemu.git/include/qemu/osdep.h:49,
  from ../target/ppc/excp_helper.c:19:
/home/legoater/work/qemu/qemu.git/include/exec/helper-head.h:23:27: note: 
previous declaration of ‘helper_pminsn’ with type ‘void(CPUArchState *, 
uint32_t)’ {aka ‘void(CPUArchState *, unsigned int)’}
23 | #define HELPER(name) glue(helper_, name)
   |   ^~~

Cc: Daniel Henrique Barboza
Fixes: 7778a575c7 ("ppc: Add P7/P8 Power Management instructions")
Signed-off-by: Cédric Le Goater
Reviewed-by: Daniel Henrique Barboza
---
  target/ppc/excp_helper.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH for-8.0 v2 2/3] target/s390x: Fix float_comp_to_cc() prototype


On 3/21/23 09:16, Cédric Le Goater wrote:

From: Cédric Le Goater 

GCC13 reports an error :

../target/s390x/tcg/fpu_helper.c:123:5: error: conflicting types for 
‘float_comp_to_cc’ due to enum/integer mismatch; have ‘int(CPUS390XState *, 
FloatRelation)’ {aka ‘int(struct CPUArchState *, FloatRelation)’} 
[-Werror=enum-int-mismatch]

   123 | int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare)
   | ^~~~
In file included from ../target/s390x/tcg/fpu_helper.c:23:
../target/s390x/s390x-internal.h:302:5: note: previous declaration of 
‘float_comp_to_cc’ with type ‘int(CPUS390XState *, int)’ {aka ‘int(struct 
CPUArchState *, int)’}
   302 | int float_comp_to_cc(CPUS390XState *env, int float_compare);
   | ^~~~

Cc: Thomas Huth 
Cc: Richard Henderson 
Cc: David Hildenbrand 
Cc: Ilya Leoshkevich 
Fixes: 71bfd65c5f ("softfloat: Name compare relation enum")
Signed-off-by: Cédric Le Goater 
Reviewed-by: Philippe Mathieu-Daudé 


Reviewed-by: Richard Henderson 

r~

Re: [PATCH for-8.0 3/3] target/ppc: Fix helper_pminsn() prototype


On 3/21/23 01:33, Cédric Le Goater wrote:

From: Cédric Le Goater

GCC13 reports an error:

../target/ppc/excp_helper.c:2625:6: error: conflicting types for 
‘helper_pminsn’ due to enum/integer mismatch; have ‘void(CPUPPCState *, 
powerpc_pm_insn_t)’ {aka ‘void(struct CPUArchState *, powerpc_pm_insn_t)’} 
[-Werror=enum-int-mismatch]
  2625 | void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
   |  ^
In file included from /home/legoater/work/qemu/qemu.git/include/qemu/osdep.h:49,
  from ../target/ppc/excp_helper.c:19:
/home/legoater/work/qemu/qemu.git/include/exec/helper-head.h:23:27: note: 
previous declaration of ‘helper_pminsn’ with type ‘void(CPUArchState *, 
uint32_t)’ {aka ‘void(CPUArchState *, unsigned int)’}
23 | #define HELPER(name) glue(helper_, name)
   |   ^~~

Cc: Daniel Henrique Barboza
Fixes: 7778a575c7 ("ppc: Add P7/P8 Power Management instructions")
Signed-off-by: Cédric Le Goater
---
  target/ppc/excp_helper.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH for-8.0 2/3] target/s390x: Fix float_comp_to_cc() prototype


On 3/21/23 01:33, Cédric Le Goater wrote:

From: Cédric Le Goater

GCC13 reports an error :

../target/s390x/tcg/fpu_helper.c:123:5: error: conflicting types for 
‘float_comp_to_cc’ due to enum/integer mismatch; have ‘int(CPUS390XState *, 
FloatRelation)’ {aka ‘int(struct CPUArchState *, FloatRelation)’} 
[-Werror=enum-int-mismatch]

   123 | int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare)
   | ^~~~
In file included from ../target/s390x/tcg/fpu_helper.c:23:
../target/s390x/s390x-internal.h:302:5: note: previous declaration of 
‘float_comp_to_cc’ with type ‘int(CPUS390XState *, int)’ {aka ‘int(struct 
CPUArchState *, int)’}
   302 | int float_comp_to_cc(CPUS390XState *env, int float_compare);
   | ^~~~

Cc: Thomas Huth
Cc: Richard Henderson
Cc: David Hildenbrand
Cc: Ilya Leoshkevich
Fixes: 71bfd65c5f ("softfloat: Name compare relation enum")
Signed-off-by: Cédric Le Goater
---
  target/s390x/s390x-internal.h | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

2023-03-21 Thread Wu, Fei

On 3/22/2023 11:31 AM, Richard Henderson wrote:
> On 3/21/23 19:47, Wu, Fei wrote:
 You should be making use of different softmmu indexes, similar to how
 ARM uses a separate index for PAN (privileged access never) mode.  If
 I read the manual properly, PAN == !SUM.

 When you do this, you need no additional flushing.
>>>
>>> Hi Fei,
>>>
>>> Let's follow Richard's advice.
>>> Yes, I'm thinking about how to do it, and thank Richard for the advice.
>>
>> My question is:
>> * If we ensure this separate index (S+SUM) has no overlapping tlb
>> entries with S-mode (ignore M-mode so far), during SUM=1, we have to
>> look into both (S+SUM) and S index for kernel address translation, that
>> should be not desired.
> 
> This is an incorrect assumption.  S+SUM may very well have overlapping
> tlb entries with S.
> With SUM=1, you *only* look in S+SUM index; with SUM=0, you *only* look
> in S index.
> 
> The only difference is a check in get_physical_address is no longer
> against MSTATUS_SUM directly, but against the mmu_index.
> 
>> * If all the tlb operations are against (S+SUM) during SUM=1, then
>> (S+SUM) could contain some duplicated tlb entries of kernel address in S
>> index, the duplication means extra tlb lookup and fill.
> 
> Yes, if the same address is probed via S and S+SUM, there is a
> duplicated lookup.  But this is harmless.
> 
> 
>> Also if we want
>> to flush tlb entry of specific addr0, we have to flush both index.
> 
> Yes, this is also true.  But so far target/riscv is making no use of
> per-mmuidx flushing. At the moment you're *only* using tlb_flush(cpu),
> which flushes every mmuidx.  Nor are you making use of per-page flushing.
> 
> So, really, no change required at all there.
> 
Got it, let me try this method.

Thanks,
Fei.

> 
> r~

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

On 3/21/23 19:47, Wu, Fei wrote:

You should be making use of different softmmu indexes, similar to how
ARM uses a separate index for PAN (privileged access never) mode. If
I read the manual properly, PAN == !SUM.

When you do this, you need no additional flushing.

Hi Fei,

Let's follow Richard's advice.
Yes, I'm thinking about how to do it, and thank Richard for the advice.

My question is:
* If we ensure this separate index (S+SUM) has no overlapping tlb
entries with S-mode (ignore M-mode so far), during SUM=1, we have to
look into both (S+SUM) and S index for kernel address translation, that
should be not desired.

This is an incorrect assumption. S+SUM may very well have overlapping tlb
entries with S.
With SUM=1, you *only* look in S+SUM index; with SUM=0, you *only* look in S
index.

The only difference is a check in get_physical_address is no longer against MSTATUS_SUM
directly, but against the mmu_index.

* If all the tlb operations are against (S+SUM) during SUM=1, then
(S+SUM) could contain some duplicated tlb entries of kernel address in S
index, the duplication means extra tlb lookup and fill.

Yes, if the same address is probed via S and S+SUM, there is a duplicated lookup. But
this is harmless.

Also if we want
to flush tlb entry of specific addr0, we have to flush both index.

Yes, this is also true. But so far target/riscv is making no use of per-mmuidx flushing.
At the moment you're *only* using tlb_flush(cpu), which flushes every mmuidx. Nor are you
making use of per-page flushing.

So, really, no change required at all there.

Re: [PATCH v2 0/3] Enable -cpu ,help

2023-03-21 Thread Dinah B

Friendly ping for code review on this patch series.

Full series:
https://lore.kernel.org/qemu-devel/20230314100026.536079-1-dinahbaum...@gmail.com/

Thanks,
-DInah

On Tue, Mar 14, 2023 at 6:00 AM Dinah Baum  wrote:

> Part 1 is a refactor/code motion patch for
> qapi/machine target required for setup of
>
> Part 2 which enables query-cpu-model-expansion
> on all architectures
>
> Part 3 implements the ',help' feature
>
> Limitations:
> Currently only 'FULL' expansion queries are implemented since
> that's the only type enabled on the architectures that
> allow feature probing
>
> Unlike the 'device,help' command, default values aren't
> printed
>
> Dinah Baum (3):
>   qapi/machine-target: refactor machine-target
>   cpu, qapi, target/arm, i386, s390x: Generalize
> query-cpu-model-expansion
>   cpu, qdict, vl: Enable printing options for CPU type
>
>  MAINTAINERS  |   1 +
>  cpu.c|  61 +++
>  include/exec/cpu-common.h|  10 +++
>  include/qapi/qmp/qdict.h |   2 +
>  qapi/machine-target-common.json  | 130 +++
>  qapi/machine-target.json | 129 +-
>  qapi/meson.build |   1 +
>  qemu-options.hx  |   7 +-
>  qobject/qdict.c  |   5 ++
>  softmmu/vl.c |  36 -
>  target/arm/arm-qmp-cmds.c|   7 +-
>  target/arm/cpu.h |   7 +-
>  target/i386/cpu-sysemu.c |   7 +-
>  target/i386/cpu.h|   6 ++
>  target/s390x/cpu.h   |   7 ++
>  target/s390x/cpu_models_sysemu.c |   6 +-
>  16 files changed, 278 insertions(+), 144 deletions(-)
>  create mode 100644 qapi/machine-target-common.json
>
> --
> 2.30.2
>
>

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

2023-03-21 Thread LIU Zhiwei




On 2023/3/22 10:47, Wu, Fei wrote:

On 3/22/2023 9:58 AM, LIU Zhiwei wrote:

On 2023/3/22 0:10, Richard Henderson wrote:

On 3/20/23 23:37, fei2...@intel.com wrote:

From: Fei Wu 

Kernel needs to access user mode memory e.g. during syscalls, the window
is usually opened up for a very limited time through MSTATUS.SUM, the
overhead is too much if tlb_flush() gets called for every SUM change.
This patch saves addresses accessed when SUM=1, and flushs only these
pages when SUM changes to 0. If the buffer is not large enough to save
all the pages during SUM=1, it will fall back to tlb_flush when
necessary.

The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
most of the time kernel accesses 1 or 2 pages, it's very rare to see
more than 4 pages accessed.

It's not necessary to save/restore these new added status, as
tlb_flush() is always called after restore.

Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
other syscalls benefit a lot from this one too.

This is not the correct approach.

You should be making use of different softmmu indexes, similar to how
ARM uses a separate index for PAN (privileged access never) mode.  If
I read the manual properly, PAN == !SUM.

When you do this, you need no additional flushing.

Hi Fei,

Let's follow Richard's advice.
Yes, I'm thinking about how to do it, and thank Richard for the advice.

My question is:
* If we ensure this separate index (S+SUM) has no overlapping tlb
entries with S-mode (ignore M-mode so far), during SUM=1,

Yes, every mmu index will have their own cache.

we have to
look into both (S+SUM) and S index for kernel address translation, that
should be not desired.
No, we  have to choose one, because each access will be constrained with 
a mmu idex.


* If all the tlb operations are against (S+SUM) during SUM=1, then
(S+SUM) could contain some duplicated tlb entries of kernel address in S
index, the duplication means extra tlb lookup and fill. Also if we want
to flush tlb entry of specific addr0, we have to flush both index.


This is not the case.

Zhiwei



I will take a look at how arm handles this.

Thanks,
Fei.


Zhiwei



r~

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

2023-03-21 Thread Wu, Fei

On 3/22/2023 9:58 AM, LIU Zhiwei wrote:
> 
> On 2023/3/22 0:10, Richard Henderson wrote:
>> On 3/20/23 23:37, fei2...@intel.com wrote:
>>> From: Fei Wu 
>>>
>>> Kernel needs to access user mode memory e.g. during syscalls, the window
>>> is usually opened up for a very limited time through MSTATUS.SUM, the
>>> overhead is too much if tlb_flush() gets called for every SUM change.
>>> This patch saves addresses accessed when SUM=1, and flushs only these
>>> pages when SUM changes to 0. If the buffer is not large enough to save
>>> all the pages during SUM=1, it will fall back to tlb_flush when
>>> necessary.
>>>
>>> The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
>>> most of the time kernel accesses 1 or 2 pages, it's very rare to see
>>> more than 4 pages accessed.
>>>
>>> It's not necessary to save/restore these new added status, as
>>> tlb_flush() is always called after restore.
>>>
>>> Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
>>> other syscalls benefit a lot from this one too.
>>
>> This is not the correct approach.
>>
>> You should be making use of different softmmu indexes, similar to how
>> ARM uses a separate index for PAN (privileged access never) mode.  If
>> I read the manual properly, PAN == !SUM.
>>
>> When you do this, you need no additional flushing.
> 
> Hi Fei,
> 
> Let's follow Richard's advice.
>Yes, I'm thinking about how to do it, and thank Richard for the advice.

My question is:
* If we ensure this separate index (S+SUM) has no overlapping tlb
entries with S-mode (ignore M-mode so far), during SUM=1, we have to
look into both (S+SUM) and S index for kernel address translation, that
should be not desired.

* If all the tlb operations are against (S+SUM) during SUM=1, then
(S+SUM) could contain some duplicated tlb entries of kernel address in S
index, the duplication means extra tlb lookup and fill. Also if we want
to flush tlb entry of specific addr0, we have to flush both index.

I will take a look at how arm handles this.

Thanks,
Fei.

> Zhiwei
> 
>>
>>
>> r~

Re: [PATCH 4/4] hw/pci: Ensure pci_add_capability() is called before device is realized

2023-03-21 Thread Michael S. Tsirkin

On Tue, Mar 14, 2023 at 12:14:35PM +0100, Philippe Mathieu-Daudé wrote:
> PCI capabilities can't appear magically at runtime.
> Guests aren't expecting that. Assert all capabilities
> are added _before_ a device instance is realized.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/pci/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index ac41fcbf6a..ed60b352e4 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2397,7 +2397,7 @@ static void pci_del_option_rom(PCIDevice *pdev)
>   * On success, pci_add_capability() returns a positive value
>   * that the offset of the pci capability.
>   * On failure, it sets an error and returns a negative error
> - * code.
> + * code. @pdev must be unrealized.
>   */
>  int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> uint8_t offset, uint8_t size,
> @@ -2406,6 +2406,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>  uint8_t *config;
>  int i, overlapping_cap;
>  
> +assert(!DEVICE(pdev)->realized);
> +
>  if (!offset) {
>  offset = pci_find_space(pdev, size);
>  /* out of PCI config space is programming error */

Fails in CI:

https://gitlab.com/mstredhat/qemu/-/jobs/3976974199

qemu-system-i386: ../hw/pci/pci.c:2409: pci_add_capability: Assertion 
`!DEVICE(pdev)->realized' failed.
Broken pipe
../tests/qtest/libqtest.c:193: kill_qemu() detected QEMU death from signal 6 
(Aborted) (core dumped)
TAP parsing error: Too few tests run (expected 49, got 40)
(test program exited with status code -6)



> -- 
> 2.38.1

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

2023-03-21 Thread LIU Zhiwei




On 2023/3/22 0:10, Richard Henderson wrote:

On 3/20/23 23:37, fei2...@intel.com wrote:

From: Fei Wu 

Kernel needs to access user mode memory e.g. during syscalls, the window
is usually opened up for a very limited time through MSTATUS.SUM, the
overhead is too much if tlb_flush() gets called for every SUM change.
This patch saves addresses accessed when SUM=1, and flushs only these
pages when SUM changes to 0. If the buffer is not large enough to save
all the pages during SUM=1, it will fall back to tlb_flush when
necessary.

The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
most of the time kernel accesses 1 or 2 pages, it's very rare to see
more than 4 pages accessed.

It's not necessary to save/restore these new added status, as
tlb_flush() is always called after restore.

Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
other syscalls benefit a lot from this one too.


This is not the correct approach.

You should be making use of different softmmu indexes, similar to how 
ARM uses a separate index for PAN (privileged access never) mode.  If 
I read the manual properly, PAN == !SUM.


When you do this, you need no additional flushing.


Hi Fei,

Let's follow Richard's advice.

Zhiwei




r~

Re: [PATCH RESEND] hw/i2c: Enable an id for the pca954x devices

2023-03-21 Thread Corey Minyard

On Tue, Mar 21, 2023 at 11:27:44AM -0700, Patrick Venture wrote:
> This allows the devices to be more readily found and specified.
> Without setting the id field, they can only be found by device type
> name, which doesn't let you specify the second of the same device type
> behind a bus.

So basically, this helps you find a specific device if you have more
than one of them.  Yeah, probably a good idea in this case.

> 
> Tested: Verified that by default the device was findable with the id
> 'pca954x[77]', for an instance attached at that address.
> 
> Signed-off-by: Patrick Venture 
> Reviewed-by: Hao Wu 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/i2c/i2c_mux_pca954x.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> index a9517b612a..4f8c2d6ae1 100644
> --- a/hw/i2c/i2c_mux_pca954x.c
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -20,6 +20,7 @@
>  #include "hw/i2c/i2c_mux_pca954x.h"
>  #include "hw/i2c/smbus_slave.h"
>  #include "hw/qdev-core.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/sysbus.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> @@ -43,6 +44,8 @@ typedef struct Pca954xState {
>  
>  bool enabled[PCA9548_CHANNEL_COUNT];
>  I2CBus *bus[PCA9548_CHANNEL_COUNT];
> +
> +char *id;
>  } Pca954xState;
>  
>  /*
> @@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass, void 
> *data)
>  s->nchans = PCA9548_CHANNEL_COUNT;
>  }
>  
> +static void pca954x_realize(DeviceState *dev, Error **errp)
> +{
> +Pca954xState *s = PCA954X(dev);
> +DeviceState *d = DEVICE(s);
> +if (s->id) {
> +d->id = g_strdup(s->id);
> +} else {
> +d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
> +}
> +}
> +
>  static void pca954x_init(Object *obj)
>  {
>  Pca954xState *s = PCA954X(obj);
> @@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
>  }
>  }
>  
> +static Property pca954x_props[] = {
> +DEFINE_PROP_STRING("id", Pca954xState, id),
> +DEFINE_PROP_END_OF_LIST()
> +};

There is already an "id=" thing in qemu for doing links between front
ends and back ends.  That's probably not the best name to use.  Several
devices, like network devices, use "name" for identification in the
monitor.

-corey

> +
>  static void pca954x_class_init(ObjectClass *klass, void *data)
>  {
>  I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> @@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass, void 
> *data)
>  rc->phases.enter = pca954x_enter_reset;
>  
>  dc->desc = "Pca954x i2c-mux";
> +dc->realize = pca954x_realize;
>  
>  k->write_data = pca954x_write_data;
>  k->receive_byte = pca954x_read_byte;
> +
> +device_class_set_props(dc, pca954x_props);
>  }
>  
>  static const TypeInfo pca954x_info[] = {
> -- 
> 2.35.1.894.gb6a874cedc-goog
> 


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 3/3] Add support for TPM devices over I2C bus





On 3/21/23 01:30, Ninad Palsule wrote:

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices. I2C model only supports
TPM2 protocol.




+
+/* Send data to TPM */
+static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
+{
+if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
+uint16_t tis_reg;
+uint32_t data;
+int  i;
+
+tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], >size);
+
+/* Index 0 is always a register */
+for (i = 1; i < i2cst->offset; i++) {
+data = (i2cst->data[i] & 0xff);
+tpm_tis_write_data(>state, tis_reg, data, 1);
+}



I think there should be tpm_tis_set_data_buffer function that you can call 
rather than transferring the data byte-by-byte.

Thanks for the series!

  Stefan

Re: [PATCH 3/3] Add support for TPM devices over I2C bus





On 3/21/23 01:30, Ninad Palsule wrote:

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices. I2C model only supports
TPM2 protocol.

This commit includes changes for the common code.
- Added I2C emulation model. Logic was added in the model to temporarily
   cache the data as I2C interface works per byte basis.
- New tpm type "tpm-tis-i2c" added for I2C support. User specify this
   string on command line.

Testing:
   TPM I2C device modulte is tested using SWTPM (software based TPM
   package). The qemu used the rainier machine and it was connected to
   swtpm over the socket interface.

   The command to start swtpm is as follows:
   $ swtpm socket --tpmstate dir=/tmp/mytpm1\
  --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock  \
  --tpm2 --log level=100

   The command to start qemu is as follows:
   $ qemu-system-arm -M rainier-bmc -nographic \
 -kernel ${IMAGEPATH}/fitImage-linux.bin \
 -dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
 -initrd ${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
 -drive 
file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
 -net nic -net 
user,hostfwd=:127.0.0.1:-:22,hostfwd=:127.0.0.1:2443-:443 \
 -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
 -tpmdev emulator,id=tpm0,chardev=chrtpm \
 -device tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e



Please add this command line example also to the documentation.

When you run scripts/checkpatch.pl over this patch it reports the following 
relevant complaints:

WARNING: Block comments use a leading /* on a separate line
#255: FILE: hw/tpm/tpm_tis_i2c.c:190:
+/* If data is for FIFO then it is received from tpm_tis_common buffer

WARNING: Block comments use a leading /* on a separate line
#345: FILE: hw/tpm/tpm_tis_i2c.c:280:
+/* Get the backend pointer. It is not initialized propery during





   Note: Currently you need to specify the I2C bus and device address on
 command line. In future we can add a device at board level.

Signed-off-by: Ninad Palsule 
---
  hw/tpm/meson.build   |   1 +
  hw/tpm/tpm_tis_i2c.c | 342 +++
  include/sysemu/tpm.h |   3 +
  3 files changed, 346 insertions(+)
  create mode 100644 hw/tpm/tpm_tis_i2c.c

diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index 7abc2d794a..76fe3cb098 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -1,6 +1,7 @@
  softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
  softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
  softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: 
files('tpm_tis_sysbus.c'))
+softmmu_ss.add(when: 'CONFIG_TPM_TIS_I2C', if_true: files('tpm_tis_i2c.c'))
  softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
  softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_ppi.c'))
  softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_ppi.c'))
diff --git a/hw/tpm/tpm_tis_i2c.c b/hw/tpm/tpm_tis_i2c.c
new file mode 100644
index 00..3c45af4140
--- /dev/null
+++ b/hw/tpm/tpm_tis_i2c.c
@@ -0,0 +1,342 @@
+/*
+ * tpm_tis_i2c.c - QEMU's TPM TIS I2C Device
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * Implementation of the TIS interface according to specs found at
+ * http://www.trustedcomputinggroup.org. This implementation currently
+ * supports version 1.3, 21 March 2013
+ * In the developers menu choose the PC Client section then find the TIS
+ * specification.
+ *
+ * TPM TIS for TPM 2 implementation following TCG PC Client Platform
+ * TPM Profile (PTP) Specification, Familiy 2.0, Revision 00.43
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "hw/qdev-properties.h"
+#include "hw/acpi/tpm.h"
+#include "migration/vmstate.h"
+#include "tpm_prop.h"
+#include "tpm_tis.h"
+#include "qom/object.h"
+#include "block/aio.h"
+#include "qemu/main-loop.h"
+
+/* TPM TIS I2C registers */
+#define TPM_TIS_I2C_REG_LOC_SEL  0x00
+#define TPM_TIS_I2C_REG_ACCESS   0x04
+#define TPM_TIS_I2C_REG_INT_ENABLE   0x08
+#define TPM_TIS_I2C_REG_INT_CAPABILITY   0x14
+#define TPM_TIS_I2C_REG_STS  0x18
+#define TPM_TIS_I2C_REG_DATA_FIFO0x24
+#define TPM_TIS_I2C_REG_INTF_CAPABILITY  0x30
+#define TPM_TIS_I2C_REG_DATA_CSUM_ENABLE 0x40
+#define TPM_TIS_I2C_REG_DATA_CSUM_GET0x44
+#define TPM_TIS_I2C_REG_DID_VID  0x48
+#define TPM_TIS_I2C_REG_RID  0x4c
+#define TPM_TIS_I2C_REG_UNKNOWN  0xff
+
+/* Operations */
+#define OP_SEND   1
+#define OP_RECV   2
+
+typedef struct TPMStateI2C {
+/*< private >*/
+I2CSlave parent_obj;
+
+int  offset; /* offset in to data[] */
+int  size;   /* Size of the

Re: s390 migration crash

2023-03-21 Thread Peter Xu

On Tue, Mar 21, 2023 at 08:24:37PM +, Dr. David Alan Gilbert wrote:
> Hi Peter's,
>   Peter M pointed me to a seg in a migration test in CI; I can reproduce
> it:
>   * On an s390 host

How easy to reproduce?

>   * only as part of a make check - running migration-test by itself
> doesn't trigger for me.
>   * It looks like it's postcopy preempt
> 
> (gdb) bt full
> #0  iov_size (iov=iov@entry=0x2aa00e60670, iov_cnt=) at 
> ../util/iov.c:88
> len = 13517923312037845750
> i = 17305
> #1  0x02aa004d068c in qemu_fflush (f=0x2aa00e58630) at 
> ../migration/qemu-file.c:307
> local_error = 0x0
> #2  0x02aa004d0e04 in qemu_fflush (f=) at 
> ../migration/qemu-file.c:297
> #3  0x02aa00613962 in postcopy_preempt_shutdown_file 
> (s=s@entry=0x2aa00d1b4e0) at ../migration/ram.c:4657
> #4  0x02aa004e12b4 in migration_completion (s=0x2aa00d1b4e0) at 
> ../migration/migration.c:3469
> ret = 
> current_active_state = 5
> must_precopy = 0
> can_postcopy = 0
> in_postcopy = true
> pending_size = 0
> __func__ = "migration_iteration_run"
> iter_state = 
> s = 0x2aa00d1b4e0
> thread = 
> setup_start = 
> thr_error = 
> urgent = 
> #5  migration_iteration_run (s=0x2aa00d1b4e0) at ../migration/migration.c:3882
> must_precopy = 0
> can_postcopy = 0
> in_postcopy = true
> pending_size = 0
> __func__ = "migration_iteration_run"
> iter_state = 
> s = 0x2aa00d1b4e0
> thread = 
> setup_start = 
> thr_error = 
> urgent = 
> #6  migration_thread (opaque=opaque@entry=0x2aa00d1b4e0) at 
> ../migration/migration.c:4124
> iter_state = 
> s = 0x2aa00d1b4e0
> --Type  for more, q to quit, c to continue without paging--
> thread = 
> setup_start = 
> thr_error = 
> urgent = 
> #7  0x02aa00819b8c in qemu_thread_start (args=) at 
> ../util/qemu-thread-posix.c:541
> __cancel_buf = 
> {__cancel_jmp_buf = {{__cancel_jmp_buf = {{__gregs = 
> {4396782422080, 4393751543808, 4397299389454, 4396844235904, 2929182727824, 
> 2929182933488, 4396843986792, 4397299389455, 33679382915066768, 
> 33678512846981306}, __fpregs = {4396774031360, 8392704, 2929182933488, 0, 
> 4396782422272, 2929172491858, 4396774031360, 1}}}, __mask_was_saved = 0}}, 
> __pad = {0x3ffb4a77a60, 0x0, 0x0, 0x0}}
> __cancel_routine = 0x2aa00819bf0 
> __not_first_call = 
> start_routine = 0x2aa004e08f0 
> arg = 0x2aa00d1b4e0
> r = 
> #8  0x03ffb7b1e2e6 in start_thread () at /lib64/libc.so.6
> #9  0x03ffb7aafdbe in thread_start () at /lib64/libc.so.6
> 
> It looks like it's in the preempt test:
> 
> (gdb) where
> #0  0x03ffb17a0126 in __pthread_kill_implementation () from 
> /lib64/libc.so.6
> #1  0x03ffb1750890 in raise () from /lib64/libc.so.6
> #2  0x03ffb172a340 in abort () from /lib64/libc.so.6
> #3  0x02aa0041c130 in qtest_check_status (s=) at 
> ../tests/qtest/libqtest.c:194
> #4  0x03ffb1a3b5de in g_hook_list_invoke () from /lib64/libglib-2.0.so.0
> #5  
> #6  0x03ffb17a0126 in __pthread_kill_implementation () from 
> /lib64/libc.so.6
> #7  0x03ffb1750890 in raise () from /lib64/libc.so.6
> #8  0x03ffb172a340 in abort () from /lib64/libc.so.6
> #9  0x02aa00420318 in qmp_fd_receive (fd=) at 
> ../tests/qtest/libqmp.c:80
> #10 0x02aa0041d5ee in qtest_qmp_receive_dict (s=0x2aa01eb2700) at 
> ../tests/qtest/libqtest.c:713
> #11 qtest_qmp_receive (s=0x2aa01eb2700) at ../tests/qtest/libqtest.c:701
> #12 qtest_vqmp (s=s@entry=0x2aa01eb2700, fmt=fmt@entry=0x2aa00487100 "{ 
> 'execute': 'query-migrate' }", ap=ap@entry=0x3ffc247cc68)
> at ../tests/qtest/libqtest.c:765
> #13 0x02aa00413f1e in wait_command (who=who@entry=0x2aa01eb2700, 
> command=command@entry=0x2aa00487100 "{ 'execute': 'query-migrate' }")
> at ../tests/qtest/migration-helpers.c:73
> #14 0x02aa00414078 in migrate_query (who=who@entry=0x2aa01eb2700) at 
> ../tests/qtest/migration-helpers.c:139
> #15 migrate_query_status (who=who@entry=0x2aa01eb2700) at 
> ../tests/qtest/migration-helpers.c:161
> #16 0x02aa00414480 in check_migration_status (ungoals=0x0, 
> goal=0x2aa00495c7e "completed", who=0x2aa01eb2700) at 
> ../tests/qtest/migration-helpers.c:177
> #17 wait_for_migration_status (who=0x2aa01eb2700, goal=, 
> ungoals=0x0) at ../tests/qtest/migration-helpers.c:202
> #18 0x02aa0041300e in migrate_postcopy_complete 
> (from=from@entry=0x2aa01eb2700, to=to@entry=0x2aa01eb3000, 
> args=args@entry=0x3ffc247cf48)
> at ../tests/qtest/migration-test.c:1137
> #19 0x02aa004131a4 in test_postcopy_common (args=0x3ffc247cf48) at 
> ../tests/qtest/migration-test.c:1162
> #20 test_postcopy_preempt () at ../tests/qtest/migration-test.c:1178
> 
> Looking at the iov and file it's garbage; so it makes me think this is
>

Re: [PATCH 2/3] Add support for TPM devices over I2C bus





On 3/21/23 01:30, Ninad Palsule wrote:

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices.

This commit includes changes for the common code.
- Added support for the new checksum registers which are required for
   the I2C support. The checksum calculation is handled in the qemu
   common code.
- Added wrapper function for read and write data so that I2C code can
   call it without MMIO interface.

Signed-off-by: Ninad Palsule 
---
  hw/tpm/tpm_tis.h|  2 ++
  hw/tpm/tpm_tis_common.c | 33 +
  include/hw/acpi/tpm.h   |  2 ++
  3 files changed, 37 insertions(+)

diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
index f6b5872ba6..16b7baddd8 100644
--- a/hw/tpm/tpm_tis.h
+++ b/hw/tpm/tpm_tis.h
@@ -86,5 +86,7 @@ int tpm_tis_pre_save(TPMState *s);
  void tpm_tis_reset(TPMState *s);
  enum TPMVersion tpm_tis_get_tpm_version(TPMState *s);
  void tpm_tis_request_completed(TPMState *s, int ret);
+uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size);
+void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size);
  
  #endif /* TPM_TPM_TIS_H */

diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
index 503be2a541..3c82f63179 100644
--- a/hw/tpm/tpm_tis_common.c
+++ b/hw/tpm/tpm_tis_common.c
@@ -26,6 +26,8 @@
  #include "hw/irq.h"
  #include "hw/isa/isa.h"
  #include "qapi/error.h"
+#include "qemu/bswap.h"
+#include "qemu/crc-ccitt.h"
  #include "qemu/module.h"
  
  #include "hw/acpi/tpm.h"

@@ -422,6 +424,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
  shift = 0; /* no more adjustments */
  }
  break;
+case TPM_TIS_REG_DATA_CSUM_GET:
+val = bswap16(crc_ccitt(0, s->buffer, s->rw_offset));


Should this not rather be cpu_to_be16() so that it would also work on a big 
endian host (assuming you tested this on a little e endian host)?


+break;
  case TPM_TIS_REG_INTERFACE_ID:
  val = s->loc[locty].iface_id;
  break;
@@ -447,6 +452,15 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr 
addr,
  return val;
  }
  
+/*

+ * A wrapper read function so that it can be directly called without
+ * mmio.
+ */
+uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size)
+{
+return tpm_tis_mmio_read(s, addr, size);
+}
+
  /*
   * Write a value to a register of the TIS interface
   * See specs pages 33-63 for description of the registers
@@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
  case TPM_TIS_REG_INT_VECTOR:
  /* hard wired -- ignore */
  break;
+case TPM_TIS_REG_DATA_CSUM_ENABLE:
+/*
+ * Checksum implemented by common code so no need to set
+ * any flags.
+ */
+break;
+case TPM_TIS_REG_DATA_CSUM_GET:
+/* This is readonly register so ignore */
+break;
  case TPM_TIS_REG_INT_STATUS:
  if (s->active_locty != locty) {
  break;
@@ -703,6 +726,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
  break;
  case TPM_TIS_REG_DATA_FIFO:
  case TPM_TIS_REG_DATA_XFIFO ... TPM_TIS_REG_DATA_XFIFO_END:
+


you can remove this one


  /* data fifo */
  if (s->active_locty != locty) {
  break;
@@ -767,6 +791,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
  }
  }
  
+/*

+ * A wrapper write function so that it can be directly called without
+ * mmio.
+ */
+void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size)
+{
+tpm_tis_mmio_write(s, addr, val, size);
+}'
+
  const MemoryRegionOps tpm_tis_memory_ops = {
  .read = tpm_tis_mmio_read,
  .write = tpm_tis_mmio_write,
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 559ba6906c..db12c002f4 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -40,6 +40,8 @@
  #define TPM_TIS_REG_STS   0x18
  #define TPM_TIS_REG_DATA_FIFO 0x24
  #define TPM_TIS_REG_INTERFACE_ID  0x30
+#define TPM_TIS_REG_DATA_CSUM_ENABLE  0x40
+#define TPM_TIS_REG_DATA_CSUM_GET 0x44
  #define TPM_TIS_REG_DATA_XFIFO0x80
  #define TPM_TIS_REG_DATA_XFIFO_END0xbc
  #define TPM_TIS_REG_DID_VID   0xf00


Looks good.

[RFC PATCH] tests/qemu-iotests: serialise all the qemu-iotests

Something on OpenBSD fails with multiple tests running at once and
fiddling with J=1 on invocation just made everything else very slow.

Based-on: 20230318114644.1340899-1-alex.ben...@linaro.org
Signed-off-by: Alex Bennée 
Cc: Daniel P. Berrangé 
---
 tests/qemu-iotests/meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index a162f683ef..d572205a60 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -64,6 +64,7 @@ foreach format, speed: qemu_iotests_formats
depends: qemu_iotests_binaries,
env: qemu_iotests_env,
protocol: 'tap',
+   is_parallel : false,
timeout: 180,
suite: suites)
   endforeach
-- 
2.39.2

Re: [PATCH 1/3] Add support for TPM devices over I2C bus





On 3/21/23 01:29, Ninad Palsule wrote:

This is a documentation change for I2C TPM device support.

Qemu already supports devices attached to ISA and sysbus.
This drop adds support for the I2C bus attached TPM devices.

Signed-off-by: Ninad Palsule 
---
  docs/specs/tpm.rst | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 535912a92b..79a79f0640 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -21,11 +21,14 @@ QEMU files related to TPM TIS interface:
   - ``hw/tpm/tpm_tis_common.c``
   - ``hw/tpm/tpm_tis_isa.c``
   - ``hw/tpm/tpm_tis_sysbus.c``
+ - ``hw/tpm/tpm_tis_i2c.c``
   - ``hw/tpm/tpm_tis.h``
  
  Both an ISA device and a sysbus device are available. The former is

  used with pc/q35 machine while the latter can be instantiated in the
-Arm virt machine.
+Arm virt machine. An I2C device support is also added which can be
+instantiated in the arm based emulation machine. I2C model only supports
+TPM2 protocol.



An I2C device is also supported for the Arm virt machine. This device only 
supports the TPM 2 protocol.

   Stefan

  
  CRB interface

  -

Re: [RFC PATCH] tests/avocado: re-factor igb test to avoid timeouts


On 21/3/23 19:17, Alex Bennée wrote:

The core of the test was utilising "ethtool -t eth1 offline" to run
through a test sequence. For reasons unknown the test hangs under some
configurations of the build on centos8-stream. Fundamentally running
the old fedora-31 cloud-init is just too much for something that is
directed at testing one device. So we:

   - replace fedora with a custom kernel + buildroot rootfs
   - rename the test from IGB to NetDevEthtool
   - re-factor the common code, add (currently skipped) tests for other
  devices which support ethtool
   - remove the KVM limitation as its fast enough to run in KVM or TCG

Signed-off-by: Alex Bennée 
Cc: Akihiko Odaki 
---
  tests/avocado/igb.py| 38 --
  tests/avocado/netdev-ethtool.py | 93 +
  2 files changed, 93 insertions(+), 38 deletions(-)
  delete mode 100644 tests/avocado/igb.py
  create mode 100644 tests/avocado/netdev-ethtool.py

diff --git a/tests/avocado/igb.py b/tests/avocado/igb.py
deleted file mode 100644
index abf5dfa07f..00
--- a/tests/avocado/igb.py
+++ /dev/null
@@ -1,38 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0-or-later
-# ethtool tests for igb registers, interrupts, etc
-
-from avocado_qemu import LinuxTest
-
-class IGB(LinuxTest):
-"""
-:avocado: tags=accel:kvm
-:avocado: tags=arch:x86_64
-:avocado: tags=distro:fedora
-:avocado: tags=distro_version:31
-:avocado: tags=machine:q35
-"""
-
-timeout = 180
-
-def test(self):
-self.require_accelerator('kvm')
-kernel_url = self.distro.pxeboot_url + 'vmlinuz'
-kernel_hash = '5b6f6876e1b5bda314f93893271da0d5777b1f3c'
-kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
-initrd_url = self.distro.pxeboot_url + 'initrd.img'
-initrd_hash = 'dd0340a1b39bd28f88532babd4581c67649ec5b1'
-initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
-
-# Ideally we want to test MSI as well, but it is blocked by a bug
-# fixed with:
-# 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=28e96556baca7056d11d9fb3cdd0aba4483e00d8
-kernel_params = self.distro.default_kernel_params + ' pci=nomsi'
-
-self.vm.add_args('-kernel', kernel_path,
- '-initrd', initrd_path,
- '-append', kernel_params,
- '-accel', 'kvm',
- '-device', 'igb')
-self.launch_and_wait()
-self.ssh_command('dnf -y install ethtool')
-self.ssh_command('ethtool -t eth1 offline')
diff --git a/tests/avocado/netdev-ethtool.py b/tests/avocado/netdev-ethtool.py
new file mode 100644
index 00..da0a22d51c
--- /dev/null
+++ b/tests/avocado/netdev-ethtool.py
@@ -0,0 +1,93 @@
+# ethtool tests for emulated network devices
+#
+# This test leverages ethtool's --test sequence to validate network
+# device behaviour.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import time
+
+from avocado import skip
+
+from avocado_qemu import QemuSystemTest
+from avocado_qemu import exec_command, exec_command_and_wait_for_pattern
+from avocado_qemu import wait_for_console_pattern
+
+class NetDevEthtool(QemuSystemTest):
+"""
+:avocado: tags=arch:x86_64
+:avocado: tags=machine:q35
+"""
+
+KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 root=/dev/sda console=ttyS0 '
+# Runs in about 20s under KVM, 26s under TCG, 37s under GCOV
+timeout = 45
+
+def common_test_code(self, netdev, extra_args=None):
+base_url = ('https://fileserver.linaro.org/s/'
+'kE4nCFLdQcoBF9t/download?'
+'path=%2Figb-net-test=' )
+
+# This custom kernel has drivers for all the supported network
+# devices we can emulate in QEMU
+kernel_url = base_url + 'bzImage'
+kernel_hash = '784daede6dab993597f36efbf01f69f184c55152'
+kernel_path = self.fetch_asset(name="bzImage",
+   locations=(kernel_url), 
asset_hash=kernel_hash)
+
+rootfs_url = base_url + 'rootfs.ext4'
+rootfs_hash = '7d28c1bf429de3b441a63756a51f163442ea574b'
+rootfs_path = self.fetch_asset(name="rootfs.ext4",
+   locations=(rootfs_url),
+   asset_hash=rootfs_hash)
+
+kernel_params = self.KERNEL_COMMON_COMMAND_LINE
+if extra_args:
+kernel_params += extra_args
+
+self.vm.add_args('-kernel', kernel_path,
+ '-append', kernel_params,
+ '-blockdev',
+ 
f"driver=raw,file.driver=file,file.filename={rootfs_path},node-name=hd0",
+ '-device', 'driver=ide-hd,bus=ide.0,unit=0,drive=hd0',
+ '-device', netdev)
+
+self.vm.set_console(console_index=0)
+self.vm.launch()
+
+

Re: [PATCH v4 3/4] qga/vss-win32: fix warning for clang++-15

2023-03-21 Thread Pierrick Bouvier

Sorry to come back on this, but it seems this specific commit was not 
integrated in trunk.


@Konstantin Kostiuk: If you plan to integrate this later (before 8.0 
tag), sorry for the noise. Since rc1 was published today, I think it may 
have been "lost".


If someone wants to merge it, that would be nice.

Thanks,
Pierrick

On 2/21/23 16:30, Pierrick Bouvier wrote:

Reported when compiling with clang-windows-arm64.

../qga/vss-win32/install.cpp:537:9: error: variable 'hr' is used uninitialized 
whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
 if (!(ControlService(service, SERVICE_CONTROL_STOP, NULL))) {
 ^~
../qga/vss-win32/install.cpp:545:12: note: uninitialized use occurs here
 return hr;
^~
Signed-off-by: Pierrick Bouvier 
Fixes: 917ebcb170 ("qga-win: Fix QGA VSS Provider service stop failure")
Reviewed-by: Konstantin Kostiuk 
Reviewed-by: Philippe Mathieu-Daudé 
---
  qga/vss-win32/install.cpp | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index b57508fbe0..b8087e5baa 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -518,7 +518,7 @@ namespace _com_util
  /* Stop QGA VSS provider service using Winsvc API  */
  STDAPI StopService(void)
  {
-HRESULT hr;
+HRESULT hr = S_OK;
  SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
  SC_HANDLE service = NULL;

Re: [PATCH v3 2/3] qapi: Do not generate empty enum

2023-03-21 Thread Eric Blake

On Tue, Mar 21, 2023 at 03:19:28PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote:
> > On 16/3/23 15:57, Markus Armbruster wrote:
> > > Daniel P. Berrangé  writes:
> > > 
> > > > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
> > > > > Philippe Mathieu-Daudé  writes:
> > > > > 
> > > > > > Per the C++ standard, empty enum are ill-formed. Do not generate
> > > 
> > > The C standard.  The C++ standard doesn't apply here :)
> > 
> > I can't find how empty enums are considered by the C standard...
> 
> The C standard doesn't really matter either.
> 
> What we actually care about is whether GCC and CLang consider the
> empty enums to be permissible or not. or to put it another way...
> if it compiles, ship it :-)

But it doesn't compile:

$ cat foo.c
typedef enum Blah {
} Blah;
int main(void) {
  Blah b = 0;
}
$ gcc -o foo -Wall foo.c
foo.c:2:1: error: empty enum is invalid
2 | } Blah;
  | ^
foo.c: In function ‘main’:
foo.c:4:5: error: unknown type name ‘Blah’; use ‘enum’ keyword to refer to the 
type
4 | Blah b = 0;
  | ^~~~
  | enum 
foo.c:4:10: warning: unused variable ‘b’ [-Wunused-variable]
4 | Blah b = 0;
  |  ^

So we _do_ need to avoid creating an enum with all members optional in
the configuration where all options are disabled, if we want that
configuration to compile.  Or require that all QAPI enums have at
least one non-optional member.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

RE: [PATCH v3 2/2] Use black code style for python scripts

2023-03-21 Thread Taylor Simpson




> -Original Message-
> From: Marco Liebel (QUIC) 
> Sent: Monday, March 20, 2023 3:26 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson ; Philippe Mathieu-Daudé
> ; Markus Armbruster ; Daniel P .
> Berrangé ; Marco Liebel (QUIC)
> 
> Subject: [PATCH v3 2/2] Use black code style for python scripts
> 
> Signed-off-by: Marco Liebel 
> ---
>  target/hexagon/dectree.py   | 396 ---
>  target/hexagon/gen_analyze_funcs.py | 135 +++---
>  target/hexagon/gen_helper_funcs.py  | 338 +++--
>  target/hexagon/gen_helper_protos.py | 165 ---
>  target/hexagon/gen_idef_parser_funcs.py |  75 +--
>  target/hexagon/gen_op_attribs.py|  10 +-
>  target/hexagon/gen_op_regs.py   |  77 +--
>  target/hexagon/gen_opcodes_def.py   |   4 +-
>  target/hexagon/gen_printinsn.py |  80 +--
>  target/hexagon/gen_shortcode.py |  17 +-
>  target/hexagon/gen_tcg_func_table.py|  14 +-
>  target/hexagon/gen_tcg_funcs.py | 614 +---
>  target/hexagon/hex_common.py| 177 ---
>  13 files changed, 1191 insertions(+), 911 deletions(-)

Acked-by: Taylor Simpson 
Tested-by: Taylor Simpson

RE: [PATCH v3 1/2] Use f-strings in python scripts

2023-03-21 Thread Taylor Simpson




> -Original Message-
> From: Marco Liebel (QUIC) 
> Sent: Monday, March 20, 2023 3:26 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson ; Philippe Mathieu-Daudé
> ; Markus Armbruster ; Daniel P .
> Berrangé ; Marco Liebel (QUIC)
> 
> Subject: [PATCH v3 1/2] Use f-strings in python scripts
> 
> Replace python 2 format string with f-strings
> 
> Signed-off-by: Marco Liebel 
> ---
>  target/hexagon/gen_analyze_funcs.py | 115 -
>  target/hexagon/gen_helper_funcs.py  |  54 ++--
>  target/hexagon/gen_helper_protos.py |  10 +-
>  target/hexagon/gen_idef_parser_funcs.py |   8 +-
>  target/hexagon/gen_op_attribs.py|   4 +-
>  target/hexagon/gen_op_regs.py   |  10 +-
>  target/hexagon/gen_opcodes_def.py   |   2 +-
>  target/hexagon/gen_printinsn.py |  14 +-
>  target/hexagon/gen_shortcode.py |   2 +-
>  target/hexagon/gen_tcg_func_table.py|   2 +-
>  target/hexagon/gen_tcg_funcs.py | 317 +++-
>  target/hexagon/hex_common.py|   4 +-
>  12 files changed, 243 insertions(+), 299 deletions(-)

Reviewed-by: Taylor Simpson 
Tested-by: Taylor Simpson

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

2023-03-21 Thread Paolo Bonzini

Il lun 20 mar 2023, 16:42 Thomas Huth  ha scritto:

> Would it make sense to add it e.g. to "-action" instead, i.e. something
> like
> "-action teardown=async" ?
>

-action is just a wrapper for the action-set QMP command. I don't think it
fits very well; its arguments are only guest actions while asynchronous
tear down happens for example when you issue a quit command on the monitor.

Paolo

  Thomas
>
>

Re: About the instance_finalize callback in VFIO PCI

2023-03-21 Thread Paolo Bonzini

Il mar 21 mar 2023, 18:30 Cédric Le Goater  ha scritto:

> I would have thought that user_creatable_cleanup would have taken care
> of it. But it's not. This needs some digging.
>

user_creatable_cleanup is only for -object, not for -device.

Paolo


> C.
>
>
> > By the way, i also debugged other instance_finalize callback functions,
> > if my understanding is right, all instance_finalize callback should be
> > called from object_unref(object) from qemu_cleanup(void) in
> > ./softmmu/runstate.c. But there is no VFIO related object_unref() call in
> > this cleanup function, So the instance_finalize callback in vfio pci
> > should be useless? thanks!
> >
> > Regards,
> > Yang
> >
> >
>
>

s390 migration crash

2023-03-21 Thread Dr. David Alan Gilbert

Hi Peter's,
  Peter M pointed me to a seg in a migration test in CI; I can reproduce
it:
  * On an s390 host
  * only as part of a make check - running migration-test by itself
doesn't trigger for me.
  * It looks like it's postcopy preempt

(gdb) bt full
#0  iov_size (iov=iov@entry=0x2aa00e60670, iov_cnt=) at 
../util/iov.c:88
len = 13517923312037845750
i = 17305
#1  0x02aa004d068c in qemu_fflush (f=0x2aa00e58630) at 
../migration/qemu-file.c:307
local_error = 0x0
#2  0x02aa004d0e04 in qemu_fflush (f=) at 
../migration/qemu-file.c:297
#3  0x02aa00613962 in postcopy_preempt_shutdown_file 
(s=s@entry=0x2aa00d1b4e0) at ../migration/ram.c:4657
#4  0x02aa004e12b4 in migration_completion (s=0x2aa00d1b4e0) at 
../migration/migration.c:3469
ret = 
current_active_state = 5
must_precopy = 0
can_postcopy = 0
in_postcopy = true
pending_size = 0
__func__ = "migration_iteration_run"
iter_state = 
s = 0x2aa00d1b4e0
thread = 
setup_start = 
thr_error = 
urgent = 
#5  migration_iteration_run (s=0x2aa00d1b4e0) at ../migration/migration.c:3882
must_precopy = 0
can_postcopy = 0
in_postcopy = true
pending_size = 0
__func__ = "migration_iteration_run"
iter_state = 
s = 0x2aa00d1b4e0
thread = 
setup_start = 
thr_error = 
urgent = 
#6  migration_thread (opaque=opaque@entry=0x2aa00d1b4e0) at 
../migration/migration.c:4124
iter_state = 
s = 0x2aa00d1b4e0
--Type  for more, q to quit, c to continue without paging--
thread = 
setup_start = 
thr_error = 
urgent = 
#7  0x02aa00819b8c in qemu_thread_start (args=) at 
../util/qemu-thread-posix.c:541
__cancel_buf = 
{__cancel_jmp_buf = {{__cancel_jmp_buf = {{__gregs = 
{4396782422080, 4393751543808, 4397299389454, 4396844235904, 2929182727824, 
2929182933488, 4396843986792, 4397299389455, 33679382915066768, 
33678512846981306}, __fpregs = {4396774031360, 8392704, 2929182933488, 0, 
4396782422272, 2929172491858, 4396774031360, 1}}}, __mask_was_saved = 0}}, 
__pad = {0x3ffb4a77a60, 0x0, 0x0, 0x0}}
__cancel_routine = 0x2aa00819bf0 
__not_first_call = 
start_routine = 0x2aa004e08f0 
arg = 0x2aa00d1b4e0
r = 
#8  0x03ffb7b1e2e6 in start_thread () at /lib64/libc.so.6
#9  0x03ffb7aafdbe in thread_start () at /lib64/libc.so.6

It looks like it's in the preempt test:

(gdb) where
#0  0x03ffb17a0126 in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x03ffb1750890 in raise () from /lib64/libc.so.6
#2  0x03ffb172a340 in abort () from /lib64/libc.so.6
#3  0x02aa0041c130 in qtest_check_status (s=) at 
../tests/qtest/libqtest.c:194
#4  0x03ffb1a3b5de in g_hook_list_invoke () from /lib64/libglib-2.0.so.0
#5  
#6  0x03ffb17a0126 in __pthread_kill_implementation () from /lib64/libc.so.6
#7  0x03ffb1750890 in raise () from /lib64/libc.so.6
#8  0x03ffb172a340 in abort () from /lib64/libc.so.6
#9  0x02aa00420318 in qmp_fd_receive (fd=) at 
../tests/qtest/libqmp.c:80
#10 0x02aa0041d5ee in qtest_qmp_receive_dict (s=0x2aa01eb2700) at 
../tests/qtest/libqtest.c:713
#11 qtest_qmp_receive (s=0x2aa01eb2700) at ../tests/qtest/libqtest.c:701
#12 qtest_vqmp (s=s@entry=0x2aa01eb2700, fmt=fmt@entry=0x2aa00487100 "{ 
'execute': 'query-migrate' }", ap=ap@entry=0x3ffc247cc68)
at ../tests/qtest/libqtest.c:765
#13 0x02aa00413f1e in wait_command (who=who@entry=0x2aa01eb2700, 
command=command@entry=0x2aa00487100 "{ 'execute': 'query-migrate' }")
at ../tests/qtest/migration-helpers.c:73
#14 0x02aa00414078 in migrate_query (who=who@entry=0x2aa01eb2700) at 
../tests/qtest/migration-helpers.c:139
#15 migrate_query_status (who=who@entry=0x2aa01eb2700) at 
../tests/qtest/migration-helpers.c:161
#16 0x02aa00414480 in check_migration_status (ungoals=0x0, 
goal=0x2aa00495c7e "completed", who=0x2aa01eb2700) at 
../tests/qtest/migration-helpers.c:177
#17 wait_for_migration_status (who=0x2aa01eb2700, goal=, 
ungoals=0x0) at ../tests/qtest/migration-helpers.c:202
#18 0x02aa0041300e in migrate_postcopy_complete 
(from=from@entry=0x2aa01eb2700, to=to@entry=0x2aa01eb3000, 
args=args@entry=0x3ffc247cf48)
at ../tests/qtest/migration-test.c:1137
#19 0x02aa004131a4 in test_postcopy_common (args=0x3ffc247cf48) at 
../tests/qtest/migration-test.c:1162
#20 test_postcopy_preempt () at ../tests/qtest/migration-test.c:1178

Looking at the iov and file it's garbage; so it makes me think this is
something like a flush on a closed file.

Dave

-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[PATCH qemu v3] linux-user: Emulate /proc/cpuinfo output for riscv

2023-03-21 Thread Afonso Bordado


RISC-V does not expose all extensions via hwcaps, thus some userspace
applications may want to query these via /proc/cpuinfo.

Currently when querying this file the host's file is shown instead
which is slightly confusing. Emulate a basic /proc/cpuinfo file
with mmu info and an ISA string.

Signed-off-by: Afonso Bordado 
Reviewed-by: Palmer Dabbelt 
Acked-by: Palmer Dabbelt 
Reviewed-by: Laurent Vivier 
Reviewed-by: Alistair Francis 
Reviewed-by: LIU Zhiwei 
---

Thanks everyone for reviewing! Should I resend this once the 8.0
freeze is over? Or does someone queue it for inclusion in the next
version?


Changes from V2:
- Update ChangeLog Location

Changes from V1:
- Call `g_free` on ISA string.
- Use `riscv_cpu_cfg` API.
- Query `cpu_env->xl` to check for RV32.


 linux-user/syscall.c  | 34 +--
 tests/tcg/riscv64/Makefile.target |  1 +
 tests/tcg/riscv64/cpuinfo.c   | 30 +++
 3 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/riscv64/cpuinfo.c

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 24cea6fb6a..0388f8b0b0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8230,7 +8230,8 @@ void target_exception_dump(CPUArchState *env, const char 
*fmt, int code)
 }
 
 #if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN || \

-defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
+defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA) || \
+defined(TARGET_RISCV)
 static int is_proc(const char *filename, const char *entry)
 {
 return strcmp(filename, entry) == 0;
@@ -8308,6 +8309,35 @@ static int open_cpuinfo(CPUArchState *cpu_env, int fd)
 }
 #endif
 
+#if defined(TARGET_RISCV)

+static int open_cpuinfo(CPUArchState *cpu_env, int fd)
+{
+int i;
+int num_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+RISCVCPU *cpu = env_archcpu(cpu_env);
+const RISCVCPUConfig *cfg = riscv_cpu_cfg((CPURISCVState *) cpu_env);
+char *isa_string = riscv_isa_string(cpu);
+const char *mmu;
+
+if (cfg->mmu) {
+mmu = (cpu_env->xl == MXL_RV32) ? "sv32"  : "sv48";
+} else {
+mmu = "none";
+}
+
+for (i = 0; i < num_cpus; i++) {
+dprintf(fd, "processor\t: %d\n", i);
+dprintf(fd, "hart\t\t: %d\n", i);
+dprintf(fd, "isa\t\t: %s\n", isa_string);
+dprintf(fd, "mmu\t\t: %s\n", mmu);
+dprintf(fd, "uarch\t\t: qemu\n\n");
+}
+
+g_free(isa_string);
+return 0;
+}
+#endif
+
 #if defined(TARGET_M68K)
 static int open_hardware(CPUArchState *cpu_env, int fd)
 {
@@ -8332,7 +8362,7 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, 
const char *pathname, int
 #if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN
 { "/proc/net/route", open_net_route, is_proc },
 #endif
-#if defined(TARGET_SPARC) || defined(TARGET_HPPA)
+#if defined(TARGET_SPARC) || defined(TARGET_HPPA) || defined(TARGET_RISCV)
 { "/proc/cpuinfo", open_cpuinfo, is_proc },
 #endif
 #if defined(TARGET_M68K)
diff --git a/tests/tcg/riscv64/Makefile.target
b/tests/tcg/riscv64/Makefile.target
index cc3ed65ffd..df93a2ce1f 100644
--- a/tests/tcg/riscv64/Makefile.target
+++ b/tests/tcg/riscv64/Makefile.target
@@ -4,6 +4,7 @@
 VPATH += $(SRC_PATH)/tests/tcg/riscv64
 TESTS += test-div
 TESTS += noexec
+TESTS += cpuinfo
 
 # Disable compressed instructions for test-noc

 TESTS += test-noc
diff --git a/tests/tcg/riscv64/cpuinfo.c b/tests/tcg/riscv64/cpuinfo.c
new file mode 100644
index 00..296abd0a8c
--- /dev/null
+++ b/tests/tcg/riscv64/cpuinfo.c
@@ -0,0 +1,30 @@
+#include 
+#include 
+#include 
+#include 
+
+#define BUFFER_SIZE 1024
+
+int main(void)
+{
+char buffer[BUFFER_SIZE];
+FILE *fp = fopen("/proc/cpuinfo", "r");
+assert(fp != NULL);
+
+while (fgets(buffer, BUFFER_SIZE, fp) != NULL) {
+if (strstr(buffer, "processor") != NULL) {
+assert(strstr(buffer, "processor\t: ") == buffer);
+} else if (strstr(buffer, "hart") != NULL) {
+assert(strstr(buffer, "hart\t\t: ") == buffer);
+} else if (strstr(buffer, "isa") != NULL) {
+assert(strcmp(buffer, "isa\t\t: rv64imafdc_zicsr_zifencei\n") == 
0);
+} else if (strstr(buffer, "mmu") != NULL) {
+assert(strcmp(buffer, "mmu\t\t: sv48\n") == 0);
+} else if (strstr(buffer, "uarch") != NULL) {
+assert(strcmp(buffer, "uarch\t\t: qemu\n") == 0);
+}
+}
+
+fclose(fp);
+return 0;
+}
--
2.34.7

[RFC PATCH v2 0/2] Providing mount in memfd_restricted() syscall

2023-03-21 Thread Ackerley Tng

Hello,

This patchset builds upon the memfd_restricted() system call that was
discussed in the 'KVM: mm: fd-based approach for supporting KVM' patch
series, at
https://lore.kernel.org/lkml/20221202061347.1070246-1-chao.p.p...@linux.intel.com/T/#m7e944d7892afdd1d62a03a287bd488c56e377b0c

The tree can be found at:
https://github.com/googleprodkernel/linux-cc/tree/restrictedmem-provide-mount-fd

In this patchset, a modification to the memfd_restricted() syscall is
proposed, which allows userspace to provide a mount, on which the
restrictedmem file will be created and returned from the
memfd_restricted().

Allowing userspace to provide a mount allows userspace to control
various memory binding policies via tmpfs mount options, such as
Transparent HugePage memory allocation policy through
'huge=always/never' and NUMA memory allocation policy through
'mpol=local/bind:*'.

Changes since RFCv1:
+ Use fd to represent mount instead of path string, as Kirill
  suggested. I believe using fds makes this syscall interface more
  aligned with the other syscalls like fsopen(), fsconfig(), and
  fsmount() in terms of using and passing around fds
+ Remove unused variable char *orig_shmem_enabled from selftests

Dependencies:
+ Sean's iteration of the ‘KVM: mm: fd-based approach for supporting
  KVM’ patch series at
  https://github.com/sean-jc/linux/tree/x86/upm_base_support
+ Proposed fixes for these issues mentioned on the mailing list:
+ 
https://lore.kernel.org/lkml/diqzzga0fv96@ackerleytng-cloudtop-sg.c.googlers.com/

Links to earlier patch series:
+ RFC v1:
  https://lore.kernel.org/lkml/cover.1676507663.git.ackerley...@google.com/T/

Ackerley Tng (2):
  mm: restrictedmem: Allow userspace to specify mount for
memfd_restricted
  selftests: restrictedmem: Check hugepage-ness of shmem file backing
restrictedmem fd

 include/linux/syscalls.h  |   2 +-
 include/uapi/linux/restrictedmem.h|   8 +
 mm/restrictedmem.c|  63 ++-
 tools/testing/selftests/Makefile  |   1 +
 .../selftests/restrictedmem/.gitignore|   3 +
 .../testing/selftests/restrictedmem/Makefile  |  15 +
 .../testing/selftests/restrictedmem/common.c  |   9 +
 .../testing/selftests/restrictedmem/common.h  |   8 +
 .../restrictedmem_hugepage_test.c | 459 ++
 9 files changed, 561 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/restrictedmem.h
 create mode 100644 tools/testing/selftests/restrictedmem/.gitignore
 create mode 100644 tools/testing/selftests/restrictedmem/Makefile
 create mode 100644 tools/testing/selftests/restrictedmem/common.c
 create mode 100644 tools/testing/selftests/restrictedmem/common.h
 create mode 100644 
tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c

--
2.40.0.rc2.332.ga46443480c-goog

[RFC PATCH v2 2/2] selftests: restrictedmem: Check hugepage-ness of shmem file backing restrictedmem fd

2023-03-21 Thread Ackerley Tng

For memfd_restricted() calls without a userspace mount, the backing
file should be the shmem mount in the kernel, and the size of backing
pages should be as defined by system-wide shmem configuration.

If a userspace mount is provided, the size of backing pages should be
as defined in the mount.

Signed-off-by: Ackerley Tng 
---
 tools/testing/selftests/Makefile  |   1 +
 .../selftests/restrictedmem/.gitignore|   3 +
 .../testing/selftests/restrictedmem/Makefile  |  15 +
 .../testing/selftests/restrictedmem/common.c  |   9 +
 .../testing/selftests/restrictedmem/common.h  |   8 +
 .../restrictedmem_hugepage_test.c | 459 ++
 6 files changed, 495 insertions(+)
 create mode 100644 tools/testing/selftests/restrictedmem/.gitignore
 create mode 100644 tools/testing/selftests/restrictedmem/Makefile
 create mode 100644 tools/testing/selftests/restrictedmem/common.c
 create mode 100644 tools/testing/selftests/restrictedmem/common.h
 create mode 100644 
tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index f07aef7c592c..44078eeefb79 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -60,6 +60,7 @@ TARGETS += pstore
 TARGETS += ptrace
 TARGETS += openat2
 TARGETS += resctrl
+TARGETS += restrictedmem
 TARGETS += rlimits
 TARGETS += rseq
 TARGETS += rtc
diff --git a/tools/testing/selftests/restrictedmem/.gitignore 
b/tools/testing/selftests/restrictedmem/.gitignore
new file mode 100644
index ..2581bcc8ff29
--- /dev/null
+++ b/tools/testing/selftests/restrictedmem/.gitignore
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+restrictedmem_hugepage_test
diff --git a/tools/testing/selftests/restrictedmem/Makefile 
b/tools/testing/selftests/restrictedmem/Makefile
new file mode 100644
index ..8e5378d20226
--- /dev/null
+++ b/tools/testing/selftests/restrictedmem/Makefile
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0
+
+CFLAGS = $(KHDR_INCLUDES)
+CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -std=gnu99
+
+TEST_GEN_PROGS += restrictedmem_hugepage_test
+
+include ../lib.mk
+
+EXTRA_CLEAN = $(OUTPUT)/common.o
+
+$(OUTPUT)/common.o: common.c
+   $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -ffreestanding $< -o $@
+
+$(TEST_GEN_PROGS): $(OUTPUT)/common.o
diff --git a/tools/testing/selftests/restrictedmem/common.c 
b/tools/testing/selftests/restrictedmem/common.c
new file mode 100644
index ..03dac843404f
--- /dev/null
+++ b/tools/testing/selftests/restrictedmem/common.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include 
+#include 
+
+int memfd_restricted(unsigned int flags, int mount_fd)
+{
+   return syscall(__NR_memfd_restricted, flags, mount_fd);
+}
diff --git a/tools/testing/selftests/restrictedmem/common.h 
b/tools/testing/selftests/restrictedmem/common.h
new file mode 100644
index ..06284ed86baf
--- /dev/null
+++ b/tools/testing/selftests/restrictedmem/common.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef SELFTESTS_RESTRICTEDMEM_COMMON_H
+#define SELFTESTS_RESTRICTEDMEM_COMMON_H
+
+int memfd_restricted(unsigned int flags, int mount_fd);
+
+#endif  // SELFTESTS_RESTRICTEDMEM_COMMON_H
diff --git 
a/tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c 
b/tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c
new file mode 100644
index ..ae37148342fe
--- /dev/null
+++ b/tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define _GNU_SOURCE /* for O_PATH */
+#define _POSIX_C_SOURCE /* for PATH_MAX */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "linux/restrictedmem.h"
+
+#include "common.h"
+#include "../kselftest_harness.h"
+
+/*
+ * Expect policy to be one of always, within_size, advise, never,
+ * deny, force
+ */
+#define POLICY_BUF_SIZE 12
+
+static int get_hpage_pmd_size(void)
+{
+   FILE *fp;
+   char buf[100];
+   char *ret;
+   int size;
+
+   fp = fopen("/sys/kernel/mm/transparent_hugepage/hpage_pmd_size", "r");
+   if (!fp)
+   return -1;
+
+   ret = fgets(buf, 100, fp);
+   if (ret != buf) {
+   size = -1;
+   goto out;
+   }
+
+   if (sscanf(buf, "%d\n", ) != 1)
+   size = -1;
+
+out:
+   fclose(fp);
+
+   return size;
+}
+
+static bool is_valid_shmem_thp_policy(char *policy)
+{
+   if (strcmp(policy, "always") == 0)
+   return true;
+   if (strcmp(policy, "within_size") == 0)
+   return true;
+   if (strcmp(policy, "advise") == 0)
+   return true;
+   if (strcmp(policy, "never") == 0)
+   return true;
+   if (strcmp(policy, "deny") == 0)
+   return true;
+   if

[RFC PATCH v2 1/2] mm: restrictedmem: Allow userspace to specify mount for memfd_restricted

2023-03-21 Thread Ackerley Tng

By default, the backing shmem file for a restrictedmem fd is created
on shmem's kernel space mount.

With this patch, an optional tmpfs mount can be specified via an fd,
which will be used as the mountpoint for backing the shmem file
associated with a restrictedmem fd.

This change is modeled after how sys_open() can create an unnamed
temporary file in a given directory with O_TMPFILE.

This will help restrictedmem fds inherit the properties of the
provided tmpfs mounts, for example, hugepage allocation hints, NUMA
binding hints, etc.

Signed-off-by: Ackerley Tng 
---
 include/linux/syscalls.h   |  2 +-
 include/uapi/linux/restrictedmem.h |  8 
 mm/restrictedmem.c | 63 +++---
 3 files changed, 66 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/restrictedmem.h

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index f9e9e0c820c5..a23c4c385cd3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
 asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long 
len,
unsigned long home_node,
unsigned long flags);
-asmlinkage long sys_memfd_restricted(unsigned int flags);
+asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
 
 /*
  * Architecture-specific system calls
diff --git a/include/uapi/linux/restrictedmem.h 
b/include/uapi/linux/restrictedmem.h
new file mode 100644
index ..9f108dd1ac4c
--- /dev/null
+++ b/include/uapi/linux/restrictedmem.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
+#define _UAPI_LINUX_RESTRICTEDMEM_H
+
+/* flags for memfd_restricted */
+#define RMFD_TMPFILE   0x0001U
+
+#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */
diff --git a/mm/restrictedmem.c b/mm/restrictedmem.c
index c5d869d8c2d8..4d83b949d84e 100644
--- a/mm/restrictedmem.c
+++ b/mm/restrictedmem.c
@@ -1,11 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
-#include "linux/sbitmap.h"
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 
 struct restrictedmem {
@@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file 
*memfd)
return file;
 }
 
-SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
+static int restrictedmem_create(struct vfsmount *mount)
 {
struct file *file, *restricted_file;
int fd, err;
 
-   if (flags)
-   return -EINVAL;
-
fd = get_unused_fd_flags(0);
if (fd < 0)
return fd;
 
-   file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+   if (mount)
+   file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 
0, VM_NORESERVE);
+   else
+   file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
if (IS_ERR(file)) {
err = PTR_ERR(file);
goto err_fd;
@@ -223,6 +225,55 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
return err;
 }
 
+static bool is_shmem_mount(struct vfsmount *mnt)
+{
+   return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
+}
+
+static int restrictedmem_create_from_file(int mount_fd)
+{
+   int ret;
+   struct fd f;
+   struct vfsmount *mnt;
+
+   f = fdget_raw(mount_fd);
+   if (!f.file)
+   return -EBADF;
+
+   mnt = f.file->f_path.mnt;
+   if (!is_shmem_mount(mnt)) {
+   ret = -EINVAL;
+   goto out;
+   }
+
+   ret = mnt_want_write(mnt);
+   if (unlikely(ret))
+   goto out;
+
+   ret = restrictedmem_create(mnt);
+
+   mnt_drop_write(mnt);
+out:
+   fdput(f);
+
+   return ret;
+}
+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{
+   if (flags & ~RMFD_TMPFILE)
+   return -EINVAL;
+
+   if (flags == RMFD_TMPFILE) {
+   if (mount_fd < 0)
+   return -EINVAL;
+
+   return restrictedmem_create_from_file(mount_fd);
+   } else {
+   return restrictedmem_create(NULL);
+   }
+}
+
 int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
   struct restrictedmem_notifier *notifier, bool exclusive)
 {
-- 
2.40.0.rc2.332.ga46443480c-goog

[PATCH] vhost-user-blk-server: notify client about disk resize

2023-03-21 Thread Vladimir Sementsov-Ogievskiy

Currently block_resize qmp command is simply ignored by vhost-user-blk
export. So, the block-node is successfully resized, but virtio config
is unchanged and guest doesn't see that disk is resized.

Let's handle the resize by modifying the config and notifying the guest
appropriately.

After this comment, lsblk in linux guest with attached
vhost-user-blk-pci device shows new size immediately after block_resize
QMP command on vhost-user exported block node.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all! Seems, we have everything except this tiny patch to support live
disk resize for vhost-user export.

How I test:

./build/storage-daemon/qemu-storage-daemon --blockdev 
file,filename=a,node-name=f1 --export 
vhost-user-blk,node-name=f1,addr.type=unix,addr.path=/tmp/sock,writable=on,id=gg
 --chardev socket,path=qmp.sock,server=on,wait=off,id=char1 --monitor 
chardev=char1

In another terminal, connect to qmp interface by

nc -U qmp.sock

Launch QEMU:

./build/qemu-system-x86_64 -M q35,accel=kvm,memory-backend=mem -object 
memory-backend-file,share=on,id=mem,size=1G,mem-path=/dev/shm/qemu-ram -drive 
file=/home/vsementsov/work/vms/ub -chardev socket,path=/tmp/sock,id=char0 
-device vhost-user-blk-pci,chardev=char0

Then, run in the guest lsblk command to check current size of vhost-user
driven disk.

Then in nc terminal:
{'execute': 'qmp_capabilities'}
{'execute': 'block_resize', 'arguments': {'node-name': 'f1', 'size': 628531200}}

Then, check in the guest that lsblk shows new size.


 subprojects/libvhost-user/libvhost-user.h |  2 ++
 block/export/vhost-user-blk-server.c  | 24 +++
 subprojects/libvhost-user/libvhost-user.c | 10 ++
 3 files changed, 36 insertions(+)

diff --git a/subprojects/libvhost-user/libvhost-user.h 
b/subprojects/libvhost-user/libvhost-user.h
index 8c5a2719e3..49208cceaa 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -585,6 +585,8 @@ bool vu_queue_empty(VuDev *dev, VuVirtq *vq);
  */
 void vu_queue_notify(VuDev *dev, VuVirtq *vq);
 
+void vu_config_change_msg(VuDev *dev);
+
 /**
  * vu_queue_notify_sync:
  * @dev: a VuDev context
diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 3409d9e02e..e56b92f2e2 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -10,6 +10,7 @@
  * later.  See the COPYING file in the top-level directory.
  */
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "block/block.h"
 #include "subprojects/libvhost-user/libvhost-user.h" /* only for the type 
definitions */
 #include "standard-headers/linux/virtio_blk.h"
@@ -251,6 +252,27 @@ static void vu_blk_exp_request_shutdown(BlockExport *exp)
 vhost_user_server_stop(>vu_server);
 }
 
+static void vu_blk_exp_resize(void *opaque)
+{
+VuBlkExport *vexp = opaque;
+BlockDriverState *bs = blk_bs(vexp->handler.blk);
+int64_t new_size = bdrv_getlength(bs);
+
+if (new_size < 0) {
+error_printf("Failed to get length of block node '%s'",
+ bdrv_get_node_name(bs));
+return;
+}
+
+vexp->blkcfg.capacity = cpu_to_le64(new_size >> VIRTIO_BLK_SECTOR_BITS);
+
+vu_config_change_msg(>vu_server.vu_dev);
+}
+
+static const BlockDevOps vu_blk_dev_ops = {
+.resize_cb = vu_blk_exp_resize,
+};
+
 static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
  Error **errp)
 {
@@ -292,6 +314,8 @@ static int vu_blk_exp_create(BlockExport *exp, 
BlockExportOptions *opts,
 blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
  vexp);
 
+blk_set_dev_ops(exp->blk, _blk_dev_ops, vexp);
+
 if (!vhost_user_server_start(>vu_server, vu_opts->addr, exp->ctx,
  num_queues, _blk_iface, errp)) {
 blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index 0200b78e8e..0abd898a52 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -2455,6 +2455,16 @@ void vu_queue_notify_sync(VuDev *dev, VuVirtq *vq)
 _vu_queue_notify(dev, vq, true);
 }
 
+void vu_config_change_msg(VuDev *dev)
+{
+VhostUserMsg vmsg = {
+.request = VHOST_USER_BACKEND_CONFIG_CHANGE_MSG,
+.flags = VHOST_USER_VERSION,
+};
+
+vu_message_write(dev, dev->slave_fd, );
+}
+
 static inline void
 vring_used_flags_set_bit(VuVirtq *vq, int mask)
 {
-- 
2.34.1

Re: [PATCH 1/2] hw/cxl: Fix endian handling for decoder commit.

2023-03-21 Thread Fan Ni

On Tue, Mar 21, 2023 at 06:00:11PM +, Jonathan Cameron wrote:
> Not a real problem yet as all supported architectures are
> little endian, but continue to tidy these up when touching
> code for other reasons.
> 
> Signed-off-by: Jonathan Cameron 

Hi Jonathan,
Did you forget to send the other patch in this series by any chance?

Fan
> ---
>  hw/cxl/cxl-component-utils.c | 10 --
>  hw/mem/cxl_type3.c   |  9 ++---
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index b665d4f565..a3e6cf75cf 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -47,14 +47,12 @@ static void dumb_hdm_handler(CXLComponentState 
> *cxl_cstate, hwaddr offset,
>  break;
>  }
>  
> -memory_region_transaction_begin();
> -stl_le_p((uint8_t *)cache_mem + offset, value);
>  if (should_commit) {
> -ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> -ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
> -ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> +value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> +value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
>  }
> -memory_region_transaction_commit();
> +stl_le_p((uint8_t *)cache_mem + offset, value);
>  }
>  
>  static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t 
> value,
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index abe60b362c..846089ccda 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -314,14 +314,17 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int 
> which)
>  {
>  ComponentRegisters *cregs = >cxl_cstate.crb;
>  uint32_t *cache_mem = cregs->cache_mem_registers;
> +uint32_t ctrl;
>  
>  assert(which == 0);
>  
> +ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
>  /* TODO: Sanity checks that the decoder is possible */
> -ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> -ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> +ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
>  
> -ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> +stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
>  }
>  
>  static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
> -- 
> 2.37.2
>

RE: [PATCH] hw/acpi/cxl: Drop device-memory support from CFMWS entries

2023-03-21 Thread Dan Williams

Dan Williams wrote:
> While it was a reasonable idea to specify no window restricitions at the
> outset of the CXL emulation support, it turns out that in practice a
> platform will never follow the QEMU example of specifying simultaneous
> support for HDM-H and HDM-D[B] in a single window.
> 
> HDM-D mandates extra bus cycles for host/device bias protocol, and HDM-DB
> mandates extra bus cycles for back-invalidate protocol, so hardware must
> be explicitly prepared for device-memory unlike host-only memory
> (HDM-H).
> 
> In preparation for the kernel dropping support for windows that do not
> select between device and host-only memory, move QEMU exclusively to
> declaring host-only windows.

After an offline discussion determined that a sufficiently sophisticated
platform might be able to support mixed HDM-H and HDM-D[B] in the same
window, so the kernel is not going to drop this support.

Re: [PATCH v3 2/3] qapi: Do not generate empty enum

2023-03-21 Thread Markus Armbruster

Philippe Mathieu-Daudé  writes:

> Can we meet half-way only generating the MAX definitions for
> unconditional enums, keeping the conditional ones as is?
>
> -- >8 --
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> @@ -88,16 +88,18 @@ def gen_enum(name: str,
>   members: List[QAPISchemaEnumMember],
>   prefix: Optional[str] = None) -> str:
>  assert members
> -# append automatically generated _MAX value
> -enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
> -
>  ret = mcgen('''
>
>  typedef enum %(c_name)s {
>  ''',
>  c_name=c_name(name))
>
> -for memb in enum_members:
> +has_cond = any(memb.ifcond.is_present() for memb in members)
> +if has_cond:
> +# append automatically generated _MAX value
> +members += [QAPISchemaEnumMember('_MAX', None)]
> +
> +for memb in members:
>  ret += memb.ifcond.gen_if()
>  ret += mcgen('''
>  %(c_enum)s,
> @@ -105,6 +107,13 @@ def gen_enum(name: str,
>   c_enum=c_enum_const(name, memb.name, prefix))
>  ret += memb.ifcond.gen_endif()
>
> +if not has_cond:
> +ret += mcgen('''
> +#define %(c_name)s %(c_length)s
> +''',
> + c_name=c_enum_const(name, '_MAX', prefix),
> + c_length=len(members))
> +
>  ret += mcgen('''
>  } %(c_name)s;
>  ''',
> ---

I doubt the benefit "we need a silly case FOO__MAX only sometimes" is
worth the special case.

We could generate something like

#if [last_member's condition]
#define FOO__MAX (FOO_last_member + 1)
#elif [second_to_last_member's condition]
#define FOO__MAX (FOO_second_to_last_member + 1)
...
#else
#define FOO__MAX (FOO_last_unconditional_member + 1)
#endif

but whether that is worth the additional complexity seems doubtful, too.

[PATCH RESEND] hw/i2c: Enable an id for the pca954x devices

2023-03-21 Thread Patrick Venture

This allows the devices to be more readily found and specified.
Without setting the id field, they can only be found by device type
name, which doesn't let you specify the second of the same device type
behind a bus.

Tested: Verified that by default the device was findable with the id
'pca954x[77]', for an instance attached at that address.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
Reviewed-by: Philippe Mathieu-Daud?? 
---
 hw/i2c/i2c_mux_pca954x.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index a9517b612a..4f8c2d6ae1 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -20,6 +20,7 @@
 #include "hw/i2c/i2c_mux_pca954x.h"
 #include "hw/i2c/smbus_slave.h"
 #include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -43,6 +44,8 @@ typedef struct Pca954xState {
 
 bool enabled[PCA9548_CHANNEL_COUNT];
 I2CBus *bus[PCA9548_CHANNEL_COUNT];
+
+char *id;
 } Pca954xState;
 
 /*
@@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass, void 
*data)
 s->nchans = PCA9548_CHANNEL_COUNT;
 }
 
+static void pca954x_realize(DeviceState *dev, Error **errp)
+{
+Pca954xState *s = PCA954X(dev);
+DeviceState *d = DEVICE(s);
+if (s->id) {
+d->id = g_strdup(s->id);
+} else {
+d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
+}
+}
+
 static void pca954x_init(Object *obj)
 {
 Pca954xState *s = PCA954X(obj);
@@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
 }
 }
 
+static Property pca954x_props[] = {
+DEFINE_PROP_STRING("id", Pca954xState, id),
+DEFINE_PROP_END_OF_LIST()
+};
+
 static void pca954x_class_init(ObjectClass *klass, void *data)
 {
 I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
@@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass, void 
*data)
 rc->phases.enter = pca954x_enter_reset;
 
 dc->desc = "Pca954x i2c-mux";
+dc->realize = pca954x_realize;
 
 k->write_data = pca954x_write_data;
 k->receive_byte = pca954x_read_byte;
+
+device_class_set_props(dc, pca954x_props);
 }
 
 static const TypeInfo pca954x_info[] = {
-- 
2.35.1.894.gb6a874cedc-goog

RE: [PATCH 2/2] Add test for storing .new vector

> -Original Message-
> From: Peter Maydell 
> Sent: Dienstag, 21. März 2023 18:20
> To: Marco Liebel (QUIC) 
> Cc: qemu-devel@nongnu.org; Taylor Simpson ;
> Matheus Bernardino (QUIC) 
> Subject: Re: [PATCH 2/2] Add test for storing .new vector
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On Tue, 21 Mar 2023 at 14:13, Marco Liebel 
> wrote:
> >
> > Hexagon toolchain version 16.0.0 fixes a bug where the ecoding of
> > storing a .new vector was incorrect. This resulted in an incorrect
> > valued being stored. The test checks that the correct value is used.
> 
> So is this a compiler/assembler bug? Do we need to have tests
> relating to those in QEMU's test suite ?
> 
> thanks
> -- PMM

The bug was in the assembler. For the instruction that does the store of the
.new vector (vmem(r0+#0) = v3.new) it created the wrong output. So there
should be no need to have more tests, other than the one provided by this
patch.

Marco

Re: [RFC PATCH] tests/avocado: probe for multi-process support before running test



Philippe Mathieu-Daudé  writes:

> On 21/3/23 12:17, Alex Bennée wrote:
>> A recent attempt to let avocado run more tests on the CentOS stream
>> build failed because there was no gating on the multiprocess feature.
>> Like missing accelerators avocado should gracefully skip when the
>> feature is not enabled.
>> In this case we use the existence of the proxy device as a proxy for
>> multi-process support.
>> Signed-off-by: Alex Bennée 
>> Cc: Elena Ufimtseva 
>> Cc: Jagannathan Raman 
>> Cc: John G Johnson 
>> ---
>>   tests/avocado/avocado_qemu/__init__.py | 10 ++
>>   tests/avocado/multiprocess.py  |  1 +
>>   2 files changed, 11 insertions(+)
>
>
>> +"""
>> +Test for the presence of the x-pci-proxy-dev which is required
>> +to support multiprocess.
>> +"""
>> +devhelp = run_cmd([self.qemu_bin,
>> +   '-M', 'none', '-device', 'help'])[0];
>> +if devhelp.find('x-pci-proxy-dev') < 0:
>> +self.cancel('no support for multiprocess device emulation')
>
> FYI a more generic alternative to this method:
> https://lore.kernel.org/qemu-devel/20200129212345.20547-14-phi...@redhat.com/
>
> But yours just works :)

For now I want to keep it simple. We should replace it with yours once
we get a chance. Are you happy for a r-b?


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[RFC PATCH] tests/avocado: re-factor igb test to avoid timeouts

The core of the test was utilising "ethtool -t eth1 offline" to run
through a test sequence. For reasons unknown the test hangs under some
configurations of the build on centos8-stream. Fundamentally running
the old fedora-31 cloud-init is just too much for something that is
directed at testing one device. So we:

  - replace fedora with a custom kernel + buildroot rootfs
  - rename the test from IGB to NetDevEthtool
  - re-factor the common code, add (currently skipped) tests for other
 devices which support ethtool
  - remove the KVM limitation as its fast enough to run in KVM or TCG

Signed-off-by: Alex Bennée 
Cc: Akihiko Odaki 
---
 tests/avocado/igb.py| 38 --
 tests/avocado/netdev-ethtool.py | 93 +
 2 files changed, 93 insertions(+), 38 deletions(-)
 delete mode 100644 tests/avocado/igb.py
 create mode 100644 tests/avocado/netdev-ethtool.py

diff --git a/tests/avocado/igb.py b/tests/avocado/igb.py
deleted file mode 100644
index abf5dfa07f..00
--- a/tests/avocado/igb.py
+++ /dev/null
@@ -1,38 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0-or-later
-# ethtool tests for igb registers, interrupts, etc
-
-from avocado_qemu import LinuxTest
-
-class IGB(LinuxTest):
-"""
-:avocado: tags=accel:kvm
-:avocado: tags=arch:x86_64
-:avocado: tags=distro:fedora
-:avocado: tags=distro_version:31
-:avocado: tags=machine:q35
-"""
-
-timeout = 180
-
-def test(self):
-self.require_accelerator('kvm')
-kernel_url = self.distro.pxeboot_url + 'vmlinuz'
-kernel_hash = '5b6f6876e1b5bda314f93893271da0d5777b1f3c'
-kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
-initrd_url = self.distro.pxeboot_url + 'initrd.img'
-initrd_hash = 'dd0340a1b39bd28f88532babd4581c67649ec5b1'
-initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash)
-
-# Ideally we want to test MSI as well, but it is blocked by a bug
-# fixed with:
-# 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=28e96556baca7056d11d9fb3cdd0aba4483e00d8
-kernel_params = self.distro.default_kernel_params + ' pci=nomsi'
-
-self.vm.add_args('-kernel', kernel_path,
- '-initrd', initrd_path,
- '-append', kernel_params,
- '-accel', 'kvm',
- '-device', 'igb')
-self.launch_and_wait()
-self.ssh_command('dnf -y install ethtool')
-self.ssh_command('ethtool -t eth1 offline')
diff --git a/tests/avocado/netdev-ethtool.py b/tests/avocado/netdev-ethtool.py
new file mode 100644
index 00..da0a22d51c
--- /dev/null
+++ b/tests/avocado/netdev-ethtool.py
@@ -0,0 +1,93 @@
+# ethtool tests for emulated network devices
+#
+# This test leverages ethtool's --test sequence to validate network
+# device behaviour.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import time
+
+from avocado import skip
+
+from avocado_qemu import QemuSystemTest
+from avocado_qemu import exec_command, exec_command_and_wait_for_pattern
+from avocado_qemu import wait_for_console_pattern
+
+class NetDevEthtool(QemuSystemTest):
+"""
+:avocado: tags=arch:x86_64
+:avocado: tags=machine:q35
+"""
+
+KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 root=/dev/sda console=ttyS0 '
+# Runs in about 20s under KVM, 26s under TCG, 37s under GCOV
+timeout = 45
+
+def common_test_code(self, netdev, extra_args=None):
+base_url = ('https://fileserver.linaro.org/s/'
+'kE4nCFLdQcoBF9t/download?'
+'path=%2Figb-net-test=' )
+
+# This custom kernel has drivers for all the supported network
+# devices we can emulate in QEMU
+kernel_url = base_url + 'bzImage'
+kernel_hash = '784daede6dab993597f36efbf01f69f184c55152'
+kernel_path = self.fetch_asset(name="bzImage",
+   locations=(kernel_url), 
asset_hash=kernel_hash)
+
+rootfs_url = base_url + 'rootfs.ext4'
+rootfs_hash = '7d28c1bf429de3b441a63756a51f163442ea574b'
+rootfs_path = self.fetch_asset(name="rootfs.ext4",
+   locations=(rootfs_url),
+   asset_hash=rootfs_hash)
+
+kernel_params = self.KERNEL_COMMON_COMMAND_LINE
+if extra_args:
+kernel_params += extra_args
+
+self.vm.add_args('-kernel', kernel_path,
+ '-append', kernel_params,
+ '-blockdev',
+ 
f"driver=raw,file.driver=file,file.filename={rootfs_path},node-name=hd0",
+ '-device', 'driver=ide-hd,bus=ide.0,unit=0,drive=hd0',
+ '-device', netdev)
+
+self.vm.set_console(console_index=0)
+self.vm.launch()
+
+wait_for_console_pattern(self, "Welcome to

[PATCH 1/2] hw/cxl: Fix endian handling for decoder commit.

2023-03-21 Thread Jonathan Cameron via

Not a real problem yet as all supported architectures are
little endian, but continue to tidy these up when touching
code for other reasons.

Signed-off-by: Jonathan Cameron 
---
 hw/cxl/cxl-component-utils.c | 10 --
 hw/mem/cxl_type3.c   |  9 ++---
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index b665d4f565..a3e6cf75cf 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -47,14 +47,12 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, 
hwaddr offset,
 break;
 }
 
-memory_region_transaction_begin();
-stl_le_p((uint8_t *)cache_mem + offset, value);
 if (should_commit) {
-ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
-ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
-ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
+value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
+value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
+value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
 }
-memory_region_transaction_commit();
+stl_le_p((uint8_t *)cache_mem + offset, value);
 }
 
 static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t 
value,
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index abe60b362c..846089ccda 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -314,14 +314,17 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int 
which)
 {
 ComponentRegisters *cregs = >cxl_cstate.crb;
 uint32_t *cache_mem = cregs->cache_mem_registers;
+uint32_t ctrl;
 
 assert(which == 0);
 
+ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
 /* TODO: Sanity checks that the decoder is possible */
-ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
-ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
+ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
+ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
+ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
 
-ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
+stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
 }
 
 static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
-- 
2.37.2

Re: [PATCH v2 0/2] fix for #285


On 3/19/23 07:15, Emilio Cota wrote:

Ping. Any feedback on these two patches?

https://patchew.org/QEMU/20230205163758.416992-1-c...@braap.org/
https://lore.kernel.org/qemu-devel/20230205163758.416992-1-c...@braap.org/


Queued to tcg-next.


r~



Happy to resend if needed.

Thanks,
Emilio


On Fri, Feb 17, 2023 at 07:44:38 -0500, Emilio Cota wrote:

Ping.

This fixes a bug (admittedly with a big hammer) that affects
users with heavily multi-threaded user-mode workloads.

Thanks,
Emilio

On Sun, Feb 05, 2023 at 11:37:56 -0500, Emilio Cota wrote:

Changes since v1:

- Add configure check to only use QTree if Glib still implements gslice.
   If Glib doesn't, then we call Glib directly with inline functions.
- Add TODO's so that in the future (i.e. when the minimum version of
   Glib that we use doesn't implement gslice) we remove QTree.
- Add comment to the top of qtree.h.
- Make qtree-bench results more robust by running longer or more times.
- Drop deprecated API calls (they're unused in QEMU).
- Drop API calls that are too recent (they're unused in QEMU).
- Drop macro benchmark results from the TCG patch since they're too noisy.
- Add test program to the commit log so that we don't lose it in the future
   even if the bug tracker goes away.

Thanks,
Emilio

Re: [Virtio-fs] [RFC 2/2] vhost-user-fs: Implement stateful migration

2023-03-21 Thread Hanna Czenczek


On 20.03.23 13:39, Anton Kuchin wrote:

On 20/03/2023 11:33, Hanna Czenczek wrote:

On 17.03.23 19:37, Anton Kuchin wrote:

On 17/03/2023 19:52, Hanna Czenczek wrote:

On 17.03.23 18:19, Anton Kuchin wrote:

On 13/03/2023 19:48, Hanna Czenczek wrote:

A virtio-fs device's VM state consists of:
- the virtio device (vring) state (VMSTATE_VIRTIO_DEVICE)
- the back-end's (virtiofsd's) internal state

We get/set the latter via the new vhost-user operations 
FS_SET_STATE_FD,

FS_GET_STATE, and FS_SET_STATE.



[...]


  static const VMStateDescription vuf_vmstate = {
  .name = "vhost-user-fs",
-    .unmigratable = 1,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+    VMSTATE_VIRTIO_DEVICE,
+    {
+    .name = "back-end",
+    .info = &(const VMStateInfo) {
+    .name = "virtio-fs back-end state",
+    .get = vuf_load_state,
+    .put = vuf_save_state,
+    },
+    },


I've been working on stateless migration patch [1] and there was 
discussed that we
need to keep some kind of blocker by default if orchestrators rely 
on unmigratable

field in virtio-fs vmstate to block the migration.
For this purpose I've implemented flag that selects "none" or 
"external" and is checked

in pre_save, so it could be extended with "internal" option.
We didn't come to conclusion if we also need to check incoming 
migration, the discussion

has stopped for a while but I'm going back to it now.

I would appreciate if you have time to take a look at the 
discussion and consider the idea
proposed there to store internal state as a subsection of vmstate 
to make it as an option

but not mandatory.

[1] 
https://patchew.org/QEMU/20230217170038.1273710-1-antonkuc...@yandex-team.ru/


So far I’ve mostly considered these issues orthogonal.  If your 
stateless migration goes in first, then state is optional and I’ll 
adjust this series.
If stateful migration goes in first, then your series can simply 
make state optional by introducing the external option, no?


Not really. State can be easily extended by subsections but not 
trimmed. Maybe this can be worked around by defining two types of 
vmstate and selecting the correct one at migration, but I'm not sure.


I thought your patch included a switch on the vhost-user-fs device 
(on the qemu side) to tell qemu what migration data to expect. Can we 
not transfer a 0-length state for 'external', and assert this on the 
destination side?


Looks like I really need to make the description of my patch and the 
documentation more clear :) My patch proposes switch on _source_ side 
to select which data to save in the stream mostly to protect old 
orchestrators that don't expect virtio-fs to be migratable (and for 
internal case it can be extended to select if qemu needs to request 
state from backend), Michael insists that we also need to check on 
destination but I disagree because I believe that we can figure this 
out from stream data without additional device flags.






But maybe we could also consider making stateless migration a 
special case of stateful migration; if we had stateful migration, 
can’t we just implement stateless migration by telling virtiofsd 
that it should submit a special “I have no state” pseudo-state, 
i.e. by having a switch on virtiofsd instead?


Sure. Backend can send empty state (as your patch treats 0 length as 
a valid response and not error) or dummy state that can be 
recognized as stateless. The only potential problem is that then we 
need support in backend for new commands even to return dummy state, 
and if backend can support both types then we'll need some switch in 
backend to reply with real or empty state.


Yes, exactly.



Off the top of my head, some downsides of that approach would be
(1) it’d need a switch on the virtiofsd side, not on the qemu side 
(not sure if that’s a downside, but a difference for sure),


Why would you? It seems to me that this affects only how qemu treats 
the vmstate of device. If the state was requested backend sends it 
to qemu. If state subsection is present in stream qemu sends it to 
the backend for loading. Stateless one just doesn't request state 
from the backend. Or am I missing something?


and (2) we’d need at least some support for this on the virtiofsd 
side, i.e. practically can’t come quicker than stateful migration 
support.


Not much, essentially this is just a reconnect. I've sent a draft of 
a reconnect patch for old C-virtiofsd, for rust version it takes 
much longer because I'm learning rust and I'm not really good at it 
yet.


I meant these two downsides not for your proposal, but instead if we 
implemented stateless migration only in the back-end; i.e. the 
front-end would only implement stateful migration, and the back-end 
would send and accept an empty state.


Then, qemu would always request a “state” (even if it turns out empty 
for stateless migration), and qemu would always treat it the same, so 
there

Re: About the instance_finalize callback in VFIO PCI


On 3/20/23 10:31, Yang Zhong wrote:

Hello Alex and Paolo,

There is one instance_finalize callback definition in hw/vfio/pci.c, but
i find this callback(vfio_instance_finalize()) never be called during the
VM shutdown with close VM or "init 0" command in guest.

The Qemu related command:
..
-device vfio-pci,host=d9:00.0
..


well, the finalize op is correctly called for hot unplugged devices, using
device_add.


static const TypeInfo vfio_pci_dev_info = {
 .name = TYPE_VFIO_PCI,
 .parent = TYPE_PCI_DEVICE,
 .instance_size = sizeof(VFIOPCIDevice),
 .class_init = vfio_pci_dev_class_init,
 .instance_init = vfio_instance_init,
 .instance_finalize = vfio_instance_finalize,
 .interfaces = (InterfaceInfo[]) {
 { INTERFACE_PCIE_DEVICE },
 { INTERFACE_CONVENTIONAL_PCI_DEVICE },
 { }
 },
};

If my test method is wrong, would you please tell me how to trigger to
this callback when VM shutdown? thanks


I would have thought that user_creatable_cleanup would have taken care
of it. But it's not. This needs some digging.

C.

 

By the way, i also debugged other instance_finalize callback functions,
if my understanding is right, all instance_finalize callback should be
called from object_unref(object) from qemu_cleanup(void) in
./softmmu/runstate.c. But there is no VFIO related object_unref() call in
this cleanup function, So the instance_finalize callback in vfio pci
should be useless? thanks!

Regards,
Yang

Re: [PATCH 2/2] Add test for storing .new vector

On Tue, 21 Mar 2023 at 14:13, Marco Liebel  wrote:
>
> Hexagon toolchain version 16.0.0 fixes a bug where the ecoding of
> storing a .new vector was incorrect. This resulted in an incorrect
> valued being stored. The test checks that the correct value is used.

So is this a compiler/assembler bug? Do we need to have tests
relating to those in QEMU's test suite ?

thanks
-- PMM

Re: [PULL 0/8] target-arm queue

On Tue, 21 Mar 2023 at 13:20, Peter Maydell  wrote:
>
> The following changes since commit aa9e7fa4689d1becb2faf67f65aafcbcf664f1ce:
>
>   Merge tag 'edk2-stable202302-20230320-pull-request' of 
> https://gitlab.com/kraxel/qemu into staging (2023-03-20 13:43:35 +)
>
> are available in the Git repository at:
>
>   https://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20230321
>
> for you to fetch changes up to 5787d17a42f7af4bd117e5d6bfa54b1fdf93c255:
>
>   target/arm: Don't advertise aarch64-pauth.xml to gdb (2023-03-21 13:19:08 
> +)
>
> 
> target-arm queue:
>  * contrib/elf2dmp: Support Windows Server 2022
>  * hw/char/cadence_uart: Fix guards on invalid BRGR/BDIV settings
>  * target/arm: Add Neoverse-N1 IMPDEF registers
>  * hw/usb/imx: Fix out of bounds access in imx_usbphy_read()
>  * docs/system/arm/cpu-features.rst: Fix formatting
>  * target/arm: Don't advertise aarch64-pauth.xml to gdb
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.0
for any user-visible changes.

-- PMM

RE: [PATCH 2/2] Add test for storing .new vector

2023-03-21 Thread Taylor Simpson




> -Original Message-
> From: Marco Liebel (QUIC) 
> Sent: Tuesday, March 21, 2023 8:12 AM
> To: qemu-devel@nongnu.org
> Cc: Taylor Simpson ; Matheus Bernardino (QUIC)
> ; Marco Liebel (QUIC)
> 
> Subject: [PATCH 2/2] Add test for storing .new vector
> 
> Hexagon toolchain version 16.0.0 fixes a bug where the ecoding of storing a
> .new vector was incorrect. This resulted in an incorrect valued being stored.
> The test checks that the correct value is used.
> 
> Signed-off-by: Marco Liebel 
> ---
>  tests/tcg/hexagon/hvx_misc.c | 29 +
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
> index 53d5c9b44f..657e556dd4 100644
> --- a/tests/tcg/hexagon/hvx_misc.c
> +++ b/tests/tcg/hexagon/hvx_misc.c
> @@ -211,6 +211,34 @@ static void test_store_unaligned(void)
>  check_output_w(__LINE__, 2);
>  }
> 
> +static void test_store_new(void)
> +{
> +asm volatile(
> +"r0 = #0x0003\n\t"
> +"v0 = vsplat(r0)\n\t"
> +"r0 = #expect\n\t"
> +"vmem(r0+#0) = v0\n\t"

The idiom used in this file is the inline asm stores into the output buffer and 
there is vanilla C that writes to the expect buffer.  So, move the above to 
something like this (after the inline asm).
for (int j = 0; I < MAX_VEC_SIZE_BYTES / 4; j++) {
expect[0].w[j] = 3;
}

Thanks,
Taylor

Re: out of CI pipeline minutes again

2023-03-21 Thread Daniel P . Berrangé

On Mon, Feb 27, 2023 at 12:43:55PM -0500, Stefan Hajnoczi wrote:
> Here are IRC logs from a discussion that has taken place about this
> topic. Summary:
> - QEMU has ~$500/month Azure credits available that could be used for CI
> - Burstable VMs like Azure AKS nodes seem like a good strategy in
> order to minimize hosting costs and parallelize when necessary to keep
> CI duration low
> - Paolo is asking for someone from Red Hat to dedicate the time to set
> up Azure AKS with GitLab CI

3 weeks later... Any progress on getting Red Hat to assign someone to
setup Azure for our CI ?

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/2] tcg: use QTree instead of GTree


On 5/2/23 17:37, Emilio Cota wrote:

qemu-user can hang in a multi-threaded fork. One common
reason is that when creating a TB, between fork and exec
we manipulate a GTree whose memory allocator (GSlice) is
not fork-safe.

Although POSIX does not mandate it, the system's allocator
(e.g. tcmalloc, libc malloc) is probably fork-safe.

Fix some of these hangs by using QTree, which uses the system's
allocator regardless of the Glib version that we used at
configuration time.

Tested with the test program in the original bug report, i.e.:
```

void garble() {
   int pid = fork();
   if (pid == 0) {
 exit(0);
   } else {
 int wstatus;
 waitpid(pid, , 0);
   }
}

void supragarble(unsigned depth) {
   if (depth == 0)
 return ;

   std::thread a(supragarble, depth-1);
   std::thread b(supragarble, depth-1);
   garble();
   a.join();
   b.join();
}

int main() {
   supragarble(10);
}
```

Fixes: #285


Please use: 'Fixes: https://gitlab.com/qemu-project/qemu/-/issues/285'

Also,

Reported-by: Valentin David 



Signed-off-by: Emilio Cota 
---
  accel/tcg/tb-maint.c | 17 +
  tcg/region.c | 19 ++-
  2 files changed, 19 insertions(+), 17 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

[PATCH for-8.0 v2 1/3] async: Suppress GCC13 false positive in aio_bh_poll()

From: Cédric Le Goater 

GCC13 reports an error :

../util/async.c: In function ‘aio_bh_poll’:
include/qemu/queue.h:303:22: error: storing the address of local variable 
‘slice’ in ‘*ctx.bh_slice_list.sqh_last’ [-Werror=dangling-pointer=]
  303 | (head)->sqh_last = &(elm)->field.sqe_next;  
\
  | ~^~~~
../util/async.c:169:5: note: in expansion of macro ‘QSIMPLEQ_INSERT_TAIL’
  169 | QSIMPLEQ_INSERT_TAIL(>bh_slice_list, , next);
  | ^~~~
../util/async.c:161:17: note: ‘slice’ declared here
  161 | BHListSlice slice;
  | ^
../util/async.c:161:17: note: ‘ctx’ declared here

But the local variable 'slice' is removed from the global context list
in following loop of the same routine. Add a pragma to silent GCC.

Cc: Stefan Hajnoczi 
Cc: Paolo Bonzini 
Cc: Daniel P. Berrangé 
Signed-off-by: Cédric Le Goater 
---
 util/async.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/util/async.c b/util/async.c
index 21016a1ac7..de9b431236 100644
--- a/util/async.c
+++ b/util/async.c
@@ -164,7 +164,20 @@ int aio_bh_poll(AioContext *ctx)
 
 /* Synchronizes with QSLIST_INSERT_HEAD_ATOMIC in aio_bh_enqueue().  */
 QSLIST_MOVE_ATOMIC(_list, >bh_list);
+
+/*
+ * GCC13 [-Werror=dangling-pointer=] complains that the local variable
+ * 'slice' is being stored in the global 'ctx->bh_slice_list' but the
+ * list is emptied before this function returns.
+ */
+#if !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer="
+#endif
 QSIMPLEQ_INSERT_TAIL(>bh_slice_list, , next);
+#if !defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
 
 while ((s = QSIMPLEQ_FIRST(>bh_slice_list))) {
 QEMUBH *bh;
-- 
2.39.2

[PATCH for-8.0 v2 3/3] target/ppc: Fix helper_pminsn() prototype

From: Cédric Le Goater 

GCC13 reports an error:

../target/ppc/excp_helper.c:2625:6: error: conflicting types for 
‘helper_pminsn’ due to enum/integer mismatch; have ‘void(CPUPPCState *, 
powerpc_pm_insn_t)’ {aka ‘void(struct CPUArchState *, powerpc_pm_insn_t)’} 
[-Werror=enum-int-mismatch]
 2625 | void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
  |  ^
In file included from /home/legoater/work/qemu/qemu.git/include/qemu/osdep.h:49,
 from ../target/ppc/excp_helper.c:19:
/home/legoater/work/qemu/qemu.git/include/exec/helper-head.h:23:27: note: 
previous declaration of ‘helper_pminsn’ with type ‘void(CPUArchState *, 
uint32_t)’ {aka ‘void(CPUArchState *, unsigned int)’}
   23 | #define HELPER(name) glue(helper_, name)
  |   ^~~

Cc: Daniel Henrique Barboza 
Fixes: 7778a575c7 ("ppc: Add P7/P8 Power Management instructions")
Signed-off-by: Cédric Le Goater 
Reviewed-by: Daniel Henrique Barboza 
---
 target/ppc/excp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 287659c74d..199328f4b6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2622,7 +2622,7 @@ void helper_scv(CPUPPCState *env, uint32_t lev)
 }
 }
 
-void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
+void helper_pminsn(CPUPPCState *env, uint32_t insn)
 {
 CPUState *cs;
 
-- 
2.39.2

[PATCH for-8.0 v2 2/3] target/s390x: Fix float_comp_to_cc() prototype

From: Cédric Le Goater 

GCC13 reports an error :

../target/s390x/tcg/fpu_helper.c:123:5: error: conflicting types for 
‘float_comp_to_cc’ due to enum/integer mismatch; have ‘int(CPUS390XState *, 
FloatRelation)’ {aka ‘int(struct CPUArchState *, FloatRelation)’} 
[-Werror=enum-int-mismatch]

  123 | int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare)
  | ^~~~
In file included from ../target/s390x/tcg/fpu_helper.c:23:
../target/s390x/s390x-internal.h:302:5: note: previous declaration of 
‘float_comp_to_cc’ with type ‘int(CPUS390XState *, int)’ {aka ‘int(struct 
CPUArchState *, int)’}
  302 | int float_comp_to_cc(CPUS390XState *env, int float_compare);
  | ^~~~

Cc: Thomas Huth 
Cc: Richard Henderson 
Cc: David Hildenbrand 
Cc: Ilya Leoshkevich 
Fixes: 71bfd65c5f ("softfloat: Name compare relation enum")
Signed-off-by: Cédric Le Goater 
Reviewed-by: Philippe Mathieu-Daudé 
---
 target/s390x/s390x-internal.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 5d4361d35b..825252d728 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -11,6 +11,7 @@
 #define S390X_INTERNAL_H
 
 #include "cpu.h"
+#include "fpu/softfloat.h"
 
 #ifndef CONFIG_USER_ONLY
 typedef struct LowCore {
@@ -299,7 +300,7 @@ uint32_t set_cc_nz_f128(float128 v);
 uint8_t s390_softfloat_exc_to_ieee(unsigned int exc);
 int s390_swap_bfp_rounding_mode(CPUS390XState *env, int m3);
 void s390_restore_bfp_rounding_mode(CPUS390XState *env, int old_mode);
-int float_comp_to_cc(CPUS390XState *env, int float_compare);
+int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare);
 
 #define DCMASK_ZERO 0x0c00
 #define DCMASK_NORMAL   0x0300
-- 
2.39.2

[PATCH for-8.0 v2 0/3] Fixes for GCC13

Hello,

I activated a GH workflow using fedora rawhide and found out that
there were a couple of compile breakage with the new GCC. Here are
fixes, the first requiring more attention.

Thanks,

C. 

Changes in v2:

 - replaced helper routine by pragmas in util/async.c to suppress GCC
   warning

Cédric Le Goater (3):
  async: Suppress GCC13 false positive in aio_bh_poll()
  target/s390x: Fix float_comp_to_cc() prototype
  target/ppc: Fix helper_pminsn() prototype

 target/s390x/s390x-internal.h |  3 ++-
 target/ppc/excp_helper.c  |  2 +-
 util/async.c  | 13 +
 3 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.39.2

Re: [PATCH] linux-user/mips: Low down switchable NaN2008 requirement


On 21/3/23 10:23, Jiaxun Yang wrote:

2023年3月15日 08:18，Philippe Mathieu-Daudé  写道：
On 11/3/23 13:39, Jiaxun Yang wrote:

2023年3月9日 12:32，Philippe Mathieu-Daudé  写道：
On 11/2/23 18:34, Jiaxun Yang wrote:

Previously switchable NaN2008 requires fcsr31.nan2008 to be writable
for guest. However as per MIPS arch spec this bit can never be writable.
This cause NaN2008 ELF to be rejected by QEMU.
NaN2008 can be enabled on R2~R5 processors, just make it available
unconditionally.
Signed-off-by: Jiaxun Yang 
---
  linux-user/mips/cpu_loop.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c
index d5c1c7941d..b5c2ca4a3e 100644
--- a/linux-user/mips/cpu_loop.c
+++ b/linux-user/mips/cpu_loop.c
@@ -301,8 +301,7 @@ void target_cpu_copy_regs(CPUArchState *env, struct 
target_pt_regs *regs)
  }
  if (((info->elf_flags & EF_MIPS_NAN2008) != 0) !=
  ((env->active_fpu.fcr31 & (1 << FCR31_NAN2008)) != 0)) {
-if ((env->active_fpu.fcr31_rw_bitmask &
-  (1 << FCR31_NAN2008)) == 0) {
+if (!(env->insn_flags & ISA_MIPS_R2)) {
  fprintf(stderr, "ELF binary's NaN mode not supported by CPU\n");
  exit(1);
  }


Looking at R6.06 revision history:

  5.03 August 21, 2013

  • ABS2008 and NAN2008 fields of Table 5.7 “FCSR RegisterField
Descriptions” were optional in release 3 and could be R/W,
but as of release 5 are required, read-only, and preset by
hardware.



Well that’s because there is no CPU being marked as MIPS Release 3 in QEMU, and 
only
P5600 is marked as MIPS Release 5.
In reality R3 implementations are all advertising themself as R2, and later RCs 
of microAptiv
and interaptiv can all be configured as NaN2008 only. So for those CPUs we have 
binary compiled
with -march=mips32r2 -mnan=2008.
Given that default CPU of mips32r2 in QEMU is 24Kf, I think the best approach 
to deal with such
situation is to allow NaN2008 to be enabled for early processors for linux-user.
There is a NAN2008 Debian port for test:
http://repo.oss.cipunited.com/mipsel-nan2008/tarball/sid-mipsel-nan2008-20230309-1.tar.xz


$ qemu-mipsel -L sid-mipsel-nan2008-20230313-1/usr -cpu P5600 usr/bin/uname  -ms
Linux mips

What about something like:


That would lost capability of testing NaN2008 binaries again other CPU models.


Why? cpu_get_model() is just a hint, see linux-user/main.c::main():

if (cpu_model == NULL) {
cpu_model = cpu_get_model(get_elf_eflags(execfd));
}


-- >8 --
--- a/linux-user/mips/target_elf.h
+++ b/linux-user/mips/target_elf.h
@@ -15,6 +15,9 @@ static inline const char *cpu_get_model(uint32_t eflags)
 if ((eflags & EF_MIPS_MACH) == EF_MIPS_MACH_5900) {
 return "R5900";
 }
+if (eflags & EF_MIPS_NAN2008) {
+return "P5600";
+}
 return "24Kf";
}
#endif
---

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change


On 3/20/23 23:37, fei2...@intel.com wrote:

From: Fei Wu 

Kernel needs to access user mode memory e.g. during syscalls, the window
is usually opened up for a very limited time through MSTATUS.SUM, the
overhead is too much if tlb_flush() gets called for every SUM change.
This patch saves addresses accessed when SUM=1, and flushs only these
pages when SUM changes to 0. If the buffer is not large enough to save
all the pages during SUM=1, it will fall back to tlb_flush when
necessary.

The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
most of the time kernel accesses 1 or 2 pages, it's very rare to see
more than 4 pages accessed.

It's not necessary to save/restore these new added status, as
tlb_flush() is always called after restore.

Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
other syscalls benefit a lot from this one too.


This is not the correct approach.

You should be making use of different softmmu indexes, similar to how ARM uses a separate 
index for PAN (privileged access never) mode.  If I read the manual properly, PAN == !SUM.


When you do this, you need no additional flushing.


r~

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

2023-03-21 Thread Alessandro Di Federico via

On Mon, 20 Mar 2023 10:10:27 +
Alex Bennée  wrote:

> This doesn't save much as cpu-exec-common still needs to be built
> per-target for its knowledge of CPUState but this helps with keeping
> things organised.

> --- /dev/null
> +++ b/accel/tcg/cpu-exec-softmmu.c

Could `cpu_reloading_memory_map` be pushed closer to its only user
(softmmu/physmem.c) instead of creating a new file in accel/tcg?

Maybe I'm missing something, but I see other usages of current_cpu in
softmmu:

$ git grep 'current_cpu->' softmmu/|cat
softmmu/cpus.c:current_cpu->stop = true;
softmmu/memory.c:return current_cpu->cpu_index;
softmmu/runstate.c:current_cpu->crash_occurred = true;

Maybe you envision more stuff in cpu-exec-softmmu.c.

Reviewed-by: Alessandro Di Federico 

-- 
Alessandro Di Federico
rev.ng Labs

Re: [RFC PATCH 40/43] target/loongarch: Implement vreplve vpack vpick


On 3/21/23 04:31, gaosong wrote:

but for this case.
e.g
vreplve_b  vd vj, rk
index  = gpr[rk] % (128/8);
Vd->B(i) = Vj->B(index);
tcg_gen_gvec_dup_mem(MO_8, vreg_full_offset(a->vd), offsetof(CPULoongArchState, 
fpr[a->vj].vreg.B(index))), 16, 16 );


How can we get the index with cpu_env? or  need env->gpr[rk]?
The index type is not TCGv.


For this case you would load the value Vj->B(index) into a TCGv_i32,

tcg_gen_andi_i64(t0, gpr_src(rk), 15);

// Handle endian adjustment on t0, e.g. xor 15 for big-endian?

tcg_gen_trunc_i64_ptr(t1, t0);
tcg_gen_add_ptr(t1, t1, cpu_env);
tcg_gen_ld8u_i32(t2, t1, vreg_full_offset(vj));

// At this point t2 contains Vj->B(index)

tcg_gen_gvec_dup_i32(MO_8, vreg_full_offset(vd), 16, 16, t2);



r~

Re: [PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

On 3/20/23 23:44, LIU Zhiwei wrote:

On 2023/3/21 14:06, Richard Henderson wrote:

On 3/20/23 21:53, LIU Zhiwei wrote:

TS_DEAD means we will release the register allocated for this temporary. But
at basic block ending, we can still use the allocted register.

Signed-off-by: LIU Zhiwei

Test case?

I have run an Ubuntu image after this patch. It can boot.

That's surprising. I would have expected an assert with --enable-debug-tcg, but this
appears to be an oversight in tcg_reg_alloc_bb_end. We only validate the liveness data
for TEMP_EBB and TEMP_CONST, but not TEMP_TB or TEMP_GLOBAL.

But I can't find a direct test case. Because the IRs supported with flags TCG_OPF_BB_END
do not have input or output parameter, such as the set_label or br.

That's exactly why we want all GLOBAL and TB to be DEAD | MEM, so that they're saved back
to their home slots and released from their registers.

The register allocator for TCG does not work across extended basic blocks. Importantly,
if you have a forward branch like so:

g1 = func(a)
brcond ..., L1

stuff
g2 = func(b)
g1 = g2
discard g2

L1:

What value should g1->reg have at L1? The allocator does not do the global control flow
and allocation required to ensure that g1->reg is the same at the brcond and at the label.

Nominally, I would have expected one value for g1->reg at the branch, a different value
for g2->reg in the second BB, and for the assignment to steal g2->reg and move it to
g1->reg (per tcg_reg_alloc_mov of an IS_DEAD_ARG temp). Which would result in an
incorrect allocation at L1.

What are you attempting to do? Is this just guesswork?

Re: [kvm-unit-tests PATCH v10 0/7] MTTCG sanity tests for ARM

On Tue, Mar 07, 2023 at 11:28:38AM +, Alex Bennée wrote:
> I last had a go at getting these up-streamed at the end of 2021 so
> its probably worth having another go. From the last iteration a
> number of the groundwork patches did get merged:
> 
>   Subject: [kvm-unit-tests PATCH v9 0/9] MTTCG sanity tests for ARM
>   Date: Thu,  2 Dec 2021 11:53:43 +
>   Message-Id: <20211202115352.951548-1-alex.ben...@linaro.org>
> 
> So this leaves a minor gtags patch, adding the isaac RNG library which
> would also be useful for other users, see:
> 
>   Subject: [kvm-unit-tests PATCH v3 11/27] lib: Add random number generator
>   Date: Tue, 22 Nov 2022 18:11:36 +0200
>   Message-Id: <20221122161152.293072-12-mlevi...@redhat.com>
> 
> Otherwise there are a few minor checkpatch tweaks.
> 
> I would still like to enable KVM unit tests inside QEMU as things like
> the x86 APIC tests are probably a better fit for unit testing TCG
> emulation than booting a whole OS with various APICs enabled.
> 
> Alex Bennée (7):
>   Makefile: add GNU global tags support
>   add .gitpublish metadata
>   lib: add isaac prng library from CCAN
>   arm/tlbflush-code: TLB flush during code execution
>   arm/locking-tests: add comprehensive locking test
>   arm/barrier-litmus-tests: add simple mp and sal litmus tests
>   arm/tcg-test: some basic TCG exercising tests
> 
>  Makefile  |   5 +-
>  arm/Makefile.arm  |   2 +
>  arm/Makefile.arm64|   2 +
>  arm/Makefile.common   |   6 +-
>  lib/arm/asm/barrier.h |  19 ++
>  lib/arm64/asm/barrier.h   |  50 +
>  lib/prng.h|  83 +++
>  lib/prng.c| 163 ++
>  arm/tcg-test-asm.S| 171 +++
>  arm/tcg-test-asm64.S  | 170 ++
>  arm/barrier-litmus-test.c | 450 ++
>  arm/locking-test.c| 321 +++
>  arm/spinlock-test.c   |  87 
>  arm/tcg-test.c| 340 
>  arm/tlbflush-code.c   | 209 ++
>  arm/unittests.cfg | 170 ++
>  .gitignore|   3 +
>  .gitpublish   |  18 ++
>  18 files changed, 2180 insertions(+), 89 deletions(-)
>  create mode 100644 lib/prng.h
>  create mode 100644 lib/prng.c
>  create mode 100644 arm/tcg-test-asm.S
>  create mode 100644 arm/tcg-test-asm64.S
>  create mode 100644 arm/barrier-litmus-test.c
>  create mode 100644 arm/locking-test.c
>  delete mode 100644 arm/spinlock-test.c
>  create mode 100644 arm/tcg-test.c
>  create mode 100644 arm/tlbflush-code.c
>  create mode 100644 .gitpublish
> 
> -- 
> 2.39.2
>

I don't see any problem with the series, but I didn't review it closely.
I think it's unlikely we'll get reviewers, but, as the tests are
nodefault, then that's probably OK. Can you make sure all tests have a
"tcg" type prefix when they are TCG-only, like the last patch does for
its tests? That will help filter them out when building all tests as
standalone tests. Someday mkstandalone could maybe learn how to build
a directory hierarchy using the group names, e.g.

 tests/mttcg/tlb/all_other

but I don't expect to have time for that myself anytime soon, so prefixes
will likely have to do for now (or forever).

Thanks,
drew

Re: [PATCH v3 2/3] qapi: Do not generate empty enum

2023-03-21 Thread Daniel P . Berrangé

On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote:
> On 16/3/23 15:57, Markus Armbruster wrote:
> > Daniel P. Berrangé  writes:
> > 
> > > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
> > > > Philippe Mathieu-Daudé  writes:
> > > > 
> > > > > Per the C++ standard, empty enum are ill-formed. Do not generate
> > 
> > The C standard.  The C++ standard doesn't apply here :)
> 
> I can't find how empty enums are considered by the C standard...

The C standard doesn't really matter either.

What we actually care about is whether GCC and CLang consider the
empty enums to be permissible or not. or to put it another way...
if it compiles, ship it :-)

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [kvm-unit-tests PATCH v10 4/7] arm/tlbflush-code: TLB flush during code execution

On Tue, Mar 21, 2023 at 04:02:21PM +0100, Andrew Jones wrote:
...
> > +
> > +# TLB Torture Tests
> > +[tlbflush-code::all_other]
> 
> It's better to use '-', '_', '.', or ',' than '::' because otherwise the
> standalone test will have a filename like tests/tlbflush-code::all_other
> which will be awkward for shells.
> 
> BTW, have you tried running these tests as standalone? Since they're
> 'nodefault' it'd be good if they work that way.
> 
> > +file = tlbflush-code.flat
> > +smp = $(($MAX_SMP>4?4:$MAX_SMP))
> > +groups = nodefault mttcg
> > +
> > +[tlbflush-code::page_other]
> > +file = tlbflush-code.flat
> > +smp = $(($MAX_SMP>4?4:$MAX_SMP))
> > +extra_params = -append 'page'
> > +groups = nodefault mttcg
> > +
> > +[tlbflush-code::all_self]
> > +file = tlbflush-code.flat
> > +smp = $(($MAX_SMP>4?4:$MAX_SMP))
> > +extra_params = -append 'self'
> > +groups = nodefault mttcg
> > +
> > +[tlbflush-code::page_self]
> > +file = tlbflush-code.flat
> > +smp = $(($MAX_SMP>4?4:$MAX_SMP))
> > +extra_params = -append 'page self'
> > +groups = nodefault mttcg

Shouldn't these also be in something like a "tlb" group?

Thanks,
drew

Re: [kvm-unit-tests PATCH v10 5/7] arm/locking-tests: add comprehensive locking test

On Tue, Mar 07, 2023 at 11:28:43AM +, Alex Bennée wrote:
> This test has been written mainly to stress multi-threaded TCG behaviour
> but will demonstrate failure by default on real hardware. The test takes
> the following parameters:
> 
>   - "lock" use GCC's locking semantics
>   - "atomic" use GCC's __atomic primitives
>   - "wfelock" use WaitForEvent sleep
>   - "excl" use load/store exclusive semantics
> 
> Also two more options allow the test to be tweaked
> 
>   - "noshuffle" disables the memory shuffling
>   - "count=%ld" set your own per-CPU increment count
> 
> Signed-off-by: Alex Bennée 
> Message-Id: <2028184650.661575-8-alex.ben...@linaro.org>
> 
> ---
> v9
>   - move back to unittests.cfg, drop accel=tcg
>   - s/printf/report_info
> v10
>   - dropped spare extra line in shuffle_memory
> ---
>  arm/Makefile.common |   2 +-
>  arm/locking-test.c  | 321 
>  arm/spinlock-test.c |  87 
>  arm/unittests.cfg   |  30 +
>  4 files changed, 352 insertions(+), 88 deletions(-)
>  create mode 100644 arm/locking-test.c
>  delete mode 100644 arm/spinlock-test.c
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 2c4aad38..3089e3bf 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -5,7 +5,6 @@
>  #
>  
>  tests-common  = $(TEST_DIR)/selftest.flat
> -tests-common += $(TEST_DIR)/spinlock-test.flat
>  tests-common += $(TEST_DIR)/pci-test.flat
>  tests-common += $(TEST_DIR)/pmu.flat
>  tests-common += $(TEST_DIR)/gic.flat
> @@ -13,6 +12,7 @@ tests-common += $(TEST_DIR)/psci.flat
>  tests-common += $(TEST_DIR)/sieve.flat
>  tests-common += $(TEST_DIR)/pl031.flat
>  tests-common += $(TEST_DIR)/tlbflush-code.flat
> +tests-common += $(TEST_DIR)/locking-test.flat
>  
>  tests-all = $(tests-common) $(tests)
>  all: directories $(tests-all)
> diff --git a/arm/locking-test.c b/arm/locking-test.c
> new file mode 100644
> index ..a49c2fd1
> --- /dev/null
> +++ b/arm/locking-test.c
> @@ -0,0 +1,321 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Locking Test
> + *
> + * This test allows us to stress the various atomic primitives of a VM
> + * guest. A number of methods are available that use various patterns
> + * to implement a lock.
> + *
> + * Copyright (C) 2017 Linaro
> + * Author: Alex Bennée 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define MAX_CPUS 8
> +
> +/* Test definition structure
> + *
> + * A simple structure that describes the test name, expected pass and
> + * increment function.
> + */

nit: This and many comment blocks below are missing their opening wings.

Thanks,
drew

Re: [kvm-unit-tests PATCH v10 4/7] arm/tlbflush-code: TLB flush during code execution

On Tue, Mar 07, 2023 at 11:28:42AM +, Alex Bennée wrote:
> This adds a fairly brain dead torture test for TLB flushes intended
> for stressing the MTTCG QEMU build. It takes the usual -smp option for
> multiple CPUs.
> 
> By default it CPU0 will do a TLBIALL flush after each cycle. You can
> pass options via -append to control additional aspects of the test:
> 
>   - "page" flush each page in turn (one per function)
>   - "self" do the flush after each computation cycle
>   - "verbose" report progress on each computation cycle
> 
> Signed-off-by: Alex Bennée 
> CC: Mark Rutland 
> Message-Id: <2028184650.661575-7-alex.ben...@linaro.org>
> 
> ---
> v9
>   - move tests back into unittests.cfg (with nodefault mttcg)
>   - replace printf with report_info
>   - drop accel = tcg
> ---
>  arm/Makefile.common |   1 +
>  arm/tlbflush-code.c | 209 
>  arm/unittests.cfg   |  25 ++
>  3 files changed, 235 insertions(+)
>  create mode 100644 arm/tlbflush-code.c
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 16f8c6df..2c4aad38 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -12,6 +12,7 @@ tests-common += $(TEST_DIR)/gic.flat
>  tests-common += $(TEST_DIR)/psci.flat
>  tests-common += $(TEST_DIR)/sieve.flat
>  tests-common += $(TEST_DIR)/pl031.flat
> +tests-common += $(TEST_DIR)/tlbflush-code.flat
>  
>  tests-all = $(tests-common) $(tests)
>  all: directories $(tests-all)
> diff --git a/arm/tlbflush-code.c b/arm/tlbflush-code.c
> new file mode 100644
> index ..bf9eb111
> --- /dev/null
> +++ b/arm/tlbflush-code.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * TLB Flush Race Tests
> + *
> + * These tests are designed to test for incorrect TLB flush semantics
> + * under emulation. The initial CPU will set all the others working a
> + * compuation task and will then trigger TLB flushes across the

computation

> + * system. It doesn't actually need to re-map anything but the flushes
> + * themselves will trigger QEMU's TCG self-modifying code detection
> + * which will invalidate any generated  code causing re-translation.
> + * Eventually the code buffer will fill and a general tb_lush() will
> + * be triggered.
> + *
> + * Copyright (C) 2016-2021, Linaro, Alex Bennée 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SEQ_LENGTH 10
> +#define SEQ_HASH 0x7cd707fe
> +
> +static cpumask_t smp_test_complete;
> +static int flush_count = 100;
> +static bool flush_self;
> +static bool flush_page;
> +static bool flush_verbose;
> +
> +/*
> + * Work functions
> + *
> + * These work functions need to be:
> + *
> + *  - page aligned, so we can flush one function at a time
> + *  - have branches, so QEMU TCG generates multiple basic blocks
> + *  - call across pages, so we exercise the TCG basic block slow path
> + */
> +
> +/* Adler32 */
> +__attribute__((aligned(PAGE_SIZE))) static
> +uint32_t hash_array(const void *buf, size_t buflen)
> +{
> + const uint8_t *data = (uint8_t *) buf;
> + uint32_t s1 = 1;
> + uint32_t s2 = 0;
> +
> + for (size_t n = 0; n < buflen; n++) {
> + s1 = (s1 + data[n]) % 65521;
> + s2 = (s2 + s1) % 65521;
> + }
> + return (s2 << 16) | s1;
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) static
> +void create_fib_sequence(int length, unsigned int *array)
> +{
> + int i;
> +
> + /* first two values */
> + array[0] = 0;
> + array[1] = 1;
> + for (i = 2; i < length; i++)
> + array[i] = array[i-2] + array[i-1];
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) static
> +unsigned long long factorial(unsigned int n)
> +{
> + unsigned int i;
> + unsigned long long fac = 1;
> +
> + for (i = 1; i <= n; i++)
> + fac = fac * i;
> + return fac;
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) static
> +void factorial_array(unsigned int n, unsigned int *input,
> +  unsigned long long *output)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < n; i++)
> + output[i] = factorial(input[i]);
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) static
> +unsigned int do_computation(void)
> +{
> + unsigned int fib_array[SEQ_LENGTH];
> + unsigned long long facfib_array[SEQ_LENGTH];
> + uint32_t fib_hash, facfib_hash;
> +
> + create_fib_sequence(SEQ_LENGTH, _array[0]);
> + fib_hash = hash_array(_array[0], sizeof(fib_array));
> + factorial_array(SEQ_LENGTH, _array[0], _array[0]);
> + facfib_hash = hash_array(_array[0], sizeof(facfib_array));
> +
> + return (fib_hash ^ facfib_hash);
> +}
> +
> +/* This provides a table of the work functions so we can flush each
> + * page individually
> + */
> +static void *pages[] = {_array, _fib_sequence, ,
> + _array, _computation};
> +
> +static void

Re: [PATCH-for-8.1] target/m68k/fpu_helper: Use FloatRelation enum to hold comparison result


On 3/21/23 02:49, Philippe Mathieu-Daudé wrote:

Use the FloatRelation enum to hold the comparison result (missed
in commit 71bfd65c5f "softfloat: Name compare relation enum").

Inspired-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
  target/m68k/fpu_helper.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [kvm-unit-tests PATCH v10 2/7] add .gitpublish metadata

On Tue, Mar 07, 2023 at 11:28:40AM +, Alex Bennée wrote:
> This allows for users of git-publish to have default routing for kvm
> and kvmarm patches.
> 
> Signed-off-by: Alex Bennée 
> ---
>  .gitpublish | 18 ++
>  1 file changed, 18 insertions(+)
>  create mode 100644 .gitpublish
> 
> diff --git a/.gitpublish b/.gitpublish
> new file mode 100644
> index ..39130f93
> --- /dev/null
> +++ b/.gitpublish
> @@ -0,0 +1,18 @@
> +#
> +# Common git-publish profiles that can be used to send patches to QEMU 
> upstream.
> +#
> +# See https://github.com/stefanha/git-publish for more information
> +#
> +[gitpublishprofile "default"]
> +base = master
> +to = k...@vger.kernel.org
> +cc = qemu-devel@nongnu.org
> +cccmd = scripts/get_maintainer.pl --noroles --norolestats --nogit 
> --nogit-fallback 2>/dev/null
> +
> +[gitpublishprofile "arm"]
> +base = master
> +to = kvm...@lists.cs.columbia.edu
> +cc = k...@vger.kernel.org
> +cc = qemu-devel@nongnu.org
> +cc = qemu-...@nongnu.org
> +cccmd = scripts/get_maintainer.pl --noroles --norolestats --nogit 
> --nogit-fallback 2>/dev/null

Should we also set the prefix for these?

 prefix = kvm-unit-tests PATCH

And maybe even, 'signoff = true'?

Otherwise,

Acked-by: Andrew Jones 

Thanks,
drew

Re: [kvm-unit-tests PATCH v10 1/7] Makefile: add GNU global tags support

On Tue, Mar 07, 2023 at 11:28:39AM +, Alex Bennée wrote:
> If you have ctags you might as well offer gtags as a target.
> 
> Signed-off-by: Alex Bennée 
> Message-Id: <2028184650.661575-4-alex.ben...@linaro.org>
> 
> ---
> v10
>   - update .gitignore
> ---
>  Makefile   | 5 -
>  .gitignore | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 6ed5deac..f22179de 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -145,6 +145,9 @@ cscope:
>   -name '*.[chsS]' -exec realpath --relative-base=$(CURDIR) {} \; 
> | sort -u > ./cscope.files
>   cscope -bk
>  
> -.PHONY: tags
> +.PHONY: tags gtags
>  tags:
>   ctags -R
> +
> +gtags:
> + gtags
> diff --git a/.gitignore b/.gitignore
> index 33529b65..4d5f460f 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -12,6 +12,9 @@ tags
>  patches
>  .stgit-*
>  cscope.*
> +GPATH
> +GRTAGS
> +GTAGS
>  *.swp
>  /lib/asm
>  /lib/config.h
> -- 
> 2.39.2
>

Acked-by: Andrew Jones

Re: [PATCH v3 2/3] qapi: Do not generate empty enum


On 16/3/23 15:57, Markus Armbruster wrote:

Daniel P. Berrangé  writes:




But all of this falls apart with conditional members!

Example 1 (taken from qapi/block-core.json):

 { 'enum': 'BlockdevAioOptions',
   'data': [ 'threads', 'native',
 { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' } ] }

Generates now:

 typedef enum BlockdevAioOptions {
 BLOCKDEV_AIO_OPTIONS_THREADS,
 BLOCKDEV_AIO_OPTIONS_NATIVE,
 #if defined(CONFIG_LINUX_IO_URING)
 BLOCKDEV_AIO_OPTIONS_IO_URING,
 #endif /* defined(CONFIG_LINUX_IO_URING) */
 BLOCKDEV_AIO_OPTIONS__MAX,
 } BlockdevAioOptions;

BLOCKDEV_AIO_OPTIONS__MAX is 3 if defined(CONFIG_LINUX_IO_URING), else
2.

After the next patch:

 typedef enum BlockdevAioOptions {
 BLOCKDEV_AIO_OPTIONS_THREADS,
 BLOCKDEV_AIO_OPTIONS_NATIVE,
 #if defined(CONFIG_LINUX_IO_URING)
 BLOCKDEV_AIO_OPTIONS_IO_URING,
 #endif /* defined(CONFIG_LINUX_IO_URING) */
 #define BLOCKDEV_AIO_OPTIONS__MAX 3
 } BlockdevAioOptions;

Now it's always 3.

Example 2 (same with members reordered):

 { 'enum': 'BlockdevAioOptions',
   'data': [ { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' },
 'threads', 'native' ] }

Same problem for __MAX, additional problem for __DUMMY:

 typedef enum BlockdevAioOptions {
 BLOCKDEV_AIO_OPTIONS__DUMMY = 0,
 #if defined(CONFIG_LINUX_IO_URING)
 BLOCKDEV_AIO_OPTIONS_IO_URING = 0,
 #endif /* defined(CONFIG_LINUX_IO_URING) */
 BLOCKDEV_AIO_OPTIONS_THREADS,
 BLOCKDEV_AIO_OPTIONS_NATIVE,
 #define BLOCKDEV_AIO_OPTIONS__MAX 3
 } BlockdevAioOptions;

If CONFIG_LINUX_IO_URING is off, the enum starts at 1 instead of 0.

Arrays indexed by the enum start with a hole.  Code using them is
probably not prepared for holes.


Can we meet half-way only generating the MAX definitions for
unconditional enums, keeping the conditional ones as is?

-- >8 --
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
@@ -88,16 +88,18 @@ def gen_enum(name: str,
  members: List[QAPISchemaEnumMember],
  prefix: Optional[str] = None) -> str:
 assert members
-# append automatically generated _MAX value
-enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
-
 ret = mcgen('''

 typedef enum %(c_name)s {
 ''',
 c_name=c_name(name))

-for memb in enum_members:
+has_cond = any(memb.ifcond.is_present() for memb in members)
+if has_cond:
+# append automatically generated _MAX value
+members += [QAPISchemaEnumMember('_MAX', None)]
+
+for memb in members:
 ret += memb.ifcond.gen_if()
 ret += mcgen('''
 %(c_enum)s,
@@ -105,6 +107,13 @@ def gen_enum(name: str,
  c_enum=c_enum_const(name, memb.name, prefix))
 ret += memb.ifcond.gen_endif()

+if not has_cond:
+ret += mcgen('''
+#define %(c_name)s %(c_length)s
+''',
+ c_name=c_enum_const(name, '_MAX', prefix),
+ c_length=len(members))
+
 ret += mcgen('''
 } %(c_name)s;
 ''',
---

Re: [PATCH v3 2/3] qapi: Do not generate empty enum


On 16/3/23 15:57, Markus Armbruster wrote:

Daniel P. Berrangé  writes:


On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:

Philippe Mathieu-Daudé  writes:


Per the C++ standard, empty enum are ill-formed. Do not generate


The C standard.  The C++ standard doesn't apply here :)


I can't find how empty enums are considered by the C standard...


But all of this falls apart with conditional members!

Example 1 (taken from qapi/block-core.json):

 { 'enum': 'BlockdevAioOptions',
   'data': [ 'threads', 'native',
 { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' } ] }

Generates now:

 typedef enum BlockdevAioOptions {
 BLOCKDEV_AIO_OPTIONS_THREADS,
 BLOCKDEV_AIO_OPTIONS_NATIVE,
 #if defined(CONFIG_LINUX_IO_URING)
 BLOCKDEV_AIO_OPTIONS_IO_URING,
 #endif /* defined(CONFIG_LINUX_IO_URING) */
 BLOCKDEV_AIO_OPTIONS__MAX,
 } BlockdevAioOptions;

BLOCKDEV_AIO_OPTIONS__MAX is 3 if defined(CONFIG_LINUX_IO_URING), else
2.

After the next patch:

 typedef enum BlockdevAioOptions {
 BLOCKDEV_AIO_OPTIONS_THREADS,
 BLOCKDEV_AIO_OPTIONS_NATIVE,
 #if defined(CONFIG_LINUX_IO_URING)
 BLOCKDEV_AIO_OPTIONS_IO_URING,
 #endif /* defined(CONFIG_LINUX_IO_URING) */
 #define BLOCKDEV_AIO_OPTIONS__MAX 3
 } BlockdevAioOptions;

Now it's always 3.


Good catch... Using:

-- >8 --
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
@@ -110,8 +110,11 @@ def gen_enum(name: str,

 ret += mcgen('''
 } %(c_name)s;
+_Static_assert(%(c_last_enum)s == %(c_length)s - 1, "%(c_name)s");
 ''',
- c_name=c_name(name))
+ c_name=c_name(name),
+ c_last_enum=c_enum_const(name, members[-1].name, prefix),
+ c_length=len(members))

 ret += mcgen('''
---

I get:

./qapi/qapi-types-block-core.h:355:1: error: static_assert failed due to 
requirement 'BLOCKDEV_AIO_OPTIONS_NATIVE == 3 - 1' "BlockdevAioOptions"
_Static_assert(BLOCKDEV_AIO_OPTIONS_IO_URING == 3 - 1, 
"BlockdevAioOptions");

^  ~~
./qapi/qapi-types-block-core.h:430:1: error: static_assert failed due to 
requirement 'BLOCKDEV_DRIVER_VVFAT == 47 - 1' "BlockdevDriver"

_Static_assert(BLOCKDEV_DRIVER_VVFAT == 47 - 1, "BlockdevDriver");
^  ~~~
./qapi/qapi-types-char.h:110:1: error: static_assert failed due to 
requirement 'CHARDEV_BACKEND_KIND_MEMORY == 22 - 1' "ChardevBackendKind"

_Static_assert(CHARDEV_BACKEND_KIND_MEMORY == 22 - 1, "ChardevBackendKind");
^  ~
...

[PATCH v15 2/4] vhost_vdpa: fix the input in trace_vhost_vdpa_listener_region_del()

In trace_vhost_vdpa_listener_region_del, the value for llend
should change to int128_get64(int128_sub(llend, int128_one()))

Signed-off-by: Cindy Lu 
---
 hw/virtio/vhost-vdpa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bc6bad23d5..92c2413c76 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -288,7 +288,8 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
*listener,
 iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
 llend = vhost_vdpa_section_end(section);
 
-trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
+trace_vhost_vdpa_listener_region_del(v, iova,
+int128_get64(int128_sub(llend, int128_one(;
 
 if (int128_ge(int128_make64(iova), llend)) {
 return;
-- 
2.34.3

[PATCH v15 4/4] vhost-vdpa: Add support for vIOMMU.

1. The vIOMMU support will make vDPA can work in IOMMU mode. This
will fix security issues while using the no-IOMMU mode.
To support this feature we need to add new functions for IOMMU MR adds and
deletes.

Also since the SVQ does not support vIOMMU yet, add the check for IOMMU
in vhost_vdpa_dev_start, if the SVQ and IOMMU enable at the same time
the function will return fail.

2. Skip the iova_max check vhost_vdpa_listener_skipped_section(). While
MR is IOMMU, move this check to vhost_vdpa_iommu_map_notify()

Verified in vp_vdpa and vdpa_sim_net driver

Signed-off-by: Cindy Lu 
---
 hw/virtio/trace-events |   2 +-
 hw/virtio/vhost-vdpa.c | 159 ++---
 include/hw/virtio/vhost-vdpa.h |  11 +++
 3 files changed, 161 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 8f8d05cf9b..de4da2c65c 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -33,7 +33,7 @@ vhost_user_create_notifier(int idx, void *n) "idx:%d n:%p"
 vhost_vdpa_dma_map(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, 
uint64_t iova, uint64_t size, uint64_t uaddr, uint8_t perm, uint8_t type) 
"vdpa:%p fd: %d msg_type: %"PRIu32" asid: %"PRIu32" iova: 0x%"PRIx64" size: 
0x%"PRIx64" uaddr: 0x%"PRIx64" perm: 0x%"PRIx8" type: %"PRIu8
 vhost_vdpa_dma_unmap(void *vdpa, int fd, uint32_t msg_type, uint32_t asid, 
uint64_t iova, uint64_t size, uint8_t type) "vdpa:%p fd: %d msg_type: %"PRIu32" 
asid: %"PRIu32" iova: 0x%"PRIx64" size: 0x%"PRIx64" type: %"PRIu8
 vhost_vdpa_listener_begin_batch(void *v, int fd, uint32_t msg_type, uint8_t 
type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
-vhost_vdpa_listener_commit(void *v, int fd, uint32_t msg_type, uint8_t type)  
"vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
+vhost_vdpa_iotlb_batch_end_once(void *v, int fd, uint32_t msg_type, uint8_t 
type)  "vdpa:%p fd: %d msg_type: %"PRIu32" type: %"PRIu8
 vhost_vdpa_listener_region_add(void *vdpa, uint64_t iova, uint64_t llend, void 
*vaddr, bool readonly) "vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64" vaddr: %p 
read-only: %d"
 vhost_vdpa_listener_region_del(void *vdpa, uint64_t iova, uint64_t llend) 
"vdpa: %p iova 0x%"PRIx64" llend 0x%"PRIx64
 vhost_vdpa_add_status(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 0c8c37e786..39720d12a6 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -26,6 +26,7 @@
 #include "cpu.h"
 #include "trace.h"
 #include "qapi/error.h"
+#include "hw/virtio/virtio-access.h"
 
 /*
  * Return one past the end of the end of section. Be careful with uint64_t
@@ -60,13 +61,21 @@ static bool 
vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
  iova_min, section->offset_within_address_space);
 return true;
 }
+/*
+ * While using vIOMMU, sometimes the section will be larger than iova_max,
+ * but the memory that actually maps is smaller, so move the check to
+ * function vhost_vdpa_iommu_map_notify(). That function will use the 
actual
+ * size that maps to the kernel
+ */
 
-llend = vhost_vdpa_section_end(section);
-if (int128_gt(llend, int128_make64(iova_max))) {
-error_report("RAM section out of device range (max=0x%" PRIx64
- ", end addr=0x%" PRIx64 ")",
- iova_max, int128_get64(llend));
-return true;
+if (!memory_region_is_iommu(section->mr)) {
+llend = vhost_vdpa_section_end(section);
+if (int128_gt(llend, int128_make64(iova_max))) {
+error_report("RAM section out of device range (max=0x%" PRIx64
+ ", end addr=0x%" PRIx64 ")",
+ iova_max, int128_get64(llend));
+return true;
+}
 }
 
 return false;
@@ -158,9 +167,8 @@ static void vhost_vdpa_iotlb_batch_begin_once(struct 
vhost_vdpa *v)
 v->iotlb_batch_begin_sent = true;
 }
 
-static void vhost_vdpa_listener_commit(MemoryListener *listener)
+static void vhost_vdpa_iotlb_batch_end_once(struct vhost_vdpa *v)
 {
-struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener);
 struct vhost_dev *dev = v->dev;
 struct vhost_msg_v2 msg = {};
 int fd = v->device_fd;
@@ -176,7 +184,7 @@ static void vhost_vdpa_listener_commit(MemoryListener 
*listener)
 msg.type = v->msg_type;
 msg.iotlb.type = VHOST_IOTLB_BATCH_END;
 
-trace_vhost_vdpa_listener_commit(v, fd, msg.type, msg.iotlb.type);
+trace_vhost_vdpa_iotlb_batch_end_once(v, fd, msg.type, msg.iotlb.type);
 if (write(fd, , sizeof(msg)) != sizeof(msg)) {
 error_report("failed to write, fd=%d, errno=%d (%s)",
  fd, errno, strerror(errno));
@@ -185,6 +193,124 @@ static void vhost_vdpa_listener_commit(MemoryListener 
*listener)
 v->iotlb_batch_begin_sent = false;
 }
 
+static void vhost_vdpa_listener_commit(MemoryListener

[PATCH v15 1/4] vhost: expose function vhost_dev_has_iommu()

To support vIOMMU in vdpa, need to exposed the function
vhost_dev_has_iommu, vdpa will use this function to check
if vIOMMU enable.

Signed-off-by: Cindy Lu 
---
 hw/virtio/vhost.c | 2 +-
 include/hw/virtio/vhost.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index a266396576..fd746b085b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -107,7 +107,7 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
 }
 }
 
-static bool vhost_dev_has_iommu(struct vhost_dev *dev)
+bool vhost_dev_has_iommu(struct vhost_dev *dev)
 {
 VirtIODevice *vdev = dev->vdev;
 
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a52f273347..f7f10c8fb7 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -336,4 +336,5 @@ int vhost_dev_set_inflight(struct vhost_dev *dev,
struct vhost_inflight *inflight);
 int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
struct vhost_inflight *inflight);
+bool vhost_dev_has_iommu(struct vhost_dev *dev);
 #endif
-- 
2.34.3

[PATCH v15 3/4] vhost-vdpa: Add check for full 64-bit in region delete

The unmap ioctl doesn't accept a full 64-bit span. So need to
add check for the section's size in vhost_vdpa_listener_region_del().

Signed-off-by: Cindy Lu 
---
 hw/virtio/vhost-vdpa.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 92c2413c76..0c8c37e786 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -316,10 +316,28 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
*listener,
 vhost_iova_tree_remove(v->iova_tree, *result);
 }
 vhost_vdpa_iotlb_batch_begin_once(v);
+/*
+ * The unmap ioctl doesn't accept a full 64-bit. need to check it
+ */
+if (int128_eq(llsize, int128_2_64())) {
+llsize = int128_rshift(llsize, 1);
+ret = vhost_vdpa_dma_unmap(v, VHOST_VDPA_GUEST_PA_ASID, iova,
+   int128_get64(llsize));
+
+if (ret) {
+error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
+ "0x%" HWADDR_PRIx ") = %d (%m)",
+ v, iova, int128_get64(llsize), ret);
+}
+iova += int128_get64(llsize);
+}
 ret = vhost_vdpa_dma_unmap(v, VHOST_VDPA_GUEST_PA_ASID, iova,
int128_get64(llsize));
+
 if (ret) {
-error_report("vhost_vdpa dma unmap error!");
+error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
+ "0x%" HWADDR_PRIx ") = %d (%m)",
+ v, iova, int128_get64(llsize), ret);
 }
 
 memory_region_unref(section->mr);
-- 
2.34.3

[PATCH v15 0/4] vhost-vdpa: add support for vIOMMU

These patches are to support vIOMMU in vdpa device

changes in V3
1. Move function vfio_get_xlat_addr to memory.c
2. Use the existing memory listener, while the MR is
iommu MR then call the function iommu_region_add/
iommu_region_del

changes in V4
1.make the comments in vfio_get_xlat_addr more general

changes in V5
1. Address the comments in the last version
2. Add a new arg in the function vfio_get_xlat_addr, which shows whether
the memory is backed by a discard manager. So the device can have its
own warning.

changes in V6
move the error_report for the unpopulated discard back to
memeory_get_xlat_addr

changes in V7
organize the error massage to avoid the duplicate information

changes in V8
Organize the code follow the comments in the last version

changes in V9
Organize the code follow the comments

changes in V10
Address the comments

changes in V11
Address the comments
fix the crash found in test

changes in V12
Address the comments, squash patch 1 into the next patch
improve the code style issue

changes in V13
fail to start if IOMMU and svq enable at same time
improve the code style issue

changes in V14
Address the comments

changes in V15
Address the comments

Cindy Lu (4):
  vhost: expose function vhost_dev_has_iommu()
  vhost_vdpa: fix the input in trace_vhost_vdpa_listener_region_del()
  vhost-vdpa: Add check for full 64-bit in region delete
  vhost-vdpa: Add support for vIOMMU.

 hw/virtio/trace-events |   2 +-
 hw/virtio/vhost-vdpa.c | 182 ++---
 hw/virtio/vhost.c  |   2 +-
 include/hw/virtio/vhost-vdpa.h |  11 ++
 include/hw/virtio/vhost.h  |   1 +
 5 files changed, 184 insertions(+), 14 deletions(-)

-- 
2.34.3

Re: [PATCH 01/10] metadata: add .git-blame-ignore-revs


On 20/3/23 11:10, Alex Bennée wrote:

Someone mentioned this on IRC so I thought I would try it out with a
few commits that are pure code style fixes.

Signed-off-by: Alex Bennée 
---
  .git-blame-ignore-revs | 18 ++
  1 file changed, 18 insertions(+)
  create mode 100644 .git-blame-ignore-revs

diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs
new file mode 100644
index 00..24208ece8c
--- /dev/null
+++ b/.git-blame-ignore-revs
@@ -0,0 +1,18 @@
+#
+# List of code-formatting clean ups the git blame can ignore
+#
+#   git blame --ignore-revs-file .git-blame-ignore-revs
+#
+# or
+#
+#   git config blame.ignoreRevsFile .git-blame-ignore-revs
+#
+
+# gdbstub: clean-up indents
+ad9e4585b3c7425759d3eea697afbca71d2c2082
+
+# e1000e: fix code style
+0eadd56bf53ab196a16d492d7dd31c62e1c24c32
+
+# target/riscv: coding style fixes
+8c7fed9218b407792120bcfda0347ed16205


Please amend:

+# replace TABs with spaces
+48805df9c22a0700fba4b3b548fafaa21726ca68

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

[PATCH 2/2] Add test for storing .new vector

Hexagon toolchain version 16.0.0 fixes a bug where the ecoding of
storing a .new vector was incorrect. This resulted in an incorrect
valued being stored. The test checks that the correct value is used.

Signed-off-by: Marco Liebel 
---
 tests/tcg/hexagon/hvx_misc.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
index 53d5c9b44f..657e556dd4 100644
--- a/tests/tcg/hexagon/hvx_misc.c
+++ b/tests/tcg/hexagon/hvx_misc.c
@@ -211,6 +211,34 @@ static void test_store_unaligned(void)
 check_output_w(__LINE__, 2);
 }
 
+static void test_store_new(void)
+{
+asm volatile(
+"r0 = #0x0003\n\t"
+"v0 = vsplat(r0)\n\t"
+"r0 = #expect\n\t"
+"vmem(r0+#0) = v0\n\t"
+
+"r0 = #output\n\t"
+"r1 = #0x0001\n\t"
+"r2 = #0x0002\n\t"
+"r3 = #0x0004\n\t"
+
+"v1 = vsplat(r1)\n\t"
+"v2 = vsplat(r2)\n\t"
+"v3 = vsplat(r3)\n\t"
+
+"{"
+"   v3.w,q0 = vadd(v1.w, v2.w):carry\n\t"
+"   vmem(r0+#0) = v3.new\n\t"
+"}"
+
+::: "r0", "r1", "r2", "r3", "v0", "v1", "v2", "v3", "q0", "memory"
+);
+
+check_output_w(__LINE__, 1);
+}
+
 static void test_masked_store(bool invert)
 {
 void *p0 = buffer0;
@@ -620,6 +648,7 @@ int main()
 test_load_unaligned();
 test_store_aligned();
 test_store_unaligned();
+test_store_new();
 test_masked_store(false);
 test_masked_store(true);
 test_new_value_store();
-- 
2.25.1

[PATCH 0/2] Update hexagon toolchain

Updates the hexagon toolchain and adds a test for a bug that was fixed
by the new version.

Marco Liebel (2):
  Use hexagon toolchain version 16.0.0
  Add test for storing .new vector

 .../dockerfiles/debian-hexagon-cross.docker   |  2 +-
 tests/tcg/hexagon/hvx_misc.c  | 29 +++
 2 files changed, 30 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/2] Use hexagon toolchain version 16.0.0

Signed-off-by: Marco Liebel 
---
 tests/docker/dockerfiles/debian-hexagon-cross.docker | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/debian-hexagon-cross.docker 
b/tests/docker/dockerfiles/debian-hexagon-cross.docker
index 5308ccb8fe..b99d99f943 100644
--- a/tests/docker/dockerfiles/debian-hexagon-cross.docker
+++ b/tests/docker/dockerfiles/debian-hexagon-cross.docker
@@ -27,7 +27,7 @@ RUN apt-get update && \
 
 
 ENV TOOLCHAIN_INSTALL /opt
-ENV TOOLCHAIN_RELEASE 15.0.3
+ENV TOOLCHAIN_RELEASE 16.0.0
 ENV TOOLCHAIN_BASENAME 
"clang+llvm-${TOOLCHAIN_RELEASE}-cross-hexagon-unknown-linux-musl"
 ENV TOOLCHAIN_URL 
https://codelinaro.jfrog.io/artifactory/codelinaro-toolchain-for-hexagon/v${TOOLCHAIN_RELEASE}/${TOOLCHAIN_BASENAME}.tar.xz
 
-- 
2.25.1

Re: [PATCH v4 0/9] improvement to Python detection, preparation for dropping 3.6


On 22/2/23 15:37, Paolo Bonzini wrote:

This is my take on John's patches to improve Python detection and to
prepare for dropping Python 3.6 support.

The main change with respect to John's work is that lcitool is updated
and the container images for CI can install Sphinx via pip; this
way documentation is still built on the CentOS 8 jobs.

A smaller change is that patch "configure: Look for auxiliary Python
installations" will only look at the $PYTHON variable if it is set,
without falling back to a PATH search.

This series includes the final patch to drop support for Python 3.6,
but it makes sense even without it.

Paolo

Supersedes: <20230221012456.2607692-1-js...@redhat.com>


FWIW:

Different patches 1 & 2 have been merged 2 days after you posted
this series (merge commit c3aeccc0ab):
- commit aef633e765 ("python: support pylint 2.16")
- commit 6832189fd7 ("python: drop pipenv")

Patch 3 clashes with commit 1b1be8d3cc ("meson: stop
looking for 'sphinx-build-3'")


John Snow (5):
   python: support pylint 2.16
   python: drop pipenv
   meson: prefer 'sphinx-build' to 'sphinx-build-3'
   configure: Look for auxiliary Python installations
   configure: Add courtesy hint to Python version failure message

Paolo Bonzini (5):
   configure: protect against escaping venv when running Meson
   lcitool: update submodule
   docs/devel: update and clarify lcitool instructions
   ci, docker: update CentOS and OpenSUSE Python to non-EOL versions
   Python: Drop support for Python 3.6

Re: [PULL 0/7] ui/ fixes for 8.0

On Tue, 21 Mar 2023 at 09:04,  wrote:
>
> From: Marc-André Lureau 
>
> The following changes since commit aa9e7fa4689d1becb2faf67f65aafcbcf664f1ce:
>
>   Merge tag 'edk2-stable202302-20230320-pull-request' of 
> https://gitlab.com/kraxel/qemu into staging (2023-03-20 13:43:35 +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/marcandre.lureau/qemu.git tags/ui-pull-request
>
> for you to fetch changes up to 49152ac47003ca21fc6f2a5c3e517f79649e1541:
>
>   ui: fix crash on serial reset, during init (2023-03-21 11:46:22 +0400)
>
> 
> ui/ fixes for 8.0
>
> 
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.0
for any user-visible changes.

-- PMM

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

On Tue, 21 Mar 2023 at 13:55, Mostafa Saleh  wrote:
>
> Hi Peter,
>
> On Tue, Mar 21, 2023 at 01:34:55PM +, Peter Maydell wrote:
> > > >>> + */
> > > >>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > > >>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, 
> > > >>> ID_AA64MMFR0, PARANGE);
> > > >> is this working in accelerated mode?
> > > > I didn't try with accel, I will give it a try, but from what I see, that
> > > > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > > > accel/kvm-all.c, so it seems this would work for accelerated mode.
> > >
> > > yeah I ma not familiar enough with that code but it is worth to be 
> > > checked.
> >
> > I'm a bit unsure about fishing around in the CPU ID registers for this.
> > That's not what you would do in real hardware, you'd just say "the
> > system is supposed to configure the CPU and the SMMU correctly".
> >
> > Also, there is no guarantee that CPU 0 is related to this SMMU in
> > particular.
> Sorry, missed this point in last email.
>
> So, we leave it this way, or there is a better way to discover PARANGE?

If you really need to know it, put a QOM property on the SMMU device
and have the board code set it. (This is analogous to how it works
in hardware: there are tie-off signals on the SMMU for the OAS value.)

-- PMM

Re: [PULL 0/7] ui/ fixes for 8.0

2023-03-21 Thread Erico Nunes

On 21/03/2023 10:03, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> The following changes since commit aa9e7fa4689d1becb2faf67f65aafcbcf664f1ce:
> 
>   Merge tag 'edk2-stable202302-20230320-pull-request' of 
> https://gitlab.com/kraxel/qemu into staging (2023-03-20 13:43:35 +)
> 
> are available in the Git repository at:
> 
>   https://gitlab.com/marcandre.lureau/qemu.git tags/ui-pull-request
> 
> for you to fetch changes up to 49152ac47003ca21fc6f2a5c3e517f79649e1541:
> 
>   ui: fix crash on serial reset, during init (2023-03-21 11:46:22 +0400)
> 
> 
> ui/ fixes for 8.0
> 
> 
> 
> Erico Nunes (1):
>   ui/sdl2: remove workaround forcing x11
> 
> Marc-André Lureau (6):
>   win32: add qemu_close_socket_osfhandle()
>   ui/spice: fix SOCKET handling regression
>   ui/dbus: fix passing SOCKET to GSocket API & leak
>   ui/gtk: fix cursor moved to left corner
>   ui: return the default console cursor when con == NULL
>   ui: fix crash on serial reset, during init


May I also suggest this one as a fix for 8.0:
https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg05667.html

It was already reviewed about a month ago.

Thanks

Erico

vma-pthread unstable on aarch64 hardware

Date: Tue, 21 Mar 2023 13:48:20 +
User-agent: mu4e 1.9.22; emacs 29.0.60

Hi,

Chasing down some unstable check-tcg tests and I can get vma-pthread to
fail fairly reliably on the CI configuration ('../../configure'
'--enable-debug' '--static' '--disable-system' '--disable-pie') although
it seems to hold up on the default configuration ok.

  retry.py -n 30 -c -- ./qemu-aarch64 ./tests/tcg/aarch64-linux-user/vma-pthread
  ...
  **
  ERROR:../../accel/tcg/cpu-exec.c:1019:cpu_exec_setjmp: assertion failed: (cpu 
== current_cpu)
  Bail out! ERROR:../../accel/tcg/cpu-exec.c:1019:cpu_exec_setjmp: assertion 
failed: (cpu == current_cpu)
  ...
  Results summary:
  0: 29 times (96.67%), avg time 1.503 (0.00 varience/0.00 deviation)
  -5: 1 times (3.33%), avg time 0.252 (0.00 varience/0.00 deviation)
  Ran command 30 times, 29 passes

That said it might be responsible for some of the other tests that fail
when I do something like:

  cd tests/tcg/aarch64-linux-user/
  retry.py -n 30 --until -- make -f ../Makefile.target run

where I've seen random failures in float_convs, mte-1 and testthread
which make me wonder if this is some sort of toolchain/build config
issue?

Any ideas?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Mostafa Saleh

Hi Peter,

On Tue, Mar 21, 2023 at 01:34:55PM +, Peter Maydell wrote:
> > >>> + */
> > >>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > >>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> > >>> PARANGE);
> > >> is this working in accelerated mode?
> > > I didn't try with accel, I will give it a try, but from what I see, that
> > > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > > accel/kvm-all.c, so it seems this would work for accelerated mode.
> >
> > yeah I ma not familiar enough with that code but it is worth to be checked.
> 
> I'm a bit unsure about fishing around in the CPU ID registers for this.
> That's not what you would do in real hardware, you'd just say "the
> system is supposed to configure the CPU and the SMMU correctly".
> 
> Also, there is no guarantee that CPU 0 is related to this SMMU in
> particular.
Sorry, missed this point in last email.

So, we leave it this way, or there is a better way to discover PARANGE?

Thanks,
Mostafa

Re: [PATCH] .editorconfig: set max line at 70 chars for QAPI files

2023-03-21 Thread Markus Armbruster

Marc-André Lureau  writes:

> Hi
>
> On Tue, Mar 7, 2023 at 4:32 PM  wrote:
>>
>> From: Marc-André Lureau 
>>
>> This seems to be the preferred style.
>>
>> The EditorConfig property is not supported by all editors:
>> https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  .editorconfig | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/.editorconfig b/.editorconfig
>> index 7303759ed7..8c5ebc6a1b 100644
>> --- a/.editorconfig
>> +++ b/.editorconfig
>> @@ -47,3 +47,4 @@ emacs_mode = glsl
>>  [*.json]
>>  indent_style = space
>>  emacs_mode = python
>> +max_line_length = 70
>
> ack or nack ?

I think we should first address the doc syntax misfeature that pushes us
to the right, and clean up existing overlong lines.  Can't say how hard
the former would be, so I'm having a look.

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Eric Auger

Hi Peter,

On 3/21/23 14:34, Peter Maydell wrote:
> thout having read much of the context, but why
> would we need to migrate the ID registers? They are constant, read-only,
> so they will be the same value on both source and destination.
this series modifies the values of IDR[5] (oas).  So my understanding is
the guest is likely to behave differently on src and dst, depending on
the qemu version, no?

Thanks

Eric

Re: [PATCH 1/2] hw/i2c: smbus_slave: Reset state on reset


On 20/3/23 23:14, Joe Komlodi wrote:

If a reset comes while the SMBus device is not in its idle state, it's
possible for it to get confused on valid transactions post-reset.

Signed-off-by: Joe Komlodi 
---
  hw/i2c/smbus_slave.c | 9 +
  1 file changed, 9 insertions(+)


Reviewed-by: Philippe Mathieu-Daudé

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Mostafa Saleh

Hi Peter,

On Tue, Mar 21, 2023 at 01:34:55PM +, Peter Maydell wrote:
> > >>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> > >> I am not sure you can change that easily. In case of migration this is
> > >> going to change the behavior of the device, no?
> > > I see IDR registers are not migrated. I guess we can add them in a
> > > subsection and if they were not passed (old instances) we set OAS to
> > > 44.
> > > Maybe this should be another change outside of this series.
> > Indeed tehy are not migrated so it can lead to inconsistent behavior in
> > both source and dest. This deserves more analysis to me. In case you
> > would decide to migrate IDR regs this would need to be done in that
> > series I think. Migration must not be broken by this series.
> 
> Jumping in here without having read much of the context, but why
> would we need to migrate the ID registers? They are constant, read-only,
> so they will be the same value on both source and destination.

Currently OAS for SMMU is hardcoded to 44 bits, and the SMMU manual says
"OAS reflects the maximum usable PA output from the last stage of
AArch64 translations, and must match the system physical address size.
The OAS is discoverable from SMMU_IDR5.OAS."
This patch implements OAS based on CPU PARANGE, but this would break
migration from old instances that ran with 44 bits OAS to new code that
configures it based on the current CPU.
So one idea is to migrate the IDRs (or atleast IDR5).
Maybe that is not the best solution, we just need a way to know if the
old instance needs to be 44 bits or not.

Thanks,
Mostafa

[PATCH v2] target/riscv: reduce overhead of MSTATUS_SUM change

2023-03-21 Thread Fei Wu

Kernel needs to access user mode memory e.g. during syscalls, the window
is usually opened up for a very limited time through MSTATUS.SUM, the
overhead is too much if tlb_flush() gets called for every SUM change.
This patch saves addresses accessed when SUM=1, and flushs only these
pages when SUM changes to 0. If the buffer is not large enough to save
all the pages during SUM=1, it will fall back to tlb_flush when
necessary.

The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
most of the time kernel accesses 1 or 2 pages, it's very rare to see
more than 4 pages accessed.

It's not necessary to save/restore these new added status, as
tlb_flush() is always called after restore.

Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
other syscalls benefit a lot from this one too.

Signed-off-by: Fei Wu 
Reviewed-by: LIU Zhiwei 
Reviewed-by: Weiwei Li 
---
 target/riscv/cpu.h|  4 
 target/riscv/cpu_helper.c |  7 +++
 target/riscv/csr.c| 14 +-
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..926dbce59f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -383,6 +383,10 @@ struct CPUArchState {
 uint64_t kvm_timer_compare;
 uint64_t kvm_timer_state;
 uint64_t kvm_timer_frequency;
+
+#define MAX_CACHED_SUM_U_ADDR_NUM 4
+uint64_t sum_u_count;
+uint64_t sum_u_addr[MAX_CACHED_SUM_U_ADDR_NUM];
 };
 
 OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..d701017a60 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1068,6 +1068,13 @@ restart:
 (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
 *prot |= PAGE_WRITE;
 }
+if ((pte & PTE_U) && (mode == PRV_S) &&
+get_field(env->mstatus, MSTATUS_SUM)) {
+if (env->sum_u_count < MAX_CACHED_SUM_U_ADDR_NUM) {
+env->sum_u_addr[env->sum_u_count] = addr;
+}
+++env->sum_u_count;
+}
 return TRANSLATE_SUCCESS;
 }
 }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ab566639e5..e7dfdc6a93 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1246,9 +1246,21 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
 
 /* flush tlb on mstatus fields that affect VM */
 if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-MSTATUS_MPRV | MSTATUS_SUM)) {
+MSTATUS_MPRV)) {
 tlb_flush(env_cpu(env));
+env->sum_u_count = 0;
+} else if ((mstatus & MSTATUS_SUM) && !(val & MSTATUS_SUM)) {
+if (env->sum_u_count > MAX_CACHED_SUM_U_ADDR_NUM) {
+tlb_flush_by_mmuidx(env_cpu(env), 1 << PRV_S | 1 << PRV_M);
+} else {
+for (int i = 0; i < env->sum_u_count; ++i) {
+tlb_flush_page_by_mmuidx(env_cpu(env), env->sum_u_addr[i],
+ 1 << PRV_S | 1 << PRV_M);
+}
+}
+env->sum_u_count = 0;
 }
+
 mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
 MSTATUS_SPP | MSTATUS_MPRV | MSTATUS_SUM |
 MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR |
-- 
2.25.1

Re: [RFC PATCH] tests/avocado: probe for multi-process support before running test


On 21/3/23 12:17, Alex Bennée wrote:

A recent attempt to let avocado run more tests on the CentOS stream
build failed because there was no gating on the multiprocess feature.
Like missing accelerators avocado should gracefully skip when the
feature is not enabled.

In this case we use the existence of the proxy device as a proxy for
multi-process support.

Signed-off-by: Alex Bennée 
Cc: Elena Ufimtseva 
Cc: Jagannathan Raman 
Cc: John G Johnson 
---
  tests/avocado/avocado_qemu/__init__.py | 10 ++
  tests/avocado/multiprocess.py  |  1 +
  2 files changed, 11 insertions(+)




+"""
+Test for the presence of the x-pci-proxy-dev which is required
+to support multiprocess.
+"""
+devhelp = run_cmd([self.qemu_bin,
+   '-M', 'none', '-device', 'help'])[0];
+if devhelp.find('x-pci-proxy-dev') < 0:
+self.cancel('no support for multiprocess device emulation')


FYI a more generic alternative to this method:
https://lore.kernel.org/qemu-devel/20200129212345.20547-14-phi...@redhat.com/

But yours just works :)

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

On Tue, 21 Mar 2023 at 13:23, Eric Auger  wrote:
>
> Hi Mostafa,
>
> On 3/21/23 14:06, Mostafa Saleh wrote:
> > Hi Eric,
> >
> >>> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
> >>> address
> >>> + * size.
> >>> + */
> >>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> >>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> >>> PARANGE);
> >> is this working in accelerated mode?
> > I didn't try with accel, I will give it a try, but from what I see, that
> > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > accel/kvm-all.c, so it seems this would work for accelerated mode.
>
> yeah I ma not familiar enough with that code but it is worth to be checked.

I'm a bit unsure about fishing around in the CPU ID registers for this.
That's not what you would do in real hardware, you'd just say "the
system is supposed to configure the CPU and the SMMU correctly".

Also, there is no guarantee that CPU 0 is related to this SMMU in
particular.

> >
> >>> +
> >>>  /**
> >>>   * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> >>>   *   multi-level stream table
> >>> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> >>> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> >>> bits */
> >>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> >> I am not sure you can change that easily. In case of migration this is
> >> going to change the behavior of the device, no?
> > I see IDR registers are not migrated. I guess we can add them in a
> > subsection and if they were not passed (old instances) we set OAS to
> > 44.
> > Maybe this should be another change outside of this series.
> Indeed tehy are not migrated so it can lead to inconsistent behavior in
> both source and dest. This deserves more analysis to me. In case you
> would decide to migrate IDR regs this would need to be done in that
> series I think. Migration must not be broken by this series.

Jumping in here without having read much of the context, but why
would we need to migrate the ID registers? They are constant, read-only,
so they will be the same value on both source and destination.

thanks
-- PMM

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Eric Auger




On 3/21/23 14:29, Mostafa Saleh wrote:
> On Tue, Mar 21, 2023 at 02:23:03PM +0100, Eric Auger wrote:
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> bits */
> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
 I am not sure you can change that easily. In case of migration this is
 going to change the behavior of the device, no?
>>> I see IDR registers are not migrated. I guess we can add them in a
>>> subsection and if they were not passed (old instances) we set OAS to
>>> 44.
>>> Maybe this should be another change outside of this series.
>> Indeed tehy are not migrated so it can lead to inconsistent behavior in
>> both source and dest. This deserves more analysis to me. In case you
>> would decide to migrate IDR regs this would need to be done in that
>> series I think. Migration must not be broken by this series
> I agree, I meant to drop this patch from the series as it is not
> really related to stage-2, and we can have another patch for this +
> migration for IDR if needed after doing proper analysis.

Ah OK. I get it now. Yes this looks sensible

Thanks

Eric
>
> Thanks,
> Mostafa
>

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Mostafa Saleh

On Tue, Mar 21, 2023 at 02:23:03PM +0100, Eric Auger wrote:
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> >>> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> >>> bits */
> >>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> >> I am not sure you can change that easily. In case of migration this is
> >> going to change the behavior of the device, no?
> > I see IDR registers are not migrated. I guess we can add them in a
> > subsection and if they were not passed (old instances) we set OAS to
> > 44.
> > Maybe this should be another change outside of this series.
> Indeed tehy are not migrated so it can lead to inconsistent behavior in
> both source and dest. This deserves more analysis to me. In case you
> would decide to migrate IDR regs this would need to be done in that
> series I think. Migration must not be broken by this series

I agree, I meant to drop this patch from the series as it is not
really related to stage-2, and we can have another patch for this +
migration for IDR if needed after doing proper analysis.

Thanks,
Mostafa

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

2023-03-21 Thread liweiwei




On 2023/3/21 21:22, Wu, Fei wrote:

On 3/21/2023 8:58 PM, liweiwei wrote:

On 2023/3/21 14:37, fei2...@intel.com wrote:

From: Fei Wu 

Kernel needs to access user mode memory e.g. during syscalls, the window
is usually opened up for a very limited time through MSTATUS.SUM, the
overhead is too much if tlb_flush() gets called for every SUM change.
This patch saves addresses accessed when SUM=1, and flushs only these
pages when SUM changes to 0. If the buffer is not large enough to save
all the pages during SUM=1, it will fall back to tlb_flush when
necessary.

The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
most of the time kernel accesses 1 or 2 pages, it's very rare to see
more than 4 pages accessed.

It's not necessary to save/restore these new added status, as
tlb_flush() is always called after restore.

Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
other syscalls benefit a lot from this one too.

Signed-off-by: Fei Wu 
Reviewed-by: LIU Zhiwei 
---
   target/riscv/cpu.h    |  4 
   target/riscv/cpu_helper.c |  7 +++
   target/riscv/csr.c    | 14 +-
   3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..926dbce59f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -383,6 +383,10 @@ struct CPUArchState {
   uint64_t kvm_timer_compare;
   uint64_t kvm_timer_state;
   uint64_t kvm_timer_frequency;
+
+#define MAX_CACHED_SUM_U_ADDR_NUM 4
+    uint64_t sum_u_count;
+    uint64_t sum_u_addr[MAX_CACHED_SUM_U_ADDR_NUM];
   };
     OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..5ad0418eb6 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1068,6 +1068,13 @@ restart:
   (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
   *prot |= PAGE_WRITE;
   }
+    if ((pte & PTE_U) && (mode & PRV_S) &&
+    get_field(env->mstatus, MSTATUS_SUM)) {
+    if (env->sum_u_count < MAX_CACHED_SUM_U_ADDR_NUM) {
+    env->sum_u_addr[env->sum_u_count] = addr;
+    }
+    ++env->sum_u_count;
+    }
   return TRANSLATE_SUCCESS;
   }
   }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ab566639e5..74b7638c8a 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1246,9 +1246,21 @@ static RISCVException
write_mstatus(CPURISCVState *env, int csrno,
     /* flush tlb on mstatus fields that affect VM */
   if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-    MSTATUS_MPRV | MSTATUS_SUM)) {
+    MSTATUS_MPRV)) {
   tlb_flush(env_cpu(env));
+    env->sum_u_count = 0;
+    } else if ((mstatus & MSTATUS_SUM) && !(val & MSTATUS_SUM)) {
+    if (env->sum_u_count > MAX_CACHED_SUM_U_ADDR_NUM) {
+    tlb_flush(env_cpu(env));

SUM seems only affect S mode TLB. Maybe we can only flush S mode TLB here.


It's also in effect when MPRV=1 and MPP=S in M mode, we can only flush
the tlb of PRV_S and PRV_M.


OK. Good point.

Regards,

Weiwei Li



Thanks,
Fei.


+    } else {
+    for (int i = 0; i < env->sum_u_count; ++i) {
+    tlb_flush_page_by_mmuidx(env_cpu(env),
env->sum_u_addr[i],
+ 1 << PRV_S | 1 << PRV_M);

Similar case here.

Regards,

Weiwei Li


+    }
+    }
+    env->sum_u_count = 0;
   }
+
   mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
   MSTATUS_SPP | MSTATUS_MPRV | MSTATUS_SUM |
   MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR |

[PULL 1/8] target/arm: Add Neoverse-N1 registers

From: Chen Baozi 

Add implementation defined registers for neoverse-n1 which
would be accessed by TF-A. Since there is no DSU in Qemu,
CPUCFR_EL1.SCU bit is set to 1 to avoid DSU registers definition.

Signed-off-by: Chen Baozi 
Reviewed-by: Peter Maydell 
Tested-by: Marcin Juszkiewicz 
Message-id: 20230313033936.585669-1-chenba...@phytium.com.cn
Signed-off-by: Peter Maydell 
---
 target/arm/cpu64.c | 69 ++
 1 file changed, 69 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 4066950da15..0fb07cc7b6d 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "cpu.h"
+#include "cpregs.h"
 #include "qemu/module.h"
 #include "sysemu/kvm.h"
 #include "sysemu/hvf.h"
@@ -1027,6 +1028,72 @@ static void aarch64_a64fx_initfn(Object *obj)
 /* TODO:  Add A64FX specific HPC extension registers */
 }
 
+static const ARMCPRegInfo neoverse_n1_cp_reginfo[] = {
+{ .name = "ATCR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 7, .opc2 = 0,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "ATCR_EL2", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 4, .crn = 15, .crm = 7, .opc2 = 0,
+  .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "ATCR_EL3", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 7, .opc2 = 0,
+  .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "ATCR_EL12", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 5, .crn = 15, .crm = 7, .opc2 = 0,
+  .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "AVTCR_EL2", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 4, .crn = 15, .crm = 7, .opc2 = 1,
+  .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUACTLR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 0,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUACTLR2_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 1,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUACTLR3_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 2,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+/*
+ * Report CPUCFR_EL1.SCU as 1, as we do not implement the DSU
+ * (and in particular its system registers).
+ */
+{ .name = "CPUCFR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 0, .opc2 = 0,
+  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 4 },
+{ .name = "CPUECTLR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 1, .opc2 = 4,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0x961563010 },
+{ .name = "CPUPCR_EL3", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 1,
+  .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUPMR_EL3", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 3,
+  .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUPOR_EL3", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 2,
+  .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUPSELR_EL3", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 6, .crn = 15, .crm = 8, .opc2 = 0,
+  .access = PL3_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUPWRCTLR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 7,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "ERXPFGCDN_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 2,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "ERXPFGCTL_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 1,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "ERXPFGF_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 15, .crm = 2, .opc2 = 0,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+};
+
+static void define_neoverse_n1_cp_reginfo(ARMCPU *cpu)
+{
+define_arm_cp_regs(cpu, neoverse_n1_cp_reginfo);
+}
+
 static void aarch64_neoverse_n1_initfn(Object *obj)
 {
 ARMCPU *cpu = ARM_CPU(obj);
@@ -1094,6 +1161,8 @@ static void aarch64_neoverse_n1_initfn(Object *obj)
 
 /* From D5.1 AArch64 PMU register summary */
 cpu->isar.reset_pmcr_el0 = 0x410c3000;
+
+define_neoverse_n1_cp_reginfo(cpu);
 }
 
 static void aarch64_host_initfn(Object *obj)
-- 
2.34.1

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Eric Auger

Hi Mostafa,

On 3/21/23 14:06, Mostafa Saleh wrote:
> Hi Eric,
>
>>> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
>>> address
>>> + * size.
>>> + */
>>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
>>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
>>> PARANGE);
>> is this working in accelerated mode?
> I didn't try with accel, I will give it a try, but from what I see, that
> ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> accel/kvm-all.c, so it seems this would work for accelerated mode.

yeah I ma not familiar enough with that code but it is worth to be checked.
>
>>> +
>>>  /**
>>>   * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>>>   *   multi-level stream table
>>> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
>>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
>>> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
>>> bits */
>>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
>> I am not sure you can change that easily. In case of migration this is
>> going to change the behavior of the device, no?
> I see IDR registers are not migrated. I guess we can add them in a
> subsection and if they were not passed (old instances) we set OAS to
> 44.
> Maybe this should be another change outside of this series.
Indeed tehy are not migrated so it can lead to inconsistent behavior in
both source and dest. This deserves more analysis to me. In case you
would decide to migrate IDR regs this would need to be done in that
series I think. Migration must not be broken by this series.

Thanks

Eric
>
> Thanks,
> Mostafa
>

[PULL 3/8] contrib/elf2dmp: fix code style

From: Viktor Prutyanov 

Originally elf2dmp were added with some code style issues,
especially in pe.h header, and some were introduced by
2d0fc797faaa73fbc1d30f5f9e90407bf3dd93f0. Fix them now.

Signed-off-by: Viktor Prutyanov 
Reviewed-by: Annie Li 
Message-id: 2023011246.883679-2-vik...@daynix.com
Signed-off-by: Peter Maydell 
---
 contrib/elf2dmp/pe.h| 100 ++--
 contrib/elf2dmp/addrspace.c |   1 +
 contrib/elf2dmp/main.c  |   9 ++--
 3 files changed, 57 insertions(+), 53 deletions(-)

diff --git a/contrib/elf2dmp/pe.h b/contrib/elf2dmp/pe.h
index c2a4a6ba7c2..807d0063649 100644
--- a/contrib/elf2dmp/pe.h
+++ b/contrib/elf2dmp/pe.h
@@ -33,70 +33,70 @@ typedef struct IMAGE_DOS_HEADER {
 } __attribute__ ((packed)) IMAGE_DOS_HEADER;
 
 typedef struct IMAGE_FILE_HEADER {
-  uint16_t  Machine;
-  uint16_t  NumberOfSections;
-  uint32_t  TimeDateStamp;
-  uint32_t  PointerToSymbolTable;
-  uint32_t  NumberOfSymbols;
-  uint16_t  SizeOfOptionalHeader;
-  uint16_t  Characteristics;
+uint16_t  Machine;
+uint16_t  NumberOfSections;
+uint32_t  TimeDateStamp;
+uint32_t  PointerToSymbolTable;
+uint32_t  NumberOfSymbols;
+uint16_t  SizeOfOptionalHeader;
+uint16_t  Characteristics;
 } __attribute__ ((packed)) IMAGE_FILE_HEADER;
 
 typedef struct IMAGE_DATA_DIRECTORY {
-  uint32_t VirtualAddress;
-  uint32_t Size;
+uint32_t VirtualAddress;
+uint32_t Size;
 } __attribute__ ((packed)) IMAGE_DATA_DIRECTORY;
 
 #define IMAGE_NUMBEROF_DIRECTORY_ENTRIES 16
 
 typedef struct IMAGE_OPTIONAL_HEADER64 {
-  uint16_t  Magic; /* 0x20b */
-  uint8_t   MajorLinkerVersion;
-  uint8_t   MinorLinkerVersion;
-  uint32_t  SizeOfCode;
-  uint32_t  SizeOfInitializedData;
-  uint32_t  SizeOfUninitializedData;
-  uint32_t  AddressOfEntryPoint;
-  uint32_t  BaseOfCode;
-  uint64_t  ImageBase;
-  uint32_t  SectionAlignment;
-  uint32_t  FileAlignment;
-  uint16_t  MajorOperatingSystemVersion;
-  uint16_t  MinorOperatingSystemVersion;
-  uint16_t  MajorImageVersion;
-  uint16_t  MinorImageVersion;
-  uint16_t  MajorSubsystemVersion;
-  uint16_t  MinorSubsystemVersion;
-  uint32_t  Win32VersionValue;
-  uint32_t  SizeOfImage;
-  uint32_t  SizeOfHeaders;
-  uint32_t  CheckSum;
-  uint16_t  Subsystem;
-  uint16_t  DllCharacteristics;
-  uint64_t  SizeOfStackReserve;
-  uint64_t  SizeOfStackCommit;
-  uint64_t  SizeOfHeapReserve;
-  uint64_t  SizeOfHeapCommit;
-  uint32_t  LoaderFlags;
-  uint32_t  NumberOfRvaAndSizes;
-  IMAGE_DATA_DIRECTORY DataDirectory[IMAGE_NUMBEROF_DIRECTORY_ENTRIES];
+uint16_t  Magic; /* 0x20b */
+uint8_t   MajorLinkerVersion;
+uint8_t   MinorLinkerVersion;
+uint32_t  SizeOfCode;
+uint32_t  SizeOfInitializedData;
+uint32_t  SizeOfUninitializedData;
+uint32_t  AddressOfEntryPoint;
+uint32_t  BaseOfCode;
+uint64_t  ImageBase;
+uint32_t  SectionAlignment;
+uint32_t  FileAlignment;
+uint16_t  MajorOperatingSystemVersion;
+uint16_t  MinorOperatingSystemVersion;
+uint16_t  MajorImageVersion;
+uint16_t  MinorImageVersion;
+uint16_t  MajorSubsystemVersion;
+uint16_t  MinorSubsystemVersion;
+uint32_t  Win32VersionValue;
+uint32_t  SizeOfImage;
+uint32_t  SizeOfHeaders;
+uint32_t  CheckSum;
+uint16_t  Subsystem;
+uint16_t  DllCharacteristics;
+uint64_t  SizeOfStackReserve;
+uint64_t  SizeOfStackCommit;
+uint64_t  SizeOfHeapReserve;
+uint64_t  SizeOfHeapCommit;
+uint32_t  LoaderFlags;
+uint32_t  NumberOfRvaAndSizes;
+IMAGE_DATA_DIRECTORY DataDirectory[IMAGE_NUMBEROF_DIRECTORY_ENTRIES];
 } __attribute__ ((packed)) IMAGE_OPTIONAL_HEADER64;
 
 typedef struct IMAGE_NT_HEADERS64 {
-  uint32_t Signature;
-  IMAGE_FILE_HEADER FileHeader;
-  IMAGE_OPTIONAL_HEADER64 OptionalHeader;
+uint32_t Signature;
+IMAGE_FILE_HEADER FileHeader;
+IMAGE_OPTIONAL_HEADER64 OptionalHeader;
 } __attribute__ ((packed)) IMAGE_NT_HEADERS64;
 
 typedef struct IMAGE_DEBUG_DIRECTORY {
-  uint32_t Characteristics;
-  uint32_t TimeDateStamp;
-  uint16_t MajorVersion;
-  uint16_t MinorVersion;
-  uint32_t Type;
-  uint32_t SizeOfData;
-  uint32_t AddressOfRawData;
-  uint32_t PointerToRawData;
+uint32_t Characteristics;
+uint32_t TimeDateStamp;
+uint16_t MajorVersion;
+uint16_t MinorVersion;
+uint32_t Type;
+uint32_t SizeOfData;
+uint32_t AddressOfRawData;
+uint32_t PointerToRawData;
 } __attribute__ ((packed)) IMAGE_DEBUG_DIRECTORY;
 
 #define IMAGE_DEBUG_TYPE_CODEVIEW   2
diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
index 53ded170618..0b04cba00e5 100644
--- a/contrib/elf2dmp/addrspace.c
+++ b/contrib/elf2dmp/addrspace.c
@@ -11,6 +11,7 @@
 static struct pa_block *pa_space_find_block(struct pa_space *ps, uint64_t pa)
 {
 size_t i;
+
 for (i = 0; i < ps->block_nr; i++) {
 if (ps->block[i].paddr <= pa &&
 pa <= ps->block[i].paddr + ps->block[i].size) {
diff --git

Re: [PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

2023-03-21 Thread Wu, Fei

On 3/21/2023 8:58 PM, liweiwei wrote:
> 
> On 2023/3/21 14:37, fei2...@intel.com wrote:
>> From: Fei Wu 
>>
>> Kernel needs to access user mode memory e.g. during syscalls, the window
>> is usually opened up for a very limited time through MSTATUS.SUM, the
>> overhead is too much if tlb_flush() gets called for every SUM change.
>> This patch saves addresses accessed when SUM=1, and flushs only these
>> pages when SUM changes to 0. If the buffer is not large enough to save
>> all the pages during SUM=1, it will fall back to tlb_flush when
>> necessary.
>>
>> The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
>> most of the time kernel accesses 1 or 2 pages, it's very rare to see
>> more than 4 pages accessed.
>>
>> It's not necessary to save/restore these new added status, as
>> tlb_flush() is always called after restore.
>>
>> Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
>> other syscalls benefit a lot from this one too.
>>
>> Signed-off-by: Fei Wu 
>> Reviewed-by: LIU Zhiwei 
>> ---
>>   target/riscv/cpu.h    |  4 
>>   target/riscv/cpu_helper.c |  7 +++
>>   target/riscv/csr.c    | 14 +-
>>   3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 638e47c75a..926dbce59f 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -383,6 +383,10 @@ struct CPUArchState {
>>   uint64_t kvm_timer_compare;
>>   uint64_t kvm_timer_state;
>>   uint64_t kvm_timer_frequency;
>> +
>> +#define MAX_CACHED_SUM_U_ADDR_NUM 4
>> +    uint64_t sum_u_count;
>> +    uint64_t sum_u_addr[MAX_CACHED_SUM_U_ADDR_NUM];
>>   };
>>     OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index f88c503cf4..5ad0418eb6 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -1068,6 +1068,13 @@ restart:
>>   (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
>>   *prot |= PAGE_WRITE;
>>   }
>> +    if ((pte & PTE_U) && (mode & PRV_S) &&
>> +    get_field(env->mstatus, MSTATUS_SUM)) {
>> +    if (env->sum_u_count < MAX_CACHED_SUM_U_ADDR_NUM) {
>> +    env->sum_u_addr[env->sum_u_count] = addr;
>> +    }
>> +    ++env->sum_u_count;
>> +    }
>>   return TRANSLATE_SUCCESS;
>>   }
>>   }
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index ab566639e5..74b7638c8a 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1246,9 +1246,21 @@ static RISCVException
>> write_mstatus(CPURISCVState *env, int csrno,
>>     /* flush tlb on mstatus fields that affect VM */
>>   if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
>> -    MSTATUS_MPRV | MSTATUS_SUM)) {
>> +    MSTATUS_MPRV)) {
>>   tlb_flush(env_cpu(env));
>> +    env->sum_u_count = 0;
>> +    } else if ((mstatus & MSTATUS_SUM) && !(val & MSTATUS_SUM)) {
>> +    if (env->sum_u_count > MAX_CACHED_SUM_U_ADDR_NUM) {
>> +    tlb_flush(env_cpu(env));
> 
> SUM seems only affect S mode TLB. Maybe we can only flush S mode TLB here.
> 
It's also in effect when MPRV=1 and MPP=S in M mode, we can only flush
the tlb of PRV_S and PRV_M.

Thanks,
Fei.

>> +    } else {
>> +    for (int i = 0; i < env->sum_u_count; ++i) {
>> +    tlb_flush_page_by_mmuidx(env_cpu(env),
>> env->sum_u_addr[i],
>> + 1 << PRV_S | 1 << PRV_M);
> 
> Similar case here.
> 
> Regards,
> 
> Weiwei Li
> 
>> +    }
>> +    }
>> +    env->sum_u_count = 0;
>>   }
>> +
>>   mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
>>   MSTATUS_SPP | MSTATUS_MPRV | MSTATUS_SUM |
>>   MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR |
>

[PULL 4/8] contrib/elf2dmp: move PE dir search to pe_get_data_dir_entry