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

[Bernhard Beschow]

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