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

["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 ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|