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.
>
> On Tue, Aug 29, 2023 at 09:35:17PM -0700, Vikram Garhwal wrote:
> > diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h
> > index 4dce905fde..a4b1aa9e5d 100644
> > --- a/include/hw/xen/xen_native.h
> > +++ b/include/hw/xen/xen_native.h
> > @@ -523,4 +523,20 @@ static inline int xen_set_ioreq_server_state(domid_t
> > dom,
> > enable);
> > }
> >
> > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41500
>
> xendevicemodel_set_irq_level() was introduced in Xen 4.15, so this
> should say '<' and not '<=', otherwise, we have:
> include/hw/xen/xen_native.h:527:19: error: static declaration of
> ‘xendevicemodel_set_irq_level’ follows non-static declaration
Thanks for spotting this.
I will create a fix and gitlab-ci for the patch.
>
> > +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.
>
> > 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?
>
> Cheers,
>
> --
> Anthony PERARD
