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"
+ );
}
if update_irq {
self.update();
--
2.49.0On 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. The latter can be
optionally built to feed into qemu log, as well as the other dynamic trace
backends.
Is there a compelling reason to use 'qemu_log', that isn't acceptable for
trace probe points ?
This is an indirect way of asking whether qemu_log_mask should be exposed
to rust, or would exposing tracing be sufficient ?
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 :|
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.
>The latter can be
>optionally built to feed into qemu log, as well as the other dynamic trace
>backends.
The use of qemu_log() in qemu_log_mask() seems like an implementation detail to me. Theoretically, qemu_log_mask() could use a different backend if this got implemented, and wouldn't require code changes throughout QEMU.
Best regards,
Bernhard
>
>Is there a compelling reason to use 'qemu_log', that isn't acceptable for
>trace probe points ?
>
>This is an indirect way of asking whether qemu_log_mask should be exposed
>to rust, or would exposing tracing be sufficient ?
>
>With regards,
>Daniel
On Wed, 2 Apr 2025, Bernhard Beschow wrote: > Am 31. März 2025 09:18:05 UTC schrieb "Daniel P. Berrangé" <berrange@redhat.com>: >> 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'd say that traces are like debug printfs that you don't normally want to see but may be interesting for developers for debugging a specific device model or QEMU part so you can enable the relevant ones for that device or part. Logs are something you want to notify the user or admin about and is not tracing internal operation of a device. But there may be some overlap as some logs could be converted to traces but they are under log for historical reasons as previously there were debug printfs controlled by defines in devices that were later converted to traces and logs that could be controlled in run time which were used for some traces before trace points existed. Then were probably kept as logs just to not change the command line because it's easier to type -d in_asm then say -trace enable="tcg_in_asm" or something like that. Regards, BALATON Zoltan >> The latter can be >> optionally built to feed into qemu log, as well as the other dynamic trace >> backends. > > The use of qemu_log() in qemu_log_mask() seems like an implementation detail to me. Theoretically, qemu_log_mask() could use a different backend if this got implemented, and wouldn't require code changes throughout QEMU. > > Best regards, > Bernhard > >> >> Is there a compelling reason to use 'qemu_log', that isn't acceptable for >> trace probe points ? >> >> This is an indirect way of asking whether qemu_log_mask should be exposed >> to rust, or would exposing tracing be sufficient ? >> >> With regards, >> Daniel > >
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 :|
On Wed, 2 Apr 2025 at 14:28, Daniel P. Berrangé <berrange@redhat.com> wrote: > 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>: > > >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 ? Yeah; you can turn on and off a tracepoint, and you can select them by wildcard, but there's no categorization of them (into eg "this is basically the equivalent of a debug printf" vs "this is something that is a guest error you probably want to know about"). There's also no way to say "turn on this logging with one switch, and it will print multiple lines or more than one thing" (at least not in the spirit of what the tracepoint API expects; you could have a trace_in_asm tracepoint that took a "%s" and output whatever you liked as the string, of course). And debug-logging is more documented: '-d help' shows what you can turn on and off and has at least a brief description of what it is you're getting. For tracepoints you're hoping that the name is vaguely descriptive and also hoping that the device/subsystem/etc named its tracepoints in a way that lets you usefully wildcard them. Also, the qemu_log() logging assumes "we're sending text to a logfile, we can format e.g. register dumps and disassembly as arbitrary laid out plaintext". That's fine if your tracepoint backend is also "we just send the text to a logfile/etc", but I don't know if all of the tracepoint backends would be so happy with that. thanks -- PMM
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
© 2016 - 2025 Red Hat, Inc.