Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu <peterx@redhat.com>]

On Wed, Nov 20, 2024 at 12:45:19PM +0100, Paolo Bonzini wrote:
> Il mar 19 nov 2024, 22:43 Peter Xu <peterx@redhat.com> ha scritto:
> 
> > > The easiest way to check is probably to print the type of every
> > successful
> > > object_dynamic_cast and object_class_dynamic_cast. I suspect the result
> > > will be virtio-blk-device and/or scsi-hd, but maybe those already do an
> > > unsafe cast (pointer type cast) instead of object_dynamic_cast.
> >
> > Yes, it sounds more reasonable to me to optimize specific call sites so far
> > rather than provides something generic.
> 
> Though it could still be a
> > generic API so that devices can opt-in.
> 
> 
> One of the things that I am excited about for Rust is checking at compile
> time whether a cast is to a superclass, which makes it safe automatically.

I see.  However looks like it doesn't easily apply to the ahci example
below, where it could conditionally fail the cast (and where I got it
wrong..)?

> 
> > I can give it some measurement if there is, otherwise I'm
> > > > guessing whatever changes could fall into the noise.
> > >
> > >
> > > Yes, probably. At most you can identify if there any heavy places out of
> > > the 34000 calls, and see if they can use an unsafe cast.
> >
> > I can still trivially do this.
> >
> > I traced qemu using bpf
> 
> 
> Nice! I want to know more. :))

I also only learned it yesterday, where I only used to use k*probes
previously. :-) That's:

$ cat qemu.bpf
uprobe:/home/peterx/git/qemu/bin/qemu-system-x86_64:object_class_dynamic_cast
{
        @out[ustack()]++;
}
$ sudo bpftrace --usdt-file-activation ./qemu.bpf

> 
> > and interestingly in my case close to half (over
> > 10000+) of the calls are about ahci_irq_lower() from different higher level
> > stack (yeah I used IDE in my setup.. with a split irqchi..), where it has:
> >
> >     PCIDevice *pci_dev = (PCIDevice *)
> > object_dynamic_cast(OBJECT(dev_state),
> >
> >  TYPE_PCI_DEVICE);
> >
> > So IIUC that can be open to a unsafe cast too
> 
> 
> Hmm no it can't because there's also sysbus AHCI. The fix would be to add
> an AHCIClass and make irq toggling into a method there

Yep, I overlooked the lines of code later.. :(

> 
> but considering IDE is ODD FIXES stage, I'm not sure if I should send a
> > patch at all.  However I copied John regardless.
> >
> 
> Well, MAINTAINERS only says the kind of work that the maintainer is doing,
> you can always do more. However it seems like not a small amount, so maybe
> adding a comment is enough if somebody else wants to do it?

Can do.

-- 
Peter Xu