Re: [RFC v2 2/2] hw/riscv: Add server platform reference machine

[Heinrich Schuchardt]

On 3/22/24 08:14, Marcin Juszkiewicz wrote:

W dniu 22.03.2024 o 05:55, Alistair Francis pisze:

I see no mention of device trees in the spec, but I do see ACPI. Do we
really expect a server platform to use DTs?


This platform "kind of" follows sbsa-ref where we have very minimalistic 
device tree sharing information qemu->firmware.


libfdt is small, format is known and describes hardware. Firmware is 
free to make use of it in any way it wants.


On sbsa-ref we parse DT in TF-A (base firmware) and provide hardware 
information to higher level (edk2) via SMC mechanism. Then EDK2 creates 
ACPI tables and provide them to the Operating System.


We should ensure that only either an ACPI table or a device-tree 
description is passed to the OS and not both, e.g. when using


qemu-system-riscv64 -kernel vmlinux -M sbsa-ref

But that requirement is not machine specific.

Best regards

Heinrich



In physical system some parts of information provided in DT would be 
read by firmware from onboard Embedded Controller chip.



These functions should be shared with the virt machine if we really do
want DTs, but I'm not convinced we do

Re: [RISC-V][tech-server-platform] [RISC-V][tech-server-soc] [RFC 2/2] target/riscv: Add server platform reference cpu

[Heinrich Schuchardt]

On 07.03.24 08:36, Wu, Fei2 wrote:

On 3/6/2024 9:26 PM, Wu, Fei wrote:

On 3/5/2024 1:58 PM, Wu, Fei wrote:

On 3/5/2024 3:43 AM, Daniel Henrique Barboza wrote:



On 3/4/24 07:25, Fei Wu wrote:

The harts requirements of RISC-V server platform [1] require RVA23 ISA
profile support, plus Sv48, Svadu, H, Sscofmpf etc. This patch provides
a virt CPU type (rvsp-ref) as compliant as possible.

[1]
https://github.com/riscv-non-isa/riscv-server-platform/blob/main/server_platform_requirements.adoc

Signed-off-by: Fei Wu 
--->   hw/riscv/server_platform_ref.c |  6 +++-
   target/riscv/cpu-qom.h |  1 +
   target/riscv/cpu.c | 62 ++
   3 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/server_platform_ref.c
b/hw/riscv/server_platform_ref.c
index ae90c4b27a..52ec607cee 100644
--- a/hw/riscv/server_platform_ref.c
+++ b/hw/riscv/server_platform_ref.c
@@ -1205,11 +1205,15 @@ static void
rvsp_ref_machine_class_init(ObjectClass *oc, void *data)
   {
   char str[128];
   MachineClass *mc = MACHINE_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+    TYPE_RISCV_CPU_RVSP_REF,
+    };
     mc->desc = "RISC-V Server SoC Reference board";
   mc->init = rvsp_ref_machine_init;
   mc->max_cpus = RVSP_CPUS_MAX;
-    mc->default_cpu_type = TYPE_RISCV_CPU_BASE;
+    mc->default_cpu_type = TYPE_RISCV_CPU_RVSP_REF;
+    mc->valid_cpu_types = valid_cpu_types;


I suggest introducing this patch first, then the new machine type that
will use it as a default
CPU. The reason is to facilitate future bisects. If we introduce the
board first, a future bisect
might hit the previous patch, the board will be run using RV64 instead
of the correct CPU, and
we'll have different results because of it.


Good suggestion.


   mc->pci_allow_0_address = true;
   mc->default_nic = "e1000e";
   mc->possible_cpu_arch_ids = riscv_numa_possible_cpu_arch_ids;
diff --git a/target/riscv/cpu-qom.h b/target/riscv/cpu-qom.h
index 3670cfe6d9..adb934d19e 100644
--- a/target/riscv/cpu-qom.h
+++ b/target/riscv/cpu-qom.h
@@ -49,6 +49,7 @@
   #define TYPE_RISCV_CPU_SIFIVE_U54
RISCV_CPU_TYPE_NAME("sifive-u54")
   #define TYPE_RISCV_CPU_THEAD_C906
RISCV_CPU_TYPE_NAME("thead-c906")
   #define TYPE_RISCV_CPU_VEYRON_V1
RISCV_CPU_TYPE_NAME("veyron-v1")
+#define TYPE_RISCV_CPU_RVSP_REF RISCV_CPU_TYPE_NAME("rvsp-ref")
   #define TYPE_RISCV_CPU_HOST RISCV_CPU_TYPE_NAME("host")
     OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5ff0192c52..bc91be702b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -2282,6 +2282,67 @@ static void rva22s64_profile_cpu_init(Object *obj)
     RVA22S64.enabled = true;
   }
+
+static void rv64_rvsp_ref_cpu_init(Object *obj)
+{
+    CPURISCVState *env = _CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+
+    riscv_cpu_set_misa_ext(env, RVG | RVC | RVS | RVU | RVH | RVV);
+
+    /* FIXME: change to 1.13 */
+    env->priv_ver = PRIV_VERSION_1_12_0;
+
+    /* RVA22U64 */
+    cpu->cfg.mmu = true;
+    cpu->cfg.ext_zifencei = true;
+    cpu->cfg.ext_zicsr = true;
+    cpu->cfg.ext_zicntr = true;
+    cpu->cfg.ext_zihpm = true;
+    cpu->cfg.ext_zihintpause = true;
+    cpu->cfg.ext_zba = true;
+    cpu->cfg.ext_zbb = true;
+    cpu->cfg.ext_zbs = true;
+    cpu->cfg.zic64b = true;
+    cpu->cfg.ext_zicbom = true;
+    cpu->cfg.ext_zicbop = true;
+    cpu->cfg.ext_zicboz = true;
+    cpu->cfg.cbom_blocksize = 64;
+    cpu->cfg.cbop_blocksize = 64;
+    cpu->cfg.cboz_blocksize = 64;
+    cpu->cfg.ext_zfhmin = true;
+    cpu->cfg.ext_zkt = true;


You can change this whole block with:

RVA22U64.enabled = true;


riscv_cpu_add_profiles() will check if we have a profile enabled and, if
that's the
case, we'll enable all its extensions in the CPU.

In the near future, when we implement a proper RVA23 support, we'll be
able to just do
a single RVA23S64.enabled = true in this cpu_init(). But for now we can
at least declare
RVA22U64 (perhaps RVA22S64) support for this CPU.



Hi Daniel,

I'm not sure if it's a regression or the usage has been changed. I'm not
able to use '-cpu rva22s64' on latest qemu (db596ae190).


I did a quick git bisect and found that commit d06f28db6 "target/riscv:
move 'mmu' to riscv_cpu_properties[]" disabled mmu by default, so that
an explicit mmu option should be added to qemu command line like '-cpu
rva22s64,mmu=true', I think rva22s64 should enable it by default.

Thanks,
Fei.


It is nice that the MMU can be disabled. But is there any reason why the 
MMU should be disabled by default on the virt machine (which typically 
is used to run an operating system)?


Can we add mmu=true as default to the rv64 CPU?

Best regards

Heinrich

Re: [RISC-V][tech-server-platform] [RFC 0/2] Add RISC-V Server Platform Reference Board

[Heinrich Schuchardt]

On 04.03.24 11:25, Wu, Fei2 wrote:

The RISC-V Server Platform specification[1] defines a standardized set
of hardware and software capabilities, that portable system software,
such as OS and hypervisors can rely on being present in a RISC-V server
platform. This patchset provides a RISC-V Server Platform (RVSP)
reference implementation on qemu which is in compliance with the spec
as faithful as possible.

The reference board can be running with tag edk2-stable202308 in
upstream edk2 repo[2].

The qemu command line used:

$QEMU -nographic -m 4G -smp 2 \
 -machine rvsp-ref,pflash0=pflash0,pflash1=pflash1 \
 -blockdev 
node-name=pflash0,driver=file,read-only=on,filename=RISCV_VIRT_CODE.fd \
 -blockdev node-name=pflash1,driver=file,filename=RISCV_VIRT_VARS.fd \
 -bios fw_dynamic.bin \
 -drive file=$BRS_IMG,if=ide,format=raw

Since there is no ACPI table generated in this new machine type, a
corresponding EDK-II platform (WIP) is needed to provide related ACPI
tables.


The need for having a platform separate to virt for compliance testing 
was discussed in RISE in RVI meetings. Thanks for driving this.


Will the EDK II platform also generate the SMBIOS table or shall this be 
handled in QEMU?


Will further work in edk2-platforms be needed to provide full compliance 
with the the server platform specification?


Best regards

Heinrich



For testing purposes only, we used a workaround to generate the ACPI
tables in Qemu with a dedicated downstream patch.

[1] https://github.com/riscv-non-isa/riscv-server-platform
[2] https://github.com/tianocore/edk2.git

Fei Wu (2):
   hw/riscv: Add server platform reference machine
   target/riscv: Add server platform reference cpu

  configs/devices/riscv64-softmmu/default.mak |1 +
  hw/riscv/Kconfig|   13 +
  hw/riscv/meson.build|1 +
  hw/riscv/server_platform_ref.c  | 1248 +++
  target/riscv/cpu-qom.h  |1 +
  target/riscv/cpu.c  |   62 +
  6 files changed, 1326 insertions(+)
  create mode 100644 hw/riscv/server_platform_ref.c

Re: [PATCH, v2] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

On 29.02.24 13:34, Peter Maydell wrote:

On Thu, 29 Feb 2024 at 11:17, Heinrich Schuchardt
 wrote:

But yes, I'm not surprised that CXL runs into this. Heinrich,
are you doing CXL testing, or is this some other workload?


I am running the UEFI Self-Certification Tests (SCT) on EDK 2 using:

qemu-system-riscv64 \
-M virt,acpi=off -accel tcg -m 4096 \
-serial mon:stdio \
-device virtio-gpu-pci \
-device qemu-xhci \
-device usb-kbd \
-drive
if=pflash,format=raw,unit=0,file=RISCV_VIRT_CODE.fd,readonly=on \
-drive if=pflash,format=raw,unit=1,file=RISCV_VIRT_VARS.fd \
-drive file=sct.img,format=raw,if=virtio \
-device virtio-net-device,netdev=net0 \
-netdev user,id=net0

This does not invoke any CXL related stuff.


Hmm, that doesn't seem like it ought to be running into this.
What underlying memory region is the guest trying to do
the virtio queue access to?

-- PMM


The error occurs while the SCT is executing function 
BBTestReadBlocksConformanceAutoTest 
(https://github.com/tianocore/edk2-test/blob/cabb98d44be94e7547605435a0be7c4946d10f8b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/BlockIo/BlackBoxTest/BlockIoBBTestConformance.c#L45)


This code is accessing the drive defined as
-drive file=sct.img,format=raw,if=virtio .

In the conformance test correct error handling for invalid parameters of 
the UEFI block IO protocol is tested. This includes calling the UEFI API to


* read with incorrectly aligned buffers
* read with invalid LBA parameter
* read with buffer size not being a multiple of the sector size

In all these cases the UEFI API implemented by EDK II is expected to 
return an error.


Best regards

Heinrich

Re: [PATCH, v2] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

On 29.02.24 12:11, Peter Maydell wrote:

On Thu, 29 Feb 2024 at 10:59, Jonathan Cameron
 wrote:


On Thu, 29 Feb 2024 09:38:29 +
Peter Maydell  wrote:


On Wed, 28 Feb 2024 at 19:07, Heinrich Schuchardt
 wrote:


On 28.02.24 19:39, Peter Maydell wrote:

The limitation to a page dates back to commit 6d16c2f88f2a in 2009,
which was the first implementation of this function. I don't think
there's a particular reason for that value beyond that it was
probably a convenient value that was assumed to be likely "big enough".

I think the idea with this bounce-buffer has always been that this
isn't really a code path we expected to end up in very often --
it's supposed to be for when devices are doing DMA, which they
will typically be doing to memory (backed by host RAM), not
devices (backed by MMIO and needing a bounce buffer). So the
whole mechanism is a bit "last fallback to stop things breaking
entirely".

The address_space_map() API says that it's allowed to return
a subset of the range you ask for, so if the virtio code doesn't
cope with the minimum being set to TARGET_PAGE_SIZE then either
we need to fix that virtio code or we need to change the API
of this function. (But I think you will also get a reduced
range if you try to use it across a boundary between normal
host-memory-backed RAM and a device MemoryRegion.)


If we allow a bounce buffer only to be used once (via the in_use flag),
why do we allow only a single bounce buffer?

Could address_space_map() allocate a new bounce buffer on every call and
address_space_unmap() deallocate it?

Isn't the design with a single bounce buffer bound to fail with a
multi-threaded client as collision can be expected?


Yeah, I don't suppose multi-threaded was particularly expected.
Again, this is really a "handle the case where the guest does
something silly" setup, which is why only one bounce buffer.

Why is your guest ending up in the bounce-buffer path?


Happens for me with emulated CXL memory.


Can we put that in the "something silly" bucket? :-)
But yes, I'm not surprised that CXL runs into this. Heinrich,
are you doing CXL testing, or is this some other workload?


I am running the UEFI Self-Certification Tests (SCT) on EDK 2 using:

qemu-system-riscv64 \
  -M virt,acpi=off -accel tcg -m 4096 \
  -serial mon:stdio \
  -device virtio-gpu-pci \
  -device qemu-xhci \
  -device usb-kbd \
  -drive 
if=pflash,format=raw,unit=0,file=RISCV_VIRT_CODE.fd,readonly=on \

  -drive if=pflash,format=raw,unit=1,file=RISCV_VIRT_VARS.fd \
  -drive file=sct.img,format=raw,if=virtio \
  -device virtio-net-device,netdev=net0 \
  -netdev user,id=net0

This does not invoke any CXL related stuff.

Best regards

Heinrich




I think the case I saw
was split descriptors in virtio via address space caches
https://elixir.bootlin.com/qemu/latest/source/hw/virtio/virtio.c#L4043

One bounce buffer is in use for the outer loop and another for the descriptors
it is pointing to.


Mmm. The other assumption made in the design of the address_space_map()
API I think was that it was unlikely that a device would be trying
to do two DMA operations simultaneously. This is clearly not
true in practice. We definitely need to fix one end or other of
this API.

(I'm not sure why the bounce-buffer limit ought to be per-AddressSpace:
is that just done in Matthias' series so that we can attach an
x-thingy property to the individual PCI device?)

-- PMM

Re: [PATCH v7 2/5] softmmu: Support concurrent bounce buffers

[Heinrich Schuchardt]

On 12.02.24 09:06, Mattias Nissler wrote:

When DMA memory can't be directly accessed, as is the case when
running the device model in a separate process without shareable DMA
file descriptors, bounce buffering is used.

It is not uncommon for device models to request mapping of several DMA
regions at the same time. Examples include:
  * net devices, e.g. when transmitting a packet that is split across
several TX descriptors (observed with igb)
  * USB host controllers, when handling a packet with multiple data TRBs
(observed with xhci)

Previously, qemu only provided a single bounce buffer per AddressSpace
and would fail DMA map requests while the buffer was already in use. In
turn, this would cause DMA failures that ultimately manifest as hardware
errors from the guest perspective.

This change allocates DMA bounce buffers dynamically instead of
supporting only a single buffer. Thus, multiple DMA mappings work
correctly also when RAM can't be mmap()-ed.

The total bounce buffer allocation size is limited individually for each
AddressSpace. The default limit is 4096 bytes, matching the previous
maximum buffer size. A new x-max-bounce-buffer-size parameter is
provided to configure the limit for PCI devices.

Signed-off-by: Mattias Nissler 
---
  hw/pci/pci.c|  8 
  include/exec/memory.h   | 14 +++
  include/hw/pci/pci_device.h |  3 ++
  system/memory.c |  5 ++-
  system/physmem.c| 80 +
  5 files changed, 74 insertions(+), 36 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6496d027ca..036b3ff822 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -85,6 +85,8 @@ static Property pci_props[] = {
  QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
  DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present,
  QEMU_PCIE_ARI_NEXTFN_1_BITNR, false),
+DEFINE_PROP_SIZE("x-max-bounce-buffer-size", PCIDevice,
+ max_bounce_buffer_size, DEFAULT_MAX_BOUNCE_BUFFER_SIZE),
  DEFINE_PROP_END_OF_LIST()
  };
  
@@ -1203,6 +1205,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,

 "bus master container", UINT64_MAX);
  address_space_init(_dev->bus_master_as,
 _dev->bus_master_container_region, pci_dev->name);
+pci_dev->bus_master_as.max_bounce_buffer_size =
+pci_dev->max_bounce_buffer_size;
  
  if (phase_check(PHASE_MACHINE_READY)) {

  pci_init_bus_master(pci_dev);
@@ -2632,6 +2636,10 @@ static void pci_device_class_init(ObjectClass *klass, 
void *data)
  k->unrealize = pci_qdev_unrealize;
  k->bus_type = TYPE_PCI_BUS;
  device_class_set_props(k, pci_props);
+object_class_property_set_description(
+klass, "x-max-bounce-buffer-size",
+"Maximum buffer size allocated for bounce buffers used for mapped "
+"access to indirect DMA memory");
  }
  
  static void pci_device_class_base_init(ObjectClass *klass, void *data)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6995a443d3..e7bc4717ea 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -,13 +,7 @@ typedef struct AddressSpaceMapClient {
  QLIST_ENTRY(AddressSpaceMapClient) link;
  } AddressSpaceMapClient;
  
-typedef struct {

-MemoryRegion *mr;
-void *buffer;
-hwaddr addr;
-hwaddr len;
-bool in_use;
-} BounceBuffer;
+#define DEFAULT_MAX_BOUNCE_BUFFER_SIZE (4096)
  
  /**

   * struct AddressSpace: describes a mapping of addresses to #MemoryRegion 
objects
@@ -1137,8 +1131,10 @@ struct AddressSpace {
  QTAILQ_HEAD(, MemoryListener) listeners;
  QTAILQ_ENTRY(AddressSpace) address_spaces_link;
  
-/* Bounce buffer to use for this address space. */

-BounceBuffer bounce;
+/* Maximum DMA bounce buffer size used for indirect memory map requests */
+uint64_t max_bounce_buffer_size;
+/* Total size of bounce buffers currently allocated, atomically accessed */
+uint64_t bounce_buffer_size;
  /* List of callbacks to invoke when buffers free up */
  QemuMutex map_client_list_lock;
  QLIST_HEAD(, AddressSpaceMapClient) map_client_list;
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index d3dd0f64b2..f4027c5379 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -160,6 +160,9 @@ struct PCIDevice {
  /* ID of standby device in net_failover pair */
  char *failover_pair_id;
  uint32_t acpi_index;
+
+/* Maximum DMA bounce buffer size used for indirect memory map requests */
+uint64_t max_bounce_buffer_size;
  };
  
  static inline int pci_intx(PCIDevice *pci_dev)

diff --git a/system/memory.c b/system/memory.c
index ad0caef1b8..1cf89654a1 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3133,7 +3133,8 @@ void address_space_init(AddressSpace *as, MemoryRegion 
*root, const char *name)
  as->ioeventfds = NULL;
  

Re: [PATCH, v2] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

On 29.02.24 02:11, Peter Xu wrote:

On Wed, Feb 28, 2024 at 08:07:47PM +0100, Heinrich Schuchardt wrote:

On 28.02.24 19:39, Peter Maydell wrote:

On Wed, 28 Feb 2024 at 18:28, Heinrich Schuchardt
 wrote:


On 28.02.24 16:06, Philippe Mathieu-Daudé wrote:

Hi Heinrich,

On 28/2/24 13:59, Heinrich Schuchardt wrote:

virtqueue_map_desc() is called with values of sz exceeding that may
exceed
TARGET_PAGE_SIZE. sz = 0x2800 has been observed.


Pure (and can also be stupid) question: why virtqueue_map_desc() would map
to !direct mem?  Shouldn't those buffers normally allocated from guest RAM?



We only support a single bounce buffer. We have to avoid
virtqueue_map_desc() calling address_space_map() multiple times.
Otherwise
we see an error

   qemu: virtio: bogus descriptor or out of resources

Increase the minimum size of the bounce buffer to 0x1 which matches
the largest value of TARGET_PAGE_SIZE for all architectures.

Signed-off-by: Heinrich Schuchardt 
---
v2:
  remove unrelated change
---
system/physmem.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index e3ebc19eef..3c82da1c86 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as,
*plen = 0;
return NULL;
}
-/* Avoid unbounded allocations */
-l = MIN(l, TARGET_PAGE_SIZE);
+/*
+ * There is only one bounce buffer. The largest occuring
value of
+ * parameter sz of virtqueue_map_desc() must fit into the bounce
+ * buffer.
+ */
+l = MIN(l, 0x1);


Please define this magic value. Maybe ANY_TARGET_PAGE_SIZE or
TARGETS_BIGGEST_PAGE_SIZE?

Then along:
 QEMU_BUILD_BUG_ON(TARGET_PAGE_SIZE <= TARGETS_BIGGEST_PAGE_SIZE);


Thank you Philippe for reviewing.

TARGETS_BIGGEST_PAGE_SIZE does not fit as the value is not driven by the
page size.
How about MIN_BOUNCE_BUFFER_SIZE?
Is include/exec/memory.h the right include for the constant?

I don't think that TARGET_PAGE_SIZE has any relevance for setting the
bounce buffer size. I only mentioned it to say that we are not
decreasing the value on any existing architecture.

I don't know why TARGET_PAGE_SIZE ever got into this piece of code.
e3127ae0cdcd ("exec: reorganize address_space_map") does not provide a
reason for this choice. Maybe Paolo remembers.


The limitation to a page dates back to commit 6d16c2f88f2a in 2009,
which was the first implementation of this function. I don't think
there's a particular reason for that value beyond that it was
probably a convenient value that was assumed to be likely "big enough".

I think the idea with this bounce-buffer has always been that this
isn't really a code path we expected to end up in very often --
it's supposed to be for when devices are doing DMA, which they
will typically be doing to memory (backed by host RAM), not
devices (backed by MMIO and needing a bounce buffer). So the
whole mechanism is a bit "last fallback to stop things breaking
entirely".

