Re: [PATCH 0/2] arm: Add collie and sx functional tests
On 10/27/24 15:26, Cédric Le Goater wrote:
On 10/27/24 23:11, Guenter Roeck wrote:
On 10/27/24 14:13, Cédric Le Goater wrote:
On 10/26/24 17:32, Guenter Roeck wrote:
On 10/26/24 03:02, Cédric Le Goater wrote:
[ ... ]
I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.
Our only use case, in 2019, was to boot QEMU ast2600 machines from an
eMMC device using an OpenBMC FW image like the ones we find on IBM
Power10 Rainier systems. I agree we only focused on this scenario.
Most of the support should be there for other use cases, and it's now
a question of finding the right tunables for the user.
I did a quick experiment using 2 patches,
one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
area in emmc image")
@@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
sect = 0;
}
size = sect << HWBLOCK_SHIFT;
- size -= sd_bootpart_offset(sd);
+ if (sd_is_emmc(sd)) {
+ size -= sd->boot_part_size * 2;
+ }
sect = sd_addr_to_wpnum(size) + 1;
and another on hw/arm/aspeed.c to remove the setting of the eMMC
device properties when it is not bootable :
@@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
return;
}
card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
- if (emmc) {
+ if (emmc && boot_emmc) {
qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
qdev_prop_set_uint8(card, "boot-config",
boot_emmc ? 0x1 << 3 : 0x0);
(I am not saying this is correct)
Works for me, though, and it is much better than mandating the existence
of boot partitions.
Yes. However, if the emmc device was user creatable, we could use :
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
-device emmc,bus=sdhci-bus.2,drive=emmc0
and with boot partitions:
-M boot-emmc=true \
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
-device
emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
The above would be my preferred approach if acceptable. The "sd-bus"
bus identifier should be changed in other machines tough.
No real preference here, though my understanding is that emmc devices
are by definition built-in, and that is what emmc_class_init() says as well.
Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
not support any index values. That may be just my lack of knowledge, though.
No, you are right. On a real ast2600-evb, the eMMC device is indeed
soldered on the board. But, for testing purposes, it is sometime
interesting to add some flexibility in the machine definition and
in the modeling too. This avoids "hard-coding" default devices in
the machines and lets the user define its own variant models using
the QEMU command line.
I would agree, but I had a number of my patches rejected because while
they would be useful for testing they would not accurately reflect the
hardware. So nowadays I gave up even trying to upstream such changes.
Guenter
Re: [PATCH v17 02/14] hw/ppc/spapr_pci: Do not create DT for disabled PCI device
On 10/22/24 2:06 PM, Akihiko Odaki wrote:
Disabled means it is a disabled SR-IOV VF and hidden from the guest.
Do not create DT when starting the system and also keep the disabled PCI
device not linked to DRC, which generates DT in case of hotplug.
Signed-off-by: Akihiko Odaki
---
hw/ppc/spapr_pci.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5c0024bef9c4..679a22fe4e79 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1291,8 +1291,7 @@ static void spapr_dt_pci_device_cb(PCIBus *bus, PCIDevice
*pdev,
PciWalkFdt *p = opaque;
int err;
-if (p->err) {
-/* Something's already broken, don't keep going */
+if (p->err || !pdev->enabled) {
return;
}
@@ -1592,10 +1591,10 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
uint32_t slotnr = PCI_SLOT(pdev->devfn);
/*
- * If DR is disabled we don't need to do anything in the case of
- * hotplug or coldplug callbacks.
+ * If DR or the PCI device is disabled we don't need to do anything
+ * in the case of hotplug or coldplug callbacks.
*/
-if (!phb->dr_enabled) {
+if (!phb->dr_enabled || !pdev->enabled) {
return;
}
Thank you. This is the right place to be called from the hotplug handler
instead of the spapr_pci_dt_populate() unlike I mentioned before..
@@ -1680,6 +1679,11 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
}
g_assert(drc);
+
+if (!drc->dev) {
+return;
I agree with the change here, but were you able to get to this path? I
don't see
this if condition being entered with,
qemu-system-ppc64 -nographic -serial none -device
spapr-pci-host-bridge,index=4,id=pci.1 -device igb,id=igb0 <<<
'device_del igb0'
Regards,
Shivaprasad
+}
+
g_assert(drc->dev == plugged_dev);
if (!spapr_drc_unplug_requested(drc)) {
Re: [PATCH v17 02/14] hw/ppc/spapr_pci: Do not create DT for disabled PCI device
On 2024/10/28 12:08, Shivaprasad G Bhat wrote:
On 10/22/24 2:06 PM, Akihiko Odaki wrote:
Disabled means it is a disabled SR-IOV VF and hidden from the guest.
Do not create DT when starting the system and also keep the disabled PCI
device not linked to DRC, which generates DT in case of hotplug.
Signed-off-by: Akihiko Odaki
---
hw/ppc/spapr_pci.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5c0024bef9c4..679a22fe4e79 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1291,8 +1291,7 @@ static void spapr_dt_pci_device_cb(PCIBus *bus,
PCIDevice *pdev,
PciWalkFdt *p = opaque;
int err;
- if (p->err) {
- /* Something's already broken, don't keep going */
+ if (p->err || !pdev->enabled) {
return;
}
@@ -1592,10 +1591,10 @@ static void spapr_pci_plug(HotplugHandler
*plug_handler,
uint32_t slotnr = PCI_SLOT(pdev->devfn);
/*
- * If DR is disabled we don't need to do anything in the case of
- * hotplug or coldplug callbacks.
+ * If DR or the PCI device is disabled we don't need to do anything
+ * in the case of hotplug or coldplug callbacks.
*/
- if (!phb->dr_enabled) {
+ if (!phb->dr_enabled || !pdev->enabled) {
return;
}
Thank you. This is the right place to be called from the hotplug handler
instead of the spapr_pci_dt_populate() unlike I mentioned before..
@@ -1680,6 +1679,11 @@ static void
spapr_pci_unplug_request(HotplugHandler *plug_handler,
}
g_assert(drc);
+
+ if (!drc->dev) {
+ return;
I agree with the change here, but were you able to get to this path? I
don't see
this if condition being entered with,
qemu-system-ppc64 -nographic -serial none -device spapr-pci-host-
bridge,index=4,id=pci.1 -device igb,id=igb0 <<< 'device_del igb0'
No. VFs bypass the hotplug path when unplugging. For context, see
unparent_vfs() in "[PATCH v17 09/14] pcie_sriov: Reuse SR-IOV VF device
instances.
Regards,
Akihiko Odaki
Regards,
Shivaprasad
+ }
+
g_assert(drc->dev == plugged_dev);
if (!spapr_drc_unplug_requested(drc)) {
Re: [PATCH 0/2] arm: Add collie and sx functional tests
On 10/27/24 23:11, Guenter Roeck wrote:
On 10/27/24 14:13, Cédric Le Goater wrote:
On 10/26/24 17:32, Guenter Roeck wrote:
On 10/26/24 03:02, Cédric Le Goater wrote:
[ ... ]
I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.
Our only use case, in 2019, was to boot QEMU ast2600 machines from an
eMMC device using an OpenBMC FW image like the ones we find on IBM
Power10 Rainier systems. I agree we only focused on this scenario.
Most of the support should be there for other use cases, and it's now
a question of finding the right tunables for the user.
I did a quick experiment using 2 patches,
one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
area in emmc image")
@@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
sect = 0;
}
size = sect << HWBLOCK_SHIFT;
- size -= sd_bootpart_offset(sd);
+ if (sd_is_emmc(sd)) {
+ size -= sd->boot_part_size * 2;
+ }
sect = sd_addr_to_wpnum(size) + 1;
and another on hw/arm/aspeed.c to remove the setting of the eMMC
device properties when it is not bootable :
@@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
return;
}
card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
- if (emmc) {
+ if (emmc && boot_emmc) {
qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
qdev_prop_set_uint8(card, "boot-config",
boot_emmc ? 0x1 << 3 : 0x0);
(I am not saying this is correct)
Works for me, though, and it is much better than mandating the existence
of boot partitions.
Yes. However, if the emmc device was user creatable, we could use :
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
-device emmc,bus=sdhci-bus.2,drive=emmc0
and with boot partitions:
-M boot-emmc=true \
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
-device
emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
The above would be my preferred approach if acceptable. The "sd-bus"
bus identifier should be changed in other machines tough.
No real preference here, though my understanding is that emmc devices
are by definition built-in, and that is what emmc_class_init() says as well.
Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
not support any index values. That may be just my lack of knowledge, though.
No, you are right. On a real ast2600-evb, the eMMC device is indeed
soldered on the board. But, for testing purposes, it is sometime
interesting to add some flexibility in the machine definition and
in the modeling too. This avoids "hard-coding" default devices in
the machines and lets the user define its own variant models using
the QEMU command line.
C.
Guenter
If you end up submitting those patches, please feel free to add
Tested-by: Guenter Roeck
I should send these fixes for QEMU 9.2.
Thanks,
C.
Re: [PATCH 0/2] arm: Add collie and sx functional tests
On 10/26/24 17:32, Guenter Roeck wrote:
On 10/26/24 03:02, Cédric Le Goater wrote:
[ ... ]
I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.
Our only use case, in 2019, was to boot QEMU ast2600 machines from an
eMMC device using an OpenBMC FW image like the ones we find on IBM
Power10 Rainier systems. I agree we only focused on this scenario.
Most of the support should be there for other use cases, and it's now
a question of finding the right tunables for the user.
I did a quick experiment using 2 patches,
one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
area in emmc image")
@@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
sect = 0;
}
size = sect << HWBLOCK_SHIFT;
- size -= sd_bootpart_offset(sd);
+ if (sd_is_emmc(sd)) {
+ size -= sd->boot_part_size * 2;
+ }
sect = sd_addr_to_wpnum(size) + 1;
and another on hw/arm/aspeed.c to remove the setting of the eMMC
device properties when it is not bootable :
@@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
return;
}
card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
- if (emmc) {
+ if (emmc && boot_emmc) {
qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
qdev_prop_set_uint8(card, "boot-config",
boot_emmc ? 0x1 << 3 : 0x0);
(I am not saying this is correct)
Works for me, though, and it is much better than mandating the existence
of boot partitions.
Yes. However, if the emmc device was user creatable, we could use :
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
-device emmc,bus=sdhci-bus.2,drive=emmc0
and with boot partitions:
-M boot-emmc=true \
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
-device
emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
The above would be my preferred approach if acceptable. The "sd-bus"
bus identifier should be changed in other machines tough.
If you end up submitting those patches, please feel free to add
Tested-by: Guenter Roeck
I should send these fixes for QEMU 9.2.
Thanks,
C.
Re: [PATCH 0/2] arm: Add collie and sx functional tests
On 10/27/24 14:13, Cédric Le Goater wrote:
On 10/26/24 17:32, Guenter Roeck wrote:
On 10/26/24 03:02, Cédric Le Goater wrote:
[ ... ]
I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.
Our only use case, in 2019, was to boot QEMU ast2600 machines from an
eMMC device using an OpenBMC FW image like the ones we find on IBM
Power10 Rainier systems. I agree we only focused on this scenario.
Most of the support should be there for other use cases, and it's now
a question of finding the right tunables for the user.
I did a quick experiment using 2 patches,
one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
area in emmc image")
@@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
sect = 0;
}
size = sect << HWBLOCK_SHIFT;
- size -= sd_bootpart_offset(sd);
+ if (sd_is_emmc(sd)) {
+ size -= sd->boot_part_size * 2;
+ }
sect = sd_addr_to_wpnum(size) + 1;
and another on hw/arm/aspeed.c to remove the setting of the eMMC
device properties when it is not bootable :
@@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
return;
}
card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
- if (emmc) {
+ if (emmc && boot_emmc) {
qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
qdev_prop_set_uint8(card, "boot-config",
boot_emmc ? 0x1 << 3 : 0x0);
(I am not saying this is correct)
Works for me, though, and it is much better than mandating the existence
of boot partitions.
Yes. However, if the emmc device was user creatable, we could use :
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
-device emmc,bus=sdhci-bus.2,drive=emmc0
and with boot partitions:
-M boot-emmc=true \
-blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
-device
emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
The above would be my preferred approach if acceptable. The "sd-bus"
bus identifier should be changed in other machines tough.
No real preference here, though my understanding is that emmc devices
are by definition built-in, and that is what emmc_class_init() says as well.
Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
not support any index values. That may be just my lack of knowledge, though.
Guenter
If you end up submitting those patches, please feel free to add
Tested-by: Guenter Roeck
I should send these fixes for QEMU 9.2.
Thanks,
C.
