Re: [PATCH v6 0/8] KVM: mm: fd-based approach for supporting KVM guest private memory

[Chao Peng]

On Mon, Jun 06, 2022 at 01:09:50PM -0700, Vishal Annapurve wrote:
> >
> > Private memory map/unmap and conversion
> > ---
> > Userspace's map/unmap operations are done by fallocate() ioctl on the
> > backing store fd.
> >   - map: default fallocate() with mode=0.
> >   - unmap: fallocate() with FALLOC_FL_PUNCH_HOLE.
> > The map/unmap will trigger above memfile_notifier_ops to let KVM map/unmap
> > secondary MMU page tables.
> >
> 
> >QEMU: https://github.com/chao-p/qemu/tree/privmem-v6
> >
> > An example QEMU command line for TDX test:
> > -object tdx-guest,id=tdx \
> > -object memory-backend-memfd-private,id=ram1,size=2G \
> > -machine 
> > q35,kvm-type=tdx,pic=no,kernel_irqchip=split,memory-encryption=tdx,memory-backend=ram1
> >
> 
> There should be more discussion around double allocation scenarios
> when using the private fd approach. A malicious guest or buggy
> userspace VMM can cause physical memory getting allocated for both
> shared (memory accessible from host) and private fds backing the guest
> memory.
> Userspace VMM will need to unback the shared guest memory while
> handling the conversion from shared to private in order to prevent
> double allocation even with malicious guests or bugs in userspace VMM.

I don't know how malicious guest can cause that. The initial design of
this serie is to put the private/shared memory into two different
address spaces and gives usersapce VMM the flexibility to convert
between the two. It can choose respect the guest conversion request or
not.

It's possible for a usrspace VMM to cause double allocation if it fails
to call the unback operation during the conversion, this may be a bug
or not. Double allocation may not be a wrong thing, even in conception.
At least TDX allows you to use half shared half private in guest, means
both shared/private can be effective. Unbacking the memory is just the
current QEMU implementation choice.

Chao
> 
> Options to unback shared guest memory seem to be:
> 1) madvise(.., MADV_DONTNEED/MADV_REMOVE) - This option won't stop
> kernel from backing the shared memory on subsequent write accesses
> 2) fallocate(..., FALLOC_FL_PUNCH_HOLE...) - For file backed shared
> guest memory, this option still is similar to madvice since this would
> still allow shared memory to get backed on write accesses
> 3) munmap - This would give away the contiguous virtual memory region
> reservation with holes in the guest backing memory, which might make
> guest memory management difficult.
> 4) mprotect(... PROT_NONE) - This would keep the virtual memory
> address range backing the guest memory preserved
> 
> ram_block_discard_range_fd from reference implementation:
> https://github.com/chao-p/qemu/tree/privmem-v6 seems to be relying on
> fallocate/madvise.
> 
> Any thoughts/suggestions around better ways to unback the shared
> memory in order to avoid double allocation scenarios?
> 
> Regards,
> Vishal

Re: [PATCH] avocado/boot_linux_console.py: Update ast2600 test

[Cédric Le Goater]

On 6/7/22 03:19, Joel Stanley wrote:

Update the test_arm_ast2600_debian test to

  - the latest Debian kernel
  - use the Rainier machine instead of Tacoma

Both of which contains support for more hardware and thus exercises more
of the hardware Qemu models.

Signed-off-by: Joel Stanley 


Reviewed-by: Cédric Le Goater 

Thanks,

C.


---
  tests/avocado/boot_linux_console.py | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/avocado/boot_linux_console.py 
b/tests/avocado/boot_linux_console.py
index 6b1533c17c8b..477b9b887754 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -1046,18 +1046,18 @@ def test_arm_vexpressa9(self):
  def test_arm_ast2600_debian(self):
  """
  :avocado: tags=arch:arm
-:avocado: tags=machine:tacoma-bmc
+:avocado: tags=machine:rainier-bmc
  """
  deb_url = ('http://snapshot.debian.org/archive/debian/'
-   '20210302T203551Z/'
+   '20220606T211338Z/'
 'pool/main/l/linux/'
-   'linux-image-5.10.0-3-armmp_5.10.13-1_armhf.deb')
-deb_hash = 
'db40d32fe39255d05482bea48d72467b67d6225bb2a2a4d6f618cb8976f1e09e'
+   'linux-image-5.17.0-2-armmp_5.17.6-1%2Bb1_armhf.deb')
+deb_hash = 
'8acb2b4439faedc2f3ed4bdb2847ad4f6e0491f73debaeb7f660c8abe4dcdc0e'
  deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash,
  algorithm='sha256')
-kernel_path = self.extract_from_deb(deb_path, 
'/boot/vmlinuz-5.10.0-3-armmp')
+kernel_path = self.extract_from_deb(deb_path, 
'/boot/vmlinuz-5.17.0-2-armmp')
  dtb_path = self.extract_from_deb(deb_path,
-
'/usr/lib/linux-image-5.10.0-3-armmp/aspeed-bmc-opp-tacoma.dtb')
+
'/usr/lib/linux-image-5.17.0-2-armmp/aspeed-bmc-ibm-rainier.dtb')
  
  self.vm.set_console()

  self.vm.add_args('-kernel', kernel_path,

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Geert Uytterhoeven]

CC arnd

On Sun, Jun 5, 2022 at 9:32 AM Stafford Horne  wrote:
> On Sun, Jun 05, 2022 at 10:58:14AM +0900, Stafford Horne wrote:
> > On Fri, Jun 03, 2022 at 09:05:09AM +0200, Geert Uytterhoeven wrote:
> > > On Thu, Jun 2, 2022 at 9:59 PM Stafford Horne  wrote:
> > > > On Thu, Jun 02, 2022 at 09:08:52PM +0200, Geert Uytterhoeven wrote:
> > > > > On Thu, Jun 2, 2022 at 1:42 PM Joel Stanley  wrote:
> > > > > > On Fri, 27 May 2022 at 17:27, Stafford Horne  
> > > > > > wrote:
> > > > > > > This patch add the OpenRISC virtual machine 'virt' for OpenRISC.  
> > > > > > > This
> > > > > > > platform allows for a convenient CI platform for toolchain, 
> > > > > > > software
> > > > > > > ports and the OpenRISC linux kernel port.
> > > > > > >
> > > > > > > Much of this has been sourced from the m68k and riscv virt 
> > > > > > > platforms.
> > > > >
> > > > > > I enabled the options:
> > > > > >
> > > > > > CONFIG_RTC_CLASS=y
> > > > > > # CONFIG_RTC_SYSTOHC is not set
> > > > > > # CONFIG_RTC_NVMEM is not set
> > > > > > CONFIG_RTC_DRV_GOLDFISH=y
> > > > > >
> > > > > > But it didn't work. It seems the goldfish rtc model doesn't handle a
> > > > > > big endian guest running on my little endian host.
> > > > > >
> > > > > > Doing this fixes it:
> > > > > >
> > > > > > -.endianness = DEVICE_NATIVE_ENDIAN,
> > > > > > +.endianness = DEVICE_HOST_ENDIAN,
> > > > > >
> > > > > > [0.19] goldfish_rtc 96005000.rtc: registered as rtc0
> > > > > > [0.19] goldfish_rtc 96005000.rtc: setting system clock to
> > > > > > 2022-06-02T11:16:04 UTC (1654168564)
> > > > > >
> > > > > > But literally no other model in the tree does this, so I suspect 
> > > > > > it's
> > > > > > not the right fix.
> > > > >
> > > > > Goldfish devices are supposed to be little endian.
> > > > > Unfortunately m68k got this wrong, cfr.
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e2ac4a3327479f7e2744cdd88a5c823f2057bad
> > > > > Please don't duplicate this bad behavior for new architectures
> > > >
> > > > Thanks for the pointer, I just wired in the goldfish RTC because I 
> > > > wanted to
> > > > play with it.  I was not attached to it. I can either remove it our 
> > > > find another
> > > > RTC.
> > >
> > > Sorry for being too unclear: the mistake was not to use the Goldfish
> > > RTC, but to make its register accesses big-endian.
> > > Using Goldfish devices as little-endian devices should be fine.
> >
> > OK, then I would think this patch would be needed on Goldfish.  I tested 
> > this
> > out and it seems to work:
>
> Sorry, it seems maybe I mis-understood this again.  In Arnd's mail [1] he, at
> the end, mentions.
>
> It might be a good idea to revisit the qemu implementation and make
> sure that the extra byteswap is only inserted on m68k and not on
> other targets, but hopefully there are no new targets based on goldfish
> anymore and we don't need to care.
>
> So, it seems that in addition to my patch we would need something in m68k to
> switch it back to 'native' (big) endian?
>
> Looking at the m68k kernel/qemu interface I see:
>
> Pre 5.19:
>(data) <-- kernel(readl / little) <-- m68k qemu (native / big) - RTC/PIC
>(data) <-- kernel(__raw_readl / big) <-- m68k qemu (native / big) - TTY
>
> 5.19:
>(data) <-- kernel(gf_ioread32 / big) <-- m68k qemu (native / big) - all
>
> The new fixes to add gf_ioread32/gf_iowrite32 fix this for goldfish and m68k.
> This wouldn't have been an issue for little-endian platforms where 
> readl/writel
> were originally used.
>
> Why can't m68k switch to little-endian in qemu and the kernel?  The m68k virt
> platform is not that old, 1 year? Are there a lot of users that this would be 
> a big
> problem?
>
> [1] 
> https://lore.kernel.org/lkml/CAK8P3a1oN8NrUjkh2X8jHQbyz42Xo6GSa=5n0gd6vqcxrjm...@mail.gmail.com/
>
> -Stafford
>
> > Patch:
> >
> > diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c
> > index 35e493be31..f1dc5af297 100644
> > --- a/hw/rtc/goldfish_rtc.c
> > +++ b/hw/rtc/goldfish_rtc.c
> > @@ -219,7 +219,7 @@ static int goldfish_rtc_post_load(void *opaque, int
> > version_id)
> >  static const MemoryRegionOps goldfish_rtc_ops = {
> >  .read = goldfish_rtc_read,
> >  .write = goldfish_rtc_write,
> > -.endianness = DEVICE_NATIVE_ENDIAN,
> > +.endianness = DEVICE_LITTLE_ENDIAN,
> >  .valid = {
> >  .min_access_size = 4,
> >  .max_access_size = 4
> >
> > Boot Log:
> >
> > io scheduler mq-deadline registered
> > io scheduler kyber registered
> > Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> > 9000.serial: ttyS0 at MMIO 0x9000 (irq = 2, base_baud = 
> > 125) is a 16550A
> > printk: console [ttyS0] enabled
> > loop: module loaded
> > virtio_blk virtio1: [vda] 32768 512-byte logical blocks (16.8 MB/16.0 
> > MiB)
> > Freeing initrd memory: 1696K
> >*goldfish_rtc 96005000.rtc: registered as rtc0
> >*goldfish

RE: [PATCH v3 0/4] xlnx-zcu102: fix the display port.

[Frederic Konrad]

> -Original Message-
> From: Peter Maydell 
> Sent: 06 June 2022 11:20
> To: Frederic Konrad 
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org;
> edgar.igles...@gmail.com; alist...@alistair23.me; Sai Pavan Boddu
> ; Edgar Iglesias ;
> fkon...@amd.com
> Subject: Re: [PATCH v3 0/4] xlnx-zcu102: fix the display port.
> 
> On Wed, 1 Jun 2022 at 18:24,  wrote:
> >
> > From: Frederic Konrad 
> >
> > Hi,
> >
> > This patch set fixes some issues with the DisplayPort for the ZCU102:
> >
> > The first patch fixes the wrong register size and thus the risk of register
> > overflow.
> >
> > The three other one add a vblank interrupt required by the linux driver:
> >   - When using the VNC graphic backend and leaving it unconnected, in the
> best
> > case the gfx_update callback is called once every 3000ms which is
> > insufficient for the driver.  This is fixed by providing a VBLANK 
> > interrupt
> > from a ptimer.
> >   - This requirement revealed two issues with the IRQ numbers and the
> > interrupt disable logic fixed by the two last patches.
> >
> > Tested by:
> >   - booting Petalinux with the framebuffer enabled.
> >   - migrating the running guest and ensure that the vblank timer still fire
> correctly.
> 
> Hi; you forgot to bump the version_id in the vmstate struct when you
> added the new field. Since that was the only problem, I've taken the
> series into target-arm.next and made that change there. I also added
> a note to the commit message that this is a migration break for the
> xlnx-zcu102 board.

Oops, thanks for the fix Peter.

Fred

> 
> thanks
> -- PMM

Re: [PATCH v2 08/16] ppc/pnv: user created pnv-phb for powernv9

[Frederic Barrat]

On 03/06/2022 23:00, Daniel Henrique Barboza wrote:

  static void pnv_phb4_realize(DeviceState *dev, Error **errp)
  {
  PnvPHB4 *phb = PNV_PHB4(dev);
+    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
+    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
  XiveSource *xsrc = &phb->xsrc;
+    BusState *s;
+    Error *local_err = NULL;
  int nr_irqs;
  char name[32];
+    if (!chip) {
+    error_setg(errp, "invalid chip id: %d", phb->chip_id);
+    return;
+    }
+
+    /* User created PHBs need to be assigned to a PEC */
+    if (!phb->pec) {
+    phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
+    if (local_err) {
+    error_propagate(errp, local_err);
+    return;
+    }
+    }
+
+    /* Reparent the PHB to the chip to build the device tree */
+    pnv_chip_parent_fixup(chip, OBJECT(phb->phb_base), phb->phb_id);



Didn't you mean to do that only for the user-created case? 


It's done for both because the default generated PHBs are being created 
loosely
from the chip starting on 3f4c369ea63e ("ppc/pnv: make PECs create and 
realize
PHB4s"). We had to amend the code after that commit to fix the QOM 
hierarchy
of these devices. 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM 
hierarchy") has

more details.


And why not attaching the PHB to the PEC instead of the chip, like in 
pnv_pec_default_phb_realize() ? I think it makes more sense to keep 
the chip->PEC->PHB parenting in the qom tree (and incidentally, that's 
where I'd prefer to have the phb3 model move to).


I can only speculate since I didn't participate in the initial design. My
educated guess is that we didn't want to show PECs in qtree, hence we
made the PHB a child of the chip instead. I'll also guess that this can be
due to how PHB3 worked and we just copied the existing design.



I don't believe that's correct though. As Cedric replied, the PECs show 
up in the qom tree on P9/P10, with chip->PEC->PHB, in that order. And I 
think that's fine, that's the model we should tend to (and which would 
require a fix on P8, since there we have chip->phb->pbcq, which is 
backwards. The pbcq object is the equivalent of the PEC).


So I think we should keep the qom relationship as it currently is on 
P9/P10, since it looks good. On P8, we can keep the current status for 
now and, as discussed privately, have another series later to clean 
things up.


  Fred

Re: [PATCH v2 1/1] Fix the coredump when memory backend id conflicts with default_ram_id

[Li Zhang]

Thanks Philippe.

Hi Igor,
Any comments about this patch?

On Wed, Jun 1, 2022 at 2:28 PM Philippe Mathieu-Daudé via
 wrote:
>
> Cc'ing Igor
>
> On Fri, May 20, 2022 at 11:56 AM Li Zhang  wrote:
> >
> > When no memory backend is specified in machine options,
> > a default memory device will be added with default_ram_id.
> > However, if a memory backend object is added in QEMU options
> > and id is the same as default_ram_id, a coredump happens.
> >
> > Command line:
> > qemu-system-x86_64 -name guest=vmtest,debug-threads=on \
> > -machine pc-q35-6.0,accel=kvm,usb=off,vmport=off \
> > -smp 16,sockets=16,cores=1,threads=1 \
> > -m 4G \
> > -object memory-backend-ram,id=pc.ram,size=4G \
> > -no-user-config -nodefaults -nographic
> >
> > Stack trace of thread 16903:
> > #0  0x7fb109a9318b raise (libc.so.6 + 0x3a18b)
> > #1  0x7fb109a94585 abort (libc.so.6 + 0x3b585)
> > #2  0x558c34bc89be error_handle_fatal (qemu-system-x86_64 + 
> > 0x9c89be)
> > #3  0x558c34bc8aee error_setv (qemu-system-x86_64 + 0x9c8aee)
> > #4  0x558c34bc8ccf error_setg_internal (qemu-system-x86_64 + 
> > 0x9c8ccf)
> > #5  0x558c349f6899 object_property_try_add (qemu-system-x86_64 + 
> > 0x7f6899)
> > #6  0x558c349f7df8 object_property_try_add_child 
> > (qemu-system-x86_64 + 0x7f7df8)
> > #7  0x558c349f7e91 object_property_add_child (qemu-system-x86_64 + 
> > 0x7f7e91)
> > #8  0x558c3454686d create_default_memdev (qemu-system-x86_64 + 
> > 0x34686d)
> > #9  0x558c34546f58 qemu_init_board (qemu-system-x86_64 + 0x346f58)
> > #10 0x558c345471b9 qmp_x_exit_preconfig (qemu-system-x86_64 + 
> > 0x3471b9)
> > #11 0x558c345497d9 qemu_init (qemu-system-x86_64 + 0x3497d9)
> > #12 0x558c344e54c2 main (qemu-system-x86_64 + 0x2e54c2)
> > #13 0x7fb109a7e34d __libc_start_main (libc.so.6 + 0x2534d)
> > #14 0x558c344e53ba _start (qemu-system-x86_64 + 0x2e53ba)
> >
> > Signed-off-by: Li Zhang 
> > ---
> >  hw/core/machine.c | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index b03d9192ba..3867af7a8a 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1290,9 +1290,17 @@ MemoryRegion *machine_consume_memdev(MachineState 
> > *machine,
> >  static bool create_default_memdev(MachineState *ms, const char *path, 
> > Error **errp)
> >  {
> >  Object *obj;
> > +ObjectProperty *prop;
> >  MachineClass *mc = MACHINE_GET_CLASS(ms);
> >  bool r = false;
> >
> > +prop = object_property_find(object_get_objects_root(), 
> > mc->default_ram_id);
> > +if (prop) {
> > +error_report("Memory backend id conflicts with default_ram_id %s",
> > + mc->default_ram_id);
> > +exit(EXIT_FAILURE);
> > +}
> > +
> >  obj = object_new(path ? TYPE_MEMORY_BACKEND_FILE : 
> > TYPE_MEMORY_BACKEND_RAM);
> >  if (path) {
> >  if (!object_property_set_str(obj, "mem-path", path, errp)) {
> > --
> > 2.34.1
> >
>


-- 

Best Regards
-Li

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Arnd Bergmann]

On Tue, Jun 7, 2022 at 10:11 AM Geert Uytterhoeven  wrote:
> On Sun, Jun 5, 2022 at 9:32 AM Stafford Horne  wrote:
> > On Sun, Jun 05, 2022 at 10:58:14AM +0900, Stafford Horne wrote:
> > It might be a good idea to revisit the qemu implementation and make
> > sure that the extra byteswap is only inserted on m68k and not on
> > other targets, but hopefully there are no new targets based on goldfish
> > anymore and we don't need to care.
> >
> > So, it seems that in addition to my patch we would need something in m68k to
> > switch it back to 'native' (big) endian?
> >
> > Looking at the m68k kernel/qemu interface I see:
> >
> > Pre 5.19:
> >(data) <-- kernel(readl / little) <-- m68k qemu (native / big) - RTC/PIC
> >(data) <-- kernel(__raw_readl / big) <-- m68k qemu (native / big) - TTY
> >
> > 5.19:
> >(data) <-- kernel(gf_ioread32 / big) <-- m68k qemu (native / big) - all
> >
> > The new fixes to add gf_ioread32/gf_iowrite32 fix this for goldfish and 
> > m68k.
> > This wouldn't have been an issue for little-endian platforms where 
> > readl/writel
> > were originally used.
> >
> > Why can't m68k switch to little-endian in qemu and the kernel?  The m68k 
> > virt
> > platform is not that old, 1 year? Are there a lot of users that this would 
> > be a big
> > problem?
> >
> > [1] 
> > https://lore.kernel.org/lkml/CAK8P3a1oN8NrUjkh2X8jHQbyz42Xo6GSa=5n0gd6vqcxrjm...@mail.gmail.com/

Goldfish is a very old platform, as far as I know only the kernel port is new.
I don't know when qemu started shipping goldfish, but changing it now would
surely break compatibility with whatever OS the port was originally made for.

  Arnd

Re: [PATCH 32/71] target/arm: Create ARMVQMap

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:21, Richard Henderson
 wrote:
>
> Pull the three sve_vq_* values into a structure.
> This will be reused for SME.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v2 03/16] ppc/pnv: add PnvPHB base/proxy device

[Frederic Barrat]

On 07/06/2022 08:42, Cédric Le Goater wrote:

On 6/2/22 18:16, Frederic Barrat wrote:



On 31/05/2022 23:49, Daniel Henrique Barboza wrote:

The PnvPHB device is going to be the base device for all other powernv
PHBs. It consists of a device that has the same user API as the other
PHB, namely being a PCIHostBridge and having chip-id and index
properties. It also has a 'backend' pointer that will be initialized
with the PHB implementation that the device is going to use.

The initialization of the PHB backend is done by checking the PHB
version via a 'version' attribute that can be set via a global machine
property.  The 'version' field will be used to make adjustments based on
the running version, e.g. PHB3 uses a 'chip' reference while PHB4 uses
'pec'. To init the PnvPHB bus we'll rely on helpers for each version.
The version 3 helper is already added (pnv_phb3_bus_init), the PHB4
helper will be added later on.

For now let's add the basic logic of the PnvPHB object, which consists
mostly of pnv_phb_realize() doing all the work of checking the
phb->version set, initializing the proper backend, passing through its
attributes to the chosen backend, finalizing the backend realize and
adding a root port in the end.

Signed-off-by: Daniel Henrique Barboza 
---
  hw/pci-host/meson.build |   3 +-
  hw/pci-host/pnv_phb.c   | 123 
  hw/pci-host/pnv_phb.h   |  39 +
  3 files changed, 164 insertions(+), 1 deletion(-)
  create mode 100644 hw/pci-host/pnv_phb.c
  create mode 100644 hw/pci-host/pnv_phb.h

diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
index c07596d0d1..e832babc9d 100644
--- a/hw/pci-host/meson.build
+++ b/hw/pci-host/meson.build
@@ -35,5 +35,6 @@ specific_ss.add(when: 'CONFIG_PCI_POWERNV', 
if_true: files(

    'pnv_phb3_msi.c',
    'pnv_phb3_pbcq.c',
    'pnv_phb4.c',
-  'pnv_phb4_pec.c'
+  'pnv_phb4_pec.c',
+  'pnv_phb.c',
  ))
diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
new file mode 100644
index 00..fa8472622f
--- /dev/null
+++ b/hw/pci-host/pnv_phb.c
@@ -0,0 +1,123 @@
+/*
+ * QEMU PowerPC PowerNV Proxy PHB model
+ *
+ * Copyright (c) 2022, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/visitor.h"
+#include "qapi/error.h"
+#include "hw/pci-host/pnv_phb.h"
+#include "hw/pci-host/pnv_phb3.h"
+#include "hw/pci-host/pnv_phb4.h"
+#include "hw/ppc/pnv.h"
+#include "hw/qdev-properties.h"
+#include "qom/object.h"
+
+
+static void pnv_phb_realize(DeviceState *dev, Error **errp)
+{
+    PnvPHB *phb = PNV_PHB(dev);
+    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
+    g_autofree char *phb_typename = NULL;
+    g_autofree char *phb_rootport_typename = NULL;
+
+    if (!phb->version) {
+    error_setg(errp, "version not specified");
+    return;
+    }
+
+    switch (phb->version) {
+    case 3:
+    phb_typename = g_strdup(TYPE_PNV_PHB3);
+    phb_rootport_typename = g_strdup(TYPE_PNV_PHB3_ROOT_PORT);
+    break;
+    case 4:
+    phb_typename = g_strdup(TYPE_PNV_PHB4);
+    phb_rootport_typename = g_strdup(TYPE_PNV_PHB4_ROOT_PORT);
+    break;
+    case 5:
+    phb_typename = g_strdup(TYPE_PNV_PHB5);
+    phb_rootport_typename = g_strdup(TYPE_PNV_PHB5_ROOT_PORT);
+    break;
+    default:
+    g_assert_not_reached();
+    }
+
+    phb->backend = object_new(phb_typename);
+    object_property_add_child(OBJECT(dev), "phb-device", phb->backend);
+
+    /* Passthrough child device properties to the proxy device */
+    object_property_set_uint(phb->backend, "index", phb->phb_id, errp);
+    object_property_set_uint(phb->backend, "chip-id", phb->chip_id, 
errp);
+    object_property_set_link(phb->backend, "phb-base", OBJECT(phb), 
errp);

+
+    if (phb->version == 3) {
+    object_property_set_link(phb->backend, "chip",
+ OBJECT(phb->chip), errp);
+    } else {
+    object_property_set_link(phb->backend, "pec", 
OBJECT(phb->pec), errp);

+    }



The patch is fine, but it just highlights that we're doing something 
wrong. I don't believe there's any reason for the chip/pec/phb 
relationship to be different between P8 and P9/P10. One day, a brave 
soul could try to unify the models, it would avoid test like that.


I think this is the first thing to do: introduce a PEC model to match
P8 HW and unify PHB3/4/5. We have been dragging this for too long (2014 
...)



I agree it's needed, but it's also orthogonal to this series. And I 
wouldn't force it on Daniel since we're getting into PCI territory.


  Fred




It shouldn't be that complex with all the cleanups that have been done.

Thanks,

C.


It would be a good cleanup series to do if we ever extend the model 
with yet another version :-)






+
+    if (!qdev_realize(DEVICE(phb->backend), NULL, errp)) {
+    return;
+   

Re: [PATCH 34/71] target/arm: Generalize cpu_arm_{get, set}_default_vec_len

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:10, Richard Henderson
 wrote:
>
> Rename from cpu_arm_{get,set}_sve_default_vec_len,
> and take the pointer to default_vq from opaque.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/cpu64.c | 27 ++-
>  1 file changed, 14 insertions(+), 13 deletions(-)
>

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 31/71] target/arm: Move error for sve%d property to arm_cpu_sve_finalize

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:10, Richard Henderson
 wrote:
>
> Keep all of the error messages together.  This does mean that
> when setting many sve length properties we'll only generate
> one error, but we only really need one.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/cpu64.c | 15 +++
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 33/71] target/arm: Generalize cpu_arm_{get,set}_vq

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:29, Richard Henderson
 wrote:
>
> Rename from cpu_arm_{get,set}_sve_vq, and take the
> ARMVQMap as the opaque parameter.
>
> Signed-off-by: Richard Henderson 
> ---
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 36/71] target/arm: Unexport aarch64_add_*_properties

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:17, Richard Henderson
 wrote:
>
> These functions are not used outside cpu64.c,
> so make them static.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/cpu.h   | 3 ---
>  target/arm/cpu64.c | 4 ++--
>  2 files changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 35/71] target/arm: Move arm_cpu_*_finalize to internals.h

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:12, Richard Henderson
 wrote:
>
> Drop the aa32-only inline fallbacks,
> and just use a couple of ifdefs.
>
> Signed-off-by: Richard Henderson 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v2 08/16] ppc/pnv: user created pnv-phb for powernv9

[Frederic Barrat]

On 07/06/2022 08:35, Cédric Le Goater wrote:


Also, the comment seems wrong to me. The qom parenting doesn't matter 
when building the device tree. 


it does. See pnv_dt_xscom()



Yeah, what I meant is that on P9, there's no "dt_scom" method for the 
PHB. The PHBs are added by the dt_scom() of the PEC. So the parenting of 
the PHB doesn't really matter.


I was actually wondering why it was done that way. If we have a clean 
qom tree (again, only on P9/P10 because P8 is wrong), then the PEC could 
add the "pbcq@xx" layer in the device tree, then call the qom 
children, i.e. the PHBs, and they would each add themselves (each phb 
adds the 'stack@xx' entry in the device tree).


But then I see your comment about giving headaches for user-created 
devices. So something else to discuss...


  Fred

Re: [PATCH v3 10/49] semihosting: Clean up common_semi_flen_cb

[Alex Bennée]

Richard Henderson  writes:

> Do not read from the gdb struct stat buffer if the callback is
> reporting an error. Use common_semi_cb to finish returning results.
>
> Signed-off-by: Richard Henderson 
> ---
>  semihosting/arm-compat-semi.c | 20 +++-
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 88e1c286ba..8792180974 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -346,15 +346,17 @@ static target_ulong common_semi_flen_buf(CPUState *cs)
>  static void
>  common_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
>  {
> -/* The size is always stored in big-endian order, extract
> -   the value. We assume the size always fit in 32 bits.  */
> -uint32_t size;
> -cpu_memory_rw_debug(cs, common_semi_flen_buf(cs) + 32,
> -(uint8_t *)&size, 4, 0);
> -size = be32_to_cpu(size);
> -common_semi_set_ret(cs, size);
> -errno = err;
> -set_swi_errno(cs, -1);
> +if (!err) {
> +/*
> + * The size is always stored in big-endian order, extract
> + * the value. We assume the size always fit in 32 bits.
> + */
> +uint32_t size;
> +cpu_memory_rw_debug(cs, common_semi_flen_buf(cs) + 32,
> +(uint8_t *)&size, 4, 0);

I did a double take but compared to the actual code it calls I guess
its fine. Technically I think: 

cpu_memory_rw_debug(cs, common_semi_flen_buf(cs) + 32,
(void *) &size, sizeof(size), 0);

might be slightly cleaner but whatever:

Reviewed-by: Alex Bennée 


> +ret = be32_to_cpu(size);
> +}
> +common_semi_cb(cs, ret, err);
>  }
>  
>  static int common_semi_open_guestfd;


-- 
Alex Bennée

[PATCH 0/3] Cavium Octeon MIPS extensions

[Pavel Dovgalyuk]

The following series includes emulation of the platform-specific MIPS extension
for Cavium Octeon CPUS:
- basic Octeon vCPU model
- custom instruction decoder for Octeon
- implementation of arithmetic and logic instructions