The address_space_map() API says that it's allowed to return
a subset of the range you ask for, so if the virtio code doesn't
cope with the minimum being set to TARGET_PAGE_SIZE then either
we need to fix that virtio code or we need to change the API
of this function. (But I think you will also get a reduced
range if you try to use it across a boundary between normal
host-memory-backed RAM and a device MemoryRegion.)


If we allow a bounce buffer only to be used once (via the in_use flag), why
do we allow only a single bounce buffer?

Could address_space_map() allocate a new bounce buffer on every call and
address_space_unmap() deallocate it?

Isn't the design with a single bounce buffer bound to fail with a
multi-threaded client as collision can be expected?


See:

https://lore.kernel.org/r/20240212080617.2559498-1-mniss...@rivosinc.com

For some reason that series didn't land, but it seems to be helpful in this
case too if e.g. there can be multiple of such devices.

Thanks,



Hello Peter Xu,

thanks for pointing to your series. What I like about it is that it 
removes the limit of a single bounce buffer per AddressSpace.


Unfortunately it does not solve my problem. You limit the sum of all of 
the allocations for a single AddressSpcace to 
DEFAULT_MAX_BOUNCE_BUFFER_SIZE = 4096 which is too small for my use case.


Why do we need a limit?
Why is it so tiny?

Best regards

Heinrich

Re: [PATCH, v2] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

On 28.02.24 19:39, Peter Maydell wrote:

On Wed, 28 Feb 2024 at 18:28, Heinrich Schuchardt
 wrote:


On 28.02.24 16:06, Philippe Mathieu-Daudé wrote:

Hi Heinrich,

On 28/2/24 13:59, Heinrich Schuchardt wrote:

virtqueue_map_desc() is called with values of sz exceeding that may
exceed
TARGET_PAGE_SIZE. sz = 0x2800 has been observed.

We only support a single bounce buffer. We have to avoid
virtqueue_map_desc() calling address_space_map() multiple times.
Otherwise
we see an error

  qemu: virtio: bogus descriptor or out of resources

Increase the minimum size of the bounce buffer to 0x1 which matches
the largest value of TARGET_PAGE_SIZE for all architectures.

Signed-off-by: Heinrich Schuchardt 
---
v2:
 remove unrelated change
---
   system/physmem.c | 8 ++--
   1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index e3ebc19eef..3c82da1c86 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as,
   *plen = 0;
   return NULL;
   }
-/* Avoid unbounded allocations */
-l = MIN(l, TARGET_PAGE_SIZE);
+/*
+ * There is only one bounce buffer. The largest occuring
value of
+ * parameter sz of virtqueue_map_desc() must fit into the bounce
+ * buffer.
+ */
+l = MIN(l, 0x1);


Please define this magic value. Maybe ANY_TARGET_PAGE_SIZE or
TARGETS_BIGGEST_PAGE_SIZE?

Then along:
QEMU_BUILD_BUG_ON(TARGET_PAGE_SIZE <= TARGETS_BIGGEST_PAGE_SIZE);


Thank you Philippe for reviewing.

TARGETS_BIGGEST_PAGE_SIZE does not fit as the value is not driven by the
page size.
How about MIN_BOUNCE_BUFFER_SIZE?
Is include/exec/memory.h the right include for the constant?

I don't think that TARGET_PAGE_SIZE has any relevance for setting the
bounce buffer size. I only mentioned it to say that we are not
decreasing the value on any existing architecture.

I don't know why TARGET_PAGE_SIZE ever got into this piece of code.
e3127ae0cdcd ("exec: reorganize address_space_map") does not provide a
reason for this choice. Maybe Paolo remembers.


The limitation to a page dates back to commit 6d16c2f88f2a in 2009,
which was the first implementation of this function. I don't think
there's a particular reason for that value beyond that it was
probably a convenient value that was assumed to be likely "big enough".

I think the idea with this bounce-buffer has always been that this
isn't really a code path we expected to end up in very often --
it's supposed to be for when devices are doing DMA, which they
will typically be doing to memory (backed by host RAM), not
devices (backed by MMIO and needing a bounce buffer). So the
whole mechanism is a bit "last fallback to stop things breaking
entirely".

The address_space_map() API says that it's allowed to return
a subset of the range you ask for, so if the virtio code doesn't
cope with the minimum being set to TARGET_PAGE_SIZE then either
we need to fix that virtio code or we need to change the API
of this function. (But I think you will also get a reduced
range if you try to use it across a boundary between normal
host-memory-backed RAM and a device MemoryRegion.)


If we allow a bounce buffer only to be used once (via the in_use flag), 
why do we allow only a single bounce buffer?


Could address_space_map() allocate a new bounce buffer on every call and 
address_space_unmap() deallocate it?


Isn't the design with a single bounce buffer bound to fail with a 
multi-threaded client as collision can be expected?


Best regards

Heinrich

Re: [PATCH, v2] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

On 28.02.24 16:06, Philippe Mathieu-Daudé wrote:

Hi Heinrich,

On 28/2/24 13:59, Heinrich Schuchardt wrote:
virtqueue_map_desc() is called with values of sz exceeding that may 
exceed

TARGET_PAGE_SIZE. sz = 0x2800 has been observed.

We only support a single bounce buffer. We have to avoid
virtqueue_map_desc() calling address_space_map() multiple times. 
Otherwise

we see an error

 qemu: virtio: bogus descriptor or out of resources

Increase the minimum size of the bounce buffer to 0x1 which matches
the largest value of TARGET_PAGE_SIZE for all architectures.

Signed-off-by: Heinrich Schuchardt 
---
v2:
remove unrelated change
---
  system/physmem.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index e3ebc19eef..3c82da1c86 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as,
  *plen = 0;
  return NULL;
  }
-    /* Avoid unbounded allocations */
-    l = MIN(l, TARGET_PAGE_SIZE);
+    /*
+ * There is only one bounce buffer. The largest occuring 
value of

+ * parameter sz of virtqueue_map_desc() must fit into the bounce
+ * buffer.
+ */
+    l = MIN(l, 0x1);


Please define this magic value. Maybe ANY_TARGET_PAGE_SIZE or
TARGETS_BIGGEST_PAGE_SIZE?

Then along:
   QEMU_BUILD_BUG_ON(TARGET_PAGE_SIZE <= TARGETS_BIGGEST_PAGE_SIZE);


Thank you Philippe for reviewing.

TARGETS_BIGGEST_PAGE_SIZE does not fit as the value is not driven by the 
page size.

How about MIN_BOUNCE_BUFFER_SIZE?
Is include/exec/memory.h the right include for the constant?

I don't think that TARGET_PAGE_SIZE has any relevance for setting the 
bounce buffer size. I only mentioned it to say that we are not 
decreasing the value on any existing architecture.


I don't know why TARGET_PAGE_SIZE ever got into this piece of code. 
e3127ae0cdcd ("exec: reorganize address_space_map") does not provide a 
reason for this choice. Maybe Paolo remembers.


Best regards

Heinrich




  bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
  bounce.addr = addr;
  bounce.len = l;

Re: [PATCH 1/1] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

On 28.02.24 13:46, Heinrich Schuchardt wrote:

virtqueue_map_desc() is called with values of sz exceeding that may exceed
TARGET_PAGE_SIZE. sz = 0x2800 has been observed.

We only support a single bounce buffer. We have to avoid
virtqueue_map_desc() calling address_space_map() multiple times. Otherwise
we see an error

 qemu: virtio: bogus descriptor or out of resources

Increase the minimum size of the bounce buffer to 0x1 which matches
the largest value of TARGET_PAGE_SIZE for all architectures.

Signed-off-by: Heinrich Schuchardt 
---
  roms/edk2 | 2 +-
  roms/seabios-hppa | 2 +-
  system/physmem.c  | 8 ++--
  3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/roms/edk2 b/roms/edk2
index edc6681206..b8a3eec88c 16
--- a/roms/edk2
+++ b/roms/edk2
@@ -1 +1 @@
-Subproject commit edc6681206c1a8791981a2f911d2fb8b3d2f5768
+Subproject commit b8a3eec88cc74bbfe7fb389d026cc7d1d8a989c8


I have resent v2 of the patch w/o this unrelated change.


diff --git a/roms/seabios-hppa b/roms/seabios-hppa
index 03774edaad..e4eac85880 16
--- a/roms/seabios-hppa
+++ b/roms/seabios-hppa
@@ -1 +1 @@
-Subproject commit 03774edaad3bfae090ac96ca5450353c641637d1
+Subproject commit e4eac85880e8677f96d8b9e94de9f2eec9c0751f
diff --git a/system/physmem.c b/system/physmem.c
index e3ebc19eef..3c82da1c86 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as,
  *plen = 0;
  return NULL;
  }
-/* Avoid unbounded allocations */
-l = MIN(l, TARGET_PAGE_SIZE);
+/*
+ * There is only one bounce buffer. The largest occuring value of
+ * parameter sz of virtqueue_map_desc() must fit into the bounce
+ * buffer.
+ */
+l = MIN(l, 0x1);
  bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
  bounce.addr = addr;
  bounce.len = l;

[PATCH, v2] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

virtqueue_map_desc() is called with values of sz exceeding that may exceed
TARGET_PAGE_SIZE. sz = 0x2800 has been observed.

We only support a single bounce buffer. We have to avoid
virtqueue_map_desc() calling address_space_map() multiple times. Otherwise
we see an error

qemu: virtio: bogus descriptor or out of resources

Increase the minimum size of the bounce buffer to 0x1 which matches
the largest value of TARGET_PAGE_SIZE for all architectures.

Signed-off-by: Heinrich Schuchardt 
---
v2:
remove unrelated change
---
 system/physmem.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index e3ebc19eef..3c82da1c86 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as,
 *plen = 0;
 return NULL;
 }
-/* Avoid unbounded allocations */
-l = MIN(l, TARGET_PAGE_SIZE);
+/*
+ * There is only one bounce buffer. The largest occuring value of
+ * parameter sz of virtqueue_map_desc() must fit into the bounce
+ * buffer.
+ */
+l = MIN(l, 0x1);
 bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
 bounce.addr = addr;
 bounce.len = l;
-- 
2.43.0

[PATCH 1/1] physmem: avoid bounce buffer too small

[Heinrich Schuchardt]

virtqueue_map_desc() is called with values of sz exceeding that may exceed
TARGET_PAGE_SIZE. sz = 0x2800 has been observed.

We only support a single bounce buffer. We have to avoid
virtqueue_map_desc() calling address_space_map() multiple times. Otherwise
we see an error

qemu: virtio: bogus descriptor or out of resources

Increase the minimum size of the bounce buffer to 0x1 which matches
the largest value of TARGET_PAGE_SIZE for all architectures.

Signed-off-by: Heinrich Schuchardt 
---
 roms/edk2 | 2 +-
 roms/seabios-hppa | 2 +-
 system/physmem.c  | 8 ++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/roms/edk2 b/roms/edk2
index edc6681206..b8a3eec88c 16
--- a/roms/edk2
+++ b/roms/edk2
@@ -1 +1 @@
-Subproject commit edc6681206c1a8791981a2f911d2fb8b3d2f5768
+Subproject commit b8a3eec88cc74bbfe7fb389d026cc7d1d8a989c8
diff --git a/roms/seabios-hppa b/roms/seabios-hppa
index 03774edaad..e4eac85880 16
--- a/roms/seabios-hppa
+++ b/roms/seabios-hppa
@@ -1 +1 @@
-Subproject commit 03774edaad3bfae090ac96ca5450353c641637d1
+Subproject commit e4eac85880e8677f96d8b9e94de9f2eec9c0751f
diff --git a/system/physmem.c b/system/physmem.c
index e3ebc19eef..3c82da1c86 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as,
 *plen = 0;
 return NULL;
 }
-/* Avoid unbounded allocations */
-l = MIN(l, TARGET_PAGE_SIZE);
+/*
+ * There is only one bounce buffer. The largest occuring value of
+ * parameter sz of virtqueue_map_desc() must fit into the bounce
+ * buffer.
+ */
+l = MIN(l, 0x1);
 bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
 bounce.addr = addr;
 bounce.len = l;
-- 
2.43.0

[PATCH v4 3/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.
Set the default processor family in the type 4 table.

The implementation is based on the corresponding ARM and Loongson code.

With the patch the following firmware tables are provided:

etc/smbios/smbios-anchor
etc/smbios/smbios-tables

Signed-off-by: Heinrich Schuchardt 
---
v4:
remove a superfluous #ifdef
v3:
use misa_mxl_max to determine bitness
v2:
set processor family
---
 hw/riscv/Kconfig |  1 +
 hw/riscv/virt.c  | 42 ++
 2 files changed, 43 insertions(+)

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index a50717be87..5d644eb7b1 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -41,6 +41,7 @@ config RISCV_VIRT
 select RISCV_IMSIC
 select SIFIVE_PLIC
 select SIFIVE_TEST
+select SMBIOS
 select VIRTIO_MMIO
 select FW_CFG_DMA
 select PLATFORM_BUS
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index f9fd1341fc..1b333af4f0 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -36,6 +36,7 @@
 #include "hw/riscv/boot.h"
 #include "hw/riscv/numa.h"
 #include "kvm/kvm_riscv.h"
+#include "hw/firmware/smbios.h"
 #include "hw/intc/riscv_aclint.h"
 #include "hw/intc/riscv_aplic.h"
 #include "hw/intc/sifive_plic.h"
@@ -1263,6 +1264,45 @@ static void create_platform_bus(RISCVVirtState *s, 
DeviceState *irqchip)
 sysbus_mmio_get_region(sysbus, 0));
 }
 
+static void virt_build_smbios(RISCVVirtState *s)
+{
+MachineClass *mc = MACHINE_GET_CLASS(s);
+MachineState *ms = MACHINE(s);
+uint8_t *smbios_tables, *smbios_anchor;
+size_t smbios_tables_len, smbios_anchor_len;
+struct smbios_phys_mem_area mem_array;
+const char *product = "QEMU Virtual Machine";
+
+if (kvm_enabled()) {
+product = "KVM Virtual Machine";
+}
+
+smbios_set_defaults("QEMU", product, mc->name, false,
+true, SMBIOS_ENTRY_POINT_TYPE_64);
+
+if (riscv_is_32bit(>soc[0])) {
+smbios_set_default_processor_family(0x200);
+} else {
+smbios_set_default_processor_family(0x201);
+}
+
+/* build the array of physical mem area from base_memmap */
+mem_array.address = s->memmap[VIRT_DRAM].base;
+mem_array.length = ms->ram_size;
+
+smbios_get_tables(ms, _array, 1,
+  _tables, _tables_len,
+  _anchor, _anchor_len,
+  _fatal);
+
+if (smbios_anchor) {
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-tables",
+smbios_tables, smbios_tables_len);
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-anchor",
+smbios_anchor, smbios_anchor_len);
+}
+}
+
 static void virt_machine_done(Notifier *notifier, void *data)
 {
 RISCVVirtState *s = container_of(notifier, RISCVVirtState,
@@ -1351,6 +1393,8 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 riscv_setup_direct_kernel(kernel_entry, fdt_load_addr);
 }
 
+virt_build_smbios(s);
+
 if (virt_is_acpi_enabled(s)) {
 virt_acpi_setup(s);
 }
-- 
2.43.0

[PATCH v4 4/4] qemu-options: enable -smbios option on RISC-V

[Heinrich Schuchardt]

With SMBIOS support added for RISC-V we also should enable the command line
option.

Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Daniel Henrique Barboza 
Acked-by: Alistair Francis 
Reviewed-by: Andrew Jones 
---
v4:
no change
v3:
no change
v2:
new patch
---
 qemu-options.hx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index d90bdffc7a..935aec7cb9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2697,7 +2697,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
 "specify SMBIOS type 17 fields\n"
 "-smbios type=41[,designation=str][,kind=str][,instance=%d][,pcidev=str]\n"
 "specify SMBIOS type 41 fields\n",
-QEMU_ARCH_I386 | QEMU_ARCH_ARM | QEMU_ARCH_LOONGARCH)
+QEMU_ARCH_I386 | QEMU_ARCH_ARM | QEMU_ARCH_LOONGARCH | QEMU_ARCH_RISCV)
 SRST
 ``-smbios file=binary``
 Load SMBIOS entry from binary file.
-- 
2.43.0

[PATCH v4 1/4] smbios: add processor-family option

[Heinrich Schuchardt]

For RISC-V the SMBIOS standard requires specific values of the processor
family value depending on the bitness of the CPU.

Add a processor-family option for SMBIOS table 4.

