Re: [PATCH 2/2] rust/hw/char/pl011/src/device: Implement logging

[Bernhard Beschow <shentey@gmail.com>]

Am 2. April 2025 13:27:53 UTC schrieb "Daniel P. Berrangé" <berrange@redhat.com>:
>On Wed, Apr 02, 2025 at 09:33:16AM +0000, Bernhard Beschow wrote:
>> 
>> 
>> Am 31. März 2025 09:18:05 UTC schrieb "Daniel P. Berrangé" <berrange@redhat.com>:
>> >On Sun, Mar 30, 2025 at 10:58:57PM +0200, Bernhard Beschow wrote:
>> >> Now that there is logging support in Rust for QEMU, use it in the pl011 device.
>> >> 
>> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> >> ---
>> >>  rust/hw/char/pl011/src/device.rs | 12 ++++++++----
>> >>  1 file changed, 8 insertions(+), 4 deletions(-)
>> >> 
>> >> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
>> >> index bf88e0b00a..d5470fae11 100644
>> >> --- a/rust/hw/char/pl011/src/device.rs
>> >> +++ b/rust/hw/char/pl011/src/device.rs
>> >> @@ -8,9 +8,11 @@
>> >>      chardev::{CharBackend, Chardev, Event},
>> >>      impl_vmstate_forward,
>> >>      irq::{IRQState, InterruptSource},
>> >> +    log::{LOG_GUEST_ERROR, LOG_UNIMP},
>> >>      memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
>> >>      prelude::*,
>> >>      qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
>> >> +    qemu_log_mask,
>> >>      qom::{ObjectImpl, Owned, ParentField},
>> >>      static_assert,
>> >>      sysbus::{SysBusDevice, SysBusDeviceImpl},
>> >> @@ -298,8 +300,7 @@ pub(self) fn write(
>> >>              DMACR => {
>> >>                  self.dmacr = value;
>> >>                  if value & 3 > 0 {
>> >> -                    // qemu_log_mask(LOG_UNIMP, "pl011: DMA not implemented\n");
>> >> -                    eprintln!("pl011: DMA not implemented");
>> >> +                    qemu_log_mask!(LOG_UNIMP, "pl011: DMA not implemented\n");
>> >>                  }
>> >>              }
>> >>          }
>> >> @@ -535,7 +536,7 @@ fn read(&self, offset: hwaddr, _size: u32) -> u64 {
>> >>                  u64::from(device_id[(offset - 0xfe0) >> 2])
>> >>              }
>> >>              Err(_) => {
>> >> -                // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
>> >> +                qemu_log_mask!(LOG_GUEST_ERROR, "pl011_read: Bad offset {offset}\n");
>> >>                  0
>> >>              }
>> >>              Ok(field) => {
>> >> @@ -567,7 +568,10 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
>> >>                  .borrow_mut()
>> >>                  .write(field, value as u32, &self.char_backend);
>> >>          } else {
>> >> -            eprintln!("write bad offset {offset} value {value}");
>> >> +            qemu_log_mask!(
>> >> +                LOG_GUEST_ERROR,
>> >> +                "pl011_write: Bad offset {offset} value {value}\n"
>> >> +            );
>> >>          }
>> >
>> >General conceptual question .....  I've never understood what the dividing
>> >line is between use of 'qemu_log_mask' and trace points.
>> 
>> I *think* it's the perspective: If you want to see any issues, regardless
>> of which device, use the -l option, i.e. qemu_log_mask(). If, however,
>> you want to see what a particular device does, use tracepoints.
>
>I guess I'd say that the latter ought to be capable of satisfying the
>former use case too, given a suitable trace point selection. If it
>can't, then perhaps that's telling us the way we select trace points
>is insufficiently expressive ?

Tracepoints often encode some context in the function name, e.g. the device name and the operation being performed. One could give up this context information in the function names and pass it as arguments to generic trace_unimp() and trace_guest_error() functions. The drawback is that this doesn't provide any advantage over the current logging functionality such as device filtering. That could be achieved by  trace_$device_$operation_{unimp,guest_error} functions which is a convention that has to be enforced manually.

Best regards,
Bernhard

>
>With regards,
>Daniel