---

Pavel Dovgalyuk (3):
  target/mips: introduce generic Cavium Octeon CPU model
  target/mips: implement Octeon-specific BBIT instructions
  target/mips: implement Octeon-specific arithmetic instructions


 target/mips/helper.h|   1 +
 target/mips/tcg/meson.build |   3 +
 target/mips/tcg/octeon.decode   |  39 +
 target/mips/tcg/octeon_helper.c |  22 +++
 target/mips/tcg/octeon_helper.h.inc |  10 ++
 target/mips/tcg/octeon_translate.c  | 235 
 target/mips/tcg/translate.c |   5 +
 target/mips/tcg/translate.h |   1 +
 8 files changed, 316 insertions(+)
 create mode 100644 target/mips/tcg/octeon.decode
 create mode 100644 target/mips/tcg/octeon_helper.c
 create mode 100644 target/mips/tcg/octeon_helper.h.inc
 create mode 100644 target/mips/tcg/octeon_translate.c

--
Pavel Dovgalyuk

[PATCH 1/3] target/mips: introduce generic Cavium Octeon CPU model

[Pavel Dovgalyuk]

This patch adds generic Octeon vCPU for providing
Octeon-specific instructions.

Signed-off-by: Pavel Dovgalyuk 
---
 target/mips/cpu-defs.c.inc |   30 ++
 target/mips/mips-defs.h|1 +
 2 files changed, 31 insertions(+)

diff --git a/target/mips/cpu-defs.c.inc b/target/mips/cpu-defs.c.inc
index 582f940070..a60e48433c 100644
--- a/target/mips/cpu-defs.c.inc
+++ b/target/mips/cpu-defs.c.inc
@@ -921,6 +921,36 @@ const mips_def_t mips_defs[] =
 .insn_flags = CPU_MIPS64R2 | ASE_DSP | ASE_DSP_R2,
 .mmu_type = MMU_TYPE_R4000,
 },
+{
+/*
+ * A generic CPU providing MIPS64 Cavium Octeon features.
+ * PRid is taken from Octeon 68xx CPUs
+ * FIXME: Eventually this should be replaced by a real CPU model.
+ */
+.name = "Octeon",
+.CP0_PRid = 0x000D9100,
+.CP0_Config0 = MIPS_CONFIG0 | (0x1 << CP0C0_AR) | (0x2 << CP0C0_AT) |
+   (MMU_TYPE_R4000 << CP0C0_MT),
+.CP0_Config1 = MIPS_CONFIG1 | (0x3F << CP0C1_MMU) |
+   (1 << CP0C1_IS) | (4 << CP0C1_IL) | (1 << CP0C1_IA) |
+   (1 << CP0C1_DS) | (4 << CP0C1_DL) | (1 << CP0C1_DA) |
+   (1 << CP0C1_PC) | (1 << CP0C1_WR) | (1 << CP0C1_EP),
+.CP0_Config2 = MIPS_CONFIG2,
+.CP0_Config3 = MIPS_CONFIG3 | (1 << CP0C3_LPA) | (1 << CP0C3_DSPP) ,
+.CP0_Config4 = MIPS_CONFIG4 | (1U << CP0C4_M) |
+   (0x3c << CP0C4_KScrExist) | (1U << CP0C4_MMUExtDef) |
+   (3U << CP0C4_MMUSizeExt),
+.CP0_LLAddr_rw_bitmask = 0,
+.CP0_LLAddr_shift = 4,
+.CP0_PageGrain = (1 << CP0PG_ELPA),
+.SYNCI_Step = 32,
+.CCRes = 2,
+.CP0_Status_rw_bitmask = 0x12F8,
+.SEGBITS = 42,
+.PABITS = 49,
+.insn_flags = CPU_MIPS64R2 | INSN_OCTEON | ASE_DSP,
+.mmu_type = MMU_TYPE_R4000,
+},
 
 #endif
 };
diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index 0a12d982a7..a6cebe0265 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -42,6 +42,7 @@
 #define INSN_LOONGSON2E   0x0400ULL
 #define INSN_LOONGSON2F   0x0800ULL
 #define INSN_LOONGSON3A   0x1000ULL
+#define INSN_OCTEON   0x2000ULL
 /*
  *   bits 52-63: vendor-specific ASEs
  */

[PATCH 2/3] target/mips: implement Octeon-specific BBIT instructions

[Pavel Dovgalyuk]

This patch introduces Octeon-specific decoder and implements
check-bit-and-jump instructions.

Signed-off-by: Pavel Dovgalyuk 
---
 target/mips/tcg/meson.build|2 +
 target/mips/tcg/octeon.decode  |   14 ++
 target/mips/tcg/octeon_translate.c |   53 
 target/mips/tcg/translate.c|5 +++
 target/mips/tcg/translate.h|1 +
 5 files changed, 75 insertions(+)
 create mode 100644 target/mips/tcg/octeon.decode
 create mode 100644 target/mips/tcg/octeon_translate.c

diff --git a/target/mips/tcg/meson.build b/target/mips/tcg/meson.build
index 98003779ae..7ee969ec8f 100644
--- a/target/mips/tcg/meson.build
+++ b/target/mips/tcg/meson.build
@@ -3,6 +3,7 @@ gen = [
   decodetree.process('msa.decode', extra_args: '--decode=decode_ase_msa'),
   decodetree.process('tx79.decode', extra_args: '--static-decode=decode_tx79'),
   decodetree.process('vr54xx.decode', extra_args: 
'--decode=decode_ext_vr54xx'),
+  decodetree.process('octeon.decode', extra_args: 
'--decode=decode_ext_octeon'),
 ]
 
 mips_ss.add(gen)
@@ -24,6 +25,7 @@ mips_ss.add(files(
 ))
 mips_ss.add(when: 'TARGET_MIPS64', if_true: files(
   'tx79_translate.c',
+  'octeon_translate.c',
 ), if_false: files(
   'mxu_translate.c',
 ))
diff --git a/target/mips/tcg/octeon.decode b/target/mips/tcg/octeon.decode
new file mode 100644
index 00..c06d576292
--- /dev/null
+++ b/target/mips/tcg/octeon.decode
@@ -0,0 +1,14 @@
+# Octeon Architecture Module instruction set
+#
+# Copyright (C) 2022 Pavel Dovgalyuk
+#
+# SPDX-License-Identifier: LGPL-2.1-or-later
+#
+
+# Branch on bit set or clear
+# BBIT0  110010 . . 
+# BBIT032110110 . . 
+# BBIT1  111010 . . 
+# BBIT13210 . . 
+
+BBIT 11 set:1 shift:1 10 rs:5 p:5 offset:16
diff --git a/target/mips/tcg/octeon_translate.c 
b/target/mips/tcg/octeon_translate.c
new file mode 100644
index 00..bd87066b01
--- /dev/null
+++ b/target/mips/tcg/octeon_translate.c
@@ -0,0 +1,53 @@
+/*
+ * Octeon-specific instructions translation routines
+ *
+ *  Copyright (c) 2022 Pavel Dovgalyuk
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "tcg/tcg-op.h"
+#include "tcg/tcg-op-gvec.h"
+#include "exec/helper-gen.h"
+#include "translate.h"
+
+/* Include the auto-generated decoder.  */
+#include "decode-octeon.c.inc"
+
+static bool trans_BBIT(DisasContext *ctx, arg_BBIT *a)
+{
+int p = a->p;
+
+if (ctx->hflags & MIPS_HFLAG_BMASK) {
+#ifdef MIPS_DEBUG_DISAS
+LOG_DISAS("Branch in delay / forbidden slot at PC 0x"
+  TARGET_FMT_lx "\n", ctx->base.pc_next);
+#endif
+generate_exception_end(ctx, EXCP_RI);
+return true;
+}
+
+/* Load needed operands */
+TCGv t0 = tcg_temp_new();
+gen_load_gpr(t0, a->rs);
+
+if (a->shift) {
+p += 32;
+}
+tcg_gen_andi_tl(t0, t0, 1ULL << p);
+
+/* Jump conditions */
+if (a->set) {
+tcg_gen_setcondi_tl(TCG_COND_NE, bcond, t0, 0);
+} else {
+tcg_gen_setcondi_tl(TCG_COND_EQ, bcond, t0, 0);
+}
+
+ctx->hflags |= MIPS_HFLAG_BC;
+ctx->btarget = ctx->base.pc_next + 4 + a->offset * 4;
+ctx->hflags |= MIPS_HFLAG_BDS32;
+
+tcg_temp_free(t0);
+return true;
+}
diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 6de5b66650..4f41a9b00a 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -15963,6 +15963,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext 
*ctx)
 if (cpu_supports_isa(env, INSN_VR54XX) && decode_ext_vr54xx(ctx, 
ctx->opcode)) {
 return;
 }
+#if defined(TARGET_MIPS64)
+if (cpu_supports_isa(env, INSN_OCTEON) && decode_ext_octeon(ctx, 
ctx->opcode)) {
+return;
+}
+#endif
 
 /* ISA extensions */
 if (ase_msa_available(env) && decode_ase_msa(ctx, ctx->opcode)) {
diff --git a/target/mips/tcg/translate.h b/target/mips/tcg/translate.h
index 9997fe2f3c..55053226ae 100644
--- a/target/mips/tcg/translate.h
+++ b/target/mips/tcg/translate.h
@@ -215,6 +215,7 @@ bool decode_ase_msa(DisasContext *ctx, uint32_t insn);
 bool decode_ext_txx9(DisasContext *ctx, uint32_t insn);
 #if defined(TARGET_MIPS64)
 bool decode_ext_tx79(DisasContext *ctx, uint32_t insn);
+bool decode_ext_octeon(DisasContext *ctx, uint32_t insn);
 #endif
 bool decode_ext_vr54xx(DisasContext *ctx, uint32_t insn);
 

[PATCH 3/3] target/mips: implement Octeon-specific arithmetic instructions

[Pavel Dovgalyuk]

This patch implements several Octeon-specific instructions:
- BADDU
- DMUL
- EXTS/EXTS32
- CINS/CINS32
- POP/DPOP
- SEQ/SEQI
- SNE/SNEI

Signed-off-by: Pavel Dovgalyuk 
---
 target/mips/helper.h|1 
 target/mips/tcg/meson.build |1 
 target/mips/tcg/octeon.decode   |   25 +
 target/mips/tcg/octeon_helper.c |   22 
 target/mips/tcg/octeon_helper.h.inc |   10 ++
 target/mips/tcg/octeon_translate.c  |  182 +++
 6 files changed, 241 insertions(+)
 create mode 100644 target/mips/tcg/octeon_helper.c
 create mode 100644 target/mips/tcg/octeon_helper.h.inc

diff --git a/target/mips/helper.h b/target/mips/helper.h
index de32d82e98..d68abdeac1 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -597,3 +597,4 @@ DEF_HELPER_FLAGS_2(rddsp, 0, tl, tl, env)
 
 /* Vendor extensions */
 #include "tcg/vr54xx_helper.h.inc"
+#include "tcg/octeon_helper.h.inc"
diff --git a/target/mips/tcg/meson.build b/target/mips/tcg/meson.build
index 7ee969ec8f..1852366846 100644
--- a/target/mips/tcg/meson.build
+++ b/target/mips/tcg/meson.build
@@ -26,6 +26,7 @@ mips_ss.add(files(
 mips_ss.add(when: 'TARGET_MIPS64', if_true: files(
   'tx79_translate.c',
   'octeon_translate.c',
+  'octeon_helper.c',
 ), if_false: files(
   'mxu_translate.c',
 ))
diff --git a/target/mips/tcg/octeon.decode b/target/mips/tcg/octeon.decode
index c06d576292..ababf59e42 100644
--- a/target/mips/tcg/octeon.decode
+++ b/target/mips/tcg/octeon.decode
@@ -12,3 +12,28 @@
 # BBIT13210 . . 
 
 BBIT 11 set:1 shift:1 10 rs:5 p:5 offset:16
+
+# Arithmetic
+# BADDU rd, rs, rt
+# DMUL rd, rs, rt
+# EXTS rt, rs, p, lenm1
+# EXTS32 rt, rs, p, lenm1
+# CINS rt, rs, p, lenm1
+# CINS32 rt, rs, p, lenm1
+# DPOP rd, rs
+# POP rd, rs
+# SEQ rd, rs, rt
+# SEQI rt, rs, immediate
+# SNE rd, rs, rt
+# SNEI rt, rs, immediate
+
+@r3  .. rs:5 rt:5 rd:5 . ..
+@bitfield.. rs:5 rt:5 lenm1:5 p:5 . shift:1
+
+BADDU011100 . . . 0 101000 @r3
+DMUL 011100 . . . 0 11 @r3
+EXTS 011100 . . . . 11101 . @bitfield
+CINS 011100 . . . . 11001 . @bitfield
+POP  011100 rs:5 0 rd:5 0 10110 dw:1
+SEQNE011100 rs:5 rt:5 rd:5 0 10101 ne:1
+SEQNEI   011100 rs:5 rt:5 imm:s10 10111 ne:1
diff --git a/target/mips/tcg/octeon_helper.c b/target/mips/tcg/octeon_helper.c
new file mode 100644
index 00..e9650c58bd
--- /dev/null
+++ b/target/mips/tcg/octeon_helper.c
@@ -0,0 +1,22 @@
+/*
+ *  MIPS Octeon emulation helpers
+ *
+ *  Copyright (c) 2022 Pavel Dovgalyuk
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "exec/helper-proto.h"
+
+target_ulong helper_pop(target_ulong arg)
+{
+int i;
+int res = 0;
+for (i = 0 ; i < 64 ; ++i) {
+res += arg & 1;
+arg >>= 1;
+}
+
+return res;
+}
diff --git a/target/mips/tcg/octeon_helper.h.inc 
b/target/mips/tcg/octeon_helper.h.inc
new file mode 100644
index 00..cfc051ef47
--- /dev/null
+++ b/target/mips/tcg/octeon_helper.h.inc
@@ -0,0 +1,10 @@
+/*
+ *  MIPS Octeon emulation helpers
+ *
+ *  Copyright (c) 2022 Pavel Dovgalyuk
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+#if defined(TARGET_MIPS64)
+DEF_HELPER_1(pop, tl, tl)
+#endif
diff --git a/target/mips/tcg/octeon_translate.c 
b/target/mips/tcg/octeon_translate.c
index bd87066b01..c4ef3e5bcb 100644
--- a/target/mips/tcg/octeon_translate.c
+++ b/target/mips/tcg/octeon_translate.c
@@ -51,3 +51,185 @@ static bool trans_BBIT(DisasContext *ctx, arg_BBIT *a)
 tcg_temp_free(t0);
 return true;
 }
+
+static bool trans_BADDU(DisasContext *ctx, arg_BADDU *a)
+{
+TCGv t0, t1;
+
+if (a->rt == 0) {
+/* nop */
+return true;
+}
+
+t0 = tcg_temp_new();
+t1 = tcg_temp_new();
+gen_load_gpr(t0, a->rs);
+gen_load_gpr(t1, a->rt);
+
+tcg_gen_add_tl(t0, t0, t1);
+tcg_gen_andi_i64(cpu_gpr[a->rd], t0, 0xff);
+
+tcg_temp_free(t0);
+tcg_temp_free(t1);
+
+return true;
+}
+
+static bool trans_DMUL(DisasContext *ctx, arg_DMUL *a)
+{
+TCGv t0, t1;
+
+if (a->rt == 0) {
+/* nop */
+return true;
+}
+
+t0 = tcg_temp_new();
+t1 = tcg_temp_new();
+gen_load_gpr(t0, a->rs);
+gen_load_gpr(t1, a->rt);
+
+tcg_gen_mul_i64(cpu_gpr[a->rd], t0, t1);
+
+tcg_temp_free(t0);
+tcg_temp_free(t1);
+
+return true;
+}
+
+static bool trans_EXTS(DisasContext *ctx, arg_EXTS *a)
+{
+TCGv t0, t1;
+int p;
+TCGLabel *l1;
+
+if (a->rt == 0) {
+/* nop */
+return true;
+}
+
+p = a->p;
+if (a->shift) {
+p += 32;
+}
+
+t0 = tcg_temp_new();
+t1 = tcg_temp_new();
+gen_load_gpr(t1, a->rs);
+
+tcg_gen_movi_tl(t0, ((1ULL << (a->lenm1 + 1)) - 1) << p);
+tcg_gen_and_tl(t1, t1, t0);
+tcg_gen_

[PATCH 0/7] More tests/tcg cleanups

[Paolo Bonzini]

Building on the introduction of config-$target.mak, make tests/tcg a
"regular" subdirectory that is entered simply with "make -C", like the
ROMs or the plugins.

The next step could be to unify all the sub-make rules; this series
stops short of that.

Paolo

Paolo Bonzini (7):
  meson: put cross compiler info in a separate section
  build: include pc-bios/ part in the ROMS variable
  configure: allow more host/target combos to use the host compiler
  configure: move tests/tcg/Makefile.prereqs to root build directory
  configure: store container engine in config-host.mak
  tests: simplify Makefile invocation for tests/tcg
  tests/tcg: remove -f from Makefile invocation

 Makefile  | 17 +++
 configure | 90 +--
 meson.build   | 15 +++---
 tests/Makefile.include| 13 ++---
 tests/docker/Makefile.include |  2 +-
 tests/tcg/Makefile.target |  2 +-
 6 files changed, 77 insertions(+), 62 deletions(-)

-- 
2.36.1

[PATCH 6/7] tests: simplify Makefile invocation for tests/tcg

[Paolo Bonzini]

The tests/tcg Makefile invocation contains the paths to DOCKER_SCRIPT
and TARGET.

One can just resolve the path to docker.py in configure so that submakes
do not need the DOCKER_SCRIPT variable.  In order to remove the TARGET
variable, create a config-target.mak file in tests/tcg/$TARGET.  For now
config-$target.mak is created before the check for compiler presence,
while tests/tcg/$TARGET is created afterwards.  This is temporary.

Signed-off-by: Paolo Bonzini 
---
 configure | 23 ++-
 tests/Makefile.include|  9 +++--
 tests/tcg/Makefile.target |  2 +-
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/configure b/configure
index 0fd5a3cfad..9d49ea4c5b 100755
--- a/configure
+++ b/configure
@@ -1824,6 +1824,9 @@ if test $use_containers = "yes"; then
 podman) container=podman ;;
 no) container=no ;;
 esac
+if test "$container" != "no"; then
+docker_py="$python $source_path/tests/docker/docker.py --engine 
$container"
+fi
 fi
 
 # cross compilers defaults, can be overridden with --cross-cc-ARCH
@@ -2130,16 +2133,16 @@ write_target_makefile() {
 write_container_target_makefile() {
   echo "$1: docker-image-$container_image" >> Makefile.prereqs
   if test -n "$container_cross_cc"; then
-echo "CC=\$(DOCKER_SCRIPT) cc --cc $container_cross_cc -i 
qemu/$container_image -s $source_path --"
-echo "CCAS=\$(DOCKER_SCRIPT) cc --cc $container_cross_cc -i 
qemu/$container_image -s $source_path --"
+echo "CC=$docker_py cc --cc $container_cross_cc -i qemu/$container_image 
-s $source_path --"
+echo "CCAS=$docker_py cc --cc $container_cross_cc -i qemu/$container_image 
-s $source_path --"
   fi
-  echo "AR=\$(DOCKER_SCRIPT) cc --cc $container_cross_ar -i 
qemu/$container_image -s $source_path --"
-  echo "AS=\$(DOCKER_SCRIPT) cc --cc $container_cross_as -i 
qemu/$container_image -s $source_path --"
-  echo "LD=\$(DOCKER_SCRIPT) cc --cc $container_cross_ld -i 
qemu/$container_image -s $source_path --"
-  echo "NM=\$(DOCKER_SCRIPT) cc --cc $container_cross_nm -i 
qemu/$container_image -s $source_path --"
-  echo "OBJCOPY=\$(DOCKER_SCRIPT) cc --cc $container_cross_objcopy -i 
qemu/$container_image -s $source_path --"
-  echo "RANLIB=\$(DOCKER_SCRIPT) cc --cc $container_cross_ranlib -i 
qemu/$container_image -s $source_path --"
-  echo "STRIP=\$(DOCKER_SCRIPT) cc --cc $container_cross_strip -i 
qemu/$container_image -s $source_path --"
+  echo "AR=$docker_py cc --cc $container_cross_ar -i qemu/$container_image -s 
$source_path --"
+  echo "AS=$docker_py cc --cc $container_cross_as -i qemu/$container_image -s 
$source_path --"
+  echo "LD=$docker_py cc --cc $container_cross_ld -i qemu/$container_image -s 
$source_path --"
+  echo "NM=$docker_py cc --cc $container_cross_nm -i qemu/$container_image -s 
$source_path --"
+  echo "OBJCOPY=$docker_py cc --cc $container_cross_objcopy -i 
qemu/$container_image -s $source_path --"
+  echo "RANLIB=$docker_py cc --cc $container_cross_ranlib -i 
qemu/$container_image -s $source_path --"
+  echo "STRIP=$docker_py cc --cc $container_cross_strip -i 
qemu/$container_image -s $source_path --"
 }
 
 
@@ -2597,6 +2600,8 @@ for target in $target_list; do
   fi
   if test $got_cross_cc = yes; then
   mkdir -p tests/tcg/$target
+  ln -sf ../config-$target.mak tests/tcg/$target/config-target.mak
+  echo "TARGET=$target" >> $config_target_mak
   echo "QEMU=$PWD/$qemu" >> $config_target_mak
   echo "EXTRA_CFLAGS=$target_cflags" >> $config_target_mak
   echo "run-tcg-tests-$target: $qemu\$(EXESUF)" >> Makefile.prereqs
diff --git a/tests/Makefile.include b/tests/Makefile.include
index f4ba4027ea..f2182ead1e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -47,23 +47,20 @@ $(foreach TARGET,$(TCG_TESTS_TARGETS), \
 .PHONY: $(TCG_TESTS_TARGETS:%=build-tcg-tests-%)
 $(TCG_TESTS_TARGETS:%=build-tcg-tests-%): build-tcg-tests-%: 
$(BUILD_DIR)/tests/tcg/config-%.mak
$(call quiet-command, \
-$(MAKE) -C tests/tcg/$* -f ../Makefile.target $(SUBDIR_MAKEFLAGS) \
-DOCKER_SCRIPT="$(DOCKER_SCRIPT)" \
-TARGET="$*" SRC_PATH="$(SRC_PATH)", \
+$(MAKE) -C tests/tcg/$* -f ../Makefile.target $(SUBDIR_MAKEFLAGS), 
\
 "BUILD","$* guest-tests")
 
 .PHONY: $(TCG_TESTS_TARGETS:%=run-tcg-tests-%)
 $(TCG_TESTS_TARGETS:%=run-tcg-tests-%): run-tcg-tests-%: build-tcg-tests-%
$(call quiet-command, \
$(MAKE) -C tests/tcg/$* -f ../Makefile.target $(SUBDIR_MAKEFLAGS) \
-TARGET="$*" SRC_PATH="$(SRC_PATH)" SPEED=$(SPEED) run, 
\
+SPEED=$(SPEED) run, \
 "RUN", "$* guest-tests")
 
 .PHONY: $(TCG_TESTS_TARGETS:%=clean-tcg-tests-%)
 $(TCG_TESTS_TARGETS:%=clean-tcg-tests-%): clean-tcg-tests-%:
$(call quiet-command, \
-   $(MAKE) -C tests/tcg/$* -f ../Makefile.target $(SUBDIR_MAKEFLAGS) \
-TARGE

[PATCH 2/7] build: include pc-bios/ part in the ROMS variable

[Paolo Bonzini]

Include the full path in TARGET_DIR, so that messages from sub-Makefiles
are clearer.  Also, prepare for possibly building firmware outside
pc-bios/ from the Makefile,

Signed-off-by: Paolo Bonzini 
---
 Makefile  | 12 +---
 configure |  6 +++---
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 3c0d89057e..ec4445db9a 100644
--- a/Makefile
+++ b/Makefile
@@ -186,16 +186,14 @@ include $(SRC_PATH)/tests/Makefile.include
 
 all: recurse-all
 
-ROM_DIRS = $(addprefix pc-bios/, $(ROMS))
-ROM_DIRS_RULES=$(foreach t, all clean, $(addsuffix /$(t), $(ROM_DIRS)))
-# Only keep -O and -g cflags
-.PHONY: $(ROM_DIRS_RULES)
-$(ROM_DIRS_RULES):
+ROMS_RULES=$(foreach t, all clean, $(addsuffix /$(t), $(ROMS)))
+.PHONY: $(ROMS_RULES)
+$(ROMS_RULES):
$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" 
TARGET_DIR="$(dir $@)" $(notdir $@),)
 
 .PHONY: recurse-all recurse-clean
-recurse-all: $(addsuffix /all, $(ROM_DIRS))
-recurse-clean: $(addsuffix /clean, $(ROM_DIRS))
+recurse-all: $(addsuffix /all, $(ROMS))
+recurse-clean: $(addsuffix /clean, $(ROMS))
 
 ##
 
diff --git a/configure b/configure
index e69537c756..b1aa97e470 100755
--- a/configure
+++ b/configure
@@ -2242,7 +2242,7 @@ if test -n "$target_cc" &&
 fi
 done
 if test -n "$ld_i386_emulation"; then
-roms="optionrom"
+roms="pc-bios/optionrom"
 config_mak=pc-bios/optionrom/config.mak
 echo "# Automatically generated by configure - do not modify" > 
$config_mak
 echo "TOPSRC_DIR=$source_path" >> $config_mak
@@ -2253,7 +2253,7 @@ fi
 
 probe_target_compilers ppc ppc64
 if test -n "$target_cc" && test "$softmmu" = yes; then
-roms="$roms vof"
+roms="$roms pc-bios/vof"
 config_mak=pc-bios/vof/config.mak
 echo "# Automatically generated by configure - do not modify" > $config_mak
 echo "SRC_DIR=$source_path/pc-bios/vof" >> $config_mak
@@ -2272,7 +2272,7 @@ if test -n "$target_cc" && test "$softmmu" = yes; then
   echo "WARNING: Your compiler does not support the z900!"
   echo " The s390-ccw bios will only work with guest CPUs >= z10."
 fi
-roms="$roms s390-ccw"
+roms="$roms pc-bios/s390-ccw"
 config_mak=pc-bios/s390-ccw/config-host.mak
 echo "# Automatically generated by configure - do not modify" > $config_mak
 echo "SRC_PATH=$source_path/pc-bios/s390-ccw" >> $config_mak
-- 
2.36.1

[PATCH 3/7] configure: allow more host/target combos to use the host compiler

[Paolo Bonzini]

Do not require a cross-compiler prefix for e.g. i386 on x86_64, or
big endian on little endian.

Signed-off-by: Paolo Bonzini 
---
 configure | 34 +-
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/configure b/configure
index b1aa97e470..28f8c6188b 100755
--- a/configure
+++ b/configure
@@ -2058,19 +2058,27 @@ probe_target_compiler() {
   compute_target_variable $1 target_objcopy objcopy
   compute_target_variable $1 target_ranlib ranlib
   compute_target_variable $1 target_strip strip
-  if test "$1" = $cpu; then
-: ${target_cc:=$cc}
-: ${target_ccas:=$ccas}
-: ${target_as:=$as}
-: ${target_ld:=$ld}
-: ${target_ar:=$ar}
-: ${target_as:=$as}
-: ${target_ld:=$ld}
-: ${target_nm:=$nm}
-: ${target_objcopy:=$objcopy}
-: ${target_ranlib:=$ranlib}
-: ${target_strip:=$strip}
-  fi
+  case "$1:$cpu" in
+aarch64:aarch64_be | aarch64_be:aarch64 | \
+arm:armeb | armeb:arm | \
+i386:x86_64 | \
+ppc:ppc64* | \
+ppc64*:ppc64* | \
+sparc:sparc64 | \
+"$cpu:$cpu")
+  : ${target_cc:=$cc}
+  : ${target_ccas:=$ccas}
+  : ${target_as:=$as}
+  : ${target_ld:=$ld}
+  : ${target_ar:=$ar}
+  : ${target_as:=$as}
+  : ${target_ld:=$ld}
+  : ${target_nm:=$nm}
+  : ${target_objcopy:=$objcopy}
+  : ${target_ranlib:=$ranlib}
+  : ${target_strip:=$strip}
+  ;;
+  esac
   if test -n "$target_cc"; then
 case $1 in
   i386|x86_64)
-- 
2.36.1

[PATCH 4/7] configure: move tests/tcg/Makefile.prereqs to root build directory

[Paolo Bonzini]

It will not be specific to tests/tcg anymore once it will be possible to
build firmware using container-based cross compilers too.  Prepare for that
already, after all Makefile.prereqs is not _used_ by tests/tcg.

Signed-off-by: Paolo Bonzini 
---
 Makefile   |  5 -
 configure  | 15 +++
 tests/Makefile.include |  3 ---
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/Makefile b/Makefile
index ec4445db9a..9fc750ee70 100644
--- a/Makefile
+++ b/Makefile
@@ -42,6 +42,9 @@ configure: ;
 ifneq ($(wildcard config-host.mak),)
 include config-host.mak
 
+include Makefile.prereqs
+Makefile.prereqs: config-host.mak
+
 git-submodule-update:
 .git-submodule-status: git-submodule-update config-host.mak
 Makefile: .git-submodule-status
@@ -216,7 +219,7 @@ qemu-%.tar.bz2:
 
 distclean: clean
-$(quiet-@)test -f build.ninja && $(NINJA) $(NINJAFLAGS) -t clean -g || 
:
-   rm -f config-host.mak
+   rm -f config-host.mak Makefile.prereqs
rm -f tests/tcg/config-*.mak
rm -f config.status
rm -f roms/seabios/config.mak
diff --git a/configure b/configure
index 28f8c6188b..6b38b0815c 100755
--- a/configure
+++ b/configure
@@ -2126,6 +2126,7 @@ write_target_makefile() {
 }
 
 write_container_target_makefile() {
+  echo "$1: docker-image-$container_image" >> Makefile.prereqs
   if test -n "$container_cross_cc"; then
 echo "CC=\$(DOCKER_SCRIPT) cc --cc $container_cross_cc -i 
qemu/$container_image -s $source_path --"
 echo "CCAS=\$(DOCKER_SCRIPT) cc --cc $container_cross_cc -i 
qemu/$container_image -s $source_path --"
@@ -2234,6 +2235,8 @@ for f in $LINKS ; do
 fi
 done
 
+echo "# Automatically generated by configure - do not modify" > 
Makefile.prereqs
+
 # Mac OS X ships with a broken assembler
 roms=
 probe_target_compilers i386 x86_64
@@ -2471,10 +2474,7 @@ if test "$safe_stack" = "yes"; then
 fi
 
 # tests/tcg configuration
-(makefile=tests/tcg/Makefile.prereqs
-echo "# Automatically generated by configure - do not modify" > $makefile
-
-config_host_mak=tests/tcg/config-host.mak
+(config_host_mak=tests/tcg/config-host.mak
 echo "# Automatically generated by configure - do not modify" > 
$config_host_mak
 echo "SRC_PATH=$source_path" >> $config_host_mak
 echo "HOST_CC=$host_cc" >> $config_host_mak
@@ -2570,9 +2570,8 @@ for target in $target_list; do
   ;;
   esac
   elif test -n "$container_image"; then
-  echo "build-tcg-tests-$target: docker-image-$container_image" >> 
$makefile
   echo "BUILD_STATIC=y" >> $config_target_mak
-  write_container_target_makefile >> $config_target_mak
+  write_container_target_makefile build-tcg-tests-$target >> 
$config_target_mak
   case $target in
   aarch64-*)
   echo "CROSS_CC_HAS_SVE=y" >> $config_target_mak
@@ -2595,11 +2594,11 @@ for target in $target_list; do
   mkdir -p tests/tcg/$target
   echo "QEMU=$PWD/$qemu" >> $config_target_mak
   echo "EXTRA_CFLAGS=$target_cflags" >> $config_target_mak
-  echo "run-tcg-tests-$target: $qemu\$(EXESUF)" >> $makefile
+  echo "run-tcg-tests-$target: $qemu\$(EXESUF)" >> Makefile.prereqs
   tcg_tests_targets="$tcg_tests_targets $target"
   fi
 done
-echo "TCG_TESTS_TARGETS=$tcg_tests_targets" >> $makefile)
+echo "TCG_TESTS_TARGETS=$tcg_tests_targets" >> config-host.mak)
 
 if test "$skip_meson" = no; then
   cross="config-meson.cross.new"
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3accb83b13..f4ba4027ea 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -36,9 +36,6 @@ export SRC_PATH
 
 SPEED = quick
 
--include tests/tcg/Makefile.prereqs
-tests/tcg/Makefile.prereqs: config-host.mak
-
 # Per guest TCG tests
 BUILD_TCG_TARGET_RULES=$(patsubst %,build-tcg-tests-%, $(TCG_TESTS_TARGETS))
 CLEAN_TCG_TARGET_RULES=$(patsubst %,clean-tcg-tests-%, $(TCG_TESTS_TARGETS))
-- 
2.36.1

[PATCH 5/7] configure: store container engine in config-host.mak

[Paolo Bonzini]

In preparation for removing $(DOCKER_SCRIPT) from the tests/tcg configuration
files, have Make use the same container engine that had been probed at
configure time.

Signed-off-by: Paolo Bonzini 
---
 configure | 11 ---
 tests/docker/Makefile.include |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 6b38b0815c..0fd5a3cfad 100755
--- a/configure
+++ b/configure
@@ -1819,9 +1819,11 @@ esac
 
 container="no"
 if test $use_containers = "yes"; then
-if has "docker" || has "podman"; then
-container=$($python $source_path/tests/docker/docker.py probe)
-fi
+case $($python $source_path/tests/docker/docker.py probe) in
+*docker) container=docker ;;
+podman) container=podman ;;
+no) container=no ;;
+esac
 fi
 
 # cross compilers defaults, can be overridden with --cross-cc-ARCH
