Re: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Paolo Bonzini]

On 30/11/2015 15:38, Cornelia Huck wrote:
> It obviously
> requires an irqchip; but if you need some configuration/enablement
> beforehand, you'll get different values depending on when you retrieve
> the cap. So does KVM_CAP_IRQFD mean "irqfds are available in principle"
> or "everything has been setup for usage of irqfds"? I'd assume the
> former.

It should be the former, yes.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Pavel Fedin]

 Hello!

> >  b) I simply drop it as it is, because current qemu knows about the 
> > dependency and does not
> try to use irqfd without irqchip,
> > because there's simply no use for them. But, well, perhaps there would be 
> > an exception in
> vhost, i don't remember testing it.
> 
> Wouldn't an irqfd emulation cover vhost?

 I've just tested, and no, it does not cause any problems with qemu. It happens 
to correctly detect that the whole thing is not
running and falls back to not using vhost. This is output from my qemu:
--- cut ---
2015-12-01T11:03:16.135724Z qemu-system-arm: Error binding guest notifier: 11
2015-12-01T11:03:16.135849Z qemu-system-arm: unable to start vhost net: 11: 
falling back on userspace virtio
--- cut ---

 So, the resume is: we just drop this patch and only N1 remains.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


> -Original Message-
> From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf 
> Of Cornelia Huck
> Sent: Monday, November 30, 2015 5:38 PM
> To: Pavel Fedin
> Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org; 'Marc Zyngier'; 
> 'Christoffer Dall';
> 'Gleb Natapov'; 'Paolo Bonzini'
> Subject: Re: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on 
> KVM_CAP_IRQCHIP
> 
> On Mon, 30 Nov 2015 15:41:20 +0300
> Pavel Fedin <p.fe...@samsung.com> wrote:
> 
> >  Hello!
> >
> > > >  Thank you for the note, i didn't know about irqchip-specific 
> > > > capability codes. There's
> the
> > > > same issue with PowerPC, now i
> > > > understand why there's no KVM_CAP_IRQCHIP for them. Because they have 
> > > > KVM_CAP_IRQ_MPIC
> and
> > > > KVM_CAP_IRQ_XICS, similar to S390.
> > > >  But isn't it just weird? I understand that perhaps we have some real 
> > > > need to
> distinguish
> > > > between different irqchip types, but
> > > > shouldn't the kernel also publish KVM_CAP_IRQCHIP, which stands just 
> > > > for "we support
> some
> > > > irqchip virtualization"?
> > > >  May be we should just add this for PowerPC and S390, to make things 
> > > > less ambiguous?
> > >
> > > Note that we explicitly need to _enable_ the s390 cap (for
> > > compatibility). I'd need to recall the exact details but I came to the
> > > conclusion back than that I could not simply enable KVM_CAP_IRQCHIP for
> > > s390 (and current qemu would fail to enable the s390 cap if we started
> > > advertising KVM_CAP_IRQCHIP now).
> >
> >  OMG... I've looked at the code, what a mess...
> >  If i was implementing this, i'd simply introduce kvm_vm_enable_cap(s, 
> > KVM_CAP_IRQCHIP, 0),
> > which would be allowed to fail with -ENOSYS, so that backwards 
> > compatibility is kept and an
> existing API is reused... But, well,
> > it's already impossible to unscramble an egg... :)
> >  Ok, i think in current situation we could choose one of these ways (both 
> > are based on the
> fact that it's obvious that irqfd require
> > IRQCHIP).
> >  a) I look for an alternate way to report KVM_CAP_IRQFD dynamically, and 
> > maybe PowerPC and
> S390 follow this way.
> 
> The thing is: _when_ can you report KVM_CAP_IRQFD? It obviously
> requires an irqchip; but if you need some configuration/enablement
> beforehand, you'll get different values depending on when you retrieve
> the cap. So does KVM_CAP_IRQFD mean "irqfds are available in principle"
> or "everything has been setup for usage of irqfds"? I'd assume the
> former.
> 
> >  b) I simply drop it as it is, because current qemu knows about the 
> > dependency and does not
> try to use irqfd without irqchip,
> > because there's simply no use for them. But, well, perhaps there would be 
> > an exception in
> vhost, i don't remember testing it.
> 
> Wouldn't an irqfd emulation cover vhost?
> 
> >  So what shall we do?
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Pavel Fedin]

 Hello!

> > case KVM_CAP_INTERNAL_ERROR_DATA:
> >  #ifdef CONFIG_HAVE_KVM_MSI
> > case KVM_CAP_SIGNAL_MSI:
> > +   /* Fallthrough */
> >  #endif
> > +   case KVM_CAP_CHECK_EXTENSION_VM:
> > +   return 1;
> >  #ifdef CONFIG_HAVE_KVM_IRQFD
> > case KVM_CAP_IRQFD:
> > case KVM_CAP_IRQFD_RESAMPLE:
> > +   return kvm_vm_ioctl_check_extension(kvm, KVM_CAP_IRQCHIP);
> 
> This won't work for s390, as it doesn't have KVM_CAP_IRQCHIP but
> KVM_CAP_S390_IRQCHIP (which needs to be enabled).

 Thank you for the note, i didn't know about irqchip-specific capability codes. 