The value of processor-family may exceed 255 and therefore must be provided
in the Processor Family 2 field. Set the Processor Family field to 0xFE
which signals that the Processor Family 2 is used.

Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Alistair Francis 
Reviewed-by: Andrew Jones 
---
v4:
no change
v3:
no change
v2:
new patch
---
 hw/smbios/smbios.c | 13 +++--
 qemu-options.hx|  4 ++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 2a90601ac5..647bc6d603 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -102,6 +102,7 @@ static struct {
 #define DEFAULT_CPU_SPEED 2000
 
 static struct {
+uint16_t processor_family;
 const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
 uint64_t max_speed;
 uint64_t current_speed;
@@ -110,6 +111,7 @@ static struct {
 .max_speed = DEFAULT_CPU_SPEED,
 .current_speed = DEFAULT_CPU_SPEED,
 .processor_id = 0,
+.processor_family = 0x01, /* Other */
 };
 
 struct type8_instance {
@@ -337,6 +339,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
 .name = "part",
 .type = QEMU_OPT_STRING,
 .help = "part number",
+}, {
+.name = "processor-family",
+.type = QEMU_OPT_NUMBER,
+.help = "processor family",
 }, {
 .name = "processor-id",
 .type = QEMU_OPT_NUMBER,
@@ -726,7 +732,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
 snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
 SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
 t->processor_type = 0x03; /* CPU */
-t->processor_family = 0x01; /* Other */
+t->processor_family = 0xfe; /* use Processor Family 2 field */
 SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
 if (type4.processor_id == 0) {
 t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
@@ -758,7 +764,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
 t->thread_count = (threads_per_socket > 255) ? 0xFF : threads_per_socket;
 
 t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
-t->processor_family2 = cpu_to_le16(0x01); /* Other */
+t->processor_family2 = cpu_to_le16(type4.processor_family);
 
 if (tbl_len == SMBIOS_TYPE_4_LEN_V30) {
 t->core_count2 = t->core_enabled2 = cpu_to_le16(cores_per_socket);
@@ -1402,6 +1408,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
 return;
 }
 save_opt(_pfx, opts, "sock_pfx");
+type4.processor_family = qemu_opt_get_number(opts,
+ "processor-family",
+ 0x01 /* Other */);
 save_opt(, opts, "manufacturer");
 save_opt(, opts, "version");
 save_opt(, opts, "serial");
diff --git a/qemu-options.hx b/qemu-options.hx
index ced8284863..d90bdffc7a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2686,7 +2686,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
 "specify SMBIOS type 3 fields\n"
 "-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
 "  [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
-"  [,processor-id=%d]\n"
+"  [,processor-family=%d,processor-id=%d]\n"
 "specify SMBIOS type 4 fields\n"
 "-smbios 
type=8[,external_reference=str][,internal_reference=str][,connector_type=%d][,port_type=%d]\n"
 "specify SMBIOS type 8 fields\n"
@@ -2714,7 +2714,7 @@ SRST
 ``-smbios 
type=3[,manufacturer=str][,version=str][,serial=str][,asset=str][,sku=str]``
 Specify SMBIOS type 3 fields
 
-``-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-id=%d]``
+``-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-family=%d][,processor-id=%d]``
 Specify SMBIOS type 4 fields
 
 ``-smbios type=11[,value=str][,path=filename]``
-- 
2.43.0

[PATCH v4 2/4] smbios: function to set default processor family

[Heinrich Schuchardt]

Provide a function to set the default processor family.

Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Andrew Jones 
---
v4:
no change
v3:
no change
v2:
new patch
---
 hw/smbios/smbios.c   | 7 +++
 include/hw/firmware/smbios.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 647bc6d603..c0c5a81e66 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -989,6 +989,13 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
 field = value;\
 }
 
+void smbios_set_default_processor_family(uint16_t processor_family)
+{
+if (type4.processor_family <= 0x01) {
+type4.processor_family = processor_family;
+}
+}
+
 void smbios_set_defaults(const char *manufacturer, const char *product,
  const char *version, bool legacy_mode,
  bool uuid_encoded, SmbiosEntryPointType ep_type)
diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index 7f3259a630..6e514982d4 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -295,6 +295,7 @@ void smbios_set_cpuid(uint32_t version, uint32_t features);
 void smbios_set_defaults(const char *manufacturer, const char *product,
  const char *version, bool legacy_mode,
  bool uuid_encoded, SmbiosEntryPointType ep_type);
+void smbios_set_default_processor_family(uint16_t processor_family);
 uint8_t *smbios_get_table_legacy(MachineState *ms, size_t *length);
 void smbios_get_tables(MachineState *ms,
const struct smbios_phys_mem_area *mem_array,
-- 
2.43.0

[PATCH v4 0/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.

With the series the following firmware tables are provided:

etc/smbios/smbios-anchor
etc/smbios/smbios-tables

Add processor-family to the '-smbios type=4' command line options.

v4:
remove a superfluous #ifdef
v3:
use misa_mxl_max to determine bitness
v2:
set processor family

Heinrich Schuchardt (4):
  smbios: add processor-family option
  smbios: function to set default processor family
  target/riscv: SMBIOS support for RISC-V virt machine
  qemu-options: enable -smbios option on RISC-V

 hw/riscv/Kconfig |  1 +
 hw/riscv/virt.c  | 42 
 hw/smbios/smbios.c   | 20 +++--
 include/hw/firmware/smbios.h |  1 +
 qemu-options.hx  |  6 +++---
 5 files changed, 65 insertions(+), 5 deletions(-)

-- 
2.43.0

Re: [PATCH v2 3/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

On 22.01.24 13:59, Andrew Jones wrote:

On Mon, Jan 22, 2024 at 01:28:18PM +0100, Heinrich Schuchardt wrote:

On 22.01.24 10:57, Andrew Jones wrote:

On Fri, Dec 29, 2023 at 01:07:23PM +0100, Heinrich Schuchardt wrote:

...

+#if defined(TARGET_RISCV32)
+smbios_set_default_processor_family(0x200);
+#elif defined(TARGET_RISCV64)
+smbios_set_default_processor_family(0x201);
+#endif


I think we should use misa_mxl_max to determine the family, rather than
TARGET_*, because, iirc, we're slowly working our ways towards allowing
rv32 cpus to be instantiated with qemu-system-riscv64.


Hello Andrew,

thank you for reviewing. I guess you mean something like:

 if (riscv_is_32bit(>soc[0])) {
 smbios_set_default_processor_family(0x200);
#if defined(TARGET_RISCV64)
 } else {
 smbios_set_default_processor_family(0x201);
#endif
 }


Yes, but I'm not sure we need the #ifdef around the 64-bit part.


I just followed the style in riscv_cpu_validate_misa_mxl().

Best regards

Heinrich





riscv_is_32bit returns harts->harts[0].env.misa_mxl_max == MXL_RV32.

Some real hardware has a 32bit hart and multiple 64bit harts. Will QEMU
support mixing harts with different bitness on the virt machine in future?
In that case we would have to revisit the code using misa_mxl_max in
multiple places.



Never say never, but I don't think there has been much effort to support
these types of configurations with a single QEMU binary. My googling is
failing me right now, but I seem to recall that there may have been
efforts to implement Arm big.LITTLE with multiprocess QEMU [1]. IOW, I
think we're safe to use misa_mxl_max, since we'll have one for each QEMU
instance and we'll use a different QEMU instance for each hart bitness.

[1] docs/system/multi-process.rst

Thanks,
drew

[PATCH v3 4/4] qemu-options: enable -smbios option on RISC-V

[Heinrich Schuchardt]

With SMBIOS support added for RISC-V we also should enable the command line
option.

Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Daniel Henrique Barboza 
Acked-by: Alistair Francis 
Reviewed-by: Andrew Jones 
---
v3:
no change
v2:
new patch
---
 qemu-options.hx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index d90bdffc7a..935aec7cb9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2697,7 +2697,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
 "specify SMBIOS type 17 fields\n"
 "-smbios type=41[,designation=str][,kind=str][,instance=%d][,pcidev=str]\n"
 "specify SMBIOS type 41 fields\n",
-QEMU_ARCH_I386 | QEMU_ARCH_ARM | QEMU_ARCH_LOONGARCH)
+QEMU_ARCH_I386 | QEMU_ARCH_ARM | QEMU_ARCH_LOONGARCH | QEMU_ARCH_RISCV)
 SRST
 ``-smbios file=binary``
 Load SMBIOS entry from binary file.
-- 
2.43.0

[PATCH v3 3/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.
Set the default processor family in the type 4 table.

The implementation is based on the corresponding ARM and Loongson code.

With the patch the following firmware tables are provided:

etc/smbios/smbios-anchor
etc/smbios/smbios-tables

Signed-off-by: Heinrich Schuchardt 
---
v3:
use misa_mxl_max to determine bitness
v2:
set processor family
---
 hw/riscv/Kconfig |  1 +
 hw/riscv/virt.c  | 44 
 2 files changed, 45 insertions(+)

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index a50717be87..5d644eb7b1 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -41,6 +41,7 @@ config RISCV_VIRT
 select RISCV_IMSIC
 select SIFIVE_PLIC
 select SIFIVE_TEST
+select SMBIOS
 select VIRTIO_MMIO
 select FW_CFG_DMA
 select PLATFORM_BUS
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index f9fd1341fc..1b333af4f0 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -36,6 +36,7 @@
 #include "hw/riscv/boot.h"
 #include "hw/riscv/numa.h"
 #include "kvm/kvm_riscv.h"
+#include "hw/firmware/smbios.h"
 #include "hw/intc/riscv_aclint.h"
 #include "hw/intc/riscv_aplic.h"
 #include "hw/intc/sifive_plic.h"
@@ -1263,6 +1264,47 @@ static void create_platform_bus(RISCVVirtState *s, 
DeviceState *irqchip)
 sysbus_mmio_get_region(sysbus, 0));
 }
 
+static void virt_build_smbios(RISCVVirtState *s)
+{
+MachineClass *mc = MACHINE_GET_CLASS(s);
+MachineState *ms = MACHINE(s);
+uint8_t *smbios_tables, *smbios_anchor;
+size_t smbios_tables_len, smbios_anchor_len;
+struct smbios_phys_mem_area mem_array;
+const char *product = "QEMU Virtual Machine";
+
+if (kvm_enabled()) {
+product = "KVM Virtual Machine";
+}
+
+smbios_set_defaults("QEMU", product, mc->name, false,
+true, SMBIOS_ENTRY_POINT_TYPE_64);
+
+if (riscv_is_32bit(>soc[0])) {
+smbios_set_default_processor_family(0x200);
+#if defined(TARGET_RISCV64)
+} else {
+smbios_set_default_processor_family(0x201);
+#endif
+}
+
+/* build the array of physical mem area from base_memmap */
+mem_array.address = s->memmap[VIRT_DRAM].base;
+mem_array.length = ms->ram_size;
+
+smbios_get_tables(ms, _array, 1,
+  _tables, _tables_len,
+  _anchor, _anchor_len,
+  _fatal);
+
+if (smbios_anchor) {
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-tables",
+smbios_tables, smbios_tables_len);
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-anchor",
+smbios_anchor, smbios_anchor_len);
+}
+}
+
 static void virt_machine_done(Notifier *notifier, void *data)
 {
 RISCVVirtState *s = container_of(notifier, RISCVVirtState,
@@ -1351,6 +1393,8 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 riscv_setup_direct_kernel(kernel_entry, fdt_load_addr);
 }
 
+virt_build_smbios(s);
+
 if (virt_is_acpi_enabled(s)) {
 virt_acpi_setup(s);
 }
-- 
2.43.0

[PATCH v3 0/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.

With the series the following firmware tables are provided:

etc/smbios/smbios-anchor
etc/smbios/smbios-tables

Add processor-family to the '-smbios type=4' command line options.

v3:
use misa_mxl_max to determine bitness
v2:
set processor family

Heinrich Schuchardt (4):
  smbios: add processor-family option
  smbios: function to set default processor family
  target/riscv: SMBIOS support for RISC-V virt machine
  qemu-options: enable -smbios option on RISC-V

 hw/riscv/Kconfig |  1 +
 hw/riscv/virt.c  | 44 
 hw/smbios/smbios.c   | 20 ++--
 include/hw/firmware/smbios.h |  1 +
 qemu-options.hx  |  6 ++---
 5 files changed, 67 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] smbios: add processor-family option

[Heinrich Schuchardt]

For RISC-V the SMBIOS standard requires specific values of the processor
family value depending on the bitness of the CPU.

Add a processor-family option for SMBIOS table 4.

The value of processor-family may exceed 255 and therefore must be provided
in the Processor Family 2 field. Set the Processor Family field to 0xFE
which signals that the Processor Family 2 is used.

Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Alistair Francis 
Reviewed-by: Andrew Jones 
---
v3:
no change
v2:
new patch
---
 hw/smbios/smbios.c | 13 +++--
 qemu-options.hx|  4 ++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 2a90601ac5..647bc6d603 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -102,6 +102,7 @@ static struct {
 #define DEFAULT_CPU_SPEED 2000
 
 static struct {
+uint16_t processor_family;
 const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
 uint64_t max_speed;
 uint64_t current_speed;
@@ -110,6 +111,7 @@ static struct {
 .max_speed = DEFAULT_CPU_SPEED,
 .current_speed = DEFAULT_CPU_SPEED,
 .processor_id = 0,
+.processor_family = 0x01, /* Other */
 };
 
 struct type8_instance {
@@ -337,6 +339,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
 .name = "part",
 .type = QEMU_OPT_STRING,
 .help = "part number",
+}, {
+.name = "processor-family",
+.type = QEMU_OPT_NUMBER,
+.help = "processor family",
 }, {
 .name = "processor-id",
 .type = QEMU_OPT_NUMBER,
@@ -726,7 +732,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
 snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
 SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
 t->processor_type = 0x03; /* CPU */
-t->processor_family = 0x01; /* Other */
+t->processor_family = 0xfe; /* use Processor Family 2 field */
 SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
 if (type4.processor_id == 0) {
 t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
@@ -758,7 +764,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
 t->thread_count = (threads_per_socket > 255) ? 0xFF : threads_per_socket;
 
 t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
-t->processor_family2 = cpu_to_le16(0x01); /* Other */
+t->processor_family2 = cpu_to_le16(type4.processor_family);
 
 if (tbl_len == SMBIOS_TYPE_4_LEN_V30) {
 t->core_count2 = t->core_enabled2 = cpu_to_le16(cores_per_socket);
@@ -1402,6 +1408,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
 return;
 }
 save_opt(_pfx, opts, "sock_pfx");
+type4.processor_family = qemu_opt_get_number(opts,
+ "processor-family",
+ 0x01 /* Other */);
 save_opt(, opts, "manufacturer");
 save_opt(, opts, "version");
 save_opt(, opts, "serial");
diff --git a/qemu-options.hx b/qemu-options.hx
index ced8284863..d90bdffc7a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2686,7 +2686,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
 "specify SMBIOS type 3 fields\n"
 "-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
 "  [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
-"  [,processor-id=%d]\n"
+"  [,processor-family=%d,processor-id=%d]\n"
 "specify SMBIOS type 4 fields\n"
 "-smbios 
type=8[,external_reference=str][,internal_reference=str][,connector_type=%d][,port_type=%d]\n"
 "specify SMBIOS type 8 fields\n"
@@ -2714,7 +2714,7 @@ SRST
 ``-smbios 
type=3[,manufacturer=str][,version=str][,serial=str][,asset=str][,sku=str]``
 Specify SMBIOS type 3 fields
 
-``-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-id=%d]``
+``-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-family=%d][,processor-id=%d]``
 Specify SMBIOS type 4 fields
 
 ``-smbios type=11[,value=str][,path=filename]``
-- 
2.43.0

[PATCH v3 2/4] smbios: function to set default processor family

[Heinrich Schuchardt]

Provide a function to set the default processor family.

Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Andrew Jones 
---
v3:
no change
v2:
new patch
---
 hw/smbios/smbios.c   | 7 +++
 include/hw/firmware/smbios.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 647bc6d603..c0c5a81e66 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -989,6 +989,13 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
 field = value;\
 }
 
+void smbios_set_default_processor_family(uint16_t processor_family)
+{
+if (type4.processor_family <= 0x01) {
+type4.processor_family = processor_family;
+}
+}
+
 void smbios_set_defaults(const char *manufacturer, const char *product,
  const char *version, bool legacy_mode,
  bool uuid_encoded, SmbiosEntryPointType ep_type)
diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index 7f3259a630..6e514982d4 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -295,6 +295,7 @@ void smbios_set_cpuid(uint32_t version, uint32_t features);
 void smbios_set_defaults(const char *manufacturer, const char *product,
  const char *version, bool legacy_mode,
  bool uuid_encoded, SmbiosEntryPointType ep_type);
+void smbios_set_default_processor_family(uint16_t processor_family);
 uint8_t *smbios_get_table_legacy(MachineState *ms, size_t *length);
 void smbios_get_tables(MachineState *ms,
const struct smbios_phys_mem_area *mem_array,
-- 
2.43.0

Re: [PATCH v2 3/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

On 22.01.24 10:57, Andrew Jones wrote:

On Fri, Dec 29, 2023 at 01:07:23PM +0100, Heinrich Schuchardt wrote:

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.
Set the default processor family in the type 4 table.

The implementation is based on the corresponding ARM and Loongson code.

With the patch the following firmware tables are provided:

 etc/smbios/smbios-anchor
 etc/smbios/smbios-tables

Signed-off-by: Heinrich Schuchardt 
---
v2:
set processor family
---
  hw/riscv/Kconfig |  1 +
  hw/riscv/virt.c  | 42 ++
  2 files changed, 43 insertions(+)

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index b6a5eb4452..1e11ac9432 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -41,6 +41,7 @@ config RISCV_VIRT
  select RISCV_IMSIC
  select SIFIVE_PLIC
  select SIFIVE_TEST
+select SMBIOS
  select VIRTIO_MMIO
  select FW_CFG_DMA
  select PLATFORM_BUS
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d2eac24156..a876dd8f34 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -36,6 +36,7 @@
  #include "hw/riscv/boot.h"
  #include "hw/riscv/numa.h"
  #include "kvm/kvm_riscv.h"
+#include "hw/firmware/smbios.h"
  #include "hw/intc/riscv_aclint.h"
  #include "hw/intc/riscv_aplic.h"
  #include "hw/intc/riscv_imsic.h"
@@ -1249,6 +1250,45 @@ static void create_platform_bus(RISCVVirtState *s, 
DeviceState *irqchip)
  sysbus_mmio_get_region(sysbus, 0));
  }
  
+static void virt_build_smbios(RISCVVirtState *s)

+{
+MachineClass *mc = MACHINE_GET_CLASS(s);
+MachineState *ms = MACHINE(s);
+uint8_t *smbios_tables, *smbios_anchor;
+size_t smbios_tables_len, smbios_anchor_len;
+struct smbios_phys_mem_area mem_array;
+const char *product = "QEMU Virtual Machine";
+
+if (kvm_enabled()) {
+product = "KVM Virtual Machine";
+}
+
+smbios_set_defaults("QEMU", product, mc->name, false,
+true, SMBIOS_ENTRY_POINT_TYPE_64);
+
+#if defined(TARGET_RISCV32)
+smbios_set_default_processor_family(0x200);
+#elif defined(TARGET_RISCV64)
+smbios_set_default_processor_family(0x201);
+#endif


I think we should use misa_mxl_max to determine the family, rather than
TARGET_*, because, iirc, we're slowly working our ways towards allowing
rv32 cpus to be instantiated with qemu-system-riscv64.


Hello Andrew,

thank you for reviewing. I guess you mean something like:

if (riscv_is_32bit(>soc[0])) {
smbios_set_default_processor_family(0x200);
#if defined(TARGET_RISCV64)
} else {
smbios_set_default_processor_family(0x201);
#endif
}

riscv_is_32bit returns harts->harts[0].env.misa_mxl_max == MXL_RV32.

Some real hardware has a 32bit hart and multiple 64bit harts. Will QEMU 
support mixing harts with different bitness on the virt machine in 
future? In that case we would have to revisit the code using 
misa_mxl_max in multiple places.


Best regards

Heinrich




+
+/* build the array of physical mem area from base_memmap */
+mem_array.address = s->memmap[VIRT_DRAM].base;
+mem_array.length = ms->ram_size;
+
+smbios_get_tables(ms, _array, 1,
+  _tables, _tables_len,
+  _anchor, _anchor_len,
+  _fatal);
+
+if (smbios_anchor) {
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-tables",
+smbios_tables, smbios_tables_len);
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-anchor",
+smbios_anchor, smbios_anchor_len);
+}
+}
+
  static void virt_machine_done(Notifier *notifier, void *data)
  {
  RISCVVirtState *s = container_of(notifier, RISCVVirtState,
@@ -1337,6 +1377,8 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
  riscv_setup_direct_kernel(kernel_entry, fdt_load_addr);
  }
  
+virt_build_smbios(s);

+
  if (virt_is_acpi_enabled(s)) {
  virt_acpi_setup(s);
  }
--
2.43.0




Otherwise,

Reviewed-by: Andrew Jones 

Thanks,
drew

Re: [PATCH v2 1/4] smbios: add processor-family option

[Heinrich Schuchardt]

On 1/5/24 06:24, Alistair Francis wrote:

On Fri, Dec 29, 2023 at 10:48 PM Heinrich Schuchardt
 wrote:


For RISC-V the SMBIOS standard requires specific values of the processor
family value depending on the bitness of the CPU.


Can you provide some details of where this is described? I can't seem to find it

Alistair


System Management BIOS (SMBIOS) Reference Specification 3.7.0 (DSP0134)
https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.7.0.pdf
7.5.2 Processor Information — Processor Family
Table 23 – Processor Information: Processor Family field

200h 512 RISC-V RV32
201h 513 RISC-V RV64
202h 514 RISC-V RV128

While for other architectures values for different CPU generations have 
been defined the values for RISC-V only depend on the bitness.


Best regards

Heinrich





Add a processor-family option for SMBIOS table 4.

The value of processor-family may exceed 255 and therefore must be provided
in the Processor Family 2 field. Set the Processor Family field to 0xFE
which signals that the Processor Family 2 is used.

Signed-off-by: Heinrich Schuchardt 
---
v2:
 new patch
---
  hw/smbios/smbios.c | 13 +++--
  qemu-options.hx|  4 ++--
  2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 2a90601ac5..647bc6d603 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -102,6 +102,7 @@ static struct {
  #define DEFAULT_CPU_SPEED 2000

  static struct {
+uint16_t processor_family;
  const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
  uint64_t max_speed;
  uint64_t current_speed;
@@ -110,6 +111,7 @@ static struct {
  .max_speed = DEFAULT_CPU_SPEED,
  .current_speed = DEFAULT_CPU_SPEED,
  .processor_id = 0,
+.processor_family = 0x01, /* Other */
  };

  struct type8_instance {
@@ -337,6 +339,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
  .name = "part",
  .type = QEMU_OPT_STRING,
  .help = "part number",
+}, {
+.name = "processor-family",
+.type = QEMU_OPT_NUMBER,
+.help = "processor family",
  }, {
  .name = "processor-id",
  .type = QEMU_OPT_NUMBER,
@@ -726,7 +732,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
  snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
  SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
  t->processor_type = 0x03; /* CPU */
-t->processor_family = 0x01; /* Other */
+t->processor_family = 0xfe; /* use Processor Family 2 field */
  SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
  if (type4.processor_id == 0) {
  t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
@@ -758,7 +764,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
  t->thread_count = (threads_per_socket > 255) ? 0xFF : threads_per_socket;

  t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
-t->processor_family2 = cpu_to_le16(0x01); /* Other */
+t->processor_family2 = cpu_to_le16(type4.processor_family);

  if (tbl_len == SMBIOS_TYPE_4_LEN_V30) {
  t->core_count2 = t->core_enabled2 = cpu_to_le16(cores_per_socket);
@@ -1402,6 +1408,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
  return;
  }
  save_opt(_pfx, opts, "sock_pfx");
+type4.processor_family = qemu_opt_get_number(opts,
+ "processor-family",
+ 0x01 /* Other */);
  save_opt(, opts, "manufacturer");
  save_opt(, opts, "version");
  save_opt(, opts, "serial");
diff --git a/qemu-options.hx b/qemu-options.hx
index b66570ae00..7bdb414345 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2694,7 +2694,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
  "specify SMBIOS type 3 fields\n"
  "-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
  "  
[,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
-"  [,processor-id=%d]\n"
+"  [,processor-family=%d,processor-id=%d]\n"
  "specify SMBIOS type 4 fields\n"
  "-smbios 
type=8[,external_reference=str][,internal_reference=str][,connector_type=%d][,port_type=%d]\n"
  "specify SMBIOS type 8 fields\n"
@@ -2722,7 +2722,7 @@ SRST
  ``-smbios 
type=3[,manufacturer=str][,version=str][,serial=str][,asset=str][,sku=str]``
  Specify SMBIOS type 3 fields

-``-smbios 
type=4[,sock_pfx=str][,manufacturer=

Re: [PATCH for 8.2.1] hw/net: cadence_gem: Fix MDIO_OP_xxx values

[Heinrich Schuchardt]

On 1/2/24 16:08, Philippe Mathieu-Daudé wrote:

On 2/1/24 15:18, Bin Meng wrote:

Testing upstream U-Boot with 'sifive_u' machine we see:

   => dhcp
   ethernet@1009: PHY present at 0
   Could not get PHY for ethernet@1009: addr 0
   phy_connect failed

This has been working till QEMU 8.1 but broken since QEMU 8.2.


s/till/until/?


These are synonyms. Till is more informal. No need to change.




Fixes: 1b09eeb122aa ("hw/net/cadence_gem: use FIELD to describe
PHYMNTNC register fields")
Reported-by: Heinrich Schuchardt 
Signed-off-by: Bin Meng 

---

  hw/net/cadence_gem.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 296bba238e..472ce9c8cf 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -199,8 +199,8 @@ REG32(PHYMNTNC, 0x34) /* Phy Maintenance reg */
  FIELD(PHYMNTNC, PHY_ADDR, 23, 5)
  FIELD(PHYMNTNC, OP, 28, 2)
  FIELD(PHYMNTNC, ST, 30, 2)
-#define MDIO_OP_READ    0x3
-#define MDIO_OP_WRITE   0x2
+#define MDIO_OP_READ    0x2
+#define MDIO_OP_WRITE   0x1
  REG32(RXPAUSE, 0x38) /* RX Pause Time reg */
  REG32(TXPAUSE, 0x3c) /* TX Pause Time reg */


Reviewed-by: Philippe Mathieu-Daudé 



Thank you Bin for the fix.

With the fix I was able to download a file via TFTP to U-Boot
sifive_unleashed_defconfig on the emulated board. Cf.
docs/system/riscv/sifive_u.rst.

Tested-by: Heinrich Schuchardt 

Re: [PATCH v2 1/1] docs: pcie: describe PCIe option ROMs

[Heinrich Schuchardt]

On 8/14/22 17:32, Heinrich Schuchardt wrote:

Provide a descriptions of the options that control the emulation of option
ROMS for PCIe devices.

Signed-off-by: Heinrich Schuchardt 
---
v2:
correct description of rombar property
use romfile= to suppress option ROM loading
---
  docs/pcie.txt | 28 
  1 file changed, 28 insertions(+)

diff --git a/docs/pcie.txt b/docs/pcie.txt
index 89e3502075..b60f189bd4 100644
--- a/docs/pcie.txt
+++ b/docs/pcie.txt
@@ -292,6 +292,34 @@ PCI-PCI Bridge slots can be used for legacy PCI host 
devices.
  If you can see the "Express Endpoint" capability in the
  output, then the device is indeed PCI Express.
  
+8. Option ROM

+=
+PCIe devices may provide an option ROM. The following properties control the
+emulation of the option ROM:
+
+``rombar`` (default: ``1``)
+  For vfio-pci devices a vendor and product ID based denylist exists which
+  controls if an available option ROM shall be probed. The 'rombar' option
+  allows to override this setting. The value is used as follows:
+  0 = skip probing, 1 = force probing
+
+``romfile``
+  Defines the name of the file to be loaded as option ROM.
+  The file size may neither exceed 2 GiB nor ``romsize``.
+  Some devices like virtio-net-pci define a default file name.
+
+``romsize`` (default: ``-1``)
+  Specifies the size of the option ROM in bytes. The value must be either
+  ``-1`` or a power of two. ``-1`` signifies unlimited size.
+
+Some QEMU PCIe devices like virtio-net-pci use an option ROM by default. In the
+following example the option ROM of a virtio-net-pci device is disabled by
+specifying an empty ``romfile`` property. This is useful for architectures 
where
+QEMU does not supply an option ROM file.
+
+.. code-block:: console
+
+-device virtio-net-pci,netdev=eth1,mq=on,romfile=
  
  7. Virtio devices

  =


Hello Michael, hello Marcel,

Unfortunately the patch was never reviewed.

The patch is available at:

https://lore.kernel.org/qemu-devel/20220814153220.2439468-1-heinrich.schucha...@canonical.com/

https://patchwork.kernel.org/project/qemu-devel/patch/20220814153220.2439468-1-heinrich.schucha...@canonical.com/
Best regards

Heinrich

[PATCH v2 0/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.

With the series the following firmware tables are provided:

etc/smbios/smbios-anchor
etc/smbios/smbios-tables

Add processor-family to the '-smbios type=4' command line options.

Heinrich Schuchardt (4):
  smbios: add processor-family option
  smbios: function to set default processor family
  target/riscv: SMBIOS support for RISC-V virt machine
  qemu-options: enable -smbios option on RISC-V

 hw/riscv/Kconfig |  1 +
 hw/riscv/virt.c  | 42 
 hw/smbios/smbios.c   | 20 ++--
 include/hw/firmware/smbios.h |  1 +
 qemu-options.hx  |  6 +++---
 5 files changed, 65 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v2 2/4] smbios: function to set default processor family

[Heinrich Schuchardt]

Provide a function to set the default processor family.

Signed-off-by: Heinrich Schuchardt 
---
v2:
new patch
---
 hw/smbios/smbios.c   | 7 +++
 include/hw/firmware/smbios.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 647bc6d603..03fe736565 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -989,6 +989,13 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
 field = value;\
 }
 
+void smbios_set_default_processor_family(uint16_t processor_family)
+{
+if (type4.processor_family <= 0x01) {
+type4.processor_family = processor_family;
+}
+}
+
 void smbios_set_defaults(const char *manufacturer, const char *product,
  const char *version, bool legacy_mode,
  bool uuid_encoded, SmbiosEntryPointType ep_type)
diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index 7f3259a630..6e514982d4 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -295,6 +295,7 @@ void smbios_set_cpuid(uint32_t version, uint32_t features);
 void smbios_set_defaults(const char *manufacturer, const char *product,
  const char *version, bool legacy_mode,
  bool uuid_encoded, SmbiosEntryPointType ep_type);
+void smbios_set_default_processor_family(uint16_t processor_family);
 uint8_t *smbios_get_table_legacy(MachineState *ms, size_t *length);
 void smbios_get_tables(MachineState *ms,
const struct smbios_phys_mem_area *mem_array,
-- 
2.43.0

[PATCH v2 1/4] smbios: add processor-family option

[Heinrich Schuchardt]

For RISC-V the SMBIOS standard requires specific values of the processor
family value depending on the bitness of the CPU.

Add a processor-family option for SMBIOS table 4.

The value of processor-family may exceed 255 and therefore must be provided
in the Processor Family 2 field. Set the Processor Family field to 0xFE
which signals that the Processor Family 2 is used.

Signed-off-by: Heinrich Schuchardt 
---
v2:
new patch
---
 hw/smbios/smbios.c | 13 +++--
 qemu-options.hx|  4 ++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 2a90601ac5..647bc6d603 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -102,6 +102,7 @@ static struct {
 #define DEFAULT_CPU_SPEED 2000
 
 static struct {
+uint16_t processor_family;
 const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
 uint64_t max_speed;
 uint64_t current_speed;
@@ -110,6 +111,7 @@ static struct {
 .max_speed = DEFAULT_CPU_SPEED,
 .current_speed = DEFAULT_CPU_SPEED,
 .processor_id = 0,
+.processor_family = 0x01, /* Other */
 };
 
 struct type8_instance {
@@ -337,6 +339,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
 .name = "part",
 .type = QEMU_OPT_STRING,
 .help = "part number",
+}, {
+.name = "processor-family",
+.type = QEMU_OPT_NUMBER,
+.help = "processor family",
 }, {
 .name = "processor-id",
 .type = QEMU_OPT_NUMBER,
@@ -726,7 +732,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
 snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
 SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
 t->processor_type = 0x03; /* CPU */
-t->processor_family = 0x01; /* Other */
+t->processor_family = 0xfe; /* use Processor Family 2 field */
 SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
 if (type4.processor_id == 0) {
 t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
@@ -758,7 +764,7 @@ static void smbios_build_type_4_table(MachineState *ms, 
unsigned instance)
 t->thread_count = (threads_per_socket > 255) ? 0xFF : threads_per_socket;
 
 t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
-t->processor_family2 = cpu_to_le16(0x01); /* Other */
+t->processor_family2 = cpu_to_le16(type4.processor_family);
 
 if (tbl_len == SMBIOS_TYPE_4_LEN_V30) {
 t->core_count2 = t->core_enabled2 = cpu_to_le16(cores_per_socket);
@@ -1402,6 +1408,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
 return;
 }
 save_opt(_pfx, opts, "sock_pfx");
+type4.processor_family = qemu_opt_get_number(opts,
+ "processor-family",
+ 0x01 /* Other */);
 save_opt(, opts, "manufacturer");
 save_opt(, opts, "version");
 save_opt(, opts, "serial");
diff --git a/qemu-options.hx b/qemu-options.hx
index b66570ae00..7bdb414345 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2694,7 +2694,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
 "specify SMBIOS type 3 fields\n"
 "-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
 "  [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
-"  [,processor-id=%d]\n"
+"  [,processor-family=%d,processor-id=%d]\n"
 "specify SMBIOS type 4 fields\n"
 "-smbios 
type=8[,external_reference=str][,internal_reference=str][,connector_type=%d][,port_type=%d]\n"
 "specify SMBIOS type 8 fields\n"
@@ -2722,7 +2722,7 @@ SRST
 ``-smbios 
type=3[,manufacturer=str][,version=str][,serial=str][,asset=str][,sku=str]``
 Specify SMBIOS type 3 fields
 
-``-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-id=%d]``
+``-smbios 
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-family=%d][,processor-id=%d]``
 Specify SMBIOS type 4 fields
 
 ``-smbios type=11[,value=str][,path=filename]``
-- 
2.43.0

[PATCH v2 3/4] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.
Set the default processor family in the type 4 table.

The implementation is based on the corresponding ARM and Loongson code.

With the patch the following firmware tables are provided:

etc/smbios/smbios-anchor
etc/smbios/smbios-tables

Signed-off-by: Heinrich Schuchardt 
---
v2:
set processor family
---
 hw/riscv/Kconfig |  1 +
 hw/riscv/virt.c  | 42 ++
 2 files changed, 43 insertions(+)

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index b6a5eb4452..1e11ac9432 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -41,6 +41,7 @@ config RISCV_VIRT
 select RISCV_IMSIC
 select SIFIVE_PLIC
 select SIFIVE_TEST
+select SMBIOS
 select VIRTIO_MMIO
 select FW_CFG_DMA
 select PLATFORM_BUS
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d2eac24156..a876dd8f34 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -36,6 +36,7 @@
 #include "hw/riscv/boot.h"
 #include "hw/riscv/numa.h"
 #include "kvm/kvm_riscv.h"
+#include "hw/firmware/smbios.h"
 #include "hw/intc/riscv_aclint.h"
 #include "hw/intc/riscv_aplic.h"
 #include "hw/intc/riscv_imsic.h"
@@ -1249,6 +1250,45 @@ static void create_platform_bus(RISCVVirtState *s, 
DeviceState *irqchip)
 sysbus_mmio_get_region(sysbus, 0));
 }
 
+static void virt_build_smbios(RISCVVirtState *s)
+{
+MachineClass *mc = MACHINE_GET_CLASS(s);
+MachineState *ms = MACHINE(s);
+uint8_t *smbios_tables, *smbios_anchor;
+size_t smbios_tables_len, smbios_anchor_len;
+struct smbios_phys_mem_area mem_array;
+const char *product = "QEMU Virtual Machine";
+
+if (kvm_enabled()) {
+product = "KVM Virtual Machine";
+}
+
+smbios_set_defaults("QEMU", product, mc->name, false,
+true, SMBIOS_ENTRY_POINT_TYPE_64);
+
+#if defined(TARGET_RISCV32)
+smbios_set_default_processor_family(0x200);
+#elif defined(TARGET_RISCV64)
+smbios_set_default_processor_family(0x201);
+#endif
+
+/* build the array of physical mem area from base_memmap */
+mem_array.address = s->memmap[VIRT_DRAM].base;
+mem_array.length = ms->ram_size;
+
+smbios_get_tables(ms, _array, 1,
+  _tables, _tables_len,
+  _anchor, _anchor_len,
+  _fatal);
+
+if (smbios_anchor) {
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-tables",
+smbios_tables, smbios_tables_len);
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-anchor",
+smbios_anchor, smbios_anchor_len);
+}
+}
+
 static void virt_machine_done(Notifier *notifier, void *data)
 {
 RISCVVirtState *s = container_of(notifier, RISCVVirtState,
@@ -1337,6 +1377,8 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 riscv_setup_direct_kernel(kernel_entry, fdt_load_addr);
 }
 
+virt_build_smbios(s);
+
 if (virt_is_acpi_enabled(s)) {
 virt_acpi_setup(s);
 }
-- 
2.43.0

[PATCH v2 4/4] qemu-options: enable -smbios option on RISC-V

[Heinrich Schuchardt]

With SMBIOS support added for RISC-V we also should enable the command line
option.

Signed-off-by: Heinrich Schuchardt 
---
v2:
new patch
---
 qemu-options.hx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 7bdb414345..5ed82df11f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2705,7 +2705,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
 "specify SMBIOS type 17 fields\n"
 "-smbios type=41[,designation=str][,kind=str][,instance=%d][,pcidev=str]\n"
 "specify SMBIOS type 41 fields\n",
-QEMU_ARCH_I386 | QEMU_ARCH_ARM | QEMU_ARCH_LOONGARCH)
+QEMU_ARCH_I386 | QEMU_ARCH_ARM | QEMU_ARCH_LOONGARCH | QEMU_ARCH_RISCV)
 SRST
 ``-smbios file=binary``
 Load SMBIOS entry from binary file.
-- 
2.43.0

[PATCH v2 1/1] docs/system/riscv: document acpi parameter of virt machine

[Heinrich Schuchardt]

Since QEMU v8.0.0 the RISC-V virt machine has a switch to disable ACPI
table generation. Add it to the documentation.

Fixes: 168b8c29cedb ("hw/riscv/virt: Add a switch to disable ACPI")
Signed-off-by: Heinrich Schuchardt 
---
v2:
mention that acpi=on is the default
---
 docs/system/riscv/virt.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index f5fa7b8b29..9a06f95a34 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -95,6 +95,11 @@ The following machine-specific options are supported:
   SiFive CLINT. When not specified, this option is assumed to be "off".
   This option is restricted to the TCG accelerator.
 
+- acpi=[on|off|auto]
+
+  When this option is "on" (which is the default), ACPI tables are generated 
and
+  exposed as firmware tables etc/acpi/rsdp and etc/acpi/tables.
+
 - aia=[none|aplic|aplic-imsic]
 
   This option allows selecting interrupt controller defined by the AIA
-- 
2.40.1

Re: [PATCH 1/1] docs/system/riscv: document acpi parameter of virt machine

[Heinrich Schuchardt]

On 20.12.23 07:57, Sunil V L wrote:

On Tue, Dec 19, 2023 at 03:38:29PM +0100, Heinrich Schuchardt wrote:

Since QEMU v8.0.0 the RISC-V virt machine has a switch to disable ACPI
table generation. Add it to the documentation.

Fixes: 168b8c29cedb ("hw/riscv/virt: Add a switch to disable ACPI")
Signed-off-by: Heinrich Schuchardt 
---
  docs/system/riscv/virt.rst | 5 +
  1 file changed, 5 insertions(+)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index f5fa7b8b29..4e134ff2ac 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -95,6 +95,11 @@ The following machine-specific options are supported:
SiFive CLINT. When not specified, this option is assumed to be "off".
This option is restricted to the TCG accelerator.
  
+- acpi=[on|off|auto]

+
+  When this option is "on", ACPI tables are generated and exposed as firmware
+  tables etc/acpi/rsdp and etc/acpi/tables.
+

Hi Heinrich,

Should we add, When not specified or set to auto, this option is assumed
to be "on"?


Sure.

Best regards

Heinrich

[PATCH 1/1] docs/system/riscv: document acpi parameter of virt machine

[Heinrich Schuchardt]

Since QEMU v8.0.0 the RISC-V virt machine has a switch to disable ACPI
table generation. Add it to the documentation.

Fixes: 168b8c29cedb ("hw/riscv/virt: Add a switch to disable ACPI")
Signed-off-by: Heinrich Schuchardt 
---
 docs/system/riscv/virt.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index f5fa7b8b29..4e134ff2ac 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -95,6 +95,11 @@ The following machine-specific options are supported:
   SiFive CLINT. When not specified, this option is assumed to be "off".
   This option is restricted to the TCG accelerator.
 
+- acpi=[on|off|auto]
+
+  When this option is "on", ACPI tables are generated and exposed as firmware
+  tables etc/acpi/rsdp and etc/acpi/tables.
+
 - aia=[none|aplic|aplic-imsic]
 
   This option allows selecting interrupt controller defined by the AIA
-- 
2.40.1

Re: [PATCH 1/1] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

On 12/18/23 09:49, Sunil V L wrote:

Hi Heinrich,

Thanks for the patch!.

On Mon, Dec 18, 2023 at 08:40:18AM +0100, Heinrich Schuchardt wrote:

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.

The implementation is based on the corresponding ARM and Loongson code.

With the patch the following firmware tables are provided:

 etc/smbios/smbios-anchor
 etc/smbios/smbios-tables

Booting Ubuntu 23.10 via EDK II allowed displaying the SMBIOS table using
the dmidecode command:

 Handle 0x0100, DMI type 1, 27 bytes
 System Information
 Manufacturer: QEMU
 Product Name: QEMU Virtual Machine
 Version: virt
 ...

Signed-off-by: Heinrich Schuchardt 
---
  hw/riscv/Kconfig |  1 +
  hw/riscv/virt.c  | 36 
  2 files changed, 37 insertions(+)

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index b6a5eb4452..1e11ac9432 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -41,6 +41,7 @@ config RISCV_VIRT
  select RISCV_IMSIC
  select SIFIVE_PLIC
  select SIFIVE_TEST
+select SMBIOS
  select VIRTIO_MMIO
  select FW_CFG_DMA
  select PLATFORM_BUS
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d2eac24156..6c27cb5330 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -36,6 +36,7 @@
  #include "hw/riscv/boot.h"
  #include "hw/riscv/numa.h"
  #include "kvm/kvm_riscv.h"
+#include "hw/firmware/smbios.h"
  #include "hw/intc/riscv_aclint.h"
  #include "hw/intc/riscv_aplic.h"
  #include "hw/intc/riscv_imsic.h"
@@ -1249,6 +1250,39 @@ static void create_platform_bus(RISCVVirtState *s, 
DeviceState *irqchip)
  sysbus_mmio_get_region(sysbus, 0));
  }
  
+static void virt_build_smbios(RISCVVirtState *s)

+{

Can we avoid duplicating this function which exists in other
architectures?



Every architecture uses it own structures (e.g. RISCVVirtState) and 
constants (e.g VIRT_DRAM). As long as this is not addressed we will have 
to live with this piece of code duplication.


After this patch is accepted we will have to work on improving SMBIOS 
3.7.0 compliance:


* Table 22
* * field Processor Family should contain a RISC-V specfic value. Maybe 
derived from TARGET_RISCV##.
* * field Processor ID should contain the value of mvendorid of hart 0 
(i.e. cpu->cfg.mvendorid).


* Table 44

The required contents of this table are provided in 
https://github.com/riscv/riscv-smbios .


Best regards

Heinrich

[PATCH 1/1] target/riscv: SMBIOS support for RISC-V virt machine

[Heinrich Schuchardt]

Generate SMBIOS tables for the RISC-V mach-virt.
Add CONFIG_SMBIOS=y to the RISC-V default config.

The implementation is based on the corresponding ARM and Loongson code.

With the patch the following firmware tables are provided:

etc/smbios/smbios-anchor
etc/smbios/smbios-tables

Booting Ubuntu 23.10 via EDK II allowed displaying the SMBIOS table using
the dmidecode command:

Handle 0x0100, DMI type 1, 27 bytes
System Information
Manufacturer: QEMU
Product Name: QEMU Virtual Machine
Version: virt
...

Signed-off-by: Heinrich Schuchardt 
---
 hw/riscv/Kconfig |  1 +
 hw/riscv/virt.c  | 36 
 2 files changed, 37 insertions(+)

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index b6a5eb4452..1e11ac9432 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -41,6 +41,7 @@ config RISCV_VIRT
 select RISCV_IMSIC
 select SIFIVE_PLIC
 select SIFIVE_TEST
+select SMBIOS
 select VIRTIO_MMIO
 select FW_CFG_DMA
 select PLATFORM_BUS
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d2eac24156..6c27cb5330 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -36,6 +36,7 @@
 #include "hw/riscv/boot.h"
 #include "hw/riscv/numa.h"
 #include "kvm/kvm_riscv.h"
+#include "hw/firmware/smbios.h"
 #include "hw/intc/riscv_aclint.h"
 #include "hw/intc/riscv_aplic.h"
 #include "hw/intc/riscv_imsic.h"
@@ -1249,6 +1250,39 @@ static void create_platform_bus(RISCVVirtState *s, 
DeviceState *irqchip)
 sysbus_mmio_get_region(sysbus, 0));
 }
 
+static void virt_build_smbios(RISCVVirtState *s)
+{
+MachineClass *mc = MACHINE_GET_CLASS(s);
+MachineState *ms = MACHINE(s);
+uint8_t *smbios_tables, *smbios_anchor;
+size_t smbios_tables_len, smbios_anchor_len;
+struct smbios_phys_mem_area mem_array;
+const char *product = "QEMU Virtual Machine";
+
+if (kvm_enabled()) {
+product = "KVM Virtual Machine";
+}
+
+smbios_set_defaults("QEMU", product, mc->name, false,
+true, SMBIOS_ENTRY_POINT_TYPE_64);
+
+/* build the array of physical mem area from base_memmap */
+mem_array.address = s->memmap[VIRT_DRAM].base;
+mem_array.length = ms->ram_size;
+
+smbios_get_tables(ms, _array, 1,
+  _tables, _tables_len,
+  _anchor, _anchor_len,
+  _fatal);
+
+if (smbios_anchor) {
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-tables",
+smbios_tables, smbios_tables_len);
+fw_cfg_add_file(s->fw_cfg, "etc/smbios/smbios-anchor",
+smbios_anchor, smbios_anchor_len);
+}
+}
+
 static void virt_machine_done(Notifier *notifier, void *data)
 {
 RISCVVirtState *s = container_of(notifier, RISCVVirtState,
@@ -1337,6 +1371,8 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
 riscv_setup_direct_kernel(kernel_entry, fdt_load_addr);
 }
 
+virt_build_smbios(s);
+
 if (virt_is_acpi_enabled(s)) {
 virt_acpi_setup(s);
 }
-- 
2.40.1

[PATCH v2 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]

[Heinrich Schuchardt]

The CSR register mseccfg is used by multiple extensions: Smepm and Zkr.

Consider this when checking the existence of the register.

Fixes: 77442380ecbe ("target/riscv: rvk: add CSR support for Zkr")
Signed-off-by: Heinrich Schuchardt 
---
v2:
rebase on alistair23/riscv-to-apply-next
---
 target/riscv/csr.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4ca96ddd1d..fc26b52c88 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -528,11 +528,14 @@ static RISCVException pmp(CPURISCVState *env, int csrno)
 return RISCV_EXCP_ILLEGAL_INST;
 }
 
-static RISCVException smepmp(CPURISCVState *env, int csrno)
+static RISCVException have_mseccfg(CPURISCVState *env, int csrno)
 {
 if (riscv_cpu_cfg(env)->ext_smepmp) {
 return RISCV_EXCP_NONE;
 }
+if (riscv_cpu_cfg(env)->ext_zkr) {
+return RISCV_EXCP_NONE;
+}
 
 return RISCV_EXCP_ILLEGAL_INST;
 }
@@ -4766,7 +4769,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
 [CSR_VSIPH]   = { "vsiph",   aia_hmode32, NULL, NULL, rmw_vsiph },
 
 /* Physical Memory Protection */
-[CSR_MSECCFG]= { "mseccfg", smepmp, read_mseccfg, write_mseccfg,
+[CSR_MSECCFG]= { "mseccfg",   have_mseccfg, read_mseccfg, 
write_mseccfg,
  .min_priv_ver = PRIV_VERSION_1_11_0   },
 [CSR_PMPCFG0]= { "pmpcfg0",   pmp, read_pmpcfg,  write_pmpcfg  },
 [CSR_PMPCFG1]= { "pmpcfg1",   pmp, read_pmpcfg,  write_pmpcfg  },
-- 
2.40.1

[PATCH 1/1] target/riscv: correct csr_ops[CSR_MSECCFG]

[Heinrich Schuchardt]

The CSR register mseccfg is used by multiple extensions: Smepm and Zkr.

Consider this when checking the existence of the register.

Fixes: 77442380ecbe ("target/riscv: rvk: add CSR support for Zkr")
Signed-off-by: Heinrich Schuchardt 
---
 target/riscv/csr.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4b4ab56c40..07c0cfb7d8 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -523,11 +523,14 @@ static RISCVException pmp(CPURISCVState *env, int csrno)
 return RISCV_EXCP_ILLEGAL_INST;
 }
 
-static RISCVException epmp(CPURISCVState *env, int csrno)
+static RISCVException have_mseccfg(CPURISCVState *env, int csrno)
 {
 if (riscv_cpu_cfg(env)->epmp) {
 return RISCV_EXCP_NONE;
 }
+if (riscv_cpu_cfg(env)->ext_zkr) {
+return RISCV_EXCP_NONE;
+}
 
 return RISCV_EXCP_ILLEGAL_INST;
 }
@@ -4379,7 +4382,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
 [CSR_VSIPH]   = { "vsiph",   aia_hmode32, NULL, NULL, rmw_vsiph },
 
 /* Physical Memory Protection */
-[CSR_MSECCFG]= { "mseccfg",  epmp, read_mseccfg, write_mseccfg,
+[CSR_MSECCFG]= { "mseccfg",   have_mseccfg, read_mseccfg, 
write_mseccfg,
  .min_priv_ver = PRIV_VERSION_1_11_0   },
 [CSR_PMPCFG0]= { "pmpcfg0",   pmp, read_pmpcfg,  write_pmpcfg  },
 [CSR_PMPCFG1]= { "pmpcfg1",   pmp, read_pmpcfg,  write_pmpcfg  },
-- 
2.40.1

Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"

[Heinrich Schuchardt]

On 4/25/23 12:25, Sunil V L wrote:

Currently, virt machine supports two pflash instances each with
32MB size. However, the first pflash is always assumed to
contain M-mode firmware and reset vector is set to this if
enabled. Hence, for S-mode payloads like EDK2, only one pflash
instance is available for use. This means both code and NV variables
of EDK2 will need to use the same pflash.

The OS distros keep the EDK2 FW code as readonly. When non-volatile
variables also need to share the same pflash, it is not possible
to keep it as readonly since variables need write access.

To resolve this issue, the code and NV variables need to be separated.
But in that case we need an extra flash. Hence, modify the convention
such that pflash0 will contain the M-mode FW only when "-bios none"
option is used. Otherwise, pflash0 will contain the S-mode payload FW.
This enables both pflash instances available for EDK2 use.

Example usage:
1) pflash0 containing M-mode FW
qemu-system-riscv64 -bios none -pflash  -machine virt
or
qemu-system-riscv64 -bios none \
-drive file=,if=pflash,format=raw,unit=0 -machine virt

2) pflash0 containing S-mode payload like EDK2
qemu-system-riscv64 -pflash  -pflash  -machine  
virt
or
qemu-system-riscv64 -bios  \
-pflash  \
-pflash  \