@@ -2401,6 +2403,9 @@ if test -n "$gdb_bin"; then
 fi
 fi
 
+if test "$container" != no; then
+echo "ENGINE=$container" >> $config_host_mak
+fi
 echo "ROMS=$roms" >> $config_host_mak
 echo "MAKE=$make" >> $config_host_mak
 echo "PYTHON=$python" >> $config_host_mak
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index e68f91b853..d9b6ab7b41 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -14,7 +14,7 @@ DOCKER_DEFAULT_REGISTRY := 
registry.gitlab.com/qemu-project/qemu
 endif
 DOCKER_REGISTRY := $(if $(REGISTRY),$(REGISTRY),$(DOCKER_DEFAULT_REGISTRY))
 
-ENGINE := auto
+ENGINE ?= auto
 DOCKER_SCRIPT=$(SRC_PATH)/tests/docker/docker.py --engine $(ENGINE)
 
 CUR_TIME := $(shell date +%Y-%m-%d-%H.%M.%S.)
-- 
2.36.1

Re: [PATCH 38/71] target/arm: Introduce sve_vqm1_for_el_sm

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:28, Richard Henderson
 wrote:
>
> When Streaming SVE mode is enabled, the size is taken from
> SMCR_ELx instead of ZCR_ELx.  The format is shared, but the
> set of vector lengths is not.  Further, Streaming SVE does
> not require any particular length to be supported.
>
> Adjust sve_vqm1_for_el to pass the current value of PSTATE.SM
> to the new function.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Peter Maydell 

thanks
-- PMM

[PATCH 1/7] meson: put cross compiler info in a separate section

[Paolo Bonzini]

While at it, remove a dead assignment and simply inline the value of the
"target" variable, which is used just once.

Signed-off-by: Paolo Bonzini 
---
 meson.build | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index 21cd949082..3f38b3ee4f 100644
--- a/meson.build
+++ b/meson.build
@@ -3740,21 +3740,24 @@ endif
 summary_info += {'strip binaries':get_option('strip')}
 summary_info += {'sparse':sparse}
 summary_info += {'mingw32 support':   targetos == 'windows'}
+summary(summary_info, bool_yn: true, section: 'Compilation')
 
 # snarf the cross-compilation information for tests
+summary_info = {}
+have_cross = false
 foreach target: target_dirs
   tcg_mak = meson.current_build_dir() / 'tests/tcg' / 'config-' + target + 
'.mak'
   if fs.exists(tcg_mak)
 config_cross_tcg = keyval.load(tcg_mak)
-target = config_cross_tcg['TARGET_NAME']
-compiler = ''
 if 'CC' in config_cross_tcg
-  summary_info += {target + ' tests': config_cross_tcg['CC']}
+  summary_info += {config_cross_tcg['TARGET_NAME']: config_cross_tcg['CC']}
+  have_cross = true
 endif
-   endif
+  endif
 endforeach
-
-summary(summary_info, bool_yn: true, section: 'Compilation')
+if have_cross
+  summary(summary_info, bool_yn: true, section: 'Cross compilers')
+endif
 
 # Targets and accelerators
 summary_info = {}
-- 
2.36.1

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Jason A. Donenfeld]

+ Arnd

On Sun, Jun 5, 2022 at 10:19 AM Jason A. Donenfeld  wrote:
>
> Hi folks,
>
> On Sun, Jun 05, 2022 at 04:32:13PM +0900, Stafford Horne wrote:
> > Why can't m68k switch to little-endian in qemu and the kernel?  The m68k 
> > virt
> > platform is not that old, 1 year? Are there a lot of users that this would 
> > be a big
> > problem?
>
> I also share this perspective. AFAICT, m68k virt platform *just*
> shipped. Fix this stuff instead of creating more compatibility bloat for
> a platform with no new silicon. The risks of making life difficult for
> 15 minutes for all seven and a half users of that code that only now has
> become operational is vastly dwarfed by the good sense to just fix the
> mistake. Treat the endian thing as a *bug* rather than a sacred ABI.
> Bugs only become sacred if you let them sit for years and large numbers
> of people grow to rely on spacebar heating. Otherwise they're just bugs.
> This can be fixed.
>
> Jason

Re: [PATCH v5 01/45] block: BlockDriver: add .filtered_child_is_backing field

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

Unfortunately not all filters use .file child as filtered child. Two
exclusions are mirror_top and commit_top. Happily they both are private
filters. Bad thing is that this inconsistency is observable through qmp
commands query-block / query-named-block-nodes. So, could we just
change mirror_top and commit_top to use file child as all other filter
driver is an open question. Probably, we could do that with some kind
of deprecation period, but how to warn users during it?

For now, let's just add a field so we can distinguish them in generic
code, it will be used in further commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/commit.c   |  1 +
  block/mirror.c   |  1 +
  include/block/block_int-common.h | 13 +
  3 files changed, 15 insertions(+)


Reviewed-by: Hanna Reitz 

[PATCH 7/7] tests/tcg: remove -f from Makefile invocation

[Paolo Bonzini]

Name the symbolic link "Makefile" and place it in the target subdirectory.

Signed-off-by: Paolo Bonzini 
---
 configure  | 3 ++-
 tests/Makefile.include | 7 +++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 9d49ea4c5b..f35847c3cd 100755
--- a/configure
+++ b/configure
@@ -2224,7 +2224,6 @@ fi
 # tests might fail. Prefer to keep the relevant files in their own
 # directory and symlink the directory instead.
 LINKS="Makefile"
-LINKS="$LINKS tests/tcg/Makefile.target"
 LINKS="$LINKS pc-bios/optionrom/Makefile"
 LINKS="$LINKS pc-bios/s390-ccw/Makefile"
 LINKS="$LINKS pc-bios/vof/Makefile"
