Re: [QEMU][PATCH v4 1/2] xen_arm: Create virtio-mmio devices during initialization

[Anthony PERARD]

On Wed, Oct 11, 2023 at 12:22:46PM -0700, Vikram Garhwal wrote:
> Hi Anthony,
> On Thu, Oct 05, 2023 at 11:40:57AM +0100, Anthony PERARD wrote:
> > Hi Vikram,
> > 
> > This patch prevent QEMU from been build with Xen 4.15. See comments.
> > 
> > Also, why didn't you CC all the maintainers of
> > include/hw/xen/xen_native.h?
> I missed it. Initial version didn't have this file change and i missed 
> updating
> my cc list.

I use `cccmd` to never miss anyone, and I don't have to build a cc list ;-)

$ git config sendemail.cccmd
scripts/get_maintainer.pl --noroles --norolestats --nogit --nogit-fallback

> > > +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle 
> > > *dmod,
> > > +                                               domid_t domid, uint32_t 
> > > irq,
> > > +                                               unsigned int level)
> > > +{
> > > +    return 0;
> > 
> > Shouldn't this return something like -ENOSYS, instead of returning a
> > success?
> Changed return to -ENOSYS for older version.

Actually, at least on linux, looks like the function would return either
-1 or 0, and set errno. It seems that xendevicemodel_set_irq_level()
ultimately called ioctl(), but also the code in
xen.git/tools/libs/devicemodel/ also only returns -1 or 0.

So it's probably best to set errno=ENOSYS and return -1.

> > 
> > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
> > > index 1d3e6d481a..7393b37355 100644
> > > --- a/hw/arm/xen_arm.c
> > > +++ b/hw/arm/xen_arm.c
> > > +
> > > +static void xen_set_irq(void *opaque, int irq, int level)
> > > +{
> > > +    xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level);
> > 
> > So, you just ignore the return value here. Shouldn't there be some kind
> > of error check?
> > 
> > And is it OK to create a virtio-mmio device without an error, even when
> > we could find out that it never going to work (e.g. on Xen 4.14)?
> This is something Oleksandr can answer better as it was written by him. But
> I think we can print an error "virtio init failed" and exit the
> machine init. Does that aligns with your thinking?

Something like that, yes, if possible. It would be a bit difficult
because xen_set_irq() seems to only be a handler which might only be
called after the machine as started. So I'm not sure what would be best
to do here.

Thanks,

-- 
Anthony PERARD