Re: [PATCH v3 for-5.2 2/3] ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers

[Cédric Le Goater]

Some more comments, because I still think there are some shortcuts.

My feeling is that all the kvmppc_xive* could be part of a QOM interface
defining how to use a kernel device backend. When the kernel IRQ device 
is not available, under TCG or under an hypervisor not advertising 
support at the KVM level, the QOM interface kernel device backend 
would be a no-op, else it would implement what the kvmppc_xive_* do
today. So, it's something we would change like ->intc after an interrupt 
mode has been negotiated. 

It's an intuition regarding POWER10/XIVE2 and nested support which 
could need a different interface of the KVM XIVE device in the future. 
I don't think we need today but it would clarify some of the shortcuts. 
  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define spapr_xive_in_kernel(xive) \
> +    (kvm_irqchip_in_kernel() && (xive)->fd != -1)
> +

Here, we have a shortcut. kvm_irqchip_in_kernel() is a compilation 
trick but the real handler :

        {
                return SPAPR_XIVE(xrtr)->fd != -1;
        }

is a shortcut. We are using ->fd to know if QEMU is connected with 
a KVM device or not.


>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
>  {
>      XiveSource *xsrc = &xive->source;
>      int i;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_synchronize_state(xive, &local_err);

With a QOM interface for a kernel device backend, this would become :

        XIVE_BACKEND_GET_CLASS(xive->backend)->synchronize_state(xive);

and we could drop all the 'if' statement.



Makes sense ? I think XICS behaves the same.

Thanks,

C.