@@ -2483,6 +2482,7 @@ fi
 
 # tests/tcg configuration
 (config_host_mak=tests/tcg/config-host.mak
+mkdir -p tests/tcg
 echo "# Automatically generated by configure - do not modify" > 
$config_host_mak
 echo "SRC_PATH=$source_path" >> $config_host_mak
 echo "HOST_CC=$host_cc" >> $config_host_mak
@@ -2600,6 +2600,7 @@ for target in $target_list; do
   fi
   if test $got_cross_cc = yes; then
   mkdir -p tests/tcg/$target
+  ln -sf $source_path/tests/tcg/Makefile.target tests/tcg/$target/Makefile
   ln -sf ../config-$target.mak tests/tcg/$target/config-target.mak
   echo "TARGET=$target" >> $config_target_mak
   echo "QEMU=$PWD/$qemu" >> $config_target_mak
diff --git a/tests/Makefile.include b/tests/Makefile.include
index f2182ead1e..8f44a20da3 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -47,20 +47,19 @@ $(foreach TARGET,$(TCG_TESTS_TARGETS), \
 .PHONY: $(TCG_TESTS_TARGETS:%=build-tcg-tests-%)
 $(TCG_TESTS_TARGETS:%=build-tcg-tests-%): build-tcg-tests-%: 
$(BUILD_DIR)/tests/tcg/config-%.mak
$(call quiet-command, \
-$(MAKE) -C tests/tcg/$* -f ../Makefile.target $(SUBDIR_MAKEFLAGS), 
\
+$(MAKE) -C tests/tcg/$* $(SUBDIR_MAKEFLAGS), \
 "BUILD","$* guest-tests")
 
 .PHONY: $(TCG_TESTS_TARGETS:%=run-tcg-tests-%)
 $(TCG_TESTS_TARGETS:%=run-tcg-tests-%): run-tcg-tests-%: build-tcg-tests-%
$(call quiet-command, \
-   $(MAKE) -C tests/tcg/$* -f ../Makefile.target $(SUBDIR_MAKEFLAGS) \
-SPEED=$(SPEED) run, \
+   $(MAKE) -C tests/tcg/$* $(SUBDIR_MAKEFLAGS) SPEED=$(SPEED) run, \
 "RUN", "$* guest-tests")
 
 .PHONY: $(TCG_TESTS_TARGETS:%=clean-tcg-tests-%)
 $(TCG_TESTS_TARGETS:%=clean-tcg-tests-%): clean-tcg-tests-%:
$(call quiet-command, \
-   $(MAKE) -C tests/tcg/$* -f ../Makefile.target $(SUBDIR_MAKEFLAGS) 
clean, \
+   $(MAKE) -C tests/tcg/$* $(SUBDIR_MAKEFLAGS) clean, \
 "CLEAN", "$* guest-tests")
 
 .PHONY: build-tcg
-- 
2.36.1

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Stafford Horne]

On Tue, Jun 07, 2022 at 10:42:08AM +0200, Arnd Bergmann wrote:
> On Tue, Jun 7, 2022 at 10:11 AM Geert Uytterhoeven  
> wrote:
> > On Sun, Jun 5, 2022 at 9:32 AM Stafford Horne  wrote:
> > > On Sun, Jun 05, 2022 at 10:58:14AM +0900, Stafford Horne wrote:
> > > It might be a good idea to revisit the qemu implementation and make
> > > sure that the extra byteswap is only inserted on m68k and not on
> > > other targets, but hopefully there are no new targets based on 
> > > goldfish
> > > anymore and we don't need to care.
> > >
> > > So, it seems that in addition to my patch we would need something in m68k 
> > > to
> > > switch it back to 'native' (big) endian?
> > >
> > > Looking at the m68k kernel/qemu interface I see:
> > >
> > > Pre 5.19:
> > >(data) <-- kernel(readl / little) <-- m68k qemu (native / big) - 
> > > RTC/PIC
> > >(data) <-- kernel(__raw_readl / big) <-- m68k qemu (native / big) - TTY
> > >
> > > 5.19:
> > >(data) <-- kernel(gf_ioread32 / big) <-- m68k qemu (native / big) - all
> > >
> > > The new fixes to add gf_ioread32/gf_iowrite32 fix this for goldfish and 
> > > m68k.
> > > This wouldn't have been an issue for little-endian platforms where 
> > > readl/writel
> > > were originally used.
> > >
> > > Why can't m68k switch to little-endian in qemu and the kernel?  The m68k 
> > > virt
> > > platform is not that old, 1 year? Are there a lot of users that this 
> > > would be a big
> > > problem?
> > >
> > > [1] 
> > > https://lore.kernel.org/lkml/CAK8P3a1oN8NrUjkh2X8jHQbyz42Xo6GSa=5n0gd6vqcxrjm...@mail.gmail.com/
> 
> Goldfish is a very old platform, as far as I know only the kernel port is new.
> I don't know when qemu started shipping goldfish, but changing it now would
> surely break compatibility with whatever OS the port was originally made for.

Hi Arnd,

As far as I can tell goldfish in qemu is not very old. There are 3 devices, 2 
were
added for the m68k virt machine, and 1 for riscv virt.

$ git lo -- hw/rtc/goldfish_rtc.c
2022-01-28 2f93d8b04a Peter Maydellrtc: Move RTC function prototypes to 
their own header 
2021-03-04 6b9409ba5f Laurent Vivier   goldfish_rtc: re-arm the alarm after 
migration 
2020-10-13 16b66c5626 Laurent Vivier   goldfish_rtc: change MemoryRegionOps 
endianness to DEVICE_NATIVE_ENDIAN 
2020-07-22 8380b3a453 Jessica Clarke   goldfish_rtc: Fix non-atomic read 
behaviour of TIME_LOW/TIME_HIGH 
2020-02-10 9a5b40b842 Anup Patel   hw: rtc: Add Goldfish RTC device 

$ git lo -- hw/char/goldfish_tty.c
2021-11-09 65b4c8c759 Philippe Mathieu-Daudé hw/m68k: Fix typo in SPDX tag 
2021-03-15 8c6df16ff6 Laurent Vivier   hw/char: add goldfish-tty 

$  git lo -- hw/intc/goldfish_pic.c
2021-11-09 65b4c8c759 Philippe Mathieu-Daudé hw/m68k: Fix typo in SPDX tag 
2021-03-15 8785559390 Laurent Vivier   hw/intc: add goldfish-pic 

The mips/loongson3_virt machine now also uses the goldfish_rtc.

The problem with the goldfish device models is that they were defined as
DEVICE_NATIVE_ENDIAN.

$ grep endianness hw/*/goldfish*
hw/char/goldfish_tty.c:.endianness = DEVICE_NATIVE_ENDIAN,
hw/intc/goldfish_pic.c:.endianness = DEVICE_NATIVE_ENDIAN,
hw/rtc/goldfish_rtc.c:.endianness = DEVICE_NATIVE_ENDIAN,

RISC-V is little-endian so when it was added there was no problem with running
linux goldfish drivers.

MIPS Longson3, added last year, seems to be running as little-endian well, I
understand MIPS can support both big and little endian. However according to
this all Loongson cores are little-endian.

https://en.wikipedia.org/wiki/Loongson

As I understand when endianness of the devices in qemu are defined as
DEVICE_NATIVE_ENDIAN the device endian takes the endian of the target CPU.
This means that MIPS Loongson3 and RISC-V are affectively running as
little-endian which is what would be expected.

So it appears to me that in qemu that m68k is the only architecture that is
providing goldfish devices on a big-endian architecture.  Also, as far as I
know Linux is the only OS that was updated to cater for that.  If there are
other firmware/bootloaders that expect that maybe they could be updated too?

-Stafford

Re: [PATCH v5 03/45] block/blklogwrites: don't care to remove bs->file child on failure

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

We don't need to remove bs->file, generic layer takes care of it. No
other driver cares to remove bs->file on failure by hand.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/blklogwrites.c | 4 
  1 file changed, 4 deletions(-)


Reviewed-by: Hanna Reitz 

Re: [PATCH 04/21] aspeed: i2c: Use reg array instead of individual vars

[Cédric Le Goater]

On 6/7/22 01:49, Joel Stanley wrote:

On Mon, 6 Jun 2022 at 15:08, Cédric Le Goater  wrote:


From: Joe Komlodi 

Using a register array will allow us to represent old-mode and new-mode
I2C registers by using the same underlying register array, instead of
adding an entire new set of variables to represent new mode.


The downside of this approach is you lose the safety of having
discrete types. A write to s->regs[R_FOO] can overwrite R_BAR.




As part of this, we also do additional cleanup to use ARRAY_FIELD_
macros instead of FIELD_ macros on registers.

Signed-off-by: Joe Komlodi 
Change-Id: Ib94996b17c361b8490c042b43c99d8abc69332e3
Message-Id: <20220331043248.2237838-5-koml...@google.com>
Signed-off-by: Cédric Le Goater 
---
  include/hw/i2c/aspeed_i2c.h |  11 +-
  hw/i2c/aspeed_i2c.c | 286 +---
  2 files changed, 133 insertions(+), 164 deletions(-)



@@ -858,12 +834,12 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
  {
  AspeedI2CBus *s = ASPEED_I2C_BUS(dev);

-s->intr_ctrl = 0;
-s->intr_status = 0;
-s->cmd = 0;
-s->buf = 0;
-s->dma_addr = 0;
-s->dma_len = 0;
+s->regs[R_I2CD_INTR_CTRL] = 0;
+s->regs[R_I2CD_INTR_STS] = 0;
+s->regs[R_I2CD_CMD] = 0;
+s->regs[R_I2CD_BYTE_BUF] = 0;
+s->regs[R_I2CD_DMA_ADDR] = 0;
+s->regs[R_I2CD_DMA_LEN] = 0;


Could this become a memset of s->regs?


yes. It should. I can take care of it.

Thanks,

C.




  i2c_end_transfer(s->bus);
  }

@@ -921,10 +897,10 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus 
*bus)
  static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
  {
  uint8_t *pool_page =
-&bus->controller->pool[FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL,
-  POOL_PAGE_SEL) * 0x100];
+&bus->controller->pool[ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL,
+POOL_PAGE_SEL) * 0x100];

-return &pool_page[FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, OFFSET)];
+return &pool_page[ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, OFFSET)];
  }

  static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
--
2.35.3

Re: [PATCH 37/71] target/arm: Add cpu properties for SME

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:33, Richard Henderson
 wrote:
>
> Mirror the properties for SVE.  The main difference is
> that any arbitrary set of powers of 2 may be supported,
> and not the stricter constraints that apply to SVE.
>
> Include a property to control FEAT_SME_FA64, as failing
> to restrict the runtime to the proper subset of insns
> could be a major point for bugs.
>
> Signed-off-by: Richard Henderson 

> @@ -589,10 +589,13 @@ static void cpu_arm_get_vq(Object *obj, Visitor *v, 
> const char *name,
>  ARMCPU *cpu = ARM_CPU(obj);
>  ARMVQMap *vq_map = opaque;
>  uint32_t vq = atoi(&name[3]) / 128;
> +bool sve = vq_map == &cpu->sve_vq;
>  bool value;
>
> -/* All vector lengths are disabled when SVE is off. */
> -if (!cpu_isar_feature(aa64_sve, cpu)) {
> +/* All vector lengths are disabled when feature is off. */
> +if (sve
> +? !cpu_isar_feature(aa64_sve, cpu)
> +: !cpu_isar_feature(aa64_sme, cpu)) {
>  value = false;
>  } else {
>  value = extract32(vq_map->map, vq - 1, 1);

I was wondering what you were going to do about this bit; the
comparison against &cpu->sve_vq feels a bit awkward but I guess
it's the simplest thing.

> +void arm_cpu_sme_finalize(ARMCPU *cpu, Error **errp)
> +{
> +uint32_t vq_map = cpu->sme_vq.map;
> +uint32_t vq_init = cpu->sme_vq.init;
> +uint32_t vq_supported = cpu->sme_vq.supported;
> +uint32_t vq;
> +
> +if (vq_map == 0) {
> +if (!cpu_isar_feature(aa64_sme, cpu)) {
> +cpu->isar.id_aa64smfr0 = 0;
> +return;
> +}
> +
> +/* TODO: KVM will require limitations via SMCR_EL2. */
> +vq_map = vq_supported & ~vq_init;

Do we currently forbid setting these properties entirely for KVM
(or just not provide them) ?

> +static void aarch64_add_sme_properties(Object *obj)
> +{
> +ARMCPU *cpu = ARM_CPU(obj);
> +uint32_t vq;
> +
> +object_property_add_bool(obj, "sme", cpu_arm_get_sme, cpu_arm_set_sme);
> +object_property_add_bool(obj, "sme_fa64", cpu_arm_get_sme_fa64,
> + cpu_arm_set_sme_fa64);
> +
> +for (vq = 1; vq <= ARM_MAX_VQ; vq <<= 1) {
> +char name[8];
> +sprintf(name, "sme%d", vq * 128);
> +object_property_add(obj, name, "bool", cpu_arm_get_vq,
> +cpu_arm_set_vq, NULL, &cpu->sme_vq);
> +}
> +
> +#ifdef CONFIG_USER_ONLY
> +/* Mirror linux /proc/sys/abi/sme_default_vector_length. */
> +object_property_add(obj, "sme-default-vector-length", "int32",
> +cpu_arm_get_default_vec_len,
> +cpu_arm_set_default_vec_len, NULL,
> +&cpu->sme_default_vq);
> +#endif
> +}

These new properties should be documented in
docs/system/arm/cpu-features.rst, similar to the SVE ones.

thanks
-- PMM

Re: [PATCH 1/5] hw/smbios: add core_count2 to smbios table type 4

[Igor Mammedov]

On Mon, 6 Jun 2022 13:11:36 +0200
Julia Suvorova  wrote:

> On Thu, Jun 2, 2022 at 4:35 PM Igor Mammedov  wrote:
> >
> > On Thu, 2 Jun 2022 16:31:25 +0200
> > Igor Mammedov  wrote:
> >  
> > > On Tue, 31 May 2022 14:40:15 +0200
> > > Julia Suvorova  wrote:
> > >  
> > > > On Sat, May 28, 2022 at 6:34 AM Ani Sinha  wrote:  
> > > > >
> > > > >
> > > > >
> > > > > On Fri, 27 May 2022, Julia Suvorova wrote:
> > > > >  
> > > > > > In order to use the increased number of cpus, we need to bring 
> > > > > > smbios
> > > > > > tables in line with the SMBIOS 3.0 specification. This allows us to
> > > > > > introduce core_count2 which acts as a duplicate of core_count if we 
> > > > > > have
> > > > > > fewer cores than 256, and contains the actual core number per 
> > > > > > socket if
> > > > > > we have more.
> > > > > >
> > > > > > core_enabled2 and thread_count2 fields work the same way.
> > > > > >
> > > > > > Signed-off-by: Julia Suvorova   
> > > > >
> > > > > Other than the comment below,
> > > > > Reviewed-by: Ani Sinha 
> > > > >  
> > > > > > ---
> > > > > >  include/hw/firmware/smbios.h |  3 +++
> > > > > >  hw/smbios/smbios.c   | 11 +--
> > > > > >  2 files changed, 12 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/include/hw/firmware/smbios.h 
> > > > > > b/include/hw/firmware/smbios.h
> > > > > > index 4b7ad77a44..c427ae5558 100644
> > > > > > --- a/include/hw/firmware/smbios.h
> > > > > > +++ b/include/hw/firmware/smbios.h
> > > > > > @@ -187,6 +187,9 @@ struct smbios_type_4 {
> > > > > >  uint8_t thread_count;
> > > > > >  uint16_t processor_characteristics;
> > > > > >  uint16_t processor_family2;
> > > > > > +uint16_t core_count2;
> > > > > > +uint16_t core_enabled2;
> > > > > > +uint16_t thread_count2;  
> >
> > on the other hand,
> > is it ok unconditionally extend type 4 and use v3 structure
> > if qemu was started with v2 smbios?  
> 
> We have a flag for the entry point type, not the smbios version per
> se. Additional fields added to structures were not previously tracked
> (for instance, processor_family2 is v2.6 and status is v2.0). AFAIU it
that's a bug, unless there is a very good reason to keep doing that,
I'd not do that.

> can affect only the total table length and the maximum structure size

table length is fixed depending on used version,
so if we follow it, we should set length and use part of structure
correctly if old version is specified (i.e.
   1. old structure + v30 structure,
   2. copy only a relevant part of v30 structure and
  fixup length when v2 version is asked for
though, I'd prefer #1

> (word). But if you like, I can raise an error (warning) if someone
> tries to start a vm with cpus > 255 and smbios v2.

it's a separate thing, I'd error out
(it will break users that use v2 with too may CPUs but then
they should fix their configuration to use v3)

> Best regards, Julia Suvorova.
> 
> > > > >
> > > > > I would add a comment along the lines of
> > > > > /* section 7.5, table 21 smbios spec version 3.0.0 */  
> > > >
> > > > Ok  
> > >
> > > With Ani's comment fixed
> > >
> > > Reviewed-by: Igor Mammedov 
> > >  
> > > >  
> > > > > >  } QEMU_PACKED;
> > > > > >
> > > > > >  /* SMBIOS type 11 - OEM strings */
> > > > > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > > > > > index 60349ee402..45d7be6b30 100644
> > > > > > --- a/hw/smbios/smbios.c
> > > > > > +++ b/hw/smbios/smbios.c
> > > > > > @@ -709,8 +709,15 @@ static void 
> > > > > > smbios_build_type_4_table(MachineState *ms, unsigned instance)
> > > > > >  SMBIOS_TABLE_SET_STR(4, serial_number_str, type4.serial);
> > > > > >  SMBIOS_TABLE_SET_STR(4, asset_tag_number_str, type4.asset);
> > > > > >  SMBIOS_TABLE_SET_STR(4, part_number_str, type4.part);
> > > > > > -t->core_count = t->core_enabled = ms->smp.cores;
> > > > > > -t->thread_count = ms->smp.threads;
> > > > > > +
> > > > > > +t->core_count = (ms->smp.cores > 255) ? 0xFF : ms->smp.cores;
> > > > > > +t->core_enabled = t->core_count;
> > > > > > +
> > > > > > +t->core_count2 = t->core_enabled2 = cpu_to_le16(ms->smp.cores);
> > > > > > +
> > > > > > +t->thread_count = (ms->smp.threads > 255) ? 0xFF : 
> > > > > > ms->smp.threads;
> > > > > > +t->thread_count2 = cpu_to_le16(ms->smp.threads);
> > > > > > +
> > > > > >  t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
> > > > > >  t->processor_family2 = cpu_to_le16(0x01); /* Other */
> > > > > >
> > > > > > --
> > > > > > 2.35.1
> > > > > >
> > > > > >  
> > > > >  
> > > >  
> > >  
> >  
> 

Re: [PATCH] Hexagon (target/hexagon) remove unused encodings

[Philippe Mathieu-Daudé via]

On 7/6/22 00:23, Taylor Simpson wrote:

Remove encodings guarded by ifdef that is not defined

Signed-off-by: Taylor Simpson 
---
  target/hexagon/imported/encode_pp.def | 23 ---
  1 file changed, 23 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 39/71] target/arm: Add SVL to TB flags

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:18, Richard Henderson
 wrote:
>
> We need SVL separate from VL for RDSVL at al, as well as

"et al"

> ZA storage loads and stores, which do not require PSTATE.SM.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/cpu.h   | 12 
>  target/arm/translate.h |  1 +
>  target/arm/helper.c|  8 +++-
>  target/arm/translate-a64.c |  1 +
>  4 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e41a75a3a3..0c32c3afaa 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3292,6 +3292,7 @@ FIELD(TBFLAG_A64, MTE0_ACTIVE, 19, 1)
>  FIELD(TBFLAG_A64, SMEEXC_EL, 20, 2)
>  FIELD(TBFLAG_A64, PSTATE_SM, 22, 1)
>  FIELD(TBFLAG_A64, PSTATE_ZA, 23, 1)
> +FIELD(TBFLAG_A64, SVL, 24, 4)

Given that both SVE and SME start with an 'S', maybe
"SME_VL" would be less prone to confusion? On the other hand,
SVL is the architectural name, so maybe that's best.

>  /*
>   * Helpers for using the above.
> @@ -3337,6 +3338,17 @@ static inline int sve_vq_cached(CPUARMState *env)
>  return EX_TBFLAG_A64(env->hflags, VL) + 1;
>  }
>
> +/**
> + * sme_vq_cached
> + * @env: the cpu context
> + *
> + * Return the SVL cached within env->hflags, in units of quadwords.
> + */
> +static inline int sme_vq_cached(CPUARMState *env)

Same remark as earlier about not needing to put "cached" in the function name.

Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 2/5] bios-tables-test: teach test to use smbios 3.0 tables

[Igor Mammedov]

On Mon, 6 Jun 2022 12:52:00 +0200
Julia Suvorova  wrote:

> On Thu, Jun 2, 2022 at 5:04 PM Igor Mammedov  wrote:
> >
> > On Fri, 27 May 2022 18:56:48 +0200
> > Julia Suvorova  wrote:
> >  
> > > Introduce the 64-bit entry point. Since we no longer have a total
> > > number of structures, stop checking for the new ones at the EOF
> > > structure (type 127).
> > >
> > > Signed-off-by: Julia Suvorova 
> > > ---
> > >  tests/qtest/bios-tables-test.c | 101 -
> > >  1 file changed, 75 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/tests/qtest/bios-tables-test.c 
> > > b/tests/qtest/bios-tables-test.c
> > > index a4a46e97f0..0ba9d749a5 100644
> > > --- a/tests/qtest/bios-tables-test.c
> > > +++ b/tests/qtest/bios-tables-test.c
> > > @@ -75,6 +75,14 @@
> > >  #define OEM_TEST_ARGS  "-machine x-oem-id=" OEM_ID 
> > > ",x-oem-table-id=" \
> > > OEM_TABLE_ID
> > >
> > > +#define SMBIOS_VER21 0
> > > +#define SMBIOS_VER30 1
> > > +
> > > +typedef struct {
> > > +struct smbios_21_entry_point ep21;
> > > +struct smbios_30_entry_point ep30;
> > > +} smbios_entry_point;
> > > +
> > >  typedef struct {
> > >  bool tcg_only;
> > >  const char *machine;
> > > @@ -88,8 +96,8 @@ typedef struct {
> > >  uint64_t rsdp_addr;
> > >  uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */];
> > >  GArray *tables;
> > > -uint32_t smbios_ep_addr;
> > > -struct smbios_21_entry_point smbios_ep_table;
> > > +uint64_t smbios_ep_addr[2];
> > > +smbios_entry_point smbios_ep_table;
> > >  uint16_t smbios_cpu_max_speed;
> > >  uint16_t smbios_cpu_curr_speed;
> > >  uint8_t *required_struct_types;
> > > @@ -533,10 +541,10 @@ static void test_acpi_asl(test_data *data)
> > >  free_test_data(&exp_data);
> > >  }
> > >
> > > -static bool smbios_ep_table_ok(test_data *data)
> > > +static bool smbios_ep2_table_ok(test_data *data)
> > >  {
> > > -struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> > > -uint32_t addr = data->smbios_ep_addr;
> > > +struct smbios_21_entry_point *ep_table = &data->smbios_ep_table.ep21;
> > > +uint32_t addr = data->smbios_ep_addr[SMBIOS_VER21];
> > >
> > >  qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> > >  if (memcmp(ep_table->anchor_string, "_SM_", 4)) {
> > > @@ -559,29 +567,59 @@ static bool smbios_ep_table_ok(test_data *data)
> > >  return true;
> > >  }
> > >
> > > -static void test_smbios_entry_point(test_data *data)
> > > +static bool smbios_ep3_table_ok(test_data *data)
> > > +{
> > > +struct smbios_30_entry_point *ep_table = &data->smbios_ep_table.ep30;
> > > +uint64_t addr = data->smbios_ep_addr[SMBIOS_VER30];
> > > +
> > > +qtest_memread(data->qts, addr, ep_table, sizeof(*ep_table));
> > > +if (memcmp(ep_table->anchor_string, "_SM3_", 5)) {
> > > +return false;
> > > +}
> > > +
> > > +if (acpi_calc_checksum((uint8_t *)ep_table, sizeof *ep_table)) {
> > > +return false;
> > > +}
> > > +
> > > +return true;
> > > +}
> > > +
> > > +static int test_smbios_entry_point(test_data *data)
> > >  {
> > >  uint32_t off;
> > > +bool found_ep2 = false, found_ep3 = false;
> > >
> > >  /* find smbios entry point structure */
> > >  for (off = 0xf; off < 0x10; off += 0x10) {
> > > -uint8_t sig[] = "_SM_";
> > > +uint8_t sig[] = "_SM3_";  
> >
> > well I'd just add a separate sig3  
> 
> Ok
> 
> > >  int i;
> > >
> > >  for (i = 0; i < sizeof sig - 1; ++i) {
> > >  sig[i] = qtest_readb(data->qts, off + i);
> > >  }
> > >
> > > -if (!memcmp(sig, "_SM_", sizeof sig)) {
> > > +if (!found_ep2 && !memcmp(sig, "_SM_", sizeof sig - 2)) {  
> >
> > keep original v2 code and just add similar chunk for v3,
> > drop found_foo locals,
> > that should make it easier to read/follow
> > (i.e. less conditions to think about and no magic fiddling with the length 
> > of signature)  
> 
> The idea was to reuse existing code, but since it doesn't improve
> things much, it makes sense to repeat it.
> 
> > >  /* signature match, but is this a valid entry point? */
> > > -data->smbios_ep_addr = off;
> > > -if (smbios_ep_table_ok(data)) {
> > > -break;
> > > +data->smbios_ep_addr[SMBIOS_VER21] = off;
> > > +if (smbios_ep2_table_ok(data)) {
> > > +found_ep2 = true;
> > > +}
> > > +} else if (!found_ep3 && !memcmp(sig, "_SM3_", sizeof sig - 1)) {
> > > +data->smbios_ep_addr[SMBIOS_VER30] = off;
> > > +if (smbios_ep3_table_ok(data)) {
> > > +found_ep3 = true;
> > >  }
> > >  }
> > > +
> > > +if (found_ep2 || found_ep3) {
> > > +break;
> > > +}
> > >  }
> > >
> > > -g_assert_cmphex(off, <, 0x10);
> > > +g_assert_cmphex(da

Re: [PATCH v5 02/45] block: introduce bdrv_open_file_child() helper

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

Almost all drivers call bdrv_open_child() similarly. Let's create a
helper for this.

The only not updated driver that call bdrv_open_child() to set
bs->file is raw-format, as it sometimes want to have filtered child but
don't set drv->is_filter to true.


Also snapshot-access, which uses DATA | PRIMARY.


Possibly we should implement drv->is_filter_func() handler, to consider
raw-format as filter when it works as filter.. But it's another story.

Note also, that we decrease assignments to bs->file in code: it helps
us restrict modifying this field in further commit.

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


[...]


diff --git a/block/filter-compress.c b/block/filter-compress.c
index d5be538619..b2cfa9a9a5 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -30,10 +30,8 @@
  static int compress_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
  {
-bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
-   BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
-   false, errp);
-if (!bs->file) {
+int ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+if (ret < 0) {
  return -EINVAL;


Should probably be `return ret;` like elsewhere.

With that done:

Reviewed-by: Hanna Reitz 

Re: [PATCH 0/7] More tests/tcg cleanups

[Paolo Bonzini]

On 6/7/22 11:40, Paolo Bonzini wrote:

Building on the introduction of config-$target.mak


Brain fart, or perhaps selective amnesia: building on the removal of 
Makefile.qemu.


Paolo


, make tests/tcg a
"regular" subdirectory that is entered simply with "make -C", like the
ROMs or the plugins.

Re: [PATCH 40/71] target/arm: Move pred_{full, gvec}_reg_{offset, size} to translate-a64.h

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:21, Richard Henderson
 wrote:
>
> We will need these functions in translate-sme.c.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 1/8] hw/cxl: Make the CXL fixed memory window setup a machine parameter.

[Jonathan Cameron via]

On Mon, 6 Jun 2022 10:24:43 -0700
Ben Widawsky  wrote:

> On 22-05-31 09:26:27, Paolo Bonzini wrote:
> > On 5/30/22 15:45, Jonathan Cameron via wrote:  
> > > +object_property_add(obj, "cxl-fmw", "CXLFixedMemoryWindow",
> > > +machine_get_cfmw, machine_set_cfmw,
> > > +NULL, state);
> > > +object_property_set_description(obj, "cxl-fmw",
> > > +"CXL Fixed Memory Window");  
> > 
> > Perhaps "CML fixed memory windows (array)" or something like that?
> > 
> > Paolo  
> 
> I had a mail which I apparently never sent. I'd like to see 'fmw' renamed, 
> since
> that has no decoder ring in any spec that I'm aware of.
> 
> Why not keep cfmws nomenclature? It's well defined.

IIRC s is for structure and this isn't a structure, so I dropped the s.

Expanding cxl makes this a lot more meaningful outside of CXL related
specifications.  I preferred the full wording as is currently upstream
but the command lines are insanely long given the new form.

Jonathan


> 
> Ben

Re: [PATCH 41/71] target/arm: Add infrastructure for disas_sme

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:41, Richard Henderson
 wrote:
>
> This includes the build rules for the decoder, and the
> new file for translation, but excludes any instructions.
>
> Signed-off-by: Richard Henderson 

> @@ -14814,7 +14814,12 @@ static void 
> aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>  }
>
>  switch (extract32(insn, 25, 4)) {
> -case 0x0: case 0x1: case 0x3: /* UNALLOCATED */
> +case 0x0:
> +if (!disas_sme(s, insn)) {
> +unallocated_encoding(s);
> +}
> +break;
> +case 0x1: case 0x3: /* UNALLOCATED */
>  unallocated_encoding(s);
>  break;
>  case 0x2:

This is grabbing slightly more of the encoding space than it should
according to the Arm ARM Table C4-1 "Main encoding table": SME
encodings require bit 31 == 1 (unlike SVE where bit 31 is not decoded
at this level).

Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Arnd Bergmann]

On Tue, Jun 7, 2022 at 11:47 AM Stafford Horne  wrote:
> On Tue, Jun 07, 2022 at 10:42:08AM +0200, Arnd Bergmann wrote:

> > Goldfish is a very old platform, as far as I know only the kernel port is 
> > new.
> > I don't know when qemu started shipping goldfish, but changing it now would
> > surely break compatibility with whatever OS the port was originally made 
> > for.
>
> Hi Arnd,
>
> As far as I can tell goldfish in qemu is not very old. There are 3 devices, 2 
> were
> added for the m68k virt machine, and 1 for riscv virt.
>
> $ git lo -- hw/char/goldfish_tty.c
> 2021-11-09 65b4c8c759 Philippe Mathieu-Daudé hw/m68k: Fix typo in SPDX tag
> 2021-03-15 8c6df16ff6 Laurent Vivier   hw/char: add goldfish-tty
>
> $  git lo -- hw/intc/goldfish_pic.c
> 2021-11-09 65b4c8c759 Philippe Mathieu-Daudé hw/m68k: Fix typo in SPDX tag
> 2021-03-15 8785559390 Laurent Vivier   hw/intc: add goldfish-pic

That is much younger than Laurent made it appear, from his earlier explanations
I expected this to have shipped a long time ago and been used in other
OSs to the
point where it cannot be fixed.

> The mips/loongson3_virt machine now also uses the goldfish_rtc.
>
> The problem with the goldfish device models is that they were defined as
> DEVICE_NATIVE_ENDIAN.
>
> $ grep endianness hw/*/goldfish*
> hw/char/goldfish_tty.c:.endianness = DEVICE_NATIVE_ENDIAN,
> hw/intc/goldfish_pic.c:.endianness = DEVICE_NATIVE_ENDIAN,
> hw/rtc/goldfish_rtc.c:.endianness = DEVICE_NATIVE_ENDIAN,
>
> RISC-V is little-endian so when it was added there was no problem with running
> linux goldfish drivers.
>
> MIPS Longson3, added last year, seems to be running as little-endian well, I
> understand MIPS can support both big and little endian. However according to
> this all Loongson cores are little-endian.
>
> https://en.wikipedia.org/wiki/Loongson
>
> As I understand when endianness of the devices in qemu are defined as
> DEVICE_NATIVE_ENDIAN the device endian takes the endian of the target CPU.
>
> This means that MIPS Loongson3 and RISC-V are affectively running as
> little-endian which is what would be expected.

Not really, the definition of DEVICE_NATIVE_ENDIAN in qemu is much less
well-defined than that as I understand, it is just whatever the person adding
support for that CPU thought would be the right one. A lot of CPUs can
run either big-endian or little-endian code, and e.g. on ARM, qemu
DEVICE_NATIVE_ENDIAN is just always little-endian, regardless of
what the CPU runs, while I think on MIPS it would be whatever the CPU
is actually executing.

  Arnd

Re: [PATCH] Hexagon (target/hexagon) make VyV operands use a unique temp

[Philippe Mathieu-Daudé via]

On 7/6/22 00:23, Taylor Simpson wrote:

VyV operand is only used in the vshuff and vdeal instructions.  These
instructions write to both VyV and VxV operands.  In the case where
both operands are the same register, we need a separate location for
VyV.  We use the existing vtmp field in CPUHexagonState.

Test case added in tests/tcg/hexagon/hvx_misc.c

Signed-off-by: Taylor Simpson 
---
  tests/tcg/hexagon/hvx_misc.c| 45 +
  target/hexagon/gen_tcg_funcs.py |  9 +++
  2 files changed, 49 insertions(+), 5 deletions(-)



+static void test_vshuff(void)
+{
+/* Test that vshuff works when the two operands are the same register */
+const uint32_t splat = 0x089be55c;
+const uint32_t shuff = 0x454fa926;
+MMVector v0, v1;
+
+memset(expect, 0x12, sizeof(MMVector));
+memset(output, 0x34, sizeof(MMVector));
+
+asm volatile("v25 = vsplat(%0)\n\t"
+ "vshuff(v25, v25, %1)\n\t"
+ "vmem(%2 + #0) = v25\n\t"
+ : /* no outputs */
+ : "r"(splat), "r"(shuff), "r"(output)
+ : "v25", "memory");
+
+/*
+ * The semantics of Hexagon are the operands are pass-by-value, so create
+ * two copies of the vsplat result.
+ */
+for (int i = 0; i < MAX_VEC_SIZE_BYTES / 4; i++) {


Nitpicking, possibly 4 -> sizeof(v0.uw[0]).


+v0.uw[i] = splat;
+v1.uw[i] = splat;
+}


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] avocado/boot_linux_console.py: Update ast2600 test

[Philippe Mathieu-Daudé via]

On 7/6/22 03:19, Joel Stanley wrote:

Update the test_arm_ast2600_debian test to

  - the latest Debian kernel
  - use the Rainier machine instead of Tacoma


Why can't we keep both?


Both of which contains support for more hardware and thus exercises more
of the hardware Qemu models.


"QEMU"


Signed-off-by: Joel Stanley 
---
  tests/avocado/boot_linux_console.py | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/avocado/boot_linux_console.py 
b/tests/avocado/boot_linux_console.py
index 6b1533c17c8b..477b9b887754 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -1046,18 +1046,18 @@ def test_arm_vexpressa9(self):
  def test_arm_ast2600_debian(self):
  """
  :avocado: tags=arch:arm
-:avocado: tags=machine:tacoma-bmc
+:avocado: tags=machine:rainier-bmc
  """
  deb_url = ('http://snapshot.debian.org/archive/debian/'
-   '20210302T203551Z/'
+   '20220606T211338Z/'
 'pool/main/l/linux/'
-   'linux-image-5.10.0-3-armmp_5.10.13-1_armhf.deb')
-deb_hash = 
'db40d32fe39255d05482bea48d72467b67d6225bb2a2a4d6f618cb8976f1e09e'
+   'linux-image-5.17.0-2-armmp_5.17.6-1%2Bb1_armhf.deb')
+deb_hash = 
'8acb2b4439faedc2f3ed4bdb2847ad4f6e0491f73debaeb7f660c8abe4dcdc0e'
  deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash,
  algorithm='sha256')
-kernel_path = self.extract_from_deb(deb_path, 
'/boot/vmlinuz-5.10.0-3-armmp')
+kernel_path = self.extract_from_deb(deb_path, 
'/boot/vmlinuz-5.17.0-2-armmp')
  dtb_path = self.extract_from_deb(deb_path,
-
'/usr/lib/linux-image-5.10.0-3-armmp/aspeed-bmc-opp-tacoma.dtb')
+
'/usr/lib/linux-image-5.17.0-2-armmp/aspeed-bmc-ibm-rainier.dtb')
  
  self.vm.set_console()

  self.vm.add_args('-kernel', kernel_path,

Re: [PATCH 4/5] bios-tables-test: add test for number of cores > 255

[Igor Mammedov]

On Mon, 6 Jun 2022 13:38:57 +0200
Julia Suvorova  wrote:

> On Thu, Jun 2, 2022 at 5:20 PM Igor Mammedov  wrote:
> >
> > On Fri, 27 May 2022 18:56:50 +0200
> > Julia Suvorova  wrote:
> >  
> > > The new test is run with a large number of cpus and checks if the
> > > core_count field in smbios_cpu_test (structure type 4) is correct.
> > >
> > > Choose q35 as it allows to run with -smp > 255.
> > >
> > > Signed-off-by: Julia Suvorova 
> > > ---
> > >  tests/qtest/bios-tables-test.c | 35 +-
> > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tests/qtest/bios-tables-test.c 
> > > b/tests/qtest/bios-tables-test.c
> > > index 0ba9d749a5..f2464adaa0 100644
> > > --- a/tests/qtest/bios-tables-test.c
> > > +++ b/tests/qtest/bios-tables-test.c
> > > @@ -100,6 +100,8 @@ typedef struct {
> > >  smbios_entry_point smbios_ep_table;
> > >  uint16_t smbios_cpu_max_speed;
> > >  uint16_t smbios_cpu_curr_speed;
> > > +uint8_t smbios_core_count;
> > > +uint16_t smbios_core_count2;
> > >  uint8_t *required_struct_types;
> > >  int required_struct_types_len;
> > >  QTestState *qts;
> > > @@ -640,8 +642,9 @@ static inline bool smbios_single_instance(uint8_t 
> > > type)
> > >
> > >  static bool smbios_cpu_test(test_data *data, uint32_t addr)
> > >  {
> > > +uint8_t real_cc, expect_cc = data->smbios_core_count;  
> >
> > %s/expect/expected/
> > also I'd s/real_cc/core_count/
> >  
> > > +uint16_t real, real_cc2, expect_cc2 = data->smbios_core_count2;  
> > ditto
> >  
> > >  uint16_t expect_speed[2];
> > > -uint16_t real;
> > >  int offset[2];
> > >  int i;
> > >
> > > @@ -660,6 +663,20 @@ static bool smbios_cpu_test(test_data *data, 
> > > uint32_t addr)
> > >  }
> > >  }
> > >
> > > +real_cc = qtest_readb(data->qts, addr + offsetof(struct 
> > > smbios_type_4, core_count));
> > > +real_cc2 = qtest_readw(data->qts, addr + offsetof(struct 
> > > smbios_type_4, core_count2));
> > > +
> > > +if (expect_cc && (real_cc != expect_cc)) {
> > > +fprintf(stderr, "Unexpected SMBIOS CPU count: real %u expect 
> > > %u\n",
> > > +real_cc, expect_cc);
> > > +return false;  
> >
> > since you are rewriting it anyways, how about
> > if (expect_cc) {
> >   g_assert_cmpuint(...)
> > }
> >
> > instead of printing/propagating error  
> 
> That works. But I still need to return something, unless you want to
> change the original code too.

just change it, as it makes code simpler (maybe a separate patch
that should go before this one)

> 
> Best regards, Julia Suvorova.
> 
> > > +}
> > > +if ((expect_cc == 0xFF) && (real_cc2 != expect_cc2)) {
> > > +fprintf(stderr, "Unexpected SMBIOS CPU count2: real %u expect 
> > > %u\n",
> > > +real_cc2, expect_cc2);
> > > +return false;
> > > +}
> > > +
> > >  return true;
> > >  }
> > >
> > > @@ -905,6 +922,21 @@ static void test_acpi_q35_tcg(void)
> > >  free_test_data(&data);
> > >  }
> > >
> > > +static void test_acpi_q35_tcg_core_count2(void)
> > > +{
> > > +test_data data = {
> > > +.machine = MACHINE_Q35,
> > > +.variant = ".core-count2",
> > > +.required_struct_types = base_required_struct_types,
> > > +.required_struct_types_len = 
> > > ARRAY_SIZE(base_required_struct_types),
> > > +.smbios_core_count = 0xFF,
> > > +.smbios_core_count2 = 275,
> > > +};
> > > +
> > > +test_acpi_one("-machine smbios-entry-point-type=64 -smp 275", &data);
> > > +free_test_data(&data);
> > > +}
> > > +
> > >  static void test_acpi_q35_tcg_bridge(void)
> > >  {
> > >  test_data data;
> > > @@ -1787,6 +1819,7 @@ int main(int argc, char *argv[])
> > >  qtest_add_func("acpi/piix4/pci-hotplug/off",
> > > test_acpi_piix4_no_acpi_pci_hotplug);
> > >  qtest_add_func("acpi/q35", test_acpi_q35_tcg);
> > > +qtest_add_func("acpi/q35/core-count2", 
> > > test_acpi_q35_tcg_core_count2);
> > >  qtest_add_func("acpi/q35/bridge", test_acpi_q35_tcg_bridge);
> > >  qtest_add_func("acpi/q35/multif-bridge", 
> > > test_acpi_q35_multif_bridge);
> > >  qtest_add_func("acpi/q35/mmio64", test_acpi_q35_tcg_mmio64);  
> >  
> 

Re: [PATCH] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Jonathan Cameron via]

On Mon, 6 Jun 2022 10:39:52 -0700
Ben Widawsky  wrote:

> On 22-05-31 13:39:53, Jonathan Cameron wrote:
> > Without being able to write these registers, no interleaving is possible.
> > More refined checks of HDM register state on commit to follow.
> > 
> > Signed-off-by: Jonathan Cameron 
> > ---
> >  hw/cxl/cxl-component-utils.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index 7985c9bfca..993248b5c0 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -174,6 +174,8 @@ static void hdm_init_common(uint32_t *reg_state, 
> > uint32_t *write_msk)
> >  write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
> >  write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
> >  write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > 0x;
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 
> > 0x;  
> 
> I wonder if this should be 0. It will be weird for endpoints to have a skip
> value of 0xff.

For EP _LO should be 0xf000. But we haven't implemented skip yet IIRC.

It should be all bits set for host bridges (or switches) and that's the
bug this is fixing.

We have access to the device type at the caller of this function, so I can make 
it
right for both changes with a trivial change.

Will send a v2 shortly...

Thanks,

Jonathan

> 
> >  }
> >  }
> >  
> > -- 
> > 2.32.0
> >   

Re: [PATCH 11/21] test/avocado/machine_aspeed.py: Add an I2C RTC test

[Cédric Le Goater]

On 6/7/22 01:16, Joel Stanley wrote:

On Mon, 6 Jun 2022 at 15:08, Cédric Le Goater  wrote:


Add a RTC device on bus 15 and check that the ouput of the hwclock


spelling: output


command matches the current year.

Signed-off-by: Cédric Le Goater 


Reviewed-by: Joel Stanley 


---
  tests/avocado/machine_aspeed.py | 8 
  1 file changed, 8 insertions(+)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index a3b4b9e5093c..28b8a4c8124b 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -136,10 +136,18 @@ def test_arm_ast2600_evb_builroot(self):

  self.vm.add_args('-device',
   'tmp423,bus=aspeed.i2c.bus.15,address=0x4c');
+self.vm.add_args('-device',
+ 'ds1338,bus=aspeed.i2c.bus.15,address=0x32');


Is there any value running this on the 2400 and 2500 machine types
too?


We could do that, yes. Send patches !


They all use the same model so perhaps not?


Currently, all models are exercised more or less in the same way by
the upstream Linux driver. Things are different for the AST1030 using
Zephir and for the AST2600 when using newer drivers from the SDK.

These images seem to be using the new AST2600 register mode:

  https://github.com/AspeedTech-BMC/openbmc/releases/

  root@ast2600-default:~# dmesg | grep i2c
  [0.211289] i2c global registered
  [1.442027] i2c_dev: i2c /dev entries driver
  [1.447944] i2c_new_aspeed 1e78a280.i2c-bus: NEW-I2C: i2c-bus [4]: adapter 
[100 khz] mode [2]
  [1.451158] ipmb-dev 5-0010: i2c_slave_register: client slave flag not 
set. You might see address collisions
  [1.451660] i2c_new_aspeed 1e78a300.i2c-bus: NEW-I2C: i2c-bus [5]: adapter 
[100 khz] mode [2]
  [1.454567] ipmb-dev 6-0012: i2c_slave_register: client slave flag not 
set. You might see address collisions
  [1.454938] i2c_new_aspeed 1e78a380.i2c-bus: NEW-I2C: i2c-bus [6]: adapter 
[100 khz] mode [2]
  [1.462953] i2c_new_aspeed 1e78a400.i2c-bus: NEW-I2C: i2c-bus [7]: adapter 
[95 khz] mode [2]
  [1.466394] i2c_new_aspeed 1e78a480.i2c-bus: NEW-I2C: i2c-bus [8]: adapter 
[100 khz] mode [2]
  [1.468394] i2c_new_aspeed 1e78a500.i2c-bus: NEW-I2C: i2c-bus [9]: adapter 
[100 khz] mode [2]

It could be an additional avocado test.


C.



  self.do_test_arm_aspeed_buidroot_start(image_path, '0xf00')
  exec_command_and_wait_for_pattern(self,
'i2cget -y 15 0x4c 0xff', '0x23');
  exec_command_and_wait_for_pattern(self,
'i2cget -y 15 0x4c 0xfe', '0x55');

+exec_command_and_wait_for_pattern(self,
+ 'echo ds1307 0x32 > /sys/class/i2c-dev/i2c-15/device/new_device',
+ 'i2c i2c-15: new_device: Instantiated device ds1307 at 0x32');
+year = time.strftime("%Y")
+exec_command_and_wait_for_pattern(self, 'hwclock -f /dev/rtc1', year);
+
  self.do_test_arm_aspeed_buidroot_poweroff()
--
2.35.3

[PATCH] configure: update list of preserved environment variables

[Paolo Bonzini]

INSTALL and LIBTOOL are not used anymore, but OBJCFLAGS is new and
was not listed.

Signed-off-by: Paolo Bonzini 
---
 configure | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/configure b/configure
index f35847c3cd..ce81419629 100755
--- a/configure
+++ b/configure
@@ -2737,13 +2737,12 @@ preserve_env CC
 preserve_env CFLAGS
 preserve_env CXX
 preserve_env CXXFLAGS
-preserve_env INSTALL
 preserve_env LD
 preserve_env LDFLAGS
 preserve_env LD_LIBRARY_PATH
-preserve_env LIBTOOL
 preserve_env MAKE
 preserve_env NM
+preserve_env OBJCFLAGS
 preserve_env OBJCOPY
 preserve_env PATH
 preserve_env PKG_CONFIG
-- 
2.36.1

[PATCH] configure: ignore --make

[Paolo Bonzini]

Setting the MAKE variable to a GNU Make executable does not really have
any effect: if a non-GNU Make is used, the QEMU Makefile will fail to
parse.  Just remove everything related to --make and $make as dead code.

Signed-off-by: Paolo Bonzini 
---
 configure | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/configure b/configure
index ce81419629..154e041b8e 100755
--- a/configure
+++ b/configure
@@ -493,20 +493,16 @@ gnu/kfreebsd)
 ;;
 freebsd)
   bsd="yes"
-  make="${MAKE-gmake}"
   # needed for kinfo_getvmmap(3) in libutil.h
 ;;
 dragonfly)
   bsd="yes"
-  make="${MAKE-gmake}"
 ;;
 netbsd)
   bsd="yes"
-  make="${MAKE-gmake}"
 ;;
 openbsd)
   bsd="yes"
-  make="${MAKE-gmake}"
 ;;
 darwin)
   bsd="yes"
@@ -517,7 +513,6 @@ darwin)
 ;;
 sunos)
   solaris="yes"
-  make="${MAKE-gmake}"
 # needed for CMSG_ macros in sys/socket.h
   QEMU_CFLAGS="-D_XOPEN_SOURCE=600 $QEMU_CFLAGS"
 # needed for TIOCWIN* defines in termios.h
@@ -628,8 +623,6 @@ case "$cpu" in
 CPU_CFLAGS="-m64 -mcpu=ultrasparc" ;;
 esac
 
-: ${make=${MAKE-make}}
-
 # We prefer python 3.x. A bare 'python' is traditionally
 # python 2.x, but some distros have it as python 3.x, so
 # we check that too
@@ -709,7 +702,7 @@ for opt do
   ;;
   --objcc=*) objcc="$optarg"
   ;;
-  --make=*) make="$optarg"
+  --make=*)
   ;;
   --install=*)
   ;;
@@ -1024,7 +1017,6 @@ Advanced options (experts only):
   --cross-cc-ARCH=CC   use compiler when building ARCH guest test cases
   --cross-cc-cflags-ARCH=  use compiler flags when building ARCH guest tests
   --cross-prefix-ARCH=PREFIX cross compiler prefix when building ARCH guest 
test cases
-  --make=MAKE  use specified make [$make]
   --python=PYTHON  use specified python [$python]
   --meson=MESONuse specified meson [$meson]
   --ninja=NINJAuse specified ninja [$ninja]
@@ -1079,10 +1071,6 @@ if test -z "$python"
 then
 error_exit "Python not found. Use --python=/path/to/python"
 fi
-if ! has "$make"
-then
-error_exit "GNU make ($make) not found"
-fi
 
 # Note that if the Python conditional here evaluates True we will exit
 # with status 1 which is a shell 'false' value.
@@ -2409,7 +2397,6 @@ if test "$container" != no; then
 echo "ENGINE=$container" >> $config_host_mak
 fi
 echo "ROMS=$roms" >> $config_host_mak
