Am 6. Oktober 2024 17:12:16 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 5 Oct 2024, Bernhard Beschow wrote:
>> The CCSR space is just a container which is meant to be covered by platform
>> device memory regions. However, QEMU only implements a subset of these devices.
>> Add some logging to see which devices a guest attempts to access.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/ppce500_ccsr.c | 32 +++++++++++++++++++++++++++++++-
>> hw/ppc/trace-events | 3 +++
>> 2 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
>> index 5d0e1e0e89..6659560674 100644
>> --- a/hw/ppc/ppce500_ccsr.c
>> +++ b/hw/ppc/ppce500_ccsr.c
>> @@ -13,12 +13,42 @@
>>
>> #include "qemu/osdep.h"
>> #include "ppce500_ccsr.h"
>> +#include "qemu/log.h"
>> +#include "trace.h"
>> +
>> +static uint64_t ppce500_ccsr_io_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> + uint64_t value = 0;
>> +
>> + trace_ppce500_ccsr_io_read(addr, value, size);
>> + qemu_log_mask(LOG_UNIMP,
>> + "%s: unimplemented [0x%" HWADDR_PRIx "] -> 0\n",
>> + __func__, addr);
>
>I'm not sure having both unimp log and traces is the best way. I thought unimp log with an unimplemented device area would be the simplest and least intrusive for the code but if you prefer traces then maybe we don't need unimp logs. But adding these otherwise empty functions (which won't get populated as subdevices have their own regions) still bothers me a bit but not enough to block this if others have no opinion on it.
>
>I also had this patch:
>https://patchew.org/QEMU/cover.1728232526.git.balaton@eik.bme.hu/
>which I first thought might help but that's about guest_errors not unimp logs so does not apply here. What other unimp logs get in the way here that makes traces a better choice?
I have some rough implementations of a few other device models with unimp logging. Having a dedicated trace here allows to see which unoccupied CCSR regions a guest pokes. The unimp logging here actually disturbs the above, so I'd remove it here.
Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> +
>> + return value;
>> +}
>> +
>> +static void ppce500_ccsr_io_write(void *opaque, hwaddr addr, uint64_t value,
>> + unsigned size)
>> +{
>> + trace_ppce500_ccsr_io_write(addr, value, size);
>> + qemu_log_mask(LOG_UNIMP,
>> + "%s: unimplemented [0x%" HWADDR_PRIx "] <- 0x%" PRIx32 "\n",
>> + __func__, addr, (uint32_t)value);
>> +}
>> +
>> +static const MemoryRegionOps ppce500_ccsr_ops = {
>> + .read = ppce500_ccsr_io_read,
>> + .write = ppce500_ccsr_io_write,
>> + .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>>
>> static void ppce500_ccsr_init(Object *obj)
>> {
>> PPCE500CCSRState *s = CCSR(obj);
>>
>> - memory_region_init(&s->ccsr_space, obj, "e500-ccsr", MPC85XX_CCSRBAR_SIZE);
>> + memory_region_init_io(&s->ccsr_space, obj, &ppce500_ccsr_ops, obj,
>> + "e500-ccsr", MPC85XX_CCSRBAR_SIZE);
>> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->ccsr_space);
>> }
>>
>> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
>> index 1f125ce841..ca4c231c9f 100644
>> --- a/hw/ppc/trace-events
>> +++ b/hw/ppc/trace-events
>> @@ -143,6 +143,9 @@ ppc_irq_cpu(const char *action) "%s"
>> ppc_dcr_read(uint32_t addr, uint32_t val) "DRCN[0x%x] -> 0x%x"
>> ppc_dcr_write(uint32_t addr, uint32_t val) "DRCN[0x%x] <- 0x%x"
>>
>> +ppce500_ccsr_io_read(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] -> 0x%08x (size: 0x%" PRIu8 ")"
>> +ppce500_ccsr_io_write(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] <- 0x%08x (size: 0x%" PRIu8 ")"
>> +
>> # prep_systemio.c
>> prep_systemio_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
>> prep_systemio_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
>>