On amd64 and arm64 unit=0 is used for code and unit=1 is used for 
variables. Shouldn't riscv64 do the same?


Best regards

Heinrich


-machine  virt
or
qemu-system-riscv64 -bios  \
-drive file=,if=pflash,format=raw,unit=0 \
-drive file=,if=pflash,format=raw,unit=1,readonly=on  \
-machine virt

Signed-off-by: Sunil V L 
Reported-by: Heinrich Schuchardt 
---
The issue is reported at
https://salsa.debian.org/qemu-team/edk2/-/commit/c345655a0149f64c5020bfc1e53c619ce60587f6

The patch is based on Alistair's riscv-to-apply.next branch.

Changes since v1:
1) Simplified the fix such that it doesn't break current EDK2.

  hw/riscv/virt.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4e3efbee16..ca445d3d02 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1296,10 +1296,11 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
  kernel_entry = 0;
  }
  
-if (drive_get(IF_PFLASH, 0, 0)) {

+if (drive_get(IF_PFLASH, 0, 0) &&
+ machine->firmware && !strcmp(machine->firmware, "none")) {
  /*
- * Pflash was supplied, let's overwrite the address we jump to after
- * reset to the base of the flash.
+ * Pflash0 was supplied with "-bios none", let's overwrite the address
+ * we jump to after reset to the base of the flash.
   */
  start_addr = virt_memmap[VIRT_FLASH].base;
  }

Re: [PATCH] hw/riscv: virt: Enable booting M-mode or S-mode FW from pflash0

[Heinrich Schuchardt]

On 4/21/23 06:33, Sunil V L wrote:

Currently, virt machine supports two pflash instances each with
32MB size. However, the first pflash is always assumed to
contain M-mode firmware and reset vector is set to this if
enabled. Hence, for S-mode payloads like EDK2, only one pflash
instance is available for use. This means both code and NV variables
of EDK2 will need to use the same pflash.

The OS distros keep the EDK2 FW code as readonly. When non-volatile
variables also need to share the same pflash, it is not possible
to keep it as readonly since variables need write access.

To resolve this issue, the code and NV variables need to be separated.
But in that case we need an extra flash. Hence, modify the convention
such that pflash0 will contain the M-mode FW only when "-bios none"
option is used. Otherwise, pflash0 will contain the S-mode payload FW.
This enables both pflash instances available for EDK2 use.

Example usage:
1) pflash0 containing M-mode FW
qemu-system-riscv64 -bios none -pflash  -machine virt
or
qemu-system-riscv64 -bios none \
-drive file=,if=pflash,format=raw,unit=0 -machine virt