There's the same issue with PowerPC, now i
understand why there's no KVM_CAP_IRQCHIP for them. Because they have 
KVM_CAP_IRQ_MPIC and KVM_CAP_IRQ_XICS, similar to S390.
 But isn't it just weird? I understand that perhaps we have some real need to 
distinguish between different irqchip types, but
shouldn't the kernel also publish KVM_CAP_IRQCHIP, which stands just for "we 
support some irqchip virtualization"?
 May be we should just add this for PowerPC and S390, to make things less 
ambiguous?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Cornelia Huck]

On Mon, 30 Nov 2015 14:56:38 +0300
Pavel Fedin  wrote:

>  Hello!
> 
> > >   case KVM_CAP_INTERNAL_ERROR_DATA:
> > >  #ifdef CONFIG_HAVE_KVM_MSI
> > >   case KVM_CAP_SIGNAL_MSI:
> > > + /* Fallthrough */
> > >  #endif
> > > + case KVM_CAP_CHECK_EXTENSION_VM:
> > > + return 1;
> > >  #ifdef CONFIG_HAVE_KVM_IRQFD
> > >   case KVM_CAP_IRQFD:
> > >   case KVM_CAP_IRQFD_RESAMPLE:
> > > + return kvm_vm_ioctl_check_extension(kvm, KVM_CAP_IRQCHIP);
> > 
> > This won't work for s390, as it doesn't have KVM_CAP_IRQCHIP but
> > KVM_CAP_S390_IRQCHIP (which needs to be enabled).
> 
>  Thank you for the note, i didn't know about irqchip-specific capability 
> codes. There's the same issue with PowerPC, now i
> understand why there's no KVM_CAP_IRQCHIP for them. Because they have 
> KVM_CAP_IRQ_MPIC and KVM_CAP_IRQ_XICS, similar to S390.
>  But isn't it just weird? I understand that perhaps we have some real need to 
> distinguish between different irqchip types, but
> shouldn't the kernel also publish KVM_CAP_IRQCHIP, which stands just for "we 
> support some irqchip virtualization"?
>  May be we should just add this for PowerPC and S390, to make things less 
> ambiguous?

Note that we explicitly need to _enable_ the s390 cap (for
compatibility). I'd need to recall the exact details but I came to the
conclusion back than that I could not simply enable KVM_CAP_IRQCHIP for
s390 (and current qemu would fail to enable the s390 cap if we started
advertising KVM_CAP_IRQCHIP now).

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Cornelia Huck]

On Mon, 30 Nov 2015 12:40:45 +0300
Pavel Fedin  wrote:

> Now at least ARM is able to determine whether the machine has
> virtualization support for irqchip or not at runtime. Obviously,
> irqfd requires irqchip.
> 
> Signed-off-by: Pavel Fedin 
> ---
>  virt/kvm/kvm_main.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 7873d6d..a057d5e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2716,13 +2716,15 @@ static long 
> kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
>   case KVM_CAP_INTERNAL_ERROR_DATA:
>  #ifdef CONFIG_HAVE_KVM_MSI
>   case KVM_CAP_SIGNAL_MSI:
> + /* Fallthrough */
>  #endif
> + case KVM_CAP_CHECK_EXTENSION_VM:
> + return 1;
>  #ifdef CONFIG_HAVE_KVM_IRQFD
>   case KVM_CAP_IRQFD:
>   case KVM_CAP_IRQFD_RESAMPLE:
> + return kvm_vm_ioctl_check_extension(kvm, KVM_CAP_IRQCHIP);

This won't work for s390, as it doesn't have KVM_CAP_IRQCHIP but
KVM_CAP_S390_IRQCHIP (which needs to be enabled).

>  #endif
> - case KVM_CAP_CHECK_EXTENSION_VM:
> - return 1;
>  #ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
>   case KVM_CAP_IRQ_ROUTING:
>   return KVM_MAX_IRQ_ROUTES;

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Pavel Fedin]

Now at least ARM is able to determine whether the machine has
virtualization support for irqchip or not at runtime. Obviously,
irqfd requires irqchip.

Signed-off-by: Pavel Fedin 
---
 virt/kvm/kvm_main.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7873d6d..a057d5e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2716,13 +2716,15 @@ static long kvm_vm_ioctl_check_extension_generic(struct 
kvm *kvm, long arg)
case KVM_CAP_INTERNAL_ERROR_DATA:
 #ifdef CONFIG_HAVE_KVM_MSI
case KVM_CAP_SIGNAL_MSI:
+   /* Fallthrough */
 #endif
+   case KVM_CAP_CHECK_EXTENSION_VM:
+   return 1;
 #ifdef CONFIG_HAVE_KVM_IRQFD
case KVM_CAP_IRQFD:
case KVM_CAP_IRQFD_RESAMPLE:
+   return kvm_vm_ioctl_check_extension(kvm, KVM_CAP_IRQCHIP);
 #endif