-echo "MAKE=$make" >> $config_host_mak
 echo "PYTHON=$python" >> $config_host_mak
 echo "GENISOIMAGE=$genisoimage" >> $config_host_mak
 echo "MESON=$meson" >> $config_host_mak
@@ -2740,7 +2727,6 @@ preserve_env CXXFLAGS
 preserve_env LD
 preserve_env LDFLAGS
 preserve_env LD_LIBRARY_PATH
-preserve_env MAKE
 preserve_env NM
 preserve_env OBJCFLAGS
 preserve_env OBJCOPY
-- 
2.36.1

Re: [PATCH v2 0/8] hw/cxl: Move CXL emulation options and state to machines.

[Jonathan Cameron via]

On Mon, 6 Jun 2022 10:33:38 -0700
Ben Widawsky  wrote:

> On 22-06-01 17:42:27, Jonathan Cameron wrote:
> > Changes since v1 (thanks to Paolo Bonzini)
> > * Update 'description' of cxl-fmw as suggested to mention it's an array.
> > * Add a wrapper cxl_hook_up_pxb_registers() to cxl-host.c as it'll be common
> >   for all machines using CXL with PXB.
> > 
> > Run through the CI at:
> > https://gitlab.com/jic23/qemu/-/pipelines/553257456
> >  
> > V1 Cover letter:
> > 
> > Currently only machine with CXL support upstream is i386/pc but arm/virt
> > patches have been posted and once this is merged an updated series will
> > follow. Switch support is queued behind this as well because they both
> > include documentation updates.
> > 
> > Paolo Bonzini highlighted a couple of issues with the current CXL
> > emulation code.
> > 
> > * Top level parameter rather than machine for fixed memory windows
> > 
> >   The --cxl-fixed-memory-window top level command line parameters won't play
> >   well with efforts to make it possible to instantiate entire machines via
> >   RPC. Better to move these to be machine configuration.  This change is
> >   relatively straight forward, but does result in very long command lines
> >   (cannot break fixed window setup into multiple -M entries).
> > 
> > * Move all CXL stuff to machine specific code and helpers
> > 
> >   To simplify the various interactions between machine setup and host
> >   bridges etc, currently various CXL steps are called from the generic
> >   core/machine.c and softmmu/vl.c + there are CXL elements in MachineState.
> > 
> >   Much of this is straight forward to do with one exception:
> >   The CXL pci_expander_bridge host bridges require MMIO register space.
> >   This series does this by walking the bus and filling the register space
> >   in via the machine_done callback. This is similar to the walk done for
> >   identifying host bridges in the ACPI building code but it is rather ugly
> >   and postpones rejection of PXB_CXL instances where cxl=off (default).
> > 
> > All comments welcome, but the first patch at least changes the command-line
> > so to avoid have to add backwards compatibility code, it would be great
> > to merge that before 7.1 is released.
> >   
> 
> LGTM overall. I'm not thrilled with introducing another [sub]scronym "fmw", 
> but
> otherwise, no complaints.

Agreed, it's a less than ideal compromise :( 

Spelling it out now seems too long as it has to be repeated a lot in a given 
commandline.
I think cfmw is too vague for a QEMU parameter and for CXL only the ACPI 
"structure"
(hence the s) has defined acronym of CFMWS.  The underlying actual routing 
hardware
is impdef so never given an acronym (there is only one direct reference to the
window itself in the CXL spec and that's spelled out as "fixed-memory window").

Jonathan



> Series is:
> Reviewed-by: Ben Widawsky 
> 
> > Thanks,
> > 
> > Jonathan
> > 
> > Jonathan Cameron (8):
> >   hw/cxl: Make the CXL fixed memory window setup a machine parameter.
> >   hw/acpi/cxl: Pass in the CXLState directly rather than MachineState
> >   hw/cxl: Push linking of CXL targets into i386/pc rather than in
> > machine.c
> >   tests/acpi: Allow modification of q35 CXL CEDT table.
> >   pci/pci_expander_bridge: For CXL HB delay the HB register memory
> > region setup.
> >   tests/acpi: Update q35/CEDT.cxl for new memory addresses.
> >   hw/cxl: Move the CXLState from MachineState to machine type specific
> > state.
> >   hw/machine: Drop cxl_supported flag as no longer useful
> > 
> >  docs/system/devices/cxl.rst |   4 +-
> >  hw/acpi/cxl.c   |   9 +-
> >  hw/core/machine.c   |  28 --
> >  hw/cxl/cxl-host-stubs.c |   9 +-
> >  hw/cxl/cxl-host.c   | 100 ++--
> >  hw/i386/acpi-build.c|   8 +-
> >  hw/i386/pc.c|  31 +++---
> >  hw/pci-bridge/meson.build   |   5 +-
> >  hw/pci-bridge/pci_expander_bridge.c |  32 ---
> >  hw/pci-bridge/pci_expander_bridge_stubs.c   |  14 +++
> >  include/hw/acpi/cxl.h   |   5 +-
> >  include/hw/boards.h |   3 +-
> >  include/hw/cxl/cxl.h|   9 +-
> >  include/hw/cxl/cxl_host.h   |  23 +
> >  include/hw/i386/pc.h|   2 +
> >  include/hw/pci-bridge/pci_expander_bridge.h |  12 +++
> >  qapi/machine.json   |  13 +++
> >  softmmu/vl.c|  46 -
> >  tests/data/acpi/q35/CEDT.cxl| Bin 184 -> 184 bytes
> >  tests/qtest/bios-tables-test.c  |   4 +-
> >  tests/qtest/cxl-test.c  |   4 +-
> >  21 files changed, 222 insertions(+), 139 deletions(-)
> >  create mode 100644 hw/pci-bridge/pci_expander_bridge_stubs.c
> >  create mode 100644 in

Re: [PATCH 04/21] aspeed: i2c: Use reg array instead of individual vars

[Cédric Le Goater]

On 6/7/22 01:49, Joel Stanley wrote:

On Mon, 6 Jun 2022 at 15:08, Cédric Le Goater  wrote:


From: Joe Komlodi 

Using a register array will allow us to represent old-mode and new-mode
I2C registers by using the same underlying register array, instead of
adding an entire new set of variables to represent new mode.


The downside of this approach is you lose the safety of having
discrete types. A write to s->regs[R_FOO] can overwrite R_BAR.




As part of this, we also do additional cleanup to use ARRAY_FIELD_
macros instead of FIELD_ macros on registers.

Signed-off-by: Joe Komlodi 
Change-Id: Ib94996b17c361b8490c042b43c99d8abc69332e3
Message-Id: <20220331043248.2237838-5-koml...@google.com>
Signed-off-by: Cédric Le Goater 
---
  include/hw/i2c/aspeed_i2c.h |  11 +-
  hw/i2c/aspeed_i2c.c | 286 +---
  2 files changed, 133 insertions(+), 164 deletions(-)



@@ -858,12 +834,12 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
  {
  AspeedI2CBus *s = ASPEED_I2C_BUS(dev);

-s->intr_ctrl = 0;
-s->intr_status = 0;
-s->cmd = 0;
-s->buf = 0;
-s->dma_addr = 0;
-s->dma_len = 0;
+s->regs[R_I2CD_INTR_CTRL] = 0;
+s->regs[R_I2CD_INTR_STS] = 0;
+s->regs[R_I2CD_CMD] = 0;
+s->regs[R_I2CD_BYTE_BUF] = 0;
+s->regs[R_I2CD_DMA_ADDR] = 0;
+s->regs[R_I2CD_DMA_LEN] = 0;


Could this become a memset of s->regs?


yes. I will do it.

Thanks,

C.




  i2c_end_transfer(s->bus);
  }

@@ -921,10 +897,10 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus 
*bus)
  static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
  {
  uint8_t *pool_page =
-&bus->controller->pool[FIELD_EX32(bus->ctrl, I2CD_FUN_CTRL,
-  POOL_PAGE_SEL) * 0x100];
+&bus->controller->pool[ARRAY_FIELD_EX32(bus->regs, I2CD_FUN_CTRL,
+POOL_PAGE_SEL) * 0x100];

-return &pool_page[FIELD_EX32(bus->pool_ctrl, I2CD_POOL_CTRL, OFFSET)];
+return &pool_page[ARRAY_FIELD_EX32(bus->regs, I2CD_POOL_CTRL, OFFSET)];
  }

  static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
--
2.35.3

[PATCH v2] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Jonathan Cameron via]

Without being able to write these registers, no interleaving is possible.
More refined checks of HDM register state on commit to follow.

Signed-off-by: Jonathan Cameron 
---
v2: (Ben Widawsky)
- Correctly set a tighter write mask for the endpoint devices where this
  register has a different use.

 hw/cxl/cxl-component-utils.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 7985c9bfca..40a0f752f2 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, uint32_t 
*write_msk)
 reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
 }
 
-static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
+static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
+enum reg_type type)
 {
 int decoder_count = 1;
 int i;
@@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t 
*write_msk)
 write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
 write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
 write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
+if (type == CXL2_DEVICE) {
+write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
0xf000;
+} else {
+write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
0x;
+}
+write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 0x;
 }
 }
 
-- 
2.32.0

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Peter Maydell]

On Tue, 7 Jun 2022 at 11:12, Stafford Horne  wrote:
>
> On Tue, Jun 07, 2022 at 10:42:08AM +0200, Arnd Bergmann wrote:
> > Goldfish is a very old platform, as far as I know only the kernel port is 
> > new.
> > I don't know when qemu started shipping goldfish, but changing it now would
> > surely break compatibility with whatever OS the port was originally made 
> > for.
>
> Hi Arnd,
>
> As far as I can tell goldfish in qemu is not very old. There are 3 devices, 2 
> were
> added for the m68k virt machine, and 1 for riscv virt.

Yep, these are new for (upstream) QEMU, and AIUI the only OS we care
about for these is Linux, really. My understanding is that these devices
were added because they were conveniently available in Linux :-)
Where they do have a much older history is in the Android emulator,
but upstream QEMU doesn't care about that.

> The problem with the goldfish device models is that they were defined as
> DEVICE_NATIVE_ENDIAN.
>
> $ grep endianness hw/*/goldfish*
> hw/char/goldfish_tty.c:.endianness = DEVICE_NATIVE_ENDIAN,
> hw/intc/goldfish_pic.c:.endianness = DEVICE_NATIVE_ENDIAN,
> hw/rtc/goldfish_rtc.c:.endianness = DEVICE_NATIVE_ENDIAN,
>
> RISC-V is little-endian so when it was added there was no problem with running
> linux goldfish drivers.
>
> MIPS Longson3, added last year, seems to be running as little-endian well, I
> understand MIPS can support both big and little endian. However according to
> this all Loongson cores are little-endian.
>
> https://en.wikipedia.org/wiki/Loongson
>
> As I understand when endianness of the devices in qemu are defined as
> DEVICE_NATIVE_ENDIAN the device endian takes the endian of the target CPU.
> This means that MIPS Loongson3 and RISC-V are affectively running as
> little-endian which is what would be expected.

DEVICE_NATIVE_ENDIAN means "whatever the 'native' endianness of the target
CPU architecture is". This is a compile-time thing, and doesn't change
if the CPU changes its endianness at runtime. So for instance for Arm
boards DEVICE_NATIVE_ENDIAN and DEVICE_LITTLE_ENDIAN are the same thing,
even if the guest OS is running with SCTLR_EL1.EE set (and even if a
particular board in qemu-system-arm sets up the CPU so it leaves reset
with .EE set to 1) The analogy on real hardware is that the way these
"switch endianness" CPUs work is that they just flip the bytes in the
data on their way out of the CPU, so changing the endianness in the CPU
doesn't cause devices to change the way they behave. QEMU's "target
endianness" is kind of like a property of the interconnect/system design
in some ways.

>From QEMU's point of view, the thing we really don't want is devices
that magically change behaviour when the CPU switches endianness
at runtime, because those are weirdly unlike real hardware. (Virtio
is the main offender in this regard, but we're stuck with that.)
Devices that happen to be wired up differently on different target
architectures are fine for us. I don't have any definite examples
to hand, but my understanding is that this happens with real hardware
too, where a device (maybe 8250-compatible UART or Lance ethernet
are examples?) with 32-bit registers might be typically wired up in
a system for a big-endian CPU such that the guest code can write
a 32-bit word to it and get the "obvious" ordering matching the
device datasheet. This sort of thing is what DEVICE_NATIVE_ENDIAN
was intended for. (There are also various places where we use it
where perhaps we should not where a device is exclusively used
on a CPU of a particular endianness, and so you could equally write
DEVICE_LITTLE_ENDIAN or whatever without any behaviour change.)

So I don't have a strong view on whether these devices should
be DEVICE_NATIVE_ENDIAN or DEVICE_LITTLE_ENDIAN (except that
my impression is that a DEVICE_LITTLE_ENDIAN device on a
big-endian system is a bit weird, because it means the guest
has to byteswap everything. You see that with PCI devices because
the PCI spec mandates LE, but not often elsewhere).

If there's an official-ish spec for how goldfish devices are
supposed to behave (does anybody have a pointer to one?) and it says
"always little-endian" then that would probably suggest that fixing
m68k would be nice if we can.

thanks
-- PMM

Re: [PATCH 16/35] acpi: ipmi: use AcpiDevAmlIf interface to build IPMI device descriptors

[Michael S. Tsirkin]

On Mon, May 16, 2022 at 11:25:51AM -0400, Igor Mammedov wrote:
> convert ad-hoc way we use to generate AML for ISA/SMB IPMI devices
> to a generic approach (i.e. make devices provide its own AML blobs
> like it is done with other ISA devices (ex. KBD))
> 
> Signed-off-by: Igor Mammedov 

could not apply this. rebase and repost?

> ---
>  include/hw/acpi/ipmi.h |  9 ++--
>  hw/acpi/ipmi-stub.c|  2 +-
>  hw/acpi/ipmi.c | 49 +-
>  hw/i386/acpi-build.c   | 17 ++-
>  hw/ipmi/isa_ipmi_bt.c  |  4 
>  hw/ipmi/isa_ipmi_kcs.c |  4 
>  hw/ipmi/smbus_ipmi.c   |  4 
>  7 files changed, 42 insertions(+), 47 deletions(-)
> 
> diff --git a/include/hw/acpi/ipmi.h b/include/hw/acpi/ipmi.h
> index c38483565c..6c8079c97a 100644
> --- a/include/hw/acpi/ipmi.h
> +++ b/include/hw/acpi/ipmi.h
> @@ -9,13 +9,8 @@
>  #ifndef HW_ACPI_IPMI_H
>  #define HW_ACPI_IPMI_H
>  
> -#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/acpi_aml_interface.h"
>  
> -/*
> - * Add ACPI IPMI entries for all registered IPMI devices whose parent
> - * bus matches the given bus.  The resource is the ACPI resource that
> - * contains the IPMI device, this is required for the I2C CRS.
> - */
> -void build_acpi_ipmi_devices(Aml *table, BusState *bus);
> +void build_ipmi_dev_aml(AcpiDevAmlIf *adev, Aml *scope);
>  
>  #endif /* HW_ACPI_IPMI_H */
> diff --git a/hw/acpi/ipmi-stub.c b/hw/acpi/ipmi-stub.c
> index f525f71c2d..befaf0a882 100644
> --- a/hw/acpi/ipmi-stub.c
> +++ b/hw/acpi/ipmi-stub.c
> @@ -10,6 +10,6 @@
>  #include "qemu/osdep.h"
>  #include "hw/acpi/ipmi.h"
>  
> -void build_acpi_ipmi_devices(Aml *table, BusState *bus)
> +void build_ipmi_dev_aml(AcpiDevAmlIf *adev, Aml *scope)
>  {
>  }
> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> index c30b44fcf5..a20e57d465 100644
> --- a/hw/acpi/ipmi.c
> +++ b/hw/acpi/ipmi.c
> @@ -62,46 +62,27 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>  return crs;
>  }
>  
> -static Aml *aml_ipmi_device(IPMIFwInfo *info)
> +void build_ipmi_dev_aml(AcpiDevAmlIf *adev, Aml *scope)
>  {
>  Aml *dev;
> -uint16_t version = ((info->ipmi_spec_major_revision << 8)
> -| (info->ipmi_spec_minor_revision << 4));
> +IPMIFwInfo info = {};
> +IPMIInterface *ii = IPMI_INTERFACE(adev);
> +IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
> +uint16_t version;
>  
> -assert(info->ipmi_spec_minor_revision <= 15);
> +iic->get_fwinfo(ii, &info);
> +assert(info.ipmi_spec_minor_revision <= 15);
> +version = ((info.ipmi_spec_major_revision << 8)
> +  | (info.ipmi_spec_minor_revision << 4));
>  
> -dev = aml_device("MI%d", info->uuid);
> +dev = aml_device("MI%d", info.uuid);
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("IPI0001")));
>  aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s",
> - info->interface_name)));
> -aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid)));
> -aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info)));
> -aml_append(dev, aml_name_decl("_IFT", aml_int(info->interface_type)));
> + info.interface_name)));
> +aml_append(dev, aml_name_decl("_UID", aml_int(info.uuid)));
> +aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(&info)));
> +aml_append(dev, aml_name_decl("_IFT", aml_int(info.interface_type)));
>  aml_append(dev, aml_name_decl("_SRV", aml_int(version)));
>  
> -return dev;
> -}
> -
> -void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
> -{
> -
> -BusChild *kid;
> -
> -QTAILQ_FOREACH(kid, &bus->children,  sibling) {
> -IPMIInterface *ii;
> -IPMIInterfaceClass *iic;
> -IPMIFwInfo info;
> -Object *obj = object_dynamic_cast(OBJECT(kid->child),
> -  TYPE_IPMI_INTERFACE);
> -
> -if (!obj) {
> -continue;
> -}
> -
> -ii = IPMI_INTERFACE(obj);
> -iic = IPMI_INTERFACE_GET_CLASS(obj);
> -memset(&info, 0, sizeof(info));
> -iic->get_fwinfo(ii, &info);
> -aml_append(scope, aml_ipmi_device(&info));
> -}
> +aml_append(scope, dev);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6dce8354cc..ca5cab87ba 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -38,6 +38,7 @@
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/isa/isa.h"
> +#include "hw/acpi/acpi_aml_interface.h"
>  #include "hw/input/i8042.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "sysemu/tpm.h"
> @@ -71,7 +72,6 @@
>  #include "hw/i386/intel_iommu.h"
>  #include "hw/virtio/virtio-iommu.h"
>  
> -#include "hw/acpi/ipmi.h"
>  #include "hw/acpi/hmat.h"
>  #include "hw/acpi/viot.h"
>  
> @@ -870,7 +870,6 @@ static void build_isa_devices_aml(Aml *table)
>  assert(obj && !ambiguous);
>

Re: [PATCH 13/21] aspeed: Add I2C buses to AST1030 model

[Cédric Le Goater]

On 6/7/22 01:18, Joel Stanley wrote:

On Mon, 6 Jun 2022 at 15:09, Cédric Le Goater  wrote:


From: Troy Lee 

Instantiate the I2C buses in AST1030 model and create two slave device
for ast1030-evb.

Signed-off-by: Troy Lee 
Signed-off-by: Jamin Lin 
Signed-off-by: Steven Lee 
[ clg : - adapted to current ast1030 upstream models
 - fixed typo in commit log ]
Message-Id: <20220324100439.478317-3-troy_...@aspeedtech.com>
Signed-off-by: Cédric Le Goater 


Reviewed-by: Joel Stanley 

one question about a comment below.


---
  hw/arm/aspeed.c | 13 +
  hw/arm/aspeed_ast10x0.c | 18 ++
  2 files changed, 31 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 98dc185acd9a..5c3802308e80 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1401,6 +1401,18 @@ static void aspeed_minibmc_machine_init(MachineState 
*machine)
 AST1030_INTERNAL_FLASH_SIZE);
  }

+static void ast1030_evb_i2c_init(AspeedMachineState *bmc)
+{
+AspeedSoCState *soc = &bmc->soc;
+
+/* U10 24C08 connects to SDA/SCL Groupt 1 by default */
+uint8_t *eeprom_buf = g_malloc0(32 * 1024);
+smbus_eeprom_init_one(aspeed_i2c_get_bus(&soc->i2c, 0), 0x50, eeprom_buf);
+
+/* U11 LM75 connects to SDA/SCL Group 2 by default */
+i2c_slave_create_simple(aspeed_i2c_get_bus(&soc->i2c, 1), "tmp105", 0x4d);
+}
+
  static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
void *data)
  {
@@ -1412,6 +1424,7 @@ static void 
aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
  amc->hw_strap1 = 0;
  amc->hw_strap2 = 0;
  mc->init = aspeed_minibmc_machine_init;
+amc->i2c_init = ast1030_evb_i2c_init;
  mc->default_ram_size = 0;
  mc->default_cpus = mc->min_cpus = mc->max_cpus = 1;
  amc->fmc_model = "sst25vf032b";
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index d53454168403..a2ed275712fb 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -114,6 +114,9 @@ static void aspeed_soc_ast1030_init(Object *obj)
  object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu), "hw-strap1");
  object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu), "hw-strap2");

+snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
+object_initialize_child(obj, "i2c", &s->i2c, typename);
+
  snprintf(typename, sizeof(typename), "aspeed.timer-%s", socname);
  object_initialize_child(obj, "timerctrl", &s->timerctrl, typename);

@@ -188,6 +191,21 @@ static void aspeed_soc_ast1030_realize(DeviceState 
*dev_soc, Error **errp)
  }
  sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, sc->memmap[ASPEED_DEV_SCU]);

+/* I2C */
+
+object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(&s->sram),
+ &error_abort);
+if (!sysbus_realize(SYS_BUS_DEVICE(&s->i2c), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c), 0, sc->memmap[ASPEED_DEV_I2C]);
+for (i = 0; i < ASPEED_I2C_GET_CLASS(&s->i2c)->num_busses; i++) {
+qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->armv7m),
+sc->irqmap[ASPEED_DEV_I2C] + i);
+/* The AST2600 I2C controller has one IRQ per bus. */


I know it's the same hardware, but is the "AST2600" part of the comment correct?


I will take care of it.

Thanks,

C.





+sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
+}
+
  /* LPC */
  if (!sysbus_realize(SYS_BUS_DEVICE(&s->lpc), errp)) {
  return;
--
2.35.3

Re: [PATCH v2] hw/nvme: allow to pass a memory backend object for the CMB

[Wertenbroek Rick]

Hello,
Sorry about that, I had the same issue with the patch for some reason, I think 
it is the git send email that
messed up the patch (couldn’t directly send via smtp so I created a draft via 
IMAP before sending).
Anyway, below is the patch, it was created on the master branch (commit 
81c7ed41a1b33031f3e4fe24191a998a492044b8).
I copy pasted the contents (plain text) of the .patch and could apply it.
Let me know if this doesn’t work and I’ll fork the QEMU git and make the patch 
available there.

From 79b622edbf3032751250822007ef11b2d7de31e2 Mon Sep 17 00:00:00 2001
Message-Id: 
<79b622edbf3032751250822007ef11b2d7de31e2.1648820166.git.rick.wertenbr...@heig-vd.ch>
From: Rick Wertenbroek 
Date: Fri, 1 Apr 2022 15:05:07 +0200
Subject: [PATCH] hw/nvme: allow to pass a memory backend object for the CMB

Adds the optional -cmbdev= option that takes a QEMU memory backend
-object to be used to for the CMB (Controller Memory Buffer).
This option takes precedence over cmb_size_mb= if both used.
(The size will be deduced from the memory backend option).

Signed-off-by: Rick Wertenbroek 
---
 hw/nvme/ctrl.c | 65 ++
 hw/nvme/nvme.h |  9 +++
 2 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 03760ddeae..9bcc7d6db0 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -29,6 +29,7 @@
  *  -device nvme-subsys,id=,nqn=
  *  -device nvme,serial=,id=, \
  *  cmb_size_mb=, \
+ *  [cmbdev=,] \
  *  [pmrdev=,] \
  *  max_ioqpairs=, \
  *  aerl=,aer_max_queued=, \
@@ -44,6 +45,11 @@
  * offset 0 in BAR2 and supports only WDS, RDS and SQS for now. By default, the
  * device will use the "v1.4 CMB scheme" - use the `legacy-cmb` parameter to
  * always enable the CMBLOC and CMBSZ registers (v1.3 behavior).
+ * Enabling cmb emulation can also be achieved by pointing to a memory-backend
+ * For example:
+ * -object memory-backend-ram,id=,size= \
+ * -device nvme,...,cmbdev=
+ * cmbdev= will take precedence over cmb_size_mb= when both provided.
  *
  * Enabling pmr emulation can be achieved by pointing to memory-backend-file.
  * For example:
@@ -341,16 +347,26 @@ static bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
 return false;
 }
 
-lo = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
-hi = lo + int128_get64(n->cmb.mem.size);
+if (n->cmb.dev) {
+lo = n->params.legacy_cmb ? 
host_memory_backend_get_memory(n->cmb.dev)->addr : n->cmb.cba;
+hi = lo + 
int128_get64(host_memory_backend_get_memory(n->cmb.dev)->size);
+} else {
+lo = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
+hi = lo + int128_get64(n->cmb.mem.size);
+}
 
 return addr >= lo && addr < hi;
 }
 
 static inline void *nvme_addr_to_cmb(NvmeCtrl *n, hwaddr addr)
 {
-hwaddr base = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
-return &n->cmb.buf[addr - base];
+if (n->cmb.dev) {
+hwaddr base = n->params.legacy_cmb ? 
host_memory_backend_get_memory(n->cmb.dev)->addr : n->cmb.cba;
+return memory_region_get_ram_ptr(&n->cmb.dev->mr) + (addr - base);
+} else {
+hwaddr base = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
+return &n->cmb.buf[addr - base];
+}
 }
 
 static bool nvme_addr_is_pmr(NvmeCtrl *n, hwaddr addr)