2) pflash0 containing S-mode payload like EDK2
qemu-system-riscv64 -pflash  -pflash  -machine  virt
or
qemu-system-riscv64 -bios  \
-pflash  \
-pflash  \
-machine  virt
or
qemu-system-riscv64 -bios  \
-drive file=,if=pflash,format=raw,unit=0,readonly=on \
-drive file=,if=pflash,format=raw,unit=1 \
-machine virt

Signed-off-by: Sunil V L 
Reported-by: Heinrich Schuchardt 


QEMU 7.2 (and possibly 8.0 to be released) contains the old behavior.

Changed use of command line parameters should depend on the version of
the virt machine, i.e. virt-7.2 should use the old behavior and virt as
alias for virt-8.0 should use the new behavior. Please, have a look at
the option handling in hw/arm/virt.c and macro DEFINE_VIRT_MACHINE().

Best regards

Heinrich


---
  hw/riscv/virt.c | 51 ++---
  1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4e3efbee16..1187a60d6e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1245,7 +1245,7 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
  target_ulong firmware_end_addr, kernel_start_addr;
  const char *firmware_name = riscv_default_firmware_name(>soc[0]);
  uint32_t fdt_load_addr;
-uint64_t kernel_entry;
+uint64_t kernel_entry = 0;

  /*
   * Only direct boot kernel is currently supported for KVM VM,
@@ -1266,42 +1266,29 @@ static void virt_machine_done(Notifier *notifier, void 
*data)
  firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
   start_addr, NULL);

-if (drive_get(IF_PFLASH, 0, 1)) {
-/*
- * S-mode FW like EDK2 will be kept in second plash (unit 1).
- * When both kernel, initrd and pflash options are provided in the
- * command line, the kernel and initrd will be copied to the fw_cfg
- * table and opensbi will jump to the flash address which is the
- * entry point of S-mode FW. It is the job of the S-mode FW to load
- * the kernel and initrd using fw_cfg table.
- *
- * If only pflash is given but not -kernel, then it is the job of
- * of the S-mode firmware to locate and load the kernel.
- * In either case, the next_addr for opensbi will be the flash address.
- */
-riscv_setup_firmware_boot(machine);
-kernel_entry = virt_memmap[VIRT_FLASH].base +
-   virt_memmap[VIRT_FLASH].size / 2;
-} else if (machine->kernel_filename) {
+if (drive_get(IF_PFLASH, 0, 0)) {
+if (machine->firmware && !strcmp(machine->firmware, "none")) {
+/*
+ * Pflash was supplied but bios is none, let's overwrite the
+ * address we jump to after reset to the base of the flash.
+ */
+start_addr = virt_memmap[VIRT_FLASH].base;
+} else {
+/*
+ * Pflash was supplied but bios is not none. In this case,
+ * base of the flash would contain S-mode payload.
+ */
+riscv_setup_firmware_boot(machine);
+kernel_entry = virt_memmap[VIRT_FLASH].base;
+}
+}
+
+if (machine->kernel_filename && !kernel_entry) {
  kernel_start_addr = riscv_calc_kernel_start_addr(>soc[0],
   firmware_end_addr);

  kernel_entry = riscv_load_kernel(machine, >soc[0],
   kernel_start_addr, true, NULL);
-} else {
-   /*
-* If dynamic firmware is used, it doesn't know where is the next mode
-* if kernel argument is not set.
-*/
-kernel_entry = 0;
-}
-
-if (drive_get(IF_PFLASH, 0, 0)) {
-/*
- *

[PATCH v2 1/1] docs: pcie: describe PCIe option ROMs

[Heinrich Schuchardt]

Provide a descriptions of the options that control the emulation of option
ROMS for PCIe devices.

Signed-off-by: Heinrich Schuchardt 
---
v2:
correct description of rombar property
use romfile= to suppress option ROM loading
---
 docs/pcie.txt | 28 
 1 file changed, 28 insertions(+)

diff --git a/docs/pcie.txt b/docs/pcie.txt
index 89e3502075..b60f189bd4 100644
--- a/docs/pcie.txt
+++ b/docs/pcie.txt
@@ -292,6 +292,34 @@ PCI-PCI Bridge slots can be used for legacy PCI host 
devices.
 If you can see the "Express Endpoint" capability in the
 output, then the device is indeed PCI Express.
 
+8. Option ROM
+=
+PCIe devices may provide an option ROM. The following properties control the
+emulation of the option ROM:
+
+``rombar`` (default: ``1``)
+  For vfio-pci devices a vendor and product ID based denylist exists which
+  controls if an available option ROM shall be probed. The 'rombar' option
+  allows to override this setting. The value is used as follows:
+  0 = skip probing, 1 = force probing
+
+``romfile``
+  Defines the name of the file to be loaded as option ROM.
+  The file size may neither exceed 2 GiB nor ``romsize``.
+  Some devices like virtio-net-pci define a default file name.
+
+``romsize`` (default: ``-1``)
+  Specifies the size of the option ROM in bytes. The value must be either
+  ``-1`` or a power of two. ``-1`` signifies unlimited size.
+
+Some QEMU PCIe devices like virtio-net-pci use an option ROM by default. In the
+following example the option ROM of a virtio-net-pci device is disabled by
+specifying an empty ``romfile`` property. This is useful for architectures 
where
+QEMU does not supply an option ROM file.
+
+.. code-block:: console
+
+-device virtio-net-pci,netdev=eth1,mq=on,romfile=
 
 7. Virtio devices
 =
-- 
2.36.1

Re: [PATCH 1/1] docs: pcie: describe PCIe option ROMs

[Heinrich Schuchardt]

On 7/20/22 08:42, Heinrich Schuchardt wrote:

Provide a descriptions of the options that control the emulation of option
ROMS for PCIe devices.

Signed-off-by: Heinrich Schuchardt 
---
  docs/pcie.txt | 25 +
  1 file changed, 25 insertions(+)

diff --git a/docs/pcie.txt b/docs/pcie.txt
index 89e3502075..a22c1f69f7 100644
--- a/docs/pcie.txt
+++ b/docs/pcie.txt
@@ -292,6 +292,31 @@ PCI-PCI Bridge slots can be used for legacy PCI host 
devices.
  If you can see the "Express Endpoint" capability in the
  output, then the device is indeed PCI Express.
  
+8. Option ROM

+=
+PCIe devices may provide an option ROM. The following properties control the
+emulation of the option ROM:
+
+``rombar`` (default: ``1``)
+  Specifies that an option ROM is available. If set to ``0``, no option ROM
+  is present.
+
+``romsize`` (default: ``-1``)
+  Specifies the size of the option ROM in bytes. The value must be either
+  ``-1`` or a power of two. ``-1`` signifies unlimited size.
+
+``romfile``
+  Defines the name of the file to be loaded as option ROM.
+  Some devices like virtio-net-pci define a default file name.
+  The file size may neither exceed 2 GiB nor ``romsize``.
+
+Some QEMU PCIe devices like virtio-net-pci use an option ROM by default. In the
+following example the option ROM of a virtio-net-pci device is disabled. This
+is useful for architectures where QEMU does not supply an option ROM file.
+
+.. code-block:: console
+
+-device virtio-net-pci,netdev=eth1,mq=on,rombar=0


If no ROM file exists, this is good enough.

If it does exist and I create multiple virtio-net-pci devices with 
rombar=0, I get an error indicating that the same ROM is used twice:


qemu-system-riscv64: -device virtio-net-pci,netdev=eth1,mq=on,rombar=0: 
duplicate fw_cfg file name: genroms/efi-virtio.rom


I ended up using to solve the problem:

-device virtio-net-pci,netdev=eth1,mq=on,rombar=0,romfile=

I guess some input from the maintainers would be helpful here:

- Why is the file read if rombar=0? Is this a bug?
- Why can't the same option ROM file be used twice? It is read-only.

Best regards

Heinrich

  
  7. Virtio devices

  =

[PATCH 1/1] docs: pcie: describe PCIe option ROMs

[Heinrich Schuchardt]

Provide a descriptions of the options that control the emulation of option
ROMS for PCIe devices.

Signed-off-by: Heinrich Schuchardt 
---
 docs/pcie.txt | 25 +
 1 file changed, 25 insertions(+)

diff --git a/docs/pcie.txt b/docs/pcie.txt
index 89e3502075..a22c1f69f7 100644
--- a/docs/pcie.txt
+++ b/docs/pcie.txt
@@ -292,6 +292,31 @@ PCI-PCI Bridge slots can be used for legacy PCI host 
devices.
 If you can see the "Express Endpoint" capability in the
 output, then the device is indeed PCI Express.
 
+8. Option ROM
+=
+PCIe devices may provide an option ROM. The following properties control the
+emulation of the option ROM:
+
+``rombar`` (default: ``1``)
+  Specifies that an option ROM is available. If set to ``0``, no option ROM
+  is present.
+
+``romsize`` (default: ``-1``)
+  Specifies the size of the option ROM in bytes. The value must be either
+  ``-1`` or a power of two. ``-1`` signifies unlimited size.
+
+``romfile``
+  Defines the name of the file to be loaded as option ROM.
+  Some devices like virtio-net-pci define a default file name.
+  The file size may neither exceed 2 GiB nor ``romsize``.
+
+Some QEMU PCIe devices like virtio-net-pci use an option ROM by default. In the
+following example the option ROM of a virtio-net-pci device is disabled. This
+is useful for architectures where QEMU does not supply an option ROM file.
+
+.. code-block:: console
+
+-device virtio-net-pci,netdev=eth1,mq=on,rombar=0
 
 7. Virtio devices
 =
-- 
2.36.1

[PATCH] hw/arm/virt: impact of gic-version on max CPUs

[Heinrich Schuchardt]

Describe that the gic-version influences the maximum number of CPUs.

Signed-off-by: Heinrich Schuchardt 
---
 docs/system/arm/virt.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 1544632b67..1af3f6a0a8 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -96,9 +96,9 @@ gic-version
   Valid values are:
 
   ``2``
-GICv2
+GICv2 - This limits the number of CPUs to 8.
   ``3``
-GICv3
+GICv3 - This allows up to 512 CPUs.
   ``host``
 Use the same GIC version the host provides, when using KVM
   ``max``
-- 
2.34.1

Re: [PATCH] hw/arm: add control knob to disable kaslr_seed via DTB

[Heinrich Schuchardt]

On 12/15/21 13:09, Alex Bennée wrote:

Generally a guest needs an external source of randomness to properly
enable things like address space randomisation. However in a trusted
boot environment where the firmware will cryptographically verify
components having random data in the DTB will cause verification to
fail. Add a control knob so we can prevent this being added to the
system DTB.

Signed-off-by: Alex Bennée
Cc: Ilias Apalodimas
Cc: Jerome Forissier
---
  docs/system/arm/virt.rst |  7 +++
  include/hw/arm/virt.h|  1 +
  hw/arm/virt.c| 32 ++--
  3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 850787495b..c86a4808df 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -121,6 +121,13 @@ ras
Set ``on``/``off`` to enable/disable reporting host memory errors to a guest
using ACPI and guest external abort exceptions. The default is off.



Tested on top of QEMU v6.1.0

Tested-by: Heinrich Schuchardt 

Re: [PATCH 00/16] fdt: Make OF_BOARD a boolean option

[Heinrich Schuchardt]

On 10/27/21 16:55, Tom Rini wrote:

On Wed, Oct 27, 2021 at 03:23:01PM +0200, Heinrich Schuchardt wrote:

[snip]

One passed to U-Boot for fixups and further passed to the OS. This
devicetree may originate from a prior boot stage, from a file loaded by
U-Boot, or from a later bootstage, e.g systemd-boot's devicetree command.


I assume systemd-boot is implementing the same logic that extlinux.conf
has used for forever, yes?


It is loading the file and then calls U-Boot's implementation of the EFI 
Device Tree Fixup Protocol for fixups before passing the device-tree to 
the OS.





This devicetree will not contain any U-Boot specific information.


To repeat, it must only have official bindings, yes, regardless of what
project they come from.



Don't expect prior firmware stages to provide any U-Boot specific stuff 
whatever official or non-official U-Boot specific bindings exist.


Best regards

Heinrich

Re: [PATCH 00/16] fdt: Make OF_BOARD a boolean option

[Heinrich Schuchardt]

On 10/27/21 15:15, François Ozog wrote:

Hi,

On Wed, 27 Oct 2021 at 14:48, Tom Rini > wrote:


On Fri, Oct 15, 2021 at 12:03:44PM -0600, Simon Glass wrote:
 > Hi all,
 >
 > On Thu, 14 Oct 2021 at 09:28, Tom Rini mailto:tr...@konsulko.com>> wrote:
 > >
 > > On Thu, Oct 14, 2021 at 09:17:52AM -0600, Simon Glass wrote:
 > > > Hi Tom,
 > > >
 > > > On Thu, 14 Oct 2021 at 08:56, Tom Rini mailto:tr...@konsulko.com>> wrote:
 > > > >
 > > > > On Wed, Oct 13, 2021 at 12:06:02PM -0600, Simon Glass wrote:
 > > > > > Hi François,
 > > > > >
 > > > > > On Wed, 13 Oct 2021 at 11:35, François Ozog
mailto:francois.o...@linaro.org>> wrote:
 > > > > > >
 > > > > > > Hi Simon
 > > > > > >
 > > > > > > Le mer. 13 oct. 2021 à 16:49, Simon Glass
mailto:s...@chromium.org>> a écrit :
 > > > > > >>
 > > > > > >> Hi Tom, Bin,François,
 > > > > > >>
 > > > > > >> On Tue, 12 Oct 2021 at 19:34, Tom Rini
mailto:tr...@konsulko.com>> wrote:
 > > > > > >> >
 > > > > > >> > On Wed, Oct 13, 2021 at 09:29:14AM +0800, Bin Meng
wrote:
 > > > > > >> > > Hi Simon,
 > > > > > >> > >
 > > > > > >> > > On Wed, Oct 13, 2021 at 9:01 AM Simon Glass
mailto:s...@chromium.org>> wrote:
 > > > > > >> > > >
 > > > > > >> > > > With Ilias' efforts we have dropped
OF_PRIOR_STAGE and OF_HOSTFILE so
 > > > > > >> > > > there are only three ways to obtain a devicetree:
 > > > > > >> > > >
 > > > > > >> > > >    - OF_SEPARATE - the normal way, where the
devicetree is built and
 > > > > > >> > > >       appended to U-Boot
 > > > > > >> > > >    - OF_EMBED - for development purposes, the
devicetree is embedded in
 > > > > > >> > > >       the ELF file (also used for EFI)
 > > > > > >> > > >    - OF_BOARD - the board figures it out on its own
 > > > > > >> > > >
 > > > > > >> > > > The last one is currently set up so that no
devicetree is needed at all
 > > > > > >> > > > in the U-Boot tree. Most boards do provide one,
but some don't. Some
 > > > > > >> > > > don't even provide instructions on how to boot
on the board.
 > > > > > >> > > >
 > > > > > >> > > > The problems with this approach are documented
at [1].
 > > > > > >> > > >
 > > > > > >> > > > In practice, OF_BOARD is not really distinct
from OF_SEPARATE. Any board
 > > > > > >> > > > can obtain its devicetree at runtime, even it is
has a devicetree built
 > > > > > >> > > > in U-Boot. This is because U-Boot may be a
second-stage bootloader and its
 > > > > > >> > > > caller may have a better idea about the hardware
available in the machine.
 > > > > > >> > > > This is the case with a few QEMU boards, for
example.
 > > > > > >> > > >
 > > > > > >> > > > So it makes no sense to have OF_BOARD as a
'choice'. It should be an
 > > > > > >> > > > option, available with either OF_SEPARATE or
OF_EMBED.
 > > > > > >> > > >
 > > > > > >> > > > This series makes this change, adding various
missing devicetree files
 > > > > > >> > > > (and placeholders) to make the build work.
 > > > > > >> > >
 > > > > > >> > > Adding device trees that are never used sounds
like a hack to me.
 > > > > > >> > >
 > > > > > >> > > For QEMU, device tree is dynamically generated on
the fly based on
 > > > > > >> > > command line parameters, and the device tree you
put in this series
 > > > > > >> > > has various hardcoded  values which
normally do not show up
 > > > > > >> > > in hand-written dts files.
 > > > > > >> > >
 > > > > > >> > > I am not sure I understand the whole point of this.
 > > > > > >> >
 > > > > > >> > I am also confused and do not like the idea of
adding device trees for
 > > > > > >> > platforms that are capable of and can / do have a
device tree to give us
 > > > > > >> > at run time.
 > > > > > >>
 > > > > > >> (I'll just reply to this one email, since the same
points applies to
 > > > > > >> all replies I think)
 > > > > > >>
 > > > > > >> I have been thinking about this and discussing it with
people for a
 > > > > > >> few months now. I've been signalling a change like
this for over a
 > > > > > >> month now, on U-Boot contributor calls and in
discussions with Linaro
 > > > > > >> people. I sent a patch (below) to try to explain
things. I hope it is
 > > > > > >> not a surprise!
 > > > > > >>
 > > > > > >> The issue here is that we need a devicetree in-tree in
U-Boot, to
 > > > > > >> avoid the mess that has been created by
OF_PRIOR_STAGE, OF_BOARD,
 > > > > > >> BINMAN_STANDALONE_FDT and to a lesser extent,
OF_HOSTFILE. Between
 > > > > > >> Ilias' series and this one we can get ourselves on a
stronger footing.
 > > > > > >> 

Re: [PATCH 00/16] fdt: Make OF_BOARD a boolean option

[Heinrich Schuchardt]

On 10/13/21 03:01, Simon Glass wrote:

With Ilias' efforts we have dropped OF_PRIOR_STAGE and OF_HOSTFILE so
there are only three ways to obtain a devicetree:

- OF_SEPARATE - the normal way, where the devicetree is built and
   appended to U-Boot
- OF_EMBED - for development purposes, the devicetree is embedded in
   the ELF file (also used for EFI)
- OF_BOARD - the board figures it out on its own

The last one is currently set up so that no devicetree is needed at all
in the U-Boot tree. Most boards do provide one, but some don't. Some
don't even provide instructions on how to boot on the board.

The problems with this approach are documented at [1].

In practice, OF_BOARD is not really distinct from OF_SEPARATE. Any board
can obtain its devicetree at runtime, even it is has a devicetree built
in U-Boot. This is because U-Boot may be a second-stage bootloader and its
caller may have a better idea about the hardware available in the machine.
This is the case with a few QEMU boards, for example.

So it makes no sense to have OF_BOARD as a 'choice'. It should be an
option, available with either OF_SEPARATE or OF_EMBED.

This series makes this change, adding various missing devicetree files
(and placeholders) to make the build work.


Why should we add dummy files with irrelevant content (see patch 06/16) 
to make the build work? Why don't you fix the Makefile instead?


Best regards

Heinrich



It also provides a few qemu clean-ups discovered along the way.

This series is based on Ilias' two series for OF_HOSTFILE and
OF_PRIOR_STAGE removal.

It is available at u-boot-dm/ofb-working

[1] 
https://patchwork.ozlabs.org/project/uboot/patch/20210919215111.3830278-3-...@chromium.org/


Simon Glass (16):
   arm: qemu: Mention -nographic in the docs
   arm: qemu: Explain how to extract the generate devicetree
   riscv: qemu: Explain how to extract the generate devicetree
   arm: qemu: Add a devicetree file for qemu_arm
   arm: qemu: Add a devicetree file for qemu_arm64
   riscv: qemu: Add devicetree files for qemu_riscv32/64
   arm: rpi: Add a devicetree file for rpi_4
   arm: vexpress: Add a devicetree file for juno
   arm: xenguest_arm64: Add a fake devicetree file
   arm: octeontx: Add a fake devicetree file
   arm: xilinx_versal_virt: Add a devicetree file
   arm: bcm7xxx: Add a devicetree file
   arm: qemu-ppce500: Add a devicetree file
   arm: highbank: Add a fake devicetree file
   fdt: Make OF_BOARD a bool option
   Drop CONFIG_BINMAN_STANDALONE_FDT

  Makefile   |3 +-
  arch/arm/dts/Makefile  |   20 +-
  arch/arm/dts/bcm2711-rpi-4-b.dts   | 1958 
  arch/arm/dts/bcm7xxx.dts   |   15 +
  arch/arm/dts/highbank.dts  |   14 +
  arch/arm/dts/juno-r2.dts   | 1038 +
  arch/arm/dts/octeontx.dts  |   14 +
  arch/arm/dts/qemu-arm.dts  |  402 +
  arch/arm/dts/qemu-arm64.dts|  381 +
  arch/arm/dts/xenguest-arm64.dts|   15 +
  arch/arm/dts/xilinx-versal-virt.dts|  307 
  arch/powerpc/dts/Makefile  |1 +
  arch/powerpc/dts/qemu-ppce500.dts  |  264 
  arch/riscv/dts/Makefile|2 +-
  arch/riscv/dts/qemu-virt.dts   |8 -
  arch/riscv/dts/qemu-virt32.dts |  217 +++
  arch/riscv/dts/qemu-virt64.dts |  217 +++
  configs/bcm7260_defconfig  |1 +
  configs/bcm7445_defconfig  |1 +
  configs/highbank_defconfig |2 +-
  configs/octeontx2_95xx_defconfig   |1 +
  configs/octeontx2_96xx_defconfig   |1 +
  configs/octeontx_81xx_defconfig|1 +
  configs/octeontx_83xx_defconfig|1 +
  configs/qemu-ppce500_defconfig |2 +
  configs/qemu-riscv32_defconfig |1 +
  configs/qemu-riscv32_smode_defconfig   |1 +
  configs/qemu-riscv32_spl_defconfig |4 +-
  configs/qemu-riscv64_defconfig |1 +
  configs/qemu-riscv64_smode_defconfig   |1 +
  configs/qemu-riscv64_spl_defconfig |3 +-
  configs/qemu_arm64_defconfig   |1 +
  configs/qemu_arm_defconfig |1 +
  configs/rpi_4_32b_defconfig|1 +
  configs/rpi_4_defconfig|1 +
  configs/rpi_arm64_defconfig|1 +
  configs/vexpress_aemv8a_juno_defconfig |1 +
  configs/xenguest_arm64_defconfig   |1 +
  configs/xilinx_versal_virt_defconfig   |1 +
  doc/board/emulation/qemu-arm.rst   |   19 +-
  doc/board/emulation/qemu-riscv.rst |   12 +
  dts/Kconfig|   27 +-
  tools/binman/binman.rst|   20 -
  43 files changed, 4922 insertions(+), 61 deletions(-)
  create mode 100644 arch/arm/dts/bcm2711-rpi-4-b.dts
  create mode 100644 arch/arm/dts/bcm7xxx.dts
  create mode 100644 arch/arm/dts/highbank.dts
  create mode 100644 arch/arm/dts/juno-r2.dts
  create mode 

Re: [PATCH 06/16] riscv: qemu: Add devicetree files for qemu_riscv32/64

[Heinrich Schuchardt]

On 10/13/21 03:01, Simon Glass wrote:

Add these files, generated from qemu, so there is a reference devicetree
in the U-Boot tree.

Split the existing qemu-virt into two, since we need a different
devicetree for 32- and 64-bit machines.



You only sent patch 6/16 and 15/16 to me. No clue why. Please, send 
complete patchsets instead of selected patches which cannot be reviewed 
without the context.


Which devices exist depends on the QEMU comannd line.

The files you create here do neither reflect the superset of all QEMU 
settings nor the minimum set. They do not include all devices supported 
on QEMU by U-Boot either.


You cannot assume that the values in this patch will match values used 
by the next invocation of QEMU.


Hence it is totally unclear what this patch might be good for.

Best regards

Heinrich


Signed-off-by: Simon Glass 
---

  arch/riscv/dts/Makefile  |   2 +-
  arch/riscv/dts/qemu-virt.dts |   8 -
  arch/riscv/dts/qemu-virt32.dts   | 217 +++
  arch/riscv/dts/qemu-virt64.dts   | 217 +++
  configs/qemu-riscv32_defconfig   |   1 +
  configs/qemu-riscv32_smode_defconfig |   1 +
  configs/qemu-riscv32_spl_defconfig   |   2 +-
  configs/qemu-riscv64_defconfig   |   1 +
  configs/qemu-riscv64_smode_defconfig |   1 +
  configs/qemu-riscv64_spl_defconfig   |   2 +-
  10 files changed, 441 insertions(+), 11 deletions(-)
  delete mode 100644 arch/riscv/dts/qemu-virt.dts
  create mode 100644 arch/riscv/dts/qemu-virt32.dts
  create mode 100644 arch/riscv/dts/qemu-virt64.dts

diff --git a/arch/riscv/dts/Makefile b/arch/riscv/dts/Makefile
index b6e9166767b..90d3f35e6e3 100644
--- a/arch/riscv/dts/Makefile
+++ b/arch/riscv/dts/Makefile
@@ -2,7 +2,7 @@
  
  dtb-$(CONFIG_TARGET_AX25_AE350) += ae350_32.dtb ae350_64.dtb

  dtb-$(CONFIG_TARGET_MICROCHIP_ICICLE) += microchip-mpfs-icicle-kit.dtb
-dtb-$(CONFIG_TARGET_QEMU_VIRT) += qemu-virt.dtb
+dtb-$(CONFIG_TARGET_QEMU_VIRT) += qemu-virt32.dtb qemu-virt64.dtb
  dtb-$(CONFIG_TARGET_OPENPITON_RISCV64) += openpiton-riscv64.dtb
  dtb-$(CONFIG_TARGET_SIFIVE_UNLEASHED) += hifive-unleashed-a00.dtb
  dtb-$(CONFIG_TARGET_SIFIVE_UNMATCHED) += hifive-unmatched-a00.dtb
diff --git a/arch/riscv/dts/qemu-virt.dts b/arch/riscv/dts/qemu-virt.dts
deleted file mode 100644
index fecff542b91..000
--- a/arch/riscv/dts/qemu-virt.dts
+++ /dev/null
@@ -1,8 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Copyright (C) 2021, Bin Meng 
- */
-
-/dts-v1/;
-
-#include "binman.dtsi"
diff --git a/arch/riscv/dts/qemu-virt32.dts b/arch/riscv/dts/qemu-virt32.dts
new file mode 100644
index 000..3c449413523
--- /dev/null
+++ b/arch/riscv/dts/qemu-virt32.dts
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021, Bin Meng 
+ */
+
+/dts-v1/;
+
+#include "binman.dtsi"
+
+/ {
+   #address-cells = <0x02>;
+   #size-cells = <0x02>;
+   compatible = "riscv-virtio";
+   model = "riscv-virtio,qemu";
+
+   fw-cfg@1010 {
+   dma-coherent;
+   reg = <0x00 0x1010 0x00 0x18>;
+   compatible = "qemu,fw-cfg-mmio";
+   };
+
+   flash@2000 {
+   bank-width = <0x04>;
+   reg = <0x00 0x2000 0x00 0x200
+   0x00 0x2200 0x00 0x200>;
+   compatible = "cfi-flash";
+   };
+
+   chosen {
+   bootargs = [00];
+   stdout-path = "/soc/uart@1000";
+   };
+
+   memory@8000 {
+   device_type = "memory";
+   reg = <0x00 0x8000 0x00 0x800>;
+   };
+
+   cpus {
+   #address-cells = <0x01>;
+   #size-cells = <0x00>;
+   timebase-frequency = <0x989680>;
+
+   cpu@0 {
+   phandle = <0x01>;
+   device_type = "cpu";
+   reg = <0x00>;
+   status = "okay";
+   compatible = "riscv";
+   riscv,isa = "rv32imafdcsu";
+   mmu-type = "riscv,sv32";
+
+   interrupt-controller {
+   #interrupt-cells = <0x01>;
+   interrupt-controller;
+   compatible = "riscv,cpu-intc";
+   phandle = <0x02>;
+   };
+   };
+
+   cpu-map {
+
+   cluster0 {
+
+   core0 {
+   cpu = <0x01>;
+   };
+   };
+   };
+   };
+
+   soc {
+   #address-cells = <0x02>;
+   #size-cells = <0x02>;
+   compatible = "simple-bus";
+   ranges;
+
+   rtc@101000 {
+   interrupts = <0x0b>;
+   interrupt-parent = 

[PATCH 1/1] util/uri: do not check argument of uri_free()

[Heinrich Schuchardt]

uri_free() checks if its argument is NULL in uri_clean() and g_free().
There is no need to check the argument before the call.

Signed-off-by: Heinrich Schuchardt 
---
 block/nfs.c |  4 +---
 block/ssh.c |  4 +---
 capstone|  2 +-
 util/uri.c  | 22 ++
 4 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 7dff64f489..9aeaefb364 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -147,9 +147,7 @@ out:
 if (qp) {
 query_params_free(qp);
 }
-if (uri) {
-uri_free(uri);
-}
+uri_free(uri);
 return ret;
 }

diff --git a/block/ssh.c b/block/ssh.c
index b51a031620..9f88480ae8 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -237,9 +237,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 return 0;

  err:
-if (uri) {
-  uri_free(uri);
-}
+uri_free(uri);
 return -EINVAL;
 }

diff --git a/capstone b/capstone
index f8b1b83301..31254f17e3 16
--- a/capstone
+++ b/capstone
@@ -1 +1 @@
-Subproject commit f8b1b833015a4ae47110ed068e0deb7106ced66d
+Subproject commit 31254f17e3c6025f3465d1c974a8c96e338ddbe0
diff --git a/util/uri.c b/util/uri.c
index 8bdef84120..ff72c6005f 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -1340,7 +1340,7 @@ static void uri_clean(URI *uri)

 /**
  * uri_free:
- * @uri:  pointer to an URI
+ * @uri:  pointer to an URI, NULL is ignored
  *
  * Free up the URI struct
  */
@@ -1939,15 +1939,9 @@ step_7:
 val = uri_to_string(res);

 done:
-if (ref != NULL) {
-uri_free(ref);
-}
-if (bas != NULL) {
-uri_free(bas);
-}
-if (res != NULL) {
-uri_free(res);
-}
+uri_free(ref);
+uri_free(bas);
+uri_free(res);
 return val;
 }

@@ -2190,12 +2184,8 @@ done:
 if (remove_path != 0) {
 ref->path = NULL;
 }
-if (ref != NULL) {
-uri_free(ref);
-}
-if (bas != NULL) {
-uri_free(bas);
-}
+uri_free(ref);
+uri_free(bas);

 return val;
 }
--
2.30.2

[PATCH v3 1/2] hw/nvme: namespace parameter for EUI-64

[Heinrich Schuchardt]

The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI-64.

Signed-off-by: Heinrich Schuchardt 
Acked-by: Klaus Jensen 
---
v3:
no change
v2:
fix typo %s/EUI64/EUI-64/
---
 docs/system/nvme.rst |  4 +++
 hw/nvme/ctrl.c   | 58 ++--
 hw/nvme/ns.c |  2 ++
 hw/nvme/nvme.h   |  1 +
 4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index f7f63d6bf6..b5f8288d7c 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -81,6 +81,10 @@ There are a number of parameters available:
   Set the UUID of the namespace. This will be reported as a "Namespace UUID"
   descriptor in the Namespace Identification Descriptor List.

+``eui64``
+  Set the EUI-64 of the namespace. This will be reported as a "IEEE Extended
+  Unique Identifier" descriptor in the Namespace Identification Descriptor 
List.
+
 ``bus``
   If there are more ``nvme`` devices defined, this parameter may be used to
   attach the namespace to a specific ``nvme`` device (identified by an ``id``
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8dd9cb2ccb..f37c4fd635 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4436,19 +4436,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
 NvmeIdentify *c = (NvmeIdentify *)>cmd;
 uint32_t nsid = le32_to_cpu(c->nsid);
 uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
-
-struct data {
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v[NVME_NIDL_UUID];
-} uuid;
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v;
-} csi;
-};
-
-struct data *ns_descrs = (struct data *)list;
+uint8_t *pos = list;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v[NVME_NIDL_UUID];
+} QEMU_PACKED uuid;
+struct {
+NvmeIdNsDescr hdr;
+uint64_t v;
+} QEMU_PACKED eui64;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v;
+} QEMU_PACKED csi;

 trace_pci_nvme_identify_ns_descr_list(nsid);