-   case KVM_CAP_CHECK_EXTENSION_VM:
-   return 1;
 #ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
case KVM_CAP_IRQ_ROUTING:
return KVM_MAX_IRQ_ROUTES;
-- 
2.4.4

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Pavel Fedin]

 Hello!

> >  Thank you for the note, i didn't know about irqchip-specific capability 
> > codes. There's the
> > same issue with PowerPC, now i
> > understand why there's no KVM_CAP_IRQCHIP for them. Because they have 
> > KVM_CAP_IRQ_MPIC and
> > KVM_CAP_IRQ_XICS, similar to S390.
> >  But isn't it just weird? I understand that perhaps we have some real need 
> > to distinguish
> > between different irqchip types, but
> > shouldn't the kernel also publish KVM_CAP_IRQCHIP, which stands just for 
> > "we support some
> > irqchip virtualization"?
> >  May be we should just add this for PowerPC and S390, to make things less 
> > ambiguous?
> 
> Note that we explicitly need to _enable_ the s390 cap (for
> compatibility). I'd need to recall the exact details but I came to the
> conclusion back than that I could not simply enable KVM_CAP_IRQCHIP for
> s390 (and current qemu would fail to enable the s390 cap if we started
> advertising KVM_CAP_IRQCHIP now).

 OMG... I've looked at the code, what a mess...
 If i was implementing this, i'd simply introduce kvm_vm_enable_cap(s, 
KVM_CAP_IRQCHIP, 0),
which would be allowed to fail with -ENOSYS, so that backwards compatibility is 
kept and an existing API is reused... But, well,
it's already impossible to unscramble an egg... :)
 Ok, i think in current situation we could choose one of these ways (both are 
based on the fact that it's obvious that irqfd require
IRQCHIP).
 a) I look for an alternate way to report KVM_CAP_IRQFD dynamically, and maybe 
PowerPC and S390 follow this way.
 b) I simply drop it as it is, because current qemu knows about the dependency 
and does not try to use irqfd without irqchip,
because there's simply no use for them. But, well, perhaps there would be an 
exception in vhost, i don't remember testing it.
 So what shall we do?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Cornelia Huck]

On Mon, 30 Nov 2015 15:41:20 +0300
Pavel Fedin  wrote:

>  Hello!
> 
> > >  Thank you for the note, i didn't know about irqchip-specific capability 
> > > codes. There's the
> > > same issue with PowerPC, now i
> > > understand why there's no KVM_CAP_IRQCHIP for them. Because they have 
> > > KVM_CAP_IRQ_MPIC and
> > > KVM_CAP_IRQ_XICS, similar to S390.
> > >  But isn't it just weird? I understand that perhaps we have some real 
> > > need to distinguish
> > > between different irqchip types, but
> > > shouldn't the kernel also publish KVM_CAP_IRQCHIP, which stands just for 
> > > "we support some
> > > irqchip virtualization"?
> > >  May be we should just add this for PowerPC and S390, to make things less 
> > > ambiguous?
> > 
> > Note that we explicitly need to _enable_ the s390 cap (for
> > compatibility). I'd need to recall the exact details but I came to the
> > conclusion back than that I could not simply enable KVM_CAP_IRQCHIP for
> > s390 (and current qemu would fail to enable the s390 cap if we started
> > advertising KVM_CAP_IRQCHIP now).
> 
>  OMG... I've looked at the code, what a mess...
>  If i was implementing this, i'd simply introduce kvm_vm_enable_cap(s, 
> KVM_CAP_IRQCHIP, 0),
> which would be allowed to fail with -ENOSYS, so that backwards compatibility 
> is kept and an existing API is reused... But, well,
> it's already impossible to unscramble an egg... :)
>  Ok, i think in current situation we could choose one of these ways (both are 
> based on the fact that it's obvious that irqfd require
> IRQCHIP).
>  a) I look for an alternate way to report KVM_CAP_IRQFD dynamically, and 
> maybe PowerPC and S390 follow this way.

The thing is: _when_ can you report KVM_CAP_IRQFD? It obviously
requires an irqchip; but if you need some configuration/enablement
beforehand, you'll get different values depending on when you retrieve
the cap. So does KVM_CAP_IRQFD mean "irqfds are available in principle"
or "everything has been setup for usage of irqfds"? I'd assume the
former.

>  b) I simply drop it as it is, because current qemu knows about the 
> dependency and does not try to use irqfd without irqchip,
> because there's simply no use for them. But, well, perhaps there would be an 
> exception in vhost, i don't remember testing it.

Wouldn't an irqfd emulation cover vhost?

>  So what shall we do?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 2/2] KVM: Make KVM_CAP_IRQFD dependent on KVM_CAP_IRQCHIP

[Pavel Fedin]

 Hello!

> >  b) I simply drop it as it is, because current qemu knows about the 
> > dependency and does not
> try to use irqfd without irqchip,
> > because there's simply no use for them. But, well, perhaps there would be 
> > an exception in
> vhost, i don't remember testing it.
> 
> Wouldn't an irqfd emulation cover vhost?

 Of course it would. At least it should, but perhaps will need some minor 
tweaks.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html