Re: [PATCH qemu] hw: add ATmega16u4 and ATmega32u4 MCUs

[Michael Rolnik]

Reviewed-by: Michael Rolnik 

On Sun, May 14, 2023 at 12:54 AM ~rmsyn  wrote:

> From: rmsyn 
>
> Adds support for ATmega16u4 and ATmega32u4 MCU definitions.
>
> Defines interrupts, memory layout, and machine types for generic
> ATmega16u4 and ATmega32u4 MCUs.
>
> Signed-off-by: rmsyn 
> ---
>  hw/avr/arduino.c |  36 ++
>  hw/avr/atmega.c  | 122 +++
>  hw/avr/atmega.h  |   2 +
>  3 files changed, 160 insertions(+)
>
> diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
> index 48ef478346..be04e412e6 100644
> --- a/hw/avr/arduino.c
> +++ b/hw/avr/arduino.c
> @@ -129,6 +129,34 @@ static void arduino_mega2560_class_init(ObjectClass
> *oc, void *data)
>  amc->xtal_hz= 16 * 1000 * 1000; /* CSTCE16M0V53-R0 */
>  };
>
> +static void arduino_mega16u4_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
> +
> +/*
> + *
> https://ww1.microchip.com/downloads/en/devicedoc/atmel-7766-8-bit-avr-atmega16u4-32u4_datasheet.pdf
> + */
> +mc->desc= "Arduino Mega 16u4 (ATmega16u4)";
> +mc->alias   = "mega16u4";
> +amc->mcu_type   = TYPE_ATMEGA16U4_MCU;
> +amc->xtal_hz= 16 * 1000 * 1000; /* CSTCE16M0V53-R0 */
> +};
> +
> +static void arduino_mega32u4_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
> +
> +/*
> + *
> https://ww1.microchip.com/downloads/en/devicedoc/atmel-7766-8-bit-avr-atmega16u4-32u4_datasheet.pdf
> + */
> +mc->desc= "Arduino Mega 32u4 (ATmega32u4)";
> +mc->alias   = "mega32u4";
> +amc->mcu_type   = TYPE_ATMEGA32U4_MCU;
> +amc->xtal_hz= 16 * 1000 * 1000; /* CSTCE16M0V53-R0 */
> +};
> +
>  static const TypeInfo arduino_machine_types[] = {
>  {
>  .name  = MACHINE_TYPE_NAME("arduino-duemilanove"),
> @@ -146,6 +174,14 @@ static const TypeInfo arduino_machine_types[] = {
>  .name  = MACHINE_TYPE_NAME("arduino-mega-2560-v3"),
>  .parent= TYPE_ARDUINO_MACHINE,
>  .class_init= arduino_mega2560_class_init,
> +}, {
> +.name  = MACHINE_TYPE_NAME("arduino-mega-16u4"),
> +.parent= TYPE_ARDUINO_MACHINE,
> +.class_init= arduino_mega16u4_class_init,
> +}, {
> +.name  = MACHINE_TYPE_NAME("arduino-mega-32u4"),
> +.parent= TYPE_ARDUINO_MACHINE,
> +.class_init= arduino_mega32u4_class_init,
>  }, {
>  .name   = TYPE_ARDUINO_MACHINE,
>  .parent = TYPE_MACHINE,
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index a34803e642..292ad9a447 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -27,6 +27,17 @@ enum AtmegaPeripheral {
>  GPIOG, GPIOH, GPIOI, GPIOJ, GPIOK, GPIOL,
>  USART0, USART1, USART2, USART3,
>  TIMER0, TIMER1, TIMER2, TIMER3, TIMER4, TIMER5,
> +RESET,
> +INT0, INT1, INT2, INT3, INT4, INT5, INT6,
> +PCINT0,
> +USB_GEN, USB_EP,
> +WDT,
> +SPI,
> +ANALOG_COMP,
> +ADC,
> +EE_READY,
> +TWI,
> +SPM_READY,
>  PERIFMAX
>  };
>
> @@ -98,6 +109,30 @@ static const peripheral_cfg dev168_328[PERIFMAX] = {
>  [GPIOC] = {  0x26 },
>  [GPIOB] = {  0x23 },
>  [GPIOA] = {  0x20 },
> +}, dev16u4_32u4[PERIFMAX] = {
> +[POWER1]= {  0x65 },
> +[POWER0]= {  0x64 },
> +[TIMER4]= {  0x4c, POWER1, 4, 0x72, 0x39, true },
> +[SPM_READY] = {  0x4a },
> +[TWI]   = {  0x48 },
> +[TIMER3]= {  0x3e, POWER1, 3, 0x71, 0x38, true },
> +[EE_READY]  = {  0x3c },
> +[ADC]   = {  0x3a },
> +[ANALOG_COMP]   = {  0x38 },
> +[USART1]= {  0x32, POWER1, 0 },
> +[SPI]   = {  0x30 },
> +[TIMER0]= {  0x2a, POWER0, 5, 0x6e, 0x35, true },
> +[TIMER1]= {  0x20, POWER0, 3, 0x6f, 0x36, true },
> +[WDT]   = {  0x18 },
> +[USB_GEN]   = {  0x14 },
> +[USB_EP]= {  0x16 },
> +[PCINT0]= {  0x12 },
> +[INT6]  = {  0x0e },
> +[INT3]  = {  0x08 },
> +[INT2]  = {  0x06 },
> +[INT1]  = {  0x04 },
> +[INT0]  = {  0x02 },
> +[RESET] = {  0x00 },
>  };
>
>  enum AtmegaIrq {
> @@ -117,6 +152,17 @@ enum AtmegaIrq {
>  TIMER4_COMPC_IRQ, TIMER4_OVF_IRQ,
>  TIMER5_CAPT_IRQ, TIMER5_COMPA_IRQ, TIMER5_COMPB_IRQ,
>  TIMER5_COMPC_IRQ, TIMER5_OVF_IRQ,
> +RESET_IRQ,
> +INT0_IRQ, INT1_IRQ, INT2_IRQ, INT3_IRQ, INT4_IRQ, INT5_IRQ, INT6_IRQ,
> +PCINT0_IRQ,
> +USB_GEN_IRQ, USB_EP_IRQ,
> +WDT_IRQ,
> +SPI_IRQ,
> +ANALOG_COMP_IRQ,
> +ADC_IRQ,
> +EE_READY_IRQ,
> +TWI_IRQ,
> +SPM_READY_IRQ,
>  IRQ_COUNT
>  };
>
> @@ -184,6 +230,44 @@ static const u

Re: [REPOST PATCH v3 5/5] amd_iommu: report x2APIC support to the operating system

[Bui Quang Minh]

On 5/12/23 21:39, Michael S. Tsirkin wrote:

On Tue, Apr 11, 2023 at 09:24:40PM +0700, Bui Quang Minh wrote:

This commit adds XTSup configuration to let user choose to whether enable
this feature or not. When XTSup is enabled, additional bytes in IRTE with
enabled guest virtual VAPIC are used to support 32-bit destination id.

Additionally, this commit changes to use IVHD type 0x11 in ACPI table for
feature report to operating system. This is because Linux does not use
XTSup in IOMMU Feature Reporting field of IVHD type 0x10 but only use XTSup
bit in EFR Register Image of IVHD 0x11 to indicate x2APIC support (see
init_iommu_one in linux/drivers/iommu/amd/init.c)

Signed-off-by: Bui Quang Minh 


I'm concerned that switching to type 11 will break some older guests.
It would be better if we could export both type 10 and type 11
ivhd. A question however would be how does this interact
with older guests. For example:
https://lists.linuxfoundation.org/pipermail/iommu/2016-January/015310.html
it looks like linux before 2016 only expected one ivhd entry?


Export both type 0x10 and 0x11 looks reasonable to me. Before the above 
commit, I see that Linux still loops through multiple ivhd but only 
handles one with type 0x10. On newer kernel, it will choose to handle 
the type that appears last corresponding the first devid, which is weird 
in my opinion.


+static u8 get_highest_supported_ivhd_type(struct acpi_table_header *ivrs)
+{
+   u8 *base = (u8 *)ivrs;
+   struct ivhd_header *ivhd = (struct ivhd_header *)
+   (base + IVRS_HEADER_LENGTH);
+   u8 last_type = ivhd->type;
+   u16 devid = ivhd->devid;
+
+   while (((u8 *)ivhd - base < ivrs->length) &&
+  (ivhd->type <= ACPI_IVHD_TYPE_MAX_SUPPORTED)) {
+   u8 *p = (u8 *) ivhd;
+
+   if (ivhd->devid == devid)
+   last_type = ivhd->type;
+   ivhd = (struct ivhd_header *)(p + ivhd->length);
+   }
+
+   return last_type;
+}

So when exposing type 0x10 following by 0x11, old kernel only parses 
0x10 and does not support x2APIC while new kernel parse 0x11 and support 
x2APIC. I will expose both types in the next version.



Some research and testing here would be benefitial.
Similarly for windows guests.

Thanks!




---
  hw/i386/acpi-build.c | 28 ++--
  hw/i386/amd_iommu.c  | 21 +++--
  hw/i386/amd_iommu.h  | 16 +++-
  3 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ec857a117e..72d6bb2892 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2339,7 +2339,7 @@ static void
  build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
  const char *oem_table_id)
  {
-int ivhd_table_len = 24;
+int ivhd_table_len = 40;
  AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
  GArray *ivhd_blob = g_array_new(false, true, 1);
  AcpiTable table = { .sig = "IVRS", .rev = 1, .oem_id = oem_id,
@@ -2349,18 +2349,19 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, 
const char *oem_id,
  /* IVinfo - IO virtualization information common to all
   * IOMMU units in a system
   */
-build_append_int_noprefix(table_data, 40UL << 8/* PASize */, 4);
+build_append_int_noprefix(table_data,
+ (1UL << 0) | /* EFRSup */
+ (40UL << 8), /* PASize */
+ 4);
  /* reserved */
  build_append_int_noprefix(table_data, 0, 8);
  
-/* IVHD definition - type 10h */

-build_append_int_noprefix(table_data, 0x10, 1);
+/* IVHD definition - type 11h */
+build_append_int_noprefix(table_data, 0x11, 1);
  /* virtualization flags */
  build_append_int_noprefix(table_data,
   (1UL << 0) | /* HtTunEn  */
- (1UL << 4) | /* iotblSup */


btw this should have been iotlbsup?


- (1UL << 6) | /* PrefSup  */
- (1UL << 7),  /* PPRSup   */
+ (1UL << 4),  /* iotblSup */
   1);
  
  /*


hmm why are you removing these other flags?


According to the AMD IOMMU specification, the bit 6, 7 are reserved in 
type 0x11 which are PerfSup, PPRSup respectively in type 0x10 so I 
remove those flags when changing to type 0x11. In type 0x11, these 
feature are reported via the below EFR Register Image I believe.





@@ -2404,13 +2405,12 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, 
const char *oem_id,
  build_append_int_noprefix(table_data, 0, 2);
  /* IOMMU info */
  build_append_int_noprefix(table_data, 0, 2);
-/* IOMMU Feature Reporting */
-build_append_int_noprefix(table_data,
- (48UL << 30) | /* HATS   */
-

Re: [PATCH 1/2] target/arm: Saturate L2CTLR_EL1 core count field rather than overflowing

[Richard Henderson]

On 5/12/23 10:02, Peter Maydell wrote:

The IMPDEF sysreg L2CTLR_EL1 found on the Cortex-A35, A53, A57, A72
and which we (arguably dubiously) also provide in '-cpu max' has a
2 bit field for the number of processors in the cluster. On real
hardware this must be sufficient because it can only be configured
with up to 4 CPUs in the cluster. However on QEMU if the board code
does not explicitly configure the code into clusters with the right
CPU count we default to "give the value assuming that all CPUs in
the system are in a single cluster", which might be too big to fit
in the field.

Instead of just overflowing this 2-bit field, saturate to 3 (meaning
"4 CPUs", so at least we don't overwrite other fields in the register.
It's unlikely that any guest code really cares about the value in
this field; at least, if it does it probably also wants the system
to be more closely matching real hardware, i.e. not to have more
than 4 CPUs.

This issue has been present since the L2CTLR was first added in
commit 377a44ec8f2fac5b back in 2014. It was only noticed because
Coverity complains (CID 1509227) that the shift might overflow 32 bits
and inadvertently sign extend into the top half of the 64 bit value.

Signed-off-by: Peter Maydell
---
  target/arm/cortex-regs.c | 11 +--
  1 file changed, 9 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 2/2] hw/arm/vexpress: Avoid trivial memory leak of 'flashalias'

[Richard Henderson]

On 5/12/23 10:02, Peter Maydell wrote:

In the vexpress board code, we allocate a new MemoryRegion at the top
of vexpress_common_init() but only set it up and use it inside the
"if (map[VE_NORFLASHALIAS] != -1)" conditional, so we leak it if not.
This isn't a very interesting leak as it's a tiny amount of memory
once at startup, but it's easy to fix.

We could silence Coverity simply by moving the g_new() into the
if() block, but this use of g_new(MemoryRegion, 1) is a legacy from
when this board model was originally written; we wouldn't do that
if we wrote it today. The MemoryRegions are conceptually a part of
the board and must not go away until the whole board is done with
(at the end of the simulation), so they belong in its state struct.

This machine already has a VexpressMachineState struct that extends
MachineState, so statically put the MemoryRegions in there instead of
dynamically allocating them separately at runtime.

Spotted by Coverity (CID 1509083).

Signed-off-by: Peter Maydell
---
  hw/arm/vexpress.c | 40 
  1 file changed, 20 insertions(+), 20 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 3/3] hw/intc: Add NULL pointer check on LoongArch ipi device

[Richard Henderson]

On 5/12/23 03:04, Song Gao wrote:

When ipi mailbox is used, cpu_index is decoded from iocsr register.
cpu maybe does not exist. This patch adss NULL pointer check on
ipi device.

Signed-off-by: Song Gao
---
  hw/intc/loongarch_ipi.c | 40 +---
  hw/intc/trace-events|  1 +
  2 files changed, 30 insertions(+), 11 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 1/4] hw/timer/i8254_common: Share "iobase" property via base class

[Mark Cave-Ayland]

On 13/05/2023 11:09, Bernhard Beschow wrote:


Both TYPE_KVM_I8254 and TYPE_I8254 have their own but same implementation of
the "iobase" property. The storage for the property already resides in
PITCommonState, so also move the property definition there.

Signed-off-by: Bernhard Beschow 
---
  hw/i386/kvm/i8254.c | 1 -
  hw/timer/i8254.c| 6 --
  hw/timer/i8254_common.c | 6 ++
  3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
index 191a26fa57..6a7383d877 100644
--- a/hw/i386/kvm/i8254.c
+++ b/hw/i386/kvm/i8254.c
@@ -301,7 +301,6 @@ static void kvm_pit_realizefn(DeviceState *dev, Error 
**errp)
  }
  
  static Property kvm_pit_properties[] = {

-DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
  DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", KVMPITState,
 lost_tick_policy, LOST_TICK_POLICY_DELAY),
  DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/timer/i8254.c b/hw/timer/i8254.c
index c8388ea432..c235496fc9 100644
--- a/hw/timer/i8254.c
+++ b/hw/timer/i8254.c
@@ -350,11 +350,6 @@ static void pit_realizefn(DeviceState *dev, Error **errp)
  pc->parent_realize(dev, errp);
  }
  
-static Property pit_properties[] = {

-DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
-DEFINE_PROP_END_OF_LIST(),
-};
-
  static void pit_class_initfn(ObjectClass *klass, void *data)
  {
  PITClass *pc = PIT_CLASS(klass);
@@ -366,7 +361,6 @@ static void pit_class_initfn(ObjectClass *klass, void *data)
  k->get_channel_info = pit_get_channel_info_common;
  k->post_load = pit_post_load;
  dc->reset = pit_reset;
-device_class_set_props(dc, pit_properties);
  }
  
  static const TypeInfo pit_info = {

diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index 050875b497..e4093e2904 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -240,6 +240,11 @@ static const VMStateDescription vmstate_pit_common = {
  }
  };
  
+static Property pit_common_properties[] = {

+DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
  static void pit_common_class_init(ObjectClass *klass, void *data)
  {
  DeviceClass *dc = DEVICE_CLASS(klass);
@@ -252,6 +257,7 @@ static void pit_common_class_init(ObjectClass *klass, void 
*data)
   * done by board code.
   */
  dc->user_creatable = false;
+device_class_set_props(dc, pit_common_properties);
  }
  
  static const TypeInfo pit_common_type = {


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 2/4] hw/arm/omap: Remove unused omap_uart_attach()

[Mark Cave-Ayland]

On 13/05/2023 11:09, Bernhard Beschow wrote:


The function is unused since commit
bdad3654d3c55f478e538037d9eccd204e5fc8ee ('hw/arm/nseries: Remove
invalid/unnecessary n8x0_uart_setup()').

Signed-off-by: Bernhard Beschow 
---
  include/hw/arm/omap.h | 1 -
  hw/char/omap_uart.c   | 9 -
  2 files changed, 10 deletions(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index c275d9b681..067e9419f7 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -724,7 +724,6 @@ struct omap_uart_s *omap2_uart_init(MemoryRegion *sysmem,
  qemu_irq txdma, qemu_irq rxdma,
  const char *label, Chardev *chr);
  void omap_uart_reset(struct omap_uart_s *s);
-void omap_uart_attach(struct omap_uart_s *s, Chardev *chr);
  
  struct omap_mpuio_s;

  qemu_irq *omap_mpuio_in_get(struct omap_mpuio_s *s);
diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c
index 1c890b9201..6848bddb4e 100644
--- a/hw/char/omap_uart.c
+++ b/hw/char/omap_uart.c
@@ -175,12 +175,3 @@ struct omap_uart_s *omap2_uart_init(MemoryRegion *sysmem,
  
  return s;

  }
-
-void omap_uart_attach(struct omap_uart_s *s, Chardev *chr)
-{
-/* TODO: Should reuse or destroy current s->serial */
-s->serial = serial_mm_init(get_system_memory(), s->base, 2, s->irq,
-   omap_clk_getrate(s->fclk) / 16,
-   chr ?: qemu_chr_new("null", "null", NULL),
-   DEVICE_NATIVE_ENDIAN);
-}


A quick grep agrees, so:

Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 3/4] hw/char/parallel: Export TYPE_ISA_PARALLEL macro

[Mark Cave-Ayland]

On 13/05/2023 11:09, Bernhard Beschow wrote:


Rather than using a string literal which is prone to typos let's use a macro
instead which is caught by the compiler if mistyped.

Signed-off-by: Bernhard Beschow 
---
  include/hw/char/parallel.h | 2 ++
  hw/char/parallel-isa.c | 2 +-
  hw/char/parallel.c | 1 -
  hw/isa/isa-superio.c   | 3 ++-
  4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/hw/char/parallel.h b/include/hw/char/parallel.h
index 0a23c0f57e..29d2876d00 100644
--- a/include/hw/char/parallel.h
+++ b/include/hw/char/parallel.h
@@ -4,6 +4,8 @@
  #include "hw/isa/isa.h"
  #include "chardev/char.h"
  
+#define TYPE_ISA_PARALLEL "isa-parallel"

+


We don't really want to separate the QOM TYPE_* constant macro and the OBJECT_* 
declaration macro.


It looks like the real issue is that ParallelState should be in 
include/hw/char/parallel.h, and ISAParallelState along with its QOM macros should be 
in a new include/hw/char/parallel-isa.h header.


Once that is done then using the TYPE_ISA_PARALLEL QOM macro will "just work".


  void parallel_hds_isa_init(ISABus *bus, int n);
  
  bool parallel_mm_init(MemoryRegion *address_space,

diff --git a/hw/char/parallel-isa.c b/hw/char/parallel-isa.c
index 1ccbb96e70..547ae69304 100644
--- a/hw/char/parallel-isa.c
+++ b/hw/char/parallel-isa.c
@@ -21,7 +21,7 @@ static void parallel_init(ISABus *bus, int index, Chardev 
*chr)
  DeviceState *dev;
  ISADevice *isadev;
  
-isadev = isa_new("isa-parallel");

+isadev = isa_new(TYPE_ISA_PARALLEL);
  dev = DEVICE(isadev);
  qdev_prop_set_uint32(dev, "index", index);
  qdev_prop_set_chr(dev, "chardev", chr);
diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index af551e7864..3d32589bb3 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -93,7 +93,6 @@ typedef struct ParallelState {
  PortioList portio_list;
  } ParallelState;
  
-#define TYPE_ISA_PARALLEL "isa-parallel"

  OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
  
  struct ISAParallelState {

diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index c81bfe58ef..53b80de0ce 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -20,6 +20,7 @@
  #include "hw/isa/superio.h"
  #include "hw/qdev-properties.h"
  #include "hw/input/i8042.h"
+#include "hw/char/parallel.h"
  #include "hw/char/serial.h"
  #include "trace.h"
  
@@ -51,7 +52,7 @@ static void isa_superio_realize(DeviceState *dev, Error **errp)

  } else {
  name = g_strdup_printf("parallel%d", i);
  }
-isa = isa_new("isa-parallel");
+isa = isa_new(TYPE_ISA_PARALLEL);
  d = DEVICE(isa);
  qdev_prop_set_uint32(d, "index", i);
  if (k->parallel.get_iobase) {



ATB,

Mark.

Re: [PATCH 4/4] hw/isa/i82378: Remove unused "io" attribute

[Mark Cave-Ayland]

On 13/05/2023 11:09, Bernhard Beschow wrote:


The attribute isn't used since commit 5c9736789b79ea49cd236ac326f0a414f63b1015
"i82378: Cleanup implementation".

Signed-off-by: Bernhard Beschow 
---
  hw/isa/i82378.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index 5432ab5065..63e0857208 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -34,7 +34,6 @@ struct I82378State {
  
  qemu_irq cpu_intr;

  qemu_irq *isa_irqs_in;
-MemoryRegion io;
  };
  
  static const VMStateDescription vmstate_i82378 = {


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq()

[Mark Cave-Ayland]

On 13/05/2023 12:53, Bernhard Beschow wrote:


Am 27. April 2023 12:31:10 UTC schrieb Mark Cave-Ayland 
:

On 26/04/2023 19:25, Bernhard Beschow wrote:


Am 26. April 2023 11:33:40 UTC schrieb Mark Cave-Ayland 
:

On 22/04/2023 16:07, Bernhard Beschow wrote:


isa_get_irq() asks for an ISADevice which piix-ide doesn't provide.
Passing a NULL pointer works but causes the isabus global to be used
then. By fishing out TYPE_ISA_BUS from the QOM tree it is possible to
achieve the same as using isa_get_irq().

This is an alternative solution to commit 9405d87be25d 'hw/ide: Fix
crash when plugging a piix3-ide device into the x-remote machine' which
allows for cleaning up the ISA API while keeping PIIX IDE functions
user-createable.

Signed-off-by: Bernhard Beschow 
---
hw/ide/piix.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 6942b484f9..a3a15dc7db 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -104,7 +104,8 @@ static void piix_ide_reset(DeviceState *dev)
pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
}
-static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
+static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
+  Error **errp)
{
static const struct {
int iobase;
@@ -124,7 +125,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
 object_get_typename(OBJECT(d)), i);
return false;
}
-ide_bus_init_output_irq(&d->bus[i], isa_get_irq(NULL, 
port_info[i].isairq));
+ide_bus_init_output_irq(&d->bus[i],
+isa_bus_get_irq(isa_bus, port_info[i].isairq));


I don't think is the right solution here, since ultimately we want to move the 
IRQ routing out of the device itself and into the PCI-ISA bridge. I'd go for 
the same solution as you've done for VIA IDE in patch 2, i.e. update the PIIX 
interrupt handler to set the legacy_irqs in PCIIDEState, and then wire them up 
to the ISA IRQs 14 and 15 similar to as Phil as done in his patches:


The problem is user-creatable PIIX-IDE. IMO we should stick to our deprecation 
process before going this step because this will break it.


Thomas posted some links from previous discussions where it seems that this 
hack is still in use:

https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg00780.html
https://lists.gnu.org/archive/html/qemu-block/2021-04/msg00746.html

So it seems we can't even deprecate this, as it's working around missing 
functionality in q35 :(

Certainly it seems that we should add a check that will fail the machine if 
there is more than one -device piix3-ide on the command line, since I can't see 
that could ever work properly.

I'm leaning towards adding a device property that must be set to enabled in 
order for PIIX IDE realize() to succeed, leave it disabled by default and only 
enable it for the q35 machine. Does that seem like a reasonable solution?


I'd rather declare this to be out of scope of this series. First, this series 
contains a lot of material already. Second, this patch attempts to preserve 
current behavior.

This patch is actually a preparation for the next one. In the next patch the 
(non-obvious) check for presence of the ISABus get removed so we need this patch to 
preserve behavior. Otherwise machines without an ISA bus will crash if piix3-ide gets 
user-created. One machine that would crash is the "remote" machine IIRC.


Hmmm. At the moment we seem to have a circular dependency around all the various IDE 
tidy-up series around :/  If we decide that this series should go first then I prefer 
this solution to Phil's proposal which breaks the contract that 
qdev_connect_gpio_out() should always occur after realize.


Phil, how are things looking for your time re: these IDE changes - are you able to 
spend time looking at them?


Alternatively if you are happy for me to pick up the IDE stuff, pull everything 
together into a series proposal, and then submit the final PR then I'm happy to do 
that too.



Best regards,
Bernhard




https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-4-phi...@linaro.org/

https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-5-phi...@linaro.org/

This also reminds me, given that the first patch above is doing wiring in 
pc_init1() then we are still missing part of your tidy-up series :/


bmdma_init(&d->bus[i], &d->bmdma[i], d);
ide_bus_register_restart_cb(&d->bus[i]);
@@ -136,14 +138,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
{
PCIIDEState *d = PCI_IDE(dev);
uint8_t *pci_conf = dev->config;
+ISABus *isa_bus;
+bool ambiguous;
  pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
  bmdma_init_ops(d, &piix_bmdma_ops);
pci_register

Re: [PATCH 2/8] migration: Add precopy initial data handshake

[Cédric Le Goater]

Hello Avihai,


+static int loadvm_handle_initial_data_enable(MigrationIncomingState *mis)
+{
+InitialDataInfo buf;
+SaveStateEntry *se;
+ssize_t read_size;
+
+read_size = qemu_get_buffer(mis->from_src_file, (void *)&buf, sizeof(buf));
+if (read_size != sizeof(buf)) {
+error_report("%s: Could not get data buffer, read_size %ld, len %lu",


please use %zd and %zu for ssize_t.

Thanks,

C.

Resources on deeper understanding of Translation blocks

[Gautam Bhat]

Hi,

I am going through some translation code for existing targets.

I would like to know if there are any good resources on deeper
understanding of translation blocks? Also some advice on the best way
to read code related to translation in Qemu and trying it out maybe
using the debugger, printing etc? I am getting lost trying to make
sense of the translation code.

Thanks,
Gautam.

Re: [PATCH v4 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support

[Niek Linnenbank]

On Wed, May 10, 2023 at 12:30 PM  wrote:

> From: qianfan Zhao 
>
> Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU,
> and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3
> for In-Car Entertainment usage, A40i and A40pro are variants that
> differ in applicable temperatures range (industrial and military).
>
> Signed-off-by: qianfan Zhao 
>
Reviewed-by: Niek Linnenbank 


> ---
>  hw/arm/Kconfig |  10 +
>  hw/arm/allwinner-r40.c | 418 +
>  hw/arm/bananapi_m2u.c  | 129 ++
>  hw/arm/meson.build |   1 +
>  include/hw/arm/allwinner-r40.h | 110 +
>  5 files changed, 668 insertions(+)
>  create mode 100644 hw/arm/allwinner-r40.c
>  create mode 100644 hw/arm/bananapi_m2u.c
>  create mode 100644 include/hw/arm/allwinner-r40.h
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 2d7c457955..b7a84f6e3f 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -374,6 +374,16 @@ config ALLWINNER_H3
>  select USB_EHCI_SYSBUS
>  select SD
>
> +config ALLWINNER_R40
> +bool
> +default y if TCG && ARM
> +select ALLWINNER_A10_PIT
> +select SERIAL
> +select ARM_TIMER
> +select ARM_GIC
> +select UNIMP
> +select SD
> +
>  config RASPI
>  bool
>  default y if TCG && ARM
> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> new file mode 100644
> index 00..b743d64253
> --- /dev/null
> +++ b/hw/arm/allwinner-r40.c
> @@ -0,0 +1,418 @@
> +/*
> + * Allwinner R40/A40i/T3 System on Chip emulation
> + *
> + * Copyright (C) 2023 qianfan Zhao 
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "qemu/bswap.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "hw/char/serial.h"
> +#include "hw/misc/unimp.h"
> +#include "hw/usb/hcd-ehci.h"
> +#include "hw/loader.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/arm/allwinner-r40.h"
> +
> +/* Memory map */
> +const hwaddr allwinner_r40_memmap[] = {
> +[AW_R40_DEV_SRAM_A1]= 0x,
> +[AW_R40_DEV_SRAM_A2]= 0x4000,
> +[AW_R40_DEV_SRAM_A3]= 0x8000,
> +[AW_R40_DEV_SRAM_A4]= 0xb400,
> +[AW_R40_DEV_MMC0]   = 0x01c0f000,
> +[AW_R40_DEV_MMC1]   = 0x01c1,
> +[AW_R40_DEV_MMC2]   = 0x01c11000,
> +[AW_R40_DEV_MMC3]   = 0x01c12000,
> +[AW_R40_DEV_PIT]= 0x01c20c00,
> +[AW_R40_DEV_UART0]  = 0x01c28000,
> +[AW_R40_DEV_GIC_DIST]   = 0x01c81000,
> +[AW_R40_DEV_GIC_CPU]= 0x01c82000,
> +[AW_R40_DEV_GIC_HYP]= 0x01c84000,
> +[AW_R40_DEV_GIC_VCPU]   = 0x01c86000,
> +[AW_R40_DEV_SDRAM]  = 0x4000
> +};
> +
> +/* List of unimplemented devices */
> +struct AwR40Unimplemented {
> +const char *device_name;
> +hwaddr base;
> +hwaddr size;
> +};
> +
> +static struct AwR40Unimplemented r40_unimplemented[] = {
> +{ "d-engine",   0x0100, 4 * MiB },
> +{ "d-inter",0x0140, 128 * KiB },
> +{ "sram-c", 0x01c0, 4 * KiB },
> +{ "dma",0x01c02000, 4 * KiB },
> +{ "nfdc",   0x01c03000, 4 * KiB },
> +{ "ts", 0x01c04000, 4 * KiB },
> +{ "spi0",   0x01c05000, 4 * KiB },
> +{ "spi1",   0x01c06000, 4 * KiB },
> +{ "cs0",0x01c09000, 4 * KiB },
> +{ "keymem", 0x01c0a000, 4 * KiB },
> +{ "emac",   0x01c0b000, 4 * KiB },
> +{ "usb0-otg",   0x01c13000, 4 * KiB },
> +{ "usb0-host",  0x01c14000, 4 * KiB },
> +{ "crypto", 0x01c15000, 4 * KiB },
> +{ "spi2",   0x01c17000, 4 * KiB },
> +{ "sata",   0x01c18000, 4 * KiB },
> +{ "usb1-host",  0x01c19000, 4 * KiB },
> +{ "sid",0x01c1b000, 4 * KiB },
> +{ "usb2-host",  0x01c1c000, 4 * KiB },
> +{ "cs1",0x01c1d000, 4 * KiB },
> +{ "spi3",   0x01c1f000, 4 * KiB },
> +{ "ccu",0x01c2, 1 * KiB },
> +{ "rtc",0x01c20400, 1 * KiB },
> +{ "pio",0x01c20800, 1 * KiB },
> +{ "owa",0x01c21000, 1 * KiB },
> +{ "ac97",   0x01c21400, 1 * KiB },
> +{ "cir0",   0x01c21800, 1 * KiB },
> +{ "cir1",   0x01c21c00, 1 * KiB },
> + 

Re: [PATCH v4 02/11] hw/arm/allwinner-r40: add Clock Control Unit

[Niek Linnenbank]

On Wed, May 10, 2023 at 12:30 PM  wrote:

> From: qianfan Zhao 
>
> The CCU provides the registers to program the PLLs and the controls
> most of the clock generation, division, distribution, synchronization
> and gating.
>
> This commit adds support for the Clock Control Unit which emulates
> a simple read/write register interface.
>
> Signed-off-by: qianfan Zhao 
>
Reviewed-by: Niek Linnenbank 


> ---
>  hw/arm/allwinner-r40.c  |   8 +-
>  hw/misc/allwinner-r40-ccu.c | 209 
>  hw/misc/meson.build |   1 +
>  include/hw/arm/allwinner-r40.h  |   2 +
>  include/hw/misc/allwinner-r40-ccu.h |  65 +
>  5 files changed, 284 insertions(+), 1 deletion(-)
>  create mode 100644 hw/misc/allwinner-r40-ccu.c
>  create mode 100644 include/hw/misc/allwinner-r40-ccu.h
>
> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> index b743d64253..128c0ca470 100644
> --- a/hw/arm/allwinner-r40.c
> +++ b/hw/arm/allwinner-r40.c
> @@ -42,6 +42,7 @@ const hwaddr allwinner_r40_memmap[] = {
>  [AW_R40_DEV_MMC1]   = 0x01c1,
>  [AW_R40_DEV_MMC2]   = 0x01c11000,
>  [AW_R40_DEV_MMC3]   = 0x01c12000,
> +[AW_R40_DEV_CCU]= 0x01c2,
>  [AW_R40_DEV_PIT]= 0x01c20c00,
>  [AW_R40_DEV_UART0]  = 0x01c28000,
>  [AW_R40_DEV_GIC_DIST]   = 0x01c81000,
> @@ -80,7 +81,6 @@ static struct AwR40Unimplemented r40_unimplemented[] = {
>  { "usb2-host",  0x01c1c000, 4 * KiB },
>  { "cs1",0x01c1d000, 4 * KiB },
>  { "spi3",   0x01c1f000, 4 * KiB },
> -{ "ccu",0x01c2, 1 * KiB },
>  { "rtc",0x01c20400, 1 * KiB },
>  { "pio",0x01c20800, 1 * KiB },
>  { "owa",0x01c21000, 1 * KiB },
> @@ -253,6 +253,8 @@ static void allwinner_r40_init(Object *obj)
>  object_property_add_alias(obj, "clk1-freq", OBJECT(&s->timer),
>"clk1-freq");
>
> +object_initialize_child(obj, "ccu", &s->ccu, TYPE_AW_R40_CCU);
> +
>  for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
>  object_initialize_child(obj, mmc_names[i], &s->mmc[i],
>  TYPE_AW_SDHOST_SUN5I);
> @@ -367,6 +369,10 @@ static void allwinner_r40_realize(DeviceState *dev,
> Error **errp)
>  memory_region_add_subregion(get_system_memory(),
>  s->memmap[AW_R40_DEV_SRAM_A4],
> &s->sram_a4);
>
> +/* Clock Control Unit */
> +sysbus_realize(SYS_BUS_DEVICE(&s->ccu), &error_fatal);
> +sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccu), 0,
> s->memmap[AW_R40_DEV_CCU]);
> +
>  /* SD/MMC */
>  for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
>  qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->gic),
> diff --git a/hw/misc/allwinner-r40-ccu.c b/hw/misc/allwinner-r40-ccu.c
> new file mode 100644
> index 00..d82fee12db
> --- /dev/null
> +++ b/hw/misc/allwinner-r40-ccu.c
> @@ -0,0 +1,209 @@
> +/*
> + * Allwinner R40 Clock Control Unit emulation
> + *
> + * Copyright (C) 2023 qianfan Zhao 
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "hw/sysbus.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "hw/misc/allwinner-r40-ccu.h"
> +
> +/* CCU register offsets */
> +enum {
> +REG_PLL_CPUX_CTRL   = 0x,
> +REG_PLL_AUDIO_CTRL  = 0x0008,
> +REG_PLL_VIDEO0_CTRL = 0x0010,
> +REG_PLL_VE_CTRL = 0x0018,
> +REG_PLL_DDR0_CTRL   = 0x0020,
> +REG_PLL_PERIPH0_CTRL= 0x0028,
> +REG_PLL_PERIPH1_CTRL= 0x002c,
> +REG_PLL_VIDEO1_CTRL = 0x0030,
> +REG_PLL_SATA_CTRL   = 0x0034,
> +REG_PLL_GPU_CTRL= 0x0038,
> +REG_PLL_MIPI_CTRL   = 0x0040,
> +REG_PLL_DE_CTRL = 0x0048,
> +REG_PLL_DDR1_CTRL   = 0x004c,
> +REG_AHB1_APB1_CFG   = 0x0054,
> +REG_APB2_CFG= 0x0058,
> +REG_MMC0_CLK= 0x0088,
> +REG_MMC1_CLK= 0x008c,
> +REG_MMC2_CLK= 0x0090,
> +REG_MMC3_CLK= 0x0094,
> +REG_USBPHY_CFG  = 0x00cc,
> +REG_PLL_DDR_AUX = 0x00f0,
> +REG_DRAM_CFG= 0x00f4,
> + 

Re: [PATCH v4 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support

[Niek Linnenbank]

Hi Qianfan,


On Wed, May 10, 2023 at 12:30 PM  wrote:

> From: qianfan Zhao 
>
> Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU,
> and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3
> for In-Car Entertainment usage, A40i and A40pro are variants that
> differ in applicable temperatures range (industrial and military).
>
> Signed-off-by: qianfan Zhao 
> ---
>  hw/arm/Kconfig |  10 +
>  hw/arm/allwinner-r40.c | 418 +
>  hw/arm/bananapi_m2u.c  | 129 ++
>  hw/arm/meson.build |   1 +
>  include/hw/arm/allwinner-r40.h | 110 +
>  5 files changed, 668 insertions(+)
>  create mode 100644 hw/arm/allwinner-r40.c
>  create mode 100644 hw/arm/bananapi_m2u.c
>  create mode 100644 include/hw/arm/allwinner-r40.h
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 2d7c457955..b7a84f6e3f 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -374,6 +374,16 @@ config ALLWINNER_H3
>  select USB_EHCI_SYSBUS
>  select SD
>
> +config ALLWINNER_R40
> +bool
> +default y if TCG && ARM
> +select ALLWINNER_A10_PIT
> +select SERIAL
> +select ARM_TIMER
> +select ARM_GIC
> +select UNIMP
> +select SD
> +
>  config RASPI
>  bool
>  default y if TCG && ARM
> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> new file mode 100644
> index 00..b743d64253
> --- /dev/null
> +++ b/hw/arm/allwinner-r40.c
> @@ -0,0 +1,418 @@
> +/*
> + * Allwinner R40/A40i/T3 System on Chip emulation
> + *
> + * Copyright (C) 2023 qianfan Zhao 
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "qemu/bswap.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "hw/char/serial.h"
> +#include "hw/misc/unimp.h"
> +#include "hw/usb/hcd-ehci.h"
> +#include "hw/loader.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/arm/allwinner-r40.h"
> +
> +/* Memory map */
> +const hwaddr allwinner_r40_memmap[] = {
> +[AW_R40_DEV_SRAM_A1]= 0x,
> +[AW_R40_DEV_SRAM_A2]= 0x4000,
> +[AW_R40_DEV_SRAM_A3]= 0x8000,
> +[AW_R40_DEV_SRAM_A4]= 0xb400,
> +[AW_R40_DEV_MMC0]   = 0x01c0f000,
> +[AW_R40_DEV_MMC1]   = 0x01c1,
> +[AW_R40_DEV_MMC2]   = 0x01c11000,
> +[AW_R40_DEV_MMC3]   = 0x01c12000,
> +[AW_R40_DEV_PIT]= 0x01c20c00,
> +[AW_R40_DEV_UART0]  = 0x01c28000,
> +[AW_R40_DEV_GIC_DIST]   = 0x01c81000,
> +[AW_R40_DEV_GIC_CPU]= 0x01c82000,
> +[AW_R40_DEV_GIC_HYP]= 0x01c84000,
> +[AW_R40_DEV_GIC_VCPU]   = 0x01c86000,
> +[AW_R40_DEV_SDRAM]  = 0x4000
> +};
> +
> +/* List of unimplemented devices */
> +struct AwR40Unimplemented {
> +const char *device_name;
> +hwaddr base;
> +hwaddr size;
> +};
> +
> +static struct AwR40Unimplemented r40_unimplemented[] = {
> +{ "d-engine",   0x0100, 4 * MiB },
> +{ "d-inter",0x0140, 128 * KiB },
> +{ "sram-c", 0x01c0, 4 * KiB },
> +{ "dma",0x01c02000, 4 * KiB },
> +{ "nfdc",   0x01c03000, 4 * KiB },
> +{ "ts", 0x01c04000, 4 * KiB },
> +{ "spi0",   0x01c05000, 4 * KiB },
> +{ "spi1",   0x01c06000, 4 * KiB },
> +{ "cs0",0x01c09000, 4 * KiB },
> +{ "keymem", 0x01c0a000, 4 * KiB },
> +{ "emac",   0x01c0b000, 4 * KiB },
> +{ "usb0-otg",   0x01c13000, 4 * KiB },
> +{ "usb0-host",  0x01c14000, 4 * KiB },
> +{ "crypto", 0x01c15000, 4 * KiB },
> +{ "spi2",   0x01c17000, 4 * KiB },
> +{ "sata",   0x01c18000, 4 * KiB },
> +{ "usb1-host",  0x01c19000, 4 * KiB },
> +{ "sid",0x01c1b000, 4 * KiB },
> +{ "usb2-host",  0x01c1c000, 4 * KiB },
> +{ "cs1",0x01c1d000, 4 * KiB },
> +{ "spi3",   0x01c1f000, 4 * KiB },
> +{ "ccu",0x01c2, 1 * KiB },
> +{ "rtc",0x01c20400, 1 * KiB },
> +{ "pio",0x01c20800, 1 * KiB },
> +{ "owa",0x01c21000, 1 * KiB },
> +{ "ac97",   0x01c21400, 1 * KiB },
> +{ "cir0",   0x01c21800, 1 * KiB },
> +{ "cir1",   0x01c21c00, 1 * KiB },
> +{ "pcm0",   0

Re: [PATCH v4 03/11] hw: allwinner-r40: Complete uart devices

[Niek Linnenbank]

Hi Qianfan,


On Wed, May 10, 2023 at 12:30 PM  wrote:

> From: qianfan Zhao 
>
> R40 has eight UARTs, support both 16450 and 16550 compatible modes.
>
> Signed-off-by: qianfan Zhao 
> ---
>  hw/arm/allwinner-r40.c | 31 ---
>  include/hw/arm/allwinner-r40.h |  8 
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> index 128c0ca470..537a90b23d 100644
> --- a/hw/arm/allwinner-r40.c
> +++ b/hw/arm/allwinner-r40.c
> @@ -45,6 +45,13 @@ const hwaddr allwinner_r40_memmap[] = {
>  [AW_R40_DEV_CCU]= 0x01c2,
>  [AW_R40_DEV_PIT]= 0x01c20c00,
>  [AW_R40_DEV_UART0]  = 0x01c28000,
> +[AW_R40_DEV_UART1]  = 0x01c28400,
> +[AW_R40_DEV_UART2]  = 0x01c28800,
> +[AW_R40_DEV_UART3]  = 0x01c28c00,
> +[AW_R40_DEV_UART4]  = 0x01c29000,
> +[AW_R40_DEV_UART5]  = 0x01c29400,
> +[AW_R40_DEV_UART6]  = 0x01c29800,
> +[AW_R40_DEV_UART7]  = 0x01c29c00,
>

After adding the uarts to the memory map here, you should remove them from
the unimplemented array.

 [AW_R40_DEV_GIC_DIST]   = 0x01c81000,
>  [AW_R40_DEV_GIC_CPU]= 0x01c82000,
>  [AW_R40_DEV_GIC_HYP]= 0x01c84000,
> @@ -160,6 +167,10 @@ enum {
>  AW_R40_GIC_SPI_UART1 =  2,
>  AW_R40_GIC_SPI_UART2 =  3,
>  AW_R40_GIC_SPI_UART3 =  4,
>

Since you put the addition of UART1-7 in this patch, probably it makes
sense to have adding the lines 'AW_R40_GIC_SPI_UART1/2/3' also part of this
patch.

With the two above remarks resolved, the patch looks good to me.

Reviewed-by: Niek Linnenbank 

Regards,
Niek

> +AW_R40_GIC_SPI_UART4 = 17,
> +AW_R40_GIC_SPI_UART5 = 18,
> +AW_R40_GIC_SPI_UART6 = 19,
> +AW_R40_GIC_SPI_UART7 = 20,
>  AW_R40_GIC_SPI_TIMER0= 22,
>  AW_R40_GIC_SPI_TIMER1= 23,
>  AW_R40_GIC_SPI_MMC0  = 32,
> @@ -387,9 +398,23 @@ static void allwinner_r40_realize(DeviceState *dev,
> Error **errp)
>  }
>
>  /* UART0. For future clocktree API: All UARTS are connected to
> APB2_CLK. */
> -serial_mm_init(get_system_memory(), s->memmap[AW_R40_DEV_UART0], 2,
> -   qdev_get_gpio_in(DEVICE(&s->gic),
> AW_R40_GIC_SPI_UART0),
> -   115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
> +for (int i = 0; i < AW_R40_NUM_UARTS; i++) {
> +static const int uart_irqs[AW_R40_NUM_UARTS] = {
> +AW_R40_GIC_SPI_UART0,
> +AW_R40_GIC_SPI_UART1,
> +AW_R40_GIC_SPI_UART2,
> +AW_R40_GIC_SPI_UART3,
> +AW_R40_GIC_SPI_UART4,
> +AW_R40_GIC_SPI_UART5,
> +AW_R40_GIC_SPI_UART6,
> +AW_R40_GIC_SPI_UART7,
> +};
> +const hwaddr addr = s->memmap[AW_R40_DEV_UART0 + i];
> +
> +serial_mm_init(get_system_memory(), addr, 2,
> +   qdev_get_gpio_in(DEVICE(&s->gic), uart_irqs[i]),
> +   115200, serial_hd(i), DEVICE_NATIVE_ENDIAN);
> +}
>
>  /* Unimplemented devices */
>  for (i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
> diff --git a/include/hw/arm/allwinner-r40.h
> b/include/hw/arm/allwinner-r40.h
> index 3be9dc962b..959b5dc4e0 100644
> --- a/include/hw/arm/allwinner-r40.h
> +++ b/include/hw/arm/allwinner-r40.h
> @@ -41,6 +41,13 @@ enum {
>  AW_R40_DEV_CCU,
>  AW_R40_DEV_PIT,
>  AW_R40_DEV_UART0,
> +AW_R40_DEV_UART1,
> +AW_R40_DEV_UART2,
> +AW_R40_DEV_UART3,
> +AW_R40_DEV_UART4,
> +AW_R40_DEV_UART5,
> +AW_R40_DEV_UART6,
> +AW_R40_DEV_UART7,
>  AW_R40_DEV_GIC_DIST,
>  AW_R40_DEV_GIC_CPU,
>  AW_R40_DEV_GIC_HYP,
> @@ -70,6 +77,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AwR40State, AW_R40)
>   * which are currently emulated by the R40 SoC code.
>   */
>  #define AW_R40_NUM_MMCS 4
> +#define AW_R40_NUM_UARTS8
>
>  struct AwR40State {
>  /*< private >*/
> --
> 2.25.1
>
>

-- 
Niek Linnenbank

Re: [PATCH v4 04/11] hw: arm: allwinner-r40: Add i2c0 device

[Niek Linnenbank]

On Wed, May 10, 2023 at 12:30 PM  wrote:

> From: qianfan Zhao 
>
> TWI(i2c) is designed to be used as an interface between CPU host and the
> serial 2-Wire bus. It can support all standard 2-Wire transfer, can be
> operated in standard mode(100kbit/s) or fast-mode, supporting data rate
> up to 400kbit/s.
>
> Signed-off-by: qianfan Zhao 
>
Reviewed-by: Niek Linnenbank 

> ---
>  hw/arm/allwinner-r40.c | 11 ++-
>  include/hw/arm/allwinner-r40.h |  3 +++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> index 537a90b23d..4bc582630c 100644
> --- a/hw/arm/allwinner-r40.c
> +++ b/hw/arm/allwinner-r40.c
> @@ -52,6 +52,7 @@ const hwaddr allwinner_r40_memmap[] = {
>  [AW_R40_DEV_UART5]  = 0x01c29400,
>  [AW_R40_DEV_UART6]  = 0x01c29800,
>  [AW_R40_DEV_UART7]  = 0x01c29c00,
> +[AW_R40_DEV_TWI0]   = 0x01c2ac00,
>  [AW_R40_DEV_GIC_DIST]   = 0x01c81000,
>  [AW_R40_DEV_GIC_CPU]= 0x01c82000,
>  [AW_R40_DEV_GIC_HYP]= 0x01c84000,
> @@ -115,7 +116,6 @@ static struct AwR40Unimplemented r40_unimplemented[] =
> {
>  { "uart7",  0x01c29c00, 1 * KiB },
>  { "ps20",   0x01c2a000, 1 * KiB },
>  { "ps21",   0x01c2a400, 1 * KiB },
> -{ "twi0",   0x01c2ac00, 1 * KiB },
>  { "twi1",   0x01c2b000, 1 * KiB },
>  { "twi2",   0x01c2b400, 1 * KiB },
>  { "twi3",   0x01c2b800, 1 * KiB },
> @@ -167,6 +167,7 @@ enum {
>  AW_R40_GIC_SPI_UART1 =  2,
>  AW_R40_GIC_SPI_UART2 =  3,
>  AW_R40_GIC_SPI_UART3 =  4,
> +AW_R40_GIC_SPI_TWI0  =  7,
>  AW_R40_GIC_SPI_UART4 = 17,
>  AW_R40_GIC_SPI_UART5 = 18,
>  AW_R40_GIC_SPI_UART6 = 19,
> @@ -270,6 +271,8 @@ static void allwinner_r40_init(Object *obj)
>  object_initialize_child(obj, mmc_names[i], &s->mmc[i],
>  TYPE_AW_SDHOST_SUN5I);
>  }
> +
> +object_initialize_child(obj, "twi0", &s->i2c0, TYPE_AW_I2C_SUN6I);
>  }
>
>  static void allwinner_r40_realize(DeviceState *dev, Error **errp)
> @@ -416,6 +419,12 @@ static void allwinner_r40_realize(DeviceState *dev,
> Error **errp)
> 115200, serial_hd(i), DEVICE_NATIVE_ENDIAN);
>  }
>
> +/* I2C */
> +sysbus_realize(SYS_BUS_DEVICE(&s->i2c0), &error_fatal);
> +sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c0), 0,
> s->memmap[AW_R40_DEV_TWI0]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c0), 0,
> +   qdev_get_gpio_in(DEVICE(&s->gic),
> AW_R40_GIC_SPI_TWI0));
> +
>  /* Unimplemented devices */
>  for (i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
>  create_unimplemented_device(r40_unimplemented[i].device_name,
> diff --git a/include/hw/arm/allwinner-r40.h
> b/include/hw/arm/allwinner-r40.h
> index 959b5dc4e0..95366f4eee 100644
> --- a/include/hw/arm/allwinner-r40.h
> +++ b/include/hw/arm/allwinner-r40.h
> @@ -26,6 +26,7 @@
>  #include "hw/intc/arm_gic.h"
>  #include "hw/sd/allwinner-sdhost.h"
>  #include "hw/misc/allwinner-r40-ccu.h"
> +#include "hw/i2c/allwinner-i2c.h"
>  #include "target/arm/cpu.h"
>  #include "sysemu/block-backend.h"
>
> @@ -48,6 +49,7 @@ enum {
>  AW_R40_DEV_UART5,
>  AW_R40_DEV_UART6,
>  AW_R40_DEV_UART7,
> +AW_R40_DEV_TWI0,
>  AW_R40_DEV_GIC_DIST,
>  AW_R40_DEV_GIC_CPU,
>  AW_R40_DEV_GIC_HYP,
> @@ -89,6 +91,7 @@ struct AwR40State {
>  AwA10PITState timer;
>  AwSdHostState mmc[AW_R40_NUM_MMCS];
>  AwR40ClockCtlState ccu;
> +AWI2CState i2c0;
>  GICState gic;
>  MemoryRegion sram_a1;
>  MemoryRegion sram_a2;
> --
> 2.25.1
>
>

-- 
Niek Linnenbank

Re: [PATCH 16/18] tests/qtest/meson.build: Run the net filter tests only with default devices

[Paolo Bonzini]

Il ven 12 mag 2023, 14:41 Thomas Huth  ha scritto:

> These tests rely on a default NIC to be available. Skip them if we
> used the "--without-default-devices" configure option.
>
> Signed-off-by: Thomas Huth 
>

This is the only patch I have some qualms about, because it reduces
coverage in legitimate setups where the default NIC _is_ included in the
binary.

Still a lot better than before, but please add a FIXME here. We can perhaps
try to use QMP to check if the machines have a usable default NIC, and if
not skip the test, but this should not block the bulk of your work from
being merged.

So, apart from this issue, series

Acked-by: Paolo Bonzini 

(Since I have only skimmed the contents of the individual patches but liked
them enough—or disliked for this one...—to reply already).

Paolo


---
>  tests/qtest/meson.build | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 48cd35b5b2..8fec3103b5 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -35,9 +35,9 @@ qtests_cxl = \
>(config_all_devices.has_key('CONFIG_CXL') ? ['cxl-test'] : [])
>
>  qtests_filter = \
> -  (slirp.found() ? ['test-netfilter'] : []) + \
> -  (config_host.has_key('CONFIG_POSIX') ? ['test-filter-mirror'] : []) + \
> -  (config_host.has_key('CONFIG_POSIX') ? ['test-filter-redirector'] : [])
> +  (get_option('default_devices') and slirp.found() ? ['test-netfilter'] :
> []) + \
> +  (get_option('default_devices') and config_host.has_key('CONFIG_POSIX')
> ? ['test-filter-mirror'] : []) + \
> +  (get_option('default_devices') and config_host.has_key('CONFIG_POSIX')
> ? ['test-filter-redirector'] : [])
>
>  qtests_i386 = \
>(slirp.found() ? ['pxe-test'] : []) + \
> @@ -221,9 +221,7 @@ qtests_aarch64 = \
> 'migration-test']
>
>  qtests_s390x = \
> -  (slirp.found() ? ['pxe-test', 'test-netfilter'] : []) +
>  \
> -  (config_host.has_key('CONFIG_POSIX') ? ['test-filter-mirror'] : []) +
>\
> -  (config_host.has_key('CONFIG_POSIX') ? ['test-filter-redirector'] : [])
> + \
> +  qtests_filter + \
>['boot-serial-test',
> 'drive_del-test',
> 'device-plug-test',
> --
> 2.31.1
>
>

Re: [REPOST PATCH v3 5/5] amd_iommu: report x2APIC support to the operating system

[Michael S. Tsirkin]

On Sun, May 14, 2023 at 03:55:11PM +0700, Bui Quang Minh wrote:
> On 5/12/23 21:39, Michael S. Tsirkin wrote:
> > On Tue, Apr 11, 2023 at 09:24:40PM +0700, Bui Quang Minh wrote:
> > > This commit adds XTSup configuration to let user choose to whether enable
> > > this feature or not. When XTSup is enabled, additional bytes in IRTE with
> > > enabled guest virtual VAPIC are used to support 32-bit destination id.
> > > 
> > > Additionally, this commit changes to use IVHD type 0x11 in ACPI table for
> > > feature report to operating system. This is because Linux does not use
> > > XTSup in IOMMU Feature Reporting field of IVHD type 0x10 but only use 
> > > XTSup
> > > bit in EFR Register Image of IVHD 0x11 to indicate x2APIC support (see
> > > init_iommu_one in linux/drivers/iommu/amd/init.c)
> > > 
> > > Signed-off-by: Bui Quang Minh 
> > 
> > I'm concerned that switching to type 11 will break some older guests.
> > It would be better if we could export both type 10 and type 11
> > ivhd. A question however would be how does this interact
> > with older guests. For example:
> > https://lists.linuxfoundation.org/pipermail/iommu/2016-January/015310.html
> > it looks like linux before 2016 only expected one ivhd entry?
> 
> Export both type 0x10 and 0x11 looks reasonable to me. Before the above
> commit, I see that Linux still loops through multiple ivhd but only handles
> one with type 0x10. On newer kernel, it will choose to handle the type that
> appears last corresponding the first devid, which is weird in my opinion.
> +static u8 get_highest_supported_ivhd_type(struct acpi_table_header *ivrs)
> +{
> + u8 *base = (u8 *)ivrs;
> + struct ivhd_header *ivhd = (struct ivhd_header *)
> + (base + IVRS_HEADER_LENGTH);
> + u8 last_type = ivhd->type;
> + u16 devid = ivhd->devid;
> +
> + while (((u8 *)ivhd - base < ivrs->length) &&
> +(ivhd->type <= ACPI_IVHD_TYPE_MAX_SUPPORTED)) {
> + u8 *p = (u8 *) ivhd;
> +
> + if (ivhd->devid == devid)
> + last_type = ivhd->type;
> + ivhd = (struct ivhd_header *)(p + ivhd->length);
> + }
> +
> + return last_type;
> +}

Yes I don't get the logic here either.
Talk to kernel devs who wrote this?

commit 8c7142f56fedfc6824b5bca56fee1f443e01746b
Author: Suravee Suthikulpanit 
Date:   Fri Apr 1 09:05:59 2016 -0400

iommu/amd: Use the most comprehensive IVHD type that the driver can support

The IVRS in more recent AMD system usually contains multiple
IVHD block types (e.g. 0x10, 0x11, and 0x40) for each IOMMU.
The newer IVHD types provide more information (e.g. new features
specified in the IOMMU spec), while maintain compatibility with
the older IVHD type.

Having multiple IVHD type allows older IOMMU drivers to still function
(e.g. using the older IVHD type 0x10) while the newer IOMMU driver can use
the newer IVHD types (e.g. 0x11 and 0x40). Therefore, the IOMMU driver
should only make use of the newest IVHD type that it can support.

This patch adds new logic to determine the highest level of IVHD type
it can support, and use it throughout the to initialize the driver.
This requires adding another pass to the IVRS parsing to determine
appropriate IVHD type (see function get_highest_supported_ivhd_type())
before parsing the contents.

[Vincent: fix the build error of IVHD_DEV_ACPI_HID flag not found]

Signed-off-by: Wan Zongshun 
Signed-off-by: Suravee Suthikulpanit 
Signed-off-by: Joerg Roedel 



> 
> So when exposing type 0x10 following by 0x11, old kernel only parses 0x10
> and does not support x2APIC while new kernel parse 0x11 and support x2APIC.
> I will expose both types in the next version.
> 
> > Some research and testing here would be benefitial.
> > Similarly for windows guests.
> > 
> > Thanks!
> > 
> > 
> > 
> > > ---
> > >   hw/i386/acpi-build.c | 28 ++--
> > >   hw/i386/amd_iommu.c  | 21 +++--
> > >   hw/i386/amd_iommu.h  | 16 +++-
> > >   3 files changed, 44 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index ec857a117e..72d6bb2892 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -2339,7 +2339,7 @@ static void
> > >   build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char 
> > > *oem_id,
> > >   const char *oem_table_id)
> > >   {
> > > -int ivhd_table_len = 24;
> > > +int ivhd_table_len = 40;
> > >   AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
> > >   GArray *ivhd_blob = g_array_new(false, true, 1);
> > >   AcpiTable table = { .sig = "IVRS", .rev = 1, .oem_id = oem_id,
> > > @@ -2349,18 +2349,19 @@ build_amd_iommu(GArray *table_data, BIOSLinker 
> > > *linker, const char *oem_id,
> > >   /* IVinfo - IO virtualization information common

Re: [PATCH] virtio: Prepend "virtio" prefix in virtio_error

[Alex Bennée]

Maxim Kostin  writes:

> Rename virtio_error function to virtio_error_impl and wrap it with
> virtio_error macro, it adds the "virtio" prefix to the error message.
>
> Remove redundant "virtio" prefixes in virtio_error calls.
>
> Signed-off-by: Maxim Kostin 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/net: Move xilinx_ethlite.c to the target-independent source set

[Alistair Francis]

On Mon, May 8, 2023 at 10:04 PM Thomas Huth  wrote:
>
> Now that the tswap() functions are available for target-independent
> code, too, we can move xilinx_ethlite.c from specific_ss to softmmu_ss
> to avoid that we have to compile this file multiple times.
>
> Signed-off-by: Thomas Huth 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/net/xilinx_ethlite.c | 2 +-
>  hw/net/meson.build  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c
> index 99c22819ea..89f4f3b254 100644
> --- a/hw/net/xilinx_ethlite.c
> +++ b/hw/net/xilinx_ethlite.c
> @@ -25,7 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "qom/object.h"
> -#include "cpu.h" /* FIXME should not use tswap* */
> +#include "exec/tswap.h"
>  #include "hw/sysbus.h"
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
> diff --git a/hw/net/meson.build b/hw/net/meson.build
> index e2be0654a1..a7860c5efe 100644
> --- a/hw/net/meson.build
> +++ b/hw/net/meson.build
> @@ -43,7 +43,7 @@ softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: 
> files('npcm7xx_emc.c'))
>  softmmu_ss.add(when: 'CONFIG_ETRAXFS', if_true: files('etraxfs_eth.c'))
>  softmmu_ss.add(when: 'CONFIG_COLDFIRE', if_true: files('mcf_fec.c'))
>  specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_llan.c'))
> -specific_ss.add(when: 'CONFIG_XILINX_ETHLITE', if_true: 
> files('xilinx_ethlite.c'))
> +softmmu_ss.add(when: 'CONFIG_XILINX_ETHLITE', if_true: 
> files('xilinx_ethlite.c'))
>
>  softmmu_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('net_rx_pkt.c'))
>  specific_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-net.c'))
> --
> 2.31.1
>
>

Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world

[Alistair Francis]

On Mon, May 8, 2023 at 11:48 AM Li, Yong  wrote:
>
> Hi Alistair,
>
> Thanks for the information, what I'm doing is to implement the
> StandaloneMm and secure boot feature for RISC-V by following the ARM's way
>
> https://trustedfirmware-a.readthedocs.io/en/latest/components/secure-partition-manager-mm.html

That is something worth including in the commit message, to help
explain what your patch is trying to do.

>
> So here what I need from virt is actually the VIRT_SECURE_UART which
> will be delicately and isolated/used for secure world like it is in arm
> virt
>
> (the isolation could be controlled by riscv worldguard feature if qemu
> will support)
>
> Similar definition in ARM virt is
> https://github.com/qemu/qemu/blob/38441756b70eec5807b5f60dad11a93a91199866/hw/arm/virt.c#L142

The ARM implementation isn't the same as this patch though. The ARM
virt machine added a secure memory region and guarded the entire
change behind a flag.

You can see the below commit for more details on the ARM implementation

commit 3df708eb48180fcf11956b81fd6a036cd13ed5f1
Author: Peter Maydell 
Date:   Thu Jan 21 14:15:07 2016 +

   hw/arm/virt: add secure memory region and UART

   Add a secure memory region to the virt board, which is the
   same as the nonsecure memory region except that it also has
   a secure-only UART in it. This is only created if the
   board is started with the '-machine secure=on' property.

>
> I guess the secure uart should not be pass-through from the pcie, it
> would be more reasonable to make it a dedicated one in virt.c compared
> to the UART0 in normal world.

Why can't the secure world not use the existing UART and the
non-secure world use a PCIe UART?

>
>
> So sorry, I did not know the background and did not make it clear in the
> patch (it is not a second uart for normal world usage for vm,
> application and etc),

The patch does add a second UART for use by anyone though. It's not
only available to secure world

>
> It is an UART for secure world. I guess I can re-do the patch and change
> the VIRT_UART1 to VIRT_SECURE_UART  to make it clear.

It would also be worth pointing to documentation or a spec that
describes why having a second UART is important for secure world.

It's probably worth sending a v4 with a detailed commit message
describing why this patch is required. That should include details
about why a second UART is important for secure world. That helps the
patch get accepted in the first place, but also include useful
information for future users.

Alistair

>
> Please let me know if further comments. Thanks so much!
>
>
> On 2023/5/8 7:05, Alistair Francis wrote:
> > On Tue, Apr 25, 2023 at 5:36 PM Yong Li  wrote:
> >> The virt machine can have two UARTs and the second UART
> >> can be used by the secure payload, firmware or OS residing
> >> in secure world. Will include the UART device to FDT in a
> >> seperated patch.
> >>
> >> Signed-off-by: Yong Li 
> >> Reviewed-by: LIU Zhiwei 
> >> Reviewed-by: Philippe Mathieu-Daudé 
> > This has come up before (see
> > https://gitlab.com/qemu-project/qemu/-/issues/955) and we decided that
> > we don't want to add a second UART. If you would like a second one you
> > can attach it via PCIe.
> >
> > I think we need a really compelling reason to add another UART. There
> > was a push recently to move more towards a "PCIe board" where
> > everything is attached via PCIe, and this is going in the opposite
> > direction.
> >
> > Alistair
> >
> >> ---
> >>   hw/riscv/virt.c | 4 
> >>   include/hw/riscv/virt.h | 2 ++
> >>   2 files changed, 6 insertions(+)
> >>
> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> index 4e3efbee16..8e11c4b9b3 100644
> >> --- a/hw/riscv/virt.c
> >> +++ b/hw/riscv/virt.c
> >> @@ -88,6 +88,7 @@ static const MemMapEntry virt_memmap[] = {
> >>   [VIRT_APLIC_S] =  {  0xd00, APLIC_SIZE(VIRT_CPUS_MAX) },
> >>   [VIRT_UART0] ={ 0x1000, 0x100 },
> >>   [VIRT_VIRTIO] =   { 0x10001000,0x1000 },
> >> +[VIRT_UART1] ={ 0x10002000, 0x100 },
> >>   [VIRT_FW_CFG] =   { 0x1010,  0x18 },
> >>   [VIRT_FLASH] ={ 0x2000, 0x400 },
> >>   [VIRT_IMSIC_M] =  { 0x2400, VIRT_IMSIC_MAX_SIZE },
> >> @@ -1506,6 +1507,9 @@ static void virt_machine_init(MachineState *machine)
> >>   serial_mm_init(system_memory, memmap[VIRT_UART0].base,
> >>   0, qdev_get_gpio_in(DEVICE(mmio_irqchip), UART0_IRQ), 399193,
> >>   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> >> +serial_mm_init(system_memory, memmap[VIRT_UART1].base,
> >> +0, qdev_get_gpio_in(DEVICE(mmio_irqchip), UART1_IRQ), 399193,
> >> +serial_hd(1), DEVICE_LITTLE_ENDIAN);
> >>
> >>   sysbus_create_simple("goldfish_rtc", memmap[VIRT_RTC].base,
> >>   qdev_get_gpio_in(DEVICE(mmio_irqchip), RTC_IRQ));
> >> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> >> index e5c474b26e..8d2f8f22

[PATCH] i386: Add new CPU model EmeraldRapids

[Qian Wen]

Emerald Rapids (EMR) is the next generation of Xeon server processor
after Sapphire Rapids (SPR).

Curretly, regarding the feature set that can be exposed to guest, there
isn't any one new comparing with SPR cpu model, except that EMR has a
different model number.

Though it's practicable to define EMR as an alias of a new version of
SPR by only updating the model number and model name, it loses the
flexibility when new version of EMR cpu model are needed for adding new
features (that hasn't virtalized/supported by KVM yet).

So just add EMR as a standalone cpu model.

Signed-off-by: Qian Wen 
Reviewed-by: Xiaoyao Li 
---
 target/i386/cpu.c | 125 ++
 1 file changed, 125 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6576287e5b..4f2a894aa4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3598,6 +3598,131 @@ static const X86CPUDefinition builtin_x86_defs[] = {
 { /* end of list */ },
 },
 },
+{
+.name = "EmeraldRapids",
+.level = 0x20,
+.vendor = CPUID_VENDOR_INTEL,
+.family = 6,
+.model = 207,
+.stepping = 1,
+.features[FEAT_1_EDX] =
+CPUID_FP87 | CPUID_VME | CPUID_DE | CPUID_PSE | CPUID_TSC |
+CPUID_MSR | CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC |
+CPUID_SEP | CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV |
+CPUID_PAT | CPUID_PSE36 | CPUID_CLFLUSH | CPUID_MMX | CPUID_FXSR |
+CPUID_SSE | CPUID_SSE2,
+.features[FEAT_1_ECX] =
+CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSSE3 |
+CPUID_EXT_FMA | CPUID_EXT_CX16 | CPUID_EXT_PCID | CPUID_EXT_SSE41 |
+CPUID_EXT_SSE42 | CPUID_EXT_X2APIC | CPUID_EXT_MOVBE |
+CPUID_EXT_POPCNT | CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_AES |
+CPUID_EXT_XSAVE | CPUID_EXT_AVX | CPUID_EXT_F16C | 
CPUID_EXT_RDRAND,
+.features[FEAT_8000_0001_EDX] =
+CPUID_EXT2_SYSCALL | CPUID_EXT2_NX | CPUID_EXT2_PDPE1GB |
+CPUID_EXT2_RDTSCP | CPUID_EXT2_LM,
+.features[FEAT_8000_0001_ECX] =
+CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM | CPUID_EXT3_3DNOWPREFETCH,
+.features[FEAT_8000_0008_EBX] =
+CPUID_8000_0008_EBX_WBNOINVD,
+.features[FEAT_7_0_EBX] =
+CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_HLE |
+CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 |
+CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | CPUID_7_0_EBX_RTM |
+CPUID_7_0_EBX_AVX512F | CPUID_7_0_EBX_AVX512DQ |
+CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | CPUID_7_0_EBX_SMAP |
+CPUID_7_0_EBX_AVX512IFMA | CPUID_7_0_EBX_CLFLUSHOPT |
+CPUID_7_0_EBX_CLWB | CPUID_7_0_EBX_AVX512CD | CPUID_7_0_EBX_SHA_NI 
|
+CPUID_7_0_EBX_AVX512BW | CPUID_7_0_EBX_AVX512VL,
+.features[FEAT_7_0_ECX] =
+CPUID_7_0_ECX_AVX512_VBMI | CPUID_7_0_ECX_UMIP | CPUID_7_0_ECX_PKU 
|
+CPUID_7_0_ECX_AVX512_VBMI2 | CPUID_7_0_ECX_GFNI |
+CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
+CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
+CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57 |
+CPUID_7_0_ECX_RDPID | CPUID_7_0_ECX_BUS_LOCK_DETECT,
+.features[FEAT_7_0_EDX] =
+CPUID_7_0_EDX_FSRM | CPUID_7_0_EDX_SERIALIZE |
+CPUID_7_0_EDX_TSX_LDTRK | CPUID_7_0_EDX_AMX_BF16 |
+CPUID_7_0_EDX_AVX512_FP16 | CPUID_7_0_EDX_AMX_TILE |
+CPUID_7_0_EDX_AMX_INT8 | CPUID_7_0_EDX_SPEC_CTRL |
+CPUID_7_0_EDX_ARCH_CAPABILITIES | CPUID_7_0_EDX_SPEC_CTRL_SSBD,
+.features[FEAT_ARCH_CAPABILITIES] =
+MSR_ARCH_CAP_RDCL_NO | MSR_ARCH_CAP_IBRS_ALL |
+MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY | MSR_ARCH_CAP_MDS_NO |
+MSR_ARCH_CAP_PSCHANGE_MC_NO | MSR_ARCH_CAP_TAA_NO,
+.features[FEAT_XSAVE] =
+CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
+CPUID_XSAVE_XGETBV1 | CPUID_XSAVE_XSAVES | CPUID_D_1_EAX_XFD,
+.features[FEAT_6_EAX] =
+CPUID_6_EAX_ARAT,
+.features[FEAT_7_1_EAX] =
+CPUID_7_1_EAX_AVX_VNNI | CPUID_7_1_EAX_AVX512_BF16 |
+CPUID_7_1_EAX_FZRM | CPUID_7_1_EAX_FSRS | CPUID_7_1_EAX_FSRC,
+.features[FEAT_VMX_BASIC] =
+MSR_VMX_BASIC_INS_OUTS | MSR_VMX_BASIC_TRUE_CTLS,
+.features[FEAT_VMX_ENTRY_CTLS] =
+VMX_VM_ENTRY_LOAD_DEBUG_CONTROLS | VMX_VM_ENTRY_IA32E_MODE |
+VMX_VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL |
+VMX_VM_ENTRY_LOAD_IA32_PAT | VMX_VM_ENTRY_LOAD_IA32_EFER,
+.features[FEAT_VMX_EPT_VPID_CAPS] =
+MSR_VMX_EPT_EXECONLY |
+MSR_VMX_EPT_PAGE_WALK_LENGTH_4 | MSR_VMX_EPT_PAGE_WALK_LENGTH_5 |
+MSR_VMX_EPT_WB | MSR_VMX_EPT_2MB | MSR_VMX_EPT_1GB |
+MSR_VMX_EPT_INVEPT 

Re: [PATCH] target: ppc: Correctly initialize HILE in HID-0 for book3s processors

[Nicholas Piggin]

On Sat Apr 29, 2023 at 12:30 AM AEST, Fabiano Rosas wrote:
> Vaibhav Jain  writes:
>
> > Hi Fabiano,
> >
> > Thanks for looking into this patch and apologies for the delayed reponse.
> > Fabiano Rosas  writes:
> >
> >> Narayana Murty N  writes:
> >>
> >>> On PPC64 the HILE(Hypervisor Interrupt Little Endian) bit in HID-0
> >>> register needs to be initialized as per isa 3.0b[1] section
> >>> 2.10. This bit gets copied to the MSR_LE when handling interrupts that
> >>> are handled in HV mode to establish the Endianess mode of the interrupt
> >>> handler.
> >>>
> >>> Qemu's ppc_interrupts_little_endian() depends on HILE to determine Host
> >>> endianness which is then used to determine the endianess of the guest 
> >>> dump.
> >>>
> >>
> >> Not quite. We use the interrupt endianness as a proxy to guest
> >> endianness to avoid reading MSR_LE at an inopportune moment when the
> >> guest is switching endianness.
> > Agreed
> >
> >> This is not dependent on host
> >> endianness. The HILE check is used when taking a memory dump of a
> >> HV-capable machine such as the emulated powernv.
> >
> > I think one concern which the patch tries to address is the guest 
> > memorydump file
> > generated of a BigEndian(BE) guest on a LittleEndian(LE) host is not 
> > readable on
> > the same LE host since 'crash' doesnt support cross endianess
> > dumps. Also even for a LE guest on LE host the memory dumps are marked as BE
> > making it not possible to analyze any guest memory dumps on the host.
> >
>
> From QEMU's perspective there's no "host" in this equation. We'll
> generate a BE dump for a BE guest and a LE dump for a LE guest. Anything
> different is a bug in QEMU (as the one this patch addresses).

I'm trying to figure out what's going on here. On one hand we are
creating a dump for/in the host. The dump is just a format that
describes register metadata, the same values can be represented just
fine with either endian. Memory has no endianness (without data
structures). So from that perspective, we do want to dump host endian
format.

OTOH crash could be taught foreign-endianness in which case the
endianness of the ELF file could be useful metadata about the
target I suppose. But ILE != MSR[LE] at any given time.

ILE seems like a half way house. It doesn't always give host endian
dumps so crash won't always work. It doesn't always give the machine
operating mode either. So why is it better to take guest ILE mode than
HV ILE mode?

I guess the first thing we need is a better and precise description of
the problem and the desired resolution. PPC64 has powernv and pseries,
both of which can support guests in various ways (PR, HV, nested HV),
and then when running guests the target itself also functions as a host,
so need to make all that unambiguous and use correct terminoogy in
the changelog.

> > However setting the HILE based on host endianess of qemu might not be
> > the right way to fix this problem. Based on an off mailing list discussion
> > with Narayana, he is working on another patch which doesnt set HILE
> > based on host endianess. However the problem seems to be stemming from
> > fact that qemu on KVM is using the HILE to set up the endianess of
> > memory-dump elf and since its not setup correctly the memory dumps are
> > in wrong endianess.
> >
> >> I think the actual issue might be that we're calling
> >> ppc_interrupts_little_endian with hv=true for the dump.
> >>
> > Yes, that is currently the case with cpu_get_dump_info(). Excerpt from
> > that function below that sets the endianess of the dump:
> >
> > if (ppc_interrupts_little_endian(cpu, cpu->env.has_hv_mode)) {
>
> This should probably be looking at cpu->vhyp or MSR_HVB since
> has_hv_mode will not change after we init the cpu.
>
> > info->d_endian = ELFDATA2LSB;
> > } else {
> > info->d_endian = ELFDATA2MSB;
> > }
> >
> > for pseries kvm guest cpu->env.has_hv_mode is already set hence
> > ppc_interrupts_little_endian() assumes its running in 'hv' mode. The new
> > patch from Narayana will be addressing this.
> >
> >>> Currently the HILE bit is never set in the HID0 register even if the
> >>> qemu is running in Little-Endian mode. This causes the guest dumps to be
> >>> always taken in Big-Endian byte ordering. A guest memory dump of a
> >>> Little-Endian guest running on Little-Endian qemu guest fails with the
> >>> crash tool as illustrated below:
> >>>
> >>
> >> Could you describe in more detail what is your setup? Specifically
> >> whether both guests are running TCG or KVM (info kvm) and the state of
> >> the nested-hv capability in QEMU command line.
> > Currently the issue is seen with any pseries KVM guest running on a PowerNV 
> > host.

Okay originally I thought you were talking about a powernv target
that is running a pseries guest and dumping that. But after re-reading, I
think you're talking about dumping a pseries target?

Questions still remain about why that's the best way to go. If the
target was running

Re: css_clear_io_interrupt() error handling

[Markus Armbruster]

Halil Pasic  writes:

> On Thu, 11 May 2023 14:20:51 +0200
> Markus Armbruster  wrote:
> [..]
>> >
>> > In my opinion the best way to deal with such situations would be to
>> > abort() in test/development and log a warning in production. Of course  
>> 
>> Understand, but...
>> 
>> > assert() wouldn't give me that, and it wouldn't be locally consistent at
>> > all.  
>> 
>> ... nothing behaves like that so far.
>> 
>
> I understand. And I agree with all statements from your previous mail. 
>
>> Let's try to come to a conclusion.  We can either keep the current
>> behavior, i.e. abort().  Or we change it to just print something.
>> 
>> If we want the latter: fprintf() to stderr, warn_report(), or trace
>> point?
>> 
>> You are the maintainer, so the decision is yours.
>> 
>> I could stick a patch into a series of error-related cleanup patches I'm
>> working on.
>
> I would gladly take that offer. Given that we didn't see any crashes and
> thus violations of assumptions up till now, and that both the kvm and the
> qemu implementations are from my perspective stable, I think not forcing
> a crash is a good option. From the options you offered, warn_report()
> looks the most compelling to me, but I would trust your expertise to pick
> the actually best one.
>
> Thank you very much.

You're welcome!

>> [*] I'm rather fond of the trick to have oopsie() fork & crash.
>
> I never thought of this, but I do actually find it very compelling
> to get a dump while keeping the workload alive. Especially if
> it was oopsie_once() so one does not get buried in dumps. But we don't
> do things like this in QEMU, or do we?

No, we don't.

Re: [PATCH v2 04/12] simpletrace: update code for Python 3.11

[Mads Ynddal]

> On 9 May 2023, at 16.38, Stefan Hajnoczi  wrote:
> 
> On Tue, May 02, 2023 at 11:23:31AM +0200, Mads Ynddal wrote:
>> From: Mads Ynddal 
>> 
>> The call to `getargspec` was deprecated and in Python 3.11 it has been
>> removed in favor of `getfullargspec`.
> 
> Please add that getfullargspec() is available in Python 3.6, the minimum
> Python version required by QEMU.
> 
> That makes it clear that its safe to merge this patch.
> 
> Otherwise:
> Reviewed-by: Stefan Hajnoczi 

I'll get this added.

Re: [PATCH v2 02/12] simpletrace: Annotate magic constants from QEMU code

[Mads Ynddal]

> 
> From my reply to v1 of this patch series:
> 
> This is fragile since this information will be outdated if the C source
> code changes (e.g. renaming files or variables).
> 
> Instead I would add the following comment:
> 
>  # This is the binary format that the QEMU "simple" trace backend
>  # emits. There is no specification documentation because the format is
>  # not guaranteed to be stable. Trace files must be parsed with the
>  # same trace-events-all file and the same simpletrace.py file that
>  # QEMU was built with.
> 
> I hope that clarifies the scope of the binary format and someone wishing
> to look into the format would then know to look at the "simple" trace
> backend.
> 
> Stefan


I must have missed that. I'll add this too.