@@ -4462,17 +4462,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
 }

 /*
- * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
- * structure, a Namespace UUID (nidt = 3h) must be reported in the
- * Namespace Identification Descriptor. Add the namespace UUID here.
+ * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must
+ * provide a valid Namespace UUID in the Namespace Identification 
Descriptor
+ * data structure. QEMU does not yet support setting NGUID.
  */
-ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
-ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
-memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
-
-ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
-ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
-ns_descrs->csi.v = ns->csi;
+uuid.hdr.nidt = NVME_NIDT_UUID;
+uuid.hdr.nidl = NVME_NIDL_UUID;
+memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
+memcpy(pos, , sizeof(uuid));
+pos += sizeof(uuid);
+
+if (ns->params.eui64) {
+eui64.hdr.nidt = NVME_NIDT_EUI64;
+eui64.hdr.nidl = NVME_NIDL_EUI64;
+eui64.v = cpu_to_be64(ns->params.eui64);
+memcpy(pos, , sizeof(eui64));
+pos += sizeof(eui64);
+}
+
+csi.hdr.nidt = NVME_NIDT_CSI;
+csi.hdr.nidl = NVME_NIDL_CSI;
+csi.v = ns->csi;
+memcpy(pos, , sizeof(csi));
+pos += sizeof(csi);

 return nvme_c2h(n, list, sizeof(list), req);
 }
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 3fec9c6273..45e457de6a 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
 id_ns->mcl = cpu_to_le32(ns->params.mcl);
 id_ns->msrc = ns->params.msrc;
+id_ns->eui64 = cpu_to_be64(ns->params.eui64);

 ds = 31 - clz32(ns->blkconf.logical_block_size);
 ms = ns->params.ms;
@@ -511,6 +512,7 @@ static Property nvme_ns_props[] = {
 DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
 DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
 DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
+DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
 DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0),
 DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0),
 DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 93a7e0e538..ac90e13d7b 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -83,6 +83,7 @@ typedef struct NvmeNamespaceParams {
 bool shared;
 uint32_t nsid;
 QemuUUID uuid;
+uint64_t eui64;

 uint16_t ms;
 uint8_t  mset;
--
2.30.2

[PATCH v3 2/2] hw/nvme: default for namespace EUI-64

[Heinrich Schuchardt]

On machines with version > 6.0 replace a missing EUI-64 by a generated
value.

Signed-off-by: Heinrich Schuchardt 
---
v3:
use 52-54-00-00-00-00-00-00 as starting values for generating
EUI-64s
v2:
new patch
---
 docs/system/nvme.rst | 2 ++
 hw/core/machine.c| 1 +
 hw/nvme/ns.c | 9 +
 hw/nvme/nvme.h   | 2 ++
 4 files changed, 14 insertions(+)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index b5f8288d7c..33a15c7dbc 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -84,6 +84,8 @@ There are a number of parameters available:
 ``eui64``
   Set the EUI-64 of the namespace. This will be reported as a "IEEE Extended
   Unique Identifier" descriptor in the Namespace Identification Descriptor 
List.
+  Since machine type 6.1 a non-zero default value is used if the parameter
+  is not provided. For earlier machine types the field defaults to 0.

 ``bus``
   If there are more ``nvme`` devices defined, this parameter may be used to
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55b9bc7817..d0e934 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@
 GlobalProperty hw_compat_6_0[] = {
 { "gpex-pcihost", "allow-unmapped-accesses", "false" },
 { "i8042", "extended-state", "false"},
+{ "nvme-ns", "eui64-default", "off"},
 };
 const size_t hw_compat_6_0_len = G_N_ELEMENTS(hw_compat_6_0);

diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 45e457de6a..4275c3db63 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -56,6 +56,7 @@ void nvme_ns_init_format(NvmeNamespace *ns)

 static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 {
+static uint64_t ns_count;
 NvmeIdNs *id_ns = >id_ns;
 uint8_t ds;
 uint16_t ms;
@@ -73,6 +74,12 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 id_ns->nmic |= NVME_NMIC_NS_SHARED;
 }

+/* Substitute a missing EUI-64 by an autogenerated one */
+++ns_count;
+if (!ns->params.eui64 && ns->params.eui64_default) {
+ns->params.eui64 = ns_count + NVME_EUI64_DEFAULT;
+}
+
 /* simple copy */
 id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
 id_ns->mcl = cpu_to_le32(ns->params.mcl);
@@ -533,6 +540,8 @@ static Property nvme_ns_props[] = {
params.max_open_zones, 0),
 DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
params.zd_extension_size, 0),
+DEFINE_PROP_BOOL("eui64-default", NvmeNamespace, params.eui64_default,
+ true),
 DEFINE_PROP_END_OF_LIST(),
 };

diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index ac90e13d7b..371ac9bfd8 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -26,6 +26,7 @@

 #define NVME_MAX_CONTROLLERS 32
 #define NVME_MAX_NAMESPACES  256
+#define NVME_EUI64_DEFAULT ((uint64_t)0x5254)

 typedef struct NvmeCtrl NvmeCtrl;
 typedef struct NvmeNamespace NvmeNamespace;
@@ -84,6 +85,7 @@ typedef struct NvmeNamespaceParams {
 uint32_t nsid;
 QemuUUID uuid;
 uint64_t eui64;
+bool eui64_default;

 uint16_t ms;
 uint8_t  mset;
--
2.30.2

[PATCH v3 0/2] hw/nvme: namespace parameter for EUI-64

[Heinrich Schuchardt]

The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI-64.

v3:
use 52-54-00-00-00-00-00-00 as starting values for generating
    EUI-64s

Heinrich Schuchardt (2):
  hw/nvme: namespace parameter for EUI-64
  hw/nvme: default for namespace EUI-64

 docs/system/nvme.rst |  6 +
 hw/core/machine.c|  1 +
 hw/nvme/ctrl.c   | 58 ++--
 hw/nvme/ns.c | 11 +
 hw/nvme/nvme.h   |  3 +++
 5 files changed, 56 insertions(+), 23 deletions(-)

--
2.30.2

Re: [PATCH v2 3/3] hw/nvme: default for namespace EUI-64

[Heinrich Schuchardt]

On 6/14/21 9:23 PM, Heinrich Schuchardt wrote:

On 6/14/21 8:31 PM, Klaus Jensen wrote:

On Jun 12 01:46, Heinrich Schuchardt wrote:

On machines with version > 6.0 replace a missing EUI-64 by a generated
value.

Signed-off-by: Heinrich Schuchardt 
---
v2:
new patch
---
docs/system/nvme.rst | 2 ++
hw/core/machine.c    | 1 +
hw/nvme/ns.c | 9 +
hw/nvme/nvme.h   | 2 ++
4 files changed, 14 insertions(+)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index b5f8288d7c..33a15c7dbc 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -84,6 +84,8 @@ There are a number of parameters available:
``eui64``
  Set the EUI-64 of the namespace. This will be reported as a "IEEE
Extended
  Unique Identifier" descriptor in the Namespace Identification
Descriptor List.
+  Since machine type 6.1 a non-zero default value is used if the
parameter
+  is not provided. For earlier machine types the field defaults to 0.

``bus``
  If there are more ``nvme`` devices defined, this parameter may be
used to
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55b9bc7817..d0e934 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@
GlobalProperty hw_compat_6_0[] = {
    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
    { "i8042", "extended-state", "false"},
+    { "nvme-ns", "eui64-default", "off"},
};
const size_t hw_compat_6_0_len = G_N_ELEMENTS(hw_compat_6_0);

diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 45e457de6a..4275c3db63 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -56,6 +56,7 @@ void nvme_ns_init_format(NvmeNamespace *ns)

static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
{
+    static uint64_t ns_count;
    NvmeIdNs *id_ns = >id_ns;
    uint8_t ds;
    uint16_t ms;
@@ -73,6 +74,12 @@ static int nvme_ns_init(NvmeNamespace *ns, Error
**errp)
    id_ns->nmic |= NVME_NMIC_NS_SHARED;
    }

+    /* Substitute a missing EUI-64 by an autogenerated one */
+    ++ns_count;
+    if (!ns->params.eui64 && ns->params.eui64_default) {
+    ns->params.eui64 = ns_count + NVME_EUI64_DEFAULT;
+    }
+
    /* simple copy */
    id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
    id_ns->mcl = cpu_to_le32(ns->params.mcl);
@@ -533,6 +540,8 @@ static Property nvme_ns_props[] = {
   params.max_open_zones, 0),
    DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
   params.zd_extension_size, 0),
