Re: [PATCH 18/23] hw/sd/sdhci: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sdhci.c | 62 +--
1 file changed, 26 insertions(+), 36 deletions(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 87122e4245..3ed2d8658a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -37,7 +37,6 @@
#include "migration/vmstate.h"
#include "sdhci-internal.h"
#include "qemu/log.h"
-#include "qemu/module.h"
#include "trace.h"
#include "qom/object.h"
@@ -1598,15 +1597,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
sdhci_common_class_init(klass, data);
}
-static const TypeInfo sdhci_sysbus_info = {
-.name = TYPE_SYSBUS_SDHCI,
-.parent = TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(SDHCIState),
-.instance_init = sdhci_sysbus_init,
-.instance_finalize = sdhci_sysbus_finalize,
-.class_init = sdhci_sysbus_class_init,
-};
-
/* --- qdev bus master --- */
static void sdhci_bus_class_init(ObjectClass *klass, void *data)
@@ -1617,13 +1607,6 @@ static void sdhci_bus_class_init(ObjectClass *klass,
void *data)
sbc->set_readonly = sdhci_set_readonly;
}
-static const TypeInfo sdhci_bus_info = {
-.name = TYPE_SDHCI_BUS,
-.parent = TYPE_SD_BUS,
-.instance_size = sizeof(SDBus),
-.class_init = sdhci_bus_class_init,
-};
-
/* --- qdev i.MX eSDHC --- */
#define USDHC_MIX_CTRL 0x48
@@ -1882,12 +1865,6 @@ static void imx_usdhc_init(Object *obj)
s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
}
-static const TypeInfo imx_usdhc_info = {
-.name = TYPE_IMX_USDHC,
-.parent = TYPE_SYSBUS_SDHCI,
-.instance_init = imx_usdhc_init,
-};
-
/* --- qdev Samsung s3c --- */
#define S3C_SDHCI_CONTROL2 0x80
@@ -1946,18 +1923,31 @@ static void sdhci_s3c_init(Object *obj)
s->io_ops = &sdhci_s3c_mmio_ops;
}
-static const TypeInfo sdhci_s3c_info = {
-.name = TYPE_S3C_SDHCI ,
-.parent = TYPE_SYSBUS_SDHCI,
-.instance_init = sdhci_s3c_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_SDHCI_BUS,
+.parent = TYPE_SD_BUS,
+.instance_size = sizeof(SDBus),
+.class_init = sdhci_bus_class_init,
+},
+{
+.name = TYPE_SYSBUS_SDHCI,
+.parent = TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(SDHCIState),
+.instance_init = sdhci_sysbus_init,
+.instance_finalize = sdhci_sysbus_finalize,
+.class_init = sdhci_sysbus_class_init,
+},
+{
+.name = TYPE_IMX_USDHC,
+.parent = TYPE_SYSBUS_SDHCI,
+.instance_init = imx_usdhc_init,
+},
+{
+.name = TYPE_S3C_SDHCI,
+.parent = TYPE_SYSBUS_SDHCI,
+.instance_init = sdhci_s3c_init,
+},
};
-static void sdhci_register_types(void)
-{
-type_register_static(&sdhci_sysbus_info);
-type_register_static(&sdhci_bus_info);
-type_register_static(&imx_usdhc_info);
-type_register_static(&sdhci_s3c_info);
-}
-
-type_init(sdhci_register_types)
+DEFINE_TYPES(types)
Re: [PATCH 22/23] hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/usb/hcd-ehci-sysbus.c | 118 +--
1 file changed, 50 insertions(+), 68 deletions(-)
diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index 2b1652f7a8..87a3bebe3e 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -19,7 +19,6 @@
#include "hw/qdev-properties.h"
#include "hw/usb/hcd-ehci.h"
#include "migration/vmstate.h"
-#include "qemu/module.h"
static const VMStateDescription vmstate_ehci_sysbus = {
.name= "ehci-sysbus",
@@ -97,17 +96,6 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void
*data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_type_info = {
-.name = TYPE_SYS_BUS_EHCI,
-.parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(EHCISysBusState),
-.instance_init = ehci_sysbus_init,
-.instance_finalize = ehci_sysbus_finalize,
-.abstract = true,
-.class_init= ehci_sysbus_class_init,
-.class_size= sizeof(SysBusEHCIClass),
-};
-
static void ehci_platform_class_init(ObjectClass *oc, void *data)
{
SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -118,12 +106,6 @@ static void ehci_platform_class_init(ObjectClass *oc, void
*data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_platform_type_info = {
-.name = TYPE_PLATFORM_EHCI,
-.parent= TYPE_SYS_BUS_EHCI,
-.class_init= ehci_platform_class_init,
-};
-
static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
{
SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -134,12 +116,6 @@ static void ehci_exynos4210_class_init(ObjectClass *oc,
void *data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_exynos4210_type_info = {
-.name = TYPE_EXYNOS4210_EHCI,
-.parent= TYPE_SYS_BUS_EHCI,
-.class_init= ehci_exynos4210_class_init,
-};
-
static void ehci_aw_h3_class_init(ObjectClass *oc, void *data)
{
SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -150,12 +126,6 @@ static void ehci_aw_h3_class_init(ObjectClass *oc, void
*data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_aw_h3_type_info = {
-.name = TYPE_AW_H3_EHCI,
-.parent= TYPE_SYS_BUS_EHCI,
-.class_init= ehci_aw_h3_class_init,
-};
-
static void ehci_npcm7xx_class_init(ObjectClass *oc, void *data)
{
SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -168,12 +138,6 @@ static void ehci_npcm7xx_class_init(ObjectClass *oc, void
*data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_npcm7xx_type_info = {
-.name = TYPE_NPCM7XX_EHCI,
-.parent= TYPE_SYS_BUS_EHCI,
-.class_init= ehci_npcm7xx_class_init,
-};
-
static void ehci_tegra2_class_init(ObjectClass *oc, void *data)
{
SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -184,12 +148,6 @@ static void ehci_tegra2_class_init(ObjectClass *oc, void
*data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_tegra2_type_info = {
-.name = TYPE_TEGRA2_EHCI,
-.parent= TYPE_SYS_BUS_EHCI,
-.class_init= ehci_tegra2_class_init,
-};
-
static void ehci_ppc4xx_init(Object *o)
{
EHCISysBusState *s = SYS_BUS_EHCI(o);
@@ -207,13 +165,6 @@ static void ehci_ppc4xx_class_init(ObjectClass *oc, void
*data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_ppc4xx_type_info = {
-.name = TYPE_PPC4xx_EHCI,
-.parent= TYPE_SYS_BUS_EHCI,
-.class_init= ehci_ppc4xx_class_init,
-.instance_init = ehci_ppc4xx_init,
-};
-
/*
* Faraday FUSBH200 USB 2.0 EHCI
*/
@@ -282,24 +233,55 @@ static void fusbh200_ehci_class_init(ObjectClass *oc,
void *data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
}
-static const TypeInfo ehci_fusbh200_type_info = {
-.name = TYPE_FUSBH200_EHCI,
-.parent= TYPE_SYS_BUS_EHCI,
-.instance_size = sizeof(FUSBH200EHCIState),
-.instance_init = fusbh200_ehci_init,
-.class_init= fusbh200_ehci_class_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_SYS_BUS_EHCI,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(EHCISysBusState),
+.instance_init = ehci_sysbus_init,
+.instance_finalize = ehci_sysbus_finalize,
+.abstract = true,
+.class_init= ehci_sysbus_class_init,
+.class_size= sizeof(SysBusEHCIClass),
+},
+{
+.name = TYPE_PLATFORM_EHCI,
+.parent
Re: [PATCH 21/23] hw/rtc/ds1338: Prefer DEFINE_TYPES() macro
Sob is missing.
Thanks,
C.
On 9/23/24 11:30, Bernhard Beschow wrote:
---
hw/rtc/ds1338.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/hw/rtc/ds1338.c b/hw/rtc/ds1338.c
index a5fe221418..6de13caf99 100644
--- a/hw/rtc/ds1338.c
+++ b/hw/rtc/ds1338.c
@@ -14,7 +14,6 @@
#include "hw/i2c/i2c.h"
#include "migration/vmstate.h"
#include "qemu/bcd.h"
-#include "qemu/module.h"
#include "qom/object.h"
#include "sysemu/rtc.h"
@@ -227,16 +226,13 @@ static void ds1338_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_ds1338;
}
-static const TypeInfo ds1338_info = {
-.name = TYPE_DS1338,
-.parent= TYPE_I2C_SLAVE,
-.instance_size = sizeof(DS1338State),
-.class_init= ds1338_class_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_DS1338,
+.parent= TYPE_I2C_SLAVE,
+.instance_size = sizeof(DS1338State),
+.class_init= ds1338_class_init,
+},
};
-static void ds1338_register_types(void)
-{
-type_register_static(&ds1338_info);
-}
-
-type_init(ds1338_register_types)
+DEFINE_TYPES(types)
Re: [PATCH 20/23] hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/i2c/smbus_eeprom.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 9e62c27a1a..1d4d9704bf 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -151,19 +151,16 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass,
void *data)
dc->user_creatable = false;
}
-static const TypeInfo smbus_eeprom_info = {
-.name = TYPE_SMBUS_EEPROM,
-.parent= TYPE_SMBUS_DEVICE,
-.instance_size = sizeof(SMBusEEPROMDevice),
-.class_init= smbus_eeprom_class_initfn,
+static const TypeInfo types[] = {
+{
+.name = TYPE_SMBUS_EEPROM,
+.parent= TYPE_SMBUS_DEVICE,
+.instance_size = sizeof(SMBusEEPROMDevice),
+.class_init= smbus_eeprom_class_initfn,
+},
};
-static void smbus_eeprom_register_types(void)
-{
-type_register_static(&smbus_eeprom_info);
-}
-
-type_init(smbus_eeprom_register_types)
+DEFINE_TYPES(types)
void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
{
Re: [PATCH 19/23] hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/block/pflash_cfi01.c | 21 -
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 7b6ec64442..cf11dada29 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -47,7 +47,6 @@
#include "qemu/bitops.h"
#include "qemu/host-utils.h"
#include "qemu/log.h"
-#include "qemu/module.h"
#include "qemu/option.h"
#include "hw/sysbus.h"
#include "migration/vmstate.h"
@@ -947,20 +946,16 @@ static void pflash_cfi01_class_init(ObjectClass *klass,
void *data)
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
}
-
-static const TypeInfo pflash_cfi01_info = {
-.name = TYPE_PFLASH_CFI01,
-.parent = TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(PFlashCFI01),
-.class_init = pflash_cfi01_class_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_PFLASH_CFI01,
+.parent = TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(PFlashCFI01),
+.class_init = pflash_cfi01_class_init,
+},
};
-static void pflash_cfi01_register_types(void)
-{
-type_register_static(&pflash_cfi01_info);
-}
-
-type_init(pflash_cfi01_register_types)
+DEFINE_TYPES(types)
PFlashCFI01 *pflash_cfi01_register(hwaddr base,
const char *name,
Re: [PATCH 17/23] hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch
On 9/23/24 11:30, Bernhard Beschow wrote:
Allows to clearly mark code sections relying on this device type.
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/ppc/e500.c | 2 +-
hw/intc/Kconfig | 4
hw/intc/meson.build | 3 +--
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 4ee4304a8a..149e608324 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -828,7 +828,7 @@ static DeviceState
*ppce500_init_mpic_qemu(PPCE500MachineState *pms,
static DeviceState *ppce500_init_mpic_kvm(const PPCE500MachineClass *pmc,
Error **errp)
{
-#ifdef CONFIG_KVM
+#ifdef CONFIG_OPENPIC_KVM
DeviceState *dev;
CPUState *cs;
diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index dd405bdb5d..a3df98ae59 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -16,6 +16,10 @@ config OPENPIC
bool
select MSI_NONBROKEN
+config OPENPIC_KVM
+bool
+depends on OPENPIC && KVM
+
config APIC
bool
select MSI_NONBROKEN
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index 6bfdc4eb33..b9de6bf5c6 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -48,8 +48,7 @@ specific_ss.add(when: 'CONFIG_IOAPIC', if_true:
files('ioapic.c'))
specific_ss.add(when: 'CONFIG_LOONGSON_LIOINTC', if_true:
files('loongson_liointc.c'))
specific_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_gic.c'))
specific_ss.add(when: 'CONFIG_OMPIC', if_true: files('ompic.c'))
-specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_OPENPIC'],
- if_true: files('openpic_kvm.c'))
+specific_ss.add(when: 'CONFIG_OPENPIC_KVM', if_true: files('openpic_kvm.c'))
specific_ss.add(when: 'CONFIG_POWERNV', if_true: files('xics_pnv.c',
'pnv_xive.c', 'pnv_xive2.c'))
specific_ss.add(when: 'CONFIG_PPC_UIC', if_true: files('ppc-uic.c'))
specific_ss.add(when: 'CONFIG_RX_ICU', if_true: files('rx_icu.c'))
Re: [PATCH 15/23] hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/ppc/mpc8544_guts.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
index 6688fd44c3..cbb1e3adda 100644
--- a/hw/ppc/mpc8544_guts.c
+++ b/hw/ppc/mpc8544_guts.c
@@ -18,7 +18,6 @@
*/
#include "qemu/osdep.h"
-#include "qemu/module.h"
#include "qemu/log.h"
#include "sysemu/runstate.h"
#include "cpu.h"
@@ -141,16 +140,13 @@ static void mpc8544_guts_initfn(Object *obj)
sysbus_init_mmio(d, &s->iomem);
}
-static const TypeInfo mpc8544_guts_info = {
-.name = TYPE_MPC8544_GUTS,
-.parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(GutsState),
-.instance_init = mpc8544_guts_initfn,
+static const TypeInfo types[] = {
+{
+.name = TYPE_MPC8544_GUTS,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(GutsState),
+.instance_init = mpc8544_guts_initfn,
+},
};
-static void mpc8544_guts_register_types(void)
-{
-type_register_static(&mpc8544_guts_info);
-}
-
-type_init(mpc8544_guts_register_types)
+DEFINE_TYPES(types)
Re: [PATCH 16/23] hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/net/fsl_etsec/etsec.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 3fdd16ef2e..9bd886b996 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -36,7 +36,6 @@
#include "registers.h"
#include "qapi/error.h"
#include "qemu/log.h"
-#include "qemu/module.h"
/* #define HEX_DUMP */
/* #define DEBUG_REGISTER */
@@ -431,17 +430,14 @@ static void etsec_class_init(ObjectClass *klass, void
*data)
dc->user_creatable = true;
}
-static const TypeInfo etsec_info = {
-.name = TYPE_ETSEC_COMMON,
-.parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(eTSEC),
-.class_init= etsec_class_init,
-.instance_init = etsec_instance_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_ETSEC_COMMON,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(eTSEC),
+.class_init= etsec_class_init,
+.instance_init = etsec_instance_init,
+},
};
-static void etsec_register_types(void)
-{
-type_register_static(&etsec_info);
-}
-
-type_init(etsec_register_types)
+DEFINE_TYPES(types)
Re: [PATCH 13/23] hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/pci-host/ppce500.c | 42 ++
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index d7ff2ba778..1ce79ea20c 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -21,7 +21,6 @@
#include "hw/pci/pci_device.h"
#include "hw/pci/pci_host.h"
#include "qemu/bswap.h"
-#include "qemu/module.h"
#include "hw/pci-host/ppce500.h"
#include "qom/object.h"
@@ -508,17 +507,6 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
dc->user_creatable = false;
}
-static const TypeInfo e500_host_bridge_info = {
-.name = TYPE_PPC_E500_PCI_BRIDGE,
-.parent= TYPE_PCI_DEVICE,
-.instance_size = sizeof(PPCE500PCIBridgeState),
-.class_init= e500_host_bridge_class_init,
-.interfaces = (InterfaceInfo[]) {
-{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
-{ },
-},
-};
-
static Property pcihost_properties[] = {
DEFINE_PROP_UINT32("first_slot", PPCE500PCIState, first_slot, 0x11),
DEFINE_PROP_UINT32("first_pin_irq", PPCE500PCIState, first_pin_irq, 0x1),
@@ -535,17 +523,23 @@ static void e500_pcihost_class_init(ObjectClass *klass,
void *data)
dc->vmsd = &vmstate_ppce500_pci;
}
-static const TypeInfo e500_pcihost_info = {
-.name = TYPE_PPC_E500_PCI_HOST_BRIDGE,
-.parent= TYPE_PCI_HOST_BRIDGE,
-.instance_size = sizeof(PPCE500PCIState),
-.class_init= e500_pcihost_class_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_PPC_E500_PCI_BRIDGE,
+.parent= TYPE_PCI_DEVICE,
+.instance_size = sizeof(PPCE500PCIBridgeState),
+.class_init= e500_host_bridge_class_init,
+.interfaces= (InterfaceInfo[]) {
+{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
+{ },
+},
+},
+{
+.name = TYPE_PPC_E500_PCI_HOST_BRIDGE,
+.parent= TYPE_PCI_HOST_BRIDGE,
+.instance_size = sizeof(PPCE500PCIState),
+.class_init= e500_pcihost_class_init,
+},
};
-static void e500_pci_register_types(void)
-{
-type_register_static(&e500_pcihost_info);
-type_register_static(&e500_host_bridge_info);
-}
-
-type_init(e500_pci_register_types)
+DEFINE_TYPES(types)
Re: [PATCH 14/23] hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/gpio/mpc8xxx.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/hw/gpio/mpc8xxx.c b/hw/gpio/mpc8xxx.c
index 63b7a5c881..de183c3be5 100644
--- a/hw/gpio/mpc8xxx.c
+++ b/hw/gpio/mpc8xxx.c
@@ -23,7 +23,6 @@
#include "hw/irq.h"
#include "hw/sysbus.h"
#include "migration/vmstate.h"
-#include "qemu/module.h"
#include "qom/object.h"
#define TYPE_MPC8XXX_GPIO "mpc8xxx_gpio"
@@ -208,17 +207,14 @@ static void mpc8xxx_gpio_class_init(ObjectClass *klass,
void *data)
device_class_set_legacy_reset(dc, mpc8xxx_gpio_reset);
}
-static const TypeInfo mpc8xxx_gpio_info = {
-.name = TYPE_MPC8XXX_GPIO,
-.parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(MPC8XXXGPIOState),
-.instance_init = mpc8xxx_gpio_initfn,
-.class_init= mpc8xxx_gpio_class_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_MPC8XXX_GPIO,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(MPC8XXXGPIOState),
+.instance_init = mpc8xxx_gpio_initfn,
+.class_init= mpc8xxx_gpio_class_init,
+},
};
-static void mpc8xxx_gpio_register_types(void)
-{
-type_register_static(&mpc8xxx_gpio_info);
-}
-
-type_init(mpc8xxx_gpio_register_types)
+DEFINE_TYPES(types)
Re: [PATCH 11/23] hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/i2c/mpc_i2c.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
index 3d79c15653..16f4309ea9 100644
--- a/hw/i2c/mpc_i2c.c
+++ b/hw/i2c/mpc_i2c.c
@@ -20,7 +20,6 @@
#include "qemu/osdep.h"
#include "hw/i2c/i2c.h"
#include "hw/irq.h"
-#include "qemu/module.h"
#include "hw/sysbus.h"
#include "migration/vmstate.h"
#include "qom/object.h"
@@ -345,16 +344,13 @@ static void mpc_i2c_class_init(ObjectClass *klass, void
*data)
dc->desc = "MPC I2C Controller";
}
-static const TypeInfo mpc_i2c_type_info = {
-.name = TYPE_MPC_I2C,
-.parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(MPCI2CState),
-.class_init= mpc_i2c_class_init,
+static const TypeInfo types[] = {
+{
+.name = TYPE_MPC_I2C,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(MPCI2CState),
+.class_init= mpc_i2c_class_init,
+},
};
-static void mpc_i2c_register_types(void)
-{
-type_register_static(&mpc_i2c_type_info);
-}
-
-type_init(mpc_i2c_register_types)
+DEFINE_TYPES(types)
Re: [PATCH 10/23] hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access
On 9/23/24 11:30, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/i2c/mpc_i2c.c| 9 +
hw/i2c/trace-events | 5 +
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
index 2467d1a9aa..3d79c15653 100644
--- a/hw/i2c/mpc_i2c.c
+++ b/hw/i2c/mpc_i2c.c
@@ -24,6 +24,7 @@
#include "hw/sysbus.h"
#include "migration/vmstate.h"
#include "qom/object.h"
+#include "trace.h"
/* #define DEBUG_I2C */
@@ -224,8 +225,8 @@ static uint64_t mpc_i2c_read(void *opaque, hwaddr addr, unsigned size)
break;
}
-DPRINTF("%s: addr " HWADDR_FMT_plx " %02" PRIx32 "\n", __func__,
- addr, value);
+trace_mpc_i2c_read(addr, value);
+
return (uint64_t)value;
}
@@ -234,8 +235,8 @@ static void mpc_i2c_write(void *opaque, hwaddr addr,
{
MPCI2CState *s = opaque;
-DPRINTF("%s: addr " HWADDR_FMT_plx " val %08" PRIx64 "\n", __func__,
- addr, value);
+trace_mpc_i2c_write(addr, value);
+
switch (addr) {
case MPC_I2C_ADR:
s->adr = value & CADR_MASK;
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 6900e06eda..f708a7ace1 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -35,6 +35,11 @@ aspeed_i2c_bus_write(uint32_t busid, uint64_t offset,
unsigned size, uint64_t va
aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send
%d/%d 0x%02x"
aspeed_i2c_bus_recv(const char *mode, int i, int count, uint8_t byte) "%s recv
%d/%d 0x%02x"
+# mpc_i2c.c
+
+mpc_i2c_read(uint64_t addr, uint32_t value) "[0x%" PRIx64 "] -> 0x%02" PRIx32
+mpc_i2c_write(uint64_t addr, uint32_t value) "[0x%" PRIx64 "] <- 0x%02" PRIx32
+
# npcm7xx_smbus.c
npcm7xx_smbus_read(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
Re: [PATCH 05/23] hw/ppc/e500: Add missing device tree properties to i2c controller node
On 9/23/24 11:29, Bernhard Beschow wrote: When compiling a decompiled device tree blob created with dumpdtb, dtc complains with: /soc@e000/i2c@3000: incorrect #address-cells for I2C bus /soc@e000/i2c@3000: incorrect #size-cells for I2C bus Fix this by adding the missing device tree properties. Signed-off-by: Bernhard Beschow Reviewed-by: Cédric Le Goater Thanks, C. --- hw/ppc/e500.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 228287b457..e2a4f265a5 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -196,6 +196,8 @@ static void dt_i2c_create(void *fdt, const char *soc, const char *mpic, qemu_fdt_setprop_cells(fdt, i2c, "cell-index", 0); qemu_fdt_setprop_cells(fdt, i2c, "interrupts", irq0, 0x2); qemu_fdt_setprop_phandle(fdt, i2c, "interrupt-parent", mpic); +qemu_fdt_setprop_cell(fdt, i2c, "#size-cells", 0); +qemu_fdt_setprop_cell(fdt, i2c, "#address-cells", 1); qemu_fdt_setprop_string(fdt, "/aliases", alias, i2c); g_free(i2c);
Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer
On 9/23/24 11:29, Bernhard Beschow wrote:
The env pointer isn't used outside the for loop, so move it inside. After that,
the firstenv pointer is never read, so remove it.
Just wondering, have you considered introducing an PowerPCCPU array
under the machine state ?
This would be an intermediate step towards the introduction of an SoC
model (in the long term)
Thanks,
C.
Signed-off-by: Bernhard Beschow
---
hw/ppc/e500.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 75b051009f..f68779a1ea 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
MachineClass *mc = MACHINE_CLASS(pmc);
PCIBus *pci_bus;
-CPUPPCState *env = NULL;
uint64_t loadaddr;
hwaddr kernel_base = -1LL;
int kernel_size = 0;
@@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
IrqLines *irqs;
DeviceState *dev, *mpicdev;
DriveInfo *dinfo;
-CPUPPCState *firstenv = NULL;
MemoryRegion *ccsr_addr_space;
SysBusDevice *s;
PPCE500CCSRState *ccsr;
@@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
irqs = g_new0(IrqLines, smp_cpus);
for (i = 0; i < smp_cpus; i++) {
PowerPCCPU *cpu;
+CPUPPCState *env;
CPUState *cs;
cpu = POWERPC_CPU(object_new(machine->cpu_type));
@@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
&error_abort);
qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
-if (!firstenv) {
-firstenv = env;
-}
-
irqs[i].irq[OPENPIC_OUTPUT_INT] =
qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
irqs[i].irq[OPENPIC_OUTPUT_CINT] =
@@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
}
}
-env = firstenv;
-
if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
error_report("RAM size must be multiple of %" PRIu64,
RAM_SIZES_ALIGN);
exit(EXIT_FAILURE);
Re: [PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info
On 9/23/24 11:29, Bernhard Beschow wrote:
The struct is allocated once with g_new0() but never free()'d. Fix the leakage
by adding an attribute to struct PPCE500MachineState which avoids the
allocation.
Signed-off-by: Bernhard Beschow
---
hw/ppc/e500.h | 8
hw/ppc/e500.c | 17 -
2 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 8c09ef92e4..557ce6ad93 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -5,10 +5,18 @@
#include "hw/platform-bus.h"
#include "qom/object.h"
+typedef struct boot_info {
+uint32_t dt_base;
+uint32_t dt_size;
+uint32_t entry;
+} boot_info;
or simply move the fields under the machine state struct to avoif
the struct boot_info which doesn't seem that necessary. Is it ?
Thanks,
C.
+
struct PPCE500MachineState {
/*< private >*/
MachineState parent_obj;
+boot_info boot_info;
+
/* points to instance of TYPE_PLATFORM_BUS_DEVICE if
* board supports dynamic sysbus devices
*/
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 3bd12b54ab..75b051009f 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -80,13 +80,6 @@
#define PLATFORM_CLK_FREQ_HZ (400 * 1000 * 1000)
-struct boot_info
-{
-uint32_t dt_base;
-uint32_t dt_size;
-uint32_t entry;
-};
-
static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
int nr_slots, int *len)
{
@@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
bool kernel_as_payload;
hwaddr bios_entry = 0;
target_long payload_size;
-struct boot_info *boot_info = NULL;
int dt_size;
int i;
unsigned int smp_cpus = machine->smp.cpus;
@@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
/* Register reset handler */
if (!i) {
/* Primary CPU */
-boot_info = g_new0(struct boot_info, 1);
qemu_register_reset(ppce500_cpu_reset, cpu);
-env->load_info = boot_info;
+env->load_info = &pms->boot_info;
} else {
/* Secondary CPUs */
qemu_register_reset(ppce500_cpu_reset_sec, cpu);
@@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
}
assert(dt_size < DTB_MAX_SIZE);
-boot_info->entry = bios_entry;
-boot_info->dt_base = dt_base;
-boot_info->dt_size = dt_size;
+pms->boot_info.entry = bios_entry;
+pms->boot_info.dt_base = dt_base;
+pms->boot_info.dt_size = dt_size;
}
static void e500_ccsr_initfn(Object *obj)
Re: [PATCH 23/23] hw/vfio/platform: Let vfio_start_eventfd_injection() take VFIOPlatformDevice pointer
On 9/23/24 11:30, Bernhard Beschow wrote:
Avoids one downcast, making the code more type-safe.
Signed-off-by: Bernhard Beschow
That's a bit unrelated to your e500 series. Anyhow,
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/platform.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index a85c199c76..77bbfbf62c 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -318,13 +318,12 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
/**
* vfio_start_eventfd_injection - starts the virtual IRQ injection using
* user-side handled eventfds
- * @sbdev: the sysbus device handle
+ * @vdev: the VFIO platform device handle
* @irq: the qemu irq handle
*/
-static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
+static void vfio_start_eventfd_injection(VFIOPlatformDevice *vdev, qemu_irq
irq)
{
-VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
VFIOINTp *intp;
QLIST_FOREACH(intp, &vdev->intp_list, next) {
@@ -417,7 +416,7 @@ fail_vfio:
kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
abort();
fail_irqfd:
-vfio_start_eventfd_injection(sbdev, irq);
+vfio_start_eventfd_injection(vdev, irq);
return;
}
Re: [PATCH 00/23] E500 Cleanup
On 9/23/24 23:25, Bernhard Beschow wrote: Am 23. September 2024 20:23:54 UTC schrieb "Cédric Le Goater" : Hello Bernhard, Hi Cédric, On 9/23/24 11:29, Bernhard Beschow wrote: This series is part of a bigger series exploring data-driven machine creation using device tree blobs on top of the e500 machines [1]. It contains patches to make this exploration easier which are also expected to provide value in themselves. The cleanup starts with the e500 machine class itself, then proceeds with machine-specific device models and concludes with more or less loosely related devices. Device cleanup mostly consists of using the DEFINE_TYPES() macro. Since you recently took a look at the machine models, would you be willing to take over maintenance of the e500 ? It shouldn't be an enormous amount of work. Are you referring to the machine and related devices or the CPU? I'm somewhat familiar with the P102x and could take over but the CPU would be a different beast. Please take a look at the MAINTAINERS file. You will see it is not that large and the CPU target models are not part of it. Thanks, C. Best regards, Bernhard Thanks, C. [1] https://github.com/shentok/qemu/tree/e500-fdt Bernhard Beschow (23): hw/ppc/e500: Do not leak struct boot_info hw/ppc/e500: Reduce scope of env pointer hw/ppc/e500: Prefer QOM cast hw/ppc/e500: Remove unused "irqs" parameter hw/ppc/e500: Add missing device tree properties to i2c controller node hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal resources hw/ppc/e500: Extract ppce500_ccsr.c hw/ppc/ppce500_ccsr: Log access to unimplemented registers hw/ppc/mpc8544_guts: Populate POR PLL ratio status register hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch hw/sd/sdhci: Prefer DEFINE_TYPES() macro hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro hw/rtc/ds1338: Prefer DEFINE_TYPES() macro hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro hw/vfio/platform: Let vfio_start_eventfd_injection() take VFIOPlatformDevice pointer MAINTAINERS | 2 +- hw/ppc/e500-ccsr.h | 2 + hw/ppc/e500.h| 8 +++ hw/block/pflash_cfi01.c | 21 +++ hw/gpio/mpc8xxx.c| 22 +++- hw/i2c/mpc_i2c.c | 29 +- hw/i2c/smbus_eeprom.c| 19 +++ hw/net/fsl_etsec/etsec.c | 22 +++- hw/pci-host/ppce500.c| 54 -- hw/ppc/e500.c| 61 +--- hw/ppc/mpc8544_guts.c| 32 +++ hw/ppc/ppce500_ccsr.c| 67 ++ hw/rtc/ds1338.c | 20 +++ hw/sd/sdhci.c| 62 +--- hw/usb/hcd-ehci-sysbus.c | 118 +-- hw/vfio/platform.c | 7 +-- hw/i2c/trace-events | 5 ++ hw/intc/Kconfig | 4 ++ hw/intc/meson.build | 3 +- hw/ppc/meson.build | 1 + hw/ppc/trace-events | 3 + 21 files changed, 285 insertions(+), 277 deletions(-) create mode 100644 hw/ppc/ppce500_ccsr.c
Re: [PATCH 00/23] E500 Cleanup
Hello Bernhard, On 9/23/24 11:29, Bernhard Beschow wrote: This series is part of a bigger series exploring data-driven machine creation using device tree blobs on top of the e500 machines [1]. It contains patches to make this exploration easier which are also expected to provide value in themselves. The cleanup starts with the e500 machine class itself, then proceeds with machine-specific device models and concludes with more or less loosely related devices. Device cleanup mostly consists of using the DEFINE_TYPES() macro. Since you recently took a look at the machine models, would you be willing to take over maintenance of the e500 ? It shouldn't be an enormous amount of work. Thanks, C. [1] https://github.com/shentok/qemu/tree/e500-fdt Bernhard Beschow (23): hw/ppc/e500: Do not leak struct boot_info hw/ppc/e500: Reduce scope of env pointer hw/ppc/e500: Prefer QOM cast hw/ppc/e500: Remove unused "irqs" parameter hw/ppc/e500: Add missing device tree properties to i2c controller node hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal resources hw/ppc/e500: Extract ppce500_ccsr.c hw/ppc/ppce500_ccsr: Log access to unimplemented registers hw/ppc/mpc8544_guts: Populate POR PLL ratio status register hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch hw/sd/sdhci: Prefer DEFINE_TYPES() macro hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro hw/rtc/ds1338: Prefer DEFINE_TYPES() macro hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro hw/vfio/platform: Let vfio_start_eventfd_injection() take VFIOPlatformDevice pointer MAINTAINERS | 2 +- hw/ppc/e500-ccsr.h | 2 + hw/ppc/e500.h| 8 +++ hw/block/pflash_cfi01.c | 21 +++ hw/gpio/mpc8xxx.c| 22 +++- hw/i2c/mpc_i2c.c | 29 +- hw/i2c/smbus_eeprom.c| 19 +++ hw/net/fsl_etsec/etsec.c | 22 +++- hw/pci-host/ppce500.c| 54 -- hw/ppc/e500.c| 61 +--- hw/ppc/mpc8544_guts.c| 32 +++ hw/ppc/ppce500_ccsr.c| 67 ++ hw/rtc/ds1338.c | 20 +++ hw/sd/sdhci.c| 62 +--- hw/usb/hcd-ehci-sysbus.c | 118 +-- hw/vfio/platform.c | 7 +-- hw/i2c/trace-events | 5 ++ hw/intc/Kconfig | 4 ++ hw/intc/meson.build | 3 +- hw/ppc/meson.build | 1 + hw/ppc/trace-events | 3 + 21 files changed, 285 insertions(+), 277 deletions(-) create mode 100644 hw/ppc/ppce500_ccsr.c
Re: [PATCH v16 04/13] s390x/pci: Avoid creating zpci for VFs
Hello,
On 9/13/24 05:44, Akihiko Odaki wrote:
VFs are automatically created by PF, and creating zpci for them will
result in unexpected usage of fids. Currently QEMU does not support
multifunction for s390x so we don't need zpci for VFs anyway.
Signed-off-by: Akihiko Odaki
---
hw/s390x/s390-pci-bus.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 3e57d5faca18..1a620f5b2a04 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1080,6 +1080,16 @@ static void s390_pcihost_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
pbdev = s390_pci_find_dev_by_target(s, dev->id);
if (!pbdev) {
+/*
+ * VFs are automatically created by PF, and creating zpci for them
+ * will result in unexpected usage of fids. Currently QEMU does not
+ * support multifunction for s390x so we don't need zpci for VFs
+ * anyway.
+ */
+if (pci_is_vf(pdev)) {
+return;
+}
+
pbdev = s390_pci_device_new(s, dev->id, errp);
if (!pbdev) {
return;
@@ -1167,7 +1177,9 @@ static void s390_pcihost_unplug(HotplugHandler
*hotplug_dev, DeviceState *dev,
int32_t devfn;
pbdev = s390_pci_find_dev_by_pci(s, PCI_DEVICE(dev));
-g_assert(pbdev);
+if (!pbdev) {
+return;
+}
I don't understand this change. Could you please explain ?
Thanks,
C.
s390_pci_generate_plug_event(HP_EVENT_STANDBY_TO_RESERVED,
pbdev->fh, pbdev->fid);
@@ -1206,7 +1218,10 @@ static void s390_pcihost_unplug_request(HotplugHandler
*hotplug_dev,
* we've checked the PCI device already (to prevent endless
recursion).
*/
pbdev = s390_pci_find_dev_by_pci(s, PCI_DEVICE(dev));
-g_assert(pbdev);
+if (!pbdev) {
+return;
+}
+
pbdev->pci_unplug_request_processed = true;
qdev_unplug(DEVICE(pbdev), errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
Re: [PATCH v16 02/13] hw/ppc/spapr_pci: Do not create DT for disabled PCI device
Hello,
Adding :
Harsh for QEMU/PPC pseries machine,
Shivaprasad for KVM/PPC VFIO and IOMMU support.
Could you please give us your feedback on these changes ?
Thanks,
C.
On 9/13/24 05:44, Akihiko Odaki wrote:
Disabled means it is a disabled SR-IOV VF or it is powered off, and
hidden from the guest.
Signed-off-by: Akihiko Odaki
---
hw/ppc/spapr_pci.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7cf9904c3546..f63182a03c41 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1296,6 +1296,10 @@ static void spapr_dt_pci_device_cb(PCIBus *bus,
PCIDevice *pdev,
return;
}
+if (!pdev->enabled) {
+return;
+}
+
err = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->offset);
if (err < 0) {
p->err = err;
Re: [PATCH v16 03/13] hw/ppc/spapr_pci: Do not reject VFs created after a PF
Adding :
Harsh for QEMU/PPC pseries machine,
Shivaprasad for KVM/PPC VFIO and IOMMU support.
Thanks,
C.
On 9/13/24 05:44, Akihiko Odaki wrote:
A PF may automatically create VFs and the PF may be function 0.
Signed-off-by: Akihiko Odaki
---
hw/ppc/spapr_pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f63182a03c41..ed4454bbf79e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1573,7 +1573,9 @@ static void spapr_pci_pre_plug(HotplugHandler
*plug_handler,
* hotplug, we do not allow functions to be hotplugged to a
* slot that already has function 0 present
*/
-if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
+if (plugged_dev->hotplugged &&
+!pci_is_vf(pdev) &&
+bus->devices[PCI_DEVFN(slotnr, 0)] &&
PCI_FUNC(pdev->devfn) != 0) {
error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
" additional functions can no longer be exposed to guest.",
Re: [PATCH v2 43/48] hw/ppc: remove return after g_assert_not_reached()
On 9/12/24 09:39, Pierrick Bouvier wrote:
This patch is part of a series that moves towards a consistent use of
g_assert_not_reached() rather than an ad hoc mix of different
assertion mechanisms.
Signed-off-by: Pierrick Bouvier
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/ppc/ppc.c | 1 -
hw/ppc/spapr_events.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index e6fa5580c01..fde46194122 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -267,7 +267,6 @@ static void power9_set_irq(void *opaque, int pin, int level)
break;
default:
g_assert_not_reached();
-return;
}
}
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 38ac1cb7866..4dbf8e2e2ef 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -646,7 +646,6 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t
hp_action,
* that don't support them
*/
g_assert_not_reached();
-return;
}
if (hp_id == RTAS_LOG_V6_HP_ID_DRC_COUNT) {
Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements
Better to fix than to guess if there are users, I think.
Yes, but it will require some knowledge of s390x, which I cannot provide.
Commit 57da367b9ec4 ("s390x/pci: forbid multifunction pci device") says having
a multifunction device will make the guest spin forever. That is not what Cédric observed
with igb so it may no longer be relevant, but I cannot be sure that the problem is
resolved without understanding how multifunction devices are supposed to work with s390x.
Ideally someone with s390x expertise should check relevant hardware documentation and confirm QEMU properly implements mutlifunction devices.
I just cc'ed the s390x PCI maintainers on patch 4. Let's see.
Thanks,
C.
Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization
+Matthew +Eric
Side note for the maintainers :
Before this change, the igb device, which is multifunction, was working
fine under Linux.
Was there a fix in Linux since :
57da367b9ec4 ("s390x/pci: forbid multifunction pci device")
6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
?
s390 PCI devices do not have extended capabilities, so the igb device
does not expose the SRIOV capability and only the PF is accessible but
it doesn't seem to be an issue. (Btw, CONFIG_PCI_IOV is set to y in the
default Linux config which is unexpected)
Thanks,
C.
On 8/23/24 07:00, Akihiko Odaki wrote:
The SR-IOV PFs set the multifunction bits during device realization so
check them after that. This forbids adding SR-IOV devices to s390x.
Signed-off-by: Akihiko Odaki
---
hw/s390x/s390-pci-bus.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 3e57d5faca18..00b2c1f6157b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -971,14 +971,7 @@ static void s390_pcihost_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
"this device");
}
-if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-PCIDevice *pdev = PCI_DEVICE(dev);
-
-if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
-error_setg(errp, "multifunction not supported in s390");
-return;
-}
-} else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
+if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
if (!s390_pci_alloc_idx(s, pbdev)) {
@@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
pdev = PCI_DEVICE(dev);
+if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
+error_setg(errp, "multifunction not supported in s390");
+return;
+}
+
if (!dev->id) {
/* In the case the PCI device does not define an id */
/* we generate one based on the PCI address */
Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements
On 9/10/24 15:34, Michael S. Tsirkin wrote:
On Tue, Sep 10, 2024 at 03:21:54PM +0200, Cédric Le Goater wrote:
On 9/10/24 11:33, Akihiko Odaki wrote:
On 2024/09/10 18:21, Michael S. Tsirkin wrote:
On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
Supersedes: <20240714-rombar-v2-0-af1504ef5...@daynix.com>
("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
I submitted a RFC series[1] to add support for SR-IOV emulation to
virtio-net-pci. During the development of the series, I fixed some
trivial bugs and made improvements that I think are independently
useful. This series extracts those fixes and improvements from the RFC
series.
[1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6d...@daynix.com/
Signed-off-by: Akihiko Odaki
I don't think Cédric's issues have been addressed, am I wrong?
Cédric, what is your take?
I put the URI to Cédric's report here:
https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7...@redhat.com
This issue was dealt with patch "s390x/pci: Check for multifunction after device
realization". I found that s390x on QEMU does not support multifunction and SR-IOV
devices accidentally circumvent this restriction, which means igb was never supposed to
work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the
restriction is properly enforced.
yes, indeed and it seems to fix :
6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
I will update patch 4.
Thanks,
C.
That said, the igb device worked perfectly fine under the s390x machine.
And it works for you after this patchset, yes?
ah no, IGB is not an available device for the s390x machine anymore :
qemu-system-s390x: -device igb,netdev=net1,mac=C0:FF:EE:00:00:13:
multifunction not supported in s390
This is what commit 57da367b9ec4 ("s390x/pci: forbid multifunction
pci device") initially required (and later broken by 6069bcdeacee).
So I guess we are fine with the expected behavior.
Thanks,
C.
Re: [PATCH for-9.2 v15 04/11] s390x/pci: Check for multifunction after device realization
On 8/23/24 07:00, Akihiko Odaki wrote:
The SR-IOV PFs set the multifunction bits during device realization so
check them after that. This forbids adding SR-IOV devices to s390x.
Signed-off-by: Akihiko Odaki
May be add :
Fixes: 6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug
handler")
?
Anyhow,
Tested-by: Cédric Le Goater
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/s390x/s390-pci-bus.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 3e57d5faca18..00b2c1f6157b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -971,14 +971,7 @@ static void s390_pcihost_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
"this device");
}
-if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-PCIDevice *pdev = PCI_DEVICE(dev);
-
-if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
-error_setg(errp, "multifunction not supported in s390");
-return;
-}
-} else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
+if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev);
if (!s390_pci_alloc_idx(s, pbdev)) {
@@ -1069,6 +1062,11 @@ static void s390_pcihost_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
pdev = PCI_DEVICE(dev);
+if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
+error_setg(errp, "multifunction not supported in s390");
+return;
+}
+
if (!dev->id) {
/* In the case the PCI device does not define an id */
/* we generate one based on the PCI address */
Re: [PATCH for-9.2 v15 00/11] hw/pci: SR-IOV related fixes and improvements
On 9/10/24 11:33, Akihiko Odaki wrote:
On 2024/09/10 18:21, Michael S. Tsirkin wrote:
On Fri, Aug 23, 2024 at 02:00:37PM +0900, Akihiko Odaki wrote:
Supersedes: <20240714-rombar-v2-0-af1504ef5...@daynix.com>
("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
I submitted a RFC series[1] to add support for SR-IOV emulation to
virtio-net-pci. During the development of the series, I fixed some
trivial bugs and made improvements that I think are independently
useful. This series extracts those fixes and improvements from the RFC
series.
[1]: https://patchew.org/QEMU/20231210-sriov-v2-0-b959e8a6d...@daynix.com/
Signed-off-by: Akihiko Odaki
I don't think Cédric's issues have been addressed, am I wrong?
Cédric, what is your take?
I put the URI to Cédric's report here:
https://lore.kernel.org/r/75cbc7d9-b48e-4235-85cf-49dacf3c7...@redhat.com
This issue was dealt with patch "s390x/pci: Check for multifunction after device
realization". I found that s390x on QEMU does not support multifunction and SR-IOV
devices accidentally circumvent this restriction, which means igb was never supposed to
work with s390x. The patch prevents adding SR-IOV devices to s390x to ensure the
restriction is properly enforced.
yes, indeed and it seems to fix :
6069bcdeacee ("s390x/pci: Move some hotplug checks to the pre_plug handler")
I will update patch 4.
Thanks,
C.
That said, the igb device worked perfectly fine under the s390x machine.
Re: [PATCH v2 19/19] qapi/vfio: Rename VfioMigrationState to Qapi*, and drop prefix
On 9/4/24 13:18, Markus Armbruster wrote:
QAPI's 'prefix' feature can make the connection between enumeration
type and its constants less than obvious. It's best used with
restraint.
VfioMigrationState has a 'prefix' that overrides the generated
enumeration constants' prefix to QAPI_VFIO_MIGRATION_STATE.
We could simply drop 'prefix', but then the enumeration constants
would look as if they came from kernel header linux/vfio.h.
Rename the type to QapiVfioMigrationState instead, so that 'prefix' is
not needed.
Signed-off-by: Markus Armbruster
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
qapi/vfio.json | 9 -
hw/vfio/migration.c | 2 +-
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/qapi/vfio.json b/qapi/vfio.json
index eccca82068..b53b7caecd 100644
--- a/qapi/vfio.json
+++ b/qapi/vfio.json
@@ -7,7 +7,7 @@
##
##
-# @VfioMigrationState:
+# @QapiVfioMigrationState:
#
# An enumeration of the VFIO device migration states.
#
@@ -32,10 +32,9 @@
#
# Since: 9.1
##
-{ 'enum': 'VfioMigrationState',
+{ 'enum': 'QapiVfioMigrationState',
'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
-'pre-copy', 'pre-copy-p2p' ],
- 'prefix': 'QAPI_VFIO_MIGRATION_STATE' }
+'pre-copy', 'pre-copy-p2p' ] }
##
# @VFIO_MIGRATION:
@@ -63,5 +62,5 @@
'data': {
'device-id': 'str',
'qom-path': 'str',
- 'device-state': 'VfioMigrationState'
+ 'device-state': 'QapiVfioMigrationState'
} }
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 262d42a46e..17199b73ae 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -81,7 +81,7 @@ static const char *mig_state_to_str(enum
vfio_device_mig_state state)
}
}
-static VfioMigrationState
+static QapiVfioMigrationState
mig_state_to_qapi_state(enum vfio_device_mig_state state)
{
switch (state) {
Re: [PATCH v5 0/8] virtio-net: add support for SR-IOV emulation
On 7/30/24 19:56, Michael S. Tsirkin wrote:
On Tue, Jul 30, 2024 at 09:26:20PM +0900, Akihiko Odaki wrote:
On 2024/07/30 20:37, Michael S. Tsirkin wrote:
On Mon, Jul 15, 2024 at 02:19:06PM +0900, Akihiko Odaki wrote:
Based-on: <20240714-rombar-v2-0-af1504ef5...@daynix.com>
("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
OK I will revert this for now. We'll try again after the release,
there will be time to address s390.
I'd like to know if anybody wants to use igb on a s390x machine configured
with libvirt. Such a configuration is already kind of broken, and it is
likely to require significant effort on both side of libvirt and QEMU to fix
it.
I assume Cédric wouldn't report it if was unused.
As an alternative, I'm also introducing SR-IOV support to virtio-net-pci. It
does not suffer the same problem with igb thanks to its flexible
configuration mechanism.
Regards,
Akihiko Odaki
Sounds like this needs more review time, anyway.
Using an emulated IGB device in a s390x VM is not a common scenario.
The IGB device is not supported downstream (RHEL on Z). However, the
change broke the s390x machines I use for upstream QEMU development,
I removed the IGB device as a fix for now.
The Z PCI device model has 'uid' and 'fid' properties which are set
by the VMM, and in this case, they are auto-generated, hence the
conflicting ids with libvirt. This is Z specific but, possibly there
are subtle use cases on other platforms which could have similar
consequences. Something to be aware of.
Also, and this is why I thought it was important to report. Creating
PCI devices at machine init time (with -nodefaults) in the back of the
management layer is a no-no. libvirt requests to have full control
on the machine topology and this change seems like a violation of
this rule, even if VFs are kind of special.
Daniel, did I understand correctly the above constraint on the machine
definition ?
I can't say if we should revert for 9.1. Again, this Z is specific.
The changes were introduced to catch errors early when the PF device
is realized. There is no doubt they are useful. Some rework is needed
to avoid the conflicting id issue though.
Thanks,
C.
Re: [PATCH 04/18] qapi/common: Drop temporary 'prefix'
On 7/30/24 10:10, Markus Armbruster wrote:
Recent commit "qapi: Smarter camel_to_upper() to reduce need for
'prefix'" added a temporary 'prefix' to delay changing the generated
code.
Revert it. This improves OffAutoPCIBAR's generated enumeration
constant prefix from OFF_AUTOPCIBAR_OFF to OFF_AUTO_PCIBAR_OFF.
Signed-off-by: Markus Armbruster
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
qapi/common.json | 1 -
hw/vfio/pci.c| 10 +-
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/qapi/common.json b/qapi/common.json
index 25726d3113..7558ce5430 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -92,7 +92,6 @@
# Since: 2.12
##
{ 'enum': 'OffAutoPCIBAR',
- 'prefix': 'OFF_AUTOPCIBAR', # TODO drop
'data': [ 'off', 'auto', 'bar0', 'bar1', 'bar2', 'bar3', 'bar4', 'bar5' ] }
##
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2407720c35..0a99e55247 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1452,7 +1452,7 @@ static bool vfio_pci_relocate_msix(VFIOPCIDevice *vdev,
Error **errp)
int target_bar = -1;
size_t msix_sz;
-if (!vdev->msix || vdev->msix_relo == OFF_AUTOPCIBAR_OFF) {
+if (!vdev->msix || vdev->msix_relo == OFF_AUTO_PCIBAR_OFF) {
return true;
}
@@ -1464,7 +1464,7 @@ static bool vfio_pci_relocate_msix(VFIOPCIDevice *vdev, Error **errp)
/* PCI BARs must be a power of 2 */
msix_sz = pow2ceil(msix_sz);
-if (vdev->msix_relo == OFF_AUTOPCIBAR_AUTO) {
+if (vdev->msix_relo == OFF_AUTO_PCIBAR_AUTO) {
/*
* TODO: Lookup table for known devices.
*
@@ -1479,7 +1479,7 @@ static bool vfio_pci_relocate_msix(VFIOPCIDevice *vdev,
Error **errp)
return false;
}
} else {
-target_bar = (int)(vdev->msix_relo - OFF_AUTOPCIBAR_BAR0);
+target_bar = (int)(vdev->msix_relo - OFF_AUTO_PCIBAR_BAR0);
}
/* I/O port BARs cannot host MSI-X structures */
@@ -1624,7 +1624,7 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev,
Error **errp)
} else if (vfio_pci_is(vdev, PCI_VENDOR_ID_BAIDU,
PCI_DEVICE_ID_KUNLUN_VF)) {
msix->pba_offset = 0xb400;
-} else if (vdev->msix_relo == OFF_AUTOPCIBAR_OFF) {
+} else if (vdev->msix_relo == OFF_AUTO_PCIBAR_OFF) {
error_setg(errp, "hardware reports invalid configuration, "
"MSIX PBA outside of specified BAR");
g_free(msix);
@@ -3403,7 +3403,7 @@ static Property vfio_pci_dev_properties[] = {
nv_gpudirect_clique,
qdev_prop_nv_gpudirect_clique, uint8_t),
DEFINE_PROP_OFF_AUTO_PCIBAR("x-msix-relocation", VFIOPCIDevice, msix_relo,
-OFF_AUTOPCIBAR_OFF),
+OFF_AUTO_PCIBAR_OFF),
#ifdef CONFIG_IOMMUFD
DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
Re: [PATCH v48 00/12] hw/sd/sdcard: Add eMMC support
Hello, On 7/10/24 4:13 PM, Philippe Mathieu-Daudé wrote: Tag to test Aspeed tree: https://gitlab.com/philmd/qemu/-/tags/aspeed_emmc-v8 I gave this branch a try and the rainier machine boots correctly with a baked emmc image : Tested-by: Cédric Le Goater However, I am seeing a *lot* of printf messages : sd_blk_read: read error on host side sd_blk_write: write error on host side which were introduced by the initial implementation a1bb27b1e98a. This doesn't occur with my tree though. Thanks, C. Since v43: - Reordered and squashed commits (Cédric) Since v42: - Stick to spec v4.3 (re-simplified EXT_CSD register & migrate) - Fill CID register - Few changes to CSD register - Implement 'boot-mode' reset timing - Add 'boot-size' property Cédric Le Goater (2): hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR handler (CMD3) hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23) Joel Stanley (1): hw/sd/sdcard: Support boot area in emmc image Luc Michel (1): hw/sd/sdcard: Implement eMMC sleep state (CMD5) Philippe Mathieu-Daudé (6): hw/sd/sdcard: Basis for eMMC support hw/sd/sdcard: Register generic command handlers hw/sd/sdcard: Register unimplemented command handlers hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26) hw/sd/sdcard: Add eMMC 'boot-size' property hw/sd/sdcard: Implement eMMC 'boot-mode' Sai Pavan Boddu (1): hw/sd/sdcard: Add mmc SWITCH function support (CMD6) Vincent Palatin (1): hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8) include/hw/sd/sd.h | 3 + hw/sd/sd.c | 418 - hw/sd/trace-events | 3 + 3 files changed, 418 insertions(+), 6 deletions(-)
Re: [PATCH v48 00/12] hw/sd/sdcard: Add eMMC support
On 7/10/24 6:57 PM, Philippe Mathieu-Daudé wrote: On 10/7/24 17:35, Cédric Le Goater wrote: On 7/10/24 4:13 PM, Philippe Mathieu-Daudé wrote: Tag to test Aspeed tree: https://gitlab.com/philmd/qemu/-/tags/aspeed_emmc-v8 Since v43: - Reordered and squashed commits (Cédric) Cédric Le Goater (2): hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR handler (CMD3) hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23) Joel Stanley (1): hw/sd/sdcard: Support boot area in emmc image Luc Michel (1): hw/sd/sdcard: Implement eMMC sleep state (CMD5) Philippe Mathieu-Daudé (6): hw/sd/sdcard: Basis for eMMC support hw/sd/sdcard: Register generic command handlers hw/sd/sdcard: Register unimplemented command handlers hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26) hw/sd/sdcard: Add eMMC 'boot-size' property hw/sd/sdcard: Implement eMMC 'boot-mode' Sai Pavan Boddu (1): hw/sd/sdcard: Add mmc SWITCH function support (CMD6) Vincent Palatin (1): hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8) include/hw/sd/sd.h | 3 + hw/sd/sd.c | 418 - hw/sd/trace-events | 3 + 3 files changed, 418 insertions(+), 6 deletions(-) Shall we merge now ? Are you OK with this patchset? Sure. let's move forward. I'll adapt the 6/7 patches for aspeed and resend, possibly this week. Thanks, C.
Re: [PATCH v48 00/12] hw/sd/sdcard: Add eMMC support
On 7/10/24 4:13 PM, Philippe Mathieu-Daudé wrote: Tag to test Aspeed tree: https://gitlab.com/philmd/qemu/-/tags/aspeed_emmc-v8 Since v43: - Reordered and squashed commits (Cédric) Since v42: - Stick to spec v4.3 (re-simplified EXT_CSD register & migrate) - Fill CID register - Few changes to CSD register - Implement 'boot-mode' reset timing - Add 'boot-size' property Cédric Le Goater (2): hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR handler (CMD3) hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23) Joel Stanley (1): hw/sd/sdcard: Support boot area in emmc image Luc Michel (1): hw/sd/sdcard: Implement eMMC sleep state (CMD5) Philippe Mathieu-Daudé (6): hw/sd/sdcard: Basis for eMMC support hw/sd/sdcard: Register generic command handlers hw/sd/sdcard: Register unimplemented command handlers hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26) hw/sd/sdcard: Add eMMC 'boot-size' property hw/sd/sdcard: Implement eMMC 'boot-mode' Sai Pavan Boddu (1): hw/sd/sdcard: Add mmc SWITCH function support (CMD6) Vincent Palatin (1): hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8) include/hw/sd/sd.h | 3 + hw/sd/sd.c | 418 - hw/sd/trace-events | 3 + 3 files changed, 418 insertions(+), 6 deletions(-) Shall we merge now ? Thanks, C.
Re: [PATCH v47 00/19] hw/sd/sdcard: Add eMMC support
On 7/10/24 6:58 AM, Cédric Le Goater wrote:
On 7/9/24 11:39 PM, Philippe Mathieu-Daudé wrote:
On 9/7/24 17:58, Cédric Le Goater wrote:
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
Since v42:
- Stick to spec v4.3 (re-simplified EXT_CSD register & migrate)
- Fill CID register
- Few changes to CSD register
- Implement 'boot-mode' reset timing
- Add 'boot-size' property
Change required for aspeed branch:
-- >8 --
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 8c0e36badd..563816b710 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo
*dinfo, bool emmc,
if (emmc) {
- qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
+ qdev_prop_set_uint64(card, "boot-size", 1 * MiB);
}
(I'm still reluctant to merge patches 16-18)...
Then, please drop all changes related to the boot partitions. I will keep
the original patches in my tree and address the feature when I have time.
TYPE_EMMC is already great to have.
As you mentioned on today's community call, eMMC is a chipset soldered
on a board, so our boards exactly knows what size to expect on the card,
and whether boot partitions are present or not.
My remark was on the use of 3 blockdevs to represent the eMMC contents.
1 should be enough.
Having a "boot-size" property to set the BOOT_SIZE_MULT register is fine.
The Aspeed model does some assumption today when installing the first
boot area as a boot ROM :
aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(emmc0), 64 * KiB);
The "boot-size" property will clarify the settings. Please keep it.
64 * KiB is not related to the eMMC partition size. It is a SRAM
limit in which the SoC loads the boot data.
Thanks,
C.
Re: [PATCH v10 06/12] pcie_sriov: Reuse SR-IOV VF device instances
Hello,
This change introduced a regression on s390x. I could have spotted it
earlier. Sorry about that. Here is the scenario,
QEMU now creates automatically the PCI device objects representing the
VFs when the PF device is realized in pcie_sriov_pf_init(). This is
good to report errors early but it has an important drawback.
On s390x, PCI devices have a dual S390PCIBusDevice object. This device
model has 'uid' and 'fid' properties which can be either set by the VMM
or, if not, auto-generated by the S390PCIBusDevice realize handler. In
the VF case, these ids are auto-generated by QEMU and they can possibly
conflict with the uid number space of libvirt. The conflict is detected
when the machine is created and the start is aborted with a message :
2024-07-08T12:51:42.876883Z qemu-system-s390x: -device
{"driver":"zpci","uid":17,"fid":16,"target":"hostdev0","id":"zpci17"}: uid 17
already in use
This problem can occur today with a s390x VM using an IGB device.
It worked fine when the VFs were created at OS runtime because the initial
topology of the machine was in place. Adding VFs was more or less like
hotplug. AIUI, libvirt should have full control on the machine topology
and so, creating VFs in QEMU at init time in the back of libvirt seems
like a violation of this rule.
That said, the s390x case is specific and could perhaps be handled in a
special way.
Thanks,
C.
On 6/27/24 8:07 AM, Akihiko Odaki wrote:
Disable SR-IOV VF devices by reusing code to power down PCI devices
instead of removing them when the guest requests to disable VFs. This
allows to realize devices and report VF realization errors at PF
realization time.
Signed-off-by: Akihiko Odaki
---
include/hw/pci/pci.h| 5 ---
include/hw/pci/pci_device.h | 15 +++
include/hw/pci/pcie_sriov.h | 1 -
hw/pci/pci.c| 2 +-
hw/pci/pcie_sriov.c | 95 +++--
5 files changed, 56 insertions(+), 62 deletions(-)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6c92b2f70008..442017b4865d 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -644,9 +644,4 @@ static inline void pci_irq_pulse(PCIDevice *pci_dev)
MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
void pci_set_enabled(PCIDevice *pci_dev, bool state);
-static inline void pci_set_power(PCIDevice *pci_dev, bool state)
-{
-pci_set_enabled(pci_dev, state);
-}
-
#endif
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index d57f9ce83884..ca151325085d 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -205,6 +205,21 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
return PCI_BUILD_BDF(pci_bus_num(pci_get_bus(dev)), dev->devfn);
}
+static inline void pci_set_power(PCIDevice *pci_dev, bool state)
+{
+/*
+ * Don't change the enabled state of VFs when powering on/off the device.
+ *
+ * When powering on, VFs must not be enabled immediately but they must
+ * wait until the guest configures SR-IOV.
+ * When powering off, their corresponding PFs will be reset and disable
+ * VFs.
+ */
+if (!pci_is_vf(pci_dev)) {
+pci_set_enabled(pci_dev, state);
+}
+}
+
uint16_t pci_requester_id(PCIDevice *dev);
/* DMA access functions */
diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
index aa704e8f9d9f..70649236c18a 100644
--- a/include/hw/pci/pcie_sriov.h
+++ b/include/hw/pci/pcie_sriov.h
@@ -18,7 +18,6 @@
typedef struct PCIESriovPF {
uint16_t num_vfs; /* Number of virtual functions created */
uint8_t vf_bar_type[PCI_NUM_REGIONS]; /* Store type for each VF bar */
-const char *vfname; /* Reference to the device type used for the VFs */
PCIDevice **vf; /* Pointer to an array of num_vfs VF devices */
} PCIESriovPF;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 295a32714a4a..c682c3dcb68e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2822,7 +2822,7 @@ void pci_set_enabled(PCIDevice *d, bool state)
memory_region_set_enabled(&d->bus_master_enable_region,
(pci_get_word(d->config + PCI_COMMAND)
& PCI_COMMAND_MASTER) && d->enabled);
-if (!d->enabled) {
+if (d->qdev.realized) {
pci_device_reset(d);
}
}
diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index f0bde0d3fc79..faadb0d2ea85 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -20,9 +20,16 @@
#include "qapi/error.h"
#include "trace.h"
-static PCIDevice *register_vf(PCIDevice *pf, int devfn,
- const char *name, uint16_t vf_num);
-static void unregister_vfs(PCIDevice *dev);
+static void unparent_vfs(PCIDevice *dev, uint16_t total_vfs)
+{
+for (uint16_t i = 0; i < total_vfs; i++) {
+PCIDevice *vf = dev->exp.sriov_pf.vf[i];
+object_unparent(OBJECT(vf));
+object_unref(OBJECT(vf));
+
Re: [PATCH v47 00/19] hw/sd/sdcard: Add eMMC support
On 7/9/24 11:39 PM, Philippe Mathieu-Daudé wrote:
On 9/7/24 17:58, Cédric Le Goater wrote:
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
Since v42:
- Stick to spec v4.3 (re-simplified EXT_CSD register & migrate)
- Fill CID register
- Few changes to CSD register
- Implement 'boot-mode' reset timing
- Add 'boot-size' property
Change required for aspeed branch:
-- >8 --
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 8c0e36badd..563816b710 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo
*dinfo, bool emmc,
if (emmc) {
- qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
+ qdev_prop_set_uint64(card, "boot-size", 1 * MiB);
}
(I'm still reluctant to merge patches 16-18)...
Then, please drop all changes related to the boot partitions. I will keep
the original patches in my tree and address the feature when I have time.
TYPE_EMMC is already great to have.
As you mentioned on today's community call, eMMC is a chipset soldered
on a board, so our boards exactly knows what size to expect on the card,
and whether boot partitions are present or not.
My remark was on the use of 3 blockdevs to represent the eMMC contents.
1 should be enough.
Having a "boot-size" property to set the BOOT_SIZE_MULT register is fine.
The Aspeed model does some assumption today when installing the first
boot area as a boot ROM :
aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(emmc0), 64 * KiB);
The "boot-size" property will clarify the settings. Please keep it.
Other topic :
ROMs need to be installed early and IIRC, user creatable devices
are not available at that time. So, when the flash devices are be
defined on the command line with :
-blockdev node-name=fmc0,driver=file,filename=./flash.img \
-device mx66u51235f,cs=0x0,bus=ssi.0,drive=fmc0 \
the ROM can not be installed and the initial boot will execute
in place, using SPI transactions to fetch instructions which is
slower. To install a ROM, the -drive API is required.
I find the way of building the drive image described in patch #16 cumbersome,
I agree but that's the layout on real HW. The flash layouts are even more
complex.
but I'm OK
to make some concession on it, since "it works" as it. If necessary
we'll improve and deprecate the current properties.
I don't think we will need to. The properties make sense.
I'll repost and plan to merge as-is (modulo review comments).
OK. I will rebase then.
Thanks,
C.
Re: [PATCH v47 00/19] hw/sd/sdcard: Add eMMC support
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
Since v42:
- Stick to spec v4.3 (re-simplified EXT_CSD register & migrate)
- Fill CID register
- Few changes to CSD register
- Implement 'boot-mode' reset timing
- Add 'boot-size' property
Change required for aspeed branch:
-- >8 --
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 8c0e36badd..563816b710 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo
*dinfo, bool emmc,
if (emmc) {
-qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
+qdev_prop_set_uint64(card, "boot-size", 1 * MiB);
}
(I'm still reluctant to merge patches 16-18)...
Then, please drop all changes related to the boot partitions. I will keep
the original patches in my tree and address the feature when I have time.
TYPE_EMMC is already great to have.
Thanks,
C.
---
Cédric Le Goater (2):
hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR handler (CMD3)
hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23)
Joel Stanley (3):
hw/sd/sdcard: Support boot area in emmc image
hw/sd/sdcard: Subtract bootarea size from blk
hw/sd/sdcard: Add boot config support
Luc Michel (1):
hw/sd/sdcard: Implement eMMC sleep state (CMD5)
Philippe Mathieu-Daudé (11):
hw/sd/sdcard: Basis for eMMC support
hw/sd/sdcard: Register generic command handlers
hw/sd/sdcard: Register unimplemented command handlers
hw/sd/sdcard: Implement emmc_set_cid()
hw/sd/sdcard: Implement emmc_set_csd()
hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26)
hw/sd/sdcard: Add eMMC 'boot-size' property
hw/sd/sdcard: Simplify EXT_CSD values for spec v4.3
hw/sd/sdcard: Migrate ExtCSD 'modes' register
hw/sd/sdcard: Implement eMMC 'boot-mode'
hw/sd/sdcard: Enable TYPE_EMMC card model
Sai Pavan Boddu (1):
hw/sd/sdcard: Add mmc SWITCH function support (CMD6)
Vincent Palatin (1):
hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8)
include/hw/sd/sd.h | 4 +
hw/sd/sd.c | 424 -
hw/sd/trace-events | 3 +
3 files changed, 425 insertions(+), 6 deletions(-)
Re: [Aspeed PATCH v47 18/19] hw/sd/sdcard: Add boot config support
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
From: Joel Stanley
With this correctly set we can use the enable bit to detect if
partition support is enabled.
Signed-off-by: Joel Stanley
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
Also squash?
where is the "boot-config" property gone ?
Thanks,
C.
---
hw/sd/sd.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 291497468f..6aa83251f7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1047,6 +1047,12 @@ static uint32_t emmc_bootpart_offset(SDState *sd)
{
unsigned int access = sd->ext_csd[EXT_CSD_PART_CONFIG]
& EXT_CSD_PART_CONFIG_ACC_MASK;
+unsigned int enable = sd->ext_csd[EXT_CSD_PART_CONFIG]
+ & EXT_CSD_PART_CONFIG_EN_MASK;
+
+if (!enable) {
+return 0;
+}
switch (access) {
case EXT_CSD_PART_CONFIG_ACC_DEFAULT:
Re: [Aspeed PATCH v47 16/19] hw/sd/sdcard: Support boot area in emmc image
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
From: Joel Stanley
This assumes a specially constructed image:
dd if=/dev/zero of=mmc-bootarea.img count=2 bs=1M
dd if=u-boot-spl.bin of=mmc-bootarea.img conv=notrunc
dd if=u-boot.bin of=mmc-bootarea.img conv=notrunc count=64 bs=1K
cat mmc-bootarea.img obmc-phosphor-image.wic > mmc.img
truncate --size 16GB mmc.img
truncate --size 128MB mmc-bootarea.img
For now this still requires a mtd image to load the SPL:
qemu-system-arm -M tacoma-bmc -nographic \
-global driver=sd-card,property=emmc,value=true \
-drive file=mmc.img,if=sd,index=2 \
-drive file=mmc-bootarea.img,if=mtd,format=raw
Signed-off-by: Joel Stanley
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
TODO: Update QEMU command in description
hmm, this patch was also modified since last sent.
Thanks,
C.
---
include/hw/sd/sd.h | 1 +
hw/sd/sd.c | 33 +
2 files changed, 34 insertions(+)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index d35a839f5e..07435d2e17 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -132,6 +132,7 @@ struct SDCardClass {
bool (*get_readonly)(SDState *sd);
void (*set_cid)(SDState *sd);
void (*set_csd)(SDState *sd, uint64_t size);
+uint32_t (*bootpart_offset)(SDState *sd);
const struct SDProto *proto;
};
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c7f8ea11c1..5830725629 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -774,6 +774,13 @@ static uint32_t sd_blk_len(SDState *sd)
return sd->blk_len;
}
+static uint32_t sd_bootpart_offset(SDState *sd)
+{
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(sd);
+
+return sc->bootpart_offset ? sc->bootpart_offset(sd) : 0;
+}
+
static uint64_t sd_req_get_address(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1026,9 +1033,33 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq
insert)
qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
}
+/*
+ * This requires a disk image that has two boot partitions inserted at the
+ * beginning of it. The size of the boot partitions are configured in the
+ * ext_csd structure, which is hardcoded in qemu. They are currently set to
+ * 1MB each.
+ */
+static uint32_t emmc_bootpart_offset(SDState *sd)
+{
+unsigned int access = sd->ext_csd[EXT_CSD_PART_CONFIG]
+ & EXT_CSD_PART_CONFIG_ACC_MASK;
+
+switch (access) {
+case EXT_CSD_PART_CONFIG_ACC_DEFAULT:
+return sd->boot_part_size * 2;
+case EXT_CSD_PART_CONFIG_ACC_BOOT0:
+return 0;
+case EXT_CSD_PART_CONFIG_ACC_BOOT0 + 1:
+return sd->boot_part_size * 1;
+default:
+ g_assert_not_reached();
+}
+}
+
static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
{
trace_sdcard_read_block(addr, len);
+addr += sd_bootpart_offset(sd);
if (!sd->blk || blk_pread(sd->blk, addr, len, sd->data, 0) < 0) {
fprintf(stderr, "sd_blk_read: read error on host side\n");
}
@@ -1037,6 +1068,7 @@ static void sd_blk_read(SDState *sd, uint64_t addr,
uint32_t len)
static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
{
trace_sdcard_write_block(addr, len);
+addr += sd_bootpart_offset(sd);
if (!sd->blk || blk_pwrite(sd->blk, addr, len, sd->data, 0) < 0) {
fprintf(stderr, "sd_blk_write: write error on host side\n");
}
@@ -2871,6 +2903,7 @@ static void emmc_class_init(ObjectClass *klass, void
*data)
sc->set_cid = emmc_set_cid;
sc->set_csd = emmc_set_csd;
+sc->bootpart_offset = emmc_bootpart_offset;
}
static const TypeInfo sd_types[] = {
Re: [PATCH v47 11/19] hw/sd/sdcard: Add eMMC 'boot-size' property
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
Avoid hardcoding 1MiB boot size in EXT_CSD_BOOT_MULT,
expose it as QOM property.
By default, do not use any size. Board is responsible
to set the boot size property.
Signed-off-by: Philippe Mathieu-Daudé
I would move this patch at the end with the other patches adding
boot support.
Thanks,
C.
---
hw/sd/sd.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c809961418..df0e2345c0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -129,6 +129,7 @@ struct SDState {
/* Static properties */
uint8_t spec_version;
+uint64_t boot_part_size;
BlockBackend *blk;
const SDProto *proto;
@@ -490,7 +491,8 @@ static void mmc_set_ext_csd(SDState *sd, uint64_t size)
sd->ext_csd[EXT_CSD_TRIM_MULT] = 0x1; /* Trim multiplier */
sd->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] = 0x15; /* Secure feature */
sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x7; /* Boot information */
-sd->ext_csd[EXT_CSD_BOOT_MULT] = 0x8; /* Boot partition size. 128KB unit */
+ /* Boot partition size. 128KB unit */
+sd->ext_csd[EXT_CSD_BOOT_MULT] = sd->boot_part_size / (128 * KiB);
sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x6; /* Access size */
sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x4; /* HC Erase unit size */
sd->ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] = 0x01; /* HC erase timeout */
@@ -2693,6 +2695,7 @@ static Property sd_properties[] = {
};
static Property emmc_properties[] = {
+DEFINE_PROP_UINT64("boot-size", SDState, boot_part_size, 0),
DEFINE_PROP_END_OF_LIST()
};
Re: [PATCH v47 12/19] hw/sd/sdcard: Simplify EXT_CSD values for spec v4.3
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote: - Set some keys to not defined / implemented: . EXT_CSD_HPI_FEATURES . EXT_CSD_BKOPS_SUPPORT . EXT_CSD_SEC_FEATURE_SUPPORT . EXT_CSD_ERASE_TIMEOUT_MULT . EXT_CSD_PART_SWITCH_TIME . EXT_CSD_OUT_OF_INTERRUPT_TIME - Simplify: . EXT_CSD_ACC_SIZE (6 -> 1) 16KB of super_page_size -> 512B (BDRV_SECTOR_SIZE) . EXT_CSD_HC_ERASE_GRP_SIZE (4 -> 1) . EXT_CSD_HC_WP_GRP_SIZE (4 -> 1) . EXT_CSD_S_C_VCC[Q] (8 -> 1) . EXT_CSD_S_A_TIMEOUT (17 -> 1) . EXT_CSD_CARD_TYPE (7 -> 3) Dual data rate -> High-Speed mode - Update: . EXT_CSD_CARD_TYPE (7 -> 3) High-Speed MultiMediaCard @ 26MHz & 52MHz . Performances (0xa -> 0x46) Class B at 3MB/s. -> Class J at 21MB/s . EXT_CSD_REV (5 -> 3) Rev 1.5 (spec v4.41) -> Rev 1.3 (spec v4.3) - Use load/store API to set EXT_CSD_SEC_CNT - Remove R/W keys, normally zeroed at reset . EXT_CSD_BOOT_INFO Signed-off-by: Philippe Mathieu-Daudé This should be merged in patch 10. Thanks, C. --- hw/sd/sd.c | 50 ++ 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index df0e2345c0..2a687977d1 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -484,43 +484,29 @@ static void mmc_set_ext_csd(SDState *sd, uint64_t size) memset(sd->ext_csd, 0, sizeof(sd->ext_csd)); +/* Properties segment (RO) */ sd->ext_csd[EXT_CSD_S_CMD_SET] = 0b1; /* supported command sets */ -sd->ext_csd[EXT_CSD_HPI_FEATURES] = 0x3; /* HPI features */ -sd->ext_csd[EXT_CSD_BKOPS_SUPPORT] = 0x1; /* Background operations */ -sd->ext_csd[241] = 0xA; /* 1st initialization time after partitioning */ -sd->ext_csd[EXT_CSD_TRIM_MULT] = 0x1; /* Trim multiplier */ -sd->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] = 0x15; /* Secure feature */ -sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x7; /* Boot information */ +sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x0; /* Boot information */ /* Boot partition size. 128KB unit */ sd->ext_csd[EXT_CSD_BOOT_MULT] = sd->boot_part_size / (128 * KiB); -sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x6; /* Access size */ -sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x4; /* HC Erase unit size */ +sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x1; /* Access size */ +sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x01; /* HC Erase unit size */ sd->ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] = 0x01; /* HC erase timeout */ sd->ext_csd[EXT_CSD_REL_WR_SEC_C] = 0x1; /* Reliable write sector count */ -sd->ext_csd[EXT_CSD_HC_WP_GRP_SIZE] = 0x4; /* HC write protect group size */ -sd->ext_csd[EXT_CSD_S_C_VCC] = 0x8; /* Sleep current VCC */ -sd->ext_csd[EXT_CSD_S_C_VCCQ] = 0x7; /* Sleep current VCCQ */ -sd->ext_csd[EXT_CSD_S_A_TIMEOUT] = 0x11; /* Sleep/Awake timeout */ -sd->ext_csd[215] = (sectcount >> 24) & 0xff; /* Sector count */ -sd->ext_csd[214] = (sectcount >> 16) & 0xff; /* ... */ -sd->ext_csd[213] = (sectcount >> 8) & 0xff; /* ... */ -sd->ext_csd[EXT_CSD_SEC_CNT] = (sectcount & 0xff); /* ... */ -sd->ext_csd[210] = 0xa; /* Min write perf for 8bit@52Mhz */ -sd->ext_csd[209] = 0xa; /* Min read perf for 8bit@52Mhz */ -sd->ext_csd[208] = 0xa; /* Min write perf for 4bit@52Mhz */ -sd->ext_csd[207] = 0xa; /* Min read perf for 4bit@52Mhz */ -sd->ext_csd[206] = 0xa; /* Min write perf for 4bit@26Mhz */ -sd->ext_csd[205] = 0xa; /* Min read perf for 4bit@26Mhz */ -sd->ext_csd[EXT_CSD_PART_SWITCH_TIME] = 0x1; -sd->ext_csd[EXT_CSD_OUT_OF_INTERRUPT_TIME] = 0x1; -sd->ext_csd[EXT_CSD_CARD_TYPE] = 0x7; -sd->ext_csd[EXT_CSD_STRUCTURE] = 0x2; -sd->ext_csd[EXT_CSD_REV] = 0x5; -sd->ext_csd[EXT_CSD_RPMB_MULT] = 0x1; /* RPMB size */ -sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3; -sd->ext_csd[159] = 0x00; /* Max enhanced area size */ -sd->ext_csd[158] = 0x00; /* ... */ -sd->ext_csd[157] = 0xEC; /* ... */ +sd->ext_csd[EXT_CSD_HC_WP_GRP_SIZE] = 0x01; /* HC write protect group size */ +sd->ext_csd[EXT_CSD_S_C_VCC] = 0x01; /* Sleep current VCC */ +sd->ext_csd[EXT_CSD_S_C_VCCQ] = 0x01; /* Sleep current VCCQ */ +sd->ext_csd[EXT_CSD_S_A_TIMEOUT] = 0x01; /* Sleep/Awake timeout */ +stl_le_p(&sd->ext_csd[EXT_CSD_SEC_CNT], sectcount); /* Sector count */ +sd->ext_csd[210] = 0x46; /* Min write perf for 8bit@52Mhz */ +sd->ext_csd[209] = 0x46; /* Min read perf for 8bit@52Mhz */ +sd->ext_csd[208] = 0x46; /* Min write perf for 4bit@52Mhz */ +sd->ext_csd[207] = 0x46; /* Min read perf for 4bit@52Mhz */ +sd->ext_csd[206] = 0x46; /* Min write perf for 4bit@26Mhz */ +sd->ext_csd[205] = 0x46; /* Min read perf for 4bit@26Mhz */ +sd->ext_csd[EXT_CSD_CARD_TYPE] = 0b11; +sd->ext_csd[EXT_CSD_STRUCTURE] = 2; +sd->ext_csd[EXT_CSD_REV] = 3; } static void emmc_set_csd(SDState *sd, uint64_t size)
Re: [PATCH v47 19/19] hw/sd/sdcard: Enable TYPE_EMMC card model
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
Now than the implementation is functional, allow
to instantiate it.
Signed-off-by: Philippe Mathieu-Daudé
I don't think this is necessary. emmc should be functional once patch 1-15
are merged. The boot part is an extension.
Thanks,
C.
---
hw/sd/sd.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 6aa83251f7..4a6e9cc035 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2941,7 +2941,6 @@ static const TypeInfo sd_types[] = {
.name = TYPE_EMMC,
.parent = TYPE_SDMMC_COMMON,
.class_init = emmc_class_init,
-.abstract = true, /* FIXME: Remove once model fully functional */
},
};
Re: [PATCH v3 0/8] support AST2700 network
On 7/4/24 10:29 AM, Jamin Lin wrote: change from v1: - ftgmac100 - fix coding style - support 64 bits dma dram address for AST2700 change from v2: - ftgmac100: update memory region size to 0x200. - ftgmac100: introduce a new class(ftgmac100_high), class attribute and memop handlers, for FTGMAC100_*_HIGH regs read/write. - aspeed_ast27x0: update network model to ftgmac100_high to support 64 bits dram address DMA. - m25p80: support quad mode for w25q01jvq change from v3: - ftgmac100: update memory region size to 64KB. - ftgmac100: using a property to activate the region for new registers, instead of a class - ftgmac100: introduce TX and RX ring base address high registers - ftgmac100: split standalone patch for easy review - ftgmac100: update TX and RX packet buffers address to 64 bits - aspeed_ast27x0: set dma64 property for AST2700 ftgmac100 - machine_aspeed.py: update to test sdk v09.02 and network for AST2700 Jamin Lin (8): hw/net:ftgmac100: update memory region size to 64KB hw/net:ftgmac100: update ring base address to 64 bits hw/net:ftgmac100: introduce TX and RX ring base address high registers to support 64 bits hw/net:ftgmac100: update TX and RX packet buffers address to 64 bits aspeed/soc: set dma64 property for AST2700 ftgmac100 hw/block: m25p80: support quad mode for w25q01jvq machine_aspeed.py: update to test ASPEED OpenBMC SDK v09.02 for AST2700 machine_aspeed.py: update to test network for AST2700 hw/arm/aspeed_ast27x0.c | 3 + hw/block/m25p80.c | 16 hw/net/ftgmac100.c | 147 +++- include/hw/net/ftgmac100.h | 17 ++-- tests/avocado/machine_aspeed.py | 12 +-- 5 files changed, 162 insertions(+), 33 deletions(-) Applied to aspeed-next. Thanks, C.
Re: [PATCH v46 0/5] hw/sd/sdcard: Cleanups before adding eMMC support
On 7/3/24 3:43 PM, Philippe Mathieu-Daudé wrote: (patches from v42 already reviewed not reposted) Since v45: - RAZ/WI on GEN_CMD (Luc & Manos) - Rename sd_cmd_SEND_OP_COND - Introduce TYPE_SDMMC_COMMON Philippe Mathieu-Daudé (5): hw/sd/sdcard: Use spec v3.01 by default hw/sd/sdcard: Add sd_cmd_GEN_CMD handler (CMD56) hw/sd/sdcard: Rename sd_cmd_SEND_OP_COND handler hw/sd/sdcard: Introduce set_csd/set_cid handlers hw/sd/sdcard: Extract TYPE_SDMMC_COMMON from TYPE_SD_CARD hw/sd/sdmmc-internal.h | 3 ++ include/hw/sd/sd.h | 2 + hw/core/machine.c | 1 + hw/sd/core.c | 29 ++- hw/sd/sd.c | 113 ++--- 5 files changed, 81 insertions(+), 67 deletions(-) Tested-by: Cédric Le Goater Thanks, C.
Re: [PATCH v3 4/8] hw/net:ftgmac100: update TX and RX packet buffers address to 64 bits
On 7/4/24 10:29 AM, Jamin Lin wrote:
ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.
It have "TXDES 2" and "RXDES 2" to save the high part
physical address of packet buffer.
Ex: TX packet buffer address [34:0]
The "TXDES 2" bits [18:16] which corresponds the bits [34:32]
of the 64 bits address of the TX packet buffer address
and "TXDES 3" bits [31:0] which corresponds the bits [31:0]
of the 64 bits address of the TX packet buffer address.
Update TX and RX packet buffers address type to
64 bits for dram 64 bits address DMA support.
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/net/ftgmac100.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 68956aeb94..80f9cd56d5 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -175,6 +175,8 @@
#define FTGMAC100_TXDES1_TX2FIC (1 << 30)
#define FTGMAC100_TXDES1_TXIC(1 << 31)
+#define FTGMAC100_TXDES2_TXBUF_BADR_HI(x) (((x) >> 16) & 0x7)
+
/*
* Receive descriptor
*/
@@ -208,13 +210,15 @@
#define FTGMAC100_RXDES1_UDP_CHKSUM_ERR (1 << 26)
#define FTGMAC100_RXDES1_IP_CHKSUM_ERR (1 << 27)
+#define FTGMAC100_RXDES2_RXBUF_BADR_HI(x) (((x) >> 16) & 0x7)
+
/*
* Receive and transmit Buffer Descriptor
*/
typedef struct {
uint32_tdes0;
uint32_tdes1;
-uint32_tdes2;/* not used by HW */
+uint32_tdes2;/* used by HW 64 bits DMA */
uint32_tdes3;
} FTGMAC100Desc;
@@ -531,6 +535,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
int frame_size = 0;
uint8_t *ptr = s->frame;
uint64_t addr = tx_descriptor;
+uint64_t buf_addr = 0;
uint32_t flags = 0;
while (1) {
@@ -569,7 +574,12 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t
tx_ring,
len = sizeof(s->frame) - frame_size;
}
-if (dma_memory_read(&address_space_memory, bd.des3,
+buf_addr = bd.des3;
+if (s->dma64) {
+buf_addr = deposit64(buf_addr, 32, 32,
+ FTGMAC100_TXDES2_TXBUF_BADR_HI(bd.des2));
+}
+if (dma_memory_read(&address_space_memory, buf_addr,
ptr, len, MEMTXATTRS_UNSPECIFIED)) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @
0x%x\n",
__func__, bd.des3);
@@ -1022,7 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc,
const uint8_t *buf,
uint32_t flags = 0;
uint64_t addr;
uint32_t crc;
-uint32_t buf_addr;
+uint64_t buf_addr = 0;
uint8_t *crc_ptr;
uint32_t buf_len;
size_t size = len;
@@ -1087,7 +1097,12 @@ static ssize_t ftgmac100_receive(NetClientState *nc,
const uint8_t *buf,
if (size < 4) {
buf_len += size - 4;
}
+
buf_addr = bd.des3;
+if (s->dma64) {
+buf_addr = deposit64(buf_addr, 32, 32,
+ FTGMAC100_RXDES2_RXBUF_BADR_HI(bd.des2));
+}
if (first && proto == ETH_P_VLAN && buf_len >= 18) {
bd.des1 = lduw_be_p(buf + 14) | FTGMAC100_RXDES1_VLANTAG_AVAIL;
Re: [PATCH v3 2/8] hw/net:ftgmac100: update ring base address to 64 bits
On 7/4/24 10:29 AM, Jamin Lin wrote:
Update TX and RX ring base address data type to uint64_t for
64 bits dram address DMA support.
Both "Normal Priority Transmit Ring Base Address Register(0x20)" and
"Receive Ring Base Address Register (0x24)" are used for saving the
low part physical address of descriptor manager.
Therefore, changes to set TX and RX descriptor manager address bits [31:0]
in ftgmac100_read and ftgmac100_write functions.
Incrementing the version of vmstate to 2.
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/net/ftgmac100.c | 33 -
include/hw/net/ftgmac100.h | 9 -
2 files changed, 20 insertions(+), 22 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 9e1f12cd33..d026242e2b 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -515,12 +515,12 @@ out:
return frame_size;
}
-static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
-uint32_t tx_descriptor)
+static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
+uint64_t tx_descriptor)
{
int frame_size = 0;
uint8_t *ptr = s->frame;
-uint32_t addr = tx_descriptor;
+uint64_t addr = tx_descriptor;
uint32_t flags = 0;
while (1) {
@@ -726,9 +726,9 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr,
unsigned size)
case FTGMAC100_MATH1:
return s->math[1];
case FTGMAC100_RXR_BADR:
-return s->rx_ring;
+return extract64(s->rx_ring, 0, 32);
case FTGMAC100_NPTXR_BADR:
-return s->tx_ring;
+return extract64(s->tx_ring, 0, 32);
case FTGMAC100_ITC:
return s->itc;
case FTGMAC100_DBLAC:
@@ -799,9 +799,8 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
HWADDR_PRIx "\n", __func__, value);
return;
}
-
-s->rx_ring = value;
-s->rx_descriptor = s->rx_ring;
+s->rx_ring = deposit64(s->rx_ring, 0, 32, value);
+s->rx_descriptor = deposit64(s->rx_descriptor, 0, 32, value);
break;
case FTGMAC100_RBSR: /* DMA buffer size */
@@ -814,8 +813,8 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
HWADDR_PRIx "\n", __func__, value);
return;
}
-s->tx_ring = value;
-s->tx_descriptor = s->tx_ring;
+s->tx_ring = deposit64(s->tx_ring, 0, 32, value);
+s->tx_descriptor = deposit64(s->tx_descriptor, 0, 32, value);
break;
case FTGMAC100_NPTXPD: /* Trigger transmit */
@@ -957,7 +956,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const
uint8_t *buf,
FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
FTGMAC100Desc bd;
uint32_t flags = 0;
-uint32_t addr;
+uint64_t addr;
uint32_t crc;
uint32_t buf_addr;
uint8_t *crc_ptr;
@@ -1126,18 +1125,14 @@ static void ftgmac100_realize(DeviceState *dev, Error
**errp)
static const VMStateDescription vmstate_ftgmac100 = {
.name = TYPE_FTGMAC100,
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
.fields = (const VMStateField[]) {
VMSTATE_UINT32(irq_state, FTGMAC100State),
VMSTATE_UINT32(isr, FTGMAC100State),
VMSTATE_UINT32(ier, FTGMAC100State),
VMSTATE_UINT32(rx_enabled, FTGMAC100State),
-VMSTATE_UINT32(rx_ring, FTGMAC100State),
VMSTATE_UINT32(rbsr, FTGMAC100State),
-VMSTATE_UINT32(tx_ring, FTGMAC100State),
-VMSTATE_UINT32(rx_descriptor, FTGMAC100State),
-VMSTATE_UINT32(tx_descriptor, FTGMAC100State),
VMSTATE_UINT32_ARRAY(math, FTGMAC100State, 2),
VMSTATE_UINT32(itc, FTGMAC100State),
VMSTATE_UINT32(aptcr, FTGMAC100State),
@@ -1156,6 +1151,10 @@ static const VMStateDescription vmstate_ftgmac100 = {
VMSTATE_UINT32(phy_int_mask, FTGMAC100State),
VMSTATE_UINT32(txdes0_edotr, FTGMAC100State),
VMSTATE_UINT32(rxdes0_edorr, FTGMAC100State),
+VMSTATE_UINT64(rx_ring, FTGMAC100State),
+VMSTATE_UINT64(tx_ring, FTGMAC100State),
+VMSTATE_UINT64(rx_descriptor, FTGMAC100State),
+VMSTATE_UINT64(tx_descriptor, FTGMAC100State),
VMSTATE_END_OF_LIST()
}
};
diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
index 269446e858..aae57ae8cb 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -42,10 +42,6 @@ struct FTGMAC100State {
uint32_t isr;
uint32_t ier;
uint32_t rx_enabled;
-uint32_t rx_ring;
-uint32_t rx_descriptor;
-uint32_t tx_ring;
-uint32_t tx_descriptor;
uint32_t math[2];
uint32_t rbsr;
uint32_t itc;
@@ -
Re: [PATCH v3 8/8] machine_aspeed.py: update to test network for AST2700
On 7/4/24 10:29 AM, Jamin Lin wrote:
Update test case to test network connection via SSH.
Test command:
```
cd build
pyvenv/bin/avocado run
../qemu/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_aarch64_ast2700_evb_sdk_v09_02
```
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
tests/avocado/machine_aspeed.py | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 13fe128fc9..f66ad38d35 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -313,14 +313,14 @@ def do_test_arm_aspeed_sdk_start(self, image):
def do_test_aarch64_aspeed_sdk_start(self, image):
self.vm.set_console()
-self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw')
+self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw',
+ '-net', 'nic', '-net',
'user,hostfwd=:127.0.0.1:0-:22')
self.vm.launch()
self.wait_for_console_pattern('U-Boot 2023.10')
self.wait_for_console_pattern('## Loading kernel from FIT Image')
self.wait_for_console_pattern('Starting kernel ...')
-self.wait_for_console_pattern("systemd[1]: Hostname set to")
@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable on GitLab')
@@ -436,4 +436,6 @@ def test_aarch64_ast2700_evb_sdk_v09_02(self):
self.vm.add_args('-smp', str(num_cpu))
self.do_test_aarch64_aspeed_sdk_start(image_dir + 'image-bmc')
+self.wait_for_console_pattern('nodistro.0 ast2700-default ttyS12')
+self.ssh_connect('root', '0penBmc', False)
Re: [PATCH v3 5/8] aspeed/soc: set dma64 property for AST2700 ftgmac100
On 7/4/24 10:29 AM, Jamin Lin wrote:
ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.
Set dma64 property for ftgmac100 model to support
64bits dram address DMA.
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/arm/aspeed_ast27x0.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 18e6a8b10c..a9fb0d4b88 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -552,9 +552,12 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev,
Error **errp)
return;
}
+/* Net */
for (i = 0; i < sc->macs_num; i++) {
object_property_set_bool(OBJECT(&s->ftgmac100[i]), "aspeed", true,
&error_abort);
+object_property_set_bool(OBJECT(&s->ftgmac100[i]), "dma64", true,
+ &error_abort);
if (!sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), errp)) {
return;
}
Re: [PATCH v3 7/8] machine_aspeed.py: update to test ASPEED OpenBMC SDK v09.02 for AST2700
On 7/4/24 10:29 AM, Jamin Lin wrote:
Update test case to test ASPEED OpenBMC SDK v09.02 for AST2700.
ASPEED fixed TX mask issue from linux/drivers/ftgmac100.c.
It is required to use ASPEED OpenBMC SDK since v09.02
for AST2700 QEMU network testing.
A test image is downloaded from the ASPEED Forked OpenBMC GitHub
release repository :
https://github.com/AspeedTech-BMC/openbmc/releases/
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
tests/avocado/machine_aspeed.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 3a20644fb2..13fe128fc9 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -387,15 +387,15 @@ def test_arm_ast2600_evb_sdk(self):
year = time.strftime("%Y")
self.ssh_command_output_contains('/sbin/hwclock -f /dev/rtc1', year);
-def test_aarch64_ast2700_evb_sdk_v09_01(self):
+def test_aarch64_ast2700_evb_sdk_v09_02(self):
"""
:avocado: tags=arch:aarch64
:avocado: tags=machine:ast2700-evb
"""
image_url = ('https://github.com/AspeedTech-BMC/openbmc/releases/'
- 'download/v09.01/ast2700-default-obmc.tar.gz')
-image_hash =
'b1cc0fd73c7650d34c9c8459a243f52a91e9e27144b8608b2645ab19461d1e07'
+ 'download/v09.02/ast2700-default-obmc.tar.gz')
+image_hash =
'ac969c2602f4e6bdb69562ff466b89ae3fe1d86e1f6797bb7969d787f82116a7'
image_path = self.fetch_asset(image_url, asset_hash=image_hash,
algorithm='sha256')
archive.extract(image_path, self.workdir)
Re: [PATCH v3 3/8] hw/net:ftgmac100: introduce TX and RX ring base address high registers to support 64 bits
On 7/4/24 10:29 AM, Jamin Lin wrote:
ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.
It have "Normal Priority Transmit Ring Base Address Register High(0x17C)",
"High Priority Transmit Ring Base Address Register High(0x184)" and
"Receive Ring Base Address Register High(0x18C)" to save the high part physical
address of descriptor manager.
Ex: TX descriptor manager address [34:0]
The "Normal Priority Transmit Ring Base Address Register High(0x17C)"
bits [2:0] which corresponds the bits [34:32] of the 64 bits address of
the TX ring buffer address.
The "Normal Priority Transmit Ring Base Address Register(0x20)" bits [31:0]
which corresponds the bits [31:0] of the 64 bits address
of the TX ring buffer address.
Introduce a new sub region which size is 0x100 for the set of new registers
and map it at 0x100 in the container region.
This sub region range is from 0x100 to 0x1ff.
Introduce a new property and object attribute to activate the region for new
registers.
Introduce a new memop handlers for the new register read and write.
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/net/ftgmac100.c | 82 ++
include/hw/net/ftgmac100.h | 4 ++
2 files changed, 86 insertions(+)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index d026242e2b..68956aeb94 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -56,6 +56,16 @@
#define FTGMAC100_PHYDATA 0x64
#define FTGMAC100_FCR 0x68
+/*
+ * FTGMAC100 registers high
+ *
+ * values below are offset by - FTGMAC100_REG_HIGH_OFFSET from datasheet
+ * because its memory region is start at FTGMAC100_REG_HIGH_OFFSET
+ */
+#define FTGMAC100_NPTXR_BADR_HIGH (0x17C - FTGMAC100_REG_HIGH_OFFSET)
+#define FTGMAC100_HPTXR_BADR_HIGH (0x184 - FTGMAC100_REG_HIGH_OFFSET)
+#define FTGMAC100_RXR_BADR_HIGH (0x18C - FTGMAC100_REG_HIGH_OFFSET)
+
/*
* Interrupt status register & interrupt enable register
*/
@@ -913,6 +923,60 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
ftgmac100_update_irq(s);
}
+static uint64_t ftgmac100_high_read(void *opaque, hwaddr addr, unsigned size)
+{
+FTGMAC100State *s = FTGMAC100(opaque);
+uint64_t val = 0;
+
+switch (addr) {
+case FTGMAC100_NPTXR_BADR_HIGH:
+val = extract64(s->tx_ring, 32, 32);
+break;
+case FTGMAC100_HPTXR_BADR_HIGH:
+/* High Priority Transmit Ring Base High Address */
+qemu_log_mask(LOG_UNIMP, "%s: read to unimplemented register 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+break;
+case FTGMAC100_RXR_BADR_HIGH:
+val = extract64(s->rx_ring, 32, 32);
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+break;
+}
+
+return val;
+}
+
+static void ftgmac100_high_write(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size)
+{
+FTGMAC100State *s = FTGMAC100(opaque);
+
+switch (addr) {
+case FTGMAC100_NPTXR_BADR_HIGH:
+s->tx_ring = deposit64(s->tx_ring, 32, 32, value);
+s->tx_descriptor = deposit64(s->tx_descriptor, 32, 32, value);
+break;
+case FTGMAC100_HPTXR_BADR_HIGH:
+/* High Priority Transmit Ring Base High Address */
+qemu_log_mask(LOG_UNIMP, "%s: write to unimplemented register 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+break;
+case FTGMAC100_RXR_BADR_HIGH:
+s->rx_ring = deposit64(s->rx_ring, 32, 32, value);
+s->rx_descriptor = deposit64(s->rx_descriptor, 32, 32, value);
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+break;
+}
+
+ftgmac100_update_irq(s);
+}
+
static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
{
unsigned mcast_idx;
@@ -1077,6 +1141,14 @@ static const MemoryRegionOps ftgmac100_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static const MemoryRegionOps ftgmac100_high_ops = {
+.read = ftgmac100_high_read,
+.write = ftgmac100_high_write,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
static void ftgmac100_cleanup(NetClientState *nc)
{
FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
@@ -1114,6 +1186,15 @@ static void ftgmac100_realize(DeviceState *dev, Error
**errp)
TYPE_FTGMAC100 ".regs", FTGMAC100_REG_MEM_SIZE);
memory_region_add_subregion(&s->iomem_c
Re: [PATCH v3 1/8] hw/net:ftgmac100: update memory region size to 64KB
On 7/4/24 10:29 AM, Jamin Lin wrote:
According to the datasheet of ASPEED SOCs,
one MAC controller owns 128KB of register space for AST2500.
However, one MAC controller only owns 64KB of register space for AST2600
and AST2700. It set the memory region size 128KB and it occupied another
controllers Address Spaces.
Update one MAC controller memory region size to 0x1000
because AST2500 did not use register spaces over than 64KB.
Introduce a new container region size to 0x1000 and its range
is from 0 to 0xfff. This container is mapped a sub region
for the current set of register.
This sub region range is from 0 to 0xff.
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/net/ftgmac100.c | 11 ---
include/hw/net/ftgmac100.h | 4
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 25e4c0cd5b..9e1f12cd33 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -1107,9 +1107,14 @@ static void ftgmac100_realize(DeviceState *dev, Error
**errp)
s->rxdes0_edorr = FTGMAC100_RXDES0_EDORR;
}
-memory_region_init_io(&s->iomem, OBJECT(dev), &ftgmac100_ops, s,
- TYPE_FTGMAC100, 0x2000);
-sysbus_init_mmio(sbd, &s->iomem);
+memory_region_init(&s->iomem_container, OBJECT(s),
+ TYPE_FTGMAC100 ".container", FTGMAC100_MEM_SIZE);
+sysbus_init_mmio(sbd, &s->iomem_container);
+
+memory_region_init_io(&s->iomem, OBJECT(s), &ftgmac100_ops, s,
+ TYPE_FTGMAC100 ".regs", FTGMAC100_REG_MEM_SIZE);
+memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
+
sysbus_init_irq(sbd, &s->irq);
qemu_macaddr_default_if_unset(&s->conf.macaddr);
diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
index 765d1538a4..269446e858 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -14,6 +14,9 @@
#define TYPE_FTGMAC100 "ftgmac100"
OBJECT_DECLARE_SIMPLE_TYPE(FTGMAC100State, FTGMAC100)
+#define FTGMAC100_MEM_SIZE 0x1000
+#define FTGMAC100_REG_MEM_SIZE 0x100
+
#include "hw/sysbus.h"
#include "net/net.h"
@@ -30,6 +33,7 @@ struct FTGMAC100State {
NICState *nic;
NICConf conf;
qemu_irq irq;
+MemoryRegion iomem_container;
MemoryRegion iomem;
uint8_t frame[FTGMAC100_MAX_FRAME_SIZE];
Re: [PATCH v2 2/5] hw/net:ftgmac100: support 64 bits dma dram address for AST2700
Hello Jamin,
I think that, first, we should introduce a container region. In this container
region would be mapped a sub region for the current set of registers. This
container region would be the one that the SoC maps as it is done today.
Then, in a second patch, we should introduce a extra sub region for the set of
new registers and map it at 0x100 in the container region.
It is close to what you did but it lacks an overall container region.
This container region should be sized as specified in the datasheet.
Do you mean to change as following?
ftgmac100.h
#define FTGMAC100_MEM_SIZE 0x200
I would use the total size of the MMIO aperture, 0x1000, because
this region is reserved for the MAC unit logic, which means it
could grow. That's minor.
#define FTGMAC100_NR_REGS 0x100
Value is fine.
However, the NR_REGS suffix is confusing. It is not a number of
registers but a MMIO aperture width. I would use a _MEM_SIZE suffix
instead. Could be FTGMAC100_REG_MEM_SIZE.
#define FTGMAC100_REGS_HIGH_OFFSET 0x100
#define FTGMAC100_NR_REGS_HIGH 0x100
Same here.
struct FTGMAC100State {
MemoryRegion iomem_container;
MemoryRegion iomem;
MemoryRegion iomem_high;
}
Ftgmac100.c
static void ftgmac100_realize(DeviceState *dev, Error **errp)
{
memory_region_init(&s->iomem_container, OBJECT(s),
TYPE_FTGMAC100 ".container", FTGMAC100_MEM_SIZE); -->
container size 0x200
sysbus_init_mmio(sbd, &s->iomem_container);
memory_region_init_io(&s->iomem, OBJECT(s), &ftgmac100_ops, s,
TYPE_FTGMAC100 ".regs", FTGMAC100_NR_REGS); -->
current register 0x0-0xff
memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
if (s-> dma64) {
memory_region_init_io(&s->iomem_high, OBJECT(s), &ftgmac100_high_ops,
s, TYPE_FTGMAC100 ".regs.high",
FTGMAC100_NR_REGS_HIGH); --> high register 0x100-0x1ff
memory_region_add_subregion(&s->iomem_container,
FTGMAC100_REGS_HIGH_OFFSET,
&s->iomem_high);
}
}
Looks good.
Thanks,
C.
Re: [PATCH v46 5/5] hw/sd/sdcard: Extract TYPE_SDMMC_COMMON from TYPE_SD_CARD
On 7/3/24 4:07 PM, Philippe Mathieu-Daudé wrote:
On 3/7/24 16:02, Cédric Le Goater wrote:
On 7/3/24 3:43 PM, Philippe Mathieu-Daudé wrote:
In order to keep eMMC model simpler to maintain,
extract common properties and the common code from
class_init to the (internal) TYPE_SDMMC_COMMON.
Update the corresponding QOM cast macros.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sdmmc-internal.h | 3 +++
hw/sd/core.c | 29
hw/sd/sd.c | 50 --
3 files changed, 52 insertions(+), 30 deletions(-)
@@ -2508,14 +2521,19 @@ static void sd_spi_class_init(ObjectClass *klass, void
*data)
static const TypeInfo sd_types[] = {
{
- .name = TYPE_SD_CARD,
+ .name = TYPE_SDMMC_COMMON,
.parent = TYPE_DEVICE,
.instance_size = sizeof(SDState),
.class_size = sizeof(SDCardClass),
- .class_init = sd_class_init,
+ .class_init = sdmmc_common_class_init,
.instance_init = sd_instance_init,
.instance_finalize = sd_instance_finalize,
},
Shouldn't it be an abstract class ?
Ah yes, safer. Squashing:
-- >8 --
@@ -2513,6 +2513,7 @@ static const TypeInfo sd_types[] = {
{
.name = TYPE_SDMMC_COMMON,
.parent = TYPE_DEVICE,
+ .abstract = true,
.instance_size = sizeof(SDState),
.class_size = sizeof(SDCardClass),
.class_init = sdmmc_common_class_init,
with that,
Reviewed-by: Cédric Le Goater
Thanks,
C.
Re: [PATCH v46 5/5] hw/sd/sdcard: Extract TYPE_SDMMC_COMMON from TYPE_SD_CARD
On 7/3/24 3:43 PM, Philippe Mathieu-Daudé wrote:
In order to keep eMMC model simpler to maintain,
extract common properties and the common code from
class_init to the (internal) TYPE_SDMMC_COMMON.
Update the corresponding QOM cast macros.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sdmmc-internal.h | 3 +++
hw/sd/core.c | 29
hw/sd/sd.c | 50 --
3 files changed, 52 insertions(+), 30 deletions(-)
diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index cc0b69e834..91eb5b6b2f 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -11,6 +11,9 @@
#ifndef SDMMC_INTERNAL_H
#define SDMMC_INTERNAL_H
+#define TYPE_SDMMC_COMMON "sdmmc-common"
+DECLARE_OBJ_CHECKERS(SDState, SDCardClass, SDMMC_COMMON, TYPE_SDMMC_COMMON)
+
/*
* EXT_CSD Modes segment
*
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 52d5d90045..4b30218b52 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -24,6 +24,7 @@
#include "hw/sd/sd.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "sdmmc-internal.h"
#include "trace.h"
static inline const char *sdbus_name(SDBus *sdbus)
@@ -39,7 +40,7 @@ static SDState *get_card(SDBus *sdbus)
if (!kid) {
return NULL;
}
-return SD_CARD(kid->child);
+return SDMMC_COMMON(kid->child);
}
uint8_t sdbus_get_dat_lines(SDBus *sdbus)
@@ -48,7 +49,7 @@ uint8_t sdbus_get_dat_lines(SDBus *sdbus)
uint8_t dat_lines = 0b; /* 4 bit bus width */
if (slave) {
-SDCardClass *sc = SD_CARD_GET_CLASS(slave);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(slave);
if (sc->get_dat_lines) {
dat_lines = sc->get_dat_lines(slave);
@@ -65,7 +66,7 @@ bool sdbus_get_cmd_line(SDBus *sdbus)
bool cmd_line = true;
if (slave) {
-SDCardClass *sc = SD_CARD_GET_CLASS(slave);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(slave);
if (sc->get_cmd_line) {
cmd_line = sc->get_cmd_line(slave);
@@ -82,7 +83,7 @@ void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts)
trace_sdbus_set_voltage(sdbus_name(sdbus), millivolts);
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
assert(sc->set_voltage);
sc->set_voltage(card, millivolts);
@@ -95,7 +96,7 @@ int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t
*response)
trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg);
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
return sc->do_command(card, req, response);
}
@@ -109,7 +110,7 @@ void sdbus_write_byte(SDBus *sdbus, uint8_t value)
trace_sdbus_write(sdbus_name(sdbus), value);
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
sc->write_byte(card, value);
}
@@ -121,7 +122,7 @@ void sdbus_write_data(SDBus *sdbus, const void *buf, size_t
length)
const uint8_t *data = buf;
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
for (size_t i = 0; i < length; i++) {
trace_sdbus_write(sdbus_name(sdbus), data[i]);
@@ -136,7 +137,7 @@ uint8_t sdbus_read_byte(SDBus *sdbus)
uint8_t value = 0;
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
value = sc->read_byte(card);
}
@@ -151,7 +152,7 @@ void sdbus_read_data(SDBus *sdbus, void *buf, size_t length)
uint8_t *data = buf;
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
for (size_t i = 0; i < length; i++) {
data[i] = sc->read_byte(card);
@@ -165,7 +166,7 @@ bool sdbus_receive_ready(SDBus *sdbus)
SDState *card = get_card(sdbus);
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
return sc->receive_ready(card);
}
@@ -178,7 +179,7 @@ bool sdbus_data_ready(SDBus *sdbus)
SDState *card = get_card(sdbus);
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
return sc->data_ready(card);
}
@@ -191,7 +192,7 @@ bool sdbus_get_inserted(SDBus *sdbus)
SDState *card = get_card(sdbus);
if (card) {
-SDCardClass *sc = SD_CARD_GET_CLASS(card);
+SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
return sc->get_inserted(card);
}
@@ -204,7 +205,7 @@ bool sdbus_get_readonly(SDBus *sdbus)
SDState *card = get_card(sdbus);
Re: [PATCH v46 3/5] hw/sd/sdcard: Rename sd_cmd_SEND_OP_COND handler
On 7/3/24 3:43 PM, Philippe Mathieu-Daudé wrote:
The correct command name is 'SD SEND_OP_COND',
rename accordingly.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 04e8fdb262..10f2764a53 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1701,7 +1701,7 @@ static sd_rsp_type_t
sd_acmd_SET_WR_BLK_ERASE_COUNT(SDState *sd, SDRequest req)
}
/* ACMD41 */
-static sd_rsp_type_t sd_acmd_SD_APP_OP_COND(SDState *sd, SDRequest req)
+static sd_rsp_type_t sd_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
{
if (sd->state != sd_idle_state) {
return sd_invalid_state_for_cmd(sd, req);
@@ -2378,7 +2378,7 @@ static const SDProto sd_proto_sd = {
[13] = {8, sd_adtc, "SD_STATUS", sd_acmd_SD_STATUS},
[22] = {8, sd_adtc, "SEND_NUM_WR_BLOCKS",
sd_acmd_SEND_NUM_WR_BLOCKS},
[23] = {8, sd_ac, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
-[41] = {8, sd_bcr, "SD_APP_OP_COND", sd_acmd_SD_APP_OP_COND},
+[41] = {8, sd_bcr, "SEND_OP_COND", sd_cmd_SEND_OP_COND},
[42] = {8, sd_ac, "SET_CLR_CARD_DETECT",
sd_acmd_SET_CLR_CARD_DETECT},
[51] = {8, sd_adtc, "SEND_SCR", sd_acmd_SEND_SCR},
},
Re: [PATCH v46 4/5] hw/sd/sdcard: Introduce set_csd/set_cid handlers
On 7/3/24 3:43 PM, Philippe Mathieu-Daudé wrote:
In preparation of introducing eMMC support which have
different CSD/CID structures, introduce a pair of handlers
in SDCardClass.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
include/hw/sd/sd.h | 2 ++
hw/sd/sd.c | 7 +--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 29c76935a0..bfbc83c110 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -128,6 +128,8 @@ struct SDCardClass {
void (*enable)(SDState *sd, bool enable);
bool (*get_inserted)(SDState *sd);
bool (*get_readonly)(SDState *sd);
+void (*set_cid)(SDState *sd);
+void (*set_csd)(SDState *sd, uint64_t size);
const struct SDProto *proto;
};
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 10f2764a53..d46be50760 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -689,6 +689,7 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
static void sd_reset(DeviceState *dev)
{
SDState *sd = SD_CARD(dev);
+SDCardClass *sc = SD_CARD_GET_CLASS(sd);
uint64_t size;
uint64_t sect;
@@ -709,8 +710,8 @@ static void sd_reset(DeviceState *dev)
sd->size = size;
sd_set_ocr(sd);
sd_set_scr(sd);
-sd_set_cid(sd);
-sd_set_csd(sd, size);
+sc->set_cid(sd);
+sc->set_csd(sd, size);
sd_set_cardstatus(sd);
sd_set_sdstatus(sd);
@@ -2485,6 +2486,8 @@ static void sd_class_init(ObjectClass *klass, void *data)
sc->enable = sd_enable;
sc->get_inserted = sd_get_inserted;
sc->get_readonly = sd_get_readonly;
+sc->set_cid = sd_set_cid;
+sc->set_csd = sd_set_csd;
sc->proto = &sd_proto_sd;
}
Re: [PATCH v2 2/5] hw/net:ftgmac100: support 64 bits dma dram address for AST2700
On 7/3/24 10:16 AM, Jamin Lin wrote:
ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.
It have "Normal Priority Transmit Ring Base Address Register High(0x17C)",
"High Priority Transmit Ring Base Address Register High(0x184)" and
"Receive Ring Base Address Register High(0x18C)" to save the high part physical
address of descriptor manager.
Ex: TX descriptor manager address [34:0]
The "Normal Priority Transmit Ring Base Address Register High(0x17C)"
bits [2:0] which corresponds the bits [34:32] of the 64 bits address of
the TX ring buffer address.
The "Normal Priority Transmit Ring Base Address Register(0x20)" bits [31:0]
which corresponds the bits [31:0] of the 64 bits address
of the TX ring buffer address.
Besides, it have "TXDES 2" and "RXDES 2"
to save the high part physical address of packet buffer.
Ex: TX packet buffer address [34:0]
The "TXDES 2" bits [18:16] which corresponds the bits [34:32]
of the 64 bits address of the TX packet buffer address
and "TXDES 3" bits [31:0] which corresponds the bits [31:0]
of the 64 bits address of the TX packet buffer address.
Update TX/RX ring and descriptor data type to uint64_t
and supports TX/RX ring, descriptor and packet buffers
64 bits address for all ASPEED SOCs models.
Incrementing the version of vmstate to 2.
Introduce a new class(ftgmac100_high),
class attribute and memop handlers,
the realize handler instantiate a new memory
region and map it on top of the current one to
read/write FTGMAC100_*_HIGH regs.
Signed-off-by: Jamin Lin
---
hw/net/ftgmac100.c | 156 -
include/hw/net/ftgmac100.h | 24 --
2 files changed, 151 insertions(+), 29 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 4e88430b2f..3d13f54efc 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -56,6 +56,16 @@
#define FTGMAC100_PHYDATA 0x64
#define FTGMAC100_FCR 0x68
+/*
+ * FTGMAC100 High registers
+ *
+ * values below are offset by - FTGMAC100_HIGH_OFFSET from datasheet
+ * because its memory region is start at FTGMAC100_HIGH_OFFSET
+ */
+#define FTGMAC100_NPTXR_BADR_HIGH (0x17C - FTGMAC100_HIGH_OFFSET)
+#define FTGMAC100_HPTXR_BADR_HIGH (0x184 - FTGMAC100_HIGH_OFFSET)
+#define FTGMAC100_RXR_BADR_HIGH (0x18C - FTGMAC100_HIGH_OFFSET)
+
/*
* Interrupt status register & interrupt enable register
*/
@@ -165,6 +175,8 @@
#define FTGMAC100_TXDES1_TX2FIC (1 << 30)
#define FTGMAC100_TXDES1_TXIC(1 << 31)
+#define FTGMAC100_TXDES2_TXBUF_BADR_HI(x) (((x) >> 16) & 0x7)
+
/*
* Receive descriptor
*/
@@ -198,13 +210,15 @@
#define FTGMAC100_RXDES1_UDP_CHKSUM_ERR (1 << 26)
#define FTGMAC100_RXDES1_IP_CHKSUM_ERR (1 << 27)
+#define FTGMAC100_RXDES2_RXBUF_BADR_HI(x) (((x) >> 16) & 0x7)
+
/*
* Receive and transmit Buffer Descriptor
*/
typedef struct {
uint32_tdes0;
uint32_tdes1;
-uint32_tdes2;/* not used by HW */
+uint32_tdes2;/* used by HW high address */
uint32_tdes3;
} FTGMAC100Desc;
@@ -515,12 +529,14 @@ out:
return frame_size;
}
-static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
-uint32_t tx_descriptor)
+static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
+uint64_t tx_descriptor)
{
+FTGMAC100Class *fc = FTGMAC100_GET_CLASS(s);
int frame_size = 0;
uint8_t *ptr = s->frame;
-uint32_t addr = tx_descriptor;
+uint64_t addr = tx_descriptor;
+uint64_t buf_addr = 0;
To ease reading, I would extract in a standalone patch the part converting
addresses to 64 bits.
uint32_t flags = 0;
while (1) {
@@ -559,7 +575,12 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t
tx_ring,
len = sizeof(s->frame) - frame_size;
}
-if (dma_memory_read(&address_space_memory, bd.des3,
+buf_addr = bd.des3;
+if (fc->is_dma64) {
+buf_addr = deposit64(buf_addr, 32, 32,
+ FTGMAC100_TXDES2_TXBUF_BADR_HI(bd.des2));
+}
+if (dma_memory_read(&address_space_memory, buf_addr,
ptr, len, MEMTXATTRS_UNSPECIFIED)) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @
0x%x\n",
__func__, bd.des3);
@@ -726,9 +747,9 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr,
unsigned size)
case FTGMAC100_MATH1:
return s->math[1];
case FTGMAC100_RXR_BADR:
-return s->rx_ring;
+return extract64(s->rx_ring, 0, 32);
case FTGMAC100_NPTXR_BADR:
-return s->tx_ring;
+return extract64(s->tx_ring, 0, 32);
case FTGMAC100_ITC:
return s->itc;
case FTGMAC100_DBLAC:
@@ -799,9 +820,8 @@ stati
Re: [PATCH v2 5/5] test/avocado/machine_aspeed.py: update to test network for AST2700
On 7/3/24 10:16 AM, Jamin Lin wrote:
Update a test case to test network connection via ssh and
changes to test Aspeed OpenBMC SDK v09.02 for AST2700.
ASPEED fixed TX mask issue from linux/drivers/ftgmac100.c.
It is required to use ASPEED SDK image since v09.02
for AST2700 QEMU network testing.
A test image is downloaded from the ASPEED Forked OpenBMC GitHub
release repository :
https://github.com/AspeedTech-BMC/openbmc/releases/
Test command:
```
cd build
pyvenv/bin/avocado run
../qemu/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_aarch64_ast2700_evb_sdk_v09_02
```
Signed-off-by: Jamin Lin
Could you please split the patch ? The change of SDK should be
a standalone patch.
Thanks,
C.
---
tests/avocado/machine_aspeed.py | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 3a20644fb2..855da805ae 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -313,14 +313,14 @@ def do_test_arm_aspeed_sdk_start(self, image):
def do_test_aarch64_aspeed_sdk_start(self, image):
self.vm.set_console()
-self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw')
+self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw',
+ '-net', 'nic', '-net',
'user,hostfwd=:127.0.0.1:0-:22')
self.vm.launch()
self.wait_for_console_pattern('U-Boot 2023.10')
self.wait_for_console_pattern('## Loading kernel from FIT Image')
self.wait_for_console_pattern('Starting kernel ...')
-self.wait_for_console_pattern("systemd[1]: Hostname set to")
@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable on GitLab')
@@ -387,15 +387,15 @@ def test_arm_ast2600_evb_sdk(self):
year = time.strftime("%Y")
self.ssh_command_output_contains('/sbin/hwclock -f /dev/rtc1', year);
-def test_aarch64_ast2700_evb_sdk_v09_01(self):
+def test_aarch64_ast2700_evb_sdk_v09_02(self):
"""
:avocado: tags=arch:aarch64
:avocado: tags=machine:ast2700-evb
"""
image_url = ('https://github.com/AspeedTech-BMC/openbmc/releases/'
- 'download/v09.01/ast2700-default-obmc.tar.gz')
-image_hash =
'b1cc0fd73c7650d34c9c8459a243f52a91e9e27144b8608b2645ab19461d1e07'
+ 'download/v09.02/ast2700-default-obmc.tar.gz')
+image_hash =
'ac969c2602f4e6bdb69562ff466b89ae3fe1d86e1f6797bb7969d787f82116a7'
image_path = self.fetch_asset(image_url, asset_hash=image_hash,
algorithm='sha256')
archive.extract(image_path, self.workdir)
@@ -436,4 +436,5 @@ def test_aarch64_ast2700_evb_sdk_v09_01(self):
self.vm.add_args('-smp', str(num_cpu))
self.do_test_aarch64_aspeed_sdk_start(image_dir + 'image-bmc')
-
+self.wait_for_console_pattern('nodistro.0 ast2700-default ttyS12')
+self.ssh_connect('root', '0penBmc', False)
Re: [PATCH v2 4/5] hw/block: m25p80: support quad mode for w25q01jvq
On 7/3/24 10:16 AM, Jamin Lin wrote:
According to the w25q01jv datasheet at page 16,
it is required to set QE bit in "Status Register 2".
Besides, users are able to utilize "Write Status Register 1(0x01)"
command to set QE bit in "Status Register 2" and
utilize "Read Status Register 2(0x35)" command to get the QE bit status.
To support quad mode for w25q01jvq, update collecting data needed
2 bytes for WRSR command in decode_new_cmd function and
verify QE bit at the second byte of collecting data bit 2
in complete_collecting_data.
Update RDCR_EQIO command to set bit 2 of return data
if quad mode enable in decode_new_cmd.
Signed-off-by: Troy Lee
Signed-off-by: Jamin Lin
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/block/m25p80.c | 16
1 file changed, 16 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8dec134832..9e99107b42 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -416,6 +416,7 @@ typedef enum {
/*
* Micron: 0x35 - enable QPI
* Spansion: 0x35 - read control register
+ * Winbond: 0x35 - quad enable
*/
RDCR_EQIO = 0x35,
RSTQIO = 0xf5,
@@ -798,6 +799,11 @@ static void complete_collecting_data(Flash *s)
s->four_bytes_address_mode = extract32(s->data[1], 5, 1);
}
break;
+case MAN_WINBOND:
+if (s->len > 1) {
+s->quad_enable = !!(s->data[1] & 0x02);
+}
+break;
default:
break;
}
@@ -1254,6 +1260,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
s->needed_bytes = 2;
s->state = STATE_COLLECTING_VAR_LEN_DATA;
break;
+case MAN_WINBOND:
+s->needed_bytes = 2;
+s->state = STATE_COLLECTING_VAR_LEN_DATA;
+break;
default:
s->needed_bytes = 1;
s->state = STATE_COLLECTING_DATA;
@@ -1431,6 +1441,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
case MAN_MACRONIX:
s->quad_enable = true;
break;
+case MAN_WINBOND:
+s->data[0] = (!!s->quad_enable) << 1;
+s->pos = 0;
+s->len = 1;
+s->state = STATE_READING_DATA;
+break;
default:
break;
}
Re: [PATCH v2 1/5] hw/net:ftgmac100: update memory region size to 0x200
On 7/3/24 10:16 AM, Jamin Lin wrote:
According to the datasheet of ASPEED SOCs,
one MAC controller owns 128KB of register space for AST2500.
However, one MAC controller only owns 64KB of register space for AST2600
and AST2700.
It set the memory region size 128KB and it occupied another
controllers Address Spaces.
Currently, the ftgmac100 model use 0x100 register space.
To support DMA 64 bits dram address and new future mode(ftgmac100_high) which
have "Normal Priority Transmit Ring Base Address Register High(0x17C)",
"High Priority Transmit Ring Base Address Register High(0x184)" and
"Receive Ring Base Address Register High(0x18C)" to save the high part physical
address of descriptor manager.
Update memory region size to 0x200.
Signed-off-by: Jamin Lin
---
hw/net/ftgmac100.c | 2 +-
include/hw/net/ftgmac100.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 25e4c0cd5b..4e88430b2f 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -1108,7 +1108,7 @@ static void ftgmac100_realize(DeviceState *dev, Error
**errp)
}
memory_region_init_io(&s->iomem, OBJECT(dev), &ftgmac100_ops, s,
- TYPE_FTGMAC100, 0x2000);
+ TYPE_FTGMAC100, FTGMAC100_NR_REGS);
sysbus_init_mmio(sbd, &s->iomem);
sysbus_init_irq(sbd, &s->irq);
qemu_macaddr_default_if_unset(&s->conf.macaddr);
diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
index 765d1538a4..5a970676da 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -14,6 +14,8 @@
#define TYPE_FTGMAC100 "ftgmac100"
OBJECT_DECLARE_SIMPLE_TYPE(FTGMAC100State, FTGMAC100)
+#define FTGMAC100_NR_REGS 0x200
Since this value will size a memory region, I think the define name should
be changed to FTGMAC100_{MEM,REGION,MMIO}_SIZE. What ever you prefer.
Thanks,
C.
+
#include "hw/sysbus.h"
#include "net/net.h"
Re: [PATCH 3/4] tests/qtest/npcm7xx_sdhci: Access the card using its published address
On 7/2/24 4:08 PM, Philippe Mathieu-Daudé wrote:
Currently setup_sd_card() asks the card its address,
but discard the response and use hardcoded 0x4567.
Set the SDHC_CMD_RESPONSE bit to have the controller
record the bus response, and read the response from
the RSPREG0 register. Then we can select the card with
its real address.
Signed-off-by: Philippe Mathieu-Daudé
---
Cc: Hao Wu
Cc: Chris Rauer
Cc: Shengtan Mao
Cc: Patrick Venture
Cc: Tyrone Ting
---
tests/qtest/libqos/sdhci-cmd.h | 2 ++
tests/qtest/npcm7xx_sdhci-test.c | 8 ++--
2 files changed, 8 insertions(+), 2 deletions(-)
Tested-by: Cédric Le Goater
Reviewed-by: Cédric Le Goater
Thanks,
C.
diff --git a/tests/qtest/libqos/sdhci-cmd.h b/tests/qtest/libqos/sdhci-cmd.h
index 9e61dd4944..90efa028ef 100644
--- a/tests/qtest/libqos/sdhci-cmd.h
+++ b/tests/qtest/libqos/sdhci-cmd.h
@@ -22,6 +22,7 @@
#define SDHC_ARGUMENT 0x08
#define SDHC_TRNMOD 0x0C
#define SDHC_CMDREG 0x0E
+#define SDHC_RSPREG0 0x10
#define SDHC_BDATA 0x20
#define SDHC_PRNSTS 0x24
#define SDHC_BLKGAP 0x2A
@@ -38,6 +39,7 @@
#define SDHC_TRNS_MULTI 0x0020
/* CMD Reg */
+#define SDHC_CMD_RESPONSE (3 << 0)
#define SDHC_CMD_DATA_PRESENT (1 << 5)
#define SDHC_ALL_SEND_CID (2 << 8)
#define SDHC_SEND_RELATIVE_ADDR (3 << 8)
diff --git a/tests/qtest/npcm7xx_sdhci-test.c b/tests/qtest/npcm7xx_sdhci-test.c
index 5d68540e52..01f237a816 100644
--- a/tests/qtest/npcm7xx_sdhci-test.c
+++ b/tests/qtest/npcm7xx_sdhci-test.c
@@ -30,6 +30,8 @@ char *sd_path;
static QTestState *setup_sd_card(void)
{
+uint16_t rca;
+
QTestState *qts = qtest_initf(
"-machine kudo-bmc "
"-device sd-card,drive=drive0 "
@@ -43,8 +45,10 @@ static QTestState *setup_sd_card(void)
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_APP_CMD);
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4120, 0, (41 << 8));
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
-sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR);
-sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4567, 0,
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR
+| SDHC_CMD_RESPONSE);
+rca = qtest_readl(qts, NPCM7XX_MMC_BA + SDHC_RSPREG0) >> 16;
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, rca << 16, 0,
SDHC_SELECT_DESELECT_CARD);
return qts;
Re: [PATCH 1/4] hw/sd/npcm7xx_sdhci: Use TYPE_SYSBUS_SDHCI definition
On 7/2/24 4:08 PM, Philippe Mathieu-Daudé wrote:
Use the macro instead of two explicit string literals.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/npcm7xx_sdhci.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/sd/npcm7xx_sdhci.c b/hw/sd/npcm7xx_sdhci.c
index e93dab8dbd..fb51821e11 100644
--- a/hw/sd/npcm7xx_sdhci.c
+++ b/hw/sd/npcm7xx_sdhci.c
@@ -16,6 +16,7 @@
#include "qemu/osdep.h"
+#include "hw/sd/sdhci.h"
#include "hw/sd/npcm7xx_sdhci.h"
#include "migration/vmstate.h"
#include "sdhci-internal.h"
@@ -162,7 +163,7 @@ static void npcm7xx_sdhci_instance_init(Object *obj)
{
NPCM7xxSDHCIState *s = NPCM7XX_SDHCI(obj);
-object_initialize_child(OBJECT(s), "generic-sdhci", &s->sdhci,
+object_initialize_child(OBJECT(s), TYPE_SYSBUS_SDHCI, &s->sdhci,
TYPE_SYSBUS_SDHCI);
}
Re: [PATCH 2/4] hw/sd/sdhci: Log non-sequencial access as GUEST_ERROR
On 7/2/24 4:08 PM, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sdhci.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 27673e1c70..d02c3e3963 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -983,8 +983,9 @@ static inline bool
sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
{
if ((s->data_count & 0x3) != byte_num) {
-trace_sdhci_error("Non-sequential access to Buffer Data Port register"
- "is prohibited\n");
+qemu_log_mask(LOG_GUEST_ERROR,
+ "SDHCI: Non-sequential access to Buffer Data Port"
+ " register is prohibited\n");
return false;
}
return true;
Re: [PATCH 00/19] hw/sd: Add support for eMMC cards
On 6/27/24 7:10 PM, Philippe Mathieu-Daudé wrote: Trying to land what Vincent Palatin started 13 years ago! https://lore.kernel.org/qemu-devel/1311635951-11047-5-git-send-email-vpala...@chromium.org/ with fixes from Sai, Joel and Cédric. I reviewed Cédric patches but still plan to review the EXT_CSD layout and Joel "boot config" patches. In order to avoid deviation with the spec, the experimental 'x-aspeed-emmc-kludge' property is used. Adding Aspeed people for feedback. Thanks, C. Based-on: <20240627164815.82606-1-phi...@linaro.org> Full series for testing: https://gitlab.com/philmd/qemu/-/tags/emmc-v4 Including Aspeed branch from Cédric on top (aspeed-9.1): https://gitlab.com/philmd/qemu/-/tags/aspeed_emmc-v4 Cédric Le Goater (5): hw/sd/sdcard: Basis for eMMC support hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR() handler hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23) hw/sd/sdcard: Adapt sd_cmd_ALL_SEND_CID handler for eMMC (CMD2) hw/sd/sdcard: Adapt sd_cmd_APP_CMD handler for eMMC (CMD55) Joel Stanley (3): hw/sd/sdcard: Support boot area in emmc image hw/sd/sdcard: Subtract bootarea size from blk hw/sd/sdcard: Add boot config support Luc Michel (1): hw/sd/sdcard: Implement eMMC sleep state (CMD5) Philippe Mathieu-Daudé (6): hw/sd/sdcard: Introduce set_csd/set_cid handlers hw/sd/sdcard: Cover more SDCardStates hw/sd/sdcard: Register generic command handlers hw/sd/sdcard: Register unimplemented command handlers hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26) hw/sd/sdcard: Add experimental 'x-aspeed-emmc-kludge' property Sai Pavan Boddu (3): hw/sd/sdcard: Add emmc_cmd_SEND_OP_COND handler (CMD1) hw/sd/sdcard: add emmc_cmd_SEND_TUNING_BLOCK handler (CMD21) hw/sd/sdcard: Add mmc SWITCH function support (CMD6) Vincent Palatin (1): hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8) hw/sd/sdmmc-internal.h | 3 + include/hw/sd/sd.h | 6 + hw/sd/sd.c | 419 - 3 files changed, 420 insertions(+), 8 deletions(-)
Re: [PATCH 2/3] hw/sd/sdcard: Use spec v3.01 by default
On 6/27/24 9:10 AM, Philippe Mathieu-Daudé wrote:
Recent SDHCI expect cards to support the v3.01 spec
to negociate lower I/O voltage. Select it by default.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a48010cfc1..d0a1d5db18 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2280,7 +2280,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
static Property sd_properties[] = {
DEFINE_PROP_UINT8("spec_version", SDState,
- spec_version, SD_PHY_SPECv2_00_VERS),
+ spec_version, SD_PHY_SPECv3_01_VERS),
DEFINE_PROP_DRIVE("drive", SDState, blk),
/* We do not model the chip select pin, so allow the board to select
* whether card should be in SSI or MMC/SD mode. It is also up to the
Re: [PATCH 1/3] hw/sd/sdcard: Deprecate support for spec v1.10
On 6/27/24 9:10 AM, Philippe Mathieu-Daudé wrote:
We use the v2.00 spec by default since commit 2f0939c234
("sdcard: Add a 'spec_version' property, default to Spec v2.00").
Time to deprecate the v1.10 which doesn't bring much, and
is not tested.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
docs/about/deprecated.rst | 6 ++
1 file changed, 6 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index ff3da68208..02cdef14aa 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -362,6 +362,12 @@ recommending to switch to their stable counterparts:
- "Zve64f" should be replaced with "zve64f"
- "Zve64d" should be replaced with "zve64d"
+``-device sd-card,spec_version=1`` (since 9.1)
+^^
+
+SD physical layer specification v2.00 supersedes the v1.10 one.
+v2.00 is the default since QEMU 3.0.0.
+
Block device options
''''''''''''''''''''
Re: [PATCH v2 00/12] hw/sd/sdcard: Accumulation of cleanups and fixes
On 6/25/24 7:53 AM, Philippe Mathieu-Daudé wrote: Since v1: - various patches merged, few more added Various SD card cleanups and fixes accumulated over the years. Various have been useful to help integrating eMMC support (which will come later). Philippe Mathieu-Daudé (12): tests/qtest: Disable npcm7xx_sdhci tests using hardcoded RCA hw/sd/sdcard: Generate random RCA value hw/sd/sdcard: Track last command used to help logging hw/sd/sdcard: Trace block offset in READ/WRITE data accesses hw/sd/sdcard: Do not store vendor data on block drive (CMD56) hw/sd/sdcard: Send WRITE_PROT bits MSB first (CMD30) hw/sd/sdcard: Send NUM_WR_BLOCKS bits MSB first (ACMD22) hw/sd/sdcard: Use READY_FOR_DATA definition instead of magic value hw/sd/sdcard: Assign SDCardStates enum values hw/sd/sdcard: Simplify sd_inactive_state handling hw/sd/sdcard: Restrict SWITCH_FUNCTION to sd_transfer_state (CMD6) hw/sd/sdcard: Add direct reference to SDProto in SDState hw/sd/sd.c | 119 --- tests/qtest/npcm7xx_sdhci-test.c | 7 ++ hw/sd/trace-events | 4 +- 3 files changed, 70 insertions(+), 60 deletions(-) Tested-by: Cédric Le Goater Thanks, C.
Re: [PATCH 00/23] hw/sd/sdcard: Accumulation of cleanups and fixes
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote: Various SD card cleanups and fixes accumulated over the years. Various have been useful to help integrating eMMC support (which will come later). Based-on: <20240621075607.17902-1-phi...@linaro.org> st24_be_p() Philippe Mathieu-Daudé (23): hw/sd/sdcard: Correct code indentation hw/sd/sdcard: Rewrite sd_cmd_ALL_SEND_CID using switch case (CMD2) hw/sd/sdcard: Fix typo in SEND_OP_COND command name hw/sd/sdcard: Use HWBLOCK_SHIFT definition instead of magic values hw/sd/sdcard: Use registerfield CSR::CURRENT_STATE definition hw/sd/sdcard: Use Load/Store API to fill some CID/CSD registers hw/sd/sdcard: Remove ACMD6 handler for SPI mode hw/sd/sdcard: Remove explicit entries for illegal commands hw/sd/sdcard: Generate random RCA value hw/sd/sdcard: Track last command used to help logging hw/sd/sdcard: Trace update of block count (CMD23) hw/sd/sdcard: Trace block offset in READ/WRITE data accesses hw/sd/sdcard: Have cmd_valid_while_locked() return a boolean value hw/sd/sdcard: Factor sd_req_get_rca() method out hw/sd/sdcard: Only call sd_req_get_rca() where RCA is used hw/sd/sdcard: Factor sd_req_get_address() method out hw/sd/sdcard: Only call sd_req_get_address() where address is used hw/sd/sdcard: Add sd_invalid_mode_for_cmd to report invalid mode switch hw/sd/sdcard: Inline BLK_READ_BLOCK / BLK_WRITE_BLOCK macros hw/sd/sdcard: Add comments around registers and commands hw/sd/sdcard: Do not store vendor data on block drive (CMD56) hw/sd/sdcard: Send WRITE_PROT bits MSB first (CMD30) hw/sd/sdcard: Send NUM_WR_BLOCKS bits MSB first (ACMD22) hw/sd/sd.c | 278 +++-- hw/sd/sdmmc-internal.c | 2 +- hw/sd/trace-events | 7 +- 3 files changed, 159 insertions(+), 128 deletions(-) Tested-by: Cédric Le Goater Thanks, C.
Re: [PATCH 18/23] hw/sd/sdcard: Add sd_invalid_mode_for_cmd to report invalid mode switch
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
Having the mode switch displayed help to track incomplete
command implementations.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 75 +-
1 file changed, 41 insertions(+), 34 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1df16ce6a2..8d63a39a54 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -180,6 +180,17 @@ static const char *sd_version_str(enum
SDPhySpecificationVersion version)
return sdphy_version[version];
}
+static const char *sd_mode_name(enum SDCardModes mode)
+{
+static const char *mode_name[] = {
+[sd_inactive] = "inactive",
+[sd_card_identification_mode] = "identification",
+[sd_data_transfer_mode] = "transfer",
+};
+assert(mode < ARRAY_SIZE(mode_name));
+return mode_name[mode];
+}
+
static const char *sd_state_name(enum SDCardStates state)
{
static const char *state_name[] = {
@@ -1015,6 +1026,15 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState
*sd, SDRequest req)
return sd_illegal;
}
+static sd_rsp_type_t sd_invalid_mode_for_cmd(SDState *sd, SDRequest req)
+{
+qemu_log_mask(LOG_GUEST_ERROR, "%s: CMD%i in a wrong mode: %s (spec %s)\n",
+ sd_proto(sd)->name, req.cmd, sd_mode_name(sd->mode),
+ sd_version_str(sd->spec_version));
+
+return sd_illegal;
+}
+
static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
{
qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i for spec %s\n",
@@ -1154,18 +1174,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 6: /* CMD6: SWITCH_FUNCTION */
-switch (sd->mode) {
-case sd_data_transfer_mode:
-sd_function_switch(sd, req.arg);
-sd->state = sd_sendingdata_state;
-sd->data_start = 0;
-sd->data_offset = 0;
-return sd_r1;
-
-default:
-break;
+if (sd->mode != sd_data_transfer_mode) {
+return sd_invalid_mode_for_cmd(sd, req);
}
-break;
+sd_function_switch(sd, req.arg);
+sd->state = sd_sendingdata_state;
+sd->data_start = 0;
+sd->data_offset = 0;
+return sd_r1;
case 7: /* CMD7: SELECT/DESELECT_CARD */
rca = sd_req_get_rca(sd, req);
@@ -1289,33 +1305,24 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 13: /* CMD13: SEND_STATUS */
rca = sd_req_get_rca(sd, req);
-switch (sd->mode) {
-case sd_data_transfer_mode:
-if (!sd_is_spi(sd) && sd->rca != rca) {
-return sd_r0;
-}
-
-return sd_r1;
-
-default:
-break;
+if (sd->mode != sd_data_transfer_mode) {
+return sd_invalid_mode_for_cmd(sd, req);
}
-break;
+if (!sd_is_spi(sd) && sd->rca != rca) {
+return sd_r0;
+}
+
+return sd_r1;
case 15: /* CMD15: GO_INACTIVE_STATE */
-rca = sd_req_get_rca(sd, req);
-switch (sd->mode) {
-case sd_data_transfer_mode:
-if (sd->rca != rca)
-return sd_r0;
-
-sd->state = sd_inactive_state;
-return sd_r0;
-
-default:
-break;
+if (sd->mode != sd_data_transfer_mode) {
+return sd_invalid_mode_for_cmd(sd, req);
}
-break;
+rca = sd_req_get_rca(sd, req);
+if (sd->rca == rca) {
+sd->state = sd_inactive_state;
+}
+return sd_r0;
/* Block read commands (Class 2) */
case 16: /* CMD16: SET_BLOCKLEN */
Re: [PATCH 20/23] hw/sd/sdcard: Add comments around registers and commands
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
From: Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 20
1 file changed, 20 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ca2c903c5b..95e23abd30 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -317,6 +317,8 @@ static uint8_t sd_crc7(const void *message, size_t width)
return shift_reg;
}
+/* Operation Conditions register */
+
#define OCR_POWER_DELAY_NS 50 /* 0.5ms */
FIELD(OCR, VDD_VOLTAGE_WINDOW, 0, 24)
@@ -366,6 +368,8 @@ static void sd_set_ocr(SDState *sd)
}
}
+/* SD Configuration register */
+
static void sd_set_scr(SDState *sd)
{
sd->scr[0] = 0 << 4;/* SCR structure version 1.0 */
@@ -388,6 +392,8 @@ static void sd_set_scr(SDState *sd)
sd->scr[7] = 0x00;
}
+/* Card IDentification register */
+
#define MID 0xaa
#define OID "XY"
#define PNM "QEMU!"
@@ -413,6 +419,8 @@ static void sd_set_cid(SDState *sd)
sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
}
+/* Card-Specific Data register */
+
#define HWBLOCK_SHIFT 9/* 512 bytes */
#define SECTOR_SHIFT5/* 16 kilobytes */
#define WPGROUP_SHIFT 7/* 2 megs */
@@ -482,6 +490,8 @@ static void sd_set_csd(SDState *sd, uint64_t size)
sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
}
+/* Relative Card Address register */
+
static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
{
if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc) {
@@ -490,6 +500,8 @@ static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
return 0;
}
+/* Card Status register */
+
FIELD(CSR, AKE_SEQ_ERROR, 3, 1)
FIELD(CSR, APP_CMD, 5, 1)
FIELD(CSR, FX_EVENT,6, 1)
@@ -620,6 +632,8 @@ static void sd_reset(DeviceState *dev)
sect = sd_addr_to_wpnum(size) + 1;
sd->state = sd_idle_state;
+
+/* card registers */
sd->rca = 0x;
sd->size = size;
sd_set_ocr(sd);
@@ -1052,6 +1066,7 @@ static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd,
SDRequest req)
return sd_illegal;
}
+/* CMD0 */
static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
{
if (sd->state != sd_inactive_state) {
@@ -1062,6 +1077,7 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd,
SDRequest req)
return sd_is_spi(sd) ? sd_r1 : sd_r0;
}
+/* CMD1 */
static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
{
sd->state = sd_transfer_state;
@@ -1069,6 +1085,7 @@ static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd,
SDRequest req)
return sd_r1;
}
+/* CMD2 */
static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
{
switch (sd->state) {
@@ -1080,6 +1097,7 @@ static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd,
SDRequest req)
}
}
+/* CMD3 */
static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
{
switch (sd->state) {
@@ -1094,6 +1112,7 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState
*sd, SDRequest req)
}
}
+/* CMD19 */
static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
{
if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
@@ -1110,6 +1129,7 @@ static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState
*sd, SDRequest req)
return sd_r1;
}
+/* CMD23 */
static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
{
if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
Re: [PATCH 17/23] hw/sd/sdcard: Only call sd_req_get_address() where address is used
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
It will be useful later to assert only ADTC commands
(Addressed point-to-point Data Transfer Commands, defined
as the 'sd_adtc' enum) extract the address value from the
command argument.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a0193a46ea..1df16ce6a2 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -,7 +,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd,
SDRequest req)
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint16_t rca;
-uint64_t addr = sd_req_get_address(sd, req);
+uint64_t addr;
sd->last_cmd_name = sd_cmd_name(req.cmd);
/* CMD55 precedes an ACMD, so we are not interested in tracing it.
@@ -1237,7 +1237,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
}
sd->state = sd_sendingdata_state;
memcpy(sd->data, sd->csd, 16);
-sd->data_start = addr;
+sd->data_start = sd_req_get_address(sd, req);
sd->data_offset = 0;
return sd_r1;
@@ -1261,7 +1261,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
}
sd->state = sd_sendingdata_state;
memcpy(sd->data, sd->cid, 16);
-sd->data_start = addr;
+sd->data_start = sd_req_get_address(sd, req);
sd->data_offset = 0;
return sd_r1;
@@ -1337,6 +1337,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
case 17: /* CMD17: READ_SINGLE_BLOCK */
case 18: /* CMD18: READ_MULTIPLE_BLOCK */
+addr = sd_req_get_address(sd, req);
switch (sd->state) {
case sd_transfer_state:
@@ -1357,6 +1358,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
/* Block write commands (Class 4) */
case 24: /* CMD24: WRITE_SINGLE_BLOCK */
case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
+addr = sd_req_get_address(sd, req);
switch (sd->state) {
case sd_transfer_state:
@@ -1415,7 +1417,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
if (sd->size > SDSC_MAX_CAPACITY) {
return sd_illegal;
}
-
+addr = sd_req_get_address(sd, req);
switch (sd->state) {
case sd_transfer_state:
if (!address_in_range(sd, "SET_WRITE_PROT", addr, 1)) {
@@ -1437,7 +1439,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
if (sd->size > SDSC_MAX_CAPACITY) {
return sd_illegal;
}
-
+addr = sd_req_get_address(sd, req);
switch (sd->state) {
case sd_transfer_state:
if (!address_in_range(sd, "CLR_WRITE_PROT", addr, 1)) {
@@ -1459,7 +1461,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
if (sd->size > SDSC_MAX_CAPACITY) {
return sd_illegal;
}
-
+addr = sd_req_get_address(sd, req);
switch (sd->state) {
case sd_transfer_state:
if (!address_in_range(sd, "SEND_WRITE_PROT",
Re: [PATCH 19/23] hw/sd/sdcard: Inline BLK_READ_BLOCK / BLK_WRITE_BLOCK macros
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
These macros only save 3 chars and make the code harder
to maintain, simply remove them.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 8d63a39a54..ca2c903c5b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -816,8 +816,6 @@ static void sd_blk_write(SDState *sd, uint64_t addr,
uint32_t len)
}
}
-#define BLK_READ_BLOCK(a, len) sd_blk_read(sd, a, len)
-#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
#define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
#define APP_WRITE_BLOCK(a, len)
@@ -869,7 +867,7 @@ static void sd_erase(SDState *sd)
continue;
}
}
-BLK_WRITE_BLOCK(erase_addr, erase_len);
+sd_blk_write(sd, erase_addr, erase_len);
}
}
@@ -1901,7 +1899,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
if (sd->data_offset >= sd->blk_len) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
-BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
+sd_blk_write(sd, sd->data_start, sd->data_offset);
sd->blk_written ++;
sd->csd[14] |= 0x40;
/* Bzzztt Operation complete. */
@@ -1927,7 +1925,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
if (sd->data_offset >= sd->blk_len) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
-BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
+sd_blk_read(sd, sd->data_start, sd->data_offset);
sd->blk_written++;
sd->data_start += sd->blk_len;
sd->data_offset = 0;
@@ -2075,8 +2073,9 @@ uint8_t sd_read_byte(SDState *sd)
break;
case 17: /* CMD17: READ_SINGLE_BLOCK */
-if (sd->data_offset == 0)
-BLK_READ_BLOCK(sd->data_start, io_len);
+if (sd->data_offset == 0) {
+sd_blk_read(sd, sd->data_start, io_len);
+}
ret = sd->data[sd->data_offset ++];
if (sd->data_offset >= io_len)
@@ -2089,7 +2088,7 @@ uint8_t sd_read_byte(SDState *sd)
sd->data_start, io_len)) {
return 0x00;
}
-BLK_READ_BLOCK(sd->data_start, io_len);
+sd_blk_read(sd, sd->data_start, io_len);
}
ret = sd->data[sd->data_offset ++];
Re: [PATCH 16/23] hw/sd/sdcard: Factor sd_req_get_address() method out
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
Extract sd_cmd_get_address() so we can re-use it
in various SDProto handlers. Use CARD_CAPACITY and
HWBLOCK_SHIFT definitions instead of magic values.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index cb9d85bb11..a0193a46ea 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -579,6 +579,14 @@ static void sd_response_r7_make(SDState *sd, uint8_t
*response)
stl_be_p(response, sd->vhs);
}
+static uint64_t sd_req_get_address(SDState *sd, SDRequest req)
+{
+if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
+return (uint64_t) req.arg << HWBLOCK_SHIFT;
+}
+return req.arg;
+}
+
static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
{
return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
@@ -1103,7 +,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd,
SDRequest req)
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint16_t rca;
-uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
+uint64_t addr = sd_req_get_address(sd, req);
sd->last_cmd_name = sd_cmd_name(req.cmd);
/* CMD55 precedes an ACMD, so we are not interested in tracing it.
Re: [PATCH 15/23] hw/sd/sdcard: Only call sd_req_get_rca() where RCA is used
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
It will be useful later to assert only AC commands
(Addressed point-to-point Commands, defined as the
'sd_ac' enum) extract the RCA value from the command
argument.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bc47ae36bc..cb9d85bb11 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1102,7 +1102,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd,
SDRequest req)
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
-uint16_t rca = sd_req_get_rca(sd, req);
+uint16_t rca;
uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
sd->last_cmd_name = sd_cmd_name(req.cmd);
@@ -1160,6 +1160,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 7: /* CMD7: SELECT/DESELECT_CARD */
+rca = sd_req_get_rca(sd, req);
switch (sd->state) {
case sd_standby_state:
if (sd->rca != rca)
@@ -1214,6 +1215,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
return sd_r7;
case 9: /* CMD9: SEND_CSD */
+rca = sd_req_get_rca(sd, req);
switch (sd->state) {
case sd_standby_state:
if (sd->rca != rca)
@@ -1237,6 +1239,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 10: /* CMD10: SEND_CID */
+rca = sd_req_get_rca(sd, req);
switch (sd->state) {
case sd_standby_state:
if (sd->rca != rca)
@@ -1277,6 +1280,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 13: /* CMD13: SEND_STATUS */
+rca = sd_req_get_rca(sd, req);
switch (sd->mode) {
case sd_data_transfer_mode:
if (!sd_is_spi(sd) && sd->rca != rca) {
@@ -1291,6 +1295,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 15: /* CMD15: GO_INACTIVE_STATE */
+rca = sd_req_get_rca(sd, req);
switch (sd->mode) {
case sd_data_transfer_mode:
if (sd->rca != rca)
@@ -1523,6 +1528,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
/* Application specific commands (Class 8) */
case 55: /* CMD55: APP_CMD */
+rca = sd_req_get_rca(sd, req);
switch (sd->state) {
case sd_ready_state:
case sd_identification_state:
Re: [PATCH 14/23] hw/sd/sdcard: Factor sd_req_get_rca() method out
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
Extract sd_req_get_rca() so we can re-use it in various
SDProto handlers. Return a 16-bit value since RCA is 16-bit.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 510784fc82..bc47ae36bc 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -471,6 +471,14 @@ static void sd_set_csd(SDState *sd, uint64_t size)
sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
}
+static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
+{
+if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc) {
+return req.arg >> 16;
+}
+return 0;
+}
+
FIELD(CSR, AKE_SEQ_ERROR, 3, 1)
FIELD(CSR, APP_CMD, 5, 1)
FIELD(CSR, FX_EVENT,6, 1)
@@ -1094,7 +1102,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd,
SDRequest req)
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
-uint32_t rca = 0x;
+uint16_t rca = sd_req_get_rca(sd, req);
uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
sd->last_cmd_name = sd_cmd_name(req.cmd);
@@ -1110,11 +1118,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
/* Not interpreting this as an app command */
sd->card_status &= ~APP_CMD;
-if (sd_cmd_type[req.cmd] == sd_ac
-|| sd_cmd_type[req.cmd] == sd_adtc) {
-rca = req.arg >> 16;
-}
-
/* CMD23 (set block count) must be immediately followed by CMD18 or CMD25
* if not, its effects are cancelled */
if (sd->multi_blk_cnt != 0 && !(req.cmd == 18 || req.cmd == 25)) {
Re: [PATCH 13/23] hw/sd/sdcard: Have cmd_valid_while_locked() return a boolean value
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c6cc1bab11..510784fc82 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1716,7 +1716,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
return sd_illegal;
}
-static int cmd_valid_while_locked(SDState *sd, const uint8_t cmd)
+static bool cmd_valid_while_locked(SDState *sd, unsigned cmd)
{
/* Valid commands in locked state:
* basic class (0)
@@ -1730,7 +1730,7 @@ static int cmd_valid_while_locked(SDState *sd, const
uint8_t cmd)
return cmd == 41 || cmd == 42;
}
if (cmd == 16 || cmd == 55) {
-return 1;
+return true;
}
return sd_cmd_class[cmd] == 0 || sd_cmd_class[cmd] == 7;
}
Re: [PATCH 12/23] hw/sd/sdcard: Trace block offset in READ/WRITE data accesses
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
Useful to detect out of bound accesses.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 4 ++--
hw/sd/trace-events | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2586d15cbd..c6cc1bab11 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1868,7 +1868,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
trace_sdcard_write_data(sd_proto(sd)->name,
sd->last_cmd_name,
-sd->current_cmd, value);
+sd->current_cmd, sd->data_offset, value);
switch (sd->current_cmd) {
case 24: /* CMD24: WRITE_SINGLE_BLOCK */
sd->data[sd->data_offset ++] = value;
@@ -2024,7 +2024,7 @@ uint8_t sd_read_byte(SDState *sd)
trace_sdcard_read_data(sd_proto(sd)->name,
sd->last_cmd_name,
- sd->current_cmd, io_len);
+ sd->current_cmd, sd->data_offset, io_len);
switch (sd->current_cmd) {
case 6: /* CMD6: SWITCH_FUNCTION */
ret = sd->data[sd->data_offset ++];
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 724365efc3..0eee98a646 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -52,8 +52,8 @@ sdcard_lock(void) ""
sdcard_unlock(void) ""
sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
-sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value)
"%s %20s/ CMD%02d value 0x%02x"
-sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t length)
"%s %20s/ CMD%02d len %" PRIu32
+sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t offset, uint8_t
value) "%s %20s/ CMD%02d ofs %"PRIu32" value 0x%02x"
+sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t offset, uint32_t
length) "%s %20s/ CMD%02d ofs %"PRIu32" len %" PRIu32
sdcard_set_voltage(uint16_t millivolts) "%u mV"
# pxa2xx_mmci.c
Re: [PATCH 11/23] hw/sd/sdcard: Trace update of block count (CMD23)
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote: Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Thanks, C. --- hw/sd/sd.c | 1 + hw/sd/trace-events | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 4e378f7cf7..2586d15cbd 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1087,6 +1087,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req) } sd->multi_blk_cnt = req.arg; +trace_sdcard_set_block_count(sd->multi_blk_cnt); return sd_r1; } diff --git a/hw/sd/trace-events b/hw/sd/trace-events index 94a00557b2..724365efc3 100644 --- a/hw/sd/trace-events +++ b/hw/sd/trace-events @@ -43,7 +43,8 @@ sdcard_response(const char *rspdesc, int rsplen) "%s (sz:%d)" sdcard_powerup(void) "" sdcard_inquiry_cmd41(void) "" sdcard_reset(void) "" -sdcard_set_blocklen(uint16_t length) "0x%03x" +sdcard_set_blocklen(uint16_t length) "block len 0x%03x" +sdcard_set_block_count(uint32_t cnt) "block cnt 0x%"PRIx32 sdcard_inserted(bool readonly) "read_only: %u" sdcard_ejected(void) "" sdcard_erase(uint32_t first, uint32_t last) "addr first 0x%" PRIx32" last 0x%" PRIx32
Re: [PATCH 08/23] hw/sd/sdcard: Remove explicit entries for illegal commands
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
NULL handler is already handled as illegal, no need to
duplicate (that keeps this array simpler to maintain).
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index e9af834a8c..30239b28bc 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2146,12 +2146,6 @@ static const SDProto sd_proto_spi = {
.cmd = {
[0] = sd_cmd_GO_IDLE_STATE,
[1] = spi_cmd_SEND_OP_COND,
-[2 ... 4] = sd_cmd_illegal,
-[5] = sd_cmd_illegal,
-[7] = sd_cmd_illegal,
-[15]= sd_cmd_illegal,
-[26]= sd_cmd_illegal,
-[52 ... 54] = sd_cmd_illegal,
},
.acmd = {
[41]= spi_cmd_SEND_OP_COND,
@@ -2162,15 +2156,10 @@ static const SDProto sd_proto_sd = {
.name = "SD",
.cmd = {
[0] = sd_cmd_GO_IDLE_STATE,
-[1] = sd_cmd_illegal,
[2] = sd_cmd_ALL_SEND_CID,
[3] = sd_cmd_SEND_RELATIVE_ADDR,
-[5] = sd_cmd_illegal,
[19]= sd_cmd_SEND_TUNING_BLOCK,
[23]= sd_cmd_SET_BLOCK_COUNT,
-[52 ... 54] = sd_cmd_illegal,
-[58]= sd_cmd_illegal,
-[59]= sd_cmd_illegal,
},
};
Re: [PATCH 07/23] hw/sd/sdcard: Remove ACMD6 handler for SPI mode
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
There is no ACMD6 command in SPI mode, remove the pointless
handler introduced in commit 946897ce18 ("sdcard: handles
more commands in SPI mode"). Keep sd_cmd_unimplemented()
since we'll reuse it later.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b0cd30c657..e9af834a8c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1012,6 +1012,7 @@ static sd_rsp_type_t sd_cmd_illegal(SDState *sd,
SDRequest req)
}
/* Commands that are recognised but not yet implemented. */
+__attribute__((unused))
static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
{
qemu_log_mask(LOG_UNIMP, "%s: CMD%i not implemented\n",
@@ -2153,7 +2154,6 @@ static const SDProto sd_proto_spi = {
[52 ... 54] = sd_cmd_illegal,
},
.acmd = {
-[6] = sd_cmd_unimplemented,
[41]= spi_cmd_SEND_OP_COND,
},
};
Re: [PATCH 06/23] hw/sd/sdcard: Use Load/Store API to fill some CID/CSD registers
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote: The ld/st API helps noticing CID or CSD bytes refer to the same field. Multi-bytes fields are stored MSB first in CID / CSD. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Thanks, C. --- hw/sd/sd.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 24415cb9f0..b0cd30c657 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -393,10 +393,7 @@ static void sd_set_cid(SDState *sd) sd->cid[6] = PNM[3]; sd->cid[7] = PNM[4]; sd->cid[8] = PRV; /* Fake product revision (PRV) */ -sd->cid[9] = 0xde; /* Fake serial number (PSN) */ -sd->cid[10] = 0xad; -sd->cid[11] = 0xbe; -sd->cid[12] = 0xef; +stl_be_p(&sd->cid[9], 0xdeadbeef); /* Fake serial number (PSN) */ sd->cid[13] = 0x00 |/* Manufacture date (MDT) */ ((MDT_YR - 2000) / 10); sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON; @@ -462,9 +459,7 @@ static void sd_set_csd(SDState *sd, uint64_t size) sd->csd[4] = 0x5b; sd->csd[5] = 0x59; sd->csd[6] = 0x00; -sd->csd[7] = (size >> 16) & 0xff; -sd->csd[8] = (size >> 8) & 0xff; -sd->csd[9] = (size & 0xff); +st24_be_p(&sd->csd[7], size); sd->csd[10] = 0x7f; sd->csd[11] = 0x80; sd->csd[12] = 0x0a;
Re: [PATCH 05/23] hw/sd/sdcard: Use registerfield CSR::CURRENT_STATE definition
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote: Use registerfield-generated definitions to update card_status. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Thanks, C. --- hw/sd/sd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index c528c30bcf..24415cb9f0 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1788,8 +1788,8 @@ int sd_do_command(SDState *sd, SDRequest *req, * (Do this now so they appear in r1 responses.) */ sd->current_cmd = req->cmd; -sd->card_status &= ~CURRENT_STATE; -sd->card_status |= (last_state << 9); +sd->card_status = FIELD_DP32(sd->card_status, CSR, + CURRENT_STATE, last_state); } send_response:
Re: [PATCH 04/23] hw/sd/sdcard: Use HWBLOCK_SHIFT definition instead of magic values
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
From: Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 331cef5779..c528c30bcf 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -596,7 +596,7 @@ static void sd_reset(DeviceState *dev)
} else {
sect = 0;
}
-size = sect << 9;
+size = sect << HWBLOCK_SHIFT;
sect = sd_addr_to_wpnum(size) + 1;
@@ -822,8 +822,8 @@ static void sd_erase(SDState *sd)
if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
/* High capacity memory card: erase units are 512 byte blocks */
-erase_start *= 512;
-erase_end *= 512;
+erase_start <<= HWBLOCK_SHIFT;
+erase_end <<= HWBLOCK_SHIFT;
sdsc = false;
}
Re: [PATCH 03/23] hw/sd/sdcard: Fix typo in SEND_OP_COND command name
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
There is no SEND_OP_CMD but SEND_OP_COND.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 6 +++---
hw/sd/sdmmc-internal.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index addeb1940f..331cef5779 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1035,7 +1035,7 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd,
SDRequest req)
return sd_is_spi(sd) ? sd_r1 : sd_r0;
}
-static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
+static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
{
sd->state = sd_transfer_state;
@@ -2149,7 +2149,7 @@ static const SDProto sd_proto_spi = {
.name = "SPI",
.cmd = {
[0] = sd_cmd_GO_IDLE_STATE,
-[1] = sd_cmd_SEND_OP_CMD,
+[1] = spi_cmd_SEND_OP_COND,
[2 ... 4] = sd_cmd_illegal,
[5] = sd_cmd_illegal,
[7] = sd_cmd_illegal,
@@ -2159,7 +2159,7 @@ static const SDProto sd_proto_spi = {
},
.acmd = {
[6] = sd_cmd_unimplemented,
-[41]= sd_cmd_SEND_OP_CMD,
+[41]= spi_cmd_SEND_OP_COND,
},
};
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index 8648a7808d..c1d5508ae6 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -14,7 +14,7 @@
const char *sd_cmd_name(uint8_t cmd)
{
static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
- [0]= "GO_IDLE_STATE", [1]= "SEND_OP_CMD",
+ [0]= "GO_IDLE_STATE", [1]= "SEND_OP_COND",
[2]= "ALL_SEND_CID",[3]= "SEND_RELATIVE_ADDR",
[4]= "SET_DSR", [5]= "IO_SEND_OP_COND",
[6]= "SWITCH_FUNC", [7]= "SELECT/DESELECT_CARD",
Re: [PATCH 02/23] hw/sd/sdcard: Rewrite sd_cmd_ALL_SEND_CID using switch case (CMD2)
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
Keep this handler style in sync with other handlers by
using a switch() case, which might become handy to
handle other states.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/sd/sd.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 626e99ecd6..addeb1940f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1044,13 +1044,13 @@ static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd,
SDRequest req)
static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
{
-if (sd->state != sd_ready_state) {
+switch (sd->state) {
+case sd_ready_state:
+sd->state = sd_identification_state;
+return sd_r2_i;
+default:
return sd_invalid_state_for_cmd(sd, req);
}
-
-sd->state = sd_identification_state;
-
-return sd_r2_i;
}
static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
Re: [PATCH 01/23] hw/sd/sdcard: Correct code indentation
On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
Fix mis-alignment from commits 793d04f495 and 6380cd2052
("Add sd_cmd_SEND_TUNING_BLOCK" and "Add sd_cmd_SET_BLOCK_COUNT").
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 34 +-
1 file changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Cédric Le Goater
Thanks,
C.
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 16d8d52a78..626e99ecd6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1069,33 +1069,33 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState
*sd, SDRequest req)
static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
{
-if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
-return sd_cmd_illegal(sd, req);
-}
+if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
+return sd_cmd_illegal(sd, req);
+}
-if (sd->state != sd_transfer_state) {
-return sd_invalid_state_for_cmd(sd, req);
-}
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
-sd->state = sd_sendingdata_state;
-sd->data_offset = 0;
+sd->state = sd_sendingdata_state;
+sd->data_offset = 0;
-return sd_r1;
+return sd_r1;
}
static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
{
-if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
-return sd_cmd_illegal(sd, req);
-}
+if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
+return sd_cmd_illegal(sd, req);
+}
-if (sd->state != sd_transfer_state) {
-return sd_invalid_state_for_cmd(sd, req);
-}
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
-sd->multi_blk_cnt = req.arg;
+sd->multi_blk_cnt = req.arg;
-return sd_r1;
+return sd_r1;
}
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
Re: [PATCH 02/12] hw/vfio/pci: Replace sprintf() by g_strdup_printf()
On 4/12/24 17:25, Alex Williamson wrote: On Wed, 10 Apr 2024 18:06:03 +0200 Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Use g_strdup_printf() instead. Isn't this code only compiled for Linux hosts? It is not. Maybe still a valid change, but the rationale seems irrelevant. I agree the commit log should be rephrased. There is also a v2 doing a different change : https://lore.kernel.org/qemu-devel/20240411101550.99392-1-phi...@linaro.org/ This is a bit confusing. Thanks, C.
[PATCH for-9.1] migration: Add Error** argument to add_bitmaps_to_list()
This allows to report more precise errors in the migration handler
dirty_bitmap_save_setup().
Suggested-by Vladimir Sementsov-Ogievskiy
Signed-off-by: Cédric Le Goater
---
To apply on top of :
https://lore.kernel.org/qemu-devel/20240320064911.545001-1-...@redhat.com/
migration/block-dirty-bitmap.c | 34 --
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index
542a8c297b329abc30d1b3a205d29340fa59a961..a7d55048c23505fde565ca784cec3c917dca37e5
100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -481,13 +481,13 @@ static void dirty_bitmap_do_save_cleanup(DBMSaveState *s)
/* Called with the BQL taken. */
static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
- const char *bs_name, GHashTable *alias_map)
+ const char *bs_name, GHashTable *alias_map,
+ Error **errp)
{
BdrvDirtyBitmap *bitmap;
SaveBitmapState *dbms;
GHashTable *bitmap_aliases;
const char *node_alias, *bitmap_name, *bitmap_alias;
-Error *local_err = NULL;
/* When an alias map is given, @bs_name must be @bs's node name */
assert(!alias_map || !strcmp(bs_name, bdrv_get_node_name(bs)));
@@ -504,8 +504,8 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
bitmap_name = bdrv_dirty_bitmap_name(bitmap);
if (!bs_name || strcmp(bs_name, "") == 0) {
-error_report("Bitmap '%s' in unnamed node can't be migrated",
- bitmap_name);
+error_setg(errp, "Bitmap '%s' in unnamed node can't be migrated",
+ bitmap_name);
return -1;
}
@@ -525,9 +525,9 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
}
if (node_alias[0] == '#') {
-error_report("Bitmap '%s' in a node with auto-generated "
- "name '%s' can't be migrated",
- bitmap_name, node_alias);
+error_setg(errp, "Bitmap '%s' in a node with auto-generated "
+ "name '%s' can't be migrated",
+ bitmap_name, node_alias);
return -1;
}
@@ -538,8 +538,7 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
continue;
}
-if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
-error_report_err(local_err);
+if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
return -1;
}
@@ -558,9 +557,9 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
}
} else {
if (strlen(bitmap_name) > UINT8_MAX) {
-error_report("Cannot migrate bitmap '%s' on node '%s': "
- "Name is longer than %u bytes",
- bitmap_name, bs_name, UINT8_MAX);
+error_setg(errp, "Cannot migrate bitmap '%s' on node '%s': "
+ "Name is longer than %u bytes",
+ bitmap_name, bs_name, UINT8_MAX);
return -1;
}
bitmap_alias = bitmap_name;
@@ -599,7 +598,7 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
}
/* Called with the BQL taken. */
-static int init_dirty_bitmap_migration(DBMSaveState *s)
+static int init_dirty_bitmap_migration(DBMSaveState *s, Error **errp)
{
BlockDriverState *bs;
SaveBitmapState *dbms;
@@ -643,7 +642,7 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
}
if (bs && bs->drv && !bs->drv->is_filter) {
-if (add_bitmaps_to_list(s, bs, name, NULL)) {
+if (add_bitmaps_to_list(s, bs, name, NULL, errp)) {
goto fail;
}
g_hash_table_add(handled_by_blk, bs);
@@ -656,7 +655,8 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
continue;
}
-if (add_bitmaps_to_list(s, bs, bdrv_get_node_name(bs), alias_map)) {
+if (add_bitmaps_to_list(s, bs, bdrv_get_node_name(bs), alias_map,
+errp)) {
goto fail;
}
}
@@ -1218,9 +1218,7 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void
*opaque, Error **errp)
DBMSaveState *s = &((DBMState *)opaque)->save;
SaveBitmapState *dbms = NULL;
-if (init_dirty_bitmap_migration(s) < 0) {
-error_setg(errp,
- "Failed to initialize dirty tracking bitmap for blocks");
+if (init_dirty_bitmap_migration(s, errp) < 0) {
return -1;
}
--
2.44.0
Re: [PATCH for-9.1 v5 07/14] migration: Add Error** argument to .save_setup() handler
Hello Vladimir,
On 3/29/24 10:32, Vladimir Sementsov-Ogievskiy wrote:
On 20.03.24 09:49, Cédric Le Goater wrote:
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index
2708abf3d762de774ed294d3fdb8e56690d2974c..542a8c297b329abc30d1b3a205d29340fa59a961
100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -1213,12 +1213,14 @@ fail:
return ret;
}
-static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
+static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
DBMSaveState *s = &((DBMState *)opaque)->save;
SaveBitmapState *dbms = NULL;
if (init_dirty_bitmap_migration(s) < 0) {
+ error_setg(errp,
+ "Failed to initialize dirty tracking bitmap for blocks");
No, that's not about initializing a bitmap. This all is about migration of
block-dirty-bitmaps themselves.
So correct would be say "Failed to initialize migration of block dirty bitmaps".
with this, for block dirty bitmap migration:
Acked-by: Vladimir Sementsov-Ogievskiy
I had kept your previous R-b.
Should we remove it ? or is it ok if I address your comments below in a
followup patch, in which case the error message above would be removed.
Still, a lot better is add errp to init_dirty_bitmap_migration() and to
add_bitmaps_to_list() too: look,
init_dirty_bitmap_migration() fails only if add_bitmaps_to_list() fails
in turn,
add_bitmaps_to_list() have several clear failure points, where it always does
error_report (or error_report_err), which would be better to pass-through to
the user.
Good idea. Will do.
Thanks,
C.
[PATCH for-9.1 v5 07/14] migration: Add Error** argument to .save_setup() handler
The purpose is to record a potential error in the migration stream if
qemu_savevm_state_setup() fails. Most of the current .save_setup()
handlers can be modified to use the Error argument instead of managing
their own and calling locally error_report().
Cc: Nicholas Piggin
Cc: Harsh Prateek Bora
Cc: Halil Pasic
Cc: Thomas Huth
Cc: Eric Blake
Cc: Vladimir Sementsov-Ogievskiy
Cc: John Snow
Cc: Stefan Hajnoczi
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Xu
Reviewed-by: Thomas Huth
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Cédric Le Goater
---
include/migration/register.h | 3 ++-
hw/ppc/spapr.c | 2 +-
hw/s390x/s390-stattrib.c | 6 ++
hw/vfio/migration.c| 17 -
migration/block-dirty-bitmap.c | 4 +++-
migration/block.c | 13 -
migration/ram.c| 15 ---
migration/savevm.c | 4 +---
8 files changed, 29 insertions(+), 35 deletions(-)
diff --git a/include/migration/register.h b/include/migration/register.h
index
d7b70a8be68c9df47c7843bda7d430989d7ca384..64fc7c11036c82edd6d69513e56a0216d36c17aa
100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -60,10 +60,11 @@ typedef struct SaveVMHandlers {
*
* @f: QEMUFile where to send the data
* @opaque: data pointer passed to register_savevm_live()
+ * @errp: pointer to Error*, to store an error if it happens.
*
* Returns zero to indicate success and negative for error
*/
-int (*save_setup)(QEMUFile *f, void *opaque);
+int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);
/**
* @save_cleanup
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index
c417f9dd523547eabf6d66a8f505093758e80461..144a3f2b604872e09268b509b9b79ee5b2226136
100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2171,7 +2171,7 @@ static const VMStateDescription vmstate_spapr = {
}
};
-static int htab_save_setup(QEMUFile *f, void *opaque)
+static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
SpaprMachineState *spapr = opaque;
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index
b743e8a2fee84c7374460ccea6df1cf447cda44b..bc04187b2b69226db80219da1a964a87428adc0c
100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -168,19 +168,17 @@ static int cmma_load(QEMUFile *f, void *opaque, int
version_id)
return ret;
}
-static int cmma_save_setup(QEMUFile *f, void *opaque)
+static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
S390StAttribState *sas = S390_STATTRIB(opaque);
S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
-Error *local_err = NULL;
int res;
/*
* Signal that we want to start a migration, thus needing PGSTE dirty
* tracking.
*/
-res = sac->set_migrationmode(sas, true, &local_err);
+res = sac->set_migrationmode(sas, true, errp);
if (res) {
-error_report_err(local_err);
return res;
}
qemu_put_be64(f, STATTR_FLAG_EOS);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index
bf5a29ddc15b0dbc7ae9c44f289539dd0cdddb0d..5763c0b68376b1e24ef3e77c3d19fcd406922c79
100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -376,7 +376,7 @@ static int vfio_save_prepare(void *opaque, Error **errp)
return 0;
}
-static int vfio_save_setup(QEMUFile *f, void *opaque)
+static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
VFIODevice *vbasedev = opaque;
VFIOMigration *migration = vbasedev->migration;
@@ -390,8 +390,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
stop_copy_size);
migration->data_buffer = g_try_malloc0(migration->data_buffer_size);
if (!migration->data_buffer) {
-error_report("%s: Failed to allocate migration data buffer",
- vbasedev->name);
+error_setg(errp, "%s: Failed to allocate migration data buffer",
+ vbasedev->name);
return -ENOMEM;
}
@@ -401,8 +401,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
ret = vfio_migration_set_state(vbasedev,
VFIO_DEVICE_STATE_PRE_COPY,
VFIO_DEVICE_STATE_RUNNING);
if (ret) {
-error_report("%s: Failed to set new PRE_COPY state",
- vbasedev->name);
+error_setg(errp, "%s: Failed to set new PRE_COPY state",
+ vbasedev->name);
return ret;
}
@@ -413,8 +413,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
/* vfio_save_complete_precopy() will go to STOP_COPY */
break;
default:
-error_report("%s: Invalid device state %d", vbasedev->name
[PATCH for-9.1 v5 03/14] migration: Always report an error in block_save_setup()
This will prepare ground for future changes adding an Error** argument
to the save_setup() handler. We need to make sure that on failure,
block_save_setup() always sets a new error.
Cc: Stefan Hajnoczi
Reviewed-by: Fabiano Rosas
Signed-off-by: Cédric Le Goater
---
Changes in v5:
- Rebased on 2e128776dc56 ("migration: Skip only empty block devices")
migration/block.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/migration/block.c b/migration/block.c
index
2b9054889ad2ba739828594c50cf047703757e96..f8a11beb37dac3df5c2cc654db6440509d1181ea
100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -367,7 +367,7 @@ static void unset_dirty_tracking(void)
}
}
-static int init_blk_migration(QEMUFile *f)
+static int init_blk_migration(QEMUFile *f, Error **errp)
{
BlockDriverState *bs;
BlkMigDevState *bmds;
@@ -378,7 +378,6 @@ static int init_blk_migration(QEMUFile *f)
BlkMigDevState *bmds;
BlockDriverState *bs;
} *bmds_bs;
-Error *local_err = NULL;
int ret;
GRAPH_RDLOCK_GUARD_MAINLOOP();
@@ -406,6 +405,8 @@ static int init_blk_migration(QEMUFile *f)
continue;
}
if (sectors < 0) {
+error_setg(errp, "Error getting length of block device %s",
+ bdrv_get_device_name(bs));
ret = sectors;
bdrv_next_cleanup(&it);
goto out;
@@ -442,9 +443,8 @@ static int init_blk_migration(QEMUFile *f)
bs = bmds_bs[i].bs;
if (bmds) {
-ret = blk_insert_bs(bmds->blk, bs, &local_err);
+ret = blk_insert_bs(bmds->blk, bs, errp);
if (ret < 0) {
-error_report_err(local_err);
goto out;
}
@@ -714,6 +714,7 @@ static void block_migration_cleanup(void *opaque)
static int block_save_setup(QEMUFile *f, void *opaque)
{
int ret;
+Error *local_err = NULL;
trace_migration_block_save("setup", block_mig_state.submitted,
block_mig_state.transferred);
@@ -721,18 +722,27 @@ static int block_save_setup(QEMUFile *f, void *opaque)
warn_report("block migration is deprecated;"
" use blockdev-mirror with NBD instead");
-ret = init_blk_migration(f);
+ret = init_blk_migration(f, &local_err);
if (ret < 0) {
+error_report_err(local_err);
return ret;
}
/* start track dirty blocks */
ret = set_dirty_tracking();
if (ret) {
+error_setg_errno(&local_err, -ret,
+ "Failed to start block dirty tracking");
+error_report_err(local_err);
return ret;
}
ret = flush_blks(f);
+if (ret) {
+error_setg_errno(&local_err, -ret, "Flushing block failed");
+error_report_err(local_err);
+return ret;
+}
blk_mig_reset_dirty_cursor();
qemu_put_be64(f, BLK_MIG_FLAG_EOS);
--
2.44.0
[PATCH] aspeed/smc: Only wire flash devices at reset
The Aspeed machines have many Static Memory Controllers (SMC), up to
8, which can only drive flash memory devices. Commit 27a2c66c92ec
("aspeed/smc: Wire CS lines at reset") tried to ease the definitions
of these devices by allowing flash devices from the command line to be
attached to a SSI bus. For that, the wiring of the CS lines of the
Aspeed SMC controller was moved at reset. Two assumptions are made
though, first that the device has a SSI_GPIO_CS GPIO line, which is
not always the case, and second that it is flash device.
Correct this problem by ensuring that the devices attached to the bus
are the correct flash type. This fixes a QEMU abort when devices
without a CS line, such as the max111x, are passed on the command
line.
While at it, export TYPE_M25P80 used in the Xilinx Versal Virtual
machine.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2228
Fixes: 27a2c66c92ec ("aspeed/smc: Wire CS lines at reset")
Reported-by: Thomas Huth
Signed-off-by: Cédric Le Goater
---
include/hw/block/flash.h | 2 ++
hw/arm/xlnx-versal-virt.c | 3 ++-
hw/block/m25p80.c | 1 -
hw/ssi/aspeed_smc.c | 9 +
4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index de93756cbe8f..2b5ccd92f463 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -78,6 +78,8 @@ extern const VMStateDescription vmstate_ecc_state;
/* m25p80.c */
+#define TYPE_M25P80 "m25p80-generic"
+
BlockBackend *m25p80_get_blk(DeviceState *dev);
#endif
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index bfaed1aebfc6..962f98fee2ea 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -13,6 +13,7 @@
#include "qemu/error-report.h"
#include "qapi/error.h"
#include "sysemu/device_tree.h"
+#include "hw/block/flash.h"
#include "hw/boards.h"
#include "hw/sysbus.h"
#include "hw/arm/fdt.h"
@@ -759,7 +760,7 @@ static void versal_virt_init(MachineState *machine)
flash_klass = object_class_by_name(s->ospi_model);
if (!flash_klass ||
object_class_is_abstract(flash_klass) ||
-!object_class_dynamic_cast(flash_klass, "m25p80-generic")) {
+!object_class_dynamic_cast(flash_klass, TYPE_M25P80)) {
error_setg(&error_fatal, "'%s' is either abstract or"
" not a subtype of m25p80", s->ospi_model);
return;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 08a00a6d9b89..8dec134832a1 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -515,7 +515,6 @@ struct M25P80Class {
FlashPartInfo *pi;
};
-#define TYPE_M25P80 "m25p80-generic"
OBJECT_DECLARE_TYPE(Flash, M25P80Class, M25P80)
static inline Manufacturer get_man(Flash *s)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 0dff1d910778..de57e690e124 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -23,6 +23,7 @@
*/
#include "qemu/osdep.h"
+#include "hw/block/flash.h"
#include "hw/sysbus.h"
#include "migration/vmstate.h"
#include "qemu/log.h"
@@ -711,6 +712,14 @@ static void aspeed_smc_reset(DeviceState *d)
for (i = 0; i < asc->cs_num_max; i++) {
DeviceState *dev = ssi_get_cs(s->spi, i);
if (dev) {
+Object *o = OBJECT(dev);
+
+if (!object_dynamic_cast(o, TYPE_M25P80)) {
+warn_report("Aspeed SMC %s.%d : Invalid %s device type",
+BUS(s->spi)->name, i, object_get_typename(o));
+continue;
+}
+
qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
}
--
2.44.0
Re: [PATCH for 9.0] migration: Skip only empty block devices
On 3/12/24 19:41, Stefan Hajnoczi wrote:
On Tue, Mar 12, 2024 at 01:04:31PM +0100, Cédric Le Goater wrote:
The block .save_setup() handler calls a helper routine
init_blk_migration() which builds a list of block devices to take into
account for migration. When one device is found to be empty (sectors
== 0), the loop exits and all the remaining devices are ignored. This
is a regression introduced when bdrv_iterate() was removed.
Change that by skipping only empty devices.
Cc: Markus Armbruster
Suggested: Kevin Wolf
Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()")
It's not clear to me that fea68bb6e9fa introduced the bug. The code is
still <= 0 there and I don't see anything else that skips empty devices.
Can you explain the bug in fea68bb6e9fa?
Let me try. Initially, the code was :
static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
{
BlockDriverState *bs;
BlkMigDevState *bmds;
int64_t sectors;
if (!bdrv_is_read_only(bs)) {
sectors = bdrv_nb_sectors(bs);
if (sectors <= 0) {
return;
...
}
static void init_blk_migration(QEMUFile *f)
{
block_mig_state.submitted = 0;
block_mig_state.read_done = 0;
block_mig_state.transferred = 0;
block_mig_state.total_sector_sum = 0;
block_mig_state.prev_progress = -1;
block_mig_state.bulk_completed = 0;
block_mig_state.zero_blocks = migrate_zero_blocks();
bdrv_iterate(init_blk_migration_it, NULL);
}
bdrv_iterate() was iterating on all devices and exiting one iteration
early if the device was empty or an error detected. The loop applied
on all devices always, only empty devices and the ones with a failure
were skipped.
It was replaced by :
static void init_blk_migration(QEMUFile *f)
{
BlockDriverState *bs;
BlkMigDevState *bmds;
int64_t sectors;
block_mig_state.submitted = 0;
block_mig_state.read_done = 0;
block_mig_state.transferred = 0;
block_mig_state.total_sector_sum = 0;
block_mig_state.prev_progress = -1;
block_mig_state.bulk_completed = 0;
block_mig_state.zero_blocks = migrate_zero_blocks();
for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
if (bdrv_is_read_only(bs)) {
continue;
}
sectors = bdrv_nb_sectors(bs);
if (sectors <= 0) {
return;
...
}
The loop and function exit at first failure or first empty device,
skipping the remaining devices. This is a different behavior.
What we would want today is ignore empty devices, as it done for
the read only ones, return at first failure and let the caller of
init_blk_migration() report.
This is a short term fix for 9.0 because the block migration code
is deprecated and should be removed in 9.1.
Thanks,
C.
Re: [PATCH for 9.0] migration: Skip only empty block devices
On 3/12/24 13:04, Cédric Le Goater wrote:
The block .save_setup() handler calls a helper routine
init_blk_migration() which builds a list of block devices to take into
account for migration. When one device is found to be empty (sectors
== 0), the loop exits and all the remaining devices are ignored. This
is a regression introduced when bdrv_iterate() was removed.
Change that by skipping only empty devices.
Cc: Markus Armbruster
Suggested: Kevin Wolf
That's better :
Suggested-by: Kevin Wolf
Sorry for the noise,
C.
Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()")
Signed-off-by: Cédric Le Goater
---
migration/block.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/migration/block.c b/migration/block.c
index
8c6ebafacc1ffe930d1d4f19d968817b14852c69..2b9054889ad2ba739828594c50cf047703757e96
100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -402,7 +402,10 @@ static int init_blk_migration(QEMUFile *f)
}
sectors = bdrv_nb_sectors(bs);
-if (sectors <= 0) {
+if (sectors == 0) {
+continue;
+}
+if (sectors < 0) {
ret = sectors;
bdrv_next_cleanup(&it);
goto out;
[PATCH for 9.0] migration: Skip only empty block devices
The block .save_setup() handler calls a helper routine
init_blk_migration() which builds a list of block devices to take into
account for migration. When one device is found to be empty (sectors
== 0), the loop exits and all the remaining devices are ignored. This
is a regression introduced when bdrv_iterate() was removed.
Change that by skipping only empty devices.
Cc: Markus Armbruster
Suggested: Kevin Wolf
Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()")
Signed-off-by: Cédric Le Goater
---
migration/block.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/migration/block.c b/migration/block.c
index
8c6ebafacc1ffe930d1d4f19d968817b14852c69..2b9054889ad2ba739828594c50cf047703757e96
100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -402,7 +402,10 @@ static int init_blk_migration(QEMUFile *f)
}
sectors = bdrv_nb_sectors(bs);
-if (sectors <= 0) {
+if (sectors == 0) {
+continue;
+}
+if (sectors < 0) {
ret = sectors;
bdrv_next_cleanup(&it);
goto out;
--
2.44.0
Re: [PATCH v3 03/26] migration: Always report an error in block_save_setup()
On 3/4/24 22:04, Fabiano Rosas wrote:
Cédric Le Goater writes:
This will prepare ground for futur changes adding an Error** argument
to the save_setup() handler. We need to make sure that on failure,
block_save_setup() always sets a new error.
Cc: Stefan Hajnoczi
Signed-off-by: Cédric Le Goater
---
migration/block.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/migration/block.c b/migration/block.c
index
8c6ebafacc1ffe930d1d4f19d968817b14852c69..06f5857cf049df3261d2cf1d7c3607ab92350ac6
100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -367,7 +367,7 @@ static void unset_dirty_tracking(void)
}
}
-static int init_blk_migration(QEMUFile *f)
+static int init_blk_migration(QEMUFile *f, Error **errp)
{
BlockDriverState *bs;
BlkMigDevState *bmds;
@@ -378,7 +378,6 @@ static int init_blk_migration(QEMUFile *f)
BlkMigDevState *bmds;
BlockDriverState *bs;
} *bmds_bs;
-Error *local_err = NULL;
int ret;
GRAPH_RDLOCK_GUARD_MAINLOOP();
There's a negative return below at:
for (i = 0, bs = bdrv_first(&it); bs; bs = bdrv_next(&it), i++) {
if (bdrv_is_read_only(bs)) {
continue;
}
sectors = bdrv_nb_sectors(bs);
if (sectors <= 0) {
ret = sectors;
^
bdrv_next_cleanup(&it);
goto out;
}
...
Indeed.
I understand that the bdrv_nb_sectors() == 0 case only breaks the loop but
it is not considered as an error. Could the block folks confirm please ?
Thanks,
C.
I presume that must be covered by an error as well. A similar function
somewhere else reports:
total_sectors = blk_nb_sectors(blk);
if (total_sectors <= 0) {
error_report("Error getting length of block device %s",
device_name);
return -EINVAL;
}
@@ -439,9 +438,8 @@ static int init_blk_migration(QEMUFile *f)
bs = bmds_bs[i].bs;
if (bmds) {
-ret = blk_insert_bs(bmds->blk, bs, &local_err);
+ret = blk_insert_bs(bmds->blk, bs, errp);
if (ret < 0) {
-error_report_err(local_err);
goto out;
}
@@ -711,6 +709,7 @@ static void block_migration_cleanup(void *opaque)
static int block_save_setup(QEMUFile *f, void *opaque)
{
int ret;
+Error *local_err = NULL;
trace_migration_block_save("setup", block_mig_state.submitted,
block_mig_state.transferred);
@@ -718,18 +717,27 @@ static int block_save_setup(QEMUFile *f, void *opaque)
warn_report("block migration is deprecated;"
" use blockdev-mirror with NBD instead");
-ret = init_blk_migration(f);
+ret = init_blk_migration(f, &local_err);
if (ret < 0) {
+error_report_err(local_err);
return ret;
}
/* start track dirty blocks */
ret = set_dirty_tracking();
if (ret) {
+error_setg_errno(&local_err, -ret,
+ "Failed to start block dirty tracking");
+error_report_err(local_err);
return ret;
}
ret = flush_blks(f);
+if (ret) {
+error_setg_errno(&local_err, -ret, "Flushing block failed");
+error_report_err(local_err);
+return ret;
+}
blk_mig_reset_dirty_cursor();
qemu_put_be64(f, BLK_MIG_FLAG_EOS);
Re: [PATCH 05/21] hw/ppc/pnv_bmc: Use qdev_new() instead of QOM API
On 2/16/24 12:02, Philippe Mathieu-Daudé wrote:
Prefer QDev API for QDev objects, avoid the underlying QOM layer.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/ppc/pnv_bmc.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
index 99f1e8d7f9..0c1274df21 100644
--- a/hw/ppc/pnv_bmc.c
+++ b/hw/ppc/pnv_bmc.c
@@ -269,13 +269,13 @@ void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
*/
IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
{
-Object *obj;
+DeviceState *dev;
-obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
-qdev_realize(DEVICE(obj), NULL, &error_fatal);
-pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
+dev = qdev_new(TYPE_IPMI_BMC_SIMULATOR);
+qdev_realize(dev, NULL, &error_fatal);
+pnv_bmc_set_pnor(IPMI_BMC(dev), pnor);
-return IPMI_BMC(obj);
+return IPMI_BMC(dev);
}
typedef struct ForeachArgs {
Re: [PATCH v2 2/6] vfio: Avoid inspecting option QDict for rombar
On 2/12/24 09:04, Philippe Mathieu-Daudé wrote:
On 10/2/24 11:24, Akihiko Odaki wrote:
Use pci_rom_bar_explicitly_enabled() to determine if rombar is explicitly
enabled.
Signed-off-by: Akihiko Odaki
---
hw/vfio/pci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d7fe06715c4b..44178ac9355f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1010,7 +1010,6 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
{
uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
- DeviceState *dev = DEVICE(vdev);
char *name;
int fd = vdev->vbasedev.fd;
@@ -1044,7 +1043,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
}
if (vfio_opt_rom_in_denylist(vdev)) {
- if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
+ if (pci_rom_bar_explicitly_enabled(&vdev->pdev)) {
"pdev" is considered internal field, please use the DEVICE() macro
to access it.
Yes. I was just looking at vfio_pci_size_rom(). There is a test at
the beginning of this routine which should be changed to use DEVICE()
if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
/* Since pci handles romfile, just print a message and return */
if (vfio_opt_rom_in_denylist(vdev) && vdev->pdev.romfile) {
...
Thanks,
C.
Re: [PATCH 1/2] target/sh4: Deprecate the shix machine
On 1/8/24 18:15, Samuel Tardieu wrote:
The shix machine has been designed and used at Télécom Paris from 2003
to 2010. It had been added to QEMU in 2005 and has not been maintained
since. Since nobody is using the physical board anymore nor interested
in maintaining the QEMU port, it is time to deprecate it.
Signed-off-by: Samuel Tardieu
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
docs/about/deprecated.rst | 5 +
hw/sh4/shix.c | 1 +
2 files changed, 6 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2e15040246..e6a12c9077 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -269,6 +269,11 @@ Nios II ``10m50-ghrd`` and ``nios2-generic-nommu``
machines (since 8.2)
The Nios II architecture is orphan.
+``shix`` (since 9.0)
+''''''''''''''''''''
+
+The machine is no longer in existence and has been long unmaintained
+in QEMU.
Backend options
---
diff --git a/hw/sh4/shix.c b/hw/sh4/shix.c
index aa812512f0..58530b8ede 100644
--- a/hw/sh4/shix.c
+++ b/hw/sh4/shix.c
@@ -80,6 +80,7 @@ static void shix_machine_init(MachineClass *mc)
mc->init = shix_init;
mc->is_default = true;
mc->default_cpu_type = TYPE_SH7750R_CPU;
+mc->deprecation_reason = "old and unmaintained - use a newer machine
instead";
}
DEFINE_MACHINE("shix", shix_machine_init)
Re: [PATCH 2/2] hw/block: Deprecate the TC58128 block device
On 1/8/24 18:15, Samuel Tardieu wrote:
The 16MiB flash device is only used by the deprecated shix machine.
Its code it old and unmaintained, and has never been adapted to the
QOM architecture. It still contains debug statements and uses global
variables. It is time to deprecate it.
Signed-off-by: Samuel Tardieu
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
docs/about/deprecated.rst | 2 +-
hw/block/tc58128.c| 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index e6a12c9077..15e39f8bbb 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -273,7 +273,7 @@ The Nios II architecture is orphan.
''''''''''''''''''''
The machine is no longer in existence and has been long unmaintained
-in QEMU.
+in QEMU. This also holds for the TC51828 16MiB flash that it uses.
Backend options
---
diff --git a/hw/block/tc58128.c b/hw/block/tc58128.c
index d350126b27..354c13ccf0 100644
--- a/hw/block/tc58128.c
+++ b/hw/block/tc58128.c
@@ -202,6 +202,7 @@ static sh7750_io_device tc58128 = {
int tc58128_init(struct SH7750State *s, const char *zone1, const char *zone2)
{
+warn_report_once("The TC58128 flash device is deprecated - use a newer
component");
init_dev(&tc58128_devs[0], zone1);
init_dev(&tc58128_devs[1], zone2);
return sh7750_register_io_device(s, &tc58128);