@@ -6584,16 +6600,33 @@ static void nvme_init_state(NvmeCtrl *n)
 
 static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
 {
-uint64_t cmb_size = n->params.cmb_size_mb * MiB;
+uint64_t cmb_size;
 uint64_t cap = ldq_le_p(&n->bar.cap);
 
-n->cmb.buf = g_malloc0(cmb_size);
-memory_region_init_io(&n->cmb.mem, OBJECT(n), &nvme_cmb_ops, n,
-  "nvme-cmb", cmb_size);
-pci_register_bar(pci_dev, NVME_CMB_BIR,
- PCI_BASE_ADDRESS_SPACE_MEMORY |
- PCI_BASE_ADDRESS_MEM_TYPE_64 |
- PCI_BASE_ADDRESS_MEM_PREFETCH, &n->cmb.mem);
+if (n->cmb.dev) {
+if (n->params.cmb_size_mb) {
+warn_report("Option cmb_size_mb is ignored when a cmbdev is 
provided");
+}
+n->params.cmb_size_mb = n->cmb.dev->size / MiB;
+cmb_size = n->cmb.dev->size;
+
+MemoryRegion *mr = host_memory_backend_get_memory(n->cmb.dev);
+assert(mr);
+
+pci_register_bar(pci_dev, NVME_CMB_BIR,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_TYPE_64 |
+ PCI_BASE_ADDRESS_MEM_PREFETCH, mr);
+} else {
+cmb_size = n->params.cmb_size_mb * MiB;
+n->cmb.buf = g_malloc0(cmb_size);
+memory_region_init_io(&n->cmb.mem, OBJECT(n), &nvme_cmb_ops, n,
+  "nvme-cmb", cmb_size);
+pci_register_bar(pci_dev, NVME_CMB_BIR,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_

Re: [PATCH v5 05/45] tests-bdrv-drain: bdrv_replace_test driver: declare supports_backing

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

We do add COW child to the node.  In future we are going to forbid
adding COW child to the node that doesn't support backing. So, fix it
here now.

Don't worry about setting bs->backing itself: it further commit we'll


s/it/in/


update the block-layer to automatically set/unset this field in generic
code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/unit/test-bdrv-drain.c | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Hanna Reitz 

Re: [PATCH v5 04/45] test-bdrv-graph-mod: update test_parallel_perm_update test case

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

test_parallel_perm_update() does two things that we are going to
restrict in the near future:

1. It updates bs->file field by hand. bs->file will be managed
automatically by generic code (together with bs->children list).

Let's better refactor our "tricky" bds to have own state where one
of children is linked as "selected".
This also looks less "tricky", so avoid using this word.

2. It create FILTERED children that are not PRIMARY. Except for tests
all FILTERED children in the Qemu block layer are always PRIMARY as
well.  We are going to formalize this rule, so let's better use DATA
children here.


Another thing is that any node may have at most one FILTERED child at a 
time, which was already formalized in BDRV_CHILD_FILTERED’s description.



While being here, update the picture to better correspond to the test
code.

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


The change looks good, I’m just a bit confused when it comes to the 
comment describing what’s going on.



  tests/unit/test-bdrv-graph-mod.c | 70 
  1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
index a6e3bb79be..40795d3c04 100644
--- a/tests/unit/test-bdrv-graph-mod.c
+++ b/tests/unit/test-bdrv-graph-mod.c


[...]


@@ -266,15 +280,18 @@ static BlockDriver bdrv_write_to_file = {
   * The following test shows that topological-sort order is required for
   * permission update, simple DFS is not enough.
   *
- * Consider the block driver which has two filter children: one active
- * with exclusive write access and one inactive with no specific
- * permissions.
+ * Consider the block driver (write-to-selected) which has two children: one is
+ * selected so we have exclusive write access to it and for the other one we
+ * don't need any specific permissions.
   *
   * And, these two children has a common base child, like this:
+ *   (additional "top" on top is used in test just because the only public
+ *function to update permission should get a specific child to update.
+ *Making bdrv_refresh_perms() public just for this test doesn't worth it)


s/doesn't/isn't/


   *
- * ┌─┐ ┌──┐
- * │ fl2 │ ◀── │ top  │
- * └─┘ └──┘
+ * ┌─┐ ┌───┐ ┌─┐
+ * │ fl2 │ ◀── │ write-to-selected │ ◀── │ top │
+ * └─┘ └───┘ └─┘
   *   │   │
   *   │   │ w
   *   │   ▼
@@ -290,7 +307,7 @@ static BlockDriver bdrv_write_to_file = {
   *
   * So, exclusive write is propagated.
   *
- * Assume, we want to make fl2 active instead of fl1.
+ * Assume, we want to select fl2  instead of fl1.


There’s a double space after “fl2”.


   * So, we set some option for top driver and do permission update.


Here and in the rest of the comment, it’s now unclear what node “top” 
refers to.  I think it’s still the now-renamed “write-to-selected” node, 
right?  But “top” is now a different node, so I’m not 100% sure.


(On the other hand, even before this patch, there was a “top” node that 
was distinct from the former “tricky” node...  So it seems like this 
comment was already not quite right before?)



   *
   * With simple DFS, if permission update goes first through

Re: [PATCH v2] hw/nvme: clean up CC register write logic

[Klaus Jensen]

On Jun  7 13:06, Łukasz Gieryk wrote:
> On Fri, Jun 03, 2022 at 10:24:51PM +0200, Klaus Jensen wrote:
> > On Jun  1 15:28, Lukasz Maniak wrote:
> > > On Wed, May 25, 2022 at 09:35:24AM +0200, Klaus Jensen wrote:
> > > > 
> > > > +stl_le_p(&n->bar.intms, 0);
> > > > +stl_le_p(&n->bar.intmc, 0);
> > > > +stl_le_p(&n->bar.cc, 0);
> > > 
> > > Looks fine, though it seems the NVMe spec says the above registers
> > > should be cleared during each reset for VF as well.
> > > 
> > 
> > Aren't the values of all other registers than CSTS just undefined? (NVMe
> > v2.0b, Section 8.26.3)
> 
> My 2 cents –
> 
> When VF is online:
> - Both Controller Reset (CR) and PCIe Function Level Reset (FLR) can be
>   issued to given VF
> - Both resets shall return all (except specific) Nvme registers of given
>   VF to their reset values (3.7.2)
> 
> When VF is offline:
> - CR cannot be issued (only CSTS is defined, writes to CC are dropped),
>   so doesn’t need an explicit IF
> - FLR is allowed as it’s a part of the procedure to bring VF online
>   (mentioned in 8.26.3)
> - At least FLR shall reset the registers for VF
> 
> So I agree with the other Lukasz's suggestion. I would also clear
> intms/intmc/cc for both: VF and PF reset paths, regardless of the actual
> reset type.
> 

Alright thanks - sounds good.

See v3.


signature.asc
Description: PGP signature

Re: [PATCH v5 06/45] test-bdrv-graph-mod: fix filters to be filters

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

bdrv_pass_through is used as filter, even all node variables has
corresponding names. We want to append it, so it should be
backing-child-based filter like mirror_top.
So, in test_update_perm_tree, first child should be DATA, as we don't
want filters with two filtered children.

bdrv_exclusive_writer is used as a filter once. So it should be filter
anyway. We want to append it, so it should be backing-child-based
fitler too.

Make all FILTERED children to be PRIMARY as well. We are going to force
this rule by assertion soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block_int-common.h |  5 +++--
  tests/unit/test-bdrv-graph-mod.c | 24 +---
  2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 9d91ccbcbf..d68adc6ff3 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -122,8 +122,9 @@ struct BlockDriver {
  /*
   * Only make sense for filter drivers, for others must be false.
   * If true, filtered child is bs->backing. Otherwise it's bs->file.
- * Only two internal filters use bs->backing as filtered child and has this
- * field set to true: mirror_top and commit_top.
+ * Two internal filters use bs->backing as filtered child and has this
+ * field set to true: mirror_top and commit_top. There also two such test
+ * filters in tests/unit/test-bdrv-graph-mod.c.
   *
   * Never create any more such filters!


I mean, it’s just a test, of course, but it is kind of strange that you 
put this very strong imperative here just a couple of patches ago and 
now you disobey it. O:)


Makes sense, though.

Reviewed-by: Hanna Reitz 

Re: [PATCH v2] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Jonathan Cameron via]

On Tue, 7 Jun 2022 11:56:17 +0100
Jonathan Cameron  wrote:

> Without being able to write these registers, no interleaving is possible.
> More refined checks of HDM register state on commit to follow.
> 
> Signed-off-by: Jonathan Cameron 

+Cc Ben on current address.

Which reminds me - Ben, when you have time please send an update
to MAINTAINERS to switch to your preferred email address going forwards.

Thanks,

Jonathan

> ---
> v2: (Ben Widawsky)
> - Correctly set a tighter write mask for the endpoint devices where this
>   register has a different use.
> 
>  hw/cxl/cxl-component-utils.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 7985c9bfca..40a0f752f2 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, uint32_t 
> *write_msk)
>  reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
>  }
>  
> -static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
> +static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> +enum reg_type type)
>  {
>  int decoder_count = 1;
>  int i;
> @@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, 
> uint32_t *write_msk)
>  write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
>  write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
>  write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> +if (type == CXL2_DEVICE) {
> +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> 0xf000;
> +} else {
> +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> 0x;
> +}
> +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 0x;
>  }
>  }
>  

Re: [PATCH v5 07/45] block: document connection between child roles and bs->backing/bs->file

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

Make the informal rules formal. In further commit we'll add
corresponding assertions.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block-common.h | 42 
  1 file changed, 42 insertions(+)

diff --git a/include/block/block-common.h b/include/block/block-common.h
index fdb7306e78..2687a2519c 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -313,6 +313,48 @@ enum {
   *
   * At least one of DATA, METADATA, FILTERED, or COW must be set for
   * every child.
+ *
+ *
+ * = Connection with bs->children, bs->file and bs->backing fields =
+ *
+ * 1. Filters
+ *
+ * Filter drivers has drv->is_filter = true.


s/has/have/


+ *
+ * Filter driver has exactly one FILTERED|PRIMARY child, any may have other


s/Filter driver/A filter node/?

And s/any/and/, I think.


+ * children which must not have these bits (the example is copy-before-write
+ * filter that also has target DATA child).


Mild style suggestion: “one example is the copy-before write filter, 
which also has its target DATA child”



+ *
+ * Filter driver never has COW children.


Maybe “Filter nodes never have COW children.”?


+ *
+ * For all filters except for mirror_top and commit_top, the filtered child is
+ * linked in bs->file, bs->backing is NULL.
+ *
+ * For mirror_top and commit_top filtered child is linked in bs->backing and


s/commit_top filtered/commit_top, the filtered/ (like in the paragraph 
above)



+ * their bs->file is NULL. These two filters has drv->filtered_child_is_backing


s/has/have/


+ * = true.


This also applies to the two test drivers in test-bdrv-graph-mod; should 
that be mentioned, too?


Or should we just link to filtered_child_is_backing when it comes to the 
list of drivers for which this applies, e.g. by rephrasing the two 
paragraphs as follows:


For most filters, the filtered child is linked in bs->file, bs->backing 
is NULL.  For some filters (as an exception), it is the other way 
around; those drivers will have drv->filtered_child_is_backing set to 
true (see that field’s documentation for what drivers this concerns).


(Just so we don’t duplicate the list of drivers.)


+ *
+ * 2. "raw" driver (block/raw-format.c)
+ *
+ * Formally it's not a filter (drv->is_filter = false)
+ *
+ * bs->backing is always NULL
+ *
+ * Only has one child, linked in bs->file. It's role is either FILTERED|PRIMARY


s/it's/its/


+ * (like filter) either DATA|PRIMARY depending on options.


s/either/or/


+ *
+ * 3. Other drivers
+ *
+ * Doesn't have any FILTERED children.


s/Doesn't/Don't/ (because “drivers” was in plural)


+ *
+ * May have at most one COW child. In this case it's linked in bs->backing.
+ * Otherwise bs->backing is NULL. COW child is never PRIMARY.
+ *
+ * May have at most one PRIMARY child. In this case it's linked in bs->file.
+ * Otherwise bs->file is NULL.
+ *
+ * May also have some other children that don't have neither PRIMARY nor COW
+ * bits set.


I think either “that don't have the PRIMARY or COW bit set" or "that 
have neither the PRIMARY nor the COW bit set".



   */
  enum BdrvChildRoleBits {
  /*


Aside from typo/style nit picks, sounds good!

Re: [PATCH v2] hw/nvme: clean up CC register write logic

[Łukasz Gieryk]

On Fri, Jun 03, 2022 at 10:24:51PM +0200, Klaus Jensen wrote:
> On Jun  1 15:28, Lukasz Maniak wrote:
> > On Wed, May 25, 2022 at 09:35:24AM +0200, Klaus Jensen wrote:
> > > 
> > > +stl_le_p(&n->bar.intms, 0);
> > > +stl_le_p(&n->bar.intmc, 0);
> > > +stl_le_p(&n->bar.cc, 0);
> > 
> > Looks fine, though it seems the NVMe spec says the above registers
> > should be cleared during each reset for VF as well.
> > 
> 
> Aren't the values of all other registers than CSTS just undefined? (NVMe
> v2.0b, Section 8.26.3)

My 2 cents –

When VF is online:
- Both Controller Reset (CR) and PCIe Function Level Reset (FLR) can be
  issued to given VF
- Both resets shall return all (except specific) Nvme registers of given
  VF to their reset values (3.7.2)

When VF is offline:
- CR cannot be issued (only CSTS is defined, writes to CC are dropped),
  so doesn’t need an explicit IF
- FLR is allowed as it’s a part of the procedure to bring VF online
  (mentioned in 8.26.3)
- At least FLR shall reset the registers for VF

So I agree with the other Lukasz's suggestion. I would also clear
intms/intmc/cc for both: VF and PF reset paths, regardless of the actual
reset type.

Re: [RFC PATCH v4 11/36] i386/tdx: Initialize TDX before creating TD vcpus

[Gerd Hoffmann]

  Hi,

> > I guess it could be helpful for the discussion when you can outine the
> > 'big picture' for tdx initialization.  How does kvm accel setup look
> > like without TDX, and what additional actions are needed for TDX?  What
> > ordering requirements and other constrains exist?
> 
> To boot a TDX VM, it requires several changes/additional steps in the flow:
> 
>  1. specify the vm type KVM_X86_TDX_VM when creating VM with
> IOCTL(KVM_CREATE_VM);
>   - When initializing KVM accel
> 
>  2. initialize VM scope configuration before creating any VCPU;
> 
>  3. initialize VCPU scope configuration;
>   - done inside machine_init_done_notifier;
> 
>  4. initialize virtual firmware in guest private memory before vcpu running;
>   - done inside machine_init_done_notifier;
> 
>  5. finalize the TD's measurement;
>   - done inside machine init_done_notifier;
> 
> 
> And we are discussing where to do step 2).
> 
> We can find from the code of tdx_pre_create_vcpu(), that it needs
> cpuid entries[] and attributes as input to KVM.
> 
>   cpuid entries[] is set up by kvm_x86_arch_cpuid() mainly based on
>   'CPUX86State *env'
> 
>   attributes.pks is retrieved from env->features[]
>   and attributes.pmu is retrieved from x86cpu->enable_pmu
> 
> to make VM-socpe data is consistent with VCPU data, we do choose the point
> late enough to ensure all the info/configurations from VCPU are settle down,
> that just before calling KVM API to do VCPU-scope configuration.

So essentially tdx defines (some) vcpu properties at vm scope?  Given
that all vcpus typically identical (and maybe tdx even enforces this)
this makes sense.

A comment in the source code explaining this would be good.

thanks,
  Gerd

Re: [PATCH v2] hw/nvme: allow to pass a memory backend object for the CMB

[Wertenbroek Rick]

Hi,
So it seems the patch is messed up, I forked QEMU applied the patch to the 
current master.
Here it is : https://gitlab.com/rwe-reds/qemu/-/tree/master

I rebuilt QEMU to check that everything builds and runs.
Sorry for the inconvenience, I don’t know why the patch gets corrupted…

Best regards.
Rick

On 7 Jun 2022, at 13:40, Rick Wertenbroek 
mailto:rick.wertenbr...@heig-vd.ch>> wrote:

Hello,
Sorry about that, I had the same issue with the patch for some reason, I think 
it is the git send email that
messed up the patch (couldn’t directly send via smtp so I created a draft via 
IMAP before sending).
Anyway, below is the patch, it was created on the master branch (commit 
81c7ed41a1b33031f3e4fe24191a998a492044b8).
I copy pasted the contents (plain text) of the .patch and could apply it.
Let me know if this doesn’t work and I’ll fork the QEMU git and make the patch 
available there.

From 79b622edbf3032751250822007ef11b2d7de31e2 Mon Sep 17 00:00:00 2001
Message-Id: 
<79b622edbf3032751250822007ef11b2d7de31e2.1648820166.git.rick.wertenbr...@heig-vd.ch>
From: Rick Wertenbroek 
mailto:rick.wertenbr...@heig-vd.ch>>
Date: Fri, 1 Apr 2022 15:05:07 +0200
Subject: [PATCH] hw/nvme: allow to pass a memory backend object for the CMB

Adds the optional -cmbdev= option that takes a QEMU memory backend
-object to be used to for the CMB (Controller Memory Buffer).
This option takes precedence over cmb_size_mb= if both used.
(The size will be deduced from the memory backend option).

Signed-off-by: Rick Wertenbroek 
mailto:rick.wertenbr...@heig-vd.ch>>
---
hw/nvme/ctrl.c | 65 ++
hw/nvme/nvme.h |  9 +++
2 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 03760ddeae..9bcc7d6db0 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -29,6 +29,7 @@
 *  -device nvme-subsys,id=,nqn=
 *  -device nvme,serial=,id=, \
 *  cmb_size_mb=, \
+ *  [cmbdev=,] \
 *  [pmrdev=,] \
 *  max_ioqpairs=, \
 *  aerl=,aer_max_queued=, \
@@ -44,6 +45,11 @@
 * offset 0 in BAR2 and supports only WDS, RDS and SQS for now. By default, the
 * device will use the "v1.4 CMB scheme" - use the `legacy-cmb` parameter to
 * always enable the CMBLOC and CMBSZ registers (v1.3 behavior).
+ * Enabling cmb emulation can also be achieved by pointing to a memory-backend
+ * For example:
+ * -object memory-backend-ram,id=,size= \
+ * -device nvme,...,cmbdev=
+ * cmbdev= will take precedence over cmb_size_mb= when both provided.
 *
 * Enabling pmr emulation can be achieved by pointing to memory-backend-file.
 * For example:
@@ -341,16 +347,26 @@ static bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
return false;
}

-lo = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
-hi = lo + int128_get64(n->cmb.mem.size);
+if (n->cmb.dev) {
+lo = n->params.legacy_cmb ? 
host_memory_backend_get_memory(n->cmb.dev)->addr : n->cmb.cba;
+hi = lo + 
int128_get64(host_memory_backend_get_memory(n->cmb.dev)->size);
+} else {
+lo = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
+hi = lo + int128_get64(n->cmb.mem.size);
+}

return addr >= lo && addr < hi;
}

static inline void *nvme_addr_to_cmb(NvmeCtrl *n, hwaddr addr)
{
-hwaddr base = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
-return &n->cmb.buf[addr - base];
+if (n->cmb.dev) {
+hwaddr base = n->params.legacy_cmb ? 
host_memory_backend_get_memory(n->cmb.dev)->addr : n->cmb.cba;
+return memory_region_get_ram_ptr(&n->cmb.dev->mr) + (addr - base);
+} else {
+hwaddr base = n->params.legacy_cmb ? n->cmb.mem.addr : n->cmb.cba;
+return &n->cmb.buf[addr - base];
+}
}

static bool nvme_addr_is_pmr(NvmeCtrl *n, hwaddr addr)
@@ -6584,16 +6600,33 @@ static void nvme_init_state(NvmeCtrl *n)

static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
{
-uint64_t cmb_size = n->params.cmb_size_mb * MiB;
+uint64_t cmb_size;
uint64_t cap = ldq_le_p(&n->bar.cap);

-n->cmb.buf = g_malloc0(cmb_size);
-memory_region_init_io(&n->cmb.mem, OBJECT(n), &nvme_cmb_ops, n,
-  "nvme-cmb", cmb_size);
-pci_register_bar(pci_dev, NVME_CMB_BIR,
- PCI_BASE_ADDRESS_SPACE_MEMORY |
- PCI_BASE_ADDRESS_MEM_TYPE_64 |
- PCI_BASE_ADDRESS_MEM_PREFETCH, &n->cmb.mem);
+if (n->cmb.dev) {
+if (n->params.cmb_size_mb) {
+warn_report("Option cmb_size_mb is ignored when a cmbdev is 
provided");
+}
+n->params.cmb_size_mb = n->cmb.dev->size / MiB;
+cmb_size = n->cmb.dev->size;
+
+MemoryRegion *mr = host_memory_backend_get_memory(n->cmb.dev);
+assert(mr);
+
+pci_register_bar(pci_dev, NVME_CMB_BIR,
+  

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Stafford Horne]

On Tue, Jun 07, 2022 at 11:43:08AM +0100, Peter Maydell wrote:
> So I don't have a strong view on whether these devices should
> be DEVICE_NATIVE_ENDIAN or DEVICE_LITTLE_ENDIAN (except that
> my impression is that a DEVICE_LITTLE_ENDIAN device on a
> big-endian system is a bit weird, because it means the guest
> has to byteswap everything. You see that with PCI devices because
> the PCI spec mandates LE, but not often elsewhere).
> 
> If there's an official-ish spec for how goldfish devices are
> supposed to behave (does anybody have a pointer to one?) and it says
> "always little-endian" then that would probably suggest that fixing
> m68k would be nice if we can.

I think there are some conflicting thoughts on this.

In Geert's he mentioned:

  Using Goldfish devices as little-endian devices should be fine.

In Arnd's mail he mentions:

  
https://lore.kernel.org/lkml/CAK8P3a1oN8NrUjkh2X8jHQbyz42Xo6GSa=5n0gd6vqcxrjm...@mail.gmail.com/#t

  ... the device was clearly defined as having little-endian
  registers,

Based on that I was thinking that switching to DEVICE_LITTLE_ENDIAN would make
sense.

However, in a followup mail from Laurent we see:

  https://lore.kernel.org/lkml/cb884368-0226-e913-80d2-62d2b7b2e...@vivier.eu/

  The reference document[1] doesn't define the endianness of goldfish.

  [1] 
https://android.googlesource.com/platform/external/qemu/+/master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT


The documentation does not clearly specify it.  So maybe maybe or1k should just
be updated on the linux side and add gf_ioread32/gf_iowrite32 big-endian
accessors.

-Stafford

[PATCH v3] hw/nvme: clean up CC register write logic

[Klaus Jensen]

From: Klaus Jensen 

The SRIOV series exposed an issued with how CC register writes are
handled and how CSTS is set in response to that. Specifically, after
applying the SRIOV series, the controller could end up in a state with
CC.EN set to '1' but with CSTS.RDY cleared to '0', causing drivers to
expect CSTS.RDY to transition to '1' but timing out.

Clean this up.

Signed-off-by: Klaus Jensen 
---
v3:
  * clear intms/intmc/cc regardless of reset type

 hw/nvme/ctrl.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 658584d417fe..a558f5cb29c1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6190,10 +6190,15 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType 
rst)
 
 if (pci_is_vf(pci_dev)) {
 sctrl = nvme_sctrl(n);
+
 stl_le_p(&n->bar.csts, sctrl->scs ? 0 : NVME_CSTS_FAILED);
 } else {
 stl_le_p(&n->bar.csts, 0);
 }
+
+stl_le_p(&n->bar.intms, 0);
+stl_le_p(&n->bar.intmc, 0);
+stl_le_p(&n->bar.cc, 0);
 }
 
 static void nvme_ctrl_shutdown(NvmeCtrl *n)
@@ -6405,20 +6410,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
 nvme_irq_check(n);
 break;
 case NVME_REG_CC:
+stl_le_p(&n->bar.cc, data);
+
 trace_pci_nvme_mmio_cfg(data & 0x);
 
-/* Windows first sends data, then sends enable bit */
-if (!NVME_CC_EN(data) && !NVME_CC_EN(cc) &&
-!NVME_CC_SHN(data) && !NVME_CC_SHN(cc))
-{
-cc = data;
+if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) {
+trace_pci_nvme_mmio_shutdown_set();
+nvme_ctrl_shutdown(n);
+csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT);
+csts |= NVME_CSTS_SHST_COMPLETE;
+} else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) {
+trace_pci_nvme_mmio_shutdown_cleared();
+csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT);
 }
 
 if (NVME_CC_EN(data) && !NVME_CC_EN(cc)) {
-cc = data;
-
-/* flush CC since nvme_start_ctrl() needs the value */
-stl_le_p(&n->bar.cc, cc);
 if (unlikely(nvme_start_ctrl(n))) {
 trace_pci_nvme_err_startfail();
 csts = NVME_CSTS_FAILED;
@@ -6429,22 +6435,10 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
 } else if (!NVME_CC_EN(data) && NVME_CC_EN(cc)) {
 trace_pci_nvme_mmio_stopped();
 nvme_ctrl_reset(n, NVME_RESET_CONTROLLER);
-cc = 0;
-csts &= ~NVME_CSTS_READY;
-}
 
-if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) {
-trace_pci_nvme_mmio_shutdown_set();
-nvme_ctrl_shutdown(n);
-cc = data;
-csts |= NVME_CSTS_SHST_COMPLETE;
-} else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) {
-trace_pci_nvme_mmio_shutdown_cleared();
-csts &= ~NVME_CSTS_SHST_COMPLETE;
-cc = data;
+break;
 }
 
-stl_le_p(&n->bar.cc, cc);
 stl_le_p(&n->bar.csts, csts);
 
 break;
-- 
2.36.1

Re: [PATCH v3 11/49] semihosting: Clean up common_semi_open_cb

[Alex Bennée]

Richard Henderson  writes:

> Use common_semi_cb to return results instead of calling
> set_swi_errno and common_semi_set_ret directly.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v3] hw/nvme: clean up CC register write logic

[Lukasz Maniak]

On Tue, Jun 07, 2022 at 01:23:20PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> The SRIOV series exposed an issued with how CC register writes are
> handled and how CSTS is set in response to that. Specifically, after
> applying the SRIOV series, the controller could end up in a state with
> CC.EN set to '1' but with CSTS.RDY cleared to '0', causing drivers to
> expect CSTS.RDY to transition to '1' but timing out.
> 
> Clean this up.
> 
> Signed-off-by: Klaus Jensen 
> ---
> v3:
>   * clear intms/intmc/cc regardless of reset type
> 
>  hw/nvme/ctrl.c | 38 --
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 658584d417fe..a558f5cb29c1 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6190,10 +6190,15 @@ static void nvme_ctrl_reset(NvmeCtrl *n, 
> NvmeResetType rst)
>  
>  if (pci_is_vf(pci_dev)) {
>  sctrl = nvme_sctrl(n);
> +
>  stl_le_p(&n->bar.csts, sctrl->scs ? 0 : NVME_CSTS_FAILED);
>  } else {
>  stl_le_p(&n->bar.csts, 0);
>  }
> +
> +stl_le_p(&n->bar.intms, 0);
> +stl_le_p(&n->bar.intmc, 0);
> +stl_le_p(&n->bar.cc, 0);
>  }
>  
>  static void nvme_ctrl_shutdown(NvmeCtrl *n)
> @@ -6405,20 +6410,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr 
> offset, uint64_t data,
>  nvme_irq_check(n);
>  break;
>  case NVME_REG_CC:
> +stl_le_p(&n->bar.cc, data);
> +
>  trace_pci_nvme_mmio_cfg(data & 0x);
>  
> -/* Windows first sends data, then sends enable bit */
> -if (!NVME_CC_EN(data) && !NVME_CC_EN(cc) &&
> -!NVME_CC_SHN(data) && !NVME_CC_SHN(cc))
> -{
> -cc = data;
> +if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) {
> +trace_pci_nvme_mmio_shutdown_set();
> +nvme_ctrl_shutdown(n);
> +csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT);
> +csts |= NVME_CSTS_SHST_COMPLETE;
> +} else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) {
> +trace_pci_nvme_mmio_shutdown_cleared();
> +csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT);
>  }
>  
>  if (NVME_CC_EN(data) && !NVME_CC_EN(cc)) {
> -cc = data;
> -
> -/* flush CC since nvme_start_ctrl() needs the value */
> -stl_le_p(&n->bar.cc, cc);
>  if (unlikely(nvme_start_ctrl(n))) {
>  trace_pci_nvme_err_startfail();
>  csts = NVME_CSTS_FAILED;
> @@ -6429,22 +6435,10 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr 
> offset, uint64_t data,
>  } else if (!NVME_CC_EN(data) && NVME_CC_EN(cc)) {
>  trace_pci_nvme_mmio_stopped();
>  nvme_ctrl_reset(n, NVME_RESET_CONTROLLER);
> -cc = 0;
> -csts &= ~NVME_CSTS_READY;
> -}
>  
> -if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) {
> -trace_pci_nvme_mmio_shutdown_set();
> -nvme_ctrl_shutdown(n);
> -cc = data;
> -csts |= NVME_CSTS_SHST_COMPLETE;
> -} else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) {
> -trace_pci_nvme_mmio_shutdown_cleared();
> -csts &= ~NVME_CSTS_SHST_COMPLETE;
> -cc = data;
> +break;
>  }
>  
> -stl_le_p(&n->bar.cc, cc);
>  stl_le_p(&n->bar.csts, csts);
>  
>  break;
> -- 
> 2.36.1
> 

Reviewed-by: Lukasz Maniak 

Re: [PATCH v6 06/18] jobs: protect jobs with job_lock/unlock

[Emanuele Giuseppe Esposito]