+    DEFINE_PROP_BOOL("eui64-default", NvmeNamespace,
params.eui64_default,
+ true),
    DEFINE_PROP_END_OF_LIST(),
};

diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index ac90e13d7b..3fb869731d 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -26,6 +26,7 @@

#define NVME_MAX_CONTROLLERS 32
#define NVME_MAX_NAMESPACES  256
+#define NVME_EUI64_DEFAULT 0x27fed9272381cbd0UL


Is there any significance to this value? Any particular reason it is not
using the QEMU OUI (52:54:00)?


52:54:00 does not exist in http://standards-oui.ieee.org/oui/oui.txt
So this seems not to be an OUI. We should not use a number that IEEE may
sell to some other entity.

Until somebody buys a 36bit OUI in the name of QEMU for 780 USD we have
to use a locally assigned number.


I got that wrong IEEE_COMPANY_LOCALLY_ASSIGNED 0x525400 is locally
assigned and therefore not in the OUI list.

I will resubmit the patch.

Best regards

Heinrich



Best regards

Heinrich





typedef struct NvmeCtrl NvmeCtrl;
typedef struct NvmeNamespace NvmeNamespace;
@@ -84,6 +85,7 @@ typedef struct NvmeNamespaceParams {
    uint32_t nsid;
    QemuUUID uuid;
    uint64_t eui64;
+    bool eui64_default;

    uint16_t ms;
    uint8_t  mset;
--
2.30.2

Re: [PATCH v2 3/3] hw/nvme: default for namespace EUI-64

[Heinrich Schuchardt]

On 6/14/21 8:31 PM, Klaus Jensen wrote:

On Jun 12 01:46, Heinrich Schuchardt wrote:

On machines with version > 6.0 replace a missing EUI-64 by a generated
value.

Signed-off-by: Heinrich Schuchardt 
---
v2:
new patch
---
docs/system/nvme.rst | 2 ++
hw/core/machine.c    | 1 +
hw/nvme/ns.c | 9 +
hw/nvme/nvme.h   | 2 ++
4 files changed, 14 insertions(+)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index b5f8288d7c..33a15c7dbc 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -84,6 +84,8 @@ There are a number of parameters available:
``eui64``
  Set the EUI-64 of the namespace. This will be reported as a "IEEE
Extended
  Unique Identifier" descriptor in the Namespace Identification
Descriptor List.
+  Since machine type 6.1 a non-zero default value is used if the
parameter
+  is not provided. For earlier machine types the field defaults to 0.

``bus``
  If there are more ``nvme`` devices defined, this parameter may be
used to
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55b9bc7817..d0e934 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@
GlobalProperty hw_compat_6_0[] = {
    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
    { "i8042", "extended-state", "false"},
+    { "nvme-ns", "eui64-default", "off"},
};
const size_t hw_compat_6_0_len = G_N_ELEMENTS(hw_compat_6_0);

diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 45e457de6a..4275c3db63 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -56,6 +56,7 @@ void nvme_ns_init_format(NvmeNamespace *ns)

static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
{
+    static uint64_t ns_count;
    NvmeIdNs *id_ns = >id_ns;
    uint8_t ds;
    uint16_t ms;
@@ -73,6 +74,12 @@ static int nvme_ns_init(NvmeNamespace *ns, Error
**errp)
    id_ns->nmic |= NVME_NMIC_NS_SHARED;
    }

+    /* Substitute a missing EUI-64 by an autogenerated one */
+    ++ns_count;
+    if (!ns->params.eui64 && ns->params.eui64_default) {
+    ns->params.eui64 = ns_count + NVME_EUI64_DEFAULT;
+    }
+
    /* simple copy */
    id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
    id_ns->mcl = cpu_to_le32(ns->params.mcl);
@@ -533,6 +540,8 @@ static Property nvme_ns_props[] = {
   params.max_open_zones, 0),
    DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
   params.zd_extension_size, 0),
+    DEFINE_PROP_BOOL("eui64-default", NvmeNamespace,
params.eui64_default,
+ true),
    DEFINE_PROP_END_OF_LIST(),
};

diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index ac90e13d7b..3fb869731d 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -26,6 +26,7 @@

#define NVME_MAX_CONTROLLERS 32
#define NVME_MAX_NAMESPACES  256
+#define NVME_EUI64_DEFAULT 0x27fed9272381cbd0UL


Is there any significance to this value? Any particular reason it is not
using the QEMU OUI (52:54:00)?


52:54:00 does not exist in http://standards-oui.ieee.org/oui/oui.txt
So this seems not to be an OUI. We should not use a number that IEEE may
sell to some other entity.

Until somebody buys a 36bit OUI in the name of QEMU for 780 USD we have
to use a locally assigned number.

Best regards

Heinrich





typedef struct NvmeCtrl NvmeCtrl;
typedef struct NvmeNamespace NvmeNamespace;
@@ -84,6 +85,7 @@ typedef struct NvmeNamespaceParams {
    uint32_t nsid;
    QemuUUID uuid;
    uint64_t eui64;
+    bool eui64_default;

    uint16_t ms;
    uint8_t  mset;
--
2.30.2

Re: [PATCH v2 1/3] hw: virt: consider hw_compat_6_0

[Heinrich Schuchardt]

On 6/12/21 1:46 AM, Heinrich Schuchardt wrote:

virt-6.0 must consider hw_compat_6_0.

Fixes: da7e13c00b59 ("hw: add compat machines for 6.1")
Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Cornelia Huck 


This patch was applied to target-arm.next by Peter.


---
v2:
add missing Fixes: tag
---
  hw/arm/virt.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 840758666d..8bc3b408fe 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2764,6 +2764,8 @@ DEFINE_VIRT_MACHINE_AS_LATEST(6, 1)

  static void virt_machine_6_0_options(MachineClass *mc)
  {
+virt_machine_6_1_options(mc);
+compat_props_add(mc->compat_props, hw_compat_6_0, hw_compat_6_0_len);
  }
  DEFINE_VIRT_MACHINE(6, 0)

--
2.30.2

Re: [PATCH 1/1] hw: virt: consider hw_compat_6_0

[Heinrich Schuchardt]

On 6/14/21 5:35 PM, Peter Maydell wrote:

On Fri, 11 Jun 2021 at 08:32, Cornelia Huck  wrote:


On Thu, Jun 10 2021, Heinrich Schuchardt  wrote:


virt-6.0 must consider hw_compat_6_0.

Signed-off-by: Heinrich Schuchardt 
---
  hw/arm/virt.c | 2 ++
  1 file changed, 2 insertions(+)


Oops, forgot that hunk.

Fixes: da7e13c00b59 ("hw: add compat machines for 6.1")

Reviewed-by: Cornelia Huck 




Applied to target-arm.next, thanks.


Thanks for accepting the patch.

File MAINTAINERS does not indicate for section "Virt" where the relevant
git repository is located.

I assume that you relate to
https://git.linaro.org/people/pmaydell/qemu-arm.git
But I can't find the patch there. Do you still have to push?

Please, add 'T:' entries in MAINTAINERS for the sub-systems that you
maintain.

Best regards

Heinrich



-- PMM

[PATCH v2 2/3] hw/nvme: namespace parameter for EUI-64

[Heinrich Schuchardt]

The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI-64.

Signed-off-by: Heinrich Schuchardt 
Acked-by: Klaus Jensen 
---
v2:
fix typo %s/EUI64/EUI-64/
---
 docs/system/nvme.rst |  4 +++
 hw/nvme/ctrl.c   | 58 ++--
 hw/nvme/ns.c |  2 ++
 hw/nvme/nvme.h   |  1 +
 4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index f7f63d6bf6..b5f8288d7c 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -81,6 +81,10 @@ There are a number of parameters available:
   Set the UUID of the namespace. This will be reported as a "Namespace UUID"
   descriptor in the Namespace Identification Descriptor List.

+``eui64``
+  Set the EUI-64 of the namespace. This will be reported as a "IEEE Extended
+  Unique Identifier" descriptor in the Namespace Identification Descriptor 
List.
+
 ``bus``
   If there are more ``nvme`` devices defined, this parameter may be used to
   attach the namespace to a specific ``nvme`` device (identified by an ``id``
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8dd9cb2ccb..f37c4fd635 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4436,19 +4436,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
 NvmeIdentify *c = (NvmeIdentify *)>cmd;
 uint32_t nsid = le32_to_cpu(c->nsid);
 uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
-
-struct data {
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v[NVME_NIDL_UUID];
-} uuid;
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v;
-} csi;
-};
-
-struct data *ns_descrs = (struct data *)list;
+uint8_t *pos = list;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v[NVME_NIDL_UUID];
+} QEMU_PACKED uuid;
+struct {
+NvmeIdNsDescr hdr;
+uint64_t v;
+} QEMU_PACKED eui64;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v;
+} QEMU_PACKED csi;

 trace_pci_nvme_identify_ns_descr_list(nsid);

@@ -4462,17 +4462,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
 }

 /*
- * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
- * structure, a Namespace UUID (nidt = 3h) must be reported in the
- * Namespace Identification Descriptor. Add the namespace UUID here.
+ * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must
+ * provide a valid Namespace UUID in the Namespace Identification 
Descriptor
+ * data structure. QEMU does not yet support setting NGUID.
  */
-ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
-ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
-memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
-
-ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
-ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
-ns_descrs->csi.v = ns->csi;
+uuid.hdr.nidt = NVME_NIDT_UUID;
+uuid.hdr.nidl = NVME_NIDL_UUID;
+memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
+memcpy(pos, , sizeof(uuid));
+pos += sizeof(uuid);
+
+if (ns->params.eui64) {
+eui64.hdr.nidt = NVME_NIDT_EUI64;
+eui64.hdr.nidl = NVME_NIDL_EUI64;
+eui64.v = cpu_to_be64(ns->params.eui64);
+memcpy(pos, , sizeof(eui64));
+pos += sizeof(eui64);
+}
+
+csi.hdr.nidt = NVME_NIDT_CSI;
+csi.hdr.nidl = NVME_NIDL_CSI;
+csi.v = ns->csi;
+memcpy(pos, , sizeof(csi));
+pos += sizeof(csi);

 return nvme_c2h(n, list, sizeof(list), req);
 }
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 3fec9c6273..45e457de6a 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
 id_ns->mcl = cpu_to_le32(ns->params.mcl);
 id_ns->msrc = ns->params.msrc;
+id_ns->eui64 = cpu_to_be64(ns->params.eui64);

 ds = 31 - clz32(ns->blkconf.logical_block_size);
 ms = ns->params.ms;
@@ -511,6 +512,7 @@ static Property nvme_ns_props[] = {
 DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
 DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
 DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
+DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
 DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0),
 DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0),
 DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 93a7e0e538..ac90e13d7b 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -83,6 +83,7 @@ typedef struct NvmeNamespaceParams {
 bool shared;
 uint32_t nsid;
 QemuUUID uuid;
+uint64_t eui64;

 uint16_t ms;
 uint8_t  mset;
--
2.30.2

[PATCH v2 1/3] hw: virt: consider hw_compat_6_0

[Heinrich Schuchardt]

virt-6.0 must consider hw_compat_6_0.

Fixes: da7e13c00b59 ("hw: add compat machines for 6.1")
Signed-off-by: Heinrich Schuchardt 
Reviewed-by: Cornelia Huck 
---
v2:
add missing Fixes: tag
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 840758666d..8bc3b408fe 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2764,6 +2764,8 @@ DEFINE_VIRT_MACHINE_AS_LATEST(6, 1)

 static void virt_machine_6_0_options(MachineClass *mc)
 {
+virt_machine_6_1_options(mc);
+compat_props_add(mc->compat_props, hw_compat_6_0, hw_compat_6_0_len);
 }
 DEFINE_VIRT_MACHINE(6, 0)

--
2.30.2

[PATCH v2 3/3] hw/nvme: default for namespace EUI-64

[Heinrich Schuchardt]

On machines with version > 6.0 replace a missing EUI-64 by a generated
value.

Signed-off-by: Heinrich Schuchardt 
---
v2:
new patch
---
 docs/system/nvme.rst | 2 ++
 hw/core/machine.c| 1 +
 hw/nvme/ns.c | 9 +
 hw/nvme/nvme.h   | 2 ++
 4 files changed, 14 insertions(+)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index b5f8288d7c..33a15c7dbc 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -84,6 +84,8 @@ There are a number of parameters available:
 ``eui64``
   Set the EUI-64 of the namespace. This will be reported as a "IEEE Extended
   Unique Identifier" descriptor in the Namespace Identification Descriptor 
List.
+  Since machine type 6.1 a non-zero default value is used if the parameter
+  is not provided. For earlier machine types the field defaults to 0.

 ``bus``
   If there are more ``nvme`` devices defined, this parameter may be used to
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55b9bc7817..d0e934 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@
 GlobalProperty hw_compat_6_0[] = {
 { "gpex-pcihost", "allow-unmapped-accesses", "false" },
 { "i8042", "extended-state", "false"},
+{ "nvme-ns", "eui64-default", "off"},
 };
 const size_t hw_compat_6_0_len = G_N_ELEMENTS(hw_compat_6_0);

diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 45e457de6a..4275c3db63 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -56,6 +56,7 @@ void nvme_ns_init_format(NvmeNamespace *ns)

 static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 {
+static uint64_t ns_count;
 NvmeIdNs *id_ns = >id_ns;
 uint8_t ds;
 uint16_t ms;
@@ -73,6 +74,12 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 id_ns->nmic |= NVME_NMIC_NS_SHARED;
 }

+/* Substitute a missing EUI-64 by an autogenerated one */
+++ns_count;
+if (!ns->params.eui64 && ns->params.eui64_default) {
+ns->params.eui64 = ns_count + NVME_EUI64_DEFAULT;
+}
+
 /* simple copy */
 id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
 id_ns->mcl = cpu_to_le32(ns->params.mcl);
@@ -533,6 +540,8 @@ static Property nvme_ns_props[] = {
params.max_open_zones, 0),
 DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
params.zd_extension_size, 0),
+DEFINE_PROP_BOOL("eui64-default", NvmeNamespace, params.eui64_default,
+ true),
 DEFINE_PROP_END_OF_LIST(),
 };

diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index ac90e13d7b..3fb869731d 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -26,6 +26,7 @@

 #define NVME_MAX_CONTROLLERS 32
 #define NVME_MAX_NAMESPACES  256
+#define NVME_EUI64_DEFAULT 0x27fed9272381cbd0UL

 typedef struct NvmeCtrl NvmeCtrl;
 typedef struct NvmeNamespace NvmeNamespace;
@@ -84,6 +85,7 @@ typedef struct NvmeNamespaceParams {
 uint32_t nsid;
 QemuUUID uuid;
 uint64_t eui64;
+bool eui64_default;

 uint16_t ms;
 uint8_t  mset;
--
2.30.2

[PATCH v2 0/3] hw/nvme: namespace parameter for EUI-64

[Heinrich Schuchardt]

The EUI-64 field is the only identifier for NVMe namespaces in UEFI device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI-64.

v2:
include patch for hw_compat_6_0
add a patch to implement default values for the EUI-64

Heinrich Schuchardt (3):
  hw: virt: consider hw_compat_6_0
  hw/nvme: namespace parameter for EUI-64
  hw/nvme: default for namespace EUI-64

 docs/system/nvme.rst |  6 +
 hw/arm/virt.c|  2 ++
 hw/core/machine.c|  1 +
 hw/nvme/ctrl.c   | 58 ++--
 hw/nvme/ns.c | 11 +
 hw/nvme/nvme.h   |  3 +++
 6 files changed, 58 insertions(+), 23 deletions(-)

--
2.30.2

[PATCH 1/1] hw: virt: consider hw_compat_6_0

[Heinrich Schuchardt]

virt-6.0 must consider hw_compat_6_0.

Signed-off-by: Heinrich Schuchardt 
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 840758666d..8bc3b408fe 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2764,6 +2764,8 @@ DEFINE_VIRT_MACHINE_AS_LATEST(6, 1)

 static void virt_machine_6_0_options(MachineClass *mc)
 {
+virt_machine_6_1_options(mc);
+compat_props_add(mc->compat_props, hw_compat_6_0, hw_compat_6_0_len);
 }
 DEFINE_VIRT_MACHINE(6, 0)

--
2.30.2

Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64

[Heinrich Schuchardt]

Am 9. Juni 2021 21:57:26 MESZ schrieb Klaus Jensen :
>On Jun  9 20:13, Heinrich Schuchardt wrote:
>>Am 9. Juni 2021 16:39:20 MESZ schrieb "Daniel P. Berrangé"
>:
>>>On Wed, Jun 09, 2021 at 02:33:08PM +0200, Klaus Jensen wrote:
>>>> On Jun  9 14:21, Heinrich Schuchardt wrote:
>>>> > On 6/9/21 2:14 PM, Klaus Jensen wrote:
>>>> > > On Jun  9 13:46, Heinrich Schuchardt wrote:
>>>> > > > The EUI64 field is the only identifier for NVMe namespaces in
>>>UEFI device
>>>> > > > paths. Add a new namespace property "eui64", that provides
>the
>>>user the
>>>> > > > option to specify the EUI64.
>>>> > > >
>>>> > > > Signed-off-by: Heinrich Schuchardt 
>>>> > > > ---
>>>> > > > docs/system/nvme.rst |  4 +++
>>>> > > > hw/nvme/ctrl.c   | 58
>>>++--
>>>> > > > hw/nvme/ns.c |  2 ++
>>>> > > > hw/nvme/nvme.h   |  1 +
>>>> > > > 4 files changed, 42 insertions(+), 23 deletions(-)
>>>> > > >
>>>> > > > diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
>>>> > > > index f7f63d6bf6..a6042f942a 100644
>>>> > > > --- a/docs/system/nvme.rst
>>>> > > > +++ b/docs/system/nvme.rst
>>>> > > > @@ -81,6 +81,10 @@ There are a number of parameters
>available:
>>>> > > >   Set the UUID of the namespace. This will be reported as a
>>>"Namespace
>>>> > > > UUID"
>>>> > > >   descriptor in the Namespace Identification Descriptor List.
>>>> > > >
>>>> > > > +``eui64``
>>>> > > > +  Set the EUI64 of the namespace. This will be reported as a
>>>"IEEE
>>>> > > > Extended
>>>> > > > +  Unique Identifier" descriptor in the Namespace
>>>Identification
>>>> > > > Descriptor List.
>>>> > > > +
>>>> > > > ``bus``
>>>> > > >   If there are more ``nvme`` devices defined, this parameter
>>>may be
>>>> > > > used to
>>>> > > >   attach the namespace to a specific ``nvme`` device
>>>(identified by an
>>>> > > > ``id``
>>>> > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>>>> > > > index 0bcaf7192f..21f2d6843b 100644
>>>> > > > --- a/hw/nvme/ctrl.c
>>>> > > > +++ b/hw/nvme/ctrl.c
>>>> > > > @@ -4426,19 +4426,19 @@ static uint16_t
>>>> > > > nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
>>>> > > >     NvmeIdentify *c = (NvmeIdentify *)>cmd;
>>>> > > >     uint32_t nsid = le32_to_cpu(c->nsid);
>>>> > > >     uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
>>>> > > > -
>>>> > > > -    struct data {
>>>> > > > -    struct {
>>>> > > > -    NvmeIdNsDescr hdr;
>>>> > > > -    uint8_t v[NVME_NIDL_UUID];
>>>> > > > -    } uuid;
>>>> > > > -    struct {
>>>> > > > -    NvmeIdNsDescr hdr;
>>>> > > > -    uint8_t v;
>>>> > > > -    } csi;
>>>> > > > -    };
>>>> > > > -
>>>> > > > -    struct data *ns_descrs = (struct data *)list;
>>>> > > > +    uint8_t *pos = list;
>>>> > > > +    struct {
>>>> > > > +    NvmeIdNsDescr hdr;
>>>> > > > +    uint8_t v[NVME_NIDL_UUID];
>>>> > > > +    } QEMU_PACKED uuid;
>>>> > > > +    struct {
>>>> > > > +    NvmeIdNsDescr hdr;
>>>> > > > +    uint64_t v;
>>>> > > > +    } QEMU_PACKED eui64;
>>>> > > > +    struct {
>>>> > > > +    NvmeIdNsDescr hdr;
>>>> > > > +    uint8_t v;
>>>> > > > +    } QEMU_PACKED csi;
>>>> > > >
>>>> > > >     trace_pci_nvme_identify_ns_descr_list(nsid);
>>>> > > >
>>>> >

Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64

[Heinrich Schuchardt]

Am 9. Juni 2021 16:39:20 MESZ schrieb "Daniel P. Berrangé" 
:
>On Wed, Jun 09, 2021 at 02:33:08PM +0200, Klaus Jensen wrote:
>> On Jun  9 14:21, Heinrich Schuchardt wrote:
>> > On 6/9/21 2:14 PM, Klaus Jensen wrote:
>> > > On Jun  9 13:46, Heinrich Schuchardt wrote:
>> > > > The EUI64 field is the only identifier for NVMe namespaces in
>UEFI device
>> > > > paths. Add a new namespace property "eui64", that provides the
>user the
>> > > > option to specify the EUI64.
>> > > > 
>> > > > Signed-off-by: Heinrich Schuchardt 
>> > > > ---
>> > > > docs/system/nvme.rst |  4 +++
>> > > > hw/nvme/ctrl.c   | 58
>++--
>> > > > hw/nvme/ns.c |  2 ++
>> > > > hw/nvme/nvme.h   |  1 +
>> > > > 4 files changed, 42 insertions(+), 23 deletions(-)
>> > > > 
>> > > > diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
>> > > > index f7f63d6bf6..a6042f942a 100644
>> > > > --- a/docs/system/nvme.rst
>> > > > +++ b/docs/system/nvme.rst
>> > > > @@ -81,6 +81,10 @@ There are a number of parameters available:
>> > > >   Set the UUID of the namespace. This will be reported as a
>"Namespace
>> > > > UUID"
>> > > >   descriptor in the Namespace Identification Descriptor List.
>> > > > 
>> > > > +``eui64``
>> > > > +  Set the EUI64 of the namespace. This will be reported as a
>"IEEE
>> > > > Extended
>> > > > +  Unique Identifier" descriptor in the Namespace
>Identification
>> > > > Descriptor List.
>> > > > +
>> > > > ``bus``
>> > > >   If there are more ``nvme`` devices defined, this parameter
>may be
>> > > > used to
>> > > >   attach the namespace to a specific ``nvme`` device
>(identified by an
>> > > > ``id``
>> > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > > > index 0bcaf7192f..21f2d6843b 100644
>> > > > --- a/hw/nvme/ctrl.c
>> > > > +++ b/hw/nvme/ctrl.c
>> > > > @@ -4426,19 +4426,19 @@ static uint16_t
>> > > > nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
>> > > >     NvmeIdentify *c = (NvmeIdentify *)>cmd;
>> > > >     uint32_t nsid = le32_to_cpu(c->nsid);
>> > > >     uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
>> > > > -
>> > > > -    struct data {
>> > > > -    struct {
>> > > > -    NvmeIdNsDescr hdr;
>> > > > -    uint8_t v[NVME_NIDL_UUID];
>> > > > -    } uuid;
>> > > > -    struct {
>> > > > -    NvmeIdNsDescr hdr;
>> > > > -    uint8_t v;
>> > > > -    } csi;
>> > > > -    };
>> > > > -
>> > > > -    struct data *ns_descrs = (struct data *)list;
>> > > > +    uint8_t *pos = list;
>> > > > +    struct {
>> > > > +    NvmeIdNsDescr hdr;
>> > > > +    uint8_t v[NVME_NIDL_UUID];
>> > > > +    } QEMU_PACKED uuid;
>> > > > +    struct {
>> > > > +    NvmeIdNsDescr hdr;
>> > > > +    uint64_t v;
>> > > > +    } QEMU_PACKED eui64;
>> > > > +    struct {
>> > > > +    NvmeIdNsDescr hdr;
>> > > > +    uint8_t v;
>> > > > +    } QEMU_PACKED csi;
>> > > > 
>> > > >     trace_pci_nvme_identify_ns_descr_list(nsid);
>> > > > 
>> > > > @@ -4452,17 +4452,29 @@ static uint16_t
>> > > > nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
>> > > >     }
>> > > > 
>> > > >     /*
>> > > > - * Because the NGUID and EUI64 fields are 0 in the
>Identify
>> > > > Namespace data
>> > > > - * structure, a Namespace UUID (nidt = 3h) must be
>reported in the
>> > > > - * Namespace Identification Descriptor. Add the namespace
>UUID here.
>> > > > + * If the EUI64 field is 0 and the NGUID field is 0, the
>> > > > namespace must
>> > > > + * provide a valid Namespace UUID in the Namespace
>Identification
>> > > > Descriptor
>> > &g

Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64

[Heinrich Schuchardt]

On 6/9/21 2:33 PM, Klaus Jensen wrote:

On Jun  9 14:21, Heinrich Schuchardt wrote:

On 6/9/21 2:14 PM, Klaus Jensen wrote:

On Jun  9 13:46, Heinrich Schuchardt wrote:

The EUI64 field is the only identifier for NVMe namespaces in UEFI
device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI64.

Signed-off-by: Heinrich Schuchardt 
---
docs/system/nvme.rst |  4 +++
hw/nvme/ctrl.c   | 58 ++--
hw/nvme/ns.c |  2 ++
hw/nvme/nvme.h   |  1 +
4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index f7f63d6bf6..a6042f942a 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -81,6 +81,10 @@ There are a number of parameters available:
  Set the UUID of the namespace. This will be reported as a "Namespace
UUID"
  descriptor in the Namespace Identification Descriptor List.

+``eui64``
+  Set the EUI64 of the namespace. This will be reported as a "IEEE
Extended
+  Unique Identifier" descriptor in the Namespace Identification
Descriptor List.
+
``bus``
  If there are more ``nvme`` devices defined, this parameter may be
used to
  attach the namespace to a specific ``nvme`` device (identified by an
``id``
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0bcaf7192f..21f2d6843b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4426,19 +4426,19 @@ static uint16_t
nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
    NvmeIdentify *c = (NvmeIdentify *)>cmd;
    uint32_t nsid = le32_to_cpu(c->nsid);
    uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
-
-    struct data {
-    struct {
-    NvmeIdNsDescr hdr;
-    uint8_t v[NVME_NIDL_UUID];
-    } uuid;
-    struct {
-    NvmeIdNsDescr hdr;
-    uint8_t v;
-    } csi;
-    };
-
-    struct data *ns_descrs = (struct data *)list;
+    uint8_t *pos = list;
+    struct {
+    NvmeIdNsDescr hdr;
+    uint8_t v[NVME_NIDL_UUID];
+    } QEMU_PACKED uuid;
+    struct {
+    NvmeIdNsDescr hdr;
+    uint64_t v;
+    } QEMU_PACKED eui64;
+    struct {
+    NvmeIdNsDescr hdr;
+    uint8_t v;
+    } QEMU_PACKED csi;

    trace_pci_nvme_identify_ns_descr_list(nsid);

@@ -4452,17 +4452,29 @@ static uint16_t
nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
    }

    /*
- * Because the NGUID and EUI64 fields are 0 in the Identify
Namespace data
- * structure, a Namespace UUID (nidt = 3h) must be reported in the
- * Namespace Identification Descriptor. Add the namespace UUID
here.
+ * If the EUI64 field is 0 and the NGUID field is 0, the
namespace must
+ * provide a valid Namespace UUID in the Namespace Identification
Descriptor
+ * data structure. QEMU does not yet support setting NGUID.
 */
-    ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
-    ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
-    memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
-
-    ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
-    ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
-    ns_descrs->csi.v = ns->csi;
+    uuid.hdr.nidt = NVME_NIDT_UUID;
+    uuid.hdr.nidl = NVME_NIDL_UUID;
+    memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
+    memcpy(pos, , sizeof(uuid));
+    pos += sizeof(uuid);
+
+    if (ns->params.eui64) {
+    eui64.hdr.nidt = NVME_NIDT_EUI64;
+    eui64.hdr.nidl = NVME_NIDL_EUI64;
+    eui64.v = cpu_to_be64(ns->params.eui64);
+    memcpy(pos, , sizeof(eui64));
+    pos += sizeof(eui64);
+    }
+
+    csi.hdr.nidt = NVME_NIDT_CSI;
+    csi.hdr.nidl = NVME_NIDL_CSI;
+    csi.v = ns->csi;
+    memcpy(pos, , sizeof(csi));
+    pos += sizeof(csi);

    return nvme_c2h(n, list, sizeof(list), req);
}
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 992e5a13f5..ddf395d60e 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error
**errp)
    id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
    id_ns->mcl = cpu_to_le32(ns->params.mcl);
    id_ns->msrc = ns->params.msrc;
+    id_ns->eui64 = cpu_to_be64(ns->params.eui64);

    ds = 31 - clz32(ns->blkconf.logical_block_size);
    ms = ns->params.ms;
@@ -518,6 +519,7 @@ static Property nvme_ns_props[] = {
    DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
    DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
    DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
+    DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
    DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0),
    DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0),
    DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda14..abe7fab21c 100644
--- a/hw/nvme/nvme.h

Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64

[Heinrich Schuchardt]

+cc qemu-bl...@nongnu.org

On 6/9/21 1:46 PM, Heinrich Schuchardt wrote:

The EUI64 field is the only identifier for NVMe namespaces in UEFI device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI64.

Signed-off-by: Heinrich Schuchardt 
---
  docs/system/nvme.rst |  4 +++
  hw/nvme/ctrl.c   | 58 ++--
  hw/nvme/ns.c |  2 ++
  hw/nvme/nvme.h   |  1 +
  4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index f7f63d6bf6..a6042f942a 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -81,6 +81,10 @@ There are a number of parameters available:
Set the UUID of the namespace. This will be reported as a "Namespace UUID"
descriptor in the Namespace Identification Descriptor List.

+``eui64``
+  Set the EUI64 of the namespace. This will be reported as a "IEEE Extended
+  Unique Identifier" descriptor in the Namespace Identification Descriptor 
List.
+
  ``bus``
If there are more ``nvme`` devices defined, this parameter may be used to
attach the namespace to a specific ``nvme`` device (identified by an ``id``
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0bcaf7192f..21f2d6843b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4426,19 +4426,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
  NvmeIdentify *c = (NvmeIdentify *)>cmd;
  uint32_t nsid = le32_to_cpu(c->nsid);
  uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
-
-struct data {
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v[NVME_NIDL_UUID];
-} uuid;
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v;
-} csi;
-};
-
-struct data *ns_descrs = (struct data *)list;
+uint8_t *pos = list;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v[NVME_NIDL_UUID];
+} QEMU_PACKED uuid;
+struct {
+NvmeIdNsDescr hdr;
+uint64_t v;
+} QEMU_PACKED eui64;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v;
+} QEMU_PACKED csi;

  trace_pci_nvme_identify_ns_descr_list(nsid);

@@ -4452,17 +4452,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
  }

  /*
- * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
- * structure, a Namespace UUID (nidt = 3h) must be reported in the
- * Namespace Identification Descriptor. Add the namespace UUID here.
+ * If the EUI64 field is 0 and the NGUID field is 0, the namespace must
+ * provide a valid Namespace UUID in the Namespace Identification 
Descriptor
+ * data structure. QEMU does not yet support setting NGUID.
   */
-ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
-ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
-memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
-
-ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
-ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
-ns_descrs->csi.v = ns->csi;
+uuid.hdr.nidt = NVME_NIDT_UUID;
+uuid.hdr.nidl = NVME_NIDL_UUID;
+memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
+memcpy(pos, , sizeof(uuid));
+pos += sizeof(uuid);
+
+if (ns->params.eui64) {
+eui64.hdr.nidt = NVME_NIDT_EUI64;
+eui64.hdr.nidl = NVME_NIDL_EUI64;
+eui64.v = cpu_to_be64(ns->params.eui64);
+memcpy(pos, , sizeof(eui64));
+pos += sizeof(eui64);
+}
+
+csi.hdr.nidt = NVME_NIDT_CSI;
+csi.hdr.nidl = NVME_NIDL_CSI;
+csi.v = ns->csi;
+memcpy(pos, , sizeof(csi));
+pos += sizeof(csi);

  return nvme_c2h(n, list, sizeof(list), req);
  }
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 992e5a13f5..ddf395d60e 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
  id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
  id_ns->mcl = cpu_to_le32(ns->params.mcl);
  id_ns->msrc = ns->params.msrc;
+id_ns->eui64 = cpu_to_be64(ns->params.eui64);

  ds = 31 - clz32(ns->blkconf.logical_block_size);
  ms = ns->params.ms;
@@ -518,6 +519,7 @@ static Property nvme_ns_props[] = {
  DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
  DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
  DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
+DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
  DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0),
  DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0),
  DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda14..abe7fab21c 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -83,6 +83,7 @@ typedef struct NvmeNamespaceParams {
  bool shared;
  uint32_t nsid;
  QemuUUID uuid;
+uint64_t eui64;

  uint16_t ms;
  uint8_t  mset;
--
2.30.2

Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64

[Heinrich Schuchardt]

On 6/9/21 2:14 PM, Klaus Jensen wrote:

On Jun  9 13:46, Heinrich Schuchardt wrote:

The EUI64 field is the only identifier for NVMe namespaces in UEFI device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI64.

Signed-off-by: Heinrich Schuchardt 
---
docs/system/nvme.rst |  4 +++
hw/nvme/ctrl.c   | 58 ++--
hw/nvme/ns.c |  2 ++
hw/nvme/nvme.h   |  1 +
4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index f7f63d6bf6..a6042f942a 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -81,6 +81,10 @@ There are a number of parameters available:
  Set the UUID of the namespace. This will be reported as a "Namespace
UUID"
  descriptor in the Namespace Identification Descriptor List.

+``eui64``
+  Set the EUI64 of the namespace. This will be reported as a "IEEE
Extended
+  Unique Identifier" descriptor in the Namespace Identification
Descriptor List.
+
``bus``
  If there are more ``nvme`` devices defined, this parameter may be
used to
  attach the namespace to a specific ``nvme`` device (identified by an
``id``
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0bcaf7192f..21f2d6843b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4426,19 +4426,19 @@ static uint16_t
nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
    NvmeIdentify *c = (NvmeIdentify *)>cmd;
    uint32_t nsid = le32_to_cpu(c->nsid);
    uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
-
-    struct data {
-    struct {
-    NvmeIdNsDescr hdr;
-    uint8_t v[NVME_NIDL_UUID];
-    } uuid;
-    struct {
-    NvmeIdNsDescr hdr;
-    uint8_t v;
-    } csi;
-    };
-
-    struct data *ns_descrs = (struct data *)list;
+    uint8_t *pos = list;
+    struct {
+    NvmeIdNsDescr hdr;
+    uint8_t v[NVME_NIDL_UUID];
+    } QEMU_PACKED uuid;
+    struct {
+    NvmeIdNsDescr hdr;
+    uint64_t v;
+    } QEMU_PACKED eui64;
+    struct {
+    NvmeIdNsDescr hdr;
+    uint8_t v;
+    } QEMU_PACKED csi;

    trace_pci_nvme_identify_ns_descr_list(nsid);

@@ -4452,17 +4452,29 @@ static uint16_t
nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
    }

    /*
- * Because the NGUID and EUI64 fields are 0 in the Identify
Namespace data
- * structure, a Namespace UUID (nidt = 3h) must be reported in the
- * Namespace Identification Descriptor. Add the namespace UUID here.
+ * If the EUI64 field is 0 and the NGUID field is 0, the
namespace must
+ * provide a valid Namespace UUID in the Namespace Identification
Descriptor
+ * data structure. QEMU does not yet support setting NGUID.
 */
-    ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
-    ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
-    memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
-
-    ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
-    ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
-    ns_descrs->csi.v = ns->csi;
+    uuid.hdr.nidt = NVME_NIDT_UUID;
+    uuid.hdr.nidl = NVME_NIDL_UUID;
+    memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
+    memcpy(pos, , sizeof(uuid));
+    pos += sizeof(uuid);
+
+    if (ns->params.eui64) {
+    eui64.hdr.nidt = NVME_NIDT_EUI64;
+    eui64.hdr.nidl = NVME_NIDL_EUI64;
+    eui64.v = cpu_to_be64(ns->params.eui64);
+    memcpy(pos, , sizeof(eui64));
+    pos += sizeof(eui64);
+    }
+
+    csi.hdr.nidt = NVME_NIDT_CSI;
+    csi.hdr.nidl = NVME_NIDL_CSI;
+    csi.v = ns->csi;
+    memcpy(pos, , sizeof(csi));
+    pos += sizeof(csi);

    return nvme_c2h(n, list, sizeof(list), req);
}
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 992e5a13f5..ddf395d60e 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error
**errp)
    id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
    id_ns->mcl = cpu_to_le32(ns->params.mcl);
    id_ns->msrc = ns->params.msrc;
+    id_ns->eui64 = cpu_to_be64(ns->params.eui64);

    ds = 31 - clz32(ns->blkconf.logical_block_size);
    ms = ns->params.ms;
@@ -518,6 +519,7 @@ static Property nvme_ns_props[] = {
    DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
    DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
    DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
+    DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
    DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0),
    DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0),
    DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda14..abe7fab21c 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -83,6 +83,7 @@ typedef struct NvmeNamespaceParams {
    bool share

[PATCH 1/1] hw/nvme: namespace parameter for EUI64

[Heinrich Schuchardt]

The EUI64 field is the only identifier for NVMe namespaces in UEFI device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI64.

Signed-off-by: Heinrich Schuchardt 
---
 docs/system/nvme.rst |  4 +++
 hw/nvme/ctrl.c   | 58 ++--
 hw/nvme/ns.c |  2 ++
 hw/nvme/nvme.h   |  1 +
 4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index f7f63d6bf6..a6042f942a 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -81,6 +81,10 @@ There are a number of parameters available:
   Set the UUID of the namespace. This will be reported as a "Namespace UUID"
   descriptor in the Namespace Identification Descriptor List.

+``eui64``
+  Set the EUI64 of the namespace. This will be reported as a "IEEE Extended
+  Unique Identifier" descriptor in the Namespace Identification Descriptor 
List.
+
 ``bus``
   If there are more ``nvme`` devices defined, this parameter may be used to
   attach the namespace to a specific ``nvme`` device (identified by an ``id``
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0bcaf7192f..21f2d6843b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4426,19 +4426,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
 NvmeIdentify *c = (NvmeIdentify *)>cmd;
 uint32_t nsid = le32_to_cpu(c->nsid);
 uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
-
-struct data {
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v[NVME_NIDL_UUID];
-} uuid;
-struct {
-NvmeIdNsDescr hdr;
-uint8_t v;
-} csi;
-};
-
-struct data *ns_descrs = (struct data *)list;
+uint8_t *pos = list;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v[NVME_NIDL_UUID];
+} QEMU_PACKED uuid;
+struct {
+NvmeIdNsDescr hdr;
+uint64_t v;
+} QEMU_PACKED eui64;
+struct {
+NvmeIdNsDescr hdr;
+uint8_t v;
+} QEMU_PACKED csi;

 trace_pci_nvme_identify_ns_descr_list(nsid);

@@ -4452,17 +4452,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl 
*n, NvmeRequest *req)
 }

 /*
- * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
- * structure, a Namespace UUID (nidt = 3h) must be reported in the
- * Namespace Identification Descriptor. Add the namespace UUID here.
+ * If the EUI64 field is 0 and the NGUID field is 0, the namespace must
+ * provide a valid Namespace UUID in the Namespace Identification 
Descriptor
+ * data structure. QEMU does not yet support setting NGUID.
  */
-ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
-ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
-memcpy(_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
-
-ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
-ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
-ns_descrs->csi.v = ns->csi;
+uuid.hdr.nidt = NVME_NIDT_UUID;
+uuid.hdr.nidl = NVME_NIDL_UUID;
+memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
+memcpy(pos, , sizeof(uuid));
+pos += sizeof(uuid);
+
+if (ns->params.eui64) {
+eui64.hdr.nidt = NVME_NIDT_EUI64;
+eui64.hdr.nidl = NVME_NIDL_EUI64;
+eui64.v = cpu_to_be64(ns->params.eui64);
+memcpy(pos, , sizeof(eui64));
+pos += sizeof(eui64);
+}
+
+csi.hdr.nidt = NVME_NIDT_CSI;
+csi.hdr.nidl = NVME_NIDL_CSI;
+csi.v = ns->csi;
+memcpy(pos, , sizeof(csi));
+pos += sizeof(csi);

 return nvme_c2h(n, list, sizeof(list), req);
 }
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 992e5a13f5..ddf395d60e 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
 id_ns->mcl = cpu_to_le32(ns->params.mcl);
 id_ns->msrc = ns->params.msrc;
+id_ns->eui64 = cpu_to_be64(ns->params.eui64);

 ds = 31 - clz32(ns->blkconf.logical_block_size);
 ms = ns->params.ms;
@@ -518,6 +519,7 @@ static Property nvme_ns_props[] = {
 DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
 DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
 DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
+DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
 DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0),
 DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0),
 DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda14..abe7fab21c 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -83,6 +83,7 @@ typedef struct NvmeNamespaceParams {
 bool shared;
 uint32_t nsid;
 QemuUUID uuid;
+uint64_t eui64;

 uint16_t ms;
 uint8_t  mset;
--
2.30.2

[Qemu-devel] [PATCH v2 1/1] hw/arm/virt: provide a model property in the fdt

[Heinrich Schuchardt]

According to the "Devicetree Specification, Release v0.2" 'model' is a
required property of the root node.

Some software like the Debian flash-kernel package rely on this property
to identify boards.

The patch sets the model property to 'qemu,virt'.

Signed-off-by: Heinrich Schuchardt 
---
v2
use the recommended format of the model property
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 281ddcdf6e..abe366895a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -200,6 +200,7 @@ static void create_fdt(VirtMachineState *vms)
 vms->fdt = fdt;
 
 /* Header */
+qemu_fdt_setprop_string(fdt, "/", "model", "qemu,virt");
 qemu_fdt_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
 qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
 qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
-- 
2.19.1

[Qemu-devel] [PATCH 1/1] hw/arm/virt: provide a model property in the fdt

[Heinrich Schuchardt]

Device trees in the Linux kernel generally provide a model property. Some
software like the Debian flash-kernel package rely on this property to
identify boards.

The patch sets the model property for the virt boards to 'QEMU virt'.

Signed-off-by: Heinrich Schuchardt 
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 281ddcdf6e..abe366895a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -200,6 +200,7 @@ static void create_fdt(VirtMachineState *vms)
 vms->fdt = fdt;
 
 /* Header */
+qemu_fdt_setprop_string(fdt, "/", "model", "QEMU virt");
 qemu_fdt_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
 qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
 qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
-- 
2.19.1