Am 03/06/2022 um 18:40 schrieb Kevin Wolf:
> Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
>> Introduce the job locking mechanism through the whole job API,
>> following the comments  in job.h and requirements of job-monitor
>> (like the functions in job-qmp.c, assume lock is held) and
>> job-driver (like in mirror.c and all other JobDriver, lock is not held).
>>
>> Use the _locked helpers introduced before to differentiate
>> between functions called with and without job_mutex.
>> This only applies to function that are called under both
>> cases, all the others will be renamed later.
>>
>> job_{lock/unlock} is independent from real_job_{lock/unlock}.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito 
>> ---
>>  block.c |  18 ---
>>  block/replication.c |   8 ++-
>>  blockdev.c  |  17 --
>>  blockjob.c  |  56 +---
>>  job-qmp.c   |   2 +
>>  job.c   | 125 +++-
>>  monitor/qmp-cmds.c  |   6 ++-
>>  qemu-img.c  |  41 +--
>>  8 files changed, 187 insertions(+), 86 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 718e4cae8b..5dc46fde11 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4978,7 +4978,9 @@ static void bdrv_close(BlockDriverState *bs)
>>  
>>  void bdrv_close_all(void)
>>  {
>> -assert(job_next(NULL) == NULL);
>> +WITH_JOB_LOCK_GUARD() {
>> +assert(job_next(NULL) == NULL);
>> +}
>>  GLOBAL_STATE_CODE();
> 
> This series seems really hard to review patch by patch, in this case
> because I would have to know whether you intended job_next() to be
> called with the lock held or not. Nothing in job.h indicates either way
> at this point in the series.

Well if it's under lock it means all its calls will be under lock. If
some cases will be under lock and some other not, I use the _locked
version, as described in the commit description.

> 
> Patch 11 answers this by actually renaming it job_next_locked(), but
> always having to refer to the final state after the whole series is
> applied is really not how things should work. We're splitting the work
> into individual patches so that the state after each single patch makes
> sense on its own. Otherwise the whole series could as well be a single
> patch. :-(

The various function and ordering has changed pretty much in each of the
6 version I sent, because it is very difficult to understand what comes
first and what can go afterwards.

Anyways, I see what you mean but I would not move patch 11 before this
one, because otherwise we would have _locked functions used without
having even a fake lock around, and the next reviewer would complain. In
fact, I think I put it afterwards because someone initially suggested so.

Ideally we want both patches together, but then it will be a total mess
to read, so I would leave it as it is.

In addition, I don't think it would hurt to have "normal" (ie without
_locked) functions wrapped by a nop macro.

Emanuele

> 
> So I'd argue that patch 11 should probably come before this one.
> 
> Anyway, I guess I'll try to make my way to the end of the series quickly
> and then somehow try to verify whatever the state is then.
> 
> Kevin
> 

Re: [PATCH v6 05/18] job.h: add _locked duplicates for job API functions called with and without job_mutex

[Emanuele Giuseppe Esposito]

Am 03/06/2022 um 18:17 schrieb Kevin Wolf:
> Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
>> In preparation to the job_lock/unlock usage, create _locked
>> duplicates of some functions, since they will be sometimes called with
>> job_mutex held (mostly within job.c),
>> and sometimes without (mostly from JobDrivers using the job API).
>>
>> Therefore create a _locked version of such function, so that it
>> can be used in both cases.
>>
>> List of functions duplicated as _locked:
>> job_is_ready (both versions are public)
>> job_is_completed (both versions are public)
>> job_is_cancelled (_locked version is public, needed by mirror.c)
>> job_pause_point (_locked version is static, purely done to simplify the code)
>> job_cancel_requested (_locked version is static)
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito 
>> ---
>>  include/qemu/job.h | 25 +---
>>  job.c  | 48 --
>>  2 files changed, 64 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 6000463126..aa33d091b1 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -473,21 +473,40 @@ const char *job_type_str(const Job *job);
>>  /** Returns true if the job should not be visible to the management layer. 
>> */
>>  bool job_is_internal(Job *job);
>>  
>> -/** Returns whether the job is being cancelled. */
>> +/**
>> + * Returns whether the job is being cancelled.
>> + * Called with job_mutex *not* held.
>> + */
>>  bool job_is_cancelled(Job *job);
>>  
>> +/** Just like job_is_cancelled, but called between job_lock and job_unlock 
>> */
>> +bool job_is_cancelled_locked(Job *job);
>> +
>>  /**
>>   * Returns whether the job is scheduled for cancellation (at an
>>   * indefinite point).
>> + * Called with job_mutex *not* held.
>>   */
>>  bool job_cancel_requested(Job *job);
>>  
>> -/** Returns whether the job is in a completed state. */
>> +/**
>> + * Returns whether the job is in a completed state.
>> + * Called with job_mutex *not* held.
>> + */
>>  bool job_is_completed(Job *job);
>>  
>> -/** Returns whether the job is ready to be completed. */
>> +/** Same as job_is_completed(), but assumes job_lock is held. */
>> +bool job_is_completed_locked(Job *job);
> 
> Any reason why this comment is phrased differently than for
> job_is_cancelled_locked()? I don't mind which one we use, but if they
> should express the same thing, it would be better to have the same
> wording. If they should express different things, it need to be clearer
> what they are.
> 

Makes sense, I will switch to the same format as job_is_cancelled_locked().

Emanuele

> Also, I assume job_mutex is meant because job_lock() is a function, not
> the lock that is held.
> 
>> +/**
>> + * Returns whether the job is ready to be completed.
>> + * Called with job_mutex *not* held.
>> + */
>>  bool job_is_ready(Job *job);
>>  
>> +/** Same as job_is_ready(), but assumes job_lock is held. */
>> +bool job_is_ready_locked(Job *job);
> 
> Same as above.
> 
>>  /**
>>   * Request @job to pause at the next pause point. Must be paired with
>>   * job_resume(). If the job is supposed to be resumed by user action, call
> 
> Kevin
> 

Re: [PATCH v6 02/18] job.h: categorize fields in struct Job

[Emanuele Giuseppe Esposito]

Am 03/06/2022 um 18:00 schrieb Kevin Wolf:
> Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
>> Categorize the fields in struct Job to understand which ones
>> need to be protected by the job mutex and which don't.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito 
> 
> I suppose it might be a result of moving things back and forth between
> patches, but this patch doesn't really define separate categories.
> 
>>  include/qemu/job.h | 59 ++
>>  1 file changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index d1192ffd61..86ec46c09e 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn;
>>   * Long-running operation.
>>   */
>>  typedef struct Job {
>> +
>> +/* Fields set at initialization (job_create), and never modified */
> 
> This is clearly a comment starting a category, but I can't see any other
> comment indicating that another category would start.
> 
>>  /** The ID of the job. May be NULL for internal jobs. */
>>  char *id;
>>  
>> -/** The type of this job. */
>> +/**
>> + * The type of this job.
>> + * All callbacks are called with job_mutex *not* held.
>> + */
>>  const JobDriver *driver;
>>  
>> -/** Reference count of the block job */
>> -int refcnt;
>> -
>> -/** Current state; See @JobStatus for details. */
>> -JobStatus status;
>> -
>> -/** AioContext to run the job coroutine in */
>> -AioContext *aio_context;
>> -
>>  /**
>>   * The coroutine that executes the job.  If not NULL, it is reentered 
>> when
>>   * busy is false and the job is cancelled.
>> + * Initialized in job_start()
>>   */
>>  Coroutine *co;
>>  
>> +/** True if this job should automatically finalize itself */
>> +bool auto_finalize;
>> +
>> +/** True if this job should automatically dismiss itself */
>> +bool auto_dismiss;
>> +
>> +/** The completion function that will be called when the job completes. 
>>  */
>> +BlockCompletionFunc *cb;
>> +
>> +/** The opaque value that is passed to the completion function.  */
>> +void *opaque;
>> +
>> +/* ProgressMeter API is thread-safe */
>> +ProgressMeter progress;
>> +
>> +
> 
> And the end of the series, this is where the cutoff is and the rest is:
> 
> /** Protected by job_mutex */
> 
> With this in mind, it seems correct to me that everything above progress
> is indeed never changed after creating the job. Of course, it's hard to
> tell without looking at the final result, so if you have to respin for
> some reason, it would be good to mark the end of the section more
> clearly for the intermediate state to make sense.

How can I do that? I left two empty lines in this patch, I don't know
what to use to signal the end of this category.

Emanuele

Re: [PATCH v6 15/18] job: detect change of aiocontext within job coroutine

[Emanuele Giuseppe Esposito]

Am 03/06/2022 um 18:59 schrieb Kevin Wolf:
> Am 14.03.2022 um 14:37 hat Emanuele Giuseppe Esposito geschrieben:
>> From: Paolo Bonzini 
>>
>> We want to make sure access of job->aio_context is always done
>> under either BQL or job_mutex. The problem is that using
>> aio_co_enter(job->aiocontext, job->co) in job_start and job_enter_cond
>> makes the coroutine immediately resume, so we can't hold the job lock.
>> And caching it is not safe either, as it might change.
>>
>> job_start is under BQL, so it can freely read job->aiocontext, but
>> job_enter_cond is not. In order to fix this, use aio_co_wake():
>> the advantage is that it won't use job->aiocontext, but the
>> main disadvantage is that it won't be able to detect a change of
>> job AioContext.
>>
>> Calling bdrv_try_set_aio_context() will issue the following calls
>> (simplified):
>> * in terms of  bdrv callbacks:
>>   .drained_begin -> .set_aio_context -> .drained_end
>> * in terms of child_job functions:
>>   child_job_drained_begin -> child_job_set_aio_context -> 
>> child_job_drained_end
>> * in terms of job functions:
>>   job_pause_locked -> job_set_aio_context -> job_resume_locked
>>
>> We can see that after setting the new aio_context, job_resume_locked
>> calls again job_enter_cond, which then invokes aio_co_wake(). But
>> while job->aiocontext has been set in job_set_aio_context,
>> job->co->ctx has not changed, so the coroutine would be entering in
>> the wrong aiocontext.
>>
>> Using aio_co_schedule in job_resume_locked() might seem as a valid
>> alternative, but the problem is that the bh resuming the coroutine
>> is not scheduled immediately, and if in the meanwhile another
>> bdrv_try_set_aio_context() is run (see test_propagate_mirror() in
>> test-block-iothread.c), we would have the first schedule in the
>> wrong aiocontext, and the second set of drains won't even manage
>> to schedule the coroutine, as job->busy would still be true from
>> the previous job_resume_locked().
>>
>> The solution is to stick with aio_co_wake(), but then detect every time
>> the coroutine resumes back from yielding if job->aio_context
>> has changed. If so, we can reschedule it to the new context.
>>
>> Check for the aiocontext change in job_do_yield_locked because:
>> 1) aio_co_reschedule_self requires to be in the running coroutine
>> 2) since child_job_set_aio_context allows changing the aiocontext only
>>while the job is paused, this is the exact place where the coroutine
>>resumes, before running JobDriver's code.
>>
>> Reviewed-by: Stefan Hajnoczi 
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  job.c | 24 +---
>>  1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/job.c b/job.c
>> index 89c0e6bed9..10a5981748 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -543,11 +543,12 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job 
>> *job))
>>  return;
>>  }
>>  
>> -assert(!job->deferred_to_main_loop);
> 
> Why doesn't this assertion hold true any more?

Theoretically this is useless, since we are in the same critical section
once the new lock is used, right? I don't recall any other reason, I
will restore it, if I need to respin (depends on what we decide in the
other patches feedback you provided)

Thank you,
Emanuele

> 
>>  timer_del(&job->sleep_timer);
>>  job->busy = true;
>>  real_job_unlock();
>> -aio_co_enter(job->aio_context, job->co);
>> +job_unlock();
>> +aio_co_wake(job->co);
>> +job_lock();
>>  }
>>  
>>  void job_enter(Job *job)
>> @@ -568,6 +569,8 @@ void job_enter(Job *job)
>>   */
>>  static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns)
>>  {
>> +AioContext *next_aio_context;
>> +
>>  real_job_lock();
>>  if (ns != -1) {
>>  timer_mod(&job->sleep_timer, ns);
>> @@ -579,6 +582,20 @@ static void coroutine_fn job_do_yield_locked(Job *job, 
>> uint64_t ns)
>>  qemu_coroutine_yield();
>>  job_lock();
>>  
>> +next_aio_context = job->aio_context;
>> +/*
>> + * Coroutine has resumed, but in the meanwhile the job AioContext
>> + * might have changed via bdrv_try_set_aio_context(), so we need to move
>> + * the coroutine too in the new aiocontext.
>> + */
>> +while (qemu_get_current_aio_context() != next_aio_context) {
>> +job_unlock();
>> +aio_co_reschedule_self(next_aio_context);
>> +job_lock();
>> +next_aio_context = job->aio_context;
>> +}
>> +
>> +
> 
> Extra empty line.
> 
>>  /* Set by job_enter_cond_locked() before re-entering the coroutine.  */
>>  assert(job->busy);
>>  }
>> @@ -680,7 +697,6 @@ void job_resume_locked(Job *job)
>>  if (job->pause_count) {
>>  return;
>>  }
>> -
>>  /* kick only if no timer is pending */
>>  job_enter_cond_locked(job, job_timer_not_pending_locked);
>>  }
> 
> This hunk looks unrelated.
> 
> Kevin
> 

Re: ioregionfd with io_uring IORING_OP_URING_CMD

[Elena]

On Mon, Jun 06, 2022 at 03:57:47PM +0100, Stefan Hajnoczi wrote:
> Hi,
> During Elena Afanasova's Outreachy project we discussed whether
> ioregionfd should be a custom struct file_operations (anon inode) or a
> userspace-provided file (socketpair, UNIX domain socket, etc).
>

Hello Stefan,

> Back then it seemed more flexible and simpler to let userspace provide
> the file. It may be worth revisiting this decision in light of the
> recent io_uring IORING_OP_URING_CMD feature, which fits for this
> performance-critical interface.
>
And Paolo was asking about io_uring in the review of the initial
patches.

> IORING_OP_URING_CMD involves a new struct file_operations->uring_cmd()
> callback. It's a flexible userspace interface like ioctl(2) but designed
> to be asynchronous. ioregionfd can provide a uring_cmd() that reads a
> ioregionfd response from userspace and then writes the next ioregionfd
> request to userspace.
> 
> This single operation merges the request/response so only 1 syscall is
> necessary per KVM MMIO/PIO exit instead of a read() + write(). Bypassing
> the net/socket infrastructure is likely to help too.
> 
> It would be interesting to benchmark this and compare it against the
> existing userspace-provided file approach. Although it's not the same
> scenario, results for the Linux NVMe driver using ->uring_cmd() are
> promising:
> https://www.snia.org/educational-library/enabling-asynchronous-i-o-passthru-nvme-native-applications-2021
> 
Yes, looks interesting, we were thinking about adding this.

> The downside is it requires more code than general purpose I/O. In
> addition to ->uring_cmd(), it's also worth implementing struct
> file_operations read/write/poll so traditional file I/O syscalls work
> for simple applications that don't want to use io_uring.
> 
> It's possible to add ->uring_cmd() later but as a userspace developer I
> would prefer the ->uring_cmd() approach, so I'm not sure it's worth
> committing to the existing userspace-provided file approach?

Makes total sense. I am going to start working on this and will
come back with more questions.

Thank you!
Elena
> 
> Stefan

Re: [PATCH v2 03/16] ppc/pnv: add PnvPHB base/proxy device

[Daniel Henrique Barboza]

On 6/7/22 05:44, Frederic Barrat wrote:



On 07/06/2022 08:42, Cédric Le Goater wrote:

On 6/2/22 18:16, Frederic Barrat wrote:



On 31/05/2022 23:49, Daniel Henrique Barboza wrote:

The PnvPHB device is going to be the base device for all other powernv
PHBs. It consists of a device that has the same user API as the other
PHB, namely being a PCIHostBridge and having chip-id and index
properties. It also has a 'backend' pointer that will be initialized
with the PHB implementation that the device is going to use.

The initialization of the PHB backend is done by checking the PHB
version via a 'version' attribute that can be set via a global machine
property.  The 'version' field will be used to make adjustments based on
the running version, e.g. PHB3 uses a 'chip' reference while PHB4 uses
'pec'. To init the PnvPHB bus we'll rely on helpers for each version.
The version 3 helper is already added (pnv_phb3_bus_init), the PHB4
helper will be added later on.

For now let's add the basic logic of the PnvPHB object, which consists
mostly of pnv_phb_realize() doing all the work of checking the
phb->version set, initializing the proper backend, passing through its
attributes to the chosen backend, finalizing the backend realize and
adding a root port in the end.

Signed-off-by: Daniel Henrique Barboza 
---
  hw/pci-host/meson.build |   3 +-
  hw/pci-host/pnv_phb.c   | 123 
  hw/pci-host/pnv_phb.h   |  39 +
  3 files changed, 164 insertions(+), 1 deletion(-)
  create mode 100644 hw/pci-host/pnv_phb.c
  create mode 100644 hw/pci-host/pnv_phb.h

diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
index c07596d0d1..e832babc9d 100644
--- a/hw/pci-host/meson.build
+++ b/hw/pci-host/meson.build
@@ -35,5 +35,6 @@ specific_ss.add(when: 'CONFIG_PCI_POWERNV', if_true: files(
    'pnv_phb3_msi.c',
    'pnv_phb3_pbcq.c',
    'pnv_phb4.c',
-  'pnv_phb4_pec.c'
+  'pnv_phb4_pec.c',
+  'pnv_phb.c',
  ))
diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
new file mode 100644
index 00..fa8472622f
--- /dev/null
+++ b/hw/pci-host/pnv_phb.c
@@ -0,0 +1,123 @@
+/*
+ * QEMU PowerPC PowerNV Proxy PHB model
+ *
+ * Copyright (c) 2022, IBM Corporation.
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/visitor.h"
+#include "qapi/error.h"
+#include "hw/pci-host/pnv_phb.h"
+#include "hw/pci-host/pnv_phb3.h"
+#include "hw/pci-host/pnv_phb4.h"
+#include "hw/ppc/pnv.h"
+#include "hw/qdev-properties.h"
+#include "qom/object.h"
+
+
+static void pnv_phb_realize(DeviceState *dev, Error **errp)
+{
+    PnvPHB *phb = PNV_PHB(dev);
+    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
+    g_autofree char *phb_typename = NULL;
+    g_autofree char *phb_rootport_typename = NULL;
+
+    if (!phb->version) {
+    error_setg(errp, "version not specified");
+    return;
+    }
+
+    switch (phb->version) {
+    case 3:
+    phb_typename = g_strdup(TYPE_PNV_PHB3);
+    phb_rootport_typename = g_strdup(TYPE_PNV_PHB3_ROOT_PORT);
+    break;
+    case 4:
+    phb_typename = g_strdup(TYPE_PNV_PHB4);
+    phb_rootport_typename = g_strdup(TYPE_PNV_PHB4_ROOT_PORT);
+    break;
+    case 5:
+    phb_typename = g_strdup(TYPE_PNV_PHB5);
+    phb_rootport_typename = g_strdup(TYPE_PNV_PHB5_ROOT_PORT);
+    break;
+    default:
+    g_assert_not_reached();
+    }
+
+    phb->backend = object_new(phb_typename);
+    object_property_add_child(OBJECT(dev), "phb-device", phb->backend);
+
+    /* Passthrough child device properties to the proxy device */
+    object_property_set_uint(phb->backend, "index", phb->phb_id, errp);
+    object_property_set_uint(phb->backend, "chip-id", phb->chip_id, errp);
+    object_property_set_link(phb->backend, "phb-base", OBJECT(phb), errp);
+
+    if (phb->version == 3) {
+    object_property_set_link(phb->backend, "chip",
+ OBJECT(phb->chip), errp);
+    } else {
+    object_property_set_link(phb->backend, "pec", OBJECT(phb->pec), errp);
+    }



The patch is fine, but it just highlights that we're doing something wrong. I 
don't believe there's any reason for the chip/pec/phb relationship to be 
different between P8 and P9/P10. One day, a brave soul could try to unify the 
models, it would avoid test like that.


I think this is the first thing to do: introduce a PEC model to match
P8 HW and unify PHB3/4/5. We have been dragging this for too long (2014 ...)



I agree it's needed, but it's also orthogonal to this series. And I wouldn't 
force it on Daniel since we're getting into PCI territory.


I don't mind doing it after this series lands.


Thanks,


Daniel



   Fred




It shouldn't be that complex with all the cleanups that have been done.

Thanks,

C.



It would be a good cleanup series to do if we ever extend the model with yet 
ano

Re: [PATCH v5 08/45] block/snapshot: stress that we fallback to primary child

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

Actually what we chose is a primary child. Let's stress it in the code.

We are going to drop indirect pointer logic here in future. Actually
this commit simplifies the future work: we drop use of indirection in
the assertion now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/snapshot.c | 30 ++
  1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index d6f53c3065..f4ec4f9ef3 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -161,21 +161,14 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState 
*bs,
  static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
  {
  BdrvChild **fallback;
-BdrvChild *child;
+BdrvChild *child = bdrv_primary_child(bs);
  
-/*

- * The only BdrvChild pointers that are safe to modify (and which
- * we can thus return a reference to) are bs->file and
- * bs->backing.
- */
-fallback = &bs->file;
-if (!*fallback && bs->drv && bs->drv->is_filter) {
-fallback = &bs->backing;
-}
-
-if (!*fallback) {
+/* We allow fallback only to primary child */
+if (!child) {
  return NULL;
  }
+fallback = (child == bs->file ? &bs->file : &bs->backing);
+assert(*fallback == child);
  
  /*

   * Check that there are no other children that would need to be
@@ -309,15 +302,12 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
  }
  
  /*

- * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
- * was closed above and set to NULL, but the .bdrv_open() call
- * has opened it again, because we set the respective option
- * (with the qdict_put_str() call above).
- * Assert that .bdrv_open() has attached some child on
- * *fallback_ptr, and that it has attached the one we wanted
- * it to (i.e., fallback_bs).
+ * fallback was a primary child. It was closed above and set to NULL,
+ * but the .bdrv_open() call has opened it again, because we set the
+ * respective option (with the qdict_put_str() call above).
+ * Assert that .bdrv_open() has attached some BDS as primary child.


s/some/the right/?

Reviewed-by: Hanna Reitz 


   */
-assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
+assert(bdrv_primary_bs(bs) == fallback_bs);
  bdrv_unref(fallback_bs);
  return ret;
  }

Re: [PATCH v3 14/49] include/exec: Move gdb open flags to gdbstub.h

[Alex Bennée]

Richard Henderson  writes:

> There were 3 copies of these flags.  Place them in the
> file with gdb_do_syscall, with which they belong.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH] tcg: Special case split barriers before/after load

[Redha]

Hi Richard and Philippe,

I was finally able to test the patch.

As expected, the fences generated by TCG with your patch are the same
ones as in mine.

However, I was not able to reproduce the failure with the ahci-test on
my ARM system (2x 28-core Thunder X2, 4 threads per core).  I ran 500
tests concurrently to load the system, but I did not get the assert
failure.  However, I got another qemu error 229 times:

  qemu-system-x86_64: Failed to get "write" lock
  Is another process using the image [/tmp/qtest.IksKVs]?

Each time with a different temporary path. Not sure if that is directly 
related to the actual test.

Also, are we sure the test failure on the gitlab runner is due to memory
ordering issues with regular memory accesses?  Is there a way to check
the code generated by TCG for this test, similar to `-d op,out_asm`?


Redha



On 30/05/2022 17:10, Philippe Mathieu-Daudé wrote:
> Hi Richard,
> 
> On 1/5/22 01:45, Richard Henderson wrote:
>> When st:ld is not required by the guest but ld:st is, we can
>> put ld:ld+ld:st barriers after loads, and then st:st barriers
>> before stores to enforce all required barriers.
>>
>> The st:st barrier is often special cased by hosts, and that
>> is expected to be more efficient than a full barrier.
>>
>> Signed-off-by: Richard Henderson 
>> ---
>>
>> Redha, I expect this to produce exactly the same barriers as you
>> did with your 'fix guest memory ordering enforcement' patch.
>>
>> While this compiles, it does not fix the failures that I see
>> occasionally with our private gitlab runner.  The standalone
>> version of this failure is
>>
>>    export QTEST_QEMU_BINARY=./qemu-system-i386
>>    for i in `seq 1 100`; do
>>  ./tests/qtest/ahci-test > /dev/null &
>>    done
>>    wait
>>
>> About 10 to 15% of the runs will fail with
>>
>> ERROR:../src/tests/qtest/ahci-test.c:92:verify_state: assertion failed 
>> (ahci_fingerprint == ahci->fingerprint): (0xe000 == 0x29228086)
>>
>> Note that this test never seems to fail unless the system is under
>> load, thus starting 100 tests on my 80 core neoverse-n1 system.
>>
>>
>> r~
>>
>>
>> ---
>>   tcg/tcg-op.c | 55 +---
>>   1 file changed, 48 insertions(+), 7 deletions(-)
>>
>> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
>> index 5d48537927..4c568a2592 100644
>> --- a/tcg/tcg-op.c
>> +++ b/tcg/tcg-op.c
>> @@ -2834,9 +2834,6 @@ static void gen_ldst_i64(TCGOpcode opc, TCGv_i64 val, 
>> TCGv addr,
>>     static void tcg_gen_req_mo(TCGBar type)
>>   {
>> -#ifdef TCG_GUEST_DEFAULT_MO
>> -    type &= TCG_GUEST_DEFAULT_MO;
>> -#endif
>>   type &= ~TCG_TARGET_DEFAULT_MO;
>>   if (type) {
>>   tcg_gen_mb(type | TCG_BAR_SC);
>> @@ -2868,12 +2865,49 @@ static void plugin_gen_mem_callbacks(TCGv vaddr, 
>> MemOpIdx oi,
>>   #endif
>>   }
>>   +typedef enum {
>> +    BAR_LD_BEFORE,
>> +    BAR_LD_AFTER,
>> +    BAR_ST_BEFORE,
>> +} ChooseBarrier;
>> +
>> +static TCGBar choose_barrier(ChooseBarrier which)
>> +{
>> +#ifdef TCG_GUEST_DEFAULT_MO
>> +    const TCGBar guest_mo = TCG_GUEST_DEFAULT_MO;
>> +#else
>> +    const TCGBar guest_mo = TCG_MO_ALL;
>> +#endif
>> +    TCGBar ret[3];
>> +
>> +    if (guest_mo == 0) {
>> +    return 0;
>> +    }
> 
> This part ...:
> 
>> +    /*
>> + * Special case for i386 and s390x.  Because store-load is not
>> + * required by the guest, we can split the barriers such that we
>> + * wind up with a store-store barrier, which is expected to be
>> + * quicker on some hosts.
>> + */
>> +    if (guest_mo == (TCG_MO_ALL & ~TCG_MO_ST_LD)) {
>> +    ret[BAR_LD_BEFORE] = 0;
>> +    ret[BAR_LD_AFTER]  = TCG_MO_LD_LD | TCG_MO_LD_ST;
>> +    ret[BAR_ST_BEFORE] = TCG_MO_ST_ST;
>> +    } else {
> 
> ... could deserve another patch.
> 
>> +    ret[BAR_LD_BEFORE] = (TCG_MO_LD_LD | TCG_MO_ST_LD) & guest_mo;
>> +    ret[BAR_ST_BEFORE] = (TCG_MO_LD_ST | TCG_MO_ST_ST) & guest_mo;
>> +    ret[BAR_LD_AFTER]  = 0;
>> +    }
>> +    return ret[which];
>> +}
>> +
>>   void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, MemOp memop)
>>   {
>>   MemOp orig_memop;
>>   MemOpIdx oi;
>>   -    tcg_gen_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
>> +    tcg_gen_req_mo(choose_barrier(BAR_LD_BEFORE));
>> +
>>   memop = tcg_canonicalize_memop(memop, 0, 0);
>>   oi = make_memop_idx(memop, idx);
>>   @@ -2904,6 +2938,8 @@ void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, 
>> TCGArg idx, MemOp memop)
>>   g_assert_not_reached();
>>   }
>>   }
>> +
>> +    tcg_gen_req_mo(choose_barrier(BAR_LD_AFTER));
>>   }
>>     void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, MemOp 
>> memop)
>> @@ -2911,7 +2947,8 @@ void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, 
>> TCGArg idx, MemOp memop)
>>   TCGv_i32 swap = NULL;
>>   MemOpIdx oi;
>>   -    tcg_gen_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
>> +    tcg_gen_req_mo(choose_barrier(BAR_ST_BEFORE));
>> +
>>   memop = tcg

Re: [PATCH v3 15/49] include/exec: Move gdb_stat and gdb_timeval to gdbstub.h

[Alex Bennée]

Richard Henderson  writes:

> We have two copies of these structures, and require them
> in semihosting/ going forward.
>
> Signed-off-by: Richard Henderson 
> ---
>  include/exec/gdbstub.h| 25 +
>  target/m68k/m68k-semi.c   | 30 +++---
>  target/nios2/nios2-semi.c | 30 +++---
>  3 files changed, 31 insertions(+), 54 deletions(-)
>
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 2aaba9c723..33a262a5a3 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -20,6 +20,31 @@
>  #define GDB_O_TRUNC   0x400
>  #define GDB_O_EXCL0x800
>  
> +/* For gdb file i/o stat/fstat. */
> +typedef uint32_t gdb_mode_t;
> +typedef uint32_t gdb_time_t;
> +
> +struct gdb_stat {
> +  uint32_tgdb_st_dev; /* device */
> +  uint32_tgdb_st_ino; /* inode */
> +  gdb_mode_t  gdb_st_mode;/* protection */
> +  uint32_tgdb_st_nlink;   /* number of hard links */
> +  uint32_tgdb_st_uid; /* user ID of owner */
> +  uint32_tgdb_st_gid; /* group ID of owner */
> +  uint32_tgdb_st_rdev;/* device type (if inode device) */
> +  uint64_tgdb_st_size;/* total size, in bytes */
> +  uint64_tgdb_st_blksize; /* blocksize for filesystem I/O */
> +  uint64_tgdb_st_blocks;  /* number of blocks allocated */
> +  gdb_time_t  gdb_st_atime;   /* time of last access */
> +  gdb_time_t  gdb_st_mtime;   /* time of last modification */
> +  gdb_time_t  gdb_st_ctime;   /* time of last change */
> +} QEMU_PACKED;
> +
> +struct gdb_timeval {
> +  gdb_time_t tv_sec;  /* second */
> +  uint64_t tv_usec;   /* microsecond */
> +} QEMU_PACKED;
> +
>  #ifdef NEED_CPU_H
>  #include "cpu.h"
>  
> diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
> index 475a6b13b7..da0186f3ef 100644
> --- a/target/m68k/m68k-semi.c
> +++ b/target/m68k/m68k-semi.c
> @@ -45,30 +45,6 @@
>  #define HOSTED_ISATTY 12
>  #define HOSTED_SYSTEM 13
>  
> -typedef uint32_t gdb_mode_t;
> -typedef uint32_t gdb_time_t;
> -
> -struct m68k_gdb_stat {
> -  uint32_tgdb_st_dev; /* device */
> -  uint32_tgdb_st_ino; /* inode */
> -  gdb_mode_t  gdb_st_mode;/* protection */
> -  uint32_tgdb_st_nlink;   /* number of hard links */
> -  uint32_tgdb_st_uid; /* user ID of owner */
> -  uint32_tgdb_st_gid; /* group ID of owner */
> -  uint32_tgdb_st_rdev;/* device type (if inode device) */
> -  uint64_tgdb_st_size;/* total size, in bytes */
> -  uint64_tgdb_st_blksize; /* blocksize for filesystem I/O */
> -  uint64_tgdb_st_blocks;  /* number of blocks allocated */
> -  gdb_time_t  gdb_st_atime;   /* time of last access */
> -  gdb_time_t  gdb_st_mtime;   /* time of last modification */
> -  gdb_time_t  gdb_st_ctime;   /* time of last change */
> -} QEMU_PACKED;
> -
> -struct gdb_timeval {
> -  gdb_time_t tv_sec;  /* second */
> -  uint64_t tv_usec;   /* microsecond */
> -} QEMU_PACKED;
> -
>  static int translate_openflags(int flags)
>  {
>  int hf;
> @@ -90,9 +66,9 @@ static int translate_openflags(int flags)
>  
>  static void translate_stat(CPUM68KState *env, target_ulong addr, struct stat 
> *s)
>  {
> -struct m68k_gdb_stat *p;
> +struct gdb_stat *p;
>  
> -if (!(p = lock_user(VERIFY_WRITE, addr, sizeof(struct m68k_gdb_stat), 
> 0)))
> +if (!(p = lock_user(VERIFY_WRITE, addr, sizeof(struct gdb_stat),
> 0)))

checkpatch hard fails on the assignment in an if condition so it's
probably worth cleaning that up while you go.

Otherwise:

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v5 09/45] Revert "block: Let replace_child_noperm free children"

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

We are going to reimplement this behavior (clear bs->file / bs->backing
pointers automatically when child->bs is cleared) in a nicer way.

This reverts commit b0a9f6fed3d80de610dcd04a7e66f9f30a04174f.


This doesn’t really explain why it’s fine to revert this commit here.  
As far as I understand, the bug that was fixed in that commit will 
resurface when it is reverted without the proposed reimplementation, so 
technically, we cannot revert before reimplementing.


As far as I can guess, it’d be unwieldy to do the reimplementation while 
these existing changes are in the way, and it’d be one bomb of a patch 
to squash these five patches (9 to 14) into one, and that’s why you’ve 
chosen to do it this way around.


But technically, we can’t willingly break something just to keep patches 
nicer.  We can make exceptions, but then there needs to be justification 
here in the commit message.


(Or perhaps I’m wrong and it is fine at this point to revert the patch, 
but then I’d like to see the explanation for that, too, because I can’t 
see it myself.)


Hanna

Re: ioregionfd with io_uring IORING_OP_URING_CMD

[Stefan Hajnoczi]

On Tue, 7 Jun 2022 at 14:32, Elena  wrote:
> On Mon, Jun 06, 2022 at 03:57:47PM +0100, Stefan Hajnoczi wrote:
> > The downside is it requires more code than general purpose I/O. In
> > addition to ->uring_cmd(), it's also worth implementing struct
> > file_operations read/write/poll so traditional file I/O syscalls work
> > for simple applications that don't want to use io_uring.
> >
> > It's possible to add ->uring_cmd() later but as a userspace developer I
> > would prefer the ->uring_cmd() approach, so I'm not sure it's worth
> > committing to the existing userspace-provided file approach?
>
> Makes total sense. I am going to start working on this and will
> come back with more questions.

Good to hear!

Userspace needs a way to create these fds. I think a new
ioctl(KVM_CREATE_IOREGIONFD) is needed. Then the fd can be passed back
to KVM_SET_IOREGION.

Stefan

Re: [RFC PATCH 3/3] hw/openrisc: Add the OpenRISC virtual machine

[Arnd Bergmann]

On Tue, Jun 7, 2022 at 2:12 PM Stafford Horne  wrote:
> On Tue, Jun 07, 2022 at 11:43:08AM +0100, Peter Maydell wrote:
>
> However, in a followup mail from Laurent we see:
>
>   https://lore.kernel.org/lkml/cb884368-0226-e913-80d2-62d2b7b2e...@vivier.eu/
>
>   The reference document[1] doesn't define the endianness of goldfish.
>
>   [1] 
> https://android.googlesource.com/platform/external/qemu/+/master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT
>
>
> The documentation does not clearly specify it.  So maybe maybe or1k should 
> just
> be updated on the linux side and add gf_ioread32/gf_iowrite32 big-endian
> accessors.

I don't think it makes any sense to use big-endian for a new
architecture, just use
the default little-endian implementation on the linux side, and change
the qemu code
to have the backward-compatibility hack for m68k while using big-endian for
the rest.

   Arnd

Re: [PATCH 37/71] target/arm: Add cpu properties for SME

[Richard Henderson]

On 6/7/22 02:47, Peter Maydell wrote:

+void arm_cpu_sme_finalize(ARMCPU *cpu, Error **errp)
+{
+uint32_t vq_map = cpu->sme_vq.map;
+uint32_t vq_init = cpu->sme_vq.init;
+uint32_t vq_supported = cpu->sme_vq.supported;
+uint32_t vq;
+
+if (vq_map == 0) {
+if (!cpu_isar_feature(aa64_sme, cpu)) {
+cpu->isar.id_aa64smfr0 = 0;
+return;
+}
+
+/* TODO: KVM will require limitations via SMCR_EL2. */
+vq_map = vq_supported & ~vq_init;


Do we currently forbid setting these properties entirely for KVM
(or just not provide them) ?


I do not provide them.


These new properties should be documented in
docs/system/arm/cpu-features.rst, similar to the SVE ones.


Oops, yes.


r~

Re: [PATCH 39/71] target/arm: Add SVL to TB flags

[Richard Henderson]

On 6/7/22 02:58, Peter Maydell wrote:

@@ -3292,6 +3292,7 @@ FIELD(TBFLAG_A64, MTE0_ACTIVE, 19, 1)
  FIELD(TBFLAG_A64, SMEEXC_EL, 20, 2)
  FIELD(TBFLAG_A64, PSTATE_SM, 22, 1)
  FIELD(TBFLAG_A64, PSTATE_ZA, 23, 1)
+FIELD(TBFLAG_A64, SVL, 24, 4)


Given that both SVE and SME start with an 'S', maybe
"SME_VL" would be less prone to confusion? On the other hand,
SVL is the architectural name, so maybe that's best.


Yeah, my first version used SME_LEN, but in the end I thought using the architectural name 
was best.  Just above, there's commentary using the other architectural names "VL" and "NVL".



+static inline int sme_vq_cached(CPUARMState *env)


Same remark as earlier about not needing to put "cached" in the function name.


Already fixed.  :-)


r~

Re: [PATCH 41/71] target/arm: Add infrastructure for disas_sme

[Richard Henderson]

On 6/7/22 03:03, Peter Maydell wrote:

On Thu, 2 Jun 2022 at 23:41, Richard Henderson
 wrote:


This includes the build rules for the decoder, and the
new file for translation, but excludes any instructions.

Signed-off-by: Richard Henderson 



@@ -14814,7 +14814,12 @@ static void aarch64_tr_translate_insn(DisasContextBase 
*dcbase, CPUState *cpu)
  }

  switch (extract32(insn, 25, 4)) {
-case 0x0: case 0x1: case 0x3: /* UNALLOCATED */
+case 0x0:
+if (!disas_sme(s, insn)) {
+unallocated_encoding(s);
+}
+break;
+case 0x1: case 0x3: /* UNALLOCATED */
  unallocated_encoding(s);
  break;
  case 0x2:


This is grabbing slightly more of the encoding space than it should
according to the Arm ARM Table C4-1 "Main encoding table": SME
encodings require bit 31 == 1 (unlike SVE where bit 31 is not decoded
at this level).


Yeah, well, full and proper decode is done in the generated decoder.
I don't feel the need to distinguish that bit here.

r~



Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v5 09/45] Revert "block: Let replace_child_noperm free children"

[Vladimir Sementsov-Ogievskiy]

On 6/7/22 17:03, Hanna Reitz wrote:

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

We are going to reimplement this behavior (clear bs->file / bs->backing
pointers automatically when child->bs is cleared) in a nicer way.

This reverts commit b0a9f6fed3d80de610dcd04a7e66f9f30a04174f.


This doesn’t really explain why it’s fine to revert this commit here. As far as 
I understand, the bug that was fixed in that commit will resurface when it is 
reverted without the proposed reimplementation, so technically, we cannot 
revert before reimplementing.

As far as I can guess, it’d be unwieldy to do the reimplementation while these 
existing changes are in the way, and it’d be one bomb of a patch to squash 
these five patches (9 to 14) into one, and that’s why you’ve chosen to do it 
this way around.


Yes, that's the reason



But technically, we can’t willingly break something just to keep patches nicer. 
 We can make exceptions, but then there needs to be justification here in the 
commit message.


Agree, will add.

As far as I remember (and after re-reading commit message) b0a9f6fed3d80de610dc 
was not a direct fix of some concrete bug. It was a measure to prevent 
theoretic problems. And we don't have any test for it. So I think, breaking 
bisect at this point for some future test is not too bad.



(Or perhaps I’m wrong and it is fine at this point to revert the patch, but 
then I’d like to see the explanation for that, too, because I can’t see it 
myself.)

Hanna





--
Best regards,
Vladimir

Re: [PATCH v6 02/18] job.h: categorize fields in struct Job

[Paolo Bonzini]

On 6/7/22 15:20, Emanuele Giuseppe Esposito wrote:



Am 03/06/2022 um 18:00 schrieb Kevin Wolf:

Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:

Categorize the fields in struct Job to understand which ones
need to be protected by the job mutex and which don't.

Signed-off-by: Emanuele Giuseppe Esposito 


I suppose it might be a result of moving things back and forth between
patches, but this patch doesn't really define separate categories.


  include/qemu/job.h | 59 ++
  1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index d1192ffd61..86ec46c09e 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn;
   * Long-running operation.
   */
  typedef struct Job {
+
+/* Fields set at initialization (job_create), and never modified */


This is clearly a comment starting a category, but I can't see any other
comment indicating that another category would start.


  /** The ID of the job. May be NULL for internal jobs. */
  char *id;
  
-/** The type of this job. */

+/**
+ * The type of this job.
+ * All callbacks are called with job_mutex *not* held.
+ */
  const JobDriver *driver;
  
-/** Reference count of the block job */

-int refcnt;
-
-/** Current state; See @JobStatus for details. */
-JobStatus status;
-
-/** AioContext to run the job coroutine in */
-AioContext *aio_context;
-
  /**
   * The coroutine that executes the job.  If not NULL, it is reentered when
   * busy is false and the job is cancelled.
+ * Initialized in job_start()
   */
  Coroutine *co;
  
+/** True if this job should automatically finalize itself */

+bool auto_finalize;
+
+/** True if this job should automatically dismiss itself */
+bool auto_dismiss;
+
+/** The completion function that will be called when the job completes.  */
+BlockCompletionFunc *cb;
+
+/** The opaque value that is passed to the completion function.  */
+void *opaque;
+
+/* ProgressMeter API is thread-safe */
+ProgressMeter progress;
+
+


And the end of the series, this is where the cutoff is and the rest is:

 /** Protected by job_mutex */

With this in mind, it seems correct to me that everything above progress
is indeed never changed after creating the job. Of course, it's hard to
tell without looking at the final result, so if you have to respin for
some reason, it would be good to mark the end of the section more
clearly for the intermediate state to make sense.


How can I do that? I left two empty lines in this patch, I don't know
what to use to signal the end of this category.


Can you already add "/** Protected by AioContext lock */" in this patch 
and then change it later?


Paolo

Re: [PATCH 00/28] target/arm: Split out ptw.c from helper.c

[Peter Maydell]

On Sat, 4 Jun 2022 at 05:09, Richard Henderson
 wrote:
>
> The object here is to move 2500 lines out of helper.c.  Yay!
>



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH v5 13/45] block: Manipulate bs->file / bs->backing pointers in .attach/.detach

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

bs->file and bs->backing are a kind of duplication of part of
bs->children. But very useful diplication, so let's not drop them at
all:)

We should manage bs->file and bs->backing in same place, where we
manage bs->children, to keep them in sync.

Moreover, generic io paths are unprepared to BdrvChild without a bs, so
it's double good to clear bs->file / bs->backing when we detach the
child.


I think this was reproducible (rarely) with 030, but I can’t reproduce 
it now.  Oh well.



Detach is simple: if we detach bs->file or bs->backing child, just
set corresponding field to NULL.

Attach is a bit more complicated. But we still can precisely detect
should we set one of bs->file / bs->backing or not:

- if role is BDRV_CHILD_COW, we definitely deal with bs->backing
- else, if role is BDRV_CHILD_FILTERED (it must be also
   BDRV_CHILD_PRIMARY), it's a filtered child. Use
   bs->drv->filtered_child_is_backing to chose the pointer field to
   modify.
- else, if role is BDRV_CHILD_PRIMARY, we deal with bs->file
- in all other cases, it's neither bs->backing nor bs->file. It's some
   other child and we shouldn't care


Sounds correct.


OK. This change brings one more good thing: we can (and should) get rid
of all indirect pointers in the block-graph-change transactions:

bdrv_attach_child_common() stores BdrvChild** into transaction to clear
it on abort.

bdrv_attach_child_common() has two callers: bdrv_attach_child_noperm()
just pass-through this feature, bdrv_root_attach_child() doesn't need
the feature.

Look at bdrv_attach_child_noperm() callers:
   - bdrv_attach_child() doesn't need the feature
   - bdrv_set_file_or_backing_noperm() uses the feature to manage
 bs->file and bs->backing, we don't want it anymore
   - bdrv_append() uses the feature to manage bs->backing, again we
 don't want it anymore

So, we should drop this stuff! Great!

We still keep BdrvChild** argument to return the child and int return
value, and not move to simply returning BdrvChild*, as we don't want to
lose int return values.

However we don't require *@child to be NULL anymore, and even allow
@child to be NULL, if caller don't need the new child pointer.

Finally, we now set .file / .backing automatically in generic code and
want to restring setting them by hand outside of .attach/.detach.
So, this patch cleanups all remaining places where they were set.
To find such places I use:

   git grep '\->file ='
   git grep '\->backing ='
   git grep '&.*\'
   git grep '&.*\'


Awesome.

block/snapshot-access.c needs a touchup, but other than that, this still 
seems to hold.



Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c  | 156 ++-
  block/raw-format.c   |   4 +-
  block/snapshot.c |   1 -
  include/block/block_int-common.h |  15 ++-
  tests/unit/test-bdrv-drain.c |  10 +-
  5 files changed, 89 insertions(+), 97 deletions(-)

diff --git a/block.c b/block.c
index 8e8ed639fe..6b43e101a1 100644
--- a/block.c
+++ b/block.c
@@ -1438,9 +1438,33 @@ static void bdrv_child_cb_attach(BdrvChild *child)
  
  assert_bdrv_graph_writable(bs);

  QLIST_INSERT_HEAD(&bs->children, child, next);
-
-if (child->role & BDRV_CHILD_COW) {
+if (bs->drv->is_filter | (child->role & BDRV_CHILD_FILTERED)) {


Should be `||`.


+/*
+ * Here we handle filters and block/raw-format.c when it behave like
+ * filter.


I’d like this comment to expand on how they are handled.

For example, that they generally have a single PRIMARY child, which is 
also the FILTERED child, and that they may have multiple more children, 
but none of them will be a COW child.  So bs->file will be the PRIMARY 
child, unless the PRIMARY child goes into bs->backing on exceptional 
cases; and bs->backing will be nothing else.  (Which is why we ignore 
all other children.)



+ */
+assert(!(child->role & BDRV_CHILD_COW));
+if (child->role & (BDRV_CHILD_PRIMARY | BDRV_CHILD_FILTERED)) {


Why do we check for FILTERED here?  It appears to me that PRIMARY is the 
flag that tells us to put this child into bs->file (but for filters, 
sometimes we have to make an exception and put it into bs->backing).


Is the check for FILTERED just a safeguard, so that filter drivers 
always set the two in tandem?  If so, I’d make the condition just `role 
& PRIMARY` and then in an `else` path assert that `!(role & FILTERED)`.



+assert(child->role & BDRV_CHILD_PRIMARY);
+assert(child->role & BDRV_CHILD_FILTERED);
+assert(!bs->backing);
+assert(!bs->file);
+
+if (bs->drv->filtered_child_is_backing) {
+bs->backing = child;
+} else {
+bs->file = child;
+}
+}


[...]


@@ -2897,11 +2925,11 @@ static TransactionActionDrv 
bdrv_attach_child_common_drv = {
  /*
   * Common

Re: [PATCH 00/50] PS2 device QOMification - part 1

[Mark Cave-Ayland]

On 22/05/2022 19:17, Mark Cave-Ayland wrote:


This series came about when looking at improving the LASI PS2 device for
the HPPA machine: there were improvements that I was keen to make, but
was restricted because the PS2 device(s) weren't QOMified.

Trying to do everything in a single patchset would be a very large series
indeed, so here is part 1 of the series which does the basic QOMification
process and consists of:

- QOMifying the basic PS2, PS2 keyboard and PS2 mouse types

- Moving any functionality that exists in a global device init function
   directly into the relevant device, so that all device behaviour is
   configured using qdev properties and QOM

- Introducing a new I8042_MMIO type for use by the MIPS magnum machine

- Switch all PS2 devices to use qdev gpios for IRQs instead of using the
   update_irq() callback function along with the update_arg opaque

Once this work has been done, a follow-up part 2 series will finish the
remainder of the work which involves i) improving the QOM object model
now QOMification is complete and ii) removing the legacy global device
init functions for PS2 and related devices.

Testing for this series has comprised of booting a machine with each type
of PS2 device and confirming that i) the machine responds to keypresses
when using a graphical console and ii) completing a successful migration
from a machine with this series applies back to a machine running latest
git master. The test machines I used were:

- qemu-system-x86_64 -M pc for the I8042 device
- qemu-system-hppa for the LASIPS2 device
- qemu-system-arm -M versatilepb for the PL050 device
- qemu-system-mips64el -M magnum for the I8042_MMIO device

Finally the QOM tree changes caused by QOMification of the PS2 devices
trigger a failure due to a bug in the bios-tables-test qtest for subtest
/x86_64/acpi/q35/viot. This can be fixed by applying the series at
https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg04266.html
"hw/acpi/viot: generate stable VIOT ACPI tables" first.

Signed-off-by: Mark Cave-Ayland 


Mark Cave-Ayland (50):
   ps2: checkpatch fixes
   ps2: QOMify PS2State
   ps2: QOMify PS2KbdState
   ps2: QOMify PS2MouseState
   ps2: move QOM type definitions from ps2.c to ps2.h
   ps2: improve function prototypes in ps2.c and ps2.h
   ps2: introduce PS2DeviceClass
   ps2: implement ps2_reset() for the PS2_DEVICE QOM type based upon
 ps2_common_reset()
   ps2: remove duplicate setting of scancode_set in ps2_kbd_init()
   ps2: implement ps2_kbd_realize() and use it to register
 ps2_keyboard_handler
   ps2: implement ps2_mouse_realize() and use it to register
 ps2_mouse_handler
   ps2: don't use vmstate_register() in ps2_kbd_init()
   ps2: don't use vmstate_register() in ps2_mouse_init()
   pl050: checkpatch fixes
   pl050: split pl050_update_irq() into separate pl050_set_irq() and
 pl050_update_irq() functions
   lasips2: spacing fixes
   lasips2: rename ps2dev_update_irq() to lasips2_port_set_irq()
   pckbd: checkpatch fixes
   pckbd: move KBDState from pckbd.c to i8042.h
   pckbd: move ISAKBDState from pckbd.c to i8042.h
   pckbd: introduce new I8042_MMIO QOM type
   pckbd: implement i8042_mmio_reset() for I8042_MMIO device
   pckbd: add mask qdev property to I8042_MMIO device
   pckbd: add size qdev property to I8042_MMIO device
   pckbd: implement i8042_mmio_realize() function
   pckbd: implement i8042_mmio_init() function
   pckbd: alter i8042_mm_init() to return a I8042_MMIO device
   pckbd: move mapping of I8042_MMIO registers to MIPS magnum machine
   pckbd: more vmstate_register() from i8042_mm_init() to
 i8042_mmio_realize()
   pckbd: move ps2_kbd_init() and ps2_mouse_init() to
 i8042_mmio_realize()
   ps2: make ps2_raise_irq() function static
   ps2: use ps2_raise_irq() instead of calling update_irq() directly
   ps2: introduce ps2_lower_irq() instead of calling update_irq()
 directly
   ps2: add gpio for output IRQ and optionally use it in ps2_raise_irq()
 and ps2_lower_irq()
   pckbd: replace irq_kbd and irq_mouse with qemu_irq array in KBDState
   pl050: switch over from update_irq() function to PS2 device gpio
   lasips2: QOMify LASIPS2State
   lasips2: move lasips2 QOM types from lasips2.c to lasips2.h
   lasips2: rename lasips2_init() to lasips2_initfn() and update it to
 return the LASIPS2 device
   lasips2: implement lasips2_init() function
   lasips2: move mapping of LASIPS2 registers to HPPA machine
   lasips2: move initialisation of PS2 ports from lasi_initfn() to
 lasi_init()
   lasips2: add base property
   lasips2: implement lasips2_realize()
   lasips2: use qdev gpio for output IRQ
   lasips2: switch over from update_irq() function to PS2 device gpio
   pckbd: switch I8042_MMIO device from update_irq() function to PS2
 device gpio
   pckbd: add i8042_reset() function to I8042 device
   pckbd: switch I8042 device from update_irq() function to PS2 device
 gpio
   ps2: remove update_irq() function and update_arg para

Re: [PATCH v5 14/45] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:

Now the indirection is not actually used, we can safely reduce it to
simple pointer.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/snapshot.c | 39 +--
  1 file changed, 17 insertions(+), 22 deletions(-)


Looks good, just wondering whether we should drop some of the "_ptr" 
suffixes now.



diff --git a/block/snapshot.c b/block/snapshot.c
index 02a880911f..4eb9258de6 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -151,34 +151,29 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState 
*bs,
  }
  
  /**

- * Return a pointer to the child BDS pointer to which we can fall
+ * Return a pointer to child of given BDS to which we can fall
   * back if the given BDS does not support snapshots.
   * Return NULL if there is no BDS to (safely) fall back to.
- *
- * We need to return an indirect pointer because bdrv_snapshot_goto()
- * has to modify the BdrvChild pointer.
   */
-static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
+static BdrvChild *bdrv_snapshot_fallback_ptr(BlockDriverState *bs)


The _ptr part was meant to point out that it returns an indirect 
pointer; maybe we should name it bdrv_snapshot_fallback_child() now?



  {
-BdrvChild **fallback;
-BdrvChild *child = bdrv_primary_child(bs);
+BdrvChild *fallback = bdrv_primary_child(bs);
+BdrvChild *child;
  
  /* We allow fallback only to primary child */

-if (!child) {
+if (!fallback) {
  return NULL;
  }
-fallback = (child == bs->file ? &bs->file : &bs->backing);
-assert(*fallback == child);
  
  /*

   * Check that there are no other children that would need to be
   * snapshotted.  If there are, it is not safe to fall back to
- * *fallback.
+ * fallback.
   */
  QLIST_FOREACH(child, &bs->children, next) {
  if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
 BDRV_CHILD_FILTERED) &&
-child != *fallback)
+child != fallback)
  {
  return NULL;
  }
@@ -189,8 +184,8 @@ static BdrvChild 
**bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
  
  static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)

  {
-BdrvChild **child_ptr = bdrv_snapshot_fallback_ptr(bs);


Just "child" is enough (and better) now, I think.


-return child_ptr ? (*child_ptr)->bs : NULL;
+BdrvChild *child_ptr = bdrv_snapshot_fallback_ptr(bs);
+return child_ptr ? child_ptr->bs : NULL;
  }
  
  int bdrv_can_snapshot(BlockDriverState *bs)

Re: [PATCH v2] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Jonathan Cameron via]

On Tue, 7 Jun 2022 12:06:12 +0100
Jonathan Cameron  wrote:

> On Tue, 7 Jun 2022 11:56:17 +0100
> Jonathan Cameron  wrote:
> 
> > Without being able to write these registers, no interleaving is possible.
> > More refined checks of HDM register state on commit to follow.
> > 
> > Signed-off-by: Jonathan Cameron   
> 
> +Cc Ben on current address.
> 
> Which reminds me - Ben, when you have time please send an update
> to MAINTAINERS to switch to your preferred email address going forwards.

Ignore this version as clearly broken due to some git command line
fumbling. v3 shortly.

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > v2: (Ben Widawsky)
> > - Correctly set a tighter write mask for the endpoint devices where this
> >   register has a different use.
> > 
> >  hw/cxl/cxl-component-utils.c | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index 7985c9bfca..40a0f752f2 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, 
> > uint32_t *write_msk)
> >  reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
> >  }
> >  
> > -static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
> > +static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> > +enum reg_type type)
> >  {
> >  int decoder_count = 1;
> >  int i;
> > @@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, 
> > uint32_t *write_msk)
> >  write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
> >  write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
> >  write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> > +if (type == CXL2_DEVICE) {
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > 0xf000;
> > +} else {
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > 0x;
> > +}
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 
> > 0x;
> >  }
> >  }
> >
> 

[PATCH v3] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Jonathan Cameron via]

Without being able to write these registers, no interleaving is possible.
More refined checks of HDM register state on commit to follow.

Signed-off-by: Jonathan Cameron 
---
v3: Actually pass the parameter to the call...
v2: (Ben Widawsky)
- Correctly set a tighter write mask for the endpoint devices where this
  register has a different use.
  
 hw/cxl/cxl-component-utils.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 7985c9bfca..2208284ee6 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, uint32_t 
*write_msk)
 reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
 }
 
-static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
+static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
+enum reg_type type)
 {
 int decoder_count = 1;
 int i;
@@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t 
*write_msk)
 write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
 write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
 write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
+if (type == CXL2_DEVICE) {
+write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
0xf000;
+} else {
+write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
0x;
+}
+write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 0x;
 }
 }
 
@@ -239,7 +246,7 @@ void cxl_component_register_init_common(uint32_t 
*reg_state, uint32_t *write_msk
 }
 
 init_cap_reg(HDM, 5, 1);
-hdm_init_common(reg_state, write_msk);
+hdm_init_common(reg_state, write_msk, type);
 
 if (caps < 5) {
 return;
-- 
2.32.0

Re: [PATCH v3] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Ben Widawsky]

On 22-06-07 17:07:47, Jonathan Cameron wrote:
> Without being able to write these registers, no interleaving is possible.
> More refined checks of HDM register state on commit to follow.
> 
> Signed-off-by: Jonathan Cameron 
> ---
> v3: Actually pass the parameter to the call...
> v2: (Ben Widawsky)
> - Correctly set a tighter write mask for the endpoint devices where this
>   register has a different use.
>   
>  hw/cxl/cxl-component-utils.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 7985c9bfca..2208284ee6 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, uint32_t 
> *write_msk)
>  reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
>  }
>  
> -static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
> +static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> +enum reg_type type)
>  {
>  int decoder_count = 1;
>  int i;
> @@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, 
> uint32_t *write_msk)
>  write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
>  write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
>  write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> +if (type == CXL2_DEVICE) {
> +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> 0xf000;
> +} else {
> +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> 0x;
> +}
> +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 0x;

Should it be (type == CXL2_DEVICE || type == CXL2_TYPE3_DEVICE) ?

Otherwise,
Reviewed-by: Ben Widawsky 

>  }
>  }
>  
> @@ -239,7 +246,7 @@ void cxl_component_register_init_common(uint32_t 
> *reg_state, uint32_t *write_msk
>  }
>  
>  init_cap_reg(HDM, 5, 1);
> -hdm_init_common(reg_state, write_msk);
> +hdm_init_common(reg_state, write_msk, type);
>  
>  if (caps < 5) {
>  return;
> -- 
> 2.32.0
> 

Re: [PATCH v3] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Jonathan Cameron via]

On Tue, 7 Jun 2022 09:19:28 -0700
Ben Widawsky  wrote:

> On 22-06-07 17:07:47, Jonathan Cameron wrote:
> > Without being able to write these registers, no interleaving is possible.
> > More refined checks of HDM register state on commit to follow.
> > 
> > Signed-off-by: Jonathan Cameron 
> > ---
> > v3: Actually pass the parameter to the call...
> > v2: (Ben Widawsky)
> > - Correctly set a tighter write mask for the endpoint devices where this
> >   register has a different use.
> >   
> >  hw/cxl/cxl-component-utils.c | 11 +--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index 7985c9bfca..2208284ee6 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, 
> > uint32_t *write_msk)
> >  reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
> >  }
> >  
> > -static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
> > +static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> > +enum reg_type type)
> >  {
> >  int decoder_count = 1;
> >  int i;
> > @@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, 
> > uint32_t *write_msk)
> >  write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
> >  write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
> >  write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> > +if (type == CXL2_DEVICE) {
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > 0xf000;
> > +} else {
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > 0x;
> > +}
> > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 
> > 0x;  
> 
> Should it be (type == CXL2_DEVICE || type == CXL2_TYPE3_DEVICE) ?

Good point, but also for consistency I think we need 
type == CXL2_LOGICAL_DEVICE as well.

We will only exercise the match to CXL2_TYPE3_DEVICE currently
as we don't have any emulation for MLDs (and hence LD) or type 1/2 devices
(CXL2_DEVICE).

I'll send a v4 out tomorrow.

> 
> Otherwise,
> Reviewed-by: Ben Widawsky 
> 
> >  }
> >  }
> >  
> > @@ -239,7 +246,7 @@ void cxl_component_register_init_common(uint32_t 
> > *reg_state, uint32_t *write_msk
> >  }
> >  
> >  init_cap_reg(HDM, 5, 1);
> > -hdm_init_common(reg_state, write_msk);
> > +hdm_init_common(reg_state, write_msk, type);
> >  
> >  if (caps < 5) {
> >  return;
> > -- 
> > 2.32.0
> >   

Re: [PATCH 1/3] target/mips: introduce generic Cavium Octeon CPU model

[Richard Henderson]

On 6/7/22 01:59, Pavel Dovgalyuk wrote:

+{
+/*
+ * A generic CPU providing MIPS64 Cavium Octeon features.
+ * PRid is taken from Octeon 68xx CPUs
+ * FIXME: Eventually this should be replaced by a real CPU model.
+ */


You should just add the real cpu model.  No one will ever address this FIXME 
otherwise.


r~

Re: [PATCH v3] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Ben Widawsky]

On 22-06-07 17:37:02, Jonathan Cameron wrote:
> On Tue, 7 Jun 2022 09:19:28 -0700
> Ben Widawsky  wrote:
> 
> > On 22-06-07 17:07:47, Jonathan Cameron wrote:
> > > Without being able to write these registers, no interleaving is possible.
> > > More refined checks of HDM register state on commit to follow.
> > > 
> > > Signed-off-by: Jonathan Cameron 
> > > ---
> > > v3: Actually pass the parameter to the call...
> > > v2: (Ben Widawsky)
> > > - Correctly set a tighter write mask for the endpoint devices where this
> > >   register has a different use.
> > >   
> > >  hw/cxl/cxl-component-utils.c | 11 +--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > index 7985c9bfca..2208284ee6 100644
> > > --- a/hw/cxl/cxl-component-utils.c
> > > +++ b/hw/cxl/cxl-component-utils.c
> > > @@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, 
> > > uint32_t *write_msk)
> > >  reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
> > >  }
> > >  
> > > -static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
> > > +static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> > > +enum reg_type type)
> > >  {
> > >  int decoder_count = 1;
> > >  int i;
> > > @@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, 
> > > uint32_t *write_msk)
> > >  write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf000;
> > >  write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0x;
> > >  write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> > > +if (type == CXL2_DEVICE) {
> > > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > > 0xf000;
> > > +} else {
> > > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > > 0x;
> > > +}
> > > +write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 
> > > 0x;  
> > 
> > Should it be (type == CXL2_DEVICE || type == CXL2_TYPE3_DEVICE) ?
> 
> Good point, but also for consistency I think we need 
> type == CXL2_LOGICAL_DEVICE as well.

I was looking at this and I am not sure, but I defer to you.

> 
> We will only exercise the match to CXL2_TYPE3_DEVICE currently
> as we don't have any emulation for MLDs (and hence LD) or type 1/2 devices
> (CXL2_DEVICE).
> 
> I'll send a v4 out tomorrow.
> 

Sounds good, feel free to keep the r-b tag.

> > 
> > Otherwise,
> > Reviewed-by: Ben Widawsky 
> > 
> > >  }
> > >  }
> > >  
> > > @@ -239,7 +246,7 @@ void cxl_component_register_init_common(uint32_t 
> > > *reg_state, uint32_t *write_msk
> > >  }
> > >  
> > >  init_cap_reg(HDM, 5, 1);
> > > -hdm_init_common(reg_state, write_msk);
> > > +hdm_init_common(reg_state, write_msk, type);
> > >  
> > >  if (caps < 5) {
> > >  return;
> > > -- 
> > > 2.32.0
> > >   
>